Repository navigation
🐛 Keep foreground command output inside its REPL entry - #1093
Conversation
PR #1093: 🐛 Keep foreground command output inside its REPL entry8 files, +658 / -13 Scope🟡 671 lines changed. PRs under 400 receive more thorough review. StructuralOxlint structural signals:
Slop✅ Slop indicators look low. Static AnalysisOxlint: 155 diagnostics across 5 files (12 rules) no-shadow (91): packages/cli/src/repl/source-reading.ts, packages/cli/src/repl/session.ts, packages/cli/src/repl/program.ts (+1) CorrectnessNo extraneous code patterns detected. |
taras
left a comment
There was a problem hiding this comment.
Independent Fable review: PASS
Exact commit: 26ee453ea6a23a7100876433a53d5303bf7289e1. Provider claude, requested model alias fable, effort high. One original independent call completed and joined; no retry or length exception. This is the saved verdict, published by the coordinator.
Reviewed 26ee453 against 9b98cbe. Routing is correct: runEntry installs per-entry Stdio handlers at min before agent/installation/execution resources, and context-api orders scope min handlers downstream of core's per-exec max boundary, so handlers receive only bytes core forwarded after retention/bind/capture/hidden routing and never call next, so no physical write occurs; an outer test-scope min observer sits further downstream, which makes E1's leak check valid and the routing-disabled fail-before witness meaningful. Retained exec records, bindings and root output are untouched (core unchanged). Attribution uses the captured entry key; the frozen-prefix ReplLive in viewFor omits commandOutput, so frozen positions show none while a live older selection keeps its own; arrival wakes via outputs.send without touching state. Decoders are per-entry and flushed in an ensure registered before the handlers. Control escaping is a display-only copy at plain(). Cold reopen path unaffected (fresh process, saved records only). Spec and xmd run contracts hold. Only non-blocking cost and tidiness observations remain.
All six original acceptance clauses were examined: live-renderer, capture-retention, terminal-safety, lifecycle-attribution, cold-history, run-compatibility.
Nonblocking observations:
-
packages/cli/src/repl/session.ts:1021 session.commandOutput rebuilds frozen arrays for every in-process entry on each read, and program.ts calls liveOf twice per cycle (reduceRepl at 1662 and viewFor at 3021) before application.ts:6152 selects one entry. Because append merges consecutive same-channel text, cost is O(entries x channel alternations), not O(bytes), so it is small; a ReadonlyMap or a snapshot cached until the next append would remove the per-frame copy. Unmeasured; no evidence of a user-visible regression.
-
packages/cli/src/repl/source-reading.ts:279 plain() reruns inertCommandText and split over the selected entry's whole command text and reallocates every ReplReadingLine per frame while a command streams; the fit store caches cuts by text so measurement is not repeated, and the existing live Output path has the same shape. Process-local retention of every entry's command text for the session lifetime is per the accepted plan. Unmeasured growth concern only.
-
packages/cli/tests/repl-source-reading.test.ts:707 the comment 'two captioned sections' is stale now that the reading has output, commands and source keys. The expectation change itself is disclosed with its retained first failure and is required by the shape change.
-
Any @effectionx/process child pumped inside the entry scope (document daemons, installation children) is presented under the 'Command stdout/stderr' labels of that entry. This is consistent with containment and attribution to the producing round; noted so the labels are not read as exec-only.
This verdict covers the exact commit above. Required CI and current exact-main health remain delivery gates.
taras
left a comment
There was a problem hiding this comment.
Independent Fable exact-head follow-up: PASS
Commit 0297664daee4d91b0ecb3f264f5f704a194f6348. Original independent conversation retained; provider claude, requested alias fable, effort high. One follow-up after the finite correction and current-main integration; no automatic retry or length exception. The coordinator is publishing the saved verdict.
Inspected merge 0297664 (tree 380fa8d5) with parents ba71128 and main 98cc6f1; merge-base is 9b98cbe. The merge is disjoint: the eight fix files are byte-identical to ba71128, all 51 incoming files are byte-identical to main, and main touched no repl/, core foreground/execute, test-support, spec or config path. The tree equals the tested candidate tree in the focused receipt, and the committed acceptance test hash equals the identical before/after hash. The correction is one line: workComposed in application.ts applies the existing inertCommandText to the display copy of entry.terminal.output before splitting, marker prefixing and fitting, matching the center reading's plain() conversion; retained output, bindings and exec records are untouched, and the new mounted regression distinguishes the base. Remaining consumers of entry output are the chat preparation facts list (cache identity only) and program.ts live.output, which is the Sidekick turn's own reply text, so no unconverted command display remains. My earlier routing, ordering, attribution, frozen-prefix and cold-history traces are unchanged at this head. The four prior non-blocking findings still apply; nothing blocks.
All six original acceptance clauses examined: live-renderer, capture-retention, terminal-safety, lifecycle-attribution, cold-history, run-compatibility.
Nonblocking observations:
-
packages/cli/src/repl/session.ts session.commandOutput rebuilds frozen arrays for every in-process entry per read, and program.ts reads it twice per cycle (reduceRepl and viewFor) before application.ts readingOf selects one entry. Cost is O(entries x channel alternations) because same-channel text merges, so it is small; a ReadonlyMap or a snapshot cached until the next append would remove the copy. Unmeasured, unchanged at this head.
-
packages/cli/src/repl/source-reading.ts plain() reruns inertCommandText and split over the selected entry's whole command text per frame while streaming, allocating every ReplReadingLine; the fit store caches cuts by text so measurement is not repeated, and the existing live Output path has the same shape. The new workComposed conversion runs only on chat re-preparation since the preparation facts key on the raw output. Unmeasured growth concern only.
-
packages/cli/tests/repl-source-reading.test.ts:707 the comment 'two captioned sections' remains stale after the key expectation became output, commands, source.
-
Any @effectionx/process child pumped inside the entry scope (document daemons, installation children) is presented under the entry's 'Command stdout/stderr' labels. Consistent with containment and producer attribution; noted so the labels are not read as exec-only.
Original Planner acceptance also passed this exact commit. Required CI and current exact-main health remain merge gates.
Why
Ordinary unbound
sh execoutput bypasses the interactive REPL and can overwrite its panes. Fixes #1026.What changes
Stdout and stderr stream into labelled command sections within the producing entry, including output received during cleanup. Output arrival preserves selection, focus and drafts.
Entry-scoped downstream stdio handlers preserve core capture, hidden-output, retention and exit-status behavior. Command presentation stays separate from document Output. Independent streaming decoders preserve split Unicode. Control characters become readable inert text before fitting and drawing, including captured output shown in both the center reading and its associated Sidekick display.
Review guide
Start with
packages/cli/src/repl/session.ts, then the display path throughprogram.ts,application.tsandsource-reading.ts. Check terminal ownership, per-entry attribution through teardown, visibility and UI cost as content grows. The REPL specification records the boundary.How to verify it
Final signed feedback head:
0297664daee4d91b0ecb3f264f5f704a194f6348. It integrates current main's #1064 merge without changing any of the 8 exec-fix blobs or 51 incoming blobs.deno task test packages/cli/tests/repl-exec-output.test.ts packages/cli/tests/repl-sidekick-presentation.test.ts packages/cli/tests/adversarial-implementation.test.tspasses 26 suites and 110 steps.Each command carries the existing prepared
DENO_DIRso isolated-HOME subprocesses use the prepared cache. The final mounted E1 test fails unchanged with routing disabled and passes with it enabled. The associated-display regression also fails before its correction and passes with identical test bytes after it. First failures and candidate receipts are retained locally. CI owns exhaustive runtime checks.Reviews and delivery
Original Planner acceptance and the independent Fable exact-head review both pass
0297664daee4d91b0ecb3f264f5f704a194f6348. Fable retains four nonblocking cost, labeling and comment observations. Required CI and current exact-main health remain merge gates.The bounded XMD REPL experiment completed 7 coordinated role requests with one source-correction round and no automatic provider or response retry. Current-main integration and delivery-status reads use no model. Existing automatic CI review workflow runs are tracked separately.
Risks and limitations
Visible forwarded presentation lasts for this REPL process. Cold viewing uses the existing saved per-command stdout/stderr/status and starts no command. The existing upstream process-pump tail limitation remains.
Scope confirmation