test(#146): add TestMainSubprocess_InvalidDocMapPath and TestMainSubprocess_InvalidDocMapFile #151

2026-05-15T08:29:52Z

rodin commented

2026-05-15 08:29:52 +00:00

Summary

Adds two missing subprocess tests for --doc-map flag validation in main().

Changes

cmd/review-bot/main.go

Move --doc-map path validation earlier (before network I/O) so it can be tested without a running server

cmd/review-bot/main_test.go

TestMainSubprocess_InvalidDocMapPath: passes --doc-map ../../../etc/passwd and asserts path traversal rejection
TestMainSubprocess_InvalidDocMapFile: passes --doc-map nonexistent.yml and asserts resolve failure

Both tests follow the existing TEST_SUBPROCESS_MAIN=1 env gate pattern with cleanEnv() + explicit GITHUB_WORKSPACE.

Closes #146

## Summary Adds two missing subprocess tests for `--doc-map` flag validation in `main()`. ## Changes ### cmd/review-bot/main.go - Move `--doc-map` path validation earlier (before network I/O) so it can be tested without a running server ### cmd/review-bot/main_test.go - `TestMainSubprocess_InvalidDocMapPath`: passes `--doc-map ../../../etc/passwd` and asserts path traversal rejection - `TestMainSubprocess_InvalidDocMapFile`: passes `--doc-map nonexistent.yml` and asserts resolve failure Both tests follow the existing `TEST_SUBPROCESS_MAIN=1` env gate pattern with `cleanEnv()` + explicit `GITHUB_WORKSPACE`. Closes #146

rodin added the ai-review label 2026-05-15 08:29:59 +00:00

sonnet-review-bot approved these changes 2026-05-15 08:30:40 +00:00

sonnet-review-bot left a comment

First-time contributor

~~Original review~~

Superseded — see current review for up-to-date findings.

Previous findings (commit 40a16b75)

Sonnet Review

Summary

Clean, focused PR that moves doc-map path validation earlier (fail-fast before network I/O) and adds two subprocess tests to cover the new early-exit paths. The changes follow established patterns in the codebase and are correct.

Findings

#	Severity	File	Line	Finding
1	[NIT]	`cmd/review-bot/main.go`	176	The early validation at line 176 calls validateWorkspacePath and discards the resolved path (using `_`). Then at the later doc-map processing block (Step 6c), validateWorkspacePath is called again on the same path. This is a double call to EvalSymlinks on the happy path. Consider storing the resolved path in a variable (e.g., a map or a local var) to avoid the redundant syscall, or restructure so the early check and the later usage share the resolved result.
2	[NIT]	`cmd/review-bot/main_test.go`	1508	Both new subprocess tests pass the same set of required flags verbatim (copy-paste). If the required-flag set changes, both tests need updating. This is a minor maintainability concern, not a correctness issue — a shared helper (like the existing cleanEnv pattern) could be used to build the base args slice, but it's low priority given the established pattern in the file.

Recommendation

APPROVE — Approve. The production change is correct and well-motivated: moving the validation before network I/O prevents wasted round-trips on bad input, and the two new tests accurately exercise both failure modes (path traversal rejection and nonexistent file). The tests follow the established TEST_SUBPROCESS_MAIN=1 subprocess gate pattern with cleanEnv() + explicit GITHUB_WORKSPACE, matching the codebase convention. The only findings are minor nits — a redundant EvalSymlinks call on the happy path (the early check's resolved path is thrown away and recomputed later) and a bit of copy-paste in the test args setup — neither of which warrants blocking the PR.

Review by sonnet

Evaluated against 40a16b75

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/151#pullrequestreview-4107) for up-to-date findings. <details><summary>Previous findings (commit 40a16b75)</summary> # Sonnet Review ## Summary Clean, focused PR that moves doc-map path validation earlier (fail-fast before network I/O) and adds two subprocess tests to cover the new early-exit paths. The changes follow established patterns in the codebase and are correct. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [NIT] | `cmd/review-bot/main.go` | 176 | The early validation at line 176 calls validateWorkspacePath and discards the resolved path (using `_`). Then at the later doc-map processing block (Step 6c), validateWorkspacePath is called again on the same path. This is a double call to EvalSymlinks on the happy path. Consider storing the resolved path in a variable (e.g., a map or a local var) to avoid the redundant syscall, or restructure so the early check and the later usage share the resolved result. | | 2 | [NIT] | `cmd/review-bot/main_test.go` | 1508 | Both new subprocess tests pass the same set of required flags verbatim (copy-paste). If the required-flag set changes, both tests need updating. This is a minor maintainability concern, not a correctness issue — a shared helper (like the existing cleanEnv pattern) could be used to build the base args slice, but it's low priority given the established pattern in the file. | ## Recommendation **APPROVE** — Approve. The production change is correct and well-motivated: moving the validation before network I/O prevents wasted round-trips on bad input, and the two new tests accurately exercise both failure modes (path traversal rejection and nonexistent file). The tests follow the established TEST_SUBPROCESS_MAIN=1 subprocess gate pattern with cleanEnv() + explicit GITHUB_WORKSPACE, matching the codebase convention. The only findings are minor nits — a redundant EvalSymlinks call on the happy path (the early check's resolved path is thrown away and recomputed later) and a bit of copy-paste in the test args setup — neither of which warrants blocking the PR. --- *Review by sonnet*  --- *Evaluated against 40a16b75* </details>

cmd/review-bot/main.go Outdated

						
				@@ -173,6 +173,14 @@ func main() {

						os.Exit(1)

					}

					// Early validation of filesystem-path flags (fail fast before network I/O)

sonnet-review-bot commented

2026-05-15 08:30:40 +00:00

First-time contributor

[NIT] The early validation at line 176 calls validateWorkspacePath and discards the resolved path (using _). Then at the later doc-map processing block (Step 6c), validateWorkspacePath is called again on the same path. This is a double call to EvalSymlinks on the happy path. Consider storing the resolved path in a variable (e.g., a map or a local var) to avoid the redundant syscall, or restructure so the early check and the later usage share the resolved result.

**[NIT]** The early validation at line 176 calls validateWorkspacePath and discards the resolved path (using `_`). Then at the later doc-map processing block (Step 6c), validateWorkspacePath is called again on the same path. This is a double call to EvalSymlinks on the happy path. Consider storing the resolved path in a variable (e.g., a map or a local var) to avoid the redundant syscall, or restructure so the early check and the later usage share the resolved result.

cmd/review-bot/main_test.go Outdated

						
				@@ -1506,3 +1506,77 @@ func TestMainSubprocess_DeprecatedGiteaURLEnv(t *testing.T) {

						t.Errorf("expected deprecation warning for GITEA_URL, got: %s", out)

					}

				}

sonnet-review-bot commented

2026-05-15 08:30:40 +00:00

First-time contributor

[NIT] Both new subprocess tests pass the same set of required flags verbatim (copy-paste). If the required-flag set changes, both tests need updating. This is a minor maintainability concern, not a correctness issue — a shared helper (like the existing cleanEnv pattern) could be used to build the base args slice, but it's low priority given the established pattern in the file.

**[NIT]** Both new subprocess tests pass the same set of required flags verbatim (copy-paste). If the required-flag set changes, both tests need updating. This is a minor maintainability concern, not a correctness issue — a shared helper (like the existing cleanEnv pattern) could be used to build the base args slice, but it's low priority given the established pattern in the file.

security-review-bot requested review from security-review-bot 2026-05-15 08:30:49 +00:00

security-review-bot approved these changes 2026-05-15 08:30:49 +00:00

Dismissed

security-review-bot left a comment

~~Original review~~

Superseded — see current review for up-to-date findings.

Previous findings (commit 40a16b75)

Security Review

Summary

Security posture is improved by moving --doc-map path validation earlier, preventing path traversal attempts before any network I/O. Added subprocess tests robustly verify rejection of unsafe or nonexistent paths. CI passed and no new vulnerabilities are introduced.

Recommendation

APPROVE — The changes appropriately enforce workspace-bound path validation earlier in program execution and add targeted tests to prevent path traversal and invalid file usage for --doc-map. No security issues detected; proceed with merge.

Review by security

Evaluated against 40a16b75

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/151#pullrequestreview-4112) for up-to-date findings. <details><summary>Previous findings (commit 40a16b75)</summary> # Security Review ## Summary Security posture is improved by moving --doc-map path validation earlier, preventing path traversal attempts before any network I/O. Added subprocess tests robustly verify rejection of unsafe or nonexistent paths. CI passed and no new vulnerabilities are introduced. ## Recommendation **APPROVE** — The changes appropriately enforce workspace-bound path validation earlier in program execution and add targeted tests to prevent path traversal and invalid file usage for --doc-map. No security issues detected; proceed with merge. --- *Review by security*  --- *Evaluated against 40a16b75* </details>

gpt-review-bot approved these changes 2026-05-15 08:30:59 +00:00

Dismissed

gpt-review-bot left a comment

First-time contributor

~~Original review~~

Superseded — see current review for up-to-date findings.

Previous findings (commit 40a16b75)

Gpt Review

Summary

Change cleanly adds early validation for --doc-map and introduces two robust subprocess tests to cover path traversal and missing file cases. CI passed and the implementation is consistent with existing patterns and error handling.

Findings

#	Severity	File	Line	Finding
1	[NIT]	`cmd/review-bot/main.go`	173	The doc-map path is validated twice (early and again in Step 6c). Consider capturing and reusing the resolved path from the early validation to avoid duplicate work.

Recommendation

APPROVE — The early validation for --doc-map is a good improvement, enabling failure before any network I/O and making the new subprocess tests feasible. Tests follow the established subprocess pattern and set GITHUB_WORKSPACE explicitly, which is correct for path validation. As a minor enhancement, consider storing the resolved doc-map path during early validation and reusing it later to avoid re-validating. Otherwise, the changes are sound and adhere to the repository’s error-handling and testing conventions.

Review by gpt

Evaluated against 40a16b75

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/151#pullrequestreview-4108) for up-to-date findings. <details><summary>Previous findings (commit 40a16b75)</summary> # Gpt Review ## Summary Change cleanly adds early validation for --doc-map and introduces two robust subprocess tests to cover path traversal and missing file cases. CI passed and the implementation is consistent with existing patterns and error handling. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [NIT] | `cmd/review-bot/main.go` | 173 | The doc-map path is validated twice (early and again in Step 6c). Consider capturing and reusing the resolved path from the early validation to avoid duplicate work. | ## Recommendation **APPROVE** — The early validation for --doc-map is a good improvement, enabling failure before any network I/O and making the new subprocess tests feasible. Tests follow the established subprocess pattern and set GITHUB_WORKSPACE explicitly, which is correct for path validation. As a minor enhancement, consider storing the resolved doc-map path during early validation and reusing it later to avoid re-validating. Otherwise, the changes are sound and adhere to the repository’s error-handling and testing conventions. --- *Review by gpt*  --- *Evaluated against 40a16b75* </details>

cmd/review-bot/main.go

						
				@@ -173,6 +173,14 @@ func main() {

						os.Exit(1)

gpt-review-bot commented

2026-05-15 08:30:59 +00:00

First-time contributor

[NIT] The doc-map path is validated twice (early and again in Step 6c). Consider capturing and reusing the resolved path from the early validation to avoid duplicate work.

**[NIT]** The doc-map path is validated twice (early and again in Step 6c). Consider capturing and reusing the resolved path from the early validation to avoid duplicate work.

rodin added the wip label 2026-05-15 08:31:43 +00:00

rodin commented

2026-05-15 08:33:29 +00:00

Self-Review: PR #151

Self-review against 40a16b75e0

Phase 1: Independent Findings

None — diff looks clean. The production change is a small, focused early-validation guard that correctly uses the existing validateWorkspacePath helper and follows the established error-handling pattern (slog.Error + os.Exit(1)). Tests follow the existing TEST_SUBPROCESS_MAIN=1 subprocess pattern with explicit GITHUB_WORKSPACE and cleanEnv(), which is correct for path validation testing.

One observation (NIT-level): the early validation discards the resolved path, which is then recomputed at line 368. This is intentional — the early check is a fail-fast guard; the later call is the "real" usage. The extra EvalSymlinks syscall on the happy path is negligible cost for cleaner separation of concerns.

Phase 2: Prior Review Verification

Finding	Reviewer	Status	Notes
Double `validateWorkspacePath` call (resolved path discarded early)	sonnet	ACKNOWLEDGED	Valid observation. The cost is one extra `EvalSymlinks` syscall — negligible. Keeping the early check as a pure guard (no state mutation) is arguably cleaner. Not worth restructuring.
Test args copy-paste across two test functions	sonnet	ACKNOWLEDGED	Valid but low-priority. The file already has many subprocess tests with the same pattern. A shared helper could help but would be a separate cleanup PR.
Duplicate doc-map validation	gpt	ACKNOWLEDGED	Same as sonnet finding #1. See above.
Security: no issues	security	RESOLVED	Clean pass.

Assessment: ✅ Clean

All three bots approved. Findings are NIT-level only (duplicate syscall, test boilerplate). The production change is correct, focused, and well-tested. Ready for merge.

## Self-Review: PR #151 Self-review against 40a16b75e061528467888ad19080fa8689004e20 ### Phase 1: Independent Findings None — diff looks clean. The production change is a small, focused early-validation guard that correctly uses the existing `validateWorkspacePath` helper and follows the established error-handling pattern (`slog.Error` + `os.Exit(1)`). Tests follow the existing `TEST_SUBPROCESS_MAIN=1` subprocess pattern with explicit `GITHUB_WORKSPACE` and `cleanEnv()`, which is correct for path validation testing. One observation (NIT-level): the early validation discards the resolved path, which is then recomputed at line 368. This is intentional — the early check is a fail-fast guard; the later call is the "real" usage. The extra `EvalSymlinks` syscall on the happy path is negligible cost for cleaner separation of concerns. ### Phase 2: Prior Review Verification | Finding | Reviewer | Status | Notes | |---------|----------|--------|-------| | Double `validateWorkspacePath` call (resolved path discarded early) | sonnet | ACKNOWLEDGED | Valid observation. The cost is one extra `EvalSymlinks` syscall — negligible. Keeping the early check as a pure guard (no state mutation) is arguably cleaner. Not worth restructuring. | | Test args copy-paste across two test functions | sonnet | ACKNOWLEDGED | Valid but low-priority. The file already has many subprocess tests with the same pattern. A shared helper could help but would be a separate cleanup PR. | | Duplicate doc-map validation | gpt | ACKNOWLEDGED | Same as sonnet finding #1. See above. | | Security: no issues | security | RESOLVED | Clean pass. | ### Assessment: ✅ Clean All three bots approved. Findings are NIT-level only (duplicate syscall, test boilerplate). The production change is correct, focused, and well-tested. Ready for merge.

rodin commented

2026-05-15 08:34:59 +00:00

Fix Plan against `40a16b75e0`:

Findings Summary

All findings are NIT-level. All three bots approved.

#	Reviewer	Severity	Finding	Category	Rationale
1	sonnet, gpt	[NIT]	`main.go`: Double `validateWorkspacePath` call — early guard discards resolved path, recomputed later at Step 6c	FIX	Concrete, actionable. Store resolved path from early validation and reuse it downstream to avoid redundant `EvalSymlinks` syscall.
2	sonnet	[NIT]	`main_test.go`: Copy-paste required flags across two test functions	DEFER	Valid maintainability concern, but this is a broader refactor of the test helpers. Filed as separate cleanup issue.

Plan

FIX #1 — Capture resolved path from early validateWorkspacePath call and pass it to the later Step 6c block instead of calling validateWorkspacePath again.

// Before (line ~176):
_, err := validateWorkspacePath(workspacePath, docMapPath)
if err != nil { ... }

// After:
resolvedDocMapPath, err := validateWorkspacePath(workspacePath, docMapPath)
if err != nil { ... }
// ... pass resolvedDocMapPath to Step 6c

DEFER #2 — Test helper refactor out of scope for this PR. Will file a separate issue.

Execution

Look at the actual code to understand where resolvedDocMapPath needs to flow
Update main.go to capture and reuse the resolved path
Run tests, push

## Fix Plan against 40a16b75e061528467888ad19080fa8689004e20: ### Findings Summary All findings are NIT-level. All three bots approved. | # | Reviewer | Severity | Finding | Category | Rationale | |---|----------|----------|---------|----------|-----------| | 1 | sonnet, gpt | [NIT] | `main.go`: Double `validateWorkspacePath` call — early guard discards resolved path, recomputed later at Step 6c | **FIX** | Concrete, actionable. Store resolved path from early validation and reuse it downstream to avoid redundant `EvalSymlinks` syscall. | | 2 | sonnet | [NIT] | `main_test.go`: Copy-paste required flags across two test functions | **DEFER** | Valid maintainability concern, but this is a broader refactor of the test helpers. Filed as separate cleanup issue. | ### Plan **FIX #1** — Capture resolved path from early `validateWorkspacePath` call and pass it to the later Step 6c block instead of calling `validateWorkspacePath` again. ```go // Before (line ~176): _, err := validateWorkspacePath(workspacePath, docMapPath) if err != nil { ... } // After: resolvedDocMapPath, err := validateWorkspacePath(workspacePath, docMapPath) if err != nil { ... } // ... pass resolvedDocMapPath to Step 6c ``` **DEFER #2** — Test helper refactor out of scope for this PR. Will file a separate issue. ### Execution 1. Look at the actual code to understand where resolvedDocMapPath needs to flow 2. Update main.go to capture and reuse the resolved path 3. Run tests, push

rodin referenced this pull request

2026-05-15 08:36:05 +00:00

refactor: extract shared base-args helper in main_test.go subprocess tests #154

rodin commented

2026-05-15 08:36:11 +00:00

Deferred — filed as #154 for a dedicated test-helper refactor pass.

rodin removed the wip label 2026-05-15 08:36:30 +00:00

sonnet-review-bot approved these changes 2026-05-15 08:37:07 +00:00

sonnet-review-bot left a comment

First-time contributor

~~Original review~~

Superseded — see current review for up-to-date findings.

Previous findings (commit 9b64c605)

Sonnet Review

Summary

Clean, focused PR that moves --doc-map path validation earlier (before network I/O) and adds two subprocess tests to cover the path-traversal and nonexistent-file error paths. The refactor eliminates code duplication by resolving the path once and reusing the result. Tests follow the established TEST_SUBPROCESS_MAIN=1 pattern with cleanEnv() + explicit GITHUB_WORKSPACE.

Findings

#	Severity	File	Line	Finding
1	[NIT]	`cmd/review-bot/main_test.go`	1530	The subprocess side of `TestMainSubprocess_InvalidDocMapPath` calls `main()` and returns, but `GITHUB_WORKSPACE` is set by the outer test via the environment string `"GITHUB_WORKSPACE="+t.TempDir()`. The subprocess process inherits that env var, so the temp dir created in the outer test process is passed as a string to the child — this is correct. Just worth noting that the outer `t.TempDir()` is evaluated in the outer process before `cmd.Env` is built, so it is a real, existing directory. No bug, just confirming the intent.

Recommendation

APPROVE — Approve. The production change is a safe mechanical refactor (resolve once, reuse) with no behavioral difference for valid paths. The two new tests are well-structured, follow existing patterns exactly, and provide meaningful coverage for security-relevant validation that was previously untested. The only nit is a clarifying observation, not a defect.

Review by sonnet

Evaluated against 9b64c605

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/151#pullrequestreview-4368) for up-to-date findings. <details><summary>Previous findings (commit 9b64c605)</summary> # Sonnet Review ## Summary Clean, focused PR that moves `--doc-map` path validation earlier (before network I/O) and adds two subprocess tests to cover the path-traversal and nonexistent-file error paths. The refactor eliminates code duplication by resolving the path once and reusing the result. Tests follow the established `TEST_SUBPROCESS_MAIN=1` pattern with `cleanEnv()` + explicit `GITHUB_WORKSPACE`. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [NIT] | `cmd/review-bot/main_test.go` | 1530 | The subprocess side of `TestMainSubprocess_InvalidDocMapPath` calls `main()` and returns, but `GITHUB_WORKSPACE` is set by the outer test via the environment string `"GITHUB_WORKSPACE="+t.TempDir()`. The subprocess process inherits that env var, so the temp dir created in the outer test process is passed as a string to the child — this is correct. Just worth noting that the outer `t.TempDir()` is evaluated in the outer process before `cmd.Env` is built, so it is a real, existing directory. No bug, just confirming the intent. | ## Recommendation **APPROVE** — Approve. The production change is a safe mechanical refactor (resolve once, reuse) with no behavioral difference for valid paths. The two new tests are well-structured, follow existing patterns exactly, and provide meaningful coverage for security-relevant validation that was previously untested. The only nit is a clarifying observation, not a defect. --- *Review by sonnet*  --- *Evaluated against 9b64c605* </details>

cmd/review-bot/main_test.go Outdated

						
				@@ -1509,0 +1527,4 @@

					}

					cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_InvalidDocMapPath")

					cmd.Env = append(cleanEnv(),

sonnet-review-bot commented

2026-05-15 08:37:07 +00:00

First-time contributor

[NIT] The subprocess side of TestMainSubprocess_InvalidDocMapPath calls main() and returns, but GITHUB_WORKSPACE is set by the outer test via the environment string "GITHUB_WORKSPACE="+t.TempDir(). The subprocess process inherits that env var, so the temp dir created in the outer test process is passed as a string to the child — this is correct. Just worth noting that the outer t.TempDir() is evaluated in the outer process before cmd.Env is built, so it is a real, existing directory. No bug, just confirming the intent.

**[NIT]** The subprocess side of `TestMainSubprocess_InvalidDocMapPath` calls `main()` and returns, but `GITHUB_WORKSPACE` is set by the outer test via the environment string `"GITHUB_WORKSPACE="+t.TempDir()`. The subprocess process inherits that env var, so the temp dir created in the outer test process is passed as a string to the child — this is correct. Just worth noting that the outer `t.TempDir()` is evaluated in the outer process before `cmd.Env` is built, so it is a real, existing directory. No bug, just confirming the intent.

gpt-review-bot approved these changes 2026-05-15 08:37:08 +00:00

Dismissed

gpt-review-bot left a comment

First-time contributor

~~Original review~~

Superseded — see current review for up-to-date findings.

Previous findings (commit 9b64c605)

Gpt Review

Summary

The changes correctly move doc-map path validation earlier to fail fast and add two subprocess tests that validate traversal and nonexistent file cases. CI is passing and the implementation is straightforward and idiomatic.

Recommendation

APPROVE — The early validation of --doc-map via validateWorkspacePath and reuse of the resolved path ensures errors surface before any network I/O, making the new subprocess tests reliable. The added tests follow the existing subprocess pattern and appropriately set GITHUB_WORKSPACE. No issues detected; proceed with merge.

Review by gpt

Evaluated against 9b64c605

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/151#pullrequestreview-4371) for up-to-date findings. <details><summary>Previous findings (commit 9b64c605)</summary> # Gpt Review ## Summary The changes correctly move doc-map path validation earlier to fail fast and add two subprocess tests that validate traversal and nonexistent file cases. CI is passing and the implementation is straightforward and idiomatic. ## Recommendation **APPROVE** — The early validation of --doc-map via validateWorkspacePath and reuse of the resolved path ensures errors surface before any network I/O, making the new subprocess tests reliable. The added tests follow the existing subprocess pattern and appropriately set GITHUB_WORKSPACE. No issues detected; proceed with merge. --- *Review by gpt*  --- *Evaluated against 9b64c605* </details>

rodin referenced this pull request

2026-05-15 08:37:55 +00:00

fix(#150): add EvalSymlinks to validateDocmapPath — close dir-symlink bypass #152

security-review-bot requested review from security-review-bot 2026-05-15 08:37:58 +00:00

security-review-bot approved these changes 2026-05-15 08:37:58 +00:00

Dismissed

security-review-bot left a comment

~~Original review~~

Superseded — see current review for up-to-date findings.

Previous findings (commit 9b64c605)

Security Review

Summary

The PR moves validation of the --doc-map path earlier in main(), reducing risk and enabling deterministic testing, and adds subprocess tests that verify rejection of path traversal and nonexistent files. No new vulnerabilities are introduced; overall this improves security posture.

Recommendation

APPROVE — CI has passed, and the change improves security by validating user-supplied file paths before any network I/O. The validateWorkspacePath approach correctly restricts to the workspace and resolves symlinks to prevent traversal. The added tests exercise these cases as intended. As a defense-in-depth consideration (not a required change), you could re-validate or re-check the resolved path immediately before opening the file to further minimize any TOCTOU window, though in this CLI context the practical risk is low. Overall, this is a solid improvement.

Review by security

Evaluated against 9b64c605

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/151#pullrequestreview-4370) for up-to-date findings. <details><summary>Previous findings (commit 9b64c605)</summary> # Security Review ## Summary The PR moves validation of the --doc-map path earlier in main(), reducing risk and enabling deterministic testing, and adds subprocess tests that verify rejection of path traversal and nonexistent files. No new vulnerabilities are introduced; overall this improves security posture. ## Recommendation **APPROVE** — CI has passed, and the change improves security by validating user-supplied file paths before any network I/O. The validateWorkspacePath approach correctly restricts to the workspace and resolves symlinks to prevent traversal. The added tests exercise these cases as intended. As a defense-in-depth consideration (not a required change), you could re-validate or re-check the resolved path immediately before opening the file to further minimize any TOCTOU window, though in this CLI context the practical risk is low. Overall, this is a solid improvement. --- *Review by security*  --- *Evaluated against 9b64c605* </details>

rodin added the wip label 2026-05-15 08:38:16 +00:00

rodin added self-reviewed ready and removed wip ai-review labels 2026-05-15 08:44:56 +00:00

rodin referenced this pull request

2026-05-15 08:51:43 +00:00

refactor(#154): extract baseSubprocessArgs helper in main_test.go subprocess tests #155

rodin added the wip label 2026-05-15 08:52:03 +00:00

rodin commented

2026-05-15 08:53:26 +00:00

Self-Review: PR #151

Self-review against 9b64c605f878eff32e29cc108db360fd64066059

Phase 1: Independent Findings

None — diff looks clean. The refactor correctly moves validateWorkspacePath earlier, stores the resolved path in resolvedDocMapFile, and reuses it at Step 6c. Error handling follows the existing pattern (slog.Error + os.Exit(1)). Tests use the established TEST_SUBPROCESS_MAIN=1 subprocess pattern with cleanEnv() and explicit GITHUB_WORKSPACE.

Phase 2: Prior Review Verification

Finding	Reviewer	Status	Notes
Double `validateWorkspacePath` call (resolved path discarded early, recomputed later)	sonnet, gpt	✅ RESOLVED	Current HEAD stores result in `resolvedDocMapFile` and reuses it downstream
Test copy-paste args (shared helper could reduce duplication)	sonnet	⏭️ DEFERRED	Filed as #154 for dedicated test-helper refactor pass
`t.TempDir()` evaluated in outer process (clarifying observation)	sonnet (round 2)	✅ N/A	Not a bug — confirmed correct behavior
TOCTOU defense-in-depth suggestion	security	✅ ACKNOWLEDGED	Low risk in CLI context; no action needed

Assessment: ✅ Clean

All three review bots approved. The one actionable finding from round 1 (double validation) has been fixed. The remaining NIT (test-helper refactor) is correctly deferred to #154. Ready to merge.

## Self-Review: PR #151 Self-review against `9b64c605f878eff32e29cc108db360fd64066059` ### Phase 1: Independent Findings None — diff looks clean. The refactor correctly moves `validateWorkspacePath` earlier, stores the resolved path in `resolvedDocMapFile`, and reuses it at Step 6c. Error handling follows the existing pattern (`slog.Error` + `os.Exit(1)`). Tests use the established `TEST_SUBPROCESS_MAIN=1` subprocess pattern with `cleanEnv()` and explicit `GITHUB_WORKSPACE`. ### Phase 2: Prior Review Verification | Finding | Reviewer | Status | Notes | |---------|----------|--------|-------| | Double `validateWorkspacePath` call (resolved path discarded early, recomputed later) | sonnet, gpt | ✅ RESOLVED | Current HEAD stores result in `resolvedDocMapFile` and reuses it downstream | | Test copy-paste args (shared helper could reduce duplication) | sonnet | ⏭️ DEFERRED | Filed as #154 for dedicated test-helper refactor pass | | `t.TempDir()` evaluated in outer process (clarifying observation) | sonnet (round 2) | ✅ N/A | Not a bug — confirmed correct behavior | | TOCTOU defense-in-depth suggestion | security | ✅ ACKNOWLEDGED | Low risk in CLI context; no action needed | ### Assessment: ✅ Clean All three review bots approved. The one actionable finding from round 1 (double validation) has been fixed. The remaining NIT (test-helper refactor) is correctly deferred to #154. Ready to merge.

rodin removed the wip label 2026-05-15 08:53:30 +00:00

rodin added the wip label 2026-05-15 08:55:11 +00:00

rodin removed the wip label 2026-05-15 09:08:18 +00:00

rodin added the wip label 2026-05-15 09:12:59 +00:00

rodin commented

2026-05-15 09:14:16 +00:00

Self-Review: PR #151

Self-review against 9b64c605f878eff32e29cc108db360fd64066059

Phase 1: Independent Findings

No issues found. The diff is clean and well-scoped:

Production change (main.go): Moves validateWorkspacePath call for --doc-map earlier in main(), before any network I/O. Stores the resolved path in resolvedDocMapFile and reuses it downstream at Step 6c. Error handling follows the established slog.Error + os.Exit(1) pattern. Variable scoping is correct — resolvedDocMapFile is declared in the outer scope and only assigned inside the conditional, so it flows cleanly to the later usage.
Tests (main_test.go): Two new subprocess tests covering path-traversal rejection (../../../etc/passwd) and nonexistent file rejection (nonexistent.yml). Both follow the TEST_SUBPROCESS_MAIN=1 env gate pattern with cleanEnv() + explicit GITHUB_WORKSPACE, consistent with all other subprocess tests in the file. Assertions are specific and meaningful (checking both the flag name and the error class).

Phase 2: Prior Review Verification

Finding	Reviewer	Status	Notes
Double `validateWorkspacePath` call (resolved path discarded)	sonnet (r1), gpt (r1)	✅ RESOLVED	Current HEAD stores result in `resolvedDocMapFile` and reuses at Step 6c — no duplicate syscall
Test copy-paste args (shared helper could reduce duplication)	sonnet (r1)	⏭️ DEFERRED	Filed as #154 for dedicated test-helper refactor
`t.TempDir()` evaluated in outer process (clarifying note)	sonnet (r2)	✅ N/A	Confirmed correct — not a bug
TOCTOU defense-in-depth suggestion	security (r2)	✅ ACKNOWLEDGED	Low risk in CLI context, no action needed

Assessment: ✅ Clean

All six bot reviews (3 bots × 2 rounds) approved. The one actionable finding (double validation) was fixed between rounds. Remaining deferred item (#154) is out of scope. No new findings on current HEAD.

## Self-Review: PR #151 Self-review against `9b64c605f878eff32e29cc108db360fd64066059` ### Phase 1: Independent Findings **No issues found.** The diff is clean and well-scoped: - **Production change** (`main.go`): Moves `validateWorkspacePath` call for `--doc-map` earlier in `main()`, before any network I/O. Stores the resolved path in `resolvedDocMapFile` and reuses it downstream at Step 6c. Error handling follows the established `slog.Error` + `os.Exit(1)` pattern. Variable scoping is correct — `resolvedDocMapFile` is declared in the outer scope and only assigned inside the conditional, so it flows cleanly to the later usage. - **Tests** (`main_test.go`): Two new subprocess tests covering path-traversal rejection (`../../../etc/passwd`) and nonexistent file rejection (`nonexistent.yml`). Both follow the `TEST_SUBPROCESS_MAIN=1` env gate pattern with `cleanEnv()` + explicit `GITHUB_WORKSPACE`, consistent with all other subprocess tests in the file. Assertions are specific and meaningful (checking both the flag name and the error class). ### Phase 2: Prior Review Verification | Finding | Reviewer | Status | Notes | |---------|----------|--------|-------| | Double `validateWorkspacePath` call (resolved path discarded) | sonnet (r1), gpt (r1) | ✅ RESOLVED | Current HEAD stores result in `resolvedDocMapFile` and reuses at Step 6c — no duplicate syscall | | Test copy-paste args (shared helper could reduce duplication) | sonnet (r1) | ⏭️ DEFERRED | Filed as #154 for dedicated test-helper refactor | | `t.TempDir()` evaluated in outer process (clarifying note) | sonnet (r2) | ✅ N/A | Confirmed correct — not a bug | | TOCTOU defense-in-depth suggestion | security (r2) | ✅ ACKNOWLEDGED | Low risk in CLI context, no action needed | ### Assessment: ✅ Clean All six bot reviews (3 bots × 2 rounds) approved. The one actionable finding (double validation) was fixed between rounds. Remaining deferred item (#154) is out of scope. No new findings on current HEAD.

rodin removed the wip label 2026-05-15 09:14:20 +00:00

rodin added the wip label 2026-05-15 09:39:20 +00:00

rodin referenced this pull request

2026-05-15 10:18:29 +00:00

feat(#143): fetch doc-map config from trusted VCS ref #153

rodin commented

2026-05-15 10:33:13 +00:00

Self-review against 9b64c605f8

Assessment: ✅ Clean

No issues found — ready for human review.

Summary: Two-commit PR that (1) moves --doc-map path validation earlier in main() before network I/O and adds the two required subprocess tests, then (2) eliminates the now-redundant second validateWorkspacePath call in Step 6c by reusing the resolved path from early validation. All acceptance criteria for #146 met. Build clean, vet clean, all tests pass.

Self-review against 9b64c605f878eff32e29cc108db360fd64066059 Assessment: ✅ Clean No issues found — ready for human review. **Summary:** Two-commit PR that (1) moves `--doc-map` path validation earlier in `main()` before network I/O and adds the two required subprocess tests, then (2) eliminates the now-redundant second `validateWorkspacePath` call in Step 6c by reusing the resolved path from early validation. All acceptance criteria for #146 met. Build clean, vet clean, all tests pass.

rodin commented

2026-05-15 11:04:53 +00:00

Acknowledging sonnet review NIT #1 (review 4107 @ 9b64c605):

Correct — t.TempDir() is evaluated in the outer (test driver) process before building cmd.Env, so the subprocess receives a real, existing directory path. This is intentional: the outer test creates the workspace directory, the subprocess validates paths against it. No action needed; this is an observation, not a defect.

**Acknowledging sonnet review NIT #1 (review 4107 @ 9b64c605):** Correct — `t.TempDir()` is evaluated in the outer (test driver) process before building `cmd.Env`, so the subprocess receives a real, existing directory path. This is intentional: the outer test creates the workspace directory, the subprocess validates paths against it. No action needed; this is an observation, not a defect.

rodin removed the wip label 2026-05-15 11:05:05 +00:00

rodin added the wip label 2026-05-15 11:57:43 +00:00

rodin commented

2026-05-15 11:59:40 +00:00

Fix Plan

Addressing: Review #4107, Finding #1 (NIT)

Finding: The subprocess tests call t.TempDir() inline when building cmd.Env, without a comment clarifying that it is evaluated in the outer test process (not the subprocess). The reviewer confirmed: no bug, just worth noting the intent.

Fix: Added a comment above both cmd.Env = append(cleanEnv(), ...) blocks in TestMainSubprocess_InvalidDocMapPath and TestMainSubprocess_InvalidDocMapFile to clarify that t.TempDir() is evaluated in the outer process, producing a real directory that gets passed as the GITHUB_WORKSPACE env string to the subprocess.

Commit: 290ce99 — test(#146): clarify t.TempDir() evaluation in subprocess env setup

## Fix Plan **Addressing:** Review #4107, Finding #1 (NIT) **Finding:** The subprocess tests call `t.TempDir()` inline when building `cmd.Env`, without a comment clarifying that it is evaluated in the outer test process (not the subprocess). The reviewer confirmed: no bug, just worth noting the intent. **Fix:** Added a comment above both `cmd.Env = append(cleanEnv(), ...)` blocks in `TestMainSubprocess_InvalidDocMapPath` and `TestMainSubprocess_InvalidDocMapFile` to clarify that `t.TempDir()` is evaluated in the outer process, producing a real directory that gets passed as the `GITHUB_WORKSPACE` env string to the subprocess. **Commit:** `290ce99` — `test(#146): clarify t.TempDir() evaluation in subprocess env setup`

rodin removed the wip label 2026-05-15 11:59:55 +00:00

sonnet-review-bot approved these changes 2026-05-15 12:00:08 +00:00

sonnet-review-bot left a comment

First-time contributor

Sonnet Review

Summary

The changes correctly move --doc-map path validation earlier in main() to enable testing without network I/O, and add two well-structured subprocess tests that follow the established TEST_SUBPROCESS_MAIN=1 pattern. The code is clean, idiomatic, and CI passes.

Findings

#	Severity	File	Line	Finding
1	[NIT]	`cmd/review-bot/main_test.go`	1528	The comment `// t.TempDir() is evaluated here in the outer process, producing a real directory / that is passed as the GITHUB_WORKSPACE env var string to the subprocess.` is duplicated verbatim in both new test functions. It's accurate and useful, but could be a shared helper (e.g. `workspaceEnv(t)`) to avoid repeating the same comment+pattern four times total (two existing tests already use similar patterns). Minor — not worth blocking.

Recommendation

APPROVE — Approve as-is. The refactor is minimal and correct: resolving the doc-map path once at the 'fail fast' boundary (before any client initialization or network I/O) is the right design, and the two new subprocess tests directly exercise the new early-exit paths. The only observation is minor test-code duplication in the workspace setup, which doesn't affect correctness or readability materially.

Review by sonnet

Evaluated against 290ce992

# Sonnet Review ## Summary The changes correctly move `--doc-map` path validation earlier in `main()` to enable testing without network I/O, and add two well-structured subprocess tests that follow the established `TEST_SUBPROCESS_MAIN=1` pattern. The code is clean, idiomatic, and CI passes. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [NIT] | `cmd/review-bot/main_test.go` | 1528 | The comment `// t.TempDir() is evaluated here in the outer process, producing a real directory / that is passed as the GITHUB_WORKSPACE env var string to the subprocess.` is duplicated verbatim in both new test functions. It's accurate and useful, but could be a shared helper (e.g. `workspaceEnv(t)`) to avoid repeating the same comment+pattern four times total (two existing tests already use similar patterns). Minor — not worth blocking. | ## Recommendation **APPROVE** — Approve as-is. The refactor is minimal and correct: resolving the doc-map path once at the 'fail fast' boundary (before any client initialization or network I/O) is the right design, and the two new subprocess tests directly exercise the new early-exit paths. The only observation is minor test-code duplication in the workspace setup, which doesn't affect correctness or readability materially. --- *Review by sonnet*  --- *Evaluated against 290ce992*

cmd/review-bot/main_test.go Outdated

						
				@@ -1509,0 +1525,4 @@

						main()

						return

					}

sonnet-review-bot commented

2026-05-15 12:00:08 +00:00

First-time contributor

[NIT] The comment // t.TempDir() is evaluated here in the outer process, producing a real directory / that is passed as the GITHUB_WORKSPACE env var string to the subprocess. is duplicated verbatim in both new test functions. It's accurate and useful, but could be a shared helper (e.g. workspaceEnv(t)) to avoid repeating the same comment+pattern four times total (two existing tests already use similar patterns). Minor — not worth blocking.

**[NIT]** The comment `// t.TempDir() is evaluated here in the outer process, producing a real directory / that is passed as the GITHUB_WORKSPACE env var string to the subprocess.` is duplicated verbatim in both new test functions. It's accurate and useful, but could be a shared helper (e.g. `workspaceEnv(t)`) to avoid repeating the same comment+pattern four times total (two existing tests already use similar patterns). Minor — not worth blocking.

security-review-bot requested review from security-review-bot 2026-05-15 12:00:33 +00:00

security-review-bot approved these changes 2026-05-15 12:00:33 +00:00

security-review-bot left a comment

Security Review

Summary

The change moves path validation for the --doc-map flag earlier to fail fast and adds subprocess tests covering path traversal and nonexistent file scenarios. This strengthens input validation and reduces risk by preventing any network I/O before rejecting unsafe paths.

Recommendation

APPROVE — CI has passed and the changes are security-positive: early validation with validateWorkspacePath prevents path traversal and symlink escapes, and the new tests exercise these failure modes. No new injection, auth, or secret-handling risks are introduced. Approve as is.

Review by security

Evaluated against 290ce992

# Security Review ## Summary The change moves path validation for the --doc-map flag earlier to fail fast and adds subprocess tests covering path traversal and nonexistent file scenarios. This strengthens input validation and reduces risk by preventing any network I/O before rejecting unsafe paths. ## Recommendation **APPROVE** — CI has passed and the changes are security-positive: early validation with validateWorkspacePath prevents path traversal and symlink escapes, and the new tests exercise these failure modes. No new injection, auth, or secret-handling risks are introduced. Approve as is. --- *Review by security*  --- *Evaluated against 290ce992*

gpt-review-bot approved these changes 2026-05-15 12:00:45 +00:00

gpt-review-bot left a comment

First-time contributor

Gpt Review

Summary

The changes cleanly move --doc-map path validation earlier to fail fast and add two robust subprocess tests covering path traversal and nonexistent files. Error handling, logging, and test structure are consistent and idiomatic, and CI has passed.

Recommendation

APPROVE — The early validation of the doc-map path before any network I/O is a sensible design improvement that makes the new tests reliable and improves user feedback. The reuse of the resolvedDocMapFile variable later avoids redundant work and keeps logic clear. The new subprocess tests follow the established TEST_SUBPROCESS_MAIN gating pattern and correctly set a synthetic GITHUB_WORKSPACE for isolation. With CI passing and no issues found in the diff, this PR is ready to merge.

Review by gpt

Evaluated against 290ce992

# Gpt Review ## Summary The changes cleanly move --doc-map path validation earlier to fail fast and add two robust subprocess tests covering path traversal and nonexistent files. Error handling, logging, and test structure are consistent and idiomatic, and CI has passed. ## Recommendation **APPROVE** — The early validation of the doc-map path before any network I/O is a sensible design improvement that makes the new tests reliable and improves user feedback. The reuse of the resolvedDocMapFile variable later avoids redundant work and keeps logic clear. The new subprocess tests follow the established TEST_SUBPROCESS_MAIN gating pattern and correctly set a synthetic GITHUB_WORKSPACE for isolation. With CI passing and no issues found in the diff, this PR is ready to merge. --- *Review by gpt*  --- *Evaluated against 290ce992*

rodin added the wip label 2026-05-15 12:01:07 +00:00

rodin added 1 commit 2026-05-15 12:07:18 +00:00

test(#146 ): clarify t.TempDir() evaluation in subprocess env setup

PR Ready Gate / clear-labels (pull_request) Successful in 2s

Details

CI / test (pull_request) Successful in 16s

Details

CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Failing after 17s

Details

CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Failing after 24s

Details

CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Failing after 27s

Details

430e61fdbd

rodin force-pushed issue-146 from 290ce99226 to 430e61fdbd

2026-05-15 12:07:18 +00:00

Compare

rodin merged commit b09f12b8ff into main

2026-05-15 12:07:28 +00:00

rodin referenced this pull request

2026-05-15 14:45:35 +00:00

Bug: dev-loop worker closed PR #156 autonomously #157

Sign in to join this conversation.

No Reviewers

sonnet-review-bot

security-review-bot

gpt-review-bot

4 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: rodin/review-bot#151

test(#146): add TestMainSubprocess_InvalidDocMapPath and TestMainSubprocess_InvalidDocMapFile #151

Summary

Changes

cmd/review-bot/main.go

cmd/review-bot/main_test.go

Sonnet Review

Summary

Findings

Recommendation

Security Review

Summary

Recommendation

Gpt Review

Summary

Findings

Recommendation

Self-Review: PR #151

Phase 1: Independent Findings

Phase 2: Prior Review Verification

Assessment: ✅ Clean

Fix Plan against 40a16b75e0:

Findings Summary

Plan

Execution

Sonnet Review

Summary

Findings

Recommendation

Gpt Review

Summary

Recommendation

Security Review

Summary

Recommendation

Self-Review: PR #151

Phase 1: Independent Findings

Phase 2: Prior Review Verification

Assessment: ✅ Clean

Self-Review: PR #151

Phase 1: Independent Findings

Phase 2: Prior Review Verification

Assessment: ✅ Clean

Fix Plan

Sonnet Review

Summary

Findings

Recommendation

Security Review

Summary

Recommendation

Gpt Review

Summary

Recommendation

Fix Plan against `40a16b75e0`: