Skip to content

spec: update constraint ID system#570

Merged
erik-3milabs merged 30 commits into
spec/mainfrom
spec/constraint_id
Jul 23, 2026
Merged

spec: update constraint ID system#570
erik-3milabs merged 30 commits into
spec/mainfrom
spec/constraint_id

Conversation

@erik-3milabs

Copy link
Copy Markdown
Collaborator

This PR introduces content-derived constraint IDs, i.e., the ID of a constraint is derived from its content, rather than its index in the constraint list. This

  1. permit addition, removing and reorganizing constraints without changing any IDs, and
  2. enforces ID updates upon constraint modification

i.e., two constraints with the same ID are 100-ε% certain to be identical.

Note: the ID is derived in a way that updating a variable name does not lead to a name update. This is at the expense of an ID update when variables are reordered.

Before:
image

After:
image

@erik-3milabs erik-3milabs self-assigned this Apr 28, 2026
@erik-3milabs erik-3milabs added the spec Updates and improvements to the spec document label Apr 28, 2026
Comment thread spec/src.typ Outdated
Comment thread spec/chip.typ Outdated
Comment thread spec/src.typ Outdated
@claude

claude Bot commented Apr 28, 2026

Copy link
Copy Markdown
Contributor

Review

This PR adds stable, content-derived constraint IDs to the spec rendering pipeline: an FNV-1a hash of each constraint's structure is encoded into a short 4-char human-readable tag (e.g. BRH-1J3K). It also restructures the constraint table (separate tag/counter columns, per-constraint dividers) and adds short code fields to all chip TOML files.

The approach is sound. Three bugs found:


Medium — Crash on empty variable groups (src.typ:191–197)

.sum() without a default: on an empty array returns none in Typst, not an empty dict. If any chip has a variable group with zero entries, the inner .sum() returns none, causing a crash on the outer .sum(). Fix: add default: (:) to both calls.

Low — z-fill panics for ≥100 constraints in one group (chip.typ:310)

"0" * (2 - s.len()) produces a negative repeat count when the counter hits 100+. Typst panics on negative string repeat. Fix: calc.max(0, 2 - s.len()).

Low — Hash input ambiguity from unescaped separator (src.typ:182)

The chip name, group, and type are joined with , without escaping. A comma in any field makes two distinct inputs hash identically, potentially causing ID collisions. Current names are safe, but it's a latent fragility. Fix: use a separator that cannot appear in those values (e.g. \x00).

@github-actions

Copy link
Copy Markdown

Codex Code Review

No actionable issues found in the PR diff.

The changes are limited to the Typst spec rendering/tagging logic and chip metadata. I did not find security-relevant runtime changes, VM behavior changes, or significant performance concerns.

I could not run a full Typst/shiroa render because neither typst nor shiroa is installed in this environment.

@erik-3milabs
erik-3milabs marked this pull request as draft April 28, 2026 13:28
@erik-3milabs
erik-3milabs marked this pull request as ready for review April 28, 2026 13:37
@github-actions

Copy link
Copy Markdown

Codex Code Review

Findings:

  • Low - spec/tooling/chip.py:990 ignores the new TOML code field. It sets self.code = data.get("name", ""), so the validator checks name twice and never validates code. Invalid code values would pass Python tooling, and any tooling using Chip.code will see the full chip name instead of the short code added in this PR. Use data.get("code", self.name) and report self.code in the identifier error.

No security issues found in the PR diff. I did not run a Typst render because typst is not installed in this environment.

Comment thread spec/tooling/chip.py Outdated
Comment thread spec/tooling/chip.py Outdated
Comment thread spec/src.typ Outdated
@claude

claude Bot commented Apr 28, 2026

Copy link
Copy Markdown
Contributor

Review: spec/constraint_id

Overview

Replaces sequential constraint indices with content-derived IDs using FNV-1a hashing. Variables are identified by (chip, group, position, type) so renaming a variable doesn't change constraint IDs, but reordering does. The 4-character Base32 ID (20 bits of entropy) is stable across reorganisation and signals stale IDs when a constraint's semantics change. TOML chips all gain a code shorthand field.

Bugs

[High] chip.py reads wrong key for codeself.code = data.get("name", "") always resolves to the chip name; the new code field (e.g. "BRH") is never read or validated. See inline comment on line 990.

[Medium] tag excluded from constraint digestdigestable_constraint omits the tag field for interaction and template constraints. Two constraints calling different tables with identical inputs/outputs/iters receive the same ID, breaking the core invariant. See inline comment on lines 210-213.

[Low] Wrong variable in error messagereporter.asserts(self.code.isidentifier(), f"Invalid identifier: {self.name!r}") prints the name, not the code. See inline comment on line 994.

FNV-1a implementation

The two-limb 64-bit multiplication is mathematically correct: carry propagation from lo into hi is handled, and the hash[1] * prime[1] term (≥ 2⁶⁴) is correctly discarded. No issues here.

Minor observations

  • CONSTRAINT_ID_CHAR_COUNT = 4 gives 20 bits of ID space. With ~50 constraints per chip the birthday-collision probability is negligible (~0.1%), so this is fine for the current scale.
  • bytes-to-hex's z-fill silently produces wrong output for strings longer than 2 chars, but since all inputs are single bytes (0–255) this can't trigger in practice.

@RobinJadoul RobinJadoul left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Very brief look only, so far; haven't properly looked at the canonicalization/serialization of the constraints yet

