Refactor overlong functions in pkg/console/progress.go and pkg/cli/mcp_tools_privileged.go#54705
Conversation
…ools_privileged.go) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…privileged.go Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Reviewed diff per ponytail-review skill: pure mechanical function-extraction refactor. Handler closures and long string literals were pulled into named top-level functions/consts, each with exactly one caller and unchanged control flow. No reinvented stdlib, no unneeded deps, no speculative abstractions, no dead flexibility introduced.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
There was a problem hiding this comment.
Pull request overview
Refactors the targeted largefunc backlog while preserving existing MCP and console behavior.
Changes:
- Splits progress rendering and logging into focused helpers.
- Extracts MCP descriptions, handlers, argument builders, and error-envelope helpers.
- Reuses the shared MCP subprocess context helper.
Show a summary per file
| File | Description |
|---|---|
pkg/console/progress.go |
Decomposes progress updates by rendering mode. |
pkg/cli/mcp_tools_privileged.go |
Decomposes logs and audit tool registration and handling. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
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.
Request changes
This refactor is not behavior-preserving: the audit MCP handler no longer rejects variant without experiment, so an invalid request now leaks through to subprocess execution instead of failing fast as an MCP parameter error.
Blocking theme
- Validation regressed in
pkg/cli/mcp_tools_privileged.go:buildAuditCommandArgsappends--variantunconditionally, but the handler no longer enforces the documentedvariant requires experimentcontract.
I did not find any other clearly blocking issue in the touched progress-bar extraction.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 14.6 AIC · ⌖ 6.89 AIC · ⊞ 7K
Comment /review to run again
| } | ||
|
|
||
| cmdArgs = appendRepoFlagFromEnv(cmdArgs) | ||
| cmdArgs := buildAuditCommandArgs(args, runItems) |
There was a problem hiding this comment.
This refactor dropped validation for variant without experiment, so the MCP tool now forwards an invalid flag combination to gh aw audit and turns what used to be a deterministic parameter error into a later subprocess failure.
💡 Why this matters and how to fix it
The tool description still says variant requires experiment, but newAuditToolHandler now only calls resolveAuditRunItems before building args. buildAuditCommandArgs appends --variant unconditionally, and there is no remaining guard that rejects variant by itself.
That is a correctness regression in the validation layer: callers now get a much weaker error path, and any client relying on MCP-level InvalidParams handling will break.
Add the explicit validation back before spawning the subprocess, e.g.
if args.Variant != "" && args.Experiment == "" {
return nil, nil, newMCPError(jsonrpc.CodeInvalidParams, "variant requires experiment", nil)
}Keeping this check in the handler preserves the previous contract and avoids wasting time on a doomed CLI invocation.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — commenting (no blocking issues).
📋 Key Themes & Highlights
Key Themes
buildAuditErrorResultsignature asymmetry:outputStris accepted but not included in the error envelope, unlike the parallelbuildLogsCommandError. Suggest either dropping it or making the two helpers consistent.- New testability opportunity: The extracted helpers (
appendLogsFilterArgs,resolveAuditRunItems) are now independently testable; adding narrow unit tests would harden the refactor.
Positive Highlights
- ✅ Pure mechanical extraction — no behaviour changes, all logic moved verbatim
- ✅ Re-uses
newMCPSubprocessContextinstead of duplicating the gateway-deadline-detach pattern — good deepening - ✅ Constants for tool description strings is the right move; avoids off-by-one line-count issues with
largefunclinting - ✅
progress.gosplits are clean and the function names (logUpdate,renderIndeterminate,renderDeterminate) exactly match the three branches — easy to navigate
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 36.7 AIC · ⌖ 10.9 AIC · ⊞ 7.6K
Comment /matt to run again
| if args.Runtime != "" { | ||
| cmdArgs = append(cmdArgs, "--runtime", args.Runtime) | ||
| } | ||
| return appendRepoFlagFromEnv(cmdArgs) |
There was a problem hiding this comment.
[/codebase-design] outputStr is passed to buildAuditErrorResult but is only used for logging its length — it's never included in the error envelope, unlike buildLogsCommandError which adds "stdout": outputStr to errorData. The asymmetry is subtle and now more visible since the function is extracted.
💡 Suggestion
If audit stdout is genuinely not useful in the error envelope, document that clearly at the call site or drop the parameter:
if execErr != nil {
mcpLog.Printf("... stdout: %d bytes ...", len(stdout))
return buildAuditErrorResult(execErr, runItems)
}If it may be useful (e.g. partial JSON on failure), add "stdout": outputStr consistent with buildLogsCommandError.
@copilot please address this.
| cmdArgs = append(cmdArgs, "--runtime", args.Runtime) | ||
| } | ||
| if args.Firewall { | ||
| cmdArgs = append(cmdArgs, "--firewall") |
There was a problem hiding this comment.
[/tdd] The extracted helpers — especially appendLogsFilterArgs (flag-to-arg mapping) and resolveAuditRunItems (precedence/fallback logic) — are now top-level package functions and directly unit-testable without a full MCP harness. Adding narrow table-driven tests for these would lock in the refactored behaviour and prevent future regressions.
@copilot please address this.
There was a problem hiding this comment.
Clean, well-executed refactor. The extracted helpers (buildLogsCommandArgs, buildLogsEmptyResult, buildLogsCommandError, resolveAuditRunItems, buildAuditCommandArgs, buildAuditErrorResult, buildAuditDiffErrorResult, and the new*ToolHandler constructors in both files) are faithful extractions with no behavioral changes.
All return values (effectiveCount, timeoutValue, cmdArgs) from buildLogsCommandArgs are correctly threaded back into the caller for logging and subprocess timeout. LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 42.3 AIC · ⌖ 8.97 AIC · ⊞ 6.2K
Documents the decision to decompose overlong registration functions and handler closures in pkg/cli/mcp_tools_privileged.go and pkg/console/progress.go to satisfy the largefunc (max-lines=60) linter. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@copilot this PR still needs forward progress before maintainer review.
Run: https://github.com/github/gh-aw/actions/runs/32552086004
|
…04-958f-35c042510885
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Fixed in |
|
@copilot this PR still needs forward progress before maintainer review.
Run: https://github.com/github/gh-aw/actions/runs/32553383419
|
…04-958f-35c042510885
Current head |
PR Triage
|
One slice of the
largefunc(max-lines=60) refactoring backlog: the lonepkg/consoleoverflow plus the 6 findings clustered inpkg/cli/mcp_tools_privileged.go. No public API, MCP tool schema, or behavior changes.pkg/console/progress.goUpdate(61 lines) split intologUpdate,renderIndeterminate, andrenderDeterminate, matching the three branches the function already had.pkg/cli/mcp_tools_privileged.gologsToolDescription,auditToolDescription).largefunccounts a function's full source span, so these ~40-line literals dominated the registration functions' budgets.newLogsToolHandler,newAuditToolHandler,newAuditDiffToolHandler, collapsing eachregisterXToolto schema generation plus anmcp.AddToolcall.buildLogsCommandArgs,appendLogsFilterArgs,buildAuditCommandArgs,resolveAuditRunItems) and error/empty envelopes (buildLogsEmptyResult,buildLogsCommandError,buildAuditErrorResult,buildAuditDiffErrorResult).newMCPSubprocessContexthelper already used by the audit tools — same semantics, ~35 fewer lines.The custom linters report no findings of any kind in the two touched files; the repo-wide
make golint-customstill fails on the remaining backlog outside this slice.Also reverts unrelated
GH_AW_HARNESS_VERSIONdrift in threesmoke-copilot*.lock.ymlfiles that the environment regenerated, keeping the diff scoped to the two Go files.Run: https://github.com/github/gh-aw/actions/runs/32552086004> Generated by 👨🍳 PR Sous Chef · gpt54 · 28.1 AIC · ⌖ 8.19 AIC · ⊞ 9.5K · ◷