feat(hull-dotnet): port the five built membrane faces and the resolution pipeline - #3655
Conversation
|
Warning Review limit reached
Next review available in: 7 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (11)
📝 WalkthroughWalkthroughAdded the Hull.NET package with capability contracts, membrane operations, runtime transports, resolution logic, shell orchestration, project wiring, documentation, and end-to-end tests. Local subprocess transport fails closed until sandbox support is available. ChangesHull.NET implementation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
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: 13
🧹 Nitpick comments (16)
packages/hull-dotnet/Runtime/RuntimeHost.cs (1)
6-13: 🚀 Performance & Scalability | 🔵 Trivialthought:
IRuntimeHostis synchronous whileIRuntimeTransportis asynchronous. For the reference stub this seam is harmless, and it keeps in-process hosts simple.The seam does constrain future hosts. A real image runtime that performs GPU or disk work inside
Invokewill block the caller's thread for the whole operation, becauseInProcessTransportwraps the synchronous call inTask.FromResult. Consider whetherIRuntimeHostshould expose async members before the first non-stub host implements it. Changing this interface later is a breaking change for every host.No change requested in 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/hull-dotnet/Runtime/RuntimeHost.cs` around lines 6 - 13, No change is requested; leave the synchronous IRuntimeHost interface and its current transport seam unchanged.packages/hull-dotnet/Membrane/Address.cs (1)
13-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion:
Adr0061TransportTierspins an ADR number into a public type name. If ADR 0061 is later superseded or amended, the type name becomes misleading, and renaming a public type is a breaking change for consumers.Name the type for the concept and record the ADR reference in a doc comment instead.
♻️ Proposed change
+/// Transport tiers defined by ADR 0061. -public static class Adr0061TransportTiers +public static class TransportTiers { public const string LocalNetwork = "LocalNetwork"; public const string MeshVpn = "MeshVpn"; public const string ManagedRelay = "ManagedRelay"; }The ADR number itself is fine: 0061 is not one of the reserved numbers, and no locked vocabulary appears here.
As per path instructions, "avoid introducing reserved ADR numbers 0067, 0068, or 0076" and "Any future ADR amendment must use the next gap-free A-number."
🤖 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/hull-dotnet/Membrane/Address.cs` around lines 13 - 18, Rename the public type Adr0061TransportTiers to a concept-based name that does not embed the ADR number, preserving the existing LocalNetwork, MeshVpn, and ManagedRelay constants. Add a documentation comment on the renamed type recording ADR 0061 as its reference.Source: Path instructions
packages/hull-dotnet/Membrane/Transport.cs (1)
24-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick wintodo:
BaseUriaccepts anyUri, including a non-loopbackhttp://host. No code path dereferences it today, because all five operations fail closed, so there is no current exposure.When the sandbox transport is wired, validate
baseUriat construction. A local-subprocess arm should only accept a loopback host. That check is much cheaper to add now than to retrofit after the first request-issuing code path exists.🛡️ Proposed guard
public sealed class LocalSubprocessTransport(string runtimeId, Uri baseUri) : IRuntimeTransport { public string RuntimeId { get; } = runtimeId; public string Mode => AddressModes.LocalSubprocess; - public Uri BaseUri { get; } = baseUri; + public Uri BaseUri { get; } = IsLoopback(baseUri) + ? baseUri + : throw new ArgumentException( + "local-subprocess requires a loopback base URI", nameof(baseUri)); + + private static bool IsLoopback(Uri uri) => uri.IsLoopback;🤖 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/hull-dotnet/Membrane/Transport.cs` around lines 24 - 28, Update the LocalSubprocessTransport constructor to validate baseUri before assignment, accepting only URIs whose host is loopback; reject non-loopback hosts, including non-loopback http:// URIs, while preserving the existing RuntimeId, Mode, and BaseUri behavior for valid inputs.packages/hull-dotnet/Contracts/CapabilityContracts.cs (2)
43-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuenit: Four capability ids are inline string literals here, but
imageandbank-importare constants inCapabilityIds. Add the remaining ids as constants so every capability id has one definition site.♻️ Proposed change
public static class CapabilityIds { public const string Image = "image"; public const string BankImport = "bank-import"; + public const string Tts = "tts"; + public const string Embeddings = "embeddings"; + public const string Rerank = "rerank"; + public const string Generate = "generate"; }public sealed record TtsCore(string Text, string? Voice, string Format, double Timeout) - : CapabilityCore("tts"); + : CapabilityCore(CapabilityIds.Tts); public sealed record EmbeddingsCore(IReadOnlyList<string> Inputs, int Dimension, double Timeout) - : CapabilityCore("embeddings"); + : CapabilityCore(CapabilityIds.Embeddings);🤖 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/hull-dotnet/Contracts/CapabilityContracts.cs` around lines 43 - 58, Add constants for the inline capability IDs used by TtsCore, EmbeddingsCore, RerankCore, and GenerateCore in CapabilityIds, then update each corresponding constructor to reference those constants instead of string literals. Preserve the existing ID values and ensure all capability IDs have a single definition site.
193-200: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion [dotnet]: document
SelectionReasonandReasoninResolutionResultBoth fields exist in the canonical TypeScript contracts. Add a short C# comment on this record to state that
SelectionReasonis the closed selection-class drivingSpeedHint, whileReasonis the human display text.🤖 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/hull-dotnet/Contracts/CapabilityContracts.cs` around lines 193 - 200, Add a concise XML documentation comment to the ResolutionResult record clarifying that SelectionReason is the closed selection class that drives SpeedHint, while Reason contains human-readable display text; leave the record fields and behavior unchanged.packages/hull-dotnet/Runtime/ReferenceImageRuntime.cs (1)
44-59: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuenit: Two small things in
Invoke.
core.Formatis interpolated into the MIME type at line 59 with no validation. A caller supplying an arbitraryFormatproduces a malformedMimevalue in the result envelope. There is no injection sink in this cohort, because the artifact stays an in-memorystub://record. It is still worth validating against a short allowlist so consumers can trust the field. TheUri.EscapeDataStringoncore.Promptin the same line is the right call.Separately, lines 44 and 51 use
Array.Empty<Artifact>()while the rest of the file uses collection expressions.♻️ Proposed change
- jobId, "failed", 0, Array.Empty<Artifact>(), new Usage("image", 0, null, "local"), + jobId, "failed", 0, [], new Usage("image", 0, null, "local"),+ if (core.Format is not ("png" or "jpeg" or "webp")) + { + return new CapabilityResult( + jobId, "failed", 0, [], new Usage("call", 0, null, "local"), + new CapabilityError("input", false, "input.unsupported_format", $"reference runtime does not render '{core.Format}'", null)); + } + return new CapabilityResult(🤖 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/hull-dotnet/Runtime/ReferenceImageRuntime.cs` around lines 44 - 59, Update Invoke so core.Format is validated against a short supported-format allowlist before constructing the ImageArtifact MIME value, returning the existing input-failure result for unsupported formats; preserve the Uri.EscapeDataString handling for core.Prompt. Also replace the two Array.Empty<Artifact> usages in Invoke with the file’s collection-expression style.Shipyard.slnx (1)
641-644: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuenit: the solution folder name keeps the
packages/prefix that every sibling drops.All other folders in this file use the short form, for example
/contracts/,/ui-core/, and/bridge-subscription/. Use/hull/or/hull-dotnet/for consistency. The project paths themselves are correct.♻️ Proposed rename
- <Folder Name="/packages/hull-dotnet/"> + <Folder Name="/hull-dotnet/"> <Project Path="packages/hull-dotnet/Shipyard.Hull.csproj" /> <Project Path="packages/hull-dotnet/tests/Shipyard.Hull.Tests.csproj" /> </Folder>As per path instructions, "preserve established naming conventions".
🤖 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 `@Shipyard.slnx` around lines 641 - 644, Rename the solution folder entry from “/packages/hull-dotnet/” to the consistent short form “/hull-dotnet/”, while preserving both existing project paths unchanged.Source: Path instructions
packages/hull-dotnet/Membrane/Negotiate.cs (1)
37-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuenit:
SchemaMajorMatchesduplicatesIsContractVersionCompatible.Both methods parse two versions and compare majors. Keep one rule and let the schema path call it. This also keeps the two comparisons from drifting apart later.
♻️ Proposed consolidation
- private static bool SchemaMajorMatches(string shell, string runtime) - { - var shellVersion = ParseSemver(shell); - var runtimeVersion = ParseSemver(runtime); - return shellVersion is not null && runtimeVersion is not null - && shellVersion.Value.Major == runtimeVersion.Value.Major; - } + private static bool SchemaMajorMatches(string shell, string runtime) => + IsContractVersionCompatible(shell, runtime);🤖 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/hull-dotnet/Membrane/Negotiate.cs` around lines 37 - 43, Remove the duplicate major-version comparison in SchemaMajorMatches and route the schema compatibility check through the existing IsContractVersionCompatible method. Preserve the current behavior for valid and invalid semantic versions while ensuring both paths use the single shared compatibility rule.packages/hull-dotnet/tests/RoundTripTests.cs (1)
114-122: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuenit: Line 121 asserts nothing.
_ = result.Error;reads like a check inside an assertion helper but only discards the value. Either drop the line, or assert the intended invariant, for example thatErroris non-null whenStatusis"failed".♻️ Proposed change
Assert.NotNull(result.Usage); - _ = result.Error; + if (result.Status == "failed") + { + Assert.NotNull(result.Error); + }🤖 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/hull-dotnet/tests/RoundTripTests.cs` around lines 114 - 122, Update AssertUniformEnvelope so it no longer includes the ineffective `_ = result.Error` expression; either remove it or replace it with an assertion that Error is non-null when result.Status indicates failure.packages/hull-dotnet/Membrane/Invoke.cs (1)
55-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuenit: the second predicate on Line 59 is redundant.
statusequals"failed"only whennative.Statusis"failed"or invalid. Sostatus == "failed" && native.Status != "failed"already means the native status was invalid. The trailingnative.Status is not (...)list adds no branch and must be kept in sync with Line 55 by hand.♻️ Proposed simplification
- if (status == "failed" && error is null && native.Status is not ("failed" or "accepted" or "running" or "succeeded" or "partial")) + if (status == "failed" && error is null && native.Status != "failed")🤖 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/hull-dotnet/Membrane/Invoke.cs` around lines 55 - 64, In the error-assignment condition within Invoke, remove the redundant native.Status pattern check and rely on status being "failed" while native.Status is not "failed" to identify invalid statuses. Preserve the existing null-error guard and CapabilityError behavior.packages/hull-dotnet/Shell/HullShell.cs (1)
106-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuenit:
"tts"is a bare literal while the other two entries useCapabilityIds.Line 110 and Line 119 both repeat
"tts". The image and bank-import entries use constants. AddCapabilityIds.Ttsand use it in both places.🤖 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/hull-dotnet/Shell/HullShell.cs` around lines 106 - 113, Define the missing CapabilityIds.Tts constant and replace both repeated bare "tts" literals in the composition entries, including CarrierComposition and the other occurrence, with that constant while preserving the existing entry values.packages/hull-dotnet/Resolution/PackResolver.cs (4)
274-278: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuenit (non-blocking): parse version parts with invariant culture and no sign.
int.TryParse(string, out int)usesNumberStyles.Integerand the current culture. It accepts a leading sign and surrounding whitespace."1.-2.3"parses toVersionValue(1, -2, 3)instead of being rejected. The result also depends on the ambient culture, which weakens clean-checkout reproducibility.Use
NumberStyles.NonewithCultureInfo.InvariantCulture.♻️ Proposed change
+using System.Globalization;var parts = value.Trim().Split('.'); if (parts.Length == 3 - && int.TryParse(parts[0], out var major) - && int.TryParse(parts[1], out var minor) - && int.TryParse(parts[2], out var patch)) + && int.TryParse(parts[0], NumberStyles.None, CultureInfo.InvariantCulture, out var major) + && int.TryParse(parts[1], NumberStyles.None, CultureInfo.InvariantCulture, out var minor) + && int.TryParse(parts[2], NumberStyles.None, CultureInfo.InvariantCulture, out var patch))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 `@packages/hull-dotnet/Resolution/PackResolver.cs` around lines 274 - 278, Update the version-part parsing in the visible resolver logic to call the overloads of int.TryParse for parts[0], parts[1], and parts[2] with NumberStyles.None and CultureInfo.InvariantCulture, preserving the existing three-part validation and VersionValue construction.Source: Path instructions
60-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion (non-blocking): use
CompositionErrorfor an unknown scope tier.Every other catalog-validation failure in this file throws
CompositionErrorwith a machine-readableReason. Line 66 throwsArgumentOutOfRangeExceptioninstead. A caller that catchesCompositionErrorto report catalog problems will miss a malformedScopeTier.PackSpecificityis public and is also reached fromComputeClosureandMergeDefaults, so the inconsistency is observable.♻️ Proposed change
- _ => throw new ArgumentOutOfRangeException(nameof(pack), pack.ScopeTier, "Unknown scope tier") + _ => throw Error("unknown-scope-tier", $"pack '{pack.Name}' declares unknown scope tier '{pack.ScopeTier}'", [pack.Name], pack.ScopeTier)🤖 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/hull-dotnet/Resolution/PackResolver.cs` around lines 60 - 67, Update PackSpecificity’s unknown ScopeTier branch to throw CompositionError instead of ArgumentOutOfRangeException, using the established machine-readable Reason pattern used by other catalog-validation failures in the file. Preserve the existing specificity values and known-tier behavior.
245-247: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuenit (non-blocking): the nested ternary calls
TryGetValuetwice and reassignsprevious.Line 245 and Line 247 both call
owners.TryGetValue(key, out previous). The second call reassigns the sameoutvariable inside a ternary branch. The intent is clear only after tracing all three branches. A single lookup makes the three cases explicit.♻️ Proposed change
- var overrode = owners.TryGetValue(key, out var previous) && previous.Pack != packName - ? [.. previous.Overrode, previous.Pack] - : owners.TryGetValue(key, out previous) ? previous.Overrode : new List<string>(); + List<string> overrode; + if (!owners.TryGetValue(key, out var previous)) overrode = []; + else if (previous.Pack != packName) overrode = [.. previous.Overrode, previous.Pack]; + else overrode = previous.Overrode;🤖 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/hull-dotnet/Resolution/PackResolver.cs` around lines 245 - 247, Update the owner lookup around the overrode calculation to call owners.TryGetValue only once, store its result, and express the existing changed-pack, unchanged-pack, and missing-owner cases explicitly. Preserve the current overrode values and appended previous.Pack behavior while removing the nested ternary and reassignment of previous.
244-244: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winsuggestion (non-blocking): make the merged collection order deterministic.
entriesis aDictionary<string, JsonElement>. Its enumeration order is not a documented guarantee. Line 244 serializesentries.Valuesdirectly intoResolvedDefaults, so the element order of every merged collection default depends on an unspecified detail.The verification policy requires clean-checkout reproducible results. Order the entries explicitly before serializing.
♻️ Proposed change
- merged[key] = JsonSerializer.SerializeToElement(entries.Values.ToArray()); + merged[key] = JsonSerializer.SerializeToElement( + entries.OrderBy(pair => pair.Key, StringComparer.Ordinal).Select(pair => pair.Value).ToArray());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 `@packages/hull-dotnet/Resolution/PackResolver.cs` at line 244, Update the merged collection serialization at the assignment to merged[key] so entries are ordered explicitly before converting their values to an array and serializing. Use a stable ordering based on each dictionary entry’s key, preserving the existing JsonSerializer.SerializeToElement flow and resulting values.Source: Path instructions
packages/hull-dotnet/Resolution/Pipeline.cs (1)
62-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion (non-blocking): three distinct failures collapse into
"locked-entitlement".Line 65 reports
"locked-entitlement"when the provider-license gate blocks every provider. Line 72 reports the same state when the configuration resolver chooses nothing. Line 49 reports it for an unentitled non-core capability.
HullShell.InvokeAsyncderives the error code fromResolutionState, so all three producemembrane.resolution_locked_entitlement. A caller cannot tell a licensing block from a configuration gap. Introduce distinct states, for example"locked-license"and"no-configured-provider".🤖 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/hull-dotnet/Resolution/Pipeline.cs` around lines 62 - 73, Update the resolution states returned by the provider-license gate and configuration-selection branches in the pipeline method containing survivors and choice so they no longer use "locked-entitlement": return distinct states such as "locked-license" for an empty survivors set and "no-configured-provider" when choice is null. Preserve the existing entitlement state for the unentitled non-core capability path and ensure HullShell.InvokeAsync maps the new ResolutionState values to distinct error codes.
🤖 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/hull-dotnet/Membrane/Announce.cs`:
- Around line 10-13: Update the capability indexing in Announce.Index so
duplicate CapabilityId values cannot let ToDictionary throw during ConnectAsync.
Either deterministically retain one capability entry or reject the manifest
through the existing typed membrane-error/envelope path, ensuring malformed
announcements produce a structured result rather than an unhandled
ArgumentException.
In `@packages/hull-dotnet/Membrane/Invoke.cs`:
- Around line 33-48: Update the catch logic in InvokeCapabilityAsync to detect
OperationCanceledException caused by the caller’s cancellationToken and rethrow
it before transport-fault logging and CapabilityResult construction. Preserve
the existing transport-fault handling for other exceptions and cancellation not
requested by the caller.
In `@packages/hull-dotnet/Resolution/PackResolver.cs`:
- Around line 251-253: Update Satisfies to distinguish an unparseable pack
version from a valid version that fails the constraint: when TryParseVersion
cannot parse version, raise the established explicit version-parse/malformed
error instead of returning false. Preserve the existing false result for valid
versions that do not satisfy constraint, and keep constraint parse failures
handled as currently.
- Around line 168-193: Update the same-tier default resolution around the
foreach over pack.Defaults to track each key’s shape (keyed collection versus
scalar) independently of scalarContributors. Raise same-tier-conflict when
another pack uses the opposite shape, regardless of iteration order, while
preserving existing collection merging and scalar conflict behavior. Also
validate that any key declared in DefaultMergeKeys has an array value; otherwise
raise an explicit malformed-input error instead of falling back to scalar
handling.
- Around line 35-37: Update the composition-building flow around
capabilityProvenance and CompositionEntry to emit at most one entry per
CapabilityId, including when CheckTier1Collisions permits duplicate
non-domain-block realizations. Inspect the pack/provides contract for membership
metadata and stop forcing every resolved entry to "core" when "add-on" or "n-a"
is represented; align the result with HullShell.CarrierComposition and the
intended W3 behavior.
- Line 288: Update the JsonEqual method to compare JsonElement values with
JsonElement.DeepEquals rather than GetRawText string equality, ensuring
equivalent JSON formatting and property ordering are treated as equal before
same-tier conflicts are reported.
In `@packages/hull-dotnet/Resolution/Pipeline.cs`:
- Around line 100-101: Update NonResolving in Pipeline.cs to accept the failure
reason, tier, and speed hint from its callers instead of hard-coding
entitlement-gated, local, and moderate. Adjust all four call sites to pass
accurate values—use not-in-edition for the edition gate and unavailable-hardware
for hardware failures—while preserving each call site's descriptive explanation
text.
- Around line 125-130: Update the selection block in HullShell.Resolve to add
ProviderId as an explicit secondary ordering key after SpeedRank, ensuring
deterministic tie resolution. Also change the reason condition to check
pool.Length == 1 instead of survivors.Count == 1, while preserving the existing
fallback-floor and preferred-by-policy cases.
In `@packages/hull-dotnet/Runtime/ReferenceImageRuntime.cs`:
- Around line 11-17: Synchronize all mutable state in ReferenceImageRuntime,
especially _cancelledJobs updates performed by Cancel and reads exposed through
CancelledJobs, so concurrent InProcessTransport.CancelAsync calls and
enumeration are safe. Return a snapshot or otherwise prevent callers from
observing the live list, while preserving the existing API behavior. Because
this is a concurrency control-point change, flag it for independent review
rather than self-assertion.
In `@packages/hull-dotnet/Runtime/RuntimeHost.cs`:
- Around line 20-32: Update RuntimeHost’s five async methods—AnnounceAsync,
NegotiateOfferAsync, HealthAsync, InvokeAsync, and CancelAsync—to honor
cancellationToken before invoking the host and to return faulted tasks when host
operations throw instead of throwing synchronously. Add and reuse a private
generic helper for synchronous Func<T> operations, with equivalent handling for
the void Cancel operation.
In `@packages/hull-dotnet/Shell/HullShell.cs`:
- Around line 15-28: Replace the plain _connections Dictionary in HullShell with
a thread-safe ConcurrentDictionary, adding the required namespace import, while
preserving the existing ordinal key comparison and public Runtimes behavior.
Ensure ConnectAsync continues to publish connections through the concurrent
collection so concurrent ConnectAsync, reads, ProbeAsync, Resolve, and
InvokeAsync calls are safe.
- Around line 76-98: Update the invocation flow in HullShell’s resolution
handling to honor resolution.ChosenProviderId after
Resolve(request.CapabilityId) succeeds. Select the connection whose announced
providers include that exact chosen provider, or forward the chosen provider
through RuntimeConnectionLifecycle.InvokeAsync, and do not fall back to an
arbitrary compatible capability connection.
In `@packages/hull-dotnet/tests/RoundTripTests.cs`:
- Around line 87-103: Extend the round-trip test coverage around the existing
bank-import test to exercise the SEC-2 throwing path by omitting or invalidating
the idempotency key and asserting the fail-closed
IdempotencyKeyRequiredException behavior. Add focused tests for
Negotiate.Reconcile returning Compatible == false and RedactingLogSink redacting
sensitive values, using the existing test infrastructure. Mark these
control-point changes for independent review rather than relying on
self-assertion.
---
Nitpick comments:
In `@packages/hull-dotnet/Contracts/CapabilityContracts.cs`:
- Around line 43-58: Add constants for the inline capability IDs used by
TtsCore, EmbeddingsCore, RerankCore, and GenerateCore in CapabilityIds, then
update each corresponding constructor to reference those constants instead of
string literals. Preserve the existing ID values and ensure all capability IDs
have a single definition site.
- Around line 193-200: Add a concise XML documentation comment to the
ResolutionResult record clarifying that SelectionReason is the closed selection
class that drives SpeedHint, while Reason contains human-readable display text;
leave the record fields and behavior unchanged.
In `@packages/hull-dotnet/Membrane/Address.cs`:
- Around line 13-18: Rename the public type Adr0061TransportTiers to a
concept-based name that does not embed the ADR number, preserving the existing
LocalNetwork, MeshVpn, and ManagedRelay constants. Add a documentation comment
on the renamed type recording ADR 0061 as its reference.
In `@packages/hull-dotnet/Membrane/Invoke.cs`:
- Around line 55-64: In the error-assignment condition within Invoke, remove the
redundant native.Status pattern check and rely on status being "failed" while
native.Status is not "failed" to identify invalid statuses. Preserve the
existing null-error guard and CapabilityError behavior.
In `@packages/hull-dotnet/Membrane/Negotiate.cs`:
- Around line 37-43: Remove the duplicate major-version comparison in
SchemaMajorMatches and route the schema compatibility check through the existing
IsContractVersionCompatible method. Preserve the current behavior for valid and
invalid semantic versions while ensuring both paths use the single shared
compatibility rule.
In `@packages/hull-dotnet/Membrane/Transport.cs`:
- Around line 24-28: Update the LocalSubprocessTransport constructor to validate
baseUri before assignment, accepting only URIs whose host is loopback; reject
non-loopback hosts, including non-loopback http:// URIs, while preserving the
existing RuntimeId, Mode, and BaseUri behavior for valid inputs.
In `@packages/hull-dotnet/Resolution/PackResolver.cs`:
- Around line 274-278: Update the version-part parsing in the visible resolver
logic to call the overloads of int.TryParse for parts[0], parts[1], and parts[2]
with NumberStyles.None and CultureInfo.InvariantCulture, preserving the existing
three-part validation and VersionValue construction.
- Around line 60-67: Update PackSpecificity’s unknown ScopeTier branch to throw
CompositionError instead of ArgumentOutOfRangeException, using the established
machine-readable Reason pattern used by other catalog-validation failures in the
file. Preserve the existing specificity values and known-tier behavior.
- Around line 245-247: Update the owner lookup around the overrode calculation
to call owners.TryGetValue only once, store its result, and express the existing
changed-pack, unchanged-pack, and missing-owner cases explicitly. Preserve the
current overrode values and appended previous.Pack behavior while removing the
nested ternary and reassignment of previous.
- Line 244: Update the merged collection serialization at the assignment to
merged[key] so entries are ordered explicitly before converting their values to
an array and serializing. Use a stable ordering based on each dictionary entry’s
key, preserving the existing JsonSerializer.SerializeToElement flow and
resulting values.
In `@packages/hull-dotnet/Resolution/Pipeline.cs`:
- Around line 62-73: Update the resolution states returned by the
provider-license gate and configuration-selection branches in the pipeline
method containing survivors and choice so they no longer use
"locked-entitlement": return distinct states such as "locked-license" for an
empty survivors set and "no-configured-provider" when choice is null. Preserve
the existing entitlement state for the unentitled non-core capability path and
ensure HullShell.InvokeAsync maps the new ResolutionState values to distinct
error codes.
In `@packages/hull-dotnet/Runtime/ReferenceImageRuntime.cs`:
- Around line 44-59: Update Invoke so core.Format is validated against a short
supported-format allowlist before constructing the ImageArtifact MIME value,
returning the existing input-failure result for unsupported formats; preserve
the Uri.EscapeDataString handling for core.Prompt. Also replace the two
Array.Empty<Artifact> usages in Invoke with the file’s
collection-expression style.
In `@packages/hull-dotnet/Runtime/RuntimeHost.cs`:
- Around line 6-13: No change is requested; leave the synchronous IRuntimeHost
interface and its current transport seam unchanged.
In `@packages/hull-dotnet/Shell/HullShell.cs`:
- Around line 106-113: Define the missing CapabilityIds.Tts constant and replace
both repeated bare "tts" literals in the composition entries, including
CarrierComposition and the other occurrence, with that constant while preserving
the existing entry values.
In `@packages/hull-dotnet/tests/RoundTripTests.cs`:
- Around line 114-122: Update AssertUniformEnvelope so it no longer includes the
ineffective `_ = result.Error` expression; either remove it or replace it with
an assertion that Error is non-null when result.Status indicates failure.
In `@Shipyard.slnx`:
- Around line 641-644: Rename the solution folder entry from
“/packages/hull-dotnet/” to the consistent short form “/hull-dotnet/”, while
preserving both existing project paths unchanged.
🪄 Autofix
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: ce1a3a6f-1cab-4d35-a03e-7d7412518d5c
📒 Files selected for processing (19)
Shipyard.slnxpackages/hull-dotnet/Contracts/CapabilityContracts.cspackages/hull-dotnet/Membrane/Address.cspackages/hull-dotnet/Membrane/Announce.cspackages/hull-dotnet/Membrane/Invoke.cspackages/hull-dotnet/Membrane/Negotiate.cspackages/hull-dotnet/Membrane/Observe.cspackages/hull-dotnet/Membrane/RuntimeConnection.cspackages/hull-dotnet/Membrane/Transport.cspackages/hull-dotnet/README.mdpackages/hull-dotnet/Resolution/Composition.cspackages/hull-dotnet/Resolution/PackResolver.cspackages/hull-dotnet/Resolution/Pipeline.cspackages/hull-dotnet/Runtime/ReferenceImageRuntime.cspackages/hull-dotnet/Runtime/RuntimeHost.cspackages/hull-dotnet/Shell/HullShell.cspackages/hull-dotnet/Shipyard.Hull.csprojpackages/hull-dotnet/tests/RoundTripTests.cspackages/hull-dotnet/tests/Shipyard.Hull.Tests.csproj
| foreach (var pair in pack.Defaults) | ||
| { | ||
| var mergeKey = pack.DefaultMergeKeys?.GetValueOrDefault(pair.Key); | ||
| if (mergeKey is not null && pair.Value.ValueKind == JsonValueKind.Array) | ||
| { | ||
| MergeCollection(pair.Key, mergeKey, pair.Value, pack.Name, tierGroup.Key, merged, owners, collectionOwners); | ||
| continue; | ||
| } | ||
|
|
||
| if (scalarContributors.TryGetValue(pair.Key, out var priorPack) | ||
| && !JsonEqual(merged[pair.Key], pair.Value)) | ||
| { | ||
| throw Error( | ||
| "same-tier-conflict", | ||
| $"packs '{priorPack}' and '{pack.Name}' define default '{pair.Key}' differently at the same specificity tier", | ||
| [priorPack, pack.Name], pair.Key); | ||
| } | ||
|
|
||
| scalarContributors[pair.Key] = pack.Name; | ||
| var overrode = owners.TryGetValue(pair.Key, out var previous) | ||
| ? [.. previous.Overrode, previous.Pack] | ||
| : new List<string>(); | ||
| merged[pair.Key] = pair.Value.Clone(); | ||
| owners[pair.Key] = (pack.Name, overrode); | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
issue (blocking): a key handled as a keyed collection and as a scalar in the same tier silently loses data.
DefaultMergeKeys is per-pack. Two packs at the same specificity tier can disagree about whether one key is a keyed collection.
Take key K and two packs at the same tier:
- Pack A declares
DefaultMergeKeys["K"]and an array value. Line 173 routes toMergeCollection, which writesmerged["K"]andowners["K"]. It never writesscalarContributors["K"]. - Pack B does not declare a merge key for
K. Line 177 finds no entry inscalarContributors, so the same-tier conflict check is skipped. Line 190 overwrites the merged array with B's scalar.
The reverse order also loses data. If B runs first, MergeCollection reads merged["K"], sees a non-array at Line 213, starts from an empty entry set, and replaces the scalar with an array.
Neither direction raises same-tier-conflict, so a real catalog conflict resolves by iteration order. Track the key shape per tier and raise same-tier-conflict when the shapes disagree.
Related: Line 171 requires ValueKind == JsonValueKind.Array before using the merge key. If a pack declares a merge key but supplies a non-array value, the code silently falls back to scalar handling. Prefer an explicit error for that malformed input.
🐛 Sketch of a shape guard
var scalarContributors = new Dictionary<string, string>(StringComparer.Ordinal);
+ var keyShapes = new Dictionary<string, string>(StringComparer.Ordinal);
foreach (var pack in tierGroup)
{
if (pack.Defaults is null) continue;
foreach (var pair in pack.Defaults)
{
var mergeKey = pack.DefaultMergeKeys?.GetValueOrDefault(pair.Key);
+ var shape = mergeKey is not null ? "collection" : "scalar";
+ if (keyShapes.TryGetValue(pair.Key, out var priorShape) && priorShape != shape)
+ {
+ throw Error(
+ "same-tier-conflict",
+ $"packs disagree on the shape of default '{pair.Key}' at the same specificity tier ('{priorShape}' vs '{shape}')",
+ [pack.Name], pair.Key);
+ }
+
+ keyShapes[pair.Key] = shape;
if (mergeKey is not null && pair.Value.ValueKind == JsonValueKind.Array)🤖 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/hull-dotnet/Resolution/PackResolver.cs` around lines 168 - 193,
Update the same-tier default resolution around the foreach over pack.Defaults to
track each key’s shape (keyed collection versus scalar) independently of
scalarContributors. Raise same-tier-conflict when another pack uses the opposite
shape, regardless of iteration order, while preserving existing collection
merging and scalar conflict behavior. Also validate that any key declared in
DefaultMergeKeys has an array value; otherwise raise an explicit malformed-input
error instead of falling back to scalar handling.
| private static bool Satisfies(string version, string constraint) | ||
| { | ||
| if (!TryParseVersion(version, out var actual)) return false; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
question (non-blocking): an unparseable pack version reports as unsatisfiable, not as malformed.
Line 253 returns false when the actual version does not parse. Line 265 throws version-unsatisfiable when the constraint does not parse. The two malformed inputs therefore behave differently.
TryParseVersion accepts exactly three dot-separated integers. A pack version such as 1.2.0-rc1 fails to parse. CheckVersions then reports that the pack "requires X, but the catalog has X 1.2.0-rc1", which points at the constraint rather than at the unsupported version format. The PR description mentions a pinned preview SDK, so prerelease version strings are plausible in this catalog.
Confirm whether prerelease versions must be supported. If they must not be, raise an explicit parse error instead of returning false.
🤖 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/hull-dotnet/Resolution/PackResolver.cs` around lines 251 - 253,
Update Satisfies to distinguish an unparseable pack version from a valid version
that fails the constraint: when TryParseVersion cannot parse version, raise the
established explicit version-parse/malformed error instead of returning false.
Preserve the existing false result for valid versions that do not satisfy
constraint, and keep constraint parse failures handled as currently.
| private bool _degraded; | ||
| private bool _failNextInvoke; | ||
| private readonly List<string> _cancelledJobs = []; | ||
|
|
||
| public IReadOnlyList<string> CancelledJobs => _cancelledJobs; | ||
| public void SetDegraded(bool degraded) => _degraded = degraded; | ||
| public void SetFailNextInvoke(bool fail) => _failNextInvoke = fail; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
issue: ReferenceImageRuntime holds unsynchronized mutable state behind an asynchronous interface.
Cancel calls _cancelledJobs.Add with no synchronization. InProcessTransport.CancelAsync exposes this through an async API, so two callers can reach Add concurrently. Concurrent Add on List<T> can corrupt the internal array or throw. CancelledJobs also returns the live list, so a caller enumerating it during a concurrent Cancel gets an InvalidOperationException.
This type ships in the package rather than in the tests project, and HullShell.CreateReference() wires it up, so the reachable surface is wider than a test double.
🔒️ Proposed fix
- private readonly List<string> _cancelledJobs = [];
+ private readonly System.Collections.Concurrent.ConcurrentQueue<string> _cancelledJobs = new();
- public IReadOnlyList<string> CancelledJobs => _cancelledJobs;
+ public IReadOnlyList<string> CancelledJobs => _cancelledJobs.ToArray();- public void Cancel(CancelRequest request) => _cancelledJobs.Add(request.JobId);
+ public void Cancel(CancelRequest request) => _cancelledJobs.Enqueue(request.JobId);The change touches concurrency, which the review policy treats as a control-point area. Route this to an independent reviewer rather than resolving it by self-assertion.
As per 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 `@packages/hull-dotnet/Runtime/ReferenceImageRuntime.cs` around lines 11 - 17,
Synchronize all mutable state in ReferenceImageRuntime, especially
_cancelledJobs updates performed by Cancel and reads exposed through
CancelledJobs, so concurrent InProcessTransport.CancelAsync calls and
enumeration are safe. Return a snapshot or otherwise prevent callers from
observing the live list, while preserving the existing API behavior. Because
this is a concurrency control-point change, flag it for independent review
rather than self-assertion.
Source: Path instructions
| public Task<RuntimeManifest> AnnounceAsync(CancellationToken cancellationToken = default) => | ||
| Task.FromResult(host.Announce()); | ||
| public Task<NegotiateOffer> NegotiateOfferAsync(CancellationToken cancellationToken = default) => | ||
| Task.FromResult(host.NegotiateOffer()); | ||
| public Task<HealthProbe> HealthAsync(string kind, CancellationToken cancellationToken = default) => | ||
| Task.FromResult(host.Health(kind)); | ||
| public Task<CapabilityResult> InvokeAsync(InvokeRequest request, CancellationToken cancellationToken = default) => | ||
| Task.FromResult(host.Invoke(request)); | ||
| public Task CancelAsync(CancelRequest request, CancellationToken cancellationToken = default) | ||
| { | ||
| host.Cancel(request); | ||
| return Task.CompletedTask; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
issue (non-blocking): InProcessTransport does not fully honor the IRuntimeTransport async contract, in two ways.
First, a host exception escapes synchronously. If host.Invoke(request) throws, the exception leaves InvokeAsync before a Task is returned. LocalSubprocessTransport instead returns a faulted task for its failures. A caller that separates task creation from await observes different behavior from the two implementations of the same interface.
Second, cancellationToken is accepted and discarded in all five methods. An already-cancelled token still runs the full operation.
🐛 Proposed fix
public Task<CapabilityResult> InvokeAsync(InvokeRequest request, CancellationToken cancellationToken = default)
- Task.FromResult(host.Invoke(request));
+{
+ if (cancellationToken.IsCancellationRequested)
+ {
+ return Task.FromCanceled<CapabilityResult>(cancellationToken);
+ }
+
+ try
+ {
+ return Task.FromResult(host.Invoke(request));
+ }
+ catch (Exception ex)
+ {
+ return Task.FromException<CapabilityResult>(ex);
+ }
+}Apply the same shape to AnnounceAsync, NegotiateOfferAsync, HealthAsync, and CancelAsync. A small private helper that wraps a Func<T> will keep this from becoming five near-identical blocks.
🤖 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/hull-dotnet/Runtime/RuntimeHost.cs` around lines 20 - 32, Update
RuntimeHost’s five async methods—AnnounceAsync, NegotiateOfferAsync,
HealthAsync, InvokeAsync, and CancelAsync—to honor cancellationToken before
invoking the host and to return faulted tasks when host operations throw instead
of throwing synchronously. Add and reuse a private generic helper for
synchronous Func<T> operations, with equivalent handling for the void Cancel
operation.
| private readonly Dictionary<string, RuntimeConnection> _connections = new(StringComparer.Ordinal); | ||
|
|
||
| public CompositionManifest Composition => options.Composition; | ||
| public IReadOnlyList<RuntimeConnection> Runtimes => _connections.Values.ToArray(); | ||
|
|
||
| public async Task<RuntimeConnection> ConnectAsync( | ||
| IRuntimeTransport transport, | ||
| CancellationToken cancellationToken = default) | ||
| { | ||
| var connection = await RuntimeConnectionLifecycle.ConnectAsync( | ||
| transport, options.NegotiationProfile, cancellationToken).ConfigureAwait(false); | ||
| _connections[connection.RuntimeId] = connection; | ||
| return connection; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
issue: _connections is mutated without synchronization.
ConnectAsync is a public async method that writes to a plain Dictionary at Line 26. Runtimes, ProbeAsync, Resolve, and InvokeAsync read the same instance. If a caller connects two runtimes concurrently, or invokes while a connect completes, the concurrent write plus read can throw InvalidOperationException or leave the dictionary in a corrupt state.
HullShell is the shell entry point, so callers will hold one instance and use it from several request paths. Use ConcurrentDictionary<string, RuntimeConnection>, or document that the type is not thread-safe.
🔒️ Proposed fix
- private readonly Dictionary<string, RuntimeConnection> _connections = new(StringComparer.Ordinal);
+ private readonly ConcurrentDictionary<string, RuntimeConnection> _connections = new(StringComparer.Ordinal);Add the import:
+using System.Collections.Concurrent;
using Shipyard.Hull.Contracts;📝 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.
| private readonly Dictionary<string, RuntimeConnection> _connections = new(StringComparer.Ordinal); | |
| public CompositionManifest Composition => options.Composition; | |
| public IReadOnlyList<RuntimeConnection> Runtimes => _connections.Values.ToArray(); | |
| public async Task<RuntimeConnection> ConnectAsync( | |
| IRuntimeTransport transport, | |
| CancellationToken cancellationToken = default) | |
| { | |
| var connection = await RuntimeConnectionLifecycle.ConnectAsync( | |
| transport, options.NegotiationProfile, cancellationToken).ConfigureAwait(false); | |
| _connections[connection.RuntimeId] = connection; | |
| return connection; | |
| } | |
| using System.Collections.Concurrent; | |
| using Shipyard.Hull.Contracts; | |
| private readonly ConcurrentDictionary<string, RuntimeConnection> _connections = new(StringComparer.Ordinal); | |
| public CompositionManifest Composition => options.Composition; | |
| public IReadOnlyList<RuntimeConnection> Runtimes => _connections.Values.ToArray(); | |
| public async Task<RuntimeConnection> ConnectAsync( | |
| IRuntimeTransport transport, | |
| CancellationToken cancellationToken = default) | |
| { | |
| var connection = await RuntimeConnectionLifecycle.ConnectAsync( | |
| transport, options.NegotiationProfile, cancellationToken).ConfigureAwait(false); | |
| _connections[connection.RuntimeId] = connection; | |
| return connection; | |
| } |
🤖 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/hull-dotnet/Shell/HullShell.cs` around lines 15 - 28, Replace the
plain _connections Dictionary in HullShell with a thread-safe
ConcurrentDictionary, adding the required namespace import, while preserving the
existing ordinal key comparison and public Runtimes behavior. Ensure
ConnectAsync continues to publish connections through the concurrent collection
so concurrent ConnectAsync, reads, ProbeAsync, Resolve, and InvokeAsync calls
are safe.
| var resolution = Resolve(request.CapabilityId); | ||
| if (resolution.ChosenProviderId is null) | ||
| { | ||
| return new CapabilityResult( | ||
| $"job:unresolved:{request.CorrelationId}", | ||
| "failed", 0, Array.Empty<Artifact>(), new Usage("call", 0, null, resolution.Tier), | ||
| new CapabilityError("membrane", false, $"membrane.resolution_{resolution.ResolutionState.Replace('-', '_')}", resolution.Reason, null)); | ||
| } | ||
|
|
||
| var connection = _connections.Values.FirstOrDefault(item => | ||
| item.Registry.ByCapability.ContainsKey(request.CapabilityId) | ||
| && item.Negotiation.AcceptedCapabilities.Contains(request.CapabilityId, StringComparer.Ordinal)); | ||
| if (connection is null) | ||
| { | ||
| return new CapabilityResult( | ||
| $"job:no-runtime:{request.CorrelationId}", | ||
| "failed", 0, Array.Empty<Artifact>(), new Usage("call", 0, null, resolution.Tier), | ||
| new CapabilityError("membrane", false, "membrane.no_connected_runtime", $"no connected runtime hosts '{request.CapabilityId}'", null)); | ||
| } | ||
|
|
||
| var effectiveContext = context ?? options.InvokeContext; | ||
| return await RuntimeConnectionLifecycle.InvokeAsync( | ||
| connection, request, effectiveContext, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether InvokeRequest or the transport carries a provider id, and how TS binds the resolved provider.
set -euo pipefail
fd -t f -e cs . packages/hull-dotnet --exec rg -n -C3 'record InvokeRequest|ChosenProviderId|ProviderCandidate' {}
fd -t d 'resolution' | while IFS= read -r dir; do
echo "=== $dir ==="
rg -n -C3 'chosenProviderId|providerId' "$dir"
doneRepository: Harborline-Software/shipyard
Length of output: 4360
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files matching relevant names:"
fd -t f 'HullShell|InvokeRequest|RuntimeConnectionLifecycle|ResolutionResult|ProviderCandidate|CapabilityRequest|InvokeRequest' . packages/hull-dotnet packages packages/hull-dotnet --max-depth 4 | sed -n '1,160p'
echo
echo "HullShell.cs outline:"
ast-grep outline packages/hull-dotnet/Shell/HullShell.cs --view compact 2>/dev/null || true
echo
echo "HullShell.cs relevant section:"
sed -n '1,140p' packages/hull-dotnet/Shell/HullShell.cs | cat -n
echo
echo "Resolution pipeline candidates:"
sed -n '180,225p' packages/hull-dotnet/Shell/ResolutionPipeline.cs | cat -n
echo
echo "Search for Resolve results and InvokeRequest usage:"
rg -n -C3 'Resolve\(|ChosenProviderId|InvokeAsync\(|InvokeRequest\(|ProviderId|providerId|ProviderId' packages/hull-dotnet packages -g '*.cs' | sed -n '1,260p'Repository: Harborline-Software/shipyard
Length of output: 6620
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find ResolutionPipeline.cs:"
fd -t f 'ResolutionPipeline\.cs|RuntimeConnectionLifecycle\.cs|InvokeRequest\.cs|ProviderCandidate\.cs|Resolve.*\.cs' . -d 6 | sed -n '1,120p'
echo
echo "List packages top-level directory names:"
find packages -maxdepth 2 -type d | sort | sed -n '1,160p'
echo
echo "Search provider/chosen provider identifiers:"
rg -n -C3 'record ProviderCandidate|record ResolutionResult|ChosenProviderId|ProviderId|Choose\(|InvokeRequest\(' . --glob '*.cs' | sed -n '1,260p'Repository: Harborline-Software/shipyard
Length of output: 36127
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate Resolve provider selection implementation references:"
rg -n -C4 'DefaultConfigurationResolver|Choose\(.*IReadOnlyList<ProviderCandidate>|ProviderLicense|license|Hardware|HardwareResolver|StaticHardwareResolver' packages/hull-dotnet/Resolution packages/hull-dotnet/Shell packages/hull-dotnet/Contracts packages/hull-dotnet/tests --glob '*.cs' | sed -n '1,260p'
echo
echo "Inspect ProviderCandidate wire mapping:"
sed -n '60,105p' packages/hull-dotnet/Resolution/Pipeline.cs | cat -n
rg -n -C4 'class ProviderCandidate|sealed record ProviderCandidate|ProviderCandidateWire|ProviderCandidateJson' packages/hull-dotnet/Contracts packages/hull-dotnet/Resolution --glob '*.cs' | sed -n '1,220p'
echo
echo "Inspect Resolve result consumers/usages in hull-dotnet:"
rg -n -C3 'Resolve\(|ChosenProviderId|InvokeAsync\(|RuntimeConnectionLifecycle\.InvokeAsync|InvokeRequest\(' packages/hull-dotnet --glob '*.cs' | sed -n '1,260p'Repository: Harborline-Software/shipyard
Length of output: 23212
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Search for BankImport Core model / provider ID semantics:"
rg -n -C4 'BankImportCore|BankImportSource|ProviderId|providerId|FeedSource|FileSource|Invoked' packages -g '*.cs' | sed -n '1,260p'
echo
echo "Locate RuntimeConnectionLifecycle and RuntimeHost implementation:"
fd -t f 'RuntimeConnectionLifecycle\.cs|RuntimeHost\.cs|Invoke\.cs' packages/hull-dotnet | sort
echo
echo "RuntimeConnectionLifecycle.cs:"
cat -n packages/hull-dotnet/Membrane/RuntimeConnectionLifecycle.cs
echo
echo "RuntimeHost.cs:"
cat -n packages/hull-dotnet/Runtime/RuntimeHost.cs
echo
echo "Runtime lifecycle protocol usage around Invoke:"
cat -n packages/contracts/Generated/CarrierProtocol.g.cs | sed -n '1840,1900p'
rg -n -C4 'HullHostInvokeRequest|HullInvokeRequest|Choose|ProviderId|capabilityId' packages/contracts packages/hull-dotnet --glob '*.cs' | sed -n '1,260p'Repository: Harborline-Software/shipyard
Length of output: 26230
🌐 Web query:
Shipyard Hull dotnet ProviderCandidate Wire mapping ProviderCandidate Wire
💡 Result:
The term "ProviderCandidate" appears in two distinct contexts related to.NET and unrelated software architectures, while "Shipyard" and "Wire mapping" do not share a single common technical implementation in this context. 1..NET Response Compression: In the ASP.NET Core ResponseCompression middleware, ProviderCandidate is an internal class (defined in ResponseCompressionProvider.cs) used to represent a compression provider that is a candidate for handling a request based on the client's Accept-Encoding header [1][2]. It encapsulates the provider, its encoding name (e.g., "gzip", "br"), quality score, and priority [1][3]. It is not related to a "Shipyard" product or "wire mapping" in this context. 2. Unrelated Software Architectures: - There are various projects named "Shipyard" in the.NET ecosystem (e.g., a tool for packaging.NET apps on Linux [4], an IPFS client library [5], and a framework-agnostic UI tool [6]), but none of these standard repositories feature a "ProviderCandidate" class or "wire mapping" functionality. - The class ProviderCandidate exists in unrelated projects, such as the Rust sourceright crate (where it handles academic provider metadata) [7][8] and the omegon routing logic (where it handles model routing for LLM providers) [9][10]. These are also unrelated to.NET "wire mapping." If you are encountering a "wire mapping" error or configuration involving a "Shipyard" component, it is likely a highly specific or proprietary implementation, or a conflation of these separate concepts. There is no standard.NET library or "Shipyard" component that performs "wire mapping" using a "ProviderCandidate."
Citations:
- 1: https://github.com/dotnet/aspnetcore/blob/main/src/Middleware/ResponseCompression/src/ResponseCompressionProvider.cs
- 2: https://source.dot.net/Microsoft.AspNetCore.ResponseCompression/ResponseCompressionProvider.cs.html
- 3: https://github.com/dotnet/aspnetcore/blob/v3.1.6/src/Middleware/ResponseCompression/src/ResponseCompressionProvider.cs
- 4: https://github.com/CorruptComputer/Shipyard
- 5: https://github.com/ipfs-shipyard/net-ipfs-http-client/
- 6: https://github.com/Harborline-Software/shipyard
- 7: https://docs.rs/sourceright/latest/sourceright/sidecar/struct.ProviderCandidate.html
- 8: https://docs.rs/sourceright/latest/src/sourceright/sidecar.rs.html
- 9: https://github.com/styrene-lab/omegon/blob/main/core/crates/omegon/src/routing.rs
- 10: https://github.com/styrene-lab/omegon/blob/main/core/crates/omegon/src/route.rs
issue [blocking]: use resolution.ChosenProviderId at invoke time.
Resolve() selects a provider after entitlement, hardware, licensing, and configuration checks and stores the result in resolution.ChosenProviderId, while InvokeAsync() only checks that the result is not null. The method then selects the first compatible connection and forwards request unchanged, so requests can run on a different provider than the one selected by the resolution pipeline.
Forward the chosen provider id through the invoke path, or select the specific runtime whose announced providers include resolution.ChosenProviderId.
🤖 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/hull-dotnet/Shell/HullShell.cs` around lines 76 - 98, Update the
invocation flow in HullShell’s resolution handling to honor
resolution.ChosenProviderId after Resolve(request.CapabilityId) succeeds. Select
the connection whose announced providers include that exact chosen provider, or
forward the chosen provider through RuntimeConnectionLifecycle.InvokeAsync, and
do not fall back to an arbitrary compatible capability connection.
| var result = await shell.InvokeAsync(new InvokeRequest( | ||
| CapabilityIds.BankImport, | ||
| new BankImportCore( | ||
| new BankImportFileSource("a", "s.csv"), | ||
| "acct", | ||
| null, | ||
| 1000), | ||
| new Dictionary<string, System.Text.Json.JsonElement>(), | ||
| [new RequestAttachment("a", "text/csv", "s.csv", null, null)], | ||
| "idem-bank", | ||
| "corr-bank-na", | ||
| "sync")); | ||
|
|
||
| Assert.Equal("failed", result.Status); | ||
| Assert.Equal("membrane", result.Error?.FaultDomain); | ||
| Assert.Equal("membrane.resolution_not_in_edition", result.Error?.Code); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
issue: the SEC-2 idempotency guard has no test that exercises the throwing branch.
This test passes "idem-bank" as the idempotency key, so Invoke.AssertIdempotencyKey never throws. IdempotencyKeyRequiredException is the fail-closed control for a financial capability, and no test in the suite reaches it. Two other control paths are also untested: Negotiate.Reconcile returning Compatible == false, and redaction through RedactingLogSink.
The idempotency guard and the redaction control are both control points. Please route them to an independent reviewer rather than asserting the behavior in the PR description.
💚 Proposed additional test
+ [Fact]
+ public void Blank_idempotency_key_fails_closed_for_financial_capability()
+ {
+ var request = new InvokeRequest(
+ CapabilityIds.BankImport,
+ new BankImportCore(new BankImportFileSource("a", "s.csv"), "acct", null, 1000),
+ new Dictionary<string, System.Text.Json.JsonElement>(),
+ [new RequestAttachment("a", "text/csv", "s.csv", null, null)],
+ " ",
+ "corr-bank-blank",
+ "sync");
+
+ Assert.Throws<IdempotencyKeyRequiredException>(() => Invoke.AssertIdempotencyKey(request));
+ }As per path instructions, "the change's CI suite must actually exercise it (A1)" and "Flag CP-touching changes (financial/audit/security/compliance/concurrency) as needing an independent reviewer, not self-assertion (A2)".
📝 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.
| var result = await shell.InvokeAsync(new InvokeRequest( | |
| CapabilityIds.BankImport, | |
| new BankImportCore( | |
| new BankImportFileSource("a", "s.csv"), | |
| "acct", | |
| null, | |
| 1000), | |
| new Dictionary<string, System.Text.Json.JsonElement>(), | |
| [new RequestAttachment("a", "text/csv", "s.csv", null, null)], | |
| "idem-bank", | |
| "corr-bank-na", | |
| "sync")); | |
| Assert.Equal("failed", result.Status); | |
| Assert.Equal("membrane", result.Error?.FaultDomain); | |
| Assert.Equal("membrane.resolution_not_in_edition", result.Error?.Code); | |
| } | |
| var result = await shell.InvokeAsync(new InvokeRequest( | |
| CapabilityIds.BankImport, | |
| new BankImportCore( | |
| new BankImportFileSource("a", "s.csv"), | |
| "acct", | |
| null, | |
| 1000), | |
| new Dictionary<string, System.Text.Json.JsonElement>(), | |
| [new RequestAttachment("a", "text/csv", "s.csv", null, null)], | |
| "idem-bank", | |
| "corr-bank-na", | |
| "sync")); | |
| Assert.Equal("failed", result.Status); | |
| Assert.Equal("membrane", result.Error?.FaultDomain); | |
| Assert.Equal("membrane.resolution_not_in_edition", result.Error?.Code); | |
| } | |
| [Fact] | |
| public void Blank_idempotency_key_fails_closed_for_financial_capability() | |
| { | |
| var request = new InvokeRequest( | |
| CapabilityIds.BankImport, | |
| new BankImportCore(new BankImportFileSource("a", "s.csv"), "acct", null, 1000), | |
| new Dictionary<string, System.Text.Json.JsonElement>(), | |
| [new RequestAttachment("a", "text/csv", "s.csv", null, null)], | |
| " ", | |
| "corr-bank-blank", | |
| "sync"); | |
| Assert.Throws<IdempotencyKeyRequiredException>(() => Invoke.AssertIdempotencyKey(request)); | |
| } |
🤖 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/hull-dotnet/tests/RoundTripTests.cs` around lines 87 - 103, Extend
the round-trip test coverage around the existing bank-import test to exercise
the SEC-2 throwing path by omitting or invalidating the idempotency key and
asserting the fail-closed IdempotencyKeyRequiredException behavior. Add focused
tests for Negotiate.Reconcile returning Compatible == false and RedactingLogSink
redacting sensitive values, using the existing test infrastructure. Mark these
control-point changes for independent review rather than relying on
self-assertion.
Source: Path instructions
Align the .NET membrane with the TypeScript reference and add load-bearing tests.
Apply the security review selectors to the .NET Hull parity surface.
github-actions
Bot
added
the
needs-deep-review
…or taxonomy closed Merged collection defaults now emit in insertion order matching the TS Map semantics; the duplicate-announcement refusal becomes a membrane-local typed exception so CompositionError keeps exactly the five compose-time reasons; the range-grammar comment states the narrowing honestly; the secret log assertion binds the substring predicate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ctwoodwa
added
the
deep-review-passed
github-actions
Bot
removed
the
needs-deep-review
…er pin Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
In plain terms: the .NET twin of the hull capability membrane - the five built faces plus the resolution pipeline, with the round-trip suite ported.
Why it matters: W3 of the .NET edition (ADR 0167 D3); the parity pair that keeps the .NET shell able to compose capabilities.
Done when: merged after code review. Round-trip suite 5/5 green on net11.0 (verified with the pinned preview SDK); local-subprocess arm fails closed until a sandbox is wired.
Refs: #3652
🤖 Generated with Claude Code
Summary by CodeRabbit
HullShellorchestration API for connecting runtimes, resolving providers, probing health, and invoking capabilities.