Public: anyone can read this; the crew can change it.

Automate a review finding as a lint or test: Minor: the duty.ts header comment is now one very long line. Wrap it. #9969

Merged opened this

Check · Review · Land · Live done — Landed and live in production.

Merged

This adds a check that fails when a file's leading `/** … */` header comment has a line over 160 columns, and the failure message names the rule `header-width`. The new test and `tsc --noEmit` pass, and `scripts/gate.ts` printed nothing. I did not run the full suite, since the forge runs it on push. I did not add a step to `scripts/gate.ts`, because the gate already picks up every `test/*.test.ts`…

shipped production is serving it deploy 93e874d

opened this

This adds a check that fails when a file's leading /** … / header comment has a line over 160 columns, and the failure message names the rule header-width. The new test and tsc --noEmit pass, and scripts/gate.ts printed nothing. I did not run the full suite, since the forge runs it on push. I did not add a step to scripts/gate.ts, because the gate already picks up every test/.test.ts.

  • What it checks: the new test/header-width.test.ts runs longHeaderLines and offenders from scripts/header-width.ts over src, scripts and test. It reads only the header comment, not other comments or code. Running bun scripts/header-width.ts by hand does the same check and exits 1 on a new offender.
  • Existing offenders: 10 files are already over the limit. They are listed in scripts/header-width-allow.json, and the list can only shrink. A new offender fails the test, and so does a listed file that has since been wrapped.
  • The three files the reviewers named: src/perf/duty.ts is already wrapped on main, as are the header comments in src/tui/screens/hooks.ts and conflicts.ts, so none of them is on the list.
  • Why 160: at 100 or 120 columns the allow list would hold hundreds of files. This limit catches the one-very-long-line case the reviewers described. Lowering MAX_WIDTH later is a one-line change plus regenerating the list.
  • Not done: I did not rewrap the 10 listed files, and the rule does not cover // comments or comments inside functions.

To try it, run bun test test/header-width.test.ts. Making any header line longer than 160 columns should fail it with a rule header-width message.

Closes #9960

commented

This adds a check that fails when a file's leading /** … / header comment has a line over 160 columns, and the failure message names the rule header-width. The new test and tsc --noEmit pass, and scripts/gate.ts printed nothing. I did not run the full suite, since the forge runs it on push. I did not add a step to scripts/gate.ts, because the gate already picks up every test/.test.ts.

  • What it checks: the new test/header-width.test.ts runs longHeaderLines and offenders from scripts/header-width.ts over src, scripts and test. It reads only the header comment, not other comments or code. Running bun scripts/header-width.ts by hand does the same check and exits 1 on a new offender.
  • Existing offenders: 10 files are already over the limit. They are listed in scripts/header-width-allow.json, and the list can only shrink. A new offender fails the test, and so does a listed file that has since been wrapped.
  • The three files the reviewers named: src/perf/duty.ts is already wrapped on main, as are the header comments in src/tui/screens/hooks.ts and conflicts.ts, so none of them is on the list.
  • Why 160: at 100 or 120 columns the allow list would hold hundreds of files. This limit catches the one-very-long-line case the reviewers described. Lowering MAX_WIDTH later is a one-line change plus regenerating the list.
  • Not done: I did not rewrap the 10 listed files, and the rule does not cover // comments or comments inside functions.

To try it, run bun test test/header-width.test.ts. Making any header line longer than 160 columns should fail it with a rule header-width message.

Approved

Does what #9960 asks: a test fails on an over-wide header line and names rule header-width, with a shrink-only allow list. Small, no dependency, no UI so nothing for a phone or keyboard to break. Two things I would do next: (1) the CLI path in scripts/header-width.ts reports only new offenders, not stale allow-list entries as the test does, so the two doors disagree; share one verdict function. (2) Rewrap the 10 listed files so the allow list can be deleted.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs