Skip to content

[WRONG BRANCH] feat(provider-security): ChefVault client + in-memory slots (PSP-008) - #37

Closed
OnlineChef wants to merge 46 commits into
feat/all-provider-account-managementfrom
feat/provider-security-plane
Closed

[WRONG BRANCH] feat(provider-security): ChefVault client + in-memory slots (PSP-008)#37
OnlineChef wants to merge 46 commits into
feat/all-provider-account-managementfrom
feat/provider-security-plane

Conversation

@OnlineChef

@OnlineChef OnlineChef commented Aug 1, 2026

Copy link
Copy Markdown

Summary

  • Add ChefVault provider-security client (CHEF_PROVIDER_SECURITY_URL, default http://127.0.0.1:8323) with workload headers X-Chef-Workload-Id / X-Chef-Host-Id / X-Chef-Actor.
  • Implement in-memory credential slot model (active / next / retiring / revoked), immutable per-request snapshots, renewal jitter, redacted doctor/status output, and explicit error taxonomy including stale_fencing_token.
  • Stub PSP-011 degraded mode: when ChefVault is unavailable, allow bounded use of valid in-memory leases only; deny new resolution until authority recovers.
  • Wire minimal chefvault:// credentialRef integration for provider doctor/status and models auth resolution.

Stacks on #36 (feat/all-provider-account-management).

Test plan

  • bun test tests/provider-security.test.ts — slot transitions, stale fencing, degraded deny-new-resolve, no secret in status serialization
  • bun run typecheck
  • ChefVault authority endpoint smoke when PSP-006 lands on vault-api

Made with Cursor


View with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is enabled.

Greptile Summary

This change adds ChefVault-backed provider credential resolution, in-memory lease rotation, degraded-mode handling, and provider-security status reporting.

Two blocking failures were reproduced:

  • A valid ChefVault response placed in a non-active slot can either fail immediately or pair one lease's secret with another lease's metadata.
  • A ChefVault resolution failure during model discovery is converted into missing credentials, allowing an unauthenticated upstream /models request instead of surfacing the authority failure.

Confidence Score: 4/5

Not safe to merge until ChefVault failures stop falling back to unauthenticated model discovery and lease-slot resolution returns consistent credential material and metadata.

The reproduced failures affect core provider authentication behavior: valid rotated credentials can be unusable or internally inconsistent, and authority failures can be silently bypassed for model discovery.

Files Needing Attention: src/provider-security/resolve.ts, src/provider-security/slots.ts, and src/oauth/index.ts

Security Review

The provider credential boundary can be bypassed during model discovery: when ChefVault rejects a credential reference, the application continues with an unauthenticated request to the configured upstream. Lease rotation also lacks credential and metadata consistency for non-active slot responses, which can cause requests to carry a secret that does not match the associated lease fencing and expiry information. These issues should be fixed before merge.

T-Rex T-Rex Logs

What T-Rex did

  • T-Rex produced a proof for the posted P1 finding related to the ChefVault slot-hint reproduction harness.
  • T-Rex produced a proof for the posted P1 finding related to the ChefVault model-discovery reproduction script and denial path.
  • T-Rex validated contract behavior by running the executable harness and capturing the command output for the ChefVault contract scenario.
  • T-Rex produced a P1 finding proof with no artifacts attached, following additional contract-validation work.
  • T-Rex performed a second general-contract-validation run showing a direct ProviderSecurityError and discovery-path differences in the oauth flow.

View all artifacts

T-Rex Ran code and verified through T-Rex

Comments Outside Diff (2)

  1. General comment

    P1 Resolver returns unusable or internally inconsistent credentials for non-active slot hints

    • Bug
      • Confirmed at src/provider-security/resolve.ts:83-88, with request selection in src/provider-security/slots.ts:114-122. A first resolve returning slotHint: "next" is stored in slots.next, but snapshotForRequest selects only active then retiring, so line 86 throws authority_error despite a successful ChefVault response. If the resolve response is retiring while an active lease exists, line 88 returns lease.secret from retiring but the selected snapshot describes active, so API-key material and lease metadata disagree.
    • Cause
      • applyResolve honors slotHint, whereas snapshotForRequest excludes next; resolveCredentialRef independently uses the lease returned by applyResolve for apiKey and a separately selected request snapshot for metadata.
    • Fix
      • Make resolution return a self-consistent selected credential: define the intended eligibility/promotion behavior for next, then either select/promote it when it is the sole usable resolved lease or reject/defer it explicitly. Return apiKey from the same snapshot selected for the request (or require the snapshot lease ID to equal the applied lease ID) so retiring/active responses cannot mix lease material.

    T-Rex Ran code and verified through T-Rex

  2. General comment

    P1 ChefVault credential-resolution failures are downgraded to unauthenticated model discovery

    • Bug
      • For a key-auth provider with credentialRef: chefvault://team/demo-provider, a deterministic ChefVault 404 carrying code: ref_not_found reaches the resolver as ProviderSecurityError(ref_not_found). resolveModelsAuthToken catches it and returns undefined; fetchProviderModels subsequently sends GET /models to the configured upstream with no Authorization header and accepts its returned live model catalog.
    • Cause
      • src/oauth/index.ts:471-475 catches every ChefVault resolver failure and converts it to undefined. Unlike the OAuth-specific unauthenticated fallback, the surrounding key-auth discovery path does not treat an absent key as a reason to stop; it builds and executes the live discovery request without an auth header.
    • Fix
      • At src/oauth/index.ts:471-475, do not convert ChefVault resolver errors to undefined. Let the typed ProviderSecurityError propagate (or explicitly handle it as a failed/blocked discovery before any upstream request), so model discovery cannot silently bypass a ChefVault failure.

    T-Rex Ran code and verified through T-Rex

Fix All in Cursor Fix All in Codex Fix All in Claude Code Fix All in Conductor

Prompt To Fix All With AI
### Issue 1
src/provider-security/resolve.ts:83-88
**Slot responses produce inconsistent credentials**

A successful ChefVault response with `slotHint: "next"` is stored in `next`, but request snapshot selection only considers active and retiring slots. The first resolution therefore throws `authority_error` even though ChefVault returned a valid credential. A `retiring` response while an active lease exists is also inconsistent: this method returns the retiring lease's secret while attaching the active lease's snapshot metadata. Select or promote an eligible resolved slot before returning it, and derive both the secret and metadata from that same selected snapshot.

### Issue 2
src/oauth/index.ts:470-476
**ChefVault failures become missing auth**

This catch converts typed ChefVault resolution failures into `undefined`. For key-auth providers, model discovery treats that as absent credentials and proceeds to call the upstream `/models` endpoint without an `Authorization` header, accepting any unauthenticated response instead of reporting the authority failure. Allow `ProviderSecurityError` to propagate, or stop discovery explicitly when credential resolution fails.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (1): Last reviewed commit: "feat(provider-security): ChefVault clien..." | Re-trigger Greptile

Greptile also left 2 inline comments on this PR.

Context used:

  • Context used - Focking gretig zijn en niet stoppen tot perfectie.... (source)

ingwannu (Ingwannu) and others added 30 commits July 22, 2026 07:34
Init posthog-js only when VITE_POSTHOG_KEY is set; capture hashchange pageviews without identify/PII.

Co-authored-by: Cursor <cursoragent@cursor.com>
Greptile P1 on PR #2: window.location.href leaked the full URL (OAuth
?code=, invitation tokens, emails in query/hash) to PostHog as
$current_url. Report origin + pathname plus only a known hash route;
drop the query string and any unknown hash contents.
OmniRoute (open-source OpenAI-compatible free-model gateway, 250+ providers /
90+ free) wired into the opencodex provider registry as an OpenAI-compatible
provider (reuses the existing adapter kind):
- src/providers/registry.ts: OmniRoute entry (baseUrl https://api.omniroute.online/v1,
  configurable via OCX_OMNIROUTE_BASE_URL for self-hosted fleet instances; auth
  via OCX_OMNIROUTE_KEY), representative free model ids (kimi/glm/deepseek/qwen…).
- gui: provider icon + icon mapping so it appears in the provider rail.
- tests: provider-registry-parity updated for the new entry.
- docs/providers/omniroute.md: setup (key, optional self-hosted Docker), model
  selection, note that live /v1/models is the source of truth.

Additive — no change to existing providers. Local typecheck blocked by a
missing bun-types devDep in this worktree (env only; reproduces without this
change); CI verifies.

Co-authored-by: OnlineChef <280567955+OnlineChef@users.noreply.github.com>
…e-pool max) (#4)

* feat(codex): add rotation-mode + request-pacing config types

Foundation for multi-account free-pool maximization (opt-in, backward-compat):
- codexRotationMode: 'failover' (default, current sticky behavior) | 'round-robin'
  (new conversations rotate across usable pool accounts so a multi-account free
  pool multiplies throughput; thread affinity preserved; cooldown/reauth skipped).
- codexRequestPacing: jittered per-account inter-request delay [minMs,maxMs],
  off by default, so a multi-account pool never emits a regular ban-prone pattern.

Types only this commit; wiring follows. (Agent run aborted before impl.)

* feat(codex): round-robin pool rotation + jittered request pacing

Wire the opt-in multi-account free-pool maximization (types in prior commit):
- routing.ts: when codexRotationMode === 'round-robin', NEW (non-affined)
  conversations rotate across getEligiblePoolAccounts via a per-process cursor,
  so a multi-account free pool multiplies throughput instead of only failing
  over. Cooldown/reauth/soft-avoid already filtered; thread affinity preserved;
  activeCodexAccountId untouched; single account = no-op.
- pacer.ts: per-account jittered inter-request gap [minMs,maxMs] (defaults
  150-900ms), off by default (codexRequestPacing.enabled). Per-account state so
  concurrent accounts desync instead of a fixed ban-prone cadence.
- responses.ts: await codexPaceBeforeSend before the outbound Codex pool send
  (guarded by usesCodexForwardPoolAuth), no-op when disabled.

Tests (bun): 5 pacer + 5 rotation, all pass. typecheck clean. Backward-compat
(defaults preserve current failover + no-pacing behavior).

---------

Co-authored-by: OnlineChef <280567955+OnlineChef@users.noreply.github.com>
Record upstream, org fork, npm package, and the verified host pin for
OnlineChefGroep so ocx upgrades stay explicit.

Co-authored-by: Cursor <cursoragent@cursor.com>
Without prompt=select_account an already-signed-in browser re-approves
the same JWT sub, so multiauth updates the active row instead of appending.

Co-authored-by: Cursor <cursoragent@cursor.com>
… + status endpoint

- types.ts: OcxClaudeDesktopProfile에 appliedFingerprint/appliedAt 추가
- desktop-3p.ts: writeDesktop3pConfig에 SHA-256 fingerprint 반환 + metadata write 실패 시 .bak rollback
- management-api.ts: /api/claude-desktop/apply에 fingerprint 저장 + /api/claude-desktop/status 엔드포인트 (saved-vs-on-disk 비교)
- claude-messages.ts: Desktop 3P alias 감지 시 logCtx.surface = 'claude-desktop'
- request-log.ts: surface 타입 확장 ('claude' | 'claude-desktop')
- usage/log.ts, usage/summary.ts: surface 필터 확장
- types.ts: desktopAutoApply 옵션 추가
- desktop-health.ts: in-memory 요청/에러 트래커 (NEW)
- claude-messages.ts: Desktop 요청 시 recordDesktopRequest() 호출
- management-api.ts: /api/claude-desktop/status에 health 포함 + autoApplyDesktopBestEffort (provider 변경 시 자동 3P config 갱신)
- management-api.ts: buildClaudeDesktopState에 effortSupported 추가
- ClaudeDesktop.tsx: status bar (applied/stale/not-applied) + health (lastRequest, req/err) + effort badge
- i18n 4개 언어: status/health/effort 키 추가
- styles.css: .claude-status-bar, .claude-effort-badge 스타일
…arch 260722)

