fix(sdk): surface the real startup failure instead of a generic one - #4168
Conversation
e38c2b2 to
f504599
Compare
|
CI caught a stale assertion my local run could not. Fixed and pushed. What failed
That test asserted the exact generic string this PR removes: // sdk-broker-lifecycle-e2e.test.ts:3295 (before)
message: "No ready SDK endpoint remains available.",Why my local run missed itThe test begins with Worth noting for anyone else touching this file: a green local run on macOS is not evidence for the Linux-gated tests in it. The fixThe test injects a real failure via I confirmed the message is safe to surface before changing the assertion: Re-verified after the fix
Rebased onto current |
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Adversarial review — signed terminal verdict: MERGE_READY at exact head e38c2b2d15dd69814245104b2860b31158374e93 (base dev @ bbb2a51b416e72fac907e9d8bd21510ab9e3ab27, refs #4146). Review-only; not merged by this lane.
Verdict
The one-line change (message: startupFailure.message in the code-less terminal response) is correct, load-bearing, and safe across every contract surface checked: sanitization/credential safety, code-less real-failure semantics, typed consumer compatibility, and ledger message authority with no log-injection surface. MERGE_READY on substance, with exactly one required follow-up — a stale test expectation the PR missed (exact patch below, already proven in an internal repair branch).
1. Sanitization / credential safety — PASS
Production failure messages only reach the artifact through normalizeSdkStartupFailure → sanitizeSdkStartupMessage (src/sdk/startup-capability.ts), which: redacts process-scoped credentials (env names matching token/secret/password/credential/api_key/auth, NFKC-normalized) to [redacted-secret]; strips control+format chars [\p{Cc}\p{Cf}] (newlines, ANSI escapes, bidi/zero-width); redacts URLs, secret query params, key=value secret patterns and bearer tokens; collapses whitespace; truncates at 512 bytes; and falls back to a reason-specific message when empty. The broker read side independently validates the shape (isSdkStartupFailure: non-empty string ≤512 bytes) and rejects the artifact otherwise. The production post-registration failure message (Lifecycle test failure after SDK host registration.) round-trips verbatim because it is already clean — verified in the E2E run.
2. Code-less real failure vs genuinely absent fallback — PASS
The code-less branch is only reachable when a valid startup-failure artifact exists (startupFailure && cleanupProof). Every valid artifact carries a non-empty, bounded message by contract (isSdkStartupFailure + writeSessionLifecycleFailure validation + atomic EEXIST-collision write). The old generic No ready SDK endpoint remains available. was suppressing a real stored message in exactly that branch; the genuinely-absent-fallback case lives in the unchanged terminal_uncertain branch (Lifecycle startup cleanup could not be proven…). No absent-message case regresses.
3. Typed/structured consumer compatibility — PASS
BrokerResponse.error.message is typed string; startupFailure.message is a validated non-empty string, so the assignment is type-safe (tsc --noEmit clean). No response shape changes — startupFailure receipt, artifactDigest, and durableEffects.startup were already in the response pre-PR (the diff is one line). Consumers tolerate the value change: python SDK treats error as dict[str, JSONValue], session-cli wraps it into SdkSessionCliError, MCP passes error.message through.
4. Ledger message authority + no injection — PASS
The surfaced error.message and startupFailure.message are byte-identical to the stored artifact record: the broker reads the same artifact via readLifecycleFailureArtifact (canonical-JSON round-trip + digest check) that it replays for idempotency, so the exact-replay invariant holds with the real message. Every output surface JSON-encodes: session-cli writeJson (JSON.stringify), lifecycle ledger rows, and broker websocket frames. No raw multiline/control-char path reaches logs or the terminal. The change is behavior-consistent with the pre-existing code'd branch, which already surfaced startupFailure.message.
5. Verification runs (exact head, this machine)
- Exact-head lifecycle E2E before repair: reproduced exactly one primary failure — the stale
No ready SDK endpoint remains available.expectation inproduction post-registration startup failure proves cleanup and exact replay; cleanup/exact-replay assertions and the richerstartupFailure/durableEffectsfields all matched. - Internal repair (NOT the contributor branch):
fix/pr-4168-test-contract-repair@e1ff621e63updates only the stale expectation to the real sanitized message and pins authority (error.message === startupFailure.message). - Full
sdk-broker-lifecycle-e2ecohort at head + repair: 60 pass / 0 fail; one load-induced timing assertion (<1000ms, received 3085ms under a 61-test parallel run) passes in isolation — environment noise, not this PR. - Mutation check — reverting the one-line change to the generic message: 58 pass / 3 fail, and all three failures are message-propagation assertions: the PR's new code-less test, the crash-replay test's normal leg (
message: "owned synthetic startup failure"), and the repaired production test. The change is mutation-sensitive, not tautological. - Typecheck:
tsc -p packages/coding-agent/tsconfig.json --noEmitclean. Biome clean on the changed test file. - Exact-head terminal CI at
e38c2b2d(prior run): 15 jobs success; the lifecycle E2E primary failure above was the only red and is now addressed by the repair.
Required follow-up (only change-request item)
The PR updated one sibling assertion but missed the other. In packages/coding-agent/test/sdk-broker-lifecycle-e2e.test.ts, production post-registration startup failure proves cleanup and exact replay still asserts the old generic message:
- message: "No ready SDK endpoint remains available.",
+ message: "Lifecycle test failure after SDK host registration.",(and optionally message: "Lifecycle test failure after SDK host registration." inside the startupFailure block to pin authority, as the internal repair does). Apply this before/at merge — it is identical to e1ff621e63 and is proven green + mutation-sensitive.
Merge ordering
- Apply the one-line test expectation update (or carry the internal repair commit
e1ff621e63). - Re-drive exact-head CI — expect green (production unchanged; the 15 terminal jobs already pass).
- Not merged by this lane.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
|
A quick follow-up on the review above (signed MERGE_READY, review id 4895044769): I verified the pushed head The optional No further changes requested. Still review-only — not merged by this lane. |
f504599 to
d061621
Compare
9a4b457 to
511452d
Compare
A code-less lifecycle startup failure was rewritten into "No ready SDK endpoint remains available.", which named a resource that was never exhausted. The live ledger records phase "startup", reason "pending", message "SDK startup did not complete before readiness cutoff." and endpointGeneration null -- startup never reached generation recording, so no endpoint or broker registration existed to run out of. Operators chasing this were sent looking for a leak. The actual limit is the semantic-readiness budget: 10s default minus two 1s cleanup/termination windows leaves the child about 8s to become ready. Lore-id: 4146a3c1 Constraint: keep the generic text for a genuinely absent failure -- only a present message replaces it Rejected: raise the readiness budget | hides whatever is slow instead of naming it Rejected: keep the generic message and log the real one | operators read the error, not the ledger Confidence: high Scope-risk: narrow Reversibility: easy Tested: restoring the generic message turns 62 pass / 0 fail into 60 pass / 2 fail Not-tested: which startup stage consumes the 8s -- that needs per-stage timing in the failure artifact
511452d to
c93ea3a
Compare
Refs #4146. Fixes the diagnosability half — the part that made this issue hard to work on.
The message was lying
session.createfails with:That names a resource which was never exhausted.
packages/coding-agent/src/sdk/broker/lifecycle.ts:4404-4407rewrote any code-less startup failure into that string, discarding the real one.The live ledger on my machine records the truth — 33 occurrences of:
{ "phase": "startup", "reason": "pending", "message": "SDK startup did not complete before readiness cutoff.", "endpointGeneration": null, "code": null }endpointGeneration: nullis the giveaway: startup never reached generation recording (sdk/bus/index.ts:5529-5530), so no endpoint or broker registration existed to run out of.code: nullis why the rewrite branch fired.What is actually exhausted
The semantic-readiness time budget, not a pool:
startup-budget.ts)lifecycle.ts:105-125)commands/sdk.ts:615-673fails whensemanticReadyDeadlineAtwins thePromise.raceI chased a leak first — I posted a wrong "4336 dead index rows" figure on the issue and corrected it there. The index is an append-only event log; folded, it is ~11 net-registered / 10 alive, and the reaper (
lifecycle.ts:4549, sweep at:4581) works. Anyone reading that first comment should read the correction.Fix
One line: when a startup failure carries a message but no code, surface its message rather than the generic one. A genuinely absent failure still gets the old text.
Deliberately not done
Verification
bun test packages/coding-agent/test/sdk-broker-lifecycle-e2e.test.tsbun --cwd=packages/coding-agent run check:typesorigin/dev)Mutation re-run by me against the committed branch, and typecheck included — I shipped a regression earlier today that every test passed and only
tsccaught.Ruled out while investigating
lifecycle-ledger.ts:112-117, 494-600). The 279 MB.corrupt.1predates that bound.broker-spawn.logcovers broker spawning, not detached session-host stderr.session.createagainst a healthy broker. SDK discovery can disappear under concurrent lane launch; session-index lock and notification ownership leave TUI uncontrollable #4146 has two distinct causes and this PR addresses neither the race nor the timeout itself — only the misreporting.Still open on #4146
Which startup stage consumes the ~8 s. The evidence cannot distinguish model-profile startup from extension initialization from waiting on the SDK capability, because detached child stderr is discarded at
lifecycle.ts:3248-3252. Pinning it needs the active stage plus elapsed/remaining ms persisted into the failure artifact aroundcommands/sdk.ts:632-657. Keeping the issue open for that.