fix: a task must not report converged without reaching its declared state - #255
Merged
Conversation
…tate (Refs #254) `completion_check` was consulted only as a guard on whether to RUN the command, never re-evaluated afterwards. So `converged` meant "the command exited 0", not "the resource is in the state it declares" — and the lock recorded that, so the next `plan` reported `no changes` over a host that had never converged. Found in production, not by inspection. paiml/infra's `lean-toolchain` used `sudo: true`, which made $HOME=/root, so the Lean toolchain installed where the runner user could not read it: $ forjar apply -f forjar.yaml -t proof-toolchain --yes intel: 1 converged, 3 unchanged, 0 failed $ ssh intel 'command -v lean' ; echo $? 1 Every command in that script succeeded. The purpose was not achieved. Nothing noticed. The fix re-asserts the check after the command and exits non-zero when it still fails, with a message that distinguishes the two failures which previously looked identical — except that the second looked like a success: the command errored -> already reported as failure the command ran and achieved nothing -> reported as CONVERGED The check is already written and already cheap; it just ran. Running it once more turns an exit code into a statement about the world. tests/falsification_task_verifies_completion.rs, 5 cases, all EXECUTING the emitted script under bash rather than pattern-matching it — a `script.contains("completion_check")` assertion would pass on a script that never runs the check, which is the class of test that let this through. Beyond the regression itself, the tests pin the properties a careless fix would break: a genuinely converging task still passes (or the gate trains people to delete it); a failing command is still reported as a COMMAND failure, not relabelled as a convergence problem; a task with no completion_check is untouched; and the check runs AFTER the command, since evaluating it first would pass vacuously for any task whose condition already held — which is the guard semantics being replaced. Mutation-checked: reverting the fix fails `a_command_that_succeeds_without_converging_fails` and leaves the other four green, so the regression test isolates the defect rather than the suite detecting it incidentally. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #254
The defect
completion_checkwas consulted only as a guard on whether to RUN the command, never re-evaluated afterwards. Soconvergedmeant "the command exited 0", not "the resource is in the state it declares" — and the lock recorded that, so the nextplanreportedno changesover a host that had never converged.Found in production, not by inspection. paiml/infra's
lean-toolchainusedsudo: true, which made$HOME=/root, so the Lean toolchain installed where the runner user could not read it:Every command in that script succeeded. The purpose was not achieved. Nothing noticed.
The fix
Re-assert the check after the command, and exit non-zero when it still fails — with a message that distinguishes two failures which previously looked identical, except that the second looked like a success:
task=not-convergedThe check is already written and already cheap; it just ran. Running it once more turns an exit code into a statement about the world.
Tests
tests/falsification_task_verifies_completion.rs, 5 cases, all executing the emitted script under bash rather than pattern-matching it. Ascript.contains("completion_check")assertion would pass on a script that never runs the check — the class of test that let this through in the first place.Beyond the regression, they pin what a careless fix would break:
completion_checkis untouched; this must not become a way to fail tasks that never made the claimMutation-checked: reverting the fix fails
a_command_that_succeeds_without_converging_failsand leaves the other four green, so the regression test isolates the defect rather than the suite catching it incidentally.Note on CI
container_transportfails on this branch withConflict. The container name "/forjar-integration-test" is already in use. That is not from this change — it is the shared-fixture collision #250 fixes withtest_machine_named(), andmaindoes not have that fix yet (git show main:tests/container_transport.rs | grep -c test_machine_named-> 0). Everything else passes: 12,994 tests. It clears once #250 lands.Behaviour change
Tasks that were silently not converging will now fail. That is the point — they were never converged, and the lock was recording otherwise.