Repository navigation
feat: unify video sharing across CLI and local Studio - #7135
Conversation
15a19b9 to
545cc86
Compare
3168865 to
c08e949
Compare
545cc86 to
23158dd
Compare
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
5461341 to
2f79994
Compare
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at 588060c03715b3ab10d641dde941ee2bc71ea79a.
The command reuses @hyperframes/share for identity, audience, upload recovery and status rather than implementing another sharing backend. Embedded rendering also avoids claiming command success before upload (packages/cli/src/commands/render.ts:1254, share/render.ts:66-67).
Important — revalidate first-share consent after rendering (packages/cli/src/share/service.ts:165-176). Start share --private on an unshared project and hold the render. Another CLI/Desktop process creates a public share for that folder while it renders. shareForReview then returns that existing public binding: the shared kernel reuses the recorded project and deliberately skips applying a first audience once record.opened is true (packages/share/src/reviewShare.mjs:453-461, 478-482). This command checks only takedown on the returned share and calls addVersion, publishing its video to the public share despite its explicit private audience. The same gap ignores an explicit --space requested when the folder becomes bound to another workspace during rendering. The earlier assertExistingIntent checks happened before that binding existed.
Fix: validate the actual returned share against input.audience and input.space before calling addVersion; reuse assertExistingIntent rather than duplicating its predicates. Keep the selected project ID pinned into the upload so a later local binding change cannot retarget it. Add held-render tests for concurrent first sharing with a different audience/workspace, expecting refusal and zero calls to addVersion. Check the existing-update path for the same cached-binding gap: it also spans rendering, while addVersion otherwise resolves the target from disk again.
Nit — bound the local render cache (packages/cli/src/share/render.ts:56-86). Each changed project creates another UUID MP4 while the sidecar retains only the newest one; failed or changed-during-render attempts also leave files behind. Remove failed attempt artifacts and prune obsolete successful files once no pending upload needs them, or document and expose a bounded cleanup policy.
Validation at this head: bun run --cwd packages/cli test src/share/auth.test.ts src/share/render.test.ts src/share/render.lifecycle.test.ts src/share/service.test.ts src/commands/render.test.ts src/utils/render-success-state.test.ts → 152 passed. node --test packages/share/tests/kernel.test.mjs → 22 passed. A held-render probe against the actual service (bunx vitest run src/share/service.adversary-7135.test.ts) observes 2 failing consent/workspace safety cases, 10 passing existing controls: both unsafe runs call addVersion and return the concurrent public binding. The sharing primitive is mocked in that probe; the behavior it returns is verified against the real kernel branches cited above. No signed-in upload or live backend/browser E2E was exercised. bun run build passed.
Audited: new share command/auth/host/render/service and tests, shared export changes, render-success lifecycle integration, CLI registration/build/docs diff. Trusting: unchanged render engine interiors and existing backend access enforcement. Parent #7134 is merged and its head is an ancestor here; git merge-tree --write-tree origin/main HEAD succeeds. There were no prior reviews or inline comments when this review began.
Need: yes, CLI entry to existing unified reviews · Size: ok · Reuse: @hyperframes/share, credential resolver and render plan · Simpler: reuse the existing intent guard after rendering · Structure: fine
Risk: medium — upload destination and audience must still match explicit consent after a long render
— hf-review
Verdict: REQUEST CHANGES
Reasoning: A concurrent first share during rendering can redirect this command's upload to a public audience or another workspace without honoring its explicit flags. Revalidate and pin the actual destination before upload.
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at abd4c1a5698554ac0ac885f1218f08ad87ab6ba9, re-reviewing the delta from 588060c03715b3ab10d641dde941ee2bc71ea79a.
The previous consent/destination blocker is resolved. After rendering, uploadTarget checks the actual returned audience and workspace, rejects a changed project binding, and passes the validated project ID directly to addVersion (packages/cli/src/share/service.ts:172-212). Existing updates retain their previously read audience and workspace; status after upload also refuses a different binding rather than reporting another project's URL.
The saved workspace is pinned through first-share preflight and registration retries. Failure cleanup removes only the current render's UUID output and temporary sidecar (packages/cli/src/share/render.ts:65-96). Completed-cache cleanup is documented, addressing the earlier nonblocking retention note. Automatic failure cleanup remains scoped to the current render.
Validation at this head: the six focused CLI suites (bun run --cwd packages/cli test src/share/auth.test.ts src/share/render.test.ts src/share/render.lifecycle.test.ts src/share/service.test.ts src/commands/render.test.ts src/utils/render-success-state.test.ts) → 159 passed. node --test packages/share/tests/kernel.test.mjs → 22 passed. Running the updated service tests against a copy of the previous service gives 7 failures, 10 passing controls; the current service suite passes all 17. Two independent actual-module cleanup probes also pass: a failed replacement preserves the previous video/sidecar, and changed source deletes its rejected render. These are local tests with mocked render/network boundaries, not a signed-in backend replay. bun run build passed. git merge-tree --write-tree origin/main HEAD succeeds.
Audited: all changed files, post-render intent/binding checks, explicit upload ID and shared-kernel target selection, cleanup ownership and added regressions, building on the previous whole-patch review. Trusting: unchanged render-engine/backend interiors. The signed-in dev update reported in the thread was not independently replayed here. A fresh independent adversarial source pass found no additional Important defect.
Need: yes, CLI access to unified reviews · Size: ok · Reuse: existing intent guard and uploader projectId contract · Simpler: already bounded · Structure: fine
Risk: medium — authenticated video uploads and audience/destination selection
— hf-review
Verdict: APPROVE
Reasoning: The reported render-gap consent race and upload retargeting are fixed at this head, with regressions that fail against the previous service. Failed-render cleanup and documented cache removal address the earlier retention note.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Addressed both findings from review 5854052142 in 4ad4f37.
Validation: 140 focused CLI, 45 Studio and 122 kernel tests passed; CLI/Studio/Studio-server builds, typechecks, lint, formatting, complexity and commit hooks passed. Hosted checks are rerunning. No fresh live upload or recipient acceptance is claimed. Keeping this PR draft for the release hold. |
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at 4ad4f377c2f1a2dc53a218f4202c01f3c6a3e420, checking the 17-file delta from 9697c9707fcb272d05d49d47882d05cebda5d016.
The source-consent finding is resolved in the inspected paths. CLI calls default to video-only, including JSON/unattended calls; --include-source supplies affirmative consent and conflicts with --video-only. The service skips source preparation without strict includeSource === true, and the adapter repeats that guard. The Studio checkbox starts unchecked, describes unused/Git-ignored files, and passes its value on creation and updates; video-only recovery explicitly overrides it. The bridge accepts only boolean consent values. The follow-up leaves uploadTarget and checkPreparedTarget unchanged; I also reread their actual post-render audience/workspace checks and the explicit project ID passed to addVersion. A fresh independent read-only adversarial pass found no further Important defect in source selection or pending-upload recovery.
The hourly-limit follow-up is also addressed. pytest-gate.sh node --test --test-concurrency=2 tests/reviewFailurePersistence.test.mjs in packages/share passed 9/9, including the actual registration refusal and its retry sentence recovered by a fresh process. I read the command/service/adapter/bridge and Studio regressions; I did not rerun the broader CLI/Studio suites or a full build, and did not exercise live upload/recipient acceptance.
Important: resolve the current conflict in packages/share/src/reviewVersionSentences.mjs. Against freshly fetched main 8a15d9feed, git merge-tree --write-tree origin/main HEAD exits 1 and reports that single conflicted path. Main now has its own hourly-limit sentence and laterIn(ms, unknown) fallback contract; this branch adds a second version of the hourly entry. Rebase and keep one hourly entry using main's formatter contract, preserving its explicit hourly/daily fallback strings and current refusal wording. Keep the new recovered-status regression and rerun it after resolving. Do not take this branch's whole file over main, which would discard those newer changes. The current conflicting head cannot receive my approval; the source-consent blocker itself is closed.
Need: yes, explicit source scope before upload · Size: ok, focused follow-up · Reuse: existing share operation, adapter, Studio action model and retry formatter · Simpler: keep main's single hourly formatter entry when rebasing · Structure: fine (checked prepareCliShareSource, prepareSelectedSource, actionOptions and QuickShare controls; no split)
Risk: medium - local project uploads now require affirmative scope selection; the merge resolution still needs review.
Verdict: REQUEST_CHANGES
Reasoning: The requested consent and retry-guidance fixes are present. Resolve the one verified conflict with current main while preserving both changes, then request a re-review at the resulting head.
Reviewed by hf-review.
Advertise the CLI sharing bridge before bootstrapping it so standalone Studio does not request unsupported routes. Preserve origin and session checks. Budget the intended sharing artifact: clean main packs at 8,060,519 bytes versus 8,162,979 with sharing (8,169,168 in CI). Required policy and Unicode tables, bundled sharing code, and Studio controls account for the growth. Set the packed-only budget to 8,577,627, following the 5% headroom convention; retain the unpacked cap and all package checks.
4ad4f37 to
f80b3a0
Compare
|
Addressed review 5854332883 in f80b3a0 by rebasing onto main at 8a15d9f. The retry-sentence module now exactly matches main: one hourly entry, its laterIn(ms, unknown) contract, explicit hourly/daily fallbacks and current refusal wording. The fresh-process hourly regression is retained; the refusal-persistence assertion now matches the upstream wording. All 125 kernel, 140 focused CLI and 45 Studio tests pass, as do lint/format/complexity hooks; merge-tree reports no conflict. Source-consent code and UI are unchanged. CI and CodeQL are rerunning; the PR remains draft pending release approval. |
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at f80b3a0c2f5e7c6692222e0b75fbcc2afbb3e549, rechecking the rebase from 4ad4f377c2f1a2dc53a218f4202c01f3c6a3e420.
The remaining conflict is resolved. The retry-sentence module is byte-identical to main at the rebase base 8a15d9feed: one hourly entry, separate hourly/daily fallback strings, and main's current refusal wording. The recovery assertion was updated to that wording and the fresh-process hourly regression remains. git merge-tree --write-tree origin/main HEAD also succeeds against freshly fetched main 7ae1ae6494.
The range-diff shows only the formatter resolution and matching recovery assertion changed within the feature commits; the additional sentence tests come from main. The CLI/service/adapter/bridge and Studio consent implementation is unchanged from the head reviewed last round. Building on the prior consent review and this byte-for-byte preservation, source upload remains opt-in through --include-source or the unchecked Studio choice, with video-only defaults and the existing post-render audience/workspace checks preserved.
Validation at this head: pytest-gate.sh node --test --test-concurrency=2 tests/reviewFailurePersistence.test.mjs tests/reviewVersionSentences.test.mjs in packages/share passed 12/12, covering fresh-process failure recovery, hourly/daily delays and missing-delay fallbacks. This is a focused rebase review building on the earlier full-feature and consent reviews. I did not rerun the broader CLI/Studio suites, full build or live upload/recipient acceptance.
Need: yes, preserve both retry guidance and source consent during rebase · Size: ok, narrow resolution · Reuse: main's existing sentence table and retry formatter · Simpler: already one shared formatter · Structure: fine (checked versionSentence/laterIn and unchanged source-choice operation/adapter/UI; no split)
Risk: medium - authenticated uploads; this reviewed delta resolves formatter drift without changing upload scope.
Verdict: APPROVE
Reasoning: The verified conflict is closed, main's formatter behavior and the recovery regression are retained, and the previously reviewed consent fixes are unchanged. No remaining Important finding in the focused rebase delta. An independent read-only adversarial rebase pass reached the same conclusion.
Reviewed by hf-review.
terencecho
changed the title
What changed
Adds video-review sharing to the CLI and local Studio using the same projects, permissions and allowance as Desktop. Users sign in, choose an audience for the first share, and create a review link. Updates add a version to the existing project; status, copying a link and opening management do not upload local edits. Free workspaces retain the five-project allowance.
--include-source; local Studio offers an unchecked Include editable project files option. Both explain that portable project files can include unused and Git-ignored files.--video-onlyremains supported; conflicting options are rejected..hyperframesignoresupports explicit project-file exclusions.What I measured
At
f80b3a0c2: 125 sharing-kernel tests, 140 focused CLI tests and 45 Studio sharing tests passed. These cover explicit source consent, default/unattended behavior, the local bridge, source preparation, retry recovery and the hourly-limit message recovered in a fresh process. Lint, formatting, complexity checks and commit hooks passed. The rebase preserves the current upstream retry formatter; the consent implementation is unchanged from the preceding tested head.On the preceding consent implementation, CLI, Studio and Studio-server builds and CLI/Studio typechecks passed. Earlier package verification passed all 15 workspace packages and clean-consumer checks. Share and Studio package-size budgets account for the included source-policy assets and sharing UI, using the verifier's standard 5% headroom.
What I did NOT exercise
No fresh production upload/playback, second-account source access, cross-client acceptance or packaged release acceptance was performed for this review follow-up. The wider local macOS CLI suite had failures outside the focused sharing tests, including failures reproduced on clean main; the focused results above are not a claim that the full local suite passed. Hosted checks and review status are shown by GitHub.
The screenshots below are UI evidence. Historical captures and mocked responses are labeled; they do not establish live authentication, billing or upload success.
Before
Historical feature comparison from #7140: local Studio without the sharing entry point at
f78dca1b. The paired Before/After captures used the same guest Chromium session, 1280×720 viewport, Studio fixture and empty credentials. This is not a fresh capture of current main.After
The paired historical capture shows the Share entry point and signed-out recovery. Later control and consent captures follow below.
Sharing controls and failure recovery
Mocked API responses and Free entitlement demonstrate the audience control, upgrade action, retry behavior and retention of the previous ready link. These captures predate the explicit source-consent control shown below.
Explicit source consent
Isolated component captures with mocked workspace/session data, the same 900×760 Chromium viewport, dark theme and no selected audience. Before uses
9697c9707; After shows the consent fix retained in the current head. No project files were uploaded for these captures.