Repository navigation
feat(id): compiler-version provenance + journal-recorded trust decisions (#1069) - #4263
Conversation
…ons (#1069) - SourceBuildProvenance gains compiler_version (serde-defaulted for journal compat), populated in compile() from a roc version probe; a failed probe records an empty string instead of failing the compile. - New journal-only JournalEntry::ModuleTrustDecision, patterned on BuilderInstalled: install_wasm_reserved computes the decision at the trust gate (server_compiled / no_trust_list / signature_verified + publisher key) and threads it through install_program_with_trust so the actor persists it alongside the ProgramInstalled event. Replay treats it as audit-only, and it requires no journal version bump (default arm).
|
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 59 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. |
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: 0b03f8216a
ℹ️ 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".
| let compiler_version = std::process::Command::new(&roc_bin) | ||
| .arg("version") | ||
| .output() |
There was a problem hiding this comment.
If the configured compiler or its wrapper hangs while reporting its version, this synchronous output() blocks a Tokio worker indefinitely. The compilation deadline is only created later inside spawn_blocking, so neither the supplied timeout nor the empty-string fallback protects this new subprocess. This can leave a compile request permanently stuck and stall unrelated tasks sharing the worker. Run the probe off the executor with a bounded deadline and bounded captured output, treating a timeout as an unavailable version.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the follow-up commit on this PR. The version probe no longer runs as a synchronous output() on the async path: it now runs inside the compile job's existing spawn_blocking closure via probe_compiler_version, which spawns roc version as a child, drains its stdout through the same tail-bounded reader used for compiler stderr (64 KiB cap), polls try_wait against a 5 s deadline (VERSION_PROBE_TIMEOUT), and kills the child past the deadline. A timeout, spawn failure, or non-zero exit records the empty version, exactly as a failed probe did before — the artifact identity stays pinned by compiler_blake3. New unit tests: a fake compiler that reports a version (recorded), one that hangs (killed at a 200 ms test bound, records empty, returns promptly), and one that exits non-zero (records empty).
| if let Some(decision) = trust { | ||
| return self.persist(JournalEntry::ModuleTrustDecision { | ||
| module_hash: module_hash.clone(), | ||
| basis: decision.basis, | ||
| publisher: decision.publisher, | ||
| }); |
There was a problem hiding this comment.
If this audit append fails after the preceding Committed append succeeds, an ordinary admin install now returns InstallError::Unavailable without replacing the running program, even though its ProgramInstalled event is already durable. On restart, journal replay activates that replacement despite the failed install response, and no complete trust decision accompanies it. Previously ordinary admin installs had no second fallible append between persisting the event and activation. Persist the install and its trust decision as one durable record or atomic batch so an audit-write failure cannot split live and restored state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the follow-up commit on this PR. The trust decision is no longer a second journal append: it is now carried inside the ProgramInstalled event itself (WorldEventKind::ProgramInstalled.trust, #[serde(default, skip_serializing_if = "Option::is_none")], mirroring the existing provenance field), so the install and its trust decision are the single Committed record — one durable append, nothing to split. An audit-write failure is now impossible between persisting the event and activation because there is no second append. The separate JournalEntry::ModuleTrustDecision variant and its replay arm are removed; replay reads the decision from the install event. The decision is still journal-recorded with the module hash (on the event) and the verifying publisher key (in the decision), per code#1069's acceptance criteria.
Codex P2s on PR #4263: - world_compile: the roc version probe ran as a synchronous output() on the async path, unbounded. It now runs inside the compile jobs spawn_blocking closure via probe_compiler_version: child polled against a 5s deadline and killed past it, stdout drained nonblocking into a tail-bounded buffer (no reader thread to join, so a grandchild holding the pipe cannot block it either). Timeout or failure records an empty version, as a failed probe did before. - world install: the trust decision was a second journal append after the Committed install event, so an audit-write failure could split live and restored state. The decision is now carried inside ProgramInstalled itself (serde-default trust field, mirroring provenance), making install + decision one durable record; the separate ModuleTrustDecision journal entry is removed. Tests: 3 new version-probe unit tests (reported version recorded, hung compiler killed at bound, failing probe records empty); world_compile+world_session 83 pass, world::tests+world_store 152 pass, world_fork 4 + world_fork_cli 6 pass, cargo check --all-targets pass.
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Implements the #1069 remainder scoped by task 6097 (queue task 6097, lane C):
Changes
SourceBuildProvenancegainscompiler_version(#[serde(default)]so pre-field journal records still deserialize), populated inworld_compile::compile()by probingroc version; a failed probe records an empty string rather than failing a compile whose identity is already pinned bycompiler_blake3.JournalEntry::ModuleTrustDecisionpatterned onBuilderInstalled.install_wasm_reservedcomputes the decision at the trust gate —server_compiled(gate inapplicable),no_trust_list, orsignature_verifiedwith the publisher key — and threads it through the newinstall_program_with_trusthandle method so the actor persists it alongside theProgramInstalledevent. Replay treats it as audit-only; it takes the default journal-version arm (no version bump, likeBuilderInstalled).Verification (honest)
cargo checkdid not complete within the shift. CI on this PR is the compile/test gate; expect a fix-forward round if the compiler objects (most likely spots: theinstall_wasm_reservedcallers under#[cfg(feature = "sandbox")]and any exhaustiveJournalEntryhandling in tests).Need help on this PR? Tag
@codesmith-botwith what you need.