Withhold a workflow we cannot redact, and stop three crashes - #674
Open
elias-ba wants to merge 3 commits into
Open
Withhold a workflow we cannot redact, and stop three crashes#674elias-ba wants to merge 3 commits into
elias-ba wants to merge 3 commits into
Conversation
Three faults in the redaction path, none of which depend on what we decide about job code in Sentry. redact_job_bodies returned the original document whenever anything went wrong, so a workflow it could not parse, or one whose jobs sit deeper than the top level, reached the model with every body intact. It now withholds the document instead. The id walk recursed without tracking what it had entered. A YAML anchor can point at its own container and PyYAML builds that as a real cycle, so such a workflow raised RecursionError, which the bare except then swallowed into the same unredacted fallback. extract_and_preserve_components assumed jobs, triggers and edges were mappings and that a body was a string. A list of jobs, a numeric body or a null entry raised AttributeError or TypeError out of a normal chat request. Split out of #660.
2 tasks
Review found the first pass both too eager and not eager enough. Too eager: a workflow with no job bodies at all, a trigger-only one, was withheld whole. It had nothing to hide, so the planner lost its structure for nothing. A document is now withheld only when it cannot be parsed, or when it does not look like a workflow at all. Not eager enough: redaction only looked at jobs.<key>.body, so a body nested under a workflow in a project export, a job written as a list, or one pulled in through a merge key all reached the model intact. The walk now finds a body wherever it sits. Two more of the same shape, found in the same review. The read-only id strip in workflow_chat kept its own copy of the walker, so it still recursed forever on an anchor cycle and then returned the document with the ids in it; it now calls the guarded one. And workflow_has_job_code treated a scalar document as a mapping and raised TypeError. Known limit: code that appears under some other key and is aliased into a body is redacted in the body and left alone elsewhere. That matches main.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Short Description
Three faults in the redaction path. A workflow we cannot redact is now withheld rather than passed through, the id walk no longer recurses forever on a YAML anchor cycle, and preserving components tolerates a shape it did not expect.
Part of #446
Implementation Details
redact_job_bodiesreturned the original document whenever anything went wrong. A workflow it could not parse, or one whose jobs sit deeper than the top level, reached the model with every body intact. That is the opposite of what redaction is for, so it now withholds the document.The id walk recursed without tracking what it had entered. A YAML anchor can point at its own container and PyYAML builds that as a real cycle, so such a workflow raised
RecursionError. The bareexceptthen swallowed it into the same unredacted fallback, so the two faults compounded.extract_and_preserve_componentsassumed jobs, triggers and edges were mappings, and that a body was a string. A list of jobs, a numeric body or a null entry raisedAttributeErrororTypeErrorout of an ordinary chat request.Split out of #660. None of this depends on the question about job code in Sentry, which is why it is here rather than there. Each fix has a test, and each test was checked by breaking the fix again.
AI Usage
You can read more details in our
Responsible AI Policy