fix(hooks): anchor the cwd-derived branch of trust-chain self-location (#1062) - #1100

Code Review: PR #1100

Commit: f642dcd05ae3e6bef9c3a2764abe455fe48c3ae8

Summary

Closes the residue #1061 left behind: #1033/#1061 anchored the git-derived fallback branch of the trust-chain self-location, but the cwd-derived branch runs first. Under a non-bash shell BASH_SOURCE is empty, dirname "" → ".", and the derived lib dir silently becomes the caller's cwd — so a cwd that directly contains a same-named sibling satisfies the -f guard, the anchor is never consulted, and the lib is sourced unanchored. The fix discards a cwd-derived path when BASH_SOURCE is empty, forcing resolution through the existing anchor.

I verified the premise, the fix, and the test independently rather than taking the PR body's word for any of it. The engineering here is unusually careful — in particular the decision to put the regression in a new zsh-guarded file instead of extending test_lib_self_location_anchor.sh is correct, and I confirmed why (below).

Checklist Results

• Architecture & Design: Pass
• Code Quality: Pass
• Testing: Pass
• Security: Pass
• Performance: Pass
• PR Description & Glossary: FAIL — no ## Glossary section (only blocking item)
• Summary Bullet Narrative: Pass (advisory)
• Technical Decisions (AgDR):N/A — narrows an existing guard, introduces no new decision
• Adopter Handbooks: N/A — none present in this fork
• Fallow Static Analysis: N/A — no JS/TS in diff

What I verified

1. All nine sites, identical guard — confirmed.

audit-history 1 · extract-pr 1 · fresh-fork 1 · onboarding-depth-mode 1
onboarding-glossary-seen 1 · premium-hook 1 · project-board 1 · tracker 2   = 9

All nine are byte-identical modulo the variable name, and — the failure mode worth actually checking in a repo whose recent backlog is "two places that must agree, that don't" — each guard clears the same variable its own self-location line assigned. I paired assign↔clear across all nine; no crossed wires, no site given a subtly different guard.

2. The test genuinely discriminates — confirmed two ways, not one.

The PR claims fail-before/pass-after via stripping the guard lines. I reproduced that, and then went further and ran the new test against the real pre-PR base (32a84a3):

Critically, the three over-strictness controls — v2 fork cwd (zsh), v1 fork cwd (zsh), impostor cwd (bash) — pass in all three states. So the guard is not merely "strict enough", it is demonstrably not over-firing.

I also confirmed the PR's justification for a new test file: test_lib_self_location_anchor.sh exits 0 even with every guard stripped. It genuinely cannot catch #1062 — its . /dev/stdin trick yields BASH_SOURCE[0]=/dev/stdin, which is non-empty and therefore never reproduces the degradation. The new file is necessary, not redundant. (This does deviate from #1062's literal AC wording, "test added to test_lib_self_location_anchor.sh" — the deviation is correct and the AC was wrong.)

3. The bash path is genuinely unchanged.

I measured BASH_SOURCE[0] inside a sourced file rather than reasoning about it:

The function-scope row matters because two of the nine sites (_tracker_load_config_lib, _premium_hook_dir) resolve inside a function — BASH_SOURCE[0] there is the defining file, so they are covered.

The /dev/stdin case you'd worry about does not mis-fire: it is non-empty, so the guard is skipped and behaviour is bit-for-bit what it was before. The only shape that changes is eval "$(cat lib)" at bash top level — and I grepped .claude/hooks, .claude/skills, .claude/agents: there is no eval-of-a-lib and no source <(...) anywhere. Hooks launch via bash -c invoking hook script files, which source libs from real absolute paths. Unreachable in practice.

4. zsh-absent skip is honest. With zsh removed from PATH the test prints SKIP: zsh not installed… and exits 0 — it does not print "All N checks passed". Same convention as the existing test_tracker_zsh_self_location.sh. It does mean a zsh-less CI box gets zero coverage of this fix, but that's visible in the log rather than silently green.

5. No collateral damage. Every suite touching a patched lib is green at HEAD: test_tracker_review_submit 27, test_forge_aware_extract_pr 48, test_depth_mode 25, test_extract_pr 24, test_lib_premium_hook 15, test_audit_history 15, test_fresh_fork 13, test_lib_project_board 10, plus test_lib_self_location_anchor 27 and test_tracker_zsh_self_location 9.

Issues Found

⛔ Blocking — missing Glossary section.

.claude/rules/pr-quality.md makes a ## Glossary table mandatory on every PR ("If missing → the Code Reviewer agent requests changes. No exceptions."). This PR body has Summary / Fix / Test / Scope, but no Glossary.

I checked whether this is a live rule or a dead letter before raising it, because blocking an otherwise-excellent fix on a description section would be a poor trade if the rule weren't actually enforced. It is live: PRs #1096, #1090, #1075 and #1061 — this PR's direct predecessor, same subsystem — all carry one. Applying it to an external contributor's PR and not to a maintainer's would be the wrong kind of inconsistency, so I'm raising it.

This blocks on the description only. No code change is required, and nothing in the diff needs to move. Suggested content — the terms this PR actually introduces:

## Glossary
| Term | Definition |
|------|------------|
| Self-location | A library resolving the directory it lives in, so it can source its sibling libs |
| `BASH_SOURCE` | Bash-only array holding the path of the currently-executing source file; **unset under zsh** |
| `dirname ""` degradation | With `BASH_SOURCE` empty, `dirname` of an empty string yields `.`, silently making the caller's cwd the "lib directory" |
| Anchor | A presence-only marker proving a directory is a genuine apexyard fork (`.apexyard-fork`, or the `onboarding.yaml` + `apexyard.projects.yaml` pair) |
| Trust chain | The `.claude/hooks/_lib-*.sh` libraries the merge/ticket gates depend on |

Re-push isn't needed — editing the PR description is enough, and I'll re-review on the same SHA.

Suggestions (non-blocking)

suggestion: _lib-read-config.sh:91 has the same shape and is not patched. It uses the identical ${BASH_SOURCE[0]:-} empty-default followed by a [ -f "$lib_dir/_lib-ops-root.sh" ] sibling guard, so it degrades to the caller's cwd under zsh exactly like the nine patched sites — and it is squarely in the trust chain.

I am not treating this as a defect in this PR, for two reasons: #1062's own scope says "nine trust-chain self-location sites" (inherited from #1061's set), so the PR delivers exactly what it was asked for; and the guard is not a drop-in there, because that site has no anchored fallback at all — its fallback is a bare, unanchored git rev-parse --show-toplevel (deliberately, to stay usable in non-apexyard clones). Discarding the cwd path there would need the anchor added first. Worth a follow-up issue rather than scope creep here.

For completeness, three further sites use a different pattern (${BASH_SOURCE[0]:-$0} or bare) and are correctly out of scope: _lib-git-hooks-path.sh:21, _lib-extract-push-ref.sh:62, _lib-protected-branches.sh:55.

nit: one narrow behaviour change worth knowing about. Forcing the anchored fallback means a genuine fork that is not a git repo, sourced under a non-bash shell, now fails to resolve where it previously succeeded via cwd. Measured:

This is the intended narrowing, not a bug — the anchor check is git-rooted by design since #1033, a non-git directory is indistinguishable from an impostor under that design, and the framework is a git fork by construction. It's also only reachable under a non-bash sourcing shell, which the repo's own advisory in _lib-read-config.sh explicitly tells operators not to do. Flagging it only so it isn't a surprise later.

nit: the 5-line comment block is repeated verbatim at all nine sites (~45 lines of the +164). Consistent with the surrounding #1033/#1025 comment style, so I'd leave it — noting it only because a future reader editing one copy will need to remember there are nine.

Verdict

CHANGES REQUESTED — solely for the missing ## Glossary section.

To be unambiguous about what that verdict does and does not mean: the fix and its test pass review completely. The premise is real, the guard is correct and uniformly applied at all nine sites, the bash path is provably untouched, and the test is a true fail-before/pass-after regression with working over-strictness controls — verified against the actual base commit, not just a mutation. That is a better-evidenced change than most that land here. Add the glossary table to the description and this is an approve on the same SHA.

🤖 Reviewed by Rex (Code Reviewer Agent)
📌 Reviewed commit: f642dcd05ae3e6bef9c3a2764abe455fe48c3ae8

Code Review: PR #1100 (re-review)

Commit: f642dcd05ae3e6bef9c3a2764abe455fe48c3ae8 — unchanged since my previous review (1 commit, nothing pushed).

Summary

The one blocking item from my previous review — the missing ## Glossary — is resolved, and resolved the right way: by satisfying .claude/rules/pr-quality.md rather than waiving it. The maintainer addition is clearly attributed, so the contributor isn't credited with prose they didn't write.

No executable content changed. Every verification from my previous review stands at this SHA and I have not re-run it: nine sites with identical guards clearing the correct variables; true fail-before against base 32a84a3; the bash path measured as a strict no-op (including . /dev/stdin, which does not mis-fire); and all collateral suites green.

Checklist Results

• Architecture & Design: Pass
• Code Quality: Pass
• Testing: Pass
• Security: Pass
• Performance: Pass
• PR Description & Glossary: Pass — was the sole blocker; now present
• Summary Bullet Narrative: Pass (advisory)
• Technical Decisions (AgDR): N/A
• Adopter Handbooks: N/A
• Fallow Static Analysis: N/A

Glossary accuracy check

I was asked to check the six terms rather than just confirm a heading exists. Four are accurate as written: self-location, BASH_SOURCE, impostor sibling, and discriminating test. The last is worth singling out — "passes even with every guard stripped" is exactly what I measured (the old suite exits 0 in that state), so it is a precise claim, not a paraphrase.

Two rows are imprecise. Neither blocks, and neither is wrong enough to hold the merge — but both were offered for correction, so:

1. anchored resolution omits the git-root requirement. As written it reads as "a directory carrying .apexyard-fork, or the onboarding.yaml + apexyard.projects.yaml pair". The actual check is stricter — it accepts only a git rev-parse --show-toplevel root that also carries one of those anchors and contains the expected sibling. That omission matters here specifically, because the git-root requirement is the entire cause of the narrow behaviour change already recorded on this PR: a genuine fork that isn't a git repo stops resolving. A reader who takes this row at face value would find that note inexplicable. Suggested:

Only accepting a directory that proves it is a genuine apexyard fork — a git repository root that also carries the .apexyard-fork marker (or the onboarding.yaml + apexyard.projects.yaml pair) and the expected sibling library — rather than trusting whatever a path happens to resolve to.

2. trust chain overstates reachability, and contradicts the PR's own scope note. The row says loading any of these from the wrong repository "undermines every gate that depends on them". The PR's own Scope section — and #1033 and #1062 before it — deliberately frame this as an accident surface, not an access-control boundary, and note severity is bounded because hooks launch via bash -c with absolute paths, so no merge, ticket, or marker gate actually reaches these degraded branches. Shipping a glossary that is more alarming than the change it documents invites someone to later cite this row as evidence of a severity the authors explicitly disclaimed. Suggested:

The libraries and hooks that enforce apexyard's SDLC gates. Loading one from the wrong repository is a misresolution rather than an exploit — the gates themselves invoke hooks by absolute path, so none reach this branch — but the class of defect is worth closing before something does depend on it.

Both are description-only edits on an unchanged SHA. Neither gates this approval — I'd rather these land than have them block a correct fix, and if you prefer the current wording that is a defensible editorial call, not an error I'd re-review over.

Issues Found

None. My previous blocking item is cleared.

Suggestions (non-blocking)

Both already handled and needing nothing further: _lib-read-config.sh:91 is filed as #1102 with the reasoning about why the guard isn't a drop-in there, and the non-git-fork-under-zsh behaviour change is recorded on this PR as intended narrowing.

Verdict

APPROVED

The fix is correct, uniformly applied at all nine sites, provably inert under bash, and backed by a test that genuinely discriminates against the real base commit. That was already true at my previous review; the description now meets the standard the rest of the repo is held to.

Credit where it's due to the contributor: measuring that the existing suite cannot detect this defect, and adding a separate zsh-guarded file rather than bolting a case onto a test that would silently never exercise it, is the part most easily skipped and the part that makes this fix durable.

🤖 Reviewed by Rex (Code Reviewer Agent)
📌 Reviewed commit: f642dcd05ae3e6bef9c3a2764abe455fe48c3ae8

Security Review: PR #1100

Commit: f642dcd05ae3e6bef9c3a2764abe455fe48c3ae8

Summary

Nine BASH_SOURCE-empty guards across eight trust-chain libs, forcing the cwd-derived
self-location branch through the same anchored git-root check #1033/#1061 applied only to
the git-derived fallback. Rex verified the mechanics by execution (guards paired to their
own assignments, new test discriminates against base, over-strictness controls pass, bash
path unchanged). I did not re-verify those; I tested the threat-model framing instead.

The change is a net security improvement and I am not blocking it. Two findings below
concern the accuracy of the accompanying risk statements, not the code.

Checklist Results

• Secrets & Credentials: Pass
• Injection Prevention: Pass
• Auth & Authorisation: Pass (no authz surface touched)
• Data Protection: Pass
• Gate & Trust-Chain Integrity: Pass — no gate weakened, fail-open narrowed not widened

Security Issues Found

MEDIUM — the "no gate reaches these branches" justification is factually wrong, even
though the conclusion (not an access-control boundary) is right.

The PR argues the branches are unreachable by gates because hooks launch via bash -c
with absolute paths. The first half holds — settings.json-wired hooks do run under bash,
so BASH_SOURCE is populated and the guard is a no-op for them. But that is not the only
consumer. The Claude Code Bash tool on this host runs zsh, and I confirmed
${BASH_SOURCE[0]:-} is empty there. The approval and review skills — /approve-merge,
/approve-design, /approve-architecture, and this agent's own posting flow — explicitly
instruct . "$LIB_HOME/.claude/hooks/_lib-tracker.sh". Those source the patched libs, under
zsh, and they are exactly the marker-writing and review-posting paths. So the vulnerable
branch is reachable from marker-adjacent code.

What actually keeps this below an access-control boundary is a different precondition the
PR does not state: the degraded path only wins when the caller's cwd directly contains a
same-named sibling (dirname "" -> "."). Skills source with an absolute $LIB_HOME path
while cwd is a repo root or workspace clone, not a .claude/hooks/ directory — so the -f
guard fails and the anchored fallback runs anyway. Combined with the attacker already
needing write access to that cwd, the practical severity stays at misresolution.

Verdict on the framing: the conclusion is honest, the stated reason is not. Please
correct the scope note to rest on the cwd-contains-sibling precondition rather than on
"hooks are bash so nothing reaches this". Trust-chain rationale gets cited later as
authoritative, and a load-bearing claim that is wrong for the skill path will mislead the
next reader who is deciding whether a new zsh-sourced caller is safe.

MEDIUM — the residual-risk accounting undercounts unpatched sites 4:1.

The PR and #1102 present _lib-read-config.sh:91 as the one remaining site. There are
four. _lib-extract-push-ref.sh:62, _lib-git-hooks-path.sh:21, and
_lib-protected-branches.sh:55 all self-locate the same way and are not mentioned anywhere.
They use the ${BASH_SOURCE[0]} / ${BASH_SOURCE[0]:-$0} spelling rather than
${BASH_SOURCE[0]:-}, which is why a grep for the patched idiom misses them — but I
confirmed under zsh that $0 inside a sourced file is the sourcing path as written, so a
relative-path source degrades these to cwd identically. Worse, unlike the nine patched
sites, these three have no anchored fallback at all — just if [ -f "$dir/sibling" ]
then source. _lib-protected-branches.sh is the one I would prioritise: it supplies the
protected-branch regex behind block-main-push.sh.

This is pre-existing and correctly out of scope for this PR. I am flagging the
accounting, not asking for the fix here: a scope note saying "one site remains" will
cause someone to close #1102 and believe the class is finished. Please widen #1102 (or file
a sibling) to name all four, and note that the three :-$0 variants need a different fix
shape than the guard used here.

LOW — unexplained mode change. _lib-fresh-fork.sh goes 100755 -> 100644. Harmless
for a sourced lib, and arguably more correct, but it is unrelated to #1062 and unmentioned
in the PR body. Either call it out or drop it.

On the other questions asked

Q2, hostile vs accidental cwd — stated honestly. The anchors are presence-only
(.apexyard-fork, or the onboarding.yaml + apexyard.projects.yaml pair) plus a git-root
requirement, and anyone who can write the cwd can fabricate all of them. The PR says so in
those words. Nobody should read this fix as authentication, and the PR does not invite that
reading. Correct as written.

Q3, narrowing — fails closed, but noisily. A genuine fork that is not a git repo,
sourced under zsh, now fails to resolve. That is a real deployment shape (bare tarball
install, vendored copy, some CI sandboxes). The failure mode is the lib not loading its
sibling, which surfaces as a missing-function error rather than a silent wrong-repo load —
fail-closed, which is the right direction. I would still rather the anchored fallback
emitted one line to stderr naming the reason, because "command not found: config_get_or"
is a confusing signal for an adopter whose only sin was not running git init. Non-blocking.

Q4, the read-config split — correct. That lib's fallback is a deliberately unanchored
git rev-parse, so dropping this guard in would route a discarded cwd into an unanchored
path and change behaviour rather than tighten it. Deferring is the right call; see the
accounting finding above for the only thing I would change about how it was deferred.

Q5, anything made worse — nothing found. The guard is a strict narrowing: it only ever
empties a variable that was about to be used unanchored, and only when BASH_SOURCE is
empty. It cannot cause an unanchored path to be accepted that previously was not. One design
note, not a defect: ${BASH_SOURCE[0]:-$0} would have preserved correct resolution under
zsh for absolute-path sourcing instead of forcing the fallback. Discarding is more
conservative and consistent with the anchoring goal, so I would not change it — but it is
worth recording that the choice was conservatism, not necessity.

Verdict

APPROVED

Both MEDIUM findings are documentation accuracy, not code. The diff makes the trust chain
strictly harder to misresolve and weakens no gate. Please correct the scope note's
justification and widen the residual-site accounting before or shortly after merge.

🛡️ Reviewed by Hakim (Security Auditor)
📌 Reviewed commit: f642dcd05ae3e6bef9c3a2764abe455fe48c3ae8

Code Review: PR #1100

Commit: f642dcd05ae3e6bef9c3a2764abe455fe48c3ae8

Summary

Closes the residue #1061 left: #1033/#1061 anchored the git-derived fallback branch of the trust-chain self-location, but the cwd-derived branch runs first. Under a non-bash shell BASH_SOURCE is empty, dirname "" → ".", and the derived lib dir silently becomes the caller's cwd — so a cwd directly containing a same-named sibling satisfies the -f guard, the anchor is never consulted, and the lib loads unanchored. The fix discards a cwd-derived path when BASH_SOURCE is empty, forcing resolution through the existing anchor. A genuine BASH_SOURCE path is kept as-is.

This is a Heavy (trust-chain) change, so I verified it independently rather than taking the body — or the prior review — on faith. The prior atlas-apex Rex/Hakim reviews are thorough and I corroborate them; my own checks below reach the same verdict.

Checklist Results

• Architecture & Design: Pass
• Code Quality: Pass
• Testing: Pass
• Security: Pass — narrows a fail-open, weakens no gate
• Performance: Pass
• PR Description & Glossary: Pass — ## Glossary present (the earlier "missing Glossary" note is stale)
• Summary Bullet Narrative: Pass (advisory)
• Technical Decisions (AgDR): N/A — narrows an existing guard, no new decision
• Adopter Handbooks: N/A — none in this fork
• Fallow Static Analysis: N/A — no JS/TS in diff

What I verified

1. All nine guards clear the variable their own self-location line assigned — confirmed from the diff, paired site-by-site. _AUDIT_LIB_DIR, _lib_extract_pr_dir, _FRESH_FORK_LIB_DIR, _DEPTH_MODE_LIB_DIR, _GLOSSARY_SEEN_LIB_DIR, _PREMIUM_HOOK_DIR_CACHE, _lib_board_dir, and hook_dir (x2 in _lib-tracker.sh) = 9. Every guard reads ${BASH_SOURCE[0]:-} and clears the same variable the immediately-preceding assignment set. No crossed wires; the two function-scope sites (_tracker_load_config_lib, _premium_hook_dir) get the same treatment as the top-level ones and are covered because BASH_SOURCE[0] inside a sourced function is the defining file.

2. The forced fallback really is the anchored check. Read _lib-tracker.sh and _lib-fresh-fork.sh on the base: when the guard empties the dir, the if body runs git rev-parse --show-toplevel, then requires .apexyard-fork (or the onboarding.yaml + apexyard.projects.yaml pair) and the expected sibling under $root/.claude/hooks/. An impostor non-fork cwd yields no valid anchor → the dir stays empty → the sibling is never sourced (fail-closed). That is exactly #1062's acceptance criterion.

3. Nine is the complete set for this idiom. Independent grep of .claude/hooks/_lib-*.sh: the only other site with the same ${BASH_SOURCE[0]:-} + anchored-fallback shape is _lib-read-config.sh:91, and it is correctly out of scope — its fallback is a deliberately unanchored git rev-parse (via resolve_ops_root), so this guard is not a drop-in there; it needs the anchor added first. Filed as #1102. The ${BASH_SOURCE[0]:-$0} / bare variants (_lib-extract-push-ref.sh, _lib-git-hooks-path.sh, _lib-protected-branches.sh) are a different pattern and correctly untouched.

4. The bash path is provably unchanged. Under bash, BASH_SOURCE[0] is the real file path at both top level and inside a sourced-file function — non-empty, so the guard is a strict no-op and no filesystem check is added to the genuine path. . /dev/stdin yields /dev/stdin (non-empty) → also unaffected. Only empty-BASH_SOURCE (zsh) reroutes, which is the whole point.

5. The test genuinely discriminates and does not over-fire. The two impostor-zsh cases flip to IMPOSTOR_CONFIG=1 with the guards stripped and pass with them; the three over-strictness controls (v2 fork cwd, v1 fork cwd, impostor cwd under bash) pass in both states, so a genuine fork of either layout still resolves under zsh. The regression correctly lives in a new zsh-guarded file because the existing test_lib_self_location_anchor.sh cannot reproduce the empty-BASH_SOURCE degradation (. /dev/stdin gives a non-empty BASH_SOURCE[0]), and it skips cleanly when zsh is absent.

Issues Found

None blocking.

Suggestions (non-blocking)

• nit: _lib-fresh-fork.sh mode change 100755 to 100644 is unrelated to #1062 and unmentioned in the body. It is safe: this is a sourced library (. file), never executed directly, so the executable bit is behaviourally irrelevant (arguably more correct without it). Explicitly de-scoped by the maintainer; I agree it does not block.
• suggestion (already tracked): the residual same-idiom site _lib-read-config.sh:91 (#1102), and Hakim's note to widen the residual-site accounting to name the three :-$0 variants and correct the scope note's "no gate reaches these branches" framing to rest on the cwd-contains-sibling precondition. All documentation-only; none gate this PR.

Verdict

APPROVED

The premise is real, the guard is correct and uniformly applied at all nine sites (each clearing its own variable), the forced fallback is the genuine anchored git-root check, the bash path is provably inert, and the test is a true fail-before/pass-after regression with working over-strictness controls. Better-evidenced than most changes that land here. The exec-bit nit is safely de-scoped.

🤖 Reviewed by Rex (Code Reviewer Agent)
📌 Reviewed commit: f642dcd05ae3e6bef9c3a2764abe455fe48c3ae8

Code Review: PR #1100 — re-review

Commit: 1f1b9436ed44cdf11cfe21021b3229c97f27b93c
Previous verdict: CHANGES REQUESTED on 90cfe61

Summary

All three required changes are implemented, and every suggestion from the last round has been addressed in code. I verified each item against the diff and, where checkable offline, by execution: tsc --noEmit is clean, the full offline suite is 1429 passed / 123 skipped / 0 failed, and the 51 tests in config.test.ts + redaction.test.ts all pass locally.

The blocker is not in the review items — it's that the Booking DB boundary CI job is red on this commit, and it fails before any test runs. The provisioning step dies on a non-portable mktemp call that was introduced by the very fix which moved provisioning secrets into a temp vars file. Two consequences: CI is red (Gate 3), and every live-PostgreSQL assertion in this PR — including the boundary test that is now the sole proof for required change 3 — is unverified on this commit.

The fix is one line in each of three scripts. Everything else here is ready.

Checklist Results

• ✅ Architecture & Design: Pass
• ✅ Code Quality: Pass
• ✅ Testing: Fail — the live-PostgreSQL suites did not execute in CI (job aborted at provisioning)
• ✅ Security: Pass
• ✅ Performance: Pass
• ✅ PR Description & Glossary: Pass
• ⚠ Summary Bullet Narrative: Pass (advisory)
• ✅ Technical Decisions (AgDR): Pass — AgDR-0058 + AgDR-0038 linked; no new decision introduced (see note below on the boundary retraction)
• ✅ Adopter Handbooks: Pass with advisory findings
• ⚠ Fallow Static Analysis (JS/TS): N/A — fallow CLI not on PATH

Issues Found

⛔ BLOCKING — mktemp -t <bare-prefix> is not portable; provisioning fails on Linux, which is both CI and the real operator path

The Booking DB boundary job failed at step 7, "Provision and migrate the booking boundary":

mktemp: too few X's in template 'booking-provision-vars'
##[error]Process completed with exit code 1.

GNU coreutils requires -t's template to end in at least three X characters. BSD/macOS mktemp accepts a bare prefix, which is why the local rehearsal passed — I confirmed both behaviours: on this macOS box mktemp -t booking-provision-vars happily returns /var/folders/.../booking-provision-vars.tgb12DyIlg, while the CI log above is the GNU rejection.

Three call sites are affected, all broken on Linux:

• scripts/booking-db/provision.sh:34 — new in this commit, introduced by the secrets-via-vars-file fix. This is what reddens CI.
• scripts/booking-db/restore.sh:32 — pre-existing on the branch.
• scripts/booking-db/backup.sh:37 — pre-existing on the branch.

This is not merely a CI-environment quirk. The runbook targets Lightsail, so as written none of provisioning, backup, or restore can run on the target platform. It also means the PR's "backup/restore rehearsal executed locally" evidence only demonstrates macOS.

Suggested fix — portable, and avoids -t, which GNU deprecates:

vars_file="$(mktemp "${TMPDIR:-/tmp}/booking-provision-vars.XXXXXX")"

Knock-on effect — the live verification story is currently unexecuted. Because step 7 aborted, steps 8 ("Run booking boundary and persistence suites") and 9 ("Guard schema/migration drift") were both skipped. So none of these ran on 1f1b9436:

• the catalogue object-level denial test that is now the entire proof for required change 3;
• the new BOOKING_STATES ↔ booking_intents_state_check sync test;
• the reworked migrate.sh single-transaction apply + ledger insert;
• the new Prisma drift guard.

I reviewed all four as code and they look correct (details below), but please re-run CI green after the mktemp fix so they're actually exercised. Once the job is green I'll re-review promptly — I don't expect further findings.

Required changes — verification

1. TLS guard fail-open → positive allowlist: ✅ verified fixed

src/lib/booking/db.ts:56-66 now requires a match on /[?&]sslmode=(verify-ca|verify-full)\b/ when APP_ENV is production or staging, and the comment states the intent (absent sslmode means libpq's prefer; require encrypts without verifying). The four tests in config.test.ts:58-97 cover exactly the cases that mattered — disable rejected, absent sslmode rejected, require rejected in staging, verify-ca/verify-full accepted — and all pass locally. This closes the fail-open hole properly.

2. Pool-exhaustion classifier: ✅ verified fixed

src/lib/booking/errors.ts:31-34 matches pg-pool's literal acquisition-timeout message "timeout exceeded when trying to connect", plus 53300 (too_many_connections) and mid-connect termination. The runWithBookingDb chokepoint at db.ts:94-102 routes failures through toBookingDbError, so callers get the typed retryable error rather than a raw pg error. The table-driven tests at config.test.ts:200-219 are the right shape: they include negative cases (statement timeout, 23505 constraint violation, plain error, non-error) so the classifier can't quietly widen into a catch-all.

One forward-looking note, not a change request: getBookingDb() is still exported and therefore bypassable. That's fine while #1089's repositories are the intended consumers — just worth holding the discipline that all booking work goes through runWithBookingDb when those land.

3. REVOKE CONNECT no-op retracted: ✅ verified fixed

provision.sql:68-73 replaces the no-op with an accurate statement of the real boundary — credential separation plus object privileges — and is explicit that CONNECT defaults to PUBLIC and that tightening it is a catalogue-side change with its own re-granting problem. That's the honest framing. The plumbing is consistently removed (provision.sh, test-env.sh, and the runbook all drop BOOKING_CATALOGUE_DATABASES).

The replacement proof at boundary.test.ts:89-107 is the right test: it creates catalogue_probe in a co-located catalogue database as admin, then connects as the booking runtime role and asserts SELECT fails with permission denied. That asserts object-level denial rather than connection-level denial, which is precisely the boundary as now documented.

I also checked this against AgDR-0058 for divergence, since retracting a control could contradict a recorded decision. It doesn't: the AgDR asks to "prove the runtime role has no catalogue, database-creation, schema-creation, or migration privilege" and that catalogue consumers "receive no booking credential" — it never promised REVOKE CONNECT specifically. No AgDR amendment needed.

Suggestions — verification

All addressed. Verified, with a few small residual notes.

• migrate.sh atomicity — verified. PSQL carries --single-transaction (line 23) and the DDL plus ledger INSERT now go through one psql invocation, so a crash can't leave a migration applied-but-unlogged. I confirmed 0001_init contains no CONCURRENTLY / BEGIN / COMMIT, so it's transaction-safe.
• restore.sh URL rebuild — verified. I ran the parameter expansion against the two cases that were broken: .../admin_db?sslmode=verify-full now yields .../RESTORED?sslmode=verify-full (query preserved), and a non-postgres admin database name no longer corrupts the target. Dead owner variable is gone.
• provision.sh secrets — intent achieved. Passwords no longer appear in ps output, and the server-log caveat with the rotate-after-run instruction is in both the script comment and the runbook. (This is the change that carries the blocking mktemp bug.)
• PAN regex — verified. The uniform-separator form plus the Amex 4-6-5 alternation is right, and the regression test at redaction.test.ts:124-136 proves "2026-10-01 2026-11-06" no longer nominates. The three new positive cases (space-grouped, dash-grouped, Amex) pass. The accepted trade-off — mixed-separator PANs like 4111-1111 1111-1111 no longer nominate — is documented in the comment, which is the right call given the fail-closed false-positive was the live hazard.
• evidence.ts canonicalization — verified. Recursive key sort before HMAC, with the order-independence test covering nested objects inside arrays.
• encryption.ts AAD — verified. The test proves a ciphertext copied to a different row context fails authentication, and that decrypting without the AAD also fails.
• resetBookingDbForTests — verified, pool.end() at db.ts:106-111.
• test-env.sh — verified, dead MIGRATION_URL line removed and base() now strips query strings.
• BOOKING_STATES ↔ CHECK constraint test — present at boundary.test.ts:173-183, reading pg_get_constraintdef and comparing sorted vocabularies. Correct as written; did not execute in CI.
• schema.prisma money columns + drift guard — verified. All 11 Decimal fields carry @db.Decimal with no unannotated Decimal left, matching the 11 unconstrained NUMERIC columns in 0001_init; prisma validate passes. (The "12" count includes the explanatory comment line — no missing column.) I confirmed both --from-config-datasource and --exit-code exist in the installed Prisma 7.9.1, and that test-env.sh exports BOOKING_TEST_MIGRATION_URL, which the CI step propagates via GITHUB_ENV — so the wiring is right; the step just never ran.

Non-blocking findings

• migrate.sh ledger now always records durationSeconds = 0. The value is computed while building the pipe input, not after psql finishes, so it captures the time to cat the file rather than the time to apply the migration. I confirmed this with a slow-consumer stand-in: the recorded value was 0 while the pipeline genuinely took 3s. 0001_init is 10 KB, well under the 64 KB pipe buffer, so cat always returns immediately. The echo "applied $name (${duration}s)" line still prints the real duration, so the log and the ledger now disagree. The rename to durationSeconds fixed the unit mismatch, but the stored value is no longer a duration. Simplest fix that keeps atomicity: have psql compute it server-side, e.g. record clock_timestamp() into a temp table at the top of the piped input and insert the difference, or accept the ledger row without a duration and keep timing in the log only.
• Ledger column rename isn't forward-compatible with an already-provisioned database. CREATE TABLE IF NOT EXISTS won't add durationSeconds to a ledger created by the previous commit, so the INSERT fails with column "durationSeconds" does not exist. No environment is provisioned yet and CI builds fresh, so this is only a papercut for a developer with a stale local booking database — but a one-line ALTER TABLE ... ADD COLUMN IF NOT EXISTS would remove the trap.
• restore.sh validates the URL after CREATE DATABASE. The path guard at line 46 runs after the database is created at line 29, so tripping it leaves an orphan empty database, which the "already exists" check at line 24 then refuses on retry — an awkward spot for a script that runs under incident pressure. Moving the URL rebuild and validation above CREATE DATABASE makes the failure clean. Relatedly, the guard doesn't catch a path-less admin URL: postgresql://u:p@h:5432 slips through and produces postgresql://RESTORED. It fails loudly at pg_restore rather than corrupting anything, so this is minor, but stripping the scheme before the check would make the intended error message actually fire.
• The ''-doubling escape for psql \set doesn't escape a quote — it drops it. In provision.sh, ${BOOKING_APP_PASSWORD//\'/\'\'} produces \set app_password 'ab''cd', and because psql's \set concatenates multiple value arguments, that silently yields abcd rather than ab'cd. The role would get a different password than the one recorded in the secret store — a silent mismatch rather than a loud failure. Unreachable in practice since the runbook generates passwords with openssl rand -base64 32 (the base64 alphabet has no quote), but psql's escape inside single quotes is \' if you want to close it off.
• AAD is optional on both encryptField and decryptField. That's the right shape for landing it without callers, but it makes an encrypt-with / decrypt-without mismatch a runtime throw rather than a compile error. Worth making the AAD non-optional at the repository boundary in #1089 so no call site can forget it.
• PR body accuracy. The Testing section states the full repo gates are green and the restore rehearsal passed; both are true only on macOS, and the CI job is currently red. Worth refreshing once CI is green so the body matches the evidence.

Handbook Findings

⚠ Commit Message Quality — handbooks/general/commit-message-quality.md

• Commit 1f1b9436 has subject fix: address Rex review on booking DB boundary, missing the (#894) ticket scope the handbook's subject format requires (type(#TICKET): description). The body is substantial, explains the why per item, and carries Refs #894 plus Co-authored-by, so this is purely the subject prefix — and the repo squash-merges with the PR title carrying the ticket. Advisory only; the sibling commit ad5f0726 gets it right.

Migration Safety — handbooks/architecture/migration-safety.md (blocking handbook): no violation. 0001_init is the first migration on a new isolated database and is purely additive, and the schema.prisma change is a native-type annotation matching the DDL that already exists rather than a narrowing — confirmed by prisma validate and by the column-by-column match against 0001_init. No DROP / RENAME / ALTER COLUMN anywhere in the diff.

Clean Architecture Layering — handbooks/architecture/clean-architecture-layers.md: no violation; this codebase doesn't use the domain/ / application/ / infrastructure/ tree the handbook governs.

TypeScript Strict Mode — handbooks/language/typescript/strict-mode.md: no violation; no any in the new booking modules (narrowing goes through unknown casts), and tsc --noEmit is clean.

Verdict

CHANGES REQUESTED

One blocking item: fix the non-portable mktemp in provision.sh, restore.sh, and backup.sh, and get the Booking DB boundary job green so the live-PostgreSQL suites and the drift guard actually run on the reviewed commit. All three required changes from the previous round are correctly implemented and every suggestion is addressed — the non-blocking findings above are yours to take or defer.

🤖 Reviewed by Rex (Code Reviewer Agent)
📌 Reviewed commit: 1f1b9436ed44cdf11cfe21021b3229c97f27b93c