Repository navigation
Conversation
There was a problem hiding this comment.
Superseded by a newer AI review
🤖 AI Review
Both independent reviews completed. Confirmed Claude's maintenance nit: Effect exclusions depend on a manually synchronized rule list. No additional issues found. Lint execution was unavailable because dependencies are not installed.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| ⚪ NIT | .oxlintrc.json:585 |
maintainability |
claude | The Effect exclusions disable rules individually, so future preset additions can apply to previously excluded files unless this list is updated. Resulting diagnostics can fail CI. |
Stats
Claude findings: 1 · Codex findings: 0 · Confirmed: 1 · Refuted: 0 · Uncertain: 0
Models: claude-opus-5-5 + gpt-6.1-sol · Trigger: auto · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
CLI benchmark: no changes foundCommit
API calls: unchanged for all 7 commands. Binary size: 134.0 MB, unchanged. Details: timing ranges, API calls per command, slowest phasesTiming ranges
API calls per command
Slowest phasesNo command moved beyond noise, so there are no phase deltas. Comment |
|
/ai-review |
There was a problem hiding this comment.
🤖 AI Review
Both independent reviews completed. Confirmed Claude’s minor lint-coverage finding: the consolidated configuration excludes CI scripts from all oxlint checks. Codex reported no findings. Verification was static because node_modules is absent.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟡 MINOR | .oxlintrc.json:17 |
lint-coverage |
claude | Adding .github/scripts to the global ignorePatterns removes all oxlint coverage for CI automation, rather than excluding only Effect-specific rules. |
Stats
Claude findings: 1 · Codex findings: 0 · Confirmed: 1 · Refuted: 0 · Uncertain: 0
Models: claude-opus-5-5 + gpt-6.1-sol · Trigger: manual · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
kanadgupta
left a comment
There was a problem hiding this comment.
mind adding comments for all the ignores/exclusions?
kanadgupta
left a comment
There was a problem hiding this comment.
one more piece of feedback, apologies
| "check:all": "pnpm exec turbo run types:check lint:check fmt:check knip:check lint:effect:check lint:actions:check", | ||
| "fix:all": "pnpm exec turbo run lint:fix fmt:fix knip:fix && pnpm run lint:effect:fix", | ||
| "check:all": "pnpm exec turbo run types:check lint:check fmt:check knip:check lint:actions:check", | ||
| "fix:all": "pnpm exec turbo run fmt:fix --continue=always", |
There was a problem hiding this comment.
what's the context on this change (i.e., making it so pnpm fix:all only lists fmt:fix as its sole task and we use the dependsOn tree in turbo.json)?
i think the previous approach (where the tasks were listed out in this turbo run command) makes more sense to me, for a few reasons:
- these tasks not actually dependent on one another (but correct me if i'm wrong here)
- if i as a user wanted to fix oxfmt issues, i'd expect that running
pnpm exec turbo run fmt:fixwould only runoxfmtand not the other fixing commands - it's antipattern compared to
check:all
if the reasoning for this change is that we can't run these three commands in parallel, we should use --concurrency 1 instead
suggestion if these can run in parallel:
| "fix:all": "pnpm exec turbo run fmt:fix --continue=always", | |
| "fix:all": "pnpm exec turbo run lint:fix fmt:fix knip:fix", |
suggestion if these cannot run in parallel:
| "fix:all": "pnpm exec turbo run fmt:fix --continue=always", | |
| "fix:all": "pnpm exec turbo run lint:fix fmt:fix knip:fix --concurrency 1 --continue=always", |
| "cache": false, | ||
| "dependsOn": ["knip:fix"] |
There was a problem hiding this comment.
per my feedback above
| "cache": false, | |
| "dependsOn": ["knip:fix"] | |
| "cache": false |
| "cache": false, | ||
| "dependsOn": ["lint:fix"] |
There was a problem hiding this comment.
per my feedback above
| "cache": false, | |
| "dependsOn": ["lint:fix"] | |
| "cache": false |
TL;DR
folds the effect lint into
pnpm lint:check, so one.oxlintrc.jsondrives editors, agents and CIwhats introduced?
.oxlintrc.jsonextends the@effect/tsgorecommended preset, and.oxlintrc.effect.jsonand thelint:effect:*scripts go away.github/scripts, the npm launcher shim, compute stack templates and the generated API contracts move toignorePatterns, so they leave the repo lint entirelylint:fixapplies suggestions, andfix:allruns knip, lint and format in order so fixes never raceref