Skip to content

Follow-ups from PR #2109: harden shared-file-writer derivation against drift #2110

Description

@dyoshikawa

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

  1. SHARED_WRITE_FEATURES manual allowlist limits source-of-truth completenesssrc/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.

  2. settablePathsForScope catch early-return swallows getExtraSharedWritePathssrc/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

  1. getExtraSharedWritePaths is a duck-typed contract with no base-class/interface declarationcodexcli-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 []; }.

  2. Type looseness (as unknown as, unknown return) defeats derivation type safetysrc/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).

  3. Test cases 2 and 3 are logically subsumed by case 1src/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.

  4. (out of PR scope) Kilo writes mcp/rules to kilo.json but permissions to kilo.jsonckilo-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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    maintainer-scrapRough notes for AI implementation. Not for human eyes.refactoring

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions