feat(kb): platform hardening batch — coverage v2, box naming, MCP multi-key, test-session resilience, custom prompt fix#446
Merged
Conversation
Media assets (images under assets/) are document attachments, not first-class
sources. Coverage v2 auto-accounts an image against any document that embeds it
in its body, once that document is itself cited or excluded — so compiled_from
no longer needs a token per image and the exclusion ledger no longer needs a
row per image.
Motivation
- compiled_from bloat: aggregated cf drowned in image tokens (UHI 昇腾.md: 139
entries = 35 docs + 104 images), unreadable to a human.
- exclusion-ledger noise: images as first-class sources forced ~563 of UHI's
603 exclusion rows to be mechanical asset entries.
- attribution gap: the schema had no way to express "which document embeds this
image", so mediaverify and the collapse UI each guessed with heuristics.
Math (v2)
auto = { a in media_assets | exists d in (cited | excluded): edge(d->a) }
unaccounted = sources - cited - excluded - auto
- edges are derived from the raw tree (each document's body ![](), [](), and
<img src> links), resolved relative to the document + posixpath.normpath +
URL-decoded; a link to a nonexistent/non-media path yields no edge, no error.
- an ORPHAN image (embedded by no accounted document — upload residue) is NOT
auto-attached; it stays unaccounted and must be excluded with a reason so a
human still sees it (anti fail-open).
- a directly-cited asset still counts as cited (v1 compatibility).
Monotonicity (old green libraries stay green)
v2 only ADDS an accounting path, so unaccounted can only shrink. All pre-existing
selfcheck tests pass unchanged — none asserted "unreferenced asset => unaccounted"
over an assets/ path (the media-asset predicate requires an assets/ segment,
which no existing test used).
Sheet-placeholder carve-out
assets/sheets/*.md table placeholders are content files, not media
(is_media_asset requires an image extension). They stay first-class sources —
auto-attaching them would launder "the table data was never compiled" into
"covered".
Observability
coverage adds auto_attached (count) and auto_attached_sample (<=20 asset<-via
edges), so auto-accounting is never a silent fail-open.
Two-repo fixture
kbc/platform/pod/fixtures/asset-provenance/ is the shared, byte-for-byte
contract (raw tree + candidate + EXCLUSIONS + expected.json). sicore's adoption
ledger will assert the SAME expected.json. Cases: relative link, ../ cross-dir,
HTML <img>, URL-encoded path, missing target (no edge), 0-byte download-failed
placeholder, assets/sheets/*.md (not media), image shared by a cited and an
unaccounted document, orphan image, image inheriting a document's exclusion,
directly-cited asset.
S1b: box constitution now tells the compile agent to cite documents only —
images auto-attach; do not cite or exclude them one-by-one; orphan images still
need a reasoned exclusion; assets/sheets/ placeholders handled as usual.
Follow-up to coverage v2. media_citing_pages() gains a third discovery path: a candidate page that cites a DOCUMENT d in its compiled_from now inherits the image numeric-fidelity check of every image d embeds in its body (via asset_attribution_edges), unioned with the existing two paths (directly-cited compiled_from images + body (source: img) citations), which legacy pages keep using. Why: S1b tells the compile agent to stop citing images one-by-one — they auto-attach. media_citing_pages() drove the blind fresh-eyes transcription check off cited images only, so without this the numeric check would silently stop covering embedded charts — a silent fail-open on the exact fidelity risk it exists to catch, in the window before the image-audit dimension lands. Edge assets are intersected with IMAGE_SOURCE_EXTS (the transcription surface), so a media asset like .tiff is accounted by coverage but not numerically re-read. Additive only: no existing media test used an assets/ raw path, so all prior assertions pass unchanged.
Two implementation-level details locked byte-for-byte with the sicore adoption ledger (shared fixture, DESIGN §六), after cross-checking the two implementations: 1. is_media_asset segment match is now case-INSENSITIVE (`Assets/`, `report.ASSETS/` count too). The platform always writes lowercase `assets/`, so this only affects hand-authored trees, but both repos must agree. Locked by a unit test, NOT the shared fixture: an uppercase directory is not portable on a case-insensitive filesystem (it collides with `assets/`). 2. document_link_targets truncates a `#fragment` AND `?query` from the target, and does so on the STILL-ENCODED string BEFORE percent-decoding (new helper _strip_fragment_query). This matches the ledger's parse order (strip angle → truncate #/? → unescape), so an encoded %23/%3F inside a real filename survives while an actual delimiter is removed. Everything else already matched (ext set incl. tiff / no pdf, both quote styles for HTML <img>, resolve-then-membership skip rule, single unquote, Clean(Join) keeping leading ..). The shared fixture gains a `?query`-suffixed target case (raw/extras.md → assets/q.png?v=2). Neither change alters any prior fixture output; expected.json is regenerated from the reference implementation with an in-generator sanity assert on the invariants. All selfcheck + mediaverify tests green.
…to kbc-box Motivation (operations): during production troubleshooting every pod was named `agentbox-<id>`, so a KB compile box and a chat agentbox could only be told apart by label/image filtering — the compile box has bitten on-call more than once. Make the compile box distinguishable at a glance. Changes: - Image rename `kbc-compile-box` -> `siclaw-kbc-box` (Makefile build/push, helm `siclaw.compileBoxImage` derivation, values, README/docs). The explicit `agentbox.compileBoxImage` override semantics are unchanged. - Pod-name prefix declared on the BoxProfile (`podNamePrefix`): `kb-compile` and `kb-compile-codex` spawn as `kbc-box-<id>` instead of `agentbox-<id>`. Derived resources (cert Secret, hostname) follow the prefix. Chat agentboxes and the read-only, ephemeral `kb-test` box keep the `agentbox-` prefix. - Upgrade compatibility: on the first post-upgrade compile spawn the spawner reaps any pod left under the old `agentbox-<id>` name for that agent (guarded to compile boxes only — a chat box under the same name is never touched), so old and new pods do not coexist. - Keep AgentBoxManager consistent with the spawner: its pod-name lookup is now profile-aware too, and `stop(runId, profile)` / `getAsync(runId, profile)` receive the run's profile. Without this the capability reap would 404 on the old name and leak the renamed compile pod, and adopt would miss the live box. Transition: - `make push-kbc` double-pushes the same image digest under the legacy name `kbc-compile-box` for one release cycle so runtimes still pinned to it keep resolving. Remove the legacy tag/push next release. - DevOps: add a registry replication rule (ap-southeast -> cn-shanghai) for `siclaw-kbc-box` (the old name was not in the rule and had to be pushed by hand). Until then, push `siclaw-kbc-box` to cn-shanghai manually for prod. Tests: box-profile / k8s-spawner / manager suites green, including new coverage for the kbc-box prefix, the legacy-pod reap, the chat-pod guard, and the profile-aware stop/getAsync lookups.
One adapter process could previously bind only a single A2A key, so
switching Siclaw agents meant editing config and restarting. Passing the
key as a tool argument is not an option: credentials must never flow
through the model context.
Add configuration-side named keys, selected at call time by alias:
- New SICLAW_A2A_KEYS env: JSON {alias: key}. Aliases match
^[a-z0-9][a-z0-9_-]{0,31}$. The single-key SICLAW_A2A_KEY /
SICLAW_A2A_KEY_FILE form is unchanged and maps to the reserved "default"
alias; the two forms may be combined. SICLAW_AGENT_ID still pins the
default key and is rejected alongside SICLAW_A2A_KEYS. Every key is
self-resolved once at startup and any failure aborts boot (fail-fast).
- Every tool gains an optional "agent" alias argument (never a key). Tool
descriptions are populated at startup with the configured aliases and
their resolved agent ids. With one agent "agent" is optional; with
several, a create/list call that omits it errors with the alias list
rather than guessing.
- task_id -> alias is tracked in process for the server lifetime, so
wait/get/cancel auto-route to the creating key. A mismatched "agent"
argument is overridden by the recorded creator and noted in the result.
siclaw_list_tasks without "agent" aggregates across every key, tags each
task with its alias, and rebuilds the map after a restart.
Keys stay in configuration only: never a tool parameter, never logged,
never present in an error message (errors carry aliases and agent ids).
Route the new logic through an AgentRouter; keep SicoreA2aClient per-key.
Existing tests updated to the router API; add router unit tests, multi-key
routing tool tests, and a two-alias stdio e2e cold-start smoke.
…fecycle logs
Three production-confirmed defects in the read-only test-session path
(kbc/platform/pod/compile_box.py), all box-side:
1. Test turns hung on "thinking" forever. The test-session receive loop
(test_session_driver) was a bare `async for msg in receive_messages()`
with no model-stall watchdog — unlike the compile path, which runs
_consume_turn_stream under _model_stall_watchdog. A black-holed model
request (gateway accepts but never responds) left the turn active and the
UI spinning indefinitely, with no recovery.
Fix: _test_stall_watchdog + _consume_test_turn_stream reuse the compile
idle-latency semantics (any SDK event == alive; a pending read tool relaxes
to the tool bound). On a stall the turn is reaped once (no retry):
interrupt the SDK stream, emit a turn-level error + turn_done so the UI goes
idle, and keep the session live for the next question. The receive loop
discards the torn-down ResultMessage. Bound defaults to the compile idle
bound, overridable via KBC_TEST_MODEL_IDLE_TIMEOUT_S.
2. Orphaned sessions exhausted the concurrency cap. TEST_SESSIONS had no idle
TTL and no way to enumerate sessions, so a server timeout or a persistence
failure left box-side sessions that nobody closed, filling
KBC_MAX_TEST_SESSIONS (3) until pod recreation.
Fix: TestRun now tracks created_at / last_activity_at / a monotonic idle
marker (touched on open, each turn, and event consumption). A periodic
sweep (_test_session_reaper, KBC_TEST_SESSION_IDLE_TTL_S default 1800s)
tears down idle sessions and emits a close event. New GET /test-sessions
returns {"sessions":[{tid,parent_run_id,created_at,last_activity_at,done}]}
(wire contract agreed with the consumer).
3. Zero lifecycle observability. The box only printed its startup line, so pod
logs showed nothing for test-session open/close/turn activity.
Fix: _print_test_lifecycle emits one stdout line per lifecycle event
(test.open / test.close / turn.start / turn.done / turn.stalled / ttl.reap),
carrying tid + parent_run_id only — never user content.
Constraints held: CompileRun behavior untouched; destream/unrelated areas not
touched; test sessions keep faithful streaming (no de-stream shim), so the new
watchdog is their only stall guard.
Tests (test_compile_box.py, +3): stall reaper frees a wedged turn and keeps the
session live for a follow-up question; idle-TTL sweep drops orphans and spares
active sessions; GET /test-sessions returns the agreed wire shape. Full suite
green (69 passing).
…session limit
Wire the box-side test-session resilience through the TS gateway so the consumer
can reach it, aligned with the sicore side.
1. capability.testSessions (src/gateway/server.ts): new RPC method proxying the
box's GET /test-sessions. Read-only reconciliation — mirrors testClose's box
discovery and NEVER spawns/rehydrates a box just to list (absent/dead box →
empty list). Box rows pass through verbatim; the `tid` wire field is
load-bearing for the consumer and is not renamed. Response:
{run_id, sessions:[{tid,parent_run_id,created_at,last_activity_at,done}]}.
2. Stable 429 error code (kbc/platform/pod/compile_box.py): the box's
"too many concurrent test sessions" refusal now returns the structured
{error:{code,message,retriable:false}} shape (same convention as
handle_test_recommendation) with code "test_session_limit". The agentbox
error mapping (agentBoxResponseError → wrapRpcError) passes the code +
retriable=false through unchanged. Without this a bare 429 mapped to the
generic TOO_MANY_REQUESTS with retriable=true — indistinguishable from a
model rate-limit 429 and wrongly retriable. Not retriable by design: the
caller must close an idle session, not auto-retry.
3. Wire contract (src/gateway/capability/contract.ts): CAPABILITY_TEST_SESSIONS,
the request/response/summary interfaces, and TEST_SESSION_LIMIT_ERROR_CODE
documented so the box↔runtime↔consumer contract is single-sourced.
Tests: new src/gateway/server-capability-test-sessions.test.ts (passthrough with
tid preserved / empty on absent box / run_id required); box test asserts the
structured 429 body. tsc --noEmit clean; gateway vitest green; box suite 69
passing.
Two P2 follow-ups from the sicore MR review, layered on the test-session work. 1. testStart idempotency (kbc/platform/pod/compile_box.py + gateway): handle_open_test accepts an optional `idempotency_key`. A retried open with the same key returns the SAME live session (same tid/snapshot_hash/pages) with `idempotent_replay: true`, instead of opening a second session or consuming a second concurrency slot — so a lost ack / client retry cannot leave a ghost session. Checked before the cap (a replay is never 429'd) and before packing (no snapshot work). The key→tid map is process-local and cleared with the session on teardown, so a torn-down key opens fresh and can never replay a dead tid. Absent key → behavior unchanged (old-consumer compat). The gateway's capability.testStart passes the field through and, crucially, SKIPS starting a second event relay on an idempotent_replay — the box's /test-events is single-consumer, so a duplicate relay would split the stream. 2. Run-scoped listing (gateway): capability.testSessions filters the box's rows to the requested run_id. A shared box (SICLAW_COMPILE_BOX_ENDPOINT) hosts sessions for multiple parent runs; one run must never see or reap another's. Double protection with the consumer's own ParentRunID check. Field name `idempotency_key` (aligned with sicore). Tests: box test asserts same-key replay → same tid + replay flag + unchanged active count, different key → new session, teardown drops the key (fresh afterwards), no-key never deduped; gateway test asserts a shared box's two runs are mutually invisible in the list. tsc --noEmit clean; gateway vitest green; box suite 70 passing.
Follow-up to the testStart idempotency change: the relay-skip guard only fires on the rare idempotent-replay path, and a future refactor of the testStart handler could silently drop it — splitting the box's single-consumer /test-events stream. Extract the decision into a pure predicate (shouldRelayTestSession) in the relay module and unit-test both branches (replay → no relay; fresh open or flag absent → relay); the handler now calls the predicate instead of an inline check. Behavior unchanged. tsc clean; test-relay vitest green.
Align the testStart idempotency key with sicore (MR!696) and the codebase's existing convention: every consumer-minted idempotency key here is a `<noun>_id` (message_id for a turn, command_id for a typed command, operation_id for a mutation receipt) — there is no `idempotency_key`. So the field is `client_request_id`, not the `idempotency_key` the first pass used. Behavior is unchanged: same client_request_id → same live session (checked before the cap and before packing), empty/absent → open fresh. Box read + TestRun attribute + teardown cleanup, the gateway request type + passthrough, and the box test's wire bodies all renamed. box suite 70 green; tsc clean; gateway vitest green.
chat.send only read params.systemPrompt when building the box prompt. sicore's web-chat proxy never forwards that param, so a custom-prompt agent silently fell back to AgentBox's built-in default SRE persona on sicore web chat — while channel paths (dingtalk/lark) resolved the agent's own template and worked. Root-cause chain: - sicore sends chat.send without systemPrompt (proxy/handler.go). - runtime chat.send only honored params.systemPrompt -> undefined -> box uses the default template. - resolveAgentSystemPrompt (config.getAgent -> agents.system_prompt) already existed but was wired only into dingtalk/lark/task-coordinator/ delegate paths, not the web chat main path. Fix (runtime-side fallback, sicore unchanged): when the caller does not forward systemPrompt, chat.send falls back to resolveAgentSystemPrompt. An explicitly forwarded prompt (portal-standalone) still wins as-is; a resolve failure / no custom prompt falls back to undefined = built-in default, so a lookup failure never turns a chat into a failure. Session-reuse semantics unchanged: AgentBox applies the template only at session creation, so this affects NEW sessions; a warm multi-turn session keeps the prompt it was created with. chat.send is the only prompt- creating entry point (chat.steer targets an already-running session via steerSession), so no other handler needs the fallback. Adds server-chat-system-prompt.test.ts covering: explicit param wins and skips the lookup; omitted param resolves the agent's custom prompt; no custom prompt / lookup failure -> undefined default (send still acks ok).
Support multiple named A2A keys selected by alias in mcp/sicore-a2a-adapter.
Rename compile box image to siclaw-kbc-box and pod prefix to kbc-box; upgrade cleans old pods.
selfcheck coverage v2 (media assets auto-attach via citing docs) + fixtures + box role discipline + mediaverify interop.
Apply agent custom system prompt fallback on the web chat path (gateway chat.send).
Box turn-stall watchdog, idle-TTL reaper, test-session list; gateway proxy/idempotency + relay predicate.
This was referenced Jul 23, 2026
…le pods Integration fix for the #442 × #444 merge interaction. #442 renamed the compile-box pod prefix (agentbox- → kbc-box-, profile-derived) and updated onReap/onAdopt to pass rec.profile, but left the other capability call sites resolving the pod name with the default "agentbox" prefix. On main that was correct (every box was agentbox-); after the rename those sites silently point at a pod name the compile box no longer uses, so in K8s mode: - capability.testClose reports a LIVE test session as already-closed - capability.testSessions (new in #444, which reused testClose's pattern) lists no sessions for a live compile run - capability.cancel and the ensureCapabilitySession cleanup paths 404 on stop() and leak the pod instead of reaping it (ghost cleanup) Pass the run's profile at every capability-box site so getAsync/stop build the kbc-box- prefix. The profile comes from the in-memory run record, or — for the restart-safe paths (testClose/testSessions/cancel) where the record may be gone — is recovered from the consumer store via adopt(notifyOnAdopt:false), which never reattaches a relay or spawns a box. Unknown profile falls back to the default prefix (the pre-rename behavior). Chat-agent paths (chat.sessionStatus, agent.clearMemory, agent.terminate) are intentionally left on the default prefix — they operate on chat agentIds, not capability runs. Tests: memory-cold testClose/testSessions now assert getAsync is called with the recovered "kb-compile" profile (→ kbc-box-<id>); cancel asserts stop carries the profile for both in-memory and store-only runs.
chent1996
previously approved these changes
Jul 23, 2026
chent1996
left a comment
chent1996
left a comment
Collaborator
There was a problem hiding this comment.
Review
Approving, with one correction for the record and five non-blocking findings. This was reviewed as five previously-unreviewed components plus one integration fix — see the correction below.
Correction: the sub-PRs were not independently reviewed
The description says "five independently-reviewed, individually-green PRs". I checked: #439, #442, #443, #444, #445 are all CLOSED with zero reviews. "Individually green" (CI) holds; "independently reviewed" does not — this batch is the first and only review these 2,920 lines have received, and I reviewed it accordingly (each component at its riskiest seam, two deep-dives on the kbc coverage change and the MCP multi-key refactor). Please drop that phrase from the description so the merge record stays honest.
The integration fix (eaf53f0b) — verified, and it validates the batch approach
The #442×#444 interaction is real and the fix is right: every capability call site now threads the run's profile into getAsync/stop (pod names are profile-prefixed after the rename), memory-cold paths recover the profile via adopt(notifyOnAdopt:false) — correctly reusing the destructive-path adopt seam, which never reattaches a relay or spawns a box — and an unknown profile falls back to the default prefix (pre-rename behavior). Tests assert the profile threading on the memory-cold paths. Chat-agent paths staying on the default prefix is correct (they operate on chat agentIds). This interaction would have shipped broken had the five PRs merged separately; catching it is the batch's justification.
Component verification summary
- #442 (naming): manager and spawner derive the pod name from the same source (
getBoxProfile().podNamePrefix ?? "agentbox", invariant documented at both sites). The legacy-pod reaper is properly guarded — onlyboxType=kb-compile*pods are reaped under the old name, never a chat box sharing the agentId; 404 is the steady state. Renamed pods keep theapp=agentboxlabel so orphan GC andlist()still cover them. Helm derivation, Makefile, and the CLAUDE.md matrix row all updated;isCompileCapablestays fail-closed. - #444 (test-session resilience): the box-side watchdog reaps a wedged test turn without retry and latches
_stall_reapedso the interrupted ResultMessage is discarded (no double turn_done); the idle-TTL reaper compares monotonic time (clock-skew safe) and any SDK message refreshes both clocks, so an active turn can't be TTL-reaped.shouldRelayTestSessionpins the subtle single-consumer invariant (an idempotent replay must NOT start a second/test-eventsrelay — that would split the frame stream). Contract additions are documented inline (tidpass-through warning,client_request_idabsent ⇒ old behavior, structuredtest_session_limit429 distinguishable from a rate-limit 429). - #445 (custom system prompt): fallback fires only when the caller omits
systemPrompt; explicit param wins; resolve failure degrades to the built-in default (a lookup failure never fails the chat); session-creation-only semantics documented. Three-path test. Minor: this adds oneconfig.getAgentRPC to every web prompt that omits the param — fine, just noting. - #439 (coverage v2) — deep-dive: the edge rule is monotonic and fail-closed where it matters (an asset embedded by no accounted doc stays unaccounted; direct-cite wins; no double-count), observable (
auto_attached+ sample), and the fixture pins direct/inherited/shared/orphan/orphan-excluded/space/nested/query-string/HTML-img cases. The budget interaction with the #430 high-water limit is benign (if anything over-provisioned — the safe direction). Two real findings below (F1, F2). - #443 (MCP multi-key) — deep-dive: all three v1 security properties survive with dedicated post-refactor tests (no-retry submission + snapshot-not-throw with task_id, compact working responses, key hygiene incl. no key material in errors/stderr); alias routing fails closed on unknown aliases; the recorded creator wins over a mismatched
agentarg; tool schemas are non-breaking (agentoptional everywhere; v1 single-key configs behave byte-identically). Two findings below (F3, F4).
Findings (non-blocking)
- F1 —
.tiffis coverage-accounted but escapes numeric re-verification.MEDIA_ASSET_EXTSincludes.tiff(selfcheck.py:51-52) but the attribution-edge feed into mediaverify intersects withIMAGE_SOURCE_EXTS(selfcheck.py:1619 region), which does not — so a.tiffchart auto-attaches as accounted yet is never numerically re-read. The commit acknowledges the deferral, but this is an untested fail-open on exactly the fidelity surface coverage-v2 touches. Please add the one-line test pinning today's behavior, so the gap is visible rather than silent. - F2 — the sicore ledger mirror is a fixture-only coupling. The edge semantics (case-insensitive
assetssegments,?query/#fragtruncation order) are locked only by each repo running the sharedasset-provenancefixture; there is no protocol version pin. A one-sided deploy silently diverges the box view from the server ledger. Worth one line in the deploy notes and making sure the sicore side lands the same fixture before either ships. - F3 — multi-key config has no file-based path.
SICLAW_A2A_KEYSis inline env JSON only: the 0600 key-file validation exists solely for the single-keySICLAW_A2A_KEY_FILE. Multiple keys therefore live in the process environment (/proc/<pid>/environ, shell history) — a hygiene regression relative to the adapter's own v1 premise. ASICLAW_A2A_KEYS_FILE(same 0600 check) would close it. - F4 — the cross-key authorization boundary is delegated to Sicore and untested. For a task_id the adapter has not seen,
selectForTaskroutes to whichever alias the caller names; whether key B can actually read key A's task is enforced entirely server-side, and no test (here or referenced) pins that rejection. The assumption is load-bearing — please verify it against the Sicore A2A server once, and note it in the README. Related: multi-key entries can't pinagentId, so multi-key mode hard-requiresGET /self(older Sicore ⇒ boot abort by design — worth stating in the README compatibility notes). - F5 — pre-existing stale test. The
test_charset_corruption_detectionnote (asserts 第6行 vs actual 第7行, not invoked by the CI harness) is honestly flagged; please file the follow-up so it doesn't stay silently stale — a test that never runs is worse than none.
Deploy notes
Image rename means the registry must publish siclaw-kbc-box before any runtime with this change rolls out (helm derives it from the release tag; explicit compileBoxImage overrides are unaffected). First spawn per compile agent reaps the old agentbox--named pod — expect a one-time visible reap in logs during the upgrade window. And the standing kbc note: box behavior changes (coverage v2, test-session watchdog/reaper) need the box image rebuild + redeploy.
Verdict
Approved — every component held up under what was effectively its first review, the integration fix is exactly the kind of cross-PR interaction batching exists to catch, and the findings are hardening items, not defects. Please fix the "independently-reviewed" wording before merge.
…y + per-turn generation guard P1-1 (idempotency key not run-isolated): the open-test dedup map was keyed by client_request_id alone, so on a shared box two runs colliding on a request id would replay each other's tid/snapshot/fingerprint. Key it by (parent_run_id, client_request_id) and re-verify the mapped session belongs to THIS parent run before replaying; a mismatch (or stale/closed mapping) falls through to a fresh open. Teardown rebuilds the run-scoped tuple to drop the key. P1-2 (stall-then-immediate-retry frame race): the watchdog emits turn_done and frees the UI before it interrupts, so the next /test-message can arm a new turn before the reaped turn's stragglers drain. With _stall_reaped already cleared by _arm_test_turn, the late ResultMessage terminated the fresh turn (and a late partial contaminated it). Add a per-turn generation bumped each _arm_test_turn; the receive loop tags each frame (frame_gen = _results_seen + 1, results stream in strict turn order) and drops any below the armed generation, logging turn.stale_frame_dropped — never blocking on stragglers that may never arrive. Tests: test_open_test_session_idempotency_is_run_scoped (same key across two runs → distinct sessions; same key+run → replay) and test_test_session_late_reaped_frames_never_terminate_new_turn (drives the real watchdog reap, then injects the reaped turn's late partial + ResultMessage via a new queue-driven fake, asserting the fresh turn survives and completes on its own result while both stragglers are dropped).
…ator is lost Review P1 on the per-turn generation guard. The guard assumes a stall-reaped test turn's torn-down ResultMessage eventually arrives and advances _results_seen. But the reaper's client.interrupt() can be swallowed by the same black hole that wedged the turn, so that terminator may NEVER come. Then the armed generation permanently outruns _results_seen: every later turn's real frames are dropped as stale, its own ResultMessage is dropped too, the turn times out and re-reaps — a permanent miscount avalanche the existing tests never covered (they only exercised a terminator that eventually arrives). Fix: when the terminator cannot realign the stream, reconnect a fresh SDK client instead of drifting forever. - The test-stall watchdog now manages _needs_rebuild. A failed interrupt() flags it immediately. A successful interrupt() arms a bounded post-interrupt window (KBC_TEST_STALL_REBUILD_WINDOW_S, default 10s, timed inside the watchdog loop so it never blocks the handler): if the terminator lands and advances _results_seen the flag stays clear (existing generation flow resumes); if the window elapses, the flag is set. - The next /test-message that sees _needs_rebuild rebuilds the SDK client: test_session_driver becomes an epoch loop that disconnects the dead client and reconnects on the SAME pinned snapshot with generation state reset to zero. The tid / snapshot / UI session row are preserved. The SDK-side conversation history is intentionally discarded — a stalled session's history is already unreliable and the UI transcript is server-persisted; this is the accepted trade-off. A rebuild failure surfaces a retryable error, never silently. Tests (three stall timings, fake supports a raising interrupt and a never-delivered terminator): failed interrupt -> rebuild -> new turn answers; accepted interrupt but terminator never arrives -> window elapses -> rebuild; terminator arrives in-window -> no rebuild, existing drop path (regression guard). Full suite green across three consecutive runs.
…t-message After a rebuild whose reconnect failed, the driver clears `connected` and parks until the next rebuild request. But handle_test_message waited on `connected` first, so every retry timed out at the liveness gate (503 "session is still starting") and the rebuild retry path never ran — the session was stuck until the idle TTL (review P1). Reorder the handler: cheap body validation, then rebuild-if-flagged, then the liveness wait. A successful rebuild sets `connected` before signalling ready, so the order is safe in the normal (still-connected) taint case too. New HTTP regression test drives the real handler end to end: first rebuild reconnect fails (immediate retryable 503, flag re-armed), second retry rebuilds on a fresh client and the turn is answered. Verified the test fails on the previous head with exactly the reported symptom.
…ndow is unresolved After a stall reap whose interrupt() succeeded, the watchdog frees the UI (turn_done) and arms a bounded window waiting for the torn-down turn's ResultMessage. But the UI can retry immediately inside that window, and arming the retry on the old client is ambiguous (review P1): if the terminator never arrives, the retry's frames are counted one generation behind and silently dropped, while its own ResultMessage is mistaken for the lost terminator — cancelling the rebuild decision. The user's valid retry was never displayed and timed out a second time. /test-message now treats an unresolved pending-terminator window exactly like _needs_rebuild: rebuild a fresh client before arming. Deterministic by construction — no new query is ever issued while the old generation stream is in doubt, so the mistaken-terminator attribution can no longer occur. The stalled SDK history was already the accepted loss on this path. _rebuild_test_client also clears the window up front so the watchdog cannot re-flag terminator_lost mid-rebuild. The no-retry paths are unchanged: an in-window terminator with a patient user still resolves on the same client with no rebuild. New HTTP regression test drives the reviewer's exact sequence end to end: interrupt succeeds, terminator lost, retry lands inside the window — asserts exactly ONE stall timeout, the second answer is displayed, the client was rebuilt, and no stale-frame drops occurred. Verified it fails on the previous head with two turn_stalled events and the retry answer eaten.
jacoblee-io
reviewed
Jul 23, 2026
jacoblee-io
left a comment
jacoblee-io
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed all five components + the integration fix, one focused pass each (with the selfcheck graph and the MCP key-routing traced against the fixtures/code end to end). Strong batch overall — the integration fix eaf53f0b correctly closes the 404/leak gap #442 opened (every capability call site now threads rec.profile; adopt(notifyOnAdopt:false) is genuinely side-effect-free; the upgrade reaper targets old-prefix compile pods by boxType label without touching chat boxes), #445's fallback is correctly gated on strict === undefined, #443's multi-key isolation holds (key+URL are co-bound per immutable client; no cross-key leak, no key in logs), and the rename glue (image siclaw-kbc-box, helm derive, Makefile double-push alias) is complete with only intentional legacy references.
Two MEDIUM correctness bugs worth addressing before merge — inline. Plus low/hardening notes:
- #442:
prefixForProfile→getBoxProfile(profile)throws on a corrupt/forward-version stored profile; un-caught intestClose/testSessionsit turns a graceful default-prefix fallback into an RPC 500 (pre-existing fragility inherited by the fix — consider falling back toagentboxon unknown).agent.terminatestill callsstop(box.agentId)without the profile (not triggerable today, but asymmetric with the fix's own rationale).kb-testkeeps theagentbox-prefix — confirm intended. - #444: three
TEST_SESSIONS.values()iterations lack alist()snapshot (safe today — noawaitinside — but the idle reaper already snapshots; add for symmetry before anyone adds an await).capability.testCloseforwardstest_session_idwith noparent_run_idscoping (unguessable UUID mitigates; the listing path IS run-scoped). - #443: duplicate alias inside
SICLAW_A2A_KEYSJSON is silently last-wins; the multi-key form carries key material in an env var (no per-alias 0600 file) — worth a README caveat. - #445: a pre-existing (NOT introduced here) seam —
chat-gateway.ts:325buildsagent.system_prompt ?? nulluntrimmed, so a stored""/whitespace prompt reaches the box verbatim instead of falling back. Optional: normalize with.trim() || null.
Also per the PR body: the sicore side must land the same asset-provenance fixture before either ships (edge semantics are pinned only there), and test_selfcheck.py::test_charset_corruption_detection is stale on main already (out of scope). Given the two mediums I'm leaving this as comments rather than an approval.
| # next /test-message starts the SDK client fresh. Runs above the active-turn | ||
| # guard because a reaped turn is no longer active. | ||
| if run._pending_terminator_deadline is not None: | ||
| if run._results_seen > run._pending_terminator_seen: |
Collaborator
There was a problem hiding this comment.
[Medium] Lost terminator + a new turn within the window leaves the session permanently generation-misaligned (silently drops all later output until the 30-min idle reaper). This resolver treats any advance of _results_seen as "the reaped terminator arrived", but _results_seen is a bare counter that every turn advances (the stale-drop path at ~4247 and the live path at ~4255).
Trace: turn A stalls → interrupt() succeeds → the window arms _pending_terminator_seen = _results_seen with _needs_rebuild left False and the UI freed. A's terminator is black-holed (the exact failure this defends against), so _results_seen doesn't advance. Within the 10s window the user sends /test-message: because _needs_rebuild is False there's no rebuild, _arm_test_turn bumps _turn_generation but does not reset _results_seen, and B queries the same wedged client. Now _results_seen is permanently one behind _turn_generation, so every subsequent turn's frames fail frame_gen < _turn_generation and are dropped as stale — while the rebuild that should recover this is never armed (this > check short-circuits, or is bypassed because stale frames continue before reaching it). Session is stuck until the idle-TTL reaper.
Fix: gate the resolve on evidence the specific reaped terminator landed — compare against the reaped turn's _turn_generation (or track a dedicated terminator token) rather than a bare _results_seen advance.
| prose = _markdown_prose(md_text) | ||
| for captured in _MD_LINK_RE.findall(prose): | ||
| destination = captured.strip() | ||
| if destination.startswith("<") and destination.endswith(">"): |
Collaborator
There was a problem hiding this comment.
[Medium] An angle-bracketed link destination with a trailing #fragment/?query drops the edge → a good compile is wrongly FAILED. This unwrap only fires when the destination ends in >. For , _MD_LINK_RE's angle alternation (<[^>\n]+>) only matches when > is immediately followed by ), so the trailing #fig1 defeats it and the whole <assets/a b.png>#fig1 is captured by the fallback branch; startswith('<') and endswith('>') is then false (string ends in the fragment), the brackets are never stripped, and the real edge doc → assets/a b.png is lost → asset reported unaccounted → closed:false. Same input without the fragment closes correctly. Same root cause in _markdown_link_targets (~608) → a page reachable only via an anchored angle link is misreported as an orphan. Realism is low (angle brackets are used for spaced filenames, a fragment on top is uncommon), but it's deterministic and it's exactly where the box view and the sicore ledger-mirror could silently diverge — worth pinning into the shared fixture. Fix: strip the <…> wrapper after truncating #/?, or let the regex tolerate a trailing fragment/query after the angle destination.
…w generation-specific Two review findings on the stall-reap path: 1. The pending-terminator window was armed only AFTER interrupt() returned, but the stall turn_done frees the UI BEFORE that await — a retry landing while interrupt() was in flight saw neither _needs_rebuild nor a pending window and armed a misaligned turn on the wedged client. The window is now armed at the reap decision itself, before any await, so the /test-message rebuild gate covers the whole post-reap timeline. 2. The window resolver treated ANY advance of _results_seen as "the reaped terminator arrived", letting a later turn's ResultMessage impersonate a lost terminator and cancel the rebuild decision — leaving the session permanently generation-misaligned (every later frame silently dropped until the idle TTL). The window now records the reaped turn's GENERATION and only a ResultMessage advancing _results_seen to that generation resolves it (_pending_terminator_seen -> _pending_terminator_gen). The interrupt-failed branch additionally guards on the client still being current: a retry may have already rebuilt underneath the in-flight interrupt (disconnecting the old client is exactly what makes interrupt() raise then), and re-flagging would force a second, spurious rebuild. New HTTP regression test parks the reaper inside a blocked interrupt(), retries in exactly that gap, and asserts one stall, a rebuild, and the retry's answer displayed. Verified it fails on the previous head with the retry swallowed.
…ragment/?query
An anchored angle destination —  — defeated the
unwrap: the link regex's angle alternation only matches when `>` is
immediately followed by `)`, so the whole `<...>#fig1` fell to the bare
branch, endswith(">") was false, the brackets were never stripped, and the
doc->asset edge was lost — a good compile was wrongly failed as unaccounted
(review finding). Same root cause in _markdown_link_targets misreported an
anchored angle-linked page as an orphan.
Shared _unwrap_angle_destination now runs after the title strip and before
#/? truncation: it unwraps up to the first `>` and re-appends the
remainder, so the ordinary fragment/query truncation removes the anchor.
Mirrored byte-for-byte by the sicore ledger's unwrapAngleDestination
(fixed in lockstep); the shared asset-provenance fixture pins the case
with a new `guide/assets/c d.png` referenced only via
`<assets/c d.png>#fig1` — the old parser demonstrably drops that edge in
both repos.
jacoblee-io
approved these changes
Jul 23, 2026
jacoblee-io
left a comment
jacoblee-io
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. Both MEDIUM findings from the prior review are correctly and thoroughly fixed, each with a targeted regression test and cross-repo alignment.
MEDIUM 1 (test-session terminator/rebuild) — two mechanisms now close the scenario:
- The window is generation-based: arm records the reaped turn's generation (
_pending_terminator_gen = _turn_generation) and the resolver clears only on_results_seen >= _pending_terminator_gen, so a bare counter advance from an unrelated turn can no longer falsely resolve it. handle_test_messagenow rebuilds the SDK client whenever the terminator window is still open (_needs_rebuild || _pending_terminator_deadline != null), before arming — so a retry that arrives while the reaped terminator is still in doubt starts on a fresh, generation-reset client instead of the wedged one. That's exactly the lost-terminator + retry-within-window path I raised. Plus the rebuild runs before the liveness gate (avoids the 25s-timeout trap) and clears the window. Covered bytest_test_session_late_reaped_frames_never_terminate_new_turndriving a real watchdog reap + injected stragglers.
MEDIUM 2 (angle-bracket + fragment/query link) — new shared _unwrap_angle_destination unwraps to the first > and re-appends the remainder, applied in both _markdown_link_targets and document_link_targets, ordered after the title strip and before #/? truncation; mirrored byte-for-byte by the sicore ledger's unwrapAngleDestination and pinned by a new guide/assets/c d.png fixture referenced only via <assets/c d.png>#fig1 in both repos — which also closes the box↔ledger divergence risk I flagged.
Also verified 85e8ce34 run-scopes the idempotency key to (parent_run_id, client_request_id) with an ownership re-check before replay. The remaining low/hardening notes (kb-test prefix intent, agent.terminate profile symmetry, multi-key env README caveat) are optional and non-blocking. Deploy prerequisite still stands per the PR body: the sicore side must land the same asset-provenance fixture (done in lockstep here). Ship it.
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
3 participants
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.
Platform hardening batch
Consolidates five individually CI-green PRs (this batch is their first code review) into one branch so the changes can be reviewed and merged as a single unit. Each PR was merged with
--no-ff, preserving its full commit history (11 source commits under 5 merge commits). No squashing.supersedes #439 #442 #443 #444 #445 — please close those five once this batch merges.
Components
feat/mcp-multi-key-aliasmcp/sicore-a2a-adaptersupports multiple named A2A keys selected by alias (router + config + tools).feat/kbcbox-namingsiclaw-kbc-boxand pod prefix tokbc-box; upgrade reaps old-prefixed pods; profile-awarestop/getAsync.feat/kb-asset-provenancefix/chat-custom-system-promptgateway chat.sendfalls back to the agent template when the caller omitssystemPrompt).fix/kb-test-session-resiliencecapability.testSessions; gateway idempotency key +shouldRelayTestSessionpredicate.Conflict resolution
Zero textual conflicts.
src/gateway/server.tsis touched by #442, #444, and #445 but in disjoint regions (imports at distinct lines; #442 ~L474-632, #445 ~L92/L319, #444 ~L28-47/L836-1055), so git's 3-way merge auto-resolved cleanly. All three sets of changes verified to coexist (imports + bodies) and to type-check together.Integration fix (one commit on top of the merges)
fix(capability): thread run profile into box discovery/stop for compile pods(eaf53f0) — an interaction surfaced only by combining #442 and #444. #442 renamed the compile-box pod prefix (agentbox-→kbc-box-, profile-derived) and taughtonReap/onAdoptto passrec.profile, but the remaining capability call sites still resolved the pod name with the defaultagentboxprefix (correct onmain, where every box wasagentbox-; wrong once the rename lands). In K8s mode that madecapability.testClosereport a live test session as already-closed,capability.testSessions(new in #444, which reused testClose's discovery pattern) list nothing for a live run, andcapability.cancel/ session-setup cleanup 404 onstop()and leak the pod. Fix: pass the run's profile at every capability-box site — from the in-memory run record, or (for the restart-safe paths) recovered from the consumer store viaadopt(notifyOnAdopt:false), which never reattaches a relay or spawns a box. Chat-agent paths (chat.sessionStatus,agent.clearMemory,agent.terminate) are intentionally left on the default prefix.Regression (full, run on the merged + fixed tree)
__main__harness (python test_X.py):test_selfcheck,test_compile_box,test_destream,test_mediaverify,test_redblue,test_incremental,test_batching,test_source_snapshot,test_office_ingest,test_mtls_auth— all OK.npm run build) — clean.npm test) — 235 files, 4600 passed, 2 skipped, 0 failed. Includes the new suitesserver-capability-test-sessions,server-chat-system-prompt, feat(kbc): rename compile box image to siclaw-kbc-box and pod prefix to kbc-box #442'sk8s-spawner/manager/box-profile, and the profile-threading assertions updated by the integration fix.npm test, includes tsc) — 56 passed.agentbox.compileBoxEnabled=true—SICLAW_COMPILE_BOX_IMAGEderives to{registry}/siclaw-kbc-box:{tag}; full chart renders.Note for the reviewer (pre-existing, not introduced here)
test_selfcheck.py::test_charset_corruption_detectionasserts第6行but selfcheck now reports第7行(the U+FFFD sits on line 7 of the fixture). This fails identically onmainand is untouched by any batch PR — the CI harnessmain()does not invoke this function, so it has been silently stale. Out of scope for this batch; flagged for a separate follow-up.Deploy note (reviewer F2): the coverage-v2 edge semantics are pinned only by the shared
asset-provenancefixture in both repos — the sicore side must land the same fixture before either ships, or the box view and server ledger silently diverge.