Skip to content

test(openapi): write the route auth-parity meta-test both spec tables are exported for (#9851) - #9856

Merged
JSONbored merged 1 commit into
mainfrom
feat/priority-label-author-eligibility
Jul 29, 2026
Merged

test(openapi): write the route auth-parity meta-test both spec tables are exported for (#9851)#9856
JSONbored merged 1 commit into
mainfrom
feat/priority-label-author-eligibility

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Closes #9851.

The gap

Two files each export their full spec table with a comment naming the consumer:

"Exported for the meta-test that asserts every entry's declared auth matches the middleware that actually gates it."

That test did not exist. Neither symbol was referenced anywhere outside its own file — so the auth on every ORB, internal, app and repo route was unverified against what the route actually enforces, while two exports were maintained for a guarantee nobody had built. (Found by the dead-export sweep in #9852, which is what makes that check worth having.)

What it asserts

Behaviour, not wiring — deliberately. Auth here is enforced inside handlers; only /v1/internal/* has a path-scoped middleware, so "read the middleware chain" has nothing uniform to read. And a chain that exists proves less than a route that actually refuses. So both directions run against a booted app:

  • a route declaring it needs a credential must refuse without one (401/403);
  • a route declaring itself public must not answer 401.

Plus two guards on the check itself: both tables must be non-empty (a watcher that watches nothing passes forever), and every auth value must be one the document knows how to render.

Getting it honest took three corrections

Each is now a comment in the file, because each is a way this test could have passed while proving nothing:

  1. The relay and webhook handlers reject a request they cannot identify before verifying its signature. A bare request never reached the auth check. It now sends the GitHub identifying headers and nothing that authenticates — which is what proves the signature is required.
  2. Three broker routes 404 until ORB_BROKER_ENABLED, and the relay 404s on an instance with no enrollment secret. The env makes them reachable rather than skipping them — a flag-gated route still has a declared auth to honour.
  3. Both directions share one env. They didn't at first, and the mutation check caught it: with a different env a flag-gated route 404s in the public direction, so a wrongly-public declaration passed silently.

Verified by mutation: flipping a real entry's auth to public fails the suite with POST /v1/orb/token declares auth=public but answered 401. Before the third fix, that same mutation passed — which is exactly the failure mode this issue is about.

Result

No production behaviour changes. Every current route already honours its declaration — which is worth knowing, and was not knowable before.

A 400 from a signature-authenticated route counts as a refusal: this test cannot synthesize a valid payload for every webhook shape, and what it still proves is that an unsigned request never succeeds.

Note

SpecEntry is declared separately in each of the two spec files and the copies have already diverged (the internal one grew a request field, the ORB one did not). Not touched here to keep this PR to the missing guarantee, but it is the same hand-mirrored-type class the MCP epic just finished removing elsewhere — worth its own change.

… are exported for (#9851)

`ORB_AND_CONTROL_ROUTE_SPECS` and `INTERNAL_AND_PUBLIC_ROUTE_SPECS` were each exported with a comment
naming their consumer -- "the meta-test that asserts every entry's declared auth matches the
middleware that actually gates it" -- and that test did not exist. Two exports maintained for a
guarantee nobody built, while the `auth` on every ORB, internal, app and repo route went unverified.

It asserts BEHAVIOUR rather than wiring, deliberately. Auth here is enforced inside handlers (only
`/v1/internal/*` has a path-scoped middleware), so "read the middleware chain" has nothing uniform to
read -- and a chain that exists proves less than a route that actually refuses. So both directions are
exercised against a booted app: a route declaring it needs a credential must REFUSE without one, and a
route declaring itself public must not answer 401.

Getting it honest took three corrections, each of which is now a comment in the file:

- The relay and webhook handlers reject a request they cannot IDENTIFY before verifying its signature,
  so a bare request never reached the auth check at all. The test now sends the GitHub identifying
  headers and nothing that authenticates, which is what proves the signature is required.
- Three broker routes 404 until `ORB_BROKER_ENABLED`, and the relay 404s on an instance with no
  enrollment secret. The env now makes them REACHABLE instead of skipping them -- a flag-gated route
  still has a declared auth to honour.
- Both directions share one env. They did not at first, and the mutation check caught it: with a
  different env a flag-gated route 404s in the public direction, so a wrongly-public declaration
  passed. Verified by flipping a real entry's auth to `public` and watching the suite fail.

A 400 from a signature-authenticated route is accepted as a refusal: this test cannot synthesize a
valid payload for every webhook shape, and what it still proves is that an unsigned request never
succeeds.

No production behaviour changes. Every current route already honours its declaration -- which is worth
knowing, and was not knowable before.
@loopover-orb

loopover-orb Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-29 14:54:21 UTC

1 file · 1 AI reviewer · no blockers · readiness 95/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This PR adds an integration test that closes the gap flagged in #9851: two exported spec tables (`ORB_AND_CONTROL_ROUTE_SPECS`, `INTERNAL_AND_PUBLIC_ROUTE_SPECS`) claimed to feed a meta-test that never existed. The new test sends unauthenticated requests to every non-public route and asserts a 401/403, and unauthenticated requests to public routes and asserts non-401, with reasoned carve-outs for webhook signature-identification headers, 404 (unmounted route), and flag-gated broker/relay reachability. The design choices (asserting behavior rather than middleware wiring, shared reachability env, explicit webhook 400 carve-out) are well-justified in the comments and match the described corrections in the PR body.

Nits — 6 non-blocking
  • The 120_000ms timeout per test (route-auth-parity.test.ts:69,113) is generous for what should be fast in-process requests; worth checking if it's needed or just defensive padding.
  • concretePath() (route-auth-parity.test.ts:37) replaces all `{param}` segments with the same literal `route-auth-parity` for every param in a path, which could coincidentally collide with a route validation branch expecting distinct values (e.g. owner vs repo) — likely harmless given the test only checks status codes, but worth a one-line comment noting that's intentional.
  • The `known` auth-value set in the second test (route-auth-parity.test.ts:57) duplicates the `RouteAuth` union already defined in src/openapi/define-route.ts; importing the type/union instead of re-declaring the literal set would keep the two in sync automatically.
  • Consider importing `RouteAuth` from src/openapi/define-route.ts to derive the `known` set instead of hand-listing the literals, so a future auth kind addition can't silently pass this test while failing type-checking elsewhere.
  • A brief comment on why `route-auth-parity.test.ts:100` uses `Promise.resolve(app.request(...))` (probably to normalize a Response|Promise<Response> return) would help future readers.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #9851
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 14 registered-repo PR(s), 13 merged, 345 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 14 PR(s), 345 issue(s).
Improvement ℹ️ Insufficient signal risk: clean · value: insufficient-signal
Linked issue satisfaction

Addressed
The PR adds test/integration/route-auth-parity.test.ts, which imports both ORB_AND_CONTROL_ROUTE_SPECS and INTERNAL_AND_PUBLIC_ROUTE_SPECS (finally consuming the two exports named in the issue), boots the app, and asserts each entry's declared auth matches actual enforcement in both directions (must-refuse and must-not-refuse), including flag-gated/reachability handling and non-empty-table guards.

Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is registered but has no active allocation in the current snapshot.
  • Public profile languages: Python, TypeScript, Ruby, Go, MDX, Shell, Solidity, JavaScript
  • Official Gittensor activity: 14 PR(s), 345 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Then work through the remaining 2 steps in the Signals table above.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

Decision record
  • action: hold · clause: success
  • config: b13578ccec9172e4a7fefaf204bec88ada35175bdb0204c7a6fd879801e376af · pack: oss-anti-slop · ci: passed
  • record: 48f6f3eb3f5cf96260821f553e19ee1f38e0b72a3d630fc7d7402493a27e508a (schema v5, head 524c577)

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.77%. Comparing base (60d900a) to head (524c577).
⚠️ Report is 5 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9856      +/-   ##
==========================================
- Coverage   91.64%   90.77%   -0.88%     
==========================================
  Files         916      916              
  Lines      112753   112753              
  Branches    27085    27085              
==========================================
- Hits       103338   102347     -991     
- Misses       8126     9315    +1189     
+ Partials     1289     1091     -198     
Flag Coverage Δ
backend 94.10% <ø> (-1.58%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 3 files with indirect coverage changes

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LoopOver approves — the gate is satisfied and CI is green.

@JSONbored
JSONbored merged commit 46f1658 into main Jul 29, 2026
8 checks passed
@JSONbored
JSONbored deleted the feat/priority-label-author-eligibility branch July 29, 2026 15:03
JSONbored added a commit that referenced this pull request Jul 29, 2026
…b's own refusal (#9864)

#9856: gate success, CI green, mergeable_state "clean",
autonomy auto, approvals satisfied, the bot posted "LoopOver approves" and the
panel said "Suggested Action - Approve/Merge · safe to merge" -- and it never
merged. Recurring for weeks, across many PRs, with no stated reason.

The cause was never a LoopOver bug. GitHub REFUSED the merge:

  merge forbidden (403): Merging stacked PRs via this API is not supported.
  Use the web interface instead.

Stacked PRs cannot be merged through the REST API at all. The executor recorded
the refusal against that head and correctly stopped retrying. But that reason is
invisible to every surface: mergeable_state reads `clean`, no gate models it, the
audit fell through to the residual "merge withheld because no merge action was
planned", and the public comment kept promising a merge that cannot happen. Four
distinct refusals across 7 PRs all rendered identically (403 stacked; three 405
repo-policy variants).

Surface it in both places:
 - agentHoldAuditDetail reports GitHub's message when the block is for THIS head,
   checked last among the specifics (everything above it is a state LoopOver can
   still change on its own; this one only a human can).
 - the unified comment downgrades an otherwise-ready status to HELD and names the
   refusal in the verdict box, with the remedy ("merge it yourself"). Same
   downgrade-only discipline as the guardrail hold: a real gate/CI block still wins.

A block recorded against a DIFFERENT head is stale and deliberately ignored --
that commit was never attempted, so claiming GitHub refused it would be a lie and
would hide whatever is actually holding the new push.

Clock discipline: the decision path uses the pass's recorded instant
(decisionClock.nowMs), never a fresh Date.now() -- the #9492 invariant, whose
guard caught a first attempt that violated it. The publish path matches on head
SHA alone and explains why: it is a reporting surface with no decision clock, and
merge_blocked_until bounds when LoopOver may RETRY, not whether GitHub refused
this commit.
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.

openapi: the route auth-parity meta-test two files are exported FOR does not exist

1 participant