Repository navigation
Conversation
… in emit Add TypedItemKind enum to EmitGraphInfo with 10 variants (Struct, Enum, TypeAlias, TypeDecl, Function, TransportFunction, DataDef, ServiceDef, ResourceDef, Unhandled). classify_typed_item runs once during infer and stores results in item_kinds map. All three emit backends now dispatch on precomputed kind instead of re-interrogating Node properties. - Add classify_typed_item + lookup_item_kind + helper predicates to emit_info.dag - Move build_item_kinds to infer.dag (avoids circular dep with infer_items) - Replace ~40 ad-hoc classification branches across emit_rust/go/python/generic - Extract build_fn_emit_info helper in emit_rust for ownership threading - 289 tests pass, 0 diagnostics, clippy clean Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…gation in emit ServiceFieldSet (has_rest, has_shell, has_file, has_auth) is now computed once during inference via classify_service_fields and stored on EmitGraphInfo. All three emit backends (Rust, Go, Python) look up the precomputed struct via lookup_service_fields instead of calling compute_service_fields at each use site. Removes compute_service_fields and service_has_* helpers from 05_emit.dag. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…dy extraction in emit FunctionSignature precomputes params, return_type, body, and uses during inference so emit backends read structured facts instead of reaching into raw Node fields. All three emit backends (Rust, Go, Python) now use lookup_function_signature + sig fields instead of item.params/rt_type/body. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ility inspection in emit Add CapabilitySignature and ResourceDefinition types to EmitGraphInfo. classify_resource_definition extracts capability name, params, and return_type during reconcile so all three emit backends (Rust, Go, Python) read precomputed facts instead of reaching into Node children. Dispatch sites now use lookup_resource_definition match, and emit_resource_def / emit_capability_method take the precomputed types instead of raw Nodes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…alysis Ownership analysis was running twice: once in compile.dag for diagnostics, then again in emit_rust via build_ownership_index which re-called analyze_ownership per function. Now the pre-computed proofs from compile flow through emit_from_artifact_plan → emit_artifact → emit_rust, and build_ownership_index simply indexes the existing proofs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 841276f865
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| pub fn build_item_kinds(modules: Rc<Vec<Rc<TypedModule>>>) -> Rc<HashMap<String, TypedItemKind>> { | ||
| modules.iter().cloned().fold(v2_rt::rc_empty_map::<TypedItemKind>(), |acc: Rc<HashMap<String, TypedItemKind>>, tm: Rc<TypedModule>| tm.items.clone().iter().cloned().fold(acc.clone(), |inner: Rc<HashMap<String, TypedItemKind>>, item: Rc<Node>| v2_rt::rc_map_insert(inner.clone(), item.name.clone(), classify_typed_item(item.clone())))) |
There was a problem hiding this comment.
build_item_kinds indexes metadata with item.name alone, and the same unqualified key is then used by build_service_fields, build_function_signatures, and build_resource_definitions; this causes cross-module collisions because top-level item names are not globally unique (e.g. AuthScheme exists in both dsl/std/cloud.dag and dsl/std/types.dag). After this change, emitters rely on lookup_item_kind/lookup_* for every item, so a later module can overwrite an earlier module’s classification/signature and make the earlier item emit incorrectly or disappear.
Useful? React with 👍 / 👎.
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 3, MODELING: 1+/1-, ROADMAP: 0✓/1!)
INVARIANTS — Violations (3)
MODELING — Strengths
src/v2/04_emit_info.dagTypedItemKind is the right closed sum for item classification, so struct/enum and fn/transport-fn dispatch now composes as data instead of repeated connective/use checks.
MODELING — Improvements
src/v2/04_emit_info.dagFunctionSignature, ServiceFieldSet, and ResourceDefinition should be attached by structural item reference or carried on the item itself rather than re-looked-up through Map<String, ...>, so the boundary can actually move toward M4 structural identity.
ROADMAP — Incomplete
- M2 boundary sufficiency: The new emitter-facing fact tables still rely on item-name lookups and miss defaults, so emit is still compensating for an insufficient boundary instead of consuming a fail-closed structural contract.
ROADMAP — Unclaimed
- CM semantic fact precomputation: This diff does advance the CM direction by precomputing item/service/function/resource facts in EmitGraphInfo, but ROADMAP.md was not updated to record that partial milestone progress.
The PR moves several emitter decisions upstream, but it still introduces new name-keyed shadow authorities and fail-open lookup paths that break the repo’s structural-identity and fail-closed rules.
| ) | ||
| } | ||
|
|
||
| fn build_item_kinds(modules: List<TypedModule>) -> Map<String, TypedItemKind> { |
There was a problem hiding this comment.
Invariant violation: build_item_kinds/build_service_fields/build_function_signatures/build_resource_definitions index new emit authorities by item.name, which keeps the reconcile→emit boundary name-driven instead of structural and violates No duplicate representations and Boundary sufficiency.
| } | ||
|
|
||
| // Lookup precomputed service fields from EmitGraphInfo. | ||
| fn lookup_service_fields(emit_info: EmitGraphInfo, name: String) -> ServiceFieldSet { |
There was a problem hiding this comment.
Invariant violation: lookup_service_fields fabricates ServiceFieldSet { false, false, false, false } on miss, so emit silently omits required service config instead of failing closed, violating No fallbacks that fabricate.
| } else if (kind == TypedItemTypeDecl) { | ||
| "" | ||
| } else if (kind == TypedItemTransportFunction) { | ||
| match lookup_function_signature(emit_info: emit_info, name: item.name) { |
There was a problem hiding this comment.
Invariant violation: The new None => "" branch silently drops a function definition when function_signatures misses instead of producing a diagnostic, violating No fallbacks that fabricate.
The hand-edited scaffold_for_target call doesn't match what the compiler generates from the .dag source. Use the literal ".go" that the code generator produces. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 5, MODELING: 1+/1-, ROADMAP: 0✓/1!)
INVARIANTS — Violations (5)
MODELING — Strengths
src/v2/04_emit_info.dagTypedItemKindis a properly closed sum, so moving Struct/Enum and Function/TransportFunction classification to reconcile is better layering than re-deriving those cases in each backend.
MODELING — Improvements
src/v2/04_emit_info.dagFunctionSignature,ServiceFieldSet, andResourceDefinitionare still stored behindMap<String, ...>identity-proxy tables, so the model duplicates Node authority instead of composing from structural references as M4/M9 require.
ROADMAP — Incomplete
- M2 acceptance: The new boundary still fabricates on misses (
TypedItemUnhandled, empty strings, false service-field defaults), so emit is not yet consuming a fully fail-closed structural contract.
ROADMAP — Unclaimed
- CM concept categories (item classification / transport-service / function signatures / property-resource): This diff precomputes exactly those semantic facts in
EmitGraphInfo, but ROADMAP.md does not record the advance.
The PR moves useful classification work upstream, but it still relies on sentinel/name proxies and adds new fail-open miss paths, so the reconcile→emit boundary remains structurally unsound.
| | TypedItemResourceDef | ||
| | TypedItemUnhandled | ||
|
|
||
| // Check if a return type indicates a type alias (not Unit). |
There was a problem hiding this comment.
Invariant violation: is_type_alias_return_node decides alias-vs-decl by special-casing the string name "Unit", which keeps item classification name-driven instead of structural and violates Boundary sufficiency / Heuristics indicate lost structure.
| } else { | ||
| TypedItemUnhandled | ||
| } | ||
| } |
There was a problem hiding this comment.
Invariant violation: lookup_item_kind collapses a missing boundary fact into TypedItemUnhandled, a fabrication fallback that violates M5 / No fallbacks that fabricate.
| } | ||
| } else if (kind == TypedItemDataDef) { | ||
| emit_go_data_def(name: item_text, type_node: item.type_annotation.value, value: item.body.value, registry: registry, scope: scope) | ||
| } else if (kind == TypedItemServiceDef) { |
There was a problem hiding this comment.
Invariant violation: emit_go_typed_item returns "" when lookup_resource_definition misses, silently deleting a resource interface instead of failing closed, violating No fallbacks that fabricate.
| emit_py_enum_from_children(name: item_text, children: item.children, env: env) | ||
| } else if (kind == TypedItemTypeAlias) { | ||
| concat(item_text, " = ", emit_node_type(n: rt_type(n: item), target: Python)) | ||
| } else if (kind == TypedItemTypeDecl) { |
There was a problem hiding this comment.
Invariant violation: emit_py_typed_item turns missing precomputed function/resource facts into "", silently dropping definitions instead of diagnosing the broken boundary, violating No fallbacks that fabricate.
| movable: fn_movable, | ||
| owned_bindings: empty_map() | ||
| } else if (kind == TypedItemTransportFunction) { | ||
| match lookup_function_signature(emit_info: emit_info, name: item.name) { |
There was a problem hiding this comment.
Invariant violation: emit_typed_item turns missing precomputed function/resource facts into "", silently dropping emitted items instead of producing a diagnostic, violating No fallbacks that fabricate.
ChatGPT ReviewI’ve narrowed the review to a refactor that centralizes item classification and ownership into precomputed |
- Add TypeSummary/EnumRepr/lookup_emit_type_summary imports to Go and Python emit .dag files so all three backends read precomputed unit_only from EmitGraphInfo instead of re-walking enum children at render time. - Use match on Bool instead of if(not ...) for .dag compatibility. - Reconcile stage0 .rs files after merge with main: cm-track types (TypedItemKind, FunctionSignature, etc.) as base, main's fold_eligible and ownership features layered on top. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ChatGPT ReviewI reviewed the uploaded diff. Biggest takeaways:
Pasted markdown (2) ROADMAP I would block on this. The fix is to start from
The new fact tables are a good idea, but the miss path is too permissive:
Pasted markdown I would turn these into diagnostics /
In
If top-level duplicate names across modules are legal, these tables will alias and later modules will overwrite earlier ones. If the compiler already guarantees global uniqueness here, then this is fine; if not, these should be module-qualified keys. What looks good:
On roadmap review: ROADMAP If I were leaving PR comments, I’d mark #1 and #2 as merge blockers, and #3 as a strong follow-up / likely bug depending on authored-name semantics. |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 4, MODELING: 1+/2-, ROADMAP: 0✓/0!)
INVARIANTS — Violations (4)
MODELING — Strengths
src/v2/04_emit_info.dagFunctionSignature,ResourceDefinition, andServiceFieldSetmove emitter needs into explicit boundary products, which is the right M2 direction because the facts are computed once and consumed downstream.
MODELING — Improvements
src/v2/04_emit_info.dagTypedItemKindduplicatesItemKindplus type/transport facts as a second flat taxonomy; composing the existing authorities would model the domain more faithfully.src/v2/04_emit_info.dagServiceFieldSetis effectively a small capability powerset/lattice, so if it needs merges or comparisons it should reuse algebraic/set grounding instead of four ad-hoc Bool fields.
ROADMAP — Unclaimed
- M2: Boundary Sufficiency / CM: Compiler Concept Modeling: The PR adds new emit-boundary fact tables and partial ownership threading for these tracks, but ROADMAP.md was not updated to record that partial progress.
The PR moves emit toward explicit boundary facts, but several new consumers still fail open and the ownership-threading change does not land end-to-end.
| } | ||
|
|
||
| // ========================================================================= | ||
| // Entry point -- Rust target |
There was a problem hiding this comment.
Invariant violation: The new ownership parameter adds a boundary that emit_rust still does not use as authority, so this introduces speculative metadata and violates Explicit boundary contracts / No parallel implementations.
| } else { [] } | ||
| None => [] | ||
| } | ||
| ) |
There was a problem hiding this comment.
Invariant violation: collect_workflow_funcs turns a missing function_signatures fact into [], so workflow default validation silently skips broken functions instead of failing closed, violating No fallbacks that fabricate / Explicit boundary contracts.
| let kind = lookup_item_kind(emit_info: emit_info, name: item.name) | ||
| if (kind == TypedItemStruct) { | ||
| emit_go_struct_from_children(name: item_text, children: item.children, env: env) | ||
| } else if (kind == TypedItemEnum) { |
There was a problem hiding this comment.
Invariant violation: This new None => false branch fabricates unit_only = false when the enum-summary boundary is missing, so Go emit chooses a tagged-union path instead of failing closed, violating No fallbacks that fabricate / Explicit boundary contracts.
| let kind = lookup_item_kind(emit_info: emit_info, name: item.name) | ||
| if (kind == TypedItemStruct) { | ||
| emit_py_dataclass_from_children(name: item_text, children: item.children, env: env) | ||
| } else if (kind == TypedItemEnum) { |
There was a problem hiding this comment.
Invariant violation: This new None => false branch fabricates unit_only = false when the enum-summary boundary is missing, so Python emit chooses a tagged-union path instead of failing closed, violating No fallbacks that fabricate / Explicit boundary contracts.
Three missing models (MM-1 item identity, MM-2 type structure, MM-3 expression semantics) generate all the heuristic if-else forests across the compiler. Full per-file inventory with line numbers, cross-PR pattern analysis, and design directions. Shifts CM from incremental ratchet fixes to holistic model design. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 2, MODELING: 1+/1-, ROADMAP: 1✓/1!)
INVARIANTS — Violations (2)
MODELING — Strengths
src/v2/04_emit_info.dagFunctionSignatureandCapabilitySignatureat least factor repeated emitter field-plucking into explicit products instead of making each backend rediscover the same tuple of facts.
MODELING — Improvements
src/v2/04_emit_info.dagServiceFieldSetwould be more compositional if it were derived from a transport-capability structure rather than flattened into four booleans.
ROADMAP — Verified
- CM full analysis: The new roadmap pointer is backed by this diff because
src/v2/CM.mdis added and contains the promised heuristic inventory and design notes.
ROADMAP — Incomplete
- M2 Boundary Sufficiency: The new emit boundary is still not fail-closed or single-authority because item identity is duplicated and Rust emit still fabricates on missing enum summaries.
The PR improves CM documentation and factors some emitter inputs upward, but it still adds duplicate item modeling and a new Rust fail-open path, so it does not yet meet the invariant bar for M2.
| // TypedItemKind — enriched item classification precomputed during reconcile. | ||
| // | ||
| // Eliminates post-classification re-interrogation of connective and uses: | ||
| // TypeDef → split into Struct / Enum (no connective re-check) |
There was a problem hiding this comment.
Invariant violation: TypedItemKind introduces a second closed item-classification authority alongside the existing item model, so this change adds a parallel representation instead of fixing the upstream item identity root cause, violating No duplicate representations / Root-Cause Depth.
| if (kind == TypedItemStruct) { | ||
| let type_params = emit_type_params(params: item.params, source_index: env.source_index) | ||
| emit_struct_from_children(name: item_text, type_params: type_params, children: item.children, recursive_types: emit_info.recursive_type_set, rc_types: rc_types, env: env, emit_info: emit_info) | ||
| } else if (kind == TypedItemEnum) { |
There was a problem hiding this comment.
Invariant violation: This None => false branch fabricates unit_only = false when the enum summary boundary is missing, so Rust emit repairs a broken boundary downstream instead of failing closed, violating No fallbacks that fabricate / Explicit boundary contracts.
…eFieldTemplates) Resolve conflicts by taking main's .dag and stage0 versions — main's architecture diverged (TypedItemTypeDef, shared_types rename, data-driven service fields). cm-track's unit_only wiring for Go/Python is deferred to holistic CM work tracked in src/v2/CM.md. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 0, MODELING: 0+/0-, ROADMAP: 1✓/1!)
ROADMAP — Verified
- CM full analysis: The new ROADMAP reference is supported by the diff because
src/v2/CM.mdis added and contains the promised heuristic inventory and design directions.
ROADMAP — Incomplete
- PR #322 M2 + CM structural type knowledge / ownership threading: The diff is documentation-only, so it does not implement structural type knowledge or ownership-threading changes in the compiler files named by that scope.
The diff usefully documents the CM problem space, but it does not land the broader M2 and ownership work implied by the PR scope.
ChatGPT ReviewI reviewed the attached diff, not the live GitHub PR page. The uploaded patch is docs-only: Overall: good doc addition, no executable invariant violation introduced by this diff, but I’d leave a few comments before merge. Main findings Medium
That would invert the boundary:
Sometimes the missing thing is not a compiler enum; it is declaration-driven authority in
The surrounding prose still says this section is for “Theory, long-horizon, and items that land after root-cause tracks complete,” but the heading now says CM is the primary focus. Those two signals conflict. Either move CM out of that long-horizon bucket or keep it framed as a continuous/parallel track. Low
“Let consumers pattern-match on the structural facts directly” can easily preserve the current re-derivation problem under a different spelling. If each stage writes its own raw-node predicates, you still have multiple classification forests. Even if you reject taxonomies, you still need one shared boundary contract / shared structural query layer.
Useful today, but brittle in a long-lived design doc. I’d move that section to a dated appendix or branch-review log.
The nullary-call section is one of the most actionable parts of the doc, especially the Go/Python note. Since the roadmap already has call/reference-syntax work, I’d cross-link this explicitly rather than leaving it only in Invariant check I do not see a direct invariant violation introduced by this patch, because the patch is documentation only. In fact, the strongest part of the PR is that
That said, the doc should be tighter wherever it proposes new intermediate authorities. Per Compositional modeling quality for N/A for this diff. No For the modeling itself, though, the MM-1 / MM-2 / MM-3 decomposition is good. The new doc does a strong job of collapsing a lot of scattered symptoms into a small number of missing concepts. The principles section is the best part of the patch. Roadmap alignment Directionally, this is aligned. The CM summary points at the same family of problems the roadmap already cares about: structural identity, declaration-driven algebra, name-proxy deletion, and moving semantic authority out of emit. What is missing is explicit mapping from the new MM buckets to existing roadmap lanes. Right now the new section is a good summary, but it still reads more like design prose than a roadmap node. I’d want a small mapping such as:
Without that, “primary focus” risks becoming parallel narrative instead of decomposition of current milestones. General code review Since this diff is docs-only, I don’t see a new runtime regression in the patch itself. The one broader note is scope signaling: the PR title mentions structural type knowledge, concept modeling, and ownership threading, but the attached diff only contains docs. If that is intentional, fine. If not, the uploaded diff may be incomplete. My merge read: good doc, no blocking code issue in the attached patch, but I’d leave comments on the authority/layering wording in |
- CM moves from Layer 3 (deferred) to active lane in critical path - Add concrete baseline counts and grep-based acceptance tests that can't be gamed by hiding heuristics behind helpers - MM-1/2/3 each get work items and measurable done criteria - CM.md gets matching acceptance section with runnable commands - ROADMAP critical path diagram updated to show Lane 4 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Acceptance criteria are now "this pattern is unrepresentable" not "we counted zero instances." Each MM criterion is a claim about what types and module boundaries prevent, verifiable by checking imports/types. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 0, MODELING: 0+/0-, ROADMAP: 1✓/2!)
ROADMAP — Verified
- CM lane scaffolding: ROADMAP now points Lane 4 to
src/v2/CM.md, and this diff adds that file with the promised MM-1/MM-2/MM-3 sections plus a heuristic inventory.
ROADMAP — Incomplete
- CM ratchet baselines: The new CM ratchet is not mechanically grounded yet: the documented commands on the current tree yield 45 structural-interrogation sites, 25 method-name dispatch sites, and 13
classify_*hits, while the side-table grep already returns 0, so the published baselines and acceptance checks are not reproducible. - PR #322 ownership threading: The PR title claims M2 structural type knowledge and ownership threading, but this diff only adds roadmap/design docs and does not change
04_infer.dag,05_emit*.dag, or ownership code to support that milestone progress.
The CM documentation is directionally useful, but this PR is documentation-only and its new roadmap ratchets are not yet grounded in reproducible code-backed measurements.
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 0, MODELING: 0+/0-, ROADMAP: 2✓/0!)
ROADMAP — Verified
- Lane 4 / CM ownership:
ROADMAP.mdnow assigns Lane 4 to compiler concept modeling and this diff addssrc/v2/CM.mdas the referenced design artifact for that lane. - CM heuristic inventory: The new roadmap claim that there is a full CM analysis is supported by the added
src/v2/CM.md, which enumerates MM-1/MM-2/MM-3 and the per-file heuristic inventory it points to.
No new invariant violations are introduced in the changed lines; this PR is a documentation/roadmap addition and its new roadmap references are supported by the added CM analysis document.
ChatGPT ReviewI reviewed the uploaded diff. This PR is docs-only: it modifies Overall: the direction is good. The new CM lane correctly names a real root cause: repeated downstream re-derivation of facts that should be structural, which is exactly the territory covered by the invariants around lost structure, fail-open fallbacks, and single authority. The inventory in I’d leave two substantive review comments and a couple of smaller ones. 1. MM-3 is at risk of replacing string dispatch with a new compiler-owned enum, not with data-driven semantics.
My ask here: either explicitly tie 2. MM-2 is under-aligned with the existing TypeRendering/coercion roadmap, so it reads like a parallel plan.
So the issue isn’t that MM-2 is wrong; it’s that it doesn’t explicitly anchor itself to the existing E-track/M5 authority. Right now I can read three competing “final authorities” out of the docs: 3. MM-1’s design choice is still too open-ended relative to its acceptance criteria. In
But I’d narrow this. The doc should say which consumers, if any, are allowed to pattern-match on raw structural facts, and which boundary must already carry resolved item facts. Otherwise future work can “satisfy” MM-1 by merely relocating classification rather than deleting the duplicated taxonomies and fail-open side tables it’s calling out. 4. The “How to verify” checks are too import-based to prove the invariant.
I’d strengthen the acceptance criteria toward boundary shape, for example:
That would be much closer to the invariants’ “typed boundaries make bad states unrepresentable” and “no fallbacks that fabricate” standard. A couple of smaller notes:
So my merge recommendation would be: good direction, but request doc changes before merge. The two things I’d want fixed are:
|
Classifications (type/function/service) are surface sugar — emergent compositions of structure, evidence, and morphism. The compiler should work in terms of the primitives, making classification unnecessary. Same pattern as recursion dissolving into fold/descend/repeat. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 1, MODELING: 0+/0-, ROADMAP: 1✓/1!)
INVARIANTS — Violations (1)
ROADMAP — Verified
- CM lane documentation: ROADMAP now points Lane 4 to
src/v2/CM.md, and the diff does add that analysis document.
ROADMAP — Incomplete
- PR #322 M2 + ownership threading: The diff is documentation-only, so it does not substantiate the advertised M2 structural-type-knowledge or ownership-threading progress beyond planning text.
The CM documentation is useful and the roadmap link is backed by the new file, but the MM-1 plan still leaves a non-root-cause classification wrapper in scope and the PR does not evidence the broader M2/ownership progress named in its scope.
| themselves ARE the facts. The four taxonomies are redundant | ||
| interpretations that exist because no single taxonomy felt complete. | ||
|
|
||
| **Design direction:** Either (a) compute classification once and carry |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
ChatGPT ReviewI reviewed the uploaded diff rather than the live PR page. This PR is docs-only: My verdict: request changes. I like
At diff lines 9–15, the roadmap adds
The doc is directionally right about pushing facts upstream, but it repeatedly floats things like
The strongest warning sign is the combination of CM lines 219–223 (“either carry
Several things CM claims under MM-2/MM-3 are already owned elsewhere in the roadmap, especially the algebra-fidelity work:
The new roadmap acceptance table at diff lines 154–158 mostly says “emit modules don’t import X.” That is a decent grep ratchet, but it is not the real invariant. The real invariant is boundary sufficiency / explicit boundary contracts: emit should receive a type that already carries the needed fact, rather than merely not importing a symbol. I would keep the grep checks as ratchets, but rewrite the claims in terms of boundary types and authority, not import hygiene. Pasted markdown A few smaller comments:
So the short review is:
The cleanest fix would be: keep |
- Resolve ROADMAP conflicts with main (CX count, emergent properties principle paragraph) - Reframe CM as cross-cutting design lens, not competing lane owner - Acceptance criteria rewritten as boundary type claims, not import hygiene - Add invariant guardrail: new fact layers must be exact, non-lossy, and land with a downstream consumer in the same change Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ChatGPT ReviewReviewed the attached diff. This PR is docs-only: My take: the direction is mostly good and much more invariant-aware than the old “move the heuristic upstream” pattern, but I would tighten the MM-1 design before merge. 1. Invariant reviewNo executable invariant violation lands here, since nothing runnable changed. The risk is doc-level: a few parts of the new CM framing still leave room for an implementation that would violate the invariants. Main issue: MM-1 is internally inconsistent.
Those three ideas do not currently line up. As written, “option (b)” can be read as permission to keep dispatching on The doc needs to pick one target architecture and state it plainly. My recommendation:
That preserves “classification is not primitive” while also preserving boundary sufficiency. Second issue: MM-2 may accidentally bless a lossy proxy.
I would change the text to say: Third issue:
Right now that part is philosophically strong but operationally under-constrained. 2. Compositional modeling qualityThere are no For the proposed model: Strongest parts
Weakest part
That section needs one crisp end-state. Specific modeling preference
3. Roadmap alignmentMostly good. What works:
What needs cleanup:
That framing is muddled. Pick one:
I would not keep both phrasings. Minor nit:
4. General review / concernsBecause this is docs-only, I do not have runnable bug findings. The main concerns are doc quality and future implementation ambiguity. A few smaller notes:
Bottom lineGood direction overall. The new CM writeup is much closer to the repo’s invariants than the old “push the heuristic around” approach, and the roadmap guardrail is especially good. I would still ask for a revision before merge to resolve the MM-1 ambiguity and to clarify that |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 1, MODELING: 0+/0-, ROADMAP: 1✓/1!)
INVARIANTS — Violations (1)
ROADMAP — Verified
- CM lane / src/v2/CM.md: The new CM lane entry is backed by the addition of src/v2/CM.md in the same diff.
ROADMAP — Incomplete
- M4 Tier 2.6 / MM-3: src/v2/CM.md still leaves nullary invocation as NullaryCallBinding versus ExprVar->ExprCall alternatives, so function application is not yet committed to the IR-node model the roadmap says should dissolve the heuristic.
The CM roadmap promotion is supported by the new document, but the design still preserves a downstream nullary-call workaround instead of fixing the expression-level root cause.
| `foo` distinction. Target languages need `()`. Currently the emitter | ||
| re-derives this from type names or registry lookups (`04_emit_rust.dag` | ||
| lines 1685-1750). Inference already knows at `04_infer.dag:970-977`: | ||
| `binding_kind = FunctionValueBinding` + `fsig.params |> count == 0`. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 3, MODELING: 0+/0-, ROADMAP: 2✓/1!)
INVARIANTS — Violations (3)
ROADMAP — Verified
- CM heuristic inventory: The new roadmap reference to
src/v2/CM.mdis supported by the added file in this diff. - CX (164 → 0): The updated critical-path label matches the roadmap’s current ratchet count of 164 complexity violations.
ROADMAP — Incomplete
- Lane structure: The roadmap now introduces CM as Lane 4, but the Layer 2 intro still says only “Lanes 1–3 run in parallel,” so the lane model is not updated consistently.
The PR adds useful CM inventory, but it still proposes new duplicate semantic authorities and mis-models compilation as evaluation, so it is not yet invariant-aligned.
| | Normal form | Fully evaluated — no more redexes (= emitted target code) | | ||
| | fold/descend/repeat | **Rewrite strategies**: the order in which reductions are applied | | ||
| | Termination proofs | Standard rewrite-termination via ranking functions (already in termination.dag) | | ||
| | Compilation itself | **Reduction to normal form** in the target language | |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
|
||
| **Design direction:** The connective primitives stay — they're | ||
| foundational. What's missing is a cached interpretation layer. Options: | ||
| - `TypeShape` (Product/Coproduct/Scalar) computed once per type |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| they destructure with `_` and emit bare identifiers. They likely have | ||
| the same bug for nullary functions. | ||
|
|
||
| **Design direction for method dispatch:** Add `AlgebraMethodKind` enum |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 2, MODELING: 0+/0-, ROADMAP: 1✓/1!)
INVARIANTS — Violations (2)
ROADMAP — Verified
- CM design reference: The roadmap’s new
src/v2/CM.mdreference is supported by the addedsrc/v2/CM.mdand companion inventory file in this diff.
ROADMAP — Incomplete
- CM as an active lane: This PR only adds analysis docs, so the new claim that CM informs lanes 1-3 is not yet backed by any same-diff compiler boundary or consumer changes.
The PR adds useful CM documentation, but it also introduces new duplicate-authority modeling proposals and promotes CM in the roadmap faster than the code changes support.
| **Modeling implication:** Field-presence assertions (category 3) dissolve | ||
| directly with the Node decomposition. Instead of checking `body != none` | ||
| and `transport != none` separately, consumers check `has_reduction?` and | ||
| `reduction_kind?` (internal/external). Name-based checks (category 1) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
|
||
| **Modeling implication:** Method name dispatch (category 1) dissolves | ||
| when methods carry an AlgebraMethodKind or a RewriteRuleKind. The | ||
| "fold" special cases in ownership.dag dissolve when fold is a |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
ChatGPT ReviewI reviewed the attached diff against 1. Invariant reviewNo executable invariant violation lands in code here, because nothing runnable changed. The risk is at the design-doc level: the new CM framing still leaves one of the key boundaries underspecified. The main issue is MM-1. In
But later, the acceptance criteria say emit must not access raw item structural fields ( I would tighten MM-1 to say one thing plainly: emit must receive an exact, lossless item-facts boundary type and must not re-open raw item structure to rediscover semantics. That matches the repo’s boundary-sufficiency rule, the “always enrich the boundary” rule, and the existing requirement that the wrong question should be unaskable by type shape. A second invariant risk is MM-2. The document floats “make One thing I do like: the new roadmap guardrail that any new fact layer must be exact, non-lossy, and consumed by a real downstream user in the same change is exactly the right rule. It is aligned with the invariant language on end-to-end boundaries and speculative metadata. INVARIANTS 2. Compositional modeling quality for
|
Add Signal/Algebra/Reduction ontological framing to CM.md with: - Three foundational categories and working candidate (Reduction) - Node/DAG as derived structure (theorem, not axiom) - Validation: every heuristic forest maps to S/A/R categories - Known gaps: Naming/Reference, Multiplicity, Maps-between-structures - Compilation is translation (functor), not reduction to normal form Fix invariant violations from review: - Remove NullaryCallBinding as option; commit to ExprVar→ExprCall - Remove TypeShape proposal; consume connective/TypeSummary.repr - Remove AlgebraMethodKind proposal; surface existing method_def - Remove ClassifiedItem proposal; consume existing Node fields - Add practical principle: consume existing authorities, never duplicate - Reframe all design directions: "surface existing X" not "add new Y" Add CM-inventory.md companion with ~150 heuristic sites catalogued across all compiler .dag files, mapped to ontological categories. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ROADMAP: CM is a cross-cutting retrospective discipline, not Lane 4. After each feature lands, check for new modeling gaps. Modeling gaps reveal themselves through the work, not in advance. Add "consume existing authorities" principle to ROADMAP CM section. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 3, MODELING: 0+/0-, ROADMAP: 1✓/0!)
INVARIANTS — Violations (3)
ROADMAP — Verified
- CM lane documentation: The diff adds
src/v2/CM.mdandsrc/v2/CM-inventory.md, so promoting CM from a deferred note to an explicit roadmap lane is supported by the change.
The CM docs resolve several prior root-cause issues, but the roadmap and inventory still reintroduce duplicate semantic authorities for method and item identity.
|
|
||
| **Acceptance:** Connective interpretation is unrepresentable in emit. | ||
| Emit modules do not import `Connective` / `Conj` / `Disj` / | ||
| `NoConnective`. They receive a resolved type structure and dispatch on |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| **Acceptance:** Connective interpretation is unrepresentable in emit. | ||
| Emit modules do not import `Connective` / `Conj` / `Disj` / | ||
| `NoConnective`. They receive a resolved type structure and dispatch on | ||
| that. Type rendering is an exhaustive match on a sum type. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| | Lines | Heuristic | Cat | Question | With ontology | | ||
| |-------|-----------|-----|----------|---------------| | ||
| | 83-112 | `inferred_to_outputs`: connective + name checks | A | How many output fields? | Algebra (Product: expand fields, Coproduct: wrap) | | ||
| | 114-131 | `item_kind`: 7-branch priority chain | R | What emission category? | Node decomposition: `has_reduction? what_kind?` replaces 7 branches | |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
ROADMAP MM-1/2/3: rewrite work items and acceptance criteria to consume existing authorities (body/transport/connective, TypeSummary.repr, method_def/AlgebraFieldTemplate) — no new enums or classification types. CM-inventory: replace all AlgebraMethodKind/RewriteRuleKind/has_reduction references with "surface existing X" language. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 2, MODELING: 0+/0-, ROADMAP: 1✓/0!)
INVARIANTS — Violations (2)
ROADMAP — Verified
- CM cross-cutting lens: The roadmap's new CM entry is supported by this diff because it adds both src/v2/CM.md and src/v2/CM-inventory.md as the referenced design artifacts.
This diff fixes most of the previously flagged CM design regressions, but it still leaves MM-1 pointed at hiding or carrying item classification instead of exposing the existing structural authority directly.
| - Serde attributes pass through three-layer fallback chain | ||
|
|
||
| **Not imported but should be:** | ||
| - Transport protocol contracts (TransportRequest/Response shapes in std/types.dag) |
There was a problem hiding this comment.
Invariant violation: The MM-1 acceptance text says emit should not access the existing item fields directly, which contradicts Boundary sufficiency's rule to enrich boundaries by surfacing the real authority rather than hiding it behind a new form.
|
|
||
| **Ontological fix:** Direct dissolution. Node decomposition into | ||
| Signal/Algebra/Reduction makes classification a pattern match: | ||
| `{connective: NoConnective, body: present, transport: absent}` → FnItem. |
There was a problem hiding this comment.
Invariant violation: Saying item classification should be computed once and carried structurally reintroduces a second item-identity authority instead of consuming body, transport, and connective directly, violating No duplicate representations and Root-Cause Depth.
Honest assessment: ~3 of ~39 fail-open sites are modeling gaps (all trace to bare_map_node, confirming M2 diagnosis). Defensive sites are structurally eliminable at the type-system level (phase-indexed Nodes, exhaustive match on finite product space) but not via data modeling. Runtime behavior choices are irreducible. Principle: every defensive guard is a place the type system can't prove what the programmer knows. Push the proof into structure. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 1, MODELING: 0+/0-, ROADMAP: 1✓/0!)
INVARIANTS — Violations (1)
ROADMAP — Verified
- CM analysis docs: The new ROADMAP CM section links to
src/v2/CM.mdandsrc/v2/CM-inventory.md, and both documents are added in this diff.
The diff resolves most of the previously flagged duplicate-authority design proposals, but it still overreaches by introducing a new parallel foundational ontology in CM.md.
|
|
||
| ## Foundational ontology | ||
|
|
||
| The concept DAG (computation.dag §LAYERS) has two foundational |
There was a problem hiding this comment.
Invariant violation: The new Signal/Algebra/Reduction “foundational ontology” introduces a parallel foundation for the compiler instead of grounding the analysis in the repo’s existing truth-valued foundation, violating Modeling Faithfulness and Root-Cause Depth.
|
ChatGPT review in progress... (view conversation) |
CM.md: - Resolve MM-1 contradiction: "emit cannot access raw fields" vs "fields ARE the authority." One clear end-state: existing structural data flows through boundaries intact. Reading a field IS reading the authority, not re-derivation. - Add governing constraint: no new classification types, no lossy boundaries, one clear end-state per MM. - Add §Arity boundaries: Node field combinatorics (empty service, classifier disagreement, intent erasure) + container arity edges (silent under/over-parameterization, optional collapse, callable mismatch). - Clarify S/E/M as analysis vocabulary, not stored metadata. - MM-2: TypeSummary.repr only safe if proven non-lossy for ALL consumers, not just emit. ROADMAP.md: - Soften child_inferred_or_empty checkbox (partial, not complete). - One clear end-state in CM acceptance table. - TypeSummary.repr non-lossy constraint explicit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
CM section trimmed to: per-lane diagnosis table, unowned files gap, arity boundaries pointer. MM-1/2/3 detail points to CM.md as definitive source. Lane diagnosis shows highest-leverage fix per lane. Note: CM.md will fold into MODELING.md when stable. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
Summary
is_fully_resolved, thread expected types to formal params and fold init, addCallableOftoAlgebraTypeTemplate, make clone semantics data-driven viaCloneTemplates, consolidate transport identity via kind constants, eliminatebare_map_nodefrom method registryTypedItemKind,ServiceFieldSet,FunctionSignature, andResourceDefinitiononEmitGraphInfo— eliminates ad-hoc structural interrogation across all three emit backendsOwnershipProoflist from compile through emit, eliminating duplicateanalyze_ownershipcalls in the Rust emitterRc::try_unwrap, fix O(n²) block inferenceVerification
Test plan
cargo test -p v2-compiler-tests— 289 passcargo test -p v2-compiler-tests strict_compile_diagnostic_count -- --ignored— 0 diagnosticsscripts/l1-ratchet.sh --check— L1 30 ≤ 30cargo clippy --all-targets -- -D warnings— clean🤖 Generated with Claude Code