Skip to content

test(qa): keep the failed image build step's output as qa evidence - #2942

Merged
freeqaz merged 2 commits into
mainfrom
test/qa-image-build-failure-output
Oct 19, 2026
Merged

freeqaz merged 2 commits into
mainfrom
test/qa-image-build-failure-output

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Finding 2020: a kubernetes QA cell on main 097f8d0 (#2919, run 39229991331) failed in dev-up's runtime image build. It failed at deploy/runtime/Dockerfile:181, the step that runs the upstream server.required-placement-scopes.test.ts. The QA helper kept only the last eight stderr lines (BuildKit's summary). The full output stayed in the private state directory, which the cell does not upload, so the vitest failure itself was lost. The compose and OpenShell cells built the same commit fine.

Change

Test-only. When a QA command's stderr shows a failed image build (ERROR: failed to solve: or podman's Error: building at STEP), two things happen:

  • imageBuildFailureOutput takes the failed BuildKit step's own lines, at most 200:
    • It reads only the last build in the output, since one dev-up can run several builds whose step numbers repeat.
    • Steps interleave, so it uses the #<step> prefix of the last #<step> ERROR: line.
    • For podman, it takes the last lines.
    • It accepts both ERROR: failed to solve: and newer buildx's ERROR: failed to build: failed to solve:.
  • recordImageBuildFailure redacts them like other QA command output (redactedQaTail) and records them as <control-plane>-image-build-failure-<ms>.json evidence, which the cell uploads (*.json). The command's error names that file.

The Dockerfile and the in-build test are unchanged.

Verification

  • New test in tests/integration/qa-fixture-inputs.test.mjs, file 33/33.
    • Interleaved BuildKit output after an earlier successful build that reused the step number: keeps only the failed step's five lines, including the vitest failure. The newer buildx wording gives the same lines.
    • Redaction: hides a registered secret.
    • Podman: keeps the last 200 lines.
    • Other command failures: record nothing.
  • eslint and prettier are clean. Internal review: approve with two nits (newer buildx wording, repeated step numbers), both applied.

Risks

Low. Test-only, and runs only on failed image builds.

For the owner: the upstream regression test runs inside the image build with one vitest worker. On a contended runner it can fail the whole cell before any OCE check runs. Options:

  • keep it (it guards the patched bridge on the exact build);
  • move it to a CI test lane;
  • retry it once.

This PR only keeps the evidence.

Finding 2020: a kubernetes QA cell's dev-up failed in the runtime image
build at the Dockerfile step that runs the upstream
server.required-placement-scopes test. The failure kept only the last
eight stderr lines (the BuildKit summary), and the full output stayed in
the private state directory, which the cell does not upload, so the
vitest failure itself was lost.

When a QA command's stderr shows a failed image build, the failed
BuildKit step's lines (or, for podman, the last lines) are redacted like
other QA command output and recorded as an *.json evidence file, which
the cell uploads; the error names that file. The Dockerfile and the
in-build test are unchanged.
@freeqaz
freeqaz requested a review from a team as a code owner October 19, 2026 13:30
@clawsweeper

clawsweeper Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

Review fixes for finding 2020: one dev-up can run several builds whose
step numbers repeat, so the failed step is looked up in the last build
only; newer buildx prints 'ERROR: failed to build: failed to solve:'.
@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. labels Oct 19, 2026
@clawsweeper

clawsweeper Bot commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge.

What this changes

This PR saves bounded, redacted output from failed image-build steps into the existing uploaded QA evidence.

Example: BuildKit step #41 fails in server.required-placement-scopes.test.ts.

  • Before: The published command error retains eight summary lines while the test assertion remains in an unuploaded private log.
  • After: The failed step's output is saved in a kubernetes-image-build-failure-<timestamp>.json artifact, and the command error names that file.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) A focused, useful diagnostic repair with credible unit coverage and successful exact-head CI, but limited direct behavioral evidence.
Proof confidence 🦐 gold shrimp (3/6) This collaborator-authored QA-tooling PR is outside the ordinary contributor proof gate; supplied verification covers synthetic parser and redaction inputs rather than a recorded failed-build artifact journey.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: Maintainers investigating a failed QA image build cannot retrieve the underlying test failure from the uploaded evidence.
Reason: The collaborator-authored direction addresses an observed diagnostic loss with a narrow tooling change; the underlying upstream test failure remains a separate investigation.

Merge readiness

✅ Ready for maintainer review

Keep this PR open for maintainer triage: it addresses a concrete QA diagnostic gap still present on current main, with no actionable correctness findings.

Priority: P3
Reviewed head: 910c1a8eb5b8f8f09a22d52371d0e37614bc5fb6

Before merge

None.

Findings

None.

Tests

  • Missing end-to-end proof: The supplied verification does not show a real failed dev-up build producing an uploaded JSON artifact or a base-fails/head-passes comparison.
Agent review details

How this fits together

The installation QA helper launches dev-up, receives subprocess failures, and sends diagnostic JSON through the existing evidence recorder to GitHub Actions artifacts.

flowchart TB
  A[QA installation] --> B[dev-up image build]
  B --> C[Failed command stderr]
  C --> D[Failed step selection]
  D --> E[Existing QA redactor]
  E --> F[JSON evidence recorder]
  F --> G[Workflow artifact upload]
Loading

Technical review

Best possible solution:

Retain bounded, redacted failed-step diagnostics through the existing QA artifact path while preserving the original build failure.

Do we have a high-confidence way to reproduce the issue?

Current main demonstrably stores complete command output privately while exposing only an eight-line stderr tail; a build assertion above that tail is omitted from uploaded QA diagnostics.

Is this the best way to solve the issue?

The change addresses the diagnostic owner directly, reusing its recorder and redactor without introducing retries, changing build outcomes, or duplicating the separate provider-fixture repair.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 7afa66cb5532.

Provenance checked

  • Bounded, redacted command-failure diagnostics keeps the original intent (b4a8a44: Retain useful QA failure details without publishing command arguments, stdout, or credentials.)
  • Protected subprocess output keeps the original intent (b749f9e: Keep isolated observer credentials out of arguments, artifacts, and failure logs.)

Testing

Proof path: unit tests only.

Security

None.

Evidence

What I checked:

Review metrics

Metric Value Why it matters
Diff allocation Product code: 0; QA helper: +47/-1; regression coverage: +82 Growth is confined to failure diagnostics and focused coverage of supported build-output formats.

Labels

Label changes:

  • add P3: This improves maintainer diagnostics for failed QA builds without changing product runtime behavior.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Label justifications:

  • P3: This improves maintainer diagnostics for failed QA builds without changing product runtime behavior.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Capture a failed dev-up build showing the named JSON artifact contains the failed step's redacted assertion output.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

Reviewed October 19, 2026, 9:56 AM ET / 13:56 UTC.

@freeqaz
freeqaz merged commit 8fe0716 into main Oct 19, 2026
80 checks passed
@freeqaz
freeqaz deleted the test/qa-image-build-failure-output branch October 19, 2026 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant