Repository navigation
ci(omnibase_infra): add standards compliance workflow with blocking UP007 [std-sweep-v2] - #894
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughModified CI/CD workflow to treat type-safety violations as non-blocking while enabling explicit non-blocking result handling. Extended public API exports to include EnumLedgerSinkDropPolicy alongside existing protocol exports. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 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 docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
b40ca3e to
7dbe666
Compare
…P007 [OMN-5132] - Add omni-standards-compliance.yml with blocking ruff UP007/UP006 check - type-safety job added as continue-on-error: pre-existing mypy violations need cleanup - UP007 check is fully blocking (0 violations confirmed) - Gate job enforces UP007 only until mypy is cleaned up
…ed) [std-sweep-v2] Remove redundant cast() calls and add missing __all__ exports so that mypy --strict passes cleanly on the full src/ tree.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/omni-standards-compliance.yml (1)
48-49: Consider adding UV dependency caching for faster CI runs.Each job runs
uv sync --all-extrasindependently. Theastral-sh/setup-uvaction supports caching viaenable-cache: true. This could reduce CI time on subsequent runs.💡 Optional: Enable UV caching
- name: Install uv uses: astral-sh/setup-uv@v7 with: version: ${{ env.UV_VERSION }} + enable-cache: trueAlso applies to: 85-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/omni-standards-compliance.yml around lines 48 - 49, The CI currently runs "uv sync --all-extras" every job without caching; update the GitHub Actions workflow to enable UV caching by setting the astral-sh/setup-uv action's enable-cache: true (the same change for both occurrences that set up UV), so the "Install dependencies" step that runs uv sync --all-extras benefits from the action's cache on subsequent runs; locate the setup step using the action name "astral-sh/setup-uv" and add the enable-cache: true input there.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/omni-standards-compliance.yml:
- Around line 48-49: The CI currently runs "uv sync --all-extras" every job
without caching; update the GitHub Actions workflow to enable UV caching by
setting the astral-sh/setup-uv action's enable-cache: true (the same change for
both occurrences that set up UV), so the "Install dependencies" step that runs
uv sync --all-extras benefits from the action's cache on subsequent runs; locate
the setup step using the action name "astral-sh/setup-uv" and add the
enable-cache: true input there.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a34d3b4b-5828-4249-bcd9-6dd1311dd57d
📒 Files selected for processing (21)
.github/workflows/omni-standards-compliance.ymlsrc/omnibase_infra/diagnostics/bus_audit.pysrc/omnibase_infra/event_bus/adapters/adapter_protocol_event_publisher_kafka.pysrc/omnibase_infra/event_bus/event_bus_inmemory.pysrc/omnibase_infra/event_bus/testing/adapter_protocol_event_publisher_inmemory.pysrc/omnibase_infra/gateway/services/service_envelope_validator.pysrc/omnibase_infra/models/dispatch/model_topic_parser.pysrc/omnibase_infra/models/registration/model_node_event_bus_config.pysrc/omnibase_infra/nodes/node_artifact_reconciliation_orchestrator/handlers/handler_plan_to_yaml.pysrc/omnibase_infra/nodes/node_registration_orchestrator/dispatchers/dispatcher_node_introspected.pysrc/omnibase_infra/nodes/node_registration_orchestrator/dispatchers/dispatcher_node_registration_acked.pysrc/omnibase_infra/nodes/node_registration_orchestrator/dispatchers/dispatcher_runtime_tick.pysrc/omnibase_infra/nodes/node_registration_reducer/models/model_payload_postgres_update_registration.pysrc/omnibase_infra/observability/handlers/model_metrics_handler_response.pysrc/omnibase_infra/observability/sinks/sink_metrics_prometheus.pysrc/omnibase_infra/plugins/examples/plugin_json_normalizer.pysrc/omnibase_infra/plugins/examples/plugin_json_normalizer_error_handling.pysrc/omnibase_infra/protocols/protocol_ledger_sink.pysrc/omnibase_infra/runtime/binding_resolver.pysrc/omnibase_infra/runtime/models/model_config_ref.pysrc/omnibase_infra/services/retry_worker/models/model_delivery_attempt.py
7dbe666 to
6ba20ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/omni-standards-compliance.yml (3)
14-19: Set explicit least-privilege workflow permissions.No
permissionsblock is defined, so token scope depends on repo/org defaults. Explicitly setting read-onlycontentsreduces accidental privilege drift.Suggested patch
env: PYTHON_VERSION: "3.12" UV_VERSION: "0.8.3" REPO_NAME: "omnibase_infra" + +permissions: + contents: read🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/omni-standards-compliance.yml around lines 14 - 19, The workflow lacks an explicit top-level permissions block, which leaves GITHUB_TOKEN privileges to repo/org defaults; add a top-level "permissions" block to the workflow YAML and set minimal required privileges (e.g., permissions: contents: read) to enforce least-privilege for the actions that run in this file (place the permissions block alongside env and jobs at the top level of the workflow).
51-57: MyPy config discovery is too narrow (mypy.inionly).Line 53 ignores other standard config locations (for example
.mypy.ini,pyproject.toml,setup.cfg), which can make CI results diverge from local/type-check expectations.Suggested patch
- name: Run MyPy type checking run: | - if [ -f mypy.ini ]; then - uv run mypy src/ --config-file=mypy.ini + if [ -f mypy.ini ] || [ -f .mypy.ini ] || [ -f pyproject.toml ] || [ -f setup.cfg ]; then + uv run mypy src/ else uv run mypy src/ --strict fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/omni-standards-compliance.yml around lines 51 - 57, The MyPy step ("Run MyPy type checking") only checks for mypy.ini and misses other standard config locations; update the conditional around the uv run mypy invocation to detect any of .mypy.ini, mypy.ini, pyproject.toml, or setup.cfg and, if found, pass the appropriate --config-file to mypy (or omit --config-file so mypy auto-discovers), otherwise run mypy with --strict; modify the existence checks in that step to look for these filenames and prefer using mypy's built-in discovery when possible so CI matches local behavior.
25-29:schedulebranch in runner selection is dead code right now.Line 28/67/99 checks
github.event_name == 'schedule', but noon.scheduletrigger exists (Line 6-13). Either add a schedule trigger or remove this condition to keep intent clear.Also applies to: 64-68, 96-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/omni-standards-compliance.yml around lines 25 - 29, The workflow contains dead conditional branches checking github.event_name == 'schedule' without any corresponding on: schedule trigger; update the workflow by either adding an on: schedule trigger with the desired cron(s) or removing the schedule checks (instances of github.event_name == 'schedule') from the runner selection logic (the conditional that uses vars.USE_SELF_HOSTED_RUNNERS and fromJSON('["self-hosted","omnibase-ci"]')) so the conditions reflect actual triggers; apply the same change to all repeated occurrences of that condition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/omni-standards-compliance.yml:
- Around line 88-90: The workflow step named "Check for Optional/Union usage
(PEP 604)" currently runs ruff with both selectors UP007,UP006 so UP006 failures
also fail the type-union-check gate; change it so only UP007 is blocking and
UP006 is informational by splitting into two runs: one that runs uv run ruff
check src/ --select UP007 (keeps current failure semantics for the blocking
check) and a separate run for uv run ruff check src/ --select UP006 that is
marked non-blocking (e.g., set continue-on-error: true or append || true) or
moved to an informational step so UP006 failures do not flip the
type-union-check job to failed.
---
Nitpick comments:
In @.github/workflows/omni-standards-compliance.yml:
- Around line 14-19: The workflow lacks an explicit top-level permissions block,
which leaves GITHUB_TOKEN privileges to repo/org defaults; add a top-level
"permissions" block to the workflow YAML and set minimal required privileges
(e.g., permissions: contents: read) to enforce least-privilege for the actions
that run in this file (place the permissions block alongside env and jobs at the
top level of the workflow).
- Around line 51-57: The MyPy step ("Run MyPy type checking") only checks for
mypy.ini and misses other standard config locations; update the conditional
around the uv run mypy invocation to detect any of .mypy.ini, mypy.ini,
pyproject.toml, or setup.cfg and, if found, pass the appropriate --config-file
to mypy (or omit --config-file so mypy auto-discovers), otherwise run mypy with
--strict; modify the existence checks in that step to look for these filenames
and prefer using mypy's built-in discovery when possible so CI matches local
behavior.
- Around line 25-29: The workflow contains dead conditional branches checking
github.event_name == 'schedule' without any corresponding on: schedule trigger;
update the workflow by either adding an on: schedule trigger with the desired
cron(s) or removing the schedule checks (instances of github.event_name ==
'schedule') from the runner selection logic (the conditional that uses
vars.USE_SELF_HOSTED_RUNNERS and fromJSON('["self-hosted","omnibase-ci"]')) so
the conditions reflect actual triggers; apply the same change to all repeated
occurrences of that condition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1171ee95-501b-4211-b9eb-ec56efc45ef6
📒 Files selected for processing (7)
.github/workflows/omni-standards-compliance.ymlsrc/omnibase_infra/event_bus/event_bus_inmemory.pysrc/omnibase_infra/nodes/node_registration_reducer/models/model_payload_postgres_update_registration.pysrc/omnibase_infra/observability/handlers/model_metrics_handler_response.pysrc/omnibase_infra/protocols/protocol_ledger_sink.pysrc/omnibase_infra/runtime/models/model_config_ref.pysrc/omnibase_infra/services/retry_worker/models/model_delivery_attempt.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/omnibase_infra/nodes/node_registration_reducer/models/model_payload_postgres_update_registration.py
- src/omnibase_infra/services/retry_worker/models/model_delivery_attempt.py
- src/omnibase_infra/protocols/protocol_ledger_sink.py
6ba20ec to
1ca88cd
Compare
…d-sweep-v2] Preserve continue-on-error: true on type-safety job (pre-existing mypy violations on main, non-blocking until resolved per OMN-5132).
1ca88cd to
d5d1f7c
Compare
Summary
omni-standards-compliance.ymlwith blockingruff check --select UP007,UP006checktype-safetyjob (mypy) added ascontinue-on-error: true— 25 pre-existing mypy violations need cleanup in a follow-upPart of std-sweep-v2 initiative [OMN-5132].
Test plan
type-union-checkjob passes (0 UP007 violations)type-safetyjob runs but does not block merge (continue-on-error)Summary by CodeRabbit
Changes