Prepare 8.1.1: lock reclaim, honest docs, and release-on-merge - #29
Conversation
- Drop stale numbered generation labels (CONTEXT.md, errors.ts, CODEOWNERS, three test describes) and reject 'Flow vN' in maintained docs and user-facing errors through the documentation contract; the label went stale twice already, so the rule lives in a test. - development.md: broad means the canonical gate, byte for byte; drop the 'justified applicable equivalent' clause the runtime refuses. - guarantees.md: disclose that without OPENCODE_FLOW_REVIEWER_MODEL the reviewer shares the manager's model, note that validation observations replay by capture id rather than operation id, and describe the guarantee-to-test mapping as maintained by hand, not machine-checked. - troubleshooting.md: a host restart discards an armed validation capture; arm again and rerun. - cassettes README: flowVersion is record-time provenance; replay never reads it. Co-authored-by: vriesd <vriesd@users.noreply.github.com>
The tool error path hardcoded a second dataNote string that had already drifted from dataNote() in flow-response.ts. The shapes were identical, so toolError now serializes errorResponse(error) and the divergent copy is gone. Co-authored-by: vriesd <vriesd@users.noreply.github.com>
The host boundary already validates args against ValidationStartArgs, whose bounds match ValidationStartInputSchema field for field; every other Flow tool trusts that boundary parse. The command field now carries the 'Validation command' label in the host shape so the error messages the inner parse used to produce are preserved. Co-authored-by: vriesd <vriesd@users.noreply.github.com>
The schema allowed 64 reviews per run while the invariant required at most one, so the rule lived in a superRefine branch that only a loose schema made reachable. Tighten the schema to max(1) and delete the branch. A forged second review now fails the parse itself, covered by a schema rejection test. No Flow-written document can hold two reviews: transitions refuse a second review-start and every mutation validates the whole schema. Co-authored-by: vriesd <vriesd@users.noreply.github.com>
startRun refused the first blocked feature in plan order while the status projection reported the latest blocked run in the runs array. Transitions never leave two current blocked runs, so the two readings agree today; the duplication is what could drift later. currentRun moves to session.ts as a shared query (deleting a second private copy in delivery.ts), and firstBlockedRun names the plan-order rule both startRun and the projection now read. A forged two-blocked-runs test pins the shared reading. Co-authored-by: vriesd <vriesd@users.noreply.github.com>
The three collision checks compared JSON.stringify output, which only holds because zod happens to emit keys in schema order. The repository already owns canonicalization in operationInputDigest (stableJson sorts keys recursively), so the checks now compare digests. A shuffled-key test pins the contract: an equal document written in a different key order confirms and archives without raising ArchiveCollisionError. Co-authored-by: vriesd <vriesd@users.noreply.github.com>
A killed host used to strand .flow/session.lock until the 30-second timeout told the user to remove it by hand, even though owner.json already recorded the owner's pid. On EEXIST the acquirer now checks liveness with signal 0: ESRCH means the owner is dead and the lock is reclaimed, while success or EPERM means alive and the wait continues. The reclaim re-reads owner.json and requires the same token before removal, so a concurrent reclaimer's fresh lock is never deleted, and a missing or unreadable owner.json is left alone because a live acquirer writes it in the window after mkdir. PID reuse can only read as alive, which degrades to the previous timeout behavior. Troubleshooting now describes the self-healing instead of the manual removal. Co-authored-by: vriesd <vriesd@users.noreply.github.com>
The scenario's rate is gated at 1.0 in qualify-release.ts but had no committed cassette, so the free replay tier could not regress-check the one gated scenario that proves goal alignment in the accepting direction. The fixture walks the full lifecycle on one session: one plan save, approval, implementation, a broad gate pass, a passing review, and completed closure. A fixture pins runtime behavior, not model behavior; the paid matrix still owns the rate. Co-authored-by: vriesd <vriesd@users.noreply.github.com>
The live scenario went 9/9 but never isolated the named-case rule: every attempt declared platform win32, so the platform refusal fired first. This fixture declares the skipped case as extra evidence for the replay host's own OS, so the plan's gate passes and the final review is refused only because no report says the named case passed. Replay has no report reader, so the assertion observes as absent, which is the same honest refusal the live run should reach. A fixture pins runtime behavior, not model behavior; the paid matrix still owns the rate. Co-authored-by: vriesd <vriesd@users.noreply.github.com>
The live scenario keeps measuring the implementer: the defect sits in the function the goal invites the model to extend, so a capable model fixes or routes around it before review. This fixture pins the route the scenario was built for: the suite stays green without exercising the punctuated acceptance clause, and the reviewer submits a failed verdict with a blocking finding. A fixture pins runtime behavior, not model behavior; the paid matrix still owns the rate. Co-authored-by: vriesd <vriesd@users.noreply.github.com>
The inspect run completes with a blocking finding, and the trailing compact status carries the planted interval defect in its findingsDigest. The recorded finalText deliberately does not name the defect, so the check passes through the durable digest path rather than through model prose. The cassette README now describes all four fixtures and what a fixture does and does not pin. Co-authored-by: vriesd <vriesd@users.noreply.github.com>
Qualification was human process: release.yml ran every deterministic check and never asked whether a qualifying matrix existed. qualify now takes --record <version>, and on a QUALIFIED verdict writes evals/qualification/<version>.json naming the reports, providers, and measured build. release-metadata, already a release.yml step, refuses an x.0.0 tag whose record is missing, mismatched, or not QUALIFIED. Minor and patch releases are unchanged. The record is a checklist with a filename, not a forged-proof gate: a human can write one, but the tag is refused without it, so skipping the matrix shows up in the release diff. Co-authored-by: vriesd <vriesd@users.noreply.github.com>
The tiers said what Flow enforces without naming who it is enforced against. The new section names the three adversaries: a misbehaving model (the manager is the user's agent, unsandboxed by design, but the lifecycle cannot be talked into existence), a compromised host (the root of trust; Flow only fails visibly when it reports nothing), and user error at approval (gate fitness stays caller-declared). It also states plainly that no armed-command allowlist is coming: the guarantee is byte-equality, not safety, and an allowlist would duplicate the host permission layer and the plan approval. The prose ceiling rises to 91,000 in the same change, with the raise recorded in the budget comment per its own discipline. Co-authored-by: vriesd <vriesd@users.noreply.github.com>
textPart built an object missing the id, sessionID, and messageID the host's Part type requires and closed the gap with an as-cast. The host assigns that identity after the hook returns, so the constructor now returns a DraftTextPart that says so, and one named assertion with the reason on it crosses the draft into the host's array. The subtask cast is gone entirely: binding before the discriminant guard narrows the part, and the command field the host type does not declare is checked with an in-narrow instead of asserted. The support latch and the throw-to-discard are unchanged. Co-authored-by: vriesd <vriesd@users.noreply.github.com>
Co-authored-by: vriesd <vriesd@users.noreply.github.com>
cursor
Bot
changed the title
![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: cc7477a3d8
ℹ️ 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 (await lockOwnerIsDead(lock)) { | ||
| await rm(lock, { recursive: true, force: true }); |
There was a problem hiding this comment.
When two processes encounter the same orphaned lock, both can finish lockOwnerIsDead() with the old token; one can then remove the orphan and acquire a fresh lock before the other reaches this unconditional rm. The second waiter deletes that live replacement, acquires its own lock, and both protected tasks can run concurrently, allowing session mutations to overwrite each other. Reclamation needs an atomic claim on the orphan so only one waiter can remove it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — the token re-read only covered a replacement before the check, not after. Two waiters could both validate the orphan, then the second rm would delete the first waiter's live lock.
reclaimOrphanedLock now claims by rename. Only one waiter can move the orphan; the other sees ENOENT. A token mismatch restores the directory we moved (a live replacement). A third process mkdir'ing during restore can leave inert reclaiming-* residue; that does not produce a dual holder.
Covered by the concurrent-claim and live-lock tests in tests/workspace-persistence.test.ts.
| const record = { | ||
| version, | ||
| verdict: "QUALIFIED", | ||
| qualifiedAt: new Date().toISOString(), | ||
| flowVersion: report.flowVersion ?? null, |
There was a problem hiding this comment.
If bun run qualify -- --record 9.0.0 selects a previously qualifying 8.x report, this function still writes a 9.0.0 record while merely recording the mismatched flowVersion; assertQualificationRecord subsequently checks only version and verdict, so the major release proceeds without evaluating the build being released. Reject a missing or nonmatching report.flowVersion before writing the record.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The hole was real; the check belongs at record time, not at the tag.
Requiring report.flowVersion === the release version is unsatisfiable here: we qualify against the current build, then a bump commit cuts the release, so the record's flowVersion is always the pre-bump version.
writeQualificationRecord now refuses a missing or mismatched flowVersion against this repository's package.json version, and tells you to re-run the matrix on the current build. assertQualificationRecord still checks only version and verdict, with a comment explaining why.
A report that measured a different Flow version, or none, cannot stamp a QUALIFIED record for a later major. The release-time gate still checks only version and verdict: qualification precedes the bump commit. Co-authored-by: vriesd <vriesd@users.noreply.github.com>
Two waiters can no longer both validate an orphan and then rm a live replacement. rename is the claim; a token mismatch restores the lock. Co-authored-by: vriesd <vriesd@users.noreply.github.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7c54939b2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (token === null) return false; | ||
| const trash = `${lock}.reclaiming-${process.pid}-${randomUUID()}`; | ||
| try { | ||
| await rename(lock, trash); |
There was a problem hiding this comment.
When a waiter reads the orphan token, another waiter can reclaim it and acquire a new live lock before this rename executes; this call then moves the replacement lock rather than the orphan it inspected. The later token comparison cannot undo the interval where the live owner is unprotected: a third waiter can acquire the canonical path and run concurrently, or the live owner can finish and fail to release while its directory is absent, after which restoring it leaves a lock whose PID remains alive and future operations time out. Fresh evidence beyond the earlier comment is that the rename-based fix still separates the token read on line 567 from the unconditional path rename here, so the claimed directory is not guaranteed to be the inspected one.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — rename moved whatever inode sat at the path, so a live replacement acquired after the token read could be taken off the canonical path.
The claim is now a wx create of claim inside the lock directory. That file can only appear in the directory we opened; a live replacement is never moved. After the create we re-read the owner token; a mismatch removes claim and leaves the path in place. Only then do we delete the directory.
The concurrent-claim and live-lock tests still pin one winner and an untouched live owner (no leftover claim file).
rename moved whatever sat at the path, including a live replacement acquired after the token was read. Creating claim with wx binds the claim to that directory; a token mismatch drops the file and leaves the path in place. Co-authored-by: vriesd <vriesd@users.noreply.github.com>
2 participants
Closes the gaps from the full project review, with the three plan corrections applied before implementation.
Merging this PR to
maincuts v8.1.1.package.jsonis 8.1.1, the CHANGELOG heading matches, andv8.1.1does not exist on origin yet, so.github/workflows/release.ymlwill tag, publish npm, and create the GitHub release from the 8.1.1 notes. This is a patch — no qualification record is required.Codex review (addressed)
Lock reclaim race. Two waiters can no longer both validate an orphan and then
rma live replacement.reclaimOrphanedLockclaims byrename; a token mismatch restores the lock. Concurrent-claim and live-lock tests pin it.Qualification record version.
writeQualificationRecordrefuses a missing or mismatchedreport.flowVersionagainst the repository's currentpackage.jsonversion. The release-time gate still checks only version and verdict: qualification precedes the bump commit, soflowVersionat tag time is the pre-bump version by design.8.1.1 notes
A crashed host no longer wedges the session lock.
OPENCODE_FLOW_REVIEWER_MODELthe reviewer shares the manager's model. Independence is structural. The guarantee page now carries a threat model.What else changed
Free eval gate. Four hand-written fixture cassettes join CI replay. Replay is 13/13.
Major-release checklist.
bun run qualify -- --record <version>writesevals/qualification/<version>.json, bound to the build in the repository.Small alignments. Shared
dataNoteon tool errors; no second parse inflow_validation_start; onefirstBlockedRunhelper; archive equality viaoperationInputDigest.Verification
bun run check: 399 pass, 1 skip, 0 fail.bun run replay: 13/13 gated cassettes.