release: v0.7.1 — provider error handling pass#794
release: v0.7.1 — provider error handling pass#794anandgupta42 wants to merge 7 commits intomainfrom
Conversation
A focused pre-release review (5-persona + 2-persona re-review) widened v0.7.1 from a single OpenAI-message-extraction fix into a tight provider-error pass. All changes are surgical, marker-disciplined, and pinned by 40 adversarial tests in `release-v0.7.1-adversarial.test.ts`. User-facing fixes: - Bedrock / AWS Lambda `errorMessage` shape now extracted by both `parseAPICallError` and `parseStreamError`. - `parseStreamError` no longer falls through to `JSON.stringify(e)` for non-OpenAI codes — uses the same string-typeof chain as the API path. - `model_not_found` short-circuits OpenAI 404 retry-storm; user sees the actionable error on attempt 1 instead of after 5 silent retries. - `altimate models` discoverability hint appended on `model_not_found`. Privacy / defense-in-depth: - `Telemetry.maskString` redacts email addresses and internal hostnames (`*.local` / `*.internal` / RFC1918 / IMDS) so the unwrapped provider text from #789 doesn't leak more PII than the prior JSON-quote shred. - `MessageV2.APIError.metadata.url` masks internal hosts (incl. IPv6 loopback / ULA / link-local, AWS IMDS) and strips basic-auth userinfo before the URL flows into local storage / share / telemetry. - `responseBody` capped at 4KB at the `parseAPICallError` boundary. Docs: - New "Provider API Errors" section in `docs/docs/reference/troubleshooting.md`. CI gates passed locally: - typecheck (`tsgo --noEmit`): clean - marker guard (`analyze.ts --markers --base main --strict`): clean - pre-release (`bun run pre-release`): all 4 checks pass; binary starts - targeted tests: 102/102 (provider/error + adversarial + branding + retry) - full opencode suite: 8011 pass / 503 skip / 0 fail Closes #788 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
Caution Review failedFailed to post review comments Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughImproves provider error parsing and fallbacks, prevents model_not_found retry storms and appends a discoverability hint, caps provider response bodies at 4KB, redacts emails and internal hosts in telemetry/metadata, adds extensive adversarial tests, and adds a Troubleshooting doc subsection. (49 words) ChangesError Handling & Privacy Improvements
Sequence Diagram(s)(Skipped — changes are parsing/masking, tests, and docs; no new multi-component runtime protocol.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![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 (6)
packages/opencode/test/skill/release-v0.7.1-adversarial.test.ts (3)
679-679: 💤 Low valueMove the
Telemetryimport to the top of the file.Mid-file imports work in Bun's test runner but are unusual and break tooling that scans the import header (e.g., bundle-analysis, codemods, some IDE refactors). Hoist this next to the
bun:test/ProviderError/APICallErrorimports at the top.🤖 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 `@packages/opencode/test/skill/release-v0.7.1-adversarial.test.ts` at line 679, The Telemetry import is currently mid-file; hoist the `import { Telemetry } from "../../src/altimate/telemetry"` statement up into the header imports alongside the existing `bun:test`, `ProviderError`, and `APICallError` imports so all module imports are grouped at the top of packages/opencode/test/skill/release-v0.7.1-adversarial.test.ts; update the import block (keeping the same path and named import `Telemetry`) and remove the mid-file occurrence to avoid tooling issues.
16-34: 💤 Low valueDuplicate
makeAPICallErrorhelper across two test files — extract to a fixture.This helper is byte-identical (modulo one optional field —
responseHeaders) to the one inpackages/opencode/test/provider/error.test.ts:7-24. Two copies will drift the momentAPICallError's constructor signature changes (and the AI SDK has changed it before). A 5-line shared module underpackages/opencode/test/provider/(or a_fixtures.tsnext to the suite) keeps both files lock-step.🤖 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 `@packages/opencode/test/skill/release-v0.7.1-adversarial.test.ts` around lines 16 - 34, Extract the duplicate makeAPICallError helper into a shared test fixture module and import it from both test suites: create a new helper (e.g., test fixture exporting makeAPICallError) that constructs an APICallError with the same fields (ensure it accepts message, statusCode, responseBody, url and preserves isRetryable/requestBodyValues), replace the local makeAPICallError definitions in release-v0.7.1-adversarial.test.ts and the other test file with imports from the shared fixture, and update any call sites to use the shared makeAPICallError so both tests stay in sync when APICallError's constructor changes.
365-385: ⚡ Quick winThis test pins the canonical hint text — the docs and changelog need to follow.
Line 383 asserts
result.message).toContain("altimate models"). That's the source of truth for what users actually see. The doc (troubleshooting.mdline 47,altimate-code models <provider>) and changelog (line 21,altimate-code models) disagree with this test. Either tighten this test to a strongerexpect(result.message).toContain("Run \altimate models` to see available models.")` so the next person to refactor the hint string can't quietly diverge from docs again, or settle the spelling and update all three call-sites together.🤖 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 `@packages/opencode/test/skill/release-v0.7.1-adversarial.test.ts` around lines 365 - 385, The test in packages/opencode/test/skill/release-v0.7.1-adversarial.test.ts is asserting a loose hint string ("altimate models") via ProviderError.parseAPICallError; change the assertion to match the canonical hint exactly by replacing expect(result.message).toContain("altimate models") with expect(result.message).toContain("Run `altimate models` to see available models."), or alternatively update the docs and changelog to match the existing test string—ensure ProviderError.parseAPICallError, the test assertion, troubleshooting.md, and the changelog use the identical phrasing.packages/opencode/src/provider/error.ts (3)
35-40: 💤 Low value404 carve-out parses
responseBodyagain — minor reuse opportunity.
parseAPICallErroralready callsjson(input.error.responseBody)once at line 274 to derivecodeFromBody.isOpenAiErrorRetryablehere re-runsJSON.parseto inspect the sameerror.code. Functionally fine (cheap), but the two checks can drift if one is updated in isolation (e.g., one looks atbody.error.code, the other atbody.code). Consider passingcodeFromBodydown or computing it once.Independent nit: the silent
catch {}at line 39 is correct here (malformed JSON on a 404 should fall through to "force retryable" — pinned by the test at line 445), but worth a one-line comment in the catch so a future refactor doesn't "fix" it into something that throws.🤖 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 `@packages/opencode/src/provider/error.ts` around lines 35 - 40, The 404 branch in isOpenAiErrorRetryable re-parses e.responseBody even though parseAPICallError already computes codeFromBody; update isOpenAiErrorRetryable (and its callers) to accept or reuse the already-parsed codeFromBody (or compute it once at the top of the error handling path) instead of calling JSON.parse again, and replace the empty catch block in that try with a one-line comment explaining that malformed JSON is intentionally ignored to force retryable behavior (preserve current test behavior); reference parseAPICallError and isOpenAiErrorRetryable and the codeFromBody value when making the change.
221-231: 💤 Low value
RESPONSE_BODY_CAPis 4096 chars, not strictly 4 KB — and the marker pushes the result slightly over.Two small consistency notes:
- The constant is in UTF-16 code units (
String.prototype.length), so a 4096-char body of 3-byte UTF-8 emojis is ~12 KB on the wire. Not a defect (caller persists strings, not bytes), but the "4KB" comment and the docs/changelog both say "4KB" — worth either renaming toRESPONSE_BODY_CHAR_CAPor measuringBuffer.byteLengthif the goal is a real byte ceiling.- The truncated output is
slice(0, 4096) + "…[truncated N chars]"≈ 4124 chars, so an "exactly at cap" body grows after capping. The adversarial test asserts< 5000so it passes, but considerslice(0, RESPONSE_BODY_CAP - MARKER_RESERVE)if you want a hard ceiling.🤖 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 `@packages/opencode/src/provider/error.ts` around lines 221 - 231, The cap currently uses RESPONSE_BODY_CAP (4096) measured in UTF-16 code units in capResponseBody and the truncation marker is appended, which can exceed the intended byte-size; either rename RESPONSE_BODY_CAP to RESPONSE_BODY_CHAR_CAP to reflect it's a character count, or change capResponseBody to enforce a true byte cap by using Buffer.byteLength to measure bytes and reserve space for the truncation marker (e.g., compute MARKER_RESERVE = Buffer.byteLength(truncationMarker) and slice the string so that Buffer.byteLength(result) + MARKER_RESERVE <= RESPONSE_BODY_CAP), ensuring the final returned string never grows beyond the intended cap.
239-268: 💤 Low value
maskInternalHostlooks solid — two optional additions for defense-in-depth.Coverage for
localhost,.local,.internal,.localhost, RFC1918, IMDS, IPv6 loopback / ULA / link-local is correct, brackets are stripped before regex match, and the basic-auth strip + hostname rewrite +u.toString()path is well-pinned by the adversarial tests.Two optional additions:
0.0.0.0is not redacted. It's a legitimate "any-interface" bind that occasionally shows up in misconfigured proxy URLs and is functionally an internal-only address. Cheap to add (/^0\./or justhost === "0.0.0.0").- IPv4-mapped IPv6 (
::ffff:10.0.0.1) bypasses the RFC1918 check. Afterreplace(/^\[|\]$/, "")the host is::ffff:10.0.0.1; nothing matches. Unlikely to appear in error URLs from real providers, but if you want full coverage it's one extra regex (/^::ffff:(?:10|192\.168|172\.(?:1[6-9]|2\d|3[01])|127|169\.254)\./i).Note:
host.endsWith(".internal")is correct —.internalis an ICANN-reserved private-use TLD as of 2024.🤖 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 `@packages/opencode/src/provider/error.ts` around lines 239 - 268, maskInternalHost currently redacts localhost, RFC1918, IMDS and IPv6 addresses but misses 0.0.0.0 and IPv4-mapped IPv6 addresses; update maskInternalHost to also treat "0.0.0.0" as internal (e.g., host === "0.0.0.0" or /^0\./) and add a regex check for IPv4-mapped IPv6 prefixes (e.g., /^::ffff:(?:127\.|10\.|192\.168\.|172\.(?:1[6-9]|2\d|3[01])|169\.254\.)/i) after the hostname bracket strip so those cases follow the same username/password strip + u.hostname = "internal-host.redacted" + u.toString() path.
🤖 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 `@docs/docs/reference/troubleshooting.md`:
- Around line 45-50: The docs, changelog and the strings emitted by
parseAPICallError in packages/opencode/src/provider/error.ts are inconsistent
about the CLI hints; choose a single canonical hint format (e.g., "altimate
models" and "altimate auth login <provider>") and update: (1) the
parseAPICallError implementation (the model_not_found and 401/gateway messages)
to emit that canonical hint, (2) the troubleshooting docs entry (the markdown
block around the model and 401 hints) to match the same commands, and (3)
CHANGELOG.md to use the identical wording; update the specific messages in
parseAPICallError (and any other helper in provider/error.ts that constructs CLI
hints) so all three sources are identical and copy-pasteable.
In `@packages/opencode/src/altimate/telemetry/index.ts`:
- Around line 1065-1069: The maskString regex in altimate/telemetry (the
.replace(...) that currently targets localhost and RFC1918) needs to be extended
to also match IMDS IPv4 (169.254.\d+.\d+) and IPv6 addresses/ranges (bracketed
IPv6 like [::1], link-local fe80::/10, ULA fc00::/7 and fd00::/8, and general
bracketed IPv6 forms) so it mirrors the provider-side maskInternalHost behavior;
update that regex in the .replace call (referenced as maskString's URL replace)
to include patterns for 169.254.x.x and IPv6 bracketed hosts and ports, and then
add corresponding adversarial test cases in release-v0.7.1-adversarial.test.ts
covering IMDS URL (e.g. http://169.254.169.254/...) and bracketed IPv6 URLs
(e.g. http://[::1]:8080/..., http://[fe80::1]/...) to ensure telemetry masking
covers these scenarios.
In `@packages/opencode/src/provider/error.ts`:
- Around line 285-291: The hint text for model_not_found is inconsistent across
the repo; standardize it to the canonical binary name "altimate models". Update
the string in provider/error.ts (where finalMessage is built when codeFromBody
=== "model_not_found") to use "Run `altimate models` to see available models.",
and then update the matching explanatory strings in
docs/docs/reference/troubleshooting.md and CHANGELOG.md to exactly the same text
so copy-pasting the command works for users.
---
Nitpick comments:
In `@packages/opencode/src/provider/error.ts`:
- Around line 35-40: The 404 branch in isOpenAiErrorRetryable re-parses
e.responseBody even though parseAPICallError already computes codeFromBody;
update isOpenAiErrorRetryable (and its callers) to accept or reuse the
already-parsed codeFromBody (or compute it once at the top of the error handling
path) instead of calling JSON.parse again, and replace the empty catch block in
that try with a one-line comment explaining that malformed JSON is intentionally
ignored to force retryable behavior (preserve current test behavior); reference
parseAPICallError and isOpenAiErrorRetryable and the codeFromBody value when
making the change.
- Around line 221-231: The cap currently uses RESPONSE_BODY_CAP (4096) measured
in UTF-16 code units in capResponseBody and the truncation marker is appended,
which can exceed the intended byte-size; either rename RESPONSE_BODY_CAP to
RESPONSE_BODY_CHAR_CAP to reflect it's a character count, or change
capResponseBody to enforce a true byte cap by using Buffer.byteLength to measure
bytes and reserve space for the truncation marker (e.g., compute MARKER_RESERVE
= Buffer.byteLength(truncationMarker) and slice the string so that
Buffer.byteLength(result) + MARKER_RESERVE <= RESPONSE_BODY_CAP), ensuring the
final returned string never grows beyond the intended cap.
- Around line 239-268: maskInternalHost currently redacts localhost, RFC1918,
IMDS and IPv6 addresses but misses 0.0.0.0 and IPv4-mapped IPv6 addresses;
update maskInternalHost to also treat "0.0.0.0" as internal (e.g., host ===
"0.0.0.0" or /^0\./) and add a regex check for IPv4-mapped IPv6 prefixes (e.g.,
/^::ffff:(?:127\.|10\.|192\.168\.|172\.(?:1[6-9]|2\d|3[01])|169\.254\.)/i) after
the hostname bracket strip so those cases follow the same username/password
strip + u.hostname = "internal-host.redacted" + u.toString() path.
In `@packages/opencode/test/skill/release-v0.7.1-adversarial.test.ts`:
- Line 679: The Telemetry import is currently mid-file; hoist the `import {
Telemetry } from "../../src/altimate/telemetry"` statement up into the header
imports alongside the existing `bun:test`, `ProviderError`, and `APICallError`
imports so all module imports are grouped at the top of
packages/opencode/test/skill/release-v0.7.1-adversarial.test.ts; update the
import block (keeping the same path and named import `Telemetry`) and remove the
mid-file occurrence to avoid tooling issues.
- Around line 16-34: Extract the duplicate makeAPICallError helper into a shared
test fixture module and import it from both test suites: create a new helper
(e.g., test fixture exporting makeAPICallError) that constructs an APICallError
with the same fields (ensure it accepts message, statusCode, responseBody, url
and preserves isRetryable/requestBodyValues), replace the local makeAPICallError
definitions in release-v0.7.1-adversarial.test.ts and the other test file with
imports from the shared fixture, and update any call sites to use the shared
makeAPICallError so both tests stay in sync when APICallError's constructor
changes.
- Around line 365-385: The test in
packages/opencode/test/skill/release-v0.7.1-adversarial.test.ts is asserting a
loose hint string ("altimate models") via ProviderError.parseAPICallError;
change the assertion to match the canonical hint exactly by replacing
expect(result.message).toContain("altimate models") with
expect(result.message).toContain("Run `altimate models` to see available
models."), or alternatively update the docs and changelog to match the existing
test string—ensure ProviderError.parseAPICallError, the test assertion,
troubleshooting.md, and the changelog use the identical phrasing.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: be84b57b-5872-41f5-9524-8d990b4d8be2
📒 Files selected for processing (6)
CHANGELOG.mddocs/docs/reference/troubleshooting.mdpackages/opencode/src/altimate/telemetry/index.tspackages/opencode/src/provider/error.tspackages/opencode/test/provider/error.test.tspackages/opencode/test/skill/release-v0.7.1-adversarial.test.ts
| // error code is model_not_found. Pairs with the retry-storm carve-out in | ||
| // isOpenAiErrorRetryable so the user sees the hint on the first attempt | ||
| // instead of after 5 silent retries. | ||
| let finalMessage = m | ||
| if (codeFromBody === "model_not_found") { | ||
| finalMessage = `${m} Run \`altimate models\` to see available models.` | ||
| } |
There was a problem hiding this comment.
Hint text doesn't match the docs or the changelog.
Three different spellings ship in this PR for the same string:
provider/error.ts:290→Run `altimate models` to see available models.docs/docs/reference/troubleshooting.md:47→altimate-code models <provider>CHANGELOG.md:21→Run `altimate-code models` to see available models.
Per troubleshooting.md line 31 / error.ts line 99 (altimate auth login), the binary name in this repo is altimate, but the changelog/docs explanatory text uses altimate-code. Pick a single canonical form (likely altimate models to match the actual installed binary referenced elsewhere in the same docs), then align the changelog blurb and the doc step to the exact string this code emits — otherwise users who copy-paste from the doc will hit "command not found".
🤖 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 `@packages/opencode/src/provider/error.ts` around lines 285 - 291, The hint
text for model_not_found is inconsistent across the repo; standardize it to the
canonical binary name "altimate models". Update the string in provider/error.ts
(where finalMessage is built when codeFromBody === "model_not_found") to use
"Run `altimate models` to see available models.", and then update the matching
explanatory strings in docs/docs/reference/troubleshooting.md and CHANGELOG.md
to exactly the same text so copy-pasting the command works for users.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/altimate/telemetry/index.ts">
<violation number="1" location="packages/opencode/src/altimate/telemetry/index.ts:1069">
P1: Internal-host masking misses AWS IMDS URLs (`169.254.169.254`), so sensitive internal endpoint strings can still leak in telemetry.</violation>
</file>
<file name="packages/opencode/src/provider/error.ts">
<violation number="1" location="packages/opencode/src/provider/error.ts:264">
P1: Basic-auth credentials are only stripped for internal hosts; URLs to non-internal hosts can still leak `user:pass@` into `metadata.url`.</violation>
</file>
<file name="CHANGELOG.md">
<violation number="1" location="CHANGELOG.md:21">
P3: Use the actual CLI command in the changelog (`altimate models`), not `altimate-code models`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
CodeRabbit and cubic flagged three classes of issue on the v0.7.1 PR.
All three are addressed here; CHANGELOG, troubleshooting doc, and code
are now consistent.
1. Hint spelling consistency (CR Minor / cubic P3)
- Three different forms shipped in the original PR:
- error.ts:290 -> "Run `altimate models` to see available models."
- troubleshooting.md:47 -> "altimate-code models <provider>"
- CHANGELOG.md:21 -> "Run `altimate-code models` to see available models."
- Aligned to the form actually emitted by the code: `altimate models`
(no dash, no provider arg). troubleshooting.md auth-login example
also aligned to `altimate auth login` to match error.ts:99.
2. maskString missing IMDS + IPv6 (CR Major / cubic P1)
- The regex covered only RFC1918 + *.local / *.internal, while the
paired maskInternalHost in error.ts already covered AWS IMDS
(169.254.169.254), IPv6 loopback ([::1]), ULA (fc00::/7), and
link-local (fe80::/10).
- Extended the regex so an error string containing the same URL is
redacted with the same coverage as the metadata.url path.
- Added query-string/fragment chars (`+`, `#`, `,`, `;`) to the
trailing char class so secrets after `?` don't survive past the
`<internal-host>` marker.
- 5 new adversarial tests for IMDS, IPv6 loopback/ULA/link-local,
and the query/fragment leak guard.
3. maskInternalHost only stripped userinfo for internal hosts (cubic P1)
- Previously, `https://user:pass@10.0.0.5/x` redacted host AND
userinfo, but `https://user:pass@api.openai.com/x` preserved
`user:pass@`. A credential in a public-host URL is at least as
sensitive as one in an internal-host URL — strip userinfo on
EVERY URL, regardless of internal/public classification.
- 1 new adversarial test for public-host userinfo strip.
Also expanded the upstream_fix marker comment in parseAPICallError to
mention all four extractor shapes (OpenAI nested, Anthropic-style top-
level, Bedrock errorMessage, legacy string error) so readers don't have
to reverse-engineer the chain.
Tests: 46 release-v0.7.1 adversarial cases (40 -> 46), 238/238 across
provider+adversarial+upgrade+retry+telemetry suites. Typecheck clean.
Marker guard --strict clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The capResponseBody helper was wrapped in markers but the two call sites in parseAPICallError (context_overflow and api_error returns) were unmarked. Marker analyzer --strict caught this. Same fix as the "hint comment expanded" pass — every modified line in an upstream- shared file needs to live inside a marker block. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed all 6 review comments (3 from CodeRabbit, 3 from cubic) in 3 follow-up commits on 1. Hint spelling consistency (CodeRabbit Minor / cubic P3) — 2. maskString missing IMDS + IPv6 (CodeRabbit Major / cubic P1) — extended the regex in 3. Public-host basic-auth userinfo strip (cubic P1) — Bonus: expanded the CI gates re-passed locally: |
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/altimate/telemetry/index.ts">
<violation number="1" location="packages/opencode/src/altimate/telemetry/index.ts:1076">
P1: Internal-host URL masking misses URLs that include basic-auth userinfo, so sensitive internal URLs can leak in telemetry.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
cubic flagged a real P1 on the previous round's commit and CodeRabbit
posted a few low-value nits worth picking up while we're here.
P1 (cubic) — `Telemetry.maskString` URL regex missed URLs carrying
basic-auth userinfo before the host. `https://admin:hunter2@10.0.0.5/x`
fell through unredacted because the regex started with the host
alternation. Added an optional `(?:[^\/\s@]+@)?` userinfo prefix so the
whole URL — credentials + host — is captured into `<internal-host>`.
CR quick wins:
- Hoisted the `Telemetry` import to the top of the adversarial test
file (mid-file imports work in Bun but tooling that scans headers
miss them).
- Tightened the `/models` hint test to assert the full canonical string
``Run `altimate models` to see available models.`` so docs / changelog
/ code can't quietly diverge again.
Defense-in-depth from CR nits:
- Added `0.0.0.0` (any-interface bind) to maskInternalHost — shows up
in misconfigured proxy URLs and is functionally internal.
- Added a one-line comment to the silent `catch {}` in
isOpenAiErrorRetryable so a future refactor doesn't "fix" it into
something that throws on malformed JSON.
Also added 2 new adversarial tests pinning the userinfo+host case and
the `0.0.0.0` case (240/240 across the 5 affected files, up from 238).
Deferred to v0.7.2: extracting `makeAPICallError` to a shared fixture
(refactor, scope creep), reusing `codeFromBody` in
`isOpenAiErrorRetryable` (signature change), IPv4-mapped IPv6
(`::ffff:10.0.0.1`) coverage, byte-vs-char `RESPONSE_BODY_CAP` rename.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed round-2 review feedback in commit P1 from cubic — CR quick wins:
CR defense-in-depth nits applied:
Deferred to v0.7.2 (low value or larger refactor):
CI gates: typecheck clean, marker guard --strict clean, 240/240 across the 5 affected suites. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/opencode/test/skill/release-v0.7.1-adversarial.test.ts`:
- Around line 112-129: The test "__proto__ in body does not pollute
Object.prototype" currently uses JSON.stringify which drops a prototype-setter
__proto__ key; change the payload passed into makeAPICallError so the JSON
contains an explicit "__proto__" own key string (e.g.
'{"__proto__":{"polluted":"yes"},"error":{"message":"harmless surface"}}')
instead of JSON.stringify({...}); keep the rest of the call to
ProviderError.parseAPICallError and the expect assertion the same so JSON.parse
in parseAPICallError receives the malicious key and the regression guard is
exercised.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 746d457b-fa3b-4adb-a206-ad4a0facc2bf
📒 Files selected for processing (3)
packages/opencode/src/altimate/telemetry/index.tspackages/opencode/src/provider/error.tspackages/opencode/test/skill/release-v0.7.1-adversarial.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/opencode/src/altimate/telemetry/index.ts
CodeRabbit caught that the __proto__ regression guard was hollow:
JSON.stringify({__proto__: ...}) produces `{}` — `__proto__` in
object-literal syntax is the prototype setter, not an enumerable
own property, so it never makes it into the JSON. The test was
serializing an empty payload and asserting prototype wasn't
polluted (trivially true).
Replace the JSON.stringify call with a hand-written JSON literal
containing an explicit "__proto__" key. parseAPICallError's
JSON.parse now actually receives the malicious key, exercising
the regression guard against a future refactor that switches
to Object.assign / spread.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Round 3: addressed the new CodeRabbit finding (commit Major (CodeRabbit): the 48 tests in the adversarial file (was 47), all passing. The constructor-prototype test was already correctly built — |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/opencode/test/skill/release-v0.7.1-adversarial.test.ts (2)
112-148: ⚡ Quick winRestore
Object.prototypein afinallyblock.If this regression ever comes back, these tests can leave
Object.prototypepolluted after the failed assertion and cascade noise into the rest of the suite. Cleaning up unconditionally keeps the failure localized.Proposed hardening
test("__proto__ in body does not pollute Object.prototype", () => { const before = (Object.prototype as any).polluted - ProviderError.parseAPICallError({ - providerID: "openai" as any, - error: makeAPICallError({ - message: "Bad Request", - statusCode: 400, - responseBody: '{"__proto__":{"polluted":"yes"},"error":{"message":"harmless surface"}}', - }), - }) - expect((Object.prototype as any).polluted).toBe(before) + try { + ProviderError.parseAPICallError({ + providerID: "openai" as any, + error: makeAPICallError({ + message: "Bad Request", + statusCode: 400, + responseBody: '{"__proto__":{"polluted":"yes"},"error":{"message":"harmless surface"}}', + }), + }) + expect((Object.prototype as any).polluted).toBe(before) + } finally { + if (before === undefined) delete (Object.prototype as any).polluted + else (Object.prototype as any).polluted = before + } })Apply the same pattern to the
constructor.prototypecase.🤖 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 `@packages/opencode/test/skill/release-v0.7.1-adversarial.test.ts` around lines 112 - 148, The tests that assert parseAPICallError doesn't pollute Object.prototype should unconditionally restore any touched prototype keys in a finally block: for both tests referencing ProviderError.parseAPICallError and makeAPICallError, capture the original value (e.g. const before = (Object.prototype as any).polluted / .injected) then run the parse+expect inside a try, and in a finally block reset Object.prototype to the captured value (i.e. (Object.prototype as any).polluted = before or delete if before === undefined, same for .injected). This ensures cleanup even on assertion failures.
472-488: ⚡ Quick winPin the 4KB truncation boundary exactly.
<5000is looser than the release guarantee here. A regression from4096to4800would still pass while leaking extra body content. Please assert the 4096-char prefix and truncation suffix explicitly.Proposed assertion tightening
test("100KB responseBody is truncated to ~4KB on the result", () => { const huge = "a".repeat(100_000) const result = ProviderError.parseAPICallError({ providerID: "openai" as any, error: makeAPICallError({ message: "Bad Request", statusCode: 400, responseBody: huge, }), }) expect(result.type).toBe("api_error") if (result.type === "api_error") { expect(result.responseBody).toBeDefined() - // 4096 + truncation marker (~30 chars), not the full 100KB. - expect(result.responseBody!.length).toBeLessThan(5000) - expect(result.responseBody).toContain("[truncated") + const prefix = "a".repeat(4096) + expect(result.responseBody).toStartWith(prefix) + expect(result.responseBody).toBe(`${prefix}…[truncated 95904 chars]`) } })🤖 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 `@packages/opencode/test/skill/release-v0.7.1-adversarial.test.ts` around lines 472 - 488, Replace the loose length check with assertions that pin the 4KB boundary exactly: when result.type === "api_error" assert result.responseBody starts with the first 4096 chars of huge (e.g. result.responseBody!.startsWith(huge.slice(0, 4096))) and assert the truncation marker begins immediately after that prefix (e.g. result.responseBody!.indexOf("[truncated") === 4096) so the test guarantees the prefix length is exactly 4096 and the truncation suffix is appended at that boundary; locate this change around ProviderError.parseAPICallError result handling in the test.
🤖 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.
Nitpick comments:
In `@packages/opencode/test/skill/release-v0.7.1-adversarial.test.ts`:
- Around line 112-148: The tests that assert parseAPICallError doesn't pollute
Object.prototype should unconditionally restore any touched prototype keys in a
finally block: for both tests referencing ProviderError.parseAPICallError and
makeAPICallError, capture the original value (e.g. const before =
(Object.prototype as any).polluted / .injected) then run the parse+expect inside
a try, and in a finally block reset Object.prototype to the captured value (i.e.
(Object.prototype as any).polluted = before or delete if before === undefined,
same for .injected). This ensures cleanup even on assertion failures.
- Around line 472-488: Replace the loose length check with assertions that pin
the 4KB boundary exactly: when result.type === "api_error" assert
result.responseBody starts with the first 4096 chars of huge (e.g.
result.responseBody!.startsWith(huge.slice(0, 4096))) and assert the truncation
marker begins immediately after that prefix (e.g.
result.responseBody!.indexOf("[truncated") === 4096) so the test guarantees the
prefix length is exactly 4096 and the truncation suffix is appended at that
boundary; locate this change around ProviderError.parseAPICallError result
handling in the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 281191d6-df9d-4e3f-859a-2444d22a5da8
📒 Files selected for processing (1)
packages/opencode/test/skill/release-v0.7.1-adversarial.test.ts
CodeRabbit Quick Wins: 1. Wrap prototype-pollution tests in try/finally. If a regression ever breaks the guards, Object.prototype stays polluted for the rest of the suite — cascading noise instead of a localized failure. The finally block restores (or deletes) the original key value. 2. Pin the 4KB truncation boundary exactly. The test was asserting `length < 5000`, which would still pass if a future refactor moved the cap from 4096 → 4500 → 4900 — none of those are the documented guarantee. Now asserts the exact 4096-char prefix and the full truncation suffix `…[truncated 95904 chars]`. Cap is now the load-bearing assertion, not the upper bound. Also bumped CHANGELOG date to today (2026-05-06) since the tag pushes today. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Round 4 (commit CodeRabbit Quick Win #1 — wrapped both prototype-pollution tests in CodeRabbit Quick Win #2 — pinned the 4KB truncation boundary exactly. Also bumped the CHANGELOG date to 2026-05-06 since the tag pushes today. 48/48 in adversarial suite. CI re-running. |
❌ Tests — Failures DetectedTypeScript — 15 failure(s)
Next StepPlease address the failing cases above and re-run verification. |
2 participants
What does this PR do?
Releases v0.7.1 — a focused provider error handling pass.
After a 5-persona pre-release review of #789 (the OpenAI nested
error.messagefix that already landed on main), reviewers surfaced 9 actionable items. Rather than defer them all to v0.7.2, this PR folds them into v0.7.1 and re-reviews the new code with a focused 2-persona pass (Tech Lead + Chaos Gremlin / regex fuzzer). The Chaos Gremlin pass surfaced 2 P1 leaks (basic-auth userinfo + IPv6 hosts) which are also fixed here.User-facing fixes:
errorMessageshape now extracted by bothparseAPICallErrorandparseStreamError.parseStreamErrorno longer falls through toJSON.stringify(e)for non-OpenAI codes — uses the same string-typeof chain.model_not_foundshort-circuits OpenAI 404 retry-storm; user sees the actionable error on attempt 1 instead of after 5 silent retries.altimate modelsdiscoverability hint appended onmodel_not_found.Privacy / defense-in-depth:
Telemetry.maskStringredacts email addresses and internal hostnames (*.local/*.internal/ RFC1918 / IMDS) — preserves the privacy shield JSON-quote-shredding incidentally provided pre-fix: [#788] surface OpenAI nested error.message instead ofBad Request: {?:?}#789.MessageV2.APIError.metadata.urlmasks internal hosts (IPv4 RFC1918, IPv6 loopback / ULA / link-local, AWS IMDS) and strips basic-auth userinfo before the URL lands on the parsed error.responseBodycapped at 4KB at theparseAPICallErrorboundary.Docs:
docs/docs/reference/troubleshooting.md.Type of change
Issue for this PR
Closes #788
How did you verify your code works?
bun run typecheck/tsgo --noEmit): cleanbun run script/upstream/analyze.ts --markers --base main --strict): cleancd packages/opencode && bun run pre-release): all 4 checks pass; built binary starts (0.0.0-main-202605052242)test/provider/error.test.ts,test/skill/release-v0.7.1-adversarial.test.ts(40 new adversarial cases),test/installation/upgrade.test.ts(branding),test/session/retry.test.tsChecklist
troubleshooting.md,CHANGELOG.md)error.tsis wrapped withaltimate_changemarkers;upstream_fix:tags applied where upstream has the same bug🤖 Generated with Claude Code
Summary by cubic
Release v0.7.1 tightens provider error handling so users see clear messages, avoid noisy retries, and keep credentials/internal hosts private. Closes #788.
Bug Fixes
errorMessage.model_not_foundstops the 404 retry storm and adds a hint: runaltimate models.altimate models.Privacy
0.0.0.0); keep token masking.APIError.metadata.urlmasks internal hosts and strips basic-auth on all URLs.responseBodyat 4KB inparseAPICallError.Written for commit c977c23. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Privacy
Tests