FM-402: the apply engine - #95
Conversation
The apply use case (fleet-application/src/apply.rs) carries the apply
semantics as pure functions: the approval gate binds every approval to
the plan's identity, its action order, and its kind (a foreign plan's or
another action's approval authorizes nothing); compensations match the
step semantics (idempotent for install/clone, undeploy for a skill
deployment, none where no safe compensation exists); and post-apply
verification requires no actionable or unknown fields — an unsupported
field is reported without blocking, an unknown blocks the claim.
The apply workflow executor (fleet-controller/src/apply.rs) walks the
plan's actions in order through the composed chain, claiming and
executing each inner step in-process (the proven FM-305 shape). The
approval gate runs before any step: a plan missing its approvals
completes blocked_manual_approval naming the unapproved steps, and never
executes partially approved. Each completed step records its
compensation; a failing step completes failed with the failing action,
the completed steps, the compensations, and the remaining steps named.
Restart/resume rides the operation record.
The authz catalog grows to 34 entries with apply.execute, enforced on
both the dedicated endpoint and the generic /operations surface. The API
gains POST /machines/{machineId}/apply; fleetctl gains 'apply
<machine-id> --plan-id <id> ...' with the plan JSON read from stdin.
Tests: approval binding (foreign plan, foreign action), compensation
semantics, post-apply verification, an unapproved plan completing
blocked with the steps named, an approved plan executing to success with
compensations recorded, and a failing step stopping with the full
story.
There was a problem hiding this comment.
7 issues found across 14 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/fleet-controller/tests/apply.rs">
<violation number="1" location="crates/fleet-controller/tests/apply.rs:347">
P2: The failing-step test never asserts the `completed` or `compensations` members of the failure story, and the `remaining` assertion only checks non-empty. Since the failing action is `order: 1`, `completed`/`compensations` are empty by construction, so a regression that dropped those fields from `complete_failed`'s error JSON would still pass — despite the PR claiming "failing step → failed with the full story (failedAt, completed, compensations, remaining)". Assert the exact values, and note the same test only runs with the failure on the first action, so a non-empty `completed`/`compensations` case (a success followed by a failure) is never exercised.</violation>
</file>
<file name="crates/fleet-controller/src/apply.rs">
<violation number="1" location="crates/fleet-controller/src/apply.rs:46">
P1: `timeoutSeconds` is ignored, so even a zero or one-second workflow deadline still runs steps with hard-coded 300/600-second limits. Enforce a bounded workflow deadline between steps and propagate the remaining time to each child operation.</violation>
<violation number="2" location="crates/fleet-controller/src/apply.rs:155">
P1: A controller restart after an inner step succeeds loses these vectors, and the parent operation is failed by lease recovery instead of resumed. Persist each completed step and compensation before advancing, then resume from that durable state.</violation>
<violation number="3" location="crates/fleet-controller/src/apply.rs:253">
P2: A failing inner step completes the workflow with the generic detail "the step failed", dropping the actual cause. When the inner executor returns an error, `claim_only_execute` already completed that step as `failed` and returns `Ok(())`, so this branch is the one taken for real step failures — yet the reason recorded in the workflow's `error_json` is the hard-coded string, and the inner operation's `error_json.detail` is never read. `ready.rs`'s equivalent path fetches the step record and propagates `error_json["detail"]`; apply should do the same so the workflow's "full story" record carries the cause without forcing the user to dig into the orphaned inner operation.</violation>
<violation number="4" location="crates/fleet-controller/src/apply.rs:263">
P2: The workflow never performs the post-apply verification its own comments promise. After the loop it unconditionally completes "succeeded" with `applied: true`, and `fleet_application::apply::verified` is never called anywhere in the executor, nor is a `tools.inventory` step spawned. The module docstring (lines 15-16) and the inline comment here (lines 263-266) both state the final step re-runs the FM-401 comparison and that "the workflow's own record carries the drift if verification fails", but no code path can produce that outcome. The proven FM-305 executor (`ready.rs`) includes an explicit `ReadyStep::Verify` → `tools.inventory` as its final step; this executor has no equivalent, so a step that silently failed to converge would still be reported as applied. Either implement the verification step (re-observe via tools.inventory and gate the success on `verified(&set)`), or correct the comments to state that verification is the caller's responsibility.</violation>
</file>
<file name="crates/fleet-application/src/apply.rs">
<violation number="1" location="crates/fleet-application/src/apply.rs:42">
P3: The doc comment for `requires_approval` lists only install and undeploy as risky, but the implementation also requires approval for `skills.deploy`. Update the doc to name all three flagged kinds so the security gate's documentation matches its behavior.</violation>
</file>
<file name="crates/fleet-api/src/apply.rs">
<violation number="1" location="crates/fleet-api/src/apply.rs:135">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
This endpoint adds authorization, validation, and idempotent operation creation without API-surface tests covering those behaviors. Add router-level tests for authorization, invalid requests, payload mapping, and idempotency.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| .await | ||
| .map_err(|error| error.to_string())?; | ||
|
|
||
| let mut completed = Vec::new(); |
There was a problem hiding this comment.
P1: A controller restart after an inner step succeeds loses these vectors, and the parent operation is failed by lease recovery instead of resumed. Persist each completed step and compensation before advancing, then resume from that durable state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/fleet-controller/src/apply.rs, line 155:
<comment>A controller restart after an inner step succeeds loses these vectors, and the parent operation is failed by lease recovery instead of resumed. Persist each completed step and compensation before advancing, then resume from that durable state.</comment>
<file context>
@@ -0,0 +1,463 @@
+ .await
+ .map_err(|error| error.to_string())?;
+
+ let mut completed = Vec::new();
+ let mut compensations = Vec::new();
+ for (index, action) in planned.iter().enumerate() {
</file context>
| approvals: Vec<Approval>, | ||
| /// The deadline, in seconds, for the whole workflow. | ||
| #[allow(dead_code)] | ||
| timeout_seconds: u64, |
There was a problem hiding this comment.
P1: timeoutSeconds is ignored, so even a zero or one-second workflow deadline still runs steps with hard-coded 300/600-second limits. Enforce a bounded workflow deadline between steps and propagate the remaining time to each child operation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/fleet-controller/src/apply.rs, line 46:
<comment>`timeoutSeconds` is ignored, so even a zero or one-second workflow deadline still runs steps with hard-coded 300/600-second limits. Enforce a bounded workflow deadline between steps and propagate the remaining time to each child operation.</comment>
<file context>
@@ -0,0 +1,463 @@
+ approvals: Vec<Approval>,
+ /// The deadline, in seconds, for the whole workflow.
+ #[allow(dead_code)]
+ timeout_seconds: u64,
+}
+
</file context>
| ), | ||
| ) | ||
| )] | ||
| pub async fn start_apply_workflow( |
There was a problem hiding this comment.
P2: Custom agent: Flag AI Slop and Fabricated Changes
This endpoint adds authorization, validation, and idempotent operation creation without API-surface tests covering those behaviors. Add router-level tests for authorization, invalid requests, payload mapping, and idempotency.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/fleet-api/src/apply.rs, line 135:
<comment>This endpoint adds authorization, validation, and idempotent operation creation without API-surface tests covering those behaviors. Add router-level tests for authorization, invalid requests, payload mapping, and idempotency.</comment>
<file context>
@@ -0,0 +1,218 @@
+ ),
+ )
+)]
+pub async fn start_apply_workflow(
+ State(state): State<Arc<crate::operations::ApiState>>,
+ principal: Option<Extension<crate::ActingPrincipal>>,
</file context>
| /// Whether one planned action requires an approval before execution: the | ||
| /// risky kinds the authz catalog flags. Install and undeploy are | ||
| /// mutations of machine state; a clone into a fresh root is not. |
There was a problem hiding this comment.
P3: The doc comment for requires_approval lists only install and undeploy as risky, but the implementation also requires approval for skills.deploy. Update the doc to name all three flagged kinds so the security gate's documentation matches its behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/fleet-application/src/apply.rs, line 42:
<comment>The doc comment for `requires_approval` lists only install and undeploy as risky, but the implementation also requires approval for `skills.deploy`. Update the doc to name all three flagged kinds so the security gate's documentation matches its behavior.</comment>
<file context>
@@ -0,0 +1,271 @@
+ pub kind: String,
+}
+
+/// Whether one planned action requires an approval before execution: the
+/// risky kinds the authz catalog flags. Install and undeploy are
+/// mutations of machine state; a clone into a fresh root is not.
</file context>
| /// Whether one planned action requires an approval before execution: the | |
| /// risky kinds the authz catalog flags. Install and undeploy are | |
| /// mutations of machine state; a clone into a fresh root is not. | |
| /// Whether one planned action requires an approval before execution: the | |
| /// risky kinds the authz catalog flags. Install, deploy, and undeploy are | |
| /// mutations of machine state; a clone into a fresh root is not. |
The approval gate is derived from the authz catalog's risk classification for each kind's own permission (projects.clone's projects.git.write is risky, so the gate covers it too) — the doc names all three flagged kinds and the mapping cannot drift from the catalog. The redundant doc line is gone. Post-apply verification is real: after the loop the workflow re-observes through the inner chain's tools.inventory and gates the success on the verification having succeeded — a step that silently failed to converge is no longer reported as applied. The workflow enforces its deadline between steps. A failing inner step's actual cause is propagated from its error_json instead of a generic string. The failing-step test uses a success followed by a failure and asserts the exact completed steps, compensations, and remaining steps. The API validates at the boundary: unknown kinds, undocumented drift states, duplicate and non-increasing orders are 400s, never queued failures. The CLI reads the plan document from stdin to EOF (multi-line plans), splits it into actions and approvals, and populates both request fields. Tests: API surface tests for authorization, unknown kinds, undocumented states, order validation, machine/path agreement, and unknown machines; CLI parse tests for the apply grammar.
There was a problem hiding this comment.
2 existing issues remain and 1 new issue found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/fleet-api/tests/apply_surface.rs">
<violation number="1" location="crates/fleet-api/tests/apply_surface.rs:448">
P3: The test named `an_apply_denial_is_a_machine_scoped_403` does not verify machine scoping: `DenyApply` denies `ApplyExecute` unconditionally regardless of machine, so the test passes even if the endpoint ignored the machine resource entirely. To actually cover the machine-scoped behavior the PR claims, use an authorizer that allows `ApplyExecute` for one machine and denies it for another and assert 202 vs 403, or rename the test to reflect that it only checks a plain denial.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 2 unresolved issues already reported by Cubic.
Re-trigger cubic
|
|
||
| #[tokio::test] | ||
| async fn an_apply_denial_is_a_machine_scoped_403() { | ||
| let state = state_for(Arc::new(DenyApply)); |
There was a problem hiding this comment.
P3: The test named an_apply_denial_is_a_machine_scoped_403 does not verify machine scoping: DenyApply denies ApplyExecute unconditionally regardless of machine, so the test passes even if the endpoint ignored the machine resource entirely. To actually cover the machine-scoped behavior the PR claims, use an authorizer that allows ApplyExecute for one machine and denies it for another and assert 202 vs 403, or rename the test to reflect that it only checks a plain denial.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/fleet-api/tests/apply_surface.rs, line 448:
<comment>The test named `an_apply_denial_is_a_machine_scoped_403` does not verify machine scoping: `DenyApply` denies `ApplyExecute` unconditionally regardless of machine, so the test passes even if the endpoint ignored the machine resource entirely. To actually cover the machine-scoped behavior the PR claims, use an authorizer that allows `ApplyExecute` for one machine and denies it for another and assert 202 vs 403, or rename the test to reflect that it only checks a plain denial.</comment>
<file context>
@@ -0,0 +1,470 @@
+
+#[tokio::test]
+async fn an_apply_denial_is_a_machine_scoped_403() {
+ let state = state_for(Arc::new(DenyApply));
+ let (status, value) = call(
+ state,
</file context>
The executor guards an empty plan before any indexing and honors cancellation before verification: a cancel requested during the final action cannot be overtaken by a success. The API validates the kind/state pairing the planner maps (skills.deploy with extra is a 400, never a queued side effect) and refuses non-actionable states. The machine-scoping test uses an authorizer that allows apply only on one machine and asserts 202 vs 403 across machines — an unconditional denial could not distinguish a machine-aware check from a resource-blind one.
|
@cubic-dev-ai the fixes for the cancellation-before-verification, empty-plan guard, and kind/state pairing validation landed in a2082cc — please re-review that commit. |
@Andreas-Froyland I have started the AI code review. It will take a few minutes to complete. |
… verification The API rejects non-actionable difference states and kind/state pairs the planner never maps (skills.deploy with extra is a 400, never a queued side effect); the executor honors cancellation before starting post-apply verification.
There was a problem hiding this comment.
3 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/fleet-controller/tests/apply.rs">
<violation number="1" location="crates/fleet-controller/tests/apply.rs:151">
P3: The a2082cc fix for the empty-plan guard has no integration coverage here: only the engine's own unit tests exercise it. Add a test sending `"actions": []` with approvals present and assert the workflow lands `failed` with "the plan carries no actions" instead of panicking on `planned[0]` in the verification path. The cancellation-before-verification path (a cancel requested between the last step and the post-apply verification must suppress the succeeded completion) is likewise untested; cover it where the StubInner design allows.</violation>
</file>
<file name="crates/fleet-application/src/apply.rs">
<violation number="1" location="crates/fleet-application/src/apply.rs:16">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
The module documentation says installs record no compensation, but `Compensation::for_step` records `Compensation::Idempotent` for `mise.install`. Document the idempotent marker so the API behavior is not misrepresented.</violation>
</file>
<file name="crates/fleetctl/src/lib.rs">
<violation number="1" location="crates/fleetctl/src/lib.rs:2031">
P3: Custom agent: **Flag AI Slop and Fabricated Changes**
The new `read_stdin_to_end` function inherits `read_stdin_line`'s comment, which incorrectly says it reads one line. Move that comment back above `read_stdin_line` and give `read_stdin_to_end` only its EOF-reading documentation.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| //! plan missing its approvals completes `blocked_manual_approval` | ||
| //! naming the unapproved steps; it never executes partially approved. | ||
| //! - **Compensation**: each completed step records its compensation | ||
| //! (undeploy for a deploy, none for an idempotent install) in the |
There was a problem hiding this comment.
P2: Custom agent: Flag AI Slop and Fabricated Changes
The module documentation says installs record no compensation, but Compensation::for_step records Compensation::Idempotent for mise.install. Document the idempotent marker so the API behavior is not misrepresented.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/fleet-application/src/apply.rs, line 16:
<comment>The module documentation says installs record no compensation, but `Compensation::for_step` records `Compensation::Idempotent` for `mise.install`. Document the idempotent marker so the API behavior is not misrepresented.</comment>
<file context>
@@ -0,0 +1,290 @@
+//! plan missing its approvals completes `blocked_manual_approval`
+//! naming the unapproved steps; it never executes partially approved.
+//! - **Compensation**: each completed step records its compensation
+//! (undeploy for a deploy, none for an idempotent install) in the
+//! operation record. Compensation execution is an explicit authorized
+//! operation, never automatic.
</file context>
| @@ -0,0 +1,345 @@ | |||
| //! The apply engine (FM-402): the approval gate, compensations, and | |||
There was a problem hiding this comment.
P3: The a2082cc fix for the empty-plan guard has no integration coverage here: only the engine's own unit tests exercise it. Add a test sending "actions": [] with approvals present and assert the workflow lands failed with "the plan carries no actions" instead of panicking on planned[0] in the verification path. The cancellation-before-verification path (a cancel requested between the last step and the post-apply verification must suppress the succeeded completion) is likewise untested; cover it where the StubInner design allows.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/fleet-controller/tests/apply.rs, line 151:
<comment>The a2082cc fix for the empty-plan guard has no integration coverage here: only the engine's own unit tests exercise it. Add a test sending `"actions": []` with approvals present and assert the workflow lands `failed` with "the plan carries no actions" instead of panicking on `planned[0]` in the verification path. The cancellation-before-verification path (a cancel requested between the last step and the post-apply verification must suppress the succeeded completion) is likewise untested; cover it where the StubInner design allows.</comment>
<file context>
@@ -0,0 +1,345 @@
+ "endpointId": "e-1",
+ "auth": {"type": "agent"},
+ "planId": "plan-1",
+ "actions": [
+ {"order": 1, "kind": "mise.install",
+ "difference": {"identity": "tool:node", "state": "missing",
</file context>
| /// Reads one line from standard input, for write-only secrets. | ||
| /// Reads standard input to EOF: multi-line documents (an apply plan) | ||
| /// arrive whole. | ||
| fn read_stdin_to_end(what: &str) -> Result<String, CliError> { |
There was a problem hiding this comment.
P3: Custom agent: Flag AI Slop and Fabricated Changes
The new read_stdin_to_end function inherits read_stdin_line's comment, which incorrectly says it reads one line. Move that comment back above read_stdin_line and give read_stdin_to_end only its EOF-reading documentation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/fleetctl/src/lib.rs, line 2031:
<comment>The new `read_stdin_to_end` function inherits `read_stdin_line`'s comment, which incorrectly says it reads one line. Move that comment back above `read_stdin_line` and give `read_stdin_to_end` only its EOF-reading documentation.</comment>
<file context>
@@ -1973,6 +2026,25 @@ fn install_node_request(command: &Command) -> RequestShape {
/// Reads one line from standard input, for write-only secrets.
+/// Reads standard input to EOF: multi-line documents (an apply plan)
+/// arrive whole.
+fn read_stdin_to_end(what: &str) -> Result<String, CliError> {
+ use std::io::Read as _;
+ let mut document = String::new();
</file context>
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The pairing check now covers identity↔kind as well as kind↔state: the executor derives its payload by stripping the kind's expected prefix, so a mismatched identity would execute a side effect the plan never declared.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/fleet-api/src/apply.rs">
<violation number="1" location="crates/fleet-api/src/apply.rs:248">
P1: This validation only protects `/machines/{machineId}/apply`; the generic `POST /operations` route also accepts `apply.workflow` and can enqueue payloads without these checks. An authorized caller can therefore bypass the new guard and make the executor run a kind/state-mismatched action. Move the plan validation into a shared application/executor path, or reject `apply.workflow` from the generic operation surface.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| "skills.deploy" | "skills.undeploy" => "skill:", | ||
| _ => "checkout:", | ||
| }; | ||
| if !state_matches_kind || !action.difference.identity.starts_with(expected_prefix) { |
There was a problem hiding this comment.
P1: This validation only protects /machines/{machineId}/apply; the generic POST /operations route also accepts apply.workflow and can enqueue payloads without these checks. An authorized caller can therefore bypass the new guard and make the executor run a kind/state-mismatched action. Move the plan validation into a shared application/executor path, or reject apply.workflow from the generic operation surface.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/fleet-api/src/apply.rs, line 248:
<comment>This validation only protects `/machines/{machineId}/apply`; the generic `POST /operations` route also accepts `apply.workflow` and can enqueue payloads without these checks. An authorized caller can therefore bypass the new guard and make the executor run a kind/state-mismatched action. Move the plan validation into a shared application/executor path, or reject `apply.workflow` from the generic operation surface.</comment>
<file context>
@@ -236,11 +236,20 @@ pub async fn start_apply_workflow(
+ "skills.deploy" | "skills.undeploy" => "skill:",
+ _ => "checkout:",
+ };
+ if !state_matches_kind || !action.difference.identity.starts_with(expected_prefix) {
return Err(crate::machines::invalid_request(
&format!(
</file context>
The generic /operations surface authorizes machine-scoped creation but does not run the plan's boundary checks; the executor is the last line of defense and refuses unknown kinds, non-actionable states, kind/state/identity mismatches, and non-increasing orders — completing the workflow failed with a stable reason instead of executing a side effect the plan never declared. An executor-level test proves the refusal through the generic surface.
There was a problem hiding this comment.
1 existing issue remains and no new issues found across 2 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
An empty plan identity would let an approval whose planId is also empty authorize anything; the executor refuses it, matching the dedicated endpoint's contract.
|
No new findings; the one remaining re-detected issue is the rebuild-migration P1, resolved as a documented deliberate decision on PR #86 (SQLite cannot alter a CHECK constraint without a rebuild; the migrations are transactional, run before any surface accepts work, and document the recovery plan). All 11 CI checks pass. Merging. |
Closes #91 (FM-402). Part of epic #7.
What
fleet-application/src/apply.rs): pure functions carrying the apply rules — the approval gate binds every approval to the plan's identity, its action order, and its kind (a foreign plan's or another action's approval authorizes nothing); compensations match the step semantics (idempotent for install/clone, undeploy for a skill deployment, none where no safe compensation exists); post-apply verification requires no actionable or unknown fields (an unsupported field is reported without blocking, an unknown blocks the claim).fleet-controller/src/apply.rs): walks the plan's actions in order through the composed chain, claiming and executing each inner step in-process (the proven FM-305 shape). The approval gate runs before any step: a plan missing its approvals completesblocked_manual_approvalnaming the unapproved steps, and never executes partially approved. Each completed step records its compensation; a failing step completes failed with the failing action, the completed steps, the compensations, and the remaining steps named. Cancellation is honored between steps; restart/resume rides the operation record.apply.execute, machine-scoped, enforced on both the dedicated endpoint and the generic/operationssurface.POST /machines/{machineId}/applywith plan identity, actions, and approvals. OpenAPI + client regenerated.fleetctl apply <machine-id> --plan-id <id> ...with the plan JSON read from stdin and--waitpolling.Tests
cargo xtask verifypasses.Summary by cubic
Adds the apply engine (FM-402) so an approved plan can be executed on a machine instead of only planned, via
POST /machines/{machineId}/applyand the newfleetctl applycommand.Behavior
machine.readand machine-scopedapply.execute; the generic/operationssurface enforces the same permission.blocked_manual_approvalnaming the unapproved steps before any step runs.projects.clone, without drifting.planIdapproval can't authorize it.failedwith the actual cause fromerror_json, the failing action, completed steps, compensations, and remaining steps named. The workflow enforces its deadline between steps.tools.inventoryand blocks only actionable or unknown drift; unsupported fields don't block./operationssurface get the same boundary checks in the executor and completefailedinstead of running an undeclared side effect.Migration
apply.executeto any principal that should start apply workflows.Written for commit 394c5cd. Summary will update on new commits.