feat: auto-pull claude-custom main at session start #165

Merged

forgejo_admin merged 1 commit from 164-session-start-auto-pull into main

2026-03-26 04:31:02 +00:00

forgejo_admin commented

2026-03-26 04:20:16 +00:00

Contributor

Summary

SessionStart hook now auto-pulls claude-custom main so newly merged hooks and settings.json are live without manual git pull. Prevents the stale-hook gap that allowed two unapproved GroupMe messages (2026-03-25 incident).

Changes

hooks/check-claude-custom-clean.sh -- Added git fetch origin main && git pull origin main --ff-only before the dirty-state check. Four scenarios handled:
- Dirty working tree on main: skips pull, warns user to stash first
- Diverged main: warns user to rebase manually
- Network failure: warns hooks may be stale
- Feature branch: skips pull entirely (only pulls when on main)

Test Plan

Merge a PR on Forgejo, start new Claude session, verify new hook file appears without manual pull
Create local uncommitted changes on main, start session, verify warning appears and pull is skipped
Start session on a feature branch, verify no pull attempt is made
Disconnect network, start session, verify fetch warning appears but session starts

Review Checklist

Only modifies file listed in issue spec (hooks/check-claude-custom-clean.sh)
Uses --ff-only to avoid merge commits
Never blocks session -- all failures are warnings
Idempotent -- safe to run multiple times
Existing dirty-state warning preserved
No settings.json changes needed (reuses existing SessionStart matcher)

Closes #164
Incident: #160 (GroupMe messages sent without approval due to stale hooks)

## Summary SessionStart hook now auto-pulls claude-custom main so newly merged hooks and settings.json are live without manual `git pull`. Prevents the stale-hook gap that allowed two unapproved GroupMe messages (2026-03-25 incident). ## Changes - `hooks/check-claude-custom-clean.sh` -- Added `git fetch origin main && git pull origin main --ff-only` before the dirty-state check. Four scenarios handled: - **Dirty working tree on main**: skips pull, warns user to stash first - **Diverged main**: warns user to rebase manually - **Network failure**: warns hooks may be stale - **Feature branch**: skips pull entirely (only pulls when on main) ## Test Plan - [ ] Merge a PR on Forgejo, start new Claude session, verify new hook file appears without manual pull - [ ] Create local uncommitted changes on main, start session, verify warning appears and pull is skipped - [ ] Start session on a feature branch, verify no pull attempt is made - [ ] Disconnect network, start session, verify fetch warning appears but session starts ## Review Checklist - [x] Only modifies file listed in issue spec (`hooks/check-claude-custom-clean.sh`) - [x] Uses `--ff-only` to avoid merge commits - [x] Never blocks session -- all failures are warnings - [x] Idempotent -- safe to run multiple times - [x] Existing dirty-state warning preserved - [x] No settings.json changes needed (reuses existing SessionStart matcher) ## Related - Closes #164 - Incident: #160 (GroupMe messages sent without approval due to stale hooks)

forgejo_admin added 1 commit

2026-03-26 04:20:16 +00:00

feat: auto-pull claude-custom main at session start fcc799c796

SessionStart hook now fetches and fast-forward pulls main before
checking dirty state. Ensures newly merged hooks and settings.json
are live without manual git pull. Skips pull on feature branches
and warns (never blocks) on failure.

Closes #164

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

forgejo_admin commented

2026-03-26 04:20:44 +00:00

Author

Contributor

Review: LGTM

Acceptance criteria: All four met. Pull happens before dirty check, ff-only enforced, failures warn-only, existing dirty-state warning preserved.

Correctness:

set -euo pipefail + conditional wrapping (if/if !) means non-zero git returns are handled by branching logic, not by set -e. Correct.
Detached HEAD returns empty from branch --show-current, fallback to "unknown", skips pull block. Correct.
BRANCH computed once, reused in dirty-state section. No redundant git calls.

Observation (not a blocker): On main with dirty tracked files, two warnings appear: "skipping auto-pull" (line 23-27) and the file-list warning (line 48-57). This is intentional -- different information in each (skip reason vs. file list).

Scope: Single file changed, matches issue spec exactly. No unrelated changes.

No fixes needed.

## Review: LGTM **Acceptance criteria:** All four met. Pull happens before dirty check, ff-only enforced, failures warn-only, existing dirty-state warning preserved. **Correctness:** - `set -euo pipefail` + conditional wrapping (`if`/`if !`) means non-zero git returns are handled by branching logic, not by `set -e`. Correct. - Detached HEAD returns empty from `branch --show-current`, fallback to `"unknown"`, skips pull block. Correct. - `BRANCH` computed once, reused in dirty-state section. No redundant git calls. **Observation (not a blocker):** On main with dirty tracked files, two warnings appear: "skipping auto-pull" (line 23-27) and the file-list warning (line 48-57). This is intentional -- different information in each (skip reason vs. file list). **Scope:** Single file changed, matches issue spec exactly. No unrelated changes. No fixes needed.

forgejo_admin commented

2026-03-26 04:22:53 +00:00

Author

Contributor

PR #165 Review

DOMAIN REVIEW

Tech stack: Bash shell script (Claude Code SessionStart hook).

The script adds an auto-pull mechanism to hooks/check-claude-custom-clean.sh that fetches and fast-forwards ~/claude-custom on main before the existing dirty-state check runs. Four scenarios are handled: dirty working tree (skip pull), clean main (pull), diverged main (warn), and network failure (warn). All paths exit 0 -- the session is never blocked.

Shell correctness:

set -euo pipefail is present and all git commands are properly guarded: || echo "unknown", || true, and if conditionals prevent -e from terminating the script on expected failures. Correct.
2>/dev/null on git commands suppresses stderr noise from network or auth failures. Appropriate for a SessionStart hook.
--ff-only prevents accidental merge commits on main. Correct.
BRANCH is computed once at the top and reused in both the auto-pull section and the dirty-state section, avoiding the previous duplication inside the if block. Clean refactor.

Fail-open design: Every error path emits a warning and continues. The exit 0 at the bottom is preserved. This matches the hook contract (warn, never block).

Hook ordering observation: This hook is the second in the SessionStart array (after session-start-context.sh). Ideally auto-pull would run first so that any merged changes to other hooks or config take effect before they execute. However, since settings.json is only parsed once at session init time, reordering within the current session would not pick up settings changes anyway. The current placement is acceptable -- the pull ensures the NEXT session gets the latest hooks. Not a blocker.

BLOCKERS

None.

No new functionality requiring automated tests (shell hook with manual test plan -- consistent with all other hooks in this repo).
No user input to validate (reads only from local git state).
No secrets or credentials in the diff.
No DRY violations in auth/security paths.

NITS

Dual warning on dirty main: When on main with uncommitted tracked changes, the user sees two warnings: (a) "skipping auto-pull" from the new code, then (b) the detailed file list from the existing dirty-state check. Both are useful (one explains the pull skip, the other gives the file list), but they could be consolidated into a single message. Low priority -- the current behavior is clear enough.
Fetch failure message could be more precise: The warning "fetch failed (network issue?)" assumes network failure, but auth errors or misconfigured remotes would also hit this path. Consider "fetch failed (network or remote issue?)" for slightly better diagnostic guidance. Very minor.
DIRTY_TRACKED vs DIRTY naming: The auto-pull section uses DIRTY_TRACKED and the bottom section uses DIRTY, but both use the same filter (grep -v '^??') -- they are semantically identical. The naming difference suggests they differ when they do not. Consider using the same variable name, or computing it once and reusing it (for the main-branch case). Minor clarity improvement.

SOP COMPLIANCE

Branch named after issue: 164-session-start-auto-pull references #164
PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related all present
Related references parent issue: Closes #164, references incident #160
No secrets committed
No scope creep: single file modified, directly addresses the issue
Commit message is descriptive (PR title: feat: auto-pull claude-custom main at session start)

