feat: add canonical execution outcomes - #324
Merged
Merged
Conversation
…tions The installer-managed migration CI gates hardcoded the migration ledger's "latest" state as exactly 29 rows ending at migration 0028's timestamp. That coincidentally matched main, but this PR adds migration 0029, so the ledger is now 30 rows and those hardcoded assertions reject a fully-successful migration. Also update repair-epic-172-legacy-release.ts: its checkpoint classifier only recognized ledgers of exactly 27/28/29 rows (positions 0026/0027/0028). Once later migrations exist, a fully-migrated database no longer matches any of those, so the idempotent installer re-run wrongly refuses the epic-172 legacy-release repair. The 0028 checkpoint now accepts ledgers of 29 rows or more, validating only that the first 29 entries are the known-good 0000-0028 ledger; later migrations are outside this script's scope and are governed by their own migration files and the CI journal-parity gate.
Two more hardcoded main-era assumptions break once migration 0029 exists, both downstream of the gate fixed in the previous commit so CI never reached them yet: - migration-0027-expansion-assertions.sql pinned the post-0028 ledger to count 29 / max timestamp 1784274000000. apply-epic-172-s5-recovery- migration.sh actually migrates to the true latest (db:migrate, unpinned), so with 0029 present this needs to become 30 / 1785820800000. - The closed application-ACL inventory check in web-ci.yml enumerates every public table by name; execution_outcomes wasn't in any of its three arrays, so the ordinary-app boundary proof would reject it as an unaccounted-for table. Add it as its own operation_ledger_tables entry with the SELECT/INSERT/UPDATE privileges the upsert-only worker path actually uses (db/worker/execution-outcomes.ts has no DELETE).
This test reads scripts/ci/sql/migration-0027-expansion-assertions.sql as text and asserts it still contains specific hardcoded evidence strings, including the pre-0029 migration count. It wasn't updated when that SQL file's asserted count changed from 29 to 30 in the previous commit, so the zero-skip unit suite failed even though the SQL file itself was correct.
…path
Orthogonal review finding: the three upsertExecutionOutcome call sites
(admission block, run completion, run failure) were unguarded, unlike
every other non-critical write in this file (publishTaskEventBestEffort,
recordTaskLogBestEffort). A ledger write failure would abort completion
before the work package is marked complete/awaiting-review, or abort
failure recording before run:failed/artifact:created are published and
before the failure is logged -- even though the underlying artifact/status
writes had already durably succeeded. The ledger is idempotent on
(taskId, attemptKey), so a missed write is safe to retry or backfill
later; it should never block state that's already been persisted.
Also fixes a stale CI-script echo string ("...at the 29-row ledger") left
over from an earlier fix in this PR; the proof it labels runs at the fixed
0028 checkpoint regardless of how many later migrations exist.
…block kinds Orthogonal review finding: the outcome-ledger writes in persistWorkPackageHandoffBlock were only mocked, never asserted, so the new kind -> stopReasonCode mapping (broker -> admission_denied, reserved_role -> policy_blocked, filesystem_grant -> missing_capability) and the terminalBlock -> retryable inversion were unverified by CI. Add one targeted assertion per existing test that already exercises each kind. The other two upsertExecutionOutcome call sites, in executeReadyWorkPackage's completion and failure branches, are not covered here: that function is currently unreachable in both production and this test file because isWorkPackageExecutionEnabled() is hardcoded to always return false, so no test can exercise it without restructuring the module's mocking boundary.
Joncallim
marked this pull request as ready for review
August 7, 2026 07:14
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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
2 participants
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.
Summary
execution_outcomesledger and migrationWhy
Forge previously mixed transport state, semantic results, and free-text errors, making execution evidence unsafe to compare. This introduces the shared outcome boundary required by #185.
Validation
cd web && npm test -- --run __tests__/execution-outcomes.test.ts __tests__/work-package-handoff-db.test.tsgit diff --checkKnown baseline limits
npx tsc --noEmitremains blocked by pre-existing malformed or missing implicit type packages.