PR 007 — Autoflow State Machine: commit-bound, pure orchestration state - #9
PR 007 — Autoflow State Machine: commit-bound, pure orchestration state#9LogicDuke wants to merge 18 commits into
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPR 007 adds the Autoflow immutable workflow state machine. It defines commit-bound state and events, pure transitions, bounded readers, public exports, architecture rules, hostile-input handling, and lifecycle and invariant tests. ChangesAutoflow workflow state machine
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant openWorkflow
participant applyWorkflowEvent
participant WorkflowState
Caller->>openWorkflow: WorkflowBinding
openWorkflow-->>Caller: revision-zero WorkflowState
Caller->>applyWorkflowEvent: WorkflowEvent and WorkflowState
applyWorkflowEvent->>WorkflowState: validate bindings and invariants
applyWorkflowEvent-->>Caller: applied or rejected TransitionResult
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7cfbfc95c6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (readOwnProperty(verdictRecord, 'targetRepositoryId') !== snapshot.repositoryId) { | ||
| append(notCurrent, 'verdict.targetRepositoryId'); | ||
| } | ||
| if (readOwnProperty(verdictRecord, 'targetHeadSha') !== snapshot.boundCommitSha) { | ||
| append(notCurrent, 'verdict.targetHeadSha'); |
There was a problem hiding this comment.
When a corrupted or caller-forged EvidenceFreshness claims CURRENT and supplies matching target fields but retains a different repositoryId or commitSha, these checks admit it as current because the evidence's own binding fields are never compared. This lets stale or cross-repository evidence enter the current revision, and a forged human-decision can additionally clear an open human gate; validate the verdict's repositoryId and commitSha against the workflow binding as well.
Useful? React with 👍 / 👎.
| const invocations: TrackedInvocation[] = []; | ||
| for (let index = 0; index < invocationCandidates.length; index += 1) { | ||
| const tracked = readTrackedInvocation(invocationCandidates[index], revision, sequence); | ||
| if (tracked === null) { | ||
| return null; | ||
| } | ||
| append(invocations, tracked); |
There was a problem hiding this comment.
When a deserialized or cast WorkflowState contains two tracked entries with the same invocationId, this snapshot loop accepts both even though invocation identity is workflow-wide. Reporting that ID then updates only the first entry found by indexOfInvocation; subsequent reports are rejected as already reported while the duplicate remains permanently REQUESTED, making behavior depend on array order. Detect duplicate IDs here and reject the aggregate as WORKFLOW_UNREADABLE.
Useful? React with 👍 / 👎.
| const atCommitSha = readExactIdentifier(readOwnProperty(eventRecord, 'atCommitSha')); | ||
| if (atCommitSha === null) { | ||
| return rejected(original, TRANSITION_REJECTION.EVENT_PAYLOAD_INVALID, [ | ||
| 'event.atCommitSha', | ||
| ]); | ||
| } |
There was a problem hiding this comment.
When HUMAN_GATE_OPENED is submitted while the workflow is already awaiting a human and atCommitSha is malformed, this early validation returns EVENT_PAYLOAD_INVALID instead of HUMAN_GATE_ALREADY_OPEN. That contradicts the documented fixed precedence of status posture before deep payload fields and can send callers down the wrong recovery path; perform the already-open check before reading and validating atCommitSha.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex please implement the three CURRENT findings on PR #9 only.
Current reviewed HEAD:
7cfbfc9
Treat every finding on an older SHA as stale after HEAD changes.
Repair exactly:
P1 — Evidence own-binding validation
A pre-normalized/forged EvidenceFreshness must not be admitted merely because its target fields say CURRENT for this workflow.
Validate the evidence/verdict's own repository and commit binding against the workflow binding as required by the frozen PR 007 trust boundary.
A forged human-decision must never clear a human gate when its own evidence binding is stale/cross-repository.
P2 — Duplicate invocation IDs in deserialized state
While reading/validating a supplied WorkflowState, duplicate tracked invocationIds must make the aggregate WORKFLOW_UNREADABLE.
Do not allow order-dependent report behavior.
P2 — HUMAN_GATE_OPENED rejection precedence
Preserve the documented fixed precedence.
If the workflow is already AWAITING_HUMAN_DECISION, return HUMAN_GATE_ALREADY_OPEN before deep-reading/validating atCommitSha.
Preserve D1–D7 and A1–A3 exactly.
Add focused adversarial regression tests for all three repairs.
Do not redesign PR 007.
Do not add PR 008/009 functionality.
Do not refactor unrelated hardened readers.
Do not make unrelated cleanup.
Run the complete verification suite and npm audit.
Before commit/push, audit the repair for:
evidence own-binding exactness;
old-SHA/cross-repository evidence rejection;
human-decision gate safety;
duplicate invocation identity rejection;
deterministic rejection precedence;
provider/purpose/reported-status neutrality;
claim/evidence separation;
revision/sequence semantics;
no PR 008/009 scope creep;
all existing tests green.
If direct update of the current PR branch is unavailable, create a child repair PR targeting pr-007/autoflow-state-machine, not main.
Do not merge anything.
There was a problem hiding this comment.
Summary
- Hardened evidence admission so a
CURRENTverdict must match the workflow through both the evidence’s own repository/commit fields and its target repository/head fields. src/domain/workflow-transitions.tsL1131-L1156 - Prevented forged cross-repository human decisions from clearing an open human gate, with adversarial coverage for stale repository and commit bindings. tests/domain/workflow-transitions.test.tsL619-L647
- Made duplicate tracked invocation IDs render deserialized workflow state unreadable, eliminating order-dependent reporting behavior. src/domain/workflow-transitions.tsL470-L482 tests/domain/workflow-transitions.test.tsL357-L367
- Restored fixed rejection precedence for
HUMAN_GATE_OPENED: an already-open gate now returnsHUMAN_GATE_ALREADY_OPENbefore readingatCommitSha. src/domain/workflow-transitions.tsL1269-L1288 tests/domain/workflow-transitions.test.tsL1050-L1063 - Updated the frozen PR 007 architecture record to explicitly document evidence own-binding validation. docs/architecture/007-autoflow-state-machine.mdL234-L248
- Committed the repair as
24f40da(Fix workflow trust-boundary validation) and prepared child-repair PR metadata targetingpr-007/autoflow-state-machine.
Testing
- ✅
npm run verify— typecheck, lint, all 1,125 tests, and build passed. - ✅
git diff --check ⚠️ npm audit— attempted as requested, but the npm registry audit endpoint returned HTTP 403 in this environment.
|
@coderabbitai review |
|
|
@coderabbitai review |
|
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@codex please perform a fresh FINAL REVIEW of PR #9 at the CURRENT HEAD. Determine the exact current HEAD SHA yourself before reviewing. Treat every finding and review attached to an older SHA as stale. REVIEW ONLY. Do not implement repairs. Review the complete PR 007 implementation, including the Codex repair and subsequent documentation-only precedence clarification. Re-check the frozen PR 007 architecture D1–D7 and A1–A3, with particular attention to: evidence own repository/commit binding; Report findings only against the exact current HEAD. If no findings remain, explicitly state that the current HEAD is clean and ready for merge. Do not merge anything. |
|
@coderabbitai review |
|
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@coderabbitai full review |
|
Your plan includes PR reviews subject to rate limits. More reviews will be available in 53 minutes. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a22eb520b9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (let index = 0; index < length; index += 1) { | ||
| let element: unknown; | ||
| try { | ||
| element = elements[index]; |
There was a problem hiding this comment.
When a deserialized state contains a sparse invocations, evidence, or reviews array and Array.prototype has a corresponding numeric property, elements[index] reads that inherited value and materializes it as an own workflow record. A prototype-planted invocation can therefore be reported, and prototype-planted evidence or reviews become durable after the next applied event, contradicting the hostile-runtime and own-input guarantees. Require every index below length to be an own property, or reject the state as WORKFLOW_UNREADABLE.
Useful? React with 👍 / 👎.
|
|
||
| const evidence: AdmittedEvidence[] = []; | ||
| for (let index = 0; index < evidenceCandidates.length; index += 1) { | ||
| const admitted = readAdmittedEvidence(evidenceCandidates[index], revision, sequence); |
There was a problem hiding this comment.
When a deserialized or cast state contains an evidence admission whose admittedAtRevision equals the workflow's current revision but whose admittedAtCommitSha differs from boundCommitSha, this reader accepts and preserves it. Because currentness is keyed by revision, the stale or cross-commit record then appears to be a current admission and can also cause a legitimate admission with the same ID to be rejected as a duplicate; the equivalent problem exists for reviews. Reject such an internally impossible aggregate as WORKFLOW_UNREADABLE.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex please implement the two CURRENT P1 findings on PR #9 only.
Determine the exact current PR HEAD yourself before modifying anything.
Treat all findings on older SHAs as stale after HEAD changes.
Repair exactly these two findings:
P1 — Reject inherited entries in workflow lists
When reading/deserializing invocations, evidence, and reviews, every numeric index from 0 through length - 1 must be an own property of the array.
A sparse slot that resolves through Array.prototype[index] must never be accepted or materialized into workflow state.
Fail closed as WORKFLOW_UNREADABLE.
Apply this consistently to all security-relevant workflow arrays.
Add adversarial tests proving prototype-planted numeric properties cannot:
fabricate a tracked invocation;
fabricate admitted evidence;
fabricate an admitted review;
become durable after a subsequent legitimate transition.
P1 — Verify current admissions against bound commit
When reading a supplied/deserialized WorkflowState, enforce the internal invariant:
if an AdmittedEvidence or AdmittedReview has
admittedAtRevision === workflow.revision,
then
admittedAtCommitSha === workflow.boundCommitSha
must also hold exactly.
Otherwise reject the aggregate as WORKFLOW_UNREADABLE.
Do not alter the historical-admission rule: admissions from older revisions may legitimately retain their earlier admittedAtCommitSha.
Add adversarial tests for both evidence and reviews proving:
current-revision mismatched commit → WORKFLOW_UNREADABLE;
current-revision matching commit → accepted;
older-revision historical admission with prior commit → still accepted;
an impossible forged current admission cannot block or shadow a later legitimate admission by ID.
Preserve D1–D7 and A1–A3 exactly.
Do not redesign PR 007.
Do not add PR 008/009 functionality.
Do not refactor unrelated hardened readers.
Do not make unrelated cleanup.
Run the complete verification suite, npm audit, and git diff --check.
Before commit/push, audit specifically for:
inherited numeric properties;
sparse arrays;
Array.prototype poisoning;
own-property enforcement;
current revision/commit consistency;
historical admission validity;
stale evidence/review rejection;
revision vs sequence semantics;
claim/evidence separation;
provider/purpose/reportedStatus neutrality;
no PR 008/009 scope creep.
If direct update of the existing PR branch is unavailable, create a child repair PR targeting pr-007/autoflow-state-machine, not main.
Do not merge anything.
There was a problem hiding this comment.
Summary
- Confirmed the exact starting PR HEAD was
a22eb520b96615f832e17ba5eb68d3ed326ee416before making changes. - Hardened all three workflow-state list readers—invocations, evidence, and reviews—by requiring every numeric array index to be an own property. Sparse arrays resolving entries through
Array.prototypenow fail closed asWORKFLOW_UNREADABLE. src/domain/workflow-transitions.tsL183-L218 - Added current-admission consistency checks for both evidence and reviews: an admission at the workflow’s current revision must have an
admittedAtCommitShaexactly matchingboundCommitSha, while historical admissions remain valid. src/domain/workflow-transitions.tsL495-L519 - Added adversarial tests proving prototype-planted invocations, evidence, and reviews cannot be materialized or become durable through a later transition. tests/domain/workflow-invariants.test.tsL515-L547
- Added evidence and review regression coverage for mismatched current commits, matching current commits, valid historical admissions, and forged same-ID admissions failing as
WORKFLOW_UNREADABLEbefore they can shadow legitimate admissions. tests/domain/workflow-invariants.test.tsL549-L597 - Committed the repair as
067b69a(Harden workflow state admission readers) and prepared child-repair PR metadata targetingpr-007/autoflow-state-machine.
Testing
- ✅
npm test -- --run tests/domain/workflow-invariants.test.ts— 213 focused invariant tests passed. - ✅
npm run verify— typecheck, lint, all 1,130 tests, and build passed. - ✅
git diff --check ⚠️ npm audit— the npm registry audit endpoint returned HTTP 403 in this environment.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@codex please perform a fresh FINAL REVIEW of PR #9 at the CURRENT HEAD. Determine the exact current HEAD SHA yourself before reviewing. Treat every finding and review attached to an older SHA as stale. REVIEW ONLY. Do not modify code. Review the complete current PR 007 implementation, including all previous repairs. Focus especially on: current-revision invocation commit binding; Review tests critically. Do not assume 1136 green tests prove correctness. Report findings only against the exact current HEAD. If no material findings remain, explicitly state that the CURRENT HEAD is clean and ready for merge. |
|
@coderabbitai review |
|
|
@coderabbitai rate limit |
|
Your plan includes PR reviews subject to rate limits. More reviews will be available in 1 minute. |
|
@coderabbitai review |
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f570ec67e2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const gateOpen = snapshot.status === WORKFLOW_STATUS.AWAITING_HUMAN_DECISION; | ||
| const clearing = gateOpen && rawKind === EVIDENCE_KIND.HUMAN_DECISION; |
There was a problem hiding this comment.
When a cast or deserialized verdict has matching state/reason/bindings and kind: 'human-decision' but an impossible PR 004 shape such as source: null or nonempty invalidFields, it is still admitted here and clears an open human gate. Since every genuine CURRENT EvidenceFreshness has a valid source and empty invalid fields, validate those invariants before treating the kind as a recorded human decision.
Useful? React with 👍 / 👎.
| if ( | ||
| tracked === null || | ||
| (tracked.requestedAtRevision === revision && | ||
| tracked.targetCommitSha !== boundCommitSha) | ||
| ) { |
There was a problem hiding this comment.
After HEAD_OBSERVED advances the revision, these current-only checks accept historical invocations, evidence, and reviews stamped with the same prior revision but different commit SHAs. No possible workflow history can bind one revision to multiple commits, yet the next applied event freezes and preserves that corrupted audit history; validate a single consistent commit binding for every represented revision, including historical ones.
Useful? React with 👍 / 👎.
| const revision = readCount(readOwnProperty(record, 'revision'), WORKFLOW_BOUNDS.MAX_REVISION); | ||
| const sequence = readCount(readOwnProperty(record, 'sequence'), WORKFLOW_BOUNDS.MAX_SEQUENCE); |
There was a problem hiding this comment.
A deserialized state with revision: 2 and sequence: 0 passes this snapshot and can be closed into a state with revision 2 and sequence 1. Because every revision increment is itself an applied transition and every applied transition increments sequence, revision > sequence is unreachable and corrupts both the documented total ordering and later capacity behavior; reject such aggregates as WORKFLOW_UNREADABLE.
Useful? React with 👍 / 👎.
| const evidence: AdmittedEvidence[] = []; | ||
| for (let index = 0; index < evidenceCandidates.length; index += 1) { | ||
| const admitted = readAdmittedEvidence(evidenceCandidates[index], revision, sequence); | ||
| if ( | ||
| admitted === null || | ||
| (admitted.admittedAtRevision === revision && | ||
| admitted.admittedAtCommitSha !== boundCommitSha) | ||
| ) { | ||
| return null; | ||
| } | ||
| append(evidence, admitted); |
There was a problem hiding this comment.
When a cast or deserialized state already contains two evidence entries with the same (evidenceId, admittedAtRevision)—or the equivalent review pair—this reader accepts both even though the transition handlers prohibit that replay identity. A later event then makes the duplicates durable and lets them consume capacity or be double-counted by downstream policy; detect duplicate admission keys while snapshotting both collections.
Useful? React with 👍 / 👎.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
@coderabbitai rate limit |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
tests/domain/workflow-invariants.test.ts (1)
2085-2108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the purity scan token-aware instead of substring-based.
toContainmatches any substring.DatematchesupdateDateorValidate.processmatchesprocessed.cryptomatchescryptographic. A future rename insideworkflow.tsorworkflow-transitions.tscan fail this test without introducing impurity.stripCommentsalso removes/* */and//sequences that appear inside string literals or regular expressions, so the scanned text can differ from real code.Match whole tokens with word boundaries, and keep the member-access patterns explicit.
♻️ Proposed refactor
- for (const forbidden of [ - 'Date', - 'Math.random', - 'process', - 'globalThis', - 'require(', - 'node:', - 'async ', - 'await ', - 'Promise', - 'setTimeout', - 'crypto', - 'randomUUID', - ]) { - expect(source).not.toContain(forbidden); - } + for (const forbidden of [ + /\bDate\b/, + /\bMath\s*\.\s*random\b/, + /\bprocess\b/, + /\bglobalThis\b/, + /\brequire\s*\(/, + /\bfrom\s+['"]node:/, + /\basync\b/, + /\bawait\b/, + /\bPromise\b/, + /\bsetTimeout\b/, + /\bcrypto\b/, + /\brandomUUID\b/, + ]) { + expect(source).not.toMatch(forbidden); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/domain/workflow-invariants.test.ts` around lines 2085 - 2108, Update the purity scan in the test case reading workflow.ts and workflow-transitions.ts to use token-aware regular-expression matching instead of substring toContain checks. Match standalone identifiers with word boundaries, while keeping member-access patterns such as Math.random explicit, and avoid stripping comment-like sequences inside string literals or regular expressions when preparing source for scanning.tests/domain/workflow-fixtures.ts (1)
48-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider sharing
oversizedandlabelwith the existing invocation fixtures.
oversizedandlabelare byte-identical tooversizedandlabelintests/domain/invocation-fixtures.ts(lines 33-35 and 43-69). The production readers are duplicated on purpose for boundary independence, andtests/domain/reader-parity.test.tspins that duplication. Test helpers carry no such requirement, so a shared helper module would remove the copy without weakening any guarantee.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/domain/workflow-fixtures.ts` around lines 48 - 85, The oversized and label helpers are duplicated across workflow and invocation fixtures. Move them into a shared test-helper module, update both fixture modules to import and reuse those shared exports, and remove their local implementations while leaving the production reader duplication unchanged.src/domain/workflow-transitions.ts (3)
978-1002: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winFail closed instead of skipping an
undefinedelement.
appendToandreplaceAtskip an element whenelement === undefined. The guard exists to satisfynoUncheckedIndexedAccess, and the input lists come from a validated snapshot, soundefinedis not reachable today. The behaviour still contradicts the rule this module states on line 382: the layer refuses to shorten orchestration history silently. If a future change makes a hole reachable, the copy shortens without any signal.Copy the element unconditionally, so the type guard cannot become a silent drop.
♻️ Proposed change
function appendTo<T>(list: readonly T[], value: T): readonly T[] { const next: T[] = []; for (let index = 0; index < list.length; index += 1) { - const element = list[index]; - if (element !== undefined) { - append(next, element); - } + append(next, list[index] as T); } append(next, value); return objectFreeze(next); } function replaceAt<T>(list: readonly T[], target: number, value: T): readonly T[] { const next: T[] = []; for (let index = 0; index < list.length; index += 1) { - const element = list[index]; if (index === target) { append(next, value); - } else if (element !== undefined) { - append(next, element); + } else { + append(next, list[index] as T); } } return objectFreeze(next); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/workflow-transitions.ts` around lines 978 - 1002, Update appendTo and replaceAt to preserve every list position by copying each indexed element unconditionally instead of skipping undefined values; retain replaceAt’s substitution behavior for the target index and keep the existing immutable return behavior.
693-957: 🚀 Performance & Scalability | 🔵 TrivialNote the per-call validation cost as history grows.
snapshotWorkflowruns full re-validation on everyapplyWorkflowEventcall, including calls that are rejected immediately afterwards, such as theWORKFLOW_CLOSEDcheck on line 1156. Several passes are quadratic in the retained history size:
- the evidence duplicate scan at lines 808-817 is O(n²) with n up to
MAX_ADMITTED_EVIDENCE(1 024);claimSequencerescansseenSequencesfor each stamp, and that array can reach ~1 792 entries;revisionBandsOrderedis O(d²) over distinct revisions.At the declared bounds this is a few million primitive comparisons per call, and a caller that replays an N-event log pays it N times. The bounds keep it finite and deterministic, so this is not a correctness or availability defect. If a persistence layer later replays long logs, consider validating once per loaded state and passing the validated snapshot forward, rather than re-validating per event.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/workflow-transitions.ts` around lines 693 - 957, Reduce repeated validation in the applyWorkflowEvent flow by validating a loaded workflow state once and passing the validated snapshot forward instead of invoking snapshotWorkflow on every event, including immediately rejected events such as WORKFLOW_CLOSED. Preserve all existing snapshotWorkflow invariants and ensure event application still receives validated state.
1066-1078: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrive the invalid-field order from
WORKFLOW_BINDING_FIELD_ORDER, or correct its doc comment.
src/domain/workflow.tslines 357-362 state that invalid-field reporting walksWORKFLOW_BINDING_FIELD_ORDER, so the order ofinvalidFieldsis deterministic and stable. This function instead hardcodes the order in four separateappendcalls, andWORKFLOW_BINDING_FIELD_ORDERis not read by any production path. The constant is still exported publicly fromsrc/domain/index.tsline 168.The order currently matches, and
tests/domain/workflow-transitions.test.tslines 174-188 pin it, so there is no live defect. Either read the order from the constant here, or update the constant's doc comment so it does not describe behaviour the code does not implement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/workflow-transitions.ts` around lines 1066 - 1078, Update the invalid-field reporting around the invalid array construction to either iterate according to WORKFLOW_BINDING_FIELD_ORDER, or revise that constant’s public documentation in workflow.ts to stop claiming it governs invalidFields ordering. Prefer using the exported constant so the implementation and documented deterministic order remain aligned, while preserving the existing validation conditions and field names.tests/domain/reader-parity.test.ts (1)
405-414: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the test body with its name.
The test is named
pins PR 007 as having no field that truncates, but the first two assertions exerciseingestInvocationReport, which is the PR 006 boundary. The local variableworkflowholds anInvocationReportResult, not a workflow value. The only PR 007 assertion on line 414 repeats the check already made on lines 296 and 400.Consider asserting the PR 007 no-truncation property directly, for example that an oversized identifier never appears as a prefix in a
openWorkfloworapplyWorkflowEventresult.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/domain/reader-parity.test.ts` around lines 405 - 414, The test body does not directly verify PR 007’s no-truncation behavior. In the test named “pins PR 007 as having no field that truncates,” remove the unrelated ingestInvocationReport assertions and replace the duplicated workflowReadExactIdentifier check with an assertion using openWorkflow or applyWorkflowEvent that confirms an oversized identifier is never retained as a truncated prefix.tests/domain/workflow-transitions.test.ts (3)
878-886: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the finding-count assertion less brittle.
expect(serialized).not.toContain('25')guards against a stored finding count. The state also serializessequence,revision, and every identifier, so any future fixture value containing the substring "25" makes this test fail for an unrelated reason. The current fixtures avoid it, so the test passes today.Consider asserting on the absent key instead of the raw count value, for example checking that no review admission object has more than the four expected keys.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/domain/workflow-transitions.test.ts` around lines 878 - 886, Replace the brittle serialized-value check in the workflow transition test with a structural assertion that review admission objects do not contain the stored finding-count field, such as verifying they contain only the four expected keys. Keep the existing checks for review count and excluded sentinel fields unchanged.
192-219: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the unused fourth tuple column.
The
casestuple type declares a fourth elementstring | null, every row sets it tonull, and theit.eachcallback at line 213 destructures only three parameters. The column carries no assertion. It looks like a copy ofawaitingCases, where the third element is used.♻️ Proposed change
- const cases: readonly (readonly [string, () => WorkflowEvent, string, string | null])[] = [ + const cases: readonly (readonly [string, () => WorkflowEvent, string])[] = [ [ 'INVOCATION_REQUESTED', () => requestInvocation(buildInvocation({ invocationId: INVOCATION_B })), 'OPEN', - null, ], - ['INVOCATION_REPORTED', () => reportInvocation(), 'OPEN', null], - ['REVIEW_ADMITTED', () => admitReview(), 'OPEN', null], - ['EVIDENCE_ADMITTED', () => admitEvidence(), 'OPEN', null], + ['INVOCATION_REPORTED', () => reportInvocation(), 'OPEN'], + ['REVIEW_ADMITTED', () => admitReview(), 'OPEN'], + ['EVIDENCE_ADMITTED', () => admitEvidence(), 'OPEN'], [ 'EVIDENCE_ADMITTED human-decision', () => admitEvidence(buildHumanDecisionVerdict()), 'OPEN', - null, ], - ['HEAD_OBSERVED different', () => observeHead(SHA_B), 'OPEN', null], - ['HUMAN_GATE_OPENED', () => openHumanGate(), 'AWAITING_HUMAN_DECISION', null], - ['CLOSE_REQUESTED', () => closeWorkflow(), 'CLOSED', null], + ['HEAD_OBSERVED different', () => observeHead(SHA_B), 'OPEN'], + ['HUMAN_GATE_OPENED', () => openHumanGate(), 'AWAITING_HUMAN_DECISION'], + ['CLOSE_REQUESTED', () => closeWorkflow(), 'CLOSED'], ];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/domain/workflow-transitions.test.ts` around lines 192 - 219, Remove the unused fourth element from every tuple in the OPEN transition `cases` array and update its tuple type to contain only the name, event builder, and expected status. Keep the existing three-parameter `it.each` callback and assertions unchanged.
1233-1233: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlso assert the round-tripped state is accepted back by the state machine.
toEqualcompares values and ignores frozen-ness.readListinsrc/domain/workflow-transitions.tsrejects any unfrozen list at lines 420-428, so a state rebuilt withJSON.parsehas three unfrozen lists and is rejected asWORKFLOW_UNREADABLE. The source comment at lines 418-419 states that a caller must re-freeze the three collections.The current assertion proves value equality only. Adding the acceptance check would pin the documented caller obligation, and would keep the obligation visible if
readListchanges.💚 Proposed addition
expect(JSON.parse(JSON.stringify(state))).toEqual(state); + + // A JSON round trip loses frozen-ness, and `readList` requires it. The + // documented caller obligation is to re-freeze the three collections. + const parsed = JSON.parse(JSON.stringify(state)) as WorkflowState; + expect(applyWorkflowEvent(parsed, observeHead(SHA_C)).rejection).toBe( + 'WORKFLOW_UNREADABLE', + ); + const refrozen = Object.freeze({ + ...parsed, + invocations: Object.freeze(parsed.invocations), + evidence: Object.freeze(parsed.evidence), + reviews: Object.freeze(parsed.reviews), + }); + expect(applyWorkflowEvent(refrozen, observeHead(SHA_C)).rejection).toBe('WORKFLOW_CLOSED');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/domain/workflow-transitions.test.ts` at line 1233, Extend the round-trip assertion near the existing JSON equality check to pass the parsed state back through the state-machine validation path and assert it is accepted. Use the existing workflow transition/read API and confirm the result is not WORKFLOW_UNREADABLE, while preserving the current value-equality assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/domain/workflow-transitions.test.ts`:
- Around line 357-367: Update
tests/domain/workflow-transitions.test.ts#L357-L367 in the duplicate
invocation-id test to freeze the invocations array so validation reaches the
duplicate-id scan. Also update
tests/domain/workflow-transitions.test.ts#L1233-L1233 after the JSON round-trip
assertion to verify the raw parsed state is rejected with WORKFLOW_UNREADABLE
and that an equivalent re-frozen copy is accepted, covering the readList caller
requirement.
---
Nitpick comments:
In `@src/domain/workflow-transitions.ts`:
- Around line 978-1002: Update appendTo and replaceAt to preserve every list
position by copying each indexed element unconditionally instead of skipping
undefined values; retain replaceAt’s substitution behavior for the target index
and keep the existing immutable return behavior.
- Around line 693-957: Reduce repeated validation in the applyWorkflowEvent flow
by validating a loaded workflow state once and passing the validated snapshot
forward instead of invoking snapshotWorkflow on every event, including
immediately rejected events such as WORKFLOW_CLOSED. Preserve all existing
snapshotWorkflow invariants and ensure event application still receives
validated state.
- Around line 1066-1078: Update the invalid-field reporting around the invalid
array construction to either iterate according to WORKFLOW_BINDING_FIELD_ORDER,
or revise that constant’s public documentation in workflow.ts to stop claiming
it governs invalidFields ordering. Prefer using the exported constant so the
implementation and documented deterministic order remain aligned, while
preserving the existing validation conditions and field names.
In `@tests/domain/reader-parity.test.ts`:
- Around line 405-414: The test body does not directly verify PR 007’s
no-truncation behavior. In the test named “pins PR 007 as having no field that
truncates,” remove the unrelated ingestInvocationReport assertions and replace
the duplicated workflowReadExactIdentifier check with an assertion using
openWorkflow or applyWorkflowEvent that confirms an oversized identifier is
never retained as a truncated prefix.
In `@tests/domain/workflow-fixtures.ts`:
- Around line 48-85: The oversized and label helpers are duplicated across
workflow and invocation fixtures. Move them into a shared test-helper module,
update both fixture modules to import and reuse those shared exports, and remove
their local implementations while leaving the production reader duplication
unchanged.
In `@tests/domain/workflow-invariants.test.ts`:
- Around line 2085-2108: Update the purity scan in the test case reading
workflow.ts and workflow-transitions.ts to use token-aware regular-expression
matching instead of substring toContain checks. Match standalone identifiers
with word boundaries, while keeping member-access patterns such as Math.random
explicit, and avoid stripping comment-like sequences inside string literals or
regular expressions when preparing source for scanning.
In `@tests/domain/workflow-transitions.test.ts`:
- Around line 878-886: Replace the brittle serialized-value check in the
workflow transition test with a structural assertion that review admission
objects do not contain the stored finding-count field, such as verifying they
contain only the four expected keys. Keep the existing checks for review count
and excluded sentinel fields unchanged.
- Around line 192-219: Remove the unused fourth element from every tuple in the
OPEN transition `cases` array and update its tuple type to contain only the
name, event builder, and expected status. Keep the existing three-parameter
`it.each` callback and assertions unchanged.
- Line 1233: Extend the round-trip assertion near the existing JSON equality
check to pass the parsed state back through the state-machine validation path
and assert it is accepted. Use the existing workflow transition/read API and
confirm the result is not WORKFLOW_UNREADABLE, while preserving the current
value-equality assertion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c8b4db1b-12cf-4a27-b270-024139834436
📒 Files selected for processing (9)
README.mddocs/architecture/007-autoflow-state-machine.mdsrc/domain/index.tssrc/domain/workflow-transitions.tssrc/domain/workflow.tstests/domain/reader-parity.test.tstests/domain/workflow-fixtures.tstests/domain/workflow-invariants.test.tstests/domain/workflow-transitions.test.ts
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a03c0eff3b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) { | ||
| return false; | ||
| } | ||
| if (earlier < later && earlierHigh >= laterLow) { |
There was a problem hiding this comment.
When retained stamps move forward by multiple revisions, this comparison checks only their relative order and not whether enough sequence slots exist for the intervening HEAD_OBSERVED transitions. For example, a request at revision 0/sequence 1 followed by its report at revision 2/sequence 3 passes every current check, although the two required HEAD transitions must occupy sequences 2 and 3, leaving the report no possible slot; the next event then preserves an unreachable audit history. Require the sequence gap between ordered stamps to exceed the revision delta, and apply the equivalent constraint between the final stamp and the aggregate counters.
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad14bbc815
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| rawStatus === WORKFLOW_STATUS.AWAITING_HUMAN_DECISION && | ||
| humanGateOpenedAtRevision === null | ||
| ) { | ||
| return null; |
There was a problem hiding this comment.
When a deserialized state has status: AWAITING_HUMAN_DECISION, humanGateOpenedAtRevision: 0, and sequence: 0, these checks accept it even though opening the gate must already have consumed sequence 1; a subsequent HEAD_OBSERVED is then applied using that same first slot. The equivalent omission admits a CLOSED state whose sequence has no room for its closing transition. Require the aggregate chronology to reserve an unstamped sequence slot for the current gate-opening or closing transition.
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 977d854ba7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ( | ||
| (rawStatus === WORKFLOW_STATUS.AWAITING_HUMAN_DECISION || | ||
| rawStatus === WORKFLOW_STATUS.CLOSED) && | ||
| sequence <= seenSequences.length | ||
| ) { |
There was a problem hiding this comment.
Fresh evidence on this HEAD is that the new count check still accepts an unreachable awaiting-human state after a HEAD change: with revision: 1, sequence: 2, and one current-revision evidence admission stamped at sequence 2, seenSequences.length is only 1, so this condition passes even though sequence 1 is necessarily occupied by HEAD_OBSERVED and no slot remains for HUMAN_GATE_OPENED. A subsequent fact-recording event can then apply and preserve the corrupt history; verify that an unstamped slot exists at the current revision after the last required HEAD transition rather than comparing against the number of retained stamps.
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4aae023c20
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Revision never decreases, so ordering every stamped record by sequence must | ||
| // yield non-decreasing revisions. | ||
| if (!revisionBandsOrdered(spanRevisions, spanLowest, spanHighest, revision, sequence)) { |
There was a problem hiding this comment.
An OPEN state with revision: 0, sequence: 1, and all three collections empty passes these checks, after which a HEAD_OBSERVED event is applied at sequence 2. No transition can produce the supplied state: at revision 0 every sequence-advancing event either leaves a retained stamp, changes the status to awaiting-human, or closes the workflow. Because records are never deleted, accepting unexplained sequence gaps corrupts the documented total order and optimistic-concurrency token; validate an upper accounting bound for unstamped transitions as well as the existing lower chronology bounds.
Useful? React with 👍 / 👎.
| (rawStatus === WORKFLOW_STATUS.AWAITING_HUMAN_DECISION || | ||
| rawStatus === WORKFLOW_STATUS.CLOSED) && | ||
| sequence <= revision + seenSequences.length | ||
| ) { |
There was a problem hiding this comment.
Fresh evidence on this reviewed tree is that an awaiting-human state with revision: 1, sequence: 3, and one historical admission at revision 0/sequence 2 passes this count and even accepts a subsequent HEAD_OBSERVED. The historical admission must be followed by the revision-advancing HEAD at sequence 3, leaving no slot in revision 1 for the HUMAN_GATE_OPENED transition that the status requires. Comparing only the total number of stamps therefore still admits an unreachable aggregate; ensure the open-gate transition has a slot after the final transition needed to reach the current revision.
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@codex Please perform one fresh final review of PR #9 against the CURRENT HEAD only: STRICT REVIEW BOUNDARY:
This is a FINAL REVIEW GATE, not an implementation request. Do not modify code. If you find an issue, provide the exact current-HEAD counterexample and identify the affected current lines. If no actionable defects exist on this exact HEAD, state that the current PR is ready to merge. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 808e8e9f36
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Only an open gate at revision >= 1 is constrained. A gate that was already | ||
| // cleared — by a HEAD advance or by a human decision — leaves the workflow | ||
| // `OPEN`, and those histories are deliberately left alone. | ||
| if (rawStatus === WORKFLOW_STATUS.AWAITING_HUMAN_DECISION && revision > 0) { |
There was a problem hiding this comment.
When a frozen state represents HUMAN_GATE_OPENED at sequence 1, a human-decision admission at revision 0/sequence 2, and HEAD_OBSERVED reaching revision 1/sequence 3, changing it to status: CLOSED, closureReason: CALLER_CLOSED, and a cleared gate is accepted as readable and returns WORKFLOW_CLOSED. This state is unreachable because the terminal CLOSE_REQUESTED transition would require sequence 4; the placement guard only covers AWAITING_HUMAN_DECISION, so apply an analogous final-slot constraint to CLOSED states.
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@codex Please perform a fresh final review of PR #9 against the CURRENT HEAD only: Important review rules:
Please report only findings that are reproducible against: If there are no actionable findings on this exact HEAD, explicitly report that the review is clean. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c18569b4fb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (bandRevision < revision && bandHighest > latestEarlier) { | ||
| latestEarlier = bandHighest; | ||
| } | ||
| } | ||
| if (sequence < latestEarlier + 2) { |
There was a problem hiding this comment.
For a CLOSED state, CLOSE_REQUESTED must be the final transition because no subsequent event is legal, but this loop considers only stamps from earlier revisions. On the current HEAD, a revision-0 closed state with sequence: 2 and evidence stamped at sequence 2 is accepted as readable and returns WORKFLOW_CLOSED; the evidence consumed the alleged final slot, so the required close transition would actually need sequence 3. Require every retained sequence stamp, including current-revision stamps, to be strictly less than a closed aggregate's sequence.
Useful? React with 👍 / 👎.
| if ( | ||
| (rawStatus === WORKFLOW_STATUS.AWAITING_HUMAN_DECISION || | ||
| rawStatus === WORKFLOW_STATUS.CLOSED) && | ||
| sequence <= revision + seenSequences.length | ||
| ) { |
There was a problem hiding this comment.
When a closed snapshot retains humanGateOpenedAtRevision, its history necessarily contains both an unstamped HUMAN_GATE_OPENED and a later unstamped CLOSE_REQUESTED, but this lower bound reserves only one status-producing slot. For example, a revision-0 state with empty collections, sequence: 1, status: CLOSED, and humanGateOpenedAtRevision: 0 is accepted and returns WORKFLOW_CLOSED, although opening the gate consumes sequence 1 and closing requires sequence 2. Reserve the additional gate slot whenever a closed state retains an open-gate revision.
Useful? React with 👍 / 👎.
| if ( | ||
| revision === 0 && | ||
| rawStatus === WORKFLOW_STATUS.OPEN && | ||
| seenSequences.length === 0 && | ||
| sequence > 0 |
There was a problem hiding this comment.
The current HEAD still applies the upper accounting check only when there are no retained stamps. Fresh reproduction: an OPEN revision-0 state with one ci-result admission at sequence 1 but aggregate sequence: 2 is accepted and can apply HEAD_OBSERVED, even though no transition can explain slot 2—there was no HEAD, closure is terminal, and a gate would leave the status awaiting because this evidence kind cannot clear it. Extend the upper accounting bound to histories with retained records, allowing only gate slots that a retained human decision or HEAD could actually have cleared.
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@codex Please perform the FINAL convergence review of PR #9 at the exact current HEAD: Review the current HEAD only. Treat findings attached to every older SHA as stale. This is a final correctness review of the frozen PR 007 scope. Please report only concrete, reproducible defects present on this exact HEAD that violate the existing PR 007 architecture, invariants, or documented semantics. Do not propose:
Pay particular attention to the chronology/state-machine repairs and their interaction with existing D1–D7 and A1–A3 invariants. If you identify a defect, provide a minimal reproducible state/event sequence demonstrating the violation on this exact HEAD. If no such defect remains, explicitly state that no actionable current-HEAD findings were found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a01f54d58e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Deliberately bounded to revision 0 and `OPEN`. Once a HEAD advance is in | ||
| // play it clears a gate while leaving nothing behind, and no upper bound is | ||
| // claimed there. With no stamps at all this reduces to the untouched | ||
| // workflow: only sequence 0 is reachable. | ||
| if (revision === 0 && rawStatus === WORKFLOW_STATUS.OPEN) { |
There was a problem hiding this comment.
Fresh evidence on this exact HEAD: start with openWorkflow, then supply a frozen OPEN snapshot with empty frozen collections, revision: 1, and sequence: 3; applyWorkflowEvent(state, { kind: 'HEAD_OBSERVED', observedCommitSha: 'ccc' }) returns APPLIED at revision 2/sequence 4. This snapshot is unreachable: the sole HEAD needed to reach revision 1 can consume only slot 1, while neither slots 2 nor 3 can be explained—opening a gate would leave the status awaiting unless a retained human-decision stamp or another revision-advancing HEAD cleared it. Limiting the upper accounting check to revision 0 therefore still admits and perpetuates corrupt chronology.
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6a73afcb5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const gatesOpened = revision + humanDecisions; | ||
| if (sequence > seenSequences.length + revision + gatesOpened) { |
There was a problem hiding this comment.
Fresh evidence after the latest upper-bound fix: an OPEN, revision-0 snapshot with one human-decision admission stamped at sequence 1 and aggregate sequence 2 still passes this formula. That decision cannot have cleared a gate because no earlier sequence slot exists to open one, while using slot 2 to open a gate would leave the workflow awaiting; nevertheless humanDecisions licenses that unexplained slot and the next event preserves an unreachable history. Count a decision as a possible gate clear only when chronology leaves room for an earlier gate-opening transition.
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7587ad91f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // stamps at all to the untouched workflow, where only sequence 0 is | ||
| // reachable. Deliberately confined to `OPEN`: a retained gate or closure | ||
| // posture is accounted for by the lower bounds above. | ||
| if (rawStatus === WORKFLOW_STATUS.OPEN) { |
There was a problem hiding this comment.
When a deserialized AWAITING_HUMAN_DECISION state at revision 0 has empty frozen collections and sequence: 2, it passes the lower status-slot check and skips this upper bound; however, only the gate-opening transition can explain slot 1, leaving slot 2 unreachable. Checked against applyWorkflowEvent: a subsequent HEAD_OBSERVED is accepted and preserves the corrupt chronology at sequence 3. Extend the upper accounting to awaiting and closed states while reserving their required status-transition slots.
Useful? React with 👍 / 👎.
| if (admitted.admittedAtSequence - 1 > stampsBelow) { | ||
| clearingDecisions += 1; |
There was a problem hiding this comment.
Fresh evidence after this eligibility fix is an OPEN, revision-0 snapshot with human-decision admissions at sequences 2 and 3 and aggregate sequence 4: both decisions count because each sees the same unstamped slot 1, so the state is accepted and can apply HEAD_OBSERVED. In any real history, the first decision clears the gate opened at slot 1, the second clears nothing, and a gate at slot 4 would leave the workflow awaiting; count decisions only when each can be paired with its own earlier, still-uncleared gate slot.
Useful? React with 👍 / 👎.
1 participant
Purpose
PR 007 implements the Autoflow engine's state model: the commit-bound record of what has been requested and what has been independently established for one unit of work.
It answers exactly one question:
It does not answer what should happen next. Legality is domain; selection is policy, and policy belongs to a later PR.
The audit found that 12 of the 18 lifecycle distinctions exposed by the real PR 005/006 workflows are already owned by PR 003 (authority), PR 004 (freshness), PR 005 (findings), or PR 006 (claims). Only six are orchestration state, which is what keeps this layer small: 3 workflow statuses, 2 invocation states, 7 events, 16 rejection reasons.
Frozen scope
No I/O, and nothing is invoked. No agent execution, dispatch, or transport; no provider adapters; no GitHub/Claude/OpenAI/Gemini/CodeRabbit calls; no network, filesystem, subprocess, database, or Evidence Store persistence; no clock, timer, or identifier generation; no Promises or async of any kind. Both exported functions are pure functions of their arguments.
PR 007 consumes only the outputs of PR 004, PR 005, and PR 006. There is no signature that accepts an
AgentReport, aReviewSubmission, or anEvidenceRecord, so a second normalizer is a compile-time impossibility rather than a review comment. Frozen vocabulary constants are imported from those layers — redeclaringFRESHNESS.CURRENTwould create a divergent second answer — but no reader, normalizer, or validator function is.Nothing in PR 001–006 changed behaviour. Only
src/domain/index.ts(export block),tests/domain/reader-parity.test.ts(two-way → three-way), andREADME.md(one paragraph) are modified.The state machine
Governing principle: recording a fact is legal whenever the workflow is not closed; initiating work is not.
OPENAWAITING_HUMAN_DECISIONCLOSEDINVOCATION_REQUESTEDWORKFLOW_AWAITING_HUMANWORKFLOW_CLOSEDINVOCATION_REPORTEDWORKFLOW_CLOSEDREVIEW_ADMITTEDWORKFLOW_CLOSEDEVIDENCE_ADMITTED, kind ≠human-decisionWORKFLOW_CLOSEDEVIDENCE_ADMITTED, kind =human-decisionOPENOPENWORKFLOW_CLOSEDHEAD_OBSERVED, different commitrevision + 1revision + 1, clears the gate →OPENWORKFLOW_CLOSEDHEAD_OBSERVED, same commitHEAD_UNCHANGEDHEAD_UNCHANGEDWORKFLOW_CLOSEDHUMAN_GATE_OPENEDAWAITING_HUMAN_DECISIONHUMAN_GATE_ALREADY_OPENWORKFLOW_CLOSEDCLOSE_REQUESTEDCLOSEDCLOSEDWORKFLOW_CLOSEDCLOSEDis absolutely terminal: no reopen, no resurrection. A new unit of work is a new workflow.There is deliberately no
HUMAN_DECISION_RECORDEDevent. A human decision is PR 004 evidence of kindhuman-decision, arriving throughEVIDENCE_ADMITTED.EvidenceFreshnesscarries no verdict field, so this layer records that a human decided and is structurally unable to learn what they decided. PR 003's gate plus the human remain the only authority boundary.Evaluation precedence is fixed and never varies, so rejection reasons are deterministic: state readable → event readable → kind recognised → not
CLOSED→ payload slot → status posture → upstream outcome → payload fields → binding → identity/replay → capacity → apply.Ratified architecture decisions D1–D7
WorkflowStateaggregate containing tracked invocationsfindingCountremovedAdmittedReviewis a stable pointer;review.findingsis never read — no text, severity, classification, or count reaches stateboundCommitShachanges only viaHEAD_OBSERVED; monotonicrevisionis the admission keyboundCommitShais assigned in exactly one functionAWAITING_HUMAN_DECISIONis a real status; work-initiating events refused while open; fact-recording still admittedsrc/untrusted-input.ts;reader-parity.test.tsis now three-wayCAPACITY_EXCEEDED, returning the identical prior stateA1 — a human gate clears on
HEAD_OBSERVEDAn applied
HEAD_OBSERVEDunconditionally setsstatus: OPENandhumanGateOpenedAtRevision: nullalongside the rebind andrevision + 1. No branch on prior status.A human gate is commit-bound orchestration state, not authority. It is opened against the bound commit, so once that binding moves the gate is as stale as any other old-revision fact. Clearing removes no human authority: no approval is inferred, nothing is cancelled, no policy is applied, and a decision recorded against the superseded commit is subsequently refused
EVIDENCE_NOT_CURRENT. A later PR may open a new gate at the new revision when its policy requires one.Consequent pinned invariant:
humanGateOpenedAtRevisionis alwaysnullor exactlyrevision. Its only non-derivable content is whether a gate was open at closure, which is why it is retained on close. The relationship is enforced when a state is read back and asserted by a test, so the two values cannot disagree.A2 —
admittedAtCommitSharetainedAdmittedEvidenceandAdmittedRevieweach carryadmittedAtCommitShaalongsideadmittedAtRevisionandadmittedAtSequence. Past bound commits are not otherwise recoverable from the aggregate, so retaining it is what keeps a persisted state independently auditable without a companion history table that does not yet exist. Admissions keep their commit binding verbatim across later HEAD moves.A3 — unsolicited reviews remain admissible
A correctly bound, valid
ReviewResultis admitted even when itsreviewIdmatches no tracked invocation — automated forge reviewers and human reviewers produce real reviews AgentBridge did not request, and refusing them would make those invisible to orchestration.Admitting one does not: transition any invocation (only
INVOCATION_REPORTEDdoes that), imply it was requested, imply sufficiency, imply policy satisfaction, imply authority, or trigger repair. The state records nothing that distinguishes a requested review from an unsolicited one — asserted by byte-comparing the two admissions. Whether a requested, attributable, independent, or specific review is required for a given decision is a policy question owned by a later PR.revisionandsequenceThe two remain distinct and are never collapsed; they answer different questions.
sequencestarts at 0 and advances by exactly one on every applied transition, never on a rejection. It is the total ordering and the natural optimistic-concurrency token for a later persistence layer — the deliberate substitute for a clock, since this layer reads none.revisionstarts at 0 and advances only on an appliedHEAD_OBSERVED. It is the admission key.Commit ordering is never inferred. A SHA is opaque: no parent check, no ancestry test, no "is this newer". A HEAD that returns to a previous commit still advances the revision, so evidence admitted earlier cannot resurrect — revision, not SHA alone, is the admission key, which is what defends against a hostile or buggy adapter replaying a HEAD. Retained earlier admissions remain true at their own revision and commit; they simply stop counting.
Claim / evidence separation
PR 006's ladder is unchanged and PR 007 adds no rung. There is no code path from
INVOCATION_REPORTEDinto any admission list — asserted behaviourally (areported-completereport carrying 64 claims whoseclaimedCommitShaequals the bound commit produces zero admissions) and structurally (a test slices the handler out of the source and asserts it never mentions the admission lists or their types). Reaching "remotely observed" still requires a new record built from an independent adapter observation, arriving as a separateEVIDENCE_ADMITTEDevent.EVIDENCE_ADMITTEDtakes a PR 004EvidenceFreshness, not anEvidenceRecord. Freshness is never re-derived; PR 004 already answered, and its result carries the target it was answered against. The only checks are thatstateisCURRENT,reasonisBOUND_TO_CURRENT_HEAD, and bothtargetRepositoryIdandtargetHeadShamatch this workflow's binding — so a caller cannot launder stale evidence by judging it against a convenient target. No change to PR 004 was required.INVOCATION_REPORTEDbinds against the tracked invocation's commit, not the workflow's current one: a report arriving after HEAD moved is a true historical fact and is recorded, but it admits no evidence.Provider, purpose, and reported-status neutrality
Legality never depends on
providerId,agentId,purpose, orreportedStatus. They are recorded for audit and read by no branch — every reference is a presence check, a vocabulary shape check, or a store, never a comparison against a specific label.A parametrized test runs all 128 combinations of eight provider labels (including
system,root,admin,agentbridge-internal), four purposes, and four reported statuses, asserting the resulting states are identical once the three recorded label fields are normalized.reported-completeandreported-failedproduce indistinguishable transitions; arepairinvocation produces no field areviewinvocation lacks.CAPACITY_EXCEEDEDMAX_IDENTIFIER_LENGTHMAX_TRACKED_INVOCATIONSMAX_ADMITTED_EVIDENCEMAX_ADMITTED_REVIEWSMAX_REVISION/MAX_SEQUENCEExceeding a bound rejects the transition and returns the identical prior state. This is a deliberate third convention: PR 004 collapses an over-length evidence set to zero and PR 005/006 truncate and flag, but both operate on elements of a single hostile payload. A transition instead carries one discrete fact, so refusing it visibly at the call site is the only outcome that loses nothing — silently dropping orchestration history would be the dangerous result. A workflow that reaches a bound is an escalation signal for a later PR.
Identifiers reject; nothing here truncates. There is no
truncatedfield on any PR 007 type because this layer stores no prose.Security and adversarial invariants
WorkflowBinding,observedCommitSha,atCommitSha, andclosureReasonare trusted;AgentInvocationis trusted for binding and inert as authority; PR 004/005/006 results are pre-normalized but re-validated as hostile — trusting the type is not trusting the value.EvidenceTarget. No agent-controlled payload has a field through which it could be set.__proto__payload supplies nothing.WORKFLOW_UNREADABLErather than being partially trusted.Array/String/Setprototypes, replacedObject.freeze/Object.hasOwn, inherited numeric index setters. Nothing throws; everything fails closed.ALLOW/DENY/ESCALATE/AUTONOMOUS/CURRENT/STALEvalue reaches a serialized state, and the state exposes no boolean field at all.-0is rejected wherever a count is read: it compares equal to0but breaks byte identity across a JSON round trip.Explicitly excluded — PR 008 and PR 009+
PR 008: retry counts, attempt limits, repair budgets, backoff, timeouts, deadlines, cancellation policy, loop termination, convergence detection, escalation policy, cost/token ceilings, sufficiency and quorum rules, and any next-action selection.
PR 009+: concrete Claude/OpenAI/Gemini/CodeRabbit adapters, real transport, GitHub mutations, artifact existence verification, integration detection, Evidence Store persistence, live external-service adapters, and end-to-end bridge integration.
Also absent: polling, queues, schedulers, async, concurrency control, provider/reviewer routing (roles stay configuration resolved before an invocation is constructed), merge-readiness policy, approval logic, human-approval UI, dashboards, identifier generation, commit ancestry inference, and any public projection API — there is deliberately no
legalEventKinds(), because a public enumeration of what is permitted is one refactor away from being read as advice.Verification
npm run verify(typecheck + lint + test + build)npm auditgit diff --checkpackage.json,package-lock.json, tsconfig, eslint, vitest, and CI are untouchedReviewed HEAD:
7cfbfc95c6da4e65fc10a710d29ff929630760fe.Architecture record:
docs/architecture/007-autoflow-state-machine.md.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests