Skip to content

Act on the review of the Subject Sources foundation - #1330

Merged
JeroenDeDauw merged 7 commits into
subject-sources-foundationfrom
pr-1265-review-fixes
Sep 1, 2026
Merged

Act on the review of the Subject Sources foundation#1330
JeroenDeDauw merged 7 commits into
subject-sources-foundationfrom
pr-1265-review-fixes

Conversation

@alistair3149

@alistair3149 alistair3149 commented Aug 31, 2026

Copy link
Copy Markdown
Member

For #993

Follows-up to #1265, whose head this targets. Merge it into
that branch before #1265 lands.

Acts on a review of #1265. The design finding supersedes two of the bug findings, so the first commit
resolves three of them at once — worth knowing before reading the diff.

The design finding, and why it swallows two bugs

#1265 added Violation::alwaysBlocksWrites(), a hardcoded exemption letting one code block a write whatever
$wgNeoWikiEnforceValidation says. ADR 26
decides the opposite: the setting "remains the master switch", "severity never blocks a write on its own",
"with enforcement off, no violation blocks", and its Consequences claim the carve-out is generalized "instead
of growing a parallel mechanism". That is the mechanism. No ADR authorises it, and #1265 amends ADR 23, not 26.

The exemption also did not work:

  • It reached two of the three write paths. UpdateStatementAction never consulted it, so the statement
    endpoint accepted what create and replace refused — and enforcement is off by default, so on a stock wiki
    that path had no guard at all.
  • On the replace path it was bypassable regardless: ViolationDiff identity is
    (propertyName, code, valuePartIndex) and ignores args, so swapping one unreachable target for a
    different one is not a new violation.

Nothing needs it. Such a relation already degrades wherever it is read: the Neo4j projection drops the edge,
the RDF projection drops the triple, resolution yields no Subject. So the violation keeps its error severity
and its reporting, and blocks under enforcement like every other error.

Removing it fixes both bugs above, since there is no longer an exemption to miss or to bypass.

ViolationDiff ignoring args is left alone: it is
#1267, and changing it would alter blocking
semantics for every violation type — editing one bad URL into a different bad URL would start blocking.

The rest

  • A foreign relation target is named under its own Source in every projection. The native projection
    learned this in Implement the Subject Sources foundation #1265; the ontology projection kept minting every target under this wiki's base, so an EDM
    or CIDOC-CRM export asserted this wiki owns entities that live elsewhere. The rule now has one home used by
    both projectors.
  • The frontend can read a Schema reference carrying a Source. targetSchema became string | {source, name}
    server-side, and the schema format documents the object form — but the frontend still declared a string, so
    following that documentation produced targetSchema.trim is not a function on blur and [object Object] in
    the picker. The type now admits what the server sends, and nothing is rewritten in transit, so a reference
    the server sent is the reference that goes back.
  • The subject→page index keys only the local Source's ids. Widening SubjectId::isValid() widened this
    filter with it, so a slot key naming another Source became a candidate row — in a binary(32) column a
    qualified id can overrun tenfold, truncating the key. ADR 32 says ids are keyed bare and that no row means
    "no local page holds this", which is already the right answer for a sourced Subject.
  • The two statement endpoints parse their Subject id. They were the last constructing SubjectId
    directly, so <wikiId>:<localId> was read as foreign and 404'd for a Subject that is right there.

One review finding withdrawn

The review suspected schemaContentSchema.json disagreed with the domain by anchoring a Source key with $
where SubjectId uses \z. It does not: the validator compiles patterns with PCRE's DOLLAR_ENDONLY, so
$ already matches only at the very end. Verified in the container — "otherwiki\n" is refused. No production
change; a test pins the agreement, since it rests on a compilation flag no test named.

Considered, omitted

  • Extracting the now-identical blocking predicate from the three write actions. After the revert they carry
    master's own shapes and what repeats is a one-line closure; extracting it would touch three actions, three
    factories and three test factories for no behaviour change, riding on a revert whose value is that its hunks
    vanish from the diff against master.
  • A frontend SchemaReference value object. A type alias and two pure functions cover every call site without
    a toJSON round-trip that only works by accident, and without normalising a value that must survive
    untouched.
  • Widening nwsp_subject_id instead of narrowing the filter. That is the right change when a Source's Subject
    is genuinely stored in a local slot; nothing does that today, and ADR 32 says otherwise.
  • Changing the Subject-level schema field's frontend handling beyond widening its type. The lossy collapse
    considered would have gone past the finding.

Documentation

Two published statements became false and were corrected; nothing else under docs/ is touched. The
translator note for the error severity returns to master's wording, which is true again.

Reviewing

One commit per concern. Every behavioural commit was seen failing first, and the three tests that could have
passed for the wrong reason were checked by mutating the fix and watching them flip. One test written during
this work was deleted rather than kept: it asserted the right thing but passed under mutation, because Vue
swallows the render error and the wrapper keeps stale props.

AI-authored — Claude Code, Opus 5 (1M context) (ultracode); review findings and this fix batch requested by @alistair3149, who chose to take the design finding alongside the bug fixes; diff not yet human-reviewed; phpcs, phpstan, 2126 non-Database PHPUnit tests, 1522 TypeScript tests, build and lint green locally, and all 20 CI checks green including the PHPUnit matrix across MW 1.43-1.46 and master.

Production notes

The review that produced these findings ran eight parallel passes (code-review, security-review,
review-tests, three design lenses, two second opinions), each blocking finding then put to three
refute-first verifiers; 56 raw findings became seven distinct ones. Two of the four fixed here were proven
with throwaway failing tests before being reported. The fix plans were written by six parallel planners and
cross-checked against each other; two were scoped down and one was withdrawn after its premise was tested in
the container.

ADR 26 makes $wgNeoWikiEnforceValidation the master switch: "Severity never
blocks a write on its own", "With enforcement off, no violation blocks", and
its Consequences claim the non-blocking carve-out is generalized "instead of
growing a parallel mechanism". Violation::alwaysBlocksWrites() was exactly such
a mechanism, hardcoding one code's exemption from that switch, and no ADR
authorised it.

The exemption also did not hold. It reached two of the three write paths:
UpdateStatementAction never consulted it, so the statement endpoint accepted
what the create and replace endpoints refused. On the replace path it was
bypassable anyway, because ViolationDiff identity is (propertyName, code,
valuePartIndex) and ignores args, so swapping one unreachable target for a
different one is not a new violation.

Nothing needs the guard. A relation into a Source this wiki cannot reach
already degrades wherever it is read: the Neo4j projection drops the edge, the
RDF projection drops the triple, and resolution yields no Subject.

So the violation keeps its error severity and its reporting, and blocks under
enforcement like every other error. The two predicates return to the shape
master had, which is the one UpdateStatementAction still carried, leaving the
three write paths consistent again.

The tests that observed the exemption move to enforcement-on, where their
assertions can still fail; without enforcement they would have passed for the
wrong reason. testAPreExistingUnresolvableTargetDoesNotBlockAnUnrelatedEdit
moves for the same reason: it would otherwise pass even with ViolationDiff
deleted.

Two published statements become false and are corrected, and the translator
note returns to master's wording, which is true again.
Every other Subject endpoint takes its id through SubjectIdParser, which is
where the local Source key is known and so the only place an id naming this
wiki explicitly can be recognized as the bare one it means. These two still
constructed a SubjectId directly, so "<wikiId>:<localId>" was read as a foreign
id and missed the subject-to-page index: a 404 for a Subject that is right
there.

Both handlers already turn InvalidArgumentException into a 400, which is what
parseOrThrow raises, so the failure path is unchanged. A malformed local id now
answers 400 rather than 404, matching the sibling endpoints.

The parameter descriptions said 15 characters starting with "s", which stopped
being the whole story when ids gained a Source; they now read as the six
handlers migrated earlier already do.
A relation property's targetSchema became string or {source, name} on the
server: the stored-schema validator accepts both, both serializers emit both,
and the schema format documents the object as the way to name a Schema from
another Source. The frontend still declared it a string, so an admin who
followed that documentation got an editor that threw
"targetSchema.trim is not a function" the moment the field was blurred, and a
picker showing [object Object] until it did.

The type now admits what the server sends, and the three surfaces that need a
name ask for one. Nothing is rewritten on the way through, so a reference the
server sent is the reference that goes back.

A Source-qualified Schema names nothing this wiki can resolve, so the Schema
picker offers no selection for one and the Subject picker behind a relation
field searches for nothing rather than searching this wiki for a Schema that
is not here.

Covered by unit tests for the three readers, including a local name containing
a colon, and by a component test that fails with the TypeError above when the
reader is removed.
ADR 32 says the index keys ids bare, as stored, and that no row means "no local
page holds this" rather than "does not exist". Widening SubjectId::isValid()
to the qualified (source, localId) form widened this filter with it, so a slot
key naming another Source became a candidate row — in a binary(32) column that
a qualified id can overrun by ten times, truncating the key instead of
recording it.

A Subject from another Source is fetched from that Source, not from a local
revision slot, so the absence of a row is already the right answer for it.

No shipped client can write such a key: creation refuses a non-local id and the
serializer writes canonicalized ones, so this only narrows what a hand-edited
or imported slot contributes.
The native projection learned to name a relation target of another Source under
that Source's base IRI, and to drop the triple when the Source is not
registered. The ontology projection kept minting every target under this wiki's
own base, so an EDM or CIDOC-CRM export asserted that this wiki owns entities
that live elsewhere — the export being the surface where that claim travels.

The rule now has one home, used by both projectors. Three sites in the ontology
projection consult it: the two that build a triple skip it when the target
cannot be named, and the one that maps a statement's values to objects leaves
such a target out of the list.

Both new tests fail if the rule is bypassed at any of those sites.
The review suspected the stored-schema grammar disagreed with the domain's,
because it anchors a Source key with $ where SubjectId uses \z. It does not:
the validator compiles patterns with PCRE's DOLLAR_ENDONLY, under which $
matches only at the very end, so "otherwiki\n" is already refused.

That agreement rests on a compilation flag no test named, so a change of
validator or of flags could reintroduce the gap in silence. This pins it. The
test fails if the anchor is dropped.

No production change: there was nothing to fix.
schemaReferenceText() was written for a surface that does not exist: no
component renders a Source-qualified Schema name, so its only caller was its
own test. This PR's own argument is that structure has to be paid for by a need
that exists now, and this one was not.

The blank the Schema picker shows for a foreign reference stands. The picker
selects Schemas of this wiki, so there is nothing for it to select, and the
stored reference is untouched either way. Showing the reference somewhere the
admin can read it is a display question this PR does not answer.
@alistair3149
alistair3149 marked this pull request as ready for review August 31, 2026 22:07
@JeroenDeDauw
JeroenDeDauw merged commit 4e1e241 into subject-sources-foundation Sep 1, 2026
13 checks passed
@JeroenDeDauw
JeroenDeDauw deleted the pr-1265-review-fixes branch September 1, 2026 22:36
@JeroenDeDauw

Copy link
Copy Markdown
Member

Fast-forwarded into subject-sources-foundation in full — all seven commits taken, including the enforcement-switch revert, whose ADR 26 argument holds. The #1265 description now reflects the reverted semantics.

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.

2 participants