Repository navigation
fix: Crenshaw architecture review fixes [OMN-6095] - #795
Merged
jonahgabriel merged 13 commits intoMar 24, 2026
Merged
jonahgabriel merged 13 commits into
jonahgabriel merged 13 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoved hardcoded internal IP endpoints and added environment-driven LLM/embedding URL resolution; introduced CI and local validators to detect hardcoded IPs; updated quorum and node code to fail fast when env vars are missing; added docs and tests for endpoint resolution and health probing. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Caller/Test
participant Node as NodeTransitionSelectorEffect
participant Env as Environment
participant Probe as HealthProbe
participant LLM as LLM Service
Test->>Node: select(request)
Node->>Env: read LLM_CODER_FAST_URL
Env-->>Node: endpoint (or empty)
alt endpoint empty
Node-->>Test: SelectionErrorKind.MODEL_UNAVAILABLE (config error with LLM_CODER_FAST_URL)
else endpoint present
Node->>Probe: GET {endpoint}/health
Probe-->>Node: 200 / non-200 / error
alt healthy
Node->>LLM: POST /v1/chat/completions (call model)
LLM-->>Node: response
Node-->>Test: selection success
else unhealthy or network error
Node-->>Test: SelectionErrorKind.MODEL_UNAVAILABLE (includes endpoint + curl hint)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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🧪 Generate unit tests (beta)
Comment |
jonahgabriel
enabled auto-merge
March 23, 2026 16:23
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/omniclaude/lib/utils/consensus/quorum.py (2)
220-229:⚠️ Potential issue | 🟠 MajorStub mode now depends on
LLM_CODER_URL.
AIQuorum.__init__()always calls_load_models_from_config()before it decides whether AI scoring is disabled, so Line 229 turns an enabled OPENAI_COMPATIBLE config entry into a hard failure even forstub_mode=True/enable_ai_scoring=False. Resolve the default endpoint inside the scoring call path instead of while loading config.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omniclaude/lib/utils/consensus/quorum.py` around lines 220 - 229, AIQuorum.__init__ currently calls _load_models_from_config which resolves OPENAI_COMPATIBLE endpoints via _resolve_llm_coder_url, causing a hard failure when LLM_CODER_URL is missing even if stub_mode or enable_ai_scoring is False; change _load_models_from_config (or the model-loading path that sets endpoint) to only record per-model "base_url" if present and leave endpoint unset for OPENAI_COMPATIBLE entries, and move the fallback resolution using _resolve_llm_coder_url into the scoring/execution path (the method that actually performs AI scoring) so the default endpoint is resolved at use-time (and can respect stub_mode/enable_ai_scoring) rather than during AIQuorum.__init__.
67-74:⚠️ Potential issue | 🔴 CriticalThis resolves
LLM_CODER_URLat import time.Line 73 now runs during
ModelConfigconstruction, andAIQuorum.DEFAULT_MODELSstill constructsModelConfigat class definition time. Importingomniclaude.lib.utils.consensus.quorumwill therefore raise before callers can choose stub mode or monkeypatch the env, which also breaks the new top-level imports intests/lib/test_quorum.py. Defer OPENAI_COMPATIBLE endpoint resolution until the model is actually used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omniclaude/lib/utils/consensus/quorum.py` around lines 67 - 74, The code currently calls _resolve_llm_coder_url() inside ModelConfig.__post_init__, causing LLM_CODER_URL to be resolved at import/ModelConfig construction (affecting AIQuorum.DEFAULT_MODELS). Change this to defer resolution: do not call _resolve_llm_coder_url() in __post_init__; leave endpoint as None when provider == ModelProvider.OPENAI_COMPATIBLE, and implement lazy resolution (e.g., a ModelConfig.get_endpoint() method or resolve in the actual send/use path) that calls _resolve_llm_coder_url() the first time the endpoint is needed; update callers that need an endpoint to use that lazy accessor instead of relying on __post_init__ to set it.src/omniclaude/nodes/node_transition_selector_effect/node.py (1)
151-196:⚠️ Potential issue | 🟠 Major
_probe_endpoint()never affects selection flow.Line 369 adds the health probe, but the new failure paths on Lines 151-196 still return without calling it. Right now unavailable endpoints behave exactly as before—request, then timeout/error—so the new health-check resilience is not actually in the runtime path. If this is meant to stay best-effort, invoke the probe only after a failure to enrich the returned result.
Also applies to: 369-385
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omniclaude/nodes/node_transition_selector_effect/node.py` around lines 151 - 196, The RuntimeError, TimeoutError and generic Exception handlers in select transition flow (in node_transition_selector_effect.node) currently return ModelTransitionSelectorResult before ever calling _probe_endpoint(), so the new health probe added around lines ~369–385 is never used; change each exception handler (the RuntimeError except block, the TimeoutError except block, and the generic except Exception block that return ModelTransitionSelectorResult) to call _probe_endpoint() after catching the error and before returning, capture its output (and include it in the logger.extra and in the returned ModelTransitionSelectorResult.error_detail), and ensure the TimeoutError path also still resolves endpoint via _resolve_endpoint() when building the enriched message so the probe result augments the returned error information rather than being a no-op.
🧹 Nitpick comments (1)
docs/reference/NODE_BEHAVIOR_MAP.md (1)
11-15: Soften the “thin orchestration shell” rule.Line 12 reads as universal, but this same page points readers to
_build_prompt()and_call_model()insrc/omniclaude/nodes/node_transition_selector_effect/node.py. Calling this pattern “usual” instead of absolute will keep contributors from searching for a handler/backend that does not exist for nodes like this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/NODE_BEHAVIOR_MAP.md` around lines 11 - 15, Update the description for "Node" to soften the absolute claim by changing "Thin orchestration shell -- delegates to protocol/handler" to language like "Usually a thin orchestration shell -- often delegates to protocol/handler, but may implement node-specific logic (see _build_prompt() and _call_model() in node_transition_selector_effect's node.py)"; reference the _build_prompt and _call_model symbols so readers understand nodes can contain internal logic rather than assume every node relies solely on an external handler/backend.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/reference/PR_VALIDATION_PATH.md`:
- Line 3: The PR_VALIDATION_PATH.md header claims to list the "complete ordered
list of checks" but is missing checks added in ci.yml (e.g., Markdown Link
Check, F5 no compact cmd topic, Architecture Invariants, Version Pin Compliance,
Skill Hygiene, Mode Metadata Integrity, Boundary Parity, CI Summary job); update
PR_VALIDATION_PATH.md to either scope the doc to only merge-blocking validations
or mirror the workflow job names from ci.yml exactly, add the missing checks
(and their ordering) into the doc body (also ensure sections covering lines
13-65 are revised to match), and include a short note indicating whether the
document is exhaustive or intentionally scoped.
In `@scripts/validate-local.sh`:
- Line 49: Replace the local topic-naming validation call so it matches CI:
update the invocation inside the script where run_check "topic naming" calls the
Python validator (the line that currently references lint_topic_names.py) to
call validate_topic_naming.py instead; ensure the run_check invocation and any
surrounding quoting/arguments remain unchanged so the local entrypoint uses the
same validator script as .github/workflows/ci.yml.
- Around line 21-26: The post-increment arithmetic expressions ((PASS++)) and
((FAIL++)) in the script cause a non-zero exit status under set -euo pipefail;
change them to pre-increment ((++PASS)) and ((++FAIL)) so the arithmetic returns
0 and the script doesn't prematurely exit—update the two increment expressions
that reference the PASS and FAIL counters accordingly.
In `@scripts/validation/validate_no_hardcoded_ip.py`:
- Around line 36-50: The scan currently only walks Path("src") in main(), so
hardcoded IPs in other roots like tests/ or scripts/ are missed; update main()
to iterate over a configurable list of roots (e.g., ("src", "tests", "scripts")
or derive from _ALLOWED_PATHS/REPO_ROOT) and run the same rglob/line-check logic
for each root, keeping use of _IP_PATTERN and _ALLOWED_PATHS unchanged so
violations collection and reporting remain the same.
In `@src/omniclaude/hooks/lib/code_context_resolver.py`:
- Around line 108-115: The constructor in CodeContextResolver unconditionally
raises when LLM_EMBEDDING_URL is missing; change it to allow missing embedding
URL when semantic search is disabled by treating a missing qdrant_client as a
graceful-degradation path. In __init__ (or the constructor logic where
resolved_url is computed), only raise if semantic search is intended (e.g.,
qdrant_client is not None or an explicit embedding_url argument was provided);
if qdrant_client is None and no embedding URL is set, set self._embedding_url to
"" (or None) and proceed so callers creating
CodeContextResolver(qdrant_client=None) get the existing empty-result fallback
instead of an exception. Ensure references use the existing symbols
resolved_url, embedding_url, qdrant_client, and self._embedding_url so the
change is easy to locate.
---
Outside diff comments:
In `@src/omniclaude/lib/utils/consensus/quorum.py`:
- Around line 220-229: AIQuorum.__init__ currently calls
_load_models_from_config which resolves OPENAI_COMPATIBLE endpoints via
_resolve_llm_coder_url, causing a hard failure when LLM_CODER_URL is missing
even if stub_mode or enable_ai_scoring is False; change _load_models_from_config
(or the model-loading path that sets endpoint) to only record per-model
"base_url" if present and leave endpoint unset for OPENAI_COMPATIBLE entries,
and move the fallback resolution using _resolve_llm_coder_url into the
scoring/execution path (the method that actually performs AI scoring) so the
default endpoint is resolved at use-time (and can respect
stub_mode/enable_ai_scoring) rather than during AIQuorum.__init__.
- Around line 67-74: The code currently calls _resolve_llm_coder_url() inside
ModelConfig.__post_init__, causing LLM_CODER_URL to be resolved at
import/ModelConfig construction (affecting AIQuorum.DEFAULT_MODELS). Change this
to defer resolution: do not call _resolve_llm_coder_url() in __post_init__;
leave endpoint as None when provider == ModelProvider.OPENAI_COMPATIBLE, and
implement lazy resolution (e.g., a ModelConfig.get_endpoint() method or resolve
in the actual send/use path) that calls _resolve_llm_coder_url() the first time
the endpoint is needed; update callers that need an endpoint to use that lazy
accessor instead of relying on __post_init__ to set it.
In `@src/omniclaude/nodes/node_transition_selector_effect/node.py`:
- Around line 151-196: The RuntimeError, TimeoutError and generic Exception
handlers in select transition flow (in node_transition_selector_effect.node)
currently return ModelTransitionSelectorResult before ever calling
_probe_endpoint(), so the new health probe added around lines ~369–385 is never
used; change each exception handler (the RuntimeError except block, the
TimeoutError except block, and the generic except Exception block that return
ModelTransitionSelectorResult) to call _probe_endpoint() after catching the
error and before returning, capture its output (and include it in the
logger.extra and in the returned ModelTransitionSelectorResult.error_detail),
and ensure the TimeoutError path also still resolves endpoint via
_resolve_endpoint() when building the enriched message so the probe result
augments the returned error information rather than being a no-op.
---
Nitpick comments:
In `@docs/reference/NODE_BEHAVIOR_MAP.md`:
- Around line 11-15: Update the description for "Node" to soften the absolute
claim by changing "Thin orchestration shell -- delegates to protocol/handler" to
language like "Usually a thin orchestration shell -- often delegates to
protocol/handler, but may implement node-specific logic (see _build_prompt() and
_call_model() in node_transition_selector_effect's node.py)"; reference the
_build_prompt and _call_model symbols so readers understand nodes can contain
internal logic rather than assume every node relies solely on an external
handler/backend.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 725d5591-0c2c-444a-a6a1-5fac498312b1
📒 Files selected for processing (14)
.github/workflows/ci.ymldocs/reference/NODE_BEHAVIOR_MAP.mddocs/reference/PR_VALIDATION_PATH.mdscripts/validate-local.shscripts/validation/validate_no_hardcoded_ip.pysrc/omniclaude/hooks/lib/code_context_resolver.pysrc/omniclaude/lib/utils/consensus/quorum.pysrc/omniclaude/nodes/node_transition_selector_effect/contract.yamlsrc/omniclaude/nodes/node_transition_selector_effect/node.pysrc/omniclaude/nodes/node_transition_selector_effect/protocols/protocol_transition_selector.pysrc/omniclaude/shared/models/model_skill_completion_event.pytests/lib/test_quorum.pytests/nodes/test_node_transition_selector_effect/test_integration.pytests/nodes/test_node_transition_selector_effect/test_unit.py
jonahgabriel
force-pushed
the
jonahgabriel/omn-6095-crenshaw-architecture-review-fixes
branch
2 times, most recently
from
March 23, 2026 19:12
de2ee17 to
fb7d92a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (6)
docs/reference/NODE_BEHAVIOR_MAP.md (1)
19-48: Optional: label node sections as “examples” to avoid rigidity.A short qualifier (e.g., “Representative examples; exact symbols may evolve”) would keep this useful even as internals are refactored.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/NODE_BEHAVIOR_MAP.md` around lines 19 - 48, Add a short qualifier line under the "Key Nodes -- Behavior Location" heading (e.g., "Representative examples; exact symbols may evolve") and mark each node section heading as examples by appending or replacing the label with "(examples)" for the three node headings: "Transition Selector (node_transition_selector_effect)", "Local LLM Inference (node_local_llm_inference_effect)", and "Agent Routing (node_agent_routing_compute)"; this keeps the docs flexible while preserving the existing table contents and symbol references like _build_prompt(), _call_model(), VllmInferenceBackend.infer(), LocalLlmEndpointRegistry, HandlerRoutingLlm, and HandlerRoutingDefault.docs/reference/PR_VALIDATION_PATH.md (1)
3-3: Consider adding an explicit “source of truth + update rule” note.Optional: add one sentence near Line 3 or Line 74 like “If this doc and workflow differ,
.github/workflows/ci.ymland.github/required-checks.yamlare authoritative.” It helps prevent future drift ambiguity.Also applies to: 73-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/PR_VALIDATION_PATH.md` at line 3, Add a concise “source of truth + update rule” sentence to PR_VALIDATION_PATH.md: insert one near the top summary and one near the checks list that states the authoritative sources are the repository’s CI workflow file and the required-checks configuration (i.e., those YAML configs in the repo), and instructs maintainers to update those files if the doc and workflow differ so the workflow/config remain authoritative.scripts/validate-local.sh (2)
10-10: Validate CLI arguments explicitly.At Line 10, unknown args are silently accepted and ignored. Adding strict arg parsing avoids accidental misuse and makes
--quickbehavior clearer.Proposed refactor
-QUICK="${1:-}" +if [ "$#" -gt 1 ] || { [ "$#" -eq 1 ] && [ "${1}" != "--quick" ]; }; then + echo "Usage: ./scripts/validate-local.sh [--quick]" + exit 2 +fi +QUICK="${1:-}"Also applies to: 38-40, 53-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/validate-local.sh` at line 10, The script silently accepts unknown CLI args via QUICK="${1:-}"; change to explicit argument parsing so only "" or "--quick" are allowed: add a small parse block that checks $1 and if it is "--quick" sets QUICK=1, if empty leaves QUICK unset/0, otherwise prints a usage/help message and exits non‑zero; implement a reusable usage() function (or add a clear echo message) and apply the same strict check logic to the other similar arg reads referenced (the blocks around the other QUICK-like assignments at the later occurrences).
17-27: Show failing command output to improve local debugging.At Line 21, all output is discarded, so a
FAILresult gives no immediate clue. Prefer quiet-on-pass and verbose-on-fail behavior.Proposed refactor
run_check() { local name="$1" shift printf "%-45s " "$name" - if "$@" > /dev/null 2>&1; then + local output + if output="$("$@" 2>&1)"; then echo -e "${GREEN}PASS${NC}" PASS=$((PASS + 1)) else echo -e "${RED}FAIL${NC}" FAIL=$((FAIL + 1)) + printf '%s\n' "$output" fi }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/validate-local.sh` around lines 17 - 27, The run_check function currently discards all command output which hides failure details; change it to run the command with stdout/stderr redirected into a temporary file (e.g., use mktemp), check the exit status, print PASS and increment PASS when it succeeds (and remove the temp file), but on failure print FAIL, increment FAIL and then dump the temp file contents (and optionally the failing command) so the user sees the error output; ensure cleanup of the temp file and keep the existing formatting and PASS/FAIL counters in run_check.src/omniclaude/nodes/node_transition_selector_effect/node.py (2)
171-178: Consider caching the resolved endpoint to avoid redundant lookups.
_resolve_endpoint()is called again after the timeout, but_call_model_oncealready resolved it successfully (otherwise aRuntimeErrorwould have been caught earlier). This is minor overhead but could be avoided by capturing the endpoint before the model call.♻️ Optional: Cache endpoint before model call
# Call the model with timeout try: + endpoint = _resolve_endpoint() # Cache for error messages raw_output = await asyncio.wait_for( self._call_model(prompt), timeout=_SELECTION_TIMEOUT_SECONDS, ) except RuntimeError as exc: # ... except TimeoutError: logger.warning( "transition_selector.timeout", # ... ) - endpoint = _resolve_endpoint() return ModelTransitionSelectorResult( # ... ) except Exception as exc: - endpoint = _resolve_endpoint() logger.warning( # ... )This avoids the redundant env var lookup and also ensures the endpoint in the error message matches what was actually used.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omniclaude/nodes/node_transition_selector_effect/node.py` around lines 171 - 178, Capture and reuse the resolved endpoint to avoid redundant lookups: call _resolve_endpoint() once before invoking _call_model_once and store it in a local variable (used later in the timeout branch) so the error_detail in ModelTransitionSelectorResult uses that same cached endpoint; update the code paths that reference _resolve_endpoint() (notably the timeout branch creating ModelTransitionSelectorResult with SelectionErrorKind.SELECTION_TIMEOUT and _SELECTION_TIMEOUT_SECONDS) to use the cached variable instead.
127-128: Note: Variable naming uses snake_case (existing convention).The coding guidelines specify camelCase for Python variables, but this file (and the codebase) uses snake_case, which aligns with PEP 8. Since this is an established pattern and the PR follows existing conventions, flagging this as informational only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omniclaude/nodes/node_transition_selector_effect/node.py` around lines 127 - 128, The review is informational: the comment flags camelCase vs snake_case, but this module uses snake_case per PEP8; do not rename variables. Keep the existing snake_case identifiers (start_ms and correlation_id) in node_transition_selector_effect/node.py to maintain consistency with the codebase and follow PEP8; no code changes required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/reference/NODE_BEHAVIOR_MAP.md`:
- Around line 19-48: Add a short qualifier line under the "Key Nodes -- Behavior
Location" heading (e.g., "Representative examples; exact symbols may evolve")
and mark each node section heading as examples by appending or replacing the
label with "(examples)" for the three node headings: "Transition Selector
(node_transition_selector_effect)", "Local LLM Inference
(node_local_llm_inference_effect)", and "Agent Routing
(node_agent_routing_compute)"; this keeps the docs flexible while preserving the
existing table contents and symbol references like _build_prompt(),
_call_model(), VllmInferenceBackend.infer(), LocalLlmEndpointRegistry,
HandlerRoutingLlm, and HandlerRoutingDefault.
In `@docs/reference/PR_VALIDATION_PATH.md`:
- Line 3: Add a concise “source of truth + update rule” sentence to
PR_VALIDATION_PATH.md: insert one near the top summary and one near the checks
list that states the authoritative sources are the repository’s CI workflow file
and the required-checks configuration (i.e., those YAML configs in the repo),
and instructs maintainers to update those files if the doc and workflow differ
so the workflow/config remain authoritative.
In `@scripts/validate-local.sh`:
- Line 10: The script silently accepts unknown CLI args via QUICK="${1:-}";
change to explicit argument parsing so only "" or "--quick" are allowed: add a
small parse block that checks $1 and if it is "--quick" sets QUICK=1, if empty
leaves QUICK unset/0, otherwise prints a usage/help message and exits non‑zero;
implement a reusable usage() function (or add a clear echo message) and apply
the same strict check logic to the other similar arg reads referenced (the
blocks around the other QUICK-like assignments at the later occurrences).
- Around line 17-27: The run_check function currently discards all command
output which hides failure details; change it to run the command with
stdout/stderr redirected into a temporary file (e.g., use mktemp), check the
exit status, print PASS and increment PASS when it succeeds (and remove the temp
file), but on failure print FAIL, increment FAIL and then dump the temp file
contents (and optionally the failing command) so the user sees the error output;
ensure cleanup of the temp file and keep the existing formatting and PASS/FAIL
counters in run_check.
In `@src/omniclaude/nodes/node_transition_selector_effect/node.py`:
- Around line 171-178: Capture and reuse the resolved endpoint to avoid
redundant lookups: call _resolve_endpoint() once before invoking
_call_model_once and store it in a local variable (used later in the timeout
branch) so the error_detail in ModelTransitionSelectorResult uses that same
cached endpoint; update the code paths that reference _resolve_endpoint()
(notably the timeout branch creating ModelTransitionSelectorResult with
SelectionErrorKind.SELECTION_TIMEOUT and _SELECTION_TIMEOUT_SECONDS) to use the
cached variable instead.
- Around line 127-128: The review is informational: the comment flags camelCase
vs snake_case, but this module uses snake_case per PEP8; do not rename
variables. Keep the existing snake_case identifiers (start_ms and
correlation_id) in node_transition_selector_effect/node.py to maintain
consistency with the codebase and follow PEP8; no code changes required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 50db1fca-0a41-461e-88d4-2fc656809b82
📒 Files selected for processing (14)
.github/workflows/ci.ymldocs/reference/NODE_BEHAVIOR_MAP.mddocs/reference/PR_VALIDATION_PATH.mdscripts/validate-local.shscripts/validation/validate_no_hardcoded_ip.pysrc/omniclaude/hooks/lib/code_context_resolver.pysrc/omniclaude/lib/utils/consensus/quorum.pysrc/omniclaude/nodes/node_transition_selector_effect/contract.yamlsrc/omniclaude/nodes/node_transition_selector_effect/node.pysrc/omniclaude/nodes/node_transition_selector_effect/protocols/protocol_transition_selector.pysrc/omniclaude/shared/models/model_skill_completion_event.pytests/lib/test_quorum.pytests/nodes/test_node_transition_selector_effect/test_integration.pytests/nodes/test_node_transition_selector_effect/test_unit.py
✅ Files skipped from review due to trivial changes (3)
- src/omniclaude/shared/models/model_skill_completion_event.py
- src/omniclaude/nodes/node_transition_selector_effect/contract.yaml
- src/omniclaude/nodes/node_transition_selector_effect/protocols/protocol_transition_selector.py
🚧 Files skipped from review as they are similar to previous changes (6)
- src/omniclaude/hooks/lib/code_context_resolver.py
- tests/nodes/test_node_transition_selector_effect/test_integration.py
- tests/lib/test_quorum.py
- scripts/validation/validate_no_hardcoded_ip.py
- tests/nodes/test_node_transition_selector_effect/test_unit.py
- .github/workflows/ci.yml
jonahgabriel
force-pushed
the
jonahgabriel/omn-6095-crenshaw-architecture-review-fixes
branch
from
March 23, 2026 20:11
5e1279e to
b6e4579
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/validation/validate_no_hardcoded_ip.py (1)
55-57: Consider handling encoding errors gracefully.
path.read_text()defaults to the platform encoding and can raiseUnicodeDecodeErrorif a file contains non-UTF-8 bytes. This could cause the CI job to crash unexpectedly on edge cases.🛡️ Proposed defensive fix
- for i, line in enumerate(path.read_text().splitlines(), 1): + try: + content = path.read_text(encoding="utf-8") + except UnicodeDecodeError: + continue # Skip binary/non-UTF-8 files + for i, line in enumerate(content.splitlines(), 1):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/validation/validate_no_hardcoded_ip.py` around lines 55 - 57, The loop that reads file contents with path.read_text() can raise UnicodeDecodeError on non-UTF-8 bytes; update the code around the read to handle encoding errors by wrapping the read in a try/except UnicodeDecodeError (or use path.read_bytes() then bytes.decode('utf-8', errors='replace')), so that you still iterate lines and apply _IP_PATTERN and _SUPPRESS_MARKER without crashing; ensure you reference the same variables (path, _IP_PATTERN, _SUPPRESS_MARKER, violations) and preserve the existing violation message format.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/validation/validate_no_hardcoded_ip.py`:
- Around line 55-57: The loop that reads file contents with path.read_text() can
raise UnicodeDecodeError on non-UTF-8 bytes; update the code around the read to
handle encoding errors by wrapping the read in a try/except UnicodeDecodeError
(or use path.read_bytes() then bytes.decode('utf-8', errors='replace')), so that
you still iterate lines and apply _IP_PATTERN and _SUPPRESS_MARKER without
crashing; ensure you reference the same variables (path, _IP_PATTERN,
_SUPPRESS_MARKER, violations) and preserve the existing violation message
format.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8bb6d356-af80-4059-be74-0b58aa740192
📒 Files selected for processing (14)
.github/workflows/ci.ymldocs/reference/NODE_BEHAVIOR_MAP.mddocs/reference/PR_VALIDATION_PATH.mdscripts/validate-local.shscripts/validation/validate_no_hardcoded_ip.pysrc/omniclaude/hooks/lib/code_context_resolver.pysrc/omniclaude/lib/utils/consensus/quorum.pysrc/omniclaude/nodes/node_transition_selector_effect/contract.yamlsrc/omniclaude/nodes/node_transition_selector_effect/node.pysrc/omniclaude/nodes/node_transition_selector_effect/protocols/protocol_transition_selector.pysrc/omniclaude/shared/models/model_skill_completion_event.pytests/lib/test_quorum.pytests/nodes/test_node_transition_selector_effect/test_integration.pytests/nodes/test_node_transition_selector_effect/test_unit.py
✅ Files skipped from review due to trivial changes (4)
- src/omniclaude/nodes/node_transition_selector_effect/protocols/protocol_transition_selector.py
- src/omniclaude/shared/models/model_skill_completion_event.py
- scripts/validate-local.sh
- tests/nodes/test_node_transition_selector_effect/test_unit.py
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/nodes/test_node_transition_selector_effect/test_integration.py
- src/omniclaude/nodes/node_transition_selector_effect/node.py
- src/omniclaude/nodes/node_transition_selector_effect/contract.yaml
- tests/lib/test_quorum.py
- .github/workflows/ci.yml
- src/omniclaude/hooks/lib/code_context_resolver.py
- src/omniclaude/lib/utils/consensus/quorum.py
…ector [OMN-6096] Replace _DEFAULT_LLM_ENDPOINT with _resolve_endpoint() that fails fast when LLM_CODER_FAST_URL is not set. Endpoint resolution happens at call time, not import time, so misconfiguration is caught when the node is actually used. select() catches RuntimeError and translates to structured ModelTransitionSelectorResult with error_kind=MODEL_UNAVAILABLE.
…ctor [OMN-6097] Set default_endpoint to null rather than a non-routable placeholder. The contract documents the env var name in endpoint_env on the adjacent line.
…[OMN-6105,OMN-6106,OMN-6107] - Add _probe_endpoint() for best-effort LLM health checking - Improve MODEL_UNAVAILABLE and SELECTION_TIMEOUT errors with endpoint URL, env var name, and curl remediation command - Add INVARIANT docstring to _build_prompt confirming it is pure - Add 4 new unit tests covering probe, actionable errors, and purity
…ilures [OMN-6095] Root cause: ModelConfig.__post_init__ called _resolve_llm_coder_url() eagerly at class-load time (via AIQuorum.DEFAULT_MODELS), causing 3 test files to fail collection in CI where LLM_CODER_URL is not set. Fix: Add ModelConfig.resolve_endpoint() for lazy resolution. Endpoint is now resolved at API call time, not at import/construction time. Also addresses CodeRabbit review feedback: - code_context_resolver: LLM_EMBEDDING_URL now optional (graceful degradation) - validate-local.sh: fix ((PASS++)) set-e trap, use correct topic script - validate_no_hardcoded_ip.py: scan tests/ and scripts/ too, respect onex-allow-internal-ip suppression marker - PR_VALIDATION_PATH.md: add missing CI checks (Markdown Link, F5.1, Mode Metadata, Skill Hygiene, etc.)
jonahgabriel
force-pushed
the
jonahgabriel/omn-6095-crenshaw-architecture-review-fixes
branch
from
March 23, 2026 22:11
b6e4579 to
22dbe73
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/omniclaude/hooks/lib/code_context_resolver.py (1)
108-109:⚠️ Potential issue | 🟡 MinorUse camelCase for the new local variable name.
This segment introduces
resolved_url, which conflicts with the repository’s Python naming convention.Proposed fix
- resolved_url = embedding_url or os.environ.get("LLM_EMBEDDING_URL", "") - if not resolved_url: + resolvedUrl = embedding_url or os.environ.get("LLM_EMBEDDING_URL", "") + if not resolvedUrl: logger.warning( "LLM_EMBEDDING_URL is not set. " "Semantic code context resolution will be unavailable. " "Set LLM_EMBEDDING_URL in ~/.omnibase/.env to enable." ) - self._embedding_url = resolved_url + self._embedding_url = resolvedUrlAs per coding guidelines, "Use camelCase for variable and function names in Python code (follows JavaScript/TypeScript convention for clarity in multi-language contexts)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omniclaude/hooks/lib/code_context_resolver.py` around lines 108 - 109, The local variable resolved_url violates the project's camelCase naming convention; rename it to resolvedUrl (and update any usages in this function) where it's assigned from embedding_url or os.environ.get("LLM_EMBEDDING_URL", "") and where it's later referenced so the assignment and subsequent checks use resolvedUrl consistently.
🧹 Nitpick comments (2)
src/omniclaude/lib/utils/consensus/quorum.py (1)
82-95: Consider documenting the caching side effect.The method mutates
self.endpointat line 92, caching the resolved value for subsequent calls. This is a reasonable optimization, but the docstring only documents the RuntimeError case. Adding a note about the caching behavior would improve clarity for maintainers.📝 Suggested docstring update
def resolve_endpoint(self) -> str: """Return the endpoint URL, resolving lazily for OPENAI_COMPATIBLE. + For OPENAI_COMPATIBLE providers without an explicit endpoint, this + method resolves LLM_CODER_URL on first call and caches the result + in ``self.endpoint`` for subsequent calls. + Raises: RuntimeError: If the provider is OPENAI_COMPATIBLE and LLM_CODER_URL is not set. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omniclaude/lib/utils/consensus/quorum.py` around lines 82 - 95, Document that resolve_endpoint() mutates and caches the resolved URL in self.endpoint: update the resolve_endpoint docstring to state that when provider == ModelProvider.OPENAI_COMPATIBLE the method will call _resolve_llm_coder_url() and store the result in self.endpoint for reuse (i.e., it has a caching side effect), and keep the existing RuntimeError note; reference the method name resolve_endpoint, the attribute self.endpoint, the provider check ModelProvider.OPENAI_COMPATIBLE, and the helper _resolve_llm_coder_url() so maintainers know where the value comes from.scripts/validation/validate_no_hardcoded_ip.py (1)
39-57: Align Python identifiers with the repository camelCase convention.
main,scan_dirs,existing_dirs,scan_dir, andviolationscurrently use snake_case. This repo’s Python style rule requires camelCase identifiers.As per coding guidelines, "Use camelCase for variable and function names in Python code (follows JavaScript/TypeScript convention for clarity in multi-language contexts)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/validation/validate_no_hardcoded_ip.py` around lines 39 - 57, Rename the snake_case identifiers to camelCase throughout the function: change function-level variable names scan_dirs -> scanDirs, existing_dirs -> existingDirs, scan_dir -> scanDir, and violations -> violations (ensure its usage remains consistent) and update every reference accordingly inside the main() function and its loops (including the for loops, conditionals, and the violations.append call); keep all logic, constants (_ALLOWED_PATHS, _IP_PATTERN, _SUPPRESS_MARKER) and types intact and only rename the symbols so references match (e.g., for scanDir in existingDirs, if not existingDirs, etc.).
🤖 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/validation/validate_no_hardcoded_ip.py`:
- Around line 55-57: The validator can crash on non-UTF files because
path.read_text() is called without encoding; update the loop that reads files
(the call to path.read_text() used with _IP_PATTERN/_SUPPRESS_MARKER and
appending to violations) to call path.read_text(encoding="utf-8",
errors="replace") so non-UTF or binary content is tolerated and lines are safely
scanned for _IP_PATTERN without raising UnicodeDecodeError.
- Around line 21-27: The _IP_PATTERN regex currently matches IPs embedded inside
larger numeric strings and should be constrained with token boundaries; update
the _IP_PATTERN definition to wrap the existing pattern with a negative
lookbehind (?<![\d.]) at the start and a negative lookahead (?![\d.]) at the end
so matches are not preceded or followed by digits or dots, ensuring the pattern
still uses a raw string and compiles correctly (keep the same alternation for
192./10./172. and the optional port) and adjust any surrounding
quotes/concatenation accordingly in validate_no_hardcoded_ip.py.
---
Duplicate comments:
In `@src/omniclaude/hooks/lib/code_context_resolver.py`:
- Around line 108-109: The local variable resolved_url violates the project's
camelCase naming convention; rename it to resolvedUrl (and update any usages in
this function) where it's assigned from embedding_url or
os.environ.get("LLM_EMBEDDING_URL", "") and where it's later referenced so the
assignment and subsequent checks use resolvedUrl consistently.
---
Nitpick comments:
In `@scripts/validation/validate_no_hardcoded_ip.py`:
- Around line 39-57: Rename the snake_case identifiers to camelCase throughout
the function: change function-level variable names scan_dirs -> scanDirs,
existing_dirs -> existingDirs, scan_dir -> scanDir, and violations -> violations
(ensure its usage remains consistent) and update every reference accordingly
inside the main() function and its loops (including the for loops, conditionals,
and the violations.append call); keep all logic, constants (_ALLOWED_PATHS,
_IP_PATTERN, _SUPPRESS_MARKER) and types intact and only rename the symbols so
references match (e.g., for scanDir in existingDirs, if not existingDirs, etc.).
In `@src/omniclaude/lib/utils/consensus/quorum.py`:
- Around line 82-95: Document that resolve_endpoint() mutates and caches the
resolved URL in self.endpoint: update the resolve_endpoint docstring to state
that when provider == ModelProvider.OPENAI_COMPATIBLE the method will call
_resolve_llm_coder_url() and store the result in self.endpoint for reuse (i.e.,
it has a caching side effect), and keep the existing RuntimeError note;
reference the method name resolve_endpoint, the attribute self.endpoint, the
provider check ModelProvider.OPENAI_COMPATIBLE, and the helper
_resolve_llm_coder_url() so maintainers know where the value comes from.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4acb6901-5004-4c89-9539-dd8e1f41b643
📒 Files selected for processing (14)
.github/workflows/ci.ymldocs/reference/NODE_BEHAVIOR_MAP.mddocs/reference/PR_VALIDATION_PATH.mdscripts/validate-local.shscripts/validation/validate_no_hardcoded_ip.pysrc/omniclaude/hooks/lib/code_context_resolver.pysrc/omniclaude/lib/utils/consensus/quorum.pysrc/omniclaude/nodes/node_transition_selector_effect/contract.yamlsrc/omniclaude/nodes/node_transition_selector_effect/node.pysrc/omniclaude/nodes/node_transition_selector_effect/protocols/protocol_transition_selector.pysrc/omniclaude/shared/models/model_skill_completion_event.pytests/lib/test_quorum.pytests/nodes/test_node_transition_selector_effect/test_integration.pytests/nodes/test_node_transition_selector_effect/test_unit.py
✅ Files skipped from review due to trivial changes (1)
- src/omniclaude/nodes/node_transition_selector_effect/protocols/protocol_transition_selector.py
🚧 Files skipped from review as they are similar to previous changes (8)
- src/omniclaude/shared/models/model_skill_completion_event.py
- tests/nodes/test_node_transition_selector_effect/test_integration.py
- src/omniclaude/nodes/node_transition_selector_effect/contract.yaml
- scripts/validate-local.sh
- tests/lib/test_quorum.py
- .github/workflows/ci.yml
- tests/nodes/test_node_transition_selector_effect/test_unit.py
- src/omniclaude/nodes/node_transition_selector_effect/node.py
…d model-unavailable tests _resolve_endpoint() is called inside the TimeoutError and Exception handlers to populate the error_detail message. Without LLM_CODER_FAST_URL set in CI, this raises RuntimeError before the structured error result is returned. Add patch.dict(os.environ) to both affected tests.
jonahgabriel
added this pull request to the merge queue
Mar 24, 2026
jonahgabriel
deleted the
jonahgabriel/omn-6095-crenshaw-architecture-review-fixes
branch
March 24, 2026 10:35
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
Address three deficiencies identified in Micheal Crenshaw's external architecture review across 12 tasks in 3 workstreams:
_resolve_endpoint()/_resolve_llm_coder_url()functions that fail fast when env vars are unset. Add CI regression gate (validate_no_hardcoded_ip.py) preventing reintroduction.NODE_BEHAVIOR_MAP.md,PR_VALIDATION_PATH.md, and a canonicalscripts/validate-local.shentrypoint._probe_endpoint()health checking, actionableMODEL_UNAVAILABLEerror messages including endpoint URL and curl remediation, and a prompt purity invariant guard.Tickets: OMN-6096 through OMN-6107 (all Done)
Epic: OMN-6095
Plan:
docs/plans/2026-03-23-crenshaw-review-fixes.md(3 review cycles: R1-R7 adversarial, Codex CLI hostile, external reviewer tightening)Files Changed
src/omniclaude/nodes/node_transition_selector_effect/node.py-- fail-fast endpoint, health probe, actionable errors, prompt puritysrc/omniclaude/nodes/node_transition_selector_effect/contract.yaml-- null default_endpointsrc/omniclaude/lib/utils/consensus/quorum.py-- fail-fast LLM_CODER_URLsrc/omniclaude/hooks/lib/code_context_resolver.py-- fail-fast LLM_EMBEDDING_URLsrc/omniclaude/nodes/node_transition_selector_effect/protocols/protocol_transition_selector.py-- comment cleanupsrc/omniclaude/shared/models/model_skill_completion_event.py-- docstring cleanuptests/nodes/test_node_transition_selector_effect/test_unit.py-- 8 new teststests/nodes/test_node_transition_selector_effect/test_integration.py-- skipif when unsettests/lib/test_quorum.py-- updated for fail-fast behaviorscripts/validation/validate_no_hardcoded_ip.py-- CI regression gatescripts/validate-local.sh-- local validation entrypoint.github/workflows/ci.yml-- added arch-no-hardcoded-ip checkdocs/reference/NODE_BEHAVIOR_MAP.md-- contributor discoverabilitydocs/reference/PR_VALIDATION_PATH.md-- contributor discoverabilityTest plan
validate_no_hardcoded_ip.pyreturns 0 (zero violations in src/)grep -rn "192.168" src/returns zero resultsSummary by CodeRabbit
Documentation
Configuration Changes
Chores
Developer Tools
Tests