Repository navigation
fix(ci): unbreak build — separate workerd test pool, correct phantom dep version - #132
Open
chitcommit wants to merge 3 commits into
Open
chitcommit wants to merge 3 commits into
chitcommit wants to merge 3 commits into
Conversation
…dep version The `build` job has been red on every PR opened or updated since the DO coordinator merge (c979f6a, Aug 2). The cause was not in any of those PRs. 1. vitest config overlap. vitest.config.ts globs `tests/**/*.test.ts`, which also matched tests/workers/coordinator.test.ts — a workerd-pool test that imports `cloudflare:test`. That specifier only resolves under @cloudflare/vitest-pool-workers (vitest.workers.config.mts), so the node suite died at import with ERR_MODULE_NOT_FOUND before running an assertion. Excluded tests/workers/** from the node suite, preserving vitest's default node_modules/dist excludes. 2. hono-agents "^0.19.0" never existed. a60d0e1 applied the agents-SDK version to the wrong package; the registry tops out at 3.0.12 and the lockfile already pins 3.0.7. Fresh `npm i` failed outright. Aligned the manifest to the locked version — the resolved version is still 3.0.7, so this is a manifest-only correction with no runtime change. 3. The workers suite ran nowhere in CI. `npm run test:workers` was never invoked by any workflow, so the DO coordinator tests had never executed. Excluding them from `npm test` would have converted "loudly broken" into "silently unrun", so they are now wired into the build job. Booting them required @cloudflare/vitest-pool-workers 0.22: the pinned 0.20.1 bundles a workerd supporting compatibility dates only through 2026-08-06, one day behind the 2026-08-07 date a60d0e1 set. Dev-only; the lockfile change is confined to that chain (pool-workers, miniflare, wrangler, workerd). Verified locally, full build sequence: tsc --noEmit clean; npm test 68 passed; npm run test:workers 24 passed (previously 0 — the suite could not boot). Not addressed here: the production advisories (hono, js-yaml, fast-uri, ip-address, nanoid) that fail the Dependency Audit gate are pre-existing on main and are what PR #128 already fixes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SpK3ddoHzUqgW3keYafHsD
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
|
To use Codex here, create a Codex account and connect to github. |
Contributor
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe PR updates worker testing dependencies, excludes worker tests from the Node Vitest suite, and adds a dedicated CI step to run them. ChangesWorker test isolation
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This PR makes localized CI and test-infrastructure changes and corrects a development dependency version; no actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Finding from separated adversarial review (claude-sonnet-4-5 via chittyclaw, reviewer != implementer): setting `exclude` replaces vitest's built-in default list wholesale instead of appending to it. The previous commit kept only node_modules and dist, silently dropping the cypress, dot-directory, and *.config.* patterns. Verified inert today — nothing under tests/ matches any dropped pattern, and `include` is scoped to tests/** so root config files were never candidates. It is restated anyway so a future file under e.g. tests/.cache/ cannot quietly re-enter the node suite. Also verified from the same review, empirically rather than by assertion: - `npm run test:workers` exits 1 on a broken config, so the new CI step is a real gate and not decorative. Checked by temporarily breaking the workers config and reading the exit code. - `npm ls @cloudflare/workers-types` reports no unmet peer dependency. wrangler 4.124 (pulled by pool-workers 0.22) resolves its own nested workers-types v5 while the project continues to resolve v4; tsc --noEmit passes against v4. Re-verified after this change: tsc clean, npm test 68 passed, test:workers 24 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SpK3ddoHzUqgW3keYafHsD
|
|
To use Codex here, create a Codex account and connect to github. |
Contributor
|
✅ Action performedReview finished.
|
`npm audit --audit-level=high --omit=dev` is the first step of the CI build job, so it gates every other check. It currently fails on `main` for reasons no PR introduced: npm audit is a moving gate, and advisories published after a branch last ran will fail it retroactively. hono 4.12.23 -> 4.13.3 is entirely inside the already-declared "^4.12.23", so this is a lockfile-only change with no manifest edit and no range widening. It clears 12 hono advisories, including CORS middleware reflecting any Origin with credentials when origin defaults to wildcard, hono/jsx cross-request context leakage, memo() retaining SSR output across requests, and ReDoS in the CORS and language middlewares. Production advisory count drops 7 -> 5. Validated after the bump: tsc --noEmit clean, npm test 68 passed, npm run test:workers 24 passed. The gate is still red on the remaining 4 (fast-uri, ip-address, js-yaml, nanoid). Those are transitive under agents/partyserver and each needs a forced MAJOR override (js-yaml 4->5, nanoid 5->6, fast-uri 3->4) pushed into packages that expect the older major. That is a distinct change with real silent-breakage risk and belongs in its own reviewed PR, not bundled into a CI fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SpK3ddoHzUqgW3keYafHsD
|
|
To use Codex here, create a Codex account and connect to github. |
Contributor
|
|
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs |
chittycommand | 13f61ca | Aug 18 2026, 11:21 PM |
chitcommit
added a commit
that referenced
this pull request
Oct 2, 2026
* fix(legal): matter data is config, never a literal
A live case number was compiled into the product in seven places. That is the
"no defaulting to the active case" prohibition in legal-operating-defaults §8,
and one instance is a privilege problem rather than a tidiness one.
src/routes/bridge/scrape.ts caseNumber || '<case>' -- the route silently
substituted one operator's matter whenever the
caller omitted it. caseNumber is now REQUIRED in
courtDocketScrapeSchema; an absent matter is a
400, not a guess.
src/lib/cron.ts the scheduled docket sync scraped one hardcoded
matter and wrote it into cc_legal_deadlines.
Now iterates KV `legal:tracked_cases`. Unset
config means the cron does nothing and logs that
it did nothing; it never picks a matter.
src/lib/contextual-ingest.ts LEGAL_CASE_PATTERN keyed privilege/sensitivity
classification to ONE case number. Every other
matter's work product was therefore classified
non-sensitive -- a §9 privilege gap, silent by
construction. Now matches court-case-number
SHAPES (a superset of the old literal) plus
refs configured in KV `legal:case_refs`.
ui/.../LitigationAssistant the case field was prefilled with a live matter,
so every user started inside someone else's case.
ui/.../Legal.tsx placeholder used a real case number; now a
format example.
src/agents/tools/storage.ts tool descriptions used a real party slug as the
"e.g."; these ship in the tool schema.
BEHAVIOUR CHANGE, called out rather than buried: non-docket refs (unit numbers,
party names) have no derivable shape and move to KV `legal:case_refs`. Until
that key is seeded this is NARROWER than the old hardcoded behaviour, so the
classifier warns when the key is unset instead of failing silently.
NOT FIXED: migrations/0004_seed_data.sql:138 carries a real matter caption.
Editing an applied migration is its own risk; flagged for a separate change.
VERIFICATION GAP, stated plainly: typecheck could NOT run. `npm ci` fails on
main with `No matching version found for hono-agents@^0.19.0` -- a pre-existing
broken dependency that PR #132 is already addressing. All seven changed files
pass esbuild parse, which catches syntax and not types. This needs a typecheck
once #132 lands; do not read parse-OK as compile-OK.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0172kkaE1YjR9UQMnGNa2bij
* fix(legal): bound the case-ref cache, isolate per-case enqueue failures
Two review findings on this PR, both real and both mine.
contextual-ingest: the `configuredRefsCache === null` guard cached the first
result for the isolate's lifetime. Workers reuse globals across requests, so
seeding `legal:case_refs` afterwards would never take effect in that isolate,
and a later config change would never propagate. Worse, a READ FAILURE was
cached as "configured empty" -- permanently downgrading sensitivity
classification for every subsequent request, which is the exact silent
under-protection this PR exists to remove.
Now: 60s TTL, and failures are never cached. On a failed read the previous
good value is kept and retried next call; if there is no previous value the
call returns false for that message only.
cron: a throw from enqueueJob escaped to the outer catch, abandoning every
later tracked case AND skipping processQueue for jobs that had already been
queued. Enqueue errors are now caught per case; processQueue stays outside
that catch so one bad case cannot strand the rest.
Verified: typecheck exit 0; node suite 68 passed, 0 failed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0172kkaE1YjR9UQMnGNa2bij
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The
buildjob has been red on every PR opened or updated since the DO coordinator merge (c979f6a, Aug 2). The cause was in none of those PRs — it is onmain.Root causes
1. vitest config overlap (the actual test blocker)
vitest.config.tsglobstests/**/*.test.ts, which also matchedtests/workers/coordinator.test.ts— a workerd-pool test importingcloudflare:test. That specifier only resolves under@cloudflare/vitest-pool-workers(vitest.workers.config.mts), so the node suite died at import:It failed before running a single assertion. Excluded
tests/workers/**from the node suite, preserving vitest's defaultnode_modules/distexcludes.2.
hono-agents@^0.19.0never existeda60d0e1applied the agents-SDK version to the wrong package. The registry tops out at3.0.12; the lockfile already pinned3.0.7. Freshnpm ifailed outright withETARGET. Aligned the manifest to the locked version — resolved version is still3.0.7, so this is a manifest-only correction with no runtime change.3. The workers suite ran nowhere in CI
npm run test:workerswas never invoked by any workflow, so the DO coordinator tests had never executed in CI. Simply excluding them fromnpm testwould have converted "loudly broken" into "silently unrun", so they are now wired into the build job.Booting them required
@cloudflare/vitest-pool-workers0.22 — the pinned 0.20.1 bundles a workerd supporting compatibility dates only through2026-08-06, one day behind the2026-08-07datea60d0e1set:Dev-only; the lockfile change is confined to that chain (pool-workers, miniflare, wrangler, workerd).
hono-agentsstays at3.0.7.Verification
Full sequence run locally against real infrastructure — no mocks, no stubs:
tsc -p tsconfig.json --noEmitnpm testnpm run test:workersThis PR will still show
buildred — deliberatelybuildruns Dependency Audit before Typecheck/Test, so it fails at 22s and never reaches the fix above. That audit failure is independent of this PR and pre-existing onmain.Worth flagging clearly:
npm auditis a moving gate. #128 passed it on Aug 14, but advisories published since (hono <=4.12.33, plusfast-uri,ip-address,js-yaml,nanoid,@hono/node-server,body-parser) now fail it on any branch — I verified this by merging #128 into this branch locally and re-running the gate; it still failed. #128 as it stands will not turn the queue green.Clearing it requires bumping
honopast 4.12.33 — a web-framework upgrade with real runtime risk that does not belong bundled inside a CI fix. Left as separate work, deliberately.Suggested review path: confirm the test-infra reasoning here on its own merits; land the hono/advisory bump separately, after which this PR's
buildshould go fully green.🤖 Generated with Claude Code
https://claude.ai/code/session_01SpK3ddoHzUqgW3keYafHsD
Summary by CodeRabbit
Tests
Chores