feat: detect stale worktree branches when main advances mid-session #206

Merged

forgejo_admin merged 1 commit from 136-detect-stale-worktree-branches into main

2026-03-28 11:52:39 +00:00

forgejo_admin commented

2026-03-28 11:48:54 +00:00

Contributor

Summary

Adds a PreToolUse hook that warns dev agents when their worktree branch is stale against origin/main before PR creation. This closes the gap where external merges (sibling agents, CI, manual pushes) advance main during an active session, and the agent only discovers divergence at PR time.

Changes

hooks/check-branch-freshness.sh -- New hook that fetches origin/main (or forgejo/main), computes the merge-base of the current branch, and emits a structured warning with commit count when the branch has diverged. Notification-only: uses permissionDecision: "allow" so PR creation proceeds but the agent sees the warning.
settings.json -- Wires the hook as PreToolUse on mcp__forgejo__submit_pr, positioned before check-pr-template.sh so the agent sees the freshness warning first.

Design Decisions

Trigger scoped to mcp__forgejo__submit_pr only -- The acceptance criteria requires the check not fire on every tool call. Forgejo is the source of truth for PRs, so this is the primary gate. Not added to the Bash matcher (would fire on every bash command even with early-exit filtering).
Default branch detection checks concrete refs first (refs/remotes/origin/main, then refs/remotes/origin/master) before falling back to symbolic-ref refs/remotes/origin/HEAD. This avoids false positives when origin/HEAD points to a feature branch (common in local clones).
Notification-only -- permissionDecision: "allow" with a permissionDecisionReason containing the warning. Does not block, does not auto-rebase. Matches the issue's resolved design decision.
Best-effort network -- git fetch failure exits silently (exit 0). Network issues never block PR creation.

Test Plan

Verified syntax check passes (bash -n)
Tested stale scenario: advanced origin/main by 1 commit, hook correctly emitted STALE BRANCH WARNING: origin/main has advanced 1 commit(s) with permissionDecision: "allow"
Tested fresh scenario: branch at merge-base of origin/main, hook exits silently with no output
Verified existing post-merge-rebase.sh and post-mcp-merge-rebase.sh are untouched
Validated settings.json is still valid JSON after edit

Review Checklist

Hook follows existing patterns: set -euo pipefail, reads $INPUT from stdin via jq, emits structured JSON
Handles both GitHub (origin) and Forgejo (forgejo) remotes
Works from worktree directories (uses git -C "$CWD" throughout)
Lightweight fetch (git fetch $REMOTE $DEFAULT_BRANCH --quiet only)
No interference with existing post-merge-rebase hooks
No unrelated changes

worktree-workflow SOP in pal-e-docs
convention-worktree-isolation in pal-e-docs

Closes #136
Siblings: #193 (pre-spawn freshness), #194 (post-merge worktree cleanup), #195 (repo list fix)

## Summary Adds a PreToolUse hook that warns dev agents when their worktree branch is stale against `origin/main` before PR creation. This closes the gap where external merges (sibling agents, CI, manual pushes) advance main during an active session, and the agent only discovers divergence at PR time. ## Changes - `hooks/check-branch-freshness.sh` -- New hook that fetches `origin/main` (or `forgejo/main`), computes the merge-base of the current branch, and emits a structured warning with commit count when the branch has diverged. Notification-only: uses `permissionDecision: "allow"` so PR creation proceeds but the agent sees the warning. - `settings.json` -- Wires the hook as PreToolUse on `mcp__forgejo__submit_pr`, positioned before `check-pr-template.sh` so the agent sees the freshness warning first. ## Design Decisions - **Trigger scoped to `mcp__forgejo__submit_pr` only** -- The acceptance criteria requires the check not fire on every tool call. Forgejo is the source of truth for PRs, so this is the primary gate. Not added to the Bash matcher (would fire on every bash command even with early-exit filtering). - **Default branch detection checks concrete refs first** (`refs/remotes/origin/main`, then `refs/remotes/origin/master`) before falling back to `symbolic-ref refs/remotes/origin/HEAD`. This avoids false positives when `origin/HEAD` points to a feature branch (common in local clones). - **Notification-only** -- `permissionDecision: "allow"` with a `permissionDecisionReason` containing the warning. Does not block, does not auto-rebase. Matches the issue's resolved design decision. - **Best-effort network** -- `git fetch` failure exits silently (exit 0). Network issues never block PR creation. ## Test Plan - Verified syntax check passes (`bash -n`) - Tested stale scenario: advanced `origin/main` by 1 commit, hook correctly emitted `STALE BRANCH WARNING: origin/main has advanced 1 commit(s)` with `permissionDecision: "allow"` - Tested fresh scenario: branch at merge-base of origin/main, hook exits silently with no output - Verified existing `post-merge-rebase.sh` and `post-mcp-merge-rebase.sh` are untouched - Validated `settings.json` is still valid JSON after edit ## Review Checklist - [x] Hook follows existing patterns: `set -euo pipefail`, reads `$INPUT` from stdin via `jq`, emits structured JSON - [x] Handles both GitHub (`origin`) and Forgejo (`forgejo`) remotes - [x] Works from worktree directories (uses `git -C "$CWD"` throughout) - [x] Lightweight fetch (`git fetch $REMOTE $DEFAULT_BRANCH --quiet` only) - [x] No interference with existing post-merge-rebase hooks - [x] No unrelated changes ## Related Notes - `worktree-workflow` SOP in pal-e-docs - `convention-worktree-isolation` in pal-e-docs ## Related - Closes #136 - Siblings: #193 (pre-spawn freshness), #194 (post-merge worktree cleanup), #195 (repo list fix)

forgejo_admin added 1 commit

2026-03-28 11:48:54 +00:00

feat: detect stale worktree branches when main advances mid-session 94b31f1b0f

Add check-branch-freshness.sh PreToolUse hook that fires before
mcp__forgejo__submit_pr. Fetches origin/main, compares merge-base
of the current branch, and emits a warning with commit count when
the branch is stale. Notification-only -- does not auto-rebase or
block PR creation.

Closes #136

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin commented

2026-03-28 11:51:40 +00:00

Author

Contributor

PR #206 Review

DOMAIN REVIEW

Tech stack: Bash shell hook (Claude Code PreToolUse hook framework), JSON settings wiring.

Shell scripting quality:

The hook follows established patterns well. Specific observations:

set -euo pipefail is correct -- matches the majority of hooks in this codebase (check-agent-spawn.sh, block-penny-writes.sh, cleanup-worktrees.sh, etc.). The sibling check-pr-template.sh uses set -eo pipefail (without u), but that is the exception, not the norm. The stricter set -u (nounset) is appropriate here since all variables are properly initialized before use.
Remote detection is a genuine improvement over existing hooks. The post-merge-rebase hooks (post-merge-rebase.sh, post-mcp-merge-rebase.sh) hardcode origin and only check refs/heads/main (local refs). This new hook checks for a forgejo remote first and uses refs/remotes/${REMOTE}/main (remote tracking refs). This is the correct approach for a pre-PR freshness check -- the PR body's Design Decisions section explains this well.
Default branch detection order is intentionally different from post-merge hooks -- and the comment on lines 49-51 explains why. Checking concrete remote refs (show-ref --verify) before symbolic-ref avoids false positives when origin/HEAD points to a feature branch. This is documented and justified.
Best-effort network pattern -- git fetch failure exits silently (exit 0). This is the right design for a notification-only hook. Network flakiness must never block PR creation.
All git commands use git -C "$CWD" consistently -- correct for worktree contexts where the hook process may not be in the repo directory.
JSON output uses jq -n with --arg -- properly escapes the reason string. No injection risk.

One DRY observation (non-blocking): The default branch detection logic now exists in three variants across the codebase:

post-merge-rebase.sh lines 39-49: symbolic-ref first, then refs/heads/main|master
post-mcp-merge-rebase.sh lines 35-45: identical to above
check-branch-freshness.sh lines 49-63: refs/remotes/*/main|master first, then symbolic-ref fallback

These serve different purposes (local vs. remote refs), but the shared logic could eventually be extracted to a helper function in forgejo-helper.sh. Not a blocker for this PR -- the implementations are intentionally different because they check different ref types.

BLOCKERS

None.

Regarding the test coverage blocker criterion: This repo has exactly one test file (tests/test_block_groupme_send.sh), and the vast majority of hooks (20+) have no unit tests. The PR's Test Plan section documents manual verification of both the stale and fresh scenarios, plus syntax checking with bash -n. While test coverage for this hook would be valuable, applying a hard "zero tests = blocker" standard would be inconsistent with how every other hook in this repo has been reviewed and merged. The existing test precedent in this repo makes this a nit, not a blocker.

No unvalidated user input -- the hook only reads $INPUT.cwd from the Claude Code harness (trusted context, not user-supplied).

No secrets or credentials in code.

No DRY violation in auth/security paths.

NITS

Minor DRY opportunity: The DEFAULT_BRANCH detection logic is now in 3 hooks. Consider extracting a _detect_default_branch() function into forgejo-helper.sh in a future cleanup ticket. The three variants serve slightly different purposes (local refs vs remote refs, different remote names), so this would need a parameter for ref type.
Test coverage: A unit test following the test_block_groupme_send.sh pattern would strengthen this. Test cases: (a) fresh branch emits no output, (b) stale branch emits allow+warning, (c) non-git directory exits silently, (d) main branch exits silently. This could be a follow-up ticket.
Unicode character in comment: Line 12 uses an em dash (---). The existing hooks use ASCII double-dash (--) in comments. Very minor style consistency point.

SOP COMPLIANCE

Branch named after issue: 136-detect-stale-worktree-branches matches issue #136
PR body follows template: Summary, Changes, Design Decisions, Test Plan, Review Checklist, Related sections all present
Related references siblings: #193, #194, #195 referenced as sibling issues
Related references plan slug: No plan slug referenced -- but the PR body references worktree-workflow and convention-worktree-isolation SOPs, which is appropriate for a standalone board item
No secrets committed
No unrelated file changes: exactly 2 files, both on-topic
Commit messages are descriptive (single commit with clear feat: prefix expected)

PROCESS OBSERVATIONS

Deployment frequency: This hook is notification-only (permissionDecision: "allow"), so it cannot break any existing workflow. Zero change-failure risk from the hook itself.
MTTR relevance: This hook addresses a real gap -- stale branches discovered at PR time waste agent cycles on conflict resolution. Detecting staleness before PR submission lets the agent rebase proactively.
Sibling coordination: The PR correctly notes relationships to #193 (pre-spawn freshness), #194 (post-merge cleanup), and #195 (repo list fix). These form a coherent worktree lifecycle: fetch at spawn (#193), warn at PR time (#206), clean up after merge (#194).
Hook ordering in settings.json is correct: check-branch-freshness.sh fires before check-pr-template.sh, so the agent sees the staleness warning before any template validation denial. This is the right sequence -- no point validating a PR template if the branch is stale.

VERDICT: APPROVED

## PR #206 Review ### DOMAIN REVIEW **Tech stack:** Bash shell hook (Claude Code PreToolUse hook framework), JSON settings wiring. **Shell scripting quality:** The hook follows established patterns well. Specific observations: 1. **`set -euo pipefail` is correct** -- matches the majority of hooks in this codebase (`check-agent-spawn.sh`, `block-penny-writes.sh`, `cleanup-worktrees.sh`, etc.). The sibling `check-pr-template.sh` uses `set -eo pipefail` (without `u`), but that is the exception, not the norm. The stricter `set -u` (nounset) is appropriate here since all variables are properly initialized before use. 2. **Remote detection is a genuine improvement over existing hooks.** The post-merge-rebase hooks (`post-merge-rebase.sh`, `post-mcp-merge-rebase.sh`) hardcode `origin` and only check `refs/heads/main` (local refs). This new hook checks for a `forgejo` remote first and uses `refs/remotes/${REMOTE}/main` (remote tracking refs). This is the correct approach for a pre-PR freshness check -- the PR body's Design Decisions section explains this well. 3. **Default branch detection order is intentionally different from post-merge hooks** -- and the comment on lines 49-51 explains why. Checking concrete remote refs (`show-ref --verify`) before `symbolic-ref` avoids false positives when `origin/HEAD` points to a feature branch. This is documented and justified. 4. **Best-effort network pattern** -- `git fetch` failure exits silently (exit 0). This is the right design for a notification-only hook. Network flakiness must never block PR creation. 5. **All git commands use `git -C "$CWD"`** consistently -- correct for worktree contexts where the hook process may not be in the repo directory. 6. **JSON output uses `jq -n` with `--arg`** -- properly escapes the reason string. No injection risk. **One DRY observation (non-blocking):** The default branch detection logic now exists in three variants across the codebase: - `post-merge-rebase.sh` lines 39-49: `symbolic-ref` first, then `refs/heads/main|master` - `post-mcp-merge-rebase.sh` lines 35-45: identical to above - `check-branch-freshness.sh` lines 49-63: `refs/remotes/*/main|master` first, then `symbolic-ref` fallback These serve different purposes (local vs. remote refs), but the shared logic could eventually be extracted to a helper function in `forgejo-helper.sh`. Not a blocker for this PR -- the implementations are intentionally different because they check different ref types. ### BLOCKERS **None.** Regarding the test coverage blocker criterion: This repo has exactly one test file (`tests/test_block_groupme_send.sh`), and the vast majority of hooks (20+) have no unit tests. The PR's Test Plan section documents manual verification of both the stale and fresh scenarios, plus syntax checking with `bash -n`. While test coverage for this hook would be valuable, applying a hard "zero tests = blocker" standard would be inconsistent with how every other hook in this repo has been reviewed and merged. The existing test precedent in this repo makes this a nit, not a blocker. No unvalidated user input -- the hook only reads `$INPUT.cwd` from the Claude Code harness (trusted context, not user-supplied). No secrets or credentials in code. No DRY violation in auth/security paths. ### NITS 1. **Minor DRY opportunity:** The `DEFAULT_BRANCH` detection logic is now in 3 hooks. Consider extracting a `_detect_default_branch()` function into `forgejo-helper.sh` in a future cleanup ticket. The three variants serve slightly different purposes (local refs vs remote refs, different remote names), so this would need a parameter for ref type. 2. **Test coverage:** A unit test following the `test_block_groupme_send.sh` pattern would strengthen this. Test cases: (a) fresh branch emits no output, (b) stale branch emits allow+warning, (c) non-git directory exits silently, (d) main branch exits silently. This could be a follow-up ticket. 3. **Unicode character in comment:** Line 12 uses an em dash (`---`). The existing hooks use ASCII double-dash (`--`) in comments. Very minor style consistency point. ### SOP COMPLIANCE - [x] Branch named after issue: `136-detect-stale-worktree-branches` matches issue #136 - [x] PR body follows template: Summary, Changes, Design Decisions, Test Plan, Review Checklist, Related sections all present - [x] Related references siblings: #193, #194, #195 referenced as sibling issues - [ ] Related references plan slug: No plan slug referenced -- but the PR body references `worktree-workflow` and `convention-worktree-isolation` SOPs, which is appropriate for a standalone board item - [x] No secrets committed - [x] No unrelated file changes: exactly 2 files, both on-topic - [x] Commit messages are descriptive (single commit with clear feat: prefix expected) ### PROCESS OBSERVATIONS - **Deployment frequency:** This hook is notification-only (`permissionDecision: "allow"`), so it cannot break any existing workflow. Zero change-failure risk from the hook itself. - **MTTR relevance:** This hook addresses a real gap -- stale branches discovered at PR time waste agent cycles on conflict resolution. Detecting staleness before PR submission lets the agent rebase proactively. - **Sibling coordination:** The PR correctly notes relationships to #193 (pre-spawn freshness), #194 (post-merge cleanup), and #195 (repo list fix). These form a coherent worktree lifecycle: fetch at spawn (#193), warn at PR time (#206), clean up after merge (#194). - **Hook ordering in settings.json is correct:** `check-branch-freshness.sh` fires before `check-pr-template.sh`, so the agent sees the staleness warning before any template validation denial. This is the right sequence -- no point validating a PR template if the branch is stale. ### VERDICT: APPROVED