Repository navigation
feat(OMN-1547): Replace hardcoded topics with validated suffix constants - #206
Conversation
- Add platform topic suffix constants module with import-time validation - Add build_full_topic() composition helper with env/namespace/suffix validation - Update 7 platform files to use suffix constants instead of hardcoded strings - Remove hardcoded omniclaude domain topics from config_consumer.py - Consolidate introspection topic definitions (model = canonical, mixin = consumer) - Add 30 unit tests for suffix validation and topic composition
📝 WalkthroughWalkthroughAdds a new omnibase_infra.topics package with platform suffix constants and topic composition utilities, migrates hard-coded topic strings to shared SUFFIX_* constants across configs/services/tests, introduces topic validation/composition and tests, and removes module-level introspection topic constants. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/omnibase_infra/models/discovery/model_introspection_config.py`:
- Around line 31-35: Docstrings for the introspection config fields still
reference legacy "node.*" topics but the defaults now use ONEX suffixes
(SUFFIX_NODE_HEARTBEAT, SUFFIX_NODE_INTROSPECTION,
SUFFIX_REQUEST_INTROSPECTION). Update the module/class/field docstrings in
model_introspection_config.py (including the docstrings that currently describe
the heartbeat/introspection/request topics at the top of the file and the ones
around the fields at the later sections noted) so they reference the new ONEX
topic suffix defaults and show example topic names using the SUFFIX_* constants
(replace or reword any "node.*" examples), and ensure the docstrings for the
fields that use SUFFIX_NODE_HEARTBEAT, SUFFIX_NODE_INTROSPECTION, and
SUFFIX_REQUEST_INTROSPECTION accurately describe their purpose and default
behavior.
In `@src/omnibase_infra/topics/platform_topic_suffixes.py`:
- Around line 125-137: Replace the raised ValueError in _validate_all_suffixes
with an OnexError and chain it from an underlying exception to preserve context:
if validate_topic_suffix(suffix) returns not result.is_valid, construct a small
underlying Exception (e.g. Exception(result.error)) and then raise
OnexError(f"Invalid platform topic suffix '{suffix}': {result.error}") from that
exception; ensure OnexError is imported where _validate_all_suffixes,
validate_topic_suffix, and ALL_PLATFORM_SUFFIXES are referenced.
In `@src/omnibase_infra/topics/util_topic_composition.py`:
- Around line 45-52: Update the namespace validation in
util_topic_composition.py so composed topics follow ONEX lowercase conventions:
either normalize the input by lowercasing (e.g., set namespace =
namespace.lower() before further validation/composition) or explicitly reject
namespaces that contain any uppercase characters by adding a check using
namespace.islower() and raising TopicCompositionError with a clear message;
adjust the existing validation block that currently checks namespace and allowed
chars (variable name namespace and exception TopicCompositionError) accordingly
so downstream topic composition always uses lowercase.
- Around line 11-12: Change TopicCompositionError to inherit from OnexError (or
the appropriate infra base error) instead of ValueError: update the class
definition for TopicCompositionError to subclass OnexError, add or adjust the
import for OnexError, and keep the existing docstring; ensure any tests or
raises throughout the codebase still refer to TopicCompositionError but now get
the OnexError behavior for consistent project error-handling.
🧹 Nitpick comments (3)
src/omnibase_infra/services/mcp/service_mcp_tool_sync.py (1)
32-33: Consider updating the module docstring “Event Topic” to match the new constant.
IfSUFFIX_NODE_REGISTRATIONresolves to an ONEX topic, the top-level docstring still mentionsnode.registration.v1, which could mislead readers.Also applies to: 80-82
tests/unit/topics/test_util_topic_composition.py (1)
21-25: Consider importing ENV_PREFIXES to avoid drift.The hardcoded environment list
["dev", "staging", "prod", "test", "local"]could become out of sync ifENV_PREFIXESis updated in the source module. Consider importing the constant for consistency.♻️ Suggested improvement
from omnibase_infra.topics import ( SUFFIX_NODE_INTROSPECTION, SUFFIX_NODE_REGISTRATION, TopicCompositionError, build_full_topic, ) +from omnibase_infra.topics.util_topic_composition import ENV_PREFIXES ... def test_build_full_topic_with_different_envs(self) -> None: """Should work with all valid environment prefixes.""" - for env in ["dev", "staging", "prod", "test", "local"]: + for env in ENV_PREFIXES: topic = build_full_topic(env, "myapp", SUFFIX_NODE_INTROSPECTION) assert topic.startswith(f"{env}.myapp.")src/omnibase_infra/mixins/mixin_node_introspection.py (1)
240-249: Consider relocating imports to the top of the file.The topic constant imports are placed mid-file after the logger definition. While this works, Python convention is to place all imports at the module's top (after docstrings and
__future__imports). The current placement may have been intentional to separate "configuration constants" from "type imports", but it reduces discoverability.♻️ Suggested relocation
Move these imports to the top import section (around lines 196-237) with a comment grouping:
# Topic defaults from canonical config (aliased for backward compatibility) from omnibase_infra.models.discovery.model_introspection_config import ( DEFAULT_HEARTBEAT_TOPIC as HEARTBEAT_TOPIC, ) from omnibase_infra.models.discovery.model_introspection_config import ( DEFAULT_INTROSPECTION_TOPIC as INTROSPECTION_TOPIC, ) from omnibase_infra.models.discovery.model_introspection_config import ( DEFAULT_REQUEST_INTROSPECTION_TOPIC as REQUEST_INTROSPECTION_TOPIC, )
| def _validate_all_suffixes() -> None: | ||
| """Validate all suffixes at import time to fail fast on invalid format. | ||
|
|
||
| Raises: | ||
| ValueError: If any suffix fails validation with details about which | ||
| suffix failed and why. | ||
| """ | ||
| for suffix in ALL_PLATFORM_SUFFIXES: | ||
| result = validate_topic_suffix(suffix) | ||
| if not result.is_valid: | ||
| raise ValueError( | ||
| f"Invalid platform topic suffix '{suffix}': {result.error}" | ||
| ) |
There was a problem hiding this comment.
Use OnexError for suffix validation failures.
Line 135-136 raises ValueError, but project guidelines require only OnexError (with error chaining). Please switch to OnexError and chain from an underlying exception to preserve context. As per coding guidelines, only OnexError (or subclasses) should be raised with raise ... from e.
✅ Proposed fix
-from omnibase_core.validation import validate_topic_suffix
+from omnibase_core.validation import validate_topic_suffix
+from omnibase_core.errors import OnexError
@@
for suffix in ALL_PLATFORM_SUFFIXES:
result = validate_topic_suffix(suffix)
if not result.is_valid:
- raise ValueError(
- f"Invalid platform topic suffix '{suffix}': {result.error}"
- )
+ err = ValueError(result.error)
+ raise OnexError(
+ f"Invalid platform topic suffix '{suffix}': {result.error}"
+ ) from err🤖 Prompt for AI Agents
In `@src/omnibase_infra/topics/platform_topic_suffixes.py` around lines 125 - 137,
Replace the raised ValueError in _validate_all_suffixes with an OnexError and
chain it from an underlying exception to preserve context: if
validate_topic_suffix(suffix) returns not result.is_valid, construct a small
underlying Exception (e.g. Exception(result.error)) and then raise
OnexError(f"Invalid platform topic suffix '{suffix}': {result.error}") from that
exception; ensure OnexError is imported where _validate_all_suffixes,
validate_topic_suffix, and ALL_PLATFORM_SUFFIXES are referenced.
- Move imports to top of mixin_node_introspection.py (PEP 8) - Change ALL_PLATFORM_SUFFIXES from list to immutable tuple - Add edge case tests for numeric namespace validation
- Add namespace validation rules to build_full_topic() docstring - Document that numeric-only namespaces are intentionally valid - Mark mixin topic aliases as DEPRECATED with migration guidance
- Remove deprecated topic aliases per no-backwards-compat policy - Restore JSON schema example variation for documentation clarity - Add edge case tests for topic composition (unicode, long names, case)
- Add MAX_NAMESPACE_LENGTH constant (100 chars) to prevent overly long topics - Add validation in build_full_topic() with clear error message - Export MAX_NAMESPACE_LENGTH from topics package for user access - Add test_all_suffix_constants_exported to catch missing exports - Update namespace length tests to verify new validation behavior
Merge resolution: - Resolve conflict in service_mcp_tool_sync.py by keeping SUFFIX_NODE_REGISTRATION constant usage and removing unused GROUP_ID class attribute PR review fixes: 🔴 MAJOR: - platform_topic_suffixes.py: Use OnexError instead of ValueError for import-time suffix validation failures - util_topic_composition.py: TopicCompositionError now extends OnexError instead of ValueError 🟡 MINOR: - model_introspection_config.py: Update docstrings to reference new ONEX suffix constants with actual topic values - util_topic_composition.py: Add lowercase namespace validation to ensure consistent topic naming ⚪ NITPICK: - service_mcp_tool_sync.py: Update module docstring to document the SUFFIX_NODE_REGISTRATION constant being used Test updates: - Update tests to use suffix constants instead of hardcoded strings - Update TopicCompositionError tests for OnexError inheritance - Add lowercase namespace validation test
…onsumer The PR intentionally removed hardcoded domain topics from ConfigSessionConsumer and added fail-fast validation requiring explicit topic configuration. This updates the test suite to: - Add test_consumer_requires_explicit_topics: verifies ProtocolConfigurationError is raised when no topics are provided (fail-fast behavior) - Add test_consumer_uses_explicit_config: verifies consumer works correctly when topics are explicitly configured - Remove test_consumer_uses_default_config: no longer valid since empty defaults are now rejected This fixes CI failure caused by the intentional behavioral change in config_consumer.py.
| @@ -0,0 +1,140 @@ | |||
| """Platform-reserved topic suffixes for ONEX infrastructure. | |||
There was a problem hiding this comment.
File naming convention violation: Per CLAUDE.md, files containing constants should be named constants_<name>.py. This file defines module-level constants (SUFFIX_NODE_REGISTRATION, etc.) and should be renamed.
The codebase shows 100% consistency with this pattern: constants_metrics.py, constants_notification.py, constants_dlq.py.
Fix: Rename platform_topic_suffixes.py to constants_platform_topic_suffixes.py and update all imports in omnibase_infra.topics package and consuming modules.
Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.