Skip to content

Follow-up: deferred review findings from PR #626 #628

Description

@philcunliffe

Deferred, non-blocking findings from the round-2 review and triage of PR #626 (the LLP 0063 D4 exclusivity gate fail-closed fix, fix/issue-623). Triage confirmed none of these are production defects; they are worth their own follow-up rather than blocking or re-widening #626 further.

1. hyp status collapses an unreadable/unresolvable central layer into hasCentral: false

src/core/daemon/status.js:265-268:

const centralConfigPath = resolveCentralLayerPath({ stateRoot })
const centralLoaded = centralConfigPath ? await loadConfigFile(centralConfigPath) : null
const centralConfig = centralLoaded?.ok ? centralLoaded.config : null
const hasCentral = centralConfig !== null

This collapses three distinct states (no central layer at all, a central layer that fails to parse, and a central-layer path that cannot even be resolved) into a single false. After #626, hyp remote login correctly refuses in the latter two states with "this machine's central config layer ... cannot be read", but hyp status reports hasCentral: false for the exact same machine state, i.e. the two surfaces disagree about whether this machine has a central layer. Every consumer of hasCentral (buildClientActionsReport, the layered report block, the wizard's managed hint) is display/reporting, not a permission gate, so this is confusing rather than unsafe. Fix: give hyp status its own three-state read (mirroring CentralEnrollment/centralLayerResolutionFailure) with its own message and tests.

2. enrollCentralSink's seed-time D4 recheck throws after the first-sync hold marker is written

src/core/commands/central.js:209-212 (the seed-time D4 recheck) can throw before anything is written for this enrollment, but the attended-login caller (src/core/cli/remote_commands.js:720-725) writes the first-sync hold marker (LLP 0101) before calling enroll(...), so a throw here (like every other throw in enrollCentralSink) leaves that marker on disk with no clearing enrollment behind it. This is pre-existing behavior, not introduced by #626, and is bounded by LLP 0101's own design (best-effort marker, no clear-on-exit, an absolute deadline caps how long a stray hold can last) — it delays exports, it does not leak or lose data. Noted for completeness; no action required unless LLP 0101's bound is revisited.

3. Informational: the new fail-closed helper's contract is easy to violate by construction

centralLayerResolutionFailure in src/core/config/apply.js documents (in its own JSDoc) that any future permission check keyed on resolveCentralLayerPath() === null reopens the exact fail-open hole #626 just closed, because resolution is deliberately lossy for every other caller (boot, hyp status). This is already documented in-code; flagging here only so a future reviewer auditing a new central-layer-gated feature knows to grep for this note.


Backlink: #626

Metadata

Metadata

Assignees

No one assigned

    Labels

    neutral:fixDelegate this issue to neutral for an autonomous fix attempt (reproduce -> fix -> PR)neutral:stuckneutral attempted this but cannot complete it autonomously — needs a human

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions