diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e71a38a..29cf1bcd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,8 @@ All notable user-visible changes to CASCADE are documented here. The format is l ### Changed +- **Completion contracts can declare pushed-changes alternatives.** `hooks.finish.scm.pushedChangesAlternatives: [pr-response]` (valid only next to `requiresPushedChanges: true`) tells the shared completion gate that a recorded PR response — a new top-level comment or an inline review reply — combined with a clean working tree and a HEAD still at the run-start SHA satisfies the requirement instead of a push. The evaluation (`evaluatePushedChanges` in `src/backends/completion.ts`) is shared by the continuation loop and post-run processing, checks the push branch first, and fails closed on missing, malformed or contradictory evidence. Run logs now name the outcome that passed (`pushedChangesOutcome`) or list why every outcome was rejected, and the continuation prompt names an already-recorded response and forbids posting it again. No built-in agent declares an alternative yet; the evidence producers and the PR-comment opt-in follow in the next two changes. + - **Worker image pins Codex CLI 0.155.1 (was 0.145.0), with a build-time grammar check.** The bump is what makes GPT-6 Astra a first-class model (0.145.0 has no catalog entry for it and runs it on fallback metadata). Two behaviours were adjusted for the newer CLI: (1) the deprecated `--enable web_search` feature toggle is replaced by the top-level `web_search` config (`live` when the engine setting `webSearch` is true, `disabled` otherwise) — the CLI now defaults web search to on, so `webSearch: false` has to disable it explicitly; (2) the git-push deny hook was re-verified live on 0.155.1 with both GPT-5.5 and GPT-6 Astra. `Dockerfile.worker` ends with `node dist/backends/codex/grammarSmokeCli.js`, a model-free probe that feeds the exact continuation argv CASCADE generates (every option plus `resume -`) to the pinned CLI and passes only when it stops at thread lookup — an incompatible grammar fails the image build. `tests/fixtures/codex/` now carries the 0.155.1 protocol schema and a recorded exec stream, and the event-schema test reads the pinned version from `Dockerfile.worker` so a future bump without regenerated fixtures fails loudly. - **Worker spawn now resolves a verified per-project worker image and pins it to a digest** ([MNG-1697](https://linear.app/issue/MNG-1697), spec 022 plan 2 of 4). `resolveSpawnSettings` (`src/router/worker-spawn-settings.ts`) introduces an explicit `SpawnSettings.effectiveBaseImage` — the project's verified per-project image digest when configured, otherwise the global `routerConfig.workerImage`. With no per-project image this is a pure no-op refactor: `effectiveBaseImage === routerConfig.workerImage` and resolved image, snapshot behavior, and pull-fallback are identical to before. When a project sets `workerImage` with `workerImageStatus: 'verified'` and a non-empty `workerImageDigest`, the worker launches from that digest (the `Resolved spawn settings` log now records `projectWorkerImage`, `globalWorkerImage`, and `effectiveBaseImage` so a post-mortem can confirm which image won). A configured-but-unverified image (`pending` / `failed` / missing digest) throws a terminal `WorkerImageResolutionError` and never silently falls back to the global default; the dispatch-error classifier maps that error to `terminal` so BullMQ skips the retry budget. `container-manager.ts` now classifies pull-fallback, snapshot reuse, and the snapshot-404 fallback against `effectiveBaseImage` instead of the global default — so a per-project base image is pulled-on-missing exactly like the global base, a custom-image run without a snapshot is not misclassified as a snapshot reuse, and a stale snapshot 404 falls back to the verified digest (not the global image). A missing-but-pullable custom digest is pulled once and retried; an unobtainable/invalid custom digest fails loud with a grep-stable terminal error instead of relaunching on the global default. The container launch posture is unchanged (only `Memory` / `MemorySwap` / `NetworkMode` / `AutoRemove` + labels — no mounts, no privileged). Dormant for operators until the set/validation flow lands (plan 3); exercised here via seeded `ProjectConfig`. diff --git a/docs/plans/025-pr-comment-completion-and-codex-resume/1-completion-alternatives.md.done b/docs/plans/025-pr-comment-completion-and-codex-resume/1-completion-alternatives.md.done new file mode 100644 index 00000000..43bf75d5 --- /dev/null +++ b/docs/plans/025-pr-comment-completion-and-codex-resume/1-completion-alternatives.md.done @@ -0,0 +1,276 @@ +--- +id: 025 +slug: pr-comment-completion-and-codex-resume +plan: 1 +plan_slug: completion-alternatives +level: plan +parent_spec: docs/specs/025-pr-comment-completion-and-codex-resume.md +depends_on: [] +status: done +locked_at: ddcef0cd7b5be3c10ce57a9fa1e345a9578e4276 +--- + +# 025/1: Completion alternatives — shared, fail-closed evaluation + +> Part 1 of 3 in the 025-pr-comment-completion-and-codex-resume plan. Parent spec: resolve `docs/specs/025-pr-comment-completion-and-codex-resume.md*` (it may have been renamed `.done`). + +## Summary + +This plan teaches the shared completion contract to express **alternative valid outcomes** for the pushed-changes requirement, without changing what any agent experiences yet. A profile may declare `pushedChangesAlternatives: [pr-response]` next to `requiresPushedChanges: true`; the shared evaluator then accepts *either* authoritative pushed-change evidence *or* an authoritative PR-response record combined with a clean, unchanged repository. Missing, malformed, or contradictory evidence satisfies neither branch. The evaluation is engine-agnostic (it lives in `completion.ts`, consumed by the shared continuation loop and by post-processing) and contains no agent-name conditional. + +It ships **dormant**: no profile opts in (plan 3), and no tool writes the PR-response sidecar yet (plan 2). What is reviewable in isolation is the model (schema flag, requirement/evidence types), the fail-closed evaluator with its per-outcome rejection reasons, the outcome-aware continuation prompt (which already tells a resumed session not to repeat a recorded response), and the log lines that name the outcome that succeeded or why each alternative was rejected. The duplicated pushed-changes check in `postProcess.ts` is replaced by the shared evaluator so the adapter and the continuation loop cannot drift. + +Plan 2 produces the evidence (sidecar writers, `session finish` acceptance, env plumbing); plan 3 opts the PR-comment agent in, adjusts its guidance, and adds the end-to-end proof plus documentation. + +**Components delivered:** +- `src/agents/definitions/schema.ts` — `pushedChangesAlternatives` finish-hook flag (+ refinement) +- `src/agents/definitions/profiles.ts` — flag threaded into `FinishHookFlags` +- `src/backends/completion.ts` — PR-response evidence, repo-state evidence, `evaluatePushedChanges`, outcome-aware failure/prompt +- `src/backends/shared/continuationLoop.ts` — outcome logging on success, rejection reasons on continuation/failure +- `src/backends/postProcess.ts` + `src/backends/adapter.ts` — post-run gate uses the shared evaluator + +**Files owned (exclusive to this plan within this spec):** +- `src/agents/definitions/schema.ts` +- `src/agents/definitions/profiles.ts` +- `src/backends/completion.ts` +- `src/backends/shared/continuationLoop.ts` +- `src/backends/postProcess.ts` +- `src/backends/adapter.ts` +- `src/backends/secretOrchestrator.ts` (one line only: forwards `pushedChangesAlternatives` from the profile into `completionRequirements`; plan 2 owns every other change there) +- `tests/helpers/tempGitRepo.ts` (shared real-git helper, reused by plans 2 and 3) +- `tests/unit/backends/completion.test.ts` +- `tests/unit/backends/shared-continuationLoop.test.ts` +- `tests/unit/backends/postProcess.test.ts` +- `tests/unit/agents/definitions/schema.test.ts` +- `tests/unit/agents/definitions/profiles.test.ts` + +**Shared surfaces (append-only, conflicts are trivial):** +- `CHANGELOG.md` (one block under *Unreleased → Changed*) + +**Deferred to later plans in this spec:** +- Writing the PR-response sidecar from `cascade-tools scm post-pr-comment` / `reply-to-review-comment`, the `CASCADE_PR_RESPONSE_SIDECAR_PATH` env var, sidecar path creation, and `session finish` accepting the comment-only outcome — plan 2. +- `respond-to-pr-comment.yaml` opting in, task-prompt guidance, end-to-end tests, architecture docs — plan 3. + +--- + +## Spec ACs satisfied by this plan + +- Spec AC #1 (question → completes without commit) — **partial (this plan: the evaluator accepts the `pr-response` outcome; plan 2 produces the evidence; plan 3 opts the agent in)** +- Spec AC #2 (changed files, not pushed → no completion) — **partial (this plan: the `pr-response` branch requires a clean tree and unchanged HEAD and yields an actionable pushed-change reason; plan 2 mirrors it in `session finish`; plan 3 proves it end to end)** +- Spec AC #3 (pushed changes still complete under the strict path) — **full (regression pin: `pushed-changes` remains the first-checked branch and the only branch when no alternative is declared)** +- Spec AC #5 (missing / malformed / inconsistent evidence satisfies no alternative; actionable reason) — **full** +- Spec AC #6 (a recorded response is not repeated on continuation) — **partial (this plan: the continuation prompt names the recorded response URL and says not to post again; plan 3 adds the task guidance and the e2e assertion)** +- Spec AC #7 (identical across native-tool engines) — **partial (this plan: one evaluator, consumed by the shared loop every native-tool engine already uses; plan 3 adds the structural parity test)** +- Spec AC #8 (agents that require pushed changes today keep failing without a push; nothing inherits the alternative) — **partial (this plan: schema refinement + evaluator only consults alternatives that are declared; plan 3 pins that only `respond-to-pr-comment` declares one)** +- Spec AC #11 (logs identify the satisfied outcome or why each alternative was rejected) — **full** + +--- + +## Depends On + +- Nothing inside this spec. The Codex resume grammar fix (spec AC #9) and the build-time grammar probe (spec AC #10) were delivered ahead of this plan set in PR #1553 and PR #1554; this plan does not touch `src/backends/codex/**`. + +--- + +## Detailed Task List (TDD) + +### 1. Finish-hook flag: `pushedChangesAlternatives` + +**Tests first** (`tests/unit/agents/definitions/schema.test.ts`): + +- `accepts pushedChangesAlternatives: [pr-response] next to requiresPushedChanges: true` — unit — parse a definition whose `hooks.finish.scm` is `{ requiresPushedChanges: true, pushedChangesAlternatives: ['pr-response'] }` → parse succeeds and the value round-trips. Expected red: `ZodError: Unrecognized key(s) in object: 'pushedChangesAlternatives'` (or strip-then-`undefined`, depending on strictness — assert the round-trip so either failure mode is red). +- `rejects an unknown alternative name` — unit — `pushedChangesAlternatives: ['comment']` → parse throws mentioning `pr-response`. Expected red: parse succeeds (no enum yet). +- `rejects pushedChangesAlternatives without requiresPushedChanges: true` — unit — `{ pushedChangesAlternatives: ['pr-response'] }` alone, and with `requiresPushedChanges: false` → parse throws `pushedChangesAlternatives requires requiresPushedChanges: true`. Expected red: parse succeeds. +- `rejects an empty pushedChangesAlternatives list` — unit — `[]` → throws. Expected red: parse succeeds. + +**Tests first** (`tests/unit/agents/definitions/profiles.test.ts`): + +- `resolveFinishHooks threads pushedChangesAlternatives through` — unit — definition with the flag → `profile.finishHooks.pushedChangesAlternatives` equals `['pr-response']`; a definition without it → `undefined` (extend the existing "returns finishHooks from definition" test with the new key so the `toEqual` shape stays exhaustive). Expected red: `expected undefined to equal ['pr-response']`. + +**Implementation** (`src/agents/definitions/schema.ts`, `src/agents/definitions/profiles.ts`): +- `export const PUSHED_CHANGES_ALTERNATIVES = ['pr-response'] as const;` + `export type PushedChangesAlternative = (typeof PUSHED_CHANGES_ALTERNATIVES)[number];` +- `ScmFinishSchema` gains `pushedChangesAlternatives: z.array(z.enum(PUSHED_CHANGES_ALTERNATIVES)).min(1).optional()`. +- The refinement (`pushedChangesAlternatives` ⇒ `requiresPushedChanges === true`) is attached at the `FinishHooksSchema` level with `superRefine` so `ScmFinishSchema` stays a plain `ZodObject` (its `z.infer` feeds `FinishHookFlags`). +- `resolveFinishHooks` copies `scm?.pushedChangesAlternatives`. Nothing else in `profiles.ts` changes (`needsGitStateStopHooks` keeps keying off `requiresPushedChanges`). + +### 2. Evidence model: PR response + repository state + +**Tests first** (`tests/unit/backends/completion.test.ts`, new `describe('readCompletionEvidence — pr-response')` and `describe('readRepoState')`): + +- `reads a well-formed PR-response sidecar` — unit — sidecar `{ source: 'cascade-tools scm post-pr-comment', url: 'https://github.com/o/r/pull/7#issuecomment-1', kind: 'top-level', id: 1, prNumber: 7 }` → `evidence.hasAuthoritativePRResponse === true`, `evidence.prResponse` equals `{ url, kind: 'top-level', command: 'cascade-tools scm post-pr-comment' }`, `evidence.prResponseSidecarMalformed === false`. Expected red: `TypeError`/`expected undefined to be true`. +- `accepts the inline-reply kind` — unit — `kind: 'inline-reply'` → `prResponse.kind === 'inline-reply'`. Expected red: as above. +- `treats a sidecar without url as malformed, not as evidence` — unit — `{ kind: 'top-level' }` → `hasAuthoritativePRResponse === false`, `prResponseSidecarMalformed === true`. Expected red: `expected undefined to be false`. +- `treats an unknown kind as malformed` — unit — `{ url: '…', kind: 'edit' }` → malformed. Expected red: as above. +- `treats invalid JSON as malformed` — unit — file content `{not json` → malformed, no throw. Expected red: as above. +- `reports no PR response when the sidecar path is unset or the file is missing` — unit — `prResponseSidecarPath` undefined / nonexistent → `hasAuthoritativePRResponse === false`, `prResponseSidecarMalformed === false`. Expected red: `expected undefined to be false`. +- `readRepoState: clean tree at the initial commit` — unit — `git init` + one commit in a temp dir, `initialHeadSha` = that commit → `{ clean: true, headSha: , headUnchanged: true }`. Expected red: `readRepoState is not a function`. +- `readRepoState: uncommitted change ⇒ clean=false` — unit — write a file → `clean === false`, `headUnchanged === true`. Expected red: as above. +- `readRepoState: new commit ⇒ headUnchanged=false` — unit — second commit → `headUnchanged === false`, `clean === true`. Expected red: as above. +- `readRepoState: untracked file counts as dirty` — unit — untracked file only → `clean === false` (git status --porcelain lists `??`). Expected red: as above. +- `readRepoState: not a git repository ⇒ undefined` — unit — plain temp dir → `undefined`, no throw. Expected red: as above. +- `readCompletionEvidence attaches repoState only when repoDir and initialHeadSha are both given` — unit — requirements with `repoDir` but no `initialHeadSha` → `evidence.repoState === undefined`. Expected red: `expected to be undefined` (once `readRepoState` exists) — i.e. write this test after the reader so its red is the guard, not the missing function. + +**Implementation** (`src/backends/completion.ts`): +- `export const PR_RESPONSE_KINDS = ['top-level', 'inline-reply'] as const; export type PRResponseKind = (typeof PR_RESPONSE_KINDS)[number];` +- `export interface PRResponseEvidence { url: string; kind: PRResponseKind; command: string }` +- `export interface RepoStateEvidence { clean: boolean; headSha: string; headUnchanged: boolean }` +- `CompletionRequirements` gains `pushedChangesAlternatives?: readonly PushedChangesAlternative[]`, `prResponseSidecarPath?: string`, `repoDir?: string`, `initialHeadSha?: string` (the latter two are populated by plan 2; here they are consumed). +- `CompletionEvidence` gains `hasAuthoritativePRResponse: boolean`, `prResponse?: PRResponseEvidence`, `prResponseSidecarMalformed: boolean`, `repoState?: RepoStateEvidence`. +- `export function readRepoState(repoDir: string, initialHeadSha: string): RepoStateEvidence | undefined` — `execFileSync('git', ['status', '--porcelain'], { cwd: repoDir })` and `execFileSync('git', ['rev-parse', 'HEAD'], { cwd: repoDir })`; any thrown error → `undefined`. No shell, no interpolation. +- `readCompletionEvidence` reads the PR-response sidecar with the same `readJsonSidecar` helper; a parsed object with a non-empty string `url` and `kind ∈ PR_RESPONSE_KINDS` is evidence (`command` defaults to `'cascade-tools scm post-pr-comment'` when `source` is absent); a file that exists but fails that check sets `prResponseSidecarMalformed = true`. It calls `readRepoState` only when both `repoDir` and `initialHeadSha` are set. + +### 3. Evaluator: `evaluatePushedChanges` + +**Tests first** (`tests/unit/backends/completion.test.ts`, `describe('evaluatePushedChanges')`) — build `CompletionEvidence` objects directly (no filesystem): + +- `returns undefined when pushed changes are not required` — unit — `{ requiresPushedChanges: undefined }` → `undefined`. Expected red: `evaluatePushedChanges is not a function`. +- `pushed-changes evidence satisfies the requirement` — unit — `hasAuthoritativePushedChanges: true` → `{ satisfiedBy: 'pushed-changes' }`. Expected red: as above. +- `without alternatives, a PR response alone is rejected` — unit — no `pushedChangesAlternatives`, `hasAuthoritativePRResponse: true`, clean/unchanged repo → `satisfiedBy: null`, exactly one rejection `{ outcome: 'pushed-changes', reason: /no pushed-changes sidecar/ }`. Expected red: as above. +- `pr-response with a clean, unchanged repository satisfies the requirement` — unit — alternatives `['pr-response']`, response present, `repoState: { clean: true, headUnchanged: true }` → `{ satisfiedBy: 'pr-response' }`. Expected red: as above. +- `pushed-changes wins when both branches are satisfied` — unit — both true → `satisfiedBy: 'pushed-changes'`. Expected red: as above. +- `pr-response is rejected when the tree is dirty` — unit — `clean: false` → `satisfiedBy: null`; rejections contain `{ outcome: 'pr-response', reason: /uncommitted changes/ }` **and** `{ outcome: 'pushed-changes', … }`. Expected red: as above. +- `pr-response is rejected when HEAD moved` — unit — `headUnchanged: false` → reason `/HEAD moved/`. Expected red: as above. +- `pr-response is rejected when repository state is unavailable` — unit — `repoState: undefined` → reason `/repository state unavailable/`. Expected red: as above. +- `pr-response is rejected when the sidecar is malformed, naming the malformation` — unit — `prResponseSidecarMalformed: true` → reason `/malformed/`. Expected red: as above. +- `pr-response is rejected when nothing was recorded` — unit — → reason `/no PR response recorded/`. Expected red: as above. + +**Implementation** (`src/backends/completion.ts`): +- `export type PushedChangesOutcome = 'pushed-changes' | PushedChangesAlternative;` +- `export interface OutcomeRejection { outcome: PushedChangesOutcome; reason: string }` +- `export type PushedChangesEvaluation = { satisfiedBy: PushedChangesOutcome } | { satisfiedBy: null; rejections: OutcomeRejection[] }` +- `export function evaluatePushedChanges(requirements: CompletionRequirements | undefined, evidence: CompletionEvidence): PushedChangesEvaluation | undefined` + - `undefined` unless `requirements?.requiresPushedChanges`. + - Branch order: `'pushed-changes'` first, then each declared alternative in declaration order. First satisfied branch wins. When none is satisfied, return every branch's rejection reason (one per branch, in the same order). + - Reason strings are constants exported from the module (no inline literals at call sites), e.g. `REJECTION_NO_PUSH`, `REJECTION_NO_RESPONSE`, `REJECTION_RESPONSE_MALFORMED`, `REJECTION_TREE_DIRTY`, `REJECTION_HEAD_MOVED` (interpolating the two SHAs), `REJECTION_REPO_STATE_UNAVAILABLE`. + +### 4. Failure + continuation prompt + +**Tests first** (`tests/unit/backends/completion.test.ts`, `describe('getCompletionFailure — pushed-changes alternatives')`): + +- `keeps the legacy no-push error and prompt when no alternative is declared` — unit — `requiresPushedChanges: true`, no evidence → `error === COMPLETION_ERROR_NO_PUSH` and the prompt is byte-identical to today's text (assert against the exported constant introduced for it, `CONTINUATION_PROMPT_NO_PUSH`). Expected red: `expected to be ` once the alternative-aware path exists — write it first so it is green-then-guarded; its purpose is the AC #8 regression pin. +- `returns undefined when the pr-response branch is satisfied` — unit — → `undefined`. Expected red: returns the no-push failure. +- `with alternatives and no evidence, names both ways to finish` — unit — → `error === COMPLETION_ERROR_NO_PUSH_OR_RESPONSE`; prompt matches `/cascade-tools scm post-pr-comment/`, `/reply-to-review-comment/`, `/commit(,)? (and )?push/`, and `/cascade-tools session finish/`. Expected red: `error` is `COMPLETION_ERROR_NO_PUSH`. +- `with a recorded response but a modified repository, tells the session not to repeat the response` — unit — response present, `clean: false` → prompt contains the response URL, `/do not post (it|another)/i`, and `/commit and push|revert/`. Expected red: prompt lacks the URL. +- `exposes the rejections on the failure` — unit — `failure.rejections` equals the evaluator's rejection list. Expected red: `rejections` undefined. + +**Implementation** (`src/backends/completion.ts`): +- `export const COMPLETION_ERROR_NO_PUSH_OR_RESPONSE = 'Agent completed but neither authoritative pushed changes nor a substantive PR response with an unchanged repository were recorded';` +- `export const CONTINUATION_PROMPT_NO_PUSH` = the existing literal, unchanged. +- `CompletionFailure` gains `rejections?: OutcomeRejection[]`. +- `getCompletionFailure` replaces its inline pushed-changes check with `evaluatePushedChanges`; when `satisfiedBy === null`: + - no alternatives declared → legacy error + `CONTINUATION_PROMPT_NO_PUSH` (unchanged behaviour); + - alternatives declared → `COMPLETION_ERROR_NO_PUSH_OR_RESPONSE`, `rejections`, and `buildPushedChangesContinuationPrompt(evaluation, evidence)`: + - response recorded (`evidence.prResponse`) → *"CASCADE completion check failed: your PR response was already posted at {url} — do not post it again. The repository was modified after run start but no push was recorded ({reasons}). Continue from the current session: either commit and push the intended changes and then run `cascade-tools session finish`, or revert the working tree to the run-start state if the reply was the whole answer, then finish."* + - nothing recorded → *"CASCADE completion check failed: neither a PR response nor pushed changes were recorded. If the comment asked a question, reply with `cascade-tools scm post-pr-comment` (or `cascade-tools scm reply-to-review-comment` for an inline thread) without changing the repository, then run `cascade-tools session finish`. If it asked for code changes, commit and push them, then finish."* + - The ordering of the other checks (`requiresPR`, `requiresReview`, `requiresPMWrite`) is unchanged. + +### 5. Continuation loop logging + +**Tests first** (`tests/unit/backends/shared-continuationLoop.test.ts`): + +- `logs the satisfied pushed-changes outcome on completion` — unit — requirements with `requiresPushedChanges` and a pushed-changes sidecar → `logWriter` called with `'INFO'`, `/completion check passed/`, and data `{ pushedChangesOutcome: 'pushed-changes' }`. Expected red: no INFO call with that message. +- `logs pr-response as the satisfied outcome` — unit — alternatives + response sidecar + temp git repo (clean) → `{ pushedChangesOutcome: 'pr-response' }`. Expected red: as above. +- `includes per-outcome rejections in the continuation warning` — unit — alternatives, no evidence → the existing WARN carries `rejections` with two entries. Expected red: WARN data lacks `rejections`. +- `includes rejections in the exhausted-turns failure log and the result error` — unit — max turns 0 → result `error === COMPLETION_ERROR_NO_PUSH_OR_RESPONSE`, WARN/ERROR data has `rejections`. Expected red: error is the legacy string. + +**Implementation** (`src/backends/shared/continuationLoop.ts`): +- `decideContinuation` computes `evidence` once, then `failure = getCompletionFailure(requirements, evidence)`. On success, when `evaluatePushedChanges(requirements, evidence)` is defined, log `INFO` `${engineLabel} completion check passed` with `{ pushedChangesOutcome }`. On continuation and on exhaustion, add `rejections: failure.rejections` to the existing log payloads. Signature and return shape unchanged. + +### 6. Post-run gate uses the shared evaluator + +**Tests first** (`tests/unit/backends/postProcess.test.ts`): + +- `fails the result with the alternative-aware error when the evaluation rejects every outcome` — unit — `options.pushedChangesEvaluation = { satisfiedBy: null, rejections: [...] }`, `options.pushedChangesError = COMPLETION_ERROR_NO_PUSH_OR_RESPONSE` → `result.success === false`, `result.error === COMPLETION_ERROR_NO_PUSH_OR_RESPONSE`, WARN log includes `rejections`. Expected red: `TypeError` / result stays successful. +- `keeps the result successful when the evaluation is satisfied by pr-response` — unit — `{ satisfiedBy: 'pr-response' }` → unchanged. Expected red: n/a-green is acceptable only after the previous test is red-then-green; assert the log names the outcome so it has its own red (`expected logger.info to have been called with pushedChangesOutcome`). +- `leaves the result alone when no evaluation is supplied` — unit — legacy call shape → unchanged (pin for engines/paths that pass nothing). Expected red: none expected; keep as regression pin. + +Also in `tests/unit/backends/adapter.test.ts` (owned here): extend the existing post-processing expectation so the adapter passes `pushedChangesEvaluation` derived from `evaluatePushedChanges(executionPlan.completionRequirements, evidence)` only when `pushedChangesSidecarPath` is defined (mirrors the current `hasAuthoritativePushedChanges` guard). Expected red: `expected postProcessResult to have been called with objectContaining({ pushedChangesEvaluation })`. + +**Implementation** (`src/backends/postProcess.ts`, `src/backends/adapter.ts`): +- `postProcessResult` options: replace `hasAuthoritativePushedChanges?: boolean` with `pushedChangesEvaluation?: PushedChangesEvaluation` and `pushedChangesError?: string`; the pushed-changes block becomes `if (options?.requiresPushedChanges && result.success && options.pushedChangesEvaluation?.satisfiedBy === null) { warn(…rejections); result.success = false; result.error = options.pushedChangesError ?? COMPLETION_ERROR_NO_PUSH }`. On a satisfied evaluation, `logger.info('… completion outcome', { pushedChangesOutcome })`. +- `adapter.ts` passes `pushedChangesEvaluation: evaluatePushedChanges(requirements, evidence)` (only when `pushedChangesSidecarPath !== undefined`) and `pushedChangesError: resolvePushedChangesError(requirements)` — a small exported helper in `completion.ts` that `pushedChangesFailure` shares, so the adapter never picks up a PR/review failure's error by mistake. Everything else it passes stays as is. +- `secretOrchestrator.ts` forwards `profile.finishHooks.pushedChangesAlternatives` into the `completionRequirements` object so the worker-side evaluator and the adapter's error resolution see the declared alternatives (plan 2 adds `prResponseSidecarPath`, `repoDir` and `initialHeadSha`). + +--- + +## Test Plan + +### Unit tests +- [ ] `tests/unit/agents/definitions/schema.test.ts`: 4 tests — flag parsing, enum, refinement, non-empty +- [ ] `tests/unit/agents/definitions/profiles.test.ts`: 1 test (+ shape extension) — pass-through +- [ ] `tests/unit/backends/completion.test.ts`: ~26 tests — sidecar reading (6), repo state (6), evaluator (10), failure/prompt (5) +- [ ] `tests/unit/backends/shared-continuationLoop.test.ts`: 4 tests — outcome + rejection logging +- [ ] `tests/unit/backends/postProcess.test.ts`: 3 tests — evaluator-driven gate +- [ ] `tests/unit/backends/adapter.test.ts`: 1 extension — evaluation wiring + +### Integration tests +- [ ] none required — repo-state evidence is exercised against real temp git repositories in the unit project (same approach as `tests/unit/gadgets/session/core/finish-real-git.test.ts`). + +### Acceptance tests +- [ ] Per-plan ACs 1–7 below map one-to-one onto the unit groups above. + +--- + +## Manual Verification (for `[manual]`-tagged ACs only) + +*n/a — all ACs auto-tested.* + +--- + +## Acceptance Criteria (per-plan, testable) + +1. A definition may declare `hooks.finish.scm.pushedChangesAlternatives: [pr-response]` only alongside `requiresPushedChanges: true`; any other shape (unknown name, empty list, missing/false `requiresPushedChanges`) is rejected at load time. (spec AC #8) +2. `readCompletionEvidence` exposes PR-response evidence only from a sidecar with a non-empty `url` and a known `kind`; any other existing file is reported as malformed, and repository state is attached only when both `repoDir` and `initialHeadSha` are supplied. (spec AC #5) +3. `evaluatePushedChanges` returns `pushed-changes` for authoritative push evidence, `pr-response` only for a recorded response with `clean && headUnchanged`, and otherwise `satisfiedBy: null` with one reason per declared branch; alternatives that are not declared are never consulted. (spec ACs #3, #5, #8) +4. `getCompletionFailure` is byte-for-byte unchanged for profiles without alternatives, and for profiles with alternatives returns the `NO_PUSH_OR_RESPONSE` error, the rejections, and a continuation prompt that names the recorded response URL and forbids re-posting whenever a response exists. (spec ACs #2, #6, #8) +5. `decideContinuation` logs the satisfied pushed-changes outcome on success and the per-outcome rejections on continuation and exhaustion. (spec AC #11) +6. `postProcessResult` fails a run only through the shared evaluation and reports the same rejections; the adapter no longer computes a separate pushed-changes verdict. (spec ACs #7, #11) +7. No file under `src/backends/completion.ts`, `continuationLoop.ts`, or `postProcess.ts` references an agent type by name (grep guard in `completion.test.ts`: source must not match `/respond-to-pr-comment|prComment/`). (spec strategic decision 4) +8. All new/modified code has corresponding tests. +9. `npm run build` passes. +10. `npm test` passes. +11. `npm run lint` and `npm run typecheck` pass. +12. All documentation listed in Documentation Impact has been updated. + +**Partial-state criterion:** +- No built-in agent declares `pushedChangesAlternatives` after this plan; `evaluatePushedChanges` therefore only ever evaluates the `pushed-changes` branch at runtime, and every agent's completion behaviour is observably unchanged (pinned by the legacy-prompt test and the existing continuation-loop suite). + +--- + +## Documentation Impact (this plan only) + +| File | Change | +|---|---| +| `CHANGELOG.md` | *Unreleased → Changed*: the completion contract can express pushed-changes alternatives (`pushedChangesAlternatives`); evaluation is shared, fail-closed, and logs the satisfied outcome or every rejection. Note that no built-in agent opts in yet. | + +Architecture docs (`04-agent-system.md`, `05-engine-backends.md`, `adding-engines.md`) are updated in plan 3 once the behaviour is live for an agent; documenting a dormant flag would describe nothing an operator can observe. + +--- + +## Out of Scope (this plan) + +- Producing PR-response evidence (sidecar writers, env var, `session finish` acceptance) — plan 2. +- Opting `respond-to-pr-comment` in, its prompt guidance, end-to-end tests, architecture docs — plan 3. +- Spec-level exclusions: comment pre-classification, changes to other agents' completion contracts, prompt/model/iteration changes, workflow engines, Codex redesign beyond continuation compatibility, historical-run retries, schema changes, credential/redaction work. + +--- + +## Progress + + +- divergence (autonomous run, task 6): the adapter's fail-closed path needs `pushedChangesAlternatives` on `completionRequirements`, so this plan forwards it in `secretOrchestrator.ts` (one line) and takes that line's ownership from plan 2; the shared real-git helper `tests/helpers/tempGitRepo.ts` was added here for the repo-state tests; plan 2's Summary, Depends On and task 4 text were adjusted to match. +- review round 1 (autonomous run, fresh-context `z:local-review` gate): 0 must-fix. Applied should-fix: `readRepoState` git calls carry a 10 s timeout (fail-closed on a wedged git); the CLI-spawn test budgets live once in `tests/helpers/cliSpawnBudget.ts` with the per-test budget strictly above the spawn budget; both new helpers are indexed in `tests/README.md`. Applied suggestions: `applyCompletionEvidence` reads only the PR sidecar (no double git shell-out per turn), `RepoStateEvidence` carries `initialHeadSha` so outcome checks are pure functions of evidence, rejection-reason constants pin the evaluator tests, `getCompletionFailure` no longer re-narrows requirements. Deferred: returning the evaluation from `getCompletionFailure` to avoid the success-path re-evaluation (💡, cosmetic); consolidating the three near-identical CLI spawn wrappers (🕰️ pre-existing, adhoc candidate); folding `hasUncommittedChanges` onto `readRepoState` (noted in plan 2, task 5). +- [x] AC #1 +- [x] AC #2 +- [x] AC #3 +- [x] AC #4 +- [x] AC #5 +- [x] AC #6 +- [x] AC #7 +- [x] AC #8 +- [x] AC #9 +- [x] AC #10 +- [x] AC #11 +- [x] AC #12 +- verification (autonomous run): `npm run lint`, `npm run typecheck` and the four unit projects green (624 files / 11275 tests, run with `--maxWorkers=4` because an unrelated vitest+docker run on the host pushed load average past 80 and timed out CLI-spawn tests at vitest's 5 s default); the three `cascade-tools` spawn tests now carry a per-test budget equal to their 30 s spawn budget (separate `test(cli)` commit, outside this plan's file list, per the house rule that flaky equals failing). diff --git a/docs/plans/025-pr-comment-completion-and-codex-resume/2-pr-response-evidence.md b/docs/plans/025-pr-comment-completion-and-codex-resume/2-pr-response-evidence.md new file mode 100644 index 00000000..bd44efba --- /dev/null +++ b/docs/plans/025-pr-comment-completion-and-codex-resume/2-pr-response-evidence.md @@ -0,0 +1,255 @@ +--- +id: 025 +slug: pr-comment-completion-and-codex-resume +plan: 2 +plan_slug: pr-response-evidence +level: plan +parent_spec: docs/specs/025-pr-comment-completion-and-codex-resume.md +depends_on: [1-completion-alternatives.md] +status: pending +--- + +# 025/2: PR-response evidence — producers, plumbing, and `session finish` + +> Part 2 of 3 in the 025-pr-comment-completion-and-codex-resume plan. Parent spec: resolve `docs/specs/025-pr-comment-completion-and-codex-resume.md*` (it may have been renamed `.done`). + +## Summary + +Plan 1 defined what counts as a comment-only completion; this plan makes the system produce that evidence and lets the agent finish on it. `cascade-tools scm post-pr-comment` and `cascade-tools scm reply-to-review-comment` record a successful new response in a PR-response sidecar (`CASCADE_PR_RESPONSE_SIDECAR_PATH`), while `cascade-tools scm update-pr-comment` records nothing — editing the acknowledgment or a progress comment is never a substantive response. The sidecar path is created and injected only for profiles that declare the `pr-response` alternative, and its env var is allowlisted so it actually reaches engine subprocesses. The in-process gadgets used by the SDK engine record the same fact in session state so `Finish` behaves identically there. + +`cascade-tools session finish` (and the in-process `Finish` gadget) learn the same rule the shared evaluator applies: when the profile declares the alternative, a recorded response with a clean tree, no unpushed commits, and HEAD still at the run-start SHA is a valid finish; in that case the pushed-changes sidecar is **not** written, because nothing was pushed. Otherwise the strict push path is unchanged, with rejection messages that now name both ways to finish. `secretOrchestrator` populates the remaining new `CompletionRequirements` fields (`prResponseSidecarPath`, `repoDir`, `initialHeadSha`; plan 1 already forwards `pushedChangesAlternatives`) so the worker-side evaluation from plan 1 runs on real data. + +Still dormant for users: no built-in profile declares the alternative until plan 3. + +**Components delivered:** +- `src/gadgets/sessionState.ts` — `PR_RESPONSE_SIDECAR_ENV_VAR`, `recordPRResponse`, `getPRResponse` +- `src/backends/shared/envFilter.ts` — allowlist entry +- `src/gadgets/session/core/sidecar.ts` — `writePRResponseSidecar` +- `src/cli/scm/post-pr-comment.ts`, `src/cli/scm/reply-to-review-comment.ts` — sidecar writes after success +- `src/gadgets/github/PostPRComment.ts`, `src/gadgets/github/ReplyToReviewComment.ts` — session-state records +- `src/backends/sidecarManager.ts`, `src/backends/secretOrchestrator.ts` — path creation, env injection, requirement fields +- `src/gadgets/session/core/finish.ts`, `src/cli/session/finish.ts`, `src/gadgets/Finish.ts` — comment-only finish acceptance + +**Files owned (exclusive to this plan within this spec):** +- `src/gadgets/sessionState.ts` +- `src/backends/shared/envFilter.ts` +- `src/gadgets/session/core/sidecar.ts` +- `src/gadgets/session/core/finish.ts` +- `src/cli/session/finish.ts` +- `src/gadgets/Finish.ts` +- `src/cli/scm/post-pr-comment.ts` +- `src/cli/scm/reply-to-review-comment.ts` +- `src/gadgets/github/PostPRComment.ts` +- `src/gadgets/github/ReplyToReviewComment.ts` +- `src/backends/sidecarManager.ts` +- `src/backends/secretOrchestrator.ts` +- `tests/unit/gadgets/sessionState.test.ts` +- `tests/unit/backends/shared-envFilter.test.ts` +- `tests/unit/gadgets/session/core/sidecar.test.ts` +- `tests/unit/gadgets/session/core/finish.test.ts` +- `tests/unit/gadgets/session/core/finish-real-git.test.ts` +- `tests/unit/cli/session/finish.test.ts` +- `tests/unit/cli/scm/post-pr-comment-sidecar.test.ts` (new) +- `tests/unit/cli/scm/reply-to-review-comment-sidecar.test.ts` (new) +- `tests/unit/cli/scm/scm-commands.test.ts` +- `tests/unit/gadgets/github/` posting-gadget tests (new file `pr-response-record.test.ts`) +- `tests/unit/backends/sidecarManager.test.ts` + +**Shared surfaces (append-only, conflicts are trivial):** +- `CHANGELOG.md` (one block under *Unreleased → Changed*) + +**Deferred to later plans in this spec:** +- The profile opt-in, prompt guidance, end-to-end proof and architecture docs — plan 3. + +--- + +## Spec ACs satisfied by this plan + +- Spec AC #1 (question → completes without commit) — **partial (this plan: the response is recorded and `session finish` accepts it; plan 3 opts the agent in and proves the run completes)** +- Spec AC #2 (changed files, not pushed → no completion) — **partial (this plan: `session finish` rejects a dirty/moved repository even with a recorded response, with an actionable reason; plan 1 gates the worker side; plan 3 proves it)** +- Spec AC #4 (editing the acknowledgment/progress comment does not count) — **full** +- Spec AC #6 (no duplicate response on continuation) — **partial (this plan: the sidecar survives across turns so the plan-1 prompt can name it; plan 3 asserts end to end)** +- Spec AC #7 (identical across native-tool engines) — **partial (this plan: one env var, one sidecar shape, one `validateFinish`; the in-process gadgets mirror it for SDK parity)** + +--- + +## Depends On + +- Plan 1 (completion-alternatives) — provides `PushedChangesAlternative`, the `CompletionRequirements` fields (`pushedChangesAlternatives`, `prResponseSidecarPath`, `repoDir`, `initialHeadSha`), `PR_RESPONSE_KINDS` / `PRResponseKind`, `readCompletionEvidence` reading the response sidecar (reused by the `session finish` CLI), and the shared real-git test helper `tests/helpers/tempGitRepo.ts`. + +--- + +## Detailed Task List (TDD) + +### 1. Session state + env plumbing + +**Tests first** (`tests/unit/gadgets/sessionState.test.ts`): + +- `PR_RESPONSE_SIDECAR_ENV_VAR is CASCADE_PR_RESPONSE_SIDECAR_PATH` — unit — Expected red: `undefined`. +- `recordPRResponse / getPRResponse round-trip` — unit — record `{ url, kind: 'inline-reply' }` → `getPRResponse()` returns it; fresh state returns `null`. Expected red: `recordPRResponse is not a function`. +- `initSessionState resets the recorded response` — unit — record, re-init → `null`. Expected red: stale value survives. + +**Tests first** (`tests/unit/backends/shared-envFilter.test.ts`): the existing constant sweep ("every constant in `sessionState.ts` is allowlisted") goes red on its own once the new constant exists — `expected CASCADE_PR_RESPONSE_SIDECAR_PATH to be allowlisted`. Keep it as the red; add the explicit import to the test's constant list so the sweep stays exhaustive. + +**Implementation** (`src/gadgets/sessionState.ts`, `src/backends/shared/envFilter.ts`): +- `export const PR_RESPONSE_SIDECAR_ENV_VAR = 'CASCADE_PR_RESPONSE_SIDECAR_PATH';` +- `SessionStateData.prResponse: { url: string; kind: PRResponseKind } | null` (reset in `initSessionState`), `recordPRResponse(url, kind)`, `getPRResponse()`. +- Add `PR_RESPONSE_SIDECAR_ENV_VAR` to `SHARED_ALLOWED_ENV_EXACT` in the sidecar block. + +### 2. Sidecar writer + +**Tests first** (`tests/unit/gadgets/session/core/sidecar.test.ts`): + +- `writePRResponseSidecar writes the contract shape` — unit — path + `{ url, kind: 'top-level', id: 5, prNumber: 7, command: 'cascade-tools scm post-pr-comment' }` → file equals `{ source: command, url, kind, id, prNumber }` and returns `true`. Expected red: `writePRResponseSidecar is not a function`. +- `refuses to write without a url` — unit — `url: ''` → returns `false`, no file, `logger.error` called. Expected red: as above. +- `refuses an unknown kind` — unit — `kind: 'edit'` → `false`. Expected red: as above. +- `returns false quietly when the path is unset` — unit — `undefined` path → `false` and **no WARN** (only DEBUG) — unlike the other writers, most agents legitimately run without this path. Expected red: as above. + +**Implementation** (`src/gadgets/session/core/sidecar.ts`): +- `export function writePRResponseSidecar(sidecarPath: string | undefined, response: { url: string; kind: PRResponseKind; id: number; prNumber: number | null; command: string }): boolean` — validates `url` non-empty and `kind ∈ PR_RESPONSE_KINDS` (imported from `completion.ts`), writes `JSON.stringify({ source: command, url, kind, id, prNumber })`, DEBUG when unset, WARN on write failure. Overwrites: the latest successful response is the recorded one (plan 1 only needs "at least one"). + +### 3. Producers: CLI commands and in-process gadgets + +**Tests first** (`tests/unit/cli/scm/post-pr-comment-sidecar.test.ts`, mirroring `create-pr-sidecar.test.ts`): + +- `writes the PR-response sidecar with kind top-level after a successful post` — unit — mock `postPRComment` to resolve `{ id: 11, url: 'https://…#issuecomment-11', repoFullName, prNumber: 7 }`, set `CASCADE_PR_RESPONSE_SIDECAR_PATH` → file has `kind: 'top-level'`, `source: 'cascade-tools scm post-pr-comment'`, `id: 11`, `prNumber: 7`. Expected red: file missing. +- `does not write when the post fails` — unit — core rejects → no file, command exits non-zero as before. Expected red: n/a until the writer is wired; assert the file is absent so a naïve "always write" implementation goes red. + +(`tests/unit/cli/scm/reply-to-review-comment-sidecar.test.ts`): same two tests with `kind: 'inline-reply'` and `source: 'cascade-tools scm reply-to-review-comment'`. + +(`tests/unit/cli/scm/scm-commands.test.ts`): `update-pr-comment leaves the PR-response sidecar untouched` — unit — env var set, run the command with a mocked successful update → no file. This negative assertion guards a live boundary (the editing tool stays reachable and must never count). Expected red: n/a — it must be green from the start and stay green; pair it with the two positive tests so the suite proves the asymmetry. + +(`tests/unit/gadgets/github/pr-response-record.test.ts`): `PostPRComment gadget records a top-level response in session state` and `ReplyToReviewComment gadget records an inline reply` — unit — mock the core, run the gadget, `getPRResponse()` returns `{ url, kind }`. Expected red: `null`. + +**Implementation** (`src/cli/scm/post-pr-comment.ts`, `src/cli/scm/reply-to-review-comment.ts`, `src/gadgets/github/PostPRComment.ts`, `src/gadgets/github/ReplyToReviewComment.ts`): +- CLI handlers: after the core resolves, `writePRResponseSidecar(process.env[PR_RESPONSE_SIDECAR_ENV_VAR], { url: result.url, kind, id: result.id, prNumber: result.prNumber, command })` (same pattern as `create-pr.ts`). `result.url` may be undefined in the mutation-result type — treat that as "no evidence" (writer refuses) rather than throwing; the comment still posted. +- Gadgets: `recordPRResponse(result.url, kind)` after success. `update-pr-comment.ts` / `UpdatePRComment.ts` untouched. + +### 4. Sidecar path creation and requirement fields + +**Tests first** (`tests/unit/backends/sidecarManager.test.ts`): + +- `creates the PR-response sidecar path only for profiles declaring the pr-response alternative` — unit — profile `finishHooks: { requiresPushedChanges: true, pushedChangesAlternatives: ['pr-response'] }`, `needsNativeToolRuntime: true` → returned `prResponseSidecarPath` is a temp path and `projectSecrets.CASCADE_PR_RESPONSE_SIDECAR_PATH` equals it. Expected red: `prResponseSidecarPath` undefined. +- `does not create it for a plain requiresPushedChanges profile` — unit — → `undefined`, env var absent. Expected red: n/a (green pin). +- `does not create it without a native-tool runtime` — unit — `needsNativeToolRuntime: false` → `undefined`. Expected red: n/a (green pin, mirrors the other sidecars). + +Existing `secretOrchestrator` coverage lives in `tests/unit/backends/adapter.test.ts` (plan 1 file) — so pin the requirement fields here instead: `tests/unit/backends/sidecarManager.test.ts` gains `buildCompletionRequirements maps sidecar paths, alternatives, repoDir and initialHeadSha` — unit — extract the object literal at `secretOrchestrator.ts:220` into `buildCompletionRequirements(profile, artifacts, repoDir, input)` in `sidecarManager.ts` and assert the mapping (`pushedChangesAlternatives` from the profile, `repoDir` as given, `initialHeadSha === input.headSha`, `maxContinuationTurns: 2`). Expected red: `buildCompletionRequirements is not a function`. + +**Implementation** (`src/backends/sidecarManager.ts`, `src/backends/secretOrchestrator.ts`): +- `createCompletionArtifacts` adds `prResponseSidecarPath` (`cascade-pr-response-sidecar--.json`) when `needsNativeToolRuntime && profile.finishHooks.pushedChangesAlternatives?.includes('pr-response')`, injects the env var, returns it. +- `export function buildCompletionRequirements(...)`: returns the `CompletionRequirements` object (moved from `secretOrchestrator.ts`) with the remaining three new fields (`prResponseSidecarPath`, `repoDir`, `initialHeadSha`); plan 1 already forwards `pushedChangesAlternatives`. `initialHeadSha` is `input.headSha` — the same value the CLI already receives as `CASCADE_INITIAL_HEAD_SHA`, so the worker-side and CLI-side "HEAD unchanged" checks agree by construction. (Deriving it from `git rev-parse HEAD` would newly enforce the no-new-commits rule on PM-triggered runs — a behaviour change outside this spec.) +- `secretOrchestrator.ts` calls the helper; nothing else changes there. + +### 5. `session finish` accepts the comment-only outcome + +**Tests first** (`tests/unit/gadgets/session/core/finish.test.ts` — mocked git helpers, and `finish-real-git.test.ts` — real temp repo): + +- `accepts a recorded PR response when the tree is clean, nothing is unpushed, and HEAD is unchanged` — unit (mocked) — hooks `{ requiresPushedChanges: true, pushedChangesAlternatives: ['pr-response'] }`, `prResponse: { url, kind }`, `initialHeadSha` set, `hasUncommittedChanges → false`, `hasUnpushedCommits → false`, `hasNewCommits → false` → `{ valid: true, pushedChangesOutcome: 'pr-response' }`. Expected red: `{ valid: false, error: /without making any changes/ }`. +- `reports pushed-changes as the outcome when new commits were pushed` — unit — `hasNewCommits → true` → `{ valid: true, pushedChangesOutcome: 'pushed-changes' }`. Expected red: `pushedChangesOutcome` undefined. +- `rejects a recorded response when the tree is dirty, naming both ways out` — unit — `hasUncommittedChanges → true` → `valid: false`, error matches `/commit and push/` and `/revert/`. Expected red: legacy message without `revert`. +- `rejects a recorded response when HEAD moved without a push` — unit — `hasNewCommits → true`, `hasUnpushedCommits → true` → `valid: false`, error `/push/`. Expected red: n/a (already rejects) — assert the message still mentions the response was kept, i.e. `/already posted/`, for its own red. +- `does not accept the alternative without an initial HEAD SHA` — unit — `initialHeadSha: null` → strict path (fail closed): with no new commits detectable the legacy behaviour applies (`valid: true` only if the legacy checks pass) and `pushedChangesOutcome === 'pushed-changes'`; document in the test why. Expected red: `'pr-response'` reported. +- `ignores a recorded response when the profile declares no alternative` — unit — `prResponse` set, hooks `{ requiresPushedChanges: true }`, no new commits → `valid: false` with the legacy `no_new_commits` error. Expected red: n/a (green pin for spec AC #8). +- `rejects with the two-way message when nothing was recorded and nothing changed` — unit — alternatives declared, `prResponse: null`, no changes → `valid: false`, error `/post-pr-comment/` and `/commit and push/`. Expected red: legacy message. +- (`finish-real-git.test.ts`) `comment-only finish in a real repository` — unit — temp repo at commit A, `initialHeadSha = A`, response recorded → `valid: true, pushedChangesOutcome: 'pr-response'`; then `touch file` → `valid: false`. Expected red: first assertion fails with the legacy error. + +(`tests/unit/cli/session/finish.test.ts`): +- `passes the PR-response sidecar into validation and skips the pushed-changes sidecar for a comment-only finish` — unit — env `CASCADE_PR_RESPONSE_SIDECAR_PATH` → a valid sidecar, mocked `validateFinish` returning `{ valid: true, pushedChangesOutcome: 'pr-response' }` → `validateFinish` received `prResponse: { url, kind }` and `writePushedChangesSidecar` was **not** called. Expected red: `writePushedChangesSidecar` called. +- `writes the pushed-changes sidecar for a pushed-changes finish` — unit — outcome `'pushed-changes'` → called once (regression pin). Expected red: n/a. + +(`tests/unit/gadgets/session/core/finish.test.ts` or a small `tests/unit/gadgets/Finish.test.ts` if absent): the in-process `Finish` gadget passes `getPRResponse()` into `validateFinish` and applies the same sidecar rule. Expected red: `validateFinish` called without `prResponse`. + +**Implementation** (`src/gadgets/session/core/finish.ts`, `src/cli/session/finish.ts`, `src/gadgets/Finish.ts`): +- `SessionState` gains `prResponse?: { url: string; kind: PRResponseKind } | null`. +- `FinishValidationSuccess` becomes `{ valid: true; pushedChangesOutcome?: 'pushed-changes' | 'pr-response' }`. +- `checkPushedChangesHook(state)` returns `FinishValidationError | { pushedChangesOutcome }`: + 1. If `state.hooks.pushedChangesAlternatives?.includes('pr-response') && state.prResponse && state.initialHeadSha` and `!hasUncommittedChanges() && !hasUnpushedCommits(prBranch) && !hasNewCommits(initialHeadSha)` → `logger.info('[Finish] comment-only outcome accepted', { url, kind })`, outcome `'pr-response'`. + 2. Otherwise the existing three checks run unchanged in order; when alternatives are declared, the `uncommitted_changes` and `no_new_commits` messages are replaced by the two-way texts (constants: `FINISH_ERROR_DIRTY_WITH_RESPONSE`, `FINISH_ERROR_NOTHING_TO_FINISH`); when a response is recorded the message begins with "Your PR response was already posted at {url} — do not post it again." A passing strict path reports `'pushed-changes'`. +- `validateFinish` propagates the outcome. +- While here, make `hasUncommittedChanges` delegate to plan 1's `readRepoState` (`src/backends/completion.ts`) so the tree keeps one clean-tree check instead of a third ad-hoc `git status --porcelain` (plan 1 review suggestion). `cli/session/finish.ts` reads `readCompletionEvidence({ prResponseSidecarPath: process.env[PR_RESPONSE_SIDECAR_ENV_VAR], … }).prResponse` into the state and writes the pushed-changes sidecar only when `result.pushedChangesOutcome === 'pushed-changes'`. `gadgets/Finish.ts` does the same with `getPRResponse()`. + +--- + +## Test Plan + +### Unit tests +- [ ] `tests/unit/gadgets/sessionState.test.ts`: 3 tests — constant, record/get, reset +- [ ] `tests/unit/backends/shared-envFilter.test.ts`: existing sweep + 1 explicit entry +- [ ] `tests/unit/gadgets/session/core/sidecar.test.ts`: 4 tests — writer contract +- [ ] `tests/unit/cli/scm/post-pr-comment-sidecar.test.ts`: 2 tests +- [ ] `tests/unit/cli/scm/reply-to-review-comment-sidecar.test.ts`: 2 tests +- [ ] `tests/unit/cli/scm/scm-commands.test.ts`: 1 test — update never records +- [ ] `tests/unit/gadgets/github/pr-response-record.test.ts`: 2 tests — in-process records +- [ ] `tests/unit/backends/sidecarManager.test.ts`: 4 tests — path creation + requirement mapping +- [ ] `tests/unit/gadgets/session/core/finish.test.ts`: 7 tests — acceptance/rejection matrix +- [ ] `tests/unit/gadgets/session/core/finish-real-git.test.ts`: 1 test — real repo +- [ ] `tests/unit/cli/session/finish.test.ts`: 2 tests — sidecar rule +- [ ] in-process `Finish` gadget: 1 test + +### Integration tests +- [ ] none required — GitHub calls are mocked at the core boundary exactly as the existing sidecar tests do. + +### Acceptance tests +- [ ] Per-plan ACs 1–7 map onto the groups above. + +--- + +## Manual Verification (for `[manual]`-tagged ACs only) + +*n/a — all ACs auto-tested.* + +--- + +## Acceptance Criteria (per-plan, testable) + +1. A successful `cascade-tools scm post-pr-comment` writes `{ source, url, kind: 'top-level', id, prNumber }` to `CASCADE_PR_RESPONSE_SIDECAR_PATH`; `reply-to-review-comment` writes `kind: 'inline-reply'`; a failed post writes nothing; `update-pr-comment` never writes it. (spec AC #4) +2. The in-process `PostPRComment` / `ReplyToReviewComment` gadgets record the same fact in session state. (spec AC #7) +3. The env var is allowlisted and the sidecar path is created and injected only for native-tool runs of profiles that declare `pr-response`; `CompletionRequirements` carries `pushedChangesAlternatives`, `prResponseSidecarPath`, `repoDir`, and `initialHeadSha === input.headSha`. (spec AC #1) +4. `validateFinish` accepts a recorded response only with a clean tree, no unpushed commits, an unchanged HEAD, and an initial SHA to compare against, reporting `pushedChangesOutcome: 'pr-response'`; every other case follows the strict path and reports `'pushed-changes'` when it passes. (spec ACs #1, #2) +5. With alternatives declared, rejection messages name both ways to finish and, when a response exists, state its URL and that it must not be posted again; without alternatives the messages are unchanged. (spec ACs #2, #6, #8) +6. `session finish` (CLI and gadget) writes the pushed-changes sidecar only for the `'pushed-changes'` outcome. +7. Running any built-in agent still creates no PR-response sidecar path (no profile opts in yet). +8. All new/modified code has corresponding tests. +9. `npm run build` passes. +10. `npm test` passes. +11. `npm run lint` and `npm run typecheck` pass. +12. All documentation listed in Documentation Impact has been updated. + +**Partial-state criterion:** +- The two posting commands write a sidecar only when `CASCADE_PR_RESPONSE_SIDECAR_PATH` is set, and nothing sets it for a built-in agent yet; `session finish` only takes the new branch when a profile declares `pr-response`. Observable behaviour for every agent is unchanged. + +--- + +## Documentation Impact (this plan only) + +| File | Change | +|---|---| +| `CHANGELOG.md` | *Unreleased → Changed*: `post-pr-comment` / `reply-to-review-comment` record authoritative response evidence when `CASCADE_PR_RESPONSE_SIDECAR_PATH` is set; `session finish` accepts a comment-only outcome for profiles that declare `pr-response`. | + +The `cascade-tools` flag/usage surface is unchanged, so `src/gadgets/README.md` and the CLI help need no edit. + +--- + +## Out of Scope (this plan) + +- Opting `respond-to-pr-comment` in, its prompt guidance, end-to-end tests, architecture docs — plan 3. +- Judging whether a posted comment is "substantive" by content — the spec counts any successful new top-level comment or inline reply (strategic decision 2). +- Spec-level exclusions as listed in plan 1. + +--- + +## Progress + + +- [ ] AC #1 +- [ ] AC #2 +- [ ] AC #3 +- [ ] AC #4 +- [ ] AC #5 +- [ ] AC #6 +- [ ] AC #7 +- [ ] AC #8 +- [ ] AC #9 +- [ ] AC #10 +- [ ] AC #11 +- [ ] AC #12 diff --git a/docs/plans/025-pr-comment-completion-and-codex-resume/3-pr-comment-opt-in.md b/docs/plans/025-pr-comment-completion-and-codex-resume/3-pr-comment-opt-in.md new file mode 100644 index 00000000..286f8158 --- /dev/null +++ b/docs/plans/025-pr-comment-completion-and-codex-resume/3-pr-comment-opt-in.md @@ -0,0 +1,180 @@ +--- +id: 025 +slug: pr-comment-completion-and-codex-resume +plan: 3 +plan_slug: pr-comment-opt-in +level: plan +parent_spec: docs/specs/025-pr-comment-completion-and-codex-resume.md +depends_on: [2-pr-response-evidence.md] +status: pending +--- + +# 025/3: PR-comment agent opts in — guidance, end-to-end proof, documentation + +> Part 3 of 3 in the 025-pr-comment-completion-and-codex-resume plan. Parent spec: resolve `docs/specs/025-pr-comment-completion-and-codex-resume.md*` (it may have been renamed `.done`). + +## Summary + +This plan turns the dormant machinery on for the one agent the spec targets. `respond-to-pr-comment.yaml` declares `pushedChangesAlternatives: [pr-response]`, and its task prompt tells the agent exactly how to satisfy either outcome: answer a question with `cascade-tools scm post-pr-comment` (or `reply-to-review-comment` inside an inline thread) without touching the repository and then `session finish`; make requested code changes, commit, push, then finish. No other definition changes. + +The plan also carries the proof that the whole chain works: an end-to-end test drives the shared continuation loop with the real profile flags, the real sidecar writers, and a real temporary git repository, covering the question path (completes with no commit), the modified-but-unpushed path (does not complete; actionable reason; the recorded response is named and not requested again), the pushed-changes path, malformed evidence, and the fact that every other pushed-changes agent still fails without a push. A structural test pins that all three native-tool engines go through the shared loop. Finally, the architecture documentation describes the response-or-push contract and evidence-driven continuation, and `adding-engines.md` gains the requirement that continuation-capable engines preserve session identity and pass the model-free grammar check that PR #1554 introduced. + +**Components delivered:** +- `src/agents/definitions/respond-to-pr-comment.yaml` — opt-in + guidance +- `tests/unit/agents/definitions/respond-to-pr-comment.yaml.test.ts` — profile contract (new) +- `tests/unit/backends/pr-comment-completion.test.ts` — end-to-end (new) +- `tests/unit/backends/native-tool-continuation-parity.test.ts` — engine parity (new) +- `docs/architecture/04-agent-system.md`, `docs/architecture/05-engine-backends.md`, `docs/adding-engines.md`, `docs/areas/agents.md`, `CHANGELOG.md` + +**Files owned (exclusive to this plan within this spec):** +- `src/agents/definitions/respond-to-pr-comment.yaml` +- `tests/unit/agents/definitions/respond-to-pr-comment.yaml.test.ts` +- `tests/unit/backends/pr-comment-completion.test.ts` +- `tests/unit/backends/native-tool-continuation-parity.test.ts` +- `docs/architecture/04-agent-system.md` +- `docs/architecture/05-engine-backends.md` +- `docs/adding-engines.md` +- `docs/areas/agents.md` + +**Shared surfaces (append-only, conflicts are trivial):** +- `CHANGELOG.md` (one block under *Unreleased → Fixed*) + +**Deferred to later plans in this spec:** +- none — this is the last plan. + +--- + +## Spec ACs satisfied by this plan + +- Spec AC #1 (question → run completes with no commit) — **partial (this plan: opt-in + end-to-end proof; plans 1 and 2 supply evaluation and evidence)** +- Spec AC #2 (changed files, not pushed → continues or fails with an actionable pushed-change reason) — **partial (this plan: end-to-end proof)** +- Spec AC #3 (pushed changes complete under the strict path) — **partial (this plan: end-to-end reconfirmation; plan 1 holds the unit pin)** +- Spec AC #6 (continuation does not post the response again) — **partial (this plan: task guidance + end-to-end prompt assertion)** +- Spec AC #7 (behaviour identical across native-tool engines) — **partial (this plan: structural parity test)** +- Spec AC #8 (no other agent inherits the alternative) — **partial (this plan: definition sweep pins that only `respond-to-pr-comment` declares it)** + +--- + +## Depends On + +- Plan 2 (pr-response-evidence) — sidecar writers, env var, `session finish` acceptance, requirement plumbing. +- Plan 1 (completion-alternatives) — evaluator, prompt, logging (transitively). + +--- + +## Detailed Task List (TDD) + +### 1. Profile opt-in and guidance + +**Tests first** (`tests/unit/agents/definitions/respond-to-pr-comment.yaml.test.ts`, shaped like `review.yaml.test.ts`): + +- `declares the pr-response alternative next to requiresPushedChanges` — unit — `loadBuiltinDefinition('respond-to-pr-comment')` → `hooks.finish.scm` equals `{ requiresPushedChanges: true, blockGitPush: false, pushedChangesAlternatives: ['pr-response'] }`. Expected red: `pushedChangesAlternatives` undefined. +- `is the only built-in definition that declares an alternative` — unit — load every `src/agents/definitions/*.yaml`; exactly one has `pushedChangesAlternatives`. Expected red: n/a until the YAML changes (0 found → fails with "expected 1"), so it is red first. +- `tells the agent how to answer a question without touching the repository` — unit — YAML text matches `/cascade-tools scm post-pr-comment/`, `/reply-to-review-comment/`, `/without (changing|modifying) the repository|do not commit/i`, and `/cascade-tools session finish/`. Expected red: no match. +- `tells the agent to commit and push when code changes are requested` — unit — matches `/commit.*push/` and `/session finish/`. Expected red: partial match only (the push sentence exists today; the finish sentence does not). + +**Implementation** (`src/agents/definitions/respond-to-pr-comment.yaml`): +- Under `hooks.finish.scm`: add `pushedChangesAlternatives: [pr-response]`. +- Task prompt: replace the final paragraph with explicit two-outcome guidance — question → reply via `cascade-tools scm post-pr-comment` (top-level) or `cascade-tools scm reply-to-review-comment` (when the comment is in an inline review thread), leave the working tree untouched, then `cascade-tools session finish`; code change → make surgical changes, commit, push, then `cascade-tools session finish`. State once that a reply is posted exactly once and never re-posted after a continuation. + +### 2. End-to-end proof through the shared loop + +**Tests first** (`tests/unit/backends/pr-comment-completion.test.ts`): each test builds a temp git repo (commit A), resolves the real profile via `getAgentProfile('respond-to-pr-comment')`, builds requirements with plan 2's `buildCompletionRequirements` (real sidecar paths in a temp dir), and drives `runContinuationLoop` with an `executeTurn` stub that performs the side effects a real agent would (calling the real `writePRResponseSidecar` / `writePushedChangesSidecar`, committing in the repo): + +- `a question answered by a new comment completes on the first turn with no commit` — acceptance — turn 1 writes the response sidecar → result `success: true`, `executeTurn` called once, repo HEAD still A, INFO log `pushedChangesOutcome: 'pr-response'`. Expected red: `success: false` with the no-push error (before plan 3 opts in, the profile has no alternative — this test goes red on the YAML, not on code). +- `a modified but unpushed repository does not complete even with a posted comment` — acceptance — turn 1 writes the response sidecar and dirties the tree, `maxContinuationTurns: 1`, turn 2 does nothing → `success: false`, `error === COMPLETION_ERROR_NO_PUSH_OR_RESPONSE`, the turn-2 prompt contains the response URL and `/do not post/i`, and `executeTurn` called exactly twice. Expected red: turn-2 prompt lacks the URL / error is the legacy string. +- `the continuation prompt never asks for a second response once one is recorded` — acceptance — same setup; assert the turn-2 prompt does **not** match `/post-pr-comment/` as an instruction to post (it may appear inside the "already posted" sentence only — assert with the exact prompt constant from plan 1). Expected red: prompt asks to post. +- `requested code changes complete through the strict pushed-changes path` — acceptance — turn 1 commits B and writes the pushed-changes sidecar → `success: true`, log `pushedChangesOutcome: 'pushed-changes'`. Expected red: n/a (green pin for spec AC #3). +- `a malformed response sidecar satisfies nothing and the reason says so` — acceptance — turn 1 writes `{not json` → after exhaustion `success: false`, rejections include `/malformed/`. Expected red: rejections undefined. +- `every other pushed-changes agent still fails without a push` — acceptance — for each built-in definition with `requiresPushedChanges: true` other than `respond-to-pr-comment` (`implementation`, `respond-to-ci`, `respond-to-review`, `resolve-conflicts`, plus any future one — enumerate from disk), write a response sidecar, clean repo, no push → `success: false`, `error === COMPLETION_ERROR_NO_PUSH`. Expected red: n/a (green pin for spec AC #8; enumerating from disk makes a future opt-in show up here). + +**Tests first** (`tests/unit/backends/native-tool-continuation-parity.test.ts`): + +- `every native-tool engine drives turns through runContinuationLoop` — unit — for each engine directory `claude-code`, `codex`, `opencode`, the `index.ts` source imports `runContinuationLoop` from `../shared/continuationLoop.js` and calls it; the completion decision therefore has exactly one implementation. Expected red: n/a today (all three already do) — this is the guard for spec AC #7; it must fail if an engine grows its own loop. + +**Implementation:** none beyond section 1 — these tests exercise plans 1–3 together. If any goes red for a reason other than the YAML, the defect is in plan 1 or 2 and must be fixed there (edit that plan in place per the divergence rule). + +### 3. Documentation + +**Tests first** (`tests/unit/architecture-docs.test.ts` already guards `docs/areas/*` ≤ 60 lines, no ticket IDs/dates): the `docs/areas/agents.md` bullet below must keep that suite green. No new test. + +**Implementation:** +- `docs/architecture/04-agent-system.md` — *Finish hooks* table: add `scm.pushedChangesAlternatives` ("outcomes that satisfy `requiresPushedChanges` instead of a push; `pr-response` = a new top-level PR comment or inline reply, recorded by `cascade-tools scm post-pr-comment` / `reply-to-review-comment`, with a clean tree and HEAD unchanged since run start"). Add a short *Response-or-push completion* subsection: which evidence counts, that editing the ack/progress comment never counts, fail-closed evaluation, and that only `respond-to-pr-comment` opts in. +- `docs/architecture/05-engine-backends.md` — under *Continuation loop*: evidence-driven continuation (sidecars + repository state), the `pushedChangesOutcome` log line, per-outcome rejections, and the rule that a continuation prompt names an already-recorded response so a resumed session never repeats it. The Codex resume grammar sentence added by PR #1553 stays. +- `docs/adding-engines.md` — checklist item: a continuation-capable engine must resume the same session on continuation turns (never start a fresh one) and must pass the model-free grammar probe (`dist/backends/codex/grammarSmokeCli.js` pattern) at image-build time when it is subprocess-based. +- `docs/areas/agents.md` — one bullet: completion alternatives are declared per profile in YAML; never add agent-name conditionals to `src/backends/completion.ts`. +- `CHANGELOG.md` — *Unreleased → Fixed*: "A PR-comment run that answers a question now completes without an artificial commit" — user-visible behaviour, how it is evidenced, what still fails, and the log line operators can grep. + +--- + +## Test Plan + +### Unit tests +- [ ] `tests/unit/agents/definitions/respond-to-pr-comment.yaml.test.ts`: 4 tests — opt-in, uniqueness, guidance +- [ ] `tests/unit/backends/native-tool-continuation-parity.test.ts`: 1 test — engine parity guard + +### Integration tests +- [ ] none — the end-to-end tests below use real git and real sidecar writers but mock nothing external, so they run in the unit project. + +### Acceptance tests +- [ ] `tests/unit/backends/pr-comment-completion.test.ts`: 6 tests — spec ACs #1, #2, #3, #5, #6, #8 end to end + +--- + +## Manual Verification (for `[manual]`-tagged ACs only) + +*n/a — all ACs auto-tested.* + +--- + +## Acceptance Criteria (per-plan, testable) + +1. `respond-to-pr-comment` is the only built-in definition declaring `pushedChangesAlternatives`, and its value is `[pr-response]`. (spec AC #8) +2. Its task prompt names both finishing paths (reply tool + untouched repository + `session finish`; commit + push + `session finish`) and says a reply is posted once. (spec ACs #1, #6) +3. Driving the shared loop with the real profile, real sidecar writers and a real repository: a question path completes with HEAD unchanged; a dirty/unpushed path does not complete and its continuation prompt names the recorded response and does not request another; a pushed path completes; a malformed sidecar is rejected with a reason naming the malformation. (spec ACs #1, #2, #3, #5, #6) +4. Every other pushed-changes agent fails without a push even when a response sidecar exists. (spec AC #8) +5. All three native-tool engines route turns through `runContinuationLoop`. (spec AC #7) +6. `04-agent-system.md`, `05-engine-backends.md`, `adding-engines.md`, `docs/areas/agents.md` and `CHANGELOG.md` describe the contract as listed above, and `tests/unit/architecture-docs.test.ts` stays green. +7. All new/modified code has corresponding tests. +8. `npm run build` passes. +9. `npm test` passes. +10. `npm run lint` and `npm run typecheck` pass. +11. All documentation listed in Documentation Impact has been updated. + +--- + +## Documentation Impact (this plan only) + +| File | Change | +|---|---| +| `docs/architecture/04-agent-system.md` | Finish-hooks table row for `pushedChangesAlternatives`; new *Response-or-push completion* subsection. | +| `docs/architecture/05-engine-backends.md` | Evidence-driven continuation semantics, outcome log line, rejections, no-repeat rule. | +| `docs/adding-engines.md` | Continuation-capable engines: preserve session identity; subprocess engines pass a model-free grammar probe at image build. | +| `docs/areas/agents.md` | One bullet: alternatives are declared in YAML; no agent-name conditionals in completion code. | +| `CHANGELOG.md` | *Fixed* entry for comment-only PR-comment completion. | + +--- + +## Out of Scope (this plan) + +- Comment-only completion for any agent other than `respond-to-pr-comment`. +- Pre-classifying comments as question vs. change request before dispatch. +- Dashboard changes, schema changes, historical-run retries, prompt/model/iteration changes beyond the guidance paragraph above. +- Spec-level exclusions as listed in plan 1. + +--- + +## Progress + + +- [ ] AC #1 +- [ ] AC #2 +- [ ] AC #3 +- [ ] AC #4 +- [ ] AC #5 +- [ ] AC #6 +- [ ] AC #7 +- [ ] AC #8 +- [ ] AC #9 +- [ ] AC #10 +- [ ] AC #11 diff --git a/docs/plans/025-pr-comment-completion-and-codex-resume/_coverage.md b/docs/plans/025-pr-comment-completion-and-codex-resume/_coverage.md new file mode 100644 index 00000000..9909f3e7 --- /dev/null +++ b/docs/plans/025-pr-comment-completion-and-codex-resume/_coverage.md @@ -0,0 +1,67 @@ +# Coverage map for spec 025-pr-comment-completion-and-codex-resume + +Auto-generated by /plan. Tracks which plans satisfy which spec ACs. + +## Spec ACs + +| # | Spec AC (short) | Satisfied by | Status | +|---|---|---|---| +| 1 | Question → substantive response, clean/unchanged repo → run completes with no commit | plan 1 (evaluator) + plan 2 (evidence + `session finish`) + plan 3 (opt-in + e2e) | partial chain | +| 2 | Changed but uncommitted/unpushed files → no completion; actionable pushed-change reason | plan 1 (repo-state gate + prompt) + plan 2 (`session finish` rejects) + plan 3 (e2e) | partial chain | +| 3 | Committed and pushed changes complete under the strict path | plan 1 (regression pin) + plan 3 (e2e reconfirmation) | full (pinned in 1, reconfirmed in 3) | +| 4 | Editing the acknowledgment/progress comment does not count as the response | plan 2 (only `post-pr-comment` / `reply-to-review-comment` record; `update-pr-comment` never does) | full | +| 5 | Missing / malformed / inconsistent evidence satisfies no alternative; actionable reason | plan 1 (fail-closed evaluator, malformed detection, per-outcome reasons) | full | +| 6 | A response recorded before a continuation is not posted again | plan 1 (continuation prompt names it) + plan 2 (sidecar persists across turns) + plan 3 (guidance + e2e) | partial chain | +| 7 | Behaviour identical across all native-tool engines | plan 1 (single evaluator in the shared loop) + plan 2 (single env var / sidecar / `validateFinish`) + plan 3 (parity guard) | partial chain | +| 8 | Push-requiring agents keep failing without a push; none inherit the alternative | plan 1 (schema refinement, evaluator consults declared alternatives only, legacy-prompt pin) + plan 3 (definition sweep + e2e over every other agent) | partial chain | +| 9 | Codex continuation with all options reaches the existing session (no parser error) | delivered ahead of this plan set — PR #1553 (`buildArgs` ordering + structural tests, verified against the pinned CLI) | delivered (pre-plan) | +| 10 | Incompatible Codex CLI grammar fails worker-image validation before use | delivered ahead of this plan set — PR #1554 (`Dockerfile.worker` grammar probe, CI + deploy image builds) | delivered (pre-plan) | +| 11 | Run logs identify the satisfied outcome or why each alternative was rejected | plan 1 (continuation-loop + post-process logging) | full | + +## Coverage summary + +- **11 spec ACs** mapped: **9** to **3 plans**, **2** (#9, #10) already delivered by PR #1553 / PR #1554 before planning +- **3 spec ACs** with full coverage from a single plan (#4, #5, #11) plus #3 (pinned in plan 1, reconfirmed in plan 3) +- **5 spec ACs** with partial-chain coverage requiring downstream plans to complete (#1, #2, #6, #7, #8) +- **0 `[manual]` ACs** +- **0 spec ACs** unmapped + +## Plan dependency graph + +``` +1-completion-alternatives ──→ 2-pr-response-evidence ──→ 3-pr-comment-opt-in +``` + +Linear DAG, no cycles. Topological order = plan order (1 → 2 → 3). No two plans share a source or test file; `CHANGELOG.md` is the only shared (append-only) surface. + +**Concurrency:** none. Plan 2 consumes plan 1's types (`PushedChangesAlternative`, the new `CompletionRequirements` fields, `PR_RESPONSE_KINDS`) and reader; plan 3 consumes plan 2's writers and requirement builder. Both edges protect real artifacts. + +## Documentation impact mapping + +| Spec doc item | Plan | +|---|---| +| `04-agent-system.md` — alternative completion outcomes, response-or-push contract | plan 3 | +| `05-engine-backends.md` — evidence-driven continuation semantics | plan 3 (the Codex resume command hierarchy sentence landed in PR #1553) | +| `adding-engines.md` — session identity + model-free compatibility check requirement | plan 3 | +| `CHANGELOG.md` — comment-only completion | plan 3 (plans 1 and 2 add their own *Changed* blocks); Codex continuation compatibility entries landed in PR #1553 / PR #1554 | +| `CLAUDE.md` | no change (per spec) | + +## Notes + +- **Pre-plan deliveries.** The Codex half of the spec was hot-fixed before planning because it was failing a production deployment: PR #1553 reorders `codex exec` options ahead of the `resume` subcommand (spec AC #9, strategic decision 7) and PR #1554 adds the build-time grammar probe plus the Codex 0.155.1 pin (spec AC #10, strategic decision 8). Plans 1–3 do not touch `src/backends/codex/**`. +- **Non-standard-spec note:** n/a — the spec follows the /specify template. + +### Assumed decisions (autonomous run — Phase 3/4 sign-off gates stated, not asked) + +The operator ran this planning pass unattended, so the decomposition and interview decisions below were taken on the recommended option and are recorded here for audit. Any of them can be overturned by editing the plan files in place before `/implement`. + +1. **Three serial plans, not one.** Seams: shared evaluation model (1) → evidence producers and `session finish` (2) → profile opt-in, e2e proof, docs (3). Each is reviewable alone and leaves every agent's behaviour unchanged until 3. A single plan would put the fail-closed evaluator, CLI plumbing across nine files, and prompt changes in one review. +2. **Alternatives are expressed as `scm.pushedChangesAlternatives: [pr-response]` next to `requiresPushedChanges: true`.** The existing flag keeps driving sidecar creation, git-state stop hooks and env plumbing; only the evaluation points learn about the OR-group. A load-time refinement rejects the flag without `requiresPushedChanges: true`, so no agent can inherit the alternative implicitly (spec AC #8, strategic decisions 1 and 4). Rejected: a top-level `anyOf` group replacing the boolean flags — it would have forced every `requiresPushedChanges` consumer to re-derive "may need push evidence". +3. **"Substantive response" = a successful new top-level PR comment or inline review reply**, recorded by the two posting tools; content is not judged, and `update-pr-comment` never records (spec strategic decision 2, AC #4). +4. **Repository state is re-checked worker-side**, not trusted from the CLI: `readCompletionEvidence` runs `git status --porcelain` and `git rev-parse HEAD` in `repoDir` and compares against `initialHeadSha`; `session finish` applies the same rule so the agent gets the rejection immediately. Any git failure or missing SHA fails the alternative closed (spec strategic decision 6). +5. **`initialHeadSha` is `input.headSha`** (the PR head at dispatch, already exported to the CLI as `CASCADE_INITIAL_HEAD_SHA`) rather than a fresh `git rev-parse HEAD` at run start, so both sides compare against the same SHA and no new "must have new commits" rule appears for PM-triggered runs. +6. **In-process gadgets (`PostPRComment`, `ReplyToReviewComment`) record the response in session state** so the SDK engine's `Finish` gadget behaves like `cascade-tools session finish`; the spec's engine-parity AC names native-tool engines, and this costs two lines. +7. **`pushed-changes` wins when both branches are satisfied** (a push is the more authoritative outcome), and the pushed-changes sidecar is written only for that outcome. +8. **`postProcessResult` consumes the shared evaluator** instead of keeping its own pushed-changes verdict, so the adapter and the continuation loop cannot drift (spec AC #7). +9. **Architecture docs land in plan 3**, once an agent actually exhibits the behaviour; plans 1 and 2 add CHANGELOG blocks only. +10. **Test distribution:** each test lives in the plan that delivers the code under test; the end-to-end suite lives in plan 3 because only then does the real profile exercise the chain. diff --git a/docs/specs/025-pr-comment-completion-and-codex-resume.md b/docs/specs/025-pr-comment-completion-and-codex-resume.md new file mode 100644 index 00000000..3db77ea4 --- /dev/null +++ b/docs/specs/025-pr-comment-completion-and-codex-resume.md @@ -0,0 +1,142 @@ +--- +id: 025 +slug: pr-comment-completion-and-codex-resume +level: spec +title: Outcome-aware PR comment completion and reliable Codex resume +created: 2026-08-25 +status: draft +--- + +# 025: Outcome-aware PR comment completion and reliable Codex resume + +## Problem & Motivation + +The PR-comment agent handles two legitimately different kinds of work: it may answer a question by posting a comment, or it may change code and push commits. Its task guidance already distinguishes those outcomes, but its completion contract does not. Every run currently requires a newly pushed commit, so a successful comment-only response is rejected even when the requested external action completed. + +This mismatch triggered a second latent defect in a production run. After the agent successfully posted its answer, the completion gate detected no pushed-change evidence and attempted to continue the same Codex session. The continuation command placed parent execution options after the resume subcommand and session identifier. The pinned Codex CLI rejected that grammar before contacting the model, and the dashboard recorded the whole run as failed despite the requested response already being visible on the pull request. + +The two failures compound but need separate corrections. Completion must recognize the authoritative outcome that actually satisfies a mixed-purpose task, while remaining fail-closed when neither valid outcome occurred. Codex continuation must also obey the pinned CLI's command grammar so a legitimate missing-evidence continuation can reach the existing session instead of failing in argument parsing. + +--- + +## Goals + +1. A PR-comment run that successfully posts a substantive response without changing the repository completes successfully without requiring an artificial commit. +2. A PR-comment run that changes repository state still requires clean, authoritative pushed-change evidence before it can complete. +3. Completion requirements can express alternative valid outcomes without weakening the strict requirements of other agent types. +4. Successful external responses are recorded durably enough that continuation does not duplicate them. +5. Codex continuation turns preserve the original session and accept all configured execution options under the pinned CLI grammar. +6. Operators can determine which completion outcome succeeded or why no valid outcome matched from normal run logs. + +--- + +## Non-goals + +- Pre-classifying every incoming comment as a question or code-change request before agent dispatch. +- Changing the completion contracts of review-response, CI-response, conflict-resolution, implementation, review, or planning agents. +- Changing agent models, prompts, iteration budgets, or update-channel behavior. +- Replacing the native-tool continuation loop or introducing a general workflow engine. +- Redesigning the Codex integration beyond continuation command compatibility. +- Retrying or rewriting historical failed runs automatically. +- Credential rotation, historical-log cleanup, or general secret-redaction work. + +--- + +## Constraints + +- Missing, malformed, or contradictory completion evidence must fail closed. +- A comment-only outcome is valid only when a new substantive PR response was recorded successfully, the working tree is clean, and the repository HEAD is unchanged from run start. +- Editing a pre-existing acknowledgment or progress comment does not count as the substantive response. +- A changed repository can complete only through the existing authoritative pushed-change path. +- Existing strict completion behavior remains backward compatible for every agent that does not opt into alternative outcomes. +- Continuation must not repeat an external response already recorded as successful. +- Codex command construction must remain compatible with the version pinned in the worker image, and version upgrades must surface incompatible grammar before deployment. +- The change must use existing completion-artifact and process-launch infrastructure; no new runtime dependency is justified. +- All behavior changes follow test-first development. + +--- + +## User stories / Requirements + +1. **As a pull-request participant asking a question**, I receive a direct answer and see the automation run complete successfully without an unrelated code commit. +2. **As a pull-request participant requesting code changes**, I do not see the automation run complete until its repository changes are committed and pushed. +3. **As an operator**, I can distinguish a run completed by a substantive response from one completed by pushed changes. +4. **As an operator**, I see a precise failure when neither valid outcome has authoritative evidence, rather than a generic or misleading pushed-change error. +5. **As an operator**, I do not see duplicate responses when the engine receives a continuation turn after the first response was already posted. +6. **As an engine maintainer**, I can add an agent with multiple valid completion outcomes without adding agent-name conditionals to shared completion logic. +7. **As an engine maintainer upgrading Codex**, I receive an automated compatibility failure if continuation arguments no longer match the pinned CLI grammar. + +--- + +## Research Notes + +- OpenAI documents non-interactive continuation as `codex exec resume ` and describes execution controls as options of the parent `exec` command. Command construction must preserve that hierarchy rather than treating every option as valid after `resume`. [OpenAI Codex non-interactive mode](https://developers.openai.com/codex/non-interactive-mode) +- Codex's Rust command parser follows clap's subcommand model. Clap documents that parent arguments propagate into child subcommands only when they are declared global; ordinary parent-only arguments cannot be moved below a subcommand boundary. [clap argument documentation](https://docs.rs/clap/latest/clap/struct.Arg.html) +- AWS Step Functions models mixed workflows with explicit choice branches selected from current state. When no branch matches and there is no default, execution fails instead of silently claiming success. This maps well to fail-closed alternative completion outcomes. [AWS Step Functions Choice state](https://docs.aws.amazon.com/step-functions/latest/dg/state-choice.html) +- JSON Schema's `anyOf` represents the relevant validation semantics: one or more complete alternatives may satisfy the contract, unlike `allOf`, which requires every branch. Completion alternatives should remain explicit groups rather than weakening individual evidence checks. [JSON Schema composition](https://json-schema.org/understanding-json-schema/reference/combining) +- Temporal recommends durable records for external side effects and idempotent retry behavior because work may complete before the orchestrator records completion. A successful PR response therefore needs authoritative evidence that survives an engine continuation. [Temporal Activity guidance](https://docs.temporal.io/activity-definition) +- Exit status alone is insufficient for this orchestration layer: a subprocess can exit successfully while the requested workflow outcome remains unsatisfied, or a later continuation can fail after an earlier external action succeeded. Completion must be derived from outcome evidence and then mapped to the final run status. [GitHub Actions exit-code semantics](https://docs.github.com/actions/creating-actions/setting-exit-codes-for-actions) + +--- + +## Open Source Decisions + +| Tool | License | Solves | Decision | Reason | +|---|---|---|---|---| +| [OpenAI Codex CLI](https://github.com/openai/codex) | Apache-2.0 | Non-interactive agent execution and session resume | **Keep and pin** | It is already the supported engine. The defect is command construction, not missing CLI capability. | +| [XState](https://stately.ai/docs/machines) | MIT | General state-machine modeling with guarded transitions | **Skip** | Alternative completion validation is a small extension of existing infrastructure; a runtime state-machine dependency would add disproportionate surface area. | +| [Temporal](https://temporal.io/) | MIT client SDKs; server under its published license | Durable workflows, side-effect history, retries | **Reference only** | Its evidence and idempotency principles are useful, but adopting a workflow platform is far outside this correction's scope. | +| JSON Schema composition | Specification | Declarative AND/OR/XOR validation semantics | **Copy the model, not a library** | The existing completion contract can represent explicit alternatives without adding runtime schema evaluation. | + +--- + +## Strategic decisions + +1. **Model completion as explicit alternative outcomes.** A mixed-purpose agent may declare multiple complete success paths; satisfying any one path is sufficient. Rejected: removing the push requirement entirely. Reason: that would allow code-changing work to finish unpushed. +2. **Require authoritative response evidence for comment-only success.** A successful new top-level PR response or inline review reply counts; editing an acknowledgment does not. The repository must also remain clean and at its initial HEAD. Rejected: trusting final prose or a clean checkout alone. Reason: neither proves that the requested human-facing response occurred. +3. **Keep pushed-change evidence strict.** If repository state changes, only the existing clean-and-pushed outcome may complete the run. Rejected: allowing a posted comment to mask local or unpushed changes. Reason: it would strand incomplete code in the worker. +4. **Make the completion abstraction shared but opt-in.** Alternative outcomes are engine-agnostic and contain no PR-comment-agent conditional; only the PR-comment profile opts in initially. Rejected: a one-off special case. Reason: shared semantics prevent drift across native-tool engines. +5. **Persist external-response evidence and make continuation duplicate-safe.** Once a substantive response succeeds, later completion checks reuse that evidence and continuation guidance must not ask for another response. Rejected: inferring success from engine text or transient tool logs. Reason: both are non-authoritative and may be lost or duplicated across turns. +6. **Fail closed on evidence uncertainty.** Missing, malformed, or mismatched response and push evidence satisfies no branch. Rejected: best-effort success. Reason: a false success is worse than a visible, actionable failure. +7. **Treat Codex CLI grammar as a versioned integration contract.** Parent execution options remain before the resume subcommand; resume identity and follow-up input remain in the subcommand's argument domain. Rejected: suppressing the parser error or starting a fresh session. Reason: both lose continuation correctness. +8. **Verify grammar at two levels.** Focused tests lock the generated argument structure, and a model-free parser smoke check exercises the pinned CLI during worker-image validation. Rejected: mocks alone. Reason: the existing mock asserted the malformed ordering and could not detect the real parser rejection. +9. **Add no dependency.** Existing completion artifacts, process invocation, and worker validation are sufficient. Rejected: adding a state-machine or command-builder library. Reason: neither would materially improve this narrowly bounded contract. + +--- + +## Acceptance Criteria (outcome-level) + +1. When a PR-comment run receives a question, successfully posts a new substantive response, and leaves the repository clean and unchanged, the dashboard records the run as completed without any new commit. +2. When a PR-comment run changes repository files but has not committed and pushed those changes, posting a response does not allow completion; the run continues or fails with an actionable pushed-change reason. +3. When a PR-comment run commits and pushes requested code changes, authoritative pushed-change evidence completes the run under the existing strict guarantees. +4. Updating only the system-created acknowledgment or progress comment does not satisfy the substantive-response outcome. +5. Missing, malformed, or inconsistent response evidence satisfies no completion alternative and produces an actionable failure reason in run logs. +6. If a substantive response was recorded before a continuation, the continuation does not post the same response again. +7. The PR-comment completion behavior is identical across all native-tool engines that support the agent. +8. Agents that require pushed changes today continue to fail when they produce no new pushed commit; none inherit the comment-only alternative implicitly. +9. A Codex continuation with repository, model, sandbox, output, and configuration options reaches the requested existing session without an argument-parser error. +10. An incompatible Codex CLI argument grammar causes worker-image validation to fail before an affected image can execute project work. +11. Run logs identify the completion alternative that succeeded, or identify why each available alternative was rejected, without relying on the engine's prose claim. + +--- + +## Documentation Impact (high-level) + +- `docs/architecture/04-agent-system.md` — describe alternative completion outcomes and the PR-comment agent's response-or-push contract. +- `docs/architecture/05-engine-backends.md` — document evidence-driven continuation semantics and the Codex resume command hierarchy. +- `docs/adding-engines.md` — add the requirement that continuation-capable engines preserve session identity and pass a model-free compatibility check. +- `CHANGELOG.md` — record the corrected comment-only completion behavior and Codex continuation compatibility fix. +- No `CLAUDE.md` change: these rules have clear homes in engine and agent architecture documentation and are derivable from the completion contract. + +--- + +## Out of Scope + +- General classification of incoming PR comments before dispatch. +- Comment-only completion for agents other than the PR-comment agent. +- Changes to communication-channel policy or tool availability. +- A dashboard redesign or new completion-outcome UI. +- Database schema changes or historical run backfills. +- Automatic retry of runs that failed before this change. +- General command-line abstraction work across other engines. +- Credential rotation, stored-log deletion, and general-purpose secret redaction. diff --git a/src/agents/definitions/profiles.ts b/src/agents/definitions/profiles.ts index 009b73df..005291e3 100644 --- a/src/agents/definitions/profiles.ts +++ b/src/agents/definitions/profiles.ts @@ -89,6 +89,7 @@ function resolveFinishHooks(def: AgentDefinition): FinishHookFlags { requiresPR: scm?.requiresPR, requiresReview: scm?.requiresReview, requiresPushedChanges: scm?.requiresPushedChanges, + pushedChangesAlternatives: scm?.pushedChangesAlternatives, blockGitPush: scm?.blockGitPush, requiresPMWrite: pm?.requiresPMWrite, }; diff --git a/src/agents/definitions/schema.ts b/src/agents/definitions/schema.ts index b4a417f7..bccf436d 100644 --- a/src/agents/definitions/schema.ts +++ b/src/agents/definitions/schema.ts @@ -226,10 +226,19 @@ const TrailingHooksSchema = z.object({ }); // --- Finish hook schemas --- +/** + * Outcomes that may satisfy `requiresPushedChanges` instead of a push. + * `pr-response`: a new top-level PR comment or inline review reply was recorded and the + * repository is clean and still at the run-start HEAD. + */ +export const PUSHED_CHANGES_ALTERNATIVES = ['pr-response'] as const; +export type PushedChangesAlternative = (typeof PUSHED_CHANGES_ALTERNATIVES)[number]; + const ScmFinishSchema = z.object({ requiresPR: z.boolean().optional(), requiresReview: z.boolean().optional(), requiresPushedChanges: z.boolean().optional(), + pushedChangesAlternatives: z.array(z.enum(PUSHED_CHANGES_ALTERNATIVES)).min(1).optional(), blockGitPush: z.boolean().optional(), }); @@ -237,10 +246,20 @@ const PmFinishSchema = z.object({ requiresPMWrite: z.boolean().optional(), }); -const FinishHooksSchema = z.object({ - scm: ScmFinishSchema.optional(), - pm: PmFinishSchema.optional(), -}); +const FinishHooksSchema = z + .object({ + scm: ScmFinishSchema.optional(), + pm: PmFinishSchema.optional(), + }) + .superRefine((hooks, ctx) => { + if (hooks.scm?.pushedChangesAlternatives && hooks.scm.requiresPushedChanges !== true) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ['scm', 'pushedChangesAlternatives'], + message: 'pushedChangesAlternatives requires requiresPushedChanges: true', + }); + } + }); // --- Lifecycle hook schema --- /** diff --git a/src/backends/adapter.ts b/src/backends/adapter.ts index 2aef2ca2..fbe22638 100644 --- a/src/backends/adapter.ts +++ b/src/backends/adapter.ts @@ -6,7 +6,11 @@ import { finalizeEngineRun, tryUpdateRunPlanResolution } from '../agents/shared/ import { createAgentLogger } from '../agents/utils/logging.js'; import { recordInitialComment } from '../gadgets/sessionState.js'; import type { AgentInput, AgentResult, CascadeConfig, ProjectConfig } from '../types/index.js'; -import { readCompletionEvidence } from './completion.js'; +import { + evaluatePushedChanges, + readCompletionEvidence, + resolvePushedChangesError, +} from './completion.js'; import { postProcessResult } from './postProcess.js'; import { createProgressMonitor } from './progress.js'; import { buildProgressMonitorConfig, isGitHubAckComment } from './progressLifecycle.js'; @@ -181,7 +185,8 @@ export async function executeWithEngine( } // biome-ignore lint/style/noNonNullAssertion: result is always defined when execute() did not throw await hydrateNativeToolSidecars(result!, prSidecarPath, reviewSidecarPath); - const completionEvidence = readCompletionEvidence(executionPlan.completionRequirements); + const completionRequirements = executionPlan.completionRequirements ?? {}; + const completionEvidence = readCompletionEvidence(completionRequirements); postProcessResult(result, agentType, engine, input, identifier, { requiresPR: profile.finishHooks.requiresPR, @@ -189,10 +194,11 @@ export async function executeWithEngine( requiresPushedChanges: profile.finishHooks.requiresPushedChanges, requiresPMWrite: profile.finishHooks.requiresPMWrite, hasAuthoritativeReview: completionEvidence.hasAuthoritativeReview, - hasAuthoritativePushedChanges: + pushedChangesEvaluation: pushedChangesSidecarPath !== undefined - ? completionEvidence.hasAuthoritativePushedChanges + ? evaluatePushedChanges(completionRequirements, completionEvidence) : undefined, + pushedChangesError: resolvePushedChangesError(completionRequirements), hasPMWrite: pmWriteSidecarPath !== undefined ? completionEvidence.hasPMWrite : undefined, }); } finally { diff --git a/src/backends/completion.ts b/src/backends/completion.ts index bd97bfd5..fd7e4863 100644 --- a/src/backends/completion.ts +++ b/src/backends/completion.ts @@ -1,5 +1,7 @@ +import { execFileSync } from 'node:child_process'; import { existsSync, readFileSync } from 'node:fs'; +import type { PushedChangesAlternative } from '../agents/definitions/schema.js'; import type { AgentEngineResult } from './types.js'; export const COMPLETION_ERROR_NO_PR = @@ -11,18 +13,60 @@ export const COMPLETION_ERROR_NO_PUSH = export const COMPLETION_ERROR_NO_PM_WRITE = 'Agent completed but no PM write (checklist creation) was recorded'; +/** Kinds of PR response that count as a substantive reply: a new top-level comment or an inline review reply. */ +export const PR_RESPONSE_KINDS = ['top-level', 'inline-reply'] as const; +export type PRResponseKind = (typeof PR_RESPONSE_KINDS)[number]; +const DEFAULT_PR_RESPONSE_COMMAND = 'cascade-tools scm post-pr-comment'; +/** A wedged git must fail the comment-only outcome closed, not block the worker turn. */ +const GIT_STATE_TIMEOUT_MS = 10_000; + +export const REJECTION_NO_PUSH = + 'no pushed-changes sidecar (cascade-tools session finish never recorded a push)'; +export const REJECTION_NO_RESPONSE = + 'no PR response recorded (post-pr-comment / reply-to-review-comment never succeeded)'; +export const REJECTION_RESPONSE_MALFORMED = + 'PR response sidecar is malformed (missing url or unknown kind)'; +export const REJECTION_TREE_DIRTY = + 'PR response recorded but the working tree has uncommitted changes'; +export const REJECTION_REPO_STATE_UNAVAILABLE = + 'PR response recorded but repository state unavailable (repoDir/initialHeadSha missing or git failed)'; + +function rejectionHeadMoved(initialHeadSha: string, currentHeadSha: string): string { + return `PR response recorded but HEAD moved from ${initialHeadSha} to ${currentHeadSha} without a recorded push`; +} + export interface CompletionRequirements { requiresPR?: boolean; requiresReview?: boolean; requiresPushedChanges?: boolean; + /** Outcomes that satisfy `requiresPushedChanges` instead of a push (declared per profile). */ + pushedChangesAlternatives?: readonly PushedChangesAlternative[]; requiresPMWrite?: boolean; prSidecarPath?: string; reviewSidecarPath?: string; pushedChangesSidecarPath?: string; + prResponseSidecarPath?: string; pmWriteSidecarPath?: string; + /** Repository the run works in; with `initialHeadSha`, enables repository-state evidence. */ + repoDir?: string; + /** HEAD at run start; the comment-only outcome requires HEAD to still be here. */ + initialHeadSha?: string; maxContinuationTurns?: number; } +export interface PRResponseEvidence { + url: string; + kind: PRResponseKind; + command: string; +} + +export interface RepoStateEvidence { + clean: boolean; + headSha: string; + initialHeadSha: string; + headUnchanged: boolean; +} + export interface CompletionEvidence { hasAuthoritativePR: boolean; prUrl?: string; @@ -37,6 +81,12 @@ export interface CompletionEvidence { pushedCommand?: string; ackCommentDeleted?: boolean; hasPMWrite: boolean; + hasAuthoritativePRResponse: boolean; + prResponse?: PRResponseEvidence; + /** A PR-response sidecar exists but fails the contract — evidence of nothing. */ + prResponseSidecarMalformed: boolean; + /** Present only when both `repoDir` and `initialHeadSha` were supplied and git answered. */ + repoState?: RepoStateEvidence; } function readJsonSidecar(path: string | undefined): Record | undefined { @@ -57,11 +107,55 @@ function readStringProp( return typeof value === 'string' && value ? value : undefined; } +function isPRResponseKind(value: string | undefined): value is PRResponseKind { + return (PR_RESPONSE_KINDS as readonly string[]).includes(value ?? ''); +} + +function readPRResponseSidecar(path: string | undefined): { + response?: PRResponseEvidence; + malformed: boolean; +} { + if (!path || !existsSync(path)) return { malformed: false }; + const data = readJsonSidecar(path); + const url = readStringProp(data, 'url'); + const kind = readStringProp(data, 'kind'); + if (!url || !isPRResponseKind(kind)) return { malformed: true }; + return { + response: { url, kind, command: readStringProp(data, 'source') ?? DEFAULT_PR_RESPONSE_COMMAND }, + malformed: false, + }; +} + +export function readRepoState( + repoDir: string, + initialHeadSha: string, +): RepoStateEvidence | undefined { + const gitOptions = { + cwd: repoDir, + encoding: 'utf-8' as const, + stdio: ['ignore', 'pipe', 'ignore'] as ['ignore', 'pipe', 'ignore'], + timeout: GIT_STATE_TIMEOUT_MS, + }; + try { + const status = execFileSync('git', ['status', '--porcelain'], gitOptions); + const headSha = execFileSync('git', ['rev-parse', 'HEAD'], gitOptions).trim(); + return { + clean: status.trim().length === 0, + headSha, + initialHeadSha, + headUnchanged: headSha === initialHeadSha, + }; + } catch { + return undefined; + } +} + export function readCompletionEvidence(requirements?: CompletionRequirements): CompletionEvidence { const prSidecar = readJsonSidecar(requirements?.prSidecarPath); const reviewSidecar = readJsonSidecar(requirements?.reviewSidecarPath); const pushedChangesSidecar = readJsonSidecar(requirements?.pushedChangesSidecarPath); const pmWriteSidecar = readJsonSidecar(requirements?.pmWriteSidecarPath); + const prResponseSidecar = readPRResponseSidecar(requirements?.prResponseSidecarPath); const prUrl = readStringProp(prSidecar, 'prUrl'); const prCommand = readStringProp(prSidecar, 'source') ?? 'cascade-tools scm create-pr'; @@ -72,6 +166,10 @@ export function readCompletionEvidence(requirements?: CompletionRequirements): C const pushedHeadSha = readStringProp(pushedChangesSidecar, 'headSha'); const pushedCommand = readStringProp(pushedChangesSidecar, 'source') ?? 'cascade-tools session finish'; + const repoState = + requirements?.repoDir && requirements.initialHeadSha + ? readRepoState(requirements.repoDir, requirements.initialHeadSha) + : undefined; return { hasAuthoritativePR: Boolean(prUrl), @@ -90,12 +188,127 @@ export function readCompletionEvidence(requirements?: CompletionRequirements): C ? reviewSidecar.ackCommentDeleted : undefined, hasPMWrite: Boolean(pmWriteSidecar), + hasAuthoritativePRResponse: Boolean(prResponseSidecar.response), + prResponse: prResponseSidecar.response, + prResponseSidecarMalformed: prResponseSidecar.malformed, + repoState, }; } +export type PushedChangesOutcome = 'pushed-changes' | PushedChangesAlternative; + +export interface OutcomeRejection { + outcome: PushedChangesOutcome; + reason: string; +} + +export type PushedChangesEvaluation = + | { satisfiedBy: PushedChangesOutcome } + | { satisfiedBy: null; rejections: OutcomeRejection[] }; + +type OutcomeCheck = (evidence: CompletionEvidence) => string | undefined; + +function rejectPRResponse(evidence: CompletionEvidence): string | undefined { + if (evidence.prResponseSidecarMalformed) return REJECTION_RESPONSE_MALFORMED; + if (!evidence.prResponse) return REJECTION_NO_RESPONSE; + if (!evidence.repoState) return REJECTION_REPO_STATE_UNAVAILABLE; + if (!evidence.repoState.clean) return REJECTION_TREE_DIRTY; + if (!evidence.repoState.headUnchanged) { + return rejectionHeadMoved(evidence.repoState.initialHeadSha, evidence.repoState.headSha); + } + return undefined; +} + +/** One check per outcome, each returning the rejection reason or `undefined` when satisfied. */ +const OUTCOME_CHECKS: Record = { + 'pushed-changes': (evidence) => + evidence.hasAuthoritativePushedChanges ? undefined : REJECTION_NO_PUSH, + 'pr-response': rejectPRResponse, +}; + +/** + * Decide whether `requiresPushedChanges` is met. The push branch is checked first, then + * each declared alternative in order; the first satisfied branch wins. When none is + * satisfied every branch reports its reason, so operators can see why each was rejected. + */ +export function evaluatePushedChanges( + requirements: CompletionRequirements | undefined, + evidence: CompletionEvidence, +): PushedChangesEvaluation | undefined { + if (!requirements?.requiresPushedChanges) return undefined; + const outcomes: PushedChangesOutcome[] = [ + 'pushed-changes', + ...(requirements.pushedChangesAlternatives ?? []), + ]; + const rejections: OutcomeRejection[] = []; + for (const outcome of outcomes) { + const reason = OUTCOME_CHECKS[outcome](evidence); + if (!reason) return { satisfiedBy: outcome }; + rejections.push({ outcome, reason }); + } + return { satisfiedBy: null, rejections }; +} + +export const COMPLETION_ERROR_NO_PUSH_OR_RESPONSE = + 'Agent completed but neither authoritative pushed changes nor a substantive PR response with an unchanged repository were recorded'; + +export const CONTINUATION_PROMPT_NO_PUSH = + 'CASCADE completion check failed: no authoritative pushed changes were recorded for this task. Continue from the current session, commit and push the required changes, confirm the push succeeded, and only then finish.'; + +/** + * Continuation guidance when a profile accepts a PR response instead of a push. A response + * that is already recorded is named so the resumed session never posts it a second time. + */ +export function buildPushedChangesContinuationPrompt( + rejections: OutcomeRejection[], + evidence: CompletionEvidence, +): string { + const reasons = rejections + .map((rejection) => `${rejection.outcome}: ${rejection.reason}`) + .join('; '); + if (evidence.prResponse) { + return ( + `CASCADE completion check failed: your PR response was already posted at ${evidence.prResponse.url} — do not post it again. ` + + `It did not complete the run because: ${reasons}. Continue from the current session: either commit and push the intended changes and then run \`cascade-tools session finish\`, ` + + 'or revert the working tree to the run-start state if the reply was the whole answer, then finish.' + ); + } + return ( + `CASCADE completion check failed: neither a PR response nor pushed changes were recorded (${reasons}). ` + + 'If the comment asked a question, reply with `cascade-tools scm post-pr-comment` (or `cascade-tools scm reply-to-review-comment` for an inline thread) without changing the repository, then run `cascade-tools session finish`. ' + + 'If it asked for code changes, commit and push them, then finish.' + ); +} + export interface CompletionFailure { error: string; continuationPrompt: string; + /** Why each pushed-changes outcome was rejected (present only for that requirement). */ + rejections?: OutcomeRejection[]; +} + +/** The error a run records when every pushed-changes outcome is rejected. */ +export function resolvePushedChangesError( + requirements: Pick, +): string { + return requirements.pushedChangesAlternatives?.length + ? COMPLETION_ERROR_NO_PUSH_OR_RESPONSE + : COMPLETION_ERROR_NO_PUSH; +} + +function pushedChangesFailure( + pushedChangesAlternatives: readonly PushedChangesAlternative[] | undefined, + evidence: CompletionEvidence, + rejections: OutcomeRejection[], +): CompletionFailure { + const continuationPrompt = pushedChangesAlternatives?.length + ? buildPushedChangesContinuationPrompt(rejections, evidence) + : CONTINUATION_PROMPT_NO_PUSH; + return { + error: resolvePushedChangesError({ pushedChangesAlternatives }), + continuationPrompt, + rejections, + }; } export function getCompletionFailure( @@ -118,12 +331,13 @@ export function getCompletionFailure( }; } - if (requirements?.requiresPushedChanges && !evidence.hasAuthoritativePushedChanges) { - return { - error: COMPLETION_ERROR_NO_PUSH, - continuationPrompt: - 'CASCADE completion check failed: no authoritative pushed changes were recorded for this task. Continue from the current session, commit and push the required changes, confirm the push succeeded, and only then finish.', - }; + const pushedChanges = evaluatePushedChanges(requirements, evidence); + if (pushedChanges?.satisfiedBy === null) { + return pushedChangesFailure( + requirements?.pushedChangesAlternatives, + evidence, + pushedChanges.rejections, + ); } if (requirements?.requiresPMWrite && !evidence.hasPMWrite) { @@ -138,14 +352,15 @@ export function getCompletionFailure( } /** - * Read sidecar files and upgrade text-based PR evidence to authoritative. - * Shared across Claude Code and OpenCode backends. + * Read the PR sidecar and upgrade text-based PR evidence to authoritative. + * Shared across Claude Code and OpenCode backends. Only the PR sidecar is read here so a + * turn does not shell out for repository state twice. */ export function applyCompletionEvidence( result: AgentEngineResult, completionRequirements: CompletionRequirements | undefined, ): AgentEngineResult { - const evidence = readCompletionEvidence(completionRequirements); + const evidence = readCompletionEvidence({ prSidecarPath: completionRequirements?.prSidecarPath }); if (!evidence.prUrl) return result; return { ...result, diff --git a/src/backends/postProcess.ts b/src/backends/postProcess.ts index e07b6649..36ada9cd 100644 --- a/src/backends/postProcess.ts +++ b/src/backends/postProcess.ts @@ -5,6 +5,7 @@ import { COMPLETION_ERROR_NO_PR, COMPLETION_ERROR_NO_PUSH, COMPLETION_ERROR_NO_REVIEW, + type PushedChangesEvaluation, } from './completion.js'; import type { AgentEngine, AgentEngineResult } from './types.js'; @@ -23,7 +24,10 @@ export function postProcessResult( requiresPushedChanges?: boolean; requiresPMWrite?: boolean; hasAuthoritativeReview?: boolean; - hasAuthoritativePushedChanges?: boolean; + /** Shared pushed-changes verdict (`evaluatePushedChanges`); absent when no sidecar was expected. */ + pushedChangesEvaluation?: PushedChangesEvaluation; + /** Error recorded when every pushed-changes outcome is rejected; defaults to the legacy no-push error. */ + pushedChangesError?: string; hasPMWrite?: boolean; }, ): void { @@ -66,17 +70,23 @@ export function postProcessResult( result.error = COMPLETION_ERROR_NO_REVIEW; } - if ( - options?.requiresPushedChanges && - result.success && - options.hasAuthoritativePushedChanges === false - ) { - logger.warn(`${agentType} agent completed without authoritative pushed-change evidence`, { - identifier, - engine: engine.definition.id, - }); - result.success = false; - result.error = COMPLETION_ERROR_NO_PUSH; + if (options?.requiresPushedChanges && result.success && options.pushedChangesEvaluation) { + const evaluation = options.pushedChangesEvaluation; + if (evaluation.satisfiedBy === null) { + logger.warn(`${agentType} agent completed without authoritative pushed-change evidence`, { + identifier, + engine: engine.definition.id, + rejections: evaluation.rejections, + }); + result.success = false; + result.error = options.pushedChangesError ?? COMPLETION_ERROR_NO_PUSH; + } else { + logger.info(`${agentType} agent satisfied the pushed-changes requirement`, { + identifier, + engine: engine.definition.id, + pushedChangesOutcome: evaluation.satisfiedBy, + }); + } } if (options?.requiresPMWrite && result.success && options.hasPMWrite === false) { diff --git a/src/backends/secretOrchestrator.ts b/src/backends/secretOrchestrator.ts index 13c4e07f..2e565856 100644 --- a/src/backends/secretOrchestrator.ts +++ b/src/backends/secretOrchestrator.ts @@ -221,6 +221,7 @@ export async function buildExecutionPlan( requiresPR: profile.finishHooks.requiresPR, requiresReview: profile.finishHooks.requiresReview, requiresPushedChanges: profile.finishHooks.requiresPushedChanges, + pushedChangesAlternatives: profile.finishHooks.pushedChangesAlternatives, requiresPMWrite: profile.finishHooks.requiresPMWrite, prSidecarPath, reviewSidecarPath, diff --git a/src/backends/shared/continuationLoop.ts b/src/backends/shared/continuationLoop.ts index 91ab8254..501b8950 100644 --- a/src/backends/shared/continuationLoop.ts +++ b/src/backends/shared/continuationLoop.ts @@ -15,6 +15,7 @@ import { applyCompletionEvidence, type CompletionRequirements, + evaluatePushedChanges, getCompletionFailure, readCompletionEvidence, } from '../completion.js'; @@ -26,7 +27,8 @@ export type ContinuationDecision = /** * Check completion requirements and decide whether to continue or return a final result. - * Logs the continuation warning when a new turn is needed. + * Logs which pushed-changes outcome satisfied the check, or why each outcome was rejected, + * so a run log alone explains the decision. * * Extracted from claude-code/index.ts so both Claude Code and OpenCode can share * the same decision logic. @@ -41,14 +43,28 @@ export function decideContinuation( toolCallCount: number, engineLabel: string, ): ContinuationDecision { - const completionFailure = getCompletionFailure( - completionRequirements, - readCompletionEvidence(completionRequirements), - ); + const evidence = readCompletionEvidence(completionRequirements); + const completionFailure = getCompletionFailure(completionRequirements, evidence); if (!completionFailure) { + const pushedChanges = evaluatePushedChanges(completionRequirements, evidence); + if (pushedChanges?.satisfiedBy) { + logWriter('INFO', `${engineLabel} completion check passed`, { + pushedChangesOutcome: pushedChanges.satisfiedBy, + }); + } return { done: true, result: { ...result, cost: totalCost } }; } + const rejectionDetails = completionFailure.rejections + ? { rejections: completionFailure.rejections } + : {}; if (continuationTurns >= maxContinuationTurns) { + logWriter('WARN', `${engineLabel} completion check failed; continuation turns exhausted`, { + reason: completionFailure.error, + continuationTurns, + maxContinuationTurns, + toolCallCount, + ...rejectionDetails, + }); return { done: true, result: { ...result, success: false, error: completionFailure.error, cost: totalCost }, @@ -59,6 +75,7 @@ export function decideContinuation( continuationTurn: continuationTurns + 1, maxContinuationTurns, toolCallCount, + ...rejectionDetails, }); return { done: false, promptText: completionFailure.continuationPrompt }; } diff --git a/tests/README.md b/tests/README.md index 28b62976..3b480dff 100644 --- a/tests/README.md +++ b/tests/README.md @@ -38,7 +38,9 @@ tests/helpers/ ├── trpcTestHarness.ts # tRPC caller factory + ownership mock + error assertion ├── mockDb.ts # Drizzle query-chain mock builder ├── mockPMProvider.ts # PMProvider stub factory -└── mockPersonas.ts # GitHub bot-identity stub helpers +├── mockPersonas.ts # GitHub bot-identity stub helpers +├── tempGitRepo.ts # Throwaway real git repository (commit / writeFile / cleanup) +└── cliSpawnBudget.ts # Timeouts for tests that spawn the built cascade-tools CLI ``` --- diff --git a/tests/helpers/cliSpawnBudget.ts b/tests/helpers/cliSpawnBudget.ts new file mode 100644 index 00000000..3f091f05 --- /dev/null +++ b/tests/helpers/cliSpawnBudget.ts @@ -0,0 +1,8 @@ +/** + * Budgets for tests that spawn the built `cascade-tools` CLI. A cold oclif boot takes 2-5 s + * and more under full-suite CPU pressure, far past vitest's 5 s default. The test budget + * stays strictly above the spawn budget so a hung CLI fails on the spawn's captured stderr, + * not on an opaque vitest timeout. + */ +export const CLI_SPAWN_TIMEOUT_MS = 30_000; +export const CLI_TEST_TIMEOUT_MS = CLI_SPAWN_TIMEOUT_MS + 5_000; diff --git a/tests/helpers/tempGitRepo.ts b/tests/helpers/tempGitRepo.ts new file mode 100644 index 00000000..c5e9987a --- /dev/null +++ b/tests/helpers/tempGitRepo.ts @@ -0,0 +1,44 @@ +import { execFileSync } from 'node:child_process'; +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +export interface TempGitRepo { + dir: string; + git(...args: string[]): string; + /** Stages everything, commits (empty commits allowed), returns the new HEAD SHA. */ + commit(message: string): string; + writeFile(name: string, content: string): void; + cleanup(): void; +} + +/** A throwaway real git repository for tests that exercise production git calls. */ +export function createTempGitRepo(prefix = 'cascade-temp-git-'): TempGitRepo { + const dir = mkdtempSync(join(tmpdir(), prefix)); + const git = (...args: string[]): string => + execFileSync('git', ['-c', 'commit.gpgsign=false', ...args], { + cwd: dir, + encoding: 'utf-8', + stdio: ['ignore', 'pipe', 'ignore'], + }).trim(); + + git('init', '-q', '--initial-branch=main'); + git('config', 'user.email', 'temp-git@cascade.local'); + git('config', 'user.name', 'temp-git'); + + return { + dir, + git, + commit(message) { + git('add', '-A'); + git('commit', '-q', '--allow-empty', '-m', message); + return git('rev-parse', 'HEAD'); + }, + writeFile(name, content) { + writeFileSync(join(dir, name), content); + }, + cleanup() { + rmSync(dir, { recursive: true, force: true }); + }, + }; +} diff --git a/tests/unit/agents/definitions/profiles.test.ts b/tests/unit/agents/definitions/profiles.test.ts index 3cced97f..b0f45789 100644 --- a/tests/unit/agents/definitions/profiles.test.ts +++ b/tests/unit/agents/definitions/profiles.test.ts @@ -207,6 +207,7 @@ describe('getAgentProfile', () => { requiresPR: undefined, requiresReview: undefined, requiresPushedChanges: undefined, + pushedChangesAlternatives: undefined, blockGitPush: undefined, }); }); @@ -216,7 +217,12 @@ describe('getAgentProfile', () => { makeDefinition({ hooks: { finish: { - scm: { requiresPR: true, requiresReview: false, requiresPushedChanges: true }, + scm: { + requiresPR: true, + requiresReview: false, + requiresPushedChanges: true, + pushedChangesAlternatives: ['pr-response'], + }, }, }, }), @@ -227,6 +233,7 @@ describe('getAgentProfile', () => { expect(profile.finishHooks.requiresPR).toBe(true); expect(profile.finishHooks.requiresReview).toBe(false); expect(profile.finishHooks.requiresPushedChanges).toBe(true); + expect(profile.finishHooks.pushedChangesAlternatives).toEqual(['pr-response']); }); // ============================================================================ diff --git a/tests/unit/agents/definitions/schema.test.ts b/tests/unit/agents/definitions/schema.test.ts index 51594277..b95fc435 100644 --- a/tests/unit/agents/definitions/schema.test.ts +++ b/tests/unit/agents/definitions/schema.test.ts @@ -382,6 +382,63 @@ describe.concurrent('AgentDefinitionSchema', () => { } }); + it('accepts pushedChangesAlternatives: [pr-response] next to requiresPushedChanges: true', () => { + const good = { + ...validDefinition, + hooks: { + finish: { + scm: { requiresPushedChanges: true, pushedChangesAlternatives: ['pr-response'] }, + }, + }, + }; + const result = AgentDefinitionSchema.safeParse(good); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data.hooks?.finish?.scm?.pushedChangesAlternatives).toEqual(['pr-response']); + } + }); + + it('rejects an unknown pushedChangesAlternatives name', () => { + const bad = { + ...validDefinition, + hooks: { + finish: { scm: { requiresPushedChanges: true, pushedChangesAlternatives: ['comment'] } }, + }, + }; + const result = AgentDefinitionSchema.safeParse(bad); + expect(result.success).toBe(false); + if (!result.success) { + expect(JSON.stringify(result.error.issues)).toContain('pr-response'); + } + }); + + it('rejects pushedChangesAlternatives without requiresPushedChanges: true', () => { + const shapes = [ + { pushedChangesAlternatives: ['pr-response'] }, + { requiresPushedChanges: false, pushedChangesAlternatives: ['pr-response'] }, + ]; + for (const scm of shapes) { + const result = AgentDefinitionSchema.safeParse({ + ...validDefinition, + hooks: { finish: { scm } }, + }); + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.issues.map((issue) => issue.message)).toContain( + 'pushedChangesAlternatives requires requiresPushedChanges: true', + ); + } + } + }); + + it('rejects an empty pushedChangesAlternatives list', () => { + const result = AgentDefinitionSchema.safeParse({ + ...validDefinition, + hooks: { finish: { scm: { requiresPushedChanges: true, pushedChangesAlternatives: [] } } }, + }); + expect(result.success).toBe(false); + }); + it('rejects overlapping required and optional capabilities', () => { const bad = { ...validDefinition, diff --git a/tests/unit/backends/adapter.test.ts b/tests/unit/backends/adapter.test.ts index ec041105..7d211977 100644 --- a/tests/unit/backends/adapter.test.ts +++ b/tests/unit/backends/adapter.test.ts @@ -108,6 +108,7 @@ import { resolveModelConfig } from '../../../src/agents/shared/modelResolution.j import { setupRepository } from '../../../src/agents/shared/repository.js'; import { createAgentLogger } from '../../../src/agents/utils/logging.js'; import { executeWithEngine } from '../../../src/backends/adapter.js'; +import { COMPLETION_ERROR_NO_PUSH_OR_RESPONSE } from '../../../src/backends/completion.js'; import { createProgressMonitor } from '../../../src/backends/progress.js'; import type { AgentEngine } from '../../../src/backends/types.js'; import { getAllProjectCredentials } from '../../../src/config/provider.js'; @@ -664,6 +665,31 @@ describe('executeWithEngine', () => { ); }); + it('fails closed with the alternative-aware error when a profile declares pr-response but no evidence exists', async () => { + setupMocks(); + mockGetAgentProfile.mockReturnValue( + makeMockProfile({ + finishHooks: { requiresPushedChanges: true, pushedChangesAlternatives: ['pr-response'] }, + }), + ); + const engine = makeMockBackend('opencode'); + vi.mocked(engine.execute).mockResolvedValue({ success: true, output: 'Done' }); + + const result = await executeWithEngine(engine, 'respond-to-pr-comment', makeInput()); + + expect(result.success).toBe(false); + expect(result.error).toBe(COMPLETION_ERROR_NO_PUSH_OR_RESPONSE); + expect(logger.warn).toHaveBeenCalledWith( + 'respond-to-pr-comment agent completed without authoritative pushed-change evidence', + expect.objectContaining({ + rejections: [ + expect.objectContaining({ outcome: 'pushed-changes' }), + expect.objectContaining({ outcome: 'pr-response' }), + ], + }), + ); + }); + it('does not validate PR creation for non-implementation agents', async () => { setupMocks(); const engine = makeMockBackend(); diff --git a/tests/unit/backends/completion.test.ts b/tests/unit/backends/completion.test.ts index c6c61463..af7458f7 100644 --- a/tests/unit/backends/completion.test.ts +++ b/tests/unit/backends/completion.test.ts @@ -1,10 +1,26 @@ -import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; -import { join } from 'node:path'; -import { describe, expect, it } from 'vitest'; +import { join, resolve } from 'node:path'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; -import { applyCompletionEvidence } from '../../../src/backends/completion.js'; +import { + applyCompletionEvidence, + COMPLETION_ERROR_NO_PUSH, + COMPLETION_ERROR_NO_PUSH_OR_RESPONSE, + CONTINUATION_PROMPT_NO_PUSH, + type CompletionEvidence, + evaluatePushedChanges, + getCompletionFailure, + REJECTION_NO_PUSH, + REJECTION_NO_RESPONSE, + REJECTION_REPO_STATE_UNAVAILABLE, + REJECTION_RESPONSE_MALFORMED, + REJECTION_TREE_DIRTY, + readCompletionEvidence, + readRepoState, +} from '../../../src/backends/completion.js'; import type { AgentEngineResult } from '../../../src/backends/types.js'; +import { createTempGitRepo, type TempGitRepo } from '../../helpers/tempGitRepo.js'; describe('applyCompletionEvidence', () => { it('returns result unchanged when no sidecar exists', () => { @@ -111,3 +127,397 @@ describe('applyCompletionEvidence', () => { expect(updated.prEvidence?.command).toBe('cascade-tools scm create-pr'); }); }); + +const RESPONSE_URL = 'https://github.com/o/r/pull/7#issuecomment-1'; + +function evidenceWith(overrides: Partial): CompletionEvidence { + return { + hasAuthoritativePR: false, + hasAuthoritativeReview: false, + hasAuthoritativePushedChanges: false, + hasPMWrite: false, + hasAuthoritativePRResponse: false, + prResponseSidecarMalformed: false, + ...overrides, + }; +} + +describe('readCompletionEvidence — pr-response', () => { + let tempDir: string; + + beforeEach(() => { + tempDir = mkdtempSync(join(tmpdir(), 'cascade-completion-response-')); + }); + + afterEach(() => { + rmSync(tempDir, { recursive: true, force: true }); + }); + + function writeResponseSidecar(content: unknown): string { + const path = join(tempDir, 'pr-response.json'); + writeFileSync(path, typeof content === 'string' ? content : JSON.stringify(content)); + return path; + } + + it('reads a well-formed PR-response sidecar', () => { + const prResponseSidecarPath = writeResponseSidecar({ + source: 'cascade-tools scm post-pr-comment', + url: RESPONSE_URL, + kind: 'top-level', + id: 1, + prNumber: 7, + }); + + const evidence = readCompletionEvidence({ prResponseSidecarPath }); + + expect(evidence.hasAuthoritativePRResponse).toBe(true); + expect(evidence.prResponse).toEqual({ + url: RESPONSE_URL, + kind: 'top-level', + command: 'cascade-tools scm post-pr-comment', + }); + expect(evidence.prResponseSidecarMalformed).toBe(false); + }); + + it('accepts the inline-reply kind', () => { + const prResponseSidecarPath = writeResponseSidecar({ + source: 'cascade-tools scm reply-to-review-comment', + url: RESPONSE_URL, + kind: 'inline-reply', + }); + + expect(readCompletionEvidence({ prResponseSidecarPath }).prResponse?.kind).toBe('inline-reply'); + }); + + it('defaults the command when source is absent', () => { + const prResponseSidecarPath = writeResponseSidecar({ url: RESPONSE_URL, kind: 'top-level' }); + + expect(readCompletionEvidence({ prResponseSidecarPath }).prResponse?.command).toBe( + 'cascade-tools scm post-pr-comment', + ); + }); + + it('treats a sidecar without url as malformed, not as evidence', () => { + const prResponseSidecarPath = writeResponseSidecar({ kind: 'top-level' }); + + const evidence = readCompletionEvidence({ prResponseSidecarPath }); + + expect(evidence.hasAuthoritativePRResponse).toBe(false); + expect(evidence.prResponse).toBeUndefined(); + expect(evidence.prResponseSidecarMalformed).toBe(true); + }); + + it('treats an unknown kind as malformed', () => { + const prResponseSidecarPath = writeResponseSidecar({ url: RESPONSE_URL, kind: 'edit' }); + + const evidence = readCompletionEvidence({ prResponseSidecarPath }); + + expect(evidence.hasAuthoritativePRResponse).toBe(false); + expect(evidence.prResponseSidecarMalformed).toBe(true); + }); + + it('treats invalid JSON as malformed', () => { + const prResponseSidecarPath = writeResponseSidecar('{not json'); + + const evidence = readCompletionEvidence({ prResponseSidecarPath }); + + expect(evidence.hasAuthoritativePRResponse).toBe(false); + expect(evidence.prResponseSidecarMalformed).toBe(true); + }); + + it('reports no PR response when the sidecar path is unset or the file is missing', () => { + for (const prResponseSidecarPath of [undefined, join(tempDir, 'missing.json')]) { + const evidence = readCompletionEvidence({ prResponseSidecarPath }); + expect(evidence.hasAuthoritativePRResponse).toBe(false); + expect(evidence.prResponseSidecarMalformed).toBe(false); + } + }); +}); + +describe('readRepoState', () => { + let repo: TempGitRepo; + + beforeEach(() => { + repo = createTempGitRepo('cascade-repo-state-'); + }); + + afterEach(() => { + repo.cleanup(); + }); + + it('reports a clean tree at the initial commit', () => { + const initial = repo.commit('a'); + + expect(readRepoState(repo.dir, initial)).toEqual({ + clean: true, + headSha: initial, + initialHeadSha: initial, + headUnchanged: true, + }); + }); + + it('reports an uncommitted change as dirty', () => { + const initial = repo.commit('a'); + repo.writeFile('f.txt', 'x'); + repo.git('add', 'f.txt'); + + expect(readRepoState(repo.dir, initial)).toMatchObject({ clean: false, headUnchanged: true }); + }); + + it('reports a new commit as HEAD moved', () => { + const initial = repo.commit('a'); + const next = repo.commit('b'); + + expect(readRepoState(repo.dir, initial)).toEqual({ + clean: true, + headSha: next, + initialHeadSha: initial, + headUnchanged: false, + }); + }); + + it('counts an untracked file as dirty', () => { + const initial = repo.commit('a'); + repo.writeFile('untracked.txt', 'x'); + + expect(readRepoState(repo.dir, initial)?.clean).toBe(false); + }); + + it('returns undefined outside a git repository', () => { + const plainDir = mkdtempSync(join(tmpdir(), 'cascade-not-a-repo-')); + try { + expect(readRepoState(plainDir, 'abc')).toBeUndefined(); + } finally { + rmSync(plainDir, { recursive: true, force: true }); + } + }); + + it('attaches repoState to evidence only when repoDir and initialHeadSha are both given', () => { + const initial = repo.commit('a'); + + expect(readCompletionEvidence({ repoDir: repo.dir }).repoState).toBeUndefined(); + expect(readCompletionEvidence({ initialHeadSha: initial }).repoState).toBeUndefined(); + expect( + readCompletionEvidence({ repoDir: repo.dir, initialHeadSha: initial }).repoState, + ).toEqual({ clean: true, headSha: initial, initialHeadSha: initial, headUnchanged: true }); + }); +}); + +const RESPONSE = { + url: RESPONSE_URL, + kind: 'top-level' as const, + command: 'cascade-tools scm post-pr-comment', +}; +const CLEAN_REPO = { clean: true, headSha: 'aaa', initialHeadSha: 'aaa', headUnchanged: true }; +const WITH_ALTERNATIVE = { + requiresPushedChanges: true, + pushedChangesAlternatives: ['pr-response'] as const, +}; +const NO_PUSH_REJECTION = { + outcome: 'pushed-changes', + reason: REJECTION_NO_PUSH, +}; + +describe('evaluatePushedChanges', () => { + it('returns undefined when pushed changes are not required', () => { + expect(evaluatePushedChanges(undefined, evidenceWith({}))).toBeUndefined(); + expect(evaluatePushedChanges({ requiresPR: true }, evidenceWith({}))).toBeUndefined(); + }); + + it('pushed-changes evidence satisfies the requirement', () => { + expect( + evaluatePushedChanges( + { requiresPushedChanges: true }, + evidenceWith({ hasAuthoritativePushedChanges: true }), + ), + ).toEqual({ satisfiedBy: 'pushed-changes' }); + }); + + it('without alternatives, a PR response alone is rejected', () => { + expect( + evaluatePushedChanges( + { requiresPushedChanges: true }, + evidenceWith({ + hasAuthoritativePRResponse: true, + prResponse: RESPONSE, + repoState: CLEAN_REPO, + }), + ), + ).toEqual({ satisfiedBy: null, rejections: [NO_PUSH_REJECTION] }); + }); + + it('pr-response with a clean, unchanged repository satisfies the requirement', () => { + expect( + evaluatePushedChanges( + WITH_ALTERNATIVE, + evidenceWith({ + hasAuthoritativePRResponse: true, + prResponse: RESPONSE, + repoState: CLEAN_REPO, + }), + ), + ).toEqual({ satisfiedBy: 'pr-response' }); + }); + + it('pushed-changes wins when both branches are satisfied', () => { + expect( + evaluatePushedChanges( + WITH_ALTERNATIVE, + evidenceWith({ + hasAuthoritativePushedChanges: true, + hasAuthoritativePRResponse: true, + prResponse: RESPONSE, + repoState: CLEAN_REPO, + }), + ), + ).toEqual({ satisfiedBy: 'pushed-changes' }); + }); + + it('pr-response is rejected when the tree is dirty', () => { + expect( + evaluatePushedChanges( + WITH_ALTERNATIVE, + evidenceWith({ + hasAuthoritativePRResponse: true, + prResponse: RESPONSE, + repoState: { ...CLEAN_REPO, clean: false }, + }), + ), + ).toEqual({ + satisfiedBy: null, + rejections: [NO_PUSH_REJECTION, { outcome: 'pr-response', reason: REJECTION_TREE_DIRTY }], + }); + }); + + it('pr-response is rejected when HEAD moved', () => { + expect( + evaluatePushedChanges( + WITH_ALTERNATIVE, + evidenceWith({ + hasAuthoritativePRResponse: true, + prResponse: RESPONSE, + repoState: { clean: true, headSha: 'bbb', initialHeadSha: 'aaa', headUnchanged: false }, + }), + ), + ).toEqual({ + satisfiedBy: null, + rejections: [ + NO_PUSH_REJECTION, + { outcome: 'pr-response', reason: expect.stringMatching(/HEAD moved from aaa to bbb/) }, + ], + }); + }); + + it('pr-response is rejected when repository state is unavailable', () => { + expect( + evaluatePushedChanges( + WITH_ALTERNATIVE, + evidenceWith({ hasAuthoritativePRResponse: true, prResponse: RESPONSE }), + ), + ).toEqual({ + satisfiedBy: null, + rejections: [ + NO_PUSH_REJECTION, + { outcome: 'pr-response', reason: REJECTION_REPO_STATE_UNAVAILABLE }, + ], + }); + }); + + it('pr-response is rejected when the sidecar is malformed, naming the malformation', () => { + expect( + evaluatePushedChanges( + WITH_ALTERNATIVE, + evidenceWith({ prResponseSidecarMalformed: true, repoState: CLEAN_REPO }), + ), + ).toEqual({ + satisfiedBy: null, + rejections: [ + NO_PUSH_REJECTION, + { outcome: 'pr-response', reason: REJECTION_RESPONSE_MALFORMED }, + ], + }); + }); + + it('pr-response is rejected when nothing was recorded', () => { + expect( + evaluatePushedChanges(WITH_ALTERNATIVE, evidenceWith({ repoState: CLEAN_REPO })), + ).toEqual({ + satisfiedBy: null, + rejections: [NO_PUSH_REJECTION, { outcome: 'pr-response', reason: REJECTION_NO_RESPONSE }], + }); + }); +}); + +describe('getCompletionFailure — pushed-changes alternatives', () => { + it('keeps the legacy no-push error and prompt when no alternative is declared', () => { + const failure = getCompletionFailure({ requiresPushedChanges: true }, evidenceWith({})); + + expect(failure?.error).toBe(COMPLETION_ERROR_NO_PUSH); + expect(failure?.continuationPrompt).toBe(CONTINUATION_PROMPT_NO_PUSH); + }); + + it('returns undefined when the pr-response branch is satisfied', () => { + expect( + getCompletionFailure( + WITH_ALTERNATIVE, + evidenceWith({ + hasAuthoritativePRResponse: true, + prResponse: RESPONSE, + repoState: CLEAN_REPO, + }), + ), + ).toBeUndefined(); + }); + + it('with alternatives and no evidence, names both ways to finish', () => { + const failure = getCompletionFailure(WITH_ALTERNATIVE, evidenceWith({ repoState: CLEAN_REPO })); + + expect(failure?.error).toBe(COMPLETION_ERROR_NO_PUSH_OR_RESPONSE); + expect(failure?.continuationPrompt).toMatch(/cascade-tools scm post-pr-comment/); + expect(failure?.continuationPrompt).toMatch(/reply-to-review-comment/); + expect(failure?.continuationPrompt).toMatch(/commit and push/); + expect(failure?.continuationPrompt).toMatch(/cascade-tools session finish/); + }); + + it('with a recorded response but a modified repository, tells the session not to repeat the response', () => { + const failure = getCompletionFailure( + WITH_ALTERNATIVE, + evidenceWith({ + hasAuthoritativePRResponse: true, + prResponse: RESPONSE, + repoState: { ...CLEAN_REPO, clean: false }, + }), + ); + + expect(failure?.error).toBe(COMPLETION_ERROR_NO_PUSH_OR_RESPONSE); + expect(failure?.continuationPrompt).toContain(RESPONSE_URL); + expect(failure?.continuationPrompt).toMatch(/do not post (it|another)/i); + expect(failure?.continuationPrompt).toMatch(/commit and push|revert/); + }); + + it('exposes the rejections on the failure', () => { + const evidence = evidenceWith({ repoState: CLEAN_REPO }); + const evaluation = evaluatePushedChanges(WITH_ALTERNATIVE, evidence); + const failure = getCompletionFailure(WITH_ALTERNATIVE, evidence); + + expect(evaluation).toMatchObject({ satisfiedBy: null }); + expect(failure?.rejections).toEqual( + evaluation?.satisfiedBy === null ? evaluation.rejections : undefined, + ); + }); +}); + +describe('completion logic is agent-agnostic', () => { + it('never references an agent type by name', () => { + const sharedCompletionFiles = [ + 'src/backends/completion.ts', + 'src/backends/shared/continuationLoop.ts', + 'src/backends/postProcess.ts', + ]; + for (const file of sharedCompletionFiles) { + expect(readFileSync(resolve(file), 'utf8'), file).not.toMatch( + /respond-to-pr-comment|prComment/, + ); + } + }); +}); diff --git a/tests/unit/backends/postProcess.test.ts b/tests/unit/backends/postProcess.test.ts index f63a2744..fad3d763 100644 --- a/tests/unit/backends/postProcess.test.ts +++ b/tests/unit/backends/postProcess.test.ts @@ -12,6 +12,7 @@ vi.mock('../../../src/sentry.js', () => ({ captureException: vi.fn(), })); +import { COMPLETION_ERROR_NO_PUSH_OR_RESPONSE } from '../../../src/backends/completion.js'; import { postProcessResult } from '../../../src/backends/postProcess.js'; import type { AgentEngine, AgentEngineResult } from '../../../src/backends/types.js'; import { captureException } from '../../../src/sentry.js'; @@ -249,14 +250,22 @@ describe('postProcessResult', () => { }); describe('pushed-changes validation for agents with requiresPushedChanges', () => { - it('marks as failed when requiresPushedChanges agent succeeds without authoritative push evidence', () => { + const REJECTED_EVALUATION = { + satisfiedBy: null, + rejections: [ + { outcome: 'pushed-changes' as const, reason: 'no pushed-changes sidecar' }, + { outcome: 'pr-response' as const, reason: 'no PR response recorded' }, + ], + }; + + it('marks as failed with the legacy error when the evaluation rejects every outcome and no error is supplied', () => { const result = makeResult({ success: true }); const engine = makeEngine(); const input = makeInput(); postProcessResult(result, 'respond-to-review', engine, input, 'review-pr-123', { requiresPushedChanges: true, - hasAuthoritativePushedChanges: false, + pushedChangesEvaluation: REJECTED_EVALUATION, }); expect(result.success).toBe(false); @@ -265,14 +274,52 @@ describe('postProcessResult', () => { ); }); - it('passes through when requiresPushedChanges agent has authoritative push evidence', () => { + it('fails with the supplied alternative-aware error and logs every rejection', () => { + const result = makeResult({ success: true }); + const engine = makeEngine(); + const input = makeInput(); + + postProcessResult(result, 'respond-to-pr-comment', engine, input, 'comment-pr-123', { + requiresPushedChanges: true, + pushedChangesEvaluation: REJECTED_EVALUATION, + pushedChangesError: COMPLETION_ERROR_NO_PUSH_OR_RESPONSE, + }); + + expect(result.success).toBe(false); + expect(result.error).toBe(COMPLETION_ERROR_NO_PUSH_OR_RESPONSE); + expect(logger.warn).toHaveBeenCalledWith( + 'respond-to-pr-comment agent completed without authoritative pushed-change evidence', + expect.objectContaining({ rejections: REJECTED_EVALUATION.rejections }), + ); + }); + + it('keeps the result successful and logs the outcome when the evaluation is satisfied', () => { + for (const satisfiedBy of ['pushed-changes', 'pr-response'] as const) { + const result = makeResult({ success: true }); + const engine = makeEngine(); + const input = makeInput(); + + postProcessResult(result, 'respond-to-ci', engine, input, 'ci-pr-123', { + requiresPushedChanges: true, + pushedChangesEvaluation: { satisfiedBy }, + }); + + expect(result.success).toBe(true); + expect(result.error).toBeUndefined(); + expect(logger.info).toHaveBeenCalledWith( + 'respond-to-ci agent satisfied the pushed-changes requirement', + expect.objectContaining({ pushedChangesOutcome: satisfiedBy }), + ); + } + }); + + it('leaves the result alone when no evaluation is supplied', () => { const result = makeResult({ success: true }); const engine = makeEngine(); const input = makeInput(); postProcessResult(result, 'respond-to-ci', engine, input, 'ci-pr-123', { requiresPushedChanges: true, - hasAuthoritativePushedChanges: true, }); expect(result.success).toBe(true); diff --git a/tests/unit/backends/shared-continuationLoop.test.ts b/tests/unit/backends/shared-continuationLoop.test.ts index d54fa3c5..68dde1fc 100644 --- a/tests/unit/backends/shared-continuationLoop.test.ts +++ b/tests/unit/backends/shared-continuationLoop.test.ts @@ -2,12 +2,17 @@ import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { describe, expect, it, vi } from 'vitest'; +import { + COMPLETION_ERROR_NO_PUSH_OR_RESPONSE, + type CompletionRequirements, +} from '../../../src/backends/completion.js'; import { type ContinuationTurnContext, decideContinuation, runContinuationLoop, } from '../../../src/backends/shared/continuationLoop.js'; import type { AgentEngineResult } from '../../../src/backends/types.js'; +import { createTempGitRepo } from '../../helpers/tempGitRepo.js'; function makeSuccessResult(overrides: Partial = {}): AgentEngineResult { return { @@ -132,7 +137,14 @@ describe('decideContinuation', () => { ); expect(decision.result.cost).toBe(0.25); } - expect(logWriter).not.toHaveBeenCalled(); + expect(logWriter).toHaveBeenCalledWith( + 'WARN', + 'TestEngine completion check failed; continuation turns exhausted', + expect.objectContaining({ + reason: 'Agent completed but no authoritative PR creation was recorded', + continuationTurns: 2, + }), + ); }); it('uses engine label in warning log message', () => { @@ -435,3 +447,115 @@ describe('runContinuationLoop', () => { expect(capturedContexts[1].isContinuation).toBe(true); }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// decideContinuation — pushed-changes outcomes (spec 025) +// ───────────────────────────────────────────────────────────────────────────── + +describe('decideContinuation — pushed-changes outcomes', () => { + const WITH_ALTERNATIVE: CompletionRequirements = { + requiresPushedChanges: true, + pushedChangesAlternatives: ['pr-response'], + }; + + function decideWith(requirements: CompletionRequirements, continuationTurns = 0) { + const logWriter = vi.fn(); + const decision = decideContinuation( + makeSuccessResult(), + requirements, + continuationTurns, + 2, + 0.05, + logWriter, + 1, + 'TestEngine', + ); + return { decision, logWriter }; + } + + it('logs the satisfied pushed-changes outcome on completion', () => { + const dir = mkdtempSync(join(tmpdir(), 'continuation-outcome-')); + const pushedChangesSidecarPath = join(dir, 'pushed.json'); + writeFileSync(pushedChangesSidecarPath, JSON.stringify({ branch: 'feature', headSha: 'abc' })); + try { + const { decision, logWriter } = decideWith({ + requiresPushedChanges: true, + pushedChangesSidecarPath, + }); + + expect(decision.done).toBe(true); + expect(logWriter).toHaveBeenCalledWith( + 'INFO', + 'TestEngine completion check passed', + expect.objectContaining({ pushedChangesOutcome: 'pushed-changes' }), + ); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('logs pr-response as the satisfied outcome', () => { + const repo = createTempGitRepo('continuation-outcome-repo-'); + const sidecarDir = mkdtempSync(join(tmpdir(), 'continuation-outcome-sidecar-')); + const prResponseSidecarPath = join(sidecarDir, 'pr-response.json'); + writeFileSync( + prResponseSidecarPath, + JSON.stringify({ url: 'https://github.com/o/r/pull/7#issuecomment-1', kind: 'top-level' }), + ); + try { + const initialHeadSha = repo.commit('start'); + const { decision, logWriter } = decideWith({ + ...WITH_ALTERNATIVE, + prResponseSidecarPath, + repoDir: repo.dir, + initialHeadSha, + }); + + expect(decision.done).toBe(true); + expect(logWriter).toHaveBeenCalledWith( + 'INFO', + 'TestEngine completion check passed', + expect.objectContaining({ pushedChangesOutcome: 'pr-response' }), + ); + } finally { + repo.cleanup(); + rmSync(sidecarDir, { recursive: true, force: true }); + } + }); + + it('includes per-outcome rejections in the continuation warning', () => { + const { decision, logWriter } = decideWith(WITH_ALTERNATIVE); + + expect(decision.done).toBe(false); + expect(logWriter).toHaveBeenCalledWith( + 'WARN', + 'TestEngine completion check failed; continuing session', + expect.objectContaining({ + reason: COMPLETION_ERROR_NO_PUSH_OR_RESPONSE, + rejections: [ + expect.objectContaining({ outcome: 'pushed-changes' }), + expect.objectContaining({ outcome: 'pr-response' }), + ], + }), + ); + }); + + it('logs the rejections when turns are exhausted and fails with the alternative-aware error', () => { + const { decision, logWriter } = decideWith(WITH_ALTERNATIVE, 2); + + expect(decision.done).toBe(true); + if (decision.done) { + expect(decision.result.success).toBe(false); + expect(decision.result.error).toBe(COMPLETION_ERROR_NO_PUSH_OR_RESPONSE); + } + expect(logWriter).toHaveBeenCalledWith( + 'WARN', + 'TestEngine completion check failed; continuation turns exhausted', + expect.objectContaining({ + reason: COMPLETION_ERROR_NO_PUSH_OR_RESPONSE, + continuationTurns: 2, + rejections: expect.arrayContaining([expect.objectContaining({ outcome: 'pr-response' })]), + }), + ); + }); +}); diff --git a/tests/unit/cli/cascade-tools-command-suggestions.test.ts b/tests/unit/cli/cascade-tools-command-suggestions.test.ts index f5d37108..4ce401cb 100644 --- a/tests/unit/cli/cascade-tools-command-suggestions.test.ts +++ b/tests/unit/cli/cascade-tools-command-suggestions.test.ts @@ -26,6 +26,7 @@ import { spawnSync } from 'node:child_process'; import { existsSync } from 'node:fs'; import { resolve } from 'node:path'; import { describe, expect, it } from 'vitest'; +import { CLI_SPAWN_TIMEOUT_MS, CLI_TEST_TIMEOUT_MS } from '../../helpers/cliSpawnBudget.js'; const REPO_ROOT = resolve(__dirname, '../../..'); const BIN = resolve(REPO_ROOT, 'bin/cascade-tools.js'); @@ -48,7 +49,7 @@ function runCascadeTools(args: string[]): SpawnResult { cwd: REPO_ROOT, encoding: 'utf-8', env, - timeout: 30_000, + timeout: CLI_SPAWN_TIMEOUT_MS, }); return { stdout: result.stdout ?? '', stderr: result.stderr ?? '', code: result.status }; } @@ -106,6 +107,7 @@ describe('cascade-tools command_not_found hook (MNG-1442)', () => { expect(result.stderr).toContain('unknown-command'); expect(result.stderr).not.toContain('{"success"'); }, + CLI_TEST_TIMEOUT_MS, ); it.skipIf(!built)( @@ -124,6 +126,7 @@ describe('cascade-tools command_not_found hook (MNG-1442)', () => { expect(env.error.expected).toContain('read-work-item'); expect(env.error.expected).not.toContain('create-pr'); }, + CLI_TEST_TIMEOUT_MS, ); it.skipIf(!built)( @@ -141,6 +144,7 @@ describe('cascade-tools command_not_found hook (MNG-1442)', () => { expect.arrayContaining(['alerting', 'pm', 'scm', 'session']), ); }, + CLI_TEST_TIMEOUT_MS, ); it.skipIf(!built)( @@ -157,6 +161,7 @@ describe('cascade-tools command_not_found hook (MNG-1442)', () => { expect(env.error.expected).toContain('read-work-item'); expect(env.error.expected).not.toContain('create-pr'); }, + CLI_TEST_TIMEOUT_MS, ); it.skipIf(!built)( @@ -183,5 +188,6 @@ describe('cascade-tools command_not_found hook (MNG-1442)', () => { expect(env.success).toBe(false); expect(env.error.type).toBe('unknown-flag'); }, + CLI_TEST_TIMEOUT_MS, ); }); diff --git a/tests/unit/cli/cascade-tools-help.test.ts b/tests/unit/cli/cascade-tools-help.test.ts index 0e460b2d..17490ff4 100644 --- a/tests/unit/cli/cascade-tools-help.test.ts +++ b/tests/unit/cli/cascade-tools-help.test.ts @@ -19,6 +19,7 @@ import { spawnSync } from 'node:child_process'; import { existsSync } from 'node:fs'; import { resolve } from 'node:path'; import { describe, expect, it } from 'vitest'; +import { CLI_SPAWN_TIMEOUT_MS, CLI_TEST_TIMEOUT_MS } from '../../helpers/cliSpawnBudget.js'; const REPO_ROOT = resolve(__dirname, '../../..'); const BIN = resolve(REPO_ROOT, 'bin/cascade-tools.js'); @@ -34,7 +35,7 @@ function runHelp(args: string[]): { stdout: string; stderr: string; code: number cwd: REPO_ROOT, encoding: 'utf-8', env, - timeout: 30_000, + timeout: CLI_SPAWN_TIMEOUT_MS, }); return { stdout: result.stdout ?? '', stderr: result.stderr ?? '', code: result.status }; } @@ -44,36 +45,44 @@ describe('cascade-tools --help — topic summaries', () => { // from ../dist/cli/bootstrap.js). Skip with a clear message if not built. const built = existsSync(DIST); - it.skipIf(!built)('topic descriptions are explicit, not borrowed from gadgets', () => { - const { stdout, code } = runHelp(['--help']); - expect(code).toBe(0); + it.skipIf(!built)( + 'topic descriptions are explicit, not borrowed from gadgets', + () => { + const { stdout, code } = runHelp(['--help']); + expect(code).toBe(0); - // Must NOT borrow gadget descriptions. These are the first-gadget - // descriptions that prod showed leaking into topic lines. - expect(stdout).not.toMatch( - /pm\s+(?:Add a checklist|Read a work item|Post a comment|Update a work item|Create a new work item|List all work items|Move a work item)/, - ); - expect(stdout).not.toMatch(/scm\s+Create a GitHub pull request\./); - expect(stdout).not.toMatch(/alerting\s+Retrieve full details for an alerting event/); - expect(stdout).not.toMatch(/session\s+Call this gadget when you have completed all tasks/); + // Must NOT borrow gadget descriptions. These are the first-gadget + // descriptions that prod showed leaking into topic lines. + expect(stdout).not.toMatch( + /pm\s+(?:Add a checklist|Read a work item|Post a comment|Update a work item|Create a new work item|List all work items|Move a work item)/, + ); + expect(stdout).not.toMatch(/scm\s+Create a GitHub pull request\./); + expect(stdout).not.toMatch(/alerting\s+Retrieve full details for an alerting event/); + expect(stdout).not.toMatch(/session\s+Call this gadget when you have completed all tasks/); - // Must contain canonical topic summaries for every discovered topic. - expect(stdout).toContain('TOPICS'); - expect(stdout).toMatch(/pm\s+Read and write PM work items/i); - expect(stdout).toMatch(/scm\s+Interact with GitHub PRs/i); - expect(stdout).toMatch(/alerting\s+Inspect Sentry alerting/i); - expect(stdout).toMatch(/session\s+End the agent session/i); - }); + // Must contain canonical topic summaries for every discovered topic. + expect(stdout).toContain('TOPICS'); + expect(stdout).toMatch(/pm\s+Read and write PM work items/i); + expect(stdout).toMatch(/scm\s+Interact with GitHub PRs/i); + expect(stdout).toMatch(/alerting\s+Inspect Sentry alerting/i); + expect(stdout).toMatch(/session\s+End the agent session/i); + }, + CLI_TEST_TIMEOUT_MS, + ); - it.skipIf(!built)('per-gadget --help is unaffected (topic-summary fix is additive)', () => { - const { stdout, code } = runHelp(['pm', 'read-work-item', '--help']); - expect(code).toBe(0); - // Spot-check: the gadget's own description / flags are still rendered. - expect(stdout).toContain('Read a work item'); - expect(stdout).toContain('--workItemId'); - expect(stdout).toContain('--[no-]includeComments'); - expect(stdout).toContain('cascade-tools pm read-work-item --workItemId abc123'); - expect(stdout).not.toContain(`--workItemId '"abc123"'`); - expect(stdout).not.toContain(`--workItemId 'abc123'`); - }); + it.skipIf(!built)( + 'per-gadget --help is unaffected (topic-summary fix is additive)', + () => { + const { stdout, code } = runHelp(['pm', 'read-work-item', '--help']); + expect(code).toBe(0); + // Spot-check: the gadget's own description / flags are still rendered. + expect(stdout).toContain('Read a work item'); + expect(stdout).toContain('--workItemId'); + expect(stdout).toContain('--[no-]includeComments'); + expect(stdout).toContain('cascade-tools pm read-work-item --workItemId abc123'); + expect(stdout).not.toContain(`--workItemId '"abc123"'`); + expect(stdout).not.toContain(`--workItemId 'abc123'`); + }, + CLI_TEST_TIMEOUT_MS, + ); }); diff --git a/tests/unit/cli/cascade-tools-stdout-cleanliness.test.ts b/tests/unit/cli/cascade-tools-stdout-cleanliness.test.ts index 4a9adc60..92c6fc8a 100644 --- a/tests/unit/cli/cascade-tools-stdout-cleanliness.test.ts +++ b/tests/unit/cli/cascade-tools-stdout-cleanliness.test.ts @@ -22,6 +22,7 @@ import { existsSync, mkdtempSync, readFileSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join, resolve } from 'node:path'; import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { CLI_SPAWN_TIMEOUT_MS, CLI_TEST_TIMEOUT_MS } from '../../helpers/cliSpawnBudget.js'; const REPO_ROOT = resolve(__dirname, '../../..'); const BIN = resolve(REPO_ROOT, 'bin/cascade-tools.js'); @@ -56,7 +57,7 @@ function runCascadeTools( cwd: REPO_ROOT, encoding: 'utf-8', env, - timeout: 30_000, + timeout: CLI_SPAWN_TIMEOUT_MS, }); return { stdout: result.stdout ?? '', stderr: result.stderr ?? '', code: result.status }; } @@ -86,6 +87,7 @@ describe('cascade-tools — stdout is reserved for the JSON envelope', () => { expect(stdout).not.toMatch(LOG_LEVEL_PREFIX); expect(stdout).not.toContain('[cascade]'); }, + CLI_TEST_TIMEOUT_MS, ); it.skipIf(!built)( @@ -102,6 +104,7 @@ describe('cascade-tools — stdout is reserved for the JSON envelope', () => { // At least one cascade-emitted log line landed in the file. expect(fileContent).toContain('[cascade]'); }, + CLI_TEST_TIMEOUT_MS, ); it.skipIf(!built)( @@ -116,12 +119,13 @@ describe('cascade-tools — stdout is reserved for the JSON envelope', () => { const result = spawnSync( 'node', [BIN, 'pm', 'read-work-item', '--workItemId', 'NOT-A-REAL-WORK-ITEM'], - { cwd: REPO_ROOT, encoding: 'utf-8', env, timeout: 30_000 }, + { cwd: REPO_ROOT, encoding: 'utf-8', env, timeout: CLI_SPAWN_TIMEOUT_MS }, ); const stdout = result.stdout ?? ''; expect(stdout).toMatch(ENVELOPE_START); expect(stdout).not.toContain(ESC); expect(stdout).not.toContain('[cascade]'); }, + CLI_TEST_TIMEOUT_MS, ); });