Complete high-yield reliability safeguards - #1814
Conversation
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughThe PR adds complete installed-tree parity checks, extends design-system contract auditing, centralizes authorization-header handling, refines mobile search layouts and clearing, and improves branch-review flag parsing. ChangesInstalled dependency parity
Design-system contract checks
Authorization identity normalization
Mobile search behavior
Branch review flag parsing
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:76f8b4e6a5
ℹ️ 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".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
BigSimmo
commented
Aug 11, 2026
@codex resolve actionable Codex review findings for this pull request and current head using the repository instructions. This is the pull request's single automatic repair pass: do not perform a fresh review, create new standalone findings, or request another review. Work only the existing unresolved Codex threads on the current head. The only repository destination is BigSimmo/Database, and the only branch destination is the pull request head branch codex/chat-ledger-quick-wins-ledger-quick-wins at starting commit 76f8b4e; never publish fixes to a detached or synthetic work branch and never create a stacked pull request. Use the authenticated GitHub connector to commit each approved fix to BigSimmo/Database:codex/chat-ledger-quick-wins-ledger-quick-wins, then verify that the pull request head contains the published commit before reporting success. Always fix P0 and P1 findings. For P2 and lower findings, fix only clear, scoped, low-risk issues; otherwise disposition them with a concise reason. For a fixed thread, reply with as the first line and as the second line. For a no-code disposition, use followed by . These result markers authorize the workflow to close that exact thread only after it verifies a fixed commit is the pull request head; a local-only commit is not a fix. If publication or verification fails, do not use either result marker, do not claim success, and leave the thread open with the blocker. If human input or new authorization is required, do the same. Finish only after every actionable thread is fixed or dispositioned and closed, or explicitly left open for a human decision. Do not update the branch from main, address unrelated reviews, broaden scope, or create more than one scoped fix commit. Do not use external APIs, paid services, credentials, dependency changes, or broad refactors unless explicitly authorized. Add targeted tests where behavior changes and run the narrowest relevant validation. |
Summary
Testing
|
BigSimmo
commented
Aug 11, 2026
Unblock sweep snapshot: PR is at head �6e716e7d5d554e343f91f38af57a111e7aa9b7d, compare-to-main status �head 4 / behind 0 (merge-tree clean, no merge conflicts), snapshot mergeable MERGEABLE with mergeStateStatus=BLOCKED before/after a no-code sync check. Required checks all green: PR required, PR mergeability, PR policy, Build, Change scope, Static PR checks, and Production UI ((1)/(2)/(3) + critical) are pass. No unresolved review threads found that block merge/required CI. No blocker-fix commit was needed; state is cleared for required-CI mergeability. Residual risk: merge is still blocked by repo policy/state (mergeStateStatus=BLOCKED) despite green checks; I left that to you to arm/complete merge from your side. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
tests/private-client-auth.test.ts (1)
34-39: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover the declared header precedence.
The helper prioritizes
authorizationoverAuthorization, but the test only checks each key separately. Add a case with conflicting values so the request-identity contract cannot regress.Proposed regression assertion
expect(authorizationIdentity({ authorization: "Bearer lower" })).toBe("Bearer lower"); expect(authorizationIdentity({ Authorization: "Bearer upper" })).toBe("Bearer upper"); + expect(+ authorizationIdentity({+ authorization: "Bearer lower",+ Authorization: "Bearer upper",+ }),+ ).toBe("Bearer lower"); expect(authorizationIdentity({})).toBe("");🤖 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/private-client-auth.test.ts` around lines 34 - 39, Add a conflicting-header assertion to the authorizationIdentity test, providing different values for authorization and Authorization and verifying the lowercase authorization value is returned. Keep the existing casing and missing-header assertions unchanged.tests/installed-lock-parity.test.ts (1)
121-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the two stamp-invalidation branches.
The cache test is correct. Two branches of
installedTreeParitycarry no coverage, and both are central to the stamp trust model:
- The schema check at
scripts/check-installed-lock-parity.mjsline 206.- The lock-digest check at
scripts/check-installed-lock-parity.mjsline 209, which rejects a stamp that belongs to a differentpackage-lock.json.The lock-digest branch is the guard that stops a copied donor
node_modulesfrom validating against a different lockfile.scripts/setup-codex-worktree.mjsdepends on it at line 237.💚 Proposed tests
it("rejects a stamp that belongs to a different package-lock.json",()=>{constroot=treeFixture();writeFileSync(path.join(root,"package-lock.json"),JSON.stringify({lockfileVersion: 3,packages: {"node_modules/direct": {version: "1.0.0"},"node_modules/direct/node_modules/transitive": {version: "2.0.0"},},name: "changed",}),);expect(installedTreeParity(root)).toEqual(expect.objectContaining({ok: false,reason: "install stamp belongs to a different package-lock.json"}),);});it("rejects an unsupported stamp schema",()=>{constroot=treeFixture();conststampPath=path.join(root,"node_modules",".codex-installed-tree.json");conststamp=JSON.parse(readFileSync(stampPath,"utf8"));writeFileSync(stampPath,JSON.stringify({ ...stamp,schema: 99}));expect(installedTreeParity(root)).toEqual(expect.objectContaining({ok: false,reason: "install stamp schema 99 is unsupported"}),);});Import
readFileSyncand exportinstalledTreeStampNamein place of the literal file name if you prefer to avoid duplicating the constant.🤖 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/installed-lock-parity.test.ts` around lines 121 - 129, Add tests covering both untested stamp-validation branches in installedTreeParity: modify package-lock.json in a treeFixture to verify a mismatched lock digest returns ok: false with the “install stamp belongs to a different package-lock.json” reason, and modify the generated stamp’s schema to an unsupported value to verify the corresponding rejection reason. Import readFileSync and reuse the exported installedTreeStampName constant instead of duplicating the stamp filename where applicable.scripts/setup-codex-worktree.mjs (1)
39-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider removing the dead
packageNamesplumbing.
installedTreeParityvalidates the complete tree, so per-package filtering is obsolete.installationIsCompletenow discardspackageNameswithvoid packageNames.findDependencyDonorat line 60 still accepts and forwards the parameter, andmainat line 214 already omits it.Dropping the parameter from both functions removes the dead path. This changes the exported signature, and
tests/setup-codex-worktree.test.tsline 83 passes["next"], so update the test in the same change. Defer this if you prefer to keep the signature stable for now.🤖 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 `@scripts/setup-codex-worktree.mjs` around lines 39 - 50, Remove the obsolete packageNames parameter and its void discard from installationIsComplete and findDependencyDonor, updating their callers to use the new signatures while preserving complete-tree validation via installedTreeParity. Update the setup-codex-worktree test to stop passing the per-package array.tests/search-results-header-band.dom.test.tsx (1)
300-304: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the new
shrink-0contract directly.The component change at
src/components/clinical-dashboard/search-results-header-band.tsxLine [449] prevents utilities from shrinking. This test does not assert that class. Addexpect(utilities).toContain("shrink-0")so the focused regression test detects a return of the clipping behavior.🤖 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/search-results-header-band.dom.test.tsx` around lines 300 - 304, Update the test assertions for utilities to directly verify the new non-shrinking contract by adding a positive assertion that utilities contains "shrink-0". Keep the existing negative utility checks and rowClass assertions unchanged.src/components/clinical-dashboard/search-results-header-band.tsx (1)
296-301: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the phone ribbon height to a named token.
Line [301] uses the arbitrary
min-h-[3.625rem]design value in TSX. Define the measured phone height as a Tailwind 4@themetoken or an intentionally unlayered component CSS class, then use that named value here.As per coding guidelines, “Use Tailwind 4
@themetokens in src/app/globals.css and the repository's intentionally unlayered component CSS rather than introducing hardcoded design values.”🤖 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 `@src/components/clinical-dashboard/search-results-header-band.tsx` around lines 296 - 301, Replace the arbitrary min-h-[3.625rem] value in the inlineControls class within the search-results header component with a named phone-ribbon height token. Define the measured 3.625rem value in the established Tailwind 4 `@theme` section of globals.css or an intentionally unlayered component CSS class, then reference that named utility/class here.Source: Coding guidelines
🤖 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.
Inline comments:
In `@package.json`:
- Line 14: Update the postinstall flow around check-installed-lock-parity.mjs
and writeInstalledTreeStamp so pruned installs with omitted dev dependencies do
not fail on the expected lockfile mismatch. Skip or gracefully handle stamping
for this case, but ensure install-git-hooks.mjs still runs and preserve the
existing full-install fallback when the stamp is absent.
In `@scripts/branch-review-ledger.mjs`:
- Line 394: Update parseFlags so equals-form values for BOOLEAN_FLAGS are
handled before generic inline assignment: convert only "true" and "false" to
booleans, and reject any other value. Apply this consistently to json,
supersede, and dry-run while preserving existing non-boolean flag parsing, and
add focused coverage for each flag.
In `@scripts/check-installed-lock-parity.mjs`:
- Around line 212-218: Update the failure handling around
installedMetadataMatches and packageFailures so location failures and
hidden-lock metadata mismatches produce separate operator-facing reasons. Report
the actual packageFailures count only when locations fail; when metadata
mismatches without location failures, use a reason identifying the metadata
mismatch instead of fabricating a location count. Preserve packageLocations in
the returned failure object.
- Around line 139-142: Update the skip condition in the walk logic so
installedTreeStampName and VOLATILE_DIRECTORIES entries are ignored only when
relativeDirectory is empty, while preserving normal traversal and inventory of
matching entries in nested dependency directories.
In `@scripts/design-system-contract-utils.mjs`:
- Around line 1383-1385: Update the count-bearing matcher used by the ErrorState
helper at scripts/design-system-contract-utils.mjs:1383-1385 to detect plain
total when paired with result nouns, including copy such as “total results,”
while preserving existing count and member-property matches. Apply the same
paired-total detection in the failure-branch JSX logic at
scripts/design-system-contract-utils.mjs:1419-1424. Add regression cases
covering plain total and .total for both helpers in
tests/design-system-contract-utils.test.ts:29-65.
---
Nitpick comments:
In `@scripts/setup-codex-worktree.mjs`:
- Around line 39-50: Remove the obsolete packageNames parameter and its void
discard from installationIsComplete and findDependencyDonor, updating their
callers to use the new signatures while preserving complete-tree validation via
installedTreeParity. Update the setup-codex-worktree test to stop passing the
per-package array.
In `@src/components/clinical-dashboard/search-results-header-band.tsx`:
- Around line 296-301: Replace the arbitrary min-h-[3.625rem] value in the
inlineControls class within the search-results header component with a named
phone-ribbon height token. Define the measured 3.625rem value in the established
Tailwind 4 `@theme` section of globals.css or an intentionally unlayered component
CSS class, then reference that named utility/class here.
In `@tests/installed-lock-parity.test.ts`:
- Around line 121-129: Add tests covering both untested stamp-validation
branches in installedTreeParity: modify package-lock.json in a treeFixture to
verify a mismatched lock digest returns ok: false with the “install stamp
belongs to a different package-lock.json” reason, and modify the generated
stamp’s schema to an unsupported value to verify the corresponding rejection
reason. Import readFileSync and reuse the exported installedTreeStampName
constant instead of duplicating the stamp filename where applicable.
In `@tests/private-client-auth.test.ts`:
- Around line 34-39: Add a conflicting-header assertion to the
authorizationIdentity test, providing different values for authorization and
Authorization and verifying the lowercase authorization value is returned. Keep
the existing casing and missing-header assertions unchanged.
In `@tests/search-results-header-band.dom.test.tsx`:
- Around line 300-304: Update the test assertions for utilities to directly
verify the new non-shrinking contract by adding a positive assertion that
utilities contains "shrink-0". Keep the existing negative utility checks and
rowClass assertions unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f6c62da0-63af-4b3b-8140-3ceb0c13f064
📒 Files selected for processing (24)
docs/design-system/GATES.mddocs/design-system/adoption-manifest.jsonpackage.jsonscripts/branch-review-ledger.mjsscripts/check-design-system-contract.mjsscripts/check-installed-lock-parity.mjsscripts/design-system-contract-baseline.jsonscripts/design-system-contract-utils.mjsscripts/phone-chrome-plan.mjsscripts/setup-codex-worktree.mjssrc/components/DocumentViewer.tsxsrc/components/clinical-dashboard/search-results-header-band.tsxsrc/components/clinical-dashboard/use-signed-image-url.tssrc/components/services/services-navigator-page.tsxsrc/lib/authorization-header.tssrc/lib/supabase/client.tsxtests/design-system-contract-utils.test.tstests/installed-lock-parity.test.tstests/private-client-auth.test.tstests/repo-hygiene.test.tstests/search-results-header-band.dom.test.tsxtests/setup-codex-worktree.test.tstests/ui-smoke.spec.tstests/ui-tools.spec.ts
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
BigSimmo
commented
Aug 11, 2026
PR #1814 head is now at Status snapshot:
Blockers handled:
No code blocker edits were needed for mergeability at this unblock step. Residual risk:
|
Summary
ErrorStatecontract; and verify the full installed dependency tree plus structural reuse stamps.#149,#273,#274,#289,#298, and#303.RAG impact: no retrieval behaviour change - this PR changes UI query cleanup, client auth-header normalization, design-system contracts, ledger parsing, and local dependency verification only.
Verification
npm run verify:pr-local- partial on exact integrated head76f8b4e6a5742700ed3ed77b45e591062111e111: runtime, installed-lock parity (755 package locations / 51,732 files), changed-file formatting, npm-ci dry run, sitemap/docs checks, both ledger checks, lint, and typecheck passed. Full Vitest then stopped on 16 Windows-baseline failures inbundle-budget.test.tsandpr-handoff-stop.test.ts; build and RAG fixtures were not reached.npm run check:production-readiness- local Node and query-hash boot-guard safeguards passed, but the isolated worktree has no copied.env.local; Supabase/OpenAI provider configuration was therefore environment-gated and the command exited 1.UI verification not run: the broad
npm run verify:uigate was not run because the two directly affected Chromium journeys passed focused proof; the change does not alter shared UI foundations.Verification not run:
npm run verify:releasewas not run because this is a draft PR handoff, not a release-confidence claim. Live retrieval/answer evaluations were not run because retrieval, ranking, synthesis, and clinical answer behavior are unchanged.Risk and rollout
8374c3b66(or the PR merge) to restore the prior behavior; there is no schema migration, dependency version change, or persisted-data transformation.Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)Notes
#296and ledger-file mutations remain deferred because the original checkout contains overlapping concurrent edits; they are intentionally not included here.Summary by CodeRabbit
Bug Fixes
Quality Improvements