feat: V1 I/O clients — github, openrouter, workspace context#3
Merged
Conversation
PR 1 of 2 for the V1 pipeline (spec: vault plans/v1-pipeline-spec). github/client.ts (diff fetch via media type, createReview with 422 body-only fallback), openrouter/client.ts (structured-output retry ladder, max 4 calls, cost accounting) + cost-summary.ts, context/ (conventions reader, budgeted changed-file reads, bounded reverse-import related-files scan). Dead code until orchestrate.ts lands in PR 2. main.ts registers both credentials with the runner masker via core.setSecret. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 45 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughAdds workspace context discovery, validated GitHub and OpenRouter clients, structured review retries and cost reporting, bootstrap secret masking, and fixtures and tests covering these behaviors. ChangesWorkspace context assembly
GitHub review client
OpenRouter structured review
Bootstrap and project structure
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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 3 issues, and left some high level feedback:
- Consider extracting the duplicated HTTP error status helpers (e.g.,
errorStatusin GitHub client anderrorStatusCodein OpenRouter client) into a shared utility to keep error handling consistent across I/O clients. - The workspace scanning code mixes
path.joinwithposix-style paths (posix.join,posix.extname); if Windows runners are a target, it may be worth standardizing on one path convention or clearly documenting the assumptions to avoid subtle separator issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the duplicated HTTP error status helpers (e.g., `errorStatus` in GitHub client and `errorStatusCode` in OpenRouter client) into a shared utility to keep error handling consistent across I/O clients.
- The workspace scanning code mixes `path.join` with `posix`-style paths (`posix.join`, `posix.extname`); if Windows runners are a target, it may be worth standardizing on one path convention or clearly documenting the assumptions to avoid subtle separator issues.
## Individual Comments
### Comment 1
<location path="src/openrouter/__tests__/client.test.ts" line_range="270-280" />
<code_context>
+ ])
+ })
+
+ it("aborts the ladder after exactly one call on a 401", async () => {
+ const stub = makeSdkStub({
+ sendResponses: [{ error: makeStatusError(401) }],
+ })
+ const { client } = makeClient(stub)
+
+ await expect(client.requestReview(requestParams)).rejects.toThrow(
+ "OpenRouter auth/credit error — aborting without fallback: openai/gpt-5-mini: api_error (HTTP 401: HTTP 401)",
+ )
+ expect(stub.sendCalls).toHaveLength(1)
+ })
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for all auth/credit abort status codes (401/402/403), not just 401
The client treats 401, 402, and 403 as abort statuses via `ABORT_STATUSES`, but this test only covers 401. To fully validate that the ladder aborts for all configured auth/credit failures, please parameterize this test (or add separate ones) for 401, 402, and 403 and assert that no fallback model call is made in each case. This helps catch regressions if the abort set or error handling changes and retries are accidentally reintroduced for 402/403.
```suggestion
it.each([401, 402, 403])(
"aborts the ladder after exactly one call on an auth/credit error status %d",
async (status) => {
const stub = makeSdkStub({
sendResponses: [{ error: makeStatusError(status) }],
})
const { client } = makeClient(stub)
await expect(client.requestReview(requestParams)).rejects.toThrow(
`OpenRouter auth/credit error — aborting without fallback: openai/gpt-5-mini: api_error (HTTP ${status}: HTTP ${status})`,
)
expect(stub.sendCalls).toHaveLength(1)
},
)
```
</issue_to_address>
### Comment 2
<location path="src/openrouter/__tests__/client.test.ts" line_range="362-385" />
<code_context>
+ expect(result.attempts[0]?.costUsd).toBe(0.0399)
+ })
+
+ it("degrades to a null cost with a warning when the generation lookup fails", async () => {
+ const noCostResult = {
+ id: "gen-no-cost",
+ model: "openai/gpt-5-mini",
+ choices: [{ message: { content: JSON.stringify(acceptedReview) } }],
+ usage: { promptTokens: 12000, completionTokens: 800, cost: null },
+ }
+ const stub = makeSdkStub({
+ sendResponses: [{ value: noCostResult }],
+ generationResponses: [{ error: makeStatusError(500) }],
+ })
+ const { client, logger } = makeClient(stub)
+
+ const result = await client.requestReview(requestParams)
+
+ expect(result.attempts[0]?.costUsd).toBeNull()
+ expect(logger.messages).toContainEqual({
+ level: "warn",
+ message: "generation cost lookup failed",
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for the "unexpected generation response shape" warning path
You already test the successful lookup and the failure that logs `"generation cost lookup failed"`. There’s a third branch in `lookupGenerationCost` where the lookup succeeds but the response fails schema validation, logging `"unexpected generation response shape"`. Please add a test that stubs `generationResponses` with an invalid payload and asserts that `costUsd` stays null and the expected warning is logged, so all branches are covered.
```suggestion
it("degrades to a null cost with a warning when the generation lookup fails", async () => {
const noCostResult = {
id: "gen-no-cost",
model: "openai/gpt-5-mini",
choices: [{ message: { content: JSON.stringify(acceptedReview) } }],
usage: { promptTokens: 12000, completionTokens: 800, cost: null },
}
const stub = makeSdkStub({
sendResponses: [{ value: noCostResult }],
generationResponses: [{ error: makeStatusError(500) }],
})
const { client, logger } = makeClient(stub)
const result = await client.requestReview(requestParams)
expect(result.attempts[0]?.costUsd).toBeNull()
expect(logger.messages).toContainEqual({
level: "warn",
message: "generation cost lookup failed",
data: { error: "HTTP 500" },
})
})
it("degrades to a null cost with a warning when the generation lookup returns an unexpected shape", async () => {
const noCostResult = {
id: "gen-no-cost",
model: "openai/gpt-5-mini",
choices: [{ message: { content: JSON.stringify(acceptedReview) } }],
usage: { promptTokens: 12000, completionTokens: 800, cost: null },
}
const stub = makeSdkStub({
sendResponses: [{ value: noCostResult }],
// Return a successful response, but with an invalid payload shape
generationResponses: [{ value: { data: { foo: "bar" } } }],
})
const { client, logger } = makeClient(stub)
const result = await client.requestReview(requestParams)
expect(result.attempts[0]?.costUsd).toBeNull()
expect(
logger.messages.some(
(msg) => msg.level === "warn" && msg.message === "unexpected generation response shape",
),
).toBe(true)
})
it("skips the cost lookup entirely when the sdk has no generations surface", async () => {
```
</issue_to_address>
### Comment 3
<location path="src/openrouter/__tests__/cost-summary.test.ts" line_range="14-23" />
<code_context>
+describe("renderCostSummary", () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for the zero-attempts case to pin down summary behavior
Existing tests cover several non-empty attempt scenarios, but not `attempts: []`. Since this function may be called defensively, please add a test for an empty attempts array that verifies the header, table, and total line remain well-formed (e.g., `Total cost: n/a`) and that no errors are thrown or malformed markdown is produced.
Suggested implementation:
```typescript
describe("renderCostSummary", () => {
it("renders zero attempts with well-formed summary", () => {
const summary = renderCostSummary({
attempts: [],
modelUsed: "openai/gpt-5-mini",
})
expect(summary).toBe(
[
"### umm-actually cost summary",
"",
"| Model | Attempts | Prompt tokens | Completion tokens | Cost (USD) |",
"| --- | --- | --- | --- | --- |",
"| openai/gpt-5-mini | 0 | 0 | 0 | n/a |",
"",
"Total cost: n/a",
].join("\n"),
)
})
it("renders a single accepted attempt with its total", () => {
```
If the actual markdown format produced by `renderCostSummary` differs (e.g., different header text, column order, or total line wording), adjust the expected array lines to match the existing tests’ formatting conventions while still asserting:
1. The header line is present.
2. The table header is present and well-formed.
3. The model row reflects zero attempts/tokens and an `n/a` cost (or equivalent representation).
4. The total line expresses that cost is not applicable (e.g., `Total cost: n/a`).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Owner
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/github/__tests__/client.test.ts (1)
101-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a test for null body mapping.
prResponseSchemaacceptsbody: z.string().nullable(), but no test exercises the null-body path. If the.nullable()modifier were removed, no test would fail. Adding a case with"body": nullin the fixture or a dedicatedit()would guard this regression.🤖 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/__tests__/client.test.ts` around lines 101 - 110, Add coverage for the nullable pull request body accepted by prResponseSchema by adding a fetchPullRequest test fixture with body set to null, and assert the response succeeds with the body preserved as null. Keep the existing missing-fields rejection test unchanged.Source: Coding guidelines
src/openrouter/client.ts (1)
332-371: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd backoff between retries, especially for 429 rate-limit errors.
The retry loop immediately re-calls
attemptOnceafter a retryable failure with no delay. For HTTP 429 (rate limit), an immediate retry is almost certain to receive another 429, wasting the retry budget. Consider adding exponential backoff with jitter before the next iteration whenattemptResult.retryableis true and the loop will continue.♻️ Suggested backoff insertion
if (!attemptResult.retryable) break + + if (attemptNumber < MAX_ATTEMPTS_PER_MODEL) { + const baseMs = 1000 * 2 ** (attemptNumber - 1) + const jitterMs = Math.random() * 500 + await new Promise((resolve) => setTimeout(resolve, baseMs + jitterMs)) + } }🤖 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/openrouter/client.ts` around lines 332 - 371, In the retry loop around attemptOnce, add a delay before continuing after a retryable failure, only when another attempt will run. Use exponential backoff with jitter, increasing by attemptNumber and preferably honoring any available rate-limit or Retry-After information, while preserving immediate exit for non-retryable and abort outcomes.src/openrouter/__tests__/cost-summary.test.ts (1)
85-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer exact
toBeassertions overtoContainfor deterministic output.Tests 3 and 4 use
toContainto check only the total cost line, but the full markdown output is deterministic. UsingtoBewith the complete expected string would catch unexpected formatting changes in these scenarios. As per coding guidelines, "Prefer exact assertions and assert whole deterministic values rather than substrings or loose matchers."♻️ Suggested exact assertions for tests 3 and 4
it("sums costs across attempts when every attempt is priced", () => { const fallbackAttempt: ModelAttempt = { model: "anthropic/claude-haiku-4.5", outcome: "accepted", promptTokens: 11000, completionTokens: 600, costUsd: 0.01, errorSummary: null, } const summary = renderCostSummary({ attempts: [ { ...acceptedAttempt, outcome: "schema_mismatch" }, fallbackAttempt, ], modelUsed: "anthropic/claude-haiku-4.5", }) - expect(summary).toContain("Total cost: $0.052100") - expect(summary).not.toContain("(some attempts unpriced)") + expect(summary).toBe( + [ + "### umm-actually cost summary", + "", + "Model used: anthropic/claude-haiku-4.5", + "", + "| attempt | model | outcome | prompt tokens | completion tokens | cost |", + "| --- | --- | --- | --- | --- | --- |", + "| 1 | openai/gpt-5-mini | schema_mismatch | 12000 | 800 | $0.042100 |", + "| 2 | anthropic/claude-haiku-4.5 | accepted | 11000 | 600 | $0.010000 |", + "", + "Total cost: $0.052100", + ].join("\n"), + ) }) it("renders an n/a total when no attempt carries a cost", () => { const unpricedAttempt: ModelAttempt = { ...acceptedAttempt, costUsd: null, } const summary = renderCostSummary({ attempts: [unpricedAttempt], modelUsed: "openai/gpt-5-mini", }) - expect(summary).toContain("Total cost: n/a") + expect(summary).toBe( + [ + "### umm-actually cost summary", + "", + "Model used: openai/gpt-5-mini", + "", + "| attempt | model | outcome | prompt tokens | completion tokens | cost |", + "| --- | --- | --- | --- | --- | --- |", + "| 1 | openai/gpt-5-mini | accepted | 12000 | 800 | n/a |", + "", + "Total cost: n/a", + ].join("\n"), + ) })🤖 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/openrouter/__tests__/cost-summary.test.ts` around lines 85 - 101, Update the deterministic output assertions in the cost-summary tests around renderCostSummary, especially the cases asserting “Total cost: $0.052100” and “Total cost: n/a,” to use exact toBe comparisons against the complete expected markdown output. Preserve the existing test inputs and verify the entire rendered summary rather than only matching substrings.Source: Coding guidelines
src/context/__tests__/workspace.test.ts (1)
69-75: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse exact error-message assertions instead of
toThrow(string)(substring match).
.rejects.toThrow(string)checks that the message includes the substring, not that it equals it — a regression that changes the message format while keeping the substring would still pass. As per coding guidelines, tests should "Prefer exact assertions and assert whole deterministic values rather than substrings or loose matchers." Both escape-path tests (readConventions at Line 74, readChangedFiles at Line 162) rely on this loose match.♻️ Proposed fix: assert the exact thrown error
await expect( contextReader.readConventions({ conventionsFile: "../outside.md" }), - ).rejects.toThrow("path escapes the workspace: ../outside.md") + ).rejects.toThrow(new Error("path escapes the workspace: ../outside.md"))await expect( contextReader.readChangedFiles({ changedPaths: ["../../etc/passwd"], budgetTokens: 10_000, }), - ).rejects.toThrow("path escapes the workspace: ../../etc/passwd") + ).rejects.toThrow(new Error("path escapes the workspace: ../../etc/passwd"))Also applies to: 154-163
🤖 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/context/__tests__/workspace.test.ts` around lines 69 - 75, Update the rejection assertions in the readConventions and readChangedFiles tests to verify the complete deterministic error message exactly, replacing the substring-based rejects.toThrow(string) matcher while preserving the existing expected messages.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 `@fixtures/workspace/src/barrel-consumer.ts`:
- Line 1: Update the relative import in the barrel-consumer fixture to
explicitly reference the compiled JavaScript module by adding the .js extension
to the "./lib" specifier, while preserving the existing LIB_NAME import.
In `@src/context/workspace.ts`:
- Around line 123-160: Update readChangedFiles to stat each resolved changed
path before calling readFileOrNull, mirroring the MAX_SCAN_BYTES guard used by
scanWorkspaceSourceFiles. For files exceeding the size limit, add the existing
diff-only PromptFile entry and continue without reading their contents; preserve
current unreadable, binary, and token-budget handling for files within the
limit.
---
Nitpick comments:
In `@src/context/__tests__/workspace.test.ts`:
- Around line 69-75: Update the rejection assertions in the readConventions and
readChangedFiles tests to verify the complete deterministic error message
exactly, replacing the substring-based rejects.toThrow(string) matcher while
preserving the existing expected messages.
In `@src/github/__tests__/client.test.ts`:
- Around line 101-110: Add coverage for the nullable pull request body accepted
by prResponseSchema by adding a fetchPullRequest test fixture with body set to
null, and assert the response succeeds with the body preserved as null. Keep the
existing missing-fields rejection test unchanged.
In `@src/openrouter/__tests__/cost-summary.test.ts`:
- Around line 85-101: Update the deterministic output assertions in the
cost-summary tests around renderCostSummary, especially the cases asserting
“Total cost: $0.052100” and “Total cost: n/a,” to use exact toBe comparisons
against the complete expected markdown output. Preserve the existing test inputs
and verify the entire rendered summary rather than only matching substrings.
In `@src/openrouter/client.ts`:
- Around line 332-371: In the retry loop around attemptOnce, add a delay before
continuing after a retryable failure, only when another attempt will run. Use
exponential backoff with jitter, increasing by attemptNumber and preferably
honoring any available rate-limit or Retry-After information, while preserving
immediate exit for non-retryable and abort outcomes.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ab0ff55a-d5e4-4068-9c74-40a5bdde42ca
⛔ Files ignored due to path filters (3)
fixtures/workspace/dist/stale.jsis excluded by!**/dist/**fixtures/workspace/node_modules/decoy/index.jsis excluded by!**/node_modules/**fixtures/workspace/src/data.binis excluded by!**/*.bin
📒 Files selected for processing (35)
.gitignoreAGENTS.mdfixtures/openrouter.chat-result.jsonfixtures/openrouter.invalid-json.jsonfixtures/openrouter.schema-mismatch.jsonfixtures/pull.get.jsonfixtures/workspace/AGENTS.mdfixtures/workspace/src/__tests__/greeter.test.tsfixtures/workspace/src/barrel-consumer.tsfixtures/workspace/src/caller.tsfixtures/workspace/src/consumer.tsfixtures/workspace/src/greeter.tsfixtures/workspace/src/hub.tsfixtures/workspace/src/importers/importer-1.tsfixtures/workspace/src/importers/importer-2.tsfixtures/workspace/src/importers/importer-3.tsfixtures/workspace/src/importers/importer-4.tsfixtures/workspace/src/importers/importer-5.tsfixtures/workspace/src/importers/importer-6.tsfixtures/workspace/src/importers/importer-7.tsfixtures/workspace/src/importers/importer-8.tsfixtures/workspace/src/importers/importer-9.tsfixtures/workspace/src/lib/index.tsfixtures/workspace/src/registry.tssrc/context/__tests__/import-resolution.test.tssrc/context/__tests__/workspace.test.tssrc/context/import-resolution.tssrc/context/workspace.tssrc/github/__tests__/client.test.tssrc/github/client.tssrc/main.tssrc/openrouter/__tests__/client.test.tssrc/openrouter/__tests__/cost-summary.test.tssrc/openrouter/client.tssrc/openrouter/cost-summary.ts
The traversal guard in createContextReader was lexical only — readFile follows symlinks, so a PR could commit a link targeting files outside the workspace or inside .git/ (checkout token in .git/config under persist-credentials) and have the content pulled into the LLM prompt, which is echoed into a public review. Resolve real paths and re-check containment before reading: conventions reads throw on escape, changed-file reads degrade to diff-only with a warning. In-workspace symlinks (AGENTS.md -> CLAUDE.md) stay readable. The related-files scan was already safe (lstat-semantics dirent checks skip symlinks). Guardrail tests mutation-checked: all three fail with the containment check removed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HBJw5bhDigxu8HM2sPPZVf
- extractImportSpecifiers + isMissingFileError: multiline expression bodies wrapped in block bodies per AGENTS.md - readFileOrNull -> readScannedFileOrNull inside the factory: the silent catch now warns with path + error so scan exclusions are auditable - changed-file unreadable warn now carries the error detail - summarizeAttempts: map callback with ternary-in-interpolation extracted into describeAttempt - cost summary: nested ternary-in-template pulled into unpricedSuffix - BFS queue mutation in scanWorkspaceSourceFiles gets its justifying comment, matching the file's sequential-state comment pattern Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HBJw5bhDigxu8HM2sPPZVf
- github/client: toStrictEqual on the zero-comment review (toEqual cannot tell an absent `comments` key from `comments: undefined`); 422-fallback test now asserts the whole call array (first call shape + no third attempt); new test for rethrowing a status-less error - cost-summary: replace toContain with full-output toBe on the two deterministic renders - workspace: name directory pruning as a spec behavior (covers the previously untested hidden-directory branch); cover the unreadable scanned-file warn path via chmod 000 - openrouter/client: cover status-less network error retry, >200-char error summary truncation, unexpected generation response shape, and missing usage block; hoist the triplicated no-cost chat result into a factory Four mutations verified: each failed exactly its covering test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HBJw5bhDigxu8HM2sPPZVf
…related-file NUL guard, deleted-file log level - import-resolution: match side-effect static imports (import "./x.js") — the doc claimed static imports were matched, but the regex required from/import(/require( and missed the side-effect form - import-resolution: add index.tsx/index.jsx to directory-index candidates, matching the .tsx/.jsx suffixes extensionless resolution already accepts - workspace: exclude related-file candidates carrying a NUL byte — the same binary rule readChangedFiles applies - workspace: a missing changed file (deleted in the PR) logs at info, not warn — deletions are routine, warn implied breakage - cost-summary: doc said one row per billed attempt; every attempt gets a row, including unbilled network failures Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HBJw5bhDigxu8HM2sPPZVf
…, exact assertions - readChangedFiles stats before reading so an oversized changed file (lockfile, bundle) is demoted to diff-only without an unbounded read - abort-ladder test parameterized across 401/402/403 - zero-attempts cost summary pinned - null PR body mapping pinned - workspace escape tests assert exact error messages Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HBJw5bhDigxu8HM2sPPZVf
Owner
Author
ship-check — non-thread bot findings & deferred itemsDispositions for review-body findings that have no inline thread, plus items the ship-check pipeline deferred to the author. Sourcery overall comments (review body)
CodeRabbit nitpicks (review body)
Deferred by ship-check to the author
🔍 ship-check · pr-monitor · claude-fable-5 |
aliasunder
commented
Jul 11, 2026
expectTypeOf<ReturnType<typeof getOctokit>>().toExtend<OctokitLike>() — an @actions/github upgrade that drifts the SDK shape now fails tsc and the suite instead of surfacing at runtime in PR 2's wiring. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HBJw5bhDigxu8HM2sPPZVf
…eJsonOrNull Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HBJw5bhDigxu8HM2sPPZVf
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HBJw5bhDigxu8HM2sPPZVf
This was referenced Jul 12, 2026
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.
PR 1 of 2 for the V1 review pipeline. Ships the three I/O client modules, fully tested and intentionally unwired — orchestrate.ts composes them in PR 2, which keeps this PR independently reviewable and shrinks the dogfood blast radius.
What's here
src/github/client.ts— octokit wrappers behind a minimal structuralOctokitLike(stubs are plain objects,data: unknownforces Zod parsing).fetchPullRequest(Zod-validated PrContext),fetchDiff(diff media type; 406 →too_large),submitReviewwith the 422 body-only fallback: fires only when the rejected review carried inline comments, posts the caller-suppliedfallbackBody, never a third attempt.src/openrouter/client.ts— structured-output request (json_schema,strict: true) with a bounded retry ladder: 1 retry per model on transient/garbage failures (408/429/5xx/network, empty content, invalid JSON, schema mismatch), no retry on structural 4xx, immediate abort on 401/402/403 (the fallback model shares the key), ≤2 models, max 4 billed calls. Every attempt is recorded with usage — failed attempts were billed too. Accepted attempts missingusage.costget one best-effortgenerations.getGenerationlookup that can never fail the run.src/openrouter/cost-summary.ts— pure markdown table renderer for the job summary (row per attempt, total,n/afor unpriced).src/context/— workspace reads against real fs (tests run on a committed fixture tree, no fs mocking).readConventions(null when missing),readChangedFiles(diff-order token budgeting, full → diff-only flip, binary/unreadable degrade with a warning),findRelatedFiles(bounded reverse-import scan: 5k-file / 256KB caps, pruned dep/build dirs,.js→.tsremap, barrel resolution, import-count ranking with test files last, cap 8). All paths resolve under the workspace root with a traversal guard — diff paths are PR-author-influenced.src/main.ts— registers both credentials with the runner masker (core.setSecret) immediately after config parse; the JSON logger writes raw to stdout and bypasses::add-mask::otherwise.node_modules/+dist/decoy dirs (gitignore-negated) so the scan pruning is tested against the genuine article.Verification
npm run lint,npm run build,npm run prettier:check,docker build .all green..js→.tsremap, and removing the 401 abort each made exactly their covering tests fail, then were reverted.orchestrate.ts; live behavior is covered by the planted-bug E2E after PR 2.🤖 Generated with Claude Code
Summary by CodeRabbit