fix: bill a chat embedding to the user who caused it, not to the shim account (issue #1696) - #1712
Conversation
… account (issue #1696) Open WebUI's Python retrieval path is configured with RAG_OPENAI_API_BASE_URL pointing at edge-api and RAG_OPENAI_API_KEY set to OWUI_SHIM_KEY, so every web search index, every document ingest and every retrieval query reached Hive's own metered gateway as one shared platform principal. The spend was real and it was metered, and all of it settled against that account. Two halves, both required. edge-api: /v1/embeddings joins requiresPerUserAuth, so a shim-key embeddings call that carries no per-user token is refused rather than billed to the shim, and one that does carry a token is rewritten onto it. A new internal/embeddings package then serves the JWT-session request, because inference.Orchestrator resolves only an hk_ key. It holds, charges and settles through the existing sessionbilling lifecycle, prices with inference.CreditsForTokens and normalizes with inference.NormalizeEmbeddings, so this adds no second money path. open-webui: a build-time splice attaches the signed-in user's own access token to the X-Hive-Upstream-Auth carrier inside agenerate_openai_batch_embeddings, the single function every embedding on this deployment leaves through. It raises rather than falling back to the shim key when no user resolves. Customer API keys are untouched: requiresPerUserAuth is only ever consulted under the shim key.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reachedNext included review available in 6 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (16)
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. Comment |
…1696 ledger proof Finalize is a synchronous control-plane call bounded at the settlement timeout and retried once, so settling before the write put up to two of those in front of a customer who had received nothing. internal/rag states the same reason at the identical point. Open WebUI embeds a batch at a time and issues several of these per search turn, so the latency is multiplied rather than paid once. TestTheResponseIsOnTheWireBeforeTheChargeIsSettled holds the ordering in place through billingtest.OnFinalize, which is the only point that can observe the two relative to each other. Also refreshes the OWUI_SHIM_KEY comment in docker-compose.yml, which still carried the pre-fix story that embeddings authenticate and bill as that key (CodeRabbit finding), and commits the two proof captures.
Proof: the ledger, before and after, read out of PostgresNot a log line and not a boolean. A throwaway Postgres with all 132 migrations applied by The searching user's JWT deliberately carries no
Leg 1 is the defect, reproduced on a real ledger: the platform account paid for the customer's search. Leg 2 is the fail-closed rule: a call that cannot say who it belongs to is refused, and neither balance moves. Leg 3 is the fix: the same 15 credits, on the customer. The Every hold reached a terminal state exactly once across all three legs: each The chat container half, run inside the pinned imageThe patch was applied to The shim key stays on Authorization, which is what gates the carrier at edge-api, and the searching user's own token rides on the carrier. A user with no resolvable session is refused before the request is made, so nothing goes out under the shared key. Both captures are committed under No UI surface changes in this diff, so the screenshot rule does not apply; the ledger is the evidence for the behaviour claim. |
Adversarial review, pipeline modeStreams that ran, and the ones that did not, stated rather than left as an absence. CodeRabbit CLI: RAN, one finding, addressed
Valid, and exactly the failure this repository has paid for before: a stale comment that a later reader believes. Fixed in e566c44. The block now says embeddings still PRESENT the key, which is what gates the carrier, and are BILLED to the signed-in user, so the key resolving is necessary for them rather than sufficient. Security review: RAN, performed directly, no agent dispatch availableStated plainly: this reviewer has no Skill or Agent tool in its toolset, so the The carrier is a verified credential, not an asserted identity. The token on A client cannot smuggle the carrier. The change to No confused deputy downstream. The user's token never leaves edge-api: No credential reaches a log. The Python module logs The token cache is the one piece of new shared state, and it is bounded. Keyed by user id, per process, 30 second TTL against a resolver that already refreshes five minutes before expiry, capped at 512 entries with a hard reset past that. A failed resolution is deliberately not cached, so one transient database error is not thirty seconds of refused embeddings for that user. Input bounds. The request body is read through a Residual, named rather than left to be found. Database review: RAN, performed directly, same caveatNo schema change. Nothing in this diff touches Per-request cost. Two additional indexed single-row reads on the path that previously took none: Volume, checked rather than assumed. The issue describes roughly 200 embedding calls per search, which would have made this change roughly 400 extra indexed reads per search on a pool that already couples CI, agents and live chat. It is not 200 any more: Ledger writes are unchanged in shape. One Reporting reachability. No RLS surface change. edge-api connects as the app role and reads two tables it already reads on the chat path. Streams NOT run, and why
Treat the two direct passes as one reviewer's work, not as the independent dispatch the contract asks for. An independent pass on the pushed diff is still worth having before merge. |
Independent security and money path reviewFirst independent eye on this diff. The implementer had no Agent tool, so the mandatory security reviewer and database reviewer streams were marked SKIPPED and performed by the author on its own work; the CodeRabbit bot and the Codex reviewer both hit usage limits on this PR and posted rate limit notices rather than reviews, and the one CodeRabbit finding on record came from the author running the CLI locally. This pass exists to close that gap. Reviewed as a security change first and a billing change second, per the brief. Verdict: request changes. One HIGH security finding and one MEDIUM regression, both with concrete fixes posted inline. Everything else checked out, and the money path work is genuinely good. The forwarded user tokenIt cannot leak through edge-api. It can leak on the chat container side, and that is the HIGH finding. Is a legitimate caller broken by the new refusalYes, two, and neither is named in the PR. The paths the PR does claim are fine. Web search indexing, document ingest and retrieval queries all carry a user. A user with no resolvable OAuth session cannot chat today either, since Fail closed and the hold lifecycleVerified rather than taken on trust. Six refusals all land before dispatch. The reorderCannot drop a charge, for the context reasons above. The remaining platform pathsThe list of shim billed paths is complete. Only four Open WebUI upstreams point at edge-api ( Outside the shim family, one more platform absorbing path that nothing tracks as far as I can see: Patch mechanismDeterministic and loud. The patch asserts the literal occurs exactly once and the marker zero times, the Dockerfile re-checks the marker count is one and AST parses both files, so a digest bump that moves the literal fails the image build. The caveat is where it first runs: Smaller notesThe rewrite of the The proof comment is strong work and I read it rather than skimmed it. Two edge-api binaries against one database, real migrations, real JWKS validation, balances read from |
sakibsadmanshajib
added
internal
…ation, and thread the two producers that had no user (PR #1712 review) HIGH, from the independent security review. `attach` put the signed-in user's Supabase access token on the carrier without looking at where the request was going, and that destination is `app.state.config.RAG_OPENAI_API_BASE_URL`, which `POST /api/v1/retrieval/embedding/update` lets any instance admin rewrite at runtime. On this shared chat instance every tenant OWNER is an instance admin, so one tenant owner could repoint the embedding endpoint and collect a live session bearer from every other user who ran a search. Before the carrier existed the same knob leaked one shared platform key; attaching a per-user credential to it without a destination check would have been a strict escalation, and closing it belongs in the change that introduces the credential. The destination is now compared against the ENVIRONMENT, never against the persistent config, which is the same property `hive_agent_proxy.py` relies on when it reads its own destination from `os.environ`. The check runs before a credential is resolved, so a hostile destination cannot even cause a token to be minted or a cache entry to be filled. Compared as an origin rather than as a whole URL, because the path differs legitimately and the host and port are what decide who receives the credential. MEDIUM, from the same review. Two producers call the embedding function with no user at all, so the new refusal broke them: `embed_knowledge_base_metadata`, whose six callers all reach it inside a swallowed `except`, so knowledge base metadata embedding would have stopped working with no visible error and the admin reindex would have reported zero of N, and the `query_knowledge_bases` builtin. Every call site already has the identity in scope, so both are threaded rather than left broken. The builtin carries `__user__` as a plain dict, which the module resolves through Open WebUI's own model layer rather than wrapping in a stand-in that assumes what the resolver reads. Also taken from the review, both optional there: the response is flushed after the write so the settle-after-respond ordering holds by construction rather than by payload size, and the guard test is renamed to say written rather than on the wire, since a ResponseRecorder has no wire. The `requiresPerUserAuth` guard in `test_owui_task_upstream_auth.py` now also rejects an early `return false`, which would otherwise have left all four literals present and the predicate exempt.
Review round two: both blockers addressedPushed as e64509c. Thank you for the pass; the HIGH is a real escalation I introduced and did not see, and the MEDIUM is the exact trade the brief warned against. HIGH, destination check. MEDIUM, the two producers with no user. Both threaded rather than named. Optional items, both taken. PR body updated with the three things you asked be stated rather than discovered: the loss of the authz rate limiter on this path and that the credit hold is what replaces it, the dependence of that on Two issues filed for the notes you offered to raise: #1723 for Re-proofThe chat-container capture was re-run inside the pinned image against the new patch, with the gateway on one port and the host a hostile admin repointed at on another, so the two origins genuinely differ: Worth recording that the first run of leg 2 printed The ledger proof stands unchanged. The only Go behaviour change in this round is the Green: |
…s merged PR #1712 landed while this branch was open and moved Open WebUI's document RAG embeddings onto the signed-in user's own token, so a dead shim key no longer breaks them. What it still breaks is text-to-speech and speech-to-text, both of which are OWUI_SHIM_KEY, plus the bodyless model listing. The alert summary and description, the gauge's help text and the .env.example list all said RAG and are corrected here, so the page an operator receives names what is actually down. The promtool case was re-run against the new annotation text.
The PR was unmergeable, which is why GitHub built no refs/pull/1730/merge and created no pull_request CI run at all for the last three commits: the required checks were not failing, they were never triggered. One conflict, in scripts/test_owui_task_upstream_auth.py. PR #1712 rewrote the requiresPerUserAuth guard from a frozen copy of the whole function body into a presence check over the paths, for exactly the reason this branch hit: a frozen body cannot tell a removal (the relaxation the check exists to catch) from an addition that narrows what the shim key may do. Main's shape is the better one and is taken whole, with this branch's /v1/tools/ arm added to the list it checks. Verified after the merge: the vendored middleware still matches the pinned image digest, every patch that writes middleware.py applies in Dockerfile order and the result parses, and make test-scripts is green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WbVmp2Uh7FCgnqKB2TuBb5
…ge when it dies (issue #560) (#1734) Closes #560 ## The live state first, because the issue's headline claim needed re-measuring **The demo box's shim key is alive right now. This is not an active outage.** ``` hive-demo-owui-shim | hive-demo-owui-shim-key | active | 2026-07-26 13:07:28+00 | NOT REVOKED ``` Read out of `public.api_keys` on the box, keyed on the sha256 of the value in the box's own `.env`, and the running gateway agrees: `owui: OWUI_SHIM_KEY resolves to an active Hive API key on a tenant-provisioned account`. The correct invocation, since the issue asked for it to be recorded and the self-hosted data plane publishes no host port: ```bash cd ~/hive raw=$(grep -E '^SUPABASE_DB_URL=' .env | head -1 | cut -d= -f2-) eval "$(python3 scripts/derive-pooler-dsn.py --dsn "$raw" --emit-libpq-env | sed 's/^/export /')" scripts/stack-psql.sh -tA -F'|' -c "select ... from public.api_keys ..." </dev/null ``` `scripts/derive-pooler-dsn.py --emit-libpq-env` for the libpq variables, `scripts/stack-psql.sh` from the repository root on the box so psql joins the stack's compose network, and stdin redirected because that wrapper consumes it. The key lives on `hive-demo-owui-shim`, its own billing account, not on `owui-e2e-shim`, the account the nightly rotates. So the specific chain the issue describes is currently broken in the box's favour, and PR #558 is why: it added `--account-slug` and deferred revocation until the consumers are updated. What #558 did not do is make any of that enforceable. The account boundary was a sentence in a docstring, and the loud signal it added was a log line. ## What is actually still wrong **The scoping is a convention, not a boundary.** Nothing stopped a run from being pointed at CI's rotated account while a deployment carried a key on it, and nothing stopped a run from revoking keys on a deployment account without updating that deployment. Either one reopens the issue, and the second reintroduces the exact failure #558 fixed. **The signal is a log line.** `watchOWUIShimKey` probes at boot and every five minutes and logs the verdict with its cause and remedy. That is the right thing to read once you are already looking, and it is not a signal: nothing tails a container log on a schedule, so a revocation at 03:00 is still discovered by a customer. Issue #1728, filed today, is about this pattern in general. ## The fix, in three layers **1. `assert_account_scope` in `scripts/seed-owui-e2e-user.py`,** called as the first statement of `main()`, before a credential is read and before the first request. It refuses both configurations in which this script could revoke a credential something else is still carrying, so a refused run costs nothing and in particular cannot mint a key it then declines to finish wiring up. The invariant, stated once: a key that a long-lived consumer carries may only ever be revoked by the run that just replaced it in that consumer. | Configuration | Verdict | |---|---| | Reserved CI account (`owui-e2e-shim`) with `--env-file` or `OWUI_ADMIN_*` set | refused, exit 2 | | Any other account with no consumer configured | refused, exit 2 | | Reserved CI account, no consumer: the nightly's shape | allowed | | Deployment account with a consumer this run updates: the documented deployment shape | allowed | The first row is the one that closes the hole for good. It makes "CI's rotation account and a deployment's account are the same account" unreachable through the supported path, rather than a mistake the docstring asks you not to make. **2. The stale-key cleanup is filtered to this script's own nickname.** One line on the `DELETE /api_keys` params. It matters because of what the proof turned up: the box's live key is nicknamed `hive-demo-owui-shim-key`, not the `owui-e2e-shim-key` this script writes, so it was minted by hand. Layer 1 governs every path through this script; layer 2 covers the key nobody minted through it. A foreign key on the account is now left active for an operator to revoke deliberately, which is the safer of the two wrong answers available: revoking a credential whose holder was never told is the whole of this issue. **3. `hive_owui_shim_key_usable`,** exported by the probe that already exists, and read by the new `OWUIShimKeyUnusable` rule in `deploy/prometheus/alerts.yml`. Two properties, both deliberate. The gauge is **not registered at all** when no shim key is configured, because a gauge sitting at its zero value on a deployment with no chat front-end reads as "the key is dead" to every alert; with no series, `absent()` can tell "not measured here" from "measured and failing". And it is **not written at all** on a transient probe failure, so it holds the last real verdict: an unreachable control plane is not a verdict on the key, and paging someone to rotate a working credential over a cold container is the false alarm that gets an alert muted. It nearly happened live on 2026-08-14. ## How a human actually notices Not by reading a log, and not by opening a dashboard nobody opens. The chain was checked on the box rather than assumed: * `edge-api:9102` is an active Prometheus target and healthy, with no scrape error (`deploy/prometheus/prometheus.yml`, job `edge-api`). * `hive-prometheus-1`, `hive-alertmanager-1` and `hive-grafana-1` have all been up for over a week, and `--profile monitoring` is in the deploy workflow's compose invocation, so this is not a profile someone has to remember to start. * The `hive-critical` group is loaded (`/api/v1/rules` reports 8 rules in it today, 9 after this). * `deploy/docker/docker-compose.yml` routes it to the `hive-ops` receiver, whose `email_configs` sends to `ENTERPRISE_SMTP_ADMIN_EMAIL` over the relay this stack authenticates against. * The deploy workflow already asserts that every `- alert:` name in `deploy/prometheus/alerts.yml` appears in Prometheus's `/api/v1/rules`, so a rule that fails to load fails the deploy instead of going quiet. **An email to the ops mailbox, roughly ten minutes after the key stops resolving:** one five-minute probe interval plus the rule's six-minute `for`. The `for: 6m` is a deliberate trade. Shorter would fire on a single missed probe; longer would push past the "visible within minutes" the issue asks for. ## Interaction with PR #1712, which merged while this was in review It landed as `234837cef` mid-flight, which is what made this branch conflict: it edits the same comment block in `apps/edge-api/cmd/server/main.go`. Rebased onto it and resolved in its favour. The interaction runs one way only, and every claim here survives it. **#1712 narrows what the shim key is for.** Open WebUI's embeddings now carry the signed-in user's token on `X-Hive-Upstream-Auth` and `/v1/embeddings` joins `requiresPerUserAuth`, so document RAG no longer depends on this key resolving. What still does, and is named in #1712's own description as still settling against the shim account, is text-to-speech and speech-to-text (`AUDIO_TTS_OPENAI_API_KEY` and `AUDIO_STT_OPENAI_API_KEY` are both `OWUI_SHIM_KEY`), plus the bodyless `GET /v1/models`. So the silent-death surface shrinks from three features to two, and the probe, the gauge and the alert stay both correct and necessary. What the merge did make wrong was the wording: the alert, the gauge help text and `.env.example`'s list of what this key authenticates all still said document RAG. All three are corrected in the third commit, so the page an operator receives names what is actually down. `promtool test rules` was re-run against the new annotation text and still passes. The seeder half is orthogonal to #1712 entirely. ## Tests Written first, and each one confirmed red against the pre-change code rather than assumed. `scripts/test_seed_owui_e2e_user.py`: * both refusals, across all three ways a consumer can be configured (`--env-file`, `OWUI_ADMIN_TOKEN`, `OWUI_ADMIN_EMAIL` plus `OWUI_ADMIN_PASSWORD`) * both supported shapes still pass * `main()` driven with a refused configuration and `urllib.request.urlopen` replaced by a function that fails the test if called, so "the guard runs before anything is minted or revoked" is asserted rather than described. Deleting the `assert_account_scope` call turns it red with `main() reached the network on a refused configuration`. * the revocation's params parsed out of the source by AST and asserted to filter on nickname and account. Deleting the nickname filter turns it red. * a static guard that the nightly workflow still passes the reserved CI account slug, because the seeder-side guard cannot catch a nightly repointed at a deployment account (a GitHub runner configures no consumer, so that run would be allowed). `apps/edge-api/cmd/server/main_test.go`, all reading the gauge through a real `Gather` rather than the collector, since a value that never reaches `/metrics` is the same invisibility wearing a different hat: * zero on a key that does not resolve * held at its last value across repeated transient failures, then zero when a real verdict arrives, then back to one on recovery * no series exported at all without a shim key * the alert rule reads the series, which is the assertion that keeps this from becoming another unread metric ## Proof `docs/proof/shim-key-revocation-560-2026-09-02/capture.md`, and the two halves the issue asks for: **The seeder runs without revoking the key.** Executed on the box against the live database. Before: `active`, `NOT REVOKED`. The invocation that could previously have revoked it (`--account-slug hive-demo-owui-shim` with nothing to update) exits 2 with a refusal naming the issue. The nightly's own shape is still allowed, passing the guard and then dying on a deliberately unreachable database having touched nothing. After: byte-identical row, still `active`, still `NOT REVOKED`. **The loud signal fires.** The rule was evaluated by Prometheus itself, not asserted by inspection: `promtool test rules` over a series that is usable for two minutes and then zero asserts nothing firing at 5 minutes and exactly one `OWUIShimKeyUnusable` at 9 minutes with `severity=critical` and the full annotation text. Mutating the 9-minute expectation to "no alerts" turns it red, and that was run. Green: `go vet ./apps/edge-api/...`, `go test ./apps/edge-api/... -count=1 -short`, `make test-scripts`, `npm run lint:proof-tokens`, `promtool check rules`. No UI surface is touched, so the visual-proof rule does not apply to this diff; the live database readings and the promtool evaluation are the evidence for the behaviour claims. ## What this does not do It does not restore or rotate any live credential, and it changed none. If the box's key is ever found revoked, the remedy is in the alert's own description and in `.env.example`: `python3 scripts/seed-owui-e2e-user.py --account-slug hive-demo-owui-shim --tenant-slug <its tenant> --env-file .env` with `OWUI_BASE_URL` and `OWUI_ADMIN_TOKEN` set, then recreate `open-webui`. Both consumers are updated before the old key is revoked, and the guard added here now refuses that command if either is missing. It also does not revoke the leftover `hive-717-diagnostic-2026-08-04-delete-me` key found on the box's account during the proof. It is already revoked, and touching live credentials is out of scope for this PR. ## Review round two (security review 5093605279, commit 62ebb40) All five threads applied. What changed in the description above: **Layer 2 is no longer a constant.** `key_nickname` derives the nickname from the account slug (`SHIM_KEY_NICKNAME` for the reserved CI account, `<slug>-key` otherwise), used at both the mint and the cleanup. The old constant spared the box's `hive-demo-owui-shim-key` only by coincidence, and the remedy documented here would have destroyed that coincidence on first use, since the mint writes the nickname unconditionally. Derived, it reproduces the box's existing name exactly, CI's cleanup and a deployment's can never name the same key, and the remedy run revokes the key it just replaced. A key minted under any other name is still left alone, and `.env.example` and the alert now tell the operator to revoke that one deliberately instead of leaving it active forever with nobody tracking it. **`--env-file` now has to prove itself.** It proved a rewrite, not that the file belonged to the deployment carrying the key about to be revoked, so `/tmp/scratch.env` or another deployment's `.env` reached the revocation. The value the file held before the rewrite must now hash to a key on this account, which is the machine check for the invariant this PR states in prose. Plus: an empty `--account-slug` is refused rather than minting on an empty-slug account, and the reserved-account comparison is case insensitive. **The three documents agree, on the right reasoning.** The alert said document RAG was unaffected because resolution is "necessary but not sufficient" for embeddings. It is neither: `hasShimAuthorization` matches the header by string comparison and `forwardUnwrapped` swaps in the per-user token before authz, so revocation is invisible on `/v1/embeddings`. The empty model picker, the loudest symptom, is now named in the alert, the log line and `.env.example` alike. **An absent verdict and an absent scrape can no longer read as healthy.** `hive_owui_shim_key_last_verdict_seconds` carries the time of the last real verdict (never written on a transient failure, 0 until the first one) and `OWUIShimKeyVerdictStale` pages at thirty minutes, which closes the gap where the usable gauge sits at its initial 1 with nothing ever measured. `EdgeAPITargetDown` (`up{job="edge-api"} == 0`) joins `AlertmanagerDown` in `alerts/monitoring.yml`, since a target that stops being scraped took every `hive-critical` rule quiet with it. Verified: `promtool check rules` (10 rules in `alerts.yml`, 4 in `monitoring.yml`) and a four-case `promtool test rules` run covering both new rules and the discriminating negatives; `go vet` and `go test ./apps/edge-api/... -count=1 -short` green; `make test-scripts` green with each new seeder guard confirmed red against a mutation of the code; `lint:proof-tokens` clean. No live credential was read, changed or revoked in this round, and the live rows recorded in the capture are unchanged. Section 7 of `docs/proof/shim-key-revocation-560-2026-09-02/capture.md` carries the detail. ## Buglog entry ```json {"id":"bug-2026-09-02-owui-shim-key-revocable-by-seeder","date":"2026-09-02","title":"a scheduled seeder run could revoke the shim key a long-lived deployment carries, taking document RAG and voice down with no signal","error_message":"none on the operator side, which is the defect: Open WebUI's document RAG embeddings, text-to-speech and speech-to-text answer a generic invalid-key error that names no cause, while sign-in, the model picker and chat completions stay healthy because they carry the signed-in user's own token.","root_cause":"Two gaps left open after PR #558. First, the account boundary that keeps the nightly OWUI rotation off a deployment's key was documented in scripts/seed-owui-e2e-user.py's docstring and enforced nowhere: nothing refused a run pointed at CI's rotated account while a deployment carried a key on it, nothing refused a run that revoked keys on a deployment account while updating no consumer, and the stale-key cleanup deleted any key on the account regardless of who minted it or who carries it. Second, the health probe added for the same issue wrote its verdict only to the edge-api container log, and nothing reads a container log on a schedule, so a mid-life revocation stayed invisible until a customer hit it.","fix":"Add assert_account_scope, called first in main() before any request, refusing the reserved CI account with a long-lived consumer configured and any other account with none, exit 2. Filter the stale-key DELETE to the script's own key nickname so a key minted by another route is never revoked. Export hive_owui_shim_key_usable from watchOWUIShimKey (registered only when a shim key is configured, and not written on a transient probe failure so it holds its last real verdict) and add the OWUIShimKeyUnusable rule to deploy/prometheus/alerts.yml, which routes through the already-delivering Alertmanager hive-ops receiver to the ops mailbox in about ten minutes.","tags":["credential-lifecycle","open-webui","shim-key","observability","alerting","silent-failure","edge-api","seeder","issue-560"]} ```
#1727) Closes #1726 Implements the tracking discipline the owner asked for on 2026-09-02: pull requests, milestones, the Kanban board and the labels are not being maintained, so this writes the rule down and then wires it so it is enforced rather than aspirational. ## The rule `.claude/rules/tracking-discipline.md`, in the voice of its neighbours in that directory, terse and imperative with each reason given once: - **An issue exists before a fix does.** Every change starts from a GitHub issue, including a one line fix noticed while doing something else. A defect discovered mid task gets filed, not folded silently into an unrelated pull request. The body links its issue with `Closes #N`, or `Refs #N` when it delivers only part of it. Never `Closes` an issue whose acceptance criteria are not all met, because an issue closed early is how work is lost. - **Exactly one priority label**, using the four that already exist in the repository: `priority:critical` for a demo blocker or live outage, `priority:high` for needed before the demo, `priority:medium` for real work to schedule, `priority:low` for correct but not urgent. The retired `priority:P0` through `priority:P3` set does not satisfy it, and is rejected by name so it gets corrected rather than quietly counted. - **At least one area label** from `demo-surface`, `money-path`, `internal`. Their definitions live on the labels themselves and are deliberately not copied into the rule. - **A scheduled issue carries a milestone**, from the four open ones. No milestone means unscheduled, which is legitimate; a critical or high with no milestone is a tracking failure, because it claims urgency and names no release that carries it. - **Pull requests wear the labels of the issue they close**, so the board reads the same from either side. - **Priority is judged against the demo spine**: chat, embeddings, voice to text, knowledge work, Cowork, the coding agent. Breaking one of those on stage is critical regardless of diff size; an internal correctness issue is not critical however elegant the fix would be. - **The orchestrator re-triages every session**: unlabelled open issues, priorities that no longer match reality, and pull requests older than two weeks. A backlog that is not navigable by priority is the same as no backlog. ## The enforcement `scripts/check-pr-tracking.py` reads the pull request body and every issue it links, and fails when: 1. the body links no issue, using the documented verbs. A bare `#N` in prose is deliberately not a link, since bodies in this repository cite issue numbers in passing constantly and counting those would pass everything; 2. a linked issue carries no valid priority label, or carries two, or carries only a retired one; 3. a linked issue carries no area label; 4. a linked `priority:critical` or `priority:high` issue has no milestone; 5. the pull request does not carry the priority and area labels of an issue it closes; 6. a link points at a pull request rather than an issue, or the target cannot be read. An unreadable target fails rather than passing quietly. `.github/workflows/pr-tracking-gate.yml` runs it on every pull request. It is cheap by construction: checkout plus two python invocations, no toolchain, no network beyond the GitHub API. It listens for `edited`, `labeled` and `unlabeled` on top of the four types the merge gate requires, because every way to fix a failure of this gate is one of those three events, and a gate whose green state cannot be reached without an unrelated commit gets worked around rather than satisfied. It is deliberately not a step in `ci.yml` and not affected by that workflow's docs-only allow-list. That list decides whether the heavy suite runs; this gate has to run on a documentation-only pull request too, since a documentation change needs an issue exactly as much as a code change does. Folding it into `repo-policy-lints` would also mean giving that job `issues: read`, which the rest of it has no business holding. ## Can it be bypassed Two answers, both honest. **By branch protection, no, once the config is applied.** `PR is attached to a triaged issue` is added to `required_status_checks.contexts` in `.github/branch-protection-main.json`, and `.github/ci/lint-workflow-check-names.mjs` already verifies on every pull request that the context has exactly one producer, in an `if: always()` job, in a workflow with no trigger path filter that reacts to `ready_for_review`. It reports seven required contexts green on this branch. **That file is only the checked in copy.** Applying it to live branch protection still needs the documented call, which the orchestrator runs after merge: ```bash gh api -X PUT repos/sakibsadmanshajib/hive/branches/main/protection \ -H "Accept: application/vnd.github+json" \ --input .github/branch-protection-main.json ``` Until that runs, the gate is red on a non-compliant pull request but does not block the merge API. One operational note for when it does run: an already-open pull request will not publish the new context until some event fires on it, so nudge the open ones rather than assuming they are stuck. **By a determined author, yes, in one specific way.** The gate confirms that an issue is referenced and that the issue is triaged. It cannot confirm the link is honest: `Refs #N` pointing at any well labelled issue passes. That gap is unfixable by a linter, which is why the rule is addressed to the agent writing the pull request and the gate only catches the accident. This is stated in the rule file itself rather than left for someone to discover. The two carve-outs are narrow and printed in the run log rather than applied silently: Dependabot, which cannot file an issue and whose updates would otherwise stall until a human wrote one, and a pull request whose entire diff is `.wolf/buglog.jsonl`, which is the buglog-only pull request `.claude/rules/openwolf.md` mandates for a fix that already had its own issue. One extra file and the buglog carve-out is gone, so it cannot be used to attach an unrelated diff to an exempt path. ## Verification - `python3 scripts/test_check_pr_tracking.py` passes. Every assertion that matters is a negative one: an untracked body, an unlabelled issue, a retired `priority:P1`, two priorities at once, a missing area label, a critical with no milestone, a pull request missing its closed issue's labels, a link pointing at a pull request, an unreadable issue. It is registered in `make test-scripts`, so it runs inside the required `Repo policy lints (tenant + audit)` job and the comparator cannot silently stop being able to fail. - `node .github/ci/lint-workflow-check-names.mjs` reports `Merge-gate integrity OK: 36 check names across 15 pull-request workflows, 10 required contexts each published by exactly one always()-running job in a workflow with no trigger path filter that reacts to every pull_request type the merge gate needs.` - Run against three live pull requests, not fixtures. #1714 exits 0 as an exempt Dependabot update. #1712 and #1709 each exit 1 with the real finding that they close a labelled issue whose labels they do not carry, which is precisely the drift the owner named. ## Review follow ups Four findings from the CodeRabbit pass, all taken: - A full issue URL was matched for its number alone, so `Closes https://github.com/someone-else/repo/issues/7` passed whenever this repository had a triaged issue 7. `links()` now takes the repository it is validating and skips a URL pointing elsewhere; a bare `#N` is untouched, since a bare number is always local. A body linking only foreign issues now fails with that reason instead of the vaguer "links no issue". - The Dependabot carve out matched any author ending `[bot]` or starting `app/`, which handed the bypass to every other app installed on the repository. It is now an explicit list of the Dependabot identities GitHub reports. - `oauth-scope-gate.yml` said branch protection lists seven required contexts. It lists ten. - `CLAUDE.md` said an issue carries one area label. The rule and the validator both allow more than one, so it now says at least one. Regression cases for the first two: a foreign URL rejected beside the same URL shape pointed here and accepted, and six non-Dependabot bot and app authors that get no exemption. `python3 scripts/test_check_pr_tracking.py` passes. ## Billing gate evidence CodeRabbit asked for test evidence under the rule that changes touching billing need it. This diff contains no billing, credits, payments, auth or tenancy code: `git diff origin/main...HEAD --name-only` is eleven files, all of them workflows, documentation, `scripts/check-pr-tracking.py` and its test, and `tools/verify-spec-wiring.mjs`. What it does touch is *when* the `Refuse to bill a paid completion alias` step in the `live-integration` job executes, so that is what the evidence below is about. The condition did not change. It moved, verbatim, from the job's own `if:` to a `gate` step whose output every step then reads: ``` needs.changes.outputs.run == 'true' && (github.event_name == 'push' || github.event_name == 'schedule' || (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository && contains(github.event.pull_request.labels.*.name, 'run-live-integration'))) ``` Disabled path, executed. Run 33675965786 on commit `f95b60b`, this pull request, which carries no `run-live-integration` label: `Live integration (SDK tests + smoke)` concluded success in 6 seconds with the guard step and every other real step skipped. Before this change the same pull request produced no check run for that context at all, which is the reason for the conversion. Enabled path, not executed here, and deliberately so: it spends a provider allowance and is opt in by label. Two pieces of evidence stand in for it. First, the same gate-step pattern in the same workflow and the same run did take the enabled branch for the two other converted jobs, `Agent console (type + unit + build)` in 35 seconds and `Web E2E (full stack)` in 6 minutes 20, both of which run their steps only when their gate output is `true`. Second, the push to `main` that merging this creates is itself an enabled-path run, since `github.event_name == 'push'` satisfies the first arm, and it fails loudly if the guard stopped running. The guard's own coverage is unchanged and still enforced from the Go side by `TestNoCISurfaceCallsAPaidCompletionModel` in `apps/control-plane/internal/routing/ci_paid_model_guard_integration_test.go`, which runs in `Go tests (control-plane)` and passed on this branch. ## Also in this diff `.github/MERGE-POLICY.md` lists all ten required contexts, nine from `ci.yml` and the tracking gate, and its note that `ci.yml` is the only workflow allowed to publish a required check is corrected. The stale count in `oauth-scope-gate.yml` becomes ten. `CLAUDE.md` gains a two sentence pointer beside the orchestrator contract and duplicates none of the rule. No `.wolf/` file is touched. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added pull request tracking validation requiring links to triaged issues with appropriate priority and area labels. - Added automatic checks for required milestones on urgent issues and matching pull request labels. - Added limited exemptions for automated pull requests and bug-log-only changes. - **CI & Workflow Improvements** - Added the tracking validation as a required status check. - Required additional console, integration, and end-to-end checks for the main branch. - Updated checks to report clear results even when changes do not affect their areas. - **Tests** - Added coverage for tracking validation and exemption scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
1 participant
Closes #1696
What was wrong
Open WebUI's Python retrieval path is configured with
RAG_OPENAI_API_BASE_URL=http://edge-api:8080/v1andRAG_OPENAI_API_KEY=${OWUI_SHIM_KEY}(deploy/docker/docker-compose.yml), so every embedding it produces is a real, metered call on Hive's own gateway. Web search indexing is the loudest: one search fetches several pages, chunks them and embeds every chunk, which is where issue #1609's burst of roughly two hundred embedding calls came from.All of it settled against whichever account owns
OWUI_SHIM_KEY. So the spend was real and it was metered, and it landed on a platform account: the searching customer's balance and usage showed nothing they had done, one account absorbed the embedding spend of every tenant at once, and the per-tenant budget work was defeated because the spend never reached the tenant the cap applies to.Where attribution now comes from
The carrier this deployment already has, not a second scheme.
X-Hive-Upstream-Authis read byOWUIUnwrap(apps/edge-api/internal/auth/owui_unwrap.go) and honoured only when Authorization is exactly the shim key, which is the same gate the body carrier sits behind.deploy/docker/owui-patches/hive_agent_proxy.pyalready uses it for the agent-task endpoints. The credential on it is the signed-in user's own OAuth access token, verified by the JWT middleware against JWKS like any other session bearer, so this is a verified principal rather than an asserted identity.Two halves, both required, and useless apart.
edge-api.
/v1/embeddingsjoinsrequiresPerUserAuth, so a shim-key embeddings call carrying no per-user token is refused instead of billed to the shim, and one that does carry a token has its Authorization rewritten onto that token. The API-key handler cannot then serve it:inference.Orchestrator.Authorizeresolves anhk_key out of the Authorization header and a Supabase JWT is not one. So/v1/embeddingsbecomes JWT-aware through the existingjwtAwareChatHandler, exactly as/v1/chat/completionsalready is, and the newapps/edge-api/internal/embeddingspackage serves the session arm.open-webui. A build-time splice attaches the carrier inside
agenerate_openai_batch_embeddings. That is the single function every embedding on this deployment leaves through:RAG_EMBEDDING_ENGINEisopenai, sogenerate_embeddingstakes the openai arm, and both producers reach it, theget_embedding_functionclosure (retrieval queries, and web search indexing throughsave_docs_to_vector_db, which builds its own function with the same factory) and document ingest. Theuserargument was already threaded there by upstream forENABLE_FORWARD_USER_INFO_HEADERS, which is what makes this a header injection rather than a plumbing change: the identity was already present, it was simply never turned into a credential the gateway could bill against.This lands in
deploy/docker/owui-patches/, not invendor/open-webui, because the chat image builds only the frontend from the vendored tree and takes the backend from the pinned image, so a backend edit undervendor/would be inert. The splice literal was verified against the pinned image itself (ghcr.io/open-webui/open-webui:v0.10.2@sha256:9fcea9c6...), where it occurs exactly once, and the patch was applied to that extracted copy as well as to the vendored one.Coordinating with PR #1699 rather than inventing a second scheme
PR #1699 charges the Go
web_searchandweb_fetchtools per call throughsessionbilling. One attribution mechanism does serve both, and this uses it: the hold, the refusals and the single terminal state all come frominternal/sessionbilling, the same lifecycle session chat, RAG chat, the agent-task gate and now the web tools settle through. What differs is only the unit, and it differs for a real reason rather than by preference: a web tool call is a per-call charge against an internal price-carrier alias with no route, while an embedding is token metered against a real catalog alias with a route, so this path charges withinference.CreditsForTokens(D-031's per-million arithmetic) and takessessionbilling.Reserve, notReserveCharge.The chokepoint differs for the same reason #1699 gives:
rag.HTTPEmbedder.EmbedBatchis edge-api's own outbound embedder to LiteLLM and sees none of this traffic. The gap it covers, unmetered RAG ingest and query embeddings, is issue #1644 and stays open. This PR does not touch it.Bounds, stated because this is the money path
No customer may be charged for another customer's search. The principal is read from
auth.UserFrom, which only the JWT middleware populates, and the tenant it carries is whatsessionbillingresolves a billing account from.TestTwoTenantsAreChargedSeparatelydrives two searches through one handler with two billable tenants and asserts the two charges land on the two different accounts. MutatingTenantID: user.TenantIDto a constant turns it red, which was confirmed rather than assumed.No spend may silently land on a platform account after this change, and the paths that still do are named. Embeddings no longer can: a shim-key call with no carrier is refused at the middleware, and a session call with no principal, no billing account, an unreadable billing position or an unwired accounting seam is refused in the handler, each with a test asserting the upstream recorded zero dispatches.
Three paths still bill a platform account. Two are audio and inside the shim family, one is outside it and was found by the independent review; none of them is silent any more:
/v1/audio/speech, text to speech.AUDIO_TTS_OPENAI_API_KEYisOWUI_SHIM_KEY./v1/audio/transcriptions, speech to text.AUDIO_STT_OPENAI_API_KEYisOWUI_SHIM_KEY.HIVE_AGENT_ENGINE_LLM_API_KEY, one static key on the host launcher, so every sandboxed agent's inference settles against whichever account owns it. Same shape as this issue on a much more expensive surface. Filed as Sandboxed agent inference bills the platform account that owns HIVE_AGENT_ENGINE_LLM_API_KEY #1723.The two audio ones are named in
requiresPerUserAuth's own comment, incheckOWUIShimKey's, and in a test that asserts they still pass through, so the next reader finds them rather than rediscovering them. Filed as issue #1713.GET /v1/modelsalso presents the shim key and carries no spend at all.One more, not a platform account but the same family of quiet absence: an embeddings 200 carrying a vector document and no
usageblock settles at zero on both arms and is served free, with one log line as the only signal. Pre-existing, confirmed on both paths during review, filed as #1724.Existing chat behaviour must not break. A user who can chat today already has a resolvable OAuth session, because
/v1/chat/completionshas required the same credential since #1567 and would otherwise 401. Web search happens inside a chat turn, so requiring that same credential for the search's embeddings breaks nothing that works. A customer calling/v1/embeddingswith their ownhk_key is untouched:requiresPerUserAuthis only ever consulted when Authorization is exactly the shim key, andTestOWUIUnwrap_EmbeddingsWithARealAPIKey_PassesThroughholds that in place.The one behaviour that does change for a solvent user is which balance is consulted, which is the point of the issue.
What the carrier may be forwarded to, and what this PR does not keep
Added after the independent security review, which found both of these.
A user credential only ever goes to the gateway named in the environment.
agenerate_openai_batch_embeddingstakes its destination fromapp.state.config.RAG_OPENAI_API_BASE_URL, andPOST /api/v1/retrieval/embedding/updatelets any instance admin rewrite that at runtime. On this shared chat instance every tenant OWNER is an instance admin. Before this PR that knob leaked one shared platform key; attaching a per-user session bearer to the same knob without a check would have turned it into a cross-tenant session harvest, which is a strict escalation and the exact outcome the brief calls worse than the billing defect.attachtherefore compares the destination's origin againstRAG_OPENAI_API_BASE_URLandOPENAI_API_BASE_URLread fromos.environ, never from the persistent config, and refuses anything else before a credential is even resolved. Same propertyhive_agent_proxy.pyalready relies on. Proved inside the pinned image with the gateway and the hostile destination on genuinely different origins.Two producers that had no user now carry one.
embed_knowledge_base_metadata(six callers) and thequery_knowledge_basesbuiltin both called the embedding function with no user, so the new refusal would have broken them, and the first fails inside its own swallowedexcept: knowledge base metadata embedding would have stopped working with no visible error and the admin reindex would have reported zero of N. Both are threaded through the patch rather than left broken or merely named. Fixing mis-attribution by silently breaking ingest is not a trade this PR makes.The rate limit that made #1609 visible is gone on this path, and the credit hold is what replaces it.
authz.NewLimiteris reachable only throughinference.Orchestrator.Authorize, which resolves anhk_key, so JWT session traffic never touches it. #1609's burst was rate limited because those calls carried the shim key; after this PR they carry a user JWT and nothing rate limits them. What bounds them instead is the per-callDefaultHoldEmbeddingshold against the searching customer's own balance, which is a real bound and arguably the more useful one for an expensive burst, and which bounds a solvent tenant not at all. What makes that survivable is the other half of #1609:RAG_EMBEDDING_BATCH_SIZEdefaults to 100 rather than 1, so a five page search is a handful of requests per turn. If that default is ever taken back to 1, this path loses its rate limit and regains its burst on the same day.The per-tenant budget motivation in #1696 is only half delivered, and this PR does not claim otherwise. Control-plane budget windows are keyed by API key id, so a JWT settled charge still does not reach them. What is delivered, and it is the important half, is that the spend lands on the correct billing account and consumes the correct balance, which makes the account level cap and the customer's own usage correct.
Fail closed, per D-034
Six refusals, all before any provider is reached, each with a test asserting the upstream recorded zero calls:
/v1/embeddingsbilling_not_configuredbilling_unavailableupstream_actualaliasCreditsForTokensanswers zero by design for that mode and an embeddings response has no content to price instead, so serving one would charge nothing while looking completedThe Python half raises rather than degrading when no user resolves. That raise is redundant with edge-api's refusal and is kept anyway, because it names the real cause in the chat container's own log, which is the one place the gateway's 401 cannot explain itself.
The hold reaches a terminal state exactly once: charged, or released by the deferred call on every other exit, never both.
TestReleasesTheHoldWhenTheUpstreamFailsasserts one hold and one release with no charge.Proof: a real ledger, read out of the database
Live stack, a real chat account, a real search, and the balances read back from Postgres rather than from a log line. Figures and method are in the proof comment on this PR.
Tests
apps/edge-api/internal/embeddings/handler_test.go: attribution to the searching account with the exact charge magnitude (1,500 prompt tokens at 2,000,000 credits per million is exactly 3,000 credits, so a settlement that priced the wrong quantity fails even though "a charge happened" would pass), two tenants charged separately, the six refusals above, the hold released on a failed upstream, the Enterprise posture served unheld, and the response carrying the Hive alias rather than the provider.apps/edge-api/internal/auth/owui_unwrap_test.go: the refusal without a carrier, the rewrite with one, anhk_customer key passing through untouched, and a non-JSON shim-key body on this path still refusing. Confirmed RED against the pre-change predicate: the request was forwarded with a 200 and the shim key intact.scripts/test_owui_embed_attribution.py: the module driven for real against a stubbed resolver (carrier attached, refusal when no user resolves, no mutation of the caller's headers, no cross-user cache leak, no caching of a failure), the splice asserted by AST to sit inside the embedding function and before the POST, and the Go half, the compose wiring and the image build asserted to ship together.scripts/test_owui_task_upstream_auth.py'srequiresPerUserAuthguard was a frozen copy of the whole function body, so it went red on a change that NARROWS what the shim key may do. Rewritten to assert every required path is still listed, which still fails on a removal (the shortcut it exists to catch) and does not fail on an addition.Green:
go vet ./apps/edge-api/...,go test ./apps/edge-api/... -count=1 -short, andmake test-scripts.No UI surface changes, so the visual proof rule does not apply to the diff itself; the ledger proof above is the evidence for the behaviour claim.
Buglog entry
{"id":"bug-2026-09-02-chat-search-billed-to-shim","date":"2026-09-02","title":"chat web search embeddings were billed to the shared OWUI shim account instead of the user who searched","error_message":"none: the failure was silent by construction. Open WebUI's Python retrieval path posted to edge-api /v1/embeddings with RAG_OPENAI_API_KEY, which is OWUI_SHIM_KEY, so every hold and every charge resolved to that key's account and the searching customer's usage showed nothing.","root_cause":"OWUI's embedding calls carry no request body, so the __metadata.upstream_auth carrier that attributes a chat completion cannot reach them, and requiresPerUserAuth in apps/edge-api/internal/auth/owui_unwrap.go deliberately excluded /v1/embeddings for that reason: the shim key was treated as the intended credential there. The header carrier X-Hive-Upstream-Auth already existed for the same problem on bodyless agent-task calls and was never applied here. Compounding it, edge-api had no JWT-session embeddings handler at all, so even a correctly attributed request could not have been served.","fix":"Add /v1/embeddings to requiresPerUserAuth so a shim-key call with no per-user token is refused rather than billed to the shim. Add apps/edge-api/internal/embeddings, a JWT-session handler that holds, charges and settles through the existing sessionbilling lifecycle at the alias's catalog token price. Attach the signed-in user's access token to X-Hive-Upstream-Auth inside agenerate_openai_batch_embeddings via deploy/docker/owui-patches/apply_embed_attribution_1696_patch.py, raising rather than falling back to the shim when no user resolves.","tags":["billing","money-path","attribution","fail-closed","open-webui","web-search","embeddings","edge-api","issue-1696"]}