Skip to content

Prove the plan and approval journey publishes only an approved plan - #198

Merged
mberrys merged 2 commits into
devfrom
codex/issue-30-plan-approval-journey
Oct 5, 2026
Merged

mberrys merged 2 commits into
devfrom
codex/issue-30-plan-approval-journey

Conversation

@mberrys

@mberrys mberrys commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

What changed

Issue #30 wants the plan and approval journey presentable end to end: operation scope, parameters, candidate impact, preview, approver decision, terminal result. The journey itself is already built. ActionListPane.qml drives planActionList(), approveActionListPlan() and executeApprovedActionListPlan(), and EditorHost already binds an approval to the displayed plan digest, refuses a superseded one, and refuses to arm execution without that approval. What was missing is the acceptance proof the issue names: an end-to-end fixture over the governed gateway with the denied and stale cases, and an assertion on the terminal published and revalidated display.

This adds that fixture to UnitTestsEditorHost. It plans a bleed correction, asserts one plan digest on both the plan identity surface and the preview, refuses execution before any approval, honours an explicit rejection with nothing published, replaces the document revision and asserts the plan reports itself as no longer current with approval and execution both refused and nothing published, then plans and approves against the revision that is open and plans again: the run is refused until a fresh approval is given for the plan displayed at that moment, after which the correction runs to succeeded. Completion assertions bind the presentation to Core's facts: the recheck carries a verdict from Core's own closed set and never a failure, the sign-off carries the approved plan digest and a published digest of 64 lowercase hex characters distinct from the as-received input, the lifecycle summary reports a published result, and the as-received input is never the publication target.

Closes #30.

Per-criterion table:

Criterion State
Approval binds the displayed exact plan and input identity Already satisfied by fixReviewBindsToThePlannedDigestAndTheCurrentRevision; now also asserted across the preview and the identity surface, with reviewedPlanDigest equal to the digest displayed before approval, and refused after the plan is planned again on the same revision
Completion displays published and revalidated artifact Built here: the run reaches succeeded and the completion presentation is asserted against Core's verdict and the signed digests
Failure case: QML cannot call a mutation bypass or approve a changed plan Already satisfied at the C++ boundary, which is what QML can only reach: approveActionListPlan() requires Planned and fixPlanIsCurrent(), and executeApprovedActionListPlan() requires fixExecutionArmed(). Now asserted end to end after a real document replacement, after an explicit rejection, and after re-planning

Independent review

An independent reviewer was pointed at this fixture together with the product code it asserts on, on the ground that the same agent wrote both and the usual separation of duties was missing. It found that the fixture could still pass with two guards removed, and that the step described as denied never called rejectActionListPlan() at all. Each finding was confirmed against the source before anything changed: approveActionListPlan() gates on state == Planned && fixPlanIsCurrent(), so the old assertion could pass on the state check alone while the supersession clause was dead, and clearFixReview() runs at editorhost.cpp:3351 whenever a plan is accepted.

Mutation probe: each guard removed in turn, UnitTestsEditorHost rebuilt and the slot re-run, with the build exit code checked first so that a failed compile can never be read as a surviving guard.

Mutation Result
executeApprovedActionListPlan drops the fixExecutionArmed() check caught
fixPlanIsCurrent() ignores the document revision caught
fixPlannedPlanIsSuperseded() always returns false caught
clearFixReview() removed where a plan is accepted (editorhost.cpp:3351) caught
fixLifecycleStateName() digest equality forced true (:680) survives, unreachable
approveActionListPlan drops the !fixPlanIsCurrent() clause (:2164) survives, unreachable

The two survivors are defensive clauses rather than coverage. Accepting a plan clears the review and a revision change clears it again, so no state reachable through the public API holds an approved review whose digest differs from the current plan's, and this journey offers no path to a Planned controller for a superseded revision. Both are named as such in the fixture comment, the changelog fragment and the commit message instead of being counted as proven.

Release changelog

Topic PR: changes/codex-issue-30-plan-approval-journey.md (Category added, Audience operators, Breaking-Change no).

Proof

  • python scripts/agent/check-change.py --base origin/dev --build-dir build-local reports pass. Run as LOOP_SRC=<worktree> bash $TEMP/loop-build-parity.sh python scripts/agent/check-change.py --base origin/dev --head-branch codex/issue-30-plan-approval-journey --build-dir C:/.dev/repos/loop-build-parity --report $TEMP/issue30-check-change.json. Reported status: pass, head 650d982e, modules quick, risk standard, 25 checks with none non-pass, including the builds of LoopEditor, LoopLibQuick and ProductQuickAccessibilitySmoke, the builds of all ten quick test targets, and focused_tests, which runs the whole UnitTestsEditorHost target rather than only this slot. CI's agent-fast job remains the authoritative gate.
  • Rerun caveat for this host, because the same gate reports red on correct code through the repository's own wrapper. loop-check-change.sh leaves the Windows SDK off PATH, so every link step dies with LINK : fatal error LNK1158: cannot run 'rc.exe' and each affected target is reported as fail exit code 4294967295. The run above used a wrapper with the full toolchain on PATH.
  • One changes/<sanitized-head-branch>.md fragment added (Category, Audience, Breaking-Change, Summary)
  • Changed behaviour has a test that fails without the change: this change adds no behaviour, since the fixture is a guard over behaviour that already ships and is therefore green on its first run by construction. Its falsifiability is measured rather than asserted: four guard removals make it fail, listed in the mutation table above, and the two that do not are named as unreachable rather than counted.
  • Protected-path or contract change named above, with the reason it is required: none. No protected path, Core contract, schema or production surface is touched.

Validation on source SHA 650d982e:

  • UnitTestsEditorHost fixJourneyPublishesOnlyAnApprovedPlanBoundToTheDisplayedIdentity: passed, 0 failures, 7.95 s.
  • Whole UnitTestsEditorHost target inside the change-set gate's focused_tests: passed.
  • python scripts/agent/check-architecture.py --base origin/dev --head-branch codex/issue-30-plan-approval-journey: architecture contracts ok.
  • Mutation probe over six guards: four caught, two unreachable, as tabulated above.

Commands (temporary build tree, Qt and MSVC through the wrapper that puts the Windows SDK on PATH):

LOOP_SRC=<worktree> bash $TEMP/loop-build-parity.sh cmake --build C:/.dev/repos/loop-build-parity --target UnitTestsEditorHost -j 5
LOOP_SRC=<worktree> bash $TEMP/loop-build-parity.sh C:/.dev/repos/loop-build-parity/usr/bin/UnitTestsEditorHost.exe fixJourneyPublishesOnlyAnApprovedPlanBoundToTheDisplayedIdentity -o $TEMP/i30-baseline.xml,junitxml
LOOP_SRC=<worktree> bash $TEMP/loop-build-parity.sh ctest --test-dir C:/.dev/repos/loop-build-parity -R '^UnitTestsEditorHost$' --output-on-failure
python scripts/agent/check-architecture.py --base origin/dev --head-branch codex/issue-30-plan-approval-journey
bash $TEMP/i30-probe.sh   # the mutation probe: writes $TEMP/i30-probe2.log, reverts each mutation, and withholds its verdict when a build fails

Limitations. The run presents the published bytes' digest and does not materialize them as a file, so this fixture asserts the presentation rather than a filesystem write; freeing the published artifact to disk belongs to the export slice. The QML pane is exercised through the host invokables it binds to, not through a QML harness, so a QML-only regression that bypassed those invokables would not be caught here. The digest equality in fixLifecycleStateName() and the fixPlanIsCurrent() clause in approveActionListPlan() are defensive against a state this journey offers no path to, and are recorded as such rather than claimed as coverage. Only the Windows build was exercised; the Linux lane reports through CI.

Internal logic (touched behavior-bearing code)

  • Guard clauses handle invalid, stale, cancelled, absent, unauthorized, and terminal cases before the happy path. The fixture asserts the refusals before the happy path: unapproved execution, an explicit rejection, approval after a revision change, and execution after a revision change and after a re-plan, each with the as-received input digest unchanged.
  • Untrusted input is parsed once at the boundary into trusted typed or domain state. Test-only change; it reads the host's typed presentation maps.
  • Invalid state stops before partial mutation or publication and returns a descriptive error or result. The fixture asserts exactly that with !fixExecutionArmed(), the refused invokables, and recheck.available == false.
  • Names carry the domain intent, and comments explain rationale rather than restating the code.

Quality pass

  • Redundant or explanatory comments that do not match the file's style removed
  • Abnormal defensive checks and broad try/catch blocks removed where a trusted upstream boundary already guarantees the invariant, with real boundary and safety checks kept. An earlier draft scanned the run directory for a file matching the published digest; the diagnostic showed the run never writes one, so the scan was replaced by the digest assertions it was actually trying to make.
  • No any or equivalent cast added only to suppress a type error
  • Python imports stay at file scope unless a local import is required (no Python changed)
  • Generated boilerplate, needless wrappers, and local-style drift removed
  • Validation, security, cancellation, provenance, and failure handling preserved

Quality summary (1-3 sentences): The diff is one fixture in an existing mapped target plus the two changes/ files, 176 insertions and no deletions. What was removed after its own failure output said so is a filesystem assertion the platform does not satisfy; what was added after independent review is the rejection step, the re-plan refusal and the explicit not-current assertion, each of which a removed guard now makes fail. What was kept on purpose is the digest comparison against the as-received input, because that is the assertion that tells a real publication apart from a no-op.

Security and rollback

  • Untrusted input validated at the trust boundary; no new unsafe construct without an inline justification. Test-only; the fixture drives the existing governed gateway and adds no new input path.
  • Rollback: revert this commit. It adds one test fixture and two changes/ files.

Docs

  • Docs updated in this PR, or "none needed" with the reason: none needed. docs/GOVERNED_EXECUTION.md already documents plan digest, previews, approval and the publish gate, and this fixture asserts the presentation against those facts without changing them.

Self-review (BSP-002 §4.3)

  • Reviewed in the diff view, not the editor, at least 30 minutes after the final commit; overnight if the change touches security-sensitive code, data handling, or public API surface. Reviewed in the diff view after an independent review round; the 30-minute separation is not yet met for this amended commit and the review is repeated before the branch is proposed for merge.

Add the acceptance fixture for issue #30: an unapproved plan, a rejected plan, a plan whose document revision has moved, and a plan replanned after approval each neither execute nor publish anything; a run requires an approval given for the plan displayed at that moment; and completion presents the revalidated verdict with the 64-hex digest of the artifact it published, distinct from the as-received input.

The fixture is mutation-probed: removing the execution arming check, the revision comparison, the superseded predicate or the review clearing each makes it fail. The reviewed-digest equality in fixLifecycleStateName() and the fixPlanIsCurrent() clause in approveActionListPlan() are defensive against a state the journey offers no path to, and are documented as such rather than claimed as coverage.
@mberrys
mberrys force-pushed the codex/issue-30-plan-approval-journey branch from 8d6907b to 650d982 Compare October 4, 2026 08:01
@mberrys
mberrys merged commit ceb4b22 into dev Oct 5, 2026
16 checks passed
@mberrys
mberrys deleted the codex/issue-30-plan-approval-journey branch October 5, 2026 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant