[purelock] Lock down recommendAuditComparisonAction, prepareNestedMapValueForYAML, gatewayEntryToTimelineEvent with pure-functio [Content truncated due to length] - #53123
Conversation
- recommendAuditComparisonAction (pkg/cli/audit_comparison.go): 100% coverage - gatewayEntryToTimelineEvent (pkg/cli/gateway_logs_timeline.go): 100% coverage - prepareNestedMapValueForYAML (pkg/workflow/yaml.go): 100% coverage Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. Completed PR review locally; no GitHub write was needed because I found no actionable review comments to submit.
|
|
✅ Ponytail Reviewer completed successfully! Ponytail review: diff is purely additive test files (table-driven tests for 3 pure functions). No hand-rolled stdlib reimplementations, no unneeded dependencies, no speculative abstractions, and no dead flexibility introduced. Lean already. Ship.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![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 tests for recommendAuditComparisonAction, prepareNestedMapValueForYAML, and gatewayEntryToTimelineEvent. Tests are well-structured, use t.Parallel() where appropriate, and cover priority ordering, edge cases (empty timestamps, non-string map keys, scalar passthrough), and fallback behaviours. No production code is changed. LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 15.4 AIC · ⌖ 7.04 AIC · ⊞ 5.7K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — commenting with minor observations on coverage and test hygiene.
📋 Key Themes & Highlights
Key Themes
t.Parallel()inconsistency:TestGatewayEntryToTimelineEventskips parallelism used by the other two new suites.- Minor coverage gap: The
changed+success+empty-delta combination inrecommendAuditComparisonActionis not explicitly verified. []anyrecursion assumption: A comment in the YAML test acknowledges an implicit behaviour without asserting it.
Positive Highlights
- ✅ Excellent branch coverage across all three functions — every decision path is exercised.
- ✅ Priority-ordering tests for
recommendAuditComparisonActionare well-structured and exhaustive. - ✅
ServerID/ServerNamefallback for bothDIFC_FILTEREDandGUARD_POLICY_BLOCKEDis tested explicitly. - ✅ Scalar-passthrough and nil handling tested cleanly.
- ✅ All test files pass
gofmtandgo vetper the PR description.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 29.6 AIC · ⌖ 8.69 AIC · ⊞ 7.8K
Comment /matt to run again
|
|
||
| // ─── gatewayEntryToTimelineEvent ─────────────────────────────────────────────── | ||
|
|
||
| func TestGatewayEntryToTimelineEvent(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] TestGatewayEntryToTimelineEvent is missing t.Parallel() on the parent test and subtests — the other two new test functions both call it. Omitting it slows the test suite when running in parallel with other tests.
💡 Suggested fix
Add t.Parallel() immediately after t.Run(tt.name, func(t *testing.T) { for each subtest, and at the top of the parent function.
@copilot please address this.
| expectedContains: "Review first-time write-capable behavior", | ||
| }, | ||
| { | ||
| name: "newly present MCP failure", |
There was a problem hiding this comment.
[/tdd] The fallback default review message subtest (line ~221) verifies that decreased blocked requests and turns fall through to the default message. But there is no test case for all delta fields being zero-valued (AuditComparisonDelta{}) with a changed label and success conclusion — it is ambiguous whether that path also lands on the fallback or takes a different route.
💡 Suggested addition
{
name: "empty delta with changed label falls back to default",
label: "changed",
currentConclusion: "success",
delta: &AuditComparisonDelta{},
expectedContains: "Review the behavior change against the selected successful baseline",
},@copilot please address this.
| t.Errorf("expected nested key to be preserved") | ||
| } | ||
| // original map must not be mutated by copy | ||
| if &copied == &value { |
There was a problem hiding this comment.
[/tdd] The []any recursively processes elements subtest checks that arr[0] stays a map[string]any, but when the fieldName passed to recursion is empty (""), prepareNestedMapValueForYAML won't apply priority-field ordering. The comment inside the test acknowledges this but it would be clearer to assert that the recursive call's own fieldName is irrelevant here — or confirm whether an empty fieldName matches any priority field. If it accidentally does match, the element would be unexpectedly transformed.
💡 Suggested clarification
Either assert the exact type returned (e.g. it does NOT become a yaml.MapSlice), or add a comment citing which argument prepareNestedMapValueForYAML receives for the slice element so future readers don't have to check the implementation.
@copilot please address this.
There was a problem hiding this comment.
Pull request overview
Adds targeted unit tests to lock down three pure mapping and recommendation functions.
Changes:
- Covers audit recommendation branches and priorities.
- Tests gateway log-to-timeline conversion.
- Tests nested YAML ordering and copying behavior.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/audit_comparison_test.go |
Tests recommendation outcomes and priority ordering. |
pkg/cli/gateway_logs_timeline_test.go |
Tests gateway event conversion paths. |
pkg/workflow/yaml_test.go |
Tests nested YAML transformations. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (2)
pkg/cli/gateway_logs_timeline_test.go:1168
- The fixture supplies non-zero
MethodandDuration, but the test never asserts either output field. Removing the assignments atgateway_logs_timeline.go:183-184would still pass this purported mapping lock-down. Assert both values in the shared successful-event checks.
if tt.wantStatus != "" && evt.Status != tt.wantStatus {
t.Errorf("Status = %q, want %q", evt.Status, tt.wantStatus)
}
pkg/workflow/yaml_test.go:671
- This case only verifies the result type, so it still passes if the
map[string]stringbranch drops entries, values, or sorting entirely while returning anyyaml.MapSlice. Assert the complete ordered contents to lock down this distinct type-switch branch.
if _, ok := result.(yaml.MapSlice); !ok {
t.Fatalf("expected yaml.MapSlice, got %T", result)
}
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
| // original map must not be mutated by copy | ||
| if &copied == &value { | ||
| t.Errorf("expected a distinct copied map") | ||
| } |
| if !tt.wantOK { | ||
| return | ||
| } |
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (472 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 Matter
ADRs 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
|
|
🎯 Great work on the PureLock automation! This PR looks ready for review. What's here:
The PR aligns with the project's agentic development model, all tests are focused and well-documented, coverage improvements are significant, and validation passes (
|
|
🎉 This pull request is included in a new release. Release: |
Summary
Adds testify table-driven test suites for three low-coverage pure Go functions —
recommendAuditComparisonAction,gatewayEntryToTimelineEvent, andprepareNestedMapValueForYAML— and records the automation rationale in a draft ADR. No production code was changed; this is a test-coverage-only PR generated by PureLock.Change Classification
Key Changes
pkg/cli/audit_comparison_test.gorecommendAuditComparisonActioncovering priority ordering, conclusion types, deltas, and fallback logicpkg/cli/gateway_logs_timeline_test.gogatewayEntryToTimelineEventcovering timestamp parsing, type-specific field handling (DIFC_FILTERED,GUARD_POLICY_BLOCKED,tool_call,rpc_call,request), and fallback logicpkg/workflow/yaml_test.goprepareNestedMapValueForYAMLcovering map key ordering, nestedyaml.MapSlicehandling, and scalar type preservationdocs/adr/53123-purelock-automated-pure-function-test-coverage.mdImpact Assessment
Commits