When `patterns-repo` is configured, now logs at Info level:
- File paths loaded from each repo
- Count of files per repo
At Debug level logs skipped files (non-markdown/txt/yaml).
Warns if no pattern files were loaded from a repo (likely misconfigured `patterns-files` path).
Closes #64
rodin
self-assigned this 2026-05-11 06:37:22 +00:00
When patterns-repo is configured, now logs at Info level:
- File paths loaded from each repo
- Count of files per repo
At Debug level logs skipped files (non-markdown/txt/yaml).
Warns if no pattern files were loaded from a repo (likely
misconfigured patterns-files path).
Closes#64
Small, focused change that adds structured logging for pattern file loading. The implementation is correct, idiomatic, and follows the established slog patterns already used throughout the codebase.
Findings
#
Severity
File
Line
Finding
1
[NIT]
cmd/review-bot/main.go
575
The files map from client.GetAllFilesInPath is iterated with range, which has non-deterministic order in Go. This means repoLoadedFiles and repoSkippedFiles slices will have a random order each run, making log output non-reproducible. Consider sorting the slices before logging for consistent output (though this is low priority since it's purely diagnostic).
2
[NIT]
cmd/review-bot/main.go
583
The paths variable logged in the warn case ("paths", paths) is the full slice of all configured paths, not the per-repo paths that were actually attempted. This is accurate but may be slightly misleading if paths are repo-specific. The current behavior is fine given the shared paths slice, just worth noting.
Recommendation
APPROVE — Approve. The change is clean, minimal, and correct. It adds useful observability (Info for loaded files with count, Warn for empty results indicating misconfiguration, Debug for skipped files) using the idiomatic log/slog structured logging already established in the codebase. The two nits are cosmetic and do not affect correctness or functionality.
# Sonnet Review
## Summary
Small, focused change that adds structured logging for pattern file loading. The implementation is correct, idiomatic, and follows the established slog patterns already used throughout the codebase.
## Findings
| # | Severity | File | Line | Finding |
|---|----------|------|------|--------|
| 1 | [NIT] | `cmd/review-bot/main.go` | 575 | The `files` map from `client.GetAllFilesInPath` is iterated with `range`, which has non-deterministic order in Go. This means `repoLoadedFiles` and `repoSkippedFiles` slices will have a random order each run, making log output non-reproducible. Consider sorting the slices before logging for consistent output (though this is low priority since it's purely diagnostic). |
| 2 | [NIT] | `cmd/review-bot/main.go` | 583 | The `paths` variable logged in the warn case (`"paths", paths`) is the full slice of all configured paths, not the per-repo paths that were actually attempted. This is accurate but may be slightly misleading if paths are repo-specific. The current behavior is fine given the shared `paths` slice, just worth noting. |
## Recommendation
**APPROVE** — Approve. The change is clean, minimal, and correct. It adds useful observability (Info for loaded files with count, Warn for empty results indicating misconfiguration, Debug for skipped files) using the idiomatic `log/slog` structured logging already established in the codebase. The two nits are cosmetic and do not affect correctness or functionality.
---
*Review by sonnet*
<!-- review-bot:sonnet -->
---
*Evaluated against deade3c5*
[NIT] The files map from client.GetAllFilesInPath is iterated with range, which has non-deterministic order in Go. This means repoLoadedFiles and repoSkippedFiles slices will have a random order each run, making log output non-reproducible. Consider sorting the slices before logging for consistent output (though this is low priority since it's purely diagnostic).
**[NIT]** The `files` map from `client.GetAllFilesInPath` is iterated with `range`, which has non-deterministic order in Go. This means `repoLoadedFiles` and `repoSkippedFiles` slices will have a random order each run, making log output non-reproducible. Consider sorting the slices before logging for consistent output (though this is low priority since it's purely diagnostic).
[NIT] The paths variable logged in the warn case ("paths", paths) is the full slice of all configured paths, not the per-repo paths that were actually attempted. This is accurate but may be slightly misleading if paths are repo-specific. The current behavior is fine given the shared paths slice, just worth noting.
**[NIT]** The `paths` variable logged in the warn case (`"paths", paths`) is the full slice of all configured paths, not the per-repo paths that were actually attempted. This is accurate but may be slightly misleading if paths are repo-specific. The current behavior is fine given the shared `paths` slice, just worth noting.
security-review-bot
requested review from security-review-bot 2026-05-11 06:38:23 +00:00
The change adds structured logging for loaded and skipped pattern files. There are no introduced security risks or misuse of untrusted input, and CI has passed.
Recommendation
APPROVE — Approve as submitted. The new logs (info/debug) provide useful observability without exposing sensitive data or increasing attack surface. Consider keeping the file lists at debug level if log volume becomes an operational concern, but this is not a security issue.
# Security Review
## Summary
The change adds structured logging for loaded and skipped pattern files. There are no introduced security risks or misuse of untrusted input, and CI has passed.
## Recommendation
**APPROVE** — Approve as submitted. The new logs (info/debug) provide useful observability without exposing sensitive data or increasing attack surface. Consider keeping the file lists at debug level if log volume becomes an operational concern, but this is not a security issue.
---
*Review by security*
<!-- review-bot:security -->
---
*Evaluated against deade3c5*
gpt-review-bot
approved these changes 2026-05-11 06:38:46 +00:00
Changes add helpful logging around pattern file loading without altering core behavior. Implementation is straightforward, idiomatic, and consistent with existing logging and dependency conventions.
Recommendation
APPROVE — The added info/debug logs in fetchPatterns correctly capture loaded and skipped files per repo and warn when none are found, aligning with the PR intent. No new dependencies were introduced, error handling remains unchanged, and CI passed. Optionally, consider trimming the displayed paths list (you already trim per-iteration) or deduplicating file paths in cases where multiple input paths overlap to reduce log noise in very large repos. Otherwise, the changes are solid.
# Gpt Review
## Summary
Changes add helpful logging around pattern file loading without altering core behavior. Implementation is straightforward, idiomatic, and consistent with existing logging and dependency conventions.
## Recommendation
**APPROVE** — The added info/debug logs in fetchPatterns correctly capture loaded and skipped files per repo and warn when none are found, aligning with the PR intent. No new dependencies were introduced, error handling remains unchanged, and CI passed. Optionally, consider trimming the displayed paths list (you already trim per-iteration) or deduplicating file paths in cases where multiple input paths overlap to reduce log noise in very large repos. Otherwise, the changes are solid.
---
*Review by gpt*
<!-- review-bot:gpt -->
---
*Evaluated against deade3c5*
Completeness: No tests needed for observational logging, PR description clear ✅
Bot Review NITs (acknowledged)
Two cosmetic NITs from sonnet-review-bot were noted:
Map iteration order causes non-deterministic file list ordering — acceptable for diagnostic logs
paths in warn message is the full config slice — accurate reflection of what was configured
Neither affects correctness or debuggability. The implementation achieves the issue goals cleanly.
Self-review by rodin
## Self-review: PASS
No blocking issues found — ready for human review.
### Summary
Reviewed the diff against the issue requirements (#64) and codebase patterns:
- **Coherence**: Single-purpose change adding observability for pattern file loading ✅
- **Fit**: Uses idiomatic `log/slog` structured logging consistent with existing patterns ✅
- **Solution**: All issue requirements addressed (file paths, counts, skipped files, misconfiguration warning) ✅
- **Completeness**: No tests needed for observational logging, PR description clear ✅
### Bot Review NITs (acknowledged)
Two cosmetic NITs from sonnet-review-bot were noted:
1. Map iteration order causes non-deterministic file list ordering — acceptable for diagnostic logs
2. `paths` in warn message is the full config slice — accurate reflection of what was configured
Neither affects correctness or debuggability. The implementation achieves the issue goals cleanly.
---
*Self-review by rodin*
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
When
patterns-repois configured, now logs at Info level:At Debug level logs skipped files (non-markdown/txt/yaml).
Warns if no pattern files were loaded from a repo (likely misconfigured
patterns-filespath).Closes #64
Sonnet Review
Summary
Small, focused change that adds structured logging for pattern file loading. The implementation is correct, idiomatic, and follows the established slog patterns already used throughout the codebase.
Findings
cmd/review-bot/main.gofilesmap fromclient.GetAllFilesInPathis iterated withrange, which has non-deterministic order in Go. This meansrepoLoadedFilesandrepoSkippedFilesslices will have a random order each run, making log output non-reproducible. Consider sorting the slices before logging for consistent output (though this is low priority since it's purely diagnostic).cmd/review-bot/main.gopathsvariable logged in the warn case ("paths", paths) is the full slice of all configured paths, not the per-repo paths that were actually attempted. This is accurate but may be slightly misleading if paths are repo-specific. The current behavior is fine given the sharedpathsslice, just worth noting.Recommendation
APPROVE — Approve. The change is clean, minimal, and correct. It adds useful observability (Info for loaded files with count, Warn for empty results indicating misconfiguration, Debug for skipped files) using the idiomatic
log/slogstructured logging already established in the codebase. The two nits are cosmetic and do not affect correctness or functionality.Review by sonnet
Evaluated against
deade3c5@@ -567,0 +572,4 @@if len(repoLoadedFiles) > 0 {slog.Info("loaded pattern files", "repo", repoRef, "count", len(repoLoadedFiles), "files", repoLoadedFiles)} else {[NIT] The
filesmap fromclient.GetAllFilesInPathis iterated withrange, which has non-deterministic order in Go. This meansrepoLoadedFilesandrepoSkippedFilesslices will have a random order each run, making log output non-reproducible. Consider sorting the slices before logging for consistent output (though this is low priority since it's purely diagnostic).@@ -567,3 +580,4 @@}}return sb.String()}[NIT] The
pathsvariable logged in the warn case ("paths", paths) is the full slice of all configured paths, not the per-repo paths that were actually attempted. This is accurate but may be slightly misleading if paths are repo-specific. The current behavior is fine given the sharedpathsslice, just worth noting.Security Review
Summary
The change adds structured logging for loaded and skipped pattern files. There are no introduced security risks or misuse of untrusted input, and CI has passed.
Recommendation
APPROVE — Approve as submitted. The new logs (info/debug) provide useful observability without exposing sensitive data or increasing attack surface. Consider keeping the file lists at debug level if log volume becomes an operational concern, but this is not a security issue.
Review by security
Evaluated against
deade3c5Gpt Review
Summary
Changes add helpful logging around pattern file loading without altering core behavior. Implementation is straightforward, idiomatic, and consistent with existing logging and dependency conventions.
Recommendation
APPROVE — The added info/debug logs in fetchPatterns correctly capture loaded and skipped files per repo and warn when none are found, aligning with the PR intent. No new dependencies were introduced, error handling remains unchanged, and CI passed. Optionally, consider trimming the displayed paths list (you already trim per-iteration) or deduplicating file paths in cases where multiple input paths overlap to reduce log noise in very large repos. Otherwise, the changes are solid.
Review by gpt
Evaluated against
deade3c5Self-review: PASS
No blocking issues found — ready for human review.
Summary
Reviewed the diff against the issue requirements (#64) and codebase patterns:
log/slogstructured logging consistent with existing patterns ✅Bot Review NITs (acknowledged)
Two cosmetic NITs from sonnet-review-bot were noted:
pathsin warn message is the full config slice — accurate reflection of what was configuredNeither affects correctness or debuggability. The implementation achieves the issue goals cleanly.
Self-review by rodin