PR #140: missing TestMainSubprocess_InvalidDocMapPath and TestMainSubprocess_InvalidDocMapFile tests #146
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.