feat(store): tag controlled-vocabulary alias map + canonicalization backfill (#653) - #680
Open
norrietaylor wants to merge 1 commit into
Open
feat(store): tag controlled-vocabulary alias map + canonicalization backfill (#653)#680norrietaylor wants to merge 1 commit into
norrietaylor wants to merge 1 commit into
Conversation
…ackfill (#653) Implements the core of epic #653 ontology #3 — a controlled-vocabulary alias map that de-fragments tags so co-occurrence edges and entity promotion key off a single canonical form. - feeds/tags.py: pure canonicalize_tag / canonicalize_tags (alias substitution then optional namespace normalization, order-preserving dedupe). Wraps — does not modify — the existing normalize_tag so entity-node keys stay stable. - config.py: TagsConfig.aliases (alias -> canonical map). Parser validates tag grammar, flattens chains (a->b->c => a->c), and rejects cycles at load time. - store: canonicalize_existing_tags() backfill — rewrites stored tags through the alias map, idempotently (only changed rows written), no re-embedding, updated_at untouched so recency signals are preserved. Exposed as distillery_relations action="canonicalize_tags". - promote_entities() is now alias-aware (resolves aliases before the separator-collapse), so aliased variants promote to one node. - distillery.yaml.example: documented aliases sample. Operational order (Phase 4): run canonicalize_tags before promote_entities so entity nodes key off canonical tags. Deferred to a follow-up: write-path choke-point (canonicalize NEW writes) and known_namespaces / namespace_enforcement gating — not required for the backfill. Tests: pure canonicalization, config flatten/cycle/validation, idempotent backfill + within-entry dedupe, alias-aware promotion, MCP action. Full suite 3229 passed; mypy --strict and ruff clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughTag aliases can now be declared in config, canonicalized through new tag helpers, applied during entity promotion and tag backfill, and exposed through a new ChangesTag alias canonicalization
Sequence Diagram(s)sequenceDiagram
participant distillery_relations
participant DuckDBStore
participant distillery.feeds.tags
distillery_relations->>DuckDBStore: canonicalize_existing_tags(aliases, reserved_prefixes, normalize_namespaces)
DuckDBStore->>distillery.feeds.tags: canonicalize_tags(active tags, aliases, reserved_prefixes, normalize_namespaces)
distillery.feeds.tags-->>DuckDBStore: canonical tag lists
DuckDBStore-->>distillery_relations: entries_scanned, entries_rewritten, tags_collapsed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/distillery/config.py`:
- Around line 910-932: The alias flattening in _flatten_alias_map only resolves
exact keys, so it can miss second hops that appear after namespace normalization
in canonicalize_tag. Update the config loading/canonicalization flow so aliases
are flattened or validated against normalized names once reserved_prefixes are
known, or make canonicalize_tag iterate to a fixed point with cycle protection.
Ensure the fix covers the alias parsing path around _flatten_alias_map and the
config handling that builds aliases before reserved_prefixes.
In `@src/distillery/mcp/server.py`:
- Around line 1376-1378: The action list already includes canonicalize_tags, but
the RETURNS documentation in the MCP server handler still omits its success
payload. Update the response contract in server.py near the main action/returns
docs to add canonicalize_tags alongside the existing promote_entities entry, and
document the entries_scanned, entries_rewritten, and tags_collapsed fields so
clients can rely on the full success shape.
In `@src/distillery/store/duckdb.py`:
- Around line 4708-4713: The canonical tag lookup in the entity promotion flow
is missing pre-existing aliased entity nodes, which can lead to duplicate
canonical nodes. Update the promotion logic around canonicalize_tag so existing
entity-node metadata.source_tag values are normalized with the same
alias/namespace rules before the exact source_tag = canonical lookup, and reuse
or migrate any matching node before creating a new one. Use the entity-node
creation/promotion path and the canonicalize_tag callsite as the main anchors
when adjusting the lookup and insertion behavior.
- Around line 4907-4920: The bulk tag rewrite in the entries update path commits
successfully but does not verify that the rewritten tags can be read back, so
add a bounded post-commit read-back in the same flow (or extend the existing
integrity check) to materialize the touched entries.tags values for the updated
row set before calling the checkpoint helper. Use the update loop that builds
updates and the surrounding transaction block to locate the fix, and ensure any
read-back failure raises before the operation reports success.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 67a2af73-7317-4edd-b3ec-325caa102a61
📒 Files selected for processing (11)
distillery.yaml.examplesrc/distillery/config.pysrc/distillery/feeds/tags.pysrc/distillery/mcp/server.pysrc/distillery/mcp/tools/relations.pysrc/distillery/store/duckdb.pysrc/distillery/store/protocol.pytests/test_config.pytests/test_entity_promotion.pytests/test_mcp_tools/test_relations_canonicalize_tags.pytests/test_tag_canonicalization.py
Comment on lines
+910
to
+932
| def _flatten_alias_map(aliases: dict[str, str]) -> dict[str, str]: | ||
| """Resolve alias chains to a single hop and reject cycles. | ||
|
|
||
| ``{"a": "b", "b": "c"}`` flattens to ``{"a": "c", "b": "c"}`` so the runtime | ||
| lookup is O(1) and idempotent. A key mapping to itself is a permitted no-op. | ||
| A true cycle (``a -> b -> a``) raises ``ValueError``. | ||
| """ | ||
| flat: dict[str, str] = {} | ||
| for key in aliases: | ||
| seen = [key] | ||
| cur = key | ||
| while cur in aliases: | ||
| nxt = aliases[cur] | ||
| if nxt == cur: # self-map: terminal, no-op | ||
| break | ||
| if nxt in seen: | ||
| raise ValueError( | ||
| "tags.aliases contains a cycle: " + " -> ".join([*seen, nxt]) | ||
| ) | ||
| seen.append(nxt) | ||
| cur = nxt | ||
| flat[key] = cur | ||
| return flat |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Flattening misses alias hops that only appear after namespace normalization.
Line 921 only follows exact alias keys, and Line 988 parses aliases before reserved_prefixes are available. That leaves configs like project/sbx -> entity/cloudflare/workers plus entity/cloudflare-workers -> entity/cloudflare partially flattened: a single canonicalize_tag(..., normalize_namespaces=True) pass stops at entity/cloudflare-workers, while a second pass reaches entity/cloudflare. Because promotion/backfill call canonicalization once, this can persist or group on the intermediate tag instead of the true canonical one.
Please either flatten/reject alias graphs with normalization-induced second hops at load time, or make the runtime canonicalization reach a fixed point with cycle protection.
Also applies to: 988-1009
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/distillery/config.py` around lines 910 - 932, The alias flattening in
_flatten_alias_map only resolves exact keys, so it can miss second hops that
appear after namespace normalization in canonicalize_tag. Update the config
loading/canonicalization flow so aliases are flattened or validated against
normalized names once reserved_prefixes are known, or make canonicalize_tag
iterate to a fixed point with cycle protection. Ensure the fix covers the alias
parsing path around _flatten_alias_map and the config handling that builds
aliases before reserved_prefixes.
Comment on lines
+1376
to
+1378
| - action (str, required): Operation. Valid: [add, get, remove, traverse, metrics, | ||
| reconcile, list_candidates, resolve_candidate, suggest_links, promote_entities, | ||
| canonicalize_tags]. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the canonicalize_tags success payload too.
The action is listed as valid, but the RETURNS section still stops at promote_entities, so clients do not see the entries_scanned / entries_rewritten / tags_collapsed response contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/distillery/mcp/server.py` around lines 1376 - 1378, The action list
already includes canonicalize_tags, but the RETURNS documentation in the MCP
server handler still omits its success payload. Update the response contract in
server.py near the main action/returns docs to add canonicalize_tags alongside
the existing promote_entities entry, and document the entries_scanned,
entries_rewritten, and tags_collapsed fields so clients can rely on the full
success shape.
Comment on lines
+4708
to
+4713
| canonical = canonicalize_tag( | ||
| tag, | ||
| aliases=aliases, | ||
| reserved_prefixes=reserved_prefixes, | ||
| normalize_namespaces=True, | ||
| ) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Reuse pre-existing aliased entity nodes before creating canonical ones.
Now that member tags are canonicalized here, an entity node created before aliases with metadata.source_tag="entity/cloudflare-sandboxes" is missed by the later exact source_tag = canonical lookup, so promotion can create a duplicate canonical entity node. Canonicalize existing entity-node source_tag values with the same alias/namespace rules and reuse or migrate that node before insertion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/distillery/store/duckdb.py` around lines 4708 - 4713, The canonical tag
lookup in the entity promotion flow is missing pre-existing aliased entity
nodes, which can lead to duplicate canonical nodes. Update the promotion logic
around canonicalize_tag so existing entity-node metadata.source_tag values are
normalized with the same alias/namespace rules before the exact source_tag =
canonical lookup, and reuse or migrate any matching node before creating a new
one. Use the entity-node creation/promotion path and the canonicalize_tag
callsite as the main anchors when adjusting the lookup and insertion behavior.
Comment on lines
+4907
to
+4920
| if updates: | ||
| try: | ||
| conn.execute("BEGIN TRANSACTION") | ||
| for new_tags, entry_id in updates: | ||
| conn.execute( | ||
| "UPDATE entries SET tags = ? WHERE id = ?", | ||
| [new_tags, entry_id], | ||
| ) | ||
| conn.execute("COMMIT") | ||
| except Exception: | ||
| with contextlib.suppress(Exception): | ||
| conn.execute("ROLLBACK") | ||
| raise | ||
| self._checkpoint_after_write(conn) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add read-back verification after the bulk tag rewrite.
This backfill rewrites entries.tags across many rows, but after commit it only checkpoints. Add a bounded read-back that materializes the touched tags column (or extend the existing integrity verifier to include it) so tag-page corruption is caught before reporting success.
Suggested direction
- self._checkpoint_after_write(conn)
+ touched_ids = [entry_id for _, entry_id in updates]
+ self._checkpoint_after_write(conn)
+ conn.execute(
+ "SELECT id, tags FROM entries WHERE id = ANY(?)",
+ [touched_ids],
+ ).fetchall()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if updates: | |
| try: | |
| conn.execute("BEGIN TRANSACTION") | |
| for new_tags, entry_id in updates: | |
| conn.execute( | |
| "UPDATE entries SET tags = ? WHERE id = ?", | |
| [new_tags, entry_id], | |
| ) | |
| conn.execute("COMMIT") | |
| except Exception: | |
| with contextlib.suppress(Exception): | |
| conn.execute("ROLLBACK") | |
| raise | |
| self._checkpoint_after_write(conn) | |
| if updates: | |
| try: | |
| conn.execute("BEGIN TRANSACTION") | |
| for new_tags, entry_id in updates: | |
| conn.execute( | |
| "UPDATE entries SET tags = ? WHERE id = ?", | |
| [new_tags, entry_id], | |
| ) | |
| conn.execute("COMMIT") | |
| except Exception: | |
| with contextlib.suppress(Exception): | |
| conn.execute("ROLLBACK") | |
| raise | |
| touched_ids = [entry_id for _, entry_id in updates] | |
| self._checkpoint_after_write(conn) | |
| conn.execute( | |
| "SELECT id, tags FROM entries WHERE id = ANY(?)", | |
| [touched_ids], | |
| ).fetchall() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/distillery/store/duckdb.py` around lines 4907 - 4920, The bulk tag
rewrite in the entries update path commits successfully but does not verify that
the rewritten tags can be read back, so add a bounded post-commit read-back in
the same flow (or extend the existing integrity check) to materialize the
touched entries.tags values for the updated row set before calling the
checkpoint helper. Use the update loop that builds updates and the surrounding
transaction block to locate the fix, and ensure any read-back failure raises
before the operation reports success.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Phase 1 of finishing epic #653 — the controlled-vocabulary / alias map (ontology #3). De-fragments tags (e.g.
domain/sandboxvsdomain/build/sandboxing) so co-occurrence edges and entity promotion key off a single canonical form. Inert on upgrade (empty default alias map).Changes
feeds/tags.py— purecanonicalize_tag/canonicalize_tags: alias substitution → optional namespace normalization → order-preserving dedupe. Wraps, does not modify,normalize_tagso existing entity-node keys stay stable. Whole-tag match only (no substring matching); idempotent given a flattened map.config.py—TagsConfig.aliases(alias -> canonical). The parser validates tag grammar on keys/values, flattens chains (a→b→c ⇒ a→c), and rejects cycles at load time.store—canonicalize_existing_tags()backfill: rewrites stored tags through the alias map, idempotently (only changed rows written), with no re-embedding (tags don't feed embeddings) andupdated_atuntouched (preserves recency signals). Exposed asdistillery_relations action="canonicalize_tags".promote_entities()is now alias-aware — resolves aliases before the separator-collapse, so an aliased variant (entity/cloudflare-sandboxes) promotes to the same node as its canonical form (entity/cloudflare).distillery.yaml.example— documentedaliasessample.Operational order (for Phase 4)
Run
canonicalize_tagsbeforepromote_entitiesso entity nodes key off canonical tags. (No production data is touched by this PR — the rollout is gated to Phase 4.)Deferred to a follow-up PR (tracked)
known_namespaces+namespace_enforcement(off/warn/strict) gating of unknown namespaces.Neither is required for the rollout — the backfill cleans existing data, and the feed poller already applies
normalize_tagwhenenforce_namespacesis on. Splitting keeps this PR focused (matches the design's recommended PR-1/PR-2 split).Testing
ValueError, invalid key/value).mypy --strict src/clean (73 files);ruff check src/ tests/clean.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes