refactor!: close architecture review findings and unify shared contracts - #124
Merged
Conversation
The hand-written hostOf() did not treat '\' as an authority terminator, while WHATWG URL (and thus the host's real fetch) folds '\' into '/' for special schemes. 'https://evil.com\@allowed.com/x' was judged as allowed.com after userinfo stripping but actually fetched evil.com, bypassing the remote baseUrl allowlist. Backslash now terminates the authority, matching real fetch semantics; scheme:\\host forms fail closed (no host -> denied).
checkRegisterPath compared path segments literally while storage (canonicalizePath) and scope matching both fold to lowercase. Two user-visible behavior changes, both previously wrong: - 'System/x' no longer bypasses the reserved-root guard (the node would have materialized at 'system/x' despite the guard never firing); deployment-appended reservedRoots entries are folded the same way. - registerPaths declared with uppercase segments (e.g. 'Docs') now match their lowercase canonical targets instead of silently denying all. Segment splitting now reuses tree/path segments() so inner empty segments fail closed instead of being silently dropped.
…ox reverify Device hello is the third NodeConfig write entry, but its per-node checkRegisterPath never received TbAppDeps.reservedRoots, and neither did the mailbox claim/renew/complete re-verification. A device SK holding register scope could mount under a deployment-appended reserved root that ~register and system/registry would reject. Behavior change for existing deployments that configure reservedRoots (SDK hosts via ToolBridgeConfig.reservedRoots): device hello targeting those roots now fails permission_denied instead of mounting, and mailbox re-verification of such mounts now denies. Hosts that never set reservedRoots (gateway/server today) are unaffected. processDeviceHello gains an optional reservedRoots option; gateway and server callers are unchanged (they have no such setting to thread). @tool-bridge/app 0.19.0 -> 0.20.0 (0.x minor: accept->reject change).
- Centralize Web-global declares in webGlobals.ts (9 files had drifting
local 'declare const' copies); module-export form keeps downstream
tsconfig libs (DOM/workers/node) from conflicting.
- Make core fully erasableSyntaxOnly-compatible: convert 11 parameter
properties across 8 files to explicit field assignments, so consumers
compiling core sources under that flag (dashboard) can include any
core type chain.
- Delete verified-dead code: filterVisible, STORE_COMMANDS (+StoreCommand),
8 createXxxModule factory re-exports, prepareToolSearchUnits (tests now
exercise prepareToolSearchQuery). parseHelpDsl kept: app integration
tests consume it.
- assertNoCollision comment now names the real enforcement sites (mcp
adapter + device hello materialization).
- Perf shapes: countNodes uses a single subtree('') scan instead of
O(N^2/page) paging (rootSnapshot rejected: it truncates at the derived
index budget); auth/sk list batches reads via getMany.
- Single base64 implementation: oauth.ts hand-rolled copy replaced by
encoding/base64url (new base64Encode); public export moved off the
secretStore re-export onto the index.
- toolSearchFixedStatements factory dedupes the 14 fixed SQL statements
between sqlite and pg dialects (dialects now inject only placeholder
style and ::int casts).
- Unified contentType normalization (context/contentType.ts). Behavior
change: context create_upload now lowercases contentType (was
trim-only), matching the Store upload path.
- create_upload host-only exclusion is command-registry metadata
(hostOnly flag) instead of 4 hardcoded checks.
- HelpJson cmds/children derive from CmdSpec/ChildRef instead of
field-by-field copies.
- device/public.ts narrowed: mailbox 'export *' reduced to the one
consumed type (DeviceOperationCompletion); presence surface
(derivePresence & types) now exported for SDK /client consumption.
- Named view types for downstream reuse: SecretKeyView, SecretKeyCreated,
SecretEntrySummary; catalog detail types exported; ACTIONS exported via
core/protocol.
Extracted from the architecture review: five safety/consistency contracts were maintained as 2-3 hand-written copies each. app: - registryMutation.ts: assertNodeConfigMutation unifies the node-config validation chain (remote allowlist -> register path -> secret-ref -> mcp-oauth -> context -> skillhub -> tool) for system/registry and ~register; invalidateNodeDerivedState unifies the four-step derived state invalidation (3 call sites). Parity test asserts both entrances reject identical inputs identically. - deviceLifecycle.ts: reverifyDeviceAuthority / markDeviceDisconnected / reclaimDeviceSubtree / deviceSearchCapacityWarning sink the device authority re-check, disconnect bookkeeping, subtree reclaim ordering and capacity warning out of both hosts; gateway DO and server hub keep only scheduling and connection-identity concerns. - mcp bridge remote discovery goes through RemotePathProjector.projectHelp (hand-rolled rebase deleted); malformed or out-of-containment upstream paths are now rejected (intended hardening). - compactTool.ts unifies search compact truncation (2 copies); cursorCleanup.ts unifies the CAS-cursor cleanup skeleton + positiveInt (2 copies); objectProviderOpts unifies relayRefUrl assembly; pure-forwarding contextNodes wrappers deleted (callers import core); OAuth STATE_TTL_SEC/callback-path/base64url now shared; toolNodes consume the assembly-time SearchSynchronizer instead of re-creating it. - deviceMailbox endpoints register as fixed literal routes (wildcard startsWith dispatch removed); trailing-slash variants now 404. - device hello rejects case-fold collisions in exposed node paths (assertNoCollision, fail closed; was silent last-write-wins). hosts: - gateway public Env drops the TB_TEST_* test-only fields. - gateway test adapts to core's prepareToolSearchQuery. Intentional behavior changes: ~register checks register scope before the remote allowlist (dual-failure now 403, aligned with invoke); search capacity warning wording unified; server hub short-circuits invoke before identify when hello never completed; mailbox trailing-slash 404; hello collision fail-closed; cleanup without CAS -> TBError unavailable.
…ity surface - src/shared/transport.ts owns the single status->(code,retryable) fallback map, TBError body parsing, reserved-credential-header validation and timeout validation for all three web subentries (the same gateway 404 previously normalized to three different codes). Call-site messages and redaction policies unchanged. - StoreClientOptions.timeoutMs (same semantics as the fixed control-plane client): timeouts surface as retryable unavailable; caller aborts still propagate as AbortError. Removes the need for CLI-side fetch wrappers. - /client now exports the builtin/system view types (PluginView, CatalogListItem, SecretKeyView, Skill*, FederationHost, ...) type-only from core, plus runtime ACTIONS and derivePresence - the single authority surface that ends hand-copied contracts in CLI/Dashboard. tsconfig.web/tsup dts resolve core main as source entries. - Drop PreparedDeviceCredential.url (speculative ticket-auth shape; connections always use the internally built device WS URL). - Delete the root storeClient.ts compatibility shim; root re-exports ./store modules directly. toolBridge imports dispatchContextCmd from core (app forwarding layer removed). Intentional behavior unification: store/device fallback codes for non-TBError bodies now follow the shared map (404 -> not_found non-retryable, 429 -> rate_limited retryable, 5xx -> unavailable).
- parseKeyValueSpecs/parseFieldSpecs: one k=v parser behind 8 call sites; the two same-named parseFields serving --field now agree (duplicate key -> error, empty value allowed, no trim). - stdin.ts: one low-level reader (async iteration; readFileSync(0) dropped); credential inputs strip exactly one trailing newline (CRLF-aware) at both entrances. - parseObjectStorageMountOpts shares the r2/s3 mount validation between ctx and skillhub mounts; single contentType table (union of the three copies); single parsePositiveInt (strictest semantics). - Hand-copied contracts deleted: scope.ts Action/Scope/ACTIONS re-export core; types.ts NodeConfig/Virtualize/HttpToolDef/SecretKey*/Context* come from core (fixes the drifted tool variant and removes the 'as NodeConfig' escape hatches); integration.ts catalog shapes and plugin.ts manifest shapes import core (PluginView). User-visible changes (also in PR body): integration --field empty-value accepted / duplicate rejected; --key-stdin no longer full-trims; secret set stdin strips CRLF as a unit; wider contentType inference (.pdf/.mp4/ .mov/.webm/.mp3/.wav); --ttl '' now rejected and --ttl bounded by isSafeInteger.
…uthority - mountOrchestration.ts: the secret/set -> registry/write -> conditional orphan-credential rollback chain (with its 'upsert + incomplete list must not delete' safety criterion) and the OAuth follow-up toast exist exactly once; useMountRunner, MountDialog, IntegrationDialog and the RegistryPage authorize button all consume it. Wizard step diagnostics and dialog single-line errors keep their own presentation. - lib/format.ts + lib/useDebounced.ts replace per-file copies in Context/Skill browsers (humanTime signature drift resolved). - useCanvasTree uses useKeyBase() instead of hand-assembled query keys; dead useInvalidateTree removed. - Hand-copied contracts deleted: lib/types.ts re-exports the builtin view types from @tool-bridge/sdk/client (PluginView as PluginManifest, SecretKeyView as SecretKeyInfo aliases keep internal names); scope/ ACTIONS from the same surface; lib/presence.ts re-exports derivePresence from the SDK (UI LABEL/HINT/TONE tables stay local). derivePresence call sites pass an explicit ISO 'now' per the core contract; presence semantics tests live in core, dashboard keeps a re-export smoke + UI table completeness test.
…alize
- _runtime/authedClient.ts: createAuthedClient collapses the per-provider
hand-written request/errorMessage boilerplate into declarative config
(four auth header shapes + custom escape hatch; errorMessage {keys,
fallback} extraction or full mapError override, mutually exclusive).
Pilots migrated: exa (declarative), readwise (custom extractor),
postmark (ErrorCode-table mapError). Wire-level assertions unchanged.
- feishu no longer runtime-imports @tool-bridge/core (devDependency):
plugin-sdk re-exports isTBError/normalizeUpstreamError and feishu
consumes the author surface like every other provider.
- scripts/migrate/normalizeSchema.mjs is the single normalize
implementation (NORMALIZE_VERSION lives there); parity.mjs,
fingerprint.mjs, index.mjs, report.mjs and schemaParity.test.ts all
import it - the two manually-synced copies are gone.
Remaining providers are classified for follow-up migration (20 mechanical
+ 47 with custom mapError + 27 unsuited: query/path auth or raw
guardedFetch).
Two speculative wire channels shipped end-to-end but could never do anything (pre-launch is the cheapest deletion window): - commandId: the mailbox parser forced it equal to operationId and enqueue wrote it as such; ~10 wire/state occurrences, zero consumers. Removed from wire schemas, durable operation records and fixtures. Pre-launch: no migration for existing durable records (none in production). - mode 'semantic' on the GLOBAL tool search: no SearchIndex in the repo ever declared or implemented 'search:semantic'; the only reachable behavior was rejection. Wire enum, SearchCapability union, cursor 'm' field, route guards, federated-search branch, mcp bridge schema and 'tb search --mode' collapse to keyword-only. Requests carrying mode:'semantic' are now rejected by the wire schema (same terminal outcome as before, earlier). The CONTEXT search mode enum is intentionally untouched: it is plugin-protocol passthrough that external context providers can implement. Also binds the four bare schema/interface duplicates in wire.ts (toolSearchRequest, health, liveness, readiness) with z.ZodType<T> annotations so drift is a compile error, matching the file's dominant discipline.
Batch migration of the per-provider hand-written request()/errorMessage()
HTTP boilerplate onto the declarative _runtime/authedClient helper
(follow-up to the pilot exa/readwise/postmark migration). Wire-level
behavior is unchanged: URLs, header sets/casing, error codes and error
message wording are covered by per-provider tests (plugins suite: 4045
tests green).
- A-class providers declare auth header shape + errorMessage {keys,
fallback}; B-class keep provider-specific mapError/mapTransportError
logic while the helper owns auth + transport.
- Known nuance (perplexity/vercel): a JSON string-literal error body now
yields its content instead of the fallback template; no test or known
upstream exercises this.
- linear intentionally not migrated (GraphQL 200-with-errors envelope;
grouped with the C-class follow-up: query/path-auth providers and raw
guardedFetch users stripe/twilio/railway/convertapi/googledocs).
Export PluginOAuth as an explicit interface and bind its Zod schema back to that interface. This preserves compile-time schema drift checks without making the public SDK declaration bundle reference an unpacked Zod-internal path, which only the tarball verifier exposed.
Bump CLI 0.30.0, Dashboard 0.27.0, Gateway 0.25.0, plugin-sdk 0.8.0, SDK 0.21.0, and Server 0.21.0 because this round changes consumer-visible validation, wire/type exports, host Env types, SDK fallback semantics, and bundled runtime behavior. Keep App at its already-bumped 0.20.0 and align the Deploy Button template with the new Dashboard/Gateway minors.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
概要
这轮从全仓架构 review 出发，修复安全边界缺口，删除不可达的协议通道，并把 App、SDK、CLI、Dashboard 与 plugins 中重复实现的契约收敛到单一 owner。
主要结果：
commandId和全局工具搜索 semantic 通道；Context provider 的 semantic mode 保留。createAuthedClient；Linear 与 query/path/GraphQL/raw streaming 类边界留作明确 follow-up。明确的行为与兼容性变化
这不是纯内部重构，升级后以下行为会变化：
registerPaths与reservedRoots按 canonical lowercase segment 比较；device hello/mailbox 与两条 HTTP 注册入口执行同一组 deployment 约束。commandId字段，不迁移既有 durable 记录（项目仍处 pre-launch）；全局/~search的mode:'semantic'现在在 strict wire schema 处拒绝，toolSearchcapability 也删除search:semantic。Context provider 的 search mode 不受影响。~register：register scope 判定前移；白名单和 scope 同时失败时现在返回 403，而不是先返回 400。/~device/**子路径不再先读取请求 body。expose.nodes中大小写折叠冲突由静默后写覆盖改为invalid_argument，且不落任何节点。projectHelp；畸形或越界上游路径从可能被接受改为 fail closed。not_found（non-retryable）、429 →rate_limited、5xx →unavailable；新增 StoretimeoutMs；删除未使用的PreparedDeviceCredential.url与根级storeClient.ts兼容壳。integration --field空值接受、重复 key 拒绝；credential stdin 只剥一个 CRLF-aware 尾换行并保留其他空白；secret setstdin 剥一个\r\n；content type 表扩展；--ttl校验收紧；tb search --mode只接受keyword。create_upload.contentType统一 trim + lowercase；@tool-bridge/core/device不再导出 mailbox 的 server 侧符号。Env类型删除五个TB_TEST_*字段。isTBError/normalizeUpstreamError导出。版本
@tool-bridge/app：0.20.0（本轮前段已 bump）@tool-bridge/cli：0.30.0@tool-bridge/dashboard：0.27.0@tool-bridge/gateway：0.25.0@tool-bridge/plugin-sdk：0.8.0@tool-bridge/sdk：0.21.0@tool-bridge/server：0.21.0core与plugins为 private，不单独发布。合入 main 后再按依赖拓扑逐包发布；本 PR 不提前打 tag。验证
pnpm verifypnpm turbo run build：7/7 build tasks 通过，Gateway/Server 实际 bundle 包含迁移后的 provider。/client声明含PluginView、SecretKeyView、derivePresence。scripts/pack-and-verify-package.mjs --skip-install：CLI、Dashboard、Gateway、plugin-sdk、SDK、Server 六个最终 tarball 均通过 manifest、入口、协议与 JS/d.ts closure 检查。本轮明确不做