Skip to content

fix(deps): move tcloud and tcloud-agent onto the live sandbox cohort - #49

Open
drewstone wants to merge 2 commits into
mainfrom
chore/live-cohort-20260816
Open

fix(deps): move tcloud and tcloud-agent onto the live sandbox cohort#49
drewstone wants to merge 2 commits into
mainfrom
chore/live-cohort-20260816

Conversation

@drewstone

Copy link
Copy Markdown
Contributor

Why

@tangle-network/tcloud@0.4.14 and @tangle-network/tcloud-agent@0.3.3 are npm latest and both depend on @tangle-network/sandbox@^0.9.5, a retired generation. Measured chains:

tcloud@0.4.14       → sandbox@0.9.x → agent-interface@0.13.0
                                    → agent-core@0.x    → agent-interface@0.17.1
tcloud-agent@0.3.3  → the same pair

That is 4 of the 12 physical agent-interface copies a full first-party install resolves. Two copies of agent-interface in one tree means two class identities and instanceof answering false across the seam.

What changed

  • @tangle-network/sandbox: ^0.9.5>=0.27.1 <0.28.0 in both packages — the live line, declared as a range so a later patch on it resolves to one copy.
  • tcloud 0.4.14 → 0.5.0, tcloud-agent 0.3.3 → 0.4.0.
  • scripts/check-cohort-ranges.mjs + prepublishOnly: publishing fails when a first-party specifier names one version instead of a range.
  • pnpm-workspace.yaml excludes @tangle-network/* from the release-age hold as a scope rather than three pinned versions, which go stale on the next cut.

One source change was needed

sandbox 0.27.1 types BackendConfig.profile as an AgentProfile object only; 0.9.5 accepted AgentProfile | string. SandboxSdkAgentSessionTransport passed input.profile straight through, so a cataloged profile id no longer typechecks — and would have been wrong on the wire.

A string profile now travels as the model selector (backend.model.model), an object as backend.profile. That is the routing this module's own header already documents ("Cataloged profile (string): routed as model: '<profile-id>'") and the routing the bridge transport already applies.

The test that asserted the old shape moved with it, and a second test now holds the inline-profile branch so neither can silently swap.

Verification

$ pnpm install --no-frozen-lockfile        # installs sandbox 0.27.1
$ pnpm -r build                            # 4/4 packages, DTS clean
$ pnpm -r test
  tcloud-attestation  16 passed
  tcloud              295 passed | 32 skipped
  tcloud-agent        30 passed
$ node scripts/check-cohort-ranges.mjs     # exit 0, all four packages

After merge

Both packages need a release to npm — the registry is what a consumer resolves, and the repository being ahead of it is exactly how this defect survived.

…ofile as a model

The published tcloud 0.4.14 and tcloud-agent 0.3.3 depended on
@tangle-network/sandbox ^0.9.5, which carries agent-interface 0.13.0 and
agent-core 0.17.1. Installing either package beside a current first-party
package therefore resolved two extra physical copies of agent-interface.

sandbox 0.27.1 types BackendConfig.profile as an AgentProfile object only, so
the SDK transport now sends a cataloged profile id as the model selector. That
is the routing this module already documents for a string profile, and the one
the bridge transport already applies.

check:cohort-ranges runs before publish and fails when a first-party specifier
names one version instead of a range.

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — c6c9fdf1

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.

tangletools · auto-approval · reason: drewstone_author · 2026-08-16T22:36:00Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Value Audit — sound-with-nits

Verdict sound-with-nits
Coverage 2 of 2 lenses (value, usefulness)
Concerns 4 (1 low, 3 weak-concern)
Heuristic 0.0s
Duplication 0.0s
Interrogation 1370.9s (2 bridge agents)
Total 1370.9s

💰 Value — sound-with-nits

Moves both published SDKs off a retired sandbox generation onto the live cohort (lockfile-verified: 2 agent-interface copies down to 1), routes cataloged profile ids the way sandbox 0.27.1 and this module's own transports already document, and adds a publish-time guard against exact first-party pins

  • What it does: Bumps @tangle-network/sandbox from ^0.9.5 to >=0.27.1 <0.28.0 in tcloud and tcloud-agent (major bumps 0.4.14→0.5.0, 0.3.3→0.4.0), adapts SandboxSdkAgentSessionTransport so a string profile travels as backend.model.model and an inline AgentProfile as backend.profile, adds scripts/check-cohort-ranges.mjs wired into prepublishOnly to fail exact first-party version pins, and excludes @tangle-network/*
  • Goals it achieves: Eliminates duplicate physical agent-interface copies in consumer installs (measured: 4 of 12 before; pnpm-lock at HEAD~1 shows agent-interface 0.13.0 + 0.17.1 co-resident, HEAD shows a single 1.0.0), which breaks instanceof across the seam; gets the published SDKs onto the maintained sandbox line; and prevents the regression class (exact first-party pins forcing second copies) from recurring at pu
  • Assessment: Good and coherent. The profile routing is not a judgement call invented here: the pulled sandbox@0.27.1 tarball documents PromptOptions.backend as 'backend.model for a model override, backend.profile for an inline AgentProfile', and the bridge transport plus module header already routed string profiles as the model selector — the SDK transport was the outlier. The narrow >=X.Y.Z <X.Y+1.0 range is
  • Better / existing approach: none — this is the right approach. Checked: (a) no existing pin/range guard in scripts/, root package.json, or .github/workflows to extend; (b) pnpm overrides/catalog only affect this workspace's resolution, not what consumers install, so they cannot fix published-specifier duplication; (c) making sandbox a peerDependency would guarantee single-copy but forces manual install on every consumer — a
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error

🎯 Usefulness — sound

A correct, verified dependency-cohort migration that collapses two physical agent-interface copies into one (parent lockfile: 0.13.0 + 0.17.1; HEAD: single 1.0.0) with one minimal source change that matches the live sandbox SDK's actual type surface.

  • Integration: Fully wired and reachable. The source change sits in SandboxSdkAgentSessionTransport.start (packages/tcloud-agent/src/agent-runner.ts:308-323), reached through the exported sandboxSdkTransport factory (agent-runner.ts:427, re-exported index.ts:54) and exercised by two tests (agent-runner.test.ts:387-426). The check script is wired into prepublishOnly in both packages (packages/tcloud/package.json:
  • Fit with existing patterns: Follows established patterns, no competition. String-profile-as-model already exists in the sibling BridgeAgentSessionTransport (agent-runner.ts:283-285, model: input.profile) and RouterChatAgentSessionTransport.routerChatProfile (agent-runner.ts:676-677), so the SandboxSdk transport now matches the codebase's own routing convention and the module header it documents (agent-runner.ts:41). The >=0.
  • Real-world viability: Holds up. Both profile branches (string and object) are covered by tests asserting the presence of one field and the absence of the other (agent-runner.test.ts:397-401, 422-425), so neither can silently swap. The start() build is stateless per call — no shared mutable state, so concurrent sessions are safe. The merge ({ ...(backend.model ?? {}), model: input.profile }) preserves a caller's pre-exi
  • Model: opencode/deepseek/deepseek-v4-pro
  • Bridge attempts: 3
  • Bridge warning: opencode/zai-coding-plan/glm-5.2: bridge stream ended without value-audit content; opencode/kimi-for-coding/k2p7: opencode: opencode error

🔎 Heuristic Signals

🟡 Cruft: commented out code packages/tcloud-agent/src/agent-runner.ts

+// let these become a re-export without churning the consumer surface.

💰 Value Audit

🟡 Cohort guard covers 2 of the 4 packages CI publishes [maintenance] ``

.github/workflows/publish-gh-packages.yml publishes tcloud, tcloud-agent, tcloud-attestation, and tcloud-relayer via pnpm publish, but only tcloud/package.json and tcloud-agent/package.json got the prepublishOnly hook running scripts/check-cohort-ranges.mjs. Currently moot — tcloud-attestation and tcloud-relayer declare no @tangle-network/* dependencies — but a future first-party dep added there ships unguarded. Either add the hook to all four publishable manifests or run the script as a CI

🟡 Exact-pin regex cannot see workspace:*, which publishes as an exact pin [maintenance] ``

scripts/check-cohort-ranges.mjs:18 only matches literal \d+.\d+.\d+ specifiers in the source tree. pnpm replaces workspace:* with the literal current version at publish time, so a workspace:* first-party dep would pass the check yet publish exactly the duplicate-forcing pin the script exists to block. Current usage is workspace:^ (tcloud-agent), which publishes as a caret, so today's tree is clean — worth matching workspace:* (and bare workspace:) in the offender set.

🎯 Usefulness Audit

🟡 String profile could use the SDK's first-class PromptOptions.model field [ergonomics] ``

sandbox 0.27.1 exposes PromptOptions.model (types-DL7FzacJ.d.ts:2671) documented as 'per-turn model id, forwarded as backend.model.model' and equivalent to backend: { model: { model } }. The PR's backend.model.model approach (agent-runner.ts:320) is documented-equivalent and correctly preserves a caller's backend.model config (provider/apiKey/baseUrl), so this is informational, not a defect — no change required.


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260817T011548Z

@tangletools

Copy link
Copy Markdown
Contributor

❌ Needs Work — c6c9fdf1

Review health 100/100 · Reviewer score 32/100 · Confidence 85/100 · 25 findings (3 high, 3 medium, 19 low)

opencode GLM 5.2 opencode DeepSeek v4 Pro opencode DeepSeek v4 Flash aggregate
Readiness 32 48 33 32
Confidence 85 85 85 85
Correctness 32 48 33 32
Security 32 48 33 32
Testing 32 48 33 32
Architecture 32 48 33 32

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 5/5 planned shots over 7 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 7 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 7 changed files. Global verifier still owns final merge decision.

Blocking

🔴 HIGH Lockfile dropped overrides while root package.json still declares them; frozen install fails — pnpm-lock.yaml

The diff removes the top-level overrides: block (basic-ftp, fast-xml-parser, postcss, protobufjs, vite), but root package.json (unchanged in this PR) still declares pnpm.overrides with those exact five pins. CI (.github/workflows/publish-gh-packages.yml:23,33) installs with pnpm 9 and --frozen-lockfile. Reproduced: corepack pnpm@9 install --frozen-lockfile fails with ERR_PNPM_LOCKFILE_CONFIG_MISMATCH: Cannot proceed with the frozen installation. The current "overrides" configuration doesn't match the value found in the lockfile. This blocks the publish workflow and any consumer running a frozen install. Fix: either remove pnpm.overrides from root package.json in this same PR, or re-add the overrides: block to the lockfile by regenerating with pnpm 9.

🔴 HIGH Lockfile drops overrides block but root package.json still declares pnpm.overrides — frozen-lockfile install fails under CI's pnpm 9 — pnpm-lock.yaml

Evidence: diff removes the overrides: block (basic-ftp 5.3.0, fast-xml-parser 5.7.0, postcss 8.5.10, protobufjs 7.5.5, vite 8.0.5) from pnpm-lock.yaml, but root package.json (unchanged by this PR, identical at 390dbde and c6c9fdf) still declares the same five under pnpm.overrides. Reproduced at head: pnpm install --frozen-lockfile --ignore-scripts with corepack pnpm@9.15.9 (and with pnpm 10.23.0) fails with ERR_PNPM_LOCKFILE_CONFIG_MISMATCH ('The current overrides configuration doesn't match the value found in the lockfile') before installing anything. Impact: .github/workflows/publish-gh-packages.yml:33 runs exactly this command with pnpm/action-setup@v4 version:9, so the next release event fails install→build→publish for all 4 matrix packages (tcloud, tcloud-agent, tcloud-attestat

🔴 HIGH Lockfile overrides removed but root package.json still declares pnpm.overrides -> CI frozen install fails on pnpm 9 — pnpm-lock.yaml

The diff deletes the lockfile's overrides: block (basic-ftp 5.3.0, fast-xml-parser 5.7.0, postcss 8.5.10, protobufjs 7.5.5, vite 8.0.5) but root package.json is not part of this PR and still contains the equivalent pnpm.overrides. pnpm 9 (the version pinned by CI in .github/workflows/publish-gh-packages.yml line 23, pnpm/action-setup@v4 version: 9) reads that field. Verified: in a clean worktree at head c6c9fdf, pnpm@9.15.9 install --frozen-lockfile exits 1 with ERR_PNPM_LOCKFILE_CONFIG_MISMATCH — The current "overrides" configuration doesn't match the value found in the lockfile. The same install passes under pnpm 11.22.0 (which ignores the pnpm field) but that is not the

Other

🟠 MEDIUM Lockfile generated with pnpm 10/11, not CI's pnpm 9; 293 lines of non-reproducible drift — pnpm-lock.yaml

The lockfile contains pnpm 10/11-only artifacts that pnpm 9 does not emit: supports-color@7.2.0 peer suffixes (e.g. line 41 tsup@8.5.1(...)(supports-color@7.2.0)...), libc: [glibc]/[musl] platform fields (first at line 670), and the widened vite: ^6.0.0 || ^7.0.0 || ^8.0.0 peer range. Regenerating with pnpm 9.15.9 (--lockfile-only) yields a 293-line diff against the committed lockfile even aside from the overrides block. This is the same root cause as the frozen-install failure and means the lockfile cannot be reproduced with the CI toolchain; fixing the override

🟠 MEDIUM Security/version pins silently inert: fast-xml-parser already floated past its declared pin — pnpm-lock.yaml

Evidence: package.json pins fast-xml-parser to exactly 5.7.0 via pnpm.overrides, but the lockfile now resolves fast-xml-parser@5.7.1 (used by @aws-sdk/xml-builder). Under pnpm ≥11 the package.json pin is ignored entirely, so the other four 'pinned' deps are only coincidentally at their pinned versions today — I verified vite 8.0.5, postcss 8.5.10, protobufjs 7.5.5, basic-ftp 5.3.0 all still resolve identically, but nothing now prevents them floating on the next lockfile update. Impact: if these overrides were audit-response pins, the protection evaporates without any visible error (pnpm ≥11 only warns once). 5.7.1 is newer than the pin so no advisory regression is introduced by this diff. Fix: whichever route from finding 1 is taken, ensure the pins live somewhere the active pnpm major act

🟠 MEDIUM workspace:* and workspace: bypass the exact-pin guard — scripts/check-cohort-ranges.mjs

The regex /^\d+.\d+.\d+(?:[-+].)?$/ only matches bare versions. pnpm substitutes workspace: and workspace:X.Y.Z with the literal current version when packing — I reproduced this with pnpm 10.23.0: a repo spec "@x/a": "workspace:" packed to "@x/a": "1.2.3" in the tarball manifest. So the spec form that produces the exact published pin — and the single most common pnpm workspace idiom — passes the check, defeating the script's stated purpose (header line 3: 'Fail a workspace manifest that would publish an exact first-party version pin'). Current tree is safe (both first-party specs are workspace:^, script exits 0), but a future edit to workspace: reintroduces the

🟡 LOW Contradictory backend.profile + string profile now sets both fields (was overwrite) — packages/tcloud-agent/src/agent-runner.ts

Previously start() did { ...backend, profile: input.profile }, so a string profile overwrote any construction-time backend.profile. The new code routes the string to backend.model.model and leaves a pre-set backend.profile intact. If a caller configures sandboxSdkTransport({ backend: { profile: <inline> } }) AND passes a string profile to agent({ profile: 'x' }), the emitted backend now carries both profile (object) and model.model simultaneously — a combination the 0.27.1 SDK may reject or resolve ambiguously (its PromptOptions.model docstring notes a profile/model conflict throws ValidationError). This is a contrived/misconfigured-caller scenario with no test coverage, but it is a concrete behavior change (overwrite -> additive) worth a guard or a comment. Fix: only spr

🟡 LOW Load-bearing routing (cataloged id via backend.model.model) has no live test coverage — packages/tcloud-agent/src/agent-runner.ts

The entire change rests on the assumption that the sandbox runtime/router resolves a cataloged profile id carried in backend.model.model as a profile selector. Evidence for it exists elsewhere in the repo (tcloud/tests/chat.test.ts:285 routes sf-proposer as a bridge model), but the new routing is only unit-tested against a mocked sandbox.prompt, and tcloud-agent ships no e2e test. A smoke test against the live sandbox cohort would close the gap; if the runtime instead treats 'sf-proposer' as a literal provider model id, the fix silently changes which agent answers every string-profile run.

🟡 LOW Per-turn inline agentProfile now coexists with the string-profile model selector — packages/tcloud-agent/src/agent-runner.ts

When a session starts with a string profile, base.backend carries model: { model: '<id>' }. promptOptionsForTurn merges a per-turn turn.sandbox.agentProfile into backend.profile but does NOT drop the model selector, so a direct session.chat({ messages, sandbox: { agentProfile: {...} } }) sends BOTH model.model and profile on the wire — an ambiguous double selector. Pre-change, the string lived in backend.profile and the inline per-turn profile overwrote it (clean last-write-wins), so this is a regression introduced by this commit. Reachable only through the raw transport session surface (the Agent loop passes no sandbox options), so low severity. Fix: when a per-turn agentProfile is supplied, drop the string-profile-sourced model.

🟡 LOW String profile silently overrides caller-supplied backend.model.model instead of failing loud — packages/tcloud-agent/src/agent-runner.ts

{ ...(backend.model ?? {}), model: input.profile } overwrites a caller-provided options.backend.model.model without any signal. The sibling SDK path deliberately chose the opposite policy for the same situation: sandbox 0.27.1's applyRequestedModel throws ValidationError('Conflicting per-turn models ... Set one of them.') when two model sources disagree, citing silent model substitution as the bug class to avoid. Overriding is a defensible precedence (the per-run profile is the more specific choice) but it is silent. Relatedly, a consumer who starts a session with a string profile and then passes sandbox.agentProfile per-turn sends BOTH backend.model.model and backend.profile (promptOptionsForTurn at [line 722-731](https://github.com/tangle-network/tcloud/blob/c6c9fdf10c5da3a

🟡 LOW String profile silently overwrites caller-specified backend.model.model — packages/tcloud-agent/src/agent-runner.ts

{ ...backend, model: { ...(backend.model ?? {}), model: input.profile } } silently replaces a model id a consumer set via sandboxSdkTransport({ backend: { model: { model: 'X' } } }). sandbox@0.27.1's own applyRequestedModel (runtime-api.js:82) treats two disagreeing model sources as a ValidationError precisely to prevent silent model substitution ('Silent preference between two disagreeing sources is exactly the bug class this fixes'). tcloud reintroduces the silent overwrite. Impact: consumer's explicit model override ignored without error. Fix: throw when backend.model?.model is set and differs from the string profile, mirroring applyRequestedModel.

🟡 LOW New inline-profile test does not assert the run verdict — packages/tcloud-agent/tests/agent-runner.test.ts

The test awaits agent(...).run() but never checks result.verdict. If completionFromPromptResult threw (e.g. a success:false prompt result) the run would return verdict:'error' and the test would still pass as long as prompts[0].options matched, weakening it as a regression guard. The sibling test at line 394 asserts verdict:'verified'. Fix: add expect(result.verdict).toBe('verified') by capturing the run result.

🟡 LOW New inline-profile test never asserts the run outcome — packages/tcloud-agent/tests/agent-runner.test.ts

await agent({...}).run() at L416 discards the AgentRunResult; only prompts[0].options is asserted. Its sibling test (L387-402) asserts result.verdict === 'verified' and result.usd === null. Because transport.start() captures prompt options before any per-turn call, a regression that made the run fail after routing (e.g. completionFromPromptResult throwing, or a transport error swallowed into verdict:'error') would leave this test green. The routing assertion itself is correct and mutation-sensitive for the actual change, but the test would be a stronger guard with expect(result.verdict).toBe('verified') mirroring the sibling test.

🟡 LOW No coverage for caller-supplied backend merged with a string profile — packages/tcloud-agent/tests/agent-runner.test.ts

The source branch { ...backend, model: { ...(backend.model ?? {}), model: input.profile } } preserves caller-supplied backend.model fields (provider/apiKey/baseUrl) while overriding model, but no test passes sandboxSdkTransport({ backend: ... }) with a string profile to lock that merge behavior or the precedence (profile id wins over user-set backend.model.model). Verified by reading code only; add one case asserting provider survives and model is overridden.

🟡 LOW No coverage of sandboxSdkTransport backend-option merge with profile routing — packages/tcloud-agent/tests/agent-runner.test.ts

Both tests construct sandboxSdkTransport({ sandbox }) with no backend override, so the { ...backend, ... } spread in src/agent-runner.ts:318-321 is never exercised. Verified in the published sandbox 0.27.1 runtime (normalizeRuntimeBackendConfig) that backend.profile and backend.model can coexist and are both forwarded to the wire without conflict — so a caller passing backend: { profile: {...} } alongside a string cataloged profile would send both. A test with sandboxSdkTransport({ sandbox, backend: { type: 'codex' } }) + a string profile, asserting { type: 'codex', model: { model: id } } and no stale profile key, would lock the merge semantics and the coexisting-keys edge.

🟡 LOW Lockfile records deprecation of @mariozechner/pi-* dev dependencies — pnpm-lock.yaml

The regenerated lockfile adds deprecated: please use @earendil-works/pi-* instead going forward to @mariozechner/pi-agent-core, pi-ai, pi-coding-agent, and pi-tui (all devDependencies of tcloud-agent). Informational only — these packages still install and build — but the upstream packages are renamed/abandoned, so a future migration to @earendil-works/pi-* should be tracked. No action required for this PR.

🟡 LOW Toolchain version divergence: lockfile generated with pnpm ≥10.16 semantics while CI pins pnpm 9 — pnpm-lock.yaml

Evidence: the paired pnpm-workspace.yaml gains minimumReleaseAgeExclude: ['@tangle-network/*'], a recent-pnpm setting, and the lockfile regeneration stripped the overrides block per pnpm ≥11 behavior; meanwhile CI uses pnpm/action-setup@v4 with version: 9 and no packageManager field exists in any package.json (corepack defaulted to 10.23.0 then 11.22.0 across two runs on the same machine — non-reproducible toolchain). Impact: lockfile churn and the finding-1 breakage class recur whenever developer/CI pnpm majors diverge. Fix: add packageManager: pnpm@<x>@<hash> to root package.json matching CI's action-setup version so every environment regenerates the lockfile with the same major.

🟡 LOW Guard not wired into tcloud-attestation and tcloud-relayer publishes — scripts/check-cohort-ranges.mjs

prepublishOnly invokes the script only in packages/tcloud and packages/tcloud-agent, but .github/workflows/publish-gh-packages.yml publishes all four packages via 'pnpm publish --no-git-checks'. tcloud-attestation and tcloud-relayer currently declare no @tangle-network/* deps, so no active risk, but any future first-party dep added to them bypasses the check entirely. Recommend running the script in the GH Packages workflow (or CI) rather than per-package lifecycle hooks.

🟡 LOW Malformed package.json is silently skipped, not failed — scripts/check-cohort-ranges.mjs

The empty catch { continue } treats an unreadable/unparseable manifest as a pass. For a publish-time guard this fails open: a broken manifest bypasses the exact-pin check. Impact today is nil (all manifests parse; a malformed one fails build anyway), but the guard should fail the release to preserve its invariant. Fix: rethrow or collect the path into failures.

🟡 LOW Malformed package.json silently skips validation — scripts/check-cohort-ranges.mjs

The bare catch { continue } treats ENOENT (expected: non-package directories under packages/) identically to a read error or JSON syntax error in a real package's manifest. A publishable package with an unparseable package.json is silently excluded from the guard — fail-open in a check whose doc header positions it as a publish gate. Impact is limited because pnpm publish itself would fail on malformed JSON, but the guard should distinguish: only skip when the file is absent, and let a parse failure of an existing manifest become a failure entry. Fix: stat/access the manifestPath first, and let JSON.parse errors push to failures instead of continuing.

🟡 LOW No automated test for a publish-blocking gate — scripts/check-cohort-ranges.mjs

The script is wired into prepublishOnly for tcloud and tcloud-agent, so a regression (e.g. a too-broad or too-narrow regex) would fail silently or block publishing unexpectedly. There is no unit test exercising the positive (exact pin -> exit 1) and negative (ranges -> exit 0) paths. I verified the happy path manually (exit 0 on this tree) but a negative-path assertion (injecting an exact pin) is cheap to add and would pin the regex's contract.

🟡 LOW No tests or CI execution for the guard script — scripts/check-cohort-ranges.mjs

The script runs only at publish time; a regression in its regex, directory resolution, or failure path would not be caught by CI (no test suite references it). I manually verified pass (repo exit 0) and fail (fixture exit 1) paths, but a permanent test, e.g. a vitest case or a CI step invoking it, would lock the invariant. In scope only as script-quality; it is the only shot file.

🟡 LOW Success line misleads for packages with zero first-party deps — scripts/check-cohort-ranges.mjs

process.stdout.write(${manifest.name}@${manifest.version} declares first-party ranges only) prints for every non-private package, including @tangle-network/tcloud-relayer which declares no @tangle-network/* dependencies at all (verified against head package.json). The line asserts a property that was never exercised, which weakens the log as an audit trail during a publish job. Cosmetic: gate the message on at least one first-party spec seen, or emit 'no first-party dependencies' otherwise.

🟡 LOW Unparseable package.json is silently skipped — scripts/check-cohort-ranges.mjs

The try/catch around JSON.parse swallows any parse error and continues, so a manifest with a BOM, trailing comma, or other corruption would be skipped entirely and pass the gate un-checked — the exact failure this script exists to catch would go unnoticed. Prefer surfacing the parse failure (or at least recording it as a failure) rather than continuing silently.

🟡 LOW workspace: pin escapes detection — scripts/check-cohort-ranges.mjs

exactVersion only matches when the spec string is exactly a version (e.g. '0.5.0'). A pnpm 'workspace:0.5.0' spec — which pnpm rewrites to an exact '0.5.0' dependency in the published tarball, reintroducing the duplicate-copy/instanceof problem the gate exists to prevent — would not match because of the 'workspace:' prefix and would pass cleanly. Currently every first-party dep uses 'workspace:^' or a real range, so this is latent, but it is a hole in the check's core invariant. Fix: also reject specs matching /^workspace:(?:^|~|>=)?\d+.\d+.\d+$/ where the protocol resolves to an exact version.


tangletools · 2026-08-17T01:31:57Z · trace

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ 3 Blocking Findings — c6c9fdf1

Full multi-shot audit completed 5/5 planned shots over 7 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 7 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 7 changed files. Global verifier still owns final merge decision.

Full immutable report for this review: trace

Summary comment for this run: full summary


tangletools · 2026-08-17T01:31:57Z · immutable trace

…with

CI installs with pnpm 9 and --frozen-lockfile. The lockfile carried no
overrides block while the root manifest still declares pnpm.overrides for
five packages, so pnpm 9 refused the frozen install with
ERR_PNPM_LOCKFILE_CONFIG_MISMATCH and the publish workflow could not
reach build. Regenerating under pnpm 9.15.9 restores the overrides block
and removes lockfile artifacts that only pnpm 10 and 11 emit.
@drewstone

Copy link
Copy Markdown
Contributor Author

@tangletools review now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants