From 3384616a8598a7fb73638220601a4a7d24c9eced Mon Sep 17 00:00:00 2001 From: Dennison Date: Thu, 30 Jul 2026 23:45:37 +0200 Subject: [PATCH] Stabilize provider key capture test --- cmd/harnessd/main_test.go | 8 +- docs/logs/engineering-log.md | 19 ++++ docs/logs/long-term-thinking-log.md | 15 +++ ...52-provider-key-capture-sync-impact-map.md | 94 +++++++++++++++++++ ...sue-1052-provider-key-capture-sync-plan.md | 75 +++++++++++++++ docs/plans/INDEX.md | 2 + docs/plans/active-plan.md | 6 ++ 7 files changed, 218 insertions(+), 1 deletion(-) create mode 100644 docs/plans/2026-07-30-issue-1052-provider-key-capture-sync-impact-map.md create mode 100644 docs/plans/2026-07-30-issue-1052-provider-key-capture-sync-plan.md diff --git a/cmd/harnessd/main_test.go b/cmd/harnessd/main_test.go index 0bb805fa..3d2e6180 100644 --- a/cmd/harnessd/main_test.go +++ b/cmd/harnessd/main_test.go @@ -4109,6 +4109,7 @@ func TestMatrix_ProviderAPIKeyCapture(t *testing.T) { var capturedKey string var captureMu sync.Mutex + providerStarted := make(chan struct{}) env := baseEnv(addr) env["OPENAI_API_KEY"] = "matrix-test-key-xyz" @@ -4123,11 +4124,16 @@ func TestMatrix_ProviderAPIKeyCapture(t *testing.T) { captureMu.Lock() capturedKey = cfg.APIKey captureMu.Unlock() + close(providerStarted) return &noopProvider{}, nil }, "") }() - awaitHealthy(t, addr, 10*time.Second) + select { + case <-providerStarted: + case <-time.After(3 * time.Second): + t.Fatal("timed out waiting for provider factory") + } sig <- os.Interrupt select { diff --git a/docs/logs/engineering-log.md b/docs/logs/engineering-log.md index 123ff99e..43004b8f 100644 --- a/docs/logs/engineering-log.md +++ b/docs/logs/engineering-log.md @@ -77,6 +77,25 @@ - complete `internal/workflow` normal/race passed; - unchanged foreground non-TTY `./scripts/test-regression.sh` passed normal, race, and coverage at 85.6% with zero uncovered functions. +## 2026-07-30 — Issue #1052 provider API-key capture readiness coupling + +- Symptom: PR #1051's hosted race job failed because + `TestMatrix_ProviderAPIKeyCapture` did not observe `/healthz` within three + seconds; the same job log showed the server listening immediately after the + deadline. +- Cause: A provider-configuration unit contract was synchronized through the + entire parallel harness startup path, adding unrelated scheduler, watcher, + persistence, and HTTP timing. +- Intended fix: Publish a test-local signal from the injected provider factory, + assert the captured sentinel after that signal, then keep the existing + interrupt and bounded shutdown proof. +- Scope: Test and documentation only; no runtime behavior change. +- TDD evidence: Adding the direct signal wait without emitting it failed with + `timed out waiting for provider factory`; closing the channel after protected + key capture made it green. +- Verification: Focused normal and race tests passed 100 repetitions each; + complete `cmd/harnessd` normal/race suites passed; the repository regression + gate passed normal, race, and coverage at 85.6% with zero uncovered functions. ## 2026-07-30 (Workflow Failure-Event Test Timeout — Issue #1049) diff --git a/docs/logs/long-term-thinking-log.md b/docs/logs/long-term-thinking-log.md index 8b6bcf45..1bae5705 100644 --- a/docs/logs/long-term-thinking-log.md +++ b/docs/logs/long-term-thinking-log.md @@ -65,6 +65,21 @@ `broken pipe`; the green returned the child exit plus bounded stderr. Focused normal/race stress, complete workflow normal/race, and the unchanged regression gate are green at 85.6% coverage with zero uncovered functions. +## 2026-07-30 (Issue #1052 Provider API-key Capture Synchronization) + +- Command intent: Restore a zero-failure merge path for the cron/callback + repair chain after PR #1051 exposed a race-suite false negative. +- User intent: Do not waive repository failures; fix them and continue through + merged, manually proven API, TUI, and native GUI behavior. +- Success definition: + - The API-key capture test observes the provider factory directly. + - It no longer depends on a three-second full-server readiness deadline. + - Graceful shutdown remains bounded and leak-free. + - Focused repeated normal/race tests and the repository normal/race/coverage + gate pass on the exact reviewed head. +- Non-goals: Production startup, health endpoint, and global timeout changes. +- Next verification step: Make the direct signal expectation fail first, emit + it at the provider factory boundary, then run focused and full gates. ## 2026-07-30 (Workflow Failure-Event Test Timeout — Issue #1049) diff --git a/docs/plans/2026-07-30-issue-1052-provider-key-capture-sync-impact-map.md b/docs/plans/2026-07-30-issue-1052-provider-key-capture-sync-impact-map.md new file mode 100644 index 00000000..d38a2476 --- /dev/null +++ b/docs/plans/2026-07-30-issue-1052-provider-key-capture-sync-impact-map.md @@ -0,0 +1,94 @@ +# Cross-Surface Impact Map: Provider API-key capture synchronization + +## Task + +- Task / issue: #1052 +- Plan link: `2026-07-30-issue-1052-provider-key-capture-sync-plan.md` +- Owner: Codex +- Status: Implemented; promotion pending + +## Current Ownership, Callers, and Data Flow + +- Entry points: `TestMatrix_ProviderAPIKeyCapture`. +- Owning packages/types/functions and source of truth: + `cmd/harnessd/main_test.go`; injected `getenv` supplies the sentinel and the + injected `newProvider` factory receives `openai.Config`. +- Callers, consumers, events, and downstream data: Test-only call to + `runWithSignals`; no production caller changes. +- Similar abstractions searched: `awaitHealthy`, `runWithSignals`, and injected + provider factories in `cmd/harnessd`. +- Search commands/evidence: + `rg -n "awaitHealthy|ProviderAPIKeyCapture|runWithSignals" cmd/harnessd`. +- Duplication/ownership conclusion: The provider factory is already the + authoritative observation point; HTTP health is redundant for this contract. + +## Config, API, CLI, and Tools + +- User-facing config added or changed: None. +- Defaults / fallbacks: None. +- Environment variables, config files, or saved settings touched: Test-local + `OPENAI_API_KEY` sentinel remains unchanged. +- Endpoints, request fields, response fields, or server wiring affected: None. +- CLI commands, tools, wire formats, or integrations affected: None. +- Error states / validation changes: None. + +## Persistence and Compatibility + +- Schemas, migrations, caches, generated data, or ownership changes: None. +- Backward/forward compatibility and versioning: None; test-only. +- Partial rollout and mixed-version behavior: None. + +## Lifecycle, Security, and Reliability + +- Concurrency, cancellation, retries, cleanup, and resource ownership: + Synchronize directly with provider-factory invocation, then deliver the + existing interrupt and require bounded `runWithSignals` completion. +- Authentication, authorization, permissions, trust, privacy, and secrets: + The sentinel key remains test-local and is never logged. +- Failure modes, recovery, idempotency, and data repair: A missing factory + invocation or non-terminating server fails with a bounded diagnostic. + +## Product and Integration Surfaces + +- Server/runtime: Production code unchanged. +- TUI/web/macOS/other clients: None. +- Provider/model/tool catalog and routing: Provider factory observation only; + routing behavior unchanged. +- External systems and automation: GitHub Actions race reliability improves. +- UX states, keyboard/focus/accessibility/motion: None. + +## Deployment and Operations + +- Deployment/migration order and feature flags: None. +- Logs, metrics, traces, alerts, and support diagnostics: None. +- Rollback triggers and recovery steps: Revert if exact-head normal/race checks + or local full regression fail. +- Runbooks and operator docs: None. + +## Regression Tests + +- Characterization and first expected red test: Test waits for an explicit + provider-invocation signal that is initially never published. +- New acceptance tests required: Updated provider-key capture test. +- Edge, negative, failure, lifecycle, and security tests: Existing bounded + graceful-shutdown failure remains. +- Integration/e2e/real-path proof: Hosted normal/race jobs on the exact PR head. +- Cross-surface regressions to guard: Whole `cmd/harnessd` normal/race suites + and repository normal/race/coverage gate. +- Exact targeted and full commands: + `go test ./cmd/harnessd -run TestMatrix_ProviderAPIKeyCapture -count=100`; + `go test -race ./cmd/harnessd -run TestMatrix_ProviderAPIKeyCapture -count=100`; + `go test ./cmd/harnessd -count=1`; + `go test -race ./cmd/harnessd -count=1`; + `./scripts/test-regression.sh`. + +## Documentation and Handoff + +- Specs/public docs before code: Plan and impact map. +- Implementation notes/logs/indexes after code: Plan/index, engineering log, + long-term-thinking log. +- Training/onboarding/release notes: None; test-only. + +## Warning Check + +- Every surface is either mapped or explicitly marked unaffected with rationale. diff --git a/docs/plans/2026-07-30-issue-1052-provider-key-capture-sync-plan.md b/docs/plans/2026-07-30-issue-1052-provider-key-capture-sync-plan.md new file mode 100644 index 00000000..fe7b94d2 --- /dev/null +++ b/docs/plans/2026-07-30-issue-1052-provider-key-capture-sync-plan.md @@ -0,0 +1,75 @@ +# Plan: Provider API-key capture synchronization + +## Context + +- Governing GitHub issue: #1052 +- Problem: `TestMatrix_ProviderAPIKeyCapture` waits for the complete HTTP server + to become healthy within three seconds even though its contract is only to + observe the provider factory input. Hosted race execution exceeded that + unrelated deadline and blocked PR #1051. +- User impact: A false-negative CI gate prevents verified cron/callback repairs + from reaching `main`. +- Constraints: Test-only change; do not raise a global timeout or change + production startup behavior. + +## Scope + +- In scope: Direct synchronization on provider-factory invocation, bounded + process shutdown, focused repeated normal/race coverage, and the full gate. +- Out of scope: HTTP health semantics, production startup sequencing, and + global test timeout policy. + +## Documentation Contract + +- Feature status: `implemented` +- Public docs affected: None; this is test infrastructure only. +- Spec docs to update before code: This plan and its impact map. +- Implementation notes to add after code: Engineering and long-term-thinking + logs, plus final verification evidence. + +## Test Plan (TDD) + +- New failing tests to add first: Replace the HTTP readiness dependency with a + provider-invocation signal; the initial test edit must fail until the signal + is emitted from the factory seam. +- Existing tests to update: `TestMatrix_ProviderAPIKeyCapture`. +- Regression tests required: Repeated focused normal and race runs, the whole + `cmd/harnessd` package in normal/race mode, and + `./scripts/test-regression.sh`. + +## Cross-Surface Impact Map + +- See `2026-07-30-issue-1052-provider-key-capture-sync-impact-map.md`. + +## Implementation Checklist + +- [x] Define acceptance criteria in tests. +- [x] Link a contract-complete structured GitHub issue before implementation. +- [x] Record current architecture, callers, consumers, and source-of-truth search evidence. +- [x] Document feature status and exact contract before code. +- [x] Complete and reconcile the cross-surface impact map before implementation. +- [x] Add characterization coverage before structural refactors. +- [x] Write failing tests first. +- [x] Review ownership/copy semantics for exported or state-storing types when mutable fields cross boundaries. +- [x] Implement minimal code changes. +- [x] Refactor while tests remain green. +- [x] Update docs, status ledgers, and indexes. +- [x] Update engineering/system/observational logs as needed. +- [x] Run full test suite. +- [ ] Merge branch back to `main` after tests pass. + +## Risks and Mitigations + +- Risk: Signaling too early could leave `runWithSignals` blocked or leak a + server goroutine. +- Mitigation: Keep the bounded interrupt/shutdown assertion and run the focused + test repeatedly under the race detector. + +## Verification + +- Expected red: the new direct provider signal timed out before the factory + emitted it. +- Focused normal and race tests each passed 100 consecutive runs. +- The complete `cmd/harnessd` package passed in normal and race modes. +- `./scripts/test-regression.sh` passed normal, race, and coverage with 85.6% + total coverage and zero uncovered functions. diff --git a/docs/plans/INDEX.md b/docs/plans/INDEX.md index 8428f29d..a3c25ef3 100644 --- a/docs/plans/INDEX.md +++ b/docs/plans/INDEX.md @@ -8,6 +8,8 @@ - `2026-07-31-issue-1062-provider-key-matrix-health-wait-impact-map.md` — Cross-surface impact map for Issue #1062. - `2026-07-31-issue-1064-workflow-exit-precedence-plan.md` — Issue #1064 deterministic source-workflow process-exit diagnostic precedence repair. - `2026-07-31-issue-1064-workflow-exit-precedence-impact-map.md` — Cross-surface impact map for Issue #1064. +- `2026-07-30-issue-1052-provider-key-capture-sync-plan.md` — Issue #1052 direct provider-factory synchronization for the API-key capture regression. +- `2026-07-30-issue-1052-provider-key-capture-sync-impact-map.md` — Cross-surface impact map for Issue #1052. - `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. diff --git a/docs/plans/active-plan.md b/docs/plans/active-plan.md index 4123a30e..a6f92245 100644 --- a/docs/plans/active-plan.md +++ b/docs/plans/active-plan.md @@ -14,6 +14,11 @@ and unchanged regression gate are recorded. PR #1069 remains open and unmerged; the local review-fix commit still requires parent promotion and hosted reruns. PR #1060, PR #1055, and issue #1067 remain excluded. +Current status: Issue #1052 has removed the provider API-key capture test's +unrelated three-second HTTP readiness dependency. Focused repeated normal/race, +complete package normal/race, and repository normal/race/coverage gates pass; +promotion is pending before the cron/callback repair chain can merge. + Current status: Issue #1023 anytime contextual `/feedback` intake is implemented test-first and verified in its isolated worktree; targeted, full normal/race, coverage-gate, and real TUI bundle checks pass, with merge pending. @@ -26,6 +31,7 @@ review/cleanup. Current active plans: - `2026-07-31-issue-1068-dispatcher-shutdown-isolation-plan.md` +- `2026-07-30-issue-1052-provider-key-capture-sync-plan.md` - `2026-07-30-issue-1023-feedback-intake-plan.md` - `2026-06-26-adapter-first-eval-harness-plan.md` - `2026-04-05-orchestration-program-plan.md`