fix(daemon): corroborate the no-effect claim against the rebased baseline - #1622
fix(daemon): corroborate the no-effect claim against the rebased baseline#1622thymikee wants to merge 2 commits into
Conversation
…line Closes #1620. `capturePostGestureStabilizedResult` keeps a local baseline that it REBASES when the capture backend changes mid-poll: backends disagree about which nodes exist, so a cross-backend pair says nothing about the gesture and #1569 adopts the new capture as the baseline rather than concluding from it. The verdict is then computed from that comparable pair. `buildAcceptedStabilizedResult` corroborated against `pending.baselineSignature` instead — the ORIGINAL pre-gesture signature, the one the loop had just discarded as incomparable. So on any backend fallback the agent-facing claim re-introduced exactly the comparison #1569 exists to prevent, and `haveIdenticalDiscriminatingSurfaces` demands set equality, which two backends' views of one screen never satisfy. The consequence was that the warning could not fire on any screen whose capture plan falls back — i.e. every hostile screen, which is the only kind it was built for. #1600's motivating case, element-18, was a Bluesky feed; measured live on a seeded Bluesky fixture, the capture falls back to private-ax and truncates at depth 56, and no no-effect gesture ever produced the warning. Fix: corroborate against the loop's live `baselineSignature`. When no rebase happened it is `pending`'s, so the #1601 veto is unchanged for the ordinary case; when a rebase happened it is the comparable same-backend capture, which is the only pair either check should ever read. Red evidence: the new test models what two backends actually return for one screen — private-ax additionally reports a scrolled-away row the tree backend prunes — with every post-gesture capture identical to the others, so the gesture provably moved nothing. Against the previous line it fails ("a proven-inert gesture must still be reported after the capture backend falls back"); with the fix, 21/21. An earlier draft of that test used identical node content for both backends and passed against the unfixed code — vacuous, since the whole defect is that the two views differ. Recorded here because the green run looked identical either way.
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
Not ready — one P1 correctness issue and one scope issue remain:
The intended same-backend rebase fix and red-before test are otherwise sound. Most CI is green; iOS Smoke is still in progress. Do not apply |
Review P1 on #1622, and a false-positive path the previous commit introduced. `markPostGestureStabilization` records `baselineSignature: []` when the session has no pre-gesture snapshot. `decidePostGestureStabilityVerdict` already guards that correctly (`!baselineSignature?.length` -> 'trust'), but the backend-rebase branch tested plain truthiness, and `[]` is truthy. So the first capture from a different backend replaced "no before-state" with a post-gesture signature — inventing a baseline. That was harmless while corroboration read `pending.baselineSignature` (still `[]`, and `haveIdenticalDiscriminatingSurfaces` returns false on an empty side), but the previous commit pointed corroboration at the loop's live baseline, so the invented one became reachable: the loop could agree with itself and report a gesture inert with nothing to compare against. Rebase only a non-empty baseline, matching the guard the verdict already uses. Red evidence: with no `session.snapshot` and steady private-AX captures, restoring the truthiness test fails the new case ("a no-effect claim needs a real pre-gesture baseline, never one the loop invented for itself"); 22/22 with `?.length`.
|
Both addressed on P1 — correct, and it was a false-positive path this PR introduced. That was inert while corroboration read Now Red evidence, as requested — no Scope — fixed. Body now reads Gates: typecheck / lint / format clean, 22/22 on the touched suite. 🤖 Addressed by Claude Code |
|
Attempted the post-fix live check at exact head The capture reports: {"state":"recovered","backend":"private-ax",
"reason":"timed out while preparing tree snapshot on the XCTest main thread",
"reasonCode":"capture-failed","effectiveDepth":56}Tree capture times out on the first attempt here, so every capture in the session — pre-gesture baseline included — is already Two further facts from the same run, both narrowing #1620:
So the current evidence for this change is the unit level, where it is genuinely red-before-green on both findings: the backend-flip case fails against the old corroboration line, and the empty-baseline case fails against the old truthiness test. Your call on how to weigh that. If a recorded live flip is required before merge, the honest next step is (2) above — make the stabilization verdict observable on the gesture path first, then stage a screen that degrades mid-sequence. That is a separate change from this one, and I would rather do it under #1620 than grow this PR. Leaving as draft. 🤖 Addressed by Claude Code |
|
Re-reviewed exact head Still not |
Thermo-nuclear code quality reviewThe diagnosis of the defect is right, the writeup is exemplary, and the P1 catch (empty-baseline rebase) is real. But I can't approve this as-is: the corrected corroboration is vacuous after a rebase, and it now produces a confirmed false positive — the no-effect warning fires on a successful scroll whenever the capture backend flips. Runnable counterexample below; it passes on Blocker: post-rebase, the claim no longer measures the gestureAfter the loop rebases, Falsification test, using the codebase's own successful-scroll fixture (whose comment in test('a successful scroll that flips the capture backend must not claim no-effect', async () => {
vi.useFakeTimers();
const session = makeSession('ios');
// Pre-gesture baseline from the TREE backend: rows 1-2.
session.snapshot = makeSnapshotState(chromeWithListSnapshot(['row-1', 'row-2']).nodes, {
snapshotQuality: { state: 'healthy', backend: 'tree' },
});
markPostGestureStabilization(session, 'scroll', ['down']);
// Post-gesture captures from PRIVATE-AX: rows 3-4 — the scroll worked and
// the screen has settled, so every capture is identical to the others.
const capture = vi.fn(async () =>
makeSnapshotState(chromeWithListSnapshot(['row-3', 'row-4']).nodes, {
snapshotQuality: { state: 'recovered', backend: 'private-ax' },
}),
);
const resultPromise = withDiagnosticsScope({}, async () =>
capturePostGestureStabilizedResult({ session, capture, readSnapshot: (s) => s }),
);
await vi.advanceTimersByTimeAsync(10_000);
const result = await resultPromise;
assert.equal(result.gestureNoEffect, undefined,
'the scroll swapped every list cell — a no-effect claim here is a false positive');
});Measured: fails on this branch ( Trace on this branch: quiet match at ~800ms → flip detected → rebase to rows-3-4 → next quiet matches classify The headline test can't see this because its "the gesture provably moved nothing" premise is fixture decoration: after the flip the code reads the tree baseline's backend only, never its content. Replace the pre-gesture nodes with anything at all and that test still passes — which is exactly the vacuity the PR description warns about in its own earlier test draft, one layer up. Severity: by the PR's own model, hostile screens flip on every gesture (that's why all five live attempts were vetoed pre-fix). Under that regime the false positive isn't an edge case — every successful scroll on a Bluesky-class screen pays the 3.5s distrust budget and then tells the agent it did nothing, steering it into raw-swipe workarounds against a working gesture. That's arguably worse than #1600's 40 wasted re-scrolls, because it corrupts the agent's model of actions that work. The code-judo remedy is smaller than the current diff. An honest claim requires an un-rebased baseline — and when no rebase happened, the live baseline is let baselineRebased = false; // set true in the rebase branch
...
const corroborated =
verdict === 'accept-stale' &&
!baselineRebased &&
haveIdenticalDiscriminatingSurfaces(pending.baselineSignature ?? [], current.signature);Yes, this reverts the headline behavior — deliberately: the claim is undecidable post-flip, and no restructuring changes that. If #1620's element-18 warning must fire on flip screens, the sound path is to make the flip rare, not the claim reckless: if the capture-plan penalty were sticky per session/screen, the next gesture's pre-gesture snapshot would already be private-ax, the pair comparable, and the warning honest from gesture 2 onward. Related open question worth answering before any variant ships: under the stated model the flip should only occur on the first gesture after a penalty arms (afterwards P1: the
|
|
Re-review at exact head Required: continue settling if desired but fail closed on the no-effect claim after any rebase; add the successful-scroll/backend-flip regression and prove it red on this head. Also remove the ambiguous dual-baseline shape: omit baseline fields when no pre-gesture signature exists, and use a tagged/atomic baseline state (or equally unambiguous API) so only genuine pre-gesture evidence can authorize the warning. All CI being green does not clear this production false claim, and the prior live run never exercised a backend rebase. Not ready; do not apply |
|
Closing this — the premise is wrong, and #1633 states why. Rebasing onto
That is correct and this PR is not. The rebase assigns The asymmetry I filed as a bug is the design:
@thymikee's P1 here was the same defect in its most visible form: with no pre-gesture snapshot the rebase invents a baseline out of a post-gesture capture. My Consequence for #1620: the first of the two candidate vetoes is not a veto, it is the feature. A backend flip should suppress the claim. I will correct the issue to reflect that, leaving truncation drift as the only open question, and note that the observed unreachability on Bluesky is partly just correct behaviour — that screen never has a comparable pre-gesture baseline because tree capture fails before the first snapshot. I will also correct the comment I left on #1615, which repeated this wrong reading. No code from this branch should land. Closing rather than reworking. 🤖 Addressed by Claude Code |
1 participant
Part of #1620 — the truncation-drift half stays open.
The defect
capturePostGestureStabilizedResultkeeps a local baseline that it rebases when the capture backend changes mid-poll. Backends disagree about which nodes exist, so a cross-backend pair says nothing about the gesture — #1569 adopts the new capture as the baseline rather than concluding from it, and the verdict is computed from that comparable pair.buildAcceptedStabilizedResultthen corroborated againstpending.baselineSignature: the original pre-gesture signature, the one the loop had just discarded as incomparable. So on any backend fallback the agent-facing claim re-introduced exactly the comparison #1569 exists to forbid — andhaveIdenticalDiscriminatingSurfacesdemands set equality, which two backends' views of one screen never satisfy.Net effect: the
gestureNoEffectwarning could not fire on any screen whose capture plan falls back. That is every hostile screen — the only kind it was built for. #1600's motivating case, element-18, was a Bluesky feed; measured live on a seeded Bluesky fixture the capture falls back to private-ax and truncates at depth 56, and no no-effect gesture produced the warning across five attempts on two screens.The fix
Corroborate against the loop's live
baselineSignature. With no rebase it ispending's, so #1601's veto is unchanged in the ordinary case; after a rebase it is the comparable same-backend capture, which is the only pair either check should read.Red evidence
The new test models what the two backends actually return for one screen — private-ax additionally reports a scrolled-away row the tree backend prunes — with every post-gesture capture identical to the others, so the gesture provably moved nothing. Against the previous line it fails with "a proven-inert gesture must still be reported after the capture backend falls back"; with the fix 21/21.
An earlier draft of that test was vacuous and is worth knowing about: it used identical node content for both backends, so it passed against the unfixed code, and the green run looked no different from a real one. The defect is precisely that the two views differ, so the fixture has to differ too.
Not fixed here
Truncation drift is the second candidate veto from #1620 and is untouched. A depth-capped tree may still fail set equality against its own baseline; whether that happens in practice needs the live re-run this fix unblocks. #1620 stays open until that is measured.
Gates
check:affected --rungreen on this commit: 132 files / 1063 tests, "all runnable checks passed". Daemon suite 92 files / 682 tests. typecheck / lint / format / check:layering green.One process note: a later re-run failed on
android-lifecycle(~15s timeout, the flake this repo sees under load) and myvitest | grep && git pushchain pushed anyway, because a pipeline returns grep's status. Wrong chain on my part. Re-checked afterwards: that test passes 2/2 on this branch and 12/12 on an unrelated branch, and this change cannot reach Android —requiresPostGestureBaselineDistrustis Apple-only, sobaselineSignatureisundefinedthere and the corroboration returns false either way.