refactor: replace positional-string args to Compute*Key with named-field structs - #184
Merged
Merged
Conversation
…eld structs ComputeAnalysisKey and ComputeSuggestionKey took 5/8 positional strings respectively, which made same-typed arguments (e.g. adrContent vs fileContent) silently transposable at call sites with no compiler check. Introduces AnalysisKeyInput/SuggestionKeyInput structs so each field is named at the call site. Pure signature refactor -- hashParts still receives the same values in the same order, so hash output is unchanged. Closes #183 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Add digest compatibility coverage and update the existing ADR documentation.
Pull request overview
Refactors cache-key APIs from positional strings to named input structs while preserving hash behavior.
Changes:
- Added
AnalysisKeyInputandSuggestionKeyInput. - Updated production and test call sites.
- Preserved field ordering and cache-key stability.
Review notes:
- Moderate: add fixed digest or compatibility tests for both key functions.
- Nit: update the existing ADR to document
SuggestionKeyInputandFilename.
File summaries
| File | Description |
|---|---|
internal/cache/cache.go |
Defines structured cache-key inputs and updated functions. |
internal/cache/cache_test.go |
Updates cache-key and round-trip tests. |
internal/analysis/engine.go |
Uses structured inputs at production call sites. |
internal/analysis/analysis_test.go |
Updates suggestion-key test fixtures. |
Review details
Suppressed comments (2)
internal/cache/cache.go:92
- Changing this exported API leaves
docs/arch/0017-suggestion-cache-key-namespace.md:17documenting the old positionalComputeSuggestionKeycall (and omittingFilename). Please update that existing ADR to useSuggestionKeyInput, otherwise its documented API no longer matches code that contributors can compile.
func ComputeSuggestionKey(in SuggestionKeyInput) string {
internal/cache/cache_test.go:56
- These tests only compare two calls using the same implementation, so they would still pass if this refactor accidentally reordered fields before calling
hashPartswhile invalidating every existing cache key. Since preserving the digest is part of this refactor's contract, add a fixed expected digest (or a compatibility test against the pre-refactor field order) for eachCompute*Keyfunction.
a := ComputeAnalysisKey(AnalysisKeyInput{ModelName: "gpt-4", ADRContent: "adr", FileContent: "code", SystemPrompt: "sys", UserPromptTemplate: "tmpl"})
b := ComputeAnalysisKey(AnalysisKeyInput{ModelName: "gpt-4", ADRContent: "adr", FileContent: "code", SystemPrompt: "sys", UserPromptTemplate: "tmpl"})
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…e ADR Copilot review on #184 flagged two issues: the self-consistency tests for ComputeAnalysisKey/ComputeSuggestionKey would still pass if the refactor had silently reordered fields before hashing (invalidating every cache entry), and docs/arch/0017-suggestion-cache-key-namespace.md still documented the old positional signature (and omitted Filename). Adds a golden-digest test per function, verified against digests computed from the pre-#183 positional implementation with identical inputs, and updates the ADR's Decision section to the struct-based signature. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
Addressed both points from the Copilot review in ce8202a:
|
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
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.
Summary
cache.ComputeAnalysisKeyandcache.ComputeSuggestionKeytook 5 and 8 positionalstringparameters respectively, which the compiler can't catch if same-typed args (e.g.adrContent/fileContent,reasoning/quotedCode) get transposed at a call site.cache.AnalysisKeyInputandcache.SuggestionKeyInputnamed-field structs; bothCompute*Keyfunctions now take one of these instead of a flat positional list.internal/analysis/engine.go's two call sites and allinternal/cache/cache_test.go/internal/analysis/analysis_test.gocall sites updated to the new shape.This is a pure signature refactor —
hashPartsreceives the exact same values in the exact same order as before, so hash output (and cache-key stability/collision-resistance guarantees from #179/#182) is unchanged. No new ADR: this doesn't introduce a new architectural decision, just hardens an existing call-site convention.Deviations from the plan
None — implemented exactly as proposed in the issue.
Test plan
go build ./...passesgo test ./...passes (all packages, including the exact-value hash-stability/collision tests ininternal/cache/cache_test.goandinternal/analysis/analysis_test.go, which would catch any field-order regression)golangci-lint run --timeout=5mreports 0 issues on the changed code (unrelated stdlib typecheck error present in this environment predates the change)hashPartsis unchanged in both functionsCloses #183
🤖 Generated with Claude Code