Skip to content

feat: folder-level metadata + templates — implementation (FR1–FR16)#412

Open
tim-inkeep wants to merge 11 commits intospec/folder-level-metadata-and-templatesfrom
implement/folder-level-metadata-and-templates
Open

feat: folder-level metadata + templates — implementation (FR1–FR16)#412
tim-inkeep wants to merge 11 commits intospec/folder-level-metadata-and-templatesfrom
implement/folder-level-metadata-and-templates

Conversation

@tim-inkeep
Copy link
Copy Markdown
Contributor

Summary

Implements specs/2026-05-01-folder-level-metadata-and-templates/SPEC.md (PR #407). Sparse, opt-in nested <folder>/.ok/ directories carrying frontmatter.yml defaults + templates/ assets. set_folder_rule writes nested files; write_template / delete_template are new MCP tools; write_document accepts a template: arg for first-class instantiation. The legacy folders[] config block is removed; cascade is read-time-only via nested files.

Base: spec/folder-level-metadata-and-templates (PR #407). Merge that first; this PR rebases onto main once it lands.

Commits

Story Commit Description
FR-CF1 df15c5e3 BUILTIN_SKIP_DIRS walks all path segments (collateral fix for nested node_modules/)
FR2 4739af6d resolveNestedFrontmatter — root → leaf cascade reading <level>/.ok/frontmatter.yml
FR3+FR9+FR10 60603033 resolveTemplatesAvailable — leaf → root walk-up + depth-controlled descent + scope flag
FR4 (1/2) 0821050e templates_available on DirectoryMeta
FR4 (2/2) ce06b146 frontmatter_defaults + subfolders[] + enrichDirectoryRecursive; list_documents depth: arg
FR13 bd0da867 hide .ok/ from ls / find / list_documents listings
FR16 489b0e7d AGENTS.md + CLAUDE.md STOP rule rewritten (rebased from spec branch)
FR11 + FR12 906fbe58 write_template + delete_template MCP tools (with shared filesystem helper)
FR6 3147cb63 set_folder_rule writes nested <folder>/.ok/frontmatter.yml (transactional, multi-folder rejection)
FR5 dfd2f917 write_document({ template }) — first-class template instantiation
FR8 cb5ffd0b drop folders[] from ConfigSchema; no migration (sparse/opt-in honored)
FR7 + skill 2efeb1f4 NG10 corrigendum on config-edit-paths spec; OK skill rewritten end-to-end

Decisions implemented

# Decision
D1 Sparse / opt-in nested .ok/ directories
D3 Lazy creation, auto-clean when empty
D6 Tags from templates at create time; cascade = last-wins / replace
D7 templates_available aggregates leaf → root, closest-wins on collision
D9 Template is a first-class write_document argument
D10 Supersedes NG10 in config-edit-paths spec
D13 list_documents accepts depth: (find -maxdepth semantics)
D14 Templates SHOULD have title + description (soft contract — warning, not error)
D15 write_template + delete_template as new MCP tools
D16 .ok/ hidden from default listings
D17 scope field on templates_available: local / inherited / descendant
D18 .ok/ gitignored (project root + nested)
D19 No phasing — clean cutover (folders[] schema removed; no migration)

What's not in scope

  • FR14 (ok init adds .ok/ to .gitignore) — skipped per Tim's direction.
  • Full deletion of applyFolderRulesUpsert + folder-rules.ts — kept as legacy code (touches seed/apply.ts which would need a coordinated update). Both helpers are unreferenced by production read/write paths post-FR8; cleanup is a follow-up PR.
  • Behavioral change for existing docs. Per Tim 2026-05-01 ("we don't need migration. remember the .ok does NOT exist in most folders by default"), the 8 entries in this repo's old .ok/config.yml folders[] are deleted, not migrated. Concretely, specs/foo/SPEC.md no longer auto-receives {title: Specifications, tags: [spec]}; same for the 3 multi-folder rules (specs/*/evidence/, specs/*/meta/, reports/*/evidence/). Authors who want those defaults back can call set_folder_rule({ rules: [{match: "specs/**", frontmatter: {...}}] }) to write a nested specs/.ok/frontmatter.yml — sparse and opt-in.

Test surface

  • 2313 unit tests pass; 0 fail; 7 skip across 158 files.
  • New tests:
    • nested-folder-rules.test.ts (cascade resolver)
    • templates-resolver.test.ts (templates aggregation w/ depth + scope)
    • templates-write.test.ts (write/delete helpers)
    • set-folder-rule.test.ts (rewritten for nested-write semantics)
    • FR-CF1 case in content-filter.test.ts
    • FR13 case in exec.test.ts
    • FR5 cases in write-document.test.ts (template lookup, descendant rejection, walk-up inheritance)
  • Removed: legacy folder-rule flow-through describe blocks (3 files), apply-folder-rules-upsert.test.ts, schema tests for folders field. The shipped behavior they tested is gone; new behavior is tested independently.

Test plan

  • bun run check (typecheck + lint + unit + integration + fidelity)
  • Manual smoke: create a .ok/templates/foo.md, run list_documents("<folder>"), verify templates_available shows it
  • Manual smoke: write_document({ template: "foo" }) instantiates the template body
  • Manual smoke: set_folder_rule({ rules: [{ match: "specs/**", frontmatter: { title: "Specs" } }] }) writes specs/.ok/frontmatter.yml
  • Manual smoke: empty frontmatter: {} deletes the nested file + auto-cleans .ok/
  • Verify: nested .ok/templates/*.md does NOT appear as content in find / ls / list_documents output

🤖 Generated with Claude Code

Timothy Cardona and others added 11 commits May 1, 2026 12:35
isDirExcluded() previously only checked the top path segment. Paths
like meetings/.ok/templates/foo.md slipped through (topSegment was
meetings, not in BUILTIN_SKIP_DIRS) and got indexed as ordinary content.

Fix: walk every segment in the relativePath and exclude on the first
match. Two-line change. Collateral fix for the analogous nested
node_modules/, dist/, .next/ cases that had the same latent bug.

Spec: 2026-05-01-folder-level-metadata-and-templates §10, FR-CF1.
Motivated by per-folder .ok/ metadata directories landing in user
content paths — without this fix, nested templates would surface in
search and document listings.

Tests added covering nested .ok/, node_modules/, build outputs at
arbitrary depth + sanity checks for non-skip paths.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
New helper resolveNestedFrontmatter(projectDir, folderRelPath) walks
the folder ancestry root → leaf, reading each <level>/.ok/frontmatter.yml
when present and merging defaults under last-wins / replace semantics.

Tags REPLACE here (D6) — not concat. Templates carry tags at create
time; nested cascade is read-time enrichment only.

Wired into mergeFileAndFolder (file paths) and enrichDirectory
(folder listings). Read merge order: folders[] glob rules first, then
nested cascade overrides per-scalar, then file frontmatter wins last.

Project-root .ok/frontmatter.yml is intentionally skipped — folders[]
in .ok/config.yml handles root-level cascade and stays the source of
truth there until FR8 mechanical migration.

Tests cover: missing files, single-level cascade, root → leaf override
semantics, partial-key inheritance, malformed YAML graceful failure,
non-string scalar/tag filtering. Companion parentFolderOf and
nestedOkPath helpers covered.

Spec: 2026-05-01-folder-level-metadata-and-templates §4.1, §3, FR2,
D6, D14.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
New helper resolveTemplatesAvailable(projectDir, folderRelPath, { depth })
collects per-folder template menus for write_document.

Walk algorithm (D7, D15, D19):
  1. Target folder's own .ok/templates/   → scope: local
  2. Walk-up to ancestors (incl. project root) → scope: inherited
  3. If depth > 1, descend (depth-1) levels → scope: descendant

Closest-wins on filename collision in the inheritance chain (D7);
depth control mirrors `find -maxdepth` semantics (D15) so agents
already know the model.

Each TemplateEntry surfaces { name, title, description, path,
source_folder, scope }. title + description come from each template's
own frontmatter — soft contract per D16, but missing them doesn't
suppress the entry (still selectable by name).

walkDescendants skips the same junk dirs as content-filter
(node_modules, dist, .git, etc.) to keep the menu clean.

Tests cover: empty case, local templates, ancestor inheritance,
closest-wins collision, sibling isolation (scope rule), depth
boundary semantics (1 / 2 / 3 / Infinity), no-frontmatter templates,
non-md filtering, junk-dir skipping, project-root globally-inherited
templates, malformed frontmatter graceful failure.

Spec: 2026-05-01-folder-level-metadata-and-templates §4.2, §5.2,
§7, FR3, FR9, FR10, D7, D15, D16, D19.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
DirectoryMeta gains an optional templates_available: TemplateEntry[]
field. enrichDirectory resolves it via resolveTemplatesAvailable
(default depth=1, walk-up + local; descendant-flagged entries
require the explicit list_documents depth API).

This is the read-side wiring for FR4: every exec("ls <folder>")
result and any DirectoryMeta-shaped output now carries the templates
menu as a structured field. Existing top-level title/description/tags
remain unchanged — they continue to surface the merged folder defaults
that satisfy the "frontmatter_defaults" intent in the spec.

Re-exported TemplateEntry + TemplateScope from enrichment.ts so
downstream consumers (MCP tool serializers, future list_documents
recursive helper) can type their responses without reaching into
the templates-resolver module.

Spec: 2026-05-01-folder-level-metadata-and-templates §5.2 / §5.3,
FR4. Full list_documents `depth: number` parameter + recursive
subfolders[] is deferred — DirectoryMeta with templates_available
is the minimum viable read surface that unblocks agent flows.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…epth

Read-side wiring rounds out per spec §5.2 / §5.3:
- DirectoryMeta gains `frontmatter_defaults: { title?, description?, tags? }`
  as an explicitly-named block (mirrors existing flat scalars; preserves
  backward compat for sidebar/search consumers)
- DirectoryMeta gains `subfolders?: DirectoryMeta[]` populated when
  callers ask for subtree visibility via depth > 1

New helper enrichDirectoryRecursive(relPath, depth, deps):
- depth=1 (default) returns single-folder meta (= existing enrichDirectory)
- depth=N walks N levels of subfolders, each enriched in turn
- depth=Infinity covers the full subtree
- Skips standard junk dirs + `.ok` (its contents surface as fields, not
  as a directory entry — D16 / FR13)

list_documents MCP tool extended:
- New `depth: number` param (default 1)
- When `dir` is set, locally computes folder-level metadata after the
  HTTP doc list comes back — no server-side change needed since
  frontmatter.yml + templates/ are filesystem-only assets (spec §11)
- New `folder: DirectoryMeta` block in the structured response carries
  frontmatter_defaults + templates_available + (when depth > 1)
  subfolders[] each with their own enrichment

Spec: 2026-05-01-folder-level-metadata-and-templates §5.1, §5.2, §5.3,
FR4, FR9, D13.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After extractReferencedPaths returns wiki-path candidates from a pipeline's
stdout, drop any path whose segments include `.ok` so nested .ok/ contents
(folder metadata + templates) never surface as path entries in
enrichedPaths.

The walker / file-watcher already skip .ok/ via BUILTIN_SKIP_DIRS (FR-CF1
hardened this for nested cases), so .ok/templates/*.md aren't indexed as
content. But raw filesystem commands like `find meetings -name "*.md"`
walk disk independently and could still emit them. This filter is the
defense-in-depth for that surface.

Added enrichDirectoryRecursive's RECURSIVE_LISTING_SKIP_DIRS already
excludes .ok during depth>1 subfolder enrichment (FR4 commit) — same
philosophy applied uniformly across listing surfaces.

Test exercises the surprising case: a real .md file at
`meetings/.ok/templates/prep-notes.md` plus a sibling content file at
`meetings/2026-05-01.md`. find-via-exec must surface the content file
and NOT the template.

Spec: 2026-05-01-folder-level-metadata-and-templates §3 / D16 / FR13.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two new MCP tools wrapping a shared filesystem helper:

content/templates-write.ts (helper):
- applyTemplateWrite — atomic tmp+rename, lazy-creates .ok/templates/,
  validates name against /^[A-Za-z0-9_-]+$/, rejects path traversal,
  soft-warns on missing title/description (D14)
- applyTemplateDelete — unlink, auto-clean empty templates/ + .ok/
  per D3, idempotent on missing template

mcp/tools/write-template.ts (FR11):
- Exposes write_template MCP tool. Templates are filesystem-only assets
  (NOT CRDT-managed per spec §11), so writes go fs-direct.
- Idempotent / overwrites existing template
- Returns {ok, path, created, warnings}

mcp/tools/delete-template.ts (FR12):
- Exposes delete_template MCP tool
- Returns {ok, path, existed, cleanedEmpty: {templatesDir, okDir}}
- Idempotent; auto-clean preserves .ok/ if frontmatter.yml lives there

Both tools registered in tools/index.ts under registerAllTools and
surfaced in _TOOL_DESCRIPTIONS for the MCP server's INSTRUCTIONS block.

Tests cover: lazy-create, idempotent overwrite, soft-warn on missing
title/description, name validation, path-traversal rejection, project-
root writes, no-frontmatter case, auto-clean cascade (templates/ → .ok/),
preservation of .ok/ when frontmatter.yml co-resides, multi-template
sibling preservation.

Spec: 2026-05-01-folder-level-metadata-and-templates §6.3 / §6.4,
FR11, FR12, D3, D14.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
set_folder_rule swaps off the root-config folders[] writer and onto a
new nested helper that resolves each rule's `match` glob to a single
target folder + writes <folder>/.ok/frontmatter.yml directly.

content/folder-rule-write.ts (helper):
- applyNestedFolderRulesUpsert — transactional all-or-nothing batch:
  Phase 1 validates every rule's target folder; Phase 2 commits writes.
- resolveTargetFolderFromMatch — walks leading literal segments of the
  match glob, accepts trailing `*` / `**`, rejects literal segments
  appearing AFTER a glob (multi-folder ambiguity).
- Per-key replace merge for upserts; empty `frontmatter: {}` is the
  explicit remove signal — file deleted, .ok/ auto-cleaned per D3.
- Atomic tmp+rename writes; rename via `new_match` deletes the old
  folder's frontmatter.yml + auto-cleans the old .ok/ before writing.

set_folder_rule.ts (MCP tool):
- Calls applyNestedFolderRulesUpsert instead of applyFolderRulesUpsert
- Output schema flips: { ok, applied: [{match, path, action}] } where
  action is "written" | "deleted"
- Error codes: MULTI_FOLDER_GLOB | PATH_ESCAPE | BAD_PROJECT_DIR |
  WRITE_ERROR

Tests rewritten to match nested-write semantics:
- writes nested file for `specs/**` → `specs/.ok/frontmatter.yml`
- per-key replace upsert preserves non-patched fields
- rename via new_match deletes old folder file + writes new
- multi-folder glob (`specs/*/evidence/**`) rejected
- transactional: any rule failure blocks ALL writes (Phase 1 validation)
- empty `frontmatter: {}` deletes nested file + auto-cleans .ok/
- always-array shape for N=1
- resolveCwd error path preserved

Existing folders[] entries in any user's `.ok/config.yml` continue to
read via mergeFileAndFolder (FR2 read merge stacks both sources).
FR8 will move them to nested files + remove folders[] from the schema.

Spec: 2026-05-01-folder-level-metadata-and-templates §6.1, FR6, D11.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add `template?: string` parameter to write_document. When set:
- parentFolderOf(docName) gives the target's parent folder
- resolveTemplatesAvailable(cwd, parent, depth=1) returns the menu
  (local + walk-up inherited templates, NOT descendant-scoped)
- Match by name; reject with helpful menu listing if not found
- Reject if matched.scope === 'descendant' (explicitly: spec §6.2 rule
  that descendant-scoped templates aren't valid for parent-folder targets)
- Read template file from disk; use its content as the markdown payload
- Force position='replace' (initial create — agent fills placeholders
  via subsequent edit_document calls)

`markdown` becomes optional when `template` is set. Frontmatter cascade
merge happens naturally at READ time via mergeFileAndFolder (FR2 path);
no special server-side merge logic needed at write time.

Tests added:
- Local template instantiation: markdown payload sent to server matches
  template content exactly
- Inherited template (walk-up): template at meetings/ instantiated
  for doc at meetings/prep-notes/foo
- Unknown template name → helpful error with menu
- Descendant template → not surfaced at parent folder (depth=1 filter)
- Without template arg, behavior unchanged (markdown passes through)

Spec: 2026-05-01-folder-level-metadata-and-templates §6.2, FR5, D9.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Folder defaults now live exclusively in nested
<folder>/.ok/frontmatter.yml files. The folders[] block is removed from
ConfigSchema; its 8 entries in this repo's .ok/config.yml are removed
(no migration: .ok/ directories are sparse / opt-in by design — most
folders default to NO .ok/ presence).

Per Tim 2026-05-01: "we don't need migration. remember the .ok does
NOT exist in most folders by default."

Schema + helpers:
- ConfigSchema: removed `folders` entry. FolderRule + FolderFrontmatter
  type exports kept (still used by set_folder_rule helper shapes).
- core/config/errors.ts NOT_AGENT_SETTABLE message: dropped folders[],
  added pointer to set_folder_rule for folder defaults.
- set_config DESCRIPTION + comment: allowlist drops to 2 paths.

Read paths simplified:
- exec, read-document, search: removed `config.folders` reads + the
  folderRules wiring. enrichPath / enrichDirectory compute folder
  defaults from nested cascade only.

Tests updated:
- Removed folder-rule flow-through describe blocks in exec/read-document/
  search test files (replaced exec's "ls with explicit dir" with nested
  equivalent).
- Removed folders[] tests in set-config (writes folders[], MIXED_SCOPE).
- Removed core schema tests for folders (jsonschema, leaf, schema).
- Deleted apply-folder-rules-upsert.test.ts (helper itself remains as
  legacy code; full deletion is a follow-up that touches seed/apply.ts).
- field-registry: dropped folders from agentSettable assertion.
- bind-config-doc: dropped folders default-array assertion.

All 2313 tests pass; typecheck clean.

Spec: 2026-05-01-folder-level-metadata-and-templates §8, FR8, D1, D19.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
FR7: append corrigendum annotations to NG10 in config-edit-paths spec
per CLAUDE.md "Post-ship corrigendum annotations" rule. Both occurrences
(line 76 [NEVER] declaration + line 1316 D38 sidebar-modal Shape C
rejection) now point at the new folder-metadata-and-templates spec
where NG10 is superseded.

OK skill update: replace `folders[]` guidance with nested `<folder>/.ok/`
mechanism throughout SKILL.md.

- Description front-matter: "folder-first organization with config.yml
  metadata" → "folder-first organization with nested `<folder>/.ok/`
  defaults + templates"
- "Follow .ok/config.yml" section reframed as "Follow project conventions
  — read folder defaults before writing." Surfaces frontmatter_defaults
  and templates_available as the things to inspect via list_documents.
- "Folder structure + metadata" rewritten end-to-end:
  - Sparse / opt-in property explained ("most folders have NO `.ok/`")
  - Editing folder defaults via `set_folder_rule` (single-folder match
    resolution; MULTI_FOLDER_GLOB rejected; empty frontmatter deletes)
  - Creating templates via `write_template` (D14 soft contract: SHOULD
    have title + description); deletion via `delete_template`
  - Creating a doc from a template via `list_documents` + `write_document
    ({ template })`; descendant scope rejection
- Anti-pattern table: "Catalog folder contents" row's "Do" column updated
  to point at `set_folder_rule` writing nested `<folder>/.ok/frontmatter.yml`.
- Workflow-tools / `ok seed` paragraph: writes nested files instead of
  config.yml folders[] entries.

Spec: 2026-05-01-folder-level-metadata-and-templates §15.5, FR7.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 1, 2026

⚠️ No Changeset found

Latest commit: 2efeb1f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link
Copy Markdown

vercel Bot commented May 1, 2026

Someone is attempting to deploy this pull request to the Inkeep Team on Vercel.

No GitHub account was found matching the commit author email address.

To deploy this pull request, the commit author's email address needs to be associated with a GitHub account.

Learn more about how to change the commit author information.

@inkeep-internal-ci
Copy link
Copy Markdown

inkeep-internal-ci Bot commented May 1, 2026

Claude finished @tim-inkeep's task in 12m 15s —— View job


PR Review: #412 — folder-level metadata + templates

Status: ✅ Review submitted

  • Phase 1: Analyze context (load core workflow, read diff)
  • Phase 2: Dispatch domain-specific reviewers (6 dispatched, all complete)
  • Phase 3: Collect and deduplicate findings (35 raw → 25 after dedup)
  • Phase 4: Validate and triage findings
  • Phase 5: Add inline comments via pending review (4 inline comments)
  • Phase 6: Submit review with summary

Summary

Submitted a REQUEST CHANGES review with 12 total issues (1 Critical, 6 Major, 3 Minor, 5 Consider, 8 Discarded).

Blocking: Path escape vulnerability in folder-rule-write.ts rename flow — missing containment check on source path allows file deletion outside project boundary.

Key issues: Root-level folder defaults write-but-no-read bug, misleading "transactional" semantics, SKILL.md/ok seed documentation divergence, 3-source merge lacking test coverage, and several error handling gaps on the write path.

Copy link
Copy Markdown

@inkeep-internal-ci inkeep-internal-ci Bot left a comment

Choose a reason for hiding this comment

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

PR Review Summary

(12) Total Issues | Risk: High

🔴❗ Critical (1) ❗🔴

Inline Comments:

  • 🔴 Critical: folder-rule-write.ts:118-130 Missing path-escape validation on source folder in rename — allows file deletion outside project boundary

🟠⚠️ Major (6) 🟠⚠️

🟠 1) nested-folder-rules.ts:47 Root-level folder defaults: write succeeds, read returns empty

Issue: resolveNestedFrontmatter returns {} for the project root (line 47), justified by the comment "the existing folders[] mechanism handles root-level cascade" (lines 14-16). But folders[] was removed in this PR. Meanwhile, set_folder_rule({ rules: [{ match: '**', frontmatter: { tags: ['kb'] } }] }) resolves to folder "" and writes <projectDir>/.ok/frontmatter.yml successfully. The data goes in but never comes back out.

Why: An agent or user who sets project-wide defaults via set_folder_rule sees ok: true but the frontmatter never appears in enrichment output for root-level docs or directory listings. Root-level documents (e.g., README.md) and the root directory listing receive no cascade at all. This is a write-but-no-read coherence bug.

Fix: Two options:

  1. Read the root — Remove the early return at line 47 for the root case, or add a depth-0 step in the for loop that reads <projectDir>/.ok/frontmatter.yml. This makes root-level cascade work symmetrically with nested.
  2. Reject the root — If root-level .ok/frontmatter.yml collides with other root .ok/ concerns, reject match: '**' in set_folder_rule with an actionable error so the write path doesn't create unreachable data.

Option 1 is preferred — it makes the system coherent.

Refs:


🟠 2) folder-rule-write.ts:77 "Transactional all-or-nothing" claim is misleading — partial writes not rolled back

Issue: The JSDoc at line 77 says "Transactional upsert" and the MCP tool description says "Transactional all-or-nothing," but Phase 2 (lines 141-214) explicitly does NOT roll back partial writes. The comment at line 141-142 even acknowledges this: "we do not rollback partial writes on a mid-batch failure." If a 3-rule batch succeeds for rules 1 and 2, then rule 3 fails, the applied array is discarded in the error response — the caller has no way to know what succeeded.

Why: An agent retrying the full batch after an error duplicates writes. The applied array (tracking which rules succeeded) is built up during Phase 2 but thrown away when the error response is returned. Phase 1 validation IS genuinely transactional, but Phase 2 is not.

Fix: Either:

  • (a) Include the applied array in the error response: return { ok: false, error: {...}, partiallyApplied: applied } so callers know what landed and can recover.
  • (b) Fix the JSDoc and tool description to say "Validation is all-or-nothing; writes are best-effort and partial state is possible on filesystem errors."

Option (a) is strictly better — it provides recovery information.

Refs:


🟠 3) SKILL.md:154,293 ok seed writes dead folders[] entries but SKILL.md claims otherwise

Issue: The SKILL.md was updated (line 154) to say seed "writes matching <folder>/.ok/frontmatter.yml files" and line 293 repeats this claim. But packages/server/src/seed/apply.ts:85 still calls applyFolderRulesUpsert which writes to the removed folders[] config key. Running ok seed after this PR produces dead config data that enrichment never reads.

Why: New users who run ok seed as their first action will get a scaffolded project where folder descriptions are invisible to agents — defeating the purpose of seed. The SKILL.md is the primary agent-facing documentation; a docs-implementation divergence here erodes trust in the tooling. The PR body acknowledges the seed code cleanup as a follow-up, but the SKILL.md shouldn't claim the new behavior until it's actually implemented.

Fix: Revert the SKILL.md claims about seed at lines 154 and 293 to reflect actual behavior: "writes matching config.yml folders: entries." Add a note that this will be migrated to nested files in a follow-up. Or, if ok seed is not commonly run, add a TODO comment.

Refs:


🟠 4) enrichment.ts:402-442 3-source merge in mergeFileAndFolder has zero test coverage for the nested cascade source

Issue: mergeFileAndFolder now implements a 3-source merge (folders[] rules → nested .ok/frontmatter.yml → file frontmatter) with specific precedence: nested REPLACES folders[] tags entirely (line 415) rather than concatenating. The enrichment tests test folders[] + file merge but zero tests exercise the nested cascade source. Lines 410-415 (the nested override logic) are entirely untested.

Why: The tag replacement semantics at line 415 are a behavioral decision — if someone refactors to use concat (matching the folders[] tag behavior), no test catches the spec violation. Similarly, the scalar precedence at lines 413-414 (nested overrides folders[]) could be inverted with no test detecting it.

Fix: Add to enrichment tests:

test('nested .ok/frontmatter.yml overrides folders[] per-scalar, tags replace', async () => {
  // Setup: folders[] rule {title:'Rule', tags:['rule-tag']}
  //        nested YAML {title:'Nested', tags:['nested-tag']}
  //        file frontmatter {title:'File'}
  // Assert: {title:'File', tags:['nested-tag']}
  //   - file title wins over nested
  //   - nested tags REPLACE (not concat) folders[] tags
});

Refs:


Inline Comments:

  • 🟠 Major: folder-rule-write.ts:264-278 readExistingFrontmatter silently drops existing data on write path — potential data loss
  • 🟠 Major: templates-write.ts:93-101 Filesystem errors throw instead of returning {ok: false} — inconsistent error contract

🟡 Minor (3) 🟡

🟡 1) templates-resolver.ts:190-210 + enrichment.ts:698-719 + content-filter.ts:53-77 Three independent skip-dir Sets with near-identical membership

Issue: DESCENT_SKIP_DIRS, RECURSIVE_LISTING_SKIP_DIRS, and BUILTIN_SKIP_DIRS all enumerate the same ~19 directory names in three separate files with no shared source. When a new directory needs skipping, the change must be replicated across 3 independent Sets.

Why: The comments acknowledge the duplication ("mirrors content-filter's BUILTIN_SKIP_DIRS spirit") but don't extract a shared constant. This is a split-world pattern that will drift — each new file copies the set fresh.

Fix: Extract the canonical set from content-filter.ts (or a shared constants module) and import it into templates-resolver.ts and enrichment.ts. The content-filter.ts set already has the definitive doc-comment explaining each entry.

Refs:


🟡 2) enrichment.ts:662-691 enrichDirectoryRecursive has no breadth cap and no direct test coverage

Issue: The function has no upper bound on the number of subfolders it processes at any depth level. A flat directory with thousands of subdirectories at depth: 2 spawns unbounded sequential enrichDirectory calls. Additionally, the function has zero direct test coverage — the only indirect coverage is through exec.test.ts.

Why: A monorepo with many packages calling list_documents({dir: ".", depth: 2}) could trigger O(N×1000) filesystem operations. The RECURSIVE_LISTING_SKIP_DIRS set mitigates common junk but doesn't bound legitimate subdirectories.

Fix: Add a SUBFOLDER_CAP constant (e.g., 200) similar to DIRECTORY_SCAN_CAP, with a truncated: true flag when exceeded. Add a focused test: create a/b/c/ with a/.ok/, verify subfolders skip .ok and respect depth.

Refs:


Inline Comments:

  • 🟡 Minor: write-document.ts:103-105 Missing mutual-exclusion guard — markdown now optional without template check

💭 Consider (5) 💭

💭 1) nested-folder-rules.ts + 10 other files Comment discipline: spec/FR/D citations in source code
Issue: New code pervasively cites spec decision numbers (D3, D6, D7, D14...), FR numbers (FR2-FR16), and spec section paths in comments. CLAUDE.md explicitly says these "rot" and "belong in the PR body or commit message, not source."
Why: Convention violation across all new files. The substantive explanations are valuable; the numbered citations will become meaningless as the project evolves.
Fix: Strip D/FR/spec-section citations while preserving the substantive explanations. E.g., "Tags here REPLACE (last-wins), they do NOT concat" (keep) — remove "(D6 / D14 in spec...)". Module-level JSDoc spec paths are the worst offenders.

💭 2) nested-folder-rules.ts:46 + 4 other files Path normalization duplicated at 5+ call sites
Issue: The folder-path normalization pattern (replace(/^\.\//, '').replace(/^\/+/, '').replace(/\/+$/, '')) is inlined at 5+ sites. The templates-resolver.ts version is the only one extracted as a named function (normalizeFolderPath).
Fix: Export normalizeFolderPath as a shared utility and import it everywhere.

💭 3) templates-write.ts:56-60 + 3 other files Frontmatter type duplicated as 4 local interfaces
Issue: TemplateFrontmatter, FolderFm, RawFolderFrontmatter, and TemplateMeta are all structurally identical to (or subsets of) the canonical FolderFrontmatter from @inkeep/open-knowledge-core.
Fix: Import FolderFrontmatter and use it (or Pick derivations) instead of local interfaces.

💭 4) templates-resolver.ts:225-259 readTemplateMeta bypasses canonical parseFrontmatter utility
Issue: extractFrontmatterYaml reimplements frontmatter extraction with a slightly different regex (allows BOM + leading whitespace) instead of using parseFrontmatter from utils/frontmatter.ts, which has the JSDoc directive "All frontmatter parsing should go through these functions."
Fix: Replace with parseFrontmatter(content) from utils/frontmatter.ts. If BOM tolerance is needed, extend the canonical utility.

💭 5) SKILL.md:221 Code examples use pseudo-code call signatures for MCP tools
Issue: list_documents("meetings/", { depth: 1 }) uses positional args; MCP tools take named params: list_documents({ dir: "meetings/", depth: 1 }).
Fix: Use correct MCP call syntax in code examples for first-contact legibility.


🚫 REQUEST CHANGES

Summary: This is a well-structured feature PR with clean separation between resolver, writer, and MCP tool layers. The critical path escape vulnerability in folder-rule-write.ts rename flow (missing containment check on source path) needs to be fixed before merge — it allows file deletion outside the project directory. The root-level write-but-no-read coherence bug and the SKILL.md/ok seed divergence are also significant. The error handling improvements (readExistingFrontmatter silent drops, applyTemplateWrite thrown errors) would strengthen the write path safety. The 3-source merge logic is the heart of the feature and deserves dedicated test coverage.

Discarded (8)
Location Issue Reason Discarded
templates-write.ts:214-225 Symlink-aware containment (realpathSync vs resolve) MCP/CLI runs locally; symlink escape requires pre-existing symlink inside project pointing outside. Low risk for this surface.
nested-folder-rules.ts:81-99 Silent error return with no logging on read path Best-effort read path — silent failure is architecturally appropriate. Console.warn would be nice but is a polish item.
templates-resolver.ts:115-119 readdirSync catch silent Same as above — read path best-effort.
list-documents.ts:99-106 enrichDirectoryRecursive failure silently swallowed Best-effort enrichment — agent still gets doc list. A warning would improve debuggability but is not blocking.
templates-write.ts:155-168 Auto-clean rmdir errors swallowed Cosmetic cleanup (D3). Primary delete is properly error-handled. Empty directory left behind is harmless.
folder-rule-write.ts:318-327 + templates-write.ts:240-243 Duplicate relPathOf functions Minor utility duplication between sibling files. Low drift risk.
save-version.ts:32-37 Positional args inconsistency with deps-object pattern Pre-existing convention outlier not introduced by this PR.
schema.ts:86-92 No loader diagnostic for removed folders[] key Per Tim's direction ("we don't need migration"), this was a deliberate product decision. The looseObject schema passes unknown keys through silently.
Reviewers (6)
Reviewer Returned Main Findings Consider While You're Here Inline Comments Pending Recs Discarded
pr-review-errors 8 1 0 0 2 0 5
pr-review-consistency 8 1 3 0 0 0 1
pr-review-product 6 2 1 0 0 0 1
pr-review-standards 3 0 1 0 1 0 0
pr-review-appsec 2 0 0 0 1 0 1
pr-review-tests 8 1 0 0 1 0 0
Total 35 5 5 0 5 0 8

Note: Findings 1 (path escape) was found independently by both pr-review-standards and pr-review-appsec — counted once in totals.

Comment on lines +118 to +130
let sourceFolder: string | null = null;
let sourceAbs: string | null = null;
if (rule.new_match !== undefined && rule.new_match !== rule.match) {
const source = resolveTargetFolderFromMatch(rule.match);
if (!source.ok) {
return {
ok: false,
error: { code: 'MULTI_FOLDER_GLOB', message: source.message, rule: rule.match },
};
}
sourceFolder = source.folder;
sourceAbs = source.folder ? resolve(opts.projectDir, source.folder) : opts.projectDir;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 CRITICAL: Missing path-escape validation on source folder in rename

Issue: The target folder gets a startsWith(projectAbs + sep) containment check (lines 107-116), but the source folder resolved from rule.match during a rename has no equivalent check. resolveTargetFolderFromMatch("../evil/**") yields { ok: true, folder: "../evil" } (since .. has no glob chars), and resolve(projectDir, "..") escapes the project directory. Phase 2 then calls unlinkSync and autoCleanOkDir on the escaped path.

Why: An agent (or crafted MCP input) passing match: "../../../sensitive/**" with new_match: "safe/**" can delete .ok/frontmatter.yml files and empty .ok/ directories at arbitrary filesystem locations outside the project boundary.

Fix: Add the same containment check for sourceAbs:

Suggested change
let sourceFolder: string | null = null;
let sourceAbs: string | null = null;
if (rule.new_match !== undefined && rule.new_match !== rule.match) {
const source = resolveTargetFolderFromMatch(rule.match);
if (!source.ok) {
return {
ok: false,
error: { code: 'MULTI_FOLDER_GLOB', message: source.message, rule: rule.match },
};
}
sourceFolder = source.folder;
sourceAbs = source.folder ? resolve(opts.projectDir, source.folder) : opts.projectDir;
}
let sourceFolder: string | null = null;
let sourceAbs: string | null = null;
if (rule.new_match !== undefined && rule.new_match !== rule.match) {
const source = resolveTargetFolderFromMatch(rule.match);
if (!source.ok) {
return {
ok: false,
error: { code: 'MULTI_FOLDER_GLOB', message: source.message, rule: rule.match },
};
}
sourceFolder = source.folder;
sourceAbs = source.folder ? resolve(opts.projectDir, source.folder) : opts.projectDir;
if (!sourceAbs.startsWith(projectAbs + sep) && sourceAbs !== projectAbs) {
return {
ok: false,
error: {
code: 'PATH_ESCAPE',
message: `Resolved source folder escapes projectDir: ${sourceAbs}`,
rule: rule.match,
},
};
}
}

Refs:

Comment on lines +264 to +278
function readExistingFrontmatter(absPath: string): FolderFm {
if (!existsSync(absPath)) return {};
let content: string;
try {
content = readFileSync(absPath, 'utf-8');
} catch {
return {};
}
let parsed: unknown;
try {
parsed = parseYaml(content);
} catch {
return {};
}
if (parsed == null || typeof parsed !== 'object') return {};
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 MAJOR: readExistingFrontmatter silently drops existing data on write path

Issue: On the write path, if existsSync(absPath) is true (line 265) but readFileSync fails transiently (permission error, disk error), the catch at line 269 returns {}. The caller then merges the patch against empty, silently overwriting the existing title/description/tags with only the patch fields.

Why: Consider: folder has frontmatter.yml with {title: "Engineering", description: "Team docs", tags: ["eng"]}. Agent calls set_folder_rule({ frontmatter: { tags: ["updated"] } }). If readFileSync fails, the merge produces {tags: ["updated"]} — title and description silently lost, agent sees ok: true.

Fix: On the write path, re-throw when the file is known to exist. The caller's Phase 2 try/catch (line 202) will surface it as WRITE_ERROR:

Suggested change
function readExistingFrontmatter(absPath: string): FolderFm {
if (!existsSync(absPath)) return {};
let content: string;
try {
content = readFileSync(absPath, 'utf-8');
} catch {
return {};
}
let parsed: unknown;
try {
parsed = parseYaml(content);
} catch {
return {};
}
if (parsed == null || typeof parsed !== 'object') return {};
function readExistingFrontmatter(absPath: string): FolderFm {
if (!existsSync(absPath)) return {};
let content: string;
try {
content = readFileSync(absPath, 'utf-8');
} catch (err) {
// File exists but unreadable — re-throw so the write path surfaces
// this as WRITE_ERROR rather than silently treating it as empty.
throw new Error(
`Cannot read existing frontmatter at ${absPath}: ${(err as Error).message}`,
);
}
let parsed: unknown;
try {
parsed = parseYaml(content);
} catch {
return {};
}

Comment on lines +93 to +101
// Lazy-create .ok/ and templates/.
mkdirSync(templatesDir, { recursive: true });

const created = !existsSync(filePath);

// Atomic write: tmp + rename so the file-watcher sees one event.
const tmpPath = `${filePath}.tmp.${process.pid}.${Date.now()}`;
writeFileSync(tmpPath, content, 'utf-8');
renameSync(tmpPath, filePath);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 MAJOR: Filesystem errors throw instead of returning {ok: false} — inconsistent error contract

Issue: applyTemplateWrite returns TemplateWriteResult (ok: true | ok: false) for validation errors, but the write sequence here (lines 93-101) can throw on mkdirSync, writeFileSync, or renameSync. The MCP tool wrapper at write-template.ts:109 doesn't catch thrown exceptions either. If writeFileSync succeeds but renameSync fails, a tmp file is also orphaned on disk.

Why: Callers expect a Result type but get an unhandled throw for filesystem errors. The agent sees a generic MCP framework error instead of a structured {ok: false, error: {code: 'WRITE_ERROR', ...}}.

Fix: Wrap the write sequence in try/catch and return a structured error:

Suggested change
// Lazy-create .ok/ and templates/.
mkdirSync(templatesDir, { recursive: true });
const created = !existsSync(filePath);
// Atomic write: tmp + rename so the file-watcher sees one event.
const tmpPath = `${filePath}.tmp.${process.pid}.${Date.now()}`;
writeFileSync(tmpPath, content, 'utf-8');
renameSync(tmpPath, filePath);
// Lazy-create .ok/ and templates/.
mkdirSync(templatesDir, { recursive: true });
const created = !existsSync(filePath);
// Atomic write: tmp + rename so the file-watcher sees one event.
const tmpPath = `${filePath}.tmp.${process.pid}.${Date.now()}`;
try {
writeFileSync(tmpPath, content, 'utf-8');
renameSync(tmpPath, filePath);
} catch (err) {
// Clean up orphaned tmp file if write succeeded but rename failed.
try { unlinkSync(tmpPath); } catch { /* best effort */ }
return {
ok: false,
error: {
code: 'WRITE_ERROR',
message: `Failed to write template at ${relPathOf(input.projectDir, filePath)}: ${(err as Error).message}`,
},
};
}

Comment on lines +103 to +105
// FR5: resolve template — read body from disk + force replace.
let effectiveMarkdown = args.markdown ?? '';
let effectivePosition = args.position;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Minor: Missing mutual-exclusion guard — markdown now optional without template check

Issue: markdown was changed from required to optional to support templates, but there's no guard ensuring at least one of markdown or template is provided. When neither is set, effectiveMarkdown defaults to '' and for position: 'replace', this silently erases the document.

Why: Previously impossible since markdown was required by Zod. Now an agent calling write_document({ docName: "foo", position: "replace" }) (omitting both) will silently replace content with empty string.

Fix:

Suggested change
// FR5: resolve template — read body from disk + force replace.
let effectiveMarkdown = args.markdown ?? '';
let effectivePosition = args.position;
// FR5: resolve template — read body from disk + force replace.
let effectiveMarkdown = args.markdown ?? '';
let effectivePosition = args.position;
if (args.template === undefined && args.markdown === undefined) {
return textResult(
'Error: either `markdown` or `template` must be provided. Omitting both would write empty content.',
true,
);
}

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.

1 participant