Skip to content

fix(hooks): fail open when the inventory script is absent from the worktree - #1494

Merged
BigSimmo merged 5 commits into
mainfrom
claude/pre-commit-fail-open
Jul 30, 2026
Merged

fix(hooks): fail open when the inventory script is absent from the worktree#1494
BigSimmo merged 5 commits into
mainfrom
claude/pre-commit-fail-open

Conversation

@BigSimmo

@BigSimmoBigSimmo commented Jul 30, 2026

Copy link
Copy Markdown
Owner

Closes ledger row #151 (originally #143).

Summary

core.hooksPath is an absolute path to the primary checkout, so .githooks/pre-commit runs from every linked worktree — including ones whose branch predates the docs-sync tooling and therefore does not contain scripts/update-docs-inventory.mjs. Those commits aborted before the author could do anything about it:

[pre-commit] Synchronizing generated documentation...
Error: Cannot find module '<worktree>/scripts/update-docs-inventory.mjs'

This repository currently has ~45 worktrees, many on branches older than that script, so the failure is not hypothetical — it blocked a commit during the 2026-07-30 organisation session and had to be worked around per-command.

The inventory task now drops itself when its script is missing, removes docs/scripts-index.md from docs_to_check, and re-checks the all-tasks-empty early exit. Two details that are easy to get wrong:

  • The re-check is load-bearing. Without it, dropping the only selected task leaves docs_to_check empty, and the trailing git diff --name-only -- then matches every modified file in the tree — failing the commit for entirely unrelated reasons.
  • The grep -v needs || true. It exits 1 when it filters everything out, and set -eu is active, so without it the guard itself would abort the hook.

Verification

Tested in an isolated repository where the script genuinely does not exist. That distinction matters: simply deleting the file from a real worktree does not exercise this path, because the existing mixed-inputs guard sees the unstaged deletion and fails first — my initial attempt hit exactly that and proved nothing.

[pre-commit] scripts/update-docs-inventory.mjs is absent from this worktree - skipping inventory sync.
[main fad053e] probe
1 file changed, 1 insertion(+)

Commit succeeds, where the same input previously aborted with MODULE_NOT_FOUND.

With the script present the new block is a no-op — [ ! -f … ] is false and the re-check leaves all three task flags untouched — so the normal path is unchanged. sh -n passes. Prettier does not parse shell scripts, so format:check skips this file as it always has.

Note on the superseded branch

#151 described this fix as living on codex/docs-sync-automation-pr, a never-pushed local branch. That branch is now obsolete: its script and hook reached main through other PRs, and it sits 17 commits behind with a real conflict in docs/scripts-index.md, whose count sentence main's docs:update generator now owns. This PR applies the guard to main's committed version instead, so that branch can be abandoned rather than salvaged.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved documentation inventory synchronization during commits.
    • Prevents synchronization from being silently skipped when the required generator was removed or renamed.
    • Safely skips unavailable inventory updates when appropriate and avoids unnecessary documentation changes when no tasks remain.
  • Documentation

    • Added a review record documenting the pre-commit safety improvements.

…rktree
core.hooksPath is an absolute path to the primary checkout, so this hook runs
from every linked worktree - including ones whose branch predates the sync
tooling and so does not contain scripts/update-docs-inventory.mjs. Those commits
aborted with MODULE_NOT_FOUND before the author could do anything about it,
which is ledger row #151 (originally #143).
The inventory task now drops itself when its script is missing, removes
docs/scripts-index.md from docs_to_check, and re-checks the all-tasks-empty
early exit. That re-check matters: an empty docs_to_check makes the trailing
`git diff --name-only --` match every modified file in the tree and fail the
commit for unrelated reasons. The grep carries `|| true` because it exits 1 when
it filters everything out, which `set -e` would treat as a hook failure.
Verified in an isolated repository with the script genuinely absent - not merely
deleted from the working tree, which the mixed-inputs guard catches first: the
hook prints "skipping inventory sync" and the commit succeeds. With the script
present the new block is a no-op, so the normal path is unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@coderabbitai

