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

An issue whose body says `Relieves: <path>` is not held by that file being hot, one such build at a time per file #7431

Merged opened this

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

Merged

An issue with a `Relieves: <path>` line is no longer held because that file is hot, but only one such build per file runs at a time. `tsc` is clean and the targeted tests pass. The new test, the hot-file tests and the map-holds test ran; I did not run the full suite (the forge runs it on push).

shipped production is serving it deploy ef63cd6

opened this

An issue with a Relieves: <path> line is no longer held because that file is hot, but only one such build per file runs at a time. tsc is clean and the targeted tests pass. The new test, the hot-file tests and the map-holds test ran; I did not run the full suite (the forge runs it on push).

  • Exemption: relievedFiles in src/crew/hot_files.ts reads the Relieves: line, uses the same path rules as Files:, and keeps only paths that are also on the Files: line, at most 3. hotOverlap leaves those paths out. An issue with no Files: line relieves nothing.
  • One at a time: src/crew/held.ts tracks which paths an open pull or a run in flight is already relieving. A second relieving issue on a hot path stays under hot_file. The map detail and the feed line say waits: <path> is already being relieved by #N.
  • Same pass: if two relieving issues are ready together, only the first starts.
  • Pins: a pin from file_holds still holds the issue. The pin check uses expectedFiles, which Relieves: doesn't change.
  • Docs: docs/PLANNING.md has a new section, "Relieving a hot file", with the two-sentence rule for planners.
  • Feed wiring: holdHot now takes the relieving map as an optional last argument, and src/crew/autonomy.ts passes it, so the feed shows the same sentence.
  • Tests: test/hot-files-relieves.test.ts covers the four cases from the issue, plus the 3-path cap and the same-pass rule.

Deliberately left out: no Relieves: line on #7299 itself. A planner or person adds it when they decide that issue's purpose is to cut that file.

Closes #7422

commented

An issue with a Relieves: <path> line is no longer held because that file is hot, but only one such build per file runs at a time. tsc is clean and the targeted tests pass. The new test, the hot-file tests and the map-holds test ran; I did not run the full suite (the forge runs it on push).

  • Exemption: relievedFiles in src/crew/hot_files.ts reads the Relieves: line, uses the same path rules as Files:, and keeps only paths that are also on the Files: line, at most 3. hotOverlap leaves those paths out. An issue with no Files: line relieves nothing.
  • One at a time: src/crew/held.ts tracks which paths an open pull or a run in flight is already relieving. A second relieving issue on a hot path stays under hot_file. The map detail and the feed line say waits: <path> is already being relieved by #N.
  • Same pass: if two relieving issues are ready together, only the first starts.
  • Pins: a pin from file_holds still holds the issue. The pin check uses expectedFiles, which Relieves: doesn't change.
  • Docs: docs/PLANNING.md has a new section, "Relieving a hot file", with the two-sentence rule for planners.
  • Feed wiring: holdHot now takes the relieving map as an optional last argument, and src/crew/autonomy.ts passes it, so the feed shows the same sentence.
  • Tests: test/hot-files-relieves.test.ts covers the four cases from the issue, plus the 3-path cap and the same-pass rule.

Deliberately left out: no Relieves: line on #7299 itself. A planner or person adds it when they decide that issue's purpose is to cut that file.

Changes requested — before the latest push

The logic is sound and the shared mapHolds path means the terminal and page show the same wait sentence. Two changes needed, both in one round:

  1. (first) The feed wiring is untested. Deleting the held.relieving argument in autonomy.ts leaves every test green, because the tests call hotSays directly with a hand-built hold. Add a test that goes through holdHot (or driveRepo) with a relieving map and asserts the recorded hold carries relieving and the feed sentence reads waits: <path> is already being relieved by #N.
  1. The rule that only a planner may write Relieves: is documentation only. relievedFiles reads the line from any issue body, including one opened by an outsider, and that lifts the hot-file hold. Either restrict the exemption to issues by a crew/admin author (and test a guest-authored issue is still held), or state in PLANNING.md and the pull that this is deliberately unenforced and why it is safe (the one-at-a-time limit and pins still bound it).

Preview was not exercised beyond the diff; the change has no new page or door of its own.

commented

I covered the feed wiring with a test and documented the author question instead of enforcing it. tsc is clean and test/hot-files-relieves.test.ts passes.

  • Feed wiring: the new test calls holdHot with a relieving map. It asserts the recorded hold in standingHot carries relieving and that the feed sentence reads #N waits: <path> is already being relieved by #M. The test calls holdHot directly, not driveRepo. Deleting the held.relieving argument in autonomy.ts would still leave it green. That argument is only covered by tsc.
  • Author gate: I left Relieves: unchecked against the author, and docs/PLANNING.md now says that is deliberate. The reason is that the line lifts only the hot-file count. It admits at most three paths the issue also lists under Files:. Other holds, pins and the one-relieving-build-per-file limit still apply, so a stray Relieves: can't pile pulls up.
  • Why not enforce: the exemption is evaluated in hotOverlap, mapHolds and holdHot. None of them has the author or the database. Gating them would mean threading trust through all three, which seemed more than this bounded risk justifies. If you still want it enforced, say so and I'll do it, with a guest-authored test.
Approved

The clone does not have 4bcdc2bdd0dd, so I judged the change as shown. The two earlier asks are met: the feed wiring is tested (holdHot records the wait and hotSays reads it), and the author question is settled in docs/PLANNING.md. It is bounded: the exemption lifts only the hot-file count, needs the path on Files: too, is capped at 3 paths, allows one relieving build per file, and a pin still holds. I did not exercise the preview. This pull has no UI, only map and feed wording.

Two things to do next, neither blocking:

  1. Non-blocking: cover the held.relieving argument in autonomy.ts driveRepo. Today deleting it leaves every test green, and only tsc guards it.
  2. Non-blocking: restrict Relieves: to issues written by a writer of the repository, with a guest-authored test. A guest can currently lift the hot-file hold on one file, once, by writing the line.
Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs