Repository navigation
chore(scripts): relocate topic-parity scripts from omni_home [OMN-9286] - #1352
Merged
Merged
Conversation
omni_home/scripts/ is blocked by the no-functional-code pre-commit hook, which rejects any .py/.sh file in that directory. Two pre-existing scripts (check-topic-parity.py, sync-topic-registry.py — PRs #50/#51, 2026-03-13) violated this and were blocking unrelated docs-only PRs. Relocating to omnibase_infra/scripts/ per the OMN-4922 pattern (pull-all.sh). Changes: * Copy both scripts to omnibase_infra/scripts/ preserving exec bits * Replace module-level global state with OMNI_HOME env var + ModelTopicParityPaths * Add SPDX headers and satisfy mypy --strict + ruff (5 pre-existing PLW0603 + 7 missing-type-arg violations fixed in the move) * Add tests/scripts/test_topic_parity_scripts.py covering shebang, SPDX, argparse surface, and OMNI_HOME resolution Companion omni_home PR will delete the originals and repoint the CI workflow (.github/workflows/topic-parity.yml) at the new location.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds two CLI Python tools: one validates topic parity between the canonical Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (check-topic-parity)
participant Registry as Registry (topic_registry.yaml)
participant TopicsTS as TopicsTS (omnidash/shared/topics.ts)
participant Consumers as Consumers (omnidash/server/*.ts)
participant Comparator as Comparator
CLI->>Registry: load & validate YAML
CLI->>TopicsTS: parse exported topic constants
CLI->>Consumers: parse READ_MODEL_TOPICS & EXPECTED_TOPICS
CLI->>CLI: resolve constants & spread expressions
CLI->>Comparator: compare registry vs consumer sets
Comparator-->>CLI: return result (OK or TOPIC PARITY FAILURE)
sequenceDiagram
participant CLI as CLI (sync-topic-registry)
participant Registry as Registry (topic_registry.yaml)
participant Generator as Generator
participant TopicsTS as TopicsTS (omnidash/shared/topics.ts)
participant FS as FileSystem
CLI->>Registry: load & validate YAML
CLI->>Generator: derive constant names & generate block
CLI->>TopicsTS: locate BEGIN/END marker or insertion point
CLI->>FS: (--check) compare / (--write) replace-or-append / (--dry-run) print
FS-->>CLI: report result (OK or DRIFT)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/check-topic-parity.py`:
- Around line 179-184: The current code prints a warning and returns an empty
set when an expected array (e.g., READ_MODEL_TOPICS or EXPECTED_TOPICS) is not
found (the branch checking "if not match"), which lets CI pass; change this to a
hard failure by replacing the warning+return with an immediate non-zero exit or
exception (e.g., call sys.exit(1) or raise RuntimeError) so missing source
arrays cause the script to fail; reference the variables/locals in the branch
such as match, array_name, and file_path to locate and update the error
handling.
- Around line 254-310: The script only checks expected/read_model topics against
registry but not the reverse; add checks that any omniclaude topics in
registry_topics are actually referenced in EXPECTED_TOPICS and
READ_MODEL_TOPICS. After the existing parity checks (after computing
expected_not_in_registry and read_model_not_in_registry), compute
registry_not_in_expected = registry_topics - expected_topics and
registry_not_in_read_model = registry_topics - read_model_topics, filter each
set to omniclaude topics (e.g., ".omniclaude." and optional ".evt." if you want
to limit to events), and append descriptive error messages to errors for any
missing registry topics so the script returns non‑zero when registry entries are
not wired into EXPECTED_TOPICS/READ_MODEL_TOPICS. Ensure you use the same error
formatting style (errors.append and sorted iteration) as the surrounding checks.
- Around line 117-125: The YAML loader assumes the parsed `data` is a mapping
and will raise AttributeError for non-dict roots; after `data =
yaml.safe_load(f)` add a guard that checks `isinstance(data, dict)` (or
truthiness plus mapping type) and if it's not a dict print an error to stderr
(similar message as the missing topics case) and exit with code 2, then continue
to read `topics = data.get("topics", [])` and return the set comprehension
`{entry["topic"] for entry in topics}` as before.
In `@scripts/sync-topic-registry.py`:
- Around line 75-83: The code assumes yaml.safe_load(path) returns a mapping and
calls data.get("topics"), which will raise AttributeError for scalars, lists, or
None; update the logic after yaml.safe_load to validate that data is a
dict/mapping (i.e., isinstance(data, dict)) before accessing .get, and if it's
not a mapping print the same error (e.g., "ERROR: No topics found in {path}") to
stderr and sys.exit(2); reference the variables and call sites yaml.safe_load,
data, and topics to locate and guard the access.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7b24f62-2d51-417b-bffe-979048e9c3c0
📒 Files selected for processing (3)
scripts/check-topic-parity.pyscripts/sync-topic-registry.pytests/scripts/test_topic_parity_scripts.py
…cripts Four findings from the CodeRabbit review on PR #1352, all legitimate correctness improvements to pre-existing behavior that's now in-scope because we're already touching these files. - CR #1, #4: yaml.safe_load may return None or a scalar; guard with isinstance check and fail fast with type-of-value in the message. - CR #2 (MAJOR): missing top-level subscription arrays (READ_MODEL_TOPICS, EXPECTED_TOPICS) were a warning + silent pass. A rename or deletion of either array would silently succeed — exactly the breakage this gate exists to catch. Add required=True kwarg on top-level calls; recursive spread lookups still fall back to topics.ts with a warning. - CR #3 (MAJOR): the parity check only walked consumer -> registry. A newly-declared registry topic that was never wired into READ_MODEL_TOPICS or EXPECTED_TOPICS passed the gate. Add a reverse check that every registry omniclaude evt topic is covered by both consumer arrays. Tests: four new unit tests cover required-array failure, non-dict registry rejection (both scripts), and reverse-parity failure. All 10 tests pass.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/sync-topic-registry.py`:
- Around line 85-90: The load_registry function currently only checks topics for
emptiness; add validation that each item in topics is a dict and contains a
'topic' key plus at least one of 'event_type' or 'topic_base_constant' (and
verify keys are strings as needed). For any invalid entry, print a clear error
to stderr including the path and the offending entry, then sys.exit(2). Keep the
existing topics return when all entries pass validation and reference the topics
variable and load_registry function when locating the code to change.
- Around line 140-142: The code appends raw description into a single-line JSDoc
which can be broken by embedded "*/" or newlines; update the generation to
sanitize and emit a safe multi-line JSDoc: when description is truthy, first
escape any occurrences of "*/" (e.g. replace with "*\/"), split the description
on newlines, then push a "/**" line, push each escaped line prefixed with " * "
into the lines list, and push the closing " */" before appending the "export
const {const_name} = '{topic}';" line so const_name/topic/lines are used to
locate the change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f558d5a-8e85-4619-a9a8-3148b919a267
📒 Files selected for processing (3)
scripts/check-topic-parity.pyscripts/sync-topic-registry.pytests/scripts/test_topic_parity_scripts.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/scripts/test_topic_parity_scripts.py
- scripts/check-topic-parity.py
Two follow-up CodeRabbit findings on the first fix commit: - CR-minor: load_registry accepted any shape for topics entries; a dict missing 'topic' or both 'event_type'/'topic_base_constant' would raise a raw KeyError downstream instead of a structured exit-2 error with the offending index. Validate each entry's shape on load. - CR-major: descriptions were injected verbatim into /** ... */ JSDoc. A description containing '*/' or a newline would break the generated TypeScript. Escape '*/' to '*\\/' and collapse newlines to spaces. Tests: two new unit tests cover each case. All 12 tests pass.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/scripts/test_topic_parity_scripts.py (1)
258-277: Isolate theREAD_MODEL_TOPICSmismatch in this fixture.This test claims to prove the consumer subscription check, but the fixture also omits the orphan topic from
shared/topics.tsandEXPECTED_TOPICS. If the script reports those mismatches first, the test becomes order-dependent and no longer proves theREAD_MODEL_TOPICSpath.Suggested fixture tweak
(shared / "topics.ts").write_text( - "export const SUFFIX_OMNICLAUDE_COVERED = 'onex.evt.omniclaude.covered.v1';\n" + "export const SUFFIX_OMNICLAUDE_COVERED = 'onex.evt.omniclaude.covered.v1';\n" + "export const SUFFIX_OMNICLAUDE_ORPHAN = 'onex.evt.omniclaude.orphan.v1';\n" ) (server / "read-model-consumer.ts").write_text( "const READ_MODEL_TOPICS: string[] = [SUFFIX_OMNICLAUDE_COVERED];\n" ) (server / "event-bus-health-poller.ts").write_text( - "const EXPECTED_TOPICS: string[] = [SUFFIX_OMNICLAUDE_COVERED];\n" + "const EXPECTED_TOPICS: string[] = [SUFFIX_OMNICLAUDE_COVERED, SUFFIX_OMNICLAUDE_ORPHAN];\n" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/scripts/test_topic_parity_scripts.py` around lines 258 - 277, The test fixture hides the real target (READ_MODEL_TOPICS mismatch) by also omitting the orphan topic from shared/topics.ts and EXPECTED_TOPICS; update the fixture so shared/topics.ts exports both topic suffix constants (e.g., add a constant for onex.evt.omniclaude.orphan.v1) and ensure the EXPECTED_TOPICS array (in event-bus-health-poller.ts) includes both constants, leaving READ_MODEL_TOPICS (in read-model-consumer.ts) unchanged so the test isolates the READ_MODEL_TOPICS mismatch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/scripts/test_topic_parity_scripts.py`:
- Around line 240-247: The test currently only collects lines that start with
"/**" via jsdoc_lines and therefore never inspects a multi-line JSDoc block;
change the logic in tests/scripts/test_topic_parity_scripts.py to locate the
opening JSDoc line (use result.stdout.splitlines()), then collect the full block
by iterating from that index until the closing "*/" line is reached (or until a
line containing "*/"), then run assertions against the full joined block text
(e.g., ensure the block does not contain an unescaped "*/" and that it contains
the escaped "*\\/" sequence); reference the existing jsdoc_lines variable and
result.stdout when implementing the start-and-collect approach.
---
Nitpick comments:
In `@tests/scripts/test_topic_parity_scripts.py`:
- Around line 258-277: The test fixture hides the real target (READ_MODEL_TOPICS
mismatch) by also omitting the orphan topic from shared/topics.ts and
EXPECTED_TOPICS; update the fixture so shared/topics.ts exports both topic
suffix constants (e.g., add a constant for onex.evt.omniclaude.orphan.v1) and
ensure the EXPECTED_TOPICS array (in event-bus-health-poller.ts) includes both
constants, leaving READ_MODEL_TOPICS (in read-model-consumer.ts) unchanged so
the test isolates the READ_MODEL_TOPICS mismatch.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd1f32e1-b12a-4fe2-ac2e-4399f13b896b
📒 Files selected for processing (2)
scripts/sync-topic-registry.pytests/scripts/test_topic_parity_scripts.py
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/sync-topic-registry.py
CodeRabbit flagged that the previous test only filtered lines starting with /** and never inspected the full /** ... */ block body, making the */ check vacuous. Parse complete JSDoc blocks with a regex so the assertion actually verifies the escape (and that newlines are collapsed).
jonahgabriel
removed this pull request from the merge queue due to a manual request
Apr 20, 2026
jonahgabriel
added this pull request to the merge queue
Apr 20, 2026
Collaborator
Author
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.
Summary
Relocates two topic-parity scripts out of
omni_home/scripts/(which is blocked by theno-functional-codepre-commit hook) intoomnibase_infra/scripts/, following the OMN-4922 pattern.scripts/check-topic-parity.py(OMN-4963)scripts/sync-topic-registry.py(OMN-4962)Both were pre-existing violations (merged 2026-03-13 via PRs #50/#51 in omni_home) blocking unrelated docs-only PRs from running
pre-commit run --all-filescleanly.Changes
omnibase_infra/scripts/, preserving shebang + exec bitsOMNI_HOMEenv var (fail-fast via_default_omni_home(), withPath(__file__).resolve().parents[2]as fallback) plus a frozenModelTopicParityPathsPydantic modelomnibase_infra/CLAUDE.mdSPDX policy)PLW0603(global-statement) violations incheck-topic-parity.pyvia the path-model refactor--strictmissing-type-arg violations insync-topic-registry.pyby parameterizingdict→dict[str, str]tests/scripts/test_topic_parity_scripts.pywith 6 smoke tests (shebang, SPDX, argparse, env-var resolution)Companion PR (to land after this merges)
A follow-up omni_home PR will:
git rmboth scripts fromomni_home/scripts/.github/workflows/topic-parity.ymlto invoke./omnibase_infra/scripts/...Sequenced to avoid a mid-flight CI break: this PR lands first so the new paths exist; the omni_home deletion + workflow repoint ships as one commit.
Test plan
uv run pytest tests/scripts/test_topic_parity_scripts.py -v— 6/6 passinguv run ruff check scripts/check-topic-parity.py scripts/sync-topic-registry.py— cleanuv run mypy --strict scripts/check-topic-parity.py scripts/sync-topic-registry.py— cleanpre-commit run --files ...— all hooks pass including SPDX + AI-slopOMNI_HOME=/Users/jonah/Code/omni_home uv run python scripts/{check,sync}-... --checkruns with expected outputSummary by CodeRabbit
Chores
Tests