Repository navigation
Conversation
…nce turn Replace string matching between the worker's command claims and the runtime transcript with an evidence turn. After a worker's turn completes, the controller resumes the same session for one tool-less turn (a durable dispatch, dispatch_kind "evidence_turn") that shows the worker the controller's own numbered record of its shell calls: the recorded command and the recorded result (Codex: the integer exit status; Claude: the structured is_error bit, never the result text), ordered by structural features only with weights in one config object. The worker answers with call numbers per evidence field and the controller reads its own records. - unknown number, or a failed call cited under tests_passed: fabrication - a later run of the same recorded command that did not pass, an unknown result, not a verification invocation, outside the workspace, a failed replay on the final artifact, or a relevance veto: withheld (no evidence, CITED_EVIDENCE_WITHHELD, accepted unverified) - a cited script no longer in the artifact: not replayed (as #2513) - the relevance check is a model judgement that may only withhold (execution.evidence_relevance_veto, on by default) - acceptances are labelled accepted_by (check_package, transcript_evidence, command_strings, verify_command, no_evidence) in the reconciliation record and the new acceptance_basis telemetry event - records without an evidence turn keep the command-string path, and the verdict records which path decided Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Metadata
| Field | Value |
|---|---|
| PR | #2519 |
| HEAD checked | fcaf0983d8812f4e96cb34d8031a70d37c3777df |
| Request ID | req_1790962234_61 |
| Review record | 2851ccd4-ff2b-47b9-905c-2890342ea2f6 |
What Improved
- The verifier can resolve numbered citations against recorded shell calls, covering command shapes that the string matcher rejected. The PR also records evidence-path and acceptance-basis labels.
Issue Requirements
| Requirement | Status |
|---|---|
| No linked issue requirement captured | N/A |
| Resolve worker evidence by numbered, controller-recorded calls | Implemented in the reviewed path |
| Classify unknown and failed citations, withhold unproven citations, and preserve the legacy path where the turn cannot be used | Implemented in the reviewed path |
| Order calls structurally, apply a relevance veto, and label the acceptance basis | Implemented; raw-command egress requires correction |
| Keep the evidence turn as a durable follow-up dispatch | Implemented; focused dispatch tests passed |
Prior Findings Status
No prior ouroboros-agent review rounds or human review comments were present.
Blockers
| # | File:Line | Severity | Finding |
|---|---|---|---|
| 1 | src/ouroboros/orchestrator/evidence/relevance_veto.py:91 | BLOCKING | With the default relevance check enabled, every cited passing call is copied into a separate model request as its raw command. A call such as API_TOKEN=<secret> pytest -q therefore sends the credential value to the relevance provider. The evidence-turn prompt also copies raw commands. This is an immediate credential-egress risk introduced on the promised shell-call path. Sanitize sensitive command values before either prompt is constructed. |
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| 1 | PR body | Low | High | Add the required Refs #… link or use the repository’s documented exemption. The body mentions #2513 for prior behavior but does not identify a linked issue for this PR. Owner: contributor. |
Non-blocking Suggestions
None.
Test Coverage Notes
- The focused citation, evidence-turn, and dispatch tests passed: 60 tests. The PR reports passing lint, formatting, mypy, and module-size checks; its reported unit run had 66 failures attributed to the environment. I did not reproduce the full suite.
Design Notes
The root-cause class is unsanitized command data crossing a model boundary. The evidence layer should produce one safe command representation before prompt construction; migrate both render_call_list and LLMRelevanceJudge to it while keeping the recorded command available for controller-side verification.
Design / Roadmap Gate
The PR body defines the user problem, citation rules, eligible runtime conditions, affected verifier and orchestrator modules, known exclusions, and focused verification. Shell commands are supported inputs to the new turn. Sending a credential-bearing command to the relevance model reproduces under those conditions and creates an immediate security risk. The correction stays within the existing evidence subsystem; no new owner or subsystem is required.
Directional Notes
The security-memory cue directed inspection of shell-call egress. The finding rests on the current raw-command formatting and model completion call, not on memory. The PR’s stated legacy fallback and acknowledged resume limits were evaluated within its declared boundary.
Convergence Gate
-
Decision:
CONTINUE -
Review round: 1
-
Subsystem budget: 11 —
CHANGELOG.md,TELEMETRY.md,src/ouroboros/boundary,src/ouroboros/config,src/ouroboros/evolution,src/ouroboros/orchestrator,src/ouroboros/telemetry.py,tests/unit/boundary,tests/unit/evolution,tests/unit/orchestrator,tests/unit/test_telemetry.py -
Blocking findings: 1 (0 repeated)
-
Root-cause hypothesis: Raw recorded commands reach model prompts without sensitive-value sanitization.
-
Minimal reproduction: Supply a passing recorded call with
API_TOKEN=<sentinel> pytest -qto the evidence prompt and a fake relevance adapter; assert that neither captured model request contains<sentinel>. -
Subsystem budget: orchestrator/evidence, orchestrator dispatch
-
Scope expansion: none
-
Blocker classification: #1 INDEPENDENT_DEFECT
-
Stagnation decision: CONTINUE
-
Reset success condition: N/A
-
Reset prohibition: N/A
-
Reset verification command: N/A
-
Effect owner: orchestrator evidence presentation, before model egress
Test Coverage
- The focused citation, evidence-turn, and dispatch tests passed: 60 tests. The PR reports passing lint, formatting, mypy, and module-size checks; its reported unit run had 66 failures attributed to the environment. I did not reproduce the full suite.
Merge Recommendation
Request changes for the demonstrated credential-egress path. The focused tests pass, but they do not cover sensitive values in recorded commands.
Review-Metadata:
verdict: REQUEST_CHANGES
head_sha: fcaf098
request_id: req_1790962234_61
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
lee3Q
left a comment
There was a problem hiding this comment.
CHANGES_REQUESTED. call_ledger.py:192–196 returns PASSED whenever the recorded integer statuses are zero, without considering an explicit structured failure carried by the same correlated completion.
This is reachable through the actual product mapper: an item.completed command-execution envelope with exit_code: 0 and status: failed is normalized by CodexCliRuntime._convert_event to is_error: true, while retaining exit_code: 0 and tool_result.meta.exit_status: 0. The new ledger then labels the call PASSED, and evaluate_citations marks a tests_passed: [1] citation proven. The result text is irrelevant to this reproduction. The runtime resolver deliberately gives the failure channel precedence; the new verifier silently reverses it.
Please fail closed on conflicting structured completion verdicts (or preserve the failure), and add a regression test using the real mapper output. This violates the PR's adopted rule that a failed call cited under tests_passed must be rejected as fabrication. The existing replay policy is supporting context, not a separately invented citation requirement.
I ran the focused cited-evidence, evidence-turn, dispatch, and no-evidence tests: 69 passed. I separately reproduced the mapper → ledger → citation failure at this head. These used existing cached packages rather than an exact lock-file environment. Required CI is a separate merge gate and is not established by these local results.
Reviewed head: fcaf0983d8812f4e96cb34d8031a70d37c3777df.
|
Closing this old-base implementation as a divergent integration proposal, alongside its correction draft #2712. Current main The concrete failure-precedence concern was checked independently on main: top-level and nested structured #2380 remains open for its own acceptance criteria. A future evidence-turn proposal should use that existing canonical context, identify the remaining user need and bounded authority/settings decisions, and preserve current failure semantics. No issue is closed and no live-provider proof is claimed here. |
Problem
The per-criterion verifier matched the command strings a worker wrote in its typed evidence (
commands_run,tests_passed) against the runtime transcript, so every new command shape needed another matching rule (#2513 was one). A diagnosis of 8 falseFABRICATION_SUSPECTEDrejections on officially resolved SWE-bench artifacts (v0.55.4) found no fabrication. 14 claims had actually run and failed only on their shape: a test runner inside a compound call (4), aPYTHONPATH=libprefix (5), a non-final element of a command list (3), a here-document program or-knarrowing (2). The other 2 were worker-record problems: a run whose latest invocation failed, and a lint command cited as a test.A call-ID design was probed first and dropped. Neither worker model can see its own tool-call IDs. On Codex, the ID a hook can show the worker (
exec-<uuid>) is a different namespace from the transcript'sitem_N.Design (owner-approved)
Evidence turn. After the worker's turn completes, the controller resumes the same session for one tool-less turn. This is a durable dispatch:
execution.ac.attempt.dispatchedwithdispatch_kind: "evidence_turn", a prompt digest, and the primary turn sealed as superseded, the same capsule ceremony as a SessionSignal follow-up. The turn shows the worker the controller's own numbered record of its shell calls (evidence/call_ledger.py): call number, recorded result, and recorded command.exit_code/meta.exit_status).is_errorbit;is_error: falsecounts as a pass.The worker answers with call numbers per field. The controller resolves each number to its own record (
evidence/cited_evidence.py). The worker's text is never compared with the transcript.Rules for a cited number:
tests_passed:FABRICATION_SUSPECTED.tests_passedcall whose every verification command runs a script absent from the artifact is not replayed (script_absent_from_artifact, fix(verifier): a recorded run of a deleted scratch script is not fabrication #2513 semantics). A proven citation carries the pass.CITED_EVIDENCE_WITHHELD(UNAVAILABLE/ACCEPT). It flows into the feat(telemetry): count acceptances without evidence and why #2503 reasoncited_evidence_withheld.Relevance check (
evidence/relevance_veto.py,execution.evidence_relevance_veto, default on). One tool-less model completion per criterion. Only an explicitnot_relevantfor a cited passing call withholds it; an error or any other answer changes nothing. Each decision is recorded on the typed-evidence event.Ordering (
evidence/call_ordering.py). The list is ordered by a weighted sum of structural call features only: verification invocation, names a file the final workspace changed, latest of its command, passed, ran after the last edit of those files, and replayable. Ties keep transcript order. Weights live in one config object,execution.evidence_call_ordering(EvidenceCallOrderingWeights), with conservative defaults. Numbers never depend on the order.Labels.
CriterionDecision.accepted_byis one ofcheck_package,transcript_evidence,command_strings,verify_commandorno_evidence. It appears in the reconciliation record and in the newacceptance_basistelemetry event (closed counts, documented in TELEMETRY.md).VerifierVerdict.decided_byrecords which evidence path decided.Legacy path. Records produced without an evidence turn keep the command-string path unchanged: a resumed attempt, a runtime without a capsule-bound resumable session, or a primary turn that failed or paused. The CHANGELOG entry plans the string path for removal in a follow-up.
Code location.
parallel_executor.pystays at 13155 of its 13158-line budget. The new logic lives in new modules:evidence/call_ledger.py,call_ordering.py,cited_evidence.py,evidence_turn.py,relevance_veto.py,typed_evidence_event.py, andorchestrator/evidence_turn_dispatch.py. The executor change is the call site plus an extraction of the typed-evidence event fields.Decisions to confirm
evidence_turn_dispatch.run_evidence_turn.is_erroron success was verified on theclaude -pstream ("is_error": false). The SDK path throughadapter.pywas not exercised here. If the SDK leavesToolResultBlock.is_errorasNoneon success, every Claude citation readsresult_unknownand is withheld. Confirm this first on a live Claude run.later_run_failedtreats a later run with an unknown result as defeating too, which is stricter than "a later failed run".Known scope
EvidenceTurnSettings.from_config()reads live config at dispatch time. It is not part of the sealed v9 execution-semantics contract.call_tools=[]cannot stop shell use during the evidence turn. Such a turn makes the reply unusable, but it may still change the workspace after the primary snapshot.Evidence
tests/unit/orchestrator/evidence/test_cited_evidence.py):FABRICATION_SUSPECTED.defeated_by_later_run,not_a_verification_invocation).is_errormapping:truecited as passing is fabrication, the result text is not read, andis_erroralone is no verdict for an exit-status runtime.test_evidence_turn.py): the order is deterministic and structural, zero weights keep transcript order, and the reply parser accepts numbers only.test_evidence_turn_dispatch.py), end to end: the turn is a durable dispatch, citations decide, the transcript and final message stay the worker's own, an unknown number is fabrication, an unusable reply keeps the string path, and a failed turn runs no evidence turn.CodexCliRuntimeand cited the way a correct worker would. There were zero fabrications, with one exception: the sympy FULLpython /tmp/reproduce_sylow.py, which exited 1 on all three of its runs. Citing it undertests_passedis fabrication by the rule above, and the criterion still passes on itsbin/testcitation. The stub workspace lacked the repro scripts, so theirnot_replayedthere comes from the fixture.Checks
ruff check,ruff format --check,mypy src/ouroboros(693 files) andscripts/check-module-size.py --baseline-ref origin/mainall pass.pytest tests/unit: 25071 passed, 66 failed. All 66 also fail onorigin/mainin this environment (nopythonon PATH, nested MCP guard, macOS undecodable filenames, sandbox canary).🤖 Generated with Claude Code