audit fixes (P7): make the maintenance scripts refuse by default instead of mutating by default - #2620
Merged
Conversation
…ult offline run (M15) Defect: vitest.config.mts collects the destructive `caring-contacts-db` project whenever CARING_CONTACTS_DATABASE_URL is non-empty, and offlineTestEnvironment never scrubbed that variable, so a plain `npm run test` in a shell that had exported it for the documented `caring-contacts:db:test` workflow dropped and recreated the caring_contacts schema on whatever host the URL named, with no loopback check and no receipt key. Trigger: export CARING_CONTACTS_DATABASE_URL, then run `npm run test`, verify:cheap or verify:pr-local in the same shell; or point the variable at a shared Postgres. Fix: offlineTestEnvironment blanks the variable unless CARING_CONTACTS_DB_TESTS=1 is set, and even then refuses a non-loopback host. caring-contacts/run-db-tests.mjs is the only thing that sets the opt-in marker, and it refuses non-loopback hosts before spawning. Both variables now key gate receipts so a pass recorded without the DB project is not served as a pass with it. Test: tests/run-vitest-caring-contacts.test.ts. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FSPY4VSqg7WVukCvmHQP9t
…te and --all-owners (M18) Defect: `npm run enrich:documents` with no arguments treated an empty owner environment as --all-owners, wrote document and image rows through the service-role client on whichever Supabase project .env.local named, and called OpenAI — with no --write, no dry-run and no project check, unlike every sibling backfill script. Trigger: a session with a populated .env.local runs the command to see what it does. Fix: argument parsing moves to scripts/lib/enrich-documents-args.ts. --all-owners is now opt-in only (no owner is a refusal), dry-run is the default and lists what would change, --write is required for any write or OpenAI call, and checkSupabaseProjectConfig runs before the admin client is created. docs/scripts-index.md inventory regenerated by the pre-commit hook. Test: tests/enrich-documents-guards.test.ts. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FSPY4VSqg7WVukCvmHQP9t
…ail (M28) Defect: since #M6JNR8 the queue's Outcome cell is editable through `npm run issues:queue -- '#id' --outcome "..."`, but issues-report.mjs replaced that cell with the cited row's Detail for every single-ID queue row, so an operator's stop-condition or gate correction never reached /issues; its comment still claimed no inbox request type could reach the cell. Trigger: any `issues:queue --outcome` correction on a queue row citing one ID, then /issues. Fix: each derived queue row keeps Detail as its prose and carries the queue cell as `gate` whenever the two differ; the renderer prints it as "gate/stop (queue cell)". The A1 blocker projection carries the same field. Rendering moved into an exported renderIssuesReport so the output is testable. The SessionStart hook's own awk reader (.claude/hooks/issues-surface.sh) is outside this package's files and still shows Detail only. Test: tests/issues-report.test.ts "surfaces an operator's issues:queue --outcome correction". Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FSPY4VSqg7WVukCvmHQP9t
…e not shown as open (L59) Defect: issues-report.mjs read origin/main's ledger only and never docs/outstanding-issues-inbox, so rows with a pending `done` request (14 effective today) were still presented as open work and queued update/add/queue corrections were invisible. Trigger: any `npm run issues:done|update|add|queue` that has not yet been reconciled, then /issues or `npm run issues:report`. Fix: loadRevalidatedLedger now reads the pending inbox at origin/main (one `git cat-file --batch`) plus worktree-only requests, plans the batch the way issues:reconcile will, and applies it to the report copy of the ledger. The source carries `pending` counts and the closing ids, rendered as one line with the reconcile command; if the batch cannot be applied the raw ledger is used with a warning. Nothing is written — the ledger and inbox are untouched. The SessionStart hook's awk reader (.claude/hooks/issues-surface.sh) is outside this package's files and unchanged. Test: tests/issues-report.test.ts "applies the pending inbox" and "falls back to the raw ledger with a warning". Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FSPY4VSqg7WVukCvmHQP9t
…ment (L26) Defect: docs/archive/staging-tenancy-evidence-29795051547/staging-tenancy-evidence.json is a tracked repository file, yet .gitignore matched it with the CI-artifact pattern `docs/archive/staging-tenancy-evidence-*` and `npm run clean:artifacts` targets the same glob, so a clean deleted a tracked document and re-adding it needed `git add -f`. Trigger: `npm run clean:artifacts`, or any tool that honours .gitignore for that path. Fix: the ignore pattern keeps covering downloaded artifacts, with an explicit negation for the one committed evidence directory. The `clean:artifacts` glob itself lives in package.json, which is outside this package's files, so that half is left for its owner (narrow the glob so it cannot expand to the tracked directory). Test: tests/clean-artifacts-tracked-paths.test.ts. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FSPY4VSqg7WVukCvmHQP9t
… ignore it (L63) Defect: supabase/.branches/_current_branch is per-machine Supabase CLI state (the CLI's own scaffold ignores `.branches` and `.temp`), but this repository tracked it and ignored only supabase/.temp/, so any local branch switch dirtied the tree inside the directory whose every change routes a PR to migration replay and preview-branch creation. Trigger: any `supabase` CLI use that changes the local current branch, then `git status`. Fix: `git rm --cached` the file (it stays on disk for the CLI) and add supabase/.branches/ to .gitignore beside supabase/.temp/. Test: tests/clean-artifacts-tracked-paths.test.ts "does not track the Supabase CLI's per-machine branch state". Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FSPY4VSqg7WVukCvmHQP9t
… / --prune (L60) Defect: sweep-branch-ledger.mjs (REPORT ONLY) ran `git fetch` unless --no-fetch was passed, and sweep-merged-branches.mjs ran `git remote prune origin` in its default dry-run unless --no-prune was passed, so the tools an agent reaches for during a read-only audit reached GitHub without the confirmation the provider boundary requires. Trigger: `npm run sweep:branch-ledger` or `npm run branch:cleanup` with no flags. Fix: network is opt-in. sweep-branch-ledger fetches only with --fetch and otherwise prints the exact fetch command on stderr; sweep-merged-branches prunes only with --prune and otherwise says so. --no-fetch / --no-prune stay accepted and mean the default. The narrow-refspec refusal keeps failing closed and now names --fetch as the remedy. Test: tests/sweep-scripts-offline-default.test.ts. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FSPY4VSqg7WVukCvmHQP9t
…ll build string (L35) Defect: scripts/design-sync.mjs installed five npm packages at whatever version the registry served (no version, --package-lock=false) and ran `.design-sync/config.json`'s buildCmd through `shell: true` — the only non-literal shell string under scripts/. Trigger: `node scripts/design-sync.mjs` with no arguments. Fix: the registry call is opt-in (--install) and off by default; DS_SYNC_PACKAGES carries exact versions (esbuild and @types/react from the lockfile, @tailwindcss/cli at the repository's tailwindcss version) and refuses any still-unpinned entry (ts-morph, geist) unless --allow-unpinned is passed explicitly. buildCmd is parsed into argv steps with the single `cat X >> Y` redirection performed by Node, and any other shell syntax is refused. The script is importable (main guarded) so the parsing is unit-tested. The two remaining pins need a registry lookup, which this change does not make. Test: tests/design-sync-supply-chain.test.ts. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FSPY4VSqg7WVukCvmHQP9t
…ib (L128) Defect: findOwnerIdByEmail was defined three times (eval-utils.ts, import-documents.ts, enrich-documents.ts) and interactive prompting twice (cli-utils.ts confirm and an inline readline in review-therapy.mjs), so a hardening change to one copy did not reach the others. Trigger: any fix to owner lookup or prompt handling landing in only one of the copies. Fix: scripts/lib/find-owner-id-by-email.ts is the single lookup (structural client type, optional `purpose` for the not-found message); eval-utils.ts re-exports it so eval callers keep their import, and import-documents, enrich-documents and purge-query-logs import it. scripts/lib/confirm.mjs owns readline (`createPrompt`) and the y/N `confirm`; cli-utils.ts re-exports both and review-therapy.mjs builds its exact-string attestation prompt on createPrompt. No exported symbol is removed. docs/scripts-index.md inventory regenerated by the pre-commit hook. Test: tests/scripts-shared-helpers.test.ts (behaviour plus a one-definition guard). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FSPY4VSqg7WVukCvmHQP9t
…es (M15, M18, L35) The new tests indexed the offline environment with computed keys, passed a plain object where NodeJS.ProcessEnv was declared, and called dsSyncInstallSpecifiers with a narrower package map than its inferred literal type. parseEnrichArgs now takes a plain Record<string, string | undefined>, dsSyncInstallSpecifiers declares its map type in JSDoc, and the two tests annotate the environment the way tests/test-runner-safety does. No behaviour change; `npx tsc --noEmit -p tsconfig.typecheck.json` is clean. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FSPY4VSqg7WVukCvmHQP9t
…verbose (L133) Defect: every `npm run ledger:lookup` printed all ~590 content-addressed record paths on one ~60 KB `files:` line ahead of the verdict, and the review-throttle rule tells every reviewer to run it first, so each review started by paying that context cost. Trigger: `npm run ledger:lookup -- <ref>` on the current record corpus. Fix: the `files:` line reports "N ledger table(s) + M immutable record(s)" by default; `--verbose` restores the full list and `--json` still carries `sources` unchanged. No record content, ledger content or lookup verdict changes. Test: tests/ledger-lookup-output.test.ts. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FSPY4VSqg7WVukCvmHQP9t
Both sides moved the generated counts line. main's numbers reflect the scripts now on main; the count is regenerated by npm run docs:update and verified by npm run docs:check-scripts, which the gate runs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DHSyfuC6mS98ystWFiitAR
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Updates to Preview Branch (claude/audit-fix-p7) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
Contributor
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #15743 (cancelled). That run's conclusion is an aggregate and did not exercise Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
BigSimmo
marked this pull request as ready for review
September 4, 2026 18:31
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
BigSimmo
enabled auto-merge (squash)
September 4, 2026 18:31
Contributor
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_c42d9951-1d55-46b9-8b2d-be36a8a76583) |
The scripts index, branch-review index and repo-awareness snapshot are generated artefacts. Merging main brought in a new ledger record and this package adds shared script helpers, so all three had drifted from their generators and CI's docs inventory check went red. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DHSyfuC6mS98ystWFiitAR
…abase into claude/audit-fix-p7
Contributor
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_b2a17549-76c2-473c-9fa4-c4271e261130) |
Contributor
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_16fb6215-0503-4aae-b94f-4789e43202b6) |
Contributor
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_f75ea269-d5cf-4116-ac3a-8c017c383f44) |
# Conflicts: # data/repo-awareness-snapshot.json
Contributor
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_0cc9dd27-58b5-4885-9ad1-93f1ae656062) |
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
2 participants
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
Audit remediation package P7 — Scripts, test safety and ledger tooling, from
docs/audit/full-repository-audit-2026-09-02.md(PR #2573). Findings fixed:M15,M18,M28,L59,L26,L60,L35,L128,L133,L63.Two of these are guardrails around tooling that can do real damage:
M18—enrich:documentsmutated by default. It now requires--writeto change anything,--all-ownersto cross owner boundaries, and calls the Supabase project check before it constructs an admin client. The default is a dry run. Verified by reading the argument handling: there is no path where a bare invocation mutates.M15— test runs could reach a real database. The db-test helper now asserts a loopback host forCARING_CONTACTS_DATABASE_URL, and the offline test environment scrubs that variable unless an explicit opt-in is set. The subtle half is that the opt-in has to be honoured insidescripts/run-vitest.mjsitself, because it applies the offline environment to whatevercaring-contacts/run-db-tests.mjshands it — miss that and bothcaring-contacts:db:testand the CIcaring-contacts-dbjob break. Both paths were traced in the real code and confirmed working, including the CI job that setspostgres://postgres@127.0.0.1:54329/postgresand passes the guard.The rest:
L26stopsclean:artifactsmatching a git-tracked evidence directory;L60makes report-only sweep scripts default to offline behind an explicit--networkflag;L35handles the design-sync script's unpinned installs;L128consolidates duplicate owner-lookup and confirmation helpers intoscripts/lib;L133makesledger:lookupprint paths one per line instead of a single 60 KB line;L63untrackssupabase/.branches/_current_branchand ignores it.M28/L59are script-side only: the/issuesreader and its data path now honour outcome corrections and the pending inbox. Confirmed thatdocs/outstanding-issues.md,docs/branch-review-ledger.mdand the inbox appear nowhere in this diff — this package reads those files, it never writes them.Findings partially skipped, with reasons — each names a file this package does not own:
M28/L59(hook half) —.claude/hooks/issues-surface.shis not owned here, so the SessionStart hook still reads the worktree ledger without the pending inbox.L26(script half) — theclean:artifactsscript line lives inpackage.json, owned elsewhere. The.gitignorehalf is fixed.M15(documentation half) —vitest.config.mtsanddocs/testing.mdare not owned here.L35(documentation half) — the script's network behaviour belongs indocs/scripts-index.mdand.design-sync/NOTES.md, neither owned here.RAG impact: none — maintenance scripts and test-environment safety only; no retrieval, ranking, ordering or selection surface touched.
Verification
npm run verify:pr-local—- completed: check:runtime, check:installed-lock-parity, format:changed, check:diff-integrity, lint, typecheck, test, check:repo-awareness-snapshot, eval:rag:offline, eval:rag:adversarial:offline, check:medication-interactions, check:medication-lexicon-report·- failed: (none)·- not reached: (none)·Test Files 1134 passed | 1 skipped (1135)·Tests 14996 passed | 2 expected fail | 3 skipped (15001)npm run check:diff-integrity—[diff-integrity] PASS — 8 changed test file(s), 9 -> 43 test case(s), against base 51ddfcd83.No test deleted, skipped or weakened; the case count nearly quintupled.Test Files 8 passed (8) / Tests 43 passed (43)— rather than trusting the implementer's report, and traced bothM15paths and theM18argument handling in the source.git ls-files supabase/.branches/is empty and the path is ignored (L63confirmed, not assumed).npm run docs:check-inventoryafter the latestorigin/mainmerge —Docs inventory current: 301 script files, 297 npm scripts.This is the exact line that failed CI'sStatic PR checksjob before the regeneration commit (docs/scripts-index.md inventory is stale).npm run check:repo-awareness-snapshot—[repo-awareness] in step with data/repo-awareness-snapshot.json (219 pages, 858 documents, 2693 reviews)npm run check:ledger-write-discipline—Ledger write discipline passed for 353c5297f547..HEAD.npm run docs:check-index—docs/codebase-index.md coverage OK: all 66 repository roots/modules/routes and all schema tables are indexed.Verification not run:
npm run verify:ui— no UI surface changed; browser proof left to CI.Verification not run:
npm run verify:release— no release or handoff confidence is claimed.Verification not run: provider-backed gates — nothing here reaches OpenAI, Supabase, Railway or Sentry at run time. No command was run against a real database.
Note:
origin/mainwas merged in (not rebased), most recently to pick up the ledger record and On Call mode work that landed while this pull request was open. The merges were clean with no overlap against this package's own files; only the three generated indexes (docs/scripts-index.md,docs/branch-review-index.md,data/repo-awareness-snapshot.json) needed regenerating from their own generators afterwards.Risk and rollout
enrich:documentswill get a dry run instead of a mutation.Clinical Governance Preflight
This package is classified clinical-risk because it deletes the tracked
supabase/.branches/_current_branchmarker (findingL63), which puts asupabase/path in the diff. Nothing in it touches clinical content, retrieval,answers, sources or patient data.
No answer generation, citation, or source-rendering code is in this diff. The verification requirement is unchanged.
No document workflow is added. The
enrich:documentschange narrows what an operator script may do, and adds an owner boundary that previously did not exist.Clinical KB Database(sjrfecxgysukkwxsowpy)Unchanged.
enrich:documentsnow calls the Supabase project check before it constructs an admin client, so a wrong-project run is refused earlier than it was.Unchanged. No key handling moved; nothing here runs in the browser bundle.
Unchanged. The test-environment change strengthens the separation: offline test runs now have
CARING_CONTACTS_DATABASE_URLscrubbed unless an explicit opt-in is set, and the db-test helper refuses any non-loopback host.Unchanged. No source metadata, review status or freshness logic is touched.
Checked and not applicable: no clinical decision-support behaviour changes. This package changes maintenance scripts, the test environment, and one untracked-file marker.
No migration is added or altered, so nothing here reaches the live clinical
database on merge.
Notes
mainindocs/scripts-index.mdwas resolved in favour of main's regenerated counts, then the file was regenerated from its own generator after the latest merge and re-checked.🤖 Generated with Claude Code
https://claude.ai/code/session_01DHSyfuC6mS98ystWFiitAR
Note
Medium Risk
Changes operator script semantics (dry-run, explicit flags) and test/DB collection guards; mistakes could block workflows, but they reduce accidental writes to hosted Postgres and unintended enrichment.
Overview
Audit P7 hardening for maintenance scripts, test safety, and ledger/issues tooling—no product or RAG surface changes.
Destructive paths now opt-in:
enrich:documentsdefaults to dry-run (needs--writeto persist or call OpenAI), requires an explicit owner or--all-owners, and runs the Supabase project check before the admin client. Caring Contacts DB tests only run against loopback URLs;CARING_CONTACTS_DATABASE_URLis cleared in the offline test env unlesscaring-contacts/run-db-tests.mjssetsCARING_CONTACTS_DB_TESTS=1, and gate receipts treat those vars as outcome-affecting.Supply chain / network defaults:
design-syncuses pinned installs (--installfor registry), refuses unpinned packages unless--allow-unpinned, and runsbuildCmdvia parsed argv/append instead of a shell. Branch sweep scripts do not fetch or prune unless--fetch/--prune.Operator UX:
ledger:lookupsummarizes source file counts unless--verbose;issuesreport projects the pending inbox onto the ledger, shows queue gate/stop text beside row detail, and sharedscripts/libhelpers consolidate owner lookup andconfirm.Repo hygiene:
.gitignoreignoressupabase/.branches/while keeping one tracked staging evidence folder; per-machine Supabase branch state is removed from tracking.Reviewed by Cursor Bugbot for commit 11603a7. Configure here.