Repository navigation
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| node-version: 24 | ||
| cache: pnpm | ||
|
|
||
| - run: pnpm install --frozen-lockfile |
There was a problem hiding this comment.
The CI installs dependencies with --frozen-lockfile, but there's no pnpm.lock file visible in the diff. If the lockfile doesn't exist, this step will fail. Either commit a lockfile or use --no-frozen-lockfile for the CI install step.
There was a problem hiding this comment.
3 informational finding(s) (nits / suggestions, not blocking — listed here as summary, not tracked as individual threads):
README.md— The README states thatdist/is gitignored, but the CI workflow and the PR itself commit prebuilt JS to dist/. This is contradictory — dist/ should either be committed (as the CI enforces) or truly …README.md— The README now says consumers need Node ≥24 to build on install. However, since dist/ is precommitted, consumers who justnpm installthe package shouldn't need to run any build step — they only nee…scripts/copy-assets.mjs— The asset copy script matches all.mdand.jsonfiles recursively. This is fine for the current structure, but consider being more explicit (e.g., only copying from specific subdirectories like `s…
| node-version: 24 | ||
| cache: pnpm | ||
|
|
||
| - run: pnpm install --frozen-lockfile |
There was a problem hiding this comment.
The CI workflow uses --frozen-lockfile but no pnpm.lock file appears in this diff. The install step will fail unless the lockfile exists. Either commit a lockfile or change to --no-frozen-lockfile.
There was a problem hiding this comment.
1 prior bot finding(s) still unaddressed:
- thread
3369398908— The CI workflow still uses--frozen-lockfilebut no pnpm.lock file appears in the diff. The install step will fail without a lockfile.
1 informational finding(s) (nits / suggestions, not blocking — listed here as summary, not tracked as individual threads):
README.md— This statement contradicts the CI workflow which verifies that committeddist/matches the built output. Thedist/directory should be committed (not gitignored) for consumers who use the git depe…
| node-version: 24 | ||
| cache: pnpm | ||
|
|
||
| - run: pnpm install --frozen-lockfile |
There was a problem hiding this comment.
The CI workflow uses --frozen-lockfile but no pnpm.lock file appears anywhere in this diff. The install step will fail unless a lockfile is committed to the repository. Either commit a lockfile generated by pnpm install or change to --no-frozen-lockfile.
There was a problem hiding this comment.
2 prior bot finding(s) still unaddressed:
- thread
3369398908— The CI workflow still uses--frozen-lockfileat line 25 without a correspondingpnpm.lockfile in the repository. The install step will fail as noted in the original concern. - thread
3369401417— The same issue persists: the CI workflow uses--frozen-lockfilebut nopnpm.lockfile appears in the diff. The install step will fail unless a lockfile is committed.
3 informational finding(s) (nits / suggestions, not blocking — listed here as summary, not tracked as individual threads):
package.json— ThepackageManagerfield enforces pnpm 10.33.0, which is good practice. However, this version may not be available in thepnpm/action-setup@v4cache by default. Consider whether this specific vers…scripts/copy-assets.mjs— The asset glob\.(md|json)$will copy ALL markdown and JSON files from src/. This is currently correct since the only non-TypeScript files are prompt.md and schema.json, but it's a broad pattern. If…src/cli.ts— The shebang change from#!/usr/bin/env -S node --experimental-transform-typesto#!/usr/bin/env nodeis correct for the compiled JavaScript output, but the source filesrc/cli.tsstill needs the…
| node-version: 24 | ||
| cache: pnpm | ||
|
|
||
| - run: pnpm install --frozen-lockfile |
There was a problem hiding this comment.
The CI workflow uses --frozen-lockfile but no pnpm.lock file appears in this diff. The install step will fail unless a lockfile is committed. Either commit a lockfile generated by pnpm install or change to --no-frozen-lockfile.
There was a problem hiding this comment.
3 prior bot finding(s) still unaddressed:
- thread
3369398908— The CI workflow still usespnpm install --frozen-lockfileon line 25 without committing a pnpm.lock file. The install step will fail. - thread
3369401417— No pnpm.lock file was added in this diff. The--frozen-lockfileflag will cause the CI install to fail. - thread
3369413973— The diff shows no pnpm.lock file committed. The CI'spnpm install --frozen-lockfilestep will fail without it.
| node-version: 24 | ||
| cache: pnpm | ||
|
|
||
| - run: pnpm install --frozen-lockfile |
There was a problem hiding this comment.
The CI workflow uses --frozen-lockfile but no pnpm.lock file appears in this diff. The install step will fail unless a lockfile is committed. Either commit a lockfile generated by pnpm install or change to --no-frozen-lockfile.
There was a problem hiding this comment.
3 informational finding(s) (nits / suggestions, not blocking — listed here as summary, not tracked as individual threads):
README.md— This statement contradicts the npm dependency section below, which explains how to consume the reviewer as a devDependency in Node projects. Clarify that the Docker image requires no Node, but the npm…src/stages/types.ts— Breaking change: the schema flattened anchor.snippet to snippet. This changes the output format and will break consumers parsing the JSON schema or relying on the nested anchor structure. Consider ver…src/vcs/local.ts— The TTY check for color output is good, but process.stdout.isTTY can be undefined in some environments. Consider using Boolean(process.stdout.isTTY) to ensure a boolean value.
| node-version: 24 | ||
| cache: pnpm | ||
|
|
||
| - run: pnpm install --frozen-lockfile |
There was a problem hiding this comment.
The CI workflow uses --frozen-lockfile but no pnpm.lock file appears in this diff. The install step will fail unless a lockfile is committed. Either commit a lockfile generated by pnpm install or change to --no-frozen-lockfile.
1 participant
No description provided.