Review fold-in before push, twelve findings, two of them majors.
The handoff popped the interjection queue destructively and handed
the text to a send with a non-delivering refusal (the budget latch)
and a preamble that can raise before the user turn is appended — a
failure destroyed the user's words with a log line, after the charged
wake nudges were already cleared. Now: the budget latch is checked
before the pop (the message stays queued for a send with a human in
front of it, and the wake drain still runs so the worker's exit
converges); the pop returns the raw items and any non-cancel escape
restores them verbatim — ids and priorities intact — before the
failure surfaces; a cancel deliberately does not restore, because the
Stop supersedes the queued words. Content-free items (a bare priority
marker) are skipped at the shared renderer, so a lone '!!!' no longer
buys a content-free turn at the cost of both nudges.
The per-child fact block takes the roster formatter's bounds: fact
lines cap at the display cap with a counts-only overflow line, and
the wait slot keeps its larger handle cap — the body is a persistent
system turn replayed on every request, and the block previously grew
without bound as finished-but-unclosed children accumulated. The two
fact sentences and the overflow line are named template constants,
and every test assertion anchors on them; the children projection
takes the same drop-never-mangle alteration check as the open-row
fields.
Eval world seeding: node metadata is JSON-encoded exactly as
production writers store it (a raw string never matched a filtered
list_nodes lookup), the stub client pins its heartbeat window open so
a static world cannot go hollow mid-run, and the world-shape
refusals' field branches gain their own tests. Comment accuracy and
paragraph wrapping fixed at the sites the review named.
Three fixes, one per causal mechanism the round-12 baseline exposed.
The done branch moves ahead of the escalate branch. The escalate-first
order rested on a harm argument — guessing on an operator decision
outranks redone bookkeeping, so the escape hatch should be salient —
and the baseline measured its cost: 7 of 10 finished-unmarked runs
reached for the body's first populated call and escalated visibly
finished work, one mode, no tail. The next round measures the reversal
both ways: if the legit-stop cells' forbidden rate rises, the harm
argument was right and the order flips back (the pin says so in
place).
The approval-stop cell's transcript anchors its world — named repo,
named migration, named artifacts. Its forbidden runs were not sign-off
defiance: the model swept empty discovery surfaces, found a void, and
spawned explore-the-project children, so the cell was measuring
hollow-world exploration rather than dispatch discipline.
The co-delivery cell's running child gains an observations-only
progress note beside its assignment. A bare-assignment static child
cannot survive sustained honest interaction — wait times out, inspect
shows nothing, and after patience cycles the model correctly diagnoses
a hung child and cancels/respawns, which the forbid list scored as
redo. The note makes the child look alive without looking finished.
The idle-children header drops its opening idleness claim: a queued
entry delivers at whichever seam arrives next, and the drain
predicate re-verifies that children are active — never that the
coordinator is still idle — so the body now opens with the one fact
the delivery just verified.
The tasks body replaces its hedged children sentence with one
observed-fact line per child. The old sentence hedged states the
producer's read had just returned and invented activity for an idle
coordinator; the producer now threads (ws_id, state) pairs through,
and the formatter renders a running child as running (check before
redoing what it owns) and a stopped one as stopped, with the
tool-behaviour fact that wait_for_workstream returns immediately for
it. The line asserts nothing about results: no read observes whether
a child produced anything, and the immediate wait is the whole
protection — checking is cheap and finds whatever is there. Fact
lines are formatter-built beside the counts opener, so no tail
override can reach them; the formatter's old indeterminate-read hedge
branch is deleted (a failed read renders no body at all), and the
open-row status takes the same alteration check as the id.
Both bodies hand the model full workstream ids: the resolver refuses
truncated ids by design, so the roster's 8-char prefixes were not
handles — a model copying a bullet issued a call the resolver
rejects. Display prefixing stays on the operator card, derived from
the full id in the metadata.
Eval alignment: fixture child ids become production-shaped 32-hex (a
prefix looked like a different id entirely and the old shape only
resolved through the legacy branch); a body-override sweep refuses
cells without a live child at config time, keyed on the formatter's
own childless condition, so candidate text can never be measured over
a world production cannot produce.
Two nudge classes can fire from one IDLE event, tasks first, each
asserting only its own domain.
idle_tasks (advice) fires when open (pending/in_progress) tasks
exist. The body is a counts opener, the open task ids with statuses,
and typed branches that each end in a runnable tasks(...) or
wait_for_workstream(...) call populated with real server-minted ids.
Everything it says about children is governed by one observed fact:
live children present adds the caveat sentence and the
blocked-on-a-child branch; affirmatively none says nothing about
children at all. Any needs_user row parks the class entirely — at the
fire gate and the drain predicate — because with no task graph an
open task may be gated on a parked one's unanswered question; the
operator's answer is the re-arm. Gated on memory.nudges and on the
persona actually exposing the tasks tool; carries the per-class
cooldown as well as the per-bracket cap. The tasks tool itself gains
the needs_user status and a note field — the typed escalation the
body's branches point at.
idle_children (liveness) fires when children are in a live state —
the wake that lets an idle coordinator collect a finished child's
results. The body is a roster of workstream id prefixes and states,
never names: a child's name is model-authored text and does not enter
a system turn. Cap-only and cooldown-free by design, not gated on
memory.nudges, and it survives an operator Stop.
Fail-closed, event-wide: if any storage read fails while the observer
handles an IDLE event, neither nudge is queued and neither cap is
charged. Both paths run as side-effect-free plans; the commit tail is
storage-free, so no read can fail past the veto point; both drain
predicates drop on a failed read. A path's own fault (a generic
raise) still costs only that path's fire, so one class's bug cannot
strand the other.
Task text is stored verbatim and projected per audience at render:
the model-facing projection deletes angle brackets, the operator
projection keeps them, and both strip newlines and bidi/zero-width
runs. Idle cards render what the model was told, formatted for the
operator, never augmented with content the model did not receive.
Swap the trust-fence marker shape from <tag_nonce>...</tag_nonce> to
[start tag_nonce]...[end tag_nonce] for both the operator fold (system-reminder)
and the output-guard judge (tool_output). Angle-bracket markup pushed some local
models out of distribution and toward emitting their own turn-structure tokens:
chat templates built around rigid <...>-style structural tokens derail once a
few folded reminders accumulate. The start/end keywords carry no slash (no </ or
[/ closing-tag shape) and read as ordinary text.
Single-source the shape in fence.py (_OPEN_KW/_CLOSE_KW + detection_pattern) so
wrap, neutralize, the forgery/leak detector, and both trust declarations track
one definition. The nonce still rides both boundaries (unforgeable close); the
leak-vs-forgery split and the forge-in / break-out defang are preserved. The
fold is wire-only, so there is no migration; the legacy persisted-envelope
readers keep the old shape.
Add regression tests pinning each trust declaration to fence.wrap's emission so
a future keyword change fails loudly instead of silently desyncing the anchors.
Closes round-2 review findings q-3, q-4, q-5, q-7.
* **q-4:** ``_NAME_CONTROL_CHARS`` and ``_PAYLOAD_CONTROL_CHARS`` shared
7 lines of Unicode-steering character classes (zero-width / bidi /
separators / BOM / tag chars above BMP). Factored into a single
``_CONTROL_CHARS_TAIL`` constant; each regex now differs only in its
leading ASCII range. Future bidi or zero-width additions edit one
place.
Side effect: this corrects a latent bug where ``_NAME_CONTROL_CHARS``
had two literal ASCII spaces in place of U+2028 / U+2029 (line and
paragraph separators) — visible as ``r" "`` in source but rendered
as the actual codepoints in ``_PAYLOAD_CONTROL_CHARS``. After the
factoring both regexes correctly include U+2028 / U+2029, closing
the gap that would have let a workstream name with embedded line
separators forge a sibling bullet (the same vector ``\n`` was
blocked for in the original bug-1 fix).
Switched to ``\u`` escapes for readability (and to keep future Edit
tool runs against this block reliable).
* **q-3:** Tombstone clause "standing in for the deleted
``_watch_pending`` maxsize bound" survived in
``ChatSession.set_watch_runner``'s docstring after the apply-pass
trim cleaned the inline soft-cap comment. Dropped.
* **q-5:** ``test_newline_in_name_does_not_forge_extra_bullet`` carried
five WHAT-narration comments restating what the immediately-following
asserts already say. Dropped — the docstring carries the security
invariant; the assertions speak for themselves.
* **q-7:** ``patch_session_storage`` had a 14-line docstring including
fallback-guidance and self-justification ("accumulated 7 near-duplicate
sites"). Trimmed to a 3-line contract.
Closes review finding bug-1.
The shared ``sanitize_payload`` regex preserved TAB/LF/CR so multi-line
watch shell output kept its layout — necessary for the watch path, but a
correctness gap for the idle_children formatter, which renders the
user-controlled ``name`` field as a single bullet item. A child name
with an embedded ``\n`` would split the bullet across two rendered rows
and let a hostile name forge a fake sibling entry in the listing.
Splits the regex in two: ``_NAME_CONTROL_CHARS`` strips TAB/LF/CR
(used by the new ``sanitize_name`` helper for single-line name fields),
``_PAYLOAD_CONTROL_CHARS`` keeps the existing permissive shape (used by
``sanitize_payload`` for multi-line watch payloads).
``format_idle_children_nudge`` now calls ``sanitize_name``.
Adds ``test_newline_in_name_does_not_forge_extra_bullet`` — feeds a
hostile name with embedded ``\n`` + bullet-shaped continuation, asserts
the rendered listing still has exactly N bullet rows for N children
(no forged sibling), and the hostile newline got flattened to an inline
space. Adds a ``TestSanitizeName`` class mirroring the existing
``TestSanitizePayload`` shape for the new strict variant.
Renames _sanitize_child_name to sanitize_payload and widens it to be
the shared producer-side sanitiser for both idle_children and the
incoming watch_triggered nudges. The regex now skips TAB / LF / CR
so multi-line shell output rendered into a watch payload keeps its
line structure when sanitised as a whole formatted message — the
pre-switchover code path collapsed multi-line output to one line.
Adds the watch_triggered entry to _NUDGE_MAP alongside idle_children
so ``_NUDGE_MAP``-as-registry consumers (should_nudge gating, future
audit / UI tagging) recognise the type. Body is empty — payload
comes from the producer (the watch dispatch closure), same shape as
idle_children.
Implements watch-switchover plan section 3.2 plus risk register R8
(TAB/LF/CR exclusion) and step 3 (_NUDGE_MAP registration).
Adds the first concrete consumer of the wake trigger: when a coordinator
goes IDLE while interactive children are still running, a
``CoordinatorIdleObserver`` enqueues an ``idle_children`` nudge that the
``IdleNudgeWatcher`` then dispatches as a synthetic empty-user-turn
``send``. The model receives a system-reminder body listing the active
children (capped at 6 inline + 32 in the suggested ``wait_for_workstream``
call) and a nudge to block on them rather than reply prematurely.
Observer gates (in order): coord-only filter, skip if last assistant
turn used ``wait_for_workstream``, per-(ws, nudge_type) hard cap (3)
that resets only on non-wake leave-IDLE, active-children query,
``should_nudge`` cooldown. Console lifespan registers the observer
BEFORE the watcher so subscriber-fire order has the observer
enqueueing first on the same IDLE event.
Adds an opt-in ``valid_until`` predicate on ``NudgeQueue.enqueue``
(R9 from the design risk register) — drain re-checks the predicate
outside the queue lock; falsy / raising drops the entry without
delivering it. ``deliver_wake_nudge_from_queue`` now drains inline
before synthesizing the empty user turn so a stale predicate-drop
doesn't leave the wake send with empty content; ``_attach_pending_user_reminders``
consumes the pre-drained reminders via ``_wake_drained_reminders``.
The observer's ``valid_until`` uses ``count_workstreams_by_state``
(boolean check, no row fetch) instead of full ``list_workstreams``,
keeping the chat-loop user-attach path off the heavy query.
User-controlled child workstream names are sanitized
(``_sanitize_child_name``) before interpolation so a name like
``</thinking>...`` can't steer the model's reasoning channels through
the rendered body — the wire-boundary ``escape_wrapper_tags`` only
covers ``<system-reminder>`` / ``<tool_output>`` envelopes.
Cleanup pass on the metacognitive nudge stack — restores pre-split
errored-counts-toward-repeat behaviour and tightens the is_error
plumbing through the per-batch advisory hook.
The per-batch hook in ``_run_loop`` was duplicating the is_error
signal: ``self._tool_error_flags`` (set by ``_report_tool_result``)
and a string-prefix tuple (``Error`` / ``JSON parse error`` / …).
Two truth sources is what got us here — bash commands that exit
non-zero with normal stdout matched the flag but not the prefix,
the deny path matched the prefix but not the flag, and the result
was that stuck-loop detection silently broke for the most common
failure mode (the model bashing the same broken command).
Single source of truth now:
- ``_execute_tools.run_one`` deny branch routes through
``_report_tool_result(is_error=True)`` so denied calls populate
``_tool_error_flags`` like every other error path.
- The error-prefix tuple is gone; the write-success-clear gate and
the tool-error-nudge gate both read ``_tool_error_flags`` only.
Repeat-detection state moves from a ``set[str]`` (fired on the second
identical call, ignored errors entirely) to a ``RepeatDetector``
helper in ``metacognition.py`` with consecutive-streak semantics:
- Threshold raised from 2 to 3 — two-in-a-row was noisy on
legitimate transient retries; three is the cheapest stuck-loop
signal.
- Recording a different signature resets the count, so [A, A, B, A]
is two short streaks of 2 and not a streak of 4. Bounded by O(1)
state regardless of session length.
- Errored calls now count toward the streak (the split into a
separate metacog module unintentionally introduced a "skip errors"
branch — restored).
While there:
- ``metacognition._COOLDOWN_SECS`` default aligned to 300s (matches
``MemoryConfig.nudge_cooldown`` and the ``memory.nudge_cooldown``
config-store default; was set to 30 by an earlier investigation).
- The per-batch advisory block (~80 lines of mixed orchestration
inside ``_run_loop``) is extracted to
``ChatSession._apply_post_execute_advisories`` so the wired
behaviour is testable without driving ``_run_loop`` end-to-end.
Producer extraction to a dedicated module is deferred to a
follow-up; advisory producers all live on ``ChatSession`` for
now per existing convention.
- Frontend ``appendToolOutput`` (turnstone/ui/static/app.js) now
skips rendering when the parent approval block is denied or
the output starts with ``Denied by user`` / ``Blocked``,
mirroring the history-replay guard at ``_build_history``.
Previously the live SSE path didn't need this guard because
the deny path never emitted a ``tool_result`` event; the
is_error routing change above means it does now, so without
this guard the badge from ``resolveApproval`` and the SSE
output would both render.
Tests: 8 unit tests for ``RepeatDetector`` covering streak,
threshold, clear, and intervening-sig reset; 9 integration tests
for ``_apply_post_execute_advisories`` covering the wired
behaviour (3-identical fires warning + advisory + UI line, errored
calls count toward streak as a regression guard, intervening sig
resets streak, successful write clears, failed write does not,
JSON outputs tracked but not inline-warned, tool_error nudge gates
on memory_count, repeat UI line emitted on streak fire).
When a model calls the same tool with identical arguments as a previous
call, append a warning to the tool result and inject a metacognitive
nudge. This breaks loops where small local models get stuck repeating
the same action (e.g. running the same bash command 3+ times).
The repeat signature set is cleared after a warning fires, giving the
model a clean slate. Also cleared on conversation compaction.
Ref: #186
Add tool_error nudge type that fires when a tool returns an error,
prompting the model to search memories for prior feedback about the
tool or error pattern before retrying.
- Gated on nudges config (respects nudges=false)
- Only fires when memories exist (no noise on fresh workstreams)
- Broad error detection: Error*, *error:*, Command timed out, Unknown tool
- Nudge wording aligned to memory(action='search') convention
- Respects existing cooldown (5 min) and rate limiting
- 4 new tests
* fix: reduce metacognition false positives with strong/weak pattern tiers
Correction detection: split "no" handling — "no," and "no." are strong
(always fire), "no <word>" uses an allowlist of correction-context words
(pronouns, demonstratives, verbs) instead of a blocklist. Phrases like
"no problem", "no worries", "no rush" are excluded automatically.
Completion detection: move most patterns to weak tier, gated by message
length (<80 chars) and absence of continuation markers ("?", "can you",
"but", "now", "please", etc.). "thanks for X" excluded at regex level.
Strong tier (always fire): "that's all", "lgtm".
* fix: align allowlist comment with implementation (include articles)
* feat: structured memory system — typed/scoped memories with BM25 relevance and metacognitive prompting
Replace flat key-value memories table with structured_memories (migration 014).
Four memory types (user/project/feedback/reference), three scopes
(global/workstream/user). Consolidate remember/recall/forget into two tools:
memory (action-based: save/search/delete/list) and recall (conversation
history only).
BM25 relevance scoring (extracted to turnstone/core/bm25.py) selects top-5
memories for system message injection based on conversation context.
Metacognitive prompting injects ephemeral nudges after corrections, tool
denials, workstream resume, and completion signals.
Scope isolation enforced: system message injection and nudge counts filtered
to visible memories only (global + current workstream + authenticated user).
User scope requires authentication. Content capped at 32KB. ILIKE/LIKE
metacharacters escaped in both backends.
113 new tests (2053 total).
* fix: CI failure + copilot review feedback
- Fix time.monotonic() cooldown: use None sentinel instead of 0.0
default (monotonic clock starts at boot, not epoch — fresh CI
runners have uptime < 300s so cooldown check always triggered)
- Catch sa.exc.IntegrityError specifically in upsert instead of
broad Exception (copilot review)
- Preserve existing description/type on upsert when caller doesn't
explicitly set them (copilot review)
- Add last_accessed + access_count columns to schema/migration for
future LRU/LFU eviction support
Patrick Buckley