fix(ci): lint the full workspace in CI and fix the unused imports it missed - #3312
Conversation
…missed CI's lint task explicitly linted only src/ cli/ react/, while the local pre-push hook runs bare 'deno lint', which walks all workspace members including extensions/. The two gates disagreed in both directions: CI merged unused imports in ext-llm-anthropic and ext-llm-openai that then blocked every local push from a clean main. Align the lint task to the workspace-wide form and remove the two unused imports.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe Deno lint task now uses its default scope. Unused ChangesLint scope
Import cleanup
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@deno.json`:
- Line 481: Update the root lint configuration and the package.json lint script
so the default lint scope includes extensions/. Add a focused configuration
check that verifies extension sources are covered, while preserving the existing
targeted lint commands.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 80f29d8f-8550-41ae-9ca4-3f13506fa4ac
📒 Files selected for processing (3)
deno.jsonextensions/ext-llm-anthropic/src/anthropic-request-builder.tsextensions/ext-llm-openai/src/openai-responses-request-builder.ts
💤 Files with no reviewable changes (2)
- extensions/ext-llm-openai/src/openai-responses-request-builder.ts
- extensions/ext-llm-anthropic/src/anthropic-request-builder.ts
There was a problem hiding this comment.
Pull request overview
This PR aligns the CI lint task with the local (workspace-wide) lint gate and removes two unused imports in extensions that CI previously missed, preventing “green CI / blocked local push” drift.
Changes:
- Updated
deno task lintto run a workspace-widedeno lint(while keeping the scripts-specific lint runs unchanged). - Removed unused
stringifyJsonValueimports from the OpenAI and Anthropic LLM extension request builders.
Verification (author-reported):
deno task lintpasses (0 problems).- Full pre-push gate (fmt check, lint, typecheck, unit tests) passed.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
deno.json |
Expands the main lint task to run workspace-wide linting, bringing CI in line with local gating. |
extensions/ext-llm-openai/src/openai-responses-request-builder.ts |
Removes an unused stringifyJsonValue import that was previously missed by CI lint scope. |
extensions/ext-llm-anthropic/src/anthropic-request-builder.ts |
Removes an unused stringifyJsonValue import that was previously missed by CI lint scope. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The lint task was widened, but the configured include set still omitted extension sources. Add the extension source globs and a focused regression test so future lint-scope changes fail loudly before CI drifts from local checks. Constraint: This repo has no root package.json lint script to update, so the package.json portion of the review comment is stale. Rejected: Remove lint.include entirely | broader change would also sweep currently excluded file classes and make the fix less reviewable. Confidence: high Scope-risk: narrow Tested: npx --yes deno@2.7.7 test --config=scripts/test.deno.json --no-check --allow-read scripts/lint/lint-config.test.ts Tested: npx --yes deno@2.7.7 task lint Not-tested: full verify
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
scripts/lint/lint-config.test.ts:56
- This test file is not executed by any current CI job, so it will not prevent future regressions to
deno.jsonlint include/exclude. In.github/workflows/cicd.yml, the lint job runsdeno task lint(and other lint tasks), while the test jobs rundeno task test:integration/ coverage shards, none of which includescripts/lint/lint-config.test.ts(and it is also not listed indeno task test:scripts). Consider wiring this check into CI (for example, add it totest:scriptsand run that in CI, or add a smalllint:configtask invoked by the lint matrix).
Deno.test("root lint configuration covers extension sources", async () => {
const config = JSON.parse(await Deno.readTextFile("deno.json")) as DenoConfig;
const include = config.lint?.include ?? [];
const exclude = config.lint?.exclude ?? [];
const extensionSources = await collectExtensionSources();
kwakayama
left a comment
kwakayama
left a comment
There was a problem hiding this comment.
Review: 75/100 — request changes
The direction is right and the extension coverage gain is real (+232 files). But the switch from explicit paths to bare deno lint silently removes lint coverage from .js files under src/ — the exact opposite of this PR's stated purpose, and the new test cannot catch it.
P2 — lint coverage regression on production-build templates
deno.json changes the task from deno lint src/ cli/ react/ to bare deno lint, which switches enforcement from CLI paths to the lint.include globs. Those globs are .ts/.tsx only, so every .js file under src/ silently drops out of the gate.
Two files lose coverage:
src/build/production-build/templates/fallback-prefetch.jssrc/build/production-build/templates/fallback-router.js
These are production-build templates — code that ships into users' built output.
Verified behaviorally, not inferred. Appending debugger; to src/build/production-build/templates/fallback-router.js at this PR's head:
deno lint src/ cli/ react/ -> CAUGHT (no-debugger reported)
deno lint -> NOT CAUGHT (clean, exit 0)
Failure scenario: someone lands a debugger; statement, an unused variable, or any other recommended-rule violation in either template. CI passes green. The violation ships into every production build generated by the framework.
Fix — add to lint.include in deno.json:
"src/**/*.js",Please re-run the behavioral check above after the fix; it should report CAUGHT for both invocations.
P3 — the new test would not have caught this
scripts/lint/lint-config.test.ts only asserts that extension sources are covered. EXTENSION_SOURCE_EXTENSIONS is [".ts", ".tsx"], and the test never asserts that previously-covered files stay covered. It validates the direction the PR adds while leaving the direction the PR removes untested.
Two smaller points on the same test:
isCoveredByGlob()is a hand-rolled prefix/suffix approximation of glob matching, not Deno's real matcher. It validates the test's own model of the config rather than whatdeno lintactually does. Asserting on theChecked N filescount, or on a known-covered fixture file, would test reality.- Suggest extending it to assert coverage for a representative
.jspath undersrc/, so this regression class is caught rather than re-introduced.
P3 — fmt left inconsistent with lint (pre-existing, worth a follow-up)
fmt.include still lists only src, cli, react, and the fmt/fmt:check tasks still pass explicit paths. After this PR extension sources are linted but never format-checked, even though ci (format) is a required status check. Not introduced here and not blocking, but it is the same drift this PR exists to fix, one config block over.
Confirmed good
- The two removed
stringifyJsonValueimports inext-llm-anthropicandext-llm-openaiare genuinely unused — typecheck and CI green confirm. DENO_NO_PACKAGE_JSON=1correctly preserved on the task.- Moving lint scope into config rather than hardcoded CLI paths is the right call and prevents this class of drift going forward.
Score
| Axis | Score |
|---|---|
| Correctness | 30/40 |
| Test adequacy | 16/25 |
| Security / prod-safety | 17/20 |
| Maintainability | 12/15 |
| Total | 75/100 |
One-line fix away from merge-ready. Holding at request-changes because this PR is the lint contract every other open PR gets measured against — landing it with a coverage hole bakes the hole into the baseline.
|
Addressed the requested lint-contract fix at exact head |
All requested lint-contract changes are addressed at exact head 128cd53. Root lint includes src JavaScript templates and extension TypeScript sources, the configuration test uses the standard glob matcher and covers both cases, test:scripts executes it, and exact-head CI plus focused verification are green.
|
Merge confidence: 94% for exact head 128cd53. Reasoning before scheduling:
Residual risk is low and confined to future lint-scope configuration drift, which the new configuration regression now detects. This exceeds the required 90% threshold, so I am scheduling this exact head for squash merge. |
What
Aligns CI lint with the local pre-push hook and fixes the two lint errors CI had let onto main.
Why
The two lint gates disagreed in both directions:
deno task lint, which explicitly linted onlysrc/ cli/ react/—extensions/was never linted, so unusedstringifyJsonValueimports inext-llm-anthropicandext-llm-openaimerged green.deno lint, which walks all workspace members includingextensions/— so those same imports then blocked every push from a cleanmain(hit by all eight reconcile-slice PRs today, each carrying an identical unblock commit as a workaround).Changes
deno.json: thelinttask's first segment is now workspace-widedeno lint(matches the hook; 4,799 files vs ~4,300 before). The scripts-specific config invocations are unchanged, and the specialtylint:*tasks remain CI-only by design.stringifyJsonValueimport specifiers.Verification
deno task lintpasses with the new workspace-wide scope (0 problems).Note: once this merges, the identical
chore: drop unused stringifyJsonValue imports…commits in the open reconcile PRs become no-ops.Summary by CodeRabbit