Skip to content

Let approval extensions choose the revision NeoWiki publishes - #1389

Merged
JeroenDeDauw merged 2 commits into
masterfrom
1379-revision-policy
Sep 9, 2026
Merged

Let approval extensions choose the revision NeoWiki publishes#1389
JeroenDeDauw merged 2 commits into
masterfrom
1379-revision-policy

Conversation

@alistair3149

@alistair3149 alistair3149 commented Sep 9, 2026

Copy link
Copy Markdown
Member

Fixes #1379

NeoWiki published each page's latest revision everywhere except the page view, so an approval extension such as
ContentStabilization could stabilize what a reader sees on a page but not what the graph stores or the RDF export
served. It already registers a Page Property Provider with us and calls newPageRebuilder()->rebuild() when a
stable point changes; it had no way to say which revision that rebuild should read.

Extensions can now register a RevisionPolicy through NeoWikiRegistrar::setRevisionPolicy(). It answers three
questions, each asked where it is natural:

  • publishesRevision() — asked as a revision is written. A save already has its revision and only needs to know
    whether to publish it, so this costs one lookup. Return false and the graph keeps whatever it published before.
  • publishedRevision() — asked when nothing but the page is known: by PageRebuilder::rebuild(), the reprojection
    path an approval extension calls when its answer changes, and on every Schema, Layout, Mapping and configuration
    read and every RDF export. Returns the revision to publish, or null when the page has nothing publishable.
    rebuildFromPrimary() deliberately does not ask: import and undelete write a revision, so they take the save
    path's semantics.
  • revisionIsReadableBy() — asked when a caller names a revision by id, which neither publishing method can
    intercept. A refused revision answers exactly like an absent one, so the sequential revision ids stay unsweepable
    (Enforce per-page read permission on Subject, Layout, and RDF read endpoints #1046).

Registration is a single slot rather than the add* list every other extension point uses, because two extensions
cannot both decide this. A second policy is refused with a warning and the first keeps deciding. With nothing registered every surface reads the latest revision, exactly as before.

The registered policy is wrapped in FailureIsolatingRevisionPolicy, as every other extension-contributed plugin is.
A policy that throws publishes nothing and hides everything — the edit hook fires inside PageUpdater's atomic
section, so an uncaught throw there would roll the contributor's save back. It also refuses a revision from another
page, and a revision whose text is suppressed: only the current revision was ever projected before, and core forbids
suppressing that one, so a policy naming an older revision must not start publishing suppressed Subjects.

The subject-to-page index is deliberately not governed by the policy

It records where a Subject lives, not whether it is published, and it is written from every revision. Every id-keyed
read and write addresses its page through it — ReplaceSubjectAction, UpdateStatementAction, DeleteSubjectAction,
MoveSubjectAction, and the check in CreateSubjectAction that an id is not already taken — so a Subject left out of
it could not be edited, moved or deleted, and its id would read as free and be re-mintable onto another page. Adding a
Subject in an unapproved revision is the normal case on an approval wiki, so that would fire constantly.

Keeping the index out of it also makes the maintenance rebuilder correct rather than contradictory: it writes from
page_latest and needs no knowledge of the policy, which is why it can keep running under update.php.

Which surfaces this covers

The graph projection and the RDF export publish through the policy. Schemas, Layouts, Mappings and the on-wiki
configuration page read through it too, so an unapproved edit to one does not take effect until it is approved; the
Schema editor is unaffected because it loads page source through core's own /v1/page/ endpoint.

Reads by Subject id stay on the latest revision. Routing them through the policy is what an earlier revision of this
branch did, and it broke the subject editor: the same endpoint is the editor's load path, so the editor loaded
approved values and saved them back over the draft.

Known gaps, all tracked

Considered, omitted

  • Collapsing the two publishing methods into one nullable substitution. It reads as a reduction, but it forces every
    draft save to resolve and re-project a revision the graph already holds, and it was what dragged the index and the
    editor into the design.
  • Making the index follow the published revision, or hold the union of both. The first breaks editing as described
    above; the second leaves the maintenance rebuilder unable to remove a row, which is the one thing it exists to do.

Not in this change

The decision itself is not recorded in an ADR. docs/AGENTS.md says an ADR is a dated record, not retro-edited
apart from status links, so an earlier revision of this branch that appended to ADR 27 and ADR 32 has been reverted.
Revision choice wants its own ADR.

AI-authored — Claude Code, Fable 5.1 after a mid-session model switch from Opus 5 (1M context), which wrote the first design; implemented from #1379 on @alistair3149's go-ahead, redesigned after a five-pass review found the first design broke editing, then hardened after a second six-pass review; diff not yet human-reviewed; phpcs and phpstan clean, nine touched test classes pass locally including one that drives the registered policy through NeoWikiExtension's real wiring into Neo4j, CI on the previous push green across MW 1.43–master.

@alistair3149
alistair3149 force-pushed the 1379-revision-policy branch 4 times, most recently from 8d31588 to ef79483 Compare September 9, 2026 20:34
@alistair3149
alistair3149 marked this pull request as ready for review September 9, 2026 20:54
alistair3149 and others added 2 commits September 10, 2026 00:37
Fixes #1379

NeoWiki published each page's latest revision everywhere except the page view, so an approval extension such as
ContentStabilization could stabilize what a reader sees on a page but not what the graph stores or the RDF export
served. It already registers a Page Property Provider with us and calls `newPageRebuilder()->rebuild()` when a
stable point changes; it had no way to say which revision that rebuild should read.

Extensions can now register a `RevisionPolicy` through `NeoWikiRegistrar::setRevisionPolicy()`. It answers three
questions, each asked where it is natural:

- `publishesRevision()` — asked as a revision is written. A save already has its revision and only needs to know
  whether to publish it, so this costs one lookup. Return false and the graph keeps whatever it published before.
- `publishedRevision()` — asked when nothing but the page is known: by `PageRebuilder::rebuild()`, the reprojection
  path an approval extension calls when its answer changes, and on every Schema, Layout, Mapping and configuration
  read and every RDF export. Returns the revision to publish, or `null` when the page has nothing publishable.
  `rebuildFromPrimary()` deliberately does not ask: import and undelete write a revision, so they take the save
  path's semantics.
- `revisionIsReadableBy()` — asked when a caller names a revision by id, which neither publishing method can
  intercept. A refused revision answers exactly like an absent one, so the sequential revision ids stay unsweepable
  (#1046).

Registration is a single slot rather than the `add*` list every other extension point uses, because two extensions
cannot both decide this. A second policy is refused with a warning and the first keeps deciding. With nothing registered every surface reads the latest revision, exactly as before.

The registered policy is wrapped in `FailureIsolatingRevisionPolicy`, as every other extension-contributed plugin is.
A policy that throws publishes nothing and hides everything — the edit hook fires inside `PageUpdater`'s atomic
section, so an uncaught throw there would roll the contributor's save back. It also refuses a revision from another
page, and a revision whose text is suppressed: only the current revision was ever projected before, and core forbids
suppressing that one, so a policy naming an older revision must not start publishing suppressed Subjects.

## The subject-to-page index is deliberately not governed by the policy

It records where a Subject lives, not whether it is published, and it is written from every revision. Every id-keyed
read and write addresses its page through it — `ReplaceSubjectAction`, `UpdateStatementAction`, `DeleteSubjectAction`,
`MoveSubjectAction`, and the check in `CreateSubjectAction` that an id is not already taken — so a Subject left out of
it could not be edited, moved or deleted, and its id would read as free and be re-mintable onto another page. Adding a
Subject in an unapproved revision is the normal case on an approval wiki, so that would fire constantly.

Keeping the index out of it also makes the maintenance rebuilder correct rather than contradictory: it writes from
`page_latest` and needs no knowledge of the policy, which is why it can keep running under `update.php`.

## Which surfaces this covers

The graph projection and the RDF export publish through the policy. Schemas, Layouts, Mappings and the on-wiki
configuration page read through it too, so an unapproved edit to one does not take effect until it is approved; the
Schema editor is unaffected because it loads page source through core's own `/v1/page/` endpoint.

Reads by Subject id stay on the latest revision. Routing them through the policy is what an earlier revision of this
branch did, and it broke the subject editor: the same endpoint is the editor's load path, so the editor loaded
approved values and saved them back over the draft.

## Known gaps, all tracked

- Three Subject read surfaces still serve the latest revision — `action=subjects`, the page-keyed REST read, and
  referenced Subjects on other pages via `PointInTimeSubjectLookup`
  (#1390). The parse-time accessors are unchanged for a different
  reason: they need the parser's own current-revision callback rather than this interface.
- The RDF export stops answering for a page once approval is withdrawn, but what was published stays in the graph
  until the page is deleted; a rebuild does not remove it (#1391).
- Schemas and Mappings cache on the page's latest revision id, so an approval change with no page edit does not take
  effect until that page is edited or the entry expires; and a Subject is validated against the published Schema
  while the Schema editor shows the latest one (#1392).
- Undelete and import take the save path, so a restored page whose latest revision is unpublished comes back
  addressable but absent from the graph until the approval extension reprojects it.

## Considered, omitted

- Collapsing the two publishing methods into one nullable substitution. It reads as a reduction, but it forces every
  draft save to resolve and re-project a revision the graph already holds, and it was what dragged the index and the
  editor into the design.
- Making the index follow the published revision, or hold the union of both. The first breaks editing as described
  above; the second leaves the maintenance rebuilder unable to remove a row, which is the one thing it exists to do.

## Not in this change

The decision itself is not recorded in an ADR. `docs/AGENTS.md` says an ADR is a dated record, not retro-edited
apart from status links, so an earlier revision of this branch that appended to ADR 27 and ADR 32 has been reverted.
Revision choice wants its own ADR.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NzPP5bUaN1Nos9CwJoLXzW
Two fwrite( STDERR ) dumps, and the graph reads that only fed them, were left behind from debugging
testRefreshProjectsTheRevisionTheRegisteredPolicyPublishes and printed on every run.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@JeroenDeDauw

Copy link
Copy Markdown
Member

Rebased onto master to resolve the conflicts with #1386, which dropped the $user argument from OnRevisionCreatedHandler::onRevisionCreated() and from the page-properties source (the revision now supplies its own author). Besides the two textual conflicts, that change reached three spots of this branch that git had merged silently:

  • PageRebuilder now hands the handler the revision alone, after the policy substitution.
  • The two new handler tests call onRevisionCreated( $revision ) without the user argument.
  • The three new PageRebuilderTest cases use master's spy, which records a plain list of revisions. The assertion that the published revision brings its own author went with the parameter it tested: the handler now reads the author from whatever revision it is given, so [ $published ] being what the handler received covers it.

Committed separately: two fwrite( STDERR, … ) debug dumps left in PageRefreshWithoutEditTest, which printed on every run.

One interaction for the author's eye rather than a fix here. Master added two reprojection callers for #1246, onArticleRevisionVisibilitySet and LastEditorPagesRebuilder, and both go through rebuildFromPrimary(), which this branch defines as the non-substituting path. With a policy registered, a page whose latest revision is unpublished is handed to the handler as that latest revision, publishesRevision() declines it, and the hidden name stays in the graph until the approval extension reprojects the page. No draft content gets published by it, and it matches the undelete/import gap already listed in the description. Flagging it so that coupling "reads primary" to "does not substitute" stays a deliberate choice rather than an accident of the merge.

Verification: make cs clean; full local PHPUnit suite green (3233 tests, 10 skipped) before the debug cleanup, with the affected class re-run after it.

AI-authored — Claude Code, Fable 5.1 (max); one-line ask from @JeroenDeDauw to resolve the conflicts, no revisions; rebase and adaptations not yet human-reviewed; phpcs, phpstan and the full local PHPUnit suite run locally, CI pending on the new push.

@JeroenDeDauw
JeroenDeDauw merged commit c34bb69 into master Sep 9, 2026
14 checks passed
@JeroenDeDauw
JeroenDeDauw deleted the 1379-revision-policy branch September 9, 2026 23:21
JeroenDeDauw added a commit that referenced this pull request Sep 10, 2026
Follows-up to #1389

Each fact keeps one home: the index rationale on `OnRevisionCreatedHandler`, the single-slot rule on
`RevisionPolicyRegistry`, the default behaviour and the two-method split on the `RevisionPolicy` interface. Cut are
the restatements of those in `PageRebuilder`, `NeoWikiRegistrar`, `GetSubjectApi`, `NullRevisionPolicy` and two test
docblocks; the history phrases ("as NeoWiki has always done", "before this policy existed"); and the note in
`PageContentFetcher` on how the Schema editor loads, which describes another component and already lives in
`docs/extending/extending.md`. Comments only, no code changes.

Considered, omitted: the `[[ADR 32]]` pointer in the interface docblock. The wiki-link syntax resolves to nothing in
PHP, and `NullSubjectPageIndex` already cites the ADR where the index constraint is applied.

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
JeroenDeDauw added a commit that referenced this pull request Sep 10, 2026
Fixes #1391

Two simplifications of #1389, and the fix that falls out
of them.

RevisionPolicy answers one question. publishesRevision() was derivable from publishedRevision() under the
interface's own "answers must agree" contract, so the method and the contract are both gone.

OnRevisionCreatedHandler decides what a page publishes; PageRebuilder only chooses the read source.
rebuild() and rebuildFromPrimary() both hand the handler the page's current revision, which indexes that
revision's Subjects, asks the policy, and projects the revision it names — reading that revision's own
subject slot and page properties when it is not the one the handler was given.

Every path therefore goes through that one decision, including the rebuildFromPrimary() callers
#1389 exempted from substitution — import, undeletion, a
revision-visibility change and the hidden-user rebuild — which now project what the policy publishes for
the page rather than what they were handed: an import into a page with nothing approved is withdrawn
until the approval extension reprojects it, where before it was skipped and the graph left as it was. The
regression test on the visibility path locks that in.

The fix: a page the policy publishes nothing for is now withdrawn from the graph stores rather than left
as it was. Before this, a page whose approval had been revoked stayed queryable through Cypher and SPARQL
while the RDF export and the REST reads had already stopped answering for it, and no rebuild converged.
PageRefreshOutcome::Unpublished replaces SkippedUnpublishableRevision; a rebuild counts a withdrawn page
as reconciled and says so in the log. The subject-to-page index stays ungoverned by the policy: the page
still exists on the wiki, so its Subjects have to stay addressable for editing.

A consequence worth stating: FailureIsolatingRevisionPolicy turns a throwing policy, one naming a revision
of another page, and one naming a suppressed revision into null, so such a page is now withdrawn rather
than left alone. That is fail-closed for an approval feature; the error is logged on every call, and
RebuildGraphDatabases restores the graph once the policy works.

Considered, omitted:

- A third policy answer meaning "leave the graph alone", so a failing policy could be told apart from a
  deliberate revocation. The log and a rebuild cover that without new interface surface.
- Counting withdrawals on RebuildProgress and the persisted run records.
- Withdrawing on the reprojection path only, leaving the save path skipping. With the question asked once
  and in one place, that split would only make the two paths disagree.

Co-Authored-By: Claude Opus 5 (max) <noreply@anthropic.com>
JeroenDeDauw added a commit that referenced this pull request Sep 10, 2026
The section now states the contract once, in the order the reader uses it: which surfaces publish through the
policy, the class to implement, registration, what each method receives and returns, the rebuild an approval
change outside an edit needs, and the surfaces that still read the latest revision. The registration hook is no
longer repeated from Getting started, the rationale bullets are gone, and the two gap lists are one paragraph.
Retitled "Revision policy" to match the sibling headings; nothing linked the old anchor.

It documents the by-id Subject read as #1398 ships it, so that
PR belongs alongside this one. The RebuildGraphDatabases sentence in maintenance.md said the rebuild projects
each page's latest revision, which stopped being true with
#1389.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Let approval extensions choose the canonical revision NeoWiki reads and projects

2 participants