Skip to content

fix(sse): fail Zed streams without false success - #12455

Merged
diegosouzapw merged 6 commits into
release/v3.8.51from
fix/v3851-zed-stream-error-boundary
Sep 3, 2026
Merged

diegosouzapw merged 6 commits into
release/v3.8.51from
fix/v3851-zed-stream-error-boundary

Conversation

@diegosouzapw

@diegosouzapw diegosouzapw commented Sep 2, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Turn Zed Hosted status.failed frames received before model output into a canonical, sanitized
    502 readiness failure, and terminate immediately even when the upstream socket stays open.
  • Hold role/bootstrap-only frames until the first substantive delta, then preserve that partial
    output before raising a fixed public 502 stream error through the existing Chat finalization path.
  • Remove fake assistant error text and normal-success finish_reason: "stop"; partial failures now
    end with the strict Chat finish_reason: "error" frame and invoke failure/persistence callbacks
    exactly once.
  • Make client cancellation idempotent and independent of an upstream cancel() hook that stalls
    forever.

This fix is standalone and does not depend on a separate shared public-error projector change.

Security, wire, and isolation contract

Concern Guarantee
Pre-content diagnostics Only recognized message / error / code fields are selected, bounded to 512 characters, and routed through buildErrorBody()
Post-content failure Only the fixed public message Zed upstream stream failed reaches the client and completion callback
Chat framing Role + partial content are retained; one format-safe terminal error and [DONE] are emitted; neither response.failed nor a normal stop is emitted
Parent test process Performs no repository imports and does not mutate environment, globals, DB, logger, or call-log state
Child environment Explicit allowlist only: runtime path/module/locale/temp values, NODE_ENV=test, synthetic API_KEY_SECRET, and test flags; no provider credential/token, HOME, or CODEX_HOME is inherited
Child lifecycle Creates fresh DATA_DIR and plugin directory before imports, blocks ambient fetch, resets DB, closes the shared logger, restores state, and removes temp directories
Persistence proof The pipeline uses a fixture-unique model and connection ID; the local persistence callback starts empty and must receive exactly one matching 502 record

Validation

Exact candidate head: bc565e8633bcb252809fa5421f92d0ada968eb89

Class Gate Result
PASS Original TDD RED -> GREEN Initial readiness/partial fixtures exposed false HTTP 200 success; full-handler coverage also rejected leaked response.failed framing and lost content before the final implementation
PASS Shared-process contamination RED RED 4/5 under --test-isolation=none: the unrelated call-log drain assertion deterministically failed because the old Zed test changed top-level DATA_DIR/DB state
PASS Process-isolated boundary child PASS 3/3: pre-content 502/fallback eligibility, role + content + failure with an open socket, and never-resolving upstream cancellation
PASS Pure parent wrapper PASS 1/1 and asserts that the child reported 3 tests, 3 passes, 0 failures
PASS Paired isolation regression PASS 3/3 for wrapper + call-log drain under --test-isolation=none, with no concurrency override
PASS Zed/readiness/stream regressions PASS 74/74 behaviors: 71 direct regressions plus 3 child boundary tests; outer runner PASS 72/72 (71 + wrapper)
PASS Open-SSE TypeScript openSseTypecheckErrors=0
PASS ESLint / formatting Official ESLint suppressions and Prettier check passed on all owned files
PASS Repository ratchets Changelog integrity, file-size, complexity, tracked-artifacts, and git diff --check passed
PASS Secret scan secretFindings=0; the child receives no inherited provider credentials
PASS Independent adversarial production review Zero P1/high/material P2 findings after cancellation and read-ahead hardening
PASS Latest release reconciliation Merged e243b04de22da2d78900c27fde5f83f4fbc3d7f9 without conflicts; focused 52/52 tests and gates rerun on candidate bc565e8633bcb252809fa5421f92d0ada968eb89
HOLD GitHub CI Re-running on this updated draft head
HOLD Live Zed credential smoke Not run; deterministic fixtures block network access and contain no live credentials
BASE-RED Scoped executed gates None observed or attributed

Reconciliation

  • Development started from required release SHA 7802f6ea163f18f349348dbfd8710a159ac32e15
    in an isolated worktree.
  • The final branch was reconciled by merging live release tip
    e243b04de22da2d78900c27fde5f83f4fbc3d7f9; there was no overlap in the owned
    Zed production/test/changelog fragment paths.
  • Production remained byte-identical during the isolation repair:
    open-sse/executors/zed-hosted.ts SHA-256
    017686e4f41106be1cecf373dc1e939d9686c24fce93f0937758eaabde9cd4b0.
  • No merge, automerge, deployment, or live credential use is requested by this PR.

