fix(patterns): default patterns-files to empty (fetch all) #77

2026-05-11T16:24:55Z

rodin commented

2026-05-11 16:24:55 +00:00

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

## 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

rodin added 1 commit 2026-05-11 16:24:55 +00:00

fix(patterns): default patterns-files to empty (fetch all)

CI / test (pull_request) Successful in 18s

Details

CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 24s

Details

CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m11s

Details

CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m24s

Details

8b8168dc54

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

sonnet-review-bot approved these changes 2026-05-11 16:25:37 +00:00

sonnet-review-bot left a comment

First-time contributor

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

Evaluated against 8b8168dc

# 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*  --- *Evaluated against 8b8168dc*

cmd/review-bot/main_test.go

						
				@@ -504,6 +504,52 @@ func TestIsPatternFile(t *testing.T) {

					}

				}

				// TestBuildPatternPaths verifies the path-building logic for fetchPatterns.

sonnet-review-bot commented

2026-05-11 16:25:37 +00:00

First-time contributor

[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

security-review-bot approved these changes 2026-05-11 16:26:14 +00:00

security-review-bot left a comment

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 8b8168dc

# 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 8b8168dc*

gpt-review-bot approved these changes 2026-05-11 16:26:38 +00:00

gpt-review-bot left a comment

First-time contributor

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

Evaluated against 8b8168dc

# 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*  --- *Evaluated against 8b8168dc*

cmd/review-bot/main_test.go

						
				@@ -504,6 +504,52 @@ func TestIsPatternFile(t *testing.T) {

					}

gpt-review-bot commented

2026-05-11 16:26:38 +00:00

First-time contributor

[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.