fix(claude): make the session-start hook runnable, and give the agent config teeth - #2113
Merged
Conversation
…config teeth `.claude/hooks/session-start.sh` was committed as mode 100644 while both its siblings were 100755, and it is the only hook registered by bare path rather than through `bash`. Its whole body is gated on CLAUDE_CODE_REMOTE=true, so the one environment it does any work in is a Linux web container — exactly where a non-executable checkout cannot run. It is also the script that provisions the Node 24 the engine floor requires, after npm ci EBADENGINE blocked PRs #1611, #1697, #1705 and #1740. The defect was invisible locally: the primary workstation is a Windows ReFS Dev Drive with core.fileMode=false, so git ignores filesystem permission bits and a local `chmod +x` is a silent no-op. Only `git update-index --chmod=+x` can fix it. Fixed three ways so it cannot recur: the index mode, a `bash "..."` registration that stops the mode being load-bearing, and a contract test. Also in this change: - .claude/settings.json gains a permissions block. AGENTS.md's provider confirmation boundary was prose-only; this encodes it as deny/ask rules, including deny on reading .env* (a staging key leaked on 2026-08-18) and on the Supabase MCP write tools. The repo already learned that prose does not hold here — see the comment in pr-handoff-stop.sh. - check-base-freshness.mjs now emits its stale-base warning on stdout as hook JSON. Every human-readable branch used console.error, and Claude Code injects only stdout into context, so the tripwire never reached the agent. The origin/main fetch also gains a 10s timeout so a hung remote cannot burn the whole SessionStart budget. - clean-worktree.mjs gains list-only `--merged` and `--squashed`. Nothing reclaimed merged worktrees, so 49 accumulated, ~19 GB of duplicated node_modules on a 50 GB Dev Drive. Ancestor detection alone finds 2 of 49 because this repo squash-merges; the patch-id test finds 9. A `confidence:` line distinguishes proven from inferred, because the two are not the same claim and one candidate had 2 of 21 files still differing. - Explicit hook timeouts, a PreCompact hook that asks for /issues capture while the context still exists, and a push format guard that only fires where the .githooks pre-push guard is not wired. Removal stays a separate opt-in throughout; `runWorktreeCleanup()` is byte identical, so verify:preflight is unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… run `tests/session-start-hook.test.ts` asserted that the written env file contains `join(home, ".node24", …)`. On Windows `home` comes from mkdtempSync(tmpdir()) as `C:\Users\…\AppData\Local\Temp\session-start-home-XXXX`, while the hook runs under Git Bash and writes the POSIX view of the same directory, `/tmp/ session-start-home-XXXX`. The assertion therefore failed on every Windows run regardless of the diff under test. That is worse than a red test: it is a red test everyone learns to ignore. It fails inside `npm run test`, which is the last step of `verify:pr-local`, so the whole gate goes red locally for every change and the only way to use it is to decide which failures do not count. Compare the path tail instead. The unique mkdtemp basename still pins the assertion to this test's own HOME, so it loses no strength, and it now holds on both platforms. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 35 seconds Limit details: You’ve used all 1 included review currently available under your plan. You completed 101 included PR reviews in the past 7 days; at that activity level, included reviews refill at 1 review per hour. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
Comment |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…k answerable Closes the four items left open when PR #2113 was opened, each with the smallest fix that actually resolves it rather than restating it. 1. The claim that no `allow` rule can reach a provider-backed script was asserted in review and never measured. It is now a test: 36 provider-backed scripts, derived from script names rather than hand-listed, each asserted unreachable through `allow` and covered by an explicit `ask`. Mutation-checked — injecting a broad `Bash(npm run check:*)` allow rule turns 4 of them red. 2. Hook registrations are pinned to invoke through an interpreter rather than a bare path, and to carry an explicit timeout. Bare-path registration is what made the `session-start.sh` mode bug reachable; the mutation check confirms both guards bite. 3. The PreCompact hook's known limit could not be closed by reading code — the installed CLI ships a compiled binary with no inspectable bundle, so whether the platform injects its stdout into model context is not determinable here. Instead of leaving that permanently unverified, the hook now appends one line per firing to a log under the git dir. After the next compaction the log distinguishes "hook never ran" from "hook ran but its output went nowhere", and both answers are actionable. The log lives outside the worktree so it can never be staged. 4. The `newtask` skill said "~40 worktrees". It was 48 on 2026-08-18 and reached 50 during one session, so the number now carries the reason it drifts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…BigSimmo/Database into claude/code-setup-review-a95519
BigSimmo
enabled auto-merge (squash)
August 18, 2026 11:57
BigSimmo
added a commit
that referenced
this pull request
Aug 18, 2026
…e claiming "0 commits ahead" when it isn't (#2117) * fix(claude): make the session-start hook runnable and give the agent config teeth `.claude/hooks/session-start.sh` was committed as mode 100644 while both its siblings were 100755, and it is the only hook registered by bare path rather than through `bash`. Its whole body is gated on CLAUDE_CODE_REMOTE=true, so the one environment it does any work in is a Linux web container — exactly where a non-executable checkout cannot run. It is also the script that provisions the Node 24 the engine floor requires, after npm ci EBADENGINE blocked PRs #1611, #1697, #1705 and #1740. The defect was invisible locally: the primary workstation is a Windows ReFS Dev Drive with core.fileMode=false, so git ignores filesystem permission bits and a local `chmod +x` is a silent no-op. Only `git update-index --chmod=+x` can fix it. Fixed three ways so it cannot recur: the index mode, a `bash "..."` registration that stops the mode being load-bearing, and a contract test. Also in this change: - .claude/settings.json gains a permissions block. AGENTS.md's provider confirmation boundary was prose-only; this encodes it as deny/ask rules, including deny on reading .env* (a staging key leaked on 2026-08-18) and on the Supabase MCP write tools. The repo already learned that prose does not hold here — see the comment in pr-handoff-stop.sh. - check-base-freshness.mjs now emits its stale-base warning on stdout as hook JSON. Every human-readable branch used console.error, and Claude Code injects only stdout into context, so the tripwire never reached the agent. The origin/main fetch also gains a 10s timeout so a hung remote cannot burn the whole SessionStart budget. - clean-worktree.mjs gains list-only `--merged` and `--squashed`. Nothing reclaimed merged worktrees, so 49 accumulated, ~19 GB of duplicated node_modules on a 50 GB Dev Drive. Ancestor detection alone finds 2 of 49 because this repo squash-merges; the patch-id test finds 9. A `confidence:` line distinguishes proven from inferred, because the two are not the same claim and one candidate had 2 of 21 files still differing. - Explicit hook timeouts, a PreCompact hook that asks for /issues capture while the context still exists, and a push format guard that only fires where the .githooks pre-push guard is not wired. Removal stays a separate opt-in throughout; `runWorktreeCleanup()` is byte identical, so verify:preflight is unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(tests): stop the session-start hook test failing on every Windows run `tests/session-start-hook.test.ts` asserted that the written env file contains `join(home, ".node24", …)`. On Windows `home` comes from mkdtempSync(tmpdir()) as `C:\Users\…\AppData\Local\Temp\session-start-home-XXXX`, while the hook runs under Git Bash and writes the POSIX view of the same directory, `/tmp/ session-start-home-XXXX`. The assertion therefore failed on every Windows run regardless of the diff under test. That is worse than a red test: it is a red test everyone learns to ignore. It fails inside `npm run test`, which is the last step of `verify:pr-local`, so the whole gate goes red locally for every change and the only way to use it is to decide which failures do not count. Compare the path tail instead. The unique mkdtemp basename still pins the assertion to this test's own HOME, so it loses no strength, and it now holds on both platforms. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(ledger): record the Claude Code environment review for PR #2113 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(claude): pin the permission boundary and make the PreCompact hook answerable Closes the four items left open when PR #2113 was opened, each with the smallest fix that actually resolves it rather than restating it. 1. The claim that no `allow` rule can reach a provider-backed script was asserted in review and never measured. It is now a test: 36 provider-backed scripts, derived from script names rather than hand-listed, each asserted unreachable through `allow` and covered by an explicit `ask`. Mutation-checked — injecting a broad `Bash(npm run check:*)` allow rule turns 4 of them red. 2. Hook registrations are pinned to invoke through an interpreter rather than a bare path, and to carry an explicit timeout. Bare-path registration is what made the `session-start.sh` mode bug reachable; the mutation check confirms both guards bite. 3. The PreCompact hook's known limit could not be closed by reading code — the installed CLI ships a compiled binary with no inspectable bundle, so whether the platform injects its stdout into model context is not determinable here. Instead of leaving that permanently unverified, the hook now appends one line per firing to a log under the git dir. After the next compaction the log distinguishes "hook never ran" from "hook ran but its output went nowhere", and both answers are actionable. The log lives outside the worktree so it can never be staged. 4. The `newtask` skill said "~40 worktrees". It was 48 on 2026-08-18 and reached 50 during one session, so the number now carries the reason it drifts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(issues): queue the five follow-ups this session could not close Each of these is blocked on something outside the repo, so capturing them is the smallest fix that resolves them — the alternative is that they die with the session context. - P1 rec: PR churn has exhausted both review bots' budgets, so PR #2113 landed with zero automated review and subsequent PRs will too. AGENTS.md already measured the CI half of this cost; this is the second bill and the worse one. Needs a decision on whether the bundling rule gets a gate. - P2 task: nine landed worktrees, ~4.5 GB on a 51%-full Dev Drive, deliberately not removed — re-verification immediately before deletion showed two held unmerged commits despite the scan reporting none minutes earlier, and a third had been switched branches mid-scan by a live session. - P3 task: confirm D:\.npm-cache is a registered Dev Drive trusted cache; fsutil needs elevation and the non-elevated registry fallback reads empty. - P3 task: read the PreCompact hook's own log after the next compaction to settle whether its output reaches model context. The log lives under the worktree's git dir, so check it before cleaning that worktree up. - P3 task: confirm on a real web session that session-start.sh now runs. The mode bug was proven; the failure it would cause on Linux was not, because no container was available. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(clean-worktree): stop the candidate listing claiming "0 commits ahead" when it isn't Every `--squashed` candidate printed "0 commits ahead" while being genuinely ahead of origin/main — by 11, 2, 1 commits on the real fleet. A squash-merged branch keeps its original commits forever, so it stays ahead permanently; what is zero is the count of UNLANDED commits, which is a different claim. The line was therefore stating something a reader could disprove with one `git rev-list`, on a tool whose entire job is to be trusted before a deletion. It now reports both numbers: "11 ahead of origin/main, 0 unlanded commits". Also corrects the comment above the ahead check, which described it as belt-and-braces against the merge test. That is true in ancestor mode, where it is the real gate. In squash mode it is not a second opinion at all — gitAheadUnlandedCount returns 0 for any branch the squash test just accepted, so the check is satisfied by construction and can only fire on a candidate that was already skipped. Keeping it is correct; describing it as independent evidence was not. The raw count is reporting only and never gates, so no candidate set changes. Verified against the live fleet: same 9 candidates before and after, worktree count unchanged, --remove not run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(ledger): record the follow-up review for PR #2117 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A review of this repo's Claude Code setup found one live defect and several places where a documented rule had no mechanism behind it. Each item below is independently revertible.
.claude/hooks/session-start.shwas not executable. It shipped as mode100644while both its siblings were100755, and it is the only hook registered by bare path rather than throughbash. Its whole body is gated onCLAUDE_CODE_REMOTE=true, so the one environment it does any work in is a Linux web container — precisely where a non-executable checkout cannot be run. It is also the script that provisions the Node 24 the engine floor requires, afternpm ciEBADENGINE blocked PRs Fix the results-band shelf Clear deleting the search query #1611, fix(navigation): wire the forms section anchors, drop the dead presentation set #1697, feat(document-viewer): PDF-first citation landing with condensed overview #1705 and feat(in-page-nav): extract the default in-page navigation template #1740. The defect was invisible locally because the primary workstation is a Windows ReFS Dev Drive withcore.fileMode=false, where git ignores filesystem permission bits and a localchmod +xis a silent no-op; onlygit update-index --chmod=+xcan fix it. Fixed three ways so it cannot recur: the index mode, abash "$CLAUDE_PROJECT_DIR/..."registration that stops the mode being load-bearing, and a contract test that fails on any hook which is not100755or which carries CR bytes..claude/settings.jsongains a permissions block. The provider confirmation boundary in AGENTS.md was prose only. It is now also encoded as 18 deny, 35 ask and 45 allow rules:askon the provider-backed npm scripts (eval:*,test:live,verify:release,check:supabase-project,sync:pr-branches,reindex, ingestion scripts) and on Railway mutations,denyon reading.env*and on the Supabase MCP write tools. The allow list covers the offline gates that are run constantly, to cut prompt noise. Allow patterns are deliberately written so that none of them can match a provider-backed script, so the result does not depend on allow-versus-ask precedence. This repo already learned that prose does not hold here — the comment inpr-handoff-stop.shsays so directly.scripts/check-base-freshness.mjswas reporting to a stream nobody reads. Every human-readable branch usedconsole.error, including the loud "N commits BEHIND origin/main" warning, and Claude Code injects only a SessionStart hook's stdout into model context. Confirmed on a live session whose injected context carried the sibling hook's stdout but no[base-freshness]line at all, though the hook ran and exited 0. It now emits the hook JSON envelope on stdout, and only when the message is worth the context it costs — a healthy base stays silent. Theorigin/mainfetch also gains a 10s timeout so an unreachable remote cannot consume the whole SessionStart budget.scripts/clean-worktree.mjsgains list-only--mergedand--squashed. Nothing reclaimed merged worktrees:identifyOrphanedWorktreesonly ever nominated worktrees whose directory was already missing or which git had marked prunable, so a merged branch whose directory still exists was never a candidate. 49 accumulated, roughly 19 GB of duplicatednode_moduleson a 50 GB Dev Drive that is 51% full. Ancestor detection alone finds 2 of 49 because this repo squash-merges; the whole-branch patch-id test finds 9. Aconfidence:line distinguishes proven from inferred, because they are not the same claim — one candidate had 2 of 21 files still differing fromorigin/main. Removal is a separate opt-in throughout,--removeis rejected without--merged, andrunWorktreeCleanup()is byte-identical, soverify:preflightis unaffected.Hook hygiene. Explicit
timeoutvalues on every hook (the default is 60s, whichsession-start.shcan exceed on a cold container mid-npm ci); aPreCompacthook that asks for/issues capturewhile the context that would be lost still exists, rather than after compaction as the existing SessionStart reminder does; and agit pushformat guard that fires only where the.githooks/pre-pushguard is not wired, which is the actual gap for cloud sessions.tests/session-start-hook.test.tsfailed on every Windows run. The assertion at:142expectedjoin(home, ".node24", ...), a Windows path, while the hook runs under Git Bash and writes the POSIX view of the same directory. Because that file is insidenpm run test, which is the last step ofverify:pr-local, the entire PR-ready gate went red locally for every change and the only way to use it was to decide which failures did not count. It now compares the path tail using the uniquemkdtempbasename, which holds on both platforms and loses no strength.Documentation. A new "Claude Code hook scripts" section in AGENTS.md records the mode rule, the
bash "..."registration rule, LF line endings, the fail-open contract, explicit timeouts, and the stdout-versus-stderr rule — so the traps bind Codex and Cursor too, not only Claude Code. Thenewtaskskill now creates worktrees under the project's own worktree root on the Dev Drive rather than../wt-<slug>, checks free space first, and states that worktrees underC:\Users\joshs\.codex\andC:\Users\joshs\.gemini\belong to other agents and must be left alone.Verification
npm run test—Test Files 668 passed | 2 skipped (670),Tests 7129 passed | 29 skipped (7158), real exit code 0. Captured to a file rather than piped, after an earlier run throughtailmasked a non-zero status.npm run verify:pr-local— completedcheck:runtime,check:installed-lock-parity,format:changed,sitemap:check,docs:check-index,docs:check-inventory,docs:check-scripts,docs:check-links,check:branch-review-ledger,check:outstanding-issues,check:ledger-write-discipline,check:github-actions,check:ci-scope,check:gitleaks-pinned,check:ci-triage,check:pr-policy,check:gate-manifest,check:skills,check:pr-mergeability,check:verification-plan,check:codex-autofix-workflow,lint,typecheck. Itsteststep failed on the two issues described above; both are resolved and the full suite above is the re-run.100755and CR-free blobs for all five hooks in.claude/hooks/.node scripts/clean-worktree.mjs --self-test— passed, covering merged-and-clean as a candidate and dirty, ahead, detached, locked, unmerged, current and main worktrees all skipped.node scripts/clean-worktree.mjs --mergedand--merged --squashed— both ran against the live 49-worktree fleet; worktree count verified unchanged before and after.--removewas never run.runWorktreeCleanup()compared byte-for-byte againstHEAD: 1969 bytes both sides, identical,git clean -fdxstill present.npx prettier --check .— all matched files use Prettier code style.npx eslintclean on both changed scripts.verify:ui(no UI, routing or styling surface in this diff),verify:releaseand every provider-backed gate (nothing here touches OpenAI, Supabase or hosted CI behaviour),check:production-readiness(no ingestion, answer-generation, privacy or production-config change).verify:pr-localabove, and CI enforces them again.Risk and rollout
Low risk and reversible. Nothing in this diff touches product code, the RAG surfaces, Supabase, ingestion or clinical output;
pr-policy's classifier returnsclinicalRisk: false,operationalRisk: false,ragRanking: false,ui: false.The two behavioural changes worth naming:
git pushformat guard can deny a push. It only fires wherecore.hooksPathis not wired to.githooks, so a normally installed checkout never reaches it, and theCLAUDE_ALLOW_UNFORMATTED_PUSH=1prefix overrides it for a single command.Both new hook scripts fail open: any parse problem exits 0 with no decision, leaving the tool call exactly as it was.
clean-worktree.mjscannot delete anything without an explicit--removealongside--merged, and never passes--forcetogit worktree remove. No worktree was removed while preparing this change: re-verifying the candidates immediately before deletion showed two of them had gained 2 unmerged commits since the scan minutes earlier, and a third had been switched to a different branch by another session, so the removal was deliberately not performed.Rollback is
git reverton either commit independently.🤖 Generated with Claude Code