[purelock] Lock down removeUnsafeEngineEnvKeys, migrateMessagesEffectiveTokensSuffixToAICreditsSuffix with pure-function test suites - #51783
Conversation
…eMessagesEffectiveTokensSuffixToAICreditsSuffix Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Great work on this automated coverage initiative! 🎯 This PR adds comprehensive test suites for two pure functions ( ✅ removeUnsafeEngineEnvKeys: 0% → 93.8% coverage with 11 subtests covering edge cases (multiline values, comment continuations, block transitions, etc.) The PR body is excellent — it explains the purity analysis, coverage justification, and residual coverage reasoning. This aligns perfectly with the project's agentic development workflow and quality standards.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel: Identified hard violation (missing build tags) on both test files. These must be added before merge. All 4 tests are high-quality with strong purity/invariant coverage and no mock violations.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ Ponytail Reviewer completed successfully!
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Pull request overview
Adds focused coverage for two pure codemod helpers in pkg/cli.
Changes:
- Tests unsafe
engine.envkey removal and input purity. - Tests message placeholder migration and deterministic behavior.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/codemod_engine_env_secrets_pure_test.go |
Covers unsafe environment-key removal. |
pkg/cli/codemod_messages_effective_tokens_suffix_to_ai_credits_suffix_pure_test.go |
Covers scoped placeholder migration. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| unsafeKeys := map[string]struct{}{"FOO": {}} | ||
|
|
||
| result1, modified1 := removeUnsafeEngineEnvKeys(inputCopy, unsafeKeys) | ||
| // Input slice must remain unchanged. | ||
| assert.Equal(t, original, inputCopy, "input lines were mutated") |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR adds pure-function test suites for removeUnsafeEngineEnvKeys and migrateMessagesEffectiveTokensSuffixToAICreditsSuffix. Tests are well-structured with good edge-case coverage (empty input, multi-line values, scope boundary checks) and include purity/determinism verification. No production code changes. LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 17.4 AIC · ⌖ 7.1 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Generated by ✂️ Ponytail Reviewer for #51783 · auto · 26.8 AIC · ⌖ 4.32 AIC · ⊞ 6.8K
Comment /ponytail to run again
| } | ||
|
|
||
| for _, tt := range tests { | ||
| tt := tt |
There was a problem hiding this comment.
L176: delete: tt := tt loop-variable capture is unnecessary since Go 1.22 (module targets go 1.26.5) - the loop var is already scoped per iteration.
| " FOO: bar", | ||
| " BAZ: qux", | ||
| } | ||
| inputCopy := make([]string, len(original)) |
There was a problem hiding this comment.
L198-199: stdlib: manual make+copy to clone a slice. slices.Clone(original), 1 line.
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { |
There was a problem hiding this comment.
L149: delete: tt := tt loop-variable capture is unnecessary since Go 1.22 (module targets go 1.26.5).
| " messages:", | ||
| " footer: \"{effective_tokens_suffix}\"", | ||
| } | ||
| inputCopy := make([]string, len(original)) |
There was a problem hiding this comment.
L171-172: stdlib: manual make+copy to clone a slice. slices.Clone(original), 1 line.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — approving with a minor style note.
📋 Key Themes & Highlights
Key Themes
- Obsolete loop-variable capture: Both test files use
tt := ttinsidefor _, tt := range tests { t.Parallel() }. This was necessary before Go 1.22 but is redundant in Go 1.26 (see inline comments).
Positive Highlights
- ✅ Excellent table-driven structure with clear Arrange/Act/Assert separation
- ✅ Purity tests are a great addition — explicitly asserting no input mutation and deterministic output
- ✅ YAML boundary/scope edge cases are well thought out (block scalars, transitions out of
engine:/safe-outputs:blocks) - ✅
t.Parallel()used consistently at both the parent and subtest level - ✅ 93.8% and 100% coverage achieved with targeted, readable fixtures
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 34 AIC · ⌖ 7.53 AIC · ⊞ 7.1K
Comment /matt to run again
| lines: []string{ | ||
| "engine:", | ||
| " env:", | ||
| " SAFE: value", |
There was a problem hiding this comment.
[/tdd] tt := tt loop variable capture is a Go 1.21-era idiom that is no longer needed in Go 1.22+ (this module targets Go 1.26). The same pattern appears in the second test file too.
💡 Suggested cleanup
Remove the tt := tt re-assignment in both table-driven loops; loop variables are re-bound per-iteration as of Go 1.22:
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
// tt is safe here without the extra capture
})
}The extra line is harmless but adds noise and may mislead future readers into thinking the old capture rule still applies.
@copilot please address this.
| " footer: \"{ai_credits_suffix}\"", | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
[/tdd] Same tt := tt capture pattern as in the first test file — can be removed since this module is Go 1.26.
@copilot please address this.
🧪 Test Quality Sentinel Report - BLOCKING ISSUEHard Violation: Missing Build TagsBoth test files are missing the required
Fix: Add Test Quality Score: 90/100Excellent coverage: 4 behavioral tests, 19 scenarios, all with descriptive assertions. Purity tests verify design invariants. No mock violations. Strong state machine and edge-case coverage. Verdict: REQUEST_CHANGES due to build tag violation. After fix → APPROVED.
|
…rity assertions Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (390 new lines in Draft ADR committed:
What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
🎉 This pull request is included in a new release. Release: |
Adds maximum-coverage testify test suites for two pure Go functions identified by PureLock as having 0% coverage.
removeUnsafeEngineEnvKeyspkg/cli/codemod_engine_env_secrets.go:153func removeUnsafeEngineEnvKeys(lines []string, unsafeKeys map[string]struct{}) ([]string, bool)lines/unsafeKeysinputs and builds a new result slice via string operations (strings.TrimSpace, indentation helpers) — no I/O, no globals, no mutation of inputs. Verified manually: input slices/maps are never written to, and repeated calls with identical inputs return identical outputs.pkg/cliunaffected at aggregate level (large package, coverage measured via targeted-runfilter).engine:key, noenv:under engine, single/multiple key removal, multiline (block scalar) values, trailing comment continuations, blank-line handling, keys outside the unsafe set, transition out of the engine block into an unrelated top-levelenv:, and empty input. A dedicated purity test asserts the input slice is not mutated and results are deterministic across repeated calls.go tool cover -func; remaining ~6% is likely an unreachable defensive branch not requiring dedicated coverage.migrateMessagesEffectiveTokensSuffixToAICreditsSuffixpkg/cli/codemod_messages_effective_tokens_suffix_to_ai_credits_suffix.go:70func migrateMessagesEffectiveTokensSuffixToAICreditsSuffix(lines []string) ([]string, bool)lines, doing string replacement (strings.ReplaceAll) and building a new result slice; no external state, no channel/goroutine usage, no mutation of the input slice.safe-outputs:block, nomessages:block, simple scalar placeholder replacement, no-placeholder no-op, block scalar (|) multi-line replacement, placeholders outside themessages:block left untouched, exiting thesafe-outputs:block before reaching an unrelated top-levelenv:placeholder, empty input, and comment lines (verifying they are not mistaken for keys but placeholder text inside comments undermessages:is still rewritten per existing behavior). A dedicated purity test asserts no input mutation and deterministic output.go tool cover -funcreports 100.0%.Validation
gofmt -l— clean on both new test files.go vet ./pkg/cli/...— passes.go test ./pkg/cli/... -run 'TestRemoveUnsafeEngineEnvKeys|TestMigrateMessagesEffectiveTokensSuffixToAICreditsSuffix' -race -count=1— all pass.TestRenderScheduleCalendarCell_UsesANSIInColorTerminal,TestConfirmRunAddedWorkflow_ContextCancelled) exist onmainwithout these changes and are out of scope.Note on a third candidate
The precompute candidate list also included
pkg/workflow/safe_outputs_data_schema.go:120:simplifyDataSchemaNode(reported at 0% coverage), but targeted verification withgo test -run TestSimplifyDataSchemaNode -covershowed it is already covered by an existing test (TestSimplifyDataSchemaNodeinsafe_outputs_data_schema_test.go) at 100%. The precompute coverage snapshot for this function was stale; no new test was added and it was recorded asnoopin cache memory.