Repository navigation
fix(T-057): MCP consent toggle shows the real grant — write-then-reread, and re-read on mount - #10
Closed
aaroncoville wants to merge 6 commits into
Closed
aaroncoville wants to merge 6 commits into
aaroncoville wants to merge 6 commits into
Conversation
McpDefaultsSettings.toggle() persisted correctly and then rendered from a stale prop, so the control kept reading OFF while the write had landed. The green note was truthful because it was built from the intended value; the checkbox was not. For a consent control this is worse than a cosmetic bug: a human can reasonably conclude the grant failed and click again, silently revoking consent while seeing the same confirmation text. Extracts two pure functions to mcpToggleLogic.ts so the behaviour is testable without React test infrastructure, which this repo does not have: - resolveEnabledFor(mcpDefaults, id, catalog) — the derivation, which was always correct; the catalog is injectable so a test uses a fixture rather than sharing the real constant with the implementation. - applyToggle(id, next, current, deps) — persist, then RE-READ and return what actually landed. Deliberately not optimistic. The assertion that matters: getConfig reports enabled:false while next was true. An optimistic implementation returns true and fails. Verified it bites before the fix went in. 7/7 in the new file. Suite 592, 591 pass — the one failure (model picker options stay provider-specific) is pre-existing, confirmed against main's own baseline of 585/584.
The T-057 tests cover applyToggle/resolveEnabledFor. Nothing covered McpDefaultsSettings itself: comment out the setMcpDefaults call and all six of them stay green while the reported bug returns. Four defects have shipped here through exactly that hole. test/render-hooks.cjs is a ~60-line React host (seeds require.cache for react + react/jsx-runtime, the electron trick from harness-home-tilde) so a component can be mounted, clicked and re-rendered under node:test with no jsdom and no react-dom. load-ts learns JSX for .tsx. Five of the six new tests pass — the fix is sound where it is covered. The sixth is RED: the granted state survives closing and reopening the panel 'off' !== 'on' useState seeds mcpDefaults from the config prop on MOUNT only. Settings renders McpDefaultsSettings only while activeSection === 'Connections', and its config prop is App's, loaded once at start-up and never refreshed after a save (SettingsModal.tsx:444 says so in a comment). Switch sections and back and the panel remounts against a config that still says the grant never happened — the write is on disk and the control shows off. That is the reported symptom, verbatim.
GREEN for 'the granted state survives closing and reopening the panel'.
The toggle seeded its state from the config prop on mount only. That
prop is App's, loaded once at start-up and never refreshed after a save,
and SettingsModal mounts this panel only while the Connections section
is open. Toggle hive-memory on, switch section, switch back: the panel
remounts against the stale prop and shows 'off' for a grant that is on
disk. The write landed and the control shows off — the reported bug,
one remount later.
So do on mount what SettingsModal.tsx:444 already does for its own
fields: ask the disk. A consent control has to read the grant, not
remember it.
Break-it check, both directions:
- drop the useEffect -> 'the granted state survives...' RED
- make the component optimistic instead of using applyToggle's
return -> 'the label refuses to show a grant the disk did not
accept' RED, while all 6 pre-existing mcp-toggle-state tests stay
GREEN. That is the hole these component tests exist to close.
Also fixes an unrealistic stub in the failed-write test: its getConfig
reported a grant that updateConfig had thrown on, so the disk contradicted
itself. It now returns {} and the test asserts the real property.
Completed by dwight-t057qa but uncommitted when the worker hit its 3M token cap and was reaped. Committed by god to preserve delivered work; authored by the QA hire, not by the integrator.
…ship as-is
Thesis REFUTED, with the invariant that forces agreement: main's armed-set is
a strict SUBSET of what the panel shows, because buildDefaultMcpServers
conjoins `tier==='safe-readonly' || consented===true` onto the SAME expression
resolveEnabledFor evaluates on the SAME map. armed(id) => on(id). The
dangerous direction (UI off / server armed) is impossible by construction,
independent of catalog contents.
Evidence: 88 combinations (11 catalog entries x 8 stored states) enumerated
against the real resolveEnabledFor and the real buildDefaultMcpServers. Zero
divergences in all 33 states reachable through the app. The 10 divergences all
need a hand-edited non-boolean and are all the benign direction — they are the
`consented !== true` gate firing.
Probe 2b: poisoned the catalog (github-token tier=secret, defaultEnabled=true)
and main still refused to arm. The second gate is load-bearing, not commentary.
So a discarded opt-out can only come back armed for safe-readonly ids, never
write/secret.
Three LOW/defense-in-depth items, none blocking, two pre-existing:
F1 the mount .catch fails stale rather than closed — UNREACHABLE (readConfig
cannot throw, handler registered at module load, payload always cloneable)
F2 pty:spawn does not tier-filter opts.mcpDefaults — no renderer call site
sets it today; needs renderer RCE, and is not an escalation
F3 partial-map opt-out loss — safe-readonly only, pre-existing
hive-memory id chain intact: the diff touches neither mcpCatalog.ts, hive.ts
nor control.ts.
typecheck 0 errors; 604/605 tests (the 1 failure is provider-config.test.cjs,
pre-existing and untouched by this diff).
Owner
Author
|
Closed per Aaron's decision on the restore-point review: behaviour fully preserved (verified by patch-id) in the branch that became PR #16, minus material not wanted upstream. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes PR #6. That PR carried only the original fix and had skipped the QA station; Aaron asked for it to be routed through QA first. It then went through QA and security, and QA found a second, deeper bug. Please close #6 in favour of this.
Your report: "it shows 'hive enabled' in green at the bottom but the toggle is never changed in the UI. It always shows off."
Two bugs, not one
Bug 1 — the control displayed intent, not reality.
toggle()calledupdateConfig()and then rendered the optimistic value. For a consent control that is backwards: it can tell you a grant succeeded when it silently reverted.applyTogglenow writes, re-reads from disk, and returns what actually landed.Bug 2 — found by QA, and this is the one you were seeing. The component seeded its state from App's
configprop on mount only. That prop is loaded once at start-up and never refreshed, andSettingsModalmounts this panel only while Connections is open. So: toggle on → switch section → switch back → the panel remounts against the stale prop and shows OFF for a grant that is genuinely on disk. The write landed and the control says off — your exact symptom, one remount later. Fixed by re-reading the grant on mount, mirroring whatSettingsModal.tsx:444already does.Pipeline
applyToggle/resolveEnabledForextracteddwight-t057qacreed-t057secdwight-t057qawas reaped at its 3M cap with finished, uncommitted work on disk. I committed it on its behalf, attributed to the hire. A reap kills the process, not the work.Security review
My thesis: the UI decides "enabled" via
resolveEnabledForwhile main decides what is actually armed viabuildDefaultMcpServers— two separate implementations of one question. A UI showing off while the spawn path arms the server would be a server running without consent, invisible to you.Refuted, structurally:
Main's armed-set is a strict subset of the UI's on-set.
armed(id) ⟹ on(id)— a server cannot be armed unless the panel shows it ON. The reverse is possible and is the benign direction.Notably this does not rest on
defaultEnabled === (tier === 'safe-readonly'). The reviewer deliberately poisoned that invariant and main still refused to arm: the safety rests on theconsented !== truegate athive.ts:1109, which is load-bearing rather than decorative. I verified that line myself.It enumerated every catalog entry × 8 stored states — 88 combinations — against the real methods rather than spot-checking.
Three LOW findings, none blocking, two pre-existing. Filed as follow-ups, not bundled here.
Verification (reproduced independently)
mainand is fixed by PR fix: palace single-writer guard (T-065), 10M default cap (T-068), unstale model-picker test (T-070) — suite now 607/607 #9.npm run typecheck— 0 errors.