amtronics/platform-actions

Watch

Fork 0

feat: Expose the security scan output so consumers can report it #32

Merged

ahmad merged 1 commit from feature/29-security-scan-log into main

2026-09-04 06:04:15 +00:00

ahmad commented

2026-09-04 05:05:36 +00:00

Owner

Issue

Closes #29. Also unblocks ahmad/expiro#25's fourth criterion, which I left visibly unmet because a consuming workflow cannot reach inside a composite action.

Problem

A red security job is a bare word. Forgejo serves no job logs over its API, and the workaround used elsewhere — tee the output, post the tail from an if: failure() step — cannot be applied by a consumer here, because the whole job is uses: .../security-scan@v3 and there is nothing in the calling workflow to redirect.

Solution

Every scanner tees into /tmp/security-scan.log, declared as the log-path output.

Three decisions worth stating:

log-path is a literal, not a step's GITHUB_OUTPUT. A step setting it would need if: always() to run once a scan has failed the job — which is precisely when a consumer wants the log. A constant cannot fail to be set.
The log is truncated when the action starts. A runner reuses /tmp between jobs, and a tail showing an earlier job's failure is worse than no tail at all.
pipefail is what makes this safe, and every step already had it. Piping a scanner into tee without it means the step exits with tee's status — a finding reported as a clean scan.

Review notes

I tested the composed form rather than the shape, by extracting each step's real run: body from the YAML, substituting the expressions the runner would, and executing it under bash -e (the runner injects -e) against stub scanners with controlled exit codes.

Every exit code is preserved:

step	tool exit	enforce=true	enforce=false
gitleaks	0 / 1	0 / 1	0 / 0
osv-scanner	0 / 1	0 / 1	0 / 0
osv-scanner	128 (nothing to scan)	0	0
osv-scanner	2 (real failure)	2	2
trivy	0 / 1	0 / 1	—

The osv 128 and 2 rows are the ones I most wanted to see: "nothing to scan is not an error" and "a scanner that could not scan is not a pass" both survive the change. Tool output reached the log in every case.

The negative control, which is the real evidence:

pipefail removed, enforce=true, a finding present:
  Secrets       -> exit=0
  Dependencies  -> exit=0
  Image         -> exit=0

All three security scans silently pass. That is the trap this change would have introduced if the steps had not already set pipefail, and it is why I checked rather than reasoned.

The checker passes over all six actions with unchanged step counts, and README lint is clean.

Risks and trade-offs

The tail is read from a file, so the runner's log masking does not apply to it. gitleaks runs with --redact and nothing else here echoes a secret, but a future step that did would leak into a comment. Noted in the README next to the example.
Merging moves v3, and this time the actions do change — every consumer gets the tee. The behaviour they see is identical unless they read the new output; the risk is the pipefail interaction above, which is why it is tested rather than argued.
The trivy step still exits with the tool's status unconditionally, so in report-only mode it relies on --exit-code 0 rather than on the enforce branch. Pre-existing and unchanged; worth knowing, not worth fixing here.
Consumers still have to wire up the if: failure() step themselves. The README now carries the pattern, including the guard that stops an absent log posting an empty code fence — a mistake I made in expiro#26 and had to fix.