PROCESS OBSERVATIONS

This PR directly addresses the stale-hook gap that allowed the GroupMe incident (#160). The change is minimal, well-scoped, and fail-safe. Deployment frequency impact: positive -- merged hook improvements propagate automatically without manual git pull, reducing the window where a session runs stale hooks. Change failure risk: low -- all error paths are non-blocking with clear user guidance.

VERDICT: APPROVED

## PR #165 Review ### DOMAIN REVIEW **Tech stack**: Bash shell script (Claude Code SessionStart hook). The script adds an auto-pull mechanism to `hooks/check-claude-custom-clean.sh` that fetches and fast-forwards `~/claude-custom` on main before the existing dirty-state check runs. Four scenarios are handled: dirty working tree (skip pull), clean main (pull), diverged main (warn), and network failure (warn). All paths exit 0 -- the session is never blocked. **Shell correctness**: - `set -euo pipefail` is present and all git commands are properly guarded: `|| echo "unknown"`, `|| true`, and `if` conditionals prevent `-e` from terminating the script on expected failures. Correct. - `2>/dev/null` on git commands suppresses stderr noise from network or auth failures. Appropriate for a SessionStart hook. - `--ff-only` prevents accidental merge commits on main. Correct. - `BRANCH` is computed once at the top and reused in both the auto-pull section and the dirty-state section, avoiding the previous duplication inside the `if` block. Clean refactor. **Fail-open design**: Every error path emits a warning and continues. The `exit 0` at the bottom is preserved. This matches the hook contract (warn, never block). **Hook ordering observation**: This hook is the second in the SessionStart array (after `session-start-context.sh`). Ideally auto-pull would run first so that any merged changes to other hooks or config take effect before they execute. However, since `settings.json` is only parsed once at session init time, reordering within the current session would not pick up settings changes anyway. The current placement is acceptable -- the pull ensures the NEXT session gets the latest hooks. Not a blocker. ### BLOCKERS None. - No new functionality requiring automated tests (shell hook with manual test plan -- consistent with all other hooks in this repo). - No user input to validate (reads only from local git state). - No secrets or credentials in the diff. - No DRY violations in auth/security paths. ### NITS 1. **Dual warning on dirty main**: When on main with uncommitted tracked changes, the user sees two warnings: (a) "skipping auto-pull" from the new code, then (b) the detailed file list from the existing dirty-state check. Both are useful (one explains the pull skip, the other gives the file list), but they could be consolidated into a single message. Low priority -- the current behavior is clear enough. 2. **Fetch failure message could be more precise**: The warning `"fetch failed (network issue?)"` assumes network failure, but auth errors or misconfigured remotes would also hit this path. Consider `"fetch failed (network or remote issue?)"` for slightly better diagnostic guidance. Very minor. 3. **`DIRTY_TRACKED` vs `DIRTY` naming**: The auto-pull section uses `DIRTY_TRACKED` and the bottom section uses `DIRTY`, but both use the same filter (`grep -v '^??'`) -- they are semantically identical. The naming difference suggests they differ when they do not. Consider using the same variable name, or computing it once and reusing it (for the main-branch case). Minor clarity improvement. ### SOP COMPLIANCE - [x] Branch named after issue: `164-session-start-auto-pull` references #164 - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related all present - [x] Related references parent issue: Closes #164, references incident #160 - [x] No secrets committed - [x] No scope creep: single file modified, directly addresses the issue - [x] Commit message is descriptive (PR title: `feat: auto-pull claude-custom main at session start`) ### PROCESS OBSERVATIONS This PR directly addresses the stale-hook gap that allowed the GroupMe incident (#160). The change is minimal, well-scoped, and fail-safe. Deployment frequency impact: positive -- merged hook improvements propagate automatically without manual `git pull`, reducing the window where a session runs stale hooks. Change failure risk: low -- all error paths are non-blocking with clear user guidance. ### VERDICT: APPROVED