Chore/code quality improvements#37
Merged
Merged
Conversation
Covers FromEnv parsing, retryable vs non-retryable statuses, Retry-After handling, retry exhaustion semantics, context cancellation during backoff, and per-attempt request rebuilding. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…opping them BuildScanner now reports allow_patterns/custom_patterns entries it had to skip (unparseable regex, empty custom-pattern id). The security section emits each issue as a security.secrets-config fail finding, and ScanGitHistory - which never runs config validation - returns an error rather than silently scanning with reduced coverage. Adds the security.secrets-config catalog entry with fix template and A05 OWASP mapping, docs, and tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Coverage command failures previously formatted only the captured output, so a timeout or context cancellation was indistinguishable from a tool that ran and reported failures. Wrap the underlying error with %w alongside the output. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Route the eight copy-pasted 'load config -> stderr -> exit code' sites through loadConfigOrFail, and normalize literal 1/0 handler returns to the exitError/exitOK constants in the touched files. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#Code quality improvements: httpretry test coverage, secret-pattern diagnostics, CLI cleanup
This PR bundles four targeted improvements found during a codebase review.
Test coverage for ai/httpretry (test:)
The retry/backoff package used by all HTTP-backed AI providers had no direct unit tests. Adds 12 table-driven tests covering FromEnv parsing (defaults, overrides, invalid values), retryable vs. non-retryable statuses, Retry-After honoring (and fallback on malformed values), retry-exhaustion semantics (final response returned unchanged), context cancellation during backoff, per-attempt request rebuilding, and immediate return on build errors.
Invalid secret patterns are no longer silently dropped (fix(security):)
BuildScanner discarded regexp.Compile errors for allow_patterns and custom_patterns, so a typo in a pattern silently disabled it. Config validation catches this on the normal scan path, but ScanGitHistory never validates — a broken pattern meant quietly scanning history with reduced coverage.
Coverage-command failures keep the exec error (fix(quality):)
quality_coverage_lcov.go and quality_coverage_go.go formatted only the captured tool output, making a timeout or context cancellation indistinguishable from a genuinely failing test run. The underlying error is now wrapped with %w alongside the output.
CLI config-load boilerplate extracted (refactor(cli):)
The eight copy-pasted "load config → print to stderr → return exit code" sites now route through a shared loadConfigOrFail helper, and literal 1/0 handler returns in the touched files are normalized to the exitError/exitOK constants. No behavior change.
Verification
go build, go vet, gofmt, the full test suite (9 packages), and golangci-lint (0 issues) all pass.
Also captured in .claude/knowledge/testing-patterns.md: catalog rules require a fix template, and security.* rules additionally require an OWASP mapping — both enforced by existing catalog tests.