Skip to content

fix(selfhost): keep embedding errors status-only#5111

Open
JSONbored wants to merge 1 commit into
mainfrom
codex/propose-fix-for-embedding-error-data-leak
Open

fix(selfhost): keep embedding errors status-only#5111
JSONbored wants to merge 1 commit into
mainfrom
codex/propose-fix-for-embedding-error-data-leak

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • Prevent provider-controlled embedding response bodies from being embedded in thrown Error values and then promoted into operational logs/Sentry, which can leak private request/input material.

Description

  • Stop reading and propagating non-OK /embeddings response bodies by throwing a status-only error in src/selfhost/ai.ts (now throws ai_embed_http_<status> instead of including res.text() content).
  • Update unit tests in test/unit/selfhost-ai.test.ts to assert that embedding errors surface only the status code and that provider-controlled error bodies are not read.
  • Preserve existing chat-provider behavior and only alter the embeddings error path so functionality and fail-safe semantics remain unchanged except for the diagnostic detail removed.

Testing

  • Ran the targeted unit tests with npx vitest run test/unit/selfhost-ai.test.ts -t "embeddings response|provider-controlled embeddings" --pool=forks, and the focused test suite passed.
  • Ran git diff --check to verify whitespace/conflict hygiene, and it succeeded.
  • Note: a full npm run test:ci / unsharded coverage run was not executed in this environment; only the targeted unit tests listed above were run automatically and passed.

Codex Task

@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.13%. Comparing base (cd1a905) to head (0fbc29d).
⚠️ Report is 23 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5111      +/-   ##
==========================================
- Coverage   94.13%   94.13%   -0.01%     
==========================================
  Files         466      466              
  Lines       39573    39571       -2     
  Branches    14436    14435       -1     
==========================================
- Hits        37254    37252       -2     
  Misses       1664     1664              
  Partials      655      655              
Flag Coverage Δ
shard-1 46.50% <0.00%> (-0.02%) ⬇️
shard-2 33.43% <100.00%> (-0.12%) ⬇️
shard-3 31.60% <0.00%> (+0.25%) ⬆️
shard-4 32.70% <0.00%> (+0.01%) ⬆️
shard-5 33.19% <0.00%> (-0.24%) ⬇️
shard-6 45.14% <0.00%> (+0.20%) ⬆️

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

Files with missing lines Coverage Δ
src/selfhost/ai.ts 98.04% <100.00%> (-0.01%) ⬇️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JSONbored JSONbored self-assigned this Jul 11, 2026
@gittensory-orb gittensory-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 11, 2026
@gittensory-orb

gittensory-orb Bot commented Jul 11, 2026

Copy link
Copy Markdown

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-11 15:22:21 UTC

2 files · 2 AI reviewers · 3 blockers · readiness 93/100 · CI green · unstable

⏸️ Suggested Action - Manual Review

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.
  • AI reviewers agree on a likely critical defect: The PR description does not link or reference an open issue authorizing this change, and per repo policy an external contributor PR must close or clearly link an eligible issue before being treated as merge-ready. — Resolve the flagged defect, or override if the AI reviewers are mistaken, then re-run the gate.

Review summary
This narrows the /embeddings failure path in src/selfhost/ai.ts to throw only `ai_embed_http_<status>` instead of embedding up to 300 chars of the provider's raw response body, closing a path where provider-controlled (and potentially input-echoing) text could reach logs/Sentry. The change mirrors the existing chat-provider status-only error convention, and the accompanying test updates correctly assert both the status-only message and that `res.text()` is never invoked on the error path. The diff is small, self-contained, and technically correct, but it is a deliberate reversal of the diagnostic capability #4996 added specifically to avoid SSH-and-guess triage of production `ai_embed_http_400` errors — that tradeoff isn't acknowledged as a regression in the PR description beyond the stated motivation.

Blockers

  • The PR description does not link or reference an open issue authorizing this change, and per repo policy an external contributor PR must close or clearly link an eligible issue before being treated as merge-ready.
  • PR fix(selfhost): keep embedding errors status-only #5111 description: this does not close or clearly link an eligible open repo issue; add a real repo issue reference such as ```Fixes #NNNN``` or explain the maintainer-authorized issue this is attached to.
Nits — 5 non-blocking
  • This silently reverts the diagnostic detail added in fix(review): RAG embedding calls rejected with HTTP 400 (485 Sentry events, ongoing) #4996 (src/selfhost/ai.ts) without proposing any replacement (e.g. a bounded, server-side-only debug log that never reaches the thrown Error/Sentry) — worth confirming with whoever owns fix(review): RAG embedding calls rejected with HTTP 400 (485 Sentry events, ongoing) #4996 that losing that operator-visible detail is an acceptable tradeoff.
  • test/unit/selfhost-ai.test.ts's 503 case renamed to 'does not read provider-controlled embeddings error bodies' is a good addition, but consider also keeping a case for the empty-body scenario (previously 'falls back to the bare status code when body is empty') since status-only output makes it a degenerate no-op case, not strictly required now.
  • If diagnosability is still valued, consider a `console.log`/structured log emitted server-side (never included in the thrown Error) with a bounded, redacted snippet, keeping the thrown value status-only for Sentry/operational-log promotion.
  • Link the motivating issue/security report in the PR description per repo contribution policy.
  • src/selfhost/ai.ts:302: keep this status-only behavior paired with the existing `does not read provider-controlled embeddings error bodies` test, since that test guards the real leak path better than checking message text alone.

Concerns raised — review before merging

  • The PR description does not link or reference an open issue authorizing this change, and per repo policy an external contributor PR must close or clearly link an eligible issue before being treated as merge-ready.
  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.
Signal Result Evidence
Code review ❌ 3 blockers 2 reviewers, synthesized
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
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 (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 46 registered-repo PR(s), 38 merged, 416 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 46 PR(s), 416 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Improvement ✅ Minor risk: clean · value: minor — Code changes are accompanied by test evidence. LLM value judgment: moderate — The change is a small, correctly-implemented security hardening (prevents provider-controlled/potentially-sensitive response bodies from leaking into thrown errors and Sentry) with matching test coverage, though it trades away a previously-added diagnostic aid without proposing a mitigation.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 46 PR(s), 416 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
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.
[BETA] Chat with Gittensory

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

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

Full command reference: https://gittensory.aethereal.dev/docs/gittensory-commands

🟩 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 Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@gittensory-orb gittensory-orb Bot added the manual-review Gittensor contributor context label Jul 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aardvark codex 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.

1 participant