Repository navigation
Conversation
|
@alimek is attempting to deploy a commit to the Callstack Team on Vercel. A member of the Team first needs to authorize it. |
31e9317 to
3d82dda
Compare
|
I read the Tizen plugin at 31e9317 and found two defects and a gap in live evidence. The only failing check is Vercel, which is waiting for a team member to authorize a deploy from the fork, so it never built and is not caused by this change. I found no conflicts. I could not run a TV or sdb, so I reviewed the code only and could not reproduce the live claims in the PR body. A cancelled or timed-out Two leases can hold the same TV at once. The docs and help say the forward is removed when the lease ends, but removal is fire-and-forget with errors swallowed (sdb.ts:154), and the forward lives in the sdb server, so a leak outlives the daemon. The PR body lists happy-path commands without output. After the fixes above, please attach live output at the new head showing Not blocking, and fine to take or leave: Could the Tizen layer get smaller by routing sdb through Before merge, make the Tizen launch and close path cancellable through the command layer, key lease exclusivity on the canonical serial, and attach the live cleanup and failure-path evidence. This review covers 31e9317. The newer head 3d82dda changes only tv-cdp page-interactor.ts and its test, which the points above do not touch. |
|
I re-read the Tizen code at 3d82dda. The Tizen commit is unchanged by the rebase, so the points in my earlier review (#4168 (comment)) still stand:
The non-blocking notes from the earlier review also still apply. The only failing check is the Vercel fork-deploy authorization gate, so no build ran and it is not caused by this change. |
3d82dda to
50e9dcd
Compare
|
I re-read 50e9dcd. The Tizen commit is unchanged by this rebase onto the new #4098 base, so the points in my earlier review (#4168 (comment)) still stand: One addition: #4098 now re-wraps only failures with The only failing check is the Vercel fork-deploy authorization gate, so no build ran. |
a5638c3 to
9df0236
Compare
|
The cancellation and physical-TV ownership defects remain at 9df0236. Compared with the earlier review, no runtime or test change fixes either issue; the existing tests do not cover real Tizen launch cancellation or aliased TV ownership. This assessment uses static review of the supplied evidence and source; no tests or device runs were performed. The Tizen connection wrapper still drops the request signal passed by tv-cdp. Launch, close and listApps can continue vendor work after cancellation, and launch can create or retain a forward and race a retry. The owning TvConnection contract and Tizen adapter must carry the request signal through all three operations. Vendor processes must use host-kit runCmd/runCmdBackground, waits must support cancellation, and abort or failure must remove any created forward independently of the aborted signal. Preserve typed cancellation and timeout errors; rewrite only processExitError failures. The connection identity still keeps the requested name even when sdbSerial resolves IP and IP:26101 to the same TV. Named sessions using those aliases, or implicit selection, can bypass DEVICE_IN_USE and cold-launch over another session's app. TvConnection must expose a canonical identity: the resolved sdb serial for Tizen and the ares device name for webOS. tv-cdp must use it consistently for session and lease keys, holder checks, release and device-to-session lookup, while retaining the requested name for daemon selection. Verify through the production lease route that a second named session using the other alias receives DEVICE_IN_USE. Forward removal still discards errors, so the older successful command list does not prove that the persistent sdb forward is removed. After both fixes, provide commands, source SHA, device and output showing this session's forward absent from sdb forward --list after disconnect, cancellation during debug launch, cancellation after forward creation while waiting for a page, and a failed open returning COMMAND_FAILED. Also show opening an already-running app; two explicitly named sessions using IP and IP:26101 with the second refused as DEVICE_IN_USE; an unreachable TV returning a typed failure with elapsed time; and missing sdb returning TOOL_MISSING. The sole failing check appears unrelated: Vercel rejected fork-deploy authorization before any build route ran, consistent with the bot's team-authorization request, so it did not exercise the website changes. No conflicts are known. Before merge, fix cancellation and canonical identity, then provide the complete live evidence and validation receipt at the resulting head; Vercel authorization remains external. |
9df0236 to
1fae4c1
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 28 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/provider-tizen/package.json">
<violation number="1" location="packages/provider-tizen/package.json:2">
P2: This new package is publishable as soon as the PR merges, before the required npm placeholder transition is completed. Add `"private": true` until the placeholder is published under a non-`latest` tag and the release workflow is configured as its trusted publisher, then remove it in the follow-up change.
(Based on your team's feedback about new public package publishing sequence.) [8d2d0e70-d7f9-4595-b79d-236327ddda02]</violation>
</file>
<file name="packages/provider-tizen/README.md">
<violation number="1" location="packages/provider-tizen/README.md:68">
P2: This instruction is incorrect: `connect` allocates the provider lease and already lists/connects the TV over `sdb`; `open` only launches the app and attaches to its DevTools page. Update the sentence so users do not expect device errors or network activity to be deferred until `open`.</violation>
</file>
<file name="packages/provider-tizen/src/tools.ts">
<violation number="1" location="packages/provider-tizen/src/tools.ts:33">
P2: Automatic SDK discovery cannot find `sdb.exe` on Windows because this candidate omits the executable extension. Include the platform-specific executable name (or resolve the candidate using the host command layer), otherwise Windows users must supply `sdbPath` manually despite the documented SDK discovery.</violation>
</file>
<file name="packages/provider-tizen/src/sdb.ts">
<violation number="1" location="packages/provider-tizen/src/sdb.ts:155">
P2: `kill` implements close without a cancellation signal, so a canceled `close` can wait for SDB's full 60-second timeout; thread the request signal through the close contract and this call.</violation>
<violation number="2" location="packages/provider-tizen/src/sdb.ts:162">
P2: This catch hides every non-cancellation `was_kill` failure, including transport errors; ignore only the documented “app not running” result and propagate genuine SDB failures.</violation>
<violation number="3" location="packages/provider-tizen/src/sdb.ts:173">
P2: The inspector probe ignores the launch signal and deadline, so a stalled `/json` fetch can exceed `timeoutMs` and delay Ctrl-C by five seconds; make `listTargets` signal-aware or race this probe with the request signal.</violation>
</file>
<file name="packages/provider-tizen/src/runtime.ts">
<violation number="1" location="packages/provider-tizen/src/runtime.ts:33">
P2: A failed `sdb connect` for an unreachable requested TV escapes as `COMMAND_FAILED` instead of the `DEVICE_NOT_FOUND` error this resolver constructs. Catch connection-command failures, recheck the device list, and preserve tool-missing errors so callers receive the intended device-selection failure.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
| @@ -0,0 +1,49 @@ | |||
| { | |||
| "name": "@agent-device/tizen", | |||
There was a problem hiding this comment.
P2: This new package is publishable as soon as the PR merges, before the required npm placeholder transition is completed. Add "private": true until the placeholder is published under a non-latest tag and the release workflow is configured as its trusted publisher, then remove it in the follow-up change.
(Based on your team's feedback about new public package publishing sequence.) [8d2d0e70-d7f9-4595-b79d-236327ddda02]
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/provider-tizen/package.json, line 2:
<comment>This new package is publishable as soon as the PR merges, before the required npm placeholder transition is completed. Add `"private": true` until the placeholder is published under a non-`latest` tag and the release workflow is configured as its trusted publisher, then remove it in the follow-up change.
(Based on your team's feedback about new public package publishing sequence.) [8d2d0e70-d7f9-4595-b79d-236327ddda02]</comment>
<file context>
@@ -0,0 +1,49 @@
+{
+ "name": "@agent-device/tizen",
+ "version": "0.21.36-dev",
+ "type": "module",
</file context>
There was a problem hiding this comment.
Keeping the package public in this PR. With "private": true, the layering scan requires its bundled workspace imports under dependencies, and then the packed tarball cannot install (@agent-device/host-kit@0.0.0 is not on npm). We would like a maintainer to publish the placeholder and configure the trusted publisher before merge, as was done for webOS. Leaving this open for that.
1fae4c1 to
6c8b3ef
Compare
|
Thanks for the re-review. Both defects are fixed at
Cancellation. Identity. Forward removal. Validation at Live, Tizen (Samsung TV, Tizen 9.0, sdb 4.2.36 from the VS Code Tizen extension). Source is the working tree that became Tizen outputLive, webOS (LG 32LQ63006LA, webOS TV 22), same source. The TV is shown as webOS outputThe Vercel check still needs team authorization for fork deploys. |
There was a problem hiding this comment.
All reported issues were addressed across 13 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
|
Both defects from the earlier review (#4168 (comment)) are fixed at 6c8b3ef, and I found no new blocking problems in the code. The Tizen and webOS live output is author-reported; I could not reproduce it without a TV, sdb or ares. It covers disconnect, a failed open and both cancel points, but not a Tizen Not blocking, and fine to take or leave: when a failed tunnel removal coincides with another failure, On the open inline threads: the cubic-dev-ai threads on the package being public (#4168 (comment)), webOS device identity (#4168 (comment)) and the shutdown fence (#4168 (comment), which I did not trace) still stand. Seven lower-priority threads also still apply or are benign. These do not apply and can be resolved: the alias lease id (#4168 (comment)) is fixed at head with the IP alias refused as DEVICE_IN_USE; the deferred allocation in README (#4168 (comment)) is correct as written; the dropped non-cancel was_kill failure (#4168 (comment)) resurfaces typed from the debug launch; the kill signal (#4168 (comment)) is wired through |
6c8b3ef to
496bd62
Compare
|
Thanks. This round is at From your comment
cubic threads: all replied. Fixed: the identity and shutdown threads, Validation at Live, Tizen (same TV as before, source = the tree that became Tizen outputLive, webOS (same LG TV; webOS output |
There was a problem hiding this comment.
All reported issues were addressed across 17 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
496bd62 to
719b56a
Compare
|
The earlier findings from 6c8b3ef are fixed at 496bd62, and I found no new problems in the code. Leases are now keyed by the sdb serial, the shutdown fence is in place, the debug-package prerequisite and the Backspace wording are corrected, and the cancelled-launch paths are bounded. Not blocking: the one shutdown test in packages/tv-cdp/src/runtime.test.ts:325 runs before allocate, so it covers only the check at runtime.ts:201. Deleting the post-connect check at runtime.ts:206 keeps every test green. One test would cover it: start allocate with a connectTv that waits on a gate, run shutdown, open the gate, then assert that allocate rejects with the shutting-down error and that deviceInventoryProvider returns null for that lease. Take it or leave it. Two open inline threads still apply. The first is the unpublished package (#4168 (comment)). The second is the webOS entry-name fallback in ares.ts, which is not blocking (#4168 (comment)). The other inline threads are fixed or benign at this head, so the author can resolve them. CI is likely unrelated. Vercel failed at fork-deploy authorization before any build, and the other 17 checks pass, including Smoke Tests. The diff touches the docs pages Vercel builds, so the website build for them is unchecked, not failed. There are no conflicts. The Tizen and webOS live output is author-reported, and I could not reproduce it without a TV, sdb or ares. The fakes cover the cancelled open, the post-connect fence, the ares-inspect SIGKILL escalation, the 5 s sdb removal deadline and sdb.exe discovery on Windows. None of these needs a device run to merge. I did not confirm that every ares-setup-device --listfull entry carries deviceinfo.port, so the bare-IP fallback could give a TV a second identity. I did not reproduce the packed-tarball failure that the author gives as the reason to keep the package public. I did not rerun pnpm check:affected. Before merge, a maintainer needs to publish the @agent-device/tizen placeholder under a non-latest dist-tag and configure its trusted publisher, and authorize the Vercel fork deploy so the docs build runs. This review covers 496bd62. The newer head 719b56a changes only the webOS device-id wording in docs and help, which does not change the points above. |
…t signal A TV reached by two aliases (a Tizen IP and IP:26101, or two ares entries for one webOS TV) was two TVs to tv-cdp, so a second named session could take a held TV and cold-launch over its app. close and listApps also dropped the request signal, and a tunnel removal that failed went unreported. - TvConnection gains id, the one identity of the physical TV. Leases, sessions, the DEVICE_IN_USE check, release and device lookup use it; the device keeps the requested name so the daemon still selects it by --device. webOS uses the ip:port its ares entry reaches. - close and listApps take the request signal, and tv-cdp passes it through, as does the inspector target list. - PageTunnel.close resolves once the vendor tunnel is gone. close, a failed open and release wait for it, and withTunnelRemoval keeps the primary error with the removal failure in details. A removal from a page that dropped in the background fails the next close or release; a cancelled open stops waiting for it. - A runtime that is shutting down refuses new leases. - webOS waits for ares-inspect to exit, with SIGKILL after 5 s.
Add @agent-device/tizen, an optional provider plugin that drives web apps on Samsung Tizen TVs, built on the shared tv-cdp layer. The TV enters as an Android TV device owned by the plugin (id tizen:<device>). open kills the app, debug-launches it with sdb shell 0 debug, forwards the DevTools port with sdb forward, and drives the page over the Chrome DevTools Protocol. - sdb is found in the VS Code Tizen extension's SDK ($TIZEN_SDK or ~/.tizen.path.config) or at the sdbPath option; --device takes a TV IP (sdb connect <ip>:26101 runs when needed) or an sdb serial. - v1 scope matches webOS: connect, open (--relaunch), close, apps (vd_applist), appstate, snapshot, keys (directions, select, back as XF86Back), pointer, text, scroll, screenshot and logs. - Docs: add the Samsung Tizen page, mention it in plugins, commands and the sidebar, and add a Tizen flow to help remote.
719b56a to
c6092f2
Compare
|
Thanks for the re-review. Both points are in
tv-cdp, webOS, Tizen and help tests pass (197/197); typecheck, oxlint, format and doc-prose are clean. The public-package thread stays open for the npm placeholder and trusted publisher. |
|
The code at c6092f2 has no new problems since the earlier review. The webOS address documentation and implementation now agree, and the final changes add a deterministic shutdown test. Live evidence remains author-reported from the earlier commits; no new tests or live device runs were performed for this review. The npm publishing prerequisite still applies and is the sole open reviewer issue that must be resolved before merge. The webOS address documentation thread no longer applies because the docs and CLI help now match the implementation’s ip:port and USB entry-name branches, and it is already resolved. The supplied checks show 17 passes and only a Vercel authorization failure, which appears unrelated to the code and provides no build diagnostics; the website build remains unverified. No conflicts are known. Before merge, a maintainer must publish the @agent-device/tizen placeholder under a non-latest dist-tag and configure its trusted publisher as required by docs/agents/pull-requests.md; the author still reports this prerequisite as outstanding. Authorize Vercel and confirm that the website build passes. |
Summary
Adds
@agent-device/tizen, an optional provider plugin that drives web apps on Samsung Tizen TVs on the shared@agent-device/tv-cdplayer from #4098. The TV enters as a plugin-owned Android TV device (tizen:<sdb serial>):openkills the app, debug-launches it withsdb shell 0 debug, forwards the DevTools port withsdb forwardand drives the page over CDP.Two commits:
fix(tv-cdp):TvConnectiongains a canonicalid(the sdb serial; for webOS the ip:port its ares entry reaches) that keys leases and sessions, so two aliases cannot both hold one TV.closeandlistAppscarry the request signal, tunnel removal is awaited and reported, and shutdown fences new leases.feat(provider-tizen): the plugin, scope as webOS v1. Cancel reaches every sdb step and removes the forward.agent-device plugins add @agent-device/tizen export AGENT_DEVICE_SESSION=checkout-tv agent-device connect tizen --device 192.0.2.10 agent-device open com.example.app40 files. Also: catalog entry, help line, docs page; the packed smoke names its session. Before merge, a maintainer publishes the npm placeholder and sets the trusted publisher. No linked issue.
Validation
Tested SHA
496bd62fc, onmainat541099107; headc6092f265adds a shutdown test and a webOS endpoint simplification (197/197 affected package tests).check:affected --run: 14/15 stages; unit 17101/17103. Failures are the knowncommitted-source-treeflake and an Androidmanifest.test.tstimeout that passes alone.Risks: experimental; no CI device lane.
🤖 Generated with Claude Code