diff --git a/conductor/tracks/nagent_review_20260608/nagent_review_v3_20260619.md b/conductor/tracks/nagent_review_20260608/nagent_review_v3_20260619.md index ba7977ea..4c8dbc5c 100644 --- a/conductor/tracks/nagent_review_20260608/nagent_review_v3_20260619.md +++ b/conductor/tracks/nagent_review_20260608/nagent_review_v3_20260619.md @@ -352,7 +352,81 @@ The `315fe9e` commit is the verification-discipline precedent worth carrying for ## §7 Robustness -(filled in by Phase 8 — covers `065168c`, `6b762da`, `12c35b7`, `49e07f3`) +**Source:** nagent `065168c`, `6b762da`, `12c35b7`, `49e07f3` (`bin/helpers/nagent_tags.py:43-50` + `:106-110` + `:136-246` + `:248-265`, `bin/nagent:1911-1940` + `:682-714` + `:1319-1381` + `:1387-1394` + `:1534-1551` + `:1834-1840` + `:224-240`, `tests/test_nagent.py:548-590` + `:679-714` + `:1911-1940`, `tests/test_nagent_safety.py:367-400`, `tests/test_nagent_tags.py:170-182`). +**One-liner:** Four hardening commits — `scan_tag_document` extracts valid tags and ignores the rest (with EOF-capture for trailing unclosed responses); `dedupe_nodes` collapses exact-duplicate action tags within a turn; ``-output-before-`` ordering is pinned by a regression test; `` is scoped to a per-conversation scratch dir so concurrent instances never collide. +**Pattern(s) vs v2.3:** UPDATE. v2.3 Pattern 5 ("the loop") had the basic loop; v3 hardens it against four specific failure modes. The hardening is incremental — each commit is a discrete change with its own test. EXTENDS v2.3 Pattern 4 ("visible output protocol") with a lenient counterpart (`scan_tag_document`) that tolerates non-protocol output while still propagating known-tag malformation as a hard error. NEW: per-conversation scratch directory as a side artifact of the loop. +**Manual Slop implications:** Manual Slop's `send_result()` (per `docs/guide_ai_client.md`) and `dispatch_inference` should adopt the same hardening. The lenient parser discipline ("scan, extract, ignore the rest, but propagate known-tag malformation as hard error") maps to Manual Slop's tag protocol; the per-turn status block (`` with UTC + cumulative tokens) is a model Manual Slop's discussion history could adopt — the user can already see token totals but not in a structured per-turn way. The per-conversation scratch dir (keyed by conversation name) maps to Manual Slop's `tests/artifacts/` directory (gitignored, per-conversation). +**Decision candidate:** NEW Candidate 23 (MEDIUM). "Per-conversation scratch directory for Manual Slop dispatch_inference" — adopt the `conversation_scratch_dir(conversation_name)` pattern; pre-create on session start; thread through the ``-equivalent. See `decisions.md` Candidate 23. +**Cross-refs:** §3 Hooks (per-turn `` and per-turn hooks are both per-turn observability surfaces); §2 Conversation safety net (the `` block is what the safety net reads to compute the checkpoint delta). +**Source-read citations:** +- `bin/helpers/nagent_tags.py:43-50` — `parse_element(..., capture_to_eof_if_unclosed=True)` for trailing unclosed `` (065168c) +- `bin/helpers/nagent_tags.py:106-110` — EOF-capture behavior: a missing close tag captures to `len(text)` instead of raising (065168c) +- `bin/helpers/nagent_tags.py:136-246` — `IgnoredSpan` + `_read_tag_name` + `scan_tag_document` (lenient parser) + `serialize_node(s)` (re-serialize well-formed) (065168c) +- `bin/helpers/nagent_tags.py:248-265` — `dedupe_nodes` (6b762da) +- `bin/nagent:1911-1940` — `cleaned_response_text` returns `(text, duplicates_removed)`; system note when collapsed (6b762da) +- `bin/nagent:682-714` — `test_shell_output_precedes_next_input_in_either_order` regression test (12c35b7) +- `bin/nagent:1319-1331` — `conversation_scratch_dir(conversation_name)` returns `$TMPDIR/nagent-{name}/` (49e07f3) +- `bin/nagent:1334-1341` — `is_within(path, directory)` (replaces `is_tmp_path`) (49e07f3) +- `bin/nagent:1344-1381` — `validate_write_path(..., scratch_dir=...)` — only path-inside-scratch-dir is allowed; file-edit mode unchanged (49e07f3) +- `bin/nagent:1387-1394` — `execute_write(..., scratch_dir=...)` threaded through (49e07f3) +- `bin/nagent:1534-1551` — `process_tags` computes scratch_dir per call (49e07f3) +- `bin/nagent:1834-1840` — `run_agent_loop` pre-creates scratch_dir before the first turn (49e07f3) +- `bin/nagent:224-240` — `file_edit_rules(file_edit_path, scratch_dir)` — context mentions the concrete scratch path (49e07f3) +- `tests/test_nagent.py:548-590` — 3 cleaned/duplicate tests (6b762da) +- `tests/test_nagent.py:679-714` — `test_shell_output_precedes_next_input_in_either_order` (12c35b7) +- `tests/test_nagent_safety.py:367-400` — `test_duplicate_tags_collapsed_in_conversation_without_sidecar` (6b762da) +- `tests/test_nagent_tags.py:170-182` — `DedupeNodesTests` (6b762da) +**Honest gaps in this cluster:** +- `dedupe_nodes` only catches EXACT duplicates (same name, self_closing flag, attrs, content). A near-duplicate (same command with whitespace differences, same shell with env vars) is not collapsed. Whether this matters in practice is unverified. +- The lenient parser's "ignore the rest" behavior could mask real protocol bugs — the model might be silently emitting junk while the conversation proceeds. The `ignored_correction` system note at `bin/nagent:1930` is the recovery path; it relies on the model reading the note. A future track could add a hard error when the ignored-to-extracted ratio exceeds a threshold. +- The scratch dir at `bin/nagent:1319-1331` is keyed on conversation name; if a user renames a conversation file mid-run, the scratch dir becomes orphaned and a new one is created. Unverified whether this is the intended behavior. +- The `` block at the end of every turn (per `bin/nagent:1940`) is observability but not user-facing; the user sees cumulative tokens via the existing `TokenStats` rollup. The status block's primary consumer is the safety net, not the user. + +**Pattern deep-dive.** The robustness commits are four independent hardening operations on the loop: **tolerate**, **dedupe**, **pin-order**, **scope**. Tolerate: `scan_tag_document` extracts valid tags and ignores the rest, with two carve-outs — malformed *known* tags propagate as hard errors (a clear protocol mistake), and a trailing unclosed `` captures to EOF (so a finished run isn't lost to a missing close tag). Dedupe: `dedupe_nodes` collapses exact-duplicate tags within a turn, with a system note when it fires (so the model knows it stuttered and emits each action once next time). Pin-order: the ``-output-before-`` ordering is pinned by `test_shell_output_precedes_next_input_in_either_order` — the regression test is the contract; the implementation "holds by construction" but was previously unpinned. Scope: `` is restricted to a per-conversation scratch dir, eliminating the cross-instance collision class on shared `/tmp` paths. + +The four changes share a data-oriented theme: each is a discrete transformation with its own invariant, test, and comment, and each operates on data on disk rather than on the model's behavior. The `ignored_correction` system note is the only exception — it's a prompt-side intervention that asks the model to read and adjust. The rest are pure-code or pure-data. + +The lenient parser is the most subtle of the four. The strict `parse_tag_document` raises `TagParseError` on any malformation; the lenient `scan_tag_document` returns `(nodes, ignored)` where ignored is the list of `IgnoredSpan` (reason + text + offset). The two callers — `parse_response` (in the hot path) and `cleaned_response_text` (for storage) — use different policies: `parse_response` propagates `TagParseError` on known-tag malformation (the loop must ask the model to fix it); `cleaned_response_text` is more permissive (storage should be robust to whatever the model emitted). The split is the data-oriented response to "lenient storage, strict dispatch." + +A code-shape sketch using survey grammar: + +``` +scan { text, known, unwrap, eof_capture } :: (nodes, ignored) {ssdl} [I] + pos := 0 + while pos < len(text) { + if text[pos] is whitespace -> pos += 1 + elif not _read_tag_name(text, pos): + nxt := text.find("<", pos + 1) + end := len(text) if nxt == -1 else nxt + ignored += ("non-tag text", text[pos:end], pos) // skip to next tag + pos := end + elif name in known: + // strict: propagate errors for malformed known tags (except EOF-capture) + node := parse_element(text, pos, capture_to_eof=(name in eof_capture)) + nodes += node + pos := node.end + else: + try node := parse_element(text, pos) // try parsing unknown tag + except TagParseError: ignored += ("malformed ", text[pos:end], pos); pos := end + if name in unwrap: recurse into node.content + else: ignored += ("unknown tag ", text[node.start:node.end], node.start) + pos := node.end + } + +dedupe { nodes } :: nodes {ssdl} [S] + seen := {} + out := [] + for node in nodes { + key := (name, self_closing, sorted(attrs), content) + if key not in seen: seen += key; out += node + } + +scratch-dir { conversation_name } :: path {ssdl} [S] + return tmp_roots()[0] / f"nagent-{conversation_name}" + // keying on name (not per-process guid) keeps it stable across resumes +``` + +The `{ssdl}` markers note the abstractions: `scan` is an inspectable transformation (I) that produces both valid nodes and ignored spans; `dedupe` and `scratch-dir` are pure string concatenations (S). The `` block (per `bin/nagent:1940`) is the per-turn observability surface that consumes `scan`'s output (the ignored count and the duplicates count feed the block's token totals + sidecar refs). ## §8 Operating rules