Skip to content

The sync gate's hidden-pick sentence consults the policy store (LLP 0289) - #945

Open
philcunliffe wants to merge 4 commits into
masterfrom
fix/issue-935
Open

The sync gate's hidden-pick sentence consults the policy store (LLP 0289)#945
philcunliffe wants to merge 4 commits into
masterfrom
fix/issue-935

Conversation

@philcunliffe

@philcunliffe philcunliffe commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Closes the deferred review finding triaged out of PR #925 (reported in both review rounds: round 1 finding 3, round 2 finding 2). One behaviour fix with regression tests, plus the design change it needed: new decision doc LLP 0289, with an Extended-by: forward-ref appended to LLP 0276 §no-candidates (Accepted, so nothing it settled is edited).

The finding: the sync gate's hidden-pick sentence did not consult the policy store

src/core/cli/wizard/sync_scope.js

On the empty-candidate path with no visible org row, the lane printed

You picked nothing to record, but capture already set up on this machine still syncs to your server.

whenever candidatesHidden > 0. That count only says a display-filtered picked row exists; the sentence claims it ships.

Root cause

A hidden row is off every wizard screen (LLP 0202, LLP 0276) but it is still a picker source with an id, and hyp policy client raw-anthropic local-only addresses it by that id. The entry lands in the client-sync store, which is exactly what the export seam reads: buildSourceWithholdResolver withholds optedOutClientSourceIds(entries) minus the central-classified ids (src/core/runtime/source_withhold.js:129-165). The lane's only store-derived set, optedOutBefore, is computed over candidateIds, i.e. the visible candidates (sync_scope.js:80-81), so a hidden id was invisible to it. A user with a standing opt-out was told their capture syncs while the seam was already withholding all of it, on a privacy-facing screen (LLP 0188 #never-silent).

Why it needed an LLP first

The count is LLP 0276 #no-candidates' own design: "They are counts, not lists: the lane must be able to tell the truth about them without being able to name them." With no id there is nothing to look up, so the fix is a design change, which is why PR #925 deferred it.

