feat(hull): gate direct transport access and make ADR 0162 say what is enforced - #3535
Conversation
…s enforced ADR 0162 build gate 3 claimed architecture tests prevent direct transport imports outside adapters while nothing enforced it for HTTP. Eight direct call sites had accumulated under a sentence saying they could not exist. A guard that names an invariant it only partially enforces is worse than an honest narrower one, because a reader treats the claim as coverage. The new gate covers three axes: the raw shell invoke, which was already guarded; direct calls to the global fetch outside a sanctioned transport; and direct Tauri IPC imports outside the protocol adapters. The population is recorded in two separate lists. Sanctioned entries own a transport legitimately and each states why, so a reader can tell a real exception from unmigrated debt without going to look. The rest is migration debt, capped by a literal that may only be lowered. The assertion is an exact match in BOTH directions, which is what makes it a ratchet rather than a snapshot. A new call site fails because it is in neither list. A migrated one also fails, because the list still names it — so the entry must be deleted in the same change that fixes it and the list can only shrink. Tauri imports are gated with no allowlist at all, the population already being empty; adding one later would mean adding an exemption to a rule currently held without exception. A first test asserts the walk found a non-empty population, so the others cannot pass vacuously on a broken path. Verified by three mutations, each red on its own assertion: a new fetch, a migrated file left in the list, and a Tauri import outside the adapters. Two documentation claims corrected to match the code. Gate 3 now states which three axes are enforced and that the fetch list is a shrinking allowlist. And the promise that version comparison and negotiation would land in the stacked follow-up is removed: that follow-up merged without them, and a deferral whose destination has already come and gone reads as resolved. It is now stated as a requirement no projection implements, pointing at a card. The same wording is corrected in the two ICM records that carried it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
the
needs-deep-review
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe change adds an architecture test for direct ChangesTransport Boundary Gate
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/hull/src/membrane/no-direct-transport.arch.test.ts`:
- Around line 150-164: Update the direct-fetch ratchet in the test “has exactly
the recorded set of direct fetch call sites” so it detects multiple matches
within a single file instead of mapping matches to one pathname. Represent and
compare each occurrence, or track expected counts per path while preserving
sanctioned and migration-debt coverage; add a mutation test that inserts a
second direct fetch into an existing MIGRATION_DEBT file and verifies the
assertion fails.
- Around line 179-184: Update the offender filter in the direct IPC import test
so the normalized TAURI_ADAPTER_DIR includes a trailing slash before prefix
matching. Reuse that normalized boundary for the startsWith check, ensuring
similarly named sibling directories such as protocol-shadow are excluded while
files within the adapter directory remain allowed.
- Around line 105-106: Extend the import detection in the architecture test
around TAURI_IMPORT to also match dynamic import expressions such as await
import('`@tauri-apps/`…'), then apply the existing protocol-adapter path exception
to both static and dynamic matches. Add a mutation test covering the
dynamic-import syntax and confirming it is rejected outside
apps/carrier/src/protocol.
In `@docs/adrs/0162-language-agnostic-carrier-hull-contracts.md`:
- Around line 111-114: Update the statements at
docs/adrs/0162-language-agnostic-carrier-hull-contracts.md lines 111-114,
icm/02_architecture/output/architecture-carrier-language-agnostic-adapters-2026-08-01.md
lines 33-35, and
icm/06_build/output/carrier-language-agnostic-adapters-2026-08-01.md lines 22-24
to state that Hull provides runtime version comparison and negotiation.
Reference isContractVersionCompatible(), negotiate(), and connectRuntime() as
the implemented enforcement path, and remove claims that projections lack
runtime negotiation or only generate models.
🪄 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: 49e75c4c-7bee-4d10-83f2-122b20767907
📒 Files selected for processing (4)
apps/hull/src/membrane/no-direct-transport.arch.test.tsdocs/adrs/0162-language-agnostic-carrier-hull-contracts.mdicm/02_architecture/output/architecture-carrier-language-agnostic-adapters-2026-08-01.mdicm/06_build/output/carrier-language-agnostic-adapters-2026-08-01.md
| /** Direct imports of the Tauri IPC surface, which belong only in the protocol adapters. */ | ||
| const TAURI_IMPORT = /from\s+['"]@tauri-apps\// |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== KB policy query =="
pnpm kb query "apps/hull direct imports Tauri IPC" || true
echo "== Locate target file and relevant regex =="
wc -l apps/hull/src/membrane/no-direct-transport.arch.test.ts
rg -n "TAURI_IMPORT|`@tauri-apps`|dynamic|import\\(|adapter" apps/hull/src/membrane/no-direct-transport.arch.test.ts apps/carrier/src/protocol 2>/dev/null || true
echo "== File outline =="
ast-grep outline apps/hull/src/membrane/no-direct-transport.arch.test.ts || true
echo "== Relevant file excerpt =="
sed -n '1,220p' apps/hull/src/membrane/no-direct-transport.arch.test.ts
echo "== Candidate tests in repository mentioning no-direct-transport =="
rg -n "no-direct-transport|TAURI_IMPORT|`@tauri-apps`" . --glob '!node_modules' --glob '!dist' --glob '!build' | head -200Repository: Harborline-Software/shipyard
Length of output: 29477
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the exact source and run a small static regex probe that mirrors the test's intent.
python3 - <<'PY'
import pathlib, re
path = pathlib.Path("apps/hull/src/membrane/no-direct-transport.arch.test.ts")
text = path.read_text()
m = re.search(r"const TAURI_IMPORT\s*=\s*/([^/]+)", text)
print("Tauri regex source:", m.group(0) if m else "not_found")
try:
TAURI_IMPORT = eval(m.group(1))
except Exception as e:
print("eval_error:", e)
raise SystemExit(0)
samples = {
"static_import": 'import { invoke } from "`@tauri-apps/api`"',
"await_import": 'await import("`@tauri-apps/api`")',
"dynamic_import_non_blocker": 'const mod = await import("./local")',
}
for name, code in samples.items():
print(f"{name}: code={code!r} matches={bool(TAURI_IMPORT.search(code)) if TAURI_IMPORT else None}")
PYRepository: Harborline-Software/shipyard
Length of output: 278
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whether the repository contains a no-direct-transport mutation/import test already.
python3 - <<'PY'
from pathlib import Path
import re
print("== mutation-related terms in target/membrane/carrier ==")
for path in [Path("apps/hull/src/membrane"), Path("apps/carrier/src/protocol")]:
if not path.exists():
continue
for p in [Path(p) for p in files]:
pass
PY
# Use the correct dynamic find:
files=$(git ls-files | rg '(^apps/hull/src/memory|^apps/hull/src/membrane|^apps/carrier/src/protocol|no-direct-transport|arch\.test)\.ts$' || true)
for f in $files; do
if rg -n "mutation|await import|TAURI_IMPORT|\\*`@tauri-apps`" "$f" >/dev/null; then
echo "--- $f"
rg -n "mutation|await import|TAURI_IMPORT|\\*`@tauri-apps`" "$f"
fi
doneRepository: Harborline-Software/shipyard
Length of output: 334
issue [blocking]: Detect dynamic Tauri imports.
apps/hull/src/membrane/no-direct-transport.arch.test.ts only checks from '@tauri-apps/…'; await import('@tauri-apps/…') bypasses this gate outside apps/carrier/src/protocol. Add dynamic import detection before applying the adapter-path exception, and cover it with a mutation test for this syntax.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/hull/src/membrane/no-direct-transport.arch.test.ts` around lines 105 -
106, Extend the import detection in the architecture test around TAURI_IMPORT to
also match dynamic import expressions such as await import('`@tauri-apps/`…'),
then apply the existing protocol-adapter path exception to both static and
dynamic matches. Add a mutation test covering the dynamic-import syntax and
confirming it is rejected outside apps/carrier/src/protocol.
| it('confines direct Tauri IPC imports to the protocol adapters', () => { | ||
| const offenders = files | ||
| .filter((f) => TAURI_IMPORT.test(f.source)) | ||
| .map((f) => f.path) | ||
| .filter((p) => !p.startsWith(TAURI_ADAPTER_DIR.split('\\').join('/'))) | ||
| .sort() |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
issue [blocking]: Require a directory-boundary match for protocol adapters.
startsWith(TAURI_ADAPTER_DIR) also permits paths such as apps/carrier/src/protocol-shadow/client.ts. Such a file can import Tauri directly.
Normalize the allowed directory once and require its trailing / before testing the prefix.
Proposed fix
const TAURI_ADAPTER_DIR = join('apps', 'carrier', 'src', 'protocol')
+const TAURI_ADAPTER_PREFIX = `${TAURI_ADAPTER_DIR.split('\\').join('/')}/`
- .filter((p) => !p.startsWith(TAURI_ADAPTER_DIR.split('\\').join('/')))
+ .filter((p) => !p.startsWith(TAURI_ADAPTER_PREFIX))📝 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('confines direct Tauri IPC imports to the protocol adapters', () => { | |
| const offenders = files | |
| .filter((f) => TAURI_IMPORT.test(f.source)) | |
| .map((f) => f.path) | |
| .filter((p) => !p.startsWith(TAURI_ADAPTER_DIR.split('\\').join('/'))) | |
| .sort() | |
| const TAURI_ADAPTER_DIR = join('apps', 'carrier', 'src', 'protocol') | |
| const TAURI_ADAPTER_PREFIX = `${TAURI_ADAPTER_DIR.split('\\').join('/')}/` | |
| it('confines direct Tauri IPC imports to the protocol adapters', () => { | |
| const offenders = files | |
| .filter((f) => TAURI_IMPORT.test(f.source)) | |
| .map((f) => f.path) | |
| .filter((p) => !p.startsWith(TAURI_ADAPTER_PREFIX)) | |
| .sort() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/hull/src/membrane/no-direct-transport.arch.test.ts` around lines 179 -
184, Update the offender filter in the direct IPC import test so the normalized
TAURI_ADAPTER_DIR includes a trailing slash before prefix matching. Reuse that
normalized boundary for the startsWith check, ensuring similarly named sibling
directories such as protocol-shadow are excluded while files within the adapter
directory remain allowed.
…rces
An adversarial review of the previous commit passed nine probes green, four of them written in this
codebase's existing house style. The gate was real but much weaker than its own comment claimed,
and one documentation correction it made was itself false.
Worst first. The ADR was rewritten to say version comparison and negotiation are "not implemented
in any projection". They are: the comparison lives in the membrane, the generated port's negotiate
runs it and returns a structured mismatch reason, and a test covers the rejection. Reading a single
constant and generalising from it produced a false under-claim in a ratified ADR — the same defect
as the over-claim it replaced, inverted, and worse, because it would send the next engineer to
build something that already exists. The text now states what is genuinely missing: the C# and Rust
projections do not implement it, so cross-language negotiation is unproven.
The gate itself is repaired on five axes:
- counts CALL SITES, not files. A file already on the list could previously absorb unlimited
further calls silently.
- strips comments before matching, as the companion gate already did. Two recorded entries matched
only prose — the words "fetch (" inside a JSDoc — so the debt count was inflated AND could be
paid down by rewording a comment, which the in-file guidance actively invited.
- drops the separate ceiling constant. It compared two literals in the same file and could never
fail against the codebase; the counted list is the ceiling.
- scans the Tauri host directory, matching the sibling gate it claimed to be a companion to, and
the two UI packages, where seven unrecorded call sites sit inside the carrier's own dependency
graph.
- asserts every root contributed files. An entire scan root could previously be deleted with the
gate still green, because a root holding no recorded entry is unobserved.
It now also enumerates its own blind spots, because the ones that escape are the house style: an
injected fetch parameter, which is why Hull's own transport is invisible to it, the re-exported
node and Tauri transport helpers, and a dynamic plugin import.
The sanction on the carrier's node client was laundering. Roughly 101 helper call sites across 36
modules cast unparsed JSON into hand-written interfaces and exactly one is adapter-mediated. The
stated reason described that one and read as though it described the module. It now says what the
module is, and the egress is carded separately.
Verified by mutation on each repaired axis: a second call in a listed file, a comment reword, a
call in the previously unscanned host directory, and a vanished scan root.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Blocked, fixed, and re-pushed at
|
| hole | before | now |
|---|---|---|
second fetch in an already-listed file |
green | red — call-site counts, not file names |
MAX_MIGRATION_DEBT |
compared two literals in this file; could never fail | removed; the counted list is the ceiling |
a comment containing fetch ( |
counted as debt, and could be "paid down" by rewording prose | comments stripped, as the companion gate already did |
apps/carrier/src-tauri |
unscanned, inconsistent with the sibling it claims to companion | scanned |
packages/ui-* |
7 unrecorded call sites, one in a carrier dependency | scanned and recorded |
| a scan root vanishing | green | red — asserted per root |
Two of the ten original entries matched only prose and have been removed; selectedSessionClient.ts carries two call sites, not one. The real population is 15 files / 16 call sites.
The sanction was laundering, as the review suspected
nodeClient.ts has ~101 helper call sites across ~36 modules; exactly one is adapter-mediated and the rest cast unparsed JSON into hand-written interfaces. The why described that one and read as though it described the module. It now says what the module is, and the egress is carded as #3536 — the largest ungated transport surface in the app.
The gate now states its own blind spots
Injected typeof fetch parameters (which is why Hull's own loopback transport is invisible to it), the re-exported nodeFetch/tauriTransportFetch helpers, and dynamic plugin imports. ADR gate 3 says "narrow, do not close" and lists them, rather than claiming prevention.
Verified by mutation on each repaired axis. Full gate exit 0: hull 309, contracts 275, carrier-sdk 86, carrier 2405, Rust, C#, all four lints.
Auto-merge stays off pending re-review.
…erclaiming its blind spots A second adversarial review found the previous commit created a coverage hole of its own, plus a fresh false claim inside the very block whose job is honesty about limits. The gate did not run for two of the six roots it scans. The previous commit added the two UI packages to the scan because seven real call sites live there — half the recorded population — but never added them to the CI paths filter. A pull request touching only those packages skipped the job entirely and posted this required check green, so the ratchet did not run for the exact population it was extended to cover. Both paths are now in the filter. The blind-spot list claimed no XMLHttpRequest instances existed and no HTTP client library was a dependency. Both were false. A real multipart upload uses XMLHttpRequest in a scanned root, in a file this list already records for a separate fetch, and the Tauri HTTP plugin is a declared dependency of the carrier. That is the same defect this gate exists to prevent, committed inside the honesty apparatus, so the list now names both as live and says so plainly. Two further blind spots are added rather than left to be rediscovered: comment stripping is not string-aware, so a call sharing a line with a string containing a comment token is hidden. Both forms were probed. Counts corrected to measured values. The sanction note claimed roughly 101 helper call sites across about 36 modules; the real numbers are 150 production call sites across 31 modules. The call-site figure was close and the module figure was never reachable. Three wording corrections in the ADR: the companion gate makes an un-chokepointed invoke a structural failure rather than a bypass; the empty Tauri-import population is a fact about today enforced only within six roots; and the XMLHttpRequest blind spot is named as live. Independently confirmed by the review: the recorded counts match reality pair-for-pair at 15 files and 16 call sites, every ratchet direction bites, and the version-enforcement sentence is now true. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
6ea8c04 to
55d2abd
Compare
…understated Three corrections, all in the direction of the gate reading weaker rather than stronger. The dynamic Tauri import bullet said "reachable, not hypothetical" while a real instance was already in the tree — apps/carrier/src/copilot/useBootHealth.ts:72 imports @tauri-apps/api/event dynamically, in a scanned root, outside the protocol adapters, and nothing catches it. That is the same understatement this file already records having committed once for XMLHttpRequest, in a comment three bullets away. The comment-token bullet scoped the hazard to one line. That holds for a double slash and for a regex ending in an escaped slash, but a block-comment opener inside a string deletes to the next close anywhere in the file, and because the assertion is exact-match the result is a recorded count DROPPING, which the ratchet reads as a successful migration. A lexer-correct re-scan disagrees with the naive strip on zero files, so today's population is exact and this is latent, which the comment now says. The blind-spot list asserted a floor of four against six entries, so any two could be deleted silently, including the two naming a live instance. It is now an exact match like every other list here, plus a minimum length per entry, so closing a hole means deleting its entry and lowering the number in the same reviewed change. Withdraws the "150 call sites across 31 modules" figure rather than restating it. It was hand-copied into two files, reproduces under no counting rule, and named three of the six helpers. The load-bearing part is the ratio, and it is one. The ADR's list of unscanned areas named apps/kitchen-sink, which is a Blazor app with zero TypeScript files, so the disclosure stated nothing. Replaced with the real gaps, including a live fetch in apps/hull/scripts inside a package the ADR presents as covered.
ctwoodwa
added
the
deep-review-passed
1 participant
Closes #3525
Closes #3532
The problem
ADR 0162 build gate 3 said "architecture tests prevent new direct transport imports outside adapters." Nothing enforced it for HTTP. Eight direct
fetch()call sites had accumulated in the carrier under a sentence saying they could not exist — each one a boundary where an untyped wire payload becomes a typed value without passing a generated parser.A guard that names an invariant it only partially enforces is worse than an honest narrower one, because a reader treats the claim as coverage. That is what happened here.
The gate
apps/hull/src/membrane/no-direct-transport.arch.test.ts, companion to the existingno-direct-invokeguard. Three axes:fetchoutside a sanctioned transport, includingglobalThis.fetch(andwindow.fetch(;@tauri-appsimports outside the protocol adapters.The ratchet, which is the point
The population is recorded in two separate lists:
SANCTIONED_FETCH— modules that legitimately own an HTTP transport, each with a stated reason. Two entries: Hull'sloopback-transport.ts, and the carrier'snodeClient.ts, whichHttpCarrierApplicationAdapterdelegates to before parsing through the generated model. A sanctioned entry without a reason is indistinguishable from debt, so a test asserts every one carries an explanation.MIGRATION_DEBT— the eight unmigrated call sites, capped by a literal that may only ever be lowered.The assertion is an exact match in both directions, and that is what makes this a ratchet rather than a snapshot:
fetch()addedSo the list can only shrink. Tauri imports are gated with no allowlist at all, the population already being empty — adding one later would mean adding an exemption to a rule currently held without exception.
A first test asserts the walk found a non-empty population, so the rest cannot pass vacuously against a broken path or an over-eager exclude.
Verified by mutation
Three, each red on its own assertion and nowhere else:
fetch()insync-status/syncStateUtils.ts→ exact-match test fails;tenantSelectionClient.tsmigrated but left in the list → same test fails as a stale entry;@tauri-appsimport in a non-adapter file → Tauri test fails.Full gate:
CI=true node scripts/ts-suites.mjsexit 0. Hull 309 passed / 2 skipped (the five new tests), contracts 275, carrier-sdk 86, carrier 2387, Rust and all four lints clean.The documentation half (#3532)
Two claims in the merged ADR were not true of the code.
Gate 3 now states which three axes are enforced and that the
fetch()list is a shrinking allowlist, rather than implying blanket coverage.The version-enforcement line promised that comparison and negotiation would "land in the stacked follow-up." That follow-up (#3530) merged without them —
CARRIER_PROTOCOL_VERSIONis still a bare constant whose only use is a test asserting its value. A deferral whose destination has already come and gone reads as resolved, which is worse than no wording. It is now stated as a requirement no projection implements, pointing at #3534. The same wording is corrected in the two ICM records that carried it, and thesrc-tauriCI gap now points at #3533.Follow-on
The eight debt entries are the migration work; #3523 (the DTO mirrors in
nodeClient.ts) overlaps with one of them.Summary by CodeRabbit
Bug Fixes
Documentation