Background
PR #2109 introduced src/lib/shared-file-derive.ts, which derives the multi-writer set from PROCESSOR_REGISTRY x getSettablePaths (across project and global scope, plus a getExtraSharedWritePaths contract) and asserts it matches GENERATION_STEP_GRAPH. This makes a missing shared-file-writer declaration fail CI instead of being caught only in review. Refs #2081.
The PR was reviewed and merged. No high/critical correctness or security findings were raised. The items below are the non-blocking (mid/low) follow-ups captured for later.
Details
mid
-
SHARED_WRITE_FEATURES manual allowlist limits source-of-truth completeness — src/lib/shared-file-derive.ts:22-28. Derivation targets are hand-fixed to ignore/mcp/hooks/permissions/rules (5 features). If commands/subagents/skills later start writing to shared config files and someone forgets to add them here, the drift this PR aims to prevent would silently re-appear. Consider an exhaustiveness check against the Feature enum, or an explicit exclusion list with a comment/test documenting why each feature is in or out.
-
settablePathsForScope catch early-return swallows getExtraSharedWritePaths — src/lib/shared-file-derive.ts (the try/catch that does return paths;). When getSettablePaths({ global }) throws for a scope, the subsequent getExtraSharedWritePaths is skipped. Currently harmless with existing tools, but a tool whose global getSettablePaths throws while it also has a global-only extra shared path would silently drop that path. Collect extra paths outside the try/catch, independent of getSettablePaths success.
low
-
getExtraSharedWritePaths is a duck-typed contract with no base-class/interface declaration — codexcli-hooks.ts, kilo-rule.ts, etc. Coupled only via the optional call cls.getExtraSharedWritePaths?.(...). New tool authors cannot discover the hook, and signature mismatches are not caught at compile time. Consider a base-class default static getExtraSharedWritePaths(): SharedWritePath[] { return []; }.
-
Type looseness (as unknown as, unknown return) defeats derivation type safety — src/lib/shared-file-derive.ts:41-46, 100. If the getSettablePaths return shape changes, derivation may silently return empty (missed shared files). Extract a type-guard for the expected settable shape (relativeFilePath/root/alternativeRoots).
-
Test cases 2 and 3 are logically subsumed by case 1 — src/lib/shared-file-derive.test.ts:31-47. If the full-equality case passes, "no undeclared" and "no missing declaration" necessarily hold. Acceptable if intentional for clearer failure messages; add a comment noting that.
-
(out of PR scope) Kilo writes mcp/rules to kilo.json but permissions to kilo.jsonc — kilo-permissions.ts:114-119 vs kilo-mcp.ts:271-282. The PR's fix (removing permissions from the kilo.json shared set) correctly matches reality. Worth confirming whether splitting one tool's config across two files (kilo.json/kilo.jsonc) is intentional.
Solution / Next Steps
- Address the two mid items to harden the derivation against future drift and silent path loss (exhaustiveness/exclusion for the feature allowlist; move extra-path collection out of the try/catch).
- Optionally tackle the low items to improve discoverability and type safety of the
getExtraSharedWritePaths contract.
- Separately confirm the intended Kilo
kilo.json/kilo.jsonc split.
Background
PR #2109 introduced
src/lib/shared-file-derive.ts, which derives the multi-writer set fromPROCESSOR_REGISTRYxgetSettablePaths(across project and global scope, plus agetExtraSharedWritePathscontract) and asserts it matchesGENERATION_STEP_GRAPH. This makes a missing shared-file-writer declaration fail CI instead of being caught only in review. Refs #2081.The PR was reviewed and merged. No high/critical correctness or security findings were raised. The items below are the non-blocking (mid/low) follow-ups captured for later.
Details
mid
SHARED_WRITE_FEATURESmanual allowlist limits source-of-truth completeness —src/lib/shared-file-derive.ts:22-28. Derivation targets are hand-fixed toignore/mcp/hooks/permissions/rules(5 features). Ifcommands/subagents/skillslater start writing to shared config files and someone forgets to add them here, the drift this PR aims to prevent would silently re-appear. Consider an exhaustiveness check against theFeatureenum, or an explicit exclusion list with a comment/test documenting why each feature is in or out.settablePathsForScopecatch early-return swallowsgetExtraSharedWritePaths—src/lib/shared-file-derive.ts(the try/catch that doesreturn paths;). WhengetSettablePaths({ global })throws for a scope, the subsequentgetExtraSharedWritePathsis skipped. Currently harmless with existing tools, but a tool whose globalgetSettablePathsthrows while it also has a global-only extra shared path would silently drop that path. Collect extra paths outside the try/catch, independent ofgetSettablePathssuccess.low
getExtraSharedWritePathsis a duck-typed contract with no base-class/interface declaration —codexcli-hooks.ts,kilo-rule.ts, etc. Coupled only via the optional callcls.getExtraSharedWritePaths?.(...). New tool authors cannot discover the hook, and signature mismatches are not caught at compile time. Consider a base-class defaultstatic getExtraSharedWritePaths(): SharedWritePath[] { return []; }.Type looseness (
as unknown as,unknownreturn) defeats derivation type safety —src/lib/shared-file-derive.ts:41-46, 100. If thegetSettablePathsreturn shape changes, derivation may silently return empty (missed shared files). Extract a type-guard for the expectedsettableshape (relativeFilePath/root/alternativeRoots).Test cases 2 and 3 are logically subsumed by case 1 —
src/lib/shared-file-derive.test.ts:31-47. If the full-equality case passes, "no undeclared" and "no missing declaration" necessarily hold. Acceptable if intentional for clearer failure messages; add a comment noting that.(out of PR scope) Kilo writes mcp/rules to
kilo.jsonbut permissions tokilo.jsonc—kilo-permissions.ts:114-119vskilo-mcp.ts:271-282. The PR's fix (removing permissions from thekilo.jsonshared set) correctly matches reality. Worth confirming whether splitting one tool's config across two files (kilo.json/kilo.jsonc) is intentional.Solution / Next Steps
getExtraSharedWritePathscontract.kilo.json/kilo.jsoncsplit.