Conversation
Inject a composition-local title generator for Desktop E2E so fake primary replies do not race real auxiliary title requests. Keep the production generator, Host-owned fallback persistence, and retirement guard unchanged. Cover the naming/retirement boundary with a gated integration regression. Validate the unchanged archive E2E, negative ablations, and desktop suite. Fixes apache#5015 Generated-by: OpenAI Codex
Preserve deterministic E2E title injection alongside upstream WorkHub routing dependencies and fixtures. Validate the merged tree against CONTRIBUTING.md and affected CI suites; retain documented default-run timing failures and successful diagnostic reruns. Generated-by: OpenAI Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed against main at a49ba75, with the fix ablated and re-applied. The direction is right and it is a root-cause fix, not a widened assertion: the E2E fixture seeds a real anthropic connection (apps/desktop/e2e/fixtures.ts:137-160), generateTitle really calls the provider with a 15 s timeout (execution-model-authority.ts:260-290, session-title.ts:24), and the archive is refused for exactly as long as that request is in flight (session-effect-coordinator.ts:190, session-retirement-coordinator.ts:660). Injecting generateSessionTitle: async () => undefined removes the network dependency while keeping the Host's own fallback naming.
Ablation: on a normal network the flake does not reproduce with the fix removed (the placeholder key gets a fast 401), but pointing the auxiliary endpoint at a black-hole address reproduces has a live derived effect 3/3 without the fix and passes 5/5 with it. The new execution-composition.test.ts case is stable across repeated runs.
Two things before this can merge:
- Rebase onto
main. The branch conflicts inexecution-composition.ts(import list andExecutionRuntimeHostCompositionDependencies, both adjacent tomain's newworkHubRoutingModel, which is the same optional-injection pattern) and inexecution-composition.test.ts(import list and thecreateCapturedExecutionCompositiondependencies literal). All mechanical: keep both lines. - The rebase also picks up a fix this branch needs. On the PR's own base,
--repeat-each=5still fails 1/10 on the same test with a different guard —has an active or reserved root Turn(session-retirement-coordinator.ts:641) — because the spec archived as soon as the streamed reply was visible.mainfixed that in a41d3ac by waiting for the 重新生成 button (new-task-reload.spec.ts:31). With that line on top of this branch the test ran 10/10 green here; the "10/10" in the description was measured on the old base, so please let CI confirm after the rebase.
Not blocking, for the record:
- The product-side behaviour this exposes — a user's archive is refused for up to 15 s while a title is being generated, although the naming effect holds an
AbortControllerand answers no caller — deserves its own issue; this PR is right not to touch it. - Only
generateTitleis stubbed.generateRecap, memory extraction and daily review still reach the real connection from E2E; the same flake can come back under another name. Worth a note in the description, or a follow-up that gives the E2E connection a local stubbaseUrl. - "deterministic" in the title overstates it slightly: naming is still asynchronous, the window just no longer depends on an external round trip.
|
Thanks for the review! I’ve merged main into this branch, and CI is now green. |
Summary
Fixes #5015
Desktop archive E2E can fail while asynchronous session naming is still active. Inject a deterministic E2E title dependency that uses the existing Host-owned fallback/persistence path. Production title generation, recap behavior, and retirement guards remain intact.
The gated integration regression checks rejection while naming is active and successful archive after naming settles, waiting for actual Turn-residency release. No Electron assertions, sleeps, retries, or timeouts are changed by this PR.
Resolved conflicts with
mainata49ba7544c3bdd1ef648ec90c69b8f39f3e881c0, preserving upstream WorkHub routing dependencies and fixtures. The net change remains four files.Verification
Current merged tree, macOS arm64, Node 24.19.0, npm 11.19.0:
build:testalso passed.Historical pre-fix reproduction: unchanged archived-only E2E failed 2/3 times with
has a live derived effectbefore the original fix.Known validation limitations
Default local validation was not entirely green. The Runtime Host failures were
two real managed activations converge on one Host and exit at true idleandan exited owned Candidate permits one real successor in the same election. The CLI failure wasmarks an inherited running Bash card detached after rewind. Successful reruns do not erase these first-run timing failures.Linux-required sandbox smoke, Docker live Eval egress, and cross-platform checks were not run on this macOS host; CI remains required.
Merge-staged hooks also flag three byte-identical upstream files: a Biome stdin control-character conversion, a stale protocol-compatibility declaration, and a patch context blank line. Direct Biome and net-PR whitespace/epoch checks plus staged ASF checks passed. Hooks were disabled only for the merge commit; upstream files and persistent hook configuration were not changed.
AI use
Tool(s) and scope: OpenAI Codex implemented the fix, resolved conflicts, and ran validation. Commits carry
Generated-byattribution. Independent human review and merge approval remain required.Checklist
Static and change-specific checks pass; default-suite failures and diagnostic reruns are recorded above.
Does this PR entail a change in behavior?