From 986e1372af758989dfc64ab91647b14d6f715f74 Mon Sep 17 00:00:00 2001 From: Dennison Date: Thu, 30 Jul 2026 16:34:22 +0200 Subject: [PATCH] test: synchronize worktree containment cleanup (#1039) --- docs/logs/engineering-log.md | 18 ++++- docs/logs/long-term-thinking-log.md | 12 ++- ...1039-worktree-containment-ci-impact-map.md | 74 +++++++++++++++++++ ...issue-1039-worktree-containment-ci-plan.md | 57 ++++++++++++++ docs/plans/INDEX.md | 2 + internal/harness/workspace_selection_test.go | 55 +++++++++++--- 6 files changed, 206 insertions(+), 12 deletions(-) create mode 100644 docs/plans/2026-07-30-issue-1039-worktree-containment-ci-impact-map.md create mode 100644 docs/plans/2026-07-30-issue-1039-worktree-containment-ci-plan.md diff --git a/docs/logs/engineering-log.md b/docs/logs/engineering-log.md index a2b2afdb..b0f9ef16 100644 --- a/docs/logs/engineering-log.md +++ b/docs/logs/engineering-log.md @@ -71,7 +71,23 @@ - Result: the focused test passed normal/race at `-count=100`, the complete harness package passed normal/race, and `./scripts/test-regression.sh` passed with 85.6% total coverage and zero uncovered functions. - +## 2026-07-30 (Worktree Containment CI Synchronization — Issue #1039) + +- Symptom: GitHub Actions fast run 30551198514 received + `tool.call.completed` but found neither `out.txt` nor `marker.txt` in the + provisioned worktree. +- Cause: the test consumes a buffered event while the runner immediately starts + its terminal provider turn and can remove the worktree before the subscriber + performs filesystem assertions. +- Fix: hold only the test provider's terminal turn until containment + assertions finish, then release normal workspace cleanup. +- TDD evidence: a deterministic pre-subscription wait for terminal cleanup made + the existing assertion fail with both files missing. The final provider + handshake retains the same real bash command and exact filesystem checks. +- Verification: focused normal and race tests pass 100 consecutive runs each; + the full harness package passes in normal and race modes; and + `./scripts/test-regression.sh` passes normal, full race, and `coveragegate` + at 85.6% with zero uncovered functions. ## 2026-07-30 (Workflow Subscription Cancellation Test — Issue #1035) - Symptom: the full race gate failed in diff --git a/docs/logs/long-term-thinking-log.md b/docs/logs/long-term-thinking-log.md index 69ca09f2..c81c1801 100644 --- a/docs/logs/long-term-thinking-log.md +++ b/docs/logs/long-term-thinking-log.md @@ -63,7 +63,17 @@ - Outcome: a capacity-one handoff establishes the required happens-before edge while the provider still asserts the non-terminal step-two status; all local focused, package, race, and coverage gates are green. - +## 2026-07-30 (Worktree Containment CI Synchronization — Issue #1039) + +- Command intent: clear the red Linux fast gate exposed while merging the + reviewed cron/callback native conversation repairs. +- User intent: merge only when the accepted normal, race, native, API, TUI, and + GUI evidence is genuinely green rather than waiving an intermittent failure. +- Success definition: the real bash containment test inspects the provisioned + worktree before cleanup, retains every routing invariant, and passes focused + stress plus the full repository gate. +- Guardrails: issue-first isolated worktree, deterministic red evidence, + test-only synchronization, no production lifetime change, and no sleeps. ## 2026-07-30 (Workflow Subscription Cancellation Test — Issue #1035) - Command intent: clear the race-gate blocker exposed while verifying the diff --git a/docs/plans/2026-07-30-issue-1039-worktree-containment-ci-impact-map.md b/docs/plans/2026-07-30-issue-1039-worktree-containment-ci-impact-map.md new file mode 100644 index 00000000..b79134c8 --- /dev/null +++ b/docs/plans/2026-07-30-issue-1039-worktree-containment-ci-impact-map.md @@ -0,0 +1,74 @@ +# Cross-Surface Impact Map: Issue #1039 Worktree Containment CI + +## Task + +- Task / issue: synchronize containment assertions with workspace teardown, + #1039. +- Plan: `2026-07-30-issue-1039-worktree-containment-ci-plan.md`. +- Owner: Codex. +- Status: implemented and fully verified; merge pending. + +## Current Ownership, Callers, and Data Flow + +- Entry: `TestWorktreeContainment_ToolCwdIsWorktree`. +- Source of truth: runner provisions the worktree, the real bash tool writes + cwd-relative files, and terminal cleanup removes the worktree. +- Event consumer: the test subscribes through a buffered channel and examines + files after receiving `tool.call.completed`. +- Similar abstractions searched: `stubProvider.Complete`, runner subscription + tests, workspace destruction, and other provider release handshakes. +- Conclusion: the test owns the invalid consumer-timing assumption; production + routing and cleanup ownership remain unchanged. + +## Config, API, CLI, and Tools + +- Config/env/defaults: none; test-only channels. +- API/CLI/wire formats: none. +- Tools: retain the real registered bash tool and its existing command. +- Error states: retain explicit tool-error, missing-event, and timeout failures. + +## Persistence and Compatibility + +- Schemas, migrations, caches, generated data: none. +- Compatibility and mixed versions: none; no runtime change. + +## Lifecycle, Security, and Reliability + +- Concurrency/lifecycle: add a bounded test-only handshake before cleanup. +- Authentication/authorization/privacy/secrets: none. +- Recovery/idempotency: deferred release prevents deadlock after a failed + assertion; timeouts diagnose either side failing to progress. + +## Product and Integration Surfaces + +- Server/runtime: production unchanged. +- TUI/web/macOS/providers/models: none. +- External automation: GitHub Actions fast and race gates become deterministic. +- UX/accessibility: none. + +## Deployment and Operations + +- Deployment/migration/flags: none. +- Diagnostics: focused failure names the missing handshake or containment + invariant. +- Rollback: revert the test-only commit if it masks a deliberately broken cwd. + +## Regression Tests + +- Characterization/red: allow cleanup to win before event consumption and + observe missing files. +- Acceptance: release terminal completion only after containment assertions. +- Negative controls: daemon-cwd leak, wrong `pwd`, tool error, missing + provision/completion event, or never-released provider still fail. +- Commands: focused normal/race `-count=100`, harness package tests, and + `./scripts/test-regression.sh`. + +## Documentation and Handoff + +- No public docs. +- Update engineering log, long-term log, plan, impact map, and plans index. + +## Warning Check + +- All required surfaces are covered; runtime surfaces are explicitly unchanged + because the patch is confined to the regression fixture. diff --git a/docs/plans/2026-07-30-issue-1039-worktree-containment-ci-plan.md b/docs/plans/2026-07-30-issue-1039-worktree-containment-ci-plan.md new file mode 100644 index 00000000..ee610344 --- /dev/null +++ b/docs/plans/2026-07-30-issue-1039-worktree-containment-ci-plan.md @@ -0,0 +1,57 @@ +# Plan: Synchronize Worktree Containment Assertions with Cleanup + +## Context + +- Governing GitHub issue: #1039. +- Problem: the containment test consumes a buffered `tool.call.completed` + event after the runner may already have completed its next turn and removed + the worktree. +- User impact: a false-negative Linux fast gate blocks the cron/callback GUI + merge chain despite valid worktree routing. +- Constraint: keep the real bash tool and every containment assertion; do not + change production workspace lifetime to satisfy a test-only race. + +## Scope + +- In scope: add a test-provider handshake that holds the terminal provider + turn until the subscriber has inspected the live worktree. +- Out of scope: production runner, event, workspace, tool, API, and client + behavior. + +## Documentation Contract + +- Feature status: implemented; merge pending. +- Public docs affected: none; no user-facing contract changes. +- Implementation evidence: engineering and long-term logs, this plan, impact + map, and plans index. + +## Test Plan (TDD) + +- Red: deterministically let the terminal provider turn return before the + subscriber examines `tool.call.completed`; prove the worktree is absent. +- Green: block that terminal turn on a bounded test-only release, perform the + existing filesystem assertions, then release cleanup. +- Stress: focused normal/race tests at `-count=100`, harness package tests, and + the full repository regression gate. + +## Cross-Surface Impact Map + +- See `2026-07-30-issue-1039-worktree-containment-ci-impact-map.md`. + +## Implementation Checklist + +- [x] Create contract-complete bug #1039. +- [x] Record ownership and lifecycle search evidence. +- [x] Write plan and impact map before code. +- [x] Capture deterministic red evidence. +- [x] Implement the minimal test handshake. +- [x] Run focused, race, package, and full gates. +- [ ] Merge through a closing PR. + +## Risks and Mitigations + +- Risk: a blocking fixture could deadlock the test. +- Mitigation: both provider wait and subscriber wait have bounded timeouts. +- Risk: the repair could weaken the containment contract. +- Mitigation: retain real bash execution, exact resolved cwd, in-worktree file, + no daemon-cwd leak, and successful completion assertions. diff --git a/docs/plans/INDEX.md b/docs/plans/INDEX.md index 5a6e3e71..ed55ff18 100644 --- a/docs/plans/INDEX.md +++ b/docs/plans/INDEX.md @@ -8,6 +8,8 @@ - `2026-07-30-issue-1049-workflow-failure-timeout-impact-map.md` — Cross-surface impact map for Issue #1049. - `2026-07-30-issue-1044-ask-status-race-plan.md` — Issue #1044 planned synchronization of the AskUserQuestion status regression fixture. - `2026-07-30-issue-1044-ask-status-race-impact-map.md` — Cross-surface impact map for Issue #1044. +- `2026-07-30-issue-1039-worktree-containment-ci-plan.md` — Issue #1039 planned synchronization of live worktree containment assertions with terminal cleanup. +- `2026-07-30-issue-1039-worktree-containment-ci-impact-map.md` — Cross-surface impact map for Issue #1039. - `2026-07-30-issue-1035-workflow-subscription-cancel-plan.md` — Issue #1035 planned deterministic workflow subscription-cancellation regression repair. - `2026-07-30-issue-1035-workflow-subscription-cancel-impact-map.md` — Cross-surface impact map for Issue #1035. - `2026-07-30-issue-1026-feedback-direct-publish-plan.md` — Issue #1026 attached-image, publish-by-default GitHub feedback intake (implemented and verified; promotion pending). diff --git a/internal/harness/workspace_selection_test.go b/internal/harness/workspace_selection_test.go index 05cbea05..6fe584cd 100644 --- a/internal/harness/workspace_selection_test.go +++ b/internal/harness/workspace_selection_test.go @@ -867,25 +867,53 @@ func evalSymlinksWS(t *testing.T, path string) string { return resolved } -// worktreeContainmentProvider issues, on its first turn, a bash tool call that -// writes two files using paths RELATIVE to the tool's cwd: +// worktreeContainmentTestProvider issues, on its first turn, a bash tool call +// that writes two files using paths RELATIVE to the tool's cwd: // - `pwd > marker.txt` records the shell's working directory, and // - `echo hi > out.txt` writes a sentinel file. // // Because relative paths resolve against the bash tool's cwd, both files must // land inside the provisioned worktree (not the daemon startup cwd) if tool -// routing is wired to the workspace. The second turn terminates the run. -func worktreeContainmentProvider() *stubProvider { - return &stubProvider{turns: []CompletionResult{ - { +// routing is wired to the workspace. The second turn waits for the subscriber +// to inspect those files before terminating the run, so buffered event delivery +// cannot race workspace teardown. +type worktreeContainmentTestProvider struct { + mu sync.Mutex + calls int + releaseTerminal <-chan struct{} +} + +func (p *worktreeContainmentTestProvider) Complete( + ctx context.Context, _ CompletionRequest, +) (CompletionResult, error) { + p.mu.Lock() + call := p.calls + p.calls++ + p.mu.Unlock() + + switch call { + case 0: + return CompletionResult{ ToolCalls: []ToolCall{{ ID: "call-write-marker", Name: "bash", Arguments: `{"command":"pwd > marker.txt && echo hi > out.txt","timeout_seconds":30}`, }}, - }, - {Content: "done"}, - }} + }, nil + case 1: + select { + case <-p.releaseTerminal: + return CompletionResult{Content: "done"}, nil + case <-ctx.Done(): + return CompletionResult{}, ctx.Err() + case <-time.After(15 * time.Second): + return CompletionResult{}, errors.New( + "timed out waiting for containment assertions before workspace cleanup", + ) + } + default: + return CompletionResult{}, nil + } } // TestWorktreeContainment_ToolCwdIsWorktree (Deliverable A) proves that file/shell @@ -913,12 +941,18 @@ func TestWorktreeContainment_ToolCwdIsWorktree(t *testing.T) { // assertion ("file must NOT appear in daemon cwd") spuriously pass or flap. // A test-private temp dir guarantees isolation: no other test writes there. daemonCwd := t.TempDir() + releaseTerminal := make(chan struct{}) + var releaseTerminalOnce sync.Once + releaseCleanup := func() { + releaseTerminalOnce.Do(func() { close(releaseTerminal) }) + } + defer releaseCleanup() // NewDefaultRegistryWithOptions wires the real bash tool; BaseRegistryOptions // (ApprovalMode empty → FullAuto in NewDefaultRegistryWithOptions) means the // per-run registry the runner builds at the worktree path runs bash ungated. runner := NewRunner( - worktreeContainmentProvider(), + &worktreeContainmentTestProvider{releaseTerminal: releaseTerminal}, NewDefaultRegistryWithOptions(daemonCwd, DefaultRegistryOptions{ ApprovalMode: ToolApprovalModeFullAuto, }), @@ -1010,6 +1044,7 @@ func TestWorktreeContainment_ToolCwdIsWorktree(t *testing.T) { if !gotToolDone { gotToolDone = true assertContainment(wsPath) + releaseCleanup() } case EventRunCompleted: gotCompleted = true