Tracked as follow-up to #4319 (F1 acp-bridge package self-sufficiency) review thread by @wenshao. F1 declined the helper extraction itself because F1's mandate is mechanical lift + targeted fixes; a new shared util sits outside that scope.
Context
The boot path (packages/cli/src/config/config.ts's disabledTools array population around the tools.disabled settings read) and the MCP restart path (packages/cli/src/acp-integration/acpAgent.ts post-restartMcpServer settings refresh) both perform the same 4-step normalization on tools.disabled:
typeof string filter (drop non-string entries)
.trim() (handle hand-edited whitespace)
- Skip empties after trim
- Dedupe via
Set
Currently the two implementations are copy-pasted. Any future enhancement (case-folding, Unicode normalization, plugin-name aliasing, etc.) requires editing both — and the recent F1 fold-in commit b78de2719 already exposed this drift when the restart path needed the same normalization the boot path was already doing.
Proposal
Extract a single helper:
// packages/cli/src/config/normalizeDisabledTools.ts (or core/src/utils/...)
/**
* Normalize a raw `tools.disabled` settings array into a Set of
* tool names. Boot path + MCP restart path call this so both
* agree byte-for-byte on what counts as "disabled" — without that
* agreement, `ToolRegistry.has(tool.name)` exact-match check
* silently re-registers tools whose disabled-name had whitespace.
*
* Behavior:
* 1. Reject non-array input — return empty set.
* 2. Reject non-string entries — skip individually.
* 3. Trim each entry.
* 4. Skip empty-after-trim.
* 5. Dedupe (preserves first occurrence order if needed; Set
* drops it but callers don't depend on order today).
*/
export function normalizeDisabledToolList(raw: unknown): Set<string>;
Replace both inline implementations with a single call. Add unit tests for the helper covering:
- Non-array input (object, number, undefined, null) → empty Set
- Mixed-type array → only valid strings retained
- Whitespace entries (
' ', '\n', '\t') → dropped
- Duplicates (different orderings, whitespace variants like
'Foo' vs ' Foo ') → deduped
- BOM / zero-width chars in tool names → policy decision
Out of scope (probably)
- Case-folding tool names (changes external contract)
- Unicode normalization (
String.prototype.normalize) — could land as a separate decision
- Workspace-wide
tools.allowed parallel — different feature
Refs
🤖 Generated with Qwen Code
Tracked as follow-up to #4319 (F1 acp-bridge package self-sufficiency) review thread by @wenshao. F1 declined the helper extraction itself because F1's mandate is mechanical lift + targeted fixes; a new shared util sits outside that scope.
Context
The boot path (
packages/cli/src/config/config.ts'sdisabledToolsarray population around thetools.disabledsettings read) and the MCP restart path (packages/cli/src/acp-integration/acpAgent.tspost-restartMcpServersettings refresh) both perform the same 4-step normalization ontools.disabled:typeof stringfilter (drop non-string entries).trim()(handle hand-edited whitespace)SetCurrently the two implementations are copy-pasted. Any future enhancement (case-folding, Unicode normalization, plugin-name aliasing, etc.) requires editing both — and the recent F1 fold-in commit
b78de2719already exposed this drift when the restart path needed the same normalization the boot path was already doing.Proposal
Extract a single helper:
Replace both inline implementations with a single call. Add unit tests for the helper covering:
' ','\n','\t') → dropped'Foo'vs' Foo ') → dedupedOut of scope (probably)
String.prototype.normalize) — could land as a separate decisiontools.allowedparallel — different featureRefs
b78de2719(introduced the duplicate)🤖 Generated with Qwen Code