Skip to content

Commit abe59b1

Browse files
alicodingclaude
andauthored
feat: comment-hygiene — constraints not narrative, rule + gate + sweep (goal 0038) (#55)
* feat: comment-hygiene standard + gate (goal 0038, 1/2) Source comments state constraints, never decision provenance -- .claude/rules/comments.md is the standard; check-comment-hygiene.sh enforces it from lefthook pre-commit and a required CI job (same one-script-two-callers shape as check-loc.sh). Baseline: 65 violations, swept in the follow-up commit of this goal's PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FJ8wStsHyu7XPLTspNjMnQ * fix: sweep decision-provenance out of source comments (goal 0038, 2/2) All 65 baseline violations resolved per .claude/rules/comments.md: 62 comment rewrites keeping constraints/regression properties and docs citations while dropping owner quotes, session narrative, and dates; plus 3 string literals rewritten as neutral product copy (a Playwright test title, a capability-map StatusDetail, the integration-http NodeType description -- no test pinned any of the three). check-comment-hygiene.sh now exits 0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FJ8wStsHyu7XPLTspNjMnQ * docs: archive goal 0038 — acceptance met, delivered 2026-08-13 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FJ8wStsHyu7XPLTspNjMnQ --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 94d8182 commit abe59b1

58 files changed

Lines changed: 277 additions & 152 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

.claude/rules/comments.md

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
# Comment discipline — constraints, not narrative
2+
3+
No `paths` frontmatter deliberately — applies to every hand-written
4+
source file regardless of language, the same way architecture.md does.
5+
6+
**A code comment states what the code cannot: an invariant, a
7+
non-obvious constraint, an external tool's gotcha, the property a
8+
regression test pins down. It never carries decision provenance**
9+
who decided something, when, in which session, or a quote of the
10+
product owner. Phrases like "direct user decision", "owner-directed",
11+
"caught live", "this session", and calendar dates are history, not
12+
constraints; history lives in `docs/SPEC.md`, `docs/adr/`, and
13+
`docs/goals/`. A comment may cite one of those by id or section
14+
(delivery-discipline.md's existing pointer rule), and that pointer is
15+
where a reader goes for the why. The test: delete the sentence — if
16+
the next maintainer loses nothing they'd need to safely change the
17+
code, it was narrative.
18+
19+
For regression tests specifically: name the failure property, not the
20+
discovery story. "Regression: the preview's nested canvas captured
21+
page scroll" says what must keep passing; "caught live while building
22+
the Runs UI" says nothing a maintainer can use.
23+
24+
Enforced by `scripts/check-comment-hygiene.sh`, run by lefthook
25+
(pre-commit) and CI's `comment-hygiene` job — the same
26+
one-script-both-callers shape as `check-loc.sh`, so the two can't
27+
drift. It deny-lists the known provenance phrases everywhere in
28+
hand-written `.go`/`.ts`/`.tsx` files, and calendar dates on comment
29+
lines only (dates inside code/string literals — test fixtures — are
30+
legitimate). A genuine false positive (a real domain concept named
31+
"owner", say) may carry `comment-hygiene:allow` on the same line;
32+
never use the marker to smuggle actual provenance past the gate.

.github/workflows/ci.yml

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,16 @@ jobs:
8888
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
8989
- run: ./scripts/check-loc.sh
9090

91+
# Mirrors lefthook.yml's own comment-hygiene job -- same script
92+
# (.claude/rules/comments.md is the standard it enforces). Grep-only,
93+
# so like file-loc-limit it needs no build setup and no changes gate.
94+
comment-hygiene:
95+
runs-on: ubuntu-latest
96+
timeout-minutes: 5
97+
steps:
98+
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
99+
- run: ./scripts/check-comment-hygiene.sh
100+
91101
# Mirrors lefthook.yml's own rules-frontmatter job -- same script.
92102
# Catches a real bug class: a .claude/rules/*.md with an invalid
93103
# frontmatter key (e.g. a `globs:` typo instead of `paths:`) silently
@@ -402,6 +412,7 @@ jobs:
402412
needs:
403413
- changes
404414
- file-loc-limit
415+
- comment-hygiene
405416
- rules-frontmatter
406417
- root-file-naming
407418
- frontend

docs/goals/BACKLOG.md

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,15 @@ backlog section in the same commit: **work discovered mid-session that
2828
outlives the session gets a goal file and a queue position before the
2929
session ends — never left only in an ephemeral session task list.**
3030

31+
**Group E — Public-repo hygiene (2026-08-13, owner-directed: source
32+
comments explain code per standard practice — business/product
33+
decisions and the owner's own words live in docs/, cited by id, never
34+
inline in source)**
35+
1. [x] [0038 — Comment hygiene: constraints, not narrative](archive/0038-comment-hygiene.md) — delivered 2026-08-13
36+
— rule (`.claude/rules/comments.md`) + gate
37+
(`check-comment-hygiene.sh`, lefthook + CI `comment-hygiene` job in
38+
`ci-gate`) + one sweep of the 65 baseline narrative comments
39+
3140
**Group D — Trust the substrate (2026-08-11, owner-mandated: "do it
3241
properly once"; ordered first because everything else ships through
3342
this pipeline and on this code)**
Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
# Goal 0038 — Comment hygiene: constraints, not narrative
2+
3+
**Goal.** Source comments state technical constraints only. Decision
4+
provenance — owner quotes, "direct user decision", "caught live",
5+
"this session", calendar dates — moves out of `.go`/`.ts`/`.tsx`
6+
files entirely; the reasoning already lives in `docs/SPEC.md`,
7+
`docs/adr/`, and `docs/goals/`, and a comment cites those by id when
8+
the why matters. Owner-directed 2026-08-13, generalizing the earlier
9+
public-context scrub (`fix/public-context-scrub`) into a standing,
10+
enforced standard now that the repo is public.
11+
12+
**Plan.**
13+
1. `.claude/rules/comments.md` — the standard (loads unconditionally,
14+
no `paths` frontmatter).
15+
2. `scripts/check-comment-hygiene.sh` — deny-list gate over
16+
hand-written source (provenance phrases anywhere; calendar dates on
17+
comment lines only), wired into lefthook pre-commit AND a CI
18+
`comment-hygiene` job required by `ci-gate`, in the same change
19+
(CI-from-day-one constraint). Same one-script-two-callers shape as
20+
`check-loc.sh`.
21+
3. One sweep of the 65 baseline violations: keep each comment's
22+
technical constraint, drop the narrative, add a `docs/` citation
23+
where the reasoning lives elsewhere. Regression-test comments name
24+
the pinned property, not the discovery story.
25+
26+
**Acceptance** (checkable predicates)
27+
- [x] `./scripts/check-comment-hygiene.sh` exits 0 on the full tree.
28+
- [x] `.claude/rules/comments.md` exists, frontmatter check passes.
29+
- [x] lefthook.yml and ci.yml both run the script; `ci-gate` lists the
30+
CI job in `needs`.
31+
- [x] No technical information lost: every rewritten comment still
32+
states the constraint or property the original did (review
33+
check, not grep).
34+
- [x] Full local suite green; goal PR merged with `ci-gate` green.

frontend/e2e/authoring-validation.spec.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,9 @@ import { clickRowAction } from './inventoryRow'
55
// (errors block save, warnings never do), driven through the real
66
// canvas -- not just the Go-level ValidateGraph/ValidateGraphStrict
77
// unit tests (internal/domain/composition/graph_test.go). Covers the
8-
// three acceptance scenarios named in goal 0008: (a) the owner's own
9-
// demonstrated repro (a workflow whose root isn't a Trigger) can no
10-
// longer save, with the error named in the issues panel; (b) a
8+
// three acceptance scenarios named in goal 0008: (a) a workflow whose
9+
// root isn't a Trigger can no longer save, with the error named in
10+
// the issues panel; (b) a
1111
// trigger-rooted workflow ending in a Process leaf saves anyway, with
1212
// a visible warning on both the toolbar badge and the node itself, and
1313
// clicking the panel row selects that node; (c) an existing seeded
@@ -108,7 +108,7 @@ async function connectNodes(page: import('@playwright/test').Page, sourceLabel:
108108
// debounce + a real RPC) needs real slack beyond the debounce itself.
109109
const VALIDATION_TIMEOUT = 5_000
110110

111-
test('The owner\'s repro: a workflow whose root is not a Trigger cannot be saved, with the error named in the issues panel', async ({ page }) => {
111+
test('A workflow whose root is not a Trigger cannot be saved, with the error named in the issues panel', async ({ page }) => {
112112
await page.goto('/')
113113
await page.getByRole('link', { name: 'Workflows' }).click()
114114
await page.getByTestId('new-workflow').click()

frontend/e2e/composition-export-import.spec.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ test('Importing a workflow file adds a new, independent workflow without touchin
4848
label: 'E2E imported workflow',
4949
description: 'Imported by an e2e test',
5050
// A Trigger root (docs/adr/0028) -- a lone Capture node alone is
51-
// now the owner's own unsaveable repro.
51+
// unsaveable.
5252
nodes: [
5353
{ ID: 't', NodeTypeID: 'trigger-manual' },
5454
{ ID: 'c', NodeTypeID: 'capture-clipboard-html' },

frontend/e2e/global-setup.ts

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,13 @@ import { fileURLToPath } from 'node:url'
1919
const REPO_ROOT = path.resolve(fileURLToPath(new URL('.', import.meta.url)), '../..')
2020

2121
// The output path lives under frontend/e2e/.build/, NOT the repo's
22-
// shared bin/ -- learned live 2026-08-11: bin/ is also where `task dev`
23-
// (and build/package's `clean` = rm -rf bin/) operate, so an e2e suite
24-
// and a dev loop running concurrently could destroy each other's
25-
// artifacts mid-flight (a suite lost its server binary mid-run: 91
26-
// spawn-ENOENT failures; a dev app lost its own .app bundle). A
27-
// dedicated, gitignored path removes the shared-artifact contention
28-
// structurally instead of trying to sequence the two loops.
22+
// shared bin/ -- bin/ is also where `task dev` (and build/package's
23+
// `clean` = rm -rf bin/) operate, so an e2e suite and a dev loop
24+
// running concurrently could destroy each other's artifacts mid-flight
25+
// (a suite losing its server binary mid-run means spawn-ENOENT
26+
// failures; a dev app losing its own .app bundle). A dedicated,
27+
// gitignored path removes the shared-artifact contention structurally
28+
// instead of trying to sequence the two loops.
2929
export default function globalSetup(): void {
3030
execFileSync('go', ['build', '-tags', 'server', '-o', 'frontend/e2e/.build/mill-server', '.'], {
3131
cwd: REPO_ROOT,

frontend/e2e/layout.spec.ts

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -83,11 +83,10 @@ test('the titlebar band\'s left segment tracks the sidebar column: it resizes an
8383
expect(sidebarBox).not.toBeNull()
8484
expect(Math.abs(sidebarBox!.width - collapsedLeftBox!.width)).toBeLessThan(2)
8585

86-
// (c) the toggle lives in the SIDEBAR's own top row, below the band
87-
// (owner design 2026-08-11: the control sits next to the nav it
88-
// controls; collapsed, the same slot is the Mill icon that swaps to
89-
// this expand affordance on hover/focus). Its x stays inside the
90-
// rail column.
86+
// (c) the toggle lives in the SIDEBAR's own top row, below the band:
87+
// the control sits next to the nav it controls; collapsed, the same
88+
// slot is the Mill icon that swaps to this expand affordance on
89+
// hover/focus. Its x stays inside the rail column.
9190
const toggleBox = await page.getByRole('button', { name: 'Expand navigation' }).boundingBox()
9291
expect(toggleBox).not.toBeNull()
9392
expect(toggleBox!.y).toBeGreaterThanOrEqual(collapsedLeftBox!.y + collapsedLeftBox!.height - 1)
@@ -154,10 +153,9 @@ test.describe('tab strip vs build badge', () => {
154153
test.use({ viewport: { width: 1000, height: 800 } })
155154

156155
test('the tab-strip overflow button is clickable with the build badge present, and the badge lives inside the band', async ({ page }) => {
157-
// Regression coverage for the live-caught overlay bug (2026-08-11):
158-
// the build-identity badge was position:fixed at the window's
159-
// top-right, exactly over the tab strip's pinned "All open tabs" ⌄
160-
// button, and intercepted its clicks -- the same overlay-blocks-chrome
156+
// Regression: a position:fixed build-identity badge at the window's
157+
// top-right sat exactly over the tab strip's pinned "All open tabs" ⌄
158+
// button and intercepted its clicks -- the same overlay-blocks-chrome
161159
// class the isolated-data ribbon already hit once with the sidebar
162160
// toggle. The fix makes the badge a flex participant in the band's
163161
// own right segment, so overlap is impossible by construction.

frontend/e2e/workflow-hover-preview.spec.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ test('Hovering the child-workflow NODE previews the child without blanking the p
2929
// The child's real layout renders (its own React Flow instance with
3030
// its three nodes), not just a text summary.
3131
await expect(preview.locator('.react-flow__node')).toHaveCount(3)
32-
// Regression (caught live): the preview's nested React Flow used to
32+
// Regression: the preview's nested React Flow used to
3333
// join the parent canvas's store and EMPTY it -- the parent's two
3434
// nodes must still be on its canvas while the preview is open
3535
// (the overlay portals outside the tabpanel, so this scope only

frontend/playwright.config.ts

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -55,11 +55,10 @@ export default defineConfig({
5555
// Playwright's own default reporter (list locally, dot in CI) never
5656
// writes a report to disk -- ci.yml's e2e job already expects one at
5757
// frontend/playwright-report on failure (its own upload-artifact
58-
// step), but with no reporter configured that path never existed,
59-
// confirmed directly against a real failed CI run (2026-08-11's
60-
// Linux-e2e investigation): "No files were found with the provided
61-
// path" swallowed the one artifact that would have shown per-failure
62-
// screenshots/traces instead of bare text logs. `open: 'never'` so a
58+
// step), but with no reporter configured that path never existed:
59+
// "No files were found with the provided path" swallowed the one
60+
// artifact that would have shown per-failure screenshots/traces
61+
// instead of bare text logs. `open: 'never'` so a
6362
// local `npx playwright test` run never pops a browser tab
6463
// mid-session.
6564
reporter: [['html', { open: 'never' }], ['list']],

0 commit comments

Comments
 (0)