Prevent compliance self-acceptance through nullable reviewer metadata - #386
Conversation
Record the proposed high-risk Phase A for Delivery #385 under M1 review semantics. Keep the runtime unchanged until its own exact semantic approval, and distinguish new promotion from historical replay.
samovers
left a comment
samovers
left a comment
There was a problem hiding this comment.
PR #386 — design review
Verdict: No blocking design findings. Ready for the semantic-approval step, not for merge as a completed repair.
Reviewed head: 047266833d04797cce773a3cd221b8363bda78c0.
Base: e50ae95f43d0e73b12113463d0e4ea85eebdc14b.
The diff adds only the 232-line self-review eligibility RFC; it does not yet change runtime behavior or add regression tests. This is a Phase A design review, not implementation verification, task-user semantic approval, baseline admission, merge authorization, or production permission.
Technical assessment
The proposed change addresses the actual cause.
In ReviewPromotionGate.run, explicit reviewerPartyRef: null currently escapes both the compliance self-review condition and the distinct-reviewer guard. However, emit_self_review_promotion records the acting party as the reviewer regardless of that hint. Removing the compliance condition's reviewer-hint comparison therefore aligns eligibility with the review act that would actually occur:
ctx.commit_class == "COMPLIANCE_ASSERTION" and confirmedThat is the appropriate narrow correction. Rejecting null at ingress or inventing a fallback reviewer would change more behavior without improving this enforcement boundary.
Sources: kernel/stages.py:625–665, kernel/emission.py:169–193.
The existing independent-review path remains compatible with the proposal.
The normalizer sets acceptance_target only for GOVERNANCE_DECISION, so adding review-target fields to a compliance submission cannot bypass the proposed routing through the queue-emission branch. The queue validator checks the stored asserter against the actual acting party and refuses disallowed self-review before emitting a review decision. The proposed compliance predicate does not interfere with a distinct reviewer's separate governance decision.
Sources: kernel/stages.py:274–279, kernel/validators.py:561–587.
The historical limitation is explicit rather than concealed.
Replay runs before promotion eligibility and can return the original accepted references without creating another acceptance. Consequently, this repair will not remove incorrect historical acceptances or stop their matching replay responses from exposing those references. The RFC preserves that behavior deliberately and requires a base-to-candidate probe against retained historical records. This is consistent with the stated scope, not an overlooked promise to repair history.
Sources: kernel/emission.py:477–589, proposed RFC — historical replay.
The verification plan tests the outcome that matters.
It requires more than a REQUIRE_REVIEW response: the same null-origin assertion must remain pending, survive the asserter's refused acceptance, and then be accepted once by an authorized distinct reviewer. It also requires persisted-record and edge checks, a demonstrated failure on the unmodified base, HTTP and public-pipeline coverage, and replay/digest-conflict checks. Those obligations are already in the proposal; I found no need to expand this into a general eligibility framework or broader lifecycle redesign.
Source: proposed RFC — decision invariants and falsifiable evidence.
Validation and remaining limits
I confirmed that lightweight workflow run 34712299485, attempt 1, completed successfully for the reviewed SHA. That is not evidence that the eligibility defect has been fixed—the implementation is absent from this head. I did not execute runtime tests or PostgreSQL probes during this review.
Recommendation: Proceed with the proposed decision card without design changes. After approval, keep the runtime correction, same-claim lifecycle regression, historical-replay probe, and necessary documentation/inventory updates together in this PR. Implementation review and fresh execution evidence remain required before merge.
No new blocking design findings or additional scope requirements.
PR #386 — independent Phase A design review (executed)Reviewed head: Verdict: one Blocker, one Follow-up, two Preferences. The proposed predicate is correct and I measured it closing the defect end to end, including the same-claim queue lifecycle and the history non-effect the RFC defers to post-approval. The Blocker is not in the predicate: it is that this decision card, whose stated trust boundary is self-review eligibility enforcement before new accepted-force emission, leaves a second, larger, undisclosed self-acceptance on the samedecision point and does not name it. Position of this passThere is exactly one review on this PR: 5187685070 ( I am the PR author's own account. GitHub will not let me APPROVE or REQUEST_CHANGES here, and a self-review does not satisfy an "independent review" gate. Both reviews on this PR are Identity and scope, re-derivedIndependent clone, E06 — the base-to-candidate history probe the RFC defers to post-approvalI ran it now, because it is the strongest claim in the RFC and the cheapest to get wrong. One persistent disposable database; phase 1 executes on the unmodified base bytes and creates the fictional bad acceptance; phase 2 executes on the patched bytes against that same retained database.
Exactly the RFC's §"Historical replay is deliberately preserved", measured: a matching historical accepted-null key still hands back the old accepted references and creates no new assertion, review, consequence or edge; the same body under a new key is a new eligibility evaluation and is refused. The Material limit paragraph is accurate, and it is the right thing to have written down. No regression in the suites the RFC names
The two are Other RFC statements I checked in code
Checked and decided were not findingsRecording these so the next reviewer does not re-spend the time.
What could not be checked, and why
What my method made easier than production
Bottom lineThe predicate is right, minimal, and achieves everything E01–E06 promise — I measured all of it, including the history probe the plan defers. Approve version 1 only after B1's three paragraphs are in, because approval freezes E01–E07 and E04 currently reads as a closed list that is not closed. F1 and the two Preferences cost a sentence each and should ride along. Then the real work is the implementation head: fresh evidence, nothing reused from #384, and the queue assertion written against the derived disposition rather than |
Remove the reviewer-hint comparison from compliance eligibility so explicit null cannot create a new self-accepted review. Preserve raw replay identity and the usable queue, cover the same-claim lifecycle, and document the retained history limit and separate observation follow-up. Delivery #385; approved OFARM2-LEGACY-SELF-REVIEW-ELIGIBILITY-001 version 1. Local regression 6/6, compatibility 208/208, conformance 23/23, production closure 2/2 and package checks pass.
|
Exact-head content review completed for PR #386 / Delivery #385 at 81f1ce1 with zero demonstrated in-scope Blockers. The complete coordinated report is preserved in the PR description; SHA256 a36315eec436767776d5c4cdd47e5985fb8635df5da026caa9b9415c97c9c4db. Local verification and residuals are disclosed there. This comment invokes only the existing baseline admission mechanism; it is not human semantic approval, merge authority, publication success or production permission. OFARM2_BASELINE_ADMISSION |
samovers
left a comment
samovers
left a comment
There was a problem hiding this comment.
PR #386 — independent Phase A design review (executed)
Reviewed head: 047266833d04797cce773a3cd221b8363bda78c0.
Base: e50ae95f43d0e73b12113463d0e4ea85eebdc14b (tree 17b134856f75391df50cf2dc298dc3bcef6c6208, parents b6017da1dbfac80b5c2e98641aaa34d96691d087 + 9b37d12cc2211b50e66eb3abe8c3e58b8e96f402 — the RFC's "reviewed runtime tree" claim is exact).
Verdict: one Blocker, one Follow-up, two Preferences. The proposed predicate is correct and I measured it closing the defect end to end, including the same-claim queue lifecycle and the history non-effect the RFC defers to post-approval. The Blocker is not in the predicate: it is that this decision card, whose stated trust boundary is self-review eligibility enforcement before new accepted-force emission, leaves a second, larger, undisclosed self-acceptance on the same decision point and does not name it.
Position of this pass
There is exactly one review on this PR: 5187685070 (samovers, COMMENTED, 2026-09-12T19:08:22Z, 4,967 chars), which states in its own words "I did not execute runtime tests or PostgreSQL probes during this review." No issue comments, no inline findings. This pass is therefore the first executed review at this head. It confirms four of that review's source-inspection claims by measurement, supplies the E06 base-to-candidate probe that review and the RFC both defer, and adds one finding neither has.
I am the PR author's own account. GitHub will not let me APPROVE or REQUEST_CHANGES here, and a self-review does not satisfy an "independent review" gate. Both reviews on this PR are samovers. Do not treat the gate as met.
Identity and scope, re-derived
Independent clone, git fetch origin refs/pull/386/head:pr386:
| Fact | Measured |
|---|---|
| Files changed | 1 — docs/rfcs/OFARM_Legacy_Self_Review_Eligibility_RFC_v0_1.md |
| Additions / deletions | 232 / 0 (API: additions 232, deletions 0, changed_files 1, commits 1) |
| RFC blob | b1dafbc0992c91d1c42b65733718e36b69fe1b2e |
| RFC size | 232 lines, 16,183 bytes, SHA-256 cc644f247308bbb1bf5b61db6ebec81f015a7dd5412c6049c247c3d8ffbf1b7e |
git diff --check e50ae95 pr386 |
clean |
| PR state | open, draft |
| Referenced work | #385 open ("Prevent nullable reviewer metadata from permitting legacy compliance self-accept"), #180 open (Tracking Epic) — both exist and match |
Runtime, tests, inventory and prior decision records are untouched at this head. That matches the status file and the PR body.
Method and environment
Everything below was executed against real PostgreSQL through the real legacy HTTP route (POST /commit, POST /review/accept) with the repo's function-isolated disposable-database fixture and fictional kernel.demo records only. Probe files were written into my clone, run, and deleted; the clone is clean.
Where a measurement needed the candidate semantics, I applied the RFC's §"Smallest proposed correction" literally — delete the reviewer-hint comparison from the confirmed-compliance condition, nothing else:
- if (ctx.commit_class == "COMPLIANCE_ASSERTION" and confirmed
- and sub.get("reviewerPartyRef", ctx.acting_party) == ctx.acting_party):
+ if ctx.commit_class == "COMPLIANCE_ASSERTION" and confirmed:kernel/stages.py 760 → 759 lines; 1 insertion, 2 deletions. That is the whole patch I measured.
Environment honesty. CPython 3.11.15, PostgreSQL 16.13, dependencies installed from PyPI at current versions (psycopg 3.3.5, fastapi, starlette, pydantic, jsonschema, pytest) — not the pinned 3.12.13 / 17.10 / hash-locked requirements-review-baseline.lock set. I could not build the pinned interpreter today: both codeload.github.com and www.python.org are refused by this session's egress (CONNECT tunnel failed, response 403). Consequences stated where they matter, and in "could not check" below.
Blocker B1 — the decision card does not disclose that an entire promoting class self-accepts on the same decision, unguarded
Violated invariant. D8 scopes self-review to routine operation claims; D17 extends it narrowly to bounded farm-owned structure and says so explicitly — "extends D8 narrowly as a recorded decision, not an accidental consequence of the generic promotion path." After this correction, OBSERVATION_ASSERTION still self-accepts on the very decision this RFC governs, under no recorded decision at all, and needs no metadata trick to do it.
| High-risk field | Value |
|---|---|
| Entry point | POST /commit on the legacy M1 transport (kernel/legacy_m1/api.py), the same route the RFC names as "the real negative". Public production governed routes are closed (kernel/api.py:74-83 blocks /commit, /review/*, views) — same bound as the defect being fixed, not a weaker one. |
| Actor | A party holding OBSERVE_CREATE_OBSERVATION and REVIEW_ACCEPT on the farm — the one-person-farm grant shape D8 exists for. demo.FARMER holds it. |
| Path | ReviewPromotionGate.run → no routing condition matches OBSERVATION_ASSERTION at all (kernel/stages.py:630, :645 and :658 are compliance / structure / distinct-named-reviewer only) → review_route_reasons empty → confirmed true → REVIEW_ACCEPT authority check passes → emit_self_review_promotion() (:730). |
| Preconditions | Well-formed submission, resolving durable evidence, confirmAccept: true, reviewer metadata omitted (or null, or self-named). Nothing malformed, nothing null-dependent. |
| Consequence | PROMOTE_ACCEPTED; AssertionRecord IN_FORCE; one ReviewDecision whose decidedByPartyRef equals assertedByPartyRef; one AcceptedEventConsequence (ACCEPTED_OBSERVATION_OCCURRENCE_STATE → STATE_CHANGE_ACCEPTED). An accountable-review record naming the asserter as its own reviewer — exactly the artifact E01/E03 exist to prevent. |
Measured at base e50ae95 and unchanged under the candidate patch, over real HTTP:
observation/reviewer-omitted 200 PROMOTE_ACCEPTED reviews=1 consequences=1
observation/reviewer-null 200 PROMOTE_ACCEPTED reviews=1 consequences=1
observation/reviewer-self 200 PROMOTE_ACCEPTED reviews=1 consequences=1
observation/reviewer-distinct 200 REQUIRE_REVIEW reviews=0 consequences=0
decidedBy = assertedBy = party:demo.farmer.one ; assertionType OBSERVATION_ASSERTION ; claimState IN_FORCE
The same class, routed to the queue, is refused to the same actor — which is the direct/queued asymmetry in one pair of calls:
observation/no-confirm-capture 200 RETAIN_DRAFT (pending assertion)
observation/queue-accept-by-asserter 200 RETAIN_DRAFT HUMAN_APPROVAL_REQUIRED
observation/queue-accept-by-distinct 200 PROMOTE_ACCEPTED reviews=1 consequences=1
kernel/validators.py:578-587 is the refusing code, and its comment is the point: "D8 holds at the queue door for BOTH verbs", enforced as an allowlist over policy.SELF_ACCEPTABLE_ASSERTION_TYPES. Measured: SELF_ACCEPTABLE_ASSERTION_TYPES == {"OPERATION_CLAIM_ASSERTION"}; promoting classes == {COMPLIANCE_ASSERTION, OBSERVATION_ASSERTION, OPERATION_CLAIM, STRUCTURE_ASSERTION}. So the queue door admits one class; the direct door admits three, one of which has no decision behind it.
This is not my inference from unexercised code. The repo already encodes it as a positive control: kernel/tests/test_review_fixes.py::test_h3_promoting_observation_with_evidence_promotes asserts PROMOTE_ACCEPTED for a FARMER-asserted, confirmAccept: true observation. I grepped DECISIONS.md and ERRATA.md: no decision extends self-acceptance to observations. docs/REVIEW_DISPUTE_SEMANTICS.md:174 states the opposite — "D8/D17 grant a farmer self-acceptance of bounded classes."
Why this blocks the decision rather than becoming a Follow-up. The deliverable under review is a decision card about to be approved verbatim, and the RFC binds itself: "Changes to capability, effects/non-effects, authority, E01–E07 ... require a new decision version and exact approval." Two of its statements about the surface it governs cannot be verified as written without discovering this:
- "It enforces the existing D8 restriction" — it enforces D8 for one of the two classes that violate it.
- E04 — "Existing permitted routine-operation and bounded farm-owned-structure self-acceptance remain functional ... No new eligibility guarantee for other classes." A reviewer checking E04 will confirm both named paths still work and conclude the permitted set is {routine operation, bounded structure}. There is a third path in force, and E04's wording ("no new guarantee") reads as "we are not widening" rather than "a larger unguarded self-acceptance remains".
- The containment table assigns "Routing before new acceptance" to "Existing promotion gate and emitters". For observations the existing promotion gate performs no routing whatsoever.
The same workspace already chose the correct remedy shape for exactly this situation one PR ago: #384 merged with its defect open because docs/REVIEW_DISPUTE_SEMANTICS.md:110-117 states the residual in plain words and names Delivery #385 as its owner. B1 asks for the same treatment, nothing more.
Smallest acceptable fix — all of it text, all of it in this PR:
- In the RFC, state the measured residual where the problem and scope are set: confirmed
OBSERVATION_ASSERTIONself-acceptance is reachable on the legacy route by its own asserter with no reviewer metadata at all, is refused at the queue door for the same actor, and is out of scope here. Reword E04 so it enumerates what actually self-accepts rather than implying a closed pair, and soften "enforces the existing D8 restriction" to "enforces the existing D8 restriction for confirmed compliance assertions". - In the Phase B documentation update this RFC already owns (
docs/REVIEW_DISPUTE_SEMANTICS.md), add the residual next to the #385 note it will be replacing, in the same form #384 used. - Open a named Delivery for the runtime part and cite it, as #385 was cited.
I am not asking for a runtime change to the observation path in this PR. Narrowing it removes a capability that currently works and is its own decision with its own approval. The Blocker is the silence, not the scope.
Counter-argument, stated. AGENTS.md defines a Blocker as a demonstrated in-scope failure, and the RFC's non-goals list "observation semantics". On a strict reading of scope this is a Follow-up. I rejected that reading because the artifact under review is a decision record about self-review eligibility, the omission is on the exact axis it governs, the defect is measured rather than argued, and the fix is three paragraphs the author is already writing. If the task user disagrees, demote it — but do not approve version 1 with the text unchanged, because approval freezes E01–E07.
Follow-up F1 — the identical comparison survives at :646; I measured it unreachable, so record that, because the next change can make it live
EXC-004 deletes the compliance reviewer-hint comparison as "misleading". The structure condition at kernel/stages.py:646 keeps the byte-identical expression:
and sub.get("reviewerPartyRef", ctx.acting_party) == ctx.acting_partySame defect shape: reviewerPartyRef: null makes it False, and :658 treats None as "no distinct reviewer", so a structure assertion outside the D17 bounded class with a null reviewer would take the self-review path. I tried to reach it five ways, all with reviewerPartyRef: null and confirmAccept: true (so policy.structure_self_acceptable(...) is False):
structure/no-schemaVersion 200 RETAIN_DRAFT EVIDENCE_INSUFFICIENT
structure/empty-schemaVersion 200 RETAIN_DRAFT EVIDENCE_INSUFFICIENT
structure/null-schemaVersion 200 RETAIN_DRAFT EVIDENCE_INSUFFICIENT
structure/null-payload 200 RETAIN_DRAFT EVIDENCE_INSUFFICIENT
structure/empty-payload 200 RETAIN_DRAFT EVIDENCE_INSUFFICIENT
structure/unrecognized-kind ×2 200 RETAIN_DRAFT EVIDENCE_INSUFFICIENT
(positive control) structure/recognized-payload 200 PROMOTE_ACCEPTED
Every hostile shape dies at EVIDENCE_SUFFICIENCY before promotion, so the :646 null hole is latent, not live — which is why it is a Follow-up and not a second Blocker. It is latent for a reason worth writing down: the only way to make a structure payload kind committable is to add it to policy.STRUCTURE_PAYLOAD_IDENTITY_TYPE, and that same addition is what makes structure_self_acceptable return True. The hole opens the moment those two stop being the same act. Record the measurement so a future reviewer does not have to re-spend it, and so nobody deletes the compliance comparison as "misleading" while leaving its twin with no note.
Preference P1 — the predicate keeps the denylist shape that produced this defect, one file away from the allowlist that did not
The proposal hard-codes the class name: ctx.commit_class == "COMPLIANCE_ASSERTION" and confirmed. The queue door (validators.py:578-587) enforces the same rule as an allowlist over policy.SELF_ACCEPTABLE_ASSERTION_TYPES, and policy.COMMIT_CLASS_TO_ASSERTION_TYPE already maps all four promoting classes into that vocabulary. A denylist of per-class conditions is what let compliance be guarded by a metadata comparison and observation by nothing; the allowlist form cannot have that failure mode, because a new promoting class defaults to "needs a reviewer".
I am filing this as a Preference, not a Blocker, and EXC-006's rebuttal is sound as far as it goes: switching to an allowlist changes behavior for observations, i.e. it is the runtime decision B1 explicitly does not demand here. But the decision that the predicate stays a per-class denylist should be a stated choice with its reason, not the residue of "remove one comparison". One sentence in EXC-006 naming the allowlist alternative and why it is deferred would close it.
Preference P2 — write the Phase B queue assertion against the derived disposition, not claimState
Measured under the candidate, one claim through the full E03 sequence:
routed_outcome REQUIRE_REVIEW
queued_accept_outcome PROMOTE_ACCEPTED (inForceResultCategory COMPLIANCE_FACT)
claimState_after_queued_accept PENDING_REVIEW
review_edges 1
reviews / consequences 1 / 1
unreachable_authoritative_records []
The direct self-review path this fix removes left the AssertionRecord at claimState: IN_FORCE (measured at base). The queued path leaves it at PENDING_REVIEW forever and carries acceptance on the ReviewDecision + REVIEW edge + consequence — correct under D20 and Kernel rule 1 (the queued assertion is never edited; disposition is derived). It is pre-existing and already exercised by test_94, so it is not a finding. But it is the obvious way to write E03's test wrong: a Phase B assertion that the accepted claim reaches IN_FORCE will fail, and "fixing" it by editing the record would be a real regression. Worth one line in the verification plan.
What I reproduced and confirmed
The defect, at the base the RFC names
COMPLIANCE_ASSERTION + confirmAccept: true + reviewerPartyRef: null, asserted by demo.FARMER over POST /commit at e50ae95:
200 PROMOTE_ACCEPTED assertions=1 reviews=1 consequences=1
ReviewDecision.decidedByPartyRef = party:demo.farmer.one (the asserter), decisionOutcomeState ACCEPTED
AssertionRecord.claimState = IN_FORCE
The full reviewer-metadata matrix at base — eight shapes, one of them an acceptance:
reviewerPartyRef |
base | candidate |
|---|---|---|
| omitted | REQUIRE_REVIEW | REQUIRE_REVIEW |
null |
PROMOTE_ACCEPTED (1 review, 1 consequence) | REQUIRE_REVIEW |
self (demo.FARMER) |
REQUIRE_REVIEW | REQUIRE_REVIEW |
distinct (demo.ADVISOR) |
REQUIRE_REVIEW | REQUIRE_REVIEW |
"" / 0 / false / [] |
REQUIRE_REVIEW | REQUIRE_REVIEW |
Present-null sweep over twelve neighbouring keys on the same valid compliance submission, to confirm #385 is right-sized and not a symptom of a general null problem. Only reviewerPartyRef converts a refusal into an acceptance:
reviewerPartyRef PROMOTE_ACCEPTED <-- the defect
payload / evidenceRefs RETAIN_DRAFT EVIDENCE_INSUFFICIENT
supersedesConsequenceRef RETAIN_DRAFT EVIDENCE_REFERENCE_UNAVAILABLE
subjectType RETAIN_DRAFT IDENTITY_UNRESOLVED
subjectRef RETAIN_DRAFT SCOPE_NOT_AUTHORIZED
ingressChannel HTTP 422
targetScopes, requestedPromotionTarget, eventTime, actingAgentRef, aiAssistance
REQUIRE_REVIEW (identical to omitting the key)
E01–E07, measured against the candidate patch
| ID | Result |
|---|---|
| E01 | Fresh null-reviewer confirmed compliance → REQUIRE_REVIEW / HUMAN_APPROVAL_REQUIRED, 1 pending assertion (claimState PENDING_REVIEW), 0 reviews, 0 consequences. The same input on the unmodified base → PROMOTE_ACCEPTED with both. Both halves of E01 demonstrated. |
| E02 | All eight reviewer shapes → review-required (table above). Raw metadata never becomes reviewer identity: no ReviewDecision is emitted on any of them. false / omitted keep their existing capture behavior; malformed confirmAccept keeps its pre-transaction 422 (PR #384's parser, unchanged here). |
| E03 | Continued from E01's exact assertion: asserter's /review/accept → RETAIN_DRAFT HUMAN_APPROVAL_REQUIRED, claim not consumed (PENDING_REVIEW, 0 REVIEW edges); demo.ADVISOR then accepts once → PROMOTE_ACCEPTED, COMPLIANCE_FACT, 1 review / 1 consequence, decidedByPartyRef = party:demo.advisor.one; duplicate → RETAIN_DRAFT SUPERSEDED_RECORD_USED. A caller-supplied decisionTime: "2001-01-01T00:00:00Z" is ignored — emitted decidedAt was the server time. The "usable queue" promise holds. |
| E04 | Routine-operation self-review and bounded D17 structure self-acceptance both still PROMOTE_ACCEPTED under the patch. Wrong-typed and unbounded-structure cases gain nothing. See B1 for what this invariant fails to enumerate. |
| E05 | No regression in the authority/evidence/retirement suites — test_correction_authorization.py, test_correction_transactions.py pass identically at base and candidate. Routing emits pending state only: 0 consequences, 0 retirements on every routed case. |
| E06 | Two-phase probe, run for real (see next section). |
| E07 | unreachable_authoritative_records() == [] after the queue lifecycle. One-file diff, clean whitespace. Transaction ownership, signing, production closure untouched by a one-line predicate in a gate — I inspected rather than audited, and make no full-lifecycle claim. |
E06 — the base-to-candidate history probe the RFC defers to post-approval
I ran it now, because it is the strongest claim in the RFC and the cheapest to get wrong. One persistent disposable database; phase 1 executes on the unmodified base bytes and creates the fictional bad acceptance; phase 2 executes on the patched bytes against that same retained database.
phase=base NEW_REQUEST PROMOTE_ACCEPTED
+17 records, +11 edges, +7 gate-log rows, +1 idempotency row
assert:2ad4293844394c8b review:14c5366453fd4fd6 conseq:fb93905a84a247ee
phase=candidate (same key, same body) REPLAY_REUSED_RESULT / REPLAY_MATCH_REUSED_RESULT
returns the SAME assert/review/conseq refs, replayOf cir:e4a0078728e34894
+3 records (commitingressrequest, commitingressresult, promotiontrace),
+1 gate-log row, +0 edges, +0 idempotency rows
phase=candidate (same body, FRESH key) REQUIRE_REVIEW HUMAN_APPROVAL_REQUIRED
0 reviews, 0 consequences
Exactly the RFC's §"Historical replay is deliberately preserved", measured: a matching historical accepted-null key still hands back the old accepted references and creates no new assertion, review, consequence or edge; the same body under a new key is a new eligibility evaluation and is refused. The Material limit paragraph is accurate, and it is the right thing to have written down.
No regression in the suites the RFC names
test_review_confirmation.py, test_m2_review.py, test_correction_authorization.py, test_correction_transactions.py, test_conformance.py, test_review_fixes.py, test_stages.py:
candidate: 2 failed, 229 passed (136s)
base: 2 failed, 229 passed (147s) <- same two tests, identical
The two are test_conformance.py::test_04_gate_sequencing_fixtures_live and ::test_10_document_assembly_freeze_trace, both failing on assert result["refused"] is False. They are an artifact of my harness, not of the patch: test_conformance.py alone passes 23/23, and test_conformance.py + test_m2_identities.py with the patch pass 42/42. Running the conformance suite alongside other files in one session pollutes its session-scoped store. Calibration is what matters here and it is clean — the failure set is byte-identical on both sides, so the comparison holds, but this is not a green conformance claim.
Other RFC statements I checked in code
- "
sub.get("reviewerPartyRef", ctx.acting_party)returnsNonefor a present null, suppressing the compliance routing rule" —kernel/stages.py:631. True. - "The separate body-named-reviewer guard also excludes
None" —:659,not in (None, ctx.acting_party). True. - "The direct emitter always names
ctx.acting_partyas both asserter and reviewer" — true, and measured: every self-review promotion I produced haddecidedByPartyRef == assertedByPartyRef. This is what makes the removed comparison a no-op for correctness rather than a load-bearing check. - "The normalizer populates
acceptance_targetonly forGOVERNANCE_DECISION" —:277-279, exact. And probed: a compliance submission carryingreviewTargetAssertionRef,reviewTargetConsequenceRef, or both plusreviewAction: REVIEW_ACCEPT→REQUIRE_REVIEWevery time, and the named pending target keptPENDING_REVIEWwith 0 REVIEW edges. The class boundary holds. - "
IngressNormalizer.runandReplayWriter.writeshort-circuit before the promotion gate" —stages.py:202returnsGateReplay(ReplayWriter().write(ctx, prior))on an existing key;ReplayWriteriskernel/emission.py:470. True. - "For a distinct-body-named compliance request, both existing routing explanations may now appear" — measured exactly: problems
["Self-review out of scope", "Distinct reviewer requires own act"], bothHUMAN_APPROVAL_REQUIRED, so the loggedreason_codeand the outcome are unchanged. The RFC's prediction is precise and the "diagnostic addition is permitted" framing is right. - "The existing conformance tests 93/94 provide valid compliance/queue fixture patterns" —
test_93_governed_acceptance_semanticshas aqueue_compliance()helper;test_94_second_hostile_regressionsalready runs the full distinct-named compliance → advisor-accepts → double-accept-refused lifecycle. Good citation;test_94is the template E03 wants. - "HTTP actor mismatch retains its existing 403 precedence" —
test_http_actor_binding_precedes_malformed_confirmationpasses unchanged. - "Public production governed routes remain closed" —
kernel/api.py:74-83blocks/commit,/review/accept|reject|contest,/records/{id}, both views. True, and it bounds both this defect and B1 to the legacy development surface.
Checked and decided were not findings
Recording these so the next reviewer does not re-spend the time.
- Architecture budgets — the usual place an unfound Blocker hides here, and it is empty this time.
kernel/stages.pyappears in noMODULE_BUDGETS,COMMAND_MODULE_BUDGETSorGROUP_BUDGETSentry, so the predicate edit (−1 line) pays into nothing.TEST_GLOBSmatches none of the RFC's named test files, soMAX_TEST_LINES = 800andTEST_MODULE_BUDGETSdo not apply to them either (test_review_confirmation.pyis at 284 lines;test_conformance.pyat 2,065 already exceeds 800 and is legal precisely because it is unmatched). Phase B's planned six cases cannot breach a budget. No finding. - Generated inventory.
conformance/review_baseline_test_inventory.json:entryCount4,475,entriesSha256160c1ac7ce1991ec…, pinningnodeid+sourceModule+sourcePathperREVIEW_BASELINE.md:76-90; adding tests requires the explicitupdate-inventorymaintenance command, which the RFC names.conformance/review_baseline_config.json'sknownGreenBaseline.observedInRun.testSummary.collected = 491is not enforced —run_review_baseline.pyreads onlyactionspins andpostgresqlImageDigestfrom that object.warningPolicyisexact-inventorywith one expected entry whosenodeidis""andwhenis"collect", so new tests do not multiply it. The RFC's expected-areas list is adequate. No finding. substays used after the deletion (:646,:658-662), so no dead-local or lint consequence.- Other promoting classes. Only four classes promote;
NOTE,HYPOTHESIS_ASSERTION,EVIDENCE_RECORD,ADVISORY_OUTPUTare non-promoting andRETAIN_DRAFT. Of the four,OPERATION_CLAIM(D8) and boundedSTRUCTURE_ASSERTION(D17) are decided,COMPLIANCE_ASSERTIONis what this PR fixes, andOBSERVATION_ASSERTIONis B1. The enumeration is closed. - Keeping the
:658distinct-reviewer guard is correct: it is the only thing routing a distinct-named operation claim or observation, both measured. EXC-004's "still-used" is accurate. - Duplicate JSON keys in the submission are last-wins in one dict for both parser and gate — same as #384, not a finding.
What could not be checked, and why
- The pinned exact-interpreter gates (
conformance/rewrite_architecture_check.py's architecture / temporal / package-contract checks). They refuse any interpreter other than CPython 3.12.13 (_authenticate_full_execution_profile,UNSUPPORTED_PYTHON_VERSION), and this session's egress refuses bothcodeload.github.comandwww.python.orgwith proxy 403, so I could not build it. The author's recorded PASS (exit 0, 8.559s) is unverified by me. I substituted direct reads of the checker's budget tables, which is what the budget question actually needs, but it is not the same as running the gate. - PostgreSQL 17.10 and the hash-locked dependency set. Everything above ran on PG 16.13 with current PyPI wheels. The kernel Store does not enforce the 17.10 pin the way the security-audit provisioning path does, so these suites run — but role/RLS/wire-protocol-sensitive behavior is out of scope for this review and I make no claim about it.
- The hosted lightweight run 34712299485 and the Phase A evidence digests. The GitHub API is refused from my container for this repo; I read PR metadata through the other machine but did not re-verify artifact digests or recompute the design-review report hash
65f53d8d…. - Transaction atomicity, signing, custody, publication — inspected at the level of "a one-line predicate in one gate cannot reach them", not audited. E07 should not be read as audited by this pass.
What my method made easier than production
- Authority was handed to me.
demo.FARMERholdsASSERT_COMPLIANCE,OBSERVE_CREATE_OBSERVATIONandREVIEW_ACCEPTon the demo farm bydemo.bootstrap. A real grant topology might not co-locate assert and review rights on one party, which is the precondition both the fixed defect and B1 depend on. My probes prove the gate's behavior given that grant, not that the grant is common. - The legacy test transport binds the acting party from an
x-acting-partyheader. No OIDC, no principal resolution, no token. Production governed routes are closed, so the reachability I demonstrated is development-surface reachability — for B1 as much as for the defect. - Fresh isolated databases per probe removed ordering, concurrency and accumulated-state effects entirely. Nothing here says anything about concurrent commits on one key, and the replay probe was strictly sequential in one process at a time.
- I simulated the candidate by patching one line in my own clone. That is the RFC's stated patch and nothing else, but it is my transcription of prose, not the author's implementation. Every E01–E06 result above must be re-measured against the real Phase B head; treat them as a prediction that the proposed design is achievable, not as implementation evidence.
- PG 16 / CPython 3.11 / unpinned wheels is a more permissive oracle than the pinned baseline. It passed 229/231 identically on both sides, which is good calibration for this comparison and nothing more.
Bottom line
The predicate is right, minimal, and achieves everything E01–E06 promise — I measured all of it, including the history probe the plan defers. Approve version 1 only after B1's three paragraphs are in, because approval freezes E01–E07 and E04 currently reads as a closed list that is not closed. F1 and the two Preferences cost a sentence each and should ride along. Then the real work is the implementation head: fresh evidence, nothing reused from #384, and the queue assertion written against the derived disposition rather than claimState.
PR #386 — implementation re-reviewVerdict: No new in-scope blocking findings at Implementation assessmentThe fix removes the actual bypass without introducing another policy mechanism. if ctx.commit_class == "COMPLIANCE_ASSERTION" and confirmed:The caller’s reviewer hint can no longer suppress that restriction. Routing reaches the existing pending-assertion path before direct acceptance and retirement authorization. The distinct-body reviewer guard, bounded-structure condition, queue branches, and direct review-authority check remain in place. I found no reason to replace this with input coercion, a blanket null rejection, or a new eligibility abstraction. The regression tests exercise a usable independent-review path. Raw-input and replay behavior receive meaningful coverage. Remaining issues and their dispositionThe observation self-review defect remains a separate unresolved defect—not a successful compatibility behavior. Historical incorrect acceptances are not repaired. The extraction diagnostic is still failing. Verification limitsI confirmed that trusted baseline workflow 34751542790, attempt 1, completed successfully. The updated evidence packet reports 4,481 tests passed in each of two full runs, plus successful equivalence, platform, native, and publication verification. I did not execute local PostgreSQL tests, independently reproduce the historical probe, or independently authenticate the final published archive during this review. The detailed execution counts and archive-verification claims remain attributed to that packet. Recommendation: No further code changes requested for this scoped repair. Proceed to the separate exact-head final acceptance step, retaining the observation, structure, historical-replay, and extraction limitations explicitly. This review supplies neither merge authorization nor production-readiness approval. |
samovers
left a comment
samovers
left a comment
There was a problem hiding this comment.
PR #386 — implementation review (executed), head 81f1ce1
Reviewed head: 81f1ce1138242c668d13a969337f3e55d8cd4b2b.
Base: e50ae95f43d0e73b12113463d0e4ea85eebdc14b.
Prior pass by me: design review 5191268560 at 0472668 (1 Blocker, 1 Follow-up, 2 Preferences).
Verdict: one new Blocker, small and cheap to close. All four of my prior findings are closed — B1 on its own stated terms. The runtime change is byte-identical to the patch I measured last pass, the six new regressions are genuinely calibrated (I mutated them four ways), the inventory and the hosted-execution binding both verify independently, and the author's E06 history probe reproduces mine delta for delta on a different OS, interpreter and PostgreSQL. The Blocker is that the and confirmed half of the new predicate is pinned by nothing in the repository, and the E02 row of the evidence packet points at confirmation tests that never execute the changed branch.
Prior findings — disposition, checked not assumed
| Finding | Status | What I verified |
|---|---|---|
B1 — undisclosed unguarded OBSERVATION_ASSERTION self-acceptance |
Closed | The RFC gains a "Known K04 residual" block naming the behavior, REVIEW_DISPUTE_SEMANTICS.md gains the same residual in the active semantics, both name Delivery #387 (open, correctly scoped, cites the discrepancy and the refused queued acceptance), "enforces the existing D8 restriction" is now qualified "for confirmed compliance assertions", and E04 is explicitly non-exhaustive. That is exactly the three-part fix I asked for, and I asked for no runtime change. The body notes I have not withdrawn the Blocker label; I withdraw it now — the disclosure was the remedy. |
F1 — the twin comparison at :646 |
Closed, and improved on | The RFC's new paragraph is factually correct and I re-derived it: StructureCarrierValidator refuses contract.kind not in policy.STRUCTURE_PAYLOAD_IDENTITY_TYPE at kernel/validators.py:766 with EVIDENCE_INSUFFICIENT — the exact refusal my seven hostile shapes produced — and structure_self_acceptable keys off the same table at kernel/policy.py:202–211. Both line references are exact. The added caveat ("if carrier admission and that eligibility set diverge, the retained comparison needs review") is the right statement of the residual risk, better than my "latent" wording. |
| P1 — denylist vs allowlist predicate | Closed | EXC-006 now names the alternative and why it is deferred: it would change observation behavior and require an explicit D17 exception. That is the stated choice I asked for. |
| P2 — write E03 against the derived disposition | Closed | The verification plan now says so in the RFC, and the test enforces it: assert assertion_row["payload"]["claimState"] == "PENDING_REVIEW" plus a full-row equality check after acceptance. Nobody will "fix" this into an IN_FORCE mutation. |
Blocker B2 — the confirmed conjunct of the new predicate is unpinned by every test in the repository
Violated criterion. The RFC's E02 requires "Literal false/omission of confirmation retain existing capture behavior", and EXC-003 requires that "each guarantee traces to the existing gate/emitter and actual public-entry/Store evidence". The shipped predicate satisfies E02. No test detects its violation.
The new condition has two conjuncts:
if ctx.commit_class == "COMPLIANCE_ASSERTION" and confirmed:I mutated the head tree four ways and ran the six new regressions against each:
| Mutation | Result |
|---|---|
| m1 restore the design-base reviewer-hint comparison | 3 failed / 3 passed — …until_distinct_acceptance, …cannot_supply_review[null], …replay_preserves_raw_identity_and_history |
m2 drop and confirmed |
6 passed |
m3 replace the hint comparison with … is None (a null-only "fix") |
2 failed — [omitted], [self] |
m4 if False: (delete the condition) |
5 failed / 1 passed |
m1 reproduces the author's recorded base run exactly — same count, same three test names — from an independent clone on Linux/CPython 3.11.15/PG 16.13 against their macOS/3.12.13/PG 17.10. m3 is the one I most wanted to see fail, and it does: the suite distinguishes a genuine class-level rule from a null patch, which is the defect class that produced #385 in the first place. m4's survivor is [body-named-distinct], correctly held by the :658 guard. Three of four mutations are caught. m2 is not caught by anything.
m2 is not cosmetic. Measured over the real route at this head, and under m2:
head (81f1ce1) m2 (drop "and confirmed")
compliance/confirm-omitted RETAIN_DRAFT no problems REQUIRE_REVIEW HUMAN_APPROVAL_REQUIRED
compliance/confirm-false RETAIN_DRAFT no problems REQUIRE_REVIEW HUMAN_APPROVAL_REQUIRED
compliance/confirm-true REQUIRE_REVIEW HUMAN_APPROVAL… REQUIRE_REVIEW HUMAN_APPROVAL_REQUIRED
m2 converts unconfirmed compliance capture into a routed review exception: a different outcome enum, a HUMAN_APPROVAL_REQUIRED problem the caller did not earn, and the amend_case_for_routing=True emitter path instead of the plain capture path at kernel/stages.py:703-708 — the branch whose own comment is "no review act: capture is not commitment (Kernel rule 3)". That is a direct E02 violation and a Kernel rule 3 violation, and it ships green.
How green. I ran m2 against every test file in kernel/tests/ that submits a COMPLIANCE_ASSERTION — test_conformance.py, test_correction_authorization.py, test_m2_identities.py, test_stages.py, test_self_review_eligibility.py — plus test_review_confirmation.py and test_m2_review.py:
m2: test_correction_authorization + test_m2_identities 149 passed
m2: test_review_confirmation + test_stages + test_m2_review + new 62 passed
m2: test_conformance (own session/database) 23 passed
total 234 passed, 0 failed
Why the E02 row does not cover this. The body's E02 row says "Existing confirmation tests cover omission/false/true, malformed values and transport-binding precedence. The recorded compatibility pass includes those unchanged controls." Those tests are test_review_confirmation.py::test_http_confirmation_value_matrix_preserves_history and its neighbours, built on demo.spray_submission, whose helper hard-codes "commitClass": "OPERATION_CLAIM" (profile_si_ffs/test_fixtures/demo_payloads.py:236). An operation claim never evaluates the compliance condition. The sentence is true about those tests and misleading about this guarantee: read in an E02 row about the compliance change, it implies coverage of the class whose predicate changed, and there is none.
Smallest acceptable fix, entirely inside the file this PR already adds:
- Extend
test_direct_compliance_reviewer_metadata_cannot_supply_review(or add one small sibling) with the confirmation axis:confirmAcceptomitted and literalfalse→RETAIN_DRAFT, one pending assertion, no routing reason inproblems, no review or consequence. Two parameters;_compliance()and_assert_no_acceptancealready exist in the module. - Regenerate the inventory (4,481 → 4,483) with the prescribed
update-inventorycommand. - Correct the E02 row to say which class each cited control actually exercises.
Counter-argument, stated. m2 is over-restriction, not a bypass — it refuses things it should capture; it cannot create a wrongful acceptance. The shipped runtime is correct, and a reviewer who reads "Blocker" as "the code is wrong" should read this as a Preference. I file it as a Blocker because in this repository the deliverable is the evidence packet as much as the code: the packet asserts E02 is covered, one conjunct of the PR's own new predicate has no regression anywhere in 4,481 collected tests, and the fix is two parametrize entries. EXC-003 is a criterion the decision itself sets, and this is the one place at this head where a guarantee does not trace to evidence.
Preference P3 — cite the posted review, not only an internal message id
The RFC's new supplementary-evidence paragraph is the most careful attribution I have seen in this series: it names my interpreter, database and unpinned wheels, flags that my harness-pollution diagnosis is my diagnosis and not independently verified, and refuses to let the measurements substitute for fresh implementation evidence. All correct, and the transcribed numbers (229/2 each side, 23/23, 42/42) are accurate.
It binds that evidence to msg_01a0971d-c6fa-7ca3-8f6a-bb14ef0b4a4f with SHA-256 f568d974…. Nobody outside the task can resolve either. The same content is a durable public artifact: review 5191268560, whose body hashes to e90217cb974bef9c9e5c4ece5a997489193bb2d322b03a94775304837bb14960 — recomputed from the API at posting time and again today. Citing the review id and that digest alongside the message reference would make the attribution checkable by the same method this repository uses for everything else. Preference only; it delays nothing.
What I verified at this head
The runtime change is the patch I already measured
git diff e50ae95 81f1ce1 -- kernel/stages.py produces blob e14f255, byte-identical to the transcription I measured last pass. My prior E01–E06 measurements therefore transfer without re-derivation, and I re-ran the important ones anyway.
Full reviewer-metadata matrix at this head, real POST /commit, eight shapes:
omitted / null / self / distinct / "" / 0 / [] / {"a":1}
-> REQUIRE_REVIEW, HUMAN_APPROVAL_REQUIRED, 1 pending assertion (PENDING_REVIEW),
0 reviews, 0 consequences (all eight)
Positive controls at the same head, unchanged:
operation-claim self-review PROMOTE_ACCEPTED 1 review 1 consequence IN_FORCE
structure bounded self-accept PROMOTE_ACCEPTED 1 review 1 consequence IN_FORCE
observation (omitted/null/self) PROMOTE_ACCEPTED 1 review 1 consequence IN_FORCE <- the #387 residual, as disclosed
The observation rows are the measurement behind the new residual text. The disclosure is accurate: omitted, null and self-named all self-accept; only a distinct body-named reviewer routes.
Regression, at this head
test_review_confirmation, test_m2_review, test_correction_authorization,
test_correction_transactions, test_review_fixes, test_stages,
test_m2_identities, test_self_review_eligibility 233 passed, 0 failed (156s)
test_conformance (own session/database) 23 passed, 0 failed
Note for the record: the two conformance failures in my previous pass were, as I said then, an artifact of running test_conformance.py in one session with other files. Running it in its own database here reproduces 23/23 on both sides, and the author's packet independently records 23/23. That diagnosis is now confirmed rather than asserted.
Generated inventory — recomputed, not trusted
Loaded conformance/run_review_baseline.py and ran the repository's own _inventory_document over the committed entries:
entryCount 4481 (file: 4481) base was 4475, delta +6
added 6, removed 0, reattributed 0
entriesSha256 4228e2c4677100264847b1de78dd195b756e1162121ab08861795883e923e839 MATCH
entries sorted/normalised identically MATCH
The six added nodeids are exactly the six the new module collects — I compared the inventory rows against --collect-only on that file: exact match, with sourceModule/sourcePath correct on all six.
I did not reproduce a full-repository collection comparison, and I want to be explicit about why, because a naive run looks alarming: my environment has 19 collection errors from absent optional dependencies and a different pytest than the pinned 9.1.1, giving 3,589 collected, 961 inventory rows uncollected and 69 parametrize ids differing only in how non-ASCII and control characters are escaped. That is my environment, not drift. The hosted twice-run baseline at 4,481 collected/selected/passed each is the only thing that settles full-inventory agreement, and I did not run it.
The hosted evidence actually binds to this head
The check that matters here needs no download and no credentials:
refs/pull/386/merge = 4ce9a0511ca1e620414629b8e02e3bb29fa259b6
tree a1c8c553e0f99a9d2a81fbe8d6ae9eac950f1902 == git rev-parse 81f1ce1^{tree}
parents e50ae95 (main) + 81f1ce1 (head)
and the packet's own hosted-coordinates.json records executionMerge: 4ce9a051…. I derived that merge commit from git independently and it agrees. So the baseline gate executed the reviewed content, not a stale or rebuilt tree.
Run metadata, read live:
34751232428 pull-request-lightweight success attempt 1 head_sha 81f1ce1138 (pull_request)
34751542790 reviewed-head baseline gate success attempt 1 head_sha e50ae95f43 (issue_comment)
34753058161 evidence-publication success attempt 1 head_sha e50ae95f43 (workflow_run)
artifact 10316712345 evidence-publication-receipt 1,395 bytes expired=false run 34753058161
The two e50ae95 head_shas are the expected shape for issue_comment/workflow_run triggers — the workflow file comes from the default branch while the checkout is the merge ref — and the merge-tree check above is what actually ties them to the candidate.
Admission comment 5652671269: author samovers, association OWNER, created_at == updated_at (unedited), 595 bytes, body SHA-256 2917165b85e6b50bc1da2bb6804fd1d494e5d4d8c27543f21152328deca41d6c — equals the packet's claimed bodySha256. It invokes the existing baseline admission and claims no verdict of its own.
The history probe reproduces mine exactly
The packet's history-summary.json and my independent two-phase base→candidate probe from the previous pass agree on every number:
replay deltas kernel_record +3, kernel_edge 0, kernel_gate_log +1, kernel_idempotency 0, all derived tables 0
fresh deltas kernel_record +10, kernel_edge +6, kernel_gate_log +6, kernel_idempotency +1
base PROMOTE_ACCEPTED -> candidate REPLAY_REUSED_RESULT with the original refs -> fresh key REQUIRE_REVIEW
Two independent implementations of the same probe, on different operating systems, interpreters and PostgreSQL major versions, produced identical row deltas. That is the strongest form this evidence can take short of the hosted run, and the packet's refusal to call it a concurrency or cleanup claim is right.
Extraction diagnostic — ran it on both sides
e50ae95: RESULT: FAIL (2 failures)
81f1ce1: RESULT: FAIL (2 failures)
Identical failures, identical paths: no file/glob review record covering conformance/review_baseline_test_inventory.json or kernel/tests/test_rewrite_architecture_check.py. The first of those is a file this PR edits, which is the case where a permanently-red check stops being a useful regression detector. It is not vacuous here: the failing set is derived from current seed-scan hits, so adding a configured seed term to the inventory would have produced a third failure, and it did not — none of the six added nodeids carries one. The count staying at 2 is therefore a real result about this delta, not an unchanged constant. The body's applicability reasoning (conformance/CONFORMANCE.md:23, :58–66; the root review-baseline inventory is a different artifact from profile_si_ffs/extraction_inventory/) reads correctly to me, and it claims no waiver.
Other claims checked in the packet
- Local counts add up: 208 (compatibility) + 23 (conformance) + 6 (candidate regressions) + 2 (production closure) = 239.
regression-base.lognames the same three failures m1 produces here.- Five files, 607 additions / 12 deletions;
git diff --checkclean; runtime 1 insertion / 2 deletions. docs/REVIEW_DISPUTE_SEMANTICS.mdnow states, in the active semantics rather than only in an RFC: the bounded compliance guarantee across all four reviewer-metadata shapes; the replay/history limit; that queue acceptance leavesclaimState: PENDING_REVIEWbytes unchanged; and the observation residual as "not permitted D8 behavior" owned by #387. Every one of those sentences is a measurement I have reproduced.
Checked and decided were not findings
REVIEW_DISPUTE_SEMANTICS.md:174's older "D8/D17 grant a farmer self-acceptance of bounded classes" is untouched. It remains normatively true — D8/D17 do grant bounded classes; the runtime exceeds the grant — and the new residual block upstream says so explicitly. Leaving it is right.- The
amend_case_for_routing=Trueemitter path is not new for the null case; omitted and self-named compliance already took it at base. No new emitter reachability. - Compliance submissions carrying
reviewTargetAssertionRef/reviewTargetConsequenceRef/reviewActionstill cannot select the queue-emitter branch (acceptance_targetisGOVERNANCE_DECISION-only). Re-probed at this head. - The inventory's 32-add/2-delete line count is JSON formatting plus the
entryCountandentriesSha256lines; the entry-level delta is +6/−0 as claimed. - The new module reuses
test_review_confirmation's_snapshot,_assert_valid_replay,GOVERNED_TABLESandOMITTED. I checked for the failure mode from PR #323/#326 — a fixture more permissive than the real posture — and found none: every assertion runs against the real Store and the real routes, and m1/m3/m4 prove the assertions bite.
What could not be checked, and why
- The pinned interpreter gates again. CPython 3.12.13 is still unbuildable from this session:
codeload.github.comreturns proxy 403 andwww.python.orgisconnect_rejectedby organization egress policy. So the mandatory package/architecture/temporal checks, Ruff and the twice-run baseline are unverified by me; the packet'sRESULT: PASS (0 failures)precommit log stands unaudited. My runs are CPython 3.11.15, PostgreSQL 16.13, unpinned wheels. - Linux wheel hash authentication, the receipt ZIP digest
sha256:2874bc25…, the two 4,481-case baseline logs, the native ARM64/AMD64 job contents and the consumer report — all need credentials or downloads I did not perform. The merge-tree and comment-digest checks above are what I could do read-only, and they passed. - The internal
msg_…approval and card references and their SHA-256s are not resolvable from outside the task. See P3. - Concurrency, transaction ownership, custody, production authentication. Not exercised. Legacy test transport with header-bound actors and fictional demo grants throughout; production governed routes remain closed (
kernel/api.py:74-83), which is what bounds both this fix and the #387 residual to the development surface.
What my method made easier than production
Same four as last pass, unchanged: demo.FARMER is handed co-located assert and review authority by demo.bootstrap; the legacy transport binds the actor from a header with no OIDC; every probe ran in a fresh isolated database with no concurrency; and my interpreter, database and wheels are all looser than the pins. One addition specific to this pass: mutation testing proves what the suite catches, not that the suite is complete. m2 is the gap I found by choosing that mutation; a different conjunct or a different gate might hide another, and four mutations is not a mutation-adequacy result.
Bottom line
B1, F1, P1 and P2 are closed, and B1 is closed exactly as I asked — disclosed in both active documents, owned by #387, with the D8 claim narrowed. The implementation is the patch I measured, the tests are calibrated against three of four hostile mutations, and the inventory, merge-tree and admission bindings all verify independently. Close B2 with two parametrize entries and an inventory regeneration, fix the E02 sentence, and this head has no finding I can demonstrate. P3 costs one line and makes the attribution checkable by anyone.
Address PR #386 review B2 with omitted and false confirmation controls through real HTTP and Store evidence. Regenerate the inventory and distinguish compliance capture coverage from operation-claim parser controls. Cite the public design review for P3. Runtime and approved E01–E07 remain unchanged; M1 review eligibility, Delivery #385.
|
Bounded B2 correction and affected E02 review completed for PR #386 / Delivery #385 at 8a11d3f with zero demonstrated in-scope Blockers. The full report is in the PR description; SHA256 8d5ef6bd7d57ecd2ac09f7ab90187841c57c88bc5e7726b7153fdd045327317e. B2 is closed and P3 addressed; runtime and approved semantics are unchanged. Local tests and honest residuals are recorded. This invokes the existing baseline admission only, not semantic approval, publication success, merge authority or production permission. OFARM2_BASELINE_ADMISSION |
1 participant
Prevent new compliance self-acceptance through reviewer hints
Closes #385. Related Tracking Epic #180 and issue #179. Observation eligibility remains separate Delivery #387.
Completed technical verification — 2026-09-13
Ready for final task-user acceptance at
8a11d3f2182e9c0504dab2763f01d9c83202dd8e. B2 is closed by executed compliance-capture controls and corrected E02 attribution; P3 is addressed. Runtime and approved E01–E07 are unchanged. Fresh bounded review found zero demonstrated in-scope Blockers. Final live capture shows the same open, non-draft, mergeable head and policy base, no new review finding or close/reopen transition, and an unchanged unedited OWNER admission. Merge is not authorized.ddbc3af8811c9faf7ef9f9f218eb4d5642a9480b68e8ebaec25b7295e7c1c144.e50ae95f43d0e73b12113463d0e4ea85eebdc14b; admitted execution merge8ca3d352f6d6e248f3052b3006db4b48beb6a375, tree63d532112ed7c705997d0d71f5f6d0fec12928f7, exactly the candidate tree.sha256:4cfcb2bfcf5ade9e2027525fdef70339d6d7ee6a33065b7a11bdd10d51f7ee0c; receipt JSON SHA256285dbf1473c620ea3a08a124a10a594b671a13f734ed45e31d6cb11184dc69ce. All four source and all five published IDs/digests, workflow refs/policy SHAs, runs/attempts, exact head/admission and receipt-last publication were verified.abe205ceb950a5ff2ee67a2160071c67ff9eddfcc6811e82c1d11957c1aad95f, captured 18:03:10–18:03:37 UTC. Both published and provisional equivalence records were independently recomputed. Native image archives were not downloaded or re-executed locally: their live API/receipt bindings and small index were checked; the trusted publisher re-authenticated native artifact contents. This verifies the capture interval, not future merge authority.All current evidence is preserved under
plans/kernel-self-review-eligibility-evidence/review-fix-b2/in the task workspace. Earlier 81f1ce1 evidence is historical and has not been reused as current admission/publication. Historical accepted-null replay, observation #387, conditional structure risk and extraction FAIL2 remain disclosed; the conditional extraction-status gate is inapplicable, not waived. Main protection remains disabled, so these gates are procedural. No runtime expansion, audit restart, deployment or merge occurred.Next: present the replacement complete same-task final packet and end the turn. Only a later exact task-user authorization can allow fresh live checks and a normal expected-head GitHub merge.
Approved boundary and final scope
Primary trust boundary: self-review eligibility for new legacy compliance acceptance, before new accepted-force emission. Decision OFARM2-LEGACY-SELF-REVIEW-ELIGIBILITY-001 version 1, original card
msg_0d813f8071772f9a016aa59fc72b5487d293c170fb684a3b55(2026-09-12T18:54:24.109Z), then exact task-user approvalmsg_01a09a37-50c0-7991-acaa-b175fe812417(2026-09-13T10:01:51.552Z), in task01a07cc8-4157-7b33-a0ca-becb772e0e8b. Originals were retrieved directly again before this correction; later user messages only requested review reading. These references navigate the original authority; GitHub reviews and this body do not supply approval or merge authority.The one runtime change remains removal of the reviewer-hint comparison from
if ctx.commit_class == "COMPLIANCE_ASSERTION" and confirmed. It routes otherwise-valid confirmed compliance through existing review-required capture regardless of null, omitted, self or distinct body-named reviewer metadata. Raw metadata does not establish an independent reviewer. An authorized distinct actor can subsequently accept the same pending assertion once.Full diff against base
e50ae95f43d0e73b12113463d0e4ea85eebdc14b: five files, 689 additions / 12 deletions. Runtimekernel/stages.pyremains one insertion/two deletions and is byte-identical to the previous implementation head (SHA-256834264977d8082365b8dcff11cf82cb4eaf410ff1b21fb2fbf0fa8aa46732c59). Other files:kernel/tests/test_self_review_eligibility.py(264 lines, eight collected cases), generatedconformance/review_baseline_test_inventory.json(4,483 entries),docs/REVIEW_DISPUTE_SEMANTICS.mdanddocs/rfcs/OFARM_Legacy_Self_Review_Eligibility_RFC_v0_1.md. This correction alone changes the test, inventory and RFC, 91 additions/9 deletions. E01–E07 remain byte-identical; active semantics and all runtime are unchanged from81f1ce1.Review correction and evidence trace
Executed implementation review 5191463884 identified B2, a real E02 evidence gap: all six initial compliance tests used true confirmation; existing confirmation-matrix tests submit operation claims and never execute the compliance condition. The shipped runtime was correct, but removing
and confirmedescaped those tests. The earlier E02 evidence row implied coverage it did not supply. It is superseded by the explicit class attribution below.test_http_unconfirmed_compliance_keeps_capture_only[omitted/false]exercises COMPLIANCE_ASSERTION through real HTTP and Store. Both require RETAIN_DRAFT, empty problems, one pending compliance assertion, no review/consequence or REVIEW edge, original raw-input digest/prior rows and a stored REVIEW_PROMOTION result without a routing reason. Existing four compliance reviewer controls still require review when confirmation is true.test_review_confirmation.pyseparately exercises OPERATION_CLAIM confirmation and shared parser/transport controls; it is not evidence for the compliance-specific conjunct.Local verification of this correction
and confirmedremoved: 2 failed / 6 deliberately deselected, 1.52s. Both new cases fail because REQUIRE_REVIEW replaces RETAIN_DRAFT. Expected failure is preserved honestly; the real worktree runtime was never changed by the mutation.c7595e8f475ae25c5e7268a45344e31787b01c7d108244df27a0455c3942e157; entries SHA-256737e688267c914da5d829097004d7ea9f94726f80e0d02b9136478574573b384.kernel/tests/test_rewrite_architecture_check.py. Existing conditional extraction-status gate is inapplicable to this test/documentation correction; extraction-status/navigation and checker/review records are unchanged. Changed-path seed sequences are identical; no new country/profile authority enters the kernel. No waiver or all-green extraction claim.Environment: CPython 3.12.13/macOS ARM64, all installed versions match the earlier lock-verified local set, PostgreSQL 17.10 from the exact pinned image, function-isolated fictional fixture databases. Local matching versions do not authenticate Linux wheel hashes. The uniquely labelled B2 container, temporary databases and mutation source copy were removed after capture; all evidence retained. Other containers and worktrees were preserved. The two pytest runs and selected mutation each retain one known Starlette/httpx warning. Local evidence lives under
plans/kernel-self-review-eligibility-evidence/review-fix-b2/in the task workspace. The initial implementation's 239 local passes, full history capture and 4,481-case hosted evidence remain intact as explicitly earlier-head evidence.Findings and non-effects
The new review explicitly closes B1, F1, P1 and P2 and withdraws B1's Blocker label. Earlier reports saying it had not been withdrawn describe the earlier state. Observation's runtime defect still belongs to open #387; documentary closure does not repair it. The structure comparison requires future review if carrier admission and eligibility diverge. P3 is addressed by citing public design review 5191268560 and its verified body SHA-256
e90217cb974bef9c9e5c4ece5a997489193bb2d322b03a94775304837bb14960beside the original task-message citation. That public body is the old design review plus one terminal newline; API attachment to the implementation head does not change its stated design-head scope.Reviewer measurements used CPython3.11/PG16/unpinned wheels. They found the B2 gap and independently checked selected source, inventory, merge-tree and admission facts, but did not authenticate the full archives, pinned gates or full hosted run contents. They do not substitute for this head's fresh verification. Historical accepted-null replay still returns old accepted references: no historical migration or repair is included. No observation/structure runtime decision, new authority, custody, transaction, publication mechanism, deployment or production activation is included.
EXC-001–006
One existing authoritative decision path; no duplicate authority, state, normalization or fallback. The two added real-entry/Store tests close the specific E02 trace gap and fail against the measured counterexample. The obsolete compliance comparison remains deleted; still-used distinct-body/structure controls remain. No kernel abstraction is added. Null coercion/rejection would alter ingress/replay, and a shared allowlist would introduce separate observation/D17 decisions. This test/documentation correction preserves the approved vertical slice and all decision-level effects.
Next: present the completed replacement packet and yield for later exact-head user merge authorization; no merge is authorized yet.
Complete bounded exact-head B2 correction review
PR #386 — bounded B2 correction review
Reviewed head:
8a11d3f2182e9c0504dab2763f01d9c83202dd8e.Correction parent:
81f1ce1138242c668d13a969337f3e55d8cd4b2b.PR base:
e50ae95f43d0e73b12113463d0e4ea85eebdc14b.Tree:
63d532112ed7c705997d0d71f5f6d0fec12928f7.Repository / Delivery / PR:
samovers/OFARM2, #385, #386.Decision:
OFARM2-LEGACY-SELF-REVIEW-ELIGIBILITY-001 version 1.Primary trust boundary: self-review eligibility for new legacy compliance acceptance.
Verdict: zero demonstrated in-scope Blockers in the correction and affected
E02 invariant. B2 is closed by the new executable evidence and corrected
attribution; P3 is addressed. No new Follow-ups or Preferences.
This is the bounded post-Blocker review required by AGENTS.md, not another
unconstrained content review or a restarted audit. I read the complete
81f1ce1..8a11d3fdiff, relevant existing HTTP/Store/fixture helpers, the approvedE02 and EXC-003 requirements, and the captured commands and results listed below.
I independently checked current HEAD, a clean worktree, changed-file hashes,
whitespace, inventory entries/digest, unchanged runtime/active semantics/E01–E07,
and the cached public review body digest. I did not run tests, databases,
workflows or GitHub operations during this review.
B2 — direct evidence now covers the confirmed conjunct
The added
test_http_unconfirmed_compliance_keeps_capture_onlyhas exactly twoparameters: omission and literal
False. Its_compliance()fixture is a validfictional
COMPLIANCE_ASSERTION, with the existing null reviewer hint anddurable demo evidence. Each parameter changes only confirmation.
_commituses the real legacy
POST /commitroute throughTestClient, with the fixture'sheader-bound actor; the route uses the actual Store and pipeline.
fresh_envcreates and drops a function-isolated database and bootstraps fictional records.
The test does not bypass the gate or mock its result.
Both cases assert
RETAIN_DRAFT, an empty problems list, exactly one returnedpending compliance assertion naming its asserter, no emitted review or accepted
consequence, unchanged persisted review/consequence records, and no REVIEW
edge. They also verify the input and stored raw digest, preserve existing record
bytes, and inspect actual
kernel_gate_logrows for exactly(RETAIN_DRAFT, None)at REVIEW_PROMOTION. Thus the proof covers both the publicresult and durable effects/routing evidence; it is not a response-label-only
test or another OPERATION_CLAIM control.
Captured candidate execution passed all eight focused cases. The separate
disposable-copy mutation removes only
and confirmedfrom the compliancecondition; both new cases then fail at the intended outcome comparison because
the mutation yields
REQUIRE_REVIEWinstead ofRETAIN_DRAFT. Six existingcases were deliberately deselected in that calibration. The mutable copy's
test digest equals the committed candidate test digest. This demonstrates the
missing E02 guard is now pinned; it is not an exhaustive mutation-adequacy claim.
The RFC explicitly separates these compliance controls from the existing
operation-claim confirmation and shared parser/transport tests. It acknowledges
the original six-case prediction missed this axis, preserves the initial
results as historical, and records the focused correction without changing the
approved invariant. The live PR's E02 evidence row must use that same precise
attribution when the root task synchronizes publication metadata; an earlier
head's packet or baseline is not the new head's evidence.
P3 — public review attribution is verifiable
The RFC now cites
review 5191268560
beside the existing task-message reference. I recomputed its cached API body
SHA-256 as
e90217cb974bef9c9e5c4ece5a997489193bb2d322b03a94775304837bb14960.Removing the public body's one terminal newline produces the task-review hash
f568d9740295a1c9b45511e4963adeb810cd54eecdc37f6047c28a3fa5249e4a.The RFC accurately notes that the body concerns the old design head despite
GitHub attaching the formal review to the earlier implementation head. These
are evidence/navigation references, not semantic approval or independent human
authority. I used the root task's fresh cached API response; I did not make an
additional network request.
Scope, inventory and evidence
The correction changes only the existing test module, generated baseline
inventory and RFC: 91 insertions / 9 deletions across three files. The full PR
remains five files, 689 insertions / 12 deletions. Runtime and active semantics
are byte-identical to
81f1ce1; the seven approved RFC invariant rows are alsobyte-identical. There is no changed allow/deny decision, authority, identity,
history/replay rule, deployment posture or independent capability. Version-1
semantic approval therefore continues to cover this in-boundary evidence fix;
the required new exact-head review and fresh applicable hosted evidence still
apply.
I independently recomputed the canonical inventory digest and entry delta:
4,481 to 4,483, exactly the two new parameter nodeids, no removals,
reattributions or duplicate nodeids. Entries digest:
737e688267c914da5d829097004d7ea9f94726f80e0d02b9136478574573b384.The prescribed
update-inventorycommand exited 0 and recorded 4,483 pinnedtests. No new country/profile seed terms are introduced by these changes.
git diff --checkindependently passedThe extraction failures remain the inventory and unchanged architecture-test
paths. This correction does not change extraction-status/navigation artifacts,
and the test/inventory delta adds no seed term. The documented conditional
extraction-status gate remains inapplicable; no waiver or passing extraction
claim is supplied. Ruff PASS is recorded by the root task, but I did not rerun
Ruff or authenticate a separate Ruff transcript in this bounded review.
The recorded local interpreter is CPython 3.12.13 on macOS ARM64, with versions
matching the previously verified lock; Linux wheel hash authentication remains
a fresh hosted obligation. The recorded PostgreSQL 17.10 container uses the
baseline image digest, localhost binding, task/Delivery ownership labels and
tmpfs data. The command wrapper clears inherited OFARM variables and supplies
the owned connection coordinates. Cleanup records removal of that owned
container, its data and the disposable mutation copy, with other containers
untouched. I inspected these records and the fixture implementation; I did not
independently repeat container cleanup or DB probes.
Continuing limits and authority
Review 5191463884 explicitly closes its B1/F1/P1/P2 requests and withdraws B1's
Blocker label. Their documentary closure does not repair the observation
runtime discrepancy owned by Delivery #387, remove the structure comparison,
or change historical accepted-null replay. All remain disclosed. No new audit,
concurrency, production authentication, custody, transaction or native artifact
execution claim follows from these two tests.
EXC-001/002: existing runtime authority and shared fixture helpers remain the
single paths; no parallel policy or durable state is added. EXC-003: the missing
compliance-capture invariant now reaches actual public-entry/Store evidence and
fails under its specific counterexample. EXC-004: the original deleted runtime
comparison remains deleted; this fix removes misleading evidence attribution.
EXC-005: no abstraction is added. EXC-006: two parameters in one small sibling
test are sufficient; a larger matrix or runtime policy consolidation is not
needed to close this demonstrated gap.
authority-retrieval.jsonpreserves the root task's direct retrieval of theoriginal complete card and later exact user approval, their stable IDs/order,
and only the later review-read requests. This review checks consistency of that
record; it does not manufacture or replace approval. There is no PR #386 merge
authorization. Prior
81f1ce1hosted results remain historical, not current-headadmission or proof.
Digest bindings
SHA-256 values recomputed during this review:
b108b809b7824f5d64e21e4ae24847c4404be0fcd36dca00137f97b1334d5391c7595e8f475ae25c5e7268a45344e31787b01c7d108244df27a0455c3942e15706a82afcd8e7176b2aa3c3da843923cfe9deca6833eb6a9d41af0919eab8e25e834264977d8082365b8dcff11cf82cb4eaf410ff1b21fb2fbf0fa8aa46732c59candidate-scope.jsonat review0d9bf88cfcc8fdb99561e3cfcd192902fa5ec2d1f08ba582e7d9ff40d98a1d7bregression-candidate.log6255780398abfde4730942bd7a292b62c2434b8ae705237271c4f495eed48353mutation-m2.logd61a9e2a2c67f9b333e8f64620e403cf08e6c4a89f8bd6afd4a98a1e9d6b7df0shared-confirmation.logbdfcae67566d0854de92458e08874639b02128082e6930d74cbb8fba25c80c48package-precommit.logffde5958f9f39f8122b9c3953e4c8bd444822a544a4f0c25eb2e89c26d674699inventory-delta.json1ee741e832115e28311a4e1062e445fb66d6136aa684f95116490cb7612ba43cscope-check.json99fef70309223d2fc76ada875526ed9e70acdbda364d6abeb0d214947bb7afe4authority-retrieval.json9f12053146a7ddc905e7701bdb591714e3213887fab69f12d78b18c0e9d7e6d8cleanup.json7f945d581c643c8b39d1e63d0034f9a18bec4dac63ab8462c54b2f378f5ef253Next: synchronize the PR's E02 attribution, use fresh unedited admission for
this reviewed head, complete required hosted execution and trusted publication,
then present a replacement exact-head packet and yield for the separate later
task-user merge authorization.