diff --git a/docs/logs/engineering-log.md b/docs/logs/engineering-log.md index 87a0e4fb..ef9c2f2d 100644 --- a/docs/logs/engineering-log.md +++ b/docs/logs/engineering-log.md @@ -14,6 +14,20 @@ workflows package passed normal/race, and `./scripts/test-regression.sh` passed with 85.6% total coverage and zero uncovered functions. +## 2026-07-30 (AskUserQuestion Status-Test Publication Race — Issue #1044) + +- Symptom: GitHub Actions `make test-race` reported a concurrent write/read of + the test-local run ID, then sampled an empty status instead of `running`. +- Cause: `StartRun` dispatches the provider before returning, while the fixture + assigned its closure-captured ID only after the return. +- Planned fix: publish the returned ID through a capacity-one channel before + collecting events; completion step two consumes the handoff before sampling. +- Verification contract: focused normal/race stress, harness normal/race, full + regression, and GitHub required checks. +- 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 (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 cfbaa965..3f0efddc 100644 --- a/docs/logs/long-term-thinking-log.md +++ b/docs/logs/long-term-thinking-log.md @@ -13,6 +13,20 @@ - Outcome: the stopped ten-second timer preserves the exact live-event contract and diagnostics while focused, package, race, and coverage gates are green. +## 2026-07-30 (AskUserQuestion Status-Test Publication Race — Issue #1044) + +- Command intent: clear the exact harness race that blocked required checks for + the cron/callback GUI merge chain. +- User intent: merge only after every accepted baseline gate is green. +- Success definition: retain the mid-run denial/status contract while removing + unsynchronized fixture state, then pass focused, package, full, and hosted + gates. +- Guardrails: issue-first isolated worktree, test-only synchronization, no + production dispatch or status change. +- 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 (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-1044-ask-status-race-impact-map.md b/docs/plans/2026-07-30-issue-1044-ask-status-race-impact-map.md new file mode 100644 index 00000000..74f1b2c8 --- /dev/null +++ b/docs/plans/2026-07-30-issue-1044-ask-status-race-impact-map.md @@ -0,0 +1,67 @@ +# Cross-Surface Impact Map: Issue #1044 AskUserQuestion Status-Test Race + +## Task + +- Task / issue: synchronize the status regression fixture, #1044. +- Plan: `2026-07-30-issue-1044-ask-status-race-plan.md`. +- Owner: Codex. +- Status: implemented and fully verified locally; hosted checks pending. + +## Current Ownership, Callers, and Data Flow + +- Entry: `TestDeniedAskUserQuestionDoesNotStrandRunStatus`. +- Production source of truth: `Runner.StartRun` stores a run then dispatches it + asynchronously; `Runner.GetRun` returns the status snapshot. +- Test flow: `funcProvider` completion step two samples the run status through a + closure-captured ID that the test publishes after `StartRun` returns. +- Search conclusion: `CompletionRequest` has no run ID, and adjacent fixtures + do not share this closure. + +## Config, API, CLI, and Tools + +- Config/env/defaults: none. +- API/CLI/wire/tool behavior: unchanged. +- Error/status contract: retain `running`, reject `waiting_for_user`, and + require an eventual terminal status. + +## Persistence and Compatibility + +- State: test-local channel only. +- Schemas/migrations/caches: none. +- Compatibility: no runtime change. + +## Lifecycle, Security, and Reliability + +- Concurrency: a capacity-one channel establishes the happens-before edge. +- Auth/privacy/secrets: none. +- Failure/recovery: the existing status assertions remain diagnostic. + +## Product and Integration Surfaces + +- Harness production, API, TUI, web, macOS GUI, and providers: no code change. +- Automation: race-enabled CI becomes repeatable. +- UX/accessibility: none. + +## Deployment and Operations + +- Deployment/migration/flags: none. +- Rollback: revert if the handoff no longer samples step-two state. +- Observability/operator docs: none. + +## Regression Tests + +- Red: GitHub Actions race report on the test-local `runID`. +- Green: focused normal/race `-count=100`. +- Controls: denial does not end the run, status is exactly `running` mid-run, + and the final state is terminal. +- Full: harness normal/race and `./scripts/test-regression.sh`. + +## Documentation and Handoff + +- Update engineering log, long-term log, plan, impact map, and plans index. +- No public docs. + +## Warning Check + +- Every runtime/product surface is explicitly unaffected because the change is + confined to the fixture's identity publication. diff --git a/docs/plans/2026-07-30-issue-1044-ask-status-race-plan.md b/docs/plans/2026-07-30-issue-1044-ask-status-race-plan.md new file mode 100644 index 00000000..9f06feb1 --- /dev/null +++ b/docs/plans/2026-07-30-issue-1044-ask-status-race-plan.md @@ -0,0 +1,62 @@ +# Plan: Synchronize AskUserQuestion Status-Test Run Identity + +## Context + +- Governing GitHub issue: #1044. +- Problem: `TestDeniedAskUserQuestionDoesNotStrandRunStatus` assigns its + closure-captured run ID after `StartRun` has already dispatched the provider + goroutine. The provider can read that string concurrently. +- Red evidence: GitHub Actions `make test-race` reported the write at line 170 + racing the provider read at line 150, followed by an empty status assertion. +- Constraint: preserve the production status contract and the test's mid-run + observation. + +## Scope + +- In scope: a test-local, one-shot run-ID handoff. +- Out of scope: production dispatch ordering, provider requests, permissions, + and status transitions. + +## Documentation Contract + +- Feature status: test-only bug repair. +- Public docs affected: none. +- Evidence: engineering and long-term logs, this plan, impact map, plans index. + +## Test Plan + +- Red: retain the exact GitHub Actions race-detector report as the failing + regression evidence. +- Green: focused normal and race tests at `-count=100`. +- Package: `internal/harness` normal and race. +- Full: `./scripts/test-regression.sh` and GitHub required checks. + +## Cross-Surface Impact Map + +- See `2026-07-30-issue-1044-ask-status-race-impact-map.md`. + +## Implementation Checklist + +- [x] Create contract-complete bug #1044. +- [x] Capture the exact CI race report and architecture search evidence. +- [x] Write the plan and impact map before code. +- [x] Add an explicit run-ID publication handoff. +- [x] Run focused stress, package, and full local gates. +- [ ] Pass GitHub required checks. +- [ ] Merge through a closing PR. + +## Verification + +- Focused normal and race tests passed at `-count=100`. +- The complete `internal/harness` package passed normal and race tests. +- `./scripts/test-regression.sh` passed normal, race, and the 85.6% coverage + gate with zero uncovered functions. + +## Risks and Mitigations + +- Risk: synchronization could move the observation after completion. +- Mitigation: the provider still reads status in completion step two, before it + returns the terminal content. +- Risk: an unbuffered handoff could deadlock if dispatch timing changes. +- Mitigation: use a capacity-one channel and publish immediately after + `StartRun` returns. diff --git a/docs/plans/INDEX.md b/docs/plans/INDEX.md index ec848957..d88d3da9 100644 --- a/docs/plans/INDEX.md +++ b/docs/plans/INDEX.md @@ -2,6 +2,8 @@ - `2026-07-30-issue-1049-workflow-failure-timeout-plan.md` — Issue #1049 planned contention-tolerant workflow failure-event regression wait. - `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-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/tool_gates_regression_test.go b/internal/harness/tool_gates_regression_test.go index ea6da6ca..d5e458a5 100644 --- a/internal/harness/tool_gates_regression_test.go +++ b/internal/harness/tool_gates_regression_test.go @@ -133,7 +133,7 @@ func TestDeniedAskUserQuestionDoesNotStrandRunStatus(t *testing.T) { // terminal status, so the final value proves nothing. Step 2 samples the // status the run is carrying while it continues working after the denial. var runner *Runner - var runID string + runIDs := make(chan string, 1) var statusAfterDenial RunStatus step := 0 provider := &funcProvider{ @@ -147,6 +147,7 @@ func TestDeniedAskUserQuestionDoesNotStrandRunStatus(t *testing.T) { }}}, nil } if step == 2 { + runID := <-runIDs if snapshot, ok := runner.GetRun(runID); ok { statusAfterDenial = snapshot.Status } @@ -167,7 +168,7 @@ func TestDeniedAskUserQuestionDoesNotStrandRunStatus(t *testing.T) { if err != nil { t.Fatalf("start run: %v", err) } - runID = run.ID + runIDs <- run.ID if _, err := collectRunEvents(t, runner, run.ID); err != nil { t.Fatalf("collect events: %v", err) }