Repository navigation
pin-watch: the bump step's shell parses (it never did), and an unread upstream fails it instead of passing as a clean day - #1181
Conversation
… it instead of passing as a clean day The step that opens one PR per moved repo was committed with an unterminated quote (`git commit -m "pin: $repo at $newsha""`), so `bash -n` refuses the whole step. Nothing executed it until a pin moved, which is exactly what the first scheduled run would have done: moved=1, then a syntax error, no PR, and no bump for any repo. $ bash -n <the step> # before line 23: unexpected EOF while looking for matching `"' Two more defects in the same step, both of the "clean and could-not-check look alike" kind: a bump that failed for any reason was a warning and a `continue`, so a Hub outage produced a green run with no PR; and `git switch -c` aborts the run if the branch survived an earlier failure. `--bump` now returns 4 for the two cases that are a human decision (repo gone upstream, tracked file gone) and 1 for anything else, the step skips 4 with a warning and fails on anything else, and the branch is created with -C.
… step must open one PR per moved repo The guard for the class, not just the instance: every `run:` block in .github/workflows/*.yml is lifted out of the text and run through `bash -n` (no PyYAML in CI), then the pin bump step is driven against a local bare origin with a stubbed `gh` and `python3`: one branch, one commit and one PR per moved repo, only models/pins.json in the commit, a repo that went upstream warned about and skipped, and an unread upstream failing the run. Against the pre-fix workflow it reports 9 failures; against this tree, 30 ok.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 6d24a252 — the parse bug is real, the fix parses, and the new test bites on the old workflow exactly as claimed.
The bug, reproduced from main's own bytes: .github/workflows/pin-watch.yml:95 (main) is
git commit -q -m "pin: $repo at $newsha"" — one quote too many. At this head it is :104 with the extra quote gone. So the step had never parsed, and nothing ran it because it is behind moved != 0.
The test bites — run at this head, then against origin/main's workflow with everything else unchanged:
$ python3 tests/workflow_shell_test.py # head: all checks passed
$ git show origin/main:.github/workflows/pin-watch.yml > .github/workflows/pin-watch.yml
$ python3 tests/workflow_shell_test.py # main: 9 check(s) failed
FAIL pin-watch: the pin bump step's shell parses: bash: line 23: unexpected EOF while looking for matching `"'
FAIL pin-watch: a gone repo does not fail the run: bash: -c: line 23: unexpected EOF …
FAIL pin-watch: one 'gh pr create' per moved repo: []
FAIL pin-watch: the gone repo is warned about, not bumped
FAIL pin-watch: one branch per moved repo, none for the gone one: []
FAIL pin-watch: ok/one has a pushed branch at main: []
FAIL pin-watch: ok/two has a pushed branch at main: []
FAIL pin-watch: an unread upstream fails the run instead of a green no-PR day: rc=2
So it fails for the syntax reason first and then for the behaviour, which is the right order for a test that is supposed to catch both.
The class guard covers the repo, not just this file: 14 workflow files, 14 "N run block(s) parse" lines — bump-{hrx,laya,lemonade,tokenizers,xdna}, census-watch, census, ci, hrx-pins-guard, hw-gate, issue-agent, pin-watch (3 blocks), pr-agent, sync-lemonade-fork. pin-watch is the only one that used to fail, which is what the body says.
The rc semantics are wired on both sides: the tool returns 4 for the two human-decision cases (tools/pin_watch.py:549,570 in the self-test, :625,631 in the code), the step warns and continues on 4, and exit 1s with ::error:: on anything else (pin-watch.yml:90-99). python3 tools/pin_watch.py --self-test -> 35 ok. add_test(NAME workflow_shell …) at CMakeLists.txt:557. tools/copyright.py --check rc 0.
One nit, in the same class the PR is about: git switch -C now resets the local branch, but the remote side is not reset. If a run dies mid-loop after opening a bump PR (the next repo returning a non-4 failure exit 1s the step), the re-run recreates the local branch from main and then git push -q origin "$branch" is a non-fast-forward against the stale remote branch — so the re-run fails at the push, or at gh pr create with "a PR already exists", and again every later repo goes unbumped. A git push -q -f origin "$branch" for these bot branches (or a gh pr list --head "$branch" skip before the push) would close it. Not blocking: it needs a failure to have happened first, and the PR's own test drives one repository at a time.
|
Third commit Measured on the committed pins: one
|
The docstring said --pr-body prints the body "from the report --check just wrote", and it did not: it re-ran the whole sweep. At 89 s per sweep that is one full read of all 147 repos per bump PR, so a day when a vendor re-pushes many repos gets slower the more it has to report - and hammers the Hub exactly when the run must not fail. --check --json FILE writes that same sweep (changes + notes) beside the report, and --pr-body REPO --from FILE reads it instead of re-checking. Without --from it still recomputes, so an ad hoc single call reads the same as before. The workflow passes pin-report.json to both, so a moved-repo day costs one sweep. Measured on the committed pins: --check 89 s, then --pr-body --from 0.12 s, and the body is diff-identical to the 89 s recomputation. --self-test 38/38, including a --pr-body --from call with head()/tree() replaced by a stub that raises, so a hidden re-read would fail the test. The workflow rehearsal in tests/workflow_shell_test.py now fails the step if --from or its JSON is missing.
a6aabdf to
5771c12
Compare
|
Head is now
Re-verified at this head: |
…ean pin First production run of the workflow (workflow_dispatch on main, run 42123604191): token present, checkout and the sweep worked, and the bump step died exactly where #1181 says it does (`line 23: unexpected EOF while looking for matching "'`). The sweep's first line was worth more than the crash: # Model pins: 145 of 147 checked, 2 not checked, 1 moved (2026-11-05) Two pins are invisible to the workflow's HF_TOKEN — `bong-water-water-bong/ZAYA1-8B-Q4_K_M-GGUF` and `bong-water-water-bong/ZAYA1-8B-QLoRA-GGUF` answer 401 anonymously and 200 with the box's token, i.e. they are private. A 401 is neither transient nor "gone": no token change can happen inside the run, so the honest thing is to say what it means and let a human fix the token. - `check()` reports 401/403 as "the token cannot see this repo (401) — a private or gated repo needs a token with access", still counted in the report's first line (so it can never print an all-clear over it). - the workflow annotates the run with that first line when anything was not checked: visible on the run list without failing the PR flow, which a secret-scope problem would only do every day. - docs/model-pins.md says what a 401 means and that the count is the contract. --self-test 41/41, including a 401 on one repo that must not hide the other repo's move (exit 1, first line '1 of 2 checked, 1 not checked').
|
I dispatched the workflow on
So the fix is not theoretical: without it the daily run is red at 05:43 UTC, opens nothing, and the one thing it exists for never happens. That run also showed something the fix had to cover, which is the fourth commit
Needs a human, not a patch: the |
|
CI green at Re-verdict requested. The workflow stays broken on |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at dca4ad30 — re-derived at this head, and the new work is an improvement on the same theme.
python3 tests/workflow_shell_test.py-> all checks passed (30 cases), including the new one-sweep path.python3 tools/pin_watch.py --self-test-> 41 ok (was 35), with the four new cases covering--check --json,--pr-body --from(asserting it does not read the Hub), the 401/403 "token cannot see this repo" note, and the usage error (3) for a repo that did not move.- What the push adds, all in the "clean and could-not-check must not look alike" class:
--checkalso writes the sweep as JSON, so the PR step no longer re-reads 147 repos once per bumped repo; a first line of "not checked" now emits a::warning::on the run;--pr-bodylost its|| true, so a body that cannot be produced fails instead of opening a PR with an empty description; and a private/gated repo (401/403) is named as not checked rather than passing as unchanged. - The parse fix itself is unchanged from my review:
git commit -q -m "pin: $repo at $newsha"(pin-watch.yml:104), and the pre-fix workflow still reds 9 checks when I swaporigin/main's file in.
Nit, unchanged: a re-run after a mid-loop failure still meets the stale remote branch at git push (no -f, no gh pr list --head skip). It needs a failure first, so it is not a gate.
|
Prep for the second landing: #1181 and #1193 conflict in The conflicts (two hunks, one file). Rebased either way, The defect only the union has. #1181 made Branch
I am not touching either PR's head: both are READY at the reviewed SHA. If #1193 lands first I rebase this onto |
…ut-unpinned repo is not "did not move" `--json` is what the workflow's PR step reads, and its docstring calls it "the sweep". #1181 added it when a sweep was only pins; #1193 added `--orgs`, which finds repos by a different route (we publish them and nobody pinned them). Applied together, `--check --orgs --json` wrote `date, pins, changes, notes`: the census rows survived only in the Markdown report, and the artifact the PR step consumes described half of what the run looked at — the F1/#1157 failure mode, an artifact whose prose is wider than its contents. Measured before, on the union tree: a sweep with `1 of 148 moved, 1 published but not pinned` wrote a JSON with `changes: [unsloth/Qwen3.8-Flash-Next-GGUF]` and no census key at all, and `--pr-body our-org/unpinned --from sweep.json` answered "did not move" — the one word that would send a reader looking at the wrong thing, and (with `|| true` removed by #1181) the shape of failure that opens an empty-bodied PR. Now: the JSON carries `published_not_pinned` and both `--pr-body` paths (`--from`, and the in-memory one after a `--check --orgs`) refuse a census row in one wording — "published but not pinned … needs `--pin`, not a bump PR". A sweep written before this commit has no such key and can still only say "did not move"; that is why the writer is fixed in the same change. `--self-test` 51 -> 56: the sweep runs with both flags and exits 1; the JSON holds both halves; a census row is refused in those words; and a pin that *did* move still prints its body from the same file (so the fix cannot have been "refuse everything"). `tests/workflow_shell_test.py` still passes. This commit belongs to whichever of #1181/#1193 lands *second*: it is the only change that is about the two of them meeting. Rebased onto current main either way the union tree is identical.
… until someone pins it (#1193) * pin-watch: census our own orgs, so a repo we publish is not invisible until someone pins it --check iterates over pins, so a repo that was never pinned is watched by nobody: the template audit of 2026-11-05 had to be done by hand for exactly that reason. --orgs A,B adds one listing per org (full=true carries the siblings) and reports every repo that ships a model artifact or a chat template and is not in the table, under 'Published, not pinned', in the report's first line and as a run annotation. Measured on the committed pins (19:3x): of 167 repos in the two orgs, 92 are not in models/pins.json — 89 of them hold only .1bp packs (the 1BP lane's format, not this table's business) and 2 are private (this file is public, docs/model-pins.md), so exactly one is reported: bong-water-water-bong/ZR1-1.5B-Strix. --pin now selects the files this table exists for (.gguf, .q4nx, chat_template.jinja, tokenizer_config.json) rather than .gguf alone: a container or LoRA repo has no .gguf, so it could not be pinned at all before this. --self-test 45/45, including: a published repo that ships a template is reported and an .1bp-only repo is not, a private repo stays out, an already-pinned repo is not reported twice, an org that cannot be listed is a note rather than a clean day, and --pin on a container repo pins the model plus both template files. * models/pins.json: pin bong-water-water-bong/ZR1-1.5B-Strix (the one published repo the census found) 6 files at 64f092fe: three chat_template.jinja (root and both checkpoints, all b6835114b7303ddd78919a82e4d9f7d8c26ed0d7dfc36beeb12d524f6144eab1 — the same digest the audit measured and the ZR1 GGUF arms carry) and three tokenizer_config.json that carry no chat_template key (459 B each, so null is the honest value, not a missing read). Nothing else in the file moved. * pin-watch: the sweep's JSON carries the census half, so a published-but-unpinned repo is not "did not move" `--json` is what the workflow's PR step reads, and its docstring calls it "the sweep". #1181 added it when a sweep was only pins; #1193 added `--orgs`, which finds repos by a different route (we publish them and nobody pinned them). Applied together, `--check --orgs --json` wrote `date, pins, changes, notes`: the census rows survived only in the Markdown report, and the artifact the PR step consumes described half of what the run looked at — the F1/#1157 failure mode, an artifact whose prose is wider than its contents. Measured before, on the union tree: a sweep with `1 of 148 moved, 1 published but not pinned` wrote a JSON with `changes: [unsloth/Qwen3.8-Flash-Next-GGUF]` and no census key at all, and `--pr-body our-org/unpinned --from sweep.json` answered "did not move" — the one word that would send a reader looking at the wrong thing, and (with `|| true` removed by #1181) the shape of failure that opens an empty-bodied PR. Now: the JSON carries `published_not_pinned` and both `--pr-body` paths (`--from`, and the in-memory one after a `--check --orgs`) refuse a census row in one wording — "published but not pinned … needs `--pin`, not a bump PR". A sweep written before this commit has no such key and can still only say "did not move"; that is why the writer is fixed in the same change. `--self-test` 51 -> 56: the sweep runs with both flags and exits 1; the JSON holds both halves; a census row is refused in those words; and a pin that *did* move still prints its body from the same file (so the fix cannot have been "refuse everything"). `tests/workflow_shell_test.py` still passes. This commit belongs to whichever of #1181/#1193 lands *second*: it is the only change that is about the two of them meeting. Rebased onto current main either way the union tree is identical. --------- Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
`--pr-body` for a repo that did not move ends in `sys.exit("… did not move …")`, i.e. a string, so
Python exits **1** — the header called it "a usage error (3)", and 3 is `--self-test` failing. The
header's exit paragraph also folded "--check found nothing readable" into 1 where the tool returns 2
(`main()` ends with `return 2` when every pin lands in `notes`, and the self-test asserts it), while
docs/model-pins.md said "There are three outcomes" and stopped at 2, leaving 3 and 4 — both of which
CI and the workflow branch on — undocumented.
Measured on main `1dfe2708` before the change, against the reviewer's own repro:
$ printf '{"changes":[],"notes":[],"pins":1,"published_not_pinned":[]}' > sweep.json
$ python3 tools/pin_watch.py --pr-body fake/moved --from sweep.json # -> 1
$ python3 tools/pin_watch.py # -> 2
The docs table was {0,1,2} against a header listing {0,1,2,3,4}, so the pair disagreed on the same
five codes; now both list all five, with 0 widened to "or the requested output was printed", 1 to
"or the requested --pr-body repo did not move", 2 to "usage or no pin could be read at all".
The check that keeps them together: `--self-test` now compares the header's list with the table's rows
(it fails on main's pair, verified against `git show origin/main:…`), and runs the tool in a
subprocess so the did-not-move code is asserted at the **process** level (1), not through the test's
own `SystemExit` mapping — which is why the old assertions could not have caught it. Self-test
**56 -> 60**, `tests/workflow_shell_test.py` all checks passed.
Reported by lane pi-docs in #1193's review thread; that PR merged with this loose end (it is #1181's
line, on files #1193 last owned). Reported, not filed: no issue number.
Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>
Root cause
The step that opens one PR per moved repo — the whole point of
pin-watch.yml, merged this evening as #1148 — does not parse:The line is
git commit -q -m "pin: $repo at $newsha""— one quote too many. Nothing executed that step before, because it only runs whenmoved != 0; day one reports 1 of 147 moved (unsloth/Qwen3.8-Flash-Next-GGUF), so the scheduled 05:43 UTC run would have been its first reader:moved=1, then a syntax error, then no bump PR for any repo, and the failure looks like an infrastructure hiccup rather than "everything you asked the watcher for did not happen".Second and third defects in the same step
Both are the "clean and could-not-check look alike" class this lane fixed in
--checklast round, still present in the bump path:--bumpfails because the Hub is down / rate-limitscontinue: green run, no PR, no alarm::error::): an unread upstream is not a clean day--bumpfails because the repo or a tracked file is gone upstreamcontinue(correct)--bumpreturns 4git switch -caborts the run, so every later repo goes unbumpedgit switch -C--bumptherefore returns4for the two human-decision cases (404 repo, tracked file vanished) and1for anything else; the workflow skips4with a warning and fails on anything outside0/4. Documented in the tool's docstring, and the two self-test expectations moved from1to4(35/35 ok).Reproduction of the fixed path, not just the parse
bash -nis the smallest proof; the honest one is that the step now does what it claims.tests/workflow_shell_test.py(new, wired into ctest asworkflow_shell) drives the step against a local bareorigin, a stubbedghand a stubbedpython3, and asserts one branch + one commit + onegh pr create --head <that branch>per moved repo,models/pins.jsonas the only file in the commit, a gone-upstream repo warned about and left to a human, and a broken upstream failing the run:Same test against the pre-fix workflow (
git checkout -- .github/workflows/pin-watch.yml): 9 failures, starting with the syntax one and then every behavioural claim, andrc=2on the unread case — so the test bites rather than decorating.The generic half of the test is the class guard: it lifts every
run:block out of every.github/workflows/*.ymlandbash -ns it (CI has no PyYAML, so the block is de-indented from the text — the same techniquetests/bump_workflow_report_test.pyuses for the four bump workflows). Today pin-watch is the only file it rejects; the other 14 pass.Notes
census_watch,bump_workflow_report,workflow_shellall pass.models/pins.jsonis untouched — day one is still "1 of 147 moved", and that decision stays a reviewed merge.tools/copyright.py --checkrc 0 (the new test carries the Apache header).