Repository navigation
Conversation
The thrown message is now "tenant isolation requires lease id from leases.allocate.", and the admission tests match that wording. The Limrun page adds a provider that calls leases.allocate in acquire, returns lease.leaseId with sessionIsolation set to tenant, and calls leases.release with the same id. The note states that the daemon is started with the instance URL and token, and that neither call creates or deletes the instance. Fixes callstack#3350
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
|
Reviewed e9b6074. The code change looks good: the new error text points to leases.allocate, and I found no blocking problem. No conflicts. The failing check is the Android emulator smoke job (helperTransport 'instrumentation' vs expected 'persistent-session'). It looks unrelated, since this PR only changes the INVALID_ARGS message text, the admission test regexes and a docs page, and the smoke scenario does not reach that missing-leaseId path. I did not check whether it also fails on main. A second Smoke Tests job is still queued. Not blocking: the limrun.md example (https://github.com/callstack/agent-device/blob/e9b6074/website/docs/docs/limrun.md#L116) allocates without retainOnClose: true while the provider releases the lease itself, so a session close in between would end the lease and the release would return released:false. Adding the flag with one sentence of explanation would fix it. Also, "leases.allocate" is an SDK name, so CLI-only users who hit this INVALID_ARGS path see a method name they cannot call. A typed hint could carry that pointer instead of the message text. You can take or leave both. The open inline thread on the limrun.md example still applies (#3635 (comment)). It is the same retainOnClose gap as above, and it is a doc gap, not a production defect. I did not run request-admission.test.ts, so the 15-pass claim is unverified. I checked the docs example against the type declarations and registry code, not against a running daemon. I also did not check whether a CHANGELOG entry is needed for the message change. Before merge, optionally add retainOnClose: true to the example, then rerun the Android smoke job. |
The provider example releases its lease itself, so allocate it with retainOnClose: true. Without it, a session close before release ends the lease and leases.release returns released: false. Also note that a retained lease still expires after its ttlMs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KFTE4BHbGYzgf8Uhzcppdh
|
Added |
|
The fixes since e9b6074 hold at d42f9b5: limrun.md now passes retainOnClose: true and explains the lifecycle, which matches lease-registry.ts:297. I did not re-check the one resolved inline thread on its own. I did not re-run the request-admission tests or lint. I did not compile or run the docs example, which is a docs-only change. I found no new code problems. Smoke Tests fails in the iOS runner XCTest testSynthesizedReplacementLeavesAMaskedFieldUnverified. This PR changes one error string, test regexes and docs, so it does not touch that route, and the failure looks unrelated. The author's Android-flake explanation does not match the log. Repo Guards fails in check:affected lint after about 0.02s and prints no diagnostic, so I cannot tell the cause. The base is stale, and fallow flagged 882 changed files against the old base fc224bc, which a 3-file diff cannot cause. There are no conflicts. Could you rebase onto current main and rerun CI? A clean rerun would show that the Repo Guards lint and the iOS Smoke failures come from the stale base or a flake. A small question, not blocking. The pointer to leases.allocate is remediation guidance, and AppError already has a |
Summary
Enabling tenant isolation without a lease id fails with an invalid-arguments error that only says a lease id is required. The Limrun guide also never showed how a provider should attach to an instance a harness already owns. normalizeLeaseAdmissionRequest throws INVALID_ARGS with the literal "tenant isolation requires lease id." when normalizeLeaseId returns nothing, and admission uses that check for close and for other commands. The Limrun page described passing an existing instance URL and token, then stopped short of allocating a lease and placing the returned id on the client scope.
The thrown message is now "tenant isolation requires lease id from leases.allocate.", and the admission tests match that wording. The Limrun page adds a provider that calls leases.allocate in acquire, returns lease.leaseId with sessionIsolation set to tenant, and calls leases.release with the same id. The note states that the daemon is started with the instance URL and token, and that neither call creates or deletes the instance.
Fixes #3350
Test plan
npx --no-install vitest run src/sessions/tests/request-admission.test.ts finished with all 15 tests passing.