chore: Extract the failure-tail reporter into an action #45

ahmad · 2026-09-05T06:07:05Z

ahmad commented

2026-09-05 06:07:05 +00:00

Issue

Closes #38

Problem

Four hand-copied reporters had drifted, and this repository's own copy carried the defect the others were fixed for:

tail -c 30000 /tmp/validate.log > /tmp/report.tail 2>/dev/null || true
... fs.existsSync("/tmp/report.tail") ? readFileSync(...) : "(no output was captured…)"

The redirection creates the file before tail can fail, so existsSync is always true, the read returns "", and that fallback is unreachable — an empty code fence, the same bug as ahmad/expiro#27. So this is a consolidation and a fix.

Solution

A report-job-failure composite action, this repository's own validate migrated onto it, and a README section that documents the capture half the action cannot do.

Review notes

Migrating this repo's own consumer is how the fourth criterion is met without a four-repository pull request. expiro, portfolio and imamah follow as their own issues — one change touching four repositories could not be reviewed or reverted as a unit.

Verified by driving the action's shell at a local HTTP server, substituting only the hostname:

log absent               "No output was captured, so the job failed before the first step that tees…"
log present but empty    same message
log has content          "FAIL check-actions.py"                       (tail unchanged)
stale tail + empty log   same message                                  (stale not reported)

The third criterion — a reporting failure must not mask the job's failure — has its own cases:

post returns HTTP 500    "Could not post the failure comment (HTTP 500)…"   exit 0
server unreachable       "…(HTTP 000)…"                                     exit 0
empty pr-number (push)   "No pull request number… (a push build)."          exit 0

No set -e, and a non-2xx never fails the step.

A mutation of mine was broken, and I am reporting it because the corrected one changes the conclusion's basis. Deleting the fallback line left an if with an empty body — a syntax error, confirmed by bash -n. It produced "nothing posted", which I could have read as "the guard matters" for entirely the wrong reason. Replacing the fallback with a no-op keeps the shell valid and isolates the behaviour:

rm -f removed                      "STALE" reported as this run's output      killed
fallback neutered (valid syntax)   ''  the empty code fence returns           killed

curl and jq, and what that costs. A composite action cannot assume node, so it posts with curl. ahmad/imamah's job container is node:22-bookworm and posts through node's fetch for exactly that reason — the README says a node-only image should install curl and jq or keep its own reporter, rather than leaving that divergence to be rediscovered.

uses: ./report-job-failure in this repo's own workflow rather than @v3: the released tag will not carry this action until the merge moves it, so the consumer under review must use the tree under review.

Risks and trade-offs

The README section corrects a claim several comments in these repositories still make — that the run log is not fetchable. It is (/actions/jobs/{job_id}/logs); the trap is that a status target_url carries a per-repository index, not the id those routes take. The action's justification is discoverability, and the README now says so rather than repeating the false premise.

jq missing is a silent no-op with a message on stderr, consistent with the third criterion. That is the right trade here, but it does mean a consumer on an image without jq gets no comment and only a line in the log saying why.

### Issue Closes #38 ### Problem Four hand-copied reporters had drifted, and **this repository's own copy carried the defect the others were fixed for**: ```sh tail -c 30000 /tmp/validate.log > /tmp/report.tail 2>/dev/null || true ... fs.existsSync("/tmp/report.tail") ? readFileSync(...) : "(no output was captured…)" ``` The redirection creates the file before `tail` can fail, so `existsSync` is always true, the read returns `""`, and that fallback is unreachable — an empty code fence, the same bug as `ahmad/expiro#27`. So this is a consolidation *and* a fix. ### Solution A `report-job-failure` composite action, this repository's own `validate` migrated onto it, and a README section that documents the capture half the action cannot do. ### Review notes **Migrating this repo's own consumer is how the fourth criterion is met without a four-repository pull request.** `expiro`, `portfolio` and `imamah` follow as their own issues — one change touching four repositories could not be reviewed or reverted as a unit. **Verified by driving the action's shell at a local HTTP server, substituting only the hostname:** ``` log absent "No output was captured, so the job failed before the first step that tees…" log present but empty same message log has content "FAIL check-actions.py" (tail unchanged) stale tail + empty log same message (stale not reported) ``` **The third criterion — a reporting failure must not mask the job's failure — has its own cases:** ``` post returns HTTP 500 "Could not post the failure comment (HTTP 500)…" exit 0 server unreachable "…(HTTP 000)…" exit 0 empty pr-number (push) "No pull request number… (a push build)." exit 0 ``` No `set -e`, and a non-2xx never fails the step. **A mutation of mine was broken, and I am reporting it because the corrected one changes the conclusion's basis.** Deleting the fallback line left an `if` with an empty body — a *syntax error*, confirmed by `bash -n`. It produced "nothing posted", which I could have read as "the guard matters" for entirely the wrong reason. Replacing the fallback with a no-op keeps the shell valid and isolates the behaviour: ``` rm -f removed "STALE" reported as this run's output killed fallback neutered (valid syntax) '' the empty code fence returns killed ``` **`curl` and `jq`, and what that costs.** A composite action cannot assume node, so it posts with `curl`. `ahmad/imamah`'s job container is `node:22-bookworm` and posts through node's `fetch` for exactly that reason — the README says a node-only image should install `curl` and `jq` or keep its own reporter, rather than leaving that divergence to be rediscovered. **`uses: ./report-job-failure`** in this repo's own workflow rather than `@v3`: the released tag will not carry this action until the merge moves it, so the consumer under review must use the tree under review. ### Risks and trade-offs The README section corrects a claim several comments in these repositories still make — that the run log is not fetchable. It is (`/actions/jobs/{job_id}/logs`); the trap is that a status `target_url` carries a per-repository index, not the id those routes take. The action's justification is discoverability, and the README now says so rather than repeating the false premise. `jq` missing is a silent no-op with a message on stderr, consistent with the third criterion. That is the right trade here, but it does mean a consumer on an image without `jq` gets no comment and only a line in the log saying why.

ahmad added 1 commit

2026-09-05 06:07:06 +00:00

chore: Extract the failure-tail reporter into an action

PR / validate (pull_request) Successful in 2m55s

Details

ed5afd80d2

Four hand-copied reporters had drifted, and this repository's own had
the defect the others were fixed for: the redirection creates the tail
before tail can fail, so an absent log read back as an empty string
and the fallback was unreachable.

The action never fails the step — it runs when the job has already
failed, and a reporter that cannot post must not replace a clear red
with a confusing one.

Closes #38

ahmad commented

2026-09-05 06:11:09 +00:00

validate green on ed5afd8. Verification against each acceptance criterion.

Given a job that failed with output in the log, when the action runs, then a comment on the pull request holds the tail.

Driven at a local HTTP server with only the hostname substituted, so the guards, jq body and curl headers are exercised: a populated log posts FAIL check-actions.py unchanged.

Given the log is missing or empty, when the action runs, then the comment says so rather than showing an empty code fence.

Absent, empty, and stale-tail-with-empty-log all post "No output was captured, so the job failed before the first step that tees into …", and the stale content is not reported.

Given the comment cannot be posted, when the action runs, then the job's own failure is still what the run reports.

post returns HTTP 500    "Could not post the failure comment (HTTP 500)…"   exit 0
server unreachable       "…(HTTP 000)…"                                     exit 0
empty pr-number (push)   "No pull request number… (a push build)."          exit 0

No set -e, no failure on a non-2xx, and jq missing is a message rather than a crash.

Given a consumer migrates to the action, when its job fails, then the comment is the same as before the migration.

This repository's own validate is migrated. The comment is not merely the same — it is better, because the copy being replaced was broken: > /tmp/report.tail created the file, so existsSync was always true, the read returned "", and its "(no output was captured…)" fallback could never fire. The migration replaces an empty fence with a sentence.

Given the README, when a new repository adds a job, then it says to use pipefail with a pipeline and not the exec > >(tee) form, and why.

Both are in the new section, with the reasons rather than the rule alone: pipefail because a run: body is bash -e without it and cmd | tee otherwise exits with tee's status — which turned a red job green in ahmad/portfolio #70; and against exec > >(tee …) because that tee outlives the shell, so the log can be absent when a later step reads it.

Mutations: rm -f removed reports STALE as this run's output; the fallback neutered brings the empty fence back. Both killed. My first attempt at the second was a broken mutant — deleting the line left an if with an empty body, a syntax error — and is described in the PR body, because it would have supported the right conclusion for the wrong reason.

Not proven by this run: that the migrated step posts in anger. It is if: failure(), which a green build never reaches; the evidence is the local exercise above. The first genuine red here will confirm it.

`validate` green on `ed5afd8`. Verification against each acceptance criterion. > Given a job that failed with output in the log, when the action runs, then a comment on the pull request holds the tail. Driven at a local HTTP server with only the hostname substituted, so the guards, `jq` body and `curl` headers are exercised: a populated log posts `FAIL check-actions.py` unchanged. > Given the log is missing or empty, when the action runs, then the comment says so rather than showing an empty code fence. Absent, empty, and stale-tail-with-empty-log all post *"No output was captured, so the job failed before the first step that tees into …"*, and the stale content is not reported. > Given the comment cannot be posted, when the action runs, then the job's own failure is still what the run reports. ``` post returns HTTP 500 "Could not post the failure comment (HTTP 500)…" exit 0 server unreachable "…(HTTP 000)…" exit 0 empty pr-number (push) "No pull request number… (a push build)." exit 0 ``` No `set -e`, no failure on a non-2xx, and `jq` missing is a message rather than a crash. > Given a consumer migrates to the action, when its job fails, then the comment is the same as before the migration. This repository's own `validate` is migrated. The comment is not merely the same — it is **better**, because the copy being replaced was broken: `> /tmp/report.tail` created the file, so `existsSync` was always true, the read returned `""`, and its "(no output was captured…)" fallback could never fire. The migration replaces an empty fence with a sentence. > Given the README, when a new repository adds a job, then it says to use `pipefail` with a pipeline and not the `exec > >(tee)` form, and why. Both are in the new section, with the reasons rather than the rule alone: `pipefail` because a `run:` body is `bash -e` without it and `cmd | tee` otherwise exits with `tee`'s status — which turned a red job green in `ahmad/portfolio` #70; and against `exec > >(tee …)` because that `tee` outlives the shell, so the log can be absent when a later step reads it. **Mutations:** `rm -f` removed reports `STALE` as this run's output; the fallback neutered brings the empty fence back. Both killed. My first attempt at the second was a broken mutant — deleting the line left an `if` with an empty body, a syntax error — and is described in the PR body, because it would have supported the right conclusion for the wrong reason. **Not proven by this run:** that the migrated step posts in anger. It is `if: failure()`, which a green build never reaches; the evidence is the local exercise above. The first genuine red here will confirm it.

ahmad changed title from ~~WIP: chore: Extract the failure-tail reporter into an action~~ to chore: Extract the failure-tail reporter into an action

2026-09-05 06:11:10 +00:00

ahmad_bot reviewed

2026-09-05 06:32:53 +00:00

ahmad_bot left a comment

Two findings, both non-blocking. The extraction is sound and the fix inside it is real: this repository's own copy had the unreachable-fallback bug, so consolidating and fixing in one change is the right call rather than two.

Verified rather than read:

curl -w '%{http_code}' against an unreachable server returns HTTP 000 and the case reports it correctly. The || true matters here — || echo 000 would produce 000000, which is a trap a previous copy in this fleet fell into.
The reporter cannot fail the step in any path I could drive: unreachable server, missing dependency, empty pr-number. All exit 0. Third criterion holds.
No teed step in this repository holds a secret, so the masking gap below is a hazard for adopters rather than a live exposure here.

Two things I want to credit specifically.

Reporting the broken mutation instead of its result. Deleting the fallback left an if with an empty body, and a syntax error produces "nothing posted" for a reason that has nothing to do with the guard — a green-looking mutation table built on that would have been worse than none. Replacing it with a no-op and rerunning is the correct repair, and saying so is what makes the rest of the table believable.

uses: ./report-job-failure rather than @v3. A consumer under review must exercise the tree under review; pointing at the released tag would have validated the old action and told you nothing. This repository has no pipeline that would have caught the difference — every change here reaches four consumers unexecuted — which makes self-consumption the only real test available, and it is the one you used.

On the README correcting the log-fetchability claim: thank you for taking it. The premise was mine and it had spread into issue bodies as settled fact; putting the correction where consumers read it does more than my correcting my own notes.

The findings are inline: a security note that belongs in the README now that this is shared, and an asymmetry between the jq and curl guards.

Two findings, both non-blocking. The extraction is sound and the fix inside it is real: this repository's own copy had the unreachable-fallback bug, so consolidating and fixing in one change is the right call rather than two. **Verified rather than read:** - `curl -w '%{http_code}'` against an unreachable server returns `HTTP 000` and the `case` reports it correctly. The `|| true` matters here — `|| echo 000` would produce `000000`, which is a trap a previous copy in this fleet fell into. - The reporter cannot fail the step in any path I could drive: unreachable server, missing dependency, empty `pr-number`. All exit 0. Third criterion holds. - No teed step in this repository holds a secret, so the masking gap below is a hazard for adopters rather than a live exposure here. **Two things I want to credit specifically.** Reporting the broken mutation instead of its result. Deleting the fallback left an `if` with an empty body, and a syntax error produces "nothing posted" for a reason that has nothing to do with the guard — a green-looking mutation table built on that would have been worse than none. Replacing it with a no-op and rerunning is the correct repair, and saying so is what makes the rest of the table believable. `uses: ./report-job-failure` rather than `@v3`. A consumer under review must exercise the tree under review; pointing at the released tag would have validated the old action and told you nothing. This repository has no pipeline that would have caught the difference — every change here reaches four consumers unexecuted — which makes self-consumption the only real test available, and it is the one you used. **On the README correcting the log-fetchability claim:** thank you for taking it. The premise was mine and it had spread into issue bodies as settled fact; putting the correction where consumers read it does more than my correcting my own notes. The findings are inline: a security note that belongs in the README now that this is shared, and an asymmetry between the `jq` and `curl` guards.

report-job-failure/action.yml

					
				@ -0,0 +57,4 @@

				          tail -c "${MAX_BYTES}" "${LOG_PATH}" > "${TAIL_PATH}" || true

				        fi

				        TAIL="$(cat "${TAIL_PATH}" 2>/dev/null || true)"

The tail is read back from the file tee wrote and posted straight to the API, so the runner's secret masking never applies to it. ::add-mask:: redacts what the runner renders in its own log; it cannot touch bytes a child process wrote into a pipe, and nothing here is echoed for it to redact anyway.

You know this — you wrote it in ahmad/imamah#152's risks:

The reporter reads back the file tee wrote, so the runner's log masking does not apply to it.

That warning did not make it into the README, and this is the change that turns a hand-copied snippet into something other repositories adopt. The consumer who leaks a token will be one who read this README and not that pull request description.

Not a live exposure today, and I checked rather than assuming: none of this repository's three teed steps (Test the checker, Check the actions, README lint) has a secret in scope. The hazard is what adoption invites — a consumer tees a deploy or publish step, and its token lands in a pull request comment.

Concrete fix, a README paragraph beside the curl/jq one:

The tail bypasses the runner's secret masking. It is read from the file tee wrote and posted verbatim, so a masked value is masked only in the run log, not here. Do not tee a step that holds a secret — capture it separately, or redact before it reaches the log.

Worth considering for the description: too, since that is what someone sees when they go looking for the inputs.

The tail is read back from the file `tee` wrote and posted straight to the API, so **the runner's secret masking never applies to it**. `::add-mask::` redacts what the runner renders in its own log; it cannot touch bytes a child process wrote into a pipe, and nothing here is echoed for it to redact anyway. You know this — you wrote it in `ahmad/imamah#152`'s risks: > The reporter reads back the file `tee` wrote, so the runner's log masking does not apply to it. That warning did not make it into the README, and this is the change that turns a hand-copied snippet into something other repositories adopt. The consumer who leaks a token will be one who read this README and not that pull request description. Not a live exposure today, and I checked rather than assuming: none of this repository's three teed steps (`Test the checker`, `Check the actions`, `README lint`) has a secret in scope. The hazard is what adoption invites — a consumer tees a deploy or publish step, and its token lands in a pull request comment. Concrete fix, a README paragraph beside the `curl`/`jq` one: > **The tail bypasses the runner's secret masking.** It is read from the file `tee` wrote and posted verbatim, so a masked value is masked only in the run log, not here. Do not tee a step that holds a secret — capture it separately, or redact before it reaches the log. Worth considering for the `description:` too, since that is what someone sees when they go looking for the inputs.

report-job-failure/action.yml

					
				@ -0,0 +71,4 @@

				          '{body: ("The `" + $name + "` job failed. The tail of its output, so it can be read here rather than by fetching the run log:\n\n```\n" + $tail + "\n```")}' \

				          > "${TAIL_PATH}.json"

				        code="$(curl -sS -o /dev/null -w '%{http_code}' -X POST \

jq gets an explicit command -v guard with a clear message; curl does not, and the README tells node-image consumers to install both — so its absence is an anticipated case that reports poorly. Measured:

curl missing   ->  Could not post the failure comment (HTTP ); the job result is unchanged.

Exit 0, so the third criterion still holds and the job's own failure is not masked — this is a legibility fault, not a correctness one. But HTTP ) with an empty code reads as a broken reporter rather than a missing dependency, on the exact image the README warns about.

command -v curl >/dev/null || {
  echo "curl is not installed, so the failure comment cannot be posted." >&2
  exit 0
}

While here: || true after the curl is the right choice and worth not losing. -w '%{http_code}' already prints 000 on a connection failure, so the tempting || echo 000 yields 000000 and falls through the case as garbage. Confirmed both ways — unreachable server gives a clean HTTP 000.

`jq` gets an explicit `command -v` guard with a clear message; `curl` does not, and the README tells node-image consumers to install *both* — so its absence is an anticipated case that reports poorly. Measured: ``` curl missing -> Could not post the failure comment (HTTP ); the job result is unchanged. ``` Exit 0, so the third criterion still holds and the job's own failure is not masked — this is a legibility fault, not a correctness one. But `HTTP )` with an empty code reads as a broken reporter rather than a missing dependency, on the exact image the README warns about. ```sh command -v curl >/dev/null || { echo "curl is not installed, so the failure comment cannot be posted." >&2 exit 0 } ``` While here: `|| true` after the `curl` is the right choice and worth not losing. `-w '%{http_code}'` already prints `000` on a connection failure, so the tempting `|| echo 000` yields `000000` and falls through the `case` as garbage. Confirmed both ways — unreachable server gives a clean `HTTP 000`.

ahmad changed title from ~~chore: Extract the failure-tail reporter into an action~~ to WIP: chore: Extract the failure-tail reporter into an action

2026-09-05 06:34:08 +00:00

ahmad added 1 commit

2026-09-05 06:35:06 +00:00

fix: Guard curl too, and warn that the tail is not masked

PR / validate (pull_request) Successful in 1m26s

Details

61dc712d03

jq was guarded and curl was not, so its absence reported as HTTP with
an empty code — a broken reporter rather than a missing tool, on the
one image the README warns about.

The masking warning was in a pull request description rather than the
README, which is where an adopter reads it. The tail is posted
verbatim, so a teed step holding a secret puts it in a comment.

Refs #38

ahmad commented

2026-09-05 06:35:29 +00:00

Both taken, fixed in 61dc712. Back to WIP: while CI runs.

The masking warning is the one I should have caught, and you identified exactly why I did not. I wrote it in ahmad/imamah#152's risks section — where a reviewer of that pull request reads it once — and not in the README, which is what an adopter reads. A warning in a merged PR description is a warning nobody will meet. It is now beside the curl/jq paragraph, and in the action's own description: so it is visible from a consumer's editor:

The tail is read from the file tee wrote and posted verbatim, so ::add-mask:: — which only redacts what the runner renders into its own log — does not touch it. Do not tee a step that holds a secret.

Your framing is the right one: not a live exposure here, since none of this repository's three teed steps has a secret in scope, but the hazard adoption invites. This change is precisely what turns a hand-copied snippet into something other repositories take.

The curl asymmetry — measured both ways. A PATH with jq and deliberately without curl:

with the guard      "curl is not installed, so the failure comment cannot be posted."   exit 0
without the guard   "curl: command not found"
                    "Could not post the failure comment (HTTP ); the job result…"       exit 0

The second is what you reported, and it reads as a broken reporter rather than a missing tool — on the one image the README tells people to watch. Now guarded symmetrically with jq, same shape and same exit.

My first attempt at that test was a bad fixture, worth mentioning given the theme of this pull request: I built a restricted PATH that still contained /usr/bin, so curl was found and the guard never fired — the run "passed" while proving nothing. The second attempt used env -i with a directory holding only the tools under test.

On || true after curl: noted and kept, for the reason you give. -w '%{http_code}' already prints 000 on a connection failure, so || echo 000 concatenates to 000000 and falls through the case as garbage — a copy in this fleet did exactly that, and I have added nothing that would tempt the next person to reintroduce it.

Nothing else changed; the extraction, the migration and the four log-state cases are as verified above.

Both taken, fixed in `61dc712`. Back to `WIP:` while CI runs. **The masking warning is the one I should have caught, and you identified exactly why I did not.** I wrote it in `ahmad/imamah#152`'s risks section — where a reviewer of that pull request reads it once — and not in the README, which is what an adopter reads. A warning in a merged PR description is a warning nobody will meet. It is now beside the `curl`/`jq` paragraph, and in the action's own `description:` so it is visible from a consumer's editor: > The tail is read from the file `tee` wrote and posted verbatim, so `::add-mask::` — which only redacts what the runner renders into its own log — does not touch it. **Do not tee a step that holds a secret.** Your framing is the right one: not a live exposure here, since none of this repository's three teed steps has a secret in scope, but the hazard adoption invites. This change is precisely what turns a hand-copied snippet into something other repositories take. **The `curl` asymmetry — measured both ways.** A `PATH` with `jq` and deliberately without `curl`: ``` with the guard "curl is not installed, so the failure comment cannot be posted." exit 0 without the guard "curl: command not found" "Could not post the failure comment (HTTP ); the job result…" exit 0 ``` The second is what you reported, and it reads as a broken reporter rather than a missing tool — on the one image the README tells people to watch. Now guarded symmetrically with `jq`, same shape and same exit. My first attempt at that test was a bad fixture, worth mentioning given the theme of this pull request: I built a restricted `PATH` that still contained `/usr/bin`, so `curl` was found and the guard never fired — the run "passed" while proving nothing. The second attempt used `env -i` with a directory holding only the tools under test. **On `|| true` after `curl`:** noted and kept, for the reason you give. `-w '%{http_code}'` already prints `000` on a connection failure, so `|| echo 000` concatenates to `000000` and falls through the `case` as garbage — a copy in this fleet did exactly that, and I have added nothing that would tempt the next person to reintroduce it. Nothing else changed; the extraction, the migration and the four log-state cases are as verified above.

ahmad changed title from ~~WIP: chore: Extract the failure-tail reporter into an action~~ to chore: Extract the failure-tail reporter into an action

2026-09-05 06:37:58 +00:00

ahmad commented

2026-09-05 06:38:00 +00:00

validate green on 61dc712; back to ready.

