fix(cli): surface post-scan write failures and sanitize diagnostics.json - #134
Conversation
The handoff prompt points the agent at .docker-doctor/ and tells it to read the full report there. The per-rule .txt files run every message and path through sanitizeMessage and sanitizePath, which flatten control characters and cap the length; diagnostics.json, the file the prompt names first, was written straight from the report. Rule messages quote Dockerfile lines verbatim, so that was the one agent-facing surface where scanned content arrived unflattened and unbounded, which is exactly what the sanitizer exists to prevent. help text is authored by the rules, so it stays as written.
The interactive wizard ran inside catch {} with an "Ignore prompt
errors" comment, but the prompts never reject. What that block actually
caught was every write in the post-scan path: the GitHub workflow
scaffold, .docker-doctor/, the .gitignore entry, the skill install.
On a read-only checkout the user answered yes, nothing was written, and
the CLI exited 0 without a word. Name the failure and fold it into the
exit code. Scan-incomplete still outranks it, so a run that could not
read a Dockerfile keeps reporting 2 rather than 1.
The success paths already assign process.exitCode and return, which lets Node flush a pending write before it exits. The five error paths called process.exit instead, which does not wait: piping --json into a file and hitting an error could truncate the message the CI job then prints. The GitHub Action redirects both streams to files, so it sits squarely in that case. Signal handling keeps process.exit: 130 and 143 are meant to be immediate.
🦋 Changeset detectedLatest commit: 79e30ca The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest Docker Doctor scan for this pull request. Learn more about Docker Doctor.
Score: 84 / 100 · 3 issues
Scanned by Docker Doctor for commit |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reachedNext included review available in 52 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Security review: nothing to raise. Read the wizard error handling and the diagnostics.json sanitization, which closes the one agent‑facing surface that previously bypassed it. The sanitizer caps length and flattens control characters, exactly as the per‑rule .txt files already do. |
PunGrumpy
added
kind: bug
commit: |
1 participant
Description
Three problems on the post-scan path, all about failures that never reached the user.
The wizard swallowed every write failure. The whole interactive block ran inside
catch {}with an "Ignore prompt errors" comment, but the prompts never reject. What that block actually caught was every write on the path: the GitHub workflow scaffold,.docker-doctor/, the.gitignoreentry, the skill install. On a read-only checkout the user answered yes, nothing was written, and the CLI exited 0 without a word.Scan-incomplete still outranks it, so a run that could not read a Dockerfile keeps reporting 2 rather than 1.
diagnostics.jsonwas the one agent-facing surface that skipped the sanitizer. The per-rule.txtfiles run every message and path throughsanitizeMessageandsanitizePath, which flatten control characters and cap the length.diagnostics.json, the file the handoff prompt names first and tells the agent to read in full, was written straight from the report. Rule messages quote Dockerfile lines verbatim, so that was exactly the gap the sanitizer exists to close.helpis authored by the rules, never by scanned content, so it stays as written.Five error paths called
process.exit. The success paths already assignprocess.exitCodeand return, which lets Node flush a pending write first.process.exitdoes not wait, so piping--jsoninto a file and hitting an error could truncate the message the CI job then prints.action.ymlredirects both streams to files, so it sits squarely in that case. Signal handling keepsprocess.exit: 130 and 143 are meant to be immediate.Related Issues
N/A. Found while tracing what the agent handoff writes and what it tells the agent to read.
Checklist
Screenshots (if applicable)
N/A, CLI output only.
Additional Notes
Tests: a
diagnostics-dircase asserting the JSON and the.txtreport agree on a message carrying a newline and an ANSI escape, and a CLI case pinningrules explainon an unknown rule at exit 1 now that it no longer callsprocess.exit. The existing "large--jsonoutput over a pipe is not truncated" test guards the third change. CLI: 40 pass.Both halves of the wizard change were also exercised by hand under a pty: the happy path prints exactly what it did before and exits 0, and a
chmod 555checkout produces the message above and exits 1.Two judgment calls to flag:
runInteractiveWizardreturns a boolean rather than the caller readingprocess.exitCodeback, so the precedence between a wizard failure and an incomplete scan is explicit at the call site.POST_SCAN_FAILURE_EXIT_CODEis a named constant next to the existingSCAN_FAILURE_EXIT_CODEso the precedence comment has somewhere to live. Bare1literals elsewhere are untouched.Considered and left out: prompting before
.docker-doctor/is recreated or.gitignoreis appended. The directory is tool-owned by name and the menu already says what happens, so a second prompt costs more than it protects.