Skip to content

refactor(agent): remove CherryClaw branding and the Soul Mode toggle#16726

Open
vaayne wants to merge 51 commits into
mainfrom
remove-cherryClaw
Open

refactor(agent): remove CherryClaw branding and the Soul Mode toggle#16726
vaayne wants to merge 51 commits into
mainfrom
remove-cherryClaw

Conversation

@vaayne

@vaayne vaayne commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Before this PR:

  • Agents had a Soul Mode / Autonomous Mode toggle (configuration.soul_enabled). Only soul-enabled agents got the claw/autonomy MCP tools, the agent-memory MCP server, the workspace persona system prompt (SOUL.md / USER.md / FACT.md), and the autonomous-operation disallowed-tools overlay. Enabling it force-switched the agent to bypassPermissions, and scheduled tasks / channels refused agents without it.
  • CherryClaw branding lingered across the repo: a src/main/ai/agents/cherryclaw/ directory (the engine actually powering soul mode for every agent), a docs/references/cherryclaw/ doc set, brand mentions in system prompts and channel help text, and an agent.cherryClaw.* i18n namespace.

After this PR:

  • Soul-mode behavior is unconditional for every agent: the cherry-tools autonomy tools + agent-memory MCP server are always injected and allow-listed, the PromptBuilder workspace prompt always replaces the SDK preset, and all agents are eligible for scheduled tasks and channels.
  • The soul disallowed-tools overlay is deleted outright: agents keep AskUserQuestion, plan-mode, and worktree tools where the tool registry allows them (Cron* / TodoWrite / NotebookEdit remain blocked by their pre-existing exposure: 'disabled' registry classification). Exception added after review: headless runs (channel-triggered and scheduled-task dispatches) still disallow AskUserQuestion — they have no responder, so a question would stall the run.
  • Review hardening on top of the removal: the auto-approved config tool now rejects channels owned by another agent; notify / config are user-disableable (exposure: 'user') like cron; agents with non-blank user instructions skip the bootstrap onboarding interview; the prompt no longer references the unmounted mcp__skills__skills tool.
  • The soul_enabled flag is fully removed: no UI toggle, no zod schema field, no reads anywhere. permission_mode is now a fully independent setting and its field is always visible in the agent edit dialog.
  • CherryClaw branding is gone: the cherryclaw/ engine directory is flattened into src/main/ai/agents/ (prompt / bootstrap / heartbeat), prompts and channel help are de-branded, the CherryClaw doc set is deleted, and agent.cherryClaw.channels.* / agent.cherryClaw.tasks.* i18n keys are flattened to agent.channels.* / agent.tasks.* across all locales.
  • The old claw MCP server is merged into cherry-tools; autonomy tools now live under mcp__cherry-tools__cron, mcp__cherry-tools__notify, and mcp__cherry-tools__config.
  • Dead code removed along the way: seedWorkspaceTemplates, PromptBuilder.buildToolGuidance, and the always-true branches of composeToolGuidance / adjustAllowedToolsForMcp.

Fixes # N/A

Why we need it and why it was done in this way

CherryClaw's autonomy features were productized as soul mode and recent work already made personality + autonomy the default for new agents. Keeping a toggle (and a brand) for behavior that every agent should have added a second code path, a confusing permission-mode coupling, stale docs, and tool-name drift. Removing the flag and merging the autonomy tools into cherry-tools gives the app one agent behavior instead of two.

The following tradeoffs were made:

  • The soul overlay's interactive-tool bans are not carried over: AskUserQuestion, plan-mode, and worktree tools stay available to every agent unless the tool registry disables them. The assistant role's pre-existing AskUserQuestion disable is preserved explicitly, and headless (channel / scheduled-task) runs keep AskUserQuestion disallowed via a transient dispatch flag.
  • Data-migration shims that reference the legacy name are intentionally kept: the 'cherry-claw''claude-code' read-time type remap in AgentService and the 'cherry-claw-default' id remap migrator. The unrelated cherry-claw.png model icon is untouched.
  • The stored soul_enabled configuration key is simply ignored (the configuration zod schema is .loose()), so no data migration is needed.
  • Existing references to old MCP tool names (mcp__claw__*) are not kept as aliases; callers must use the new mcp__cherry-tools__* names.

The following alternatives were considered:

  • Renaming cherryclaw/ to workspace/ or persona/ instead of flattening — rejected: workspace is heavily overloaded in this codebase and the directory only held a few agent-engine modules.
  • Keeping the toggle but defaulting it on — rejected: leaves the dual code path and the permission-mode coupling in place.
  • Keeping a separate claw MCP server name as a compatibility alias — rejected: this PR removes the CherryClaw product surface rather than preserving a legacy synonym.

Links to places where the discussion took place: N/A

Breaking changes

The Soul Mode / Autonomous Mode toggle is removed from agent settings; every agent now always runs with autonomy features (scheduled tasks, channels, cron/notify/config tools, agent-memory tools, workspace persona prompt). Scheduled tasks and channels no longer require Soul Mode or bypassPermissions.

