Conversation
Size Report
Startup median (7 runs, lower is better):
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/commands/system/index.ts">
<violation number="1" location="src/commands/system/index.ts:87">
P3: The change is described as exporting a "frozen" map, but `satisfies` only constrains types — the object and every nested schema stay mutable at runtime, and the new reference-equality tests share the same reference so an in-place mutation would pass unnoticed. Either drop the "frozen" characterization or actually free the map (deep-`Object.freeze`) if immutability is the intent.</violation>
<violation number="2" location="src/commands/system/index.ts:188">
P1: The appstate schema rejects successful HarmonyOS responses. Use the same `android | harmonyos` platform vocabulary as `AppStateCommandResult` so MCP clients can validate HarmonyOS appstate results.</violation>
<violation number="3" location="src/commands/system/index.ts:199">
P1: The keyboard schema rejects HarmonyOS dismiss and enter results. Add `harmonyos` to the platform enum so the MCP output schema accepts every platform the keyboard runtime returns.</violation>
<violation number="4" location="src/commands/system/index.ts:210">
P2: The keyboard output schema omits the contract's `mechanism` field, so MCP consumers cannot discover the iOS dismiss-key disclosure from `outputSchema`. Add an optional `mechanism` enum for `dismissKey`.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| // packages/contracts/src/keyboard.ts — flat closed shape; `platform`/`action` always present. | ||
| keyboard: objectSchema( | ||
| { | ||
| platform: enumSchema(['android', 'ios']), |
There was a problem hiding this comment.
P1: The keyboard schema rejects HarmonyOS dismiss and enter results. Add harmonyos to the platform enum so the MCP output schema accepts every platform the keyboard runtime returns.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/commands/system/index.ts, line 199:
<comment>The keyboard schema rejects HarmonyOS dismiss and enter results. Add `harmonyos` to the platform enum so the MCP output schema accepts every platform the keyboard runtime returns.</comment>
<file context>
@@ -64,6 +74,158 @@ const TV_REMOTE_LONGPRESS_PRESET_MS = 500;
+ // packages/contracts/src/keyboard.ts — flat closed shape; `platform`/`action` always present.
+ keyboard: objectSchema(
+ {
+ platform: enumSchema(['android', 'ios']),
+ action: enumSchema(['status', 'dismiss', 'enter']),
+ visible: booleanSchema(),
</file context>
| platform: enumSchema(['android', 'ios']), | |
| platform: enumSchema(['android', 'harmonyos', 'ios']), |
| ), | ||
| objectSchema( | ||
| { | ||
| platform: constSchema('android'), |
There was a problem hiding this comment.
P1: The appstate schema rejects successful HarmonyOS responses. Use the same android | harmonyos platform vocabulary as AppStateCommandResult so MCP clients can validate HarmonyOS appstate results.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/commands/system/index.ts, line 188:
<comment>The appstate schema rejects successful HarmonyOS responses. Use the same `android | harmonyos` platform vocabulary as `AppStateCommandResult` so MCP clients can validate HarmonyOS appstate results.</comment>
<file context>
@@ -64,6 +74,158 @@ const TV_REMOTE_LONGPRESS_PRESET_MS = 500;
+ ),
+ objectSchema(
+ {
+ platform: constSchema('android'),
+ package: stringSchema(),
+ activity: stringSchema(),
</file context>
| platform: constSchema('android'), | |
| platform: enumSchema(['android', 'harmonyos']), |
| inputMethodPackage: stringSchema(), | ||
| focusedPackage: stringSchema(), | ||
| focusedResourceId: stringSchema(), | ||
| inputOwner: enumSchema(['app', 'ime', 'unknown']), |
There was a problem hiding this comment.
P2: The keyboard output schema omits the contract's mechanism field, so MCP consumers cannot discover the iOS dismiss-key disclosure from outputSchema. Add an optional mechanism enum for dismissKey.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/commands/system/index.ts, line 210:
<comment>The keyboard output schema omits the contract's `mechanism` field, so MCP consumers cannot discover the iOS dismiss-key disclosure from `outputSchema`. Add an optional `mechanism` enum for `dismissKey`.</comment>
<file context>
@@ -64,6 +74,158 @@ const TV_REMOTE_LONGPRESS_PRESET_MS = 500;
+ inputMethodPackage: stringSchema(),
+ focusedPackage: stringSchema(),
+ focusedResourceId: stringSchema(),
+ inputOwner: enumSchema(['app', 'ime', 'unknown']),
+ message: stringSchema(),
+ },
</file context>
| inputOwner: enumSchema(['app', 'ime', 'unknown']), | |
| inputOwner: enumSchema(['app', 'ime', 'unknown']), | |
| mechanism: enumSchema(['dismissKey']), |
| * `additionalProperties: false`, so additive response fields such as `settle`/`cost` keep | ||
| * validating. `back`'s settle observation is grafted separately by the trait derivation pass. | ||
| */ | ||
| export const SYSTEM_COMMAND_OUTPUT_SCHEMAS = { |
There was a problem hiding this comment.
P3: The change is described as exporting a "frozen" map, but satisfies only constrains types — the object and every nested schema stay mutable at runtime, and the new reference-equality tests share the same reference so an in-place mutation would pass unnoticed. Either drop the "frozen" characterization or actually free the map (deep-Object.freeze) if immutability is the intent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/commands/system/index.ts, line 87:
<comment>The change is described as exporting a "frozen" map, but `satisfies` only constrains types — the object and every nested schema stay mutable at runtime, and the new reference-equality tests share the same reference so an in-place mutation would pass unnoticed. Either drop the "frozen" characterization or actually free the map (deep-`Object.freeze`) if immutability is the intent.</comment>
<file context>
@@ -64,6 +74,158 @@ const TV_REMOTE_LONGPRESS_PRESET_MS = 500;
+ * `additionalProperties: false`, so additive response fields such as `settle`/`cost` keep
+ * validating. `back`'s settle observation is grafted separately by the trait derivation pass.
+ */
+export const SYSTEM_COMMAND_OUTPUT_SCHEMAS = {
+ back: objectSchema(
+ {
</file context>
|
Reviewed at 110cb57. The moved system-family schemas match the originals, but the new Not blocking: the new test could pin CI: checks are still queued. |
|
CI at 110cb57: the Coverage failure is likely caused by this PR. The eager-closure budget test in |
|
Reviewed at a595a82. This fixes the finding from the review at 110cb57: Not blocking: CI is still running. Coverage runs the eager-closure budget test on this exact import route, so a Coverage failure would be related to this change. Coverage and Repo Guards need to pass at a595a82. |
thymikee
added
the
ready-for-human
a595a82 to
5c22d65
Compare
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/contracts/src/fold-runtime.ts">
<violation number="1" location="packages/contracts/src/fold-runtime.ts:1">
P1: This removes `FOLD_SCREEN_COORDINATE_SPACE` from the published `@agent-device/contracts/fold-runtime` subpath, breaking existing consumers that import the value there. Preserve the compatibility re-export from `device-rotation.ts` while using the relocated constant internally.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| @@ -1,13 +1,10 @@ | |||
| import type { FoldPose, SetFoldPoseInput } from './device-rotation.ts'; | |||
| import type { FoldPose, FoldScreenCoordinateSpace, SetFoldPoseInput } from './device-rotation.ts'; | |||
There was a problem hiding this comment.
P1: This removes FOLD_SCREEN_COORDINATE_SPACE from the published @agent-device/contracts/fold-runtime subpath, breaking existing consumers that import the value there. Preserve the compatibility re-export from device-rotation.ts while using the relocated constant internally.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/contracts/src/fold-runtime.ts, line 1:
<comment>This removes `FOLD_SCREEN_COORDINATE_SPACE` from the published `@agent-device/contracts/fold-runtime` subpath, breaking existing consumers that import the value there. Preserve the compatibility re-export from `device-rotation.ts` while using the relocated constant internally.</comment>
<file context>
@@ -1,13 +1,10 @@
-import type { FoldPose, SetFoldPoseInput } from './device-rotation.ts';
-import { FOLD_SCREEN_COORDINATE_SPACE } from './device-rotation.ts';
+import type { FoldPose, FoldScreenCoordinateSpace, SetFoldPoseInput } from './device-rotation.ts';
import type { RuntimeOperationFact } from './platform-runtime.ts';
</file context>
| import type { FoldPose, FoldScreenCoordinateSpace, SetFoldPoseInput } from './device-rotation.ts'; | |
| import type { FoldPose, FoldScreenCoordinateSpace, SetFoldPoseInput } from './device-rotation.ts'; | |
| export { FOLD_SCREEN_COORDINATE_SPACE } from './device-rotation.ts'; |
|
Addressed both review notes.
Pushed back on the automated review's HarmonyOS enum and keyboard CI: the earlier iOS and Android Smoke Tests failures on this PR (a simulator readiness timeout and an "adb: device offline" emulator disconnect) were infra flakes unrelated to this change, which only touches MCP schema definitions. Both have already passed on the rerun; the other Smoke Tests jobs are still running but were green before this push too. Rebased and force-pushed the child branch on top of these two commits so its own diff stays clean. |
5c22d65 to
c02b2c7
Compare
|
One more from the automated review: the last push dropped |
|
Reviewed at c02b2c7. Not blocking: the constant is now declared in both device-rotation.ts and fold-runtime.ts; a small leaf module that both import from would keep one declaration. Both Smoke Tests jobs were still queued at review time. |
… module Moves the 10 hand-authored MCP outputSchema entries for back, home, orientation, app-switcher, fold, action-button, tv-remote, clipboard, appstate, and keyboard out of command-output-schemas.ts into a frozen SYSTEM_COMMAND_OUTPUT_SCHEMAS map exported from src/commands/system/index.ts, following the #2810 projection seam. Registers the family in PROJECTED_FAMILIES and moves its shape-assertion tests into a colocated command-tools-system-schemas.test.ts, including a reference-equality proof that accounts for the one entry (back) the settle-observation derivation pass copies rather than passing through untouched. Part of #2819.
…losure FOLD_SCREEN_COORDINATE_SPACE lived in fold-runtime.ts, a module the CLI closure did not otherwise load. Importing it directly from src/commands/system/index.ts pulled fold-runtime.ts (and its own dependents) into cli.ts's eager import graph, tripping the eager-closure-budgets gate (295 -> 296 modules). Move the constant's declaration to device-rotation.ts, which the same file already loads via the @agent-device/contracts/device facade for DEVICE_ROTATIONS/FOLD_POSES, and re-export it from the device facade so importers reach it there. fold-runtime.ts keeps publishing FOLD_SCREEN_COORDINATE_SPACE (an external, released API on this subpath) as a literal rather than a re-export, so importing it does not eagerly load device-rotation.ts and its own eager closure does not grow either; a type-only import of the new FoldScreenCoordinateSpace alias still makes any drift between the two a compile error. Its runtime value consumer (platform-apple's fold pose runtime) now takes the constant from the device facade it already imports.
constSchema was hand-copied into src/commands/system/index.ts, drifting from the identical private helper in command-output-schemas.ts. Export it once from command-input.ts, next to the other schema-primitive builders, and import it at both sites. The ownership test for the projected family map only checked that each of the module's own entries names the module as owner; it did not check the inverse, so a system-family command the registry attributes to this module but missing from SYSTEM_COMMAND_OUTPUT_SCHEMAS would still pass. Compare the two sets directly.
c02b2c7 to
bc830f5
Compare
|
Rebased onto the updated main; new head is bc830f5. The three commits replayed cleanly with no conflicts, and check:affected passes on it. |
This branch has not been deployed
1 participant
Summary
Moves the 10 hand-authored MCP
outputSchemaentries for the system-family commands (back,home,orientation,app-switcher,fold,action-button,tv-remote,clipboard,appstate,keyboard) out ofsrc/mcp/command-output-schemas.tsinto aSYSTEM_COMMAND_OUTPUT_SCHEMASmap exported from the owning module,src/commands/system/index.ts, registered inPROJECTED_FAMILIESper the #2810 projection seam. Each family's schemas live next to the command they describe instead of in one large hand-authored file.Test coverage for these schemas moves out of the legacy aggregations into a colocated
command-tools-system-schemas.test.ts, including a reference-equality proof that the composed map holds the module's own object (not a copy) for every entry, and a bidirectional ownership check (module <-> registry) after review.constSchemais now shared fromsrc/commands/command-input.tsinstead of being duplicated between this module andcommand-output-schemas.ts.FOLD_SCREEN_COORDINATE_SPACE's declaration moved todevice-rotation.ts, which this module already loads through the@agent-device/contracts/devicefacade, so importing it no longer pullsfold-runtime.tsinto the CLI's eager module closure;fold-runtime.tsstill exports the same value on its published subpath (type-checked against the new declaration) for existing external consumers.Part of #2819. 8 files touched, no behavior change.
Validation
Tested at
c02b2c766e3b769eafcc4b8b0e57d08329b315ee.pnpm check:affected --run: 554 test files / 4260 tests passed, all runnable checks passed (format, lint, typecheck, layering, fallow, build, vitest-related).COMMAND_OUTPUT_SCHEMASfor all 10 migrated commands via a small Node script importingsrc/mcp/command-output-schemas.tsdirectly (Node 26 native TS), once onorigin/mainand once on this head:diff before.json after.jsonprinted no differences ("IDENTICAL") -- the composed map is unchanged for all 10 keys.fold's spread entry a shallow copy instead of referencing the module's own object; this failed both the reference-equality test and the extended disjointness test, confirming the tests guard the "not a copy" invariant. Reverted before committing.src/commands/system/index.tsimportedFOLD_SCREEN_COORDINATE_SPACEfrom@agent-device/contracts/fold-runtime, a module the CLI closure did not otherwise load (295 -> 296 modules,eager-closure-budgets.test.ts). Fixed by relocating the declaration todevice-rotation.ts(already loaded via thedevicefacade);fold-runtime.tsstill exports the constant as a literal (not a re-export) so its own published subpath's closure does not grow either, and a type-only import of the newFoldScreenCoordinateSpacealias makes any drift between the two a compile error.pnpm check:affected --runpasses the closure gate locally and Coverage is green on this head.FOLD_SCREEN_COORDINATE_SPACEfrom the released@agent-device/contracts/fold-runtimesubpath (in npm since v0.21.9), which would have broken existing importers. Fixed as described above -- the value ships from both subpaths, type-locked to one declaration.No unresolved risk.