Take the job's outcome as an argument, not as an assumption #10
bit
Reference in New Issue
Block a user
Delete Branch "fix/report-status-argument"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
report_job_log.pyhardcoded the word failed into the comment header. Everycaller guards the step with
if: failure(), so that was true by construction —until an unguarded probe in weblib-viewer#10 ran it on a job that passed,
and the successful run posted a comment reading as a failure report. Anyone
scrolling that PR would conclude the probe had failed.
That matters because the interesting uses of this script are exactly the ones
that want
if: always(): a probe, or a job whose output is the point ratherthan its exit code.
What changed
--statussupplies the outcome (JOB_STATUSin the environment does the same,matching how every other argument here already defaults).
The default is still
failed. The four repos take this script from@mainand pass only the log path, so a change in required arguments would have broken
all of them at once. That backward compatibility has its own test, and the test
was shown to fail when the default is flipped.
Two decisions worth flagging, either of which could reasonably have gone the
other way:
argparsechoices=.${{ job.status }}yieldssuccess/failure/cancelled/skipped, and both those andpassed/failedare mapped — but an unrecognised status is put into theheader verbatim rather than rejected.
choices=would exit 2 on a value thetable has not heard of and the log would never be posted; a reporter must not
become the thing that reports nothing.
"failed before the build started" whatever the outcome. It now only says that
for a failure.
Verified
test_report_job_log.pyis new: stdlib only and offline, posting to anhttp.serveron localhost that keeps what it is sent, so every test reads thecomment back. An exit status of 0 proves nothing here — the script
deliberately swallows HTTP errors so a failure to report cannot mask the failure
being reported. Three tests drive the CLI as a subprocess with only environment
variables set, the way a workflow does.
Each check was proved to fire by injecting the fault, then reverting it:
failedagain (the original bug)DEFAULT_STATUS = "passed"(would break the four callers)choices=would%in the--statushelp textAll five reverted; the file's checksum matches the pre-injection copy, and the
suite is green again after each.
The rendered headers, from the real CLI against a throwaway server:
Not verified, and two things found on the way
.gitea/workflowsat all,and no
flake.lockforwith-nixpkgs.shto read, so adding one is aseparate decision rather than a line in this PR. Filed as #9.
if: always()is aper-repo judgement, not this repo's to make. The README shows the call.
__pycache__/report_job_log.cpython-313.pycwas tracked (committed byaccident in
061d8b2). Importing the module from the tests rewrites it, so itshowed up in this diff as stale bytecode of a file that had just changed. It
is untracked here and a
.gitignoreadded.Closes #8
`report_job_log.py` hardcoded the word *failed* into the comment header. Every caller guards the step with `if: failure()`, so it was true by construction -- until an unguarded probe in weblib-viewer#10 ran it on a job that passed, and the successful run posted a comment reading as a failure report. Anyone scrolling that PR would conclude the probe had failed. The interesting uses of this script are exactly the ones that want `if: always()`: a probe, or a job whose *output* is the point rather than its exit code. Those all lied in the header. `--status` now supplies the outcome. **The default is `failed`**, which is what `if: failure()` means, so the four consuming repos are untouched -- a change in required arguments would have broken all of them at once, since they take this script from `@main`. `JOB_STATUS` in the environment does the same, matching how every other argument here already reads its default from the Actions environment. `${{ job.status }}` yields `success`/`failure`/`cancelled`/`skipped` while a human writing the flag reaches for `passed`/`failed`, so both spellings are accepted and the expression can be passed straight through. An **unrecognised** status goes into the header verbatim rather than being rejected: `argparse`'s `choices=` would exit 2 on a value the table has not heard of, and the log -- the whole reason this script exists -- would never be posted. A reporter must not become the thing that reports nothing. The "no log file" note was status-dependent too; it claimed the step "failed before the build started" regardless. ## Verified `test_report_job_log.py`, new here: stdlib only and offline, posting to an `http.server` on localhost that keeps what it is sent, so each test reads the comment back. An exit status of 0 proves nothing -- the script deliberately swallows HTTP errors so a failure to report cannot mask the failure being reported. 21 tests, 0 skipped, 1.2s. Three of them drive the CLI as a subprocess with only environment variables set, the way a workflow does. Each check was shown to fire by injecting the fault and reverting it: | injected fault | result | |---|---| | header hardcodes `failed` again (the original bug) | 10 failures | | `DEFAULT_STATUS = "passed"` (would break the four callers) | 8 failures | | unknown status raises, as `choices=` would | 2 errors | | missing-log note keeps the failure wording | 1 failure | | a stray `%` in the `--status` help text | 1 failure | All five reverted; the file's checksum matches the pre-injection copy. Also drops a tracked `__pycache__/report_job_log.cpython-313.pyc` and adds a `.gitignore`. It was committed by accident in061d8b2and importing the module from the tests rewrites it, so it would otherwise show up in every future diff as stale bytecode of a file that had already changed. Closes #8 Co-authored-by: bit <bit@das-labor.org>b4fcba7ccato028c162874`status_phrase` renders a status it does not know verbatim, deliberately: `argparse`'s `choices=` would exit 2 on an unexpected value and the log -- the entire reason this script exists -- would never be posted. But the verbatim value lands in a code span inside a **bold** header, so a backtick in it closes the span early and the rest renders as markdown: --status 'x` **loud** `y' -> **`tests` finished with status `x` **loud** `y`** Nothing hostile is expected: the value comes from `${{ job.status }}` or a hand-written flag, both written by whoever wrote the workflow. It is worth closing anyway because this repo is public and four others consume the script as a composite action, so a branch name or a matrix value could reach this argument later without anyone revisiting this function. Backticks are removed rather than escaped -- there is no escape for a backtick inside a code span, only a wider fence, and the status is a short word rather than something whose exact bytes matter. Found in the cold re-read of #10, not by the suite, so the test that covers it was proved to fail without the fix. Co-authored-by: bit <bit@das-labor.org>Cold re-read done. One change pushed,
13e396a.Re-measured here rather than taken from the agent's report:
grep -ciE "skipped|expected failure"→ 0), 1.1 s.failedheader fails 10 of them. Reverted;cksum report_job_log.pyback to3892834760 8913,git diff --statempty.main's copy and this branch's copy as subprocesses against the same local server with the existing call shape (log path only, no new flag) and compared the posted bodies: identical. Still identical after my commit.success→ passed,cancelled→ was cancelled,skipped→ was skipped, unknown → verbatim, empty--status→ failed,JOB_STATUShonoured, and the flag beating the env var.t0ken. Nothing secret added, which this repo being public makes worth stating explicitly.What I changed
The verbatim path for an unrecognised status broke out of its own code span:
Backticks are now removed from that value. Not a security finding —
${{ job.status }}and a hand-written flag are both written by whoever wrote the workflow, so there is no untrusted input here today. It is worth closing anyway because this repo is public and shared by four others, so a branch name or a matrix value could reach this argument later without anyone revisiting the function. Backticks are removed rather than escaped because there is no escape for a backtick inside a code span, only a wider fence.The suite gained a test for it, and I proved it fails without the fix (1 failure) before reverting.
Agreeing with two judgement calls
choices=. Right, and for the reason given: the script exists because a red job says nothing, so it must not become the thing that says nothing. Exiting 2 on an unfamiliar status would throw away the log.__pycache__removal. Scope-adjacent but correct — a tracked.pycof a file this PR changes is stale by construction, and it was committed by accident in061d8b2.Not merged, and why
weblib-ci PRs need your approval, so this is not armed. Requesting your review now that the re-read is done — deliberately not before, since a push after an approval would revoke it.
Also filed by the agent: #9, that nothing runs this repo's own tests. It has no
.gitea/workflowsat all and noflake.lockforwith-nixpkgs.shto read, so the 22 tests only run when someone runs them by hand.View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.