Repository navigation
feat(OMN-8765): AST-based deterministic-skill routing CI gate - #888
Merged
Merged
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughAdds deterministic skill routing validation: CI job, pre-commit hook and wrapper, Python CLI scanner for Tier 1 Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Pre as Pre-commit Hook
participant Validator as Validator CLI
participant Skills as Skills Repo
participant CI as GitHub Actions
Dev->>Pre: commit triggers pre-commit
Pre->>Validator: run pre_commit_validate_deterministic_skills.sh
Validator->>Skills: locate skills root (env / sibling / OMNI_HOME)
alt skills root found
Validator->>Skills: scan SKILL.md (AST + shell heuristics)
Validator-->>Pre: report + exit code
else skills root missing
Validator-->>Pre: print "Skipping... gate" and exit 0
end
CI->>Validator: run `check-deterministic-skills` job
Validator-->>CI: write summary to $GITHUB_STEP_SUMMARY
CI->>CI: `quality-gate` requires `check-deterministic-skills` == success
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
🧪 Generate unit tests (beta)
Comment |
jonahgabriel
enabled auto-merge
April 24, 2026 05:59
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
3 tasks
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
795-842: 🛠️ Refactor suggestion | 🟠 Major
contract-complianceis waited on but never enforced byQuality Gate.This block adds
contract-compliancetoneeds, but never readsneeds.contract-compliance.result, never reports it, and never includes it in the pass condition. Because the job usesif: always(),Quality Gatecan still pass and unblock the 40-way test matrix aftercontract-compliancefails.Suggested wiring
steps: - name: Evaluate quality checks run: | @@ secrets="${{ needs.detect-secrets.result }}" sdk_boundary="${{ needs.sdk-boundary-check.result }}" + contract_compliance="${{ needs.contract-compliance.result }}" @@ echo "| Detect Secrets | $secrets |" >> $GITHUB_STEP_SUMMARY echo "| SDK Boundary Guard | $sdk_boundary |" >> $GITHUB_STEP_SUMMARY + echo "| Contract Compliance Check | $contract_compliance |" >> $GITHUB_STEP_SUMMARY @@ [[ "$docs" == "success" ]] && \ [[ "$enum_gov" == "success" ]] && \ [[ "$secrets" == "success" ]] && \ - [[ "$sdk_boundary" == "success" ]]; then + [[ "$sdk_boundary" == "success" ]] && \ + [[ "$contract_compliance" == "success" ]]; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 795 - 842, The workflow adds contract-compliance to needs but never reads or enforces it; define a variable contract_compliance="${{ needs.contract-compliance.result }}" (like the other checks), echo its result into the summary (e.g., "| Contract Compliance | $contract_compliance |"), and include [[ "$contract_compliance" == "success" ]] in the main gate if-condition so the Quality Gate actually blocks when contract-compliance fails; update the variable name and the if-statement near the block that sets lint/pyright/exports/... and the gate comment to ensure consistency.
🤖 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/validate_deterministic_skill_routing.py`:
- Around line 344-358: The SyntaxError branch in the try/except inside the AST
parse currently returns a RoutingViolation (CHECK_PARSE_ERROR) which is counted
by main(); instead, treat parse errors as non-blocking by logging the parse
failure and returning an empty list so it won't increment the violation count.
Update the except SyntaxError as exc handler in the code that calls
ast.parse(block.source) to call the existing logger (or Python logging) with the
same message (including skill_name, skill_path, and exc.lineno) and then return
[] instead of the RoutingViolation instance so main() won't fail on pseudocode
blocks.
- Around line 622-647: The inline dispatch regex (_INLINE_DISPATCH_RE) is too
permissive and treats placeholder text like `onex node <node_name>` as a real
dispatch; update the pattern used by _INLINE_DISPATCH_RE (and adjust
_count_document_dispatches usage if needed) so the token after the command is a
real node identifier (e.g. restrict to a character class like [A-Za-z0-9_.-]+
and forbid angle brackets or literal '<'/'>'/placeholders), rather than matching
any non-whitespace; ensure the regex still allows the optional leading `uv run`
and the variants `onex run-node|node|run` while only counting valid node/topic
names.
- Around line 263-336: The visitor currently only flags calls when the owner
identifier is the literal module name, letting aliasing and from-imports bypass
checks; fix by tracking import aliases and from-imported names (e.g., add
self.import_aliases mapping local_name -> module_root or (module_root,
original_name) inside visit_Import and visit_ImportFrom), then in visit_Call
resolve the true module/name before checking: for Attribute owners resolve
func.value.id via self.import_aliases (fall back to the identifier) and test
(resolved_root, attr) against SUBPROCESS_ORCHESTRATION_CALLS; also handle direct
Name calls (ast.Name as node.func) by resolving the name via self.import_aliases
to detect from-imported functions (e.g., run) and check (resolved_root, name)
against SUBPROCESS_ORCHESTRATION_CALLS; leave the BANNED_LLM_ATTR_CALLS logic
intact but apply the same alias resolution when inner is an ast.Attribute or
ast.Name.
In `@tests/unit/scripts/test_validate_deterministic_skill_routing.py`:
- Around line 26-27: Add a module-level pytest marker by defining the variable
pytestmark = pytest.mark.unit at the top of the test module so the suite under
tests/unit is classified as a unit test; locate the test module (where pytest is
imported) and add the module-level variable pytestmark referencing
pytest.mark.unit.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 795-842: The workflow adds contract-compliance to needs but never
reads or enforces it; define a variable contract_compliance="${{
needs.contract-compliance.result }}" (like the other checks), echo its result
into the summary (e.g., "| Contract Compliance | $contract_compliance |"), and
include [[ "$contract_compliance" == "success" ]] in the main gate if-condition
so the Quality Gate actually blocks when contract-compliance fails; update the
variable name and the if-statement near the block that sets
lint/pyright/exports/... and the gate comment to ensure consistency.
🪄 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: 6b581634-7298-479b-a959-740590c7e9e2
📒 Files selected for processing (5)
.github/workflows/ci.yml.pre-commit-config.yamlscripts/pre_commit_validate_deterministic_skills.shscripts/validate_deterministic_skill_routing.pytests/unit/scripts/test_validate_deterministic_skill_routing.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/validate_deterministic_skill_routing.py (1)
510-513:⚠️ Potential issue | 🟠 MajorReject placeholder shell dispatches like
onex node <node_name>.
_line_is_dispatch()currently treats any third token as a valid dispatch target, so a fenced bash example/placeholder still satisfies the contract. The inline scanner already rejects placeholders, but shell blocks can still bypass the gate.Suggested fix
+_NODE_IDENTIFIER_RE = re.compile(r"^[A-Za-z0-9_][A-Za-z0-9_.-]*$") + def _line_is_dispatch(tokens: tuple[str, ...], raw: str) -> bool: @@ if head_tokens[0] == "onex" and len(head_tokens) >= 3: subcmd = head_tokens[1] - if subcmd in {"run", "run-node", "node"}: + target = head_tokens[2] + if ( + subcmd in {"run", "run-node", "node"} + and _NODE_IDENTIFIER_RE.fullmatch(target) + ): return True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/validate_deterministic_skill_routing.py` around lines 510 - 513, The code in _line_is_dispatch() currently treats any third token as a valid dispatch target (head_tokens[2]), allowing placeholder examples like "onex node <node_name>" to pass; update the check after subcmd extraction (head_tokens, subcmd) to validate the dispatch target is a real identifier by rejecting tokens that look like placeholders (e.g., start with "<" and end with ">" or contain angle brackets) or otherwise clearly non-identifiers; specifically, when subcmd in {"run","run-node","node"} ensure the candidate token (head_tokens[2]) does not match a placeholder pattern like r'^<.*>$' (or contain '<' or '>') before returning True.
🤖 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/validate_deterministic_skill_routing.py`:
- Around line 289-319: The current visit_ImportFrom method only checks
node.module roots so a statement like "from google import generativeai" isn't
recognised; update visit_ImportFrom to also inspect each alias (alias.name /
alias.asname) and construct the full import root (e.g.
f"{node.module}.{alias.name}") or check alias.name against banned SDK spellings,
then call _banned_module_root on that combined name and, if banned, append a
RoutingViolation (same shape as the existing block using self.violations,
RoutingViolation, CHECK_LLM_IMPORT and self._abs_line(node.lineno)); keep the
existing subprocess/os alias handling intact and ensure generic_visit(node) is
still called.
---
Duplicate comments:
In `@scripts/validate_deterministic_skill_routing.py`:
- Around line 510-513: The code in _line_is_dispatch() currently treats any
third token as a valid dispatch target (head_tokens[2]), allowing placeholder
examples like "onex node <node_name>" to pass; update the check after subcmd
extraction (head_tokens, subcmd) to validate the dispatch target is a real
identifier by rejecting tokens that look like placeholders (e.g., start with "<"
and end with ">" or contain angle brackets) or otherwise clearly
non-identifiers; specifically, when subcmd in {"run","run-node","node"} ensure
the candidate token (head_tokens[2]) does not match a placeholder pattern like
r'^<.*>$' (or contain '<' or '>') before returning True.
🪄 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: cceeb347-2c03-49d7-be65-74767c99f744
📒 Files selected for processing (2)
scripts/validate_deterministic_skill_routing.pytests/unit/scripts/test_validate_deterministic_skill_routing.py
jonahgabriel
disabled auto-merge
April 24, 2026 13:11
jonahgabriel
enabled auto-merge
April 24, 2026 13:11
Contributor
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/validate_deterministic_skill_routing.py`:
- Around line 438-467: In _tokenize_shell(), the continuation buffer
(buffer/buffer_start) can remain non-empty if the last physical line ends with a
backslash, dropping that logical line; after the for loop finishes but before
returning result, check if buffer is non-empty and construct the final logical
line exactly like inside the loop (join segments, compute start_rel from
buffer_start, reset buffer), then tokenize and append a ShellLine to result (use
block.start_line + start_rel, raw=logical, tokens as in the try/except). Ensure
this mirrors the existing tokenization flow so final continued lines are not
lost.
- Around line 565-567: The prose-fallback check in _is_prose_fallback reads head
= line.tokens[0] and thus misses commands prefixed by environment assignments
(e.g., FOO=1 echo ...); update _is_prose_fallback to skip leading tokens that
match environment variable assignments (tokens like NAME=VALUE) and choose the
first non-assignment token as head before checking membership in
_PROSE_FALLBACK_VERBS, taking care to handle the case where all tokens are
assignments or tokens is empty.
- Around line 542-552: The current early-return only looks for the literal
substrings "onex run" / "onex node" in raw, so quoted/argv forms like
"['onex','run-node',...]" or hyphen/underscore variants bypass it; update the
check that inspects raw (the lowered variable) to detect "onex" plus any
run/node variant as separate argv forms or quoted list entries — e.g., require
"onex" in lowered AND any of [" run", " run-", " run_", "run-node", "run_node",
" node", "['onex'", '"onex' ] (or use a small regex to match onex followed later
by \b(run|run-node|run_node|node)\b or quoted/array patterns) — while keeping
wrapper_prefixes and the python/-c detection logic intact (look for these
patterns before returning True).
- Around line 520-523: The current dispatch detection treats commands like "onex
run-node --help" or "onex run-node -v" as valid targets; update the branch that
checks onex_tokens and subcmd to also validate the target token (onex_tokens[2])
is a real node/name instead of an option/placeholder: after checking subcmd in
{"run","run-node","node"}, read target = onex_tokens[2] and only return True if
target is non-empty and does not start with '-' and is not a help flag (e.g.,
not in {'--help','-h'}); use the existing onex_tokens and subcmd symbols so the
check sits next to the current logic.
🪄 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: 4d8e9ded-2915-4ea7-bf0f-bf44d8eabc41
📒 Files selected for processing (2)
pyproject.tomlscripts/validate_deterministic_skill_routing.py
✅ Files skipped from review due to trivial changes (1)
- pyproject.toml
jonahgabriel
disabled auto-merge
April 24, 2026 16:36
jonahgabriel
enabled auto-merge
April 24, 2026 16:36
Add a CI lint gate (`check-deterministic-skills`) that blocks merge when any Tier 1 deterministic skill in `omniclaude` violates the SkillRoutingError routing contract. Replaces line-count heuristics with AST analysis over embedded python blocks and structural token-matching over shell blocks, per amendment A4 on OMN-8765. What the validator enforces per Tier 1 skill: - Zero LLM SDK imports (anthropic / openai / google.generativeai) - No banned LLM API calls (client.messages.create, client.completions.create) - No subprocess orchestration calls in python blocks - No `python -c` / `bash -c` wrappers around the dispatch - No conditional prose-fallback branches after routing failure - At least one node-dispatch declaration (onex run-node / onex node / Kafka publish to `onex.cmd.*`) — multi-line backslash continuations merged before tokenizing - SKILL.md references `SkillRoutingError` with `do not produce prose` Wiring: - `scripts/validate_deterministic_skill_routing.py` — AST validator, with auto-discovery of the omniclaude sibling clone via `$OMNI_HOME` or `$DETERMINISTIC_SKILL_ROOT` env var. - `scripts/pre_commit_validate_deterministic_skills.sh` — pre-commit wrapper that skips silently when the omniclaude sibling is absent. - `.github/workflows/ci.yml` — new blocking job `check-deterministic-skills` (Phase 1) that clones omniclaude and runs the validator; added to the `quality-gate` aggregator's evaluation. - `.pre-commit-config.yaml` — `validate-deterministic-skill-routing` hook. - `tests/unit/scripts/test_validate_deterministic_skill_routing.py` — 18 unit tests covering positive path + every structural violation category. All 18 current Tier 1 deterministic skills in omniclaude pass locally. Test suite (`tests/unit/scripts/`) runs 623/623 green; mypy --strict and ruff clean on both new files; pre-commit full run passes.
Use actions/setup-python with env.PYTHON_VERSION (3.12) to match the version pinned in pyproject.toml instead of relying on the runner's default python3, mirroring the setup used in all other Phase 1 validation jobs.
…R body PR body updated to eliminate bare OMN-8737 / OMN-8749 token matches (non-breaking hyphens in parent-epic / predecessor references so the receipt-gate ticket regex \bOMN-\d+\b only matches OMN-8765).
Addresses 5 unresolved review threads on omnibase_core#888: 1. CodeQL: remove unused MISSING_NODE_SKILLS global (and its doc reference) 2. Alias-aware subprocess detection — previously only `subprocess.run(...)` with literal `subprocess` identifier was flagged; now also trips on `import subprocess as sp; sp.run(...)`, `from subprocess import run; run(...)`, and the `os` equivalents. Banned-names frozensets extracted to shared constants to keep the alias table and tuple pairs in sync. 3. Parse errors are non-blocking — unparseable Python blocks (common in pseudocode examples) now log via `logging` and return `[]` instead of emitting a CHECK_PARSE_ERROR violation that main() counts and fails on. 4. Dispatch regex rejects the boilerplate placeholder `onex node <node_name>` — was previously counting the generic routing-contract sentence as a valid dispatch declaration, letting skills satisfy the gate without naming any real node. Target must now be a real identifier (`[A-Za-z0-9_][A-Za-z0-9_.\-]*`) — no angle brackets. 5. Tighten `_PUBLISH_DECLARATION_RE` to recognise `publishes` / `published` / `emits` / `emitted` — the `\b` after bare `publish` failed on `publishes` because `s` is a word char. This was a latent bug exposed by fix #4 (the inline placeholder was previously masking the prose-publish gap in the Kafka-skill test fixture). 6. Add `pytestmark = pytest.mark.unit` to the unit test module per repo convention. Test coverage: - test_flags_subprocess_orch_via_aliased_import - test_flags_subprocess_orch_via_from_import - test_flags_os_system_via_from_import - test_unparseable_python_block_is_not_blocking - test_placeholder_inline_dispatch_does_not_satisfy_contract - test_real_inline_dispatch_satisfies_contract All 24 tests green; mypy --strict clean on both files.
…ort, unused constant Three fixes to the AST-based deterministic-skill routing gate: 1. _line_is_dispatch: accept 'uv run onex run-node X' shell prefix so skills using the uv wrapper are not incorrectly flagged as missing a dispatch declaration (was root cause of merge_sweep violation). 2. visit_ImportFrom: catch 'from google import generativeai' by constructing the full module.alias path and checking _banned_module_root on it — previously only the module root 'google' was checked, which is not in BANNED_LLM_MODULES (only 'google.generativeai' is). 3. Remove SUBPROCESS_ORCHESTRATION_CALLS frozenset — became unused after the alias-tracking rewrite in the prior commit; CodeQL flagged it. 4. pyproject.toml: add T201 per-file-ignore for the validator script so ruff does not flag intentional CLI print() output.
…eclaration Some deterministic skills (e.g. session) intentionally keep the canonical `onex run-node` command in prompt.md rather than SKILL.md to avoid double- counting in per-skill audit gates that scan all files in the skill directory. The validator now checks prompt.md as a secondary source when SKILL.md yields zero dispatch evidence.
- _tokenize_shell: flush continuation buffer at EOF so backslash-terminated final lines are not silently dropped - _line_is_dispatch: reject placeholder/option targets (<node_name>, --help) so boilerplate routing-contract sentences don't satisfy the dispatch check - _is_wrapped_dispatch: broaden early-exit guard to catch quoted argv forms like ['onex','run-node',...] via regex instead of literal substring match - _is_prose_fallback: skip leading env-assignment tokens (FOO=1 echo ...) before checking the command verb against _PROSE_FALLBACK_VERBS
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/validate_deterministic_skill_routing.py (1)
916-916: Consider movingimport osto module level.The
osimport inside_default_skills_root()is unconventional. Sinceosis a standard library module with minimal overhead, placing it at the module level with other imports improves readability.♻️ Suggested change
Add to the import block at the top:
import sys from dataclasses import dataclass, field from pathlib import Path +import osThen remove the local import:
def _default_skills_root() -> Path: ... - import os - env = os.environ.get("DETERMINISTIC_SKILL_ROOT")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/validate_deterministic_skill_routing.py` at line 916, The local import of os inside _default_skills_root() should be moved to the module-level import block; remove the inline "import os" from the _default_skills_root() function and add a single "import os" with the other top-of-file imports so the function simply uses os.path.join/os.environ etc. without an internal import.
🤖 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/validate_deterministic_skill_routing.py`:
- Around line 496-501: The banner tuple contains a duplicated entry ("example:")
— update the tuple used in the startswith check (the one iterating over
("usage:", "example:", "example:", "run via:", "see ", "see:")) to remove the
duplicate "example:" (or replace the tuple with an explicit deduplicated
collection) so the loop over banners uses unique values; the change should be
made near the code that computes lowered = raw.lower().lstrip() and then
iterates over the banners for the startswith checks.
---
Nitpick comments:
In `@scripts/validate_deterministic_skill_routing.py`:
- Line 916: The local import of os inside _default_skills_root() should be moved
to the module-level import block; remove the inline "import os" from the
_default_skills_root() function and add a single "import os" with the other
top-of-file imports so the function simply uses os.path.join/os.environ etc.
without an internal import.
🪄 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: 9bc23f55-e5f9-446c-9d09-a65306ba2b0a
📒 Files selected for processing (1)
scripts/validate_deterministic_skill_routing.py
jonahgabriel
force-pushed
the
jonah/omn-8765-skill-routing-gate
branch
from
April 25, 2026 05:23
9665355 to
4577917
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/validate_deterministic_skill_routing.py (1)
130-130: Minor:CHECK_PARSE_ERRORconstant is defined but unused.Since parse errors are now logged and skipped (non-blocking), this constant is never emitted. Consider removing it to avoid confusion.
♻️ Suggested removal
CHECK_PROSE_FALLBACK = "PROSE_FALLBACK_BRANCH" CHECK_MISSING_ROUTING_ERROR = "MISSING_SKILL_ROUTING_ERROR" -CHECK_PARSE_ERROR = "CODE_BLOCK_PARSE_ERROR"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/validate_deterministic_skill_routing.py` at line 130, The constant CHECK_PARSE_ERROR is defined but no longer used; remove the unused symbol CHECK_PARSE_ERROR (the top-level constant assignment) from scripts/validate_deterministic_skill_routing.py and delete any stray references or TODOs that mention it so the codebase doesn't contain confusing dead constants; ensure there are no remaining imports or usages of CHECK_PARSE_ERROR after removal.
🤖 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/validate_deterministic_skill_routing.py`:
- Line 130: The constant CHECK_PARSE_ERROR is defined but no longer used; remove
the unused symbol CHECK_PARSE_ERROR (the top-level constant assignment) from
scripts/validate_deterministic_skill_routing.py and delete any stray references
or TODOs that mention it so the codebase doesn't contain confusing dead
constants; ensure there are no remaining imports or usages of CHECK_PARSE_ERROR
after removal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6f0fa8a8-2bda-470b-86dc-252bc683a814
📒 Files selected for processing (6)
.github/workflows/ci.yml.pre-commit-config.yamlpyproject.tomlscripts/pre_commit_validate_deterministic_skills.shscripts/validate_deterministic_skill_routing.pytests/unit/scripts/test_validate_deterministic_skill_routing.py
🚧 Files skipped from review as they are similar to previous changes (2)
- pyproject.toml
- scripts/pre_commit_validate_deterministic_skills.sh
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
2 participants
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
Adds the W3 hard-gate CI lint check from OMN-8765 (parent epic:
OMN‑8737). New jobcheck-deterministic-skillsblocks merge if any Tier 1 deterministic skill inomniclaudeviolates the SkillRoutingError routing contract, using AST analysis (python blocks) and structural token-matching (shell blocks) instead of the line-count heuristics theOMN‑8749predecessor used.What the validator enforces per Tier 1 skill
anthropic,openai,google.generativeai)client.messages.create,client.completions.create,...chat.completions)subprocess.run,subprocess.Popen,os.system, …)python -c/bash -c/sh -cwrappers around the dispatchonex run-node/onex nodein a shell block or inline-code, OR a Kafka publish to anonex.cmd.*topic). Multi-line backslash continuations are merged before tokenizing.SkillRoutingErrorwithdo not produce proseSee the module docstring for the full amendment-A4 mapping. The ticket's "exactly one dispatch" phrasing is interpreted as "at least one, unambiguous" because Kafka-publish skills declare dispatch as a payload schema, not an imperative shell command.
Files
scripts/validate_deterministic_skill_routing.py— AST validator (mypy --strict clean, ~900 lines)scripts/pre_commit_validate_deterministic_skills.sh— pre-commit wrapper that skips silently when the omniclaude sibling is absenttests/unit/scripts/test_validate_deterministic_skill_routing.py— 18 unit tests (clean skill passes + every negative path).github/workflows/ci.yml— new blocking Phase 1 jobcheck-deterministic-skills; added toquality-gateaggregator's evaluation.pre-commit-config.yaml—validate-deterministic-skill-routinghookVerification
uv run pytest tests/unit/scripts/ -q→ 623 passed (18 new)uv run mypy --strict scripts/validate_deterministic_skill_routing.py→ cleanuv run mypy --strict tests/unit/scripts/test_validate_deterministic_skill_routing.py→ cleanuv run ruff format/ruff check src/ tests/→ cleanpre-commit runon all changed files → all hooks green, including the new OMN-8765 hookpython scripts/validate_deterministic_skill_routing.py --skills-root /…/omniclaude/plugins/onex/skills→OK — 18 Tier 1 skill(s) scanned, 0 violationsAll 18 current Tier 1 deterministic skills in omniclaude pass the new gate, so merging this PR activates the blocking gate cleanly.
DoD Evidence (from OMN-8765)
check-deterministic-skillsblocks merge if any deterministic shim lacks dispatch / contains LLM SDK import / contains subprocess orchestration / contains conditional prose fallbackTest plan
check-deterministic-skillsjob turns green on this PRquality-gateaggregator passes with the new gate includedimport anthropicto a Tier 1 SKILL.md fails the gate (regression coverage from unit test suite)🔗 Linear: OMN-8765
Summary by CodeRabbit
Chores
New Features
Tests