Targeted custom-lint cleanup: defer-in-loop resource handling and continuation options struct #43414
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
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 |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| # ADR-43414: Options Struct for Continuation Builder and Defer-in-Loop Fix in pkg/cli | ||
|
|
||
| **Date**: 2026-07-05 | ||
| **Status**: Draft | ||
| **Deciders**: Unknown (automated lint compliance via Copilot SWE agent) | ||
|
|
||
| --- | ||
|
|
||
| ### Context | ||
|
|
||
| Two mechanical lint violations existed in `pkg/cli`: | ||
|
|
||
| 1. `buildContinuationIfNeeded` in `logs_orchestrator.go` accepted 11 positional parameters (`workflowName`, `startDate`, `endDate`, `engine`, `branch`, `afterRunID`, `count`, `timeoutMinutes`, plus three booleans). This violated the project's parameter-count lint rule and made call sites error-prone because two same-type arguments (e.g., adjacent `string` parameters) can be silently transposed with no compiler error. | ||
|
|
||
| 2. `waitForServerReady` in `mcp_inspect_mcp_scripts_server.go` registered a `defer resp.Body.Close()` inside a polling loop. Per the `deferinloop` linter rule (see [ADR-40679](40679-add-deferinloop-linter.md)), deferred calls inside a loop do not execute until the enclosing function returns, not at the end of each iteration — potentially accumulating open response bodies. | ||
|
|
||
| The bulk of the line count in this PR (1512 of 1599 additions) is a JSON reformatting of `pkg/actionpins/data/action_pins.json` and `pkg/workflow/data/action_pins.json` from 2-space to 4-space indentation; this is cosmetic with no functional impact and does not represent a design decision. | ||
|
|
||
| ### Decision | ||
|
|
||
| We will introduce a `continuationOptions` struct to group the eight configuration parameters of `buildContinuationIfNeeded`, reducing the positional argument count to three meaningful booleans plus one options value. We will extract an `isServerReady` helper so that `resp.Body.Close()` is deferred within the helper's scope rather than the polling loop body. Both changes preserve existing behavior while bringing the code into compliance with project lint rules established in [ADR-40679](40679-add-deferinloop-linter.md) and the parameter-count linter. | ||
|
|
||
| ### Alternatives Considered | ||
|
|
||
| #### Alternative 1: Suppress lint violations with `//nolint` directives | ||
|
|
||
| Add inline `//nolint:paramcount` and `//nolint:deferinloop` comments to suppress the findings in-place without changing the code. This is the lowest-effort option and leaves the function signatures untouched. It was not chosen because suppression defeats the purpose of these lint rules — they enforce maintainability invariants that the project explicitly adopted via ADRs — and would set a precedent for bypassing them in newly written code. | ||
|
|
||
| #### Alternative 2: Close the response body explicitly (without defer) inside the loop | ||
|
|
||
| Replace `defer resp.Body.Close()` with an immediate `resp.Body.Close()` at the point where the success branch returns, keeping the defer-in-loop fix without extracting a helper function. For the parameter-count issue, defer changing the signature by splitting `buildContinuationIfNeeded` into two smaller focused functions instead of grouping parameters into a struct. These options were not chosen because the helper extraction makes the readiness check self-contained and reusable, while the struct approach is already established in this codebase (see [ADR-31336](31336-refactor-download-workflow-logs-to-options-struct.md)) and is the idiomatic Go solution for grouping related parameters. | ||
|
|
||
| ### Consequences | ||
|
|
||
| #### Positive | ||
| - Call sites for `buildContinuationIfNeeded` are now self-documenting: named struct fields make the mapping from variable to parameter explicit, eliminating silent transposition bugs. | ||
| - Response bodies in `waitForServerReady` are closed immediately at the end of each successful polling iteration via the helper's deferred close, removing the risk of resource accumulation across iterations. | ||
| - The code passes the project's `deferinloop` and parameter-count lint rules without suppressions, keeping lint signal clean for future contributors. | ||
|
|
||
| #### Negative | ||
| - Callers of `buildContinuationIfNeeded` must construct a `continuationOptions` struct literal instead of passing positional arguments; existing call sites required mechanical updates. | ||
| - The `continuationOptions` type is unexported (`continuationOptions` vs `ContinuationOptions`), limiting it to `pkg/cli` — if the function is later promoted or tested from outside the package, the type must be exported at that point. | ||
|
|
||
| #### Neutral | ||
| - Test files (`logs_orchestrator_unit_test.go`) required mechanical refactoring to use struct literal syntax; no test behavior changed. | ||
| - JSON indentation in `pkg/actionpins/data/action_pins.json` and `pkg/workflow/data/action_pins.json` changed from 2-space to 4-space; no functional impact. | ||
|
|
||
| --- | ||
|
|
||
| *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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -148,11 +148,21 @@ func selectPaginationCursorDate(filteredRuns []WorkflowRun, oldestFetchedCreated | |
| // - countLimitReached: in fetchAllInRange mode the count cap was hit before the | ||
| // date window was exhausted; the next page starts just before the oldest run | ||
| // returned in this batch. | ||
| type continuationOptions struct { | ||
|
Contributor
There was a problem hiding this comment. [/codebase-design] 💡 Suggested placement// continuationOptions collects the pagination parameters forwarded to a
// ContinuationData cursor when a batch is incomplete.
type continuationOptions struct { ... }
// buildContinuationIfNeeded returns a ContinuationData cursor ...
func buildContinuationIfNeeded(...) *ContinuationData {Go convention: a struct's doc-comment precedes the struct; the function doc-comment should stay immediately above the function. @copilot please address this.
Contributor
There was a problem hiding this comment.
💡 Suggested fixMove type continuationOptions struct {
workflowName string
// ...
}
// buildContinuationIfNeeded returns a ContinuationData cursor when more runs may
// be available after this batch, or nil if the full result set was collected.
//
// A continuation is emitted in two cases:
// - timeoutReached: ...
// - countLimitReached: ...
func buildContinuationIfNeeded(Or alternatively, add a short standalone doc comment for |
||
| workflowName string | ||
| startDate string | ||
| endDate string | ||
| engine string | ||
| branch string | ||
| afterRunID int64 | ||
| count int | ||
| timeoutMinutes int | ||
| } | ||
|
|
||
| func buildContinuationIfNeeded( | ||
|
Contributor
There was a problem hiding this comment. [/codebase-design] 💡 Analysis
The only values not in @copilot please address this. |
||
| processedRuns []ProcessedRun, | ||
| timeoutReached, countLimitReached bool, | ||
| opts continuationOptions, | ||
| ) *ContinuationData { | ||
| if len(processedRuns) == 0 || (!timeoutReached && !countLimitReached) { | ||
| return nil | ||
|
|
@@ -166,15 +176,15 @@ func buildContinuationIfNeeded( | |
| } | ||
| return &ContinuationData{ | ||
| Message: message, | ||
| WorkflowName: opts.workflowName, | ||
| Count: opts.count, | ||
| StartDate: opts.startDate, | ||
| EndDate: opts.endDate, | ||
| Engine: opts.engine, | ||
| Branch: opts.branch, | ||
| AfterRunID: opts.afterRunID, | ||
| BeforeRunID: oldestRunID, | ||
| Timeout: opts.timeoutMinutes, | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -735,8 +745,16 @@ outerLoop: | |
|
|
||
| // Build continuation data if timeout was reached and there are processed runs, | ||
| // OR if a date-range fetch hit the count limit (more runs may exist in the window). | ||
| continuation := buildContinuationIfNeeded(processedRuns, timeoutReached, countLimitReached, continuationOptions{ | ||
| workflowName: workflowName, | ||
| startDate: startDate, | ||
| endDate: endDate, | ||
| engine: engine, | ||
| branch: ref, | ||
| afterRunID: afterRunID, | ||
| count: count, | ||
| timeoutMinutes: timeoutMinutes, | ||
| }) | ||
|
|
||
| return renderLogsOutput(processedRuns, renderLogsOutputOptions{ | ||
| outputDir: outputDir, | ||
|
|
||
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 |
|---|---|---|
|
|
@@ -144,10 +144,16 @@ func TestBuildContinuationIfNeeded(t *testing.T) { | |
| } | ||
|
|
||
| t.Run("count limit reached emits cursor with correct message and BeforeRunID", func(t *testing.T) { | ||
| c := buildContinuationIfNeeded(runs, false, true, continuationOptions{ | ||
| workflowName: "my-workflow", | ||
| startDate: "2026-06-01", | ||
| endDate: "2026-06-30", | ||
| engine: "claude", | ||
| branch: "main", | ||
| afterRunID: 0, | ||
|
Contributor
There was a problem hiding this comment. [/tdd] 💡 Suggested additiont.Run("afterRunID is propagated to cursor", func(t *testing.T) {
c := buildContinuationIfNeeded(runs, false, true, continuationOptions{
workflowName: "my-workflow",
startDate: "2026-06-01",
afterRunID: 42,
count: 100,
})
require.NotNil(t, c)
assert.Equal(t, int64(42), c.AfterRunID)
})@copilot please address this. |
||
| count: 100, | ||
| timeoutMinutes: 3, | ||
| }) | ||
| require.NotNil(t, c, "expected continuation when countLimitReached=true") | ||
| assert.Equal(t, int64(2999), c.BeforeRunID, "BeforeRunID should be oldest processed run") | ||
| assert.Equal(t, "2026-06-01", c.StartDate) | ||
|
|
@@ -157,26 +163,46 @@ func TestBuildContinuationIfNeeded(t *testing.T) { | |
| }) | ||
|
|
||
| t.Run("timeout reached emits cursor with timeout message", func(t *testing.T) { | ||
|
Contributor
There was a problem hiding this comment. [/tdd] The timeout test (line 165) doesn't assert 💡 Suggested additionsassert.Equal(t, 10, c.Timeout, "timeoutMinutes should be forwarded")
assert.Equal(t, "claude", c.Engine, "engine should be forwarded")
assert.Equal(t, 50, c.Count)@copilot please address this. |
||
| c := buildContinuationIfNeeded(runs, true, false, continuationOptions{ | ||
| workflowName: "my-workflow", | ||
| startDate: "2026-06-01", | ||
| endDate: "", | ||
| engine: "claude", | ||
| branch: "", | ||
| afterRunID: 0, | ||
| count: 50, | ||
| timeoutMinutes: 10, | ||
| }) | ||
| require.NotNil(t, c, "expected continuation when timeoutReached=true") | ||
| assert.Equal(t, int64(2999), c.BeforeRunID) | ||
| assert.Contains(t, c.Message, "Timeout reached") | ||
| }) | ||
|
|
||
| t.Run("neither flag set returns nil", func(t *testing.T) { | ||
| c := buildContinuationIfNeeded(runs, false, false, continuationOptions{ | ||
| workflowName: "my-workflow", | ||
| startDate: "2026-06-01", | ||
| endDate: "", | ||
| engine: "claude", | ||
| branch: "", | ||
| afterRunID: 0, | ||
| count: 100, | ||
| timeoutMinutes: 3, | ||
| }) | ||
| assert.Nil(t, c, "expected nil when neither timeout nor count limit was reached") | ||
| }) | ||
|
|
||
| t.Run("empty processedRuns returns nil even when count limit reached", func(t *testing.T) { | ||
| c := buildContinuationIfNeeded(nil, false, true, continuationOptions{ | ||
| workflowName: "my-workflow", | ||
| startDate: "2026-06-01", | ||
| endDate: "", | ||
| engine: "claude", | ||
| branch: "", | ||
| afterRunID: 0, | ||
| count: 100, | ||
| timeoutMinutes: 3, | ||
| }) | ||
| assert.Nil(t, c, "expected nil when no runs were processed") | ||
| }) | ||
| } | ||
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.
The
continuationOptionsstruct is inserted between thebuildContinuationIfNeededgodoc comment block (lines 142–150) and the function declaration. In Go, a doc comment is attached to the immediately following declaration — so godoc will now associate the multi-line comment withcontinuationOptions, leavingbuildContinuationIfNeededundocumented.Fix: move the struct before the
// buildContinuationIfNeeded returns...comment, and optionally add its own doc comment:@copilot please address this.