fix(vendor): make main() exit non-zero when any crawler target raises (#872)

[JarryShaw]

Please follow the guide below

• Searched for similar pull requests
• Followed the coding style (make pylint, make mypy, make isort)
• make test passes, and a test case covers the change
• Added a changelog entry under docs/source/changelog/ and regenerated CHANGELOG.md, if the change is user-visible — N/A, changelog centralised in docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657

What is the purpose of your pull request?

• fix — corrects a defect

---

Description of your pull request and other information

Fixes #872. python -m pcapkit.vendor <target> could not fail: run() swallowed every crawler exception as a filterable VendorRuntimeWarning, and main() always return 0. This grew into three pieces after review, following the owner's ruling (quoted verbatim):

I take (b). And only discard changes made by a non-zero sub-vendor. zero-exited sub-vendor's updates should be kept. So the idea is for cron jobs, at most warn about failed sub-vendors, but never fail the entire job.

1. Exit code (pcapkit/vendor/__main__.py) — run() returns whether its target succeeded; main() attempts every target regardless of earlier failures and returns 1 if any raised. A raising target's qualified name and exception repr also reach stderr unconditionally via print(), since the warning is filterable and PCAPKIT_VERBOSE defaults false. This exit code stays load-bearing for every consumer — including a manual run — and is unchanged from earlier rounds.

2. Atomic write (pcapkit/vendor/default.py) — Vendor.__init__'s previous last line, open(const_file, 'w'), truncates the file immediately, before any content is written. A failure during the write itself (not just fetch or render, which already left the file untouched) could therefore destroy a good file and leave a truncated one to be committed. _write_atomic() now renders to a temp file in the same directory and os.replace()s onto the target — atomic on the same filesystem — so every failure mode leaves the previous file bit-for-bit untouched.

On per-target restore: I concluded the workflow does not need a git checkout -- <path> step per failed target, and did not add one. With the atomic write, a failed target's own const file is never touched at all — not modified, not truncated — so git diff/git status shows no change for that path in the first place; there is nothing to discard. Consequently I also did not add a formal, machine-parseable failed-target contract to the CLI: the workflow's grep ' failed: ' against the existing human-readable stderr line (added in round 1) is best-effort only, used to enrich the $GITHUB_STEP_SUMMARY/::warning:: text — if the format ever drifts, the workflow falls back to a generic "see the step log" annotation rather than depending on the grep for correctness.

3. Workflow (.github/workflows/cron-vendor.yml) — the "Update Vendor" step no longer aborts under bash -e when pcapkit-vendor exits non-zero: the exit code is captured explicitly (set +e / set -e around the one command, not a bare || true), so targets that succeeded still get isorted, diffed, committed and pushed. Failures are surfaced loudly: the full stderr is replayed into the step log, a ### :warning: block naming the failed target(s) is appended to $GITHUB_STEP_SUMMARY, and a ::warning:: annotation is emitted per failed target — visible on the run without opening the log. Verified directly against stub pcapkit-vendor/isort executables in a scratch sandbox: multi-failure, clean success, and a non-zero exit with no parseable target line all behave as intended, and the step's own exit code is always 0 so later steps proceed.

Tests: tests/vendor/test_vendor_exit_code_unit.py (5 methods) and tests/vendor/test_vendor_atomic_write_unit.py (2 methods), both through the real entrypoints with stub Vendor subclasses — no network calls. The atomic-write regression method simulates a failure during the write by making print() raise once the destination is already open; run against pre-fix default.py it fails (the previous file is truncated to ''), and passes with the fix. 7/7 methods green together under plain unittest.

[JarryShaw]

NEEDS CHANGES on 58f883c83 — cross-review (opus; author sonnet). The production change in pcapkit/vendor/__main__.py is correct as written and I found no defect in it. One test is vacuous, and one consequence needs your ruling.

1. The test guarding the new print() passes without the fix, so nothing pins it. test_failing_target_and_error_reach_stderr_unconditionally never suppresses warnings, and the pre-existing warn() already writes the qualified name and the exception repr to stderr under default filters. Measured:

default filters   -> stderr non-empty: True   contains repr: True
warnings ignored  -> stderr non-empty: False

So delete the print() line and that test still passes — the whole premise of its own comment ("the only thing guaranteed to reach a CI log regardless of warning filters") is untested. Wrap the call in warnings.simplefilter('ignore'); that takes the pre-fix failure count from 3/5 to 4/5. Also prefer assertEqual(exit_code, 1) over assertNotEqual(..., 0), since main()'s docstring promises exactly 1.

2. Needs a decision from you — this changes what a broken crawler costs. GitHub Actions runs run: under bash -e, so a non-zero pcapkit-vendor at cron-vendor.yml:123 aborts the Update Vendor step before the isort at :132, and Verify Changed files (:134), Bump Version, Commit changes and Push changes all then skip. Net effect the first time any single registry goes down: the other ~116 crawlers regenerate to disk and every one of them is thrown away — nothing isorted, committed or pushed. Pre-fix, those 116 landed. That defeats in CI the very intent the commit message states in-process ("a dead crawler should not block the rest from regenerating").

Two ways to go, and it is your call: (a) accept it — a red cron and no partial commits until a human fixes the crawler; or (b) defer the exit in the workflow so the good crawls still land — pcapkit-vendor || rc=$? … exit $rc after the commit steps, or continue-on-error plus an explicit final gate. If (b), it belongs in this same change.

Good news on the risk: nothing is broken today. The latest successful cron run (36423035362, 2026-09-28T12:37Z) has zero VendorRuntimeWarning/VendorRequestWarning lines across all ~117 crawlers — and since warn() demonstrably reaches stderr under default filters, absence in the log is real evidence, not the vacuous "it was green" (pre-fix it could not go red).

Verified and not disputed: sys is imported unconditionally at __main__.py:14, so the new print() in the exception handler cannot NameError; exit codes are 0 all-succeed / 1 any-fail, in both orders, with the succeeding target still running; main() consumes falsiness so a None return fails safe; PCAPKIT_CI_MODE raises rather than prompting, and PCAPKIT_VENDOR_RETRY=10 only widens retries, so neither masks the new exit code; 5 methods, 3 failing pre-fix exactly as claimed, stubs touch no network.

Non-blocking omission for a follow-up: a mixed list like pcapkit-vendor arp typo still exits 0 — a renamed vendor module is warned about and skipped, which is #872's defect one layer up.

[JarryShaw]

NEEDS CHANGES on 54639e181 — cross-review round 2 (opus; author sonnet). The fix round 2 was asked for is done and verified; what is left is a two-line comment that now contradicts both the measured output and its own module docstring.

Round 1's defect is repaired and now genuinely pinned. With origin/main's pcapkit/vendor/__main__.py swapped in, the new file fails 4 of 5 methods — exactly the four claimed, and the stderr one fails with '…FailingCrawler' not found in '', i.e. the buffer is genuinely empty pre-fix. Restoring gives 5/5. Stronger still: deleting only the print() line (120→119 lines, both return statements intact) fails exactly one test, that one; and with the suppression removed but print() still absent, the buffer fills with 172 bytes of VendorRuntimeWarning and the assertion passes — which is precisely why the suppression had to be added. The reviewer also ruled out a second stderr path: pcapkit/utilities/logging.py attaches only a NullHandler by default and its module-level StreamHandler binds the real stderr at import, so it cannot forge a pass into the buffer.

The remaining item — tests/vendor/test_vendor_exit_code_unit.py:140-142:

# The regression this issue is about: pre-fix, main() ends on a bare
# `return 0` with no accounting at all, so this assertion is the one
# that fails (0 != 0) against origin/main.

Both specifics are stale residue of round 2's own edit. (0 != 0) was the failure text of the old assertNotEqual(exit_code, 0); the assertion is now assertEqual(exit_code, 1) and the measured text is 0 != 1. And "this assertion is the one that fails", singular, is the very claim round 2 was dispatched to correct — the module docstring 100 lines above now correctly says 4 of the 5 methods fail, so the file contradicts itself. One line fixes it.

Verified and not disputed: the round-2 diff (58f883c83..54639e181) contains only the three requested changes plus import warnings — pcapkit/vendor/__main__.py and .github/workflows/cron-vendor.yml are untouched, which I confirmed independently by an empty restricted diff; assertNotEqual(exit_code, …) appears nowhere in the file any more; the module docstring's four names and per-method reasons match the measurements exactly; no live crawl was run and pcapkit/const/ stayed clean across every suite run.

Still open and not mine to settle: the bash -e consequence — one dead registry aborts Update Vendor before the isort at cron-vendor.yml:132, so the week's ~116 good regenerations are discarded rather than committed. That is the needs: decision on this PR.

[JarryShaw]

GOOD TO GO on 08d0fea45 — round 3 is the one-line comment fix and nothing else, which I verified completely rather than sending another review round.

git diff 54639e181 08d0fea45 is +3/−2 in one file, entirely inside the comment at tests/vendor/test_vendor_exit_code_unit.py:139-142, and the file is token-identical to the reviewed head with comments dropped and strings collapsed (534 vs 534). So nothing executable moved from the head that round 2's review already cleared, and the previous verdict's sole outstanding item is closed: the comment now says (0 != 1), matching the measured failure text, and points at the module docstring for the full 4-of-5 list instead of re-asserting a singular failure.

The record for this PR, carried forward: the production change in pcapkit/vendor/__main__.py is correct — run() returns bool, main() attempts every target and returns 0 all-succeed / 1 any-fail, with an unconditional print(..., file=sys.stderr) beside the still-filterable warn and traceback.print_exc() left gated on VERBOSE. sys is imported unconditionally at :14, so the new print cannot NameError inside the handler. The test genuinely pins it: 4 of 5 methods fail against origin/main, deleting only the print() line fails exactly the stderr test, and removing the suppression makes that assertion pass with the print absent. PCAPKIT_CI_MODE raises rather than prompting and PCAPKIT_VENDOR_RETRY=10 only widens retries, so neither masks the new exit code.

This verdict covers the change as it stands. It does not settle the scope question still open on this PR: under bash -e a non-zero pcapkit-vendor aborts Update Vendor before the isort at .github/workflows/cron-vendor.yml:132, so Verify Changed files, Bump Version, Commit changes and Push changes all skip and a week's ~116 good regenerations are discarded rather than committed. If you want the red build and the good crawls landed, the workflow tweak belongs in this same PR and I will have it added before you merge. needs: decision stays on until you rule.

Non-blocking, for a follow-up issue rather than this PR: a mixed target list (pcapkit-vendor arp typo) still exits 0, since a bogus entry is only warned about — #872's defect one layer up.

[JarryShaw]

What needs my decision?

[JarryShaw]

What needs my decision?

Whether this PR should also change .github/workflows/cron-vendor.yml, because the exit-code fix changes what one dead registry costs you.

The code itself is settled — review: good-to-go on 08d0fea45, 53 green with 5 still running. The open question is scope.

Today pcapkit-vendor cannot fail, so a broken crawler is a silent no-op and the other ~116 crawlers still regenerate, get isorted, committed and pushed. After this merge it exits 1, and GitHub Actions runs run: under bash -e, so that non-zero aborts the Update Vendor step at :123 before the isort at :132 — and Verify Changed files (:134), Bump Version (:138), Commit changes (:148) and Push changes all then skip. Net: the week's ~116 good regenerations are computed and thrown away instead of landing.

Two options:

• (a) Accept it. Red cron, nothing committed until a human fixes the crawler. No further change; merge as is.
• (b) Defer the exit so the good crawls still land. Capture the status and exit at the end of the job — pcapkit-vendor || rc=$? in the step, then a final exit ${rc:-0} after the commit/push steps (or continue-on-error on that step plus an explicit gate at the end). Red cron and the 116 land. If you pick this, it belongs in this same PR and I will have it added and re-reviewed before you merge.

For what it is worth: nothing is broken right now. Cron run 36423035362 (12:37Z) has zero VendorRuntimeWarning/VendorRequestWarning lines across all ~117 crawlers, and since warn() demonstrably reaches stderr under default filters, that absence is real evidence rather than "the run was green" — pre-fix the job could not go red at all.

My recommendation is (b): the whole point of main() attempting every target is that one dead registry should not cost the rest, and under set -e option (a) undoes that at the only place it matters.

[JarryShaw]

I take (b). And only discard changes made by a non-zero sub-vendor. zero-exited sub-vendor's updates should be kept. So the idea is for cron jobs, at most earn about failed sub-vendors, but never fail the entire job.

[JarryShaw]

Recorded, verbatim:

I take (b). And only discard changes made by a non-zero sub-vendor. zero-exited sub-vendor's updates should be kept. So the idea is for cron jobs, at most earn about failed sub-vendors, but never fail the entire job.

So the target shape is per-target granularity, and a cron that warns rather than fails — which is more than option (b) as I framed it. Three requirements, and I have relayed them:

1. A failed sub-vendor's changes are discarded; a zero-exited one's are kept. In the common case there is nothing to discard — Vendor.__init__ (pcapkit/vendor/default.py:408+) does _request() and context(data) before the open(const_file, 'w') on its last line, so a fetch or render failure leaves the previous const file untouched. The gap is a failure during the write, which can truncate a good file; making the write atomic (temp file + rename) closes it, and that is now in scope even though python -m pcapkit.vendor exits 0 when a crawler raises, so a no-op regeneration looks like success #872 originally excluded default.py.
2. The cron never fails the whole job — it warns. .github/workflows/cron-vendor.yml:123 runs bare pcapkit-vendor under bash -e, so today's non-zero would abort the step before the isort at :132 and skip Verify Changed files, Bump Version, Commit changes and Push changes. That step must tolerate the non-zero, surface the failed targets loudly (job summary and/or an annotation), and let the successful regenerations commit and push.
3. The exit code stays load-bearing for everyone else. main() keeps returning 1 when any target raised — that is what makes a manual run and any other consumer able to tell a no-op from a real regeneration. Only the cron chooses to tolerate it.

needs: decision removed. Round 4 is with the author; the PR stays review: needs-changes until the new head is reviewed, since the scope now includes a workflow change and possibly default.py.

[JarryShaw]

NEEDS CHANGES on a0748fb17 — cross-review (opus; author sonnet). The mechanism holds: set +e/vendor_exit=$?/set -e really does survive GHA's bash -e (step exit 0 on vendor exit 1, 0, 3 and 127; the step's status becomes isort's, as intended), shellcheck finds nothing new, no crawler overrides the write path, and the atomic-write pin fails on main with '' != 'GOOD CONTENT\n'. Three things to fix.

1. cat "$RUNNER_TEMP/vendor-stderr.log" >&2 at :140 is unguarded and sits after set -e is restored. It is the one command in the new block that can abort the step — exactly the property this PR exists to establish. || true.

2. The _write_atomic permission comment is false as written. I measured it rather than taking the report: open(p,'w') ignores mode on an existing file, and creates with 0o666 & ~umask on a new one.

case	old open(w)	new atomic
existing file at 0600	0o600	0o644
new file at umask 077	0o600	0o644

Only true at umask 022. Practical impact here is near zero (git records just the exec bit, CI is umask 022), so fix the comment or os.stat the destination's mode — but do not leave the claim standing.

3. assertFalse(self.vendor_main.run(failing)) passes against pre-fix code, since pre-fix run() returns None. The module docstring at :48-51 says it is kept "for the property it does pin post-fix (an explicit False, not a merely-falsy value)" — assertFalse cannot check that. assertIs(..., False).

Cosmetic, and I would take the second: grep ' failed: ' also matches the VendorRuntimeWarning line and any exception whose text contains failed: , so one target can emit two annotations — anchor it. And mkstemp's fd leaks if os.chmod or os.fdopen raises; let os.fdopen own it immediately.

Confirmed and closed out: per-target restore is genuinely unnecessary. PCAPKIT_VENDOR_RETRY is request-level only (default.py:39 used solely inside _request's fetch loop at :589-623), so a retry never follows a write — there is no window where a target both replaces its const file and then fails.

[JarryShaw]

NEEDS CHANGES on 16a73b49f — cross-review (opus; author sonnet). The snapshot/restore design is sound and every claim about shipped behaviour checks out. What fails is the pin.

1. Deleting the raise leaves the suite fully green. I reproduced this myself — removed the raise from _snapshot_and_restore's except BaseException: and ran both files:

TREE …/pcapkit/vendor/__main__.py  MUTANT: True
run=8 failures=0 errors=0

Two other mutations are caught (deleting os.replace and removing the with from run() both fail the byte-for-byte pin), but this one is not, and the regression it hides is precisely what #872 exists to prevent — a target no longer reports its own error:

head    -> run()=False | __main__.StubCrawler failed: RuntimeError('simulated failure')
mutant  -> run()=False | __main__.StubCrawler failed: FileNotFoundError(2, 'No such file or directory')

assertFalse(run(...)) still passes by accident: with raise gone, control falls through to os.remove(backup), the backup has just been renamed away, so FileNotFoundError propagates and the target fails anyway. Fix: assert on what run() reported, not merely that it returned falsy — assertWarnsRegex(VendorRuntimeWarning, 'simulated failure') or a stderr capture requiring the crawler's own text. And make it structural while you are there: else: os.remove(backup) rather than relying on raise to skip the cleanup, since the comment at __main__.py:106-109 is currently load-bearing prose.

2. There is a second capability regression, and only one is disclosed. A writable file inside a non-writable directory: pre-fix regenerated it, this head fails the target because mkstemp cannot create the backup (PermissionError(13), content preserved). Fail-safe, and rounds 4-8 needed directory write too — but disclose both, and note the asymmetry that a mkstemp failure fails the target while a _dest_path failure does not.

Confirmed clean: the @classmethod conversion is safe — _dest_path's old body used self exactly once as inspect.getfile(type(self)), no crawler overrides it, and class-call equals instance-call for 121/121 crawlers; test_vendor_dest_path_unit.py is 4/0/0. The backup is always in the destination's own directory (SAME DIR = True, same st_dev), so my EXDEV worry was structurally impossible. Mode restoration works — 0o640 in, 0o640 out, content intact, mtime preserved, which does justify collapsing the three mode branches, though nothing now asserts it. Workflow byte-identical (600fd5b1f), sandbox still 0 / 0 / 1-when-isort-fails. run=3 / run=5 / run=8 all reproduce, and the pre-fix run fails 2 methods, not 1 — test_successful_run_replaces_the_previous_file_content also fails with None is not true, which you undersold.

Cosmetic: add the 0o640 mode assertion to the byte-for-byte test; reword "there is nothing left to clean up on this path either way" (true only when os.replace succeeds — a restore failure masks the original error and orphans the .bak); the symlink divergence moved to the restore path and round 9 dropped the discussion rounds 4-8 had; and os.remove(backup) on the success path is unguarded where rounds 4-8 used contextlib.suppress(OSError), which is also the owner's stated preference.

[JarryShaw]

GOOD TO GO on 89e5996c0 — cross-review (opus; author sonnet). The round-9 blocker is properly fixed, all three of your mutation numbers reproduce exactly, both capability regressions and the symlink divergence match measurement clause for clause, and the workflow is provably untouched (600fd5b1f at both shas).

The else: is justified by a stronger version of your own argument. Deleting raise does not merely return the wrong value — it is totally silent: contextlib._GeneratorContextManager.__exit__ returns truthy, so run()'s own except never runs and there is no warning and no stderr line at all, with main() exiting 0 on a failed crawl. That is #872 reintroduced with the diagnostic removed. Data stays safe; the lie is confined to the exit code.

Four non-blocking items, and I am asking for the first three in one short round:

1. except BaseException: is unpinned. I mutated it to except Exception: myself — run=3 failures=0 errors=0, suite fully green — and the measured consequence of a KeyboardInterrupt mid-crawl is a truncated const file and an orphaned .const.py.*.bak left inside pcapkit/const/. The shipped code is right; nothing holds it there. A ~10-line test closes it.

2. A false clause in _snapshot_and_restore's docstring (__main__.py:73-75): "this crawler goes on to fail loudly anyway once Vendor.__init__ calls the same _dest_path itself." True for the first trigger it enumerates (a crawler outside the pcapkit.vendor tree), false for the second — an instance-method _dest_path raises TypeError on the class call but resolves fine on the instance call, so the target runs unprotected and succeeds silently (run() -> True, content replaced, no warning). The PR's own test-module docstring describes this correctly, so two docstrings in one PR disagree.

3. The # pylint: disable=no-else-raise pragma is inert. R1720 applies to if/elif/else after a raise, never to try/except/else; with the pragma removed and --enable=no-else-raise the file still scores 10.00/10, and pylint itself reports I0021: Useless suppression of 'no-else-raise'. Not a CI gate — the Makefile's PYLINT_FLAGS enables F,E,W,R, never I — so drop it rather than work around it.

4. Not asking: deleting os.close(fd) survives as an untested fd leak (harmless on Linux, the path still exists and copy2 reopens it), and swapping the else: back to an unindented statement while keeping raise survives too — correctly, since the two forms are semantically identical and no test can distinguish them.

Also confirmed: contextlib.suppress(OSError) wraps exactly one statement and cannot mask a success-path bug, and the success-path cleanup is pinned (neutering os.remove(backup) fails the listdir assertion); assertWarnsRegex cannot pass for the wrong reason, since the warning text is derived from the reported error; except BaseException: also covers GeneratorExit; the rename is a rename (R073) with no stale references and the PR is 5 files; run=3 / run=5 / run=4 and the pre-fix run=3 failures=2 all reproduce with both files swapped from origin/main; the anchored grep still emits exactly one annotation where the warning line also contains failed: 503; pylint 4.0.8 on the changed file is 10.00/10.