feat: documentation-root model for file organization (026) - #104
Merged
Conversation
Adds docs/guarantees.md: safety (S1-S6), laws (L1-L8), conformance (C1) and reporting (R1-R2). Each is deliberately about one thing so it can be argued with on its own, and each carries a status, because several are aspirational rather than true today -- the same reason every entry under docs/confluence/ carries provenance. The point is that they can be cited. A spec or PR names the ids it changes; a change that would break one needs an argument. Two rules keep that working: ids are permanent and never reused, and a status downgrade is stated rather than noticed later. Four groups because the kinds differ in how they are verified. Safety violations do damage; laws are algebraic and property-testable; C1 is agreement with an external spec rather than an internal property; and the reporting pair are not invariants at all -- publishing a dead link may be acceptable, doing it silently is not. Overwriting and removal are separated for the same reason. Overwriting is in scope of the operation, so consent is a flag; removal is out of scope, so the guarantee is that it does not happen unless removing is what was asked for. S4-S6 are written before anything removes because the alternative was a guarantee that expires: "nothing in Confluence is deleted" would have gone false the day a prune feature shipped, and is recorded as retired rather than deleted. Also adds a Non-goals section, for decisions that constrain future work the way a guarantee does without being properties themselves. The first entry is symlinks: markfluence does not follow one that leaves the project. That is a scope decision and an enforcement mechanism at once, since scoping reads to an os.Root is what makes S2 hold by construction instead of by convert.withinRoot's lexical comparison -- which a symlinked directory inside the tree passes today while reading from outside it. internal/attachfile already relies on os.Root for exactly this on the write side.
This captures max length for attachment name and comment and behavior when exceeding that length. HTTP 400 when the comment exceeds the length. HTTP 500 when the name exceeds the length which is unfortunate.
markfluence has three path conventions and no two of them agree about what a path is relative to. Resolution is relative to the markdown file, naming encodes the reference as written, and export placement is relative to --dest and refuses escapes. Six scenarios in the spec show what that costs, each with the tree, the commands, and the measured output: a link to a subdirectory silently reaching the wrong page, the same file publishing differently from two working directories, one shared image acquiring two identities, a layout the README endorses that cannot be exported at all, unresolved links reported nowhere, and a `parent:` path read with no bound. The model is one root, with every recorded path relative to it. The root is the directory holding markfluence.yaml, found by walking up, or the markdown file's own directory when there is no project file. Markdown still writes page-relative paths so a preview renders them; what changes is that the recorded attachment source and the link index key become root-relative. There is no flat mode and no nested mode -- for a one-level tree the two are byte-identical, so flat is what this already does rather than a thing to build. Three costs, stated rather than discovered later. Attachment names get longer, and the comment's 255-character limit caps a path at 165. Which kind of file move is free gets inverted: moving a page becomes free and moving an asset does not, where today it is the reverse. And the no-config default is stricter than today, so the shared-parent layout is repaired only once a root is declared. Everything the spec raises is decided. R1 is deliberately not fixed here -- it needs a converter diagnostic rather than a path decision -- and S2 gets more reachable before the enforcement lands, which items 2 and 7 cover. Deferred work is tracked in #42, #59, #99, #100 and #101 rather than left in the document.
Breaks the 17-item spec into nine dependency-ordered commits, settles terminology two discovery passes needed that 025's prose didn't distinguish (the root vs. the .env lookup), defers multi-page export to #59, and folds a minimal R1 fix into the link-index commit since it's cheap once that index exists and needed for manually verifying the rest.
Discover(startDir) walks up from startDir looking for markfluence.yaml, falling back to startDir itself when none is found. Stats a filename rather than listing a directory (needs only execute permission, so an EACCES on an ancestor keeps walking rather than failing), and never follows a symlink in the walk. Root.FS is an os.Root scoped to the discovered directory, ready for the S1/S2-bounded reads later commits add.
loadEnvFile now calls project.Discover(cwd) instead of reading the literal "./.env" -- so running from a subdirectory of a markfluence.yaml-marked project finds .env at the root, the same way it will for credentials on any command. With no project file, discovery falls back to the working directory itself, so behavior is unchanged from today. This is the narrower of the two discovery passes 026 distinguishes: it starts from the working directory rather than a markdown file's own directory, runs once before any file is touched, and isn't reported -- its only job is answering "where is .env." --env-file is unaffected; it's still an explicit, required-if-set path.
…commit 3) project.FromPath builds a Root directly from an explicit directory, skipping discovery entirely; project.Resolve(override, startDir) is the combinator every future caller (create, update, attachment-upload) will use: the override wins when set, otherwise it discovers from startDir as before. --root is a persistent flag, applied uniformly to the whole invocation (not per file) -- it's one value, not a per-file setting. Directory completion via internal/completion. --env-file's help text is corrected to match commit 2's discovery-based default. Nothing reads rootFlag yet; that starts in the next commit, which threads the root through the converter.
MdToConfluence takes a *project.Root per file instead of calling os.Getwd(). withinRoot's lexical comparison is replaced by a read through root.FS (mirroring internal/attachfile's existing os.Root pattern), which is what makes an escape through a symlinked intermediate directory refused rather than merely uncommon -- os.Root catches it, a lexical check cannot see it at all. The image leaf refuses a symlink outright (Lstat, not Stat), even one that resolves inside root. images.go now records Source root-relative instead of relative to the referencing file, and derives the attachment name from that same root-relative path -- this is the change that fixes Scenario C (one shared image referenced from two depths no longer gets two identities) and, as a side effect, Scenario D for single-page export. internal/project.Cache resolves a root per starting directory, reusing it (and the open os.Root handle) for every later file in the same directory. create and update each build one, discover per file during their existing per-file validation, and report every distinct root used once (human output only -- --json's roots field is follow-up work, not done here). Regression suite: images-shared-parent gets an explicit root in test.input and its golden changes -- 025 names this case as the whole point of the model. Added dedicated (non-golden) tests for the two symlink refusals, since a checked-in symlink fixture is fragile across platforms; internal/attachfile already tests its equivalent the same way. Fixed along the way: a nil-pointer panic in the first draft of the image-leaf logic, where "lstatErr == nil" was read as "Lstat succeeded" but is also true when Lstat was never called at all (a lexically escaping path). Caught by the images-broken regression case, not by hand.
Notes what actually happened rather than leaving the plan describing work not yet done: the --json half of root reporting split into its own follow-up, and the nil-pointer bug the regression suite caught.
New internal/linkindex package: Build(root) walks the tree once, via root.FS (so it cannot descend a symlinked directory), keying the page and anchor maps by root-relative path instead of basename-in-one- directory. SetPage overrides/injects an entry -- the hook create's reserve phase (commit 8) needs to seed ids that exist only in memory. Cache builds one Index per distinct root, shared across a batch the same way project.Cache already shares roots. internal/convert's docKey (filepath.Base) is gone; resolveDocKey resolves a link/anchor destination to a root-relative key the same way images already resolve to a root-relative Source, and needs no escape clamp -- an escaping key just never matches anything in an index built by walking downward from root in the first place. This is what fixes Scenario A (a link to sub/dup.md no longer reaches ./dup.md) and Scenario F (a link that could traverse above root is now a real, harmless miss rather than accidentally safe by basename flattening). Minimal R1, folded in here since it's cheap once the index exists: rewriteDocLink now warns when a same-tree .md-shaped link doesn't resolve, reusing the r.warnings list images.go already populates for a broken image. A dedicated check command (tree-wide audit, per-reason messages) stays out of scope. githubSlug/confluenceSlug moved into linkindex (exported, since aclink.go's reverse-direction headingSlugs still needs them); a second, unrelated whitespace-collapsing regexp in storage_to_md.go stays local rather than importing linkindex for a coincidentally-identical pattern. Regression suite: two new cases (link-cross-directory, link-outside-root) exercise Scenarios A and F end-to-end; two existing cases (doc-links-encoded, internal-doc-links) already had an unresolved link and gain the new warning in their golden, unchanged otherwise. docs/guarantees.md: L1, C1, L2, L3 move to Holds -- L2/L3 were already true as of commit 4 and should have been updated then; caught up here rather than left for the commit-9 sweep. R1 moves to Partial (the minimal warning, not the dedicated diagnostic). S2 moves to Partial: the image leaf and link/anchor resolution both hold now: the remaining gap is parent:, commit 6. README's link-resolution section no longer claims "there is no warning for this," which the minimal R1 warning makes false.
Notes what actually happened: two new regression fixtures rather than extending existing ones for Scenario E, docKey removed rather than moved, and the L2/L3 guarantees.md catch-up from commit 4.
resolveParent now reads a frontmatter parent: .md reference through root.FS instead of the bare filesystem. A path resolving outside root is a hard error naming the root, not an unresolved-and-reported case the way a link is -- a parent is load-bearing, and publishing under the wrong one (or silently under none) is worse than not publishing at all. A symlinked parent target is refused the same way a symlinked image leaf is. Root discovery in resolveFile moved earlier so resolveParent can use it; nothing else about resolveFile's validation order changed. No existing test exercised resolveParent's .md path at all (checked: grep found none), so this adds coverage for the ordinary cases (not found, no page_id yet, in-set, published) alongside the two new ones (escape, symlink) rather than relying on the change being incidentally exercised elsewhere. docs/guarantees.md: S2 moves from Partial to Holds -- all three reads 025 names (image leaf, link/anchor resolution, parent:) are now enforced.
…mment (026 commit 7) attachment-upload records a root-relative source (internal/project, discovered from the uploaded file's own directory) instead of filepath.Base(f). With no markfluence.yaml above a file this reduces to the bare basename exactly as before, so nothing changes for the common case; a declared root spanning subdirectories is what makes the recorded source keep its structure, matching what publishing a page referencing the same file would record. Closes #101. The attachment comment's checksum truncates to 32 hex characters (128 bits) instead of the full 64-hex digest: the comment only needs to detect that a file's bytes changed, not resist an adversary, and it's the bigger, cheaper lever on the 255-character ceiling than shortening the "markfluence: " prefix would be -- worth keeping full-length, since it's the ownership marker S5 rests on and a human can recognize it browsing a page's attachments. parseAttachmentComment already tolerated any hex length, so nothing there needed to change; planAttachments' comparison now truncates a *stored* checksum to the same length before comparing, so an attachment stamped by an older (64-hex) markfluence is still recognized as unchanged rather than re-uploaded once just because the format moved on. internal/attachfile.Resolve's doc comment is reframed: under a root-relative model, an image whose path escaped root was refused before ever being recorded as an attachment, so a legitimate Source markfluence writes can no longer contain an escaping ".." at all. The clamp is now purely a guard against a maliciously-edited attachment comment (server data) or an older markfluence's page-relative Source, not a rule about a layout markfluence's own writer produces. No functional change; the existing escape tests stay, since the threat model they guard against is unrelated to how markfluence writes.
…(026 commit 8) create's phase 2 now reserves a content-less stub for every file (title, parent, no body), in topological order, before phase 3 converts and publishes any of them. Each reserved id is fed into the shared link index immediately (Index.SetPage, keyed by convert.DocKeyFor -- exported so this seeds the identical key MdToConfluence resolves against, rather than a second computation that could drift from it), regardless of --no-persist. By the time phase 3 converts anything, every id any file in the batch might link to already exists, so link resolution stops depending on creation order: a link pointing "forward," "backward," or part of a cycle between two files in the same batch all resolve the same way. This turned out to matter more than 025 emphasized: after commit 5's link index landed, a fresh create batch's cross-links had *regressed* to fully unresolved (the shared index is a disk snapshot taken before any of the batch's files existed, and nothing refreshed it) -- worse than the pre-025 per-directory rebuild, which at least resolved the backward case. Commit 8 isn't just an improvement on top of 5; it's the fix for a gap 5 opened and this plan always intended to close before shipping. reserveOne/publishOne replace createInOrder/createOne. Persisting the reserved id still happens immediately after reserve, unless --no-persist -- so an interrupted run has already recorded a page_id a plain `update` can finish publishing against, rather than leaving the file with no trace of a page that might already exist. --dry-run creates nothing in either phase but still runs publish's conversion for a preview, using whatever the index has -- the same limitation dry-run already had, just relocated to where the split puts it. Accepted cost, named in 025: every page's v1 is now a permanent content-less stub, and a run interrupted between reserve and publish leaves one behind rather than no page at all. No existing test called createOne/createInOrder directly (checked), so this is new coverage rather than a rewrite of broken assertions: cmd/create/run_test.go adds a fakeConfluence double and tests the property this commit exists for directly -- two sibling files linking to each other resolve regardless of direction -- plus --no-persist still seeding the index, a failed reservation cascading to skip its child's publish, --dry-run creating nothing, and the stub/publish version sequencing (v1 empty, v2 real content). Also: human output for a batch now renders after both phases complete rather than streaming per file as each one finishes, since a result isn't final until publish does -- and publish can't start for anyone until reserve finishes for everyone. Disclosed simplification, not requested by the plan.
New docs/root-model.md: discovery (the two passes, settled precisely -- the root vs. the narrower .env lookup), the project file, the CVE review restated for a reader who hasn't seen _plans/026, what S1/S2 bound, attachment identity, link resolution, and multi-root batches. Conceptual; points at the README for how-tos. README gets a new "## The documentation root" section (model summary plus four recipes: moving a file, moving a page's own images -- which now churns, inverting the pre-model behavior -- moving a shared asset, and setting up a shared assets directory) placed after "## Markdown page structure" per the plan. Two passages that predated this whole plan were stale and got corrected in place rather than left standing beside a new section that contradicted them: Configure's ".env from the current directory" and Body's "run markfluence from the root of your documentation tree" (plus its now-wrong ../assets/logo.png -> ..%2Fassets%2Flogo.png encoding example, which is root-relative now, not page-relative). docs/guarantees.md: L5/L6's explanatory prose was stale the same way -- it named a cause (export refusing an asset above the page) that commit 4 had already fixed for single-page export -- corrected alongside the final status sweep, which otherwise confirmed nothing was left Aspirational that should have moved. _plans/026 marked complete: all 9 commits landed, annotated throughout with what actually happened where it differed from the plan going in. Three things explicitly remain open past this plan's scope: the --json roots field split off from commit 4 and never revisited, multi-page export (#59), and a dedicated check command for the full form of R1.
… section Scoped tokens: cut commentary that didn't earn its place inline. The documentation root: lead with "do you need a markfluence.yaml?" (single files: no; a linked/shared-asset tree: yes, at its root) before the precise walking-up-the-tree definition -- the abstract version alone doesn't help a reader who hasn't seen this model before.
markfluence has never had a release, so a note about pages published under an encoding scheme that predates any release, and a "muscle memory" aside aimed at users of a prior behavior, both address a population that doesn't exist yet. Kept the actual trade-off in the second case (moving a page's images churns; moving just the page doesn't) since that's worth knowing regardless of history.
Removed legacyChecksumPrefix ("mzcld:checksum:", the original Python
tool's format) and truncatedChecksum (the 64-hex/32-hex tolerance
bridging this week's format change). parseAttachmentComment now
recognizes exactly one form; anything else -- a hand-uploaded file, or
one stamped by any markfluence predating this comment format --
reads as unmanaged and is re-uploaded once on the next sync, the same
as any other unrecognized comment.
Deliberate: while actively developing against a real Confluence
instance, carrying parsing code for formats this same tool wrote
minutes or years ago isn't worth the complexity, even though real
attachments in both old formats exist on live pages right now. A
one-time re-upload is an acceptable, self-correcting cost.
Test fallout: TestSyncAttachmentsSkipsWhenLegacyChecksumMatches and
TestBuildResultLegacyManaged (cmd/attachmentlist) tested exactly the
removed behavior and are deleted outright. Three other tests used the
legacy prefix or an untruncated checksum only as fixture plumbing to
reach an unrelated assertion (checksum-differs, no-source-recorded,
current-checksum-matches) -- rewritten against the one current format
rather than removed, since the properties they check still hold.
CLAUDE.md's architecture notes updated to match; they described the
legacy parsing as current.
…-result consistency attachment-upload's rootRelativeSource never checked that --root actually contains the file being uploaded; project.Resolve applies an override with no containment check of its own, so a --root naming an unrelated directory could encode an escaping "../"-prefixed path straight into the attachment name. Refused now, matching the same guard images.go and create's resolveParent already have. resolveParent's escape-through-a-symlinked-directory case (an escape only os.Root can see, not a lexical one) was falling through to a plain "parent file not found" instead of naming the actual reason -- images.go already made this same distinction; resolveParent didn't. internal/project.Cache.walkAndCache now backfills every ancestor directory visited during a walk to the same shared *Root, not just the exact starting directory -- so a batch spanning many subdirectories of one project pays for Discover's walk (and os.OpenRoot) once for the whole subtree, closing the residual quadratic-ish cost a per-exact- directory cache still left. The no-project-file fallback is deliberately excluded from this backfill (bound only to the directory Resolve was actually called with), so an unrelated directory sharing an ancestor still falls back to itself rather than inheriting someone else's fallback root. client.Options gained Roots: when a caller already built its own project.Cache (create/update/attachment-upload all do), passing it into client.Resolve makes --root also redirect the .env lookup, and shares the discovery cost instead of paying for a second, independent walk to the same root. createResult.fail now explicitly nulls page_id/url on every failure; failKeepingPage is the one exception, for when a page really was created and only the frontmatter write-back afterward failed -- the id has to stay visible or the page becomes untraceable. Schema description updated to name both exceptions. The link index is now seeded even under --dry-run (with an empty page_id), so a cross-link between two new files in the same batch previews as resolved instead of a false "not resolved" warning. internal/convert.DocKeyFor and (*storageRenderer).resolveDocKey now share one rootRelativeKey helper instead of two independent copies of the same filepath.Rel/ToSlash computation. make check clean; full suite passes, including new tests for every case above (TestLocalAttachmentsRejectsRootEscape, TestResolveParentEscapeThroughSymlinkedDirectoryNamesTheEscape, TestCacheResolveBackfillsSharedRoot, TestCacheResolveFallbackDoesNotContaminateAncestors, TestResolveRootsOverridesEnvDiscovery, TestCreateAllPublishFailureNullsPageIDAndURL, TestCreateAllDryRunCrossLinkResolves, TestReserveOneWriteFailureKeepsPageIDVisible).
docs/root-model.md, README, and 026's terminology section all described the .env lookup as fully independent of --root. That stopped being true once client.Options gained Roots: those three commands already build a project.Cache for their own per-file resolution and now share it with the .env pass instead of a second, independent project.Discover(cwd) call, so --root's override reaches .env too, for exactly the commands --root already mattered for. Corrected in all three places, plus a note on the cache's own walkAndCache backfill optimization that shipped alongside it.
The --json half of root reporting split off from commit 4 of 026 and was never picked back up -- root: <dir> lines only ever reached human output. Envelope gains a top-level Roots []string (per-invocation, not per-result, so it lives on the envelope rather than on createSummary/updateSummary as first sketched), defaulting to [] like Results. create (success and abort paths), update, and attachment-upload set it from the batch's project.Cache; every other command keeps the default, since none of them has a per-file root concept. attachment-upload also gains the human-output root: <dir> lines it had never gotten alongside create/update in commit 4.
willkg
force-pushed
the
file-org-fixing
branch
from
August 29, 2026 15:07
618e872 to
30caaee
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #105, #101.
_plans/025_file-organization.md, sequenced into commits by_plans/026_file-organization-implementation.md: per-file root discovery (internal/project, walking up formarkfluence.yaml, falling back to the file's own directory),.envresolution anchored to that root, an explicit--rootoverride, a root-relative link/anchor index (internal/linkindex) so cross-file links resolve correctly across a multi-file tree, andcreate's three-phase restructure (preflight/reserve/publish) needed to make that indexing work for pages created in the same batch.--json"roots"field (every distinct documentation root a command resolved, sorted,[]by default) and the matching human-outputroot: <dir>line — the JSON half of root reporting had been split off from an earlier commit and is included here.docs/root-model.mddocuments the model; README's documentation-root section gained a practical decision guide, and unrelated stale pre-release history was trimmed from it.docs/guarantees.md's S1/S2/L5/L6 statuses were updated to reflect what's now actually true.Test plan
attachment-upload, aresolveParentmessaging inconsistency, aCache.walkAndCachebackfill optimization, and acreateResultfailure-path split) were verified against the full test suite and committed.markfluence.yamlroot under/tmp/mf-smoke, cleaned up locally afterward) — rancreate(dry-run and real),update, andattachment-upload, confirming both--json's"roots"field and the human-outputroot:line report the correct resolved directory in each command, and that a root-less command (find) still emits"roots": []per the schema default. The created page (markfluence roots smoke test) was left in place as a fixture per the project's own testing convention.make check(vet, fmt-check, test, build, lint) is green on the branch.