Skip to content

feat: hold an agent run that shipped clean code for the wrong task - #6119

Merged
atomantic merged 1 commit into
mainfrom
claim/issue-5994
Sep 3, 2026
Merged

feat: hold an agent run that shipped clean code for the wrong task#6119
atomantic merged 1 commit into
mainfrom
claim/issue-5994

Conversation

@atomantic

Copy link
Copy Markdown
Owner

Summary

PortOS's reviewers answer "is this good code?". None of them could answer "is this the code that was asked for?"CODE_REVIEW_SYSTEM_PROMPT hands the model a unified diff and nothing else, and the whole configurable reviewer chain is diff-scoped the same way. The user-visible consequence: a CoS agent could ship a PR that was clean, well-reviewed, green in CI, and quietly did something other than the task that was written. evaluateSuccessCriteria proved changes existed and shipped; nothing proved they were the requested changes.

This adds a second, objective-aware review at agent completion.

  • server/lib/goalFidelity.js (new, pure) — the verdict vocabulary (ship / fix-first / rethink), taskObjective(task) (the trusted operator-authored objective: the TASK's description + prompt block, never the agent's transcript), the settings resolver, and the parse/validate of the model's structured answer. normalizeGoalFidelityVerdict returns null for an unusable answer, which is never collapsed into a ship pass or a rethink hold.
  • server/lib/gitCommitProbe.js — gains runWindowDiff, the diff half of the run-window probes. Base is resolved on committer date exactly as commitsSince filters on it, so the two can't disagree about which commits belong to the run. Every failure is a reason with a null diff, never '' — "git could not answer" must not read as "the run changed nothing".
  • server/services/codeReview.jsGOAL_FIDELITY_SYSTEM_PROMPT + runLocalGoalFidelityReview. Objective and diff ride one message in separate adaptive fences, each labelled with its trust level: the objective is the requirement, the diff is untrusted evidence that can never become an instruction. Returns a validated verdict or an error — never model prose.
  • server/services/agentFinalization.js — the completion gate. Runs only on a run that would otherwise be recorded a success (same posture as the branch-jack downgrade). A rethink downgrades it to needs-attention with a named suggestedFix; fix-first is recorded as advisory and still ships.
  • server/services/review.js — bridges a hold into a Review Hub alert, so "the agent built the wrong thing" reaches someone who was not watching /cos/agents.
  • Settings › Code Reviewers + /cos/agents — pick the local model (or inherit the chain's), switch the gate off, and read the verdict with its named missing/unrequested items.

Decisions worth flagging

  • The reviewer set is the local-LLM backends only. The gate runs inside finalizeAgent, in the server process, as one synchronous request; the CLI reviewers are invoked by the follow-up agent from a prompt and have no server-side entry point. A configured backend outside that set declines rather than being silently swapped for one the user did not pick.
  • On by default, inert without a local model. With nothing configured it inherits whichever local-LLM reviewer the quality chain already runs (and that reviewer's pinned model/effort), so the common case is configuring nothing; with no local reviewer anywhere it simply never runs, which is why "default on" cannot surprise an existing workflow.
  • Fail-open throughout. Gate off, no objective, an unreadable diff, a reviewer that errored or answered with prose — every one leaves the run's outcome exactly as it found it. A gate that could hold a run because ollama restarted would be worse than no gate.
  • Untrusted text stays text. missing / unrequested are model-authored strings derived from an untrusted diff. They are trimmed, capped, and have the ]( markdown link/image separator split apart, because the Review Hub renders an alert description through PortOS's markdown renderer.

Test plan

  • server/lib/goalFidelity.test.js (new) — objective composition and its cap, config resolution and its three decline paths, verdict validation, the null-means-nothing-judged sentinel, and link neutralization.
  • server/services/agentFinalization.goalFidelity.test.js (new) — the gate end to end: it judges the task objective and not the transcript, records ship without disturbing the run, treats fix-first as advisory, holds rethink with the right category/error/event, fails open across five distinct failure shapes, and never re-judges an already-failed run. Verified to fail before the fix.
  • server/lib/gitCommitProbe.test.jsrunWindowDiff: window resolution, empty-vs-unreadable, four decline paths, and cap-inclusive truncation.
  • server/lib/validation.test.js, server/services/codeReview.test.js, client/.../CodeReviewersTab.test.jsx, client/.../AgentCard.test.jsx — schema accept/reject, prompt shape + fence escaping, settings round-trip, rendered verdict.
  • Full server suite (38,673 passed) and full client suite (10,218 passed) green; client build clean.
  • Local ollama reviewer (the configured chain) returned No findings.

Closes #5994

https://claude.ai/code/session_01ChZ1DzJoaTsPoouRcrf8rJ

…5994)

PortOS's reviewers answer "is this good code?". None of them could answer "is
this the code that was asked for?", because none of them ever see the request —
the code-review prompt is handed a unified diff and nothing else. So a CoS agent
could ship a PR that was clean, well-reviewed and green in CI, and quietly do
something other than the task the user wrote. The run-level evidence gate proved
changes existed and shipped; nothing proved they were the requested changes.

A finished run's accumulated diff is now re-read against the task's own stated
objective, in a fresh context, and answers three questions the quality chain
cannot: is anything that was asked for missing, is anything that was not asked
for smuggled in, and is the verification evidence real. The verdict is ship,
fix-first, or rethink; a rethink records the run as needing attention rather
than complete, and raises a Review Hub alert naming what is missing or
unrequested. Settings > Code Reviewers picks the local model it runs on
(independently of the quality chain, or inheriting it) and can switch it off.

Fresh context is the mechanism: the objective comes from the task record, never
the agent's transcript, which would hand the reviewer the assumptions that
produced the drift in the first place. The gate fails OPEN throughout — a
missing local backend, an unreadable diff, or a model that answers with prose
all leave the run's outcome exactly as they found it, so a local-model outage
can never convert a queue of good runs into held ones.

Claude-Session: https://claude.ai/code/session_01ChZ1DzJoaTsPoouRcrf8rJ
@atomantic
atomantic merged commit e51fddd into main Sep 3, 2026
7 checks passed
@atomantic
atomantic deleted the claim/issue-5994 branch September 3, 2026 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[repo-study-fable-advisor-goal-fidelity-review] Review a finished agent run's diff against the task's stated objective, not just for code quality

1 participant