Skip to content

captain-hook: ⚡️ Coalesce reviews before spawn and skip recorded correction anchors - #272

Merged
yasyf merged 2 commits into
mainfrom
perf/review-pipeline-coalescing
Oct 3, 2026
Merged

yasyf merged 2 commits into
mainfrom
perf/review-pipeline-coalescing

Conversation

@yasyf

@yasyf yasyf commented Oct 2, 2026

Copy link
Copy Markdown
Owner

Summary

Claim pending SessionStart/SessionEnd review work before starting a child, preserving one running pass plus at most one queued follow-up per transcript directory. Prepare correction evidence only for anchors without a ledger row matching their exact (session_id, event_uuid), sharing one lazily opened ledger handle per scan.

Why

Every eligible SessionStart or SessionEnd detached a Python review child before pending/repo-lock coalescing. A child that lost the lock had already paid for interpreter startup, imports, and git remote get-url. Earlier live evidence included 1,161 replay-* review child logs in one cache tree.

When a transcript changed, prepare_corrections → harvest_pairs → BoundedGit also revisited historical anchors already recorded in the correction ledger, with a budget of up to 84 Git subprocesses per call.

What changed

  • captain_hook/review/pipeline.py: Claim the transcript directory’s pending-pass flock before enrollment or origin lookup. Skip spawning when a queued pass already covers that directory. Hand the locked descriptor to the child, which explicitly calls LOCK_UN and then closes it upon acquiring the repo lock. This admits a follow-up even while the parent still holds its descriptor copy.
  • captain_hook/review/cli.py: Accept --pending-fd and forward it to spawn_session.
  • captain_hook/review/settings.py: Add ReviewPaths with only db_path for guard admission. Keep full ReviewSettings validation inside the child’s recording boundary, so malformed reviewer settings still reach a child that records the failure.
  • captain_hook/review/scan.py: Add CorrectionLedger, opening one handle lazily per scan and closing it on exit. Batch recorded-anchor checks with one lookup per distinct session in each batch, matching both session and event UUID before correction preparation or Git work. Schedule lookups from the preparation thread onto the existing event loop with run_coroutine_threadsafe, without asyncio.run in this path.
  • captain_hook/snapshots/review.py: Reuse that ledger handle in record_correction_drafts; the corrections library permits one engine per ledger file per process.
  • tests/test_review_pipeline.py: Add 5 pending-admission tests and update descriptor-handoff assertions.
  • tests/test_snapshot_review_consumers.py: Add 3 rescan tests with preparation and Git-call counters; update callers to pass the ledger or recorded-anchor lookup.
  • tests/snapshot_review_helpers.py: Stub CorrectionLedger.recorded in the shared fixture.
  • CHANGELOG.md: Record spawn coalescing and recorded-correction filtering under Unreleased → Fixed.

Tests

The 5 tests in tests/test_review_pipeline.py::TestPendingAdmission cover:

  • 2 SessionStarts → 1 child and 1 enrollment probe while pending work is claimed.
  • Failed spawn releases the claim, allowing a second attempt.
  • The inherited claim blocks rivals until the child acquires the repo lock, then admits a follow-up.
  • An active child admits the next event while the parent still holds its descriptor copy.
  • Malformed HOOKS_REVIEW_JUDGE_CONCURRENCY still spawns 1 recording child.

The 3 tests in tests/test_snapshot_review_consumers.py cover:

  • 8 recorded anchors growing to 10 → exactly 2 newly prepared anchors.
  • Unchanged transcript → 0 prepared batches and 0 additional Git calls on rescan.
  • An anchor recorded only under a superseded event_uuid remains eligible for preparation.

No local pytest run because the host was saturated. CI grades these assertions.

Notes

Stop sweeps remain keyed by each worktree’s cwd hash. Re-keying them by repository is deferred because it would add an origin lookup to the Stop path.

@yasyf
yasyf force-pushed the perf/review-pipeline-coalescing branch from e9cc285 to 692c27a Compare October 3, 2026 03:49
yasyf added 2 commits October 2, 2026 23:17
…ection anchors

Claude-Session-Id: 95aa46a7-b92a-4c7f-80dc-775333ed92a9
Context: The detach admission regression patched process-global Popen, so enrollment consumed the stub before either detach.

Summary: Freeze enrollment and assert both attempts are detach calls with failure breadcrumbs.

Motivation: Verify claim release through the intended failure path across the Linux and macOS CI shards.

Details: Add an enrolled stub, check pending-fd argv, and count the two detach-failed log entries.
@yasyf
yasyf force-pushed the perf/review-pipeline-coalescing branch from 692c27a to 2e1d2a3 Compare October 3, 2026 06:17
@yasyf
yasyf merged commit cd2b30c into main Oct 3, 2026
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant