fix(preflight+diagnostics): stop bogus hook-event warnings and distinguish preflight source labels (lr-7e22)#300
Conversation
…EVENTS (lr-7e22) PreCompact, PostCompact, SessionStart, UserPromptSubmit, PermissionRequest, TaskCreated, TeammateIdle, TaskCompleted are real, current hook event names (verified against code.claude.com/docs/en/hooks, cross-checked via web search) that the stale allowlist did not recognize, producing false-positive "unknown hook event" warnings for operators using them -- notably PreCompact, the operator's screenshotted complaint.
… fix wrong assertion
Adds coverage for the 8 newly-valid hook events and for scope tagging
(validateSettingsObject/validateSettingsFile/runPreflight now accept an
optional scope param distinguishing 'user' vs 'project' settings files).
Also fixes a pre-existing failing assertion ("PreToolUse must not appear")
that was checking the wrong thing: diagnostic messages legitimately echo the
full "Valid events: ..." list for operator guidance, so a valid event name
like PreToolUse correctly appears as a substring there. The assertion now
checks that PreToolUse is never named as the *offending* unknown key, which
is what the test's own comment says it intends to verify.
… scope (lr-7e22)
runPreflight validates both ~/.claude/settings.json and the project's
.claude/settings.json independently, emitting one legitimate diagnostic per
unknown hook key per file. formatDiagnosticSource('preflight') rendered an
identical "Preflight" label for all of them, so multiple correct warnings
visually stacked as apparent duplicates (lr-5db5 investigation finding).
settings-preflight.js now attaches scope:'user'|'project' to each
diagnostic. formatDiagnosticSource(source, scope) appends a terse suffix
(e.g. "Preflight · user" vs "Preflight · project") when scope is present and
recognized; diagnostics.js threads entry.scope through to both the toast
header and panel row render sites. Preserves lr-2fdd mobile anchoring and
lr-b580 panel pointer/dismiss/ARIA behavior -- neither is touched.
![clagentic-reviewer[bot]](https://avatars.githubusercontent.com/in/3944489?s=60&v=4){kind=small}
There was a problem hiding this comment.
PEACHES — clean
Five checkpoints: all passed.
Checkpoint (a) — VALID_HOOK_EVENTS sync: The 8 added events (PreCompact, PostCompact, SessionStart, UserPromptSubmit, PermissionRequest, TaskCreated, TeammateIdle, TaskCompleted) are cited against code.claude.com/docs/en/hooks as of 2026-07-01 in the source comment. No valid warnings suppressed; this is a pure allowlist expansion for observed-in-field events.
Checkpoint (b) — Scope threading: Scope parameter flows cleanly from settings-preflight.js through validateSettingsObject/validateSettingsFile/runPreflight (attaching 'user' or 'project' to each diagnostic at source), then diagnostic-format.js (SCOPE_LABELS), then diagnostics.js (both toast and panel render sites pass entry.scope). Backward-compatible; no god-function.
Checkpoint (c) — Test assertion fix: Old assertion checked if substring "PreToolUse" appeared anywhere (always true because messages echo the full valid event list). New assertion correctly checks for the pattern "unknown hook event 'PreToolUse'" (which only appears if it's actually flagged as unknown). Regression tests added for scope attachment. Proper fix, not green-washing.
Checkpoint (d) — Brand rules: No bare "clagentic" found in user-facing strings; all match clagentic-console.brand-product-name rule (peaches.yaml).
Checkpoint (e) — lr-2fdd and lr-b580 untouched: lr-b580 (panel pointer trap, click-outside dismiss) already committed in branch HEAD (6054d45). This PR inherits it. lr-2fdd (toast mobile anchoring) untouched.
No blocking findings.
![clagentic-security[bot]](https://avatars.githubusercontent.com/in/4134208?s=60&v=4){kind=small}
There was a problem hiding this comment.
BOBBIE — clean
Security audit of PR #300 (fix/lr-7e22-preflight-hook-events-and-source-label, HEAD b363384) against RULEBOOK.md and .crew/bobbie.yaml.
Scope reviewed: lib/settings-preflight.js, lib/public/modules/diagnostic-format.js, lib/public/modules/diagnostics.js, test/*.
Findings: none blocking.
Checks performed:
- DOM injection surface (a): scope values are only ever looked up against a fixed allowlist object (SCOPE_LABELS = {user, project}) in diagnostic-format.js:21-24,38 — any unrecognized/attacker-influenced scope string is dropped, never concatenated into rendered text. All new/changed render sites (diagnostics.js:136, 149, 237, 244) use .textContent exclusively, never .innerHTML, for source labels and message text (including the offending hook-key name and file path already carried in entry.message pre-PR). No new DOM injection surface introduced.
- Path handling (b): path.join(userHome, ".claude", "settings.json") and path.join(projectDir, ".claude", "settings.json") (settings-preflight.js:224,233) are pre-existing lines from commit 94a8e8e (feat lr-1a26), unchanged by this PR — only the trailing scope-tag argument was added to the validateSettingsFile() call. No new traversal/symlink surface introduced by this diff.
- Secrets (c): diagnostics carry only hook-key names, file paths, and JSON-shape error text — no settings.json values that could be credentials are surfaced, in this PR or pre-existing code.
Scanner results (evaluated against Pre-Report Gate, all attributed to pre-existing code outside this diff, not introduced by PR #300):
- gitleaks: 6 hits, all in .claude/worktrees/*/lib/certs/privkey.pem (unrelated worktree cert artifacts, not part of this diff) — dropped.
- semgrep --config=auto: 2 path-traversal WARNING hits on settings-preflight.js:224,233 — both are the pre-existing path.join() call sites (introduced in commit 94a8e8e, not this PR); this PR only adds a scope-tag argument to the neighboring call — dropped as not introduced by this PR.
- osv-scanner: 46 known vulnerabilities across 17 packages in package-lock.json (nodemailer, handlebars, ws, undici, etc.) — package-lock.json is not touched by this PR — out of scope, dropped.
scanners_run: gitleaks (ok), semgrep --config=auto (ok), osv-scanner (ok)
review-result
{"reviewer": "bobbie", "review_status": "clean", "head_sha": "b363384299c9f508c122cd29161ba836d23b60a4", "pr_number": 300}
clagentic gate-note — authorized
Authorize rationale: PEACHES clean (pullrequestreview-4613545196), BOBBIE clean (pullrequestreview-4613552663), tests 608/608. lr-7e22. |
clagentic-merger
Bot
deleted the
fix/lr-7e22-preflight-hook-events-and-source-label
branch
What changed
Two coupled defects producing the stacked-warning UX (split from lr-5db5, investigated/no-defect):
VALID_HOOK_EVENTS was stale. Added 8 real, current Claude Code hook events verified against the live docs (code.claude.com/docs/en/hooks, hook lifecycle table), cross-checked via web search:
These 8 are not exhaustive (live docs list about 30 total hook events as of this sync) -- only the ones actually observed in the operators claude settings.json that needed verification. Events not independently verified were left out rather than guessed at, per the tasks own escape clause.
runPreflight validates both ~/.claude/settings.json and the project .claude/settings.json independently, emitting one legitimate diagnostic per unknown hook key per file. formatDiagnosticSource returned an identical Preflight label for every one, so multiple correct, distinct warnings visually stacked as apparent duplicates (root cause identified in lr-5db5).
settings-preflight.js now attaches a scope field (user or project) to each diagnostic based on which settings file produced it. formatDiagnosticSource(source, scope) appends a terse suffix when scope is present and recognized, e.g. Preflight - user vs Preflight - project, keeping the label a small mono string per the existing CSS. diagnostics.js threads entry.scope through to both the toast header and panel row render sites so both surfaces agree.
Why
Operator-reported, real UX bug (lr-7e22, split from lr-5db5 investigation). The false-positive warning was misleading (told the operator their config was wrong when it was not), and the identical labels made genuinely distinct warnings look like a rendering bug.
Test status
npm test: 608/608 pass (0 failures). The tasks noted pre-existing known failure (validateSettingsObject: multiple unknown hook keys produce one warning each, in test/settings-preflight-lr-1a26.test.js) is fixed correctly, not silenced.
The assertion PreToolUse must not appear was checking the wrong thing: diagnostic messages legitimately echo the full Valid events list for operator guidance, so a valid event name like PreToolUse correctly appears there as a substring. The assertion now checks that PreToolUse is never named as the offending unknown key, which is what the tests own inline comment says it intends to verify. This was a pre-existing test-implementation mismatch unrelated to the VALID_HOOK_EVENTS additions -- confirmed by reproducing the same failure on baseline main before any code changes in this PR.
15 new tests added: 3 for the new hook events (allowlist membership plus a PreCompact regression case), 2 for scope propagation in validateSettingsObject, 1 for validateSettingsFile, 1 for runPreflight (asserts one user-scope and one project-scope diagnostic from two separate temp settings files), 5 for formatDiagnosticSource label behavior with scope, 3 source-text regression checks confirming both diagnostics.js render sites (toast and panel) pass entry.scope through.
Preserved behavior
Task: lr-7e22