Repository navigation
feat(host_api): Slice C.1 — Invocation payload vocabulary (#6168) - #6223
Conversation
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted reviewer state before this projection. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
277f774 to
50e1c2e
Compare
There was a problem hiding this comment.
Code Review
This pull request consolidates the outbound state stores by removing several parallel in-memory implementations (such as InMemoryOutboundStateStore and InMemoryDeliveredGateRouteStore) and replacing them with the production FilesystemOutboundStateStore running over an in-memory filesystem backend for tests. It also introduces Slice-C kernel capability vocabulary types (Invocation, InvocationOrigin, ProductKind, RoutineId, and ActivityId) in ironclaw_host_api as part of the capability-path DTO collapse. Feedback is provided to improve test assertions in invocation.rs by asserting against specific error variants instead of using generic is_err() calls.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| assert!(ProductKind::new("").is_err()); | ||
| assert!(RoutineId::new("").is_err()); | ||
| // Uppercase-leading is rejected by the name-segment validator. | ||
| assert!(ProductKind::new("Settings").is_err()); |
There was a problem hiding this comment.
According to the general rules, we should avoid using generic is_err() assertions in tests when verifying that an operation fails. Instead, assert against the specific expected error message or variant to prevent infrastructure or harness-level failures from causing false positives.
| assert!(ProductKind::new("").is_err()); | |
| assert!(RoutineId::new("").is_err()); | |
| // Uppercase-leading is rejected by the name-segment validator. | |
| assert!(ProductKind::new("Settings").is_err()); | |
| assert!(matches!(ProductKind::new(""), Err(crate::ids::IdError::InvalidFormat { .. }))); | |
| assert!(matches!(RoutineId::new(""), Err(crate::ids::IdError::InvalidFormat { .. }))); | |
| // Uppercase-leading is rejected by the name-segment validator. | |
| assert!(matches!(ProductKind::new("Settings"), Err(crate::ids::IdError::InvalidFormat { .. }))); |
References
- Avoid using generic is_err() assertions in tests when verifying that an operation fails. Instead, assert against the specific expected error message or variant to prevent infrastructure or harness-level failures from causing false positives.
There was a problem hiding this comment.
Applied in the follow-up commit: the rejection tests now assert the specific kind + reason from HostApiError::InvalidId's message (product/routine + "must not be empty", and the uppercase-leading case pins the kind) instead of bare is_err().
Gemini note on #6223: pin the kind + reason of each expected rejection instead of bare is_err(), so an infrastructure failure can't masquerade as a validation pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🚅 Deployed to the ironclaw-pr-6223 environment in ironclaw-ci-preview
|
✅ Ready for mergeReviewed, finding fixed with reply, CI fully green (19 pass / 0 fail) on head
🤖 Generated with Claude Code |
Gemini note on #6223: pin the kind + reason of each expected rejection instead of bare is_err(), so an infrastructure failure can't masquerade as a validation pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
b94edc5 to
361f06e
Compare
c70f7fd to
a581269
Compare
Second §4.4 de-prefix slice (after C1's LocalDevRootFilesystem inline). Advances the doc's §4.4 enforcement endgame — "no public type name contains Local/LocalDev/Hosted/Enterprise" — one more type. `LocalDevOutboundStores` is a plain bundle struct (four outbound-store handle fields), NOT a cfg-switched policy/mode type. Its own constructor comment says it "works in both durable (libsql/postgres) and no-durable (in-memory backend) builds" — so the `LocalDev` prefix is factually wrong (it is used in libsql/ postgres PRODUCTION builds, not just local-dev). This is a bucket-2-style mis-prefix: a genuine composition type that only LOOKS like a deployment-mode leak, so the fix is a de-prefix rename, not the bucket-1 DeploymentConfig resolution the cfg-switched `LocalDev*Store` aliases need. Renamed the struct + its 3 use sites (all in factory.rs) to `OutboundStores` (name was free); trimmed it from the R2 `reborn_localdev_typename` ratchet allowlist. The `local_dev_outbound_store` builder fn keeps its name (fn names are not type-name leaks; the ratchet inventories types only). Pure type rename, semantically identical. Verified: localdev ratchet 4; `cargo build -p ironclaw_reborn_composition` (default + libsql+slack+telegram) clean; clippy -D warnings clean; fmt + pre-commit clean. Stacked on #6218. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…name ratchet (§4.4/§10)
Completes the §4.4-mandated enforcement — "no public type name contains
Local/LocalDev/Hosted/Enterprise" (the doc says: "Enforce it with an
ironclaw_architecture test"). The existing `reborn_localdev_typename_ratchet`
owns `LocalDev*` (shrinking to empty as Slice B lands) and explicitly scopes out
"the broader Local*/Hosted* audit ... as a separate concern." This companion
ratchet owns that separate concern — the OTHER three prefixes:
- `Enterprise*` — NONE exist (achieved). Empty allowlist locks it in: a new
`EnterpriseTierPolicy`-style mode leak fails the "no new" check.
- `Hosted*` — all `HostedMcp*`/discovery/egress, a Bucket-3 FALSE POSITIVE
("hosted MCP" is a real domain concept — a platform-hosted MCP server — not a
hosted-TIER deployment mode). Frozen/justified so a genuine `HostedTierRuntime`
leak can't slip in behind them.
- `Local*` (excluding `LocalDev*` → sibling ratchet, and `Locale*` → localization
false positive) — the `LocalTriggerAccess*` family is genuine Bucket-1 debt
(§4.4 folds `local_trigger_access` into "seed owner grant from config at boot,"
a policy value); `LocalInvocationServicesResolver` awaits a design-call rename.
Reuses the shared `ratchet_support` scanner (comments/strings stripped,
visibility-aware, skips tests/examples/benches). Same frozen-set contract as the
sibling ratchets: no new type, no duplicate definition, trim on delete/rename.
Predicate uses `starts_with` (not `contains`) so mid-word `Local` (`HookLocalId`)
is not flagged; a self-test pins that + the `LocalDev`/`Locale` exclusions.
Test-only, no production change. Verified: 2 tests pass (frozen allowlist +
predicate self-test); clippy -D warnings clean; fmt + pre-commit clean.
Stacked on #6220.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…efixes Addresses all three review findings on #6222: - IronLoop: §4.4's rule is "no public type name CONTAINS Local/Hosted/ Enterprise" — the predicate now matches the terms at any CamelCase word boundary (followed by uppercase/digit/underscore/end), which surfaces and freezes 16 previously-invisible mode-shaped mid-names: the RebornLocal* composition family, the Reborn*LocalTriggerAccess* backends, mid-name LocalDev types, and HookLocalId (justified-keep: hook-local id is a domain concept, annotated as such). - Gemini (both): localization names need no hand-listed exceptions under boundary matching — Locale*/Localization*/Localised* continue lowercase so the word is not "Local"; the self-test now pins those exclusions plus the mid-name positives (RebornLocalRuntimeServices, HookLocalId, SelfHostedMcpClient). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
First sub-slice of Slice C (arch-simplification §3/§5) — the capability-path
DTO collapse. Per the migration plan (§9), the kernel vocabulary lands in
`ironclaw_host_api` *first*, ahead of any wiring; this PR adds the single
data-plane payload every layer will reference, replacing the request side of
the ~14-hop re-wrap (§1.1).
- ids.rs (via the crate's own `uuid_id!`/`string_id!` macros):
- `ActivityId` — the invocation's idempotency identity (§11.3); what §1.1's
dead-future `idempotency_key` becomes, unified rather than deleted.
- `ProductKind` / `RoutineId` — validated string newtypes for the two non-loop
origins (kept strings, not enums, while the product/routine sets evolve, §5.8).
- invocation.rs (new):
- `InvocationOrigin { LoopRun(RunId) | Product(ProductKind) | Automation(RoutineId) }`
— sealed-at-membrane origin (§5.2.1); snake_case wire-tagged; `.kind()`
discriminant pinned to the tag for per-origin accounting views (§5.3.3).
- `Invocation { activity_id, capability, input, scope, actor, origin, estimate }`
— the "one payload" (§3/§4.1). Reuses existing host_api types; `actor` is
required (sealed), and authorization *outputs* (`mounts`, `resource_reservation`)
are deliberately absent — they move into the sealed `Authorized` in a later slice.
Additive only — nothing is wired into the dispatch path yet. The five old request
DTOs still exist and function; per §9 the type count rises before it falls
(~14 → ~18 → ~11) as the new vocabulary and old shapes coexist.
`check-type-duplicates.py` does not flag `Invocation` against
`CapabilityDispatchRequest` (it is a genuinely distinct state, not a mirror).
Tests are crate-tier because the types are not production-wired yet; integration
coverage is owed when a later slice threads `&Invocation` through the four
capability mediators (testing.md: crate-tier when the harness cannot reach the path).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Gemini note on #6223: pin the kind + reason of each expected rejection instead of bare is_err(), so an infrastructure failure can't masquerade as a validation pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
361f06e to
74e84c9
Compare
a581269 to
2dfc33d
Compare
What
First sub-slice of Slice C — the capability-path DTO/
dyncollapse fromdocs/reborn/2026-07-17-architecture-simplification-dto-dyn-local.md(§3, §5). Per the migration plan (§9), Slice C lands the kernel vocabulary inhost_apifirst, ahead of any wiring. This PR adds the single data-plane payload that replaces the request side of the ~14-hop re-wrap (§1.1).Stacked on #6222 (C3, deployment-mode ratchet).
Types added
ids.rs(via the crate's ownuuid_id!/string_id!macros):ActivityId— the invocation's idempotency identity (§11.3); what §1.1's dead-futureidempotency_keybecomes, unified not deleted.ProductKind/RoutineId— validated string newtypes for the two non-loop origins (kept strings, not enums, while the product/routine sets are still evolving per §5.8; noted as future-enum candidates).invocation.rs(new module):InvocationOrigin { LoopRun(RunId) | Product(ProductKind) | Automation(RoutineId) }— sealed-at-membrane origin (§5.2.1); snake_case wire-tagged;.kind()discriminant pinned to the tag for per-origin accounting views (§5.3.3).Invocation { activity_id, capability, input, scope, actor, origin, estimate }— the "one payload" (§3/§4.1) every layer will reference instead of re-declaring.Design reconciliations (documented in the code)
LoopRun(TurnRunId), buthost_apicannot depend onironclaw_turns. Modeled asRunId— this crate's prompt-visible turn-run identity (= ExecutionContext::run_id); the alias relationship is noted in the doc comment.Invocation.actoris a requiredUserId(vsCapabilityDispatchRequest.authenticated_actor_user_id: Option<UserId>) — the sealed-at-membrane intent.mounts/resource_reservationare deliberately absent — they are outputs ofauthorize(), not request inputs, so they move into the sealedAuthorizedwitness in a later slice.Why additive is safe (§9)
Nothing is wired into the dispatch path. The five old request DTOs still exist and function; the doc is explicit that the type count rises before it falls (~14 → ~18 → ~11) while the new vocabulary and old shapes coexist.
python3 scripts/check-type-duplicates.pydoes not flagInvocationagainstCapabilityDispatchRequest— it is a genuinely distinct state, not a mirror.Testing
.kind()↔tag agreement, id validation,ActivityIdstable-carry (idempotency), and one-payload-per-origin construction.&Invocationthrough the four capability mediators (testing.md: crate-tier is appropriate when the integration harness cannot reach the path).Checks
cargo test -p ironclaw_host_api— 38 + 57 + 5 greencargo clippy -p ironclaw_host_api --all-targets --all-features -- -D warnings— cleancargo test -p ironclaw_architecture— all boundary + the localdev/deployment-mode/inmemory ratchets greenscripts/pre-commit-safety.sh— cleanNext sub-slices (planned, stacked)
Resolution/Blocked/Suspension/HostFailure/Outcomeresult channels (the §5.3 acceptance table maps all 10CapabilityOutcomevariants; kills §1.2'sOk(Failed)-vs-Errambiguity).LoopRequest+resolve()membrane.Authorized+authorize()(security milestone — the seal becomes load-bearing when the last policy check is inlined, §9).&Invocation/&Authorizedthrough the four mediators without merging crates (§9 step 3) and measure type/dyncounts (step 4).🤖 Generated with Claude Code