From b5d19bd4809f40cc748f452ef85fb86ce99b5d45 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Tue, 9 Jun 2026 15:12:26 -0700 Subject: [PATCH] FN-6134: extend surface-enumeration gates to UI affordance changes Apply the surface-enumeration review gate to UI affordance add/remove work across triage, review, tests, and docs. - require triage prompts to add Surface Enumeration guidance for UI affordance add/remove tasks in standard and fast modes - tighten reviewer guidance to block single-surface UI affordance coverage and leftover shell cleanup gaps - expand engine prompt tests to cover the new UI affordance gate wording - document the shared checklist and motivating incidents in AGENTS.md and docs/testing.md Files changed: AGENTS.md | 3 +++ docs/testing.md | 6 +++++- packages/engine/src/__tests__/reviewer.test.ts | 26 ++++++++++++++++++++++++-- packages/engine/src/__tests__/triage.test.ts | 19 ++++++++++++++++++- packages/engine/src/reviewer.ts | 5 +++-- packages/engine/src/triage.ts | 24 ++++++++++++++---------- 6 files changed, 67 insertions(+), 16 deletions(-) Fusion-Task-Id: FN-6134 Fusion-Task-Lineage: 0bc733eb-d160-48cc-817d-69138290faec --- AGENTS.md | 3 +++ docs/testing.md | 6 ++++- .../engine/src/__tests__/reviewer.test.ts | 26 +++++++++++++++++-- packages/engine/src/__tests__/triage.test.ts | 19 +++++++++++++- packages/engine/src/reviewer.ts | 5 ++-- packages/engine/src/triage.ts | 24 ++++++++++------- 6 files changed, 67 insertions(+), 16 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 9ccf624a0b..da4976eb69 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -103,9 +103,12 @@ pnpm verify:workspace # deep opt-in verification (lint -> test:full -> build); - When fixing a bug, the regression test must assert the general invariant across ALL known surfaces — not only the single reported reproduction. - Surface enumeration is now an enforced bug-fix artifact: the spec must include a `## Surface Enumeration` section, planning must REVISE when that section is missing, and review must REVISE any repro-only regression test. +- The Surface Enumeration gate also applies to tasks that add or remove UI affordances (icons, buttons, chevrons, toggles, badges, menu entries, click targets), including Review Level 0 cosmetic tasks. - Enumerate the surfaces before filing or closing the fix: every provider/bridge for streaming and agent paths, both desktop and mobile breakpoints for UI behavior, empty/undefined/duplicate/populated data states, and every shared hook/component/module/helper that reuses the affected logic. +- After removing a UI affordance, explicitly check for and clean up empty button shells, orphaned click targets, now-unused wrappers, and dangling aria-labels across both desktop and mobile breakpoints. - Use the canonical checklist in `docs/testing.md` → **Surface Enumeration checklist** so planning and review enumerate the same surfaces. - Motivating incidents: streamed-response spacing was fixed three times before the invariant was fully covered (FN-5787, FN-5789, FN-5803), the usage "Show hidden" button regressed three times before broader coverage stuck (FN-5797, FN-5875, FN-5919), and the auto-merge blank-dashboard fix re-opened after desktop-only coverage missed mobile Android (FN-5751). +- Motivating incident for UI affordances: the workflow-row drop-down arrow removal took three tasks (FN-6115 → FN-6118 → FN-6123) because the affordance rendered in two components and mobile kept an empty 36×36 `btn-icon` button shell. - If a regression test only proves the exact reported case, it is incomplete; extend it until the invariant holds across all known surfaces. ### Port 4040 is Reserved diff --git a/docs/testing.md b/docs/testing.md index e64608225f..c158019d21 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -289,9 +289,13 @@ Prefer `it.each` over copy-pasted `it()` blocks. When trimming, keep: first case ### Surface Enumeration checklist -Copy this checklist into a bug-fix task's `## Surface Enumeration` section and make the regression tests prove the invariant across every checked surface. See `AGENTS.md` → **Standing Rule: Fix the Invariant, Not the Repro (FN-5893)** for the enforced planning/review contract. +Copy this checklist into a bug-fix or UI-affordance add/remove task's `## Surface Enumeration` section and make the implementation tests prove the invariant across every checked surface. This checklist applies to bug-fix tasks and UI-affordance add/remove tasks that add, remove, or restructure icons, buttons, chevrons/arrows, toggles, badges, menu entries, or click targets. See `AGENTS.md` → **Standing Rule: Fix the Invariant, Not the Repro (FN-5893)** for the enforced planning/review contract. - [ ] Providers / bridges / execution paths touched by the invariant - [ ] Desktop + mobile breakpoints / platforms that exercise the behavior - [ ] Empty / undefined / duplicate / populated data states - [ ] Shared hooks / components / modules / helpers reusing the logic +- [ ] Every component that renders the affordance (search the codebase for the icon/class/testid, not just the one the user pointed at) +- [ ] Leftover shells after removal — empty buttons, orphaned click targets, now-unused wrappers, dangling aria-labels — are explicitly checked and fixed/hidden + +Motivating incident: FN-6115/FN-6118/FN-6123 — a single workflow-row chevron required three tasks to fully remove because the affordance rendered across multiple components and one mobile surface kept an empty `btn-icon` button shell. diff --git a/packages/engine/src/__tests__/reviewer.test.ts b/packages/engine/src/__tests__/reviewer.test.ts index 54c2893ed7..13f5f1e15f 100644 --- a/packages/engine/src/__tests__/reviewer.test.ts +++ b/packages/engine/src/__tests__/reviewer.test.ts @@ -297,15 +297,37 @@ describe("FN-5928 surface-enumeration review-gate wording", () => { expect(REVIEWER_SYSTEM_PROMPT).toContain("Missing or incomplete coverage is a blocking REVISE"); expect(REVIEWER_SYSTEM_PROMPT).toContain("desktop + mobile breakpoints/platforms"); expect(REVIEWER_SYSTEM_PROMPT).toContain("shared hooks/components/modules/helpers"); + expect(REVIEWER_SYSTEM_PROMPT).toContain("bug-fix specs and UI-affordance add/remove specs"); }); it("requires code reviews to reject repro-only regression tests for bug fixes", () => { - expect(REVIEWER_SYSTEM_PROMPT).toContain("repro-only regression test"); - expect(REVIEWER_SYSTEM_PROMPT).toContain("spanning the `## Surface Enumeration` checklist"); + expect(REVIEWER_SYSTEM_PROMPT).toContain("single-surface-only test"); + expect(REVIEWER_SYSTEM_PROMPT).toContain("doesn't verify the invariant across the spec's enumerated surfaces"); + expect(REVIEWER_SYSTEM_PROMPT).toContain("Keep enforcing FN-5893 for bug fixes"); expect(REVIEWER_SYSTEM_PROMPT).toContain("FN-5787/FN-5789/FN-5803"); expect(REVIEWER_SYSTEM_PROMPT).toContain("FN-5797/FN-5875/FN-5919"); expect(REVIEWER_SYSTEM_PROMPT).toContain("FN-5751"); }); + + it("requires spec/code reviews to enforce surface enumeration for UI-affordance add/remove tasks", () => { + expect(REVIEWER_SYSTEM_PROMPT).toContain("leftover shells after removal"); + expect(REVIEWER_SYSTEM_PROMPT).toContain("For bug fixes and UI-affordance add/remove changes"); + expect(REVIEWER_SYSTEM_PROMPT).toContain("UI-affordance removals"); + expect(REVIEWER_SYSTEM_PROMPT).toContain("For UI-affordance add/remove changes, apply the same surface-enumeration strictness"); + expect(REVIEWER_SYSTEM_PROMPT).toContain("FN-6115/FN-6118/FN-6123"); + }); + + it("demonstrates the gate firing on a single-component UI-removal spec", () => { + const singleComponentRemovalSpec = + "## Mission\nRemove the workflow-row chevron from WorkflowRow.tsx only."; + + expect(singleComponentRemovalSpec).toContain("WorkflowRow.tsx only"); + expect(REVIEWER_SYSTEM_PROMPT).toContain("searches for ALL components rendering the affordance"); + expect(REVIEWER_SYSTEM_PROMPT).toContain("not just the one the user pointed at"); + expect(REVIEWER_SYSTEM_PROMPT).toContain("leftover shells after removal"); + expect(REVIEWER_SYSTEM_PROMPT).toContain("empty button shells"); + expect(REVIEWER_SYSTEM_PROMPT).toContain("Issue REVISE when coverage stops at the single reported surface"); + }); }); describe("reviewStep — context-limit retry", () => { diff --git a/packages/engine/src/__tests__/triage.test.ts b/packages/engine/src/__tests__/triage.test.ts index 907499bf06..0e9cda5049 100644 --- a/packages/engine/src/__tests__/triage.test.ts +++ b/packages/engine/src/__tests__/triage.test.ts @@ -676,6 +676,11 @@ describe("FN-5893 invariant regression wording", () => { expect(prompt).toContain("docs/testing.md"); expect(prompt).toContain("duplicate / populated data states"); expect(prompt).toContain("shared hooks/components/modules/helpers"); + expect(prompt).toContain("UI-affordance add/remove"); + expect(prompt).toContain("For bug fixes and UI-affordance add/remove tasks"); + expect(prompt).toContain( + "For bug-fix and UI-affordance add/remove tasks, paste and fill in this checklist", + ); } expect(corePromptSource).toContain("## Surface Enumeration"); @@ -692,11 +697,23 @@ describe("FN-5893 invariant regression wording", () => { "Run targeted tests for changed files, asserting the invariant across all known surfaces", ); expect(prompt).toContain( - "For bug-fix tasks, paste and fill in this checklist in the `## Surface Enumeration` section", + "For bug-fix and UI-affordance add/remove tasks, paste and fill in this checklist in the `## Surface Enumeration` section", ); } }); + it("requires Surface Enumeration for UI-affordance add/remove tasks regardless of review-level analysis", () => { + for (const prompt of [TRIAGE_SYSTEM_PROMPT, FAST_TRIAGE_SYSTEM_PROMPT]) { + expect(prompt).toContain("bug-fix tasks and UI-affordance add/remove tasks"); + expect(prompt).toContain("every component that renders the affordance"); + expect(prompt).toContain("searching the codebase for the icon/class/testid"); + expect(prompt).toContain("leftover shells after removal"); + expect(prompt).toContain("empty buttons"); + } + + expect(FAST_TRIAGE_SYSTEM_PROMPT).not.toContain("## Review Level"); + }); + it("pins the canonical docs checklist heading", () => { const docsTestingSource = readFileSync( fileURLToPath(new URL("../../../../docs/testing.md", import.meta.url)), diff --git a/packages/engine/src/reviewer.ts b/packages/engine/src/reviewer.ts index 6cdadec3f4..1aa9662fe2 100644 --- a/packages/engine/src/reviewer.ts +++ b/packages/engine/src/reviewer.ts @@ -121,7 +121,7 @@ Concrete examples: ### Test Gaps - [Missing test scenarios] -- [For bug fixes, call out any repro-only regression test that does not assert the invariant across the enumerated surfaces. Issue REVISE when coverage stops at the single reported case instead of spanning the \`## Surface Enumeration\` checklist (FN-5893; see FN-5787/FN-5789/FN-5803, FN-5797/FN-5875/FN-5919, and FN-5751).] +- [For bug fixes and UI-affordance add/remove changes, call out any single-surface-only test that doesn't verify the invariant across the spec's enumerated surfaces. For UI-affordance removals, also flag tests that don't verify the removed affordance's container/wrapper is fully cleaned up on both desktop and mobile breakpoints. Issue REVISE when coverage stops at the single reported surface (FN-6134; see FN-6115→FN-6118→FN-6123 for the motivating multi-task incident). Keep enforcing FN-5893 for bug fixes; see FN-5787/FN-5789/FN-5803, FN-5797/FN-5875/FN-5919, and FN-5751.] ### Suggestions - [Optional improvements, not blocking] @@ -146,7 +146,7 @@ Concrete examples: - **File scope accuracy:** [All affected files listed? No extras?] - **Dependency correctness:** [Dependencies exist and are appropriate?] - **Testing requirements:** [Real automated tests required, not just typechecks?] -- **Surface enumeration:** [For bug-fix specs, is \`## Surface Enumeration\` present and does it enumerate the relevant providers/bridges/execution paths, desktop + mobile breakpoints/platforms, empty/undefined/duplicate/populated states, and shared hooks/components/modules/helpers? Missing or incomplete coverage is a blocking REVISE.] +- **Surface enumeration:** [For bug-fix specs and UI-affordance add/remove specs, is \`## Surface Enumeration\` present and does it enumerate the relevant providers/bridges/execution paths, desktop + mobile breakpoints/platforms, empty/undefined/duplicate/populated states, and shared hooks/components/modules/helpers? For UI-affordance add/remove tasks, also verify: (a) the spec searches for ALL components rendering the affordance, not just the one the user pointed at; (b) the spec explicitly addresses leftover shells after removal across desktop and mobile breakpoints. Missing or incomplete coverage is a blocking REVISE.] - **Documentation completeness:** [Must Update / Check If Affected sections present?] - **Dangling task-document references:** [No \`.fusion/tasks//\` path is cited in Context, Steps, or File Scope unless the file exists or is explicitly created as a \`(new)\` artifact in this spec. References to nonexistent task-local artifacts are a blocking REVISE.] - **Sizing & review level:** [Size and review level appropriate for the work?] @@ -202,6 +202,7 @@ Do NOT demand function-level implementation checklists. When reviewing tests, check that they verify observable behavior and regression risk (not only implementation trivia). Flag REVISE when key edge cases or failure modes for changed behavior are untested. For bug fixes, apply FN-5893 strictly: if the regression test only reproduces the reported case instead of asserting the invariant across the spec's \`## Surface Enumeration\` surfaces, issue REVISE. Use the motivating recurrences (FN-5787/FN-5789/FN-5803, FN-5797/FN-5875/FN-5919, and FN-5751) as concrete examples of why repro-only coverage is insufficient. +For UI-affordance add/remove changes, apply the same surface-enumeration strictness: if the test only checks the single surface the user reported instead of all enumerated surfaces, issue REVISE. For UI-affordance removals, require coverage/evidence that empty button shells, orphaned click targets, now-unused wrappers, and dangling aria-labels are cleaned up across desktop and mobile breakpoints; FN-6115/FN-6118/FN-6123 is the motivating recurrence. ## Worktree Boundary Review diff --git a/packages/engine/src/triage.ts b/packages/engine/src/triage.ts index d8ac33a5ba..8ff648e6b4 100644 --- a/packages/engine/src/triage.ts +++ b/packages/engine/src/triage.ts @@ -120,7 +120,7 @@ Follow this structure exactly: ## Surface Enumeration -{Required for bug-fix tasks: a checklist enumerating every surface the fixed invariant must hold across. Include every provider/bridge for streaming and agent paths; desktop AND mobile breakpoints; empty/undefined/duplicate/populated data states; and every hook/component/module that shares the affected logic. Use the canonical checklist in docs/testing.md as the starting point.} +{Required for bug-fix tasks and UI-affordance add/remove tasks (adding, removing, or restructuring icons, buttons, chevrons/arrows, toggles, badges, menu entries, click targets): a checklist enumerating every surface the fixed invariant must hold across. Include every provider/bridge for streaming and agent paths; desktop AND mobile breakpoints; empty/undefined/duplicate/populated data states; and every hook/component/module that shares the affected logic. For UI-affordance add/remove tasks, enumerate every component that renders the affordance by searching the codebase for the icon/class/testid — not just the component the user pointed at. Explicitly check for leftover shells after removal (empty buttons, orphaned click targets, now-unused wrappers, dangling aria-labels) across both desktop and mobile breakpoints. Use the canonical checklist in docs/testing.md as the starting point.} ## Dependencies @@ -158,11 +158,13 @@ Follow this structure exactly: - [ ] {Specific, verifiable outcome} - [ ] Run targeted tests for changed files, asserting the invariant across all known surfaces (enumerate every provider/bridge, desktop + mobile breakpoints, and empty/undefined/populated data states) -For bug-fix tasks, paste and fill in this checklist in the \`## Surface Enumeration\` section: +For bug-fix and UI-affordance add/remove tasks, paste and fill in this checklist in the \`## Surface Enumeration\` section: - [ ] Providers / bridges / execution paths touched by the invariant - [ ] Desktop + mobile breakpoints / platforms that exercise the behavior - [ ] Empty / undefined / duplicate / populated data states - [ ] Shared hooks / components / modules / helpers reusing the logic +- [ ] Every component that renders the affordance (search the codebase for the icon/class/testid, not just the one the user pointed at) +- [ ] Leftover shells after removal — empty buttons, orphaned click targets, now-unused wrappers, dangling aria-labels — are explicitly checked and fixed/hidden **Artifacts:** - \`path/to/file\` (new | modified) @@ -238,9 +240,9 @@ files with assertions that run via a test runner. Typechecks and builds are NOT tests. Manual verification is NOT a test. - Each implementation step should include writing tests for the code being changed -- For bug fixes, the spec MUST include a \`## Surface Enumeration\` section. During self-review via \`fn_review_spec()\`, treat a missing section on a bug-fix spec as a blocking REVISE. -- For bug fixes, populate \`## Surface Enumeration\` with this checklist from \`docs/testing.md\`: providers/bridges/execution paths; desktop + mobile breakpoints/platforms; empty/undefined/duplicate/populated data states; shared hooks/components/modules/helpers. -- For bug fixes, regression tests must assert the invariant across all known surfaces — enumerate every provider/bridge, desktop + mobile breakpoints, and empty/undefined/populated data states — not just the reported repro (see FN-5787/FN-5789/FN-5803 and FN-5751) +- For bug fixes and UI-affordance add/remove tasks, the spec MUST include a \`## Surface Enumeration\` section. During self-review via \`fn_review_spec()\`, treat a missing section on a bug-fix or UI-affordance add/remove spec as a blocking REVISE. +- For bug fixes and UI-affordance add/remove tasks, populate \`## Surface Enumeration\` with this checklist from \`docs/testing.md\`: providers/bridges/execution paths; desktop + mobile breakpoints/platforms; empty/undefined/duplicate/populated data states; shared hooks/components/modules/helpers; every component that renders the affordance; leftover shells after removal. +- For bug fixes and UI-affordance add/remove tasks, regression tests must assert the invariant across all known surfaces — enumerate every provider/bridge, desktop + mobile breakpoints, empty/undefined/populated data states, and for UI-affordance changes every component rendering the affordance plus leftover shells after removal — not just the reported repro (see FN-5787/FN-5789/FN-5803, FN-5751, and FN-6115/FN-6118/FN-6123) - The final Testing step runs lint, impacted/package-scoped tests first, and project typecheck when the repo exposes one. Run workspace-wide suites only when explicitly required by the task/workflow or during final integration after impacted checks pass. - Specs must instruct executors to fix lint failures and quality-gate failures directly, even when the required edits extend beyond the original File Scope - If the project has no test framework, the Testing step must include setting one up @@ -424,7 +426,7 @@ Follow this structure exactly: ## Surface Enumeration -{Required for bug-fix tasks: a checklist enumerating every surface the fixed invariant must hold across. Include every provider/bridge for streaming and agent paths; desktop AND mobile breakpoints; empty/undefined/duplicate/populated data states; and every hook/component/module that shares the affected logic. Use the canonical checklist in docs/testing.md as the starting point.} +{Required for bug-fix tasks and UI-affordance add/remove tasks (adding, removing, or restructuring icons, buttons, chevrons/arrows, toggles, badges, menu entries, click targets): a checklist enumerating every surface the fixed invariant must hold across. Include every provider/bridge for streaming and agent paths; desktop AND mobile breakpoints; empty/undefined/duplicate/populated data states; and every hook/component/module that shares the affected logic. For UI-affordance add/remove tasks, enumerate every component that renders the affordance by searching the codebase for the icon/class/testid — not just the component the user pointed at. Explicitly check for leftover shells after removal (empty buttons, orphaned click targets, now-unused wrappers, dangling aria-labels) across both desktop and mobile breakpoints. Use the canonical checklist in docs/testing.md as the starting point.} ## Dependencies @@ -462,11 +464,13 @@ Follow this structure exactly: - [ ] {Specific, verifiable outcome} - [ ] Run targeted tests for changed files, asserting the invariant across all known surfaces (enumerate every provider/bridge, desktop + mobile breakpoints, and empty/undefined/populated data states) -For bug-fix tasks, paste and fill in this checklist in the \`## Surface Enumeration\` section: +For bug-fix and UI-affordance add/remove tasks, paste and fill in this checklist in the \`## Surface Enumeration\` section: - [ ] Providers / bridges / execution paths touched by the invariant - [ ] Desktop + mobile breakpoints / platforms that exercise the behavior - [ ] Empty / undefined / duplicate / populated data states - [ ] Shared hooks / components / modules / helpers reusing the logic +- [ ] Every component that renders the affordance (search the codebase for the icon/class/testid, not just the one the user pointed at) +- [ ] Leftover shells after removal — empty buttons, orphaned click targets, now-unused wrappers, dangling aria-labels — are explicitly checked and fixed/hidden **Artifacts:** - \`path/to/file\` (new | modified) @@ -537,9 +541,9 @@ If this task REMOVES existing functionality (deleting modules, settings, API end ## Testing requirements - Require real automated tests with assertions that run in the project's test runner - Typecheck/build/manual checks are not tests and cannot replace tests -- For bug fixes, the spec MUST include a \`## Surface Enumeration\` section. During self-review via \`fn_review_spec()\`, treat a missing section on a bug-fix spec as a blocking REVISE. -- For bug fixes, populate \`## Surface Enumeration\` with this checklist from \`docs/testing.md\`: providers/bridges/execution paths; desktop + mobile breakpoints/platforms; empty/undefined/duplicate/populated data states; shared hooks/components/modules/helpers. -- For bug fixes, regression tests must assert the invariant across all known surfaces — enumerate every provider/bridge, desktop + mobile breakpoints, and empty/undefined/populated data states — not just the reported repro (see FN-5787/FN-5789/FN-5803 and FN-5751) +- For bug fixes and UI-affordance add/remove tasks, the spec MUST include a \`## Surface Enumeration\` section. During self-review via \`fn_review_spec()\`, treat a missing section on a bug-fix or UI-affordance add/remove spec as a blocking REVISE. +- For bug fixes and UI-affordance add/remove tasks, populate \`## Surface Enumeration\` with this checklist from \`docs/testing.md\`: providers/bridges/execution paths; desktop + mobile breakpoints/platforms; empty/undefined/duplicate/populated data states; shared hooks/components/modules/helpers; every component that renders the affordance; leftover shells after removal. +- For bug fixes and UI-affordance add/remove tasks, regression tests must assert the invariant across all known surfaces — enumerate every provider/bridge, desktop + mobile breakpoints, empty/undefined/populated data states, and for UI-affordance changes every component rendering the affordance plus leftover shells after removal — not just the reported repro (see FN-5787/FN-5789/FN-5803, FN-5751, and FN-6115/FN-6118/FN-6123) - Include targeted tests in implementation steps and full quality-gate runs in final verification ## Duplicate check