The delta since the approved-in-substance head is the two review fixes, both verified above: the curl guard measured with and without on a PATH that genuinely lacks it, and the masking warning moved into the README and the action's description: where an adopter meets it.

Everything else is unchanged and stands as verified: the four log states, the three "cannot post" paths all exiting 0, the migration of this repository's own validate, and the two mutations — with the broken first attempt described rather than quietly repaired.

`validate` green on `61dc712`; back to ready. The delta since the approved-in-substance head is the two review fixes, both verified above: the `curl` guard measured with and without on a `PATH` that genuinely lacks it, and the masking warning moved into the README and the action's `description:` where an adopter meets it. Everything else is unchanged and stands as verified: the four log states, the three "cannot post" paths all exiting 0, the migration of this repository's own `validate`, and the two mutations — with the broken first attempt described rather than quietly repaired.

ahmad_bot approved these changes

2026-09-05 07:02:08 +00:00

ahmad_bot left a comment

Approved. Both findings are closed. Re-reviewed only the delta (ed5afd80d2..61dc712d03); CI green.

The masking warning went further than I asked. I suggested the README and said the description: was worth considering; it is in both. The description is what someone reads when they go looking for the inputs, and it is the only part a consumer sees without leaving their own workflow file — so that is the copy that will actually prevent the leak. The README wording is also better than mine for naming it as the hazard adoption invites rather than a defect in the action.

Verified by driving the extracted script with a genuinely minimal PATH, containing only cat, rm, tail, tr and — where the case called for it — jq:

curl absent                curl is not installed, so the failure comment cannot be posted.   exit 0
curl absent, jq present    same                                                              exit 0
empty pr-number            No pull request number… (a push build).                           exit 0
log absent                 (falls through to the no-output message)                          exit 0
log empty                  same                                                              exit 0

No network call is even attempted once curl is missing, and every path still exits 0 — the third criterion holds across the new early exits, which is the thing a new exit in a reporter can quietly break.

Placing the curl guard before the jq one is the right order: with both missing you are told about the tool that would have done the posting rather than the one that would have formatted it.

A correction to my own process, worth recording. My first attempt at this verification was wrong: I built stub directories but left /usr/bin on PATH, so both tools were still found, neither guard was exercised, and all three "cases" returned an identical HTTP 401 — from a real request to the live API with a dummy token. It looked like evidence and was not. The run above uses env -i with an explicit PATH and bash invoked absolutely, which is what actually removes a dependency. Flagging it because a harness that silently tests nothing is the failure mode this repository's own reviews keep turning up, and I am not exempt from it.

Approved. Both findings are closed. Re-reviewed only the delta (`ed5afd80d2..61dc712d03`); CI green. **The masking warning went further than I asked.** I suggested the README and said the `description:` was worth considering; it is in both. The description is what someone reads when they go looking for the inputs, and it is the only part a consumer sees without leaving their own workflow file — so that is the copy that will actually prevent the leak. The README wording is also better than mine for naming it as the hazard adoption invites rather than a defect in the action. **Verified by driving the extracted script with a genuinely minimal `PATH`**, containing only `cat`, `rm`, `tail`, `tr` and — where the case called for it — `jq`: ``` curl absent curl is not installed, so the failure comment cannot be posted. exit 0 curl absent, jq present same exit 0 empty pr-number No pull request number… (a push build). exit 0 log absent (falls through to the no-output message) exit 0 log empty same exit 0 ``` No network call is even attempted once `curl` is missing, and every path still exits 0 — the third criterion holds across the new early exits, which is the thing a new `exit` in a reporter can quietly break. Placing the `curl` guard before the `jq` one is the right order: with both missing you are told about the tool that would have done the posting rather than the one that would have formatted it. **A correction to my own process, worth recording.** My first attempt at this verification was wrong: I built stub directories but left `/usr/bin` on `PATH`, so both tools were still found, neither guard was exercised, and all three "cases" returned an identical `HTTP 401` — from a real request to the live API with a dummy token. It looked like evidence and was not. The run above uses `env -i` with an explicit `PATH` and `bash` invoked absolutely, which is what actually removes a dependency. Flagging it because a harness that silently tests nothing is the failure mode this repository's own reviews keep turning up, and I am not exempt from it.

