feat(mimd-0025): durable intent execution#1368
Conversation
# Conflicts: # magicblock-accounts/src/scheduled_commits_processor.rs # magicblock-api/src/magic_validator.rs
# Conflicts: # magicblock-accounts/src/scheduled_commits_processor.rs # magicblock-api/src/magic_validator.rs # magicblock-committor-service/src/committor_processor.rs # magicblock-committor-service/src/service.rs # test-integration/test-committor-service/tests/test_ix_commit_local.rs
# Conflicts: # magicblock-api/src/magic_validator.rs # magicblock-committor-service/src/committor_processor.rs
# Conflicts: # Cargo.lock # Cargo.toml # programs/magicblock/src/intent_bundles/schedule/mod.rs # programs/magicblock/src/intent_bundles/schedule/process_schedule_cloned_undelegation.rs # programs/magicblock/src/lib.rs # programs/magicblock/src/magicblock_processor.rs # test-integration/Cargo.lock
# Conflicts: # magicblock-api/src/magic_sys_adapter.rs # magicblock-api/src/magic_validator.rs # magicblock-committor-service/Cargo.toml # magicblock-committor-service/src/committor_processor.rs # magicblock-committor-service/src/error.rs # magicblock-committor-service/src/intent_engine/intent_execution_engine.rs # magicblock-committor-service/src/intent_executor/mod.rs # magicblock-committor-service/src/lib.rs # magicblock-committor-service/src/persist/commit_persister.rs # magicblock-committor-service/src/service.rs # magicblock-committor-service/src/transactions.rs # test-integration/test-committor-service/Cargo.toml # test-integration/test-committor-service/tests/test_ix_commit_local.rs
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
magicblock-magic-program-api/src/instruction.rs (1)
85-85: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winGate the
MagicBlockInstructionwire-format change behind a version bump. These payload changes alter the bincode encoding, and the validator deserializes instruction bytes directly, so mixed-version producers/consumers or any persisted instruction data will stop inter-operating unless the protocol is coordinated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-magic-program-api/src/instruction.rs` at line 85, The MagicBlockInstruction wire format change in the instruction enum must be gated behind a protocol/version bump because adding ScheduledCommitSent(u64) changes the bincode encoding. Update the versioning/compatibility path around MagicBlockInstruction and its deserialization so mixed-version producers and consumers are coordinated before this variant is exposed, and ensure any instruction-byte handling is version-aware where the validator reads it directly.magicblock-api/src/magic_validator.rs (1)
196-203: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the fresh-start broker startup path
Broker::connect()has no explicit connection timeout and retries forever, so a broker outage can stall validator startup before this branch returns. Add an outer timeout here or cap the connect/retry policy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-api/src/magic_validator.rs` around lines 196 - 203, The fresh-start path in magic_validator::source can block startup indefinitely because Broker::connect() may retry forever without an explicit bound. Update this branch to enforce a startup timeout or limit the broker retry/connect policy before awaiting the connection, and keep the change localized around the is_fresh_start logic that creates broker and calls get_snapshot.
♻️ Duplicate comments (1)
magicblock-metrics/src/metrics/mod.rs (1)
165-177: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRemove the new production metric initialization panics.
Line 173 and Line 177 add
.unwrap()paths in production metric initialization; route construction through fallible registration/init or add explicit invariant justification for the hard-coded metric definitions. As per path instructions, "{magicblock-*,programs,storage-proto}/**: Treat any usage of.unwrap()or.expect()in production Rust code as a MAJOR issue. These should not be categorized as trivial or nit-level concerns. Request proper error handling or explicit justification with invariants."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-metrics/src/metrics/mod.rs` around lines 165 - 177, The new metric statics in metrics::mod introduce production panics via IntCounter::new(...).unwrap() and the existing IntGauge::new(...).unwrap_or_else(...panic!), so replace these initialization paths with fallible registration/initialization handling or add a clear invariant-based justification if panic is truly impossible. Update the affected static refs INFLIGHT_SUBSCRIPTION_UPDATES_GAUGE, EVICTED_ACCOUNTS_COUNT, and PROGRAM_SUBSCRIPTION_DISCOVERED_DLP_UPDATE_DELEGATED_ELSEWHERE_COUNT to use safe construction patterns consistent with the rest of the metrics module.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@magicblock-magic-program-api/src/instruction.rs`:
- Around line 394-402: The public contract for ScheduleUndelegation documents
the delegated clone target as readonly even though the instruction mutates it
via mark_account_as_undelegated and the builder already treats it as writable.
Update the account reference in instruction.rs for ScheduleUndelegation so
account 1 is marked writable, and keep the builder/handler contract aligned with
that symbol.
---
Outside diff comments:
In `@magicblock-api/src/magic_validator.rs`:
- Around line 196-203: The fresh-start path in magic_validator::source can block
startup indefinitely because Broker::connect() may retry forever without an
explicit bound. Update this branch to enforce a startup timeout or limit the
broker retry/connect policy before awaiting the connection, and keep the change
localized around the is_fresh_start logic that creates broker and calls
get_snapshot.
In `@magicblock-magic-program-api/src/instruction.rs`:
- Line 85: The MagicBlockInstruction wire format change in the instruction enum
must be gated behind a protocol/version bump because adding
ScheduledCommitSent(u64) changes the bincode encoding. Update the
versioning/compatibility path around MagicBlockInstruction and its
deserialization so mixed-version producers and consumers are coordinated before
this variant is exposed, and ensure any instruction-byte handling is
version-aware where the validator reads it directly.
---
Duplicate comments:
In `@magicblock-metrics/src/metrics/mod.rs`:
- Around line 165-177: The new metric statics in metrics::mod introduce
production panics via IntCounter::new(...).unwrap() and the existing
IntGauge::new(...).unwrap_or_else(...panic!), so replace these initialization
paths with fallible registration/initialization handling or add a clear
invariant-based justification if panic is truly impossible. Update the affected
static refs INFLIGHT_SUBSCRIPTION_UPDATES_GAUGE, EVICTED_ACCOUNTS_COUNT, and
PROGRAM_SUBSCRIPTION_DISCOVERED_DLP_UPDATE_DELEGATED_ELSEWHERE_COUNT to use safe
construction patterns consistent with the rest of the metrics module.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9bc01ef4-d475-4cdb-a4e5-ea3fc455f75d
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.tomlmagicblock-api/src/magic_sys_adapter.rsmagicblock-api/src/magic_validator.rsmagicblock-magic-program-api/src/instruction.rsmagicblock-metrics/src/metrics/mod.rsprograms/magicblock/src/intent_bundles/schedule/mod.rsprograms/magicblock/src/intent_bundles/schedule/process_schedule_cloned_undelegation.rsprograms/magicblock/src/lib.rsprograms/magicblock/src/magicblock_processor.rsprograms/magicblock/src/utils/instruction_utils.rstest-integration/Cargo.toml
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
magicblock-magic-program-api/src/instruction.rs (1)
85-85: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winGate the
MagicBlockInstructionwire-format change behind a version bump. These payload changes alter the bincode encoding, and the validator deserializes instruction bytes directly, so mixed-version producers/consumers or any persisted instruction data will stop inter-operating unless the protocol is coordinated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-magic-program-api/src/instruction.rs` at line 85, The MagicBlockInstruction wire format change in the instruction enum must be gated behind a protocol/version bump because adding ScheduledCommitSent(u64) changes the bincode encoding. Update the versioning/compatibility path around MagicBlockInstruction and its deserialization so mixed-version producers and consumers are coordinated before this variant is exposed, and ensure any instruction-byte handling is version-aware where the validator reads it directly.magicblock-api/src/magic_validator.rs (1)
196-203: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the fresh-start broker startup path
Broker::connect()has no explicit connection timeout and retries forever, so a broker outage can stall validator startup before this branch returns. Add an outer timeout here or cap the connect/retry policy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-api/src/magic_validator.rs` around lines 196 - 203, The fresh-start path in magic_validator::source can block startup indefinitely because Broker::connect() may retry forever without an explicit bound. Update this branch to enforce a startup timeout or limit the broker retry/connect policy before awaiting the connection, and keep the change localized around the is_fresh_start logic that creates broker and calls get_snapshot.
♻️ Duplicate comments (1)
magicblock-metrics/src/metrics/mod.rs (1)
165-177: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRemove the new production metric initialization panics.
Line 173 and Line 177 add
.unwrap()paths in production metric initialization; route construction through fallible registration/init or add explicit invariant justification for the hard-coded metric definitions. As per path instructions, "{magicblock-*,programs,storage-proto}/**: Treat any usage of.unwrap()or.expect()in production Rust code as a MAJOR issue. These should not be categorized as trivial or nit-level concerns. Request proper error handling or explicit justification with invariants."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-metrics/src/metrics/mod.rs` around lines 165 - 177, The new metric statics in metrics::mod introduce production panics via IntCounter::new(...).unwrap() and the existing IntGauge::new(...).unwrap_or_else(...panic!), so replace these initialization paths with fallible registration/initialization handling or add a clear invariant-based justification if panic is truly impossible. Update the affected static refs INFLIGHT_SUBSCRIPTION_UPDATES_GAUGE, EVICTED_ACCOUNTS_COUNT, and PROGRAM_SUBSCRIPTION_DISCOVERED_DLP_UPDATE_DELEGATED_ELSEWHERE_COUNT to use safe construction patterns consistent with the rest of the metrics module.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@magicblock-magic-program-api/src/instruction.rs`:
- Around line 394-402: The public contract for ScheduleUndelegation documents
the delegated clone target as readonly even though the instruction mutates it
via mark_account_as_undelegated and the builder already treats it as writable.
Update the account reference in instruction.rs for ScheduleUndelegation so
account 1 is marked writable, and keep the builder/handler contract aligned with
that symbol.
---
Outside diff comments:
In `@magicblock-api/src/magic_validator.rs`:
- Around line 196-203: The fresh-start path in magic_validator::source can block
startup indefinitely because Broker::connect() may retry forever without an
explicit bound. Update this branch to enforce a startup timeout or limit the
broker retry/connect policy before awaiting the connection, and keep the change
localized around the is_fresh_start logic that creates broker and calls
get_snapshot.
In `@magicblock-magic-program-api/src/instruction.rs`:
- Line 85: The MagicBlockInstruction wire format change in the instruction enum
must be gated behind a protocol/version bump because adding
ScheduledCommitSent(u64) changes the bincode encoding. Update the
versioning/compatibility path around MagicBlockInstruction and its
deserialization so mixed-version producers and consumers are coordinated before
this variant is exposed, and ensure any instruction-byte handling is
version-aware where the validator reads it directly.
---
Duplicate comments:
In `@magicblock-metrics/src/metrics/mod.rs`:
- Around line 165-177: The new metric statics in metrics::mod introduce
production panics via IntCounter::new(...).unwrap() and the existing
IntGauge::new(...).unwrap_or_else(...panic!), so replace these initialization
paths with fallible registration/initialization handling or add a clear
invariant-based justification if panic is truly impossible. Update the affected
static refs INFLIGHT_SUBSCRIPTION_UPDATES_GAUGE, EVICTED_ACCOUNTS_COUNT, and
PROGRAM_SUBSCRIPTION_DISCOVERED_DLP_UPDATE_DELEGATED_ELSEWHERE_COUNT to use safe
construction patterns consistent with the rest of the metrics module.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9bc01ef4-d475-4cdb-a4e5-ea3fc455f75d
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.tomlmagicblock-api/src/magic_sys_adapter.rsmagicblock-api/src/magic_validator.rsmagicblock-magic-program-api/src/instruction.rsmagicblock-metrics/src/metrics/mod.rsprograms/magicblock/src/intent_bundles/schedule/mod.rsprograms/magicblock/src/intent_bundles/schedule/process_schedule_cloned_undelegation.rsprograms/magicblock/src/lib.rsprograms/magicblock/src/magicblock_processor.rsprograms/magicblock/src/utils/instruction_utils.rstest-integration/Cargo.toml
🛑 Comments failed to post (1)
magicblock-magic-program-api/src/instruction.rs (1)
394-402: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Mark the cloned account writable in the public contract.
Line 399 documents account 1 as readonly, but the builder emits it as writable and the handler calls
mark_account_as_undelegatedon it. Clients following this doc will build an instruction that fails when the program tries to mutate the account.Proposed fix
- /// - **1.** `[]` Delegated clone target + /// - **1.** `[WRITE]` Delegated clone target📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./// Schedules undelegation immediately after a matching delegated clone /// instruction in the same transaction. /// /// # Account references /// - **0.** `[SIGNER]` Validator authority /// - **1.** `[WRITE]` Delegated clone target /// - **2.** `[]` Instructions sysvar /// - **3.** `[WRITE]` Magic Context account ScheduleUndelegation { cloned_account_pubkey: Pubkey },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-magic-program-api/src/instruction.rs` around lines 394 - 402, The public contract for ScheduleUndelegation documents the delegated clone target as readonly even though the instruction mutates it via mark_account_as_undelegated and the builder already treats it as writable. Update the account reference in instruction.rs for ScheduleUndelegation so account 1 is marked writable, and keep the builder/handler contract aligned with that symbol.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-committor-service/src/outbox/outbox_client.rs (1)
199-207: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winHandle scheduler failures by rolling back the pending commit
register_scheduled_commit_sent(sent_commit)writes straight intoSENT_COMMITS. Ifexecute()fails, that entry is never removed, so stale commit state can accumulate. Add an explicit cleanup path on the error branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-committor-service/src/outbox/outbox_client.rs` around lines 199 - 207, The pending commit is registered in SENT_COMMITS before scheduling, but if OutboxClient::execute fails the sent_commit entry is never cleaned up. Update outbox_client::OutboxClient::execute flow so the error path explicitly rolls back the registered commit (or otherwise removes it from SENT_COMMITS) after register_scheduled_commit_sent(sent_commit), using the existing sent_commit value and the execute() result to keep scheduler state consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@magicblock-committor-service/src/outbox/outbox_client.rs`:
- Around line 199-207: The pending commit is registered in SENT_COMMITS before
scheduling, but if OutboxClient::execute fails the sent_commit entry is never
cleaned up. Update outbox_client::OutboxClient::execute flow so the error path
explicitly rolls back the registered commit (or otherwise removes it from
SENT_COMMITS) after register_scheduled_commit_sent(sent_commit), using the
existing sent_commit value and the execute() result to keep scheduler state
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 363c3fb4-316e-43c4-8d6c-2959ac8be0f1
📒 Files selected for processing (4)
magicblock-committor-service/src/outbox/mod.rsmagicblock-committor-service/src/outbox/outbox_client.rsmagicblock-committor-service/src/outbox/utils.rsmagicblock-committor-service/src/service.rs
# Conflicts: # Cargo.lock # Cargo.toml # magicblock-committor-service/src/intent_executor/mod.rs # test-integration/Cargo.lock # test-integration/Cargo.toml
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-metrics/src/metrics/mod.rs (1)
171-289: 🩺 Stability & Availability | 🟠 Major | ⚖️ Poor tradeoffNew
.unwrap()calls on metric construction — MAJOR per path instructions.Every new
IntCounterVec/IntGaugeVec/HistogramVec::new(...)in this block panics via.unwrap()on construction failure. This repo's path instructions require proper error handling or an explicit invariant justification for.unwrap()/.expect()in production code undermagicblock-*/**.Since these are all static, compile-time-constant
Opts/label definitions, failure is unlikely, but per policy this should either use a fallible registration path or carry an inline comment justifying the invariant (e.g., "label counts and metric names are validated at compile time / by test coverage").As per path instructions, "
{magicblock-*,programs,storage-proto}/**: Treat any usage of.unwrap()or.expect()in production Rust code as a MAJOR issue. These should not be categorized as trivial or nit-level concerns. Request proper error handling or explicit justification with invariants."Also applies to: 379-407
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-metrics/src/metrics/mod.rs` around lines 171 - 289, The metric definitions in this block use `.unwrap()` on `IntCounterVec`, `IntGaugeVec`, and `HistogramVec` construction, which violates the production Rust error-handling policy for `magicblock-metrics`. Update the static metric initialization sites such as `CHAINLINK_BANK_PRECHECK_ACCOUNTS_TOTAL`, `CHAINLINK_PENDING_FETCH_OWNER_DURATION_SECONDS`, and the other metric constants to avoid bare unwraps by either switching to a fallible registration/initialization path or adding an explicit inline invariant justification where the failure case is impossible by design.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@magicblock-committor-service/src/transaction_preparator/mod.rs`:
- Line 101: Guard the metrics call in
transaction_preparator::mod::prepare_committor_intent so
`observe_committor_intent_alt_count` is only recorded when lookup tables are
present; currently it emits a 0 sample for empty inputs. Add a
`lookup_tables.is_empty()` check around the
`metrics::observe_committor_intent_alt_count` invocation so transactions without
ALTs skip this observation.
In `@magicblock-metrics/src/metrics/mod.rs`:
- Around line 572-578: The new COMMITTOR_INTENT_ALT_COUNT metric initialization
uses Histogram::with_opts(...).unwrap(), which adds a panic path in production.
Update the static histogram setup in metrics::mod::COMMITTOR_INTENT_ALT_COUNT to
avoid unwrap by either handling the registration error explicitly or documenting
the invariant inline where the metric is created, consistent with the
surrounding metric definitions. Ensure the fix keeps the metric initialization
safe and non-panicking in normal runtime code.
---
Outside diff comments:
In `@magicblock-metrics/src/metrics/mod.rs`:
- Around line 171-289: The metric definitions in this block use `.unwrap()` on
`IntCounterVec`, `IntGaugeVec`, and `HistogramVec` construction, which violates
the production Rust error-handling policy for `magicblock-metrics`. Update the
static metric initialization sites such as
`CHAINLINK_BANK_PRECHECK_ACCOUNTS_TOTAL`,
`CHAINLINK_PENDING_FETCH_OWNER_DURATION_SECONDS`, and the other metric constants
to avoid bare unwraps by either switching to a fallible
registration/initialization path or adding an explicit inline invariant
justification where the failure case is impossible by design.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1de1e70a-94d4-4b03-8ff0-78539ae4a81a
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlmagicblock-committor-service/src/tasks/task_info_fetcher.rsmagicblock-committor-service/src/transaction_preparator/mod.rsmagicblock-metrics/src/metrics/mod.rsprograms/magicblock/src/intent_bundles/schedule/process_schedule_cloned_undelegation.rstest-integration/Cargo.tomltest-integration/test-schedule-intent/tests/test_outbox_flow.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-metrics/src/metrics/mod.rs (1)
171-289: 🩺 Stability & Availability | 🟠 Major | ⚖️ Poor tradeoffNew
.unwrap()calls on metric construction — MAJOR per path instructions.Every new
IntCounterVec/IntGaugeVec/HistogramVec::new(...)in this block panics via.unwrap()on construction failure. This repo's path instructions require proper error handling or an explicit invariant justification for.unwrap()/.expect()in production code undermagicblock-*/**.Since these are all static, compile-time-constant
Opts/label definitions, failure is unlikely, but per policy this should either use a fallible registration path or carry an inline comment justifying the invariant (e.g., "label counts and metric names are validated at compile time / by test coverage").As per path instructions, "
{magicblock-*,programs,storage-proto}/**: Treat any usage of.unwrap()or.expect()in production Rust code as a MAJOR issue. These should not be categorized as trivial or nit-level concerns. Request proper error handling or explicit justification with invariants."Also applies to: 379-407
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-metrics/src/metrics/mod.rs` around lines 171 - 289, The metric definitions in this block use `.unwrap()` on `IntCounterVec`, `IntGaugeVec`, and `HistogramVec` construction, which violates the production Rust error-handling policy for `magicblock-metrics`. Update the static metric initialization sites such as `CHAINLINK_BANK_PRECHECK_ACCOUNTS_TOTAL`, `CHAINLINK_PENDING_FETCH_OWNER_DURATION_SECONDS`, and the other metric constants to avoid bare unwraps by either switching to a fallible registration/initialization path or adding an explicit inline invariant justification where the failure case is impossible by design.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@magicblock-committor-service/src/transaction_preparator/mod.rs`:
- Line 101: Guard the metrics call in
transaction_preparator::mod::prepare_committor_intent so
`observe_committor_intent_alt_count` is only recorded when lookup tables are
present; currently it emits a 0 sample for empty inputs. Add a
`lookup_tables.is_empty()` check around the
`metrics::observe_committor_intent_alt_count` invocation so transactions without
ALTs skip this observation.
In `@magicblock-metrics/src/metrics/mod.rs`:
- Around line 572-578: The new COMMITTOR_INTENT_ALT_COUNT metric initialization
uses Histogram::with_opts(...).unwrap(), which adds a panic path in production.
Update the static histogram setup in metrics::mod::COMMITTOR_INTENT_ALT_COUNT to
avoid unwrap by either handling the registration error explicitly or documenting
the invariant inline where the metric is created, consistent with the
surrounding metric definitions. Ensure the fix keeps the metric initialization
safe and non-panicking in normal runtime code.
---
Outside diff comments:
In `@magicblock-metrics/src/metrics/mod.rs`:
- Around line 171-289: The metric definitions in this block use `.unwrap()` on
`IntCounterVec`, `IntGaugeVec`, and `HistogramVec` construction, which violates
the production Rust error-handling policy for `magicblock-metrics`. Update the
static metric initialization sites such as
`CHAINLINK_BANK_PRECHECK_ACCOUNTS_TOTAL`,
`CHAINLINK_PENDING_FETCH_OWNER_DURATION_SECONDS`, and the other metric constants
to avoid bare unwraps by either switching to a fallible
registration/initialization path or adding an explicit inline invariant
justification where the failure case is impossible by design.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1de1e70a-94d4-4b03-8ff0-78539ae4a81a
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlmagicblock-committor-service/src/tasks/task_info_fetcher.rsmagicblock-committor-service/src/transaction_preparator/mod.rsmagicblock-metrics/src/metrics/mod.rsprograms/magicblock/src/intent_bundles/schedule/process_schedule_cloned_undelegation.rstest-integration/Cargo.tomltest-integration/test-schedule-intent/tests/test_outbox_flow.rs
🛑 Comments failed to post (2)
magicblock-committor-service/src/transaction_preparator/mod.rs (1)
101-101: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== transaction_preparator mod.rs ==\n' git ls-files 'magicblock-committor-service/src/transaction_preparator/mod.rs' \ && nl -ba magicblock-committor-service/src/transaction_preparator/mod.rs | sed -n '1,170p' printf '\n== metrics mod.rs search ==\n' git ls-files 'magicblock-metrics/src/metrics/mod.rs' \ && nl -ba magicblock-metrics/src/metrics/mod.rs | sed -n '1,260p'Repository: magicblock-labs/magicblock-validator
Length of output: 314
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== metric usage search ==\n' rg -n "observe_committor_intent_alt_count|COMMITTOR_INTENT_ALT_COUNT|intent alt" \ magicblock-committor-service magicblock-metrics -S printf '\n== surrounding call site ==\n' nl -ba magicblock-committor-service/src/transaction_preparator/mod.rs | sed -n '88,112p' printf '\n== metric definition slice ==\n' nl -ba magicblock-metrics/src/metrics/mod.rs | sed -n '1,120p'Repository: magicblock-labs/magicblock-validator
Length of output: 817
🏁 Script executed:
#!/bin/bash set -euo pipefail nl -ba magicblock-committor-service/src/transaction_preparator/mod.rs | sed -n '96,106p' printf '\n' nl -ba magicblock-metrics/src/metrics/mod.rs | sed -n '1,80p'Repository: magicblock-labs/magicblock-validator
Length of output: 214
Guard
observe_committor_intent_alt_countfor non-empty lookup tables
COMMITTOR_INTENT_ALT_COUNTis documented as only recorded when ALTs are present, but this call emits a0sample for transactions without lookup tables. Skip the observation whenlookup_tables.is_empty().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-committor-service/src/transaction_preparator/mod.rs` at line 101, Guard the metrics call in transaction_preparator::mod::prepare_committor_intent so `observe_committor_intent_alt_count` is only recorded when lookup tables are present; currently it emits a 0 sample for empty inputs. Add a `lookup_tables.is_empty()` check around the `metrics::observe_committor_intent_alt_count` invocation so transactions without ALTs skip this observation.magicblock-metrics/src/metrics/mod.rs (1)
572-578: 🩺 Stability & Availability | 🟠 Major | ⚖️ Poor tradeoff
.unwrap()on newCOMMITTOR_INTENT_ALT_COUNThistogram — MAJOR per path instructions.Same concern as the block above:
Histogram::with_opts(...).unwrap()introduces a new panic path in production code for this PR's central metric. Please justify the invariant inline or handle registration failure explicitly.As per path instructions, "
{magicblock-*,programs,storage-proto}/**: Treat any usage of.unwrap()or.expect()in production Rust code as a MAJOR issue... Request proper error handling or explicit justification with invariants."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-metrics/src/metrics/mod.rs` around lines 572 - 578, The new COMMITTOR_INTENT_ALT_COUNT metric initialization uses Histogram::with_opts(...).unwrap(), which adds a panic path in production. Update the static histogram setup in metrics::mod::COMMITTOR_INTENT_ALT_COUNT to avoid unwrap by either handling the registration error explicitly or documenting the invariant inline where the metric is created, consistent with the surrounding metric definitions. Ensure the fix keeps the metric initialization safe and non-panicking in normal runtime code.Source: Path instructions
fix: IntentStream
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
magicblock-committor-service/src/intent_engine/intent_execution_engine.rs (2)
280-290: 🩺 Stability & Availability | 🔵 TrivialUnresolved TODO: failed intents aren't locked against re-scheduling.
The comment notes
Err(T)intents aren't pubkey-locked, meaning a failed intent's accounts could be picked up again before the failure is fully accounted for. This is flagged by the author but left open.Want me to sketch an approach for locking pubkeys on failure, or should this be tracked as a follow-up issue?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-committor-service/src/intent_engine/intent_execution_engine.rs` around lines 280 - 290, The failed-intent path in IntentExecutionEngine::execute still leaves pubkeys unlocked, so an Err result can be re-scheduled before its failure is accounted for. Update the execution/scheduling flow around the inner_scheduler.complete call to lock the intent’s pubkeys on failure as well as success, using the existing intent and scheduler handling in IntentExecutionEngine and IntentScheduler. If this cannot be done safely in the current pass, convert the TODO into a tracked follow-up with an explicit failure-handling plan rather than leaving the gap implicit.
141-153: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd backoff before retrying on persistent
DBError.
continueimmediately re-enters the loop onIntentScheduleError::DBError, so a sustained DB outage causes a tight retry loop with unthrottled error logging instead of degrading gracefully.⚡ Proposed fix
Err(IntentScheduleError::DBError(err)) => { // TODO(edwin): add to alert as this is critical error error!(error = ?err, "Failed to fetch intent"); + tokio::time::sleep(DB_ERROR_RETRY_BACKOFF).await; continue; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-committor-service/src/intent_engine/intent_execution_engine.rs` around lines 141 - 153, The DBError branch in intent_execution_engine’s loop retries immediately, which can create a tight error/logging loop during a sustained outage. Update the Err(IntentScheduleError::DBError(err)) path in intent_execution_engine::run (or the surrounding loop that calls next_scheduled_intent) to wait with backoff before retrying, ideally using an increasing delay with a cap and resetting after a successful fetch. Keep the existing error log, but add the delay logic before re-entering the loop so failures degrade gracefully instead of hammering the DB.magicblock-committor-service/src/intent_executor/two_stage_intent_executor.rs (1)
237-281: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEmit intent metrics for two-stage completions here too. This path skips the
intent_metrics(...)spawn that the other executors use, so two-stage completions won’t report the execution-outcome metric.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-committor-service/src/intent_executor/two_stage_intent_executor.rs` around lines 237 - 281, The two-stage executor’s execute path is missing the intent metrics emission that other executors perform, so completions here never report execution outcome metrics. Update two_stage_intent_executor::execute to trigger the same intent_metrics(...) spawn used elsewhere, using the existing IntentExecutionResult built from execute_inner and execution_report so two-stage completions are recorded consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@magicblock-committor-service/src/intent_engine/intent_channel.rs`:
- Around line 97-109: The backlog draining logic in intent_channel.rs can
terminate early because `poll_next` currently treats `Poll::Ready(None)` the
same as a live channel item and returns `None` before `db` is emptied. Update
the `IntentChannel::poll_next` branch that checks `this.stream.poll_next(cx)` so
it only uses the stream item when it is `Some`, and otherwise falls back to
`db.pop_intent_bundle()` until the backlog is drained. Make the change in the
`poll_next` implementation to distinguish `Poll::Ready(Some(_))` from
`Poll::Ready(None)`.
---
Outside diff comments:
In `@magicblock-committor-service/src/intent_engine/intent_execution_engine.rs`:
- Around line 280-290: The failed-intent path in IntentExecutionEngine::execute
still leaves pubkeys unlocked, so an Err result can be re-scheduled before its
failure is accounted for. Update the execution/scheduling flow around the
inner_scheduler.complete call to lock the intent’s pubkeys on failure as well as
success, using the existing intent and scheduler handling in
IntentExecutionEngine and IntentScheduler. If this cannot be done safely in the
current pass, convert the TODO into a tracked follow-up with an explicit
failure-handling plan rather than leaving the gap implicit.
- Around line 141-153: The DBError branch in intent_execution_engine’s loop
retries immediately, which can create a tight error/logging loop during a
sustained outage. Update the Err(IntentScheduleError::DBError(err)) path in
intent_execution_engine::run (or the surrounding loop that calls
next_scheduled_intent) to wait with backoff before retrying, ideally using an
increasing delay with a cap and resetting after a successful fetch. Keep the
existing error log, but add the delay logic before re-entering the loop so
failures degrade gracefully instead of hammering the DB.
In
`@magicblock-committor-service/src/intent_executor/two_stage_intent_executor.rs`:
- Around line 237-281: The two-stage executor’s execute path is missing the
intent metrics emission that other executors perform, so completions here never
report execution outcome metrics. Update two_stage_intent_executor::execute to
trigger the same intent_metrics(...) spawn used elsewhere, using the existing
IntentExecutionResult built from execute_inner and execution_report so two-stage
completions are recorded consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4e4886bd-3086-42d9-8a64-df87f8463489
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
magicblock-committor-service/src/error.rsmagicblock-committor-service/src/intent_engine.rsmagicblock-committor-service/src/intent_engine/intent_channel.rsmagicblock-committor-service/src/intent_engine/intent_execution_engine.rsmagicblock-committor-service/src/intent_executor/accepted_intent_executor.rsmagicblock-committor-service/src/intent_executor/cleanup_handle.rsmagicblock-committor-service/src/intent_executor/intent_execution_client.rsmagicblock-committor-service/src/intent_executor/single_stage_intent_executor.rsmagicblock-committor-service/src/intent_executor/strategy_executor/single_stage.rsmagicblock-committor-service/src/intent_executor/strategy_executor/two_stage.rsmagicblock-committor-service/src/intent_executor/strategy_executor/utils.rsmagicblock-committor-service/src/intent_executor/two_stage_intent_executor.rsmagicblock-core/Cargo.tomlmagicblock-magic-program-api/Cargo.tomlmagicblock-magic-program-api/src/outbox.rsprograms/magicblock/src/intent_bundles/magic_scheduled_base_intent.rsprograms/magicblock/src/intent_bundles/outbox_intent_bundles.rstest-integration/test-committor-service/tests/test_intent_executor.rstest-integration/test-schedule-intent/tests/test_outbox_flow.rs
# Conflicts: # Cargo.lock # test-integration/Cargo.lock
1 participant
Summary
Breaking Changes
Test Plan
Summary by CodeRabbit
New Features
Bug Fixes