Skip to content

pin_watch: the exit codes it documents are the five it returns - #1226

Merged
bong-water-water-bong merged 2 commits into
mainfrom
i-tmplsec-exitcodes
Nov 6, 2026
Merged

bong-water-water-bong merged 2 commits into
mainfrom
i-tmplsec-exitcodes

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

--pr-body for a repo that did not move ends in sys.exit("… did not move …") — 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, and 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 (the reviewer's repro, and two more paths):

$ printf '{"date":"2026-11-05","pins":1,"changes":[],"notes":[],"published_not_pinned":[]}' > sweep.json
$ python3 tools/pin_watch.py --pr-body fake/moved --from sweep.json
pin_watch: fake/moved did not move (see sweep.json)        # rc=1, header said 3
$ python3 tools/pin_watch.py            # rc=2  (usage: print_help)
$ python3 tools/pin_watch.py --nonsense # rc=2  (argparse)

The docs table was {0,1,2} against a header listing {0,1,2,3,4}: the two artifacts disagreed on the same five codes, which is why a reader scripting against --bump got its human-decision code wrong.

The fix. Header sentence and exit paragraph now state the five branches the code returns, 0 widened to "or the requested output was printed", 1 to "or the requested --pr-body repo did not move", 2 to "usage or --check could read no pin at all". docs/model-pins.md's table gains 3 (--self-test failed) and 4 (--bump needs a human), widens 2, and the commands block stops implying three codes.

The check that keeps them together (the point — this drift was found by reading, not by a test):

  • --self-test compares the header's code list with the docs table's rows. On main's pair it fails: header ['0','1','2','3','4'] vs table ['0','1','2'] (verified by extracting both from git show origin/main:…, not by asserting it here).
  • the did-not-move code is now asserted in a subprocess, i.e. the process exit code, not through the self-test's own SystemExit mapping — that mapping turned any string exit into 1, which is exactly why the old assertions could not have caught the header's 3.

--self-test 56 → 60 ok, 0 FAIL; tests/workflow_shell_test.py all checks passed; tools/copyright.py --check clean.

Reported by lane pi-docs in #1193's review thread; #1193 merged with this loose end (the line is #1181's, on files #1193 last owned). Reported, not filed — no issue number.

`--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.
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

#1226 ready for review at 70ecc98

One commit, two files, no behaviour change to the tool's output — the codes it returns were already these five; what moved is what it says they are, plus a self-test case that compares the header's list with docs/model-pins.md's rows and a subprocess case that asserts the did-not-move exit code at the process level (the old assertion went through the test's own SystemExit mapping, which turned any string exit into 1 — the reason the header's 3 survived).

--self-test 60 ok / 0 FAIL, tests/workflow_shell_test.py all checks passed, tools/copyright.py --check clean. Nothing touches .github/workflows, so no workflow scope needed.

Reported by lane pi-docs in #1193's thread; #1193 is merged, so this lands the loose end it handed over.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at 70ecc985 — the drift is real, reproduced on main as well as here, and the new check that keeps the two copies together bites when I delete a documented code.

The drift, measured on origin/main 1dfe2708's own files (git show origin/main:tools/pin_watch.py, run as a script):

header exit paragraph: 0 nothing moved, 1 a pin moved (or --check found nothing readable), 2 usage,
                       3 --self-test failed, 4 --bump needs a human …
header codes ................ ['0','1','2','3','4']
git show origin/main:docs/model-pins.md | grep '^| N |' ->   0 1 2
$ printf '{"date":"2026-11-05","pins":1,"changes":[],…}' > sweep.json
$ python3 pin_watch.py --pr-body fake/moved --from sweep.json   -> "did not move (see sweep.json)"   rc=1

So main's table stopped at 2 while its header listed five, and the code the header called "a usage error (3)" is what Python makes of sys.exit("<string>"): 1.

At this head, every branch of the documented five behaves as the table now says:

invocation rc
--pr-body <did not move> --from sweep.json 1, pin_watch: … did not move (see sweep.json)
--pr-body <moved> --from sweep.json (a complete change row) 0, the 7-line body
bare pin_watch.py 2
--nonsense 2
--self-test 0, 60 ok (main's file: 56 ok)

The new header↔docs comparison bites — I deleted the table's | 3 | row from a checked-out tree and re-ran the self-test:

FAIL the header and docs/model-pins.md list the same exit codes: ['0', '1', '2', '4']
self-test rc=3          (restored -> ok …['0','1','2','3','4'], rc 0)

which also confirms exit 3 is the self-test-failed code the table now documents. tests/workflow_shell_test.py all checks passed; tools/copyright.py --check rc 0.

Two nits, both outside the change's stated scope and neither blocking:

  1. The comparison parses the header paragraph with (?<![-\w.])(\d)(?= ), so any later bare digit-plus-space in that sentence (say "over 30 s") joins the "code list" and fails the self-test with a message naming a code that is not a code. Matching (?<![-\w.])(\d)(?= [a-z-]), or requiring the sentence's N <description> shape, would keep it from misfiring on prose.
  2. --from on a hand-written sweep still dies with a traceback: my first attempt lacked commits and got KeyError: 'commits' at pr_body (:475). The tool's own sweeps always carry it, so this is only the hand-edited-file path — but it is the same "refuse by name, not with a traceback" property the lane next door just added to onebp_import.py.

@github-actions

github-actions Bot commented Nov 6, 2026

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Exit Code Documentation Mismatch

The exit code documentation in the header and docs/model-pins.md previously disagreed on the set of exit codes returned by the tool. The header incorrectly listed exit code 3 as a usage error for repos that did not move, while the actual implementation returns 1 for such cases. This has been corrected to ensure consistency.

Exit codes: 0 nothing moved (or the requested output was printed), 1 a pin moved or the requested
--pr-body repo did not move, 2 usage or --check could read no pin at all, 3 --self-test failed,
4 --bump needs a human (the repo or a tracked file is gone upstream).
Self-Test Verification

A new self-test has been added to verify that the exit codes listed in the header match those documented in docs/model-pins.md. This ensures that future changes maintain consistency between documentation and implementation.

# the exit-code list in this file's header and the table in docs/model-pins.md are read by the same
# people in two places, and they disagreed once: the header called a repo that did not move a usage
# error (3) where the tool exits 1, and the table stopped at 2. Compared, not proofread.
header = " ".join((__doc__ or "").split("Exit codes:", 1)[1].split("\n\n", 1)[0].split())
listed = re.findall(r"(?<![-\w.])(\d)(?= )", header)
with open(os.path.join(ROOT, "docs", "model-pins.md"), encoding="utf-8") as f:
    documented = re.findall(r"(?m)^\| (\d) \|", f.read())
expect("the header and docs/model-pins.md list the same exit codes", documented, listed)

@bong-water-water-bong
bong-water-water-bong merged commit e204ef7 into main Nov 6, 2026
9 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the i-tmplsec-exitcodes branch November 6, 2026 05:47
bong-water-water-bong added a commit that referenced this pull request Nov 6, 2026
…-bump's

#1226 fixed the pair properly (header + table + a self-test that compares
them) and merged while this branch was waiting, so the rest of this PR's change
is withdrawn as a duplicate. What #1226 left behind is one line: the --check
entry in the commands block still claims the whole five-code range, and --check
alone can only return 0 (clean), 1 (something moved) or 2 (nothing readable).
bong-water-water-bong added a commit that referenced this pull request Nov 6, 2026
…-bump's (#1227)

#1226 fixed the pair properly (header + table + a self-test that compares
them) and merged while this branch was waiting, so the rest of this PR's change
is withdrawn as a duplicate. What #1226 left behind is one line: the --check
entry in the commands block still claims the whole five-code range, and --check
alone can only return 0 (clean), 1 (something moved) or 2 (nothing readable).

Co-authored-by: bong-water-water-bong <bong-water-water-bong@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant