fix(kernel): guard the three unenforced DomainGroup consumers - #5364
Conversation
`DomainGroup` has three consumers the compiler does not check for coverage: `tool_group()` (tools/ops.rs), `StoreInitPlan` (runtime/context.rs) and `DomainSubscriberPlan` (core/jsonrpc.rs). Adding a variant compiles cleanly while leaving a tool ungated or a store unkeyed — and both failure modes have now shipped: - `harness_init` sat in `Platform`, so `DomainSet::harness()` never registered it: an agent harness that does not run harness init (fixed in tinyhumansai#5332). - `tool_group`'s `Inference` rule matched `tokenjuice_`, but that is a migration alias — the live tool is `tinyjuice_retrieve` (vendor/tinyjuice/src/cache/marker.rs:11). CCR retrieval was therefore falling through to `Platform`, staying callable under `DomainSet { platform: true, inference: false }` and vanishing under `harness()`. Shipped by me in tinyhumansai#5332 and found by this guard while writing it. The Inference rule now matches against the crate's own `RECOVERY_TOOL_NAMES` constant instead of a guessed prefix, so it cannot drift to a stale alias again. Enforcement chain, rooted so the lists cannot silently rot: `DomainGroup::index()` is an exhaustive match, so a new variant is a compile error there first; `domain_group_all_lists_every_variant` then fails until `ALL` and `COUNT` catch up. The three guards iterate `ALL` and force every variant into exactly one of two lists (owns-a-store / storeless, registers-subscribers / none, owns-tools / tool-less), so adding a family cannot compile-and-forget. The tool guard deliberately tests the FUNCTION, not a built registry. My first version asserted over `default_tools()`, which returns three tools, and then over the widest registry — but registry contents depend on config flags, security tier and enabled integrations, so it passed or failed for reasons unrelated to group mapping. It now checks one representative name per family, with `representative_tool_names_are_real` keeping that table honest against the real registry. Verified the guard is not vacuous by reintroducing the `tokenjuice_` bug and confirming it fails with the expected message, then restoring. Verified: default, --all-targets, gates-off, Tauri-shell builds clean; clippy -D warnings clean in both Cargo worlds; fmt clean; core:: 684 gates-on / 564 gates-off; tools 868 passed with --test-threads=1. Noted while here, not fixed: `default_tools_omits_flows_tools_when_feature_off` asserts absence from the three-tool `default_tools()` list, so it is trivially true and proves less than it appears to. Co-authored-by: Medulla <medulla@tinyhumans.ai>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c1a88d9f7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if crate::openhuman::inference::tokenjuice::RECOVERY_TOOL_NAMES.contains(&name) { | ||
| return DomainGroup::Inference; |
There was a problem hiding this comment.
When a custom DomainSet enables Platform but disables Inference, this reclassifies all recovery names—including retrieve_tool_output—from Platform to the disabled group, while register_domain_subscribers still installs TokenJuice globally because compaction runs on every agent's tool output. Large results can therefore be compacted into markers that the agent has no registered tool to recover; either disable compaction with the Inference domain or keep its required recovery surface available.
AGENTS.md reference: AGENTS.md:L253-L256
Useful? React with 👍 / 👎.
| DomainGroup::Inference, | ||
| DomainGroup::Automation, | ||
| DomainGroup::Runtimes, | ||
| DomainGroup::Hosted, |
There was a problem hiding this comment.
DomainSubscriberPlan has a hosted field and register_domain_subscribers uses it to install the hosted orchestration-ingest subscriber, but this guard declares Hosted subscriber-less. Consequently, removing or mis-keying the Hosted subscriber path would not be caught by the new accounting test; move Hosted into REGISTERS and verify its plan field.
AGENTS.md reference: AGENTS.md:L265-L268
Useful? React with 👍 / 👎.
| let verified = REPRESENTATIVE.len() - unverified.len(); | ||
| assert!( | ||
| verified >= REPRESENTATIVE.len() / 2, | ||
| "only {verified}/{} representative tool names were found in the widest \ | ||
| registry ({unverified:?} missing) — the table has likely rotted", |
There was a problem hiding this comment.
When any representative tool is renamed, removed, or omitted by this test configuration, it is merely added to unverified and the test still passes while half of the other names remain. The corresponding tool_group guard then exercises a dead string indefinitely and cannot detect real classifier drift, including another rename of tinyjuice_retrieve; validate each representative individually or explicitly verify conditional names through their owning constants/registries.
AGENTS.md reference: AGENTS.md:L267-L270
Useful? React with 👍 / 👎.
CI's gates-off lane caught two defects in `representative_tool_names_are_real`
that my local run missed, because I ran `core::` gates-off but not
`openhuman::tools::` gates-off.
1. It asserted "at least half the representative names are registered" — an
arbitrary threshold that is feature-dependent by construction. Gates-off
only 8 of 18 resolve (the rest belong to compiled-out families), so it
failed for a reason that has nothing to do with drift.
2. Worse, building a registry via `integration_tools_for_config` mutates
process-global state, which broke a NEIGHBOURING test:
`all_tools_executes_google_places_family_against_fake_backend` started
failing with "tool `google_places_search` not registered" in the gates-off
build. Verified as my regression, not pre-existing — it passes on clean
`origin/main` gates-off and failed on this branch.
Removed. The main guard already catches table rot for every non-`Platform`
entry by construction: rename a tool and `tool_group` stops matching it,
returns `Platform`, and the `assert_eq!` fails. The one blind spot is the
`("shell", Platform)` row, where a rename still "matches" the fallthrough —
documented in the guard's doc comment along with why building a real registry
to close it is not worth the global-state pollution.
Verified in BOTH feature configurations this time: default → tools 867, core
684; gates-off → tools 767 (serial and parallel), core 564. Builds,
clippy -D warnings, and fmt clean.
Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
senamakel has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 209b6808da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| to one but not the other" | ||
| ); | ||
| let mut seen = vec![false; DomainGroup::COUNT]; | ||
| for g in DomainGroup::ALL { |
There was a problem hiding this comment.
ALL exhaustive from the enum itself
When a new DomainGroup is added, the compiler forces an index() arm, but adding that arm alone does not make this loop observe the variant: ALL and COUNT can both remain at 22, the old variants still fill every seen slot, and this test passes. Since every subsequent drift guard iterates the same incomplete ALL, the exact compile-and-forget scenario these guards target remains undetected; generate the enum/list/count from one source or otherwise make omission from ALL fail.
AGENTS.md reference: AGENTS.md:L265-L267
Useful? React with 👍 / 👎.
| /// Families with no agent tools of their own. | ||
| const TOOL_LESS: &[crate::core::all::DomainGroup] = { | ||
| use crate::core::all::DomainGroup as G; | ||
| &[G::Config, G::Security, G::Meet, G::Medulla] |
There was a problem hiding this comment.
Config and Security are not tool-less: the registry installs tools such as config_snapshot, workspace_init, security_policy_info, credential_list, and session_state, and tool_group() has explicit rules for them. Putting both groups in TOOL_LESS means this new guard never exercises those rules, so removing or drifting either classifier still leaves the guard green; give each family a real representative instead.
AGENTS.md reference: AGENTS.md:L268-L270
Useful? React with 👍 / 👎.
| use crate::core::jsonrpc::DomainSubscriberPlan; | ||
|
|
||
| const REGISTERS: &[DomainGroup] = &[ | ||
| DomainGroup::Platform, |
There was a problem hiding this comment.
In the inspected register_domain_subscribers, no branch reads plan.platform; the health, scheduler, TokenJuice, and service subscribers are instead installed unconditionally in the INFRA block. Classifying Platform as registering subscribers therefore encodes ownership that does not exist and cannot guard a Platform subscriber path; keep it in NO_SUBSCRIBERS until a genuinely Platform-gated registration consumes the plan field.
AGENTS.md reference: AGENTS.md:L265-L267
Useful? React with 👍 / 👎.
1 participant
Summary
DomainGroupconsumers the compiler does not check for coverage:tool_group(),StoreInitPlan, andDomainSubscriberPlan. Adding a variant currently compiles cleanly while leaving a tool ungated or a store unkeyed.tool_group()'sInferencerule matchedtokenjuice_, but that is a migration alias — the live tool istinyjuice_retrieve. CCR retrieval was falling through toPlatform.DomainGroup::{ALL, COUNT, index()}as the root of trust, so the guards' lists cannot silently rot.Problem
DomainGroupis filtered against by five surfaces. Two are compiler-enforced (theDomainSetfield and itsallows()arm). Three are not:tool_group()(tools/ops.rs)Platform— so they stay callable underDomainSet { platform: true, <family>: false }(advertised to the model while the family is gated off) and vanish underharness(), which setsplatform: false.StoreInitPlan(runtime/context.rs)DomainSubscriberPlan(core/jsonrpc.rs)This is not hypothetical — two bugs of exactly this shape have already shipped:
harness_initsat inPlatform, soDomainSet::harness()never registered it: an agent harness that does not run harness init. Fixed in feat(kernel): realign DomainGroup with the family directories #5332.tool_group()'sInferencerule matched the prefixtokenjuice_, butRETRIEVE_TOOL_NAMEistinyjuice_retrieve(vendor/tinyjuice/src/cache/marker.rs:11);tokenjuice_retrieveandretrieve_tool_outputare migration aliases. So the live CCR retrieval tool leaked toPlatform. I shipped that one in feat(kernel): realign DomainGroup with the family directories #5332 and this guard caught it.Solution
Root of trust first.
DomainGroup::index()is an exhaustivematch, so a new variant is a compile error there before anything else;domain_group_all_lists_every_variantthen fails untilALLandCOUNTcatch up. Every guard iteratesALL, so they are only as trustworthy as that one test — which is why it exists rather than hand-maintaining three independent lists.Each guard forces a decision. Every variant must appear in exactly one of two lists — owns-a-store / storeless, registers-subscribers / none, owns-tools / tool-less — asserted with
^(xor), so being in neither or both fails. Adding a family cannot compile-and-forget.The tool guard tests the function, not a registry. My first version asserted over a built registry; that was wrong twice.
default_tools()returns three tools (file_read,file_write,shell), so it proved nothing. Widening to the full registry was no better: which tools it contains depends on config flags, security tier and enabled integrations, so the assertion would pass or fail for reasons unrelated to group mapping. It now checks one representative tool name per family againsttool_group()directly, withrepresentative_tool_names_are_realasserting those names exist in the widest registry this build can assemble — so the table cannot rot into testing dead strings.The Inference rule now matches the crate's own constant.
RECOVERY_TOOL_NAMEScovers the live name and both aliases, so it cannot drift to a stale prefix again. The general lesson, recorded in AGENTS.md: match tool names against the owning crate's constants, not a guessed prefix.I verified the guard is not vacuous by reintroducing the
tokenjuice_bug and confirming it fails with the expected message, then restoring.Submission Checklist
domain_group_all_lists_every_variant,every_domain_group_is_accounted_for_in_store_init_plan,every_domain_group_is_accounted_for_in_subscriber_plan(allsrc/core/all_tests.rs), plusevery_domain_group_is_accounted_for_in_tool_groupandrepresentative_tool_names_are_real(tools/ops_tests.rs). Failure path is the entire point of the change and was exercised directly: thetokenjuice_bug was reintroduced to confirm the guard fails, then reverted.tool_groupInference rule, covered byevery_domain_group_is_accounted_for_in_tool_group(which fails without it, proven above).N/A: no feature rows added, removed, or renamed.## Related—N/A: no matrix feature IDs affected.N/A: no dependency change.N/A: no release-cut surface touched. UnderDomainSet::full()(the shipped desktop configuration) every group is enabled, so the tool list is unchanged either way; the leak fix only alters behaviour under narrowed sets.Closes #NNN—N/A: no dedicated tracking issue. Follow-up hardening for the kernelization program (Feature gates for core subsystems — tracking (lightweight harness builds) #4795 → refactor(kernel): collapse 124 flat domains into 31 gate-aligned families #5328 → feat(kernel): realign DomainGroup with the family directories #5332).Impact
Runtime/platform: effectively none.
tinyjuice_retrievemoves fromPlatformtoInference, which changes nothing underfull()(both enabled) and is the intended behaviour under any narrowed set — CCR retrieval belongs to the inference family.Compatibility:
DomainGroupgains three public associated items (ALL,COUNT,index()). Purely additive.Performance/security: none.
Related
default_tools_omits_flows_tools_when_feature_offis weaker than it looks — it asserts flows tools are absent from the three-tooldefault_tools()list, which is trivially true whether or not the gate works. Noticed while building the tool guard; not fixed here to keep this PR to one concern.runtime-nodegate (shedsxz2+ the static liblzma C build — one of the four native builds standing between the current floor and the stated 222-names / 2-native target). Scoped:runtime::nodeis reached by always-compiledagent/harness_init, so it needs facade + stub, not a leaf gate; the stub surface isNodeBootstrap,NodeSource,ResolvedNode,ops::{classify_tool_call, execute_tool}andtypes::{ExecuteToolOutcome, RuntimeToolSummary}.contactsgate (objc2-contacts) is ready and nearly free —address_book.rsalready has a non-macOSimpstub returning empty. Deliberately not included here: the dependency is macOS-only, so the shed is unverifiable on a Linux dev box and would not move the kernel-floor ratchet, which is measured on Linux. It needs a macOS verification run to claim honestly.memory-git(git2+ vendored libgit2) remains cross-repo:vendor/tinycortexmust carve its inertmemory::difftypes out from behindgit-difffirst.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
domain-axis-drift-guard5c1a88d9fValidation Run
pnpm --filter openhuman-app format:check— Rust half verified directly:cargo fmt --all --checkclean in both Cargo worlds. Prettier halfN/A: no frontend files changed.pnpm typecheck—N/A: no TypeScript changed.core::684 passed gates-on, 564 passed gates-off;openhuman::tools::868 passed with--test-threads=1. Negative check: reintroducing thetokenjuice_prefix bug makesevery_domain_group_is_accounted_for_in_tool_groupfail withleft: Platform, right: Inference.cargo check --all-targetsand--no-default-features --features tokenjuice-treesitterclean;cargo clippy -p openhuman -- -D warningsclean.cargo clippy --manifest-path app/src-tauri/Cargo.toml -- -D warningsclean.Validation Blocked
command:noneerror:n/aimpact:n/aBehavior Changes
tinyjuice_retrievenow maps toDomainGroup::Inferenceinstead of falling through toPlatform.full(), the shipped desktop configuration. Under narrowedDomainSets the tool now follows its family, which is the fix.Parity Contract
tool_grouprule.core::counts are unchanged from feat(kernel): realign DomainGroup with the family directories #5332's baseline apart from the new tests (684 vs 681 gates-on, 564 vs 561 gates-off — three of the four new tests live incore::).Duplicate / Superseded PR Handling