diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a0e895..3a48b75 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,72 @@ All notable changes to `wiki` are documented here. This project follows [semantic versioning](https://semver.org); while pre-1.0, breaking changes bump the minor version. +## v0.9.0 — 2026-08-09 + +The release that makes `wiki` importable. Everything that makes the tool useful now lives in public packages, so a Go program can build a bundle index and query it directly instead of spawning the CLI and parsing its output. The CLI runs on those same packages, so there is no second implementation of anything. + +### Breaking + +1. **Package paths.** `bundle`, `index`, and `parse` moved out of `internal/` to the module root. Nothing could import them before, so this breaks no existing code — it is listed because the paths are now a commitment. +2. **A malformed `wiki.toml` is an error** rather than being silently half-read. Input that was never valid TOML but happened to scan — most likely unquoted array items, `types = [note, concept]` — now fails with a line number. +3. **Two json keys renamed**, for consistency across commands: `checkboxes` reports the entry as `entry` (was `file`), and `unresolved` reports a link's far end as `to` (was `target`). csv/tsv headers follow; text output is unchanged. +4. **`Index.OutLinks(*Entry)` is now `Index.Links(path string)`**, and returns one ref per occurrence rather than unique targets. +5. **`Entry.SetFields` takes `map[string]any`** (was `map[string]string`), accepting a `string` or a `[]string` per key. + +Two behaviour changes worth knowing that are not API breaks: writing into a read-only directory now fails where a plain write succeeded, and a hardlinked entry now diverges instead of sharing an inode. Both are consequences of atomic writes and both are argued below. + +### Changed + +- **The core packages are importable.** `bundle`, `index`, and `parse` moved out of `internal/` to the module root, so a Go program can build a bundle index and query it directly instead of spawning the CLI and parsing its output. `output` (CLI presentation) and `wikilink` (a compat shim `index` uses without exposing) stay internal. The CLI is unchanged and still runs on the same packages, so there is no second implementation of anything. + + Shelling out is a fine contract for occasional whole-bundle questions and a poor one for a consumer asking many small ones, since every invocation re-reads and re-parses the whole tree: cost scales with interaction rather than with change. It also forced consumers to reimplement rules that have one correct home here, which is how a separate UI ended up carrying its own frontmatter writer and its own link resolver. + +- **`index.ParseFilter` reads a `key=value` / `key!=value` expression.** The spelling is part of the query contract rather than of the CLI's argument handling, so it moved out of `cmd/wiki`; the flag now calls it. Any consumer accepting the same syntax gets the same parse, including the details that are easy to miss (`!=` matched before `=`, so a value may contain `=`; the value unquoted the way frontmatter is). + +- **A write API on `Entry`.** `SetField`, `SetFields`, `UnsetField`, and `SetCheckbox` change frontmatter and checkboxes surgically: the matching lines are replaced and every other byte is left alone, never parsing the frontmatter into a map and re-serializing it, which would silently drop nested maps, anchors, comments, and quoting style. `SetFields` applies several keys in one pass, since two writes can leave an entry half-updated. Each refreshes the entry in place, because inserting or removing a line shifts the line numbers that links, checkboxes, and headings all carry. `SetCheckbox` is keyed by line, the only stable identity a checkbox has. + + `SetFieldList` writes list-valued fields (`tags`, `blockers`, and anything else the bundle spells as a list). Separate from `SetField` because a list is not a string that happens to contain brackets: passing `"[a, b]"` to `SetField` writes `key: "[a, b]"`, correctly quoted for a scalar and a one-element list when read back. A key already written as a block list stays one, so the API does not reformat frontmatter it was only asked to change. + + `SetFields` takes `map[string]any`, accepting a `string` or a `[]string` per key and rejecting anything else by name and type. That mirrors `Frontmatter`, which returns the same shape, so a consumer can read the frontmatter, edit it, and write it back. With scalars and lists in separate calls, setting a status and a tag list together took two writes — precisely the half-updated entry one pass exists to prevent. Frontmatter is genuinely heterogeneous, so `map[string]string` was never the honest type for it. + +- **json keys are consistent across commands.** There were five names for two concepts: `checkboxes` called the entry `file` while `check` called it `entry`, and `unresolved` called a link's far end `target` while `links` and `backlinks` called it `to`. Now three categories with one name each — `_path` for rows that merge your frontmatter and so need a reserved key (`list`, `orphans`), `entry` for rows *about* an entry (`check`, `checkboxes`), and `from`/`to` for link rows (`links`, `backlinks`, `unresolved`). csv/tsv headers follow, since they derive from the json fields; text output is unchanged. `wiki version` also no longer claims to accept `--format`. + +- **`Index.OutLinks(*Entry)` is now `Index.Links(path string)`**, the mirror of `Backlinks(path string)`: same shape, same return type, both yielding `nil` for an unknown path. It also stops de-duplicating by target and returns one `LinkRef` per occurrence, as `Backlinks` always has. De-duplicating while still reporting a `Line` made that line the first of several, silently. Which behaviour is right depends on how the result is shown — `wiki links` prints a bare target so it collapses repeats, `wiki backlinks` prints `file:line` so it shows each one — and that is a presentation choice, so it moved to `cmd/wiki`. CLI output is unchanged. + +- **Reading and resolving, for consumers.** `Entry.Field`, `Entry.FieldList`, and `Entry.Frontmatter` reach arbitrary frontmatter; `FieldList` applies the same scalar-as-one-element-list rule matching uses, so filtering by hand agrees with `--where` rather than being subtly different. `Index.ResolveLink` and `RelativeLink` expose both directions of link spelling. + +- **`--where` works on the vocabulary commands.** `tags`, `properties`, `property`, and `checkboxes` now take `--where key=value` / `key!=value` with the same semantics `list` has (repeatable, ANDed, list-match-any), composing with `--prefix`. + + `--prefix` scoped every one of these and `--where` scoped none, so a subtree could be narrowed anywhere and a field could not. The gap showed the moment a folder held more than one kind of entry: a backlog that also holds notes reported *their* statuses (`published`, `retired`) beside the tasks', with no way to ask the narrower question short of post-processing `list --format json` through `jq`. Now there are two filters, available wherever a set of entries is narrowed: **`--prefix` for where, `--where` for what.** `checkboxes` is included because its unit is a `- [ ]` line but its *scope* is still a set of entries; a named `[file]` stays explicit and ignores both filters. + + Library signatures gained the parameter: `TagCounts`, `PropertyKeyCounts`, and `PropertyValueCounts` each take `props []PropFilter` alongside the path prefix. `checkboxes` also dropped a second copy of prefix matching and now routes through `Index.Filter` like everything else. + +- **`wiki.toml` is parsed as TOML, and `[tool.*]` is reserved.** `bundle` now uses `BurntSushi/toml` (one dependency, no transitive ones) instead of a hand-rolled line scanner, and `[tool.]` tables are space granted to other tools over the same bundle: never parsed by `wiki`, never validated, never warned about. `bundle.Bundle.Tool` carries them and `DecodeTool` unmarshals one into a caller's own struct, so no tool writes a second `wiki.toml` parser. + + Without the namespace, a tool with an opinion about a bundle had to put it in a satellite config beside `wiki.toml`, and a second tool meant a third file. `pyproject.toml` is the precedent: one file describes the directory, tools namespace their own settings inside it. Reserving space adds no opinion to the format — `wiki` gains no field it interprets and no behaviour. + + **Behaviour change:** a malformed `wiki.toml` is now an error instead of being silently half-read. The config decides what counts as an entry and which types are valid, so carrying on with a partial parse produced confidently wrong answers. This also means input that was never valid TOML but happened to work — most likely unquoted array items, `types = [note, concept]` — now fails with a line number instead of being accepted. + +### Fixed + +- **Docs described the old canonical link form.** The format spec still called root-absolute links canonical and said `tidy --links` rewrote *to* root-absolute; since relative became the canonical on-disk form (v0.7.0) both are backwards. The scaffolded `AGENTS.md` contradicted itself, describing relative links in one section and `tidy --all` producing absolute ones in another, and both READMEs told Obsidian users to write *Absolute path in vault*. The spec also still documented `skip`, a field renamed to `ignore` long ago — so a config copied from the spec silently did nothing and warned as an unknown key — and never documented `ignore_orphans` at all. + +- **A quoted list item containing a comma was read as two broken items.** `parse` split a frontmatter flow list on every comma without honouring quotes, so `tags: ["a,b", "c"]` came back as `["\"a", "b\"", "c"]` — valid YAML, silently mis-parsed, in any bundle that spelled a list that way. The split now tracks the open quote. + +- **A key inside any `wiki.toml` table silently overrode bundle config.** The line-based reader ignored table headers, so every key was treated as top-level: `[tool.wikiview] types = [...]` replaced the bundle's `types` vocabulary, last-one-wins, and entries with undeclared types then passed `check` clean. Any table containing a key named `spec`, `types`, `ignore`, or `ignore_orphans` reconfigured the bundle from inside a namespace that was supposed to be inert. + +- **A multi-line array in `wiki.toml` silently disabled the setting.** `types = [` parsed to an empty list, which means "no vocabulary declared", which allows every type — the exact opposite of what the author wrote, with nothing reported. The one-line spelling of the same vocabulary errored on an undeclared type as intended. Valid TOML that any other tool would read correctly, so nothing suggested it was being misread. + +- **Unknown-key warnings name the full path.** They reported the leaf key with its table stripped, so a nested `path` or `columns` was unfindable in a file with several tables. Now `nested.key`, and only the shallowest unrecognized key is reported, since flagging every key inside an unknown table is noise rather than information. + +- **Writes are atomic.** Every file the engine rewrites now goes through a temp file and a rename instead of `os.WriteFile`, which opens with `O_TRUNC` and so empties the file before the new content lands. Anything reading in that window — an editor, an agent, a watcher, another `wiki` run — could see an empty or partial entry, and a process killed mid-write left it truncated on disk. Measured against a concurrent reader, roughly 9% of reads saw a torn file before; none do now. Permissions are preserved across the rename, and a symlinked entry is written through rather than replaced. Atomicity is per file: a command rewriting several can still be interrupted between them, which is a separate concern. A hardlinked entry now diverges instead of sharing an inode, which is the point: two names are two entries at two paths, so each needs its own relative links, and the shared inode meant one of them ended up pointing nowhere. Writing into a read-only directory now fails where a plain write succeeded. + + **Not every system allows a replace while a file is open**, and on those the write now fails where the plain write it replaced succeeded. The replace retries briefly, but only while the error is contention, so a genuine permission error still fails at once; that covers the common cause, which is transient and not the user's doing (a scanner or indexer opening a file it just saw change). Contention held longer still fails, and the durable fix is filed as debt rather than rushed. Where a replace does not care who is reading, there is no retry and no cost. + +- **`move --include-frontmatter` finds relative frontmatter refs.** The flag shipped when root-absolute was still the canonical link form, so it matched frontmatter values by exact string equality against the moved entry's root-absolute path. Once relative links became canonical for bodies (v0.7.0), the natural spelling was the broken one: `blockers: [./task-1.md]` never equalled `/active/task-1.md`, so the flag silently did nothing and the ref dangled. Frontmatter refs are now **resolved** and matched the way body links are, so relative and root-absolute are both found, and they are **normalized to root-absolute** on write. The moved file's own relative refs are normalized too, closing the cross-folder dangle that body links already handled. Anchors are preserved and out-of-bundle values are left as authored. Only values ending in `.md` are treated as references, which is what keeps the pass from rewriting ordinary metadata like `title: Some Note` (an arbitrary string resolves to a valid in-bundle path). The opt-in caveat is unchanged: the flag rewrites every matching value, snapshot fields included. + + **Frontmatter stays root-absolute on purpose**, and does not follow bodies to relative. A root-absolute value is a *stable key*, so every entry referencing `/epics/x.md` spells it identically and `wiki list --where epic=/epics/x.md` finds them all; a relative value spells the same target differently from each directory, so no single `--where` query can match every referrer (matching is exact string equality, by design, so the tool never has to guess that a value is a path). The rendering argument that motivated relative bodies does not apply either, since frontmatter is never rendered as a link. The rule: **a body link is relative because it must navigate; a frontmatter ref is root-absolute because it must be a stable key.** + ## v0.8.0 ### Changed diff --git a/README.md b/README.md index bf1d055..0b2a1cd 100644 --- a/README.md +++ b/README.md @@ -61,7 +61,7 @@ wiki list # every entry That's the whole thing: a folder of plain Markdown. Open it in any editor, commit it to git, point Obsidian at it, point an agent at it. `wiki` simply makes it queryable and keeps it honest. > [!NOTE] -> If you use Obsidian, set it to write standard markdown links: Files and links → turn off **Use [[Wikilinks]]**, set **New link format** to *Absolute path in vault*, turn on **Automatically update internal links**. (`wiki` recognizes `[[wikilinks]]` for compatibility but flags them in `wiki check`, and `wiki tidy --wikilinks` converts them to standard links). See the [format spec](https://github.com/agentic-wiki/spec#links). +> If you use Obsidian, set it to write standard markdown links: Files and links → turn off **Use [[Wikilinks]]**, set **New link format** to *Relative path to file* (the canonical on-disk form, so links navigate in any renderer), turn on **Automatically update internal links**. (`wiki` recognizes `[[wikilinks]]` for compatibility but flags them in `wiki check`, and `wiki tidy --wikilinks` converts them to standard links). See the [format spec](https://github.com/agentic-wiki/spec#links). ## What you can ask it @@ -79,6 +79,8 @@ wiki outline /tech/infra/hetzner.md # its headings wiki table /finance/expenses.md --format csv # a dataset's table as rows (csv/json), for jq/duckdb ``` +Two filters narrow any set of entries: **`--prefix` for where, `--where` for what.** Both work on `list`, `search`, `checkboxes`, `tags`, `properties`, and `property` — so a folder that mixes kinds can still be asked a narrow question. + **Follow the graph** (the part `grep` cannot do) ```sh @@ -97,6 +99,7 @@ wiki checkboxes # every open - [ ] checkbox, across the whole wiki list --where type=task # list task entries (detailed entries) wiki tags --counts --sort=count # what you write about most wiki property status --counts # how many open vs done, draft vs final +wiki property status --counts --where type=task # …only the tasks', if the folder mixes kinds ``` **Reshape it safely** @@ -211,7 +214,7 @@ wiki check # the backlog stays conformant ## Design - **Standalone first:** agents call `wiki` directly, no server in the way. -- **Minimal on purpose:** zero external dependencies, a single static binary, native on macOS, Linux, and Windows. Git is recommended but entirely optional. +- **Minimal on purpose:** one dependency (a TOML parser, for `wiki.toml`), a single static binary, native on macOS, Linux, and Windows. Git is recommended but entirely optional. - **Files are truth:** the index is derived from disk and fully disposable. ## Develop diff --git a/backlog/2-query-surface/016-list-body.md b/backlog/2-query-surface/016-list-body.md new file mode 100644 index 0000000..8e88847 --- /dev/null +++ b/backlog/2-query-surface/016-list-body.md @@ -0,0 +1,20 @@ +--- +type: task +title: "list --body: bodies in the same json call" +status: todo +priority: low +tags: [feature, query] +--- + +`wiki list --format json` carries every entry's full frontmatter plus `_path`, which makes it a one-call snapshot of a whole bundle's metadata. It carries no **body**, so a consumer that wants to show content alongside metadata has to follow up with one `wiki read` per entry: N process spawns, each rebuilding the whole index, to fetch text the `list` pass already had open. + +Surfaced by `wikanban` (card excerpts on a board), but it applies to any renderer, exporter, or static-site generator over a bundle. + +**Proposal:** `wiki list --body`, json only, adding a `body` key per entry (frontmatter stripped, exactly what `read` returns). + +- **json only.** Bodies are multi-line; they do not belong in text/csv/tsv output. Reject the flag on other formats rather than emitting something unusable. +- **Opt-in**, because it changes the cost profile: `list` reads bodies on demand today (only `SortTime` stats timestamp-less entries), so the default stays cheap. +- **Reuses `Entry.Body()`**, so there is one definition of "the body" across `read` and `list`. +- The key is `body`, matching `read --format json`'s existing `{_path, type, body}` shape. Note the collision risk with a user's own `body:` frontmatter field, which `MarshalJSON` emits verbatim; either accept that it wins (consistent with frontmatter being the user's namespace) or reserve `_body` like `_path`. **Leaning `_body`**, for the same reason `_path` earned its underscore. + +**Acceptance:** `list --body --format json` includes each entry's body; other formats error; the flag composes with `--where`/`--prefix`/`--sort`; one bundle read, no extra spawns. Consider the same flag on `search`. diff --git a/backlog/2-query-surface/017-where-on-vocabulary-commands.md b/backlog/2-query-surface/017-where-on-vocabulary-commands.md new file mode 100644 index 0000000..f2c6f27 --- /dev/null +++ b/backlog/2-query-surface/017-where-on-vocabulary-commands.md @@ -0,0 +1,36 @@ +--- +type: task +title: "--where on the vocabulary commands (tags, properties, property)" +status: done +priority: medium +tags: [feature, query, consistency] +--- + +`--prefix` and `--where` are both filters, but only one of them works everywhere. `--prefix` scopes `list`, `search`, `checkboxes`, `tags`, `properties`, and `property`; `--where` works on `list` and `search` alone. So a subtree can be narrowed anywhere, and a field cannot. + +The gap shows up the moment a folder holds more than one kind of entry: + +```sh +wiki property status --prefix /backlog # every status under /backlog… +wiki property status --prefix /backlog --where type=task # …no such flag +``` + +A backlog folder that also holds notes, articles, or places reports *their* statuses (`published`, `retired`, `visit-only`) alongside the tasks', with no way to ask the narrower question. Today the workaround is to leave the vocabulary commands behind entirely and post-process `list --format json` through `jq`, which is a lot of ceremony for "how many tasks are in each status". + +Surfaced by `wikanban`, which used `property status --prefix` to discover a board's columns and got a column per foreign status. It no longer needs this (columns now come from the board's own filtered entries, which is both more accurate and one fewer spawn), so **nothing is blocked on it**. Filed because the asymmetry is real on its own terms. + +**Proposal:** accept `--where key=value` / `key!=value` on `tags`, `properties`, and `property`, with exactly the semantics `list` has (repeatable, ANDed, list-match-any, empty value tests emptiness). + +- The filter already exists as `index.PropFilter` and `Filter(prefix, props)`; these commands currently call the prefix-only path, so this is mostly threading the same flag through, not new matching logic. +- `checkboxes` is the interesting one: its unit is a `- [ ]` line, not an entry, but `--where` would still be a meaningful *entry* filter ("open checkboxes on blocked tasks"). Decide whether to include it or keep it prefix-only, and say which in the help. +- Keeps the story simple to explain: **two filters, `--prefix` for where and `--where` for what, both available wherever a set of entries is being narrowed.** + +**Acceptance:** `wiki property status --where type=task` reports only tasks' statuses; the same for `tags` and `properties`; `--where` composes with `--prefix`; docs and `AGENTS.md` updated so the two filters are described as the pair they are. + +**Done.** `--where` accepted on `tags`, `properties`, `property`, and `checkboxes`, with `list`'s exact semantics and composing with `--prefix`. + +`checkboxes` was included: its unit is a `- [ ]` line, but its *scope* is a set of entries, so the filter is meaningful there and excluding it would have made the story "two filters everywhere, except one place" — which is the asymmetry this task existed to remove. A named `[file]` stays explicit and ignores both filters. + +Mostly threading, as expected: the counters already routed through `Index.Filter(prefix, nil)`, so they took a `props []PropFilter` parameter and passed it on. `checkboxes` was the exception — it carried its own inline copy of prefix matching rather than calling `Filter`, behaviourally identical but a second implementation of the same rule; it now routes through `Filter` like everything else. + +Help, README, and CHANGELOG describe the pair rather than two separate flags: **`--prefix` for where, `--where` for what.** diff --git a/backlog/3-graph-and-mutation/015-frontmatter-relative-refs.md b/backlog/3-graph-and-mutation/015-frontmatter-relative-refs.md new file mode 100644 index 0000000..fc773b3 --- /dev/null +++ b/backlog/3-graph-and-mutation/015-frontmatter-relative-refs.md @@ -0,0 +1,39 @@ +--- +type: task +title: "move --include-frontmatter: find relative frontmatter refs, normalize to root-absolute" +status: done +priority: high +tags: [bug, graph, links] +--- + +**Done (2026-08-07):** frontmatter refs are now matched by *resolved target*, so a relative ref is found as readily as a root-absolute one, and normalized to the canonical **root-absolute** form on write. + +[move --include-frontmatter](./011-move-frontmatter-refs.md) shipped when root-absolute was still the canonical on-disk link form, so it matched frontmatter values by **exact string equality against the moved entry's root-absolute path**. Then [relative links became canonical](./012-switch-canonical-links.md) for bodies, and frontmatter was left behind. Two bugs fell out: + +- **A relative frontmatter ref was invisible to `move`.** `blockers: [./task-1.md]` never equalled `/active/task-1.md`, so the flag silently did nothing and the ref dangled. The natural spelling, the one that matches every body link in the same bundle, was the one that broke. +- **The moved file's own relative frontmatter refs dangled on a cross-folder move.** `move` had already gained this responsibility for body links; frontmatter never got the same treatment. + +The fix mirrors the body-link pass in `Move`, in a new `frontmatterRewrites`: + +- **Match by resolved target, not spelling.** Each value goes through `normalizeLink` (from the entry's *current* path), so relative and root-absolute refs are both found. +- **Write root-absolute.** See below: this is where frontmatter deliberately parts company with body links. +- **Normalize the moved file's own relative refs**, which would otherwise dangle from its new directory. +- **Anchors survive** (`spec: /lib/c.md#usage`), and **out-of-bundle values are left exactly as authored**, as out-of-bundle body links are. + +## Why frontmatter does not follow bodies to relative + +Writing relative was implemented first and **reverted**, because it broke the one thing the frontmatter-field recipe exists for. + +A root-absolute value is a **stable key**: every entry referencing `/epics/onboarding.md` spells it identically, so `wiki list --where epic=/epics/onboarding.md` finds them all. A relative value is a *per-file* spelling, so the same target reads `./x.md` from the root and `../epics/x.md` from `active/`, and **no single `--where` query can match every referrer**. Since `--where` is exact string equality (deliberately, so the tool never has to guess that a value is a path), relative storage silently returns a subset. It would also convert existing bundles on their first move, breaking queries that used to work. + +Meanwhile the argument that motivated relative bodies does not apply at all: 012 flipped body links because root-absolute breaks navigation on GitHub, in editors, and in VS Code preview. **Frontmatter is never rendered as a link anywhere** (GitHub shows a table, Obsidian shows properties), so there is no navigation to preserve. 012 said as much and was right. + +The resulting split is a principle worth stating, not an inconsistency: **a body link is relative because it must navigate; a frontmatter ref is root-absolute because it must be a stable key.** + +Rejected along the way: having `--where` resolve path-shaped values before comparing, so relative storage stays queryable. It would force the tool to decide which values are paths (the opacity 011 protected) on the hottest query path, break the property that `--where` matches exactly what `list --format json` prints, and still leave `property --counts`, `jq`, and every other consumer seeing fragmented spellings. + +**The `.md` suffix is the whole heuristic**, and it is what makes this safe. Resolving *every* frontmatter value would rewrite `title: Some Note` into a path, since an arbitrary string resolves to a perfectly valid in-bundle one. Requiring the value to end in `.md` (after any `#anchor`) means only unmistakable path references are touched. The opt-in caveat from 011 is unchanged and still applies: the flag rewrites *every* matching value, including a snapshot field that happens to name the moved path. + +Unchanged and still deliberately out of scope (both rejected in 011 as opinionated): `check` guessing at dangling frontmatter refs, and markdown links inside frontmatter. A plain path stays `--where`-filterable and does not collide with YAML flow-sequence syntax; wrapping it in `[text](path)` would sacrifice both for no gain, since frontmatter is not rendered. + +Tests: `TestMoveIncludeFrontmatterRelative` (relative inbound refs, the moved file's own refs across folders, anchors, non-path values, out-of-bundle values), plus updated expectations in `TestMoveIncludeFrontmatter` and `TestCmdMoveIncludeFrontmatter`. diff --git a/backlog/3-graph-and-mutation/016-set-frontmatter.md b/backlog/3-graph-and-mutation/016-set-frontmatter.md new file mode 100644 index 0000000..7b6cb55 --- /dev/null +++ b/backlog/3-graph-and-mutation/016-set-frontmatter.md @@ -0,0 +1,28 @@ +--- +type: task +title: "wiki set: write a frontmatter field from the CLI" +status: todo +priority: low +tags: [feature, mutation] +--- + +A `wiki set =` command, writing through `index.SetFields`. + +## Why it survived the library work + +The public API made this look redundant, and for Go consumers it is: `SetField` and `SetFields` are the surface, and `wikiview` calls them directly. It stays open because **the CLI's constituency is agents**, and an agent is not a Go program. + +`wiki` is described as "built for agents to call". An agent moving a task from `todo` to `in-progress` today opens the file and edits the frontmatter with its own text tools — hand-rolling the exact surgical edit `SetFields` exists to prevent, with none of its guarantees: no quoting rule, no block-list handling, no atomic write, no "change this key's lines and nothing else". That is the second-implementation failure the retro names, relocated outside the module where the guard test cannot see it. + +So the case is not "a consumer needs it" but "the largest class of caller cannot reach it". + +## Shape + +- `wiki set =` — repeatable, so several fields land in one pass (`SetFields` already exists for exactly this reason: two writes can leave an entry half-updated). +- `--unset ` for removal, mapping to `UnsetField`. +- Reserved keys (`_`-prefixed) are rejected by the library; the CLI surfaces that error rather than reimplementing the check. +- No `--force`, no type coercion: the value is written as the string given, quoted only when YAML requires it. That is the library's rule and the CLI must not add a second one. + +**Not in scope:** checkbox toggling from the CLI. `SetCheckbox` is keyed by line number, which is a fine API and a poor command-line ergonomic; it wants its own design if anyone asks. + +**Acceptance:** an agent can set and unset frontmatter fields without opening the file; the command is a thin wrapper with no editing logic of its own; existing conformance and tidy behaviour is unchanged. diff --git a/backlog/4-release-and-docs/005-public-packages.md b/backlog/4-release-and-docs/005-public-packages.md new file mode 100644 index 0000000..c53561e --- /dev/null +++ b/backlog/4-release-and-docs/005-public-packages.md @@ -0,0 +1,60 @@ +--- +type: task +title: "promote the core packages to an importable API" +status: done +priority: high +tags: [feature, api, architecture] +--- + +Everything that makes `wiki` useful lives under `internal/`, so Go forbids anyone else importing it. Any other program over a bundle must therefore spawn the CLI and parse its output. + +That is a reasonable contract for **occasional, whole-bundle questions**, and it is what `wikanban` was built on. It stops being reasonable the moment a consumer asks *many small* questions: a bundle browser wants an entry, its backlinks, a search, a tag list, a folder tree, and each of those is currently a process spawn that re-reads and re-parses the entire content tree. The cost scales with interaction rather than with change, which is exactly backwards. + +The tool is described as "a neutral, deterministic engine that indexes the folder and answers structured queries, **built for agents to call, runnable by anyone**". A Go program is the one caller that currently cannot. + +## What to promote + +- **`bundle`** — locate a bundle, read `wiki.toml`. Small, stable, already a clean surface. +- **`index`** — build, query (`Filter`, `Search`), the graph (`Links`, `Backlinks`, `Orphans`, `Broken`), and mutation (`Move`, `Check`, `Fix`, `Tidy`). +- **`parse`** — required because `index`'s API exposes its types (`Checkbox`, `Heading`, `Table`). + +**Stays internal:** `output` (CLI presentation, not a library concern) and `wikilink` (a compat shim `index` uses without exposing). + +Top-level `bundle/`, `index/`, `parse/` rather than a `pkg/` directory, which modern Go does not favour. + +## What it buys + +- **No second implementations.** `wikanban` currently carries its own surgical frontmatter writer purely because `setFrontmatterValue` is unreachable, and its own link resolver because `normalizeLink` is. Both are deliberate stand-ins, both are drift risks, and both evaporate here. (`wiki set` is still worth having for CLI users, but this removes the pressure behind it.) +- **One index, held in memory.** A consumer builds it once and answers queries from it, instead of paying O(bundle) per question. The `.wiki` incremental cache becomes an optimisation rather than a prerequisite. +- **No runtime dependency.** A Go consumer stops needing the `wiki` binary on `PATH` at all, which removes a whole class of "works on my machine". +- **Stronger correctness than shelling out.** "Cannot disagree with `wiki list`" becomes "is the same code as `wiki list`". + +## To decide before building + +- **API stability.** Promoting is a commitment. Pre-1.0 this is acceptable with a stated policy, but say it out loud rather than discovering it through a breaking change. +- **What `Index` exposes.** Today `Entry` keeps `fm` and `abs` unexported with accessors; that is a good instinct to preserve. Decide deliberately what a consumer may reach, rather than exporting fields because the move made it easy. +- **`Build` cost and reuse.** A long-lived consumer wants to rebuild incrementally, or at least cheaply. Worth pairing with [.wiki cache](../3-graph-and-mutation/004-incremental-cache.md). +- **The CLI must keep using the same packages**, or the library becomes a second implementation of the thing it was extracted from. + +## Done + +**The move.** `bundle`, `index`, and `parse` are at the module root; `output` and `wikilink` stayed internal. An external module has been verified building an index, filtering, and walking the graph against a real bundle. `--where` parsing moved from `cmd/wiki` into `index.ParseFilter`, which was a defect independent of consumers: the query syntax is part of the query contract, not of the CLI's flag handling. The CLI keeps its own error wording, since the library cannot know it was reached from a flag. + +**The four rules a consumer would otherwise reimplement**, which is the exact failure the retro records: + +- **Generic frontmatter access** — `Entry.Field` / `Entry.FieldList` mirroring `parse.String`/`parse.Strings`, plus `Frontmatter()` for the whole map. Reading a field no longer means round-tripping through JSON. +- **Link resolution** — `Index.ResolveLink` (target → bundle path, reporting out-of-bundle) and `RelativeLink` (the inverse). This is the rule that had three homes. +- **Frontmatter writes** — `SetField`, `SetFields`, `UnsetField`. The edit is surgical: it replaces exactly the lines belonging to a key and leaves every other byte alone, never parsing to a struct and re-serializing, which would silently drop what the YAML subset does not model. This is the rule that had two. +- **Checkbox toggling** — `SetCheckbox`, keyed by line because a checkbox's text may repeat within an entry. The format's inline task mechanism had no write primitive at all. + +Writes refresh the in-memory entry, so a caller holding an index never reads back what it just overwrote. + +A `BacklinkMap` was added alongside and then removed. It was justified as "what a consumer rendering every entry needs", and measuring showed that consumer does not exist: a reader renders *one* entry per request, which `Backlinks` answers in ~0.4ms on a 5k-entry bundle. The whole-graph workload it served had no caller, and the function was a seven-line inversion of data consumers already hold, not a rule they could get wrong. Precomputing it in `Build` was considered and rejected on the same evidence: it would charge every consumer ~40% of the index's footprint to save 0.4ms on a path nobody is blocked by. Recorded because the API surface is easier to add to than to take back. + +**Writes became atomic** as part of this, since a library consumer holds the index while other processes read the same files. See the CHANGELOG for the behaviour that changed with it (symlinks, hardlinks, read-only directories). + +**Deliberately deferred:** incremental rebuild, which a long-lived consumer will want and which belongs with [.wiki cache](../3-graph-and-mutation/004-incremental-cache.md). `Build` is still whole-bundle. + +**Stability:** pre-1.0, so the surface may break; the module version is the only promise. Stated here rather than discovered through a breaking change. + +**Acceptance, met:** an external Go module imports `github.com/agentic-wiki/wiki/index`, builds a bundle index, and answers the same questions the CLI does; the CLI runs on the same packages; the test suite passes. Verified as no-behaviour-change by diffing the pre-move binary against the current one — 759 read captures (7 bundles × 23 commands × 4 formats, plus error paths and every `-h`) and 59 mutating cases compared by entire resulting file tree, on both clean and deliberately dirty fixtures. diff --git a/backlog/5-serve/001-absorb-server.md b/backlog/5-serve/001-absorb-server.md new file mode 100644 index 0000000..2b635c0 --- /dev/null +++ b/backlog/5-serve/001-absorb-server.md @@ -0,0 +1,17 @@ +--- +type: task +title: "wiki serve: absorb the bundle server into the CLI" +status: cancelled +priority: high +tags: [feature, architecture, ui] +--- + +**Cancelled (2026-08-08): the UI lives in its own repo, `wikiview`.** The design work moved with it; only the [retro](./005-lessons.md) stays here, because the reverted attempt happened in this repo. + +The proposal was to bring the server in as `wiki serve`: one binary, one name, using `internal/{bundle,index,parse}` directly so the shell-out and its two stand-ins disappeared. + +**The argument against it, which decided the call:** absorbing the UI changes what `wiki` is. It is a zero-dependency static binary and a neutral engine, and serving means `net/http` plus a file watcher, roughly tripling the binary and making "zero external dependencies" false. Keeping the engine lean and the UI separate preserves the layering the spec states outright: the format stands alone, the tool is a neutral engine over it, and presentation is somebody else's job. + +**What this repo owes the decision instead**, and it is the harder half: a separate module cannot reach `internal/`, so the engine needs a real public API ([005](../4-release-and-docs/005-public-packages.md)). That is now a prerequisite rather than a third-party nicety. + +The trap to keep in view is the one the retro names. A module boundary is exactly what excused two frontmatter writers and three link resolvers last time: each was written where it was needed because the right home was unreachable. Re-establishing that boundary means the public API has to be **good enough that no consumer needs to reimplement a rule** — that is the whole test of 005, and the reason it must be designed for consumers rather than extracted to unblock a port. diff --git a/backlog/5-serve/005-lessons.md b/backlog/5-serve/005-lessons.md new file mode 100644 index 0000000..92b8aab --- /dev/null +++ b/backlog/5-serve/005-lessons.md @@ -0,0 +1,23 @@ +--- +type: task +title: "what the first attempt got wrong" +status: done +priority: medium +tags: [design, retro] +--- + +**Done (reverted, nothing shipped).** A working board UI was ported into this repo over an afternoon: ~3,800 lines of Go and ~3,400 of TypeScript. It ran, and every test passed. It was reverted anyway, and this records why so the second attempt does not repeat it. + +**Three implementations of one rule.** Resolving a link target to a bundle path existed in `index.normalizeLink`, again in the server's board model, and a third time in the browser. Each was written where it was needed rather than where it belonged. + +**Two frontmatter writers inside one binary.** The server carried its own surgical writer, built when it was a separate program and `setFrontmatterValue` was unreachable. Absorbing it removed the module boundary that had excused the duplication, and nothing replaced it, so the same rule sat twice in one module. + +**Two config files with two parsers.** `wiki-serve.toml` beside `wiki.toml`, each with its own reader, both describing the same directory. + +**Public API added to unblock a port.** `Entry.Field`, `FieldString`, `Frontmatter`, and `ParsePropFilter` were exported because the migration needed them that afternoon. [005](../4-release-and-docs/005-public-packages.md) had already written down "decide deliberately what a consumer may reach, rather than exporting fields because the move made it easy", and that is precisely what happened anyway. + +**Documentation left false rather than merely stale.** The README still said "zero external dependencies" and "no server in the way" while both had stopped being true. + +**Tests deleted rather than migrated.** Two README drift guards were dropped because their target moved, trading a real check for a green suite. + +The common thread: every one of these is a *migration* artifact. None would have been written by someone building the thing from its intended shape. Porting preserved decisions that were correct for a board over one folder and wrong for a reader over a bundle, and the compounding cost of those decisions was invisible while the tests were green. diff --git a/backlog/conformance/010-tool-config-namespace.md b/backlog/conformance/010-tool-config-namespace.md new file mode 100644 index 0000000..010ba5c --- /dev/null +++ b/backlog/conformance/010-tool-config-namespace.md @@ -0,0 +1,48 @@ +--- +type: task +title: "reserve a [tool.*] namespace in wiki.toml for other tools" +status: done +priority: medium +tags: [conformance, config, ecosystem] +--- + +`wiki check` warns on any `wiki.toml` key it does not recognize, which was the right call: a typo or a renamed field used to fail silently ([conformance/007](./007-unknown-config-keys.md)). But it also means **no other tool can put its configuration where a bundle's configuration lives.** + +So `wikanban` carries a separate `wikanban.toml` beside the bundle, purely to avoid making every user's `wiki check` noisy. A second tool would need a third file. The bundle ends up surrounded by satellite configs describing the same directory. + +**Proposal:** reserve `[tool.]` tables. `wiki` ignores them entirely: never parsed, never validated, never warned about. Everything outside that namespace keeps warning exactly as it does now. + +```toml +spec = "0.1" +types = ["task", "note"] + +[tool.wikanban] +group_by = "status" +lane_by = "assignee" +``` + +This is the `pyproject.toml` `[tool.*]` convention, and it exists for the same reason: one file describes the project, and tools namespace their own opinions inside it rather than each adding a dotfile. + +**Why it fits the separation principle rather than violating it.** The concern would be format-layer creep, the spec's own warning that "the format stands alone, the tool is a neutral engine over it, and the skill is where opinion lives". But a reserved, ignored namespace adds no opinion to the format: `wiki` gains no field it interprets, no behaviour, and no validation. It grants *space*, which is the opposite of taking a position on what belongs there. + +**To decide:** + +- **`[tool.x]` or a flat `tool.x.y` prefix?** The table form reads better and matches the precedent. +- **Does `check` verify anything at all inside it?** Recommendation: no. The moment it validates one tool's keys it has an opinion about that tool. +- **Does `bundle.Bundle` expose the raw tables** to a Go consumer (see [public packages](../4-release-and-docs/005-public-packages.md)), or does each tool re-read the file? Exposing them is the point; otherwise every tool grows a second `wiki.toml` parser, which is the drift this repo keeps refusing. +- **Migration:** none needed. Nothing uses the namespace yet, and separate files keep working for anyone who prefers them. + +**Acceptance:** `[tool.anything]` in `wiki.toml` produces no `check` warning; keys outside it still do; the tables are readable by a Go consumer without parsing the file again. + +## Done + +All three decisions above went the recommended way: `[tool.x]` tables, no validation inside them, and `bundle.Bundle.Tool` exposing them (as `toml.Primitive`, with `DecodeTool` unmarshalling one into a caller's own struct — the deferred decode is what lets `wiki` carry the tables without ever interpreting them). + +**It required parsing `wiki.toml` as TOML.** The config was read by a hand-rolled line scanner: split on newlines, cut on `=`, switch on the key name. That cannot implement this task at all, because it has no notion of a table — a header line has no `=` and was skipped, so keys inside one were indistinguishable from top-level keys. Reaching for the namespace surfaced two bugs that were already live: + +- **A key in any table silently overrode bundle config.** `[tool.x] types = [...]` replaced the bundle's vocabulary, and entries with undeclared types then passed `check` clean. The namespace this task proposes would have made that reachable by design rather than by accident. +- **A multi-line array silently disabled its setting.** `types = [` parsed to an empty list, read as "no vocabulary declared", allowing every type. Valid TOML that any other tool reads correctly. + +So `bundle` now depends on `BurntSushi/toml` (one dependency, no transitive ones). **`wiki` is no longer zero-dependency**, and the README, the retired half of [the hand-rolled parser debt](../debt/001-yaml-frontmatter-subset.md), and wikiview's design notes were corrected rather than left claiming otherwise. The frontmatter parser stays hand-rolled: it is deliberately a subset, and the surgical write API depends on that, where `wiki.toml` is a config file whose users reasonably expect all of TOML. + +A malformed config is now an error rather than a shrug, which is a behaviour change worth knowing about: input that was never valid TOML but happened to scan (unquoted array items, most likely) now fails with a line number. diff --git a/backlog/debt/001-yaml-frontmatter-subset.md b/backlog/debt/001-yaml-frontmatter-subset.md index c75c464..37a00f4 100644 --- a/backlog/debt/001-yaml-frontmatter-subset.md +++ b/backlog/debt/001-yaml-frontmatter-subset.md @@ -6,4 +6,8 @@ priority: low tags: [debt, parser] --- -`internal/parse.Frontmatter` parses only our scalar + string-list subset; `wiki.toml`'s `types` array must be one line. Chosen for zero dependencies and a network-free, single-static-binary build. Swap to `gopkg.in/yaml.v3` only if frontmatter grows nested structures — the parsers are isolated behind `parse.Frontmatter` and `bundle.parseConfig`. +`parse.Frontmatter` parses only our scalar + string-list subset. Chosen for zero dependencies and a network-free, single-static-binary build. Swap to `gopkg.in/yaml.v3` only if frontmatter grows nested structures — the parser is isolated behind `parse.Frontmatter`. + +**The `wiki.toml` half of this is gone.** The config was read by the same hand-rolled approach and it went wrong in two ways that were invisible: a multi-line array parsed to an empty list (so a declared `types` vocabulary silently allowed everything), and a key inside any table was treated as top-level (so `[tool.x] types = [...]` silently replaced the bundle's vocabulary). `bundle` now uses `BurntSushi/toml`. + +That weakens the zero-dependency argument for keeping the frontmatter parser hand-rolled, but does not remove it. The two cases are not alike: `wiki.toml` is a config file where users legitimately expect the whole of TOML, while frontmatter is deliberately a **subset**, and the surgical write API depends on that — it edits the lines belonging to a key and leaves the rest byte-for-byte, which a parse-and-reserialize round trip through a full YAML library would destroy. See [the write API](../4-release-and-docs/005-public-packages.md). diff --git a/backlog/debt/006-replace-while-open.md b/backlog/debt/006-replace-while-open.md new file mode 100644 index 0000000..1f0c876 --- /dev/null +++ b/backlog/debt/006-replace-while-open.md @@ -0,0 +1,25 @@ +--- +type: task +title: "replacing an open file: a retry, not proper semantics" +status: todo +priority: medium +tags: [debt, platform, mutation] +--- + +`writeFile` replaces an entry with a temp file and a rename. On Unix that is atomic and works regardless of who has the file open. On Windows it is neither. + +Replacing a file there means deleting the old one, and Go opens files for reading without `FILE_SHARE_DELETE`, so `os.Rename` over a file another handle has open fails with `ERROR_ACCESS_DENIED` or `ERROR_SHARING_VIOLATION`. The plain `os.WriteFile` this replaced needed only write sharing and succeeded, so atomic writes introduced a failure mode on Windows that did not exist before. + +**What shipped is a mitigation:** `rename` retries for ~110ms, but *only* while the error is one of the contention codes (`index/rename_windows.go`; the predicate is `false` on every other platform, so there is no retry and no cost off Windows). That covers the common cause, which is transient and not the user's doing — an antivirus scanner or the search indexer opening a file it just saw change. It does not cover a handle held open for longer, where the write simply fails. + +**The proper fix is POSIX rename semantics.** `SetFileInformationByHandle` with `FILE_RENAME_INFO_EX` and `FILE_RENAME_FLAG_POSIX_SEMANTICS` replaces a file even with open handles, exactly as Unix does; the existing handles keep referring to the now-unlinked file. Go's own standard library already uses the equivalent `FILE_DISPOSITION_POSIX_SEMANTICS` for *delete* (`os.Remove`), so the pattern is established — it just is not applied to rename, which still goes through `MoveFileEx(MOVEFILE_REPLACE_EXISTING)` as of Go 1.26. + +**Why it was not done now:** + +- It needs `golang.org/x/sys/windows`, a second dependency, on a tool that took its first one this release and only to fix silent config bugs. +- It needs a fallback path: `FILE_RENAME_INFO_EX` is Windows 10 1709+, and filesystems without POSIX semantics (FAT32) reject it, so the `MoveFileEx` route has to stay anyway. +- It cannot be verified where this was written. Shipping untested platform-specific syscall code in a release is a worse trade than a retry whose failure mode is "returns the error it would have returned". + +**Also unverified:** the concurrent-reader atomicity test (`TestCommandRewritesAreAtomic`) is skipped on Windows, because a reader looping that tightly holds the target permanently open and the test would measure contention rather than tearing. The property it checks is Unix-shaped; the Windows risk is the write failing, not a torn read. + +**Acceptance:** a rename over an entry with an open read handle succeeds on Windows 10 1709+ without retrying; older Windows and non-POSIX filesystems fall back to the current path; the atomicity test runs on Windows rather than skipping; verified on a real Windows runner, not cross-compiled. diff --git a/backlog/index.md b/backlog/index.md index 2a3f4ba..bbd5ca2 100644 --- a/backlog/index.md +++ b/backlog/index.md @@ -8,17 +8,22 @@ Backlog for the `wiki` CLI itself, kept in the format `wiki` implements (dogfood ## 2 — Query surface - [ ] [list --limit and --skip](./2-query-surface/015-list-limit-skip.md) +- [ ] [list --body: bodies in the same json call](./2-query-surface/016-list-body.md) (low) +- [x] [--where on the vocabulary commands](./2-query-surface/017-where-on-vocabulary-commands.md): `tags`, `properties`, `property`, and `checkboxes` take `--where` with `list`'s semantics, composing with `--prefix` — **two filters, `--prefix` for where and `--where` for what**, available wherever a set of entries is narrowed. `checkboxes` also dropped a duplicate prefix matcher and now routes through `Index.Filter`. - [ ] [status: count of ignored files](./2-query-surface/011-status-ignored-count.md) - [ ] [search --fuzzy: opt-in typo-tolerant matching](./2-query-surface/013-fuzzy-search.md) (low) - [ ] [wiki table --links raw|text|path](./2-query-surface/014-table-link-transform.md) ## 3 — Graph & mutation +- [ ] [wiki set: the missing write primitive](./3-graph-and-mutation/016-set-frontmatter.md) - [ ] [.wiki cache](./3-graph-and-mutation/004-incremental-cache.md) - [ ] [spec upgrade / cross-version migration](./3-graph-and-mutation/008-spec-upgrade.md) - [ ] [wiki move on directories](./3-graph-and-mutation/013-move-directories.md) - [ ] [backlinks/links per-page granularity](./3-graph-and-mutation/014-backlinks-granularity.md) ## 4 — Release & docs +- [x] [promote the core packages to an importable API](./4-release-and-docs/005-public-packages.md): `bundle`, `index`, `parse` at the module root (`output` and `wikilink` stayed internal), plus the four rules a consumer would otherwise reimplement — frontmatter reads (`Field`/`FieldList`/`Frontmatter`), link resolution (`ResolveLink`/`RelativeLink`), surgical frontmatter writes (`SetField`/`SetFields`/`UnsetField`), and checkbox toggling (`SetCheckbox`, which did not exist at all). `--where` parsing moved into `index.ParseFilter`. Writes refresh the in-memory entry and are now **atomic** (temp + rename). Verified no-behaviour-change by diffing the pre-move binary: 759 read captures and 59 mutating cases compared by entire file tree. Incremental rebuild deferred to [.wiki cache](./3-graph-and-mutation/004-incremental-cache.md). +- [x] [reserve a per-tool config namespace in wiki.toml](./conformance/010-tool-config-namespace.md): `[tool.]` tables are never parsed, validated, or warned about; `bundle.DecodeTool` unmarshals one into a consumer's own struct so no tool writes a second `wiki.toml` parser. Required switching to a real TOML parser (`BurntSushi/toml`), which surfaced two live bugs: a key inside any table silently overrode bundle config, and a multi-line array silently emptied its setting. **`wiki` is no longer zero-dependency.** - [ ] [reframe stack: format + tool + workflow](./4-release-and-docs/003-stack-framing.md) - [ ] [align skills repo with AGENTS.md + workflow model](./4-release-and-docs/004-skills-sync.md) @@ -26,8 +31,14 @@ Backlog for the `wiki` CLI itself, kept in the format `wiki` implements (dogfood ## Debt - [ ] [table parser: rare `|` edge cases](./debt/002-table-pipe-edge-cases.md) - [ ] [move: no rollback on a partial write](./debt/004-move-no-rollback.md) +- [ ] [replacing an open file: a retry, not proper semantics](./debt/006-replace-while-open.md) + +## 5 — Serve +- [x] [absorb the server into the CLI](./5-serve/001-absorb-server.md): **cancelled**. The UI lives in its own repo (`wikiview`); keeping `wiki` a zero-dependency neutral engine won. The consequence is that [005](./4-release-and-docs/005-public-packages.md) becomes a prerequisite rather than a nicety, and the bar it has to clear is that no consumer should ever need to reimplement a rule. +- [x] [what the first attempt got wrong](./5-serve/005-lessons.md): a working board UI was ported in and reverted. Three link resolvers, two frontmatter writers in one module, two config files with two parsers, a false README, deleted tests, and public API exported to unblock the port. Every one a migration artifact. ## Done +- [x] [move --include-frontmatter: find relative frontmatter refs](./3-graph-and-mutation/015-frontmatter-relative-refs.md): frontmatter refs are matched by **resolved target** (so a relative `blockers: [./task-1.md]` is found, not just an exact root-absolute string) and **normalized to root-absolute** on write; the moved file's own relative refs are normalized too, closing the cross-folder dangle body links already handled. Frontmatter deliberately does *not* follow bodies to relative: a root-absolute value is a **stable key** (`--where epic=/epics/x.md` finds every referrer, where relative spellings differ per directory and no single query matches them all), and the rendering argument behind 012 does not apply since frontmatter is never rendered as a link. **Body links are relative because they must navigate; frontmatter refs are root-absolute because they must be stable keys.** Anchors preserved, out-of-bundle values untouched, `.md` suffix gates the pass. Resolve-on-`--where` was considered and rejected (it would put a path heuristic in the hottest query path and break match-what-you-see). - [x] [check anchor links point at a real heading](./conformance/009-anchor-link-check.md): `wiki check` warns on a link to an existing file whose `#anchor` matches no heading there (`link anchor not found -> …`), matched by GitHub-style slug on both sides (`headingSlug` + `Entry.hasHeadingSlug`, duplicate headings disambiguated `-1`/`-2`). Covers markdown, `[[wikilink#…]]`, and same-page `#anchor` self-links. Warning severity, exit `0`; broken targets not double-reported. Refactor: links now carry a parsed `Anchor` field (retired `anchorOf(Raw)`; `Move`/check use it), and pure `#anchor` links are captured off-graph in `SelfAnchors`. Only Obsidian block refs `#^id` and text-vs-rendered-markup slugging remain out of scope. Unit + smoke tests; AGENTS/spec/CHANGELOG synced. - [x] datasets guidance + `org-wiki`→`org-base` rename: AGENTS.md teaches the entry-vs-dataset choice (uniform records you aggregate go in one `type: dataset` table queried via `wiki table | duckdb`/`jq`, not one file each; store cells raw/machine-readable so no `gsub` cleanup; a dataset is a typed file, never an `index.md`). `org-base` (renamed from `org-wiki`, since a base holds knowledge **and** datasets) gains a Records-and-datasets section with a raw invoices table, and its rollup note now splits entry-frontmatter rollups from tabular ones. Synced spec README's dataset definition and `org-base`'s suggested `types` (+`dataset`). Historical mentions of `org-wiki` in older Done entries left as-is. - [x] [relative links are the canonical on-disk form](./3-graph-and-mutation/012-switch-canonical-links.md): internal links are stored relative to the linking file (`../ref/api.md`), so they navigate in any renderer (GitHub, editors, Obsidian), not just bundle-root-aware tools. Internal graph unchanged (still keyed on the resolved root-absolute path), so `backlinks`/`orphans`/`search`/`links` are unaffected. `tidy --links` now normalizes absolute→relative; `move` writes relative and also respells the **moved file's own** outgoing links from its new dir (previously they'd dangle on a cross-folder move); `tidy --wikilinks` emits relative too. A hand-written absolute link still resolves (never "broken"); frontmatter path refs stay absolute (metadata, not rendered links). New `relativeLink` helper (inverse of `normalizeLink`); AGENTS/spec/scaffold docs + tests + smoke swept. diff --git a/bundle/bundle.go b/bundle/bundle.go new file mode 100644 index 0000000..5c6b551 --- /dev/null +++ b/bundle/bundle.go @@ -0,0 +1,156 @@ +// Package bundle locates an agentic-wiki bundle and reads its config. +// +// A bundle is a directory containing wiki.toml; the markdown content lives +// directly in that directory, with .wiki/ as a hidden, disposable cache. +package bundle + +import ( + "errors" + "fmt" + "maps" + "os" + "path/filepath" + "slices" + "strings" + + "github.com/BurntSushi/toml" +) + +// Bundle is a located agentic-wiki bundle: a directory containing wiki.toml, +// with the markdown content living directly inside it. +type Bundle struct { + Dir string // the bundle directory (holds wiki.toml); also the content root + Spec string // spec version the bundle conforms to (from wiki.toml) + Types []string // content types declared in wiki.toml + Ignore []string // paths (relative to Dir) wiki disregards: an in-bundle path is not indexed (not an entry); an out-of-bundle path silences that link's advisory + // IgnoreOrphans lists paths (relative to Dir) whose entries stay indexed but are + // not reported by `wiki orphans`: a directory subtree or an exact path. + IgnoreOrphans []string + // Unknown holds wiki.toml keys the tool does not recognize (a typo, or a + // renamed field), as dotted paths. They are inert; `check` surfaces them so + // they aren't silently ignored. + Unknown []string + + // The [tool.] tables, left undecoded. Unexported deliberately: making + // them public would put toml.Primitive in this package's signature and force + // every consumer to import the TOML library to name the type, turning an + // implementation choice into part of the contract. DecodeTool is the way in. + tool map[string]toml.Primitive + md toml.MetaData +} + +// Tools lists the names of the [tool.] tables present, sorted. +func (b *Bundle) Tools() []string { + return slices.Sorted(maps.Keys(b.tool)) +} + +// DecodeTool unmarshals the [tool.] table into v, which is any struct or +// map the caller defines. Reports whether the table was present. +// +// The namespace is space granted to other tools over the same bundle, so one +// file describes the directory instead of a satellite config per tool. wiki +// never reads inside it: no validation, no warning, no opinion. +// +// The decode happens here rather than in the consumer because the alternative is +// every tool parsing wiki.toml again: the same rule with two homes, which is the +// drift this repo keeps refusing. +func (b *Bundle) DecodeTool(name string, v any) (bool, error) { + p, ok := b.tool[name] + if !ok { + return false, nil + } + if err := b.md.PrimitiveDecode(p, v); err != nil { + return true, fmt.Errorf("wiki.toml [tool.%s]: %w", name, err) + } + return true, nil +} + +// ErrNotFound is returned when no wiki.toml is found walking up from start. +var ErrNotFound = errors.New("no wiki.toml found (not inside a wiki bundle)") + +// Discover walks up from start until it finds a directory containing wiki.toml. +func Discover(start string) (*Bundle, error) { + dir, err := filepath.Abs(start) + if err != nil { + return nil, err + } + for { + cfg := filepath.Join(dir, "wiki.toml") + if fi, err := os.Stat(cfg); err == nil && !fi.IsDir() { + return load(dir, cfg) + } + parent := filepath.Dir(dir) + if parent == dir { + return nil, ErrNotFound + } + dir = parent + } +} + +// config is the whole of wiki.toml the tool interprets. Anything else is either +// a [tool.*] table, which is deliberately opaque, or an unrecognized key. +type config struct { + Spec string `toml:"spec"` + Types []string `toml:"types"` + Ignore []string `toml:"ignore"` + IgnoreOrphans []string `toml:"ignore_orphans"` + Tool map[string]toml.Primitive `toml:"tool"` +} + +func load(root, cfg string) (*Bundle, error) { + var c config + // A malformed wiki.toml is an error rather than a shrug. The config decides + // what is an entry and which types are valid, so reading half of it and + // carrying on produces confidently wrong answers. + md, err := toml.DecodeFile(cfg, &c) + if err != nil { + return nil, fmt.Errorf("wiki.toml: %w", err) + } + // Undecoded keys are the ones no field claimed. The [tool.*] subtree is + // skipped rather than reported: reserving the namespace means wiki never has + // an opinion about what is inside it. (Its keys show as undecoded because + // toml.Primitive defers decoding, so the filter is explicit.) + var unknown []string + for _, k := range md.Undecoded() { + if len(k) > 0 && k[0] == "tool" { + continue + } + // Report the shallowest unrecognized key only: once [nested] is flagged, + // listing every key inside it adds noise, not information. + s := k.String() + if slices.ContainsFunc(unknown, func(u string) bool { return strings.HasPrefix(s, u+".") }) { + continue + } + unknown = append(unknown, s) + } + return &Bundle{ + Dir: root, + Spec: c.Spec, + Types: c.Types, + Ignore: c.Ignore, + IgnoreOrphans: c.IgnoreOrphans, + tool: c.Tool, + Unknown: unknown, + md: md, + }, nil +} + +// KnownType reports whether t is an allowed content type. A declared vocabulary +// (`types` in wiki.toml) is opt-in: when none is declared (empty list), every +// type is allowed and this returns true; when one is declared, t must be in it. +func (b *Bundle) KnownType(t string) bool { + return len(b.Types) == 0 || slices.Contains(b.Types, t) +} + +// okfVersions maps an agentic-wiki spec version to the OKF version it embeds. +// Our spec is its own thing; OKF is one ingredient, declared to OKF consumers +// via okf_version in the bundle-root index.md. A future spec may embed a +// different OKF version, or none. +var okfVersionMap = map[string]string{"0.1": "0.1"} + +// OKFVersion returns the OKF version this bundle's spec embeds, and whether the +// spec embeds OKF at all (false for an unknown or non-OKF spec). +func (b *Bundle) OKFVersion() (string, bool) { + v, ok := okfVersionMap[b.Spec] + return v, ok +} diff --git a/bundle/bundle_test.go b/bundle/bundle_test.go new file mode 100644 index 0000000..b98673a --- /dev/null +++ b/bundle/bundle_test.go @@ -0,0 +1,265 @@ +package bundle + +import ( + "os" + "path/filepath" + "reflect" + "slices" + "testing" +) + +func TestOKFVersion(t *testing.T) { + if v, ok := (&Bundle{Spec: "0.1"}).OKFVersion(); !ok || v != "0.1" { + t.Errorf("OKFVersion(0.1) = %q, %v; want 0.1, true", v, ok) + } + if _, ok := (&Bundle{Spec: "9.9"}).OKFVersion(); ok { + t.Errorf("unknown spec should not embed OKF") + } +} + +// loadConfig writes a wiki.toml and loads it, returning the bundle or the error. +func loadConfig(t *testing.T, toml string) (*Bundle, error) { + t.Helper() + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "wiki.toml"), []byte(toml), 0o644); err != nil { + t.Fatal(err) + } + return Discover(dir) +} + +func mustLoad(t *testing.T, toml string) *Bundle { + t.Helper() + b, err := loadConfig(t, toml) + if err != nil { + t.Fatal(err) + } + return b +} + +func TestLoadConfig(t *testing.T) { + b := mustLoad(t, ` +spec = "0.1" +types = ["note", "concept"] # a trailing comment +ignore = ["AGENTS.md", "../PRD.md"] +ignore_orphans = ["backlog/**"] +`) + if b.Spec != "0.1" { + t.Errorf("spec=%q", b.Spec) + } + if !reflect.DeepEqual(b.Types, []string{"note", "concept"}) { + t.Errorf("types=%#v", b.Types) + } + if !reflect.DeepEqual(b.Ignore, []string{"AGENTS.md", "../PRD.md"}) { + t.Errorf("ignore=%#v", b.Ignore) + } + if !reflect.DeepEqual(b.IgnoreOrphans, []string{"backlog/**"}) { + t.Errorf("ignore_orphans=%#v", b.IgnoreOrphans) + } + if b.Unknown != nil { + t.Errorf("a clean config should have no unknown keys, got %#v", b.Unknown) + } +} + +// A line-based reader saw `types = [` and produced an empty list, which reads as +// "no vocabulary declared" and so allowed every type — the opposite of what the +// author wrote, with nothing reported. +func TestMultilineArray(t *testing.T) { + b := mustLoad(t, ` +spec = "0.1" +types = [ + "task", + "note", +] +`) + if !reflect.DeepEqual(b.Types, []string{"task", "note"}) { + t.Errorf("types=%#v, want the two declared types", b.Types) + } + if b.KnownType("bogus") { + t.Error("a declared vocabulary must reject an undeclared type") + } +} + +// The namespace is space granted to other tools: never validated, never warned +// about, and never mistaken for bundle config. +func TestToolNamespaceIsOpaque(t *testing.T) { + b := mustLoad(t, ` +spec = "0.1" +types = ["task", "note"] + +[tool.wikiview] +default_board = "/backlog" + +[[tool.wikiview.board]] +path = "/backlog" +columns = ["todo", "done"] + +[tool.other] +types = ["this", "must", "not", "leak"] +`) + if len(b.Unknown) != 0 { + t.Errorf("[tool.*] must not warn, got %#v", b.Unknown) + } + // A line reader treated every key as top-level, so a tool's `types` silently + // replaced the bundle's vocabulary. + if !reflect.DeepEqual(b.Types, []string{"task", "note"}) { + t.Errorf("a tool table leaked into bundle config: types=%#v", b.Types) + } + if got := b.Tools(); !reflect.DeepEqual(got, []string{"other", "wikiview"}) { + t.Errorf("Tools() = %v", got) + } +} + +func TestDecodeTool(t *testing.T) { + b := mustLoad(t, ` +spec = "0.1" + +[tool.wikiview] +default_board = "/backlog" + +[[tool.wikiview.board]] +path = "/backlog" +where = ["type=task"] +columns = ["todo", "done"] +`) + var cfg struct { + DefaultBoard string `toml:"default_board"` + Board []struct { + Path string `toml:"path"` + Where []string `toml:"where"` + Columns []string `toml:"columns"` + } `toml:"board"` + } + found, err := b.DecodeTool("wikiview", &cfg) + if err != nil || !found { + t.Fatalf("DecodeTool: found=%v err=%v", found, err) + } + if cfg.DefaultBoard != "/backlog" || len(cfg.Board) != 1 { + t.Fatalf("decoded %+v", cfg) + } + if got := cfg.Board[0]; got.Path != "/backlog" || + !reflect.DeepEqual(got.Where, []string{"type=task"}) || + !reflect.DeepEqual(got.Columns, []string{"todo", "done"}) { + t.Errorf("board = %+v", got) + } + // An absent table is not an error; it is a bundle that does not use the tool. + if found, err := b.DecodeTool("absent", &cfg); found || err != nil { + t.Errorf("absent tool: found=%v err=%v", found, err) + } +} + +func TestUnknownKeysReportFullPath(t *testing.T) { + // A renamed field (the old `skip`) or a typo is inert, so surface it rather + // than let the author assume it took effect. + b := mustLoad(t, ` +spec = "0.1" +skip = ["AGENTS.md"] +tpyes = ["note"] + +[nested] +key = "value" +`) + // The shallowest unrecognized key only: [nested] is flagged, its contents are + // implied. + if want := []string{"skip", "tpyes", "nested"}; !reflect.DeepEqual(b.Unknown, want) { + t.Errorf("unknown=%#v, want %#v", b.Unknown, want) + } + + // A key nested under a *recognized* table still reports its full path, since + // a bare "key" cannot be found in a file with several tables. + b = mustLoad(t, "spec = \"0.1\"\n\n[tool.x]\nfine = 1\n\n[nested]\na = 1\nb = 2\n") + if !slices.Contains(b.Unknown, "nested") || slices.Contains(b.Unknown, "nested.a") { + t.Errorf("unknown=%#v", b.Unknown) + } +} + +// Reading half a config and carrying on produces confidently wrong answers: the +// config decides what is an entry and which types are valid. +func TestMalformedConfigIsAnError(t *testing.T) { + for _, tc := range []struct{ name, toml string }{ + {"unterminated array", "spec = \"0.1\"\ntypes = [\"a\", \n"}, + {"bare token", "spec = \"0.1\"\ntypes = [note, concept]\n"}, + {"missing value", "spec =\n"}, + {"junk line", "spec = \"0.1\"\nthis is not toml\n"}, + } { + t.Run(tc.name, func(t *testing.T) { + if _, err := loadConfig(t, tc.toml); err == nil { + t.Error("malformed wiki.toml should not load silently") + } + }) + } +} + +func TestKnownType(t *testing.T) { + b := &Bundle{Types: []string{"note", "concept"}} + for _, ty := range []string{"note", "concept"} { + if !b.KnownType(ty) { + t.Errorf("%q should be known", ty) + } + } + // With a declared vocabulary, an undeclared type is unknown. + for _, ty := range []string{"index", "log", "bogus"} { + if b.KnownType(ty) { + t.Errorf("%q is not a declared content type", ty) + } + } + // No declared vocabulary (opt-in): every type is allowed. + none := &Bundle{} + for _, ty := range []string{"note", "anything", "made-up"} { + if !none.KnownType(ty) { + t.Errorf("%q should be allowed when no vocabulary is declared", ty) + } + } +} + +func TestDiscoverWalksUp(t *testing.T) { + root := t.TempDir() + writeTOML(t, root) + deep := filepath.Join(root, "a", "b") // content lives at the bundle root, no wiki/ subfolder + if err := os.MkdirAll(deep, 0o755); err != nil { + t.Fatal(err) + } + p := mustDiscover(t, deep) + if realpath(t, p.Dir) != realpath(t, root) { + t.Errorf("Dir=%q want %q", p.Dir, root) + } + if p.Spec != "0.1" { + t.Errorf("spec=%q", p.Spec) + } +} + +func TestDiscoverExactDir(t *testing.T) { + root := t.TempDir() + writeTOML(t, root) + p := mustDiscover(t, root) // wiki.toml is right here, no walking + if realpath(t, p.Dir) != realpath(t, root) { + t.Errorf("Dir=%q want %q", p.Dir, root) + } +} + +func TestDiscoverNotFound(t *testing.T) { + if _, err := Discover(t.TempDir()); err != ErrNotFound { + t.Errorf("err=%v want ErrNotFound", err) + } +} + +func writeTOML(t *testing.T, dir string) { + t.Helper() + if err := os.WriteFile(filepath.Join(dir, "wiki.toml"), []byte("spec=\"0.1\"\ntypes=[\"note\"]\n"), 0o644); err != nil { + t.Fatal(err) + } +} + +func mustDiscover(t *testing.T, dir string) *Bundle { + t.Helper() + p, err := Discover(dir) + if err != nil { + t.Fatal(err) + } + return p +} + +func realpath(t *testing.T, p string) string { + t.Helper() + r, _ := filepath.EvalSymlinks(p) + return r +} diff --git a/cmd/wiki/commands.go b/cmd/wiki/commands.go index ecfb0bb..fc4499f 100644 --- a/cmd/wiki/commands.go +++ b/cmd/wiki/commands.go @@ -9,10 +9,10 @@ import ( "strings" "time" - "github.com/agentic-wiki/wiki/internal/index" + "github.com/agentic-wiki/wiki/index" "github.com/agentic-wiki/wiki/internal/output" - "github.com/agentic-wiki/wiki/internal/parse" "github.com/agentic-wiki/wiki/internal/scaffold" + "github.com/agentic-wiki/wiki/parse" ) func cmdInit(args []string) int { @@ -72,7 +72,7 @@ func cmdStatus(args []string) int { Orphans int `json:"orphans"` }{ idx.Bundle.Dir, idx.Bundle.Spec, - len(idx.Entries), links, len(idx.TagCounts("")), checkboxes, + len(idx.Entries), links, len(idx.TagCounts("", nil)), checkboxes, len(idx.Broken()), len(idx.Orphans()), } lines := []string{ @@ -98,21 +98,13 @@ type whereFilters []index.PropFilter func (w *whereFilters) String() string { return "" } func (w *whereFilters) Set(s string) error { - // `!=` (inequality) takes precedence over `=`, so it is matched first. - neg := false - kRaw, vRaw, ok := strings.Cut(s, "!=") - if ok { - neg = true - } else { - kRaw, vRaw, ok = strings.Cut(s, "=") - } - k := parse.Unquote(kRaw) - if !ok || k == "" { + f, err := index.ParseFilter(s) + if err != nil { + // The CLI keeps its own wording, which names the flag the user typed; + // the library cannot, since it does not know it was reached from a flag. return fmt.Errorf("--where must be key=value or key!=value, got %q", s) } - // Unquote the value the same way frontmatter is parsed, so a quote that - // survives the shell (--where 'k="v"') still compares equal to `k: "v"`. - *w = append(*w, index.PropFilter{Key: k, Value: parse.Unquote(vRaw), Negate: neg}) + *w = append(*w, f) return nil } @@ -207,39 +199,44 @@ func emitCounts(format string, rows []countRow, withCounts bool) int { } // countFlags registers the flags shared by tags/properties/property. -func countFlags(fs *flag.FlagSet, unit string) (format, sortBy, prefix *string, counts *bool) { +func countFlags(fs *flag.FlagSet, unit string) (format, sortBy, prefix *string, counts *bool, where *whereFilters) { format = fs.String("format", "text", "output format: text|json|csv|tsv") counts = fs.Bool("counts", false, "show entry count per "+unit) sortBy = fs.String("sort", "name", "sort order: name|count") prefix = fs.String("prefix", "", "filter to a path prefix") + // The vocabulary commands report what a set of entries uses, so they narrow + // that set the same two ways everything else does: --prefix for where, + // --where for what. + where = &whereFilters{} + fs.Var(where, "where", "filter entries by frontmatter: key=value or key!=value (repeatable)") return } func cmdTags(args []string) int { fs := flag.NewFlagSet("tags", flag.ExitOnError) - format, sortBy, prefix, counts := countFlags(fs, "tag") + format, sortBy, prefix, counts, where := countFlags(fs, "tag") fs.Parse(args) idx, code := loadIndex() if code != 0 { return code } - return emitCounts(*format, sortedCounts(idx.TagCounts(*prefix), *sortBy), *counts) + return emitCounts(*format, sortedCounts(idx.TagCounts(*prefix, *where), *sortBy), *counts) } func cmdProperties(args []string) int { fs := flag.NewFlagSet("properties", flag.ExitOnError) - format, sortBy, prefix, counts := countFlags(fs, "property") + format, sortBy, prefix, counts, where := countFlags(fs, "property") fs.Parse(args) idx, code := loadIndex() if code != 0 { return code } - return emitCounts(*format, sortedCounts(idx.PropertyKeyCounts(*prefix), *sortBy), *counts) + return emitCounts(*format, sortedCounts(idx.PropertyKeyCounts(*prefix, *where), *sortBy), *counts) } func cmdProperty(args []string) int { fs := flag.NewFlagSet("property", flag.ExitOnError) - format, sortBy, prefix, counts := countFlags(fs, "value") + format, sortBy, prefix, counts, where := countFlags(fs, "value") name, ok := parseWithArg(fs, args) if !ok { fmt.Fprintln(os.Stderr, "usage: wiki property [--counts --sort=name|count --prefix]") @@ -249,7 +246,7 @@ func cmdProperty(args []string) int { if code != 0 { return code } - return emitCounts(*format, sortedCounts(idx.PropertyValueCounts(name, *prefix), *sortBy), *counts) + return emitCounts(*format, sortedCounts(idx.PropertyValueCounts(name, *prefix, *where), *sortBy), *counts) } func cmdCheckboxes(args []string) int { @@ -258,6 +255,8 @@ func cmdCheckboxes(args []string) int { all := fs.Bool("all", false, "include done tasks") done := fs.Bool("done", false, "only done tasks") prefix := fs.String("prefix", "", "filter to a path prefix") + where := &whereFilters{} + fs.Var(where, "where", "filter entries by frontmatter: key=value or key!=value (repeatable)") fs.Parse(args) // Optional [file]: scope to a single entry's own checklist (its subtasks), // with flags allowed on either side of the positional (like read/outline). @@ -266,7 +265,7 @@ func cmdCheckboxes(args []string) int { target = fs.Arg(0) fs.Parse(fs.Args()[1:]) if fs.NArg() != 0 { - fmt.Fprintln(os.Stderr, "usage: wiki checkboxes [file] [--all --done --prefix]") + fmt.Fprintln(os.Stderr, "usage: wiki checkboxes [file] [--all --done --prefix --where key=value]") return 2 } } @@ -275,7 +274,8 @@ func cmdCheckboxes(args []string) int { return code } - entries := idx.Entries + // A named [file] is explicit, so the filters do not also apply to it. + entries := idx.Filter(*prefix, *where) if target != "" { e, err := idx.Resolve(target) if err != nil { @@ -285,18 +285,18 @@ func cmdCheckboxes(args []string) int { entries = []*index.Entry{e} } + // `entry`, matching check's rows: both describe an entry rather than merging + // its frontmatter, so neither needs the reserved `_path`, and they should not + // disagree on what to call the same thing. type row struct { - File string `json:"file"` - Line int `json:"line"` - Done bool `json:"done"` - Text string `json:"text"` + Entry string `json:"entry"` + Line int `json:"line"` + Done bool `json:"done"` + Text string `json:"text"` } var rows []row var lines []string for _, e := range entries { - if target == "" && *prefix != "" && !strings.HasPrefix(strings.TrimPrefix(e.Path, "/"), strings.TrimPrefix(*prefix, "/")) { - continue - } for _, t := range e.Checkboxes { switch { case *done && !t.Done: @@ -699,7 +699,7 @@ func parseWith2Args(fs *flag.FlagSet, args []string) (string, string, bool) { func cmdMove(args []string) int { fs := flag.NewFlagSet("move", flag.ExitOnError) dryRun := fs.Bool("dry-run", false, "preview the move without writing") - includeFM := fs.Bool("include-frontmatter", false, "also rewrite frontmatter values equal to the moved path (opt-in)") + includeFM := fs.Bool("include-frontmatter", false, "also rewrite frontmatter *.md values referencing the moved path (opt-in)") format := fs.String("format", "text", "output format: text|json|csv|tsv") src, dest, ok := parseWith2Args(fs, args) if !ok { @@ -752,9 +752,20 @@ func cmdLinks(args []string) int { fmt.Fprintln(os.Stderr, "wiki:", err) return 2 } - refs := idx.OutLinks(e) + // Unique targets: this view answers "what does this page point to", and it + // prints a bare path, so the same target twice would read as a bug rather + // than as two mentions. The engine returns every occurrence; deciding to + // collapse them is presentation, which is this layer's job. + // (`backlinks` prints file:line, so it shows every occurrence.) + seen := map[string]bool{} + var refs []index.LinkRef var lines []string - for _, r := range refs { + for _, r := range idx.Links(e.Path) { + if seen[r.To] { + continue + } + seen[r.To] = true + refs = append(refs, r) lines = append(lines, r.To) } output.Emit(os.Stdout, *format, lines, refs) diff --git a/cmd/wiki/commands_test.go b/cmd/wiki/commands_test.go index df6bf9d..f877957 100644 --- a/cmd/wiki/commands_test.go +++ b/cmd/wiki/commands_test.go @@ -8,7 +8,7 @@ import ( "testing" "time" - "github.com/agentic-wiki/wiki/internal/index" + "github.com/agentic-wiki/wiki/index" "github.com/agentic-wiki/wiki/internal/scaffold" ) @@ -310,6 +310,7 @@ func TestCmdMoveIncludeFrontmatter(t *testing.T) { if code != 0 { t.Fatalf("move --include-frontmatter exit %d", code) } + // Frontmatter refs stay root-absolute (the stable-key form), unlike body links. if b, _ := os.ReadFile(filepath.Join(dir, "ref.md")); !strings.Contains(string(b), "parent: /moved.md") { t.Errorf("--include-frontmatter should rewrite the field: %s", b) } @@ -894,3 +895,76 @@ func TestCmdTable(t *testing.T) { t.Errorf("missing file should exit 2, got %d", code) } } + +// The message a bad --where produces is part of the CLI's surface, and it names +// the flag the user typed. Extracting the parser into index.ParseFilter silently +// changed it once; this pins it so that cannot recur. +func TestWhereFlagErrorMessage(t *testing.T) { + var w whereFilters + err := w.Set("garbage") + if err == nil { + t.Fatal("expected an error") + } + if want := `--where must be key=value or key!=value, got "garbage"`; err.Error() != want { + t.Errorf("got %q, want %q", err.Error(), want) + } +} + +// The vocabulary commands report what a set of entries uses, so they must narrow +// that set the same two ways list does. Before this they took --prefix only, and +// a folder mixing kinds reported every kind's vocabulary. +func TestVocabularyCommandsAcceptWhere(t *testing.T) { + dir := t.TempDir() + write := func(name, body string) { + p := filepath.Join(dir, filepath.FromSlash(name)) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + } + write("wiki.toml", "spec = \"0.1\"\n") + write("index.md", "---\nokf_version: \"0.1\"\n---\n") + write("b/task.md", "---\ntype: task\nstatus: todo\ntags: [ui]\n---\n- [ ] a task subtask\n") + write("b/note.md", "---\ntype: note\nstatus: published\ntags: [prose]\n---\n- [ ] a note subtask\n") + t.Chdir(dir) + + onlyTasks := []string{"--where", "type=task"} + cases := []struct { + name string + run func() int + want, leak string + }{ + {"property", func() int { return cmdProperty(append([]string{"status"}, onlyTasks...)) }, "todo", "published"}, + {"tags", func() int { return cmdTags(onlyTasks) }, "ui", "prose"}, + {"properties", func() int { return cmdProperties(onlyTasks) }, "status", ""}, + {"checkboxes", func() int { return cmdCheckboxes(onlyTasks) }, "a task subtask", "a note subtask"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + out, code := capture(t, tc.run) + if code != 0 { + t.Fatalf("exit=%d", code) + } + if !strings.Contains(out, tc.want) { + t.Errorf("output %q missing %q", out, tc.want) + } + if tc.leak != "" && strings.Contains(out, tc.leak) { + t.Errorf("output %q leaked %q from an entry the filter excludes", out, tc.leak) + } + }) + } + + // --where composes with --prefix rather than replacing it. + out, _ := capture(t, func() int { return cmdTags([]string{"--prefix", "/b", "--where", "type=note"}) }) + if !strings.Contains(out, "prose") || strings.Contains(out, "ui") { + t.Errorf("--prefix and --where should AND, got %q", out) + } + + // A named [file] is explicit, so the filters must not also apply to it. + out, _ = capture(t, func() int { return cmdCheckboxes([]string{"/b/note.md", "--where", "type=task"}) }) + if !strings.Contains(out, "a note subtask") { + t.Errorf("a named file should ignore --where, got %q", out) + } +} diff --git a/cmd/wiki/helpers.go b/cmd/wiki/helpers.go index 593e75a..5adfc8b 100644 --- a/cmd/wiki/helpers.go +++ b/cmd/wiki/helpers.go @@ -4,8 +4,8 @@ import ( "fmt" "os" - "github.com/agentic-wiki/wiki/internal/bundle" - "github.com/agentic-wiki/wiki/internal/index" + "github.com/agentic-wiki/wiki/bundle" + "github.com/agentic-wiki/wiki/index" ) // loadIndex discovers the bundle (from --root if given, else the current diff --git a/cmd/wiki/main.go b/cmd/wiki/main.go index ad72783..a4cea67 100644 --- a/cmd/wiki/main.go +++ b/cmd/wiki/main.go @@ -26,10 +26,10 @@ Commands: outline print an entry's heading hierarchy table extract a dataset's markdown table as csv/json (--n) search full-text search over entries (every word; --any --exact --where --prefix --lines) - checkboxes list open checklist items; optional [file] scopes to one entry (--all --done --prefix) - tags list tags in use (--counts --sort=name|count --prefix) - properties list frontmatter keys in use (--counts --sort --prefix) - property list values of a frontmatter key (--counts --sort --prefix) + checkboxes list open checklist items; optional [file] scopes to one entry (--all --done --prefix --where) + tags list tags in use (--counts --sort=name|count --prefix --where) + properties list frontmatter keys in use (--counts --sort --prefix --where) + property list values of a frontmatter key (--counts --sort --prefix --where) unresolved broken internal links orphans entries with no incoming links links an entry's outgoing links @@ -42,9 +42,11 @@ Commands: Run 'wiki -h' to see a command's flags --root operate on the bundle at (default: discover from cwd) -Every command accepts --format text|json|csv|tsv (default text; csv/tsv suit list-shaped results) -Filter frontmatter with --where key=value or key!=value (repeatable = AND) on list/search; type and - tags are ordinary fields, e.g. --where type=note, --where status!=done +Every command that reports results accepts --format text|json|csv|tsv (default text; csv/tsv suit + list-shaped results); version prints a bare string +Two filters, available wherever a set of entries is narrowed (list, search, checkboxes, tags, + properties, property): --prefix for where, --where key=value or key!=value for what + (repeatable = AND). type and tags are ordinary fields, e.g. --where type=note, --where status!=done list --format json carries each entry's full frontmatter; csv/tsv carry the canonical columns Exit codes: 0 ok, 1 no match or check errors, 2 error ` diff --git a/go.mod b/go.mod index 40d5a4a..f79bc4f 100644 --- a/go.mod +++ b/go.mod @@ -1,3 +1,5 @@ module github.com/agentic-wiki/wiki go 1.24 + +require github.com/BurntSushi/toml v1.6.0 diff --git a/go.sum b/go.sum new file mode 100644 index 0000000..f74b269 --- /dev/null +++ b/go.sum @@ -0,0 +1,2 @@ +github.com/BurntSushi/toml v1.6.0 h1:dRaEfpa2VI55EwlIW72hMRHdWouJeRF7TPYhI+AUQjk= +github.com/BurntSushi/toml v1.6.0/go.mod h1:ukJfTF/6rtPPRCnwkur4qwRxa8vTRFBF0uk2lLoLwho= diff --git a/index/consumer_api_test.go b/index/consumer_api_test.go new file mode 100644 index 0000000..46a87a9 --- /dev/null +++ b/index/consumer_api_test.go @@ -0,0 +1,149 @@ +package index + +import ( + "slices" + "strings" + "testing" +) + +// The accessors below exist only for library consumers: no command calls them, +// so nothing else would notice if they broke. + +// The whole point of the copy is that a consumer reading frontmatter cannot +// edit the engine's state by accident. +func TestFrontmatterIsACopy(t *testing.T) { + idx := build(t, map[string]string{ + "index.md": "---\nokf_version: \"0.1\"\n---\n", + "a.md": "---\ntype: task\ntitle: A\ntags: [ui, api]\n---\nbody\n", + }) + e, err := idx.Resolve("/a.md") + if err != nil { + t.Fatal(err) + } + + fm := e.Frontmatter() + if got := fm["title"]; got != "A" { + t.Errorf("title=%v", got) + } + if got, ok := fm["tags"].([]string); !ok || !slices.Equal(got, []string{"ui", "api"}) { + t.Fatalf("tags=%#v", fm["tags"]) + } + + // Mutating the returned map, and the slices inside it, must not reach the entry. + fm["title"] = "clobbered" + delete(fm, "type") + fm["tags"].([]string)[0] = "clobbered" + + if e.Field("title") != "A" { + t.Errorf("mutating the copy changed the entry: title=%q", e.Field("title")) + } + if e.Type != "task" { + t.Errorf("deleting from the copy changed the entry: type=%q", e.Type) + } + if got := e.FieldList("tags"); !slices.Equal(got, []string{"ui", "api"}) { + t.Errorf("mutating a copied list reached the entry: %#v", got) + } + // A second call is unaffected by what was done to the first. + if e.Frontmatter()["title"] != "A" { + t.Error("a later call returned the mutated value") + } +} + +// A body link is written relative and the graph is keyed root-absolute, so a +// consumer rendering a body needs both directions. These are inverses. +func TestResolveAndRelativeLinkRoundTrip(t *testing.T) { + idx := build(t, map[string]string{ + "index.md": "---\nokf_version: \"0.1\"\n---\n", + "a.md": "---\ntype: note\n---\n", + "sub/b.md": "---\ntype: note\n---\n", + "sub/deep/c.md": "---\ntype: note\n---\n", + }) + + cases := []struct{ from, written, want string }{ + {"/sub/b.md", "./deep/c.md", "/sub/deep/c.md"}, + {"/sub/b.md", "../a.md", "/a.md"}, + {"/sub/deep/c.md", "../../a.md", "/a.md"}, + {"/a.md", "./sub/b.md", "/sub/b.md"}, + // A root-absolute link still resolves; it is valid, just not canonical. + {"/sub/b.md", "/a.md", "/a.md"}, + // An anchor rides along with the target. + {"/sub/b.md", "../a.md#heading", "/a.md#heading"}, + } + for _, tc := range cases { + got, outside := idx.ResolveLink(tc.from, tc.written) + if outside { + t.Errorf("ResolveLink(%q, %q) reported outside", tc.from, tc.written) + continue + } + if got != tc.want { + t.Errorf("ResolveLink(%q, %q) = %q, want %q", tc.from, tc.written, got, tc.want) + } + } + + // RelativeLink is the inverse: re-spelling a resolved target from the same + // file must resolve back to it. + for _, tc := range cases { + target, _, _ := strings.Cut(tc.want, "#") + rel := RelativeLink(tc.from, target) + back, _ := idx.ResolveLink(tc.from, rel) + if back != target { + t.Errorf("RelativeLink(%q, %q) = %q, which resolves back to %q", tc.from, target, rel, back) + } + } +} + +// A target above the bundle root is neither an edge nor broken, and a consumer +// must be able to tell, or it will try to navigate out of the bundle. +func TestResolveLinkReportsOutside(t *testing.T) { + idx := build(t, map[string]string{ + "index.md": "---\nokf_version: \"0.1\"\n---\n", + "sub/b.md": "---\ntype: note\n---\n", + }) + if _, outside := idx.ResolveLink("/sub/b.md", "../../escape.md"); !outside { + t.Error("a target above the bundle root should report outside") + } + if _, outside := idx.ResolveLink("/sub/b.md", "../index.md"); outside { + t.Error("a target inside the bundle should not report outside") + } +} + +// The query syntax is part of the query contract, so its parse belongs to the +// library. The subtleties are easy to regress: != is matched before =, so a +// value may contain =, and values are unquoted the way frontmatter is. +func TestParseFilter(t *testing.T) { + for _, tc := range []struct { + in string + key string + value string + negate bool + wantErr bool + }{ + {in: "type=task", key: "type", value: "task"}, + {in: "status!=done", key: "status", value: "done", negate: true}, + {in: "status=", key: "status", value: ""}, + {in: "status!=", key: "status", value: "", negate: true}, + // != is matched first, so this is a negation, not a key named "a!". + {in: "a!=b", key: "a", value: "b", negate: true}, + // A value may contain =, because the split takes the first operator only. + {in: "url=a=b", key: "url", value: "a=b"}, + {in: `title="quoted value"`, key: "title", value: "quoted value"}, + {in: "nope", wantErr: true}, + {in: "", wantErr: true}, + } { + t.Run(tc.in, func(t *testing.T) { + got, err := ParseFilter(tc.in) + if tc.wantErr { + if err == nil { + t.Errorf("ParseFilter(%q) should fail, got %+v", tc.in, got) + } + return + } + if err != nil { + t.Fatalf("ParseFilter(%q): %v", tc.in, err) + } + if got.Key != tc.key || got.Value != tc.value || got.Negate != tc.negate { + t.Errorf("ParseFilter(%q) = %+v, want {%q %q %v}", tc.in, got, tc.key, tc.value, tc.negate) + } + }) + } +} diff --git a/internal/index/glob.go b/index/glob.go similarity index 100% rename from internal/index/glob.go rename to index/glob.go diff --git a/internal/index/glob_test.go b/index/glob_test.go similarity index 100% rename from internal/index/glob_test.go rename to index/glob_test.go diff --git a/internal/index/index.go b/index/index.go similarity index 81% rename from internal/index/index.go rename to index/index.go index ffc2d72..06b5c26 100644 --- a/internal/index/index.go +++ b/index/index.go @@ -14,9 +14,9 @@ import ( "time" "unicode" - "github.com/agentic-wiki/wiki/internal/bundle" - "github.com/agentic-wiki/wiki/internal/parse" + "github.com/agentic-wiki/wiki/bundle" "github.com/agentic-wiki/wiki/internal/wikilink" + "github.com/agentic-wiki/wiki/parse" ) // Link is an internal link as indexed: its on-disk form (Raw, anchor kept) and @@ -56,6 +56,7 @@ type Entry struct { Headings []parse.Heading `json:"-"` wikilinks []wikilink.Link // [[wikilinks]] parsed from the body (compat); resolved into Links (Wikilink) in Build abs string + root string // the bundle dir, so an entry can re-parse itself after a write fm map[string]any } @@ -167,7 +168,7 @@ func Build(b *bundle.Bundle) (*Index, error) { if matchAnyGlob(idx.ignoreIn, "/"+filepath.ToSlash(rel)) { return nil // wiki.toml `ignore`: a declared non-entry, excluded from the content index } - e, err := parseEntry(b, path) + e, err := parseEntry(b.Dir, path) if err != nil { return err } @@ -240,12 +241,12 @@ func (idx *Index) resolveIgnore() { } } -func parseEntry(b *bundle.Bundle, abs string) (*Entry, error) { +func parseEntry(root, abs string) (*Entry, error) { data, err := os.ReadFile(abs) if err != nil { return nil, err } - rel, _ := filepath.Rel(b.Dir, abs) + rel, _ := filepath.Rel(root, abs) content := string(data) fm, body := parse.Frontmatter(content) // Links/checkboxes/headings are parsed from the frontmatter-stripped body, so @@ -255,7 +256,7 @@ func parseEntry(b *bundle.Bundle, abs string) (*Entry, error) { // rel is the file's path under the bundle root ("finance/income.md"); // entryPath is its root-absolute bundle id ("/finance/income.md"). entryPath := "/" + filepath.ToSlash(rel) - links, selfAnchors, outside := resolveLinks(b.Dir, parse.Links(body), entryPath, offset) + links, selfAnchors, outside := resolveLinks(root, parse.Links(body), entryPath, offset) checkboxes, heads := parse.Checkboxes(body), parse.Headings(body) for i := range checkboxes { checkboxes[i].Line += offset @@ -277,6 +278,7 @@ func parseEntry(b *bundle.Bundle, abs string) (*Entry, error) { Headings: heads, wikilinks: wikilinks, abs: abs, + root: root, fm: fm, }, nil } @@ -478,6 +480,30 @@ type PropFilter struct { Negate bool } +// ParseFilter reads one `key=value` / `key!=value` expression. +// +// The spelling is part of the query contract, not of the CLI's argument +// handling, so it lives here: a consumer that accepted the same syntax and +// parsed it itself would be a second implementation of a rule with one correct +// home, and the two would drift on exactly the details that are easy to miss. +// `!=` is matched before `=` so a value may itself contain `=`, and the value is +// unquoted the way frontmatter is, so a quote surviving the shell +// (`k="v"`) still compares equal to `k: "v"`. +func ParseFilter(s string) (PropFilter, error) { + neg := false + kRaw, vRaw, ok := strings.Cut(s, "!=") + if ok { + neg = true + } else { + kRaw, vRaw, ok = strings.Cut(s, "=") + } + key := parse.Unquote(kRaw) + if !ok || key == "" { + return PropFilter{}, fmt.Errorf("%q is not a filter: expected key=value or key!=value", s) + } + return PropFilter{Key: key, Value: parse.Unquote(vRaw), Negate: neg}, nil +} + // Filter returns entries under pathPrefix (empty = the whole bundle) that satisfy // every property filter (nil = no property constraint). props are ANDed; a // list-valued field matches when it includes the value, a scalar when it equals @@ -509,11 +535,12 @@ func (e *Entry) matchesAll(props []PropFilter) bool { return true } -// TagCounts returns every tag in the bundle (optionally within a path prefix) +// TagCounts returns every tag in the bundle (optionally narrowed by path prefix +// and property filters, the same pair Filter takes) // with the number of entries carrying it. -func (idx *Index) TagCounts(pathPrefix string) map[string]int { +func (idx *Index) TagCounts(pathPrefix string, props []PropFilter) map[string]int { counts := map[string]int{} - for _, e := range idx.Filter(pathPrefix, nil) { + for _, e := range idx.Filter(pathPrefix, props) { for _, t := range dedupe(parse.Strings(e.fm, "tags")) { counts[t]++ } @@ -521,11 +548,11 @@ func (idx *Index) TagCounts(pathPrefix string) map[string]int { return counts } -// PropertyKeyCounts returns every frontmatter key in use (optionally within a -// path prefix) with the number of entries that set it. -func (idx *Index) PropertyKeyCounts(pathPrefix string) map[string]int { +// PropertyKeyCounts returns every frontmatter key in use (optionally narrowed by +// path prefix and property filters) with the number of entries that set it. +func (idx *Index) PropertyKeyCounts(pathPrefix string, props []PropFilter) map[string]int { counts := map[string]int{} - for _, e := range idx.Filter(pathPrefix, nil) { + for _, e := range idx.Filter(pathPrefix, props) { for k := range e.fm { counts[k]++ } @@ -534,11 +561,11 @@ func (idx *Index) PropertyKeyCounts(pathPrefix string) map[string]int { } // PropertyValueCounts returns the distinct values of frontmatter key (optionally -// within a path prefix) with the number of entries holding each. A list-valued -// key (e.g. tags) contributes each element. -func (idx *Index) PropertyValueCounts(key, pathPrefix string) map[string]int { +// narrowed by path prefix and property filters) with the number of entries +// holding each. A list-valued key (e.g. tags) contributes each element. +func (idx *Index) PropertyValueCounts(key, pathPrefix string, props []PropFilter) map[string]int { counts := map[string]int{} - for _, e := range idx.Filter(pathPrefix, nil) { + for _, e := range idx.Filter(pathPrefix, props) { for _, v := range dedupe(parse.Strings(e.fm, key)) { counts[v]++ } @@ -642,9 +669,12 @@ func matchLine(line, q string, words []string, mode SearchMode) bool { } // BrokenLink is an internal link with no target file. +// +// The json keys match LinkRef's: a broken link is a link row like any other, and +// naming its ends differently only made them harder to join. type BrokenLink struct { From string `json:"from"` - Target string `json:"target"` + Target string `json:"to"` Line int `json:"line"` } @@ -699,17 +729,25 @@ type LinkRef struct { Line int `json:"line"` } -// OutLinks returns the entry's outgoing internal links as unique targets, in -// first-seen order. (Whether a target resolves is a health question for `check` -// / `unresolved`, not this navigational view.) -func (idx *Index) OutLinks(e *Entry) []LinkRef { - seen := map[string]bool{} - var refs []LinkRef +// Links returns the internal links written in the entry at path, one LinkRef per +// occurrence, in document order. The mirror of Backlinks, which answers the same +// question from the other end. +// +// Occurrences, not unique targets: an entry may link the same target from two +// places, and each has its own line. Collapsing them here would make Line the +// first of several, silently, which is a presentation choice the engine has no +// business making — `wiki links` shows bare targets and so dedupes, `wiki +// backlinks` shows file:line and so does not. +// +// An unknown path yields nil. Whether a target resolves is a health question for +// `check` / `unresolved`, not this navigational view. +func (idx *Index) Links(path string) []LinkRef { + e, ok := idx.byPath[path] + if !ok { + return nil + } + refs := make([]LinkRef, 0, len(e.Links)) for _, l := range e.Links { - if seen[l.Target] { - continue - } - seen[l.Target] = true refs = append(refs, LinkRef{From: e.Path, To: l.Target, Text: l.Text, Line: l.Line}) } return refs @@ -718,6 +756,12 @@ func (idx *Index) OutLinks(e *Entry) []LinkRef { // Backlinks returns every internal link that points to target, one LinkRef per // occurrence (a source that links several times appears once per link), sorted // by source path then line. Relative links count, they resolve to the same target. +// +// This scans every edge in the bundle, which is the right shape for one target +// (sub-millisecond on a 5k-entry bundle) and the wrong one for all of them: a +// loop over every entry rescans the whole graph each time, ~2s where one pass +// would be ~10ms. Nothing needs all of them yet; when something does, it wants +// a single pass over Entries building a map keyed by Link.Target, not this. func (idx *Index) Backlinks(target string) []LinkRef { var refs []LinkRef for _, e := range idx.Entries { @@ -737,7 +781,7 @@ func (idx *Index) Backlinks(target string) []LinkRef { } // FileRewrite records what a move rewrote in one entry: body links, and, when -// --include-frontmatter is set, frontmatter values equal to the moved path. +// --include-frontmatter is set, frontmatter values referencing the moved path. type FileRewrite struct { Path string `json:"_path"` Links int `json:"links"` @@ -758,11 +802,12 @@ type MoveResult struct { // src (relative or root-absolute alike, matched by resolved target) is respelled // relative from the linking file to dest, and the moved file's own outgoing links // are respelled relative from its new directory (its dir changed, so its relative -// links would otherwise dangle). With includeFrontmatter it also rewrites -// frontmatter values equal to src's path (an opt-in: frontmatter is otherwise -// opaque, since the tool cannot know a path-shaped value is a reference rather than -// a snapshot; these stay root-absolute, as frontmatter is metadata, not a rendered -// link). With dryRun it computes the plan without writing. There is no rollback: on +// links would otherwise dangle). With includeFrontmatter it treats `*.md`-suffixed +// frontmatter values as references and keeps them valid too, resolving them the same +// way but writing them root-absolute, the stable-key form a metadata field needs +// (an opt-in: frontmatter is otherwise opaque, since the tool cannot know a +// path-shaped value is a reference rather than a snapshot). +// With dryRun it computes the plan without writing. There is no rollback: on // a mid-way write error it returns what was already done so `unresolved` can surface // any leftovers. func (idx *Index) Move(srcArg, dest string, dryRun, includeFrontmatter bool) (*MoveResult, error) { @@ -815,18 +860,11 @@ func (idx *Index) Move(srcArg, dest string, dryRun, includeFrontmatter bool) (*M } // A same-directory rename leaves most spellings unchanged; drop the no-ops. rws = slices.DeleteFunc(rws, func(r rewrite) bool { return r.newRaw == r.oldRaw }) - var fmKeys map[string]bool + var fmRWs map[string][]fmRewrite if includeFrontmatter { - for k := range e.fm { - if slices.Contains(parse.Strings(e.fm, k), src.Path) { - if fmKeys == nil { - fmKeys = map[string]bool{} - } - fmKeys[k] = true - } - } + fmRWs = idx.frontmatterRewrites(e, src.Path, dest) } - if len(rws) == 0 && len(fmKeys) == 0 { + if len(rws) == 0 && len(fmRWs) == 0 { continue } raw, err := e.Raw() @@ -852,17 +890,17 @@ func (idx *Index) Move(srcArg, dest string, dryRun, includeFrontmatter bool) (*M }) } fields := 0 - if len(fmKeys) > 0 { + if len(fmRWs) > 0 { _, body := parse.Frontmatter(raw) fmEnd := strings.Count(raw[:len(raw)-len(body)], "\n") - fields = rewriteFrontmatterRefs(lines, fmEnd, fmKeys, src.Path, dest) + fields = rewriteFrontmatterRefs(lines, fmEnd, fmRWs) } if n == 0 && fields == 0 { continue } res.Rewrites = append(res.Rewrites, FileRewrite{Path: e.Path, Links: n, FrontmatterRefs: fields}) if !dryRun { - if err := os.WriteFile(e.abs, []byte(strings.Join(lines, "\n")), 0o644); err != nil { + if err := writeFile(e.abs, []byte(strings.Join(lines, "\n"))); err != nil { return res, err } } @@ -872,21 +910,100 @@ func (idx *Index) Move(srcArg, dest string, dryRun, includeFrontmatter bool) (*M if err := os.MkdirAll(filepath.Dir(destAbs), 0o755); err != nil { return res, err } - if err := os.Rename(src.abs, destAbs); err != nil { + // Through rename, not os.Rename: a move needs delete access to the + // source, so it meets the same contention a replace does even though the + // destination is known not to exist. + if err := rename(src.abs, destAbs); err != nil { return res, err } } return res, nil } -// rewriteFrontmatterRefs rewrites, within the first fmEnd lines (an entry's -// frontmatter block), any value under a key in hitKeys that equals oldPath, -// changing it to newPath. hitKeys (derived from the parsed frontmatter) gates the -// rewrite, so a path that appears only as prose inside some other value is never -// touched; the token boundaries keep it to whole values (scalar, flow, or block -// list). Returns the number of values rewritten. -func rewriteFrontmatterRefs(lines []string, fmEnd int, hitKeys map[string]bool, oldPath, newPath string) int { - tok := regexp.MustCompile(`([:\[,\s"'])` + regexp.QuoteMeta(oldPath) + `([\],\s"']|$)`) +// fmRewrite is one frontmatter value respelling: the value exactly as written on +// disk, what it becomes, and the token-boundary pattern that finds it in a line. +type fmRewrite struct { + oldRaw, newRaw string + tok *regexp.Regexp +} + +// frontmatterRewrites plans the frontmatter respellings a move entails for entry +// e, keyed by frontmatter key. It is the frontmatter twin of the body-link pass in +// Move: a value referencing src is repointed at dest, and (only for the moved file +// itself, whose directory changes, so its relative refs would otherwise dangle) a +// relative value referencing anything else is normalized in place. +// +// Matching is by resolved target, not by spelling, so a relative ref is found as +// readily as a root-absolute one. The rewrite always emits the **root-absolute** +// form, which is where frontmatter deliberately parts company with body links: +// a body link is relative because it must navigate in a renderer, while a +// frontmatter ref is never rendered as a link and instead must be a *stable key*. +// Only one spelling per target keeps `--where blockers=/active/x.md` finding every +// referrer; relative values spell the same target differently from each directory, +// so no single query can match them all. +// +// A frontmatter value is considered a reference only when it ends in `.md` (after +// any `#anchor`). That suffix is the whole heuristic, and it is what keeps the +// pass from mangling ordinary metadata: an arbitrary value like `title: Some Note` +// would otherwise resolve to a perfectly valid in-bundle path and be rewritten as +// one. Values resolving outside the bundle are left alone, as body links are. +func (idx *Index) frontmatterRewrites(e *Entry, srcPath, dest string) map[string][]fmRewrite { + var out map[string][]fmRewrite + for k := range e.fm { + var seen map[string]bool + for _, v := range parse.Strings(e.fm, k) { + if p, _, _ := strings.Cut(v, "#"); !strings.HasSuffix(p, ".md") { + continue + } + abs, escapes := normalizeLink(idx.Bundle.Dir, e.Path, v) + if escapes { + continue + } + newRaw := abs // already root-absolute: the canonical frontmatter form + switch { + case stripAnchor(abs) == srcPath: + newRaw = dest + anchorOf(abs) + case e.Path != srcPath: + continue // only the moved file normalizes its refs to unmoved targets + } + if newRaw == v || seen[v] { + continue + } + if seen == nil { + seen = map[string]bool{} + } + seen[v] = true + if out == nil { + out = map[string][]fmRewrite{} + } + out[k] = append(out[k], fmRewrite{ + oldRaw: v, + newRaw: newRaw, + tok: regexp.MustCompile(`([:\[,\s"'])` + regexp.QuoteMeta(v) + `([\],\s"']|$)`), + }) + } + // Longest first, so a value that is a suffix-substring of another is never + // matched inside it. + slices.SortFunc(out[k], func(a, b fmRewrite) int { return len(b.oldRaw) - len(a.oldRaw) }) + } + return out +} + +func stripAnchor(p string) string { p, _, _ = strings.Cut(p, "#"); return p } + +func anchorOf(p string) string { + if _, a, ok := strings.Cut(p, "#"); ok { + return "#" + a + } + return "" +} + +// rewriteFrontmatterRefs applies planned respellings within the first fmEnd lines +// (an entry's frontmatter block). Keying by frontmatter key gates the rewrite, so a +// path that appears only as prose inside some other value is never touched; the +// token boundaries keep it to whole values (scalar, flow, or block list). Returns +// the number of values rewritten. +func rewriteFrontmatterRefs(lines []string, fmEnd int, rewrites map[string][]fmRewrite) int { n, lastKey := 0, "" for i := 0; i < fmEnd && i < len(lines); i++ { t := strings.TrimRight(lines[i], "\r") @@ -903,17 +1020,16 @@ func rewriteFrontmatterRefs(lines []string, fmEnd int, hitKeys map[string]bool, owner = strings.TrimSpace(key) lastKey = owner } - if !hitKeys[owner] { - continue - } c := 0 - nt := tok.ReplaceAllStringFunc(t, func(m string) string { - c++ - sub := tok.FindStringSubmatch(m) - return sub[1] + newPath + sub[2] - }) + for _, rw := range rewrites[owner] { + t = rw.tok.ReplaceAllStringFunc(t, func(m string) string { + c++ + sub := rw.tok.FindStringSubmatch(m) + return sub[1] + rw.newRaw + sub[2] + }) + } if c > 0 { - lines[i] = nt + cr + lines[i] = t + cr n += c } } @@ -1102,12 +1218,12 @@ func (idx *Index) fixOKFVersion(apply bool) (*Fix, error) { if err != nil { return nil, err } - updated, err := setFrontmatterValue(string(raw), "okf_version", want) + updated, err := setFrontmatterValue(string(raw), "okf_version", want, false) if err != nil { return nil, err } if apply { - if err := os.WriteFile(root.abs, []byte(updated), 0o644); err != nil { + if err := writeFile(root.abs, []byte(updated)); err != nil { return nil, err } } @@ -1234,7 +1350,7 @@ func (idx *Index) ConvertWikilinks(apply bool) ([]Fix, error) { changed = true } if apply && changed { - if err := os.WriteFile(e.abs, []byte(strings.Join(lines, "\n")), 0o644); err != nil { + if err := writeFile(e.abs, []byte(strings.Join(lines, "\n"))); err != nil { return nil, err } } @@ -1279,51 +1395,13 @@ func (idx *Index) normalizeEntryLinks(e *Entry, apply bool) ([]Fix, error) { }) } if apply { - if err := os.WriteFile(e.abs, []byte(strings.Join(lines, "\n")), 0o644); err != nil { + if err := writeFile(e.abs, []byte(strings.Join(lines, "\n"))); err != nil { return nil, err } } return changes, nil } -// setFrontmatterValue returns content with the frontmatter `key` set to a quoted -// value, updating an existing key in place or inserting it as the last -// frontmatter line. It errors if there is no frontmatter block, and preserves -// the file's existing line endings. -func setFrontmatterValue(content, key, value string) (string, error) { - lines := strings.Split(content, "\n") - if len(lines) == 0 || strings.TrimRight(lines[0], "\r") != "---" { - // No frontmatter block yet: create one carrying just this key. - return fmt.Sprintf("---\n%s: \"%s\"\n---\n%s", key, value, content), nil - } - closeIdx := -1 - for i := 1; i < len(lines); i++ { - if strings.TrimRight(lines[i], "\r") == "---" { - closeIdx = i - break - } - } - if closeIdx == -1 { - return "", fmt.Errorf("unterminated frontmatter") - } - cr := "" - if strings.HasSuffix(lines[0], "\r") { - cr = "\r" - } - newLine := fmt.Sprintf(`%s: "%s"`, key, value) + cr - for i := 1; i < closeIdx; i++ { - if k, _, ok := strings.Cut(strings.TrimRight(lines[i], "\r"), ":"); ok && strings.TrimSpace(k) == key { - lines[i] = newLine - return strings.Join(lines, "\n"), nil - } - } - out := make([]string, 0, len(lines)+1) - out = append(out, lines[:closeIdx]...) - out = append(out, newLine) - out = append(out, lines[closeIdx:]...) - return strings.Join(out, "\n"), nil -} - // parseTimestamp parses an ISO 8601 timestamp in the two forms the spec allows: // an RFC 3339 datetime or a bare YYYY-MM-DD date. ok is false if neither matches. func parseTimestamp(s string) (time.Time, bool) { @@ -1365,3 +1443,50 @@ func looksLikeNonISODate(heading string) bool { func hasPathPrefix(path, prefix string) bool { return strings.HasPrefix(strings.TrimPrefix(path, "/"), strings.TrimPrefix(prefix, "/")) } + +// Field returns a frontmatter value as a scalar ("" when absent or list-valued). +func (e *Entry) Field(key string) string { return parse.String(e.fm, key) } + +// FieldList returns a frontmatter value as a list, treating a lone scalar as a +// one-element list. +// +// That normalization is the same one MatchProperty applies, so a consumer +// filtering entries by hand reaches the same answer as `--where` rather than a +// subtly different one. +func (e *Entry) FieldList(key string) []string { return parse.Strings(e.fm, key) } + +// Frontmatter returns the entry's frontmatter verbatim, exactly as written, with +// no per-field coercion. +// +// The map is a copy, lists included: the index holds the original, and a caller +// that could mutate it would be editing the engine's state rather than reading it. +func (e *Entry) Frontmatter() map[string]any { + out := make(map[string]any, len(e.fm)) + for k, v := range e.fm { + if list, ok := v.([]string); ok { + out[k] = slices.Clone(list) + continue + } + out[k] = v + } + return out +} + +// ResolveLink resolves a link target as written in the entry at fromPath to its +// canonical root-absolute bundle path, the same key the graph is built on. +// +// outside reports that the target lands above the bundle root, which is neither +// an edge nor broken: it points out of a self-contained bundle, so callers skip +// it rather than resolving it on disk. +// +// Exported because anything rendering a body has to turn its links into +// something it can navigate, and doing that by hand is how the rule ends up with +// more than one home. +func (idx *Index) ResolveLink(fromPath, target string) (path string, outside bool) { + return normalizeLink(idx.Bundle.Dir, fromPath, target) +} + +// RelativeLink spells a resolved root-absolute target as it should be written on +// disk from fromPath: the canonical on-disk form, relative so it navigates in any +// renderer. The inverse of ResolveLink, and what anything writing a link needs. +func RelativeLink(fromPath, target string) string { return relativeLink(fromPath, target) } diff --git a/internal/index/index_test.go b/index/index_test.go similarity index 90% rename from internal/index/index_test.go rename to index/index_test.go index 4386158..014edd1 100644 --- a/internal/index/index_test.go +++ b/index/index_test.go @@ -1,13 +1,14 @@ package index import ( + "maps" "os" "path/filepath" "strings" "testing" - "github.com/agentic-wiki/wiki/internal/bundle" - "github.com/agentic-wiki/wiki/internal/parse" + "github.com/agentic-wiki/wiki/bundle" + "github.com/agentic-wiki/wiki/parse" ) func build(t *testing.T, files map[string]string) *Index { @@ -437,7 +438,8 @@ func TestMoveIncludeFrontmatter(t *testing.T) { t.Errorf("default move must NOT touch the epic: field: %q", read(base, "login.md")) } - // --include-frontmatter: frontmatter values equal to the path move too... + // --include-frontmatter: frontmatter refs move too, staying root-absolute (the + // stable-key form; body links go relative, frontmatter does not). idx := build(t, files) res, err := idx.Move("/epics/auth.md", "/epics/authn.md", false, true) if err != nil { @@ -469,6 +471,56 @@ func TestMoveIncludeFrontmatter(t *testing.T) { } } +// Frontmatter refs are matched by resolved target, like body links, so a relative +// ref is found as readily as a root-absolute one. Unlike body links they are +// *written* root-absolute: one spelling per target is what keeps `--where` able to +// find every referrer. The moved file's own relative refs are normalized too, since +// its directory changed and they would otherwise dangle. +func TestMoveIncludeFrontmatterRelative(t *testing.T) { + files := map[string]string{ + "index.md": "---\nokf_version: \"0.1\"\n---\n[t](/tasks/a.md)\n", + "tasks/a.md": "---\ntype: task\ntitle: Some Note\nstatus: todo\nblockers: [./b.md, ../lib/c.md]\nspec: ../lib/c.md#usage\nexternal: ../../outside.md\n---\na\n", + "tasks/b.md": "---\ntype: task\nblockers: [./a.md]\n---\nb\n", + "lib/c.md": "---\ntype: note\n---\nc\n", + } + read := func(idx *Index, rel string) string { + b, _ := os.ReadFile(filepath.Join(idx.Bundle.Dir, filepath.FromSlash(rel))) + return string(b) + } + idx := build(t, files) + if _, err := idx.Move("/tasks/a.md", "/archive/2026/a.md", false, true); err != nil { + t.Fatal(err) + } + + // An inbound relative ref is found and repointed, and normalized to the + // canonical root-absolute form so every referrer spells it identically. + if got := read(idx, "tasks/b.md"); !strings.Contains(got, "blockers: [/archive/2026/a.md]") { + t.Errorf("inbound relative ref should be repointed: %q", got) + } + + moved := read(idx, "archive/2026/a.md") + // The moved file's own relative refs would dangle from the new directory, so + // they are normalized in place. + if !strings.Contains(moved, "blockers: [/tasks/b.md, /lib/c.md]") { + t.Errorf("moved file's own refs should be normalized: %q", moved) + } + // An anchor survives the normalization. + if !strings.Contains(moved, "spec: /lib/c.md#usage") { + t.Errorf("anchor should be preserved: %q", moved) + } + // Values that are not `.md` paths are metadata, not refs, even though they + // would resolve to a valid in-bundle path. This is the guard that keeps the + // pass from rewriting `title` to `./Some Note`. + if !strings.Contains(moved, "title: Some Note") || !strings.Contains(moved, "status: todo") { + t.Errorf("non-path values must be left alone: %q", moved) + } + // A value resolving outside the bundle is left exactly as authored, as + // out-of-bundle body links are. + if !strings.Contains(moved, "external: ../../outside.md") { + t.Errorf("out-of-bundle value must be left alone: %q", moved) + } +} + func TestCheckClean(t *testing.T) { idx := build(t, map[string]string{ "index.md": "---\nokf_version: \"0.1\"\n---\n[a](/a.md)\n", @@ -1001,7 +1053,7 @@ func TestSetFrontmatterValue(t *testing.T) { {"create", "no frontmatter here\n", "---\nokf_version: \"0.1\"\n---\nno frontmatter here\n"}, } for _, c := range cases { - got, err := setFrontmatterValue(c.in, "okf_version", "0.1") + got, err := setFrontmatterValue(c.in, "okf_version", "0.1", false) if err != nil || got != c.want { t.Errorf("%s: got %q err=%v, want %q", c.name, got, err, c.want) } @@ -1136,9 +1188,19 @@ func TestLinkGraph(t *testing.T) { }) a, _ := idx.Resolve("a.md") - // outgoing links deduped by target (a links to /b.md twice) - if out := idx.OutLinks(a); len(out) != 2 || out[0].To != "/b.md" || out[1].To != "/nope.md" { - t.Fatalf("a out-links = %+v want [/b.md /nope.md] (deduped)", out) + // Every occurrence, in document order: a links to /b.md twice, and each has + // its own line. Collapsing them is the CLI's presentation choice, not the + // engine's, so the engine reports what is written. + out := idx.Links(a.Path) + if len(out) != 3 || out[0].To != "/b.md" || out[1].To != "/b.md" || out[2].To != "/nope.md" { + t.Fatalf("a links = %+v, want [/b.md /b.md /nope.md] in document order", out) + } + if out[0].Line == out[1].Line { + t.Errorf("the two /b.md occurrences should carry distinct lines, got %d and %d", out[0].Line, out[1].Line) + } + // The mirror of Backlinks: both keyed by path, both one ref per occurrence. + if idx.Links("/does-not-exist.md") != nil { + t.Error("an unknown path should yield nil") } // backlinks: every occurrence (a links to /b.md twice -> both rows), sorted by @@ -1429,16 +1491,16 @@ func TestCounts(t *testing.T) { }) // TagCounts: y is in a and b (twice in b, but an entry counts once). - tags := idx.TagCounts("") + tags := idx.TagCounts("", nil) if len(tags) != 3 || tags["x"] != 1 || tags["y"] != 2 || tags["z"] != 1 { t.Errorf("TagCounts = %v, want {x:1, y:2, z:1}", tags) } - if sub := idx.TagCounts("sub/"); len(sub) != 1 || sub["z"] != 1 { + if sub := idx.TagCounts("sub/", nil); len(sub) != 1 || sub["z"] != 1 { t.Errorf("TagCounts(sub/) = %v, want {z:1}", sub) } // PropertyKeyCounts: index.md contributes only okf_version (reserved, no type). - keys := idx.PropertyKeyCounts("") + keys := idx.PropertyKeyCounts("", nil) for key, want := range map[string]int{"okf_version": 1, "type": 3, "status": 3, "tags": 3} { if keys[key] != want { t.Errorf("PropertyKeyCounts[%q] = %d, want %d (all: %v)", key, keys[key], want, keys) @@ -1446,19 +1508,58 @@ func TestCounts(t *testing.T) { } // PropertyValueCounts: scalar key (status, type) and list key (tags), with prefix. - if st := idx.PropertyValueCounts("status", ""); st["open"] != 2 || st["done"] != 1 { + if st := idx.PropertyValueCounts("status", "", nil); st["open"] != 2 || st["done"] != 1 { t.Errorf("PropertyValueCounts(status) = %v, want {open:2, done:1}", st) } - if ty := idx.PropertyValueCounts("type", ""); ty["note"] != 2 || ty["concept"] != 1 { + if ty := idx.PropertyValueCounts("type", "", nil); ty["note"] != 2 || ty["concept"] != 1 { t.Errorf("PropertyValueCounts(type) = %v, want {note:2, concept:1}", ty) } - if tg := idx.PropertyValueCounts("tags", ""); tg["x"] != 1 || tg["y"] != 2 || tg["z"] != 1 { + if tg := idx.PropertyValueCounts("tags", "", nil); tg["x"] != 1 || tg["y"] != 2 || tg["z"] != 1 { t.Errorf("PropertyValueCounts(tags) = %v, want {x:1, y:2, z:1}", tg) } - if st := idx.PropertyValueCounts("status", "sub/"); len(st) != 1 || st["open"] != 1 { + if st := idx.PropertyValueCounts("status", "sub/", nil); len(st) != 1 || st["open"] != 1 { t.Errorf("PropertyValueCounts(status, sub/) = %v, want {open:1}", st) } - if got := idx.PropertyValueCounts("nope", ""); len(got) != 0 { + if got := idx.PropertyValueCounts("nope", "", nil); len(got) != 0 { t.Errorf("PropertyValueCounts(nope) = %v, want empty", got) } } + +// The asymmetry this closes: a folder holding more than one kind of entry could +// be narrowed by path but not by field, so a board's status vocabulary came back +// polluted with the statuses of the notes filed beside it. +func TestVocabularyCommandsTakeWhere(t *testing.T) { + idx := build(t, map[string]string{ + "index.md": "---\nokf_version: \"0.1\"\n---\n", + "backlog/a.md": "---\ntype: task\nstatus: todo\ntags: [ui]\n---\n", + "backlog/b.md": "---\ntype: task\nstatus: done\ntags: [ui, api]\n---\n", + "backlog/note.md": "---\ntype: note\nstatus: published\ntags: [prose]\n---\n", + "elsewhere/other.md": "---\ntype: task\nstatus: blocked\ntags: [infra]\n---\n", + }) + task := []PropFilter{{Key: "type", Value: "task"}} + + got := idx.PropertyValueCounts("status", "/backlog", task) + want := map[string]int{"todo": 1, "done": 1} + if !maps.Equal(got, want) { + t.Errorf("status under /backlog for tasks = %v, want %v (a note's status leaked in)", got, want) + } + // --prefix and --where each still narrow on their own. + if got := idx.PropertyValueCounts("status", "/backlog", nil); len(got) != 3 { + t.Errorf("prefix alone = %v, want all three statuses", got) + } + if got := idx.PropertyValueCounts("status", "", task); len(got) != 3 { + t.Errorf("where alone = %v, want the three tasks' statuses", got) + } + + if got, want := idx.TagCounts("/backlog", task), map[string]int{"ui": 2, "api": 1}; !maps.Equal(got, want) { + t.Errorf("TagCounts = %v, want %v", got, want) + } + if got := idx.PropertyKeyCounts("/backlog", task); got["type"] != 2 || got["status"] != 2 { + t.Errorf("PropertyKeyCounts = %v, want 2 tasks' keys only", got) + } + // Negation and AND compose as they do on list. + notDone := []PropFilter{{Key: "type", Value: "task"}, {Key: "status", Value: "done", Negate: true}} + if got, want := idx.TagCounts("/backlog", notDone), map[string]int{"ui": 1}; !maps.Equal(got, want) { + t.Errorf("negated filter = %v, want %v", got, want) + } +} diff --git a/index/rename.go b/index/rename.go new file mode 100644 index 0000000..5ff1f4e --- /dev/null +++ b/index/rename.go @@ -0,0 +1,33 @@ +package index + +import ( + "os" + "time" +) + +// rename moves src onto dst, retrying only while the failure is contention: +// something else holding one of them open at that instant. +// +// Not every system lets a file be renamed or replaced while another handle is +// open on it. Where that is so, the failure is usually transient and not the +// user's doing, and a short wait clears it; contendedReplace says whether an +// error is that case, and is false on systems where a rename does not care who +// is reading. +// +// Any other error returns immediately, since retrying a real permission error +// would only delay reporting it. +// +// This is a mitigation rather than a cure: contention held longer than the +// window still fails. See the backlog task on replace semantics. +// +// This file is the only sanctioned home for os.Rename, and a test enforces +// that: every rename in the engine has to inherit this handling, or a caller +// that reached for the raw call would silently lose it. +func rename(src, dst string) error { + err := os.Rename(src, dst) + for i := 0; err != nil && contendedReplace(err) && i < 10; i++ { + time.Sleep(time.Duration(i+1) * 2 * time.Millisecond) // ~110ms over ten tries + err = os.Rename(src, dst) + } + return err +} diff --git a/index/rename_main.go b/index/rename_main.go new file mode 100644 index 0000000..7df66b5 --- /dev/null +++ b/index/rename_main.go @@ -0,0 +1,10 @@ +//go:build !windows + +// The ordinary case, and the one worth reading first: a rename moves a file +// whatever handles are open on it. rename_win.go is the exception. + +package index + +// contendedReplace is always false here: a failed rename is a real error, so +// retrying it would only delay the report. +func contendedReplace(error) bool { return false } diff --git a/index/rename_win.go b/index/rename_win.go new file mode 100644 index 0000000..355812e --- /dev/null +++ b/index/rename_win.go @@ -0,0 +1,31 @@ +//go:build windows + +package index + +import ( + "errors" + "syscall" +) + +// Go's syscall package exports ERROR_ACCESS_DENIED but not the sharing codes. +const ( + errSharingViolation = syscall.Errno(32) // ERROR_SHARING_VIOLATION + errLockViolation = syscall.Errno(33) // ERROR_LOCK_VIOLATION +) + +// contendedReplace reports whether a failed replace is worth retrying. +// +// Here a replace deletes the file it is replacing, and files opened for reading +// do not permit that, so the operation fails while anything else holds the entry +// open. The usual cause is transient and not the user's doing: a scanner or an +// indexer opening a file it just saw change. +// +// A handle held longer than the retry window is a different problem, and it +// surfaces as the error rather than being hidden. The durable fix is a replace +// with POSIX semantics, which the standard library does not use — see the +// backlog task on replace semantics. +func contendedReplace(err error) bool { + return errors.Is(err, syscall.ERROR_ACCESS_DENIED) || + errors.Is(err, errSharingViolation) || + errors.Is(err, errLockViolation) +} diff --git a/index/write.go b/index/write.go new file mode 100644 index 0000000..f638d31 --- /dev/null +++ b/index/write.go @@ -0,0 +1,462 @@ +package index + +import ( + "errors" + "fmt" + "maps" + "os" + "path/filepath" + "regexp" + "slices" + "strconv" + "strings" +) + +// ErrNoFrontmatter is returned when asked to set a field on a reserved file that +// carries none. `index.md` and `log.md` are defined as having no frontmatter, so +// giving them a block would break the format rather than annotate the file. +var ErrNoFrontmatter = errors.New("reserved file carries no frontmatter") + +// writeFile replaces a file's contents atomically: a temp file in the same +// directory, then a rename over the target. +// +// os.WriteFile opens with O_TRUNC, so the file is emptied before the new content +// lands, and anything reading it in that window sees an empty or partial entry. +// The window is small but real: an editor, an agent, or a watcher may be reading +// the same file, and a process killed mid-write leaves the entry truncated on +// disk. A rename within one filesystem is atomic, so a reader sees either the +// whole old file or the whole new one, and a crash leaves the original intact. +// +// Atomicity is **per file**. A command rewriting several of them can still be +// interrupted between two writes; that is a separate problem and this does not +// pretend to solve it. +// +// A rename breaks a hardlink, and deliberately so. Two names for one inode are +// two entries in the index, at two paths, so a relative link is normalized +// differently for each; sharing the inode meant the second write clobbered the +// first and left one of them pointing nowhere. Each name now gets the content +// that is correct where it sits. +// +// No fsync: the risk being closed here is a concurrent reader seeing a torn +// file, not power loss. Durability across a crash would need the temp file and +// its directory synced, which costs real time on every write. +func writeFile(abs string, content []byte) error { + // Resolve first: a rename replaces whatever name it is given, so writing to + // a symlinked entry would swap the link for a regular file and silently + // fork it into two diverging copies. Writing through to the target is what + // a plain write did. A broken or unresolvable link falls back to the path + // as given, which then fails the same way a plain write would. + if real, err := filepath.EvalSymlinks(abs); err == nil { + abs = real + } + dir := filepath.Dir(abs) + tmp, err := os.CreateTemp(dir, ".wiki-*.tmp") + if err != nil { + return err + } + name := tmp.Name() + defer os.Remove(name) // a no-op once the rename has succeeded + + perm := os.FileMode(0o644) + if fi, err := os.Stat(abs); err == nil { + perm = fi.Mode().Perm() // keep whatever the user set + } + if _, err := tmp.Write(content); err != nil { + tmp.Close() + return err + } + if err := tmp.Close(); err != nil { + return err + } + if err := os.Chmod(name, perm); err != nil { + return err + } + return rename(name, abs) +} + +// SetField writes key: value into the entry's frontmatter, preserving every +// other byte of the file, and refreshes the entry in the index. +func (e *Entry) SetField(key, value string) error { + return e.SetFields(map[string]any{key: value}) +} + +// SetFields writes several frontmatter fields in one pass. Values may be a +// string or a []string; anything else is an error naming the key and its type. +// +// map[string]any rather than map[string]string so this mirrors Frontmatter, +// which returns the same shape: a consumer can read the frontmatter, edit it, +// and write it back. With separate scalar and list calls, setting a status and a +// tag list together took two writes, which is exactly the half-updated entry one +// pass exists to prevent. Frontmatter is genuinely heterogeneous, so a +// map[string]string was never the honest type for it. +// +// One pass rather than a loop of SetField: two writes can leave an entry +// half-updated if the second fails, and a consumer changing two related fields +// (a status and an assignee, say) means them as one change. +// +// The edit is surgical. It finds the `---` fence, replaces exactly the lines +// belonging to each key, and leaves the rest of the file untouched — never +// parsing the frontmatter into a map and re-serializing it, which would silently +// drop everything the YAML subset does not model (nested maps, anchors, +// comments, quoting style). +func (e *Entry) SetFields(fields map[string]any) error { + if len(fields) == 0 { + return errors.New("no fields to set") + } + // Validate every key and value before writing anything, so a rejected batch + // applies nothing rather than the prefix that happened to be valid. + for key, v := range fields { + if err := validFieldKey(key); err != nil { + return err + } + switch v.(type) { + case string, []string: + default: + return fmt.Errorf("field %q: value must be a string or []string, got %T", key, v) + } + } + raw, err := e.Raw() + if err != nil { + return err + } + out := raw + // Sorted, so the same set of fields always produces the same file rather + // than one whose new-key order depends on map iteration. + for _, key := range slices.Sorted(maps.Keys(fields)) { + switch v := fields[key].(type) { + case string: + out, err = setFrontmatterValue(out, key, v, e.reserved()) + case []string: + out, err = setFrontmatterList(out, key, v, e.reserved()) + } + if err != nil { + return err + } + } + return e.commit(raw, out) +} + +// SetFieldList writes a list-valued frontmatter field: `tags`, `blockers`, and +// anything else the bundle spells as a list. +// +// Separate from SetField because a list is not a string that happens to contain +// brackets. Passing "[a, b]" to SetField writes `key: "[a, b]"` — correctly +// quoted for a scalar, and a one-element list when read back. +// +// The existing shape is kept: a key already written as a block list stays one, +// so the API does not reformat frontmatter it was only asked to change. A new +// key is written flow-style, which is what the scaffolds use. +func (e *Entry) SetFieldList(key string, values []string) error { + return e.SetFields(map[string]any{key: values}) +} + +// UnsetField removes a key from the entry's frontmatter, including a block list +// belonging to it. +func (e *Entry) UnsetField(key string) error { + if err := validFieldKey(key); err != nil { + return err + } + raw, err := e.Raw() + if err != nil { + return err + } + return e.commit(raw, unsetFrontmatterValue(raw, key)) +} + +// SetCheckbox sets the done state of the `- [ ]` item at the given line. +// +// Keyed by line because that is the only stable identity a checkbox has: its +// text may repeat within an entry, and parse.Checkbox already carries the line. +// Exactly one character changes. +func (e *Entry) SetCheckbox(line int, done bool) error { + raw, err := e.Raw() + if err != nil { + return err + } + lines := strings.Split(raw, "\n") + if line < 1 || line > len(lines) { + return fmt.Errorf("no line %d in %s", line, e.Path) + } + i := line - 1 + m := checkboxMark.FindStringSubmatchIndex(lines[i]) + if m == nil { + return fmt.Errorf("no checkbox on line %d of %s", line, e.Path) + } + mark := " " + if done { + mark = "x" + } + lines[i] = lines[i][:m[2]] + mark + lines[i][m[3]:] + return e.commit(raw, strings.Join(lines, "\n")) +} + +// commit writes the new content when it differs and refreshes the entry, so a +// caller holding an index never reads back what it just overwrote. +// +// A no-op write is skipped deliberately: it would touch the mtime and wake every +// file watcher for a change that did not happen. +func (e *Entry) commit(before, after string) error { + if after == before { + return nil + } + if err := writeFile(e.abs, []byte(after)); err != nil { + return err + } + return e.refresh() +} + +// refresh re-parses the entry in place, so a caller holding an index never reads +// back what it just overwrote. +// +// A full re-parse rather than patching the field that changed: setting a key +// that was absent inserts a line, and replacing a block list with a scalar +// removes several, either of which shifts every line number below it. Links, +// checkboxes, and headings all carry line numbers, so anything less would leave +// them quietly pointing at the wrong lines. +// +// The entry keeps its identity, so pointers the index and its callers already +// hold stay valid. Not refreshed: how *other* entries' wikilinks resolve, which +// depends on this entry's `aliases` and is settled by a Build pass. Editing +// aliases through this API therefore wants a rebuild. +func (e *Entry) refresh() error { + fresh, err := parseEntry(e.root, e.abs) + if err != nil { + return err + } + e.Type = fresh.Type + e.Links = fresh.Links + e.SelfAnchors = fresh.SelfAnchors + e.Outside = fresh.Outside + e.Checkboxes = fresh.Checkboxes + e.Headings = fresh.Headings + e.wikilinks = fresh.wikilinks + e.fm = fresh.fm + return nil +} + +// checkboxMark matches a GFM checkbox's mark, capturing the single character +// between the brackets so exactly that character can be replaced. +var checkboxMark = regexp.MustCompile(`^\s*[-*+] \[([ xX])\]`) + +// validFieldKey rejects keys the engine must not write: the reserved underscore +// namespace (`_path` is the index's, not the user's) and anything that would not +// survive as a YAML key. +func validFieldKey(key string) error { + switch { + case strings.TrimSpace(key) == "": + return errors.New("key must not be empty") + case strings.HasPrefix(key, "_"): + return fmt.Errorf("key %q is reserved (leading underscore)", key) + case strings.ContainsAny(key, ":\n\r"): + return fmt.Errorf("key %q must not contain ':' or a newline", key) + } + return nil +} + +// reserved reports whether the entry is one of the two filenames the format +// defines as carrying no frontmatter. +func (e *Entry) reserved() bool { + // The bundle-root index.md is the documented exception: it carries okf_version. + if e.Path == "/index.md" { + return false + } + base := e.base() + return base == "index.md" || base == "log.md" +} + +// formatField renders a `key: value` line, quoting only when the value needs it +// and otherwise matching whatever style the line already used. +func formatField(key, value, existing string) string { + if needsQuote(value) { + return fmt.Sprintf("%s: %q", key, value) + } + if e := strings.TrimSpace(existing); len(e) >= 2 && (e[0] == '\'' || e[0] == '"') && e[len(e)-1] == e[0] { + return fmt.Sprintf("%s: %c%s%c", key, e[0], value, e[0]) // keep the author's quoting + } + return key + ": " + value +} + +// needsQuote reports whether a value would not survive unquoted. Deliberately +// conservative: a wrongly-bare value corrupts the file, while a needlessly +// quoted one is merely noisy. +// +// Numbers are quoted, which matters more than it looks: a bare `0.1` is a float, +// so a version or an identifier written bare would stop being the string it was. +func needsQuote(v string) bool { + if v == "" || v != strings.TrimSpace(v) { + return true + } + if strings.ContainsAny(v, ":#[]{},&*!|>'\"%@`\n\r\t") { + return true + } + if _, err := strconv.ParseFloat(v, 64); err == nil { + return true + } + switch strings.ToLower(v) { + case "true", "false", "null", "yes", "no", "on", "off", "~": + return true + } + return v[0] == '-' || v[0] == '?' +} + +// setFrontmatterValue returns content with the frontmatter key set to value, +// changing that key's lines and nothing else. +// +// A file with no frontmatter gets a block, unless it is a reserved file, which by +// definition has none. Replacing a key that held a block list takes the list's +// items with it, or they would orphan into whatever key came next. +func setFrontmatterValue(content, key, value string, reserved bool) (string, error) { + lines := strings.Split(content, "\n") + open, closed := frontmatterFence(lines) + if open < 0 { + if reserved { + return "", ErrNoFrontmatter + } + return fmt.Sprintf("---\n%s\n---\n%s", formatField(key, value, ""), content), nil + } + if closed < 0 { + return "", errors.New("unterminated frontmatter") + } + + if i, end, existing, ok := findKey(lines, open, closed, key); ok { + out := append([]string{}, lines[:i]...) + out = append(out, formatField(key, value, existing)+carriage(lines[i])) + return strings.Join(append(out, lines[end+1:]...), "\n"), nil + } + // Absent: insert before the closing fence, so the new key joins the block + // rather than displacing anything already in it. + out := append([]string{}, lines[:closed]...) + out = append(out, formatField(key, value, "")+carriage(lines[closed])) + return strings.Join(append(out, lines[closed:]...), "\n"), nil +} + +// setFrontmatterList is setFrontmatterValue for a list value, emitting one or +// more lines in place of the key's existing ones. +func setFrontmatterList(content, key string, values []string, reserved bool) (string, error) { + lines := strings.Split(content, "\n") + open, closed := frontmatterFence(lines) + if open < 0 { + if reserved { + return "", ErrNoFrontmatter + } + return fmt.Sprintf("---\n%s\n---\n%s", flowList(key, values), content), nil + } + if closed < 0 { + return "", errors.New("unterminated frontmatter") + } + + if i, end, _, ok := findKey(lines, open, closed, key); ok { + // end > i means the key owned `- item` lines, so it was written as a + // block. Keep it that way. + var repl []string + if end > i && len(values) > 0 { + repl = blockList(key, values) + } else { + repl = []string{flowList(key, values)} + } + cr := carriage(lines[i]) + for j := range repl { + repl[j] += cr + } + out := append([]string{}, lines[:i]...) + out = append(out, repl...) + return strings.Join(append(out, lines[end+1:]...), "\n"), nil + } + out := append([]string{}, lines[:closed]...) + out = append(out, flowList(key, values)+carriage(lines[closed])) + return strings.Join(append(out, lines[closed:]...), "\n"), nil +} + +// flowList renders `key: [a, b]`. An empty list is written `key: []`, which is +// a declared-but-empty field, not the same thing as an absent one. +func flowList(key string, values []string) string { + items := make([]string, len(values)) + for i, v := range values { + items[i] = quoteItem(v) + } + return key + ": [" + strings.Join(items, ", ") + "]" +} + +// blockList renders a key over several lines, one `- item` each. +func blockList(key string, values []string) []string { + out := make([]string, 0, len(values)+1) + out = append(out, key+":") + for _, v := range values { + out = append(out, " - "+quoteItem(v)) + } + return out +} + +// quoteItem quotes a list item that would not survive bare. A comma or a +// bracket ends the item in flow style, so the rule is stricter than for a +// scalar, where those characters are harmless. +func quoteItem(v string) string { + if needsQuote(v) || strings.ContainsAny(v, ",[]") { + return fmt.Sprintf("%q", v) + } + return v +} + +// unsetFrontmatterValue removes a key and any block list belonging to it, +// returning content unchanged when the key is absent. +func unsetFrontmatterValue(content, key string) string { + lines := strings.Split(content, "\n") + open, closed := frontmatterFence(lines) + if open < 0 || closed < 0 { + return content + } + i, end, _, ok := findKey(lines, open, closed, key) + if !ok { + return content + } + return strings.Join(append(append([]string{}, lines[:i]...), lines[end+1:]...), "\n") +} + +// findKey locates a key's line within a frontmatter block, along with the last +// line belonging to it: a block list's `- item` lines belong to the key above +// them, so replacing or removing the key has to take them too. +func findKey(lines []string, open, closed int, key string) (start, end int, existing string, ok bool) { + for i := open + 1; i < closed; i++ { + line := strings.TrimRight(lines[i], "\r") + if line == "" || line[0] == ' ' || line[0] == '\t' || strings.HasPrefix(strings.TrimSpace(line), "- ") { + continue // a nested or block-list line belongs to the key above it + } + k, v, cut := strings.Cut(line, ":") + if !cut || strings.TrimSpace(k) != key { + continue + } + end = i + if strings.TrimSpace(v) == "" { // a block list follows + for j := i + 1; j < closed; j++ { + if strings.HasPrefix(strings.TrimSpace(strings.TrimRight(lines[j], "\r")), "- ") { + end = j + continue + } + break + } + } + return i, end, v, true + } + return 0, 0, "", false +} + +// frontmatterFence locates the opening and closing `---` lines, or -1 when the +// content has no frontmatter block at all. +func frontmatterFence(lines []string) (open, closed int) { + if len(lines) == 0 || strings.TrimRight(lines[0], "\r") != "---" { + return -1, -1 + } + for i := 1; i < len(lines); i++ { + if strings.TrimRight(lines[i], "\r") == "---" { + return 0, i + } + } + return 0, -1 +} + +func carriage(line string) string { + if strings.HasSuffix(line, "\r") { + return "\r" + } + return "" +} diff --git a/index/write_test.go b/index/write_test.go new file mode 100644 index 0000000..5edaaf1 --- /dev/null +++ b/index/write_test.go @@ -0,0 +1,643 @@ +package index + +import ( + "os" + "path/filepath" + "regexp" + "runtime" + "slices" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/agentic-wiki/wiki/bundle" +) + +// The contract the write API exists to keep: setting a field changes that +// field's lines and nothing else. Every frontmatter shape the YAML subset +// supports, and several it deliberately skips, must survive untouched — a +// parse-and-reserialize would quietly drop exactly what it does not model. +func TestSetFieldPreservesEverythingElse(t *testing.T) { + const doc = `--- +type: task # a trailing comment +title: "Quoted: with a colon" +status: todo +tags: [feature, ui] +blockers: + - /active/a.md + - /active/b.md +nested: + deep: + key: value +description: | + a block scalar + spanning lines +--- + +# Body + +Prose with a [link](./other.md). +` + idx := build(t, map[string]string{"index.md": "---\nokf_version: \"0.1\"\n---\n", "a.md": doc}) + e, err := idx.Resolve("/a.md") + if err != nil { + t.Fatal(err) + } + if err := e.SetField("status", "in-progress"); err != nil { + t.Fatal(err) + } + got, _ := os.ReadFile(filepath.Join(idx.Bundle.Dir, "a.md")) + want := strings.Replace(doc, "status: todo", "status: in-progress", 1) + if string(got) != want { + t.Errorf("bytes outside the target line changed.\n--- got ---\n%s\n--- want ---\n%s", got, want) + } + // The in-memory entry must not still report the old value. + if e.Field("status") != "in-progress" { + t.Errorf("entry not refreshed: status=%q", e.Field("status")) + } +} + +func TestSetFieldShapes(t *testing.T) { + tests := []struct{ name, doc, key, value, want string }{ + {"bare stays bare", "---\nstatus: todo\n---\nb\n", "status", "done", "---\nstatus: done\n---\nb\n"}, + {"author's quoting kept", "---\nstatus: \"todo\"\n---\nb\n", "status", "done", "---\nstatus: \"done\"\n---\nb\n"}, + {"a value needing quotes gets them", "---\ns: a\n---\nb\n", "s", "in: progress", "---\ns: \"in: progress\"\n---\nb\n"}, + // A bare `true` would parse as a boolean and a bare 0.1 as a float, so + // neither would still be the string that was set. + {"yaml keyword quoted", "---\nk: a\n---\nb\n", "k", "true", "---\nk: \"true\"\n---\nb\n"}, + {"number quoted", "---\nk: a\n---\nb\n", "k", "0.1", "---\nk: \"0.1\"\n---\nb\n"}, + {"absent key inserted", "---\ntype: task\n---\nb\n", "status", "todo", "---\ntype: task\nstatus: todo\n---\nb\n"}, + // Replacing a list with a scalar must take the list's items with it, or + // they orphan into the next key. + {"block list replaced whole", "---\ntags:\n - a\n - b\ntype: task\n---\nb\n", "tags", "c", "---\ntags: c\ntype: task\n---\nb\n"}, + {"flow list replaced", "---\ntags: [a, b]\ntype: task\n---\nb\n", "tags", "c", "---\ntags: c\ntype: task\n---\nb\n"}, + {"crlf survives", "---\r\ns: todo\r\n---\r\nb\r\n", "s", "done", "---\r\ns: done\r\n---\r\nb\r\n"}, + {"no frontmatter gets a block", "# Just a body\n", "s", "todo", "---\ns: todo\n---\n# Just a body\n"}, + // A key that is a prefix of another must not match it. + {"similar key untouched", "---\ns_note: keep\ns: todo\n---\nb\n", "s", "done", "---\ns_note: keep\ns: done\n---\nb\n"}, + // The body may contain something that looks like frontmatter. + {"only the leading block", "---\ns: todo\n---\nt\n---\ns: decoy\n---\n", "s", "done", "---\ns: done\n---\nt\n---\ns: decoy\n---\n"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got, err := setFrontmatterValue(tc.doc, tc.key, tc.value, false) + if err != nil { + t.Fatal(err) + } + if got != tc.want { + t.Errorf("got:\n%q\nwant:\n%q", got, tc.want) + } + }) + } +} + +// index.md and log.md carry no frontmatter by definition, so giving one a block +// would break the format. The bundle-root index.md is the documented exception. +func TestSetFieldAndReservedFiles(t *testing.T) { + if _, err := setFrontmatterValue("# Board\n", "status", "todo", true); err != ErrNoFrontmatter { + t.Errorf("err=%v, want ErrNoFrontmatter", err) + } + idx := build(t, map[string]string{ + "index.md": "---\nokf_version: \"0.1\"\n---\nhome\n", + "sub/index.md": "# a folder board\n", + }) + root, _ := idx.Resolve("/index.md") + if root.reserved() { + t.Error("the bundle-root index.md carries okf_version, so it is not frontmatter-less") + } + sub, _ := idx.Resolve("/sub/index.md") + if !sub.reserved() { + t.Error("a folder index.md carries no frontmatter") + } +} + +func TestSetFieldsIsOnePassAndDeterministic(t *testing.T) { + idx := build(t, map[string]string{ + "index.md": "---\nokf_version: \"0.1\"\n---\n", + "a.md": "---\ntype: task\nstatus: todo\nassignee: john\npriority: high\n---\nbody\n", + }) + e, _ := idx.Resolve("/a.md") + if err := e.SetFields(map[string]any{"status": "done", "assignee": "mary"}); err != nil { + t.Fatal(err) + } + got, _ := os.ReadFile(filepath.Join(idx.Bundle.Dir, "a.md")) + want := "---\ntype: task\nstatus: done\nassignee: mary\npriority: high\n---\nbody\n" + if string(got) != want { + t.Errorf("got:\n%q\nwant:\n%q", got, want) + } + + // Inserted keys land in a stable order, not one depending on map iteration. + var first string + for i := range 8 { + idx := build(t, map[string]string{ + "index.md": "---\nokf_version: \"0.1\"\n---\n", + "b.md": "---\ntype: task\n---\nbody\n", + }) + e, _ := idx.Resolve("/b.md") + if err := e.SetFields(map[string]any{"zeta": "1", "alpha": "2", "mid": "3"}); err != nil { + t.Fatal(err) + } + b, _ := os.ReadFile(filepath.Join(idx.Bundle.Dir, "b.md")) + if i == 0 { + first = string(b) + } else if string(b) != first { + t.Fatalf("run %d differs:\n%q\nvs\n%q", i, b, first) + } + } +} + +func TestSetFieldsRejectsReservedKeys(t *testing.T) { + idx := build(t, map[string]string{"index.md": "---\nokf_version: \"0.1\"\n---\n", "a.md": "---\ntype: task\n---\n"}) + e, _ := idx.Resolve("/a.md") + for _, key := range []string{"_path", "", "a:b"} { + if err := e.SetField(key, "v"); err == nil { + t.Errorf("SetField(%q) should be rejected", key) + } + } + // A rejected batch applies nothing at all. + if err := e.SetFields(map[string]any{"status": "done", "_path": "/x.md"}); err == nil { + t.Error("a reserved key should fail the whole batch") + } + if raw, _ := e.Raw(); strings.Contains(raw, "done") { + t.Error("a rejected batch must not partially apply") + } +} + +func TestUnsetField(t *testing.T) { + idx := build(t, map[string]string{ + "index.md": "---\nokf_version: \"0.1\"\n---\n", + "a.md": "---\ntype: task\ntags:\n - a\n - b\nstatus: todo\n---\nbody\n", + }) + e, _ := idx.Resolve("/a.md") + if err := e.UnsetField("tags"); err != nil { + t.Fatal(err) + } + got, _ := os.ReadFile(filepath.Join(idx.Bundle.Dir, "a.md")) + if want := "---\ntype: task\nstatus: todo\n---\nbody\n"; string(got) != want { + t.Errorf("got %q want %q", got, want) + } + before, _ := e.Raw() + if err := e.UnsetField("absent"); err != nil { + t.Fatal(err) + } + if after, _ := e.Raw(); after != before { + t.Error("removing an absent key changed the file") + } +} + +// The format's inline task mechanism, which had no write primitive at all. +func TestSetCheckbox(t *testing.T) { + const doc = "---\ntype: note\n---\n# Steps\n\n- [ ] first\n- [x] second\n- [ ] first\n\nprose\n" + idx := build(t, map[string]string{"index.md": "---\nokf_version: \"0.1\"\n---\n", "a.md": doc}) + e, _ := idx.Resolve("/a.md") + if len(e.Checkboxes) != 3 { + t.Fatalf("checkboxes=%d, want 3", len(e.Checkboxes)) + } + + // Keyed by line, so the duplicate text stays distinguishable. + if err := e.SetCheckbox(e.Checkboxes[2].Line, true); err != nil { + t.Fatal(err) + } + got, _ := os.ReadFile(filepath.Join(idx.Bundle.Dir, "a.md")) + want := "---\ntype: note\n---\n# Steps\n\n- [ ] first\n- [x] second\n- [x] first\n\nprose\n" + if string(got) != want { + t.Errorf("got:\n%q\nwant:\n%q", got, want) + } + if !e.Checkboxes[2].Done || e.Checkboxes[0].Done { + t.Error("entry not refreshed after toggling") + } + + // And back again. + if err := e.SetCheckbox(e.Checkboxes[1].Line, false); err != nil { + t.Fatal(err) + } + if raw, _ := e.Raw(); !strings.Contains(raw, "- [ ] second") { + t.Errorf("unticking failed:\n%s", raw) + } + // A line holding no checkbox is an error, not a silent no-op. + if err := e.SetCheckbox(1, true); err == nil { + t.Error("expected an error for a line holding no checkbox") + } + if err := e.SetCheckbox(9999, true); err == nil { + t.Error("expected an error for a line past the end") + } +} + +// A write must not touch the file when nothing changed, or every no-op would +// wake a watcher for a change that did not happen. +func TestNoOpWriteDoesNotTouchFile(t *testing.T) { + idx := build(t, map[string]string{"index.md": "---\nokf_version: \"0.1\"\n---\n", "a.md": "---\nstatus: todo\n---\nb\n"}) + e, _ := idx.Resolve("/a.md") + abs := filepath.Join(idx.Bundle.Dir, "a.md") + before, _ := os.Stat(abs) + if err := e.SetField("status", "todo"); err != nil { + t.Fatal(err) + } + after, _ := os.Stat(abs) + if !before.ModTime().Equal(after.ModTime()) { + t.Error("a no-op write touched the file") + } +} + +// Writes go through a temp file and a rename, so a reader never sees a torn +// file, and the entry's permissions survive. +func TestWriteIsAtomicAndKeepsMode(t *testing.T) { + dir := t.TempDir() + abs := filepath.Join(dir, "a.md") + if err := os.WriteFile(abs, []byte("old\n"), 0o600); err != nil { + t.Fatal(err) + } + if err := writeFile(abs, []byte("new\n")); err != nil { + t.Fatal(err) + } + if got, _ := os.ReadFile(abs); string(got) != "new\n" { + t.Errorf("content=%q", got) + } + // Not every system models POSIX permission bits; where it does not, Chmod + // only toggles read-only and Perm() reports a fixed value, so asserting 0600 + // would be testing the platform rather than the code. + if runtime.GOOS != "windows" { + fi, _ := os.Stat(abs) + if fi.Mode().Perm() != 0o600 { + t.Errorf("mode=%v, want 0600: a rename must not reset the user's permissions", fi.Mode().Perm()) + } + } + ents, _ := os.ReadDir(dir) + for _, en := range ents { + if strings.HasPrefix(en.Name(), ".wiki-") { + t.Errorf("temp file left behind: %s", en.Name()) + } + } +} + +// Atomicity has to hold for the commands, not just for the helper. This drives a +// real rewrite while a reader watches, and counts any read that matches neither +// the before nor the after state. With os.WriteFile in place of writeFile it +// reports hundreds of torn reads; it is the regression guard for that swap. +func TestCommandRewritesAreAtomic(t *testing.T) { + // The risk measured here is a torn read, which only exists where a replace + // can happen while a file is open. Where it cannot, the write fails instead — + // that is what rename's retry is for — and a reader looping this tightly + // would hold the target open throughout, measuring contention rather than + // atomicity. + if runtime.GOOS == "windows" { + t.Skip("replace-while-open is refused here rather than tearing; see rename()") + } + filler := strings.Repeat("a line of the entry body\n", 3000) + before := "---\ntype: note\n---\n[x](/index.md)\n" + filler // absolute: tidy rewrites it + after := "---\ntype: note\n---\n[x](./index.md)\n" + filler // relative: the canonical form + + dir := t.TempDir() + os.WriteFile(filepath.Join(dir, "wiki.toml"), []byte("spec = \"0.1\"\n"), 0o644) + os.WriteFile(filepath.Join(dir, "index.md"), []byte("---\nokf_version: \"0.1\"\n---\nhome\n"), 0o644) + target := filepath.Join(dir, "a.md") + + // Re-dirtying must not itself be observable as a torn write, or the test + // would be measuring its own setup. + reset := func() { + if err := os.WriteFile(target+".reset", []byte(before), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Rename(target+".reset", target); err != nil { + t.Fatal(err) + } + } + reset() + + var torn, reads int64 + stop := make(chan struct{}) + done := make(chan struct{}) + go func() { + defer close(done) + for { + select { + case <-stop: + return + default: + if b, err := os.ReadFile(target); err == nil { + atomic.AddInt64(&reads, 1) + if s := string(b); s != before && s != after { + atomic.AddInt64(&torn, 1) + } + } + } + } + }() + + rewrites := 0 + for range 50 { + reset() + b, err := bundle.Discover(dir) + if err != nil { + t.Fatal(err) + } + idx, err := Build(b) + if err != nil { + t.Fatal(err) + } + fixes, err := idx.NormalizeLinks(true) + if err != nil { + t.Fatal(err) + } + rewrites += len(fixes) + } + close(stop) + <-done + + if rewrites == 0 { + t.Fatal("nothing was rewritten, so this proved nothing") + } + if torn > 0 { + t.Errorf("%d of %d reads saw a partial file: writes are not atomic", torn, reads) + } +} + +// A rename replaces the name it is given, so an unresolved write would swap a +// symlinked entry for a regular file and fork it into two diverging copies. +// Writing through to the target is what a plain write did. +func TestWriteFollowsSymlinks(t *testing.T) { + dir := t.TempDir() + real := filepath.Join(dir, "real.md") + link := filepath.Join(dir, "link.md") + if err := os.WriteFile(real, []byte("before\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Symlink("real.md", link); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + if err := writeFile(link, []byte("after\n")); err != nil { + t.Fatal(err) + } + if fi, err := os.Lstat(link); err != nil || fi.Mode()&os.ModeSymlink == 0 { + t.Error("the symlink was replaced by a regular file") + } + if b, _ := os.ReadFile(real); string(b) != "after\n" { + t.Errorf("target not updated: %q", b) + } +} + +// writeFile is an unexported helper, so nothing stops the next mutating command +// from reaching for os.WriteFile and quietly reintroducing torn writes — which +// is how the engine ended up with four of them. This fails the build instead of +// relying on anyone remembering. +// +// Scoped to this package because it is the only one that rewrites entries a +// reader may hold open. internal/scaffold creates a bundle that does not exist +// yet, so it has no such reader. +func TestNoDirectWritesOutsideWriteFile(t *testing.T) { + // Each rule names the one function that owns it and the file that function + // lives in. os.CreateTemp is writeFile's own tool, so the pattern matches + // the bare os.Create( only. + rules := []struct { + banned *regexp.Regexp + homeIn string // the one file allowed to use it; empty means nowhere + use string + because string + }{ + { + // No home: writeFile builds on os.CreateTemp, which these patterns + // deliberately do not match, so nothing in the package needs these. + regexp.MustCompile(`os\.(WriteFile|Create|OpenFile|Truncate)\(`), + "", "writeFile", + "it renames a temp file into place, so a reader never sees a partial entry " + + "and a process killed mid-write cannot truncate one", + }, + { + // The reason this needs enforcing: os.Rename works on every developer + // machine that does not refuse to touch open files, so a raw call + // passes review and CI, and only fails for users elsewhere. + regexp.MustCompile(`os\.Rename\(`), + "rename.go", "rename", + "it retries while the failure is contention, which a raw os.Rename " + + "silently drops on systems that refuse to move an open file", + }, + } + + entries, err := os.ReadDir(".") + if err != nil { + t.Fatal(err) + } + checked, enforced := 0, map[string]bool{} + for _, f := range entries { + name := f.Name() + if !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + checked++ + src, err := os.ReadFile(name) + if err != nil { + t.Fatal(err) + } + for i, line := range strings.Split(string(src), "\n") { + for _, r := range rules { + m := r.banned.FindString(line) + if m == "" { + continue + } + if r.homeIn != "" && name == r.homeIn { + enforced[r.homeIn] = true // the sanctioned use + continue + } + t.Errorf("%s:%d uses %s — go through %s instead, because %s", + name, i+1, m, r.use, r.because) + } + } + } + if checked == 0 { + t.Fatal("no source files scanned, so this proved nothing") + } + // A rule with a sanctioned home must find the call there. If it moves or goes + // away, the rule is guarding nothing and would pass silently forever. + for _, r := range rules { + if r.homeIn != "" && !enforced[r.homeIn] { + t.Errorf("%s no longer contains the call %s guards; the rule is dead", r.homeIn, r.use) + } + } +} + +// A list is not a string that happens to contain brackets: SetField would quote +// it into a scalar, so a consumer had no way to write `tags` or `blockers` — the +// two list fields the format uses most. +func TestSetFieldList(t *testing.T) { + tests := []struct{ name, doc, key, want string }{ + { + "flow stays flow", + "---\ntags: [ui, api]\ntype: task\n---\nb\n", "tags", + "---\ntags: [ui, api, new]\ntype: task\n---\nb\n", + }, + { + // The API was asked to change a value, not to reformat the file. + "block stays block", + "---\nblockers:\n - /x.md\n - /y.md\ntype: task\n---\nb\n", "blockers", + "---\nblockers:\n - ui\n - api\n - new\ntype: task\n---\nb\n", + }, + { + "absent key is inserted flow-style", + "---\ntype: task\n---\nb\n", "tags", + "---\ntype: task\ntags: [ui, api, new]\n---\nb\n", + }, + { + // Replacing a scalar with a list must not leave the old quoting. + "scalar becomes a list", + "---\ntags: \"just one\"\ntype: task\n---\nb\n", "tags", + "---\ntags: [ui, api, new]\ntype: task\n---\nb\n", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got, err := setFrontmatterList(tc.doc, tc.key, []string{"ui", "api", "new"}, false) + if err != nil { + t.Fatal(err) + } + if got != tc.want { + t.Errorf("got:\n%q\nwant:\n%q", got, tc.want) + } + }) + } +} + +// Items are quoted more strictly than scalars: a comma or a bracket would end +// the item in flow style, where in a scalar they are harmless. +func TestSetFieldListQuoting(t *testing.T) { + for _, tc := range []struct { + name string + in []string + want string + }{ + {"plain items bare", []string{"ui", "api"}, "---\nk: [ui, api]\n---\n"}, + {"comma forces quotes", []string{"a,b", "c"}, "---\nk: [\"a,b\", c]\n---\n"}, + {"brackets force quotes", []string{"[x]"}, "---\nk: [\"[x]\"]\n---\n"}, + {"colon forces quotes", []string{"a: b"}, "---\nk: [\"a: b\"]\n---\n"}, + {"number forces quotes", []string{"0.1"}, "---\nk: [\"0.1\"]\n---\n"}, + {"empty list is explicit", nil, "---\nk: []\n---\n"}, + {"paths stay bare", []string{"/a/b.md"}, "---\nk: [/a/b.md]\n---\n"}, + } { + t.Run(tc.name, func(t *testing.T) { + got, err := setFrontmatterList("---\nk: old\n---\n", "k", tc.in, false) + if err != nil { + t.Fatal(err) + } + if got != tc.want { + t.Errorf("got %q want %q", got, tc.want) + } + }) + } +} + +// The written form must survive a round trip, or the API produces frontmatter +// its own parser misreads. +func TestSetFieldListRoundTrips(t *testing.T) { + for _, values := range [][]string{ + {"ui", "api"}, + {"a,b", "c: d", "0.1", "[x]"}, + {"/epics/x.md", "/epics/y.md"}, + } { + idx := build(t, map[string]string{ + "index.md": "---\nokf_version: \"0.1\"\n---\n", + "a.md": "---\ntype: task\ntags: [old]\n---\nbody\n", + }) + e, _ := idx.Resolve("/a.md") + if err := e.SetFieldList("tags", values); err != nil { + t.Fatal(err) + } + if got := e.FieldList("tags"); !slices.Equal(got, values) { + raw, _ := e.Raw() + t.Errorf("wrote %q, read back %#v, want %#v", raw, got, values) + } + } +} + +// The reason SetFields takes map[string]any: a scalar and a list set together +// are one write. With separate calls they were two, and a failure between them +// left the entry half-updated — the exact thing one pass exists to prevent. +func TestSetFieldsMixesScalarsAndLists(t *testing.T) { + idx := build(t, map[string]string{ + "index.md": "---\nokf_version: \"0.1\"\n---\n", + "a.md": "---\ntype: task\nstatus: todo\ntags: [old]\nblockers:\n - /x.md\n---\nbody\n", + }) + e, _ := idx.Resolve("/a.md") + if err := e.SetFields(map[string]any{ + "status": "done", + "tags": []string{"ui", "api"}, + "blockers": []string{"/y.md", "/z.md"}, + "assignee": "mary", + }); err != nil { + t.Fatal(err) + } + got, _ := os.ReadFile(filepath.Join(idx.Bundle.Dir, "a.md")) + want := "---\ntype: task\nstatus: done\ntags: [ui, api]\nblockers:\n - /y.md\n - /z.md\nassignee: mary\n---\nbody\n" + if string(got) != want { + t.Errorf("got:\n%q\nwant:\n%q", got, want) + } + // Everything reads back as the type it was written as. + if e.Field("status") != "done" || e.Field("assignee") != "mary" { + t.Errorf("scalars: status=%q assignee=%q", e.Field("status"), e.Field("assignee")) + } + if !slices.Equal(e.FieldList("tags"), []string{"ui", "api"}) { + t.Errorf("tags=%#v", e.FieldList("tags")) + } + if !slices.Equal(e.FieldList("blockers"), []string{"/y.md", "/z.md"}) { + t.Errorf("blockers=%#v", e.FieldList("blockers")) + } +} + +// map[string]any moves the type check to runtime, so it has to be a real check +// with a message that names what went wrong — and it must reject before writing. +func TestSetFieldsRejectsUnsupportedTypes(t *testing.T) { + idx := build(t, map[string]string{ + "index.md": "---\nokf_version: \"0.1\"\n---\n", + "a.md": "---\ntype: task\nstatus: todo\n---\nbody\n", + }) + e, _ := idx.Resolve("/a.md") + before, _ := e.Raw() + + for _, v := range []any{42, true, 1.5, nil, []int{1}, map[string]string{"a": "b"}} { + err := e.SetFields(map[string]any{"status": "done", "bad": v}) + if err == nil { + t.Errorf("value %#v (%T) should be rejected", v, v) + continue + } + if !strings.Contains(err.Error(), "bad") { + t.Errorf("error should name the offending key, got %v", err) + } + } + // A rejected batch writes nothing, including the valid keys beside it. + if after, _ := e.Raw(); after != before { + t.Errorf("a rejected batch modified the file:\n%q", after) + } + // Round-tripping Frontmatter through SetFields is the point of the shape. + fm := e.Frontmatter() + fm["status"] = "in-progress" + if err := e.SetFields(fm); err != nil { + t.Fatalf("Frontmatter() output should be accepted by SetFields: %v", err) + } + if e.Field("status") != "in-progress" { + t.Errorf("status=%q", e.Field("status")) + } +} + +// The retry exists for systems that refuse to replace a file another handle has +// open. It cannot be provoked where a replace does not care, so what is pinned +// here is the contract: it succeeds normally, and a non-contention error is +// returned at once rather than retried. +func TestRenameRetriesThenGivesUp(t *testing.T) { + dir := t.TempDir() + src := filepath.Join(dir, "src") + if err := os.WriteFile(src, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + if err := rename(src, filepath.Join(dir, "dst")); err != nil { + t.Fatalf("a plain rename should succeed on the first attempt: %v", err) + } + + // An impossible destination: the error is returned, and bounded. + start := time.Now() + err := rename(filepath.Join(dir, "dst"), filepath.Join(dir, "no-such-dir", "x")) + if err == nil { + t.Fatal("renaming into a missing directory should fail") + } + if elapsed := time.Since(start); elapsed > 2*time.Second { + t.Errorf("gave up after %v; the retry must stay bounded", elapsed) + } +} diff --git a/internal/bundle/bundle.go b/internal/bundle/bundle.go deleted file mode 100644 index 08970d5..0000000 --- a/internal/bundle/bundle.go +++ /dev/null @@ -1,119 +0,0 @@ -// Package bundle locates an agentic-wiki bundle and reads its config. -// -// A bundle is a directory containing wiki.toml; the markdown content lives -// directly in that directory, with .wiki/ as a hidden, disposable cache. -package bundle - -import ( - "errors" - "os" - "path/filepath" - "slices" - "strings" - - "github.com/agentic-wiki/wiki/internal/parse" -) - -// Bundle is a located agentic-wiki bundle: a directory containing wiki.toml, -// with the markdown content living directly inside it. -type Bundle struct { - Dir string // the bundle directory (holds wiki.toml); also the content root - Spec string // spec version the bundle conforms to (from wiki.toml) - Types []string // content types declared in wiki.toml - Ignore []string // paths (relative to Dir) wiki disregards: an in-bundle path is not indexed (not an entry); an out-of-bundle path silences that link's advisory - // IgnoreOrphans lists paths (relative to Dir) whose entries stay indexed but are - // not reported by `wiki orphans`: a directory subtree or an exact path. - IgnoreOrphans []string - // Unknown holds wiki.toml keys the tool does not recognize (a typo, or a - // renamed field). They are inert; `check` surfaces them so they aren't - // silently ignored. - Unknown []string -} - -// ErrNotFound is returned when no wiki.toml is found walking up from start. -var ErrNotFound = errors.New("no wiki.toml found (not inside a wiki bundle)") - -// Discover walks up from start until it finds a directory containing wiki.toml. -func Discover(start string) (*Bundle, error) { - dir, err := filepath.Abs(start) - if err != nil { - return nil, err - } - for { - cfg := filepath.Join(dir, "wiki.toml") - if fi, err := os.Stat(cfg); err == nil && !fi.IsDir() { - return load(dir, cfg) - } - parent := filepath.Dir(dir) - if parent == dir { - return nil, ErrNotFound - } - dir = parent - } -} - -func load(root, cfg string) (*Bundle, error) { - data, err := os.ReadFile(cfg) - if err != nil { - return nil, err - } - spec, types, ignore, ignoreOrphans, unknown := parseConfig(string(data)) - return &Bundle{ - Dir: root, - Spec: spec, - Types: types, - Ignore: ignore, - IgnoreOrphans: ignoreOrphans, - Unknown: unknown, - }, nil -} - -// KnownType reports whether t is an allowed content type. A declared vocabulary -// (`types` in wiki.toml) is opt-in: when none is declared (empty list), every -// type is allowed and this returns true; when one is declared, t must be in it. -func (b *Bundle) KnownType(t string) bool { - return len(b.Types) == 0 || slices.Contains(b.Types, t) -} - -// okfVersions maps an agentic-wiki spec version to the OKF version it embeds. -// Our spec is its own thing; OKF is one ingredient, declared to OKF consumers -// via okf_version in the bundle-root index.md. A future spec may embed a -// different OKF version, or none. -var okfVersionMap = map[string]string{"0.1": "0.1"} - -// OKFVersion returns the OKF version this bundle's spec embeds, and whether the -// spec embeds OKF at all (false for an unknown or non-OKF spec). -func (b *Bundle) OKFVersion() (string, bool) { - v, ok := okfVersionMap[b.Spec] - return v, ok -} - -// parseConfig reads the tiny wiki.toml we define: a `spec` string, and `types`, -// `ignore`, and `ignore_orphans` arrays, each on one line. Deliberately minimal -// (no TOML dependency). Any other key is collected in unknown so `check` can flag -// it rather than let a typo or a renamed field pass unnoticed. -func parseConfig(s string) (spec string, types, ignore, ignoreOrphans, unknown []string) { - for line := range strings.SplitSeq(s, "\n") { - line = strings.TrimSpace(line) - if line == "" || strings.HasPrefix(line, "#") { - continue - } - key, val, ok := strings.Cut(line, "=") - if !ok { - continue - } - switch k := strings.TrimSpace(key); k { - case "spec": - spec = parse.Unquote(val) - case "types": - types = parse.List(val) - case "ignore": - ignore = parse.List(val) - case "ignore_orphans": - ignoreOrphans = parse.List(val) - default: - unknown = append(unknown, k) - } - } - return spec, types, ignore, ignoreOrphans, unknown -} diff --git a/internal/bundle/bundle_test.go b/internal/bundle/bundle_test.go deleted file mode 100644 index cf8ff2c..0000000 --- a/internal/bundle/bundle_test.go +++ /dev/null @@ -1,135 +0,0 @@ -package bundle - -import ( - "os" - "path/filepath" - "reflect" - "testing" -) - -func TestOKFVersion(t *testing.T) { - if v, ok := (&Bundle{Spec: "0.1"}).OKFVersion(); !ok || v != "0.1" { - t.Errorf("OKFVersion(0.1) = %q, %v; want 0.1, true", v, ok) - } - if _, ok := (&Bundle{Spec: "9.9"}).OKFVersion(); ok { - t.Errorf("unknown spec should not embed OKF") - } -} - -func TestParseConfig(t *testing.T) { - spec, types, _, _, _ := parseConfig("spec = \"0.1\"\ntypes = [\"note\", \"concept\"]\n# a comment\n") - if spec != "0.1" { - t.Errorf("spec=%q", spec) - } - if !reflect.DeepEqual(types, []string{"note", "concept"}) { - t.Errorf("types=%#v", types) - } -} - -func TestParseConfigMessy(t *testing.T) { - // Spaces, bare + quoted tokens; internal space preserved; no spec line. - if _, types, _, _, _ := parseConfig("types = [ \"a\" , b , \"c d\" ]\n"); !reflect.DeepEqual(types, []string{"a", "b", "c d"}) { - t.Errorf("types=%#v", types) - } - if spec, empty, _, _, _ := parseConfig("types = []\n"); spec != "" || empty != nil { - t.Errorf("spec=%q types=%#v, want empty", spec, empty) - } -} - -func TestParseConfigIgnore(t *testing.T) { - _, _, ignore, orphans, _ := parseConfig("spec=\"0.1\"\ntypes=[\"note\"]\nignore=[\"AGENTS.md\", \"../PRD.md\"]\nignore_orphans=[\"backlog/**\"]\n") - if !reflect.DeepEqual(ignore, []string{"AGENTS.md", "../PRD.md"}) { - t.Errorf("ignore=%#v", ignore) - } - if !reflect.DeepEqual(orphans, []string{"backlog/**"}) { - t.Errorf("ignore_orphans=%#v", orphans) - } -} - -func TestParseConfigUnknownKeys(t *testing.T) { - // A renamed field (the old `skip`) or a typo is inert; parseConfig collects it - // so `check` can flag it rather than let it pass silently. - _, _, _, _, unknown := parseConfig("spec=\"0.1\"\nskip=[\"AGENTS.md\"]\ntpyes=[\"note\"]\n") - if !reflect.DeepEqual(unknown, []string{"skip", "tpyes"}) { - t.Errorf("unknown=%#v, want [skip tpyes]", unknown) - } - // A clean config yields no unknown keys. - if _, _, _, _, u := parseConfig("spec=\"0.1\"\ntypes=[\"note\"]\nignore=[]\nignore_orphans=[]\n"); u != nil { - t.Errorf("unknown=%#v, want nil", u) - } -} - -func TestKnownType(t *testing.T) { - b := &Bundle{Types: []string{"note", "concept"}} - for _, ty := range []string{"note", "concept"} { - if !b.KnownType(ty) { - t.Errorf("%q should be known", ty) - } - } - // With a declared vocabulary, an undeclared type is unknown. - for _, ty := range []string{"index", "log", "bogus"} { - if b.KnownType(ty) { - t.Errorf("%q is not a declared content type", ty) - } - } - // No declared vocabulary (opt-in): every type is allowed. - none := &Bundle{} - for _, ty := range []string{"note", "anything", "made-up"} { - if !none.KnownType(ty) { - t.Errorf("%q should be allowed when no vocabulary is declared", ty) - } - } -} - -func TestDiscoverWalksUp(t *testing.T) { - root := t.TempDir() - writeTOML(t, root) - deep := filepath.Join(root, "a", "b") // content lives at the bundle root, no wiki/ subfolder - if err := os.MkdirAll(deep, 0o755); err != nil { - t.Fatal(err) - } - p := mustDiscover(t, deep) - if realpath(t, p.Dir) != realpath(t, root) { - t.Errorf("Dir=%q want %q", p.Dir, root) - } - if p.Spec != "0.1" { - t.Errorf("spec=%q", p.Spec) - } -} - -func TestDiscoverExactDir(t *testing.T) { - root := t.TempDir() - writeTOML(t, root) - p := mustDiscover(t, root) // wiki.toml is right here, no walking - if realpath(t, p.Dir) != realpath(t, root) { - t.Errorf("Dir=%q want %q", p.Dir, root) - } -} - -func TestDiscoverNotFound(t *testing.T) { - if _, err := Discover(t.TempDir()); err != ErrNotFound { - t.Errorf("err=%v want ErrNotFound", err) - } -} - -func writeTOML(t *testing.T, dir string) { - t.Helper() - if err := os.WriteFile(filepath.Join(dir, "wiki.toml"), []byte("spec=\"0.1\"\ntypes=[\"note\"]\n"), 0o644); err != nil { - t.Fatal(err) - } -} - -func mustDiscover(t *testing.T, dir string) *Bundle { - t.Helper() - p, err := Discover(dir) - if err != nil { - t.Fatal(err) - } - return p -} - -func realpath(t *testing.T, p string) string { - t.Helper() - r, _ := filepath.EvalSymlinks(p) - return r -} diff --git a/internal/scaffold/files/AGENTS.md b/internal/scaffold/files/AGENTS.md index feec491..4be7783 100644 --- a/internal/scaffold/files/AGENTS.md +++ b/internal/scaffold/files/AGENTS.md @@ -102,7 +102,7 @@ Knowledge often arrives rough and matures in place. You write the file; `wiki` o ```sh wiki move --dry-run /a.md /archive/a.md # preview the link rewrites wiki move /a.md /archive/a.md # relocate + rewrite every inbound link in one pass -wiki tidy # preview canonicalization; `tidy --all` applies (links→absolute, names→slugs) +wiki tidy # preview canonicalization; `tidy --all` applies (links→relative, names→slugs) ``` Prefer `wiki move` over read-delete-rewrite by hand: hand-moving strands every backlink. @@ -143,6 +143,7 @@ Git is optional but highly recommended: it is the undo for a base an agent edits - Every entry has a `type` (reserved `index.md`/`log.md`); slug filenames (lowercase, hyphenated, no spaces); shallow folders (2–3 levels). - Root-absolute links, no wikilinks: `wiki check` flags any `[[wikilink]]` (they're resolved into the graph for compatibility, but aren't part of the format), so rewrite them as standard Markdown links (or run `wiki tidy --wikilinks` to convert them). -- Every command takes `--format text|json|csv|tsv` (`json`/`csv`/`tsv` for structured output you can pipe). `list --format json` carries each entry's full frontmatter (every field, not just the shown columns), so `wiki list --where type=task --format json | jq …` is the reporting surface for rollups the CLI does not compute itself. text and csv/tsv show only the canonical columns, the entry's path and `type` (the two fields every entry is guaranteed to have); everything else (title, tags, status, …) is json-only. In json the entry's own file path is under the reserved key `_path` (leading underscore; the basename is just `basename(_path)`), so a frontmatter field named `name:` or `path:` round-trips untouched; every other key is your frontmatter verbatim. +- Every command that reports results takes `--format text|json|csv|tsv` (`json`/`csv`/`tsv` for structured output you can pipe; `version` prints a bare string). `list --format json` carries each entry's full frontmatter (every field, not just the shown columns), so `wiki list --where type=task --format json | jq …` is the reporting surface for rollups the CLI does not compute itself. text and csv/tsv show only the canonical columns, the entry's path and `type` (the two fields every entry is guaranteed to have); everything else (title, tags, status, …) is json-only. +- json keys follow the shape of what is being reported. `list` and `orphans` merge your frontmatter into each row, so the entry's own path is under the **reserved key `_path`** (leading underscore; the basename is just `basename(_path)`) and a frontmatter field named `name:` or `path:` round-trips untouched — every other key is your frontmatter verbatim. Rows *about* an entry (`check`, `checkboxes`) name it `entry`. Link rows (`links`, `backlinks`, `unresolved`) name their two ends `from` and `to`. - Exit codes: `0` ok (enumerations return `0` even when empty), `1` no match (`search`/`table`) or `check` errors, `2` a real error. - `wiki` not installed? `brew install agentic-wiki/tap/wiki`, or see the [wiki repo](https://github.com/agentic-wiki/wiki). diff --git a/internal/scaffold/files/workflows/project-backlog/WORKFLOW.md b/internal/scaffold/files/workflows/project-backlog/WORKFLOW.md index a9fba88..8085fe6 100644 --- a/internal/scaffold/files/workflows/project-backlog/WORKFLOW.md +++ b/internal/scaffold/files/workflows/project-backlog/WORKFLOW.md @@ -101,7 +101,20 @@ A **milestone** (`type: milestone`) is a release or target; an epic (or a lone t ## Dependencies -Model *blocks* / *blocked-by* as real Markdown links between issues, not prose like `Depends on: tz-bug`: only a link is an edge the graph can follow. `wiki backlinks /active/tz-bug.md` then shows what finishing it would unblock, and `wiki unresolved` surfaces links to issues or specs not written yet: candidate prerequisites. +A dependency is *blocks* / *blocked-by* between two issues, and never prose like `Depends on: tz-bug`, which nothing can follow. Two recipes, the same trade-off as epics above; **pick one and keep it**, so every issue records a blocker the same way. + +- **Body link.** The blocked issue links its prerequisite in the body: `Blocked by [Timezone bug](./tz-bug.md)`. A real graph edge, so `wiki backlinks /active/tz-bug.md` shows what finishing it would unblock, `wiki move` maintains it with no flag, and it keeps the prerequisite off `wiki orphans`. What it does not carry is *which kind* of relation it is (a link is just a link), so "what is blocked right now" is something you read, not something you query. +- **`blockers:` field.** The blocked issue lists its prerequisites as paths: + + ```yaml + blockers: [/active/tz-bug.md, /backlog/dedup.md] + ``` + + Explicit, typed, and filterable: `wiki list --where blockers=/active/tz-bug.md` is every issue waiting on that one. Reach for it when something other than a human reads the backlog (a board UI, a scheduling script), since a machine needs the relation named, not inferred from where a link sits. The field-not-an-edge caveats apply: `backlinks` will not follow it, the prerequisite still needs a body link from somewhere to stay off `orphans`, and a relocation needs `wiki move … --include-frontmatter`. + + **Write these root-absolute.** A relative `./tz-bug.md` resolves fine and `move` maintains it, but `--where` is exact string equality, so the same target spelled `./tz-bug.md` from one folder and `../active/tz-bug.md` from another cannot both be found by one query. Root-absolute is one spelling per target, which is the whole point of using a field instead of a link. (`move --include-frontmatter` normalizes to it.) + +Either way `wiki unresolved` surfaces links to issues or specs not yet written: candidate prerequisites. And note what neither recipe does: nothing *enforces* a blocker. An issue can be moved to `in-progress` with its blockers open, and the backlog will simply say so. ## Multiple teams diff --git a/internal/scaffold/scaffold_test.go b/internal/scaffold/scaffold_test.go index 8e7f855..448459b 100644 --- a/internal/scaffold/scaffold_test.go +++ b/internal/scaffold/scaffold_test.go @@ -8,7 +8,7 @@ import ( "strings" "testing" - "github.com/agentic-wiki/wiki/internal/parse" + "github.com/agentic-wiki/wiki/parse" ) func TestWrite(t *testing.T) { diff --git a/internal/wikilink/wikilink.go b/internal/wikilink/wikilink.go index 5883e61..104075c 100644 --- a/internal/wikilink/wikilink.go +++ b/internal/wikilink/wikilink.go @@ -3,8 +3,8 @@ // package only recognizes them so the graph stays correct and `tidy` can // convert them to canonical markdown, resolving the way Obsidian would, with // zero promises. It is deliberately quarantined here so the standard markdown -// link parsing (internal/parse) and the graph (internal/index) stay clean and -// only call in. Ported from the obsy predecessor; pure and dependency-free. +// link parsing (parse) and the graph (index) stay clean and only call in. +// Ported from the obsy predecessor; pure and dependency-free. // // All paths are wiki bundle paths: root-absolute and slash-separated (e.g. // /finance/income.md), so this package uses the slash-only `path` package, never diff --git a/internal/wikilink/wikilink_test.go b/internal/wikilink/wikilink_test.go index c586ef3..191f03c 100644 --- a/internal/wikilink/wikilink_test.go +++ b/internal/wikilink/wikilink_test.go @@ -73,7 +73,7 @@ func TestResolve(t *testing.T) { "/sub/note-a.md", // same basename as /note-a.md, deeper "/sub/child.md", "/sub/deep/other.md", - "/sub/dup.md", // equal-depth pair with /team/dup.md, for the same-folder tiebreak + "/sub/dup.md", // equal-depth pair with /team/dup.md, for the same-folder tiebreak "/team/dup.md", } none := map[string]string{} diff --git a/internal/parse/parse.go b/parse/parse.go similarity index 92% rename from internal/parse/parse.go rename to parse/parse.go index 2644227..156c2b9 100644 --- a/internal/parse/parse.go +++ b/parse/parse.go @@ -81,7 +81,7 @@ func parseYAMLSubset(block string, fm map[string]any) { val = stripComment(strings.TrimSpace(val)) switch { case strings.HasPrefix(val, "["): - fm[key] = List(val) + fm[key] = list(val) case strings.HasPrefix(val, "|") || strings.HasPrefix(val, ">"): // block scalar: gather the following more-indented lines var blk []string @@ -140,18 +140,45 @@ func stripComment(s string) string { return s } -// List parses a bracketed, comma-separated list of quoted or bare tokens, -// e.g. `[a, "b c", d]` into ["a", "b c", "d"]. Empties are dropped. -func List(val string) []string { +// list parses a frontmatter flow list of quoted or bare tokens, e.g. +// `[a, "b c", d]` into ["a", "b c", "d"]. Empties are dropped. +// +// Unexported: this is how the frontmatter subset spells a list, not a general +// parser. It was exported for wiki.toml, which now goes through a real TOML +// reader. +// The split honours quoting: a comma inside "a,b" separates nothing, and +// splitting on every comma would turn one item into two broken halves. +func list(val string) []string { val = strings.TrimSpace(val) val = strings.TrimPrefix(val, "[") val = strings.TrimSuffix(val, "]") + var out []string - for p := range strings.SplitSeq(val, ",") { - if p = Unquote(p); p != "" { + var item strings.Builder + var quote byte // the open quote character, or 0 outside one + flush := func() { + if p := Unquote(item.String()); p != "" { out = append(out, p) } + item.Reset() + } + for i := range len(val) { + switch c := val[i]; { + case quote != 0: + if c == quote { + quote = 0 + } + item.WriteByte(c) + case c == '"' || c == '\'': + quote = c + item.WriteByte(c) + case c == ',': + flush() + default: + item.WriteByte(c) + } } + flush() return out } diff --git a/internal/parse/parse_test.go b/parse/parse_test.go similarity index 94% rename from internal/parse/parse_test.go rename to parse/parse_test.go index 22f97d3..10ab593 100644 --- a/internal/parse/parse_test.go +++ b/parse/parse_test.go @@ -136,12 +136,27 @@ func TestUnquote(t *testing.T) { } func TestList(t *testing.T) { - if got := List(`[a, "b c" , , d]`); !reflect.DeepEqual(got, []string{"a", "b c", "d"}) { - t.Errorf("List = %#v", got) + if got := list(`[a, "b c" , , d]`); !reflect.DeepEqual(got, []string{"a", "b c", "d"}) { + t.Errorf("list = %#v", got) } - if got := List("[]"); got != nil { + if got := list("[]"); got != nil { t.Errorf("empty list = %#v, want nil", got) } + // A comma inside quotes separates nothing. Splitting on every comma turned + // one item into two broken halves ("a and b"). + for _, tc := range []struct { + in string + want []string + }{ + {`["a,b", c]`, []string{"a,b", "c"}}, + {`['x,y', 'z']`, []string{"x,y", "z"}}, + {`["a: b", "0.1", "[x]"]`, []string{"a: b", "0.1", "[x]"}}, + {`["only one"]`, []string{"only one"}}, + } { + if got := list(tc.in); !reflect.DeepEqual(got, tc.want) { + t.Errorf("list(%s) = %#v, want %#v", tc.in, got, tc.want) + } + } } func TestStringStrings(t *testing.T) {