Conversation
Document the existing result-to-trace-to-case traversal and its limits. The runtime already retains the distinct decisions; four fictional database scenarios verified the reading examples. No runtime, contract, authority or inventory change is needed. Addresses Delivery #389.
samovers
left a comment
There was a problem hiding this comment.
PR #393 — Request changes
Reviewed head: 5fedcc16776dd47f93e399cf5297d46e8711dbf4.
Scope: docs/REVIEW_DISPUTE_SEMANTICS.md only, 69 additions / 3 deletions.
One P2 documentation-correctness Blocker (B1), detailed inline at lines 925–927. The new general instruction labels the retained case outcome as the evidence-floor evaluation, but the existing runtime can amend that outcome after the evidence gate to incorporate non-evidence review-routing requirements. This undermines the guide's intended distinction between evidence sufficiency and acceptance decisions.
The smallest correction is documentation-only: label the fields as the retained sufficiency-case outcome, and explain that routed cases may incorporate later review-routing reasons. Read that outcome alongside the evidence-gate outcome and the final result's problems. No change to case semantics, stored records, links, endpoints, or runtime behavior is needed.
Remaining assessment
The forward traversal, GET payload unwrapping, separation of exact problem codes from trace gate entries, and replay warning match the implementation. In particular, replay creates a new trace without carrying forward the original case link. No additional Blocker was found in those instructions.
Sources inspected at the reviewed head include the legacy record reader, the gates, the record and replay writers, and the sufficiency-case builders.
Validation limits: Source review of the diff, relevant runtime paths, and existing test code. Tests were not executed and the author's local probe captures were not inspected.
Disposition: Fix B1 within this documentation-only PR. No separate Follow-up or runtime redesign is warranted. This review is not merge authorization.
Submitted as a COMMENT review because GitHub does not permit the authenticated PR author to submit a formal REQUEST_CHANGES review. The review verdict above remains Request changes.
| 3. If the trace contains `evidenceSufficiencyCaseRef`, fetch that record and | ||
| unwrap `payload`. Label its `outcome.decision` and `outcome.rationale` as | ||
| the **evidence-floor evaluation**, alongside the final commit outcome. |
There was a problem hiding this comment.
B1 — [P2 / Blocker] The retained case outcome is not always the evidence-floor evaluation
This instruction applies generally to records returned from /commit, but the retained outcome.decision and outcome.rationale can incorporate non-evidence review-routing requirements, not just the earlier evidence-floor evaluation.
Concrete counterexample: Submit an otherwise-valid routine operation with complete evidence, confirmAccept: true, and a distinct reviewer named in reviewerPartyRef. The evidence gate can build an ALLOW case and log SATISFIED. ReviewPromotionGate.run() then adds HUMAN_APPROVAL_REQUIRED because naming another reviewer in the submitter's body is not that reviewer's own review act.
That path calls emit_pending_assertion(amend_case_for_routing=True). Before persistence, PromotionEmitter._store_case() calls amend_case_for_routing(), which changes the retained case outcome to REQUIRE_REVIEW, replaces its rationale with the routing explanation, and sets attestationAllowed to false. This is visible in the existing promotion gate, case-storage path, and case-routing amendment.
Violated invariant: Reader guidance must accurately distinguish evidence sufficiency from acceptance/review-routing decisions. Labeling this retained outcome as the evidence-floor evaluation presents a review-act requirement under an evidence-floor label.
Smallest acceptable fix: Rename the label to “retained sufficiency-case outcome” and add a qualification such as:
For routed submissions, the retained case may incorporate review-routing reasons added after the evidence gate. Its
REQUIRE_REVIEWoutcome does not, by itself, establish that evidence was missing. Read it alongside the evidence-gate outcome and the final result's problems.
This is an in-scope documentation correction only. Do not change runtime behavior, case semantics, stored records, links, or endpoints to resolve it.
samovers
left a comment
There was a problem hiding this comment.
PR #393 review — legacy reader guide for evidence sufficiency vs final outcome
Head reviewed: f804beb626c7026f6d25373caa9c427c1c5fc25c (tree 66cc839334a38b2986f0eb027bd5d5c1ddcdf557)
Base: 157e130dffb1f38ca4138e55075c5dde04778cc2 (tree 5c2e197e522a316513931315a26f05c1c03cb91d)
Scope: docs/REVIEW_DISPUTE_SEMANTICS.md only, +74/−3 (blob 2002c7c3, file sha256 42b54626…a82fa45). The B1 correction commit 5fedcc1..f804beb is +7/−2.
Delivery: #389. Classification: root AGENTS.md "Review classifications" (Blocker / Follow-up / Preference). The PR has no review-classification section of its own.
Verdict: 1 Blocker, 1 Follow-up, 1 Preference. Not ready to merge at this head.
Where this pass sits
This is an independent third pass on the PR. Earlier passes:
- The author's sealed review at
5fedcc1: zero findings. The author has since said it missed B1. - Review 5207958022 / inline 4013941567 at
5fedcc1: one Blocker (B1). Source-only; no tests run. - The author's "bounded B1 correction review" at
f804beb: B1 closed, zero findings. It checked the source chain only, with no runtime run.
I extracted the head myself, ran my own probes over the real legacy HTTP reader, and read the earlier reviews only after measuring. The new Blocker is in the B1 fix itself. The fix copied B1's suggested wording word for word. That wording rests on a premise that holds for one of the three places routing reasons come from, not all three.
Method
- Fresh
git cloneofsamovers/OFARM2,refs/pull/393/headfetched into its own worktree. Head, tree, base and +74/−3 match the PR body. - PostgreSQL 16.13 (
initdb, socket/tmp/pgman:54317), CPython 3.12.3 venv withrequirements-review-baseline.lock. The kernelStoredoes not enforce the 17.10 pin; only tenant provisioning does. - One throwaway probe module,
kernel/tests/test_zz_probe393.py, which I wrote myself. It uses onlyconftest.fresh_env,create_test_appanddemofixtures, not the author's probe. Every read goes throughGET /records/{id}withx-acting-party, exactly as the guide tells a reader to. The module was never committed. - Result:
4 passed(3.0 s + 1.2 s). The JSON captures are cited below.
Blocker B2 — "Read it alongside the evidence-gate outcome" cannot tell a routing requirement from an unmet floor item
Text at head (lines 928–931):
For routed submissions, the retained case may incorporate review-routing reasons added after the evidence gate. Its
REQUIRE_REVIEWoutcome alone does not establish that evidence was missing. Read it alongside the evidence-gate outcome and the final result's problems.
The table (line 944) teaches the same heuristic: "ALLOW; evidence gate logs SATISFIED" means "The evidence floor passed."
Violated invariant: #389's outcome and its first acceptance criterion. The guide must name "the exact existing fields used to obtain the final outcome … alongside evidence sufficiency". B1 applied the same invariant: reader guidance must not present a routing requirement as an evidence judgement. This is that invariant in reverse. The guide now points the reader to a field that presents an unmet evidence-floor item as a pass.
Mechanism (unchanged runtime, read at head):
review_route_reasonshas three producers, not one:- before the evidence gate, in
ValidationGate:validators.py:434(event time outside the plausibility window, codeEVIDENCE_INSUFFICIENT),:943,:1099(ACTOR_BINDING_UNRESOLVED),:1178/:1187(product/crop binding),:1446; - inside the evidence gate:
stages.py:542ctx.review_route_reasons.extend(floor_failures), the policy'ssoftItems["product-binding", "crop-binding"]; - after it, in
ReviewPromotionGate:stages.py:659/676/687. This is the only producer B1 considered.
- before the evidence gate, in
- Whatever the floor case decided, unless it was
REFUSE, the floor path logsctx.log("EVIDENCE_SUFFICIENCY", "SATISFIED")with no rationale (stages.py:551). A case whose soft floor item is missing (decision == "REQUIRE_REVIEW",bundleStatus: PARTIAL) is logged asSATISFIED. amend_case_for_routing(sufficiency.py) then overwritesoutcomethe same way for all three producers.
So a routed operation always shows evidence gate SATISFIED plus case REQUIRE_REVIEW, whether or not an evidence-floor item was missing. The two fields the guide names cannot tell these apart.
Measured (captures routed.json, sources.json). All three are /commit with confirmAccept: true by party:demo.farmer.one, current event time:
| Attempt | Where the route came from | Evidence gate | Retained case | Final result problems |
|---|---|---|---|---|
(a) body names reviewerPartyRef: advisor |
after the gate (B1's example) | SATISFIED, no rationale |
REQUIRE_REVIEW, all 6 arguments SUPPORTED, codes [ATTESTATION_AUTHORITY_MISSING] |
HUMAN_APPROVAL_REQUIRED "Distinct reviewer requires own act" |
(c) binding_refs=[PRODUCT_BINDING] (no crop binding) |
validation and the gate's own soft floor | SATISFIED, no rationale |
REQUIRE_REVIEW, crop-binding: REVIEW_REQUIRED, codes [MISSING_REQUIRED_EVIDENCE] |
IDENTITY_UNRESOLVED "Crop binding missing"; IDENTITY_UNRESOLVED "Floor item requires review" |
(f) event time 2999-01-01T00:00:00Z |
before the gate | SATISFIED, no rationale |
REQUIRE_REVIEW, all 6 SUPPORTED, codes [TIMESTAMP_INCOMPLETE] |
EVIDENCE_INSUFFICIENT "Event time outside plausibility window" |
The consequence is real, not a matter of wording. An advisor then calls POST /review/accept on the queued assertion with no reviewer evidence:
a_accept {'decision': 'PROMOTE_ACCEPTED', 'problems': [],
'evidenceGate': [['SATISFIED', 'all SI evidence-floor items satisfied']], 'new_cases': 1}
c_accept {'decision': 'RETAIN_DRAFT',
'problems': [['EVIDENCE_INSUFFICIENT', 'Acceptance floor unmet', 'ERROR']],
'evidenceGate': [['INSUFFICIENT', "evidence floor unmet: missing ['route-reasons-resolved']; ..."]],
'new_cases': 0}
For (c), evidence was missing. The claim cannot be accepted until a reviewer attaches new durable evidence (IDENTITY_UNRESOLVED ∈ NEEDS_EVIDENCE_CODES). Yet at commit time it showed the same evidence gate SATISFIED and the same case REQUIRE_REVIEW as (a), which an advisor accepts outright. A reader who follows the guide sees "the evidence gate passed, so this routing came in after it and does not mean missing evidence". For (c) and (f) that conclusion is wrong, and the text never says routes can arise before or inside the gate.
Calibration. The probe does detect a change in the gate outcome. With one throwaway edit to stages.py:551 (log SATISFIED_WITH_EXCEPTIONS when the case is not ALLOW), only (c) changed: SATISFIED → SATISFIED_WITH_EXCEPTIONS; (a), (b), (d) and (e) stayed SATISFIED. The file was restored afterwards (git status: only the probe untracked). So the gap the finding rests on is line 551 at the unchanged runtime. It is not a probe artifact.
High-risk fields.
- Entry point: the legacy development/conformance
POST /commit→GET /records/{id}(kernel/legacy_m1/api.py:143, 282). Production governed routes stay closed. - Actor: any farm-read principal following §8 (measured as farmer and advisor).
- Path:
ValidationGate/EvidenceSufficiencyGate:542,551→ReviewPromotionGate:719-728→PromotionEmitter._store_case→amend_case_for_routing. - Precondition: an operation claim with a missing soft floor item (crop or product binding) or a validation-time route. Both are ordinary pilot inputs.
- Consequence: the guide leads the reader to report "evidence floor passed" for a claim whose evidence floor is not met and which cannot be accepted without new evidence. That is the exact confusion #389 exists to remove.
- Reproduction: the (c) row above.
Smallest acceptable fix (documentation only; no runtime change):
- Replace "added after the evidence gate" with wording that covers all three producers. For example: "may incorporate review-routing reasons raised during validation, by the evidence gate's soft floor items, or by review routing."
- Say that on the operation/compliance floor path, evidence gate
SATISFIEDmeans only "no hard floor item was missing". It does not mean every floor item was supported, and it carries no rationale. - Name the fields that do separate the cases: the retained case's
arguments[].conclusion(REVIEW_REQUIRED/UNSUPPORTEDperruleRef) andevidenceBundles[].bundleStatus. These are not rewritten by the routing amendment. Also name the resultproblems[]titles and codes. - Optionally add (c) as a fifth table row: "
REQUIRE_REVIEWcase; evidence gateSATISFIED; floor argumentREVIEW_REQUIRED→ a soft floor item is unmet; acceptance needs new reviewer evidence."
Counter-argument, stated fairly. The sentence says "may incorporate", which is literally true. It also tells the reader to read problems[], and in (c) the problem title "Floor item requires review" does reveal the gap. #389's acceptance criterion itself uses the narrow "later routing reasons" phrasing, so the PR meets that criterion as written. I still call this a Blocker for three reasons:
- The one field the guide names for evidence sufficiency, the evidence-gate outcome, is identical in the passing and failing cases.
- The causal explanation ("after the gate") is false for two of three producers.
- The criterion's own "name the exact fields … alongside evidence sufficiency" is not met, because the only fields that separate them (
arguments[],bundleStatus) go unmentioned.
This is the same standard B1 was held to.
Follow-up F1 — the traversal's first step returns 403 to the farm owner when targetScopes has no FARM entry
The guide says: "A later authorized GET of that ID returns the stored result" (line 915), and "these reads retain their existing farm-read authorization checks" (line 910).
_read_farm_scopes (legacy_m1/api.py:227-256) looks for a FARM scope on the record. If there is none, it follows semanticEventRef and then requestId. The event envelope's anchorScopes is the caller's targetScopes verbatim (stages.py:228). The ingress request carries the same targetScopes. The contract requires targetScopes (minItems: 1, ten-value enum including FIELD). The repo's own tests submit FIELD-only scopes (test_correction_authorization.py:351).
Measured (misc.json):
scopes_field_only_op_selfreview {'decision': 'PROMOTE_ACCEPTED', 'result': 403, 'trace': 403, 'case_direct_status': 200}
scopes_field_only_obs {'decision': 'RETAIN_DRAFT', 'result': 403, 'trace': 403}
scopes_farm_and_field_op_selfreview{'decision': 'PROMOTE_ACCEPTED', 'result': 200, 'trace': 200, 'case': 200}
The farm owner, who holds a full grant, gets PERMISSION_REDACTED on her own result and trace. The case is readable, because its anchorScopes is hard-coded to FARM, but it cannot be reached without the trace. This is a failure to resolve the farm, not an authorization decision, so "existing farm-read authorization checks" does not describe it.
Why Follow-up, not Blocker. The defect is in the unchanged reader. It fails closed, disclosing nothing, and fixing it is runtime work outside this documentation boundary. If B2's fix is being made anyway, one sentence in §8 would be cheap and accurate: "the reader resolves the farm from FARM-typed scopes only; a submission whose targetScopes omits the FARM entry returns 403 for its result and trace." The runtime repair belongs in a separate Delivery.
Preference P1 — say that EVIDENCE_INSUFFICIENT in problems[] is not itself an evidence decision
Row 3 pairs EVIDENCE_INSUFFICIENT with "the evidence gate refused before promotion". The same code also appears as:
- a
WARNINGon aPROMOTE_ACCEPTEDresult with caseALLOW: dose9999.0gives['EVIDENCE_INSUFFICIENT', 'Dose-range advisory', 'WARNING'](routed.json(b); ERRATA E-006); - the routing warning in (f) above.
§8 already says to read severities and titles. One clause ("reason codes are reused across advisories, routes and refusals; read severity and title with the code") would stop the table row being read as a lookup rule. This is optional.
Checked and decided were not findings
- Table rows 1–4 reproduce exactly with my own fixtures (
table.json):- direct observation: 0 new cases,
NOT_REQUIREDwith its rationale,RETAIN_DRAFT+HIGH_CONSEQUENCE_BLOCKED; - queued observation: 1 case
ALLOW,SATISFIED, same refusal, no emitted refs; - Party-as-evidence: no case,
INSUFFICIENT,EVIDENCE_INSUFFICIENT/ERROR, and it is the last gate; - queued operation: 1 case
ALLOW,PROMOTE_ACCEPTED, one review and one consequence, thenCURRENT_STATE_MATERIALIZATION UPDATED.
- direct observation: 0 new cases,
- For all 200 reads, the stored result equals the returned body (
storedEqualsReturned: True). The trace'srequestIdandfinalOutcomematch the result. - Gate entries carry
gate/outcome/ optionalrationale/relatedArtifactRefs, neverreasonCode(stages.py:163-172). This is correct. (The DB gate log does storereason_code, but it is not reachable over HTTP.) - Rejection:
RETAIN_DRAFT,problems: [], evidence gateNOT_REQUIRED, no new case, one ReviewDecision withdecisionOutcomeState: REJECTED. Matches. - Replay (identical
/commitbody):REPLAY_REUSED_RESULT, a singleINGRESS_NORMALIZATIONgate, noevidenceSufficiencyCaseRef, emitted refs carried forward,replayOfRequestIdequal to the firstrequestId.GETon that ID returns anofarm.commitingressrequest.v0.1, not a result. Every replay sentence is accurate. - "A case alone has no result/trace backlink": correct. The case only has
subject.subjectRef= assertion. - An unconfirmed operation gives case
ALLOW+RETAIN_DRAFTwith no problems. This is a third ALLOW-without-acceptance shape not in the table, but it is covered by "Neither caseALLOWnorattestationAllowedsubstitutes for acceptance." - Read authorization: advisor reads result, trace and case (200). Worker and inspector get 403. A delegated worker can
/commit(200) but gets 403 reading his own result. That is a real grant decision (artifact_family="OTHER") and is covered by line 910. - Links: the anchor slug computes to
8-reading-retained-evidence-existing-legacy-behaviorand matches. All four../kernel/...targets and therfcs/links exist.§§3.5–3.6exist.git diff --check: clean. - Gates: no conformance checker or test reads this file.
grep -rn REVIEW_DISPUTE_SEMANTICS conformance/has zero hits, while the same grep findsdocs/rfcs/paths intemporal_contract_candidate_check.py, so the search is known to work. So the package/architecture/temporal checks cannot be moved by this diff. I did not rebuild CPython 3.12.13 to rerun them, and I rely on the author's PASS and CI run 34951372642 for that.
What I could not check
- PostgreSQL 17.10: I used 16.13. None of the paths above touch tenant provisioning or anything version-specific. The author's captures on 17.10 agree with mine on all four shared rows.
- Compliance-assertion routing (
stages.py:659, self-review of a compliance claim): not run. Its route is added after the gate, so it behaves like (a).
What my method made easier than production
demofixtures grant farmer and advisor everything, so every read that should succeed does. A narrower real grant set would add more 403s of the F1 kind, not fewer.- My probe knew in advance which producer each route came from. A real reader does not, and that is the point of B2.
- Every probe ran in one fresh database with no concurrent writers and no prior replays under the same key.
Posting note
samovers is the PR author, so GitHub will accept this only as a COMMENT review. It is not a formal REQUEST_CHANGES. Neither this review nor 5207958022 is an independent approval.
Delivery: #389 (parent #179). Current head:
f993a1af172c715f302b643bff091bd5439dbf7a.Problem, capability and boundary
The existing legacy API exposes a final commit result, a promotion trace and an optional sufficiency case, but interpreting any one status alone can confuse evidence sufficiency with acceptance. The guide documents the existing authorized result → trace → case path and its limits.
Primary boundary: documentation of evidence-decision reporting. Only
docs/REVIEW_DISPUTE_SEMANTICS.mdchanges: 93 additions / 3 deletions overall. The current B2/F1/P1 correction is +24/-5 fromf804beb. No runtime, test, fixture, inventory, contract, record, authority, endpoint or workflow changes. Routine descriptive documentation requires no new semantic approval; no prior PR approval is transferred. Every merge still requires the later exact-head task-user authorization.Final behavior described and acceptance criteria
payload, its final outcome and all problems, then its matching trace and optional case. A missing case alone does not establish evidence failure; ALLOW alone does not authorize acceptance.The five table examples are fictional and match captured existing behavior. No missing endpoint or implemented reader converting ALLOW to acceptance was demonstrated; documentation is the smallest complete outcome. Non-goals: changing evidence policy, grants, case records, promotion eligibility, replay, currentness, production posture or restarting the audit.
Review handling
Review 5207958022 found B1. Review 5209450131 then found B2 in the first correction, plus F1 and P1. The prior zero-Blocker reviews missed those points and remain historical.
B2 is corrected with all three routing sources, the qualified SATISFIED meaning, preserved per-rule conclusions, and a measured missing-crop example. One suggested remedy was wrong: bundle status is rewritten by routing; the corrected guide explicitly says so. P1's reason-code qualification is included. F1's existing limitation is documented, but its read-resolution repair is outside this PR. A separate Delivery draft is prepared; creation of that issue awaits explicit publication approval. No remote issue is claimed and no runtime repair is authorized.
Current-head bounded correction review: B2 closed; zero Blockers, one unresolved outside-boundary Follow-up (F1), zero unresolved Preferences. Report SHA256
633edd4b56ccb8c629e1a97dc2b17306d719c732e15c4b9df8a4b51cc90243c5. The review independently verified the corrected source path, exact one-file scope, whitespace and all sixteen capture digests; no second runtime test run or full audit. Earlier packets for5fedcc1andf804bebare superseded. No PR #393 merge authorization exists.Verification
f804beb: 6 passed, 1 expected Starlette warning in 4.72s. Real legacy HTTP and fictional demo records covered three route sources, advisor acceptance of the distinct-reviewer claim versus missing-crop refusal without new evidence, advisory-code reuse and FIELD-only versus FARM+FIELD reads.Excellence
EXC-001: existing record owners remain authoritative. EXC-002: no duplicate state, validation or reader. EXC-003: corrected statements trace to existing source and focused captures. EXC-004: remove misleading after-gate-only wording. EXC-005: no abstraction added. EXC-006: a guide and existing fields suffice; no new record, reverse endpoint or kernel machinery.
Technical review handling is complete. F1 issue creation and the prepared Delivery #389 status update both await explicit publication approval after automatic review rejected those issue writes. The existing Delivery description therefore still names the previous head; the corrected code and current verification are recorded here. No rejected issue body is republished here.
Next: resolve the two issue-publication approvals, complete their readbacks, then present the replacement packet unmerged for later exact-head authorization.