refactor: state machine runtime + recoverability - #145
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 356be8f414
ℹ️ 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".
| self.resource_key_ref.canonical_value()?, | ||
| ), | ||
| ( | ||
| "typed_allocation_state_ref".to_owned(), | ||
| self.typed_allocation_state_ref.canonical_value()?, |
There was a problem hiding this comment.
When a valid TypedResourcePolicy produces the same allocation-state and fencing refs for two later effects on one resource, both resource_advanced records hash to the same ResourceLedgerRecordRef because this canonical value omits both predecessor and effect_key. Consequently, resource_head() can remain unchanged across an append and a stale expected_resource_head can pass try_bind_and_allocate's CAS check, defeating the append-only resource coordination contract; include the predecessor and effect identity in the content-addressed record schema.
AGENTS.md reference: AGENTS.md:L53-L55
Useful? React with 👍 / 👎.
| ) | ||
| })?; | ||
| return Ok(vec![canonical_json(field)?, canonical_json(value)?]); |
There was a problem hiding this comment.
For arrays whose schema ordering is field:emission_ordinal, field:output_ordinal, or field:query_ordinal, this converts the integer field to canonical JSON text and later compares the resulting Strings lexicographically. A valid ascending sequence containing ordinal 10 is therefore rejected because "9" > "10", while an incorrectly ordered sequence such as 1, 10, 2 is accepted; this breaks settlements with at least 11 outputs or fact emissions and fact selections with at least 11 queries, so bounded integer fields must be compared numerically.
AGENTS.md reference: AGENTS.md:L55-L57
Useful? React with 👍 / 👎.
| Value::Array(values) => { | ||
| for (index, value) in values.iter().enumerate() { | ||
| validate_native_canonical_value(value, &format!("{path}[{index}]"), depth + 1)?; | ||
| } | ||
| Ok(()) |
There was a problem hiding this comment.
When mfm.primitive-canonical_value.v1 contains an unwrapped native array, this recursive branch never enforces the annex's max_array_items limit of 1,048,576 (and the object branch likewise omits max_object_entries). An array with 1,048,577 small values remains well below the 16 MiB byte limit and is accepted and hashed despite being outside the frozen contract, allowing oversized hostile collections and their allocation cost through strict_decode; check the item/entry bounds before recursing.
Useful? React with 👍 / 👎.
| let algorithm = parts.next(); | ||
| let prefix = parts.next(); | ||
| digest.is_some_and(is_lower_hex_64) | ||
| && algorithm == Some("sha256-jcs-v1") | ||
| && prefix.is_some_and(|prefix| !prefix.is_empty()) |
There was a problem hiding this comment.
For mfm.primitive-semantic_identity.v1, which is used for semantic_type_id fields, this accepts any nonempty prefix followed by sha256-jcs-v1 and a digest. For example, x:sha256-jcs-v1:<64 hex> passes the annex codec even though mfm_ids::SemanticTypeId::parse rejects it because it lacks the required category, namespace, name, and version components; use the typed identity parser so persisted values accepted by the codec remain consumable by the public typed API.
AGENTS.md reference: AGENTS.md:L81-L82
Useful? React with 👍 / 👎.
| "sha256-v1" => Ok(Self::Sha256V1), | ||
| "sha256-jcs-v1" => Ok(Self::Sha256JcsV1), |
There was a problem hiding this comment.
Once sha256-v1 is accepted by the shared DigestAlgorithm parser, every generic Identity<K> parser also accepts it because parse_identity performs no category-specific algorithm check. Persisted values such as run:sha256-v1:<digest>, node:sha256-v1:<digest>, and schema or semantic-type identities using the raw-content algorithm therefore become valid typed IDs even though the frozen identity encodings reserve sha256-v1 for ContentDigest; restrict this variant to content-digest parsing so raw-content and JCS semantic identities cannot be substituted.
AGENTS.md reference: AGENTS.md:L55-L56
Useful? React with 👍 / 👎.
| match (view.allocation(), policy_binding) { | ||
| (None, None) => {} | ||
| (Some((_, evidence)), Some(candidate)) if evidence.policy_binding() == candidate => {} | ||
| _ => return Err(ExecutorError::ResourcePolicyNotRevalidated), |
There was a problem hiding this comment.
After restoring an effect with a resource allocation, a caller can mint TargetEntryAuthority merely by supplying a ResourcePolicyBinding equal to the retained pair; this branch neither runs TypedResourcePolicy::validate_history nor requires authority produced by try_bind_and_allocate. A syntactically valid restored snapshot whose allocation state violates the selected account-sequence, inventory, or fencing policy can therefore reach the external target without the restart-time typed revalidation required by the design; require an affine policy-validation result rather than accepting the reference pair alone.
AGENTS.md reference: AGENTS.md:L42-L43
Useful? React with 👍 / 👎.
|
@codex review this |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef80b07592
ℹ️ 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".
| create_new_atomic(first_path, first_bytes)?; | ||
| create_new_atomic(second_path, second_bytes) |
There was a problem hiding this comment.
If another process creates second_path after the preflight checks, or writing that path otherwise fails, the export has already been installed at first_path and this function returns an error without removing it. The CLI then reports a failed export, but retrying with the same paths is rejected because the first path exists, leaving a bundle without its required ContentRef sidecar until the user manually cleans it up. Stage both outputs before exposing either, or remove the first output when publication of the second definitively fails.
Useful? React with 👍 / 👎.
| let body: Value = serde_json::from_slice(bytes) | ||
| .map_err(|_| crate::wallet_rpc::EvmWalletRpcFailure::InvalidResponse)?; |
There was a problem hiding this comment.
When a provider returns duplicate JSON object keys, deserializing directly into serde_json::Value silently keeps one value instead of rejecting the ambiguous response. This also defeats the later exact-shape check: for example, duplicate code or message members collapse to an error object of length two and can incorrectly satisfy the exact already known classifier, consuming the transaction convergence path under a response that did not actually have the certified shape. Use a duplicate-rejecting deserializer at the raw response boundary before classifying or retaining any wallet result.
Useful? React with 👍 / 👎.
| let (identity, signature) = | ||
| tokio::task::spawn_blocking(move || blocking_signer.sign(digest)) |
There was a problem hiding this comment.
When Tokio's blocking pool is saturated, this queues the keystore work only after the generation guard has completed, so an arbitrarily long delay can separate the fence verdict from the first key-file access. If the generation is fenced during that delay, the queued closure still opens the unlock and keystore files and signs, violating the provider contract that the guard succeed immediately before key access. Carry a guard or lease into the blocking operation, or otherwise revalidate at the actual access boundary.
AGENTS.md reference: AGENTS.md:L91-L94
Useful? React with 👍 / 👎.
1 participant
No description provided.