fix(okf): harden imports and prepare v3.13.1 - #561
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } catch { | ||
| return err(new Error(`unresolvable import path: ${relPath}`)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Correctness/security: dangling-symlink confinement fix is scoped to OKF import only, not the shared write path
Good catch adding this dangling-symlink check ("A dangling link is not a missing output: writes would follow its target") — but it's added only to this private importPath helper, not to resolveVaultPath/realpathConfined in src/storage/local.ts, which every other write path relies on (vault_write at src/tools/write.ts:503 and :2526, src/backfill/apply.ts, src/interview/transcript.ts).
realpathConfined there treats an ENOENT on any path component the same whether it's "nothing there yet" or "a dangling symlink sits there": it walks up until an ancestor resolves, then lexically reattaches the tail, so a dangling symlink whose target lies outside the vault root still passes confinement. writeFile then follows that symlink and creates the file at its target outside the vault — the same bug class this PR just fixed, left open on the actual MCP-exposed write tools.
Suggest hoisting this component-walk check into resolveVaultPath (or a helper both call) instead of duplicating vault-confinement logic in src/okf/.
There was a problem hiding this comment.
Acknowledged as a pre-existing shared-resolver concern outside this OKF-only patch. Tracked separately for a scoped shared-storage fix and cross-caller regression coverage. This release keeps the supplied import fix focused; it does not claim to harden every write surface.
| const plan: ImportPlanItem[] = []; | ||
| const writtenPaths: string[] = []; | ||
| let skipped = 0; | ||
| const skipped = 0; |
There was a problem hiding this comment.
skipped is now hardcoded to 0 and never incremented — the whole preflight loop below returns err(...) on the first unreadable/unparseable/invalid file instead of collecting a warning and continuing, so a partial-skip outcome can no longer happen. That makes ImportResult.skipped dead weight and makes the r.skipped > 0 branches in src/okf/index.ts:111 and :163 unreachable dead code. Worth removing the field (and those branches) rather than keeping a counter that can only ever be 0.
There was a problem hiding this comment.
Retaining ImportResult.skipped for API compatibility in this patch release. Successful preflight currently returns zero; removing the public result field and CLI handling is unnecessary to fix the import failure semantics.
| for (const doc of prepared) { | ||
| const prior = await catFileBlob(vaultRoot, `HEAD:./${doc.canonicalPath}`); | ||
| if (!prior.ok || prior.value !== doc.text) commitPaths.push(doc.canonicalPath); | ||
| } |
There was a problem hiding this comment.
This spawns one git cat-file blob subprocess per prepared document just to determine which paths changed since the last commit. For a bundle with hundreds of documents that's hundreds of sequential subprocess spawns on every import (including retries where nothing changed). A single batched call — e.g. git diff --name-only HEAD -- <paths> or hashing each doc.text with git hash-object --stdin and comparing against a single git ls-tree HEAD listing — would do the same detection in one or two subprocess calls instead of N.
There was a problem hiding this comment.
Agreed that the sequential Git lookups are an optimization opportunity. Keeping explicit HEAD-content comparisons for this correctness release, including unchanged retries after failed commits. A batched replacement needs coverage for untracked files, staged state, unborn HEAD, and unusual paths; it is a follow-up rather than a release blocker.
1 participant
OKF import now validates the entire document batch before mutation, confines source and destination paths, and reports incomplete writes, commits, or indexing as failures. Retries can finish a previously failed commit or index step without manufacturing an empty commit. Runtime write failures can leave partial files; CLI help and errors describe that state explicitly.
Applies the supplied patch unchanged, including regression tests for path confinement, batch validation, idempotent retries, and failure reporting. Prepares v3.13.1 in package metadata and the changelog so the fix can be released immediately after merge. Release notes also cover dependency and webhook fixes already on main.
Validation on this branch after rebasing onto current main:
Closes #540.