[jonahgabriel]

Summary

• Removes live Consul HTTP call from HandlerRegistryApiListInstances — replaces with a clean empty-list stub (Consul was decommissioned per OMN-3540; T5-A investigation (OMN-4856) confirmed no CONSUL_HOST in env, k8s manifests, or Docker infra)
• Removes all CONSUL_* error codes from node_registry_effect/contract.yaml (6 error codes, consul circuit breaker backend, consul health check, consul tags/capabilities)
• Adds pre-commit hook no-consul-references (pygrep: CONSUL_HOST|consul.connect.io in src/) to block reintroduction
• Adds CI step in test.yml ONEX Validators job to enforce the same check in CI

Risk

Low — Consul was already decommissioned (OMN-3540). The handler was reading CONSUL_HOST env var that does not exist in production config. The change makes the handler return the same result it was already returning on every real invocation (empty list, because Consul was unreachable).

Test Evidence

• All 34 pre-commit hooks pass (including new no-consul-references hook)
• Pre-push: mypy and architecture layer validation pass
• grep -rn "CONSUL_HOST" src/ returns clean after the change
• Acceptance criterion: grep -r "CONSUL" src/omnibase_infra/ returns zero matches for CONSUL_HOST (warning codes in catalog_warning_codes.py are intentional — see OMN-3540 stub design)

Rollback

Revert this commit. No migration, no data change, no config change required.

Linear

Closes OMN-4857
Part of OMN-4832 (Epic 5: Consul Decommission Audit)

Summary by CodeRabbit

• Chores
  • Consul service-discovery decommissioned: node registration and instance discovery now Postgres-only; Consul-specific flows removed and instance-list responses return empty where applicable.
  • Added safeguards to prevent reintroducing Consul references: a CI validation step and a pre-commit hook now block Consul-related strings in source files.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 492fec1b-7ae1-4594-be5c-ef7ac39c186d

📥 Commits

Reviewing files that changed from the base of the PR and between be40e99 and bc45aec.

📒 Files selected for processing (1)

• tests/unit/nodes/test_handler_registry_api_operational.py

---

📝 Walkthrough

Walkthrough

Removes Consul service discovery: adds CI and pre-commit checks to block Consul references, replaces Consul-backed list_instances handler with an empty response, and updates the node registry contract to PostgreSQL-only (rename capability, remove Consul errors/health checks, update metadata).

Changes

Cohort / File(s)	Summary
CI & pre-commit .github/workflows/test.yml, .pre-commit-config.yaml	Adds a CI step "No Consul references" and a local pre-commit no-consul-references pygrep hook to fail on CONSUL_HOST or consul.connect.io matches under src/.
List Instances Handler src/omnibase_infra/nodes/node_registry_api_effect/handlers/handler_registry_api_list_instances.py	Removes all Consul query and error-handling logic; handler now always returns {"instances": [], "total": 0}.
Registry Contract src/omnibase_infra/nodes/node_registry_effect/contract.yaml	Removes Consul backend: renames dual_backend_registration → postgres_registration, deletes Consul-specific error codes, restricts INVALID_TARGET_BACKEND to postgres, removes Consul health check, and updates metadata tags (remove consul, dual-backend; add postgresql).
Tests tests/unit/nodes/test_handler_registry_api_operational.py	Updates test to instantiate handler with no Consul args, remove Consul-error mocking, and assert empty instances; renames test method accordingly.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hopped through code and found a tidy clue,
Consul paths cleared — now Postgres gets its due,
A CI guard and pre-commit sniffs away the dust,
Empty lists humbly follow the new trust,
I nibble a carrot, cheering code that's true 🥕

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly summarizes the main objective: removing Consul functionality and adding prevention mechanisms to block its reintroduction, which aligns with all major changes across CI, pre-commit, handlers, and contract files.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch jonahgabriel/omn-4857-t5-b-remove-consul-handler-add-recurrence-prevention

📝 Coding Plan

• Generate coding plan for human review comments

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Tip

You can disable sequence diagrams in the walkthrough.

Disable the reviews.sequence_diagrams setting to disable sequence diagrams in the walkthrough.

[coderabbitai]

🧹 Nitpick comments (3)

src/omnibase_infra/nodes/node_registry_effect/contract.yaml (2)

97-108: Stale comments describe dual-backend Consul/PostgreSQL behavior.

These comments describe parallel execution with both HandlerConsulRegister and HandlerPostgresUpsert, but Consul handlers no longer exist. Consider updating or removing these references to avoid confusion.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/omnibase_infra/nodes/node_registry_effect/contract.yaml` around lines 97
- 108, The comments in the contract.yaml describe dual-backend behavior
referencing HandlerConsulRegister and HandlerPostgresUpsert which no longer
exist; update the documentation block to remove or replace Consul-specific
references: locate the "Parallel Execution Behavior" section and either remove
mentions of Consul handlers (HandlerConsulRegister) and the dual-backend
example, or replace them with the current handler names and backends used by the
register_node flow; keep/clarify descriptions of execution_mode ("parallel"),
partial_failure_handling, and retry_partial_failure semantics so they reflect
the current implementation and avoid referencing stale Consul handlers.

---

46-56: Stale comments reference Consul in the design rationale.

The design rationale comments (lines 46-48, 54-55) still describe parallel execution for "Consul + Postgres" backends, but Consul is now decommissioned. While the response model intentionally retains consul_result for backward compatibility, these comments could mislead future readers.

📝 Suggested comment updates

 # Design Rationale:
-#   - Parallel execution for independent backends (Consul + Postgres)
-#   - Partial failure handling allows one backend to succeed even if other fails
+#   - Parallel execution architecture (originally Consul + Postgres, now Postgres-only)
+#   - Partial failure handling retained for future multi-backend support
    #   - Each handler is responsible for a single backend operation
 #
 # Execution Flow:
    #   1. Receive ModelRegistryRequest with operation type
    #   2. Route to all matching handlers based on payload type
-#   3. Execute handlers in parallel (for dual-backend operations)
+#   3. Execute handlers (single backend post OMN-4857)
    #   4. Aggregate results with partial failure handling
    #   5. Return ModelRegistryResponse with combined status

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/omnibase_infra/nodes/node_registry_effect/contract.yaml` around lines 46
- 56, Update the design rationale comments to remove references to the
decommissioned "Consul + Postgres" pairing and instead describe the current
backend arrangement (e.g., Postgres as the active backend), while noting that
the response model still contains consul_result for backward compatibility;
specifically edit the block describing parallel execution and partial failure
handling so it no longer mentions Consul but explains that handlers may target
multiple backends (active and legacy fields) and that ModelRegistryRequest is
routed to handlers and aggregated into ModelRegistryResponse with
partial-failure semantics, keeping consul_result as a preserved compatibility
field.

.github/workflows/test.yml (1)

137-141: CI check pattern is less comprehensive than pre-commit hook.

The CI step only checks for CONSUL_HOST, but the pre-commit hook (.pre-commit-config.yaml line 329) also blocks consul.connect.io. This inconsistency could allow consul.connect.io references to pass CI but fail locally on pre-commit.

Consider aligning the patterns:

🔧 Proposed fix to align CI with pre-commit pattern

-          if grep -rn "CONSUL_HOST" src/; then
-            echo "FAIL: CONSUL_HOST references found in src/ — Consul is decommissioned (OMN-4857)"
+          if grep -rn -E "CONSUL_HOST|consul\.connect\.io" src/; then
+            echo "FAIL: Consul references found in src/ — Consul is decommissioned (OMN-4857)"
             exit 1
           fi
-          echo "PASS: No CONSUL_HOST references in src/"
+          echo "PASS: No Consul references in src/"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/test.yml around lines 137 - 141, The CI grep in
.github/workflows/test.yml currently only checks for "CONSUL_HOST" but must
match the pre-commit hook which also blocks "consul.connect.io"; update the CI
check to use an extended regex (e.g., search for CONSUL_HOST or
consul\.connect\.io) so the if condition fails when either pattern is present,
and update the failure/ success messages accordingly; locate the block
containing the grep invocation in test.yml and replace the single-pattern grep
with an egrep/grep -E pattern like "CONSUL_HOST|consul\.connect\.io".

🤖 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/test.yml:
- Around line 137-141: The CI grep in .github/workflows/test.yml currently only
checks for "CONSUL_HOST" but must match the pre-commit hook which also blocks
"consul.connect.io"; update the CI check to use an extended regex (e.g., search
for CONSUL_HOST or consul\.connect\.io) so the if condition fails when either
pattern is present, and update the failure/ success messages accordingly; locate
the block containing the grep invocation in test.yml and replace the
single-pattern grep with an egrep/grep -E pattern like
"CONSUL_HOST|consul\.connect\.io".

In `@src/omnibase_infra/nodes/node_registry_effect/contract.yaml`:
- Around line 97-108: The comments in the contract.yaml describe dual-backend
behavior referencing HandlerConsulRegister and HandlerPostgresUpsert which no
longer exist; update the documentation block to remove or replace
Consul-specific references: locate the "Parallel Execution Behavior" section and
either remove mentions of Consul handlers (HandlerConsulRegister) and the
dual-backend example, or replace them with the current handler names and
backends used by the register_node flow; keep/clarify descriptions of
execution_mode ("parallel"), partial_failure_handling, and retry_partial_failure
semantics so they reflect the current implementation and avoid referencing stale
Consul handlers.
- Around line 46-56: Update the design rationale comments to remove references
to the decommissioned "Consul + Postgres" pairing and instead describe the
current backend arrangement (e.g., Postgres as the active backend), while noting
that the response model still contains consul_result for backward compatibility;
specifically edit the block describing parallel execution and partial failure
handling so it no longer mentions Consul but explains that handlers may target
multiple backends (active and legacy fields) and that ModelRegistryRequest is
routed to handlers and aggregated into ModelRegistryResponse with
partial-failure semantics, keeping consul_result as a preserved compatibility
field.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7b1265e3-5c45-4009-a9c7-032f18b9a637

📥 Commits

Reviewing files that changed from the base of the PR and between fe55c3e and bc616eb.

📒 Files selected for processing (4)

• .github/workflows/test.yml
• .pre-commit-config.yaml
• src/omnibase_infra/nodes/node_registry_api_effect/handlers/handler_registry_api_list_instances.py
• src/omnibase_infra/nodes/node_registry_effect/contract.yaml