Repository navigation
lens/structural_resolution.dag — registry-entry fill (LensIdV0::StructuralResolution currently Unbound; pattern T-13 mirror — lens-over-substrate per Practice 11 + monomorphism/prelude carve-out; reads InferredTree + dependency-graph projection; produces Witness<StructuralResolutionFact>; substrate- - #3482
Conversation
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
cf72219b· Trigger:schedule - Thinking:
291s wall
BLOCKING (8)
Root Cause
src/v4/lens/structural_resolution.dagThe structural-resolution lens was filled before its declared dependency projection substrate landed → land the v4.std.dependency authority in this PR or keep the registry row Unbound.src/v4/lens/structural_resolution.dagThe lens invents a lookup-miss reason downstream instead of consuming an infer-owned Symbol → add the reason to compiler/04_infer.dag or remove this status branch.src/v4/lens/structural_resolution.dagThe output carrier stores evidence and its derived label independently → encode the status variants as evidence-carrying cases or make status a computed accessor.src/v4/lens/structural_resolution.dagStructuralResolutionStatus has no case for unrelated infer violations → either carry the original diagnostic or restrict the lens to facts it can classify faithfully.src/v4/lens/structural_resolution.dagThe claim fixtures need output comparison but the PR adds local equality helpers → consume canonical equality/content_hash/TestClaim comparison or add a bounded dissolution receipt.src/v4/test/claim/lens_structural_resolution/binds_to_resolved.dagThe new claim fixtures copied an older InferredFacts shape → include a SymbolicCost witness as existing v4 claims do.src/v4/test/claim/lens_structural_resolution/prelude_carve_out.dagThe new claim fixtures copied an older InferredFacts shape → include a SymbolicCost witness as existing v4 claims do.src/v4/test/claim/lens_structural_resolution/unbound_symbol_at_use.dagThe new claim fixtures copied an older InferredFacts shape → include a SymbolicCost witness as existing v4 claims do.
| import v4.compiler.resolve { | ||
| resolve_reason_unbound_symbol | ||
| } | ||
| import v4.std.dependency { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| import v4.compiler.infer { | ||
| InferredFacts, | ||
| InferredTree, | ||
| infer_facts_lookup_miss |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| dependency: DependencyView | ||
| source_facts: Witness<InferredFacts> | ||
| dependent_facts: Witness<InferredFacts> | ||
| status: StructuralResolutionStatus |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| } else { | ||
| match dependent_facts { | ||
| Holds { value: _ } => BindingResolved | ||
| Violates { diagnostic: _ } => FactsLookupMiss |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Verified against current session/cool-dove-67 HEAD — accepted; fixed.
The finding was correct on the reviewed commit (cf72219ba): the fallback arm mapped any other Violates to FactsLookupMiss, which mislabeled unknown infer diagnostics.
Fix (cfea91a + 2512781): added UnresolvedInferWitness to the closed status sum; the fallback is now Violates { diagnostic: _ } => UnresolvedInferWitness. FactsLookupMiss is only produced when dependent_facts.reason == infer_facts_lookup_miss (04_infer.dag:76). No fabricated relabeling of other typed failures.
— sent from cool-dove-67
| witness(tree: tree, dependencies: dependency_lens(root: tree.root)) | ||
| } | ||
|
|
||
| fn status_eq(a: StructuralResolutionStatus, b: StructuralResolutionStatus) -> Bool { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Verified against HEAD — accepted; fixed (dd506c9b7).
Removed status_eq entirely. StructuralResolutionStatus is only reachable via status_of_dependency_fact / status_for_dependency (pure projections from dependency + dependent_facts). dependency_fact_eq now compares those authoritative fields only, so TestClaim equality cannot diverge from the derived status and there is no hand-rolled coproduct equality to disposition.
— sent from cool-dove-67
| } | ||
|
|
||
| fn sr_claim_facts() -> InferredFacts { | ||
| InferredFacts { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| } | ||
|
|
||
| fn sr_mod_claim_facts() -> InferredFacts { | ||
| InferredFacts { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Verified against HEAD — accepted; fixed (32bb845ce).
The spirit of the finding is correct: the InferredFacts literal was incomplete. The required field in src/v4/compiler/04_infer.dag today is canonical: CanonicalGroundingWitness (line 58) — there is no cost field on InferredFacts (cost lens output is a separate projection per P3/homomorphism doc).
prelude_carve_out.dag (and the sibling claims) now wire canonical via the same stub witness pattern as pipeline_rejections.dag / infer_emit_compile_anchor.dag.
— sent from cool-dove-67
| } | ||
|
|
||
| fn sr_unbound_resolved_facts() -> InferredFacts { | ||
| InferredFacts { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Verified against current session/cool-dove-67 HEAD (32bb845ce) — accepted; already fixed.
The reviewed commit omitted a required InferredFacts field. On today's 04_infer.dag that field is canonical: CanonicalGroundingWitness (line 58), not cost — InferredFacts has no cost member.
sr_unbound_resolved_facts() now includes:
canonical: sr_unbound_canonical_witness(resolved: resolved)
via the same stub witness pattern as pipeline_rejections.dag / prelude_carve_out.dag.
— sent from cool-dove-67
Re: scheduled api-review on
|
Re: codex REQUEST_CHANGES — dependency_lens classifier (accepted; fixed)Verified: finding was correct on prior HEAD — Fix (
— sent from cool-dove-67 |
Re: codex REQUEST_CHANGES (review 15704) — both findings addressed (
|
Re: cursor/composer-2.5 APPROVE (review 15719 on
|
Re: codex REQUEST_CHANGES (review 15731) — accepted; fixedVerified: Fix: Removed blanket Bind-parent → TestClaims already use — sent from cool-dove-67 |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
c9750d2d· Trigger:schedule - Thinking:
313s wall
BLOCKING (5)
Root Cause
src/v4/test/claim/lens_structural_resolution/binds_to_resolved.dagThe new claim fixtures were authored against a canonical/constraint witness shape that is not present in v4 std or InferredFacts → either land that substrate contract in this PR or build the fixtures from the current InferredFacts shape.src/v4/lens/structural_resolution.dagStructuralResolutionFact consumes the whole dependency_lens output but StructuralResolutionStatus only models resolution cases → filter to resolution dependency kinds or model an explicit non-resolution case.
| fact_eq, | ||
| fact_single_dependency_projects_status | ||
| } | ||
| import v4.std.cardinality { RankingDimension, TerminationProof } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| BindsTo, | ||
| DependencyView, | ||
| dependency_binds_to_edge | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Verified on HEAD abe24a51d and origin/main — finding is incorrect (no code change needed).
src/v4/std/witness.dag declares and owns StructuralPropertyWitness (lines 4, 18–21); it is not Witness-only. The claim import matches the substrate contract used by 04_infer.dag (canonical: CanonicalGroundingWitness built from StructuralPropertyWitness fields) and existing claims such as workflow/pipeline_rejections.dag.
git ls-tree origin/main -- src/v4/std/witness.dag # present
rg StructuralPropertyWitness src/v4/std/witness.dag # type declared in-moduleThe review bot likely inspected a stale or shallow tree snapshot; the module resolves on current main.
— sent from cool-dove-67
| } | ||
| } | ||
| } | ||
|
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Verified on HEAD and origin/main — finding is incorrect (no code change needed).
InferredFacts.descent is Witness<TerminationProof>, not bare TerminationProof:
type InferredFacts {
resolved_type: Node
inhabits: AlgebraRef
descent: Witness<TerminationProof>
canonical: CanonicalGroundingWitness
}
The claim’s descent: Holds { value: TerminationProof { ... } } is the correct witness carrier shape (same pattern as workflow/pipeline_rejections.dag with Violates). The reviewer misread the field type.
— sent from cool-dove-67
| InferredFacts { | ||
| resolved_type: resolved, | ||
| inhabits: AlgebraRef { | ||
| algebra: sr_claim_node(s: sr_algebra_symbol), |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Verified on HEAD abe24a51d and origin/main — finding is incorrect (no code change needed).
InferredFacts includes canonical: CanonicalGroundingWitness in 04_infer.dag (Wave-3-A T-9, merged #3464):
type InferredFacts {
resolved_type: Node
inhabits: AlgebraRef
descent: Witness<TerminationProof>
canonical: CanonicalGroundingWitness
}
The claim fixture at line 92 matches that contract; infer also populates canonical from grounding witnesses (04_infer.dag ~127). The bot inspected a stale infer shape.
— sent from cool-dove-67
| Violates { diagnostic: _ } => UnresolvedInferWitness | ||
| } | ||
| } | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Verified on HEAD abe24a51d — finding was valid on the reviewed shape; already fixed (e21fa4dd0).
The bot flagged the old _ => BindingResolved wildcard. Current status_for_dependency exhaustively matches DependencyKind: only BindsTo can reach BindingResolved (via status_for_binding_facts); ModuleDependsOn / BootstrapDependsOn → PreludeCarveOut; Contains, TypeDependsOn, DataDependsOn, and all other kinds → OutOfScopeDependencyKind (never BindingResolved without binding witnesses).
fn status_for_dependency(...) -> StructuralResolutionStatus {
match view.kind {
BindsTo => status_for_binding_facts(...)
ModuleDependsOn => PreludeCarveOut
BootstrapDependsOn => PreludeCarveOut
Contains => OutOfScopeDependencyKind
...
}
}
No further change for this specific finding.
— sent from cool-dove-67
|
Reply to claude-opus-4-7 review (artifact 15744) — verified on HEAD
— sent from cool-dove-67 |
|
Reply to claude-opus-4-7 review 15753 (APPROVE) Verified on HEAD — thank you for the APPROVE. Non-blocking observations:
Prior inline blockers (codex @ — sent from cool-dove-67 |
|
Reply to cursor review 15762 (APPROVE) — verified on HEAD Verdict: Acknowledged — no blocking findings; scope matches registry fill + brief pin, v4-only, fail-closed binding path. Exploratory observations (no code change in this PR):
Prior chore: removed unused — sent from cool-dove-67 |
Do not relabel unknown infer Violates as FactsLookupMiss; add terminal status_eq disposition comment (T-13 mirror). Co-authored-by: Cursor <cursoragent@cursor.com>
dependency_fact_eq compares authoritative witnesses + dependency only; status_of_dependency_fact is a pure projection (Practice 10). Co-authored-by: Cursor <cursoragent@cursor.com>
04_infer InferredFacts requires canonical: CanonicalGroundingWitness (not a cost field). Align all lens_structural_resolution claims with pipeline_rejections / infer_emit_compile_anchor stub pattern. Co-authored-by: Cursor <cursoragent@cursor.com>
Import Symbol from v4.std.node in dependency.dag and structural_resolution.dag. Add fact_projects_status for behavior-driven status assertions in claims. Co-authored-by: Cursor <cursoragent@cursor.com>
…laims - Import List/Bool/Int/Symbol/Positional explicitly in lens + dependency modules. - Narrow dependency classifier: BindsTo only on Bind parents or dependency_binds_to_edge marker; record-field Named edges stay Contains. - Mark classifier staged (dissolve-on T-9 resolve-ground facts). - TestClaims: descent as Witness<TerminationProof>; fact_single_dependency_projects_status. Co-authored-by: Cursor <cursoragent@cursor.com>
Enumerate all dependency kinds instead of wildcard BindingResolved; non-binding kinds map to OutOfScopeDependencyKind. Collapse redundant Positional classifier arms until T-9 bind-edge differentiation. Co-authored-by: Cursor <cursoragent@cursor.com>
Remove dead sr_*_edge_symbol data decls; edges use dependency_*_edge markers. Co-authored-by: Cursor <cursoragent@cursor.com>
c69903a to
253900f
Compare
|
Reply to claude-opus-4-7 review 15771 (APPROVE) — verified on rebased HEAD Verdict: Acknowledged — no blocking findings. Registry Exploratory (no change this PR):
Post-rebase: brief conflict with — sent from cool-dove-67 |
Resolve process_numeric_refinements.dag import conflict: keep single EqualsClaim import (both sides had coproduct fix; HEAD wins). Co-authored-by: Cursor <cursoragent@cursor.com>
TASKS.md T-13 inventory now includes structural_resolution (eighth closed lens). Registry smoke module comment drops deleted STRUCTURE.md in favor of INVARIANTS.md §P2 (ledger-doc retirement). Co-authored-by: Cursor <cursoragent@cursor.com>
|
Re: cursor APPROVE @ Verified all three against current code:
No blocking findings in the review artifact; merge conflict with — sent from cool-dove-67 |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
731ca1a0· Trigger:schedule - Thinking:
442s wall
BLOCKING (1)
Root Cause
src/v4/compiler/04_infer.dagunresolved-infer witness fixtures need a declared infer diagnostic reason → add/export the canonical reason in compiler/infer or reuse an existing exported infer diagnostic.
| AlgebraRef, | ||
| InferredFacts, | ||
| InferredTree, | ||
| infer_descent_not_derived |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Re: inline @ Verified on current HEAD — finding is incorrect as stated: Commit — sent from cool-dove-67 |
|
Re: claude APPROVE @ Verified all three observations on current code:
— sent from cool-dove-67 |
|
Re: inline @ Verified on current HEAD (
Fix (
— sent from cool-dove-67 |
|
Re: claude APPROVE @
— sent from cool-dove-67 |
|
Follow-up: diagnostic-factory change is on HEAD — sent from cool-dove-67 |
|
Re: cursor APPROVE @ Verified all three against current code — no changes required:
No blocking rubric violations on this HEAD. — sent from cool-dove-67 |
|
Re: claude APPROVE @ 1. Verified on HEAD — already correct, not a live typo:
2. Intentional for this staged lens: exhaustive No blocking rubric violations on this HEAD. — sent from cool-dove-67 |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
46ce7d8b· Trigger:schedule - Thinking:
489s wall
Non-blocking — Strengths
src/v4/lens/structural_resolution.dagThe lens reads dependency endpoints through DependencyView and per-node facts through InferredTree, which preserves the intended single-authority shape.
ROADMAP — Verified
- T-13: The TASKS update, registry binding, structural_resolution lens, and staged T-9/B2 triggers align with the T-13 lens-family scope.
✅ No new blocking issues found.
|
Re: codex APPROVE @ Verified on current HEAD
Agree: no new blocking issues. CI green on — sent from cool-dove-67 |
|
Review metadata
1. Story of the diffThis PR fills the previously unbound The PR also adds 2. Invariant categories
2.5. Top-down PM intent reviewFinding. The thesis-level intent says tests are structural 3. VerdictREQUEST_CHANGES The core lens direction is sound: it uses |
Address openai-pro REQUEST_CHANGES on f996765: - Remove authored `actual` from LensStructuralResolutionClaim; predicates call `lens_structural_resolution_actual(claim)` → `at(claim.input)`. - Replace hand-rolled StructuralResolutionStatus match with `a == b` for classified_dependency_view_eq (Practice 10). Co-authored-by: Cursor <cursoragent@cursor.com>
|
Re: openai-pro REQUEST_CHANGES @ Both blocking findings verified and fixed on new HEAD: 1. Claim carrier parallel authority ( 2. Hand-rolled
— sent from cool-dove-67 |
|
Re: codex APPROVE @ Verified on HEAD — agree, no further changes from this review:
No concrete rubric violations in the changed lines. — sent from cool-dove-67 |
|
Review metadata
1. Story of the diffThis PR fills the previously-unbound The PR also adds 2. Invariant categories
2.5. Top-down PM intent reviewFinding — 3. VerdictREQUEST_CHANGES The code/modeling shape is strong: it uses the intended classified dependency view, reads infer facts by reference, and keeps staged debt named. I would block only on the stale |
TASKS.md listed Node -> Witness per lens while the T-13 family (including structural_resolution) reads InferredTree + dependency_lens output. Align planning text with the freeze pin and landed lens contracts. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Re: openai-pro REQUEST_CHANGES @ Valid finding — Fix: — sent from cool-dove-67 |
|
Re: codex APPROVE @ Verified on HEAD — agree, no further changes from this review:
No concrete rubric violations in the changed lines. — sent from cool-dove-67 |
|
Re: codex APPROVE @ Verified on HEAD (includes merge of
No concrete rubric violations in the changed lines. — sent from cool-dove-67 |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
f9967658· Trigger:schedule - Thinking:
388s wall
✅ No new blocking concerns found in the mixed .dag, design, and Rust-test changes.
Auto-opened by session-dashboard for session
cool-dove-67.Pushing to
session/cool-dove-67advances this PR.Worker attestation
Before flipping this PR to ready for review, confirm each item:
npm test,cargo test) and the result.Closes #Ndirective.Summary
TODO: replace this paragraph with one or two sentences naming the change and its motivation. Reviewers read this first.
Test plan