feat: let anyone publish a progress report - #1
Merged
Conversation
This PR lets accounts other than the maintainer produce automated progress reports, and stops a round being spent when the operator could never land one. The allowlist moves out of the caller workflow and into `.github/progress-publishers.txt` on the roadmap repository, read at the pull request's base commit so a report cannot list its own author. Adding a publisher becomes an ordinary reviewed pull request against a path CODEOWNERS assigns to the core team, rather than an edit to a workflow. `due` now refuses up front unless the operator is both listed and able to push. Neither failure was previously detected: an operator without push access ran `plan`, `facts` and the writing model before dying on the push, and one who could push but was not listed opened a pull request the merge check refuses permanently. That second case was the worse one, because an open progress pull request marks its area in flight, so it silently stopped every operator from reporting on that roadmap. In-flight marking now expires after one cadence period and names the stuck pull request instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`str.splitlines` breaks on U+2028, U+2029, U+0085 and the vertical tab and form feed; a terminal, an editor and GitHub's diff view generally do not. Left in, `# looks like one comment<U+2028>999999 attacker` reads as a single comment to the human reviewing the pull request and as a comment plus a live allowlist entry to the parser. The file's entire security value is that a core-team member read the diff, so anything making it read differently to a human than to this parser is rejected rather than normalised. Splitting is now on `\n` alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This PR removes the author allowlist entirely, accepts pull requests opened from forks, and replaces the identity check with a check on the reported window. Identity was never what made auto-merging these reports safe; the shape of the diff was. A report may touch exactly one roadmap's STATUS.md and PROGRESS.md and nothing else, the log may only grow at its end byte for byte, no pull request content is ever checked out or executed, and no write token exists until every check has passed. An allowlist added nothing to that and only excluded contributors. What the allowlist was quietly doing was limiting abuse, and one check was missing for that. Cursor continuity pinned where a report starts, but its end was free: a report could name any 40-hex string as `to_sha`, land, and leave the area's cursor at that value, then repeat from there indefinitely. Each link would burn a window that could never afterwards be reported, and each would post to Zulip. `to_sha` must now be a commit reachable from TauCeti's `docgen` branch and strictly after `from_sha`, so a bogus report costs exactly what a real one costs and reverts the same way. Reachability rather than equality with the tip, because the tip advances whenever documentation is published and equality would refuse reports that were correct when their round began. Accepting forks took three further changes. `apply` now pushes to the operator's fork when it cannot push to the canonical repository, so a contributor with no write access can publish at all; and both trigger paths in the caller workflow excluded fork heads, which would have meant a fork's build completing and the report never being considered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nner The window check bounds where a report may point but not how many may be sent. An area whose cursor is far behind the documentation branch has room in front of it -- 1312 commits, measured, for one reported long ago -- and that room can be cut into as many single-commit windows as there are commits. Each would satisfy every other check, and each would post to Zulip. An area may now be reported again only 20 hours after its last report landed, read from the base branch's history of that PROGRESS.md rather than from anything the pull request claims. That is below the planner's 24h cadence, so it never refuses a legitimate report, and it caps the announcement channel at the rate the project intends regardless of who is sending. The gate also now checks the window's from_sha, not only its to_sha, against the section it validates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The cadence limit is keyed on the roadmap, and a roadmap with no previous report is always allowed, so an actor able to invent area names could invent unlimited exempt "first reports": TauCetiRoadmap/Bogus1/, Bogus2/, each creating a directory of two files and each announcing itself to Zulip. The path pattern constrained the shape of an area name but never checked that the area was real. The directory must now already hold a README.md on the base branch, which is the same rule that defines a roadmap everywhere else in this tool. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
kim-em
changed the title
`gh api user --jq .login` prints `kim-em`, unquoted, not `"kim-em"`, so `json.loads` raised on it. That was on the fork path, which is the one an operator without push access takes: the entire reason for the change. The unit test did not catch it because its stub returned the quoted form, so the fixture encoded the same wrong assumption as the code. It now returns exactly what `gh` prints. No other call parses a `--jq` scalar as JSON; the remaining `json.loads` sites read whole documents. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An external review of the open-publishing design found eight issues, four of them serious. All were real; this closes them. A section's `prs` list was attacker-supplied metadata that the planner treated as authoritative, so a report claiming thousands of numbers would leave every later window for that roadmap looking permanently empty. The SHA window is now the only authority on what a report covers. The guard that removed protected against a pull request appearing twice after a relabel, which is cosmetic; silently unreportable history is not. A roadmap's first report had no cursor to continue from, so whoever filed it chose where that roadmap's history begins and made everything earlier unreportable for good. Bootstraps are no longer auto-merged: fourteen reviews, once, for an irreversible editorial decision. Open pull requests were used as locks without regard to who opened them, so a stranger could open and instantly close one on the deterministic branch name and stop that window from ever being published. Closed pull requests now only count as a rejection when we could have opened them. The repository-wide concurrency group was a starvation lever: GitHub discards the older pending run per group, so a trickle of events on any `progress/*` pull request evicted the queued legitimate run indefinitely. It is removed -- the compare-and-swap already permits exactly one concurrent landing. Also: the caller filtered pull requests sharing a head SHA only by state, so a retargeted or renamed one could shadow the real report; branch cleanup deleted `<base>/heads/<name>` even when the head was a fork, taking an unrelated canonical branch with it; `docgen` was consulted as a mutable ref rather than resolved once to a SHA; the window check accepted a null `advances`; and the fork was assumed to be `<login>/<name>` rather than read from the API. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Refusing every bootstrap closed the hole but broke the feature. Thirteen of the fourteen roadmaps have never been reported, so a blanket refusal would have left automation with one roadmap to work on, while the worker regenerated a report for each of the other thirteen, had it refused, and repeated the next day once the stale-PR window released the area. The collector now computes the one legitimate starting point instead, by the same rule the planner uses: the first parent of the merge commit of the area's earliest merged pull request carrying its roadmap label. Three requests, and only for a first report, so at most once per roadmap ever. The gate requires it exactly. Checked against the two implementations agreeing, which is the property that matters -- if they disagreed, every bootstrap would be refused. The planner walks a local clone with --first-parent; the collector uses only the API. They return the same SHA for PDE (4eba9f5), Exchangeability (c484d4c) and ReductiveGroups (1f1d752), the last matching the bootstrap of the report that has already landed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Second review round: six of eight findings were fixed, two partly. This closes the remainder. Open pull requests were still authoritative locks regardless of who opened them. Branch names are a pure function of the window, so anyone could open one `progress/**/<Area>` pull request a day and freeze that roadmap forever; the staleness expiry bounds a single stuck pull request, not a stream of them. Both the planner and `apply` now count only pull requests we could have opened -- the roadmap organisation's, or this operator's own. A stranger's report still merges on its own merits; it just no longer stops anyone else writing one. Two operators on separate forks may now duplicate a window and waste a round, which is much cheaper than being unable to report at all. A MERGED pull request from anyone still counts, because then the window genuinely is published. The fork was identified by name, so a repository that merely shared the name could have been pushed to; it is now identified by `.parent.full_name`, and the listing is paginated rather than capped at the first hundred. Refusals claimed to be posted on the pull request but were only printed to the workflow log, and the token was read-only. Now that anyone may publish, a contributor whose report is refused would have had no feedback at all, so the comment is posted (best effort, since a refusal is a normal outcome and must not turn the run red). Also drops a caller comment that still described the concurrency group removed in the previous commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Raising `pull-requests` to write at workflow level handed it to the merge job too, which does not need it: everything that job writes uses the App token. It is now scoped to the `validate` job alone. The caller also has to grant it, because a called workflow can never exceed what the caller allows -- so the previous commit's comment would in fact have been refused for want of permission, having only just been given the code to post it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…he API Third review round. `merge_commit_sha` means different things for different merge methods, and a pull request merged into some other branch need not touch this history at all, so deriving a roadmap's starting point from it without checking could have skipped earlier history -- silently, and once per roadmap forever. The merge commit must now be reachable from the documentation branch, the cursor derived from it must be too, and the cursor must come strictly before that merge. Anything else returns nothing, the gate refuses the report, and a human bootstraps that roadmap: the right outcome for a once-per-roadmap decision that could not be checked. Verified on all fourteen roadmaps: the collector, which uses only the API, and the planner, which walks a local clone with --first-parent, agree exactly. Also: an unreadable `/user` put "" in the set of owners whose pull requests mark an area in flight, which is precisely what a deleted fork's `headRepositoryOwner` reads as, so those could block. Only non-empty logins are trusted now. And the caller granted pull-request write to its whole workflow rather than to the one job that calls the gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…est number Fourth review round, and the sharpest finding yet: both implementations picked the lowest-numbered labelled pull request as a roadmap's starting point, and numbers are assigned when a pull request is opened, not when it merges. If #100 opens before #101 but merges after it, the commit before #100's merge sits past #101 -- and windows only move forward, so #101 could never be reported. Two of the fourteen roadmaps have exactly that shape today: RepresentationTheory (#1227 opened first, #1228 merged first) and OneParameterSemigroups (#273 vs #276). Both would have silently dropped real work on their first report. Worth recording how this survived three rounds. The two implementations agreed across all fourteen roadmaps, and that agreement was offered as evidence they were right. It was not: they agreed because they shared the same wrong assumption. Agreement between implementations only means something when they derive the answer differently, which is now the case -- the planner walks `git log --first-parent` and takes the last labelled subject it sees, while the collector orders by `mergedAt` through the API. They still agree on all fourteen, and the two affected roadmaps now resolve to different cursors than before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The bootstrap stub still answered a `search/issues` call that no longer happens, so the test failed. It now returns pull requests whose lowest number is NOT the earliest merge, and a second test asserts the earliest-merged one is the one followed up -- which is the property the change exists to establish, rather than merely the code path running. Committed alongside a failing test a moment ago: the shell chain treated a match on "FAIL" as success, so the guard I thought I had was inverted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…re it starts Fifth review round. Two earlier attempts tried to recompute the planner's starting point inside the collector -- first by lowest pull request number, then by merge time -- and both were wrong for the same underlying reason: what the planner takes is a position on the documentation branch's first-parent chain, and the API cannot express first-parent membership at all. This history is not linear (14 merge commits, 21 commits off the chain), so a commit can be reachable through a second parent without lying on it, and no amount of ancestry checking distinguishes the two. So stop reproducing and check the property that was wanted all along: a starting point is acceptable exactly when nothing labelled for that roadmap merged at or before it. That is indifferent to merge method and topology and cannot be fooled by a second-parent path. Verified against live history: all fourteen roadmaps accept the planner's cursor with nothing stranded, and the cursor the original lowest-number rule produced for OneParameterSemigroups is rejected, naming the 22 pull requests it would have skipped. Building it surfaced a further bug in the check itself, which the same live comparison caught: labelled pull requests merged AFTER the documented tip are not stranded, they are merely not documented yet. Counting them would have refused every bootstrap whenever documentation lagged, which it did for four pull requests on RepresentationTheory. Only work already in the documentation branch counts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sixth review round, and the third consecutive one to find the bootstrap check wrong. The reason was the same every time and it is not fixable here: deciding whether a proposed starting point strands labelled work is a question about the first-parent chain, and the REST API expresses neither first-parent traversal nor any ordering guarantee. This history is not linear, so ancestry checks cannot recover it either. Three implementations tried. By lowest pull request number: wrong, because numbers are assigned when a pull request opens, not when it merges. By merge timestamp: wrong, because timestamps bear no relation to position on the chain. By walking the commits endpoint: wrong, because that walk is neither ordered nor first-parent-limited, so "seen before from_sha" is not an ancestry boundary. So the check is gone, and with it the pretence. A first report is refused and left for human review; the generator still writes it, and every report after it for that roadmap merges unattended. Fourteen reviews, once, in exchange for not shipping a security check that does not work. The planner no longer selects a never-reported roadmap on its own, since that would produce a report daily and have it refused daily. Naming the area is how a person asks for its first report. Net effect on the diff: 87 lines of collector removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Kevin Buzzard, whose request prompted this work, read the first published report and said it could have been three times shorter if it was meant to be human-readable. He is right, and he is the reader it is for. The report ran to 932 words. What made it long was enumeration: one paragraph walks through fifteen pull requests inline, which is a changelog with paragraph breaks. The prompt said "group by mathematical content, not by pull request" but then allowed "two to five paragraphs" with no budget, and a model asked for paragraphs will fill them. So the prompt now asks for about 300 words in at most three paragraphs, says plainly that length is not thoroughness, and shows the enumeration it must not produce beside the sentence that should replace it. The trailing index of pull request numbers is gone: it helps nobody read, and the log already links to the full record. A request in a prompt drifts, so there is also a check. `MAX_SECTION_WORDS` is 450 -- headroom over the target, far under a catalogue -- and it is enforced in `validate_update`, which means `apply` refuses on the worker before opening a pull request rather than the gate refusing one that already exists. The published report would not pass it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There were two `prompts/progress.md`. The worker read its own copy; the one in this repository was read by nothing, and the two had drifted. A fix for report length was very nearly made to the dead one, and would have changed nothing about what any model is told. The prompts now live inside the package and are served by `tauceti-progress prompt <name>`, so the words a model is given and the checks its output must pass are one versioned thing, pinned by the same SHA. The worker fetches the prompt from the build it already runs for `plan`, `facts` and `apply`, so this adds no new failure mode. Inside the package rather than beside it because `package-data` pointed at `../prompts/*.md`, which setuptools does not reliably ship from outside the package directory: a prompt that does not install is a prompt that does not exist. Verified by installing the built wheel and reading the prompt back out of it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR removes the author allowlist entirely, accepts pull requests opened from forks, and replaces the identity check with three checks on the content of a report.
Identity was never what made auto-merging these reports safe; the shape of the diff was. A report may touch exactly one roadmap's
STATUS.mdandPROGRESS.mdand nothing else, the log may only grow at its end byte for byte, no pull request content is ever checked out or executed, and no write token exists until every check has passed. An allowlist added nothing to that and only excluded contributors.What an allowlist was quietly doing was limiting abuse, and the shape checks had three gaps that only matter once anyone can publish.
to_shawas unconstrained. Cursor continuity pinned where a report starts, but its end was free: a report could name any 40-hex string, land, and leave the area's cursor at that value, then repeat from there. Each link would burn a window that could never afterwards be reported, and each would post to Zulip.to_shamust now be a commit reachable from TauCeti'sdocgenbranch and strictly afterfrom_sha. Reachability rather than equality with the tip, because the tip advances whenever documentation is published and equality would refuse reports that were correct when their round began.Nothing bounded how many reports could be sent. An area whose cursor is far behind the documentation branch has room in front of it, measured at 1312 commits for one reported long ago, and that room can be cut into as many single-commit windows as there are commits. An area may now be reported again only 20 hours after its last report landed, read from the base branch's history. That is below the planner's 24h cadence, so it never refuses a legitimate report.
Roadmap directories were a regex, not a fact. The cadence limit is keyed on the area and a first report is always allowed, so inventing area names gave unlimited exempt first reports. The directory must now already hold a
README.mdon the base branch.Accepting forks took three further changes.
applypushes to the operator's fork when it cannot push to the canonical repository, so a contributor with no write access can publish at all; both trigger paths in the caller workflow excluded fork heads, which would have meant a fork's build completing and the report never being considered; and the resolver now picks deterministically among pull requests sharing a head SHA.Separately, an open progress pull request marked its area in flight forever. One the gate refuses permanently never merges and never closes itself, so a single stuck report silently stopped every operator from reporting on that roadmap. In-flight marking now expires after one cadence period and names the stuck pull request.
The first two commits add an allowlist that the third removes. That is the history of the decision rather than an accident; the net result is less code than before it existed.
187 tests, up from 156.
🤖 Prepared with Claude Code