Issue #139 required two subprocess tests covering --doc-map path validation in main(): one for path traversal (../../../etc/passwd) and one for a nonexistent file. PR #140 added 7 tests but did not implement either of these two acceptance criteria. The --doc-map flag validation logic in main() remains untested.
Source
PR: #140 — test(#139): improve cmd/review-bot coverage from 44.6% to 49.3%
TestMainSubprocess_InvalidDocMapPath — --doc-map ../../../etc/passwd fails with path traversal error
TestMainSubprocess_InvalidDocMapFile — --doc-map nonexistent.yml fails with workspace resolve error
What needs to happen
Add TestMainSubprocess_InvalidDocMapPath: subprocess test that passes --doc-map ../../../etc/passwd and asserts non-zero exit with an error message referencing path traversal.
Add TestMainSubprocess_InvalidDocMapFile: subprocess test that passes --doc-map nonexistent.yml and asserts non-zero exit with an error message referencing workspace resolve or nonexistent file.
Both tests must follow the existing TestMainSubprocess_* pattern (env gate TEST_SUBPROCESS_MAIN=1).
## What was missed
Issue #139 required two subprocess tests covering `--doc-map` path validation in `main()`: one for path traversal (`../../../etc/passwd`) and one for a nonexistent file. PR #140 added 7 tests but did not implement either of these two acceptance criteria. The `--doc-map` flag validation logic in `main()` remains untested.
## Source
- PR: #140 — test(#139): improve cmd/review-bot coverage from 44.6% to 49.3%
- Linked issue: #139 — test: improve cmd/review-bot coverage
- Acceptance criteria missed:
- `TestMainSubprocess_InvalidDocMapPath` — `--doc-map ../../../etc/passwd` fails with path traversal error
- `TestMainSubprocess_InvalidDocMapFile` — `--doc-map nonexistent.yml` fails with workspace resolve error
## What needs to happen
- Add `TestMainSubprocess_InvalidDocMapPath`: subprocess test that passes `--doc-map ../../../etc/passwd` and asserts non-zero exit with an error message referencing path traversal.
- Add `TestMainSubprocess_InvalidDocMapFile`: subprocess test that passes `--doc-map nonexistent.yml` and asserts non-zero exit with an error message referencing workspace resolve or nonexistent file.
- Both tests must follow the existing `TestMainSubprocess_*` pattern (env gate `TEST_SUBPROCESS_MAIN=1`).
- All existing tests must continue to pass.
## References
- [PR #140](https://gitea.weiker.me/rodin/review-bot/pulls/140)
- [Issue #139](https://gitea.weiker.me/rodin/review-bot/issues/139)
The --doc-map flag path validation in main() is unreachable by existing subprocess tests because it occurs at step 6c — after PR fetch from the server. We need two subprocess tests that exercise:
Path traversal rejection (../../../etc/passwd)
Nonexistent file rejection (nonexistent.yml)
Constraints
Must follow existing TestMainSubprocess_* pattern (env gate TEST_SUBPROCESS_MAIN=1)
All existing tests must continue to pass
Cannot depend on a running Gitea/GitHub server
Proposed Approach
Move --doc-map path validation earlier in main() — right after the reviewer-name/repo/PR-number validations (around line 170) and before any network client initialization. This is the same pattern used for other path-based flags like --persona-file validation location.
Specifically:
Add an early validation block after flag.Parse() and the existing flag checks that calls validateWorkspacePath(*docMapFile, "doc-map") when *docMapFile != "". This exits with a descriptive error before any network I/O.
Keep the later step 6c logic (which loads/parses the file), but the path validation will have already passed by then, so the early validateWorkspacePath call catches bad paths.
Add TestMainSubprocess_InvalidDocMapPath — passes --doc-map ../../../etc/passwd with GITHUB_WORKSPACE set to a temp dir, asserts non-zero exit and error output containing "resolves outside workspace" or "doc-map".
Add TestMainSubprocess_InvalidDocMapFile — passes --doc-map nonexistent.yml with GITHUB_WORKSPACE set to a temp dir, asserts non-zero exit and error output referencing "failed to resolve" or "doc-map".
Error Cases
Path traversal (../../../etc/passwd): filepath.Rel detects the relative path starts with .. → exits with "doc-map resolves outside workspace"
Nonexistent file (nonexistent.yml): filepath.EvalSymlinks fails with ENOENT → exits with "failed to resolve doc-map"
Edge Cases
GITHUB_WORKSPACE unset: falls back to os.Getwd() — tests set it explicitly via env to control behavior
Both tests use the subprocess pattern: re-invoke the test binary with TEST_SUBPROCESS_MAIN=1
Pass GITHUB_WORKSPACE pointing to os.TempDir() (which exists but won't contain traversal targets or the nonexistent file)
Use cleanEnv() to strip interfering env vars, then add GITHUB_WORKSPACE explicitly
Assert non-zero exit code and expected error substring in combined output
Completion Checklist
Early validation block added for --doc-map before network calls?
TestMainSubprocess_InvalidDocMapPath passes --doc-map ../../../etc/passwd and asserts path traversal error?
TestMainSubprocess_InvalidDocMapFile passes --doc-map nonexistent.yml and asserts resolve error?
Both tests use the TEST_SUBPROCESS_MAIN=1 env gate pattern?
Both tests use cleanEnv() + explicit GITHUB_WORKSPACE?
All existing tests still pass (go test ./...)?
No changes to validateWorkspacePath function itself (already correct)?
Open Questions
The early validation for doc-map will call validateWorkspacePath which calls EvalSymlinks. For the nonexistent case, this catches it at the early check. The later step 6c ParseDocMapConfig call would also fail, but it's now unreachable for bad paths. This duplication is intentional — fail fast. Acceptable?
Should the early validation error use fmt.Fprintf(os.Stderr, ...) (like missing-flags) or slog.Error(...) (like invalid-repo)? The existing step 6c uses slog.Error. I'll match that for consistency.
## Plan
### Problem
The `--doc-map` flag path validation in `main()` is unreachable by existing subprocess tests because it occurs at step 6c — after PR fetch from the server. We need two subprocess tests that exercise:
1. Path traversal rejection (`../../../etc/passwd`)
2. Nonexistent file rejection (`nonexistent.yml`)
### Constraints
- Must follow existing `TestMainSubprocess_*` pattern (env gate `TEST_SUBPROCESS_MAIN=1`)
- All existing tests must continue to pass
- Cannot depend on a running Gitea/GitHub server
### Proposed Approach
**Move `--doc-map` path validation earlier in `main()`** — right after the reviewer-name/repo/PR-number validations (around line 170) and before any network client initialization. This is the same pattern used for other path-based flags like `--persona-file` validation location.
Specifically:
1. Add an early validation block after `flag.Parse()` and the existing flag checks that calls `validateWorkspacePath(*docMapFile, "doc-map")` when `*docMapFile != ""`. This exits with a descriptive error before any network I/O.
2. Keep the later step 6c logic (which loads/parses the file), but the path validation will have already passed by then, so the early `validateWorkspacePath` call catches bad paths.
3. Add `TestMainSubprocess_InvalidDocMapPath` — passes `--doc-map ../../../etc/passwd` with `GITHUB_WORKSPACE` set to a temp dir, asserts non-zero exit and error output containing "resolves outside workspace" or "doc-map".
4. Add `TestMainSubprocess_InvalidDocMapFile` — passes `--doc-map nonexistent.yml` with `GITHUB_WORKSPACE` set to a temp dir, asserts non-zero exit and error output referencing "failed to resolve" or "doc-map".
### Error Cases
- Path traversal (`../../../etc/passwd`): `filepath.Rel` detects the relative path starts with `..` → exits with "doc-map resolves outside workspace"
- Nonexistent file (`nonexistent.yml`): `filepath.EvalSymlinks` fails with ENOENT → exits with "failed to resolve doc-map"
### Edge Cases
- `GITHUB_WORKSPACE` unset: falls back to `os.Getwd()` — tests set it explicitly via env to control behavior
- `--doc-map` empty (default): validation block skipped entirely — no behavioral change
### Testing Strategy
- Both tests use the subprocess pattern: re-invoke the test binary with `TEST_SUBPROCESS_MAIN=1`
- Pass `GITHUB_WORKSPACE` pointing to `os.TempDir()` (which exists but won't contain traversal targets or the nonexistent file)
- Use `cleanEnv()` to strip interfering env vars, then add `GITHUB_WORKSPACE` explicitly
- Assert non-zero exit code and expected error substring in combined output
### Completion Checklist
1. Early validation block added for `--doc-map` before network calls?
2. `TestMainSubprocess_InvalidDocMapPath` passes `--doc-map ../../../etc/passwd` and asserts path traversal error?
3. `TestMainSubprocess_InvalidDocMapFile` passes `--doc-map nonexistent.yml` and asserts resolve error?
4. Both tests use the `TEST_SUBPROCESS_MAIN=1` env gate pattern?
5. Both tests use `cleanEnv()` + explicit `GITHUB_WORKSPACE`?
6. All existing tests still pass (`go test ./...`)?
7. No changes to `validateWorkspacePath` function itself (already correct)?
### Open Questions
- The early validation for doc-map will call `validateWorkspacePath` which calls `EvalSymlinks`. For the nonexistent case, this catches it at the early check. The later step 6c `ParseDocMapConfig` call would also fail, but it's now unreachable for bad paths. This duplication is intentional — fail fast. Acceptable?
- Should the early validation error use `fmt.Fprintf(os.Stderr, ...)` (like missing-flags) or `slog.Error(...)` (like invalid-repo)? The existing step 6c uses `slog.Error`. I'll match that for consistency.
TestMainSubprocess_InvalidDocMapPath added to cmd/review-bot/main_test.go (tests path traversal via validate-docmap subcommand)
TestMainSubprocess_InvalidDocMapFile added to cmd/review-bot/main_test.go (tests nonexistent file via validate-docmap subcommand)
Note: both tests exercise the validate-docmap subcommand which uses validateDocmapPath — the same path validation logic. Testing via the main review flow would require a live VCS server as --doc-map is processed at step 6c, after all VCS/PR setup.
Resolved by PR #152.
- `TestMainSubprocess_InvalidDocMapPath` added to `cmd/review-bot/main_test.go` (tests path traversal via `validate-docmap` subcommand)
- `TestMainSubprocess_InvalidDocMapFile` added to `cmd/review-bot/main_test.go` (tests nonexistent file via `validate-docmap` subcommand)
Note: both tests exercise the `validate-docmap` subcommand which uses `validateDocmapPath` — the same path validation logic. Testing via the main review flow would require a live VCS server as `--doc-map` is processed at step 6c, after all VCS/PR setup.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
What was missed
Issue #139 required two subprocess tests covering
--doc-mappath validation inmain(): one for path traversal (../../../etc/passwd) and one for a nonexistent file. PR #140 added 7 tests but did not implement either of these two acceptance criteria. The--doc-mapflag validation logic inmain()remains untested.Source
TestMainSubprocess_InvalidDocMapPath—--doc-map ../../../etc/passwdfails with path traversal errorTestMainSubprocess_InvalidDocMapFile—--doc-map nonexistent.ymlfails with workspace resolve errorWhat needs to happen
TestMainSubprocess_InvalidDocMapPath: subprocess test that passes--doc-map ../../../etc/passwdand asserts non-zero exit with an error message referencing path traversal.TestMainSubprocess_InvalidDocMapFile: subprocess test that passes--doc-map nonexistent.ymland asserts non-zero exit with an error message referencing workspace resolve or nonexistent file.TestMainSubprocess_*pattern (env gateTEST_SUBPROCESS_MAIN=1).References
Plan
Problem
The
--doc-mapflag path validation inmain()is unreachable by existing subprocess tests because it occurs at step 6c — after PR fetch from the server. We need two subprocess tests that exercise:../../../etc/passwd)nonexistent.yml)Constraints
TestMainSubprocess_*pattern (env gateTEST_SUBPROCESS_MAIN=1)Proposed Approach
Move
--doc-mappath validation earlier inmain()— right after the reviewer-name/repo/PR-number validations (around line 170) and before any network client initialization. This is the same pattern used for other path-based flags like--persona-filevalidation location.Specifically:
flag.Parse()and the existing flag checks that callsvalidateWorkspacePath(*docMapFile, "doc-map")when*docMapFile != "". This exits with a descriptive error before any network I/O.validateWorkspacePathcall catches bad paths.TestMainSubprocess_InvalidDocMapPath— passes--doc-map ../../../etc/passwdwithGITHUB_WORKSPACEset to a temp dir, asserts non-zero exit and error output containing "resolves outside workspace" or "doc-map".TestMainSubprocess_InvalidDocMapFile— passes--doc-map nonexistent.ymlwithGITHUB_WORKSPACEset to a temp dir, asserts non-zero exit and error output referencing "failed to resolve" or "doc-map".Error Cases
../../../etc/passwd):filepath.Reldetects the relative path starts with..→ exits with "doc-map resolves outside workspace"nonexistent.yml):filepath.EvalSymlinksfails with ENOENT → exits with "failed to resolve doc-map"Edge Cases
GITHUB_WORKSPACEunset: falls back toos.Getwd()— tests set it explicitly via env to control behavior--doc-mapempty (default): validation block skipped entirely — no behavioral changeTesting Strategy
TEST_SUBPROCESS_MAIN=1GITHUB_WORKSPACEpointing toos.TempDir()(which exists but won't contain traversal targets or the nonexistent file)cleanEnv()to strip interfering env vars, then addGITHUB_WORKSPACEexplicitlyCompletion Checklist
--doc-mapbefore network calls?TestMainSubprocess_InvalidDocMapPathpasses--doc-map ../../../etc/passwdand asserts path traversal error?TestMainSubprocess_InvalidDocMapFilepasses--doc-map nonexistent.ymland asserts resolve error?TEST_SUBPROCESS_MAIN=1env gate pattern?cleanEnv()+ explicitGITHUB_WORKSPACE?go test ./...)?validateWorkspacePathfunction itself (already correct)?Open Questions
validateWorkspacePathwhich callsEvalSymlinks. For the nonexistent case, this catches it at the early check. The later step 6cParseDocMapConfigcall would also fail, but it's now unreachable for bad paths. This duplication is intentional — fail fast. Acceptable?fmt.Fprintf(os.Stderr, ...)(like missing-flags) orslog.Error(...)(like invalid-repo)? The existing step 6c usesslog.Error. I'll match that for consistency.rodin referenced this issue2026-05-15 08:30:02 +00:00
Resolved by PR #152.
TestMainSubprocess_InvalidDocMapPathadded tocmd/review-bot/main_test.go(tests path traversal viavalidate-docmapsubcommand)TestMainSubprocess_InvalidDocMapFileadded tocmd/review-bot/main_test.go(tests nonexistent file viavalidate-docmapsubcommand)Note: both tests exercise the
validate-docmapsubcommand which usesvalidateDocmapPath— the same path validation logic. Testing via the main review flow would require a live VCS server as--doc-mapis processed at step 6c, after all VCS/PR setup.