## Summary
Change the default for `patterns-files` from `"README.md"` to empty string.
When `patterns-files` is empty, fetch all files from the patterns repo root instead of only fetching README.md.
This makes the common case work: users who set `patterns-repo` expect to get all pattern files, not just the README.
## Changes
- Default `patterns-files` to `""` instead of `"README.md"`
- When `patternsFiles` is empty, treat it as "fetch all from root" by passing empty path to `GetAllFilesInPath`
- Updated flag help text to clarify the new default behavior
- Added `TestBuildPatternPaths` to verify path-building logic
## Testing
- All existing tests pass
- New test verifies:
- Empty input returns `[""]` (repo root)
- Comma-separated paths are split correctly
- Whitespace is trimmed
- Empty entries between commas are skipped
Fixes #71
rodin
self-assigned this 2026-05-11 16:24:55 +00:00
Change the default for patterns-files from 'README.md' to empty string.
When patterns-files is empty, fetch all files from the patterns repo
root instead of only fetching README.md.
This makes the common case work: users who set patterns-repo expect to
get all pattern files, not just the README.
Fixes#71
Clean, focused change that fixes the default for patterns-files to empty string and adds proper path-building logic with whitespace trimming and empty-entry skipping. CI passes, tests cover the new behavior.
Findings
#
Severity
File
Line
Finding
1
[MINOR]
cmd/review-bot/main_test.go
507
TestBuildPatternPaths duplicates the path-building logic from fetchPatterns as a local closure rather than testing fetchPatterns directly. If the production logic in fetchPatterns changes, this test won't catch it. The test is testing a copy of the logic, not the function itself. Consider extracting buildPatternPaths as a package-level function and testing that instead, or writing an integration-style test that exercises fetchPatterns with a mock client.
Recommendation
APPROVE — The change is correct and well-implemented. The default change from "README.md" to "" resolves the described issue, the empty-path sentinel [""] cleanly represents "fetch from root" without a separate boolean, and whitespace trimming plus empty-entry skipping are defensive improvements over the old strings.Split behavior. CI passes. The one minor concern is that the test duplicates production logic rather than testing through the actual function, but this is not a blocker. Approve.
# Sonnet Review
## Summary
Clean, focused change that fixes the default for `patterns-files` to empty string and adds proper path-building logic with whitespace trimming and empty-entry skipping. CI passes, tests cover the new behavior.
## Findings
| # | Severity | File | Line | Finding |
|---|----------|------|------|--------|
| 1 | [MINOR] | `cmd/review-bot/main_test.go` | 507 | TestBuildPatternPaths duplicates the path-building logic from fetchPatterns as a local closure rather than testing fetchPatterns directly. If the production logic in fetchPatterns changes, this test won't catch it. The test is testing a copy of the logic, not the function itself. Consider extracting `buildPatternPaths` as a package-level function and testing that instead, or writing an integration-style test that exercises fetchPatterns with a mock client. |
## Recommendation
**APPROVE** — The change is correct and well-implemented. The default change from `"README.md"` to `""` resolves the described issue, the empty-path sentinel `[""]` cleanly represents "fetch from root" without a separate boolean, and whitespace trimming plus empty-entry skipping are defensive improvements over the old `strings.Split` behavior. CI passes. The one minor concern is that the test duplicates production logic rather than testing through the actual function, but this is not a blocker. Approve.
---
*Review by sonnet*
<!-- review-bot:sonnet -->
---
*Evaluated against 8b8168dc*
[MINOR] TestBuildPatternPaths duplicates the path-building logic from fetchPatterns as a local closure rather than testing fetchPatterns directly. If the production logic in fetchPatterns changes, this test won't catch it. The test is testing a copy of the logic, not the function itself. Consider extracting buildPatternPaths as a package-level function and testing that instead, or writing an integration-style test that exercises fetchPatterns with a mock client.
**[MINOR]** TestBuildPatternPaths duplicates the path-building logic from fetchPatterns as a local closure rather than testing fetchPatterns directly. If the production logic in fetchPatterns changes, this test won't catch it. The test is testing a copy of the logic, not the function itself. Consider extracting `buildPatternPaths` as a package-level function and testing that instead, or writing an integration-style test that exercises fetchPatterns with a mock client.
security-review-bot
requested review from security-review-bot 2026-05-11 16:26:14 +00:00
Changes adjust the default behavior for pattern fetching to include all files from the repo root when patterns-files is empty and add corresponding tests. No exploitable security vulnerabilities are introduced by these modifications.
Recommendation
APPROVE — The updated default and path-building logic are reasonable and do not introduce security issues in themselves. Ensure operationally that the configured patterns repository is curated and does not contain sensitive information (e.g., secrets in YAML files), since all included pattern files are sent to the LLM provider. Otherwise, the implementation and tests look good and CI has passed.
# Security Review
## Summary
Changes adjust the default behavior for pattern fetching to include all files from the repo root when patterns-files is empty and add corresponding tests. No exploitable security vulnerabilities are introduced by these modifications.
## Recommendation
**APPROVE** — The updated default and path-building logic are reasonable and do not introduce security issues in themselves. Ensure operationally that the configured patterns repository is curated and does not contain sensitive information (e.g., secrets in YAML files), since all included pattern files are sent to the LLM provider. Otherwise, the implementation and tests look good and CI has passed.
---
*Review by security*
<!-- review-bot:security -->
---
*Evaluated against 8b8168dc*
gpt-review-bot
approved these changes 2026-05-11 16:26:38 +00:00
Change correctly defaults patterns-files to empty and updates fetch logic to treat empty as repo root. Implementation is straightforward, idiomatic, and covered by a targeted unit test; CI passed.
Findings
#
Severity
File
Line
Finding
1
[MINOR]
cmd/review-bot/main_test.go
504
TestBuildPatternPaths re-implements the path parsing logic inline. Consider extracting the path-building into a small helper in production code (or a package-level unexported function) and testing that directly to avoid duplication and future drift.
Recommendation
APPROVE — The changes are sound: the flag default and help text now match the intended behavior, and fetchPatterns builds paths robustly (empty means repo root; entries are trimmed and empty entries skipped). The added unit test exercises the key path parsing cases. CI is green. As a small improvement, extract the path-building logic used in fetchPatterns into a helper and use it in both production and tests to prevent duplication. Otherwise, this is ready to merge.
# Gpt Review
## Summary
Change correctly defaults patterns-files to empty and updates fetch logic to treat empty as repo root. Implementation is straightforward, idiomatic, and covered by a targeted unit test; CI passed.
## Findings
| # | Severity | File | Line | Finding |
|---|----------|------|------|--------|
| 1 | [MINOR] | `cmd/review-bot/main_test.go` | 504 | TestBuildPatternPaths re-implements the path parsing logic inline. Consider extracting the path-building into a small helper in production code (or a package-level unexported function) and testing that directly to avoid duplication and future drift. |
## Recommendation
**APPROVE** — The changes are sound: the flag default and help text now match the intended behavior, and fetchPatterns builds paths robustly (empty means repo root; entries are trimmed and empty entries skipped). The added unit test exercises the key path parsing cases. CI is green. As a small improvement, extract the path-building logic used in fetchPatterns into a helper and use it in both production and tests to prevent duplication. Otherwise, this is ready to merge.
---
*Review by gpt*
<!-- review-bot:gpt -->
---
*Evaluated against 8b8168dc*
[MINOR] TestBuildPatternPaths re-implements the path parsing logic inline. Consider extracting the path-building into a small helper in production code (or a package-level unexported function) and testing that directly to avoid duplication and future drift.
**[MINOR]** TestBuildPatternPaths re-implements the path parsing logic inline. Consider extracting the path-building into a small helper in production code (or a package-level unexported function) and testing that directly to avoid duplication and future drift.
rodin
merged commit 6f86e66943 into main2026-05-11 17:45:19 +00:00
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.
sonnet-review-bot
security-review-bot
gpt-review-bot
Summary
Change the default for
patterns-filesfrom"README.md"to empty string.When
patterns-filesis empty, fetch all files from the patterns repo root instead of only fetching README.md.This makes the common case work: users who set
patterns-repoexpect to get all pattern files, not just the README.Changes
patterns-filesto""instead of"README.md"patternsFilesis empty, treat it as "fetch all from root" by passing empty path toGetAllFilesInPathTestBuildPatternPathsto verify path-building logicTesting
[""](repo root)Fixes #71
Sonnet Review
Summary
Clean, focused change that fixes the default for
patterns-filesto empty string and adds proper path-building logic with whitespace trimming and empty-entry skipping. CI passes, tests cover the new behavior.Findings
cmd/review-bot/main_test.gobuildPatternPathsas a package-level function and testing that instead, or writing an integration-style test that exercises fetchPatterns with a mock client.Recommendation
APPROVE — The change is correct and well-implemented. The default change from
"README.md"to""resolves the described issue, the empty-path sentinel[""]cleanly represents "fetch from root" without a separate boolean, and whitespace trimming plus empty-entry skipping are defensive improvements over the oldstrings.Splitbehavior. CI passes. The one minor concern is that the test duplicates production logic rather than testing through the actual function, but this is not a blocker. Approve.Review by sonnet
Evaluated against
8b8168dc@@ -504,6 +504,52 @@ func TestIsPatternFile(t *testing.T) {}}// TestBuildPatternPaths verifies the path-building logic for fetchPatterns.[MINOR] TestBuildPatternPaths duplicates the path-building logic from fetchPatterns as a local closure rather than testing fetchPatterns directly. If the production logic in fetchPatterns changes, this test won't catch it. The test is testing a copy of the logic, not the function itself. Consider extracting
buildPatternPathsas a package-level function and testing that instead, or writing an integration-style test that exercises fetchPatterns with a mock client.Security Review
Summary
Changes adjust the default behavior for pattern fetching to include all files from the repo root when patterns-files is empty and add corresponding tests. No exploitable security vulnerabilities are introduced by these modifications.
Recommendation
APPROVE — The updated default and path-building logic are reasonable and do not introduce security issues in themselves. Ensure operationally that the configured patterns repository is curated and does not contain sensitive information (e.g., secrets in YAML files), since all included pattern files are sent to the LLM provider. Otherwise, the implementation and tests look good and CI has passed.
Review by security
Evaluated against
8b8168dcGpt Review
Summary
Change correctly defaults patterns-files to empty and updates fetch logic to treat empty as repo root. Implementation is straightforward, idiomatic, and covered by a targeted unit test; CI passed.
Findings
cmd/review-bot/main_test.goRecommendation
APPROVE — The changes are sound: the flag default and help text now match the intended behavior, and fetchPatterns builds paths robustly (empty means repo root; entries are trimmed and empty entries skipped). The added unit test exercises the key path parsing cases. CI is green. As a small improvement, extract the path-building logic used in fetchPatterns into a helper and use it in both production and tests to prevent duplication. Otherwise, this is ready to merge.
Review by gpt
Evaluated against
8b8168dc@@ -504,6 +504,52 @@ func TestIsPatternFile(t *testing.T) {}[MINOR] TestBuildPatternPaths re-implements the path parsing logic inline. Consider extracting the path-building into a small helper in production code (or a package-level unexported function) and testing that directly to avoid duplication and future drift.