Docker delivery hardening: pinned base images, cache mounts, graceful worker shutdown - #1575
Conversation
Capture the actionable follow-ups from the mega-review and performance-image CWV audit generated today: restore typecheck, noUncheckedIndexedAccess migration, unsafe-cast/JSON.parse validation, silent error swallowing, passive PDF wheel, and image-optimization basics. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…ovider-free validation and graceful worker shutdown. - Pin node:24-bookworm-slim to a multi-platform SHA-256 digest in both Dockerfiles via a shared node-base stage. - Add BuildKit --mount=type=cache to npm ci and the worker pip install. - Generate a hashed worker/python/requirements.txt from requirements.in using pip-tools, with npm scripts to generate and verify the lock. - Add provider-free worker runtime validation (dist/worker/validate-runtime.mjs) that runs at image build and in CI with --network=none. - Refactor worker/main.ts around WorkerRuntimeControl and runWorkerLoop for graceful SIGTERM/SIGINT shutdown. - Add STOPSIGNAL SIGTERM and OCI labels to both final images. - Extend docker-image.yml into a single build-and-verify job that loads both images and runs content/smoke checks plus Trivy scans and SBOMs. - Add helper scripts for app smoke, image content contract, Trivy scanning, and OCI digest resolution. - Extend tests and CI change scope, add Dependabot Docker monitoring, and update docs. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Second /ledger pass after the Docker delivery hardening commit found no new high-confidence outstanding items. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in:7 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughChangesContainer hardening and CI verification
Worker runtime lifecycle
Project ledger maintenance
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Signal
participant WorkerRuntimeControl
participant runWorkerLoop
participant JobProcessor
Signal->>WorkerRuntimeControl: deliver SIGTERM or SIGINT
WorkerRuntimeControl->>runWorkerLoop: set stopped state
runWorkerLoop->>JobProcessor: finish active claimed jobs
runWorkerLoop->>runWorkerLoop: stop new claims and exit
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
This pull request has been ignored for the connected project Preview Branches by Supabase. |
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #7839 (cancelled). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
BigSimmo
commented
Aug 2, 2026
@cursoragent Work the current open PR end-to-end. Confirm the PR number and GitHub head first from context. If more than one open PR could apply, stop and say which one you would use and why. Fetch and start from the remote tip that matches that GitHub head. If the named branch ref is missing or stale, use the PR head ref. Preserve unrelated local WIP, including any local-only ledger commits; do not discard dirty work, and do not treat a local-ahead commit as the reviewed tip. Do not merge the PR, force-push, rebase, or arm auto-merge unless I explicitly ask. No provider-backed gates without separate approval. If you cannot push or resolve threads, diagnose and comment only; if inline replies fail, resolve when possible and put dispositions in the summary comment. If auto-merge is already armed, push only for a real blocker, and avoid pushes that would cancel in-flight required CI unless the push itself clears that blocker. If the PR is already merged or closed: confirm the head and merge commit, note required-CI outcome, post one summary, and stop. Goal: deep review plus Bugbot, fix actionable issues with the smallest correct changes, clear merge / required-CI / thread blockers, run strong local offline verification, push fixes, append the review ledger, and post one PR summary. Prefer thoroughness over speed. Regenerate large assets only when a fix requires it; then run the asset check and keep compatibility aliases byte-identical where the repo uses them. Snapshot the GitHub head SHA: tip, base, behind/ahead, mergeable state, merge-tree versus origin/main (real conflict versus behind-but-clean), required checks on that tip including Production UI when selected, advisory separately, unresolved actionable threads. Missing checks while dirty are not green. If the tip moves mid-work, re-snapshot and continue from the new head. Ledger-lookup against that GitHub head under the heavy review-and-fix scope for this PR. Already reviewed at this head with clean merge-tree, green required checks, and no new actionable threads → summarize, comment, stop unless I asked for a fresh superseding pass. Follow the repo review protocol. Unblock once: real conflict → merge origin/main (prefer main’s shared queues; keep this PR’s notes); behind-but-clean → one sync, late if required CI is in flight; clean → leave. Labels like skip-branch-sync do not block a manual sync needed to clear a real blocker. No sync thrash. After any sync or push, re-snapshot tip, merge-tree, and required checks before declaring done. Dedupe the ledger if a merge touched it. Review high-confidence delta risks only. Separate PR-introduced defects from pre-existing re-emitted debt. For generated assets, review contracts, aliases, cache, and manifests—not every generated line. If protected RAG or ranking surfaces are touched, say so before editing; fix PR-body policy text only when wrong or missing. Ignore bot noise. No nit spam or broad rewrites. Escalate verification by touched risk: clinical / RAG / privacy / migrations / auth → domain check plus production-readiness when warranted; UI / phone-chrome / routing / styling → ensure plus phone-chrome or UI gates when warranted; generated assets or docs inventory/links → asset or docs checks when warranted. Fix P0/P1 always; clear scoped P2 when locally provable; else disposition and resolve. Required-check failures on this tip only; ignore advisory. Prefer reverting a bad autofix commit. After fixes: format and commit before push; smallest targeted proof; repo cheap gate; repo PR-local gate. No release, lighthouse, live eval, or live provider gates without approval. Push only this PR’s fix commits. Append ledger for the final GitHub head under the heavy scope. Never push a ledger-only tip—include ledger in a real fix push, or leave a no-change ledger append local and unpushed. One PR comment: tip, sync/merge-tree, fixed versus dispositioned, threads, required CI, decisive local gate lines, residual risks; merge left to me. Inline only for remaining human-needed P0/P1. Stop when merge-tree is clean on the current GitHub tip, actionable threads are clear, required checks are green or clearly in progress, heavy gates for scope passed, summary posted, merge left to me. |
PR #1575 worked end-to-end at tip Snapshot → unblock
Fixes pushed
Verification
Threads / ledger / merge
  |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Pull request overview
