[waterbro-8]

Summary

Independent #347 delivery from 58f407ba: knowledge files can be edited, renamed, archived, restored, and deleted with confirmation. Bound package files (SKILL.md and non-knowledge/*) stay read-only.

Why

Created knowledge documents were stuck read-only, so they accumulated with no lifecycle. This PR adds the missing HTTP/IPC/UI path without mixing Jev work.

What

• Control plane: POST /docs/write|rename|archive|restore|delete, GET /docs/list?archived=1, archived reads
• Desktop IPC/preload/main whitelist for the same operations
• DocsPanel: edit, rename, archive, restore, delete-confirm, archived list
• zh-CN type/size/name display covered by renderer tests

Verification

• npm run build
• npm run typecheck:renderer
• node --test apps/server/dist/test/docs.test.js
• node --test apps/desktop/test/docs-ipc.test.cjs
• renderer: docs-lifecycle, docs-panel, docs-create-ref, tree-management, doc-viewer, i18n, i18n-cjk-gate

Notes

• Base: 58f407ba
• Exact head: 45143982acb9efcf6b95a6bbd9ccb9f73dc421c1
• Product tracking remains open: feat(knowledge): document lifecycle ops and Chinese localization for Knowledge Base #347
• Not mixed with feat(org): overlay Jev ready-host alternative when selected engine is down #465/feat(approvals): overlay Jev aging suggestion on pending items #466
• Independent CODEOWNER review and merge are required

[waterbro-8]

Blocking review on exact head 45143982acb9efcf6b95a6bbd9ccb9f73dc421c1. I converted the PR to Draft while these are open.

1. P1 — mutable routes escape the position/archive roots through ancestor symlinks. requireExistingFile() only lstats the leaf, so knowledge/escape/victim.md is accepted when escape is a symlink to an external directory. The archive root itself is also never verified as a real directory. In a local RED test, POST /docs/write returned 200 and overwrote the external victim; a symlinked .owb-docs-archive made POST /docs/archive return 200 and move knowledge/README.md outside the position package. Guard every path component (including the archive root) at the operation boundary and cover write/rename/archive/restore/delete, not only the leaf.

2. P1 — write/move collision guarantees are TOCTOU-unsafe. requireExistingFile() followed by fs.writeFile() can swap the checked leaf and truncates the live document in place. refuseExistingFile() followed by fs.rename() can overwrite a destination created between the check and rename on POSIX. Please use an atomic same-directory write protocol and a genuinely no-replace move/serialization protocol, with race/collision regression tests. A preflight lstat is not a no-overwrite guarantee.

3. P1 — old async results can cross position/view boundaries in DocsPanel. Changing archivedView clears state but does not increment readVersion, so an in-flight live read can populate the archived view afterward. The save/rename/archive/restore/delete continuations are also unguarded: switching from position A to B while an operation is pending can set A's response/document/selection into B (the old openFile closure can even start a fresh A read after the switch). Add a position+scope operation epoch/identity guard and deferred-promise tests for both read and mutation paths.

4. P2 — the new archived query fails open. archived=banana is silently treated as the live tree; the local RED test got 200 instead of 400. Reject unsupported/duplicated archived values on list/read, and make the desktop options validator reject unknown keys/values rather than silently dropping them.

5. Acceptance boundary remains unresolved. The server currently reports SKILL.md + knowledge/** as binding: "bound", readOnly: true, while this PR makes every knowledge/* file mutable without consulting binding state. The issue calls for read-only while bound and for archived files to stop participating in bindings/references. Please wire and test the actual binding/reference transition, or get the product contract clarified before Ready; path prefix alone is not binding state.

6. Defense in depth: the new destructive Electron IPC handlers ignore event. Apply the existing isTrustedWindowSender(...) check to write/rename/archive/restore/delete and test an untrusted sender.

Reproduction evidence from an isolated application of this PR:

review: write refuses a symlinked knowledge ancestor      200 !== 403
review: archive refuses a symlinked archive root          200 !== 403
review: invalid archived query fails closed               200 !== 400
tests 3; pass 0; fail 3

Keep this Draft until the exact-head fixes and focused regressions are present. Product tracking: #347

[PeterGuy326]

APPROVE on exact head c5bf1e8b286a0db98a0a60e278a80c55dd0c186d. Reviewed the cumulative #347 lifecycle change after updating onto current main. Verified locally: npm run build, renderer typecheck, docs-lifecycle 4/4, docs-panel 7/7, CJK gate, desktop docs IPC 8/8, server docs 15/15, and diff check. Write/rename/archive/restore/delete remain confined to knowledge/; SKILL.md stays bound read-only; traversal/symlink/archive-root/collision paths fail closed; late reads/writes are scoped. This is the required write/lifecycle prerequisite for follow-up #511, not the Obsidian-style autosave editor itself. PR is still Draft and current-head hosted CI is in progress, so do not merge until the author marks Ready and all checks finish green.

[PeterGuy326]

Independent exact-head review is now APPROVED on c5bf1e8b286a0db98a0a60e278a80c55dd0c186d; local build and focused server/IPC/renderer/CJK gates pass. Hosted CI is running. When all current-head checks are green, please mark the PR Ready (it remains Draft) and request final current-head review/merge. This is the prerequisite for #511.

[PeterGuy326]

@waterbro-8 Current exact head c5bf1e8b286a0db98a0a60e278a80c55dd0c186d is CLEAN, all hosted checks are green, and independent APPROVE is recorded. The remaining blocker is Draft state. Please mark Ready if the lifecycle scope is final; do not change the head unless needed, because a new SHA requires fresh review.

[sun-970]

Review — feat(knowledge): document lifecycle ops for Knowledge Base

Refs #347. CI is fully green on this head (both Node matrix jobs, both installers, both staging smokes, layout parity, CodeQL, dependency review, scorecard). This is a genuinely well-hardened change and most of my comments are about the description and two design points, not defects.

The path-safety work is the strongest part of this PR

I read every one of the 17 files. The filesystem hardening is thorough in a way that is unusual, and it is worth enumerating because the review will otherwise read as if I only found problems:

• assertMutableKnowledgePath rejects SKILL.md and anything outside knowledge/, so the bound package files are structurally unwritable — the read-only rule is enforced by path predicate, not by a UI affordance.
• Two independent escape checks. resolveArchivePath rejects .., absolute results, and any hidden segment; assertRealFileChain walks each segment with lstat and refuses symlinks at every level, not just the leaf. That is the correct defence against a symlinked intermediate directory, which a naive realpath-at-the-end check would miss.
• moveNoReplace uses fs.link + unlink rather than rename. This is the right primitive: rename would silently overwrite an existing destination, and link returning EEXIST → 409 docs_exists gives you no-clobber semantics for free. Archive/restore round-trips cannot destroy a file.
• atomicReplaceRegularFile writes to a wx-opened temp with mode 0600, then renames, then chmods, and unlinks the temp on every failure path. Correct crash-safety, and a real improvement over a plain writeFile.
• withPositionLock serialises mutations per position. Given that archive/restore rename within the same tree, this is load-bearing rather than decorative.
• assertOrCreateRealArchiveRoot re-lstats after mkdir and treats EEXIST as "verify what is actually there", so a pre-planted symlink named .owb-docs-archive is rejected rather than followed.

Two things I want changed before merge

1. readOnly: true → false on the bound context source contradicts the PR's own rule.

     {
       locator: `positions/.../SKILL.md + knowledge/**`,
       binding: "bound",
-      readOnly: true,
+      readOnly: false,

The description says "Bound package files (SKILL.md and non-knowledge/*) stay read-only." But this flag describes the SKILL.md + knowledge/** source as a whole, and it now reports readOnly: false. So SKILL.md — which assertMutableKnowledgePath explicitly refuses to write — is advertised to whatever consumes readOnly as mutable. Either:

• the flag is a coarse per-source hint (in which case say why SKILL.md's true read-only-ness is carried by the path predicate and not by this field), or
• the flag is an input to some other enforcement, in which case this is a real hole.

Whichever it is, the current state is that a boolean in the shared context-source contract and a 403 predicate say different things about the same file. Please resolve it explicitly, and if readOnly is informational, consider whether a per-glob or per-entry granularity is needed now that one source contains both mutable and immutable paths.

2. The IPC trust check is applied inconsistently, and this PR makes that pattern worse.

You added authorizeDocsIpcSender and used it on all five new handlers — good. But the existing owb:position:docs:list and owb:position:docs:read handlers still take _event and never check the sender:

ipcMain.handle("owb:position:docs:list", async (_event, positionId, options) => {
  const validated = validateDocsListRequest(positionId, options);

And owb:position:docs:create — which existed before, and which is now joined by five siblings that do check — is also still unguarded. So after this PR the docs module has a split trust model: the read path and the create path accept an untrusted sender while write/rename/archive/restore/delete reject one.

An attacker who can reach ipcRenderer can therefore still create a document (and read the whole tree) without the sender check, which is a strictly larger capability than the one you just gated. Please apply authorizeDocsIpcSender to docs:list, docs:read, and docs:create in this PR. The helper already exists, so this is three lines — and leaving the inconsistency in place makes the new guards look like they were added per-handler rather than as a boundary.

Description/record gaps

The body is a Summary + Why + What + a command list. It is missing:

• Refs #347 is correct but insufficient — #347 is "document lifecycle ops and Chinese localization for Knowledge Base". Note the title you wrote omits the localization half. Either state that localization is out of scope for this PR (and where it lands), or cover it.
• Baseline is oddly specific. "Base: 58f407ba" plus "Exact head: 4514398..." pins a base that has certainly moved. State the merge base branch and rebase; mergeable_state is currently dirty, so a reviewer cannot tell whether the pinned base is the cause.
• No File domains, Scope and non-goals, Security and compatibility, Known limitations, Risk and rollback, or validation ledger. For a change that adds five filesystem-mutating endpoints, Security and compatibility is not optional. Specifically: context-sources.ts changes a field in a shared contract — is readOnly: false a breaking change for any existing consumer? Say so either way.
• The verification list has no results. "npm run build, npm run typecheck:renderer, node --test apps/server/dist/test/docs.test.js, …" — please add the observed N/N for each, matching #524 in this repo, which does this properly.
• "Not mixed with #465/#466" is a useful claim but it is not verifiable from the description. Make it a scope statement in the Scope and non-goals heading instead of a trailing note.

Design point worth a decision

Archive and restore both move the file out of / back into the live tree, and handleDocsList with archived=1 walks .owb-docs-archive. That means the archive is a directory inside the position directory, which is also the tree that handleDocsList walks unarchived. Two consequences I would like stated:

1. walkFiles on the live tree — does it skip .owb-docs-archive? If it does not, archived documents appear in the normal list, which would make the archive invisible-but-duplicated rather than hidden.
2. The archive is per-position and undiscoverable except via the flag. If a position is deleted, do its archived documents go with it, and is that intended?

The tests presumably cover (1); I could not confirm it from the diff alone, and it is the kind of thing that is obvious to the author and invisible to a reviewer. One sentence in the description would close it.

Not blocking

• validateDocsDeleteRequest normalises archived: false to omitted rather than passing false through, while the server's parsePositionPathBody accepts an explicit boolean. Both are safe (the server treats undefined as live-tree delete, and false is not accepted on the path-request variants at all), but the asymmetry is the kind of thing that becomes a bug when a third caller appears. A note that archived: false is intentionally equivalent to absent would help.
• The zh/en locale additions are in step, which I checked — no untranslated keys on either side.

[PeterGuy326]

Maintenance decision (owner-authorized cleanup, #607): close as absorbed by main. The lifecycle implementation landed through #523; #564 adds expectedVersion protection for writes. Fresh current-main regressions pass 16/16 server document tests and 8/8 IPC tests, including lifecycle isolation, archive exclusion, no-clobber moves, sender authorization and stale-version writes. The old branch would regress the newer write contract. #347 is audited independently for lifecycle/localization closure; no customer acceptance is inferred from this PR closure.