fix: preflight diagnostic toast — false unknown-hook, doubling, placement (lr-e901)#302
Conversation
…TS (lr-e901) The CLI-stderr diagnostic path (diagnostic-patterns.js) matched 'Unknown hook event' lines generically and forwarded the CLI's own message text without checking whether the Console's allowlist (settings-preflight.js VALID_HOOK_EVENTS) actually considers the named event unknown. This let a stale/lagging CLI warning (e.g. for PreCompact) surface as a toast even though the allowlist already lists the event as valid. Reuse VALID_HOOK_EVENTS as the single source of truth: when the extracted hook name from an 'Unknown hook event: X' stderr line is in the allowlist, suppress the diagnostic instead of forwarding it. Genuinely unknown events still pass through unchanged (test 9c regression + new 9g coverage). Also fixes a stale test fixture: the 9a seed case used PostToolUse (a real, allowlisted event) as an arbitrary example of an 'unknown' hook name, which was never actually testing an unknown-hook condition. Swapped for a genuinely unknown event name.
… box (lr-e901) Two independent emitters (CLI-stderr capture + deterministic settings-preflight runPreflight()) could produce a diagnostic for the same condition. addDiagnostic() had no dedup -- every entry was pushed and toasted unconditionally, so the same warning showed twice. Add a bounded-window dedup keyed on (source, scope, message) before recording a new entry. scope is part of the identity because runPreflight() intentionally emits one diagnostic per settings file (user vs project, lr-7e22) -- those are legitimately distinct and must not collapse. Separately, .toast-diagnostic was position:fixed center-bottom, directly over the composer/input box, with no stacking offset -- concurrent toasts piled in the same spot. Re-anchor to a fixed top-right corner stack (#diagnostic-toast-container), following the existing corner- anchored, flex-column-stacked precedent already used by .notif-banner-container (notifications-center.css) rather than introducing a third layout pattern. Normal flex flow + gap handles stacking without per-toast JS offset math. Design note: the base .toast family (base.css) is itself center-bottom, not corner-anchored -- it was NOT used as the anchor precedent here since that is the exact placement defect being fixed. The corner-stack pattern instead follows .notif-banner-container, which already solves the 'multiple concurrent notifications' case in this codebase. Mobile keeps the lr-2fdd top-anchor fix intact, now applied to the container and widened to full-width for small viewports.
![clagentic-reviewer[bot]](https://avatars.githubusercontent.com/in/3944489?s=60&v=4){kind=small}
There was a problem hiding this comment.
PEACHES — clean (0 findings)
All three lr-e901 defects are correctly fixed:
-
False "unknown hook" (defect 1): lib/yoke/adapters/diagnostic-patterns.js lines 106–117 reconcile CLI stderr "Unknown hook event" lines against VALID_HOOK_EVENTS (settings-preflight.js), suppressing false positives when the CLI's installed version lags behind the allowlist. Test 9g verifies all valid events suppress, and genuinely unknown events still pass.
-
Doubling (defect 2): lib/public/modules/diagnostics.js lines 29–53 add dedup keyed on (source, scope, message) within a 5s window. Lines 114–119 guard addDiagnostic() to reject duplicates before pushing/toasting. Regression tests verify the guard, the three-part key (respecting lr-7e22 scope distinction), and bounded window.
-
Placement/stacking (defect 3): diagnostics.css lines 168–177 introduce #diagnostic-toast-container (fixed top-right, flex column, gap-stacked). Lines 185–206 move .toast-diagnostic from position:fixed center-bottom to position:relative, with translateY animation handled by the parent container. diagnostics.js lines 156–167 add _getToastContainer() helper; line 208 appends to it, not document.body. Mobile media query (lines 290–301) preserves lr-2fdd top-anchor on the container. Regression tests verify container structure, individual toast positioning, and mobile fix integrity.
Design decision (corner-anchor vs center-bottom): Task brief explicitly allows re-anchoring off the input box for stacking room; this is justified per lr-2fdd mobile precedent, not a deviation.
Code craft: No extraneous changes; amos.code-craft.1/4/8/10 met. All lr-e901 references tagged consistently. No bare "clagentic" in user-facing strings. No new SDK requires. No hardcoded paths. No cross-layer imports. Tests: 9 new (+ 619 baseline pass).
Verified: git HEAD is 7e356fb.
![clagentic-security[bot]](https://avatars.githubusercontent.com/in/4134208?s=60&v=4){kind=small}
There was a problem hiding this comment.
BOBBIE — clean
Scope: lib/yoke/adapters/diagnostic-patterns.js, lib/public/modules/diagnostics.js, lib/public/css/diagnostics.css, test/diagnostic-patterns.test.js, test/diagnostics-toast-dedup-placement-lr-e901.test.js (PR #302, lr-e901).
XSS check (lr-cc85 follow-up, per task context): confirmed no server-controlled string reaches innerHTML in this diff. lib/public/modules/diagnostics.js:187-206 (_showDiagnosticToast) and :262-279 (_appendEntry) assign entry.severity, entry.source, entry.scope, entry.message, entry.actionable.label exclusively via .textContent. The only two innerHTML assignments in the file (diagnostics.js:180 dismissBtn.innerHTML = iconHtml("x"); diagnostics.js:274 fixIcon.innerHTML = iconHtml("info")) are untouched by this diff and pass hardcoded literal icon names into lib/public/modules/icons.js:25 iconHtml(), which only ever receives fixed strings ("x", "info") at both call sites — no diagnostic-message data flows into iconHtml. No injection introduced.
lib/yoke/adapters/diagnostic-patterns.js:100-112 — new hook-event reconciliation reads a fixed internal allowlist (VALID_HOOK_EVENTS from lib/settings-preflight.js) and an anchored, non-backtracking regex (UNKNOWN_HOOK_EVENT_RE) against CLI stderr text; no exec/eval/template/deserialization sink touched.
diagnostics.js dedup (lines 29-52) uses an in-memory plain-object cache keyed on trusted-shape strings (source|scope|message) with a bounded 5s window; no unbounded growth concern beyond existing session lifetime, no secret material.
diagnostics.css changes are layout-only (position/z-index/flex), no security surface.
No package.json/package-lock.json changes in this diff — osv-scanner findings against the existing lockfile (46 known vulns, pre-existing) are out of scope for this PR (not introduced here).
gitleaks flagged private-key fixtures under .claude/worktrees/*/lib/certs/privkey.pem — confirmed untracked by git (no history for that path) and unrelated to this PR's diff; not a finding against PR #302.
early_exit_reason: not applicable — full reasoning pass completed (touched sink code, not just fixtures/renames).
scanners_run:
- gitleaks detect: ran, 0 findings in PR diff scope (6 findings in unrelated untracked worktree fixtures, out of scope)
- semgrep --config auto: ran against touched JS files, 0 findings
- osv-scanner: ran against package-lock.json, 46 pre-existing vulns, none introduced by this diff (no lockfile change in PR)
- trufflehog: not run (gitleaks covered secret-scan scope; no findings warranting a second pass)
{"reviewer": "bobbie", "review_status": "clean", "head_sha": "7e356fb160a3d9d39a8df5146c1ed66af5e420a9", "pr_number": 302}
clagentic gate-note — authorized
Authorize rationale: PEACHES clean, BOBBIE clean, tests 619/619+9 new, mergeable_state=clean per envelope lr-e901 |
Fixes all three confirmed defects in the PREFLIGHT WARNING diagnostic toast (lr-e901).
Defect 1 -- false unknown-hook toast
Root cause: the CLI-stderr diagnostic path (lib/yoke/adapters/diagnostic-patterns.js) matched "Unknown hook event" stderr lines generically and forwarded the CLI's own message text WITHOUT ever consulting VALID_HOOK_EVENTS (lib/settings-preflight.js). A hook event the allowlist already considers valid (e.g. PreCompact, added under lr-7e22) could still surface a false toast if the installed CLI version lags the allowlist.
Fix: diagnostic-patterns.js extracts the hook event name from the CLI's "Unknown hook event: X" line and checks it against VALID_HOOK_EVENTS before forwarding. If X is in the allowlist, the diagnostic is suppressed (stale/false CLI warning). Genuinely unknown events still pass through unchanged. VALID_HOOK_EVENTS is NOT widened -- reused, not duplicated, at the new call site.
Design tradeoff named per the task brief: this means the Console will now sometimes silently swallow a CLI-emitted hook warning that the allowlist considers valid, even if that CLI version genuinely does not support the hook yet (installed-CLI-version gap, not just doc lag). VALID_HOOK_EVENTS is verified against current hooks documentation and is the codebase's single source of truth, so treating it as authoritative over ad hoc CLI stderr text was judged the correct default. A CLI-version-aware check, if ever needed, belongs in settings-preflight.js, not a per-line hack in the stderr path.
Regression coverage: new test 9g in test/diagnostic-patterns.test.js covers both directions (every VALID_HOOK_EVENTS entry suppressed, a genuinely unknown event still surfaced). The existing 9a seed case used PostToolUse as an arbitrary 'unknown hook' example -- since PostToolUse is a real allowlisted event, that assertion was testing the wrong thing; swapped for a genuinely-unknown event name.
Defect 2 -- doubling (no dedup)
Root cause: addDiagnostic() in lib/public/modules/diagnostics.js pushed and toasted every incoming diagnostic unconditionally, so the same condition arriving via two independent emitters (CLI-stderr capture + deterministic settings-preflight runPreflight()) produced two toasts.
Fix: added a bounded-window (5s) dedup keyed on (source, scope, message) before recording a new entry. scope is part of the identity because runPreflight() intentionally emits one diagnostic per settings file (user vs project, lr-7e22) -- those are legitimately distinct conditions and must not be collapsed together.
Defect 3 -- placement/stacking
Root cause: .toast-diagnostic was position:fixed; bottom:80px; left:50% -- center-bottom, directly over the composer/input box, with no stacking offset for concurrent toasts.
Fix: re-anchored to a fixed top-right corner stack (#diagnostic-toast-container, position:fixed; top:48px; right:8px; display:flex; flex-direction:column; gap:8px). Toasts are appended into this shared container instead of document.body, so flex flow + gap handles stacking with no per-toast JS offset math.
Note on the task brief's suggested approach: the brief said to corner-anchor 'consistent with the base toast family'. The base .toast family (lib/public/css/base.css) is itself center-bottom, NOT corner-anchored -- it could not be the corner-anchor precedent, and is in fact the same placement pattern causing the input-box occlusion. Instead this follows the existing corner-anchored, flex-column-stacked pattern already used by .notif-banner-container (notifications-center.css), which already solves 'multiple concurrent notifications, stacked, off content' in this codebase. Named here per the task's own instruction to surface design tradeoffs.
Mobile keeps the lr-2fdd top-anchor behavior intact (now applied to the container), widened to full-width for small viewports.
Tests
npm test: 619/619 passing (610 before this change; +9 new regression tests: 2 in diagnostic-patterns.test.js for defect 1, 7 in a new test/diagnostics-toast-dedup-placement-lr-e901.test.js for defects 2 and 3 -- source-text regression checks matching this project's existing convention for DOM-dependent ESM modules, per diagnostics-panel-pointer-events-lr-b580.test.js).
Task: lr-e901