Skip to content

Design brand-identity side-channel for A3/PD-3 relation refinement: preserve brand through resolution without ident_span carrier conflict - #4579

Merged
briansrls merged 7 commits into
mainfrom
session/wise-ant-182
Jun 9, 2026
Merged

briansrls merged 7 commits into
mainfrom
session/wise-ant-182

Conversation

@briansrls

@briansrls briansrls commented Jun 9, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Model-first design report for A3/PD-3 brand-identity preservation through type resolution (docs-only).

Problem: today's TRANSITION grafts reference-site ident_span onto resolved structural nodes (with_authored_identity), overloading span with declaration identity.

Amended design (re-ratified @ head):

Authority Carrier
Source location ident_span (honest; no brand graft)
Spelling lookup Node.ident + spelling-keyed TypeEnv.bindings (lookup_type_for unchanged)
Declaration identity BindingId on Node.binding_id, registered via TypeDeclBinding in TypeEnv.decl_registry
Spelling → identity bridge decl_id_by_spelling (phase-1-limited; see below)
Id allocation graph-global BindingIdAllocator on compilation graph (survives merge_envs)

TypeBinding is unchanged (still used for infer locals/params/lets — no binding_id on it).

Rejected v1 paths (review-driven): InternTable / Node.ident as declaration identity; per-module id counters; mandatory binding_id on TypeBinding.

Phase-1 scope (explicit): single-module PD-3 dogfood only. Does not claim cross-module same-spelling identity soundness yet — bindings remains spelling-keyed; phase-2 escalates map-key migration.

Implementation atomicity: deleting with_authored_identity ident_span graft must land in the same implementation PR as with_preserved_binding_id + Node.binding_id stamping — no window where neither channel carries identity.

This PR is docs-only — no implementation. Follow-on implementation PR(s) touch 00_core, 04_env, 04_infer, 04_resolve, 04_types, compile.dag (+ stage0 regen).

Deliverable: docs/planning/brand-identity-side-channel-a3-pd3-design-2026-06-09.md

Work item: node://adhoc-8274731a-727

Test plan

  • Docs-only change; no code or tests modified
  • Design doc references .dag sites (04_resolve, 04_infer, 04_types, 04_env, 00_core)
  • Phase-1 limits, phase-2 deferral, and graft atomicity explicit in doc + this body
  • No cargo test required for this lane

@briansrls
briansrls marked this pull request as ready for review June 9, 2026 01:22
Brian Searls and others added 2 commits June 9, 2026 01:42
Codex review correctly flagged that InternTable intern ids are spelling
lookup keys, not declaration identity. Revise the design to assign
TypeBinding.binding_id at build_type_env, carry on Node.ident after
lookup/peel, and bound phase-1 PD-3 to single-module dogfood.

Co-authored-by: Cursor <cursoragent@cursor.com>
@briansrls

Copy link
Copy Markdown
Contributor Author

Response to codex REQUEST_CHANGES (spelling id ≠ declaration identity)

Finding accepted. Verified against current code:

  • InternTable is String → Int by spelling (00_core.dag:1463–1476).
  • build_type_env keys local_bindings via intern(table, authored_name_at(item)).id (04_infer.dag:5193) — same spelling in different modules shares one lookup key.
  • Stamping ident: Some(intern(type_name).id) at parse (v1 §5.1) would therefore be a spelling id, not declaration identity — PD-3 would compare names while claiming otherwise (P1/P2 violation).

Fix (commit eb6b724): Amended the design doc to:

  1. Introduce TypeBinding.binding_id — monotonic id assigned per declaration at build_type_env registration (independent of spelling).
  2. Carry on Node.ident after env lookup / alias peel — not at parse via intern(type_name).
  3. Keep InternTable as lookup key only; separate from compare authority.
  4. Bound phase-1 PD-3 to single-module dogfood (matches current test corpus); phase-2 escalates cross-module same-spelling disambiguation (bindings map key migration).

The span/identity split direction is unchanged; the authority is now modeled correctly.

— sent from wise-ant-182

Codex review on eb6b724: lookup_type_for uses node.ident as the spelling
key into bindings, so it cannot also carry BindingId. Amend design to add
Node.binding_id + TypeEnv.bindings_by_id while leaving ident lookup
semantics unchanged.

Co-authored-by: Cursor <cursoragent@cursor.com>
@briansrls

Copy link
Copy Markdown
Contributor Author

Response to codex REQUEST_CHANGES (ident vs binding_id key domain)

Finding accepted. Verified on eb6b724:

  • lookup_type_for (04_env.dag:62) does match node.ident { Some(id) => lookup_type(env, ident: id) … } where bindings is keyed by spelling intern id.
  • The amended design (§5.2) directed stamping node.ident = Some(binding.binding_id) while §5.1 kept bindings spelling-keyed — one field, two incompatible key domains. Later lookups on stamped nodes would miss.

Fix (new commit):

  1. Add explicit Node.binding_id: BindingId? — declaration-identity side-channel only.
  2. Node.ident unchanged — spelling intern id for lookup_type_for; never overwritten with BindingId.
  3. Add TypeEnv.bindings_by_id secondary index for brand_name_at / identity resolution.
  4. with_preserved_binding_id grafts binding_id, preserves structural.ident for lookup.
  5. PD-3 compare uses formal.binding_id / actual.binding_id, not ident.

— sent from wise-ant-182

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review metadata

  • Provider / model: codex / unknown
  • Commit: eb6b724a · Trigger: schedule
  • Thinking: 203s wall

BLOCKING (2)

Root Cause

  • src/v2/04_env.dag TypeBinding is reused for both TypeEnv.bindings and inference locals → split type-declaration bindings from value/scope bindings, or attach BindingId only through a declaration-env wrapper and Node stamping.
  • src/v2/04_env.dag merge_envs already merges multiple module TypeEnv maps → require a graph-global or module-qualified BindingId namespace before introducing bindings_by_id.

⚠️ The design fixes the spelling-vs-identity confusion, but the proposed carrier and id namespace still make declaration identity ambiguous at implementation time.


### 3.2 BindingId — the actual declaration-identity authority

Introduce `BindingId` as a monotonic `Int` assigned when `build_type_env` registers each type

This comment was marked as resolved.

**Sites to update** (`04_infer.dag` `build_type_env`, ~5192+):

- Maintain `next_binding_id: Int` counter per `build_type_env` invocation (or per compilation
graph — pick one authority; per-graph is simpler for cross-module phase-2).

This comment was marked as resolved.

Brian Searls and others added 2 commits June 9, 2026 02:18
…ngId

Address inline/codex review on 99340b4: TypeBinding is shared with infer
locals/params so it cannot carry mandatory binding_id; per-module id
counters collide under merge_envs. Introduce TypeDeclBinding +
decl_registry with graph-global BindingIdAllocator; leave TypeBinding
unchanged for value/scope bindings.

Co-authored-by: Cursor <cursoragent@cursor.com>
@briansrls

Copy link
Copy Markdown
Contributor Author

Response to blocking inline comments + codex root-cause review (99340b4)

All findings accepted. Verified against current code:

1. TypeBinding conflation (line ~93)

TypeBinding (04_env.dag:20) is used for both TypeEnv.bindings (type declarations) and inference scope.locals (params at 04_infer.dag:996, let bindings at 3821+, lambda slots at 1039). Mandatory binding_id on TypeBinding would force fabricated declaration ids on value bindings.

Fix: Introduce TypeDeclBinding + TypeEnv.decl_registry for type declarations only. TypeBinding struct unchanged for locals/params.

2. Per-module next_binding_id collision (line ~185)

merge_envs (04_env.dag:128) merges bindings from kernel + import + local module envs. Per-build_type_env counters would assign duplicate ids across modules.

Fix: BindingIdAllocator on the compilation graph — single global namespace; merge_envs merges decl_registry safely because ids are unique at allocation.

3. Codex root-cause summary

Same two fixes above. Declaration identity now flows: alloc_binding_id → TypeDeclBinding in decl_registry → decl_id_by_spelling bridge → stamp Node.binding_id after lookup/peel. lookup_type_for and TypeBinding for scope carriers untouched.

Amendment 3 pushed on new commit.

— sent from wise-ant-182

@briansrls
briansrls marked this pull request as draft June 9, 2026 02:59
Re-ratification follow-up: explicit that decl_id_by_spelling does not
claim cross-module same-spelling identity soundness; require
with_authored_identity deletion atomically with binding_id replacement.

Co-authored-by: Cursor <cursoragent@cursor.com>
@briansrls
briansrls marked this pull request as ready for review June 9, 2026 03:06
@briansrls
briansrls merged commit 7fd193a into main Jun 9, 2026
18 checks passed
@briansrls
briansrls deleted the session/wise-ant-182 branch June 9, 2026 03:11

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review metadata

  • Provider / model: codex / unknown
  • Commit: 69dcdb85 · Trigger: schedule
  • Thinking: 252s wall

BLOCKING (1)

Root Cause

  • docs/planning/brand-identity-side-channel-a3-pd3-design-2026-06-09.md decl_id_by_spelling remains a merged spelling-key side map rather than the same authority as the resolved binding → stamp from the lookup result’s declared identity, make collisions fail closed, or defer merging this index until qualified/keyed lookup lands.

⚠️ The design fixes the prior carrier and allocator issues, but the merged spelling-key identity index is still unsound as written.

## 4. Authority split (post-design)

| Fact | Authority | Accessor |
|------|-----------|----------|

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BLOCKING: merge_envs cannot safely merge decl_id_by_spelling alongside decl_registry because decl_id_by_spelling is keyed by spelling Int, so global BindingId uniqueness does not prevent imported same-spelling overwrites and violates P2 Boundary Discipline.

@briansrls

briansrls commented Jun 9, 2026 •

Copy link
Copy Markdown
Contributor Author

Verified the late codex finding against merged head 58639da. The coherence fix made phase-1 limitations explicit, but it did not fully close the specific soundness issue: decl_id_by_spelling is still specified as a merged spelling-key side map. If two envs contribute the same spelling, the design currently says phase-1 does not claim cross-module soundness, but it does not require the side map to fail closed on collisions or defer the merged side index until qualified/keyed lookup lands. That leaves a path for stamping a binding_id from a side map that is not the same authority as the resolved binding. I am treating this finding as valid and routing a follow-up design amendment; #4581/#4585 implementation remains held until the design is corrected and re-gated. — sent from bright-owl-653

briansrls pushed a commit that referenced this pull request Jun 9, 2026
…tamp path (Amendment 4)

Late codex finding on #4579: decl_id_by_spelling is a spelling→BindingId map that collapses
same-spelled declarations from different modules to one key under merge_envs. Any path that
derives binding_id through a spelling key is a P1/P3 violation (fabricated plausible output,
not fail-closed). This amendment sweeps the class and hardens the invariant.

Changes:
- Remove TypeEnv.decl_id_by_spelling entirely from phase-1 design (deferred to phase 2)
- Fix stamp path to be authority-direct: build_type_env stamps binding_id onto the resolved
  Node at declaration-registration time; lookup_type_for returns that already-stamped node;
  no secondary spelling-key lookup needed
- Add §5.5 class-sweep table enumerating every binding_id feed path; confirms none is
  spelling-keyed
- Update data flow diagram, build_type_env steps, infer/resolve section, verdict, and
  alternatives table
- Add Amendment 4 header with 4-point contract

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
briansrls added a commit that referenced this pull request Jun 9, 2026
…tamp path (Amendment 4) (#4587)

* WIP: Follow-up #4579 design amendment: make decl_id_by_spelling fail-closed o

* docs: amend brand-identity design — remove decl_id_by_spelling from stamp path (Amendment 4)

Late codex finding on #4579: decl_id_by_spelling is a spelling→BindingId map that collapses
same-spelled declarations from different modules to one key under merge_envs. Any path that
derives binding_id through a spelling key is a P1/P3 violation (fabricated plausible output,
not fail-closed). This amendment sweeps the class and hardens the invariant.

Changes:
- Remove TypeEnv.decl_id_by_spelling entirely from phase-1 design (deferred to phase 2)
- Fix stamp path to be authority-direct: build_type_env stamps binding_id onto the resolved
  Node at declaration-registration time; lookup_type_for returns that already-stamped node;
  no secondary spelling-key lookup needed
- Add §5.5 class-sweep table enumerating every binding_id feed path; confirms none is
  spelling-keyed
- Update data flow diagram, build_type_env steps, infer/resolve section, verdict, and
  alternatives table
- Add Amendment 4 header with 4-point contract

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* docs: fix stale 'stamp after lookup' phrases — authority is at registration (cursor review)

Four locations still said binding_id is stamped at/after lookup, contradicting Amendment 4's
authority-at-registration rule. Fixes:
- §3.2 carrier note: 'after type-decl lookup/peel' → 'at declaration-registration time'
- §3.2 'After lookup stamps…' sentence clarified: infer/resolve reads/propagates, never originates
- §4 compatibility rule: 'post-lookup obligation' → 'build_type_env-registration obligation'
- §5.2 heading: 'stamp binding_id after lookup' → 'read and propagate binding_id'
- Migration step 3: 'stamp binding_id after lookup' → 'read resolved.binding_id (already stamped at step 2)'

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* docs: name topo_resolve_types as load-bearing binding_id preservation edge (briansrls review)

The design silently relied on preserve_nominal_brand_on_resolve being the mechanism that
carries binding_id through topo_resolve_types' TypeBinding rebuild (04_infer.dag:5714).
An implementer who didn't know this edge exists would silently drop the stamp.

Changes:
- §5.2: expand topo_resolve_types paragraph to name the full preservation chain (pre node →
  preserve_nominal_brand_on_resolve → with_preserved_binding_id → rebuilt TypeBinding.resolved)
  and add an explicit implementation obligation: with_authored_identity must be replaced
  before topo_resolve_types runs, or the stamp drops on the rebuilt node
- §5.5 class sweep: add topo_resolve_types/preserve_nominal_brand_on_resolve row with the
  SAFE + atomicity-dependency annotation; update lookup_type_for row to note post-topo-resolve
  source

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* docs: fix §5.4 stale 'infer stamps after env bind' — infer/resolve only propagates (parent)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Brian Searls <briansearls1@gmail.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant