Compare commits

..

2 Commits

Author SHA1 Message Date
13e396a795 Keep an unrecognised status inside its code span
`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>
2026-09-08 08:00:58 +00:00
028c162874 Take the job's outcome as an argument, not as an assumption
`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 in 061d8b2 and 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>
2026-09-08 07:57:23 +00:00
2 changed files with 16 additions and 1 deletions

View File

@@ -82,7 +82,13 @@ def status_phrase(status):
key = DEFAULT_STATUS
if key in STATUS_PHRASES:
return STATUS_PHRASES[key]
return "finished with status `" + status.strip() + "`"
# Backticks removed, not escaped: the verbatim value goes inside a code
# span in a **bold** header, and a backtick in it closes the span early --
# the rest of the status then renders as markdown. Nothing hostile is
# expected here (`${{ job.status }}` is written by whoever wrote the
# workflow), but this repo is public and consumed by four others, and a
# branch name or matrix value could reach this argument later.
return "finished with status `" + status.strip().replace("`", "") + "`"
def missing_log_note(path, phrase):

9
test_report_job_log.py Normal file → Executable file
View File

@@ -313,6 +313,15 @@ class TestStatusPhrase(unittest.TestCase):
self.assertEqual(report_job_log.status_phrase("weird"),
"finished with status `weird`")
def test_a_backtick_in_an_unknown_status_cannot_escape_the_code_span(self):
"""The verbatim value sits in a code span inside a **bold** header, so
a backtick in it would close the span and let the rest render as
markdown. `${{ job.status }}` is workflow-author-controlled rather than
hostile, but this script is public and shared by four repos."""
phrase = report_job_log.status_phrase("x` **loud** `y")
self.assertEqual(phrase, "finished with status `x **loud** y`")
self.assertEqual(phrase.count("`"), 2)
def test_default_constant_is_failed(self):
"""Named so that changing it is a deliberate act, not a typo."""
self.assertEqual(report_job_log.DEFAULT_STATUS, "failed")