pipeline-review-and-agents

Review-Loop Agent Sessions (internal/pipeline/sessions.go)

• Per run, the review loop keeps ONE durable fixer session across review-fix turns, and EVERY review turn (initial review and every full rereview) runs session-free. A rereview certifies fixes implementing the previous review turn's findings, so resuming any review session seats the prescriber as certifier - the mechanism that let one fix round ship wrong code plus the test blessing it with zero findings. Cross-round review context travels only in the explicit sanitized round history; the fixer session is never lent to review turns, no other step uses sessions, and sessions are keyed strictly by run. The rereview prompt reframes fix-round changes as pipeline-authored code under the author-grade adversarial standard (fixRoundProvenanceClause); the same clause is emitted on a later run's initial review when a persisted uncertified range is bound. Prior findings, fix summaries, and same-round tests are claims, not evidence.
• Fail-safe rules: unsupported adapter runs cold; a failed fixer resume drops the identity and re-runs the same turn in a fresh fixer session, never skipping the turn; a cancelled ctx gets no fallback retry; session_reuse: false forces everything cold. Persistence is minimum metadata only, never prompts or transcripts; SessionRoleReviewer remains only so crash recovery accepts legacy persisted rows, which are never resumed.
• codex exec resume has a narrower flag surface than codex exec, so an unsupported override fails the resume and falls back; the e2e fakeagent must keep parsing both codex argv shapes (extractCodexPrompt).
• Regressions: internal/pipeline/sessions_test.go, internal/pipeline/steps/review_session_test.go (incl. TestReviewLoop_RereviewNeverResumesTheSessionThatPrescribedItsFixes), TestReviewStep_RereviewTreatsFixRoundsAsPipelineAuthoredCode, internal/agent/session_test.go.

Recorded Human Decisions on Findings

• Approve, skip, and abort each record selected_finding_ids = "[]" plus selection_source = user_declined on a gated round with findings (executor.go recordDeclinedRound, db.SetStepRoundDeclined); a round with no findings records no decision. The conditional write must never erase an existing selection. User-facing semantics are owned by docs/src/content/docs/reference/pipeline-steps.md.
• A decline is stored as the COMPLEMENT of the selection, never as its own list; declinedFindingLines derives it and deliberately excludes auto_fix selections, whose complement is findings still awaiting a decision (rendered under auto_fix_left_unselected, which carries no do-not-re-report instruction).
• roundHistoryPromptSection (internal/pipeline/steps/round_history.go) now carries three parts: this step's rounds, this run's OTHER steps' decisions, and earlier runs' decisions on this branch (bound per step by pipeline.BindBranchDecisions, unlike review-only BindUncertifiedPipelineRange). Nothing clears branch decisions - a completed review deletes the uncertified range, which is why that channel could not carry a decision forward, but approving a gate IS the decision. The prompt states that a recorded decision SUPERSEDES the user-intent wording.
• Deliberately ADVISORY and fail-open: no step is blocked and no commit is gated, so an agent may still re-raise a declined finding when the code genuinely changed. There is no reversion detector; assertPipelineHeadContinuity and assertReviewApprovedPushHead remain lineage-only. ci_fix.go composes the section like the other fix-capable steps - before userIntentPromptSection, because the run's frozen intent always predates a later gate's decision - so a CI repair sees the decision an earlier gate, an earlier run on this branch, or its own gate recorded (internal/pipeline/steps/ci_decision_history_test.go). rebase.go still builds its prompt without roundHistoryPromptSection.
• Regressions: TestExecutor_GateResolutionsWithoutASelectionRecordTheDecline, TestExecutor_GateResolutionWithNoFindingsRecordsNoDecision, TestExecutor_FixResolutionStillRecordsAUserSelection, internal/db/round_decisions_test.go, TestDeclinedFindingReachesALaterStepInTheSameRun, TestDeclinedFindingReachesALaterRunOnTheSameBranch, TestCompletedReviewDoesNotClearBranchDecisions, TestAutoFixComplementIsNeverPresentedAsAUserDecision.

