feat(contracts): add a gated machine-readable boundary inventory - #3539
Conversation
The protocol manifest enumerated COVERED operations only, so the uncovered Carrier surface was invisible rather than documented. That is what made "migrate incrementally" unverifiable — progress and completeness looked identical, and nobody could answer what was left without re-deriving it. The inventory is derived from the PRODUCERS — the Tauri command list, the Hull adapter faces and the production local-node route registrations — not from the schema. An inventory authored from the schema would restate what is already covered and prove nothing; the point is to name what is not. 232 boundaries: 19 covered, 213 deferred, each deferred entry carrying a reason. It is gated rather than decorative. A boundary appearing in neither the covered set nor the inventory fails the build, so the document cannot rot silently the next time somebody adds an endpoint. A stale inventory would be worse than none, because it reads as authoritative. Kept as a separate file rather than folded into the manifest: the manifest is the generator's input, and listing deferred operations there would present them to code generation as implemented contracts. No endpoint was migrated. That was explicitly out of scope and remains so. Proven by adding a fake command to the real handler list — the gate fails naming it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR adds runtime-boundary discovery and validation for Tauri, Hull, and local-node operations. It adds a checked-in inventory with covered, deferred, and blocked classifications, plus parser and integration tests. ChangesRuntime boundary inventory
Estimated code review effort: 4 (Complex) | ~45 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 |
…ing as coverage The walker matched only the five .MapGet/Post/Put/Delete/Patch helpers, so three live production boundaries were absent from an artifact published as complete — the PATCH maintenance route registered through MapMethods, the peer-to-peer sync WebSocket at /ws, and /health. It also counted ten routes from the sync and two-user dev harnesses under tools/, which the host excludes from compilation and which therefore never ship. Now discovered: MapMethods (one boundary per declared verb), MapWebSocketPath as its own kind, and MapHealthChecks. Any other .Map<Uppercase>( spelling is a hard error unless it is listed as a non-boundary with the reason it registers no route — an unknown registration form has to break the build rather than shrink the denominator in silence. The Tauri and Hull parsers had the same failure mode against plausible refactors. A command listed without a module path, a face declared without async, and a face declared below the private helper each matched nothing and vanished. Both parsers now enumerate structurally and refuse what they cannot parse. On the reasons field — 213 deferred entries carried one identical sentence, which read as 213 authored judgements and carried none. The blanket reason moves to a status-wide default, an entry may still override it, and restating the default is now an error. The three newly found boundaries carry specific reasons instead. The summary line reported "covers all 232 discovered operations" while 213 were deferred. It now prints the covered, deferred and blocked split. Net count 232 to 225. Proof — the three new forms and the unknown-form refusal are established by mutation. The gate goes red naming the missing boundary and the unclassified helper. Four new parser tests feed synthetic producers, and reverting each parser turns exactly its own test red.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
tooling/carrier-contract-codegen/boundary-inventory.mjs (4)
112-114: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuethought:
parseTauriCommandsaccepts the firstgenerate_handler![in the file, including one inside a comment.The regex at line 113 is non-greedy and unanchored. A commented-out or
#[cfg]-gatedgenerate_handler![...]earlier inlib.rswins, and the real list is never read. The result is a silently wrong command set, which is the exact fail-open class the header comment at lines 109-111 says these parsers replaced.Every other branch in this file is fail-closed. Rejecting a file that contains more than one
generate_handler![would keep that 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 `@tooling/carrier-contract-codegen/boundary-inventory.mjs` around lines 112 - 114, Update parseTauriCommands to detect all generate_handler![ occurrences and reject the source when more than one is present, rather than selecting the first match. Preserve the existing missing-handler error for zero matches and parse the command list only when exactly one occurrence is found.
83-88: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winsuggestion: the manifest cross-check does not detect two covered boundaries that claim the same
operationId.Lines 86-87 compare the two id lists by membership only. If two
coveredentries both carryhull.invoke, both checks pass, and a different manifest operation can still look covered because its id happens to appear once. The counts never have to agree.The
coveredset is the coverage claim in this artifact, so a duplicated claim is the one error worth catching hardest. Add a uniqueness check.♻️ Proposed check for duplicate covered operation ids
const coveredIds = entries.filter((entry) => entry.status === 'covered').map((entry) => entry.operationId).sort() + const claimed = new Set() + for (const id of coveredIds) { + if (claimed.has(id)) errors.push(`manifest operation ${id} is claimed by more than one covered runtime boundary`) + claimed.add(id) + } for (const id of manifestIds) if (!coveredIds.includes(id)) errors.push(`covered manifest operation has no runtime boundary: ${id}`)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tooling/carrier-contract-codegen/boundary-inventory.mjs` around lines 83 - 88, Update the manifest cross-check in boundary-inventory.mjs to detect duplicate operationId values among entries with status === 'covered' before or alongside the existing membership checks. Use the covered entries’ operation IDs and report each duplicated claim, while preserving the current manifest/runtime mismatch validation.
92-102: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winsuggestion:
coveredOperationIdguesses coverage from naming instead of reading the manifest.For
tauri-commandandhull-face, this function returns an id for every boundary. It never checks that the id exists inmanifest.json. The name transform at line 96 is the sole authority.Two consequences:
- A new Tauri command is auto-classified
coveredat line 49 with an operation id that does not exist. The build fails at line 87 withcovered runtime boundary has no manifest operation, which points at the inventory rather than at the missing manifest entry.- A manifest rename that does not follow the
snake_case→camelCaserule silently breaks the mapping. Lines 94-95 already exist as hand-written exceptions to that rule, which shows the rule is not reliable.Passing the manifest operation ids into
buildInventoryand treating a boundary ascoveredonly on a real match would make the classification derived, not guessed. That change also removes the need for the two special cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tooling/carrier-contract-codegen/boundary-inventory.mjs` around lines 92 - 102, Update buildInventory and coveredOperationId to accept the manifest operation ids and classify tauri-command and hull-face boundaries as covered only when a manifest id matches, using the boundary’s command/face mapping rather than guessed naming. Remove the two hard-coded tauri command exceptions and preserve undefined for unmatched boundaries so missing manifest entries are reported as uncovered.
216-228: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winthought:
braceMatchedBodycounts braces inside strings, comments, and template literals.The scan at lines 220-225 has no lexical state. A
}inside a string literal or a template literal inhull-port-adapter.tscloses the class body early.parseHullFacesthen enumerates a truncated body, and every face declared below that point disappears.That is precisely the failure the comment at lines 135-137 says this function fixed, reintroduced through a different mechanism. Line 146 does not catch it, because a truncated body usually still yields at least one face.
Also, line 218 interpolates a
RegExpinto the message, which renders the source pattern rather than a name. A short label reads better in CI output.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tooling/carrier-contract-codegen/boundary-inventory.mjs` around lines 216 - 228, Update braceMatchedBody to scan with lexical awareness, ignoring braces inside quoted strings, comments, and template literals so only structural braces affect depth and the complete class body is returned. Also replace the RegExp interpolation in its missing-opener error with a concise descriptive label, while preserving the existing source context and unterminated-body handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/contracts/protocol/boundary-inventory.json`:
- Around line 1483-1490: Update the route-inventory generation around
parseLocalNodeRoutes to fail closed when a route expression contains an
unresolved single-brace interpolation such as {action}. Do not emit one
ambiguous inventory row for "{RouteBase}/{{id}}/{action}"; instead reject or
explicitly flag it so the registration is corrected with concrete action values
and each production route can be counted accurately.
In `@packages/contracts/protocol/README.md`:
- Around line 10-14: Update the documented boundary-inventory workflow to
distinguish auto-classified derivable operationIds from entries requiring manual
deferred or blocked classification. For manual entries, instruct maintainers to
run boundary-inventory.mjs --write first, copy the composite id from its error
output, add the classification and reason, then run --write again; remove the
inaccurate “by default” qualifier and avoid implying every producer change needs
manual classification.
In `@tooling/carrier-contract-codegen/boundary-inventory.mjs`:
- Around line 134-148: Update parseHullFaces to enumerate every non-blank
class-body member line and fail closed when a line is neither an ignorable line
nor a recognized member, rather than silently skipping unmatched shapes.
Preserve face discovery for methods, accessors, arrow-function properties, and
varying indentation; continue excluding private members and explicitly confirm
the intended treatment of protected members.
- Around line 30-33: Update the sorting in discoverRuntimeBoundaries to use
deterministic JavaScript code-unit ordering for boundary IDs instead of
localeCompare. Preserve ascending order while comparing IDs directly, avoiding
locale or ICU-dependent collation.
- Around line 170-197: Update
tooling/carrier-contract-codegen/boundary-inventory.mjs at lines 170-197,
230-258, and 216-228. Add one shared per-file comment-stripping helper and use
its output for all scanners: ensure the Map registration matchAll ignores
commented code, make argumentAt skip comment spans and report missing arguments
separately from unterminated calls, and update braceMatchedBody to track string
and template-literal state so braces inside literals do not affect depth.
- Around line 45-51: Update the `boundaries` mapping to resolve each entry’s
status first, then emit `operationId` only when that status is `covered` and
emit `reason` only when the status is `deferred` or `blocked`. Preserve prior
status values and covered-operation discovery, while ensuring generated entries
satisfy `validateInventory`.
- Around line 264-273: Update main to distinguish a missing inventory file from
malformed JSON: treat only ENOENT as absent, and report JSON parse/read failures
directly. In the --write branch, validate the freshly built inventory with
validateInventory before writeFile and only print the success message after
validation passes, ensuring written output is accepted by check mode.
In `@tooling/carrier-contract-codegen/tests/generate.test.mjs`:
- Around line 29-34: Extend the tests around validateInventory and
buildInventory with table-driven negative-path cases covering every guarded
validation branch, including duplicate IDs, invalid or missing status/reason
data, operationId and defaultReasons consistency, unlisted or stale boundaries,
and both manifest checks; assert each exact expected error string so deleting
any named check makes the test fail. Add SCHEMA_VERSION to the existing imports,
and invoke buildInventory through its write path with synthetic discovery data
to verify the generated inventory and its compatibility with validateInventory.
---
Nitpick comments:
In `@tooling/carrier-contract-codegen/boundary-inventory.mjs`:
- Around line 112-114: Update parseTauriCommands to detect all
generate_handler![ occurrences and reject the source when more than one is
present, rather than selecting the first match. Preserve the existing
missing-handler error for zero matches and parse the command list only when
exactly one occurrence is found.
- Around line 83-88: Update the manifest cross-check in boundary-inventory.mjs
to detect duplicate operationId values among entries with status === 'covered'
before or alongside the existing membership checks. Use the covered entries’
operation IDs and report each duplicated claim, while preserving the current
manifest/runtime mismatch validation.
- Around line 92-102: Update buildInventory and coveredOperationId to accept the
manifest operation ids and classify tauri-command and hull-face boundaries as
covered only when a manifest id matches, using the boundary’s command/face
mapping rather than guessed naming. Remove the two hard-coded tauri command
exceptions and preserve undefined for unmatched boundaries so missing manifest
entries are reported as uncovered.
- Around line 216-228: Update braceMatchedBody to scan with lexical awareness,
ignoring braces inside quoted strings, comments, and template literals so only
structural braces affect depth and the complete class body is returned. Also
replace the RegExp interpolation in its missing-opener error with a concise
descriptive label, while preserving the existing source context and
unterminated-body handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e28b7b1-425e-466f-9561-2c47b117a77e
📒 Files selected for processing (4)
packages/contracts/protocol/README.mdpackages/contracts/protocol/boundary-inventory.jsontooling/carrier-contract-codegen/boundary-inventory.mjstooling/carrier-contract-codegen/tests/generate.test.mjs
| { | ||
| "id": "local-node-http:POST:apps/local-node-host/Health/RecurringInvoiceRoutes.cs:$\"{RouteBase}/{{id}}/{action}\"", | ||
| "kind": "local-node-http", | ||
| "method": "POST", | ||
| "routeExpression": "$\"{RouteBase}/{{id}}/{action}\"", | ||
| "source": "apps/local-node-host/Health/RecurringInvoiceRoutes.cs", | ||
| "status": "deferred" | ||
| }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
issue: this entry hides an unknown number of real routes behind one inventory row.
Compare the routeExpression here with every neighbour. Other entries use {{id}}, which is a literal {id} route parameter in a C# interpolated string. This one is $"{RouteBase}/{{id}}/{action}". The final segment uses single braces, so action is a C# variable interpolated at registration time, not a route parameter.
The registration is therefore almost certainly inside a loop or called once per action value. One row in this file represents N production routes, and N is not visible here. The denominator this artifact exists to establish is understated by N-1 for this source.
The walker cannot resolve the value, so it cannot count correctly. Two workable options: give this entry an explicit reason that names the concrete action values it covers, or make parseLocalNodeRoutes reject a route expression that contains a single-brace interpolation it cannot resolve, consistent with the fail-closed rule applied to unknown Map* forms.
🔍 Script to reveal how many routes this registration produces
#!/bin/bash
# Show the RecurringInvoiceRoutes registration and any single-brace interpolated route expressions.
set -euo pipefail
f=apps/local-node-host/Health/RecurringInvoiceRoutes.cs
[ -f "$f" ] && { echo "== $f =="; rg -n -C 12 'MapPost|action' "$f"; } || echo "file not found: $f"
echo
echo "== all route expressions with an unresolved single-brace interpolation =="
jq -r '.boundaries[]
| select(.routeExpression != null)
| select(.routeExpression | test("\\$\"") and (test("\\{[a-z][A-Za-z0-9_]*\\}")))
| "\(.id)"' packages/contracts/protocol/boundary-inventory.json🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/contracts/protocol/boundary-inventory.json` around lines 1483 -
1490, Update the route-inventory generation around parseLocalNodeRoutes to fail
closed when a route expression contains an unresolved single-brace interpolation
such as {action}. Do not emit one ambiguous inventory row for
"{RouteBase}/{{id}}/{action}"; instead reject or explicitly flag it so the
registration is corrected with concrete action values and each production route
can be counted accurately.
| operation that has no schema or adapter. After an intentional producer change, add its explicit | ||
| classification and reason to the inventory, then normalize it with | ||
| `node tooling/carrier-contract-codegen/boundary-inventory.mjs --write`. The command refuses to | ||
| classify a new surface by default; the generator suite fails on any unlisted or stale producer | ||
| operation and on any deferred/blocked entry without a reason. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
issue: the documented workflow does not match what the tool does.
Line 10-12 instructs: add the classification and reason to the inventory, then run --write to normalize. A maintainer cannot follow that order for a new boundary. To hand-add an entry they need its exact id, and the id is a composite of kind, method, source, and routeExpression built at boundary-inventory.mjs line 168. They cannot know it before discovery runs.
The real loop is: run --write, read the id out of the thrown error at line 50, hand-add the entry with that id, run --write again. Document that loop.
Two smaller corrections:
- Line 12-13 says the command "refuses to classify a new surface by default". "by default" tells the reader a flag exists to override it. No such flag exists. Remove the qualifier.
- A new boundary whose
operationIdis derivable is auto-classifiedcoveredat line 49 with no hand edit. Only boundaries that needdeferredorblockedrequire the manual step. The current text implies every producer change needs one.
📝 Proposed wording
-operation that has no schema or adapter. After an intentional producer change, add its explicit
-classification and reason to the inventory, then normalize it with
-`node tooling/carrier-contract-codegen/boundary-inventory.mjs --write`. The command refuses to
-classify a new surface by default; the generator suite fails on any unlisted or stale producer
-operation and on any deferred/blocked entry without a reason.
+operation that has no schema or adapter. After an intentional producer change, run
+`node tooling/carrier-contract-codegen/boundary-inventory.mjs --write`. A new boundary that maps to
+an existing manifest operation is recorded as `covered` automatically. Any other new boundary makes
+the command fail and print the boundary id it refuses to classify. Add an entry for that id with an
+explicit `status` and `reason`, then run the command again to normalize the file. The generator
+suite fails on any unlisted or stale producer operation and on any deferred/blocked entry without a
+reason.📝 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.
| operation that has no schema or adapter. After an intentional producer change, add its explicit | |
| classification and reason to the inventory, then normalize it with | |
| `node tooling/carrier-contract-codegen/boundary-inventory.mjs --write`. The command refuses to | |
| classify a new surface by default; the generator suite fails on any unlisted or stale producer | |
| operation and on any deferred/blocked entry without a reason. | |
| operation that has no schema or adapter. After an intentional producer change, run | |
| `node tooling/carrier-contract-codegen/boundary-inventory.mjs --write`. A new boundary that maps to | |
| an existing manifest operation is recorded as `covered` automatically. Any other new boundary makes | |
| the command fail and print the boundary id it refuses to classify. Add an entry for that id with an | |
| explicit `status` and `reason`, then run the command again to normalize the file. The generator | |
| suite fails on any unlisted or stale producer operation and on any deferred/blocked entry without a | |
| reason. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/contracts/protocol/README.md` around lines 10 - 14, Update the
documented boundary-inventory workflow to distinguish auto-classified derivable
operationIds from entries requiring manual deferred or blocked classification.
For manual entries, instruct maintainers to run boundary-inventory.mjs --write
first, copy the composite id from its error output, add the classification and
reason, then run --write again; remove the inaccurate “by default” qualifier and
avoid implying every producer change needs manual classification.
| export async function discoverRuntimeBoundaries() { | ||
| return [...await discoverTauriCommands(), ...await discoverHullFaces(), ...await discoverLocalNodeRoutes()] | ||
| .sort((left, right) => left.id.localeCompare(right.id)) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
issue: localeCompare makes the committed inventory order depend on the host locale and ICU build.
Line 32 sorts with left.id.localeCompare(right.id) and passes no locale. That call resolves against the runtime default locale and the ICU collation data in the Node build. Collation also treats punctuation differently from code-unit order: it can weight or ignore characters such as $, ", {, and :, which appear in nearly every id in this artifact.
Two consequences for packages/contracts/protocol/boundary-inventory.json:
- A maintainer running
--writeon a machine with a differentLANG, or on asmall-icuNode build, can reorder the file with no semantic change. The diff is large and the review signal is lost. - Order sensitivity undermines clean-checkout reproducibility. A check job and a developer machine should produce byte-identical output.
Two adjacent ids in the committed file already sit where collation rules decide the outcome rather than code points: $"{RouteBase}/{{id}}/{action}" at line 1484 and $"{RouteBase}/{{id}}/generate" at line 1492. Under code-unit order { sorts after g; under many collations it does not.
These ids are machine-generated ASCII identifiers, not human-readable text. Sort them by code unit.
🐛 Proposed fix: deterministic code-unit ordering
export async function discoverRuntimeBoundaries() {
return [...await discoverTauriCommands(), ...await discoverHullFaces(), ...await discoverLocalNodeRoutes()]
- .sort((left, right) => left.id.localeCompare(right.id))
+ .sort((left, right) => (left.id < right.id ? -1 : left.id > right.id ? 1 : 0))
}If you prefer to keep a comparator call, new Intl.Collator('en', { sensitivity: 'variant' }) is still locale-data dependent. Code-unit comparison is the only fully reproducible option here.
#!/bin/bash
# Show whether the committed order matches code-unit order, and where locale collation differs.
set -euo pipefail
f=packages/contracts/protocol/boundary-inventory.json
[ -f "$f" ] || { echo "not found: $f"; exit 0; }
node --input-type=module -e '
import { readFileSync } from "node:fs"
const ids = JSON.parse(readFileSync("packages/contracts/protocol/boundary-inventory.json","utf8")).boundaries.map(b => b.id)
const codeUnit = [...ids].sort((a,b) => (a<b?-1:a>b?1:0))
const locale = [...ids].sort((a,b) => a.localeCompare(b))
const firstDiff = (x,y) => { for (let i=0;i<x.length;i++) if (x[i]!==y[i]) return i; return -1 }
console.log("entries:", ids.length)
console.log("committed === localeCompare order:", JSON.stringify(ids)===JSON.stringify(locale))
console.log("committed === code-unit order :", JSON.stringify(ids)===JSON.stringify(codeUnit))
const d = firstDiff(codeUnit, locale)
console.log("first index where the two orders diverge:", d)
if (d >= 0) console.log(" code-unit:", codeUnit[d], "\n locale :", locale[d])
console.log("ICU:", typeof Intl.Collator === "function" ? Intl.Collator().resolvedOptions().locale : "none")
'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tooling/carrier-contract-codegen/boundary-inventory.mjs` around lines 30 -
33, Update the sorting in discoverRuntimeBoundaries to use deterministic
JavaScript code-unit ordering for boundary IDs instead of localeCompare.
Preserve ascending order while comparing IDs directly, avoiding locale or
ICU-dependent collation.
| boundaries: discovered.map((boundary) => { | ||
| const prior = priorById.get(boundary.id) | ||
| const operationId = coveredOperationId(boundary) | ||
| if (prior) return { ...boundary, status: prior.status, ...(operationId ? { operationId } : {}), ...(prior.reason ? { reason: prior.reason } : {}) } | ||
| if (operationId) return { ...boundary, status: 'covered', operationId } | ||
| throw new Error(`new runtime boundary requires an explicit inventory status and reason: ${boundary.id}`) | ||
| }), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
issue (blocking): --write can emit an inventory that validateInventory rejects.
Line 48 attaches operationId whenever coveredOperationId(boundary) returns a value. It does that independently of prior.status. But coveredOperationId is purely name-derived for tauri-command and hull-face: it returns an id for every such boundary, with no manifest lookup.
Two writer/validator conflicts follow:
- A
tauri-commandorhull-faceentry that a maintainer classified asdeferredorblockedkeeps that status, and also gains anoperationId. Line 74 then fails it:deferred boundary ... must not claim a manifest operationId. - A
coveredprior entry that carries areasonre-emits thatreason. Line 72 then fails it:covered boundary ... must not carry a deferred/blocked reason.
In both cases --write produces a file that the check run rejects, and the maintainer has no clean path forward. Gate both fields on the resolved status.
🐛 Proposed fix: respect the resolved status when emitting operationId and reason
boundaries: discovered.map((boundary) => {
const prior = priorById.get(boundary.id)
const operationId = coveredOperationId(boundary)
- if (prior) return { ...boundary, status: prior.status, ...(operationId ? { operationId } : {}), ...(prior.reason ? { reason: prior.reason } : {}) }
+ if (prior) {
+ const covered = prior.status === 'covered'
+ return {
+ ...boundary,
+ status: prior.status,
+ ...(covered && operationId ? { operationId } : {}),
+ ...(!covered && prior.reason ? { reason: prior.reason } : {}),
+ }
+ }
if (operationId) return { ...boundary, status: 'covered', operationId }
throw new Error(`new runtime boundary requires an explicit inventory status and reason: ${boundary.id}`)
}),📝 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.
| boundaries: discovered.map((boundary) => { | |
| const prior = priorById.get(boundary.id) | |
| const operationId = coveredOperationId(boundary) | |
| if (prior) return { ...boundary, status: prior.status, ...(operationId ? { operationId } : {}), ...(prior.reason ? { reason: prior.reason } : {}) } | |
| if (operationId) return { ...boundary, status: 'covered', operationId } | |
| throw new Error(`new runtime boundary requires an explicit inventory status and reason: ${boundary.id}`) | |
| }), | |
| boundaries: discovered.map((boundary) => { | |
| const prior = priorById.get(boundary.id) | |
| const operationId = coveredOperationId(boundary) | |
| if (prior) { | |
| const covered = prior.status === 'covered' | |
| return { | |
| ...boundary, | |
| status: prior.status, | |
| ...(covered && operationId ? { operationId } : {}), | |
| ...(!covered && prior.reason ? { reason: prior.reason } : {}), | |
| } | |
| } | |
| if (operationId) return { ...boundary, status: 'covered', operationId } | |
| throw new Error(`new runtime boundary requires an explicit inventory status and reason: ${boundary.id}`) | |
| }), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tooling/carrier-contract-codegen/boundary-inventory.mjs` around lines 45 -
51, Update the `boundaries` mapping to resolve each entry’s status first, then
emit `operationId` only when that status is `covered` and emit `reason` only
when the status is `deferred` or `blocked`. Preserve prior status values and
covered-operation discovery, while ensuring generated entries satisfy
`validateInventory`.
| export function parseHullFaces(text, source) { | ||
| // Brace-match the whole class rather than stopping at `private requireConnection`: the old bound | ||
| // meant any face declared BELOW that helper, or declared without `async`, was silently not a | ||
| // boundary. Every method is now enumerated and each one is either a face or explicitly private. | ||
| const classBody = braceMatchedBody(text, /export class HullShellPortAdapter implements HullPort \{/, source) | ||
| const faces = [] | ||
| for (const match of classBody.matchAll(/^ {2}(?<modifiers>(?:(?:private|public|protected|static|async|readonly)\s+)*)(?<name>#?[A-Za-z_]\w*)\s*(?:<[^>(]*>)?\s*\(/gm)) { | ||
| const { modifiers, name } = match.groups | ||
| if (name === 'constructor') continue | ||
| if (name.startsWith('#') || /\bprivate\b/.test(modifiers)) continue | ||
| faces.push({ id: `hull-face:${name}`, kind: 'hull-face', face: name, source }) | ||
| } | ||
| if (faces.length === 0) throw new Error(`cannot find HullShellPortAdapter faces in ${source}`) | ||
| return faces | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
issue: parseHullFaces silently skips member shapes it does not match, which breaks the fail-closed rule the rest of the file follows.
The comment at lines 135-137 states: "Every method is now enumerated and each one is either a face or explicitly private." The regex at line 140 does not enforce that. It requires ( directly after the member name, so these public members match nothing and disappear without a word:
- an accessor:
get status(): HealthReport { ... }—getis not in the modifier list, andstatusis not followed by(; - an arrow-function property:
announce = async (request: AnnounceRequest) => { ... }—=sits between the name and(; - a member indented by anything other than exactly two spaces, because line 140 anchors on
^ {2}.
parseLocalNodeRoutes throws on an unclassified Map* form at line 193. parseHullFaces has no equivalent. Line 146 only fires when the count reaches zero, so losing one face of eight is invisible. That is the same "vanished from the denominator" failure the header comment describes.
Enumerating every non-blank line of the class body and rejecting any line that is neither a recognized member nor an ignorable line would restore the invariant.
Note: protected members are also currently counted as faces. Confirm that is intended.
🔍 Script to check which member shapes exist in the real adapter today
#!/bin/bash
# Inspect the real HullShellPortAdapter members and compare against the parser's assumptions.
set -euo pipefail
f=apps/hull/src/protocol/hull-port-adapter.ts
[ -f "$f" ] || { echo "adapter not found at $f"; exit 0; }
echo "== full member outline =="
ast-grep outline "$f" --items all
echo
echo "== accessors, arrow-function properties, and non-2-space members =="
rg -nP '^\s*(get|set)\s+\w+\s*\(|^\s*(public|protected|private|readonly|static|\s)*\w+\s*(<[^>]*>)?\s*=\s*(async\s*)?\(' "$f"
echo
echo "== indentation histogram of member-looking lines =="
rg -nP '^\s+[`#A-Za-z_`]' "$f" | sed -E 's/^[0-9]+:( *).*/\1/' | awk '{print length($0)}' | sort -n | uniq -c🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tooling/carrier-contract-codegen/boundary-inventory.mjs` around lines 134 -
148, Update parseHullFaces to enumerate every non-blank class-body member line
and fail closed when a line is neither an ignorable line nor a recognized
member, rather than silently skipping unmatched shapes. Preserve face discovery
for methods, accessors, arrow-function properties, and varying indentation;
continue excluding private members and explicitly confirm the intended treatment
of protected members.
| for (const match of text.matchAll(/\.(Map[A-Z]\w*)\s*\(/g)) { | ||
| const call = match[1] | ||
| const argumentStart = match.index + match[0].length | ||
| if (NON_BOUNDARY_MAP_CALLS.has(call)) continue | ||
| if (HTTP_VERB_MAPS.has(call)) { | ||
| push('local-node-http', call.slice(3).toUpperCase(), normalizeExpression(argumentAt(text, argumentStart, 0))) | ||
| continue | ||
| } | ||
| if (call === 'MapMethods') { | ||
| const routeExpression = normalizeExpression(argumentAt(text, argumentStart, 0)) | ||
| const verbs = [...normalizeExpression(argumentAt(text, argumentStart, 1)).matchAll(/"([A-Za-z]+)"/g)].map((verb) => verb[1].toUpperCase()) | ||
| if (verbs.length === 0) throw new Error(`cannot read the HTTP verbs of a MapMethods registration in ${source}`) | ||
| for (const verb of verbs) push('local-node-http', verb, routeExpression) | ||
| continue | ||
| } | ||
| if (call === 'MapWebSocketPath') { | ||
| push('local-node-ws', 'WEBSOCKET', normalizeExpression(argumentAt(text, argumentStart, 0))) | ||
| continue | ||
| } | ||
| if (call === 'MapHealthChecks') { | ||
| push('local-node-http', 'GET', normalizeExpression(argumentAt(text, argumentStart, 0))) | ||
| continue | ||
| } | ||
| throw new Error( | ||
| `unknown endpoint registration form ${call}( in ${source} — classify it: add it to this walker ` | ||
| + 'as a boundary, or to NON_BOUNDARY_MAP_CALLS with the reason it registers no route', | ||
| ) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
issue: three hand-rolled scanners parse source text with no lexical awareness of comments or strings.
All three findings share one root cause. Each scanner walks raw file text and assumes every character it sees is live code. Comments, string literals, and template literals are indistinguishable from real syntax. The result is that the artifact can gain phantom boundaries, lose real ones, or fail on well-formed input. That is the exact class of failure the header comments in this file say these parsers were written to end.
tooling/carrier-contract-codegen/boundary-inventory.mjs#L170-L197: strip C# comments before thematchAllat line 170, so a commented-out.MapGet(is not counted and a.MapSomething(in prose does not throw at line 193.tooling/carrier-contract-codegen/boundary-inventory.mjs#L230-L258: skip comment spans inside theargumentAtscan loop, so an apostrophe in a//comment cannot open a string that never closes. The same pre-parse comment strip removes the need for this. Also split the line 257 error so a missing argument is not reported as an unterminated call.tooling/carrier-contract-codegen/boundary-inventory.mjs#L216-L228: track string and template-literal state in thebraceMatchedBodydepth counter, so a}inside a TypeScript string cannot truncate the class body and drop the faces declared below it.
A single shared helper that returns comment-stripped text, applied once per file before parsing, resolves the first two directly and gives the third a consistent pattern to follow.
📍 Affects 1 file
tooling/carrier-contract-codegen/boundary-inventory.mjs#L170-L197(this comment)tooling/carrier-contract-codegen/boundary-inventory.mjs#L230-L258tooling/carrier-contract-codegen/boundary-inventory.mjs#L216-L228
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tooling/carrier-contract-codegen/boundary-inventory.mjs` around lines 170 -
197, Update tooling/carrier-contract-codegen/boundary-inventory.mjs at lines
170-197, 230-258, and 216-228. Add one shared per-file comment-stripping helper
and use its output for all scanners: ensure the Map registration matchAll
ignores commented code, make argumentAt skip comment spans and report missing
arguments separately from unterminated calls, and update braceMatchedBody to
track string and template-literal state so braces inside literals do not affect
depth.
| async function main() { | ||
| const discovered = await discoverRuntimeBoundaries() | ||
| let current | ||
| try { current = JSON.parse(await readFile(inventoryPath, 'utf8')) } catch { current = undefined } | ||
| if (process.argv.includes('--write')) { | ||
| const inventory = await buildInventory(current) | ||
| await writeFile(inventoryPath, `${JSON.stringify(inventory, null, 2)}\n`) | ||
| process.stdout.write(`wrote ${inventory.boundaries.length} runtime boundaries to ${relative(root, inventoryPath)}\n`) | ||
| return | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
issue: --write reports success without validating what it wrote, and line 267 hides a malformed inventory.
Two problems in main:
- Line 267 catches every failure of
JSON.parseandreadFileand setscurrent = undefined. A single trailing comma inboundary-inventory.jsonis then indistinguishable from a missing file. In check mode the run printsboundary inventory has an unsupported schemaVersionfollowed by 232unlisted runtime boundarylines. The real cause, one syntax error, never appears. DistinguishENOENTfrom a parse failure and report the parse failure directly. - The
--writebranch at lines 268-273 writes the file and returns. It never callsvalidateInventory. Combined with the issue on lines 45-51,--writecan printwrote 232 runtime boundariesand still leave the tree in a state that the check run rejects. Validating the freshly built inventory before writing would make--writeand the check agree by construction.
🐛 Proposed fix for both points
let current
- try { current = JSON.parse(await readFile(inventoryPath, 'utf8')) } catch { current = undefined }
+ try {
+ current = JSON.parse(await readFile(inventoryPath, 'utf8'))
+ } catch (error) {
+ if (error.code !== 'ENOENT') throw new Error(`cannot read ${relative(root, inventoryPath)}: ${error.message}`, { cause: error })
+ current = undefined
+ }
if (process.argv.includes('--write')) {
const inventory = await buildInventory(current)
+ const manifest = JSON.parse(await readFile(resolve(root, 'packages/contracts/protocol/manifest.json'), 'utf8'))
+ const errors = validateInventory(inventory, discovered, manifest)
+ if (errors.length) {
+ for (const error of errors) process.stderr.write(`${error}\n`)
+ process.stderr.write('refusing to write an inventory that fails its own validation\n')
+ process.exitCode = 1
+ return
+ }
await writeFile(inventoryPath, `${JSON.stringify(inventory, null, 2)}\n`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function main() { | |
| const discovered = await discoverRuntimeBoundaries() | |
| let current | |
| try { current = JSON.parse(await readFile(inventoryPath, 'utf8')) } catch { current = undefined } | |
| if (process.argv.includes('--write')) { | |
| const inventory = await buildInventory(current) | |
| await writeFile(inventoryPath, `${JSON.stringify(inventory, null, 2)}\n`) | |
| process.stdout.write(`wrote ${inventory.boundaries.length} runtime boundaries to ${relative(root, inventoryPath)}\n`) | |
| return | |
| } | |
| async function main() { | |
| const discovered = await discoverRuntimeBoundaries() | |
| let current | |
| try { | |
| current = JSON.parse(await readFile(inventoryPath, 'utf8')) | |
| } catch (error) { | |
| if (error.code !== 'ENOENT') throw new Error(`cannot read ${relative(root, inventoryPath)}: ${error.message}`, { cause: error }) | |
| current = undefined | |
| } | |
| if (process.argv.includes('--write')) { | |
| const inventory = await buildInventory(current) | |
| const manifest = JSON.parse(await readFile(resolve(root, 'packages/contracts/protocol/manifest.json'), 'utf8')) | |
| const errors = validateInventory(inventory, discovered, manifest) | |
| if (errors.length) { | |
| for (const error of errors) process.stderr.write(`${error}\n`) | |
| process.stderr.write('refusing to write an inventory that fails its own validation\n') | |
| process.exitCode = 1 | |
| return | |
| } | |
| await writeFile(inventoryPath, `${JSON.stringify(inventory, null, 2)}\n`) | |
| process.stdout.write(`wrote ${inventory.boundaries.length} runtime boundaries to ${relative(root, inventoryPath)}\n`) | |
| return | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tooling/carrier-contract-codegen/boundary-inventory.mjs` around lines 264 -
273, Update main to distinguish a missing inventory file from malformed JSON:
treat only ENOENT as absent, and report JSON parse/read failures directly. In
the --write branch, validate the freshly built inventory with validateInventory
before writeFile and only print the success message after validation passes,
ensuring written output is accepted by check mode.
| test('boundary inventory classifies every runtime producer operation', async () => { | ||
| const inventory = JSON.parse(readFileSync(boundaryInventoryPath, 'utf8')) | ||
| const manifest = JSON.parse(readFileSync(manifestPath, 'utf8')) | ||
| const discovered = await discoverRuntimeBoundaries() | ||
| assert.deepEqual(validateInventory(inventory, discovered, manifest), []) | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
issue: validateInventory and buildInventory have no negative-path coverage, so most of the guarded logic fails the falsification test.
This test asserts only that the committed inventory currently validates clean. Every error branch in validateInventory is unexercised: duplicate id (line 62), invalid status (line 64), missing reason (line 70), restated default reason (line 71), covered carrying a reason (line 72), covered without an operationId (line 73), non-covered claiming an operationId (line 74), unused defaultReasons (line 78), unlisted boundary (line 81), stale entry (line 82), and both manifest cross-checks (lines 86-87).
Delete any one of those lines and no check goes RED. buildInventory is not called by any test at all, so the --write path that maintainers are told to run in packages/contracts/protocol/README.md line 12 ships unverified.
The parser tests below do state their falsification correctly. Apply the same standard here. A table-driven test that feeds a synthetic inventory and asserts the exact expected error string per branch would cover this cheaply, and it would have caught the writer/validator conflict flagged on tooling/carrier-contract-codegen/boundary-inventory.mjs lines 45-51.
As per path instructions: "Name the mechanism that executes it and state the falsification: delete the guarded logic, the named check goes RED."
💚 Sketch of the missing negative-path test
+const baseEntry = {
+ id: 'hull-face:announce',
+ kind: 'hull-face',
+ face: 'announce',
+ source: 'apps/hull/src/protocol/hull-port-adapter.ts',
+}
+
+test('validateInventory rejects each malformed entry shape', () => {
+ const cases = [
+ [{ ...baseEntry, status: 'maybe' }, /has invalid status "maybe"/],
+ [{ ...baseEntry, status: 'deferred' }, /has no reason and no default reason/],
+ [{ ...baseEntry, status: 'covered' }, /has no manifest operationId/],
+ [{ ...baseEntry, status: 'covered', operationId: 'hull.announce', reason: 'x' }, /must not carry a deferred\/blocked reason/],
+ [{ ...baseEntry, status: 'blocked', reason: 'x', operationId: 'hull.announce' }, /must not claim a manifest operationId/],
+ ]
+ for (const [entry, expected] of cases) {
+ const errors = validateInventory(
+ { schemaVersion: SCHEMA_VERSION, boundaries: [entry] },
+ [{ id: entry.id }],
+ undefined,
+ )
+ assert.ok(errors.some((error) => expected.test(error)), `expected ${expected} in ${JSON.stringify(errors)}`)
+ }
+})
+
+test('validateInventory reports a boundary that no producer registers', () => {
+ const errors = validateInventory(
+ { schemaVersion: SCHEMA_VERSION, boundaries: [{ ...baseEntry, status: 'covered', operationId: 'hull.announce' }] },
+ [],
+ undefined,
+ )
+ assert.ok(errors.some((error) => /stale boundary inventory entry/.test(error)))
+})This needs SCHEMA_VERSION added to the import list at lines 7-14.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tooling/carrier-contract-codegen/tests/generate.test.mjs` around lines 29 -
34, Extend the tests around validateInventory and buildInventory with
table-driven negative-path cases covering every guarded validation branch,
including duplicate IDs, invalid or missing status/reason data, operationId and
defaultReasons consistency, unlisted or stale boundaries, and both manifest
checks; assert each exact expected error string so deleting any named check
makes the test fail. Add SCHEMA_VERSION to the existing imports, and invoke
buildInventory through its write path with synthetic discovery data to verify
the generated inventory and its compatibility with validateInventory.
Source: Path instructions
1 participant
feat(contracts): add a gated machine-readable boundary inventory
The protocol manifest enumerated COVERED operations only, so the uncovered Carrier surface was
invisible rather than documented. That is what made "migrate incrementally" unverifiable — progress
and completeness looked identical, and nobody could answer what was left without re-deriving it.
The inventory is derived from the PRODUCERS — the Tauri command list, the Hull adapter faces and
the production local-node route registrations — not from the schema. An inventory authored from the
schema would restate what is already covered and prove nothing; the point is to name what is not.
232 boundaries: 19 covered, 213 deferred, each deferred entry carrying a reason.
It is gated rather than decorative. A boundary appearing in neither the covered set nor the
inventory fails the build, so the document cannot rot silently the next time somebody adds an
endpoint. A stale inventory would be worse than none, because it reads as authoritative.
Kept as a separate file rather than folded into the manifest: the manifest is the generator's input,
and listing deferred operations there would present them to code generation as implemented
contracts.
No endpoint was migrated. That was explicitly out of scope and remains so.
Proven by adding a fake command to the real handler list — the gate fails naming it.
Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com
Summary by CodeRabbit
Documentation
Validation Improvements
Tests