feat(contracts): make the carrier and hull boundaries language-agnostic - #3517
feat(contracts): make the carrier and hull boundaries language-agnostic#3517ctwoodwa wants to merge 24 commits into
Conversation
Carrier's semantic contracts stopped at the component layer. The application, host and Hull boundaries were still expressed in implementation languages, with TypeScript canonical and C# and Rust kept as hand-maintained mirrors, so a future implementation in any other language would depend on TypeScript details rather than on stable product behaviour. The source of truth becomes a versioned protocol rather than a language interface. A manifest plus JSON Schema 2020-12 describes the data messages, an OpenAPI 3.1 seed describes the local-node surface, and a bounded checked-in generator emits TypeScript, C# and Rust bindings from it. The generator fails on any schema keyword it does not support rather than guessing, and a drift check proves the committed output still matches its source. The bindings are projections and never alternate sources of truth. Existing runtimes stay where they are and move behind adapters: the Hull runtime becomes one implementation behind a port, Tauri becomes an adapter to a host contract rather than the application-facing interface, and renderer clients reach the local node through a transport port. Every existing public entry point is retained as a facade, so no consumer has to move in this change. Security posture is unchanged by construction. Trusted request fields are schema-validated as host context and cannot appear in serialized client request models, the membrane remains the policy-enforcement point, and protocol invoke accepts only an injected secured entrypoint. Adds a decision record for the doctrine and amends the two that previously bound TypeScript canonicality, so a later reader does not infer it from stale prose. Ignores the new crate's build directory, which was otherwise untracked and would have been committed.
What this isImplements the approved plan The source of truth for the Carrier and Hull boundaries becomes a versioned protocol rather than a language interface. A manifest plus JSON Schema 2020-12 describes the data messages, an OpenAPI 3.1 seed describes the local-node surface, and a bounded checked-in generator emits TypeScript, C# and Rust bindings. The generator fails on any schema keyword it does not support rather than guessing, and a drift check proves the committed output still matches its source. Bindings are projections, never alternate sources of truth. Runtimes move behind adapters and every existing public entry point is retained as a facade, so no consumer has to move in this change. Verification — re-run here, not taken from the planThe plan records all six phases PASS. Those were re-run rather than trusted:
Two corrections to the plan's recorded evidenceHull is not cleanly green. Both failures are in Tauri Two things caught while folding in
Review postureNot armed. The change is large and touches the Hull policy-enforcement point and principal handling in substance even where paths do not match a |
github-actions
Bot
added
the
needs-deep-review
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR establishes schema-first Carrier and Hull contracts through JSON Schema and a versioned manifest. It generates TypeScript, C#, and Rust projections, adds validated Carrier and Hull adapters, centralizes host-owned security, migrates Carrier clients, and adds cross-language fixture tests. ChangesCarrier and Hull protocol migration
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 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
|
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 |
![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
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (5)
apps/carrier/src/renderer-log.ts-64-64 (1)
64-64: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winissue: Handle rejected renderer-log forwarding.
appendRendererLogreturns a promise. The surroundingtry/catchdoes not catch a rejected promise. Attach a rejection handler so logging failures do not create unhandled rejections or re-enter the overridden console path.Proposed fix
- void carrierHostPort().appendRendererLog(entry) + void carrierHostPort().appendRendererLog(entry).catch(() => undefined)🤖 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/renderer-log.ts` at line 64, Update the renderer-log forwarding call in the surrounding console override to handle the promise returned by appendRendererLog, attaching a rejection handler that safely suppresses or reports forwarding failures without routing back through the overridden console path or creating unhandled rejections.tooling/carrier-contract-codegen/tests/generate.test.mjs-41-46 (1)
41-46: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winissue: Normalize recursive file paths before applying exclusions.
readdirSync(..., { recursive: true })returns platform-native separators. On Windows,protocol\\tauri-carrier-adapter.tsdoes not equal the slash-delimited exclusion, so the test reports its permitted adapter import as an offender.Proposed fix
- const offenders = readdirSync(sourceRoot, { recursive: true }) + const offenders = readdirSync(sourceRoot, { recursive: true }) + .map((file) => file.replaceAll('\\', '/'))As per path instructions, “verification must be clean-checkout reproducible (A7).”
🤖 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 `@tooling/carrier-contract-codegen/tests/generate.test.mjs` around lines 41 - 46, Normalize each path returned by readdirSync in the offenders pipeline before applying the test-file, __tests__, and protocol/tauri-carrier-adapter.ts exclusions, converting platform-native separators to '/'. Keep resolve(sourceRoot, file) working with the normalized relative path and ensure the permitted adapter is excluded consistently across operating systems.Source: Path instructions
icm/03_package-design/output/package-design-carrier-language-agnostic-adapters-2026-08-01.md-10-14 (1)
10-14: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winissue [non-blocking]: Document the checked-in TypeScript projection path.
The reviewed projection is
packages/contracts/src/protocol.ts. This document namessrc/generated/carrier-protocol.generated.ts. Update the package map, or add the documented file, so implementation guidance does not point to a missing projection.🤖 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 `@icm/03_package-design/output/package-design-carrier-language-agnostic-adapters-2026-08-01.md` around lines 10 - 14, Update the package map in the carrier language-agnostic adapters design to reference the checked-in TypeScript projection at packages/contracts/src/protocol.ts instead of the missing src/generated/carrier-protocol.generated.ts, while preserving the existing C# and Rust generated-file entries.packages/contracts/src/index.ts-16-18 (1)
16-18: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick wintypo (non-blocking): sentence fragment in the capability bullet.
The bullet ends with "and Negotiate/Observe." and the next line starts mid-sentence with a lowercase word.
Proposed wording
- * S4 two-layer license gate (SEC-6 no-credential), and Negotiate/Observe. - * retained TypeScript compatibility surface. ADR 0162 moves the - * inter-language Carrier/Hull boundary to generated protocol bindings. + * S4 two-layer license gate (SEC-6 no-credential), and Negotiate/Observe. + * This is a retained TypeScript compatibility surface. ADR 0162 moves the + * inter-language Carrier/Hull boundary to generated protocol bindings.🤖 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/contracts/src/index.ts` around lines 16 - 18, Correct the capability bullet in the module documentation around the S4 two-layer license gate so “and Negotiate/Observe” and the following “retained TypeScript compatibility surface” text form a complete, grammatically correct sentence. Preserve the stated capabilities and ADR 0162 reference while fixing the sentence break and capitalization.packages/contracts/src/capability.ts-4-10 (1)
4-10: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick wintypo (non-blocking): the surrounding doctrine text now contradicts the new ADR 0162 sentence.
Lines 6-8 state that generated bindings own the inter-language boundary. Lines 4-5 and 9-10 still state that this package is the single source and that ".NET local-node and Python workers mirror these shapes". The mirror-and-authority wording describes the model this PR replaces, so a reader who stops at line 10 keeps hand-maintaining the C# mirror.
Proposed wording
- * The X-1 v0 build-gate (ADR 0123 X-1 / ADR 0124 X-1): `@shipyard/contracts` is - * the SINGLE SOURCE for the Hull membrane's envelope/manifest/resolution types. * These inference/capability types are the retained TypeScript compatibility surface. ADR 0162 * supersedes the former TS-canonical ruling for inter-language boundaries; those consumers use - * the generated `@shipyard/contracts/protocol` port and DTO projection. The - * .NET local-node and Python workers mirror these shapes; this package is the - * authority. + * the generated `@shipyard/contracts/protocol` port and DTO projection, whose single source is + * `packages/contracts/protocol`. Do not hand-maintain .NET or Python mirrors of these shapes.🤖 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/contracts/src/capability.ts` around lines 4 - 10, Update the doctrine comment surrounding the X-1 build-gate in capability.ts to consistently reflect ADR 0162: remove or revise the single-source, authority, and “.NET local-node and Python workers mirror these shapes” wording, and state that inter-language consumers use the generated `@shipyard/contracts/protocol` port and DTO projections. Keep the retained TypeScript compatibility-surface description accurate.Source: Path instructions
🧹 Nitpick comments (5)
apps/hull/src/protocol/hull-port-adapter.ts (1)
139-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: reuse
HullShell's connection lookup instead of re-implementing it here.
HullShellalready has arequireConnectionbacked by aMaplookup (seeapps/hull/src/shell/hull-shell.ts). This adapter reimplements the same "find or throw" logic with a linear scan overshell.runtimes, and with a different error message. Expose a public accessor (for exampleHullShell.requireConnectionorHullShell.getConnection) and have this adapter delegate to it, so the lookup and its error text stay single-sourced.As per path instructions, "avoid hand-parallel duplicate copies of single-source things (A4)."
♻️ Proposed refactor
- private requireConnection(runtimeId: string): RuntimeConnection { - const connection = this.options.shell.runtimes.find((item) => item.runtimeId === runtimeId) - if (!connection) throw new Error(`HullPort: runtime '${runtimeId}' is not connected`) - return connection - } + private requireConnection(runtimeId: string): RuntimeConnection { + return this.options.shell.requireConnection(runtimeId) + }(Requires exposing
requireConnection/getConnectionas a public method onHullShell.)🤖 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 139 - 143, Expose HullShell’s existing Map-backed connection lookup as a public requireConnection or getConnection method, then update HullPortAdapter.requireConnection to delegate to that method instead of scanning shell.runtimes. Preserve HullShell’s centralized lookup and error message as the single source of behavior.tooling/carrier-contract-codegen/generate.mjs (1)
97-99: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winsuggestion [tooling]: define the host-command count from a single documented source.
10is hard-coded even though ADR 0162 and the generator test already specify the ten host commands. Keeping separate inventory and count invariants creates drift when the manifest changes. Use a named constant with the ADR/doc reference, or a single manifest/ADR source that both the generator and parity test can derive.🤖 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 `@tooling/carrier-contract-codegen/generate.mjs` around lines 97 - 99, Replace the hard-coded host-command count in the validation around hostCommands with a named constant or shared manifest-derived value documenting ADR 0162, and use that same source for the generator’s inventory check and the parity test. Preserve the exact-count validation while ensuring future command-list changes cannot leave the count and command inventory out of sync.Source: Coding guidelines
packages/contracts/Shipyard.Contracts.csproj (1)
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuethought (non-blocking):
NoWarn=CS1591hides the XML-doc requirement for public members.The review policy requires XML documentation on new or changed public members. Generated code is a reasonable exception. Please make the exception explicit, for example by scoping the suppression to the generated file with a comment that states why.
🤖 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/contracts/Shipyard.Contracts.csproj` at line 8, The CS1591 suppression in Shipyard.Contracts.csproj is too broad and does not document its generated-code exception. Scope the suppression to the generated contract output, or add an adjacent comment explicitly stating that CS1591 is suppressed only because the affected members are generated, while preserving XML documentation enforcement for other public members.Source: Path instructions
packages/contracts/rust/Cargo.toml (1)
1-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion (non-blocking): pin the toolchain floor and mark the crate unpublished.
rust-versionturns an edition mismatch into a clear cargo error.publish = falseprevents an accidental crates.io release of a generated projection.Proposed manifest additions
[package] name = "shipyard-carrier-contracts" version = "0.1.0" edition = "2024" +rust-version = "1.85" +publish = false license = "MIT"🤖 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/contracts/rust/Cargo.toml` around lines 1 - 6, Add rust-version to the [package] manifest for shipyard-carrier-contracts to pin the minimum supported Rust toolchain, and set publish = false to prevent publishing the generated projection. Preserve the existing package metadata and use the repository’s intended toolchain floor.packages/contracts/rust/src/generated.rs (1)
441-473: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffthought [non-blocking]: make generated port futures
Sendif they need task-boundary execution.Native
async fnin these traits does not requireSend, and async trait methods are not dyn-compatible. IfCarrierHostPort/related ports are meant for impls that might pass futures across task boundaries, mirror the generated surface withtrait_variant::makeor explicitSendreturn bounds to avoid a later breaking surface change.🤖 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/contracts/rust/src/generated.rs` around lines 441 - 473, Update the generated port traits HullPort, CarrierHostPort, and CarrierApplicationPort so their async method futures are Send-capable when used across task boundaries, using the project’s established trait_variant::make pattern or equivalent explicit Send bounds. Preserve the existing method signatures and result types while applying the same generated convention consistently to all related ports.
🤖 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/membrane/data-location-client.ts`:
- Line 54: Replace the local DataLocationStatus projection in
apps/carrier/src/membrane/data-location-client.ts at lines 54-54 with the
generated DataLocationStatus import and remove the duplicate local type.
Likewise, replace the local RendererLogEntry projection in
apps/carrier/src/renderer-log.ts at lines 42-42 with its generated binding
import and remove the duplicate, keeping generated protocol types as the sole
source of these contracts.
In `@apps/carrier/src/membrane/nodeClient.ts`:
- Around line 131-134: Replace the hand-maintained NodeStatus interface in
apps/carrier/src/membrane/nodeClient.ts:131-134 with the generated host-port
result type; import and use generated PeerSyncConfig in
apps/carrier/src/membrane/peer-sync-client.ts:26-32 instead of local wire
fields; and in apps/carrier/src/sync-status/syncStatusClient.ts:37-39 remove the
SyncStatus assertion by returning the generated type directly or explicitly
mapping fields.
In `@apps/carrier/src/protocol/http-carrier-application-adapter.ts`:
- Line 13: Fix the unused _request parameter in getSyncStatus by omitting it if
the method remains assignable to the interface, or explicitly marking it as used
if the parameter is required. Preserve the existing CarrierSyncStatus behavior
and interface compatibility.
In `@apps/carrier/src/protocol/tauri-carrier-adapter.ts`:
- Line 37: Update the methods at hullHealth and the other affected methods to
remove their unused _request: EmptyRequest parameters, then remove the
now-unused EmptyRequest import. Preserve each method’s existing return type and
behavior.
In `@apps/hull/src/protocol/hull-port-adapter.test.ts`:
- Around line 33-73: Expand the HullShellPortAdapter test suite to cover the
remaining HullPort methods announce, negotiate, address, observe, and resolve,
asserting their mapped inputs and outputs, including connection.manifest fields.
Strengthen the existing invoke test to compare the complete result with success
via strict equality rather than checking only status, preserving coverage for
the adapter’s cast boundary.
In `@apps/hull/src/protocol/hull-port-adapter.ts`:
- Around line 87-99: Reconcile the Hull and membrane invoke envelopes in
HullPortAdapter.invoke: either extend the generated v1 request to carry provider
inputs, attachments, and transport, or explicitly constrain and document this
path as sync-only with no attachments. Replace the unknown double-casts for
CapabilityCore and ProtocolCapabilityResult with validated field mappings or an
explicit compatibility contract between the generated and imported types.
In `@docs/adrs/0162-language-agnostic-carrier-hull-contracts.md`:
- Around line 35-37: Update ADR 0162’s Status section to include the independent
security-engineering approval or verdict for the Secure/PEP and host-owned
trusted-context changes before retaining the “Accepted” status; do not rely
solely on the requires-council metadata.
In `@icm/06_build/output/carrier-language-agnostic-adapters-2026-08-01.md`:
- Around line 28-29: Update the Rust verification documentation around the Tauri
cargo check result to use a clean-checkout reproducible test input: either
commit a deterministic placeholder fixture or include its creation and cleanup
in the declared verification command, and ensure the command no longer depends
on an undocumented temporary file.
In
`@icm/07_review/output/carrier-language-agnostic-adapters-review-2026-08-01.md`:
- Line 3: Update the review record’s verdict and supporting evidence to identify
an independent security reviewer and document their result for the secured Hull
invocation and trusted context boundary changes. Do not mark the review as PASS
until this independent review evidence is recorded.
In `@package.json`:
- Around line 54-56: Add a required CI job for the carrier contract generator
that runs from a clean checkout and invokes both the package scripts
carrier-contracts:codegen:check and carrier-contracts:codegen:test. Integrate it
with the existing CI workflow so generated-output drift or generator test
failures block merges.
In `@packages/contracts/protocol/fixtures/manifest.json`:
- Around line 1-14: Make packages/contracts/protocol/fixtures/manifest.json the
single fixture inventory and have the generator project it into the C# Cases in
packages/contracts/tests/ProtocolFixtureTests.cs:12-24, the Rust assertions in
packages/contracts/rust/src/lib.rs:19-31, and the TypeScript it.each loop in
packages/contracts/src/__tests__/protocol-fixtures.test.ts:44-55; remove the
hand-written parallel lists while preserving compile-time fixture paths for
Rust. Add a guard that fails when any fixture file lacks a manifest entry, and
verify it goes RED by adding an unlisted fixture.
In `@packages/contracts/protocol/manifest.json`:
- Around line 28-29: Update the carrier.host.nodeStatus contract and its
associated NodeStatus response handling so sessionToken is never serialized
across the CarrierHostPort renderer boundary. Keep the token host-owned and
inject it only through the trusted HTTP adapter or a narrow host proxy, while
preserving fail-closed behavior for requests that lack host-side token access.
In `@packages/contracts/protocol/schemas/carrier-protocol.schema.json`:
- Around line 222-230: Remove sessionToken from the NodeStatus schema and its
required fields, then update the native application adapter to inject the token
internally when invoking CarrierApplicationPort rather than serializing it in
the host response. Regenerate bindings and update all affected fixtures to match
the revised NodeStatus shape.
- Line 59: Update the CapabilityResult error schema to represent nullability
with anyOf, combining a ProtocolError $ref branch with a null type branch; do
not place type and $ref as sibling constraints, so both valid ProtocolError
objects and null Option/nullable payloads are accepted.
In `@packages/contracts/rust/src/generated.rs`:
- Around line 119-121: Update the Rust generation logic in generate.mjs so
nullable fields that are optional in the C# and TypeScript projections are
emitted with Serde defaults, allowing missing JSON keys to deserialize as None
rather than requiring presence. Apply this consistently to error and the listed
nullable fields, including all applicable EnrollmentStatus fields, while
preserving Option<T> for explicitly provided null values.
In `@tooling/carrier-contract-codegen/generate.mjs`:
- Around line 16-19: Update tsDefinition, csDefinition, rustDefinition, and the
port interface emitters to read definition.description and property.description
and emit documentation for every generated public type and member. Use JSDoc for
TypeScript, XML documentation for C#, and /// comments for Rust, preserving
existing output when descriptions are absent.
In `@tooling/carrier-contract-codegen/tests/generate.test.mjs`:
- Around line 66-73: Replace the source-text assertions in the Hull protocol
adapter test with a controlled dependency test that injects a secured invocation
entrypoint and a failing raw shell invocation, then verifies the adapter uses
only the injected secured entrypoint. Update the test around the adapter’s
construction and invocation symbols to cover aliases or alternate raw-call
spellings, and obtain independent security review for this boundary change.
- Around line 39-48: The production renderer check currently flags the direct
isTauri import in peer-sync-client.ts even though the assertion only intends to
prohibit direct core or HTTP Tauri APIs. Update the test’s offender pattern to
target only the prohibited `@tauri-apps/api/core` and `@tauri-apps/plugin-http`
imports, while preserving the existing source-root, test-file, and adapter
exclusions.
---
Minor comments:
In `@apps/carrier/src/renderer-log.ts`:
- Line 64: Update the renderer-log forwarding call in the surrounding console
override to handle the promise returned by appendRendererLog, attaching a
rejection handler that safely suppresses or reports forwarding failures without
routing back through the overridden console path or creating unhandled
rejections.
In
`@icm/03_package-design/output/package-design-carrier-language-agnostic-adapters-2026-08-01.md`:
- Around line 10-14: Update the package map in the carrier language-agnostic
adapters design to reference the checked-in TypeScript projection at
packages/contracts/src/protocol.ts instead of the missing
src/generated/carrier-protocol.generated.ts, while preserving the existing C#
and Rust generated-file entries.
In `@packages/contracts/src/capability.ts`:
- Around line 4-10: Update the doctrine comment surrounding the X-1 build-gate
in capability.ts to consistently reflect ADR 0162: remove or revise the
single-source, authority, and “.NET local-node and Python workers mirror these
shapes” wording, and state that inter-language consumers use the generated
`@shipyard/contracts/protocol` port and DTO projections. Keep the retained
TypeScript compatibility-surface description accurate.
In `@packages/contracts/src/index.ts`:
- Around line 16-18: Correct the capability bullet in the module documentation
around the S4 two-layer license gate so “and Negotiate/Observe” and the
following “retained TypeScript compatibility surface” text form a complete,
grammatically correct sentence. Preserve the stated capabilities and ADR 0162
reference while fixing the sentence break and capitalization.
In `@tooling/carrier-contract-codegen/tests/generate.test.mjs`:
- Around line 41-46: Normalize each path returned by readdirSync in the
offenders pipeline before applying the test-file, __tests__, and
protocol/tauri-carrier-adapter.ts exclusions, converting platform-native
separators to '/'. Keep resolve(sourceRoot, file) working with the normalized
relative path and ensure the permitted adapter is excluded consistently across
operating systems.
---
Nitpick comments:
In `@apps/hull/src/protocol/hull-port-adapter.ts`:
- Around line 139-143: Expose HullShell’s existing Map-backed connection lookup
as a public requireConnection or getConnection method, then update
HullPortAdapter.requireConnection to delegate to that method instead of scanning
shell.runtimes. Preserve HullShell’s centralized lookup and error message as the
single source of behavior.
In `@packages/contracts/rust/Cargo.toml`:
- Around line 1-6: Add rust-version to the [package] manifest for
shipyard-carrier-contracts to pin the minimum supported Rust toolchain, and set
publish = false to prevent publishing the generated projection. Preserve the
existing package metadata and use the repository’s intended toolchain floor.
In `@packages/contracts/rust/src/generated.rs`:
- Around line 441-473: Update the generated port traits HullPort,
CarrierHostPort, and CarrierApplicationPort so their async method futures are
Send-capable when used across task boundaries, using the project’s established
trait_variant::make pattern or equivalent explicit Send bounds. Preserve the
existing method signatures and result types while applying the same generated
convention consistently to all related ports.
In `@packages/contracts/Shipyard.Contracts.csproj`:
- Line 8: The CS1591 suppression in Shipyard.Contracts.csproj is too broad and
does not document its generated-code exception. Scope the suppression to the
generated contract output, or add an adjacent comment explicitly stating that
CS1591 is suppressed only because the affected members are generated, while
preserving XML documentation enforcement for other public members.
In `@tooling/carrier-contract-codegen/generate.mjs`:
- Around line 97-99: Replace the hard-coded host-command count in the validation
around hostCommands with a named constant or shared manifest-derived value
documenting ADR 0162, and use that same source for the generator’s inventory
check and the parity test. Preserve the exact-count validation while ensuring
future command-list changes cannot leave the count and command inventory out of
sync.
🪄 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: b4bbb68c-9d33-4b43-aef5-9b55d5289532
⛔ Files ignored due to path filters (4)
apps/carrier/src-tauri/Cargo.lockis excluded by!**/*.lock,!**/*.lockpackages/contracts/Generated/CarrierProtocol.g.csis excluded by!**/generated/**packages/contracts/rust/Cargo.lockis excluded by!**/*.lock,!**/*.lockpackages/contracts/src/generated/carrier-protocol.generated.tsis excluded by!**/*.generated.*,!**/generated/**
📒 Files selected for processing (62)
Shipyard.slnxapps/carrier/src-tauri/Cargo.tomlapps/carrier/src-tauri/src/lib.rsapps/carrier/src/copilot/resolvePilotProvider.tsapps/carrier/src/membrane/client.tsapps/carrier/src/membrane/data-location-client.tsapps/carrier/src/membrane/device-capability-client.tsapps/carrier/src/membrane/nodeClient.tsapps/carrier/src/membrane/peer-sync-client.tsapps/carrier/src/membrane/principal.tsapps/carrier/src/protocol/http-carrier-application-adapter.tsapps/carrier/src/protocol/tauri-carrier-adapter.tsapps/carrier/src/renderer-log.tsapps/carrier/src/sync-status/syncStatusClient.tsapps/hull/src/index.tsapps/hull/src/membrane/runtime-connection.tsapps/hull/src/protocol/hull-port-adapter.test.tsapps/hull/src/protocol/hull-port-adapter.tsapps/local-node-host/Health/SyncStatusRoutes.csapps/local-node-host/Shipyard.LocalNodeHost.csprojdocs/adrs/0123-flight-deck-capability-slot-providers.mddocs/adrs/0124-hull-membrane-multi-tier-execution-contract.mddocs/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.mdpackage.jsonpackages/contracts/README.mdpackages/contracts/Shipyard.Contracts.csprojpackages/contracts/package.jsonpackages/contracts/protocol/README.mdpackages/contracts/protocol/fixtures/capability-result.jsonpackages/contracts/protocol/fixtures/carrier-sync-status.jsonpackages/contracts/protocol/fixtures/data-location-status.jsonpackages/contracts/protocol/fixtures/device-capability-profile.jsonpackages/contracts/protocol/fixtures/health-report.jsonpackages/contracts/protocol/fixtures/hull-invoke-request.jsonpackages/contracts/protocol/fixtures/manifest.jsonpackages/contracts/protocol/fixtures/node-status.jsonpackages/contracts/protocol/fixtures/peer-sync-config.jsonpackages/contracts/protocol/fixtures/principal.jsonpackages/contracts/protocol/fixtures/renderer-log-entry.jsonpackages/contracts/protocol/manifest.jsonpackages/contracts/protocol/openapi/carrier-application.openapi.jsonpackages/contracts/protocol/schemas/carrier-protocol.schema.jsonpackages/contracts/rust/.gitignorepackages/contracts/rust/Cargo.tomlpackages/contracts/rust/src/generated.rspackages/contracts/rust/src/lib.rspackages/contracts/src/__tests__/protocol-fixtures.test.tspackages/contracts/src/capability.tspackages/contracts/src/index.tspackages/contracts/src/protocol.tspackages/contracts/tests/ProtocolFixtureTests.cspackages/contracts/tests/Shipyard.Contracts.Tests.csprojtooling/carrier-contract-codegen/generate.mjstooling/carrier-contract-codegen/tests/generate.test.mjs
|
|
||
| try { | ||
| const value = await invoke<DataLocationStatus>('get_data_location_status') | ||
| const value = await carrierHostPort().dataLocationStatus({}) as DataLocationStatus |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant files =="
git ls-files | rg 'apps/carrier/src/(membrane/data-location-client|renderer-log)|protocol/.*|generated|bindings|schema|manifest|karot' || true
echo
echo "== target files context =="
for f in apps/carrier/src/membrane/data-location-client.ts apps/carrier/src/renderer-log.ts; do
if [ -f "$f" ]; then
echo "--- $f ($(wc -l < "$f") lines) ---"
sed -n '1,140p' "$f" | cat -n
fi
done
echo
echo "== occurrences of local types and generated imports =="
rg -n "type DataLocationStatus|interface DataLocationStatus|type RendererLogEntry|interface RendererLogEntry|from ['\"][^'\"]*protocol|from ['\"][^'\"]*tauri|generated|Bindings|bindings|DataLocationStatus|RendererLogEntry" apps/carrier/src . --glob '!node_modules' --glob '!dist' --glob '!build' | head -n 200
echo
echo "== package/config hints =="
fd -i 'package.json|tsconfig.json|build.config.*|unplugin|corepack|pnpm-lock.yaml' . | sed 's#^\./##' | head -n 200Repository: Harborline-Software/shipyard
Length of output: 48240
🌐 Web query:
Karota generator types bindings documentation
💡 Result:
There is no documentation for "Karota generator types bindings." It appears that the query may be based on a misunderstanding or a conflation of different terms [1]. Our research indicates the following regarding the terms used: 1. Kubota Generators: There is a well-known manufacturer of power generation equipment called Kubota [2][3]. Their documentation covers various generator series (such as the SQ, GL, J, and KJ series) [2] and technical specifications, but it does not use the terminology "generator types bindings" [4][3]. 2. Karota: This term refers to several unrelated entities, including a graphic design studio [5][6], a location in India (where generator dealers are present) [7][8], a non-profit association [1], and a surname [9][10]. None of these are associated with technical software or generator bindings documentation. 3. Bindings: In a technical context, "bindings" usually refers to software interfaces (e.g., language bindings for an API). There is no evidence connecting this concept to anything named "Karota" [9][10]. If you are looking for information on "Kubota generator" technical specifications or software integration, it is recommended to consult the official Kubota Generator website [2]. If "Karota" refers to a specific software project or library, please provide additional context, as it does not appear to be a standard term in this field.
Citations:
- 1: https://karota.de/2026/03/02/karota-42/
- 2: https://generator.kubota.com/home.html
- 3: https://www.kubotaengine.com/everything-you-need-to-know-about-kubota-generators/
- 4: https://generator.kubota.com/products/60hz/j_series.html
- 5: https://karota.ch/
- 6: https://karota.ch/projekte_category/web/
- 7: https://www.justdial.com/Narnaul/Book-Binding-Services/nct-10052014
- 8: https://www.justdial.com/Narnaul/Generator-Dealers-in-Karota/nct-10227980
- 9: https://linkedin.com/in/eugene-koroto
- 10: https://linkedin.com/in/isaac-karota-276a71202
issue [blocking]: Remove hand-maintained protocol projections.
Generated protocol bindings are the contract source. Local duplicate types can drift when the schema changes.
apps/carrier/src/membrane/data-location-client.ts#L54-L54: import the generatedDataLocationStatustype and remove the local projection.apps/carrier/src/renderer-log.ts#L42-L42: import the generatedRendererLogEntrytype and remove the local projection.
Treat the schema, manifest, and generated bindings as single-source projections; avoid hand-maintained duplicates or drift.
📍 Affects 2 files
apps/carrier/src/membrane/data-location-client.ts#L54-L54(this comment)apps/carrier/src/renderer-log.ts#L42-L42
🤖 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/membrane/data-location-client.ts` at line 54, Replace the
local DataLocationStatus projection in
apps/carrier/src/membrane/data-location-client.ts at lines 54-54 with the
generated DataLocationStatus import and remove the duplicate local type.
Likewise, replace the local RendererLogEntry projection in
apps/carrier/src/renderer-log.ts at lines 42-42 with its generated binding
import and remove the duplicate, keeping generated protocol types as the sole
source of these contracts.
Source: Path instructions
| interface NodeStatus { | ||
| state: 'starting' | 'running' | 'failed' | 'stopped' | ||
| base_url: string | null | ||
| session_token: string | null | ||
| baseUrl: string | null | ||
| sessionToken: string | null |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
issue [blocking]: Use generated protocol types at every Carrier boundary.
The generated contract is bypassed by local type copies and a type assertion. This prevents TypeScript from detecting schema drift.
apps/carrier/src/membrane/nodeClient.ts#L131-L134: remove the localNodeStatusinterface and consume the generated host-port result type.apps/carrier/src/membrane/peer-sync-client.ts#L26-L32: import generatedPeerSyncConfiginstead of maintaining its wire fields locally.apps/carrier/src/sync-status/syncStatusClient.ts#L37-L39: removeas SyncStatus; return the generated type directly or add an explicit field mapping.
As per path instructions, “Treat the schema, manifest, and generated bindings as single sources/projections—avoid hand-maintained duplicates or drift.”
📍 Affects 3 files
apps/carrier/src/membrane/nodeClient.ts#L131-L134(this comment)apps/carrier/src/membrane/peer-sync-client.ts#L26-L32apps/carrier/src/sync-status/syncStatusClient.ts#L37-L39
🤖 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/membrane/nodeClient.ts` around lines 131 - 134, Replace the
hand-maintained NodeStatus interface in
apps/carrier/src/membrane/nodeClient.ts:131-134 with the generated host-port
result type; import and use generated PeerSyncConfig in
apps/carrier/src/membrane/peer-sync-client.ts:26-32 instead of local wire
fields; and in apps/carrier/src/sync-status/syncStatusClient.ts:37-39 remove the
SyncStatus assertion by returning the generated type directly or explicitly
mapping fields.
Source: Path instructions
| import { nodeGet, resolveNode } from '../membrane/nodeClient.js' | ||
|
|
||
| export class HttpCarrierApplicationAdapter implements CarrierApplicationPort { | ||
| async getSyncStatus(_request: EmptyRequest): Promise<CarrierSyncStatus> { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
issue [blocking]: Fix the ESLint error.
_request is unused and triggers @typescript-eslint/no-unused-vars. Mark it as used, or omit the implementation parameter if the interface remains assignable.
Proposed fix
async getSyncStatus(_request: EmptyRequest): Promise<CarrierSyncStatus> {
+ void _request
const node = await resolveNode()📝 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.
| async getSyncStatus(_request: EmptyRequest): Promise<CarrierSyncStatus> { | |
| async getSyncStatus(_request: EmptyRequest): Promise<CarrierSyncStatus> { | |
| void _request | |
| const node = await resolveNode() |
🧰 Tools
🪛 ESLint
[error] 13-13: '_request' 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/carrier/src/protocol/http-carrier-application-adapter.ts` at line 13,
Fix the unused _request parameter in getSyncStatus by omitting it if the method
remains assignable to the interface, or explicitly marking it as used if the
parameter is required. Preserve the existing CarrierSyncStatus behavior and
interface compatibility.
Source: Linters/SAST tools
| return invoke('hull_invoke', { args: request }) | ||
| } | ||
|
|
||
| hullHealth(_request: EmptyRequest): Promise<HealthReport> { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg 'apps/carrier/src/protocol/tauri-carrier-adapter\.ts$' || true
echo "== file excerpt =="
if [ -f apps/carrier/src/protocol/tauri-carrier-adapter.ts ]; then
sed -n '1,120p' apps/carrier/src/protocol/tauri-carrier-adapter.ts | nl -ba
fi
echo "== eslint config/package references =="
git ls-files | rg '(^|/)(eslint\.config\.(js|mjs|cjs|ts)|\.eslint\.cjs|eslint\.config\.(js|mjs|cjs|ts)|package\.json|pnpm-lock\.yaml)$' | while read -r f; do
echo "--- $f"
if [[ "$f" == package.json ]]; then
jq '.scripts // {}, .devDependencies, .dependencies' "$f" 2>/dev/null || sed -n '1,220p' "$f"
elif [[ "$f" == pnpm-lock.yaml ]]; then
rg -n '`@typescript-eslint`|eslint|no-unused-vars|varsIgnorePattern:_|argsIgnorePattern:_|argsIgnorePattern:' "$f" | head -120 || true
else
rg -n '`@typescript-eslint`|no-unused-vars|varsIgnorePattern:|argsIgnorePattern:' "$f" || true
fi
done
echo "== all _request usages in target =="
rg -n '_request|EmptyRequest' apps/carrier/src/protocol/tauri-carrier-adapter.ts || true
echo "== typescript eslint unused params behavior probe if installed =="
node - <<'JS'
fs = require('fs');
try {
const lib = require('`@typescript-eslint/parser`');
console.log('`@typescript-eslint/parser` available');
} catch(e) {
console.log('`@typescript-eslint/parser` unavailable:', e.code || e.message);
}
try {
const lib = require('typescript-eslint');
console.log('typescript-eslint flat config available');
} catch(e) {
console.log('typescript-eslint unavailable:', e.code || e.message);
}
JSRepository: Harborline-Software/shipyard
Length of output: 294
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "== file excerpt =="
sed -n '1,120p' apps/carrier/src/protocol/tauri-carrier-adapter.ts
echo "== all _request/EmptyRequest usages in target =="
rg -n '_request|EmptyRequest' apps/carrier/src/protocol/tauri-carrier-adapter.ts || true
echo "== lint config/package references =="
while IFS= read -r f; do
echo "--- $f"
if [ "$f" = package.json ]; then
sed -n '1,220p' "$f"
elif [ "$f" = pnpm-lock.yaml ]; then
rg -n '`@typescript-eslint`|eslint|no-unused-vars|varsIgnorePattern|argsIgnorePattern' "$f" | sed -n '1,160p' || true
else
rg -n '`@typescript-eslint`|no-unused-vars|varsIgnorePattern|argsIgnorePattern' "$f" || true
fi
done < <(git ls-files | rg '(^|/)(eslint\.config\.(js|mjs|cjs|ts)|\.eslint\.cjs|eslint\.config\.(js|mjs|cjs|ts)|package\.json|pnpm-lock\.yaml)$')Repository: Harborline-Software/shipyard
Length of output: 3722
🏁 Script executed:
#!/bin/bash
set -u
echo "== file excerpt =="
sed -n '1,120p' apps/carrier/src/protocol/tauri-carrier-adapter.ts
echo "== all _request/EmptyRequest usages in target =="
rg -n '_request|EmptyRequest' apps/carrier/src/protocol/tauri-carrier-adapter.ts || true
echo "== lint config/package references =="
while IFS= read -r f; do
echo "--- $f"
if [ "$f" = package.json ]; then
sed -n '1,220p' "$f"
elif [ "$f" = pnpm-lock.yaml ]; then
rg -n '`@typescript-eslint`|eslint|no-unused-vars|varsIgnorePattern|argsIgnorePattern' "$f" | sed -n '1,160p' || true
else
rg -n '`@typescript-eslint`|no-unused-vars|varsIgnorePattern|argsIgnorePattern' "$f" || true
fi
done < <(git ls-files | rg '(^|/)(eslint\.config\.(js|mjs|cjs|ts)|\.eslint\.cjs|eslint\.config\.(js|mjs|cjs|ts)|package\.json|pnpm-lock\.yaml)$')Repository: Harborline-Software/shipyard
Length of output: 3722
🏁 Script executed:
#!/bin/bash
set -u
files=()
while IFS= read -r f; do
files+=("$f")
done < <(git ls-files | rg '(^|/)(eslint\.config\.(js|mjs|cjs|ts)|\.eslint\.cjs|eslint\.config\.(js|mjs|cjs|ts)|package\.json|pnpm-lock\.yaml)$')
count=${`#files`[@]}
echo "found $count files"
for f in "${files[@]}"; do
echo "--- $f"
if [ "$f" = package.json ]; then
sed -n '1,220p' "$f"
elif [ "$f" = pnpm-lock.yaml ]; then
rg -n '`@typescript-eslint`|eslint|no-unused-vars|varsIgnorePattern|argsIgnorePattern' "$f" | sed -n '1,160p' || true
else
rg -n '`@typescript-eslint`|no-unused-vars|varsIgnorePattern|argsIgnorePattern' "$f" || true
fi
doneRepository: Harborline-Software/shipyard
Length of output: 232
🏁 Script executed:
#!/bin/bash
set -u
tmpfile="$(mktemp)"
git ls-files '>' "$tmpfile"
grep -n -E '(^|/)(eslint\.config\.([jmcs]s|ts)|\.eslint\.cjs|package\.json|pnpm-lock\.yaml)$' "$tmpfile" || true
echo "--- target file"
sed -n '1,120p' apps/carrier/src/protocol/tauri-carrier-adapter.ts
echo "--- target imports/usages"
grep -En '_request|EmptyRequest' apps/carrier/src/protocol/tauri-carrier-adapter.ts || true
echo "--- lint package/config references"
grepfile() {
rg -n '`@typescript-eslint`|eslint|no-unused-vars|varsIgnorePattern|argsIgnorePattern' "$1" || true
}
while IFS= read -r f; do
echo "--- $f"
if [ "$(basename "$f")" = package.json ]; then
sed -n '1,220p' "$f"
elif [ "$(basename "$f")" = pnpm-lock.yaml ]; then
grepfile "$f" | sed -n '1,160p'
else
grepfile "$f"
fi
done < "$tmpfile"Repository: Harborline-Software/shipyard
Length of output: 3737
issue [blocking]: Remove unused empty-request parameters.
EmptyRequest is only used on unused _request parameters; drop these parameters and remove the unused import.
Also applies to: lines 45, 49, 58, 62, and 66.
🧰 Tools
🪛 ESLint
[error] 37-37: '_request' 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/carrier/src/protocol/tauri-carrier-adapter.ts` at line 37, Update the
methods at hullHealth and the other affected methods to remove their unused
_request: EmptyRequest parameters, then remove the now-unused EmptyRequest
import. Preserve each method’s existing return type and behavior.
Source: Linters/SAST tools
| describe('HullShellPortAdapter', () => { | ||
| it('routes protocol Invoke only through the injected secured entrypoint', async () => { | ||
| const securedInvoke = vi.fn().mockResolvedValue(success) | ||
| const adapter = new HullShellPortAdapter({ shell: shell(), securedInvoke }) | ||
|
|
||
| await expect(adapter.invoke({ | ||
| capability: 'image', | ||
| core: { prompt: 'harbor at dawn' }, | ||
| correlationId: 'corr-port-1', | ||
| idempotencyKey: 'idem-port-1', | ||
| })).resolves.toMatchObject({ status: 'succeeded' }) | ||
|
|
||
| expect(securedInvoke).toHaveBeenCalledOnce() | ||
| expect(securedInvoke).toHaveBeenCalledWith(expect.objectContaining({ | ||
| capabilityId: 'image', | ||
| correlationId: 'corr-port-1', | ||
| idempotencyKey: 'idem-port-1', | ||
| })) | ||
| }) | ||
|
|
||
| it('denies Secure preflight when no policy inspector is configured', async () => { | ||
| const adapter = new HullShellPortAdapter({ shell: shell(), securedInvoke: vi.fn() }) | ||
| await expect(adapter.secure({ | ||
| operationId: 'hull.invoke', | ||
| capabilityId: 'image', | ||
| correlationId: 'corr-port-2', | ||
| })).resolves.toEqual(expect.objectContaining({ allowed: false, authority: 'CP' })) | ||
| }) | ||
|
|
||
| it('projects composition membership without exposing shell implementation details', async () => { | ||
| const adapter = new HullShellPortAdapter({ shell: shell(), securedInvoke: vi.fn() }) | ||
| await expect(adapter.compose({ | ||
| editionId: 'carrier', | ||
| capabilityIds: ['image', 'tts'], | ||
| })).resolves.toEqual({ | ||
| editionId: 'carrier', | ||
| acceptedCapabilityIds: ['image'], | ||
| rejectedCapabilityIds: ['tts'], | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
suggestion (parity coverage gap): add tests for the remaining HullPort methods.
HullPort declares 8 methods (announce, negotiate, address, secure, invoke, observe, resolve, compose). This file only tests invoke, secure, and compose. announce, negotiate, address, observe, and resolve have no coverage here, so a regression in any of those adapter methods (for example, a field dropped when mapping connection.manifest in announce) would not be caught by this suite.
Also consider strengthening the invoke test at line 43: toMatchObject({ status: 'succeeded' }) only checks one field. Given hull-port-adapter.ts casts the result through as unknown as ProtocolCapabilityResult, a stricter assertion (toEqual(success)) here would catch a field mismatch that the type system currently cannot.
As per path instructions, the code-review-policy.yaml requires that "adapter work needs unit and parity tests."
🤖 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 33 - 73,
Expand the HullShellPortAdapter test suite to cover the remaining HullPort
methods announce, negotiate, address, observe, and resolve, asserting their
mapped inputs and outputs, including connection.manifest fields. Strengthen the
existing invoke test to compare the complete result with success via strict
equality rather than checking only status, preserving coverage for the adapter’s
cast boundary.
Independent deep review — CHANGES REQUESTED, four blocking findingsReviewed from a clean checkout by a reviewer that had not seen the authoring session. Nine candidates were generated and killed by the refute pass — including renderer-controlled principal on Tauri invoke, renderer-controlled session token, an immediate raw-shell bypass, and separate fixture corpora. Four survived, and all four are the change not enforcing what it claims. The ratifier has directed that all four be fixed before merge, because this becomes the source of truth for every future language binding. 1.
|
Four blocking findings from an independent review, all of the same kind: the change asserted properties it did not enforce. The secured Hull entrypoint was a plain function type, so any composition could satisfy it with a raw executor and reach execution with no authenticate, authorize or redaction. It is now branded, its only factory composes the policy path, and a compile-time negative test proves a raw executor cannot be assigned. Inverting that assertion fails the typecheck, so the test is not inert. The generator listed enum, minimum, maximum and additionalProperties as supported and then dropped their meaning outside TypeScript, so the schema restricted a progress value to a range while the C# and Rust bindings accepted anything, and a closed object accepted an injected property. Those keywords now project into real enums, validating accessors and strict deserializers in every language, with negative fixtures proving each rejection. Format and default were removed from the supported set instead, because the generator does not project them and claiming otherwise is the defect being fixed. The TypeScript fixture suite parsed with an assertion that erases at runtime and then checked one property, while the other two languages round-tripped the same files. It now validates and compares the complete payload, and corrupting a fixture turns it red. The drift check and generator tests existed only as scripts nobody ran. They are wired into the suite the repository actually runs and the tooling path is in the workflow filter. That gate then immediately earned itself by catching the next defect. Which was this: consolidating every renderer host call into one adapter meant the eight modules importing it for unrelated reasons dragged the node-discovery command string into the browser bundle, where a web client must not assume a host a browser does not have. The discovery path moves behind a build-time profile alias, so the desktop keeps the real call and the web client compiles a stub. The adapter stays consolidated and its architecture test still passes. Full TypeScript suite passes, including both bundle guards. C# 11 of 11, Rust 2 of 2, whole solution builds with no errors. Refs: 3517
Seventy pure libc field additions with no dependency change, written by this machine's pnpm version whenever a suite runs. Not part of this work, and the third time today a generated file has been swept into a commit by staging everything. Reverted to origin's.
…e the OpenAPI projection Second review, two findings. The secured Hull entrypoint was branded, and a brand lives only in the type system. A caller could write the executor as unknown as the branded type and inject it, and the adapter would call it, so a capability could execute with no authenticate, authorize, idempotency or redaction. The compile-time test was real — inverting it does fail the typecheck — but it only stops the accidental case, which is a smaller guarantee than the one being claimed. So the parameter is gone. The adapter now takes the shell, the host-stamped principal and the policy decision point, builds the secured closure in its constructor and keeps it in a private field. There is nothing left to cast into, and the ordering is owned by the adapter rather than promised by its type. The seams that remain injectable are fail-closed when omitted: without a token consumer a confirmation-bearing command can never pass, and without a principal verifier the host-stamped principal stays attribution-grade rather than widening. The OpenAPI document was hand-maintained and read by neither the generator nor the drift check, so changing an operation regenerated all three bindings, passed the gate, and left OpenAPI describing the old wire contract. That is the second manually maintained canonical representation this work set out not to create. It is now generated from the same manifest and schema as everything else, so the existing check covers it. Proven by making them disagree: the drift check reported all four projections stale and exited non-zero. Full TypeScript gate passes including both bundle guards. Whole solution builds with no errors, C# 11 of 11, Rust 2 of 2, generator tests 7 of 7. Refs: 3517
…r's object Third review. Removing the injectable executor was necessary and not sufficient. The private closure still retained the caller's options object and read its collaborators at invocation time, so keeping a reference and then repointing the policy decision point at an always-permit function and the shell at another executor let a capability execute with authorization reduced to a no-op. The same worked for the host-stamped principal and for the token and verifier seams. A readonly field freezes the reference, not the object, and a type annotation constrains no JavaScript caller. Every collaborator is now read once at construction and closed over as an immutable local, so mutating the caller's object afterwards cannot reach the invocation path. The compatibility factory does the same. Observed before the fix: the substituted policy and shell produced a successful invocation. Observed after: six of six pass, including mutation of the nested secure options. Also, generated TypeScript numeric validation accepted values that are not JSON numbers at all. It now requires a finite number, with negative fixtures for the two infinities and for not-a-number. The C# and Rust bindings already rejected those tokens during deserialization, so only the TypeScript emitter needed it. Full TypeScript gate passes, including all three bundle guards. Refs: 3517
Round two — a fifth finding, and the four fixes re-verifiedTwo things here. A fifth blocking finding found after the previous round, and the result of One of the four is discharged. Three are not, and the fix work introduced two new defects. Every Part one — the fifth findingThe boundary funnel reuses a code that already means something narrower. Blocking. The defect
export const CARRIER_PROTOCOL_INVALID_PAYLOAD_CODE = 'membrane.invalid_native_status' as constThat string is not new. It already exists on The funnel now emits the same string for two further causes, so three distinct failures share one code:
Consequence one — an operator cannot tell the three apartThe contract makes the code the branch key: Cause B tells an operator to go look at the peer or host that sent the payload. Cause C means the Consequence two — "native" is false on the HTTP path
The faultDomain question, and what it turned up
Is Is it right here? No, for a different reason. A second defect, found while checking that
It is not inert. Generated The fixThree codes, split by cause rather than by transport:
Transport identity does not go in the code string. The single entry point becomes two — One follow-up is deliberately not in this branch and will be carded: Why it mattersThe point of this change is that the boundary stops being defined by whichever language happens to be Part two — the four fixes, re-verifiedEach posted finding was checked independently against the working tree before committing. Finding 4 — the drift gate never runs — DISCHARGEDThe whole chain of custody holds: the gate is invoked, the path filters cover both Finding 1 — securedInvoke is a convention, not a structure — NOT DISCHARGEDThe injectable executor is genuinely gone — Construct the adapter with What does now hold: idempotency and redaction are structurally in-path, because Two further gaps. There is still no production construction site — To close: take a branded PDP minted only by a factory that reads the ADR 0128 registry, and a principal Finding 2 — declared-supported keywords silently discarded — NOT DISCHARGEDThe three named counter-examples are now genuinely rejected in all three languages. But the mechanism
Finding 3 — the TypeScript suite does not do the round trip it claims — NOT DISCHARGEDThe Second half: New defect — the tests carrying the security guarantee are outside the TypeScript gate
Where this leaves the branchNot committed, not armed. The fifth finding, the fault-domain narrowing and the tsconfig gap are being Worth recording for the fleet: this is the fourth review round today in which a gate or a test was found |
The ingress funnel reused 'membrane.invalid_native_status', which already meant one narrow thing in the hull membrane: a native envelope with a missing or invalid status. Three distinct causes then shared one code, so an operator could not tell a malformed peer payload from our own producer breaking its own schema, and the word native was false on the HTTP path. Split by cause rather than by transport: - protocol.invalid_inbound_payload, an untrusted payload arriving over a transport - protocol.invalid_outbound_payload, a payload this process constructed and then failed to validate - membrane.invalid_native_status stays untouched in the membrane, meaning what it always meant Two entry points replace the single funnel, so direction cannot be got wrong by omission. Transport identity moves to a typed operationId carried on the error. The ids come from the generated manifest union, so a hand-invented id is a compile error rather than runtime drift. Also narrows the ProtocolError fault domain back to the three values ADR 0124 ratifies. This branch had widened it to five with no amendment and no producer anywhere in the tree; a result carrying one of the two extra values failed the membrane guard and was silently rewritten to provider, whose retryable default is true, converting a terminal fault into a retryable one on the path that decides whether to fall back. All three projections regenerated. Adds a no-emit tsconfig covering the hull protocol tests. The build config excludes test files so they do not ship, which also meant they were never typechecked at all: four AuthorityDecision literals used the wrong field name and the compiler never saw them. This does not close the remaining review findings on the secured-invoke structure, the C# integer-enum projection, or the fixture round trip. Those stay open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…emits Wiring the generated parser into the live sync-status path made a latent schema error reachable. SyncPeerWire in the local node host declares a nullable offline duration and a nullable error code, carries no ignore-when-writing-null, and therefore emits an explicit null for both. The canonical schema typed offlineDurationMs as a plain required integer and errorCode as a plain string, so every projection rejected the response: a peer that has never been reached sends a null duration, and any peer without an error sends a null code, which is the ordinary case. Before the parser was wired in, that path cast and the disagreement was invisible. It stayed invisible to the round-trip suites in all three languages because the shared fixture carried an empty peers array, so SyncPeerStatus was never exercised by any of them. Types both fields as nullable, matching the producer, and regenerates the projections. The minimum and integer checks still apply to a present value. Widens the shared fixture to three peers covering the shapes the node actually emits: reached, never reached with an error code, and a security event with no error code. This puts the model under the TypeScript, C# and Rust round trips rather than leaving it uncovered. Adds direct tests naming the C# record as the producer of record, so a future editor reconciles against the deployed wire rather than adjusting the wire to suit the schema. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round three — adversarial review of the whole change, and the patterns to adoptFive independent lenses over the schema, generator, three projections, trust boundary and gates 22 confirmed, 6 refuted, 1 already fixed and pushed. Refuted candidates are listed at the bottom Fixed on the branch already — it was a live break
Two things made it invisible. Before this branch wired the parser into the live HTTP path, that route Fixed in Confirmed — contract correctness1. 2. 3. 4. Rust makes every required-nullable field missing-tolerant. The generator maps nullable to 5. Explicit 6. The C# projection matches wire names case-insensitively, silently defeating the 7. Confirmed — the generator fails openAll three exit 0 while emitting output that is wrong or will not compile. 8. Enums are only emitted for top-level properties. Recursive array and map types lose the owner 9. Array-of-union is emitted without parentheses ( 10. Generated identifiers are never escaped or checked for uniqueness. Confirmed — gates that cannot fail11. The C# and Rust fixture lists are hand-duplicated from the manifest. TypeScript reads 12. The generator's own rejection paths are never exercised. Only the first test runs the 13. 14. The renderer-reaches-Tauri-only-through-its-adapter guard matches two module specifiers out of Refuted — do not rediscover these
Patterns to adoptRanked by value for cost. The first one is the precondition for most of the others.
Adopt first — the shared corpus. Extend Then differential testing. Same case ids through the real TypeScript validator, C# Then the capability matrix. Replace the flat Then execution attestation. A suite registry where each declared suite emits Explicitly not recommended, having been considered and rejected: generator-emitted semantic The through-lineNine of the twenty-two confirmed findings are the same shape: an artefact that is authoritative in |
Making the contract nullable to match the deployed node surfaced that the carrier's own peer model disagreed with the same wire. It typed the offline duration as a plain number, so a real null was compared numerically and coerced to zero. That is a live defect, not a typing detail. The node emits a null duration whenever there is no successful reach to measure from, and its four-state derivation evaluates the SHOULD branch — strikes or backoff — before the never-reached branch. A peer that has never once connected and is actively failing therefore arrives as SHOULD with a null duration, and isShouldCalm answered that null was below the threshold. The least healthy peer in the roster rendered in the calmest bucket, neutral grey, which is the opposite of what this surface exists to say. Types the duration and error code as nullable, and gives null its own meaning throughout: - a null duration is never calm, because there is no baseline to be inside - the overdue figure reports nothing rather than inventing a number, while the row stays actionable - the device row and activity log suppress the duration suffix instead of printing zero, leaving the never-reached label to speak for itself Adds a regression test for the never-reached SHOULD peer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ntional Removing the injectable executor moved the hole down rather than closing it. The policy decision point was a plain function type and the principal an object with only an id, both supplied by the caller. An always-allow lambda plus any non-empty id reached the real shell invoke with the confirmation branch short-circuited and, with no decision sink, no audit record. The membrane could be reduced to a no-op by its own caller. Both credentials are now branded and have exactly one mint each. The decision point is minted only by a factory that reads the ADR 0128 command authority registry, failing closed to confirmation-required when the registry cannot be read. The principal is minted only from the host OS identity. A bare lambda and an id-shaped object are now compile errors, asserted as such in the type test. Adds the production composition that was missing entirely: every construction of the adapter lived in its own test file, so nothing proved the secured path composed at all. The architecture guard becomes structural rather than a per-file string grep, gains the scanned root it was missing, and stops being case-sensitive. A brand is only as strong as the rule that nothing outside its mint may produce one, and a double cast reopens the bypass in two words where the type system cannot object. The guard now fences casts to either branded credential outside the two mints, with a synthetic forgery proving the fence bites. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…able The schema declares Draft 2020-12 and then wrote the nullable error property as a reference with a sibling type of object-or-null. Under that dialect both apply, and the referenced definition is object-only, so the canonical artefact forbade the null its own shipped fixture uses and all three projections accept. Rewritten as anyOf, which is valid in the declared dialect and generates identical output. The reason it survived is the larger problem: nothing validated the fixtures against the canonical schema. All three language suites exercise the generated projections, never the schema itself, so the artefact presented as canonical had no gate at all. Adds one, using the JSON Schema validator already present in the repository, covering every fixture the manifest lists and the validity of the schema document itself. Verified by mutation: removing a required key from a fixture turns it red and names the constraint. Also makes the sync-status error code required rather than optional. The generated C# property for an optional field carries ignore-when-writing-null and therefore dropped the key on re-serialization, while the node that produces this wire always emits it, so the projection could not reproduce its own producer's payload and the C# round trip drifted. The field was already nullable; requiring it matches the deployed wire and removes one instance of the optional-versus-explicit-null divergence between the three languages. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fetchSyncStatus wrapped its fetch in a blanket catch and returned the sample fixture whenever sample mode was active. That erased the distinction the fallback exists to express. No node reachable is what the sample data is for; a node that IS reachable and returned something unparseable is a real fault, and after this branch wired the protocol parser in, that fault was being swallowed too. The fixture contains a fabricated security event, so a node failing to parse rendered a security event that never happened. The adapter now raises a typed unavailable error, and only that error reaches the sample fallback. Transport and protocol failures rethrow unchanged, so the boundary code, direction and operation id survive to the error surface. With sample mode off, an unreachable node reports unavailable rather than silently succeeding. Three regression tests cover the boundary fault, the request failure and the fabricated security event. Verified by mutation: restoring the blanket catch turns exactly those three red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
negotiate discarded both version fields the caller declares and returned the cached result computed earlier from the shell profile and the runtime offer. A caller declaring an incompatible contract version received compatible true, and the outbound validation this branch added only checked the shape of that answer, never its truth. ADR 0124 makes version negotiation a must-have correctness invariant precisely so the runtimes need no lockstep release, and it was present in name only. The declaration is now parsed through the generated inbound parser, then reconciled with the existing semver rules: the contract major must match the shell's, and each declared capability schema is checked against what the shell actually supports. Accepted capabilities are intersected with the caller's, so a capability the caller cannot speak is no longer reported as agreed. A malformed version fails closed with a reason naming what mismatched. The shell negotiation profile is now carried on the runtime connection, which is what made reconciliation possible at this seam. Five regression tests. Verified by mutation: restoring the cached answer turns three of them red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…g projections The generator exited zero in five distinct situations while writing output that was wrong or would not compile. The keyword allowlist was the root of it: it checked keyword NAMES against a flat set and never asked whether an emitter had a projection for that keyword in that position, so it recorded intent rather than capability. Replaced with a per-keyword, per-target capability matrix that separates semantic keywords from metadata, and generation now aborts BEFORE writing anything when a construct cannot be projected in every target. The tests assert both a non-zero exit and an empty output directory, so a partial write cannot masquerade as success. Constructs now rejected rather than mis-emitted: enums in array-item positions and scalar enum definitions, which produced empty generic arguments in C# and a Rust struct that could not deserialize its own schema value; identifiers needing escaping, sibling names colliding after normalization, and Rust reserved words, none of which were checked at all. Correcting the original report: TypeScript was affected too, since interface fields are emitted raw rather than quoted. Two constructs are now projected properly instead of rejected, because both are legitimate and trivially emittable: an integer enum gets its C# converter, which it never had because the emitter gated converters on the enum being all strings, and an array of a union is parenthesized so the static type stops disagreeing with what the validator accepts. Eight new tests feed the generator synthetic schemas exercising each case. All fifteen pass, the drift check is clean, and the C# suite round-trips at eleven of eleven. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…round the brand Branding the policy decision point and the principal broke the build of a real production caller, the SDK's in-process runtime host. That caller was invisible to the change that introduced the brands because that work was scoped to the hull package, which is also why it reported no production construction site existing. The break is the brand working: it found a caller assembling security credentials ad hoc, which previously typechecked in silence. The wrong repair would be a cast in the SDK. That is precisely the bypass the new architecture fence exists to catch, and it would have to be allowlisted to pass, which defeats the fence. Instead the hull now takes the classifier as a parameter and brands the result, so the SDK keeps injecting the ADR 0128 registry it already owns. The dependency runs from the SDK to the hull, so the hull cannot read that registry itself, which is why the SDK injected it in the first place. That also retires the relative-path registry reader the hull had grown to work around the same problem; two paths to one registry is one too many. The principal gains a mint that validates shape and freezes a snapshot. Its doc comment states plainly what it does NOT establish: it cannot prove where the value came from or that the id is non-anonymous. Overclaiming there would be the same defect this review has been finding everywhere else. The documented public seam for a renderer that read the OS user host-side keeps working, now through the mint. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…hange a landing zone Three confirmed divergences meant the premise of this contract, that the projections agree, was false in ways no suite could see. Rust made every required-nullable field missing-tolerant, because the serde default was emitted from whether a property was required rather than whether it was nullable. Thirteen fields across ten models accepted absence in Rust that TypeScript and C# required. Rust now uses presence-checking deserializers; verified by mutation, removing the error key from a fixture now fails with a missing-field error where it previously passed. An explicit null on an optional property was rejected by TypeScript and accepted by the other two, and the C# projection matched wire names case-insensitively, which silently defeated the strict unknown-property enforcement the generator emits for it. Both now agree across all three. The binding constraint throughout was the recorded trap that unknown-field polarity must be identical in every binding, since a laxer TypeScript accepts what C# and Rust reject. Separately, the contract was a lockstep contract wearing an anti-lockstep policy. ADR 0162 calls an added optional field protocol-minor when old consumers ignore it safely, and with thirty-five closed models that condition was never satisfiable, which sits badly with ADR 0124's requirement that runtimes need no lockstep release. Top-level models stay closed. Nine allowlisted metadata carriers gain a predeclared extensions object, and adding a member inside it is protocol-minor while a new top-level property remains protocol-major. Artifact and ProviderDescriptor keep their existing principled openness and nothing else inherits it. The allowlist is data, and the generator fails closed on an extensions bag anywhere it is not allowed. Verified by mutation: an unknown top-level key on a carrier that HAS a bag is still rejected. The ADR 0162 amendment this implies is drafted but deliberately not written here, since committing an ADR is a ratification act. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ty union Two gate-honesty findings, and one instructive collision between them. The guard named for the invariant that renderer code reaches Tauri only through its adapter matched two module specifiers out of the whole Tauri surface, and a production file bypassed it. It now covers the api namespace and the http and shell plugins. The bypassing file turned out to be a legitimate UI-event use, so it is an explicit named carve-out with a comment rather than a silently widened matcher: a guard that names an invariant it only partly enforces is worse than an honest narrow one. The device capability profile had two disagreeing definitions of one type inside one package, a closed seven-value union by hand and a bare string array in the projection that accepted anything. Checking what actually produces and consumes the field settled which was right: the Rust host emits four of the values, renderer fallbacks supply the other three, and consumers already reject anything outside the set. So the canonical schema now carries the closed union and all three languages enforce it. Doing that put an enum in an array-item position, which the fail-closed capability matrix rejected because C# had no projection there, and generation aborted having written nothing. That was the matrix working correctly, not an obstacle to route around, so the resolution is to project the construct rather than widen the schema back or delete the check. All three targets now emit it, and the matrix DECLARES that position supported per target rather than merely stopping short of rejecting it. Verified that the mechanism survives: an enum injected at a map-value position still aborts before writing, naming the keyword, the target and the position. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…uites The claim that the same fixtures round-trip in TypeScript, C# and Rust was not enforced by anything. TypeScript read the manifest; C# and Rust each enumerated their own hand-written list, so a fixture added to the manifest exercised TypeScript alone and left the other two green and unchanged. The Rust suite was not wired into CI at all, so one of the three had no enforced strictness whatsoever. The TypeScript round trip could also not fail for any input. The generated parser returns its argument by identity, so the test parsed a fixture, cloned the result and compared the clone to the same object. It now loads its expected value independently, which is what makes the comparison mean something. All three suites now read the manifest, and the manifest carries an expected outcome per case rather than being positive-only. Four negative fixtures cover the constraint classes the generator projects: a missing required field, a value outside an enum, a number below a minimum, and an unknown property on a closed model. Every language must reject every reject-case. Rust now runs inside the existing required suite gate rather than beside it. Verified the way the finding demanded: making one negative fixture VALID turns TypeScript, C# and Rust red simultaneously. One edit to shared data, three languages fail. That is the property that was missing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Remediation — all findings from rounds one to three closedNine commits, Security boundaryThe secured invoke path is now structurally unforgeable. Both credentials are branded with a Two things the first pass missed, found and closed here:
The principal mint's doc comment states plainly what it does not establish — it cannot prove The contract is now authoritative
The three projections now agree
The generator fails closedThe flat keyword allowlist recorded intent, not capability. It is now a per-keyword, per-target When that gate later blocked legitimate work it was fixed upstream, not weakened. Adding the CompatibilityTop-level models stay closed. Nine allowlisted metadata carriers gain a predeclared The ADR 0162 amendment this implies is drafted and deliberately not committed, since committing The gates are real now
The proof for this section is a single mutation: making one negative fixture valid turns TypeScript, Note on what this does not coverI wrote or directed most of this code, so my assessment of it is worth less than an independent one. |
There was a problem hiding this comment.
Actionable comments posted: 11
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)
232-242: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winissue: a malformed
opts.principalproduces a rejected promise instead of a structuredCapabilityResultfailure.
mintMembranePrincipalthrows synchronously if its input lacksid/displayName/kind. That call sits before thetry/catchthat wrapsbootShell(). Every other failure path in this function — boot failure,shell.invokefailure — is normalized into aCapabilityResultviamembraneFailure(...). A malformed caller-supplied principal breaks that convention and surfaces as an unhandled rejection instead.🛡️ Proposed fix
- const principal = mintMembranePrincipal(opts.principal ?? currentOsPrincipal()) - let shell: HullShell try { + var principal = mintMembranePrincipal(opts.principal ?? currentOsPrincipal()) shell = await bootShell() } catch (err) { return membraneFailure( - `failed to boot the Hull core: ${err instanceof Error ? err.message : String(err)}`, - 'membrane.carrier_core_boot_fault', + err instanceof Error ? err.message : String(err), + 'membrane.carrier_core_boot_fault', ) }(Adjust variable declaration/scoping and error-code choice to fit the surrounding conventions; the key point is that minting failure should route through the same
membraneFailurepath.)🤖 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 232 - 242, Move the `mintMembranePrincipal(opts.principal ?? currentOsPrincipal())` call into the existing error-handling scope around `bootShell`, adjusting `principal` declaration as needed, so malformed principal input is caught and returned through `membraneFailure(...)` with the appropriate principal-minting error code. Preserve the existing structured failures for shell boot and invocation errors.
🧹 Nitpick comments (6)
apps/hull/src/protocol/hull-port-adapter.ts (1)
241-245: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuenit:
requireConnectionthrows an untypedErroracross a typed boundary.
announce,address, andobservereturnCarrierProtocolBoundaryErrorfor payload failures but a plainErrorfor an unknown runtime. Callers must then branch on message text. Consider a typed protocol error so every failure from this port has one 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.ts` around lines 241 - 245, Update requireConnection to throw the established CarrierProtocolBoundaryError type instead of a plain Error when the runtime is missing, preserving the existing message and ensuring announce, address, and observe expose a consistent typed failure shape.apps/hull/src/membrane/host-principal.ts (1)
18-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: derive the accepted kinds from the contracts package instead of listing them here.
Line 24 hardcodes
'local-os-user'and'service'.PrincipalKindis defined inpackages/contracts/src/principal. If that union gains or renames a member, this check drifts from the canonical definition and rejects a valid principal at the membrane boundary. Export a type guard or a readonly kind list from the contracts package and use it here, so the validation stays derived from one source.As per path instructions: "avoid hand-parallel duplicate copies of single-source things (A4)."
🤖 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/host-principal.ts` around lines 18 - 27, The mintMembranePrincipal validation hardcodes the accepted principal kinds instead of using the canonical contracts definition. Export and use a PrincipalKind type guard or readonly kind list from packages/contracts/src/principal, replacing the inline kind comparison while preserving rejection of unsupported values.Source: Path instructions
tooling/carrier-contract-codegen/generate.mjs (1)
1021-1021: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuenit: both branches of the
reprternary returni64.
value.type === 'integer' ? 'i64' : 'i64'always yieldsi64. Replace it with the literal, or implement the intended distinction.- if (!stringEnum) lines.push(`#[repr(${value.type === 'integer' ? 'i64' : 'i64'})]`) + if (!stringEnum) lines.push('#[repr(i64)]')🤖 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 `@tooling/carrier-contract-codegen/generate.mjs` at line 1021, Update the repr attribute generation in the stringEnum branch to remove the redundant ternary and use the single i64 value directly, unless a genuine type distinction is intended and supported.apps/carrier/src/protocol/tauri-carrier-adapter.ts (1)
42-46: 🔒 Security & Privacy | 🔵 TrivialRoute the Hull-invoke seam through independent review.
This adapter's
hullInvokeforwardscapabilityandcoredata to the nativehull_invokeTauri command. The referenced code-review policy treats Tauri commands and Hull invocation as security-critical and requires an independent reviewer for CP-touching changes rather than self-assertion. Confirm this path has independent sign-off, separate from the author's own verification notes.Based on path instructions: "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/carrier/src/protocol/tauri-carrier-adapter.ts` around lines 42 - 46, Mark the hullInvoke path in TauriCarrierAdapter as requiring independent review under the CP-touching change policy, and record separate reviewer sign-off rather than relying on the author's verification notes. Do not alter the request forwarding or native hull_invoke behavior.apps/hull/tsconfig.test.json (1)
15-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: extend
includeto cover the newsrc/membrane/*.test.tsfiles added by this PR.The comment explains this gate exists because tests "constructed an
AuthorityDecisionwith the wrong field name" undetected. This PR's ownapps/hull/src/membrane/credential-mints.test.tsconstructsAuthorityDecision-shaped objects and sits outsidesrc/protocol/**, so it does not get the same compile-time protection this file was added to provide. Consider widening the glob to include the new membrane test files rather than only the pre-existing runtime-suite backlog exclusion.As per path instructions for
**, "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 `@apps/hull/tsconfig.test.json` around lines 15 - 19, Update the tsconfig.test.json include configuration to cover the new src/membrane/*.test.ts files alongside src/protocol/**/*.ts, ensuring the change’s CI test suite exercises that coverage while leaving the excluded runtime suites unaffected.Source: Path instructions
apps/hull/src/protocol/hull-port-adapter.type-test.ts (1)
9-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winthought: the exec-seam check is a deny-list; consider an exact-shape assertion.
ExecutionPathCannotBeSuppliedonly forbids the three named keyssecuredInvoke,invoke,executor. A future refactor that reintroduces a caller-controlled execution seam under a different property name would pass this check silently. Asserting the exact expected key set ofHullPortAdapterOptions(allow-list) would catch that case too.🤖 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 9 - 15, Replace the deny-list check in ExecutionPathCannotBeSupplied with an exact key-set assertion for HullPortAdapterOptions, using the currently supported option keys as the allow-list. Ensure the compile-time test fails when any additional property is introduced, including renamed execution seams.
🤖 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/protocol/http-carrier-application-adapter.ts`:
- Line 20: Update getSyncStatus so its required interface parameter remains
assignable to CarrierApplicationPort while no longer triggering no-unused-vars;
retain the parameter and explicitly mark _request as used within the method,
without changing the method’s behavior.
In `@apps/hull/src/membrane/no-direct-invoke.arch.test.ts`:
- Around line 21-25: Update SCANNED_ROOTS in the no-direct-invoke architecture
test to include packages/carrier-sdk/src, matching the documented scan scope
alongside the existing Carrier and Hull roots. Preserve the current scanning and
enforcement behavior so carrier-sdk files, including runtime-host.ts, are
checked for direct shell.invoke calls.
- Around line 240-246: Replace the local isBypass closure and its regex-based
assertions with direct calls to hasStructuralBypass from the relevant tests.
Pass each source with a synthetic filename, including one ending in
/apps/hull/src/protocol/hull-port-adapter.ts for the bound-access case, and
preserve the expected true/false outcomes so the tests cover
isSanctionedBoundAccess and isInsideSecureExecutor through the production AST
path.
- Around line 67-99: Replace the regex-based detection in brandForgeryOffenders
with TypeScript AST traversal, matching AsExpression and TypeAssertion nodes
whose asserted type is MembranePrincipal or PolicyDecisionPoint, including
chained assertions such as “as any as …” and angle-bracket casts. Remove the
executableSource comment-stripping dependency from this guard while preserving
BRAND_MINTS exclusions and offender reporting.
In `@apps/hull/src/protocol/hull-port-adapter.ts`:
- Around line 145-146: Replace the self-comparison in the invalidSchema check
with the exported isParsableContractVersion predicate, importing it from
../membrane/negotiate.js. Update the guard to validate each capability schema
version explicitly while preserving the existing invalid-schema handling.
In `@apps/hull/src/protocol/hull-port-adapter.type-test.ts`:
- Line 21: Update the ESLint `@typescript-eslint/no-unused-vars` configuration to
set argsIgnorePattern to '^_' so the unused _command parameters in forgedPdp and
the corresponding function at the other occurrence are accepted; if widening the
configuration is not appropriate, remove the unused parameters while preserving
the AuthorityDecision behavior.
In `@packages/contracts/rust/src/generated.rs`:
- Around line 1006-1023: Update the optional-field handling in generate.mjs so
the errorCode field of SyncPeerStatus generates with serde default and
skip_serializing_if = "Option::is_none" attributes, allowing the JSON key to be
omitted while preserving null support. Regenerate the Rust contracts so
SyncPeerStatus.error_code reflects this configuration, including the
corresponding occurrence around the additional referenced generated field.
In `@packages/contracts/src/__tests__/protocol-schema.test.ts`:
- Line 1: Add the ajv v8 package to the dependencies or devDependencies of
packages/contracts/package.json so the Ajv2020 import in protocol-schema.test.ts
resolves, then regenerate the lockfile by reinstalling dependencies. Keep the
existing import unchanged.
In `@tooling/carrier-contract-codegen/generate.mjs`:
- Around line 607-616: Replace the expression-hash-based mapName generation in
tsValidationLines with a monotonic map counter, declared at module scope and
reset at the start of tsValidator. Increment the counter for each
additionalProperties map and use its value to generate unique const names within
each validator.
- Around line 144-155: Update the securitySchemes validation loop to reject any
scheme whose type is not a supported value, including typos such as htttp. Keep
the existing http and apiKey-specific checks, and ensure unknown types fail
before generateOpenApi copies the manifest into components.securitySchemes.
In `@tooling/carrier-contract-codegen/tests/generate.test.mjs`:
- Around line 137-148: Validate that both export anchors used by the adapter
slices in the test exist before calling slice, and fail the test when either
indexOf lookup returns -1. Keep the existing adapterOptions and adapterClass
assertions unchanged so deleting the guarded logic still makes the named
security checks fail.
---
Outside diff comments:
In `@packages/carrier-sdk/src/runtime-host.ts`:
- Around line 232-242: Move the `mintMembranePrincipal(opts.principal ??
currentOsPrincipal())` call into the existing error-handling scope around
`bootShell`, adjusting `principal` declaration as needed, so malformed principal
input is caught and returned through `membraneFailure(...)` with the appropriate
principal-minting error code. Preserve the existing structured failures for
shell boot and invocation errors.
---
Nitpick comments:
In `@apps/carrier/src/protocol/tauri-carrier-adapter.ts`:
- Around line 42-46: Mark the hullInvoke path in TauriCarrierAdapter as
requiring independent review under the CP-touching change policy, and record
separate reviewer sign-off rather than relying on the author's verification
notes. Do not alter the request forwarding or native hull_invoke behavior.
In `@apps/hull/src/membrane/host-principal.ts`:
- Around line 18-27: The mintMembranePrincipal validation hardcodes the accepted
principal kinds instead of using the canonical contracts definition. Export and
use a PrincipalKind type guard or readonly kind list from
packages/contracts/src/principal, replacing the inline kind comparison while
preserving rejection of unsupported values.
In `@apps/hull/src/protocol/hull-port-adapter.ts`:
- Around line 241-245: Update requireConnection to throw the established
CarrierProtocolBoundaryError type instead of a plain Error when the runtime is
missing, preserving the existing message and ensuring announce, address, and
observe expose a consistent typed failure shape.
In `@apps/hull/src/protocol/hull-port-adapter.type-test.ts`:
- Around line 9-15: Replace the deny-list check in ExecutionPathCannotBeSupplied
with an exact key-set assertion for HullPortAdapterOptions, using the currently
supported option keys as the allow-list. Ensure the compile-time test fails when
any additional property is introduced, including renamed execution seams.
In `@apps/hull/tsconfig.test.json`:
- Around line 15-19: Update the tsconfig.test.json include configuration to
cover the new src/membrane/*.test.ts files alongside src/protocol/**/*.ts,
ensuring the change’s CI test suite exercises that coverage while leaving the
excluded runtime suites unaffected.
In `@tooling/carrier-contract-codegen/generate.mjs`:
- Line 1021: Update the repr attribute generation in the stringEnum branch to
remove the redundant ternary and use the single i64 value directly, unless a
genuine type distinction is intended and supported.
🪄 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: abbf6256-9937-40a8-ab9d-9fa7dea92930
⛔ Files ignored due to path filters (2)
packages/contracts/Generated/CarrierProtocol.g.csis excluded by!**/generated/**packages/contracts/src/generated/carrier-protocol.generated.tsis excluded by!**/*.generated.*,!**/generated/**
📒 Files selected for processing (45)
.github/workflows/ts-suites.ymlapps/carrier/src/protocol/http-carrier-application-adapter.test.tsapps/carrier/src/protocol/http-carrier-application-adapter.tsapps/carrier/src/protocol/tauri-carrier-adapter.test.tsapps/carrier/src/protocol/tauri-carrier-adapter.tsapps/carrier/src/protocol/tauri-node-discovery-adapter.test.tsapps/carrier/src/protocol/tauri-node-discovery-adapter.tsapps/carrier/src/protocol/webclient-node-discovery-adapter.tsapps/carrier/src/sync-status/L2DevicePanel.tsxapps/carrier/src/sync-status/L4ActivityLog.tsxapps/carrier/src/sync-status/syncStateUtils.test.tsapps/carrier/src/sync-status/syncStateUtils.tsapps/carrier/src/sync-status/syncStatusClient.test.tsapps/carrier/src/sync-status/syncStatusClient.tsapps/carrier/src/sync-status/types.tsapps/carrier/vite.config.tsapps/hull/package.jsonapps/hull/src/index.tsapps/hull/src/membrane/credential-mints.test.tsapps/hull/src/membrane/host-principal.tsapps/hull/src/membrane/no-direct-invoke.arch.test.tsapps/hull/src/membrane/pep.tsapps/hull/src/membrane/runtime-connection.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.jsonpackages/carrier-sdk/src/runtime-host.tspackages/contracts/protocol/README.mdpackages/contracts/protocol/fixtures/carrier-sync-status.jsonpackages/contracts/protocol/fixtures/device-capability-profile.jsonpackages/contracts/protocol/manifest.jsonpackages/contracts/protocol/openapi/carrier-application.openapi.jsonpackages/contracts/protocol/schemas/carrier-protocol.schema.jsonpackages/contracts/rust/src/generated.rspackages/contracts/rust/src/lib.rspackages/contracts/src/__tests__/protocol-fixtures.test.tspackages/contracts/src/__tests__/protocol-schema.test.tspackages/contracts/src/__tests__/protocol-validation.test.tspackages/contracts/src/protocol-validation.tspackages/contracts/src/protocol.tspackages/contracts/tests/ProtocolFixtureTests.csscripts/ts-suites.mjstooling/carrier-contract-codegen/generate.mjstooling/carrier-contract-codegen/tests/generate.test.mjs
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/contracts/src/protocol.ts
- apps/hull/src/membrane/runtime-connection.ts
- packages/contracts/protocol/fixtures/device-capability-profile.json
- apps/carrier/src/sync-status/syncStatusClient.ts
- packages/contracts/protocol/schemas/carrier-protocol.schema.json
| } | ||
|
|
||
| export class HttpCarrierApplicationAdapter implements CarrierApplicationPort { | ||
| async getSyncStatus(_request: EmptyRequest) { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major
issue [blocking]: Fix the unused getSyncStatus parameter.
@typescript-eslint/no-unused-vars still reports _request. The underscore prefix does not suppress this rule. Keep the interface parameter and mark it as used, or remove it only if the implementation remains assignable to CarrierApplicationPort. This repeats the unresolved issue from the previous review.
Proposed fix
async getSyncStatus(_request: EmptyRequest) {
+ void _request
const node = await resolveNode()🧰 Tools
🪛 ESLint
[error] 20-20: '_request' 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/carrier/src/protocol/http-carrier-application-adapter.ts` at line 20,
Update getSyncStatus so its required interface parameter remains assignable to
CarrierApplicationPort while no longer triggering no-unused-vars; retain the
parameter and explicitly mark _request as used within the method, without
changing the method’s behavior.
Source: Linters/SAST tools
| /** | ||
| * A cast that FORGES one of the membrane's branded credentials. | ||
| * | ||
| * Branding `MembranePrincipal` and `PolicyDecisionPoint` stops a caller passing a bare object or | ||
| * lambda, which is what closed the always-allow bypass. But a brand is only as strong as the rule | ||
| * that nothing outside its mint may produce one: `{ id: 'os:anyone' } as unknown as | ||
| * MembranePrincipal` typechecks and reopens the bypass in two words. The type system cannot forbid | ||
| * that, so the fence lives here. | ||
| */ | ||
| const BRAND_FORGERY = /\bas\s+(?:unknown\s+as\s+)?(?:MembranePrincipal|PolicyDecisionPoint)\b/ | ||
|
|
||
| /** The only files permitted to mint a branded credential — the mints themselves. */ | ||
| const BRAND_MINTS = [ | ||
| join('membrane', 'host-principal.ts'), | ||
| join('membrane', 'pep.ts'), | ||
| ] | ||
|
|
||
| /** Files that forge a branded membrane credential outside its sanctioned mint. */ | ||
| function brandForgeryOffenders(roots: string[]): string[] { | ||
| const out: string[] = [] | ||
| for (const root of roots) { | ||
| for (const file of collectSources(root)) { | ||
| if (BRAND_MINTS.some((mint) => file.endsWith(mint))) continue | ||
| if (BRAND_FORGERY.test(executableSource(readFileSync(file, 'utf8')))) out.push(file) | ||
| } | ||
| } | ||
| return out | ||
| } | ||
|
|
||
| /** Remove comments before applying the source guard; prose must not satisfy the gate. */ | ||
| function executableSource(source: string): string { | ||
| return source.replace(/\/\/[^\n]*|\/\*[\s\S]*?\*\//g, '') | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
Does TypeScript's "as any as X" double type assertion bypass the same structural-type check as "as unknown as X"?
💡 Result:
Yes, both as any as X and as unknown as X function identically as double type assertions to bypass TypeScript's structural type-checking rules [1][2][3]. TypeScript normally restricts type assertions to cases where one type is a subtype of the other, or vice versa, to prevent accidental, unsafe coercions [4][2][5]. When these conditions are not met, a direct assertion (e.g., value as TargetType) results in a compile-time error [5][6]. To bypass this, developers use a "double assertion" (or "multi-step assertion") by converting the value to an intermediate type that is compatible with all other types—either any or unknown [4][2][5]. The behavior is equivalent for both: 1. The expression value as unknown as TargetType asserts that value is unknown (which is allowed because every type can be converted to unknown) [7][3]. 2. It then asserts that the unknown value is TargetType (which is allowed because unknown can be asserted to any type) [2][3]. 3. The same logic applies to value as any as TargetType [1][2]. While both are equally effective at bypassing structural checks and are technically equivalent in terms of compile-time safety, they differ in maintainability and strictness: - Using as unknown as X is generally preferred in modern TypeScript development [1][2][3]. It is more explicit about the developer's intent to treat the value as "something I don't yet know" rather than "something I don't care about" (which any implies) [3]. - Many linting configurations (such as @typescript-eslint/no-explicit-any) flag as any but allow as unknown, making as unknown the standard choice for enforcing cleaner, more trackable code [1][2][3].
Citations:
- 1: https://javascript.plainenglish.io/why-as-unknown-as-type-should-be-banned-bfd946c3e8d6
- 2: https://basarat.gitbook.io/typescript/type-system/type-assertion
- 3: https://stackoverflow.com/questions/64937030/typescripts-as-unknow-as-number-usage
- 4: https://betterstack.com/community/guides/scaling-nodejs/type-assertions-casting/
- 5: https://www.typescriptlang.org/docs/handbook/2/everyday-types.html
- 6: Type assertion considers empty object compatible with any interface microsoft/TypeScript#25539
- 7: https://www.typescriptlang.org/docs/handbook/type-compatibility.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find policy and target file:"
git ls-files '_shared/engineering/code-review-policy.yaml' 'apps/hull/src/membrane/no-direct-invoke.arch.test.ts' | sed -n '1,20p'
echo
echo "Inspect target file around issue:"
wc -l apps/hull/src/membrane/no-direct-invoke.arch.test.ts
sed -n '1,180p' apps/hull/src/membrane/no-direct-invoke.arch.test.ts
echo
echo "Search AST-based hasStructuralBypass references:"
rg -n "hasStructuralBypass|no-direct-invoke|brandForgeryOffenders|BRAND_FORGERY|PolicyDecisionPoint|MembranePrincipal" apps/hull _shared -S || trueRepository: Harborline-Software/shipyard
Length of output: 19204
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check test runner and TypeScript availability:"
node - <<'JS'
try {
console.log('ts_available', !!require.resolve('typescript'))
console.log('ts_version', require('typescript').version)
} catch (e) {
console.log('ts_available false')
console.log(String(e))
}
JS
echo
echo "Behavioral source-guard probe:"
node - <<'JS'
const BRAND_FORGERY = /\bas\s+(?:unknown\s+as\s+)?(?:MembranePrincipal|PolicyDecisionPoint)\b/
const BRAND_MINTS = []
function executableSource(source) {
return source.replace(/\/\/[^\n]*|\/\*[\s\S]*?\*\//g, '')
}
function brandForgeryOffenders(sources) {
const out = []
for (const file of Object.keys(sources)) {
const source = sources[file]
if (BRAND_FORGERY.test(executableSource(source))) out.push(file)
}
return out
}
const cases = {
'as unknown as MembranePrincipal': "const p = { id: 'os:anyone' } as unknown as MembranePrincipal"
, 'as any as MembranePrincipal': "const p = { id: 'os:anyone' } as any as MembranePrincipal"
, 'angle-bracket cast': "const p = <MembranePrincipal>{ id: 'os:anyone' }"
, 'line-comment with slash-in-string before cast': "const url = 'http://example.com?//x'; const p = { id: 'os:anyone' } as unknown as MembranePrincipal"
, 'string-comment with slash-in-string before direct cast': "const url = 'http://example.com?//x'; const d = ((c) => ({ authority: 'AP' })) as PolicyDecisionPoint"
, 'literal containing token only': "const s = 'MembranePrincipal'; const d = ((c) => ({ authority: 'AP' })) as PolicyDecisionPoint"
}
for (const [file, source] of Object.entries(cases)) {
const stripped = executableSource(source)
const hit = BRAND_FORGERY.test(stripped)
console.log(file, JSON.stringify({stripped, hit}))
}
console.log('files_detected', brandForgeryOffenders(cases).sort())
const ts = require('typescript')
const targetTypes = new Set(['MembranePrincipal', 'PolicyDecisionPoint'])
function asOfNode(typeNode) {
if (!ts.isTypeNode(typeNode)) return
switch (typeNode.kind) {
case ts.SyntaxKind.AnyKeyword:
return 'unknown'
case ts.SyntaxKind.TypeReference:
return typeNode.typeName.getText()
}
return '?'
}
function getTarget(file, source, target) {
const tree = ts.createSourceFile(file, source, ts.ScriptTarget.Latest, true, ts.ScriptKind.TS)
let targetNode = null
function visit(node) {
if (targetNode) return
if (ts.isAsExpression(node)) {
if (node.type.getText() === target) targetNode = {kind: 'asExpression', node}
else if (ts.isAsExpression(node.expression) && node.expression.type.getText() === target) return true
}
if (ts.isTypeAssertionExpression(node) && ts.isTypeReference(node.type) && node.type.typeName.getText() === target) targetNode = {kind: 'typeAssertion', node}
ts.forEachChild(node, visit)
}
ts.forEachChild(tree, visit)
return targetNode
}
for (const [desc, source] of Object.entries(cases.slice?.0 || cases)) {
for (const target of targetTypes) {
const found = getTarget('probe.ts', source, target)
if (!found) continue
console.log(desc, target, found.kind)
}
}
JSRepository: Harborline-Software/shipyard
Length of output: 979
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant test expectations:"
sed -n '250,268p' apps/hull/src/membrane/no-direct-invoke.arch.test.ts
echo
echo "Behavioral source-guard probe:"
node - <<'JS'
const BRAND_FORGERY = /\bas\s+(?:unknown\s+as\s+)?(?:MembranePrincipal|PolicyDecisionPoint)\b/
function executableSource(source) {
return source.replace(/\/\/[^\n]*|\/\*[\s\S]*?\*\//g, '')
}
const cases = [
['as unknown as MembranePrincipal', "const p = { id: 'os:anyone' } as unknown as MembranePrincipal"],
['as any as MembranePrincipal', "const p = { id: 'os:anyone' } as any as MembranePrincipal"],
['angle-bracket cast', "const p = <MembranePrincipal>{ id: 'os:anyone' }"],
['line-comment with slash-in-string before cast', "const url = 'http://example.com?//x'; const p = { id: 'os:anyone' } as unknown as MembranePrincipal"],
['string-comment slash-in-template before direct cast', "const url = `${'http://example.com?//x'}`; const d = ((c) => ({ authority: 'AP' })) as PolicyDecisionPoint"],
]
for (const [file, source] of cases) {
const stripped = executableSource(source)
const hit = BRAND_FORGERY.test(stripped)
console.log(`${file}\tstripped=${JSON.stringify(stripped)}\thit=${hit}`)
}
const ts = require('typescript')
function targetInAsOrTypeAssertion(source, target) {
const tree = ts.createSourceFile('probe.ts', source, ts.ScriptTarget.Latest, true, ts.ScriptKind.TS)
let found = null
ts.forEachChild(tree, function visit(node) {
if (found) return
if (ts.isAsExpression(node)) {
const text = node.type.getText()
if (text === target || ts.isAsExpression(node.expression) && node.expression.type.getText() === target) found = {kind: 'asExpression', typeText: text}
} else if (ts.isTypeAssertionExpression(node) && ts.isTypeReference(node.type) && node.type.typeName.getText() === target) {
found = {kind: 'typeAssertion', typeText: target}
}
if (!found) ts.forEachChild(node, visit)
})
return found
}
console.log(JSON.stringify([
['as unknown as MembranePrincipal', targetInAsOrTypeAssertion("const p = { id: 'os:anyone' } as unknown as MembranePrincipal", 'MembranePrincipal')],
['as any as MembranePrincipal', targetInAsOrTypeAssertion("const p = { id: 'os:anyone' } as any as MembranePrincipal", 'MembranePrincipal')],
['angle-bracket cast', targetInAsOrTypeAssertion("const p = <MembranePrincipal>{ id: 'os:anyone' }", 'MembranePrincipal')],
], null, 2))
JSRepository: Harborline-Software/shipyard
Length of output: 2571
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect TypeScript package metadata:"
node - <<'JS'
const fs = require('fs')
const pkg = JSON.parse(fs.readFileSync('node_modules/typescript/package.json', 'utf8'))
console.log('typescript', pkg.version, pkg.types)
console.log('main', pkg.main)
console.log('scripts', Object.fromEntries(Object.entries(pkg.scripts || {}).filter(([k]) => k === 'test' || k === 'test:offline' || k.startsWith('test'))))
JS
echo
echo "TypeScript programmatic probe using bundled lib definitions:"
node - <<'JS'
const ts = require('typescript')
function targetInAsOrTypeAssertion(source, targetName) {
const tree = ts.createSourceFile(
'probe.ts',
source,
ts.ScriptTarget.Latest,
true,
ts.ScriptKind.TS
)
let found = null
function visit(node) {
if (found) return
if (ts.isAsExpression(node)) {
const typeText = node.type.getText()
if (typeText === targetName || ts.isAsExpression(node.expression) && node.expression.type.getText() === targetName) {
found = {kind: 'asExpression', typeText}
}
}
if (!found && ts.isTypeAssertionExpression(node) && ts.isTypeReference(node.type) && node.type.typeName.getText() === targetName) {
found = {kind: 'typeAssertion', typeText}
}
if (!found) ts.forEachChild(node, visit)
}
ts.forEachChild(tree, visit)
return found
}
const cases = [
["const p = { id: 'os:anyone' } as unknown as MembranePrincipal", 'MembranePrincipal'],
["const p = { id: 'os:anyone' } as any as MembranePrincipal", 'MembranePrincipal'],
["const p = <MembranePrincipal>{ id: 'os:anyone' }", 'MembranePrincipal'],
]
console.log(JSON.stringify(cases.map(source => [source, targetInAsOrTypeAssertion(source, 'MembranePrincipal')]), null, 2))
JSRepository: Harborline-Software/shipyard
Length of output: 1002
🌐 Web query:
TypeScript 7 syntax "as any as X" "as any as" type assertion
💡 Result:
The syntax described is not a specific feature of a version called TypeScript 7, but rather a chained type assertion pattern used in TypeScript to bypass strict type-checking rules [1][2]. This pattern, often written as expr as any as T or expr as unknown as T, is known as a double assertion [1][2]. TypeScript generally prohibits assertions between two types if neither type is a subtype of the other, as this often indicates an error in logic [1]. However, when you need to perform such a conversion—for example, when you are certain that a value is of a specific type despite what the compiler can infer—you can use an intermediate type that is compatible with all types [1][2]. By first asserting the value to any (which is compatible with everything) or unknown (the top type), you "break" the compiler's strict safety check and can then assert the value to your desired target type T [1][2]. Key points regarding this pattern: 1. Functionality: It treats the expression as the target type T, effectively instructing the compiler to suppress any type mismatch errors [3][2]. 2. Safety: It is considered inherently unsafe because it hides potential type mismatches from the compiler [3][4]. It does not change the actual runtime behavior of the code; it only affects compile-time type checking [3][2]. 3. any vs unknown: Using as unknown as T is often preferred over as any as T by linting rules (such as @typescript-eslint/no-explicit-any) because unknown is more type-safe; it forces you to explicitly assert the type before using it, whereas any silently disables all further type-checking on that value [5][6]. 4. Best Practices: It is widely recommended to avoid this pattern whenever possible [6][4]. Instead, developers are encouraged to use type guards, define more accurate types, or use adapter layers to handle data structures, as double assertions can hide bugs and lead to fragile code [3][4][7].
Citations:
- 1: https://www.typescriptlang.org/docs/handbook/2/everyday-types.html
- 2: https://betterstack.com/community/guides/scaling-nodejs/type-assertions-casting/
- 3: https://dev.to/typescriptteatime/typescripts-as-keyword-might-not-be-what-you-think-2bpo
- 4: Clean up
as anytype assertions in the codebase microsoft/vscode#269213 - 5: https://basarat.gitbook.io/typescript/type-system/type-assertion
- 6: https://javascript.plainenglish.io/why-as-unknown-as-type-should-be-banned-bfd946c3e8d6
- 7: https://thelinuxcode.com/type-assertions-in-typescript-a-practical-production-focused-guide/
issue [blocking]: use AST coverage for the brand-mint fence.
brandomForgeryOffenders() still relies on text matching, while hasStructuralBypass() already uses the TypeScript AST. The brand guard can miss valid double assertions such as as any as MembranePrincipal and angle-bracket casts like <MembranePrincipal>x, and the comment-removal regex can strip // that appears inside strings/templates on the same line as a real forgery. Parse MembranePrincipal and PolicyDecisionPoint with TypeScript for AsExpression and TypeAssertion nodes instead of regex filtering.
🤖 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 detection in brandForgeryOffenders with TypeScript AST
traversal, matching AsExpression and TypeAssertion nodes whose asserted type is
MembranePrincipal or PolicyDecisionPoint, including chained assertions such as
“as any as …” and angle-bracket casts. Remove the executableSource
comment-stripping dependency from this guard while preserving BRAND_MINTS
exclusions and offender reporting.
| const invalidSchema = Object.entries(declaration.capabilitySchemaVersions) | ||
| .find(([, schemaVersion]) => !isContractVersionCompatible(schemaVersion, schemaVersion)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
issue: isContractVersionCompatible(schemaVersion, schemaVersion) compares a value with itself.
The call always compares equal majors, so it returns true for every parseable version and false only when parseSemver fails. The check therefore works today purely as a parse-validity test, and the name states something else. If isContractVersionCompatible later short-circuits identical inputs to true, the "capability-schema-version invalid" branch silently stops firing and malformed declarations pass negotiation. Use an explicit parse predicate.
♻️ Proposed change
- const invalidSchema = Object.entries(declaration.capabilitySchemaVersions)
- .find(([, schemaVersion]) => !isContractVersionCompatible(schemaVersion, schemaVersion))
+ const invalidSchema = Object.entries(declaration.capabilitySchemaVersions)
+ .find(([, schemaVersion]) => !isParsableContractVersion(schemaVersion))Export isParsableContractVersion from ../membrane/negotiate.js so the intent is explicit and the guard is independently testable.
🤖 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 145 - 146, Replace
the self-comparison in the invalidSchema check with the exported
isParsableContractVersion predicate, importing it from ../membrane/negotiate.js.
Update the guard to validate each capability schema version explicitly while
preserving the existing invalid-schema handling.
| #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] | ||
| #[serde(deny_unknown_fields)] | ||
| pub struct SyncPeerStatus { | ||
| #[serde(rename = "deviceId")] | ||
| pub device_id: String, | ||
| #[serde(rename = "label")] | ||
| pub label: String, | ||
| #[serde(rename = "state")] | ||
| pub state: SyncPeerStatusState, | ||
| #[serde(rename = "lastReachedAt", deserialize_with = "deserialize_sync_peer_status_last_reached_at")] | ||
| pub last_reached_at: Option<String>, | ||
| #[serde(rename = "offlineDurationMs", deserialize_with = "deserialize_sync_peer_status_offline_duration_ms")] | ||
| pub offline_duration_ms: Option<i64>, | ||
| #[serde(rename = "isSecurityEvent")] | ||
| pub is_security_event: bool, | ||
| #[serde(rename = "errorCode", deserialize_with = "deserialize_sync_peer_status_error_code")] | ||
| pub error_code: Option<String>, | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
issue (blocking): SyncPeerStatus.error_code diverges from the optional errorCode field in TypeScript.
The PeerStatus interface declares errorCode?: string | null — optional and nullable. Its siblings lastReachedAt and offlineDurationMs are declared as string | null and number | null (not optional), and the generated Rust fields for those two correctly omit default, requiring the JSON key but allowing null. error_code at line 1021-1023 uses the same "required key" pattern, but TypeScript allows the errorCode key to be absent entirely. A producer that omits errorCode (a legitimate payload per the TypeScript contract) fails to deserialize in Rust with a missing-field error.
This is the same class of bug the prior review flagged for other fields (RuntimeHealth.detail, NodeStatus.base_url/session_token, etc.), which appears correctly fixed elsewhere in this file, but error_code still needs the fix. Fix the generator in tooling/carrier-contract-codegen/generate.mjs so it treats errorCode as optional (add default, skip_serializing_if = "Option::is_none"), then regenerate.
🔧 Expected shape of the generated field after the fix
- #[serde(rename = "errorCode", deserialize_with = "deserialize_sync_peer_status_error_code")]
+ #[serde(rename = "errorCode", default, skip_serializing_if = "Option::is_none", deserialize_with = "deserialize_sync_peer_status_error_code")]
pub error_code: Option<String>,#!/bin/bash
# Description: Confirm the schema marks errorCode optional and locate the generator's optional-field logic.
fd -t f 'carrier-protocol.schema.json' packages/contracts --exec rg -n -B3 -A10 '"SyncPeerStatus"' {}
fd -t f 'generate.mjs' tooling --exec rg -n -B5 -A15 'skip_serializing_if|default,' {}Also applies to: 1044-1050
🤖 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/contracts/rust/src/generated.rs` around lines 1006 - 1023, Update
the optional-field handling in generate.mjs so the errorCode field of
SyncPeerStatus generates with serde default and skip_serializing_if =
"Option::is_none" attributes, allowing the JSON key to be omitted while
preserving null support. Regenerate the Rust contracts so
SyncPeerStatus.error_code reflects this configuration, including the
corresponding occurrence around the additional referenced generated field.
| const securitySchemes = candidateManifest.securitySchemes ?? {} | ||
| for (const [name, scheme] of Object.entries(securitySchemes)) { | ||
| if (!/^[A-Za-z][A-Za-z0-9_-]*$/.test(name)) { | ||
| throw new Error(`invalid security scheme name ${name}`) | ||
| } | ||
| if (scheme.type === 'http' && typeof scheme.scheme !== 'string') { | ||
| throw new Error(`security scheme ${name}: http schemes must declare scheme`) | ||
| } | ||
| if (scheme.type === 'apiKey' && !['header', 'query', 'cookie'].includes(scheme.in)) { | ||
| throw new Error(`security scheme ${name}: apiKey schemes must declare header, query, or cookie in`) | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
issue: the security-scheme validator accepts an unknown type.
The loop checks the shape only when scheme.type is http or apiKey. A scheme with any other type value, including a typo such as htttp, passes validation. generateOpenApi then copies securitySchemes verbatim into components.securitySchemes, so an unenforceable scheme reaches the published contract. The manifest is the single source of truth for the security surface, so this gate must fail closed.
🔒 Proposed fix to reject unknown scheme types
if (!/^[A-Za-z][A-Za-z0-9_-]*$/.test(name)) {
throw new Error(`invalid security scheme name ${name}`)
}
+ if (!['http', 'apiKey'].includes(scheme.type)) {
+ throw new Error(`security scheme ${name}: unsupported type ${String(scheme.type)}`)
+ }
if (scheme.type === 'http' && typeof scheme.scheme !== 'string') {As per path instructions: "A protective gate must be fail-closed by default" and "Maintain one canonical schema/manifest source of truth".
📝 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 securitySchemes = candidateManifest.securitySchemes ?? {} | |
| for (const [name, scheme] of Object.entries(securitySchemes)) { | |
| if (!/^[A-Za-z][A-Za-z0-9_-]*$/.test(name)) { | |
| throw new Error(`invalid security scheme name ${name}`) | |
| } | |
| if (scheme.type === 'http' && typeof scheme.scheme !== 'string') { | |
| throw new Error(`security scheme ${name}: http schemes must declare scheme`) | |
| } | |
| if (scheme.type === 'apiKey' && !['header', 'query', 'cookie'].includes(scheme.in)) { | |
| throw new Error(`security scheme ${name}: apiKey schemes must declare header, query, or cookie in`) | |
| } | |
| } | |
| const securitySchemes = candidateManifest.securitySchemes ?? {} | |
| for (const [name, scheme] of Object.entries(securitySchemes)) { | |
| if (!/^[A-Za-z][A-Za-z0-9_-]*$/.test(name)) { | |
| throw new Error(`invalid security scheme name ${name}`) | |
| } | |
| if (!['http', 'apiKey'].includes(scheme.type)) { | |
| throw new Error(`security scheme ${name}: unsupported type ${String(scheme.type)}`) | |
| } | |
| if (scheme.type === 'http' && typeof scheme.scheme !== 'string') { | |
| throw new Error(`security scheme ${name}: http schemes must declare scheme`) | |
| } | |
| if (scheme.type === 'apiKey' && !['header', 'query', 'cookie'].includes(scheme.in)) { | |
| throw new Error(`security scheme ${name}: apiKey schemes must declare header, query, or cookie in`) | |
| } | |
| } |
🤖 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 `@tooling/carrier-contract-codegen/generate.mjs` around lines 144 - 155, Update
the securitySchemes validation loop to reject any scheme whose type is not a
supported value, including typos such as htttp. Keep the existing http and
apiKey-specific checks, and ensure unknown types fail before generateOpenApi
copies the manifest into components.securitySchemes.
Source: Path instructions
| } else if (type === 'object') { | ||
| if (typeof value.additionalProperties === 'object') { | ||
| const mapName = `map${Math.abs(expression.split('').reduce((sum, char) => sum + char.charCodeAt(0), 0))}` | ||
| lines.push(`${bodyIndent}const ${mapName} = protocolObject(${expression}, ${pathExpression})`) | ||
| lines.push(`${bodyIndent}for (const [key, entry] of Object.entries(${mapName})) {`) | ||
| lines.push(...tsValidationLines('entry', `${pathExpression} + '.' + key`, value.additionalProperties, `${bodyIndent} `)) | ||
| lines.push(`${bodyIndent}}`) | ||
| } else { | ||
| lines.push(`${bodyIndent}protocolObject(${expression}, ${pathExpression})`) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
issue: the map validator name can collide and emit duplicate const declarations.
mapName is derived from the sum of the character codes of expression. Two different property paths in the same generated validator can produce the same sum, for example object["ab"] and object["ba"]. The generator then emits two const map<N> declarations in one function scope, and the generated TypeScript does not compile. Use a monotonic counter per validator instead of a hash of the expression.
🛠️ Proposed fix sketch
- const mapName = `map${Math.abs(expression.split('').reduce((sum, char) => sum + char.charCodeAt(0), 0))}`
+ mapCounter += 1
+ const mapName = `map${mapCounter}`Declare let mapCounter = 0 at module scope and reset it at the start of tsValidator.
🤖 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 `@tooling/carrier-contract-codegen/generate.mjs` around lines 607 - 616,
Replace the expression-hash-based mapName generation in tsValidationLines with a
monotonic map counter, declared at module scope and reset at the start of
tsValidator. Increment the counter for each additionalProperties map and use its
value to generate unique const names within each validator.
| const adapterClass = adapter.slice( | ||
| adapter.indexOf('export class HullShellPortAdapter'), | ||
| ) | ||
| const adapterOptions = adapter.slice( | ||
| adapter.indexOf('export interface HullPortAdapterOptions'), | ||
| adapter.indexOf('export class HullShellPortAdapter'), | ||
| ) | ||
| assert.doesNotMatch(adapterOptions, /securedInvoke|executor/) | ||
| assert.match(adapterClass, /this\.#shell = options\.shell/) | ||
| assert.match(adapterClass, /this\.#securedInvoke = createSecuredHullInvoke\(options\)/) | ||
| assert.doesNotMatch(adapterClass, /this\.options/) | ||
| assert.doesNotMatch(adapterClass, /\.invoke\s*\(/) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
suggestion: fail loudly when the slice anchors are missing.
adapter.indexOf('export class HullShellPortAdapter') returns -1 if the class is renamed. slice(-1) then yields the last character, and adapterOptions becomes an empty string. The two assert.doesNotMatch checks at lines 144 and 147 then pass without inspecting any adapter code, so the security guard becomes tautological after a rename. Assert that both anchors exist before slicing.
🧪 Proposed fix
+ const classIndex = adapter.indexOf('export class HullShellPortAdapter')
+ const optionsIndex = adapter.indexOf('export interface HullPortAdapterOptions')
+ assert.ok(classIndex >= 0, 'HullShellPortAdapter anchor missing')
+ assert.ok(optionsIndex >= 0 && optionsIndex < classIndex, 'HullPortAdapterOptions anchor missing')
- const adapterClass = adapter.slice(
- adapter.indexOf('export class HullShellPortAdapter'),
- )
- const adapterOptions = adapter.slice(
- adapter.indexOf('export interface HullPortAdapterOptions'),
- adapter.indexOf('export class HullShellPortAdapter'),
- )
+ const adapterClass = adapter.slice(classIndex)
+ const adapterOptions = adapter.slice(optionsIndex, classIndex)As per path instructions: "state the falsification: delete the guarded logic, the named check goes RED."
📝 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 adapterClass = adapter.slice( | |
| adapter.indexOf('export class HullShellPortAdapter'), | |
| ) | |
| const adapterOptions = adapter.slice( | |
| adapter.indexOf('export interface HullPortAdapterOptions'), | |
| adapter.indexOf('export class HullShellPortAdapter'), | |
| ) | |
| assert.doesNotMatch(adapterOptions, /securedInvoke|executor/) | |
| assert.match(adapterClass, /this\.#shell = options\.shell/) | |
| assert.match(adapterClass, /this\.#securedInvoke = createSecuredHullInvoke\(options\)/) | |
| assert.doesNotMatch(adapterClass, /this\.options/) | |
| assert.doesNotMatch(adapterClass, /\.invoke\s*\(/) | |
| const classIndex = adapter.indexOf('export class HullShellPortAdapter') | |
| const optionsIndex = adapter.indexOf('export interface HullPortAdapterOptions') | |
| assert.ok(classIndex >= 0, 'HullShellPortAdapter anchor missing') | |
| assert.ok(optionsIndex >= 0 && optionsIndex < classIndex, 'HullPortAdapterOptions anchor missing') | |
| const adapterClass = adapter.slice(classIndex) | |
| const adapterOptions = adapter.slice(optionsIndex, classIndex) | |
| assert.doesNotMatch(adapterOptions, /securedInvoke|executor/) | |
| assert.match(adapterClass, /this\.#shell = options\.shell/) | |
| assert.match(adapterClass, /this\.#securedInvoke = createSecuredHullInvoke\(options\)/) | |
| assert.doesNotMatch(adapterClass, /this\.options/) | |
| assert.doesNotMatch(adapterClass, /\.invoke\s*\(/) |
🤖 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 `@tooling/carrier-contract-codegen/tests/generate.test.mjs` around lines 137 -
148, Validate that both export anchors used by the adapter slices in the test
exist before calling slice, and fail the test when either indexOf lookup returns
-1. Keep the existing adapterOptions and adapterClass assertions unchanged so
deleting the guarded logic still makes the named security checks fail.
Source: Path instructions
…oisting The canonical-schema validation gate imports ajv, and it was never declared as a dependency of this package. It resolved locally through the workspace root's hoisted copy, so every local run passed. CI installs this package standalone with the workspace ignored and the lockfile frozen, where no hoisted copy exists, and the suite died on a module it was never told it needed. Declared as a devDependency with the standalone lockfile updated to match. Only this package's manifest and lockfile change; the root lockfile is untouched. Verified by reproducing what CI does rather than what is convenient: a standalone install with the workspace ignored and the lockfile frozen, then the suite. The gate passes and the package's 275 tests pass under that install. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Declaring ajv in the contracts package updated that package's own standalone lockfile but left the root one stale, and every job that installs the workspace with a frozen lockfile refused to start. The standalone install and the workspace install are two separate consumers of the same manifest, and both have to be told. Only the importer entry is committed. Regenerating the root lockfile also wanted to add seventy libc annotations to unrelated binary packages, which is churn from a local pnpm version rather than anything this change needs, and sweeping it in is a trap this repository has already been caught by more than once today. Those lines are excluded and the root frozen install is verified to succeed without them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@packages/contracts/protocol/fixtures/manifest.json`:
- Around line 15-18: Update the negative-fixture tests to validate each
fixture’s actual schema keyword or error path instead of only checking that
constraint is non-empty, mapping hull-invoke-request-unknown-property.json’s
unknown-property value to additionalProperties. Apply equivalent assertions in
the C# and Rust fixture suites, and remove or correct skip gates so all required
protocol validation suites run from a clean checkout.
In `@packages/contracts/tests/ProtocolFixtureTests.cs`:
- Around line 14-26: Require the `NegativeFixtures` property in the C#
`FixtureManifest` instead of making it nullable, and remove the `?? []` fallback
in the `Cases` data construction. Ensure `ReadManifest` fails during test
discovery when `negativeFixtures` is absent, while still concatenating the
required collection with `manifest.Fixtures`.
🪄 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: a90a9f9c-2e40-482a-a4ef-0bd2140af9ff
⛔ Files ignored due to path filters (2)
packages/contracts/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/pnpm-lock.yamlpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/pnpm-lock.yaml
📒 Files selected for processing (10)
packages/contracts/package.jsonpackages/contracts/protocol/fixtures/capability-result-below-minimum.jsonpackages/contracts/protocol/fixtures/hull-invoke-request-unknown-property.jsonpackages/contracts/protocol/fixtures/manifest.jsonpackages/contracts/protocol/fixtures/principal-invalid-enum.jsonpackages/contracts/protocol/fixtures/principal-missing-required.jsonpackages/contracts/rust/src/lib.rspackages/contracts/src/__tests__/protocol-fixtures.test.tspackages/contracts/tests/ProtocolFixtureTests.csscripts/ts-suites.mjs
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/contracts/src/tests/protocol-fixtures.test.ts
- packages/contracts/package.json
- scripts/ts-suites.mjs
…ing caller objects An independent review found the security layer still bypassable after the previous attempt closed it. Branding the policy type achieved nothing, because the minting factory branded whatever function it was handed: a caller could pass an allow-everything policy and have it stamped as official, then run a confirmation-required command with no confirmation and, with no decision sink configured, no audit record. A brand proves a value came through a wrapper. It cannot prove the value is trustworthy. The ratified diagnosis was that the wrong KIND of thing crossed the boundary. Policy is now a STORE and an EVALUATOR. The store owns the authority data, is immutable, is read directly from the canonical source, and fails closed to confirmation-required when that source is missing or unreadable. The evaluator is built from the store and is a deterministic function of command to decision. The security layer accepts no policy from anyone; the parameter is gone. This restores direct reading of the authority source, which had been deleted in favour of the toolkit handing policy in. That deletion is what opened the hole. Two more instances of the same defect are closed the same way. The confirmation-token check is no longer a caller-supplied function: the broker holds its state privately, so a caller may present a token but cannot replace the check with one that returns true. And identity is host-stamped, with the caller-supplied principal option removed, so a caller can no longer act as another identity. The Node bridge now ignores a principal found in its payload; the brands are types and were never enforcing anything at that boundary. The architecture guard gains the blind spots the review named: bracket access, a shadowed local named like the chokepoint, and an allowlist that matched by path suffix. Verified the review's exact reproduction no longer compiles, and that the identity and token options no longer exist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…share The schema asked for any non-negative integer, which no set of three languages can honour identically: JavaScript loses integer precision above 9007199254740991 while C# and Rust use 64-bit integers, so a value between those bounds was accepted by one language and mangled or rejected by another. All nine integer fields now carry that upper bound and all three generated validators enforce it, so the contract means one thing everywhere. The same precision loss silently corrupts large numbers inside extensions bags, which carry arbitrary JSON and cannot be bounded by the schema. The README promised extension members round-trip unchanged, which was false; it now documents the limit precisely rather than leaving a promise the code does not keep. Two generator fail-opens close alongside. A required entry naming a property that was never declared was silently dropped, so all three projections accepted a payload the canonical schema rejects. And an enum nested inside an array of arrays was neither supported nor rejected by the capability matrix: generation SUCCEEDED and wrote projections referencing a type nobody emitted. That middle state was the real defect, so the position is now modelled and fails closed; nothing in the schema needs nesting. Closed models keep rejecting a key whose casing does not match a declared property. The two intentionally open models stop doing so: C# was rejecting a case variant as an incorrectly-cased known property while TypeScript accepted it and Rust preserved it, and a model declared open should not refuse arbitrary keys. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three findings from an independent review, all of the same kind: a gate that runs, goes green, and cannot fail for the reason it exists. The canonical schema gate iterated the positive fixtures only. The negative ones were never validated against the schema at all, so any of them could have been replaced with arbitrary JSON and the gate would have stayed green. They are now validated too, and a negative fixture the schema ACCEPTS turns the gate red. The negative cases asserted that parsing threw, not that the constraint they name was the one that failed. Each case already recorded its constraint, so the data was there and only the assertion ignored it. A fixture that violated a different rule passed, and a case pointed at a model that does not exist passed as well, because unknown-model is itself an error the assertion accepted. All three languages now check the rejection matches the declared constraint, and an unknown model is a hard failure rather than something a reject-case can absorb. Nineteen of thirty-six definitions were unreachable from any case. Coverage now spans every definition, adding the classes the review named: array-item enums with a real value rather than an empty list, integer bounds, exact wire-name rejection, a non-null protocol error with its retry field, typed map values, and round trips of the two open models. The durable part is a coverage assertion that enumerates the schema's definitions and fails naming any that has no case, so this gap cannot silently reopen. Verified by mutation rather than by report: a negative fixture rewritten as valid, one violating the wrong constraint, one pointing at a non-existent model, and the removal of a model's only case each turn the suites red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Agreed on scope, and the two specifics check outConcur with keeping this a governed migration rather than an endpoint rollout. Both cited specifics The DTO mirrors are real, and it is a pattern rather than an instance. Status against the six requests1. DTO mirrors and boundary casts — not started. Two confirmed sites above; a sweep is needed 2. Principal ownership — in flight. The ratified fix removes credential construction from the 3. Generator regression tests — largely done, two open. 22 codegen tests now cover the verified 4. Machine-readable boundary inventory — not done. 5. Architecture gate — partial, and weaker than it appears. The structural guard covers the raw 6. Mandatory independent security review — agreed, and non-negotiable on the evidence. Three RecommendationFinish the trust-boundary fix and the two live behavioural regressions on this branch. Land the Nothing is armed and nothing will be until the open High findings are closed and an independent |
The enforcement point was insecure because authority-bearing values were constructible from outside its trust boundary. The package re-exported its internals wholesale, so the subject-identity mint and the human-authorization-evidence issuer were both public API: a caller could mint an arbitrary subject and have the audit sink record it, or instantiate the evidence issuer, propose a token for a confirmation-required command, confirm it themselves, and reach the real executor with no human involved. This was an encapsulation failure, not a missing-validation bug. Nominal typing could not help, because a brand records only that a value passed through a designated constructor, never that the constructor validated anything or that the caller was entitled to invoke it. An unvalidating factory reachable by an untrusted caller is indistinguishable from a cast. Three prior rounds each closed the one constructor a review named and left the same pattern in a sibling, because the defect was treated as missing checks at call sites. Credential construction is removed from the public surface. The enforcement point exposes only operations that consume already-issued credentials, and issuance is reachable solely from composition roots that can attest provenance. The enforcement point accepts registered credentials as inputs and never issues them. Consumers were migrated onto composed entrypoints that take no credentials rather than being handed the constructors. The guarantee is structural rather than validation-dependent, which is the acceptance condition that distinguishes this from the previous three attempts: a test asserts a shape-only subject is still rejected WITH THE CALL-SITE VALIDATION REMOVED. Verified from a real consumer package that all three constructors are now unreachable, and that the built package exports none of them at runtime. Also restores fail-closed on an unattributed request at the native bridge. Removing payload-supplied identity was correct, but it had been replaced with an unconditional host stamp, so an anonymous request began succeeding with host authority. Payload identity stays ignored and an unattributed request is refused; both hold together. The bridge no longer falls back to a synthetic identity when the operating-system user cannot be determined, which previously produced a valid-looking subject instead of a refusal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…enforces The compatibility section called an added optional field protocol-minor when old consumers ignore it safely. With thirty-five closed models that condition is never satisfiable, so the rule described a contract the code could not honour and every additive change was in fact breaking. States the rule the extensions mechanism implements: closed by default, additive metadata lands in a predeclared bag on allowlisted carriers and is minor, a new top-level property on a closed model is major. Records that identical strictness across the three bindings is a requirement rather than a preference, since a laxer projection accepts payloads its siblings reject and that is the divergence a shared schema exists to prevent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (10)
apps/hull/src/membrane/authority-registry.ts (2)
65-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuenit: type the store entries so the two casts disappear.
loadPolicyStorealready proves thatauthorityis'AP' | 'CP'andsummaryis astring.PolicyStorestill types both asunknown, so Lines 70-71 needascasts. StoreAuthorityDecisionvalues directly and the evaluator becomes cast-free.♻️ Proposed refactor
export interface PolicyStore { - readonly commands: Readonly<Record<string, Readonly<AuthorityRegistryEntry>>> + readonly commands: Readonly<Record<string, AuthorityDecision>> readonly readable: boolean } @@ const entry = store.commands[command] - if (entry?.authority === 'AP' || entry?.authority === 'CP') { - return { - authority: entry.authority as MembraneAuthorityClass, - summary: entry.summary as string, - } - } + if (entry !== undefined) return entry
MembraneAuthorityClassthen becomes an unused import.🤖 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 65 - 79, Update the PolicyStore entry type used by loadPolicyStore so authority is typed as 'AP' | 'CP' and summary as string, matching AuthorityDecision. Then simplify createPolicyEvaluator to return entry.authority and entry.summary directly, and remove the now-unused MembraneAuthorityClass import.
36-62: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winissue [blocking]: fail closed when the registry file is invalid, not missing.
readFileSync(..., 'utf8')throws on missing files, but the same catch also swallowsJSON.parseandTypeErrorresults from malformed registry entries. A missing source may fall back to generated output, and an invalid source falls back too. Separate the two cases: only fall back on file-not-found/unavailable, otherwise log and returnEMPTY_STORE.🤖 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 36 - 62, Update loadPolicyStore so only missing or unavailable registry files continue to the next source; JSON parse failures and invalid authority entries must log the error and immediately return EMPTY_STORE. Separate file-reading errors from parsing and validation around readFileSync, JSON.parse, and the entry checks, preserving the existing generated-copy fallback only for file-not-found/unavailable cases.Source: Path instructions
apps/hull/src/protocol/hull-port-adapter.type-test.ts (1)
29-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winthought: one module-resolution failure would silence all five negative import checks.
Each
@ts-expect-errorabsorbs whatever error the following import produces. If../index.jsever fails to resolve, TypeScript reports TS2307 on every line, all five directives consume it, and the gate reports success while proving nothing.Add one import from
../index.jswithout a directive, using a symbol that must stay exported. If the module stops resolving, that line fails and the gate reports the real cause.♻️ Proposed refactor
// These are host-only issuers. The public Hull entry point intentionally has no type-only escape // hatch for importing them either. +// A resolution anchor: if `../index.js` stops resolving, this line fails instead of the +// `@ts-expect-error` directives below silently absorbing TS2307. +import type { HullShell as PublicHullShell } from '../index.js' // `@ts-expect-error` host principal minting is not part of the public Hull API. import type { mintMembranePrincipal } from '../index.js'Add
void (undefined as unknown as PublicHullShell)alongside the existingvoidstatements. Substitute a symbol that the public barrel is guaranteed to export.🤖 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 29 - 38, Add an undirected type import from ../index.js for a symbol guaranteed to remain part of the public Hull API, and reference it in the existing type-test statements (for example via a void expression). Keep the five `@ts-expect-error` imports unchanged so module resolution is validated independently of their expected failures.packages/contracts/rust/src/lib.rs (2)
81-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: derive the known-model set from
parse_modelinstead of a second hand-maintained list.
is_known_modelrepeats all 37 model names already listed inparse_model. The two lists agree today. They can diverge when a model is added to only one of them. A single source removes that risk.One option is to reuse
parse_modelas the oracle, because it already returns a distinct error for an unknown model.♻️ Proposed refactor
fn parse_model(model: &str, source: &str) -> Result<Value, String> { match model { @@ - _ => Err(format!("unknown protocol fixture model {model}")), + _ => Err(format!("{UNKNOWN_MODEL_PREFIX}{model}")), } } + + const UNKNOWN_MODEL_PREFIX: &str = "unknown protocol fixture model "; fn is_known_model(model: &str) -> bool { - matches!( - model, - "EmptyRequest" - | "EmptyResponse" - // ... 35 more names ... - ) + !matches!( + parse_model(model, ""), + Err(ref error) if error.starts_with(UNKNOWN_MODEL_PREFIX) + ) }
parse_model(model, "")fails on the empty source for every known model, so the check stays cheap and never reports a false positive.🤖 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/contracts/rust/src/lib.rs` around lines 81 - 122, Replace the hand-maintained model-name list in is_known_model with a check against parse_model(model, ""). Treat the model as known when parse_model returns the expected empty-source error for a recognized model, and return false for unknown-model errors, preserving the existing boolean interface.
129-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winthought: the exemption tuples hard-code three fixtures into the assertion.
When the diagnostic does not name the offending property, the assertion accepts three specific
(model, path, constraint)tuples. That couples the shared assertion to individual fixtures. A new fixture whose diagnostic omits the property name requires another arm here.Consider moving the exemption into the manifest as a per-case flag, for example
diagnosticNamesPath: false. The assertion then stays generic, and the C# and TypeScript suites can read the same flag.Not blocking for this PR.
🤖 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/contracts/rust/src/lib.rs` around lines 129 - 143, Replace the hard-coded model/path/constraint exceptions in the assertion around the shared diagnostic validation with a per-case manifest flag such as diagnosticNamesPath. Skip the path-name requirement only when that case flag is explicitly false, keeping the default behavior requiring diagnostics to identify the offending path. Ensure the manifest and C# and TypeScript consumers read the same flag.apps/hull/src/membrane/public-api.test.ts (1)
68-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: the legacy-broker branch cannot execute, because the first test guarantees the export is absent.
The test at Lines 34-47 asserts that
TrustedConfirmationBrokeris not a member ofpublicHull. The guard at Line 72 therefore evaluates tofalseon every run, and Lines 73-90 never execute. The block reads as coverage of the self-mint path but contributes none.Remove the branch. Test 1 already pins the export boundary, and the assertions at Lines 93-112 already cover the fake-broker rejection.
As per path instructions, "real/offline paths must be verified not skip-gated (A8)".
🤖 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 68 - 91, Remove the conditional legacyBroker branch and its self-mint execution from the test, since the earlier publicHull export assertion guarantees it cannot run. Keep the existing export-boundary assertion and the fake-broker rejection assertions around secureInvoke unchanged so the real/offline path is not skip-gated.Source: Path instructions
apps/hull/src/membrane/pep.test.ts (1)
104-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffsuggestion (non-blocking): the compatibility shim weakens the assertions of the tests that still route through it.
This local
secureInvokeadapts the removed 5-argument signature. On lines 114-120 it does more than adapt: when an alias exists, it replaces the caller'sconfirmationwith a broker-minted evidence object taken fromstore.evidences.A legacy test that passes a hand-built confirmation therefore no longer exercises the confirmation it declared. The shim swaps in a valid one before
secureInvokeActualsees it. Those tests now assert the shim's substitution rather than the PEP's acceptance criteria.The new tests on lines 193-243 avoid this by calling
secureInvokeActualdirectly. That is the right pattern. Migrating the remaining call sites to the direct signature would let you delete the shim, thebrokerForConsumerWeakMap on lines 95-102, the_legacyPolicyparameter on line 107, and thetestPdpplaceholder on line 64.This is scaffolding cost, not a correctness defect, so it can follow in a separate change.
🤖 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 104 - 129, Replace remaining test call sites that use the local secureInvoke compatibility shim with direct secureInvokeActual calls using its current signature. Then remove secureInvoke, the brokerForConsumer WeakMap, its _legacyPolicy parameter, and the unused testPdp placeholder, ensuring tests continue passing their hand-built confirmations directly to secureInvokeActual.apps/hull/src/membrane/credential-issuer.ts (1)
15-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuethought (non-blocking): proposed-but-never-consumed tokens stay in the broker map.
proposeinserts into the per-brokerMapon line 32. Onlyconsumeremoves an entry. A token that is proposed and then abandoned is retained for the lifetime of the broker.This is safe in the current wiring.
runDemoCpOperationinapps/hull/src/membrane/composed.tsconstructs a freshTrustedConfirmationBrokerper operation, andbrokerStatesis aWeakMap, so the wholeMapis collected when the broker is dropped.If a future change makes a broker long-lived and shared, add an expiry to the outstanding entries. Recording this now so the assumption is explicit rather than discovered later.
🤖 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-issuer.ts` around lines 15 - 32, Document the lifetime assumption near TrustedConfirmationBroker: proposed tokens remain in its per-broker Map until consumed, and cleanup currently relies on each broker being short-lived and held through brokerStates, a WeakMap. If the broker becomes long-lived or shared, add expiry and removal for abandoned OutstandingConfirmation entries; do not change the current behavior otherwise.apps/hull/src/membrane/credential-boundary.ts (1)
31-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion (non-blocking): narrow
registerTrustedConfirmationBrokerto the broker type.The parameter is typed
object. The only caller is theTrustedConfirmationBrokerconstructor inapps/hull/src/membrane/credential-issuer.tsat line 26. The runtime contract enforced byconsumeTrustedConfirmationon line 44 is narrower still: the value must expose a callableconsume.A structural parameter type moves that contract to compile time. It also stops a future edit from registering an arbitrary object as a trusted broker without a type error. The runtime guard on lines 40-47 stays as the load-bearing check.
Use a structural type rather than importing the class, so the boundary module keeps its current dependency direction and does not create an import cycle with
credential-issuer.ts.♻️ Proposed narrowing
-export function registerTrustedConfirmationBroker(broker: object): void { +interface ConfirmationConsumer { + consume(token: string, command: string): boolean +} + +export function registerTrustedConfirmationBroker(broker: ConfirmationConsumer): void { brokers.add(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/credential-boundary.ts` around lines 31 - 33, Update registerTrustedConfirmationBroker to accept a structural broker type exposing a callable consume method, matching the contract used by consumeTrustedConfirmation. Define the type locally rather than importing TrustedConfirmationBroker, and leave the existing runtime guard unchanged.packages/contracts/src/__tests__/protocol-schema.test.ts (1)
35-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: assert AJV
keywordandinstancePathagainst the declared constraint/path.Lines 38-39 only verify that
constraintandpathare non-empty, and line 40 only checks invalidity. A negative fixture can fail for an unrelated reason while still passing this loop. Add anajv.getSchema(...)assertion that matcheskeyword/params.missingPropertyforrequiredandinstancePathfor property-path failures.🤖 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/contracts/src/__tests__/protocol-schema.test.ts` around lines 35 - 41, The negative-fixture loop should verify the reported AJV validation error, not only that validation fails. Capture the validation result around the schema validator and assert its error’s keyword and location match each fixture’s declared constraint/path, using params.missingProperty for required violations and instancePath for property-path failures; retain the existing schema and fixture metadata assertions.
🤖 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-96: Remove the stale ProposalBroker docblock above
hostPrincipal() and replace it with documentation describing hostPrincipal()’s
actual role: resolving and returning the current local OS user identity, or null
when unavailable or empty. Keep the broker-cycle documentation with
runDemoCpOperation in composed.ts.
In `@apps/hull/src/membrane/composed.ts`:
- Around line 22-28: Replace the optional globalThis.crypto randomUUID
expression in the correlationId construction with the CSPRNG randomUUID imported
from node:crypto, removing the Date.now fallback. Keep idempotencyKey derived
from the resulting correlationId so both tokens remain cryptographically random.
In `@apps/hull/src/membrane/host-principal.ts`:
- Around line 38-41: Update apps/hull/src/membrane/host-principal.ts lines 38-41
in currentHostPrincipal() to catch userInfo() failures and return the defined
failure value. In apps/carrier/src-tauri/hull-invoker.mjs lines 278-290, remove
the local hostPrincipal() implementation, use the resolver exported from
`@shipyard/hull`, and preserve membrane.anonymous_principal as the fail-closed
envelope.
- Around line 38-41: Update currentHostPrincipal to wrap the userInfo() lookup
and username processing in try/catch, matching the existing hostPrincipal
helper’s behavior. Convert any userInfo failure, including missing passwd
entries, into the function’s uniform unavailable-host-username error instead of
allowing the SystemError to escape.
In `@apps/hull/src/membrane/node-bridge-principal.test.ts`:
- Around line 30-41: Update the test’s spoofed principal in the bridge
invocation to use an identity guaranteed not to match the host, rather than
“os:root”. In the assertions after parsing the result, explicitly verify
proposedBy.id and confirmedBy.id are not equal to the spoof value, while
retaining the existing host identity checks. Keep the change scoped to the test
case exercising principal handling.
In `@apps/hull/src/membrane/pep.ts`:
- Around line 56-65: Update the module-load policy initialization around
loadPolicyStore and policyEvaluator to record a single observable warning when
the loaded store has readable false. Include the store’s readable, source,
destination, and URL values in that log, while preserving
createPolicyEvaluator’s fail-closed behavior and avoiding repeated logging
during command evaluation.
In `@apps/hull/src/membrane/signed-principal.ts`:
- Around line 394-395: In the post-verification registration flow around
registerMembranePrincipal, create a fresh copy of envelope.principal, freeze it,
register that frozen copy, and return the same immutable object. Preserve the
existing verification and nonce ordering, and ensure the caller-supplied
envelope.principal is not registered or returned by reference.
In `@apps/hull/src/protocol/hull-port-adapter.ts`:
- Around line 73-84: Move the currentHostPrincipal() call from
createSecuredHullInvoke() into the returned invoke function so factory
construction remains non-throwing. Catch identity-resolution failures there and
return the standard membrane.anonymous_principal failure envelope, while
preserving secureInvoke behavior for successfully resolved principals.
In `@apps/hull/src/protocol/hull-port-adapter.type-test.ts`:
- Line 54: Remove the unused _command parameter from the policyDecisionPoint
function in the type-test fixture, keeping its returned authority and summary
values unchanged and preserving the existing `@ts-expect-error` assertion.
In `@packages/carrier-sdk/src/command-authority-parity.test.ts`:
- Around line 67-70: Replace the substring-only assertions in the
command-authority parity test with a behavior test that loads the `.mjs` bridge,
invokes its public entry point, and verifies the call reaches `secureInvoke`.
Also assert that the bridge does not read or classify `command-authority.json`
directly, and ensure the CI suite executes this test and obtains the required
independent security review before merge.
In `@packages/carrier-sdk/src/cp-notification-loop.test.ts`:
- Around line 89-117: Replace the local legacy-token translation in secureInvoke
in packages/carrier-sdk/src/cp-notification-loop.test.ts (lines 89-117) and the
duplicate adapter in packages/carrier-sdk/src/cp-rejection.test.ts (lines
72-100) so tests use actual Hull-issued confirmation evidence from a completed
confirmation flow, or accept non-forgeable completed-confirmation evidence
through the adapter contract; do not mint TrustedConfirmationBroker evidence
from caller-provided confirmedBy after merely consuming a proposal token. Obtain
independent security review and ensure the CI suite exercises these CP changes.
In `@packages/carrier-sdk/src/principal.ts`:
- Around line 40-51: Add isolated CI-covered tests for currentOsPrincipal() in
principal.test.ts that mock os.userInfo() to throw and to return a blank or
whitespace username, asserting both fail with the expected errors. Ensure the
tests do not depend on the real OS principal and are included in an unskipped CI
test job.
In `@packages/carrier-sdk/src/sdk.ts`:
- Around line 155-156: Update the JSDoc for the invocation path around
invokeInProcess to remove the claim that this.actingAs is passed through.
Document that invokeInProcess creates the host-owned secured invoker and issues
the invocation principal on the host side, while caller options cannot replace
the stored identity.
In `@packages/contracts/src/__tests__/protocol-fixtures.test.ts`:
- Around line 97-102: Move the parser-accepted sentinel throw in the
negative-fixture test outside the try/catch that calls
parseCarrierProtocolModel, so only parser errors reach assertDeclaredConstraint;
preserve the existing constraint validation for rejected fixtures and ensure
wrongly accepted fixtures report the direct acceptance failure.
In `@packages/contracts/tests/ProtocolFixtureTests.cs`:
- Around line 13-23: Make NegativeFixtures required in the FixtureManifest
record and remove the null-coalescing fallback from the manifest fixture
enumeration. Ensure ReadManifest deserialization rejects manifests missing
negativeFixtures, matching the required field contract in Rust.
- Around line 68-77: Update the rejection-capture logic in the test around
JsonSerializer.Deserialize so only the deserialization call is inside the
catchable try block; perform the XunitException for an accepted fixture after
that block, when no exception was captured. Preserve the existing rejection
assertions and ensure the CI test path exercises the parser-acceptance case.
---
Nitpick comments:
In `@apps/hull/src/membrane/authority-registry.ts`:
- Around line 65-79: Update the PolicyStore entry type used by loadPolicyStore
so authority is typed as 'AP' | 'CP' and summary as string, matching
AuthorityDecision. Then simplify createPolicyEvaluator to return entry.authority
and entry.summary directly, and remove the now-unused MembraneAuthorityClass
import.
- Around line 36-62: Update loadPolicyStore so only missing or unavailable
registry files continue to the next source; JSON parse failures and invalid
authority entries must log the error and immediately return EMPTY_STORE.
Separate file-reading errors from parsing and validation around readFileSync,
JSON.parse, and the entry checks, preserving the existing generated-copy
fallback only for file-not-found/unavailable cases.
In `@apps/hull/src/membrane/credential-boundary.ts`:
- Around line 31-33: Update registerTrustedConfirmationBroker to accept a
structural broker type exposing a callable consume method, matching the contract
used by consumeTrustedConfirmation. Define the type locally rather than
importing TrustedConfirmationBroker, and leave the existing runtime guard
unchanged.
In `@apps/hull/src/membrane/credential-issuer.ts`:
- Around line 15-32: Document the lifetime assumption near
TrustedConfirmationBroker: proposed tokens remain in its per-broker Map until
consumed, and cleanup currently relies on each broker being short-lived and held
through brokerStates, a WeakMap. If the broker becomes long-lived or shared, add
expiry and removal for abandoned OutstandingConfirmation entries; do not change
the current behavior otherwise.
In `@apps/hull/src/membrane/pep.test.ts`:
- Around line 104-129: Replace remaining test call sites that use the local
secureInvoke compatibility shim with direct secureInvokeActual calls using its
current signature. Then remove secureInvoke, the brokerForConsumer WeakMap, its
_legacyPolicy parameter, and the unused testPdp placeholder, ensuring tests
continue passing their hand-built confirmations directly to secureInvokeActual.
In `@apps/hull/src/membrane/public-api.test.ts`:
- Around line 68-91: Remove the conditional legacyBroker branch and its
self-mint execution from the test, since the earlier publicHull export assertion
guarantees it cannot run. Keep the existing export-boundary assertion and the
fake-broker rejection assertions around secureInvoke unchanged so the
real/offline path is not skip-gated.
In `@apps/hull/src/protocol/hull-port-adapter.type-test.ts`:
- Around line 29-38: Add an undirected type import from ../index.js for a symbol
guaranteed to remain part of the public Hull API, and reference it in the
existing type-test statements (for example via a void expression). Keep the five
`@ts-expect-error` imports unchanged so module resolution is validated
independently of their expected failures.
In `@packages/contracts/rust/src/lib.rs`:
- Around line 81-122: Replace the hand-maintained model-name list in
is_known_model with a check against parse_model(model, ""). Treat the model as
known when parse_model returns the expected empty-source error for a recognized
model, and return false for unknown-model errors, preserving the existing
boolean interface.
- Around line 129-143: Replace the hard-coded model/path/constraint exceptions
in the assertion around the shared diagnostic validation with a per-case
manifest flag such as diagnosticNamesPath. Skip the path-name requirement only
when that case flag is explicitly false, keeping the default behavior requiring
diagnostics to identify the offending path. Ensure the manifest and C# and
TypeScript consumers read the same flag.
In `@packages/contracts/src/__tests__/protocol-schema.test.ts`:
- Around line 35-41: The negative-fixture loop should verify the reported AJV
validation error, not only that validation fails. Capture the validation result
around the schema validator and assert its error’s keyword and location match
each fixture’s declared constraint/path, using params.missingProperty for
required violations and instancePath for property-path failures; retain the
existing schema and fixture metadata assertions.
🪄 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: bb3cacc1-59fa-41e2-ba2e-e9d8578b2185
⛔ Files ignored due to path filters (2)
packages/contracts/Generated/CarrierProtocol.g.csis excluded by!**/generated/**packages/contracts/src/generated/carrier-protocol.generated.tsis excluded by!**/*.generated.*,!**/generated/**
📒 Files selected for processing (66)
apps/carrier/src-tauri/hull-invoker.mjsapps/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/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/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.tspackages/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.mdpackages/contracts/protocol/fixtures/address-request.jsonpackages/contracts/protocol/fixtures/address-result.jsonpackages/contracts/protocol/fixtures/announce-request.jsonpackages/contracts/protocol/fixtures/announce-result.jsonpackages/contracts/protocol/fixtures/artifact.jsonpackages/contracts/protocol/fixtures/backup-status.jsonpackages/contracts/protocol/fixtures/capability-result.jsonpackages/contracts/protocol/fixtures/compose-request.jsonpackages/contracts/protocol/fixtures/compose-result.jsonpackages/contracts/protocol/fixtures/cp-demo-request.jsonpackages/contracts/protocol/fixtures/device-capability-profile-above-safe-integer.jsonpackages/contracts/protocol/fixtures/device-capability-profile.jsonpackages/contracts/protocol/fixtures/empty-request.jsonpackages/contracts/protocol/fixtures/empty-response.jsonpackages/contracts/protocol/fixtures/enrollment-status.jsonpackages/contracts/protocol/fixtures/hull-host-invoke-request.jsonpackages/contracts/protocol/fixtures/manifest.jsonpackages/contracts/protocol/fixtures/negotiate-request.jsonpackages/contracts/protocol/fixtures/negotiate-result.jsonpackages/contracts/protocol/fixtures/observe-request.jsonpackages/contracts/protocol/fixtures/principal-case-variant.jsonpackages/contracts/protocol/fixtures/protocol-error.jsonpackages/contracts/protocol/fixtures/provider-descriptor.jsonpackages/contracts/protocol/fixtures/resolution-result.jsonpackages/contracts/protocol/fixtures/resolve-request.jsonpackages/contracts/protocol/fixtures/runtime-capability.jsonpackages/contracts/protocol/fixtures/runtime-health.jsonpackages/contracts/protocol/fixtures/secure-request.jsonpackages/contracts/protocol/fixtures/secure-result.jsonpackages/contracts/protocol/fixtures/sync-cadence.jsonpackages/contracts/protocol/fixtures/sync-peer-status.jsonpackages/contracts/protocol/fixtures/usage.jsonpackages/contracts/protocol/schemas/carrier-protocol.schema.jsonpackages/contracts/rust/src/generated.rspackages/contracts/rust/src/lib.rspackages/contracts/src/__tests__/protocol-fixtures.test.tspackages/contracts/src/__tests__/protocol-schema.test.tspackages/contracts/tests/ProtocolFixtureTests.cstooling/carrier-contract-codegen/generate.mjstooling/carrier-contract-codegen/tests/generate.test.mjs
🚧 Files skipped from review as they are similar to previous changes (9)
- apps/hull/src/membrane/credential-mints.test.ts
- packages/contracts/protocol/fixtures/capability-result.json
- apps/hull/src/membrane/no-direct-invoke.arch.test.ts
- packages/contracts/protocol/README.md
- packages/contracts/protocol/schemas/carrier-protocol.schema.json
- tooling/carrier-contract-codegen/tests/generate.test.mjs
- apps/hull/src/protocol/hull-port-adapter.test.ts
- tooling/carrier-contract-codegen/generate.mjs
- packages/contracts/rust/src/generated.rs
| /** | ||
| * 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. | ||
| */ | ||
| class BridgeProposalBroker { | ||
| /** @type {Map<string, { command: string }>} */ | ||
| #outstanding = new Map() | ||
|
|
||
| /** Mint a single-use, command-bound token and record it outstanding. */ | ||
| propose(command) { | ||
| const t = randomUUID() | ||
| this.#outstanding.set(t, { command }) | ||
| return t | ||
| } | ||
|
|
||
| /** | ||
| * Atomically verify + consume a token bound to `command` (the `TokenConsumer` | ||
| * contract). Returns true ONLY if the token exists + is bound to `command`, and | ||
| * CONSUMES it so a replay returns false. | ||
| * | ||
| * @param {string} t | ||
| * @param {string} command | ||
| */ | ||
| consume(t, command) { | ||
| const entry = this.#outstanding.get(t) | ||
| if (entry === undefined || entry.command !== command) return false | ||
| this.#outstanding.delete(t) | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * The carrier PDP — the membrane PEP's authority classifier (ADR 0134 Decision 2, | ||
| * the injectable `PolicyDecisionPoint`). The bridge runs exactly ONE command — | ||
| * `invoke` — which is AP in the ADR 0128 registry (the same classification | ||
| * `@shipyard/carrier-sdk`'s `authorityOf` gives `invoke`). | ||
| * | ||
| * F2 (ADR 0134 P1a — CONVERGE the two PDPs): this no longer hardcodes its own | ||
| * AP-command set. The bridge cannot `import` carrier-sdk (it spawns standalone, no | ||
| * runtime coupling to the SDK build), but it CAN read the SAME canonical | ||
| * `command-authority.json` the SDK's `authorityOf` reads — so there is ONE source of | ||
| * truth. Before P1a the two classifiers were hand-parallel and drifted; the rejection | ||
| * path makes a CP/AP disagreement load-bearing (a command CP in one path / AP in the | ||
| * other now changes whether it is refused). A parity test asserts the two agree. | ||
| * | ||
| * Resolution: try a co-located `./command-authority.json` first (the eventual bundled | ||
| * layout — the bridge + its registry travel together as Tauri resources), then fall | ||
| * back to the carrier-sdk source (the dev/test layout). Unknown commands fail-closed to | ||
| * CP, matching `authorityOf`. A load failure ALSO fails closed (every command → CP) so | ||
| * the bridge never silently widens authority. | ||
| */ | ||
| const CANONICAL_AUTHORITY_PATHS = [ | ||
| new URL('./command-authority.json', import.meta.url), | ||
| new URL('../../../packages/carrier-sdk/src/command-authority.json', import.meta.url), | ||
| ] | ||
|
|
||
| function loadCommandAuthority() { | ||
| for (const url of CANONICAL_AUTHORITY_PATHS) { | ||
| try { | ||
| const file = JSON.parse(readFileSync(url, 'utf8')) | ||
| const commands = file?.commands | ||
| if (commands && typeof commands === 'object') return commands | ||
| } catch { | ||
| // try the next candidate path | ||
| } | ||
| } | ||
| return null // fail-closed: no registry → every command is CP | ||
| } | ||
|
|
||
| const COMMAND_AUTHORITY = loadCommandAuthority() | ||
|
|
||
| function carrierPdp(command) { | ||
| const entry = COMMAND_AUTHORITY?.[command] | ||
| if (entry && entry.authority === 'AP') { | ||
| return { authority: 'AP', summary: entry.summary ?? `${command} (AP) — reversible/read op` } | ||
| } | ||
| if (entry && entry.authority === 'CP') { | ||
| return { authority: 'CP', summary: entry.summary ?? `'${command}' — confirmation-required` } | ||
| function hostPrincipal() { | ||
| let username = '' | ||
| try { | ||
| username = userInfo().username | ||
| } catch { | ||
| return null | ||
| } | ||
| return { authority: 'CP', summary: `'${command}' — fail-closed to confirmation-required` } | ||
| const safeUsername = username.trim() | ||
| if (safeUsername.length === 0) return null | ||
| return Object.freeze({ | ||
| id: `os:${safeUsername}`, | ||
| displayName: safeUsername, | ||
| kind: 'local-os-user', | ||
| }) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
issue: the docblock describes a ProposalBroker that no longer exists, and it now sits on hostPrincipal().
Lines 70-81 describe "A minimal ProposalBroker" and a "propose → confirm → consume cycle IN THIS PROCESS". That code moved to runDemoCpOperation in apps/hull/src/membrane/composed.ts. The comment is now attached to hostPrincipal(), which resolves an OS identity and performs no brokering. A future reader will look for broker logic in this function.
📝 Proposed fix
-/**
- * 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 host OS identity for the attribution pre-check.
+ *
+ * This value is NOT the authenticated identity. Hull resolves that itself inside
+ * `createSecuredHullInvoke`. This helper only answers "is a host identity resolvable
+ * at all", so an unattributed standalone spawn fails closed before Hull is composed.
+ * Returns `null` when the OS user is unavailable.
+ */📝 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.
| /** | |
| * 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. | |
| */ | |
| class BridgeProposalBroker { | |
| /** @type {Map<string, { command: string }>} */ | |
| #outstanding = new Map() | |
| /** Mint a single-use, command-bound token and record it outstanding. */ | |
| propose(command) { | |
| const t = randomUUID() | |
| this.#outstanding.set(t, { command }) | |
| return t | |
| } | |
| /** | |
| * Atomically verify + consume a token bound to `command` (the `TokenConsumer` | |
| * contract). Returns true ONLY if the token exists + is bound to `command`, and | |
| * CONSUMES it so a replay returns false. | |
| * | |
| * @param {string} t | |
| * @param {string} command | |
| */ | |
| consume(t, command) { | |
| const entry = this.#outstanding.get(t) | |
| if (entry === undefined || entry.command !== command) return false | |
| this.#outstanding.delete(t) | |
| return true | |
| } | |
| } | |
| /** | |
| * The carrier PDP — the membrane PEP's authority classifier (ADR 0134 Decision 2, | |
| * the injectable `PolicyDecisionPoint`). The bridge runs exactly ONE command — | |
| * `invoke` — which is AP in the ADR 0128 registry (the same classification | |
| * `@shipyard/carrier-sdk`'s `authorityOf` gives `invoke`). | |
| * | |
| * F2 (ADR 0134 P1a — CONVERGE the two PDPs): this no longer hardcodes its own | |
| * AP-command set. The bridge cannot `import` carrier-sdk (it spawns standalone, no | |
| * runtime coupling to the SDK build), but it CAN read the SAME canonical | |
| * `command-authority.json` the SDK's `authorityOf` reads — so there is ONE source of | |
| * truth. Before P1a the two classifiers were hand-parallel and drifted; the rejection | |
| * path makes a CP/AP disagreement load-bearing (a command CP in one path / AP in the | |
| * other now changes whether it is refused). A parity test asserts the two agree. | |
| * | |
| * Resolution: try a co-located `./command-authority.json` first (the eventual bundled | |
| * layout — the bridge + its registry travel together as Tauri resources), then fall | |
| * back to the carrier-sdk source (the dev/test layout). Unknown commands fail-closed to | |
| * CP, matching `authorityOf`. A load failure ALSO fails closed (every command → CP) so | |
| * the bridge never silently widens authority. | |
| */ | |
| const CANONICAL_AUTHORITY_PATHS = [ | |
| new URL('./command-authority.json', import.meta.url), | |
| new URL('../../../packages/carrier-sdk/src/command-authority.json', import.meta.url), | |
| ] | |
| function loadCommandAuthority() { | |
| for (const url of CANONICAL_AUTHORITY_PATHS) { | |
| try { | |
| const file = JSON.parse(readFileSync(url, 'utf8')) | |
| const commands = file?.commands | |
| if (commands && typeof commands === 'object') return commands | |
| } catch { | |
| // try the next candidate path | |
| } | |
| } | |
| return null // fail-closed: no registry → every command is CP | |
| } | |
| const COMMAND_AUTHORITY = loadCommandAuthority() | |
| function carrierPdp(command) { | |
| const entry = COMMAND_AUTHORITY?.[command] | |
| if (entry && entry.authority === 'AP') { | |
| return { authority: 'AP', summary: entry.summary ?? `${command} (AP) — reversible/read op` } | |
| } | |
| if (entry && entry.authority === 'CP') { | |
| return { authority: 'CP', summary: entry.summary ?? `'${command}' — confirmation-required` } | |
| function hostPrincipal() { | |
| let username = '' | |
| try { | |
| username = userInfo().username | |
| } catch { | |
| return null | |
| } | |
| return { authority: 'CP', summary: `'${command}' — fail-closed to confirmation-required` } | |
| const safeUsername = username.trim() | |
| if (safeUsername.length === 0) return null | |
| return Object.freeze({ | |
| id: `os:${safeUsername}`, | |
| displayName: safeUsername, | |
| kind: 'local-os-user', | |
| }) | |
| } | |
| /** | |
| * Resolve the host OS identity for the attribution pre-check. | |
| * | |
| * This value is NOT the authenticated identity. Hull resolves that itself inside | |
| * `createSecuredHullInvoke`. This helper only answers "is a host identity resolvable | |
| * at all", so an unattributed standalone spawn fails closed before Hull is composed. | |
| * Returns `null` when the OS user is unavailable. | |
| */ | |
| function hostPrincipal() { | |
| let username = '' | |
| try { | |
| username = userInfo().username | |
| } catch { | |
| return null | |
| } | |
| const safeUsername = username.trim() | |
| if (safeUsername.length === 0) return null | |
| return Object.freeze({ | |
| id: `os:${safeUsername}`, | |
| displayName: safeUsername, | |
| kind: 'local-os-user', | |
| }) | |
| } |
🤖 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 - 96, Remove the
stale ProposalBroker docblock above hostPrincipal() and replace it with
documentation describing hostPrincipal()’s actual role: resolving and returning
the current local OS user identity, or null when unavailable or empty. Keep the
broker-cycle documentation with runDemoCpOperation in composed.ts.
| 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}`, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
issue (blocking): Date.now() is not a CSPRNG, and the weak value flows into idempotencyKey.
Line 22 falls back to Date.now() when globalThis.crypto?.randomUUID is missing. Line 28 derives idempotencyKey from that same value. Date.now() has millisecond resolution and is fully predictable. Two demo-cp-op runs in the same millisecond produce an identical idempotencyKey, which opens a replay or double-execution window on a CP-classified command.
Hull runs on Node, so node:crypto randomUUID is always available. Import it and delete the fallback.
🔒️ Proposed fix
import type { CapabilityResult, InvokeRequest } from '`@shipyard/contracts`'
+import { randomUUID } from 'node:crypto'- const correlationId = `carrier-demo-cp-op-${globalThis.crypto?.randomUUID?.() ?? Date.now()}`
+ const correlationId = `carrier-demo-cp-op-${randomUUID()}`As per path instructions: "CSPRNG (node:crypto) for correlation/idempotency/confirm tokens, never Math.random" and "CSPRNG for idempotencyKey (replay/double-exec window if it carries to CP ops)".
📝 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 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}`, | |
| import type { CapabilityResult, InvokeRequest } from '`@shipyard/contracts`' | |
| import { randomUUID } from 'node:crypto' | |
| const correlationId = `carrier-demo-cp-op-${randomUUID()}` | |
| const request: InvokeRequest = { | |
| capabilityId: 'tts', | |
| core: { text: note, voice: null, format: 'aiff', timeout: 30_000 } as never, | |
| providerInputs: {}, | |
| attachments: [], | |
| idempotencyKey: `idem-${correlationId}`, |
🤖 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 22 - 28, Replace the
optional globalThis.crypto randomUUID expression in the correlationId
construction with the CSPRNG randomUUID imported from node:crypto, removing the
Date.now fallback. Keep idempotencyKey derived from the resulting correlationId
so both tokens remain cryptographically random.
Source: Path instructions
| export function currentHostPrincipal(): MembranePrincipal { | ||
| const username = userInfo().username.trim() | ||
| if (username.length === 0) throw new Error('host OS username is unavailable') | ||
| return mintMembranePrincipal(localOsUserPrincipal(username)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
issue: two parallel host-identity resolvers with different failure contracts. The repository now resolves the OS host identity in two places. currentHostPrincipal() throws when userInfo() fails. The bridge's local hostPrincipal() returns null. The bridge guard therefore cannot protect the later Hull call, and the uniform failure envelope is lost when userInfo() throws.
apps/hull/src/membrane/host-principal.ts#L38-L41: wrapuserInfo()intry/catchso the resolver has one defined failure mode.apps/carrier/src-tauri/hull-invoker.mjs#L278-L290: delete the localhostPrincipal()copy, use the resolver exported from@shipyard/hull, and keepmembrane.anonymous_principalas the fail-closed envelope.
📍 Affects 2 files
apps/hull/src/membrane/host-principal.ts#L38-L41(this comment)apps/carrier/src-tauri/hull-invoker.mjs#L278-L290
🤖 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/host-principal.ts` around lines 38 - 41, Update
apps/hull/src/membrane/host-principal.ts lines 38-41 in currentHostPrincipal()
to catch userInfo() failures and return the defined failure value. In
apps/carrier/src-tauri/hull-invoker.mjs lines 278-290, remove the local
hostPrincipal() implementation, use the resolver exported from `@shipyard/hull`,
and preserve membrane.anonymous_principal as the fail-closed envelope.
Source: Path instructions
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
issue: userInfo() can throw, and the throw escapes the uniform failure envelope.
Line 39 calls userInfo() without a guard. On hosts where the UID has no passwd entry, such as some containers, userInfo() throws SystemError. createSecuredHullInvoke calls currentHostPrincipal() at construction time. In apps/carrier/src-tauri/hull-invoker.mjs line 301 that call sits outside any try, so the process exits with no envelope on stdout.
The same file's local hostPrincipal() helper already wraps userInfo() in try/catch. Align this function with that behavior.
🛡️ Proposed fix
export function currentHostPrincipal(): MembranePrincipal {
- const username = userInfo().username.trim()
+ let username = ''
+ try {
+ username = userInfo().username.trim()
+ } catch (err) {
+ throw new Error(`host OS username is unavailable: ${err instanceof Error ? err.message : String(err)}`)
+ }
if (username.length === 0) throw new Error('host OS username is unavailable')
return mintMembranePrincipal(localOsUserPrincipal(username))
}📝 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 function currentHostPrincipal(): MembranePrincipal { | |
| const username = userInfo().username.trim() | |
| if (username.length === 0) throw new Error('host OS username is unavailable') | |
| return mintMembranePrincipal(localOsUserPrincipal(username)) | |
| export function currentHostPrincipal(): MembranePrincipal { | |
| let username = '' | |
| try { | |
| username = userInfo().username.trim() | |
| } catch (err) { | |
| throw new Error(`host OS username is unavailable: ${err instanceof Error ? err.message : String(err)}`) | |
| } | |
| if (username.length === 0) throw new Error('host OS username is unavailable') | |
| return mintMembranePrincipal(localOsUserPrincipal(username)) | |
| } |
🤖 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/host-principal.ts` around lines 38 - 41, Update
currentHostPrincipal to wrap the userInfo() lookup and username processing in
try/catch, matching the existing hostPrincipal helper’s behavior. Convert any
userInfo failure, including missing passwd entries, into the function’s uniform
unavailable-host-username error instead of allowing the SystemError to escape.
| principal: { id: 'os:root', displayName: 'root', kind: 'local-os-user' }, | ||
| }), | ||
| encoding: 'utf8', | ||
| }) | ||
| const result = JSON.parse(raw) as { | ||
| status: string | ||
| meta?: { proposedBy?: { id?: string }; confirmedBy?: { id?: string } } | ||
| } | ||
|
|
||
| expect(result.status).toBe('succeeded') | ||
| expect(result.meta?.proposedBy?.id).toBe(hostId()) | ||
| expect(result.meta?.confirmedBy?.id).toBe(hostId()) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
issue: the assertions can pass without testing the boundary when the host user is root.
The test spoofs id: 'os:root' on line 30. It then asserts that proposedBy.id and confirmedBy.id equal hostId() on lines 40-41.
If the test process runs as root, hostId() returns 'os:root'. The spoofed identity and the host identity become the same string. Both assertions then pass whether the bridge ignores the payload principal or trusts it. The test reports success while proving nothing. Containerised CI runners commonly execute as root, so this is reachable rather than theoretical.
Two changes make the oracle sound. First, pick a spoof value that cannot collide with any real host identity. Second, assert the negative explicitly, so the test states the property it is named for.
Note also that hostId() on lines 12-20 reimplements the bridge's own derivation, and line 26 forwards ...process.env to the child. A shared mistake in that derivation is therefore invisible to this test. The explicit negative assertion below does not depend on that shared logic.
🛡️ Proposed fix to make the oracle collision-proof
+const SPOOFED_ID = 'os:spoofed-not-a-real-account-9f3a1c'
+
describe('Node Hull bridge identity boundary', () => {
it('ignores a principal authored in the JSON payload', () => {
const raw = execFileSync(process.execPath, [BRIDGE], {
cwd: join(REPO_ROOT, 'apps', 'carrier'),
env: { ...process.env, HULL_IMAGE_REAL: '0' },
input: JSON.stringify({
op: 'cp-execute',
note: 'bridge identity boundary',
- principal: { id: 'os:root', displayName: 'root', kind: 'local-os-user' },
+ principal: { id: SPOOFED_ID, displayName: 'spoofed', kind: 'local-os-user' },
}),
encoding: 'utf8',
})
const result = JSON.parse(raw) as {
status: string
meta?: { proposedBy?: { id?: string }; confirmedBy?: { id?: string } }
}
expect(result.status).toBe('succeeded')
+ // The payload identity must never surface, independent of who runs the test.
+ expect(result.meta?.proposedBy?.id).not.toBe(SPOOFED_ID)
+ expect(result.meta?.confirmedBy?.id).not.toBe(SPOOFED_ID)
expect(result.meta?.proposedBy?.id).toBe(hostId())
expect(result.meta?.confirmedBy?.id).toBe(hostId())
})
})📝 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.
| principal: { id: 'os:root', displayName: 'root', kind: 'local-os-user' }, | |
| }), | |
| encoding: 'utf8', | |
| }) | |
| const result = JSON.parse(raw) as { | |
| status: string | |
| meta?: { proposedBy?: { id?: string }; confirmedBy?: { id?: string } } | |
| } | |
| expect(result.status).toBe('succeeded') | |
| expect(result.meta?.proposedBy?.id).toBe(hostId()) | |
| expect(result.meta?.confirmedBy?.id).toBe(hostId()) | |
| const SPOOFED_ID = 'os:spoofed-not-a-real-account-9f3a1c' | |
| principal: { id: SPOOFED_ID, displayName: 'spoofed', kind: 'local-os-user' }, | |
| }), | |
| encoding: 'utf8', | |
| }) | |
| const result = JSON.parse(raw) as { | |
| status: string | |
| meta?: { proposedBy?: { id?: string }; confirmedBy?: { id?: string } } | |
| } | |
| expect(result.status).toBe('succeeded') | |
| // The payload identity must never surface, independent of who runs the test. | |
| expect(result.meta?.proposedBy?.id).not.toBe(SPOOFED_ID) | |
| expect(result.meta?.confirmedBy?.id).not.toBe(SPOOFED_ID) | |
| expect(result.meta?.proposedBy?.id).toBe(hostId()) | |
| expect(result.meta?.confirmedBy?.id).toBe(hostId()) |
🤖 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/node-bridge-principal.test.ts` around lines 30 - 41,
Update the test’s spoofed principal in the bridge invocation to use an identity
guaranteed not to match the host, rather than “os:root”. In the assertions after
parsing the result, explicitly verify proposedBy.id and confirmedBy.id are not
equal to the spoof value, while retaining the existing host identity checks.
Keep the change scoped to the test case exercising principal handling.
| import { | ||
| consumeTrustedConfirmation, | ||
| isRegisteredConfirmationEvidence, | ||
| isRegisteredMembranePrincipal, | ||
| } from './credential-boundary.js' | ||
| import { createPolicyEvaluator, loadPolicyStore } from './authority-registry.js' | ||
|
|
||
| declare const membranePrincipalBrand: unique symbol | ||
|
|
||
| const policyEvaluator = createPolicyEvaluator(loadPolicyStore()) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify EMPTY_STORE, the readable flag, and any diagnostics on policy-store load failure.
set -euo pipefail
REG=$(fd -t f 'authority-registry.ts' -p apps/hull | head -n1)
echo "== $REG =="
cat -n "$REG"
echo "== is the readable flag consumed anywhere? =="
rg -n -C 3 '\breadable\b' apps/hull packages 2>/dev/null || echo "no consumers of readable"
echo "== any logging/diagnostics in the registry loader? =="
rg -n -C 2 'console\.|logger|warn|emit|diagnostic' "$REG" || echo "no diagnostics emitted"
echo "== secureInvoke call sites and whether they pass a broker =="
ast-grep run --pattern 'secureInvoke($$$)' --lang typescript apps/hull packages 2>/dev/null | head -n 80Repository: Harborline-Software/shipyard
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files without relying on output size heavy rg result sets.
PEP=$(fd -t f 'pep.ts' -p apps/hull | head -n1)
REG=$(fd -t f 'authority-registry.ts' -p apps/hull | head -n1)
ADAPTER=""
for name in hull-port-adapter.ts protocol_adapter.ts; do
for f in $(fd -t f "$name" -p apps/hull); do
case "$f" in
*/hull-port-adapter.ts) ADAPTER="$f"; break ;;
esac
done
done
echo "PEP=$PEP"
echo "REG=$REG"
echo "ADAPTER=$ADAPTER"
for file in "$PEP" "$REG" "$ADAPTER"; do
echo "== file $file exists ==" "$([[ -f "$file" ]] && echo yes || echo no)"
done
echo "== authority-registry.ts =="
cat -n "$REG"
echo "== selected PEP section =="
sed -n '45,95p;320,380p;420,458p' "$PEP" | cat -n
echo "== adapter lines =="
[[ -n "$ADAPTER" ]] && sed -n '50,95p' "$ADAPTER" | cat -n
echo "== deterministic trace for EMPTY_STORE and invoke =="
node - <<'JS'
const EMPTY_STORE = Object.freeze({ commands: new Map(), readable: false })
function createPolicyEvaluator(store) {
return function evaluate(command) {
if (store.commands?.has(command)) return store.commands.get(command)
if (!store.readable) return authority: 'CP' // not literal object, placeholder
return authority: 'CP'
}
}
createPolicyEvaluator(EMPTY_STORE)
JSRepository: Harborline-Software/shipyard
Length of output: 14831
🏁 Script executed:
#!/bin/bash
set -euo pipefail
PEP=apps/hull/src/membrane/pep.ts
ADAPTER=apps/hull/src/protocol/hull-port-adapter.ts
echo "== secureInvoke continuation =="
sed -n '95,150p' "$PEP" | cat -n
echo "== credential-boundary confirmation consumer =="
cred=$(fd -t f 'credential-boundary.ts' -p apps/hull/src/membrane | head -n1)
echo "credential-boundary=$cred"
cat -n "$cred"
echo "== inspect command-authority JSON invoke entries if present =="
for p in packages/carrier-sdk/src/command-authority.json packages/carrier-sdk/dist/command-authority.json; do
echo "== $p =="
if [[ -f "$p" ]]; then
python3 - <<'PY' "$p"
import json, sys
p=sys.argv[1]
with open(p, encoding='utf-8') as f:
data=json.load(f)
commands=data.get('commands', {})
if 'invoke' in commands:
print(json.dumps({'invoke': commands['invoke']}, indent=2, sort_keys=True))
else:
print('no invoke command')
print(json.dumps({'commands_count': len(commands), 'commands_sample_start': list(commands.keys())[:20]}, indent=2, sort_keys=True))
PY
else
echo "missing"
fi
done
echo "== read-only JS model of the enforcement trace =="
node - <<'JS'
const EMPTY_STORE = Object.freeze({
commands: Object.freeze(Object.create(null)),
readable: false
})
function createPolicyEvaluator(store) {
return (command) => {
const entry = store.commands[command]
if (entry?.authority === 'AP' || entry?.authority === 'CP') {
return { authority: entry.authority, summary: entry.summary }
}
return { authority: 'CP', summary: `unknown command '${command}' — fail-closed to confirmation-required` }
}
}
const policyEvaluator = createPolicyEvaluator(EMPTY_STORE)
console.log('policyEvaluator.invoke =', JSON.stringify(policyEvaluator('invoke')))
console.log('broker undefined truthy:', !!undefined)
console.log('consume unknown broker without broker body returns false when token cannot be consumed')
JSRepository: Harborline-Software/shipyard
Length of output: 6314
issue: emit an observable signal when EMPTY_STORE is used.
loadPolicyStore() turns every registry-read failure into readable: false, and createPolicyEvaluator maps every unknown command to CP. invoke is AP in the source registry, but packages/carrier-sdk/dist/command-authority.json is missing, while the source path is outside this repository. A clean checkout can therefore start with policyEvaluator('invoke') == { authority: 'CP', summary: "unknown command 'invoke' — fail-closed to confirmation-required" }, causing every secured invoke to reject via membrane.authority_rejected.
Keep the fail-closed decision, but fail closed only after recording why. Log once at module load with the readable source/destination and URL values.
🤖 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 56 - 65, Update the module-load
policy initialization around loadPolicyStore and policyEvaluator to record a
single observable warning when the loaded store has readable false. Include the
store’s readable, source, destination, and URL values in that log, while
preserving createPolicyEvaluator’s fail-closed behavior and avoiding repeated
logging during command evaluation.
| export function currentOsPrincipal(): LocalOsUserPrincipal { | ||
| let username = '' | ||
| let username: string | ||
| try { | ||
| username = userInfo().username | ||
| } catch { | ||
| username = process.env.USER ?? process.env.USERNAME ?? '' | ||
| throw new Error('OS username is unavailable; refusing anonymous Carrier authority') | ||
| } | ||
| return localOsUserPrincipal(username) | ||
| const trimmed = username.trim() | ||
| if (trimmed.length === 0) { | ||
| throw new Error('OS username is blank; refusing anonymous Carrier authority') | ||
| } | ||
| return Object.freeze(localOsUserPrincipal(trimmed)) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
pnpm kb query "Which Carrier SDK tests exercise currentOsPrincipal when os.userInfo throws or returns a blank username, and how are those tests included in CI?"
rg -n -C 4 \
'currentOsPrincipal|OS username is unavailable|OS username is blank|userInfo\(' \
packages/carrier-sdk --glob '*.{test,spec}.ts'Repository: Harborline-Software/shipyard
Length of output: 8733
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "## candidate files"
fd -a 'principal\.(ts|test\.ts)|sdk\.test\.ts|package.json|vitest.*|\.github/workflows|pnpm-lock.yaml' . | sed 's#^\./##'
echo
echo "## principal.ts outline/content"
ast-grep outline packages/carrier-sdk/src/principal.ts --view expanded || true
cat -n packages/carrier-sdk/src/principal.ts | sed -n '1,120p'
echo
echo "## principal.test.ts"
cat -n packages/carrier-sdk/src/principal.test.ts | sed -n '1,220p'
echo
echo "## references to currentOsPrincipal/tests only"
rg -n -C 3 'currentOsPrincipal|OS username is unavailable|OS username is blank|userInfo\(' packages packages/carrier-sdk -g '!node_modules' -g '!dist' -g '!build'Repository: Harborline-Software/shipyard
Length of output: 33309
🌐 Web query:
pnpm kb query Carrier SDK tests currentOsPrincipal userInfo throws blank username CI
💡 Result:
The issue you are encountering with os.userInfo() resulting in a blank or throwing an error in CI environments is a known behavior in Node.js applications when the execution environment lacks a valid entry for the current user in /etc/passwd [1][2][3][4]. This commonly occurs in containerized CI environments (such as Docker) or when using AD/LDAP users where the OS cannot resolve the user information [1][3][4]. The Node.js os.userInfo() function specifically requires this system data to succeed; otherwise, it throws a SystemError (typically ENOENT) [1][3][4]. To resolve or prevent this in your tests and CI workflows, consider the following strategies: 1. Use Environment Variables: Implement a fallback in your code to prefer process.env.USER or process.env.USERNAME before calling os.userInfo() [3][4]. 2. Provide a Fallback Value: Ensure your logic handles cases where os.userInfo() fails or returns a blank object by using a default string (e.g., "unknown-user") [4]. 3. Configure Environment: If using containers, ensure the environment is correctly set up to support user resolution, or use CDK_HOME (or similar environment variables) to point to a directory that does not rely on home-directory lookups [1][3]. While your query mentioned the "Carrier SDK," please note that this is a common issue across many SDKs and tools (such as AWS CDK or VS Code Toolkits) that rely on os.userInfo() to identify the current principal [1][2][3][4]. If you are working with a specific proprietary or private "Carrier SDK," verify if it has a configuration to bypass system username lookups or if you can wrap the calls in a safe-getter utility [4].
Citations:
- 1: aws-auth: sdk-provider uses os.userInfo() which fails in containers without a home directory aws/aws-cdk#17454
- 2: context-provider: Context loading fails when /etc/passwd is not available aws/aws-cdk#18824
- 3: (api/aws-auth): Error on CDK commands when user does not exist aws/aws-cdk#19401
- 4: fix(env): getUserInfo() may fail on AD/LDAP aws/aws-toolkit-vscode#5279
todo [blocking]: Add CI coverage for currentOsPrincipal() fail-closed paths.
packages/carrier-sdk/src/principal.test.ts only exercises normal and deterministic successful currentOsPrincipal() reads. The packages/carrier-sdk/src/sdk.test.ts SDK tests also use the real, non-blank OS principal. Add isolated tests that force os.userInfo() to throw and return a blank/whitespace username, then route them through an unskipped CI job.
🤖 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/principal.ts` around lines 40 - 51, Add isolated
CI-covered tests for currentOsPrincipal() in principal.test.ts that mock
os.userInfo() to throw and to return a blank or whitespace username, asserting
both fail with the expected errors. Ensure the tests do not depend on the real
OS principal and are included in an unskipped CI test job.
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: Correct the invocation authority documentation.
The JSDoc says that this.actingAs is carried into invokeInProcess. Lines 155-156 no longer pass that principal. invokeInProcess now creates a host-owned secured invoker. Update the documentation to describe host-side principal issuance.
🤖 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, Update the JSDoc for
the invocation path around invokeInProcess to remove the claim that
this.actingAs is passed through. Document that invokeInProcess creates the
host-owned secured invoker and issues the invocation principal on the host side,
while caller options cannot replace the stored identity.
| try { | ||
| parseCarrierProtocolModel(model, candidate) | ||
| throw new Error(`${file}: parser accepted a fixture violating ${constraint}`) | ||
| } catch (error) { | ||
| assertDeclaredConstraint(error, testCase) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
issue: the "parser accepted" sentinel is caught by its own catch.
Line 99 throws inside the try, so line 100 catches that sentinel and forwards it to assertDeclaredConstraint. When a negative fixture is wrongly accepted, the reported failure is a constraint-diagnostic mismatch, not the real cause. Move the sentinel out of the try so the acceptance failure surfaces directly.
💚 Proposed fix
- try {
- parseCarrierProtocolModel(model, candidate)
- throw new Error(`${file}: parser accepted a fixture violating ${constraint}`)
- } catch (error) {
- assertDeclaredConstraint(error, testCase)
- }
+ let rejection: unknown
+ let accepted = false
+ try {
+ parseCarrierProtocolModel(model, candidate)
+ accepted = true
+ } catch (error) {
+ rejection = error
+ }
+ expect(accepted, `${file}: parser accepted a fixture violating ${constraint}`).toBe(false)
+ assertDeclaredConstraint(rejection, testCase)📝 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.
| try { | |
| parseCarrierProtocolModel(model, candidate) | |
| throw new Error(`${file}: parser accepted a fixture violating ${constraint}`) | |
| } catch (error) { | |
| assertDeclaredConstraint(error, testCase) | |
| } | |
| let rejection: unknown | |
| let accepted = false | |
| try { | |
| parseCarrierProtocolModel(model, candidate) | |
| accepted = true | |
| } catch (error) { | |
| rejection = error | |
| } | |
| expect(accepted, `${file}: parser accepted a fixture violating ${constraint}`).toBe(false) | |
| assertDeclaredConstraint(rejection, testCase) |
🤖 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/contracts/src/__tests__/protocol-fixtures.test.ts` around lines 97 -
102, Move the parser-accepted sentinel throw in the negative-fixture test
outside the try/catch that calls parseCarrierProtocolModel, so only parser
errors reach assertDeclaredConstraint; preserve the existing constraint
validation for rejected fixtures and ensure wrongly accepted fixtures report the
direct acceptance failure.
| private sealed record FixtureManifest( | ||
| List<FixtureCase> Fixtures, | ||
| List<FixtureCase>? NegativeFixtures); | ||
|
|
||
| public static TheoryData<string, string, string, string?, string?> Cases | ||
| { | ||
| get | ||
| { | ||
| var manifest = ReadManifest(); | ||
| var cases = new TheoryData<string, string, string, string?, string?>(); | ||
| foreach (var fixture in manifest.Fixtures.Concat(manifest.NegativeFixtures ?? [])) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
issue: NegativeFixtures is still optional in the C# manifest record.
A previous review asked for this and the thread is marked as addressed, but Line 15 still declares List<FixtureCase>? NegativeFixtures and Line 23 still applies ?? []. If negativeFixtures is absent from manifest.json, the C# suite builds a valid data set and runs zero rejection cases. The Rust FixtureManifest in packages/contracts/rust/src/lib.rs requires the field, so the two language suites enforce different manifest contracts.
🐛 Proposed fix
- foreach (var fixture in manifest.Fixtures.Concat(manifest.NegativeFixtures ?? []))
+ var negativeFixtures = manifest.NegativeFixtures
+ ?? throw new InvalidOperationException(
+ "protocol fixture manifest is missing negativeFixtures");
+ foreach (var fixture in manifest.Fixtures.Concat(negativeFixtures))🤖 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/contracts/tests/ProtocolFixtureTests.cs` around lines 13 - 23, Make
NegativeFixtures required in the FixtureManifest record and remove the
null-coalescing fallback from the manifest fixture enumeration. Ensure
ReadManifest deserialization rejects manifests missing negativeFixtures,
matching the required field contract in Rust.
Source: Path instructions
| Exception rejection; | ||
| try | ||
| { | ||
| JsonSerializer.Deserialize(source, modelType, Options); | ||
| throw new Xunit.Sdk.XunitException($"{file}: parser accepted a fixture violating {constraint}"); | ||
| } | ||
| catch (Exception error) | ||
| { | ||
| rejection = error; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
issue [blocking]: the catch block swallows the "parser accepted" failure.
throw new Xunit.Sdk.XunitException(...) at Line 72 is inside the try block. The catch (Exception error) at Line 74 catches it and assigns it to rejection. When the parser accepts a fixture that must be rejected, the test does not fail at Line 72. It continues and asserts against the text of that XunitException instead.
Today the later assertions still fail for the current fixtures, so the gate holds by accident. The reported failure names the wrong cause, and any fixture whose declared path and constraint keyword appear in the exception text would pass a fixture that the parser wrongly accepted.
Capture the deserialization exception without wrapping the success path in the same try.
As per path instructions, "the change's CI suite must actually exercise it (A1)".
🐛 Proposed fix
- Exception rejection;
- try
- {
- JsonSerializer.Deserialize(source, modelType, Options);
- throw new Xunit.Sdk.XunitException($"{file}: parser accepted a fixture violating {constraint}");
- }
- catch (Exception error)
- {
- rejection = error;
- }
-
+ var rejection = Record.Exception(() => JsonSerializer.Deserialize(source, modelType, Options));
+ Assert.True(
+ rejection is not null,
+ $"{file}: parser accepted a fixture violating {constraint}");
+
var diagnostic = rejection.ToString();📝 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.
| Exception rejection; | |
| try | |
| { | |
| JsonSerializer.Deserialize(source, modelType, Options); | |
| throw new Xunit.Sdk.XunitException($"{file}: parser accepted a fixture violating {constraint}"); | |
| } | |
| catch (Exception error) | |
| { | |
| rejection = error; | |
| } | |
| var rejection = Record.Exception(() => JsonSerializer.Deserialize(source, modelType, Options)); | |
| Assert.True( | |
| rejection is not null, | |
| $"{file}: parser accepted a fixture violating {constraint}"); | |
| var diagnostic = rejection.ToString(); |
🤖 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/contracts/tests/ProtocolFixtureTests.cs` around lines 68 - 77,
Update the rejection-capture logic in the test around JsonSerializer.Deserialize
so only the deserialization call is inside the catchable try block; perform the
XunitException for an accepted fixture after that block, when no exception was
captured. Preserve the existing rejection assertions and ensure the CI test path
exercises the parser-acceptance case.
Source: Path instructions
|
Superseded by a split. Closing in favour of two stacked pull requests.
WhyThis branch combined two changes. Three independent reviews confirmed the contract work sound while the credential boundary absorbed remediation round after round, so the finished half could not land. Splitting let the contract work merge on its own merits and gave the credential boundary a review scoped to one question. The split is lossless: the two halves reproduce this branch's head exactly, apart from fixes made afterwards. What the split surfaced that this branch was hidingThis branch did not compile. A CP-demo regression, live and reachable from shipped UI. A strict validator was wired onto a response the bridge emits with a seventh key. The failure envelope is the narrow already-conforming one, so a rejected operation validated and reported honestly while an allowed, properly-confirmed one failed validation and surfaced as rejected. Root cause: the conformance corpus was authored from the schema rather than captured from the producers, so it could never see the disagreement. A credential could escape the trust boundary. The mints were private; the minted values were not. Because the enforcement point authenticates by object identity, a leaked principal is a bearer token good for any command — and three public channels handed one out. Every prior round had asked whether a credential could be constructed; none had asked whether one could get out. Full details in each pull request. |
1 participant
Summary
Testing
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation