refactor(move): relocate replay resume and failure responses - #3030
Conversation
thymikee
changed the title
Size Report
Startup median (7 runs, lower is better):
|
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 14 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/replay-port/src/daemon-port/__tests__/session-replay-runtime-failure-response.test.ts">
<violation number="1" location="packages/replay-port/src/daemon-port/__tests__/session-replay-runtime-failure-response.test.ts:47">
P3: The `reas<var:MODE>` assertion looks like garbage text without context. It is a real guard — the detail key `reason` ends in the substring `on`, which is MODE's scrub value, so if `scrubReplayVarValues` were ever applied to the details object's keys, `reason` would become `reas<var:MODE>` — but the connection is invisible and silently decays if anyone changes MODE's value in the fixture. Add a comment explaining the guard (and that it depends on `reason` containing the scrub value `on`).</violation>
</file>
<file name="src/daemon/__tests__/replay-coordinator-ownership.test.ts">
<violation number="1" location="src/daemon/__tests__/replay-coordinator-ownership.test.ts:35">
P3: `resolveRelativeTarget` returns null for any specifier that doesn't normalize under `src/` (line 75), so the chain checks don't understand the package-rooted files added here. For the moved files, the `the divergence-report chain rejects unresolved dynamic imports` assertion (via `unresolvedDynamicImportSites`) classes any legitimate dynamic import — an in-package one (`./replay-session-binding.ts`) or a workspace one (`@agent-device/...`) — as "unresolved" and fails; today the files happen to contain none, so the test passes only vacuously. Conversely, the coordinator-factory/SessionStore checks can only fire on cross-tree relative specifiers, so workspace-alias imports are invisible to them. Resolve package-rooted targets into a non-null `target` so the dynamic-import guard discriminates real unresolved specifiers from in-repo ones, while keeping `src/`-rooted resolution unchanged so the cross-tree SessionStore/coordinator checks keep working.</violation>
<violation number="2" location="src/daemon/__tests__/replay-coordinator-ownership.test.ts:35">
P2: These two chain files moved out of `src`, but `listProductionSourceFiles()` walks only the `src` root (line 54), so the `createReplayCoordinator has exactly one production call site` and "must not be re-exported" assertions no longer scan them — they were covered while located at `src/daemon/replay/internal/`. The chain-level "never imports the coordinator factory" test can't compensate: it only matches direct named value imports (`importsValueBinding` requires an `ImportSpecifier` bound to `createReplayCoordinator`), so a namespace import, `export *` re-export, or dynamic import of the coordinator from either moved file passes every assertion in this file. The files this diff deliberately places in the protected chain are now the only chain members outside the single-construction-site guarantee. Add `packages/replay-port/src/daemon-port` to the `roots` in `listProductionSourceFiles` so they stay under the same construction-site and re-export scans as before the move.</violation>
</file>
<file name="packages/replay-port/src/daemon-port/__tests__/session-replay-resume.test.ts">
<violation number="1" location="packages/replay-port/src/daemon-port/__tests__/session-replay-resume.test.ts:27">
P3: These mid-file comment blocks narrate implementation decisions and review history (#1262 "re-review", ADR 0012 rationales) between tests, which AGENTS.md's comment rule explicitly discourages ("No control-flow narration or review history in comments; encode invariants in names/types/boundaries/tests"). The tests already encode each boundary case in its name and assertions; keep at most a one-line note where the boundary is non-obvious (e.g., the empty-tail-without-session rule).</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| /** The divergence-report chain: never a second `ReplayCoordinator`, never a bare `SessionStore`. */ | ||
| const DIVERGENCE_CHAIN_FILES = [ | ||
| 'src/daemon/replay/internal/session-replay-resume.ts', | ||
| 'packages/replay-port/src/daemon-port/session-replay-resume.ts', |
There was a problem hiding this comment.
P2: These two chain files moved out of src, but listProductionSourceFiles() walks only the src root (line 54), so the createReplayCoordinator has exactly one production call site and "must not be re-exported" assertions no longer scan them — they were covered while located at src/daemon/replay/internal/. The chain-level "never imports the coordinator factory" test can't compensate: it only matches direct named value imports (importsValueBinding requires an ImportSpecifier bound to createReplayCoordinator), so a namespace import, export * re-export, or dynamic import of the coordinator from either moved file passes every assertion in this file. The files this diff deliberately places in the protected chain are now the only chain members outside the single-construction-site guarantee. Add packages/replay-port/src/daemon-port to the roots in listProductionSourceFiles so they stay under the same construction-site and re-export scans as before the move.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/daemon/__tests__/replay-coordinator-ownership.test.ts, line 35:
<comment>These two chain files moved out of `src`, but `listProductionSourceFiles()` walks only the `src` root (line 54), so the `createReplayCoordinator has exactly one production call site` and "must not be re-exported" assertions no longer scan them — they were covered while located at `src/daemon/replay/internal/`. The chain-level "never imports the coordinator factory" test can't compensate: it only matches direct named value imports (`importsValueBinding` requires an `ImportSpecifier` bound to `createReplayCoordinator`), so a namespace import, `export *` re-export, or dynamic import of the coordinator from either moved file passes every assertion in this file. The files this diff deliberately places in the protected chain are now the only chain members outside the single-construction-site guarantee. Add `packages/replay-port/src/daemon-port` to the `roots` in `listProductionSourceFiles` so they stay under the same construction-site and re-export scans as before the move.</comment>
<file context>
@@ -32,11 +32,11 @@ const RUNTIME_FILE = 'src/daemon/handlers/session-replay-command.ts';
/** The divergence-report chain: never a second `ReplayCoordinator`, never a bare `SessionStore`. */
const DIVERGENCE_CHAIN_FILES = [
- 'src/daemon/replay/internal/session-replay-resume.ts',
+ 'packages/replay-port/src/daemon-port/session-replay-resume.ts',
'src/daemon/replay/internal/session-replay-divergence.ts',
'src/daemon/replay/internal/session-replay-target-verification.ts',
</file context>
| artifactPaths: [artifactPath], | ||
| }); | ||
| expect(response.error.details).toHaveProperty('reason'); | ||
| expect(response.error.details).not.toHaveProperty('reas<var:MODE>'); |
There was a problem hiding this comment.
P3: The reas<var:MODE> assertion looks like garbage text without context. It is a real guard — the detail key reason ends in the substring on, which is MODE's scrub value, so if scrubReplayVarValues were ever applied to the details object's keys, reason would become reas<var:MODE> — but the connection is invisible and silently decays if anyone changes MODE's value in the fixture. Add a comment explaining the guard (and that it depends on reason containing the scrub value on).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/replay-port/src/daemon-port/__tests__/session-replay-runtime-failure-response.test.ts, line 47:
<comment>The `reas<var:MODE>` assertion looks like garbage text without context. It is a real guard — the detail key `reason` ends in the substring `on`, which is MODE's scrub value, so if `scrubReplayVarValues` were ever applied to the details object's keys, `reason` would become `reas<var:MODE>` — but the connection is invisible and silently decays if anyone changes MODE's value in the fixture. Add a comment explaining the guard (and that it depends on `reason` containing the scrub value `on`).</comment>
<file context>
@@ -0,0 +1,48 @@
+ artifactPaths: [artifactPath],
+ });
+ expect(response.error.details).toHaveProperty('reason');
+ expect(response.error.details).not.toHaveProperty('reas<var:MODE>');
+});
</file context>
| expect(response.error.details).not.toHaveProperty('reas<var:MODE>'); | |
| // Guard: details keys never pass through scrubReplayVarValues. The key | |
| // 'reason' ends with MODE's scrub value ('on'), so key-level scrubbing would | |
| // surface here as a 'reas<var:MODE>' property; keep that only if MODE's value stays 'on'. | |
| expect(response.error.details).not.toHaveProperty('reas<var:MODE>'); |
| /** The divergence-report chain: never a second `ReplayCoordinator`, never a bare `SessionStore`. */ | ||
| const DIVERGENCE_CHAIN_FILES = [ | ||
| 'src/daemon/replay/internal/session-replay-resume.ts', | ||
| 'packages/replay-port/src/daemon-port/session-replay-resume.ts', |
There was a problem hiding this comment.
P3: resolveRelativeTarget returns null for any specifier that doesn't normalize under src/ (line 75), so the chain checks don't understand the package-rooted files added here. For the moved files, the the divergence-report chain rejects unresolved dynamic imports assertion (via unresolvedDynamicImportSites) classes any legitimate dynamic import — an in-package one (./replay-session-binding.ts) or a workspace one (@agent-device/...) — as "unresolved" and fails; today the files happen to contain none, so the test passes only vacuously. Conversely, the coordinator-factory/SessionStore checks can only fire on cross-tree relative specifiers, so workspace-alias imports are invisible to them. Resolve package-rooted targets into a non-null target so the dynamic-import guard discriminates real unresolved specifiers from in-repo ones, while keeping src/-rooted resolution unchanged so the cross-tree SessionStore/coordinator checks keep working.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/daemon/__tests__/replay-coordinator-ownership.test.ts, line 35:
<comment>`resolveRelativeTarget` returns null for any specifier that doesn't normalize under `src/` (line 75), so the chain checks don't understand the package-rooted files added here. For the moved files, the `the divergence-report chain rejects unresolved dynamic imports` assertion (via `unresolvedDynamicImportSites`) classes any legitimate dynamic import — an in-package one (`./replay-session-binding.ts`) or a workspace one (`@agent-device/...`) — as "unresolved" and fails; today the files happen to contain none, so the test passes only vacuously. Conversely, the coordinator-factory/SessionStore checks can only fire on cross-tree relative specifiers, so workspace-alias imports are invisible to them. Resolve package-rooted targets into a non-null `target` so the dynamic-import guard discriminates real unresolved specifiers from in-repo ones, while keeping `src/`-rooted resolution unchanged so the cross-tree SessionStore/coordinator checks keep working.</comment>
<file context>
@@ -32,11 +32,11 @@ const RUNTIME_FILE = 'src/daemon/handlers/session-replay-command.ts';
/** The divergence-report chain: never a second `ReplayCoordinator`, never a bare `SessionStore`. */
const DIVERGENCE_CHAIN_FILES = [
- 'src/daemon/replay/internal/session-replay-resume.ts',
+ 'packages/replay-port/src/daemon-port/session-replay-resume.ts',
'src/daemon/replay/internal/session-replay-divergence.ts',
'src/daemon/replay/internal/session-replay-target-verification.ts',
</file context>
| }); | ||
| assert.deepEqual(resume, { allowed: true, from: 2, planDigest: 'abc123' }); | ||
| }); | ||
| // --- repairHint 'record-and-heal' shifts `from` to failedIndex + 1 (ADR 0012 |
There was a problem hiding this comment.
P3: These mid-file comment blocks narrate implementation decisions and review history (#1262 "re-review", ADR 0012 rationales) between tests, which AGENTS.md's comment rule explicitly discourages ("No control-flow narration or review history in comments; encode invariants in names/types/boundaries/tests"). The tests already encode each boundary case in its name and assertions; keep at most a one-line note where the boundary is non-obvious (e.g., the empty-tail-without-session rule).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/replay-port/src/daemon-port/__tests__/session-replay-resume.test.ts, line 27:
<comment>These mid-file comment blocks narrate implementation decisions and review history (#1262 "re-review", ADR 0012 rationales) between tests, which AGENTS.md's comment rule explicitly discourages ("No control-flow narration or review history in comments; encode invariants in names/types/boundaries/tests"). The tests already encode each boundary case in its name and assertions; keep at most a one-line note where the boundary is non-obvious (e.g., the empty-tail-without-session rule).</comment>
<file context>
@@ -0,0 +1,172 @@
+ });
+ assert.deepEqual(resume, { allowed: true, from: 2, planDigest: 'abc123' });
+});
+// --- repairHint 'record-and-heal' shifts `from` to failedIndex + 1 (ADR 0012
+// decision 6, R2): the agent already performed the diverged step manually, so
+// resuming AT it would re-diverge on the exact same step. ---
</file context>
|
Reviewed at 710c9c4. This looks ready for human review: it's a pure move of the replay resume and failure-response modules into packages/replay-port, with no behavior change. CI is green: Integration Tests, Typecheck, Bundle Size, and Repo Guards all pass, and Coverage (job 108923784522) now passes too, exercising the moved code through the unit run. Two Smoke Tests checks (job 108923785116, job 108923784771) are still pending rather than failed, and since this diff only touches packages/replay-port and daemon import paths, there's no reason to expect them to be affected — but if either comes back red touching replay-port resolution, that's worth a follow-up look. I didn't rerun pnpm check:affected myself; this relies on the PR body's reported pass at this head plus the current green checks. No live device validation was done or needed here, since there's no device-facing behavior change. Not blocking: listProductionSourceFiles() in src/daemon/tests/replay-coordinator-ownership.test.ts:54 only walks roots=['src'], so now that session-replay-resume.ts and session-replay-runtime-failure-response.ts live under packages/replay-port/src/daemon-port/, they've dropped out of the single-call-site and no-re-export scan for createReplayCoordinator — worth extending the walked roots to include packages/replay-port/src/daemon-port to restore that coverage, but it can be taken or left. |
thymikee
added
the
ready-for-human
710c9c4 to
25c07af
Compare
1 participant
Summary
@agent-device/replay-port; daemon callers use package exports.ad-replayandsession-journaldependencies and the latter's command-registry build flag declaration. This stack layer touches 14 files; rename-aware diff is 246 additions and 220 deletions.Part of #2663. This PR depends on #3028; the remaining daemon replay tree is being migrated in subsequent stack layers.
Validation
710c9c44d1ee1b74a80252fc16367c1abd823b54:pnpm check:affected --runpassed (613 related files, 5,419 tests; wire compatibility and provider integration passed).pnpm check:quick,pnpm check:layering,pnpm check:fallow --base origin/main, and focused 28 tests passed..adand Maestro characterization will run on the completed package migration commit.