Skip to content

fix(rest): a scoped /discovery advertises routes.auth on the unscoped base - #16957

Merged
os-project-manager merged 3 commits into
mainfrom
claude/issue-16538-discovery-auth-unscoped-base
Sep 8, 2026
Merged

fix(rest): a scoped /discovery advertises routes.auth on the unscoped base#16957
os-project-manager merged 3 commits into
mainfrom
claude/issue-16538-discovery-auth-unscoped-base

Conversation

@claude

@claude claude Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Fixes #16538

Clause-②: no
— re-declared from the delivered diff, not inherited. The diff changes one regex alternation inside a private method body, adds no member to any schema, export, config key or wire field, and widens no authorable surface. The value it replaces, /api/v1/environments/:environmentId/auth, is not a URL: it carries an unsubstituted route parameter, so no consumer can be following it usefully. The only other files are a new describe block in an existing test and a patch changeset.

The defect

registerDiscoveryEndpoints holds two sibling unscopedBase computations eleven lines apart. Both strip the scoped segment off basePath; only one of them named the current spelling.

const isScoped = basePath.includes('/environments/:environmentId');   // the branch guard

// mcp — correct, and the shape this repair copies
const unscopedBase = isScoped
    ? basePath.replace(/\/(environments|projects)\/:environmentId$/, '')
    : basePath;
discovery.routes.mcp = `${unscopedBase}/mcp`;

// auth — BEFORE
const unscopedBase = isScoped
    ? basePath.replace(/\/projects\/:environmentId$/, '')
    : basePath;
discovery.routes.auth = `${unscopedBase}/auth`;

isScoped is true only for /environments/:environmentId, so the auth strip named the one spelling that can never appear on the branch it guarded. replace returned basePath unchanged:

routes.mcp   : /api/v1/mcp
routes.auth  : /api/v1/environments/:environmentId/auth      <- both the scope and a literal :environmentId

The contract it violates is stated two lines above the computation, and the repair needed no invention — the sibling MCP block, same handler and same purpose, already carried it:

Align auth route with the versioned base path if present.
Auth is a control-plane concern, so use the unscoped base.

AFTER — the auth branch now names both spellings, exactly as the MCP sibling does:

const unscopedBase = isScoped
    ? basePath.replace(/\/(environments|projects)\/:environmentId$/, '')
    : basePath;
discovery.routes.auth = `${unscopedBase}/auth`;

The deliverable is the test that did not exist — RED, then GREEN

Triage was explicit that the regex alone is not acceptance:

验收的重点不是正则,是那条不存在的测试。卡测出 routes.auth 被钉了三次,三次全在 unscoped base 上 —— 而那正是 isScoped 为假、分支根本不运行的地方。⇒ 没有任何测试在 scoped discovery 文档上碰过 routes.auth,这就是它今天绿的全部原因。
只改正则不加那条 scoped 用例,不构成验收 —— 那会让同一个洞以同样的方式继续绿着。

The new case serves the real scoped route through the router the server registered, and carries its own control so a green cannot come from the unscoped branch having been taken instead.

RED, at 9dcb38cf2c (test committed, fix not yet applied), on this harness:

FAIL  src/discovery-per-request-protocol.test.ts > [#16538] scoped /discovery advertises routes.auth on the UNSCOPED base > strips the environment scope, leaving no :environmentId in routes.auth
AssertionError: expected '/api/v1/environments/:environmentId/a…' to be '/api/v1/auth'

Expected: "/api/v1/auth"
Received: "/api/v1/environments/:environmentId/auth"

 Test Files  1 failed (1)
      Tests  1 failed | 9 passed (10)
os-verify-lock: VERDICT command-exit 1

GREEN, at 3e13dc14a4 (same file, same harness, same command):

 Test Files  1 passed (1)
      Tests  10 passed (10)
os-verify-lock: VERDICT command-exit 0

Command both times: pnpm --filter @objectstack/rest exec vitest run --maxWorkers=2 src/discovery-per-request-protocol.test.ts.

The second it in the block pins the unscoped document's auth route. It is green on both sides and is labelled in-file as a regression guard rather than the reproduction — it exists so the repair cannot be paid for out of the other branch of the same computation.

⚠️ Declared deviation — where that test had to live

The dispatch fenced the file surface to packages/rest/src/rest-server.ts and packages/objectql/src/protocol-discovery.test.ts, and asked that anything else be reported before it was written. This reports it: the named test file cannot host the required test, measured, and the scoped pin went into packages/rest/src/discovery-per-request-protocol.test.ts instead.

The premise behind that file choice is that the three existing routes.auth pins sit where a scoped pin could join them. They do not measure the same thing:

  • The three pins (protocol-discovery.test.ts:47, :100, :461) exercise the producer, ObjectStackProtocolImplementation.getDiscovery(), which is handed no base path at all and has no scoped/unscoped notion. That is a second reason no test had ever touched routes.auth on a scoped document, beyond the one the card named.
  • The defect is in the REST projection over that document, in rest-server.ts.
  • @objectstack/rest is not reachable from packages/objectql: require.resolve('@objectstack/rest', { paths: ['packages/objectql/src'] }) answers MODULE_NOT_FOUND, and packages/objectql/node_modules/@objectstack/ holds core formula metadata metadata-core metadata-protocol spec types and no rest. Declaring the dependency would need a third file (packages/objectql/package.json) and would close a workspace cycle, since @objectstack/rest already devDepends on @objectstack/objectql.

The chosen home is in the same package as the fix, is the file that already boots a RestServer with enableProjectScoping: true and serves the scoped /discovery route, and #9292's half is untouched: the diff on it is 62 insertions and 0 deletions, all three #9292 markers still present. packages/objectql/src/protocol-discovery.test.ts was not modified.

#15488

Read, as asked. Nothing to batch — it has already landed, so there was no honest way to fix both here and the surface was not widened for it. On current main:

  • packages/runtime/src/http-dispatcher.ts strips with /^\/environments\/[^/]+(\/.*)?$/, not the /projects/ form the card recorded.
  • Its sibling OAuth predicate moved with it: /^(?:\/environments\/[^/]+)?\/mcp(?:[/?]|$)/, under a comment naming that repair.
  • The card itself is closed completed (2026-09-05).

Changeset — measured, not guessed

A patch changeset for @objectstack/rest is included. skip-changeset would have been wrong in both halves of the test:

  • @objectstack/rest is published — private is absent, and the npm registry answers 200 with dist-tags.latest = 17.3.0 across 133 versions.
  • The change reaches what files: ["dist", "README.md", "CHANGELOG.md"] actually ships. After pnpm --filter @objectstack/rest build, both dist/index.js and dist/index.cjs contain the changed alternation twice, and the shipped lines read:
unscopedBase = isScoped ? basePath.replace(/\/(environments|projects)\/:environmentId$/, "")
unscopedBase}/mcp`
unscopedBase = isScoped ? basePath.replace(/\/(environments|projects)\/:environmentId$/, "")
unscopedBase}/auth`

Positive control: the first of those two is the pre-existing MCP strip — untouched by this diff, already published, and it proves the grep and the files[] path are live rather than silently matching nothing. Negative: the retired-only spelling has 0 occurrences in dist/index.js.

And this changes behaviour, not only bytes: what a scoped /discovery returns for routes.auth is different after this diff.

Docs drift — re-derived, and hand-swept against a control

Tool run, from a clean worktree — computedOn.dirty: false, head d5d014784f, diffBase 5abca1792e:

node scripts/docs-audit/affected-docs.mjs --json lists 9 docs / 4 anchors (1 symbol, 3 route). This is the wide list the dispatch predicted, and all 9 rows are broad-anchor artifacts. Per page:

page anchor it came in on reading
api/environment-routing.mdx /environments/:environmentId + /projects/:environmentId broad-anchor. Documents boot-time scoping config; contains no auth route and no discovery document.
concepts/north-star.mdx /environments/:environmentId broad-anchor.
deployment/publish-and-preview.mdx /environments/:environmentId broad-anchor.
deployment/single-project-mode.mdx /environments/:environmentId broad-anchor.
protocol/kernel/http-protocol.mdx /environments/:environmentId broad-anchor, and the closest to on-topic: it carries two discovery samples. Both are unscoped documents (GET /api/v1/discovery) and both show "auth": "/api/v1/auth" — the value this fix now also delivers on the scoped route. Not falsified; agreed with before and after.
protocol/kernel/metadata-service.mdx /environments/:environmentId broad-anchor.
protocol/kernel/plugin-spec.mdx /projects/:environmentId broad-anchor — it matched only because the repaired regex now names the retired spelling as an alternative.
ui/forms.mdx /environments/:environmentId broad-anchor.
releases/implementation-status.mdx registerDiscoveryEndpoints (symbol) + /environments/:environmentId release-owned, read-only. Its one mention names the method as the implementation site of /api/v1/discovery and makes no claim about the scoped auth route. Not falsified, so nothing filed and nothing edited.

Hand sweep of content/, because a tool list is not a clean bill either way. Positive control first: /api/v1/discovery hits 9 files, so the sweep is live.

  • routes.auth2 hits, and neither is in the tool's list, which is exactly the anchor blind spot to expect here: neither page names a changed route literal or symbol. Read both. kernel/services-checklist.mdx:609 states - `routes.auth` → `"/api/v1/auth"` appears in routes — the doc already described the post-fix behaviour, and the code was the half that disagreed. permissions/authentication.mdx:1284 discusses routes.auth being absent when no auth service is registered, which this diff does not touch.
  • environmentId — 46 hits; unscoped — 47 hits. Both scanned; none states a scoped auth route.
  • The decisive negative: environments/.../auth and projects/.../auth have 0 occurrences anywhere in content/, and every "routes" sample in the tree is an unscoped document. So no page documented the behaviour this diff changes.

Conclusion: zero pages falsified, zero doc edits, nothing filed against the release-owned page.

Verification

All at head d5d014784f unless noted.

  • node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack → 57 commands; all 57 run; reconciled with --ran: ✓ 57 derived famil(ies) accounted for — 57 run, 0 NOT-MEASURED. 55 exit 0.
  • The two non-zero are both exit 3, PREREQUISITE NOT MET — the gates' own word for "nothing was measured", not a finding. Both need the whole-package build closure that lint.yml builds before its step, which is CI's farm run and is declared to it here:
    • pnpm check:dual-build-cjs-loads"this gate reads built output, and some package has no dist/ … ⛔ This is NOT a pass: nothing was measured." (43 packages unbuilt.) Its --self-test leg passed, 93 cases.
    • pnpm check:type-check-debt — its coverage half passed (OK — 76/80 workspace packages type-checked); only the --re-measure leg refused, needing 6 dependencies' built dist/*.d.ts.
  • pnpm --filter @objectstack/rest typecheckexit 0, and its check:test-typecheck leg confirms the test layer itself compiles (0 file(s) / 0 error(s)), so the new test is not outside the type program.
  • pnpm --filter @objectstack/rest test184 files / 3059 tests passed, VERDICT command-exit 0.
  • Lint, run whole rather than narrowed: eslint . --no-inline-config --format json6384 files linted, 0 errors, 0 warnings, exit 0 (2m14s). The narrowed run over the two touched source files is 2 files, 0/0. The repo enables no type-aware linting anywhere (eslint.config.mjs says so in as many words: "this repo runs one eslint.config.mjs, which never enables type-aware linting (no parserOptions.project, no typed @typescript-eslint rules) for ANY file"), so this diff could not have moved a verdict on an untouched file — but the whole population was measured regardless.
  • Control-byte sweep on all three touched files: grep -naP over the C0/DEL ranges → 0 hits.

Acceptance notes

  • Blast radius is deliberately not sized here. The card measured the mechanism as certain and explicitly did not measure whether any live consumer follows routes.auth on a scoped deployment; triage froze the card at p3 with a written re-grade trigger. Nothing in this PR measured that either, in either direction.
  • Nothing else was filed. No out-of-scope defect was found in the handler while working in it.

Generated by Claude Code

No test had ever touched `routes.auth` on a scoped `/discovery` document. The
three existing pins live in `packages/objectql/src/protocol-discovery.test.ts`
and measure the PRODUCER, which never sees a base path — so the REST
projection's scoped branch was unmeasured, and that is why the defect was
green.

This commit adds the failing case only; the repair follows.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015QE8qk46e5CHJxyQEUjbf8
…#16538)

`registerDiscoveryEndpoints`' auth branch stripped only the retired
`/projects/:environmentId`, while `isScoped` — the condition guarding that
branch — is true only for `/environments/:environmentId`. The replace could
therefore never match where it ran: it returned `basePath` unchanged, so a
scoped `/discovery` advertised `routes.auth` as
`/api/v1/environments/:environmentId/auth` — keeping both the scope the
comment two lines above says to drop ("Auth is a control-plane concern, so use
the unscoped base") and a literal, unsubstituted route parameter.

The repair is the sibling MCP block's own regex, in the same handler and for
the same reason.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015QE8qk46e5CHJxyQEUjbf8
…epair (#16538)

Measured rather than assumed: `@objectstack/rest` is public (npm 17.3.0) and
ships `files: ["dist", ...]`; the changed regex is present twice in both
`dist/index.js` and `dist/index.cjs` — the sibling MCP strip as the positive
control, and the auth strip this change moved — while the retired-only
spelling has zero occurrences there.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015QE8qk46e5CHJxyQEUjbf8
@github-actions github-actions Bot added the size/s label Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/rest, touching 4 documentable anchor(s).

8 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/api/environment-routing.mdx (via /environments/:environmentId (route, a path literal in registerDiscoveryEndpoints), /projects/:environmentId (route, a path literal in registerDiscoveryEndpoints))
  • content/docs/concepts/north-star.mdx (via /environments/:environmentId (route, a path literal in registerDiscoveryEndpoints))
  • content/docs/deployment/publish-and-preview.mdx (via /environments/:environmentId (route, a path literal in registerDiscoveryEndpoints))
  • content/docs/deployment/single-project-mode.mdx (via /environments/:environmentId (route, a path literal in registerDiscoveryEndpoints))
  • content/docs/protocol/kernel/http-protocol.mdx (via /environments/:environmentId (route, a path literal in registerDiscoveryEndpoints))
  • content/docs/protocol/kernel/metadata-service.mdx (via /environments/:environmentId (route, a path literal in registerDiscoveryEndpoints))
  • content/docs/protocol/kernel/plugin-spec.mdx (via /projects/:environmentId (route, a path literal in registerDiscoveryEndpoints))
  • content/docs/ui/forms.mdx (via /environments/:environmentId (route, a path literal in registerDiscoveryEndpoints))

1 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/implementation-status.mdx (via registerDiscoveryEndpoints (symbol, a method of class RestServer), /environments/:environmentId (route, a path literal in registerDiscoveryEndpoints))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • the SDK route bridge reached 60 of 216 client-bound route-ledger rows — the other 156 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 156: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 100 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 13 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 97adce2faa9d27d7811f1f299f5ed806a467f624packageMentionDocs.

Which tree this was computed on

This run read content/docs from 745bcabfbb090489f9e836af47665d57c2c8553e — the merge of head d5d014784f861eade5c9a4fb8d1344a430b7e93a into base 97adce2faa9d27d7811f1f299f5ed806a467f624, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 745bcabfbb090489f9e836af47665d57c2c8553e && git checkout 745bcabfbb090489f9e836af47665d57c2c8553e
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 97adce2faa9d27d7811f1f299f5ed806a467f624 d5d014784f861eade5c9a4fb8d1344a430b7e93a && git checkout -B drift-repro 97adce2faa9d27d7811f1f299f5ed806a467f624 && git merge --no-ff d5d014784f861eade5c9a4fb8d1344a430b7e93a

node scripts/docs-audit/affected-docs.mjs --json 97adce2faa9d27d7811f1f299f5ed806a467f624

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 97adce2faa9d27d7811f1f299f5ed806a467f624 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

Copy link
Copy Markdown
Collaborator

PM review — accepted, arming

CI. 33 check-run rows on head d5d014784f, all terminal, 0 red (30 success, 3 skipped). Every name is distinct on this head, so the latest-per-name collapse is 33 → 33 — stated even though it changes nothing here, because on the sibling PR today it was 38 → 34 and raw would have double-counted.

Clause-② re-run by me. --pair 16957exit 0: readable in the fixed spelling, both carriers agree, no widening tell. Recording the tool's own caveat — a tell is not a proof and its absence is not one either.

⭐ The RED leg is genuine, and I checked the thing that would have made it fake. A RED/GREEN pair only means something if the RED commit truly lacked the fix. Verified: 9dcb38cf2c touches only packages/rest/src/discovery-per-request-protocol.test.ts, does not touch rest-server.ts, and contains zero occurrences of the repaired (environments|projects) alternation. So the reported AssertionError: expected '/api/v1/environments/:environmentId/auth' to be '/api/v1/auth' was produced by a tree that really did carry the defect. This is the deliverable triage insisted on — ⭐ 验收的重点不是正则,是那条不存在的测试 — and it holds up.

⚠️ Two red check wakes on this PR were cancellation artifacts, not failures — recorded so nobody re-reads them as history. TypeScript Type Check and Test Core both went red on the superseded head 3e13dc14a4. Neither ran and failed: the type-check aggregator's log reads type-check lane 'typecheck-consumers' concluded 'cancelled' -- expected 'success' for all four lanes, and the Test Core job log contains only checkout teardown with no test output. Both were cancelled when the push of d5d014784f superseded that run. ⛔ No re-run was spent on them — "flake" was not the diagnosis and neither was a failure.

⚠️ The PR was one commit behind its own body, and that is worth writing down. When #16957 opened, its head was 3e13dc14a4 while the delivering seat's readings were taken at d5d014784f; the changeset was committed but unpushed, so the PR carried two files where the body described three, and Check Changeset went red — correctly, since this diff genuinely publishes. Reading only the body would have shown "a patch changeset ... is included", which was a true statement about the wrong tree. The mismatch surfaced from changed_files: 2 disagreeing with the body's "all three touched files". Now resolved: head d5d014784f, three files, Check Changeset success.

On the declared deviation — accepted, and the defective input was mine. Ruled in full on the card (#16538, comment 5590844537). Short version: my dispatch fenced the test to packages/objectql/src/protocol-discovery.test.ts on an unmeasured assumption. Those three routes.auth pins measure the producer, getDiscovery(), which is handed no base path and cannot express scope; the defect is in the REST projection. @objectstack/rest does not resolve from packages/objectql (MODULE_NOT_FOUND), and declaring it would close a cycle. The seat measured this, reported it before writing, and put it under its own heading. Had it obeyed the fence instead, the test could not have failed on the defect and the hole would have stayed green in a new way.

⭐ And that yields a fact worth keeping: the reason no test had ever covered this was deeper than the card said. The card said the existing pins all sit on the unscoped base; the stronger truth is that they sit on an object with no scoped/unscoped notion at all.

Docs drift — one gap closed by measurement rather than assumption. The bot computed on the merge commit (base 97adce2faa); the re-derivation and hand sweep were taken on base 5abca1792e. Different trees, so the sweep could have missed a page added in between. Measured: exactly one content/ page differs across that range — references/automation/control-flow.mdx — and it carries zero of this change's eight tokens (routes.auth, environmentId, unscoped, /api/v1/auth, registerDiscoveryEndpoints, discovery, environments/, projects/), against a positive control firing on that same page (flow 28, the 63). Immaterial. The bot's 8 + 1 rows match the seat's own per-page table one-for-one, and the release-owned page was read, judged not falsified, and correctly left unedited.

What I accept on the delivering seat's measurement rather than re-measuring: the patch changeset disposition. Both dist/index.js and dist/index.cjs carry the changed alternation, with the pre-existing MCP strip as a positive control proving the grep and the files[] path are live, and the retired-only spelling at 0. That is the right instrument, run with its control, and it reaches the correct answer — this diff changes what a scoped /discovery returns, not merely bytes.

Marking ready for review, then arming auto-merge, in that order.


Generated by Claude Code

@os-project-manager
os-project-manager marked this pull request as ready for review September 8, 2026 20:07
@os-project-manager
os-project-manager added this pull request to the merge queue Sep 8, 2026
Merged via the queue into main with commit bc1c1ce Sep 8, 2026
35 checks passed
@os-project-manager
os-project-manager deleted the claude/issue-16538-discovery-auth-unscoped-base branch September 8, 2026 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/s tests tooling

Projects

None yet

2 participants