MCP tool names changed from mcp__claw__* to mcp__cherry-tools__*:

  • mcp__claw__cronmcp__cherry-tools__cron
  • mcp__claw__notifymcp__cherry-tools__notify
  • mcp__claw__configmcp__cherry-tools__config

Documented in v2-refactor-temp/docs/breaking-changes/2026-07-03-remove-soul-mode-toggle.md; no user data migration is required.

Special notes for your reviewer

  • Review focus: whether dropping compatibility aliases for mcp__claw__* is acceptable, whether all Claude Code Agents should default to autonomy tools, and whether the preserved assistant-role AskUserQuestion special case is still correct.
  • git log shows the moved engine files as renames (cherryclaw/prompt.tsagents/prompt.ts, etc.) — review with rename detection on.
  • Verification on the final rebased tree: pnpm lint, pnpm test, and pnpm build:check pass locally. GitHub CI is green: changes, catalog-hand-edit-check, changeset-check, basic-checks, general-test, and render-test passed (notify skipped).

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

  • Branch: This PR targets the correct branch — main for active development, v1 for v1 maintenance fixes
  • PR: The PR description is expressive enough and will help future contributors
  • Code: Write code that humans can understand and Keep it simple
  • Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
  • Upgrade: Impact of this change on upgrade flows was considered and addressed if required
  • Documentation: A user-guide update was considered and is present (link) or not required. Check this only when the PR introduces or changes a user-facing feature or behavior.
  • Self-review: I have reviewed my own code (e.g., via /gh-pr-review, gh pr diff, or GitHub UI) before requesting review from others

Release note

Removed the Soul Mode (Autonomous Mode) toggle from agent settings — all agents now always have autonomy features (scheduled tasks, channels, cron/notify/config tools, agent-memory tools, workspace persona prompt), and the permission mode setting is now independent of autonomy. Action required only for integrations that referenced old MCP tool names: use mcp__cherry-tools__cron/notify/config instead of mcp__claw__cron/notify/config.

@vaayne vaayne requested a review from a team July 3, 2026 15:40
@vaayne vaayne requested review from 0xfullex and DeJeune as code owners July 3, 2026 15:40
vaayne added a commit that referenced this pull request Jul 3, 2026
…ntry

The commit hashes recorded before the rebase onto origin/main are stale;
point the entry at the PR instead.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
@vaayne vaayne marked this pull request as draft July 3, 2026 15:53
vaayne added a commit that referenced this pull request Jul 4, 2026
…n registry design doc

The soul-mode entry now lists all PR #16726 commits and documents the
interactive-tool overlay removal (agents regain AskUserQuestion,
plan-mode, and worktree tools). The declarative-tool-registry design doc
gets a status note marking soul de-gating and the claw→cherry-tools
merge as landed so its claw/soul references read as history.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
vaayne added a commit that referenced this pull request Jul 6, 2026
…ntry

The commit hashes recorded before the rebase onto origin/main are stale;
point the entry at the PR instead.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
vaayne added a commit that referenced this pull request Jul 6, 2026
…n registry design doc

The soul-mode entry now lists all PR #16726 commits and documents the
interactive-tool overlay removal (agents regain AskUserQuestion,
plan-mode, and worktree tools). The declarative-tool-registry design doc
gets a status note marking soul de-gating and the claw→cherry-tools
merge as landed so its claw/soul references read as history.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
@vaayne vaayne force-pushed the remove-cherryClaw branch from e65ef2b to 4b07039 Compare July 6, 2026 06:34
@vaayne vaayne marked this pull request as ready for review July 6, 2026 06:34
vaayne added 16 commits July 6, 2026 14:43
…n process

All agents now run with autonomy enabled, so the soul_enabled branches
collapse: claw + agent-memory MCP servers are always injected, the
mcp__claw__ auto-approve prefix and allowlist wildcards always apply,
AGENT_DISALLOWED_TOOLS (renamed from SOUL_MODE_DISALLOWED_TOOLS) is
always spread, and buildSystemPrompt always uses the PromptBuilder
workspace prompt (the preset claude_code path became unreachable).
AgentTaskService drops the soul/bypassPermissions eligibility gate;
the claw config tool no longer reports soul_enabled.

Renderer, zod schema, i18n, and CherryClaw branding follow in
subsequent commits.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
…schema

With soul-mode behavior unconditional in the main process, the flag has
no remaining effect: remove the SoulModeField toggle, the soulEnabled
form state and its bidirectional coupling with bypassPermissions
(permission_mode is now a fully independent setting), the soul-gated
composer placeholder branch, the soul_enabled: true seeds on agent
creation presets, the scheduled-task agent eligibility filter, and the
channels soulModeRequired warning. soul_enabled is removed from the
configuration zod schema; the schema is .loose() so stored configs
keep parsing with the stale key ignored.

