test(hull): add the cross-lane ADR 0103 conformance vectors and divergence registry - #3673
Conversation
github-actions
Bot
added
the
needs-deep-review
|
Warning Review limit reached
Next review available in: 35 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 (5)
📝 WalkthroughWalkthroughAdds shared Hull conformance vectors and a divergence registry. Adds TypeScript and .NET suites that execute normalization, redaction, resolution, pack composition, announcements, and invocation logging cases. ChangesHull conformance enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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: 6
🧹 Nitpick comments (5)
apps/hull/src/conformance/hull-conformance.test.ts (2)
44-46: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winsuggestion: the two lanes use different equality definitions for the same fixtures.
equalcomparesJSON.stringifyoutput, which is key-order sensitive.packages/hull-dotnet/tests/ConformanceVectorTests.csline 247 usesJsonElement.DeepEquals, which ignores object member order. A property-order change innormalizeToEnvelopewould fail this lane and pass the .NET lane, and the failure would be reported as an undeclared divergence.Parity must be evaluated against one common definition. Compare structurally after sorting keys.
♻️ Proposed order-insensitive comparison
+function canonical(value: unknown): unknown { + if (Array.isArray(value)) return value.map(canonical) + if (value !== null && typeof value === 'object') { + return Object.fromEntries( + Object.entries(value) + .sort(([a], [b]) => (a < b ? -1 : a > b ? 1 : 0)) + .map(([k, v]) => [k, canonical(v)]), + ) + } + return value +} + function equal(left: unknown, right: unknown): boolean { - return JSON.stringify(left) === JSON.stringify(right) + return JSON.stringify(canonical(left)) === JSON.stringify(canonical(right)) }🤖 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/conformance/hull-conformance.test.ts` around lines 44 - 46, Update the equal function to perform order-insensitive structural comparison by recursively sorting object keys before comparing values, matching JsonElement.DeepEquals semantics while preserving array ordering and primitive comparisons.
201-211: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: an unknown surface silently runs the announce path.
The ternary chain treats every non-
m3, non-redactionvector as announce.runAnnouncecatches all errors and returns athrowsshape, so a vector with a new surface produces a confusing expectation mismatch instead of a clear unsupported-surface failure.RunAsyncin the .NET harness throwsInvalidOperationExceptionfor the same case. Match that behavior.♻️ Proposed explicit dispatch
- const actual = candidate.surface === 'm3' - ? runM3(candidate.input) - : candidate.surface === 'redaction' - ? runRedaction(candidate.input, candidate.operation) - : runAnnounce(candidate.input) + let actual: unknown + if (candidate.surface === 'm3') actual = runM3(candidate.input) + else if (candidate.surface === 'redaction') actual = runRedaction(candidate.input, candidate.operation) + else if (candidate.surface === 'announce') actual = runAnnounce(candidate.input) + else throw new Error(`Unsupported conformance vector: ${candidate.surface}/${candidate.operation}`)🤖 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/conformance/hull-conformance.test.ts` around lines 201 - 211, Update the test dispatch in the parameterized conformance case to explicitly support only the known surfaces: call runM3 for “m3”, runRedaction for “redaction”, and runAnnounce for “announce”; otherwise throw an unsupported-surface error matching the .NET harness behavior instead of defaulting to runAnnounce.packages/hull-dotnet/tests/ConformanceVectorTests.cs (3)
92-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winquestion: should the redaction vectors compare different seams in each lane?
This lane routes the payload through
RedactingLogSink(packages/hull-dotnet/Membrane/Observe.cslines 26-38), which callsRedaction.RedactJson. The TypeScript lane callsredactDeepdirectly. A future change to either sink wrapper would move one lane without moving the other, and the vector would report a lane divergence that is really a seam difference.Two smaller points. The
LogRecordcorrelation id on line 103 is hardcoded to"corr-103", which matches the current fixture value by coincidence; derive it from the vector or use an unrelated value. Line 104 passes"ok"as the message forredact-deep, so the message path is unexercised for that operation.Redaction is a compliance-critical surface. These expectations should carry an independent reviewer sign-off rather than a 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/tests/ConformanceVectorTests.cs` around lines 92 - 110, Update RunRedaction so the .NET conformance vectors exercise the same underlying redaction seam as the TypeScript redactDeep lane, rather than routing through RedactingLogSink; also derive the LogRecord correlation ID from the vector or use an unrelated value, and pass the vector’s actual text/value into the message for redact-deep so that path is covered. Mark these compliance-sensitive expectations as requiring independent reviewer sign-off rather than self-assertion.Source: Path instructions
269-272: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: fail loudly when a required vector property is missing.
Stringreturnsstring.Emptyfor an absent property whenever no fallback is given. A fixture typo instatus,jobId, orfaultDomainthen produces a wrong result rather than a clear error. For example,String(native, "status")returning""feeds an empty status intoInvoke.NormalizeToEnvelope, which changes fail-closed normalization semantics.Make the empty fallback explicit at the call sites that want it, and throw otherwise.
♻️ Proposed strict accessor
private static string String(JsonElement value, string property, string? fallback = null) => value.TryGetProperty(property, out var result) && result.ValueKind != JsonValueKind.Null ? result.GetString()! - : fallback ?? string.Empty; + : fallback ?? throw new InvalidOperationException($"Conformance vector is missing required property '{property}'.");🤖 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/ConformanceVectorTests.cs` around lines 269 - 272, Update the String helper in ConformanceVectorTests so it throws when the requested property is missing or null and no fallback is supplied, instead of implicitly returning string.Empty. Review each String call site, including status, jobId, and faultDomain, and pass an explicit empty-string fallback wherever that behavior is intentional.
17-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: run each vector as its own test case.
All 24 vectors execute inside one
foreachin a single[Fact]. The first mismatch throws and the remaining vectors never run, so one break hides every other break. The TypeScript lane already reports per-vector cases throughit.each.This also affects the stated completion criteria. This file defines 2 test cases, while the PR objectives report hull-dotnet 15/15. Confirm which suite produces the 15 results, or correct the criterion.
♻️ Proposed per-vector theory
- [Fact] - public async Task Shared_vectors_match_canonical_expectation_or_declared_dotnet_variant() - { - var vectors = LoadArray("vectors.json"); - var divergences = LoadObject("divergences.json"); - - foreach (var vector in vectors.EnumerateArray()) - { - var actual = await RunAsync(vector); - AssertConformant(vector, actual, divergences); - } - } + public static TheoryData<string> VectorIds() + { + var data = new TheoryData<string>(); + foreach (var vector in LoadArray("vectors.json").EnumerateArray()) + { + data.Add(vector.GetProperty("id").GetString()!); + } + + return data; + } + + [Theory] + [MemberData(nameof(VectorIds))] + public async Task Shared_vector_matches_canonical_expectation_or_declared_dotnet_variant(string id) + { + var vector = LoadArray("vectors.json").EnumerateArray() + .Single(candidate => candidate.GetProperty("id").GetString() == id); + var actual = await RunAsync(vector); + AssertConformant(vector, actual, LoadObject("divergences.json")); + }As per path instructions: "the diff must match the PR's stated scope (A6)".
🤖 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/ConformanceVectorTests.cs` around lines 17 - 28, Replace the single Shared_vectors_match_canonical_expectation_or_declared_dotnet_variant [Fact] loop with per-vector test cases in ConformanceVectorTests so each vector runs independently and one failure does not stop the rest. Use the existing RunAsync and AssertConformant flow, but move the vector iteration into a theory-style per-case setup keyed by the vector identity from vectors.json. Also verify the completion metric against the actual test suite producing the 15 results, or update the PR criterion if this file is not the source of that count.Source: Path instructions
🤖 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 `@_shared/conformance/hull/divergences.json`:
- Around line 14-19: Remove the invalid pack-semver-strictness entry from the
divergence registry, since its expectedVariant matches the canonical vector
expectation. In the shared conformance canary used by both lanes, add an
assertion that each divergence entry’s expectedVariant is not equal to the
referenced vector’s canonical expected value, reusing the existing equality
helper and vector lookup.
In `@apps/hull/src/conformance/hull-conformance.test.ts`:
- Around line 180-187: Update the record extraction in the conformance test to
assert that invokeCapability produced exactly one record before accessing
records[0]. Use the test framework’s strict single-record assertion, matching
the .NET harness behavior for both zero and multiple records, then read message,
correlationId, runtimeId, and capabilityId from the asserted record.
- Around line 221-224: Update the test title in the invoke-log-principal test
case to use the candidate’s actual description instead of the literal
$description placeholder. Modify the test declaration around the
invoke-log-principal candidate while preserving its existing candidate creation
and assertion flow.
In `@packages/hull-dotnet/tests/ConformanceVectorTests.cs`:
- Around line 64-90: Update RunNormalize and the typed conversion around
CapabilityError so an omitted native error.retryable remains absent when passed
to Invoke.NormalizeToEnvelope, using bool? if the CLR model supports it; do not
convert absence to false in the harness. If the model cannot represent absence,
document that constraint in the divergence rationale, and add or update the CI
test so m3-normalize-missing-retryable exercises the production normalizer path.
- Around line 196-208: Update the RunInvokeLogAsync harness so the principal
attribution in the vector is represented consistently, either by binding the
supplied principalId into the InvokeContext path if that symbol supports it or
by revising the divergence rationale to explicitly state that InvokeContext only
carries Secrets and LogSink and cannot model the TypeScript principal field.
Keep the Invoke.InvokeCapabilityAsync call and the existing
ConformanceTransport/InvokeRequest setup unchanged, and make the asserted
behavior match the actual .NET context shape.
- Around line 38-46: Align lane validation across both canaries: in
packages/hull-dotnet/tests/ConformanceVectorTests.cs lines 38-46, replace the
hard-coded lane assertion in the LoadObject loop with membership validation for
dotnet or typescript. In apps/hull/src/conformance/hull-conformance.test.ts
lines 190-199, retain the two-value lane regex and add a guard preventing dotnet
entries from being consumed by assertConformant in the TypeScript lane. Ensure
both canaries continue treating the vector’s canonical expectation as
authoritative for the lane not named by each entry.
---
Nitpick comments:
In `@apps/hull/src/conformance/hull-conformance.test.ts`:
- Around line 44-46: Update the equal function to perform order-insensitive
structural comparison by recursively sorting object keys before comparing
values, matching JsonElement.DeepEquals semantics while preserving array
ordering and primitive comparisons.
- Around line 201-211: Update the test dispatch in the parameterized conformance
case to explicitly support only the known surfaces: call runM3 for “m3”,
runRedaction for “redaction”, and runAnnounce for “announce”; otherwise throw an
unsupported-surface error matching the .NET harness behavior instead of
defaulting to runAnnounce.
In `@packages/hull-dotnet/tests/ConformanceVectorTests.cs`:
- Around line 92-110: Update RunRedaction so the .NET conformance vectors
exercise the same underlying redaction seam as the TypeScript redactDeep lane,
rather than routing through RedactingLogSink; also derive the LogRecord
correlation ID from the vector or use an unrelated value, and pass the vector’s
actual text/value into the message for redact-deep so that path is covered. Mark
these compliance-sensitive expectations as requiring independent reviewer
sign-off rather than self-assertion.
- Around line 269-272: Update the String helper in ConformanceVectorTests so it
throws when the requested property is missing or null and no fallback is
supplied, instead of implicitly returning string.Empty. Review each String call
site, including status, jobId, and faultDomain, and pass an explicit
empty-string fallback wherever that behavior is intentional.
- Around line 17-28: Replace the single
Shared_vectors_match_canonical_expectation_or_declared_dotnet_variant [Fact]
loop with per-vector test cases in ConformanceVectorTests so each vector runs
independently and one failure does not stop the rest. Use the existing RunAsync
and AssertConformant flow, but move the vector iteration into a theory-style
per-case setup keyed by the vector identity from vectors.json. Also verify the
completion metric against the actual test suite producing the 15 results, or
update the PR criterion if this file is not the source of that count.
🪄 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: 9ac7d4fa-3cc1-4453-ac12-4b0538fe07d3
📒 Files selected for processing (4)
_shared/conformance/hull/divergences.json_shared/conformance/hull/vectors.jsonapps/hull/src/conformance/hull-conformance.test.tspackages/hull-dotnet/tests/ConformanceVectorTests.cs
| "pack-semver-strictness": { | ||
| "lane": "dotnet", | ||
| "expectedVariant": { "throws": "CompositionError", "reason": "version-unsatisfiable" }, | ||
| "rationale": "Both lanes deliberately reject prerelease, build-metadata, and wildcard forms; the registry records the strict numeric-triplet policy at the CLR boundary.", | ||
| "source": "README row: semver parsing strictness" | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
issue (non-blocking): pack-semver-strictness declares a divergence that does not exist.
The expectedVariant here is byte-identical to the canonical expectation in _shared/conformance/hull/vectors.json lines 355. AssertConformant matches the canonical branch first, so the variant branch is unreachable. The rationale confirms this: it states that both lanes reject the same forms.
The canary only checks that a divergence id maps to a vector. It cannot detect an entry whose variant equals the canonical expectation. That inverts the PR objective: the registry now records a lane difference that the vectors say is not a lane difference.
Remove the entry, and add a canary assertion that rejects a variant equal to the canonical expectation.
♻️ Proposed removal
- "pack-semver-strictness": {
- "lane": "dotnet",
- "expectedVariant": { "throws": "CompositionError", "reason": "version-unsatisfiable" },
- "rationale": "Both lanes deliberately reject prerelease, build-metadata, and wildcard forms; the registry records the strict numeric-triplet policy at the CLR boundary.",
- "source": "README row: semver parsing strictness"
- },Add the reverse-canary assertion in both lanes, for example in apps/hull/src/conformance/hull-conformance.test.ts:
expect(equal(entry.expectedVariant, vector(id).expected), `${id}: variant equals canonical expectation`).toBe(false)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 `@_shared/conformance/hull/divergences.json` around lines 14 - 19, Remove the
invalid pack-semver-strictness entry from the divergence registry, since its
expectedVariant matches the canonical vector expectation. In the shared
conformance canary used by both lanes, add an assertion that each divergence
entry’s expectedVariant is not equal to the referenced vector’s canonical
expected value, reusing the existing equality helper and vector lookup.
Source: Path instructions
| const record = records[0] | ||
| return { | ||
| message: record.message, | ||
| correlationId: record.correlationId, | ||
| runtimeId: record.runtimeId, | ||
| capabilityId: record.capabilityId, | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
nit: assert the record count before you read records[0].
If invokeCapability writes no record, record.message throws a TypeError and the real cause is lost. The .NET harness uses sink.Records.Single(), which fails clearly on both zero records and more than one. Match that strictness.
🐛 Proposed guard
- const record = records[0]
+ expect(records, 'invoke log emitted an unexpected number of records').toHaveLength(1)
+ const record = records[0]📝 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 record = records[0] | |
| return { | |
| message: record.message, | |
| correlationId: record.correlationId, | |
| runtimeId: record.runtimeId, | |
| capabilityId: record.capabilityId, | |
| } | |
| } | |
| expect(records, 'invoke log emitted an unexpected number of records').toHaveLength(1) | |
| const record = records[0] | |
| return { | |
| message: record.message, | |
| correlationId: record.correlationId, | |
| runtimeId: record.runtimeId, | |
| capabilityId: record.capabilityId, | |
| } | |
| } |
🤖 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/conformance/hull-conformance.test.ts` around lines 180 - 187,
Update the record extraction in the conformance test to assert that
invokeCapability produced exactly one record before accessing records[0]. Use
the test framework’s strict single-record assertion, matching the .NET harness
behavior for both zero and multiple records, then read message, correlationId,
runtimeId, and capabilityId from the asserted record.
| it('invoke-log-principal — $description', async () => { | ||
| const candidate = vector('invoke-log-principal') | ||
| assertConformant(candidate, await runInvokeLog(candidate.input)) | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
typo: $description is a literal here.
Interpolation applies only inside it.each. This test prints the raw title invoke-log-principal — $description.
📝 Proposed fix
- it('invoke-log-principal — $description', async () => {
- const candidate = vector('invoke-log-principal')
+ const invokeLogVector = vector('invoke-log-principal')
+ it(`invoke-log-principal — ${invokeLogVector.description}`, async () => {
+ const candidate = invokeLogVector📝 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.
| it('invoke-log-principal — $description', async () => { | |
| const candidate = vector('invoke-log-principal') | |
| assertConformant(candidate, await runInvokeLog(candidate.input)) | |
| }) | |
| const invokeLogVector = vector('invoke-log-principal') | |
| it(`invoke-log-principal — ${invokeLogVector.description}`, async () => { | |
| const candidate = invokeLogVector |
🤖 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/conformance/hull-conformance.test.ts` around lines 221 - 224,
Update the test title in the invoke-log-principal test case to use the
candidate’s actual description instead of the literal $description placeholder.
Modify the test declaration around the invoke-log-principal candidate while
preserving its existing candidate creation and assertion flow.
| private static JsonElement RunNormalize(JsonElement input) | ||
| { | ||
| var native = input.GetProperty("native"); | ||
| var error = native.TryGetProperty("error", out var nativeError) && nativeError.ValueKind != JsonValueKind.Null | ||
| ? new CapabilityError( | ||
| String(nativeError, "faultDomain"), | ||
| nativeError.TryGetProperty("retryable", out var retryable) && retryable.ValueKind == JsonValueKind.True, | ||
| String(nativeError, "code"), | ||
| String(nativeError, "message"), | ||
| null) | ||
| : null; | ||
| var usage = native.GetProperty("usage"); | ||
| var result = new CapabilityResult( | ||
| String(native, "jobId"), | ||
| String(native, "status"), | ||
| native.GetProperty("progress").GetDouble(), | ||
| [], | ||
| new Usage( | ||
| String(usage, "unit"), | ||
| usage.GetProperty("quantity").GetDouble(), | ||
| usage.GetProperty("costMicros").ValueKind == JsonValueKind.Null ? null : usage.GetProperty("costMicros").GetDouble(), | ||
| String(usage, "tier")), | ||
| error); | ||
|
|
||
| var normalized = Invoke.NormalizeToEnvelope(result, String(input, "fallbackJobId")); | ||
| return Serialize(normalized); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
issue (non-blocking): the harness decides the retryable default before the normalizer runs.
Line 70 evaluates TryGetProperty("retryable", out var retryable) && retryable.ValueKind == JsonValueKind.True. When the property is absent, TryGetProperty returns false and the expression yields false. The m3-normalize-missing-retryable vector (_shared/conformance/hull/vectors.json line 64) omits retryable exactly to test the default.
Invoke.NormalizeToEnvelope therefore never sees a missing value; it sees false, chosen here. The declared divergence records retryable: false as .NET behavior, but the harness produced that value, not the production normalizer. The registry rationale states "the supplied bool is retained", and no bool is supplied.
If the typed CLR model genuinely cannot represent an absent retryable, use bool? in the conversion or state the model constraint in the rationale. Do not let a harness default stand in for a lane divergence.
As per path instructions: "the change's CI suite must actually exercise it (A1)".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/hull-dotnet/tests/ConformanceVectorTests.cs` around lines 64 - 90,
Update RunNormalize and the typed conversion around CapabilityError so an
omitted native error.retryable remains absent when passed to
Invoke.NormalizeToEnvelope, using bool? if the CLR model supports it; do not
convert absence to false in the harness. If the model cannot represent absence,
document that constraint in the divergence rationale, and add or update the CI
test so m3-normalize-missing-retryable exercises the production normalizer path.
Source: Path instructions
| private static async Task<JsonElement> RunInvokeLogAsync(JsonElement input) | ||
| { | ||
| var sink = new InMemoryLogSink(); | ||
| var transport = new ConformanceTransport(String(input, "runtimeId")); | ||
| var request = new InvokeRequest( | ||
| String(input, "capabilityId"), | ||
| new ImageCore("conformance", new ImageSize(1, 1), 1, 1, "png", 1), | ||
| new Dictionary<string, JsonElement>(), | ||
| [], | ||
| "idem-conformance", | ||
| String(input, "correlationId"), | ||
| String(input, "transport")); | ||
| await Invoke.InvokeCapabilityAsync(transport, request, new InvokeContext(null, sink)); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Resolve the InvokeContext principal parameter type and any principal usage in the .NET invoke log path.
set -euo pipefail
fd -e cs . packages/hull-dotnet --exec rg -n -C4 'record\s+InvokeContext|class\s+InvokeContext|InvokeContext\('
fd -e cs . packages/hull-dotnet --exec rg -n -C4 'principal'Repository: Harborline-Software/shipyard
Length of output: 3299
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -e cs . packages/hull-dotnet
echo "== Invoke.cs outline =="
ast-grep outline packages/hull-dotnet/src/Invoke.cs --view expanded || true
echo "== relevant Invoke.cs lines =="
cat -n packages/hull-dotnet/src/Invoke.cs | sed -n '1,220p'
echo "== tests snippets =="
cat -n packages/hull-dotnet/tests/ConformanceVectorTests.cs | sed -n '170,225p'
cat -n packages/hull-dotnet/tests/InvokeTests.cs | sed -n '1,140p'
echo "== vector principal/idle/rationale relevant sections =="
python3 - <<'PY'
import json
from pathlib import Path
p=Path("_shared/conformance/hull/vectors.json")
data=json.loads(p.read_text())
for i,item in enumerate(data):
if i==361:
print(i+1, json.dumps(item, indent=2)[:3000])
PY
echo "== registry references for invoke log principal =="
rg -n -C 4 'invoke-log-principal|principal-7|principal attribution|InvokeContext|does not yet carry|not yet carry' .Repository: Harborline-Software/shipyard
Length of output: 1184
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== knowledge-base query =="
pnpm kb query "ConformanceVectorTests _shared/conformance/hull/vectors.json invoke-log-principal InvokeContext principal"Repository: Harborline-Software/shipyard
Length of output: 2934
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate InvokeContext and Invoke usage =="
rg -n -C 5 'sealed\s+record\s+InvokeContext|InvokeContext\s*\(|Invoke\InvokeCapabilityAsync|primary\(\)|InvokeContext' packages/hull-dotnet _shared/conformance packages -g '*.cs' -g '*.csproj' -g '*.json' -g '*.md' -g '*.yaml' -g '*.yml'
echo "== Invok.cs content if present =="
if [ -f packages/hull-dotnet/Membrane/Invoke.cs ]; then
cat -n packages/hull-dotnet/Membrane/Invoke.cs | sed -n '1,220p'
fi
echo "== ConformanceVectorTests lines 170-225 =="
cat -n packages/hull-dotnet/tests/ConformanceVectorTests.cs | sed -n '170,225p'
echo "== Vector entry 362 =="
python3 - <<'PY'
import json
from pathlib import Path
p=Path("_shared/conformance/hull/vectors.json")
data=json.loads(p.read_text())
for idx,item in enumerate(data, start=1):
if idx == 362:
print(json.dumps({idx,item}, indent=2)[:4000])
break
else:
print("entry 362 not found, len=", len(data))
PY
echo "== Registry/text references to principal in hull vectors or invoke log =="
rg -n -C 3 'invoke-log-principal|invoke-log.*principal|principal-7|principal attribution|InvokeContext.*principal|invoke-log' _shared/conformance/hull _shared -g '*.json' -g '*.md' -g '*.yaml' -g '*.yml' || trueRepository: Harborline-Software/shipyard
Length of output: 450
issue (blocking): bind principalId to the invoke context or update the divergence rationale
The vector supplies principalId: "principal-7", but this harness still passes new InvokeContext(null, sink). The .NET InvokeContext only exposes Secrets and LogSink, so this path cannot observe principal attribution; it only proves a null principal emits no principal= segment. If the rationale says the .NET wire model cannot carry the TypeScript principal attribution field, state that concrete type limitation.
🤖 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/ConformanceVectorTests.cs` around lines 196 - 208,
Update the RunInvokeLogAsync harness so the principal attribution in the vector
is represented consistently, either by binding the supplied principalId into the
InvokeContext path if that symbol supports it or by revising the divergence
rationale to explicitly state that InvokeContext only carries Secrets and
LogSink and cannot model the TypeScript principal field. Keep the
Invoke.InvokeCapabilityAsync call and the existing
ConformanceTransport/InvokeRequest setup unchanged, and make the asserted
behavior match the actual .NET context shape.
Source: Path instructions
ctwoodwa
added
the
deep-review-passed
github-actions
Bot
removed
the
needs-deep-review
1 participant
In plain terms: one shared set of test fixtures both the TypeScript and .NET implementations must pass, with a registry for the deliberately-different edge cases - drift is now conformant, declared, or red.
Why it matters: enforces the CIC projection principle (lanes are projections of common definitions); parity is checked against the definition, not the sibling lane.
Done when: merged. 24 vectors (announce/M3/redaction/resolution/pack-DAG), 6 declared .NET divergences with rationale, both runners green (hull 25/25, hull-dotnet 15/15), dead-entry canary in place.
Closes #3669
🤖 Generated with Claude Code
Summary by CodeRabbit