Repository navigation
fix: authenticate log streaming, artifact downloads, and video playback - #1531
Josh Duffney (duffney) wants to merge 2 commits into
Conversation
Extract final delivery implementation from source 1f8ea3c onto agreed main 2101bd0. Retain existing auth, project scoping and report behavior; exclude RBAC and mutation-idempotency changes. Record isolated non-RBAC browser acceptance and remaining deployment/browser limits. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f31285c-39b5-461e-9550-48f23fcf2882
Test Results (Node.js 22)test: Run #342
🎉 All tests passed! |
|
Reviewed the authenticated-delivery changes and the local real-worker test results. I recommend fixing these two P2 issues before merging. 1. [P2] Recognize ky network errors when retrying artifact requestsLocation: The retry guard checks Reproduction: With the real Suggested resolution: Use ky’s 2. [P2] Isolate native download navigation in the interaction-story testsLocation: The cancellation story uses a fixed 500 ms response delay. Under slower execution, it can pass even though a download has already been dispatched. Happy DOM then navigates the document to Reproduction: This also occurs with Suggested resolution: Stub native anchor download navigation in the test harness and restore the document URL between tests. Replace the timing-dependent response delay with a controlled pending response, and assert that cancellation aborts the request without dispatching a saved file—not only that the button becomes enabled again. Validation and delivery evidence
The real-worker recording was only 5.56 seconds, so this session did not independently establish throttled long-video playback or seeking beyond buffered bytes. It also did not repeat the full account/project/logout, cross-client, worker-restart, production-nginx, or prefix-deployment acceptance matrix. Earlier model/Judge compatibility failures were local environment issues, not demonstrated regressions in this PR. |
Recognize wrapped browser network failures at GET establishment while preserving bounded retries, cancellation and no accepted-body replay. Keep cancellation-story responses pending until abort, prevent native download navigation in the harness and verify abort without download dispatch. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f31285c-39b5-461e-9550-48f23fcf2882
|
Addressed both P2 findings from #1531 (comment) in 99e4156.
Validation on the follow-up:
Existing MSAL/nested-button and bundle-size warnings remain nonblocking. No new native-browser/deployment acceptance is claimed for this focused follow-up; previously documented limits remain. The change is pushed without rewriting the original delivery checkpoint; the dependent upper-layer owners are being notified of the new lower SHA. |
Test Results (Node.js 22)test: Run #372
🎉 All tests passed! |
Summary
Authenticate all three browser delivery surfaces through the existing bearer transport: live/stored SSE logs, buffered artifact/archive downloads, and native progressive video playback/seeking. Bind delivery to account, project, resource and resolved attempt; cancel replaced sessions and never substitute latest artifacts for unavailable historical attempts.
Video uses a narrowly routed, client-bound service-worker bridge with authenticated range requests, backpressure and cancellation, rather than full-recording Blob buffering. The API validates ranges/selectors and streams abortably; nginx serves the actual worker asset and denies uncontrolled bridge routes. Tokens stay out of URLs and the worker; protected responses are not persisted in CacheStorage.
This is the independently usable lower layer, extracted onto
2101bd0d61c877caadb5c60d03056c67a0b08296from final source1f8ea3c92f03b1516c9483fb02c0c47c6c4ccd48. It does not introduce new RBAC, memberships, capability policy, authentication-mode changes, mutation idempotency or report shutdown. Existing bearer/login transport, project scoping, report functionality and no-token rollout remain intact. The separate upper RBAC contribution will depend on this branch.Demos
User-visible delivery fixes; before/after walkthrough recordings are not attached. This remains a draft. Generated media was used for the browser checks below, not as a walkthrough or real-worker recording.
Before
Not recorded. Raw browser delivery could not consistently carry the Portal bearer to protected logs, downloads and native media requests.
After
Not recorded. The isolated browser checks below verified authenticated delivery and native progressive playback/seek.
Testing
Previously executed against this exact, unchanged extraction checkpoint; broad suites were not rerun solely for publication.
Passed:
pnpm exec vitest run apps/portal/src/lib/log-stream.test.ts apps/portal/src/hooks/use-log-stream.test.ts apps/portal/src/lib/api-client.test.ts apps/portal/src/lib/artifact-delivery.test.ts apps/portal/src/hooks/use-artifact-session.test.tsx apps/portal/src/hooks/useHarExtraction.test.tsx apps/portal/src/components/ArtifactDownloadButton.test.tsx apps/portal/src/components/artifact-consumers.test.tsx apps/portal/src/components/artifact-stories.play.test.tsx apps/portal/src/pages/RunDetail.artifacts.test.tsx apps/portal/src/lib/authenticated-media.test.ts apps/portal/src/components/VideoPlayer.test.tsx apps/portal/src/components/VideoPlayer.play.test.tsx apps/portal/src/components/LogViewer.scroll.test.tsx --reporter=dotResult: 14 files, 203 tests passed.
pnpm exec vitest run apps/portal/src --reporter=dotResult: 79 files, 658 tests passed.
pnpm exec vitest run apps/api/src/routes/requests/video.test.ts apps/api/src/routes/requests/archive.test.ts apps/api/src/routes/requests/logs.test.ts apps/api/src/routes/requests/har.test.ts apps/api/src/routes/requests/atif.test.ts apps/api/src/routes/requests/snapshots.test.ts apps/api/src/routes/requests/tool-calls.test.ts --reporter=defaultResult: 7 files, 132 tests passed.
pnpm --filter shared buildpnpm --filter telemetry buildpnpm --filter db-migrations buildpnpm --filter api buildpnpm --filter portal buildpnpm --filter portal build-storybookpnpm notice:checkpnpm install --frozen-lockfile --ignore-scripts --offlinegit diff --checkgit merge-base --is-ancestor 2101bd0d61c877caadb5c60d03056c67a0b08296 HEADGenuine isolated non-RBAC browser acceptance passed: Chromium 156.0.8078.12, actual Vite Portal and built API, separate MongoDB/Redis/Azurite/Entra Local fixtures, normal trusted localhost TLS, real MSAL redirect and Scope enrollment. No mocked authentication, benchmark, worker, scheduler or external deployment.
bytes=10059776-, resumed at 70.2148 seconds and canceled two obsolete transfers.Limits: production nginx/prefix-hosted acceptance, Safari/Firefox, worker restart/idle expiry, every interrupted-body/delayed-history browser case, top-level one-shot recording, real-worker capture/upload and Mongo change-stream completion were not browser-established. Standalone Mongo emitted its expected change-stream limitation during live-log checking; Redis final-event completion passed. Protocol/component/API tests are not claimed as native-browser proof for these remaining cases. No upper-layer/RBAC acceptance is claimed. Existing no-token rollout remains unchanged. Existing Browserslist/bundle-size and some Portal test warnings were nonblocking.
Documentation and compatibility
Updated realtime data flow, authenticated progressive recordings, codebase delivery and the architecture index in
AGENTS.md.No database migration or new CLI capability is required: these fixes use existing API artifact/log routes and preserve CLI behavior. Archive downloads retain complete-file buffering; videos do not. The internal Portal batch-download helper now requires explicit project and cancellation context. Rebuild the Portal image to include the public worker asset; a source hot reload alone does not update a copied deployment asset. Progressive playback requires service workers, streaming responses and trustworthy localhost/HTTPS; unsupported browsers fail visibly without a full-video fallback. Prefix-hosted deployments need matching worker and deny routes as documented. No deployment was performed.
Added pinned
eventsource-parser@3.0.6, its canonical lockfile entries and license notice; unrelated lockfile churn was excluded.Checklist
up()/down()and keep it CosmosDB-compatible.NOTICE/NOTICE-REVIEW.txtwithpnpm noticeas needed.Migration item: N/A, no database/model changes. CLI item: existing CLI capabilities and API contracts are retained; no new CLI command is needed. Demo items remain outstanding, not marked N/A for this user-visible fix.