Repository navigation
p2 - #596
p2#596
Conversation
|
claude-review (director) — ✅ ship it Clean dissolution. The approach is exactly right:
Per Caller checkVerified the only live consumer is Minor note (non-blocking)Two small things to consider for a followup if you're back in this area:
Ship criteria
One down on the P2 list — nice focused scope. |
SG-2 handwritten-parse snapshot ratchet keys off parse-tree fnv for each corpus path. The types.dag comment update in PR #596 changed the digest; regenerate parse_corpus_manifest.txt so CI matches. Made-with: Cursor
briansrls
left a comment
There was a problem hiding this comment.
codex · gpt-5.4 · a9b80101
✅ Review (blocking: 0, non-blocking: 1+/1-)
Non-blocking — Strengths
dsl/std/filesystem.dagMixed (.dag models + docs): collapsing FileClassification to type FileClassification = FileEntry removes the parallel carrier while preserving the probe-surface name, which is the right single-authority shape.
Non-blocking — Improvements (fix in-PR if easy, else defer to roadmap)
docs/lens-library-design.mdThe new "intentional aliases excluded per policy" claim makes alias handling load-bearing for lens_structural_duplicates, but section 2.1 still does not define how the lens distinguishes sanctioned aliases from true duplicates; document that rule in the lens-library implementation lane before the lens lands.
✅ The substantive model change is clean, the doc updates mostly track it accurately, and I found no blocking issues in the PR.
This comment has been minimized.
This comment has been minimized.
Codex review on #596: document how sanctioned type aliases are excluded from duplicate-shape hashing vs true twin carriers, align DuplicatesConfig + algorithm sketch + tests, and point §9 at the same rule (CODING.md feedback_naming_is_aliasing). Made-with: Cursor
briansrls
left a comment
There was a problem hiding this comment.
codex · gpt-5.4 · ab2996fe
BLOCKING (1)
Root Cause
docs/lens-library-design.mdThe policy only distinguishes transparent alias vs nominal declaration, but the missing split is alias-vs-canonical-name exclusion vs alias-vs-alias duplicate detection; choose one rule and align the examples, tests, and success criteria to it.
ROADMAP — Verified
- P2 FileClassification/FileEntry duplicate: This PR really does collapse the duplicate carrier to one FileEntry record plus a namespace alias, so removing that debt line from ROADMAP.md is accurate.
| declaration is a namespace into the substrate, not a new | ||
| materialized structure. Concretely: | ||
|
|
||
| 1. **Transparent alias (excluded by default)** — Declarations lowered |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
Codex blocking feedback: spell out alias-vs-canonical exclusion vs alias-vs-alias detection, wire channels + algorithm + tests + §5/§9 to the same framework so implementation guidance is not ambiguous. Made-with: Cursor
|
|
briansrls
left a comment
There was a problem hiding this comment.
codex · gpt-5.4 · 70d8f9bb
BLOCKING (1)
Root Cause
docs/lens-library-design.mdv3 lowers each where predicate to a fresh declaration and proves equivalence structurally, so a canonical_refinement_id here would either add a parallel authority or miss same-shape peers; define Rule 2 on structural predicate equivalence or fingerprint and align the algorithm plus test text to that same fact.
Non-blocking — Strengths
dsl/std/filesystem.dagCollapsing FileClassification to a FileEntry alias removes the duplicate substrate carrier while preserving the probe-surface API name for callers.
ROADMAP — Verified
- P2 FileClassification/FileEntry duplicate: Removing the debt bullet is accurate because this PR replaces the second record with a namespace alias rather than a parallel carrier.
| is set, apply **Rule 1**: omit or fold pure nominal `type A = B` | ||
| into `B`'s bucket (never emit alias-vs-canonical here). | ||
| 3. **Refinement-alias channel:** For each `type Name = Carrier where …`, | ||
| compute `hash(resolved_carrier_id, canonical_refinement_id)`. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
Addresses blocking inline re: '= Carrier where …' vs NonEmptyStr/ NonEmptyString: Rule 1 is alias-vs-canonical only; channel (2) + Rule 2 keeps peer refinement duplicates visible. Made-with: Cursor
Blocking review: channel (2) must not imply id-based refinement equality; align with v3 structural discharge and thesis-style no- duplicate-representations — structural_refinement_key from lowered DAG, algorithm + tests + DuplicateKind comment updated. Made-with: Cursor
… (§2.1) Codex blocking feedback (sha 70d8f9b): predicates lower to fresh decls; id-based canonical_refinement_id would fork authority or miss peers — spell single authority as structural_refinement_key, update algorithm + implementation note + lowering test control. Made-with: Cursor
briansrls
left a comment
There was a problem hiding this comment.
codex · gpt-5.4 · 99ee2ccb
✅ Review (blocking: 0, non-blocking: 2+/0-)
Non-blocking — Strengths
docs/lens-library-design.mdThe new Rule 1 / Rule 2 split giveslens_structural_duplicatesone coherent single-authority story: pure aliases fold into the canonical carrier, while peer refinement aliases are still surfaced.dsl/std/filesystem.dagCollapsingFileClassificationintoFileEntryplus a namespace alias removes the duplicate substrate carrier without losing the probe-surface API name.
ROADMAP — Verified
- P2 FileClassification/FileEntry duplicate: Removing the debt bullet is accurate because this PR leaves one record in
std/filesystem.dagand demotesFileClassificationto an alias instead of a second authority.
✅ The load-bearing ambiguities from the earlier review are addressed, and the filesystem cleanup plus roadmap update are consistent with the thesis and current v3 refinement behavior.
|
✅ Review (blocking: 0, non-blocking: 2+/0-) Non-blocking — Strengths
ROADMAP — Verified
✅ The load-bearing ambiguities from the earlier review are addressed, and the filesystem cleanup plus roadmap update are consistent with the thesis and current v3 refinement behavior. |
Blocking inline near DuplicateKind: clarify bucket hash uses structural_refinement_key only; DeclarationId is blame/span only. Made-with: Cursor
briansrls
left a comment
There was a problem hiding this comment.
codex · gpt-5.4 · 6f20d8df
✅ Review (blocking: 0, non-blocking: 0+/1-)
Non-blocking — Improvements (fix in-PR if easy, else defer to roadmap)
docs/lens-library-design.mdThe new §2.1 prose correctly demotesNonEmptyStr/NonEmptyStringto an audit-era example, but later changed sections still describe it as a current expected repo finding even thoughdsl/std/types.dagnow only carriesNonEmptyStr; tighten those examples before they become stale lens-test expectations.
✅ The duplicate-record substrate issue is fixed cleanly and the prior refinement-authority ambiguities are resolved; only the stale example inventory looks worth a follow-up.
|
✅ Review (blocking: 0, non-blocking: 0+/1-) Non-blocking — Improvements (fix in-PR if easy, else defer to roadmap)
✅ The duplicate-record substrate issue is fixed cleanly and the prior refinement-authority ambiguities are resolved; only the stale example inventory looks worth a follow-up. |
…ntory Codex: std/types.dag only has NonEmptyStr today — expected findings, §5, §9, and Rule 2 examples now distinguish live-repo targets vs synthetic Rule 2 fixtures so lens CI expectations cannot drift stale. Made-with: Cursor
briansrls
left a comment
There was a problem hiding this comment.
codex · gpt-5.4 · f0a835be
✅ Review (blocking: 0, non-blocking: 2+/0-)
Non-blocking — Strengths
docs/lens-library-design.mdThe updated Rule 1/Rule 2 language is consistent with live repo state: FileClassification/FileEntry is historical, and Rule 2 refinement examples now point to synthetic fixtures instead of pretending both std spellings still exist.dsl/std/filesystem.dagCollapsing FileClassification to a FileEntry alias removes the duplicate substrate carrier while preserving the probe-surface name that dsl/std/resources.dag still consumes.
ROADMAP — Verified
- P2 FileClassification/FileEntry duplicate: Removing that debt line is accurate because the PR leaves one carrier in std/filesystem.dag and demotes FileClassification to a namespace alias.
✅ The single-authority cleanup, lens-policy clarification, and roadmap/doc updates all line up with the thesis and the current structural refinement implementation.
|
✅ Review (blocking: 0, non-blocking: 2+/0-) Non-blocking — Strengths
ROADMAP — Verified
✅ The single-authority cleanup, lens-policy clarification, and roadmap/doc updates all line up with the thesis and the current structural refinement implementation. |
|
claude-review (director) — ✅ ship it 4/4 green. FileClassification/FileEntry dissolution via type alias. Docs + ROADMAP updated cleanly. Per |
…5966's concept_decl_facts(roots) host builtin merged (src/v2/std/concept_index.dag), the scaffold's own dissolution trigger fired. Ground decl_facts_project.rs's DeclFact projection logic onto .dag (05_emit_rust.dag delegation), co (#6158) * WIP: Dissolve decl_facts_project.rs (174 LOC) HAND_MAINTAINED seed file: #596 * WIP: Dissolve decl_facts_project.rs (174 LOC) HAND_MAINTAINED seed file: #596 * WIP: Dissolve decl_facts_project.rs (174 LOC) HAND_MAINTAINED seed file: #596 * WIP: Dissolve decl_facts_project.rs (174 LOC) HAND_MAINTAINED seed file: #596 * WIP: Dissolve decl_facts_project.rs (174 LOC) HAND_MAINTAINED seed file: #596 * Address review: marshal real decl nodes, de-fork ItemKind, restore body witness. eval_decl_facts now projects parse-only type/body nodes (not identity stubs), uses ItemKind as the single carrier name, keeps fn-like predicate authority in decl_index.dag only, and extends the floor witness to require observable body. Co-authored-by: Cursor <cursoragent@cursor.com> * WIP: Dissolve decl_facts_project.rs (174 LOC) HAND_MAINTAINED seed file: #596 * doc: state non-test corpus boundary on decl_facts_for_roots. Co-authored-by: Cursor <cursoragent@cursor.com> * WIP: Dissolve decl_facts_project.rs (174 LOC) HAND_MAINTAINED seed file: #596 * fix: marshal where-refined type exprs after #6159 merge. #6159 removed concept_decl_node_parse_only_from_source; decl_facts TypeItem marshaling now uses concept_decl_node. Unwrap `T where …` Conj nodes in type_expr_authored_name so coproduct fields like `Int where range(min: 2)` marshal in parse-only walks (e.g. GoMajorVersionSuffix). Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Brian Searls <briansearls1@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Opened from session-dashboard for session
zesty-pike-603.