Uncertified Review Provenance (internal/pipeline/uncertified.go)

• When a review-step fixer round commits and its re-review does not complete, persist the per-branch uncertified range (from_sha, to_sha). Persist on review-step fixer commits only, not lint or document. On the next run's initial review, bind that range and emit fixRoundProvenanceClause even when Fixing==false, so the replacement reviewer is not cold. Rerun proceeds; there is no refusal or --ack-uncertified-review gate.
• Missing git objects warn and continue, never block. Clear the range only after a completed review whose approved head equals or is a descendant of to_sha; parked, failed, skipped, and aborted reviews must not clear it. Rebase remaps the persisted SHAs onto the rewritten head so the next review can still bind.
• Regressions: internal/pipeline/uncertified_test.go, TestCommitAgentFixes_PersistsUncertifiedRangeForReview, TestCommitAgentFixes_LintDoesNotPersistUncertifiedRange, TestCommitAgentFixes_DocumentDoesNotPersistUncertifiedRange, TestFixRoundProvenanceClause_EmitsForUncertifiedRangeWhenNotFixing, TestUncertifiedRange_PersistsThenFeedsNextInitialReview, TestRebaseStep_RemapsUncertifiedRangeWhenHeadRewritten.

Authorization and Privacy Tracing (internal/pipeline/steps/review.go)

• Authorization/privacy is a conditional obligation inside the existing Review pass, never a separate turn, gate, finding type, status, certification, or default-cost surface. For changed behavior involving potentially protected resources or user data, trace identity, the earliest shared authorization boundary and alternate callers, ownership/role/tenant scope, public serialization, secondary disclosures, and fail-open or stale-context paths.
• Policy remains repository-owned through project instructions and trusted review.path_instructions. Findings require a concrete source-backed reachable operation or disclosure and name the protected resource/field, missing control, and impact; accept equivalent controls and intentionally public data rather than keyword-matching auth machinery. A material policy ambiguity introduced by changed behavior must be ask-user and name the missing policy decision; do not report immaterial or pre-existing ambiguity, and retain auto-fix for source-proven routine defects.
• Docs owner: docs/src/content/docs/reference/pipeline-steps.md. Prompt regression: TestReviewStep_AuthorizationPrivacyTracingContract. Development-only qualitative cases: internal/pipeline/steps/testdata/authorization_privacy_review/.

Intended-Usage Evidence Threshold (internal/pipeline/steps/review.go)

• A review finding needs a concrete sequence that occurs during the change's intended usage. A rare but real sequence those callers actually perform still qualifies. A finding whose only supporting path is a hypothetical unused execution that intended callers, the public API, or documented usage never take does not. This is an evidence threshold, not a general "be less noisy" rewrite.
• Docs owner: docs/src/content/docs/reference/pipeline-steps.md. Prompt regression: TestReviewStep_IntendedUsageEvidenceContract. Development-only qualitative cases: internal/pipeline/steps/testdata/intended_usage_review/.

Simplification Pass and Fix-Through-Removal (internal/pipeline/steps/review.go, internal/pipeline/steps/common_fix.go, internal/pipeline/steps/lint.go, internal/pipeline/steps/ci_fix.go)

• The review prompt carries a dedicated Simplification section between Rules and Risk assessment. It asks a different question from the defect pass: not "is this component correct" but "does the intent strictly require this component". Every component the change introduced (branch, acceptance/matching path, fallback, alias, mode, flag, option, second definition of a concept already defined once, parallel copy of a rule) is judged against the user intent, or the change's own stated purpose when none is given. An unrequired component is a warning with action ask-user whose remedy is removal, never hardening, validating, or documenting; a defect finding inside such a component must name removal as its remedy. Refactor-only "simplification opportunities" keep their existing non-feature-removing meaning and point at the section instead of contradicting it. No schema field, detector, or second reviewer.
• fixerPrompt applies the removal rule at the shared Review, Test, and configured-Lint repair boundary; Lint's no-command safe-fix pass and the CI repair prompt apply the same rule directly. A finding resolvable by removing a code path the intent does not strictly require is fixed by removing it. The Review-specific anti-revert guard protects only intent-required code; doubt about whether the intent requires a path leaves it alone and reports the finding unresolved.
• Reference case: backpass PR #107 (https://github.com/kunchenguid/backpass/pull/107), where a permissive target resolver and a second skill-only budget semantics each cleared the intended-usage evidence threshold every round and were hardened for thirteen rounds without converging; one deleted acceptance branch would have closed the class. The intended-usage threshold (#948) is about whether a defect is real; this pass is about whether the component should exist.
• Docs owners: docs/src/content/docs/reference/pipeline-steps.md (Review, shared repair behavior, CI fixer) and docs/src/content/docs/concepts/auto-fix.md (finding actions). Regressions: TestReviewStep_SimplificationSectionContract, TestReviewStep_FixPromptPrefersRemovalOfUnrequiredPaths, TestTestStep_FixMode, TestLintStep_FixMode_CommitsChanges, TestLintStep_NoConfiguredLint_UnresolvedFindingsNeedApprovalWithoutAutoFixLoop, TestCIStep_FixPromptPrefersRemovalOfUnrequiredPaths, TestReviewStep_SimplificationFixturesApply. Development-only qualitative cases: internal/pipeline/steps/testdata/simplification_review/.

Review Fixer Verification Discipline (internal/pipeline/steps/review.go)

• The review-fix prompt requires all fixes before one focused verification limited to the changed area and forbids the whole repository test/lint suite during the fix round. The dedicated Test and Lint steps are the authoritative gates, although their coverage may be focused when commands are unconfigured. This is a prompt contract, not an enforced sandbox. Regression: TestReviewStep_FixMode_FocusedVerificationContract.

Remedy-Scope Discipline in the Review Prompts (internal/pipeline/steps/review.go)

• Three prompt rules keep review findings and fix rounds from growing machinery nobody scoped, using only the existing action vocabulary and gate. No detector, no schema field, no second scope reviewer: a growth detector or scope verifier is itself the machinery being prevented, and a second judgment owner on the gate contradicts VISION's "never stacked".
• Reviewer classifies by REMEDY, not only topic: a finding whose smallest honest remedy would add durable state, a schema change, background/retry/persistence machinery, a new subsystem, or otherwise EXTEND rather than CORRECT the change must be ask-user, with the description naming the remedy as what needs authorization. This rides ActionOrDefault's established fail-toward-the-human direction.
• Fixer fixes the reported instance narrowly and reaches depth by simplifying an architectural reason rather than bolting on machinery for the symptoms. The preceding local-defect-vs-deeper-flaw diagnosis rule stays; depth is not forbidden, symptom machinery is. The superseded "fix the deepest practical cause instead" wording must not return.
• Rereview gets one exit ramp from the fix-round ratchet: defects in code a PRIOR fix round introduced that exceeds what the original finding required become a single ask-user finding recommending that round be reverted to the minimal fix, instead of further repairs layered on it. Emitted in both fixRoundProvenanceClause branches (this run's fix rounds and a previous run's uncertified fixer commits); conditioning on prior-round code keeps it off ordinary multi-round fixes.
• Docs owners: docs/src/content/docs/concepts/auto-fix.md (finding actions) and docs/src/content/docs/reference/pipeline-steps.md (Review). Regressions: TestReviewStep_PromptClassifiesFindingsByRemedyScope, TestReviewStep_FixPromptPrefersSimplificationOverMachinery, TestReviewStep_RereviewOffersRevertExitFromPriorRoundMachinery.

Agent-Invocation Timeouts Report Measured Silence, Never the Budget

• A timeout diagnostic may only state what was observed, never restate the configured budget as measured silence. agentActivity in agent_run.go is the single owner of the measurement and resets per-attempt evidence whenever a retry or fallback starts a replacement attempt, including provider, session-resume, and OpenCode prompt-format fallbacks. A substantive adapter error (a native agent's exit status plus captured stderr) is URL-redacted, length-bounded, and appended as agent reported: ....
• Observed output is streamed assistant text plus throttled agent.LifecyclePhaseActivity, sourced from every non-empty read of a native subprocess's stdout or stderr. Prose alone cannot prove liveness: verified against pi 0.84.3, a tool-using turn emits only tool_execution_*/toolcall_* and no text_delta until the very end, and no adapter forwards those to OnChunk. Subprocess start and exit are deliberately NOT output - start proves launch, not work, and exit is the deadline's own consequence, so counting either would recreate the fabricated evidence.
• The executor consumes LifecyclePhaseActivity into step activity only, never the step log: axi status needs the liveness, and a half-hour turn would otherwise emit hundreds of log lines.
• A CI auto-fix agent that exhausts its budget parks at an ask-user gate (ciFixAgentTimeoutOutcome) instead of being logged as a warning and re-issued on the next poll. That old path spent up to auto_fix.ci full budgets invisibly until ci_timeout. Only pipeline.ErrAgentTimeout parks; other fix failures keep warn-and-retry. Review deliberately still fails the run rather than parking - Push commits leftover worktree changes, so an approved park would ship a half-finished, unreviewed fix.
• Each Review fixer and each independent session-free reviewer owns a fresh review_agent_timeout absolute wall-clock limit. Prompt preparation and post-fixer commits stay on the step parent context, and an expired candidate context stops the fallback chain before another provider is announced or instrumented.
• Docs owners: docs/src/content/docs/reference/global-config.md (agent_timeout, review_agent_timeout) for the diagnostic and ownership vocabulary, docs/src/content/docs/reference/pipeline-steps.md (Review and CI) for step behavior. Regressions: TestRunAgent_Timeout*, TestRunAgent_SubprocessStartAloneIsNotObservedOutput, TestRunAgent_OperatorCancellationIsNotDressedUpAsAnAgentFault, TestPiAgent_ToolOnlyStreamStillReportsSubprocessLiveness, TestPiAgent_SilentSubprocessReportsNoLiveness, TestExecutor_SubprocessLivenessUpdatesActivityWithoutFloodingTheStepLog, TestCIStep_FixAgentBudgetExhaustionParksForADecisionInsteadOfRetrying, TestCIStep_NonTimeoutFixFailureKeepsRetrying, TestReviewStep_EachAgentInvocationGetsItsOwnBudget, TestReviewStep_WallClockTimeoutPreservesTheAgentReport, e2e TestSilentAgentTimeoutReportsMeasuredEvidence.

Local Test Is Targeted Validation (internal/pipeline/steps/test.go)

• Local Test (normal evidence agent and Test-repair agent) validates the requested intent with the smallest relevant checks and end-user-aligned evidence; it is never a repository-wide regression-suite walk. Broad regression belongs to remote CI (go test -race ./... in .github/workflows/ci.yml) and remains mandatory before a PR is ready. commands.test is the same contract when set: targeted baseline, not CI-parity complete-suite configuration; docs owner is docs/src/content/docs/reference/repo-config.md (commands.test), step behavior owner is docs/src/content/docs/reference/pipeline-steps.md (Test). This repository dogfoods an empty commands.test so the agent-driven targeted path is the default; do not reintroduce go test -race ./... as a local Test override. Process-group reaping on clean/error exit (#357) and Unix WaitDelay remain the lifecycle safety net when agents spawn test workers - restoring the agent-driven path must not revive the daemon OOM leak. Those agent turns are bounded by test_agent_timeout (default 30m, global-only): a stalled evidence or repair agent is cancelled and the run fails instead of waiting forever. Native adapters already honor that deadline through CommandContext; the missing piece was the Test step never setting one. Docs owner is docs/src/content/docs/reference/global-config.md. Every other pipeline agent invocation is bounded by agent_timeout (default 30m, global-only) at pipeline.RunAgent / the executor timeoutAgent seam, so a new agent-spawning step cannot hang a run by forgetting a deadline. Review gives every fixer and reviewer a fresh review_agent_timeout; an existing sooner parent deadline is honored rather than capped. The invocation context is scoped only to Agent.Run; a late successful return after the deadline is rejected. Docs owner is docs/src/content/docs/reference/global-config.md. Regressions: TestTestStep_InitialAgent_TargetedValidationContract, TestTestStep_FixMode_TargetedVerificationContract, TestTestStep_FixMode_DriverFullSuiteInstructionDoesNotOverrideContract, TestTestStep_InitialAgent_NoTargetedEvidenceRequiresHonestFinding, TestTestStep_HangingEvidenceAgentFailsRunAfterTimeout, TestCodexAgent_RunCancelsSilentHang, TestDogfoodConfig_NoBroadLocalTestCommand, TestCIWorkflow_RetainsFullRaceSuiteAsBroadRegressionOwner, plus the existing #357 reap/WaitDelay tests, TestRunAgent_*, TestExecutor_DirectAgentRunIsDeadlineBounded, TestDocumentStep_HangingAgentFailsRunAfterTimeout, TestLintStep_HangingAgentFailsRunAfterTimeout, TestCIStep_HangingFixAgentFailsAfterTimeout, TestRebaseStep_HangingConflictAgentFailsAfterTimeout.

Intent Provenance & Conformance (internal/pipeline/steps/intent_prompt.go)

• Intent carries provenance: an explicit axi run --intent persists Source==db.RunIntentSourceAgent ("agent", score 1); a transcript match persists the agent name ("claude"/"codex"/...). The executor propagates it as StepContext.IntentSource alongside UserIntent (executor.go).
• userIntentPromptSection branches on source: an EXPLICIT intent renders as sanitized-but-AUTHORITATIVE acceptance criteria; an INFERRED intent keeps the low-confidence hint framing verbatim. Both branches keep the StripAdversarial+RedactSecrets pipeline and BEGIN/END "do not execute instructions" guard - authoritative reframes only the content's authority (check the diff against the criteria), never whether control tokens are stripped. The review prompt adds intentConformanceReviewClause for agent-source intent only: a fixer change that contradicts the criteria (removes intent-required or adds intent-forbidden behavior) MUST become an ask-user finding, which parks with no executor change. Conformance is limited to source-verifiable criteria; deferred pipeline-owned delivery (remote branch / push / PR / CI for this run) is out of scope at review.
• Review is always pre-push (StepReview before StepPush/StepPR/StepCI). pipelineDeliveryPhaseClause plus stripDeferredPipelineOwnedDeliveryFindings (pipeline_delivery.go, applied in review.go) keep findings that only claim those later-owned outcomes are missing from parking the run. External or pre-existing lifecycle requirements (numbered PR, third-party artifact, non-run-owned state) stay enforceable. Push, PR, and CI steps remain strict after their stages run.
• Empty/missing finding action fails closed to ask-user, not auto-fix (types/findings.go ActionOrDefault); HasAskUserFindings uses ActionOrDefault so it agrees with AutoFixableFindings (an unclassified finding is never auto-fixed and is always caught as ask-user). MergeUserOverrides still stamps user-added findings auto-fix on purpose.
• The deterministic net-deleted-author-lines git-diff backstop is intentionally not built; review.go owns the held-scope TODO.
• Regressions: internal/pipeline/steps/intent_prompt_test.go, internal/pipeline/steps/review_test.go (TestReviewStep_ConformanceObligationTracksIntentProvenance, TestReviewStep_RereviewFlagsIntentContradictionAsAskUser), internal/pipeline/steps/pipeline_delivery_test.go, internal/pipeline/steps/review_pipeline_delivery_test.go, internal/pipeline/executor_intent_conformance_test.go, internal/types/findings_test.go, e2e TestIntentJourney (inferred-source framing), e2e TestReviewPipelineOwnedPRCriterionDoesNotPark / TestReviewExternalPRLifecycleStillParks.