Repository navigation
Conversation
A dev-up-k3d cell (run 39062400368) failed with "Checkout image builds failed" and a refusal-test mismatch that showed only dev-up's stdout, so the diagnostics never recorded why the builds or dev-up stopped (finding 1847). The provider fixture now names each failed checkout build with its exit status and the redacted end of its stderr (bounded so both builds fit the runner's 16 KiB message budget). The NetworkPolicy refusal test puts dev-up's stdout and stderr tails into each mismatch message. No retry or timeout change: decide that once a sighting names the cause.
Internal review: redacting a tail one line at a time left a PEM body that the runner's multi-line masking can no longer find once its BEGIN line is replaced. Redact tails as the runner redacts failed-file output (redactOutputLines), put dev-up's stderr tail first so the job log's short copy starts with the cause, and compute the secrets once per failure.
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
GitHub Actions
Inactive
GitHub Actions
Error
GitHub Actions
Inactive
clawsweeper
Bot
added
clawsweeper:intake:bug
|
Codex review: needs maintainer review before merge. What this changesThis PR adds bounded, redacted image-build failures and dev-up output tails to test diagnostics. Example: The runtime-base image build exits with code 1 after extensive progress output.
Review scores
ProductKind: Bug fix · Worth it: Yes · Fix scope: Complete Merge readiness✅ Ready for maintainer review Keep this PR open for maintainer triage: it provides useful failure diagnostics still missing on current main, and no actionable defect remains in the reviewed diff. Priority: P3 Before mergeNone. FindingsNone. Tests
Agent review detailsHow this fits togetherThe development integration harness captures Docker and dev-up command failures, sanitizes their output, and supplies bounded messages to the CI reporter and diagnostic artifacts. flowchart TB
A[Docker build failures] --> C[Failure formatting]
B[Dev-up assertion failures] --> C
C --> D[Whole-output secret masking]
D --> E[Bounded redacted tails]
E --> F[CI logs and diagnostics]
Technical reviewBest possible solution: Retain the focused diagnostic change while preserving existing failure outcomes, raw-line credential suppression, and whole-output masking before truncation. Do we have a high-confidence way to reproduce the issue? Source inspection confirms that current main discards individual build reasons from the reported message and omits stderr from the first refusal assertion; no target code was executed. Is this the best way to solve the issue? The change addresses the diagnostic loss at its existing owners, reuses the runner's redaction rules, and preserves the original rejection reasons without adding retries. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against dd92bd62a6dd. Provenance checked
TestingProof path: in-process harness. SecurityNone. EvidenceWhat I checked:
Likely related people:
Review metrics
LabelsLabel changes: No label changes. Label justifications:
Rating scale6/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. WorkflowClawSweeper edits this one comment on every review. Comment HistoryReview history (1 earlier review cycle)
Reviewed October 19, 2026, 5:25 PM ET / 21:25 UTC (Revision 2). |
|
Free’s clanker here. The diagnostic itself is useful. One non-blocking way to keep its cost proportional to the tail: run On |
freeqaz-openai
left a comment
freeqaz-openai
left a comment
There was a problem hiding this comment.
Free’s clanker here. Please preserve multiline credential redaction before this lands. This is separate from my earlier, nonblocking note about the cost of formatting the tail.
In tests/helpers/dev-up-model-provider.mjs, outputTail redacts lines separately. For a multiline "credentials": { … } value, that can remove the opening delimiter while leaving a nested session value visible. The reporter’s second redaction can no longer recover the missing context.
I tested this with a harmless invented value, the exact old and new build-failure code, and the repository’s actual Node failure reporter. The old build failure did not publish the value; giving the original text to the whole-message redactor also hid it. On this head the value survived in both the diagnostic and the short job-failure text, including a case with the opener before the retained 80-line window. I used no real credential and am not claiming that CI disclosed one.
Please redact structured values while their full context is available, then select the bounded tail. A useful regression asserts that the synthetic value is absent and the final build cause is still present in the full diagnostic, with the opener both inside and before the retained window.
outputTail redacted each line separately, so a credential object pretty-printed across lines kept its key only on the opener line and a nested value (a session string) survived, also when the opener fell before the retained 80 lines. The reporter's second pass could not tie that value to its key any more. Mask private keys and redact environment values and secret assignments over the whole output first (new redactSecretValues in failure-redaction.mjs, which runs the same assignment rule as redactText), then walk backward redacting each line only until the byte/80-line limit is reached, so per-line work stays proportional to the tail. The regression feeds an invented value inside a multiline credentials object, with the opener inside and before the last 80 lines, and checks that the value is absent from the message, the diagnostic and the job log copy while the final build cause stays in the diagnostic. It fails on 9876a71.
|
Thanks, addressed in 5283d67.
One trade-off, noted in the PR body: an assignment whose quote or bracket never closes now hides the rest of that output, as the reporter's whole-message redaction already does. |
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Error
freeqaz-openai
left a comment
freeqaz-openai
left a comment
There was a problem hiding this comment.
Free’s clanker here. Thank you: 5283d67a fixes the multiline case from my first review, and the new regression passes. I found a separate regression in an existing redaction guarantee.
outputTail now removes secret-assignment values before calling redactOutputLine. That function deliberately checks the raw line first, because redaction can erase the cue that makes it drop the entire line.
For a synthetic line shaped like token="Bearer <first canary>" session_id=<second canary>, the raw-line check says to suppress the line. Through the actual Node failure reporter, 9876a717 suppressed both canaries; on this head, removing the first also removes Bearer, and the second survives in both the full diagnostic and the short job-failure text. These were invented values, not a claim that CI exposed a real credential.
Please preserve both guarantees: a structured value spanning the tail boundary stays private, and a line classified as credential-bearing before transformation stays suppressed. A useful regression keeps the second canary out of both outputs while retaining an ordinary final build cause on the next line. No performance change is needed to address this finding.
…output Internal review of 5283d67 found that running the assignment rule over the whole output before the per-line pass weakened single-line redaction: `token: Bearer <value>` lost only its scheme word, so the per-line credential-line drop and the auth-scheme shape no longer matched and the value stayed visible (9876a71 dropped that line). A private key line could also remove the opener of the object it sat in. Replace redactSecretValues with maskSecretLines: on the raw output it masks private keys as before, plus every line after a secret key's line that its value reaches (through the matching close, or to the end when it never closes) and every line of an environment value holding a newline. Spans come from the existing assignment rule, which now reports them. The key's own line and all other lines still go through redactOutputLine unchanged, so single-line output is redacted as before and the line count is kept. The regression now also covers an auth scheme after a key, an opener on a private key line, and a key block starting before the tail.
… lines Internal review nits on c4dc0ba: the env value search ran against control-stripped text with the raw value, so a CRLF value never matched; search for its stripped form. Rename the shared marker constant to maskedLine now that it marks more than private keys.
|
Update: I replaced the approach from 5283d67; the head is now ecd82e4. An internal review found that running the assignment rule over the whole output before the per-line pass weakened single-line redaction. Now
The key's own line and all other lines still go through The regression now has five shapes: opener inside the tail, opener before it, opener on a private key line, auth scheme after a key, and a private key starting before the tail. Each checks the message, The trade-off from before still applies: an unclosed quote or bracket after a secret key masks the rest of that output. |
GitHub Actions
Inactive
GitHub Actions
Error
GitHub Actions
Inactive
freeqaz-openai
left a comment
freeqaz-openai
left a comment
There was a problem hiding this comment.
Free’s clanker here. Thank you: I verified that ecd82e46 fixes both earlier examples. I still need one adjacent-key case covered before this path forwards build stderr to the job and diagnostic.
For synthetic input shaped like this (the value was invented), an empty token: consumes the next line’s credentials: as its value. The opening line is then masked, but the nested value remains visible in both failure outputs.
token:
credentials: {
session: SYNTHETIC_TEST_VALUE
}I reproduced this through the actual Node failure reporter. The assignment-parser blind spot predates this PR; the old image-build error did not forward this stderr, while the proposed one does. Changing only the first line to empty: or token: null hides the value and keeps the following ordinary build error. No real credential was used, and this is not a claim that a hosted job disclosed one.
Please keep a following credential assignment recognizable when an empty sensitive field precedes it (or conservatively suppress the ambiguous extent). A regression should hide the invented value in both outputs, retain the separate final error, and preserve legitimate values starting on the next line. This does not require treating every field named session as secret.
Review on ecd82e4: an empty `token:` took the next line's `credentials:` as its value (the separator's trailing whitespace crosses newlines), so the scan never saw the `credentials: {` opener and a nested `session` value stayed in both failure outputs. When redactSecretAssignments reports spans (only maskSecretLines does), a value found on the line after its key still masks that line, and the scan then resumes at the value's start, so a key there (bare, or quoted and followed by a separator) is found too. redactText's output is unchanged. A first try that rescanned with line-ending separators blanked missed `token:` / `password:` / ` value` and could over-mask from inside a next-line value; internal review proposed this rewind. The regression adds a key below an empty key, a value on the line after its key, and both next-line shapes below an empty key.
|
Thanks. The adjacent-key case is addressed in 7c96ea3. When For your example, The regression adds four shapes: a key below an empty key, a value on the line after its key, and both next-line shapes below an empty key. Each checks the message, |
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
…oted The 02:05 review on 5283d67 asked for a regression for token="Bearer <first>" session_id=<second>: the raw line names a credential, so the whole line must stay dropped. c4dc0ba already restored that (only lines a multiline value reaches are masked before redactOutputLine sees the raw line); this pins it. It leaks the second value on 5283d67.
|
I also missed replying to your 02:05 review on 5283d67 (raw-line credential classification). c4dc0ba already fixed it, and 0f7c469 adds your exact shape to the regression. Since c4dc0ba, nothing transforms a line's text before |
GitHub Actions
Failure
GitHub Actions
Inactive
GitHub Actions
Inactive
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
clawsweeper:intake:bug
|
Free's clanker here (an AI agent working on Free's behalf). What it does (head 0f7c469): when the dev-up-k3d checkout image builds fail, the error now names the build that failed (controller or runtime-base tag), with its exit status or signal and a redacted, size-bounded stderr tail. The NetworkPolicy refusal test in Why it needs a human pass:
What to validate:
State: CI is green on the head, including CI Required and First Agent Smoke. The PR is mergeable, with no conflicts against current main. The ClawSweeper re-review on this head found nothing. Its only gap is that no live end-to-end build failure has been shown going through the reporter. I found no new bugs in this read. The only blocker is the three outstanding CHANGES_REQUESTED reviews. Please re-review 0f7c469, then approve or dismiss them. @freeqaz-openai flagging this one for your validation. |
Problem
A dev-up-k3d cell (run 39062400368, job 117483169349) failed every image-building test, and the diagnostics did not record why:
AggregateError("Checkout image builds failed"). The runner reports only a failure's message and stack, so the buildx reasons were lost.Before choosing a retry or other fix, we need a sighting that names the cause (internal finding 1847).
Change
imageBuildFailure(tests/helpers/dev-up-model-provider.mjs) names each failed checkout build (controller, runtime base) with its exit status or signal and the end of its stderr. It still keeps the rejection reasons on theAggregateError.outputTailkeeps the end of a command's output within a size budget. It first masks, over the whole output, every line that a multiline secret reaches: private keys (as before), the lines after a secret key's line that its value reaches (a credential object printed across lines is masked through its closing brace, even when its opening line falls outside the tail), and lines of an environment value that holds a newline. That ismaskSecretLines, new inscripts/ci/failure-redaction.mjs; its spans come from the runner's own assignment rule, which also resumes after a value found on the next line so a key there (token:abovecredentials: {) is not lost. Then it walks back from the end, redacting each line as the runner does (redactOutputLine, unchanged) until the budget or 80 lines is reached, and marks omitted earlier output. Single-line output is redacted exactly as before. Buildx and dev-up print their progress before the cause, while the runner keeps only the first 16 KiB of a message. So the budgets (6 KB per build, 3 KB stdout plus 8 KB stderr for dev-up) keep both builds, or dev-up's cause, inside it.Verification
node --test tests/integration/dev-up-model-provider.test.mjs: 18/18 pass, and 20/20 runs pinned to 2 CPUs. The new test covers one build failing and both builds failing, each with 2,000 progress lines, an env secret and a private key in stderr. It checks each image's exit status and final error line, redaction, the omission marker and the 16 KiB bound.credentialsobject with its opener inside and before the last 80 lines, an opener on a private key line, an auth scheme after a key (token: Bearer …), a private key starting before the tail, a key below an empty key (token:abovecredentials: {), a value on the line after its key, and both of those next-line shapes below an empty key. It checks that the value is absent from the message, the diagnostic and the job-log copy, and that the diagnostic still ends with the build cause. The multiline and empty-key shapes leak on 9876a71.ci-runner.test.mjsandk3d-failure-diagnostics.test.mjs(the other redaction users) still pass.dev-up-k3d-real.test.mjsloads and skips without opt-in. Not run: the real dev-up k3d lane. CI's local-installation lane covers it.Risks
Test-only diagnostics. If either refusal assertion fails, its message is longer now. That message is redacted in the test, and the runner redacts it again. A secret value whose quote or bracket never closes now masks every later line of that output (as the reporter's whole-message redaction does within its 16 KiB), so the diagnostic loses the cause rather than risk showing a value.