Conversation
thymikee
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
14 issues found across 380 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/daemon/interaction/internal/interaction-touch-fill.ts">
<violation number="1" location="src/daemon/interaction/internal/interaction-touch-fill.ts:213">
P1: A fill that completes after its admitted session is retired now fails response construction with `session_lifetime_ended`. Resolve the lifetime optionally and, like the targeted-touch response builder, omit the retired result's reference frame and settle observation rather than requiring the old lifetime.
(Based on your team's feedback about skipping settle refs after retirement.)</violation>
</file>
<file name="src/__tests__/test-utils/store-factory.ts">
<violation number="1" location="src/__tests__/test-utils/store-factory.ts:22">
P3: `ref.session !== session` compares object identity, but `SessionStore.update(ref, patch)` replaces the stored record with a new object (`next = { ...current, ...changes }; entry.current = next`). Re-calling this helper with the originally published session object after an update on the same address therefore throws "A different test session occupies this address" for the same logical session. Pass the current record (e.g. the ref's `session`) or document that callers must, since the identity check cannot tell a replaced record from a foreign one.</violation>
</file>
<file name="packages/capture-kit/src/durable-capture/session-binding.fixtures.ts">
<violation number="1" location="packages/capture-kit/src/durable-capture/session-binding.fixtures.ts:22">
P3: `lookup` throws "Test session retired" whenever the address has no entry, including the first-time case where `set` was never called for it (all callers `set` before binding). A never-registered session isn't retired, so the message misleads when a test binds to a typo'd or unseeded address. Use a distinct message for the unregistered case, e.g. "Test session not found".</violation>
</file>
<file name="src/daemon/__tests__/replay-suite/session-test-suite.test.ts">
<violation number="1" location="src/daemon/__tests__/replay-suite/session-test-suite.test.ts:500">
P2: Same mismatch as the macOS hunk: when `sessionStore.get(req.session)` returns an existing record (the `?? makeAndroidSession(...)` fallback is there precisely because that branch is reachable), `sessionStore.publish(req.session, session)` throws `session_address_occupied` instead of the previous no-op/replace `set`. `get` already returns the live record whose field mutation is a durable write, so publishing the same address back is unnecessary and now fatal. Only publish when the session was newly created (`get` returned undefined).</violation>
</file>
<file name="src/daemon/screen-recording-session-binding.ts">
<violation number="1" location="src/daemon/screen-recording-session-binding.ts:15">
P2: This recreates the delegated binding for every `read`, discarding its last observed resource. If the published recording is replaced and the session then retires, `read()` returns the original handle instead of the latest owned handle; retain one delegated binding for the wrapper lifetime.
(Based on your team's feedback about retaining session capture resources after retirement.)</violation>
<violation number="2" location="src/daemon/screen-recording-session-binding.ts:19">
P2: This allows a binding whose published lifetime has ended to create a second session lifetime under the same address. Check the existing `published` ref’s lifetime before the publish path so delayed adoption cannot resurrect the draft.</violation>
</file>
<file name="src/daemon/__tests__/replay-divergence/session-replay-divergence-observation.test.ts">
<violation number="1" location="src/daemon/__tests__/replay-divergence/session-replay-divergence-observation.test.ts:130">
P3: `store.get('default')` is always undefined in this test: only `'cwd:worktree:default'` (and ref.address) is ever published, so this assertion never fails and verifies nothing about lifetime binding. Remove it; the meaningful assertions are `store.get(ref.address)` plus the state/snapshot checks.</violation>
</file>
<file name="packages/capture-kit/src/durable-capture/adoption.ts">
<violation number="1" location="packages/capture-kit/src/durable-capture/adoption.ts:64">
P2: The post-cleanup `canPersist()` gate can strand the manifest written by this attempt when the bound lifetime retires during async disposal. Always run the fence-guarded `confirmFailedAdoptionTransition`; its fence check prevents overwriting a successor's record while allowing this record to become `completed`.</violation>
</file>
<file name="src/daemon/__tests__/app-log-session-resource.test.ts">
<violation number="1" location="src/daemon/__tests__/app-log-session-resource.test.ts:382">
P3: The "successor manifest" this test writes is the failed adoption's own `runtime.result.envelope` (same sessionId, fence, and lifecycle as the 'active' record adoption already wrote to `resourcePath`), so the final `{lifecycle: 'open'}` assertion passes even if a regression rewrote the record back to the adoption's envelope instead of preserving legitimate successor evidence. Write a distinguishable envelope — e.g., the successor fence used in the 'late adoption disposes its pending handle' test — so the preservation claim is actually verified.</violation>
</file>
<file name="packages/capture-kit/src/durable-capture/adoption.test.ts">
<violation number="1" location="packages/capture-kit/src/durable-capture/adoption.test.ts:91">
P3: This retired-lifetime failure path reports `reportUndurableCleanup` with `{ confirmed: true }` (canPersist() is false after the lifetime retires, so no tombstone is persisted and the cleanup is confirmed), but the test never asserts the report. The two sibling tests in this file both assert their `reportUndurableCleanup` outcomes, while this test leaves that distinct reporting branch uncovered.</violation>
</file>
<file name="packages/platform-android/src/recording/failed-finish.test.ts">
<violation number="1" location="packages/platform-android/src/recording/failed-finish.test.ts:145">
P3: `sessionStore.get` and `sessionStore.set` are now dead code: the shared coordinator mutates `session` only through `binding.adopt`/`binding.clear`, and `get`/`set` are never invoked. Keep only `resolveSessionDir` (or inline it) so the test reflects the new binding contract.</violation>
</file>
<file name="src/daemon/selector-runtime-backend.ts">
<violation number="1" location="src/daemon/selector-runtime-backend.ts:90">
P2: `ref` refreshes captures to the current same-lifetime session, but native text reads still use the admission-time `session` captured by `createSelectorBackend`. Resolve `params.ref` immediately before each native read and build its execution context from that current session, otherwise a rebuild during capture can use stale app, surface, or trace metadata.</violation>
</file>
<file name="packages/capture-kit/src/capture-admission/__tests__/durable-capture-resource.fixtures.ts">
<violation number="1" location="packages/capture-kit/src/capture-admission/__tests__/durable-capture-resource.fixtures.ts:67">
P3: `context.session` is now a stale snapshot: `binding.adopt`/`binding.clear` replace the store entry with a new object via `slot.replace`, so the returned `session` (the original `{}` object seeded before the binding was created) never reflects an adopted or cleared resource. Test authors reading `context.session.appLog` get `undefined` even after a successful start. Drop `session` from the returned context, or derive it from the binding instead of seeding it separately.</violation>
</file>
<file name="packages/host-kit/src/internal/process-lock.ts">
<violation number="1" location="packages/host-kit/src/internal/process-lock.ts:166">
P2: When `releaseReclaimMutex` throws on the path where the acquisition already succeeded, the raw fs error propagates while the lock directory, owner record, and the mutation guard file all stay behind, permanently wedging the path. Unlike the acquisition-failure branch, this branch never rolls back (`fs.rmdirSync(lockDirPath)` only runs in the `acquireUnderMutationGuard` catch) and never removes the guard file. Because `holdReclaimMutex` is checked before the lock is ever inspected, every later `tryAcquireProcessLock` returns `busy` at the guard check and can never reach the reclaim that would clear the left-over lock. Unlink failure is rare (external deletion, `EPERM`/`EACCES`), but when it happens the only recovery is manual removal of both paths; the caller additionally receives a bare `ENOENT`/`EPERM` error instead of the typed AppError the rest of this module reports.</violation>
</file>
Note: This PR contains a large number of files. cubic selects up to 200 of the highest-priority eligible files for this review, so some files may not have been reviewed.
Re-trigger cubic
| result: FillCommandResult; | ||
| text: string; | ||
| flags: CommandFlags | undefined; | ||
| staleRefsWarning: string | undefined; | ||
| }): InteractionResponsePayloads { | ||
| const { session, result } = params; | ||
| const { result, ref, sessionStore } = params; | ||
| const session = sessionStore.requireCurrent(ref); |
There was a problem hiding this comment.
P1: A fill that completes after its admitted session is retired now fails response construction with session_lifetime_ended. Resolve the lifetime optionally and, like the targeted-touch response builder, omit the retired result's reference frame and settle observation rather than requiring the old lifetime.
(Based on your team's feedback about skipping settle refs after retirement.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/daemon/interaction/internal/interaction-touch-fill.ts, line 213:
<comment>A fill that completes after its admitted session is retired now fails response construction with `session_lifetime_ended`. Resolve the lifetime optionally and, like the targeted-touch response builder, omit the retired result's reference frame and settle observation rather than requiring the old lifetime.
(Based on your team's feedback about skipping settle refs after retirement.) </comment>
<file context>
@@ -198,13 +202,15 @@ async function prepareFillRefTarget(
}): InteractionResponsePayloads {
- const { session, result } = params;
+ const { result, ref, sessionStore } = params;
+ const session = sessionStore.requireCurrent(ref);
const maestroFallback = maestroFallbackDisclosure(
params.flags?.maestro?.allowNonHittableCoordinateFallback === true,
</file context>
| platform: 'android', | ||
| }); | ||
| sessionStore.set(req.session, session); | ||
| sessionStore.publish(req.session, session); |
There was a problem hiding this comment.
P2: Same mismatch as the macOS hunk: when sessionStore.get(req.session) returns an existing record (the ?? makeAndroidSession(...) fallback is there precisely because that branch is reachable), sessionStore.publish(req.session, session) throws session_address_occupied instead of the previous no-op/replace set. get already returns the live record whose field mutation is a durable write, so publishing the same address back is unnecessary and now fatal. Only publish when the session was newly created (get returned undefined).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/daemon/__tests__/replay-suite/session-test-suite.test.ts, line 500:
<comment>Same mismatch as the macOS hunk: when `sessionStore.get(req.session)` returns an existing record (the `?? makeAndroidSession(...)` fallback is there precisely because that branch is reachable), `sessionStore.publish(req.session, session)` throws `session_address_occupied` instead of the previous no-op/replace `set`. `get` already returns the live record whose field mutation is a durable write, so publishing the same address back is unnecessary and now fatal. Only publish when the session was newly created (`get` returned undefined).</comment>
<file context>
@@ -497,7 +497,7 @@ test('test aggregates snapshot diagnostics from replay session samples', async (
platform: 'android',
});
- sessionStore.set(req.session, session);
+ sessionStore.publish(req.session, session);
return { ok: true, data: { replayed: 1, healed: 0 } };
},
</file context>
| ).rejects.toMatchObject({ code: 'COMMAND_FAILED' }); | ||
| expect(start.forceCleanup).toHaveBeenCalledOnce(); | ||
| expect(context.sessionStore.get(context.sessionName)).toBe(successor); | ||
| expect(testCaptureStore.read(context.resourcePath).status).toBe('missing'); |
There was a problem hiding this comment.
P3: This retired-lifetime failure path reports reportUndurableCleanup with { confirmed: true } (canPersist() is false after the lifetime retires, so no tombstone is persisted and the cleanup is confirmed), but the test never asserts the report. The two sibling tests in this file both assert their reportUndurableCleanup outcomes, while this test leaves that distinct reporting branch uncovered.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/capture-kit/src/durable-capture/adoption.test.ts, line 91:
<comment>This retired-lifetime failure path reports `reportUndurableCleanup` with `{ confirmed: true }` (canPersist() is false after the lifetime retires, so no tombstone is persisted and the cleanup is confirmed), but the test never asserts the report. The two sibling tests in this file both assert their `reportUndurableCleanup` outcomes, while this test leaves that distinct reporting branch uncovered.</comment>
<file context>
@@ -68,3 +68,25 @@ test('a failed terminal transition preserves the primary error and reports it un
+ ).rejects.toMatchObject({ code: 'COMMAND_FAILED' });
+ expect(start.forceCleanup).toHaveBeenCalledOnce();
+ expect(context.sessionStore.get(context.sessionName)).toBe(successor);
+ expect(testCaptureStore.read(context.resourcePath).status).toBe('missing');
+});
</file context>
| expect(testCaptureStore.read(context.resourcePath).status).toBe('missing'); | |
| expect(testCaptureStore.read(context.resourcePath).status).toBe('missing'); | |
| expect(context.reportUndurableCleanup).toHaveBeenCalledWith(context.device, { confirmed: true }); |
| get: () => session, | ||
| set: (_name: string, next: AndroidRecordingSession) => { | ||
| session = next; | ||
| }, | ||
| resolveSessionDir: (name) => path.join(sessionsDir, name), | ||
| resolveSessionDir: (name: string) => path.join(sessionsDir, name), |
There was a problem hiding this comment.
P3: sessionStore.get and sessionStore.set are now dead code: the shared coordinator mutates session only through binding.adopt/binding.clear, and get/set are never invoked. Keep only resolveSessionDir (or inline it) so the test reflects the new binding contract.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/platform-android/src/recording/failed-finish.test.ts, line 145:
<comment>`sessionStore.get` and `sessionStore.set` are now dead code: the shared coordinator mutates `session` only through `binding.adopt`/`binding.clear`, and `get`/`set` are never invoked. Keep only `resolveSessionDir` (or inline it) so the test reflects the new binding contract.</comment>
<file context>
@@ -128,42 +127,50 @@ async function adoptAndroidRecording(params: {
- const sessionStore: DurableCaptureSessionStore<AndroidRecordingSession> = {
- set: (_name, next) => {
+ const sessionStore = {
+ get: () => session,
+ set: (_name: string, next: AndroidRecordingSession) => {
session = next;
</file context>
| get: () => session, | |
| set: (_name: string, next: AndroidRecordingSession) => { | |
| session = next; | |
| }, | |
| resolveSessionDir: (name) => path.join(sessionsDir, name), | |
| resolveSessionDir: (name: string) => path.join(sessionsDir, name), | |
| resolveSessionDir: (name: string) => path.join(sessionsDir, name), |
| const session: TestCaptureSession = {}; | ||
| sessionStore.set(sessionName, session); | ||
| return { | ||
| binding: makeCaptureSessionBinding(sessionStore, sessionName, { |
There was a problem hiding this comment.
P3: context.session is now a stale snapshot: binding.adopt/binding.clear replace the store entry with a new object via slot.replace, so the returned session (the original {} object seeded before the binding was created) never reflects an adopted or cleared resource. Test authors reading context.session.appLog get undefined even after a successful start. Drop session from the returned context, or derive it from the binding instead of seeding it separately.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/capture-kit/src/capture-admission/__tests__/durable-capture-resource.fixtures.ts, line 67:
<comment>`context.session` is now a stale snapshot: `binding.adopt`/`binding.clear` replace the store entry with a new object via `slot.replace`, so the returned `session` (the original `{}` object seeded before the binding was created) never reflects an adopted or cleared resource. Test authors reading `context.session.appLog` get `undefined` even after a successful start. Drop `session` from the returned context, or derive it from the binding instead of seeding it separately.</comment>
<file context>
@@ -72,6 +64,10 @@ export function makeDurableCaptureContext(
const session: TestCaptureSession = {};
sessionStore.set(sessionName, session);
return {
+ binding: makeCaptureSessionBinding(sessionStore, sessionName, {
+ read: (session) => session.appLog,
+ replace: (session, appLog) => ({ ...session, appLog, appLogFailure: undefined }),
</file context>
971332d to
26db357
Compare
d3d0c57 to
7e46a5a
Compare
Size Report
Startup median (7 runs, lower is better):
|
26db357 to
bc132d7
Compare
e4ea909 to
408192c
Compare
bc132d7 to
2ce928f
Compare
|
Rechecked the original 26-thread review against the runtime tree inherited from #3170 at The scanner fixes are at Four threads are resolved with evidence: log truncation preserves its inode; the legacy guard control never waits behind an external guard; release already waits for the guard; and a real exclusive write against the hardened lock directory returns handled The remaining owning-layer reviews stay open:
Test-control and documentation findings are tracked too: distinguish successor envelopes, assert cleanup reports, remove stale fixture projections/no-op methods, make the zombie control match its title, and strengthen the remaining fixture controls. The guard-held release assertion and ADR release wording are now corrected in the later parent The earlier “fake process group” diagnosis was too strong. In #3131’s exact-head Integration Tests job, screenshot cleanup signals group These follow-ups remain tracked in #3116. Review findings are assessed at their owning boundary, including regression controls, before resolution. |
408192c to
94807d3
Compare
|
At 2ce928f the R7 gate still misses foreign session writes in most files that hold a The rule to enforce is this: every Could the Not blocking: the same ~120-line tracking layer exists only to avoid a false match when The open cubic-dev-ai threads do not apply to this 2-file diff, so please resolve them. They point at files outside this change (inherited from the #3170 base or not in this diff): interaction-touch-fill.ts:213, daemon-client-lifecycle.ts:232, session-repair-tombstone.ts:74, session-test-suite.test.ts:500, screen-recording-session-binding.ts:15, screen-recording-session-binding.ts:19, capture-kit adoption.ts:66, daemon-registration-owner.ts:318, selector-runtime-backend.ts:90, host-kit process-lock.ts:189, installation.md:123, store-factory.ts:22, session-binding.fixtures.ts:22, session-replay-divergence-observation.test.ts:130, app-log-session-resource.test.ts:382, adoption.test.ts:91, failed-finish.test.ts:149, daemon-exit-wait.test.ts:223, durable-capture-resource.fixtures.ts:67, daemon-client-lifecycle.ts:429. I read the code but did not run the scanner or its tests, so the gap above comes from reading |
2ce928f to
14c30ab
Compare
408192c to
111f50f
Compare
|
Consolidated into #3155 as part of reducing #3116 to seven PRs. The composition preserves the complete pre-consolidation source tree, including tests and later review corrections. This PR is superseded; its review discussion and native evidence remain available. Outstanding findings transfer to the owning keeper in the implementation record. |
|
1 participant
Summary
Finish the session-write scanner review for #3155 and #3116. Optional chains, record clones/rest copies and destructured refs keep their ownership identity. Nested writes through a tracked
SessionRef.sessionreach R7; unrelated request session names stay outside that recognition.Patch-return collection skips nested functions. Fixtures must parse as valid modules, preventing parser recovery from masking invalid examples. Two files changed; the scanner remains a scoped AST gate.
Validation
Reconciled head:
14c30ab142d4e977ab6467f3fc84a070f59e0294. The review base pins #3170 at111f50f783b703adc6731537f9b3581bae4b040e; #3170 remains a prerequisite.pnpm check:affected --base 111f50f783b703adc6731537f9b3581bae4b040e --runpasses all runnable checks.git range-diffconfirms the reviewed patch is unchanged; both files are byte-identical to the prior head.2ce928f5e6. Checks on the reconciled head are pending. No device run is required for this tooling-only diff.