- desktop-3p.ts: prefer1m:true 설정 (supports1m 모델에, 공식 스키마 정렬)
- desktop-3p.ts: isClaudeShapedId() 가드 추가 (Ollama '0 usable' 거부 방어)
- Luna 5레인 연구 기반: 공식 inferenceModels 스키마, 커뮤니티 모델 소실 이슈
- Add nl.ts translations + Dutch pages (Instellingen, Modellen, Systeem, Verkeer)
- Refactor App.tsx into provider-workspace shell
- Add quota bars + use-provider-quotas hook
- Extend de/en i18n with new keys
- Add depas.css styling, provider-quota and provider-workspace-shell CSS
- Update FLEET.md
… ja/ru

Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
- App: re-apply saved theme on mount instead of wiping data-theme
- App: route legacy deep links (#codex-auth/#api/#claude/#combos/#subagents)
  through a per-page sub-target so bookmarks open the right tab/section
- App/Modellen/Systeem/Verkeer: drive all visible copy through t() and add
  the backing keys to every locale dict (en/nl/de/ko/zh/ru/ja)
- management-api: return non-OK when a single-provider quota probe yields no
  report so the GUI can show the refresh-failure state

Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
…ional divergence

Two upstream source-contract tests were merged into this fleet-pin branch but
never matched its deliberately divergent implementation, so CI's Test step was
red before this change (both already failed at parent f55fcb5).

- oauth-tos-warning: this host intentionally drops "cursor" from the elevated
  ToS-risk set (documented patch in gui/src/oauth-tos-risk.ts) so the warning
  modal never blocks the multi-account Cursor login flow. Assert cursor is
  unmarked instead of "elevated"; keep github-copilot as the elevated case.
- provider-workspace-rail: App.tsx here localizes the shell to Dutch page ids
  and models routing as { page, target } via readRouteFromHash/canonicalHash,
  not the upstream hashBelongsToPage/"providers/workspace" helper. Assert the
  branch's actual canonicalization + subroute-target threading contract.

Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
…new routing code

The GUI lint step was failing on the deep-link/localization work from 05d8801:

- react-refresh/only-export-components: move readTheme/applyTheme/Theme out of the
  Instellingen component file into a dedicated gui/src/theme.ts util module (matching
  the existing format-bytes.ts / formatUptime.ts convention); import from App + Settings.
- react-hooks/set-state-in-effect: replace the target-sync effects in Modellen, Systeem
  and Verkeer with React's documented render-phase "adjust state when a prop changes"
  pattern, preserving the deep-link tab/section open behavior.
- react-hooks/refs: update pausedRef inside an effect instead of during render in Verkeer.
- local-i18n/no-hardcoded-ui-strings: route the remaining receipt-row/detail literals
  (tok, s, status, upstream:, id) through t() and add vk.rowTokens/rowDuration/detailStatus/
  detailUpstream/detailId to all seven locale dicts.

Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
…a dedup, tab reset, routing tests

- Instellingen: track theme in React state so the active theme re-renders on select
- i18n/shared: keep English browser locales (en-*) instead of defaulting them to Dutch
- use-provider-quotas: dedupe in-flight requests by URL so a forced refresh (?refresh=1) never joins/loses to a non-forced one
- Modellen: reset the active tab to the default when the routed target is absent/invalid
- Extract hash routing into gui/src/route.ts (parseHash/canonicalHash) and rewrite the rail routing test to exercise real parse/canonicalize behavior

Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
chore(fleet): reconcile fleet pin with main
docs: document maintainers and review ownership
…low-providers

feat(providers): add Tencent and SiliconFlow providers
feat(gui): optional PostHog EU analytics (CHE-785)
… repair, cursor fixes)

# Conflicts:
#	src/server/management-api.ts
# Conflicts:
#	gui/src/App.tsx
#	gui/src/i18n/de.ts
#	gui/src/i18n/en.ts
#	gui/src/i18n/ja.ts
#	gui/src/i18n/ko.ts
#	gui/src/i18n/ru.ts
#	gui/src/i18n/zh.ts
OnlineChef and others added 15 commits July 25, 2026 12:48
- Remove FLEET.md (upstream pin document)
- Remove upstream remote references in README/MAINTAINERS/CODEOWNERS
- Add VERSIONING.md documenting our independent semantic versioning
- Add RELEASE_PROCESS.md with release instructions
- Create CHANGELOG.md with merged features and fork history
- Add .github/dependabot.yml for weekly dependency updates
- Enhance CI with security audit, CodeQL analysis, and workflow linting
- Update README with fork banner and CI badge
- Release workflow now allows any main-branch release (stable or alpha/preview)
- Preview releases can use any prerelease suffix (e.g. -alpha.1, -beta.1)
- Removed preview branch requirement (we use main for everything)
- Added ROADMAP.md with short/medium/long term plans
- Replace upstream license/docs links with OnlineChefGroep URLs
- Add CI status badge to all translated READMEs
- Remove upstream documentation link
… URLs

- Source code repo constants (star-prompt, update/job, update/notify)
- PR target enforcement workflow (dev→main, docs links)
- Issue quality workflow (docs links)
- Issue templates (security, contributing docs URLs)
- Test scripts (expected URLs, closed_by user)
- All READMEs (en, ko, zh-CN, ru, ja): replace lidge-jun URLs with OnlineChefGroep
- GUI (Dashboard.tsx, Models.tsx): fix docs links
- docs-site: astro.config, robots.txt, Landing.astro, all translated docs
- CONTRIBUTING.md: replace upstream contributing link
- tests/update-job.test.ts: fix expected release URL
BREAKING CHANGE: This is our first fully independent release, detached from
upstream versioning. The version jumps from 2.7.33 to 1.0.0-alpha.1.

- New versioning scheme: see VERSIONING.md
- All upstream references removed from code, docs, and configs
- Enhanced CI with security auditing and dependabot
…us, persisted rotation

All 5 audit actions implemented:

1. Server-side PostHog telemetry (src/telemetry/posthog-server.ts)
   - Opt-in via OCX_POSTHOG_KEY, EU host default, no PII
   - Batched fire-and-forget, never throws
   - Wired into appendUsageEntry for request_terminal events

2. Token/cost budget alerts (src/usage/budgets.ts, src/usage/pricing.ts)
   - Rolling daily + weekly windows, persist across restarts
   - log/posthog/webhook alert actions
   - GET /api/budgets/summary, PUT /api/budgets endpoints
   - Estimated EUR pricing for major providers

3. ocx status now shows today's token usage + budget status

4. Latency percentiles (src/usage/percentiles.ts)
   - p50/p95/p99 per provider for TTFT + total duration
   - GET /api/latency-stats?window=1h|24h|7d endpoint

5. Persisted round-robin cursor + auto-enable pacer for multi-account pools
   - Cursor survives restarts via rotation-state.json
   - Pacer defaults ON for round-robin pools with >1 account

All 43 new + existing affected tests pass. Typecheck clean.
Align ROADMAP.md and CHANGELOG.md with the actual fork, integration, release, npm ownership, dependency-audit, and upstream-intake state.
Repair missing message/reasoning item IDs in opt-in Responses passthrough streams, add regression coverage, restore workflow linting with actionlint, and remove duplicate advanced CodeQL steps that conflict with repository default setup.
Use Bun's canonical lockfile for dependency auditing, retain JSON audit evidence, block high/critical advisories, and remediate the discovered fast-uri finding with a pinned override.
Collapse Codex-only auth nav into Providers, add configurable
per-provider fallback hops, and keep runtime failover on the
existing combo engine so sofie patches stay reproducible.

Co-authored-by: Cursor <cursoragent@cursor.com>
…stub (PSP-008)

Add provider-security plane client for chefvault:// credential refs with
workload headers, active/next/retiring/revoked slot model, renewal jitter,
redacted doctor/status surfaces, and bounded degraded mode when ChefVault is
unreachable. Wire minimal resolve path for provider doctor/status and models auth.

Co-authored-by: Cursor <cursoragent@cursor.com>
@qodo-code-review

Copy link
Copy Markdown

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

@coderabbitai

coderabbitai Bot commented Aug 1, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (2)
  • ^dev$
  • ^preview$

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: eeba906a-1a68-482b-9c35-c19de311febe

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@OnlineChef
OnlineChef force-pushed the feat/all-provider-account-management branch from a20daf9 to c23de7a Compare August 1, 2026 07:04
@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown

⚠️ Wrong target branch

This pull request currently targets feat/all-provider-account-management, but pull requests must target one of dev or dev2-go.

Its title has been prefixed with [WRONG BRANCH].

OnlineChef Please retarget this PR to dev. Most contributions go to dev first; use dev2-go only for scoped Go native-port work. main receives only release promotions. See our Contributing guide for details. Thanks! 🙏

This pull request is being kept as a draft automatically. Once the target branch is corrected, it will be marked ready for review again.

@github-actions github-actions Bot changed the title feat(provider-security): ChefVault client + in-memory slots (PSP-008) [WRONG BRANCH] feat(provider-security): ChefVault client + in-memory slots (PSP-008) Aug 1, 2026
Add in-memory credential slots, workload-authenticated ChefVault client,
chefvault:// resolve path, redacted doctor/status, and bounded degraded
mode when the secret authority is unavailable.

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

