fix(sessions): make auto-title actually fire (lr-62157d) - #406
Conversation
…(lr-62157d) buildMetaLine never wrote titleAutoGenerated/titleManuallySet/turnCount, and loadSessions never hydrated them, so every daemon restart silently reset all three to false/0. The guards at sdk-message-processor.js and sdk-bridge.js that depend on this state were guarding against state that could never survive a restart -- the keystone cause of auto-title never having fired on this machine (MILLER fnd-d21956). Follows the existing write-when-true convention used by bookmarked/pendingAutoResume. Also persists titleProvisional (used by the follow-up commit's guard fix).
'===' meant any turn incrementing past the threshold without evaluating the gate (early return, error path, non-result terminal event) permanently disqualified the session from auto-title. '>=' lets a missed window self-heal on a later turn; idempotence still comes from !session.titleAutoGenerated (now durable, prior commit).
…l (lr-62157d) Two related defects in the first-message provisional title (project-user-message.js): 1. The provisional title looked exactly like a real title, so autoGenerateTitle's guard (next commit) had no signal telling it the raw 50-char truncation was safe to overwrite. session.title is now tagged with session.titleProvisional=true when set here. 2. A >=500-char paste with no typed text was titled the literal 'Image' -- the client diverts such pastes into msg.pastes[] with msg.text empty (input.js), so msg.text alone always fell through to the 'Image' fallback meant for the genuinely image-only case. Now falls back to msg.pastes[0], with whitespace collapsed before truncating (pastes are commonly multi-line). Extracted as deriveProvisionalTitle(), a pure function, and exported as _test_deriveProvisionalTitle following the _test_-prefixed exposure convention already used by yoke/adapters/codex.js's _test_resolveTitleModel.
…62157d) The guard used to be just !titleManuallySet, which could not tell 'already properly titled' apart from 'still carrying the raw first-message truncation' -- this is the third of three independently correct subsystems whose composition made auto-title dead (MILLER loop_class: double). Now overwrites when (titleProvisional || !title) && !titleManuallySet. Also returns the generateTitle().then() promise chain (previously fire-and-forget) so this is directly awaitable in tests; exposed as _test_autoGenerateTitle following the _test_-prefixed exposure convention.
…-overwrite (lr-62157d) Round-trip, gate-reachability and provisional-overwrite tests for the auto-title fix, per MILLER's diagnosis (fnd-d21956/fnd-3291e4): - session-auto-title-persist: buildMetaLine -> loadSessions round trip for titleAutoGenerated/titleManuallySet/titleProvisional/turnCount. - sdk-message-processor-auto-title-gate: turn-threshold gate fires when skipped past (not just landed on exactly), and titleAutoGenerated correctly suppresses a later fire. - sdk-bridge-auto-title-provisional-overwrite: autoGenerateTitle replaces a provisional title, never a manually-set one. - project-user-message-paste-title: a >=500-char paste with empty msg.text derives a title from the paste (whitespace-collapsed), not the literal 'Image'; image-only still yields 'Image'. All four files verified failing against pre-fix code with the predicted symptom (missing seam / gate never firing / flags reset to undefined on reload) before this fix landed -- 15 failures across the 4 new files plus 1 pre-existing unrelated flake (daemon EX_CONFIG test), none masking a false green.
|
PEACHES — blocking lib/project-user-message.js:46 — amos.code-craft.12 — deriveProvisionalTitle type guard. If msg.pastes[0] is non-string, .replace() throws TypeError. Canonical usage at line 414 assumes string; line 46 lacks guard. Fix: source && typeof source === "string" ? source.replace(/\s+/g, " ").trim().substring(0, 50) : "" Observations: Checked: |
…0] (lr-62157d) PEACHES fnd-439007 (PR #406 review, head_sha f59ef92): msg is raw client-controlled WS JSON with no server-side schema validation (project-connection.js's bare JSON.parse). The bundled browser client only ever sends strings in pastes[] (input.js pushes p.text, always a string, only for a paste >=500 chars), but nothing enforces that for an arbitrary WS client. pastes[0] was never previously consulted for title derivation before this task -- a non-string entry there (e.g. an image-paste-shaped object) is a genuinely new reachable path, not a hypothetical, and would throw a TypeError out of .replace(). Fix: typeof-guard falls through to the existing 'Image' literal for a non-string source -- exactly the case that literal was originally written for. Picking pastes[0] (not scanning for the first string entry) is still correct: the array is homogeneous by client contract even though not server-enforced, so 'first entry' and 'first string entry' coincide for every real client; a defensive type guard on that one entry is the right-sized fix, not a rewrite of the selection logic for a shape no real caller produces. Test added and negative-controlled: reverted this guard alone (keeping the new test) and reran -- the new test failed with the predicted 'source.replace is not a function' TypeError (plus the same unrelated pre-existing daemon-bootstrap-guard.test.js flake seen in the original PR's negative control, confirmed flaky by an immediate clean rerun with no code changes). With the guard restored: 1495/1495 (excluding the same flake) / 1496/1496 on a clean rerun.
|
BOBBIE — clean Reviewed PR #406 (lr-62157d, base 2e006c5..head f59ef92) touching lib/sessions.js, lib/sdk-message-processor.js, lib/project-user-message.js, lib/sdk-bridge.js, plus four new test files. Findings: none blocking, none nit.
Scanners: gitleaks detect (range 2e006c5..f59ef92) - no leaks found. trufflehog git (same range) - 0 verified/unverified secrets. semgrep (--config=auto and --config=p/javascript against the four changed files materialized from the head commit, run at /workspace/clagentic-console) - both invocations stalled past 150s at the registry-fetch stage (no outbound network in this environment); scanner_status=unavailable, judgment-only SAST applied. osv-scanner - skipped, no package.json/package-lock.json change in this diff. Early-exit: not applicable, full manual review performed. |
…-62157d) BOBBIE (PR #406 review, comment 5403852401, non-blocking code-craft polish): plain substring(0, 50) indexes by UTF-16 code unit, so a cut landing inside a surrogate pair (most emoji, many CJK supplementary-plane characters) produces a lone unpaired surrogate -- renders as a broken/replacement glyph in the sidebar. Newly reachable in a way it wasn't before this task: pasted content is now a title source, and pasted text is far more likely to contain emoji/CJK than a typed first line. No existing safe-truncation helper in the codebase (checked -- every other truncation call site uses plain substring; out of scope to fix those here, this is a minimal fix for the newly-reachable path only). Array.from(str) iterates by Unicode code point (Node >=20 floor supports this natively, no dependency needed), so slicing the resulting array and rejoining never splits a surrogate pair -- extracted as truncateCodePointSafe(), used only by deriveProvisionalTitle. Test added and negative-controlled: reverted this fix alone (keeping the new test) and reran -- failed with the predicted lone unpaired high surrogate (U+D83D) in the title. With the fix restored: full suite passes (excluding the same unrelated pre-existing daemon-bootstrap-guard.test.js timing flake noted in the prior commit, confirmed flaky again by an immediate clean rerun).
|
PEACHES — clean (0 blocking findings) fnd-439007 (PEACHES): type guard on msg.pastes[0] correctly prevents TypeError when a non-string paste entry reaches .replace(). Guard at lib/project-user-message.js:73 uses typeof check, falls through to Image literal as intended. BOBBIE fold-in: truncateCodePointSafe() helper (lines 41-45) correctly uses Array.from(str) to iterate by code points, preventing lone-surrogate splits at emoji/CJK boundaries. Node >=20 floor supports this natively; scoped to single call site (line 74) — appropriate minimal fix. titleProvisional flag distinguishes first-message fallback from LLM-generated title. Guard at sdk-bridge.js:668 correctly allows auto-title replacement while protecting user-set titles. Durability round-trip verified: lib/sessions.js:165-168 writes all four flags (titleAutoGenerated, titleManuallySet, titleProvisional, turnCount) write-when-true; lines 766-769 restore with proper defaults. Persistence survives daemon restart. Turn-threshold gate fixed: sdk-message-processor.js:792 uses >= (not ===), enabling self-healing when a session skips the exact turn-2 window. Test suite: four regression-test files added, each driving real code. Negative-control verified: each fix reverted alone with test kept confirms expected failure (TypeError: source.replace is not a function; lone surrogate in title). codePointAt(0) correctly identifies surrogates. Test assertions match failure modes. No regressions: YOKE routing, brand rules, persistence schema all unchanged. |
|
BOBBIE re-audit of PR #406 (2e006c5..72fdf3b), superseding the prior clean verdict on f59ef92. Self-fetched head_sha 72fdf3b via bobbie-github-security read, confirmed against local git and matching the claim. Scope: two fold-in commits (a27a6ca, 72fdf3b) on lib/project-user-message.js since the prior audit -- the typeof-guard on msg.pastes[0] before .replace() (PEACHES fnd-439007), plus AMoS truncateCodePointSafe() surrogate-pair fix (both from my prior nit). Reviewed the unvalidated-WS-input observation AMoS surfaced (msg.pastes has no server-side schema validation; project-connection.js does a bare JSON.parse). Traced all consumers of msg.pastes: deriveProvisionalTitle() is the only call site invoking a string method on pastes[0], and it is now typeof-guarded (falls through to Image for non-string). The history-recording call site (~line 414, sm.recordHistoryEntry/appendToSessionFile) stores msg.pastes as opaque JSON with no method call on it -- not a type-confusion path. autoGenerateTitle/generateTitle in sdk-bridge.js consumes userMessages (built from message text), not raw pastes[], so the YOKE LLM adapter is not exposed to an unvalidated pastes shape either. No further finding beyond the fix already landed. Scanner coverage this run:
No findings against this diff. |
|
Merged via clagentic-loadout v0.2.0
|
Fixes lr-62157d: auto-title has never fired once on this machine.
MILLER diagnosed (fnd-d21956/fnd-3291e4, conf 0.93/0.9) three independently-correct subsystems whose composition made the feature dead (loop_class: double). Live evidence: across ~160 real session files, zero meta lines ever contained titleAutoGenerated or titleManuallySet, zero titles were LLM-generated - every title is a raw 50-char mid-word truncation, and zero [auto-title] log lines exist despite logging on every branch.
Changes (all four required; the provisional-title tag is the keystone)
lib/sessions.js: buildMetaLine() never wrote titleAutoGenerated, titleManuallySet or turnCount, and loadSessions() never hydrated them. Every daemon restart silently reset all three to false/0. Now persisted write-when-true, matching the existing bookmarked/pendingAutoResume convention. Also persists titleProvisional.
lib/sdk-message-processor.js: the turn-threshold gate used strict equality, so any turn incrementing past the threshold without evaluating the gate permanently disqualified the session. Changed to greater-or-equal; idempotence still comes from titleAutoGenerated being false (now durable).
lib/project-user-message.js: the first-message provisional title looked exactly like a real title. Now tagged with session.titleProvisional = true. Also fixes the Image paste bug (MILLER fnd-3291e4): a paste of 500+ chars with no typed text is diverted server-side into msg.pastes[] with msg.text empty, so it always fell through to the Image fallback meant for genuine image-only messages. Now sources the title from the first paste, whitespace-collapsed before truncating. Extracted as a pure deriveProvisionalTitle() helper, exported as _test_deriveProvisionalTitle.
lib/sdk-bridge.js: autoGenerateTitle guard now overwrites when the title is provisional or absent, and the session has not been manually renamed. Exposed as _test_autoGenerateTitle for direct testing.
Fold-in 1: PEACHES fnd-439007 (comment 5403805146, against head_sha f59ef92)
deriveProvisionalTitle() lacked a type guard on msg.pastes[0]: msg is raw client-controlled WS JSON with no server-side schema validation. The bundled browser client only ever sends strings in pastes[], but nothing enforces that for an arbitrary WS client, and pastes[0] was never previously consulted for title derivation before this task - a non-string entry there is a genuinely new reachable path that would throw a TypeError out of .replace(). Fixed with a typeof guard that falls through to the existing Image literal.
pastes[0] is still the right index to pick: the array is homogeneous by client contract even though not server-enforced, so first entry and first string entry coincide for every real client; a defensive type guard on that one entry is the right-sized fix, not a rewrite of the selection logic for a shape no real caller produces.
Fold-in 2: BOBBIE (comment 5403852401, clean review, one non-security code-craft nit)
Surrogate-pair truncation: plain substring(0, 50) can split a multi-byte UTF-16 sequence, producing a broken glyph. Newly reachable because pasted content (far more likely to contain emoji/CJK) is now a title source. No existing safe-truncation helper in the codebase; added a small truncateCodePointSafe() using Array.from (Node 20 floor supports this natively, no new dependency), scoped to this one call site rather than a codebase-wide truncation sweep.
Both fold-ins have negative-controlled regression tests: each was reverted alone (keeping its test) and rerun, confirmed failing with the predicted symptom (TypeError for the type guard; a lone unpaired surrogate code unit for the truncation fix) before being restored.
Tests
Five test files, all driving real production code (no reimplementation):
Test run
npm test (via scripts/check-test-count.js, hard per-file result check plus a 1300-test floor): 1497/1497 passing on a clean rerun, exit 0. One unrelated pre-existing daemon-bootstrap-guard.test.js timing flake observed intermittently across runs, confirmed flaky (passes on immediate rerun with zero code changes) and unrelated to this diff.
Notes
TASK: lr-62157d