From 028c16287464e77d73ecad58a6e2ad69867ff52f Mon Sep 17 00:00:00 2001 From: claude Date: Tue, 8 Sep 2026 07:54:27 +0000 Subject: [PATCH 1/2] 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 --- .gitignore | 4 + README.md | 43 ++- __pycache__/report_job_log.cpython-313.pyc | Bin 10058 -> 0 bytes report_job_log.py | 68 ++++- test_report_job_log.py | 322 +++++++++++++++++++++ 5 files changed, 431 insertions(+), 6 deletions(-) create mode 100644 .gitignore delete mode 100644 __pycache__/report_job_log.cpython-313.pyc create mode 100755 test_report_job_log.py diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..94a0f09 --- /dev/null +++ b/.gitignore @@ -0,0 +1,4 @@ +# The test suite imports report_job_log, and a tracked .pyc of it went stale +# the moment the source changed. +__pycache__/ +*.pyc diff --git a/README.md b/README.md index c9e14e8..07c9cf5 100644 --- a/README.md +++ b/README.md @@ -14,12 +14,25 @@ fetch it — which was measured to be the difference between one step and three. | file | what it does | |---|---| | `with-nixpkgs.sh` | Runs a command with one nixpkgs package on PATH, pinned to the *consuming* repo's `flake.lock`. Avoids `nix shell nixpkgs#x`, which re-resolves the registry and refetches a channel tarball whenever the branch moves. | -| `report_job_log.py` | Posts the tail of a build log as a PR comment. Exists because `actions/jobs/{id}/logs` returns 500 for every id on Gitea 1.25.2, so a red job otherwise says only that it failed. | +| `report_job_log.py` | Posts the tail of a build log as a PR comment. Exists because `actions/jobs/{id}/logs` returns 500 for every id on Gitea 1.25.2, so a red job otherwise says only that it failed. Takes `--status`; see below. | | `sync_blocked_label.py` | Keeps `Status/Blocked` in step with Gitea's dependency graph. Resolves the label from the repo *or the organisation*, and never touches an issue marked `Status/On Hold` or `Status/Abandoned`. | All three are standard library / plain bash only. They are *run*, not built, so this repo has no flake. +`test_report_job_log.py` covers the reporter. It is standard library and +offline — the forge it posts to is an `http.server` on localhost that keeps +what it is sent, so a test reads the comment back rather than trusting an exit +status of 0, which this script returns even when the POST failed. + +```bash +python3 test_report_job_log.py # 21 tests, ~1.2s, no network +``` + +There is no workflow running it: this repo has no `.gitea/workflows` at all, +and no `flake.lock` for `with-nixpkgs.sh` to read. Run it by hand before +pushing. + ## Using it ```yaml @@ -57,6 +70,34 @@ The `outputs.path` row is listed separately on purpose: the action *running* and its output *reaching the caller* are different claims, and a composite action returning an empty string is exactly the sort of thing that looks green. +### `report_job_log.py --status` + +The header used to be hardcoded to *failed*. That is true of every caller here, +because each guards the step with `if: failure()` — but a probe run under +`if: always()` posted a failure report for a job that had passed +(weblib-viewer#10, filed as #8). + +**The default is still `failed`**, so a caller passing only the log path is +unchanged. A step that can run on success has to say so: + +```yaml +- name: report the log + if: always() + env: + GITEA_TOKEN: ${{ secrets.GITEA_TOKEN }} + run: | + bash "${{ steps.ci.outputs.path }}/with-nixpkgs.sh" python3 \ + python3 "${{ steps.ci.outputs.path }}/report_job_log.py" /tmp/build.log \ + --status "${{ job.status }}" +``` + +`${{ job.status }}` yields `success`/`failure`/`cancelled`/`skipped`, so those +spellings are accepted alongside `passed`/`failed`. `JOB_STATUS` in the +environment does the same thing if a flag is awkward. An **unrecognised** +status is put in the header verbatim rather than rejected: `argparse`'s +`choices=` would exit 2 on a value this list has not heard of, and the log — +the entire reason the script exists — would never be posted. + ### Why not a flake input These are scripts a workflow runs, not derivations. A flake input would cost a diff --git a/__pycache__/report_job_log.cpython-313.pyc b/__pycache__/report_job_log.cpython-313.pyc deleted file mode 100644 index ed6dd00d0c1ae0d92b55626a119ba260eeaaf95a..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 10058 zcmey&%ge>Uz`(G5)x4}4Nd|_;APx+(KpCG;F)%PpWe8>{VhCmoX7Xk%Vg%DnMND9t zxriA|vlOv_Y1SfEFwIuP4yHMZ*n^piID=V=xMEm>#Gs}kQA!NKtfq_*fg+w@HYtW8 z-e7hqh9bUT4k?Br{$Nfih9ZGro?tF1hAhS+!5E=nZmS3ehC~Slh8Wf$HHc1RQi&l( zP>I15Dg-lIfgznKok^27aX&KyLqL9Ui9$(6szOO(W{yIBnnI#NQfX#Rib76)xH zLZU)JX-#R7ngfxNot~kp`MYco{>UPYDsBPUa^9yfq_C= zQGTvMLSk}BW`16=epY@`v3_-CO09lQetL0&LRx;2f|gclS!z+GmX<MNk(dMX0bwIQhsR(S9wMy$V+(&$@#@45HA#G=qMyAlqY7E=zz^hRLIXu z)lJGQQAo@yE>A7eQwYh(ELJEeEhz?R1^d1{Bef`1A+v-lIWbQmDOI5;H8Dj;AuqL} zM4==ftSUJpF)uw;p(G=*L?Jn`v^X_IArllf!6k`#DTzfX3OSicMTtd~3i)|Cl{yMJ znc1ld3B{Fp$?-`!`N`R-De*apNvSz{1(gZlaL6oC$V*L4DTaG9sTAZuuFSlW)S`l- z)RNR9JuWVv%(B#Ch0HvKg!0s+oXjMB2&J2x2??XjVg;~MlE9&$01BHTaKIKr;-&=T zFR**7*)tDQ78ZhS8i!>3CIBod5O8Hp!iQL$xz75E6z*-`4kik1^FP=nZ;m3 zi%LN8k)NlatIGv8Ss@eTeud=x0si4qG%gjsBkp*+7RJ1r-_ zT%oumwLl@gG_fcpHANw*QXwHT%}OCHF*B#MC{;rgD98b8&&w10sQdQfn&0(+ZA|G^vo9SC(0np9f0FRtgF3o*^EgPVqsm z0sg_BA^t&;2|8RbLDw)>zmWI<#}JPM1r1Qr1?7~0Acefr+@#bZO&zdqSI77ee{Wa6 z1bvt`2p62v;6??z28Fo>#fJv@BTI3=9m+3=9mP)fhpATquJE zSeSt!QH6ma8p>v12x5XW;fx?wI1|nYWq=hnFmqBE85q(TquF6f5Gt4$@))BzVB$gG zf*8hw&`J!Uj4<=^K!q)1Frz8dB?=7TOhLR5b;x8e6HJFKBis#43=9ek1`K)3q0CtI zFvIkyFeozUz{3FK_B19126+Yr1_cH`OIv1nhBQV^mMVvkj8uhyAcYc8*#RoPKzX7x zFAtO>((;R-2_2MXQp-~FN)!qbD|7Neg-Cvpf?s}Ks-E919#HWb4=rAAu|X3S`z@yY z;#({Qi6t4g*itJpi%W`cvE&z|=G|h+D$dWl#R4+s7IS)Pi6+Y}F0kS8V8u*n8Mj!$ z{98-~Ma7`H3zT(k2|@~1q|9?m7}@4{h{Z)h3=9mq3=9m#i3|)34Ghn?Mdt*~51$!6 zKW=8+3a{(3<`-qnZ}1D>;pFYey22?rBj}ER$aKz0oHLXr^LDU)XJ+7%dceuoFWD(M z!R|7r(sfRa1xXD~H@JEG%{t8{Xk6x&zQ7{=ot=S8>MEz?uVQWn28JEtPE5>)n7Et- z*por-g~cK~Nv1J@lO!`d8o@~^lpzQ?0Yh~mqMeT+m^qY@k0F@Fm?e)T8k$@f7=n=P zfa(f@CrhXR0|P@S3oJ3@v4*k+aUx`3tYFq)wooQM24f~fVw7VbU=zs4Faw};C~Gi# zFoz`*6GI+bG!ImeA&3vgLeNSKp=?2d2tJGjtKrd;dN3#451=*(YPMk)X7F%kV94VL zbsE~l1oe~tlc~Bua zv8X6B71E>#2m;mFnI*-rVn9#9kt+dK2gavl78fLzBxfXmj7}&{&PYuu%}GsAC@RfU zNXbtH)v3_73#dFv%PcA`Q7B6-D$dN$Q^-%_0vAEW$wiq3B?`rvxdl0u3I#==c0-DS zQeJ)us2)dd$AK$NC2;MMT9FB=Xbco|bs=prm`hSqz?OqsH=xQmHASJgB(X9zMIp5! z5mfzv+K0uNIiQj)5#q=EJWx%RS*!qY2^Xa3f;0ds^b~yZ^Rq!^BghAkkW?rwP)N=P z`9q-u6w-QJT#gC_rNtQ_J&6iXMdg_#847v%p!g^*$;`=7$Vdc*2c&^kk`JoqL6y0V zLU~4Ja)v@?F;_-@UTSfP6{wX4YQuq4E0kn_T5#YND5#a@cZ*T~7oUD&L8gA0p+2a5 zEY`1*(g#Ijv3+q#Vo9nkC=saTWaeg;*qR#L;&m)7$;dCttV#qmZGW+qcrh?f;pprzB^A<}+esKv%V?q8cRQZamXrlCifsseK!TAZ7K)+t6-gPd8i(Co|gfDZc zHaOp4VQ=?q^qXM6AZ{AePAv z<~t%1)4e8n%_yE4eh3OwsTvE(lbKLd9VeqOuBQ!d9)pDw8~B z?B++qtFU0E2*Y4zT~-DLT`mR&n7OcmT><1ieweKY8g4#IFarp}^eHez3&2zb3By$6sa>XFchgU zFfiO=ar6uJyv1BxnskdTB{exeCG{3_PHNsQ*5ZPk%o0tuB2bIy7Av^yD+0AdZm|?+ zR;AuzE=nv1nOIR$46+7XIw(Mj2Nuu>LXjr}149_7EVv9V3qTFUlnE~2rs4-CR!)f< zJbeA`o$eDHrxZ=Eo>+Z_NBM@Z=ydam=94WuT%eLuoM$l3Fj=9vQ1ha+_GJ;BD?GX% z*ckW)`_ntqyE8kOzwbym$#S-^OiUul8SMSisomK*#k3ra5XYj?2R z;O1}e{_#baK|uU710$!z4Po&vmk-R0oD#pjbHkdf`V0&Vy^PNC%>9l6&b&+q$OrR1f7+$u4nyR>SbvmpW$?sPLYKInqN~BV5vVy^1S;N&K+V}AQ1J!s=;f8>BmA$|tHzRLF$O^X&8avE(XdKYIz~$56 zdy*3BDSweuegX4}pbH?D!3O6$ppu}gYKHQBjhPy=wXRF)uVBB- zZvg7R8eZo#xXx+ts~FUPI;5=YEX90SipyDpJsDICLz5H(C`7^8ScL=BJB@~kV;v$< zVn9^MNGd@kC|naV89{I8Vzz4*q|Vg>-uFbxUOJ#^k_@x3^+nmhX2F{bc-_xUhMn*pDw6xj@Cs%K=C@?V8Pft}_(u@t1|OefgxQqlqm>Q%z!7|aH>IiHMcD12QK{{Y*6av;T2?AANC^8K2 zSu&8p!LlGSkDT&U4(21IScc>>`Cx`%1r~-pwqV6jrXWy15zPgB48cmFY_O0>WMg0m zRt{z3V+d9;X7*VoPsJgU0u+i z)h$7c2`o*gs0YC*1-W4w^&r@^`w$oRyyhvK7=Dtg_0> z2T$99#x(Qt6~Ix>gzI<)hA8LuT$ElQUMJxlQmKGGgFRAwRE52{h^po`@_c%1kHdLe*?NE-tV9B#;9@ z^Q@rhb@1$Zeja4D2|WD{p2P*s=7PqiLCFOa=%7h_5F3(W@=Hq!N=tOWbGwkq)#wCJ zvMDM-nhQ#Z)xa@<2%TGoQo6~RntEJZMV6qr7f#ME%>e}xd{uyz!YxLHOdUoBh9b~# zU=b+if|4sJ0IPT)wx*_llJ+e|8OS3wr@#%Cg>LbH22T^AIw6A~?9iEB@L1?A z9v4?P$55XTNG4#;%qu|%g*bZp+~NXTAD>v1UR(sKtH8aVTdd%5^deA06FfZz9`1zn zlR!P~B2Y{gg@QUpps^1~_bVR61@(i9KrNIaRnU+Nr$+dXQT{-7yhn(3IXCPQmM(3YRz))`Z_Nw7hN@deJcS zBQqmcIAep;0~WS+|3?1_t``&>8vHM^INpE?cq~a;p0zM*dEUaj3#J~I)jUD6URY!| z#9UT$`p&?p?#*~dTylEV#H#6a6YDOhSzZ>m`pyg%w)6VH%)l!0k%5ubhY@C@=aQo3 zRST<@*Db8ukb7C(<`CjwA z?r`v4;Pbk|;eCT&-~q2-e|Ts3b>4t0654B=)_bk=TJN{g?}D}8WgY*^5&;cvH`sX_ z>+T4NOxK#IHC=C_-UUUY%RUntU6(KSum)B4PqF`e&;J3E;slEo^bQ_n|GS`+jZK_PyzK< zuX8J3p53) zUY0bvE@^pD(( z!f6BBcAgzd*YzDQ>N_6Lx~%VeLC^03m;V)(fE(OAKfZ7?@CfzWblP0!R=miq2pV%2 z1Vxa5@&d=@ZVTNOSgbI-At*FmY@*m?i5V(0j3>xl7gW0_sJ5Vd1LtKys|y0wAD9{W zZSOd{9k)7Y^@*90)1UDPzrb(Mn7!byV$cZ68EYLsIfip`asm9z=WHA~1GtzkaIpsP zGvDHfj|b(W_;^i5KTVOM98h-30ui7r2PtQmic*WpL0OXrGPR=zo}dBe{abucA?RdJ zQ2|J65lAs;z@#8EC$$LFu0SeUz{Lo-@Bn39ko};QIPvkfSU^*CD;Yk68Xb@^R}P!p ze9$zZT~Q|k0|RJ$qPUcSf#Cx)BO~KS7I8+F&qfT40=F5sZ!>U$5!Y=7;oA(N5BN12 zctDXS+Q9RHje&!!y{f6|f{@l_cI^h1Pi*XrGM_|b7zIAr@iPj1QekH_{32q=$onCK kk%5J$qv|rVBxtn7qsgN)q&w^?i^vCN1{SHJ4h9AW08Z3)0RR91 diff --git a/report_job_log.py b/report_job_log.py index c7b2389..436d07d 100755 --- a/report_job_log.py +++ b/report_job_log.py @@ -17,9 +17,16 @@ Usage, from a workflow step guarded by `if: failure()`: report_job_log.py /tmp/build.log +The header says *failed* by default, which is what that guard means. A step +guarded by `if: always()` -- a probe, or a job whose output is the point rather +than its exit code -- has to say so, or the comment reports a failure that did +not happen: + + report_job_log.py /tmp/build.log --status "${{ job.status }}" + Everything else comes from the Actions environment: `GITHUB_REPOSITORY`, -`GITHUB_EVENT_PATH` (for the PR number), `GITEA_TOKEN`/`GITHUB_TOKEN`, and -`GITHUB_SERVER_URL`. +`GITHUB_EVENT_PATH` (for the PR number), `GITEA_TOKEN`/`GITHUB_TOKEN`, +`GITHUB_SERVER_URL`, and `JOB_STATUS` if `--status` is not passed. Remove this once the instance is updated and logs can be read directly. """ @@ -41,6 +48,51 @@ DEFAULT_TAIL = 12000 # nix colours its output, and the raw escapes are noise in Markdown. ANSI = re.compile(r"\x1b\[[0-9;]*[A-Za-z]") +# The header used to hardcode "failed". Every caller guards the step with +# `if: failure()`, so that was true by construction -- until an `if: always()` +# probe in weblib-viewer#10 posted a failure report for a job that had passed. +# The default stays `failed` so those guarded callers are unchanged. +DEFAULT_STATUS = "failed" + +# `${{ job.status }}` yields success/failure/cancelled/skipped; a human writing +# the flag by hand reaches for passed/failed. Accept both spellings so a call +# site can pass the expression straight through. +STATUS_PHRASES = { + "failed": "failed", + "failure": "failed", + "passed": "passed", + "success": "passed", + "succeeded": "passed", + "cancelled": "was cancelled", + "canceled": "was cancelled", + "skipped": "was skipped", +} + + +def status_phrase(status): + """The verb for the comment header. + + An unrecognised status is reported verbatim rather than rejected. This + script exists because a red job says nothing, so it must not itself become + the thing that says nothing: `argparse`'s `choices=` would exit 2 on a + status this table has not heard of, and the log would never be posted. + """ + key = (status or "").strip().lower() + if not key: + key = DEFAULT_STATUS + if key in STATUS_PHRASES: + return STATUS_PHRASES[key] + return "finished with status `" + status.strip() + "`" + + +def missing_log_note(path, phrase): + """What to say when the log file the step named is not there.""" + if phrase == "failed": + return (f"The step produced no log file at `{path}` — it failed " + "before the build started.") + return (f"The step produced no log file at `{path}` — nothing was " + "captured.") + def pull_request_number(event_path): """The PR this job is running for, from the event payload, or None.""" @@ -123,6 +175,11 @@ def main(argv=None): ap.add_argument("--pr", type=int, default=None) ap.add_argument("--job", default=os.environ.get("GITHUB_JOB") or "job") ap.add_argument("--tail", type=int, default=DEFAULT_TAIL) + ap.add_argument("--status", + default=os.environ.get("JOB_STATUS") or DEFAULT_STATUS, + help="outcome of the job being reported: failed (the " + "default, and what `if: failure()` means), passed, " + "cancelled, skipped, or ${{ job.status }} verbatim") args = ap.parse_args(argv) if not args.repo or not args.token: @@ -138,14 +195,15 @@ def main(argv=None): print("no pull request for this run: nothing to comment on") return 0 + phrase = status_phrase(args.status) + if not os.path.exists(args.log): - text = ("The step produced no log file at " - f"`{args.log}` — it failed before the build started.") + text = missing_log_note(args.log, phrase) else: text = "```\n" + tail(args.log, args.tail).rstrip() + "\n```" sha = (os.environ.get("GITHUB_SHA") or "")[:8] - body = (f"**`{args.job}` failed**{f' on `{sha}`' if sha else ''}.\n\n" + body = (f"**`{args.job}` {phrase}**{f' on `{sha}`' if sha else ''}.\n\n" "Job logs return 500 on this Gitea, so here is the tail of the " "build output, posted by [`report_job_log.py`]" "(https://git.chaosbit.de/weblib/weblib-ci).\n\n" diff --git a/test_report_job_log.py b/test_report_job_log.py new file mode 100755 index 0000000..0240806 --- /dev/null +++ b/test_report_job_log.py @@ -0,0 +1,322 @@ +#!/usr/bin/env python3 +"""Tests for `report_job_log.py`. + +Standard library only, like everything else here, and offline: the Gitea it +posts to is a `http.server` on localhost that keeps the comments it is sent, so +a test can *re-read* what was written instead of trusting an exit code of 0. + +Run them with any python3: + + python3 test_report_job_log.py + +The point of most of them is the comment **header**. `report_job_log.py` used +to hardcode the word "failed", so a step guarded by `if: always()` posted a +failure report for a job that had passed (weblib-ci#8, found in +weblib-viewer#10). The header is the one thing a reader sees before the log, so +it is the one thing worth asserting on. +""" + +import contextlib +import io +import json +import os +import subprocess +import sys +import threading +import unittest +from http.server import BaseHTTPRequestHandler, HTTPServer + +import report_job_log + +HERE = os.path.dirname(os.path.abspath(__file__)) +SCRIPT = os.path.join(HERE, "report_job_log.py") + +# Environment variables the script reads. Every test clears all of them, so a +# stray one in the ambient shell (this suite is meant to be runnable on a +# runner, where several of these are set) cannot change a result. +ACTIONS_ENV = [ + "GITHUB_REPOSITORY", "GITHUB_EVENT_PATH", "GITHUB_SERVER_URL", + "GITHUB_SHA", "GITHUB_JOB", "GITHUB_HEAD_REF", "GITHUB_REF_NAME", + "GITEA_HOST", "GITEA_TOKEN", "GITHUB_TOKEN", "JOB_STATUS", +] + + +class FakeGitea(BaseHTTPRequestHandler): + """Just enough of the API: list open PRs, and accept a comment.""" + + comments = [] # class-level; reset per test + pulls = [] + + def _send(self, code, payload): + raw = json.dumps(payload).encode() + self.send_response(code) + self.send_header("Content-Type", "application/json") + self.send_header("Content-Length", str(len(raw))) + self.end_headers() + self.wfile.write(raw) + + def do_GET(self): + if "/pulls" in self.path: + return self._send(200, type(self).pulls) + self._send(404, {"message": "no"}) + + def do_POST(self): + length = int(self.headers.get("Content-Length") or 0) + payload = json.loads(self.rfile.read(length) or b"{}") + type(self).comments.append({ + "path": self.path, + "auth": self.headers.get("Authorization"), + "body": payload.get("body", ""), + }) + self._send(201, {"html_url": "http://example.invalid/c/1"}) + + def log_message(self, *a): # keep the test output readable + pass + + +class ServerTestCase(unittest.TestCase): + """A test that runs the whole entry point against the fake forge.""" + + def setUp(self): + FakeGitea.comments = [] + FakeGitea.pulls = [] + self.server = HTTPServer(("127.0.0.1", 0), FakeGitea) + self.host = "http://127.0.0.1:%d" % self.server.server_address[1] + # `shutdown()` only takes effect on the next poll, so the default + # 0.5s interval charged this suite half a second per test -- measured + # 4.6s for nine tests, 0.6s after. + self.thread = threading.Thread( + target=self.server.serve_forever, kwargs={"poll_interval": 0.01}, + daemon=True) + self.thread.start() + self.addCleanup(self.server.server_close) + self.addCleanup(self.server.shutdown) + + self.log = os.path.join(self.mkdtemp(), "build.log") + with open(self.log, "w") as fh: + fh.write("nix-build \x1b[31msomething\x1b[0m\nDISTINCTIVE-LINE\n") + + for name in ACTIONS_ENV: + os.environ.pop(name, None) + + def mkdtemp(self): + import tempfile + d = tempfile.mkdtemp() + self.addCleanup(lambda: __import__("shutil").rmtree(d, + ignore_errors=True)) + return d + + def posted(self): + """What the server actually stored, read back from the server. + + An exit status of 0 is not evidence that a comment was written: the + script deliberately swallows HTTP errors so that a failure to report + does not mask the failure being reported, and returns 0 either way. + """ + return FakeGitea.comments + + def run_main(self, *argv): + """The entry point in-process, so a traceback is readable.""" + out = io.StringIO() + with contextlib.redirect_stdout(out): + code = report_job_log.main([ + self.log, "--repo", "weblib/weblib-ci", "--host", self.host, + "--token", "t0ken", "--pr", "8", "--job", "tests", *argv]) + self.assertEqual(code, 0) + self.assertIn("reported to weblib/weblib-ci#8", out.getvalue()) + return code + + def run_cli(self, *argv, env=None): + """The entry point as a subprocess: argv parsing and shebang included.""" + full = dict(os.environ) + full.update(env or {}) + proc = subprocess.run( + [sys.executable, SCRIPT, self.log, "--repo", "weblib/weblib-ci", + "--host", self.host, "--token", "t0ken", "--pr", "8", + "--job", "tests", *argv], + capture_output=True, text=True, env=full, cwd=HERE) + return proc + + +class TestHeader(ServerTestCase): + + def test_default_is_still_failed(self): + """Backward compatibility: the four repos pass only the log path.""" + self.run_main() + body = self.posted()[0]["body"] + self.assertIn("**`tests` failed**", body) + self.assertNotIn("passed", body) + + def test_status_passed(self): + self.run_main("--status", "passed") + body = self.posted()[0]["body"] + self.assertIn("**`tests` passed**", body) + self.assertNotIn("failed", body) + + def test_job_status_expression_success(self): + """`${{ job.status }}` yields `success`, not `passed`.""" + self.run_main("--status", "success") + self.assertIn("**`tests` passed**", self.posted()[0]["body"]) + + def test_job_status_expression_failure(self): + self.run_main("--status", "failure") + self.assertIn("**`tests` failed**", self.posted()[0]["body"]) + + def test_cancelled(self): + self.run_main("--status", "cancelled") + self.assertIn("**`tests` was cancelled**", self.posted()[0]["body"]) + + def test_case_and_whitespace_are_not_a_failure_report(self): + self.run_main("--status", " Success \n") + self.assertIn("**`tests` passed**", self.posted()[0]["body"]) + + def test_unknown_status_still_reports(self): + """A status the table has not heard of must not cost us the log. + + `argparse(choices=...)` would exit 2 here, and the log this script + exists to surface would never be posted. + """ + self.run_main("--status", "neutral") + body = self.posted()[0]["body"] + self.assertIn("**`tests` finished with status `neutral`**", body) + self.assertIn("DISTINCTIVE-LINE", body) + + def test_empty_status_falls_back_to_the_default(self): + self.run_main("--status", "") + self.assertIn("**`tests` failed**", self.posted()[0]["body"]) + + def test_sha_and_log_survive_a_non_default_status(self): + os.environ["GITHUB_SHA"] = "0123456789abcdef" + self.run_main("--status", "passed") + body = self.posted()[0]["body"] + self.assertIn("**`tests` passed** on `01234567`.", body) + self.assertIn("DISTINCTIVE-LINE", body) + self.assertNotIn("\x1b[31m", body) # ANSI still stripped + + +class TestEnvironment(ServerTestCase): + + def test_job_status_env_is_honoured(self): + proc = self.run_cli(env={"JOB_STATUS": "success"}) + self.assertEqual(proc.returncode, 0, proc.stderr) + self.assertIn("**`tests` passed**", self.posted()[0]["body"]) + + def test_flag_beats_the_environment(self): + proc = self.run_cli("--status", "failed", env={"JOB_STATUS": "success"}) + self.assertEqual(proc.returncode, 0, proc.stderr) + self.assertIn("**`tests` failed**", self.posted()[0]["body"]) + + def test_unset_environment_is_still_failed(self): + proc = self.run_cli() + self.assertEqual(proc.returncode, 0, proc.stderr) + self.assertIn("**`tests` failed**", self.posted()[0]["body"]) + + +class TestMissingLog(ServerTestCase): + + def test_missing_log_when_failed(self): + self.log = os.path.join(self.mkdtemp(), "absent.log") + self.run_main() + self.assertIn("it failed before the build started", + self.posted()[0]["body"]) + + def test_missing_log_when_passed_does_not_claim_a_failure(self): + self.log = os.path.join(self.mkdtemp(), "absent.log") + self.run_main("--status", "passed") + body = self.posted()[0]["body"] + self.assertIn("nothing was captured", body) + self.assertNotIn("failed", body) + + +class TestEndToEnd(ServerTestCase): + """The CLI, driven the way a workflow drives it: environment, no `--pr`.""" + + def _event_file(self, number): + path = os.path.join(self.mkdtemp(), "event.json") + with open(path, "w") as fh: + json.dump({"pull_request": {"number": number}}, fh) + return path + + def _run(self, *argv, env=None): + full = dict(os.environ) + full.pop("GITEA_HOST", None) + full.update({ + "GITHUB_REPOSITORY": "weblib/weblib-ci", + "GITHUB_SERVER_URL": self.host, + "GITEA_TOKEN": "t0ken", + "GITHUB_JOB": "probe", + "GITHUB_SHA": "abcdef0123456789", + "GITHUB_EVENT_PATH": self._event_file(42), + }) + full.update(env or {}) + return subprocess.run([sys.executable, SCRIPT, self.log, *argv], + capture_output=True, text=True, env=full, + cwd=HERE) + + def test_always_guarded_probe_that_passed(self): + proc = self._run("--status", "success") + self.assertEqual(proc.returncode, 0, proc.stderr) + self.assertIn("reported to weblib/weblib-ci#42", proc.stdout) + + # The write is only real if the resource says so. + posted = self.posted() + self.assertEqual(len(posted), 1, posted) + self.assertEqual(posted[0]["path"], + "/api/v1/repos/weblib/weblib-ci/issues/42/comments") + self.assertEqual(posted[0]["auth"], "token t0ken") + body = posted[0]["body"] + self.assertIn("**`probe` passed** on `abcdef01`.", body) + self.assertNotIn("failed", body) + self.assertIn("DISTINCTIVE-LINE", body) + + def test_failure_guarded_job_is_unchanged(self): + proc = self._run() + self.assertEqual(proc.returncode, 0, proc.stderr) + self.assertIn("**`probe` failed** on `abcdef01`.", + self.posted()[0]["body"]) + + def test_branch_lookup_path_still_works(self): + """No event payload: the PR is found by head branch, and reported.""" + FakeGitea.pulls = [{"number": 7, "head": {"ref": "fix/x"}}] + proc = self._run("--status", "passed", + env={"GITHUB_EVENT_PATH": "", + "GITHUB_HEAD_REF": "fix/x"}) + self.assertEqual(proc.returncode, 0, proc.stderr) + self.assertIn("reported to weblib/weblib-ci#7", proc.stdout) + self.assertIn("**`probe` passed**", self.posted()[0]["body"]) + + def test_help_still_renders(self): + """argparse `%`-expands `help=`; a stray `%` there breaks `--help`.""" + proc = subprocess.run([sys.executable, SCRIPT, "--help"], + capture_output=True, text=True, cwd=HERE) + self.assertEqual(proc.returncode, 0, proc.stderr) + self.assertIn("--status", proc.stdout) + + +class TestStatusPhrase(unittest.TestCase): + """The unit under the header, without a server.""" + + def test_table(self): + for status, want in [ + ("failed", "failed"), ("failure", "failed"), + ("passed", "passed"), ("success", "passed"), + ("succeeded", "passed"), + ("cancelled", "was cancelled"), ("canceled", "was cancelled"), + ("skipped", "was skipped"), + ("SUCCESS", "passed"), (" failure ", "failed"), + (None, "failed"), ("", "failed"), + ]: + with self.subTest(status=status): + self.assertEqual(report_job_log.status_phrase(status), want) + + def test_unknown_is_verbatim(self): + self.assertEqual(report_job_log.status_phrase("weird"), + "finished with status `weird`") + + 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") + + +if __name__ == "__main__": + unittest.main(verbosity=2) -- 2.51.2 From 13e396a795ceb9c22d5285e6837660cee71dfa19 Mon Sep 17 00:00:00 2001 From: claude Date: Tue, 8 Sep 2026 08:00:58 +0000 Subject: [PATCH 2/2] 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 --- report_job_log.py | 8 +++++++- test_report_job_log.py | 9 +++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/report_job_log.py b/report_job_log.py index 436d07d..d589dab 100755 --- a/report_job_log.py +++ b/report_job_log.py @@ -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): diff --git a/test_report_job_log.py b/test_report_job_log.py index 0240806..4334e32 100755 --- a/test_report_job_log.py +++ b/test_report_job_log.py @@ -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") -- 2.51.2