fix(test): harden the new failure diagnostics by freeqaz · Pull Request #2946 · openclaw/openclaw-enterprise

freeqaz · 2026-10-19T15:05:04Z

Problem

bughunt-129 audited the new failure diagnostics from #2940, #2941 and #2942:

2025 (medium, test(qa): keep the failed image build step's output as qa evidence #2942): no cluster-free test ran the code that turns a failed image build step into uploaded evidence (createQaInstallation's f.run calling recordImageBuildFailure). Mutants that wrote raw, unredacted build output into image-build-failure-*.json, recorded nothing, or dropped the privateFailureOutput gate passed all 33 tests.
2026 (test(qa): keep the failed image build step's output as qa evidence #2942): the build output was cut to the failed step's last 200 lines before private keys were masked. A key whose BEGIN line fell outside the window and that had no END line kept 199 of 200 lines.
2027 (test(openshell): explain failed outage redeploys and stuck namespace deletes #2940): the 40 s bound on listing what remains of a stuck Namespace, the 40-object cap, the 300-character / first-line cut on kubectl stderr, and test(qa): keep the failed image build step's output as qa evidence #2942's 800-character line cut were untested.
2028 (test(openshell): explain failed outage redeploys and stuck namespace deletes #2940): when that listing hit its deadline, everything already read (phase, finalizers, and the Namespace conditions that usually say why it is stuck) was replaced by "listing what remains timed out".
2029 (test(plugins): list marker-bearing user messages when a codex turn lacks its prompt #2941): the evidence script parsed every transcript row, including old rows outside the window it checks, so one malformed history row failed a passing run.
2030 (test(plugins): list marker-bearing user messages when a codex turn lacks its prompt #2941): the denied-path message and the "newest ten" cut of markerUserMessages were untested.

Change

Test helpers and their tests only:

tests/helpers/qa-installation.mjs: the body of f.run moves unchanged into an exported runQaCommand(f, command, args, options), which f.run calls, so a test can drive it with a real failing subprocess and no cluster (2025). imageBuildFailureOutput masks private keys over all of stderr before taking the failed step and its last lines (2026).
tests/helpers/kubernetes-real.mjs: withinDiagnosticDeadline takes an optional sleep (as retryKubectlRead already does), passed through by deleteNamespaceExplained, so a test can hold the deadline's timer. The listing records each line as it reads it; a timed-out listing reports those lines, then "listing what remains timed out" (2028). A read that the deadline aborted adds no "listing ... failed" line, so that report is deterministic.
tests/helpers/plugin-driver-real.mjs: the evidence script skips rows before the evidence window that are not JSON (parseEvent) instead of throwing (2029). A malformed row inside the window still fails the evidence ("transcript row N in the evidence window is not JSON"), so a no-tool-call or denied check can't pass over a row that might hide a call.
Tests:
- tests/integration/qa-fixture-inputs.test.mjs: the runner records a failed build step as evidence with a registered secret, an environment secret, a bearer token and a PEM body all masked, lines cut at 800 characters, and nothing recorded for a protected command (2025, 2027); an unterminated key with 250 body lines leaves none in the 200 recorded lines (2026).
- tests/conformance/kubernetes-real-kubectl.test.mjs: the 40 s default deadline applies, a held or stalled listing keeps the phase, finalizers and conditions it read, a real stand-in kubectl killed by the deadline reports exactly the read lines plus the timeout, and the 40-object cap and stderr cuts hold (2027, 2028).
- tests/integration/plugin-driver-real-preflight.test.mjs: a malformed row before the window doesn't fail the run, and one inside the window fails the no-tool-call check (2029); a denied turn with 12 marker messages names the denial and lists the newest ten, and a non-string idempotency key reads as null (2030).

All fake values; no secret is printed.

Verification

Mutant	Killed by
record the step's lines unredacted; record raw stderr; replace the call with `Promise.resolve(undefined)`; drop the `privateFailureOutput` gate	QA command runner test
`lineChars` 800 -> 1000	QA command runner test
cut before masking (main); mask only the cut window	unterminated private key test
listing called without `withinDiagnosticDeadline`; `listTimeoutMs` 40000 -> 600000	Namespace listing deadline test
drop the partial result on timeout	Namespace listing deadline test
object cap 40 -> 50; stderr cut 300 -> 400; keep every stderr line	Namespace listing caps test
strict `JSON.parse` in the window search or the marker listing	session evidence test
malformed in-window row silently skipped	session evidence test
deadline-aborted read still adds a "listing objects failed" line	Namespace listing "timeout alone" test (fails on the first-round code too)
drop `markerUserMessages` from the denied message; change its text; remove `.slice(-10)`; `.slice(0, 10)`; accept any idempotency key type	session evidence test

The 2026, 2028 and 2029 tests fail on main.
20/20 at 2 CPUs each: plugin-driver-real-preflight, qa-fixture-inputs, kubernetes-real-kubectl, before and after the internal review's changes (in-window malformed rows, aborted reads, masking comment). Two survivors after review: keeping only one half of the abort guard (AbortError or deadline passed), since both are true together on this path. k3d-failure-diagnostics (also uses withinDiagnosticDeadline) passes. 113/113 across the four files in one run.
No new test files, so no suite mapping change. Prettier and eslint pass on the touched files; these .mjs helpers are outside tsc's project.
Open PRs touching these files: no new conflicted file or hunk (git merge-tree, main vs this head): feat(runtime): apply Gateway tool policy to dedicated OCE deployments #2633 6/6, feat(plugins): let administrators approve pinned plugin releases #2398 3/3, fix(runtime): copy public install helpers for Podman builds #1543 19/19, Add OCE policy simulation and tester for network and plugin actions #2445 4/4, Draft implementation: managed outbound HTTPS for dedicated Gateways #2518 0/0.

Risks

runQaCommand reads env and the directories from the installation object f, which holds the same objects f.run used before; behaviour is unchanged.
A timed-out listing now reports partial lines followed by the timeout note, so its text is longer; it is still bounded by the same caps.
Findings: 2025-2030 (bughunt-129).

… keys over all of stderr Findings 2025 and 2026. The failed image build evidence was only tested through the exported helpers, so the runner's wiring (redaction, the protected-output gate, recording at all) could regress unseen. The runner moves into an exported runQaCommand(f, ...) that f.run calls, and a test runs it with a real failing subprocess and no cluster: the evidence file is written, keeps the failed step, masks the registered, environment, bearer and PEM values the subprocess printed, cuts lines at 800 characters, and is not written for protected output. imageBuildFailureOutput cut stderr to the failed step's last 200 lines before redaction masked private keys, so an unterminated key whose BEGIN line fell outside the window kept its body. Keys are now masked over all of stderr first.

…in its bounds Findings 2027 and 2028. When the listing of what holds a Namespace hit its deadline, the whole description was replaced by 'listing what remains timed out', dropping the phase, finalizers and conditions already read. The listing now pushes lines as it reads them, and a timed-out listing reports them with the timeout note. withinDiagnosticDeadline takes an injectable sleep (as retryKubectlRead does), passed through deleteNamespaceExplained, so a test holds the deadline's timer and reads the 40 s default instead of waiting for it. New tests pin that default and the partial result (held and real timers), the 40-object cap, and the 300-character first-line cut of a failed read's stderr.

…turn's marker listing Findings 2029 and 2030. The session evidence script JSON-parsed every transcript row of the session, including rows long before the evidence window, so one malformed old row made it exit 1 on a passing run. Rows that are not JSON now establish nothing and are skipped. A new preflight test proves a passing turn with a malformed earlier row, and a denied Codex turn without its prompt mirror: its failure message names the denied turn and lists the newest ten marker-bearing user messages, reporting only string idempotency keys.

… malformed rows Two mutants survived the new tests: keeping every stderr line of a failed listing read (the 300-character cut hid it), and parsing in-window rows strictly (the window search stops at the marker before the old malformed row). A failed discovery read with a short two-line stderr and a malformed row inside the evidence window now kill them.

clawsweeper · 2026-10-19T15:05:09Z

🦞👀
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.

…s window Review of the finding 2029 fix: skipping every malformed transcript row let a truncated tool-call row inside the evidence window pass the disabled-tool phase's check that the turn made no call. Only rows before the window are skipped now; one inside it fails the evidence script, naming the row. The test keeps the earlier-turn malformed row passing and expects the no-call check to fail on an in-window truncated tool-call row. Also says in qa-installation why an unterminated private key earlier in stderr leaves the image build evidence all markers: masking runs to the end, the safe direction, as in redactedQaTail.

…isting Review of the finding 2028 fix: a read the deadline stops rejects with an AbortError as the deadline passes, and its partialKubectlRead catch could push 'listing objects failed: The operation was aborted' before the timed-out listing was reported. The catch now rethrows an AbortError, or any failure after the deadline, without adding a line. The new test runs a stand-in kubectl on PATH through the real client, so the stalled object read is a child process the deadline kills; it failed with the extra line before this change.

clawsweeper · 2026-10-19T15:20:07Z

Codex review: needs maintainer review before merge.

What this changes

This PR hardens build-output redaction, preserves partial namespace diagnostics, and confines strict transcript parsing to the current evidence window.

Example: A failed image build prints an unterminated private key followed by 250 key-body lines.

Before: Trimming to 200 lines before masking can retain private-key body text.
After: Masking the complete stderr before trimming removes the key body from recorded evidence.

Review scores

Measure	Result	What it means
Overall readiness	🦐 gold shrimp (3/6)	Useful, focused repairs with credible harness coverage and no actionable findings; confidence is supported by source inspection and reported tests rather than shipped-entry-point proof.
Proof confidence	🦐 gold shrimp (3/6)	This maintainer-authored test-helper change is outside the ordinary contributor proof gate; reported subprocess and SQLite tests exercise its diagnostic owners, while inspected UI media does not demonstrate these changes.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: Contributors investigating failed QA runs can receive incomplete diagnostics, lose evidence to malformed historical rows, or retain private-key body text in build evidence.
Reason: The maintainer-authored direction repairs source-visible diagnostic defects and adds focused coverage without expanding platform behavior.

Merge readiness

✅ Ready for maintainer review

Keep this PR open for maintainer triage: it repairs concrete QA diagnostic failures, and the reviewed patch has no actionable findings.

Priority: P3
Reviewed head: 58e8dcb148508ee3a73e2a69fb34039deb7736bf

Before merge

None.

Findings

None.

Agent review details

How this fits together

QA helpers consume subprocess stderr, Kubernetes metadata, and SQLite transcript events, then produce bounded failure messages and redacted evidence artifacts.

flowchart TD
  A[QA scenarios] --> B[Command runner]
  B --> C[Build output redaction]
  C --> F[Failure evidence]
  A --> D[Namespace diagnostics]
  D --> F
  A --> E[Transcript evidence reader]
  E --> F

Technical review

Best possible solution:

Retain these focused repairs in the existing diagnostic owners and land them after required checks succeed.

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

Current main visibly trims build output before key masking, discards partial namespace reads on timeout, and parses unrelated historical transcript rows strictly; focused regressions cover each path, but no tests were run during this review.

Is this the best way to solve the issue?

The repairs preserve existing owners, evidence-window boundaries, redaction gates, and diagnostic limits while addressing the failure causes directly.

AGENTS.md: found and applied where relevant.

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

Provenance checked

Image-build evidence selection and redaction keeps the original intent (8fe0716: Preserve the failed build step as bounded, redacted uploaded QA evidence when the ordinary stderr tail loses its cause.)
Namespace listing and diagnostic deadlines keeps the original intent (b891f54: Explain stuck namespace deletion using metadata-only reads bounded within teardown limits.)
Diagnostic timer and cancellation keeps the original intent (42fb0a9: Prevent stalled diagnostic reads from hiding the original test failure.)
Current-turn transcript window keeps the original intent (b4b6a0e: Earlier allowed results in a shared session must not establish evidence for the current turn.)
QA command execution keeps the original intent (ca55503: Share command execution within the canonical installation QA workflow; later changes added private-output and redaction protections.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

Reviewed change and policy: Reviewed the pinned introduction diff and original helper implementations. Root AGENTS.md applies; no nested tests AGENTS.md, maintainer-notes directory, or pr-simplify skill was found. The requested single-reviewer, read-only procedure overrides the bundled autoreview-helper instruction; no helper, second reviewer, or tests were run, and the checkout remains clean. (AGENTS.md, 58e8dcb14850)
Redaction repair preserves protected output: imageBuildFailureOutput now masks private keys over all stderr before selecting and trimming the failed step. The extracted runner retains the privateFailureOutput gate, environment-secret redaction, and private command-output handling. (tests/helpers/qa-installation.mjs:84, 58e8dcb14850)
Partial namespace evidence survives timeout: The listing accumulates completed reads outside the deadline race and includes them with its timeout result; deadline-aborted reads cannot append a misleading read-failure line. Existing object, stderr, request-timeout, and deletion bounds remain intact. (tests/helpers/kubernetes-real.mjs:145, 58e8dcb14850)
Transcript evidence remains strict within its window: Malformed historical rows no longer abort window discovery or marker diagnostics, while a malformed row inside the selected window throws before a no-tool-call assertion can succeed. The current-turn boundary remains unchanged. (tests/helpers/plugin-driver-real.mjs:578, 58e8dcb14850)
Distinct regression coverage: Added cases exercise the real command helper with a failing subprocess, the evidence script against local SQLite, and deadline cancellation through the real kubectl client with a stand-in executable. The body reports failures on main for the three repaired behaviors and 113 passing tests across four files; those runs were not independently repeated. (tests/integration/qa-fixture-inputs.test.mjs:387, 58e8dcb14850)
Earlier diagnostics and original intent: The merged diagnostics in test(openshell): explain failed outage redeploys and stuck namespace deletes #2940, test(plugins): list marker-bearing user messages when a codex turn lacks its prompt #2941, and test(qa): keep the failed image build step's output as qa evidence #2942 establish the bounded, metadata-only, redacted evidence paths being hardened. [plugins] Add real Codex per-call approval integration coverage #387 explains why transcript evidence must remain scoped to the current turn. (def181770d66)

Likely related people:

Free Wortley: Raw commit 42fb0a9 adds tests/helpers/kubernetes-real.mjs:75 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 42fb0a903372; files: tests/helpers/kubernetes-real.mjs)
kevinlin-openai: Raw commit ca55503 adds tests/helpers/qa-installation.mjs:120 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: ca5550332760; files: tests/helpers/qa-installation.mjs)
stevenlee-oai: Raw commit b4b6a0e adds tests/helpers/plugin-driver-real.mjs:489 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: b4b6a0e0d870; files: tests/helpers/plugin-driver-real.mjs)

Review metrics

Metric	Value	Why it matters
Diff allocation	Product code +0/-0; QA helpers +98/-59; tests +432/-0	Growth covers distinct redaction, timeout, cancellation, and transcript failures; helper churn includes extracting the existing command runner.

Labels

Label changes:

No label changes.

Label justifications:

P3: The change improves contributor-facing QA diagnostics and regression coverage 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.

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.

History

Review history (1 earlier review cycle)

reviewed 2026-10-19T15:18:47.935Z sha e9ac9ea :: needs maintainer review before merge. :: none

Reviewed October 19, 2026, 11:44 AM ET / 15:44 UTC (Revision 2).

freeqaz · 2026-10-19T15:40:44Z

@clawsweeper re-review

clawsweeper · 2026-10-19T15:40:53Z

🦞👀
Exact review queued.

Re-review progress:

State: Complete
Detail: The durable review result and its route handoff completed.
Run: https://github.com/openclaw/clawsweeper/actions/runs/39280523303
Updated: 2026-10-19T15:47:09.157Z

freeqaz added 4 commits October 19, 2026 14:44

freeqaz requested a review from a team as a code owner October 19, 2026 15:05

freeqaz added 2 commits October 19, 2026 15:16

freeqaz merged commit cadeaf2 into main Oct 19, 2026
60 of 63 checks passed

freeqaz deleted the ux-fixes-131-diag branch October 19, 2026 16:33

freeqaz mentioned this pull request Oct 19, 2026

fix(console): the Namespace recheck survives a failed read and stops on terminal states #2944

Merged

Navigation Menu

Repository navigation

fix(test): harden the new failure diagnostics - #2946

fix(test): harden the new failure diagnostics#2946
freeqaz merged 6 commits into
mainfrom
ux-fixes-131-diag

freeqaz commented Oct 19, 2026 •

edited

Loading

Uh oh!

clawsweeper Bot commented Oct 19, 2026 •

edited

Loading

Uh oh!

clawsweeper Bot commented Oct 19, 2026 •

edited

Loading

How this fits together

Technical review

Provenance checked

Testing

Security

Evidence

Review metrics

Labels

Rating scale

Workflow

History

Uh oh!

freeqaz commented Oct 19, 2026

Uh oh!

clawsweeper Bot commented Oct 19, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

freeqaz commented Oct 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Change

Verification

Risks

Uh oh!

clawsweeper Bot commented Oct 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ClawSweeper review complete

Uh oh!

clawsweeper Bot commented Oct 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this changes

Review scores

Product

Merge readiness

Before merge

Findings

How this fits together

Technical review

Provenance checked

Testing

Security

Evidence

Review metrics

Labels

Rating scale

Workflow

History

Uh oh!

freeqaz commented Oct 19, 2026

Uh oh!

clawsweeper Bot commented Oct 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

freeqaz commented Oct 19, 2026 •

edited

Loading

clawsweeper Bot commented Oct 19, 2026 •

edited

Loading

clawsweeper Bot commented Oct 19, 2026 •

edited

Loading

clawsweeper Bot commented Oct 19, 2026 •

edited

Loading