Skip to content

fix: narrow client result contracts#1155

Merged
thymikee merged 1 commit into
mainfrom
fix/1153-client-result-narrowing
Jul 8, 2026
Merged

fix: narrow client result contracts#1155
thymikee merged 1 commit into
mainfrom
fix/1153-client-result-narrowing

Conversation

@thymikee

@thymikee thymikee commented Jul 8, 2026

Copy link
Copy Markdown
Member

Summary

Narrow Node client result types for closed daemon projections from #1153.

Adds public contracts for wait, prepare, push, and trigger-app-event, wires them into CommandResultMap and MCP output schemas, and tightens JSON payload typing for push/app-event payloads.

Closes #1153

Validation

  • pnpm typecheck
  • pnpm format
  • pnpm exec vitest run src/core/command-descriptor/tests/command-result.test.ts src/mcp/tests/command-tools.test.ts src/tests/client.test.ts src/commands/management/runtime/apps.test.ts src/commands/management/index.test.ts src/core/dispatch-payload.test.ts src/core/tests/dispatch-trigger-app-event.test.ts
  • git diff --check

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 1.6 MB 1.6 MB +1.3 kB
JS gzip 507.7 kB 508.1 kB +386 B
npm tarball 610.1 kB 611.2 kB +1.0 kB
npm unpacked 2.2 MB 2.2 MB +3.8 kB

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 28.0 ms 27.6 ms -0.4 ms
CLI --help 56.4 ms 57.9 ms +1.5 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/registry.js +26 B +10 B
dist/src/selector-runtime.js +3 B +1 B

@thymikee thymikee force-pushed the fix/1153-client-result-narrowing branch from 6fe46b6 to 889428a Compare July 8, 2026 15:31
@thymikee thymikee force-pushed the fix/adr0008-cli-client-backed-derived branch from 0beb726 to 83b3646 Compare July 8, 2026 15:33
Base automatically changed from fix/adr0008-cli-client-backed-derived to main July 8, 2026 15:55
@thymikee thymikee force-pushed the fix/1153-client-result-narrowing branch from 889428a to b0c06b1 Compare July 8, 2026 16:01
@thymikee thymikee marked this pull request as ready for review July 8, 2026 16:12
@thymikee

thymikee commented Jul 8, 2026

Copy link
Copy Markdown
Member Author

Reviewed the narrowing. The typed spine, MCP schemas, and the JsonObject migration all line up cleanly with the projections — push, trigger-app-event, and the toDaemonWaitData path match field-for-field. Two spots where the new contract under-declares what the daemon actually puts on the wire, plus one minor behavior change:

1. PrepareCommandResult omits fields the projection always emits. prepareIosRunnerResponseData spreads ...result (src/daemon/handlers/session.ts:131), and result: PrepareIosRunnerResult carries runner (required — emitted on every success) plus optional cache / artifact / xctestrunPath / recoveryReason / failureReason. None appear in src/contracts/prepare.ts, so the "compact projection" the docstring describes isn't actually compact — runner leaks untyped on every prepare call. Either narrow the projection to explicitly pick the contract fields (so runtime matches the type), or add the ones that are genuinely public to the contract.

2. WaitCommandResult omits selectorChain. The iOS selector fast-path returns data: payload unstripped (src/daemon/selector-runtime.ts:373-376), so a public wait <selector> on iOS emits selectorChain: [...], which isn't in src/contracts/wait.ts. Note the sibling is fast-path strips it via stripSelectorChain (selector-runtime.ts:349) — the wait path looks like it should too. Stripping it there makes runtime match the new contract and fixes a small internal-field leak in one move.

3. (minor) trigger-app-event payload loses read-time validation. Swapping looseObjectField()jsonSchemaField<JsonObject>(looseObjectSchema()) in src/commands/management/push.ts keeps the same JSON schema but drops the optionalRecord reader, so a non-object payload is no longer rejected at field-read with Expected payload to be an object. — it's now caught later in assertJsonObject with a different message. Functionally still rejected, just a different stage/wording; flagging in case byte-identical errors matter.

@thymikee

thymikee commented Jul 8, 2026

Copy link
Copy Markdown
Member Author

Reviewed current head b0c06b1 against #1153 and ADR 0008. The PR moves closed result contracts for wait, prepare, push, and trigger-app-event into public contract modules, wires them into CommandResultMap/client methods and MCP output schemas, and keeps the narrowing limited to daemon projections with focused client/schema/runtime tests. CI is green across 21/21 checks. No actionable blockers found.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Jul 8, 2026
@thymikee thymikee force-pushed the fix/1153-client-result-narrowing branch from b0c06b1 to b940304 Compare July 8, 2026 16:28
@thymikee

thymikee commented Jul 8, 2026

Copy link
Copy Markdown
Member Author

New head b940304 arrived after my ready review of b0c06b1, and checks are currently running again. I removed the stale ready-for-human label for now; I will re-review/relabel once the current head is clean.

@thymikee thymikee removed the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Jul 8, 2026
@thymikee thymikee merged commit 106c238 into main Jul 8, 2026
21 checks passed
@thymikee thymikee deleted the fix/1153-client-result-narrowing branch July 8, 2026 16:35
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-07-08 16:35 UTC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Track Node client typed-result narrowing

1 participant