[malibio]

Closes #3454.

What changes

A terminal chat's summary and session_id now hold what ADR-061 §6–§7 say they hold.

Summary. It was the first 500 bytes of raw terminal output. It is now prose, or absent:

• The capture buffer's output is reduced to plain text: escape sequences (CSI, OSC, DCS and the other string sequences, 7-bit and 8-bit) and control characters are removed, a repainted screen is kept once, and a buffer that overflowed drops its partial first line.
• A model loaded on this machine writes two or three sentences about that text. The reply goes through the same stripping before it is stored.
• An OpenAI-compatible endpoint is never used for this, because terminal output would leave the machine. With no local model, the chat has no summary.
• The session's end is written first and the summary second, so a finished session doesn't read as running while the model works. The first write clears the summary a previous session on the same node left.

Session id. It was the daemon's PTY session id. It is now the id the harness gave its own conversation, read from the harness's session store when the session exits:

• Claude Code: ~/.claude/projects/<working directory, slugged>/<id>.jsonl.
• Codex: the session_meta line of the rollout under ~/.codex/sessions/ whose cwd is the session's working directory.
• Each PTY session runs in a working directory of its own, so the directory identifies the conversation. The conversation written to last wins when the harness recorded several.
• Other harnesses have no id; a session without one clears the id a previous session left.

The PTY session id is no longer stored on the node. It is a handle on a live process and is useless after the daemon restarts. ListSessions now reports each session's node_id, and the viewer asks the daemon which session is running for its node. A node that reads active with no running session now shows the launch form rather than a blank terminal.

Node type sequence

• Type: ai-chat-pty, a core subtype of the abstract ai-chat (structured category unchanged).
• Fields: none added or removed. session_id (local_only) changes what it holds; summary (base) changes how it is produced. Both are still written through the validated update.
• Generated TypeScript: regenerated for the session_id doc comment.
• Docs: NodeSpaceAI/nodespace-docs@e111d92 updates the ai-chat-pty sheet in components/node-types.md, ADR-088 (the two open questions this resolves become decisions), architecture/ai-chat-node.md and architecture/agent-session.md.

Tests

• plain_text.rs: each sequence family, carriage returns, cursor movement, repaints, output ending inside a sequence.
• capture.rs: a sequence split across reads; an overflowed buffer that starts inside a sequence.
• harness_session.rs: fixture session stores for Claude Code and Codex (match by working directory, latest wins, non-session files ignored, symlinked directory, no store).
• capture_service.rs: the stored summary for a transcript with escapes and for an overflowed buffer; no summary without a summarizer; the harness id stored for both harnesses in the local_only field; a missing id clears the previous one.
• session_summary.rs, local_agent_service.rs: reply sanitizing; only a local model summarizes, and a remote endpoint is never called.
• Viewer: re-attaches to the session the daemon reports for the node; launch no longer writes session_id.

bun run test:changed passes (frontend, scripts, browser, types drift, Rust workspace).

Not verified

• The Codex rollout layout is taken from its documented format; Codex isn't installed on this machine, so it is covered by fixtures only. The Claude Code layout was checked against real session directories.
• No summary was generated with the real local model in this PR; the model path is tested with stub engines.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review: PR #3487

Review Type: Initial Review

Summary

The PR does what the issue asks and is a clear net improvement: the summary is no longer 500 bytes of raw scrollback, the stored id is the harness's own, and the PTY id is no longer stored on the node. There are no blockers. Four gaps are worth folding in before merge; all are confirmed by reading the code. The first two concern the privacy guarantee the PR itself states.

What I ran: nextest -p nodespace-agent pty:: (56 passed) and the viewer's vitest file (9 passed). I did not run the daemon tests or the full test:changed. I confirmed the Claude Code store layout (<slug>/<uuid>.jsonl, .claude slugged to --claude) against real directories on this machine. The Codex layout and summary generation with the real model are unverified, as the PR says.

Requirements Check

Criterion	Status	Notes
Follows node-type-sequence.md; ai-chat-pty sheet stays accurate	✅	No field added or removed. Wire type doc comment changed in nodespace-types, generated TS regenerated, schema description updated, and nodespace-docs e111d92 updates the sheet and ADR-088. No literal node_type comparisons or undeclared keys. The sheet's "never raw output" is stronger than the code enforces (finding 2).
Stored summary has no ANSI or control sequences and is prose, or is absent (tests with escapes and with overflow)	✅	Both tests exist in capture_service.rs. The no-escape property holds structurally, because the reply goes through strip_terminal_sequences. "Is prose" rests on a prompt instruction only (finding 2).
Claude Code and Codex harness id stored local_only (fixture stores)	✅	Fixture tests in harness_session.rs, plus an end-to-end test in capture_service.rs that also asserts the schema field is local_only.
bun run test:changed passes; quality:fix clean	✅ (claimed)	Not re-run in full; the targeted runs above pass and the branch passed the push lint.

Code Review Findings

🟡 Important

1. local_engine() can hand back a remote engine during a model swap
📁 packages/daemon/src/services/local_agent_service.rs:308-314 (with set_engine_if_changed, 376-390)

• Problem: local_engine() reads active_model_id under one lock, then the engine slot under another. set_engine_if_changed writes the engine slot first and active_model_id afterwards. During a local-to-OpenAI-compatible swap there is a window where the id still names the local model and the slot already holds the remote engine. A summary that reads in that window sends terminal output to the endpoint.
• How likely: the window is two uncontended lock acquisitions, so it is narrow. The swap is lazy (first turn against the new model), which is exactly when the summarizer's idle wait can end.
• Test gap: a_terminal_session_is_summarized_only_by_a_model_on_this_machine only checks states after a swap has completed.
• Recommendation: decide locality from the engine actually returned, not from a separately locked id. Either add a method on ChatInferenceEngine (say runs_on_this_machine(), false for OpenAiCompatInferenceEngine and the no-op engine), or keep (model_id, engine) under one lock.
• Principle: a "never leaves the machine" invariant should be structural, not dependent on two locks being read in a lucky order (ADR-061 §7).

2. Nothing but the prompt stops the summary being a copy of the output
📁 packages/daemon/src/services/session_summary.rs:84-118, 125-161; packages/agent/src/local_agent/prompt_templates.rs:75

• Problem: the only guards on the model's reply are the escape stripping and the 600-character cap. A small model shown a short session can echo its input, and that echo is stored in a field that is not local_only. The prompt's doc comment admits it is "an instruction, not a guarantee".
• Docs overclaim: the module doc ("It is therefore never the session's output") and the node-types sheet ("never raw output") state it as a guarantee.
• Unverified: no summary was generated with the real model in this PR, so how often E4B echoes is unknown.
• Recommendation: add a deterministic backstop in generate_summary. For example, drop the summary when the whitespace-normalised reply is contained in the normalised input, or when it contains any input line above some length verbatim. Add a test with an engine that echoes its prompt. Run one real session through E4B before merge.
• Principle: ADR-061 §7 says a best-effort measure must not be presented as structural.

3. A late summary can land on a node that has since hosted another session
📁 packages/daemon/src/services/capture_service.rs:133-140

• Problem: the summary write happens after wait_for_idle_stable (unbounded while a chat turn runs) plus generation, and then writes at whatever version the node has. Nothing checks that the node still describes this session.
• Scenario: session A ends, the user starts session B on the same node, and B ends while A's summary is still pending. A's summary can then land after B's clearing write, or after B's own summary, and sit beside B's session_id, exit_code and transcript.
• Contradiction: the doc on build_session_end_properties says the node "never shows the previous session's summary against this session".
• Recommendation: current_version already reads the node. In the summary write, skip unless the node is still ended with last_active == session.ended_at. Add a test that finalizes A with a blocking summarizer, finalizes B, then releases A.
• Principle: a deferred write must re-validate the state it was computed for.

4. Previous session's summary and transcript survive a session that saves neither
📁 packages/daemon/src/services/capture_service.rs:253-259

• Problem: summary is cleared only when saves_summary(config), and transcript is never cleared. A node can host one session after another. If session 1 ran at full and session 2 runs with capture off, at metadata_only or at summary, the node keeps session 1's content.
• Effect: the ended view (ai-chat-pty-session.svelte:266-279) shows session 1's summary or transcript under session 2's exit code and harness id.
• Scope: this predates the PR, but the PR fixes the same bug for session_id and for summary at the summary level, and it now documents the invariant.
• Recommendation: always write summary: null, and write transcript: null unless the level is full. Extend a_session_without_a_harness_id_clears_the_previous_one or add a sibling test.
• Principle: one rule for every field that describes "the session that just ended".

🟢 Suggestions

5. The viewer picks an arbitrary session when a node has several
📁 packages/desktop-app/src/lib/components/viewers/ai-chat-pty-session.svelte:167-181; packages/daemon/src/services/agent_session_service.rs (LaunchSession)

• sessions.find(...) takes the first match from a HashMap-ordered list.
• Two sessions per node are possible: two panes on the same node, or a launch after a failed list_sessions.
• Prefer the newest startedAt. Better, have LaunchSession refuse a node_id that already has a running session, now that the node id is the lookup key.

6. No test crosses the whole summary path
📁 packages/daemon/src/services/capture_service.rs (tests)

• assert_no_control_sequences(&summary) in the_summary_of_a_session_with_escape_sequences_holds_none checks a canned stub string, so it cannot fail.
• The real property is covered in pieces (asked() holds no escapes; sanitize_summary strips).
• One test driving finalize_capture with LocalModelSummarizer and an engine that echoes escape sequences would cover the composition.

7. find_harness_session_id doc says more than the code does
📁 packages/agent/src/pty/harness_session.rs:37-38

• The doc says a record last written before started_at "is not read". Only the Codex path applies that; the Claude path ignores started_at.
• That is fine, since the project directory is unique per session, but the doc should say so.

Nit: packages/daemon/src/services/chat_idle_gate.rs:3: the module doc still says the only background user is title generation.

Nit: packages/daemon/src/services/capture_service.rs:134: capture.plain_text() runs over up to 1 MiB before summarize finds there is no local model. Checking availability first would skip that on machines without one.

Nit: packages/agent/src/pty/capture.rs: SessionCapture::overflowed() is public but only used by tests.

What I checked and found sound

• Escape-sequence state machine: traced CSI (including private parameters and a line feed inside a sequence), OSC/DCS/APC/PM with BEL, ST and ESC \ terminators, an ESC inside a string sequence, charset and keypad escapes, CR overwrite, backspace, and output ending mid-sequence. All behave as documented.
• Overflow handling: eviction is by whole chunk, so the buffer starts at a read boundary; dropping to the first \n removes the partial sequence.
• Harness lookup: runs under spawn_blocking. The PTY child gets HOME and none of CLAUDE_CONFIG_DIR / CODEX_HOME, so the store is where the code looks. Session directories persist after exit, so canonicalize works.
• Two-step write: the end of session is recorded before the model is waited on, both writes go through the validated update with conflict retry, and session_id: null clears the previous id.
• Viewer: keyed per pane and node id, so the onMount lookup is correct. The !activeSessionId guard covers a launch racing the lookup, and a failed lookup falls back to the launch form.
• CLAUDE.md: no compatibility code, lint suppression, issue numbers in comments, or raw console.*.

Recommendation

APPROVE. No blockers, and the change is a clear net improvement over storing raw scrollback. Findings 1 to 4 should be folded into this PR rather than filed separately. Finding 1 is the one I would not merge without, since it is the invariant the PR is built on and the fix is small.

[malibio]

Automated pragmatic-code-review: APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.

[malibio]

Review addressed (commit 4b4a91b)

Addressed

1. local_engine() swap window. ChatInferenceEngine gains runs_on_this_machine() (default false; LlamaChatInferenceEngine returns true). local_engine() asks the engine it is about to return, so the model id and the engine slot are no longer read from two locks. A test puts a remote engine in the slot under a local model's id and gets None.
2. Copied output. generate_summary discards a reply that repeats a line of the output (40+ characters) or shares any unbroken run of 20+ characters with it (a path, a URL, a key, including a value lifted out of KEY=value). The module doc and the docs now say what is enforced and that it is a copy check, not a secret scrubber.
3. Late summary. The summary write re-reads the node and is skipped unless it is still ended at this session's last_active. Tested with a summarizer during which another session starts, and starts and ends.
4. Stale content. Every field describing the ended session is written each time: summary is always cleared at session end and transcript is cleared unless the level is full.
5. Several sessions for one node. The viewer attaches to the latest by startedAt.
6. Whole-path test. output_with_escape_sequences_yields_a_summary_with_none runs capture → plain text → model → sanitizer with an engine that writes escape sequences back. The vacuous assertion is removed.
7. Docs. find_harness_session_id's doc says which store the started_at filter applies to; the idle gate's doc names both background users.

Found while addressing

• .gitignore has a session*.rs pattern, so session_summary.rs was never committed: the branch as first pushed would not have compiled. The module is renamed terminal_summary.rs and is now in the branch.

Run against the real model

• New #[ignore]d live test live_terminal_summary: the native E4B model summarized two sample sessions (a bug fix; a deploy that printed an API token and a URL). All 6 replies were kept, were prose, and carried neither the token nor the absolute path. The three repetitions per session were identical, so this is two independent samples, not six.

Skipped

• Refusing a second LaunchSession for a node that already has a running session: new daemon behaviour beyond this issue. The viewer's newest-session pick covers the lookup.
• Checking model availability before computing the plain text: stripping 1 MiB takes milliseconds.
• Making SessionCapture::overflowed() private: the daemon's tests use it across the crate boundary.

Still not verified: the Codex rollout layout against a real Codex install (fixtures only).

bun run test:changed passes on this commit. Docs follow-up: NodeSpaceAI/nodespace-docs@87ae9be.

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: four Important findings were addressed with logic changes (a trait method deciding locality, a new copy check, a conditional second write, always-cleared fields), and a module was added to the branch that the first review only saw on disk.

[malibio]

Pragmatic Code Review: PR #3487

Review Type: Re-Review (commit 4b4a91b4, on top of 7b672829)

Previous Review Summary

	Count
Previous recommendations	10 (findings 1–7, three nits)
Addressed	7 (findings 1, 3, 4, 6, 7; the viewer half of 5; the idle-gate doc nit)
Addressed with a gap left	1 (finding 2, see A and B below)
Validly skipped	3 (the LaunchSession half of 5; plain text before the availability check; overflowed() visibility)
Skipped without good reason	0
New issues	2 Important (both in the copy check), 3 Suggestions, 2 nits

No blockers. The two Important findings are the two sides of the new copy check's calibration: it discards honest summaries that name a file or function, and it keeps a short session echoed back whole.

What I ran

• nextest -p nodespace-daemon --lib filtered to capture_service, terminal_summary and the two locality tests: 30 passed.
• The daemon it test binary compiles with live_terminal_summary in it. I did not run the live test.
• The viewer's vitest file: 9 passed.
• A verbatim copy of copies_output, built outside the repo, for timing and for the probes quoted in A and B.
• Not run: the full test:changed.

Verification of the first review's findings

#	Finding	Status	Checked against the code
1	local_engine() swap window	✅ Addressed	local_engine() now reads one lock and asks the engine it returns. summarize keeps that Arc for the request, so a later swap cannot redirect it. The mid-swap assertion fails on the old implementation.
2	Nothing stops the summary copying the output	⚠️ Addressed, gap left	copies_output exists, is called in generate_summary, and is tested. See A and B.
3	Late summary lands on another session	✅ Addressed	The guard is re-evaluated on every attempt, and the write is at the version that read returned, so a session starting between check and write causes a conflict and a re-check.
4	Stale summary and transcript survive	✅ Addressed	summary: null and transcript: null|value are written every time. The new test fails on the old code for the capture-off and metadata_only cases.
5	Viewer picks an arbitrary session	✅ Viewer half; LaunchSession refusal validly skipped	Newest startedAt wins. The test puts the expected session in the middle of the list, so a first-match find fails it. Refusing a second launch is new daemon behaviour outside this issue.
6	No whole-path test	✅ Addressed	output_with_escape_sequences_yields_a_summary_with_none runs capture, plain text, engine and sanitizer. The vacuous assertion is gone.
7	find_harness_session_id doc	✅ Addressed	The doc now says the filter is Codex-only, and why.
Nit	Idle-gate module doc	✅ Addressed
Nit	Plain text computed before the availability check	Validly skipped	It does not affect correctness.
Nit	overflowed() is public	Validly skipped	capture_service.rs:615 uses it from the daemon crate.

Requirements Check (issue #3454)

Criterion	Status	Notes
Follows node-type-sequence.md; the ai-chat-pty sheet stays accurate	✅	No field added or removed. Docs commits e111d92 and 87ae9be update the sheet, ADR-088 and ai-chat-node.md. The wording "repeats no line of the output" is slightly stronger than the code (B).
Stored summary has no ANSI or control sequences and is prose, or is absent (tests with escapes and with overflow)	✅	Escape-free holds structurally through sanitize_summary. Both required tests exist and pass. "Is prose" is now backed by a copy check with the limits in A and B.
Claude Code and Codex harness id stored local_only (fixture stores)	✅	Unchanged since the first review, apart from the doc fix.
bun run test:changed passes; quality:fix clean	✅ (author's claim)	Not re-run in full; the targeted runs above pass.

Code Review Findings

🟡 Important

A. The copy check discards honest summaries that name a file, function or branch
📁 packages/daemon/src/services/terminal_summary.rs:71-74, 151-158

• Problem: any 20-character run without a space that the summary shares with the output discards the whole summary. In a coding session that is an ordinary identifier, not only a path or a key.
• Confirmed with a verbatim copy of the function, against output containing Update(packages/daemon/src/services/local_agent_service.rs), a warning naming build_session_end_properties, and On branch issue-3454-pty-capture-summary:
  • "Edited local_agent_service.rs and ran the tests." is discarded.
  • "Fixed a warning in build_session_end_properties." is discarded.
  • "Worked on branch issue-3454-pty-capture-summary." is discarded.
• Why it matters: the failure is safe (no summary), but naming the file that was edited is what a good summary of a coding session does. Neither live sample has an identifier that long outside an absolute path, so the 6-of-6 kept result says nothing about this case.
• Recommendation: either is enough.
  • Narrow the run check to what it is meant to catch: runs containing /, :// or =, or mixing letters and digits over a longer length. Leave pure [A-Za-z_.-] identifiers alone.
  • Or keep the threshold and add a third live sample whose output names long files and functions, so the discard rate is measured before merge.
  • In both cases, log the discard at info with the reason (run or line), so the rate is visible in the field.
• Principle: a safety check that silently removes the feature for its main use case should be measured, not assumed.

B. A short session echoed back whole is still stored
📁 packages/daemon/src/services/terminal_summary.rs:66-69, 159-162 (module doc 11-16)

• Problem: the line check fires only when a whole output line of 40 or more characters appears in the summary. Two echo shapes get through.
• Confirmed with the same copy, against the output Edit parser.rs / All 42 tests pass / export TOKEN=abc123def456 / Running the parser test suite against the new grammar, 42 tests in 3 files:
  • The reply "Edit parser.rs All 42 tests pass export TOKEN=abc123def456" (three lines verbatim, with a short token) is kept.
  • The reply "It said: Running the parser test suite against the new grammar, 42 tests" (62 characters of one line, not the whole line) is kept.
• Why it matters: the first is the scenario the first review named: a small model shown a short session echoes it. The module doc, ADR-088 and the node-types sheet say the reply "repeats no line of the output", which is more than is enforced.
• Recommendation:
  • Add the first review's other option: discard when the whitespace-normalised summary is contained in the whitespace-normalised output.
  • Replace or supplement the whole-line test with a shared run of consecutive words (eight, say), which catches partial lines and copies that cross a line break.
  • Add both replies above to a_reply_that_repeats_the_output_is_discarded.
  • Otherwise, change the three doc sentences to "no whole line of 40 or more characters".
• Principle: ADR-061 §7: a best-effort check must state its limits and not read as a guarantee.

A and B are not blockers. Core has no sync path, so nothing leaves the machine today, and the result is strictly better than 7b672829. Both are a few lines in one function and belong in this PR.

🟢 Suggestions

C. The .gitignore pattern that hid the module is still there
📁 .gitignore:109-120

• session*.rs is unanchored and matches at any depth; three negations already exist for real modules. Renaming the file avoided the trap and left it for the next session_*.rs.
• Its neighbours debug_*.rs, research_*.rs, check_*.rs and temp_*.rs have the same shape.
• A local test:changed passes with the file on disk, so only the merge gate catches the omission.
• Anchor these to the repo root (/session*.rs, and so on) and drop the three negations.

D. The session-end task skips the shutdown token and pins the PTY session while it waits
📁 packages/daemon/src/services/terminal_summary.rs:231-234; packages/daemon/src/services/agent_session_service.rs:194-244

• Titling races its idle wait against shutdown.cancelled() (local_agent_service.rs:1156-1159). The summary wait does not.
• The spawned task holds its PtySession clone and the capture snapshot until finalize_capture returns, which is as long as a chat turn runs. session is only needed for session.id in the final log line.
• I did not observe harm from either; this is an inconsistency with titling.
• Copy the id out and drop session before finalize_capture. Give LocalModelSummarizer the same select! as titling if a token is within reach.

E. startedAt is in whole seconds
📁 packages/desktop-app/src/lib/components/viewers/ai-chat-pty-session.svelte:173-175; packages/daemon/src/services/agent_session_service.rs:392

• Two launches in the same second tie, and the pick falls back to HashMap order.
• This is only reachable by two panes launching within a second, so it is acceptable as is. Refusing a second launch for a node (the skipped half of finding 5) would remove the case.

Nitpicks

• Nit: packages/daemon/tests/it/live_terminal_summary.rs:11: the documented command is plain cargo test; CLAUDE.md asks for .tools/bin/cargo-nextest nextest run -p nodespace-daemon --test it live_terminal_summary:: --run-ignored only.
• Nit: packages/daemon/src/services/terminal_summary.rs:133-136: a runaway reply is cut at the last ., which can fall inside parser.rs or v1.2. Requiring the . to be followed by a space or the end avoids it.

Checked and found sound

• Engine locality. There are three production engines:
  • LlamaChatInferenceEngine reports local.
  • OpenAiCompatInferenceEngine and NoOpInferenceEngine take the default false.
  • No production wrapper engine exists; the generic RecordingEngine<E> is inside #[cfg(test)].
  • A loopback OpenAI-compatible endpoint also gets no summary, which is the conservative reading of ADR-088.
• copies_output cost. On 1 MiB of output with a 600-character summary, optimised: about 11 ms for prose, about 31 ms for identifier-heavy prose, about 99 ms for a single unbroken 600-character token. No concern.
• still_describes.
  • A type extending ai-chat-pty keeps these fields in the ai-chat-pty and ai-chat buckets (a_type_extending_the_terminal_chat_is_written), so the bucket lookup holds for subtypes.
  • AiChatPtyNode::from_node accepts only the exact type, so reading the buckets directly is the right choice here.
  • The last_active string round-trips unchanged; the summary-writing tests would fail otherwise.
  • Both halves of the condition are exercised: next session started (active), and next session ended (last_active moved).
• Stored nulls. string_prop and the typed converter read null as absent, the generated TypeScript fields are optional, and the viewer uses ?? null and truthiness. session_id: null was already stored this way. No consumer breaks.
• Self-contained branch. Every mod in services/mod.rs, pty/mod.rs and tests/it/main.rs is tracked. No ignored or untracked source file exists under packages/ or scripts/. No reference to session_summary remains except session_summary_prompt.
• CLAUDE.md. No compatibility code, literal node_type comparison, lifecycle_status read, lint suppression or issue number in comments.

Recommendation

APPROVE. The first review's findings are addressed and the fixes for 1, 3 and 4 are structurally right, with tests that fail on the old code. A and B should be folded into this PR: they are small and sit in one function, and the PR's own docs describe what that function enforces. Neither blocks the merge.

[malibio]

Automated pragmatic-code-review (re-review): APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.

[malibio]

Re-review addressed (commit a4891dc)

Addressed

• A. Honest summaries discarded. The long-run check now fires only on a run shaped like a path, a URL, an assignment or a key (it holds /, \, :, = or @, or eight or more digits in twenty characters). The three probes from the review (local_agent_service.rs, build_session_end_properties, issue-3454-pty-capture-summary) are kept, and are in the tests. Discards are logged at info with the reason, without the reply.
• B. Short and partial echoes kept. copied_from_output replaces the whole-line check with two checks: the reply is itself a passage of the output, or it repeats eight words running of it (whitespace collapsed, across line breaks, ignoring the punctuation around the run). Both probes from the review are discarded, and are in the tests.
• C. .gitignore. session*.rs is anchored to the repo root and its three negations are removed. I left the neighbouring patterns alone: they hide nothing today, and changing them is outside this PR.
• D (half). The task drops its PtySession before awaiting the summary.
• Nits. The live test documents the nextest command. A runaway reply is cut at punctuation followed by a space, so not at the dot in parser.rs.

Live run, repeated on this commit: a third sample session that names long files, a function and a branch was added. All 9 replies across the three sessions were kept. Repetitions of a session were identical again, so this is three independent samples.

Stated limits (in the module doc, ADR-088 and the tests): a short secret, a key made only of letters, and a bare host name are not caught.

Skipped

• D (other half): racing the idle wait against a shutdown token. The summarizer holds the process-global agent, which has no token; runtime shutdown ends the task.
• E: two sessions launched for one node within the same second tie on startedAt. Reachable only from two panes within a second.

bun run test:changed passes on this commit. Docs: NodeSpaceAI/nodespace-docs@0d3a24a.

Re-Review Decision

Decision: RE-REVIEW NEEDED (scoped to copied_from_output and its callers)

Rationale: the copy check was rewritten in response to two Important findings. The other changes are small and mechanical.

[malibio]

Pragmatic Code Review: PR #3487

Review Type: Re-Review (second), scoped to commit a4891dca (git diff 4b4a91b4..a4891dca)

Previous Review Summary

	Count
Previous recommendations	7 (findings A–E, two nits)
Addressed	5 (A, C, the drop(session) half of D, both nits)
Addressed with a small gap left	1 (B, see finding 1)
Validly skipped	2 (the shutdown-token half of D; E)
Skipped without good reason	0
New issues	3 Suggestions, 2 nits. No Critical or Important finding.

What I ran

• .tools/bin/cargo-nextest nextest run -p nodespace-daemon --lib terminal_summary: 13 passed.
• A verbatim copy of sanitize_summary, copied_from_output and is_machine_text, built outside the repo, for the probes quoted below.
• Not run: the live test (it needs the model), and the full test:changed.

Verification of the last review's findings

#	Finding	Status	Checked against the code
A	Honest summaries that name a file, function or branch are discarded	✅ Addressed	The three probes from the review are kept, and are in a_long_name_is_not_machine_text_and_a_path_or_a_key_is. The discard is logged at info with the reason and without the reply. Finding 2 lists the shapes that are still discarded.
B	A short session echoed back whole is kept	⚠️ Addressed, small gap	Both probes from the review are now discarded (Passage and Words), and both are in the tests. The same echo with a full stop added is kept: finding 1.
C	.gitignore session*.rs matches at any depth	✅ Addressed	The two tracked matches (packages/agent/src/pty/session.rs, packages/cli/src/commands/session.rs) are not ignored under the anchored pattern. src/acp/session.rs no longer exists, so its negation was dead. No session*.rs exists on disk outside target/, and git status --untracked-files=all is clean, so nothing that was hidden at depth now shows up.
D	Session-end task pins the PTY session	✅ Addressed	drop(session) comes after the last read of session (CompletedSession copies id and node_id), and the log line reads completed.id.
D	No race against a shutdown token	Validly skipped	Confirmed: LocalModelSummarizer holds SharedLocalAgent, which has no token. shutdown_token is on the per-database LocalAgentServiceInner, which the summarizer is not given (assembly.rs:510, 654). The wait is bounded by chat activity, and the runtime ends the task at daemon shutdown. Passing a per-database token in is new wiring for no observed harm.
E	Same-second startedAt tie	Validly skipped	Needs two panes launching a session for one node within a second. The result is the wrong one of two live terminals in the viewer, with no data written.
Nit	Live test documents cargo test	✅ Addressed
Nit	Runaway reply cut at a . inside parser.rs	✅ Addressed	Cut is at . , ! or ? . truncated[..=end] is safe: end is the byte offset of an ASCII character, so end + 1 is a char boundary. Checked with multi-byte text before and after the cut.

Code Review Findings

No Critical or Important findings. Three suggestions, all in one function and all failing safe (no summary) or within the limits the module already states.

🟢 Suggestions

1. An echo with a full stop added is not a Passage
📁 packages/daemon/src/services/terminal_summary.rs:185-187

• Problem: the passage check compares the reply as written. The words check on the next line trims the punctuation around its needle; the passage check does not.
• Confirmed against the test's own short session (Edit parser.rs / All 42 tests pass / export TOKEN=abc123def456):
  • Edit parser.rs All 42 tests pass is Passage. This is the reply the test uses.
  • Edit parser.rs All 42 tests pass. is kept. Six words, so the eight-word check does not apply either.
  • Through sanitize_summary, the raw replies "Edit parser.rs\nAll 42 tests pass." and "- Edit parser.rs\n- All 42 tests pass" are both kept.
• Why it matters: a model usually ends its reply with a full stop, so the scenario finding B named is closed only for a reply without one. The exposure is small: under eight words, and an echo of eight or more is caught by the words check with or without the full stop.
• Recommendation: trim the passage needle the same way (trim_matches(|c: char| !c.is_alphanumeric()), and require it non-empty), and add the full-stop reply to a_reply_that_repeats_the_output_is_discarded.
• Principle: two checks for the same thing should normalise their input the same way.

2. A window of punctuation-only tokens matches any output, and edge punctuation lowers the eight-word threshold
📁 packages/daemon/src/services/terminal_summary.rs:188-194

• Problem: trim_matches runs on the joined window, so a token that is only punctuation at either end of a window is removed whole.
  • If all eight tokens are punctuation, the needle is empty, and output.contains("") is true for every output, the empty one included.
  • If the first or last token is punctuation, the window holds seven words (six if both are).
• Confirmed:
  • copied_from_output("- - - - - - - -", "") returns Words.
  • Step one ... ... ... ... ... ... ... ... finished it. against unrelated output returns Words.
  • It printed — so that it can handle nested — and more returns Words with six shared words.
  • It ran the tests - 14 tests run: 14 passed, 0 skipped - and stopped. returns Words with seven shared words (14 tests run: 14 passed, 0 skipped), a phrase a faithful summary may use.
• Why it matters: the first case is unlikely in a real reply (a four-column Markdown table separator would do it) and fails safe. The second means the test comment "Seven words running ... are not copies" holds only when no dash or other free-standing punctuation sits next to the phrase.
• Recommendation: trim each token's outer punctuation only where it is attached to a word, or skip a window whose first or last token has no alphanumeric character. At the least, require the trimmed needle to be non-empty.
• Principle: an empty needle is a degenerate input and should be handled explicitly.

3. Some names a summary would use are still read as machine text
📁 packages/daemon/src/services/terminal_summary.rs:204-211 (module doc 18-24)

• Problem: : and / in any 20-character window, and eight digits, also match things that are names and not paths or keys.
• Confirmed, each against output that holds the string:
  • Fixed a moved-value error in AgentSessionHandler::finalize_capture. is discarded (a qualified function name).
  • Fixed the error at capture_service.rs:615:9 and reran the tests. is discarded.
  • Wrote 2026-10-02-release-notes.md. is discarded (eight digits in a dated file name).
  • Edited services/capture_service.rs and the tests pass. and Installed nodespace-daemon@0.14.2-beta.1. are discarded.
• Why it matters: the module doc says a long plain name, "a file, a function, a branch", is not a copy. A Rust path to a function and a dated file name are both. The failure is safe, and the info log now makes the rate visible, so this is calibration and not a defect. The third live sample has no :: name or dated file in it, so its 3-of-3 says nothing about these.
• Recommendation: either is enough.
  • Say in the module doc that a qualified name (Type::method), a file:line reference, a relative path and a dated file name count as machine text.
  • Or do not count :: as a :, and leave the rest as is.
• Principle: a best-effort check states its limits in both directions (ADR-061 §7).

Nitpicks

• Nit: packages/daemon/src/services/terminal_summary.rs:28: the doc line was not re-wrapped after the edit (about 95 columns).
• Nit: packages/daemon/src/services/terminal_summary.rs (tests) and packages/daemon/tests/it/live_terminal_summary.rs:71: the fixture branch name issue-3454-pty-capture-summary carries an issue number. It is test data and not a comment, so it does not break the documentation rule, but a neutral 30-character branch name tests the same thing.

Checked and found sound

• Sentence cut. No byte-boundary panic is possible. A reply with no . in its first 600 characters falls through to the word-boundary cut, as before. Two oddities are older than this commit and unchanged by it: e.g. counts as a sentence end, and a reply starting with . is cut to ..
• Empty inputs. An empty or whitespace-only summary returns None from all three checks. An empty output returns None for any reply that holds a word.
• Unicode. The word and run windows are built on chars and split_whitespace, and matched correctly on accented text and an accented path.
• Stated limits. The module doc, the missed cases in the test, ADR-088 and the ai-chat-pty sheet in node-types.md say the same thing: a short secret, a key of letters alone and a bare host name are not caught. The probe agrees (The token is abc123def456. is kept).
• .gitignore. No tracked file became ignored. Cargo.lock and packages/desktop-app/src-tauri/history.txt are tracked and ignored, by other patterns, before and after this commit.
• Live test. The third sample's array length, escape sequences and assertions are consistent with the other two. assert_eq!(copied_from_output(..), None) now reports which check fired.
• CLAUDE.md. No compatibility code, lint suppression, literal node_type comparison or lifecycle_status read in the commit.

Recommendation

APPROVE. Findings A and B are addressed, the tests pin the review's probes, and the skipped items are reasonable to skip. Findings 1 and 2 are a few lines in copied_from_output and are worth folding in; finding 3 can be a doc sentence. None of them blocks the merge.

[malibio]

Automated pragmatic-code-review (second re-review): APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.

[malibio]

Second re-review addressed (commit 0575f4f)

Addressed

• 1. Echo with a full stop. The passage and eight-word checks now compare bare words on both sides (whitespace collapsed, the punctuation around each word dropped). Edit parser.rs All 42 tests pass. and the list form are discarded, and are in the tests.
• 2. Punctuation-only tokens. A token with no letter or digit is not a word, so it neither makes a run of eight nor shortens one. The three probes (- - - - - - - -, the ellipsis run, six shared words between dashes) return None, and are in the tests.
• 3. Names read as machine text. :: is not a separator, judged on the whole word so a run that ends between the two colons does not see a lone one; AgentSessionHandler::finalize_capture is kept. The module doc now lists what is still discarded: a relative path, a file:line reference, a name carrying eight digits.
• Nits. The doc line is re-wrapped; fixtures use a neutral branch name.

Live run on this commit: 8 of 9 replies kept across the three sample sessions. The one missing reply was a failed generation (Metal out of memory while another process held the GPU), not a discard. A first attempt at the run failed outright for the same reason.

bun run test:changed passes on this commit.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: the last review had no Critical or Important findings. This commit changes one function's normalisation in response to its three suggestions, every probe the reviewer gave is now a test, and nothing outside terminal_summary.rs and its fixtures changed.