From 4eaa12fef2c68bffeb70418790dd1bb44a2f3468 Mon Sep 17 00:00:00 2001 From: Max Hsu Date: Tue, 3 Nov 2026 04:49:15 +0800 Subject: [PATCH] fix(cli): let worktree set --comment "" clear the comment (#37515) `worktree set` read --comment through getOptionalStringFlag, which turns an empty value into undefined, so `--comment ""` and `--comment=` sent no comment field and the stored comment survived while the command reported ok:true. Keep an explicit empty value for this flag only; the shared helper and the RPC schema are unchanged. Fixes #37464 --- src/cli/flags.ts | 10 +++++ src/cli/handlers/worktree.ts | 4 +- src/cli/index-worktree-set.test.ts | 23 +++++++++++ src/cli/specs/worktree-set.ts | 2 + .../runtime-managed-worktree-metadata.test.ts | 40 +++++++++++++++++++ 5 files changed, 78 insertions(+), 1 deletion(-) diff --git a/src/cli/flags.ts b/src/cli/flags.ts index e5fa27304d4..45b0bbe0047 100644 --- a/src/cli/flags.ts +++ b/src/cli/flags.ts @@ -41,6 +41,16 @@ export function getOptionalStringFlag( return typeof value === 'string' && value.length > 0 ? value : undefined } +// Clearing flags (e.g. `worktree set --comment ""`) must keep the empty value instead of dropping it. +export function getOptionalStringFlagAllowingEmpty( + flags: Map, + name: string +): string | undefined { + const value = flags.get(name) + rejectValuelessFlag(value, name) + return typeof value === 'string' ? value : undefined +} + export function getPresentStringFlag( flags: Map, name: string diff --git a/src/cli/handlers/worktree.ts b/src/cli/handlers/worktree.ts index 8c0d1b3d69e..53b004184b4 100644 --- a/src/cli/handlers/worktree.ts +++ b/src/cli/handlers/worktree.ts @@ -18,6 +18,7 @@ import { RuntimeClientError } from '../runtime-client' import { getOptionalPositiveIntegerFlag, getOptionalStringFlag, + getOptionalStringFlagAllowingEmpty, getPresentStringFlag, getRequiredStringFlag } from '../flags' @@ -243,7 +244,8 @@ export const WORKTREE_HANDLERS: Record = { displayName: getOptionalStringFlag(flags, 'display-name'), ...reviewLinks, ...linearIssueLink, - comment: getOptionalStringFlag(flags, 'comment'), + // An explicit empty value clears the stored comment. + comment: getOptionalStringFlagAllowingEmpty(flags, 'comment'), workspaceStatus: getOptionalStringFlag(flags, 'workspace-status'), isUnread, parentWorktree: await withWorktreeSelectorRecovery( diff --git a/src/cli/index-worktree-set.test.ts b/src/cli/index-worktree-set.test.ts index 762a944a4b0..9500f6f61da 100644 --- a/src/cli/index-worktree-set.test.ts +++ b/src/cli/index-worktree-set.test.ts @@ -559,6 +559,29 @@ describe('orca cli worktree awareness', () => { ) }) + it.each([[['--comment', '']], [['--comment=']]])( + 'sends an explicit empty comment for %j so worktree.set clears it', + async (flags) => { + queueFixtures( + callMock, + okFixture('req_clear_comment', { + worktree: buildWorktree('/tmp/repo/child', 'feature/child') + }) + ) + vi.spyOn(console, 'log').mockImplementation(() => {}) + + await main( + ['worktree', 'set', '--worktree', 'id:repo::/tmp/repo/child', ...flags, '--json'], + '/tmp/repo' + ) + + expect(callMock).toHaveBeenCalledExactlyOnceWith( + 'worktree.set', + expect.objectContaining({ worktree: 'id:repo::/tmp/repo/child', comment: '' }) + ) + } + ) + it.each([ [['--unread', '--read'], 'Choose either --unread or --read'], [['--read', '--unread'], 'Choose either --unread or --read'], diff --git a/src/cli/specs/worktree-set.ts b/src/cli/specs/worktree-set.ts index 5ef857157b1..9627e17100c 100644 --- a/src/cli/specs/worktree-set.ts +++ b/src/cli/specs/worktree-set.ts @@ -24,6 +24,7 @@ export const WORKTREE_SET_COMMAND_SPEC: CommandSpec = { ], notes: [ 'Workspace status ids match the board columns (defaults: todo, in-progress, in-review, completed); custom statuses use their configured id.', + 'Pass --comment "" (or --comment=) to clear the comment.', 'Pass --linear-issue null to clear the Linear issue link.', 'Pass --pr null, --gitlab-issue null, or --gitlab-mr null to clear only the matching link.', 'Each link flag updates its own field and leaves unrelated links unchanged. GitLab URLs must match the stored source project or remote; they cannot select a foreign project.' @@ -31,6 +32,7 @@ export const WORKTREE_SET_COMMAND_SPEC: CommandSpec = { examples: [ 'orca worktree set --worktree active --linear-issue STA-335 --json', 'orca worktree set --worktree active --linear-issue null --json', + 'orca worktree set --worktree active --comment "" --json', 'orca worktree set --worktree active --workspace-status in-review --unread --json' ] } diff --git a/src/main/runtime/runtime-managed-worktree-metadata.test.ts b/src/main/runtime/runtime-managed-worktree-metadata.test.ts index c3ce8fd3f66..23c3660e195 100644 --- a/src/main/runtime/runtime-managed-worktree-metadata.test.ts +++ b/src/main/runtime/runtime-managed-worktree-metadata.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it, vi } from 'vitest' import type { WorktreeMeta } from '../../shared/worktree/meta-types' import { hostWorktreeTarget } from '../../shared/worktree/host-qualified-identity' import type { Worktree } from '../../shared/worktree/types' +import { mergeWorktreeMetaForWrite } from '../persistence/loading-store/worktree-meta-write-normalization' import { updateRuntimeManagedWorktreeMetadata } from './runtime-managed-worktree-metadata' import type { ResolvedWorktree } from './runtime-worktree-path-identity' import type { RuntimeStore } from './runtime-store-contract' @@ -43,6 +44,45 @@ describe('updateRuntimeManagedWorktreeMetadata', () => { ) }) + it('persists an empty comment so a set comment can be cleared (#37464)', async () => { + const worktree = { + id: 'repo-1::/workspace/app', + repoId: 'repo-1', + hostId: 'local', + path: '/workspace/app', + instanceId: 'instance-1' + } as unknown as ResolvedWorktree + let row: WorktreeMeta | undefined + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the update only reads these store members. + const store = { + getRepos: () => [], + setWorktreeMeta: (_target: unknown, meta: Partial) => { + row = mergeWorktreeMetaForWrite(row, meta) + } + } as unknown as RuntimeStore + const ports = { + resolveWorktree: vi.fn(async () => worktree), + validateParent: vi.fn(), + invalidateResolved: vi.fn(), + invalidateScan: vi.fn(), + notifyChanged: vi.fn(), + showWorktree: vi.fn(async () => worktree as unknown as Worktree) + } + const update = (comment: string) => + updateRuntimeManagedWorktreeMetadata({ + selector: `id:${worktree.id}`, + updates: { comment }, + store, + ports + }) + + await update('waiting on review') + expect(row?.comment).toBe('waiting on review') + + await update('') + expect(row?.comment).toBe('') + }) + describe('a folder workspace removed while its resolution was in flight (#22712)', () => { const folderRepo = { id: 'repo-1', path: '/workspace/app', displayName: 'app', kind: 'folder' } // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the update only reads id, repoId, hostId and instanceId.