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
|
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 |
|
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'); | ||
| }); |
|
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
|
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".`, | ||
| ); |
|
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 |
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>
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 <raft-mobile-ark@mail.build>
68c21a0 to
fa0b33c
Compare
|
Follow-up pushed at fa0b33c (rebased onto 367e795). Resolved conflicts without dropping upstream changelog/ledger entries; fixed Node runtime normalization and .ad open/runtime-set roundtrips through one kernel runtime-platform vocabulary; corrected runtime-set guidance. A new proxy inventory/materialization test also exposed missing HarmonyOS serial propagation, now fixed. Local affected gates passed, including 4,834 related tests and wire compatibility; the PR body has exact evidence and RED/GREEN details. Live proxy/device evidence is still NOT RUN because local DevEco startup is blocked on license acceptance. I am not requesting ready-for-human or claiming merge readiness. Could a maintainer approve/trigger the upstream exact-head CI if this fork run requires approval? I will continue with the live lifecycle receipt when the emulator is available. |
There was a problem hiding this comment.
🟡 Changes recommended
A follow-up update is needed so remote-bridge gating recognizes harmonyos-instance; otherwise HarmonyOS proxy leases may silently skip metro/bridge behavior in dependent CLI commands.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 18/18 changed files
- Comments generated: 1
- Review effort level: Lite
| const LEASE_BACKENDS = [ | ||
| 'ios-simulator', | ||
| 'ios-instance', | ||
| 'android-instance', | ||
| 'harmonyos-instance', | ||
| ] as const; |
|
The earlier runtime-hint, script roundtrip, and serial propagation gaps are fixed in fa0b33c. No further code findings from this review. The remaining requirement is a live HarmonyOS proxy run covering inventory → open → snapshot/input/logs → close, with lease and artifact evidence; the reported DevEco license prompt still blocks that validation, so ready-for-human should wait. CI separately needs maintainer approval before it can run; this is not a test failure. |
Summary
HarmonyOS devices exposed by a proxy can select the additive
harmonyos-instancelease backend. Selected serials survive command materialization, and HarmonyOS runtime hints survive Node response normalization and.adrecording/replay. Runtime platform validation is derived from one kernel vocabulary; existing non-leafappleselection remains compatible, while leaf mismatches remain strict.Related to #2265. Unsupported providers remain fail-closed; this does not add HarmonyOS capacity to iOS/Android-only cloud providers.
Validation
Published head:
fa0b33c679820943bf15e7228cba3d0dedcfa033, tree6218ca9415334b96080e427d9f81fed6ee880214, base367e795ee72458ef2a340a627e5e9a1680c03712. 18 touched files in the complete PR.Local affected gate passed: format, lint, typecheck, layering, dead-code audit, build, 4,834 related tests in 628 files, and released-surface wire compatibility. The first related run exposed a host
AWS_REGION=ap-southeast-1assumption in the unchanged AWS profile test; the isolated test passed with AWS region overrides unset, then the complete affected gate passed under the same clean environment. No test skipped or unrelated AWS code changed.Freshly rebased onto upstream main; preserved the existing changelog entries and wire ledger. Response normalization and script roundtrip regressions were observed failing before the fix. The proxy route test exposed missing serial propagation before that was corrected. Removing HarmonyOS from the shared runtime vocabulary makes the schema/open normalization regression fail.
Tests cover mocked proxy inventory through backend inference, command materialization, real LeaseRegistry admission and release, plus leaf/non-leaf runtime compatibility. These are not live-device receipts.
Live HarmonyOS proxy inventory → open → snapshot/input/logs → close remains NOT RUN: the local DevEco emulator refuses startup pending human acceptance of its updated license, and no physical OHOS seat is available. Exact-head upstream CI and maintainer review are still required. This PR is not merge-ready and must not receive
ready-for-humanon the strength of local tests alone.CLI lease grammar, runtime guidance, and Unreleased changelog are updated; no skills changed. The scope crosses the runtime-platform contract, its CLI/Node/daemon/script consumers, and focused tests as required by the end-to-end issue.