Add add-wizard integration coverage for manifest bootstrap ordering#47517
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds end-to-end coverage for manifest bootstrap ordering in add-wizard.
Changes:
- Adds a local manifest fixture with early and late bootstrap actions.
- Adds a fake
ghCLI and PTY-backed interactive test. - Improves authentication handling for existing integration coverage.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/add_wizard_tuistory_integration_test.go |
Tests manifest prompts before engine selection. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Medium
| output, err := runTuistory(t, "-s", sessionName, "wait", text, "--timeout", fmt.Sprintf("%d", timeoutMs)) | ||
| if err != nil && strings.Contains(output, "requires an interactive terminal") { | ||
| t.Skipf("tuistory session is not interactive in this environment: %s", output) | ||
| } | ||
| require.NoError(t, err, "Expected tuistory to find %q. Output: %s", text, output) |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ 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.
REQUEST_CHANGES — Four correctness bugs in the new PTY harness; all must be fixed before merge.
### Blocking issues (4)
-
donechannel consumed bywaitForText→close()always kills (line 259):doneis a 1-buffered send channel. AfterwaitForTextdrains it,close()blocks until the 2-second fallback fires and callsKill()unconditionally. Replace with a closedchan struct{}+ mutex-protected exit-error field. -
Race between
cmd.Wait()andio.Copy— final output missed (line 229):cmd.Wait()can return while the PTY master buffer still holds unread bytes.readAll()called immediately after may miss the last lines, causing flaky"process exited before output contained X"failures. -
io.Copygoroutine never joined — data race under-race(line 207):close()closes the PTY but does not wait for the goroutine to exit. The goroutine can still write tosession.outputafter the test returns, which is a data race. -
Kill()without subsequentWait()— zombie process (line 262): SIGKILL is sent but exit status is never reaped, leaving a zombie on Linux and a leakedcmd.Wait()goroutine.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 37.8 AIC · ⌖ 5.13 AIC · ⊞ 5.7K
Comment /review to run again
|
|
||
| go func() { | ||
| _, _ = io.Copy(&session.output, ptmx) | ||
| }() |
There was a problem hiding this comment.
done channel is consumed by waitForText, so close() always hits the 2-second kill timeout.
💡 Details and suggested fix
done is a buffered channel of size 1. Any call to waitForText that receives from s.done drains the channel; when close() subsequently selects on case <-s.done it blocks until the 2-second fallback fires and calls Kill() — even when the process already exited cleanly. On slow CI this adds a guaranteed 2-second penalty per test and spuriously kills a process that may already be gone.
Fix: close a sentinel chan struct{} instead of sending to it, and store the exit error in a mutex-protected field:
type interactivePTYSession struct {
cmd *exec.Cmd
ptmx *os.File
output lockedBuffer
done chan struct{} // closed on exit, never sent to
exitMu sync.Mutex
exitErr error
}
// goroutine:
go func() {
err := cmd.Wait()
s.exitMu.Lock()
s.exitErr = err
s.exitMu.Unlock()
close(s.done)
}()Both waitForText and close() can then observe exit independently via <-s.done without consuming each other's value.
| func (b *lockedBuffer) Write(p []byte) (int, error) { | ||
| b.mu.Lock() | ||
| defer b.mu.Unlock() | ||
| return b.b.Write(p) |
There was a problem hiding this comment.
Race condition: cmd.Wait() completes before io.Copy has drained the PTY buffer — final output lines can be missed.
💡 Details and suggested fix
When the child process exits, cmd.Wait() sends to s.done. waitForText immediately calls readAll() on receipt. However, the PTY master-side buffer can still contain bytes that the io.Copy goroutine has not yet written into session.output — there is no happens-before guarantee between cmd.Wait() returning and io.Copy finishing. This means waitForText can spuriously return "process exited before output contained X" even when X was actually emitted as one of the final lines.
Fix: drain the PTY after cmd.Wait() returns before signalling completion:
go func() {
err := cmd.Wait()
// Close ptmx to unblock io.Copy, then wait for it to drain
_ = ptmx.Close()
copyDone.Wait() // WaitGroup decremented in the io.Copy goroutine
s.exitMu.Lock()
s.exitErr = err
s.exitMu.Unlock()
close(s.done)
}()This ensures readAll() sees the complete output when s.done is closed.
| return &addWizardManifestTuistorySetup{ | ||
| tempDir: tempDir, | ||
| binaryPath: globalBinaryPath, | ||
| packagePath: packagePath, |
There was a problem hiding this comment.
io.Copy goroutine is never joined — goroutine leak and data race under -race.
💡 Details and suggested fix
close() calls ptmx.Close() which eventually causes io.Copy to return an error and exit, but there is no synchronisation point that waits for the goroutine to finish. The test function can return while the goroutine is still writing to session.output, which is a data race detectable with go test -race. Additionally, the goroutine outlives the test, so if the test process re-uses allocations, memory-safety guarantees break.
Fix with a sync.WaitGroup:
var wg sync.WaitGroup
wg.Add(1)
go func() {
defer wg.Done()
_, _ = io.Copy(&session.output, ptmx)
}()
// in close():
_ = s.ptmx.Close()
wg.Wait() // guarantee goroutine has finished before returning| }() | ||
| go func() { | ||
| session.done <- cmd.Wait() | ||
| }() |
There was a problem hiding this comment.
Kill() without a subsequent Wait() — zombie process and goroutine leak.
💡 Details and suggested fix
When the 2-second timeout fires, close() calls s.cmd.Process.Kill() but never calls cmd.Wait(). On Linux this leaves the child as a zombie (SIGKILL was delivered but the exit status was never reaped). The cmd.Wait() goroutine is still blocking in the background — it will eventually be unblocked by the OS, but there is no guarantee it returns before the test binary exits, constituting a goroutine leak.
Fix: drain s.done (with an additional short timeout) after killing:
case <-time.After(2 * time.Second):
_ = s.cmd.Process.Kill()
select {
case <-s.done: // reap
case <-time.After(500 * time.Millisecond):
}
🧪 Test Quality Sentinel Report✅ Test Quality Score: 88/100 — Excellent
📊 Metrics (1 test)
Test Classification
Verdict
Test Quality Highlights:
Token Budget: Analyzed 1 test function. Full file diff inspected.
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (290 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
|
There was a problem hiding this comment.
Review: Add-wizard manifest bootstrap integration test
Three blocking issues found in the new PTY-backed test harness.
Issues:
-
Line 188 — Bash
caseglob bug (blocking): The?in?per_page=100is a bash shell wildcard, not a literal?. Additionally, realgh apicalls from bootstrap code pass query params via--fieldflags — the path argument won't contain?per_page=100— so this arm will never match, causing the fakeghto exit 1 and hard-fail the test. -
Line 248 — PTY failure hard-fails instead of skipping (blocking): If PTY allocation fails in headless CI,
require.NoErrormarks the test as FAIL rather than SKIP. -
Line 279 —
donechannel drained bywaitForText, breakingclose()(blocking): OncewaitForTextconsumes the exit error from the buffered channel,close()can never receive from it and always hits the 2-second kill timeout.
See inline comments for fix suggestions.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 64.1 AIC · ⌖ 4.85 AIC · ⊞ 5K
| echo '{"permission":"write"}' | ||
| exit 0 | ||
| ;; | ||
| /repos/octo/example/actions/variables?per_page=100|/repos/octo/example/actions/secrets?per_page=100|/repos/octo/example/actions/secrets|/orgs/octo/actions/secrets) |
There was a problem hiding this comment.
Bug: ? in bash case pattern is a glob wildcard, not a literal ?
The pattern /repos/octo/example/actions/variables?per_page=100 uses ? as a shell glob (matches any single character). Worse, the real gh api invocations from bootstrap code typically pass query params via --field flags — the path argument won't contain ?per_page=100 at all — so this arm will never match and the fake gh will exit 1 with unexpected gh invocation, hard-failing the test.
Fix by matching the path prefix with a trailing *:
/repos/octo/example/actions/variables*|/repos/octo/example/actions/secrets*|/orgs/octo/actions/secrets*)@copilot please address this.
| func startInteractivePTYSession(t *testing.T, cmd *exec.Cmd) *interactivePTYSession { | ||
| t.Helper() | ||
|
|
||
| ptmx, err := pty.StartWithSize(cmd, &pty.Winsize{Cols: 140, Rows: 40}) |
There was a problem hiding this comment.
PTY failure hard-fails the test instead of skipping
startInteractivePTYSession uses require.NoError on the pty.StartWithSize call. In a headless CI environment where PTY allocation fails (e.g., no controlling terminal), this will mark the test as failed rather than skipped — masking the real unavailability.
Consider converting to a skip similar to how the tuistory path handles this:
ptmx, err := pty.StartWithSize(cmd, &pty.Winsize{Cols: 140, Rows: 40})
if err != nil {
t.Skipf("PTY not available in this environment: %v", err)
}@copilot please address this.
| } | ||
|
|
||
| select { | ||
| case err := <-s.done: |
There was a problem hiding this comment.
Bug: done channel is drained by waitForText, causing close() to always time out
When the process exits during waitForText, the error is consumed from s.done (line 279). But close() also selects on s.done (line 311). Since done is a buffered channel of size 1 and was already drained, close() will never receive and will always hit the 2-second timeout, then kill the process (which may already be dead), leaking resources and adding 2 seconds to every test run that exercises this path.
Fix with a sync.Once or by storing the exit error after the first read:
type interactivePTYSession struct {
// ...
exitErr error
exitOnce sync.Once
}
func (s *interactivePTYSession) waitDone() error {
s.exitOnce.Do(func() { s.exitErr = <-s.done })
return s.exitErr
}Then use s.waitDone() in both waitForText and close().
@copilot please address this.
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — requesting changes on four correctness and reliability issues in the new PTY harness.
📋 Key Themes & Highlights
Issues
- Done channel drain (line 279) —
doneis a buffered send-once channel; a secondwaitForTextcall after process exit will block forever. Useclose(done)as a broadcast instead. - Goroutine lifetime (line 258) — the
io.Copygoroutine has no completion signal, creating a potential race between the finalreadAll()assertion and the goroutine's last write. Add acopyDonechannel and drain it inclose. - Temp dir cleanup (line 396) — bare
defer os.RemoveAllwon't run after arequirefailure. Replace witht.Cleanup. - Empty stub responses (line 188) — the fake
ghscript returns no JSON body for several API paths. This silently degrades the test rather than surfacing a real parser failure.
Positive Highlights
- ✅ Clean separation: PTY session harness, fixture setup, and assertions are well-factored
- ✅ Thread-safe
lockedBufferavoids data races on the output buffer - ✅ Good use of
--no-secretand environment var overrides to isolate the wizard from the CI environment - ✅ Ordering assertion via index comparison is more precise than just checking presence
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 49.2 AIC · ⌖ 5.11 AIC · ⊞ 6.7K
Comment /matt to run again
| } | ||
|
|
||
| select { | ||
| case err := <-s.done: |
There was a problem hiding this comment.
[/tdd] The done channel is drained here but never refilled — a second call to waitForText after the process has exited will block indefinitely at the channel select instead of falling through to the timeout.
💡 Suggested fix: close the channel instead of sending
Use a closed channel as a broadcast signal so multiple waitForText calls all see the exit:
// in goroutine:
go func() {
s.exitErr = cmd.Wait()
close(s.done) // broadcast to all waiters
}()
// in waitForText select:
case <-s.done: // non-destructive read on closed channelThis avoids the send-once / receive-once pitfall of buffered channels.
@copilot please address this.
| } | ||
|
|
||
| go func() { | ||
| _, _ = io.Copy(&session.output, ptmx) |
There was a problem hiding this comment.
[/tdd] The goroutine copying PTY output has no clean shutdown path — when close(t) closes the PTY, io.Copy returns an error that's silently discarded. If the test panics or the PTY closes mid-read, the goroutine may linger and race against buffer reads.
💡 Suggested improvement
Route the copy goroutine's done signal back to the session so close can wait for it:
copyDone := make(chan struct{})
go func() {
defer close(copyDone)
_, _ = io.Copy(&session.output, ptmx)
}()
// store copyDone in session, wait in close()This prevents data races in the final readAll() assertion after close().
@copilot please address this.
|
|
||
| func TestTuistoryAddWizardManifestBootstrapRunsBeforeEngineSelection(t *testing.T) { | ||
| setup := setupAddWizardManifestTuistoryTest(t) | ||
| defer func() { |
There was a problem hiding this comment.
[/tdd] Using defer os.RemoveAll in the test body bypasses t.Cleanup, which means the temp directories won't be cleaned up if the test panics before the deferred function runs in a goroutine context. Prefer t.Cleanup to guarantee removal regardless of test exit path.
💡 Suggested replacement
setup := setupAddWizardManifestTuistoryTest(t)
t.Cleanup(func() {
_ = os.RemoveAll(setup.tempDir)
_ = os.RemoveAll(setup.fakeGHDir)
})t.Cleanup runs even on t.FailNow() / require failures.
@copilot please address this.
| echo '{"permission":"write"}' | ||
| exit 0 | ||
| ;; | ||
| /repos/octo/example/actions/variables?per_page=100|/repos/octo/example/actions/secrets?per_page=100|/repos/octo/example/actions/secrets|/orgs/octo/actions/secrets) |
There was a problem hiding this comment.
[/tdd] The fake gh script silently exits 0 for several API paths (/repos/.../actions/variables, secrets, etc.) without emitting any JSON — callers expecting a JSON body will get an empty response and may silently misbehave rather than failing loudly.
💡 Suggested fix
Return minimal valid JSON for each stubbed path so the wizard doesn't silently degrade:
/repos/octo/example/actions/variables?per_page=100)
echo '{"variables":[]}'
exit 0
;;
/repos/octo/example/actions/secrets?per_page=100)
echo '{"secrets":[]}'
exit 0
;;Silent empty responses mask real parsing bugs from reaching the test assertions.
@copilot please address this.
| require.NotEqual(t, -1, enginePromptIndex, "Expected to find engine selection prompt in session output") | ||
| assert.Less(t, earlyPromptIndex, enginePromptIndex, "Pre-install bootstrap prompt should appear before engine selection") | ||
|
|
||
| session.interrupt(t) |
There was a problem hiding this comment.
[/tdd] The test ends with session.interrupt(t) but never verifies that the process actually exited. If the Ctrl-C is swallowed (e.g. the wizard ignores SIGINT during a prompt), the deferred session.close will wait 2 seconds then kill — silently hiding the regression.
💡 Suggested addition
After the interrupt, wait for the process to exit cleanly:
session.interrupt(t)
select {
case <-session.done:
// clean exit
case <-time.After(5 * time.Second):
t.Error("process did not exit after Ctrl-C")
}This turns a cleanup timeout into an explicit test failure.
@copilot please address this.
|
🎉 This pull request is included in a new release. Release: |
PR #47462 changed
add-wizardto surfaceaw.ymlconfig before the generic engine setup flow, but that behavior was only covered by unit tests. This adds integration coverage that exercises the wizard end-to-end and verifies the bootstrap phase ordering at the terminal boundary.Scope
add_wizard_tuistory_integration_test.gowith coverage for local package manifests that declareconfigactions.ghauth is unavailable.Manifest bootstrap flow
aw.ymlpackage fixture with a pre-installrepo-variableaction and a post-installhandoffaction.Interactive harness
ghresponses with a minimal fake CLI to isolate the flow under test from external GitHub state.What this guards
The new integration path verifies that
Enter optional bootstrap variableis rendered beforeWhich coding agent would you like to use?, and that the handoff message is absent at that point.