docs(commands): command-system design proposal (commands as a core foundation)#250
Open
Stvad wants to merge 7 commits into
Open
docs(commands): command-system design proposal (commands as a core foundation)#250Stvad wants to merge 7 commits into
Stvad wants to merge 7 commits into
Conversation
Propose upgrading the action system into a typed, observable command substrate (commands as a core foundation, events as the reactive twin). Frames the four gaps that separate today's Action from a command — result channel (the only structural blocker), typed args vs deps, invocation-time middleware, one dispatch lifecycle — and the equation Command = Action's identity/binding/context union Verb's typed signature/middleware. Lays out the model, how commands relate to events, why the bespoke verb seams (navigationVerb/pasteDecisionVerb) fold into the generic bus, a return-channel-first migration path, and open questions. Design-only; no code changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…verb nav, trust) Revision pass after a 3-agent adversarial review. Findings addressed: - BLOCKER: promote synchronous dispatch to a first-class requirement (new section 6). Async-only dispatch breaks preventDefault, the sync not-handled fall-through, and currentTarget capture on the keyboard/ pointer/click hot path — the navigation work already had to carve out a sync escape around its async verb. Add dispatchSync + DECLINE. - MAJOR: correct the critical path to G1 -> G3 -> G4 -> G2 -> fold. G1 alone only buys 'dispatch returns a typed result'; retiring the verb needs G3 (a home for its middleware) + G4 (uniform application). - MAJOR: the false sentinel can't double as a typed Result; make the decline channel out-of-band (DECLINE). - MAJOR: navigation is TWO verbs (intent pure/fallback + execute effectful/rethrow); one command can't carry two onError policies, so the fold yields a resolve/apply command pair. - MAJOR: add trust/ordering/cost callout for untrusted-extension wildcard middleware; add an Alternatives-considered section (1.1) and drop the unsubstantiated 'single highest-leverage' superlative. - MINOR: fix events 'after is in-path' contradiction; correct caller count (~20 -> 60+); fix srs example (wraps isVisible, not handler); reword 'no data return channel'; add failure-surfacing/test open Qs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Split Command into discriminated run (async, dispatch-only) vs runSync (Result | DECLINE, chord/pointer-bindable); state + type-enforce the invariant that sync-dispatched commands have synchronous run, mirroring how ActionHandlerResult forbids Promise<false> today. - Wildcard middleware: async confirm-guards are async-path-only and would bypass keyboard-triggered commands; name the two-tier risk and require picking a protocol instead of advertising 'one guard for every command'. - Navigate fold: scope honestly to 2 of 5 nav seams (back/forward + URL-restore bypass the verbs via writePanelContent); the click already goes through the bus today and apply is async, so re-scope the win to G1/G2/G3 rather than 'uniform middleware on the sync click'. - Reconcile the (A)-vs-(C) stance: state recommendation (C) w/ (A) fallback; building past G1 commits to (C); decision owed before step 2. - Minor: §3 priority-vs-build-order clause; add §1.1 to the ToC. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ay be async Self-caught overcorrection from the round-1 fix: I'd written runSync as (i) => Result | DECLINE and 'chord-bound commands MUST be synchronous', which would forbid async keyboard effects (delete -> repo.tx) that are ubiquitous today. The real invariant mirrors ActionHandlerResult exactly: it forbids Promise<false>, NOT Promise<void>. So runSync forbids Promise<DECLINE> (the fall-through decision must be in-tick) but allows Promise<Result> (the effect runs async, unawaited by the coordinator). Signature -> typeof DECLINE | MaybePromise<Result>; §6 commitment reworded. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…utor; arg-gating) - S1: correct the paste claim. Paste is NOT gated on the sync-resolution task and is not a runSync command — its DOM handlers preventDefault eagerly and await the decision today, so it folds onto the async dispatch path (the easy case). The sync task gates keyboard-chord pure verbs, not the paste decision. (§6 footnote, §8 bullet) - S2: the 'middleware = verb facet, generalized' lift is literal only for the async face. VerbFacet.run is async-only and awaits the impl to build VerbOutcome for after; the no-await dispatchSync can't, so the sync path needs its own lighter executor with a degraded after (sync before/ decorator; invocation-only after). Stated explicitly. (§5) - F1: note canDispatch/isVisible are deps-only (not args) by design — they run at chord-resolution before args exist; arg-dependent gating goes in run/runSync. (§5 sketch) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…substantive findings) Two independent round-3 reviewers found the design coherent end-to-end with no substantive findings; applying the trivial polish they flagged: - G4 card: 'one lifecycle, two execution faces' instead of bald 'one pipeline' (the two-face reality is established in §5/§6/§9). - §7: note the 'after = in-path lifecycle hook' framing is async-face only; on dispatchSync the outcome isn't awaited, so a sync-path after sees invocation, not success/failure (cross-ref §5 + the §10 open Q). - §9: reword 'four dispatch entries' to 'dispatch-by-id call sites' to avoid clashing with §2's 'three entries' framing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…(B+) staging, G4 gated Adversarial re-review (design-soundness + ground-truth + quality lenses) found the (C) recommendation mispriced at the decisive comparison and three contract holes in the §5 sketch. Substantive changes: - §1.1: re-price (B) — both verbs retire on the async face under G1+G2+G3 (navigate.apply is irreducibly async; keyboard navigations route through navigate() internally). New recommendation: staged path, G4 + sync-face middleware behind a named gate. - §5: DECLINE representable on BOTH faces (no false→undefined-style coercion can reappear); run gains a DECLINE arm; typed-handles-first (by-string dispatch is the explicitly-untyped edge, not a fake generic — VS Code executeCommand<T> cited as the anti-pattern, in-repo CodeMirror StateCommand as corroborating prior art); veto needs its own VETOED outcome (sync DECLINE-veto would release the chord to the browser un-preventDefaulted); resolution named as lifecycle stage zero with the id-uniqueness invariant and the per-winner middleware granularity; args provenance for bound/palette commands stated. - §2: table corrected (pointer/gesture take supplied deps, no activation requirement, return handled-boolean) + agent bridge added as the fifth, dispatch-bypassing path with an untyped result channel; transform inventory completed (srs handler wraps, spatial description replacement). - §9: counts corrected (≈17 prod call sites, not 60+); critical path G1→G2→G3→folds with G4 gated; step 3 gains the coverage duty (hold fire / gesture progress / agent bridge bypass the shared loop) and the sync-deciding contract; step 4 fixes the paste contradiction and adds the two fold caveats (extension-barrel shim, effect-free intent probe); step 5 reframed as policy-matrix with the palette isVisible/canDispatch mismatch as the motivating defect. - Quality pass: house doc-status banner added; review scar tissue excised; sync/async story consolidated into §6; warn callouts 7→2; decision promoted from footnote to a keyed callout. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
What
A high-level design proposal for upgrading the action system into a typed, observable command substrate — the highest-leverage extensibility primitive we identified after the navigation-seam work.
Design-only: one HTML doc,
docs/command-system-design.html. No code changes.Why
The navigation work proved a pattern we keep repeating: to make one behaviour extensible (observe / wrap / veto / replace), we hand-build a bespoke seam for it (
navigationVerb,pasteDecisionVerb). Each is correct in isolation but is a new vocabulary the rest of the system doesn't share — the "double thing" smell.The doc's organizing idea:
We already have two partial command systems that don't know about each other:
defineVerbFacet— typed signature +impl/decorator/before/aftermiddleware; but no identity, no bindingThe four gaps it frames
canDispatchsplit the code already flags as a TODOAlso covers: commands↔events (with the audit-B3 typed-bus constraint), why the bespoke verbs fold into the generic bus once G1+G3 land, a return-channel-first migration path, and open questions.
Grounding
Re-read against master:
src/shortcuts/types.ts,src/shortcuts/runAction.ts,src/shortcuts/effectiveActions.ts,src/facets/verbFacet.ts,src/plugins/app-intents/appIntents.ts.Scope
Proposal for discussion — not an implementation plan. Companion to
docs/extensibility-axes.md,docs/extension-seam-gaps.md,docs/navigation-redesign.md.🤖 Generated with Claude Code