Repository navigation
feat(OMN-13644): plumb context_pack_hash onto all delegation terminals + projection - #1455
Conversation
…s + projection Capture context_pack_hash ONCE at request acceptance into DelegationWorkflowState and thread it through the single terminal builder (_emit_terminal) onto the canonical ModelDelegationResult, so it persists on COMPLETED and FAILED/ESCALATED terminals alike — escalation re-routing or prompt-text loss between attempts no longer drops it. The projection converter (_canonical_result_to_task_delegated_payload) now carries the hash into ModelTaskDelegatedEvent so the row mapping reads a real value rather than the '' default. Depends on OMN-13646 (omnibase_core ModelDelegationResult.context_pack_hash, core 0.46.1 @ 52ee18b4c8fcc517996084437266f0738caf1cb5); pin bumped here.
|
Warning Review limit reached
More reviews will be available in 40 minutes and 32 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds an OMN-13644 work-tracking contract, updates the ChangesDelegation context hash plumbing
Sequence Diagram(s)sequenceDiagram
participant ModelDelegationRequest
participant HandlerDelegationWorkflow
participant DelegationWorkflowState
participant ModelDelegationResult
participant _canonical_result_to_task_delegated_payload
participant ModelTaskDelegatedEvent
ModelDelegationRequest->>HandlerDelegationWorkflow: handle_delegation_request()
HandlerDelegationWorkflow->>DelegationWorkflowState: store context_pack_hash
HandlerDelegationWorkflow->>ModelDelegationResult: emit terminal context_pack_hash
ModelDelegationResult->>_canonical_result_to_task_delegated_payload: convert payload
_canonical_result_to_task_delegated_payload->>ModelTaskDelegatedEvent: set context_pack_hash
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
| 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_delegation_orchestrator/handlers/handler_delegation_workflow.py:489: loop = HandlerComplianceLoop()
src/omnimarket/nodes/node_delegation_orchestrator/handlers/handler_delegation_workflow.py:707: HandlerEscalationDecision()\n```\n\nHandler classes should be wired only in workflow_runner.py or adapter modules. Direct instantiation elsewhere bypasses the contract system.\n\n
Powered by node_pr_review_bot
✅ PR Review Bot — CLEANFindings: 0
Verdict semantics
Powered by node_pr_review_bot — FSM pipeline with multi-model review fan-out |
1 similar comment
✅ PR Review Bot — CLEANFindings: 0
Verdict semantics
Powered by node_pr_review_bot — FSM pipeline with multi-model review fan-out |
|
| 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: 1
🧹 Nitpick comments (1)
tests/unit/delegation/test_context_pack_hash_plumb_omn13644.py (1)
161-227: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one lifecycle-terminal regression test.
This suite covers the quality-gate
COMPLETEDpath and the inference-errorFAILEDpath, but the handler change also threadscontext_pack_hashthroughhandle_agent_task_lifecycleinsrc/omnimarket/nodes/node_delegation_orchestrator/handlers/handler_delegation_workflow.pyLine 1967. A small A2A lifecycle terminal assertion would lock down the remaining changed emitter and fully match the “all delegation terminals” contract.🤖 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 `@tests/unit/delegation/test_context_pack_hash_plumb_omn13644.py` around lines 161 - 227, Add a regression test for the A2A lifecycle terminal path, since the current suite only covers the quality-gate COMPLETED and inference-error FAILED terminals. Extend TestContextPackHashOnTerminals with a case that drives handle_agent_task_lifecycle in HandlerDelegationWorkflow and asserts the terminal event/canonical result preserves context_pack_hash, using the existing _CONTEXT_HASH and the same acceptance-pinned behavior checked by the other terminal tests.
🤖 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 `@docs/work-tracking/contracts/OMN-13644.yaml`:
- Around line 59-64: The deploy-gate evidence entry uses a noncanonical check
command, so the validator will miss it despite the correct description. Update
the DOD check under the deploy-gate item in OMN-13644.yaml to use the
repository’s canonical printf-based placeholder format and ensure the
check_value explicitly contains the word deploy. Keep the surrounding contract
structure the same and adjust only the deploy-gate evidence entry so the
validator recognizes it.
---
Nitpick comments:
In `@tests/unit/delegation/test_context_pack_hash_plumb_omn13644.py`:
- Around line 161-227: Add a regression test for the A2A lifecycle terminal
path, since the current suite only covers the quality-gate COMPLETED and
inference-error FAILED terminals. Extend TestContextPackHashOnTerminals with a
case that drives handle_agent_task_lifecycle in HandlerDelegationWorkflow and
asserts the terminal event/canonical result preserves context_pack_hash, using
the existing _CONTEXT_HASH and the same acceptance-pinned behavior checked by
the other terminal tests.
🪄 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: 83d0b8a7-0a04-4188-8dca-85c804307d2e
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
docs/work-tracking/contracts/OMN-13644.yamlpyproject.tomlsrc/omnimarket/nodes/node_delegation_orchestrator/handlers/handler_delegation_workflow.pysrc/omnimarket/nodes/node_projection_delegation/handlers/handler_projection_delegation.pytests/unit/delegation/test_context_pack_hash_plumb_omn13644.py
…k_value (CodeRabbit)
1 participant
OMN-13644 — Plumb
context_pack_hashonto all delegation terminals + projectionProblem
ModelDelegationResultgained acontext_pack_hashfield upstream (OMN-13646, omnibase_core 0.46.1 @52ee18b4c8fcc517996084437266f0738caf1cb5) but omnimarket never set it. The delegation orchestrator already collected the hash intoTerminalEmissionInputs.context_pack_hashat the three terminal sites, yet_emit_terminalbuiltModelDelegationResult(...)without it (dead field), and the projection converter dropped it — so everydelegation_eventsrow read the''default regardless of arm.Change
52ee18b4c8fcc517996084437266f0738caf1cb5(0.46.1) inpyproject.toml([tool.uv.sources]+ override) + refresheduv.lock. The existing>=0.45.0,<0.47.0constraint already allows it.handler_delegation_workflow.py): capturecontext_pack_hashonce at request acceptance intoDelegationWorkflowState.context_pack_hash; the three terminal sites now read that pinned value (not the live request), and_emit_terminalsetsModelDelegationResult(context_pack_hash=…). It therefore persists identically on COMPLETED and FAILED/ESCALATED terminals — escalation re-routing or prompt-text loss between attempts can no longer drop it.handler_projection_delegation.py):_canonical_result_to_task_delegated_payloadcarries the hash intoModelTaskDelegatedEvent, so the row mapping reads a real value rather than the''default; a missing field falls back to''honestly.The hash is non-empty only when an actual context pack was injected (ON arm, gated by
_context_pack_hash_for_eventoncontext_packpresence); the''OFF-arm default is preserved verbatim.Canonical compliance
Canonical only: CONTRACT + NODE + HANDLER. No bespoke co-writer event (the OMN-13629 anti-pattern), no
Plugin*, no new REST; topics stay contract-sourced; event-bus is the transport. No hardcoded absolute paths; strongly-typed; no defensive defaults (the documented''OFF-arm default excepted). Dev lane only — no prod/stability/judge touch.Dependencies
Depends on OMN-13646 (core
context_pack_hashfield), pinned at rev52ee18b4c8fcc517996084437266f0738caf1cb5.dod_evidence
DoD contract:
docs/work-tracking/contracts/OMN-13644.yaml.uv run pytest tests/unit/delegation/test_context_pack_hash_plumb_omn13644.py -q→ 7 passed. Proves the acceptance-pinned hash reaches the COMPLETED and FAILED canonical terminals and the projection converter; OFF-arm''preserved; missing field →''.uv run pytest tests/unit/delegation/ tests/test_golden_chain_projection_delegation.py tests/test_projection_delegation_api_contract.py -q→ 499 passed, 1 skipped.uv run pytest tests/ -q(no-k) → 9818 passed, 54 skipped, 3 failed — the 3 failures are pre-existing live-Kafka integration tests (tests/integration/test_cost_event_publisher_kafka.py,test_kafka_integration_bus_smoke.py) that time out with no local broker (runtime is on.201, not local); unrelated to this change.uv run mypy <two touched handlers> --strict→ Success: no issues found.ruff format+ruff check --fixclean;pre-commit run --all-filesclean (only the test-regenerated dep-health evidence artifacts were touched and reverted — not part of this PR).Deploy
No runtime deploy here — pure handler-logic plumb + dependency pin bump. The serialized dev redeploy + live psql proof (hash non-empty on COMPLETED and FAILED rows) runs separately (Redeploy #1).
Evidence-Source: OCC#3189
Evidence-Ticket: OMN-13644