@diegosouzapw
diegosouzapw marked this pull request as ready for review September 3, 2026 23:58
@diegosouzapw
diegosouzapw merged commit 9d92d71 into release/v3.8.51 Sep 3, 2026
20 of 21 checks passed
linhdmn added a commit to linhdmn/omniroute-INITIAL_PASSWORD-fix that referenced this pull request Sep 4, 2026
…om release/v3.8.51

All 15 failing tests across shards 1-4 reproduce on pristine base c41ec7f;
this drains them at the source so every PR stops inheriting them.

Sanitizer pipeline (regressions introduced by 2265ce7's path-span rewrite):

- errorPathRedaction: credential assignments are now redacted BEFORE path spans
  resolve (errorSanitization runs an early redactLabeledCredentialAssignments pass
  in sanitizeErrorMessageWithStackPolicy), and the span scanner treats sanitizer
  output as prose boundaries — 'label=[REDACTED]', bare '[REDACTED]', and
  'label:' introducers (CREDENTIAL_LABEL_BOUNDARY mirrors BLOCKED_KEYS) stop the
  fail-closed span instead of being swallowed with it. 'with' joins
  CLEAR_PROSE_BOUNDARIES. Fixes: error-sanitizer-sk-key-qv45 (AIza 33-char
  fixture), huggingchat transport + stream boundaries, stream-handler public
  boundary, dashboard request-failed redaction, tunnel canary.

- credentialPatterns: google pattern /AIza[0-9A-Za-z_-]{35}/ → {20,} to match the
  GHSA-qv45-56jc-4wmj test oracle (the qv45 fixture key is 33 chars after AIza;
  the {35} form missed it and redactSensitiveErrorText returned it verbatim).

- error.ts SAFE_PUBLIC_ERROR_IDENTIFIERS: + huggingchat_generation_error,
  zai_stream_error — both were passing through projectPublicErrorIdentifier as
  raw provider codes before cheaper-inference#12506 added the bounded vocabulary; without them
  huggingchat-stream-error-boundary and zai-web-silent-empty-repro see
  'bad_gateway' instead of the pinned codes.

- imageGeneration.saveImageErrorResult: String(error) throws
  'Cannot convert object to primitive value' on the null-prototype objects
  sanitizeUpstreamDetails returns — use JSON.stringify. Fixes 3 codex
  image-generation tests.

- streamFailureBoundary: new keepReadable option; emitTranslatedFailureAndAbort
  (parsed.error mid-stream path) uses it so the forwarded response.failed frame
  is not discarded when the readable errors — restores the kiro
  response.failed contract from cheaper-inference#12454/cheaper-inference#12455 that 2265ce7 broke.

- chat.ts image-model rejection hint: 'Use POST /v1/images/generations instead.'
  → 'Then POST …' — 'Use' is not a prose boundary, so the fail-closed span
  swallowed the endpoint and the cheaper-inference#6457 assertion never matched.

- sync-models route: parseError messages gain a 'GET' route-context marker
  ('Invalid JSON response from GET /models') so the path redactor's route shield
  preserves the endpoint (test updated to the new strings).

- tunnel-routes canary: updated for the sanitizer's intentional growth —
  quoted-path and windows-shape leaks are now covered by the shared sanitizer;
  the canary pins the new coverage map per its own instructions.
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…2455)

Validado em lote numa worktree combinada com os 14 PRs desta campanha de error-boundary sobre o tip de `release/v3.8.51`: `typecheck:core` limpo e **120/120** nos 23 arquivos de teste que os PRs trazem.

Um ponto que só apareceu no tree combinado: **cheaper-inference#12465 e cheaper-inference#12466 criam o mesmo arquivo novo** `open-sse/utils/streamReadiness.ts` (que não existe no tip) com desenhos divergentes de cancelamento — `cancelled` + `releaseLock` imediato num, `readInFlight`/`cancelRequested` com `cancelReader` fire-and-forget no outro. Adotei a versão do cheaper-inference#12466, que difere e defere o release do lock para quando a leitura em voo termina, e validei a escolha rodando as suítes dos **dois** PRs contra ela: 21/21 no readiness compartilhado e 22/22 incluindo o boundary do Perplexity.
@diegosouzapw
diegosouzapw deleted the fix/v3851-zed-stream-error-boundary branch October 18, 2026 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant