Repository navigation
docs: amend brand-identity design — remove decl_id_by_spelling from stamp path (Amendment 4) - #4587
Conversation
…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>
…ration (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>
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
2a038c4c· Trigger:schedule - Thinking:
288s wall
BLOCKING (1)
Root Cause
src/v2/04_infer.dagresolve_env_bindings/topo_resolve_typesrewritesTypeEnv.bindingsafter local registration → add an explicit obligation to preservebinding_idthrough those resolver rebuilds, or stamp the final resolved binding fromTypeDeclBindingbefore lookup consumers run.
| | `peel_nominal_alias_identity` — alias peel | reads `binding_id` from the looked-up `TypeBinding.resolved` node (stamped at registration) | No | **SAFE** | | ||
| | `TypeBinding.resolved.binding_id` — returned by `lookup_type_for` | set at `build_type_env` registration, not at lookup | No | **SAFE** | | ||
| | `nominal_ref_node` — parse-time ref construction | `binding_id: none` — no value set | N/A | **SAFE (none)** | | ||
| | Inference scope locals / param `TypeBinding` | not touched (no `binding_id` on `TypeBinding` struct, no `decl_registry` entry) | N/A | **SAFE (absent)** | |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
… 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>
…ly propagates (parent) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
b29b29ba· Trigger:schedule - Thinking:
207s wall
✅ Amendment 4 removes spelling-key identity from the stamp path and I found no remaining blocking contradictions with the thesis or invariants.
|
This finding was addressed in commit cdd8817 (pushed before b29b29b). The current HEAD already names the
The inline comment appears to have been written against an earlier SHA and relayed after the fix landed. No code change needed. — sent from swift-moth-293 |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
2bb50875· Trigger:schedule - Thinking:
186s wall
Non-blocking — Strengths
docs/planning/brand-identity-side-channel-a3-pd3-design-2026-06-09.mdThe amendment removes spelling-key identity from the stamp path and keeps declaration identity single-authority and fail-closed.
✅ No blocking concerns found.
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
179310d0· Trigger:schedule - Thinking:
252s wall
✅ The docs-only amendment removes spelling-key identity from the stamp path and keeps BindingId authority registration-direct with no blocking concerns.
Summary
Design amendment to
docs/planning/brand-identity-side-channel-a3-pd3-design-2026-06-09.md(follow-up to #4579). Late codex finding:decl_id_by_spellingas designed in #4579 is a spelling→BindingIdmap that routes thebinding_idstamp through a spelling key — a P1/P3 violation.This PR is docs-only. No implementation files changed.
Work item:
node://adhoc-9d1bcff7-6974-point contract (acceptance criteria)
Authority-only
binding_idstamping.Node.binding_idis set only from a value produced byalloc_binding_id(direct) or copied from a node already stamped byalloc_binding_id(transitive graft).build_type_envstampsbinding_idonto the resolvedNodeat declaration-registration time — not after lookup, not from a spelling key. The stamp is carried on theNodevalue shared betweenTypeDeclBinding.resolvedandTypeBinding.resolved, so any subsequentlookup_type_forreturns a node that already carries the stamp.decl_id_by_spellingremoved from the soundness-critical stamp path. The field does not exist in phase-1TypeEnv. The stamp path in §5.2 is now authority-direct: afterlookup_type_forreturnsTypeBinding.resolved, the caller readsresolved.binding_id— no secondaryMap<Int, BindingId>lookup.Any future spelling→id index must be lookup-only, fail-closed on collision, and is deferred to phase 2. If a spelling→
BindingIdindex is eventually needed for cross-module lookup optimisation it must: (a) be constructed fail-closed (collision = ambiguous/absent, never silent winner-pick), (b) never be a stamp source, and (c) only be introduced with cross-module collision tests. Phase 1 ships without it.Class sweep: no spelling-keyed path feeds
binding_id. §5.5 enumerates every path that writesNode.binding_id. All are authority-direct (fromalloc_binding_iddirectly or transitive graft of an already-stamped node). The one spelling-keyed path (decl_id_by_spellinglookup → stamp) is explicitly listed as REMOVED.Changed claims
TypeEnvstruct (§3.2)decl_id_by_spelling: Map<Int, BindingId>presentbuild_type_envsteps (§5.1)decl_id_by_spelling[item_ident] = binding_id; step 4 stamps nodedecl_id_by_spellingentrydecl_id_by_spelling→decl_registry"binding_iddirectly from the resolvedNodereturned bylookup_type_for"decl_id_by_spelling → TypeDeclBindingnode in infer/resolve subgraphenv_bindsubgraph at registrationdecl_id_by_spellingis honest but phase-1-limited"decl_id_by_spellingis deferred entirely (Amendment 4)"decl_id_by_spellingas stamp pathTypeEnv.decl_id_by_spellingas new typebinding_idfeed pathTest plan
cargo testrequiredbinding_idfeed path; confirms none is spelling-keyeddecl_id_by_spellingreferences in the design doc updated🤖 Generated with Claude Code