Skip to content

fix(openapi): stop the seam erasing path parameters on any route with a body (#9705) - #9739

Merged
JSONbored merged 2 commits into
mainfrom
fix/route-spec-path-params-9705
Jul 29, 2026
Merged

fix(openapi): stop the seam erasing path parameters on any route with a body (#9705)#9739
JSONbored merged 2 commits into
mainfrom
fix/route-spec-path-params-9705

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Closes #9705.

The defect

registerRouteSpec built one object literal and handed it to registry.registerPath. That literal contained the key request twice — once where path parameters were added, and again inside a conditional spread that fired when a body or query was declared. A property arriving through a later spread replaces an earlier one of the same name, and TypeScript does not flag it, because the duplicate arrives via a spread rather than as a literal duplicate key.

So any route declaring both a templated path segment and a request body or query published no parameters array at all. That is precisely the failure pathParameters()'s own doc comment says it exists to prevent: a templated segment with no matching parameter is a schema-validation warning and leaves a generated client holding a URL it cannot fill.

It was latent only because no production caller passes request yet — every entry in orb-and-control-route-specs.ts and internal-and-public-route-specs.ts is response-only. The first migrated POST /v1/repos/:owner/:repo/... handler, the stated next step of #9531, would have lost owner and repo from the published document silently.

The existing test registered exactly the broken combination (/v1/spec-only/:id plus a body) and asserted only expect(operation?.requestBody).toBeDefined(), so the defect passed the suite.

The fix

Build request once, merging path parameters, body, and query into a single key. The single registry.registerPath call, pathParameters(), and toSpecPath() are untouched — the fix is to stop emitting the key twice, not to restructure the emitter.

Fails-before / passes-after

Verified by stashing only src/openapi/define-route.ts and re-running the suite. Against the pre-fix emitter:

× contributes an operation without registering any route
× keeps every path parameter alongside a request BODY
× keeps every path parameter alongside a QUERY schema
× keeps path parameters, the body, and the query together in ONE operation
  Tests  4 failed | 11 passed (15)

After the fix, 15 passed. The fourth new case — a route with path params and no request at all — passes both ways by design: it is the arm that must be unchanged, and it is there so a regression in the other direction cannot hide.

Also in this PR

The ORB and webhook auth levels in securityFor had no test. They are the two levels that are not a LoopOver credential at all — an ORB-issued instance token, and a webhook verified by a body signature carrying no bearer — so publishing the generic bearer/cookie pair for either would tell a client to send something the route does not accept. Now asserted.

Validation

  • apps/loopover-ui/public/openapi.json regenerated: byte-identical, as the issue predicted while no caller passes a request. ui:openapi:check passes.
  • npx vitest run --changed=origin/main: 1913 passed, 142 files.
  • tsc --noEmit clean.
  • src/openapi/define-route.ts patch is comment-and-deletion only — no new executable lines — and every arm of the merged conditional (path params present/absent × body present/absent × query present/absent) is exercised by the four new/extended cases.

… a body (#9705)

registerRouteSpec built one object literal carrying the key `request` twice: path parameters
in the first, body and query in a conditional spread further down. A property arriving through
a later spread replaces an earlier one of the same name, and TypeScript does not flag it
because the duplicate is a spread rather than a literal duplicate key. So any route declaring
BOTH a templated segment and a request body or query published no `parameters` array at all --
exactly what pathParameters() exists to prevent, since a templated segment with no matching
parameter is a schema-validation warning and leaves a generated client holding a URL it cannot
fill.

Latent only because every production entry today is response-only. The first migrated
POST /v1/repos/:owner/:repo/... would have dropped owner and repo silently, and the existing
test registered precisely that combination while asserting only that requestBody was defined.

The fix is to build `request` once; the emitter, pathParameters(), and toSpecPath() are
untouched. The regenerated openapi.json is byte-identical, as expected while no caller passes
a request. Four cases pin it -- body, query, both, and the unchanged no-request arm -- and each
of the first three fails against the pre-fix emitter. The existing spec-only assertion now
also checks its `id` parameter, and a new case covers the ORB and webhook auth levels, whose
own security schemes had no test.
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 29, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
loopover-ui 8b72a90 Commit Preview URL

Branch Preview URL
Jul 29 2026, 07:31 AM

@loopover-orb

loopover-orb Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Warning

⏸️ LoopOver review result - manual review recommended

Review updated: 2026-07-29 07:48:18 UTC

2 files · 1 AI reviewer · no blockers · CI green · clean

⏸️ Suggested Action - Manual Review

Review summary
This fixes a real bug where registerRouteSpec's object literal contained two `request` keys — one for path parameters, another via a conditional spread for body/query — with the spread silently clobbering the path-parameter key whenever a route declared a body or query, since JS object spread lets a later key win without a TS diagnostic. The fix correctly merges all three (path params, body, query) into a single `request` object built once, and the regression tests directly exercise the previously-broken combinations (body-only, query-only, both) against a route with two path segments, verifying `parameters` survives. The change is narrow, closes the linked issue #9705, and the new tests fail on the pre-fix code as claimed by the description.

Nits — 5 non-blocking
  • The large comment block added at src/openapi/define-route.ts:166-173 duplicates most of the PR description verbatim inside the source file; consider trimming it to the essential invariant (spread-key collision) since the historical narrative belongs in the PR/commit message.
  • test/unit/define-route.test.ts adds an unrelated 'gives the ORB ingress its OWN schemes' test in the `defineRoute` describe block that isn't part of the openapi(seam): registerRouteSpec drops path parameters when a route declares a body or query #9705 fix — likely scope creep from a different change, worth confirming it isn't duplicated elsewhere.
  • Consider extracting the `request` merge back into a small helper (e.g., `buildRequest(options)`) if this pattern is likely to recur elsewhere in the file, though at one call site it's not strictly necessary.
  • The Workers Builds CI failure has no detail provided and this branch is 2 commits behind default; worth rebasing to see if the failure is pre-existing rather than caused by this diff.
  • 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 #9705
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, 370 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 14 PR(s), 370 issue(s).
Improvement ✅ Minor risk: clean · value: minor
Linked issue satisfaction

Partially addressed
The diff correctly builds a single `request` key merging path parameters, body, and query, and adds strong regression tests for path+body, path+query, path+body+query, and path-only cases as required, extending the existing spec-only assertion to include the `id` parameter. However, the issue explicitly requires `apps/loopover-ui/public/openapi.json` to be regenerated via `npm run ui:openapi` and

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: not available
  • Official Gittensor activity: 14 PR(s), 370 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 1 step 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 <question> answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat <question> 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: a73b48d48a180b1b1b03ba119bb158c5a5b03165aab4886e35e104fd750f5227 · pack: oss-anti-slop · ci: failed
  • record: 635a6b28c65f2e729eb63638824febd5a250d06cfeb46edc330456bce742122f (schema v5, head a37eb7e)

🟩 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 89.46%. Comparing base (9e2fab1) to head (8b72a90).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9739      +/-   ##
==========================================
- Coverage   90.28%   89.46%   -0.82%     
==========================================
  Files         907      907              
  Lines      113356   113318      -38     
  Branches    26894    26891       -3     
==========================================
- Hits       102342   101383     -959     
- Misses       9683    10846    +1163     
+ Partials     1331     1089     -242     
Flag Coverage Δ
backend 94.07% <ø> (-1.48%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/openapi/define-route.ts 96.96% <ø> (-0.26%) ⬇️

... and 4 files with indirect coverage changes

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 29, 2026
@JSONbored
JSONbored merged commit 8e02fab into main Jul 29, 2026
8 checks passed
@JSONbored
JSONbored deleted the fix/route-spec-path-params-9705 branch July 29, 2026 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

openapi(seam): registerRouteSpec drops path parameters when a route declares a body or query

1 participant