Skip to content

fix(api): validate discovery routes against published schemas - #10155

Closed
andriypolanski wants to merge 2 commits into
JSONbored:mainfrom
andriypolanski:fix/10040-discovery-schema-validation-2
Closed

fix(api): validate discovery routes against published schemas#10155
andriypolanski wants to merge 2 commits into
JSONbored:mainfrom
andriypolanski:fix/10040-discovery-schema-validation-2

Conversation

@andriypolanski

Copy link
Copy Markdown
Contributor

Summary

  • Derive FindOpportunitiesRequestSchema / IssueRagRetrieveRequestSchema from @loopover/contract's FindOpportunitiesInput / RetrieveIssueContextInput (single source for MCP, REST, OpenAPI).
  • Parse both discovery route bodies with those schemas before the hand-rolled validators; map schema failures onto the existing invalid_request + reason shapes.
  • Add .min(1) on RetrieveIssueContextInput owner/repo/title; stop silent truncation of over-long labels/body; document the cross-field targets/searchQuery rule on the OpenAPI operation.

Test plan

  • npx vitest run test/unit/find-opportunities.test.ts test/unit/mcp-issue-rag.test.ts test/unit/mcp-find-opportunities.test.ts test/unit/contract-registry.test.ts test/unit/openapi.test.ts
  • npx vitest run test/integration/api.test.ts -t "REGRESSION: discovery routes"
  • npm run ui:openapi

Fixes #10040

andriypolanski and others added 2 commits July 31, 2026 09:07
Single-source FindOpportunities and IssueRag request bodies from
@loopover/contract, enforce them on REST before hand-rolled checks, and
stop silent truncation of over-long labels/body.

Co-authored-by: Cursor <cursoragent@cursor.com>
…lidation

Update validateIssueRagInput and route rejection expectations so length
bounds live on RetrieveIssueContextInput and empty/malformed bodies map
to invalid_body, matching the discovery schema gate.

Co-authored-by: Cursor <cursoragent@cursor.com>
@loopover-orb

loopover-orb Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Caution

🛑 LoopOver review result - fixes required

Review updated: 2026-07-31 10:41:23 UTC

14 files · 1 AI reviewer · no blockers · CI failing · unstable

🛑 Suggested Action - Fix Blockers

Review summary
This PR centralizes discovery-route validation onto zod schemas derived from @​loopover/contract (FindOpportunitiesInput / RetrieveIssueContextInput), parses request bodies against those schemas before the hand-rolled cross-field validators, and regenerates openapi.json/spec.ts to match — removing duplicated bound checks (target/owner/repo length, labels, topK, search-query length, languages) from src/mcp/find-opportunities.ts and src/mcp/issue-rag.ts. The change is well-traced: schema failures now collapse to a single 'invalid_body' reason instead of the previous granular reasons (owner_too_long, too_many_targets, etc.), which the extensive test updates in test/integration/api.test.ts and the various unit test files consistently reflect. The RetrieveIssueContextInput.owner/repo/title now require .min(1), closing the previously-published empty-string-is-valid gap noted in issue #10040, and this is wired through routes.ts, the MCP contract, and the regenerated openapi.json in lockstep.

Nits — 5 non-blocking
  • src/mcp/issue-rag.ts's cleanLabels now checks `value.length > PREFLIGHT_LIMITS.labelChars` on the post-trim label, but since RetrieveIssueContextInput already caps raw label length pre-trim on both the REST route (via IssueRagRetrieveRequestSchema) and the MCP tool contract, a trimmed value can never exceed that bound in production — this branch is only exercised by unit tests calling validateIssueRagInput directly; worth a comment noting it's defense-in-depth rather than a reachable production check.
  • The PR description says schema failures are 'mapped onto the existing invalid_request + reason shapes,' but in practice src/api/routes.ts:3600 and :3632 collapse every schema failure to a single generic 'invalid_body' reason rather than preserving the previous per-field reasons — worth calling out explicitly since API consumers relying on specific reason strings (e.g. owner_too_long) will now only see invalid_body.
  • CI shows validate-code/validate-tests/validate failing with no detail provided on a branch 19 commits behind the default branch — likely explained by drift from main rather than a defect visible in this diff; worth rebasing to confirm before merge.
  • Consider dropping the now-unreachable post-trim label-length re-check in cleanLabels (src/mcp/issue-rag.ts) or documenting it explicitly as a deliberate defense-in-depth guard rather than a live validation path.
  • Since the generic 'invalid_body' reason loses granularity, consider surfacing zod's flatten().fieldErrors in a debug/dev-only field so API consumers troubleshooting a 400 aren't left guessing which field failed.

CI checks failing

  • codecov/patch — 92.00% of diff hit (target 99.00%)
  • validate

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 #10040
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: 159 registered-repo PR(s), 104 merged, 22 issue(s).
Contributor context ✅ Confirmed Gittensor contributor andriypolanski; Gittensor profile; 159 PR(s), 22 issue(s).
Improvement ✅ Minor risk: clean · value: minor
Linked issue satisfaction

Addressed
The PR single-sources the request schemas from @​loopover/contract's FindOpportunitiesInput/RetrieveIssueContextInput, wires both REST handlers to zod-parse against them before the hand-rolled validators (which are trimmed to only the cross-field/dedup/trim logic), preserves the existing invalid_request response shapes, stops the silent truncation of over-long labels/body, and adds .min(1) to owner

Review context
  • Author: andriypolanski
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 159 PR(s), 22 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Triage stale or unlinked PRs.
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: 606b296524b7a385931bf5755fd0ff7a2e7e0555dac8d371e34899f2c97bb6bc · pack: oss-anti-slop · ci: failed
  • note: diverted by the randomized close-audit holdout (calibration: randomized ε-holdout on would-close PRs with propensity logging #8831) — the deterministic pipeline would otherwise have closed this PR
  • record: 4bdc5dbd233b2be150ee60e6c31b41b9587b5cf65eba7d3bb9674b9489be7715 (schema v6, head b8c0859)
Visual preview
Route Viewport Before (production) After (this PR's preview) Diff
/ desktop before /
before /
after /
after /
/ mobile before / (mobile)
before / (mobile)
after / (mobile)
after / (mobile)

Click any thumbnail to open the full-size screenshot. Before = production · After = this PR's preview deploy.

Scroll preview
Route Before (production) After (this PR's preview)
/ before / (scroll)
before / (scroll)
after / (scroll)
after / (scroll)

A short scroll-through clip (desktop) — click either thumbnail to open the full animation. Evidence for scroll-linked behavior a single screenshot can't show.

🟩 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.

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 31, 2026
@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.42%. Comparing base (f8fe08d) to head (b8c0859).
⚠️ Report is 24 commits behind head on main.

Files with missing lines Patch % Lines
src/api/routes.ts 87.50% 0 Missing and 1 partial ⚠️
src/mcp/issue-rag.ts 93.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main   #10155       +/-   ##
===========================================
+ Coverage   91.96%   96.42%    +4.46%     
===========================================
  Files         931        6      -925     
  Lines      113976     3133   -110843     
  Branches    27516      873    -26643     
===========================================
- Hits       104818     3021   -101797     
+ Misses       7858       21     -7837     
+ Partials     1300       91     -1209     
Flag Coverage Δ
backend 96.42% <92.00%> (+0.74%) ⬆️
control-plane ?
engine ?
rees ?

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

Files with missing lines Coverage Δ
...ckages/loopover-contract/src/tools/local-branch.ts 100.00% <ø> (ø)
src/mcp/find-opportunities.ts 100.00% <ø> (ø)
src/openapi/schemas.ts 100.00% <100.00%> (ø)
src/openapi/spec.ts 99.27% <ø> (ø)
src/api/routes.ts 95.63% <87.50%> (+0.01%) ⬆️
src/mcp/issue-rag.ts 97.14% <93.33%> (-2.86%) ⬇️

... and 925 files with indirect coverage changes

@loopover-orb loopover-orb Bot added manual-review Gittensor contributor context and removed manual-review Gittensor contributor context labels Jul 31, 2026
@loopover-orb loopover-orb Bot removed the manual-review Gittensor contributor context label Jul 31, 2026
@loopover-orb loopover-orb Bot added manual-review Gittensor contributor context and removed manual-review Gittensor contributor context labels Jul 31, 2026
@loopover-orb loopover-orb Bot added manual-review Gittensor contributor context and removed manual-review Gittensor contributor context labels Jul 31, 2026

@JSONbored JSONbored left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Failing CI, closing.

@JSONbored JSONbored closed this Jul 31, 2026
JSONbored added a commit that referenced this pull request Jul 31, 2026
…together

#10155 is flapping in production on beta.7: the label is
removed and re-added roughly every 90 seconds -- four cycles in eight minutes,
each pair a GitHub write and a subscriber notification. The audit trail shows
both operations in the SAME pass, two seconds apart.

Three post-plan transforms surface the manual-review hold -- the merge
circuit-breaker, the close circuit-breaker, and the close-audit holdout (#8831).
All three carried the identical idempotency check, which looks for an existing
ADD (`labelOp !== "remove"`) and therefore cannot see a planned REMOVE. When the
planner has already scheduled a release -- section 1b does, whenever nothing IT
knows about still wants a hold -- the transform appended an add next to that
remove, and the executor performed both.

Latent until #10116. `noManualReviewHoldWanted` used to include
`!mergeableStateUnstable`, which suppressed the release on exactly these PRs and
accidentally masked the missing case. Removing that term (correctly -- it was the
latch keeping #10098 stuck) let the release fire and exposed the contradiction.
#10116 did not create this; the label was previously a sticky latch, quieter but
strictly more broken.

The planner cannot fix it from its side. Section 1b's doc calls
noManualReviewHoldWanted "every reason that would ADD this label, in one place";
it is not, and cannot be, because these three run AFTER planning -- #8831 landed
long after that comment was written. Extending the planner's list would just be
one more thing to remember on the next transform.

So one shared withManualReviewHoldLabel() drops a planned release of the same
label before adding, and all three call sites use it. The contradiction becomes
unrepresentable where the add happens. The remove is dropped rather than the add
skipped: a transform only gets there by having just diverted a merge or a close,
so its hold is strictly newer than the release decided before that diversion.

Scoped to this label only -- dropping every label remove would silently defeat
the stale-disposition-label cleanup, which is its own mutation test.

gate.closeAuditHoldoutPct is 20 (the maximum) in the global Orb config, so about
one in five would-close PRs is eligible for the path that triggered this.

Closes #10164
JSONbored added a commit that referenced this pull request Jul 31, 2026
…together (#10167)

#10155 is flapping in production on beta.7: the label is
removed and re-added roughly every 90 seconds -- four cycles in eight minutes,
each pair a GitHub write and a subscriber notification. The audit trail shows
both operations in the SAME pass, two seconds apart.

Three post-plan transforms surface the manual-review hold -- the merge
circuit-breaker, the close circuit-breaker, and the close-audit holdout (#8831).
All three carried the identical idempotency check, which looks for an existing
ADD (`labelOp !== "remove"`) and therefore cannot see a planned REMOVE. When the
planner has already scheduled a release -- section 1b does, whenever nothing IT
knows about still wants a hold -- the transform appended an add next to that
remove, and the executor performed both.

Latent until #10116. `noManualReviewHoldWanted` used to include
`!mergeableStateUnstable`, which suppressed the release on exactly these PRs and
accidentally masked the missing case. Removing that term (correctly -- it was the
latch keeping #10098 stuck) let the release fire and exposed the contradiction.
#10116 did not create this; the label was previously a sticky latch, quieter but
strictly more broken.

The planner cannot fix it from its side. Section 1b's doc calls
noManualReviewHoldWanted "every reason that would ADD this label, in one place";
it is not, and cannot be, because these three run AFTER planning -- #8831 landed
long after that comment was written. Extending the planner's list would just be
one more thing to remember on the next transform.

So one shared withManualReviewHoldLabel() drops a planned release of the same
label before adding, and all three call sites use it. The contradiction becomes
unrepresentable where the add happens. The remove is dropped rather than the add
skipped: a transform only gets there by having just diverted a merge or a close,
so its hold is strictly newer than the release decided before that diversion.

Scoped to this label only -- dropping every label remove would silently defeat
the stale-disposition-label cleanup, which is its own mutation test.

gate.closeAuditHoldoutPct is 20 (the maximum) in the global Orb config, so about
one in five would-close PRs is eligible for the path that triggered this.

Closes #10164
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.

api(discovery): validate /v1/opportunities/find and /v1/issue-rag/retrieve against the schemas their spec publishes

2 participants