Skip to content

sdk(knowledge): reject rule IDs that already head a section on any catalog page - #142

Merged
tzhouam merged 2 commits into
mainfrom
fix/knowledge-rule-id-tree-unique
Sep 10, 2026
Merged

tzhouam merged 2 commits into
mainfrom
fix/knowledge-rule-id-tree-unique

Conversation

@tzhouam

@tzhouam tzhouam commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Why

Rule IDs are auditable tree-wide: Direct routing resolves a rule by exact ID, and since #136 widened the catalog an owner's topic pages share the owner's ID prefix. KnowledgeCurator only checked the target page, so the first auto batch on the widened catalog (#140) reused DIFF-2y (already in rules-checkpoint-loading.md) and DIFF-4w (rules-worker-observability.md), and the parked 09-08 batch reused four more (OMNI-CI-3e, OMNI-CI-1e, DIFF-1o, DIFF-2ab). Exact-ID searches and routes then return two unrelated rules.

What

  • validate_proposals scans every catalog page (all repositories and the general pages, headings at any level, since owner pages nest some rules under ###) once, and rejects a proposal whose ID already heads a section on another page (rule_id already exists on <page>) or that reuses an ID accepted earlier in the same batch on a different page. The same-page reason is unchanged.
  • apply re-runs the tree-wide check under the apply lock before any write, so two batches validated against the same tree cannot both land one new ID on sibling pages; a same-page change stays the hash check's job.
  • The prompt contract states the tree-wide rule so the model picks a fresh ID instead of burning a repair round.

No API shape change; the knowledge API version stays 1.1.0.

Tests

test_sdk_knowledge_v1.py: rejection on another page of the same owner, on another repository via a general page, on a ### heading; batch-internal cross-page duplicate; apply-time recheck under the lock (second batch refused, target page untouched). Full knowledge/SDK selection green locally.

🤖 Generated with Claude Code

https://claude.ai/code/session_018MAFmwP3u5qAMDgHfM6LFi

…talog page

Rule IDs are auditable tree-wide: Direct routing resolves a rule by exact
ID, and since #136 widened the catalog an owner's topic pages share the
owner's ID prefix. The curator only checked the target page, so the first
auto batch on the widened catalog (#140) reused `DIFF-2y` (already in
rules-checkpoint-loading.md) and `DIFF-4w` (rules-worker-observability.md),
and the parked 09-08 batch reused four more. Searches and exact-ID routes
then return two unrelated rules for one ID.

validate_proposals now scans every catalog page in scope once, rejects a
proposal whose ID heads a section on another page ("rule_id already exists
on <page>"), and rejects a second proposal in the same batch that reuses an
earlier accepted ID on a different page. The same-page rejection reason is
unchanged. The prompt contract states the tree-wide rule so the model picks
a fresh ID instead of burning a repair round.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018MAFmwP3u5qAMDgHfM6LFi
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 10, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-10T04:09:42.087020Z e2e3f04 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e2e3f044e2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +485 to +486
if match.group("rule") not in ids:
ids.append(match.group("rule"))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reject repeated IDs within one proposed section

When a proposal contains both its declared ## X-2 — ... heading and a nested ### X-2 — ... heading, this deduplication collapses them to one ID before both the validation-time proposed_ids check and the apply-time seen_ids check. If X-2 was initially unused, the proposal is accepted and applied with two exact-ID headings, defeating the new tree-wide uniqueness guarantee; repeated occurrences within the section need to be rejected rather than removed here.

Useful? React with 👍 / 👎.

…heck

The SPEC freshness gate flagged sdk.md as stale after the curator change.
State the new contract: a rule ID is unique across every catalog page and
heading level (nested headings inside a section included), two proposals
in one batch cannot share one, and apply re-checks it under the lock.
Marker bumped to 2026-09-10.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018MAFmwP3u5qAMDgHfM6LFi
@tzhouam
tzhouam merged commit c0ca8ed into main Sep 10, 2026
2 checks passed
tzhouam added a commit that referenced this pull request Sep 10, 2026
The batch was distilled on the SDK pinned before #142 and against a main
that predates #140/#145, so it reused two IDs those batches landed and its
audit ran against the retired baseline. Merge main in and curate:

- VOMNI-CFG-1s (CLI-only negative alias) -> VOMNI-CFG-1t and
  MCPMO-2e (audio_embeds registration) -> MCPMO-2f: both IDs already head
  other rules on main; every ID now heads exactly one section tree-wide.
- The configuration entry page crossed the 32 KiB split cap once main's
  VOMNI-CFG-1s and this batch's 1t both sat on it. Both engine-args rules
  move to the existing topic page rules-legacy-engine-args.md (the owner's
  engine-args topic), the entry page's Direct table links there, and the
  nearest _index.md row is extended. No inbound anchors referenced them.
- Every cited PR added to its page's `sources:` frontmatter; Direct table
  rows for OMNI-CI-2g/2h, DIFF-1af, MCPMO-2f, VOMNI-CFG-1t and _index.md
  rows for benchmark, ci, configuration and diffusion.

The review gate also noted that the SDK deduplicates heading IDs within a
single section, so a section repeating its own ID in a nested heading is
not rejected; that is an SDK change and ships separately.

Validators: check_knowledge_tree 0 errors, check_wiki_lint 0 errors,
test_thin_mcp_server and test_knowledge_source green.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018MAFmwP3u5qAMDgHfM6LFi
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