Redact a job body wherever it sits, and stop three crashes - #674
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.
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.
| except Exception: | ||
| pass | ||
| return yaml_str | ||
| return WITHHELD_NOTICE |
There was a problem hiding this comment.
Why are we withholding the workflow entirely? It might cause confusing behaviour if the model can't see the structure at all. It won't be able to inspect_job_code either if it doesn't know the key names.
There was a problem hiding this comment.
Good question, and the worry is fair. The reason it's the whole document is that if the parse failed then I haven't redacted anything, and the bodies are still sitting there in the text, so handing it back gives the model the code rather than protecting it. One bad indent in agent output was enough to do that on main.
It fires more rarely than it reads, though. I ran both versions over Lightning's real project exports and it never withheld one, and in the cases where it does fire there was no structure to show anyway.
You're right about inspect_job_code. #676 helps there, since a key that matches nothing now comes back with the real key names. I've also split the notice in two, so it at least says which of the two failures happened.
There was a problem hiding this comment.
The job code doesn't need to be protected, we hold it back as lazy loading for saving tokens. If we redact the entire workflow, the model might say "I can't see your workflow" while if we give it, it could say "there's an indentation issue after step x".
There was a problem hiding this comment.
Apologies Hanna, I completely misunderstood here. I read this as a privacy control, so I kept arguing about a leak that isn't one. inspect_job_code hands over any body on request, so there was never anything to keep from the model in the first place.
I've taken the withholding out. A document we can't parse now comes back as it came, so the model can say what's wrong with the YAML. What's left is the nested-jobs walk, which is the token saving you're describing and was being missed on project exports, plus the three crash fixes.
Thanks for pushing on it twice.
A document that cannot be parsed and one whose shape we cannot redact both returned the same line, so the planner could not tell a workflow it could have read from one that was never valid.
# Conflicts: # services/workflow_chat/workflow_chat.py
Job bodies are deferred to save tokens, not kept from the model: it reads any of them with inspect_job_code, and subagents get the whole YAML anyway. So withholding a document it cannot parse only costs it the structure, when it could have said what was wrong with it.
elias-ba
changed the title
2 participants
Short Description
Two faults in the redaction path. A job body deeper than the top level was sent in full, and preserving components crashed on a shape the model did not expect. The id walk also no longer recurses forever on a YAML anchor cycle.
Part of #446
Implementation Details
Bodies are replaced with a placeholder before the workflow reaches the planner to save tokens, not to keep the code from it: the model reads any body it wants with
inspect_job_code, and subagents are handed the whole YAML anyway. The walk only looked at top-leveljobs, so a project export, whose jobs nest under each workflow, sent every body through in full. On Lightning's own fixtures that is twelve bodies per 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. It shares one guard with the body walk now.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.A document we cannot parse comes back as it came. Withholding it would only cost the planner the structure, when it could tell the user what is wrong with the YAML instead.
Split out of #660. 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