Copy link
Copy Markdown
Author

Superseded by #38 on the same head after fallback stack landed.

@OnlineChef OnlineChef closed this Aug 1, 2026
Comment on lines +83 to +88
const lease = this.slotStore.applyResolve(ref, response, this.now());
const snapshot = this.slotStore.snapshotForRequest(ref, this.now());
if (!snapshot) {
throw new ProviderSecurityError("authority_error", "resolve succeeded but no usable snapshot was stored");
}
return { apiKey: lease.secret, snapshot, source: "chefvault" };

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Slot responses produce inconsistent credentials

A successful ChefVault response with slotHint: "next" is stored in next, but request snapshot selection only considers active and retiring slots. The first resolution therefore throws authority_error even though ChefVault returned a valid credential. A retiring response while an active lease exists is also inconsistent: this method returns the retiring lease's secret while attaching the active lease's snapshot metadata. Select or promote an eligible resolved slot before returning it, and derive both the secret and metadata from that same selected snapshot.

Context Used: Focking gretig zijn en niet stoppen tot perfectie.... (source)

Artifacts

Executable ChefVault slot-hint reproduction harness

  • This deterministic Bun source drives next-only and retiring-with-active resolve paths against the real resolver and slot store, showing the two claimed outcomes.

Observed ChefVault slot-hint reproduction output

  • This captured command output shows the next-only authority_error and the retiring secret paired with active snapshot metadata, confirming the defect.

Focused provider-security regression-suite output

  • This captured Bun test output shows the pre-existing provider-security suite completed with 12 passing tests while not covering the reproduced paths.

View artifacts

T-Rex Ran code and verified through T-Rex

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/provider-security/resolve.ts
Line: 83-88

Comment:
**Slot responses produce inconsistent credentials**

A successful ChefVault response with `slotHint: "next"` is stored in `next`, but request snapshot selection only considers active and retiring slots. The first resolution therefore throws `authority_error` even though ChefVault returned a valid credential. A `retiring` response while an active lease exists is also inconsistent: this method returns the retiring lease's secret while attaching the active lease's snapshot metadata. Select or promote an eligible resolved slot before returning it, and derive both the secret and metadata from that same selected snapshot.

**Context Used:** Focking gretig zijn en niet stoppen tot perfectie.... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Cursor Fix in Codex Fix in Claude Code Fix in Conductor

Comment thread src/oauth/index.ts
Comment on lines +470 to +476
if (credentialRef && isChefVaultRef(credentialRef)) {
try {
const resolved = await globalProviderCredentialResolver.resolveCredentialRef(credentialRef);
return resolved.apiKey;
} catch {
return undefined;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 security ChefVault failures become missing auth

This catch converts typed ChefVault resolution failures into undefined. For key-auth providers, model discovery treats that as absent credentials and proceeds to call the upstream /models endpoint without an Authorization header, accepting any unauthenticated response instead of reporting the authority failure. Allow ProviderSecurityError to propagate, or stop discovery explicitly when credential resolution fails.

Context Used: Focking gretig zijn en niet stoppen tot perfectie.... (source)

Artifacts

Executable ChefVault model-discovery reproduction script

  • Runs local deterministic ChefVault-denial and upstream-model services through the production resolver and catalog discovery path, demonstrating the differing outcomes.

Typed ChefVault denial before model discovery

  • The direct resolver run exits 0 after capturing `ProviderSecurityError(ref_not_found)` from ChefVault and confirms no upstream request was made.

Unauthenticated upstream model discovery after ChefVault denial

  • The discovery integration run exits 0 after ChefVault returns `ref_not_found`, then records an upstream `/models` request with `authorization: null` and an accepted live model.

View artifacts

T-Rex Ran code and verified through T-Rex

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/oauth/index.ts
Line: 470-476

Comment:
**ChefVault failures become missing auth**

This catch converts typed ChefVault resolution failures into `undefined`. For key-auth providers, model discovery treats that as absent credentials and proceeds to call the upstream `/models` endpoint without an `Authorization` header, accepting any unauthenticated response instead of reporting the authority failure. Allow `ProviderSecurityError` to propagate, or stop discovery explicitly when credential resolution fails.

**Context Used:** Focking gretig zijn en niet stoppen tot perfectie.... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Cursor Fix in Codex Fix in Claude Code Fix in Conductor

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.

4 participants