[purelock] Lock down recommendAuditComparisonAction, prepareNestedMapValueForYAML, gatewayEntryToTimelineEvent with pure-functio [Content truncated due to length] #53123
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| # ADR-53123: PureLock — Automated Pure-Function Test Coverage | ||
|
|
||
| **Date**: 2026-08-16 | ||
| **Status**: Draft | ||
| **Deciders**: Unknown (automation-generated PR by PureLock) | ||
|
|
||
| --- | ||
|
|
||
| ### Context | ||
|
|
||
| Several pure Go functions in `pkg/cli/` and `pkg/workflow/` had statement coverage in the 46–61% range (`recommendAuditComparisonAction` at 46.7%, `gatewayEntryToTimelineEvent` at 48.5%, `prepareNestedMapValueForYAML` at 60.9%). These functions contain non-trivial branching logic — priority ordering, type switches, and fallback chains — that create regression risk. No existing workflow systematically targeted pure functions for comprehensive branch coverage; coverage improvements happened reactively during feature work, leaving gaps that were difficult to close without dedicated effort. | ||
|
|
||
| ### Decision | ||
|
|
||
| We will use PureLock, an automated precompute pass that ranks pure Go functions by coverage deficit and cyclomatic complexity, and generates comprehensive testify table-driven test suites for the top-ranked candidates. PureLock targets only functions with no observable side effects (no I/O, no global mutation), enabling full branch coverage via pure input/output assertions. Each generated test file is reviewed and committed to the PR branch before merge. | ||
|
|
||
| ### Alternatives Considered | ||
|
|
||
| #### Alternative 1: Manual test authoring by feature owners | ||
|
|
||
| Tests are written by the engineer implementing a feature, as part of normal PR workflow. This is the existing practice for most of the codebase. Why not chosen: coverage improvements for pre-existing low-coverage functions depend entirely on whether an engineer proactively adds tests outside their feature scope. In practice, low-coverage pure functions persist indefinitely because no one has an incentive to write tests for code they did not change. | ||
|
|
||
| #### Alternative 2: Integration or end-to-end tests | ||
|
|
||
| Cover the target functions indirectly by exercising the higher-level workflows that call them. Why not chosen: integration tests cannot guarantee 100% branch-level coverage of a specific function, take significantly longer to run, require network-dependent infrastructure (e.g., `httptest` servers that are blocked in the sandbox), and obscure which branches were actually exercised. | ||
|
|
||
| ### Consequences | ||
|
|
||
| #### Positive | ||
| - Achieves 100% function-level statement coverage for each targeted pure function, closing coverage gaps that would otherwise persist indefinitely. | ||
| - Scales automatically: PureLock can identify and generate tests for many low-coverage functions without requiring human initiative or scope creep into feature work. | ||
| - Table-driven tests with explicit subtest names serve as living documentation of expected function behavior and branch semantics. | ||
|
|
||
| #### Negative | ||
| - Generated test files can drift from production code as function signatures or branch logic evolve; reviewers must ensure tests remain meaningful after refactors. | ||
| - Automation-generated PRs consume merge queue capacity and require reviewer bandwidth to inspect generated test cases for correctness, even when no production code changes. | ||
|
|
||
| #### Neutral | ||
| - PureLock PRs are generated by a bot (`github-actions`) rather than a human author; they carry `automation`, `testing`, and `coverage` labels for filtering. | ||
| - Full-package test runs may be blocked in the sandbox by network-dependent tests in unrelated files; targeted `-run` flags are required to validate the generated tests in CI. | ||
|
|
||
| --- | ||
|
|
||
| *ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* |
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -990,3 +990,191 @@ func TestRenderUnifiedTimelineStream_SteeringEvent(t *testing.T) { | |
| t.Errorf("output missing steering message; got:\n%s", out) | ||
| } | ||
| } | ||
|
|
||
| // ─── gatewayEntryToTimelineEvent ─────────────────────────────────────────────── | ||
|
|
||
| func TestGatewayEntryToTimelineEvent(t *testing.T) { | ||
|
Contributor
Author
There was a problem hiding this comment. [/tdd] 💡 Suggested fixAdd @copilot please address this. |
||
| baseTS := "2024-01-15T10:00:00Z" | ||
| baseTime := time.Date(2024, 1, 15, 10, 0, 0, 0, time.UTC) | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| entry GatewayLogEntry | ||
| wantOK bool | ||
| wantKind TimelineEventKind | ||
| wantServer string | ||
| wantTool string | ||
| wantStatus string | ||
| wantError string | ||
| wantReason string | ||
| wantAuthor string | ||
| }{ | ||
| { | ||
| name: "unparseable timestamp returns false", | ||
| entry: GatewayLogEntry{Timestamp: "not-a-timestamp", Event: "tool_call"}, | ||
| wantOK: false, | ||
| }, | ||
| { | ||
| name: "empty timestamp returns false", | ||
| entry: GatewayLogEntry{Timestamp: "", Event: "tool_call"}, | ||
| wantOK: false, | ||
| }, | ||
| { | ||
| name: "DIFC_FILTERED uses ServerID when present", | ||
| entry: GatewayLogEntry{ | ||
| Timestamp: baseTS, | ||
| Type: "DIFC_FILTERED", | ||
| ServerID: "srv-1", | ||
| ServerName: "srv-fallback", | ||
| ToolName: "tool-a", | ||
| Reason: "blocked secrecy", | ||
| AuthorLogin: "octocat", | ||
| }, | ||
| wantOK: true, | ||
| wantKind: TimelineKindDIFCFiltered, | ||
| wantServer: "srv-1", | ||
| wantTool: "tool-a", | ||
| wantReason: "blocked secrecy", | ||
| wantAuthor: "octocat", | ||
| }, | ||
| { | ||
| name: "DIFC_FILTERED falls back to ServerName when ServerID empty", | ||
| entry: GatewayLogEntry{ | ||
| Timestamp: baseTS, | ||
| Type: "DIFC_FILTERED", | ||
| ServerName: "srv-fallback", | ||
| }, | ||
| wantOK: true, | ||
| wantKind: TimelineKindDIFCFiltered, | ||
| wantServer: "srv-fallback", | ||
| }, | ||
| { | ||
| name: "GUARD_POLICY_BLOCKED uses ServerID when present", | ||
| entry: GatewayLogEntry{ | ||
| Timestamp: baseTS, | ||
| Type: "GUARD_POLICY_BLOCKED", | ||
| ServerID: "srv-2", | ||
| ToolName: "tool-b", | ||
| Reason: "policy violation", | ||
| Message: "guard rejected the call", | ||
| }, | ||
| wantOK: true, | ||
| wantKind: TimelineKindGuardPolicyBlocked, | ||
| wantServer: "srv-2", | ||
| wantTool: "tool-b", | ||
| wantReason: "policy violation", | ||
| wantError: "guard rejected the call", | ||
| }, | ||
| { | ||
| name: "GUARD_POLICY_BLOCKED falls back to ServerName when ServerID empty", | ||
| entry: GatewayLogEntry{ | ||
| Timestamp: baseTS, | ||
| Type: "GUARD_POLICY_BLOCKED", | ||
| ServerName: "srv-fallback-guard", | ||
| }, | ||
| wantOK: true, | ||
| wantKind: TimelineKindGuardPolicyBlocked, | ||
| wantServer: "srv-fallback-guard", | ||
| }, | ||
| { | ||
| name: "tool_call event with explicit status is preserved", | ||
| entry: GatewayLogEntry{ | ||
| Timestamp: baseTS, | ||
| Event: "tool_call", | ||
| ServerName: "srv-3", | ||
| ToolName: "tool-c", | ||
| Method: "call", | ||
| Duration: 12.5, | ||
| Status: "success", | ||
| }, | ||
| wantOK: true, | ||
| wantKind: TimelineKindToolCall, | ||
| wantServer: "srv-3", | ||
| wantTool: "tool-c", | ||
| wantStatus: "success", | ||
| }, | ||
| { | ||
| name: "rpc_call event with error sets status to error", | ||
| entry: GatewayLogEntry{ | ||
| Timestamp: baseTS, | ||
| Event: "rpc_call", | ||
| ToolName: "tool-d", | ||
| Error: "boom", | ||
| }, | ||
| wantOK: true, | ||
| wantKind: TimelineKindToolCall, | ||
| wantTool: "tool-d", | ||
| wantStatus: "error", | ||
| wantError: "boom", | ||
| }, | ||
| { | ||
| name: "request event with error level sets status to error", | ||
| entry: GatewayLogEntry{ | ||
| Timestamp: baseTS, | ||
| Event: "request", | ||
| Level: "error", | ||
| }, | ||
| wantOK: true, | ||
| wantKind: TimelineKindToolCall, | ||
| wantStatus: "error", | ||
| }, | ||
| { | ||
| name: "request event with no error defaults to success", | ||
| entry: GatewayLogEntry{ | ||
| Timestamp: baseTS, | ||
| Event: "request", | ||
| }, | ||
| wantOK: true, | ||
| wantKind: TimelineKindToolCall, | ||
| wantStatus: "success", | ||
| }, | ||
| { | ||
| name: "unknown type and unknown event returns false", | ||
| entry: GatewayLogEntry{ | ||
| Timestamp: baseTS, | ||
| Type: "SOMETHING_ELSE", | ||
| Event: "unknown_event", | ||
| }, | ||
| wantOK: false, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| evt, ok := gatewayEntryToTimelineEvent(tt.entry) | ||
| if ok != tt.wantOK { | ||
| t.Fatalf("gatewayEntryToTimelineEvent() ok = %v, want %v", ok, tt.wantOK) | ||
| } | ||
| if !tt.wantOK { | ||
| return | ||
| } | ||
|
Comment on lines
+1148
to
+1150
|
||
| if !evt.Time.Equal(baseTime) { | ||
| t.Errorf("Time = %v, want %v", evt.Time, baseTime) | ||
| } | ||
| if evt.Source != TimelineSourceGateway { | ||
| t.Errorf("Source = %v, want %v", evt.Source, TimelineSourceGateway) | ||
| } | ||
| if evt.Kind != tt.wantKind { | ||
| t.Errorf("Kind = %v, want %v", evt.Kind, tt.wantKind) | ||
| } | ||
| if tt.wantServer != "" && evt.ServerName != tt.wantServer { | ||
| t.Errorf("ServerName = %q, want %q", evt.ServerName, tt.wantServer) | ||
| } | ||
| if tt.wantTool != "" && evt.ToolName != tt.wantTool { | ||
| t.Errorf("ToolName = %q, want %q", evt.ToolName, tt.wantTool) | ||
| } | ||
| if tt.wantStatus != "" && evt.Status != tt.wantStatus { | ||
| t.Errorf("Status = %q, want %q", evt.Status, tt.wantStatus) | ||
| } | ||
| if tt.wantError != "" && evt.Error != tt.wantError { | ||
| t.Errorf("Error = %q, want %q", evt.Error, tt.wantError) | ||
| } | ||
| if tt.wantReason != "" && evt.Reason != tt.wantReason { | ||
| t.Errorf("Reason = %q, want %q", evt.Reason, tt.wantReason) | ||
| } | ||
| if tt.wantAuthor != "" && evt.AuthorLogin != tt.wantAuthor { | ||
| t.Errorf("AuthorLogin = %q, want %q", evt.AuthorLogin, tt.wantAuthor) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/tdd] The
fallback default review messagesubtest (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 achangedlabel andsuccessconclusion — 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.