feat: [ENG-2929] add brv channel core interfaces and enforce server/c…#745
Conversation
…ore import boundary.
|
Claude finished @leehpham's task in 5m 35s —— View job Claude Code Review — feat: [ENG-2929] add brv channel core interfaces and enforce server/core import boundary
SummaryOverall score: 4 / 5 This is a clean, well-scoped foundation PR. It adds six new core-layer interfaces for the channel subsystem, enforces a The interfaces are well-shaped: ports only, args-objects throughout, I could not run What's strong
Findings (none blocking)
Out of scope / observations (not flagged inline)
Verification
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| ResourceLinkContentBlockSchema, | ||
| EmbeddedResourceContentBlockSchema, | ||
| ]) | ||
|
|
There was a problem hiding this comment.
issue (non-blocking): This file is touched only for blank-line additions, but the same PR adds a CLAUDE.md rule forbidding milestone labels and open-question numbers in code/doc comments. The module-level doc at the top of this file violates the new rule in three places:
- line 6:
subsystem (M0-1) - line 14:
(open question Q2) - line 18-19:
re-introduced in M5+
The TODO(ENG-3034) markers added in curate-prompt-builder.ts and i-curate-executor.ts lead by example for the exception clause; cleaning these three sites in the same PR would do the same for the rule itself. Restate the durable why (e.g. "deferred until cross-machine member shapes are defined") instead of the milestone label.
| /** Stops and evict the driver for a single key. No-op when absent. */ | ||
| release(args: DriverPoolReleaseArgs): Promise<void> | ||
|
|
||
| /** Stops and evict every driver in the pool (daemon shutdown). */ | ||
| releaseAll(): Promise<void> | ||
|
|
||
| /** Stops and evict all drivers belonging to a channel (channel close / archive). */ | ||
| releaseChannel(channelId: string): Promise<void> |
There was a problem hiding this comment.
nit (style): Per the new CLAUDE.md rule, doc comments on functions/methods should start with a third-person singular present-tense verb. These three docs start with a third-person verb but then coordinate with a bare imperative (evict) where the conjugated form (evicts) is needed:
- line 38: "Stops and evict the driver for a single key." → "Stops and evicts the driver for a single key."
- line 41: "Stops and evict every driver in the pool" → "Stops and evicts every driver in the pool"
- line 44: "Stops and evict all drivers belonging to a channel" → "Stops and evicts all drivers belonging to a channel"
| /** Stops and evict the driver for a single key. No-op when absent. */ | |
| release(args: DriverPoolReleaseArgs): Promise<void> | |
| /** Stops and evict every driver in the pool (daemon shutdown). */ | |
| releaseAll(): Promise<void> | |
| /** Stops and evict all drivers belonging to a channel (channel close / archive). */ | |
| releaseChannel(channelId: string): Promise<void> | |
| /** Stops and evicts the driver for a single key. No-op when absent. */ | |
| release(args: DriverPoolReleaseArgs): Promise<void> | |
| /** Stops and evicts every driver in the pool (daemon shutdown). */ | |
| releaseAll(): Promise<void> | |
| /** Stops and evicts all drivers belonging to a channel (channel close / archive). */ | |
| releaseChannel(channelId: string): Promise<void> |
| /** | ||
| * Dispatches a prompt and stream the turn as it unfolds. Each yielded | ||
| * {@link TurnEventPayload} is a base-field-free slice; the orchestrator stamps | ||
| * `channelId` / `turnId` / `deliveryId` / `memberHandle` / `seq` / `emittedAt` | ||
| * before persisting and broadcasting. The iterator completes when the turn | ||
| * reaches a terminal state and may throw to signal a driver-level failure. | ||
| */ | ||
| prompt(args: AgentDriverPromptArgs): AsyncIterableIterator<TurnEventPayload> |
There was a problem hiding this comment.
nit (style): Per the new CLAUDE.md "third-person singular present-tense" rule, the leading verb is conjugated correctly but the coordinated second verb is not:
"Dispatches a prompt and stream the turn as it unfolds."
Should be "streams" to match "dispatches".
| /** | |
| * Dispatches a prompt and stream the turn as it unfolds. Each yielded | |
| * {@link TurnEventPayload} is a base-field-free slice; the orchestrator stamps | |
| * `channelId` / `turnId` / `deliveryId` / `memberHandle` / `seq` / `emittedAt` | |
| * before persisting and broadcasting. The iterator completes when the turn | |
| * reaches a terminal state and may throw to signal a driver-level failure. | |
| */ | |
| prompt(args: AgentDriverPromptArgs): AsyncIterableIterator<TurnEventPayload> | |
| /** | |
| * Dispatches a prompt and streams the turn as it unfolds. Each yielded | |
| * {@link TurnEventPayload} is a base-field-free slice; the orchestrator stamps | |
| * `channelId` / `turnId` / `deliveryId` / `memberHandle` / `seq` / `emittedAt` | |
| * before persisting and broadcasting. The iterator completes when the turn | |
| * reaches a terminal state and may throw to signal a driver-level failure. | |
| */ | |
| prompt(args: AgentDriverPromptArgs): AsyncIterableIterator<TurnEventPayload> |
| /** Current lifecycle status. */ | ||
| readonly status: AgentDriverStatus | ||
|
|
||
| /** Tears the session down and release resources. Idempotent. */ |
There was a problem hiding this comment.
nit (style): Same coordinated-verb mismatch as on prompt:
"Tears the session down and release resources."
Should be "releases" to match "tears".
| /** Tears the session down and release resources. Idempotent. */ | |
| /** Tears the session down and releases resources. Idempotent. */ |
| /** | ||
| * Payload-only `TurnEvent`: a variant's fields WITHOUT the base coordination | ||
| * metadata (`channelId`, `turnId`, `deliveryId`, `memberHandle`, `emittedAt`, | ||
| * `seq`). A driver is oblivious to channel-side coordinates — the orchestrator | ||
| * stamps the base fields (including the gap-free monotonic `seq`) as it relays | ||
| * each payload into the transcript, so correct ordering can only be assigned by | ||
| * that single writer. | ||
| * | ||
| * Derived structurally from {@link TurnEvent} via a distributive conditional so | ||
| * the two can never drift: adding a new `TurnEvent` variant updates this type | ||
| * automatically. The omitted keys MUST mirror the `TurnEvent` base shape. | ||
| */ | ||
| export type TurnEventPayload = TurnEvent extends infer T | ||
| ? T extends TurnEvent | ||
| ? Omit<T, 'channelId' | 'deliveryId' | 'emittedAt' | 'memberHandle' | 'seq' | 'turnId'> | ||
| : never | ||
| : never |
There was a problem hiding this comment.
suggestion (robustness, non-blocking): TurnEventPayload is a brilliant derived type via distributive conditional, but the Omit list is a hand-maintained literal mirror of the TurnEventBaseShape keys defined in src/shared/types/channel.ts:241-248. The two cannot drift today, but nothing prevents them from drifting tomorrow — adding a base field to TurnEventBaseShape silently leaks it through every payload yielded by every IAgentDriver, and TypeScript will not flag it.
The doc comment acknowledges this ("The omitted keys MUST mirror the TurnEvent base shape") — that's an invariant a type check could enforce. Consider exporting a TurnEventBaseFields shape (or just the key-union type TurnEventBaseKey = 'channelId' | 'turnId' | ... derived from the schema) from shared/types/channel.ts and using Omit<T, TurnEventBaseKey> here. The cost is one shared key-union; the gain is "MUST mirror" becomes structurally enforced instead of comment-policed.
Not blocking for this PR, but worth a follow-up before there's more than one driver implementing the interface.
| */ | ||
| export interface IChannelStore { | ||
| /** Persists a new member record under a channel. */ | ||
| addMember(args: ChannelStoreAddMemberArgs): Promise<void> |
There was a problem hiding this comment.
question (spec gap): This contract is silent on collision semantics. createChannel explicitly states "rejects when channelId already exists" (line 46) and removeMember explicitly states "No-op when absent" (line 58) — both excellent. But addMember does not say what happens if a member with the same handle already exists under that channelId. Three behaviors are all defensible:
- Reject (invariant: handles are unique per channel; caller must check)
- Overwrite (last-write-wins; idempotent re-invite)
- No-op (first-write-wins; idempotent re-invite)
Each implies a different invite/re-invite flow. Picking one and documenting it here is cheaper than discovering the divergence between two infra adapters later.
Same kind of gap on IChannelOrchestrator.cancelTurn (i-channel-orchestrator.ts:39) — what's the behavior when turnId doesn't exist or is already in a terminal state?
| */ | ||
| export interface ITranscriptStore { | ||
| /** Appends one stamped `TurnEvent` to a turn's log. */ | ||
| appendTurnEvent(args: AppendTurnEventArgs): Promise<void> |
There was a problem hiding this comment.
question (spec gap): The contract doesn't say what appendTurnEvent does when an event with an already-persisted seq arrives, or when events are appended in non-monotonic seq order. Given the architectural note in i-agent-driver.ts:3-9 that the orchestrator is the sole writer of seq and treats it as gap-free monotonic, two defensible behaviors:
- Trust the orchestrator — append as-given; out-of-order is the caller's bug.
- Defensive — reject on gap / duplicate / regression so a buggy orchestrator can't poison the transcript.
Either is fine, but stating it here is cheap and pins down what an adapter must implement. Same kind of clarity that createChannel already provides on IChannelStore.
| { | ||
| group: ['**/server/infra/**', '../infra/**', '../../infra/**', '../../../infra/**', '../../../../infra/**'], | ||
| message: | ||
| 'core must not import from server/infra. Depend on an interface in core/interfaces and let infra implement it (dependency inversion).', | ||
| }, |
There was a problem hiding this comment.
nit (defensive): The sibling-relative patterns top out at ../../../../infra/** (4 ups). Current max depth in src/server/core/ is interfaces/<a>/<b>/file.ts → ../../../infra/ to reach src/server/infra/, so today there's one level of headroom. The day someone adds src/server/core/interfaces/channel/subdir/another/file.ts, the sibling-relative escape route reopens: a 5-up relative import like ../../../../../infra/foo.js won't match **/server/infra/** (no literal server/infra/ substring) and won't match ../../../../infra/** (one .. short).
Cheap fixes (either is fine):
- Add
../../../../../infra/**and../../../../../../infra/**to the list for headroom. - Or, since the rule message says "core must not import from server/infra", prefer a single pattern based on
import/no-restricted-pathsif available, which resolves the import to an absolute file path before matching — that closes the relative-import escape hatch by construction.
Not blocking — the rule does what it advertises at the present directory depth. Worth a follow-up before the channel subsystem nests deeper.
| **Comments / JSDoc**: | ||
| - Doc comments for functions and methods start with a third-person singular present-tense verb (`Creates a channel`, `Returns the driver`, `Appends an event`), not the imperative (`Create`, `Return`, `Append`). This applies to method/function descriptions; type, property, and parameter docs are not constrained this way. |
There was a problem hiding this comment.
praise: Strongly agree with formalising the "Linear IDs / milestone labels don't belong in code" guidance — these always rot. The carve-out for TODO(ENG-1234) against a tracked follow-up is exactly the right shape: it points at the issue tracker as the source of truth instead of trying to mirror it inside the comment.
The TODO(ENG-3034) markers added in curate-prompt-builder.ts and i-curate-executor.ts model the exception clause nicely; cleaning up the pre-existing M0-1 / Q2 / M5+ violations in src/shared/types/channel.ts (which is touched by this PR) would model the rule clause in the same change — see the comment on that file.
1 participant
…ore import boundary.
Summary
Type of change
Scope (select all touched areas)
Linked issues
Root cause (bug fixes only, otherwise write
N/A)Test plan
User-visible changes
List user-visible changes (including defaults, config, or CLI output).
If none, write
None.Evidence
Attach at least one:
Checklist
npm test)npm run lint)npm run typecheck)npm run build)mainRisks and mitigations
List real risks for this PR. If none, write
None.