fix(hull): move credential issuance inside the trust boundary - #3530
Conversation
📝 WalkthroughWalkthroughThe change centralizes Hull authority, host identity, credential validation, and secured invocation. It adds a language-neutral Hull shell adapter, updates Carrier bridge and SDK delegation, and expands security, architecture, typecheck, and integration validation. ChangesHull membrane security and composition
Hull protocol adapter
Carrier integration
Architecture and delivery records
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
github-actions
Bot
added
the
needs-deep-review
|
thought (non-blocking): Accessibility audit (advisory)The sharded axe audit is report-only while the baseline and runtime budget mature.
Shard 1 reportShard 2 reportShard 3 report |
b7b4368 to
db3519f
Compare
ctwoodwa
added
the
deep-review-passed
The credential half of pull request 3517, stacked on the contract half. Three review rounds each found the previous round's remediation had closed the one constructor a review named and left the same pattern in a sibling. The cause was the export surface, so the fix is structural rather than another call-site patch. Authorization is split into a policy STORE and a policy EVALUATOR. The store is the source of truth: immutable, null-prototype, frozen, and fail-closed to the most restrictive class when its source cannot be read. The evaluator is built from the store and returns allow or deny for the current request. The security layer reads the source of truth directly, so no mutable permission object crosses a trust boundary and the secured invoke takes no policy parameter at all. Credential construction leaves the public surface entirely. The hull exports only enforcement operations and credential INPUT types; the issuers are private, reachable solely from host composition roots. A caller therefore receives no subject or confirmation constructor through any exported path. The guarantee is asserted structurally rather than by call-site validation: a test proves a shape-only principal is still rejected with the per-call-site check removed, because that is the property three rounds of call-site fixes failed to hold. The node bridge is fail-closed again on both halves of the property at once. Caller-supplied identity in the payload is ignored, AND an unattributed request is refused rather than proceeding under host authority. Removing the payload identity was correct on its own but had been paired with an unconditional host stamp, so an anonymous request began succeeding with the host's rights. Verified by mutation rather than by report, and mutated toward a different wrong answer: a red suite proves a check READS its input, not that it CHECKS the property. The built package exposes 115 runtime exports, none a credential constructor, and the exploit the reviews described fails on all three imports when compiled from a consuming package. Restores the build gates that ADR 0162 marked deferred while only the contract half had landed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An independent review proved at runtime that untrusted code could obtain a value the PEP accepts as authority — not by forging one, but by being handed one. Making the mints private closed only half the property. The PEP authenticates by object identity, so a registered principal is a bearer token: whoever holds it is authenticated for ANY command, not the one it was issued for. Privacy of the constructor stops a caller CONSTRUCTING a credential and does nothing to stop one ESCAPING. Three channels leaked one, all reachable from outside the boundary and all compiling with no casts, so the brand-forgery fence never fired: - a caller-supplied decision sink received the live registered object; - a caller-supplied shell received it in the invoke context; - a composed operation returned it inside its result metadata. Now both halves are enforced. Only trusted host code registers, and nothing crossing back out carries a registered object. Anything that needs to say WHO acted carries an inert attribution — a frozen, null-prototype, never-registered copy that the PEP refuses exactly like any other shape-only value. Identity survives the boundary; authority does not. The outbound guard is deliberately generic. It walks the whole object graph rather than checking a known list of fields, because a checklist only ever catches the egress someone remembered, and that is how this defect recurred across four remediation rounds. A future fourth channel fails the tests without anyone adding it to a list. Closes the test asymmetry that let this through. Every prior test asked whether a credential could be constructed; none asked whether one could get out. Five tests now cover the outward direction, including the end-to-end replay and one that guards the guard by nesting a credential three levels down. Verified red-then-green: with the fix reverted the replay test reports the executor actually running under stolen authority, rather than a refusal that would have understated the impact. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The seam between the contract work and the credential work, and the one property neither could check on its own. The contract half validates the composed operation's metadata against a model whose principal fields are all required. The credential half replaced the credential in those fields with inert attribution, which copies each field only when present. They are present today only because the host mint validates all three at runtime, while the membrane principal type declares two of them optional. Relax that mint and the attribution silently drops a required field, the strict outbound assert throws on the SUCCESS path, and a confirmed operation reports itself rejected — the exact defect the contract half just fixed. Writing this test also caught a stale build artifact for the third time in this work: the contracts dist in the worktree predated the model the contract half added, so the assert reported an unknown model rather than a shape problem. The test failing loudly is the point; a green run against a stale dist is what hid the original defect for hours. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ssuer is pinned Two hardening items from the independent re-check, which cleared the escape question. The first is a fail-open this work introduced. Verifying a signed envelope now REGISTERS its principal, which is what makes it authority — but the trusted-issuer set is optional, with a long-standing default that accepts any structurally-valid signature. That default was harmless while a verification result was inert data. It is not harmless once verifying mints a credential: a face wiring the verifier without pinning issuers would hand a self-signing caller full authority, and the enforcement point's own registration re-check could not catch it, having been satisfied during verification. Promotion is now gated on a non-empty trust set. An unpinned envelope still verifies, so nothing reading the outcome changes, but its principal stays unregistered and is refused like any other shape-only value. Every real call site already pins issuers; only the verify function's own unit tests omit it, and those assert the outcome rather than registration. The second is a claim that outran its implementation. The outbound guard walked own enumerable properties only, while its comment promised that any future egress would fail the tests. It would have caught the three channels already found and nothing shaped differently — a class instance holding a credential behind a prototype accessor, an inherited field, a non-enumerable property. Since an overstated comment is exactly what let four remediation rounds pass, the walk now covers own and inherited, enumerable or not, string- and symbol-keyed, plus map keys. What it still cannot see is written down rather than left implied. Both verified by mutation. Without the issuer gate the executor RUNS under a self-signed principal; without the widened walk the guard's own test fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
b8a82be to
52af81c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/carrier-sdk/src/runtime-host.ts (1)
156-161: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winissue: removing
principalfromInProcessInvokeOptionsfalsifies theCarrierClient.invokedoc inpackages/carrier-sdk/src/sdk.ts.The removal itself is right. A caller can no longer supply identity, so the host owns it. That is the point of the PR.
The downstream effect is a stale contract.
packages/carrier-sdk/src/sdk.tsLines 140-147 still state that "the client'sactingAsprincipal is carried intoinvokeInProcess, which routes throughsecureInvoke". After this changeinvokeInProcessaccepts no principal, andcreateSecuredHullInvokemints the identity fromcurrentHostPrincipal(). A reader comparingCarrierClient.actingAsagainst the recorded decision principal will expect them to be the same value for the same reason, and that reasoning no longer holds.
InProcessInvokeOptionsis exported, so this is also a breaking type change for any external caller that passedprincipal.Update the
sdk.tsdoc to say the host composition root mints the invoke principal, and thatactingAscovers the CP propose/confirm path only.#!/bin/bash # Find callers that still pass a `principal` option to the in-process invoke path. rg -nP -C 4 'invokeInProcess\(|InProcessInvokeOptions' apps packages # Show the sdk.ts doc block that this removal invalidates. rg -nP -B 12 -A 8 'return invokeInProcess' packages/carrier-sdk/src/sdk.ts🤖 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 `@packages/carrier-sdk/src/runtime-host.ts` around lines 156 - 161, Update the CarrierClient.invoke documentation in sdk.ts to state that the host composition root mints the invoke principal, rather than carrying actingAs into invokeInProcess or secureInvoke; clarify that actingAs applies only to the CP propose/confirm path, while preserving the surrounding API documentation.
🧹 Nitpick comments (10)
apps/hull/src/membrane/pep.ts (1)
354-362: 🔒 Security & Privacy | 🔵 TrivialCP-touching security layer: flag for independent review.
secureInvokeis the membrane's central authentication/authorization chokepoint (principal registration, policy evaluation, confirmation consumption). Per the review policy, this class of change needs sign-off from an independent reviewer rather than author self-assertion.As per path instructions,
**requires: "Flag CP-touching changes (financial/audit/security/compliance/concurrency) as needing an independent reviewer, not self-assertion (A2)."🤖 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 `@apps/hull/src/membrane/pep.ts` around lines 354 - 362, Flag the change to secureInvoke for independent security review and sign-off, as it is the membrane’s authentication and authorization chokepoint; do not treat author self-assertion as sufficient approval.Source: Path instructions
apps/hull/src/membrane/pep.test.ts (1)
95-102: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuenit [non-blocking]:
brokerForConsumeris declared after its only writer.
tokenStorewrites tobrokerForConsumerat line 81, and theconstbinding is at line 95. The function declaration is hoisted, so a top-level call totokenStore()before line 102 would hit the temporal dead zone. Today every call happens inside anitcallback, so the code works. Move theWeakMapdeclaration abovetokenStoreto remove the ordering dependency.🤖 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 `@apps/hull/src/membrane/pep.test.ts` around lines 95 - 102, Move the brokerForConsumer WeakMap declaration above tokenStore, its only writer, so tokenStore cannot access the binding before initialization. Preserve the existing WeakMap type and contents unchanged.apps/hull/src/protocol/hull-port-adapter.test.ts (2)
286-302: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valuenit [non-blocking]: this call site passes the credential into the shell invoke context.
Line 294 hands
principal, the registered credential, tohull.invokeas the context. Production code increateSecuredHullInvokedeliberately passesprincipalAttribution(principal)instead, because the shell context reaches untrusted code.The executor never runs in this test, so nothing leaks. The example still models the pattern the PR removes. Use
principalAttribution(principal)so the test does not teach the wrong shape.🤖 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 `@apps/hull/src/protocol/hull-port-adapter.test.ts` around lines 286 - 302, Update the callback passed to secureInvoke in the “rejects a registry CP command without a broker token” test to pass principalAttribution(principal) as the hull.invoke context instead of the raw principal, matching createSecuredHullInvoke and ensuring the test models the safe context shape.
221-246: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winnit [non-blocking]: the
as nevercast defeats the stated intent of this test.The test is titled "composes the compatibility factory without accepting credential inputs". Line 226 casts the options object with
as never, so the type system checks nothing at this call. The object passed is already validSecuredHullInvokeOptions, so the cast is not needed.Remove it. The type-surface proof already lives in
apps/hull/src/protocol/hull-port-adapter.type-test.ts.♻️ Proposed fix
- const invoke = createSecuredHullInvoke({ - shell: trustedShell, - } as never) + const invoke = createSecuredHullInvoke({ shell: trustedShell })🤖 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 `@apps/hull/src/protocol/hull-port-adapter.test.ts` around lines 221 - 246, Remove the unnecessary `as never` cast from the options object passed to `createSecuredHullInvoke` in the compatibility factory test. Keep the existing options unchanged so TypeScript validates them as `SecuredHullInvokeOptions`, while leaving the type-surface coverage in `hull-port-adapter.type-test.ts` intact.apps/hull/src/membrane/signed-principal.ts (1)
56-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuethought [non-blocking]: this import makes verification a mutating operation.
signed-principal.tsnow has a write dependency on the credential registry. A reader ofverifySignedPrincipalexpects a pure predicate. The module header and theVerifySignedPrincipalOptionsdocs still describe verification as a pure check. Consider naming the effect in the function-level JSDoc so callers know thatverifySignedPrincipalcan grant authority.🤖 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 `@apps/hull/src/membrane/signed-principal.ts` at line 56, Document the credential-registry mutation in the function-level JSDoc for verifySignedPrincipal, explicitly stating that verification may register or grant authority through registerMembranePrincipal. Keep the existing pure-check documentation accurate by distinguishing this side effect from the predicate result.apps/hull/src/membrane/runtime-connection.ts (1)
70-71: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valuenit [non-blocking]: both stored values are shared mutable references.
shellProfilecomes from the caller, andmanifestcomes fromtransport.announce(). Neither is copied or frozen.ShellNegotiationProfiledeclares its fields withoutreadonly, andHullShellPortAdapter.negotiatereadsconnection.shellProfileto reconcile a later negotiation. A mutation afterconnectRuntimechanges negotiation outcomes for an already-connected runtime.Consider
Object.freezeon both, or store structural copies.🤖 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 `@apps/hull/src/membrane/runtime-connection.ts` around lines 70 - 71, In the connectRuntime state initialization, prevent later mutations of shellProfile and manifest from changing an established connection by storing frozen values or structural copies. Update the connection fields used by HullShellPortAdapter.negotiate, preserving the existing negotiation data and behavior.apps/hull/src/membrane/public-api.test.ts (2)
78-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winissue [non-blocking]: this assertion is skip-gated and can never fail on this branch.
publicHull.TrustedConfirmationBrokeris removed by this PR. The test at line 41 asserts exactly that. Sotypeof legacyBroker === 'function'is always false here, and the assertion at line 97 never runs.The review policy requires that real paths are verified and not skip-gated. A block that no CI run can enter provides no signal. The comment explains the intent, but the intent is served by the export assertion at lines 41-54, which does go red if the constructor returns.
Delete the block, or move it to a separate suite that runs against the pre-fix commit.
🤖 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 `@apps/hull/src/membrane/public-api.test.ts` around lines 78 - 98, Remove the skip-gated legacy TrustedConfirmationBroker test block from the current public API test, including its broker construction, secureInvoke call, and legacyResult assertion. Keep the existing export-removal assertion as the verification for this behavior; do not add replacement coverage in this suite.Source: Path instructions
270-294: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winpraise: this test is precise about a subtle mechanism.
The test works because
mintMembranePrincipalreturns a new frozen copy, sosubjectitself never enters the registry, and the envelope principal stays unregistered.suggestion [non-blocking]: add the positive half of the pinned-issuer gate.
State the falsification for
signed-principal.tslines 409-411. If the promotion branch is deleted, this test stays green, because it asserts rejection when no issuer is pinned. Nothing here asserts that a pinned issuer does promote the principal and allow execution.Add a test that passes
trustedIssuers: new Set([attacker.issuerId])and expectsexecuteto be called. That makes both directions of the gate falsifiable.🤖 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 `@apps/hull/src/membrane/public-api.test.ts` around lines 270 - 294, Extend the public API verification tests with a positive case alongside the existing untrusted-issuer rejection test: configure verifySignedPrincipal with trustedIssuers containing attacker.issuerId, then assert secureInvoke succeeds and execute is called. Reuse the existing attacker, signed principal, request, and execution setup as appropriate so the test specifically verifies trusted issuer promotion.Source: Path instructions
apps/hull/src/protocol/hull-port-adapter.ts (1)
129-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winnit [non-blocking]:
isContractVersionCompatible(schemaVersion, schemaVersion)compares a value with itself.The intent is a parse check, not a compatibility check. It works only because the predicate returns false for an unparseable string. A reader cannot see that from the call, and a future change that makes the predicate return true for equal inputs removes the validation without any test naming it.
Use an explicit version parser or add a named helper such as
isParseableContractVersion.🤖 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 `@apps/hull/src/protocol/hull-port-adapter.ts` around lines 129 - 140, Replace the self-comparison in the invalidSchema calculation with an explicit contract-version parseability check, using an existing parser or a clearly named helper such as isParseableContractVersion. Update the surrounding validation in the declaration capability-schema flow while preserving the current invalidSchema detection and compatibility result behavior.apps/hull/src/membrane/no-direct-invoke.arch.test.ts (1)
67-99: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winsuggestion: reuse the AST walker for brand-forgery detection instead of regex text matching.
brandForgeryOffendersmatchesBRAND_FORGERYagainstexecutableSource, a regex-based comment stripper (.replace(/\/\/[^\n]*|\/\*[\s\S]*?\*\//g, ''), Line 98). This file already builds a real AST for raw-invoke detection (hasStructuralBypass). A naive comment/string-agnostic regex can misfire inside string or template literals (stripping content that looks like a comment but is not) and cannot see through a cast performed via an intermediate type alias.Detecting
ts.isAsExpression(node)/ts.isTypeAssertionExpression(node)where the type node's text containsMembranePrincipalinside the existing AST walk would give the same enforcement mechanism (and the same test coverage) as the raw-invoke gate, closing the comment/string edge cases the regex approach cannot rule out.🤖 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 `@apps/hull/src/membrane/no-direct-invoke.arch.test.ts` around lines 67 - 99, Replace the regex-based BRAND_FORGERY and executableSource checks in brandForgeryOffenders with the existing AST-walking mechanism used by hasStructuralBypass. During traversal, detect ts.isAsExpression or ts.isTypeAssertionExpression nodes whose type refers to MembranePrincipal, while preserving the BRAND_MINTS exemption and offender-file results; remove the comment-stripping and regex-only detection path.
🤖 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 `@apps/carrier/src-tauri/hull-invoker.mjs`:
- Around line 70-81: Replace the outdated doc block above hostPrincipal() with
documentation describing hostPrincipal as the function that resolves the current
OS user identity. Move the host-side enforcement note to the cp-execute call
site immediately before runDemoCpOperation(note), where Hull-owned broker
enforcement is actually invoked.
In `@apps/hull/src/membrane/authority-registry.ts`:
- Around line 24-62: Make packaged runtime registry resolution in
loadPolicyStore fail visibly when no registry can be loaded instead of returning
EMPTY_STORE; retain the explicit sourceUrls parameter for tests and update
REGISTRY_URLS to resolve a registry that is actually included in the packaged
apps/hull output. Update credential-mints.test.ts to pass explicit registry URLs
through loadPolicyStore so tests remain deterministic and isolated from package
layout.
In `@apps/hull/src/membrane/composed.ts`:
- Around line 17-60: Wrap the entire runDemoCpOperation flow, including
currentHostPrincipal, broker token creation/confirmation, request construction,
and secureInvoke, in defensive error handling so no exception escapes. Preserve
secureInvoke’s existing CapabilityResult behavior, and convert any
pre-secureInvoke failure into a uniform failed CapabilityResult envelope
consistent with the established invoke error shape.
In `@apps/hull/src/membrane/credential-boundary.ts`:
- Around line 76-83: Update the Map and Set branches in the node-walking logic
to push their entries or values, then fall through to the existing own-property
traversal instead of continuing. Preserve the collection entry traversal while
ensuring own properties such as credential-bearing fields are also inspected.
In `@apps/hull/src/membrane/no-direct-invoke.arch.test.ts`:
- Line 57: Update hasStructuralBypass to select ts.ScriptKind.TSX for .tsx
sources before the existing .mjs and TypeScript kind selection; retain
ts.ScriptKind.TS for other non-MJS files so JSX is parsed correctly and shell
invokes after JSX remain detectable.
- Around line 130-165: Extend bindingIsSecureInvoke to recursively inspect
ObjectBindingPattern and ArrayBindingPattern nodes, including nested
BindingElements and aliased names, and return true when any bound identifier is
secureInvoke. Keep scopeShadowsSecureInvoke using this helper for variable
declarations and parameters so raw shell.invoke calls within
destructuring-shadowed scopes are rejected.
In `@apps/hull/src/membrane/node-bridge-principal.test.ts`:
- Around line 12-20: Update hostId() to reuse currentHostPrincipal() from
host-principal.ts as the source of the expected principal identity instead of
duplicating username derivation and fallback behavior. Preserve the test’s
intent while ensuring its expected value tracks currentHostPrincipal’s
formatting and fail-closed behavior.
In `@apps/hull/src/membrane/pep.test.ts`:
- Around line 74-128: Remove the test-local secureInvoke shim and migrate all
remaining call sites in this file to secureInvokeActual using its current
argument order, preserving each call’s intended options and broker handling. Do
not rewrite supplied confirmation evidence through brokerForConsumer aliases;
tests must exercise the production contract directly.
- Line 33: Change the ANON fixture to a plain object cast as Principal instead
of calling mintMembranePrincipal, so it exercises the shape-only and
unregistered path; retain mintMembranePrincipal for attributed fixtures.
Separately inspect mintMembranePrincipal and determine whether blank or
whitespace-only ids should be rejected at mint time, without changing that
behavior unless supported by the surrounding contract.
In `@apps/hull/src/membrane/pep.ts`:
- Around line 386-398: The rejection logic around verifyPrincipal must
distinguish a validly signed but unregistered principal from a malformed or
failed verification. Update UnverifiedPrincipalError’s reason type and the
reason selection in this block so verification.ok with a non-registered
verification.principal reports an explicit unregistered reason, while preserving
the existing malformed and verification.reason paths.
In `@apps/hull/src/membrane/signed-principal.ts`:
- Around line 394-411: Update verifySignedPrincipal so successful verification
snapshots envelope.principal before registration, rather than registering the
caller-provided object directly. Use the snapshot for registerMembranePrincipal
and return it as the verified principal, preserving the existing trustedIssuers
gate and identity-based checks.
In `@apps/hull/src/protocol/hull-port-adapter.test.ts`:
- Around line 94-107: Update the negotiation logic exercised by
HullShellPortAdapter.negotiate so an unsupported capability schema major is
reported in the response reason, while preserving compatible: true and the empty
acceptedCapabilities result. Include the capability name and declared versus
supported schema versions, and update the corresponding test expectation to
assert the descriptive reason instead of null.
In `@apps/hull/src/protocol/hull-port-adapter.ts`:
- Around line 163-184: Apply inbound protocol validation consistently across
HullShellPortAdapter: in hull-port-adapter.ts, call parseInboundProtocolModel at
the start of secure, invoke, address, observe, resolve, and compose, then use
each parsed payload; remove invoke’s as unknown as CapabilityCore cast after
validation. In hull-port-adapter.test.ts, add an adapter.invoke rejection test
for a missing idempotencyKey, asserting code protocol.invalid_inbound_payload,
direction inbound, and operationId hull.invoke; the test must fail if invoke’s
parse call is removed.
In `@apps/hull/src/protocol/hull-port-adapter.type-test.ts`:
- Around line 51-55: Remove the unused _command parameter from the
policyDecisionPoint function in rejectedPolicyOption while retaining the
function expression and `@ts-expect-error` directive so the excess-property type
error remains covered.
In `@apps/hull/tsconfig.test.json`:
- Around line 15-19: Update the test typecheck configuration’s include scope to
cover both src/protocol and src/membrane, while explicitly excluding the
deferred cpu-image-floor-runtime, cpu-image-floor-smoke, and runtime-connection
suites. Keep the existing runtime backlog deferral note accurate, and ensure the
pnpm typecheck gate now fails when a membrane test uses an invalid
AuthorityDecision field.
In `@packages/carrier-sdk/src/cp-rejection.test.ts`:
- Around line 72-100: Replace the duplicated secureInvoke shim with one shared
helper that uses real TrustedConfirmationBroker evidence and passes forged
confirmations unchanged, preserving no-confirmation rejection cases. In
packages/carrier-sdk/src/cp-rejection.test.ts#L72-L100, use the shared helper;
in packages/carrier-sdk/src/cp-notification-loop.test.ts#L89-L117, remove the
duplicate and stop passing confirmedBy to trusted.confirm. Remove the deep
issuer imports in packages/carrier-sdk/src/cp-rejection.test.ts#L30-L32 and
packages/carrier-sdk/src/cp-notification-loop.test.ts#L44-L46, routing remaining
imports through Hull’s package exports. Ensure CI exercises both suites and flag
the CP-touching change for independent review.
In `@packages/carrier-sdk/src/sdk.test.ts`:
- Around line 208-216: Keep the existing caller-supplied principal test, and
restore a separate SDK-level test covering an anonymous or unregistered
principal being rejected fail-closed. Exercise the rejection through
invokeInProcess and assert the operation does not succeed and that the decision
sink records no decision, preserving the prior SDK behavior and ensuring the CI
suite runs this guard.
In `@packages/carrier-sdk/src/sdk.ts`:
- Around line 155-156: Correct the misleading identity-protection documentation
around the SDK invocation: update the docblock near the caller options to state
the actual behavior, and remove the `{ ...opts }` spread in the
`invokeInProcess` call or describe it only as a shallow copy, since it does not
sanitize caller keys. Preserve the identity enforcement implemented by
`invokeInProcess` and `createSecuredHullInvoke`.
---
Outside diff comments:
In `@packages/carrier-sdk/src/runtime-host.ts`:
- Around line 156-161: Update the CarrierClient.invoke documentation in sdk.ts
to state that the host composition root mints the invoke principal, rather than
carrying actingAs into invokeInProcess or secureInvoke; clarify that actingAs
applies only to the CP propose/confirm path, while preserving the surrounding
API documentation.
---
Nitpick comments:
In `@apps/hull/src/membrane/no-direct-invoke.arch.test.ts`:
- Around line 67-99: Replace the regex-based BRAND_FORGERY and executableSource
checks in brandForgeryOffenders with the existing AST-walking mechanism used by
hasStructuralBypass. During traversal, detect ts.isAsExpression or
ts.isTypeAssertionExpression nodes whose type refers to MembranePrincipal, while
preserving the BRAND_MINTS exemption and offender-file results; remove the
comment-stripping and regex-only detection path.
In `@apps/hull/src/membrane/pep.test.ts`:
- Around line 95-102: Move the brokerForConsumer WeakMap declaration above
tokenStore, its only writer, so tokenStore cannot access the binding before
initialization. Preserve the existing WeakMap type and contents unchanged.
In `@apps/hull/src/membrane/pep.ts`:
- Around line 354-362: Flag the change to secureInvoke for independent security
review and sign-off, as it is the membrane’s authentication and authorization
chokepoint; do not treat author self-assertion as sufficient approval.
In `@apps/hull/src/membrane/public-api.test.ts`:
- Around line 78-98: Remove the skip-gated legacy TrustedConfirmationBroker test
block from the current public API test, including its broker construction,
secureInvoke call, and legacyResult assertion. Keep the existing export-removal
assertion as the verification for this behavior; do not add replacement coverage
in this suite.
- Around line 270-294: Extend the public API verification tests with a positive
case alongside the existing untrusted-issuer rejection test: configure
verifySignedPrincipal with trustedIssuers containing attacker.issuerId, then
assert secureInvoke succeeds and execute is called. Reuse the existing attacker,
signed principal, request, and execution setup as appropriate so the test
specifically verifies trusted issuer promotion.
In `@apps/hull/src/membrane/runtime-connection.ts`:
- Around line 70-71: In the connectRuntime state initialization, prevent later
mutations of shellProfile and manifest from changing an established connection
by storing frozen values or structural copies. Update the connection fields used
by HullShellPortAdapter.negotiate, preserving the existing negotiation data and
behavior.
In `@apps/hull/src/membrane/signed-principal.ts`:
- Line 56: Document the credential-registry mutation in the function-level JSDoc
for verifySignedPrincipal, explicitly stating that verification may register or
grant authority through registerMembranePrincipal. Keep the existing pure-check
documentation accurate by distinguishing this side effect from the predicate
result.
In `@apps/hull/src/protocol/hull-port-adapter.test.ts`:
- Around line 286-302: Update the callback passed to secureInvoke in the
“rejects a registry CP command without a broker token” test to pass
principalAttribution(principal) as the hull.invoke context instead of the raw
principal, matching createSecuredHullInvoke and ensuring the test models the
safe context shape.
- Around line 221-246: Remove the unnecessary `as never` cast from the options
object passed to `createSecuredHullInvoke` in the compatibility factory test.
Keep the existing options unchanged so TypeScript validates them as
`SecuredHullInvokeOptions`, while leaving the type-surface coverage in
`hull-port-adapter.type-test.ts` intact.
In `@apps/hull/src/protocol/hull-port-adapter.ts`:
- Around line 129-140: Replace the self-comparison in the invalidSchema
calculation with an explicit contract-version parseability check, using an
existing parser or a clearly named helper such as isParseableContractVersion.
Update the surrounding validation in the declaration capability-schema flow
while preserving the current invalidSchema detection and compatibility result
behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b9a4a096-ecb2-4e37-bdd0-782a168dcd4d
📒 Files selected for processing (40)
apps/carrier/src-tauri/hull-invoker.mjsapps/hull/package.jsonapps/hull/src/index.tsapps/hull/src/membrane/authority-registry.tsapps/hull/src/membrane/composed.tsapps/hull/src/membrane/credential-boundary.tsapps/hull/src/membrane/credential-issuer.tsapps/hull/src/membrane/credential-mints.test.tsapps/hull/src/membrane/host-principal.tsapps/hull/src/membrane/invoke.tsapps/hull/src/membrane/no-direct-invoke.arch.test.tsapps/hull/src/membrane/node-bridge-principal.test.tsapps/hull/src/membrane/pep.test.tsapps/hull/src/membrane/pep.tsapps/hull/src/membrane/public-api.test.tsapps/hull/src/membrane/runtime-connection.tsapps/hull/src/membrane/signed-principal.tsapps/hull/src/protocol/hull-port-adapter.test.tsapps/hull/src/protocol/hull-port-adapter.tsapps/hull/src/protocol/hull-port-adapter.type-test.tsapps/hull/tsconfig.test.jsondocs/adrs/0162-language-agnostic-carrier-hull-contracts.mdicm/00_intake/output/intake-carrier-language-agnostic-adapters-2026-08-01.mdicm/01_discovery/output/discovery-carrier-language-agnostic-adapters-2026-08-01.mdicm/02_architecture/output/architecture-carrier-language-agnostic-adapters-2026-08-01.mdicm/03_package-design/output/package-design-carrier-language-agnostic-adapters-2026-08-01.mdicm/05_implementation-plan/output/carrier-language-agnostic-adapters-2026-08-01.mdicm/06_build/output/carrier-language-agnostic-adapters-2026-08-01.mdicm/07_review/output/carrier-language-agnostic-adapters-review-2026-08-01.mdicm/08_release/output/carrier-language-agnostic-adapters-release-2026-08-01.mdpackages/carrier-sdk/src/authority.tspackages/carrier-sdk/src/command-authority-parity.test.tspackages/carrier-sdk/src/cp-notification-loop.test.tspackages/carrier-sdk/src/cp-rejection.test.tspackages/carrier-sdk/src/principal.tspackages/carrier-sdk/src/runtime-host.tspackages/carrier-sdk/src/sdk.test.tspackages/carrier-sdk/src/sdk.tspackages/contracts/protocol/README.mdtooling/carrier-contract-codegen/tests/generate.test.mjs
| /** | ||
| * A minimal ProposalBroker for the bridge's `cp-execute` op. | ||
| * | ||
| * The bridge is single-request-per-spawn — it cannot share a broker instance with | ||
| * the renderer across separate spawns. For the `cp-execute` op we run a FULL | ||
| * propose → consume cycle IN THIS PROCESS so the real `secureInvoke` TokenConsumer | ||
| * exercises the load-bearing token-binding check (P1b-1). | ||
| * propose → confirm → consume cycle IN THIS PROCESS so the real `secureInvoke` | ||
| * broker check exercises the load-bearing token-binding check (P1b-1). | ||
| * | ||
| * This is NOT a separate gate from the renderer-side UX proposal: it is the | ||
| * HOST-SIDE ENFORCEMENT proof — the same mechanism Sunfish's real CP ops (post JE, | ||
| * void payment) will use when the carrier-sdk is the execution face. | ||
| */ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
issue: this doc block describes a broker that no longer exists in this file, and it is now attached to hostPrincipal.
The comment states "A minimal ProposalBroker for the bridge's cp-execute op" and describes a propose → confirm → consume cycle run "IN THIS PROCESS". This PR removed that broker from the bridge. cp-execute now calls runDemoCpOperation(note) at Line 313, and Hull owns the broker. The function under this comment is hostPrincipal(), which resolves the OS user. A reader of a security-relevant bridge is told the wrong location for the enforcement.
Replace the block with a description of hostPrincipal, and move the enforcement note to Line 311.
📝 Proposed doc replacement
-/**
- * A minimal ProposalBroker for the bridge's `cp-execute` op.
- *
- * The bridge is single-request-per-spawn — it cannot share a broker instance with
- * the renderer across separate spawns. For the `cp-execute` op we run a FULL
- * propose → confirm → consume cycle IN THIS PROCESS so the real `secureInvoke`
- * broker check exercises the load-bearing token-binding check (P1b-1).
- *
- * This is NOT a separate gate from the renderer-side UX proposal: it is the
- * HOST-SIDE ENFORCEMENT proof — the same mechanism Sunfish's real CP ops (post JE,
- * void payment) will use when the carrier-sdk is the execution face.
- */
+/**
+ * Resolve the local OS user as this process's host identity, or `null` when the
+ * OS reports no usable username.
+ *
+ * This value is a PRECONDITION probe only. Hull's `currentHostPrincipal()` mints the
+ * principal that the PEP authenticates; nothing here is passed across the membrane.
+ */🤖 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 `@apps/carrier/src-tauri/hull-invoker.mjs` around lines 70 - 81, Replace the
outdated doc block above hostPrincipal() with documentation describing
hostPrincipal as the function that resolves the current OS user identity. Move
the host-side enforcement note to the cp-execute call site immediately before
runDemoCpOperation(note), where Hull-owned broker enforcement is actually
invoked.
| /** The source and generated copies used by the carrier build. Source is preferred. */ | ||
| const REGISTRY_URLS = [ | ||
| new URL('../../../../packages/carrier-sdk/src/command-authority.json', import.meta.url), | ||
| new URL('../../../../packages/carrier-sdk/dist/command-authority.json', import.meta.url), | ||
| ] | ||
|
|
||
| const EMPTY_STORE: PolicyStore = Object.freeze({ | ||
| commands: Object.freeze(Object.create(null) as Record<string, Readonly<AuthorityRegistryEntry>>), | ||
| readable: false, | ||
| }) | ||
|
|
||
| /** Read and freeze the canonical registry; an unavailable registry means every command is CP. */ | ||
| export function loadPolicyStore(sourceUrls: readonly URL[] = REGISTRY_URLS): PolicyStore { | ||
| for (const url of sourceUrls) { | ||
| try { | ||
| const parsed = JSON.parse(readFileSync(fileURLToPath(url), 'utf8')) as AuthorityRegistryFile | ||
| if (parsed.commands == null || typeof parsed.commands !== 'object') continue | ||
| const commands: Record<string, Readonly<AuthorityRegistryEntry>> = Object.create(null) | ||
| for (const [command, entry] of Object.entries(parsed.commands)) { | ||
| if ( | ||
| entry == null | ||
| || typeof entry !== 'object' | ||
| || (entry.authority !== 'AP' && entry.authority !== 'CP') | ||
| || typeof entry.summary !== 'string' | ||
| ) { | ||
| throw new TypeError(`invalid authority entry for '${command}'`) | ||
| } | ||
| commands[command] = Object.freeze({ | ||
| authority: entry.authority, | ||
| summary: entry.summary, | ||
| }) | ||
| } | ||
| return Object.freeze({ commands: Object.freeze(commands), readable: true }) | ||
| } catch { | ||
| // Try the generated copy before failing closed. | ||
| } | ||
| } | ||
| return EMPTY_STORE | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Query the coding/KB context for runtime path/packaging expectations if available.
if command -v pnpm >/dev/null 2>&1; then
pnpm kb query "packaging runtime path resolution CARGO_MANIFEST_DIR import.meta.url apps/hull packages/carrier-sdk" || true
else
echo "pnpm not available"
fi
echo
echo "Repository files of interest:"
git ls-files | rg '(^|/)(authority-registry\.ts|credential-mints\.test\.ts|command-authority\.json|.*config.*|package\.json|pnpm-workspace\.yaml)$' | sed -n '1,200p'
echo
echo "authority-registry.ts outline:"
ast-grep outline apps/hull/src/membrane/authority-registry.ts --view expanded || true
echo
echo "authority-registry.ts relevant content:"
cat -n apps/hull/src/membrane/authority-registry.ts | sed -n '1,140p'
echo
echo "credential-mints.test.ts relevant content:"
cat -n apps/hull/src/membrane/credential-mints.test.ts | sed -n '1,120p'
echo
echo "Search for loadPolicyStore/sourceUrls usages:"
rg -n "loadPolicyStore\(|sourceUrls|REGISTRY_URLS|command-authority\.json" apps/hull packages/carrier-sdk 2>/dev/null || trueRepository: Harborline-Software/shipyard
Length of output: 15889
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect import/export and build-related config references:"
rg -n "import\.meta\.url|__dirname|__filename|fileURLToPath|sourceUrls|loadPolicyStore|command-authority\.json|dist/command-authority|src/command-authority" -g '!**/node_modules/**' -g '!**/.git/**' . | sed -n '1,240p'
echo
echo "Workspace/package hints:"
for f in pnpm-workspace.yaml package.json apps/hull/package.json packages/carrier-sdk/package.json; do
if [ -f "$f" ]; then
echo "--- $f ---"
sed -n '1,200p' "$f"
fi
doneRepository: Harborline-Software/shipyard
Length of output: 43920
issue [blocking]: Make registry path resolution robust for packaged apps/hull. REGISTRY_URLS hardcodes a relative offset from import.meta.url, but apps/hull package files only publish dist and do not publish or rebuild packages/carrier-sdk. If every configured URL fails, policy loads silently fail-closed with all commands treated as CP. Change loadPolicyStore() to accept explicit sourceUrls in tests and make packaged runtime resolution fail after a visible registry load error.
📍 Affects 2 files
apps/hull/src/membrane/authority-registry.ts#L24-L62(this comment)apps/hull/src/membrane/credential-mints.test.ts#L8-L16
🤖 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 `@apps/hull/src/membrane/authority-registry.ts` around lines 24 - 62, Make
packaged runtime registry resolution in loadPolicyStore fail visibly when no
registry can be loaded instead of returning EMPTY_STORE; retain the explicit
sourceUrls parameter for tests and update REGISTRY_URLS to resolve a registry
that is actually included in the packaged apps/hull output. Update
credential-mints.test.ts to pass explicit registry URLs through loadPolicyStore
so tests remain deterministic and isolated from package layout.
Source: Path instructions
| export async function runDemoCpOperation(note = 'demo-cp-op from the Carrier UI'): Promise<CapabilityResult> { | ||
| const principal = currentHostPrincipal() | ||
| const attribution = principalAttribution(principal) | ||
| const broker = new TrustedConfirmationBroker() | ||
| const command = 'demo-cp-op' | ||
| const token = broker.propose(command) | ||
| const confirmation = broker.confirm(token, command, principal) | ||
| const correlationId = `carrier-demo-cp-op-${globalThis.crypto?.randomUUID?.() ?? Date.now()}` | ||
| const request: InvokeRequest = { | ||
| capabilityId: 'tts', | ||
| core: { text: note, voice: null, format: 'aiff', timeout: 30_000 } as never, | ||
| providerInputs: {}, | ||
| attachments: [], | ||
| idempotencyKey: `idem-${correlationId}`, | ||
| correlationId, | ||
| transport: 'sync', | ||
| } | ||
|
|
||
| return secureInvoke( | ||
| { command, capabilityId: 'tts', request, confirmation }, | ||
| principal, | ||
| async () => ({ | ||
| jobId: `job:demo-cp-op:${correlationId}`, | ||
| status: 'succeeded', | ||
| progress: 1, | ||
| artifacts: [], | ||
| usage: { unit: 'call', quantity: 1, tier: 'local' }, | ||
| error: null, | ||
| meta: { | ||
| command, | ||
| confirmed: true, | ||
| note, | ||
| executedAt: new Date().toISOString(), | ||
| // INERT attribution. `secureInvoke` returns the executor's result unmodified, so anything | ||
| // placed here reaches every caller — and the credential is a bearer token the PEP would | ||
| // accept back for any command. Attribution is what a result needs; authority is not. | ||
| proposedBy: attribution, | ||
| confirmedBy: attribution, | ||
| }, | ||
| }), | ||
| {}, | ||
| broker, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Wrap runDemoCpOperation so it cannot throw past secureInvoke's uniform-envelope guarantee.
currentHostPrincipal(), broker.propose(), and broker.confirm() all run before secureInvoke is called and can each throw (unavailable/blank OS username, missing crypto.randomUUID, or an unknown/mismatched confirmation token). secureInvoke itself never throws and always returns a CapabilityResult envelope (the M3 promise documented in pep.ts), but runDemoCpOperation can reject before reaching it. The 'invoke' op path in apps/carrier/src-tauri/hull-invoker.mjs explicitly wraps its chokepoint "defensively so any unexpected throw becomes a uniform envelope on stdout rather than an exit-without-output"; this composition root does not get the same treatment, so a rare host-identity or broker fault here can surface as an unhandled rejection instead of a uniform failure result.
🛡️ Proposed fix: contain pre-`secureInvoke` faults in a uniform failure envelope
export async function runDemoCpOperation(note = 'demo-cp-op from the Carrier UI'): Promise<CapabilityResult> {
- const principal = currentHostPrincipal()
- const attribution = principalAttribution(principal)
- const broker = new TrustedConfirmationBroker()
- const command = 'demo-cp-op'
- const token = broker.propose(command)
- const confirmation = broker.confirm(token, command, principal)
- const correlationId = `carrier-demo-cp-op-${globalThis.crypto?.randomUUID?.() ?? Date.now()}`
+ const correlationId = `carrier-demo-cp-op-${globalThis.crypto?.randomUUID?.() ?? Date.now()}`
+ let principal, attribution, broker, confirmation
+ const command = 'demo-cp-op'
+ try {
+ principal = currentHostPrincipal()
+ attribution = principalAttribution(principal)
+ broker = new TrustedConfirmationBroker()
+ const token = broker.propose(command)
+ confirmation = broker.confirm(token, command, principal)
+ } catch (err) {
+ return {
+ jobId: `job:demo-cp-op:${correlationId}`,
+ status: 'failed',
+ progress: 0,
+ artifacts: [],
+ usage: { unit: 'call', quantity: 0, tier: 'local' },
+ error: {
+ faultDomain: 'membrane',
+ retryable: false,
+ code: 'membrane.demo_cp_op_setup_fault',
+ message: err instanceof Error ? err.message : String(err),
+ },
+ }
+ }
const request: InvokeRequest = {
...📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function runDemoCpOperation(note = 'demo-cp-op from the Carrier UI'): Promise<CapabilityResult> { | |
| const principal = currentHostPrincipal() | |
| const attribution = principalAttribution(principal) | |
| const broker = new TrustedConfirmationBroker() | |
| const command = 'demo-cp-op' | |
| const token = broker.propose(command) | |
| const confirmation = broker.confirm(token, command, principal) | |
| const correlationId = `carrier-demo-cp-op-${globalThis.crypto?.randomUUID?.() ?? Date.now()}` | |
| const request: InvokeRequest = { | |
| capabilityId: 'tts', | |
| core: { text: note, voice: null, format: 'aiff', timeout: 30_000 } as never, | |
| providerInputs: {}, | |
| attachments: [], | |
| idempotencyKey: `idem-${correlationId}`, | |
| correlationId, | |
| transport: 'sync', | |
| } | |
| return secureInvoke( | |
| { command, capabilityId: 'tts', request, confirmation }, | |
| principal, | |
| async () => ({ | |
| jobId: `job:demo-cp-op:${correlationId}`, | |
| status: 'succeeded', | |
| progress: 1, | |
| artifacts: [], | |
| usage: { unit: 'call', quantity: 1, tier: 'local' }, | |
| error: null, | |
| meta: { | |
| command, | |
| confirmed: true, | |
| note, | |
| executedAt: new Date().toISOString(), | |
| // INERT attribution. `secureInvoke` returns the executor's result unmodified, so anything | |
| // placed here reaches every caller — and the credential is a bearer token the PEP would | |
| // accept back for any command. Attribution is what a result needs; authority is not. | |
| proposedBy: attribution, | |
| confirmedBy: attribution, | |
| }, | |
| }), | |
| {}, | |
| broker, | |
| ) | |
| } | |
| export async function runDemoCpOperation(note = 'demo-cp-op from the Carrier UI'): Promise<CapabilityResult> { | |
| const correlationId = `carrier-demo-cp-op-${globalThis.crypto?.randomUUID?.() ?? Date.now()}` | |
| let principal, attribution, broker, confirmation | |
| const command = 'demo-cp-op' | |
| try { | |
| principal = currentHostPrincipal() | |
| attribution = principalAttribution(principal) | |
| broker = new TrustedConfirmationBroker() | |
| const token = broker.propose(command) | |
| confirmation = broker.confirm(token, command, principal) | |
| } catch (err) { | |
| return { | |
| jobId: `job:demo-cp-op:${correlationId}`, | |
| status: 'failed', | |
| progress: 0, | |
| artifacts: [], | |
| usage: { unit: 'call', quantity: 0, tier: 'local' }, | |
| error: { | |
| faultDomain: 'membrane', | |
| retryable: false, | |
| code: 'membrane.demo_cp_op_setup_fault', | |
| message: err instanceof Error ? err.message : String(err), | |
| }, | |
| } | |
| } | |
| const request: InvokeRequest = { | |
| capabilityId: 'tts', | |
| core: { text: note, voice: null, format: 'aiff', timeout: 30_000 } as never, | |
| providerInputs: {}, | |
| attachments: [], | |
| idempotencyKey: `idem-${correlationId}`, | |
| correlationId, | |
| transport: 'sync', | |
| } | |
| return secureInvoke( | |
| { command, capabilityId: 'tts', request, confirmation }, | |
| principal, | |
| async () => ({ | |
| jobId: `job:demo-cp-op:${correlationId}`, | |
| status: 'succeeded', | |
| progress: 1, | |
| artifacts: [], | |
| usage: { unit: 'call', quantity: 1, tier: 'local' }, | |
| error: null, | |
| meta: { | |
| command, | |
| confirmed: true, | |
| note, | |
| executedAt: new Date().toISOString(), | |
| // INERT attribution. `secureInvoke` returns the executor's result unmodified, so anything | |
| // placed here reaches every caller — and the credential is a bearer token the PEP would | |
| // accept back for any command. Attribution is what a result needs; authority is not. | |
| proposedBy: attribution, | |
| confirmedBy: attribution, | |
| }, | |
| }), | |
| {}, | |
| broker, | |
| ) | |
| } |
🤖 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 `@apps/hull/src/membrane/composed.ts` around lines 17 - 60, Wrap the entire
runDemoCpOperation flow, including currentHostPrincipal, broker token
creation/confirmation, request construction, and secureInvoke, in defensive
error handling so no exception escapes. Preserve secureInvoke’s existing
CapabilityResult behavior, and convert any pre-secureInvoke failure into a
uniform failed CapabilityResult envelope consistent with the established invoke
error shape.
| if (node instanceof Map) { | ||
| for (const [k, v] of node) stack.push(k, v) | ||
| continue | ||
| } | ||
| if (node instanceof Set) { | ||
| for (const v of node) stack.push(v) | ||
| continue | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
suggestion: also walk own properties of Map and Set nodes.
The continue statements at Line 78 and Line 82 stop the walk after the entries. A Map or Set that carries a credential on an own property, for example const m = new Map(); m.principal = cred, is not reported. The module doc states the guard is deliberately generic so a future egress fails the tests without anyone adding a field to a list. This gap narrows that guarantee.
🛡️ Proposed fix: push entries, then fall through to the property walk
if (node instanceof Map) {
for (const [k, v] of node) stack.push(k, v)
- continue
}
- if (node instanceof Set) {
+ else if (node instanceof Set) {
for (const v of node) stack.push(v)
- continue
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (node instanceof Map) { | |
| for (const [k, v] of node) stack.push(k, v) | |
| continue | |
| } | |
| if (node instanceof Set) { | |
| for (const v of node) stack.push(v) | |
| continue | |
| } | |
| if (node instanceof Map) { | |
| for (const [k, v] of node) stack.push(k, v) | |
| } | |
| else if (node instanceof Set) { | |
| for (const v of node) stack.push(v) | |
| } |
🤖 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 `@apps/hull/src/membrane/credential-boundary.ts` around lines 76 - 83, Update
the Map and Set branches in the node-walking logic to push their entries or
values, then fall through to the existing own-property traversal instead of
continuing. Preserve the collection entry traversal while ensuring own
properties such as credential-bearing fields are also inspected.
|
|
||
| /** Source extensions that can call the membrane (TS faces + the .mjs bridge). */ | ||
| const SOURCE_EXT = /\.(ts|mjs)$/ | ||
| const SOURCE_EXT = /\.(ts|tsx|mjs)$/i |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
TypeScript createSourceFile ts.ScriptKind.TS parses JSX syntax <Foo> as type assertion
💡 Result:
When using ts.createSourceFile with ts.ScriptKind.TS, the TypeScript parser does not support JSX syntax [1][2]. Because both JSX elements (e.g., <Foo>) and traditional TypeScript type assertions (e.g., <Type>value) share the same angle-bracket syntax, they are inherently ambiguous [2][3][4]. To prevent this ambiguity, the TypeScript compiler strictly separates the parsing modes [2][3]: 1. ScriptKind.TS: The parser treats angle-bracket constructs as type assertions [2]. It is not configured to scan for or parse JSX elements [5]. 2. ScriptKind.TSX: The parser is explicitly enabled to handle JSX syntax [5]. Because JSX is enabled, the compiler disallows the use of <Type>value type assertions to avoid confusion, requiring the use of the as operator (e.g., value as Type) instead [2][3]. If you provide a string containing JSX syntax (e.g., const x = <Foo />;) to ts.createSourceFile with ts.ScriptKind.TS, the parser will fail to interpret the JSX correctly because it expects a type assertion or a different expression structure [6]. This often results in syntax errors, as the parser interprets the < as the start of a type assertion and then fails when it encounters tokens (like / or tags) that do not conform to valid type assertion syntax [6][7]. To parse files containing JSX, you must use ts.ScriptKind.TSX [8]. Note that if you use TSX, you must also refactor any legacy <Type>value assertions in your code to value as Type [2][3].
Citations:
- 1: Enable JSX syntax in .ts files microsoft/TypeScript#26489
- 2: https://www.typescriptlang.org/docs/handbook/jsx.html
- 3: https://github.com/Microsoft/TypeScript-Handbook/blob/master/pages/JSX.md
- 4: Allow JSX without using
.tsxextension microsoft/TypeScript#30503 - 5: https://github.com/microsoft/TypeScript-Compiler-Notes/blob/main/codebase/src/compiler/scanner.md
- 6: Specialized parsing error for apparent JSX tag in .ts file microsoft/TypeScript#56101
- 7: Fix parsing of generic type parameters in arrow functions with JSX syntax microsoft/TypeScript#59485
- 8: createSourceFile generates incorrect AST microsoft/TypeScript#32249
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repository files matching no-direct-invoke:\n'
fd -a 'no-direct-invoke\.arch\.test\.ts$' . || true
printf '\nTarget file excerpt:\n'
if [ -f apps/hull/src/membrane/no-direct-invoke.arch.test.ts ]; then
wc -l apps/hull/src/membrane/no-direct-invoke.arch.test.ts
sed -n '1,260p' apps/hull/src/membrane/no-direct-invoke.arch.test.ts | cat -n
fi
printf '\nTypeScript availability/version:\n'
if command -v node >/dev/null 2>&1; then
node -e "const ts=require('typescript'); console.log(ts.version, Object.prototype.toString.call(ts));"
fi
printf '\nProbe parser behavior if TypeScript is available:\n'
node - <<'JS'
try {
const ts = require('typescript');
const tsScriptKindTS = ts.ScriptKind.TS;
const tsScriptKindJSX = ts.ScriptKind.JSX;
const tsScriptKindTSX = ts.ScriptKind.TSX;
const jsFile = 'x.tsx';
const jsCode = '<Foo />\n';
const results = {
TS: null,
JSX: null,
TSX: null,
};
for (const kind of ['TS', 'JSX', 'TSX']) {
try {
const sf = ts.createSourceFile(jsFile, jsCode, ts.ScriptTarget.ES2015, true, ts(kind));
results[kind] = {
sourceFileErrorCount: sf.parseDiagnostics.length,
diagnostics: sf.parseDiagnostics.map(d => ({
code: d.code,
messageChain: d.messageChain ? d.messageChain.messageText : null,
})),
};
} catch (e) {
results[kind] = { error: String(e && e.message ? e.message : e) };
}
}
console.log(JSON.stringify(results, null, 2));
const mixedJSXAndTypeAssert = jsFile + ' contains JSX+type assertion parsed as TS:\n';
let mixedCode = 'let x = <Foo />\nlet y = <string>123\n';
try {
const sf = ts.createSourceFile(jsFile, mixedCode, ts.ScriptTarget.ES2015, true, ts.ScriptKind.TS);
console.log('mixed TS parseErrors:', sf.parseDiagnostics.length);
for (const d of sf.parseDiagnostics) {
console.log(d.code + ': ' + ts.flattenDiagnosticMessageText(d.messageChain && d.messageChain.messageText, '\n'));
}
} catch (e) {
console.log('mixed TS error:', e && e.message ? e.message : e);
}
const bypassLikeCode = '<Foo />\nconst secureInvokeBypass = createUnauthorizedHullInvoke()\n';
const sf = ts.createSourceFile(jsFile, bypassLikeCode, ts.ScriptTarget.ES2015, true, ts.ScriptKind.TSX);
console.log('bypassLike TSX parseErrors:', sf.parseDiagnostics.length);
const body = sf.statements.map(s => s.getText());
console.log(body.join('\n'));
} catch (e) {
console.log('typescript not available or probe failed:', String(e && e.message ? e.message : e));
}
JSRepository: Harborline-Software/shipyard
Length of output: 12788
🌐 Web query:
TypeScript 4.0 createSourceFile .tsx file ScriptKind TypeScript compiler API .ScriptKind.TSX
💡 Result:
To parse a.tsx file using the TypeScript Compiler API, you should use the ts.createSourceFile function and explicitly pass ts.ScriptKind.TSX as the final argument [1][2]. The ts.createSourceFile function signature is as follows: ts.createSourceFile(fileName: string, sourceText: string, languageVersion: ScriptTarget, setParentNodes?: boolean, scriptKind?: ScriptKind): SourceFile When working with.tsx files, providing the ScriptKind is necessary to inform the parser that the input contains JSX syntax, ensuring the file is parsed correctly even if the file extension does not match or if you are parsing raw strings [1][3]. Example usage: import * as ts from 'typescript'; const sourceFile = ts.createSourceFile( 'example.tsx', 'const component =
Citations:
- 1: https://microsoft-typescript.mintlify.app/api/parser
- 2: https://stackoverflow.com/questions/50942026/find-out-which-identifiers-are-unresolved-in-a-tsx-script
- 3: https://github.com/typescript-eslint/typescript-eslint/blob/e7f5e36ecb65002c1ab1e67359cab1527192c471/packages/typescript-estree/src/create-program/createSourceFile.ts
- 4: https://github.com/Microsoft/TypeScript/blob/d8d5dafe1a701a0b9466e53637f0ff3c63d55835/src/compiler/core.ts
🌐 Web query:
TypeScript compiler createsourcefile file extension scriptKind inference ts.ScriptKind.TSX ts.ScriptKind.TS
💡 Result:
When using the TypeScript Compiler API, the ts.createSourceFile function accepts an optional scriptKind parameter to explicitly define the nature of the source file (e.g., whether it contains TypeScript, JavaScript, or JSX) [1][2]. ScriptKind Inference and Behavior: 1. Manual Specification: You can explicitly provide a ts.ScriptKind value (such as ts.ScriptKind.TS, ts.ScriptKind.TSX, ts.ScriptKind.JS, or ts.ScriptKind.JSX) as the final argument to ts.createSourceFile [1]. This is the recommended approach if the file extension is ambiguous or if you want to ensure consistent parsing behavior regardless of the file name [3]. 2. Automatic Inference: If you do not provide a scriptKind argument, the compiler does not always automatically infer the correct kind based solely on the file extension within the createSourceFile function itself [1]. Internally, when the TypeScript compiler manages files (such as in a Program or ProjectService), it uses utility functions to map file extensions to the appropriate ScriptKind [3][4]. 3. Common Mapping Pattern: External tools often implement their own mapping logic to ensure that ts.createSourceFile is called with the correct scriptKind [5][3]. A standard pattern is: -.ts -> ts.ScriptKind.TS -.tsx -> ts.ScriptKind.TSX -.js -> ts.ScriptKind.JS -.jsx -> ts.ScriptKind.JSX -.json -> ts.ScriptKind.JSON [3] If you are building tooling, it is best practice to derive the ScriptKind from the file extension using your own helper function before calling ts.createSourceFile [3]. This prevents parsing errors—for example, attempting to parse JSX syntax in a file that the compiler treats as a standard TS file (which lacks JSX support) [6]. The scriptKind property is attached to the resulting SourceFile object, though it is marked as @internal in the TypeScript compiler codebase [7][8]. Tools that need to differentiate between file types (e.g., deciding whether to apply specific transformations) often rely on this property [7].
Citations:
- 1: https://microsoft-typescript.mintlify.app/api/parser
- 2: https://jsr.io/@ts-morph/common/doc/~/ts.createSourceFile
- 3: https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/typescript-estree/src/create-program/getScriptKind.ts
- 4: https://github.com/microsoft/TypeScript/blob/main/src/server/project.ts
- 5: https://github.com/typescript-eslint/typescript-eslint/blob/e7f5e36ecb65002c1ab1e67359cab1527192c471/packages/typescript-estree/src/create-program/createSourceFile.ts
- 6: feat: allow explicit ScriptKind extraFileExtensions microsoft/TypeScript#60178
- 7: API: Expose a way to figure out the
scriptKindmicrosoft/TypeScript#49914 - 8: https://github.com/microsoft/TypeScript/blob/637d5746/src/compiler/utilities.ts
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Search for TypeScript usage and parser probes:\n'
rg -n "createSourceFile|ScriptKind|\\.tsx|tsScriptKind|ScriptKind\\.TSX|ScriptKind\\.TS" . -g '!node_modules' -g '!dist' -g '!build' | head -200
printf '\nNode TS probe using runtime require path via ts transpile if available:\n'
node - <<'JS'
try {
const ts = require('typescript');
console.log('typescript', ts.version);
for (const kind of ['TS', 'TSX', 'JSX']) {
try {
const sf = ts.createSourceFile('example.tsx', '<Foo />\n', ts.ScriptTarget.ES2020, true, ts['ScriptKind'][kind]);
console.log(kind, 'parseDiagnostics=', sf.parseDiagnostics.length);
for (const d of sf.parseDiagnostics) {
console.log(' ', d.code + ': ' + ts.flattenDiagnosticMessageText(d.messageChain && d.messageChain.messageText, '\n'));
}
} catch (e) {
console.log(kind, 'CREATE_ERROR', e && e.message ? e.message : String(e));
}
}
} catch (e) {
console.log('typescript require unavailable', e && e.message ? e.message : String(e));
}
JS
printf '\nSource of node_modules typescript createSourceFile if installed:\n'
node_modules=
if [ -f node_modules/typescript/lib/typescript.js ]; then
node_modules=node_modules/typescript/lib/typescript.js
elif [ -n "${TMPOUT:-}" ]; then
:
fi
if [ -n "$node_modules" ]; then
rg -n "function createSourceFile|ScriptKind" node_modules/typescript/lib/typescript.js | head -50
fiRepository: Harborline-Software/shipyard
Length of output: 39250
🌐 Web query:
site:typescript-eslint.io createSourceFile scriptKind not specified .tsx extension
💡 Result:
When using the TypeScript Compiler API's ts.createSourceFile function with a.tsx extension, you should explicitly specify the ScriptKind to ensure the parser correctly handles JSX syntax. If the ScriptKind is not specified, the compiler may not correctly identify the file as containing JSX, leading to parsing errors. To resolve this, provide ts.ScriptKind.TSX as the third argument to the function: ts.createSourceFile( fileName, sourceText, languageVersion, setParentNodes, ts.ScriptKind.TSX); By explicitly passing ts.ScriptKind.TSX, you inform the compiler that the content contains JSX, regardless of the file extension or the absence of an inferred kind.
Citations:
- 1: https://typescript-eslint.io/packages/project-service
- 2: https://typescript-eslint.io/packages/scope-manager
- 3: https://typescript-eslint.io/packages/type-utils/generated/
- 4: https://typescript-eslint.io/getting-started/
- 5: https://typescript-eslint.io/packages/ast-spec/generated/
issue [blocking]: parse .tsx sources with ts.ScriptKind.TSX.
SOURCE_EXT includes .tsx, but hasStructuralBypass uses ts.ScriptKind.TS for every non-.mjs file. In .tsx files, JSX angle brackets must use ts.ScriptKind.TSX; using TS can make JSX fail parsing or build the wrong AST node shape. A raw shell invoke after JSX in such a file can be missed by the bypass scan. Add the explicit .tsx branch before selecting ts.ScriptKind.TS.
🤖 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 `@apps/hull/src/membrane/no-direct-invoke.arch.test.ts` at line 57, Update
hasStructuralBypass to select ts.ScriptKind.TSX for .tsx sources before the
existing .mjs and TypeScript kind selection; retain ts.ScriptKind.TS for other
non-MJS files so JSX is parsed correctly and shell invokes after JSX remain
detectable.
| const rejectedPolicyOption: HullPortAdapterOptions = { | ||
| shell: trustedShell, | ||
| // @ts-expect-error A caller cannot supply the removed policy function seam. | ||
| policyDecisionPoint: (_command: string) => ({ authority: 'AP', summary: 'caller-controlled allow' }), | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔴 Critical | ⚡ Quick win
issue [blocking]: ESLint fails on the unused _command parameter.
Static analysis reports '_command' is defined but never used under @typescript-eslint/no-unused-vars at line 54. The repository config does not appear to exempt the leading underscore.
Remove the parameter. The @ts-expect-error still fires, because the excess-property check on policyDecisionPoint is what produces the error, not the signature.
🐛 Proposed fix
const rejectedPolicyOption: HullPortAdapterOptions = {
shell: trustedShell,
// `@ts-expect-error` A caller cannot supply the removed policy function seam.
- policyDecisionPoint: (_command: string) => ({ authority: 'AP', summary: 'caller-controlled allow' }),
+ policyDecisionPoint: () => ({ authority: 'AP', summary: 'caller-controlled allow' }),
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const rejectedPolicyOption: HullPortAdapterOptions = { | |
| shell: trustedShell, | |
| // @ts-expect-error A caller cannot supply the removed policy function seam. | |
| policyDecisionPoint: (_command: string) => ({ authority: 'AP', summary: 'caller-controlled allow' }), | |
| } | |
| const rejectedPolicyOption: HullPortAdapterOptions = { | |
| shell: trustedShell, | |
| // `@ts-expect-error` A caller cannot supply the removed policy function seam. | |
| policyDecisionPoint: () => ({ authority: 'AP', summary: 'caller-controlled allow' }), | |
| } |
🧰 Tools
🪛 ESLint
[error] 54-54: '_command' is defined but never used.
(@typescript-eslint/no-unused-vars)
🤖 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 `@apps/hull/src/protocol/hull-port-adapter.type-test.ts` around lines 51 - 55,
Remove the unused _command parameter from the policyDecisionPoint function in
rejectedPolicyOption while retaining the function expression and
`@ts-expect-error` directive so the excess-property type error remains covered.
Source: Linters/SAST tools
| // Scoped to the protocol surface deliberately. Switching the compiler on over every hull test at | ||
| // once surfaces a pre-existing backlog in the runtime suites (cpu-image-floor-runtime, | ||
| // cpu-image-floor-smoke, runtime-connection) that this change did not create and should not | ||
| // absorb. Widen this glob as those are cleared — see the follow-up card. | ||
| "include": ["src/protocol/**/*.ts"], |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
issue (non-blocking): the new typecheck gate does not cover the membrane security suites this PR adds.
The header states that the security-carrying suites were outside the compiler, and that four of them built an AuthorityDecision with a wrong field name. The include glob covers only src/protocol/**/*.ts. The membrane suites — src/membrane/pep.test.ts, src/membrane/public-api.test.ts, src/membrane/credential-mints.test.ts, src/membrane/node-bridge-principal.test.ts — stay outside the gate. The deferral note names only the runtime suites (cpu-image-floor-runtime, cpu-image-floor-smoke, runtime-connection) as the blocked backlog, so the membrane suites appear unblocked.
Widen the glob to the membrane directory, and exclude the three named runtime suites explicitly. The falsification then holds: rename a field on AuthorityDecision in a membrane test, and pnpm typecheck goes RED.
🔧 Proposed scoping change
- "include": ["src/protocol/**/*.ts"],
- "exclude": ["dist"]
+ "include": ["src/protocol/**/*.ts", "src/membrane/**/*.ts"],
+ "exclude": [
+ "dist",
+ "src/membrane/cpu-image-floor-runtime.test.ts",
+ "src/membrane/cpu-image-floor-smoke.test.ts",
+ "src/membrane/runtime-connection.test.ts"
+ ]Run the following script to confirm which membrane suites are still outside the gate and where the deferred runtime suites live:
#!/bin/bash
# List every hull test file and mark whether the new tsconfig.test.json include glob covers it.
fd -e ts -g '*.test.ts' apps/hull/src | sort | while IFS= read -r f; do
case "$f" in
apps/hull/src/protocol/*) echo "COVERED $f" ;;
*) echo "UNCOVERED $f" ;;
esac
done
# Show the base config's include/exclude so the override semantics are explicit.
cat -n apps/hull/tsconfig.jsonAs per path instructions: "the change's CI suite must actually exercise it (A1)" and "Name the mechanism that executes it and state the falsification".
🤖 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 `@apps/hull/tsconfig.test.json` around lines 15 - 19, Update the test typecheck
configuration’s include scope to cover both src/protocol and src/membrane, while
explicitly excluding the deferred cpu-image-floor-runtime,
cpu-image-floor-smoke, and runtime-connection suites. Keep the existing runtime
backlog deferral note accurate, and ensure the pnpm typecheck gate now fails
when a membrane test uses an invalid AuthorityDecision field.
Source: Path instructions
| function secureInvoke( | ||
| target: Parameters<typeof secureInvokeActual>[0], | ||
| principal: typeof ALICE, | ||
| _legacyPolicy: unknown, | ||
| execute: Parameters<typeof secureInvokeActual>[2], | ||
| opts: { consumeToken?: TokenConsumer } = {}, | ||
| ) { | ||
| const trusted = new TrustedConfirmationBroker() | ||
| const confirmation = target.confirmation | ||
| const valid = confirmation != null | ||
| && opts.consumeToken?.(confirmation.token, confirmation.command) === true | ||
| const trustedConfirmation = valid && confirmation != null | ||
| ? trusted.confirm( | ||
| trusted.propose(confirmation.command), | ||
| confirmation.command, | ||
| confirmation.confirmedBy, | ||
| ) | ||
| : confirmation | ||
| const actualTarget = valid && confirmation != null | ||
| ? { ...target, confirmation: trustedConfirmation } | ||
| : target | ||
| return secureInvokeActual( | ||
| actualTarget, | ||
| principal, | ||
| execute, | ||
| {}, | ||
| valid ? trusted : undefined, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
issue: one copy-pasted test adapter both weakens the CP gate assertions and forces a reach into Hull's host-only issuer.
The shared root cause is a local secureInvoke shim, duplicated verbatim in both suites. The shim validates the caller's confirmation against the legacy ProposalBroker, then discards it and mints fresh registered evidence from a real TrustedConfirmationBroker. Two consequences follow from that single decision. The PEP's registered-evidence check can never fail in either suite, so a regression that accepted unregistered evidence would still pass. And minting requires TrustedConfirmationBroker, which is host-only and absent from Hull's export map, so both files import it by deep relative path and step around the very boundary this PR establishes.
Fix the shim once, in a shared helper, and both symptoms close. Drive each test from a real TrustedConfirmationBroker at the start: propose, confirm, then pass the resulting evidence as the target confirmation. Negative cases then pass a genuinely unregistered object and stay negative. With no minting in the tests, the host-only import is no longer needed.
packages/carrier-sdk/src/cp-rejection.test.ts#L72-L100: replace the minting shim so a forged confirmation reaches the PEP unchanged; keep the existing no-confirmation cases, which already passundefinedas the broker and correctly reject.packages/carrier-sdk/src/cp-notification-loop.test.ts#L89-L117: delete this duplicate shim and import the shared helper; also stop passingconfirmation.confirmedByintotrusted.confirm, becauseTrustedConfirmationBroker.confirmdoes not verify that the value is a registered membrane principal.packages/carrier-sdk/src/cp-rejection.test.ts#L30-L32: drop theapps/hull/src/membrane/credential-issuer.jsimport once the shim no longer mints evidence; route the remainingpep.jsandhost-principal.jsneeds through Hull's package export map.packages/carrier-sdk/src/cp-notification-loop.test.ts#L44-L46: apply the same import change.
As per path instructions: "avoid hand-parallel duplicate copies of single-source things (A4)", "the change's CI suite must actually exercise it (A1)", and "Flag CP-touching changes (financial/audit/security/compliance/concurrency) as needing an independent reviewer, not self-assertion (A2)".
📍 Affects 2 files
packages/carrier-sdk/src/cp-rejection.test.ts#L72-L100(this comment)packages/carrier-sdk/src/cp-notification-loop.test.ts#L89-L117packages/carrier-sdk/src/cp-rejection.test.ts#L30-L32packages/carrier-sdk/src/cp-notification-loop.test.ts#L44-L46
🤖 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 `@packages/carrier-sdk/src/cp-rejection.test.ts` around lines 72 - 100, Replace
the duplicated secureInvoke shim with one shared helper that uses real
TrustedConfirmationBroker evidence and passes forged confirmations unchanged,
preserving no-confirmation rejection cases. In
packages/carrier-sdk/src/cp-rejection.test.ts#L72-L100, use the shared helper;
in packages/carrier-sdk/src/cp-notification-loop.test.ts#L89-L117, remove the
duplicate and stop passing confirmedBy to trusted.confirm. Remove the deep
issuer imports in packages/carrier-sdk/src/cp-rejection.test.ts#L30-L32 and
packages/carrier-sdk/src/cp-notification-loop.test.ts#L44-L46, routing remaining
imports through Hull’s package exports. Ensure CI exercises both suites and flag
the CP-touching change for independent review.
Source: Path instructions
| it('ignores a caller-supplied principal option and records the host identity', async () => { | ||
| const sink = new InMemoryDecisionSink() | ||
| // A blank-id principal is anonymous — authenticate rejects it before execute. | ||
| const anon = { id: ' ', displayName: '', kind: 'local-os-user' } as Principal | ||
| const result = await invokeInProcess('image', imageCore, { principal: anon, decisionSink: sink }) | ||
| expect(result.status).toBe('failed') | ||
| expect(result.error?.code).toBe('membrane.anonymous_principal') | ||
| // The authorize step never ran (authenticate short-circuited) — no decision recorded. | ||
| expect(sink.decisions).toHaveLength(0) | ||
| const result = await invokeInProcess('image', imageCore, { | ||
| principal: { id: 'os:root', displayName: 'root', kind: 'local-os-user' }, | ||
| decisionSink: sink, | ||
| } as never) | ||
| expect(result.status).toBe('succeeded') | ||
| expect(sink.decisions[0]?.principal.id).toBe(currentHostId()) | ||
| }) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
issue: this test replaces the anonymous-principal fail-closed coverage with a weaker property.
The new test asserts that a caller-supplied principal option is ignored and that the host identity is recorded. That is a useful property, and as never is the right way to force the removed option through at runtime.
It is not the property the removed test covered. The previous test asserted that an anonymous principal is refused and that no decision is recorded. This suite now has no assertion that fails if the PEP starts accepting an unregistered principal. apps/hull/src/membrane/public-api.test.ts Lines 60-64 covers a forged principal at the Hull layer, so the guarantee is not unverified overall — but the SDK-level rejection path lost its guard, and the SDK is the surface external callers use.
Keep the new test and restore a fail-closed case alongside it.
💚 Proposed additional coverage
it('ignores a caller-supplied principal option and records the host identity', async () => {
const sink = new InMemoryDecisionSink()
const result = await invokeInProcess('image', imageCore, {
principal: { id: 'os:root', displayName: 'root', kind: 'local-os-user' },
decisionSink: sink,
} as never)
expect(result.status).toBe('succeeded')
expect(sink.decisions[0]?.principal.id).toBe(currentHostId())
})
+
+ it('records an inert attribution, never a registered credential', async () => {
+ const sink = new InMemoryDecisionSink()
+ await invokeInProcess('image', imageCore, { decisionSink: sink })
+ const recorded = sink.decisions[0]?.principal
+ expect(recorded).toBeDefined()
+ // The recorded value must not be usable as a credential: it is a fresh copy,
+ // not the object the PEP authenticates by identity.
+ expect(Object.isFrozen(recorded)).toBe(true)
+ expect(Object.getPrototypeOf(recorded)).toBeNull()
+ })As per path instructions: "the change's CI suite must actually exercise it (A1)".
🤖 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 `@packages/carrier-sdk/src/sdk.test.ts` around lines 208 - 216, Keep the
existing caller-supplied principal test, and restore a separate SDK-level test
covering an anonymous or unregistered principal being rejected fail-closed.
Exercise the rejection through invokeInProcess and assert the operation does not
succeed and that the decision sink records no decision, preserving the prior SDK
behavior and ensuring the CI suite runs this guard.
Source: Path instructions
| // The host stamps WHO. Caller options cannot replace the stored identity. | ||
| return invokeInProcess(capability, core, { ...opts }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
issue [non-blocking]: the comment claims a guarantee this line does not provide.
{ ...opts } is a shallow copy. It forwards every key the caller supplied, including a principal key if one is present at runtime. It removes nothing. The actual protection is in invokeInProcess at packages/carrier-sdk/src/runtime-host.ts lines 177-200, which reads only decisionSink and inlineAudio and builds the identity through createSecuredHullInvoke.
Two fixes:
- Drop the spread or say what it does. As written, a later reader may treat
{ ...opts }as a sanitizing step and rely on it. - The docblock at lines 139-147 still states that "the client's
actingAsprincipal is carried intoinvokeInProcess". That is no longer true. Update it in the same change, or the next reader trusts a path that no longer exists.
♻️ Proposed fix
- // The host stamps WHO. Caller options cannot replace the stored identity.
- return invokeInProcess(capability, core, { ...opts })
+ // `invokeInProcess` resolves the host identity itself and reads only `decisionSink`
+ // and `inlineAudio` from `opts`. This client no longer passes an identity.
+ return invokeInProcess(capability, core, opts)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // The host stamps WHO. Caller options cannot replace the stored identity. | |
| return invokeInProcess(capability, core, { ...opts }) | |
| // `invokeInProcess` resolves the host identity itself and reads only `decisionSink` | |
| // and `inlineAudio` from `opts`. This client no longer passes an identity. | |
| return invokeInProcess(capability, core, opts) |
🤖 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 `@packages/carrier-sdk/src/sdk.ts` around lines 155 - 156, Correct the
misleading identity-protection documentation around the SDK invocation: update
the docblock near the caller options to state the actual behavior, and remove
the `{ ...opts }` spread in the `invokeInProcess` call or describe it only as a
shallow copy, since it does not sanitize caller keys. Preserve the identity
enforcement implemented by `invokeInProcess` and `createSecuredHullInvoke`.
1 participant
What this is
The credential half of pull request 3517, stacked on #3529. Based on that branch, not on
main— GitHub will retarget this tomainonce 3529 merges.3529 carries the language-agnostic protocol contract, which three independent review rounds confirmed sound. This carries the Hull credential boundary, which got pulled in when the first of those rounds touched it and then absorbed four remediation rounds of its own. Separating them lets the finished half land and gives this half a review scoped to one question.
Why this is structural rather than another call-site fix
Each of the three review rounds found that the previous round's remediation had closed the one constructor the review named and left the identical pattern in a sibling. A policy function was branded; the brand was decoration because the mint stamped anything, so the parameter was removed; then the principal mint and the confirmation broker turned out to have the same defect. The cause was never the individual call site — it was the export surface.
What lands
Authorization splits into a policy store and a policy evaluator. The store is the source of truth: immutable, null-prototype, frozen, and fail-closed to the most restrictive class when its source cannot be read. The evaluator is built from the store and answers allow/deny for the current request. The security layer reads the source of truth directly, so no mutable permission object crosses a trust boundary and
secureInvoketakes no policy parameter at all.Credential construction leaves the public surface. The hull exports only enforcement operations and credential input types. Issuers are private, reachable solely from host composition roots, so no exported path hands a caller a subject or confirmation constructor.
The guarantee is asserted structurally.
apps/hull/src/membrane/public-api.test.tsproves a shape-only principal is still rejected with the per-call-site validation removed. That is the property three rounds of call-site fixes failed to hold, so it is the property the test pins.The node bridge is fail-closed on both halves of its property at once. Caller-supplied identity in the payload is ignored, and an unattributed request is refused rather than proceeding under host authority. Removing the payload identity was right on its own, but it had been paired with an unconditional host stamp — so an anonymous request started succeeding with the host's rights. Both must hold together.
This also restores the build gates ADR 0162 marked deferred while only the contract half had landed: the architecture test forbidding direct transport imports, and the PEP and host-stamped-principal negative tests.
Verification
By mutation rather than by report, and mutated toward a different wrong answer — a red suite proves a check reads its input, not that it checks the property.
node scripts/ts-suites.mjs(the full CI gate): exit 0@shipyard/hull297 passed, 2 skipped — 30 more than the contract half, which is this boundary's suite@shipyard/contracts275 / 275 ·@shipyard/carrier-sdk86 / 86 ·@shipyard/carrier2385 passed, 2 skipped · Rust pass · lint clean across all four packagesThe contract half and this half together reproduce the original branch head exactly, apart from two type errors fixed in 3529 that a stale build artifact had been hiding.
Not armed
This wants the narrowly-scoped independent security review before it merges — one question, one boundary.
Summary by CodeRabbit