Comment thread spec/src.typ Outdated
Comment thread spec/chip.typ Outdated
Comment thread spec/chip.typ Outdated
Comment thread spec/src.typ Outdated
Comment thread spec/chip.typ Outdated
Comment thread spec/chip.typ Outdated
Comment thread spec/src.typ Outdated
Comment thread spec/src.typ Outdated
Comment thread spec/src.typ Outdated
Comment thread spec/src.typ Outdated
Comment thread spec/src.typ Outdated
Comment thread spec/src.typ Outdated
Comment thread spec/src.typ Outdated
Comment thread spec/src.typ Outdated
Base automatically changed from spec/main to main June 8, 2026 20:38
@RobinJadoul
RobinJadoul changed the base branch from main to spec/main June 9, 2026 12:33
@yetanotherco yetanotherco deleted a comment from github-actions Bot Jun 25, 2026
@erik-3milabs
erik-3milabs requested a review from RobinJadoul June 26, 2026 07:57
Comment thread spec/src.typ
Comment thread spec/src.typ Outdated
Comment thread spec/src.typ Outdated
RobinJadoul and others added 2 commits June 29, 2026 15:15
Co-authored-by: Erik <159244975+erik-3milabs@users.noreply.github.com>
@erik-3milabs

Copy link
Copy Markdown
Collaborator Author

/ai-review

@github-actions

Copy link
Copy Markdown

AI Review

PR #570 · 23 changed files

Findings

No non-rejected structured findings were reported.

Reviewer Lanes

Lane Model Prompt Status Findings

Native Codex and Claude reviews run separately and post their own comments. They are not included in this structured provenance report.

Raw lane outputs, candidates, final issues, and model metrics are uploaded as workflow artifacts.

@github-actions

Copy link
Copy Markdown

Codex Code Review

  • Highspec/src.typ:143: lo + hi.bit-lshift(32) reconstructs an unsigned 64-bit FNV value in Typst’s signed 64-bit integer. Whenever hi has its top bit set, this overflows and aborts spec generation. Serialize the two 32-bit limbs separately instead.

  • Highspec/src.typ:161: val.to-bytes() uses unsigned encoding by default, but many constraints contain negative integers (multiplicity = -1, negative constants/exponents). Loading those chips will fail while assigning IDs. Encode integers with signed: true or define an explicit sign-aware representation.

Comment thread spec/src.typ Outdated
Comment thread spec/tooling/chip.py Outdated
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Review summary

Scope: this PR is entirely spec/Typst rendering + Python tooling — no Rust/VM/executor/GPU code is touched, so the safety criteria around unsafe blocks, device memory, and instruction semantics don't apply here. The content-derived constraint ID design is sound and well thought out. A few notes:

Correctness / design (all low severity)

  • The digest correctly excludes only display/documentation fields (desc, ref, constraint) — verified that every constraint carrying a constraint string also carries the semantic poly/input/multiplicity fields that are hashed, so editing the math still changes the ID. Good.
  • FNV-1a: the high 32-bit limb never influences the final ID (only the low 3 bytes of lo are consumed by digest_to_id, and lo's recurrence doesn't read hi). Not a bug, but the high-limb work is effectively dead and hi.bit-lshift(32) relies on i64 shift-into-sign-bit wraparound. See inline comment — worth confirming the collision claim with the true 20-bit effective entropy in mind.
  • code uniqueness is not enforced anywhere; constraint tags are code + "-" + id, so two chips sharing a code would produce ambiguous cross-references. The hand-assigned codes are currently unique — fine for now, but a duplicate would go unnoticed.
  • Minor if self.code: guard in chip.py (inline).

Nits already fine

  • .sum(default: 0) / .sum(default: (:)) additions correctly guard empty iterators.
  • The table now has 5 columns (added leading super-index column); header, tag tuple (2 cells), and the extra polynomial/description rows all consistently emit 5 cells.

Nothing blocking.

@erik-3milabs

Copy link
Copy Markdown
Collaborator Author

Codex Code Review

  • Highspec/src.typ:143: lo + hi.bit-lshift(32) reconstructs an unsigned 64-bit FNV value in Typst’s signed 64-bit integer. Whenever hi has its top bit set, this overflows and aborts spec generation. Serialize the two 32-bit limbs separately instead.

This is not correct; the last operation in the loop truncates hi to its lower 32 bits. Applying bit-lshift(32) to such a number does not result in an error, even when the 31st bit is set.

  • Highspec/src.typ:161: val.to-bytes() uses unsigned encoding by default, but many constraints contain negative integers (multiplicity = -1, negative constants/exponents). Loading those chips will fail while assigning IDs. Encode integers with signed: true or define an explicit sign-aware representation.

This is incorrect; -1.to-bytes() simply produces (255,255,255,255,255,255,255,255).

@erik-3milabs

Copy link
Copy Markdown
Collaborator Author

Review summary

...

  • FNV-1a: the high 32-bit limb never influences the final ID (only the low 3 bytes of lo are consumed by digest_to_id, and lo's recurrence doesn't read hi). Not a bug, but the high-limb work is effectively dead and hi.bit-lshift(32) relies on i64 shift-into-sign-bit wraparound. See inline comment — worth confirming the collision claim with the true 20-bit effective entropy in mind.

This is an interesting observation. I don't think this is a problem, but it does go to show that FNV-1a is more finicky than I thought it would be...

@erik-3milabs
erik-3milabs requested a review from RobinJadoul July 20, 2026 09:41

@RobinJadoul RobinJadoul left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@erik-3milabs
erik-3milabs requested a review from RobinJadoul July 20, 2026 14:54
Comment thread spec/src.typ
@erik-3milabs

Copy link
Copy Markdown
Collaborator Author

@claude

@erik-3milabs
erik-3milabs requested a review from RobinJadoul July 23, 2026 07:31
@erik-3milabs
erik-3milabs merged commit 4a11583 into spec/main Jul 23, 2026
2 checks passed
@erik-3milabs
erik-3milabs deleted the spec/constraint_id branch July 23, 2026 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

spec Updates and improvements to the spec document

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants