Record what markfluence published, so a later run has a merge base - #160
Merged
Merged
Conversation
Closes-plan-for: #149. `update` overwrites a page that has moved on since the local copy was made, with no warning and nothing in --json a consumer could branch on. The mtime skip looks like it protects against this and does not. Telling "I changed it" from "they changed it" needs a merge base -- what *this copy* was derived from -- which is a per-copy fact no page-side state can hold, so the fix is an append-only JSONL log at the project root, uncommitted. Two things settled the shape, and the second follows from the first. **"update --force" means "always PUT", full stop.** No logic may suppress the request; CI narrows its own file list from git state, which docs/github-actions.md already documents. That reverses #149's 12:35 comment, which split the skip into a divergence check --force overrides and an idempotence check it does not, the latter made *stateless* so a fresh CI checkout could use it. Statelessness was a requirement only because CI has no local state, so with CI on --force it buys nothing and both checks sit behind one "if !force". **Which is fortunate, because the stateless comparison does not work.** Measured: Confluence injects a fresh ac:macro-id uuid into every macro on write, and a code block is a macro -- so a sent-vs-stored byte comparison fails for most real pages rather than for the HTML-comment edge case storage-format.md records. Making it work needs comments, whitespace collapse, macro-id/local-id and self-closing spacing, and a case it misses fails toward "differs forever", silently republishing everything. So the log carries two fields answering two orthogonal questions, both checked only without --force: page_version decides divergence and refuses the file, publish_sha256 decides idempotence and skips the body PUT. **The sha covers exactly what the body PUT would send** -- the resolved title and the rendered body. A source sha misses a sibling gaining a page_id and a converter change; a render-only sha misses a title change, since convert.ConfluencePage carries no title. Out of the sha: page_width, labels, attachment bytes and paths (each has its own pass, and none of those writes bumps the page version), the version message (hashing it would make --message "typo fix" republish the tree), page_id (its own field, so a retarget discards the base), space/parent, and the ConfluencePage diagnostic fields. Decisions #149 left open, resolved here: - **The mtime check is deleted with no fallback.** Keeping it would preserve exactly the failure modes this issue exists to fix, for exactly the population that has no base -- which at first is everyone. - **Divergence is a per-file failure** with a new CONFLICT code, not a warning that publishes anyway. - **A body-unchanged skip writes a log line too**, which is load-bearing: once the sha does the skipping most runs skip, and a publish-only log would never accrue a base in a tree already published. - **export computes its shas in a post-walk pass**, since a render taken during the walk would be wrong -- siblings are not on disk yet. A page it skipped records no line: that file is somebody's, possibly edited. - **An unknown base warns once per run, not per file**, and the two situations that never self-heal -- no project root, an unreadable log -- get their own warning naming a remedy. - **No command sets the base.** A set-base verb is a way to make markfluence believe a copy is current when it is not, which is the failure this plan exists to fix reached through a supported verb. --force is the honest form. Two findings that are not this plan's to fix, both to be filed separately. L4 (publish-is-idempotent) is marked Holds and does not: F2 is a counterexample, mtime is what implements it, and git does not preserve mtimes. And client.updateLanded's body comparison can never match for a page holding a code block, for the macro-id reason above -- narrow, since it only runs when a PUT errors, but total when it does.
One log per project root, at <root>/.markfluence/log.jsonl. Nothing reads it yet; this is the half of #149 that accrues bases so the checks that consume them have something to work with the day they land. It exists because nothing else can tell "the page differs because I have edits to publish" from "the page differs because somebody published first". That needs a merge base -- what *this copy* was derived from -- and no page-side state can hold it, since the page cannot know what a given local copy came from. So a publish records the version it left the page at and a hash of what it sent, and the last successful line for a file is that copy's base. **Sum covers exactly what the body PUT sends**, the resolved title and the rendered body. The title is in because convert.ConfluencePage carries none -- update passes it to UpdatePage separately -- so a render-only hash would skip the publish of a file whose only change was its title. Page width, labels and attachments are deliberately out: each has its own pass that runs whether or not the body is republished, so folding them in would bump the page version for a change that never touched the body. The parts are length-prefixed so a title cannot run into a body; that framing is part of the persisted format, and changing it invalidates every recorded base. **Nothing here may fail a command.** It is advisory bookkeeping: read skips a line it cannot parse and returns what it has, an absent log is not an error, and an unreadable one leaves every file with no base -- which means publish, exactly what markfluence did before any of this existed. A line from a newer markfluence carrying an unknown field is accepted rather than refused, which is what lets a field be added later with no format version. **Not committed**, and the directory ignores itself: a planted .gitignore holding "*", never overwritten. A shared repository is itself a declaration that the repository is the source of truth, which is the arrangement where --force is the answer and no base is consulted -- so the log serves a local copy with the source of truth in Confluence, where per-checkout state is the right shape. Committing it would buy nothing and would conflict on every concurrent publish. For returns nil for a root with no markfluence.yaml and every method is nil-safe, which is the whole of the "no root, no log" rule: Root.Dir is a fallback there -- the file's own directory rather than a root anybody declared -- so planting state would scatter a .markfluence per directory. Cache hands out one Log per root so a batch reads each log once rather than once per file.
A line per file, appended from the batch loop as that file completes rather than once at the end -- #139 D10's rule, since a run that dies partway must leave every already-published page recorded. The sha is taken from the same title and body handed to UpdatePage, at the call site, so the recorded value and the published one cannot drift apart. **A skip records nothing**, and that is the one decision here worth arguing. The skip in this change is the mtime check, which establishes only that two timestamps are ordered a certain way -- not that this copy matches the page. A line would claim a base nothing verified, and would then silence divergence detection for a page somebody edited in the UI, which is precisely the failure #149 exists to fix. The content-based skip that replaces it does record one, because matching the page is exactly what it verified. Also recording nothing: --dry-run, since a preview that logged would claim a publish happened; a file nothing claims, since there is no page for a base to be against; and a file with no manifest key, since a key is what a line is looked up by. A failure records a failed line, which Base skips. It is history for debugging, gated on having got far enough to know the page id -- a line naming only a filename says nothing worth keeping. A failed write is a warning and never a failure: the page is published by the time this runs, so failing the result would report that it did not happen -- the non-fatal shape pagewidth.Apply and labels.Apply already have. The cost lands on the next run, which sees a base trailing the live page by this very publish and reports a divergence that --force resolves. The three new result fields stay out of --json deliberately: they are bookkeeping about the run, not a report about the page.
A created page is as much a merge base as an updated one: the file and the page are in step the moment create finishes, which is exactly what a base records. Inside the publish loop rather than after it, for the same reason create already writes each file's metadata as that page is published -- #139 D10, a run that dies partway must leave every finished page recorded. Only a successful publish records. A create that failed either left no page at all, so there is nothing for a base to describe, or left the stub S7 names -- and that stub is finished by `markfluence update`, which records its own line when it does. A failure line here would name a page that may have been rolled back. One rule from #149 turns out not to apply. It required a source sha to be taken *after* create's frontmatter write-back, since that changes the file's bytes. A publish sha needs no such ordering: the write-back adds a page_id, which moves neither the title nor the rendered body. The version recorded is version+1, where the page lands: create reserves a content-less stub and publishes into it, so a page is at v2 when create finishes and a base naming v1 would read as a divergence on the next run.
export is the other end of the merge base. Arrangement 2 -- Confluence is the source of truth, a local copy is an export that gets edited and published back -- begins here, so without a line written now the first update of an exported file has nothing to compare against and overwrites whatever the page has become in the meantime. **Two passes, and the split is forced rather than chosen.** A sha taken during the walk would be wrong: a file's render depends on the link index over the whole tree, and its siblings are not on disk yet, so it would not be the value a later update recomputes. But waiting for the end would break D10. So the walk writes a version-only line as each page is written, and a second pass after it -- index built over the finished tree -- appends a fuller line carrying the sha. The reader takes the last successful line, and the degrade-per-field rule covers a run that died between the two. The second pass is self-consistent in the way that matters: it hashes our own render rather than comparing against the page, so it does not depend on round-trip fidelity at all and L5/L6 being Partial is irrelevant. What it does have to match is update's inputs, which is why the title comes from the file's frontmatter and the space key from the live page's webui link. **A skipped page records nothing.** export skips a page whose file already exists (S3), and that file is somebody's -- possibly edited. A base claiming it was derived from this version is a claim export has no grounds for, and it would silence divergence detection for exactly the file most likely to need it. The recorder is built after writeProjectFile, deliberately: for a multi-page export that marker is what makes dest a root at all, so resolving the log before it would walk past dest and key every line against whatever project happens to be above. A single-page export has no marker of its own and correctly keys against the root it landed in, verified live. Verified live end to end against the personal space: create, update, then export into a subdirectory. The export's recorded sha came out byte-identical to the update's, which is the property the whole design rests on -- the base export records is exactly what a later update computes from the same file. Page purged.
CLAUDE.md gains internal/actionlog: what a line is, that Sum covers exactly what the body PUT sends and nothing else, that nothing in the log may fail a command, and that a root with no project file gets none. docs/root-model.md gains a .markfluence/ section beside markfluence.yaml's, since a reader finding that directory in their tree needs to know what it is before they need to know what it is for. The uncommitted argument is the structural one rather than the ergonomic one: a shared repository is itself a declaration that the repository is the source of truth, which is the arrangement that uses --force and consults no base -- so the log serves a local copy with the source of truth in Confluence, where another person's sync point is irrelevant to mine.
…sible **A failed append was silent under --json.** It went through ui.Hint, and every ui helper is a no-op in JSON mode -- so an unwritable .markfluence (a read-only checkout, a full disk, a file shadowing the directory) let `export --json` report every page as a clean success while recording no base anywhere. It now warns on the page's own result, which is a schema field and is what update and create already do for the identical failure. That needed recordWalk and recordShas to take the result by pointer; both are called before the result is appended, so the warning travels. A test plants a regular file where the state directory goes, which is the one way to make MkdirAll fail portably. **recordShas built the link index before checking whether anything would use it.** linkindex.Build walks and parses every .md under the *discovered* root, not under what this run wrote -- so re-running an export over an already-exported tree (every page skipped, so no line to write) paid for a full-tree walk, and a single-page export into a large docs repo walked the whole ancestor project for one file. anyRecordable gates it. **project.Discover's error vanished.** A malformed markfluence.yaml above the destination is fatal to update and inert here, so an export would record no bases at all and report a clean run, leaving no hint that the project file is why divergence detection will later find nothing. A ui.Debug line keeps "nothing may fail the export" while making it diagnosable.
An S1 (no-write-outside-root) break in the package I just added, and the only write in the tree that did not go through the os.Root every other path uses. os.OpenFile with O_APPEND|O_CREATE follows a symlink. So a `.markfluence/log.jsonl` symlinked anywhere -- planted by whoever can write the project directory -- had markfluence append JSON to a file outside the root. Measured rather than reasoned: the bare call wrote straight through such a link into a file in another directory. The attacker needs write access to the project already, which is why this is low rather than alarming. It still matters: appending to an arbitrary path outside the repository is a privilege they do not otherwise have, and S1 is stated as Holds. internal/convert/images.go already refuses a symlinked image leaf and every read is scoped by root.FS; this was the gap. Log now holds the *project.Root and every read, write, mkdir and stat goes through root.FS, which refuses an escape even through a symlinked intermediate directory -- what a lexical containment check cannot see. Path() survives as the absolute path for a message naming the file, and nothing does IO with it. Refusing the *escape* rather than refusing symlinks outright is the right reading of S1: a link that stays inside the root is fine, and os.Root's own semantics are exactly that. The test plants a link pointing out of the root, asserts Append fails, and asserts the target is byte-unchanged.
Writing the S1 test raised the question the plan had not answered: what can a hostile or corrupt log actually do? The answer is a boundary worth stating before PR 2 makes the log a decision input at all. The log is read-only input to two comparisons -- a version against a version, a sha against a sha -- and the page a run writes to always comes from frontmatter or a pages: entry. So a planted log can suppress a publish or provoke a refusal, and cannot redirect one. The page_id a line carries is compared, never followed. Also notes that the log's own file handling obeys S1 through the root's os.Root, since the plan describes the writes and did not say what scopes them.
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.
The plan for #149, plus its first half:
internal/actionlog, written bycreate/update/exportand read by nothing yet. The checks that consume it are the next PR — shipping the writer first means bases start accruing while that one is written, which matters because protection here accrues rather than migrating.The problem, in one paragraph
updatecannot tell "the page differs from my file because I have edits to publish" from "the page differs because somebody published first". Distinguishing those needs a merge base — what this copy was derived from — and no page-side state can hold it, because the page cannot know what a given local copy came from. The mtime skip looks like it protects against this and does not: git does not preserve mtimes, so a clone, pull, checkout ortouchall look exactly like an edit, and it compares the local filesystem's clock against Atlassian's._plans/041_page-moved-detection.mdhas the full argument, the scenario table, and the decisions.Two things settled the design, and the second follows from the first
update --forcemeans "always PUT", full stop. No logic may suppress the request; CI narrows its own file list from git state, whichdocs/github-actions.mdalready documents. That reverses a comment on #149 which had split the skip into a divergence check--forceoverrides and an idempotence check it does not — the latter made stateless, so a fresh CI checkout could use it. Statelessness was a requirement only because CI has no local state, so with CI on--forceit buys nothing, and both checks can sit behind oneif !force.Which is fortunate, because the stateless comparison does not work. Measured against a live page: Confluence injects a fresh
ac:macro-iduuid into every macro on write, and a code block is a macro — so a sent-vs-stored byte comparison fails for most real pages, not just for the HTML-comment edge casestorage-format.mdrecords. Making it work would need comments, whitespace collapse,ac:macro-id/ac:local-idand self-closing spacing, and a case it misses fails toward "differs forever", silently republishing everything.What a line records
{"time":"...","action":"update","status":"ok","file":"docs/some-page.md","page_id":"123456789","page_version":44,"publish_sha256":"b800cc4f…","markfluence":"1.2.3"}page_versionanswers divergence,publish_sha256answers idempotence, and they are read independently so a line carrying one still answers half the question. Nothing ever comparestime— deciding by clock is the defect being removed.Sumcovers exactly what the bodyPUTsends: the resolved title and the rendered body. The title is in becauseconvert.ConfluencePagecarries none —updatepasses it toUpdatePageseparately — so a render-only hash would skip the publish of a file whose only change was its title. Page width, labels and attachments are deliberately out: each has its own pass that runs whether or not the body is republished, so folding them in would bump the page version for a change that never touched the body.Rules worth knowing
Not committed, and the directory ignores itself. A shared repository is itself a declaration that the repository is the source of truth — the arrangement where
--forceis the answer and no base is consulted — so the log serves a local copy with the source of truth in Confluence, where per-checkout state is the right shape. A planted.markfluence/.gitignoreholding*keeps it out of git without editing a.gitignoremarkfluence does not own.Nothing in it may fail a command. A missing, unreadable, corrupt or half-written log degrades the check that reads it and never the run. A root with no
markfluence.yamlgets no log at all, matching #139's rule that a root with no project file refuses rather than creating one.A skip records nothing. The skip that exists today is the mtime check, which establishes only that two timestamps are ordered a certain way — a line would claim a base nothing verified and would then silence divergence detection for a page edited in the UI, which is precisely the failure #149 exists to fix. The content-based skip that replaces it does record one, because matching the page is exactly what it verified.
exporttakes two passes, and the split is forced. A sha taken during the walk would be wrong — a file's render depends on the link index over the whole tree, and its siblings are not on disk yet — but waiting for the end would break #139's D10. So the walk writes a version-only line per page, and a pass afterwards appends a fuller line with the sha.Verified live
Against the personal space: create, update, then export into a subdirectory, page purged. The export's recorded sha came out byte-identical to the update's, which is the property the whole design rests on — the base
exportrecords is exactly what a laterupdaterecomputes from the same file. That was not a given; it means the round trip is a fixed point through the hash, not only through the markdown.Reviews
A code review found three findings, all in
cmd/export/actionlog.goand all the same class: the log failing invisibly. The--jsonone was real —ui.Hintis a no-op in JSON mode, so an unwritable.markfluenceletexport --jsonreport every page as a clean success while recording no base.A security review found nothing above the bar, but writing the question first turned up an S1 (
no-write-outside-root) break in the new package, confirmed by experiment:os.OpenFilewithO_APPEND|O_CREATEfollows a symlink, so a symlinkedlog.jsonlhad markfluence append JSON to a file outside the root. This was the only write in the tree not going through the root'sos.Root. Fixed, with a test that plants a link out of the root and asserts the target is byte-unchanged.That also raised the boundary the next PR must not cross, now recorded in the plan: the log is never a source of a page id, a path, or anything a request is built from. It is read-only input to two comparisons, and the page a run writes to always comes from frontmatter or a
pages:entry — so a planted log can suppress a publish or provoke a refusal, and cannot redirect one.Not in this PR
The checks themselves, deleting the mtime skip, the
CONFLICTcode,S8, and correcting L4 (publish-is-idempotent), which is marked Holds and is not — F2 is a counterexample and mtime is what implements it. Also unfixed and to be filed separately:client.updateLanded's body comparison can never match for a page holding a code block, for the sameac:macro-idreason — narrow, since it only runs when a PUT errors, but total when it does.