Orphaned i18n keys are removed in a follow-up commit.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
…gents engine

The former cherryclaw/ directory is the default prompt/heartbeat engine
for every agent now, so its three modules (prompt, seedWorkspace,
heartbeat) move up into src/main/ai/agents/. Delete the dead
seedWorkspaceTemplates and buildToolGuidance paths and simplify
composeToolGuidance (claw guidance always applies). De-brand prompt and
channel-security wording, drop the CherryClaw docs set and its image,
and update architecture doc trees.

Kept intentionally: the 'cherry-claw' → 'claude-code' read-time type
remap in AgentService and the 'cherry-claw-default' id remap migrator —
both are data-migration shims for existing databases. agent.cherryClaw.*
i18n keys are renamed in the next commit.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
…rryClaw namespace

Delete the orphaned soul keys (library.config.agent.field.soul_enabled.*,
agent.settings.soulMode.*, the channels soulModeRequired warning, the
obsolete permission_mode.help text, and the unreferenced
agent.cherryClaw soul/sandbox/scheduler/warning/heartbeat groups) and
rename agent.cherryClaw.channels.* / agent.cherryClaw.tasks.* to
agent.channels.* / agent.tasks.* across all twelve locale files, with
code references updated. agent.input.soul_placeholder becomes
agent.input.agent_placeholder (the {{key}} placeholder variant is still
used by the missing-agent composer, so both keys remain). Brand-suffixed
strings like "bypassPermissions (Soul)" are neutralized, and lingering
soul-mode comment references in main are cleaned up.

Adds the breaking-changes entry documenting the removed Autonomous Mode
toggle.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
…ntry

The commit hashes recorded before the rebase onto origin/main are stale;
point the entry at the PR instead.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
…verlay

Remove AGENT_DISALLOWED_TOOLS (the former soul-mode overlay): agents
keep AskUserQuestion, plan-mode, and worktree tools. Cron*, TodoWrite,
and NotebookEdit stay blocked regardless — they are classified
exposure: 'disabled' in the declarative tool registry, which was always
the authoritative layer for them. The assistant role's pre-existing
AskUserQuestion disable is restored explicitly.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
Last CherryClaw branding residue: the in-process autonomy MCP server
'claw' becomes 'cherry', so its tools are now mcp__cherry__cron /
mcp__cherry__notify / mcp__cherry__config. Registry keys, the
auto-approve prefix and allowlist wildcard, condition predicates,
prompts (bootstrap + channel security + tool guidance), the
agent.tools.builtin.CherryCron i18n label key, and tests follow.

Prefix matching keeps the trailing __ delimiter, so mcp__cherry__ and
mcp__cherry-tools__ cannot shadow each other; a regression test guards
this. Agents whose persisted disabledTools contain the old
mcp__claw__cron name lose that override (accepted pre-release).

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
Fold the standalone cherry MCP server (cron/notify/config) into
cherry-tools so all injected builtin tools live under one server and
one mcp__cherry-tools__ namespace. The autonomy handlers move verbatim
to cherryAutonomyTools.ts; CherryBuiltinToolsServer registers them only
when constructed with a per-session agent context. cron/notify/config
join the explicit auto-approve list (they were blanket-allowed as
mcp__cherry__* before), while kb_manage keeps its per-call approval.

Pre-release break: persisted disabledTools entries using the old
mcp__cherry__cron name lose their override; no remap shim added.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
… merge

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
The server's only production call site (settingsBuilder) always has the
session's agent context, so the optional no-context mode was speculative
flexibility with zero users. Make the context required and drop the
conditional autonomy-tool registration plus its dead-mode test. Also fix
the builtinTools comment: the tool registry hardcodes the runtime names
and does not import these constants.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
The branch deleted seedWorkspaceTemplates, leaving only the bootstrap
instructions and the SOUL.md content threshold — nothing in the file
seeds a workspace anymore.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
…n registry design doc

The soul-mode entry now lists all PR #16726 commits and documents the
interactive-tool overlay removal (agents regain AskUserQuestion,
plan-mode, and worktree tools). The declarative-tool-registry design doc
gets a status note marking soul de-gating and the claw→cherry-tools
merge as landed so its claw/soul references read as history.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
Soul-mode removal made the soul example prompt ('Help me connect to
Telegram') the placeholder for every agent session, dropping the send
shortcut and slash-search hints. Restore the generic placeholder and
delete the now-orphaned agent.input.agent_placeholder key from all
locales.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
Signed-off-by: Vaayne <liu.vaayne@gmail.com>
Signed-off-by: Vaayne <liu.vaayne@gmail.com>
Signed-off-by: Vaayne <liu.vaayne@gmail.com>
@vaayne vaayne force-pushed the remove-cherryClaw branch from 4b07039 to a8e1a84 Compare July 6, 2026 06:49
vaayne and others added 3 commits July 6, 2026 15:34
Signed-off-by: Vaayne <liu.vaayne@gmail.com>
update_channel / remove_channel / reconnect_channel took a raw channel_id
and mutated any agent's channel. The config tool is auto-approved and now
injected for every agent, so reject foreign channels with the same
"not found" error as missing ones (no existence leak).

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
vaayne added 3 commits July 7, 2026 13:55
Review flagged residual 'wires skills' claims in the tool-registry doc. The
SkillsServer is defined but injected nowhere by buildMcpServers(); reconcile the
PR-7 line, the de-soul-gating risk, and the 'skills newly wired' risk item to say
wiring stays deferred/unmounted, matching the earlier note and classification.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
…istorical

Review flagged that the fs-usage audit still points at deleted/renamed paths.
PR #16726 removed seedWorkspace.ts and moved prompt.ts/heartbeat.ts to ai/agents/
and claw.ts to ai/mcp/servers/cherryAutonomyTools.ts. Add historical markers (incl.
the un-flagged heartbeat entry, same class) so future audit work doesn't chase dead
paths, and refresh the import-style summary row.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
The idle-reuse guard also rebuilds when no live connection exists yet but a
non-headless connect is still in flight (connecting set, connection undefined) and
a headless run arrives. Add the regression test for that branch alongside the two
live-connection headless cases.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
@vaayne

vaayne commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

All three suggestions fixed + full self-review pass

Thanks @eeee0717. Addressed all three, and rather than trickle these out I did a complete independent + adversarial sweep of the whole diff to close the loop — details at the end.

1. skills MCP doc drifta489d261e2
Reconciled every remaining "wires/newly-wired skills" claim in declarative-tool-registry.md (PR-7 line, de-soul-gating risk, and the "skills newly wired" risk item) to say wiring stays deferred — SkillsServer defined but unmounted, matching the earlier note + classification. Swept all 10 skills mentions in the doc; they're now consistent.

2. Stale cherryclaw/claw fs-audit paths3d022563e2
Marked the relocated entries historical in fs-usage-audit.md: seedWorkspace.ts removed, prompt.ts/heartbeat.tsai/agents/, claw.tsai/mcp/servers/cherryAutonomyTools.ts, plus the import-style summary row. Included the un-flagged heartbeat.ts entry (same class) so future audit work doesn't chase any dead path.

3. In-flight connecting headless rebuild test967b1185e7
Added the regression test for the branch where no live connection exists yet but a non-headless connect is still in flight and a headless run arrives → rebuild. Now all four idle-reuse states are covered (live+match reuse, live+mismatch rebuild, connecting+mismatch rebuild, headless+headless reuse).


Full-diff review (not just these lines). Ran my own pass plus an independent adversarial review of origin/main...HEAD across five angles — correctness after soul-gate removal, channel-ownership isolation, headless staleness, dead-code/orphans, and test gaps. Both came back clean:

  • Isolation: every channel_id-accepting handler in cherryAutonomyTools.ts validates channel.agentId === this.agentId (cron add, config update/remove/reconnect); notify is structurally scoped via getAgentAdapters(this.agentId).
  • Orphans: zero dangling cherryclaw/seedWorkspace/SOUL_MODE/autonomous-toggle references in live source.
  • settingsBuilder de-soul-gating: the adjustAllowedToolsForMcp change is additive (adds SDK pre-approval for the now-always-injected cherry-tools/agent-memory) — it doesn't restrict built-ins, which still route through canUseTool.

CI green (general-test, basic-checks, changeset, catalog, changes). Re-requesting @eeee0717.

@vaayne vaayne requested a review from eeee0717 July 7, 2026 06:03

@eeee0717 eeee0717 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Findings:

  • Blocking: src/main/data/services/AgentTaskService.ts:96 and :190 still pass raw task channelIds into replaceTaskSubscriptions() without checking that each channel belongs to the task's agentId. The MCP cron add path now validates ownership, but the DataApi/settings path can still create or update Agent A's scheduled task with Agent B's channel id, which sends scheduled output into the wrong Telegram/Slack/etc. channel. Please enforce channel.agentId === agentId at the service boundary, and/or filter the settings channel picker by the selected task agent before writing subscriptions.
  • Blocking: src/main/ai/runtime/claudeCode/settingsBuilder.ts:640 still treats any session with a persisted channel link as headless via channelService.findBySessionId(session.id), even when the current turn was opened from the interactive renderer path with headless: false. Opening a channel-linked session in the app UI would therefore rebuild settings with AskUserQuestion disabled despite an interactive responder being present. Now that headless is explicitly plumbed through scheduled/channel call sites, the build-time policy should key off the current run's options.headless === true rather than the persistent channel link fallback, or otherwise distinguish channel-triggered turns from UI-triggered turns.
  • Suggestion: v2-refactor-temp/docs/ai/declarative-tool-registry.md:75 and :146 still describe buildMcpServers() as injecting in-process MCP servers only when at least one tool is enabled. Current code and tests inject cherry-tools and agent-memory for every session and rely on disallowedTools / hooks for per-tool policy, with skills still unmounted. Please update those stale lines to match the current model.
  • Suggestion: v2-refactor-temp/docs/ai/data-cluster.md:87 still lists soul_enabled as a typed AgentConfigurationSchema key, but this PR removes that key from the schema and documents stored values as ignored. Please remove it from the current key list or mark it historical so follow-up data/API reviews do not treat it as live schema surface.

CI: GitHub checks are green for this head (changes, catalog-hand-edit-check, changeset-check, basic-checks, general-test, and render-test passed; notify skipped). Local pnpm lint/test/format was not run per repository review rules.

修复后请 request eeee0717 再进行后续 review。

vaayne added 4 commits July 7, 2026 14:45
Signed-off-by: Vaayne <liu.vaayne@gmail.com>
createTask/updateTask validated nothing about incoming channelIds, so the
DataApi/settings path could attach another agent's channel to a task and
deliver scheduled output into the wrong IM channel. Enforce
channel.agentId === agentId at the service boundary with the same
'not found' response as the MCP cron guard (no existence leak), and
filter the Tasks settings channel picker by the task's agent so users
are never offered a channel the service would reject.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
buildToolPermissions treated any session with a persisted channel link
as headless, so opening a channel-linked session interactively in the
app disabled AskUserQuestion despite a live responder. Scheduled and
channel dispatches both pass headless: true explicitly and the runtime
rebuilds warm connections on a headless mismatch, so the
findBySessionId fallback is redundant on headless paths and wrong on
interactive ones — drop it.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
cherry-tools/agent-memory are injected for every session with per-tool
policy via disallowedTools/hooks, and soul_enabled is no longer a typed
AgentConfigurationSchema key — mark it historical.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
@vaayne

vaayne commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

Review follow-up — all four findings addressed (8fd7456a91..8e34071121)

Thanks @eeee0717. Both blockings and both suggestions are fixed; also merged latest main (6db9d4ed31).

Blocking 1 — task channel ownership (8fd7456a91)

AgentTaskService.createTask/updateTask now validate every incoming channel id against the task's agentId before touching subscriptions, with the same notFound('Channel', id) for foreign and missing ids as the MCP cron guard — no existence leak. createTask validates before registerJobSchedule, so a bad id no longer needs the rollback path; updateTask accepts [] to clear subscriptions. On the UX side, the Tasks settings channel picker (create form and task detail) is now filtered to the task's agent, and switching the agent in the create form drops selections that no longer belong.

Tests: create/update reject foreign and missing ids (no schedule left behind), own-channel success, [] clears; renderer asserts foreign channels are not offered.

Blocking 2 — headless derivation (faf693c9a1)

buildToolPermissions now keys off options.headless === true only; the channelService.findBySessionId(session.id) fallback is dropped. Both headless producers (runAgentTask.ts, ChannelMessageHandler.ts) pass the flag explicitly, and AgentSessionRuntimeService already rebuilds a warm connection on a headless mismatch, so the fallback was redundant on headless paths and wrong for channel-linked sessions opened interactively. The other findBySessionId use (channel prompt context in buildSystemPrompt) is unrelated and untouched; no other headless-inference site exists under runtime/claudeCode/.

Tests updated: explicit headless: true disallows AskUserQuestion; a channel-linked interactive session keeps it; the assistant-role disable stays.

Suggestions (8e34071121)

  • declarative-tool-registry.md — MCP injection text (both §Layer 3 and the PR-7 entry) now says cherry-tools/agent-memory are injected for every session with per-tool policy via disallowedTools/hooks, skills still defined-but-unmounted.
  • data-cluster.mdsoul_enabled removed from the live typed-key list and marked historical (stored values ignored by the loose schema).

Verification on the merged tree: pnpm lint, pnpm test, pnpm build:check all pass locally.

@vaayne vaayne requested a review from eeee0717 July 7, 2026 06:59
CherryNotify and CherryConfig are exposure 'user' like CherryCron, so
the agent edit dialog renders them via agent.tools.builtin.<key> — but
only CherryCron had locale entries, leaving the two new cards in
untranslated fallback. The dynamic template key is invisible to
i18n:check.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
@vaayne

vaayne commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

Full-PR multi-lens review (3 parallel independent reviewers, basis 8e34071121)

Ran a complete adversarial review of the whole diff across three lenses — main-process correctness & security, removal-residue & regression audit, renderer & shared data layer. Every claimed finding was re-verified against the code before this report; one reviewer claim was rejected as a false positive (the fs-usage-audit "stale headers" are intentional historical snapshots, annotated as such by 3d022563e2).

Fixed — missing i18n for the promoted autonomy tools (36bc41a5be)

CherryNotify / CherryConfig became exposure: 'user' alongside CherryCron, so the agent edit dialog renders them via agent.tools.builtin.<key>.label/.description — but only CherryCron had locale entries. The two new tool cards silently fell back to English in every locale, and the dynamic template key means i18n:check can't catch it. Added entries to en-us/zh-cn/zh-tw (matching the established 频道/頻道 terminology) and synced translate/*.

Known residual edge — headless flag is not applied on the enqueue/steer path

The beginTurn headless-mismatch rebuild added in 0ae4127130 covers idle-session reuse, but when a channel message arrives while its bound session is busy (isSessionBusyAgentChatContextProvider.prepareDispatch enqueue branch → enqueueUserMessage), the dispatch's headless: true is never applied: the message is either steered into the live turn or queued onto the existing connection, both running with the connection's already-baked (non-headless) policy — AskUserQuestion stays available for that channel-triggered turn.

Bounded impact, verified: scheduled tasks always create a fresh session per fire (runAgentTask.ts), so only channel dispatches can hit this, and only when the user has the channel-bound session open and streaming at that exact moment — in which case an interactive responder usually is present and can answer. The bad sub-case is a queued channel turn starting after the user stops watching: an unanswered AskUserQuestion would stall collectStreamResponse's executionDone await (no timeout).

Not patching this inline: a correct fix needs per-turn policy reconciliation on a shared warm connection (steer vs queue vs rebuild-between-turns), which is real design work on a state machine that already went through several review rounds here. Proposing a follow-up issue instead — options are (a) plumb headless through enqueueUserMessage and force a connection rebuild before running a queued headless turn, or (b) add a timeout/auto-answer guard on the channel side. @eeee0717 if you'd rather see (a) in this PR, say the word.

Everything else — clean

  • Autonomy tool authorization (cron add/list/remove, notify targets, config channel ops) all correctly scoped to the owning agent; no action escapes it.
  • No soul_enabled / cherryClaw / mcp__claw__ residue anywhere in src/ beyond the intentionally kept migration shims; no dangling imports from the flattened engine dir; deleted helpers (seedWorkspaceTemplates, buildToolGuidance, removed always-true branches) have zero remaining callers.
  • i18n flattening verified key-by-key: every agent.channels.* / agent.tasks.* call site resolves; no orphaned or leftover keys in locales or translate/*.
  • Persona/bootstrap/heartbeat guards hold for the assistant role and headless runs; configuration saves preserve unknown keys (loose schema) so stored soul_enabled values survive untouched as intended; breaking-changes doc matches the code.

Verification: pnpm i18n:check and targeted checks pass locally; CI running on the new head.

@eeee0717 eeee0717 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This review was translated automatically.

Findings:

  • Blocking: src/main/ai/streamManager/context/AgentChatContextProvider.ts:98 drops the backend headless: true state when a channel-bound session is already busy. The busy branch only calls enqueueUserMessage(sessionId, userMessage), and enqueueUserMessage() can either steer into the current non-headless connection or queue the next turn without changing the connection's baked tool policy. A channel message that arrives during an interactive turn can therefore still expose AskUserQuestion for a later responder-less channel turn and stall. Please carry headless state through the enqueue/steer path and rebuild or tighten the runtime policy before the queued headless turn runs.
  • Blocking: src/main/data/services/AgentChannelWorkflowService.ts:67 allows a channel's agentId to be changed without clearing or revalidating existing agent_channel_task subscriptions. src/main/ai/agents/runAgentTask.ts:149 then reads subscribed channels only by schedule id and sends through their adapters, so a task owned by Agent A can keep delivering through a channel after that channel is reassigned to Agent B. Please clear subscriptions on channel ownership changes, or enforce task/channel owner matching at the subscription read/delivery boundary.
  • Suggestion: src/main/ai/mcp/servers/cherryAutonomyTools.ts:90 documents omitted channel_ids as "auto-bind all agent channels", but addJob() only binds explicit ids or the current source channel; non-channel invocations create a task with no channel subscriptions. Please align the schema text with the current behavior or implement the advertised all-channel default.
  • Suggestion: src/renderer/pages/settings/ChannelsSettings/ChannelForms.tsx:74 emits undefined when the user chooses inherited permission mode, and src/renderer/pages/settings/ChannelsSettings/ChannelDetail.tsx:489 drops undefined before PATCH. That means an existing channel permission override cannot be cleared back to inherit from the settings UI. The API already accepts nullable values, so send null for the inherit case.

After fixing, please request eeee0717 for further review.

Verification:

  • Reviewed PR #16726 at 36bc41a5be9767b5aa19821ffc8396bab39dd3bf in isolated worktree /Users/cherryai004/Documents/Cherry_Studio_Dev/cherry-studio-pr-review/.codex-tmp/pr-review-sweep-20260707T071612Z-16726-36bc41a5.
  • Ran six scoped reviewer passes and used only results that repeated PR #16726, this head SHA, and the assigned worktree.
  • Checked existing reviews/comments to avoid repeating previously raised issues.
  • Checked GitHub CI: general-test failed in src/main/core/job/__tests__/JobManager.integration.test.ts on a 20s timeout for arms a delayed job after commit and promotes it at scheduledAt; render-test, basic-checks, changes, changeset-check, and catalog-hand-edit-check passed; notify skipped.
  • Local pnpm lint/test/format not run per repository review rules.

Original Content

Findings:

  • Blocking: src/main/ai/streamManager/context/AgentChatContextProvider.ts:98 drops the backend headless: true state when a channel-bound session is already busy. The busy branch only calls enqueueUserMessage(sessionId, userMessage), and enqueueUserMessage() can either steer into the current non-headless connection or queue the next turn without changing the connection's baked tool policy. A channel message that arrives during an interactive turn can therefore still expose AskUserQuestion for a later responder-less channel turn and stall. Please carry headless state through the enqueue/steer path and rebuild or tighten the runtime policy before the queued headless turn runs.
  • Blocking: src/main/data/services/AgentChannelWorkflowService.ts:67 allows a channel's agentId to be changed without clearing or revalidating existing agent_channel_task subscriptions. src/main/ai/agents/runAgentTask.ts:149 then reads subscribed channels only by schedule id and sends through their adapters, so a task owned by Agent A can keep delivering through a channel after that channel is reassigned to Agent B. Please clear subscriptions on channel ownership changes, or enforce task/channel owner matching at the subscription read/delivery boundary.
  • Suggestion: src/main/ai/mcp/servers/cherryAutonomyTools.ts:90 documents omitted channel_ids as "auto-bind all agent channels", but addJob() only binds explicit ids or the current source channel; non-channel invocations create a task with no channel subscriptions. Please align the schema text with the current behavior or implement the advertised all-channel default.
  • Suggestion: src/renderer/pages/settings/ChannelsSettings/ChannelForms.tsx:74 emits undefined when the user chooses inherited permission mode, and src/renderer/pages/settings/ChannelsSettings/ChannelDetail.tsx:489 drops undefined before PATCH. That means an existing channel permission override cannot be cleared back to inherit from the settings UI. The API already accepts nullable values, so send null for the inherit case.

修复后请 request eeee0717 再进行后续 review。

Verification:

  • Reviewed PR #16726 at 36bc41a5be9767b5aa19821ffc8396bab39dd3bf in isolated worktree /Users/cherryai004/Documents/Cherry_Studio_Dev/cherry-studio-pr-review/.codex-tmp/pr-review-sweep-20260707T071612Z-16726-36bc41a5.
  • Ran six scoped reviewer passes and used only results that repeated PR #16726, this head SHA, and the assigned worktree.
  • Checked existing reviews/comments to avoid repeating previously raised issues.
  • Checked GitHub CI: general-test failed in src/main/core/job/__tests__/JobManager.integration.test.ts on a 20s timeout for arms a delayed job after commit and promotes it at scheduledAt; render-test, basic-checks, changes, changeset-check, and catalog-hand-edit-check passed; notify skipped.
  • Local pnpm lint/test/format not run per repository review rules.

vaayne added 12 commits July 7, 2026 15:44
…at fire time

A channel dispatch landing on a busy session goes through
enqueueUserMessage, which cannot rebuild the connection's baked tool
policy — so a queued channel turn ran with AskUserQuestion available
and no responder. Track headless per queued message (mirroring the
steer-mark pattern), stamp it on the turn that message opens, and deny
AskUserQuestion in canUseTool when the live current turn is headless.
Messages steered into a live interactive turn shed the mark: an
interactive responder exists there. Baked disallowedTools stays as the
belt for fully-headless connections.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
Rebinding a channel to another agent left its agent_channel_task rows
intact, and runAgentTask read subscriptions by schedule id alone — so
Agent A's scheduled output kept flowing into Agent B's channel. Clear
the channel's task subscriptions on an agentId change (deliberately not
restored if the follow-up sync fails: conservative beats wrong-agent
delivery) and filter delivery to channels owned by the task's agent as
the authoritative guard for legacy rows and races.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
The schema advertised 'omit to auto-bind all agent channels', but addJob
binds explicit ids or the current source channel only.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
The inherit option emitted undefined, which handleSave drops before
PATCH — an existing override could never be cleared. Send null, which
the API schema already accepts.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
…ions

A headless turn steered by another headless input previously rolled into
an interactive continuation turn, reopening AskUserQuestion mid-run.
Compute rollHeadless at the steer boundary (before the marks are
consumed) and inherit it in startContinuationTurn; any interactive
participant keeps the continuation interactive.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
…stored

Subscriptions created during the transient rebind window (between the
optimistic agentId update and the sync failure) belonged to the new
agent; restoring the old agentId must clear them again.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
Legacy agent_channel_task rows could link a task to a channel owned by
a different agent; importing them verbatim would violate the v2
invariant that task delivery stays within the owning agent. Skip such
rows and report them via skippedCrossAgentChannelLinks.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
The assistant builtin role has no persona workspace or autonomy loop;
running a heartbeat against it created pointless sessions. Return a
skip result before any session or workspace work.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
app.proxy.url may carry basic-auth userinfo; injecting it verbatim into
the system prompt leaks credentials to the model provider. Strip
username/password while keeping host and port.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
The memory append API reads a tags array, not a tag string, so the
bootstrap completion log was written untagged. The prompt also told the
model to edit memory/FACT.md directly while an earlier rule forbade it;
FACT.md is read inline and written only via the memory update action.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
The config fields are allowed_chat_ids (allowed_channel_ids on
Discord); allow_ids does not exist.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
…hema

add_channel validates with ChannelConfigSchema, which requires full
feishu/wechat credentials up front — the QR-code-on-omission flow the
descriptions promised can never be reached; point QR reconnection at
reconnect_channel or the settings UI instead. Also state that enabled
defaults to true only on add and is left unchanged when omitted on
update.

Signed-off-by: Vaayne <liu.vaayne@gmail.com>
@vaayne

vaayne commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

Review follow-up — all four findings addressed (36bc41a5be..802dbcf0b6)

Thanks @eeee0717. Both blockings and both suggestions are fixed at head 802dbcf0b6, plus additional hardening from an independent re-review pass over the same surface.

Blocking 1 — busy branch drops headless (64e050717b + 1184bda149)

Headless is now a per-turn property enforced at fire time, not only a baked connection setting:

  • The busy branch passes { headless: req.headless === true } into enqueueUserMessage(), which marks the message id in a per-session headlessMessageIds set (mirroring steerMessageIds). startNextTurn consumes the mark, so a queued channel message runs as a headless turn even on a warm interactive connection.
  • canUseTool in settingsBuilder denies AskUserQuestion whenever AgentSessionRuntimeService.isCurrentTurnHeadless(sessionId) is true, instructing the model to proceed and state assumptions. The baked disallowedTools entry stays as a second layer for turns headless from the start.
  • Steer continuations inherit correctly (1184bda149): at the steer boundary we compute whether the interrupted turn and every steering input were headless; only then does the roll continuation stay headless. Any interactive participant makes the continuation interactive — a responder exists.

Tests cover the enqueue path, the fire-time denial, and all three roll combinations (headless+headless, headless+interactive, interactive+headless).

Blocking 2 — channel agentId rebind keeps task subscriptions (9ef6050390 + a39455d652)

Fixed on both sides of the boundary:

  • Data hygiene: rebinding a channel's agentId clears its agent_channel_task subscriptions; if syncChannel fails and the old agentId is restored, subscriptions created during the transient window are cleared again (a39455d652).
  • Authoritative delivery guard: runAgentTask filters subscribed channels by channel.agentId === agentId before sending, so wrong-agent delivery is impossible even if a stale row survives.

Suggestions

  • channel_ids "auto-bind all agent channels" (6d5c1c1c6d) — the schema text now matches actual behavior: explicit ids, else the current source channel, else no subscriptions.
  • Permission mode can't be cleared back to inherit (5c3f95499b) — the inherit option now emits null, which survives the PATCH filter.

Additional hardening (independent re-review at the same head)

  • 3e8a56b0d1 — the v1 migrator no longer imports agent_channel_task rows whose channel owner differs from the task owner (same cross-agent invariant as Blocking 2, at the import boundary); skips are logged as skippedCrossAgentChannelLinks.
  • b873ed53ee — heartbeat runs skip the assistant builtin role instead of creating pointless sessions.
  • a6f1e95a5b — proxy URLs are credential-redacted before injection into the assistant system prompt (pre-existing leak of basic-auth userinfo to the provider).
  • 7df2c6d061 — the bootstrap completion log uses tags: ["bootstrap"] (the memory API ignores the singular tag, so the entry was written untagged), and the prompt no longer contradicts itself about FACT.md write paths (memory tool only).
  • b61a978f15/whoami help names the real config fields (allowed_chat_ids / allowed_channel_ids); allow_ids doesn't exist.
  • 802dbcf0b6 — feishu/wechat add_channel descriptions no longer promise a QR-on-omitted-credentials flow that the strict ChannelConfigSchema makes unreachable; QR reconnection points at reconnect_channel / settings UI, and the enabled default semantics are stated accurately.

Pre-existing issues found but out of scope for this PR (follow-ups to be filed): plan/small model clear writes undefined and never persists (agentForm.ts), per-field save clobbers in-progress drafts in ChannelForms, timeoutMinutes: null treated as keep-old in AgentTaskService, stale agent_task tables section in data-cluster.md.

Verification

pnpm lint, pnpm test, pnpm build:check all green locally at 802dbcf0b6.

@vaayne vaayne marked this pull request as draft July 7, 2026 10:06
@vaayne vaayne marked this pull request as ready for review July 7, 2026 10:06
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.

3 participants