Targeted custom-lint cleanup: defer-in-loop resource handling and continuation options struct#43414
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Hey
If you'd like a hand cleaning this up, you can assign this prompt to your coding agent:
|
|
❌ Design Decision Gate 🏗️ failed during design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
This PR targets a small set of mechanical lint fixes in pkg/cli (resource cleanup in a polling loop and reducing an over-parameterized helper) and updates related unit tests. It also includes a large re-sync/reformat of embedded action pin JSON data.
Changes:
- Refactors MCP scripts server readiness polling by extracting request/response handling into
isServerReady(...)to avoiddeferin a loop. - Introduces a
continuationOptionsstruct to replace an 11-parameterbuildContinuationIfNeeded(...)signature, and updates unit tests accordingly. - Re-syncs/reformats
action_pins.jsonembedded data files used for action/container pinning.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/mcp_inspect_mcp_scripts_server.go | Extracts response handling into a helper to avoid loop-scoped defers during readiness polling. |
| pkg/cli/logs_orchestrator.go | Introduces continuationOptions to reduce positional parameter coupling in continuation building. |
| pkg/cli/logs_orchestrator_unit_test.go | Updates unit tests to construct and pass continuationOptions. |
| pkg/actionpins/data/action_pins.json | Large action pin JSON re-sync/reformat (embedded fallback pin table). |
| pkg/workflow/data/action_pins.json | Large action pin JSON re-sync/reformat (compiler-side embedded pin table). |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/5 changed files
- Comments generated: 3
- Review effort level: Low
| if verbose { | ||
| mcpInspectLog.Printf("Server is ready on port %d", port) | ||
| } | ||
| if isServerReady(client, req, port, verbose) { |
| { | ||
| "entries": { | ||
| "actions-ecosystem/action-add-labels@v1.1.3": { | ||
| "repo": "actions-ecosystem/action-add-labels", | ||
| "version": "v1.1.3", | ||
| "sha": "c96b68fec76a0987cd93957189e9abd0b9a72ff1", | ||
| "inputs": { | ||
| "github_token": { | ||
| "description": "A GitHub token.", | ||
| "default": "${{ github.token }}" | ||
| }, | ||
| "labels": { | ||
| "description": "The labels' name to be added. Must be separated with line breaks if there're multiple labels.", | ||
| "required": true | ||
| }, | ||
| "number": { | ||
| "description": "The number of the issue or pull request." | ||
| }, | ||
| "repo": { | ||
| "description": "The owner and repository name. e.g.) Codertocat/Hello-World", | ||
| "default": "${{ github.repository }}" | ||
| "entries": { | ||
| "actions-ecosystem/action-add-labels@v1.1.3": { |
| { | ||
| "entries": { | ||
| "actions-ecosystem/action-add-labels@v1.1.3": { | ||
| "repo": "actions-ecosystem/action-add-labels", | ||
| "version": "v1.1.3", | ||
| "sha": "c96b68fec76a0987cd93957189e9abd0b9a72ff1", | ||
| "inputs": { | ||
| "github_token": { | ||
| "description": "A GitHub token.", | ||
| "default": "${{ github.token }}" | ||
| }, | ||
| "labels": { | ||
| "description": "The labels' name to be added. Must be separated with line breaks if there're multiple labels.", | ||
| "required": true | ||
| }, | ||
| "number": { | ||
| "description": "The number of the issue or pull request." | ||
| }, | ||
| "repo": { | ||
| "description": "The owner and repository name. e.g.) Codertocat/Hello-World", | ||
| "default": "${{ github.repository }}" | ||
| "entries": { | ||
| "actions-ecosystem/action-add-labels@v1.1.3": { |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (4 tests)
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
Review: Targeted custom-lint cleanup
Overall: Both refactors achieve their stated goals correctly — the defer-in-loop fix and the options-struct conversion are clean and behavior-preserving. One doc placement issue needs a fix before merge.
✅ mcp_inspect_mcp_scripts_server.go — isServerReady correctly isolates defer resp.Body.Close() inside the helper, eliminating the loop-scoped accumulation. The original req is consumed once per iteration since a new req is constructed in the loop, so passing it through is safe.
**✅ logs_orchestrator.go / test file** — continuationOptions` struct cleanly reduces an 11-parameter signature. All field mapping is 1-to-1 and correct. Tests fully cover the updated call sites.
continuationOptions type declaration is inserted between the existing // buildContinuationIfNeeded returns... doc comment block and the function declaration. Go's godoc rules attach a comment to the immediately following top-level declaration, so the comment will now document continuationOptions, not buildContinuationIfNeeded. See the inline comment for the one-line fix.
i️ JSON formatting noise — The two action_pins.json files changed only indentation (2→4 spaces). This is mechanically correct but adds 1500+ lines of churn to the diff. No action needed — just worth noting for future PR scoping.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 47.9 AIC · ⌖ 5.15 AIC · ⊞ 4.9K
| // - 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 { |
There was a problem hiding this comment.
The continuationOptions struct is inserted between the buildContinuationIfNeeded godoc 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 with continuationOptions, leaving buildContinuationIfNeeded undocumented.
Fix: move the struct before the // buildContinuationIfNeeded returns... comment, and optionally add its own doc comment:
// continuationOptions holds the pagination parameters forwarded to the
// next fetch batch when a timeout or count limit interrupts the current one.
type continuationOptions struct { ... }
// buildContinuationIfNeeded returns a ContinuationData cursor...
func buildContinuationIfNeeded(...) *ContinuationData {@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — no blocking issues, but a few targeted improvements worth addressing.
📋 Key Themes & Highlights
Key Themes
- Test coverage gaps:
afterRunID,timeoutMinutes, andenginepropagation are not asserted in the updated tests; the newisServerReadyhelper has no isolated test. - Struct design clarity:
continuationOptionsis placed inside thebuildContinuationIfNeededdoc-comment block and lacks its own doc-comment; the struct also closely mirrorsContinuationData, which warrants a brief explanation. - Interface depth:
isServerReady's signature exposes a per-call*http.Requestrather than encapsulating URL/context internally — a minor but real leaky abstraction.
Positive Highlights
- ✅ defer-in-loop fix is correct — extracting
isServerReadyeliminates the accumulation problem cleanly. - ✅ Options struct is a clear improvement over 11 positional parameters; named fields make call sites self-documenting.
- ✅ All four existing tests were updated to use the new struct API — no regressions introduced.
- ✅ Existing
TestWaitForServerReady_*tests continue to cover context-cancellation and timeout paths.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 78.6 AIC · ⌖ 5.79 AIC · ⊞ 6.6K
Comment /matt to run again
| // - 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 { |
There was a problem hiding this comment.
[/codebase-design] continuationOptions is placed inside the doc-comment block for buildContinuationIfNeeded, visually breaking the comment-to-function association. Moving it above line 142 (before the doc-comment) with its own short comment keeps Go conventions and makes navigation easier.
💡 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.
| timeoutMinutes int | ||
| } | ||
|
|
||
| func buildContinuationIfNeeded( |
There was a problem hiding this comment.
[/codebase-design] continuationOptions mirrors 8 of ContinuationData's 9 fields — it's essentially a read-only projection of that struct. Consider whether callers should just build a partial ContinuationData directly (omitting BeforeRunID and Message which are computed), or document why a separate struct is intentionally preferred over a lighter approach.
💡 Analysis
ContinuationData fields: Message, WorkflowName, Count, StartDate, EndDate, Engine, Branch, AfterRunID, BeforeRunID, Timeout.
continuationOptions fields: workflowName, startDate, endDate, engine, branch, afterRunID, count, timeoutMinutes.
The only values not in continuationOptions are Message and BeforeRunID — both computed inside the function. If this function is only called from one place, the extra indirection adds minimal value; if it's called from multiple places with varied subsets, the struct is well-justified. A brief doc-comment on the struct explaining the rationale would help future readers.
@copilot please address this.
| endDate: "2026-06-30", | ||
| engine: "claude", | ||
| branch: "main", | ||
| afterRunID: 0, |
There was a problem hiding this comment.
[/tdd] afterRunID: 0 is explicitly set in every test case but is also the zero-value for int64, making it invisible noise. This is fine if a future test with a non-zero afterRunID is anticipated, but there is currently no assertion that c.AfterRunID equals the value passed in — meaning the field is untested. Consider adding an assertion for AfterRunID propagation (e.g., pass afterRunID: 42 and assert c.AfterRunID == 42) to lock in that contract.
💡 Suggested addition
t.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.
| @@ -157,26 +163,46 @@ func TestBuildContinuationIfNeeded(t *testing.T) { | |||
| }) | |||
|
|
|||
| t.Run("timeout reached emits cursor with timeout message", func(t *testing.T) { | |||
There was a problem hiding this comment.
[/tdd] The timeout test (line 165) doesn't assert c.Timeout or c.Engine — both are set in continuationOptions but not verified. With the new struct API it's easy to forget to assert a field; adding assertions for all propagated fields turns this test into a full contract spec.
💡 Suggested additions
assert.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.
|
|
||
| var errMCPScriptsServerStartupTimeout = errors.New("mcp-scripts HTTP server failed to start within timeout") | ||
|
|
||
| func isServerReady(client *http.Client, req *http.Request, port int, verbose bool) bool { |
There was a problem hiding this comment.
[/codebase-design] isServerReady takes a pre-built *http.Request as a parameter, but the request is created fresh on every loop iteration in waitForServerReady. Since http.Request objects can't be reused after client.Do, this is fine — but the signature design leaks an implementation detail of the caller. Consider whether accepting (client, url string) and creating the request internally would yield a simpler interface; the current design forces the caller to know the request-per-call constraint.
💡 Alternative signature
func isServerReady(ctx context.Context, client *http.Client, url string, port int, verbose bool) bool {
req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
if err != nil {
return false
}
resp, err := client.Do(req)
...
}This encapsulates request creation inside the helper and eliminates the per-call construction in the loop.
@copilot please address this.
| mcpInspectLog.Printf("Server is ready on port %d", port) | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
[/tdd] isServerReady is now a testable standalone function, but there are no unit tests for it. The existing tests cover waitForServerReady end-to-end (context cancellation, timeout) but don't exercise isServerReady in isolation — e.g., it always returns false when the server is unreachable, and returns true on a 200 response. Adding a small table-driven test with an httptest.Server would pin this contract.
💡 Suggested test skeleton
func TestIsServerReady(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusOK)
}))
defer srv.Close()
client := srv.Client()
req, _ := http.NewRequestWithContext(t.Context(), http.MethodGet, srv.URL, nil)
if !isServerReady(client, req, 0, false) {
t.Fatal("expected isServerReady to return true for a live server")
}
}@copilot please address this.
Documents the design decisions behind PR #43414: - continuationOptions struct to replace 11-positional-parameter signature - isServerReady helper to resolve defer-in-loop lint violation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
REQUEST_CHANGES — two issues must be fixed before merging.
🔍 Findings summary
Critical/High
isServerReadydoes not check HTTP status code (mcp_inspect_mcp_scripts_server.go): any HTTP response — including 4xx/5xx — is treated as server-ready. A misconfigured or faultedmcp-server.cjs(404, 500) will silently pass readiness polling and the MCP client will connect to a broken server.
Medium
- Doc comment misattribution (
logs_orchestrator.go): insertingtype continuationOptions structimmediately after the// buildContinuationIfNeeded ...comment causes Go's doc tooling to attach that comment to the type instead of the function. The function becomes undocumented.
Dropped
- Body-drain before
resp.Body.Close(): with a 1 s client timeout and a polling delay between iterations, connection-reuse loss is negligible here — not actionable. continuationOptionsrefactor is clean and correct; the struct/test changes are mechanical and well-executed.
🔎 Code quality review by PR Code Quality Reviewer · 99 AIC · ⌖ 5.22 AIC · ⊞ 5.4K
Comment /review to run again
| mcpInspectLog.Printf("Server is ready on port %d", port) | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
isServerReady returns true for any HTTP response, including 4xx/5xx — a server that starts up but returns 500 will be treated as ready, causing waitForServerReady to return nil and the caller to proceed against a broken server.
💡 Suggested fix
Check resp.StatusCode before declaring the server ready:
func isServerReady(client *http.Client, req *http.Request, port int, verbose bool) bool {
resp, err := client.Do(req)
if err != nil {
return false
}
defer resp.Body.Close()
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
return false
}
if verbose {
mcpInspectLog.Printf("Server is ready on port %d", port)
}
return true
}Without this check, a mcp-scripts server that boots but immediately faults (e.g. missing mcp-server.cjs, returns 404) will appear healthy and the caller will connect to a non-functional server.
| // - 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 { |
There was a problem hiding this comment.
continuationOptions type declaration breaks the buildContinuationIfNeeded doc comment — Go attaches a doc comment to the immediately following declaration, so the multi-line comment above now documents continuationOptions, not buildContinuationIfNeeded. The function becomes undocumented.
💡 Suggested fix
Move continuationOptions before the doc comment block, or add a blank line between the doc comment and the type, then place the type immediately before the function:
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 continuationOptions so both declarations are documented correctly.
|
@copilot revert action pins changes |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
/souschef |
|
✅ PR Sous Chef finished PR sous-chef nudges. processed=5; skipped_checks_running=1; skipped_last_comment_from_sous_chef=0; skipped_cooldown=2; nudged=2; branch_update_attempts=1; formatter_pushes=0; merge_main_scheduled=0 |
|
🎉 This pull request is included in a new release. Release: |
This PR addresses a focused slice of the targeted lint bucket by fixing two mechanical findings in
pkg/cli: deferred cleanup inside a polling loop and an excessive-parameter helper signature. Scope is intentionally narrow and aligned to the requested lint subgroup.### Replace defer-in-loop pattern in MCP server readiness polling
waitForServerReadyby introducingisServerReady(...).Body.Close()into a helper-local defer so cleanup remains immediate/safe without a loop-scoped defer accumulation pattern.### Reduce parameter count in continuation builder
continuationOptionsinlogs_orchestrator.go.buildContinuationIfNeeded(...)to accept the options struct instead of 11 positional parameters.### Align unit tests with new helper contract
logs_orchestrator_unit_test.gocall sites to constructcontinuationOptionsexplicitly.