Repository navigation
id(world): wire manifest apply — module install + schedules over the wire (#807) - #1820
Conversation
apply_steps now applies every step over the wire: InstallModule compiles a manifest source on the host or downloads a known hash and re-installs its bytes under the manifest seed; ScheduleAt/CancelSchedule go through new admin frames (set_schedule/cancel_schedule) that journal before replying, with replace-by-cancel-then-set semantics in the actor. current_world_state reads real live state through a new world_state frame: pending schedules for capability or admin readers, and the install seed only for admin readers — the seed never enters a participant projection (#240). Shell contract gains the three client frames and two reply frames (additive, revision 2). step_is_applied now counts every kind (#818 semantics preserved: only wired steps count). Tests: session-level admin-only/replace/no-op-cancel coverage, protocol round-trips incl. seed hidden from capability readers, compile-source reading, updated applied-count test.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 18 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9e97c7357
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The apply-wiring change added ServerFrame::Schedules and ServerFrame::WorldState and registered both in shell-contract.v1.json, but the exhaustive server_frame_type test helper was not updated, so cargo-check failed with E0004. Add both arms and both SERVER_FRAME_TYPES entries.
|
CI settled on the previous head and I triaged all three failures:
All other checks passed on the previous head (including test-shard-0/1/2/3, cargo-check, fmt). Parking the queue task until CI on 6f9451c lands; merge follows if test-unit passes with only the pre-existing/infra failures remaining. |
|
Resumed by queue task 2661 (lane C). CI classification on head 6f9451c:
Fix applied: merged current main (c58507a) into the branch — clean merge, new head e4b094b; |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4b094bbcf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Not merged: four unresolved P2 findings in this PR's new apply path (queue task 2661)Resumed by queue task 2661 (openchamber scheduled lane). The CI classification from the previous lane still holds — But a Codex review landed on the current head
Not merging until at least (1) and (2) are resolved, since they contradict the acceptance criteria. Parking the queue task (2661) with these as the resume brief. — repo-worker-code (openchamber scheduled lane), queue task 2661 |
…ncels and profile first (#807) Three Codex P2 findings on PR #1820: (4) AdminScheduleAt persisted the ScheduleCancelled journal entry and removed the pending schedule before check_schedule validated the replacement, so a replacement past the horizon destroyed a valid schedule in memory and on journal replay. check_schedule now has a replacement-aware form (the replaced entry is neither a duplicate nor a use of the capacity) and the actor validates with it before persisting anything; the cancel-then-set journal order itself is unchanged (replay refuses two pending sets for one id). New core test covers duplicate/capacity/horizon under replacement. (2) apply_steps ran steps in plan order, which emits ScheduleAt before CancelSchedule, so a manifest replacing ids at MAX_PENDING_SCHEDULES failed the add and skipped the cancel. Cancels now execute before additions. (3) The plan places InstallModule before SetProfile, so a module+profile apply instantiated the guest under the old profile. SetProfile now executes before the remaining steps (a profile applies on the next install).
…#807) Codex P2 finding (1) on PR #1820: the InstallModule { hash } apply path fetched bytes through the participant download_module, which serves only the ACTIVE module, so a manifest naming a different hash could never install it. Add an admin-authorized stored-module retrieval: new download_module_admin client frame (shell contract revision 3) - WorldService::stored_module_bytes reads the blob store every install pins into, by hash, active or not, with the same size and hash re-verification as the participant path; commands/world.rs apply now downloads via the admin token and installs the bytes under the manifest seed as before. A hash the host never installed is refused with a clear message - those bytes cannot be conjured. The participant download is unchanged (active module only, capability-auth). Its streaming tail is shared with the admin session (send_module_bytes), and the client receive path is shared (download_module_with). Test: peers_install_then_download_the_active_module_by_hash extended - after a second install makes the first module non-active, the participant download refuses it, the admin download returns its exact bytes, a wrong admin token is refused, and an unknown hash is refused.
All four P2 findings addressed — new head
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 514b97783b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ixes (#807) CI classification on PR #1820 (run 40853234653): test-unit, crash-gate-sampled and crash-gate-sweep all failed with E0004 - the DownloadModuleAdmin variant added by this PR was handled in the first-frame match (world_session/mod.rs:1252) but not in handle_client_frame for joined participants. It joins its sibling DownloadModule in the already-joined group. cargo-fmt-check diffs in world_net.rs / world_session/mod.rs applied verbatim from the CI log; the meta_client.rs fmt fix (exact CI diff at :53) was already in the worktree uncommitted and is included.
…operation, grants/ceiling before install, native module retrieval Dispositions on PR #1820 (details on the PR threads): (1) schedule replacement validation already landed in actor.rs - validate-before-cancel via check_schedule_replacing; (2) inactive-module retrieval already landed as DownloadModuleAdmin/stored_module_bytes; (3) profile-before-install and (4) cancels-before-additions already landed in apply_steps ordering. Still live and fixed here: (5) the apply sequence reused one stream per operation - admin frames are served one operation per stream, so download->install (and every later step, and the current_world_state reads) wrote to finished sessions; WorldClient::fresh_session now opens a per-operation session on the same connection, used by current_world_state and every apply step. (6) grants/ceiling could still run after a compile in plan order; ordered_steps (extracted, unit-tested) now orders cancels, profile, grants, ceiling, install, schedule additions. (7) stored_module_bytes only searched the blob store; native installs live only in the module directory, which it now falls back to via ModuleDir::load. Tests: ordered_steps order pinned; wire test sequencing install/download/install on fresh sessions over one connection; stored_module_bytes module-dir fallback.
Parked again — CI starved on head
|
apply_steps and its reachability-order test call ordered_steps, but the helper was lost from world.rs, so the lib did not compile (CI clippy-lint E0425 at world.rs:976 plus cascade type errors). Restore it as a stable sort by reachability rank: cancel schedules, set profile, set grants, set ceiling, install module, schedule additions.
basic: nix flake check (cargo-fmt-check) failed on dd872c3 with rustfmt diffs in world.rs (apply_steps call, capability report) and world_session tests; format-only change, no behaviour difference.
|
🤖 Agent note · picked up (resume)
🤖 Agent note · parked
|
|
Agent note — parked: pre-existing clippy-lint break blocks merge
|
|
🤖 Agent note · parked
|
Closes the apply-wiring gap in #807:
id world applyreconciled grants, ceiling and profile over the wire, but module and schedule steps were only reported, anddiff/plancompared schedules and the install seed against fabricated empty state.What changed
apply_stepsapplies every step (pkgs/id/src/commands/world.rs):InstallModulecompiles a manifest source on the host, or downloads a known module hash and re-installs its bytes under the manifest seed;ScheduleAt/CancelSchedulecall the new admin client operations.step_is_appliednow counts every kind — the id(world-manifest): PR #801 — 'id world apply' prints 'applied N step(s)' for steps it explicitly did not apply, and diff/plan run against a fabricated seed=0 / empty-schedules state #818 guard stays explicit per kind so a future reported-only step cannot silently claim credit.world_session):set_schedule/cancel_scheduleframes, admin-only, journaled in the actor before the reply. Re-setting a pending id journals a cancel before the replacement set (journal replay refuses two pending sets for one id). Cancelling an absent id is a no-op.world_stateframe):current_world_statenow reads the pending schedules and the install seed from the server instead ofBTreeMap::new()/0. The seed is returned only to admin readers — a capability-only read (e.g.diff) gets the schedules and a null seed, so the seed never enters a participant projection (Install seed is readable by every joiner #240). Apply always reads with its admin token, so install steps use the real current seed.set_schedule,cancel_schedule,world_state; reply framesschedules,world_state.applyupdated to match.Tests
set_schedule/cancel_schedulereplies carry the full pending map;world_statehides the seed from a capability reader (null) and returns the real seed (4242, from a test install) to the admin reader; no credential is refused.commands::world: applied-count test now asserts every step kind counts; new test for manifest source reading (directory main +.rocsiblings, single file).id-check).Second identical apply is a no-op by construction: plan compares against the now-real live state (module hash, schedules, seed), so a converged world plans zero steps.
Queue task 2661 (from triage task 2247).
Need help on this PR? Tag
@codesmith-botwith what you need.