Four minor cleanups: two docs contradicting the code, a lossy seam, two weak tests - #461
Four minor cleanups: two docs contradicting the code, a lossy seam, two weak tests#461mimi1vx wants to merge 4 commits into
Conversation
… fallback A None from last_output was documented as a benign fallback, but HostOutput::default() is exit 0, which every Check reads as success — the host silently passes and stays in the reboot map instead of failing loudly. Rewrite the doc to state the hazard, WARN at the use site naming the host, and pin the current fallback behaviour with a MockGroup test so a future change to fail-the-check instead is a deliberate, visible decision.
…across the seam Check returned a bare String, so a check that stopped at a cancellation checkpoint could not tell perform_operation apart from one that genuinely failed — UpdateError.cancelled was hardcoded false on this path. Widen Check to Result<(), CheckFailure> (mirrors RebootFailure's shape) and thread the flag through OperationReport.check_failures and the WorkflowRegistry::check adapter into UpdateError. No producer sets cancelled on this path yet; the flag is representability, exercised by a real HostsGroup-backed test.
perform_uninstall_reports_a_transactional_host_that_never_reconnects asserted only the failing host's name, so it passed for any reboot failure cause. Add its install sibling's two assertions: the reason names the unreachable cause and does not claim either of the other two reachable causes, which route the rollback differently.
plan_provider_resolves_the_role_specific_table asserted only that the install and uninstall doers differ, which a role swap in from_operation_role also satisfies. Compare each resolved doer against one built straight from that role's own table entry instead, so only the correct mapping passes.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #461 +/- ##
=======================================
Coverage 96.43% 96.43%
=======================================
Files 194 194
Lines 45621 45702 +81
=======================================
+ Hits 43994 44074 +80
- Misses 1627 1628 +1 ☔ View full report in Codecov by Harness. |
plusky
left a comment
plusky
left a comment
There was a problem hiding this comment.
All four items do what they claim, and the two judgement calls I checked hardest both hold: the probe_failed omission from CheckFailure is genuinely safe (the only producer is UpdateError::probe_failure, reached solely from the update path, which drives its own template and never crosses the Operation seam — and the doc says exactly that, as a decision rather than an oversight), and item 4's self-reference concern (expected doer built from the same table the adapter reads) is covered one layer up by sl_report.rs pinning the literal command text, as the issue itself already argued. The uninstall test now matches its install sibling assertion-for-assertion, including the discriminating negative. CI 16/16 on 024f2ad5.
Two things to fix before merge — both, a little ironically, instances of this PR's own theme.
1. aggregate_failures's cancelled comment now argues from a premise this PR removed
reports/update_flow.rs, the summary branch:
cancelledis deliberately NOT propagated here. Every cancellation in this
module is an earlyreturn Err, so no cancelled error ever reaches a
failuresvec — the line would be dead […]
That was true while the seam was Result<(), String>: a cancelled check was unrepresentable, so the vec provably never carried one. This PR's perform_operation now builds UpdateError { cancelled: failure.cancelled, … } straight from report.check_failures and hands the vec to aggregate_failures — the exact construction the comment says cannot happen. It stays dead in practice only because no production check emits CheckFailure::cancelled today, which is a statement about the current producers, not about the module's structure — one live producer away from false, and the comment gives a future reader a guarantee the types no longer make.
The behaviour itself is defensible either way (a lone cancelled check failure routes verbatim with the flag; a summary drops it, which for the mixed case is just the outranking rule — though note the all-cancelled case loses the flag too, unlike probe_failed's all()). I am not asking for a behaviour change — just rewrite the comment to argue from the new premise: representable but unproduced, single-failure carries it, summary deliberately does not, and why. This is a "doc contradicting the code" being introduced by the PR that closes the doc-contradiction issue.
2. The headline seam fix is not pinned on the side that had the bug
a_cancelled_check_failure_arrives_in_the_report_with_the_flag_intact stops at OperationReport — the mtui-hosts side, which never had the bug. The lossy half was perform_operation's map, and its fix is the five lines cancelled: failure.cancelled. Revert exactly that line to cancelled: false — the pre-PR behaviour — and nothing in the workspace goes red: the existing is_cancelled assertions all exercise flow-level checkpoint cancels, not the check seam. By this tree's own bar ("a pinning test that was never red pins nothing"), the one mutation this PR exists to prevent is currently uncaught.
Cheapest closure: drive perform_install (or perform_uninstall) with a provider whose check returns CheckFailure::cancelled(…) on the single-failure path and assert err.is_cancelled() — one test, and it red/greens on that exact line.
Non-blocking
missing_last_output_silently_passes_the_check_and_keeps_the_reboot cannot see its own fixture wiring break: if with_missing_output stopped being consulted, last_output answers Some(HostOutput::default()) — also exit 0 — and every assertion still passes, so the mutation the test names (fallback changed to fail the check) would silently stop being covered. A one-line liveness guard (assert!(group.last_output("h1").is_none()) before the run, or asserting the new WARN) makes the fixture prove it is in the state the test's name claims. Same shape as the stub-sentinel guards the downgrade probe tests use.
2 participants
Summary
crates/mtui-hosts/src/target/operation.rs:OperationGroup::last_output's doc called theNonefallback benign, butHostOutput::default()is exit 0, which every check reads as success — the host silently passes and stays in the reboot map. Doc now names the hazard, the use site logs a WARN naming the host, and aMockGrouptest pins the current fallback behaviour.Stringto a typedCheckFailure { reason, cancelled }(mirroringRebootFailure's shape), so a check that stops at a cancellation checkpoint can carry that fact acrossmtui-hostsintoUpdateError.cancelledinstead of being hardcoded tofalse.perform_uninstall_reports_a_transactional_host_that_never_reconnectsonly asserted the host name, so it passed for any reboot-failure cause. Brought it up to its install sibling's two discriminating assertions.plan_provider_resolves_the_role_specific_tableasserted the two doers merely differ, which a role swap also satisfies. Now compares each resolved doer against one built from that role's own table entry.Testing
cargo fmt --all --checkcargo clippy --workspace --all-targets -- -D warningsRUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps --all-features --document-private-itemscargo test --workspacecargo test -p mtui-mcp -F mcpcargo build --workspace --no-default-featuresand--all-featuresEach new/strengthened test was observed failing against the unfixed/unswapped code, then restored, before landing.
No CHANGELOG entry — no user-visible change.
closes #408