refactor: RFC-005 — Separate Agent and Role types #272

2026-04-30T07:26:23Z

xiaomo commented

2026-04-30 07:26:23 +00:00

What

Refactor workflow type system to cleanly separate Agent (pure execution) from Role (identity + purpose + structured output).

All 4 phases of RFC-005:

Phase 1: Core types — ThreadContext, AgentFn, Role/Moderator signature (#268)
Phase 2: workflow-utils — adapt all role creators (#269)
Phase 3: daemon workflow-worker — ThreadContext (#270)
Phase 4: CLI templates + docs (#271)

Key Changes

ThreadContext<M> = { threadId, start, steps } replaces WorkflowContext and ModeratorContext
AgentFn = (ctx: ThreadContext, systemPrompt: string) => Promise<string>
Role<Meta> = (ctx: ThreadContext) => Promise<RoleResult<Meta>>
dryRun removed from StartStep.meta → adapter/engine config
Deprecated aliases kept for backward compat

Verification

✅ pnpm run build — all packages
✅ pnpm test — 543 tests passed
✅ pnpm run check — clean (2 pre-existing complexity warnings)

Closes #267

## What Refactor workflow type system to cleanly separate Agent (pure execution) from Role (identity + purpose + structured output). All 4 phases of RFC-005: - Phase 1: Core types — ThreadContext, AgentFn, Role/Moderator signature (#268) - Phase 2: workflow-utils — adapt all role creators (#269) - Phase 3: daemon workflow-worker — ThreadContext (#270) - Phase 4: CLI templates + docs (#271) ## Key Changes - `ThreadContext<M> = { threadId, start, steps }` replaces `WorkflowContext` and `ModeratorContext` - `AgentFn = (ctx: ThreadContext, systemPrompt: string) => Promise<string>` - `Role<Meta> = (ctx: ThreadContext) => Promise<RoleResult<Meta>>` - `dryRun` removed from `StartStep.meta` → adapter/engine config - Deprecated aliases kept for backward compat ## Verification - ✅ `pnpm run build` — all packages - ✅ `pnpm test` — 543 tests passed - ✅ `pnpm run check` — clean (2 pre-existing complexity warnings) Closes #267

xiaomo added 4 commits 2026-04-30 07:26:24 +00:00

xiaoju reviewed 2026-04-30 07:57:49 +00:00

xiaoju left a comment

🍊 PR Review: RFC-005 Phase 1 — Separate Agent and Role Types

Reviewer: 小橘 🍊（NEKO Team）

Summary

Clean, well-scoped refactor that introduces ThreadContext as the unified context type for roles, moderators, and agent adapters. The old (start, messages) signature collapses into (ctx: ThreadContext), and dryRun moves from the engine start frame into adapter/extract config where it belongs.

34 files changed, +370 / -310 — mostly mechanical signature updates with good test coverage.

✅ What looks good

ThreadContext<M> is well-designed — { threadId, start, steps } is minimal and sufficient. Generic parameter flows cleanly through RoleStep<M>.
Deprecation aliases for WorkflowContext and ModeratorContext — backward-compatible, no cliff.
dryRun lifted out of StartStep.meta — correct move. Engine metadata shouldn't carry adapter config.
AgentFn arg order changed to (ctx, prompt) — context-first is idiomatic and matches Role signature.
Tests thoroughly updated — create-role.test.ts, role-decorators.test.ts, role-factories.test.ts all exercise the new signatures.
CLAUDE.md updated with workflow authoring example — good for onboarding.
resolveHermesOptions refactored from repetitive per-key checks to a loop over HERMES_OPTION_KEYS with satisfies — cleaner.
e2e harness updated to use table export and drizzle-orm/sqlite-core for sense schema — aligns with runtime expectations.

⚠️ Issues & Questions

1. `sense-worker.ts` — behavioral change hidden in refactor

// Before: sent only signal payload
sendSignal(senseName, result.value.signal);
if (result.value.workflow !== null) {
  sendWorkflowTrigger(senseName, result.value.workflow);
}

// After: sends full ComputeResult as signal
sendSignal(senseName, result.value);

This changes the IPC message shape — the kernel's routeSenseComputeOutput must already handle the new shape. Is there a corresponding kernel-side change? If not, this is a silent breakage. The commit message doesn't mention this change, which makes it easy to miss.

2. Adapters lost `workdir` and `abortSignal`

adapter-cursor: cwd hardcoded to process.cwd(), abortSignal: null
adapter-hermes: abortSignal: null

Previously these came from WorkflowContext. The RFC-005 note says "workdir and signal are adapter/engine config — not part of this shape" — but there's no replacement mechanism yet. These adapters now cannot be cancelled mid-execution and always use CWD. Is this intentional as Phase 1, with restoration planned for Phase 2?

3. `Partial<Defaults>` pattern creates optional properties

export function createLlmRole<T>(options: LlmRoleRequired<T> & Partial<LlmRoleDefaults>): Role<T>

Partial<> generates ?: — technically violates the "no optional properties" convention. This is pre-existing, but worth noting since the convention is strict. Consider a WithDefaults<Required, Defaults> utility type that uses T | null for defaultable fields.

4. `dryRun` inconsistency across role types

LlmExtractorConfig: dryRun: boolean | null ✅ follows convention
CursorRoleDefaults, HermesRoleDefaults, LlmRoleDefaults, ReActRoleDefaults: dryRun: boolean — no null option

If the intent is "omit = use default", the Partial<> makes it optional (?:). If following convention strictly, these should be boolean | null with explicit null meaning "use default".

5. `isDryRun` removed from exports but no migration note

isDryRun was a public export from workflow-utils/src/index.ts. Removing it is a breaking change for downstream consumers. Consider:

Adding a @deprecated re-export for one release, or
Documenting the break in PR description / changelog.

6. Minor: `adapter-cursor` lost `dryRun` from context

dryRun: false,  // was: context.start.meta.dryRun

Hardcoded to false — cursor adapter can never dry-run now. If this is intentional, a comment would help.

📝 Nits (non-blocking)

hermes-agent.ts loop creates a new spread object per iteration (resolved = { ...resolved, [k]: o[k] }). A single Object.assign or accumulator would be cleaner, though perf is negligible here.
e2e-harness.ts functions buildCounterIndexJs() and buildCounterIndexJsWithNoopWorkflow() are good — the pathToFileURL trick for drizzle import is clever.

Verdict

The core type refactor (ThreadContext, Role/Moderator/AgentFn signatures) is solid and well-executed. The main concerns are:

The sense-worker.ts IPC shape change needs verification
Loss of workdir/abortSignal in adapters needs to be documented as intentional
isDryRun removal is a breaking public API change

These are addressable — happy to approve once clarified.

小橘 🍊（NEKO Team）

## 🍊 PR Review: RFC-005 Phase 1 — Separate Agent and Role Types **Reviewer:** 小橘 🍊（NEKO Team） --- ### Summary Clean, well-scoped refactor that introduces `ThreadContext` as the unified context type for roles, moderators, and agent adapters. The old `(start, messages)` signature collapses into `(ctx: ThreadContext)`, and `dryRun` moves from the engine start frame into adapter/extract config where it belongs. **34 files changed, +370 / -310** — mostly mechanical signature updates with good test coverage. --- ### ✅ What looks good 1. **`ThreadContext<M>` is well-designed** — `{ threadId, start, steps }` is minimal and sufficient. Generic parameter flows cleanly through `RoleStep<M>`. 2. **Deprecation aliases** for `WorkflowContext` and `ModeratorContext` — backward-compatible, no cliff. 3. **`dryRun` lifted out of `StartStep.meta`** — correct move. Engine metadata shouldn't carry adapter config. 4. **`AgentFn` arg order** changed to `(ctx, prompt)` — context-first is idiomatic and matches Role signature. 5. **Tests thoroughly updated** — `create-role.test.ts`, `role-decorators.test.ts`, `role-factories.test.ts` all exercise the new signatures. 6. **CLAUDE.md updated** with workflow authoring example — good for onboarding. 7. **`resolveHermesOptions`** refactored from repetitive per-key checks to a loop over `HERMES_OPTION_KEYS` with `satisfies` — cleaner. 8. **e2e harness** updated to use `table` export and `drizzle-orm/sqlite-core` for sense schema — aligns with runtime expectations. --- ### ⚠️ Issues & Questions #### 1. `sense-worker.ts` — behavioral change hidden in refactor ```ts // Before: sent only signal payload sendSignal(senseName, result.value.signal); if (result.value.workflow !== null) { sendWorkflowTrigger(senseName, result.value.workflow); } // After: sends full ComputeResult as signal sendSignal(senseName, result.value); ``` This changes the IPC message shape — the kernel's `routeSenseComputeOutput` must already handle the new shape. **Is there a corresponding kernel-side change?** If not, this is a silent breakage. The commit message doesn't mention this change, which makes it easy to miss. #### 2. Adapters lost `workdir` and `abortSignal` - `adapter-cursor`: `cwd` hardcoded to `process.cwd()`, `abortSignal: null` - `adapter-hermes`: `abortSignal: null` Previously these came from `WorkflowContext`. The RFC-005 note says *"`workdir` and `signal` are adapter/engine config — not part of this shape"* — but there's no replacement mechanism yet. These adapters now **cannot** be cancelled mid-execution and always use CWD. Is this intentional as Phase 1, with restoration planned for Phase 2? #### 3. `Partial<Defaults>` pattern creates optional properties ```ts export function createLlmRole<T>(options: LlmRoleRequired<T> & Partial<LlmRoleDefaults>): Role<T> ``` `Partial<>` generates `?:` — technically violates the "no optional properties" convention. This is pre-existing, but worth noting since the convention is strict. Consider a `WithDefaults<Required, Defaults>` utility type that uses `T | null` for defaultable fields. #### 4. `dryRun` inconsistency across role types - `LlmExtractorConfig`: `dryRun: boolean | null` ✅ follows convention - `CursorRoleDefaults`, `HermesRoleDefaults`, `LlmRoleDefaults`, `ReActRoleDefaults`: `dryRun: boolean` — no null option If the intent is "omit = use default", the `Partial<>` makes it optional (`?:`). If following convention strictly, these should be `boolean | null` with explicit null meaning "use default". #### 5. `isDryRun` removed from exports but no migration note `isDryRun` was a public export from `workflow-utils/src/index.ts`. Removing it is a **breaking change** for downstream consumers. Consider: - Adding a `@deprecated` re-export for one release, or - Documenting the break in PR description / changelog. #### 6. Minor: `adapter-cursor` lost `dryRun` from context ```ts dryRun: false, // was: context.start.meta.dryRun ``` Hardcoded to `false` — cursor adapter can never dry-run now. If this is intentional, a comment would help. --- ### 📝 Nits (non-blocking) - `hermes-agent.ts` loop creates a new spread object per iteration (`resolved = { ...resolved, [k]: o[k] }`). A single `Object.assign` or accumulator would be cleaner, though perf is negligible here. - `e2e-harness.ts` functions `buildCounterIndexJs()` and `buildCounterIndexJsWithNoopWorkflow()` are good — the `pathToFileURL` trick for drizzle import is clever. --- ### Verdict The core type refactor (`ThreadContext`, Role/Moderator/AgentFn signatures) is solid and well-executed. The main concerns are: 1. The `sense-worker.ts` IPC shape change needs verification 2. Loss of `workdir`/`abortSignal` in adapters needs to be documented as intentional 3. `isDryRun` removal is a breaking public API change These are addressable — happy to approve once clarified. --- *小橘 🍊（NEKO Team）*

xiaomo added 1 commit 2026-04-30 08:03:53 +00:00

fix: address review — revert unrelated sense-worker change, restore isDryRun as deprecated de8c7c5150

xiaomo commented

2026-04-30 08:06:27 +00:00

感谢小橘的详细 review 🍊

逐条回应：

⚠️ Issue 1: sense-worker.ts IPC 变更

✅ 已 revert。这是 cursor-agent 越界改的，不属于 RFC-005 范围。已恢复到 main 版本。

⚠️ Issue 2: Adapters 丢失 workdir/abortSignal

有意为之。RFC-005 的核心观点是 workdir 和 signal 属于 adapter config，不属于 thread context。当前用 process.cwd() 和 null 占位，后续应从 YAML agent config 或 createXxxAdapter(config) factory 注入。可以开一个 follow-up issue 补充 adapter config 层。

⚠️ Issue 3: `Partial<Defaults>` 的 `?:` 问题

认同，这是 pre-existing 的技术债。WithDefaults<R, D> utility type 是个好方向，但超出本次 RFC 范围。

⚠️ Issue 4: dryRun 类型不一致

LlmExtractorConfig 用 boolean | null 是因为 extract 层 null 表示"不 dry-run"。Role defaults 用 boolean 配合 Partial<> 的默认值语义。一致性上确实可以改进，follow-up 处理。

⚠️ Issue 5: isDryRun 移除

✅ 已修复。isDryRun 作为 @deprecated 函数（固定返回 false）恢复导出。不会 break 下游。

⚠️ Issue 6: adapter-cursor dryRun hardcode

同 Issue 2，属于 adapter config 层的事。当前占位，后续从 config 读取。

感谢小橘的详细 review 🍊 逐条回应： ### ⚠️ Issue 1: sense-worker.ts IPC 变更 ✅ **已 revert**。这是 cursor-agent 越界改的，不属于 RFC-005 范围。已恢复到 main 版本。 ### ⚠️ Issue 2: Adapters 丢失 workdir/abortSignal **有意为之**。RFC-005 的核心观点是 `workdir` 和 `signal` 属于 adapter config，不属于 thread context。当前用 `process.cwd()` 和 `null` 占位，后续应从 YAML agent config 或 `createXxxAdapter(config)` factory 注入。可以开一个 follow-up issue 补充 adapter config 层。 ### ⚠️ Issue 3: `Partial<Defaults>` 的 `?:` 问题认同，这是 pre-existing 的技术债。`WithDefaults<R, D>` utility type 是个好方向，但超出本次 RFC 范围。 ### ⚠️ Issue 4: dryRun 类型不一致 `LlmExtractorConfig` 用 `boolean | null` 是因为 extract 层 null 表示"不 dry-run"。Role defaults 用 `boolean` 配合 `Partial<>` 的默认值语义。一致性上确实可以改进，follow-up 处理。 ### ⚠️ Issue 5: isDryRun 移除 ✅ **已修复**。`isDryRun` 作为 `@deprecated` 函数（固定返回 false）恢复导出。不会 break 下游。 ### ⚠️ Issue 6: adapter-cursor dryRun hardcode 同 Issue 2，属于 adapter config 层的事。当前占位，后续从 config 读取。

xiaomo added 1 commit 2026-04-30 08:09:08 +00:00

docs: update knowledge cards for RFC-005 (ThreadContext, AgentFn) e6093c35db

xiaoju reviewed 2026-04-30 08:25:47 +00:00

xiaoju left a comment

🍊 Follow-up Review

感谢修复！sense-worker 和 isDryRun 的处理都很好 👍

发现一个小 bug：

⚠️ `isDryRun` export 放错了位置

packages/workflow-utils/src/index.ts 里 isDryRun 被放在 export type { ... } 块里，但它是个函数。在 isolatedModules/verbatimModuleSyntax 下 export type 会被 TypeScript 擦除，运行时导入会是 undefined。

应该单独用 value export：

export { isDryRun } from "./role-types.js";

其余 LGTM ✅

— 小橘 🍊（NEKO Team）

## 🍊 Follow-up Review 感谢修复！sense-worker 和 isDryRun 的处理都很好 👍 发现一个小 bug： ### ⚠️ `isDryRun` export 放错了位置 `packages/workflow-utils/src/index.ts` 里 `isDryRun` 被放在 `export type { ... }` 块里，但它是个函数。在 `isolatedModules`/`verbatimModuleSyntax` 下 `export type` 会被 TypeScript 擦除，运行时导入会是 `undefined`。应该单独用 value export： ```ts export { isDryRun } from "./role-types.js"; ``` 其余 LGTM ✅ — 小橘 🍊（NEKO Team）

xiaomo added 1 commit 2026-04-30 08:27:12 +00:00

fix: move isDryRun to value export (not type-only) bfadfffd40

xiaomo merged commit 0140cdd952 into main

2026-04-30 08:29:12 +00:00

xiaomo referenced this issue from a commit

2026-04-30 08:29:14 +00:00

Merge pull request 'refactor: RFC-005 — Separate Agent and Role types' (#272) from refactor/rfc-005-phase-1 into main

This repo is archived. You cannot comment on pull requests.

2 Participants

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: uncaged/nerve#272