LLP 0289 §ask-the-store settles it: the hidden picks cross into the lane as ids, and the lane spends them on the store rather than on the screen. Naming a hidden row stays forbidden (asserted by tests, before and after); what the lane gains is the ability to ask about a row it may not print. A privacy screen denied the identity of the thing it describes cannot check its own claim, and a wrong claim costs more (LLP 0188 #never-silent) than the lane holding an id it never renders (LLP 0202).

The hidden locked rows keep their count, deliberately: an org row always syncs (LLP 0188 #locked) and the export seam drops opt-out entries for central-classified sources, so a store entry for one is inert and that sentence is unconditional because the fact is. The asymmetry between the two fields is the asymmetry between the two kinds of row.

The rejected alternative is recorded too: having the orchestrator pre-answer the question keeps the count but makes the wizard a second reader of a privacy store the lane already reads, with a different failure mode (the lane degrades a corrupt store to a warning and a skipped step; a read in the orchestrator would throw out of the whole run).

The fix

  • RunWizardSyncScopeOptions.candidatesHidden (count) becomes candidatesHiddenIds (src/core/cli/wizard/types.d.ts); lockedHidden is unchanged.
  • src/core/cli/wizard/index.js passes the ids the display filter dropped from picked.descriptors.
  • runWizardSyncScope reads the store's full opted-out set (not only the slice covering the visible candidates) and fires the sentence only when at least one hidden pick has no standing local-only entry. Every hidden pick withheld means nothing leaves the machine, so the branch reads the fourth sentence, "You picked nothing to record, so nothing syncs to your server.", which is now the true one. The check is "any hidden pick ships", never "every one does".

The visible-candidate path is untouched: optedOutBefore still governs what the gate and menu render and what the write keeps, because those screens edit only what they show. The path stays noQuestion and writes nothing, so LLP 0191 #back-edges and the step counter are unaffected.

The other branch the issue names (sync_scope.js:126)

The issue asks for one change covering both :107 and :126. :126 is LLP 0281 §visible-org-row, which is on PR #925's branch and not on master, so it is not in this diff. This PR mints the candidatesHiddenIds answer that branch needs; when #925 lands, its condition consults the same boolean and the two branches keep agreeing. Recorded in LLP 0289 §not-done, and the merge point is the same few lines, so the conflict resolution is mechanical.

Testing

The load-bearing regression test drives the real sync lane through runInitWizard, so it asserts on what the user reads rather than on the lane's inputs and is therefore independent of the options-shape change (a lane-level test written against the new field would pass vacuously on pre-fix code, which ignores the field).

test file before after
runInitWizard: a hidden picked row with a standing opt-out does not make the lane claim capture syncs test/core/cli/wizard/index.test.js fails passes
runInitWizard: a hidden picked row with no opt-out keeps the sentence that says capture still syncs same passes (boundary pin) passes
zero visible candidates with a hidden picked row already opted out: says nothing syncs test/core/cli/wizard/sync_scope.test.js n/a (new field) passes
zero visible candidates with one hidden pick opted out and one standing: does not claim nothing syncs same fails on the pre-fix lane passes
a stale opt-out for a hidden locked row does not soften the fleet sentence same passes (pins the deliberate asymmetry) passes

The failing-before observation, at origin/master with only the test added:

not ok 23 - runInitWizard: a hidden picked row with a standing opt-out does not make the lane claim capture syncs
  error: 'the only standing pick is withheld by the store, so nothing ships'
  actual: |-
    Step 3 of 5 · Choose what syncs
    You picked nothing to record, but capture already set up on this machine still syncs to your server.

After the fix that line reads You picked nothing to record, so nothing syncs to your server. Re-verified by reverting sync_scope.js and index.js in place and re-running the two files.

Every case also asserts the withheld row is still never named, opted out or not.

  • npm test: 4533 pass / 0 fail / 1 skipped.
  • npm run typecheck: clean.

Fixes #935

test and others added 2 commits August 19, 2026 11:44
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… the hidden-pick decision in the span

Self-review of PR #945 found two items.

1. medium, recorded not fixed: `hiddenCandidateSyncs` reads the client
   policy store, but the export seam withholds
   `optedOutClientSourceIds(entries)` minus the central-classified ids.
   A hidden pick is non-central by construction only while the wizard's
   locked set resolved, and `computeLockedSafe` (LLP 0182) and a converge
   timeout (LLP 0129) both return `[]` on purpose. On such a run a hidden
   central row with a stale opt-out reads as withheld and the lane prints
   "nothing syncs" while the daemon exports it. Closing it needs the lane
   to tell "no central rows" from "could not tell", a signal the
   orchestrator does not send; that is a design change, so LLP 0289
   #not-done records the corner and the code says which case it is.

2. low, fixed: the branch the store read decides was invisible in
   telemetry. `wizard.sync_scope.finish` now carries `hidden_picks` and,
   on the no-candidates path, `hidden_picks_syncing`, so "it said nothing
   syncs but rows shipped" is triageable from the signal. Counts and a
   boolean, never the ids.

npm test 4533 pass / 0 fail; npm run typecheck clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@philcunliffe

Copy link
Copy Markdown
Contributor Author

Review record: PR #945 @ 3f669d2e

Verdict: findings (2). One low finding fixed and pushed; one medium finding is real but needs a design signal that does not exist, so it is recorded rather than smuggled in. Follow-up commit: 41b312ca.

Baseline on 3f669d2e: npm test 4533 pass / 0 fail / 1 skipped, npm run typecheck clean, npm run smoke -- walkthrough_picker_to_first_query ok.


Finding 1 - medium - src/core/cli/wizard/sync_scope.js:94 - the lane's "does it ship?" test is the store's answer, not the seam's

hiddenCandidateSyncs treats a local-only entry as decisive. The seam it speaks for does not:

// src/core/runtime/source_withhold.js:163
withheld = new Set(optedOutClientSourceIds(entries).filter((id) => !centralIds.has(id)))

An opt-out entry for a central-classified source is inert - the row ships. LLP 0289 assumes that cannot reach candidatesHiddenIds, because lockedSources is the central set (src/core/cli/wizard/join.js:141) and picked.descriptors is the non-locked slice. That equation holds only while the wizard's locked set actually resolved, and two paths return an empty one on purpose:

  • computeLockedSafe (src/core/cli/wizard/index.js:1104) swallows a layered-resolution failure and returns [] ("the export seam, not the picker, enforces the org boundary", LLP 0182).
  • a converge timeout returns { status: 'ok', lockedSources: [] } (src/core/cli/wizard/join.js:112, LLP 0129).

Concrete run: the user writes hyp policy client raw-anthropic local-only before the org adopts that source (allowed - src/core/commands/policy.js:515 blocks the write only once the source is already central, and adoption never clears an existing entry). The org later adopts it, so it is central and hidden. On a re-run whose locked resolution fails, locked = [], lockedHidden = 0, the carried hidden row lands in candidatesHiddenIds, optedOutAll contains it, hiddenCandidateSyncs is false, and the lane prints "You picked nothing to record, so nothing syncs to your server." while the daemon - resolving the same config successfully at boot - drops the inert entry and ships every row. Pre-PR that run printed the true "still syncs" line, so it is a narrow regression in the LLP 0188 #never-silent direction.

Not fixed here, deliberately. Both obvious repairs fail on the same rock: the wizard cannot subtract central ids on the very run where computing them is what failed, and the lane cannot substitute "no locked rows at all" for it - that is also the healthy shape of an enrolled machine whose org configures nothing, i.e. the exact case LLP 0289's main test covers, so the fix would undo the fix. Closing it needs the orchestrator to distinguish "no central rows" from "could not tell", a signal that does not exist today and whose introduction is a design decision about the degraded path, not an implementation detail of this one.

So it is recorded the way this PR already records the LLP 0281 overlap:

  • LLP 0289 §not-done gains a paragraph naming the exception, both paths that produce it, why the corner is accepted for now (it needs a resolution failure and a stale entry and a hidden central row at once, where the bug this PR fixes needed only one hyp policy client invocation), and what closing it would require.
  • sync_scope.js says at the call site which answer it is holding and points at that section, so the next reader does not have to re-derive the gap.

Worth a follow-up issue if anyone wants the degraded path made honest.

Finding 2 - low - src/core/cli/wizard/sync_scope.js:293 - the new branch decision was invisible in telemetry - fixed

finishSpan recorded candidates, sources_opted_out, and status, none of which separate the four sentences the empty-candidate path can print. This PR adds a fifth input and a store read that chooses between two of them, so a report of "the wizard told me nothing syncs but rows shipped" could not be triaged from the emitted signal - hidden_picks: 0 and hidden_picks: 2, all withheld print the same line. Against the repo's log-driven-development guidance.

wizard.sync_scope.finish now carries hidden_picks (count, on every return) and hidden_picks_syncing (boolean, on the no-candidates return the store read governs). Counts and a boolean, never the ids - the lane holds those to ask the store, not to record them (LLP 0202).


Checked and cleared

  • No dangling candidatesHidden: one producer (index.js:561), one consumer, no other reference in code, tests, or smokes; typecheck clean.
  • picked.descriptors.filter((d) => !visibleCandidateIds.has(d.id)) is the exact complement - visiblePickerDescriptors filters on d.hidden !== true and nothing else (src/core/cli/walkthrough.js:2251).
  • optedOutBefore (raw entry membership) and optedOutClientSourceIds (class === 'local-only') cannot disagree: parseListShape rejects any other class (client_sync.js:17,93), so the two store reads in the lane are consistent.
  • optedOutAll is computed after the ClientSyncListUnreadableError guard, so the corrupt-store skip is unchanged; a missing store still reads as "nothing opted out" and reaches the pre-PR sentence.
  • The write path is untouched: kept = existing.filter((e) => !candidateIds.has(e.source)) still preserves entries for hidden picks, so the lane never clears an opt-out it cannot show. sourcesOptedOut is telemetry-only in index.js, so the noQuestion return writes nothing.
  • Anchors 0289#ask-the-store, 0276#no-candidates, 0276#sync-gate, 0202#carry-through, 0192#fail-closed, 0188#locked all resolve. 0188#opt-out, 0188#never-silent, 0188#migration, 0191#back-edges do not exist as anchors, but those refs are pre-existing repo-wide including in untouched parts of this file - not this PR's regression.
  • LLP 0289 does not collide (highest on the branch is 0277), and the Extended-by: blockquote added to LLP 0276 §no-candidates matches the form used elsewhere in the corpus.
  • Conventions: no semicolons, no em dashes, JSDoc types only, no inline import('...'), no @typedef.

After the follow-up commit: npm test 4533 pass / 0 fail / 1 skipped, npm run typecheck clean.

test and others added 2 commits August 20, 2026 15:05
Master landed PR #925 (LLP 0281 #visible-org-row), which added the sibling
no-candidates branch this PR's LLP 0289 #not-done anticipated. Both sides
touch the same few lines of the sync lane, so the merge is where the two
branches either agree on one fact or drift apart.

Resolution:

- `src/core/cli/wizard/sync_scope.js`: master's org-row branch gated on
  `candidatesHidden > 0`, the count this PR replaces with
  `candidatesHiddenIds`. It now reads the same `hiddenCandidateSyncs`
  boolean its no-locked sibling reads, which is exactly what LLP 0289
  #not-done said it should do when #925 landed. A hidden pick the store
  already withholds does not ship, so the exhaustive fleet sentence is true
  again and the two branches turn on one question rather than disagreeing
  about it. Both branches of that fork now record `hidden_picks_syncing` on
  the span.
- `llp/0276-...decision.md`: both sides appended an `Extended-by:` note to
  #no-candidates. Both are kept, master's (LLP 0281) first.
- `test/core/cli/wizard/sync_scope.test.js`: both sides' tests are kept,
  master's two ported to `candidatesHiddenIds`, plus a new pin for the case
  the merge creates - an org row beside a hidden pick already opted out
  keeps the exhaustive sentence. That pin fails on the mechanical rename
  and passes on this resolution.
- `llp/0281-...decision.md`: forward-ref to LLP 0289 #ask-the-store on
  #visible-org-row, since its branch now carries that `@ref`.
- `llp/0289-...decision.md`: #not-done records that LLP 0281 has landed and
  the residual is closed.

npm test: 5017 pass / 0 fail / 1 skipped. npm run typecheck: clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e it is

Self-review of the master merge found one finding, fixed here.

medium: the merge re-gated LLP 0281 #visible-org-row's branch on
`hiddenCandidateSyncs`, a question about shipping, when what that
Accepted decision settled was a question about ownership. It says so
itself: "no clause here is affirmatively false about *what* leaves the
machine - which is why round 2 left it", and the rule it adopted is "a
sentence names only the rows whose owner it names."

So on an enrolled machine with a visible org row, a hidden carried pick
(LLP 0202 #carry-through), and a standing `hyp policy client
raw-anthropic local-only`, the merged lane printed "Everything you
picked is managed by your fleet and always syncs." over a row the fleet
does not manage and that does not sync - the exact claim #925 had just
removed from master, restored by this branch's merge and pinned by a new
test.

The branch now splits the two claims:

- the fleet sentence narrows whenever a hidden pick exists, withheld or
  not, which is what LLP 0281 settled and what master does today
- the second line, the one that promises an export, is the store's
  answer and prints only when a hidden pick is not already withheld,
  the same question the no-locked sibling asks

So the two branches still agree about what leaves the machine, without
this one taking back an owner's claim it had given up. The withheld row
is still never named.

LLP 0289 #not-done and #consequences record which half of that branch
the store governs; the `Extended-by:` blockquote this PR appends to
LLP 0281 #visible-org-row is corrected to match. Nothing LLP 0281
settled is edited.

The reworked test fails on the pre-fix lane ("Everything you picked is
managed by your fleet and always syncs." where none of it is the
fleet's) and passes after.

npm test: 5016 pass / 0 fail / 1 skipped (the one failure in this
sandbox, `hyparquet-floor-pin`, reproduces unmodified at 9549f38 and is
a shared-node_modules artifact). npm run typecheck: clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@philcunliffe

Copy link
Copy Markdown
Contributor Author

Review record: PR #945 @ 9549f384 (the master merge)

Verdict: findings (1 medium). Fixed and pushed as a4d46700. The round's subject is the merge commit 9549f384, which resolved this branch against PR #925 landing on master; 3f669d2e and 41b312ca were reviewed in the previous round.

Baseline at 9549f384: npm test 5016 pass / 1 fail / 1 skipped, npm run typecheck clean. The single failure is test/core/hyparquet-floor-pin.test.js ("the read path resolves the one root hyparquet"), which reproduces byte-identically on the unmodified head SHA in this sandbox. It reads the installed node_modules layout (a nested hypgrep/node_modules/hyparquet@1.27.1 below the LLP 0222 floor) and is independent of every file in this diff. Not a finding against the PR; CI is the authority.


Finding 1 - medium - src/core/cli/wizard/sync_scope.js:152 - the merge re-gated LLP 0281's branch on the wrong question, restoring the claim #925 had just removed - fixed

The merge pointed the org-row branch at hiddenCandidateSyncs, the same boolean its no-locked sibling reads, and the merge commit message and LLP 0289 §not-done both justify that as the two branches agreeing on one fact. They are not one fact. That branch prints two claims, and only one of them is the policy store's to answer.

LLP 0281 §visible-org-row is explicit that the claim it settled is about ownership, not shipping:

Unlike the cases LLP 0276 fixed, no clause here is affirmatively false about what leaves the machine - which is why round 2 left it. But the fix is the same rule that doc already applies twice [...] a sentence names only the rows whose owner it names.

An opt-out entry settles whether a carried pick ships. It does not make that pick the fleet's row. Gating the narrowing on the store therefore hands the fleet back exactly the owner's claim LLP 0281 exists to prevent, over a source the user had explicitly marked local-only.

The concrete run. Enrolled machine; the org locks a visible claude row; raw-anthropic survives as a hidden carried pick (LLP 0202 #carry-through); the user has previously run hyp policy client raw-anthropic local-only. Then hiddenCandidateSyncs is false, line 152's branch is skipped, and the screen reads:

Everything you picked is managed by your fleet and always syncs.
  capture claude

raw-anthropic was picked, is not fleet-managed, and does not sync. On master today (sync_scope.js:124, gated on candidatesHidden > 0) that run correctly prints the narrowed pair, so this is a regression the merge introduced against behaviour that had just landed, and the new test at test/core/cli/wizard/sync_scope.test.js:578 pinned the regressed wording in.

It is not an LLP 0188 #never-silent break (the error over-claims syncing rather than under-claiming it), which is why medium and not high. But it is a false sentence on a privacy screen, and it silently undoes an Accepted decision during a conflict resolution, which is the failure mode a merge review is for.

Fix. The branch now splits the two questions instead of collapsing them:

if (hiddenCandidates.length > 0) {
  opts.stdout.write('Your fleet manages these and they always sync:\n')
  for (const d of opts.locked ?? []) opts.stdout.write(`  ${d.label}\n`)
  if (hiddenCandidateSyncs) {
    opts.stdout.write('Capture already set up on this machine also syncs to your server.\n')
  }
  ...
  • the fleet sentence narrows whenever a hidden pick exists, withheld or not: LLP 0281's rule, and what master does today
  • the second line, the one that promises an export, is the store's answer and prints only when a hidden pick is not already withheld: the same question the no-locked sibling asks

So the two branches still agree about what leaves the machine, without this one taking back an owner's claim it had given up. That is a strictly better agreement than the merge's, since the withheld-pick case now says neither a false ownership thing nor a false shipping thing. The row is still never named, opted out or not (asserted).

Docs. LLP 0289 §not-done's "it has since landed" paragraph and §consequences now record which half of that branch the store governs, and the Extended-by: blockquote this PR appends to LLP 0281 §visible-org-row is corrected to match. Both are edits to text introduced by this unmerged PR, so nothing an Accepted doc settled is touched: LLP 0281's own prose is unchanged, and LLP 0289 has never been on master.

Test. test/core/cli/wizard/sync_scope.test.js:578 is reworked from pinning the exhaustive sentence to pinning the split. Verified failing on the pre-fix lane by restoring sync_scope.js from 9549f384 in place:

not ok 1 - zero visible candidates with an org row and a hidden pick already opted out: drops the sync line, keeps the narrowed fleet sentence
    The input was expected to not match the regular expression /Everything you picked is managed by your fleet/. Input:
      'Everything you picked is managed by your fleet and always syncs.\n'

and passing after.


Checked and cleared on the merge specifically

  • No master content lost. git diff HEAD origin/master at 9549f384 touches exactly the 8 files this PR owns and nothing else, so the merge dropped nothing from master.
  • No stale candidatesHidden. No occurrence outside LLP prose (where it is correct as history) in any .js, .ts, test, or smoke. Typecheck clean, so no caller silently passes a number into the renamed field.
  • Branch ordering intact. lockedHidden > 0 is still tested first; the ClientSyncListUnreadableError guard still precedes the new optedOutClientSourceIds(existing) read, so the corrupt-store skip is unchanged; a missing store still reads as nothing opted out (LLP 0188 #migration).
  • The write path is untouched. kept = existing.filter((e) => !candidateIds.has(e.source)) still preserves entries for hidden picks, and the path stays noQuestion (LLP 0191 #back-edges, step counter unaffected).
  • optedOutClientSourceIds is a real export of src/core/usage-policy/index.js, and cannot disagree with optedOutBefore: parseListShape rejects any class but local-only.
  • The lockedHidden-keeps-a-count asymmetry is factually backed. src/core/runtime/source_withhold.js:163 does drop central-classified ids from the withheld set, so a stale entry for an org row really is inert.
  • Anchors 0289#ask-the-store, 0289#not-done, 0276#no-candidates, 0276#sync-gate, 0281#visible-org-row, 0202#carry-through, 0188#locked, 0192#fail-closed all resolve. LLP 0289 does not collide with anything on master.
  • Conventions: no semicolons, no em dashes, JSDoc types only, no inline import('...'), no @typedef.

Coverage of issue #935

Both branches the issue names are now closed by this PR. sync_scope.js:107 (the no-locked branch) was closed by 3f669d2e; :126 (LLP 0281's org-row branch, which reached master via #925 after the issue was written) is closed by this round: its Capture already set up on this machine also syncs to your server. line no longer prints unconditionally on a hidden pick, and the fleet sentence beside it keeps the narrowing LLP 0281 gave it. Nothing the issue lists is dropped.

Standing, not new

The previous round's medium (the export seam withholds optedOutClientSourceIds(entries) minus the central-classified ids, so a hidden central row with a stale opt-out reads as withheld on a run where computeLockedSafe or a converge timeout returned []) is unchanged and still recorded in LLP 0289 §not-done rather than fixed. Re-confirmed as a genuine design gap needing a signal the orchestrator does not send: worth its own issue, not this PR.

After the fix, at a4d46700: npm test 5016 pass / 1 fail (the pre-existing sandbox hyparquet-floor-pin, identical at 9549f384) / 1 skipped; npm run typecheck clean.

@philcunliffe

Copy link
Copy Markdown
Contributor Author

Deferred residual findings tracked in #970.

Triage at head a4d4670046cf94aa1868c6b13065d9aa6ab033b9: the one unresolved finding (round 1 finding 1, re-confirmed standing in round 2) is a documented design gap, not a merge blocker. It needs a resolution failure, a stale opt-out entry, and a hidden central row at once, and closing it requires a "no central rows" vs "could not tell" signal the orchestrator does not send today, i.e. a design decision (LLP 0289 §not-done), not a fix this PR can contain. The unconditional wrong claim this PR removes needed only one hyp policy client invocation, so shipping is the safer side of the trade. All CI checks green, PR mergeable.

@philcunliffe
philcunliffe marked this pull request as ready for review August 20, 2026 16:39
@philcunliffe philcunliffe added the neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030) label Aug 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Follow-up: deferred review findings from PR #925

1 participant