Repository navigation
fix(runner): quote docker mount fields that contain commas - #64
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 maintainer review before merge. Reviewed October 7, 2026, 6:12 AM ET / 10:12 UTC (Revision 7). ClawSweeper reviewWhat this changesThe PR encodes Docker bind mounts as CSV records and adds CLI integration coverage for paths containing commas and quotation marks while preserving mount permissions. Merge readiness✅ Ready for maintainer review The fix remains necessary on main, and no blocking defect was found. The updated head resolves the previous proof gap through a successful Docker-backed CLI integration step. Priority: P2 Review scores
Verification
How this fits togetherClawScan runs command-backed scanners and judges inside Docker by default. Its sandbox runner converts target paths and configured mounts into Docker arguments that control container access to host files. flowchart TD
A[Scan target] --> C[Sandbox runner]
B[Configured mounts] --> C
C --> D[Select mount permissions]
D --> E[Encode CSV mount records]
E --> F[Docker scanner process]
F --> G[Raw scanner evidence]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Use standard CSV encoding at the Docker mount boundary and retain integration coverage that checks actual container access and permissions. Do we have a high-confidence way to reproduce the issue? Yes, source provides a high-confidence path: current main inserts comma-containing host paths directly into Docker's CSV mount argument. No failing current-main execution was performed during this read-only review. Is this the best way to solve the issue? Yes, standard CSV encoding repairs the serialization boundary while preserving existing permission selection, and the new integration coverage exercises Docker acceptance and filesystem effects. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 6cde6b180106. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (6 earlier review cycles)
|
* chore(autoreview): use the shared canonical skill * docs(autoreview): preserve repository review priority
Encode each Docker mount as a CSV record and preserve the existing permission decisions. Add CLI integration coverage for comma/quote paths, read-only targets and rules, and writable caches; run it in CI. Co-authored-by: Sebastien Tardif <sebtardif@ncf.ca>
|
Merged as ff17e1d. Thanks @SebTardif. The v0.2.0 binary reproduces Docker's CSV parse failure on the synthetic comma/quote path. The fixed CLI passed real Docker integration tests for ordinary and comma/quote paths, including denied target/rules writes and successful explicit cache writes. The test now runs in CI. Validation passed on a disposable Linux host: |
Docker parses each
--mountargument as a CSV record. ClawScan previously concatenated paths into that record without escaping them, so a skill or explicit mount such asMy Skill, "v2"failed before its sandbox could start.Encode each complete mount record with Go's CSV writer while preserving mount selection and permissions. Add serialization coverage and a real ClawScan CLI integration test that checks target/rules read access, rejected writes to read-only mounts, and successful writes to an explicitly writable cache. Run the Docker test in CI and document the supported paths.
Thanks @SebTardif for the fix and initial regression test.
Validation:
bare " in non-quoted-field.go test -count=1 ./...,go vet ./..., CLI build/help/static scan,make docs-site, npm wrapper/helper tests, and npm pack smoke all pass on a disposable Linux host.go build ./cmd/clawscan && docker pull alpine:3.23 && node --test scripts/docker-mounts.test.mjspasses against real Docker. Both ordinary paths and comma/quote paths retain the intended permissions:5fb0301c5d2d6742eb6327a548786a19869fe742, including the new Docker check. CodeQL and submission validation also passed.