feat(#143): fetch doc-map config from trusted VCS ref #153
Reference in New Issue
Block a user
Delete Branch "issue-143"
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?
Summary
Fixes #143.
Problem
The doc-map YAML config was read from the local workspace (the PR branch checkout). A malicious PR author could:
.review-bot/doc-map.ymlin their branch to map any path glob to sensitive design docsFix
Add
--doc-map-trusted-ref/DOC_MAP_TRUSTED_REFflag. When set:main)When unset (backward compat):
Recommended usage
Changes
review/docmap.goParseDocMapConfigContent(content, source string)— parse doc-map from fetched VCS contentparseDocMapBytes— shared parse logic extracted fromParseDocMapConfigcmd/review-bot/main.go--doc-map-trusted-ref/DOC_MAP_TRUSTED_REFflag added.gitea/actions/review/action.ymldoc-map-trusted-refinput added with security descriptionDOC_MAP_TRUSTED_REFenv var wired throughREADME.mddoc-map-trusted-refrow added to inputs tablereview/docmap_test.goTestParseDocMapConfigContent_ValidTestParseDocMapConfigContent_EmptyContentTestParseDocMapConfigContent_InvalidYAMLTestParseDocMapConfigContent_UnknownKeysTest Results
Checklist
ParseDocMapConfigContentaddedaction.ymlupdated with new inputgo vet ./...cleanscripts/check-deps.shcleanOriginal reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
a4f04897)Sonnet Review
Summary
This PR correctly implements a security fix for doc-map config injection by fetching the config from a trusted VCS ref instead of the PR branch. The implementation is clean, well-structured, backward compatible, and follows established patterns in the codebase. Tests are thorough and appropriate.
Findings
review/docmap.gocmd/review-bot/main.goRecommendation
APPROVE — Approve. The security fix is correctly implemented: the doc-map config is fetched from a trusted ref via VCS API when
--doc-map-trusted-refis set, a 404 is a hard error with no silent fallback, and the PR branch only contributes the list of changed files. The refactoring ofparseDocMapBytesas a shared internal function is clean and idiomatic. Error variables are correctly scoped to avoid shadowing the outererr. Backward compatibility is preserved with an appropriate security warning. Tests cover valid, empty, invalid YAML, and unknown-key cases. The two minor nits are truly cosmetic and do not warrant blocking the PR.Review by sonnet
Evaluated against
a4f04897@@ -101,6 +101,7 @@ func main() {aicoreResourceGroup := flag.String("aicore-resource-group", envOrDefault("AICORE_RESOURCE_GROUP", "default"), "SAP AI Core resource group (for provider=aicore)")docMapFile := flag.String("doc-map", envOrDefault("DOC_MAP_FILE", ""), "Path to YAML file mapping source path globs to governing design docs")docMapMaxBytes := flag.Int("doc-map-max-bytes", envOrDefaultInt("DOC_MAP_MAX_BYTES", review.DefaultDocMapMaxBytes), "Maximum bytes of injected doc content (default 102400)")docMapTrustedRef := flag.String("doc-map-trusted-ref", envOrDefault("DOC_MAP_TRUSTED_REF", ""), "Git ref (e.g. main) to fetch the doc-map config from via VCS API instead of local workspace. Recommended to prevent PR branch from controlling which docs are injected.")[NIT] The flag description for --doc-map-trusted-ref is a long inline string literal. Style-wise this is consistent with other flags in the file so no change needed, but it's worth noting that very long flag descriptions can be hard to read in --help output.
@@ -56,0 +60,4 @@// (e.g. "main:main@<ref>").//// Use this when the config content has been fetched from a trusted VCS ref// rather than read from the local workspace.[NIT] The doc comment for ParseDocMapConfigContent says 'e.g. "main:main@"' but the actual format used in main.go is 'owner/repo@ref:path'. Minor doc inconsistency, no functional impact.
Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
a4f04897)Gpt Review
Summary
Solid security-focused enhancement adding a trusted-ref fetch for doc-map, with clear fallback and logging. Implementation is clean, tests cover new parsing path, and CI passed.
Findings
review/docmap.goerr) instead of the relaxed parse error (err2). Returningerr2would provide a more accurate parse failure message for genuinely invalid YAML.review/docmap.gosourceparameter mentions a format like "main:main@", but the code constructsowner/repo@ref:path. Consider updating the example to match the actual format to avoid confusion.README.mddoc-map-trusted-refinput to the Action Inputs table, which is great. Consider also addingDOC_MAP_TRUSTED_REFto the README's Environment Variables mapping section for completeness and parity with other flags.Recommendation
APPROVE — The changes correctly prevent PRs from controlling which docs are injected by allowing the doc-map to be fetched from a trusted VCS ref and treating fetch/parse failures as hard errors. The fallback warns when using the PR workspace, which preserves backward compatibility. Tests cover the new ParseDocMapConfigContent path, and the action plus CLI wiring is consistent. Addressing the minor error-wrapping detail in review/docmap.go and tightening a comment/example would improve clarity, and updating the README environment variable table would enhance documentation. Otherwise, ship it.
Review by gpt
Evaluated against
a4f04897@@ -210,6 +210,7 @@ AI Core handles OAuth token management and deployment discovery automatically. M| `system-prompt-file` | No | `""` | Local file with additional system prompt instructions || `doc-map` | No | `""` | Path to a YAML file mapping source path globs to governing design docs || `doc-map-max-bytes` | No | `102400` | Maximum bytes of injected doc content from doc-map (default 100KB) || `doc-map-trusted-ref` | No | `""` | Git ref (e.g. `main`) to fetch the doc-map config from via VCS API instead of local workspace. **Recommended for security** — prevents a PR from modifying the doc-map config to inject arbitrary docs. |[NIT] You added the
doc-map-trusted-refinput to the Action Inputs table, which is great. Consider also addingDOC_MAP_TRUSTED_REFto the README's Environment Variables mapping section for completeness and parity with other flags.@@ -55,1 +57,4 @@// ParseDocMapConfigContent parses a doc-map YAML config from an in-memory// string. The source parameter is used only for error messages and log entries// (e.g. "main:main@<ref>").[NIT] The comment example for the
sourceparameter mentions a format like "main:main@", but the code constructsowner/repo@ref:path. Consider updating the example to match the actual format to avoid confusion.@@ -56,2 +70,4 @@// ParseDocMapConfig and ParseDocMapConfigContent.func parseDocMapBytes(data []byte, source string) (*DocMapConfig, error) {var cfg DocMapConfigif err := yaml.UnmarshalWithOptions(data, &cfg, yaml.Strict()); err != nil {[MINOR] When strict YAML unmarshal fails and relaxed unmarshal also fails, the returned error wraps the original strict-mode error (
err) instead of the relaxed parse error (err2). Returningerr2would provide a more accurate parse failure message for genuinely invalid YAML.Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
a4f04897)Security Review
Summary
Security-focused change correctly fetches the doc-map configuration from a trusted VCS ref, preventing PR-branch manipulation. The implementation is consistent with existing safeguards (path traversal checks, size caps) and handles VCS 404s as hard errors. One defense-in-depth improvement is recommended for validating the trusted ref input.
Findings
cmd/review-bot/main.goRecommendation
APPROVE — The changes materially improve security by preventing PR branches from controlling which design docs are injected, and they fail closed on VCS fetch errors. Parsing refactoring is sound, and doc path validation and size limiting remain in place. To further harden the solution, add input validation for doc-map-trusted-ref (e.g., allow only characters valid in Git refs, reject control characters and whitespace). This reduces the risk of malformed refs affecting API calls or causing log injection. With that minor improvement, the feature is robust; approval recommended.
Review by security
Evaluated against
a4f04897a4f04897b6to3222c765c9Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
3222c765)Sonnet Review
Summary
This PR correctly addresses the security vulnerability by adding a
--doc-map-trusted-refflag that fetches the doc-map config from a trusted VCS ref instead of the PR branch. The implementation is clean, well-tested, and backward compatible. CI passes.Findings
review/docmap.goParseDocMapConfigContentuses"main:main@<ref>"as the example source string, but the actual usage inmain.goformats it as"owner/repo@ref:path". The example is slightly misleading — minor documentation inconsistency.cmd/review-bot/main.go--doc-mappath now happens before client initialization (good for fail-fast), but when--doc-map-trusted-refis set, theresolvedDocMapFilevariable is never used (the trusted-ref path fetches from VCS, ignoring the local file). This is correct behavior but the variable nameresolvedDocMapFileimplies a local file will be used. A comment clarifying that this is only used in the local-workspace fallback path would improve readability.Recommendation
APPROVE — Approve. The security fix is correct and well-structured: the trusted-ref path is a hard error on 404 (no silent fallback), the local-workspace path emits a clear security warning, and the existing
ParseDocMapConfigis cleanly refactored into a sharedparseDocMapBytesfunction. The newParseDocMapConfigContentfunction follows the existing patterns. Tests cover all four new scenarios (valid, empty, invalid YAML, unknown keys). The two findings are cosmetic nits that don't affect correctness.Review by sonnet
Evaluated against
3222c765@@ -173,6 +174,17 @@ func main() {os.Exit(1)}// Early validation of filesystem-path flags (fail fast before network I/O)[NIT] The early validation of
--doc-mappath now happens before client initialization (good for fail-fast), but when--doc-map-trusted-refis set, theresolvedDocMapFilevariable is never used (the trusted-ref path fetches from VCS, ignoring the local file). This is correct behavior but the variable nameresolvedDocMapFileimplies a local file will be used. A comment clarifying that this is only used in the local-workspace fallback path would improve readability.@@ -56,0 +61,4 @@//// Use this when the config content has been fetched from a trusted VCS ref// rather than read from the local workspace.func ParseDocMapConfigContent(content, source string) (*DocMapConfig, error) {[NIT] The doc comment for
ParseDocMapConfigContentuses"main:main@<ref>"as the example source string, but the actual usage inmain.goformats it as"owner/repo@ref:path". The example is slightly misleading — minor documentation inconsistency.Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
3222c765)Security Review
Summary
Security-focused change correctly fetches the doc-map config from a trusted VCS ref, preventing PR-branch manipulation. Path traversal is guarded, limits are enforced for injected doc size, and CI is passing. I see no exploitable issues introduced by this PR.
Recommendation
APPROVE — The new --doc-map-trusted-ref flow hardens against PR-controlled config and correctly fails closed on fetch/parse errors without silently falling back. Local-path use is still validated to prevent traversal, and doc content is bounded by size limits. As a defense-in-depth enhancement (optional), consider validating the trusted ref string against a conservative git-ref pattern to prevent odd values, and monitor any directories referenced in doc-map for potential large expansions. Otherwise, this is a solid security improvement — proceed with merge.
Review by security
Evaluated against
3222c765Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
3222c765)Gpt Review
Summary
Solid security-focused enhancement: adds a trusted-ref mode for doc-map with proper API fetch and strict parse, updates action inputs, README, and tests. Overall code is idiomatic, error handling is consistent, and CI passed.
Findings
cmd/review-bot/main.goreview/docmap.goRecommendation
APPROVE — The changes correctly introduce a secure trusted-ref mechanism for fetching the doc-map, enforce strict YAML parsing with a lenient fallback and warning, and wire the new option through the composite action, README, and tests. Error handling and logging are consistent and idiomatic, and the CI passed. One improvement to consider: in trusted-ref mode, skip the local workspace path validation for the doc-map file to avoid unnecessary coupling to the PR branch state and let the VCS fetch be authoritative. The minor naming/logging nit in docmap.go is optional. With these small adjustments, the feature would be even more robust, but the current implementation remains acceptable to merge.
Review by gpt
Evaluated against
3222c765Review — Config B (Opus investigates, GPT-5 judges)
Summary
This is a well-structured security fix that addresses a real attack vector: a malicious PR author controlling which design docs get injected into the LLM prompt by modifying the doc-map config on their branch. The fix is clean — a new
--doc-map-trusted-refflag that fetches the config via VCS API from a trusted ref instead of reading from the local workspace.Verdict: APPROVED_WITH_FEEDBACK
The security logic is correct, the VCS integration is sound, tests are adequate, and backward compatibility is maintained. One bug worth addressing before merge:
🐛 Bug: Early
validateWorkspacePathfails when trusted-ref is set but file is absent locallyLocation:
cmd/review-bot/main.golines 178-185 (the early validation block)validateWorkspacePathcallsfilepath.EvalSymlinks(fullPath)which requires the file to exist on disk. When--doc-map-trusted-refis set, the intent is to fetch the config from VCS — the file may not exist in the local workspace (e.g., a fresh clone without the config, or a repo where the config only exists onmain).This means if you set
doc-map-trusted-ref: mainand the doc-map file doesn't exist locally, the program exits at this early validation before it ever reaches the VCS fetch code.Fix: Skip
validateWorkspacePathwhendocMapTrustedRefis non-empty. You only need local path validation for the local-workspace fallback path:Note: You may also want to validate the
*docMapFilepath for traversal attacks when used as the VCS path argument (e.g., ensure no../segments), but that's a simpler string check rather than requiring the file to exist locally.✅ What's Good
Security model is correct: The PR branch only contributes changed file paths; the config that decides which docs to inject comes from a trusted ref. A 404 from VCS is a hard error (no silent fallback), which prevents downgrade attacks.
Clean refactoring: Extracting
parseDocMapBytesas shared logic betweenParseDocMapConfig(file-based) andParseDocMapConfigContent(VCS-fetched) is the right approach. No duplication.Backward compatibility: Existing users without
--doc-map-trusted-refcontinue to work (with an appropriate security warning). The warning message is actionable — it tells operators exactly what flag to set.Both VCS backends covered: Both
giteaVCSAdapterandgithubVCSAdapteralready implementGetFileContentRef, so this works for both Gitea and GitHub repos.Tests cover the key paths:
ParseDocMapConfigContenttests (valid, empty, invalid YAML, unknown keys) and the subprocess tests for path traversal / nonexistent file.Documentation is complete: README, CHANGELOG, and action.yml all updated consistently.
Minor Observations
The
sourceformat stringfmt.Sprintf("%s/%s@%s:%s", owner, repoName, *docMapTrustedRef, *docMapFile)is a nice touch for error context — makes it immediately clear where the config came from in logs.The
err→loadErr/fetchErr/parseErrrename in the Step 6c block improves clarity by avoiding shadow confusion with the outererr.No integration test for the VCS fetch path exists (the subprocess tests only cover local path validation failures). This is acceptable since
GetFileContentRefis already tested in the gitea/github client test suites, but a future improvement could add a mock VCS server test.test
🤖 Multi-Model AI Review (Config B: Opus investigates, GPT-5 judges)
Overall Verdict: APPROVE ✅
Summary
This PR correctly addresses the prompt-injection vulnerability where a malicious PR author could modify
.review-bot/doc-map.ymlto map arbitrary paths to design docs, injecting those docs into the LLM prompt. The--doc-map-trusted-refflag fetches the config from a trusted VCS ref (e.g.,main) via API instead of from the PR branch checkout, closing the attack surface. The implementation is clean, backward-compatible, and well-tested.Security Assessment
✅ Core fix is correct. Fetching the doc-map config from a trusted ref via
vcs.GetFileContentRefprevents the PR branch from controlling which docs get injected.✅ Hard error on VCS 404. No silent fallback that could be exploited — a missing config at the trusted ref is a hard failure.
✅ Backward compatibility maintained. Unset
--doc-map-trusted-refpreserves prior behavior with a warning.✅ Path safety for local reads. The
validateWorkspacePathcall (withEvalSymlinks) guards against path traversal and symlink attacks in the local-workspace fallback path.⚠️ Usability edge (non-blocking): When
--doc-map-trusted-refis set,validateWorkspacePathstill runs and callsfilepath.EvalSymlinks, which requires the local file to exist. An operator who configures a trusted-ref workflow but does not have the doc-map file locally (it lives only on the trusted branch) will get a confusing crash before the VCS fetch is attempted. This is fail-closed (safe) but could block legitimate remote-only configurations. The recommended fix is to skipEvalSymlinksand use syntactic path validation only when--doc-map-trusted-refis set.Code Quality
✅ Clean refactor. Extracting
parseDocMapBytesas a shared helper is the right abstraction.ParseDocMapConfigContenthas a clean interface (string in,*DocMapConfigout, source string for error context).✅ Tests are correct. The four
ParseDocMapConfigContenttests cover the right cases. The subprocess tests for path validation exercise the actual binary behavior.Inline Comments
cmd/review-bot/main.go(~line 178, early validation block)When
--doc-map-trusted-refis set, the early validation block still callsvalidateWorkspacePathwhich callsfilepath.EvalSymlinks, requiring the file to exist locally. Consider splitting validation so only syntactic checks run when trusted-ref is set (the file may only exist at the trusted ref, not in the PR workspace):CHANGELOG.mdThe
doc-map-trusted-reffeature appears in both### Securityand### Addedsections, creating a duplicate entry. The Security entry is the right primary location — consider removing or merging the Added entry to avoid redundancy.Pipeline Metadata
Multi-Model AI Review — PR #153 (Config B: Opus investigates, GPT-5 judges)
Pipeline: Stage 1 breadth scan → Stage 2 deep dive (Opus) → Stage 3 cross-validation (GPT-5) → Stage 4 synthesis
Summary
This PR correctly addresses a real security vulnerability: a malicious PR author could modify
.review-bot/doc-map.ymlon their branch to inject arbitrary design docs into the LLM prompt. The--doc-map-trusted-refflag fetches the config from a trusted VCS ref instead. The design is sound and the backward-compatibility story is clean. Tests pass.Verdict: REQUEST_CHANGES (1 minor, 2 NITs)
Findings
🟡 MINOR: Early workspace path validation runs even when
--doc-map-trusted-refis setFile:
cmd/review-bot/main.go:178-186validateWorkspacePathcallsfilepath.EvalSymlinks, which requires the file to exist in the local workspace. When--doc-map-trusted-refis set,resolvedDocMapFileis never used — the code fetches the config via VCS API at line 381. But the early-validation still runs and will exit if the file does not exist on disk.The security model says the PR branch should not control the config, but the tool currently requires the config file to exist in the PR branch checkout anyway (just to pass path validation before ignoring it). If a repo uses the trusted-ref feature and the config only lives on
main(not on the PR branch), the tool will fail with"failed to resolve doc-map: ..."before ever attempting the VCS fetch.Suggested fix: Skip early workspace validation when
--doc-map-trusted-refis set:🔵 NIT: No integration test for the
--doc-map-trusted-refsubprocess pathFile:
cmd/review-bot/main_test.goThe new subprocess tests cover local-workspace scenarios (path traversal, nonexistent file). The trusted-ref branch has no equivalent integration test. The PR description explicitly calls out "VCS 404 is a hard error" as a security property — but this is untested at the integration level. A
TestMainSubprocess_DocMapTrustedRefVCS404(with a mock VCS server returning 404) would close this gap.🔵 NIT: Docstring example in
ParseDocMapConfigContentis incorrectFile:
review/docmap.go:63The example
"main:main@<ref>"does not match the actual format produced bymain.go:392:Update the example to
"owner/repo@<ref>:<path>".What Looks Good
parseDocMapBytesrefactoring is clean; both parse functions share the YAML logic correctly.err → parseErr,loadErr) in Step 6c avoids pre-existing variable shadowing.ParseDocMapConfigContentcover all four cases: valid, empty, invalid YAML, unknown keys.action.yml, README, and CHANGELOG are all correctly updated with security guidance.Review generated by multi-model pipeline (Config B: Opus investigates, GPT-5 judges) — 2026-05-15
🤖 AI Review — PR #153 (Config B: Opus investigates, GPT-5 judges)
Summary
Security fix for #143 is correctly implemented. The core attack vector (malicious PR modifying doc-map config to inject arbitrary docs into LLM prompts) is fully closed when
--doc-map-trusted-refis set. One logic bug found that undermines the feature's usability in CI.Findings
🔴 MAJOR-1: Early
validateWorkspacePathblocks trusted-ref when file doesn't exist locallyFile:
cmd/review-bot/main.go(lines 176-185)Problem:
validateWorkspacePathcallsfilepath.EvalSymlinkswhich requires the file to exist on disk. When--doc-map-trusted-refis set, the--doc-mapvalue is used as a VCS API path parameter — the local file is never read. If the doc-map doesn't exist in the local workspace (sparse checkout, file deleted on PR branch but exists onmain, or operators who rely entirely on VCS fetch), the tool exits before reaching the VCS API call.Fix:
Impact: Breaks the trusted-ref feature in environments without full PR branch checkout. The whole point of trusted-ref is to decouple from the local workspace.
🟡 MINOR-1: Godoc example format doesn't match actual usage
File:
review/docmap.goline ~60The
ParseDocMapConfigContentgodoc says(e.g. "main:main@<ref>")but actual usage produces formatowner/repo@ref:path. Fix the doc example to match.🟡 MINOR-2: No integration/subprocess test for the trusted-ref VCS fetch path
Current tests only validate local-fallback error paths (path traversal, nonexistent file). There's no test confirming the happy path when
--doc-map-trusted-refis set. Given this is the security-critical path, a subprocess test would add confidence. (Unit tests forParseDocMapConfigContentexist and are good.)Security Assessment
DocFetcher.GetFileContent)Code Quality
parseDocMapBytesrefactor — idiomatic shared implementationfetchErr,parseErr,loadErr) avoiding shadow bugsVerdict
NEEDS_WORK — Fix MAJOR-1 (conditional early validation). The security logic is sound but the early-validation placement creates a correctness bug that defeats the feature's purpose in legitimate CI scenarios. After that fix, this is ready to ship.
Self-review against
3222c765c9Assessment: ✅ Clean
Security: 🟢
The attack vector is well understood and properly fixed. Fetching the doc-map config from a trusted VCS ref via API means a PR branch cannot influence which design docs are injected into the LLM prompt. VCS 404 is a hard fail (
os.Exit(1)) with no silent fallback — this is exactly right, since a silent fallback to the local copy would defeat the whole fix. The security warning on the local-workspace path is prominent and actionable.Code Quality: 🟢
parseDocMapBytescleanly eliminates duplication between the local-file and in-memory parse paths.sourceparameter format (owner/repo@ref:path) is informative for error messages.resolvedDocMapFileresolution (from #146) correctly removes the redundantvalidateWorkspacePathcall in Step 6c.err→parseErr/loadErr) is good hygiene.Test Coverage: 🟡
ParseDocMapConfigContentis well covered: Valid, Empty, InvalidYAML, UnknownKeys — thorough at the unit layer.--doc-map-trusted-refset. Not a correctness blocker given the logic is a simple branch and the mock supports it, but worth a follow-up issue.Verdict
Implementation is correct and the security fix is sound. Ready for merge. The coverage gap is minor and can be addressed in a follow-up.
Self-review against
3222c765c9Assessment: ⚠️ Needs attention
Self-Review: pr-153 — 2026-05-15
Issues Found
40a16b7(test),9b64c60(fix),3222c76(feat)Code Quality (Pre-Rebase Assessment)
The feature implementation (PR #143 core: fetch doc-map from trusted VCS ref) appears sound:
Verdict After Rebase
Once rebased, this PR should be ready for merge. The self-review assessment is currently blocked on the merge conflict resolution.
Self-review against
2e65a10c4aAssessment: ✅ Clean
No issues found — ready for human review.
Scope: fix(#143): skip local doc-map validation when --doc-map-trusted-ref is set
All tests pass.
Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
2e65a10c)Sonnet Review
Summary
This PR correctly implements the security fix for doc-map config injection by adding a
--doc-map-trusted-refflag that fetches the config from a trusted VCS ref instead of the PR branch. The implementation is clean, well-tested, and follows established Go patterns throughout the codebase. CI passes.Findings
review/docmap.goParseDocMapConfigContentfunction converts astringto[]bytewith[]byte(content)before passing toparseDocMapBytes. This is a trivial allocation but the function signature could alternatively accept[]bytedirectly. This is a minor style point — the current string signature is ergonomic for callers that have a string fromGetFileContentRef, so this is acceptable as-is.cmd/review-bot/main.goresolvedDocMapFilevariable is declared at a wide scope and left as the zero value (empty string) when*docMapTrustedRef != "". It is then used later in theelsebranch of the trusted-ref check. This is correct — the empty string is only used in the else branch — but it is slightly fragile. A future refactor could accidentally useresolvedDocMapFilein the trusted-ref path. A comment noting thatresolvedDocMapFileis intentionally empty whendocMapTrustedRefis set would improve clarity, or the variable could be moved into theelseblock. No bug here.Recommendation
APPROVE — APPROVE. The security fix is correctly implemented: the doc-map config is now fetched from a trusted VCS ref when
--doc-map-trusted-refis set, a 404 from VCS is a hard error with no fallback, and the PR branch cannot influence which docs are injected. The refactoring ofparseDocMapBytesas shared logic is clean and idiomatic. Tests cover all four new cases (_Valid,_EmptyContent,_InvalidYAML,_UnknownKeys) plus the end-to-end subprocess tests for the new flag behavior. The two NITs are cosmetic and do not warrant blocking the PR.Review by sonnet
Evaluated against
2e65a10c@@ -173,6 +174,20 @@ func main() {os.Exit(1)[NIT] The
resolvedDocMapFilevariable is declared at a wide scope and left as the zero value (empty string) when*docMapTrustedRef != "". It is then used later in theelsebranch of the trusted-ref check. This is correct — the empty string is only used in the else branch — but it is slightly fragile. A future refactor could accidentally useresolvedDocMapFilein the trusted-ref path. A comment noting thatresolvedDocMapFileis intentionally empty whendocMapTrustedRefis set would improve clarity, or the variable could be moved into theelseblock. No bug here.@@ -56,0 +65,4 @@data := []byte(content)return parseDocMapBytes(data, source)}[NIT] The
ParseDocMapConfigContentfunction converts astringto[]bytewith[]byte(content)before passing toparseDocMapBytes. This is a trivial allocation but the function signature could alternatively accept[]bytedirectly. This is a minor style point — the current string signature is ergonomic for callers that have a string fromGetFileContentRef, so this is acceptable as-is.Security Review
Summary
The changes introduce a secure mechanism to fetch the doc-map configuration from a trusted VCS ref and fail closed on errors, significantly reducing prompt-injection and exfiltration risk. No exploitable vulnerabilities or unsafe patterns were observed in the diff.
Recommendation
APPROVE — Approve this PR. The implementation correctly fetches doc-map configuration from a trusted ref via the VCS API, avoids silent fallbacks, and maintains existing path traversal defenses for doc paths. Error handling fails closed, logging avoids sensitive data exposure, and CI has passed. Consider, as a defense-in-depth enhancement, validating the doc-map file path string (e.g., reject '..' and backslashes) even when using trusted ref, though current usage is generally safe within typical VCS content APIs.
Review by security
Evaluated against
2e65a10cGpt Review
Summary
Well-scoped security enhancement: the doc-map config can now be sourced from a trusted VCS ref with clear fail-fast behavior on 404 and a safe fallback with warnings. Code is idiomatic, respects repo conventions, adds solid tests, and updates the composite action and docs appropriately.
Findings
cmd/review-bot/main.goREADME.mdRecommendation
APPROVE — The changes correctly implement fetching the doc-map from a trusted VCS ref, enforce a hard error on missing config, and preserve backward compatibility with a clear security warning when reading from the local workspace. Error handling is clean (no panics), logging is informative, and additional unit tests cover both parsing the in-memory YAML and CLI behavior around path validation and trusted ref usage. The composite action wiring and README/CHANGELOG updates look good. Aside from minor logging clarity for the case when the PR file list is unavailable and a small docs table omission, the implementation is solid. Approve as-is; optionally address the minor documentation and logging improvements in a follow-up.
Review by gpt
Evaluated against
2e65a10c2e65a10c4ato02dfc12141Self-review against
02dfc12141Assessment: ✅ Clean
No issues found — ready for human review.
Sonnet Review
Summary
This PR correctly addresses a security vulnerability where a malicious PR branch could modify the doc-map config to inject arbitrary design docs into the LLM prompt. The implementation is clean and idiomatic Go: the refactoring extracts shared parse logic into
parseDocMapBytes, adds a new exportedParseDocMapConfigContentfunction, wires the trusted-ref flag through the CLI, and provides solid test coverage. CI passes.Findings
review/docmap.godatainParseDocMapConfigContentis assigned from[]byte(content)and then immediately passed toparseDocMapBytes. This is correct and idiomatic, but the intermediate variabledatacould be inlined asparseDocMapBytes([]byte(content), source)to reduce noise. Minor style only — current code is perfectly fine.cmd/review-bot/main.go--doc-map-trusted-refis quite long for a flag help string. Consider shortening it to fit within a terminal line. This is cosmetic only and has no functional impact.Recommendation
APPROVE — Approve. The security fix is correctly implemented: the PR branch is prevented from controlling which docs are injected when
--doc-map-trusted-refis set, a 404 from VCS is a hard error (no silent fallback to local), and the local-workspace fallback emits a security warning. The refactoring ofparseDocMapBytesas shared logic is clean and follows the extract-shared-implementation pattern. Test coverage is thorough, including the subprocess test that verifies local validation is skipped when the trusted-ref flag is present. The two findings are purely cosmetic nits that do not warrant changes.Review by sonnet
Evaluated against
02dfc121@@ -101,6 +101,7 @@ func main() {aicoreResourceGroup := flag.String("aicore-resource-group", envOrDefault("AICORE_RESOURCE_GROUP", "default"), "SAP AI Core resource group (for provider=aicore)")docMapFile := flag.String("doc-map", envOrDefault("DOC_MAP_FILE", ""), "Path to YAML file mapping source path globs to governing design docs")docMapMaxBytes := flag.Int("doc-map-max-bytes", envOrDefaultInt("DOC_MAP_MAX_BYTES", review.DefaultDocMapMaxBytes), "Maximum bytes of injected doc content (default 102400)")docMapTrustedRef := flag.String("doc-map-trusted-ref", envOrDefault("DOC_MAP_TRUSTED_REF", ""), "Git ref (e.g. main) to fetch the doc-map config from via VCS API instead of local workspace. Recommended to prevent PR branch from controlling which docs are injected.")[NIT] The flag description string for
--doc-map-trusted-refis quite long for a flag help string. Consider shortening it to fit within a terminal line. This is cosmetic only and has no functional impact.@@ -56,3 +71,4 @@func parseDocMapBytes(data []byte, source string) (*DocMapConfig, error) {var cfg DocMapConfigif err := yaml.UnmarshalWithOptions(data, &cfg, yaml.Strict()); err != nil {// Re-parse without strict mode to log which keys are unknown.[NIT] The variable
datainParseDocMapConfigContentis assigned from[]byte(content)and then immediately passed toparseDocMapBytes. This is correct and idiomatic, but the intermediate variabledatacould be inlined asparseDocMapBytes([]byte(content), source)to reduce noise. Minor style only — current code is perfectly fine.