ci: audit the dashvm engine dependency set and document the equivalence hotfix rule - #4939
DCG-Claude wants to merge 8 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: dashpay/platform/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-09-29T16:32:41.429Z |
|
|
github-actions
Bot
added
the
waiting-bots
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
Verified the supplied findings against head d4bdf40 and confirmed one blocking security issue and one documentation correction. The new PR-triggered audit exposes shell interpolation of attacker-controlled branch names in the downstream status workflow; a local reproduction with a dummy token confirmed command execution. All 31 checker self-tests and the range-wide whitespace check passed; no live advisory audit was performed.
🔴 1 blocking | 🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
normalbygpt-6-astra(effort low) — The PR adds substantial CI audit logic with exception validation, subprocess handling, index checks, and self-tests, but changes no consensus, funds, cryptography, network deserialization, or storage migration behavior. - Phase 1 reviewers: not run (skipped for throughput: 15 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `.github/workflows/security-audit-status.yml`:
- [BLOCKING] .github/workflows/security-audit-status.yml:7: Sanitize branch names before subscribing to PR-triggered audit runs
Subscribing to the engine audit makes this downstream workflow reachable from fork pull requests; the previously subscribed audits only run on schedules or manual dispatch. Line 24 inserts `${{ github.event.workflow_run.head_branch }}` directly into shell source. A valid Git branch name such as `audit-$(printenv${IFS}GH_TOKEN>&2)` therefore executes command substitution inside the quoted URL. I reproduced this locally using a dummy token. Once this workflow is present on the default branch, an attacker can trigger the new audit with a manifest-changing PR and execute commands in the downstream workflow, which receives its own repository token rather than inheriting the fork job's restricted permissions. Pass the branch through an environment variable, expand it as quoted data, and supply it as a URL-encoded API parameter. Explicitly restrict the aggregator token to the permissions required to read workflow results; its effective permissions currently depend on repository defaults.
In `book/src/architecture/engine-dependencies-and-hotfixes.md`:
- [SUGGESTION] book/src/architecture/engine-dependencies-and-hotfixes.md:82-86: Describe parser/engine version alignment as planned rather than implemented
The chapter says the validation crate already pins the wasm-tools line and that parser and engine agree down to the patch level, but this head does not implement that guarantee. No package manifest declares wasmparser, wasm-encoder, or Wasmtime dependencies; the root engine metadata lists names only, and the wasm-tools 0.244.0 entries in Cargo.lock are transitive dependencies rather than a validation-crate exact pin. Describe exact parser/engine alignment as a requirement for the upcoming validation/runtime crates, not an established property. This preserves the chapter's distinction between existing mechanisms and future work.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 2 only (queue backlog)
The final head cleanly implements the dedicated DashVM dependency audit and hardens the workflow aggregator against pull-request branch injection. The two prior findings are fixed and verified in the current source; the remaining engine-set activation concern is intentional pre-runtime behavior and is not actionable within this PR.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
normalbygpt-6-astra(effort low) — The diff adds substantial CI auditing logic, workflow integration, metadata validation, and documentation, but it does not itself modify consensus execution, funds movement, cryptography, networking deserialization, or storage migrations. - Phase 1 reviewers: not run (skipped for throughput: 18 PRs queued, above the 10 limit)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort high); agentphase2-reviewer,gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
|
/self-reviewed 5c823e0 |
|
@coderabbitai review No review for |
github-actions
Bot
added
ready-for-human
… workspace ignore list The engine (Wasmtime, Cranelift and the wasm-tools crates that validate and encode modules) decides consensus results, so an advisory against one of its crates cannot be one line in the base-wide nightly audit that is red on other crates and honours the workspace ignore list. The root manifest gains [workspace.metadata.dashvm.engine] naming the engine crates, with commented shapes for scoped acknowledgements and declared temporary patches. check-engine-advisories.py runs cargo-audit from a fresh directory so .cargo/audit.toml is not consulted, audits every resolved version of an engine crate, reports crates the lockfile does not contain as planned, and fails on vulnerabilities, unmaintained, unsound or yanked engine crates, expired or over-long exceptions (90 days, provisional) and undeclared [patch] entries. --self-test proves each exit code in memory; --report evaluates a saved report offline. Cargo.lock is unchanged. Refs #4683 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…changes Security: DashVM Engine installs cargo-audit 0.22.2 (cached between runs), exercises the checker's self-test so the failure path is proven before the real audit, then audits the engine set and appends the report to the step summary. It runs nightly after the base-wide audit, on dispatch, and on pull requests that change the lockfile, a manifest, the audit configuration, the checker or the workflow. The Security Status aggregator and DEV_STATUS.md list it; .cargo/audit.toml says its ignore list does not cover the engine. Refs #4683 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The book gains an Architecture chapter that states the DashVM engine policy (pinned upstream Wasmtime with Cranelift, a small integration layer, fixes contributed upstream, temporary patches as declared and expiring exceptions), names the engine crate set and its advisory sources, explains what the engine audit automates and what stays operational, classifies advisories by reachability under the pinned profile and by consequence with the April 2026 Wasmtime batch as worked examples, and states the execution/fee-equivalence hotfix rule: an engine change ships without a protocol version only with equivalence evidence over accepted code and recorded executions on both architectures, otherwise it is a new engine profile through the normal upgrade path. A runbook and a task map close the chapter. No audit report, security owner or stage audit gates building. Refs #4683 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… the report on failure Review round 1. A [patch] entry may rename the crate it patches, so the checker resolves each entry through its package field before requiring a declaration. cargo-audit 0.22.2 keeps its exit code and prints a valid JSON report when it could not update or open the crates.io index or a yank lookup failed, and in JSON mode it says nothing on stderr about it; the checker now surfaces stderr, treats those diagnostics as an incomplete run (exit 2), and reads the yank status of every resolved engine crate from the sparse crates.io index itself, failing on an unreadable index rather than passing. Self-tests cover the renamed patch, the subprocess diagnostics and the index read through a file:// index. The audit step guards the checker against the runner shell's errexit so its report is printed and summarised when it fails. The chapter now applies the equivalence rule to every engine change: advisory reachability decides how the vulnerability is handled, not whether the replacement release needs the evidence. Refs #4683 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…s the branch as data Subscribing the aggregator to the engine audit, which runs on pull requests, made its workflow_run job reachable from fork branches while it expanded the head branch name inside shell source with the repository token in scope. The job now skips runs triggered by pull requests (there is nothing to aggregate for a pull request branch), holds only actions: read, and passes the branch through an environment variable to gh api as a URL-encoded parameter. The chapter no longer claims the validation crate already pins the wasm-tools line; that crate is not in this tree. The exact pin is stated as a requirement on the upcoming validation and runtime crates, and the lockfile's current wasm-tools entries are described as transitive. Refs #4683 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
5c823e0 to
9a6f66b
Compare
github-actions
Bot
added
waiting-bots
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 2 only (queue backlog)
Verified the complete diff at 9a6f66b and confirmed that both prior findings are fixed. One non-blocking issue remains: the status aggregator can substitute a pull-request result for the upstream engine audit result. All 31 checker self-tests, Python syntax validation, diff whitespace checks, and local aggregator fixtures passed; live advisory retrieval and GitHub-hosted execution were not exercised.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
normalbygpt-6-astra(effort low) — The substantial Python advisory checker and CI workflow changes involve nontrivial audit validation and exception handling, but the diff changes tooling and documentation rather than consensus, funds, cryptography, network deserialization, or storage migrations. - Phase 1 reviewers: not run (skipped for throughput: 13 PRs queued, above the 10 limit)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort high); agentphase2-reviewer,gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `.github/workflows/security-audit-status.yml`:
- [SUGGESTION] .github/workflows/security-audit-status.yml:37-39: Exclude pull-request runs when selecting audit conclusions
The job-level guard excludes pull requests from triggering aggregation, but this query still selects the latest run of any event type with the matching head-branch name. The newly subscribed engine workflow runs on fork PRs, so a newer successful PR run from a branch named `v5.0-dev` can replace a failed upstream scheduled engine audit in the next aggregation triggered by another scheduled or dispatched audit. A fork author controls that branch name and the checker executed by their PR. I reproduced the step reporting `All security audits passing` with a local API fixture containing a newer successful same-branch PR run and an older failed scheduled engine run. Restrict candidate runs to `schedule` and `workflow_dispatch` before choosing the newest result, using event-filtered requests or sufficient pagination rather than fetching one run and rejecting it afterward. This affects the monitoring signal; it does not restore shell injection or change the underlying engine audit's conclusion.
The runs API lists a fork pull request run under the fork's branch name, and the engine audit now runs on pull requests, so the newest run on a branch could be a pull request result standing in for a failed scheduled audit. The aggregator reads the newest scheduled run and the newest dispatched run of each workflow through the event filter and keeps the newer of the two. Refs #4683 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The pull-request audit executes the checker and workflow definition from the pull request's merge commit, allowing a dependency-changing PR to modify the audit logic and report a false-clean result. The date parser also mishandles valid TOML datetime values by passing datetime objects into date arithmetic, producing an uncaught traceback instead of a clean audit failure. The three prior findings were independently verified as fixed.
🔴 1 blocking | 🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
normalbygpt-6-astra(effort low) — The substantial Python advisory checker and CI integration introduce nontrivial tooling behavior and failure handling, but the diff does not change consensus, funds movement, cryptography, peer-facing deserialization, or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— architecture-layering (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— security-auditor (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 13% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `.github/workflows/security-audit-engine.yml`:
- [BLOCKING] .github/workflows/security-audit-engine.yml:49-75: Run the pull-request audit with trusted checker code
This workflow runs on pull_request events and checks out the pull-request merge commit before executing the checker and its self-tests. A fork contributor can change Cargo.lock or a manifest and modify the checker, the workflow, or both in the same pull request so that the job exits successfully without auditing the submitted dependency set. The resulting check cannot serve as a reliable security gate, especially if it is made required later. Run the audit logic from a trusted base-branch revision while treating only the pull request's manifest and lockfile as untrusted input, or use another protected workflow design that prevents pull-request-controlled audit code from executing.
In `.github/scripts/check-engine-advisories.py`:
- [SUGGESTION] .github/scripts/check-engine-advisories.py:285-294: Normalize TOML datetimes before checking expiry
tomllib returns datetime.datetime for a bare TOML datetime such as expires = 2026-12-31T00:00:00. Because datetime.datetime is a subclass of datetime.date, parse_date returns it unchanged; comparing it with today or subtracting today then raises TypeError. The checker therefore emits an uncaught traceback instead of its intended clean invalid-date failure, although it still fails closed.
…l date literals A pull_request job checks out the pull request's merge commit, so a pull request that changed the lockfile could also change the checker or the workflow and report a clean audit of nothing. The engine audit now runs on pull_request_target: the workflow and the checker come from the base branch, the pull request's root manifest and lockfile are fetched by ref as data files, and the checker's new --base-manifest option fails a submitted engine set smaller than the base branch's. Nothing from the pull request is executed and the token keeps contents: read. The concurrency group keys pull request runs on their number, since github.ref is the base branch under pull_request_target, and the status aggregator admits scheduled and dispatched runs by name. tomllib returns datetime.datetime for a TOML datetime literal; parse_date now takes its day instead of passing it into date arithmetic, and the self-tests cover a datetime literal, a passed date literal and a non-date value. Refs #4683 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the complete diff at 959058b. All five prior findings are fixed, but the normative hotfix rule still permits consensus-visible changes based only on historical and corpus comparisons; the PR audit also fetches a mutable head ref rather than the triggering revision. All 36 checker self-tests, local aggregation fixtures, and diff whitespace checks passed; live advisory fetching and GitHub-hosted execution were not rerun, and the worktree remains unchanged.
🔴 1 blocking | 🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
normalbygpt-6-astra(effort low) — The diff adds substantial advisory-checking logic, exception validation, and security-audit workflow orchestration, but changes only tooling, CI, metadata, and documentation rather than consensus or any other qualifying critical surface. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— architecture-layering (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— security-auditor (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 13% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort high); agentphase2-reviewer,gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `book/src/architecture/engine-dependencies-and-hotfixes.md`:
- [BLOCKING] book/src/architecture/engine-dependencies-and-hotfixes.md:257-261: Keep consensus behavior changes under protocol dispatch, not replay coverage
Matching recorded executions and corpus vectors establishes replay compatibility, not equivalence for future executions under the same protocol. An accepted contract can contain an input-dependent branch affected by a compiler bug that neither history nor the corpus exercises. A replacement engine can satisfy all five checks yet return a different result when that branch is first invoked during a rolling node upgrade. The worked example at lines 195–204 explicitly calls the replacement a hotfix when no committed execution depended on the bug, so the opening requirement that changes be invisible to guests does not resolve this contradiction. Require a separate justification that a software-only hotfix cannot change consensus for any input admitted by the existing profile, consistent with the shipped-generation rule in coding-conventions.md. Historical and corpus comparisons remain necessary evidence, but known reachable semantic, fee, admission, or resource-outcome changes require a new protocol-selected engine profile even when history replays identically. Update the worked example and runbook accordingly.
In `.github/workflows/security-audit-engine.yml`:
- [SUGGESTION] .github/workflows/security-audit-engine.yml:71-74: Fetch the triggering event's immutable PR head SHA
The audit reads the current refs/pull/<number>/head rather than github.event.pull_request.head.sha. Rerunning an older workflow after another push therefore audits the newer manifest and lockfile while retaining the older event context; a queued run can encounter the same mismatch if the ref advances before this fetch. Printing FETCH_HEAD exposes the discrepancy but does not prevent it, and concurrency cancellation does not prevent manually rerunning an older event. Pass the event's head SHA through an environment variable and fetch/read that exact commit, preserving the separation between trusted checker code and submitted data.
…he event's head sha Matching every recorded execution and corpus vector shows that history replays identically, not that the next execution will: an accepted module can hold an input-dependent path that neither history nor the corpus has taken, and a compiler change on that path would make nodes on two builds disagree under one protocol version. The rule therefore gains a sixth required condition, a recorded argument that the change cannot alter a consensus-visible outcome for any admitted input under the pinned profile, in line with the shipped-generation rule of the coding conventions. A guest-reachable, guest-visible fix is a new engine profile through a protocol upgrade even when history replays identically; the aarch64 worked example, the committed-wrong-result and pre-activation paragraphs and the runbook say so. The pull request audit fetches the two submitted files at the event's immutable head commit instead of the moving pull request ref, so a rerun or a queued run audits the revision the event names. Refs #4683 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
⚠️ DEGRADED — Re-review — Final validation — Phase 2 only (queue backlog)
⚠️ DEGRADED review. The primary review models were unavailable (gpt-6-astraunavailable: Request rejected (429) · All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The us), so this review ran on stand-in models:gpt-5.6-luna→muse-spark-1.3-contributor,gpt-5.6-sol→muse-spark-1.3-contributor,gpt-5.6-terra→muse-spark-1.3-contributor,gpt-6-astra→muse-spark-1.3-contributor. Both review phases and the independent verifiers still ran, but on weaker models, with Phase 1 capped athigheffort. Treat the verdict as provisional; a full-strength re-review will run on the next push once the primary models are back.
CI-only PR adding a DashVM engine advisory audit and the equivalence hotfix rule as docs and workflows, with no Rust or consensus code changes. Verified all workflows, the checker including its 36-case self-test, and the book chapter at the exact head. All seven prior threads are fixed and no new defects were found.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: security-auditor); reviewer 4: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: general); reviewer 5: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: architecture-layering); reviewer 6: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: security-auditor); final verifier: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: astra-verifier, role: final-verifier)
- Degraded mode:
gpt-6-astraunavailable: Request rejected (429) · All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The us (detected by lane, since 2026-09-29T17:11:05Z); stand-insgpt-5.6-luna→muse-spark-1.3-contributor,gpt-5.6-sol→muse-spark-1.3-contributor,gpt-5.6-terra→muse-spark-1.3-contributor,gpt-6-astra→muse-spark-1.3-contributor; Phase 1 effort capped athigh - Triage:
normalbymuse-spark-1.3-contributor(standing in forgpt-6-astra) (effort low) — The diff adds substantial advisory-checking logic, CI workflows, metadata, and documentation, but does not itself change consensus rules or any other qualifying critical surface. - Phase 1 reviewers: not run (skipped for throughput: 15 PRs queued, above the 10 limit)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
muse-spark-1.3-contributor(standing in forgpt-6-astra) — final-verifier; agentastra-verifier - Phase 2 reviewers:
muse-spark-1.3-contributor(standing in forgpt-6-astra) — general (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — architecture-layering (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — security-auditor (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — general (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — architecture-layering (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — security-auditor (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
|
/self-reviewed 75b62e6 |
|
@coderabbitai review No review for |
github-actions
Bot
added
bot-review-skipped
github-actions
Bot
removed
the
bot-review-skipped
|
Friendly nudge: this PR has been green and bot-approved for 5 days and awaits a human review. 🤖 Posted autonomously by DashVM (Claude) on behalf of pasta. |
|
Ready for review — needs QuantumExplorer or ktechmidas or shumkov. |
github-actions
Bot
added
ready-for-human
This branch has not been deployed
2 participants
Issue being fixed or feature implemented
Part of the smart-contract plan in #4626, task R08-10 (section 8, workstream #4683): track security fixes and advisories for the selected engine dependencies and apply the execution/fee-equivalence hotfix rule.
The WebAssembly engine (Wasmtime with Cranelift, and the wasm-tools crates that validate and encode modules) decides consensus results, so an advisory against one of its crates is a signal that a node release or a protocol upgrade is due. Today that signal would be one line in the base-wide nightly Rust audit, which is red on unrelated crates and honours the workspace ignore list in
.cargo/audit.toml. The rule that decides whether an engine fix may ship as a software-only hotfix or must be a protocol upgrade existed only in the planning issue.Owner decision Q01 binds this work: pinned upstream Wasmtime with Cranelift and a small Dash integration layer, fixes contributed upstream, a temporary patch as an exception rather than a standing engine fork. This PR encodes "exception" as a declared, justified, dated entry that CI fails once it expires.
What was done?
Cargo.toml: new[workspace.metadata.dashvm.engine]table naming the engine crate set (wasmtime,wasmtime-environ,wasmtime-internal-cranelift,cranelift-codegen,cranelift-frontend,cranelift-native,wasmparser,wasm-encoder) with commented shapes for scopedacknowledgedentries (advisory id, reason, expiry) and declared temporarypatches(crate, upstream fix, expiry). Cargo ignores workspace metadata for resolution;Cargo.lockis byte-identical..github/scripts/check-engine-advisories.py(stdlib Python, runs on the 3.12 of the ubuntu-24.04 runners): runscargo audit --json --file <lockfile>from a fresh temporary directory so.cargo/audit.tomlis not consulted, audits every resolved version of an engine crate, reports crates the lockfile does not resolve yet as planned, and fails on vulnerabilities against engine crates, unmaintained, unsound or yanked engine crates, expired or over-long exceptions, exceptions without a reason or an upstream fix, and[patch.*]entries for engine crates without a declaration. Renamed[patch]entries are resolved to the crate they patch through theirpackagefield. cargo-audit's stderr is surfaced and a run whose advisory database, index update or yank lookup failed is an error, not a pass; because JSON mode silences those diagnostics, the checker also reads the yank status of every resolved engine crate from the sparse crates.io index directly (--index-url,--offline). Exit codes 0 clean, 1 findings, 2 audit could not run or was incomplete.--base-manifestholds a submitted engine set to at least the trusted branch's set; TOML date and datetime literals are accepted as expiry values.--self-testevaluates 36 cases (in-memory manifests and reports, a fakecargosubprocess, afile://index) and checks each exit code and message;--report <json>evaluates a saved report offline;--todaypins the evaluation date..github/workflows/security-audit-engine.yml(Security: DashVM Engine): nightly at 23:45 UTC after the base-wide audit, on dispatch, and (throughpull_request_target, so the base branch's workflow and checker run and the pull request'sCargo.tomlandCargo.lockare fetched as data files and audited with--base-manifestagainst the base branch's engine set) on pull requests tomasterandv*-devthat touchCargo.lock,Cargo.toml,packages/**/Cargo.toml,.cargo/audit.toml, the checker or the workflow. Installs cargo-audit 0.22.2 with the tool's lockfile (cached between runs), runs the self-test before the real audit, and appends the report to the step summary; the checker is guarded against the runner shell's errexit so the report is shown and summarised when it fails..github/workflows/security-audit-status.ymlandDEV_STATUS.md: the aggregator and the status page list the new workflow; the status page says the engine set is audited separately and on lockfile-changing pull requests. Because the engine audit runs on pull requests, the aggregator (aworkflow_runjob with the repository token) now skips runs triggered by pull requests, is restricted toactions: read, and passes the head branch togh apias a URL-encoded parameter through an environment variable instead of expanding it in shell source. It also selects only scheduled and dispatched runs (newest of the two) when reading each audit's conclusion, so a pull request run listed under the same branch name cannot stand in for the scheduled result..cargo/audit.toml: a comment saying the ignore list does not apply to the engine set and where engine exceptions go.book/src/architecture/engine-dependencies-and-hotfixes.mdwith itsSUMMARY.mdentry under Architecture: why the engine is a consensus dependency and how theengine_profilenumber in the DashVM table versions it; the engine policy; the dependency table with roles, pin owners and advisory sources; how advisories are tracked and what stays operational; the two-axis classification (reachability under the pinned profile, consequence) with the April 2026 Wasmtime batch as worked examples; the normative execution/fee-equivalence hotfix rule; a runbook; a task map from each evidence source to the task that produces it. The chapter states that no external audit report, named security owner or closed stage audit is a prerequisite to building.No Rust source, version table, fee, limit, proto or SDK surface changes. Everything under
packages/is byte-identical.Overlap note for sibling lanes: R11-07 documents the hotfix-versus-upgrade rule from the recovery side and R03-05 tracks advisories for host-capability dependencies. This chapter is the single normative statement of the equivalence criterion and the single tracking mechanism; those lanes should link here rather than restate.
How Has This Been Tested?
Local, macOS arm64, Python 3.14.7, cargo-audit 0.22.2 installed into a private root, mdBook 0.4.40 and actionlint 1.7.7 from their release tarballs. Every command's output went to a file and the exit code was captured.
python3 .github/scripts/check-engine-advisories.py --self-testself-test: 36 of 36 checks passedpython3 .github/scripts/check-engine-advisories.py(cargo-audit on PATH)wasmparserandwasm-encoder0.244.0 audited clean, six crates planned; 0 failurespython3 .github/scripts/check-engine-advisories.py --index-url https://index.crates.io/does-not-existcannot read the crates.io index entry for wasmparser: an unreadable index is an error, not a passbash -eo pipefailwith a failing checkercaptured status=1, exit 1 (the guarded command survives errexit)python3 .github/scripts/check-engine-advisories.py --report /tmp/audit.json[patch.crates-io] wasmtimeFAILlineswasmparserFAIL unmaintained engine crate wasmparser 0.244.0: RUSTSEC-2025-0141, proving the workspace ignore list is bypassedactionlint .github/workflows/security-audit-engine.yml .github/workflows/security-audit-status.ymlmdbook build book -d /tmp/book-outcargo metadata --format-version 1 --locked --no-depsmetadata.dashvm.engineshasum Cargo.lockbefore and after82ac131eb4bb61a59794b275eda6bfa549cdd281both timescargo fmt --all --check,git diff --checkcargo check --workspace --all-targetsgrepfor U+2014 in the chapter and in every added lineNot run: clippy and the verify-only cut, since no Rust crate is touched. CI evidence for the workflow: the
pull_requestruns on the earlier heads (latest 5e2d2c4) passed with the self-test and a clean live audit in the log. Since the switch topull_request_targetthe workflow definition is read from the base branch, where it does not exist yet, so this PR cannot exercise its own final workflow on CI; thepull_request_targetpath was rehearsed locally in a fixture repository (hostile pull request with a gutted checker and a dropped engine crate fails under the base checker through the real cargo-audit; a benign pull request that grows the set passes). After merge the first scheduled run, or aworkflow_dispatchonv5.0-dev, exercises it live.Breaking Changes
None. No consensus, wire, storage or API change. The only runtime effect is a new CI workflow and an aggregator entry.
Decisions taken (provisional values)
MAX_EXCEPTION_DAYSin the checker). Provisional: the issue register has no value for it. Chosen because Wasmtime patch releases on a supported line have shipped at least monthly in 2026, so a quarter is long enough to bump and short enough to be noticed.unmaintained,unsoundandyankedon an engine crate are policy failures under Q01, not advisories to defer.noticeis printed only.Review round 1
[patch]entries (wasmtime_old = { package = "wasmtime", ... }) are resolved to the patched crate; self-test added.thepastaclaw review (head d4bdf40)
workflow_runjob reachable from forks while it expandedhead_branchin shell source. Fixed by skipping pull-request-triggered runs, restricting the token toactions: read, and passing the branch through an environment variable as agh api -fparameter; verified with a hostile branch name against a fakegh.wasm-toolspin as a requirement on the upcoming validation and runtime crates and says today's lockfile entries are transitive.thepastaclaw re-review (head 9a6f66b)
event=scheduleandevent=workflow_dispatchand takes the newer of the two; verified with a fixture holding a newer successful pull request run and an older failed scheduled run (reported as failure) and with the real runs API on the default branch.thepastaclaw re-review (head 5e2d2c4)
pull_request_target, so the workflow and checker come from the base branch; the pull request's root manifest and lockfile are fetched by ref as data files and audited with the base branch's checker, with--base-manifestfailing any submitted set smaller than the base's. The token stays atcontents: readand nothing from the pull request is executed. The status aggregator's guard now admits only scheduled and dispatched runs by name.parse_datenormalisesdatetime.datetimeto its day, and self-tests cover a datetime literal, a passed date literal and a non-date value (clean failure, no traceback).Checklist:
For repository code-owners and collaborators only
Refs #4683
Dash-Tasks: R08-10
🤖 Generated with Claude Code
PR Hygiene ·
75b62e6.cargo/audit.toml,Cargo.toml,DEV_STATUS.mdand 2 more) — QuantumExplorer or shumkovgithub(.github/scripts/check-engine-advisories.py,.github/workflows/security-audit-engine.yml,.github/workflows/security-audit-status.yml) — ktechmidas or shumkovWhen every box is checked the
PR Hygienecheck passes and this can merge.