feat(remote): add HarmonyOS proxy lease backend - #2266
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new harmonyos-instance backend is added to the contract but still needs follow-up updates to hardcoded CLI/remote-config validation lists and related platform compatibility/test coverage to avoid runtime/UX breakage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR extends the remote connection/lease contract surface to recognize HarmonyOS devices by introducing a new harmonyos-instance lease backend and wiring HarmonyOS proxy devices + CLI lease-backend resolution to use it.
Changes:
- Extend shared contracts to include
SessionRuntimeHints.platform: 'harmonyos'andLeaseBackend: 'harmonyos-instance'. - Resolve
--platform harmonyosand HarmonyOS proxy devices to theharmonyos-instancelease backend for allocation/heartbeat/close flows. - Update the CLI error text for cases where a lease backend must be explicitly determined.
File summaries
| File | Description |
|---|---|
src/cli/commands/connection-runtime.ts |
Adds HarmonyOS → harmonyos-instance backend resolution for flags and proxy devices; updates related error messaging. |
packages/kernel/src/contracts.ts |
Expands the shared contract unions for session runtime hints and lease backend backends to include HarmonyOS. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| export type SessionRuntimeHints = { | ||
| platform?: 'ios' | 'android'; | ||
| platform?: 'ios' | 'android' | 'harmonyos'; | ||
| metroHost?: string; |
| const DAEMON_LOCK_POLICIES = ['reject', 'strip'] as const; | ||
| export type DaemonLockPolicy = (typeof DAEMON_LOCK_POLICIES)[number]; | ||
| const LEASE_BACKENDS = ['ios-simulator', 'ios-instance', 'android-instance'] as const; | ||
| const LEASE_BACKENDS = ['ios-simulator', 'ios-instance', 'android-instance', 'harmonyos-instance'] as const; |
| if (flags.leaseBackend) return flags.leaseBackend; | ||
| if (flags.platform === 'android') return 'android-instance'; | ||
| if (flags.platform === 'ios') return 'ios-instance'; | ||
| if (flags.platform === 'harmonyos') return 'harmonyos-instance'; |
|
Thanks for taking this on — HarmonyOS support is a welcome addition and the overall shape of the change looks right. A few things need to land before it works end to end, mostly allowlists that still need the new backend added. Blocking
Worth a look
Docs and tests
CI hasn't run on this yet, so it's worth a rerun once the above is pushed. Happy to help with any of it if useful — thanks again for the contribution! Generated by Claude Code |
There was a problem hiding this comment.
🟡 Changes recommended
The new HarmonyOS backend/runtime literals are added to the shared contract but existing daemon-side validation/allowlists still reject them, which will break real lease admission and runtime-hint usage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Lite
| export type SessionRuntimeHints = { | ||
| platform?: 'ios' | 'android'; | ||
| platform?: 'ios' | 'android' | 'harmonyos'; | ||
| metroHost?: string; |
| export function resolveRequestedLeaseBackend(flags: CliFlags): LeaseBackend | undefined { | ||
| if (flags.leaseBackend) return flags.leaseBackend; | ||
| if (flags.platform === 'android') return 'android-instance'; | ||
| if (flags.platform === 'ios') return 'ios-instance'; | ||
| if (flags.platform === 'harmonyos') return 'harmonyos-instance'; | ||
| return undefined; |
|
Addressed the blocking review items in
Validation: The provider daemon still needs to expose an actual HarmonyOS lease implementation before this can be considered end-to-end; this PR keeps unsupported providers fail-closed and documents that scope. |
There was a problem hiding this comment.
🔵 Needs a closer look
The updated runtime/platform compatibility check introduces a behavior regression that can drop runtime hints for non-leaf platform selectors (e.g., apple) and should be corrected before approval.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/cli/commands/connection-runtime.ts:740
isRuntimeCompatibleWithPlatformnow enforces strict equality for any--platformselector. This is a behavior change from the prior logic (which only enforced forios/android) and can cause runtime hints (e.g., metroHost/metroPort) to be dropped when callers use non-leaf selectors likeapple,vega, etc. If the intent is only to extend the strict check to HarmonyOS, keep the previous guard and addharmonyosto it.
src/tests/remote-connection.test.ts:51- This test uses
as neverto bypass theCliFlagstype, which can hide real type errors. Other tests in this file pass a minimalCliFlagsobject instead; do the same here so the compiler keeps protecting the call site.
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Follow-up pushed in
Validation: The PR remains intentionally contract-level: actual proxy daemon/provider Harmony lease implementation and end-to-end open/heartbeat/close still require maintainer design and follow-up. Until then, proxy OHOS remains unsupported at runtime. |
There was a problem hiding this comment.
🔵 Needs a closer look
The contract/schema now accepts HarmonyOS runtime hints, but daemon-side runtime-hint normalization still only supports iOS/Android, so HarmonyOS runtime hints will fail at runtime.
Review details
Suppressed comments (1)
packages/kernel/src/contracts.ts:13
SessionRuntimeHints.platformanddaemonRuntimeSchemanow accept "harmonyos", but the daemon-side runtime hint normalization still only recognizes ios/android (e.g.src/daemon/session-runtime.tsrejects any other value andtoRuntimePlatformmaps only ios/android). This makes HarmonyOS runtime hints fail at runtime despite the updated contract/schema.
export type SessionRuntimeHints = {
platform?: 'ios' | 'android' | 'harmonyos';
metroHost?: string;
metroPort?: number;
bundleUrl?: string;
launchUrl?: string;
};
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
BLOCKED at
|
Signed-off-by: Ark <artin@cat.ms>
Signed-off-by: Ark <artin@cat.ms>
Signed-off-by: Ark <artin@cat.ms>
Signed-off-by: Ark <artin@cat.ms>
Signed-off-by: Ark <artin@cat.ms>
af89c1f to
1533c52
Compare
|
Rebased onto current upstream main |
There was a problem hiding this comment.
🟡 Changes recommended
The PR introduces a runtime-hints contract mismatch and a likely compatibility regression (plus missing wire-compat ledger updates) that can break expected behavior and/or CI.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
src/cli/commands/connection-runtime.ts:741
isRuntimeCompatibleWithPlatformnow comparesruntime.platform("ios"|"android"|"harmonyos") directly toCliFlags['platform'](PlatformSelector, including values like "apple", "macos", "vega", etc). This is a behavior change from the previous guard and will drop stored runtime hints when callers use selectors like--platform apple, even though that alias is commonly accepted elsewhere.
if (!runtime.platform || !platform) {
return true;
}
return runtime.platform === platform;
}
packages/kernel/src/contracts.ts:275
- This expands
SessionRuntimeHints.platform/daemonRuntimeSchemato accept "harmonyos", but the daemon-side runtime hint normalization still only supports "ios" and "android" (e.g.src/daemon/session-runtime.tsrejects anything else vianormalizeRuntimePlatformInput/toRuntimePlatform). As-is, the contract says HarmonyOS is allowed while the daemon will still throwINVALID_ARGS, so HarmonyOS runtime hints are not actually supported yet.
export const daemonRuntimeSchema = schema<SessionRuntimeHints>((input, path) => {
const record = expectObject(input, path);
return {
platform: optionalEnum(record, 'platform', ['ios', 'android', 'harmonyos'] as const, path),
metroHost: optionalString(record, 'metroHost', path),
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Lite
| const LEASE_BACKENDS = ['ios-simulator', 'ios-instance', 'android-instance', 'harmonyos-instance'] as const; | ||
| export type LeaseBackend = (typeof LEASE_BACKENDS)[number]; |
| - Added the `harmonyos-instance` remote lease backend so HarmonyOS proxy devices can participate in | ||
| the same explicit allocation, heartbeat, and close lifecycle as other remote instances (#2266). | ||
|
|
||
| - Fixed: `settings airplane on|off` now takes an Android device offline. It is applied through |
Signed-off-by: Ark <artin@cat.ms>
|
Addressed the maintainer review in
Validation on the rebased branch: |
There was a problem hiding this comment.
🟡 Changes recommended
It overwrites an existing Unreleased changelog entry and introduces a test that should be colocated with the existing lease-registry-scope test module for consistency.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
CHANGELOG.md:7
- This change replaces (removes) the existing Unreleased changelog entry about strict
wait absent <selector>polling (#2236). If that feature is still unreleased, it should remain in the Unreleased section and this HarmonyOS entry should be added as an additional bullet rather than overwriting it.
- Added the `harmonyos-instance` lease contract and CLI/runtime plumbing as a prerequisite for
HarmonyOS proxy support; provider/daemon allocation remains gated until its end-to-end lifecycle
is implemented and validated (#2266).
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
| test('HarmonyOS platform resolves to its proxy lease backend', () => { | ||
| assert.equal(resolveRequestedLeaseBackend({ platform: 'harmonyos' } as never), 'harmonyos-instance'); | ||
| }); |
| test('normalizeLeaseBackend accepts HarmonyOS instance backend', () => { | ||
| assert.equal(normalizeLeaseBackend('harmonyos-instance'), 'harmonyos-instance'); | ||
| }); |
Signed-off-by: Ark <artin@cat.ms>
|
Follow-up |
There was a problem hiding this comment.
🔵 Needs a closer look
It currently drops an existing Unreleased changelog entry and introduces test issues (misplaced test and a type-escaping as never) that should be corrected before approval.
Review details
Suppressed comments (3)
CHANGELOG.md:8
- This edit removes the existing Unreleased changelog entry about strict
wait absentpolling (#2236). Unless that item was intentionally dropped elsewhere, it should be kept and the HarmonyOS lease note added alongside it to avoid losing release notes.
- Added the `harmonyos-instance` lease contract and CLI/runtime plumbing as a prerequisite for
HarmonyOS proxy support; provider/daemon allocation remains gated until its end-to-end lifecycle
is implemented and validated (#2266).
src/daemon/tests/lease-registry.test.ts:15
- This new test exercises
normalizeLeaseBackendfromlease-registry-scope.ts, but it’s being added tolease-registry.test.ts. There is already a dedicatedlease-registry-scope.test.ts; moving this test there keeps tests aligned with the module under test and avoids mixing scope-validation coverage into the registry suite.
test('normalizeLeaseBackend accepts HarmonyOS instance backend', () => {
assert.equal(normalizeLeaseBackend('harmonyos-instance'), 'harmonyos-instance');
});
src/tests/remote-connection.test.ts:52
- Using
as neverhere defeats type-checking and can mask future signature changes toresolveRequestedLeaseBackend. Prefer constructing a realCliFlagsvalue via the existingforceConnectFlagshelper (already imported in this file).
test('HarmonyOS platform resolves to its proxy lease backend', () => {
assert.equal(resolveRequestedLeaseBackend({ platform: 'harmonyos' } as never), 'harmonyos-instance');
});
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
Signed-off-by: Ark <artin@cat.ms>
|
Follow-up pushed ( |
There was a problem hiding this comment.
🟡 Changes recommended
A runtime-hints error message in src/daemon/session-runtime.ts still states iOS/Android-only support, which is now misleading after adding HarmonyOS to accepted runtime-hint platforms.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
| if (value === undefined) return platform; | ||
| if (value !== 'ios' && value !== 'android') { | ||
| if (value !== 'ios' && value !== 'android' && value !== 'harmonyos') { | ||
| throw new AppError( | ||
| 'INVALID_ARGS', | ||
| `Invalid open runtime platform: ${String(value)}. Use "ios" or "android".`, | ||
| `Invalid open runtime platform: ${String(value)}. Use "ios", "android", or "harmonyos".`, | ||
| ); |
Signed-off-by: Ark <artin@cat.ms>
|
Addressed latest Copilot note in |
|
Correction: PR head is now |
There was a problem hiding this comment.
🟢 Approval recommended
The change is strictly additive across the shared contract and validation layers, and is backed by focused unit tests plus wire-compat ledger updates.
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
BLOCKED at exact head 68c21a0. The earlier contract/surface findings are resolved, but this head has zero GitHub check runs, so there is no exact-head CI evidence. The linked #2265 acceptance path is also still unproven: please record a HarmonyOS proxy inventory → open → snapshot/input/logs → close run with lease identity and artifact provenance. Add focused planted-red route coverage for daemon runtime-schema acceptance, runtime normalization/typing, leaf-only HarmonyOS compatibility while preserving non-leaf apple, and proxy inventory → leaseBackendForDevice → allocation/materialization. The two new unit tests do not exercise those production routes. |
|
Still BLOCKED at exact head 68c21a0. The branch is now conflicting with main and still has no GitHub checks. The earlier production-route evidence gap remains: resolve the conflict, cover runtime-schema admission through provider inventory, Harmony lease allocation/materialization, and proxy open/snapshot/input/logs/close with preserved identity/provenance, include planted-red/live evidence, then run exact-head CI. |
|
Sentinel recheck at exact head
Resolve the branch conflict, cover the real proxy inventory → backend → allocation/materialization path, record required live lifecycle evidence, and run exact-head CI. Do not apply |
|
Re-review at exact head |
Summary
harmonyos-instanceto the shared lease backend contractMotivation
The proxy inventory already exposes HarmonyOS devices and the local host runtime can operate them, but 0.20.10 rejects remote
openbefore lease admission because no HarmonyOS backend exists. This keeps provider lifecycle fail-closed while enabling the existing generic lease path to carry HarmonyOS.Validation
corepack pnpm typecheckcorepack pnpm test --run src/__tests__/remote-connection.test.ts(50 passed)git diff --checkThis is intentionally a design/minimal contract PR. Physical OHOS devices are out of scope; end-to-end daemon/provider implementation and lease tests are required before release.