fix review findings: document the util/ group + align branded-ids RFC with dsh-brand
Adding packages/util/brand/ created a new top-level packages/util/ group that the hierarchy/dependency docs never enumerated. Document it: - Add packages/util/README.md, the group README (low-level zero-dependency utilities shared across groups; lists dsh-brand). - packages/README.md: add the util/ group to the group table, dsh-brand to the package table, and dsh-brand to the dependency graph. Correct the now-false "no harness deps" claims — dsh-llm and dsh-bash both depend on dsh-brand (verified dsh-bash imports Branded from dsh-brand, not dsh-llm; dsh-session and dsh-agent depend on it too). - Root AGENTS.md Repository Layout: add the util/ group with brand/. Align the implemented branded-ids RFC with what shipped: Branded lives in @deepseek-ai/dsh-brand (packages/util/brand/), and dsh-bash depends only on that utility package instead of dsh-llm. Fix the BashTaskId import source, the illustrative snippet, and the opening policy reference (now dsh-brand).
This commit is contained in:
Tianyi Cui
@@ -71,6 +71,9 @@ packages/ Harness packages, grouped by role at packages/<group>/<pkg>/.
|
||||
feeds stdin lines to the agent (shared by the demos)
|
||||
llm-replay/ record/replay adapter: short-circuits llm/stream from a
|
||||
recorded session JSONL (keyless snapshot tests)
|
||||
util/ low-level zero-dependency utilities shared across groups
|
||||
brand/ type-only Branded<B> nominal-typing primitive (no runtime
|
||||
code, no harness deps; owns the brand for cross-boundary ids)
|
||||
examples/ Runnable demos (not workspaces; see examples/AGENTS.md). echo-agent
|
||||
= mock model + echo tool + stdio UI + JSONL persistence, wired via
|
||||
cordis.yml. coding-agent = the real thing: DeepSeek V4 + bash tools
|
||||
|
||||
@@ -4,7 +4,7 @@ Status: implemented (proposed and accepted 2026-06-20)
|
||||
|
||||
## Problem
|
||||
|
||||
The harness already brands three identifiers — `CallId` (`packages/llm/llm/src/brand.ts`), `SessionId` (`packages/core/session/src/types.ts`), and `AgentId` (`packages/core/agent/src/types.ts`) — using the `Branded<B> = string & { readonly [BRAND]: B }` machinery and a zero-cost cast factory per type. `brand.ts` also states the governing policy: *"Branding is for IDs that cross package boundaries and could plausibly be confused; not every string needs a brand."* That policy is right; the problem is that it is only half-applied. Two gaps let a structurally-identical-but-semantically-wrong string slip through the type checker today.
|
||||
The harness already brands three identifiers — `CallId` (`packages/llm/llm/src/brand.ts`), `SessionId` (`packages/core/session/src/types.ts`), and `AgentId` (`packages/core/agent/src/types.ts`) — using the `Branded<B> = string & { readonly [BRAND]: B }` machinery (owned by the type-only `@deepseek-ai/dsh-brand` package at `packages/util/brand/` — see its [README](../../../../packages/util/brand/README.md)) and a zero-cost cast factory per type. `dsh-brand` also states the governing policy: *"Branding is for ids that cross package boundaries and could plausibly be confused; not every string needs a brand."* That policy is right; the problem is that it is only half-applied. Two gaps let a structurally-identical-but-semantically-wrong string slip through the type checker today.
|
||||
|
||||
**Gap 1 — unbranded cross-boundary IDs in the bash seam.** The background-task id is a plain `string`: `BashTask.id: string` (`packages/bash/bash/src/types.ts`), carried as `string` through the whole executor seam (`BashExecutor.get`/`ownerOf`/`readOutput`/`kill(id: string)` in `packages/bash/bash/src/index.ts`) and validated/passed as `string` by the model-facing tools (`validateTaskId`, `assertTaskAccess`, the `task_id` schema arg in `packages/bash/tool-bash/src/index.ts`). It is generated by a per-executor counter — `` `bash-${this.nextTaskId++}` `` in `packages/bash/bash-local/src/index.ts` — which gives it **exactly the same `name-N` shape as `SessionId`'s default** (`` `session-${++counter}` `` in `packages/core/session/src/index.ts`). A bash task id and a session id are trivially swappable at a call site and the compiler says nothing. This is the headline case the user asked about, and it is a model-facing id (the model passes `task_id` back to `bash_output`/`bash_kill`), so a confusion here is reachable from untrusted input.
|
||||
|
||||
@@ -16,7 +16,7 @@ The bash **owner token** is the related sub-case: `BashExecRequest.owner?: strin
|
||||
|
||||
A type-only change. Brands are zero-cost casts; nothing about runtime behavior, serialization, comparison, or the wire format changes. The work is in three parts, all honoring the existing "not every string" policy.
|
||||
|
||||
- **Brand the bash task id.** Add `BashTaskId = Branded<'BashTaskId'>` plus its same-named factory in `packages/bash/bash/src/types.ts` (the package that *owns* the id), importing `Branded` from `@deepseek-ai/dsh-llm` exactly as `SessionId`/`AgentId` already do. Thread it through `BashTask.id`, the `BashExecutor` seam methods (`get`/`ownerOf`/`readOutput`/`kill`), the generation site in `dsh-bash-local` (brand the counter output once, at creation), and the `dsh-tool-bash` validate/access surface (`validateTaskId` returns a `BashTaskId`; `task_id` is branded at the tool boundary where the model's string arrives).
|
||||
- **Brand the bash task id.** Add `BashTaskId = Branded<'BashTaskId'>` plus its same-named factory in `packages/bash/bash/src/types.ts` (the package that *owns* the id), importing `Branded` from `@deepseek-ai/dsh-brand` exactly as `SessionId`/`AgentId` already do. The brand primitive lives in the dependency-free `dsh-brand` utility package precisely so `dsh-bash` can brand its ids by depending on it alone — it never pulls in `dsh-llm` (or `dsh-session`) just to reach `Branded`. Thread it through `BashTask.id`, the `BashExecutor` seam methods (`get`/`ownerOf`/`readOutput`/`kill`), the generation site in `dsh-bash-local` (brand the counter output once, at creation), and the `dsh-tool-bash` validate/access surface (`validateTaskId` returns a `BashTaskId`; `task_id` is branded at the tool boundary where the model's string arrives).
|
||||
|
||||
- **Mint a distinct `OwnerToken` brand.** Add `OwnerToken = Branded<'OwnerToken'>` in `packages/bash/bash/src/types.ts`; type `BashExecRequest.owner` / `BashExecSpec.owner` / `BashExecutor.ownerOf` as `OwnerToken | undefined`. The `dsh-tool-bash` consumer casts the agent's `session.header.id` (a `SessionId`) into an `OwnerToken` at the boundary — the one place the two vocabularies meet. The bash seam never imports `dsh-session`. (Rationale in the next section.)
|
||||
|
||||
@@ -25,7 +25,7 @@ A type-only change. Brands are zero-cost casts; nothing about runtime behavior,
|
||||
Illustrative shape (the factory pattern is identical to the three existing brands):
|
||||
|
||||
```ts ignore-check
|
||||
import type { Branded } from '@deepseek-ai/dsh-llm'
|
||||
import type { Branded } from '@deepseek-ai/dsh-brand'
|
||||
|
||||
/** A background bash task handle (generated `bash-N` by the local executor). */
|
||||
export type BashTaskId = Branded<'BashTaskId'>
|
||||
|
||||
@@ -14,17 +14,19 @@ Packages are grouped by modular role at `packages/<group>/<pkg>/`. The group dir
|
||||
| [`session-persistence/`](session-persistence/README.md) | Persistence capability family: the seam + JSONL/SQLite backends | Product — stable surface |
|
||||
| [`ui/`](ui/README.md) | Editor/client integration surfaces (the ACP bridge) | Product — stable surface |
|
||||
| [`support/`](support/README.md) | Dev/test/example infrastructure (invariants, stdio UI, replay adapter) | Support — lower compatibility expectations |
|
||||
| [`util/`](util/README.md) | Low-level zero-dependency utilities shared across groups (the `Branded<B>` primitive) | Support — small, stable, harness-dep-free |
|
||||
|
||||
The split is the point: a package's group says whether it is part of the product API or support/test/example infrastructure, so release and removal decisions do not have to treat every package as an equal public contract. New packages join an existing group; adding a new top-level group is a deliberate act (extend the group READMEs and the hierarchy docs).
|
||||
|
||||
## Dependency graph
|
||||
|
||||
```
|
||||
dsh-llm (no harness deps — pure vocabulary)
|
||||
dsh-bash (no harness deps — abstract executor seam)
|
||||
dsh-session ← dsh-llm
|
||||
dsh-brand (no harness deps — type-only Branded<B> primitive)
|
||||
dsh-llm ← dsh-brand (vocabulary; brands CallId)
|
||||
dsh-bash ← dsh-brand (abstract executor seam; brands BashTaskId/OwnerToken)
|
||||
dsh-session ← dsh-llm, dsh-brand
|
||||
dsh-system-prompt ← dsh-llm
|
||||
dsh-agent ← dsh-llm, dsh-session
|
||||
dsh-agent ← dsh-llm, dsh-session, dsh-brand
|
||||
dsh-tools ← dsh-llm, dsh-system-prompt, dsh-agent
|
||||
dsh-bash-local ← dsh-bash (BashExecutor impl)
|
||||
dsh-tool-bash ← dsh-bash, dsh-tools (bash tool schemas)
|
||||
@@ -61,6 +63,7 @@ The rule: plugins depend on interfaces, never on the concrete loop. `dsh-agent-l
|
||||
| `acp/` | `ui` | Agent Client Protocol bridge: serves the agent to an ACP editor over JSON-RPC stdio | (drives `ctx.agents`/`ctx.sessions`) |
|
||||
| `ui-stdio/` | `support` | Minimal stdio (readline) UI plugin: renders `agent/*` events, feeds stdin lines to the agent | (drives `ctx.agents`) |
|
||||
| `llm-replay/` | `support` | Record/replay adapter: short-circuits `llm/stream` with chunks from a recorded session JSONL (keyless snapshot tests) | (listens on `llm/stream`) |
|
||||
| `brand/` | `util` | Type-only `Branded<B>` nominal-typing primitive (no runtime code, no harness deps) | (none — type-only) |
|
||||
|
||||
Each package has its own `README.md` with purpose, service API, events, extension points, and deliberate non-goals (TODOs).
|
||||
|
||||
|
||||
9
packages/util/README.md
Normal file
9
packages/util/README.md
Normal file
@@ -0,0 +1,9 @@
|
||||
# util/ — low-level shared utilities
|
||||
|
||||
Zero-dependency primitives shared across the other groups. A package lands here when it owns a tiny, foundational type or helper that several capability families need but that belongs to none of them — keeping it out of any one group avoids a capability package depending on an unrelated one just to reach a shared primitive. These are **support** packages: small, stable, and free of harness dependencies.
|
||||
|
||||
| Package | Role |
|
||||
|---|---|
|
||||
| `brand/` | The type-only `Branded<B>` nominal-typing primitive (no runtime code, no harness deps) |
|
||||
|
||||
`dsh-brand` is the canonical case: it owns ONLY the `Branded<B>` helper, so a capability package can brand the ids it owns (`dsh-bash`'s `BashTaskId`/`OwnerToken`, `dsh-session`'s `SessionId`, …) by depending on `dsh-brand` alone, without pulling in an unrelated package just to reach `Branded`.
|
||||
Reference in New Issue
Block a user