ahmad commented

2026-09-05 07:03:14 +00:00

Merging with a feat: squash title rather than chore:, and the reason is load-bearing rather than cosmetic.

release@v3 derives its bump from the squash subject: feat: minor, fix: patch, anything else BUMP=none. No release means the v3 tag does not move — it currently points at 6ebe5524, which does not contain report-job-failure/. So merging this as a chore would publish a README telling consumers to write

uses: https://git.ahmadelmasri.com/amtronics/platform-actions/report-job-failure@v3

against a tag where that path does not exist, and the three consumer migrations would fail on their first run.

A new shared action that other repositories will consume is a feature by any reading. The issue is labelled type/chore because the work was consolidation, but the label describes the work and the commit type decides whether it ships — which is the distinction I got wrong on ahmad/expiro#21 this morning, where a type/chore label led me to merge a caller-visible fix as chore: and cut no release. Harmless there because that repository's publish is gated off; here it would leave the action unreachable at the version its own documentation names.

Everything else is as reviewed: approved on 61dc712, validate green, both review findings fixed and verified.

Merging with a **`feat:`** squash title rather than `chore:`, and the reason is load-bearing rather than cosmetic. `release@v3` derives its bump from the squash subject: `feat:` minor, `fix:` patch, anything else `BUMP=none`. No release means the `v3` tag does not move — it currently points at `6ebe5524`, which **does not contain `report-job-failure/`**. So merging this as a chore would publish a README telling consumers to write ```yaml uses: https://git.ahmadelmasri.com/amtronics/platform-actions/report-job-failure@v3 ``` against a tag where that path does not exist, and the three consumer migrations would fail on their first run. A new shared action that other repositories will consume is a feature by any reading. The issue is labelled `type/chore` because the *work* was consolidation, but the label describes the work and the commit type decides whether it ships — which is the distinction I got wrong on `ahmad/expiro#21` this morning, where a `type/chore` label led me to merge a caller-visible fix as `chore:` and cut no release. Harmless there because that repository's publish is gated off; here it would leave the action unreachable at the version its own documentation names. Everything else is as reviewed: approved on `61dc712`, `validate` green, both review findings fixed and verified.

ahmad merged commit 71fb9d5ad8 into main

2026-09-05 07:03:17 +00:00

ahmad referenced this pull request from a commit

2026-09-05 07:03:18 +00:00

feat: Extract the failure-tail reporter into an action (#45)

ahmad deleted branch feature/38-report-job-failure

2026-09-05 07:03:23 +00:00

ahmad_bot commented

2026-09-05 08:01:01 +00:00

Correcting something I wrote in the review above.

This repository has no pipeline that would have caught the difference — every change here reaches four consumers unexecuted

The first half is wrong. This repository does have a pull request pipeline: validate installs a YAML parser, tests the checker, runs it over the action definitions, and lints the README — and it ran green on both heads of this pull request. I carried that claim from a stale note of mine that predates it.

The half that stands, and the half the point rested on, is that validate does not execute an action against a real consumer job. So a change here still reaches four consumers unexercised, and uses: ./report-job-failure remains the only end-to-end test available — which is what made using it the right call. The conclusion is unchanged; the reason I gave for it was overstated.

Correcting something I wrote in the review above. > This repository has no pipeline that would have caught the difference — every change here reaches four consumers unexecuted The first half is wrong. This repository does have a pull request pipeline: `validate` installs a YAML parser, tests the checker, runs it over the action definitions, and lints the README — and it ran green on both heads of this pull request. I carried that claim from a stale note of mine that predates it. The half that stands, and the half the point rested on, is that `validate` does not *execute* an action against a real consumer job. So a change here still reaches four consumers unexercised, and `uses: ./report-job-failure` remains the only end-to-end test available — which is what made using it the right call. The conclusion is unchanged; the reason I gave for it was overstated.

Sign in to join this conversation.

No reviewers