fix(audit): System Audit Remediation and Improvements - #1262
Conversation
- Fix #ISSUE-01: RAG Score Saturation Tie-Breaking - Fix #ISSUE-03: Heavy !important CSS Overrides in Mobile Stack using @layer - Fix #ISSUE-04: Legacy Supabase Project Ref Leakage - Fix #ISSUE-05: High Serial Latency in cheap verification gate via parallelism - Fix #ISSUE-06: Cross-Platform Font Rendering Drift via maxDiffPixelRatio - Fix #ISSUE-07: Extract offline preflight helper functions into rag-preflight-utils.ts - Implemented #IMP-05 parallel verification orchestrator
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 34939609 | Triggered | PostgreSQL Credentials | 1f32144 | scripts/lib/rag-preflight-utils.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
📝 WalkthroughWalkthroughThe pull request updates Supabase project guidance, introduces parallel cheap verification, changes test lock contention behavior, adds RAG preflight helpers, reorganizes Therapy Compass CSS layers, configures visual diff limits, and handles PDF extractor exit code 137. ChangesSupabase target guidance
Verification orchestration
RAG preflight utilities
Therapy Compass visual behavior
PDF extractor termination handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant npm
participant verify-cheap-parallel.mjs
participant CheapChecks
participant typecheck
participant test
npm->>verify-cheap-parallel.mjs: Run verify:cheap:internal
verify-cheap-parallel.mjs->>CheapChecks: Execute configured checks in parallel
verify-cheap-parallel.mjs->>typecheck: Run after cheap checks
verify-cheap-parallel.mjs->>test: Run after typecheck
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:833307d8a7
ℹ️ 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".
| async function run() { | ||
| console.log("Starting parallel verification..."); | ||
| try { | ||
| await Promise.all(parallelTasks.map(runTask)); |
There was a problem hiding this comment.
When verify:cheap runs on a typical developer or CI host, this starts all 23 gates simultaneously, including the memory-intensive ESLint and Knip analyses; because the outer heavyweight-lock token is inherited, the nested lint wrapper is treated as reentrant and does not serialize the workload. The expected one-heavy-command-at-a-time behavior is therefore bypassed, risking memory exhaustion or severe contention in the repository's baseline gate. Use a bounded worker pool and keep heavyweight analyses sequential; a focused check should assert the maximum concurrent child count.
AGENTS.md reference: AGENTS.md:L167-L168
Useful? React with 👍 / 👎.
| }); | ||
| } catch (err) { | ||
| console.error(`\n✘ [FAILED] ${err.message}`); | ||
| process.exit(1); |
There was a problem hiding this comment.
When any fast gate fails while slower gates are still running, Promise.all rejects immediately and this process.exit(1) abandons every spawned shell without terminating or awaiting it. The outer lock is then released while those orphaned checks can continue consuming resources or writing caches concurrently with a retry; an isolated stub reproduction left all 22 sibling tasks running after the verifier exited. Track the children and terminate their process trees, or await all tasks to settle, before exiting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@playwright.visual.config.ts`:
- Around line 12-13: Lower the global maxDiffPixelRatio from 0.05 to a stricter
threshold for both toHaveScreenshot and toMatchSnapshot in the visual
configuration. Preserve or add lower-tolerance checks for mobile and !important
styling, only retaining a higher ratio where demonstrable visual noise requires
it.
In `@scripts/lib/rag-preflight-utils.ts`:
- Around line 14-19: Update the maxTokens and maxCost validations in the
preflight utility to check explicitly for undefined, so zero-valued limits are
enforced and any positive estimate produces an error. Add boundary tests
covering maxTokens: 0 and maxCost: 0 while preserving the existing behavior for
undefined limits.
In `@scripts/test-run-lock.mjs`:
- Around line 176-178: Update both test-mode guards in acquireHeavyRunLock to
read NODE_ENV from the injected environment parameter instead of process.env,
ensuring custom environments consistently control fail-fast behavior and retry
handling.
In `@scripts/verify-cheap-parallel.mjs`:
- Around line 50-63: Update the verification flow around parallelTasks, runTask,
and the catch handler to retain references to every spawned child process and
terminate all still-active children when any task fails. Ensure cleanup runs
before process.exit(1), while preserving the existing Promise.all and test
execution behavior.
In `@tests/therapy-compass-responsive-contract.test.ts`:
- Around line 88-92: Update the responsive contract assertions around the
therapy CSS source to extract and validate the `@media` (max-width: 640px) block,
rather than matching rules across the whole file. Ensure grouped
.tc-mobile-stack and .tc-compare-table selectors are matched when
comma-separated, and add an assertion that the utilities layer is declared after
the components layer so mobile overrides retain their cascade precedence.
🪄 Autofix (Beta)
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 Plus
Run ID: c6c3a8e0-dac3-42e3-9908-62dacc205672
📒 Files selected for processing (17)
.claude/agents/supabase-schema-guardian.mdAGENTS.mdREADME.mddocs/auth-connection-cap-runbook.mddocs/codebase-index.mddocs/codex-prompt-playbook.mdpackage.jsonplaywright.visual.config.tsplugins/clinical-kb/skills/clinical-kb-workflow/SKILL.mdscripts/check-gate-manifest.mjsscripts/lib/rag-preflight-utils.tsscripts/test-run-lock.mjsscripts/verify-cheap-parallel.mjssrc/components/therapy-compass/therapy-compass.csssrc/lib/extractors/document.tstests/pdf-extractor.test.tstests/therapy-compass-responsive-contract.test.ts
| toHaveScreenshot: { maxDiffPixelRatio: 0.05 }, | ||
| toMatchSnapshot: { maxDiffPixelRatio: 0.05 }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Playwright testConfig expect toHaveScreenshot toMatchSnapshot maxDiffPixelRatio option
💡 Result:
In Playwright, the maxDiffPixelRatio option is used to define an acceptable ratio of differing pixels (between 0 and 1) compared to the total number of pixels in an image during snapshot comparisons [1][2]. Both toHaveScreenshot and toMatchSnapshot support this option [1][2]. You can configure maxDiffPixelRatio globally or per project in your Playwright test configuration, as well as override it on a per-test basis [3]. Global Configuration To set a default value for all tests, use the defineConfig function in your playwright.config.ts file [3]: import { defineConfig } from '@playwright/test'; export default defineConfig({ expect: { toHaveScreenshot: { maxDiffPixelRatio: 0.1, // Example: 10% tolerance }, toMatchSnapshot: { maxDiffPixelRatio: 0.1, }, }, }); Usage in Tests You can also pass the option directly within your test assertions to override the global setting [3][2]: // Using toHaveScreenshot await expect(page).toHaveScreenshot('example.png', { maxDiffPixelRatio: 0.05, }); // Using toMatchSnapshot expect(await page.screenshot).toMatchSnapshot('example.png', { maxDiffPixelRatio: 0.05, }); Key Differences While both methods support pixel comparison options like maxDiffPixelRatio and maxDiffPixels, they are intended for different use cases [3][4]: toHaveScreenshot: Designed specifically for visual regression testing of full pages or locators [1][3]. It includes built-in features for waiting for screenshots to stabilize and handles image comparison automatically [1]. toMatchSnapshot: A more general-purpose matcher that compares arbitrary data, including text, binary data, and images [3][2]. It does not include the specialized screenshot stabilization features found in toHaveScreenshot [1][2].
Citations:
- 1: https://playwright.dev/docs/api/class-pageassertions
- 2: https://playwright.dev/docs/api/class-snapshotassertions
- 3: https://playwright.dev/docs/test-snapshots
- 4: [Question]: Clarification on
toHaveScreenshotandtoMatchSnapshotmicrosoft/playwright#21632
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"== repo files of interest =="
git ls-files | rg '(^|/)playwright.*\.(ts|js|mjs|cjs|json|lock)$|(^|/)package\.json$|(^|/)package-lock\.json$|(^|/)pnpm-lock\.yaml$|(^|/)yarn\.lock$'||trueechoecho"== playwright.visual.config.ts =="if [ -f playwright.visual.config.ts ];then
cat -n playwright.visual.config.ts
fiechoecho"== playwright config files =="forfin playwright.config.ts playwright.config.js playwright.config.mjs playwright.config.cjs package.json package-lock.json pnpm-lock.yaml yarn.lock;doif [ -f"$f" ];thenecho"--- $f ---"if [ "$f"= package.json ];then
node -e "const p=require('./package.json'); console.log(JSON.stringify({scripts:p.scripts,dependencies:p.dependencies && {playwright:p.dependencies.playwright, '`@playwright/test`':p.dependencies['`@playwright/test`']},devDependencies:p.devDependencies && {playwright:p.devDependencies.playwright, '`@playwright/test`':p.devDependencies['`@playwright/test`']}}, null, 2))"elif rg -q "maxDiffPixelRatio|playwright|`@playwright/test`""$f";then
rg -n "maxDiffPixelRatio|playwright|`@playwright/test`|\"playwright\"|\"`@playwright/test`\"""$f" -C 2
fifidoneechoecho"== CSS cascade/important/layer mentions in migrations/sql/ts/js =="
rg -n "!important|`@layer`|layer|mobile|override|cascade""$(git ls-files | rg '\.(sql|ts|js|yml|yaml|md)$'|| true)" -C 1 ||trueRepository: BigSimmo/Database
Length of output: 50374
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"== visual config screen settings =="
sed -n '1,220p' playwright.visual.config.ts
echoecho"== visual spec and matching files =="
git ls-files 'tests/*ui-visual-artifacts*''tests/*visual*''tests/*screenshots*''tests/*snapshot*'| sed -n '1,200p'echoecho"== references to maxDiffPixelRatio in repo =="
rg -n "maxDiffPixelRatio|maxDiffPixels|toHaveScreenshot|toMatchSnapshot|!important|`@layer`" playwright playwright.visual.config.ts playwright.config.ts tests/*.ts 2>/dev/null ||trueechoecho"== playwright erts/API docs for expect options in installed package =="
node_modules/.bin/playwright --version ||trueif [ -d node_modules/playwright ];then
rg -n "maxDiffPixelRatio|toMatchSnapshot|toHaveScreenshot|expect:" node_modules/playwright -g '*.d.ts' -g '*.js'| sed -n '1,160p'fiechoecho"== deterministic pixel-budget math for 5% diff at common viewport sizes =="
python3 - <<'PY'for w,h in [(1280,720),(1920,1080),(375,667),(390,844)]: print(f"{w}x{h}: 5% = {w*h*0.05:.0f} pixels, 1% = {w*h*0.01:.0f} pixels")PYRepository: BigSimmo/Database
Length of output: 16570
Tighten the visual diff guard for @layer visual changes
maxDiffPixelRatio: 0.05 is accepted by Playwright 1.61.1 for both matchers, but it permits roughly 46k differing pixels at 1280×720. Since this PR changes the CSS cascade, use a tighter global ratio unless there’s noise that truly requires it, and keep mobile/!important styling covered by lower-tolerance visual checks.
🤖 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 `@playwright.visual.config.ts` around lines 12 - 13, Lower the global
maxDiffPixelRatio from 0.05 to a stricter threshold for both toHaveScreenshot
and toMatchSnapshot in the visual configuration. Preserve or add lower-tolerance
checks for mobile and !important styling, only retaining a higher ratio where
demonstrable visual noise requires it.
| if (constraints.maxTokens && estimatedTokens > constraints.maxTokens) { | ||
| errors.push(`Estimated tokens (${estimatedTokens}) exceeds maximum allowed (${constraints.maxTokens}).`); | ||
| } | ||
| if (constraints.maxCost && estimatedCost > constraints.maxCost) { | ||
| errors.push(`Estimated cost ($${estimatedCost}) exceeds maximum allowed ($${constraints.maxCost}).`); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not skip zero-valued limits.
The checks at Line 14 and Line 18 are bypassed when maxTokens or maxCost is 0, allowing any positive estimate through. Use explicit undefined checks and add zero-boundary tests.
Proposed fix
- if (constraints.maxTokens && estimatedTokens > constraints.maxTokens) {+ if (constraints.maxTokens !== undefined && estimatedTokens > constraints.maxTokens) {
...
- if (constraints.maxCost && estimatedCost > constraints.maxCost) {+ if (constraints.maxCost !== undefined && estimatedCost > constraints.maxCost) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(constraints.maxTokens&&estimatedTokens>constraints.maxTokens){ | |
| errors.push(`Estimated tokens (${estimatedTokens}) exceeds maximum allowed (${constraints.maxTokens}).`); | |
| } | |
| if(constraints.maxCost&&estimatedCost>constraints.maxCost){ | |
| errors.push(`Estimated cost ($${estimatedCost}) exceeds maximum allowed ($${constraints.maxCost}).`); | |
| if(constraints.maxTokens!==undefined&&estimatedTokens>constraints.maxTokens){ | |
| errors.push(`Estimated tokens (${estimatedTokens}) exceeds maximum allowed (${constraints.maxTokens}).`); | |
| } | |
| if(constraints.maxCost!==undefined&&estimatedCost>constraints.maxCost){ | |
| errors.push(`Estimated cost ($${estimatedCost}) exceeds maximum allowed ($${constraints.maxCost}).`); |
🤖 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/lib/rag-preflight-utils.ts` around lines 14 - 19, Update the
maxTokens and maxCost validations in the preflight utility to check explicitly
for undefined, so zero-valued limits are enforced and any positive estimate
produces an error. Add boundary tests covering maxTokens: 0 and maxCost: 0 while
preserving the existing behavior for undefined limits.
| if (owner && processIsAlive(owner.pid)) { | ||
| if (attempt < 15) { | ||
| const isTest = process.env.NODE_ENV === "test"; | ||
| if (!isTest && attempt < 15) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"== files =="
fd -a 'test-run-lock\.mjs$'.||trueecho"== outline =="
ast-grep outline scripts/test-run-lock.mjs --view expanded 2>/dev/null | sed -n '1,220p'||trueecho"== relevant lines =="
sed -n '70,210p' scripts/test-run-lock.mjs | nl -ba -v70
echo"== acquireHeavyRunLock usages/tests =="
rg -n "acquireHeavyRunLock|NODE_ENV|environment\\s*:" -S . --glob '!node_modules' --glob '!dist' --glob '!build'| sed -n '1,240p'Repository: BigSimmo/Database
Length of output: 940
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"== scripts/test-run-lock.mjs lines 82-210 =="
awk 'NR>=82 && NR<=210 {printf "%6d\t%s\n", NR, $0}' scripts/test-run-lock.mjs
echo"== usages of acquireHeavyRunLock / environment override =="
rg -n "acquireHeavyRunLock\\(|environment\\s*:" -S . --glob '!node_modules' --glob '!dist' --glob '!build'| head -200
echo"== test-mode and NODE_ENV mentions in scripts tests =="
rg -n "acquireHeavyRunLock|testRunLockInternals|NODE_ENV|environment:" scripts --glob '*.mjs' --glob '*.js' --glob '*.ts' --glob '*.tsx'| sed -n '1,240p'||trueRepository: BigSimmo/Database
Length of output: 14792
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"== offline test environment references =="
rg -n "offlineTestEnvironment|run-vitest|run-live-tests|run-playwright|NODE_ENV|testRunLockInternals" scripts tests --glob '*.mjs' --glob '*.js' --glob '*.ts' --glob '*.tsx'| sed -n '1,240p'echo"== read-only semantic probe for environment.NODE_ENV omission =="
node - <<'JS'function acquireLockAttemptWith(nodeEnvGlobal, environment) { // Mirrors the relevant conditional semantics around lines 176-178 / 191-193 const actualIsTest = environment.NODE_ENV === "test"; return actualIsTest ? "fail-fast" : "retry";}console.log({ globalTestEnvInjectedThroughEnvironment: acquireLockAttemptWith(undefined, { NODE_ENV: "test" }), globalNotTestWhenEnvironmentHasTest: acquireLockAttemptWith("development", { NODE_ENV: "test" }), globalTestNoEnvironmentNodeEnv: acquireLockAttemptWith("test", {}),});JSRepository: BigSimmo/Database
Length of output: 8178
Use the injected environment for test-mode detection.
acquireHeavyRunLock accepts environment, but both test-mode guards read process.env.NODE_ENV. Passing { NODE_ENV: "test" } through the injected environment bypasses the fail-fast path, while a custom injected environment can differ from the global process environment. Use environment.NODE_ENV in both branches.
Proposed fix
- const isTest = process.env.NODE_ENV === "test";+ const isTest = environment.NODE_ENV === "test";
...
- const isTest = process.env.NODE_ENV === "test";+ const isTest = environment.NODE_ENV === "test";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(owner&&processIsAlive(owner.pid)){ | |
| if(attempt<15){ | |
| constisTest=process.env.NODE_ENV==="test"; | |
| if(!isTest&&attempt<15){ | |
| if(owner&&processIsAlive(owner.pid)){ | |
| constisTest=environment.NODE_ENV==="test"; | |
| if(!isTest&&attempt<15){ |
🤖 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/test-run-lock.mjs` around lines 176 - 178, Update both test-mode
guards in acquireHeavyRunLock to read NODE_ENV from the injected environment
parameter instead of process.env, ensuring custom environments consistently
control fail-fast behavior and retry handling.
| await Promise.all(parallelTasks.map(runTask)); | ||
| await runTask("typecheck"); | ||
| console.log("Starting tests..."); | ||
| await new Promise((resolve, reject) => { | ||
| const child = spawn("npm", ["run", "test"], { shell: true, stdio: "inherit" }); | ||
| child.on("close", (code) => { | ||
| if (code !== 0) reject(new Error("test")); | ||
| else resolve(); | ||
| }); | ||
| }); | ||
| } catch (err) { | ||
| console.error(`\n✘ [FAILED] ${err.message}`); | ||
| process.exit(1); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"## Locate file"
git ls-files | rg '(^|/)verify-cheap-parallel\.mjs$'||trueecho"## File excerpt"if [ -f scripts/verify-cheap-parallel.mjs ];then
wc -l scripts/verify-cheap-parallel.mjs
sed -n '1,140p' scripts/verify-cheap-parallel.mjs | cat -n
fiecho"## Search for cleanup/cancellation patterns"
rg -n "Promise\.all|Promise\.allSettled|spawn\(|child\.(unref|kill|stdin|stdout|stderr)|on\('close'|on\(\"close\"|process\.exit" scripts/verify-cheap-parallel.mjs ||trueRepository: BigSimmo/Database
Length of output: 2940
🌐 Web query:
Node.js process.exit child processes forked processes killed on exit
💡 Result:
In Node.js, calling process.exit in the parent process does not automatically terminate or kill forked child processes [1][2]. When the parent process terminates, its child processes become "orphaned" (unless they were spawned as detached processes, in which case they continue running independently) [3][1]. Key points regarding the behavior: 1. Parent Termination: If the parent process exits, forked child processes continue to run unless explicitly handled [1][2]. Node.js does not automatically manage the lifecycle of children once the parent terminates [2]. 2. Manual Cleanup: To ensure child processes are terminated when the parent exits, you must explicitly manage this in your code [2]. A common approach is to listen for signals (such as SIGTERM or SIGINT) or the 'exit' event in the parent process, and then call subprocess.kill on each active child process [4][2]. 3. Child Self-Termination: A child process can exit itself by calling process.exit [5][4][6]. This is often the cleanest way to shut down a child process when its work is complete [4]. 4. IPC Channels: If a child process is forked with an IPC channel, the child can potentially detect when the parent process disconnects (via the 'disconnect' event) and shut itself down gracefully [5][7]. 5. Detached Processes: If you spawn a process with the detached: true option, it is intended to run independently of the parent process, and the parent will not automatically terminate it upon exiting [3][1]. If you notice child processes hanging after a parent process exits, it is likely because they were not explicitly signaled to shut down [4][1]. Implementing a cleanup mechanism in the parent's exit handler is the standard practice for ensuring all child processes are terminated [4][2].
Citations:
- 1: https://stackoverflow.com/questions/56789814/nodejs-forked-processes-get-killed-if-one-process-calls-exit
- 2: https://www.exratione.com/2013/05/die-child-process-die
- 3: https://nodejs.org/api/child_process.html
- 4: https://stackoverflow.com/questions/53269578/node-js-process-exit-vs-childprocess-kill
- 5: https://nodejs.org/api/process.html
- 6: https://stackoverflow.com/questions/34114428/proper-way-of-killing-forked-child-processes
- 7: https://github.com/krux/nodejs-pkg/blob/master/doc/api/child_process.markdown
Terminate remaining verification tasks when one task fails.
Promise.all aborts on the first failure, but process.exit(1) leaves the other spawned npm run children active. Those orphaned checks can continue running after the script exits, so keep active child references and shut them down before exiting.
🤖 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/verify-cheap-parallel.mjs` around lines 50 - 63, Update the
verification flow around parallelTasks, runTask, and the catch handler to retain
references to every spawned child process and terminate all still-active
children when any task fails. Ensure cleanup runs before process.exit(1), while
preserving the existing Promise.all and test execution behavior.
| expect(therapyCssSource).toContain("grid-template-columns: minmax(0, 1fr);"); | ||
| expect(therapyCssSource).toContain(".tc-root .tc-mobile-grid-2"); | ||
| expect(therapyCssSource).toContain(".tc-root .tc-mobile-static"); | ||
| expect(therapyCssSource).toContain(".tc-root .tc-compare-table"); | ||
| expect(therapyCssSource).toContain("overflow-x: auto !important;"); | ||
| expect(therapyCssSource).toContain("overflow-x: auto;"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
These assertions no longer guard the behavior they were rewritten for.
They are unscoped whole-file toContain checks. overflow-x: auto; also matches .tc-root .tc-nav-007 and .tc-root .tc-compare-tabs in the components layer, so Line 92 passes even if the entire @media (max-width: 640px) block were removed.
More importantly, !important was what previously guaranteed these mobile overrides win; that guarantee is now cascade-layer ordering (utilities declared after components). Nothing here asserts that ordering, so a future edit that drops or reorders the @layer wrappers silently breaks every phone override with a green test.
Consider scoping the rule extraction the way the sibling assertion at Lines 51-73 already does, and pinning the layer contract.
🧪 Scope the assertions and pin layer ordering
- expect(therapyCssSource).toContain("grid-template-columns: minmax(0, 1fr);");- expect(therapyCssSource).toContain(".tc-root .tc-mobile-grid-2");- expect(therapyCssSource).toContain(".tc-root .tc-mobile-static");- expect(therapyCssSource).toContain(".tc-root .tc-compare-table");- expect(therapyCssSource).toContain("overflow-x: auto;");+ // Mobile overrides rely on `utilities` being declared after `components`+ // now that `!important` has been removed.+ const componentsAt = therapyCssSource.indexOf("`@layer` components");+ const utilitiesAt = therapyCssSource.indexOf("`@layer` utilities");+ expect(componentsAt).toBeGreaterThanOrEqual(0);+ expect(utilitiesAt).toBeGreaterThan(componentsAt);++ const mobileStackRule =+ therapyCssSource.match(/\.tc-root \.tc-mobile-stack\s*\{([^}]*)\}/)?.[1] ?? "";+ expect(mobileStackRule).toContain("grid-template-columns: minmax(0, 1fr);");+ expect(mobileStackRule).not.toContain("!important");++ const compareTableRule =+ therapyCssSource.match(/\.tc-root \.tc-compare-table\s*\{([^}]*)\}/)?.[1] ?? "";+ expect(compareTableRule).toContain("overflow-x: auto;");+ expect(compareTableRule).not.toContain("!important");++ expect(therapyCssSource).toContain(".tc-root .tc-mobile-grid-2");+ expect(therapyCssSource).toContain(".tc-root .tc-mobile-static");Note the .tc-mobile-stack / .tc-compare-table regexes must account for those selectors appearing in comma-separated selector lists (Lines 959-962 and 993-999); adjust the pattern if the grouped form is kept.
🤖 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/therapy-compass-responsive-contract.test.ts` around lines 88 - 92,
Update the responsive contract assertions around the therapy CSS source to
extract and validate the `@media` (max-width: 640px) block, rather than matching
rules across the whole file. Ensure grouped .tc-mobile-stack and
.tc-compare-table selectors are matched when comma-separated, and add an
assertion that the utilities layer is declared after the components layer so
mobile overrides retain their cascade precedence.
Closing without merge after the 2026-07-27 open-PR review + Bugbot sweep. Why: Tip is not merge-ready: GitGuardian FAIL, PR-policy missing Clinical Governance Preflight, and the parallel Useful thin deltas (therapy-compass |
System Audit Remediation
This PR implements the remediation steps identified in the system audit report.
Fixes
Note on Improvements
Summary by CodeRabbit
Bug Fixes
Performance & Reliability
Documentation
Testing