Repository navigation
Run parent-owned headless XMD children with live attachment - #1044
Conversation
Add the late-UI-question-after-embedded-answer case, a lost B-notification case, a malformed B-result (consumeTurn parse failure) case, and a stock ACPX permission exchange, closing the acceptance gaps named in the J3/H5, H6/J5, H6, and J4 correction. Extend the test-support fixture with an optional per-event transform hook so a test can corrupt or drop one ACP runtime event without touching production code.
PR #1044: Run parent-owned headless XMD children with live attachment51 files, +7427 / -1495 Scope🔴 PR has 8922 lines changed. Split into focused PRs. 🟡 8922 lines changed. PRs under 400 receive more thorough review. 🟡 51 files changed. Are all changes related? 🟡 Changes span 10 directories. StructuralOxlint structural signals:
SlopOxlint slop signals:
Static AnalysisOxlint: 277 diagnostics across 20 files (24 rules) no-shadow (89): packages/cli/src/sidekick-acp/bridge.ts, packages/cli/tests/support/headless-repl.ts, packages/cli/src/sidekick-acp/posix.ts (+4) CorrectnessNo extraneous code patterns detected. |
taras
left a comment
taras
left a comment
There was a problem hiding this comment.
Independent review by Codex gpt-6-astra (high reasoning), at 579c0e390e1d68b348a8f232c5911fab57fc131c.
Two P2 findings need correction before merge: retired ACP presentation records accumulate until further questions fail, and each UI reattachment leaves another renderer owned by the long-lived root. Both are described inline with focused reproduction evidence and fixes that preserve the accepted behavior.
I reviewed correctness, performance and minimalism against the accepted child lifecycle, exact identity, cancellation, original input owners, terminal isolation and transient response contracts. The existing ownership split is appropriate; these fixes belong at the two lifetime boundaries and do not require a broader protocol or feature redesign. I also inspected the supplied acceptance/source-correspondence and exact-head focused regression evidence.
Bounded, model-free measurements used the real root/attachment/renderer with the repository's scripted provider and recording terminal on macOS arm64, Deno 2.9.7. The comparison is the ordinary parent at the same reviewed head; the new child feature has no supported old-version baseline.
| Measured terminal-write boundary | Attached child | Ordinary parent |
|---|---|---|
| Input-to-write p50 / p95 / worst | 10.05 / 14.06 / 16.55 ms | 10.08 / 12.83 / 13.77 ms |
| Active writes per second | 20.41 | 20.39 |
| Write interval p50 / p95 / worst | 49.25 / 54.18 / 55.84 ms | 50.00 / 53.88 / 57.17 ms |
Each workload used a 120×30 terminal, five warmup inputs and 40 measured text/backspace inputs at 50 ms cadence over about 1.96 seconds. These are completed writes into the terminal adapter/emulator, not physical visible-paint FPS or maximum throughput. Startup and idle time are excluded.
Three canonical child starts in an already active parent took 112.88–136.08 ms; exact Shutdown entries took 59.91–65.50 ms, including entry/journal work. Sixteen EOF detachments joined in 1.39–4.68 ms, and three actual detach-control clicks joined in 2.06–4.75 ms. Terminal readers/listeners and raw mode were restored. Repeated attachment also exposed the retained-renderer memory finding below.
Raw samples, reproduction harnesses and a V8 CPU profile are retained for the implementor. Samples reach rendering/layout, WASM and Effection scheduling; the profile includes startup and GC, and processor-version/native-symbol limitations prevent reliable native attribution. Initial sandbox socket denials were preserved as environment failures; the successful normal-temporary-directory startup run required local socket access. No live model journey or exhaustive confidence suite was run for this review.
Required CI/main-health and qualified runner-measured weights remain separate delivery gates. This review does not certify them or authorize merge.
taras
left a comment
taras
left a comment
There was a problem hiding this comment.
Independent review by Claude Fable 5.1 (claude-fable-5-1) at 1bf9ca7fe11fbf05c3fa62ba0fe78d9cee853805 against 541ed6526c60024a02ac8c1a635828ff9bbc8152. This is the explicitly requested follow-up review after the two lifecycle repairs; it does not reuse the earlier Codex review's verdict.
Verdict: no blocking finding. One low-severity (P3) lifetime observation is posted inline with a bounded recommendation; it does not need to gate this PR.
Correctness. I read the repository instructions, the complete architecture document, the Effection contract, the affected ACP/REPL/language/release specifications, the whole feature diff and the two repairs. I traced: canonical Session placement with the xmd_session allocation journaled before acquisition and acquired only after genuine activate succeeds; creator cancellation halting and joining startup before registration, and the parent owning the child afterwards; exact lookup by live map plus the still-selected Sidekick conversation, refusing unknown or ended references without replacement; XMD.Session.Shutdown refusing foreign IDs; the one-UI lease, numbered presentations acknowledged only at matching dimensions, stale-pointer filtering per connection frame, and the 5 s unacknowledged-frame refusal; the per-generation renderer now acquired in a held task and halted on detach, disconnect and generation replacement; retired presentations released after their callback joins while an in-flight response still classifies as stale; the embedded callback entering the captured Prompt subscriber scope with abort/retirement/teardown joins; the single event reader mapping responses to the two transient Prompt notices that Core excludes from reply text; and the npm declaration restoration. I found no defect in those paths. The three headless regression files pass at this exact head (5 tests / 30 steps).
Minimalism. The repairs sit at their existing lifetime boundaries and add no protocol, identity or policy surface. Two optional simplifications, neither requested: children.close is registered twice (profile scope and runReplProgram), the second registration being a no-op; and frames cross the wire as JSON number arrays, which inflates a full redraw about 3.1× and costs a few ms of schema validation per side (numbers below). Both preserve accepted behavior if changed and are fine to leave.
Performance (bounded, model-free, real root/socket/UI client, scripted provider, macOS arm64, Deno 2.9.7). The boundary is completion of the terminal adapter write into the headless emulator, not visible paint or an FPS budget.
| Adapter-write boundary, 120×30, 40 inputs at 50 ms | Attached child | Ordinary parent (same head) |
|---|---|---|
| input→write p50 / p95 / worst | 10.28 / 14.10 / 17.51 ms | 11.28 / 14.08 / 24.48 ms |
| active writes/s | 20.39 | 20.45 |
Repaired boundaries at this head: across 12 attach/detach cycles on one child, newly created live Effection scopes still alive after detach were 26 after the first attachment (the root-owned keyed tree) and 0 after every later one; RSS after GC moved from 288,243,712 to 287,637,504 bytes over the twelve cycles, where the previous review measured roughly +1.09 MiB per cycle at 579c0e39. 130 UI-answered ACP question presentations retired and released 130/130 records with 0 live callbacks and 0 retained records (p50 2.69 ms per cycle); the 65th offer that previously failed succeeds. Parent root halt with its child joined in 9.4–11.1 ms; EOF detach joined in p50 2.95 ms (n=16); the typed draft was redrawn in the first frame of every reattachment. First full frames were 8,996 bytes (120×30) to 55,523 bytes (400×100), all well under the 200,000-byte frame bound; client-side frame validation took 1.2–6.3 ms once per full redraw and under 0.1 ms per incremental frame. A whole-process V8 profile is retained; JavaScript ticks fall in the layout engine, zod validation, Effection scheduling and frame preparation, and nothing points at the repaired paths as hot. Native attribution is unreliable (processor version mismatch) and is not claimed.
Delivery status, stated rather than certified. The CI run for this head was still in progress when this review was posted; main-health and qualified runner-measured test weights remain separate delivery gates. This review does not authorize merge.
| if (input === undefined) { | ||
| return null; | ||
| } | ||
| if (presentations.size >= 64) { |
There was a problem hiding this comment.
[P3, non-blocking] Presentation records have no owner-connection cleanup, so an abnormally ended bridge leaks them into this child-wide bound
retire() keeps a record (it must, so an in-flight response still classifies as stale), and only respond() and release() delete one. Every child-release comes from the bridge process (useChildPresentations in presentations.ts, in update() and its finalizer); each request arrives on a fresh socket, so this map cannot tell which bridge offered a record and nothing on the root side releases one when that bridge dies. A graceful ACP close or stdin EOF does run the finalizer, and I measured 130 UI-answered presentations releasing 130/130 at this head, so the ordinary path is correct. The residual case is a xmd sidekick child-acp process that is killed or crashes while holding an offer: its records stay here until child shutdown, and 64 such losses make every later question or permission presentation in this child fail with the bound error, while the child itself keeps waiting.
Bounded recommendation that preserves the accepted stale classification: when the bound is reached, evict records whose current() is false (their request has ended, so they can never be accepted) oldest first, and refuse only when 64 live-request records remain. An in-flight response to an evicted record then meets the existing "unknown or already classified" refusal, which is the same end state the bound gives today and only in that double-failure case. A regression could offer 64 records with their questions ended and no release, then expect the 65th offer for a new question to succeed.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| // dnt emits inferred declarations for JavaScript even when a supplied .d.ts | ||
| // declares its interface. Keep those declarations and their type-only siblings | ||
| // in directories the emitted graph actually contains. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
| // in directories the emitted graph actually contains. |
Use the complete artifact from runner measurement69 attempt2. All three runtimes and assembly succeeded at dde8ba5; no partial attempt records are reused.
Why
Delegate work to an XMD child from an active parent REPL, keep that work alive across entries and ACP prompts, and attach an interactive UI to the same conversation.
What changes
Before: XMD delegation lacked a supported parent-owned headless child with a reusable live reference and interactive attachment.
After: start with
<Agent name="xmd"><Session name="worker">…</Session></Agent>, capture the child with<XMD.Session as="child" />, reusechild.sessionId, and attach withxmd repl --attach <sessionId>. Detaching leaves the child running. Explicit shutdown or parent termination stops it and joins cleanup. Unknown or ended references refuse without starting a replacement.Questions and permission requests reach ACP and the attached UI through the same owners. Responses are explicit, validated and accepted once; stale responses refuse. Answer/permission result notices remain separate from reply text and durable Prompt records.
How it works
Startup journals the allocated identity before acquisition, arms cleanup before resources, and registers the ready child synchronously with its parent. Replay looks up that exact reference. Retired ACP presentations release their exact records after callback join; each UI renderer generation ends and joins on detach or replacement.
Review guide
Start with the shipped child guide and the headless lifecycle, attachment and client tests. Then trace the headless owners,
repl/program.ts, ACP presentation/response handling and session policy. Inspect cancellation, replay, terminal isolation, stale input, complete first-frame reattachment and cleanup joins.Supporting delivery fixes preserve supplied npm declarations and type-only siblings, resolve stock ACPX through its owning package, and verify the ordered shipped-profile installations and immutability.
What must stay true
How to verify it
08657ffc, with focused CI-repair acceptance at120ee367and source correspondence retained.1bf9ca7fpass the three headless files: 5 tests / 30 steps. The completed-screen regression passes 5 steps. Same-Astra targeted verification passes both lifetime regressions and resolves the two original P2 findings.279afefaf691ff210517d4177ae99c914438f4e8. Required CI run 2306 succeeds with 55 passing jobs, including requiredgreen, and the expected composability skip; all 57 current-head checks have completed (56 success, one expected skip). Current Mainaae9572calso has successful authoritative push CI.Scope
Included: supported child composition/guide, live attachment, explicit shared request responses, lifetime regressions, required packaging/profile/client fixes and qualified runner weights. Ordinary full acceptance and structural decisions remain intact.
New abstractions
The owned child reference and attachment boundary let ACP and an interactive UI address the same live execution without creating another evaluator or writer. The two transient response-result variants report question and permission outcomes outside assistant reply text.
Generated or mechanical changes
test-weights.jsonis the exact assembled runner artifact from attempt 2, committed deliberately. Runtime test commands, corpus exclusions and installed shard counts are unchanged by that artifact commit. The later Main #1049 integration changes review-guide prose; the reviewed feature patch remains byte-identical.Risks and limitations
Scope confirmation
Closes #1005.
Closes #1042.