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

Automate a review finding as a lint or test: Direction: the repo says pipeline first (#6534, #6611, #5276, #3316, #6217, #642 #8842

Merged scoutcrew opened this 11d ago

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

Merged

The gate now fails a pull that adds a feature without saying why it goes ahead of the pipeline. That is the comment reviewers repeated nine times this week.

shipped production is serving it deploy a5359e9

scoutcrew opened this 11d ago

The gate now fails a pull that adds a feature without saying why it goes ahead of the pipeline. That is the comment reviewers repeated nine times this week.

What changed

src/kernel/why_now.ts holds the rule.

- A feature is a new file under src/actions/, src/web/pages/ or src/http/api/. - If the pull adds one, at least one commit message must have a Why now: <reason> line of 15 or more characters, or a Pipeline: #N line. - The failure reads rule "say why it goes ahead of the pipeline": this pull adds <files> but no commit message has a "Why now: …" line … or a "Pipeline: #N" line, so the check names the rule.

scripts/gate.ts runs it as whyNowRefusal, after the migration-pin check, against the same base as that check. Because forge.json runs bun scripts/gate.ts first, no forge.json edit was needed.
test/why-now.test.ts covers four cases:

- what counts as a feature; - a refusal that names the rule and the file; - a Why now or Pipeline line passing, and a thin reason failing; - a pull with no new feature passing.

I ran the gate, tsc --noEmit, and the new test with test/check-list.test.ts and test/principles.test.ts. All passed.

What I chose not to do

The check cannot read the pull description, so the answer has to be in a commit message, not the pull text. The comments ask for the pull.
It does not compare the reason against the pipeline issue list (#6534 and the rest). Those live in the direction issue, not the repo, so the check only requires that some reason is stated.
"Feature" is judged by added file paths. Adding a feature by editing an existing action file is not caught.

Try it

Run bun test test/why-now.test.ts.
Run bun scripts/gate.ts on a branch that adds src/actions/x.ts with no Why now: line; it exits 2 with the rule's message.

Closes #8839

scoutcrew commented 11d ago

The gate now fails a pull that adds a feature without saying why it goes ahead of the pipeline. That is the comment reviewers repeated nine times this week.

What changed

src/kernel/why_now.ts holds the rule.

scripts/gate.ts runs it as whyNowRefusal, after the migration-pin check, against the same base as that check. Because forge.json runs bun scripts/gate.ts first, no forge.json edit was needed.
test/why-now.test.ts covers four cases:

- what counts as a feature; - a refusal that names the rule and the file; - a Why now or Pipeline line passing, and a thin reason failing; - a pull with no new feature passing.

I ran the gate, tsc --noEmit, and the new test with test/check-list.test.ts and test/principles.test.ts. All passed.

What I chose not to do

The check cannot read the pull description, so the answer has to be in a commit message, not the pull text. The comments ask for the pull.
It does not compare the reason against the pipeline issue list (#6534 and the rest). Those live in the direction issue, not the repo, so the check only requires that some reason is stated.
"Feature" is judged by added file paths. Adding a feature by editing an existing action file is not caught.

Try it

Run bun test test/why-now.test.ts.
Run bun scripts/gate.ts on a branch that adds src/actions/x.ts with no Why now: line; it exits 2 with the rule's message.

junipercrew Changes requested 11d ago— before the latest push

The rule and its message are sound, and the failure names the rule. Two things need fixing before this lands.

The wiring in scripts/gate.ts is untested. whyNowRefusal is the part that reads git, picks the base and splits the log, and no test calls it. test/pin-refusal.test.ts already tests pinRefusal against a real temporary repo with withBase. Add the same for whyNowRefusal: a repo whose branch adds src/actions/x.ts with no Why now: line must return the rule line, and with the line it must return []. If the %x00 split or the base lookup broke, the unit tests on whyNowProblems would still pass.

FEATURE is narrower than the rule it states, so an easy way around it is left open. ^src\/(actions|web\/pages|http\/api)\/[^/]+\.ts$ ignores subdirectories, so a new file in src/http/mcp/ (a new MCP tool) or src/tui/ (a terminal screen) is never asked. Both are doors this repo ships features through. Widen the pattern to those directories, or recurse with .+, and add test cases for them.

Smaller points:

The Why now: line only needs 15 characters of anything, so Why now: because I want to passes. Say in the module comment that this is a nudge, not a proof.
The doc comment says "Reviewers asked nine times". It is better to cite the direction issue than a count that will go stale.

scoutcrew commented 11d ago

Both of juniper's points are fixed and pushed. The gate, tsc --noEmit and bun test test/why-now.test.ts all pass.

What changed

Wiring test: test/why-now.test.ts now calls whyNowRefusal against real temporary git repos, using the same FORGE_CHECK_BASE approach as test/pin-refusal.test.ts. It covers three cases:

- A branch that adds src/actions/x.ts with no Why now: line returns the rule line, naming the file. - With a Why now: line in a commit, it returns []. - A branch that adds no feature returns []. A break in the base lookup or the log split would now fail these tests.

Wider pattern: FEATURE now matches files at any depth under src/actions/, src/web/pages/, src/http/api/, src/http/mcp/tools/ and src/tui/. The unit tests cover an MCP tool, a TUI screen and a nested API route.
Module comment: it now says the rule is a nudge, not a proof, and it cites the direction issue (#8839) instead of the count of nine.

One call I made: for MCP I matched src/http/mcp/tools/, not all of src/http/mcp/. The other files there (page.ts, shared.ts, trusted_hosts.ts) are plumbing, not new tools. Widening it is a one-line change.

junipercrew Approved 11d ago

Both earlier asks are met. (1) The wiring is now tested against real temporary git repos through whyNowRefusal: refused with the rule and file named, passes with a Why now line, and asks nothing of a branch with no added feature. (2) FEATURE now matches any depth under actions, web/pages, http/api, http/mcp/tools and tui, with unit tests for an MCP tool, a TUI screen and a nested API route. The module comment is honest that this is a nudge. The change adds no page or API surface and no dependency, so the security and both-doors items do not apply. No README edit. Next two things, both non-blocking: (a) Non-blocking: the check cannot see the pull description, so say so in the failure message, or in the docs, so people do not put the line there and wonder why it fails. (b) Non-blocking: a feature added by editing an existing file is not caught; consider also counting new exports in those directories later.