Repository navigation
refactor: one method answers a kernel request, and a delegate is assembled in named steps - #1888
Conversation
Code metrics gate: PASSEDJudged: the 7 files this change touches. Files it does not touch never fail it; anything found there is listed under Outside this change for information.
Already over at the base (backlog, not this change's): 1
102 findings in files this change does not touchOutside this change (informational): 102
...and 77 more; the job log lists every row.
Thresholds and sources: |
Three places built the same response envelope -- the reply kind, the status, the result and the source marker -- and two of them also decided whether the outcome belonged in the failure ledger. `_reply` does all of it: the status comes from the result it is handed, the reply kind and the recipient from the request it answers, and a failed or errored status is logged for the FAILURE RECOVERY block. `_auto_reject` was that method with the status hard-coded, so it goes. Cyclomatic complexity of _handle_request drops from 21 to 20, which takes it off the baseline; cognitive from 84 to 82, its length from 214 lines to 191, and the module from 1596 to 1578. The new method reads 2. The two auto-reject tests this supersedes are deleted; the parametrized envelope test they are subsets of carries their assertions, including the valid_kinds the unknown-kind refusal lists. Refactor with nothing observable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
06c3f64 to
d714dc7
Compare
chaojhou
left a comment
chaojhou
left a comment
There was a problem hiding this comment.
PR #1888 -- refactor: one method answers a kernel request
What it does: IntentRouter._handle_request used to build the <kind>_done response envelope three separate ways (the _auto_reject refusal helper, the lanes_busy deferred reply, the programmatic-handler reply), and wrote the rule that sends failed or errored outcomes to the failure ledger twice. This PR replaces all of that with one IntentRouter._reply(request_msg, result, *, source). It takes the status from the result and the reply kind and recipients from the request, and records failed/error outcomes. Status, source, kind, sender, recipient and ledger behaviour are unchanged on all five reply paths, and _handle_request cyclomatic complexity drops from 21 to 20.
Blocking issues: 1
- [V5] The PR conflicts with main. The conflict comes from #1893, not #1889 [verified]
Problem:mergeable: CONFLICTING.git merge-tree origin/main d714dc7f8conflicts in src/hyperloom/orchestrator/loop/intent_router.py:1296. Main (#1893, 5c45242) replaced the line after the programmatic reply, which used to cache only successful trace_analyze results, withself._record_trace_analyze_outcome(merged_payload, result). This PR replaces the reply envelope directly above that line. #1889'starget_agent = str(intent.payload["target_agent"]).strip()merges cleanly and is kept.
Impact: The PR cannot land as is. If the hunk is resolved by taking the PR side, #1893 is reverted without any error: a failed trace_analyze is no longer remembered, so the same trace gets re-analysed.
Action: Rebase onto main and resolve the hunk asawait self._reply(request_msg, result, source=cache_hit_source or "programmatic_handler")followed by main'sif kind == "trace_analyze" and cache_hit_source is None: self._record_trace_analyze_outcome(merged_payload, result). That resolution measures_handle_requestat C901 = 20 with ruff 0.16.2. After the rebase, re-measure the size table in the description and thescripts/code_metrics_baseline.txtentries.
SKIPPED: V4 -- openprs.txt has # PARTIAL: lines for #1869, #1675, #1276, #1154 and #1032 (only each PR's first 100 files were compared), so overlap with those PRs was not checked.
SKIPPED: scoped pytest -- the only local interpreter (Python 3.12) has no pytest. Per-path equivalence was checked by reading base against head; CI test shards are green at d714dc7.
Checked: intent_router.py _reply, _handle_request, removed _auto_reject (base 190604f vs head d714dc7 vs origin/main 7e57b1e); test_profile_and_kernel_handlers.py test_coordinator_auto_reject_answers_in_one_envelope; test_regression_locks.py test_integrate_deferred_when_lanes_busy; tree-wide git grep at the head for _auto_reject, coordinator_auto_reject, programmatic_handler and lanes_busy across src, docs and scripts; docs/reference/kernel-execution-path.md:29 | Ran: git merge-tree --write-tree origin/main d714dc7f8; ruff 0.16.2 C901/PLR0913 on base, head, main and a simulated resolution (_handle_request 21 -> 20, and 20 after resolution) | Base: 190604f | Head: d714dc7
…ly-one-envelope # Conflicts: # scripts/code_metrics_baseline.txt # src/hyperloom/orchestrator/loop/intent_router.py
|
Rebased onto main; the one blocking issue is resolved exactly as prescribed. The hunk at await self._reply(request_msg, result, source=cache_hit_source or "programmatic_handler")
# Cache a successful trace_analyze, or remember a failure the same trace reproduces.
if kind == "trace_analyze" and cache_hit_source is None:
self._record_trace_analyze_outcome(merged_payload, result)#1893 is kept: a failed trace_analyze is still remembered, so the same trace is not re-analysed. #1889's Re-measured on the rebased tree as asked:
Verification: @chaojhou ready for another look. |
`IntentRouter._handle_request` built the answer to a kernel_agent request inline: a five-level nest that looked the result up in shared state, ran the handler under its action's lanes, bracketed it with lifecycle events, and then folded what came back into shared state and the trajectory timeline. `loop/kernel_request.py` now owns the resolution. It answers a request from shared state (`remembered_outcome`) or by running its handler (`run_kernel_handler`), and reports one `KernelRequestOutcome`: the result, the source the reply names, and whether it came from shared state. `_handle_request` refuses what no handler will run, replies with the outcome, and hands it to `_record_kernel_outcome`. `_lifecycle_paths` moved there as `lifecycle_paths`; the three test modules that import it follow. No observable change: internal structure only. Status, source, kind, sender, recipient, lifecycle events and ledger behaviour are unchanged on every reply path, including the lane deferral. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`handle_delegate` validated the delegate, seeded its params per action, warmed a specialist's params, derived an idempotency key and drove the registry's create-or-reuse retry loop in one body. The per-action work now sits in steps that name it: `_seed_grid_params` gives a sweep or explore delegate the params an approved proposal would have carried, `_prepare_specialist_dispatch` warms a specialist's params and refuses the delegate when its patch fails preflight, and `_create_delegated_task` registers the task, stepping the idempotency key past terminated twins. A `DelegateRequest` carries the source, intent, action name and params those steps share. No observable change: internal structure only. The pruned advisory, the policy denials and their rules and hints, the wave fan-out, the auto-key fingerprint and the queued event are unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
chennyiiis
changed the title
|
Rebased onto main (the conflict the review flagged is settled: Two further changes since your review, both pure structure:
Every unit in both files is now at or under the cyclomatic ceiling of 20: Re-requesting review. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| finally: | ||
| task.cancel() | ||
| with suppress(asyncio.CancelledError): | ||
| await task |
…ly-one-envelope # Conflicts: # src/hyperloom/orchestrator/loop/intent_router.py
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| finally: | ||
| task.cancel() | ||
| with suppress(asyncio.CancelledError): | ||
| await task |
… an import cycle `phases` reaches back into `loop`, so `kernel_request` importing `phases.machine_state` at module level put `loop.intent_router` inside a cycle whose names CodeQL reports as possibly undefined. The interval is read where the heartbeat uses it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| async def kernel_step_heartbeat(router: Any, kind: str, started: float): | ||
| """Keep orchestration's bus timestamp moving through an inline step.""" | ||
| # Imported here: ``phases`` reaches back into ``loop``, so taking it at module level would close an import cycle. | ||
| from ..phases.machine_state import KERNEL_HEARTBEAT_SEC |
|
CI is green on One fix since the last push: CodeQL flagged @chaojhou review is re-requested. |
chaojhou
left a comment
chaojhou
left a comment
There was a problem hiding this comment.
PR #1888 -- refactor: one method answers a kernel request, and a delegate is assembled in named steps
What it does: This is a pure refactor of IntentRouter's two request entry points. The three kernel-reply envelopes become one _reply. Kernel-request resolution moves to a new loop/kernel_request.py, which handles the cache and rejection short-circuits, the lanes and lease, the lifecycle bracket and the heartbeat. handle_delegate is split into _seed_grid_params, _prepare_specialist_dispatch and _create_delegated_task. ruff 0.16.2 measures C901 for handle_delegate going from 22 to 12 and for _handle_request from 21 to 7. A script that ran the base and head modules on 27 identical scenarios produced the same event traces, apart from one theoretical edge where a None status is turned into the string "None".
Blocking issues: none
Re-review of the 2026-10-17 finding: resolved. The branch now merges cleanly with main, and #1893's self._record_trace_analyze_outcome(merged_payload, result) is present after the programmatic reply and reached on the same paths.
Checked: IntentRouter._handle_request (+ _reply, _record_kernel_outcome, kernel_request.remembered_outcome / run_kernel_handler / seed_integrate_base (yes); IntentRouter.handle_delegate (+ _seed_grid_params, _is_specialist_wave, _prepare_specialist_dispatch, _create_delegated_task, DelegateReques (yes); IntentRouter._auto_reject (removed) -> _reply (yes) | Read diff, body and the round-1 review (gh api, read-only, chaojhou CHANGES_REQUESTED at d714dc7). Exported base ef93838 and head 08a61a7 with git archive to a local script,head}. Ran the equivalence driver C:/Users/chaojhou/AppData/Local/Temp/hl-cc-1888/drv/drive.py on both, giving 27 scenarios across _handle_request and handle_delegate; the only diff was the status None to 'None' edge. Measured C901 with ruff 0.16.2 (pip --target into a local script: base handle_delegate 22 and _handle_request 21; head 12 and 7; new helpers at most 8. Ran git merge-tree against origin/main: clean. Ran git log ef93838..origin/main for each PR file: only baseline commits #2021, #1984, #1986. Ran a tree-wide git grep for _auto_reject, _lifecycle_paths, _kernel_step_heartbeat, _KERNEL_HEARTBEAT_SEC, REQUEST_KIND_TO_OWNED_ACTION and intent_router logger users. Checked docs/reference | Base: ef93838 | Head: 08a61a7
|
Follow-up needed: Production code: +456/−372 (net +84), plus a new module and three NamedTuples ( Follow-up
Simplify instead of relocate
|
4 participants
Three things now answer a kernel request and enqueue a delegate, where one method did each.
One reply envelope.
_auto_rejectcollapsed three copies of the refusal envelope but left the envelope itself written three times: the refusal path, thelanes_busydeferred reply and the programmatic-handler reply all built the same{in_reply_to, kind: <kind>_done, status, result, source}message, and two of them also decided whether the outcome belonged in the failure ledger._reply(request_msg, result, source=...)does all of it: the status comes from the result it is handed, the reply kind and the recipient from the request it answers, and a failed or errored status is logged for the FAILURE RECOVERY prompt block._auto_rejectgoes.failedcoordinator_auto_rejectfailedlanes_busydeferredlanes_busydeferredis neither failed nor errorresult.get("status", "ok")programmatic_handlerResolving a kernel request moves to its own module.
_handle_requestbuilt the answer inline, five levels deep: look the result up in shared state, short-circuit a kernel patch already rejected, inject the baseline throughput an integrate omitted, take the action's catalogue lanes, bracket the handler with lifecycle events, then fold what came back into shared state and the trajectory timeline.loop/kernel_request.pynow owns the resolution and reports oneKernelRequestOutcome(the result, the source the reply names, whether it came from shared state)._handle_requestrefuses what no handler will run, replies with the outcome, and hands it to_record_kernel_outcome._lifecycle_pathsmoved there aslifecycle_paths; the three test modules that import it follow.A sibling module rather than more of
intent_router.py: the file was already over the module-length limit and in the baseline, so the new helpers had to leave it, not grow it. It drops from 1629 lines to 1469, andkernel_request.pyis 244.A delegate is assembled in named steps.
handle_delegatevalidated the delegate, seeded its params per action, warmed a specialist's params, derived an idempotency key and drove the registry's create-or-reuse retry loop in one body._seed_grid_paramsgives a sweep or explore delegate the params an approved proposal would have carried,_is_specialist_wavenames the shape that fans out into one task per entry;_prepare_specialist_dispatchwarms a specialist's params and refuses the delegate when its patch fails preflight;_create_delegated_taskregisters the task, stepping the idempotency key past terminated twins. ADelegateRequestcarries the source, intent, action name and params those steps share, so no step takes a long parameter list.Review feedback on #1881.
test_coordinator_request_unknown_kind_auto_rejectedandtest_coordinator_request_kernel_disabled_auto_rejectedare deleted as superseded, per review finding 3 on #1881. They were not quite strict subsets - the first also asserted that the refusal listsvalid_kinds- so the parametrized envelope test gains a column for the result keys a refusal must carry, and that assertion comes with it. The three modules that imported_lifecycle_pathsimportlifecycle_pathsfrom its new module; their assertions are unchanged.Run locally:
pytestover the 94 test modules naming the intent router, a delegate, a kernel request kind or the lifecycle paths - 3925 passed, including every envelope, lane-deferral, lifecycle and delegate-denial lock. The remaining failures are a missing optionalTraceLensinstall and the recipe/dotenv and vendored-SDK modules, which this diff does not touch; CI runs the full suite.Every unit in both files is at or under the cyclomatic ceiling of 20, and every unit this change adds is under all four gated limits.
handle_delegate_handle_request_record_kernel_outcome(new)_create_delegated_task(new)_prepare_specialist_dispatch(new)_seed_grid_params(new)kernel_request.run_kernel_handler(new)kernel_request.remembered_outcome(new)kernel_request.close_kernel_step(new)kernel_request.seed_integrate_base_tput(new)scripts/code_metrics_baseline.txtloseshandle_delegate's cyclomatic entry, both cognitive entries and_handle_request's function-lines entry, and lowershandle_delegateto 92 lines and the module entry to 1438.handle_delegatestays over the 80-line trigger and keeps its (lowered) baseline entry: what is left in it is the validate-then-enqueue flow itself, and cutting it further would buy helpers named for their position rather than their job. Measured with the pinnedruff 0.16.2andcomplexipy 8.0.1and the gate's own collectors.Removed: two hand-written copies of the response envelope, the second copy of the failed-outcome logging rule, and the five-level nest that stood in for naming what resolving a kernel request does. Preserved: every reply keeps its status, source, reply kind and recipient; the ledger records exactly the outcomes it recorded before; the lifecycle START/END/ERROR events, their
detailstrings and their artifact sets are unchanged, as are the delegate's pruned advisory, its policy denials with their rules and hints, the specialist wave fan-out and the auto-key fingerprint.None - refactor with nothing observable.
Breaking changes: no
PR addresses single concern: yes -
IntentRouter's two request-handling entry points, brought to the complexity ceiling; the specialist half of refactor: one rule, one place, for warming a specialist and refusing a request #1881 is refactor: one table owns every value warmed into a specialist #1887.Root cause is upstream: no
🤖 Generated with Claude Code