Repository navigation
fix(OMN-15430): shlex-aware shape-guard tokenization - #1958
Conversation
_invalid_check_value_reason (the OMN-15382 pre-execution prose guard) tokenized check_value with a naive cmd_str.strip().split() after stripping a leading VAR= token. On the OMN-15170 sigpipe-safe shape (OMN-15411) -- body="$(gh api ... | base64 -d)" && printf '%s' "$body" | grep -qF '<marker>' -- the naive split breaks apart inside the quoted command substitution and inspects an inner word (api) as the first token, producing a false INVALID_CHECK_VALUE_NOT_A_COMMAND on a check that runs correctly under bash -o pipefail -c. Tokenizes with shlex.split(cmd_str, posix=True) instead: quote-respecting, so the whole quoted substitution stays one token. Strips leading VAR=VAL assignment tokens AND leading shell control operators (&&, ||, ;, |, ...) before resolving the first real command token -- the OMN-15170 shape's first shlex token is the whole body=$(...) assignment, the next is &&, and only the token after that (printf) is the real command. On a genuine shlex parse failure (unbalanced quotes) the guard stays fail-closed INVALID -- a check_value bash itself cannot parse is genuinely invalid. RED-first proof: the pre-fix naive-split guard (checked out from origin/dev HEAD before this diff) rejects the exact OMN-15170 check_value with first token 'api' is not a resolvable executable -- confirmed via git show HEAD:.../evidence_collector.py exec'd in isolation. Post-fix the same string resolves cleanly (no INVALID reason). Non-regression: bare prose (Recorded product receipt: ...) still INVALID; first-token-ends-with-colon still INVALID; ./script.sh + declared cwd (the round-1 OMN-15382 fix) still resolves; a genuinely unparseable string (unbalanced quote) still INVALID; leading VAR=VAL assignment stripping unchanged; a leading-operator-only command with no real command token stays fail-closed INVALID. 267/267 node_dod_verify tests pass, ruff format/check clean, mypy --strict clean, pre-commit clean (OMNIBASE_INFRA_PATH set for topic-naming-lint). Closes OMN-15430.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
✅ Hostile Reviewer — PASSEDBlocking findings (critical/error): 0 Gate semantics
Powered by omniintelligence.review_pairing.cli_review — node-based adversarial review via HandlerLlmCliSubprocess (OMN-8524) |
|
| Rule | Description |
|---|---|
| ARCH-TOPIC-001 | No hardcoded Kafka topic strings in handler code |
| ARCH-DI-001 | No event_bus=None bypass |
| ARCH-DI-002 | No direct Handler instantiation outside workflow_runner/adapters |
| ARCH-DI-003 | No reinvented DI containers |
| ARCH-TOPIC-002 | contract.yaml topics follow `onex.{cmd |
ARCH-DI-002: Direct Handler instantiation (warning)\n\n```\nsrc/omnimarket/nodes/node_dod_verify/services/evidence_collector.py:1196: output = HandlerDodEvidenceGithubEffect().handle(command)
src/omnimarket/nodes/node_dod_verify/services/evidence_collector.py:1232: output = HandlerDodEvidenceGithubEffect().handle(command)
src/omnimarket/nodes/node_dod_verify/services/evidence_collector.py:1692: output = HandlerDodEvidenceGithubEffect().handle(command)
src/omnimarket/nodes/node_dod_verify/services/evidence_collector.py:1718: output = HandlerDodEvidenceGithubEffect().handle(command)\n```\n\nHandler classes should be wired only in workflow_runner.py or adapter modules. Direct instantiation elsewhere bypasses the contract system.\n\n
Static architectural lint — no model inference (OMN-14176).
…ure literal The new test embeds the real OMN-15170/OMN-15411 check_value string verbatim (required for a faithful shape-guard regression proof), which contains the jonahgabriel/steel_onslaught path the Leaked Literals Gate (OMN-10580) flags. Adds a ticketed onex-allow-test-fixture annotation on the same line, per the gate's documented allowlist mechanism.
…5516) * evidence: OCC companion pass 1 for OmniNode-ai/omnimarket#1958 * evidence: OCC companion self-bind for #5516 * fix(OMN-15430): format OCC companion receipts --------- Co-authored-by: node-occ-companion-effect <occ-companion-effect@omninode.ai> Co-authored-by: Jonah Gray <jonah@omninode.ai>
…rsession (#5543) * fix(OMN-15430): literal-pin the deploy-scope DoD via append-only supersession contracts/OMN-15430.yaml dod-deploy-assessment carried bare runner-substituted PR_NUMBER/REPO placeholder tokens under an id embedding no pr-<N> segment. The id does not match node_dod_verify _EVIDENCE_ID_BINDING_RE, so _repo_and_pr_from_evidence_id yields no binding, _lookup_pr_for_ticket has no repo to scope a PR search to, and the item hard-REDs at _resolve_command_placeholders with PR_LOOKUP_FAILED before the command is ever shelled out. Reproduced on three independent dod_verify runs; it is the single red blocking the OMN-15430 flip. Supersedes that entry (append-only, supersedes_dod_evidence idiom per OMN-14875/OMN-15382, precedent OCC#5496/#5523) with dod-deploy-assessment-pr-1958-literal-pin: same deploy-scope path regex, literally pinned to OmniNode-ai/omnimarket#1958 with a literal --repo and no runner-substituted token, SIGPIPE-safe (producer drained into a shell variable before any grep reads), shlex-parseable, fail-closed on producer failure. Proven by execution: GREEN 3/3 under bash -o pipefail -c, 1/1 under sh -c; RED under three mutations (absent anchor path, repointed at OCC#5516, nonexistent repo). * evidence(OMN-15430): PASS receipts + self-bind for the literal-pinned deploy DoD Adds the OCC self-bind entry occ-self-bind-pr-5543 (the occ-preflight eligibility gate requires a PASS receipt binding OMN-15430 to this PR) and executed-probe PASS receipts for it and for dod-deploy-assessment-pr-1958-literal-pin. Both receipts were minted by executing the declared check_value live on the gate host under bash -o pipefail -c, not hand-authored: exit 0 for both. contract_entry_sha256 bindings verified against compute_contract_entry_sha256() post-yamlfmt (unchanged across the append, per OMN-13888/OMN-14411).
1 participant
Summary
Fixes
_invalid_check_value_reasoninsrc/omnimarket/nodes/node_dod_verify/services/evidence_collector.py(OMN-15430). The OMN-15382 pre-execution "prose vs command" shape guard tokenizedcheck_valuewith a naivecmd_str.strip().split()after stripping a leadingVAR=token. On the OMN-15170 sigpipe-safe shape (OMN-15411):the naive split breaks apart inside the quoted
$(...)command substitution and inspects an inner word (api) as the "first token", producing a falseINVALID_CHECK_VALUE_NOT_A_COMMANDon a check that runs correctly (bash -o pipefail -c '<same string>'exits 0). This was the single blocker keepingdod_verify OMN-15170from a clean table.Fix
shlex.split(cmd_str, posix=True)— quote-respecting, so the whole quoted substitution stays one token.VAR=VALassignment tokens and leading shell control operators (&&,||,;,;;,|,&) before resolving the first real command token. The OMN-15170 shape's first shlex token is the wholebody=$(...)assignment; the next is&&; only the token after that (printf) is the real command.check_valuebash itself cannot parse is genuinely invalid, never silently passed through.RED-first proof
Checked out the pre-fix source at
origin/devHEAD (git show HEAD:.../evidence_collector.py), exec'd the naive-split guard in isolation, and confirmed it rejects the exact OMN-15170check_valuewithfirst token 'api' is not a resolvable executable.... Post-fix, the same string resolves cleanly (_invalid_check_value_reasonreturnsNone).Non-regression
Recorded product receipt: ...) stillINVALIDINVALID./script.sh+ declaredcwd:(the round-1 OMN-15382 fix) still resolvesINVALIDVAR=VALassignment stripping unchangedINVALIDTest plan
tests/unit/nodes/node_dod_verify/test_omn_15430_shlex_shape_guard.py(10 tests) covering the OMN-15170 shape acceptance + all non-regression cases abovetests/unit/nodes/node_dod_verify/suite: 267/267 passedruff format --check/ruff checkclean on touched filesmypy --strictclean onevidence_collector.pypre-commit runclean on touched files (run on.200withOMNIBASE_INFRA_PATHset fortopic-naming-lint)Cites OMN-15430. Related: OMN-15382 (guard origin), OMN-15411 (the OMN-15170 sigpipe-safe rewrite this unblocks), OMN-15170 (the blocked flip).
Evidence-Source: OCC#5516
Evidence-Ticket: OMN-15430