Conversation
425c63a to
e2f5781
Compare
92ad5fb to
d0de354
Compare
XuPeng-SH
left a comment
There was a problem hiding this comment.
Review verdict: changes requested.
GitHub rejected the REQUEST_CHANGES event because the authenticated account is the PR author. Posting the findings as a COMMENT review; this does not set GitHub's formal changes-requested status.
Reviewed commit d0de354 against 4f2b3d0. Request changes for the workspace recovery upload boundary described inline.
Review covered the recovery capture/upload/seal/publication path and cleanup ownership, attachment and interaction fencing, cross-surface state handling, and retry-context placement. A valid snapshot containing a file larger than 4 MiB cannot complete the newly exposed upload protocol.
Independent validation: 12 work-surface harness Python tests and 47 schema-inventory Python tests passed; git diff --check passed. Cargo is unavailable in this review environment, so Rust tests and a live HTTP/database reproduction were not run. The finding is based on the production router, Bytes extractor, capture representation, and seal validation. Static Checks CI passed; Test Suite CI was still running at submission.
| .route( | ||
| "/v1/works/{work_id}/branches/{branch_id}/workspace-recovery-artifacts/{artifact_id}/chunks/{digest}", | ||
| get(crate::server::work_handlers::get_work_workspace_recovery_chunk_handler) | ||
| .put(crate::server::work_handlers::put_work_workspace_recovery_chunk_handler), |
There was a problem hiding this comment.
[P2] Make the file-blob upload contract compatible with the HTTP body limit
A valid snapshot containing, for example, a single 5 MiB tracked fixture can pass begin admission but can never finish this upload. The production router applies DefaultBodyLimit::max(4 * 1024 * 1024) (server/mod.rs:181), and this handler extracts the entire request as Bytes, so the PUT returns 413 before the handler runs. Capture stores each complete file as one blob (workspace_snapshot_capture.rs:969-982); workspace_chunk_refs_match_plan also requires one chunk with exactly that blob's digest and size, so callers cannot work around the limit by splitting the file. There is no corresponding per-file admission limit in the manifest. Please align the transport and snapshot contract (for example, support bounded subchunks with corresponding seal validation, or define and consistently enforce a supported file-size limit), and add a test through the production router for a file over 4 MiB.
There was a problem hiding this comment.
Fixed in 5a1fa2b. Workspace snapshot manifests and capture now reject files over the shared 16 MiB complete-blob limit, capture reads at most limit+1 bytes to stay bounded during growth races, and the production recovery-chunk PUT route overrides the generic 4 MiB body limit with that same contract. Added manifest/capture regressions plus a production-router test sending an 8 MiB blob.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Follow-up deep review of unchanged head d0de354. One additional actionable finding: the Web execution card no longer exposes recovery for a durable switch left in switching after the original handler is lost. Together with the earlier >4 MiB workspace upload issue, the review verdict remains changes requested.
Traced the UI through the read-only observation handler, the committed execution-binding transition, the retry handler's explicit crash-after-begin path, and next-turn admission. Also expanded inspection of branch/Session cleanup, invocation-ledger compaction, interaction-response handling, and tool-result/turn-settlement changes. Setup and API lifecycle shell contract suites passed. Rust and React suites were not run locally (Cargo and Web node_modules unavailable), and no live crash/database reproduction was performed.
This is a COMMENT review because GitHub already rejected REQUEST_CHANGES from the authenticated PR-author account.
| <Button | ||
| size="sm" | ||
| onClick={() => | ||
| void (operation.state === "switching" ? checkSwitch() : retryMove()) |
There was a problem hiding this comment.
[P2] Preserve an explicit resume action for stranded switching operations
If the Server restarts after begin_execution_switch commits but before complete_execution_switch, the durable receipt and binding remain switching. The only action now shown for that state calls checkSwitch(), while retryMove() explicitly rejects it. However, get_work_branch_execution_switch_handler only reads the receipt; there is no background switch executor to finish the lost request. Repeated checks therefore never advance this operation, and next-turn admission keeps returning execution_binding_switching (run/lifecycle/mod.rs:9593-9605). The retry endpoint already explicitly supports this crash-after-begin case and completes an existing switching receipt (work_handlers.rs:8089-8202). Please retain read-only checking but also expose a controller-authorized resume/retry action for an interrupted switching operation. Add coverage with a permanently switching receipt whose original handler has exited, rather than a mock that eventually reports success by itself.
There was a problem hiding this comment.
Fixed in 5a1fa2b. A durable switching receipt now has two explicit Web actions: read-only Check device change and controller-authorized Resume device change. Resume reuses the recorded operation through the existing crash-after-begin retry endpoint, while failed operations retain Try device change again. Added Web coverage for a receipt that remains switching across every observation and a services assertion that a stranded switching receipt is retry-authorizable.
d0de354 to
5a1fa2b
Compare
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review of 5a1fa2b: request changes for one remaining P2 issue below. The previous upload-body-limit and stranded switching-operation findings are addressed in this revision.
Reviewed the workspace capture/upload/seal/publication path, recovery-point basis checks and artifact retention, branch deletion cleanup, execution-switch recovery, interaction handling, and Web transcript refresh behavior. The new finding is reproduced by executing the actual transcript merge helpers from this revision under Node: an empty current page merged with a server page containing items 51–100, has_more=true and next_before_item_seq=51 produces has_more=false and a null cursor.
Local validation: live-harness Python tests 12/12; schema inventory tests 47/47; API lifecycle and setup shell contract checks passed. Cargo is unavailable and Web dependencies are not installed here, so Rust/DB and full Web suites were not rerun. GitHub Static Checks and PR Title passed; Test Suite was still in progress when checked. The real TUI-to-Web journey remains unverified locally.
Posted as COMMENT because the authenticated account is also the PR author; the review verdict is request changes.
| const items = [...bySeq.values()].sort((left, right) => left.item_seq - right.item_seq); | ||
| // Once the user has reached the beginning, never reintroduce a cursor from | ||
| // a newer server window; doing so would request duplicate history. | ||
| const nextBefore = current.has_more ? current.next_before_item_seq : null; |
There was a problem hiding this comment.
[P2] Preserve the incoming pagination cursor when the current transcript is empty
An empty current page does not prove that the user has loaded the beginning of the incoming history. For example, open a new Work with an empty transcript, leave the tab hidden while TUI commits more than the 50-item page limit, then return. transcriptWindowsConnect returns true for the empty page, and this line preserves its has_more=false instead of the new server cursor. Executing these helpers with incoming items 51–100 and next_before_item_seq=51 yields has_more=false/null, so the Earlier button disappears and items 1–50 cannot be loaded; overlapping subsequent refreshes keep that state until a full remount. Treat an empty current window as replacement by the incoming page (or otherwise retain its pagination facts), and cover empty-to-paginated refresh in a regression test.
Problem
Durable Work and Session execution needed to remain usable when the user changes surface, edge, or execution owner, and when a run retries after a failed provider turn or history compaction. The old path could lose the active user context, expose incomplete Work state, or make cross-surface recovery depend on a fragile local projection.
What changed
active_turn_frame.v1runtime attachment. It is immutable per human turn, bounded as a preview, retained only for append-only placement, and never rewrites canonical conversation history or strict provider prefixes.mainis retained for required/resolved/queued/replayed callbacks.Validation
make format-check— passed.make check— passed (clippy, compile checks, SDK build/typecheck, Web typecheck).make test-contract— passed.make test-offline— passed: 21,177 base tests, 6,080 e2e-hook runtime tests, SDK typecheck/343 tests/build, Web typecheck/606 tests/build.cargo test -p astra-test-harness --all-targets— passed: 495 + 3 + 11 + 5 tests.prompt_cache_breakwas observed.The opt-in real TUI-to-Web Playwright journey was not run because this environment has no disposable
ASTRA_HARNESS_ACCESS_TOKEN; the harness refuses to read or invent credentials.