Repository navigation
Implement DiffVouch as a standalone Go CLI - #4
Conversation
![diffvouch[bot]](https://avatars.githubusercontent.com/in/4838354?s=60&v=4){kind=small}
There was a problem hiding this comment.
DiffVouch review
Rating: 2.5/5 — Needs work
The implementation has strong bounded-diff and structured-output foundations, but it should not merge before untrusted filenames are safely rendered and publication performs its final SHA validation after preparing the review. Runtime disclosure of disabled redaction, early gate validation, and rating boundary corrections are also needed.
Blocking issues
- high · Escape untrusted filenames before displaying them
internal/gitdiff/gitdiff.go:354
A repository can use filenames containing newline, ESC, or other terminal-control characters to forge output or emit terminal escape sequences when DiffVouch reports skipped/excluded files or finding locations. Treating repository paths as untrusted requires escaping control characters at presentation boundaries.
Recommendation: Retain raw paths internally, but quote or escape control characters before rendering them to terminals, logs, errors, or review Markdown. Add tests using newline and ESC-containing filenames. - medium · Revalidate the PR after preparing publication
internal/github/github.go:758
The PR can be updated after the validation but before review creation. Because the request explicitly supplies the previously reviewed commit_id, this can publish a review for stale code despite the advertised base/head safety check.
Recommendation: Fetch and process the PR diff first, then re-fetch and validate the live base/head immediately before creating the review. Where supported, also use an API precondition to narrow the remaining race.
Non-blocking issues
- medium · Warn at runtime before sending an unredacted patch
internal/review/review.go:151
Users who have not read the documentation receive no warning that credentials in the patch will be transmitted unchanged. This contradicts the stated requirement to warn while redaction is disabled and can lead to accidental secret disclosure.
Recommendation: Emit a prominent warning before the first provider call whenever unredacted patch submission is enabled, including for JSON/output-file workflows through an appropriate diagnostics channel. - medium · Validate quality-gate severity before provider calls
internal/review/review.go:128
A typo in --fail-on-severity, or an invalid configured threshold, can incur API cost and spend several minutes reviewing before the command discards the result with an argument error.
Recommendation: Validate both CLI and repository-configured severity thresholds before collecting the patch or invoking a provider. - medium · Align rating labels with the documented ranges
internal/rating/rating.go:34
The documented ranges define 1.5–2.4 as High risk and 2.5–3.4 as Needs work. Consequently, scores from 2.0 through 2.4 are mislabeled; a critical finding capped at 1.5 is also labeled correctly only by coincidence with the lower branch.
Recommendation: Change the High risk upper threshold to 2.5 and retain Needs work for scores from 2.5 through 3.4. Add boundary tests for 1.4, 1.5, 2.4, 2.5, 3.4, and 3.5.
Needs verification
- Verify whether the GitHub review endpoint offers a suitable conditional-request mechanism for making publication validation stronger than a final pre-POST refetch.
Rating breakdown
| Dimension | Score |
|---|---|
| Correctness | 3.2/5 |
| Security | 2.6/5 |
| Maintainability | 4.0/5 |
| Testing | 3.4/5 |
| Scope | 4.5/5 |
Provider: codex/cli · Model: gpt-5.6-sol · Reviewed commit: dd3d61a904876e9bf8f20ecf9d48e50181272a3f
There was a problem hiding this comment.
DiffVouch review
Rating: 2.5/5 — Needs work
Reviewed the complete patch in 2 chunks. Chunk 1 has strong fail-closed collection and publication checks, but it contains two blocking trust-boundary issues: Claude is launched from the untrusted repository, and arbitrary provider text is rendered directly to terminals. Oversized patches also have an edge-case chunk-corruption defect. The fallback store has a blocking lost-update race because its read-modify-write operations are not serialized. It also panics when the fallback file contains valid JSON null.
Blocking issues
- high · Claude reviews are launched inside the untrusted repository
internal/provider/provider.go:152
The Claude review therefore starts in the invoking repository. Claude Code can load repository-scoped instructions, settings, and lifecycle hooks before reviewing the supplied patch. A malicious change can consequently influence the supposedly trusted review context or cause repository-configured hook commands to run, violating the stated isolation and no-repository-code-execution guarantees.
Recommendation: Run Claude from a newly created empty temporary directory and explicitly disable project/local setting sources, hooks, plugins, and MCP configuration. Add a test using a repository-local configuration or startup hook to verify it is neither loaded nor executed. - high · Untrusted provider text reaches the terminal without escaping
internal/render/render.go:22
Provider responses are untrusted even after schema validation. A response containing ANSI/OSC terminal sequences can manipulate terminal output, forge subsequent diagnostics, create misleading hyperlinks, or modify the clipboard on supporting terminals. Only file paths currently receive terminal-safe escaping.
Recommendation: Apply a terminal-specific sanitizer to every provider-controlled string before rendering human-readable output, escaping control and formatting characters while leaving JSON output unchanged. Cover summaries, all finding fields, observations, and verification notes with tests containing CSI and OSC sequences. - high · Concurrent fallback updates can silently lose secrets
internal/secret/store.go:54
Concurrent Store/Delete calls can read the same snapshot and then overwrite each other. For example, two successful stores of different names can leave only the last writer's value, even though both calls returned success. This can silently lose credentials.
Recommendation: Serialize the complete read-modify-write transaction with a lock that works across processes, then atomically replace the file while holding that lock.
Non-blocking issues
- medium · Chunk splitting can mistake filename text for a hunk header
internal/gitdiff/gitdiff.go:397
A valid filename or other header value containing@@can be selected as the hunk boundary. For a file exceeding the chunk limit, this truncates the repeated file header and sends malformed chunks to the provider while the final result is still labeled complete. Findings may then be misattributed or discarded during location validation.
Recommendation: Locate the first hunk with the same line-anchored expression used for subsequent hunks, such as(?m)^@@, and add an oversized-file test whose quoted filename contains@@. - low · JSON null fallback file causes a panic
internal/secret/store.go:54
A syntactically validsecrets.jsoncontainingnullcrashes Store instead of returning an error or recovering with an empty store.
Recommendation: After unmarshalling, reject a nil map as invalid or initialize it withmap[string]string{}before assignment.
Needs verification
- Review patch chunk 2 before making an overall merge decision; omitted implementation was not assessed here.
Rating breakdown
| Dimension | Score |
|---|---|
| Correctness | 3.1/5 |
| Security | 2.0/5 |
| Maintainability | 3.7/5 |
| Testing | 3.4/5 |
| Scope | 4.4/5 |
Provider: codex/cli · Model: gpt-5.6-sol · Reviewed commit: 9bc7d264494735b3843ddf995d4245f51c42cd38
| args = append(args, "--effort", a.effort) | ||
| } | ||
| args = append(args, "-p") | ||
| command := exec.CommandContext(ctx, "claude", args...) |
There was a problem hiding this comment.
Claude reviews are launched inside the untrusted repository
The Claude review therefore starts in the invoking repository. Claude Code can load repository-scoped instructions, settings, and lifecycle hooks before reviewing the supplied patch. A malicious change can consequently influence the supposedly trusted review context or cause repository-configured hook commands to run, violating the stated isolation and no-repository-code-execution guarantees.
Recommendation: Run Claude from a newly created empty temporary directory and explicitly disable project/local setting sources, hooks, plugins, and MCP configuration. Add a test using a repository-local configuration or startup hook to verify it is neither loaded nor executed.
|
|
||
| func Terminal(result model.ReviewResult) string { | ||
| var output strings.Builder | ||
| fmt.Fprintf(&output, "DiffVouch review: %.1f/5 — %s\n\n%s\n\n", result.Rating.Overall, result.Rating.Label, result.Summary) |
There was a problem hiding this comment.
Untrusted provider text reaches the terminal without escaping
Provider responses are untrusted even after schema validation. A response containing ANSI/OSC terminal sequences can manipulate terminal output, forge subsequent diagnostics, create misleading hyperlinks, or modify the clipboard on supporting terminals. Only file paths currently receive terminal-safe escaping.
Recommendation: Apply a terminal-specific sanitizer to every provider-controlled string before rendering human-readable output, escaping control and formatting characters while leaving JSON output unchanged. Cover summaries, all finding fields, observations, and verification notes with tests containing CSI and OSC sequences.
| current.WriteString(section) | ||
| continue | ||
| } | ||
| hunkAt := strings.Index(section, "@@ ") |
There was a problem hiding this comment.
Chunk splitting can mistake filename text for a hunk header
A valid filename or other header value containing @@ can be selected as the hunk boundary. For a file exceeding the chunk limit, this truncates the repeated file header and sends malformed chunks to the provider while the final result is still labeled complete. Findings may then be misattributed or discarded during location validation.
Recommendation: Locate the first hunk with the same line-anchored expression used for subsequent hunks, such as (?m)^@@ , and add an oversized-file test whose quoted filename contains @@ .
| values, err := loadFile() | ||
| if err != nil { | ||
| return Ref{}, err | ||
| } |
There was a problem hiding this comment.
Concurrent fallback updates can silently lose secrets
Concurrent Store/Delete calls can read the same snapshot and then overwrite each other. For example, two successful stores of different names can leave only the last writer's value, even though both calls returned success. This can silently lose credentials.
Recommendation: Serialize the complete read-modify-write transaction with a lock that works across processes, then atomically replace the file while holding that lock.
| values, err := loadFile() | ||
| if err != nil { | ||
| return Ref{}, err | ||
| } |
There was a problem hiding this comment.
JSON null fallback file causes a panic
A syntactically valid secrets.json containing null crashes Store instead of returning an error or recovering with an empty store.
Recommendation: After unmarshalling, reject a nil map as invalid or initialize it with map[string]string{} before assignment.
There was a problem hiding this comment.
DiffVouch review
Rating: 2.5/5 — Needs work
Reviewed the complete patch in 2 chunks. Chunk 1 introduces a substantial, well-tested CLI foundation, but the Claude adapter lacks filesystem isolation and GitHub publication trusts provider-generated Markdown. Large GitHub diffs can also be silently truncated, and concurrent credential configuration is not safely serialized. No actionable issue is evidenced by this patch chunk. The secret-store implementation has sensible backend validation, locking, error handling, and focused regression coverage.
Blocking issues
- high · Claude reviewer runs inside the untrusted repository
internal/provider/provider.go:152
Claude Code can discover project-level instructions, settings, or hooks from its working directory. Running it inside the reviewed repository undermines the promised isolation and may allow repository-controlled configuration to influence the review or execute during CLI startup.
Recommendation: Run Claude from a newly created empty temporary directory and explicitly disable project/local settings, hooks, and automatic repository discovery using supported Claude CLI controls. - medium · Untrusted provider text is published as raw GitHub Markdown
internal/github/github.go:803
Provider output is derived from adversarial patch text and must remain untrusted. Markdown such as mentions, links, HTML, or forged headings can therefore be published by the configured bot, potentially notifying users or making the review misleading.
Recommendation: Escape or neutralize GitHub Markdown and mentions in all provider-controlled fields before constructing overall and inline review bodies. Preserve only deliberately supported formatting.
Non-blocking issues
- medium · Large GitHub diffs are silently truncated during publication
internal/github/github.go:512
A pull-request diff larger than 8 MiB is silently truncated.Publishtreats that partial diff as complete, drops inline findings whose lines occur after the cutoff, and can still report successful publication.
Recommendation: Read up to8<<20 + 1bytes and fail closed when the extra byte is present, or use a complete paginated/file-based mechanism for determining commentable lines. - medium · Concurrent configuration writes can corrupt or lose credentials
internal/privatefile/privatefile.go:15
Two simultaneous authentication or GitHub configuration commands can truncate and write the same temporary inode, encounter rename races, or overwrite each other's independently updated configuration. This can corrupt the JSON file or lose credential references.
Recommendation: Serialize the entire global-config read/modify/write transaction with a cross-process lock and use a uniquely created temporary file in the destination directory before atomic replacement. - low · API-transport validation test passes for the wrong reason
internal/config/config_test.go:70
The assertion passes because the trusted commit does not contain the file, not becausedefault_transport: apireaches and fails configuration validation. Removing the API-transport validation could leave this test green.
Recommendation: Commitunsafe.ymlbefore loading it, or place it outside the repository so parsing reachesvalidate, and assert that the returned error specifically identifies API transport.
Needs verification
- Confirm whether every supported Claude CLI version's
--safe-modedisables project instructions, settings, and startup hooks; the adapter does not otherwise isolate its working directory. - The supplied chunk relies on internal/privatefile.Write for secure permissions and write behavior, but that implementation was not included in this chunk.
Rating breakdown
| Dimension | Score |
|---|---|
| Correctness | 3.4/5 |
| Security | 2.7/5 |
| Maintainability | 3.5/5 |
| Testing | 3.6/5 |
| Scope | 3.9/5 |
Provider: codex/cli · Model: gpt-5.6-sol · Reviewed commit: 581ecc227138b57d496fe035ca239e0c2a5f6afa
| args = append(args, "--effort", a.effort) | ||
| } | ||
| args = append(args, "-p") | ||
| command := exec.CommandContext(ctx, "claude", args...) |
There was a problem hiding this comment.
Medium confidence: Claude reviewer runs inside the untrusted repository
Claude Code can discover project-level instructions, settings, or hooks from its working directory. Running it inside the reviewed repository undermines the promised isolation and may allow repository-controlled configuration to influence the review or execute during CLI startup.
Recommendation: Run Claude from a newly created empty temporary directory and explicitly disable project/local settings, hooks, and automatic repository discovery using supported Claude CLI controls.
| } | ||
| locationText += "`" | ||
| } | ||
| fmt.Fprintf(&output, "- **%s · %s**%s\n %s\n **Recommendation:** %s\n", finding.Severity, finding.Title, locationText, finding.Explanation, finding.Recommendation) |
There was a problem hiding this comment.
Untrusted provider text is published as raw GitHub Markdown
Provider output is derived from adversarial patch text and must remain untrusted. Markdown such as mentions, links, HTML, or forged headings can therefore be published by the configured bot, potentially notifying users or making the review misleading.
Recommendation: Escape or neutralize GitHub Markdown and mentions in all provider-controlled fields before constructing overall and inline review bodies. Preserve only deliberately supported formatting.
| if response.StatusCode < 200 || response.StatusCode >= 300 { | ||
| detail := string(raw) | ||
| if len(detail) > 4096 { | ||
| detail = detail[:4096] |
There was a problem hiding this comment.
Large GitHub diffs are silently truncated during publication
A pull-request diff larger than 8 MiB is silently truncated. Publish treats that partial diff as complete, drops inline findings whose lines occur after the cutoff, and can still report successful publication.
Recommendation: Read up to 8<<20 + 1 bytes and fail closed when the extra byte is present, or use a complete paginated/file-based mechanism for determining commentable lines.
| return err | ||
| } | ||
| _ = os.Chmod(filepath.Dir(path), 0o700) | ||
| temporary := path + ".tmp" |
There was a problem hiding this comment.
Concurrent configuration writes can corrupt or lose credentials
Two simultaneous authentication or GitHub configuration commands can truncate and write the same temporary inode, encounter rename races, or overwrite each other's independently updated configuration. This can corrupt the JSON file or lose credential references.
Recommendation: Serialize the entire global-config read/modify/write transaction with a cross-process lock and use a uniquely created temporary file in the destination directory before atomic replacement.
| _ = os.WriteFile(path, []byte("version: 1\nprovider:\n default_transport: api\n"), 0o600) | ||
| if _, err := LoadRepository(repo, path, "HEAD"); err == nil { | ||
| t.Fatal("API transport should not be selectable by repository config") | ||
| } |
There was a problem hiding this comment.
API-transport validation test passes for the wrong reason
The assertion passes because the trusted commit does not contain the file, not because default_transport: api reaches and fails configuration validation. Removing the API-transport validation could leave this test green.
Recommendation: Commit unsafe.yml before loading it, or place it outside the repository so parsing reaches validate, and assert that the returned error specifically identifies API transport.
Summary
diffvouchThis Go CLI supersedes the Python CLI approach in #3; that PR is left unchanged for reference.
Security and publication behavior
COMMENTeventValidation
go test -race ./...go vet ./...git diff --checkgpt-5.6-solat high effort