feat: log loaded pattern files for debugging #65

2026-05-11T06:37:22Z

rodin commented

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

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

rodin added 1 commit 2026-05-11 06:37:22 +00:00

feat: log loaded pattern files for debugging

CI / test (pull_request) Successful in 19s

Details

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

Details

CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 42s

Details

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

Details

deade3c5a0

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

sonnet-review-bot approved these changes 2026-05-11 06:38:00 +00:00

sonnet-review-bot left a comment

First-time contributor

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

Evaluated against deade3c5

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

cmd/review-bot/main.go

						
				@@ -567,0 +572,4 @@

						if len(repoLoadedFiles) > 0 {

							slog.Info("loaded pattern files", "repo", repoRef, "count", len(repoLoadedFiles), "files", repoLoadedFiles)

						} else {

sonnet-review-bot commented

2026-05-11 06:38:00 +00:00

First-time contributor

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

cmd/review-bot/main.go

						
				@@ -567,3 +580,4 @@

						}

					}

					return sb.String()

				}

sonnet-review-bot commented

2026-05-11 06:38:00 +00:00

First-time contributor

[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

security-review-bot approved these changes 2026-05-11 06:38:23 +00:00

security-review-bot left a comment

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 deade3c5

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

gpt-review-bot approved these changes 2026-05-11 06:38:46 +00:00

gpt-review-bot left a comment

First-time contributor

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

Evaluated against deade3c5

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

rodin commented

2026-05-11 06:51:29 +00:00

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:

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*

rodin added the self-reviewed label 2026-05-11 06:51:34 +00:00

aweiker merged commit 4c327b61d4 into main

2026-05-11 06:53:00 +00:00

Sign in to join this conversation.

No Reviewers

sonnet-review-bot

security-review-bot

gpt-review-bot

4 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: rodin/review-bot#65