### Issue Closes #29. Also unblocks `ahmad/expiro#25`'s fourth criterion, which I left visibly unmet because a consuming workflow cannot reach inside a composite action. ### Problem A red `security` job is a bare word. Forgejo serves no job logs over its API, and the workaround used elsewhere — tee the output, post the tail from an `if: failure()` step — cannot be applied by a consumer here, because the whole job is `uses: .../security-scan@v3` and there is nothing in the calling workflow to redirect. ### Solution Every scanner tees into `/tmp/security-scan.log`, declared as the `log-path` output. Three decisions worth stating: - **`log-path` is a literal, not a step's `GITHUB_OUTPUT`.** A step setting it would need `if: always()` to run once a scan has failed the job — which is precisely when a consumer wants the log. A constant cannot fail to be set. - **The log is truncated when the action starts.** A runner reuses `/tmp` between jobs, and a tail showing an earlier job's failure is worse than no tail at all. - **`pipefail` is what makes this safe**, and every step already had it. Piping a scanner into `tee` without it means the step exits with `tee`'s status — a finding reported as a clean scan. ### Review notes **I tested the composed form rather than the shape**, by extracting each step's real `run:` body from the YAML, substituting the expressions the runner would, and executing it under `bash -e` (the runner injects `-e`) against stub scanners with controlled exit codes. Every exit code is preserved: | step | tool exit | enforce=true | enforce=false | |---|---|---|---| | gitleaks | 0 / 1 | 0 / **1** | 0 / 0 | | osv-scanner | 0 / 1 | 0 / **1** | 0 / 0 | | osv-scanner | 128 (nothing to scan) | 0 | 0 | | osv-scanner | 2 (real failure) | **2** | **2** | | trivy | 0 / 1 | 0 / **1** | — | The osv `128` and `2` rows are the ones I most wanted to see: "nothing to scan is not an error" and "a scanner that could not scan is not a pass" both survive the change. Tool output reached the log in every case. **The negative control, which is the real evidence:** ``` pipefail removed, enforce=true, a finding present: Secrets -> exit=0 Dependencies -> exit=0 Image -> exit=0 ``` All three security scans silently pass. That is the trap this change would have introduced if the steps had not already set `pipefail`, and it is why I checked rather than reasoned. The checker passes over all six actions with unchanged step counts, and README lint is clean. ### Risks and trade-offs - **The tail is read from a file, so the runner's log masking does not apply to it.** gitleaks runs with `--redact` and nothing else here echoes a secret, but a future step that did would leak into a comment. Noted in the README next to the example. - Merging moves `v3`, and this time the actions **do** change — every consumer gets the tee. The behaviour they see is identical unless they read the new output; the risk is the `pipefail` interaction above, which is why it is tested rather than argued. - The trivy step still exits with the tool's status unconditionally, so in report-only mode it relies on `--exit-code 0` rather than on the `enforce` branch. Pre-existing and unchanged; worth knowing, not worth fixing here. - Consumers still have to wire up the `if: failure()` step themselves. The README now carries the pattern, including the guard that stops an absent log posting an empty code fence — a mistake I made in `expiro#26` and had to fix.

ahmad added 1 commit

2026-09-04 05:05:36 +00:00

feat: Expose the security scan output so consumers can report it

PR / validate (pull_request) Successful in 9m9s

Details

dcb19924e4

Forgejo serves no job logs over its API, so a red security job was a
bare word: the whole job is a composite action, and a consuming
workflow cannot redirect the output of one. Every scanner now tees
into a path the action declares as log-path.

Declared as a literal rather than threaded through a step's
GITHUB_OUTPUT. A step setting it would need if: always() to run once a
scan has already failed the job, which is exactly when the log is
wanted; a constant cannot fail to be set.

The log is truncated when the action starts, because a runner reuses
/tmp between jobs and a tail showing an earlier failure is worse than
no tail.

Every step already set pipefail, which is what keeps the tee from
swallowing a finding. Verified rather than assumed: with pipefail
removed, all three steps report success on a finding in enforce
mode.

ahmad commented

2026-09-04 05:32:51 +00:00

Author

Owner

Verification against #29's acceptance criteria

PR / validate green on dcb1992; the checker passes all six actions with unchanged step counts and README lint is clean.

Given the action runs, when it finishes, then its combined output is at a path the action exposes and the README names.
/tmp/security-scan.log, declared as the log-path output and documented in the README's output table. Every scanner tees into it; the harness below confirms tool output reached the file in all fourteen cases.

Given a step inside the action fails, when the job ends, then that path still holds the output written before the failure.
This is the case the design turns on. The output is a literal, not a value threaded through a step — a step setting it would need if: always() to run once a scan has already failed the job, which is precisely when a consumer wants the log. Verified at enforce=true with a finding: the step exits 1 and the log holds the scanner's output.

Given a consuming workflow reads that path in an if: failure() step, when the job fails, then the tail can be posted without reading the run log.
The path is fixed and the README carries the pattern, including the guard I got wrong in expiro#26: check [ -s log ] rather than > file || true, because the redirection creates the file and an absent log otherwise reads back as an empty string and posts an empty code fence.

Given the action pipes output to tee, when a piped command fails, then the step still fails.
Every step already set pipefail, which is what makes this change safe rather than catastrophic. Proved by removing it:

pipefail removed, enforce=true, a finding present:
  Secrets       -> exit=0
  Dependencies  -> exit=0
  Image         -> exit=0

All three security scans silently pass. That is the failure this criterion exists to prevent, and it was one set -o away.

How it was tested

Each step's real run: body was extracted from the YAML, its expressions substituted as the runner would, and executed under bash -e — the runner injects -e, which is why these steps capture exit codes inline — against stub scanners with controlled exit codes.

step	tool exit	enforce=true	enforce=false
gitleaks	0 / 1	0 / 1	0 / 0
osv-scanner	0 / 1	0 / 1	0 / 0
osv-scanner	128	0	0
osv-scanner	2	2	2
trivy	0 / 1	0 / 1	—

The two osv rows are the ones worth reading: 128 stays "nothing to scan is not an error", and 2 stays "a scanner that could not scan is not a pass" in both modes. Neither is something the tee could have been allowed to change.

What this does not do

Consumers still wire up their own if: failure() step; this only makes it possible. ahmad/expiro#25 names that follow-up and its fourth criterion is what this unblocks.

The trivy step exits with the tool's status unconditionally, so report-only mode depends on --exit-code 0 rather than on the enforce branch. Pre-existing, unchanged, and visible in the table above as the missing cell.

### Verification against #29's acceptance criteria `PR / validate` green on `dcb1992`; the checker passes all six actions with unchanged step counts and README lint is clean. **Given the action runs, when it finishes, then its combined output is at a path the action exposes and the README names.** `/tmp/security-scan.log`, declared as the `log-path` output and documented in the README's output table. Every scanner tees into it; the harness below confirms tool output reached the file in all fourteen cases. **Given a step inside the action fails, when the job ends, then that path still holds the output written before the failure.** This is the case the design turns on. The output is a **literal**, not a value threaded through a step — a step setting it would need `if: always()` to run once a scan has already failed the job, which is precisely when a consumer wants the log. Verified at `enforce=true` with a finding: the step exits 1 and the log holds the scanner's output. **Given a consuming workflow reads that path in an `if: failure()` step, when the job fails, then the tail can be posted without reading the run log.** The path is fixed and the README carries the pattern, including the guard I got wrong in `expiro#26`: check `[ -s log ]` rather than `> file || true`, because the redirection creates the file and an absent log otherwise reads back as an empty string and posts an empty code fence. **Given the action pipes output to `tee`, when a piped command fails, then the step still fails.** Every step already set `pipefail`, which is what makes this change safe rather than catastrophic. Proved by removing it: ``` pipefail removed, enforce=true, a finding present: Secrets -> exit=0 Dependencies -> exit=0 Image -> exit=0 ``` All three security scans silently pass. That is the failure this criterion exists to prevent, and it was one `set -o` away. ### How it was tested Each step's real `run:` body was extracted from the YAML, its expressions substituted as the runner would, and executed under `bash -e` — the runner injects `-e`, which is why these steps capture exit codes inline — against stub scanners with controlled exit codes. | step | tool exit | enforce=true | enforce=false | |---|---|---|---| | gitleaks | 0 / 1 | 0 / **1** | 0 / 0 | | osv-scanner | 0 / 1 | 0 / **1** | 0 / 0 | | osv-scanner | 128 | 0 | 0 | | osv-scanner | 2 | **2** | **2** | | trivy | 0 / 1 | 0 / **1** | — | The two osv rows are the ones worth reading: `128` stays "nothing to scan is not an error", and `2` stays "a scanner that could not scan is not a pass" in **both** modes. Neither is something the tee could have been allowed to change. ### What this does not do Consumers still wire up their own `if: failure()` step; this only makes it possible. `ahmad/expiro#25` names that follow-up and its fourth criterion is what this unblocks. The trivy step exits with the tool's status unconditionally, so report-only mode depends on `--exit-code 0` rather than on the `enforce` branch. Pre-existing, unchanged, and visible in the table above as the missing cell.

ahmad changed title from ~~WIP: feat: Expose the security scan output so consumers can report it~~ to feat: Expose the security scan output so consumers can report it

2026-09-04 05:32:52 +00:00

ahmad_bot approved these changes

2026-09-04 06:02:54 +00:00

ahmad_bot left a comment

Approving. I reproduced the whole table independently, including the negative control, which is the part that matters here.

Every teed step has pipefail, which I checked by parsing the action rather than reading the diff — all three tee twice, all three set it:

step 1 Secrets (gitleaks)         tees=2  pipefail=True
step 2 Dependencies (osv-scanner) tees=2  pipefail=True
step 3 Image (trivy)              tees=2  pipefail=True

Exit codes survive the pipe, running each step's real run: body under bash -e against stub scanners:

gitleaks  clean/finding, enforce=true   -> 0 / 1        finding, enforce=false -> 0
osv       clean/findings, enforce=true  -> 0 / 1        findings, enforce=false -> 0
osv       128 nothing to scan           -> 0            2 real failure          -> 2
trivy     clean/finding, enforce=true   -> 0 / 1        clean, enforce=false    -> 0

The osv 128 and 2 rows behave as you said: nothing to scan is not an error, and a scanner that could not scan is not a pass.

The negative control, reproduced:

pipefail removed, enforce=true, finding present:  step1 -> 0   step2 -> 0   step3 -> 0
pipefail present, same inputs:                    step1 -> 1   step2 -> 1   step3 -> 1

All three security scans silently pass without it. Testing that rather than arguing it is the right instinct, and it is why "every step already had pipefail" is worth having verified rather than assumed — it is the single property this change depends on.

(My first trivy run showed exit=1, log=0B on the clean case and I nearly reported it. It was my harness: I had not set THRESHOLD, and set -u kills the step at line 2. Worth mentioning only because a reviewer's broken fixture looks exactly like a real finding.)

Against #29: the path is exposed as an output and named in the README; the log holds what was written before a failure, since each scanner tees as it runs rather than at the end; the README carries the consuming if: failure() pattern; and pipefail is set on every piped step, measured above. All four hold.

Two things I like more than the change itself. Making log-path a literal rather than a step output — a step setting it would need if: always() to survive the very failure the consumer wants to read, and a constant cannot fail to be set. And carrying the empty-log guard into the README example, so the mistake from expiro#26 is not the first thing every consumer copies.

One question I could not answer, so I am not making it a finding. The log is a fixed /tmp/security-scan.log, and you truncate on the premise that a runner reuses /tmp between jobs. I could not determine from infrastructure-vps whether this runner executes jobs in per-job containers or shares a filesystem — if it shares, two security jobs running concurrently would interleave into one file. The consequence is a mixed tail on a failure, not a wrong exit code, so it is diagnostic quality rather than correctness. You are better placed to know which model the runner uses; if it shares, ${GITHUB_RUN_ID} in the filename would settle it.

Merging moves v3, and this time every consumer's actions genuinely change. Approval does not merge.

Approving. I reproduced the whole table independently, including the negative control, which is the part that matters here. **Every teed step has `pipefail`**, which I checked by parsing the action rather than reading the diff — all three tee twice, all three set it: ``` step 1 Secrets (gitleaks) tees=2 pipefail=True step 2 Dependencies (osv-scanner) tees=2 pipefail=True step 3 Image (trivy) tees=2 pipefail=True ``` **Exit codes survive the pipe**, running each step's real `run:` body under `bash -e` against stub scanners: ``` gitleaks clean/finding, enforce=true -> 0 / 1 finding, enforce=false -> 0 osv clean/findings, enforce=true -> 0 / 1 findings, enforce=false -> 0 osv 128 nothing to scan -> 0 2 real failure -> 2 trivy clean/finding, enforce=true -> 0 / 1 clean, enforce=false -> 0 ``` The osv `128` and `2` rows behave as you said: nothing to scan is not an error, and a scanner that could not scan is not a pass. **The negative control, reproduced:** ``` pipefail removed, enforce=true, finding present: step1 -> 0 step2 -> 0 step3 -> 0 pipefail present, same inputs: step1 -> 1 step2 -> 1 step3 -> 1 ``` All three security scans silently pass without it. Testing that rather than arguing it is the right instinct, and it is why "every step already had `pipefail`" is worth having verified rather than assumed — it is the single property this change depends on. (My first trivy run showed `exit=1, log=0B` on the clean case and I nearly reported it. It was my harness: I had not set `THRESHOLD`, and `set -u` kills the step at line 2. Worth mentioning only because a reviewer's broken fixture looks exactly like a real finding.) **Against #29:** the path is exposed as an output and named in the README; the log holds what was written before a failure, since each scanner tees as it runs rather than at the end; the README carries the consuming `if: failure()` pattern; and `pipefail` is set on every piped step, measured above. All four hold. **Two things I like more than the change itself.** Making `log-path` a literal rather than a step output — a step setting it would need `if: always()` to survive the very failure the consumer wants to read, and a constant cannot fail to be set. And carrying the empty-log guard into the README example, so the mistake from `expiro#26` is not the first thing every consumer copies. **One question I could not answer, so I am not making it a finding.** The log is a fixed `/tmp/security-scan.log`, and you truncate on the premise that a runner reuses `/tmp` between jobs. I could not determine from `infrastructure-vps` whether this runner executes jobs in per-job containers or shares a filesystem — if it shares, two `security` jobs running concurrently would interleave into one file. The consequence is a mixed tail on a failure, not a wrong exit code, so it is diagnostic quality rather than correctness. You are better placed to know which model the runner uses; if it shares, `${GITHUB_RUN_ID}` in the filename would settle it. Merging moves `v3`, and this time every consumer's actions genuinely change. Approval does not merge.

ahmad commented

2026-09-04 06:04:12 +00:00

Author

Owner

Merging. Answering your question first, because it deserves a real answer rather than a shrug.

I could not determine the runner's filesystem model either, so I looked at what is actually reachable instead:

ahmad/expiro    security job: containerised (container: node:22-bookworm)
ahmad/portfolio security job: none

The only consumer of this action runs it inside a per-job container, so /tmp is that container's and the interleaving you describe is not reachable today. That is a fact about the consumers rather than about the runner, so it answers the practical question without answering yours.

I am not making the ${GITHUB_RUN_ID} change now, and the reason is worth stating: RUNNER_TEMP and runner.temp are referenced nowhere in this repository or the fleet, so their behaviour on this runner is untested. If runner.temp expands to empty, the output becomes /security-scan.log while the shell writes somewhere else — a silently wrong path, which is a worse failure than the mixed tail it would prevent. Swapping a known-good constant for an unverified expression on an approved change, to fix something no current consumer can hit, is the wrong trade.

Filed as #33 with both candidate fixes and the evidence above, so whoever picks it up can test runner.temp on the runner first and then choose. It becomes real the moment a consumer adds an uncontainerised security job, which is exactly when someone will be reading that issue.

On your harness note — thank you for including it. A reviewer's broken fixture looking exactly like a real finding is the same shape as the mistake I made on expiro#28, where an ALTER that silently did nothing made a real test look vacuous. Twice in one night, from both directions.

On v3. Unlike #30, this one changes the actions themselves: every consumer gets the tee. The behaviour they see is unchanged unless they read the new output, and the property that guarantees that — pipefail on every piped step — is the one we have now both measured independently.

Merging. Answering your question first, because it deserves a real answer rather than a shrug. **I could not determine the runner's filesystem model either**, so I looked at what is actually reachable instead: ``` ahmad/expiro security job: containerised (container: node:22-bookworm) ahmad/portfolio security job: none ``` The only consumer of this action runs it inside a per-job container, so `/tmp` is that container's and the interleaving you describe is not reachable today. That is a fact about the consumers rather than about the runner, so it answers the practical question without answering yours. **I am not making the `${GITHUB_RUN_ID}` change now**, and the reason is worth stating: `RUNNER_TEMP` and `runner.temp` are referenced nowhere in this repository or the fleet, so their behaviour on this runner is untested. If `runner.temp` expands to empty, the output becomes `/security-scan.log` while the shell writes somewhere else — a silently wrong path, which is a worse failure than the mixed tail it would prevent. Swapping a known-good constant for an unverified expression on an approved change, to fix something no current consumer can hit, is the wrong trade. Filed as `#33` with both candidate fixes and the evidence above, so whoever picks it up can test `runner.temp` on the runner first and then choose. It becomes real the moment a consumer adds an uncontainerised `security` job, which is exactly when someone will be reading that issue. **On your harness note** — thank you for including it. A reviewer's broken fixture looking exactly like a real finding is the same shape as the mistake I made on `expiro#28`, where an `ALTER` that silently did nothing made a real test look vacuous. Twice in one night, from both directions. **On `v3`.** Unlike `#30`, this one changes the actions themselves: every consumer gets the tee. The behaviour they see is unchanged unless they read the new output, and the property that guarantees that — `pipefail` on every piped step — is the one we have now both measured independently.

ahmad merged commit f90809148a into main

2026-09-04 06:04:15 +00:00

ahmad referenced this pull request from a commit

2026-09-04 06:04:16 +00:00

feat: Expose the security scan output so consumers can report it (#32)

ahmad referenced this pull request

2026-09-04 06:04:44 +00:00

chore: Make the security-scan log path safe for uncontainerised jobs #33