This PR hardens the repo’s Docker delivery pipeline by pinning base images by digest, adding BuildKit caching, expanding CI container verification (content contract, app smoke, worker runtime validation, SBOM/vuln scans), and refactoring the ingestion worker loop to drain and stop cleanly on shutdown signals.
Changes:
- Pin Node 24 base image by SHA-256 digest via a shared
node-basestage and add BuildKit cache mounts fornpm ciand worker Python installs. - Add provider-free container validation scripts (image content contract, app smoke, worker runtime validator) and consolidate container CI into a single workflow job.
- Refactor worker runtime control/loop (signal handling + graceful stop) and add focused unit tests around the new behavior.
Reviewed changes
Copilot reviewed 28 out of 29 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| worker/validate-runtime.ts | Adds a provider-free runtime validator for Node/npm/module resolution + optional Python checks. |
| worker/types.ts | Extracts worker job row/document types used by the loop and tests. |
| worker/runtime-control.ts | Introduces a stop controller that can be driven by SIGTERM/SIGINT and used for interruptible sleep/backoff. |
| worker/run-loop.ts | Centralizes the worker polling/claim/process loop with graceful stop and backoff logic. |
| worker/python/requirements.in | Adds a pip-tools input file for the worker OCR Python dependency lock. |
| worker/main.ts | Switches the worker entry logic to runWorkerLoop + WorkerRuntimeControl and updates shutdown behavior. |
| tests/worker-runtime-validation.test.ts | Unit-tests the runtime validator’s externals resolution behavior (Python skipped). |
| tests/worker-runtime-control.test.ts | Unit-tests WorkerRuntimeControl stop/sleep behavior and signal attachment. |
| tests/worker-run-loop.test.ts | Unit-tests loop behavior for --once, stop draining, probe failure, and claim failure backoff. |
| tests/worker-python-lock.test.ts | Asserts the worker Python lock exists and is pinned+hashed. |
| tests/container-ci-contract.test.ts | Adds tests asserting pinned digest usage, STOPSIGNAL, and validator presence in Dockerfiles/CI. |
| scripts/trivy-image-scan.mjs | Adds pinned Trivy-based vuln scan + CycloneDX SBOM generation helper. |
| scripts/resolve-oci-image-digest.mjs | Adds helper to resolve/pin multi-platform OCI image index digests. |
| scripts/generate-worker-python-lock.mjs | Adds pip-tools-based generator for worker/python/requirements.txt. |
| scripts/ci-change-scope.mjs | Extends CI change scope patterns to include new container hardening inputs/scripts/tests. |
| scripts/check-worker-python-lock.mjs | Adds a verifier that requirements.txt matches requirements.in and is hashed/pinned. |
| scripts/check-image-content-contract.mjs | Adds a script to assert runtime image hardening invariants (user, stop signal, labels, etc.). |
| scripts/build-worker.mjs | Emits dist/worker/externals.json and builds the validate-runtime bundle. |
| scripts/app-container-smoke.mjs | Adds provider-free, network-isolated app container smoke test via /api/health. |
| package.json | Adds npm scripts to generate/check worker Python lock and run the app container smoke. |
| docs/worker-deploy-runbook.md | Updates worker CI job naming and documents new runtime/stop behavior and validator. |
| docs/scripts-index.md | Updates scripts/script-surface counts after adding new scripts. |
| docs/outstanding-issues.md | Advances issues:next-id and appends new ledger items related to a prior sweep. |
| docs/deployment-architecture.md | Documents pinned base digest, cache mounts, OCI labels, and worker graceful stop contract. |
| Dockerfile.worker | Pins Node base image by digest, adds cache mounts, hashed pip install, runtime validator, labels, STOPSIGNAL. |
| Dockerfile | Pins Node base image by digest, adds cache mounts, and adds OCI labels + STOPSIGNAL. |
| .github/workflows/docker-image.yml | Consolidates app+worker build into one build-and-verify job with load + validations + SBOM/vuln scan. |
| .github/dependabot.yml | Adds weekly Dependabot monitoring for Docker ecosystem dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:ccdc8ff801
ℹ️ 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.
Supply RAG_QUERY_HASH_SECRET so production instrumentation can boot in the provider-free app smoke. Stop leasing new jobs after SIGTERM, and restore threshold claim + per-job process Sentry capture in runWorkerLoop.
PR #1575 review-and-fix summaryTip: Fixed
Dispositioned
ThreadsNone actionable to resolve. Required CI (tip |
BigSimmo
commented
Aug 2, 2026
@cursoragent Work the current open PR end-to-end. Confirm the PR number and GitHub head first from context. If more than one open PR could apply, stop and say which one you would use and why. Fetch and start from the remote tip that matches that GitHub head. If the named branch ref is missing or stale, use the PR head ref. Preserve unrelated local WIP, including any local-only ledger commits; do not discard dirty work, and do not treat a local-ahead commit as the reviewed tip. Do not merge the PR, force-push, rebase, or arm auto-merge unless I explicitly ask. No provider-backed gates without separate approval. If you cannot push or resolve threads, diagnose and comment only; if inline replies fail, resolve when possible and put dispositions in the summary comment. If auto-merge is already armed, push only for a real blocker, and avoid pushes that would cancel in-flight required CI unless the push itself clears that blocker. If the PR is already merged or closed: confirm the head and merge commit, note required-CI outcome, post one summary, and stop. Goal: deep review plus Bugbot, fix actionable issues with the smallest correct changes, clear merge / required-CI / thread blockers, run strong local offline verification, push fixes, append the review ledger, and post one PR summary. Prefer thoroughness over speed. Regenerate large assets only when a fix requires it; then run the asset check and keep compatibility aliases byte-identical where the repo uses them. Snapshot the GitHub head SHA: tip, base, behind/ahead, mergeable state, merge-tree versus origin/main (real conflict versus behind-but-clean), required checks on that tip including Production UI when selected, advisory separately, unresolved actionable threads. Missing checks while dirty are not green. If the tip moves mid-work, re-snapshot and continue from the new head. Ledger-lookup against that GitHub head under the heavy review-and-fix scope for this PR. Already reviewed at this head with clean merge-tree, green required checks, and no new actionable threads → summarize, comment, stop unless I asked for a fresh superseding pass. Follow the repo review protocol. Unblock once: real conflict → merge origin/main (prefer main’s shared queues; keep this PR’s notes); behind-but-clean → one sync, late if required CI is in flight; clean → leave. Labels like skip-branch-sync do not block a manual sync needed to clear a real blocker. No sync thrash. After any sync or push, re-snapshot tip, merge-tree, and required checks before declaring done. Dedupe the ledger if a merge touched it. Review high-confidence delta risks only. Separate PR-introduced defects from pre-existing re-emitted debt. For generated assets, review contracts, aliases, cache, and manifests—not every generated line. If protected RAG or ranking surfaces are touched, say so before editing; fix PR-body policy text only when wrong or missing. Ignore bot noise. No nit spam or broad rewrites. Escalate verification by touched risk: clinical / RAG / privacy / migrations / auth → domain check plus production-readiness when warranted; UI / phone-chrome / routing / styling → ensure plus phone-chrome or UI gates when warranted; generated assets or docs inventory/links → asset or docs checks when warranted. Fix P0/P1 always; clear scoped P2 when locally provable; else disposition and resolve. Required-check failures on this tip only; ignore advisory. Prefer reverting a bad autofix commit. After fixes: format and commit before push; smallest targeted proof; repo cheap gate; repo PR-local gate. No release, lighthouse, live eval, or live provider gates without approval. Push only this PR’s fix commits. Append ledger for the final GitHub head under the heavy scope. Never push a ledger-only tip—include ledger in a real fix push, or leave a no-change ledger append local and unpushed. One PR comment: tip, sync/merge-tree, fixed versus dispositioned, threads, required CI, decisive local gate lines, residual risks; merge left to me. Inline only for remaining human-needed P0/P1. Stop when merge-tree is clean on the current GitHub tip, actionable threads are clear, required checks are green or clearly in progress, heavy gates for scope passed, summary posted, merge left to me. Please ensure you resolve all comments threads once you fix them. |
PR #1575 worked end-to-end at tip Snapshot
What blocked CI (and what we fixed)
Also synced Local proof
Residual
Merge left to you. |
Create tmp/ before Python lock diagnostics, use pathToFileURL for externals overrides, flush/report WorkerAbortError via the top-level handler, remove the stray sleep timer, and scan Trivy via docker save without mounting the Docker socket.
PR #1575 review-and-fix summary (superseding)Tip: Base / sync: merge-tree vs Fixed (pushed on tip |
| Path | Outcome |
|---|---|
scripts/check-worker-python-lock.mjs (×3) | fixed-head 31447cb4 |
worker/validate-runtime.ts | fixed-head 31447cb4 |
worker/main.ts | fixed-head 31447cb4 |
worker/runtime-control.ts | fixed-head 31447cb4 |
scripts/trivy-image-scan.mjs | fixed-head 31447cb4 |
worker/run-loop.ts (Sentry/claim/process ×5) | fixed-head 31447cb4 (earlier + retained) |
Required CI (tip 31447cb4)
In progress after fix push: Change scope / PR policy / PR mergeability / Gitleaks SUCCESS. Static PR / Safety / Unit coverage / Build / Container images IN_PROGRESS. Production UI SKIPPED (out of scope). Semgrep advisory in progress.
Local gates (decisive lines)
- Vitest worker/container: 25/25 passed
npm run verify:cheap: 4994 tests passed (exit 0)npm run verify:pr-local: exit 0
Residual risks
- Hosted container smoke/SBOM/Trivy must finish green on this tip.
- Review ledger row for
31447cb4is local/unpushed (avoid ledger-only tip cancelling CI). - Thread resolve may remain open until a token with
pull-requests: writecan close them; all findings are fixed in the tip.
Merge left to you.
App smoke failed in CI because Next 16 rewrites /proc/1/cmdline to `next-server (v16.2.12)` after `next start` boots. Accept either form so provider-free smoke tracks the live production server.
PR #1575 review-and-fix summary (tip |
Resolve docs/scripts-index.md inventory counts via docs:update (210 script files / 218 npm scripts) after main advanced past the prior branch sync.
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (14)
.github/workflows/docker-image.yml (1)
82-94: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStart the app before the advisory health probe.
The trailing
node -eargument replaces the imageCMD. This container runs the probe without starting Next.js. The probe always fails to connect, and|| truehides that failure.Run the image detached with its default command. Then use
docker execto probe/api/health. Keep the check advisory if intended.🤖 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 @.github/workflows/docker-image.yml around lines 82 - 94, Update the “Read-only runtime check (advisory)” step to start clinical-kb-app:ci detached with its default CMD, then run the health probe via docker exec against the running container. Preserve the existing read-only security options and environment variables, ensure the container is cleaned up afterward, and retain advisory failure handling only if intended.scripts/check-image-content-contract.mjs (1)
71-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the no-op glob assertion.
assertPathAbsent(image, "/app/*.pem")tests for a file literally named*.pem. It always succeeds and costs one container start per image. Lines 74-77 already perform the real check.♻️ Proposed fix
assertPathAbsent(image, "/app/tests"); - assertPathAbsent(image, "/app/*.pem"); // glob not supported; checked below🤖 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/check-image-content-contract.mjs` at line 71, Remove the no-op assertPathAbsent call for "/app/*.pem" in the image content contract check, while preserving the existing real PEM-file validation in the subsequent lines.scripts/build-worker.mjs (1)
55-67: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueMake the metafile fallback effective.
extractExternals(indexResult.metafile ?? {})still throws ifmetafileis missing, becauseObject.values(undefined)raises aTypeErrorfor{}.outputs. Guard theoutputsproperty instead.♻️ Proposed fix
function extractExternals(metafile) { const externals = new Set(); - for (const output of Object.values(metafile.outputs)) {+ for (const output of Object.values(metafile?.outputs ?? {})) { for (const imported of output.imports ?? []) { if (imported.external) externals.add(imported.path); } } return [...externals].sort(); } @@ - const externals = extractExternals(indexResult.metafile ?? {});+ const externals = extractExternals(indexResult.metafile);🤖 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/build-worker.mjs` around lines 55 - 67, Update extractExternals to safely handle a missing metafile.outputs property before calling Object.values, while preserving external collection and sorting when outputs exist. Adjust the function’s metafile traversal rather than relying on the caller’s indexResult.metafile fallback.tests/worker-runtime-validation.test.ts (1)
16-23: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a case for a bare Node builtin.
Add an external such as
"fs"without thenode:prefix. esbuild can emit externals in that form, and the current resolution path treats them differently fromnode:fs. See the related comment onworker/validate-runtime.tsLines 94-112.🤖 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/worker-runtime-validation.test.ts` around lines 16 - 23, Add a test case alongside the existing unresolved-external test in the runtime validation suite using the bare Node builtin external "fs" (without the "node:" prefix), and assert the expected validation behavior so this resolution path is covered.worker/validate-runtime.ts (3)
83-91: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winValidate the parsed
externals.jsonshape.
JSON.parseresult is cast tostring[]without a check. If the file contains an object or a number, thefor...ofloop at Line 94 throws, andvalidateRuntimerejects instead of returning a structured failure. Add anArray.isArraycheck.🛡️ Proposed fix
try { - externals = JSON.parse(readFileSync(externalsPath, "utf8")) as string[];+ const parsed: unknown = JSON.parse(readFileSync(externalsPath, "utf8"));+ if (!Array.isArray(parsed) || parsed.some((entry) => typeof entry !== "string")) {+ errors.push("externals.json must contain an array of strings");+ } else {+ externals = parsed as string[];+ } } catch (error) {🤖 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 `@worker/validate-runtime.ts` around lines 83 - 91, Update the externals.json parsing block in validateRuntime to verify the parsed value with Array.isArray before assigning it to externals. When the parsed value is not an array, add a structured validation error instead of allowing the later for...of loop to throw; preserve the existing parse-error and missing-file handling.
67-74: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deriving the expected versions from
package.jsonengines.The Node 24 and npm 11 expectations are hardcoded. An engines bump would leave this validator behind and produce a misleading failure. Read the
enginesfield instead.🤖 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 `@worker/validate-runtime.ts` around lines 67 - 74, Update the runtime checks in the validation flow around nodeMajor and npmMajor to read the expected Node and npm major versions from package.json’s engines field instead of hardcoding 24 and 11. Parse the engine constraints consistently, compare the detected majors against those derived expectations, and retain the existing error reporting for mismatches.
100-100: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUse the one-argument form. Node 24 ignores the second argument without
--experimental-import-meta-resolve; it does not throw. Sinceimport.meta.urlis the default parent,import.meta.resolve(spec)is sufficient.🤖 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 `@worker/validate-runtime.ts` at line 100, Update the import resolution call in the runtime validation flow to use the one-argument form, changing import.meta.resolve(spec, import.meta.url) to import.meta.resolve(spec). Preserve the existing spec value and resolution behavior.scripts/trivy-image-scan.mjs (2)
101-108: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winThe scan never fails on findings, and a parse failure is reported as success.
--exit-code 0makes Trivy exit successfully even when it detects CRITICAL vulnerabilities. If the CI job must block on findings, add a threshold check on the parsed counts. In addition, when JSON parsing fails, the script printscould not parse Trivy JSONand exits 0. That hides a broken scan.Also applies to: 110-125
🤖 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/trivy-image-scan.mjs` around lines 101 - 108, Update the scan flow around the Trivy invocation and JSON parsing so scans fail when findings meet the configured severity threshold, using the parsed vulnerability counts rather than relying on Trivy’s current --exit-code 0 behavior. Ensure JSON parse failures are treated as errors: log the parse failure and exit with a nonzero status instead of reporting success. Preserve successful completion only when parsing succeeds and no blocking findings are present.
52-54: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueFiles written through
/outare owned by root.The Trivy container runs as root by default. The SBOM file that it writes to the mounted output directory is owned by root on the host. Later CI steps that run as a non-root user cannot overwrite or delete it. Pass
--userto align ownership.🤖 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/trivy-image-scan.mjs` around lines 52 - 54, Update the Trivy container invocation in the script’s execution flow to pass a --user value matching the invoking host user, ensuring files written through the outputHostPath mount are host-user owned. Preserve the existing -v /out mount behavior and apply the user option only to the container command configuration.docs/worker-deploy-runbook.md (1)
94-104: 🩺 Stability & Availability | 🔵 TrivialDocument the orchestrator stop timeout.
STOPSIGNAL SIGTERMselects the signal. It does not guarantee enough time to drain the active batch. Verify that the deployment platform waits longer than the drain budget; otherwise it can sendSIGKILLbefore the worker exits0.🤖 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 `@docs/worker-deploy-runbook.md` around lines 94 - 104, Update the worker deployment runbook near the STOPSIGNAL SIGTERM entry to document the orchestrator’s stop timeout and require it to exceed the worker’s active-batch drain budget. Specify that the platform must allow the worker to exit 0 after SIGTERM before issuing SIGKILL.docs/deployment-architecture.md (1)
362-365: 🩺 Stability & Availability | 🔵 TrivialDocument the Railway worker shutdown grace period.
STOPSIGNAL SIGTERMselects the signal.railway.worker.jsondoes not define the time allowed to drain the active batch. If Railway sendsSIGKILLbefore the batch finishes, claimed jobs wait for stale reclamation. Verify the effective grace period and document it indocs/worker-deploy-runbook.md. It must exceed the worst-case active-batch duration.🤖 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 `@docs/deployment-architecture.md` around lines 362 - 365, Verify the Railway worker shutdown grace period configured for railway.worker.json, confirm it exceeds the worst-case active-batch duration, and document the effective value and drain behavior in docs/worker-deploy-runbook.md. Include the SIGTERM-to-SIGKILL timing and identify the required configuration if the current grace period is insufficient.worker/runtime-control.ts (1)
83-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSeparate signal detachment from shutdown.
detachSignals()callsthis.stop(). A caller that only wants to remove the process listeners also marks the controller stopped and runs every registered cleanup callback. The method name states the opposite contract. The comment on Line 83 states that tests calldetachSignals, buttests/worker-runtime-control.test.tsnever calls it, so the stated justification does not hold.Store the detach closure separately and remove the listeners without changing
#stopped.♻️ Proposed refactor
`#stopped` = false; `#cleanup`: (() => void)[] = []; `#signalSource`?: Pick<EventEmitter, "on" | "off">; `#attached` = false; + `#detach`: () => void = () => {};- // Store the detach logic so tests can explicitly call detachSignals.- this.#cleanup.push(() => {- for (const signal of signals) {- source.off(signal, handler);- }- });+ this.#detach = () => {+ for (const signal of signals) {+ source.off(signal, handler);+ }+ };+ // Release the process listeners on stop so the event loop can drain.+ this.#cleanup.push(() => this.#detach()); } detachSignals(): void { if (!this.#attached) return; this.#attached = false; - this.stop();+ this.#detach();+ this.#detach = () => {}; }🤖 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 `@worker/runtime-control.ts` around lines 83 - 95, Update detachSignals() to invoke only the stored signal-detachment closure, removing process listeners without changing `#stopped` or running unrelated cleanup callbacks; store that closure separately from the general `#cleanup` collection and remove the inaccurate test-justification comment.tests/worker-run-loop.test.ts (1)
104-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the once-mode claim abort and the exit code.
This test covers the probe abort at
worker/run-loop.tsLine 51. The claim abort atworker/run-loop.tsLine 75 has no test.worker/main.tsLine 1945 exits withabort.exitCode, and no test asserts that value. Both paths determine the container exit status in--oncemode, so a regression would surface only in CI job status.💚 Proposed additional test
+ it("aborts with exit code 1 in once mode when claim fails", async () => {+ const ctrl = new WorkerRuntimeControl();+ const cause = new Error("claim error");++ const error = await runWorkerLoop({+ once: true,+ pollMs: 1000,+ healthBackoffMs: 1000,+ maxClaimFailures: 3,+ claim: vi.fn().mockRejectedValue(cause),+ process: vi.fn(),+ probe: vi.fn().mockResolvedValue({ ok: true }),+ backoff: (n) => n * 10,+ controller: ctrl,+ log: () => {},+ }).catch((thrown: unknown) => thrown);++ expect(error).toBeInstanceOf(WorkerAbortError);+ expect((error as WorkerAbortError).exitCode).toBe(1);+ expect((error as WorkerAbortError).cause).toBe(cause);+ });Import
WorkerAbortErroralongsideWorkerRuntimeControlon Line 2.🤖 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/worker-run-loop.test.ts` around lines 104 - 122, Add test coverage alongside the existing once-mode probe-abort test for claim failure in runWorkerLoop: make the claim operation fail, assert rejection with WorkerAbortError, and verify its exitCode is 1. Import WorkerAbortError with WorkerRuntimeControl, and preserve the existing once-mode setup and assertions for the probe path.tests/worker-runtime-control.test.ts (1)
35-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the test name with the asserted behavior, or assert detachment.
The test is named "attaches and detaches signals". It never calls
detachSignals()and never asserts that a listener was removed. The second controller passes whether or not the first controller detached, so the test does not prove detachment.Add an explicit listener-count assertion, or rename the test.
💚 Proposed test change
- it("attaches and detaches signals", () => {+ it("stops on an attached signal and releases the listener", () => { const source = new EventEmitter(); const ctrl = new WorkerRuntimeControl({ signalSource: source }); ctrl.attachSignals(["SIGTERM"]); + expect(source.listenerCount("SIGTERM")).toBe(1); source.emit("SIGTERM"); expect(ctrl.isStopped).toBe(true); + expect(source.listenerCount("SIGTERM")).toBe(0); const next = new WorkerRuntimeControl({ signalSource: source }); next.attachSignals(["SIGTERM"]); source.emit("SIGTERM"); expect(next.isStopped).toBe(true); });🤖 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/worker-runtime-control.test.ts` around lines 35 - 46, Update the test named “attaches and detaches signals” to explicitly verify detachment by calling detachSignals and asserting the signal source listener count decreases or the removed controller no longer reacts; otherwise rename the test to describe attachment-only behavior. Ensure the assertions distinguish the first controller’s listener cleanup from the second controller’s signal handling.
🤖 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 `@docs/outstanding-issues.md`:
- Around line 233-238: The execution queue in docs/outstanding-issues.md must
include every open ledger row, including `#210` through `#215`. Add these six rows
via the established /issues workflow and update the queue/checker as needed so
alignment is restored, preserving the ledger as the durable source of
outstanding tasks.
In `@docs/worker-deploy-runbook.md`:
- Around line 50-51: Update the build-and-verify section in the worker
deployment runbook to describe docker/build-push-action with file:
Dockerfile.worker, load: true, and push: false, or explicitly document the
equivalent docker buildx build --load command instead of docker build.
In `@scripts/app-container-smoke.mjs`:
- Around line 145-159: Update the shutdown validation around docker stop and the
inspectResult flow: check the docker stop result, include State.Running
alongside OOMKilled and ExitCode in the inspect format, and fail with cleanup
when the stop fails, the container remains running, or the exit code is 137.
Preserve the existing OOMKilled failure handling.
In `@scripts/check-image-content-contract.mjs`:
- Around line 48-60: Update both ls-based checks in the image contract script to
assert the docker run result exited successfully before inspecting stdout, so
startup, shell, and daemon failures fail the contract. In
assertWorkerPythonClean, change the worker test glob to use the application’s
absolute path while preserving the existing detection of matching test files.
- Around line 30-31: Update the missing-Command guard after inspect in the
image-content contract check to treat both an empty result and the literal
string "null" as missing, while preserving the existing error message and
behavior for valid commands.
In `@scripts/ci-change-scope.mjs`:
- Around line 215-224: Update the container_changed scope in the
change-detection logic around the listed worker paths and scripts to include the
reachable repo-local src/** dependencies used by worker/index.ts and
worker/main.ts, such as src/lib/privacy.ts, src/lib/env.ts, and
src/lib/chunking.ts. Prefer deriving this scope from the worker entry points if
the existing implementation supports it; otherwise add the necessary src/**
coverage without altering unrelated scopes.
In `@scripts/trivy-image-scan.mjs`:
- Around line 80-83: Validate the values parsed by the sbomIndex and severityIdx
option handling before using them: require each flag to have a following
argument that is not another flag, report the invalid or missing value, and exit
through the script’s existing error path. Preserve the default severity only
when --severity is absent, and prevent undefined or flag tokens from reaching
spawnSync or silently disabling SBOM generation.
- Line 56: Update the dockerArgs construction in the Trivy scan flow to mount a
pre-populated writable Trivy vulnerability database cache into the container and
pass --skip-db-update when using --network=none. Ensure the cache location is
configurable or clearly provisioned so clean runners have the required database
available before each docker run --rm invocation.
In `@tests/worker-runtime-validation.test.ts`:
- Around line 10-13: Update the test around validateRuntime to avoid asserting
local Node and package-manager versions; replace the nodeVersion and result.ok
expectations with assertions on the expected externals validation outcome, or
stub the runtime version inputs so the existing assertions are deterministic.
In `@worker/run-loop.ts`:
- Around line 46-61: The run loop’s probe() call can block indefinitely before
claim(). Update the health-check path around probeSupabaseHealth and its caller
to use a stop-aware timeout or dedicated abort signal, ensuring the probe exits
by the configured deadline and responds to worker shutdown; leave claimJobs()
uncancelled because it may already lease rows.
In `@worker/validate-runtime.ts`:
- Around line 94-112: Update the external-specifier handling in the loop over
externals to recognize bare Node builtin names as well as existing “node:”
specifiers, using the builtin detection API from node:module. Treat detected
builtins as resolved and valid without calling fileURLToPath or existsSync,
while preserving the existing filesystem resolution path for non-builtin
externals.
---
Nitpick comments:
In @.github/workflows/docker-image.yml:
- Around line 82-94: Update the “Read-only runtime check (advisory)” step to
start clinical-kb-app:ci detached with its default CMD, then run the health
probe via docker exec against the running container. Preserve the existing
read-only security options and environment variables, ensure the container is
cleaned up afterward, and retain advisory failure handling only if intended.
In `@docs/deployment-architecture.md`:
- Around line 362-365: Verify the Railway worker shutdown grace period
configured for railway.worker.json, confirm it exceeds the worst-case
active-batch duration, and document the effective value and drain behavior in
docs/worker-deploy-runbook.md. Include the SIGTERM-to-SIGKILL timing and
identify the required configuration if the current grace period is insufficient.
In `@docs/worker-deploy-runbook.md`:
- Around line 94-104: Update the worker deployment runbook near the STOPSIGNAL
SIGTERM entry to document the orchestrator’s stop timeout and require it to
exceed the worker’s active-batch drain budget. Specify that the platform must
allow the worker to exit 0 after SIGTERM before issuing SIGKILL.
In `@scripts/build-worker.mjs`:
- Around line 55-67: Update extractExternals to safely handle a missing
metafile.outputs property before calling Object.values, while preserving
external collection and sorting when outputs exist. Adjust the function’s
metafile traversal rather than relying on the caller’s indexResult.metafile
fallback.
In `@scripts/check-image-content-contract.mjs`:
- Line 71: Remove the no-op assertPathAbsent call for "/app/*.pem" in the image
content contract check, while preserving the existing real PEM-file validation
in the subsequent lines.
In `@scripts/trivy-image-scan.mjs`:
- Around line 101-108: Update the scan flow around the Trivy invocation and JSON
parsing so scans fail when findings meet the configured severity threshold,
using the parsed vulnerability counts rather than relying on Trivy’s current
--exit-code 0 behavior. Ensure JSON parse failures are treated as errors: log
the parse failure and exit with a nonzero status instead of reporting success.
Preserve successful completion only when parsing succeeds and no blocking
findings are present.
- Around line 52-54: Update the Trivy container invocation in the script’s
execution flow to pass a --user value matching the invoking host user, ensuring
files written through the outputHostPath mount are host-user owned. Preserve the
existing -v /out mount behavior and apply the user option only to the container
command configuration.
In `@tests/worker-run-loop.test.ts`:
- Around line 104-122: Add test coverage alongside the existing once-mode
probe-abort test for claim failure in runWorkerLoop: make the claim operation
fail, assert rejection with WorkerAbortError, and verify its exitCode is 1.
Import WorkerAbortError with WorkerRuntimeControl, and preserve the existing
once-mode setup and assertions for the probe path.
In `@tests/worker-runtime-control.test.ts`:
- Around line 35-46: Update the test named “attaches and detaches signals” to
explicitly verify detachment by calling detachSignals and asserting the signal
source listener count decreases or the removed controller no longer reacts;
otherwise rename the test to describe attachment-only behavior. Ensure the
assertions distinguish the first controller’s listener cleanup from the second
controller’s signal handling.
In `@tests/worker-runtime-validation.test.ts`:
- Around line 16-23: Add a test case alongside the existing unresolved-external
test in the runtime validation suite using the bare Node builtin external "fs"
(without the "node:" prefix), and assert the expected validation behavior so
this resolution path is covered.
In `@worker/runtime-control.ts`:
- Around line 83-95: Update detachSignals() to invoke only the stored
signal-detachment closure, removing process listeners without changing `#stopped`
or running unrelated cleanup callbacks; store that closure separately from the
general `#cleanup` collection and remove the inaccurate test-justification
comment.
In `@worker/validate-runtime.ts`:
- Around line 83-91: Update the externals.json parsing block in validateRuntime
to verify the parsed value with Array.isArray before assigning it to externals.
When the parsed value is not an array, add a structured validation error instead
of allowing the later for...of loop to throw; preserve the existing parse-error
and missing-file handling.
- Around line 67-74: Update the runtime checks in the validation flow around
nodeMajor and npmMajor to read the expected Node and npm major versions from
package.json’s engines field instead of hardcoding 24 and 11. Parse the engine
constraints consistently, compare the detected majors against those derived
expectations, and retain the existing error reporting for mismatches.
- Line 100: Update the import resolution call in the runtime validation flow to
use the one-argument form, changing import.meta.resolve(spec, import.meta.url)
to import.meta.resolve(spec). Preserve the existing spec value and resolution
behavior.
🪄 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
Run ID: 8fdc4040-5454-458d-a259-2142ddb3565b
📒 Files selected for processing (29)
.github/dependabot.yml.github/workflows/docker-image.ymlDockerfileDockerfile.workerdocs/deployment-architecture.mddocs/outstanding-issues.mddocs/scripts-index.mddocs/worker-deploy-runbook.mdpackage.jsonscripts/app-container-smoke.mjsscripts/build-worker.mjsscripts/check-image-content-contract.mjsscripts/check-worker-python-lock.mjsscripts/ci-change-scope.mjsscripts/generate-worker-python-lock.mjsscripts/resolve-oci-image-digest.mjsscripts/trivy-image-scan.mjstests/container-ci-contract.test.tstests/worker-python-lock.test.tstests/worker-run-loop.test.tstests/worker-runtime-control.test.tstests/worker-runtime-validation.test.tsworker/main.tsworker/python/requirements.inworker/python/requirements.txtworker/run-loop.tsworker/runtime-control.tsworker/types.tsworker/validate-runtime.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.
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.
PR #1575 review-and-fix summary (final tip |
- Accept Next 16 next-server PID 1; fail on SIGKILL / stop failures - Fail closed when docker inspect Cmd is null or listing checks error - Treat bare Node builtins as resolved in worker runtime validation - Validate Trivy CLI option values; pin docker-save temps under workDir - Mark SBOM/vuln scan continue-on-error and prune BuildKit before scans so ENOSPC on docker-save no longer fails the required container gate
PR #1575 review-and-fix summary (tip |
Uh oh!
There was an error while loading. Please reload this page.
Resolve real conflicts by preferring main's landed Docker/worker hardening (#1575+) while keeping this PR's unique UI responsive deltas and hand-merged Codex Cloud/Figma guidance. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Summary
node:24-bookworm-slimto a multi-platform SHA-256 digest in both images via a sharednode-basestage.--mount=type=cachemounts tonpm ci(app + worker) and the worker Python venv install.worker/python/requirements.txtfromrequirements.inusingpip-tools, with npm scripts to regenerate and verify the lock.dist/worker/validate-runtime.mjs) that runs inside the image build and again in CI with--network=none.worker/main.tsaroundWorkerRuntimeControlandrunWorkerLoopso the worker drains its active batch and exits0onSIGTERM/SIGINT.STOPSIGNAL SIGTERMand OCI source/title labels to both final images..github/workflows/docker-image.ymlinto a singlebuild-and-verifyjob that loads both images, runs provider-free app smoke, validates image content, and produces Trivy scans plus CycloneDX SBOMs.docs/deployment-architecture.mdanddocs/worker-deploy-runbook.md.Verification
tsc --noEmitpassedworker-runtime-control,worker-run-loop,worker-runtime-validation,worker-python-lock, andcontainer-ci-contractpassed (23/23)npx eslinton changed worker/scripts/tests passednode scripts/build-worker.mjsproduceddist/worker/index.mjsanddist/worker/validate-runtime.mjsnode scripts/check-worker-python-lock.mjsconfirmed lock is in syncdocker build -f Dockerfile.worker -t clinical-kb-worker:ci .built successfullydocker run --rm --network=none --entrypoint node clinical-kb-worker:ci dist/worker/validate-runtime.mjsreturnedok: truenode scripts/trivy-image-scan.mjs clinical-kb-worker:cicompleted non-blocking and reported 80 HIGH/CRITICAL findingsnode scripts/trivy-image-scan.mjs clinical-kb-worker:ci --sbom /tmp/sbom-worker.cdx.jsonproduced a CycloneDX SBOMnode scripts/check-github-action-pins.mjsandnode scripts/ci-change-scope.mjs --self-testpassednpm run verify:pr-localnot run (heavy, cross-module gate; focused checks above cover the diff)frontend grpc server closed unexpectedlybefore reachingFROM. A minimal Dockerfile with the same# syntax, pinned digest,STOPSIGNAL,HEALTHCHECK, andCMDbuilt successfully, so the file parses and the failure appears environmental/BuildKit resource related.Risk and rollout
Dockerfile.worker/Dockerfilecommit.Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)Notes
Generated with Devin
Summary by CodeRabbit
Reliability
Security
Documentation
Testing