fix: address HIGH and MEDIUM findings from the #19 reviews - #21
Merged
Merged
Conversation
maybe_escalate gated only on delegation_verdict != "failed", but that verdict is "failed" for ANY non-success terminal status, so a CANCELLED job (explicit user intent to stop) or a TIMED_OUT job was silently re-dispatched to a larger, costlier peer. Gate on JobState.SUCCEEDED first so only a declared-gate failure (check / scope / verify) escalates; a delegate that did not finish cleanly does not. TIMED_OUT does not escalate: it produced no graded artifact and a bigger, slower peer is at least as likely to time out again. Also guard the child-staging writes (create_job_dir, prompt/command writes, save_state) against OSError so the module's "never raises" contract holds on a read-only or full disk.
tempfile.mkstemp() ran before run_verification's try block, so a read-only /tmp, a full disk, or a restricted TMPDIR raised OSError out of the verifier — which both docstrings promise never happens — past the worker's unguarded caller, leaving the job non-terminal forever after the check and scope gates already ran and destroying the delegate's real work. Extract the schema file into a _schema_file context manager that degrades to non-structured mode when the temp file cannot be created and always unlinks on exit, so verification never propagates OSError. This also trims run_verification back under the 50-line guideline.
Credential scrubbing was wired into the durable-job worker but not the default
`crossagent --agent ... --prompt ...` invocation: _run_advisor ran the advisor
subprocess with no env=, so it inherited the caller's full unscrubbed
os.environ, and --pass-env was registered only for `start`. The security claim
("credential withholding for delegates") therefore exceeded the implementation
for the original dispatch mode.
Scrub via the same credentials.scrub_env helper the worker uses (one shared
policy, no drift) and add the --pass-env escape hatch to the foreground parser.
…isk staging from maybe_escalate
Relocates CheckResultDict/ScopeResultDict/ScopeStatus/VerifyResultDict/VerifyVerdict out of jobs.py into a new dependency-free types.py. This removes the inverted back-import where the gate producers (check/scope/verify) imported their own persisted-record shapes from their consumer (jobs). jobs re-exports the three it uses as Job field annotations for jobs_mod.* callers (worker). Behaviour unchanged; 513 tests green.
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
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.
Fixes every finding from the two reviews on #19. Stacked on
feat/delegation-scope, so merging this lands them in #19.HIGH (3)
maybe_escalategated ondelegation_verdict != "failed", but that returns"failed"for any non-SUCCEEDEDterminal, socrossagent cancelsilently re-dispatched the task to a larger, costlier advisorescalate.pynow gates onSUCCEEDEDfirst: only a declared gate failure (check/scope/verify) escalatestempfile.mkstemp()outside its owntrybroke the documented "never raises" contract. On a read-only/tmpor full disk theOSErrorescaped intoworker_mainafter the check and scope ran but before the terminal record was persisted — job wedged non-terminal forever, completed work discardedverify.pyextracts a_schema_file()context manager with setup in its owntry/except; failure degrades to an error outcome. SameOSErrorhardening applied toescalate.pycli.py's_run_advisorcalledrunner.run(...)with noenv=, socrossagent --agent … --prompt …handed the advisor the full unscrubbed environment, with no--pass-envescape hatch on that pathcli.pynow passes a scrubbedenv=, sharing one policy with the job pathTIMED_OUTwas decided deliberately, not inherited.CANCELLEDis unambiguous — explicit user intent, never escalate.TIMED_OUTcut both ways; the call was do not escalate, because a timeout yields no gate verdict and a larger model is typically slower, so escalating tends to burn budget timing out again. Reasoning is in-code atescalate.py:122-133so a future reader can disagree with the decision rather than rediscover the behaviour.MEDIUM (4)
objandinforenamed.run_verification71 → under 50 lines (_interpret_run_outcomeextracted).maybe_escalate135 → 94 lines (eligibility guard, child-Jobbuild, and disk staging extracted). Still above the 50-line guideline — see Known gaps.types.pyholds the shared gate-result shapes;jobs.py1061 → 985. The review suggested moving these intoscope.py/verify.py, but those already import both the types andJobfromjobs.py, so that direction creates a circular import — a neutral module is the clean route and it also removes the awkward back-import.Verification
checkandformat --checkgreen.Known gaps (deliberate)
maybe_escalateis still 94 lines. Further splitting would have produced helpers with long parameter lists called once — worse than a linear function that reads cleanly.jobs.py(985) andcli.py(1021) remain over the 800-line guideline. Both predate this work; decomposing them is out of scope for a fix PR on a branch already under review..git/hooks/*writes and out-of-repo writes both reporting scopeok— are not addressed here. They are gaps in enumeration rather than defects in the matcher, and warrant their own change.