feat: unify the provider tables and resolve the chat provider on the server - #2540
Merged
Dani Akash (DaniAkash) merged 5 commits intoSep 3, 2026
Merged
Conversation
Both are providers for the chat, and everything above the database already said so: the chat target is one union with a kind on the client, and the wire target has always been a discriminated union. Only storage disagreed, and it charged for the mismatch. Selecting an acp agent left the default provider pointing at the previously selected llm one, because the write was conditional on kind. A scheduled job could reference an llm provider and nothing else, so picking Claude Code in chat was possible while scheduling against it was not. Migration 0010 creates the unified table and copies both sources in; 0011 repoints scheduled jobs and drops the old tables. Two migrations rather than one, so the copy proves itself before anything is dropped, and because drizzle cannot diff a simultaneous add and remove without a rename prompt. Drizzle put its drops ahead of its own foreign key pragma, which fired the ON DELETE SET NULL on scheduled jobs and silently unlinked every job from its provider. Reordered so the drops happen after the rebuild, inside the block where enforcement is off. The default provider now lives on a column, and a partial unique index admits exactly one row, of either kind. Not keyed by profile: sqlite treats nulls as distinct in a unique index, so pairing it with the unset profile column would let every row be default at once. Also brings the packaged-build schema fallback current. It had drifted four migrations behind, so a build without migration files would have created a database with none of this epic's tables. The test's copy of the migration history is now derived from the journal, since a hand written duplicate is what let the drift go unnoticed.
The provider block is gone from the chat body. A request names an id, or names nothing and gets whichever provider is selected, and the server fills in the model, endpoint and credentials from the row. Sixteen fields of provider configuration collapse into one, and the api key, the aws secret and the session token stop crossing the wire on every message. Every one of those fields is still accepted. The extension updates independently of the browser binary, so a shipped build can be running a client that sends the whole configuration inline; the server stops requiring them, not accepting them, and a row it does not recognise leaves whatever the client sent in place. The selection moved to the server with the tables it points at, so choosing a coding agent now records it. It could not before: the default lived in extension storage and only ever named an llm provider, so picking an agent left it pointing at the provider chosen before it. The scheduled runner drops its provider lookup entirely, and the guard that came with it. That guard existed because an unreachable list and an empty one looked alike, so a job could run on the built-in provider with the wrong credentials and still be recorded as a success. There is nothing to tell apart now: the job names an id and the server resolves it. Refine prompt still resolves on the client. It posts to its own endpoint with its own schema, and giving it the same treatment is separate work.
Contributor
✅ Tests passed: 1372/1375Ran 8 of 16 suites (8 not affected by this change).
|
Contributor
Greptile SummaryThe PR unifies LLM and ACP provider persistence, moves default-provider selection and chat-provider hydration to the server, and migrates scheduled jobs to the unified provider table. The latest authorization fix still permits unknown provider IDs to select OAuth-backed or BrowserOS providers whose credentials are subsequently supplied by the server.
Confidence Score: 3/5This PR is not safe to merge until every request that receives server-held OAuth or gateway credentials is required to pass the trusted-app check. DaniAkash stated that the stored-credential escalation was fixed, but an unknown provider ID with an inline OAuth-backed or BrowserOS provider remains a concrete counterexample: hydration marks it as caller-owned, the route skips authorization, and downstream resolution injects a server-held credential into the outbound model request. Files Needing Attention: packages/browseros-agent/apps/server/src/api/routes/chat.ts and packages/browseros-agent/apps/server/src/api/services/chat-provider-config.ts
|
| Filename | Overview |
|---|---|
| packages/browseros-agent/apps/server/src/api/routes/chat.ts | Adds server-side provider hydration and a provenance-based authorization check, but the check occurs before downstream server-backed credential resolution is accounted for. |
| packages/browseros-agent/apps/server/src/api/services/chat-provider-config.ts | Hydrates known provider rows and preserves legacy inline configurations, including unknown-ID requests that can later select server-backed authentication. |
| packages/browseros-agent/apps/server/src/lib/db/migrations/0010_add_unified_providers_table.sql | Creates and populates the unified provider table while preserving provider configuration and default state. |
| packages/browseros-agent/apps/server/src/lib/db/migrations/0011_drop_split_provider_tables.sql | Rebuilds scheduled-job references before dropping the split provider tables to preserve existing provider assignments. |
| packages/browseros-agent/apps/app/lib/messaging/server/buildChatRequestBody.ts | Replaces inline provider configuration with provider identity for new BrowserOS chat requests. |
Sequence Diagram
sequenceDiagram
participant U as Untrusted local caller
participant R as POST /chat
participant H as Provider hydration
participant C as LLM config resolver
participant L as External model
U->>R: Unknown provider ID + inline OAuth provider
R->>H: Hydrate request
H-->>R: "usedStoredProvider=false"
R->>R: Skip trusted-app check
R->>C: Resolve provider configuration
C->>C: Load stored OAuth/gateway credential
C->>L: Request using server-held credential
Prompt To Fix All With AI
### Issue 1
packages/browseros-agent/apps/server/src/api/routes/chat.ts:92-94
**Server credentials bypass trust check**
When an untrusted no-Origin caller supplies an unknown target ID with an inline OAuth-backed or BrowserOS provider type, hydration sets `usedStoredProvider` to false and this condition skips the trusted-app check. Downstream provider resolution then injects the user's stored OAuth token or the server's gateway credential into the outbound model request, allowing unauthorized usage and quota consumption.
**How this was verified:** The unknown-ID request was traced through the skipped provenance gate and server-side credential resolver to the outbound provider constructor.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (2): Last reviewed commit: "fix(server): gate chat on trust when the..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
A browseros chat request is deliberately allowed without the app-origin check, on the reasoning that it carries its own credentials and so can only spend what the caller already had. Resolving the provider from storage broke that reasoning: naming an id, or naming nothing and taking the selected provider, would have let any local caller spend the user's key against an external service. The check now applies exactly when the configuration came from a stored row. A request that brought its own is as unrestricted as it was before, so the capability that reasoning was about is untouched.
Contributor
Author
…ntial The previous gate keyed on whether a stored provider row was read, on the reasoning that a request naming no known row must have brought its own key. That is false for four provider types. The oauth three take a token from this machine's oauth store and browseros takes the gateway credential, so naming one with an unknown id skipped the check and had the server hand over a credential the caller never held. The predicate lives beside resolveLLMConfig, since it has to mirror those branches exactly and would drift if the route kept its own copy. This particular hole predates the change: the exemption for browseros requests and the credential injection behind it were both already there. It is fixed here because the gate added alongside it claims the ungated path is safe, and that claim has to hold. The chat integration test now sends the origin header the extension always sends. It was relying on the exemption this closes, and the routes the background alarm runner already calls carry that header today.
CI failed with `Export named 'SERVER_CREDENTIALED_PROVIDERS' not found` against a file that plainly exports it. A module factory is a total replacement, so everything it omits disappears for every file importing that module afterwards, and bun's registry is process wide. Adding an export to a module someone mocks partially is enough to break a different file entirely. The factory now spreads the real module and overrides only the function under test. This does not reproduce locally: file ordering is stable on APFS and not on ext4, which the test runner's own notes call out as the reason this class of failure kills CI while local runs pass.
Dani Akash (DaniAkash)
merged commit Sep 3, 2026
c3e5bba
into
epic/local-first-storage
13 checks passed
Dani Akash (DaniAkash)
added a commit
that referenced
this pull request
Sep 4, 2026
…yncing them (#2542) * chore(app): add react-query-kit for the local-first storage epic The epic moves provider and schedule config from extension storage onto the local server, which means a large set of new query and mutation hooks over Hono RPC. Kit factories are the repo standard for those, and the app is on plain react-query today, so the dependency lands before the hooks do. 3.3.4 peers on @tanstack/react-query ^4 || ^5; the app is on ^5.101.4. * feat(server): add local storage for llm providers and scheduled jobs First phase of moving provider and schedule config off the cloud and onto the machine. Server side only: nothing reads or writes these tables yet, so the extension is unaffected and this ships dark. Two tables beside the existing agents, conversations and oauth. Credentials live here in the clear, next to the oauth tokens already in this database, protected by filesystem permissions and nothing more. That is the same posture they had in extension storage, and it is why the cloud copy of a provider was never usable: it deliberately never carried a key. Both tables carry a nullable profile_id, always null for now. No extension API exposes a browser profile identifier, so every profile on a machine shares one database. The column exists so isolation can be turned on later without a second migration. Upserts key on the client-supplied id and leave created_at alone on conflict. The migration that follows re-runs per profile and after partial failures, so landing twice has to be indistinguishable from landing once. The job to provider reference sets null rather than cascading. A job whose provider was deleted should surface as needing attention rather than disappearing because of a delete made elsewhere. * feat(server): add the schema and migration for the new tables The previous commit shipped the routes, stores and tests but not the tables they depend on. The server .gitignore has a bare db/ rule, meant for a runtime database directory, which also matches src/lib/db/ and silently swallowed the new schema files and the migration. The existing schema files are tracked only because they were force-added the same way. * fix(server): anchor the db and identity ignore rules to the app root A bare db/ rule matches a directory of that name at any depth, so it covered src/lib/db/ and tests/lib/db/ as well as the runtime directory it was meant for. New schema files and migrations landed ignored, and the existing ones are tracked only because they were force-added. Anchoring both rules with a leading slash keeps the runtime directories ignored while leaving source and tests alone. Nothing on disk was missing from the repository, so this closes a trap rather than recovering anything. * feat(app): stop syncing to the cloud (#2519) * feat(app): stop syncing to the cloud and say so Every write path to the cloud is gone. Providers, scheduled jobs and chat turns now stay on the machine, and a one-time notice on the settings page tells the user what changed. The chat change is the significant one. A signed-in user's turns were uploaded by the client while a signed-out user's were persisted by the local server during /chat. Everyone takes the local path now, so history lands in SQLite regardless of session. The client keeps no write of its own, which also retires the durable turn buffer that existed only to survive an interrupted upload. Incognito still persists nowhere. The legacy conversation migration is kept but no longer branches on session: it always drains pre-upgrade local:conversations into the local server, which is the direction the rest of this work moves data. The sign-in promote is deleted outright rather than disabled. It uploaded the local server's history and then deleted the rows it had uploaded, so under this model it would move data off the machine and drop the local copy. Cloud reads are untouched. Cloud history still displays and a cloud conversation can still be opened, which the next phase turns into a union with the local list. The notice is past tense because sync stops in this same release, so a warning about the future would describe something that already happened. It answers the question people actually have, which is whether they are losing anything: providers, agents and schedules keep working, cloud chats stay visible in history for now. Two consequences of the ignore fix that shipped with the tables. Biome respects .gitignore, so src/lib/db was never linted and had drift, including in the schema files added last phase. The hand-written files are formatted here; the drizzle-generated migration metadata is excluded instead, since formatting it would fight the generator on every migration. * fix(app): do not drain a legacy conversation the server did not store The import route is insert-if-absent, so an id already on the server is answered with a success that wrote nothing. The client only checked the HTTP status, reported the conversation as handled, and the caller then deleted the legacy copy from extension storage. Where the existing server row was an older, shorter version of the same conversation, the messages it did not contain were gone. The route already reported this as `imported`; the client discarded it. A skipped import is now only treated as handled once the server row is confirmed to hold every message the legacy copy has, compared by message id rather than by count so a same-length but different row is not mistaken for the same content. Anything unconfirmed stays in storage for the next attempt. The bug predates this branch, but only logged-out users reached this path before. Routing everyone through it made a latent problem universal, so it belongs here rather than in a follow-up. * feat(app): show local and cloud history together (#2520) * feat(app): show local and cloud history together History was one or the other: signed in showed only the cloud list, signed out showed only the local server. A signed-in user could not see the conversations their own machine was storing, which is now where every new chat lands. The local list is always shown and comes first. Conversations still held in the account appear beneath it under their own heading, saying what they are and that they do not live on this device. Grouped rather than interleaved: the cloud is a shelf that empties when it is retired, blending it into the local list would hide that, and merging two cursor-paginated sources by date against one scroll position buys nothing here. Deduplicated by id, local winning. The same conversation id is used by extension storage, the local server and the cloud, so anything synced before sync was turned off exists in both lists. Two composition problems came out of rendering both at once. The list owned its own scroll container and rendered a <main>, which was fine while exactly one ever rendered and would have been two competing scroll areas and two landmarks side by side; the screen owns a single one now. The empty state also read "No conversations yet" directly above a populated cloud section, which is the common case immediately after this ships, so it names the store it is talking about instead. * fix(app): keep paging the cloud past a page that is entirely local Cloud pagination is driven by a sentinel rendered inside the list, and the list is not rendered while the section has nothing to show. A page whose conversations all exist locally deduplicates away to nothing, so the section returned null, the sentinel never mounted, and the cloud-only conversations behind that page could not be reached. Two guards were involved. The section returned early when nothing was visible, and the sentinel itself sits in the branch the list renders only when it has conversations, so removing the first guard alone would not have helped. The stalling page is the ordinary one immediately after this ships. Legacy conversations are drained into the local server while the same conversations are already in the account, and being the most recent they sort onto the first cloud page. The section now pulls the next page itself while it has nothing visible and pages remain, handing back to the sentinel as soon as something renders. It terminates when the pages run out, so a user whose whole account history is duplicated locally walks the pages once and is shown nothing, which is correct. The decision is a pure function so the conditions are testable without a renderer, including that a fetch is not stacked on one already in flight. * refactor(app): drop the legacy conversation drain Conversations are already written straight to SQLite: the server persists each completed turn during /chat. The drain was a separate path that read pre-upgrade local:conversations from extension storage and posted them back to the server, which is a hop the data does not need. It also ran only for logged-out users before this epic, and widening it to everyone was not asked for. That widening was the sole way a conversation could end up in both the account and SQLite, which is what the deduplication in the history union exists to handle. Nothing writes local:conversations any more, so the leftovers stay in extension storage untouched rather than being deleted. Conversation history is the data we accept losing when the cloud is retired, so paying to move it was the wrong trade. Removes the migration module, its helpers and tests, the client import helper, and the legacy storage definition, along with a dead serial runner left behind when the sign-in promote was removed. Deduplication and the paging that goes past a fully deduplicated page stay. Overlap is now only possible from a promote that uploaded to the account and then failed to delete the local rows, which could easily cover a whole page. * fix(app): stop cloud history auto-advance after a failed page A rejected fetchNextPage leaves hasNextPage true, because it is derived from the last successful page, while the in-flight flag clears. Every input to the advance guard returned to its pre-fetch value, so the section restarted the fetch with no user interaction and a persistently failing request retried forever. Guard on isFetchNextPageError so a failed page settles instead. * feat: migrate providers and scheduled jobs into local storage (#2523) * feat(server): add insert-if-absent import for providers and jobs The import must fill gaps without replacing. The app writes to these tables directly, so an upsert would let a second run restore a stale copy over a row the user edited since. onConflictDoNothing gives the absent-or-present decision in one statement. Also guards /llm-providers and /scheduled-jobs with the app-origin check the other protected routes already use. The blanket trusted-origin middleware only rejects a request carrying a disallowed Origin, so one with no Origin passed straight through to rows holding API keys. * feat(app): migrate providers and scheduled jobs into the server once Reads extension storage and the browseros.providers pref backup, unions them with storage winning, and posts both to the import endpoints. The pref backup covers the reinstall case where extension storage was cleared but the per-profile pref outlived it. The cloud is not a source. Its scheduled jobs include every job deleted since the deletion queue lost its only reader, so importing them would bring deleted jobs back. Its providers never carried credentials and already surface through the incomplete-provider prompt in AI settings. A done marker in per-profile storage stops it repeating. The marker is set only after both imports land, so a failure retries on next startup, which is safe because the server inserts only what is absent. * fix(app): drop unimportable entries instead of failing the batch The import is one request, so a single entry the server rejects returned 400 for every provider in it, blocked the scheduled jobs behind it, and left the done marker unset. That would repeat on every startup, because the pref backup it came from has no migration path and the user cannot edit it. Providers and jobs are now checked against exactly what the server requires, and optional fields holding the wrong type are dropped so the server default applies rather than the batch failing. Filtering runs before the merge so an unusable stored entry cannot win the id and take a good backup copy with it. Removed provider types are excluded too. Storage migrations drop them, the pref backup never gets that treatment, so a stale one could import a provider of a type the app no longer supports. * feat: read and write llm providers through the server (#2537) * feat(app): read and write llm providers through the server useLlmProviders keeps its exact interface so both consumers, AI settings and chat target selection, are untouched. Underneath it is now a react-query-kit query over Hono RPC instead of extension storage. It gains an unavailable state. Previously an empty list meant the user had no providers, and the hook seeded the built-in one in response. Over HTTP a failed load looks the same as an empty one, so seeding moved into the fetcher where it can only run on a confirmed empty response, and AI settings now says the list could not be loaded rather than showing none. Saving a single-instance provider used to collapse earlier copies as a side effect of writing the whole list. That is now an explicit plan of one PUT and the deletes it displaces. The default provider id stays in extension storage. It is a per-profile preference and every profile shares one database, so a column would make them share a default too. A stale id costs nothing because it is resolved on read. Logout no longer deletes providers or scheduled jobs. That was right while they were account data synced to the cloud; they are now local data the account does not back. * fix(app): do not substitute a provider the caller did not choose An unreachable provider list returned an empty array, so a scheduled job that named a provider found no match and fell through to the built-in one. It ran on a different model with different credentials and was recorded as completed. The list being unreachable says nothing about whether that provider exists, so the two cases are now distinguished and naming a provider that cannot be loaded fails the run instead. A provider that was genuinely deleted still falls back, as before. Deleting a provider also persisted the replacement default before attempting the delete, so a failed delete left the provider configured but no longer default with nothing to show for it. The delete goes first; a default id left pointing at a deleted provider is repaired on read. * fix(app): never resolve a provider from a list that failed to load The previous guard only covered a job that named a provider, which left the same hole one step over. A job that names none still has a choice behind it: the configured default, whose id lives in extension storage but whose model and credentials live in the list. So an unreachable list sent those runs to the built-in provider and recorded them completed, which is the case this guard existed to prevent. The condition drops to the list itself, which also states the invariant plainly. An empty list keeps the fallback, because that is the server answering that it genuinely has no providers rather than not answering. * feat: move scheduled jobs and run history to the server (#2538) * feat(server): add local storage for scheduled job runs Job definitions had a table; their run history did not, so it was the one part of the domain with nowhere to live on this side. Runs cascade on job delete, unlike the job to provider reference which is set null. A job whose provider was removed is a job needing attention, whereas a run whose job was removed means nothing, and deleting a job already removed its runs before this table existed. The tool call log is a json column. Its input field is optional here where the extension has it required: an unknown already admits undefined, so the two describe the same values, and matching the validator avoids asserting the difference away at the route boundary. * feat(server): carry the per-job run cap across with the runs The extension kept fifteen runs per job, trimming as it created each one. Now that it no longer owns the history that policy has to live here, or the table grows without bound. It applies on every write rather than only on creation, which is bounded and idempotent, so it holds however the run was written. The import path does not prune, staying purely additive; the next real run trims. * feat(app): read and write scheduled jobs and runs through the server The hooks keep their shape, so the tasks page, the results view, the card and the new tab panel are unchanged apart from where they import from. Both gain an unavailable state, since an empty list and an unreachable server are now the same shape without one. The alarm runner distinguishes them everywhere it reads. Treating a failed load as an empty list would read as nothing being scheduled: alarms would not be rebuilt on startup and schedules would quietly stop firing, with no failed run to show for it. It skips the pass instead and retries on the next startup. Extension storage no longer carries the data, but it still carries the change signal. Runs are written by the background while the side panel and new tab display them, and storage watch is what kept those in step. A revision item is bumped after a write so every mounted view refetches. Run history is imported once, under its own marker. It cannot share the provider and job marker because that import must never run twice: extension storage is frozen now, so a second pass would insert back whatever the user has since deleted. Also removes the scheduled job deletion queue, whose only reader went when sync did, and the mount-time storage read that chose the opening tab, which is now derived so it settles when the history arrives. * fix(app): record a finished run against the current job Recording that a run finished wrote back the job as it was read before the run started. A run can take minutes and the job stays editable throughout, so a rename, a schedule change, a disable or a different provider chosen while it was going would be silently reverted. The old code merged into a freshly read list; passing the job object instead was an attempt to save a read and is what lost the update. It takes an id again, so a stale snapshot cannot be handed to it, and it skips the write when the job was deleted mid-run rather than resurrecting it. * chore: sync the local-first storage epic with main (#2539) * fix(server): steer ACP agents to browseros, not a co-installed browseros-neo (#2517) * fix(server): steer ACP agents to browseros, not a co-installed browseros-neo * refactor(server): replace ACP skill file with system prompt + workspace CLAUDE.md/AGENTS.md * refactor(server): slim the agent system prompt and move tool guidance into the tools (#2521) * refactor(server): slim the agent system prompt, move tool guidance into tools (TKT-947) * docs: tidy prompt comments * fix(browser-mcp): fence run structured output so untrusted values reach the model marked * test(server): expect fenced run structured output in browser + dual-era tests * feat(app): give first-run its own setup step instead of the settings page (#2511) * feat(app): give first-run its own setup step instead of the settings page Finishing the native onboarding dropped the user on the full AI settings screen: sidebar, configured list, promos, default-target control, usage and billing links. That is an administrative surface, and it was someone's first minute with the product. Adds #/onboarding/ai, a bare route beside features and outside every layout, carrying the provider catalogue and nothing else. Connecting anything hands off to #/home, which is the new tab page, so the first thing after setup is the thing the product is for. The handoff fires on the transition to connected, never on the state. A subscription template takes the user off the page and back, so success arrives as a change to the provider list rather than from a submit handler, and a user who opens the route with providers already configured has to stay on it rather than being bounced. Connected cannot mean a non-empty provider list: a built-in entry is seeded on first load, so it means any provider that is not that one, or any agent. The dialog and OAuth wiring moves out of BrowserOsAiPane into a shared hook, since the catalogue only raises intent and something has to own the four dialogs. The settings page behaves exactly as before. Includes a skip, because both onboarding exits still land here and the page has no sidebar to escape through. * fix(app): hand off when an already-configured user connects something The handoff compared a boolean: not-connected becoming connected. For anyone who already had a provider or an agent that boolean was true on arrival and stayed true, so adding another connected nothing and the page never moved. Only a profile with nothing configured could ever reach the new tab page. It now compares a count against a baseline taken when both lists settle, so what matters is whether the user connected something on this visit rather than whether they had ever connected anything. Deleting does not count: the count has to grow. Readiness now waits on the agent list too, via its flag rather than . The two lists load on separate async chains, and that hook documents that reads false for a render while the list is still empty, so a baseline taken on the providers alone could miss existing agents and fire the moment they arrived. The previous behaviour was covered by a test asserting a user who arrives already connected is not handed off. That test encoded the bug, so it is replaced by two that cover adding to an existing setup. * refactor(app): hand off from onboarding on the add callback, make added provider default * fix(app): set the added provider or agent as the active chat target before handoff * fix(app): hand off with the persisted provider id so an OAuth reconnect resolves * chore: browseros-claw update * chore: bump version * chore: bump app onboarding version to 0.0.1 (#2524) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * chore(release): snapshot browseros server alpha v0.0.152 Automated release snapshot update. * chore: bump server version to 0.0.152 (#2526) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * chore(release): update extension alpha feeds to 0.0.146.0 Automated release snapshot update. * chore: bump agent extension version to 0.0.146.0 (#2528) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * chore: sync internal-docs submodule (#2529) Co-authored-by: browseros-bot <bot@browseros.ai> * feat: unify product installation identity (#2530) * feat(chromium): unify product installation metrics * feat(agent): share BrowserOS installation identity * feat(claw): separate installation identity from consent * fix(dev): share product state root with Chromium * test(agent): cover canonical installation identity * fix(agent): clean failed identity publishes * style(claw): satisfy analytics lint * feat: agents never steal focus in BrowserClaw (#2531) * feat(claw-mcp): tabs new and pages.newPage always open in the background Agents can no longer request a foreground tab. The background field is still accepted and ignored so clients holding the old schema keep working, but it is hidden from the tool schema. Conformance cases stop assuming an agent-opened page becomes the active tab. * feat(patches): automation never steals focus pref and Browser gates Adds browseros.automation_never_steals_focus (default on for BrowserClaw). With it on, a tab with a DevTools client attached cannot switch the user's active tab or raise the window through Browser::ActivateContents, and tabs or popups its pages open after an agent click land in the background (Browser::AddNewContents, mirroring the upstream actor gate). * feat(patches): Browser.createTab defaults to background; activate commands honour the focus pref createTab now opens tabs in the background unless background=false is passed, and an explicit false only selects the tab within its window. Under browseros.automation_never_steals_focus, activateTab stops raising the window, activateWindow becomes a no-op, and createWindow plus setWindowVisibility(activate) show windows inactive. * test(claw-mcp): retired tabs background field stays accepted but inert * chore: bump version * ci: grant nightly call sites the permissions their workflows declare (#2532) * ci: grant nightly call sites the permissions their workflows declare The nightly family workflow had never started: a called workflow can only narrow the caller's GITHUB_TOKEN, so any call site whose ceiling is below what the called workflow declares fails the entire run at validation time, before a single job is created (run 33689075661, startup_failure). Six call sites were short: - prepare/finalize-claw-server granted contents: write, but release-claw-server.yml declares publish-ota and reflect-version with pull-requests: write. Both are skipped here (publish_ota: false, state_owner: suite) but validation is static and runs before if:. - build-browseros/build-browserclaw had no permissions block, so they inherited the workflow-level permissions: {} and granted nothing to nightly-macos-product.yml, which declares contents: read. - server-ota/claw-server-ota granted contents: read to publish-server-ota.yml, which declares contents + pull-requests write to publish the feed snapshot and its reconciliation pull request. Ceilings now match what release-browseros.yml and release-browserclaw.yml already use for the same called workflows. * ci: keep the nightly ceilings minimal Narrows the previous commit to the only call sites that actually elevate. Only job-level permissions inside a called workflow are validated against the caller's ceiling; a callee's workflow-level block is a default for standalone runs and is supplied by the caller when it is invoked through workflow_call. release-claw-server.yml is the only callee here that declares job-level permissions (publish-ota and reflect-version, both pull-requests: write), so it is the only ceiling that had to widen. Reverted as unnecessary: - build-browseros/build-browserclaw: nightly-macos-product.yml declares no job-level permissions and never checks out or uses the token. - server-ota/claw-server-ota: publish-server-ota.yml declares none either, and in suite mode the writes belong to reconcile-state, so contents: read is the correct least-privilege ceiling. ci_workflow_test asserts it. * ci: cover the permissions the nightly's build and OTA callees declare (#2533) The nightly still failed validation after the claw-server fix. Bisecting with push-triggered copies of the workflow on a scratch branch localised two more call sites; each was proven in isolation: - build-browseros/build-browserclaw had no permissions block, so they inherited the workflow-level permissions: {} and granted nothing to nightly-macos-product.yml, which declares contents: read. - server-ota/claw-server-ota granted contents: read to publish-server-ota.yml, which declares contents and pull-requests write. A called workflow can only narrow the caller's GITHUB_TOKEN, and that is checked statically for the whole nested tree before any job is created, so a short ceiling rejects the entire run. With both covered, a full copy of the workflow created all 17 jobs and stopped at the intended 'must run from refs/heads/main' guard. ci_workflow_test asserted the contents: read ceiling that caused this, so it encoded the bug; updated to the ceiling that actually validates. * chore: sync internal-docs submodule (#2535) Co-authored-by: browseros-bot <bot@browseros.ai> --------- Co-authored-by: Nikhil Sonti <nikhilsv92@gmail.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: browseros-bot <bot@browseros.ai> * feat: unify the provider tables and resolve the chat provider on the server (#2540) * feat(server): merge acp agents and llm providers into one table Both are providers for the chat, and everything above the database already said so: the chat target is one union with a kind on the client, and the wire target has always been a discriminated union. Only storage disagreed, and it charged for the mismatch. Selecting an acp agent left the default provider pointing at the previously selected llm one, because the write was conditional on kind. A scheduled job could reference an llm provider and nothing else, so picking Claude Code in chat was possible while scheduling against it was not. Migration 0010 creates the unified table and copies both sources in; 0011 repoints scheduled jobs and drops the old tables. Two migrations rather than one, so the copy proves itself before anything is dropped, and because drizzle cannot diff a simultaneous add and remove without a rename prompt. Drizzle put its drops ahead of its own foreign key pragma, which fired the ON DELETE SET NULL on scheduled jobs and silently unlinked every job from its provider. Reordered so the drops happen after the rebuild, inside the block where enforcement is off. The default provider now lives on a column, and a partial unique index admits exactly one row, of either kind. Not keyed by profile: sqlite treats nulls as distinct in a unique index, so pairing it with the unset profile column would let every row be default at once. Also brings the packaged-build schema fallback current. It had drifted four migrations behind, so a build without migration files would have created a database with none of this epic's tables. The test's copy of the migration history is now derived from the journal, since a hand written duplicate is what let the drift go unnoticed. * feat: resolve the chat provider on the server The provider block is gone from the chat body. A request names an id, or names nothing and gets whichever provider is selected, and the server fills in the model, endpoint and credentials from the row. Sixteen fields of provider configuration collapse into one, and the api key, the aws secret and the session token stop crossing the wire on every message. Every one of those fields is still accepted. The extension updates independently of the browser binary, so a shipped build can be running a client that sends the whole configuration inline; the server stops requiring them, not accepting them, and a row it does not recognise leaves whatever the client sent in place. The selection moved to the server with the tables it points at, so choosing a coding agent now records it. It could not before: the default lived in extension storage and only ever named an llm provider, so picking an agent left it pointing at the provider chosen before it. The scheduled runner drops its provider lookup entirely, and the guard that came with it. That guard existed because an unreachable list and an empty one looked alike, so a job could run on the built-in provider with the wrong credentials and still be recorded as a success. There is nothing to tell apart now: the job names an id and the server resolves it. Refine prompt still resolves on the client. It posts to its own endpoint with its own schema, and giving it the same treatment is separate work. * fix(server): gate chat on trust when the server supplies the credentials A browseros chat request is deliberately allowed without the app-origin check, on the reasoning that it carries its own credentials and so can only spend what the caller already had. Resolving the provider from storage broke that reasoning: naming an id, or naming nothing and taking the selected provider, would have let any local caller spend the user's key against an external service. The check now applies exactly when the configuration came from a stored row. A request that brought its own is as unrestricted as it was before, so the capability that reasoning was about is untouched. * fix(server): gate chat on every path where the server holds the credential The previous gate keyed on whether a stored provider row was read, on the reasoning that a request naming no known row must have brought its own key. That is false for four provider types. The oauth three take a token from this machine's oauth store and browseros takes the gateway credential, so naming one with an unknown id skipped the check and had the server hand over a credential the caller never held. The predicate lives beside resolveLLMConfig, since it has to mirror those branches exactly and would drift if the route kept its own copy. This particular hole predates the change: the exemption for browseros requests and the credential injection behind it were both already there. It is fixed here because the gate added alongside it claims the ungated path is safe, and that claim has to hold. The chat integration test now sends the origin header the extension always sends. It was relying on the exemption this closes, and the routes the background alarm runner already calls carry that header today. * test(server): stop a module mock dropping the exports it does not name CI failed with `Export named 'SERVER_CREDENTIALED_PROVIDERS' not found` against a file that plainly exports it. A module factory is a total replacement, so everything it omits disappears for every file importing that module afterwards, and bun's registry is process wide. Adding an export to a module someone mocks partially is enough to break a different file entirely. The factory now spreads the real module and overrides only the function under test. This does not reproduce locally: file ordering is stable on APFS and not on ext4, which the test runner's own notes call out as the reason this class of failure kills CI while local runs pass. * fix: restore cross-surface provider sync and keep credentials off provider reads (#2541) * fix(app): restore cross-surface provider sync and stop hiding credentials Three issues from testing on a real profile, two of which share a cause. Extension surfaces are separate contexts with separate query caches, and the provider list broadcast to all of them through extension storage until it moved to the server. Nothing replaced that, so a provider added in one surface stayed invisible to the others. The scheduled tasks dialog showed only the built-in provider for exactly that reason. Writers now bump a revision in extension storage, which does reach every context, mirroring what the scheduled runs already do. The same staleness turned destructive in the sidebar. Selecting a new provider wrote the choice and the server accepted it, then the repair effect ran against a list this surface had not refreshed, failed to find it, and wrote the built-in provider back over the selection. Agents were unaffected only because the repair skips them, which is why it looked like a provider problem. Absence from a list in hand is no longer read as deletion. A scheduled job can target a coding agent since the two provider tables merged, but the dialog only ever offered llm providers, so the capability was unreachable. It now offers both. Every provider read returned the api key and the aws secret. The store gained a projection that omits them and reports only whether each is set, and the one caller that needs them, building an outbound model request, asks by name. Since a client can no longer read a credential back, an upsert that omits one keeps what is stored, or a rename would wipe the key on save. Also filters the provider list on kind rather than leaning on the unknown-type guard, which dropped coding agents by accident and said something else in its comment. * fix(server): treat a blank credential as not supplied, not as a clear The keep-existing guard only skipped undefined, but a form field the user never filled in submits as an empty string, and the schema accepts it. So the very edit the guard exists to protect, renaming a provider without retyping a key that is no longer readable, wrote an empty string over the key. The flags then reported it as stored, because an empty string is not null, so nothing looked wrong until the next request failed to authenticate. Blank now counts as not supplied on the way in, and as unset on the way out. Clearing stays deliberate: send null. The four flags share one definition rather than four copies, since a divergence would only ever show on whichever credential nobody tested. * fix: wait for the server before running the one-time imports (#2544) * fix(app): wait for the server before running the one-time imports Found testing an upgrade against a real profile. The database migration worked and the extension storage was intact, yet no providers or scheduled tasks appeared, and the run looked like data loss. The imports run when the background starts, which is the same moment the server starts. They fired into a socket nothing was listening on yet and threw. Because a failed run leaves its marker unset, the next launch lost the same race, so the data never arrived at all while the database migration looked like it had succeeded. Roughly six seconds passed between the browser launching and the server answering. The existing retry did not help because it retried the wrong thing. resolveAgentServerUrlWithRetry retries getAgentServerUrl, which only reads a preference and effectively never fails, so the budget was spent before the request that needed it. The request itself had one attempt. The imports now wait on the health endpoint first, so they wait for their dependency rather than race it. Giving up returns false rather than throwing, which leaves the markers unset deliberately for the next start, and failures are reported through Sentry instead of swallowed: silence is what made this look like lost data rather than a slow start. * fix(app): probe the agent server rather than the proxy The wait used getHealthCheckUrl, which resolves the proxy port, while the imports address the agent server on the mcp port. They are separate services that can become ready at different moments, so the probe was answering a question nobody asked: a proxy up first would wave the imports through into the very race this closes, and a proxy that was down would defer imports the agent server was ready to accept. The two coincide in dev, where no proxy runs and the proxy port falls back to the server port, which is why it appeared to work. Health is now taken from the agent server's own base url, so the probe and the request address one service. --------- Co-authored-by: Nikhil Sonti <nikhilsv92@gmail.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: browseros-bot <bot@browseros.ai>
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.
Merges the two provider tables into one, moves the selected provider onto the server, and takes the provider block out of the chat request.
The tables were the problem
ACP agents and LLM providers are both providers for the chat, and everything above the database already said so: the chat target is one union with a kind on the client, and the wire target has always been a discriminated union. Only storage disagreed, and it charged for the mismatch in two places.
commitChatTargetSelectionwrote the selection unconditionally and the default only when the kind happened to be llm, so choosing a coding agent left the default pointing at whichever provider was selected before it. That is not two defaults being maintained; it is one real pointer and one stale shadow.scheduled_jobs.provider_idreferenced the llm table and nothing else, so picking Claude Code in chat was possible while scheduling against it was not.Both are gone. One table, one
is_defaultcolumn, and a partial unique index that admits exactly one default of either kind. Not keyed by profile: sqlite treats nulls as distinct in a unique index, so pairing it with the unset profile column would let every row be default at once, and the per-profile form belongs in whichever migration starts populating that column.Two migrations, and two defects caught by running them
0010 creates the unified table and copies both sources in; 0011 repoints scheduled jobs and drops the old tables. Split so the copy proves itself before anything is dropped, and because drizzle cannot diff a simultaneous add and remove without an interactive rename prompt. The two
INSERT INTO ... SELECTstatements are hand written into 0010 before it was ever applied, since drizzle generates schema changes and not data moves.Drizzle emitted invalid SQL for the expression index, splitting
coalesce(profile_id, '')on its comma and quoting each fragment as an identifier. It cannot express a sqlite expression index at this version, so the index is a plain partial one.The generated 0011 silently unlinked every scheduled job from its provider. It put the drops ahead of its own
PRAGMA foreign_keys=OFF, which fired theON DELETE SET NULLonscheduled_jobs.provider_id. A probe applying every migration to a seeded database showedprovider_idgoing fromanthropic-1tonull, which would have sent every job to the default provider on its next run. Reordered so the drops happen after the rebuild, inside the block where enforcement is off. Statement order is not part of the drizzle snapshot, so the schema still matches.That probe also confirms credentials survive, a non-default temperature survives as
0.7rather than being floored, an acp row keeps a null context window, the check constraint rejects an llm row without a model, the unique index rejects a second default, and an acp agent can be the single default.A stale fallback found on the way
bootstrapCurrentSchemarecreates the schema when a packaged build ships without migration files. It had drifted four migrations behind, stopping at 0007, so it never learned about this epic's tables: a build taking that path would have had no providers, jobs or runs at all. That gap arrived with the first phase of this epic and no test noticed, because the test kept its own transcribed copy of the migration history. The history is now derived from the journal and the file hashes, and removing an entry from the fallback fails three tests.The chat request lost sixteen fields
From 31 to 15. A request names a provider id, or names nothing and gets whichever is selected, and the server fills in the model, endpoint and credentials from the row. The api key, the aws secret and the session token stop crossing the wire on every message; a test asserts none of them appear in the body.
Every one of those fields is still accepted. The extension updates independently of the browser binary, so a shipped build can be running a client that sends the whole configuration inline. The server stops requiring them, not accepting them, and a row it does not recognise leaves what the client sent in place. Where both exist the row wins, because it is the source of truth and the client may hold a copy from before an edit.
The smallest body the endpoint now accepts is a message and a conversation id.
The scheduled runner dropped a guard along with the lookup
It no longer loads the provider list at all, which removes the guard added under review on an earlier PR. That guard existed because an unreachable list and an empty one were the same shape, so a job risked running on the built-in provider with the wrong credentials and being recorded as a success. There is nothing left to tell apart.
Verification
test:allgreen, coldtsc --noEmitclean on both appsLeft for later
refinePromptstill resolves its provider on the client. It posts to/refine-prompt, a separate endpoint with its own schema, so the same treatment there is its own change.