From 328db604b262f4f5b5925b7b2b35a07f53186a41 Mon Sep 17 00:00:00 2001 From: Abdul Wahab Date: Thu, 6 Aug 2026 15:43:21 +0500 Subject: [PATCH] Harden the constrained-budget clamp: keep findings, honest flags, guaranteed note Review follow-ups from Bugbot and Greptile on the clamp fix: - The clamp retries with the short policy before dropping finding lines that fit beside it, and a grouped result that kept only a file header no longer counts as a fit. - The full-footer session flag commits only when the full policy actually survived the clamp, so a downgraded emission does not mark the session as having seen a policy it never received. - Render paths reserve room for a pending DESIGN.md staleness note, so it is delivered inside the budget on the first emission instead of deferring behind full emissions indefinitely. AI-assisted (Cursor agent), directed and reviewed by @abdulwahabone. Co-authored-by: Cursor --- skill/scripts/hook-before-edit.mjs | 9 +- skill/scripts/hook-lib.mjs | 153 ++++++++++++++++++++--------- tests/hook.test.mjs | 64 ++++++++---- 3 files changed, 157 insertions(+), 69 deletions(-) diff --git a/skill/scripts/hook-before-edit.mjs b/skill/scripts/hook-before-edit.mjs index af003105a..cfe7f6ba8 100644 --- a/skill/scripts/hook-before-edit.mjs +++ b/skill/scripts/hook-before-edit.mjs @@ -20,6 +20,8 @@ import { GENERATED_PATH, SENSITIVE_PATH, appendDesignSystemNoteOnce, + commitFooterShown, + designNoteReserve, designSystemOptions, footerModeForSession, filterFindings, @@ -346,8 +348,8 @@ async function detectProposedHtml(detector, content, filePath, scanOptions) { } } -function cursorBlockMessage(findings, filePath, config, cwd, footerMode) { - const rendered = renderTemplate(findings, filePath, config, { cwd, footer: footerMode }); +function cursorBlockMessage(findings, filePath, config, cwd, footerMode, reserveChars) { + const rendered = renderTemplate(findings, filePath, config, { cwd, footer: footerMode, reserveChars }); const blocked = rendered.replace( '[impeccable@1] Design hook findings requiring review', '[impeccable@1] Impeccable design hook blocked this write before it landed. Design hook findings requiring review', @@ -475,9 +477,10 @@ async function main() { // policy: the full footer emits once per session, the short form after. const footerMode = footerModeForSession(cache, sessionId); const message = appendDesignSystemNoteOnce( - cursorBlockMessage(filtered, filePath, config, cwd, footerMode), + cursorBlockMessage(filtered, filePath, config, cwd, footerMode, designNoteReserve(scanOptions, cache, sessionId)), scanOptions, cache, sessionId, config, ); + commitFooterShown(cache, sessionId, message); const denial = bumpCursorDenial(cache, sessionId, filePath, filtered); persistCache(cwd, cache); if (denial.count > EDIT_COUNT_THRESHOLD) { diff --git a/skill/scripts/hook-lib.mjs b/skill/scripts/hook-lib.mjs index 4372e4eac..260854a44 100644 --- a/skill/scripts/hook-lib.mjs +++ b/skill/scripts/hook-lib.mjs @@ -23,7 +23,8 @@ * renderTemplate(findings, filePath, config, opts) * renderCleanAck(filePath, opts) / renderPendingAck(filePath, known, opts) * appendDesignSystemNote(text, scanOptions) / appendDesignSystemNoteOnce(text, scanOptions, cache, sessionId, config) - * footerModeForSession(cache, sessionId) + * designNoteReserve(scanOptions, cache, sessionId) + * footerModeForSession(cache, sessionId) / commitFooterShown(cache, sessionId, text) * shouldEmitAckForFile(filePath, config?) * writeAuditLog(env, entry) * loadDetector() -> Promise<{ detectText, detectHtml }> @@ -972,7 +973,10 @@ export function renderTemplate(findings, filePath, config, opts = {}) { if (!Array.isArray(findings) || findings.length === 0) return ''; const limits = config?.limits || DEFAULT_CONFIG.limits; const cap = Math.max(1, limits.maxFindings || DEFAULT_CONFIG.limits.maxFindings); - const maxChars = Math.max(500, limits.maxChars || DEFAULT_CONFIG.limits.maxChars); + // reserveChars holds back room for a note the caller appends after render + // (the DESIGN.md staleness note), so the final payload stays inside the + // configured budget. + const maxChars = Math.max(500, limits.maxChars || DEFAULT_CONFIG.limits.maxChars) - (opts.reserveChars || 0); const cwd = opts.cwd || process.cwd(); const display = relativize(filePath, cwd); @@ -1010,7 +1014,7 @@ function renderGroupedTemplate(groups, config, opts = {}) { const limits = config?.limits || DEFAULT_CONFIG.limits; const cap = Math.max(1, limits.maxFindings || DEFAULT_CONFIG.limits.maxFindings); - const maxChars = Math.max(500, limits.maxChars || DEFAULT_CONFIG.limits.maxChars); + const maxChars = Math.max(500, limits.maxChars || DEFAULT_CONFIG.limits.maxChars) - (opts.reserveChars || 0); const cwd = opts.cwd || process.cwd(); const total = realGroups.reduce((sum, group) => sum + group.findings.length, 0); const header = `${ENVELOPE_PREFIX} Design hook findings requiring review across ${realGroups.length} files (${total} issue(s)):`; @@ -1043,6 +1047,19 @@ function renderGroupedTemplate(groups, config, opts = {}) { return text; } +// The clamp contract, shared by both budget functions: the footer is policy, +// not detail, so it survives every clamp. Try the requested footer first; +// when it cannot fit even after dropping finding lines, retry with the short +// policy rather than sacrifice findings that fit beside it. A result that +// dropped every finding line (a grouped render can fit a bare file header) +// does not count as a fit: findings are why the emission exists. +const isFindingLine = (line) => line.startsWith('- '); + +function footerFallbacks(footer) { + const short = directiveFooter({ mode: 'short' }); + return footer === short ? [footer] : [footer, short]; +} + function clampGroupedToBudget(header, lines, footer, maxChars) { const assemble = (linesArr, omitted, footerText) => [ header, @@ -1052,16 +1069,19 @@ function clampGroupedToBudget(header, lines, footer, maxChars) { footerText, ].join('\n'); - let working = lines.slice(); - let omitted = false; - let assembled = assemble(working, omitted, footer); - while (assembled.length > maxChars && working.length > 1) { - working.pop(); - omitted = true; - assembled = assemble(working, omitted, footer); + for (const footerText of footerFallbacks(footer)) { + let working = lines.slice(); + let omitted = false; + let assembled = assemble(working, omitted, footerText); + while (assembled.length > maxChars && working.length > 1) { + working.pop(); + omitted = true; + assembled = assemble(working, omitted, footerText); + } + if (assembled.length <= maxChars && working.some(isFindingLine)) return assembled; } - if (assembled.length <= maxChars) return assembled; - return clampLastLine((linesArr, footerText) => assemble(linesArr, true, footerText), working[0], footer, maxChars); + return clampLastLine((linesArr, footerText) => assemble(linesArr, true, footerText), + lines.find(isFindingLine) || lines[0], maxChars); } function clampToBudget(header, lines, more, footer, maxChars) { @@ -1073,39 +1093,37 @@ function clampToBudget(header, lines, more, footer, maxChars) { return blocks.join('\n'); }; - let working = lines.slice(); - let moreText = more; - let assembled = assemble(working, moreText, footer); - while (assembled.length > maxChars && working.length > 1) { - working.pop(); - moreText = `... and more (see ${IMPECCABLE_COMMAND} audit).`; - assembled = assemble(working, moreText, footer); + let lastMore = more; + for (const footerText of footerFallbacks(footer)) { + let working = lines.slice(); + let moreText = more; + let assembled = assemble(working, moreText, footerText); + while (assembled.length > maxChars && working.length > 1) { + working.pop(); + moreText = `... and more (see ${IMPECCABLE_COMMAND} audit).`; + assembled = assemble(working, moreText, footerText); + } + lastMore = moreText; + if (assembled.length <= maxChars) return assembled; } - if (assembled.length <= maxChars) return assembled; - return clampLastLine((linesArr, footerText) => assemble(linesArr, moreText, footerText), working[0], footer, maxChars); + return clampLastLine((linesArr, footerText) => assemble(linesArr, lastMore, footerText), + lines.find(isFindingLine) || lines[0], maxChars); } -// Last resort when a single finding line still busts the budget. The footer -// is policy, not detail, so it survives every clamp: give it the budget -// first, clip the finding line to what remains, and when the full policy is -// itself what does not fit, downgrade to the short form rather than emit -// findings with no policy at all. The old tail-slice cut whatever happened -// to be last, which was always the footer. -function clampLastLine(build, line, footer, maxChars) { - const footerCandidates = footer === directiveFooter({ mode: 'short' }) - ? [footer] - : [footer, directiveFooter({ mode: 'short' })]; - for (const footerText of footerCandidates) { - // +1 for the newline the line itself brings when it joins the blocks. - const room = maxChars - build([], footerText).length - 1; - if (room >= 24) { - const clipped = line.length > room ? `${line.slice(0, room - 1)}…` : line; - return build([clipped], footerText); - } +// Last resort with one finding line left: the short policy gets the budget +// first, the line is clipped to what remains. The pre-fix tail-slice cut +// whatever happened to be last, which was always the footer. +function clampLastLine(build, line, maxChars) { + const footerText = directiveFooter({ mode: 'short' }); + // +1 for the newline the line itself brings when it joins the blocks. + const room = maxChars - build([], footerText).length - 1; + if (room >= 24) { + const clipped = line.length > room ? `${line.slice(0, room - 1)}…` : line; + return build([clipped], footerText); } // maxChars is floored at 500 and header + short footer fit well inside // that, so this is unreachable; keep the hard slice as the safety net. - return `${build([line], footer).slice(0, maxChars - 1)}…`; + return `${build([line], footerText).slice(0, maxChars - 1)}…`; } // `compact` drops the registry description: within one emission the first @@ -1657,9 +1675,10 @@ function consumeSessionNoticeFlag(cache, sessionId, flag) { // Once-per-session variant of appendDesignSystemNote for the emission paths // that have cache access. The staleness note names standing project state, // not new information, so one mention per session is enough. The note is -// appended after the renderer has clamped to the configured budget, so when -// it would push the emission past maxChars, defer it (without consuming the -// flag) to a later, smaller emission in the session. +// appended after the renderer has clamped to the configured budget: render +// paths reserve room for it via designNoteReserve, and the size check here +// is the safety net for the ack paths, deferring (without consuming the +// flag) to a later emission rather than busting maxChars. export function appendDesignSystemNoteOnce(text, scanOptions, cache, sessionId, config) { if (!text || !scanOptions?.designSystem?.mdNewerThanJson) return text; const maxChars = Math.max(500, config?.limits?.maxChars || DEFAULT_CONFIG.limits.maxChars); @@ -1668,15 +1687,41 @@ export function appendDesignSystemNoteOnce(text, scanOptions, cache, sessionId, return appendDesignSystemNote(text, scanOptions); } +// Render-time reservation for the note above: how many characters the +// renderer must hold back so a pending staleness note still fits inside the +// configured budget. Zero once the session has seen the note. Without the +// reservation, a session whose every emission fills the budget would defer +// the note forever. +export function designNoteReserve(scanOptions, cache, sessionId) { + if (!scanOptions?.designSystem?.mdNewerThanJson) return 0; + if (ensureSession(cache, sessionId).designNoteShown) return 0; + return DESIGN_STALE_NOTE.length + 2; +} + // Full directive footer once per session, the short reminder after. Fresh // emissions and Cursor denials share the session flag (`footerShown`), so a -// session pays the full policy exactly once however it first fires. +// session pays the full policy exactly once however it first fires. The mode +// is a peek: the clamp can downgrade a requested full footer under a tight +// budget, so the flag commits only when the full policy actually reached the +// output (commitFooterShown, matched on the footer's opening words). export function footerModeForSession(cache, sessionId) { - return consumeSessionNoticeFlag(cache, sessionId, 'footerShown') ? 'full' : 'short'; + return ensureSession(cache, sessionId).footerShown ? 'short' : 'full'; +} + +export function commitFooterShown(cache, sessionId, text) { + if (!text || !text.includes(FULL_FOOTER_SENTINEL)) return; + const session = ensureSession(cache, sessionId); + if (session.footerShown) return; + session.footerShown = true; + session.updatedAt = Date.now(); } const HOOK_ADMIN_COMMAND = `node ${quoteCommandArg(path.join(__dirname, 'hook-admin.mjs'))}`; +// Opening words of the full footer; commitFooterShown matches on it to tell +// whether the full policy survived the clamp. +const FULL_FOOTER_SENTINEL = 'Triage each finding'; + // The directive footer is the part of the hook output that steers model // behavior. Intentional moves, in order: // 1. **Imperative, not advisory.** "Triage each finding..." beats @@ -1704,7 +1749,7 @@ function directiveFooter(opts = {}) { return 'Triage per the session policy: fix real problems; persist confident false-positive or sanctioned-exception ignores via `hook-admin.mjs ignore-value` and disclose them in your reply; unsure, ask in one line.'; } return [ - 'Triage each finding, then state in your reply what you fixed, what you suppressed, and what you left standing:', + `${FULL_FOOTER_SENTINEL}, then state in your reply what you fixed, what you suppressed, and what you left standing:`, '- Real design problem: fix it. Keep intentional design as designed.', `- Confident false positive or sanctioned exception (an intentional demo or fixture, documentation of bad design, literal or domain-appropriate motion, a choice the user confirmed): persist the narrowest ignore yourself and disclose it. Run \`${HOOK_ADMIN_COMMAND} ignore-value "" --reason ""\` with the pair shown on the finding line, or value "*" plus \`--file \` when the line shows none. Write "user confirmed" in a reason only when the user did.`, '- Unsure: leave it as is and ask the user in one line.', @@ -1930,15 +1975,20 @@ export async function runHook({ stdinJson, env = {}, cwd = process.cwd(), now = } } - // Consuming a session notice flag mutates the cache, so both must happen - // before the persist that makes the flag stick across events. + // The session notice flags mutate the cache, so they must settle before + // the persist that makes them stick across events. if (freshGroups.length > 0) { const firstGroup = freshGroups[0]; const footerMode = footerModeForSession(cache, sessionId); const text = appendDesignSystemNoteOnce( - renderGroupedTemplate(freshGroups, config, { cwd: projectCwd, footer: footerMode }), + renderGroupedTemplate(freshGroups, config, { + cwd: projectCwd, + footer: footerMode, + reserveChars: designNoteReserve(scanOptions, cache, sessionId), + }), scanOptions, cache, sessionId, config, ); + commitFooterShown(cache, sessionId, text); // Fresh findings always earn the cache write, including creating // `.impeccable/`: dedup, suppression, and the notice flags need it. persistCache(projectCwd, cache); @@ -2222,9 +2272,14 @@ export async function runStopHook({ stdinJson, env = {}, cwd = process.cwd(), no // flag, so the Stop wall of text carries the one-line short footer. const footerMode = footerModeForSession(cache, sessionId); const text = appendDesignSystemNoteOnce( - renderGroupedTemplate(freshGroups, config, { cwd: projectCwd, footer: footerMode }), + renderGroupedTemplate(freshGroups, config, { + cwd: projectCwd, + footer: footerMode, + reserveChars: designNoteReserve(scanOptions, cache, sessionId), + }), scanOptions, cache, sessionId, config, ); + commitFooterShown(cache, sessionId, text); // Fresh findings earn the cache write so the next Stop fire is silent // unless new issues appear; the notice flags ride along. diff --git a/tests/hook.test.mjs b/tests/hook.test.mjs index 48c9096c0..02aa454c7 100644 --- a/tests/hook.test.mjs +++ b/tests/hook.test.mjs @@ -1213,6 +1213,18 @@ describe('renderTemplate()', () => { assert.match(text, /Triage per the session policy/, 'a clamped emission still carries the policy'); }); + it('keeps findings that fit beside the short policy instead of dropping them for the full one', () => { + const findings = [1, 2, 3].map((line) => + finding('side-tab', line, { name: 'X', description: 'short issue' })); + const text = renderTemplate(findings, '/x/a.tsx', + { ...DEFAULT_CONFIG, limits: { maxFindings: 5, maxChars: 500 } }, + { cwd: '/x' }); + assert.ok(text.length <= 500); + assert.match(text, /- L1 /); + assert.match(text, /- L2 /); + assert.match(text, /- L3 /, 'all findings survive; the clamp must not drop lines chasing the full policy'); + assert.match(text, /Triage per the session policy/); + }); }); describe('writeAuditLog()', () => { @@ -2251,30 +2263,47 @@ describe('runHook() — session-scoped notices', () => { assert.doesNotMatch(r2.stdout, /DESIGN\.md is newer/, 'the staleness note does not repeat within a session'); }); - it('defers the staleness note past an emission it would push over budget', async () => { + it('delivers the staleness note inside the budget on a full first emission', async () => { + fs.writeFileSync(getConfigPath(cwd), JSON.stringify({ + hook: { limits: { maxChars: 500 } }, + })); + const a = path.join(cwd, 'a.css'); + const b = path.join(cwd, 'b.css'); + fs.writeFileSync(a, 'noop'); + fs.writeFileSync(b, 'noop'); + const det = { + ...fakeDetector([finding('tiny-text', 1, { name: 'Tiny text', description: 'y'.repeat(600) })]), + loadDesignSystemForCwd: () => ({ present: true, mdNewerThanJson: true }), + }; + + // A finding this long would fill the whole budget; the renderer must + // reserve room so the note still lands without busting maxChars. + const r1 = await runHook({ stdinJson: event(a), env: {}, cwd, detector: det }); + const ctx1 = JSON.parse(r1.stdout).hookSpecificOutput.additionalContext; + assert.ok(ctx1.length <= 500, `final emission honors maxChars (got ${ctx1.length})`); + assert.match(ctx1, /DESIGN\.md is newer/, 'the note is delivered on the first emission, not deferred past it'); + + const r2 = await runHook({ stdinJson: event(b), env: {}, cwd, detector: det }); + const ctx2 = JSON.parse(r2.stdout).hookSpecificOutput.additionalContext; + assert.doesNotMatch(ctx2, /DESIGN\.md is newer/, 'one mention per session'); + }); + + it('keeps the full-footer flag unspent when the clamp downgrades the footer', async () => { fs.writeFileSync(getConfigPath(cwd), JSON.stringify({ hook: { limits: { maxChars: 500 } }, })); const a = path.join(cwd, 'a.css'); fs.writeFileSync(a, 'noop'); - let current = [finding('tiny-text', 1, { name: 'Tiny text', description: 'y'.repeat(600) })]; - const det = { - detectText: () => current.slice(), - detectHtml: () => current.slice(), - loadDesignSystemForCwd: () => ({ present: true, mdNewerThanJson: true }), - }; + const det = fakeDetector([finding('tiny-text', 1, { name: 'Tiny text', description: 'y'.repeat(600) })]); - // The fresh emission fills the whole budget; appending the note here - // would bust maxChars, so it must wait without burning the session flag. + // 500 chars cannot hold the full policy, so the emission carries the + // short form. The session must not be marked as having seen the full + // footer it never received. const r1 = await runHook({ stdinJson: event(a), env: {}, cwd, detector: det }); - const ctx1 = JSON.parse(r1.stdout).hookSpecificOutput.additionalContext; - assert.ok(ctx1.length <= 500, `final emission honors maxChars (got ${ctx1.length})`); - assert.doesNotMatch(ctx1, /DESIGN\.md is newer/); - - // The next emission is a small clean ack with room to spare. - current = []; - const r2 = await runHook({ stdinJson: event(a), env: {}, cwd, detector: det }); - assert.match(r2.stdout, /DESIGN\.md is newer/, 'the deferred note lands on the next emission with room'); + assert.match(r1.stdout, /Triage per the session policy/); + assert.doesNotMatch(r1.stdout, /Triage each finding/); + const cache = readCache(cwd); + assert.ok(!cache.sessions['sid-1'].footerShown, 'a downgraded footer does not spend the session flag'); }); }); @@ -3648,6 +3677,7 @@ describe('runStopHook()', () => { const ctx = JSON.parse(stop.stdout).hookSpecificOutput.additionalContext; assert.ok(ctx.length <= 500, `grouped emission honors maxChars (got ${ctx.length})`); assert.match(ctx, /Triage per the session policy/, 'a clamped grouped emission still carries the policy'); + assert.match(ctx, /\[side-tab\]/, 'a clamped grouped emission keeps finding detail, not just a file header'); }); it('exits silent and fast when the session touched no UI files', async () => {