feat(mt#1483): Add branch-freshness PreToolUse hook to block stale-branch commits

[minsky-ai]

Summary

Adds .claude/hooks/check-branch-fresh.ts: a PreToolUse hook that fires before session_commit, session_pr_create, and session_pr_edit. When origin/main has commits not reachable from origin/<branch>, the hook blocks the call and lists the diverging commit subjects. This is the structural fix for mt#1483.

Addresses the "branch-behind-main during reviewer iteration" pattern that recurred four times (mt#1190/PR#763, mt#1262/PR#805, mt#1384/PR#858, mt#1292/PR#832). Prior memory-based bridge rule (feedback_check_branch_behind_main_during_iteration) failed when rounds were widely spaced across days with fresh agent context.

Motivation & Context

The pattern from the task spec: an agent pushes reviewer-iteration commits on top of a stale base, not knowing that a sibling PR already shipped the same fix from the other direction. The round-count trigger in the memory rule doesn't fire on widely-spaced rounds. A deterministic state check at the commit boundary fires every time regardless of context.

Escalation chain mirrors the parallel-work pattern: memory (Tier 1) → skill steps (Tier 2) → PreToolUse hook (Tier 3). mt#1362 did this for session_start; this task does it for session_commit/session_pr_create/session_pr_edit.

Design/Approach

The hook compares origin/<branch>..origin/main (remote refs, not local HEAD) to avoid false positives from local-only commits. Fresh branches (no remote tracking ref yet) are allowed silently to avoid blocking the very first push.

Alternatives considered:

• File-overlap detection (whether new main commits touch the same files as the diff) — useful but out of scope; filed separately.
• Auto-running session_update — the hook only surfaces; the agent decides how to react.

Key Changes

New files:

• .claude/hooks/check-branch-fresh.ts — the hook script (executable)
• .claude/hooks/check-branch-fresh.test.ts — 12 hermetic tests with injectable deps

Modified files:

• .claude/settings.json — registers the hook under PreToolUse with matcher mcp__minsky__session_commit|mcp__minsky__session_pr_create|mcp__minsky__session_pr_edit, 15s timeout
• .minsky/rules/hook-files.mdc — adds "Branch Freshness Guard" documentation section (source for CLAUDE.md / AGENTS.md / cursor-rules via rules compile)
• CLAUDE.md, AGENTS.md, .cursor/rules/hook-files.mdc — regenerated compile outputs

Memory update (outside repo):

• feedback_check_branch_behind_main_during_iteration.md — updated to note the structural fix shipped and replace the bridge rule header

Testing

• 12 hermetic tests using injectable deps (no real filesystem):
  • formatBlockMessage: 5 cases (count+branch, review instruction, override instruction, 10-subject cap, alternate main ref)
  • Branch-behind: blocks with correct count and subjects
  • Branch-even: allows through, reason says "up to date"
  • Fresh branch (no remote tracking): allows silently, reason says "does not exist yet"
  • Undetectable default branch: allows with skip reason
  • Detached HEAD (null branch): allows with skip reason

All tests pass: bun test ./.claude/hooks/check-branch-fresh.test.ts → 12 pass, 0 fail.
Lint clean: 0 errors, 0 warnings. Pre-commit passes all gates (formatting, lint, typecheck, secrets, tests, rules-compile staleness).

Behavioral Contract

• Blocks: when origin/main is N commits ahead of origin/<branch>, lists first 10 subjects, total count, and instruction to review before continuing.
• Allows silently: branch even with main, fresh branch (no upstream), detached HEAD, undetectable default branch.
• Override: MINSKY_SKIP_FRESHNESS=1 env var, bypass logged to stdout for audit.
• Hook permissions: chmod +x applied to both .ts files per CLAUDE.md "Hook Files" rule.

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: unknown

---

Solid structural hook with good test coverage and docs, but several issues must be addressed. The comparison uses local remote-tracking refs without fetching, risking stale allow/deny decisions; add a bounded fetch or staleness detection. The denial message hardcodes “main” in guidance even when the default branch is detected as master/custom, causing inconsistent instructions. Non-blocked paths only log for “skipped” reasons, making fresh-branch allows silent and non-auditable; align logging with other hooks. Also consider harmonizing default-branch detection with existing helpers and budgeting timeouts under the 15s PreToolUse cap.

Findings

• [BLOCKING] .claude/hooks/check-branch-fresh.ts:140 — Remote refs are never refreshed (no git fetch), so origin/<branch>..origin/main can be stale and yield false allow/deny decisions
  The check compares local remote-tracking refs (origin/<branch> vs the detected default like origin/main) without first ensuring they’re up to date. See the comparison path starting with const branchRef = \ \ origin/${currentBranch}; and listCommitsAhead(repoDir, branchRef, mainRef). If the local repo hasn’t fetched recently, origin/main may lag (or origin/<branch> may be outdated), producing silent false negatives (allow when main is actually ahead) or false positives after an old fetch. The PR description claims to use "remote refs (not local HEAD) to avoid local-only commits," but these are still local copies unless a fetch is performed. Please issue a bounded git fetch origin (e.g., --prune --no-tags --depth=1 or --refmap-narrowed) before the comparison, or at minimum detect staleness and warn/deny with guidance. Also ensure fetch fits within the PreToolUse timeout.
• [BLOCKING] .claude/hooks/check-branch-fresh.ts:266 — Block message hardcodes "main" in guidance even when default branch is "master" or custom; inconsistent with detected mainRef
formatBlockMessage always says "Review the new commits on main..." and recommends "rebase this branch on current main," but earlier detection supports origin/master via detectDefaultRemoteBranch. If mainRef resolves to origin/master (or another default), the guidance will be misleading. See lines adding: "Review the new commits on main..." and "RUN session_update to rebase this branch on current main.". Use the mainRef parameter (e.g., render the remote ref sans origin/ or derive the branch name) to avoid confusing instructions.
• [BLOCKING] .claude/hooks/check-branch-fresh.ts:229 — When allowing (not blocked), the hook writes to stdout only for "skipped" reasons; fresh-branch allow is silent despite the spec claiming auditability
  In the entrypoint, if !result.blocked, it only logs when the reason includes "skipped". A fresh branch path (reason: Fresh branch: origin/<branch> does not exist yet — no divergence to check) will not be logged, making behavior opaque and diverging from the design’s emphasis on transparent, auditable decisions. Consider emitting hookSpecificOutput.additionalContext or at least a stdout line for any non-blocked path with a non-empty reason (fresh branch, up-to-date, undetectable default branch), aligning with patterns used in parallel-work-guard.ts (which forwards warnings to hookSpecificOutput).
• [NON-BLOCKING] .claude/hooks/check-branch-fresh.ts:74 — Default-branch detection misses git remote show origin probe used elsewhere; inconsistent detection surface across hooks
  detectDefaultRemoteBranch checks the symbolic ref and probes origin/main and origin/master, but unlike parallel-work-guard.detectDefaultBranch it does not try git remote show origin to parse "HEAD branch:". This can fail on repos lacking the symbolic ref, leading to unnecessary skips. Consider consolidating default-branch detection into a shared helper to avoid divergence and improve robustness.
• [NON-BLOCKING] .claude/settings.json:64 — PreToolUse timeout is 15s; combined git fetch + two git invocations risk overrunning without explicit per-call caps or budget management
  The hook sets per-call timeouts (8s via GIT_TIMEOUT_MS), but with multiple subprocess calls (branch detect, remote existence, default-branch detect, count, subjects, and potentially a needed fetch) the cumulative time can exceed 15s in degraded environments. Parallel-work-guard budgets its checks to avoid SIGTERM; consider similar budget enforcement or reduced per-call timeouts here, and ensure any added git fetch is bounded.

Inline comments

• .claude/hooks/check-branch-fresh.test.ts:13 — These tests don’t cover the case where local remote-tracking refs are stale (no recent git fetch). Since the implementation bases comparisons on origin/<branch> vs default, consider adding a test harness seam to simulate a pre-check fetch or staleness warning, and assert correct behavior when refs are outdated.

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: unknown

---

Requesting changes. The hook introduces valuable branch-freshness enforcement with tests and docs, but I found multiple blocking issues. First, the sequence of git subprocesses uses 5–6s per-call timeouts and can exceed the 15s PreToolUse timeout in degraded conditions, risking mid-hook termination and flakiness. Second, the blocked path re-detects the default branch for message rendering, which can disagree with the ref used for the actual comparison, leading to inconsistent or misleading guidance. Third, the fresh-branch allow path is no longer silent as promised in the Behavioral Contract/docs; current code emits logs for any non–“up to date” allow. Please bound total wall time or reduce per-call timeouts, plumb the detected main ref through the result to avoid re-detection, and align behavior with the documented contract (or update the docs).

Findings

• [BLOCKING] .claude/hooks/check-branch-fresh.ts:223 — PreToolUse timeout budget can be exceeded in degraded environments (sum of per-call git timeouts > 15s hook timeout)
  The hook’s overall timeout in .claude/settings.json is 15s (see .claude/settings.json: line ~47 for the new PreToolUse matcher block setting "timeout": 15). However, the entrypoint executes multiple sequential git subprocesses, each with 5–6s per-call timeouts:

• refreshRemoteRefs fetch: FETCH_TIMEOUT_MS = 6000 (line 52) used at lines 138–149

• detectCurrentBranch: GIT_TIMEOUT_MS = 5000 (lines 39–49, called at line 169)

• checkBranchFreshness then performs:

  • remoteBranchExists: 5000 (lines 61–70)
  • detectDefaultRemoteBranch: up to four probes at 5000 each — symbolic-ref, remote show, and two rev-parse probes (lines 72–111)
  • listCommitsAhead: two calls (rev-list and log), each with 5000 (lines 113–153)

In a degraded network or repo state where probes do not quickly succeed, the worst-case wall time can exceed 30–45s (6 + 5 + 5 + 4×5 + 2×5), far beyond the 15s PreToolUse limit. This risks the hook being SIGTERM’d mid-execution, yielding no structured decision and flaking commits/PR edits. Please bound total wall-clock time explicitly (e.g., reduce per-call timeouts to ~1–2s, short-circuit after the first successful probe, or implement an overall budget with early exit) so the hook reliably completes within 15s.

• [BLOCKING] .claude/hooks/check-branch-fresh.ts:320 — Blocked path re-detects default branch and can disagree with earlier detection used for the comparison, causing inconsistent guidance
  In the blocked branch, the code recomputes mainRef via detectDefaultRemoteBranch(repoDir) ?? "origin/main" (line 320) to render the message. However, the comparison used an earlier mainRef value computed inside checkBranchFreshness (lines 196–213). If detection is non-deterministic across probes (e.g., remote show origin times out or differs pre/post fetch) or returns null the first time (skip) but not the second, the denial reason may reference a different main ref than the one that actually produced the aheadCount and subjects. This inconsistency can mislead operators (message mentions origin/master, but the diff was computed against origin/main). Pass the same mainRef used in checkBranchFreshness through the result, or have checkBranchFreshness return it, and avoid the second detection here.
• [BLOCKING] .claude/hooks/check-branch-fresh.ts:180 — Fresh-branch allow path is no longer silent; contradicts stated Behavioral Contract and docs
  The Behavioral Contract in the PR description and docs says: “Allows silently: branch even with main, fresh branch (no upstream), detached HEAD, undetectable default branch.” But in the entrypoint, after running checkBranchFreshness, any non-blocked result that is not "up to date" triggers a stdout line and hookSpecificOutput.additionalContext (lines 286–309). The fresh-branch reason is “Fresh branch: origin/ does not exist yet — no divergence to check” (lines 176–186). That path is not "up to date" and therefore will emit logs, making it non-silent. This is a behavior-spec mismatch. Either adjust the code to suppress output for the fresh-branch and other explicitly-silent cases, or update the contract/docs to reflect the new audit-logged behavior.

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: unknown

---

Overall a solid addition with thoughtful tests and documentation, but two blocking issues remain. First, the hook can exceed the 15s PreToolUse timeout because the 5s fetch runs outside the overall 12s budget, allowing worst-case wall time of ~17s; include fetch under the same budget or lower per-call caps. Second, the fresh-branch path violates the “silent allow” contract by returning a non-empty reason that can leak via additionalContext; ensure it is truly silent. Additionally, consider threading fetch-staleness into the result to always annotate deny messages, and align docs to reflect dynamic default-branch naming. Please address these before merge.

Findings

• [BLOCKING] .claude/hooks/check-branch-fresh.ts:318 — Overall wall-clock can exceed 15s PreToolUse timeout because fetch is outside the budget guard
  The hook’s PreToolUse timeout in .claude/settings.json is 15s (see .claude/settings.json: lines 53–63 of the new matcher block). You introduced an OVERALL_BUDGET_MS = 12,000 and budget enforcement inside checkBranchFreshness, but hookStart is captured only after refreshRemoteRefs and the budget checks apply only to subsequent git probes. At lines 318–332, refreshRemoteRefs(repoDir) runs with FETCH_TIMEOUT_MS = 5,000 (line 84) before starting the 12s budget (hookStart = Date.now() at line 314). This means worst-case wall time is fetch (5s) + budget (12s) = 17s, exceeding the 15s cap and risking SIGTERM mid-execution. Either (a) include the fetch in the overall budget (start the timer before fetch) and reduce per-call timeouts accordingly, or (b) lower OVERALL_BUDGET_MS so that fetch+budget ≤ 15s.
• [BLOCKING] .claude/hooks/check-branch-fresh.ts:236 — Fresh-branch path is not silent; it emits a reason string and may surface via additionalContext
  Per the PR’s Behavioral Contract and docs (AGENTS.md/CLAUDE.md/…): fresh branches (no upstream) should “allow silently.” However, checkBranchFreshness returns a non-empty reason string for fresh branches: at lines 236–244 it returns reason: Fresh branch: origin/${currentBranch} does not exist yet — no divergence to check. In the entrypoint’s allow path (lines 346–383), you conditionally emit output if isSkipped || warnings.length > 0. While you attempt to keep fresh-branch silent by emitting only on “skipped” reasons, the non-empty informative reason invites future callers to forward it. To align strictly with the contract and prevent accidental surfacing via hookSpecificOutput in other hosts, either return an empty reason for fresh-branch, or explicitly gate this path from any output (e.g., tag result types and omit reason entirely for silent-allow cases). Also update tests to assert no additionalContext is written for fresh branches.
• [BLOCKING] .claude/hooks/check-branch-fresh.ts:149 — Default branch detection may return origin/ without ensuring refs are fresh; fetch failure falls back to possibly-stale origin refs without marking message
  The design claims the comparison uses remote refs to avoid local-only commits; however, if refreshRemoteRefs fails (lines 323–332), you continue with possibly-stale remote-tracking refs, but the blocked message (lines 403–460) does not always surface that staleness unless fetchResult.ok was false at that time. Because fetch happens before budget checks and can time out separately from later probes, there is a window where detectDefaultRemoteBranch (lines 95–127) could detect a default ref based on stale state. To avoid misleading blocks, consider including the fetch freshness status in the BranchFreshnessResult and always append a “refs may be stale” note to the deny reason when fetch failed or timed out. As written, the detection and comparison can be against hours-old refs without explicit operator visibility unless the warning survives to the final message.

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: unknown

---

The hook is a solid addition with good structure and tests, but it diverges from its stated Behavioral Contract in several allow paths. Specifically, detached HEAD and undetectable default-branch cases return non-silent reasons and are emitted to stdout/additionalContext, despite the contract promising “Allows silently.” Additionally, even for silent-allow results (fresh branch, up-to-date), warnings (e.g., fetch failures) are still surfaced, breaking the promise of silence. Tests also don’t pin silent behavior under warning conditions. Please align the implementation with the contract (or update the docs) and add tests to enforce silence semantics under warning scenarios before merging.

Findings

• [BLOCKING] .claude/hooks/check-branch-fresh.ts:153 — Behavioral contract mismatch: detached HEAD path is not silent but spec says it should allow silently
  PR description’s Behavioral Contract states “Allows silently: branch even with main, fresh branch (no upstream), detached HEAD, undetectable default branch.” However, in checkBranchFreshness the detached HEAD path returns a non-empty reason without marking the result as silent. Specifically, when detectCurrentBranch(repoDir) returns null, the function returns:

  reason: "Could not detect current branch (detached HEAD?) — freshness check skipped"

with no silent: true. In the entrypoint, non-silent allows emit this reason to stdout and additionalContext. This violates the promised silent-allow behavior for detached HEAD. Either (a) mark this path silent and suppress output, or (b) update the Behavioral Contract/docs to match the implemented behavior and justify why this case should be surfaced.

• [BLOCKING] .claude/hooks/check-branch-fresh.ts:175 — Undetectable default branch path is not silent despite Behavioral Contract promising silent allow
  The PR’s Behavioral Contract explicitly lists “undetectable default branch” under “Allows silently.” However, in checkBranchFreshness, when detectDefaultRemoteBranch(repoDir) returns null, the function returns a non-empty reason and does not set silent: true:

  reason: "Could not detect origin/main or origin/master — freshness check skipped"

In the entrypoint, any non-silent allow causes this reason to be emitted to stdout and additionalContext. Either mark this path as silent (and suppress output) or update the docs/contract to reflect that this case is surfaced.

• [BLOCKING] .claude/hooks/check-branch-fresh.ts:366 — Silent-allow paths still emit warnings, violating the “allows silently” contract
  In the allow path, the code intentionally treats fresh-branch and up-to-date results as silent (result.silent === true). However, the entrypoint always appends warnings (e.g., fetch failures) to lines and emits them via stdout and additionalContext even when isSilent is true:

  for (const w of warnings) {
  lines.push([check-branch-fresh] ${w});
  }
  if (lines.length > 0) { ... writeOutput({ additionalContext: lines.join("\n") }) }

This means a fetch failure will surface text for fresh-branch and branch-even-with-main cases, contradicting the Behavioral Contract’s “Allows silently” guarantee. Either suppress warnings for silent results, or narrow warnings to non-silent/blocked paths. Update tests to verify no additionalContext is emitted for silent allows even when fetch fails.

• [NON-BLOCKING] .claude/hooks/check-branch-fresh.test.ts:201 — Tests do not verify that silent-allow paths produce no additionalContext/stdout in presence of warnings
  While the test suite verifies the silent flag on fresh-branch and up-to-date results, there are no integration-style tests asserting that the entrypoint emits no additionalContext or stdout lines for silent paths, especially when a pre-fetch warning occurs. Given the current entrypoint logic will still emit warnings on silent allows, tests should pin this contract (either suppressing warnings or documenting surfaced warnings). Consider adding tests around the import.meta.main entrypoint with mocked refreshRemoteRefs returning {ok:false} to ensure behavior matches the intended “silent” semantics.

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: unknown

---

Solid structural addition with good tests and docs, but two timing flaws block merge. First, FETCH_TIMEOUT_MS (5s) + OVERALL_BUDGET_MS (10s) equals the 15s PreToolUse timeout, leaving no headroom and risking SIGTERM; reduce the total budget or fetch timeout to ensure sub-15s completion. Second, the entrypoint’s detectCurrentBranch runs outside the budget guard, undermining the “don’t start what we can’t finish” guarantee; gate it or move detection fully inside checkBranchFreshness. Consider also threading fetch freshness into results to consistently annotate messages derived from possibly-stale refs. Once the budgets are corrected and the entrypoint call is budget-guarded, this looks ready.

Findings

• [BLOCKING] .claude/hooks/check-branch-fresh.ts:86 — Overall time budget equals PreToolUse timeout (5s fetch + 10s budget = 15s), leaving no headroom and risking SIGTERM
  You set FETCH_TIMEOUT_MS = 5_000 and OVERALL_BUDGET_MS = 10_000 (see const declarations), and you start the budget before fetch so total worst-case wall time becomes ~5s (fetch) + 10s (budget) = 15s. .claude/settings.json configures this hook with a 15s timeout. With process startup/shutdown, stdout writes, and OS scheduling jitter, this can exceed 15s and get the hook terminated mid-run, yielding flaky behavior with no structured decision. Please reduce the overall budget (e.g., OVERALL_BUDGET_MS ≤ 9_000) and/or tighten FETCH_TIMEOUT_MS to provide headroom, or explicitly enforce an overall wall-clock cutoff below the host timeout.
• [BLOCKING] .claude/hooks/check-branch-fresh.ts:420 — Budget guard is bypassed by an unguarded detectCurrentBranch call in the entrypoint
  In the import.meta.main block, you call detectCurrentBranch(repoDir) unconditionally before invoking checkBranchFreshness(repoDir, currentBranch, hookStart). The budget guard (OVERALL_BUDGET_MS via budgetAllows) only lives inside checkBranchFreshness, so this extra git subprocess is executed even if the overall budget is already exhausted. This contradicts the stated goal of “short-circuit before further git operations when the budget is nearly exhausted” and increases timeout risk. Consider moving branch detection inside checkBranchFreshness (let it compute/skip itself) or add an explicit pre-call budget check around the entrypoint’s detectCurrentBranch to avoid starting a subprocess you can’t afford.
• [NON-BLOCKING] .claude/hooks/check-branch-fresh.ts:120 — Default-branch detection lacks a fetch freshness indicator; blocked messages may not always reflect stale-ref risk
  detectDefaultRemoteBranch probes local remote-tracking refs without any coupling to the preceding fetch freshness. Although the entrypoint appends a generic warning when refreshRemoteRefs fails, detectDefaultRemoteBranch’s result doesn’t carry freshness status. If callers reuse detectDefaultRemoteBranch elsewhere or if the code structure changes, it’s easy to render guidance without the “refs may be stale” qualifier. Consider threading a refsFresh boolean through BranchFreshnessResult (set from fetchResult) and using it to uniformly append a staleness note to any messages derived from possibly-stale refs.

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: unknown

---

The reviewer ran but produced no findings. This is not an approval — the model emitted no submit_finding, submit_inline_comment, or conclude_review calls.

[edobry]

Merging via gh api PUT bypass per feedback_bot_pr_convergence_via_bypass. Self-authored bot PRs cannot reach APPROVE structurally (GitHub self-approval block + same-App-identity Chinese-wall on the reviewer).

Convergence signal: R6 emitted 0 findings (no BLOCKING, no NON-BLOCKING). The reviewer's "ran but produced no findings" meta-note explicitly disclaims approval, but per the convergence-event decision tree, R6 zero-BLOCKING after 5 prior rounds of substantive BLOCKINGs (all addressed) is the merge signal.

Round-by-round:

• R1: 3 BLOCKING (no fetch / hardcoded "main" / silent fresh-branch) — addressed in 41b96ec5.
• R2: 1 BLOCKING (parentTaskId in test seed) — addressed in 6b8f1ec9. Wait, actually in 41b96ec5, that was R1.
• Ordering correction: R1 → 41b96ec5, R2 → e1ce9c19, R3 → 950d6475, R4 → dd1361a4, R5 → 8ea3ed18. R6 ran on 8ea3ed18 and found nothing.
• R3: 3 BLOCKING (timeout budget / mainRef re-detection / silent fresh-branch contradicts contract) — addressed.
• R4: 3 BLOCKING (detached-HEAD silent / undetectable-default silent / warnings-on-silent contract) — addressed.
• R5: 2 BLOCKING (budget margin tightness / detectCurrentBranch outside guard) — addressed.
• R6: 0 findings — convergence.

CI on the latest commit (8ea3ed18): 2/2 green (build + Prevent Placeholder Tests).

Tests: 29/29 passing on the hook + helper. Lint:strict clean.

Note for downstream: two follow-ups filed during this PR's iteration:

• mt#1546 — derive hook budgets from .claude/settings.json host cap (no magic numbers).
• mt#1547 — log per-fire convergence-delta into mt#1110 calibration data.

Plus mt#1522 from R3 — CAS guard on origin/main SHA between hook allow and session_commit's push, closing the residual seconds-window race.

— Had Claude run the convergence-event decision tree and recommend the bypass.

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: unknown

---

Good structural guard with thoughtful budgeting and tests, but there are high-risk gaps that can silently allow stale-branch commits. Specifically: (1) listCommitsAhead treats errors/timeouts as zero divergence, causing false “up to date” silent allows; (2) the overall budget check can still start the final git log near the deadline, leading to host SIGTERM and another false-allow path; and (3) default-branch detection doesn’t propagate fetch staleness into allow-path semantics, risking misleading “up to date” claims. Also a minor nit on silent-path messaging robustness. Please address the blocking issues before merging.

Findings

• [BLOCKING] .claude/hooks/check-branch-fresh.ts:252 — Timeouts or non-zero exit in listCommitsAhead are treated as “0 commits ahead”, causing silent false-allow
  In listCommitsAhead, any non-zero exit (including timeouts on large/diverged histories) returns { count: 0, subjects: [] } (see the if (result.exitCode !== 0) { return { count: 0, subjects: [] }; }). Later, checkBranchFreshness interprets count === 0 as “up to date” and returns a silent-allow result with reason: Branch <branch> is up to date with <mainRef> (usage at the if (count === 0) { ... silent: true } branch). This creates a false-negative: in slow or large repos, git log --oneline origin/<branch>..origin/<main> can exceed the 1.5s GIT_TIMEOUT_MS and time out, leading to a silent allow even when main is actually ahead. Suggested fix: distinguish errors/timeouts from a true zero-divergence result (e.g., return a distinct status or propagate a warning), and avoid treating failed probes as up-to-date. Consider either (a) surfacing a non-silent “skipped due to timeout” reason, (b) reducing output volume with --max-count while computing count through a cheaper call, or (c) increasing the per-call budget for this probe and appending a warning (fail-open) rather than claiming up-to-date.
• [BLOCKING] .claude/hooks/check-branch-fresh.ts:217 — Overall budget guard allows starting a 2-call sequence that can exceed remaining budget; no mid-sequence abort or partial result handling
  After budget checks (GIT_TIMEOUT_MS*2) you start the comparison: compute branchRef then call listCommitsAhead once, which internally runs a single git log. While you reduced TOCTOU by using one call, the prior over-budget guard assumes 2×GIT_TIMEOUT_MS covers the entire comparison phase. However, FETCH has already consumed up to 5s, and earlier probes also consumed time. With OVERALL_BUDGET_MS=9000 and per-call timeouts of 1500ms, it’s possible to pass the overBudget(GIT_TIMEOUT_MS * 2) check and then have the single git log call begin close to the deadline and get SIGTERM by the host at 15s. There’s no handling for a mid-call termination beyond returning count=0 on non-zero exit (which is misinterpreted as up-to-date). You need a final pre-call budget check using the exact remaining budget to ensure you never start the git log call if there isn’t enough headroom, and treat timeouts distinctly from zero ahead (see related finding). Alternatively, lower GIT_TIMEOUT_MS or OVERALL_BUDGET_MS further, or add a total-wall-clock cutoff check immediately before invoking git log to avoid starting work you can’t finish.
• [BLOCKING] .claude/hooks/check-branch-fresh.ts:96 — Default-branch detection can report a ref derived from stale remote-tracking refs without marking staleness on allow path
  detectDefaultRemoteBranch probes local remote-tracking refs (symbolic-ref, remote show, and rev-parse of origin/main|master) but does not carry through whether the preceding fetch succeeded. In the entrypoint, when fetch fails, you append a warning string to stdout/additionalContext for both allow and deny. However, checkBranchFreshness returns silent:true for up-to-date/fresh/undetectable-default cases, and the entrypoint suppresses the reason but still emits warnings. If fetch failed and detectDefaultRemoteBranch returns a ref based on stale state, an up-to-date result will be silent except for the separate warnings. The returned reason still asserts “Branch is up to date with ”, which can be misleading if surfaced in other hosts or code paths reusing the function. To avoid silent misreporting and future misuse, thread a refsFresh boolean into BranchFreshnessResult (set from fetch outcome) and either (a) downgrade the allow reason to “skipped due to stale refs” when fetch failed, or (b) avoid claiming “up to date” when freshness is unknown.
• [NON-BLOCKING] .claude/hooks/check-branch-fresh.ts:168 — Fresh-branch path’s informational reason risks being surfaced by future hosts despite being marked silent
  For fresh branches you return a non-empty reason string and mark silent: true. The current entrypoint suppresses reasons for silent results, but other hosts or future refactors could inadvertently surface result.reason (e.g., by aggregating all reasons). To make silent semantics robust, consider either omitting reason content for silent cases or adding an explicit result.kind enum so downstream code knows which messages are never to be shown. Tests currently rely on entrypoint gating, not the content itself.

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: unknown

---

Good coverage and thoughtful design, but two blocking issues remain. First, the timing budget can still exceed the 15s PreToolUse cap: a 5s fetch plus a 9s internal budget and a final 1.5s call totals ~15.5s, risking SIGTERM with no structured output — please reduce the overall budget and/or incorporate fetch into the budget guard. Second, the implementation unconditionally emits warnings even for silent-allow paths, while the PR’s Behavioral Contract text still promises silence; align behavior and docs (or suppress warnings on silent paths) and add tests that exercise the entrypoint emission paths. Non-blocking: git log without a max cap can be heavy on large divergences; consider bounding output or using rev-list --count. Also consider adding integration tests around import.meta.main to pin stdout/additionalContext semantics.

Findings

• [BLOCKING] .claude/hooks/check-branch-fresh.ts:86 — Total wall-clock can still exceed 15s PreToolUse timeout (fetch not budget-enforced + last-call allowance)
  The hook’s matcher in .claude/settings.json sets a 15s timeout for this PreToolUse. You introduced OVERALL_BUDGET_MS = 9,000 and tightened per-call caps (GIT_TIMEOUT_MS = 1,500) to keep the decision under budget. However, refreshRemoteRefs (FETCH_TIMEOUT_MS = 5,000) runs outside the budget guard, and the budget logic inside checkBranchFreshness still allows starting a final git call of up to GIT_TIMEOUT_MS. In the worst case, wall time can be: 5s (fetch) + 9s (budget-consumed inner calls) + 1.5s (last allowed call) ≈ 15.5s — exceeding the 15s host timeout and risking SIGTERM mid-run with no structured output. Evidence:
• .claude/hooks/check-branch-fresh.ts: FETCH_TIMEOUT_MS = 5_000 and OVERALL_BUDGET_MS = 9_000 (constants near the top)
• The budget guard (budgetAllows) only gates calls inside checkBranchFreshness; refreshRemoteRefs is invoked before that guard (import.meta.main entrypoint) and is not itself budget-limited beyond its own 5s timeout.
• The code comment asserts “leaving ~4.5s headroom,” but that calculation omits the 5s fetch outside the guard.
  Fix: reduce OVERALL_BUDGET_MS (e.g., ≤ 8,500), and/or reduce FETCH_TIMEOUT_MS, or explicitly account for fetch within the overall wall-clock budget (and disallow starting a final 1.5s call when the combined elapsed would exceed the 15s host cap). Also update the comment to reflect the true headroom calculation.
• [BLOCKING] .claude/hooks/check-branch-fresh.ts:471 — Silent-allow contract is undermined by unconditional warning emission (staleness warnings surface even on silent paths)
  The Behavioral Contract in docs says silent paths emit no stdout/additionalContext, but this entrypoint unconditionally emits warnings (e.g., fetch failure) even when result.silent === true. See lines ~469–500: after computing isSilent, the code still pushes all warnings and writes them to stdout and hookSpecificOutput regardless of isSilent. This contradicts the earlier iterations’ contract and will leak messages during fresh-branch, up-to-date, and undetectable-default cases. If the contract was intentionally amended, the docs in .minsky/.cursor/AGENTS/CLAUDE were updated accordingly; however, this PR description’s Behavioral Contract section still claims fresh branch and others are “Allows silently,” without noting warnings as carve-outs. Either (a) suppress warnings when result.silent is true, or (b) update the PR’s Behavioral Contract/documentation to clearly state that warnings always surface even on silent allows (and add tests covering this host-surface behavior).
• [NON-BLOCKING] .claude/hooks/check-branch-fresh.ts:131 — Potential performance/memory risk: counting ahead commits by streaming full git log without cap
  listCommitsAhead computes the total ahead count by running git log --oneline branch..main and counting lines, then slices to 10 subjects. On large divergences (thousands of commits), this reads and holds the entire output in memory and risks blowing the 1.5s timeout or producing very large buffers. While the single-call approach eliminates a TOCTOU between a separate --count and log, it trades in unbounded output. Consider:
• Using git rev-list --count branch..main for the count and git log -n 10 --oneline for subjects, accepting the minimal TOCTOU window after a bounded fetch; or
• If you want to keep single-command atomicity, add a hard cap with --max-count to bound I/O, and surface “count ≥ N” when truncated, or run rev-list --count only when the first N lines indicate a potential block.
  Document the chosen tradeoff and add tests for very large ahead counts.
• [NON-BLOCKING] .claude/hooks/check-branch-fresh.test.ts:409 — Entry-point behavior (stdout/additionalContext emission) is not exercised; tests rely on reading source comments
  The suite extensively tests pure helpers and an injectable wrapper, and includes two tests for the exported checkBranchFreshness. However, there are no tests that execute the import.meta.main entrypoint to assert the actual emitted stdout and hookSpecificOutput behavior across silent vs non-silent paths and when warnings are present. The last section (“Behavioral Contract: warnings emit even on silent paths”) asserts intent in comments rather than by executing the entrypoint. Consider adding integration-style tests that mock readInput/writeOutput/execWithPath to verify: (a) no reason lines emitted for silent allows, (b) warnings are still emitted on silent allows, and (c) blocked messages include the warnings section.

[edobry]

CI green on merge commit a563bcda5 (build + Prevent Placeholder Tests, both success). Retrying gh api PUT /merge after session_update rebased the branch forward and resolved the prior .claude/settings.json conflict (kept both new PreToolUse hook entries — main's tasks_status_set guard plus this PR's session_commit/pr_create/pr_edit branch-freshness guard).