Conversation
XuPeng-SH
left a comment
There was a problem hiding this comment.
Reviewed commit 50f51a7 from the storage, validation, lifecycle-cleanup, harness UX, and failure-path perspectives. This is my own PR, so I am submitting COMMENT; the two inline P2 findings should be fixed before merging.
The preparing-only boundary is a sound scope choice: persisted caller input does not become a verified restore point, capability assessment does not promise restore/continuation, and branch deletion includes the new records. I am not treating the explicitly deferred ready publisher, content upload, or restore UI as missing functionality in this PR.
Validation actually executed: python3 -m unittest discover -s scripts/schema -p 'test_*.py' — 47 tests, 44 passed, 3 failed, all exposing the missing work_recovery_points inventory registration. The relative executable-path regression is source analysis, not an executed Rust reproduction. This environment has neither cargo nor rustc, so I could not independently run the Rust suites or MatrixOne integration tests. GitHub reported Static Checks successful while Test Suite was still in progress when inspected; the local schema failures remain reproducible despite that green check.
Additional test-quality gap: the new repository unit test checks request-hash inequality, but the two DB integration tests do not exercise identical request replay, changed-payload request reuse, or concurrent replay. Please test these through record_preparing and assert returned identity, conflict behavior, and row count; a hash helper test alone does not verify transactional idempotency. Existing owner-isolation and branch-cleanup integration coverage is useful.
| "work_runtime_event_outbox", | ||
| WORK_RUNTIME_EVENT_OUTBOX_CREATE_SQL, | ||
| ), | ||
| ("work_recovery_points", WORK_RECOVERY_POINTS_CREATE_SQL), |
There was a problem hiding this comment.
[P2] Register the new table in the closed-world schema inventory
Adding this production table without updating scripts/schema/schema_inventory.py::TABLE_METADATA and the expected Work table set in scripts/schema/test_schema_inventory.py breaks the existing schema contract suite. On this exact commit, python3 -m unittest discover -s scripts/schema -p 'test_*.py' runs 47 tests and fails three: test_every_production_table_has_semantic_metadata (one unclassified table), test_storage_and_work_metadata_are_explicit_and_closed_world, and test_work_schema_source_and_exact_table_set; both set failures identify work_recovery_points. Add its semantic ownership/retention/rebuildability metadata and update the expected table set, then rerun the suite.
| command | ||
| .env("NO_PROXY", "localhost,127.0.0.1") | ||
| .env("no_proxy", "localhost,127.0.0.1") | ||
| .current_dir(probe_workspace) |
There was a problem hiding this comment.
[P2] Resolve the executable before changing the probe's working directory
resolve_astra_bin in main.rs returns explicit --astra-bin paths and ASTRA_BIN values unchanged after checking them in the caller's directory. Thus a supported invocation with --astra-bin ./target/debug/astra (or the equivalent ASTRA_BIN) passes binary/server checks, but this new cwd makes Command::new(astra_bin) resolve that relative executable under the empty temporary directory on Unix. Every model then fails with spawn failed: No such file or directory, even though the binary/model is healthy. Canonicalize/absolutize the executable against the original cwd before constructing subprocesses, including the retry/registration paths. Add a public preflight regression using a relative executable and assert that it runs successfully with the disposable workspace as cwd.
|
Addressed the review findings in
The stage remains preparing-only: there is still no ready publisher or restore claim. |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Re-reviewed 731faa2, including the delta from 50f51a7 and the surrounding persistence, manifest validation, cleanup, and preflight paths. This is my own PR, so I am submitting COMMENT.
The schema-inventory finding from review #790 (review) is fixed: I independently reran python3 -m unittest discover -s scripts/schema -p 'test_*.py' and all 47 tests passed. git diff --check e215c032 HEAD also passed. GitHub Test Suite, Static Checks, and PR Title are green for this head.
The previous P2 relative-executable finding remains unresolved. Neither resolve_astra_bin nor the preflight subprocess setup changed: --astra-bin ./target/debug/astra / relative ASTRA_BIN is still returned unchanged and passed to a command whose cwd is the new temporary workspace. Please resolve the executable against the original cwd before changing subprocess cwd. I am retaining the original finding rather than opening a duplicate inline thread. This is source-confirmed; I have not claimed an executed Rust reproduction.
The new repository test now meaningfully covers identical replay, changed-payload request reuse, and simultaneous replays of an existing record. One remaining coverage distinction: both concurrent calls happen after the row has already been inserted, so they do not exercise concurrent first admission. Add a separate test with a fresh request/recovery-point identity, start both record_preparing calls before any insert, and assert the intended results plus a single row by request identity. This is a test gap, not proof that the transaction implementation fails.
I also inspected the workspace portability boundary; an additional non-blocking validation concern is inline. The preparing-only scope remains appropriate and does not yet promise a usable restore point.
Validation limits: cargo/rustc are unavailable here, so I did not independently run Rust or MatrixOne tests. I ran a small temporary-directory filesystem experiment for the symlink-resolution example in the inline comment; that verifies OS path resolution, not execution of the Rust validator or an Astra restore flow.
| return false; | ||
| } | ||
| let mut components = path.split('/').collect::<Vec<_>>(); | ||
| components.pop(); |
There was a problem hiding this comment.
Validation boundary to address before ready publication/materialization (non-blocking for this preparing-only scope): this checks target components lexically and does not resolve other snapshot symlinks. For a snapshot with a -> . and b -> a/../outside, both targets pass this helper by inspection, but following b resolves to the workspace's parent/outside because a resolves to the workspace root before .. is applied. I confirmed the latter with real symlinks in a temporary directory. Please add a manifest-level chained-symlink regression and either validate the complete link graph with bounded cycle detection or explicitly document this as lexical validation while requiring containment enforcement at the materializer. The current helper must not be treated as proof that all links stay inside the workspace. There is no restore path in this PR, so this is not a claim of a currently exploitable restore vulnerability.
731faa2 to
505de37
Compare
Summary
Make one durable Work usable across TUI, Web, Server, and Edge without silently resuming or forking a Session.
/workopens an owner-scoped catalog and asks the user to Observe Work or Continue this Work. Observe is read-only; Continue is an explicit Work-scoped turn and never mutates the current chat Session.cli,tui,web,app,server, oredge). The SDK, Server, TUI, Web, and tests use the same strict contract.text_donereplaces a provisional streamed prefix so replayed answers are complete and never duplicated. Final outcomes are reliably delivered even when progress queues are full; late progress cannot overwrite a terminal state./work saveremains an honest logical checkpoint boundary for Work/branch/session facts. It does not claim to snapshot code, data, credentials, artifacts, pending effects, or an active Run.User journey
Architecture and limits
The Server owns Work, branch, Session, Run, controller, and provider-binding facts. Surface clients are projections and explicit command issuers; they do not grow separate lifecycle or checkpoint state machines. Edge replacement changes future provider selection, not an in-flight Server-owned agent loop.
Workspace snapshots/materialization, MatrixOne data portability, artifact location transfer, active Run recovery after Server loss, and inline TUI approval/prompt interaction require separate contracts. The current release reports these boundaries instead of implying that a logical recovery point can restore them.
Verification
cargo fmt --all,git diff --check, targetedcargo check, andcargo clippy -p astra-cli --all-targets -- -D warningspassed.cargo test -p astra-cli --lib: 5,053 passed, 9 ignored.text_donereplay, partial output, and cleanup failure.cargo test -p astra-thin-client --lib: 186 passed;cargo test -p astra-server-types --lib: 167 passed.ASTRA_TEST_DB_IT=1).Compatibility
The current contract intentionally requires the explicit surface field and does not retain old request shapes or compatibility aliases.