coderabbitaiBot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The pre-commit hook now checks whether the inventory generator exists in HEAD before skipping synchronization, removes obsolete inventory checks when appropriate, and exits safely when no documentation tasks remain. Tests and the review ledger record this behavior.

Changes

Inventory sync safety

Layer / File(s)Summary
Pre-commit inventory guard
.githooks/pre-commit
Checks the generator in HEAD, conditionally disables inventory synchronization, removes its documentation output from checks, and exits when no tasks remain.
Regression assertion and review record
tests/docs-inventory.test.ts, docs/branch-review-ledger.md
Tests the HEAD existence check and records PR-1494 in the review ledger.

Estimated code review effort: 3 (Moderate) | ~15–30 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Title check✅ PassedThe title clearly summarizes the hook change to fail open when the inventory script is missing from a worktree.
Description check✅ PassedThe description covers summary and verification well; only non-critical template sections like risk/rollback are omitted.
Docstring Coverage✅ PassedNo functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Comment @coderabbitai help to get the list of available commands.

@supabase

supabaseBot commented Jul 30, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project sjrfecxgysukkwxsowpy because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@BigSimmo
BigSimmo enabled auto-merge (squash) July 30, 2026 19:50

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:62ed282dda

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread.githooks/pre-commit
BigSimmo added a commit that referenced this pull request Jul 30, 2026
#150 — the four at-risk worktrees were snapshotted onto their own already-merged
branches (748ef018f, 5dbd9f965, b7eae51a4, d949859c3), so the work survives a
worktree reclaim. All four are clean now. None is pushed or reviewed; the next
action is per-snapshot promote-or-reset.
#151 — the never-pushed branch is superseded rather than salvageable: its script
and hook reached main by other routes, so the fail-open guard was applied to
main's committed hook in PR #1494 instead.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
BigSimmo added a commit that referenced this pull request Jul 30, 2026
 edits
Another session reconciled the same three rows while this one snapshotted the
worktrees. Resolution keeps this side for #149 and #150 (theirs carried no
snapshot SHAs) and unions #151: their PR #1442 provenance plus the correction
that the archived #143 row implied the fail-open was durable when only the hook
and script had landed, kept alongside this side's redirect to PR #1494.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@BigSimmo
BigSimmo disabled auto-merge July 30, 2026 19:58
@BigSimmoBigSimmo added the skip-branch-sync Opt out of hosted pr-branch-sync / update-branch on this PR label Jul 30, 2026

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
tests/docs-inventory.test.ts (1)

54-55: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Test both hook branches instead of only matching source text.

These assertions can pass while the guard is unreachable or shell-invalid. Add an isolated temporary-repository test covering both a staged generator removal (must fail closed) and a genuinely absent generator in a legacy worktree (must succeed and omit docs/scripts-index.md).

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/docs-inventory.test.ts` around lines 54 - 55, Replace the
source-text-only assertions in the docs inventory hook test with an isolated
temporary-repository integration test that executes the hook. Cover both
branches: a staged removal of the generator must fail closed, while a genuinely
absent generator in a legacy worktree must succeed and omit
docs/scripts-index.md.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/docs-inventory.test.ts`:
- Around line 54-55: Replace the source-text-only assertions in the docs
inventory hook test with an isolated temporary-repository integration test that
executes the hook. Cover both branches: a staged removal of the generator must
fail closed, while a genuinely absent generator in a legacy worktree must
succeed and omit docs/scripts-index.md.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 1a8cde61-75e2-4f0c-bbe1-50e2344368e4

📥 Commits

Reviewing files that changed from the base of the PR and between 3f7073f and 7b96a09.

📒 Files selected for processing (3)
  • .githooks/pre-commit
  • docs/branch-review-ledger.md
  • tests/docs-inventory.test.ts

@BigSimmo
BigSimmo merged commit 387c3b6 into mainJul 30, 2026
30 of 31 checks passed
@BigSimmo
BigSimmo deleted the claude/pre-commit-fail-open branch July 30, 2026 20:24
BigSimmo added a commit that referenced this pull request Jul 30, 2026
…closed (#1508)
PR #1490 was closed unmerged, so none of its content reached main. Confirmed by
content rather than id: main's #149 and #150 are unrelated rows (installed-lock
parity and CodeRabbit rate limits) that happened to take those ids, so an
id-presence check reported them as landed when they were not.
- #151 corrects the earlier claim that CI is unreadable. The PAT lacks Checks:
read but has Actions: read, so workflow runs are queryable; the endpoint that
looked authoritative returns an empty result rather than an error, which is
what made it read as a hard wall.
- #152 re-lands the at-risk worktree inventory together with the four
preservation snapshots taken on 2026-07-31, which existed in no other record.
- #153 archives the pre-commit fail-open as resolved by PR #1494.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
BigSimmo added a commit that referenced this pull request Jul 31, 2026
…esson (#1511)
Two gaps: work was done and not recorded.
- docs/branch-review-ledger.md had no row for claude/root-dir-coverage-gate-v2
(#1458), claude/pre-commit-fail-open (#1494) or claude/ledger-relanding
(#1508). AGENTS.md requires one per reviewed branch; appended retrospectively
with the merged heads.
- #154 records why three separate "did it land" checks returned false answers
this session: title greps (reworded concurrently), id greps (ids reallocated
in parallel branches), and PR state fields (squash merges, and a MERGED PR
whose content had not reached the fetched ref). Resolve the blob and grep for
distinctive prose instead.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
BigSimmo added a commit that referenced this pull request Jul 31, 2026
* issues: capture the unreadable-CI token, at-risk worktree work, and the unpushed hook fix
Three findings from the 2026-07-30 organisation session that were recorded
nowhere durable:
- #149 the session GitHub PAT lacks Checks: Read, so no agent can confirm a PR
is green. The endpoint that does work returns an empty result rather than an
error, so it reads like an absence of checks rather than an absence of
permission.
- #150 four worktrees on already-merged branches hold uncommitted work that
exists in no branch and no PR, the largest being +395/-200 across 19 files
including CI config.
- #151 the pre-commit fail-open for #143 lives only on a never-pushed local
branch, which is also 17 behind main and conflicts on the file whose count
sentence main's new docs:update generator now owns.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs(ledger): record the session-followup capture review for PR #1490
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs(ledger): record #143/#151/#149 reconciliation for PR #1490
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
* docs(ledger): supersede PR #1490 reconciliation after remote sync
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
* issues: record the worktree snapshots and redirect #151 to PR #1494#150 — the four at-risk worktrees were snapshotted onto their own already-merged
branches (748ef018f, 5dbd9f965, b7eae51a4, d949859c3), so the work survives a
worktree reclaim. All four are clean now. None is pushed or reviewed; the next
action is per-snapshot promote-or-reset.
#151 — the never-pushed branch is superseded rather than salvageable: its script
and hook reached main by other routes, so the fail-open guard was applied to
main's committed hook in PR #1494 instead.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs: remove credential metadata and correct audit dates
* docs: consolidate session follow-up findings
* docs: record consolidated follow-up review
* issues: record that #101 hydration shipped
PR #1463 merged as dba7356, so #86's "Next X3 unit — rag-hydration.ts" is
now stale. The row records the extraction as shipped and keeps the corrected
boundary: hydration re-homed only two of prepareCoverageGateResults's five
rag.ts-only dependencies, so it did not unblock that function — exactly as the
Codex review on PR #1461 predicted.
This row was deliberately dropped from #1463 itself (commit 6290d02) after
docs/outstanding-issues.md conflicted on five consecutive main syncs. Recording
it separately here is the same pattern used for #1454 via #1461.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GGEBHp4Seoh1jK1vGTNtYS
* docs(ledger): record the landed X3 hydration review
Appended with npm run ledger:append (never hand-written), keyed to the squash
commit dba7356 so ledger:lookup can resolve it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GGEBHp4Seoh1jK1vGTNtYS
* docs: fix the #101 mislabel and key the ledger row to a resolvable ref
Both defects were raised by Codex on PR #1495 and both are real; verified
against the files before accepting.
1. #101 is NOT this extraction. docs/outstanding-issues.md:138 shows #101 is
"Canary-gated retrieval parallelisation candidates" (P3, rec) — a separate,
still-open recommendation gated on a live canary pair. Calling the hydration
extraction "#101" marked that unrelated work as shipped and could have caused
the live-evaluation work to be skipped. The label came from the original task
brief and was propagated without checking it against the ledger. Both the
#86 row and the X3 work-order entry now identify the change as the X3
hydration unit (PR #1463) instead. #101's own row is untouched and still open.
2. The ledger row did not resolve. `npm run ledger:lookup --
dba7356` returned NOT REVIEWED, because the
ref cell held only the slash-form branch token and that branch no longer
resolves locally, so the throttling record could not prevent a repeat review.
Appended a superseding record keyed to the landed SHA; the same lookup now
returns ALREADY REVIEWED. The original row is retained, per the ledger's
append-only rule.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GGEBHp4Seoh1jK1vGTNtYS
* docs: record consolidated PR reviews
* docs: record ingestion recovery review
* docs(visual): document the platform-scoped baseline layout and how to seed it
`playwright.visual.config.ts` records snapshots under
`__screenshots__/{platform}/`, so a baseline taken on Windows lands in `win32/`
and is never consulted by the `ubuntu-24.04` CI job, which reads `linux/`.
Nothing said so, and committing `win32/` images looks like protection while
providing none.
Records the constraint, names the CI artifact as the supported recorder for
`linux/` baselines, and notes that comparison stays advisory until the jobs come
off `continue-on-error`. Also creates the tracked directory `.gitignore` already
claims exists, which sets `ui_changed=true` (`scripts/ci-change-scope.mjs`) so
the visual job can run and produce that first artifact.
No baselines are added here — they cannot be produced on this platform.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs: correct visual baseline adoption steps
* docs: record visual baseline guidance review
* fix(ui): repair mockup accent token references
* docs: record token-reference repair review
* docs: archive advisory UI scoping task
* docs: record advisory UI closure review
* issues: archive #151 after #1494 and mark #143 fully resolved
PR #1494 landed the fail-open guard on main, so close the open salvage
row and update the #143 archive from PARTIAL to resolved across #1442
and #1494. Also carries the merge of origin/main that cleared the
GitHub DIRTY mergeability state.
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
* docs(ledger): record PR #1490 main-sync and #151 closeout
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
* docs(ledger): record #1496 id-collision renumber for PR #1490
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
* issues: record the withdrawn live-region finding as #151 so it is not re-filed
Archive-only row. There is no defect and no work to do — the row exists purely
as a guard rail against repeating a misreading that already happened once.
search-results-header-band.tsx sets aria-live={faulted ? "off" : "polite"} on
its count/status span, which reads like a silenced failure announcement. It is
not: the band mounts a separate fault panel with role="alert" carrying the
failure title, body and Retry, and the mute is deliberate so the two do not both
speak. The reasoning is in a comment directly above the attribute, and
tests/search-results-header-band.dom.test.tsx pins it with singular role queries
that throw on duplicates.
During session 2026-07-30 (PR #1481) this was filed as a real P2 defect on the
strength of the attribute alone, and the proposed fix — escalating the count span
to role="alert"/aria-live="assertive" — would have produced a duplicate
announcement and a red test, making it worse than no change. Codex caught it.
An earlier withdrawal row was then lost to the squash that merged #1481, which
is the row-deletion shape #148 now guards against.
Also records that the mockup's escalation is correct in the mockup and must not
be ported: search-refine-adaptive-mockups.tsx has no fault panel, so there the
count span is the only announcement channel.
#148 needed no work — the merge-base deletion check landed on main
independently, and its output now reports the base it compared against.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JdPa3mHCX5ZQZZvU5GHU3r
* docs(rag): record refuted lexical probe collapse (#98)
* issues: capture the residual id-allocation hazard as #151#133 is resolved: #1444 removed merge=union and #1479 excluded the ledger from
Prettier, which together fixed conflict frequency. Neither changes id
allocation, which is still read-modify-write against the next-id marker, so
concurrent branches still claim the same number.
Measured on PR #1451: one row was renumbered #135 -> #141 -> #145 -> #147 ->
#149 across four sync cycles. The sharper finding is that GitHub's Update-branch
button resolved one such collision into duplicate #141 rows with the marker left
below main's highest id — git reported success and only
check:outstanding-issues caught it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs(issues): attribute the mobile CLS breach — a 128px reserve round trip
#147 asked which elements shift. Driving Chromium against the same
offline production build with a PerformanceObserver on layout-shift
(Lighthouse mobile emulation, reading entry.sources[].node) gives one
dominant cause on all four breaching routes: the entire main content
region moves down 128px and straight back up 128px within 15-60ms. Both
moves score, so it is pure cost with zero net movement — 100% of
/documents/search's 0.220 and about 75% of /dsm's.
The shifting element is the max-sm:pt-[var(--phone-overlay-chrome-h)]
wrapper around <main>. A MutationObserver timeline on the root style
attribute pins the mechanism rather than inferring it: the property goes
CSS seed -> 200px -> 72px, and the 200px is written when the header
stack ALREADY measures 72px (t=1552ms reserve=200px stack=72, corrected
at t=1612ms). usePhoneOverlayChromeReserve reads stack.offsetHeight
while the stack is transiently tall, publishes a value that is stale by
the time it lands, and its ResizeObserver then corrects it.
The CSS seed at globals.css:375 is correct for the settled stack, which
corrects the mechanism recorded on the now-archived #130 — that framed
the defect as the seed under-reserving by 0-8px. Measured, the driver is
a 128px transient over-reserve written by the hook, not the seed. / is
the control: it never writes the property and is the one clean route.
Variance is stated rather than smoothed: /dsm measured 0.363 and 0.219
across two runs, and this harness has no network throttling so /forms
and /therapy-compass run high locally. Only /dsm, /documents/search and
/ reproduced the live dispatch exactly.
Also recorded: attaching a MutationObserver to document.documentElement
inside a Playwright addInitScript throws before the document element
exists, silently killing the CLS observer and reporting a uniform
CLS=0.000 — a false clean bill that voided one run of this harness.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF
* docs(ledger): record the #151 capture review for PR #1506
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs(review): clarify snapshot branch state
* docs(ledger): record PR #1490 main sync after snapshot wording
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
* docs: archive rendered style contract task
* docs: record style contract closure review
* docs: record synced style contract review
* docs: record post-121 style closure review
* docs: normalize style review ledger after sync
* docs: record post-1490 style closure review
* docs: record consolidated PR 1490 review
* docs: record replacement consolidation review
* docs: record reconciled consolidation review
* docs: record post-1511 consolidation review
* docs: normalize PR 1510 ledger after main sync
* docs: record PR 1510 post-sync review
* docs: correct false #98 canary evidence and NOTES triage
Remove the incorrect probe-collapse canary attribution from #98 and
point the unread --med-accent-soft note at #157 without breaking the
seven-token TOKENS_MISSING accounting.
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
* docs(ledger): record PR #1510 evidence-correction review
Supersede the prior approve-with-no-findings row after correcting the
false #98 canary attribution and NOTES triage drift.
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
* docs: keep concurrency note inside issue table
* docs: record post-1513 consolidation review
* docs: address CodeRabbit notes on PR #1510
Fix the computed-value-time wording in design-sync notes, give #33 a
unique recommended-queue order, and drop the duplicated #98 Done block.
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
* docs(ledger): record PR #1510 CodeRabbit fix review
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-branch-syncOpt out of hosted pr-branch-sync / update-branch on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@BigSimmo