Repository navigation
feat(OMN-10580): add CI grep gate for leaked literals with allowlist governance - #547
Merged
jonahgabriel merged 10 commits intoMay 7, 2026
Conversation
…governance - Harden check_leaked_literals.sh from advisory to blocking mode - Add onex-allow-model-id annotation type for private HuggingFace model IDs - Remove block_always path policy; annotations now honoured in all paths including src/ - Add file-level exemption (#onex-allow-file) for test fixtures with many deliberate occurrences - Add leaked-literals-gate as pre-commit hook (blocking diff scope) - Rewrite reject-leaked-literals.yml GHA workflow as required status check "Leaked Literals Gate" - Annotate all src/ defaults with onex-allow-* inline annotations (OMN-10580) - Add onex-allow-file markers to all test fixtures containing deliberate lab literals - Add .leaked-literals-allowlist.yaml audit trail registry - Add docs/leaked-literals-governance.md with escape-hatch reference - Update test_leak_gate.py: src/ annotations now permitted (env-var fallback pattern)
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR implements an end-to-end leaked-literals governance: policy and contract, a blocking validation script with line- and file-level exemptions, a machine-readable allowlist, pre-commit and CI blocking gates, inline allowlist annotations across code and configs, environment-fallback refactors, and expanded tests. ChangesLeaked Literals Gate Enforcement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 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 unit tests (beta)
Comment |
|
| Verdict | Meaning | Blocks merge? |
|---|---|---|
passed |
No critical/error findings | No |
blocked |
CRITICAL or ERROR findings found | Yes |
degraded |
All models unavailable (infra) | No (pilot) |
Powered by omniintelligence.review_pairing.cli_review — node-based adversarial review via HandlerLlmCliSubprocess (OMN-8524)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
src/omnimarket/nodes/node_doc_freshness_sweep/handlers/handler_doc_freshness_sweep.py (1)
155-160: ⚡ Quick winPrefer a user-agnostic default over a personal absolute path.
Using
"/Users/jonah/Code/omni_home"as the built-in fallback is brittle and environment-specific. A home-relative fallback keeps local convenience without hardcoding one developer path.♻️ Proposed change
omni_home = ( request.omni_home or os.environ.get( "OMNI_HOME", - "/Users/jonah/Code/omni_home", # onex-allow-local-path OMN-10580 reason="OMNI_HOME env-var fallback; OMNI_HOME must be set in any non-dev environment" + str(Path.home() / "Code" / "omni_home"), # onex-allow-local-path OMN-10580 reason="OMNI_HOME env-var fallback; OMNI_HOME must be set in any non-dev environment" ) )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/omnimarket/nodes/node_doc_freshness_sweep/handlers/handler_doc_freshness_sweep.py` around lines 155 - 160, The fallback for omni_home currently hardcodes a personal absolute path; update the fallback in the omni_home assignment (in handler_doc_freshness_sweep.py where omni_home is computed) to a user-agnostic, home-relative default such as os.path.expanduser("~/.omni_home") (or similar) instead of "/Users/jonah/Code/omni_home", so that request.omni_home or OMNI_HOME env var is still honored but local fallback works across users and environments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/pr-arch-review.yml:
- Line 26: The workflow sets bot_login to a hardcoded username (bot_login) that
doesn't match the actor of the token at secrets.GITHUB_TOKEN; update the
bot_login value to the actual GitHub Actions actor string used by that token
(github-actions[bot]) so comment upsert matching uses the same actor as the
token, i.e., change the bot_login key referenced near the secrets.GITHUB_TOKEN
usage to "github-actions[bot]".
In @.github/workflows/pr-review-bot.yml:
- Line 89: The workflow hardcodes bot_login to "jonahgabriel" which mismatches
the fallback token actor; change the assignment so bot_login is set from the
workflow actor (the identity that will use the ${GITHUB_TOKEN}) by replacing the
literal with the runtime actor variable (use github.actor) so the fallback token
and bot_login match and prevent upsert/duplication issues.
In `@scripts/projection_api_server.py`:
- Around line 70-73: _dsn() currently hardcodes the host/port so POSTGRES_HOST /
POSTGRES_PORT cannot override it; update the _dsn function to read POSTGRES_HOST
and POSTGRES_PORT (e.g., via os.environ.get with the current lab IP/port as
defaults), read POSTGRES_PASSWORD as before, and build the DSN string from those
variables (and the existing database name and user) so environment overrides
work as intended; reference the _dsn function and the
POSTGRES_HOST/POSTGRES_PORT/POSTGRES_PASSWORD env names when making the change.
In `@scripts/validation/check_leaked_literals.sh`:
- Around line 87-90: The current ALLOWLIST_REGEX variable accepts any
onex-allow-* annotation regardless of the actual leaked literal class, weakening
typed exemptions; update ALLOWLIST_REGEX so it enforces that the allowlist token
suffix (internal-ip|local-path|test-fixture|raw-env|model-id) must match the
leak-class being checked (i.e., build a regex that captures the specific leak
class and only accepts the exact onex-allow-<same-class> token on the same
line), adjust the pattern used in check_leaked_literals.sh (symbol:
ALLOWLIST_REGEX) accordingly, and ensure the same change is applied to the other
occurrence referenced in the script (the block at the later lines mentioned).
- Around line 152-158: Summary: The FILE_LEVEL_EXEMPT_REGEX currently matches
the token anywhere in a file; anchor it so only real directive comment lines
(not occurrences inside strings or examples) skip the file. Fix: update
FILE_LEVEL_EXEMPT_REGEX used by the grep -qE check to require the directive
appear on its own comment line by anchoring the pattern to the start of line and
allowing optional leading whitespace and a single '#' comment marker (e.g.
change to a pattern like start-of-line, optional spaces, '#', optional space,
'onex-allow-file', then the OMN- number and reason), leaving the rest of the
grep -qE -- "${f}" logic intact so files are skipped only when a true directive
line exists.
In
`@src/omnimarket/nodes/node_baseline_capture/handlers/probes/probe_system_health.py`:
- Around line 27-29: The hardcoded Redpanda admin URL
("http://192.168.86.201:9644/v1/cluster/health") in probe_system_health.py
contradicts the inline note about REDPANDA_ADMIN_HOST and can target the lab
host incorrectly; update the construction of the Redpanda probe entry (the tuple
containing "redpanda") to read os.environ.get("REDPANDA_ADMIN_HOST") (or
similar) and build the full health URL from that value with the original URL as
the default fallback, e.g. get host = os.environ.get("REDPANDA_ADMIN_HOST",
"http://192.168.86.201:9644") then use f"{host}/v1/cluster/health" when creating
the tuple so the env override works while preserving the default.
In `@src/omnimarket/nodes/node_handoff_effect/handlers/handler_handoff_effect.py`:
- Line 23: _SSH_HOST is hardcoded despite the comment promising an
ONEX_INFRA_SSH_TARGET env-var override; change its definition to read from the
environment (e.g., _SSH_HOST = os.environ.get("ONEX_INFRA_SSH_TARGET",
"jonah@192.168.86.201")) so the handler honors the override, matching the
pattern used in handler_platform_readiness.py (os is already imported).
In `@src/omnimarket/nodes/node_ticket_work/handlers/handler_ticket_work.py`:
- Line 68: The code documents that _CANONICAL_WORKTREES is overridable via the
OMNI_WORKTREES env var but run_review() always uses the hardcoded
_CANONICAL_WORKTREES; update run_review() to read
os.environ.get("OMNI_WORKTREES") (or equivalent config loader) and use that
value if set, falling back to _CANONICAL_WORKTREES, or alternatively update the
annotation text to remove the override claim—modify the logic in the run_review
function (and any callers that reference _CANONICAL_WORKTREES directly) so the
environment override is actually applied.
---
Nitpick comments:
In
`@src/omnimarket/nodes/node_doc_freshness_sweep/handlers/handler_doc_freshness_sweep.py`:
- Around line 155-160: The fallback for omni_home currently hardcodes a personal
absolute path; update the fallback in the omni_home assignment (in
handler_doc_freshness_sweep.py where omni_home is computed) to a user-agnostic,
home-relative default such as os.path.expanduser("~/.omni_home") (or similar)
instead of "/Users/jonah/Code/omni_home", so that request.omni_home or OMNI_HOME
env var is still honored but local fallback works across users and environments.
🪄 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: e056658b-76a9-4770-82f5-d9e8895ae038
📒 Files selected for processing (69)
.github/workflows/auto-merge.yml.github/workflows/linear-close-on-merge.yml.github/workflows/pr-arch-review.yml.github/workflows/pr-review-bot.yml.github/workflows/reject-leaked-literals.yml.leaked-literals-allowlist.yaml.pre-commit-config.yamldocker-compose.projection.ymldocs/leaked-literals-governance.mddocs/work-tracking/contracts/OMN-10580.yamlscripts/ci/check_entry_point_format.pyscripts/cost_event_publisher.pyscripts/install_codex_skills.pyscripts/projection_api_server.pyscripts/run_delegation_cost_projection_process.shscripts/run_merge_sweep_phase2_proof.shscripts/validation/check_leaked_literals.shsrc/omnimarket/experiments/adk_eval/type_debt_scout_poc/__main__.pysrc/omnimarket/experiments/adk_eval/type_debt_scout_poc/handler_type_debt_scout.pysrc/omnimarket/inference/bridge_config_loader.pysrc/omnimarket/nodes/node_ab_compare_orchestrator/models_registry.yamlsrc/omnimarket/nodes/node_ab_inference_effect/models/model_inference_request.pysrc/omnimarket/nodes/node_aislop_sweep/__main__.pysrc/omnimarket/nodes/node_baseline_capture/handlers/probes/probe_kafka_topics.pysrc/omnimarket/nodes/node_baseline_capture/handlers/probes/probe_system_health.pysrc/omnimarket/nodes/node_build_loop_orchestrator/assemble_live.pysrc/omnimarket/nodes/node_ci_fix_effect/contract.yamlsrc/omnimarket/nodes/node_compliance_sweep/__main__.pysrc/omnimarket/nodes/node_database_sweep/handlers/handler_database_sweep.pysrc/omnimarket/nodes/node_dispatch_queue_drainer/handlers/handler_dispatch_queue_drainer.pysrc/omnimarket/nodes/node_doc_freshness_sweep/handlers/handler_doc_freshness_sweep.pysrc/omnimarket/nodes/node_duplication_sweep/handlers/handler_duplication_sweep.pysrc/omnimarket/nodes/node_handoff_effect/handlers/handler_handoff_effect.pysrc/omnimarket/nodes/node_nightly_loop_controller/models/model_nightly_loop.pysrc/omnimarket/nodes/node_platform_diagnostics/handlers/dimension_checks.pysrc/omnimarket/nodes/node_platform_readiness/handlers/dimension_checks.pysrc/omnimarket/nodes/node_platform_readiness/handlers/handler_platform_readiness.pysrc/omnimarket/nodes/node_pr_review_bot/contract.yamlsrc/omnimarket/nodes/node_projection_llm_cost/consumer.pysrc/omnimarket/nodes/node_runtime_sweep/__main__.pysrc/omnimarket/nodes/node_session_orchestrator/handlers/handler_session_orchestrator.pysrc/omnimarket/nodes/node_thread_reply_effect/handlers/handler_thread_reply.pysrc/omnimarket/nodes/node_ticket_work/handlers/handler_ticket_work.pytests/conftest.pytests/integration/test_cost_event_publisher_kafka.pytests/nodes/node_ab_inference_effect/test_handler.pytests/nodes/node_ci_fix_effect/test_handler_ci_fix.pytests/nodes/node_merge_sweep_triage_orchestrator/test_phase2_command_models.pytests/nodes/node_pr_review_bot/test_workflow_runner_wiring.pytests/nodes/node_thread_reply_effect/test_handler_thread_reply.pytests/test_emergency_bypass_parser.pytests/test_golden_chain_local_supervisor.pytests/test_golden_chain_nightly_loop_controller.pytests/test_golden_chain_pr_lifecycle_upgrade.pytests/test_golden_chain_review_thread_reconciler.pytests/test_model_router_authorization_and_ttl.pytests/test_model_router_exponential_backoff.pytests/test_model_router_fallback_on_degradation.pytests/test_model_router_health_check_degrades.pytests/test_model_router_publishes_degradation_event.pytests/test_model_router_routes_to_local_first.pytests/test_runtime_sha_verify_handler.pytests/test_session_bootstrap_rev7.pytests/test_webhook_reconciler.pytests/unit/config/test_settings.pytests/unit/experiments/adk_eval/harness/test_aggregator.pytests/unit/nodes/node_overnight/test_main_publish_events.pytests/unit/scripts/test_leak_gate.pytests/unit/scripts/test_run_delegation_cost_projection_process.py
…notations - _dsn() in projection_api_server.py now reads POSTGRES_HOST/PORT - _SSH_HOST in handler_handoff_effect.py now reads ONEX_INFRA_SSH_TARGET - _CANONICAL_WORKTREES in handler_ticket_work.py now reads OMNI_WORKTREES - Redpanda probe URL now reads REDPANDA_ADMIN_HOST Addresses CodeRabbit feedback on PR #547.
- pr-arch-review.yml: note that jonahgabriel fallback needs to be replaced with github-actions[bot] in a follow-up cleanup PR (deferred per CR ack) - check_leaked_literals.sh: note that type-specific annotation cross-checking is deferred to a follow-up gate hardening pass (deferred per CR ack)
…tity CR correctly flagged that the GITHUB_TOKEN actor is github-actions[bot], not the repo owner's personal login. Applying the fix directly per CR suggestion rather than deferring to a follow-up PR.
Contributor
Author
|
@coderabbitai The type-specific annotation cross-checking follow-up is now tracked at #548. Can you please resolve the open thread on |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 92
|
jonahgabriel
added this pull request to the merge queue
May 7, 2026
jonahgabriel
deleted the
jonah/omn-10580-task-8-add-ci-grep-gate-pre-commit-gha-with-allowlist
branch
May 7, 2026 18:22
4 tasks done
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
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
Implements OMN-10580: harden the advisory
check_leaked_literals.shinto a blocking gate wired as both a pre-commit hook and a required GHA status check.Gate script (
scripts/validation/check_leaked_literals.sh): default mode changed fromadvisorytoblocking; addedonex-allow-model-idannotation type; removedblock_alwaysper-path policy so annotations are honoured uniformly acrosssrc/,tests/, andscripts/; added file-level exemption for test fixtures.Pre-commit hook (
.pre-commit-config.yaml): newleaked-literals-gateentry,language: system,blocking diffscope on commit; CI runsblocking all.GHA workflow (
.github/workflows/reject-leaked-literals.yml): renamed job toLeaked Literals Gatefor required-status-check wiring; mode changed toblocking all.Source annotations: all
src/env-var fallbacks annotated withonex-allow-*inline comments.Test file exemptions: 25 test files annotated with
# onex-allow-filemarkers.Governance artifacts:
.leaked-literals-allowlist.yamlanddocs/leaked-literals-governance.md.Test suite update:
test_src_path_annotation_exemptsreplaces oldtest_src_path_annotation_does_not_exempt.DoD evidence
bash scripts/validation/check_leaked_literals.sh blocking all->findings=0Leaked literals gate (OMN-10580).........Passeduv run pytest tests/ -m unit-> 2193 passed, 0 failedEvidence-Source: OCC#774
Evidence-Ticket: OMN-10580
Ticket: OMN-10580
Summary by CodeRabbit
New Features
Documentation
Chores
Tests