[Critical] isAlreadyFormatted() only recognizes already-formatted messages when the whole string ends with ], but 429/rate-limit formatting appends quota guidance after the closing bracket. If one of those formatted 429 messages is parsed again, this guard does not match and the message can still be double-wrapped as [API Error: [API Error: ...]...]; for StructuredError with status === 429, the second pass can also append quota guidance again, possibly with a different auth-specific suffix.

Please make the idempotency check accept the known 429 suffix form, for example by detecting the formatted [API Error: ...] header without requiring the entire trimmed string to end with ], and add a regression test for an already-formatted 429 string with appended quota guidance.

— gpt-5.5 via Qwen Code /review

AlreadyReportedError is already imported here, so error instanceof AlreadyReportedError would be simpler and consistent with index.ts:98. The duck-typing check duplicates the logic from errors.ts without the helper.

[Suggestion] API_ERROR_PREFIX is exported (and re-exported via packages/core/src/index.ts) but has no external consumer — its only reader is isAlreadyFormatted() two lines below, which is itself unexported. The header comment also says "See ALREADY_FORMATTED below" but the symbol below is named isAlreadyFormatted, not ALREADY_FORMATTED — that reference is dangling.

Future-maintainer trap: a reader sees an exported API_ERROR_PREFIX with a comment promising callers can detect already-formatted strings, hunts for ALREADY_FORMATTED, finds nothing, then either gives up or re-implements the prefix-only check (the broken-on-429 logic this PR just fixed) instead of consolidating on one tested entry point.

Pick one:

• Drop the export. API_ERROR_PREFIX becomes a file-local const and the comment loses the misleading "Exported so callers can detect…" line.
• Promote isAlreadyFormatted to the export. Add export to its declaration, fix the comment to say "See isAlreadyFormatted below for the matching guard", and external callers get a single tested entry point.

via Qwen Code /review

[Suggestion] isAlreadyFormatted and getRateLimitMessage are coupled by hand: every authType-specific suffix added to the switch in getRateLimitMessage (lines 18-28) must also be appended to the three-way || chain at lines 58-62. Today there are five AuthType values but only three distinct suffix constants (USE_OPENAI / QWEN_OAUTH / USE_ANTHROPIC all fall through to _DEFAULT), so the coupling happens to be intact — but it's a side effect, not an invariant.

When a future patch adds, say, case AuthType.USE_OPENAI: return RATE_LIMIT_ERROR_MESSAGE_OPENAI;, the idempotency guard silently drops to "matches none of the three known suffixes", and the OpenAI 429 path quietly regresses to [API Error: [API Error: ...]]\n<openai message> — exactly the bug this PR fixes. The omission is invisible at the diff site (the developer is editing getRateLimitMessage, not isAlreadyFormatted).

Suggested fix: drive both functions from a single source of truth — e.g. const RATE_LIMIT_SUFFIXES = { USE_GEMINI: '...', VERTEX: '...', DEFAULT: '...' } as const; — and have isAlreadyFormatted iterate over Object.values(RATE_LIMIT_SUFFIXES). Pair with a parametric test that loops every AuthType value through getRateLimitMessage and asserts the constructed [API Error: foo]<suffix> round-trips through parseAndFormatApiError unchanged.

via Qwen Code /review

[Suggestion] Three different detection conventions for the same marker:

• errors.ts:46-50 — duck-type helper (error.isAlreadyReported === true)
• index.ts:102 — instanceof AlreadyReportedError
• nonInteractiveCli.ts:779 — instanceof AlreadyReportedError

The discriminator field isAlreadyReported = true as const (line 35) carries the JSDoc "Discriminator so tests and other consumers don't need instanceof" — but errors.test.ts imports the class and uses new AlreadyReportedError(...) directly, no consumer reaches for the discriminator. There's also no cross-realm scenario in this repo (no vm, no Workers in the CLI bundle) that would justify duck-typing over instanceof.

The current half-and-half is the worst of both worlds: a future maintainer adding a fourth detection site has no rule to follow. Pick one:

• Drop the discriminator and helper, use instanceof everywhere. Consistent with the two existing call sites; no behavior change.
• Export the helper, use it everywhere. Add a comment "isAlreadyReported(err) instead of err instanceof AlreadyReportedError to tolerate cross-bundle class-identity mismatches" so future readers know why the indirection exists.

The @yiliang114 comment was about the call-site duck-typing in nonInteractiveCli.ts (correctly fixed); this is the orthogonal helper-vs-instanceof inconsistency that remains.

via Qwen Code /review

[Suggestion] Both new "already-formatted 429" idempotency cases (lines 133-138 and 146-151) pin the same RATE_LIMIT_ERROR_MESSAGE_DEFAULT suffix ("Possible quota limitations in place..."). The other two suffix branches in isAlreadyFormatted — RATE_LIMIT_ERROR_MESSAGE_USE_GEMINI ("...request a quota increase through AI Studio...") and RATE_LIMIT_ERROR_MESSAGE_VERTEX ("...through Vertex...") — are not covered.

This is exactly the regression class @wenshao's prior [Critical] comment was protecting against. If a future refactor renames a quota-suffix constant or drops one branch from the || chain at errorParsing.ts:58-62, USE_GEMINI- or VERTEX-authenticated 429s would silently regress to double-wrap and these tests would still pass.

Suggested addition:

it('returns an already-formatted 429 USE_GEMINI message unchanged', () => {
  const formatted =
    '[API Error: Rate limit exceeded]\nPlease wait and try again later. To increase your limits, request a quota increase through AI Studio, or switch to another /auth method';
  expect(parseAndFormatApiError(formatted, AuthType.USE_GEMINI)).toBe(formatted);
});
it('returns an already-formatted 429 VERTEX message unchanged', () => {
  const formatted =
    '[API Error: Rate limit exceeded]\nPlease wait and try again later. To increase your limits, request a quota increase through Vertex, or switch to another /auth method';
  expect(parseAndFormatApiError(formatted, AuthType.USE_VERTEX_AI)).toBe(formatted);
});

