feat(sdk): can() returns CanResult; T4 dispatch-boundary tests

[vanceingalls]

What

Changes can(op) return type from boolean to CanResult = {ok:true} | {ok:false, code, message, hint?}.

Adds CAN_OK constant and canErr() helper so validateOp switch arms stay tight. Exports stable error codes (E_TARGET_NOT_FOUND, E_NO_ROOT, E_NO_GSAP_SCRIPT, E_NO_GSAP_TIMELINE, E_UNKNOWN_OP) that callers can switch on.

Also includes: resolveKeyframe() extraction from two handleSet/RemoveGsapKeyframe handlers (dedup), a quiet correctness fix for removeKeyframe when two keyframes share the same percentage, and a parseDeclarations tokenizer fix for semicolons inside quoted CSS values.

Why

boolean result gives callers no way to surface actionable error messages. CanResult lets agent callers and UI both switch on result.code and show result.hint in error UX.

How

• validateOp in mutate.ts now returns CanResult from a switch over op type
• can(op) in session.ts wraps validateOp and retains the public can(): boolean path for backward compat until callers migrate
• T4 dispatch-boundary test suite (new file, 213 lines) covers all error codes + happy paths

Test plan

• T4 dispatch-boundary tests (213 lines, new file) — all pass
• Full test suite (156/156 pass)
• oxlint / oxfmt clean

[vanceingalls]

• feat(sdk): stage 6 — sub-composition scoped ids (F9) #1434 : 2 dependent PRs (#1429 , #1441 )
• feat(sdk): stage 5 — export adapter factories from package root #1432
• feat(sdk): stage 4 — canUndo/canRedo, removeElement GSAP cascade, override-set cleanup #1431
• feat(sdk): can() returns CanResult; T4 dispatch-boundary tests #1426 👈 (View in Graphite)
• fix(sdk): fs adapter flush() tracks in-flight writes; add to T13 contract suite #1425
• feat(sdk): file-backed fs adapter + setTiming GSAP-script sync; add sdk-playground #1423
• refactor(core): swap studio-api read path from recast to acorn parser (T6e) #1392
• feat(core): parse-parity suite for acorn parser (T6d) #1370 : 1 other dependent PR (#1379 )
• feat(core): acorn GSAP write path — magic-string offset-splice (T6c) #1369
• feat(core): acorn GSAP read path with T6b differential corpus tests #1368
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[james-russo-rames-d-jusso]

Verified at HEAD d4d2899.

Big, clean diff. The CanResult = {ok:true} | {ok:false, code, message, hint?} discriminator is the right shape — exhaustive at the type level, stable codes (E_TARGET_NOT_FOUND / E_NO_ROOT / E_NO_GSAP_SCRIPT / E_NO_GSAP_TIMELINE / E_UNKNOWN_OP) callers can switch on, optional hint for human-facing UIs. The CAN_OK constant + canErr() helper keep validateOp switch arms tight without re-allocating object literals on every call. T4 dispatch-boundary test suite (213 lines, new file) covers the boundaries the lens called out: invalid target, no current state path (E_NO_GSAP_SCRIPT on non-GSAP composition), happy-path GSAP op on GSAP composition, batch-collapse, custom origin propagation, override-set accumulation.

The mutate.ts refactor extracting resolveKeyframe() from handleSetGsapKeyframe / handleRemoveGsapKeyframe deduplicates four lines of acorn-parse boilerplate cleanly. The new "bail if two keyframes share the same percentage" guard in handleRemoveGsapKeyframe is a quiet but real correctness fix — removeKeyframeFromScript matches by percentage, so two keyframes at the same pct would have removed the wrong one.

The parseDeclarations quote-aware tokenizer fix (cssWriter.ts) is a separate latent-bug fix: previously a semicolon inside a quoted CSS value (e.g. content: "a; b") would have terminated the declaration early. Worth the inclusion since CanResult tests touch setClassStyle, but it widens the PR's apparent scope; brief mention in the body would help the reviewer.

The setGsapScript(document, "") → existing?.remove() change (model.ts) is correct: previously setGsapScript would have written an empty <script> tag. The GSAP cascade in #1431 depends on this — when removing the last animation that uses the script, the cascade-removed script should disappear from the DOM, not leave an empty tag.

Concerns

• PR body empty (the template is intact, no What/Why/How filled in). For a +418/-116 PR that changes a public-API signature (can(op): boolean → CanResult) this is a real reviewer-cost hit. Worth filling in even briefly — call out the breaking-shape change so downstream consumers (the playground in this very stack, #1429 / #1443) know to grep.
• Discriminator-code stability: code: string is typed as string, not a string-literal union. The JSDoc lists the codes, but a type CanErrorCode = 'E_TARGET_NOT_FOUND' | ... would give consumers exhaustiveness checking on switch. Minor — nice-to-have, not blocking.
• .fallowrc.jsonc complexity-suppression on files.ts: the comment explains the inherited-fingerprint shift problem clearly, but flag it for follow-up — leaving executeGsapMutation at cyclomatic-58 with the file silenced is debt accruing.
• Cross-stack coherence: PR body claims (and the override-set test confirms) removeElement emits a removal marker on the override-set. #1431 implements the cascade cleanup of property keys for that removed element. The two PRs are wire-coherent; just noting for the stack reviewer.

Determinism: no Date.now() / Math.random() / fetch() / performance.now() in dispatch-boundary or validateOp paths. MY_ORIGIN = Symbol("ai-agent") in tests is fine — tests only.

Verdict: approve-with-concerns. Fill in the PR body before merge.

Review by Rames D Jusso

[miguel-heygen]

CI blocked by the oxfmt failure in #1423. Code change is substantive and well-reviewed below.

Strengths:

• CanResult shape ({ ok: true } | { ok: false; code: string; reason: string }) is a better public API than bare boolean — callers can surface meaningful error messages without a separate whyCantI() query.
• E_NO_GSAP_SCRIPT and E_UNKNOWN_OP error codes are stable, greppable strings — good for telemetry and conditional recovery.
• T4 dispatch-boundary tests (packages/sdk/src/engine/dispatch-boundary.test.ts) are thorough: they cover the happy path, the GSAP-required path, and the unknown-op path without needing full E2E setup.

Important:

• packages/sdk/src/session.ts validateOp refactor — the old return !PHASE3B_OPS.has(op.type) default: path returned true (allowing) for types not in the set. The new default: return { ok: false, code: E_UNKNOWN_OP, ... } changes the semantics: unknown op types previously passed silently through can(). This is intentional (the PR title says "dispatch boundary"), but if any caller relied on the old allow-through for forwards-compat ops, they'll now get can()→false. Confirm this is a deliberate contract tightening and document in CHANGELOG or release notes.

Nit:

• packages/sdk/src/types.ts — CanResult is exported but E_NO_GSAP_SCRIPT / E_UNKNOWN_OP string constants are not. Consumers who want to switch on result.code need the constants exported too, otherwise they magic-string match. Export them from the package root.

Verdict: APPROVE (pending #1423 oxfmt fix)
Reasoning: CanResult is a cleaner contract than boolean; T4 tests are solid. The unknown-op semantic change is flagged as important but appears intentional.

— magi

[vanceingalls]

@james-russo-rames-d-jusso — concerns addressed

PR body: filled in (What/Why/How/Test plan now populated).

code: string vs literal union: agreed that CanErrorCode = 'E_TARGET_NOT_FOUND' | ... would give exhaustiveness. Deferred — the codes are JSDoc-documented for now and the test suite enumerates them. Adding the union type is a one-line change once the full code set stabilizes across all remaining stages.

executeGsapMutation cyclomatic-58 with .fallowrc.jsonc suppression: tracked as debt. Not widening the suppression further; this function is a dispatch hub and the complexity is inherent to the op count, not accidental nesting.

PR head has been rebased (same content, new base) — force pushed to align with the stack after the stage 1–6 rebase.

The base branch was changed.