feat: terminal outcome integrity - #38
Merged
Merged
Conversation
Roundfix-Spec: 0037-terminal-outcome-integrity Roundfix-Task: task_01
Roundfix-Spec: 0037-terminal-outcome-integrity Roundfix-Task: task_02
Roundfix-Spec: 0037-terminal-outcome-integrity Roundfix-Task: task_03
Roundfix-Spec: 0037-terminal-outcome-integrity Roundfix-Task: task_04
Roundfix-Spec: 0037-terminal-outcome-integrity Roundfix-Task: task_05
Roundfix-Spec: 0037-terminal-outcome-integrity Roundfix-Task: task_06
Roundfix-Spec: 0037-terminal-outcome-integrity Roundfix-Task: task_07
Roundfix-Spec: 0037-terminal-outcome-integrity
The 2026-07-26 tooling authorization covered exactly the two roundfix SKILL.md files. The authorized skill edit deterministically changes the skill contentDigest, which five baseline/catalog identity artifacts pin, and QA finding F-01 blocked the gate because those paths were outside the recorded bounds. Record the maintainer's 2026-07-27 express authorization of exactly those five derived paths in both active Spec artifacts and align task_07's changed-path criterion. Claude-Session: https://claude.ai/code/session_01KwYiSLnmzdHmrmV4XWmQge
Roundfix-Spec: 0037-terminal-outcome-integrity
All 7 Tasks completed, QA gate verdict pass on build ef6eb44. Claude-Session: https://claude.ai/code/session_01KwYiSLnmzdHmrmV4XWmQge
Review follow-up for spec 0037: the reused-PID conservative refusal only existed behind an injected controller error. Runs now record an opaque owner start-time identity token (ps -o lstart=) beside owner_pid (schema v10); Force Stop compares the live token before any signal and fails closed through the existing owner-proof path on mismatch or unprovable identity, while token-less legacy rows keep the prior behavior per ADR-0044's degradation precedent. Orphan reclamation is unaffected: it acts only on proven absence, which a recycled PID cannot fake. Also replaces the substring-matched cleanup-warning classification with typed warning kinds; every printed byte is unchanged. Claude-Session: https://claude.ai/code/session_01KwYiSLnmzdHmrmV4XWmQge
Review follow-up for spec 0037: point the four Spec 0037 links in the user guide, the Vortex finding, and the implementation-sequence inbox note at the archived path; document the Stop Command's public exit codes in the command reference; and flip archived task_07's last acceptance checkbox, which its recorded gate evidence had already satisfied. Claude-Session: https://claude.ai/code/session_01KwYiSLnmzdHmrmV4XWmQge
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (9)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe change adds owner-process identity persistence and verified termination, compare-and-set terminal completion, stop-aware watch polling, registered agent-session cleanup, centralized outcome notification, integration reconciliation updates, and expanded tests and operational documentation. ChangesRun persistence and terminal transitions
Owner-process proof and session cleanup
Watch stop-request observation
CLI force-stop orchestration
Integration settlement and supporting artifacts
Operational contracts
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant StopCommand
participant OwnerProcessControl
participant AgentSessions
participant RunStore
StopCommand->>OwnerProcessControl: Prove and terminate recorded owner
OwnerProcessControl-->>StopCommand: Confirm owner absence
StopCommand->>AgentSessions: Cancel and close active registered sessions
StopCommand->>RunStore: CompleteRun as Stopped
RunStore-->>StopCommand: Return transitioned result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cli/cli_test.go`:
- Around line 4036-4049: Guard the agentStarted channel close in the
fakeAgentRunner onRun callback with its own sync.Once, mirroring releaseOnce, so
repeated Agent executions under --round all do not panic. Keep the existing
release synchronization and callback behavior unchanged.
In `@internal/cli/cli.go`:
- Around line 633-655: Reorder the stop flow so active agent sessions are
cleaned up only after the recorded owner identity has been validated and
TerminateAndWait succeeds. In the surrounding force-stop logic, move
bestEffortForceStopAgentSessions after the activeOwnerPID check and
ownerProcesses.TerminateAndWait call, while preserving warnings and error
results for validation or termination failures.
In `@internal/cli/orphan_unix_test.go`:
- Around line 248-282: The startCLIForceStopOwnerProcess helper currently starts
cmd.Wait in a goroutine before reading the helper’s readiness line. Move waiter
creation until after scanner.Scan and the "ready" validation complete, while
preserving cleanup’s ability to wait for the process and report early helper
failures accurately.
In `@internal/store/process_unix.go`:
- Around line 38-51: Update processStartIdentity to make the ps output
deterministic by setting TZ=UTC and LC_ALL=C in the command environment before
execution, while preserving the existing verbatim identity comparison and error
handling.
In `@internal/store/process_windows.go`:
- Around line 18-28: Update processAbsent to verify the opened process’s
liveness after syscall.OpenProcess succeeds, using GetExitCodeProcess or
WaitForSingleObject; treat only STILL_ACTIVE as present and return absent for
terminated processes, while preserving existing error handling and closing the
handle.
In `@internal/store/store.go`:
- Around line 376-395: The terminal-state list is duplicated across
compare-and-set updates and IsTerminalState, allowing future additions to become
inconsistent. In internal/store/store.go lines 376-395, create shared
terminalStates-derived placeholders and arguments for the state NOT IN clause;
in lines 752-789, reuse that generated clause and arguments and update
IsTerminalState to range over terminalStates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 521c3618-b9fa-4117-8759-aa2aa66d6b34
⛔ Files ignored due to path filters (29)
docs/_inbox/2026-07-24-spec-implementation-sequence.mdis excluded by none and included by nonedocs/findings/2026-07-16-vortex-pr87-detached-watch-notification.mdis excluded by none and included by nonedocs/specs/0037-terminal-outcome-integrity/task_01.mdis excluded by none and included by nonedocs/specs/0037-terminal-outcome-integrity/task_02.mdis excluded by none and included by nonedocs/specs/0037-terminal-outcome-integrity/task_03.mdis excluded by none and included by nonedocs/specs/0037-terminal-outcome-integrity/task_04.mdis excluded by none and included by nonedocs/specs/0037-terminal-outcome-integrity/task_05.mdis excluded by none and included by nonedocs/specs/0037-terminal-outcome-integrity/task_06.mdis excluded by none and included by nonedocs/specs/0037-terminal-outcome-integrity/task_07.mdis excluded by none and included by nonedocs/specs/_archived/0037-terminal-outcome-integrity/_prd.mdis excluded by none and included by nonedocs/specs/_archived/0037-terminal-outcome-integrity/_tasks.mdis excluded by none and included by nonedocs/specs/_archived/0037-terminal-outcome-integrity/_techspec.mdis excluded by none and included by nonedocs/specs/_archived/0037-terminal-outcome-integrity/qa/evidence/2026-07-27-terminal-outcome-integrity-rerun/constraint-audit.mdis excluded by none and included by nonedocs/specs/_archived/0037-terminal-outcome-integrity/qa/evidence/2026-07-27-terminal-outcome-integrity-rerun/docs-and-scope.mdis excluded by none and included by nonedocs/specs/_archived/0037-terminal-outcome-integrity/qa/evidence/2026-07-27-terminal-outcome-integrity-rerun/focused-regressions.mdis excluded by none and included by nonedocs/specs/_archived/0037-terminal-outcome-integrity/qa/evidence/2026-07-27-terminal-outcome-integrity-rerun/live-cli-flows.mdis excluded by none and included by nonedocs/specs/_archived/0037-terminal-outcome-integrity/qa/evidence/2026-07-27-terminal-outcome-integrity-rerun/static-gate.mdis excluded by none and included by nonedocs/specs/_archived/0037-terminal-outcome-integrity/qa/qa-report-2026-07-27.mdis excluded by none and included by nonedocs/specs/_archived/0037-terminal-outcome-integrity/task_01.mdis excluded by none and included by nonedocs/specs/_archived/0037-terminal-outcome-integrity/task_02.mdis excluded by none and included by nonedocs/specs/_archived/0037-terminal-outcome-integrity/task_03.mdis excluded by none and included by nonedocs/specs/_archived/0037-terminal-outcome-integrity/task_04.mdis excluded by none and included by nonedocs/specs/_archived/0037-terminal-outcome-integrity/task_05.mdis excluded by none and included by nonedocs/specs/_archived/0037-terminal-outcome-integrity/task_06.mdis excluded by none and included by nonedocs/specs/_archived/0037-terminal-outcome-integrity/task_07.mdis excluded by none and included by nonedocs/user-guide/commands.mdis excluded by none and included by nonedocs/user-guide/usage.mdis excluded by none and included by noneinternal/baseline/testdata/catalog.digestis excluded by!**/*.digestand included by noneskills/roundfix/SKILL.mdis excluded by!skills/**and included by none
📒 Files selected for processing (24)
.agents/skills/roundfix/SKILL.mdinternal/agent/sessions.gointernal/baseline/assets/setups/typescript-bun.jsoninternal/baseline/testdata/catalog.normalized.jsoninternal/baseline/testdata/parity-corpus/v1/fixtures/asset-sync.jsoninternal/baseline/testdata/parity-corpus/v1/manifest.jsoninternal/cli/cli.gointernal/cli/cli_test.gointernal/cli/implement.gointernal/cli/orphan_unix_test.gointernal/cli/settle.gointernal/cli/settle_test.gointernal/store/agent_selection.gointernal/store/agent_selection_test.gointernal/store/journal_test.gointernal/store/process.gointernal/store/process_other.gointernal/store/process_unix.gointernal/store/process_unix_test.gointernal/store/process_windows.gointernal/store/store.gointernal/store/store_test.gointernal/watch/watch.gointernal/watch/watch_test.go
💤 Files with no reviewable changes (1)
- internal/cli/settle.go
Dogfooding the v10 upgrade crashed watch preflight with a raw 'no such column: owner_identity': inspectBranchIntegrity scanned Active Runs through the never-migrates OpenReader before any operational Open had upgraded the live v9 database. The preflight now uses the migrating Open when a Run Database exists while preserving the conservative no-database branch, and OpenReader guards the schema version with a typed SchemaVersionError naming the found and supported versions and the operational-command remediation instead of leaking SQL errors from runs list, attach, events, gc, and settle. Claude-Session: https://claude.ai/code/session_01KwYiSLnmzdHmrmV4XWmQge
Round 1 on PR #38 raised six findings, all on this branch's own code: - Force Stop cancelled Agent Sessions before the owner was proven, so a fail-closed proof left a still-owned Active Run with its sessions torn down. The ownership rule is now a shared proof that TerminateAndWait and a new read-only ProveOwner both use, and Force Stop proves the owner first, then cancels sessions, then terminates. This keeps the spec's documented order (sessions cancelled before termination) while closing the degraded-state gap. - owner_identity is compared verbatim, so ps now runs under TZ=UTC and LC_ALL=C; a timezone or locale change no longer blocks a genuine owner. - The Windows liveness probe treated any OpenProcess success as alive; it now reads GetExitCodeProcess and treats STILL_ACTIVE as the only live state. - The terminal-state set was enumerated three times; both compare-and-set guards and IsTerminalState now share one slice. - Two test helpers were hardened: a sync.Once guard on the agent-started channel, and reading the readiness line before waiting on the helper. The Skill pair re-sync carries its derived digest pins through the baseline setup asset, catalog identity, and parity corpus, under the authorization recorded in the archived Spec's Tooling authority entries. Claude-Session: https://claude.ai/code/session_01KwYiSLnmzdHmrmV4XWmQge
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/store/process.go (1)
126-148: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winRe-prove owner identity before the force-kill escalation.
TerminateAndWaitproves ownership once at entry, then waits up togracePeriodfor the graceful SIGTERM to take effect before escalating to SIGKILL (Line 143). If the owner exits during that wait and the OS recycles the PID for an unrelated process beforewaitForAbsencenotices, the force-kill signal is sent to that unrelated process — precisely the PID-reuse hazard the owner-identity token was introduced to prevent. The proof is not re-checked anywhere between the graceful wait and the force kill.🛠️ Proposed fix: re-prove ownership immediately before force-kill
case errors.Is(gracefulErr, errGracefulSignalUnsupported): default: return ownerProcessControlError(pid, "send graceful termination", gracefulErr) } + // The owner may have exited and had its PID recycled during the grace + // wait; a stale proof must not license a kill of an unrelated process. + absent, err = controller.proveOwner(ctx, pid, recordedIdentity) + if err != nil { + return err + } + if absent { + return nil + } if err := signalOwnerProcess(pid, true); err != nil {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/store/process.go` around lines 126 - 148, Update TerminateAndWait to re-prove the owner identity immediately before signalOwnerProcess(pid, true), reusing the existing ownership token/proof mechanism established at entry. Abort or return the appropriate absence/error result when re-validation fails, and only perform the force-kill after ownership is confirmed.internal/cli/cli.go (1)
2607-2632: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winHandle terminal-outcome conflicts in watch completion.
runStore.CompleteRun(...)can returnstore.TerminalOutcomeConflictErrorwhen another terminal operation wins the race; treat that as the winning terminal state instead of printing a run failure and returningexitRunFailed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/cli.go` around lines 2607 - 2632, Update the run completion flow around runStore.CompleteRun to detect store.TerminalOutcomeConflictError and treat it as the winning terminal state: skip the failure UI, close/publish using the already-established terminal outcome and run identity, and return the corresponding successful/stopped result instead of exitRunFailed. Preserve the existing error path for all other completion errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/cli/cli.go`:
- Around line 2607-2632: Update the run completion flow around
runStore.CompleteRun to detect store.TerminalOutcomeConflictError and treat it
as the winning terminal state: skip the failure UI, close/publish using the
already-established terminal outcome and run identity, and return the
corresponding successful/stopped result instead of exitRunFailed. Preserve the
existing error path for all other completion errors.
In `@internal/store/process.go`:
- Around line 126-148: Update TerminateAndWait to re-prove the owner identity
immediately before signalOwnerProcess(pid, true), reusing the existing ownership
token/proof mechanism established at entry. Abort or return the appropriate
absence/error result when re-validation fails, and only perform the force-kill
after ownership is confirmed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2ec69301-b81a-483d-8856-7380fad3fbf0
⛔ Files ignored due to path filters (9)
docs/specs/_reviews/pr-38/round-001/issue_001.mdis excluded by none and included by nonedocs/specs/_reviews/pr-38/round-001/issue_002.mdis excluded by none and included by nonedocs/specs/_reviews/pr-38/round-001/issue_003.mdis excluded by none and included by nonedocs/specs/_reviews/pr-38/round-001/issue_004.mdis excluded by none and included by nonedocs/specs/_reviews/pr-38/round-001/issue_005.mdis excluded by none and included by nonedocs/specs/_reviews/pr-38/round-001/issue_006.mdis excluded by none and included by nonedocs/specs/_reviews/pr-38/round-001/round.mdis excluded by none and included by noneinternal/baseline/testdata/catalog.digestis excluded by!**/*.digestand included by noneskills/roundfix/SKILL.mdis excluded by!skills/**and included by none
📒 Files selected for processing (14)
.agents/skills/roundfix/SKILL.mdinternal/baseline/assets/setups/typescript-bun.jsoninternal/baseline/testdata/catalog.normalized.jsoninternal/baseline/testdata/parity-corpus/v1/fixtures/asset-sync.jsoninternal/baseline/testdata/parity-corpus/v1/manifest.jsoninternal/cli/cli.gointernal/cli/cli_test.gointernal/cli/orphan_unix_test.gointernal/store/process.gointernal/store/process_unix.gointernal/store/process_unix_test.gointernal/store/process_windows.gointernal/store/process_windows_test.gointernal/store/store.go
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
1 participant
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.
Implements spec 0037-terminal-outcome-integrity (archived in this branch after a passing QA gate) plus post-review hardening.
What changed
Verification
make verify— 2,484 Go tests across 23 packages, skill contract checks, build.docs/specs/_archived/0037-terminal-outcome-integrity/qa/qa-report-2026-07-27.md); the earlier fail report is preserved in history.a66e7a5/a4365f6.https://claude.ai/code/session_01KwYiSLnmzdHmrmV4XWmQge
Summary by CodeRabbit
New Features
stop --forceto require a provable owner process exit before completion; safer behavior to avoid PID-reuse hazards.Bug Fixes
stop --forcenow reports cleanup issues with clearer secondary-cleanup diagnostics, and preserves Active runs/locks when verification fails.settlebehavior so “kept” unresolved runs remain Unresolved.