Skip to content

Point the Validate REST descriptions at the real subject-format doc#1037

Open
alistair3149 wants to merge 1 commit into
masterfrom
fix/validate-subject-format-doc-path
Open

Point the Validate REST descriptions at the real subject-format doc#1037
alistair3149 wants to merge 1 commit into
masterfrom
fix/validate-subject-format-doc-path

Conversation

@alistair3149

Copy link
Copy Markdown
Member

ValidateSubjectApi and ValidateSubjectUpdateApi tell API consumers that their statements shape "matches docs/SubjectFormat.md". No such file has ever existed — not before the docs reorganisation either — so the generated OpenAPI spec has been serving a dead reference. Their siblings CreateSubjectApi and ReplaceSubjectApi cite the same document and were corrected to docs/api/subject-format.md in #1034, which left two of the four subject-format citations pointing nowhere.

Point both at docs/api/subject-format.md, and add DocsPathReferencesTest to guard the class of bug: it resolves every docs/….md path cited anywhere in src/ against the docs tree and fails on any that does not exist.

Why a test for two strings

Nothing checked these strings, which is how a path that never existed could be advertised in the public API spec indefinitely. It also gives the reorganised docs a backstop: RestApiDocsCoverageTest already guards the endpoint table against extension.json, but nothing guarded code-to-docs citations, and #1034 moved every doc in the tree.

Worth noting for the record: #1035 states that "an exhaustive resolver over every docs/…md reference in all tracked file types confirms every target exists". These two references survived it — the resolver matched markdown and wikitext link syntax, not bare paths inside PHP string literals. The test added here is the version of that check that runs in CI.

Verification

Run against the dev stack (MediaWiki container), not only CI:

  • DocsPathReferencesTest was seen failing first, naming exactly the two offenders:
    0 => 'src/EntryPoints/REST/ValidateSubjectApi.php:66 cites docs/SubjectFormat.md'
    1 => 'src/EntryPoints/REST/ValidateSubjectUpdateApi.php:71 cites docs/SubjectFormat.md'
    
    and passing after the fix: OK (1 test, 274 assertions).
  • Green: ValidateSubjectApi, ValidateSubjectUpdateApi, CreateSubjectApi, ReplaceSubjectApi, RestApiDocsCoverage, and ModuleSpecHandlerNeoWiki (the OpenAPI spec test, 465 assertions).
  • make phpcs clean. make stan reports 5 errors, all in src/Infrastructure/Rdf/HardfRdfStreamWriter.php (unresolved hardf symbols). That file is byte-identical to master, so they are pre-existing and unrelated to this change.

Result of a review of #1034 and #1035 requested by @alistair3149. The dead reference was surfaced by that review; including the guard test rather than only fixing the two strings was his call.
Context: the NeoWiki docs tree, the REST entry points, and the running dev stack.
Written by Claude Code, Opus 4.8 (1M context)

`ValidateSubjectApi` and `ValidateSubjectUpdateApi` told API consumers that their `statements`
shape "matches docs/SubjectFormat.md". No such file has ever existed, so the OpenAPI spec has been
serving a dead reference. Their siblings `CreateSubjectApi` and `ReplaceSubjectApi` cite the same
document and were corrected to `docs/api/subject-format.md` in #1034, which left two of the four
subject-format citations pointing nowhere.

Point both at `docs/api/subject-format.md`, and add `DocsPathReferencesTest` to guard the class of
bug: it resolves every `docs/….md` path cited anywhere in `src/` against the docs tree and fails on
any that does not exist. Seen failing on exactly the two offenders before the fix.

Nothing else checked these strings, which is why a doc that never existed could be advertised in the
public API spec indefinitely.

Follows-up to #1035

> Result of a review of #1034 and #1035 requested by @alistair3149; this gap was found by the review
> and confirmed against the dev stack, and the guard test was his call to include.
> Context: the NeoWiki docs tree and the REST entry points.
> <sub>Written by Claude Code, `Opus 4.8 (1M context)`</sub>
@alistair3149
alistair3149 marked this pull request as ready for review July 13, 2026 16:55
@malberts

malberts commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Two robustness limitations in DocsPathReferencesTest, noted for the record rather than as a change request. The current form is correct for everything in src/ today; flagging in case either pattern shows up later. I reproduced each by injecting the trigger into a src/ file and running the test unchanged (baseline is green at 274 assertions).

1. Relative ../ prefixes are resolved as root-relative. The regex matches from docs/ onward, so any ../ prefix is discarded and every match is checked against the repo root. Correctness therefore depends on the citing file sitting at a depth where the ../ count cancels out. There's exactly one such reference in the tree today: src/Domain/Mapping/Mapping.php:12, whose [ADR 17](../../../docs/adr/017-names-as-identifiers.md) link resolves only because that file is three directories deep, so ../../../ happens to land on the repo root. Copy or move that citation to a file at a different depth and it silently breaks (row 2 below). Both directions misbehave:

Injected into a src/ file The reference is actually Test verdict
../docs/guide.md from src/EntryPoints/REST/… (target exists one dir up) valid fails…_probe_rel.php:2 cites docs/guide.md
../../../docs/adr/017-names-as-identifiers.md from src/… (points above the repo root) broken passes — dangling ref not caught

2. The pattern matches anywhere in a file, including URLs. \bdocs/[…]\.md fires after any /, so a string that isn't a repo doc citation at all is treated as one:

Injected into a src/ file The reference is actually Test verdict
https://docs.example.com/docs/nonexistent-guide.md an external URL, not a citation fails…_probe_url.php:2 cites docs/nonexistent-guide.md

In practice the production-facing citations this test exists to guard — the OpenAPI param descriptions — are all written as root-relative docs/… paths, which is the form the resolver handles correctly. So (1) really only concerns developer-facing ../ links in comments, of which Mapping.php is the sole current example. Neither limitation bites today: the only two paths the regex extracts from src/ both resolve. If it becomes relevant, resolving ../-prefixed matches relative to each file's own directory closes the first, and restricting matching to string literals closes the second.

AI-authored — Claude Code, Opus 4.8 (1M context); from a review of #1037 requested by @malberts; each case above reproduced by running DocsPathReferencesTest unchanged against the PR branch in a throwaway MediaWiki container (baseline green, 274 assertions).

@malberts

malberts commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

A note on a possible follow-up, separate from this PR's dead-path fix.

Pointing these PARAM_DESCRIPTION strings at the public docs site rather than a repo path may be worth considering. They're served in the generated OpenAPI spec, so the audience is API consumers reading that spec (Swagger UI, codegen, and so on), who don't have the extension checked out. A repo-relative docs/api/subject-format.md is only resolvable with the source tree in hand; the same page is published at https://neowiki.ai/docs/api/subject-format, which is directly clickable for that reader.

Known caveats if we go that way:

  • Version skew. The public site always serves the current docs, whereas a deployed extension can be any older version. An older install would then point consumers at documentation that may describe a newer format than the one it actually implements. A repo-relative path avoids that, since it travels with the code at that version.
  • Availability. An external site is a dependency: if neowiki.ai is down, the links break.

At MVP neither is a real concern: there's effectively one active version and the docs track the current code, so there's no meaningful drift; and a site outage is rare and would likely coincide with bigger problems anyway.

Future options if these start to matter:

  • Version the links. Once the REST API carries an explicit version, the doc URL can carry the same segment (e.g. https://neowiki.ai/docs/api/v1/subject-format), and a non-backwards-compatible change moves to a new version with its own docs. An older install then always resolves to the docs for the API version it actually implements. This addresses version skew.
  • Link to the source on GitHub at a pinned ref. Pointing at the file at a fixed tag or SHA (e.g. https://github.com/ProfessionalWiki/NeoWiki/blob/<release>/docs/api/subject-format.md) gives all three properties at once: GitHub's uptime covers availability, the pinned ref matches the version the extension was built from, and GitHub renders the .md nicely. The cost is a GitHub dependency and knowing the release ref at runtime to build the link.
  • Serve the doc from the wiki's own host. A link built relative to the deployed extension (server URL + extension assets path) is served from the same host as the API, so it survives a neowiki.ai outage, and it resolves to the exact shipped version's docs, which folds in the version-skew fix for free. The catch is per-install web-server config: extension directories are often hardened or placed outside the docroot, so /extensions/NeoWiki/docs/…md may 403/404, and even when served, .md is delivered as raw text rather than a rendered page. So the URL is easy to generate but not guaranteed to resolve or read well across installs.

AI-authored — Claude Code, Opus 4.8 (1M context); from a review of #1037 with @malberts; confirmed https://neowiki.ai/docs/api/subject-format renders live before writing this.

@malberts

malberts commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

I lean slightly towards using external links instead of partial links, for the reasons above. However, that affects how the tests are implemented, so if we go that route then I don't think it makes sense to merge this and then rewrite it again. Otherwise, if everyone objects to external links (or if simply not worth more effort until somebody asks for it), then we can merge this already.

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