Conversation
docker --mount is CSV, so a path containing a comma split the mount and broke every sandboxed scanner. Quote each field the way Docker's CSV parser expects. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
|
🦞👀 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. |
|
Codex review: needs real behavior proof before merge. Reviewed October 5, 2026, 12:10 AM ET / 04:10 UTC (Revision 3). ClawSweeper reviewWhat this changesThe PR escapes Docker bind-mount fields containing commas or quotes and adds tests covering path round trips and mount permissions. Merge readiness⛔ Blocked before merge - 2 items remain The fix remains necessary: current main and v0.2.0 still emit unquoted mount fields. No actionable patch defect was found, but the prior request for Docker-backed runtime proof remains outstanding. Priority: P2 Review scores
Verification
How this fits togetherClawScan runs command-backed scanners and judges through a Docker sandbox. The sandbox runner converts target paths, working directories, and explicit mounts into Docker arguments before executing the scanner or judge. flowchart LR
A[Scanner or judge command] --> C[Sandbox runner]
B[Target paths and explicit mounts] --> C
C --> D[Select mount permissions]
D --> E[Encode mount fields as CSV]
E --> F[Docker container]
F --> G[Scanner or judge output]
Before merge
Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Keep CSV escaping at the Docker argument boundary while preserving ordinary-path output and existing read-only versus writable mount decisions. Do we have a high-confidence way to reproduce the issue? Yes, from source: an existing absolute path containing a comma reaches unescaped --mount construction on main, where Docker interprets the comma as a field separator. No runtime reproduction was executed during this read-only review. Is this the best way to solve the issue? Yes. Encoding complete mount fields as CSV matches Docker's parser and repairs the existing argument boundary without changing configuration or mount permissions. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 490bd167ea43. LabelsLabel changes: No label changes. Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
History |
What Problem This Solves
docker --mount is CSV, so a path containing a comma split the mount and
broke every sandboxed scanner. Quote each field the way Docker's CSV
parser expects.
Why This Change Was Made
This patch is limited to the files below.
internal/runner/runner_test.gointernal/runner/sandbox.goUser Impact
The case described above now follows the patched behavior. Existing commands and configuration stay in place.
Evidence
Patched commit
b5ff8ffe05af66eb54edc5c9a770c10b294b1d97onfix/clawscan-f004in/tmp/oc-batch/clawscan.Real behavior proof
/tmp/oc-batch/clawscan, commitb5ff8ffe05afcd /tmp/oc-batch/clawscan && go test -count=1 -timeout 180s -run TestDockerMounts ./internal/runner/