Repository navigation
feat(chat): ship a user-created skills library at its own Hive route - #1388
Merged
Merged
Conversation
The owner asked for the ability to add new skills. Everything behind one already worked and was never removed: the skill table and its migration, the /api/v1/skills router, and the prompt injection in the chat middleware that appends a selected or mentioned skill's content as a system message. Only two things were missing, a route that renders the editor and the permission that lets an ordinary customer reach the create endpoint. The surface ships at /skills with its own shell navigation row rather than as a restored Workspace tab. Issue #783 removed that tab and Caddyfile.owui still 404s its path, so a restored tab would have needed a proxy rule reversed to land on a path the target navigation deletes anyway. The Hive route needs no proxy change and sits outside the workspace layout's permission guard, for the same reason issue #1109 moved Knowledge to /knowledge. The permission is newly load bearing rather than newly invented. Until 2026-08-23 every tenant owner was promoted to Open WebUI admin and passed every surrounding role-or-permission gate regardless; since 20260823_03_owui_role_never_admin.sql only a platform admin is, so workspace.skills now governs every ordinary customer and defaults to false upstream. Granting it needs more than an environment variable, because user.permissions is a single persisted config row seeded on first boot: the database has outranked the environment ever since, exactly the trap hive_rag_env_config.py already exists for. That module gains a seam for leaves inside the permission tree, which it reads, merges and writes back whole. A dotted config key would have written a row nothing reads. Sharing is deliberately left at upstream's defaults, both false, so a skill is private to the account that wrote it. Cross-tenant injection was checked rather than assumed: filter_allowed_access_grants strips public grants from a non-admin write, and GET /api/v1/groups filters to the caller's own memberships, so a member can target only their own tenant's group. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WyEwUxZCArdn1ZUDkTvuQ1
|
Warning Review limit reachedNext included review available in 31 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: Pro Plus Run ID: 📒 Files selected for processing (22)
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 |
…group grants Two findings from the pre-merge review, plus the layout defect the first proof capture showed. The permission guard was on the index page, and SvelteKit does not inherit a page's script down the tree, so /skills/create and /skills/edit rendered the full editor for a session that may not write skills. Moving the guard into a route layout covers all three, and the layout is also where the page chrome belongs: these are upstream Workspace components and a bare flex region gave them neither the sidebar-width constraint nor the horizontal padding, so the list rendered underneath the sidebar and the New Skill button was pinned to the window corner. The second finding is a real hole and it is the reason this needs a backend patch. Open WebUI's shared grant filter strips public grants and individual user grants from a non-admin write and never inspects group grants at all, so a hand-built payload can grant a resource to any group id on this shared instance, including another tenant's. Skills are where that matters most, because a skill body is appended verbatim to the chat request as a system message, which makes a cross-tenant grant prompt injection rather than an over-broad share. It is not reachable today, since a non-admin cannot enumerate another tenant's group id, but that is secrecy of an identifier rather than an authorization check, and one future surface that echoes a group id to a non-member removes it. The build-time patch drops group grants naming a group the caller is not a member of, at all three skill write sites. Scoped to the skills router deliberately. The same shared function serves knowledge, models, prompts, tools, notes and folders, and changing it centrally would alter grant behaviour on surfaces this change never exercised. Issue #1396 tracks that general case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WyEwUxZCArdn1ZUDkTvuQ1
sakibsadmanshajib
marked this pull request as ready for review
August 29, 2026 09:26
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
sakibsadmanshajib
left a comment
sakibsadmanshajib
left a comment
Owner
Author
There was a problem hiding this comment.
Pre-merge review, two streams
Stream 1, CodeRabbit CLI: SKIPPED. Not a pass. coderabbit review --agent --base main returned rate_limit / "Rate limit exceeded", repo wide, with waitTime: 51 minutes and onDemandReviewAvailable: false. Nothing was reviewed by it, and its absence must not be read as a clean result.
Stream 2, Antigravity (gemini-3.1-pro-high, effort high): RAN. One correction worth recording for the next person: agy executes in ~/.gemini/antigravity-cli/scratch, not in the checkout it is launched from, measured by asking it to print pwd and git rev-parse --abbrev-ref HEAD. Its first pass reviewed an unrelated diff about credit formatting that exists nowhere on this branch, and was discarded as hallucinated rather than acted on. The findings below come from a second run with the diff and the load-bearing surrounding code pasted inline.
Stream 3, security pass: RAN as part of stream 2's brief, because a user-authored artefact that reaches a model prompt is an input-parsing path. Its finding is the one that changed the diff.
Three findings, two accepted and fixed in 4391c2e, one rejected with evidence. Details inline.
The screenshots go to the visual-proof release rather than into the tree, per the merge policy that deletes every branch. This log is the half that has to be committed, because npm run lint:proof-tokens scans docs/proof/ and nothing else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WyEwUxZCArdn1ZUDkTvuQ1
Owner
Author
Visual proofA member creates a skill through the UI, and it changes the model's answer in the very next turn. Substrate: hive-open-webui built from this branch, run standalone, ordinary non-admin account, inference through an OpenAI-compatible endpoint. Frame 1 is the behavioural claim: one chat, one model, the same question twice, control turn three sentences with no marker, skill turn one sentence ending HIVE-SKILL-PROOF-OK-2026-08-29, a string that exists nowhere but the skill body. Frames 2 and 3 are the creation flow. Frame 4 answers the folded-in question: a per-user System Prompt also reaches inference. Full log and the limits of this substrate: docs/proof/user-created-skills-2026-08-29/log.md
|
sakibsadmanshajib
added a commit
that referenced
this pull request
Aug 29, 2026
…1405) (#1426) Closes #1405. ## What was happening Measured live on `https://chat-hive.scubed.co` on 2026-08-29: a 28.6 MB attachment produced a composer chip and a `POST /api/v1/files/?process=true` that had still not returned after 105 seconds, with no progress bar, no spinner text, no timeout and no error. The chip looked identical to the chip for a 12 byte file that uploaded in under a second. Separately, `evil.exe` uploaded and was processed with a 200. The stall matters as much as the missing cap. It is the interface asserting a state the backend has not reached, which is the quiet-absence shape this repository has spent the day removing. ## The enforcement already exists. It had no value to enforce Read out of the running container rather than assumed (`docker exec hive-open-webui-1`, image `hive-open-webui:v0.10.2-branded`, box at `69e9be9b2`): - `routers/files.py:315-323` refuses an extension outside `rag.file.allowed_extensions` with **400** naming the type, before the bytes reach storage. - `routers/files.py:355-361` refuses a file over `rag.file.max_size` megabytes with **413** naming the limit, and deletes the object it had just stored. - `main.py:2014` publishes `rag.file.max_size` to clients as `file.max_size`, which is what lets `MessageInput.svelte` refuse an oversized file **before the request is made**. Both config keys were unset on the box, so all three checks were inert. ## The trap, which is #722's and is why this needs more than an env var `docker-compose.yml` has carried `RAG_FILE_MAX_SIZE` since the #1108 follow-up, with an empty default and a comment saying it serves `rag.file.max_size` to clients so the composer's guard fires client-side. It never did. `deploy/docker/owui-patches/hive_rag_env_config.py`'s `RAG_CONFIG_ENV` did not list the key, so nothing reconciled it, and the row Open WebUI's `seed_defaults` wrote on first boot (`None`, meaning no limit) has outranked the environment ever since. `RAG_ALLOWED_FILE_EXTENSIONS` was not in compose at all. So setting either variable on an already-booted box was a silent no-op, exactly like #722, #772, #832 and #997, and the same shape as the skills permission #1388 fixed. This change uses the seam #1388 built. ## Where the limits are actually enforced **Server-side, in the upload handler, on every request.** The client-side check is a faster and more legible refusal of the same rule, driven by the same persisted value published through `/api/config`; it is not the enforcement and does not need to be trusted. A client that skips it gets a 413. One residual is stated rather than left unsaid: upstream gates the **type** check on `if process and allowed_file_extensions`, so a `POST ?process=false` skips it. The size cap is not gated that way and applies to every upload. This is pre-existing, is not made worse here (there was no allowlist at all before), and needs its own exact-literal backend patch, so it is filed as **#1425**. It is also why this change does not need image extensions in the allowlist and does not break image attachments: `MessageInput.svelte` uploads anything with an `image/` MIME type through that same `process=false` path. ## The values, chosen deliberately **Size: 25 MB.** Not a preference and not a guess. `RAG_MAX_UPLOAD_BYTES` is already `26214400` on edge-api and on the markitdown sidecar, and Open WebUI multiplies its megabyte value by `1024 * 1024`, so 25 is byte for byte the ceiling Hive already enforces on its own document ingest path. Accepting more in chat than the product accepts elsewhere would give two different answers to "how large a document can Hive read". It is comfortably above any realistic demo document, and if a real one is refused the operator raises `RAG_FILE_MAX_SIZE` in `.env`, documented in `.env.example`. A refusal is immediate and names the limit, so the failure mode of a cap set too low is a legible error rather than the stall this replaces. **Types: everything this deployment can turn into text.** Derived by a rule, not by taste: Open WebUI's own `known_source_ext`, plus the extensions `Loader._get_loader` names in its own branches (pdf, doc, docx, odt, ppt, pptx, xls, xlsx, csv, msg, rst, xml, html, htm, md, epub), plus txt. 69 entries. A narrower list would refuse a file the product could have read, which fails in front of a user and is worse than no cap. `.exe`, `.zip`, `.dll`, `.bin`, `.so`, `.msi`, `.apk`, `.jar`, `.iso` and `.dmg` are all outside it, and a test asserts that. An allowlist is the only type control upstream offers, and it is needed because `_get_loader`'s final `else` falls back to `TextLoader` for any unrecognised extension: without one, an executable is "processed" into mojibake, stored, and served back on request. ## Two coercions that carry weight - **`rag.file.max_size` persists as an `int`.** Upstream parses the variable with `int()`, so a first boot seeds a number. A string happens to work in both consumers by coincidence, and a reconciled value whose type differs from the seeded one is how `ui.enable_login_form` went wrong before. - **`rag.file.allowed_extensions` persists as a `list`.** This one is load bearing. Upstream evaluates `file_extension not in allowed_file_extensions`, so persisting the raw comma string would silently turn a membership test into a **substring** test: extension `df` would be admitted because `pdf` contains it, and so would every other extension that is a substring of an allowed one. ## What this does on failure - **A malformed `RAG_FILE_MAX_SIZE`** (`"25MB"`, `"twenty"`) raises at startup naming the variable. The container fails to boot rather than booting with no cap and looking configured. - **An allowlist that parses to no extensions** (`","`) raises the same way, because an empty list is falsy in upstream's `if process and allowed_file_extensions` and would turn the check off while the configuration says it is on. - **An unset or blank variable writes nothing**, so an administrator's own choice survives and an enterprise deployment that never sets these is not silently capped. - **An oversized file** is refused client-side with a toast naming the limit before the upload starts, and server-side with 413 if the client is bypassed. - **A disallowed type** is refused server-side with 400 naming the type; `uploadFile` throws the `detail` string, `MessageInput` surfaces it as a toast and removes the chip. ## Deliberately not in this pull request **Per-file progress on the composer chip.** The reported stall was an unbounded upload, and a 25 MB cap bounds it: an oversized file is now refused before the request is made rather than hanging. A progress indicator is a separate frontend change with its own design questions and belongs in its own pull request. Saying so here rather than quietly shipping half the issue. ## Tests `scripts/test_owui_rag_env_config.py`, run by `make test-scripts`, which is a required CI check. Eight new cases. Seven were verified red before the implementation existed, individually, with the failures recorded: ``` FAIL test_upload_size_cap_is_reconciled_as_an_integer KeyError 'rag.file.max_size' FAIL test_upload_type_allowlist_is_reconciled_as_a_list KeyError 'rag.file.allowed_extensions' FAIL test_a_malformed_size_cap_is_refused_rather_than_ignored AssertionError a non-numeric RAG_FILE_MAX_SIZE was accepted PASS test_unset_upload_limits_leave_the_persisted_values_alone FAIL test_compose_sets_the_upload_size_cap FAIL test_compose_allowlist_refuses_executables FAIL test_compose_allowlist_covers_every_format_this_deployment_can_read FAIL test_env_example_documents_the_upload_limits ``` The eighth, `test_unset_upload_limits_leave_the_persisted_values_alone`, passed before the change and is reported as what it is: an invariant guard, not a red-to-green test. It could not go red beforehand because the keys did not exist at all. It can now: coercing a blank value to `0` or `[]` fails it, which is the regression it exists to catch (issue #797). `make test-scripts` exits 0. `python3 scripts/test_owui_rag_env_config.py` prints `ok`. ## Not verified end to end, and why **No 30 MB file was uploaded against a running stack carrying this change, and I am not claiming otherwise.** The chat stack cannot be started locally: this checkout's `.env` has `SUPABASE_URL`, `SUPABASE_ANON_KEY` and `SUPABASE_SERVICE_ROLE_KEY` empty, and #1254 covers the wider breakage. The deployed box cannot be used either, because the change is not on it until this merges, and mutating its persisted config by hand to simulate the result would be a production change made outside the deploy path during a period when the owner may walk the demo at any time. What was verified, on the real substrate: 1. The enforcement code inside the **running** `hive-open-webui-1` container, both the 400 and the 413 branch, at their real line numbers, matching the vendored copy this repository reasons about. 2. `/api/config` publishing `rag.file.max_size` as `file.max_size` in that same container, which is the mechanism the client-side refusal depends on. 3. The box is at `69e9be9b2`, confirmed with `git -C ~/hive rev-parse HEAD` rather than by trusting a green deploy. 4. The reconcile itself, by unit test, including both coercions and both refusal paths. The gap between that and an end-to-end upload is: this change writes two config rows, and the rows are then read by code that was verified to exist and to enforce. The first deploy after merge closes it, and a 30 MB attachment plus an `.exe` against the deployed chat is the acceptance test. No UI code is touched, so the visual proof rule does not apply to this diff; the user-visible behaviour change comes entirely from Open WebUI's own existing error paths. ## Buglog entry ```json {"date":"2026-08-29","title":"Chat upload had no size or type limit: a 30 MB file stalled with no error and an .exe was accepted","error_message":"POST /api/v1/files/?process=true never returns for a 28.6 MB attachment; no progress, no timeout, no error. evil.exe uploads with 200.","root_cause":"Open WebUI enforces rag.file.max_size and rag.file.allowed_extensions in upload_file_handler, but both were unset. RAG_FILE_MAX_SIZE was in docker-compose.yml with an empty default and a comment claiming it fed the client-side guard, while the key was missing from hive_rag_env_config.py's RAG_CONFIG_ENV, so the persisted first-boot row outranked the environment and no value could ever reach a booted box. RAG_ALLOWED_FILE_EXTENSIONS was absent entirely.","fix":"Reconcile rag.file.max_size and rag.file.allowed_extensions through hive_rag_env_config.py, with an int coercion for the size and a list coercion for the allowlist (a persisted comma string turns upstream's not-in membership test into a substring test). Compose defaults 25 MB, matching RAG_MAX_UPLOAD_BYTES 26214400, and an allowlist derived from known_source_ext plus the loader's own document branches. A malformed size or an empty allowlist fails startup rather than booting unenforced.","tags":["open-webui","uploads","persistent-config","silent-failure","issue-1405","issue-722"]} ```
This was referenced Aug 29, 2026
fix: put the skills surface inside the coverage denominator and make a name collision readable
#1451
Merged
sakibsadmanshajib
added a commit
that referenced
this pull request
Aug 29, 2026
## Summary This is the batched buglog follow-up for the pull requests merged to `main` on 2026-08-29. Its diff is `.wolf/buglog.jsonl` and nothing else. Per `.claude/rules/openwolf.md`, every fixed bug, error, failed test or failed build must be logged, but the line may never be appended on a fix branch. `merge=union` in `.gitattributes` resolves concurrent appends locally and is ignored by GitHub's server side merge, so two branches that both appended land in hard conflict there. An unmergeable pull request gets no `refs/pull/N/merge`, no `pull_request` run and therefore zero checks, and the required status gate then blocks the merge for a reason the page never states (issue #873). Each fix accordingly carried its entry in its own pull request body, and this pull request copies them onto `main` in one batch, which the protocol explicitly prefers over one pull request per entry. ## Scope examined Fifty nine pull requests merged to `main` on 2026-08-29. Forty eight of them carried at least one entry, for eighty two entries in total. Thirty two of those were already on `main` and are skipped, leaving fifty appended here from thirty four pull requests. The largest block of skips comes from #1342, the equivalent batch for the 2026-08-28 merges, which merged earlier the same day and already landed thirty six entries covering #1257, #1268, #1276, #1277, #1287, #1292, #1293, #1294, #1296, #1301, #1303, #1305, #1313, #1335 and #1337. ## What landed Fifty entries appended, one JSON object per line, append only. The 232 pre-existing lines are byte identical to `origin/main` (verified by hashing the first 232 lines of the result against the base file). Every line in the resulting file parses as JSON and carries `error_message`, `root_cause`, `fix` and `tags`. | Source | Entries | |---|---| | #1083 | 2 | | #1277 | 1 | | #1278 | 1 | | #1298 | 1 | | #1334 | 1 | | #1336 | 3 | | #1343 | 1 | | #1346 | 1 | | #1351 | 1 | | #1365 | 2 | | #1368 | 1 | | #1369 | 1 | | #1371 | 3 | | #1375 | 3 | | #1376 | 1 | | #1378 | 1 | | #1379 | 2 | | #1388 | 5 | | #1389 | 3 | | #1390 | 2 | | #1393 | 1 | | #1394 | 1 | | #1410 | 1 | | #1417 | 1 | | #1421 | 1 | | #1423 | 1 | | #1424 | 1 | | #1426 | 1 | | #1429 | 1 | | #1431 | 1 | | #1433 | 1 | | #1434 | 1 | | #1436 | 1 | | #1439 | 1 | Entries are copied verbatim from their source pull request bodies. Nothing was rewritten, no field was invented, and no field was added. No JSON needed repair: all eighty two extracted entries parsed on the first attempt and all four required fields were present on every one. ## Merged pull requests that carried no entry Eleven of the fifty nine. Recorded here because the gap is itself the useful signal. | Pull request | Title | Assessment | |---|---|---| | #1013 | chore(deps): bump the go-minor-patch group across 1 directory with 4 updates | Dependabot bump, no defect fixed, no entry expected | | #1015 | chore(deps): bump the go-minor-patch group across 1 directory with 6 updates | Dependabot bump, no entry expected | | #1016 | chore(deps): bump golang from 1.26-alpine to 1.27-alpine in /deploy/docker | Dependabot bump, no entry expected | | #1218 | chore(deps): bump postcss from 8.5.19 to 8.5.26 in /apps/desktop | Dependabot bump, no entry expected | | #1219 | chore(deps): bump golang.org/x/crypto from 0.41.0 to 0.52.0 in /apps/control-plane | Dependabot bump, no entry expected | | #1342 | chore: batch buglog entries for the 2026-08-28 merges | The previous batch pull request itself, correctly carries no entry of its own | | #1364 | chore: remove four dead skills and record the patterns that cost time | Protocol gap. The body records patterns that cost time, which is the shape of a buglog entry, but none was written as one | | #1383 | test: retire stale expected-failure markers, restore the ones that are true (#1381, #1382, #1324) | Protocol gap. Stale `it.fails` markers reading as red is a real defect that was fixed here and should have carried an entry | | #1384 | docs: correct D-047, hive-auto reverted to variable pricing (D-059) | Decision ledger correction, arguably a documentation defect, no entry written | | #1387 | chore(deps): bump next from 15.5.23 to 16.3.3 in /apps/agent-console | Dependabot bump, no entry expected | | #1398 | docs: rescue the 2026-08-25 parity captures and add the 2026-08-29 QA matrix evidence | Documentation and evidence rescue, no entry written | Six of the eleven are Dependabot bumps and one is the previous batch, so the genuine protocol gaps are #1364, #1383, #1384 and #1398. Of those, #1383 is the one worth a follow-up: it fixed a real defect class (a stale expected-failure marker reads as a red "Expect test to fail" and gets dismissed as pre-existing) and left no record. ## Entries skipped as already present Thirty two. Thirty of them matched an entry already on `main` on `error_message`, `id` or `fix`. Two more from #1278 are semantic duplicates that an exact match would have missed, and were skipped after reading the landed entries they duplicate: - #1278's `streaming content_block_start omits text field` entry is covered by the consolidated `bug-2026-08-28-anthropic-sdk-wire-conformance` entry landed from #1296, whose root cause names the same `omitempty` on `StreamContentBlock.Text`. - #1278's `GET /v1/models leaked an upstream provider name` entry is covered by `BUG-1284`, landed from #1300, which names the same `public.model_aliases.summary` publication path. #1278's third entry, on `top_k` forwarding producing a 400, is not covered anywhere on `main` and is appended here. #1342 recorded #1278 as fully "merged into #1296", which was accurate for two of its three entries. ## Note on entry quality One appended entry is thin: #1277's parity re-score record carries `error_message` of `n/a` and a root cause of "console had no privacy/data-policy surface at all". It is a parity gap record rather than a defect record. It is included exactly as written rather than embellished, per the protocol's preference for the author's own words. ## Test plan - [x] Branch cut fresh from `origin/main`, diff is `.wolf/buglog.jsonl` and nothing else - [x] First 232 lines byte identical to the base file (md5 match) - [x] All 282 resulting lines parse as JSON and carry `error_message`, `root_cause`, `fix` and `tags` - [x] No `.wolf/` telemetry (`anatomy.md`, `memory.md`, `token-ledger.json`, `hooks/_session.json`, `buglog.json`) in the commit - [ ] The six required checks report green via the inert path allowlist in `.github/workflows/ci.yml` --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
sakibsadmanshajib
added a commit
that referenced
this pull request
Aug 29, 2026
…l name/id uniqueness to the tenant (#1397) (#1437) ## Summary PR #1388 shipped a live, user-facing Skills authoring surface reusing upstream Open WebUI's `skills.py`, but that router never joined the #1186 tenant-isolation sweep, and its `skill.name` column (plus the primary-key `id` slugified from it) is unique instance-wide on a chat backend shared by every Hive tenant. This PR closes both gaps for `skills.py`. **#1186 half.** Extends `apply_router_authz_family_patch.py`, the file that already fixes this exact bare `user.role == 'admin'` bypass shape on 11 sibling routers, with `skills.py`'s five unflagged sites: the by-id read (`GetSkillById`), toggle, update, access-update and delete gates. The three listing routes (`GET /`, `GET /list`, `GET /export`) already flag-gated correctly and needed no change. Adds the matching consolidated drift-guard assertion to `Dockerfile.open-webui`. **#1397 half**, three new files, following the established `apply_*_patch.py` / `hive_*.py` shape: - `hive_skill_tenant_scope_migration.py`: a new Alembic migration that drops the single-column unique on `name` and adds a nullable `tenant_group_id` column plus a composite `(tenant_group_id, name)` unique index. Raw-SQL table rebuild, because SQLite has no `ALTER TABLE DROP CONSTRAINT` and the live DDL on the demo box confirms `UNIQUE (name)` is a real table-level constraint, not a droppable named index. - `apply_skill_tenant_scope_model_patch.py`: teaches `models/skills.py`'s `insert_new_skill` to persist `tenant_group_id`, never exposed on any API response (no `SkillModel`/response subclass declares it). - `apply_skill_tenant_scope_router_patch.py`: resolves the caller's create-time **id scope** and prefixes `id` with it. `id` is the primary key and a bare `slugify(name)`, and the id check runs BEFORE name uniqueness in `CreateNewSkill`, so it is the one that actually collides first. ## Who is protected, precisely (corrected after independent security review) An earlier revision of this PR overstated what the id-scope logic did. The security reviewer traced `_hive_resolve_tenant_group_id` and the id-prefix computation and found that two different ungrouped accounts both resolved to the same empty prefix, so they still collided on the bare id and still got 400 `ID_TAKEN`, exactly as before this change. The PR body's "no error" / "ungoverned personal namespace" claim for that case was wrong. Fixed, not just reworded: the id scope is now a real three-case fallback (`_hive_id_scope` in `apply_skill_tenant_scope_router_patch.py`): 1. **A tenant-grouped ordinary member** scopes to their `tenant_<uuid>` OWUI group, shared with every other member of that tenant. This is the case #1397 is actually about, and it is now fully closed: two different tenants can use the same skill name and neither the id nor the name collides. 2. **An ungrouped ordinary member** (checked live against the demo box: this is the COMMON case today, not an edge case -- only one `tenant_*` group exists on the whole instance, with one member, and neither `demo@hive-demo.invalid` nor the platform admin account is in any `tenant_*` group) now falls back to their own `user_id` as the id scope, instead of getting no scope at all. Two unrelated ungrouped members can now each have a skill called "Research". This closes #1397 for these accounts too, not just for the provisioned minority, and reuses `user_id`, which already exists on every skill row, rather than inventing a new identity concept. 3. **A platform admin** gets no scope at all, deliberately: an admin is not a tenant customer, and a shared, flat, platform-wide id namespace for admin-published skills is the intended behavior (two admin accounts publishing "Research" SHOULD collide, matching a shared catalogue). The persisted `tenant_group_id` DB column stays the narrow, honest fact (the real tenant group, or `NULL`) and is NOT widened to the user-id fallback: `tenant_group_id` promises a tenant, `user_id` is already a separate column with that exact meaning, and conflating them would mislead a future reader. This is safe for the `name` column too, not just `id`: NULL is never equal to NULL under a UNIQUE index (SQLite, same as Postgres/MySQL), so two ungrouped members' skills, both `tenant_group_id = NULL`, already do not collide on `name` either, with no column change needed. Only `id` (the primary key, whose default was an unprefixed bare slug shared by everyone with no scope) needed the fallback. New regression test added at the reviewer's request (`scripts/test_owui_skill_tenant_scope.py`): two different ungrouped members computing a create-time id for the same display name now get two different ids; the same ungrouped member reusing their own name still gets the same id (duplicate detection within one account is not accidentally disabled by the fallback). ## What layer actually denies a cross-tenant read or write Stated explicitly, per the reviewer's note, so nobody later assumes database enforcement that does not exist: **pure Python application logic** in `routers/skills.py` -- ownership (`skill.user_id == user.id`), `AccessGrants`, and `BYPASS_ADMIN_ACCESS_CONTROL` being `false`. This is a plain SQLite file with no row-level-security concept at all. Issue #896 (a permissive blanket-allow RLS policy) is on a completely unrelated Supabase Postgres table, `account_memberships`, and has no bearing on this SQLite skills table. ## Not in scope #1396 (group access grants not filtered against another tenant's group id). The skills half of #1396 was already closed by PR #1388's `apply_skill_group_grants_patch.py`, which this PR reads, tests against in the exact same production application order, and does not modify. The general cross-router #1396 issue stays open. ## What the follow-up authoring-UI PR may now rely on `skill.id` and `skill.name` are unique within the caller's tenant (or, for an ungrouped account, within that individual account) rather than instance-wide, so the UI need not pre-check for a cross-tenant name collision. Every by-id read/update/toggle/delete route consults `BYPASS_ADMIN_ACCESS_CONTROL` before letting a bare admin role through, matching every other router in this deployment. Both grouped and ungrouped accounts now get a working namespace: this closes #1397's practical blast radius for everyone on the deployment, not only accounts with a real `tenant_*` group. **This now unblocks that second PR**: authoring was blocked on scoping actually working, and per the above it now does, for both the grouped and ungrouped case. ## Verification **Automated** (`python3 scripts/test_owui_skill_tenant_scope.py`, 33/33 checks green): - Runs the real migration SQL against a genuine `sqlite3` connection seeded from the real old-schema DDL. Negative control reproduces the bug pre-fix. Fix proven post-rebuild. **Mutation test**: a genuine same-tenant name collision still fails post-fix. A pre-existing row survives the rebuild untouched. The migration's `down_revision` is independently recomputed from all 48 vendored migration files on every test run and asserted to match the actual chain head (a self-review catch: an earlier draft named a mid-chain revision as the head, which would have branched Alembic's chain on deploy; fixed, and this assertion is a permanent regression guard against it recurring on a future vendor bump). - Applies both new source patches to the real vendored `skills.py`/`models.py`, in isolation and idempotently, asserts marker counts, confirms valid Python. - Extracts and exercises `_hive_resolve_tenant_group_id` AND the new `_hive_id_scope` directly against fake group memberships and fake users, including the reviewer's exact regression case (two ungrouped members, same name, different resulting ids) and a same-account duplicate-detection control. - Separately confirmed the `#1186` family extension, the existing `#1396` patch, and the `#1397` router patch chain cleanly on the real vendored `skills.py`, in production Dockerfile order, landing 5 + 4 + 3 markers with no collision in any application order. **Live, against the real demo box**, fully reversible, zero footprint after. Used the same server-side token-mint technique `deploy-demo-box.yml` already uses in production (`scripts/owui-mint-admin-token.py`'s pattern: a 5-minute JWT signed with the container's own `WEBUI_SECRET_KEY`, no password, no OAuth journey, no SOCKS proxy). Two pre-existing e2e fixture accounts, added to two throwaway `tenant_*` OWUI groups created and deleted by the check script. Neither `demo@hive-demo.invalid` nor the platform admin account was touched. Against the CURRENT, unpatched production code (this PR not yet deployed): | Step | HTTP status | Body | |---|---|---| | Tenant A creates skill "Research" | 200 | `{"id":"research", ...}` | | Tenant B (different tenant) creates "Research" | **400** | `Uh-oh! This id is already registered. Please choose another id string.` | | Tenant B reads Tenant A's skill by id | 401 | `You do not have permission...` | | Tenant B overwrites Tenant A's skill | 401 | `401 Unauthorized` | | Tenant B deletes Tenant A's skill | 401 | `401 Unauthorized` | This reproduces the exact #1397 bug live and today. The unpatched code computes the same bare, unprefixed id for every account regardless of tenant-group membership, so this same table is also the live "before" evidence for the ungrouped-account case: two ungrouped accounts collide via the identical mechanism, which is exactly what the new id-scope fallback fixes. A follow-up live re-verification after this PR merges and `deploy-demo-box.yml` redeploys should confirm both a grouped-tenant pair AND an ungrouped pair now get 200 with two different final ids, plus a same-account repeat create still returning `ID_TAKEN`. That is an orchestrator-owned step (merge triggers deploy in this repo), not a builder action. Full plan/spec/verification writeup: vault `plan-2026-08-29-skills-tenant-scoping.md`. ## Buglog entry ```json {"date": "2026-08-29", "error_message": "POST /api/v1/skills/create from a second tenant fails with \"Uh-oh! This id is already registered\" when reusing a display name already used by an unrelated tenant on this shared Open WebUI instance", "root_cause": "skill.id (PK, slugified from name) and skill.name were both unique instance-wide (vendor/open-webui/backend/open_webui/models/skills.py), and skills.py never joined the #1186 sweep that flag-gated the bare admin-role bypass on 11 sibling routers", "fix": "Alembic migration drops the single-column unique and adds a composite (tenant_group_id, name) unique index; router patch resolves a three-case id scope (tenant group, else the caller's own user id, else none for admin) and prefixes the create-time id; apply_router_authz_family_patch.py gains skills.py's five #1186 sites", "tags": ["security", "multi-tenant", "owui", "skills", "sqlite-migration"]} ``` ## Test plan - [x] `python3 scripts/test_owui_skill_tenant_scope.py` (33/33 green, includes the reviewer's exact regression test and two independent mutation tests) - [x] All three `skills.py` patches (`#1186`, `#1396`, `#1397`) apply cleanly to the real vendored source, chained in production Dockerfile order - [x] Live two-tenant reproduction against the real demo box (before-fix baseline, covers both the grouped and ungrouped collision mechanism) - [x] Independent security review: APPROVE, with the one finding above, now fixed - [ ] Post-merge: confirm `deploy-demo-box.yml` succeeds, then re-run the live two-tenant check (grouped AND ungrouped pairs) to confirm the after-fix state 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01WyEwUxZCArdn1ZUDkTvuQ1 --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
sakibsadmanshajib
added a commit
that referenced
this pull request
Aug 29, 2026
…a name collision readable PR #1388 shipped the user-created skills library at /skills. Two gaps it left, both found by capturing the surface on the deployed box rather than by reading the diff. The surface was swept by nothing. The chat-coverage engine enumerated home, the sidebar, the model picker, the three composer surfaces, the user menu, search, workspace, the chat menus and the settings and workspace tabs, and /skills was in none of them. A frontend regression, a proxy rule that 404s the route, or user.permissions.workspace.skills reverting to its upstream default of false would each have emptied the surface with every gate still reporting green. It is now a swept surface with a presence bar, classified data driven because the index renders one row per skill the account owns. The duplicate-name error named a field the author never filled in. skill.id is unique across the whole instance and the editor slugifies it from the name, so two accounts naming a skill the same thing collide and upstream answers with "Uh-oh! This id is already registered. Please choose another id string." The skill holding that id is almost always one the author cannot read, so they search their own library, find nothing, and read it as a broken product rather than a taken name. The create route now explains that the name is taken, that the holder may be an account they cannot see, and that the id field is the escape hatch. It names neither the conflicting skill nor its owner: a global uniqueness check already leaks the bare fact that an id is in use and no wording takes that back, but naming the holder would turn an unavoidable existence oracle into a real disclosure. Every other failure passes through verbatim. Issue #1397 and PR #1437 scope uniqueness to the tenant, which is the real fix. This message still matters after that lands, because most accounts on this deployment have no tenant group at all and share one namespace regardless. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WyEwUxZCArdn1ZUDkTvuQ1
sakibsadmanshajib
added a commit
that referenced
this pull request
Aug 29, 2026
…a name collision readable (#1451) ## Why this is not the feature The task that produced this branch was written to build the user-created skills surface. It was already built: PR #1388 merged it at 2026-08-29T10:00:09Z, and `deploy-demo-box.yml` run 33261985611 put it on the box. Rebuilding it would have been waste, so this branch verifies what shipped on the deployed substrate and fixes the two gaps that verification found. Everything in the original brief that was framed as missing is present, and was confirmed live rather than read off the diff: - `/skills` returns 200 through Caddy and `/workspace/skills` still returns 404, so the surface issue #772 removed did not come back with it. - `user.permissions.workspace.skills` is `true` in the running container's own store, with every sibling untouched and no flat dotted row. That is the #722 persisted-config trap cleared on the real volume, which is the single claim most able to be true in the repository and false on the box. - An ordinary member signed in through the real OIDC hop, opened the editor, saved a skill, saw it listed, and had it offered by the `$` mention picker. PR #1388's own capture log named what it could not exercise, in its own words: "single sign-on, the Caddy front end, and the gateway hop". Its proof ran a standalone container against a throwaway volume with inference pointed at Groq. `docs/proof/skills-live-postmerge-2026-08-29/log.md` is that missing capture. ## Gap one: the surface was outside the coverage denominator `e2e/chat-coverage/surfaces.ts` swept `home`, the sidebar, the model picker, the three composer surfaces, the user menu, `search`, `workspace`, the chat menus, and the settings and workspace tabs. Nothing swept `/skills`, and `discoverWorkspaceTabs` never could: it enumerates `a[href^="/workspace/"]`, and this surface deliberately does not live there. Three separate things would have emptied the surface with every gate still green: a frontend regression, a proxy rule that 404s the route, and `workspace.skills` reverting to its upstream default of `false`. That is the quiet-absence shape this repository keeps paying for, so the surface is now swept and floored. The floor is a presence bar of 1, classified `dataDriven`, because the index renders one row per skill the signed-in account owns and pinning that count reds the gate whenever a run creates or deletes one. A floor of 1 still fails when the surface stops rendering, and a floor key a run never sweeps fails too, so deleting the entry point is caught rather than silently shrinking the denominator. Measured live rather than guessed: the index enumerated 47 controls with one skill owned and 42 with none, both far above the bar. ## Gap two: the duplicate-name error named a field the author never filled in `skill.id` is unique across the whole instance and the editor slugifies it from the name, so two accounts naming a skill "Research" collide. Upstream answers with `ERROR_MESSAGES.ID_TAKEN`: "Uh-oh! This id is already registered. Please choose another id string." That misdirects twice. It names a field the author never filled in, and the skill holding the id is almost always one they cannot read, so they search their own library, find nothing called Research, and read a taken name as a broken product. `lib/hive/skill-save-error.ts` rewrites that one sentence and passes every other failure through verbatim, because a rewrite that catches more than it means to is how a permission failure, a network failure and a validation failure all come to read as the same shrug. The regex is anchored on the whole upstream sentence, so the sibling `MODEL_ID_TAKEN` is not swept up with it. It deliberately names neither the conflicting skill nor its owner. A global uniqueness check already leaks the bare fact that an id is in use, and no wording takes that back while the check exists, but naming the holder would turn an unavoidable existence oracle into a real disclosure on an instance where reads are otherwise denied cross-account by ownership plus access grants. The create route is source-pinned to the helper. A helper nothing calls passes its own unit tests forever while the author on the box still reads the upstream sentence, so an unwiring fails here instead of in front of a customer. Proved able to fail by reverting the wiring: both pins went red, then green again. Only the create route is wired. `update_skill_by_id` raises no `ID_TAKEN`, so the edit route cannot produce this failure and is untouched. ## Gap three: a save could fail with no message at all Found while checking whether the rewrite would ever fire, which turned out to be the load-bearing question in the whole diff. `createNewSkill` does not reject on every failure. Its own catch does `error = err.detail` and rethrows only `if (error)`, so when the error body is not JSON carrying a `detail` key it leaves `error` undefined and resolves the promise with `null` instead. A network failure, a proxy error page whose HTML body makes `res.json()` throw, and any backend error using a different payload shape all take that path. The route's `.catch` therefore never ran for them, and `if (res)` had no else arm. The author clicked Save and nothing happened at all: no toast, no navigation, no indication whether the skill had been created. That is the same quiet-absence failure the rest of this PR exists to remove, sitting inside the handler it changes. The guard is in the route rather than in the shared upstream client, which every other skills caller uses and which this change exercises nowhere else. ## Gap four: the live coverage gate could not run at all Found while running the `surfaces=^skills$` sweep review asked for, and not scoped to this surface. The collapsed Hive shell renders **two** controls named "Open Sidebar": `count()` is 0 while the sidebar is open and 2 the moment it closes. Playwright's strict mode makes `isVisible()` on a two-element locator throw, and the `.catch(() => false)` beside it in `ensureSidebar` turned that throw into "not visible". The function clicked "Close Sidebar", succeeded, looked for the control proving the new state, got a strict-mode error instead of a boolean, and threw `the sidebar is not closed and neither toggle is on screen`. Every surface that pins the sidebar shut inherited it: `sidebar` and every `clickTop` surface, so the model picker, the three composer surfaces, the user menu and search. The live gate could not complete against the shipped shell, and nothing said so, because `live-sweep` runs only on `workflow_dispatch` or the `run-chat-coverage` label and a dispatch dies earlier still. `.first()` on both locators, which is what every other `getByRole` in that file already does. ## The sweep, and why CI could not run it `gh workflow run chat-coverage.yml -f surfaces='^skills$'` (run 33267620156) failed its own input check before reaching Playwright: `the live sweep needs: HIVE_QA_AGENT_EMAIL SUPABASE_ANON_KEY SUPABASE_SERVICE_ROLE_KEY`. All three are empty in this repository's Actions environment, so that stream is **SKIPPED**, not passed. Filed as #1459. The sweep was therefore run directly against the deployed box, driving the real engine rather than a description of it: the same `STATIC_SURFACES` entry, the same `enumerate` and `SELECTOR`, the same delta subtraction, the same `isDestructive` and `isStateful` partition, the same `proveByClick` and the real `valuePass`. ``` baseline sidebar pinned: true baseline (home, sidebar collapsed): 27 controls /skills enumerated 13 raw, 3 after the delta subtraction clickable: 2, stateful: 1 { "skills": { "total": 3, "proven": 3, "deferred": 0, "unproven": [] } } PROVEN Search Skills [input] proof=value -> hive-coverage-50095 PROVEN New Skill [a] proof=navigate /skills -> /skills/create PROVEN Select view [button] proof=dom what is on screen changed ``` Three controls, three proven, none unproven. The 13-to-3 subtraction is the `delta` fix measured: ten of the thirteen were shell, now attributed where they belong instead of counted twice. This is the empty-index case, which is also the weakest case; a populated index adds rows and a per-row switch, which is what the `dataDriven` classification keeps from being pinned. ## Merge order, satisfied **#1437 has merged** as `c205afb45`, closing #1397, and this branch is rebased onto it. The ordering constraint the earlier revisions of this description carried is discharged. That merge changed what a collision means, not only where uniqueness applies, so the message was corrected rather than left alone. `apply_skill_tenant_scope_router_patch.py` prefixes the id with a scope, and that scope is the caller's tenant group, or their own user id when they have none, or nothing at all for a platform admin. `ID_TAKEN` now fires only on a genuine duplicate inside the caller's own scope. For the common ungrouped account on this deployment the colliding skill is therefore **one of their own**, sitting in their own list. The earlier wording, "may belong to an account you cannot see", would have sent them looking away from the one place it actually is. It now reads "It may be one of your own skills, or one you do not have access to see", which is true for an ungrouped member colliding with themselves, a grouped member colliding with a peer, and an admin on the unscoped namespace. Dropping the scope claim during review is what made this a wording change rather than a factual correction: any scope word would have been wrong for some population of accounts here. ## Security Skills are user-authored text injected into a model prompt, so this was checked rather than assumed, including the parts this diff does not change. - **No skill field is rendered as markup anywhere.** `{@html` appears in none of `Skills.svelte`, `Skills/SkillEditor.svelte`, `SkillsModal.svelte`, `MessageInput/Commands/Skills.svelte`, `CommandSuggestionList.svelte` or `MentionToken.svelte`. Name, description and content all go through Svelte's escaping, so there is no stored-XSS path from a skill field into another viewer's DOM. - **Prompt injection is real and is self-scoped.** A skill body reaches the system message verbatim by design. `sharing.skills` and `sharing.public_skills` are both `false` on the deployed tree, confirmed in the live permission row, so a skill is private to the account that wrote it and a hostile body can only steer its own author's model. PR #1388's `apply_skill_group_grants_patch.py` already closed the group-grant hole that would have let one reach anyone else; #1396 tracks the same hole on the surfaces that still have it. - **The new error message discloses nothing new.** It reveals exactly what the 400 already revealed, that an id is in use, and withholds the name and the owner. - **Enforcement here is application logic, not a database layer.** Issue #896's permissive row level security is on an unrelated Supabase table and does not touch the Open WebUI SQLite skills table, so nothing behind this is backstopped by the database. ## Storage, unchanged and noted Skills remain rows in the Open WebUI SQLite volume, outside Supabase, outside every Hive migration and outside row level security. That was decided deliberately and is not revisited here. The limitation and what a real fix would cost are filed as **#1454**. Worth recording for the next reader, because two existing notes are wrong about it: the live database is `/data/webui.db`, not `/app/backend/data/webui.db`, which exists at zero bytes and is a decoy, and its `config` table is key/value rather than the single JSON `data` blob the older notes describe. ## What is not proven The model's answer on the box. The turn was composed with the skill attached and sent, `/api/chat/completions` returned 200, and the surface then rendered "You're out of credits." The account holds no credit, so no completion was produced. Nothing on the skills path failed. The injection half is not unproven in general: #1388 proved it against the identical tree, one chat and one model, the same question twice, where only the turn carrying the skill ended in a token that exists nowhere but the skill body. What stays unproven is that hop on this box with a funded account, and it is recorded as unproven rather than asserted. ## Tests - `scripts/test-owui-hive-frontend.sh`: 258 passing, up from 252. The same files run in place at image build time via `npm run test:frontend`, so a failure here fails the image build. - `apps/web-console` unit: 31 passing in `chat-coverage-lib.test.ts`. - The live sweep above, which is the assurance the floor alone cannot give. - Every new test verified red before the implementation, and the two source pins verified able to fail by reverting the wiring rather than assumed to be able to. - `node tools/lint-no-token-in-proof-captures.mjs`: ok, 223 files scanned. ## Buglog entry ```json {"id":"skills-surface-outside-coverage-denominator","date":"2026-08-29","error_message":"The /skills surface shipped by PR #1388 was swept by no chat-coverage surface, so a frontend regression, a new proxy 404 rule, or user.permissions.workspace.skills reverting to false would each have emptied it with every gate still reporting green.","root_cause":"surfaces.ts enumerates workspace tabs via a[href^=\"/workspace/\"] and lists every other surface by hand. A Hive route at /skills matches neither, so a newly shipped destination joins the sweep only if someone remembers to add it, and nothing fails when they do not.","fix":"Added a swept `skills` surface to STATIC_SURFACES and a dataDriven presence bar of 1 in surface-floors.json, with unit cover asserting the surface is swept, floored at a presence bar, and that an empty sweep fails the gate. Also rewrote the instance-wide id collision message, which named a field the author never filled in and pointed at a skill they cannot read.","tags":["chat-coverage","owui-fork","skills","silent-absence","observability"]} ``` Follows PR #1388. Blocked on PR #1437. Related: #1396, #1397, #1454, #1459. Plan and verification record: `hive/plan-2026-08-29-user-created-skills-verification.md` in the vault. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
sakibsadmanshajib
added a commit
that referenced
this pull request
Aug 29, 2026
…and-written list The list in this file named the ten routers apply_router_authz_family_patch.py rewrote when it was written. Main has since added skills.py to that family (PR #1388), the patch reads every file in its own EXPECTED_MARKERS, and it raises FileNotFoundError on a missing one, so the check went red on a merge that had nothing to do with chat deletion. Copying every router out of the vendored directory removes the second copy of that list, so there is nothing left to keep in step. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WyEwUxZCArdn1ZUDkTvuQ1
sakibsadmanshajib
added a commit
that referenced
this pull request
Aug 29, 2026
…claim (issues #916, #848) (#1462) Closes #916. Addresses the standing half of #848. ## The premise turned out to be wrong, and the wrong premise was the bug `docs/live-test-auth.md` has said, since it was written, that on the demo account every chat, task and key is "today undeletable: there is no chat-delete, task-delete or account-delete route wired up yet". That is false for chats, and the false half is load bearing. It is the reason the litter sat there: an agent reading that line concluded there was nothing to be done, so nobody tried. Documentation that is wrong is worse than documentation that is missing, because it is believed. | Surface | Delete route | Reality | | --- | --- | --- | | Chats | **Exists, and always did** | `DELETE /api/v1/chats/{id}` is live, reachable from the sidebar row menu behind a confirm dialog, hard deletes, and resolves through the user-scoped lookup for every caller on this deployment. Three conditions attach; see below | | Agent tasks | **Genuinely absent** | create, list, get, cancel, events, files, and no DELETE (`apps/control-plane/internal/agenttask/http.go:51-111`) | | Accounts | **Genuinely absent, on purpose** | `tenant_billing_accounts.account_id` references `accounts(id)` `ON DELETE RESTRICT` so deleting an account still funding a live tenant fails loudly (#828) | ## So this pull request builds no delete route Building a second one would be duplicated work. What was actually missing is enforcement, and that is what this ships. **1. The demo-account rule is now enforced rather than merely written down, on both sides of the repository.** `mintSession` in `live-auth.mjs` is the single door every JavaScript live session already passes through. It now refuses `demo@hive-demo.invalid` unless the caller declares the run read only (`readOnly: true`, or `--read-only` on the CLI). It was not the single door for the repository, and review was right to say so. Three Python scripts reach a deployed environment without importing it. `verify-control-plane.py` carried the rule only as a sentence in its docstring, with no check at all. `post-deploy-verify.py` did check, with an exact, case-sensitive `==`, so `Demo@hive-demo.invalid` or a trailing space walked past it. `verify-rag-roundtrip.py` was safe by accident of a hardcoded literal. All three now share one normalised implementation, `scripts/shared_demo_account.py`, matching the JavaScript guard's trim-and-lowercase. Be honest about what that is: a declaration gate, not a write blocker. It cannot stop a run that has declared itself read only from then sending a message. What it removes is the silent default, so pointing a suite at that account becomes a deliberate act visible in a diff. A real write blocker needs a per-request proxy, which is a great deal of machinery for a rule one line can state. The code comment says exactly this rather than overselling it. No existing caller breaks: nothing in `apps/` or `.github/` mints for that address today. The module's own usage example did name it, and is updated. **2. A structural guard on the authorization boundary, asserted against the source the image actually runs.** The first version of `scripts/test_owui_chat_delete_authz.py` read `vendor/open-webui/backend/...`. Review pointed out that this source does not ship: `Dockerfile.open-webui` builds only the frontend from `vendor/open-webui` and keeps the pinned upstream image for the Python backend, then rewrites `routers/chats.py` inside that image at build time. A guard reading the pre-patch copy is an upstream-bump detector wearing an authorization-guard docstring, and it is blind to exactly the risk it names. It now runs the real patch scripts, in the Dockerfile's order, against copies of the vendored source, and asserts on the patched result. That is the harness `scripts/test_owui_knowledge_authz.py` already uses. Doing so immediately surfaced something the old check could not see, and it sharpens the claim in the title. The patch narrows the admin arm to `if user.role == 'admin' and ENABLE_ADMIN_CHAT_ACCESS:` (#1186), and `docker-compose.yml` sets that variable to `"false"`. So on this deployment nobody reaches the unscoped path and every caller, administrator included, deletes through `get_chat_by_id_and_user_id`. Both halves are now pinned, because both are load bearing: upstream's unguarded admin arm would matter here, since every tenant OWNER holds an administrator session (#748, #948). What else the guard pins, all of it new since review: * The **order** in the non-admin arm, not just the call names: permission gate, then scoped lookup, then the 404, then the delete. A bump that keeps both scoped names and drops the `if not chat: raise 404` between them passed every check in the first version, and it is not a theoretical shape: three of that model function's writes are keyed on `chat_id` alone, so the mutant lets a non-owner destroy another user's `chat_message` rows, `shared_chat` snapshot and `automation_run.chat_id` while the victim's `Chat` row survives. * The `chat.delete` **permission gate**. Deletion for an ordinary user is configuration dependent, and an admin toggle silently returns the product to the state the old sentence described. * The **whole write set** of `delete_chat_by_id_and_user_id`, exactly. It is user scoped in its name and in one of its writes; the guard records that coupling rather than implying a scoping that is not there, and goes red if a further write appears or an existing one loses its key. * The Caddy check now **evaluates matchers against a concrete request** (`DELETE /api/v1/chats/<uuid>`) rather than scanning block structure. The line scanner it replaced had four demonstrated false negatives, and the worst was the most likely regression path: `@blocked` is a bare `path_regexp` with no `method` line at all, so a `chats` arm added there would 404 the delete route for every verb and the old check would stay green. **3. The documents are corrected, per surface**, and `docs/chat-abandoned-completions.md` answers #916. ## Cross-account denial, live Two independent run-key-scoped fixture identities on the shared chat instance. Never the demo account. ``` [A create] status=200 chatId=e85bb8ac-32f1-4bcb-a5af-2c56060ce571 [B GET A's chat] status=401 [B DELETE A's chat] status=404 [A GET after B's delete attempt] status=200 survived=true ``` B is refused on both verbs; A's row is intact. A non-owner's id simply never matches a row, so the refusal is structural rather than a check that could be forgotten. ## Soft or hard, and why Deletion is **hard**, and that is the right posture here. An `archived` column exists and is a separate, deliberate action, so the product already offers the soft path under its own name. Given buyers in finance, legal, healthcare and government, both concerns are live at once: retention obligations and a right to removal. Conflating them behind one button serves neither. A soft delete masquerading as a delete would be the worst of both, telling a regulated customer their data was removed when a row still holds it. ## What deletion does not remove, deliberately - **No credit ledger row.** The ledger is append only and lives in Postgres. Chat rows live in Open WebUI's own store. The delete path references the ledger not at all, so a deletion cannot alter what was billed. - **No audit event.** Same separation. An event recording that something existed survives the thing's deletion, which is the point of an audit trail. - The path touches `chat`, `chat_message`, `shared_chat`, and nulls `automation_run.chat_id`. Nothing else. ## Scope calls **Task deletion is out, and should be separate.** An agent task is not a chat: it carries spend, sandbox artefacts and audit events, so "delete a task" is a retention decision before it is an endpoint. **Account deletion is out, and should stay out for now.** The `ON DELETE RESTRICT` is not an oversight to route around; the migration comments it as deliberate, and #828 already tracks it. ## Proposed demo-account cleanup, for the owner to decide **Nothing has been deleted.** This is a proposal only, per instruction: that is the owner's demonstration data and it is not being cleared on an inference. `demo@hive-demo.invalid` held **24 conversations** at the time of measurement, of which **21 had no answer at all**. Every one of the 24 is automation text; none reads as genuine demo content. Earliest 2026-08-23, latest 2026-08-29. **Five were created on 2026-08-29 itself**, so this is still accumulating today, not a historical residue. Representative titles: ``` Reply with exactly: OK Reply with exactly: FRONTIER OK Reply with exactly: QAOK Count from one to twelve in words. (x4) What is 2+2? Say OK hello (x3) QAPROBE-A it's a "test" - ok Using only the attached file, state the QA inventory code. Print the word QACOWORK and stop. New Chat ``` Recommendation: delete all 24. There is nothing among them a prospect should see. Say the word and it is one pass through the route this pull request documents. Worth noting the litter is not confined to that account: `qa-tester@hive.test` holds 39, `e2e-verified+qafunded-...` 21, `e2e-verified@scubed.com.bd` 20. Those are test identities and nobody demos them, so they are lower priority, but they are the same cause, and closing them is #1476 rather than this branch. ## Review dispositions Fourteen threads, all worked. Twelve fixed, one partially fixed with the remainder filed, one measurement redone after the reviewer turned out to be more right than they claimed. | Thread | Disposition | | --- | --- | | Guard reads source that does not ship | Fixed. Runs the real patch scripts and asserts on patched output. | | Checks pin call names, not the refusal | Fixed. The order of permission gate, lookup, 404 and delete is asserted, with a self-check that rejects each mutant shape. | | Caddy scanner has four false negatives | Fixed. Matchers are evaluated against a concrete request; all four shapes are negative controls in the self-check. | | Scoped model function is scoped in one of four writes | Fixed. The whole write set is pinned and the caller coupling is recorded explicitly rather than implied away. | | The 404 is not pinned | Fixed by the same change as the order assertion above. | | No test covers the `mintSession` wiring | Fixed. Four tests drive `mintSession` and `writeStorageState` and assert no socket is opened. Deleting the guard call turns three of them red and no helper test. | | Caddy parser is order dependent | Fixed by the same change as the matcher evaluator above. | | `stop_item_tasks` runs before authorization | Fixed in the document; the code fix is #1474. | | A denylist of one address leaves three accounts open | Partial. The two Python implementations are deduplicated and normalised here; the allowlist is #1476, because it would turn two scheduled workflows red before they have run-key identities. | | "Single door" is false for the repository | Fixed. `scripts/shared_demo_account.py`, imported by all three Python scripts, with `scripts/test_shared_demo_account.py` asserting each one actually calls it rather than describing it. | | "Deletable, and always were" omits the permission | Fixed in the document, and the permission key is pinned in the guard. | | "Two charges rather than one" is unmeasured | Fixed. The claim is removed; the measurement is #1475. | | `chat_message` count has no positive control | Correct, and worse than reported. See below. | | "Scoped to the calling user" is untrue of the admin arm | Fixed, with a correction: on this deployment the admin arm is unreachable, because `ENABLE_ADMIN_CHAT_ACCESS` is `"false"`. That is now pinned rather than relied on. | ### On the `chat_message` thread The reviewer was right, and running the control they asked for proves it went further than they said. The kept conversation from the original capture, `63f4ff43-ae8c-4260-9784-c119a0e69233`, holds **zero** `chat_message` rows, while 138 of the 139 chats in that store hold two each. The original capture created its conversations through `POST /api/v1/chats/new` with a synthetic body, which writes no `chat_message` row, so "deleted chat_message rows: 0" was equally consistent with "no such row ever existed", and the two documents that escalated it into "the `chat_message` rows are gone" were resting on nothing. The capture has been redone with conversations created through the composer, which is the path that writes those rows, and both counts are printed with the kept conversation as the control. ## Testing - `make test-scripts` passes with both new scripts registered. - `scripts/test_owui_chat_delete_authz.py` was mutation tested end to end against the real files. Nine mutants, nine red, baseline and restore green: dropping the 404, dropping the permission gate, swapping the scoped calls for unscoped ones, widening the patch's admin gate from `and` to `or`, dropping `user_id` from the Chat delete, adding a `chats` arm to `@blocked`, a path-then-method mutation block, an enclosing-`handle` DELETE block, and flipping `ENABLE_ADMIN_CHAT_ACCESS` back on. - The JavaScript choke point was mutation tested the same way: deleting `assertNotSharedDemoAccount(email, { readOnly })` from `mintSession` takes the suite from 12 passed to 3 failed, and the three are the new wiring tests. Restoring it returns 12 passed. - Web console type check and build pass in the documented Docker substrate (`docker compose run --build --no-deps web-console sh -c "npm run build && npm run test:unit"`), which is the check that was failing. Precisely, since this pull request is about claims that are nearly true: the unit suite reported 1033 passed and 11 failed on that local run. Nine are `Test timed out in 5000ms` React render timeouts under local load and two are the documented `ENOENT /app/.github/workflows/ci.yml`, which only pass in CI because the web-console image does not copy `.github`. None of the nine failing files imports anything this branch changes, and `live-auth-demo-guard.test.ts` passes 12 of 12. CI is the arbiter for those. - One defect in this branch was found by CI rather than by me, after review: `test_owui_chat_delete_authz.py` hardcoded the ten routers the family patch rewrote when it was written, and main has since added `skills.py` (PR #1388), so the check went red on a merge unrelated to chat deletion. Fixed at the root by copying every router out of the vendored directory, which removes the second copy of that list entirely. No test in this change reports a state it does not have. Every guard added here was run against a mutant that breaks the behaviour it names, and the mutants are listed above rather than asserted. The one check in the previous revision that could not go red, the `chat_message` count, is named in the thread above rather than quietly re-run. ## Files touched in the vendored front end None. `vendor/open-webui` is read only in this change. ## Buglog entry ```json {"id":"bug-2026-08-29-chat-delete-doc-false","date":"2026-08-29","title":"Repository documentation claimed chat deletion did not exist, so demo-account litter was never cleaned up, and the first guard written for it asserted on source that does not ship","error_message":"docs/live-test-auth.md: 'today undeletable: there is no chat-delete, task-delete or account-delete route wired up yet (issues #828, #848)'","root_cause":"The claim was true for agent tasks and accounts and false for chats. DELETE /api/v1/chats/{id} has always existed, hard deletes, and resolves through the user-scoped lookup. The false third of the sentence was never checked against the running stack and became received wisdom: agents reading it concluded the 24 automation-litter conversations on demo@hive-demo.invalid could not be removed, so none tried. Compounding it, the rule the same document states, that a write-capable suite must never authenticate as the demo account, was enforced by nothing in JavaScript, by a case-sensitive exact string in one Python script and by a docstring sentence in another, so runs kept adding rows. The first structural guard written for the delete boundary read vendor/open-webui/backend/routers/chats.py, which Dockerfile.open-webui does not ship: only the frontend is built from vendor, the Python backend comes from the pinned upstream image, and owui-patches rewrites chats.py inside it, so the guard could not see the patch layer it existed to protect.","fix":"Corrected the document per surface after measuring live, including the chat.delete permission condition, the ENABLE_ADMIN_CHAT_ACCESS gate that keeps the admin arm unreachable on this deployment, and the pre-authorisation stop_item_tasks call. Added assertNotSharedDemoAccount to mintSession in apps/web-console/tests/e2e/support/live-auth.mjs and scripts/shared_demo_account.py to verify-control-plane.py, post-deploy-verify.py and verify-rag-roundtrip.py, with tests that assert the wiring rather than the helper. Rewrote scripts/test_owui_chat_delete_authz.py to run the real owui-patches against copies of the vendored source and assert on the patched result, to pin the ownership decision's ORDER rather than the call names, to pin the full write set of delete_chat_by_id_and_user_id, and to evaluate Caddy matchers against a concrete DELETE request instead of scanning block structure. Both registered in make test-scripts and mutation tested.","tags":["docs","authorization","open-webui","e2e","demo-readiness","chat","issue-916","issue-848"]} ``` --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
The owner asked for the ability to add new skills. This ships a user-created skills library in the chat shell, at
/skills, with its own navigation row.Everything behind a skill already worked and was never removed:
backend/open_webui/models/skills.pyand its migration define theskilltable.backend/open_webui/routers/skills.pyis mounted at/api/v1/skillsinmain.py:757.backend/open_webui/utils/middleware.py:2509-2556resolves a selected or mentioned skill, access checks it, and appends its content to the request as a system message shaped<skill name="...">content</skill>.$mention picker, the selection modal,skill_idson the outgoing request, and the store that lists them.Two things were missing. A route that renders the editor, and the permission that lets an ordinary customer reach the create endpoint.
Which layer this had to change, established before writing anything
Three layers looked like candidates and only one ships.
deploy/docker/owui-patches/run against the pinned upstream image's prebuilt bundle, andDockerfile.open-webuithen doesRUN rm -rf /app/buildfollowed byCOPY --from=frontend .../build /app/build. That bundle is discarded. The Dockerfile says so itself: those rewrites survive only as a digest drift guard.deploy/docker/Caddyfile.owui404s/workspace/skillsat the proxy, so nothing served under that path is reachable regardless of the bundle.vendor/open-webui/src/is what the frontend stage compiles and what the browser runs.So the change is in layer 3, and layer 2 is why it is not a restored Workspace tab.
Why
/skillsand not a restored/workspace/skillsIssue #783 removed the Workspace Skills tab and the proxy still 404s its path. Restoring it would mean reversing a proxy rule to land on a path the target navigation deletes anyway.
/skillsis unclaimed by upstream and matched by neither Caddy block, so no proxy change is needed, and the surface sits outsideroutes/(app)/workspace/+layout.svelte's permission guard for the same reason issue #1109 moved Knowledge to/knowledge.Skills belongs under a Customize destination in the target sidebar grammar. That container does not exist, so the row ships flat with a comment recording where it moves.
Why the permission is newly load bearing
Issue #783 removed the tab on two premises, both since falsified.
Premise one, that it was an empty surface with no Hive product behind it, is answered by the owner's ask. Premise two, that a permission-scoped hide hides nothing because every tenant OWNER is an Open WebUI admin (#748), was ended by
supabase/migrations/20260823_03_owui_role_never_admin.sqlandowui-patches/tenant_role_from_db.py, which promote only a platform admin. Every ordinary customer, tenant owners included, is Open WebUIuser, soworkspace.skillsnow gates a real audience and defaults to false upstream.Granting it takes more than an environment variable.
user.permissionsis a single persisted config row holding the whole permission tree, seeded on first boot byConfig.seed_defaults, and the database has outranked the environment ever since. That is the same traphive_rag_env_config.pyalready exists for, so the grant goes there. It could not ride the existing flat dictionaries: a config row keyeduser.permissions.workspace.skillswould persist, survive a restart, and be read by nothing, becausehas_permissionwalks the tree inside the one row. The module gains a permission seam that reads the tree, merges the leaves the environment names, and writes the tree back.Proved on a real database rather than argued: two boots against the same volume, the leaf moving
FalsetoTrueon the second, every sibling untouched, and no dotted row created. Transcript in the capture log.Security
A user-authored artefact that reaches a model prompt is an input-parsing path, so this was checked rather than assumed, and the review found something the first pass had rationalised away.
Fixed here.
filter_allowed_access_grantsstrips public grants and individual user grants from a non-admin write and never inspects group grants at all, so a hand-built payload can grant a skill to any group id on this shared instance, including another tenant's. A skill body reaches a prompt verbatim, so that is prompt injection across a tenant boundary rather than an over-broad share. The first version of this PR argued it was unreachable becauseGET /api/v1/groupsfilters to the caller's own memberships. True but not sufficient: that makes the defence secrecy of a UUID rather than an authorization check.owui-patches/apply_skill_group_grants_patch.pynow drops group grants naming a group the caller is not a member of, at all three skill write sites, and the image build asserts all four markers landed.Scoped to the skills router deliberately. The same shared function serves knowledge, models, prompts, tools, notes and folders, and changing it centrally would alter grant behaviour on surfaces this change never exercised.
Left as is. Sharing stays at upstream's defaults,
sharing.skillsandsharing.public_skillsboth false, so a skill is private to the account that wrote it.Filed, not fixed here, because neither is small and neither is introduced by this diff:
skill.nameandskill.idare unique instance wide while the editor slugifies the id from the name, so two tenants naming a skill the same thing collide with a 400. Pre-existing; this diff is what makes it reachable.The System Prompt question, folded in
Asked alongside the skills work: does the per-user System Prompt control render for an ordinary member, and does a value set there actually reach inference. Both measured live rather than read off the defaults, and the answer to both is yes, so no code changed for it.
Settings > General renders the textarea for a
role = useraccount;chat.controlsandchat.system_promptboth default true upstream and are true in this deployment's persisted tree. Setting it to a distinctive marker and asking a fresh question returned an answer beginning with that marker.Chat.svelte:2801-2806prepends the value to the outgoing message list unconditionally, which is a different shape from #1360 and #1265, where an artefact is mounted or catalogued and then never read. Frame 4 of the proof comment.Tests
Every new test was verified red first. The three that pass vacuously against today's code were each proved able to fail by a separate one-at-a-time mutation of the implementation:
test_reconcile_never_writes_a_flat_dotted_permission_row;test_reconcile_skips_the_write_when_the_tree_already_agrees;test_env_overrides_a_stale_persisted_model.The group-grant filter got the same treatment: allowing every group grant, and patching only one of the three write sites, each fail
scripts/test_owui_skill_group_grants.py, which also carries negative controls asserting the vendored router is still unpatched and still has the hole before the build runs.The Dockerfile's post-copy assertion block gains a positive check that the built bundle contains the skills route, so a frontend stage that silently built a tree without this change fails the image build instead of shipping green. Confirmed firing on a real build:
hive: shell present, removed surfaces absent.Review
Two streams, per the pipeline. Full text and the inline comments are on this PR.
rate_limit/ "Rate limit exceeded" repo wide,waitTime: 51 minutes,onDemandReviewAvailable: false. Nothing was reviewed by it.gemini-3.1-pro-high, effort high): RAN. Three findings. Two accepted and fixed in 4391c2e (the group-grant hole above, and a permission guard that sat on the index page where SvelteKit does not inherit it down to/skills/createand/skills/edit). One rejected with evidence: it claimed the reconcile tests call anasync defsynchronously and therefore cannot fail, which is not so, because this test file has a module-levelasyncio.runwrapper the pasted diff did not show.Recording one thing about that stream for the next person:
agyexecutes in~/.gemini/antigravity-cli/scratch, not in the checkout it is launched from, measured by asking it to printpwdandgit rev-parse --abbrev-ref HEAD. Its first pass reviewed an unrelated diff about credit formatting that exists nowhere on this branch, and was discarded as hallucinated. The findings above come from a second run with the diff pasted inline.Visual proof
Posted as a release-hosted comment on this PR. The behavioural frame is one chat, one model, the same question twice: the control turn answers in three sentences with no marker, the turn with the skill selected answers in one sentence ending
HIVE-SKILL-PROOF-OK-2026-08-29, a string that exists nowhere but the skill body.Substrate stated plainly, because it is not the demo box: the image built from this branch, run standalone against its own SQLite volume, signed in as an ordinary non-admin account, with inference through an OpenAI-compatible endpoint rather than the Hive gateway. Both Hive keys in this checkout are dead against
api-hive.scubed.co(Incorrect API key providedandAPI key is revoked) and minting one needs Supabase credentials this box does not have, solive-auth.mjscould not run either. The gateway hop is not what this change touches: injection happens inside Open WebUI before a provider is chosen. What this capture does not exercise, and what a post-merge capture on the demo box still should: single sign-on, the Caddy front end, and the gateway hop.docs/proof/user-created-skills-2026-08-29/log.mdcarries the full transcript, the permission-row before and after, and one incidental finding not fixed here: the vendor's "Help us translate" link is still in Settings, because the rewrite meant to remove it runs against the bundle the frontend stage discards.Relationship to the inert-artefact issues
Deliberately not the same defect family as #1360 (agent packs mounted but never read) or #1265 (marketplace skill, rule and prompt_template kinds inert). Those are the agent engine's sandbox packs, a different subsystem, and this change neither closes nor narrows either. What makes this one different is that the last hop already existed: the injection seam in the chat middleware is real and is exercised in the proof.
Deliberately not done
workspace-skills-tabbundle rewrite. It operates on the discarded pinned bundle and its remaining job, digest drift detection, is still correct./workspace/skillsstays 404.Decision record
Recorded as D-060 in
.wolf/decisions.md, a partial reversal of #783: Hive ships a user-created skills surface, and the stock Workspace tab stays removed. The id was minted on this branch;mainand the other live branches were checked and none claims it.Plan and acceptance criteria:
plan-2026-08-29-user-created-skillsin the project vault.Buglog entry
To be appended to
.wolf/buglog.jsonlonmainin a separate buglog-only pull request after this merges, per.claude/rules/openwolf.md.{"id":"owui-skills-guard-on-page-not-layout","date":"2026-08-29","error_message":"/skills/create and /skills/edit rendered the full skill editor for a session without workspace.skills; the save then failed with 401 from POST /api/v1/skills/create","root_cause":"the permission guard was written in routes/(app)/skills/+page.svelte. A SvelteKit +page.svelte applies to its exact route only and is not inherited by nested routes, unlike +layout.svelte, so the two editor routes had no guard at all","fix":"moved the guard into routes/(app)/skills/+layout.svelte, which covers the index and both editor routes; skills-surface.test.ts now reads the layout rather than the page","tags":["owui","svelte","authz","sveltekit-routing"],"pr":1388} {"id":"owui-workspace-components-need-workspace-chrome","date":"2026-08-29","error_message":"the skills index rendered underneath the sidebar with the New Skill button pinned to the window corner and the title overlapping the Hive wordmark","root_cause":"Skills.svelte and SkillEditor.svelte are upstream Workspace components authored for routes/(app)/workspace/+layout.svelte's container, which supplies both the md:max-w-[calc(100%-var(--sidebar-width))] constraint and the px-3 md:px-[18px] padding. Mounting them in a bare hv-panel-region flex region supplied neither","fix":"the /skills route layout reproduces that container minus the tab bar; a test pins all three class fragments so the chrome cannot silently go away again","tags":["owui","css","layout","fork"],"pr":1388} {"id":"owui-group-access-grants-never-filtered","date":"2026-08-29","error_message":"a non-admin could attach an access grant naming any group id on the shared instance, including another tenant's, and have it stored","root_cause":"utils/access_control.filter_allowed_access_grants strips public grants and individual user grants for a non-admin and never inspects group grants at all. For skills that is cross-tenant prompt injection, because a skill body is appended verbatim to the chat request as a system message. Unreachable in practice only because GET /api/v1/groups filters to the caller's own memberships, which is secrecy of a UUID rather than an authorization check","fix":"owui-patches/apply_skill_group_grants_patch.py drops group grants naming a group the caller is not a member of, at all three skill write sites; the image build asserts all four markers landed. The general case across the other routers is filed as #1396","tags":["owui","security","authz","multi-tenancy","prompt-injection"],"pr":1388} {"id":"agy-runs-outside-the-repo","date":"2026-08-29","error_message":"an Antigravity review returned detailed findings about a credit-formatting diff that does not exist on the branch under review","root_cause":"agy executes in ~/.gemini/antigravity-cli/scratch, not in the directory it is launched from. Measured by asking it to print pwd, git rev-parse --abbrev-ref HEAD and git diff --stat: the first returns the scratch path and both git commands fail with 'not a git repository'. Given no diff to read, it produced a plausible review of something else","fix":"paste the diff and the load-bearing surrounding code inline in the prompt rather than telling it to run git; discard any pass whose findings name files not in that paste","tags":["harness","review-streams","antigravity","false-green"],"pr":1388} {"id":"shared-scratchpad-filename-collision-crossed-prs","date":"2026-08-29","error_message":"six review threads on PR #1083, another agent's pull request, were resolved by an agent that had never read them","root_cause":"agents in one session share a scratchpad directory. A helper written to the plain name resolve-threads.sh was overwritten by another agent's script of the same name, which targeted pull request 1083, and running it acted on their PR. Same failure class as the known parallel-agent report-path collision, but the blast radius is a GitHub mutation rather than a lost file","fix":"unresolved all six immediately to restore the state found, left a note on #1083 saying what happened and that the restore was blind rather than a judgement, and prefixed every later script with the worktree id","tags":["harness","parallel-agents","scratchpad","github"],"pr":1388}