Prove basis-bound validated patch application - #24
Conversation
|
@coderabbitai review |
|
@codex please review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
✅ Action performedReview finished.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 50 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
Summary by CodeRabbit
WalkthroughAdds a compiler-authored, basis-bound workspace patch application. Edict emits the request, Echo coordinates durable admission and settlement, the adapter performs bounded writes, and tests cover recovery, refusal, retry, replay, reconciliation, artifact integrity, writer epochs, and stress. ChangesValidated patch execution
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@patch-host/src/main.rs`:
- Around line 660-716: Update open_write_store so writer-epoch acquisition uses
a fresh, explicitly sourced epoch and persisted prior-epoch linkage rather than
the static epoch_id(), fixed identity digests, and previous_epoch_id: None.
Reuse the FilesystemWalStore/WAL contract to load the prior epoch and derive or
validate unique monotonic epoch, fencing, process, host, and lease data; if
acquire_writer_epoch already guarantees this, make that guarantee explicit at
the call site or through the store API.
In `@patch/src/apply-validated-patch.edict`:
- Around line 23-32: Replace the placeholder digests in the
apply-validated-patch bindings for workspace.patch.input@1,
workspace.patch.settlement@1, and workspace.patch.reconcile@1 with the actual
SHA-256 digests of vendored schema artifacts. Ensure the build rejects any
unresolved schema references and does not permit sentinel digest values.
In `@README.md`:
- Around line 215-216: Update the README claim about compiler-artifact
substitution boundaries to match the behavior actually covered by
tests/patch-runtime.sh: either narrow it to the request and claim phases, or add
an inspect/replay recovery substitution case before retaining the
recovery-boundary claim. Prefer the minimal prose correction unless the missing
recovery test is intentionally implemented.
In `@tests/patch-runtime.sh`:
- Around line 515-517: Update the mutated-core setup around mutated_core so it
explicitly verifies that the mutation changed the artifact: capture the original
byte 0, apply the NUL substitution, and assert the original value was non-NUL
before proceeding. Keep the existing cp and dd flow, but make the fixture fail
if the substitution would be a no-op.
- Around line 410-415: Add a post-refusal assertion for the symlink case in
tests/patch-runtime.sh, checking that $patch_root/symlink/outside.txt remains
unchanged from its pre-patch content. Keep the existing stale-basis,
unauthorized, and ci-workflow assertions intact, and verify the symlink escape
target rather than only the refusal status.
- Around line 619-628: The retained-evidence leak check around the grep command
must search recursively from $patch_root, including the project root, rather
than relying on the single-depth "$patch_root"/*/*.json glob. Mirror the error
handling used by tests/runtime.sh so grep exit status 2 or any search error
fails the check, while exit status 1 still represents a clean no-match result.
- Around line 473-484: Update the settlement-size boundary comment to describe
validating the host floor rather than the exact encoded result size. In the
boundary-probe logic around boundary_result_bytes and boundary_floor, explicitly
validate that jq returned a present numeric canonicalResultByteCount before
applying the 1024 minimum, and fail with a clear named assertion if it is
missing or invalid instead of allowing set -e or arithmetic errors.
- Around line 254-266: Strengthen the tampered aperture assertion in the apply
test by reading tampered-report.json and asserting the exact aperture-mismatch
obstruction string emitted by the host, rather than relying only on run_phase’s
nonzero exit status. Keep the existing recovery inspection and secret-content
checks unchanged, and match the typed obstruction format used by the other
negative cases.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro Plus
Run ID: f0b7f94c-3e6f-4cba-990b-0a8e2fb35c6b
📒 Files selected for processing (20)
CHANGELOG.mdREADME.mddocs/roadmap.mdedict.patch.application.jsonpatch-host/Cargo.toml.templatepatch-host/src/main.rspatch/src/apply-validated-patch.edictpatch/vendor/workspace-patch/SOURCE.mdpatch/vendor/workspace-patch/adapter.cborpatch/vendor/workspace-patch/adapter.sha256patch/vendor/workspace-patch/exports.cborpatch/vendor/workspace-patch/exports.sha256patch/vendor/workspace-patch/manifest.cborpatch/vendor/workspace-patch/manifest.sha256patch/vendor/workspace-patch/request-profile-configuration.cborpatch/vendor/workspace-patch/request-profile-configuration.sha256tests/patch-build-request.jsonltests/patch-build.shtests/patch-run.shtests/patch-runtime.sh
📜 Review details
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~175-~175: Ensure spelling is correct
Context: ...ld corroborates the exact Edict source, lawpack closure, digest sidecars, Core artifact...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (33)
CHANGELOG.md (1)
30-34: LGTM!README.md (3)
163-192: LGTM!
193-214: LGTM!
220-223: LGTM!docs/roadmap.md (1)
135-156: LGTM!tests/patch-runtime.sh (8)
1-38: LGTM!
40-109: LGTM!
111-160: LGTM!
162-204: LGTM!
206-233: LGTM!
268-348: LGTM!
417-471: LGTM!
573-617: LGTM!patch/src/apply-validated-patch.edict (1)
1-18: LGTM!edict.patch.application.json (1)
1-21: LGTM!patch/vendor/workspace-patch/adapter.cbor (1)
1-1: LGTM!patch/vendor/workspace-patch/adapter.sha256 (1)
1-1: LGTM!patch/vendor/workspace-patch/exports.cbor (1)
1-1: LGTM!patch/vendor/workspace-patch/exports.sha256 (1)
1-1: LGTM!patch/vendor/workspace-patch/manifest.cbor (1)
1-1: LGTM!patch/vendor/workspace-patch/manifest.sha256 (1)
1-1: LGTM!patch/vendor/workspace-patch/request-profile-configuration.cbor (1)
1-1: LGTM!patch/vendor/workspace-patch/request-profile-configuration.sha256 (1)
1-2: LGTM!patch/vendor/workspace-patch/SOURCE.md (1)
1-27: LGTM!patch-host/Cargo.toml.template (1)
1-17: LGTM!tests/patch-build-request.jsonl (1)
1-1: LGTM!tests/patch-build.sh (1)
1-86: LGTM!tests/patch-run.sh (1)
1-42: LGTM!patch-host/src/main.rs (5)
36-59: LGTM!
80-206: LGTM!
224-400: LGTM!
402-487: LGTM!
208-222: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNon-exhaustive error classification risks silent misclassification.
Unlike
settlement_kind()below (which exhaustively matches allExternalActionSettlementKindV1variants and would fail to compile on additions), this OR-pattern silently falls through newEdictExternalActionAdmissionErrorV1variants into"requestRejected". If a future compiler-artifact-level error variant is added upstream, it won't be classified as"compilerArtifactRejected"and this PR's careful distinction between compiler-authored defects and request-level rejections quietly degrades.♻️ Suggested defensive pattern
fn is_compiler_artifact_rejection(error: &EdictExternalActionAdmissionErrorV1) -> bool { - matches!( - error, - EdictExternalActionAdmissionErrorV1::Canonical(_) - | EdictExternalActionAdmissionErrorV1::ArtifactShape - | EdictExternalActionAdmissionErrorV1::RequestCardinality - | EdictExternalActionAdmissionErrorV1::CallableStepsPresent - | EdictExternalActionAdmissionErrorV1::MissingSemanticClosure - | EdictExternalActionAdmissionErrorV1::CoreDigestMismatch - | EdictExternalActionAdmissionErrorV1::TargetDerivationMismatch - | EdictExternalActionAdmissionErrorV1::CapabilityClosureMismatch - | EdictExternalActionAdmissionErrorV1::UnsupportedExpression - | EdictExternalActionAdmissionErrorV1::InvalidDigest - ) + match error { + EdictExternalActionAdmissionErrorV1::Canonical(_) + | EdictExternalActionAdmissionErrorV1::ArtifactShape + | EdictExternalActionAdmissionErrorV1::RequestCardinality + | EdictExternalActionAdmissionErrorV1::CallableStepsPresent + | EdictExternalActionAdmissionErrorV1::MissingSemanticClosure + | EdictExternalActionAdmissionErrorV1::CoreDigestMismatch + | EdictExternalActionAdmissionErrorV1::TargetDerivationMismatch + | EdictExternalActionAdmissionErrorV1::CapabilityClosureMismatch + | EdictExternalActionAdmissionErrorV1::UnsupportedExpression + | EdictExternalActionAdmissionErrorV1::InvalidDigest => true, + // Force a compile error when warp_core adds a new variant, so the + // classification here is revisited rather than silently defaulted. + EdictExternalActionAdmissionErrorV1::RequestLevelVariant(_) => false, + } }If
EdictExternalActionAdmissionErrorV1is#[non_exhaustive], exhaustive matching isn't possible from this crate — in that case, please confirm this classification list is still complete against the current variant set.> Likely an incorrect or invalid review comment.
Both witnesses accepted the two defects CodeRabbit found on #24. The build boundaries compared the vendored closure without checking that any external-action schema identity resolved, so the all-9/8/7 and all-b/c/d sentinel digests in the compiler sources passed. The build now refuses a malformed or sentinel identity, and requires each vendored resource sidecar identity to appear in the compiler source it claims to bind. The runtime boundaries never observed the writer epoch, so a static epoch identity with no predecessor linkage was indistinguishable from a durably fenced one. The golden paths now require a fresh epoch per write phase, exact predecessor and final-commit-digest linkage, a strictly advancing start LSN, no epoch on read-only phases, no epoch reused across the ordered path, and a persisted ledger and writer lease that stay bounded.
Advances both vendored closures to Edict df80f92a, which owns canonical edict.external-action-resource/v1 artifacts for the request input, settlement, and reconciliation identities and pins their exact digests in the generator's own source. Hello Echo byte-corroborates those artifacts rather than manufacturing them: the three new artifacts and their generator-owned identity sidecars are vendored per closure, and both application configs supply the exact artifact paths through externalActionResources so Edict recomputes and validates the complete closure before publishing Core or Target IR. Replacing the sentinel identities locally would have invented the compiler contract in the consumer, which is why #24 deferred this to Edict #180. Closed #180 upstream.
Both hosts hard-coded one writer epoch: a static epoch id, fixed fencing, process, host, and lease digests, and previous_epoch_id: None, re-acquired identically on every phase. Each phase is a separate process, so that handed the same fencing identity to restarted and overlapping hosts and defeated the point of epoch fencing for durable settlement. Echo c354d531 now persists the writer-epoch ledger across restarts and exposes FilesystemWalStore::acquire_fresh_writer_epoch, which takes the filesystem writer lease, rereads the ledger, closes an epoch left by a terminated process, and derives the successor from that predecessor's identity and final commit digest. Both hosts now call it and bind the acquired epoch into every transaction context instead of a constant. Hello Echo derives no epoch identity and reuses no fencing token. Write phases project the acquired epoch chain into their reports so the invariant is observable; read-only phases take no lease and report none. Against the advanced Echo pin the previous code no longer even starts: the persisted ledger rejects re-acquiring the static epoch with WriterEpochAlreadyActive.
Updates the vendored closure provenance to the Edict commit that owns the canonical external-action resources, states that the sidecars carry generator-owned resource identities rather than digests of the enclosing files, and records that the build refuses unresolved or sentinel identities. Documents writer-epoch fencing as producer-owned in both witness sections and adds the new witness guarantees to the changelog.
|
@coderabbitai review Both remaining threads were blocked on producer work that has since merged: Edict #180 landed as #181 ( Note the widened scope: the |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@patch-host/src/main.rs`:
- Around line 210-218: Document the intentional precedence directly beside the
budget checks in the surrounding input-validation flow: when both before and
replacement exceed max_file_bytes, observationExceedsFileBudget must be returned
first. Keep the existing check order and error mappings unchanged, and make the
comment explicit so future reordering does not alter the contract covered by
tests/patch-runtime.sh.
In `@README.md`:
- Around line 183-185: Add a command -v xxd >/dev/null prerequisite check in
tests/patch-runtime.sh alongside the existing jq and b3sum checks, ensuring the
script fails early when xxd is unavailable before the hex_digest path runs.
In `@tests/lib/check-resource-identities.sh`:
- Around line 42-98: Update names_slot and the declaring-line handling in
declared_identity so the digest search is limited to the current coordinate’s
clause: truncate text at the next coordinate before calling digest_in,
preventing a successor slot on the same line from donating its digest. Adjust
names_slot to recognize the coordinate occurrence that belongs to the clause
rather than only the first occurrence in the line, while preserving the existing
boundary checks. Add a single-line successor-adoption case to
tests/resource-identity-guard.sh.
In `@tests/lib/writer-epoch-assertions.sh`:
- Around line 42-54: Strengthen assert_chained_writer_epoch in
tests/lib/writer-epoch-assertions.sh:42-54 by requiring the predecessor
writerEpoch, its epochId and startedAtLsn, and both predecessor epochId and
wal.lastCommitDigest to match the expected 64-character hexadecimal format. In
tests/writer-epoch-assertions.sh:147-158, add mutation cases deleting each
predecessor writerEpoch field and a case deleting the predecessor writerEpoch
while setting the successor previousEpochId to null; require all cases to be
rejected.
In `@tests/patch-runtime.sh`:
- Around line 610-682: Extend the resulting-basis coverage around the existing
first/second settlement flow with a reconciled or ambiguous case where the
observed workspace bytes differ from the declared replacement. Assert that the
subsequent request’s requestBasis matches the basis derived from the actual
observed post-state, but differs from a resultingBasis derived over the declared
replacement; retain the existing positive and non-constant assertions.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro Plus
Run ID: 869528b1-33a1-4312-9c5f-27865644bbcb
📒 Files selected for processing (8)
CHANGELOG.mdREADME.mdpatch-host/src/main.rstests/lib/check-resource-identities.shtests/lib/writer-epoch-assertions.shtests/patch-runtime.shtests/resource-identity-guard.shtests/writer-epoch-assertions.sh
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-07-30T11:29:25.786Z
Learnt from: flyingrobots
Repo: flyingrobots/hello-echo PR: 24
File: patch-host/src/main.rs:660-716
Timestamp: 2026-07-30T11:29:25.786Z
Learning: In patch-host (e.g., patch-host/src/main.rs and related Rust code), treat durable writer-epoch acquisition as producer-owned by Echo (per Echo `#714`). Since FilesystemWalStore::open does not reconstruct epoch state, do not add any consumer-side prior-epoch reconstruction or synthetic fencing. Ensure the producer/patch-host provides durable writer-epoch acquisition and closure evidence, supports recovery/replay, implements fresh fencing, maintains monotonic LSN progression, and rejects/ignores stale or overlapping writers. Code implementing (or relying on) any consumer-side epoch reconstruction/fencing should be considered blocked until the Echo `#714` durable-epoch contract is satisfied.
Applied to files:
patch-host/src/main.rs
🔇 Additional comments (10)
patch-host/src/main.rs (2)
105-107: LGTM!Also applies to: 186-198
513-519: 🗄️ Data Integrity & IntegrationVerify the commit-count witness reads the WAL across
retry_phase.
commitCountBeforeandcommitCountAfterboth come from the samestore.read_commits().len(). IfFilesystemWalStore::read_commitsreturns an in-memory snapshot loaded inopen_read_store, the two values are constrained by the read lifecycle rather than observing WAL growth. Makeread_commitsreread the durable log, or open a fresh store forcommitCountAfter, depending on the intended model.CHANGELOG.md (1)
43-43: LGTM!Also applies to: 54-56
README.md (1)
200-216: LGTM!Also applies to: 249-252, 267-272
tests/lib/check-resource-identities.sh (1)
131-140: LGTM!Also applies to: 179-179
tests/lib/writer-epoch-assertions.sh (1)
16-31: LGTM!Also applies to: 55-69
tests/patch-runtime.sh (2)
8-10: LGTM!Also applies to: 43-52, 159-165, 400-405, 607-608, 857-864
684-714: LGTM!tests/resource-identity-guard.sh (1)
220-245: LGTM!Also applies to: 246-262, 264-293, 295-309
tests/writer-epoch-assertions.sh (1)
103-110: LGTM!Also applies to: 134-145
![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: f74e3fbf66
ℹ️ 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".
Five findings from CodeRabbit, the substantive one being a same-line variant of the successor-adoption evasion. declared_identity stopped a following slot from donating its digest across lines but not on one line. A source packing two clauses onto a single line let the successor digest satisfy the slot that declared none, invisible whenever the two vendored identities match. The tail after a coordinate, and each later line, are now truncated at the next coordinate before a digest is taken. The chained-epoch assertion required every successor field to exist and read the predecessor without the same requirement, so a predecessor carrying no writerEpoch yielded null and a successor reporting previousEpochId null satisfied the linkage by null == null. The mutation loop only damaged the successor and never reached it. Both sides are now required, with seven predecessor mutants and the explicit null-to-null case. The README named xxd as a prerequisite that the script never checked, so a missing xxd surfaced as a shell error inside hex_digest rather than as a missing tool. The two budget checks have a precedence the witness pins: an input oversized in both reports observationExceedsFileBudget. That is now stated beside the checks so a reorder cannot silently retarget it. The resulting-basis proof had no negative direction. In every success case the declared replacement and the observed post-state are the same bytes, so a producer deriving evidence from the request rather than the observation passed. The outcomeUnknown family is the only place they differ; the evidence is now required to vary with the observed bytes and not with the declared replacement. While adding that, found beforeContentDigest carries the observed bytes rather than the declared pre-state in an outcomeUnknown settlement. Echo is correct; my assertion was wrong. The reading is now pinned, since the field name does not convey it. Reported by CodeRabbit.
Round 7 — five, and a producer contract I had misreadHead
The Major one is the third variant of one evasionRound 5 closed the commented-coordinate route. Round 6 closed the across-lines route. This is the same evasion packed onto one line: It fails closed today only because the two vendored identities differ; make them equal and the slot with no pin passes. Your read that it is currently latent rather than exploitable is right, and it is fixed regardless. The negative-direction finding was the sharpestYou were right that every success case has the declared replacement and the observed post-state as identical bytes, so nothing distinguished a producer deriving evidence from the request. A producer deriving evidence from the declared replacement fails the first; one ignoring the observation fails the second. And a correction to myselfWriting that case, my assertion that Trend
33 total. Round 7 went back up, and three of the five were in guards I had already hardened twice. I am not going to claim a trend from this. State at
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c7cf27d00
ℹ️ 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".
Three findings, all about assertions that could not observe what they claimed. The evidence probes varied the worldline while claiming to vary only the observation or the replacement, so neither dependency was isolated: a producer deriving evidence from the worldline byte alone would have produced the same A/B/A pattern and satisfied both comparisons. The worldline is now held constant across all three. Reconciliation and uncertainty settlements come from write entrypoints distinct from apply and settle, and only their posture was checked. A regression reusing or mislinking an epoch during crash reconciliation would have passed both reconciliation outcomes despite the stated every-write-phase guarantee. Both are now required to chain to their claim. A retry reporting a null writer epoch proves nothing, because the phase supplies that null itself and taking an epoch changes the persisted ledger without changing the commit count. Both witnesses now snapshot the ledger bytes before the exact and conflicting retries and require them unchanged. Confirmed the check discriminates: a read phase leaves the ledger untouched and a write phase changes it. Reported by Codex.
Round 8 — three, all about assertions that could not see what they claimedHead First, a correction: I reported to my operator that round 8 had come back silent. It had not — your review landed at 13:54:29 and my watcher started at 13:55:00. I missed it by 31 seconds and blamed the tooling. The findings were sitting there the whole time.
The retry one is the sharpest thing found in eight rounds
Both witnesses now snapshot the ledger before the exact and conflicting retries and require it unchanged. Verified it discriminates rather than assuming: The worldline confound was mine, one round oldI added those probes in round 7 specifically to fix a different failure to isolate, and introduced a confound while doing it: worldlines 97, 98, 99 across the three cases. A producer keying off worldline parity alone would have produced A/B/A and passed both comparisons. The property I was proving was undermined by the variable I varied to prove it. Trend
36 total. Rounds 6-8 are 3, 5, 3 — flat, not converging. State at
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52f50a74a1
ℹ️ 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".
The reconciliation assertion checked only that externalEvidenceDigest, evidence, and resultingBasis agreed with one another, and the two-route basis probe added earlier exercised the apply path alone. ValidatedWorkspacePatchReconcilerV1 is a separate implementation, so a regression confined to it could have emitted one arbitrary value for all three fields and passed. Extracts the two-route derivation into request_basis_for and applies it to the reconciled settlement, in both directions: the retained basis must equal one derived over the observed post-state and must differ from one over the pre-state. Reported by Codex.
Round 9 — oneHead The reconciled success assertion checked only that its three evidence fields agreed with each other, and the two-route probe I added in round 6 exercised the apply path alone. Extracted the two-route derivation into The negative half matters: without it the equality could be satisfied by a reconciler retaining the wrong bytes. Trend
37 total. First single-finding round. State at
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ec9b2fa86
ℹ️ 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".
The reconciled case runs on worldline 82 while its two basis probes ran on 91 and 92, so neither comparison isolated the byte input: a producer deriving a basis from request context could map 82 and 91 to one value and 92 to another and satisfy both assertions while ignoring the bytes. This is the same confound Codex reported at round eight for the evidence probes. I fixed those and then reintroduced it one round later in the probes added for the reconciled binding. The two-patch chain carried it too, on worldlines 95 and 96, where its negative assertion could be met by the worldline difference rather than the byte difference; that was not reported. Every basis and evidence comparison now varies only the path and the bytes. The contract is stated at request_basis_for, since the worldline parameter is the affordance that keeps producing this. Reported by Codex.
Round 10 — one, and it is my mistake for the third timeHead You reported this exact confound at round 8 for the evidence probes. I fixed those, then reintroduced it one round later in the probes I added for the reconciled binding: the reconciled case runs on worldline 82, its probes ran on 91 and 92. Same class, same file, one round apart. While fixing it I audited every other comparison of this shape and found a third instance you did not report: the two-patch chain used worldlines 95 and 96, so its negative assertion — that the two settlements do not share a basis — could be satisfied by the worldline difference rather than the byte difference. Also fixed. Every basis and evidence comparison now varies only the path and the bytes:
And they still discriminate: The worldline parameter on Trend
38 total. Two consecutive single-finding rounds, and both were confined to test scaffolding I added during this review rather than to the change under review. Nothing since round 5 has touched shipped behaviour except the observation-budget split. State at
|
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Self-audit while round 11 is pendingCodex's last review was 14:24:46 and this head has been up ~25 minutes with no response and no 👍, so round 11 is genuinely outstanding rather than clean. CodeRabbit is rate limited again. Recording that plainly because I misread a 31-second gap as silence two rounds ago. Rather than idle I swept
The one real thing it turned up is filed as #26 rather than fixed here: the observation witness cannot bind a settlement to its basis evidence, because That is a gap in what the witness can see, not a defect in shipped behaviour, and closing it means adding report fields to No code change in this update. Head is unchanged at |
The resource identity guard had become a second Edict parser: 197 lines of shell and awk covering declaration syntax, comments, coordinates, same-line clauses, sidecar terminators, digest grammar, and placeholder recognition, plus 328 lines of tests for that parser. It produced findings in three consecutive review rounds, each an evasion of its own parsing rather than a defect in the change under review. Every evasion required the producer to ship a malformed compiler source, and in exactly that case the build already compares that source byte-for-byte against the generator-owned fixture. Edict #181 owns canonical resource construction, identity derivation, closure validation, and rejection of malformed, missing, substituted, and sentinel resources, and the build invokes that validator through the public application build. Betting an awk script preserves Edict semantics more faithfully than Edict is not defense in depth. Hello Echo corroborates Edict artifacts and invokes Edict's validator. It does not partially reparse Edict source. If the public build can accept a malformed fixture, that is an Edict defect. The writer-epoch assertions stay: they witness dynamic Echo behaviour, which is the consumer's responsibility. Adds producers.lock.json with the exact Edict and Echo commits, enforced at every build boundary. The compatible pair was an operator convention, so a consumer on a stale producer was undetectable; verified the check rejects one. Adds a CI workflow that reads the lock, checks the producers out at those commits, and runs the complete witness gate with shell syntax, formatting, and strict clippy. Four witnesses have never run on GitHub, which is why real producer-pin failures were invisible on the pull request page.
Without the secret, actions/checkout reports 'Input required and not supplied: token', which does not say which token, why it is needed, or what to create. A preflight step now names the secret, the two repositories, and the scope.
I assumed the producer repositories were private and required a PRODUCER_READ_TOKEN secret, which does not exist, so the first CI run failed at checkout before running a single witness. Both repositories are public and actions/checkout needs no cross-repository token to read them. The pinned commit remains the whole contract: a moved branch cannot change what a run proves.
Simplified, pinned, and under CIHead Removed the consumer-owned Edict parser
That guard had become a second Edict parser written in shell and living in this repository: declaration syntax, comments, coordinates, same-line clauses, sidecar terminators, digest grammar, placeholder recognition. It produced findings in three consecutive review rounds, and every one was an evasion of its own parsing rather than a defect in the change under review. Every evasion also required the producer to ship a malformed source — the exact case where the build already compares that source byte-for-byte against the generator-owned fixture. Edict #181 owns canonical resource construction, identity derivation, closure validation, and rejection of malformed, missing, substituted, and sentinel resources. The build invokes that validator through the public application build. Betting an awk script preserves Edict semantics more faithfully than Edict is not defense in depth. The rule is now written into the README and
If the public build can accept a malformed fixture, that is an Edict defect. The writer-epoch assertions stay. Those witness dynamic Echo behaviour, which is the consumer's responsibility. Pinned the producer pair
The compatible pair was previously an operator convention, which is precisely why advancing it looked like a repository failure rather than a pin change. Verified the check rejects a stale checkout: Added CIReads the lock, checks Edict and Echo out at those commits, and runs the complete gate: all four witnesses, the hermetic assertion test, shell syntax, A correction: the first CI run failed because I assumed the producer repositories were private and required a Local gate at this head
Merge gateP3-P5 findings go to backlog unless they expose a false semantic claim. Merging on the expected head SHA so the branch cannot move under the decision. |
The observation witness links the producer checkouts and re-runs the build
through those links, to prove a relative producer path is canonicalized before
it reaches the generated Cargo manifest. It linked the path as given.
A symlink target is resolved against the directory holding the link, so a
relative producer path linked from a nested build directory dangles. Every
caller had passed absolute paths, so the probe had never been exercised the way
it was written to be. CI passes relative paths and the witness failed at once:
fatal: cannot change to '.build/effect-tests/relative-repos/edict'
The link path stays relative, which is what the probe exercises. The target is
now resolved. Verified every witness passes under both relative and absolute
producer paths.
Found by CI on its first successful execution.
The oversize-replacement and oversize-observation cases build bodies at the file budget, which is 131,072 hex characters. Linux caps a single argument at MAX_ARG_STRLEN, which is exactly 131,072 bytes, so passing one through --arg fails with 'Argument list too long'. macOS permits far larger arguments, so these cases had passed on every developer run and could not fail there. make_case now writes both bodies to files beside the case and reads them with --rawfile, which has no such limit. The generated request is unchanged, which the suite's digest and golden bindings confirm. Only the patch witness builds bodies of that size; the observation witness uses short literals, so it is left alone rather than changed speculatively. Found by CI.
|
@codex review please Final bounded review. Head
What changed since 1ec9b2fDeleted Added Added CI that reads the lock, checks Edict and Echo out at the pinned commits, and runs the full gate. Two fixes CI found on its first two executions, both environmental and both invisible to every prior local run and to all eleven review rounds:
Gate stateLocal: five witnesses pass under both relative and absolute producer paths; If this comes back with no P0–P2, I merge on that exact SHA. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e14abd38d2
ℹ️ 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".
The lock compared rev-parse HEAD, which reports the pinned commit regardless of uncommitted changes. All three build boundaries then compile the modified Edict and Echo sources, so a run could claim the pinned producer pair while testing different code. The vendored artifact comparisons do not cover this. They check the fixtures, not the compiler and host crates the build uses. Verified in both directions: a clean checkout at the pinned commit passes, and a checkout at that commit with one modified file is refused by path. Reported by Codex.
Final review: one P2, fixedHead Valid, and it was a false semantic claim rather than a hardening suggestion, which is exactly the bar I asked you to hold to. Proved the mechanism in an isolated repository rather than dirtying the real producer checkouts: The lock now also requires a clean worktree. Verified both directions: the pinned checkouts pass, and a checkout at the pinned commit with one modified file is refused by path. On the deletionYou did not object to removing GateFive witnesses pass locally under relative and absolute producer paths. Merging on the exact SHA once CI is green on it. |
1 participant
Proves basis-bound validated patch application end to end: Edict declares the request, Echo durably coordinates it, and only the capability-rooted adapter touches the workspace.
Producer pin
producers.lock.jsonpins the exact producers this branch is proven against:df80f92ad6242c6da31a64224666fd37aa43b0d0c354d531679861fb7bbd52ab7b7703807909ab86Every build boundary calls
tests/producer-lock.shand refuses a checkout that is not those commits. CI reads the same file. The compatible pair was previously an operator convention, which is why an advance in either producer looked like a repository failure rather than a pin change.What changed
df80f92a, which replaces the sentinelworkspace.patch.*andworkspace.snapshot.*digests with the exact identities of vendorededict.external-action-resource/v1artifacts, supplied throughexternalActionResources.FilesystemWalStore::acquire_fresh_writer_epochagainst Echoc354d531, replacing a static epoch identity that could not fence overlapping or restarted hosts. Against the advanced pin the previous code did not start at all.observationExceedsFileBudgetandreplacementExceedsRequestBudgetas distinct obstructions, so a replacement the encoded request cannot carry is refused for a stated reason instead of as a malformed request.Removed
tests/lib/check-resource-identities.shand its test. That guard had become a second Edict parser written in shell and living in this repository. Edict owns canonical resource construction, identity derivation, closure validation, and rejection of malformed, missing, substituted, and sentinel resources; the build corroborates every artifact byte-for-byte and invokes that validator. Hello Echo corroborates Edict artifacts; it does not partially reparse Edict source.Blockers
None. Both producer dependencies have merged: Edict #180 landed as flyingrobots/edict#181, Echo #714 landed as flyingrobots/echo#715.
Deferred
#26 — the observation host does not project enough basis and evidence fields to bind a settlement to the observed bytes. An observability gap rather than a behaviour defect, and a new capability rather than a repair.
Closes #11
Closes #23
Closes #25