Refuse a job code call with no step to put the result in - #676
Merged
Merged
Conversation
Ask the global assistant to change a step's code and sometimes nothing changes, with a reply that talks as though something did. A job_key that names no step already errors before any code is written. The silent case is a call with no job_key at all, which the schema allowed: the description says to set it, but required listed only message and attachments. Nothing matches, the subagent still runs, writes code, and the planner drops it because there is nowhere to stitch it. job_key is now required, so the model cannot omit it, and both execution paths refuse before spending the subagent call rather than after. The parallel path had the same hole as the single one. The errors now name the workflow's actual step keys. "Retry with the exact job key" asks the planner to fix the thing it just got wrong with no more information than it had the first time; naming them lets it correct itself in the same turn. Made job_key required rather than an enum of the real keys on purpose. An enum would make the mistake impossible, but the tools block sits in the cached prefix alongside a system prompt, so a schema that varies per workflow would re-cache both every time. One existing test asserted the old behaviour, that a keyless call generates code and reports it was not stitched. That documents the outcome rather than arguing for it, and it is the outcome OpenFn/lightning#5133 calls a bug, so it now asserts the subagent is never called.
7 tasks
elias-ba
added a commit
to OpenFn/lightning
that referenced
this pull request
Sep 8, 2026
The 5133 half only covered the planner. Apollo's router is the shortcut for a single-step code edit, which is the common case, and it discarded job_chat's diff — so the reply this was written for still said nothing. Fixed upstream in OpenFn/apollo#676, which reports it in the same shape the planner does. The changelog now names the Apollo version this needs rather than claiming it outright. Two bugs of mine. The Try again on a failed edit was gated on being in the last two messages, but a retry appends one reply, so [prompt, flagged reply, new reply] brought the stale notice's button back and every click started another job. It is gated on being the last message now. And the flag was set whenever any subagent call applied no patches, so a turn where one edit failed and another landed put "couldn't apply the change" on a reply carrying the change; it now also requires that nothing came back to apply. The join reply put "validation_error" in `reason`, which the client shows to the reader as it stands, so the fix for a crash would have displayed a code. The field errors become a sentence. The comment claiming this matched new_message's shape was wrong: those replies use `type` and `errors`, and a join reply uses `reason`. The counter was replacing the AI disclaimer rather than sitting beside it, so the disclaimer vanished from 9,500 characters on. Four test gaps, three of which let the above through. Nothing covered the channel serializer, so hardcoding the flag to false broke the feature end to end with the suite green. Nothing covered a turn with one failed and one applied edit. Nothing covered exactly 10,000 characters, which the server accepts and an off-by-one would have blocked. And the retry test used a shape a retry never produces.
… malformed YAML raising The router is the shortcut Apollo takes for a single-step code edit, so it is the common case, and it threw job_chat's diff away. A client could not tell a reply that changed nothing from one that changed something on the route most edits take. It now reports it in the same shape the planner does, so there is one place to look. The YAML helpers assumed a mapping of jobs with string keys. The workflow is a client payload, so a root that parses to a list or a string, a `jobs:` that is a list, or a numeric job key each raised — and the only handler above turns that into a 500 for the whole turn. Both helpers now fail closed to "not found". Also: the "not found" branch keyed off a falsy job body rather than a missing key, so a step that exists with no value was refused and then named in the error as though it did not exist.
2 tasks
# Conflicts: # services/yaml_utils.py
elias-ba
added a commit
to OpenFn/lightning
that referenced
this pull request
Sep 9, 2026
* Answer a too-long first message instead of dropping the connection Starting a chat with a message over the limit put a raw changeset in the join reply. A changeset has no JSON encoder, so encoding raised and the socket died before any reply went out: from the panel the assistant simply did nothing. The reply now carries the same formatted errors new_message has always returned. The trigger is a long paste, so the box also shows the limit before you hit it. Nothing until you are within five hundred characters, then a counter, then send is refused with the count in red. A counter that is always on reads as a warning about a limit almost nobody meets. Refusing is still the wrong answer; the right one is to take the paste as an attachment the way Claude and Slack do, which is #5138. Closes #4883 * Say when the assistant could not apply the code change Ask for a code change, get a reply with nothing to apply and no explanation. It reads as the assistant deciding not to help. Apollo's job-code subagent reports how many of its edits landed, and zero means it tried and could not. That count travels to us already: the subagent's whole result goes into the planner's subagent_results, which global chat spreads into meta, which build_global_message already reads. We were ignoring it. The reply itself succeeded, so this is not a failure of the message and does not belong in the failure columns. It is a flag on the message meta, alongside from_global and apply_failed, and the panel renders the same notice a failure gets. Try again is offered, and only on the newest exchange. Every way a patch fails is a property of what the model generated that turn - an anchor that is not in the code, an ambiguous one, a missing field - and Apollo has already tried once to correct itself, so another go can genuinely work. But a retry appends a new reply and leaves this one saying the same thing for good, so an older exchange would keep a button that starts a fresh job. Apollo's own warning stays in the log. One of its two sources is `f"Failed to apply edit: {str(e)}"`, which carries container paths. One case this does not cover: code generated for a job key that matched nothing, so it never reached the workflow. Apollo knows, and writes it into the model's tool result, but does not put it in the response. Closes #5133 * Act on the review of both fixes The 5133 half only covered the planner. Apollo's router is the shortcut for a single-step code edit, which is the common case, and it discarded job_chat's diff — so the reply this was written for still said nothing. Fixed upstream in OpenFn/apollo#676, which reports it in the same shape the planner does. The changelog now names the Apollo version this needs rather than claiming it outright. Two bugs of mine. The Try again on a failed edit was gated on being in the last two messages, but a retry appends one reply, so [prompt, flagged reply, new reply] brought the stale notice's button back and every click started another job. It is gated on being the last message now. And the flag was set whenever any subagent call applied no patches, so a turn where one edit failed and another landed put "couldn't apply the change" on a reply carrying the change; it now also requires that nothing came back to apply. The join reply put "validation_error" in `reason`, which the client shows to the reader as it stands, so the fix for a crash would have displayed a code. The field errors become a sentence. The comment claiming this matched new_message's shape was wrong: those replies use `type` and `errors`, and a join reply uses `reason`. The counter was replacing the AI disclaimer rather than sitting beside it, so the disclaimer vanished from 9,500 characters on. Four test gaps, three of which let the above through. Nothing covered the channel serializer, so hardcoding the flag to false broke the feature end to end with the suite green. Nothing covered a turn with one failed and one applied edit. Nothing covered exactly 10,000 characters, which the server accepts and an off-by-one would have blocked. And the retry test used a shape a retry never produces. * Cut the commentary back 53 added comment lines to 18. Two of them said the same thing about the retry gate, seven lines apart, because a later edit stacked on an earlier one. The test comments repeated the production ones almost verbatim.
hanna-paasivirta
approved these changes
Sep 9, 2026
hanna-paasivirta
left a comment
hanna-paasivirta
left a comment
Contributor
There was a problem hiding this comment.
This is great, thank you Elias! I removed some dead code that the tests were using, and verified the tests still pass. I also fixed the count shared with Lightning on applied changes to be 0 when changes fail.
Collaborator
Author
|
Thank you very much Hanna |
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
2 participants
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
Two ways a code change vanished with no way for the caller to say so: a job-code call with no
job_key, and the router's direct route discarding whether the edits landed.Fixes #677
Implementation Details
The job-code tool allowed a call with no
job_key, so nothing matched, the subagent still ran, and the code it wrote was dropped. It is required now, both execution paths refuse before spending the call, and a key matching nothing is answered with the workflow's real step keys so the planner can correct itself in the same turn.Required rather than an enum of those keys because the tools block sits in the cached prefix: a per-workflow schema would re-cache it and the system prompt behind it every turn.
The router's direct route discarded job_chat's
diff. It is the shortcut for single-step edits, so it was the case where a client could least tell a reply that changed something from one that changed nothing. It now reports it in the planner's shape; OpenFn/lightning#5143 surfaces it. The YAML helpers also no longer raise on a workflow that is not a mapping of jobs.pytest services/global_chat/tests/unit/ -q— 124 pass. The acceptance suite needs a live key and is too flaky to read from.AI Usage
Please disclose whether you've used AI in this work (it's cool, we just want to know!):
You can read more details in our Responsible AI Policy