sneak/webhooker
1
0
Fork 0
Code Issues 34 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Mark superseded commits honestly instead of skipped (closes #152) #161

Merged
clawbot merged 1 commits from issue-152-superseded-status into next 2026-08-18 00:31:55 +02:00
Conversation 4 Commits 1 Files Changed 8 +750 -34
clawbot commented 2026-08-17 22:58:22 +02:00
Collaborator
Copy Link

Closes #152.

Also implements item 2 of #147 (the hardcoded status context), because it edits the same workflow step. Item 1 there — the script/cibuild header drift against the model in sneak/prompts — is untouched and that issue stays open.

What changed

  • The status rewrite moves out of .gitea/workflows/check.yml into script/ci-mark-superseded, which the workflow now calls.
  • A superseded commit is marked failure / Superseded by a newer commit; never tested instead of skipped / same description. Never-tested no longer folds into a combined success.
  • The script also repairs the skipped statuses the previous revision wrote, as the walk passes over them.
  • The context string is derived from GITHUB_WORKFLOW, GITHUB_JOB and GITHUB_EVENT_NAME, and the script fails the step when no status on the commit being built carries that context.
  • Nothing about the walk fails quietly (see below).
  • Tests in internal/ciscript run the real script against a fake Gitea; jq is added to the builder stage so they execute in CI rather than skipping.
  • README's "CI gate honesty" section states how to read each of the three status shapes.

Nothing in the walk fails quietly

The script exists to stop CI lying quietly, so every failure path in it is loud:

  • ANCESTOR_LIMIT. It is a documented knob, so a value that is set but unparseable aborts with ANCESTOR_LIMIT must be a positive integer, got 'twenty'. Previously it went straight to git rev-list, which printed fatal: 'twenty': not an integer into an exit status discarded by || true; the walk came back empty and the step exited 0 having marked nothing — set-but-unparseable config silently defaulting, the class this repo already rejected in #80. The expansion is ${ANCESTOR_LIMIT-20}, not ${ANCESTOR_LIMIT:-20}, so an explicitly empty value is a config error like any other bad value rather than a silent fall-back to 20; unset still means 20.
  • A shallow clone. git rev-parse "${GITHUB_SHA}^" cannot resolve the parent across a shallow graft, so depth-1 is indistinguishable from a root commit and would exit 0 having walked nothing (depth > 1 walks only the ancestors that happen to be present). git rev-parse --is-shallow-repository separates the two: a shallow repository aborts with shallow repository: the ancestor walk needs full history, a genuine root commit still exits 0 with no ancestor of <sha> to check, which is a legitimate case and not an error. The workflow's fetch-depth: 0 comment now records that this step depends on it, alongside the fingerprint step.
  • The walk itself. || true is gone, so a rev-list failure aborts. A SHA the repository does not have is not one of those: its parent is equally unresolvable, so it takes the root-commit branch — but require_own_context has already aborted on Gitea's 404 for that commit, which is where that case is actually caught. The source comment now says so rather than claiming rev-list covers it.
  • Per-ancestor status reads. status_of carries the same --retry 3 --retry-delay 2 --max-time 30 as the head-commit read, captures curl's exit status instead of losing it through a pipe into jq, and aborts with cannot read commit statuses for plus the SHA. Previously an HTTP 500 on an ancestor read yielded exit 0, no POST and no message: the laundered commit stayed laundered with zero signal, and an unbounded read could hang the step.

Probed against a fake API under dash, on the pushed script:

depth-1 clone   shallow repository: the ancestor walk needs full history   rc=1  POSTS=0
root commit     no ancestor of b225050... to check                          rc=0  POSTS=0
ANCESTOR_LIMIT= ANCESTOR_LIMIT must be a positive integer, got ''           rc=1  POSTS=0
ANCESTOR_LIMIT unset (3-commit history)   marked 2 ancestors                rc=0  POSTS=2
ANCESTOR_LIMIT=1                          marked 1 ancestor                 rc=0
ANCESTOR_LIMIT=twenty                     must be a positive integer        rc=1

All three loud-failure paths carry a regression test: TestMarkSupersededRejectsAnUnparseableAncestorLimit, TestMarkSupersededFailsOnAnUnreadableAncestorStatus and TestMarkSupersededRejectsAShallowRepository.

Option taken: 2, with the evidence for rejecting 1

Option 1 (re-run the check on the superseded commit) is not reachable on this Gitea. Evidence, from the instance itself and from the source of the version it runs:

  • https://git.eeqj.de/api/v1/version reports 1.25.4.
  • Its swagger (https://git.eeqj.de/swagger.v1.json) exposes, under /repos/{owner}/{repo}/actions/, only runs (get), runs/{run} (get, delete), runs/{run}/jobs, runs/{run}/artifacts, jobs/{job_id}, jobs/{job_id}/logs, workflows, workflows/{id}/enable|disable, and workflows/{id}/dispatches. There is no rerun endpoint for a run or a job.
  • The only trigger available, workflows/{id}/dispatches, takes ref (required, refs/heads/...), not a SHA — so a historical commit can only be re-run by creating a throwaway ref at it. That run's event is workflow_dispatch, so Gitea records it under the context check / check (workflow_dispatch), a different context from the (push) one that carries the cancellation; the false state would remain and would have to be overwritten by hand anyway.
  • Cost is unbounded in the number of superseded commits. .ci-fingerprint changes with every commit that touches the build context, so each replay is a full uncached build — the honest reference point is be57609, Successful in 2m52s. A burst of N merges would queue N such builds ahead of the head commit's own run on the shared runner, and the current walk reaches 20 ancestors.
  • The cancellation itself cannot be avoided either: in services/actions/notifier_helper.go at v1.25.4, CancelPreviousJobs(repoID, ref, workflowID, event) is called for every push and pull_request_sync run with no workflow-file escape hatch, so "let each run finish per-commit" is not configurable.

Option 3 is what is in place today and is what the issue rejects, since skipped reads as green.

Why failure and not something quieter

From modules/commitstatus/commit_status.go at v1.25.4, Combine() returns failure if any context is error or failure; counts success, warning and skipped as successes; and returns pending otherwise. So skipped and warning both fold into green, and pending never clears. failure is the only state that is neither false-green nor permanently blocking.

Reproduced on next before the change, via the public API:

0e397b3  {"state":"success", statuses:[{"status":"skipped","description":"Superseded by a newer commit; never tested"}]}
95161c7  {"state":"success", statuses:[{"status":"skipped","description":"Superseded by a newer commit; never tested"}]}
be57609  {"state":"success", statuses:[{"status":"success","description":"Successful in 2m52s"}]}

Bisect archaeology after the change reads by description, all three distinguishable: Successful in ... (ran, passed), Failing after ... (ran, failed), Superseded by a newer commit; never tested (never ran). The last of those covers a manual cancellation too — the walk only ever reaches ancestors of the commit being built, so anything it can touch is by construction superseded by a newer commit, and Gitea records both causes identically as failure / Has been cancelled. The README says "cancelled, by a newer push or by hand".

Verified end-to-end on the live tracker

CI run 182, on the earlier revision e875c3e, executed the script against the real instance:

  • 0e397b3, 95161c7 and 9ae1915 flipped from combined success to failure / Superseded by a newer commit; never tested at 23:00:16.
  • be57609's genuine Successful in 2m52s was untouched.
  • require_own_context passed on the real runner, which is empirical proof that the derived context matches the live check / check (push).

How the status behaviour is validated in tests

make check alone does not exercise workflow behaviour, so the logic lives in a script and the tests execute that script — the shipped artifact, not a copy. internal/ciscript starts an httptest server that serves Gitea's combined-status endpoint and records create-status POSTs (latest status per context wins, as in Gitea), builds a throwaway two-commit git history, and runs sh script/ci-mark-superseded against it:

  • a cancelled run is marked — failure / Has been cancelled becomes failure / Superseded by a newer commit; never tested.
  • a laundered skipped status is marked — the previous revision's skipped status is repaired to failure.
  • a genuine failure is left alone, a passing run is left alone, another context is left alone.
  • TestMarkSupersededIsIdempotent — running twice posts exactly one status.
  • TestMarkSupersededRejectsAnUnparseableAncestorLimit, TestMarkSupersededFailsOnAnUnreadableAncestorStatus and TestMarkSupersededRejectsAShallowRepository — the three loud-failure paths above. The last one clones the throwaway history at --depth=1 and asserts the script exits non-zero, says shallow repository, and posts nothing.

internal/ciscript execs a file outside the Go build graph, so go test's result cache can serve a stale PASS after a script-only edit; doc.go records that, because a host make test is not evidence in that case.

Derived context, covering the #147 item 2 half:

  • TestDerivedContextMatchesGitea parses the checked-in .gitea/workflows/check.yml, takes its name and its single job id, feeds them to the script as the runner would, and asserts the status it posts carries check / check (push) — byte-identical to the string the old step hardcoded and to the context the live API shows on be57609.
  • TestMarkSupersededRejectsAnUnknownContext renames the job and asserts the script exits non-zero, prints the contexts actually present, and posts nothing.

The derivation is deliberately not byte-exact with Gitea's rule, and the script header and the README both say so so nobody later "fixes" it into a silent fallback. Gitea builds the context in services/actions/commit_status.go at v1.25.4 from the job's display name: (falling back to the job id) and the workflow's name: (falling back to the filename), while the runner exports GITHUB_JOB as the job id and GITHUB_WORKFLOW as the parsed workflow name:. Adding a display name: to the job, or dropping the workflow's name:, therefore turns every push red with a message rather than silently no-opping — which is exactly the loud failure #147 item 2 asked for.

The two properties of #119 are intact

  1. CI executes the checks it reports on. Untouched. The Fingerprint the build context step, .ci-fingerprint, its .dockerignore exemption and the COPY . . barrier in both check stages are unchanged; this PR only edits the step above them. The gate run below shows both stages really executing.
  2. No commit is left permanently pending. The script writes only failure and never pending. Branch protection evaluates the head commit of a PR, so a failure on a superseded ancestor blocks nothing.

Gate evidence

Rebased onto next at bef9986. An earlier rebase (onto 39064a3) conflicted in README's "CI gate honesty" section; resolved by keeping that commit's correction — the step runs before the fingerprint is written — and this branch's failure wording on top.

docker build --no-cache-filter=lint --no-cache-filter=builder . on the rebased tree, exit 0, cache defeated on both check stages:

#20 [lint 7/8]      RUN make fmt-check   DONE  5.1s
#21 [lint 8/8]      RUN make lint        DONE 59.4s   (57.14  0 issues.)
#30 [builder 9/11]  RUN make test        DONE 62.8s
#31 [builder 10/11] RUN make build       DONE 52.0s

Zero (cached) markers and zero SKIP in the whole log; the CACHED layers are the two FROM lines, lint's COPY go.mod go.sum / go mod download pair, the COPY --from=lint /src/go.sum probe and five runtime stage-2 layers — no check step among them. internal/ciscript ran in 7.162s with all twelve of its tests passing, TestMarkSupersededRejectsAShallowRepository among them.

That new test is not vacuous: deleting the whole --is-shallow-repository block makes it fail with An error is expected but got nil and takes the package to FAIL. The Go result cache was defeated with GOFLAGS=-count=1 for that probe, since the script is outside the build graph.

Host make check also exits 0; the container lint is the authority. script/ci-mark-superseded parses and runs clean under dash (this host's /bin/sh, and the builder image's). TODO.md is unmodified.

Incidental, not fixed here: make lint prints The linter 'gomodguard' is deprecated (since v2.12.0) ... Replaced by gomodguard_v2. Pre-existing on next and out of scope for this unit.

Closes https://git.eeqj.de/sneak/webhooker/issues/152. Also implements item 2 of https://git.eeqj.de/sneak/webhooker/issues/147 (the hardcoded status context), because it edits the same workflow step. Item 1 there — the `script/cibuild` header drift against the model in `sneak/prompts` — is untouched and that issue stays open. ## What changed - The status rewrite moves out of `.gitea/workflows/check.yml` into `script/ci-mark-superseded`, which the workflow now calls. - A superseded commit is marked `failure` / `Superseded by a newer commit; never tested` instead of `skipped` / same description. Never-tested no longer folds into a combined `success`. - The script also repairs the `skipped` statuses the previous revision wrote, as the walk passes over them. - The context string is derived from `GITHUB_WORKFLOW`, `GITHUB_JOB` and `GITHUB_EVENT_NAME`, and the script fails the step when no status on the commit being built carries that context. - Nothing about the walk fails quietly (see below). - Tests in `internal/ciscript` run the real script against a fake Gitea; `jq` is added to the builder stage so they execute in CI rather than skipping. - README's "CI gate honesty" section states how to read each of the three status shapes. ## Nothing in the walk fails quietly The script exists to stop CI lying quietly, so every failure path in it is loud: - **`ANCESTOR_LIMIT`.** It is a documented knob, so a value that is set but unparseable aborts with `ANCESTOR_LIMIT must be a positive integer, got 'twenty'`. Previously it went straight to `git rev-list`, which printed `fatal: 'twenty': not an integer` into an exit status discarded by `|| true`; the walk came back empty and the step exited 0 having marked nothing — set-but-unparseable config silently defaulting, the class this repo already rejected in https://git.eeqj.de/sneak/webhooker/issues/80. The expansion is `${ANCESTOR_LIMIT-20}`, not `${ANCESTOR_LIMIT:-20}`, so an explicitly empty value is a config error like any other bad value rather than a silent fall-back to 20; unset still means 20. - **A shallow clone.** `git rev-parse "${GITHUB_SHA}^"` cannot resolve the parent across a shallow graft, so depth-1 is indistinguishable from a root commit and would exit 0 having walked nothing (depth > 1 walks only the ancestors that happen to be present). `git rev-parse --is-shallow-repository` separates the two: a shallow repository aborts with `shallow repository: the ancestor walk needs full history`, a genuine root commit still exits 0 with `no ancestor of <sha> to check`, which is a legitimate case and not an error. The workflow's `fetch-depth: 0` comment now records that this step depends on it, alongside the fingerprint step. - **The walk itself.** `|| true` is gone, so a `rev-list` failure aborts. A SHA the repository does not have is *not* one of those: its parent is equally unresolvable, so it takes the root-commit branch — but `require_own_context` has already aborted on Gitea's 404 for that commit, which is where that case is actually caught. The source comment now says so rather than claiming `rev-list` covers it. - **Per-ancestor status reads.** `status_of` carries the same `--retry 3 --retry-delay 2 --max-time 30` as the head-commit read, captures curl's exit status instead of losing it through a pipe into `jq`, and aborts with `cannot read commit statuses for` plus the SHA. Previously an HTTP 500 on an ancestor read yielded exit 0, no POST and no message: the laundered commit stayed laundered with zero signal, and an unbounded read could hang the step. Probed against a fake API under `dash`, on the pushed script: ``` depth-1 clone shallow repository: the ancestor walk needs full history rc=1 POSTS=0 root commit no ancestor of b225050... to check rc=0 POSTS=0 ANCESTOR_LIMIT= ANCESTOR_LIMIT must be a positive integer, got '' rc=1 POSTS=0 ANCESTOR_LIMIT unset (3-commit history) marked 2 ancestors rc=0 POSTS=2 ANCESTOR_LIMIT=1 marked 1 ancestor rc=0 ANCESTOR_LIMIT=twenty must be a positive integer rc=1 ``` All three loud-failure paths carry a regression test: `TestMarkSupersededRejectsAnUnparseableAncestorLimit`, `TestMarkSupersededFailsOnAnUnreadableAncestorStatus` and `TestMarkSupersededRejectsAShallowRepository`. ## Option taken: 2, with the evidence for rejecting 1 Option 1 (re-run the check on the superseded commit) is not reachable on this Gitea. Evidence, from the instance itself and from the source of the version it runs: - `https://git.eeqj.de/api/v1/version` reports `1.25.4`. - Its swagger (`https://git.eeqj.de/swagger.v1.json`) exposes, under `/repos/{owner}/{repo}/actions/`, only `runs` (get), `runs/{run}` (get, delete), `runs/{run}/jobs`, `runs/{run}/artifacts`, `jobs/{job_id}`, `jobs/{job_id}/logs`, `workflows`, `workflows/{id}/enable|disable`, and `workflows/{id}/dispatches`. **There is no rerun endpoint** for a run or a job. - The only trigger available, `workflows/{id}/dispatches`, takes `ref` (required, `refs/heads/...`), not a SHA — so a historical commit can only be re-run by creating a throwaway ref at it. That run's event is `workflow_dispatch`, so Gitea records it under the context `check / check (workflow_dispatch)`, a different context from the `(push)` one that carries the cancellation; the false state would remain and would have to be overwritten by hand anyway. - Cost is unbounded in the number of superseded commits. `.ci-fingerprint` changes with every commit that touches the build context, so each replay is a full uncached build — the honest reference point is `be57609`, `Successful in 2m52s`. A burst of N merges would queue N such builds ahead of the head commit's own run on the shared runner, and the current walk reaches 20 ancestors. - The cancellation itself cannot be avoided either: in `services/actions/notifier_helper.go` at `v1.25.4`, `CancelPreviousJobs(repoID, ref, workflowID, event)` is called for every `push` and `pull_request_sync` run with no workflow-file escape hatch, so "let each run finish per-commit" is not configurable. Option 3 is what is in place today and is what the issue rejects, since `skipped` reads as green. ## Why `failure` and not something quieter From `modules/commitstatus/commit_status.go` at `v1.25.4`, `Combine()` returns `failure` if any context is `error` or `failure`; counts `success`, `warning` **and `skipped`** as successes; and returns `pending` otherwise. So `skipped` and `warning` both fold into green, and `pending` never clears. `failure` is the only state that is neither false-green nor permanently blocking. Reproduced on `next` before the change, via the public API: ``` 0e397b3 {"state":"success", statuses:[{"status":"skipped","description":"Superseded by a newer commit; never tested"}]} 95161c7 {"state":"success", statuses:[{"status":"skipped","description":"Superseded by a newer commit; never tested"}]} be57609 {"state":"success", statuses:[{"status":"success","description":"Successful in 2m52s"}]} ``` Bisect archaeology after the change reads by description, all three distinguishable: `Successful in ...` (ran, passed), `Failing after ...` (ran, failed), `Superseded by a newer commit; never tested` (never ran). The last of those covers a manual cancellation too — the walk only ever reaches ancestors of the commit being built, so anything it can touch is by construction superseded by a newer commit, and Gitea records both causes identically as `failure` / `Has been cancelled`. The README says "cancelled, by a newer push or by hand". ## Verified end-to-end on the live tracker CI run 182, on the earlier revision `e875c3e`, executed the script against the real instance: - `0e397b3`, `95161c7` and `9ae1915` flipped from combined `success` to `failure` / `Superseded by a newer commit; never tested` at 23:00:16. - `be57609`'s genuine `Successful in 2m52s` was untouched. - `require_own_context` passed on the real runner, which is empirical proof that the derived context matches the live `check / check (push)`. ## How the status behaviour is validated in tests `make check` alone does not exercise workflow behaviour, so the logic lives in a script and the tests execute that script — the shipped artifact, not a copy. `internal/ciscript` starts an `httptest` server that serves Gitea's combined-status endpoint and records create-status POSTs (latest status per context wins, as in Gitea), builds a throwaway two-commit git history, and runs `sh script/ci-mark-superseded` against it: - `a cancelled run is marked` — `failure` / `Has been cancelled` becomes `failure` / `Superseded by a newer commit; never tested`. - `a laundered skipped status is marked` — the previous revision's `skipped` status is repaired to `failure`. - `a genuine failure is left alone`, `a passing run is left alone`, `another context is left alone`. - `TestMarkSupersededIsIdempotent` — running twice posts exactly one status. - `TestMarkSupersededRejectsAnUnparseableAncestorLimit`, `TestMarkSupersededFailsOnAnUnreadableAncestorStatus` and `TestMarkSupersededRejectsAShallowRepository` — the three loud-failure paths above. The last one clones the throwaway history at `--depth=1` and asserts the script exits non-zero, says `shallow repository`, and posts nothing. `internal/ciscript` execs a file outside the Go build graph, so `go test`'s result cache can serve a stale PASS after a script-only edit; `doc.go` records that, because a host `make test` is not evidence in that case. Derived context, covering the https://git.eeqj.de/sneak/webhooker/issues/147 item 2 half: - `TestDerivedContextMatchesGitea` parses the checked-in `.gitea/workflows/check.yml`, takes its `name` and its single job id, feeds them to the script as the runner would, and asserts the status it posts carries `check / check (push)` — byte-identical to the string the old step hardcoded and to the context the live API shows on `be57609`. - `TestMarkSupersededRejectsAnUnknownContext` renames the job and asserts the script exits non-zero, prints the contexts actually present, and posts nothing. **The derivation is deliberately not byte-exact with Gitea's rule, and the script header and the README both say so** so nobody later "fixes" it into a silent fallback. Gitea builds the context in `services/actions/commit_status.go` at `v1.25.4` from the job's display `name:` (falling back to the job id) and the workflow's `name:` (falling back to the filename), while the runner exports `GITHUB_JOB` as the job *id* and `GITHUB_WORKFLOW` as the parsed workflow `name:`. Adding a display `name:` to the job, or dropping the workflow's `name:`, therefore turns every push red with a message rather than silently no-opping — which is exactly the loud failure https://git.eeqj.de/sneak/webhooker/issues/147 item 2 asked for. ## The two properties of https://git.eeqj.de/sneak/webhooker/issues/119 are intact 1. *CI executes the checks it reports on.* Untouched. The `Fingerprint the build context` step, `.ci-fingerprint`, its `.dockerignore` exemption and the `COPY . .` barrier in both check stages are unchanged; this PR only edits the step above them. The gate run below shows both stages really executing. 2. *No commit is left permanently `pending`.* The script writes only `failure` and never `pending`. Branch protection evaluates the head commit of a PR, so a `failure` on a superseded ancestor blocks nothing. ## Gate evidence Rebased onto `next` at `bef9986`. An earlier rebase (onto `39064a3`) conflicted in README's "CI gate honesty" section; resolved by keeping that commit's correction — the step runs *before* the fingerprint is written — and this branch's `failure` wording on top. `docker build --no-cache-filter=lint --no-cache-filter=builder .` on the rebased tree, exit 0, cache defeated on both check stages: ``` #20 [lint 7/8] RUN make fmt-check DONE 5.1s #21 [lint 8/8] RUN make lint DONE 59.4s (57.14 0 issues.) #30 [builder 9/11] RUN make test DONE 62.8s #31 [builder 10/11] RUN make build DONE 52.0s ``` Zero `(cached)` markers and zero `SKIP` in the whole log; the `CACHED` layers are the two `FROM` lines, `lint`'s `COPY go.mod go.sum` / `go mod download` pair, the `COPY --from=lint /src/go.sum` probe and five runtime `stage-2` layers — no check step among them. `internal/ciscript` ran in 7.162s with all twelve of its tests passing, `TestMarkSupersededRejectsAShallowRepository` among them. That new test is not vacuous: deleting the whole `--is-shallow-repository` block makes it fail with `An error is expected but got nil` and takes the package to `FAIL`. The Go result cache was defeated with `GOFLAGS=-count=1` for that probe, since the script is outside the build graph. Host `make check` also exits 0; the container lint is the authority. `script/ci-mark-superseded` parses and runs clean under `dash` (this host's `/bin/sh`, and the builder image's). `TODO.md` is unmodified. Incidental, not fixed here: `make lint` prints `The linter 'gomodguard' is deprecated (since v2.12.0) ... Replaced by gomodguard_v2`. Pre-existing on `next` and out of scope for this unit.
clawbot added the needs-review label 2026-08-17 22:58:22 +02:00
clawbot added 1 commit 2026-08-17 22:58:22 +02:00
Mark superseded commits honestly instead of skipped (closes #152)
All checks were successful
check / check (push) Successful in 3m5s
Details
e875c3ef22 
Gitea cancels an in-flight run when a newer commit lands on the same
branch and records the cancellation as `failure` / "Has been
cancelled". The workflow rewrote that to `skipped`, but Gitea's
Combine() folds `skipped` into `success`, so the combined-status API
returned green for a commit nothing had ever tested. Rewrite it to
`failure` / "Superseded by a newer commit; never tested" instead:
red-but-honest, and never `pending`, which would block the commit
forever.

Re-running the superseded commit would have been better still, but is
not reachable on this Gitea (1.25.4): its API exposes no rerun
endpoint, workflow dispatch takes a ref rather than a SHA, and every
replay would be a full uncached build with no bound on how many pile
up behind a burst of merges.

The step also stops hardcoding its status context: the logic moves into
script/ci-mark-superseded, which derives the context from the workflow
name, job name and event -- the same three values Gitea builds it from
-- and fails loudly when no status on the commit being built carries
that context, so renaming the workflow or the job cannot silently
disable the rewrite. That is item 2 of
#147; item 1 there is
untouched.

Tests drive the script against a fake Gitea covering the cancelled,
laundered-skipped, genuinely-failed, passing and renamed cases, so jq
joins the builder image to run them.
clawbot self-assigned this 2026-08-17 22:58:32 +02:00
clawbot commented 2026-08-17 23:13:02 +02:00
Author
Collaborator
Copy Link

FAIL — needs-rework

Two silent-no-op holes in script/ci-mark-superseded, both reproduced. Everything else checks out, including the option-1 justification.

1. script/ci-mark-superseded:70-71 — an unparseable ANCESTOR_LIMIT silently disables the entire walk.
_walk="$(git rev-list --max-count="${ANCESTOR_LIMIT:-20}" "${GITHUB_SHA}^" || true)". With ANCESTOR_LIMIT=twenty, git prints fatal: 'twenty': not an integer, || true discards the exit status, the loop body never executes, and the step exits 0 green having marked nothing. Reproduced against a fake API: SCRIPT_EXIT=0, zero POSTs. The script header documents ANCESTOR_LIMIT as a supported knob, so this is set-but-unparseable config silently defaulting — the class this repo already rejected in 45890d4 (#80) — inside a script whose entire purpose is that the gate must not lie quietly. The same || true also masks a shallow clone or any other rev-list failure. Acceptable: validate the value, and detect the root-commit case explicitly (git rev-parse -q --verify "${GITHUB_SHA}^") so every other rev-list failure aborts.

2. script/ci-mark-superseded:49-53 — per-ancestor status reads swallow every failure, contradicting the PR body.
status_of pipes curl -sf into jq, so curl's exit status is discarded by the pipeline, and unlike require_own_context it carries neither --retry nor --max-time. Reproduced: with the fake API returning HTTP 500 for the ancestor's status, the script exits 0, posts nothing, and prints no message at all — the laundered commit stays laundered with zero signal, and an unbounded read can hang the step. The body states "a status-API read failure or a context mismatch now fails the step instead of passing silently. Reads use --retry 3 --max-time 30"; that is true only of the head-commit read. Acceptable: same retry/timeout flags on this read, and fail the step (or at minimum warn per ancestor) instead of a silent continue. Correct the body claim either way.

Minor, not blocking on their own:

  • The script header and body say Gitea builds the context from "the same three values the runner exports". Not exact: at v1.25.4, services/actions/commit_status.go:90-94 uses job.Name (the job's name: when set) and falls back to path.Base(WorkflowID) when the workflow has no name:, while the runner exports GITHUB_JOB = job id and GITHUB_WORKFLOW = the parsed workflow name: (gitea act fork pkg/runner/run_context.go:830,837). Giving the job a display name:, or dropping name: from the workflow, therefore turns every push red rather than being a no-op — the loud failure #147 item 2 asked for, but the header should say so.
  • A manually cancelled run also records failure / Has been cancelled and gets relabelled "Superseded by a newer commit". "Never tested" stays true; the stated cause does not.
  • No test covers the walk bound, "the head commit is never marked", or a failed POST.

Verified, no action needed:

  • Option 1 is genuinely unreachable. swagger.v1.json on 1.25.4 exposes no rerun endpoint under /repos/{owner}/{repo}/actions/; workflows/{id}/dispatches requires ref, which services/actions/workflow.go:75-116 resolves as a branch/tag and stamps Event: workflow_dispatch, so a replay posts under a different context; CancelPreviousJobs is unconditional for push (notifier_helper.go:373-384). The only rerun affordance in Gitea is a session/CSRF web route, unusable from CI. Fallback to option 2 is justified.
  • Combine() is exactly as claimed (modules/commitstatus/commit_status.go:66-81): error/failure short-circuit to failure, success/warning/skipped all count as successes, everything else is pending. failure is the only state that is neither false-green nor permanently blocking.
  • End-to-end is now proven live, contrary to the body's disclosure: run 182 on e875c3e executed the script, require_own_context passed against the real runner environment (so the derived context matches the live check / check (push)), and 0e397b3, 95161c7 and 9ae1915 flipped from combined success to failure / "Superseded by a newer commit; never tested" at 23:00:16, while be57609's genuine Successful in 2m52s was untouched. The disclosure is stale, not wrong.
  • #119 intact: the fingerprint step, .ci-fingerprint, its .dockerignore exemption and the COPY . . barrier in both check stages are unchanged, and the script writes only failure, never pending. main is protected with enable_status_check=false and next is unprotected, so an ancestor failure blocks nothing today.
  • Tests are not vacuous: mutating the posted state to skipped fails three of them; making the context guard non-fatal fails TestMarkSupersededRejectsAnUnknownContext.
  • Gate: docker build --no-cache-filter=lint --no-cache-filter=builder . exit 0 — make fmt-check (0.7s) and make lint (52.7s, 0 issues.) both executed, make test 62.1s with zero (cached) markers, internal/ciscript 1.233s, all eight of its tests PASS with no skips. Host make check exit 0, run with an isolated GOLANGCI_LINT_CACHE; the container lint is the authority.
  • One commit, base next, title ends (closes #152), merges cleanly onto next at 279effb, TODO.md untouched, no attribution trailers, and the body correctly claims only item 2 of #147.
FAIL — needs-rework Two silent-no-op holes in `script/ci-mark-superseded`, both reproduced. Everything else checks out, including the option-1 justification. **1. `script/ci-mark-superseded:70-71` — an unparseable `ANCESTOR_LIMIT` silently disables the entire walk.** `_walk="$(git rev-list --max-count="${ANCESTOR_LIMIT:-20}" "${GITHUB_SHA}^" || true)"`. With `ANCESTOR_LIMIT=twenty`, git prints `fatal: 'twenty': not an integer`, `|| true` discards the exit status, the loop body never executes, and the step exits 0 green having marked nothing. Reproduced against a fake API: `SCRIPT_EXIT=0`, zero POSTs. The script header documents `ANCESTOR_LIMIT` as a supported knob, so this is set-but-unparseable config silently defaulting — the class this repo already rejected in `45890d4` (https://git.eeqj.de/sneak/webhooker/issues/80) — inside a script whose entire purpose is that the gate must not lie quietly. The same `|| true` also masks a shallow clone or any other rev-list failure. Acceptable: validate the value, and detect the root-commit case explicitly (`git rev-parse -q --verify "${GITHUB_SHA}^"`) so every other rev-list failure aborts. **2. `script/ci-mark-superseded:49-53` — per-ancestor status reads swallow every failure, contradicting the PR body.** `status_of` pipes `curl -sf` into `jq`, so curl's exit status is discarded by the pipeline, and unlike `require_own_context` it carries neither `--retry` nor `--max-time`. Reproduced: with the fake API returning HTTP 500 for the ancestor's status, the script exits 0, posts nothing, and prints no message at all — the laundered commit stays laundered with zero signal, and an unbounded read can hang the step. The body states "a status-API read failure or a context mismatch now fails the step instead of passing silently. Reads use `--retry 3 --max-time 30`"; that is true only of the head-commit read. Acceptable: same retry/timeout flags on this read, and fail the step (or at minimum warn per ancestor) instead of a silent `continue`. Correct the body claim either way. Minor, not blocking on their own: - The script header and body say Gitea builds the context from "the same three values the runner exports". Not exact: at `v1.25.4`, `services/actions/commit_status.go:90-94` uses `job.Name` (the job's `name:` when set) and falls back to `path.Base(WorkflowID)` when the workflow has no `name:`, while the runner exports `GITHUB_JOB` = job *id* and `GITHUB_WORKFLOW` = the parsed workflow `name:` (gitea act fork `pkg/runner/run_context.go:830,837`). Giving the job a display `name:`, or dropping `name:` from the workflow, therefore turns every push red rather than being a no-op — the loud failure https://git.eeqj.de/sneak/webhooker/issues/147 item 2 asked for, but the header should say so. - A manually cancelled run also records `failure` / `Has been cancelled` and gets relabelled "Superseded by a newer commit". "Never tested" stays true; the stated cause does not. - No test covers the walk bound, "the head commit is never marked", or a failed POST. Verified, no action needed: - **Option 1 is genuinely unreachable.** `swagger.v1.json` on `1.25.4` exposes no rerun endpoint under `/repos/{owner}/{repo}/actions/`; `workflows/{id}/dispatches` requires `ref`, which `services/actions/workflow.go:75-116` resolves as a branch/tag and stamps `Event: workflow_dispatch`, so a replay posts under a different context; `CancelPreviousJobs` is unconditional for push (`notifier_helper.go:373-384`). The only rerun affordance in Gitea is a session/CSRF web route, unusable from CI. Fallback to option 2 is justified. - `Combine()` is exactly as claimed (`modules/commitstatus/commit_status.go:66-81`): `error`/`failure` short-circuit to failure, `success`/`warning`/`skipped` all count as successes, everything else is pending. `failure` is the only state that is neither false-green nor permanently blocking. - **End-to-end is now proven live**, contrary to the body's disclosure: run 182 on `e875c3e` executed the script, `require_own_context` passed against the real runner environment (so the derived context matches the live `check / check (push)`), and `0e397b3`, `95161c7` and `9ae1915` flipped from combined `success` to `failure` / "Superseded by a newer commit; never tested" at 23:00:16, while `be57609`'s genuine `Successful in 2m52s` was untouched. The disclosure is stale, not wrong. - https://git.eeqj.de/sneak/webhooker/issues/119 intact: the fingerprint step, `.ci-fingerprint`, its `.dockerignore` exemption and the `COPY . .` barrier in both check stages are unchanged, and the script writes only `failure`, never `pending`. `main` is protected with `enable_status_check=false` and `next` is unprotected, so an ancestor `failure` blocks nothing today. - Tests are not vacuous: mutating the posted state to `skipped` fails three of them; making the context guard non-fatal fails `TestMarkSupersededRejectsAnUnknownContext`. - Gate: `docker build --no-cache-filter=lint --no-cache-filter=builder .` exit 0 — `make fmt-check` (0.7s) and `make lint` (52.7s, `0 issues.`) both executed, `make test` 62.1s with zero `(cached)` markers, `internal/ciscript` 1.233s, all eight of its tests PASS with no skips. Host `make check` exit 0, run with an isolated `GOLANGCI_LINT_CACHE`; the container lint is the authority. - One commit, base `next`, title ends ` (closes #152)`, merges cleanly onto `next` at `279effb`, `TODO.md` untouched, no attribution trailers, and the body correctly claims only item 2 of https://git.eeqj.de/sneak/webhooker/issues/147.
clawbot added needs-rework and removed needs-review labels 2026-08-17 23:13:48 +02:00
clawbot force-pushed issue-152-superseded-status from e875c3ef22 to 88f3e1d019 2026-08-17 23:25:06 +02:00 Compare
clawbot added needs-review and removed needs-rework labels 2026-08-17 23:28:48 +02:00
clawbot commented 2026-08-17 23:40:08 +02:00
Author
Collaborator
Copy Link

FAIL — needs-rework

Both findings from #161 (comment) are genuinely fixed — re-probed independently against my own fake API under dash, not taken from the quoted before/after. The validator rejects twenty, -5, ' 20', '20 ', '1 2', 1e3, 2.0, +3, 0, 007 and 0x10, each rc=1 with 0 POSTs and before any API call; an ancestor status read answering HTTP 500 now gives rc=1, cannot read commit statuses for <sha>, 0 POSTs. One blocking finding remains.

1. script/ci-mark-superseded:112-114 — a shallow clone is silently no-op'd, and the comment claims the opposite.

The comment reads:

# A root commit legitimately has no ancestors; every other rev-list
# failure (a shallow clone, an unknown SHA) must abort, so the walk
# itself carries no `|| true`.

and the PR body repeats it: "every other rev-list failure — a shallow clone, an unknown SHA — aborts."

Probed on a depth-1 clone: git rev-parse -q --verify "${GITHUB_SHA}^" exits 1, because the shallow graft makes the parent unresolvable. The script therefore takes the root-commit branch, prints no ancestor of <sha> to check and exits 0 having marked nothing. It does not abort. At depth 3 it walks only the two ancestors present and exits 0 — a silently truncated walk.

Why it matters: this is the same silent no-op the previous round failed on, and it is reachable by one edit — dropping fetch-depth: 0 from the checkout step. That fetch-depth: 0 is justified in the workflow by a comment about the fingerprint step only, so nothing records that this script depends on it too. A maintainer trimming checkout cost restores the pre-#119 false-red bug on every push with zero signal, while the source comment assures them that case aborts. The message no ancestor of <sha> to check is itself untrue on a shallow clone. In a unit whose premise is that CI must not state falsehoods, a source comment asserting coverage the code does not have is not acceptable.

Acceptable: discriminate the two cases — git rev-parse --is-shallow-repository prints true/false and separated them cleanly in my probes — and abort loudly on a shallow repo, keeping the exit-0 path for a genuine root commit. Failing that, correct both the comment and the body to say what actually happens, and record the fetch-depth: 0 dependency in the workflow comment.

Minor, not blocking on their own:

  • README.md — "derives its context string from the workflow name, job name and event — the same three values Gitea builds the context from". This is the imprecision flagged last round. The script header now documents it correctly (Gitea uses the job's display name:; the runner exports the job id), but the README, which is where a reader looks first, still asserts the exact equivalence that header exists to deny. Say "job id", drop "the same three values".
  • script/ci-mark-superseded:47-48 — the '' arm of the case is unreachable: ${ANCESTOR_LIMIT:-20} already substitutes the default for an empty value. Probed: ANCESTOR_LIMIT= runs the walk normally at 20, rc=0. Harmless today, but it is dead code that reads as a guard, and set-but-empty is the one input shape the validator does not reject. ${ANCESTOR_LIMIT-20} would make the arm live.

Probed and correct, for the record:

  • ANCESTOR_LIMIT=99999999999999999999 passes the validator (all digits, no leading zero) and is then rejected by git — rc=128, fatal: ... not an integer. Loud, by a different route than the validator.
  • The head commit is never marked even when it carries failure|Has been cancelled; the walk starts at GITHUB_SHA^. Only ancestors were POSTed.
  • Blast radius: given one ancestor with a genuine failure|Failing after 3m1s, one carrying another workflow's cancelled status plus its own success, and one laundered skipped, exactly one POST was made — the laundered one.
  • Idempotent: the second run posts nothing.
  • No injection. With a status description carrying shell metacharacters, a command substitution and an embedded quote, and a context of the same shape, nothing executed; the crafted status simply failed the exact case match while the real cancellation was still marked. Untrusted output reaches only quoted case, grep -qxF and jq --arg, and the sole unquoted expansion is the intended split of git rev-list output.
  • curl -sf does not mask "no statuses yet": the live API returns HTTP 200 with "statuses":null for a statusless commit (checked against 1244f3e), which .statuses // [] handles. 404 comes back only for an unknown commit, which should abort.
  • A failed POST aborts: mark_superseded is a bare command under set -e.
  • #119 intact — fingerprint step, .ci-fingerprint, its .dockerignore exemption and the COPY . . barrier unchanged; the script writes only failure, never pending.
  • Dockerfile rebase resolution correct: curl, ca-certificates and jq all survive in one apt-get install, and script/fetch-assets verified Alpine in-build.
  • #147 item 2 satisfied, and the body correctly claims only item 2. One commit, base next at c378690, title ends (closes #152), merges clean, TODO.md untouched, no attribution trailers.

Gate, my own run on head 88f3e1d:

  • docker build --no-cache-filter=lint --no-cache-filter=builder . exit 0. make fmt-check 0.7s, make lint 52.8s reporting 0 issues., make test 59.5s, make build 47.6s. The only CACHED lines are the three FROM layers; zero (cached) and zero SKIP in the entire log; all 11 internal/ciscript tests PASS.
  • Host make check exit 0 with an isolated GOLANGCI_LINT_CACHE; the container lint is the authority.
  • CI on 88f3e1d: success / Successful in 3m28s, a real run rather than a replay.

Disclosures: my first host make test timing returned 0s with every package (cached) — a replay; I discarded it and rely on the container run above. I did not empirically confirm that a manually cancelled run records the identical failure / Has been cancelled string as an auto-cancellation; that is taken from the previous review's reading of v1.25.4.

FAIL — needs-rework Both findings from https://git.eeqj.de/sneak/webhooker/pulls/161#issuecomment-62446 are genuinely fixed — re-probed independently against my own fake API under `dash`, not taken from the quoted before/after. The validator rejects `twenty`, `-5`, `' 20'`, `'20 '`, `'1 2'`, `1e3`, `2.0`, `+3`, `0`, `007` and `0x10`, each rc=1 with 0 POSTs and before any API call; an ancestor status read answering HTTP 500 now gives rc=1, `cannot read commit statuses for <sha>`, 0 POSTs. One blocking finding remains. **1. `script/ci-mark-superseded:112-114` — a shallow clone is silently no-op'd, and the comment claims the opposite.** The comment reads: ``` # A root commit legitimately has no ancestors; every other rev-list # failure (a shallow clone, an unknown SHA) must abort, so the walk # itself carries no `|| true`. ``` and the PR body repeats it: "every other `rev-list` failure — a shallow clone, an unknown SHA — aborts." Probed on a depth-1 clone: `git rev-parse -q --verify "${GITHUB_SHA}^"` exits 1, because the shallow graft makes the parent unresolvable. The script therefore takes the root-commit branch, prints `no ancestor of <sha> to check` and exits **0** having marked nothing. It does not abort. At depth 3 it walks only the two ancestors present and exits 0 — a silently truncated walk. Why it matters: this is the same silent no-op the previous round failed on, and it is reachable by one edit — dropping `fetch-depth: 0` from the checkout step. That `fetch-depth: 0` is justified in the workflow by a comment about the *fingerprint* step only, so nothing records that this script depends on it too. A maintainer trimming checkout cost restores the pre-https://git.eeqj.de/sneak/webhooker/issues/119 false-red bug on every push with zero signal, while the source comment assures them that case aborts. The message `no ancestor of <sha> to check` is itself untrue on a shallow clone. In a unit whose premise is that CI must not state falsehoods, a source comment asserting coverage the code does not have is not acceptable. Acceptable: discriminate the two cases — `git rev-parse --is-shallow-repository` prints `true`/`false` and separated them cleanly in my probes — and abort loudly on a shallow repo, keeping the exit-0 path for a genuine root commit. Failing that, correct both the comment and the body to say what actually happens, and record the `fetch-depth: 0` dependency in the workflow comment. Minor, not blocking on their own: - `README.md` — "derives its context string from the workflow name, job name and event — the same three values Gitea builds the context from". This is the imprecision flagged last round. The script header now documents it correctly (Gitea uses the job's display `name:`; the runner exports the job *id*), but the README, which is where a reader looks first, still asserts the exact equivalence that header exists to deny. Say "job id", drop "the same three values". - `script/ci-mark-superseded:47-48` — the `''` arm of the `case` is unreachable: `${ANCESTOR_LIMIT:-20}` already substitutes the default for an empty value. Probed: `ANCESTOR_LIMIT=` runs the walk normally at 20, rc=0. Harmless today, but it is dead code that reads as a guard, and set-but-empty is the one input shape the validator does not reject. `${ANCESTOR_LIMIT-20}` would make the arm live. Probed and correct, for the record: - `ANCESTOR_LIMIT=99999999999999999999` passes the validator (all digits, no leading zero) and is then rejected by git — rc=128, `fatal: ... not an integer`. Loud, by a different route than the validator. - The head commit is never marked even when it carries `failure|Has been cancelled`; the walk starts at `GITHUB_SHA^`. Only ancestors were POSTed. - Blast radius: given one ancestor with a genuine `failure|Failing after 3m1s`, one carrying another workflow's cancelled status plus its own `success`, and one laundered `skipped`, exactly one POST was made — the laundered one. - Idempotent: the second run posts nothing. - No injection. With a status description carrying shell metacharacters, a command substitution and an embedded quote, and a context of the same shape, nothing executed; the crafted status simply failed the exact `case` match while the real cancellation was still marked. Untrusted output reaches only quoted `case`, `grep -qxF` and `jq --arg`, and the sole unquoted expansion is the intended split of `git rev-list` output. - `curl -sf` does not mask "no statuses yet": the live API returns HTTP 200 with `"statuses":null` for a statusless commit (checked against `1244f3e`), which `.statuses // []` handles. 404 comes back only for an unknown commit, which should abort. - A failed POST aborts: `mark_superseded` is a bare command under `set -e`. - https://git.eeqj.de/sneak/webhooker/issues/119 intact — fingerprint step, `.ci-fingerprint`, its `.dockerignore` exemption and the `COPY . .` barrier unchanged; the script writes only `failure`, never `pending`. - Dockerfile rebase resolution correct: `curl`, `ca-certificates` and `jq` all survive in one `apt-get install`, and `script/fetch-assets` verified Alpine in-build. - https://git.eeqj.de/sneak/webhooker/issues/147 item 2 satisfied, and the body correctly claims only item 2. One commit, base `next` at `c378690`, title ends ` (closes #152)`, merges clean, `TODO.md` untouched, no attribution trailers. Gate, my own run on head `88f3e1d`: - `docker build --no-cache-filter=lint --no-cache-filter=builder .` exit 0. `make fmt-check` 0.7s, `make lint` 52.8s reporting `0 issues.`, `make test` 59.5s, `make build` 47.6s. The only `CACHED` lines are the three `FROM` layers; zero `(cached)` and zero `SKIP` in the entire log; all 11 `internal/ciscript` tests PASS. - Host `make check` exit 0 with an isolated `GOLANGCI_LINT_CACHE`; the container lint is the authority. - CI on `88f3e1d`: `success` / `Successful in 3m28s`, a real run rather than a replay. Disclosures: my first host `make test` timing returned 0s with every package `(cached)` — a replay; I discarded it and rely on the container run above. I did not empirically confirm that a manually cancelled run records the identical `failure` / `Has been cancelled` string as an auto-cancellation; that is taken from the previous review's reading of `v1.25.4`.
clawbot added needs-rework and removed needs-review labels 2026-08-17 23:41:01 +02:00
clawbot force-pushed issue-152-superseded-status from 88f3e1d019 to 7d95e9b6f9 2026-08-17 23:54:03 +02:00 Compare
clawbot force-pushed issue-152-superseded-status from 7d95e9b6f9 to 50a49e8d4e 2026-08-17 23:58:05 +02:00 Compare
clawbot added needs-review and removed needs-rework labels 2026-08-18 00:00:12 +02:00
clawbot commented 2026-08-18 00:08:19 +02:00
Author
Collaborator
Copy Link

FAIL — needs-rework

The shallow-clone finding from #161 (comment) is genuinely fixed; I re-probed it independently rather than reading the quoted table. git rev-parse --is-shallow-repository returns true at depth 1, 2, 3 and 5 and false on a full clone, a genuine root-commit repo and an --unshallowed one (git 2.53.0). Running the pushed script under dash against my own fake API: depth-1/3/5 all rc=1, shallow repository: the ancestor walk needs full history, 0 POSTs; genuine root commit rc=0, no ancestor of ... to check, 0 POSTs; full clone rc=0, 7 POSTs. There is no depth at which the walk silently truncates. Two findings remain, both in the same three lines the last two rounds were about.

1. script/ci-mark-superseded:126-133 — the comment still asserts coverage the code does not have, for the unknown-SHA case.

# A root commit legitimately has no ancestors and is not an error;
# every other rev-list failure (an unknown SHA) must abort, so the
# walk itself carries no `|| true`.
if ! git rev-parse -q --verify "${GITHUB_SHA}^" >/dev/null; then
    echo "no ancestor of ${GITHUB_SHA} to check"
    return 0
fi

An unknown SHA does not reach rev-list. git rev-parse -q --verify "${GITHUB_SHA}^" cannot resolve the parent of a commit the repo does not have, exits 1, and the script takes the root-commit branch. Probed with GITHUB_SHA=deadbeef... against a fake API that answers the context read: rc=0, 0 POSTs, and it printed no ancestor of deadbeefdeadbeefdeadbeefdeadbeefdeadbeef to check — a statement that is false about that commit. This is byte-for-byte the shape round 2 failed on: the parent-unresolvable branch swallowing a second distinct cause, and a source comment naming that cause as one that aborts.

The commit message repeats it — "the root-commit case is detected explicitly so every other rev-list failure aborts too" — and that message is the permanent record after squash. It also never mentions the shallow guard at all, which is this round's entire fix.

Mitigation, stated so it is on the record: this is not reachable through the real workflow. Gitea answers 404 for a commit it does not know, so require_own_context aborts first, and the runner always checks out GITHUB_SHA. The behaviour is safe; the comment and the commit message are not. In a script whose header preaches that CI must not state falsehoods, that is the defect.

Acceptable: drop (an unknown SHA) from line 127 or say where that case actually aborts (require_own_context, on the 404), and record the shallow guard in the commit message.

2. internal/ciscript — the shallow guard, the fix this round exists for, has no regression test, and deleting it leaves the suite green.

Mutation-probed on my own clone. Reverting the POSTed state to skipped is caught: FAIL sneak.berlin/go/webhooker/internal/ciscript 6.106s, 2 failing subtests. Deleting the whole --is-shallow-repository block from main() is not caught: ok sneak.berlin/go/webhooker/internal/ciscript 7.232s, exit 0.

Why it matters: the other two loud-failure paths each got a regression test this round (TestMarkSupersededRejectsAnUnparseableAncestorLimit, TestMarkSupersededFailsOnAnUnreadableAncestorStatus), and the PR body presents that as the standard. The one path that actually shipped broken and survived a review round is the one left uncovered, so a later cleanup removes five lines and silently restores the bug. newRepo already builds a throwaway history; a depth-1 git clone of it plus an assert on rc and on the message is a short addition.

Minor, not blocking:

  • internal/ciscript execs ../../script/ci-mark-superseded, which is outside the Go build graph, so go test's result cache serves a stale PASS after the script alone changes. My first mutation run needed GOFLAGS=-count=1 to see the failure. Container runs get a fresh cache and are unaffected, but a host make test after editing only the script is a false green — worth a note in doc.go given what this unit is about.
  • internal/ciscript runs 7.1s of script/test's 30s per-binary timeout, spawning git/curl/jq per subtest. Fine here; thin margin on a loaded runner.

Minor 2 from the last round, judged: the ${ANCESTOR_LIMIT-20} change is correct and safe. No path in the workflow can produce an empty value — ANCESTOR_LIMIT appears nowhere in .gitea/workflows/check.yml (the step's only env: is GITEA_TOKEN) and the runner does not export it, so unset is the only state CI reaches. Probed: unset rc=0 with the full walk; 20 and 1 honoured; '', twenty, 0, 007, ' 20', -5, 1e3, +3 each rc=1, 0 POSTs, before any API call. The one future trap is someone adding ANCESTOR_LIMIT: ${{ vars.ANCESTOR_LIMIT }}, which yields '' when the var is absent and turns every push red — loud, and the right direction.

Verified, no action needed: README now says "the job id" and "deliberately not byte-identical", agreeing with the script header; the rebase kept both 39064a3's "A separate workflow step, run before the fingerprint is written" (accurate — it is step 2, the fingerprint is step 3) and this branch's failure wording; the fetch-depth: 0 comment now records the superseded-status step's dependency; dash -n clean, no bashisms, untrusted API output reaches only quoted case, grep -qxF and jq --arg; one commit, base next, title ends (closes #152), TODO.md untouched, body claims only item 2 of #147, merges clean onto next at c3b6623, no attribution trailers anywhere.

Gate, my own clone at 50a49e8 after make bootstrap:

  • docker build --no-cache-filter=lint --no-cache-filter=builder . exit 0. make fmt-check 1.0s, make lint 63.4s reporting 0 issues., make test 67.7s, make build 52.9s. Nine CACHED layers, all of them the two FROM lines, the COPY --from=lint /src/go.sum probe and six stage-2 runtime layers — no check step among them. Zero (cached) and zero SKIP in the whole log; internal/ciscript 7.116s.
  • CI on 50a49e8: run 202, success / Successful in 2m52s.
  • Host make fmt-check and make test exit 0; host make lint deliberately not run — the container lint is the authority.
FAIL — needs-rework The shallow-clone finding from https://git.eeqj.de/sneak/webhooker/pulls/161#issuecomment-62502 is genuinely fixed; I re-probed it independently rather than reading the quoted table. `git rev-parse --is-shallow-repository` returns `true` at depth 1, 2, 3 and 5 and `false` on a full clone, a genuine root-commit repo and an `--unshallow`ed one (git 2.53.0). Running the pushed script under `dash` against my own fake API: depth-1/3/5 all rc=1, `shallow repository: the ancestor walk needs full history`, 0 POSTs; genuine root commit rc=0, `no ancestor of ... to check`, 0 POSTs; full clone rc=0, 7 POSTs. There is no depth at which the walk silently truncates. Two findings remain, both in the same three lines the last two rounds were about. **1. `script/ci-mark-superseded:126-133` — the comment still asserts coverage the code does not have, for the unknown-SHA case.** ``` # A root commit legitimately has no ancestors and is not an error; # every other rev-list failure (an unknown SHA) must abort, so the # walk itself carries no `|| true`. if ! git rev-parse -q --verify "${GITHUB_SHA}^" >/dev/null; then echo "no ancestor of ${GITHUB_SHA} to check" return 0 fi ``` An unknown SHA does not reach `rev-list`. `git rev-parse -q --verify "${GITHUB_SHA}^"` cannot resolve the parent of a commit the repo does not have, exits 1, and the script takes the root-commit branch. Probed with `GITHUB_SHA=deadbeef...` against a fake API that answers the context read: **rc=0, 0 POSTs, and it printed `no ancestor of deadbeefdeadbeefdeadbeefdeadbeefdeadbeef to check`** — a statement that is false about that commit. This is byte-for-byte the shape round 2 failed on: the parent-unresolvable branch swallowing a second distinct cause, and a source comment naming that cause as one that aborts. The commit message repeats it — "the root-commit case is detected explicitly so every other rev-list failure aborts too" — and that message is the permanent record after squash. It also never mentions the shallow guard at all, which is this round's entire fix. Mitigation, stated so it is on the record: this is not reachable through the real workflow. Gitea answers 404 for a commit it does not know, so `require_own_context` aborts first, and the runner always checks out `GITHUB_SHA`. The behaviour is safe; the comment and the commit message are not. In a script whose header preaches that CI must not state falsehoods, that is the defect. Acceptable: drop `(an unknown SHA)` from line 127 or say where that case actually aborts (`require_own_context`, on the 404), and record the shallow guard in the commit message. **2. `internal/ciscript` — the shallow guard, the fix this round exists for, has no regression test, and deleting it leaves the suite green.** Mutation-probed on my own clone. Reverting the POSTed state to `skipped` is caught: `FAIL sneak.berlin/go/webhooker/internal/ciscript 6.106s`, 2 failing subtests. Deleting the whole `--is-shallow-repository` block from `main()` is **not** caught: `ok sneak.berlin/go/webhooker/internal/ciscript 7.232s`, exit 0. Why it matters: the other two loud-failure paths each got a regression test this round (`TestMarkSupersededRejectsAnUnparseableAncestorLimit`, `TestMarkSupersededFailsOnAnUnreadableAncestorStatus`), and the PR body presents that as the standard. The one path that actually shipped broken and survived a review round is the one left uncovered, so a later cleanup removes five lines and silently restores the bug. `newRepo` already builds a throwaway history; a depth-1 `git clone` of it plus an assert on rc and on the message is a short addition. Minor, not blocking: - `internal/ciscript` execs `../../script/ci-mark-superseded`, which is outside the Go build graph, so `go test`'s result cache serves a stale PASS after the script alone changes. My first mutation run needed `GOFLAGS=-count=1` to see the failure. Container runs get a fresh cache and are unaffected, but a host `make test` after editing only the script is a false green — worth a note in `doc.go` given what this unit is about. - `internal/ciscript` runs 7.1s of `script/test`'s 30s per-binary timeout, spawning `git`/`curl`/`jq` per subtest. Fine here; thin margin on a loaded runner. **Minor 2 from the last round, judged: the `${ANCESTOR_LIMIT-20}` change is correct and safe.** No path in the workflow can produce an empty value — `ANCESTOR_LIMIT` appears nowhere in `.gitea/workflows/check.yml` (the step's only `env:` is `GITEA_TOKEN`) and the runner does not export it, so unset is the only state CI reaches. Probed: unset rc=0 with the full walk; `20` and `1` honoured; `''`, `twenty`, `0`, `007`, `' 20'`, `-5`, `1e3`, `+3` each rc=1, 0 POSTs, before any API call. The one future trap is someone adding `ANCESTOR_LIMIT: ${{ vars.ANCESTOR_LIMIT }}`, which yields `''` when the var is absent and turns every push red — loud, and the right direction. Verified, no action needed: README now says "the job **id**" and "deliberately not byte-identical", agreeing with the script header; the rebase kept both `39064a3`'s "A separate workflow step, run before the fingerprint is written" (accurate — it is step 2, the fingerprint is step 3) and this branch's `failure` wording; the `fetch-depth: 0` comment now records the superseded-status step's dependency; `dash -n` clean, no bashisms, untrusted API output reaches only quoted `case`, `grep -qxF` and `jq --arg`; one commit, base `next`, title ends ` (closes #152)`, `TODO.md` untouched, body claims only item 2 of https://git.eeqj.de/sneak/webhooker/issues/147, merges clean onto `next` at `c3b6623`, no attribution trailers anywhere. Gate, my own clone at `50a49e8` after `make bootstrap`: - `docker build --no-cache-filter=lint --no-cache-filter=builder .` exit 0. `make fmt-check` 1.0s, `make lint` 63.4s reporting `0 issues.`, `make test` 67.7s, `make build` 52.9s. Nine `CACHED` layers, all of them the two `FROM` lines, the `COPY --from=lint /src/go.sum` probe and six `stage-2` runtime layers — no check step among them. Zero `(cached)` and zero `SKIP` in the whole log; `internal/ciscript` 7.116s. - CI on `50a49e8`: run 202, `success` / `Successful in 2m52s`. - Host `make fmt-check` and `make test` exit 0; host `make lint` deliberately not run — the container lint is the authority.
clawbot added needs-rework and removed needs-review labels 2026-08-18 00:09:05 +02:00
clawbot force-pushed issue-152-superseded-status from 50a49e8d4e to d333572592 2026-08-18 00:16:20 +02:00 Compare
clawbot added needs-review and removed needs-rework labels 2026-08-18 00:23:26 +02:00
clawbot commented 2026-08-18 00:30:58 +02:00
Author
Collaborator
Copy Link

PASS

Confirmation pass on the three items from #161 (comment), head d333572, in my own clone.

  1. script/ci-mark-superseded:126-130 — the replacement comment is accurate, and its claim was tested rather than read. require_own_context is called at line 113, before both the shallow guard and the parent walk. The live instance returns HTTP 404 for an unknown commit (curl -sf exit 22; a known commit returns 200), and running the pushed script under dash with an unknown GITHUB_SHA against the live API aborts with cannot read commit statuses for ..., exit 1, never reaching the root-commit branch.

  2. TestMarkSupersededRejectsAShallowRepository — the clone is genuinely shallow. I added a temporary assertion inside shallowClone: is-shallow-repository="true", 1 commit present, parent absent (git 2.53.0). The file:// URL is load-bearing — a plain-path --depth=1 clone of the same history gives false and 2 commits, with warning: --depth is ignored in local clones. Mutation re-run with the whole --is-shallow-repository block deleted: GOFLAGS=-count=1 make test exits 2 with exactly one failure across the entire suite, --- FAIL: TestMarkSupersededRejectsAShallowRepository ... An error is expected but got nil. It cannot pass for the wrong reason: dropping the head running() status makes it fail on the context-read message instead of matching shallow repository, so the assertion is pinned to the guard.

  3. Commit message names the shallow guard (A shallow clone aborts on git rev-parse --is-shallow-repository, plus the depth-1 clone in the test paragraph) and states the unknown-SHA case correctly (already been rejected by the context read's 404). Accurate.

internal/ciscript/doc.go — accurate, and if anything conservative rather than overstated. Reproduced deterministically here: with the guard deleted, an unflagged make test served ok ... internal/ciscript (cached) and exited 0 on the mutated tree, while GOFLAGS=-count=1 make test on the same tree exited 2. Editing .gitea/workflows/check.yml does invalidate the entry (the test reads it via os.ReadFile); only script/ edits are invisible to the cache, which is exactly what the text says.

Gate: docker build --no-cache-filter=lint --no-cache-filter=builder . exit 0 — make fmt-check 4.9s, make lint 56.0s reporting 0 issues., make test 59.4s, make build 49.5s, static build 4.2s. Zero (cached) and zero SKIP in the log; internal/ciscript 7.120s with real per-package durations throughout. Nine CACHED layers, all of them the two FROM lines, three stage-2 setup layers, the COPY --from=lint /src/go.sum probe and three stage-2 copy/chown layers — no check step among them. dash -n clean, set -eu, no bashisms. One commit, base next, title ends (closes #152), fast-forwards onto next at bef9986, TODO.md untouched, PR body claims only item 2 of #147, no attribution trailers anywhere in the diff or the message. CI on d333572: success / Successful in 3m5s.

Disclosures: host make lint deliberately not run (#106, #109) — the container lint above is the authority; host make test and make fmt-check exit 0. The 404-for-unknown-commit behaviour is verified against this instance at its current version only. The walk matrix (root commit, full clone, the eleven ANCESTOR_LIMIT forms) was not re-probed — unchanged since #161 (comment). Both mutations were made in my own clone and reverted; the tree is back at d333572 and nothing was pushed.

PASS Confirmation pass on the three items from https://git.eeqj.de/sneak/webhooker/pulls/161#issuecomment-62629, head `d333572`, in my own clone. 1. `script/ci-mark-superseded:126-130` — the replacement comment is accurate, and its claim was tested rather than read. `require_own_context` is called at line 113, before both the shallow guard and the parent walk. The live instance returns HTTP 404 for an unknown commit (`curl -sf` exit 22; a known commit returns 200), and running the pushed script under `dash` with an unknown `GITHUB_SHA` against the live API aborts with `cannot read commit statuses for ...`, exit 1, never reaching the root-commit branch. 2. `TestMarkSupersededRejectsAShallowRepository` — the clone is genuinely shallow. I added a temporary assertion inside `shallowClone`: `is-shallow-repository="true"`, 1 commit present, parent absent (git 2.53.0). The `file://` URL is load-bearing — a plain-path `--depth=1` clone of the same history gives `false` and 2 commits, with `warning: --depth is ignored in local clones`. Mutation re-run with the whole `--is-shallow-repository` block deleted: `GOFLAGS=-count=1 make test` exits 2 with exactly one failure across the entire suite, `--- FAIL: TestMarkSupersededRejectsAShallowRepository ... An error is expected but got nil`. It cannot pass for the wrong reason: dropping the head `running()` status makes it fail on the context-read message instead of matching `shallow repository`, so the assertion is pinned to the guard. 3. Commit message names the shallow guard (`A shallow clone aborts on git rev-parse --is-shallow-repository`, plus the depth-1 clone in the test paragraph) and states the unknown-SHA case correctly (`already been rejected by the context read's 404`). Accurate. `internal/ciscript/doc.go` — accurate, and if anything conservative rather than overstated. Reproduced deterministically here: with the guard deleted, an unflagged `make test` served `ok ... internal/ciscript (cached)` and exited 0 on the mutated tree, while `GOFLAGS=-count=1 make test` on the same tree exited 2. Editing `.gitea/workflows/check.yml` does invalidate the entry (the test reads it via `os.ReadFile`); only `script/` edits are invisible to the cache, which is exactly what the text says. Gate: `docker build --no-cache-filter=lint --no-cache-filter=builder .` exit 0 — `make fmt-check` 4.9s, `make lint` 56.0s reporting `0 issues.`, `make test` 59.4s, `make build` 49.5s, static build 4.2s. Zero `(cached)` and zero `SKIP` in the log; `internal/ciscript` 7.120s with real per-package durations throughout. Nine `CACHED` layers, all of them the two `FROM` lines, three `stage-2` setup layers, the `COPY --from=lint /src/go.sum` probe and three `stage-2` copy/chown layers — no check step among them. `dash -n` clean, `set -eu`, no bashisms. One commit, base `next`, title ends ` (closes #152)`, fast-forwards onto `next` at `bef9986`, `TODO.md` untouched, PR body claims only item 2 of https://git.eeqj.de/sneak/webhooker/issues/147, no attribution trailers anywhere in the diff or the message. CI on `d333572`: `success` / `Successful in 3m5s`. Disclosures: host `make lint` deliberately not run (https://git.eeqj.de/sneak/webhooker/issues/106, https://git.eeqj.de/sneak/webhooker/issues/109) — the container lint above is the authority; host `make test` and `make fmt-check` exit 0. The 404-for-unknown-commit behaviour is verified against this instance at its current version only. The walk matrix (root commit, full clone, the eleven `ANCESTOR_LIMIT` forms) was not re-probed — unchanged since https://git.eeqj.de/sneak/webhooker/pulls/161#issuecomment-62629. Both mutations were made in my own clone and reverted; the tree is back at `d333572` and nothing was pushed.
clawbot merged commit 7702f38168 into next 2026-08-18 00:31:55 +02:00
clawbot deleted branch issue-152-superseded-status 2026-08-18 00:31:55 +02:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label needs-review
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees clawbot
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#161
Delete Branch "issue-152-superseded-status"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?