via Qwen Code /review

[Suggestion] The 'in JSON mode' describe block has no test for the isAlreadyReported short-circuit at errors.ts:160-167 — only the TEXT-mode case is pinned (line 208). The JSON-mode branch allocates a JsonFormatter, picks customErrorCode ?? error.exitCode, calls formatter.formatError, writes via writeStderrLine, and exits with getNumericExitCode. None of that is exercised.

A refactor of JsonFormatter.formatError or the exit-code resolution path could silently break JSON-mode error reporting for AlreadyReportedError — exactly the regression category this PR set out to make robust. The failure surface is "headless JSON consumers see the wrong exit code or a malformed error envelope".

Suggested test (sibling to the existing TEXT-mode case):

it('does not reformat or reprint AlreadyReportedError in JSON mode', async () => {
  // ...mode=JSON setup...
  const reported = new AlreadyReportedError('[API Error: 402 ...]', 42);
  await expect(handleError(reported, mockConfig)).rejects.toThrow();
  expect(mockWriteStderrLine).toHaveBeenCalledTimes(1);
  expect(mockWriteStderrLine.mock.calls[0][0]).toContain('"errorCode":42'); // or whatever JsonFormatter envelope shape
  expect(processExitMock).toHaveBeenCalledWith(42);
});

via Qwen Code /review

[Suggestion] This block — the part of the fix responsible for suppressing the literal An unexpected critical error occurred:\n<stack trace> framing called out in the bug report — has zero test coverage. There is no index.test.ts, and the runNonInteractive regression tests can't reach main().catch (they call runNonInteractive directly).

Deleting these three lines (if (error instanceof AlreadyReportedError) { process.exit(error.exitCode); }) would not fail any test. The control flow falls through to console.error('An unexpected critical error occurred:') plus the stack trace — exactly the original bug.

Two options to pin this:

1. Pin the producer end via the regression test. In nonInteractiveCli.test.ts:902, replace rejects.toBeTruthy() with rejects.toBeInstanceOf(AlreadyReportedError) (see separate inline). That at least guarantees runNonInteractive throws the marker; the index.ts catch then just has to recognize it.
2. Add a small index.test.ts. Mock main to reject with new AlreadyReportedError('x', 7); assert console.error was NOT called with the framing string and process.exit was called with 7.

Both together is even better — option 1 pins the contract, option 2 pins the consumer.

via Qwen Code /review

[Suggestion] rejects.toBeTruthy() only verifies that something was thrown. It does not pin the contract that the rest of the fix depends on:

• index.ts:102 short-circuits on error instanceof AlreadyReportedError. If a refactor of this code path replaces AlreadyReportedError with a plain new Error(text) while still preventing the double-wrap, this test passes — but the index.ts short-circuit silently breaks, and the user is back to seeing An unexpected critical error occurred: + stack trace (exactly the symptom the bug report names as item Are you interested in AI Terminal? #4 in the "before" output).
• error.exitCode === 1 is also unverified.

Suggested tightening:

const thrown = await expect(
  runNonInteractive(/* ... */),
).rejects.toBeInstanceOf(AlreadyReportedError);
// or if you'd rather assert against the rejected value:
await expect(runNonInteractive(/* ... */)).rejects.toMatchObject({
  isAlreadyReported: true,
  exitCode: 1,
});

This pins the marker contract end-to-end (producer and consumer) without adding a separate index.test.ts.

via Qwen Code /review

[Suggestion] This second throw new AlreadyReportedError(errorText) mirrors the first-turn loop at line 399, but the regression test at nonInteractiveCli.test.ts:867 only exercises the first. Every test in the suite mocks isCronEnabled to false (line 161-162), so the drain-loop branch is unreachable from CI today. A future refactor that changes the throw class here (or drops the throw) would silently re-introduce the [API Error: [API Error: ...]] double-wrap for cron / background-task notification flows, and CI would stay green.

Consider adding a sibling regression test that enables isCronEnabled, seeds a job whose itemStream yields a GeminiEventType.Error event, and asserts the same triple the first-loop test pins: rejects.toBeInstanceOf(AlreadyReportedError), no [API Error: [API Error: substring, and at most one [API Error: per stderr line.

— claude-opus-4-7[1m] via Qwen Code /qreview

[Suggestion] The skipAdapterEmit guard is the part of the fix that suppresses the duplicate [API Error: ...] stderr line (per the comment at lines 772-778), but the regression test at nonInteractiveCli.test.ts:867 doesn't directly pin this behavior. The existing assertion (nonInteractiveCli.test.ts:928) counts [API Error: occurrences per line — if a future refactor deletes this guard, two separate stderr writes appear (the stream-error line and the adapter's emit), each with only one prefix per line, and the per-line assertion still passes. The test comment at 874-877 explicitly says "JsonOutputAdapter also emits the result message on the error path, which legitimately hits stderr in TEXT mode" — that's now stale: with this guard the adapter intentionally does NOT emit in TEXT-mode AlreadyReportedError.

A total-count assertion would close the gap. The test already joins all stderr writes into stderrOutput at line 909 — adding the following one-liner pins the contract:

expect(stderrOutput.match(/\[API Error: /g)?.length ?? 0).toBe(1);

Also worth refreshing the comment at 874-877 to reflect post-PR behavior.

— claude-opus-4-7[1m] via Qwen Code /qreview