Refactor add interactive workflow run prompt - #51517
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Hey Since this is still in WIP status with no file changes yet, here are a few things to keep in mind as you continue:
The narrow scope and step-by-step checklist approach look solid. Looking forward to seeing the implementation!
|
PR Triage
Small draft refactor, low urgency.
|
There was a problem hiding this comment.
Pull request overview
Refactors interactive workflow-run orchestration into focused helpers while preserving behavior.
Changes:
- Extracts status polling, prompting, branch updates, and run URL display.
- Adds focused coverage for the workflow-dispatch gate.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/add_interactive_workflow.go |
Splits the workflow-run flow into smaller helpers. |
pkg/cli/add_interactive_workflow_test.go |
Tests run-offer eligibility. |
Review details
Tip
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
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Reviewed the refactor of add_interactive_workflow.go: each extracted helper (waitForWorkflowStatus, checkWorkflowStatusAttempt, showWorkflowStatusUnavailableInstructions, shouldOfferAddedWorkflowRun, showCodespaceRunInstructions, confirmRunAddedWorkflow, runAddedWorkflowOnce, updateLocalBranchBeforeWorkflowRun, showWorkflowRunURL) is a single-caller split required to bring checkStatusAndOfferRun under the function-length lint limit, not speculative abstraction. No dead code, reinvented stdlib, unneeded deps, or config-nobody-sets flexibility introduced. Added test targets the new testable gate function directly. No over-engineering findings to report.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Refactor add interactive workflow run prompt
Clean refactoring that improves readability and testability by extracting cohesive sub-functions from the monolithic checkStatusAndOfferRun. Behavior is preserved and the new unit test for shouldOfferAddedWorkflowRun adds good coverage.
One minor nit (non-blocking): in waitForWorkflowStatus, the intermediate found variable is redundant — if c.checkWorkflowStatusAttempt(i) { workflowFound = true; break } would be cleaner. No blocking issues.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 17.3 AIC · ⌖ 7.12 AIC · ⊞ 5.5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — commenting, not requesting changes, as these are refinements rather than blockers.
📋 Key Themes & Highlights
Key Themes
- Silent error swallow (line 51):
confirmRunAddedWorkflownow explicitly returns errors, but the caller still discards the error unconditionally. This is a correctness gap for context-cancellation propagation. - Hidden side-effect coupling (line 142):
showWorkflowStatusUnavailableInstructionssecretly callsshowFinalInstructions, making it structurally inconsistent with every other branch incheckStatusAndOfferRun. - Underutilised seams (test file): The refactor created independently-callable helpers —
checkWorkflowStatusAttempt,confirmRunAddedWorkflow,runAddedWorkflowOnce— that have no new tests despite being newly testable in isolation.
Positive Highlights
- ✅ The top-level orchestration in
checkStatusAndOfferRunis now clear and easy to follow - ✅
shouldOfferAddedWorkflowRunis a clean boolean predicate with good table-driven tests - ✅
confirmRunAddedWorkflowcorrectly returns errors rather than swallowing — the call-site should match this intent
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 32.2 AIC · ⌖ 7.85 AIC · ⊞ 7.1K
Comment /matt to run again
|
|
||
| runNow, err := confirmRunAddedWorkflow(ctx) | ||
| if err != nil { | ||
| return nil // Not critical, just skip |
There was a problem hiding this comment.
[/codebase-design] Error from confirmRunAddedWorkflow is silently swallowed — this contradicts the refactored function which explicitly returns errors via return false, err.
Before the refactor, the original swallow was understandable. Now that confirmRunAddedWorkflow surfaces errors properly, context-cancellation errors will be discarded instead of propagated.
💡 Suggestion
Distinguish user-cancel from unexpected errors:
runNow, err := confirmRunAddedWorkflow(ctx)
if err != nil {
if errors.Is(err, huh.ErrUserAborted) {
c.showFinalInstructions()
return nil
}
return err // propagate context cancellation etc.
}@copilot please address this.
| func (c *AddInteractiveConfig) showWorkflowStatusUnavailableInstructions() { | ||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Could not verify workflow status.")) | ||
| fmt.Fprintf(os.Stderr, "You can check status with: %s status\n", string(constants.CLIExtensionPrefix)) | ||
| c.showFinalInstructions() |
There was a problem hiding this comment.
[/codebase-design] showWorkflowStatusUnavailableInstructions calls c.showFinalInstructions() internally, but its caller does not call it again — this works correctly, but it creates a subtle coupling: the method has a side effect (calling another method) that violates the single-responsibility principle and makes the name misleading.
💡 Suggestion
Remove showFinalInstructions from inside showWorkflowStatusUnavailableInstructions and let the top-level flow call it explicitly (as is done in all other branches):
func (c *AddInteractiveConfig) showWorkflowStatusUnavailableInstructions() {
fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Could not verify workflow status."))
fmt.Fprintf(os.Stderr, "You can check status with: %s status
", string(constants.CLIExtensionPrefix))
// caller is responsible for showFinalInstructions
}Then at the call site (line ~29):
c.showWorkflowStatusUnavailableInstructions()
c.showFinalInstructions()
return nilThis makes all branches in checkStatusAndOfferRun structurally parallel.
@copilot please address this.
| @@ -75,3 +75,41 @@ func TestCheckStatusAndOfferRun_ContextCancelled(t *testing.T) { | |||
| t.Fatalf("expected context cancellation error, got %v", err) | |||
| } | |||
There was a problem hiding this comment.
[/tdd] The new test only covers shouldOfferAddedWorkflowRun — the simplest extracted method. The more complex extracted helpers (waitForWorkflowStatus, checkWorkflowStatusAttempt, confirmRunAddedWorkflow) have no new tests despite being independently callable now.
The refactor created clean seams; the tests don't yet exploit them.
💡 Suggested coverage additions
checkWorkflowStatusAttempt: test the verbose/non-verbose branching and the error path whenfindWorkflowsByFilenamePatternreturns an error.confirmRunAddedWorkflow: test context cancellation propagation (was previously un-testable inside the monolith).runAddedWorkflowOnce: test early-return whenprimaryWorkflowName()is empty.
These are now independently callable and mockable — this is exactly what the refactor enables.
@copilot please address this.
🧪 Test Quality Sentinel Report
📊 Metrics (3 tests)
|
… workflow Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (182 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
|
|
@copilot Quick triage nudge for this PR.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. Addressed all three open review comments in the latest commit:
|
The function-length backlog includes long CLI functions that need surgical helper extraction without behavior changes. This slice targets
pkg/cli/add_interactive_workflow.go.Split workflow status/run orchestration
checkStatusAndOfferRun.Isolated user-facing branches
Added focused coverage
Run: https://github.com/github/gh-aw/actions/runs/31310371321> Generated by 👨🍳 PR Sous Chef · gpt54 · 4.98 AIC · ⌖ 5.48 AIC · ⊞ 6.1K · ◷