Repository navigation
Conversation
…s with effect lint (CLI-2655)
Contributor
There was a problem hiding this comment.
🤖 AI Review
Both independent reviews were available: Claude reported four findings and Codex reported none. Three findings are confirmed: ineffective traversal-test coverage, import-time Path provisioning, and test fixtures without guaranteed cleanup. The secret-retention finding remains uncertain because the underlying filesystem implementation was unavailable for inspection. No duplicate findings were present.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟡 MINOR | apps/cli/src/command-internal/start-secrets-cleanup.ts:30 |
security |
claude | A failed removal interrupts sibling cleanup effects, potentially leaving other containers' staged secrets on disk. |
| 🟡 MINOR | apps/cli/src/command-internal/start-secrets-cleanup.unit.test.ts:116 |
test-coverage |
claude | The traversal payload resolves to a different location from the canary, so this test passes even if the containment guard is removed. |
| ⚪ NIT | apps/cli/src/command-internal/test-db.pg-prove-args.ts:6 |
effect-idioms |
claude | Import-time Effect.runSync provisioning hardcodes Path implementations instead of allowing buildPgProveArgs to use the caller's supplied Path service. |
| ⚪ NIT | apps/cli/src/command-internal/db-bootstrap/db-setup.unit.test.ts:160 |
test-hygiene |
claude | Temporary test directories have no guaranteed cleanup when assertions or effects fail, and several migrated tests redundantly provide BunServices. |
Stats
Claude findings: 4 · Codex findings: 0 · Confirmed: 3 · Refuted: 0 · Uncertain: 1
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.
This branch has not been deployed
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
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.
TL;DR
brings the
db-bootstrap,stack,startandtest-dbmodules under the effect lint, tests includedwhats introduced?
db-bootstrap/**,stack-*,start-*andtest-db.*join the allow list,with
db-bootstrap/local-db-runningkept out that for now since it probes the docker engine socket throughnode:httpstart-secrets-cleanupremoves staged secrets throughFileSystem, andtest-dbbuilds itspg_proveargs throughPath, instead ofnode:fsandnode:pathSchemawith the same bytes, and yieldable errors are yielded directly instead of throughEffect.failnode:imports andJSONfor effect services and schemas, and the start-secrets traversal test now aims at a real canary outside the staging rootref