Skip to content

docs(remote): say only the host ends a macos-app lease - #3253

Open
janicduplessis wants to merge 2 commits into
callstack:mainfrom
janicduplessis:docs/macos-app-lease-host-owned
Open

janicduplessis wants to merge 2 commits into
callstack:mainfrom
janicduplessis:docs/macos-app-lease-host-owned

Conversation

@janicduplessis

@janicduplessis janicduplessis commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Follow-up to #3236 (non-blocking review notes).

  • ADR 0007 and remote-proxy.md said the lease's own release is allowed without a session. A tenant lease_release is refused with MACOS_APP_LEASE_HOST_OWNED, so both now say a tenant cannot release the lease and that it ends by host DELETE /admin/leases/<lease-id>, expiry, daemon restart, or closing a session when the host set retainOnClose to false.
  • Removed lease_heartbeat and lease_release from SESSIONLESS_COMMANDS in src/daemon/macos-app-lease.ts: both are exempt from lease admission, so assertMacOsAppLeaseAdmitsRequest never sees them.

Checks: pnpm check:affected --run passes at b6a6f59.

Review in cubic

Copilot AI balanced review requested due to automatic review settings October 6, 2026 12:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The documentation and cleanup consistently reflect existing lease admission and release behavior.

Review effort: Balanced
Findings: None

What changed in this PR

Aligns macOS app lease documentation with host-owned release semantics and removes unreachable sessionless-command entries.

Changes:

  • Documents valid host-controlled lease termination paths.
  • Removes tenant lease_release from allowed macOS app lease commands.
  • Simplifies sessionless admission to open and batch.
File Description
docs/​adr/​0007-remote-device-leases.md Clarifies lease lifecycle rules.
website/​docs/​docs/​remote-proxy.md Updates user-facing lease guidance.
src/​daemon/​macos-app-lease.ts Removes admission-exempt commands from the sessionless set.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 3 files

Reply to a comment to ask cubic a question or push back. It learns from your replies.

Turn on auto-fix | Re-trigger cubic

Comment thread website/docs/docs/remote-proxy.md Outdated
Comment thread docs/adr/0007-remote-device-leases.md Outdated
@thymikee

thymikee commented Oct 6, 2026

Copy link
Copy Markdown
Member

I read the docs hunks at 7a0cc02 and found two docs errors to fix before merge. The code change only removes two unreachable SESSIONLESS_COMMANDS entries in src/daemon/macos-app-lease.ts, so runtime behavior does not change. Please fix these two: DELETE /admin/leases in website/docs/docs/remote-proxy.md and in the ADR needs the /admin/leases/<lease-id> form, because the bare path returns 405 for DELETE (the same page already shows the id form at line 125); "only the host ends it" in docs/adr/0007-remote-device-leases.md conflicts with the list that follows it, so say that a tenant cannot release the lease and that it ends by host DELETE, TTL expiry, daemon restart, or closing a session whose host set retainOnClose false. Not blocking: the ADR line still lists the lease heartbeat as a carve-out, but the registry's leaseAdmissionExempt trait exempts it, so drop it from that list or point it at the trait. The PR body could also mention the dead-entry removal.

Two open threads still apply: #3253 (comment) (bare DELETE path) and #3253 (comment) (host-ends-lease wording). Both are in the notes above, so fixing them should let you resolve both threads.

I did not run the daemon or the tests. The Smoke Tests job failed at "Preflight iOS runner through public CLI", where prepare ios-runner could not start the daemon within 15 seconds. That looks unrelated, because the diff shares no code with daemon startup or the iOS runner, but I did not rerun the job. I know of no conflicts. Before merge, please fix the two docs as described and rerun the failed iOS preflight check.

Copilot AI balanced review requested due to automatic review settings October 7, 2026 03:49
@janicduplessis

Copy link
Copy Markdown
Contributor Author

@thymikee pushed b6a6f59. Both docs now use DELETE /admin/leases/<lease-id> and say a tenant cannot release the lease; it ends by host DELETE, expiry, daemon restart, or closing a session when the host set retainOnClose to false. I dropped the heartbeat from the ADR's session carve-out and updated the PR body for the dead-entry removal. pnpm check:affected --run passes locally. I will check the iOS preflight rerun on CI.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The updated documentation still incorrectly implies that heartbeat requires an opened app session.

Review effort: Balanced
Findings: None

Previously missed (2)

In code that hasn't changed since last review

Low severity Clarify comment excludes sessionless lease heartbeat requests

src/​daemon/​macos-app-lease.ts:41

This list is no longer all requests that run without a leased-app session: lease_heartbeat still runs sessionlessly because its registry exemption bypasses this helper. Qualify the comment as describing only requests that reach macOS-app admission so future changes do not mistake heartbeat for session-bound behavior.

Low severity Document lease heartbeat as an exception to session requirements

website/​docs/​docs/​remote-proxy.md:169

The next paragraph still says every command other than open and batch requires the opened session, which incorrectly includes the heartbeat now retained here. lease_heartbeat is admission-exempt and is explicitly tested without a session in src/daemon/__tests__/request-execution-scope-macos-app-lease.test.ts:88-94; document that exception explicitly.

@thymikee

thymikee commented Oct 7, 2026

Copy link
Copy Markdown
Member

The earlier findings on 7a0cc02 are fixed at b6a6f59, and I found no new problems. The remote-proxy page now says DELETE /admin/leases/<lease-id>, which matches the route in host-lease-http.ts. The ADR no longer says only the host ends a lease. It now lists host DELETE, expiry, daemon restart, and session close with retainOnClose false, and it says a tenant cannot release the lease. The src change only removes a dead entry that no admitted route reaches. I did not run the test suite or pnpm check:affected. I relied on the 16 checks, which all pass. The daemon-restart claim rests on a grep for persistence in lease-registry.ts, not on a live restart. I only re-read the ADR and remote-proxy.md sections this change touches. Both cubic-dev-ai threads are fixed at this head, so you can resolve them: #3253 (comment) and #3253 (comment). There are no conflicts. Nothing else stops the merge, so it is ready once the maintainer is satisfied.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 7, 2026

This branch has not been deployed

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

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants