feat: scaffold V1 — configs, action metadata, pure review core#1
Merged
Conversation
- action.yml: inputs/outputs per V1 design, runs.using docker - Multi-stage node:24-slim Dockerfile (build → slim runtime) - Tooling: strict tsconfig, typescript-eslint strict (no as/!), Prettier semi:false, vitest, Node 24 - Pure core modules: config parsing (Zod over string inputs), event payload → PrContext resolution, commentable-line + annotated-diff transforms over parse-diff, finding schema with strict structured-output JSON schema, phase definitions (combined V1, V2 seam), prompt assembly, finding selection (threshold/dedupe/optional cap), review comment mapping with near-miss snapping and beyond-diff body routing - 68 tests incl. cross-module annotate↔commentable consistency check - CI: format, lint, test, build, docker build smoke (SHA-pinned actions) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
ⓘ Qodo reviews are paused because your workspace is out of credits. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Warning Review limit reached
Next review available in: 34 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR adds repository configuration, documentation, Docker and CI setup, and GitHub Action metadata. It also introduces TypeScript modules and tests for structured logging, action config parsing, GitHub event resolution, diff annotation and commentable-line computation, review finding schemas and selection, review phases and prompt building, and mapping findings into review comments and review-body text. The entrypoint is still a stub that fails with a not-yet-implemented message. Estimated code review effort: 4 (Complex) | ~75 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 security issue, 3 other issues, and left some high level feedback:
Security issues:
- BlueOak-1.0.0: Open-source license could not be identified (link)
General comments:
- In
action.yml, themodelinput description says "no provider prefix" but the exampleanthropic/claude-sonnet-4-6includes one; consider clarifying this to avoid confusion about the expected slug format. - In
comment-mapping.ts, theSNAP_DISTANCEconstant is currently hard-coded to 3; you may want to either make this configurable via action inputs or document the rationale so future changes to diff behavior are easier to reason about. - The Dockerfile runs
npm citwice (once before build and once with--omit=dev); consider restructuring the build steps to avoid reinstalling dependencies and instead prune dev deps after compilation for faster, leaner builds.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `action.yml`, the `model` input description says "no provider prefix" but the example `anthropic/claude-sonnet-4-6` includes one; consider clarifying this to avoid confusion about the expected slug format.
- In `comment-mapping.ts`, the `SNAP_DISTANCE` constant is currently hard-coded to 3; you may want to either make this configurable via action inputs or document the rationale so future changes to diff behavior are easier to reason about.
- The Dockerfile runs `npm ci` twice (once before build and once with `--omit=dev`); consider restructuring the build steps to avoid reinstalling dependencies and instead prune dev deps after compilation for faster, leaner builds.
## Individual Comments
### Comment 1
<location path="src/github/event.ts" line_range="36-37" />
<code_context>
+ }),
+})
+
+const issueCommentEventSchema = z.object({
+ issue: z.object({
+ number: z.int().positive(),
+ // Present only when the issue is actually a pull request
+ pull_request: z.looseObject({}).optional(),
+ }),
</code_context>
<issue_to_address>
**suggestion:** Using `z.looseObject({})` is discouraged and may be brittle against future zod changes.
`pull_request` is currently typed as `z.looseObject({}).optional()`, but `looseObject` is being deprecated in Zod. Please switch to `z.object({}).catchall(z.any()).optional()` (or a more specific shape if you start using its properties) to keep the same behavior while avoiding a deprecated API and clarifying that `pull_request` contents are intentionally ignored.
```suggestion
// Present only when the issue is actually a pull request.
// We intentionally ignore the contents of `pull_request`, but allow any shape.
pull_request: z.object({}).catchall(z.any()).optional(),
```
</issue_to_address>
### Comment 2
<location path="Dockerfile" line_range="10-11" />
<code_context>
+COPY src ./src
+RUN npx tsc
+
+# Prune to production dependencies only
+RUN npm ci --omit=dev
+
+# Runtime stage: dist + production node_modules, nothing else
</code_context>
<issue_to_address>
**suggestion (performance):** Running `npm ci` twice in the build stage is redundant and adds build time.
The first `npm ci` for dev dependencies is later wiped by `npm ci --omit=dev`, causing a full reinstall of `node_modules` in the same stage. Consider keeping the dev install in a separate layer (or stage), running `tsc` there, and then installing only production dependencies in another layer/stage. This avoids duplicate installs, speeds up builds, and keeps the image structure clearer while preserving runtime behavior.
</issue_to_address>
### Comment 3
<location path="src/review/__tests__/comment-mapping.test.ts" line_range="65-42" />
<code_context>
+ })
+ })
+
+ it("degrades to a single-line comment when end_line falls in a different hunk", () => {
+ const finding = makeFinding({ line: 5, end_line: 142 })
+
+ const mapped = mapFindingsToReview({
+ findings: [finding],
+ commentableByPath: makeCommentableByPath(),
+ })
+
+ expect(mapped.comments[0]).toMatchObject({ line: 5, side: "RIGHT" })
+ expect(mapped.comments[0]?.start_line).toBeUndefined()
+ })
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for multi-line edge cases: end_line equal to line, end_line before line, and non-commentable end_line
`multiLineEnd` currently guards several edge cases (end_line === line, end_line < line, and end_line not in rightLines), but the tests only exercise the different-hunk case. To better lock in the intended behavior, please add tests for:
- end_line === line → degrades to a single-line comment.
- end_line < line → degrades to a single-line comment.
- end_line not in commentable.rightLines (e.g., in a gap inside the hunk) → degrades to a single-line comment.
These will ensure future changes don’t accidentally allow invalid multi-line ranges.
Suggested implementation:
```typescript
it("degrades to a single-line comment when end_line falls in a different hunk", () => {
const finding = makeFinding({ line: 5, end_line: 142 })
const mapped = mapFindingsToReview({
findings: [finding],
commentableByPath: makeCommentableByPath(),
})
expect(mapped.comments[0]).toMatchObject({ line: 5, side: "RIGHT" })
expect(mapped.comments[0]?.start_line).toBeUndefined()
})
it("degrades to a single-line comment when end_line equals line", () => {
const finding = makeFinding({ line: 143, end_line: 143 })
const mapped = mapFindingsToReview({
findings: [finding],
commentableByPath: makeCommentableByPath(),
})
expect(mapped.comments[0]).toMatchObject({ line: 143, side: "RIGHT" })
expect(mapped.comments[0]?.start_line).toBeUndefined()
})
it("degrades to a single-line comment when end_line is before line", () => {
const finding = makeFinding({ line: 145, end_line: 143 })
const mapped = mapFindingsToReview({
findings: [finding],
commentableByPath: makeCommentableByPath(),
})
expect(mapped.comments[0]).toMatchObject({ line: 145, side: "RIGHT" })
expect(mapped.comments[0]?.start_line).toBeUndefined()
})
it("degrades to a single-line comment when end_line is not commentable in the hunk", () => {
const finding = makeFinding({ line: 143, end_line: 144 })
const commentableByPath = makeCommentableByPath()
const originalFile = commentableByPath.get("src/greeter.ts")!
// Create a gap inside the hunk by omitting line 144 from rightLines
commentableByPath.set("src/greeter.ts", {
...originalFile,
rightLines: new Set([
1, 2, 3, 4, 5, 6,
141, 142, 143, /* 144 omitted */ 145, 146, 147,
]),
})
const mapped = mapFindingsToReview({
findings: [finding],
commentableByPath,
})
expect(mapped.comments[0]).toMatchObject({ line: 143, side: "RIGHT" })
expect(mapped.comments[0]?.start_line).toBeUndefined()
})
const makeCommentableByPath = (): Map<string, CommentableFile> =>
```
These changes assume `makeFinding` and `mapFindingsToReview` are already imported and available in this test file, consistent with the existing tests. If the test suite relies on `makeCommentableByPath` being pure (returning a new Map each time), the mutation inside the "end_line is not commentable in the hunk" test is safe because it only affects that local instance. If there are other fixtures or helpers that depend on the exact `rightLines` shape, verify that no other tests in the same `describe` block reuse the mutated `commentableByPath` instance.
</issue_to_address>
### Comment 4
<location path="package-lock.json" line_range="2083-2098" />
<code_context>
</code_context>
<issue_to_address>
**security (license/minimatch):** BlueOak-1.0.0: Open-source license could not be identified
The obligations of the `BlueOak-1.0.0` license for this code could not be determined automatically. Unknown licenses may carry obligations or restrictions and should be reviewed manually to ensure compliance
*Source: trivy*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…model description, snap rationale - Dockerfile: separate prod-deps stage instead of double npm ci in one stage - comment-mapping tests: end_line === line, end_line < line, non-commentable end_line all degrade to single-line comments - action.yml: model description no longer contradicts its own example - SNAP_DISTANCE: documented why 3 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- CODE_OF_CONDUCT.md (Contributor Covenant 2.1), CONTRIBUTING.md, SECURITY.md (incl. OpenRouter data-flow disclosure + prompt-injection threat model), CHANGELOG.md seed - .gitleaks.toml (extends default ruleset), .coderabbit.yaml - husky pre-commit (tsc --noEmit + lint-staged: eslint --fix + prettier) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/github/event.ts (1)
58-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract multi-clause conditional into a named boolean.
As per coding guidelines, multi-clause conditionals should be extracted into named booleans with affirmative names. This improves readability at the call site.
♻️ Suggested refactor
- if (eventName === "pull_request" || eventName === "pull_request_target") { + const isPullRequestEvent = + eventName === "pull_request" || eventName === "pull_request_target" + + if (isPullRequestEvent) {🤖 Prompt for 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. In `@src/github/event.ts` at line 58, The conditional in the event handling logic is a multi-clause check and should be extracted into a named affirmative boolean for readability. In the code around the eventName check in the event processing path, create a descriptive boolean (for example, one indicating whether the event is a pull request event) and use that in the if statement instead of comparing eventName inline. Keep the change localized to the event handling logic so the call site reads clearly.Source: Coding guidelines
eslint.config.ts (1)
23-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBroaden the test glob and keep assertion checks strict.
This override misses
*.spec.ts, and downgrading the assertion rules to warnings lets tests bypass the repo-wide no-as/!policy. Please cover both**/*.{test,spec}.tsand keep these rules aterror.As per coding guidelines, “Do not use type assertions (
as) or non-null assertions (!) — use runtime guards or schema validation to narrow.”♻️ Proposed fix
{ - files: ["**/__tests__/**/*.ts", "**/*.test.ts"], + files: ["**/__tests__/**/*.ts", "**/*.{test,spec}.ts"], rules: { - "`@typescript-eslint/consistent-type-assertions`": [ - "warn", - { assertionStyle: "never" }, - ], - "`@typescript-eslint/no-non-null-assertion`": "warn", + "`@typescript-eslint/consistent-type-assertions`": [ + "error", + { assertionStyle: "never" }, + ], + "`@typescript-eslint/no-non-null-assertion`": "error", }, },🤖 Prompt for 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. In `@eslint.config.ts` around lines 23 - 30, The ESLint override in the config is too narrow and too permissive: update the test file glob in the top-level config to include both test and spec files, and keep the assertion-related rules enforced as errors rather than warnings. Adjust the override where the `files` pattern and the `@typescript-eslint/consistent-type-assertions` and `@typescript-eslint/no-non-null-assertion` settings are defined so `**/*.{test,spec}.ts` is covered and the repo-wide no-`as`/`!` policy remains strict.Source: Coding guidelines
🤖 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 `@Dockerfile`:
- Around line 16-23: The runtime stage in the Dockerfile currently starts from
node:24-slim and runs the ENTRYPOINT as root because there is no USER directive.
Add a non-root user in this runtime stage and switch to it before ENTRYPOINT,
keeping the existing COPY steps for prod-deps and build artifacts intact. Use
the runtime stage symbols around COPY and ENTRYPOINT to place the user setup and
ensure the container executes /app/dist/src/main.js as that non-root user.
- Around line 1-5: `npm ci --omit=dev` is still triggering the `prepare`
lifecycle script, which fails because `husky` is a devDependency not present in
the Docker image. Update the Dockerfile’s `prod-deps` and build stages to run
`npm ci` with `--ignore-scripts` so npm skips `prepare` entirely, and keep the
change aligned with the existing dependency-install steps (`prod-deps`/build
`RUN npm ci`).
In `@src/review/finding.ts`:
- Around line 33-44: Add a cross-field validation on findingSchema so end_line
cannot be less than line, since lineRange() assumes a non-inverted range and
duplicate overlap checks can fail otherwise. Keep the schema definition in
src/review/finding.ts using the existing findingSchema symbol, and implement the
check with a .refine() so the inferred type stays unchanged while validation
happens at runtime; this should remain compatible with z.toJSONSchema() output.
---
Nitpick comments:
In `@eslint.config.ts`:
- Around line 23-30: The ESLint override in the config is too narrow and too
permissive: update the test file glob in the top-level config to include both
test and spec files, and keep the assertion-related rules enforced as errors
rather than warnings. Adjust the override where the `files` pattern and the
`@typescript-eslint/consistent-type-assertions` and
`@typescript-eslint/no-non-null-assertion` settings are defined so
`**/*.{test,spec}.ts` is covered and the repo-wide no-`as`/`!` policy remains
strict.
In `@src/github/event.ts`:
- Line 58: The conditional in the event handling logic is a multi-clause check
and should be extracted into a named affirmative boolean for readability. In the
code around the eventName check in the event processing path, create a
descriptive boolean (for example, one indicating whether the event is a pull
request event) and use that in the if statement instead of comparing eventName
inline. Keep the change localized to the event handling logic so the call site
reads clearly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ad3cca53-6451-480f-b55d-9d38bebc7ed0
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (45)
.coderabbit.yaml.dockerignore.github/workflows/ci.yml.gitignore.gitleaks.toml.husky/pre-commit.nvmrc.prettierrc.jsonAGENTS.mdCHANGELOG.mdCODE_OF_CONDUCT.mdCONTRIBUTING.mdDockerfileSECURITY.mdaction.ymleslint.config.tsfixtures/issue_comment.review.jsonfixtures/openrouter.response.jsonfixtures/pull_request.opened.jsonfixtures/sample.diffpackage.jsonsrc/__tests__/config.test.tssrc/__tests__/test-logger.tssrc/config.tssrc/diff/__tests__/annotate-diff.test.tssrc/diff/__tests__/commentable-lines.test.tssrc/diff/annotate-diff.tssrc/diff/commentable-lines.tssrc/github/__tests__/event.test.tssrc/github/event.tssrc/logger.tssrc/main.tssrc/review/__tests__/comment-mapping.test.tssrc/review/__tests__/finding.test.tssrc/review/__tests__/make-finding.tssrc/review/__tests__/phases.test.tssrc/review/__tests__/prompt.test.tssrc/review/__tests__/select-findings.test.tssrc/review/comment-mapping.tssrc/review/finding.tssrc/review/phases.tssrc/review/prompt.tssrc/review/select-findings.tstsconfig.jsonvitest.config.ts
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replaces the openai-SDK-with-baseURL approach. The official SDK is generated from OpenRouter's OpenAPI spec, so provider routing preferences and usage accounting are typed first-class instead of untyped extra-body passthrough. The base SDK, not @openrouter/agent — the action makes single structured-output completion calls, not tool-orchestrated loops. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Per-run cost report (model, tokens, USD from OpenRouter usage accounting) written to the workflow step summary, disableable per repo. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Convention: INPUT_* via @actions/core getInput; GITHUB_* ambient env via env-var with an injectable env record (vault-cortex idiom). Codified in AGENTS.md; first usage lands with main.ts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pure modules (diff/, review/) may not runtime-import I/O modules (github/, openrouter/, context/) or SDK packages; type-only imports allowed. Tests exempt (fixture reads). Verified the rule fails on a planted violation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Levels with LOG_LEVEL threshold, V8-captured source location (filename:line) on info+, child loggers merging context props, and function-valued props resolved at emit time. JSON lines to stdout, error level to stderr. Replaces the thin @actions/core wrapper; failures still surface as Actions annotations via core.setFailed at the pipeline boundary. 7 behavioral tests (79 total). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…g ranges - npm ci in both Docker stages now passes --ignore-scripts: the prepare script runs husky (a devDependency), which broke the prod-deps stage with exit 127. All runtime deps are pure JS, so no install scripts are needed. - lineRange() in select-findings now orders its ends, so a model-emitted inverted range (end_line < line) can't slip past the duplicate overlap check. Regression test added (mutation-checked). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/__tests__/logger.test.ts (1)
6-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract shared
captureStreamhelper to eliminate duplication.
captureStdoutandcaptureStderrare nearly identical — only the target stream differs. A shared helper reduces duplication and makes future stream capture (e.g., custom streams) trivial.♻️ Proposed refactor
-const captureStdout = (): { lines: () => WrittenLine[] } => { - const writeSpy = vi - .spyOn(process.stdout, "write") - .mockImplementation(() => true) - onTestFinished(() => writeSpy.mockRestore()) - return { - lines: () => - writeSpy.mock.calls.map((call): WrittenLine => { - const written = call[0] - if (typeof written !== "string") - throw new Error("expected string write") - return JSON.parse(written) - }), - } -} - -const captureStderr = (): { lines: () => WrittenLine[] } => { - const writeSpy = vi - .spyOn(process.stderr, "write") - .mockImplementation(() => true) - onTestFinished(() => writeSpy.mockRestore()) - return { - lines: () => - writeSpy.mock.calls.map((call): WrittenLine => { - const written = call[0] - if (typeof written !== "string") - throw new Error("expected string write") - return JSON.parse(written) - }), - } -} +const captureStream = ( + stream: typeof process.stdout | typeof process.stderr, +): { lines: () => WrittenLine[] } => { + const writeSpy = vi + .spyOn(stream, "write") + .mockImplementation(() => true) + onTestFinished(() => writeSpy.mockRestore()) + return { + lines: () => + writeSpy.mock.calls.map((call): WrittenLine => { + const written = call[0] + if (typeof written !== "string") + throw new Error("expected string write") + return JSON.parse(written) + }), + } +} + +const captureStdout = () => captureStream(process.stdout) +const captureStderr = () => captureStream(process.stderr)🤖 Prompt for 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. In `@src/__tests__/logger.test.ts` around lines 6 - 36, The `captureStdout` and `captureStderr` helpers duplicate the same stream-spying logic, so extract a shared `captureStream` helper and reuse it from both functions. Keep the existing behavior in `logger.test.ts` by passing the target stream (`process.stdout` or `process.stderr`) and returning the same `lines()` reader, while preserving the `onTestFinished` restore and JSON parsing logic.
🤖 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 `@eslint.config.ts`:
- Around line 49-52: The second no-restricted-imports pattern in
eslint.config.ts is missing allowTypeImports: true, so type-only imports from
`@actions/`** and `@openrouter/`** are incorrectly blocked even though they should
be allowed. Update the restricted-imports configuration in the
no-restricted-imports rule so the pattern that contains group: ["node:fs",
"node:fs/**", "`@actions/`**", "`@openrouter/`**"] also includes allowTypeImports:
true, matching the intent of the first pattern and preserving type-only imports.
In `@src/logger.ts`:
- Line 1: The logger currently reads LOG_LEVEL from raw process.env at module
load time, which makes log-level behavior non-injectable and fragile in tests.
Update the logger setup in logger.ts to use env-var with an injectable env
record, and move the LOG_LEVEL lookup into createLogger so callers can pass a
custom env object for tests. Keep the existing logger behavior, but ensure
createLogger (and any helper it uses) accepts the env override and no longer
depends on module-level env access.
---
Nitpick comments:
In `@src/__tests__/logger.test.ts`:
- Around line 6-36: The `captureStdout` and `captureStderr` helpers duplicate
the same stream-spying logic, so extract a shared `captureStream` helper and
reuse it from both functions. Keep the existing behavior in `logger.test.ts` by
passing the target stream (`process.stdout` or `process.stderr`) and returning
the same `lines()` reader, while preserving the `onTestFinished` restore and
JSON parsing logic.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4d45d119-7de6-4404-81da-5d034119d3c3
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
AGENTS.mdCONTRIBUTING.mdDockerfileaction.ymleslint.config.tspackage.jsonsrc/__tests__/config.test.tssrc/__tests__/logger.test.tssrc/__tests__/test-logger.tssrc/config.tssrc/logger.tssrc/review/__tests__/select-findings.test.tssrc/review/select-findings.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- package.json
- action.yml
- src/config.ts
- src/review/tests/select-findings.test.ts
- Dockerfile
- src/tests/config.test.ts
- src/review/select-findings.ts
aliasunder
commented
Jul 9, 2026
aliasunder
left a comment
aliasunder
left a comment
Owner
Author
There was a problem hiding this comment.
Phase 1: PR Review
5 findings across 5 files (dimensions: correctness, security/performance, docs). 3 would-fix (trivial), 2 flagged as needs-design-decision.
- [D1]
src/diff/annotate-diff.ts:43—\ No newline at end of filemarkers render as phantom content lines with duplicated line numbers (verified against parse-diff source + runtime) - [D1]
src/logger.ts:83—JSON.stringifythrows on circular refs/BigInt, so a log call can crash the action - [D4]
src/review/prompt.ts:79— untrusted file content can close the</file>wrapper (prompt-injection delimiter escape) - [D4]
Dockerfile:2— mutablenode:24-slimbase tag vs. the repo's SHA-pinning convention for actions - [D6]
AGENTS.md:23— structure tree lists modules that don't exist yet (openrouter/,context/,orchestrate.ts, octokit wrappers)
Verified clean: ci.yml action SHAs match their claimed version tags (checked via GitHub API), SECURITY.md claims match the implementation, comment-mapping hunk/snap edge cases guarded, all 80 tests pass.
Verdict: ship-with-minor-fixes
🔍 ship-check · pr-review · claude-fable-5
…logger env - The second no-restricted-imports pattern (node:fs + SDKs) now sets allowTypeImports: true, matching the first pattern and the documented intent — each pattern is evaluated independently, so the flag did not carry over. - LOG_LEVEL now resolves through env-var with an injectable env record (per AGENTS.md env conventions) instead of raw process.env property access at module load. createLogger takes an optional env option, child loggers inherit it. 3 new tests (mutation-checked); the default threshold test now injects an empty record so ambient LOG_LEVEL can't flip it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
aliasunder
commented
Jul 9, 2026
aliasunder
left a comment
aliasunder
left a comment
Owner
Author
There was a problem hiding this comment.
Phase 2: Code Quality
6 findings across 5 files (naming 1, structure 2, comments 1, simplicity 2, module conventions 0).
Two additional items flagged without inline comments (design decisions, not posted as findings):
- The severity list is duplicated between
src/config.ts(SEVERITY_LEVELS) andsrc/review/finding.ts(FINDING_SEVERITIES), held consistent by a cross-module test. Single source of truth would be simpler, but importing across the two would couple action-input parsing to the LLM wire schema — the consistency test is a defensible decoupling. Worth an explicit call either way. prompt.tsinterpolatespath/reasoninto<file …>attributes unquoted-unescaped — same injection family as the Phase 1</file>delimiter finding already deferred as a design decision; resolve them together.
🔍 ship-check · code-quality · claude-fable-5
…crash guard, refactors pr-review findings: - annotate-diff: filter parse-diff's '\ No newline at end of file' marker changes — they duplicated the previous change's line number and rendered as phantom content lines, undermining anchoring (new fixture + test) - logger: JSON.stringify wrapped with a degraded fallback so circular refs / BigInt in log data can't crash the action (new test) - AGENTS.md: mark not-yet-landed modules (openrouter/, context/, orchestrate.ts, github/ octokit wrappers) as planned code-quality findings: - config: extract shared parsePositiveInteger; optional variant composes it - finding: severityRank → SEVERITY_RANK (constant-table naming consistency) - comment-mapping: capture end_line local (drops dead null re-check); extract 40-line map callback to classifyFinding - select-findings: single sort with full comparator before dedupe (was two sort passes); kept duplicate now deterministic on severity ties - annotate-diff: name LINE_NUMBER_WIDTH once — deleted-row padding was silently coupled to padStart(6) Both behavioral fixes mutation-checked. 85 tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
aliasunder
commented
Jul 9, 2026
aliasunder
left a comment
aliasunder
left a comment
Owner
Author
There was a problem hiding this comment.
Phase 3: Test Audit
5 test quality findings, 6 coverage gaps across 9 files (10 test files audited, 85 tests, all passing).
All 6 coverage gaps were mutation-verified in a sandbox copy: each listed mutation was applied to the production code and the full suite stayed green, proving the behavior is currently unpinned. No coverage regressions (no tests exist on main to regress against). Prior-phase findings fixed in 70d3145 were re-checked and are not re-reported; the fixes for the phantom no-newline lines and the logger serialization guard both carry solid dedicated tests.
🔍 ship-check · test-audit · claude-fable-5
…pendabot - Untrusted-content wrappers (file, conventions, diff, prior_findings) now carry a per-run random suffix (generateDelimiterNonce, 12 hex chars) so a malicious PR containing a literal closing tag can't break out of the wrapper into instruction position; path/reason attributes escape double quotes. buildUserPrompt takes a required delimiter_nonce param (no callers yet — orchestrate.ts lands next PR). 4 new tests incl. breakout regression (mutation-checked). SECURITY.md documents the defense. - Dockerfile base image digest-pinned on all three stages (same idiom as SHA-pinned actions in ci.yml) — the image builds on the consumer's runner, so a mutated tag flows into the container holding both tokens. New .github/dependabot.yml (docker ecosystem, weekly) restores the patch stream the pin would otherwise lose. Local docker build verified against the pinned digest. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
aliasunder
commented
Jul 9, 2026
aliasunder
left a comment
aliasunder
left a comment
Owner
Author
There was a problem hiding this comment.
Orchestrator triage — resolution for the deferred severity-duplication finding from the code-quality phase (severity list defined in both src/config.ts and src/review/finding.ts, with no consistency guard). Decision recorded in the inline comment on src/review/finding.ts for the implementing agent.
🔍 ship-check · orchestrator-triage · claude-fable-5
aliasunder
commented
Jul 9, 2026
aliasunder
left a comment
aliasunder
left a comment
Owner
Author
There was a problem hiding this comment.
Phase 4: Bug Check
9 bugs found across 7 files (all inline below). Reviewed at 70d3145 per dispatch; the PR head has since moved to ac6e817 (delimiter nonce + attribute escaping, Dockerfile digest pin, dependabot) — none of the 9 findings are fixed by those commits, all anchors verified against the current head, and finding 9 is new in ac6e817 itself.
Confidence: 4 high, 5 medium, 0 low
By dimension: description-vs-implementation 3 · boundary/off-by-one 1 · behavioral consistency 3 · input validation 1 · platform/encoding 1
Verified non-findings (checked, clean): both CI action SHA pins resolve to exactly the tags their comments claim (checked via the GitHub API); parse-diff's no-newline-marker behavior matches the annotate-diff doc comment (verified against parse-diff 0.12.0); finding.ts's strict-structured-output schema claims hold (every key required, additionalProperties false — verified on the generated schema); .dockerignore excludes tests/fixtures so the Docker build stays consistent with tsconfig's include; ENTRYPOINT path matches the rootDir output layout; generateDelimiterNonce matches SECURITY.md's 12-hex example.
🔍 ship-check · bug-check · claude-fable-5
…rds, prompt hardening, severity single-source test-audit (11 findings): sentinel emits for silent no-op tests, positive anchors before absence assertions, fixture-count anchor on the cross-module loop, full toBe on deterministic annotate/comment output, and coverage for required-integer empty rejection, cross-file dedupe, pull_request_target, malformed issue_comment, warn routing, debug source omission, LOG_LEVEL case-insensitivity, SNAP_DISTANCE boundaries, and null PR body. bug-check (9 findings): - comment-mapping: snap is now bounded by SNAP_DISTANCE itself (a nearby pure-deletion hunk could snap 488 lines away); suggestion fences size to the longest backtick run; body findings render suggestions - logger: throwing lazy props record a placeholder instead of crashing; per-call data resolves through the same lazy path as child props - prompt: truncation calls toWellFormed (no lone surrogates); PR title/ description/branch wrapped in nonce-tagged pr_metadata block, closing the SECURITY.md claim gap - tsconfig: module/moduleResolution NodeNext — compiler now enforces the .js-extension ESM rule the docs declare - CHANGELOG/CONTRIBUTING: release-workflow claims reworded to future tense severity single-source (orchestrator decision, phases pattern): config validates severity_threshold shape only; review/finding.ts owns the vocabulary via resolveSeverityThreshold (startup fail-fast when the orchestrator lands). SEVERITY_LEVELS removed from config. Also extracted the shared captureStream test helper (CodeRabbit nitpick). 24 new tests (109 total), key fixes mutation-checked. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
Responses to the two review-body items that have no inline thread: CodeRabbit nitpick (logger.test.ts — shared Phase 2 code-quality body items:
🔍 ship-check · pr-monitor · claude-fable-5 |
…braces Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ed parsing @actions/core already ships getBooleanInput, which enforces the YAML 1.2 core-schema boolean list (true|True|TRUE / false|False|FALSE) and throws on anything else — no reason to reinvent that. RawInputs now carries booleans pre-parsed from the collection boundary (main.ts, wired next PR with the orchestrator); the booleanString transform is gone and the schema validates them as plain z.boolean(). One behavior shift: a bad boolean input now fails at collection with getBooleanInput's error rather than inside parseConfig's aggregated message. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Module layering: before writing a parsing/validation helper, check the SDK's utility surface (getBooleanInput replaced the hand-rolled boolean-string transform); 'validation lives in config.ts' scopes our rules, not the platform's. - Code style: block bodies for any multiline function response; expression bodies only for one-liners. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…oundary collectRawInputs gathers strings via getInput and booleans via getBooleanInput (strict YAML 1.2 list), then parseConfig validates — bad inputs now fail loudly today instead of waiting for the orchestrator PR. Verified end-to-end: cost_summary=maybe fails with the YAML 1.2 error; valid inputs reach the pipeline stub. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ut fix - AGENTS.md env rule: event name/payload come from @actions/github context (it reads GITHUB_EVENT_NAME/GITHUB_EVENT_PATH and parses the JSON) — the old bullet's GITHUB_EVENT_PATH example steered toward hand-reading what the SDK already provides, the same trap as booleanString. env-var remains for genuinely ambient values. - AGENTS.md structure tree: main.ts line now describes what exists (input collection/validation) with pipeline + outputs marked next PR. - Remove @octokit/webhooks-types — devDependency imported nowhere; runtime payload validation is the zod schemas in event.ts. Verified clean in the same sweep: all ci.yml convention claims (SHA pins, persist-credentials, least-privilege permissions), no raw process.env reads outside the injectable logger default, parse-diff/ zod/toJSONSchema/getInput/setFailed all genuinely used. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jul 11, 2026
5 tasks
3 tasks
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.
What
Initial scaffold of the umm-actually GitHub Action: repo tooling, action metadata, Docker packaging, CI, and the complete pure (no-I/O) core of the V1 review pipeline with full unit coverage.
Contents
Tooling — ESM, Node 24, strict tsconfig (
noUncheckedIndexedAccess,exactOptionalPropertyTypes), typescript-eslint strict with type assertions banned (assertionStyle: never), Prettier (semi: false), vitest.Action packaging —
action.yml(inputs: model, fallback_model, max_findings [uncapped by default], severity_threshold, conventions_file, phases, context_budget_tokens, trace_related_files, pr_number; outputs: findings_count, review_url, model_used, skipped_reason) withimage: Dockerfileduring development — a release workflow will flip this to a digest-pinned GHCR image. Multi-stagenode:24-slimDockerfile.src/main.tsis a stub that fails loudly; the pipeline lands in the next PR.Pure core modules
config.ts— Zod validation/coercion over all-string action inputsgithub/event.ts— event payload →PrContext(complete forpull_request, needs-fetch forissue_comment)diff/commentable-lines.ts+diff/annotate-diff.ts— pure transforms overparse-diffoutput; the annotated diff prints exactly the commentable line set (enforced by a cross-module test)review/finding.ts— finding schema (severity + confidence + mandatory failure_scenario) and its strict-structured-output JSON schemareview/phases.ts— dimension text as constants;combinedphase for V1, additive seam for V2 multi-phasereview/prompt.ts— deterministic system/user prompt assembly (diff-anchored tracing scope, proof-of-work analysis field, severity rubric, anchoring contract)review/select-findings.ts— threshold filter, overlap dedupe (keeps higher severity), severity sort, optional capreview/comment-mapping.ts— findings → inline review comments with anchor validation, near-miss snapping, and beyond-diff findings routed to the review bodyTests — 68 tests across 9 files against shared fixtures (real multi-hunk diff with rename/add/delete/binary cases, event payloads, sample LLM response). Key mutations verified to fail the right tests (commentable-line computation, multi-line comment start/end ordering).
CI — format check, lint, test, build, Docker build smoke; SHA-pinned actions, least-privilege permissions,
persist-credentials: false.Notes
difffences rather than GitHub one-click suggestion blocks — suggestion blocks replace the commented range verbatim, which an LLM-supplied diff can't guarantee; deferred until the mapping can validate alignment.fixtures/sample.diffline counts were hand-verified against parse-diff output.Verification
npm test— 68/68 greennpm run lint,npm run prettier:check,npm run build— cleandocker build— succeeds;docker runexits 1 with the expected not-implemented error🤖 Generated with Claude Code