Skip to content

Commit c6ed702

Browse files
beardthelionguidovizosorenovate[bot]zopeVaibhavdavidmckayv
authored
Keep a finished suspension for the idle window, not for a day (#254)
* Keep a finished suspension for the idle window, not for a day Scale-to-zero worked once per Bot. A suspension is keyed on the Bot id and the finished row is kept so a late offer of the same key collides with it rather than running the work twice, both of which are right. What was wrong is how long it was kept: a day, so a computer that was resumed, used and left alone again was offered on every sweep after that, swallowed by its own finished row every time, and stayed awake until the row aged out. A sweep that offers work and suspends nothing looks exactly like a fleet that is busy, so nothing reported it. The day is the right window for the other half of the same purge, where it is the backoff before a suspension that keeps failing is tried again. One number could not be both, so `purge` now takes the two separately and defaults the new one to the old, leaving every other caller as it was. The cull sweep keeps a finished suspension for the idle window instead: the same clock the offer runs on, so a Bot cannot come back round as idle until its row has gone. The queue's own comment already described this wedge for the half that gives up, where five failed suspends meant a Bot never scaled to zero again. This is the same door on the other side. * Pin the window the cull sweep actually passes The fix's one load-bearing line was the one nothing ran. The script opens a database and a provider at import and sweeps as a side effect of loading, so no test executes it, and every test around it passes the retention window itself: deleting the line from the script left all twenty-two of them green. That is the failure this change is about, one level up. Read as text, the way `tests/compose.test.ts` pins the variables Compose has to name. It asserts the argument is written and where its value comes from, not that the sweep behaves, which the integration test beside it already owns against a real queue and a real database. The second assertion is there because collapsing both halves onto the idle window is the regression this fix could most easily cause: a suspension that keeps failing would be retried every few minutes instead of daily. * Show a dot on a channel a Bot has spoken in unseen (#259) * Give a membership a memory of when its channel was last read * Stamp the caller's membership read, and say so in the roster * Let a member say they have read a channel * Carry the read marker to the app and let it be stamped * Draw a dot on a channel a Bot has spoken in unseen * Mark a channel read the moment it is the one on screen * Narrow the admin and settings rails to the width their labels earn * Keep a fast clock elsewhere from turning mark-read into a storm * Say in the changelog what the unread dot is and is not * Stamp a read against the message clock, not just this one * Assert the clamped stamp without reaching through an optional chain * Regenerate the read-marker migration behind the queue and the frames * Refuse to mark a deleted channel read, matching the pin * Update azure/setup-helm action to v5 (#260) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Say what a strict content-security-policy has to allow (#225) * Point the test at the database the project actually has (#234) * Let the API reach Intelligence and sign-in when a NetworkPolicy is on (#257) * Refuse a credential written into the rest of the address (#230) Refusing a credential in the userinfo closed one spelling and left the two beside it open. A token in the query string or the fragment was accepted, and addCustomServer writes the address it was given into mcp_servers.url and into the configuration.changed audit payload verbatim. Redaction keys on the field name and url is not a sensitive one, so the secret landed in an append-only row in clear text, which is the disclosure the userinfo rule exists to prevent. The name is read rather than matched against a list. An exact-name version of this rule refused token and accepted auth_token, api_token, x-api-key and X-Amz-Signature, and an operator has no way to know which spellings the check happens to hold. Reading the name over-refuses in one direction on purpose: a misread parameter costs a rename, a missed one cannot be deleted afterwards. The fragment is split at the first question mark first, because a hash route or an OAuth-style callback puts a path in front of the parameters and reading the whole fragment as one query string turns all of it into a single name that matches nothing. metadata.goog is refused too, by asking the list browsing already uses rather than keeping a second copy here. It is Google's own short alias for the metadata server and it carries a dot and none of the suffixes this check lists, so it read as an ordinary vendor name, while the long spelling was refused only incidentally by the .internal test. * Spend an MCP token only for its own server, and only at its own address (#238) * Point a curated MCP server only at a credential of its own kind Adding a server by URL checks which credential it is being pointed at. Adding one from the catalogue took the same field from the same request and stored it unread, so a credential of any kind could be attached to a curated server and spent by the refresh that runs before the add returns. The reach is narrower than the path beside it and worth saying so. The column is a foreign key, so an id naming nothing was already refused by the database, and the one entry in the catalogue is reached with each person's own account, whose OAuth client is registered through its own call and sent to a pinned address. What was reachable is a credential of the wrong kind being accepted and spent on behalf of somebody who never agreed to it, a malformed id arriving as a database error where a refusal belongs, and the whole shape returning with the first deployment-bearer entry a fork re-adds, which the catalogue invites. Which kind an entry takes is decided beside the entry, because it is a property of the vendor's auth rather than of the request. Both add paths then ask one function the same question, so a credential that does not exist and one of the wrong kind are still refused in the same words and the endpoint cannot be asked which ids are real. The curated route maps that refusal to a 400 rather than letting it surface as a 500. Re-adding a curated server no longer clears the credential it points at. That column holds the OAuth client registering one put there, and a re-add to change an instance host said nothing about it while clearing it anyway, leaving the row orphaned and everybody who had connected told there is no client registered. * Spend an MCP token only for its own server, and only at its own address Attaching a credential to a server is the one place this deployment accepts a reference to a stored secret rather than the secret itself. Everywhere else the value arrives in the request that stores it, and the id it gets is nobody's to choose: storeAgentAuth mints its own row from the key an administrator typed. So this is the field where which secret and which address can be made to disagree, and the add is what settles it, because refreshTools runs before the call returns and sends what it decrypts to the URL from the same request. Both ways they could disagree are now refused. A credential has to belong to the server it is attached to, which the vault already records: storeMcpToken sets the provider to the server it mints for and is the only way the plugins screen makes one, so nothing a deployment can reach through the UI is refused by this. And a server that already holds a credential cannot be re-added at a different address, which is the case a check on ownership cannot see: the token does belong to that server, and only the address moved. The second is why the first is not enough alone. Both delivered a stored token to a host the caller named, before any Bot, grant or policy check existed, and a stored credential is otherwise unreadable by design. Refused rather than repaired, because both harmless readings are served by something else. Correcting a title or retrying an interrupted add sends the same URL and is untouched, a server holding no credential can still be re-addressed, and moving one that does means removing it and adding it again with the token the new address is meant to have. Curated servers are unaffected: their URL comes from the catalogue rather than the request, and an instance hostname is matched against the vendor's anchored pattern before anything is stored. The upsert test from #214 now mints its own token. It had reused one credential across two server ids, which is a shape storeMcpToken cannot produce. * Show a dot on a channel a Bot has spoken in unseen (#259) * Give a membership a memory of when its channel was last read * Stamp the caller's membership read, and say so in the roster * Let a member say they have read a channel * Carry the read marker to the app and let it be stamped * Draw a dot on a channel a Bot has spoken in unseen * Mark a channel read the moment it is the one on screen * Narrow the admin and settings rails to the width their labels earn * Keep a fast clock elsewhere from turning mark-read into a storm * Say in the changelog what the unread dot is and is not * Stamp a read against the message clock, not just this one * Assert the clamped stamp without reaching through an optional chain * Regenerate the read-marker migration behind the queue and the frames * Refuse to mark a deleted channel read, matching the pin * Update azure/setup-helm action to v5 (#260) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Say what a strict content-security-policy has to allow (#225) * Point the test at the database the project actually has (#234) * Let the API reach Intelligence and sign-in when a NetworkPolicy is on (#257) * Refuse a credential written into the rest of the address (#230) Refusing a credential in the userinfo closed one spelling and left the two beside it open. A token in the query string or the fragment was accepted, and addCustomServer writes the address it was given into mcp_servers.url and into the configuration.changed audit payload verbatim. Redaction keys on the field name and url is not a sensitive one, so the secret landed in an append-only row in clear text, which is the disclosure the userinfo rule exists to prevent. The name is read rather than matched against a list. An exact-name version of this rule refused token and accepted auth_token, api_token, x-api-key and X-Amz-Signature, and an operator has no way to know which spellings the check happens to hold. Reading the name over-refuses in one direction on purpose: a misread parameter costs a rename, a missed one cannot be deleted afterwards. The fragment is split at the first question mark first, because a hash route or an OAuth-style callback puts a path in front of the parameters and reading the whole fragment as one query string turns all of it into a single name that matches nothing. metadata.goog is refused too, by asking the list browsing already uses rather than keeping a second copy here. It is Google's own short alias for the metadata server and it carries a dot and none of the suffixes this check lists, so it read as an ordinary vendor name, while the long spelling was refused only incidentally by the .internal test. --------- Co-authored-by: Guido Vizoso <guido.vizoso9@gmail.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Vaibhav Zope <121456155+zopeVaibhav@users.noreply.github.com> Co-authored-by: David McKay <davidmckayv@users.noreply.github.com> * Carry the per-Bot egress proxy as far as the process that reads it (#250) * Carry the per-Bot egress proxy as far as the process that reads it `EGRESS_PROXY_DEFAULT` and `EGRESS_PROXY_<BOT>` are documented in .env.example and docs/configuration.md, and neither reached any process. docker-compose.yml named no EGRESS variable and had no `env_file`, and Compose hands a container only what those two blocks name. So the shared computer resolved every Bot to null and went out directly, and in the supervisor arrangement the supervisor's own environment held none either, leaving its EGRESS_PROXY passthrough with nothing to forward into the computers it creates. Nothing said so. The operator sets a proxy, the stack starts, the browser leaves by the host, and the Computers screen reports "Leaves directly" because it is reading the same empty environment. For a setting whose stated purpose is to give a security team a per-Bot address for network rules, silently doing nothing is the worst of the available failures. A file rather than more `environment:` entries because `EGRESS_PROXY_<BOT>` is derived from a Bot's id, so there is no fixed set of names to write out here. A file of its own rather than .env because that one holds the deployment's secrets, and the container driving a browser and running a Bot's shell is deliberately given what it needs and not the rest. It is optional, since going out directly is the ordinary case and must still start, and gitignored, because a proxy URL can carry a password. The all-in-one image was never affected: its s6 service runs under `with-contenv` and inherits the container's environment, which is the mechanism this restores for Compose. * Show a dot on a channel a Bot has spoken in unseen (#259) * Give a membership a memory of when its channel was last read * Stamp the caller's membership read, and say so in the roster * Let a member say they have read a channel * Carry the read marker to the app and let it be stamped * Draw a dot on a channel a Bot has spoken in unseen * Mark a channel read the moment it is the one on screen * Narrow the admin and settings rails to the width their labels earn * Keep a fast clock elsewhere from turning mark-read into a storm * Say in the changelog what the unread dot is and is not * Stamp a read against the message clock, not just this one * Assert the clamped stamp without reaching through an optional chain * Regenerate the read-marker migration behind the queue and the frames * Refuse to mark a deleted channel read, matching the pin * Update azure/setup-helm action to v5 (#260) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Say what a strict content-security-policy has to allow (#225) * Point the test at the database the project actually has (#234) * Let the API reach Intelligence and sign-in when a NetworkPolicy is on (#257) * Refuse a credential written into the rest of the address (#230) Refusing a credential in the userinfo closed one spelling and left the two beside it open. A token in the query string or the fragment was accepted, and addCustomServer writes the address it was given into mcp_servers.url and into the configuration.changed audit payload verbatim. Redaction keys on the field name and url is not a sensitive one, so the secret landed in an append-only row in clear text, which is the disclosure the userinfo rule exists to prevent. The name is read rather than matched against a list. An exact-name version of this rule refused token and accepted auth_token, api_token, x-api-key and X-Amz-Signature, and an operator has no way to know which spellings the check happens to hold. Reading the name over-refuses in one direction on purpose: a misread parameter costs a rename, a missed one cannot be deleted afterwards. The fragment is split at the first question mark first, because a hash route or an OAuth-style callback puts a path in front of the parameters and reading the whole fragment as one query string turns all of it into a single name that matches nothing. metadata.goog is refused too, by asking the list browsing already uses rather than keeping a second copy here. It is Google's own short alias for the metadata server and it carries a dot and none of the suffixes this check lists, so it read as an ordinary vendor name, while the long spelling was refused only incidentally by the .internal test. * Spend an MCP token only for its own server, and only at its own address (#238) * Point a curated MCP server only at a credential of its own kind Adding a server by URL checks which credential it is being pointed at. Adding one from the catalogue took the same field from the same request and stored it unread, so a credential of any kind could be attached to a curated server and spent by the refresh that runs before the add returns. The reach is narrower than the path beside it and worth saying so. The column is a foreign key, so an id naming nothing was already refused by the database, and the one entry in the catalogue is reached with each person's own account, whose OAuth client is registered through its own call and sent to a pinned address. What was reachable is a credential of the wrong kind being accepted and spent on behalf of somebody who never agreed to it, a malformed id arriving as a database error where a refusal belongs, and the whole shape returning with the first deployment-bearer entry a fork re-adds, which the catalogue invites. Which kind an entry takes is decided beside the entry, because it is a property of the vendor's auth rather than of the request. Both add paths then ask one function the same question, so a credential that does not exist and one of the wrong kind are still refused in the same words and the endpoint cannot be asked which ids are real. The curated route maps that refusal to a 400 rather than letting it surface as a 500. Re-adding a curated server no longer clears the credential it points at. That column holds the OAuth client registering one put there, and a re-add to change an instance host said nothing about it while clearing it anyway, leaving the row orphaned and everybody who had connected told there is no client registered. * Spend an MCP token only for its own server, and only at its own address Attaching a credential to a server is the one place this deployment accepts a reference to a stored secret rather than the secret itself. Everywhere else the value arrives in the request that stores it, and the id it gets is nobody's to choose: storeAgentAuth mints its own row from the key an administrator typed. So this is the field where which secret and which address can be made to disagree, and the add is what settles it, because refreshTools runs before the call returns and sends what it decrypts to the URL from the same request. Both ways they could disagree are now refused. A credential has to belong to the server it is attached to, which the vault already records: storeMcpToken sets the provider to the server it mints for and is the only way the plugins screen makes one, so nothing a deployment can reach through the UI is refused by this. And a server that already holds a credential cannot be re-added at a different address, which is the case a check on ownership cannot see: the token does belong to that server, and only the address moved. The second is why the first is not enough alone. Both delivered a stored token to a host the caller named, before any Bot, grant or policy check existed, and a stored credential is otherwise unreadable by design. Refused rather than repaired, because both harmless readings are served by something else. Correcting a title or retrying an interrupted add sends the same URL and is untouched, a server holding no credential can still be re-addressed, and moving one that does means removing it and adding it again with the token the new address is meant to have. Curated servers are unaffected: their URL comes from the catalogue rather than the request, and an instance hostname is matched against the vendor's anchored pattern before anything is stored. The upsert test from #214 now mints its own token. It had reused one credential across two server ids, which is a shape storeMcpToken cannot produce. * Show a dot on a channel a Bot has spoken in unseen (#259) * Give a membership a memory of when its channel was last read * Stamp the caller's membership read, and say so in the roster * Let a member say they have read a channel * Carry the read marker to the app and let it be stamped * Draw a dot on a channel a Bot has spoken in unseen * Mark a channel read the moment it is the one on screen * Narrow the admin and settings rails to the width their labels earn * Keep a fast clock elsewhere from turning mark-read into a storm * Say in the changelog what the unread dot is and is not * Stamp a read against the message clock, not just this one * Assert the clamped stamp without reaching through an optional chain * Regenerate the read-marker migration behind the queue and the frames * Refuse to mark a deleted channel read, matching the pin * Update azure/setup-helm action to v5 (#260) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Say what a strict content-security-policy has to allow (#225) * Point the test at the database the project actually has (#234) * Let the API reach Intelligence and sign-in when a NetworkPolicy is on (#257) * Refuse a credential written into the rest of the address (#230) Refusing a credential in the userinfo closed one spelling and left the two beside it open. A token in the query string or the fragment was accepted, and addCustomServer writes the address it was given into mcp_servers.url and into the configuration.changed audit payload verbatim. Redaction keys on the field name and url is not a sensitive one, so the secret landed in an append-only row in clear text, which is the disclosure the userinfo rule exists to prevent. The name is read rather than matched against a list. An exact-name version of this rule refused token and accepted auth_token, api_token, x-api-key and X-Amz-Signature, and an operator has no way to know which spellings the check happens to hold. Reading the name over-refuses in one direction on purpose: a misread parameter costs a rename, a missed one cannot be deleted afterwards. The fragment is split at the first question mark first, because a hash route or an OAuth-style callback puts a path in front of the parameters and reading the whole fragment as one query string turns all of it into a single name that matches nothing. metadata.goog is refused too, by asking the list browsing already uses rather than keeping a second copy here. It is Google's own short alias for the metadata server and it carries a dot and none of the suffixes this check lists, so it read as an ordinary vendor name, while the long spelling was refused only incidentally by the .internal test. --------- Co-authored-by: Guido Vizoso <guido.vizoso9@gmail.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Vaibhav Zope <121456155+zopeVaibhav@users.noreply.github.com> Co-authored-by: David McKay <davidmckayv@users.noreply.github.com> --------- Co-authored-by: Guido Vizoso <guido.vizoso9@gmail.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Vaibhav Zope <121456155+zopeVaibhav@users.noreply.github.com> Co-authored-by: David McKay <davidmckayv@users.noreply.github.com> * Refuse the shell and a workspace write while a person holds the wheel (#247) * Refuse the shell and a workspace write while a person holds the wheel `assertBotMayAct` was called in the navigate handler and the four action handlers, and nowhere else. `/exec` and `/files/write` were not covered, so a Bot could keep running commands and rewriting its workspace underneath somebody who had taken the browser at a login wall. The shell arrived after the wheel existed and was never wired to it. That is the property this codebase states outright. control.ts says every acting call from the Bot is refused while a person holds control, and the README says Bot actions are refused rather than queued. Both were false for the most powerful path the product exposes, and the server could not cover for it: control lives in this process, so those two call sites were the whole of the enforcement. The decision moves to `actsOnTheComputer` in authorisation.ts and is asked once by the dispatcher, after the session resolves. A per-handler check is the thing the next endpoint forgets, which is exactly how the shell came to be missing one; a list the dispatcher consults has to be added to instead. It lives beside the other path decision rather than in index.ts because that file imports Playwright at module scope, so a decision left there cannot be tested without Chrome. Reading stays open. `/files/read` and `/files/list` are not acting, and a Bot that has just been stopped still needs to say what it was doing. The two in-handler guards and their now-dead ControlError branches come out with it, so there is one place that answers this and not three. * Say in the changelog that the wheel now stops the shell A deployment behaves differently afterwards: an action that used to run during a takeover is refused, so it belongs here rather than only in the commit. * Show a dot on a channel a Bot has spoken in unseen (#259) * Give a membership a memory of when its channel was last read * Stamp the caller's membership read, and say so in the roster * Let a member say they have read a channel * Carry the read marker to the app and let it be stamped * Draw a dot on a channel a Bot has spoken in unseen * Mark a channel read the moment it is the one on screen * Narrow the admin and settings rails to the width their labels earn * Keep a fast clock elsewhere from turning mark-read into a storm * Say in the changelog what the unread dot is and is not * Stamp a read against the message clock, not just this one * Assert the clamped stamp without reaching through an optional chain * Regenerate the read-marker migration behind the queue and the frames * Refuse to mark a deleted channel read, matching the pin * Update azure/setup-helm action to v5 (#260) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Say what a strict content-security-policy has to allow (#225) * Point the test at the database the project actually has (#234) * Let the API reach Intelligence and sign-in when a NetworkPolicy is on (#257) * Refuse a credential written into the rest of the address (#230) Refusing a credential in the userinfo closed one spelling and left the two beside it open. A token in the query string or the fragment was accepted, and addCustomServer writes the address it was given into mcp_servers.url and into the configuration.changed audit payload verbatim. Redaction keys on the field name and url is not a sensitive one, so the secret landed in an append-only row in clear text, which is the disclosure the userinfo rule exists to prevent. The name is read rather than matched against a list. An exact-name version of this rule refused token and accepted auth_token, api_token, x-api-key and X-Amz-Signature, and an operator has no way to know which spellings the check happens to hold. Reading the name over-refuses in one direction on purpose: a misread parameter costs a rename, a missed one cannot be deleted afterwards. The fragment is split at the first question mark first, because a hash route or an OAuth-style callback puts a path in front of the parameters and reading the whole fragment as one query string turns all of it into a single name that matches nothing. metadata.goog is refused too, by asking the list browsing already uses rather than keeping a second copy here. It is Google's own short alias for the metadata server and it carries a dot and none of the suffixes this check lists, so it read as an ordinary vendor name, while the long spelling was refused only incidentally by the .internal test. * Spend an MCP token only for its own server, and only at its own address (#238) * Point a curated MCP server only at a credential of its own kind Adding a server by URL checks which credential it is being pointed at. Adding one from the catalogue took the same field from the same request and stored it unread, so a credential of any kind could be attached to a curated server and spent by the refresh that runs before the add returns. The reach is narrower than the path beside it and worth saying so. The column is a foreign key, so an id naming nothing was already refused by the database, and the one entry in the catalogue is reached with each person's own account, whose OAuth client is registered through its own call and sent to a pinned address. What was reachable is a credential of the wrong kind being accepted and spent on behalf of somebody who never agreed to it, a malformed id arriving as a database error where a refusal belongs, and the whole shape returning with the first deployment-bearer entry a fork re-adds, which the catalogue invites. Which kind an entry takes is decided beside the entry, because it is a property of the vendor's auth rather than of the request. Both add paths then ask one function the same question, so a credential that does not exist and one of the wrong kind are still refused in the same words and the endpoint cannot be asked which ids are real. The curated route maps that refusal to a 400 rather than letting it surface as a 500. Re-adding a curated server no longer clears the credential it points at. That column holds the OAuth client registering one put there, and a re-add to change an instance host said nothing about it while clearing it anyway, leaving the row orphaned and everybody who had connected told there is no client registered. * Spend an MCP token only for its own server, and only at its own address Attaching a credential to a server is the one place this deployment accepts a reference to a stored secret rather than the secret itself. Everywhere else the value arrives in the request that stores it, and the id it gets is nobody's to choose: storeAgentAuth mints its own row from the key an administrator typed. So this is the field where which secret and which address can be made to disagree, and the add is what settles it, because refreshTools runs before the call returns and sends what it decrypts to the URL from the same request. Both ways they could disagree are now refused. A credential has to belong to the server it is attached to, which the vault already records: storeMcpToken sets the provider to the server it mints for and is the only way the plugins screen makes one, so nothing a deployment can reach through the UI is refused by this. And a server that already holds a credential cannot be re-added at a different address, which is the case a check on ownership cannot see: the token does belong to that server, and only the address moved. The second is why the first is not enough alone. Both delivered a stored token to a host the caller named, before any Bot, grant or policy check existed, and a stored credential is otherwise unreadable by design. Refused rather than repaired, because both harmless readings are served by something else. Correcting a title or retrying an interrupted add sends the same URL and is untouched, a server holding no credential can still be re-addressed, and moving one that does means removing it and adding it again with the token the new address is meant to have. Curated servers are unaffected: their URL comes from the catalogue rather than the request, and an instance hostname is matched against the vendor's anchored pattern before anything is stored. The upsert test from #214 now mints its own token. It had reused one credential across two server ids, which is a shape storeMcpToken cannot produce. * Show a dot on a channel a Bot has spoken in unseen (#259) * Give a membership a memory of when its channel was last read * Stamp the caller's membership read, and say so in the roster * Let a member say they have read a channel * Carry the read marker to the app and let it be stamped * Draw a dot on a channel a Bot has spoken in unseen * Mark a channel read the moment it is the one on screen * Narrow the admin and settings rails to the width their labels earn * Keep a fast clock elsewhere from turning mark-read into a storm * Say in the changelog what the unread dot is and is not * Stamp a read against the message clock, not just this one * Assert the clamped stamp without reaching through an optional chain * Regenerate the read-marker migration behind the queue and the frames * Refuse to mark a deleted channel read, matching the pin * Update azure/setup-helm action to v5 (#260) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Say what a strict content-security-policy has to allow (#225) * Point the test at the database the project actually has (#234) * Let the API reach Intelligence and sign-in when a NetworkPolicy is on (#257) * Refuse a credential written into the rest of the address (#230) Refusing a credential in the userinfo closed one spelling and left the two beside it open. A token in the query string or the fragment was accepted, and addCustomServer writes the address it was given into mcp_servers.url and into the configuration.changed audit payload verbatim. Redaction keys on the field name and url is not a sensitive one, so the secret landed in an append-only row in clear text, which is the disclosure the userinfo rule exists to prevent. The name is read rather than matched against a list. An exact-name version of this rule refused token and accepted auth_token, api_token, x-api-key and X-Amz-Signature, and an operator has no way to know which spellings the check happens to hold. Reading the name over-refuses in one direction on purpose: a misread parameter costs a rename, a missed one cannot be deleted afterwards. The fragment is split at the first question mark first, because a hash route or an OAuth-style callback puts a path in front of the parameters and reading the whole fragment as one query string turns all of it into a single name that matches nothing. metadata.goog is refused too, by asking the list browsing already uses rather than keeping a second copy here. It is Google's own short alias for the metadata server and it carries a dot and none of the suffixes this check lists, so it read as an ordinary vendor name, while the long spelling was refused only incidentally by the .internal test. --------- Co-authored-by: Guido Vizoso <guido.vizoso9@gmail.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Vaibhav Zope <121456155+zopeVaibhav@users.noreply.github.com> Co-authored-by: David McKay <davidmckayv@users.noreply.github.com> * Carry the per-Bot egress proxy as far as the process that reads it (#250) * Carry the per-Bot egress proxy as far as the process that reads it `EGRESS_PROXY_DEFAULT` and `EGRESS_PROXY_<BOT>` are documented in .env.example and docs/configuration.md, and neither reached any process. docker-compose.yml named no EGRESS variable and had no `env_file`, and Compose hands a container only what those two blocks name. So the shared computer resolved every Bot to null and went out directly, and in the supervisor arrangement the supervisor's own environment held none either, leaving its EGRESS_PROXY passthrough with nothing to forward into the computers it creates. Nothing said so. The operator sets a proxy, the stack starts, the browser leaves by the host, and the Computers screen reports "Leaves directly" because it is reading the same empty environment. For a setting whose stated purpose is to give a security team a per-Bot address for network rules, silently doing nothing is the worst of the available failures. A file rather than more `environment:` entries because `EGRESS_PROXY_<BOT>` is derived from a Bot's id, so there is no fixed set of names to write out here. A file of its own rather than .env because that one holds the deployment's secrets, and the container driving a browser and running a Bot's shell is deliberately given what it needs and not the rest. It is optional, since going out directly is the ordinary case and must still start, and gitignored, because a proxy URL can carry a password. The all-in-one image was never affected: its s6 service runs under `with-contenv` and inherits the container's environment, which is the mechanism this restores for Compose. * Show a dot on a channel a Bot has spoken in unseen (#259) * Give a membership a memory of when its channel was last read * Stamp the caller's membership read, and say so in the roster * Let a member say they have read a channel * Carry the read marker to the app and let it be stamped * Draw a dot on a channel a Bot has spoken in unseen * Mark a channel read the moment it is the one on screen * Narrow the admin and settings rails to the width their labels earn * Keep a fast clock elsewhere from turning mark-read into a storm * Say in the changelog what the unread dot is and is not * Stamp a read against the message clock, not just this one * Assert the clamped stamp without reaching through an optional chain * Regenerate the read-marker migration behind the queue and the frames * Refuse to mark a deleted channel read, matching the pin * Update azure/setup-helm action to v5 (#260) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Say what a strict content-security-policy has to allow (#225) * Point the test at the database the project actually has (#234) * Let the API reach Intelligence and sign-in when a NetworkPolicy is on (#257) * Refuse a credential written into the rest of the address (#230) Refusing a credential in the userinfo closed one spelling and left the two beside it open. A token in the query string or the fragment was accepted, and addCustomServer writes the address it was given into mcp_servers.url and into the configuration.changed audit payload verbatim. Redaction keys on the field name and url is not a sensitive one, so the secret landed in an append-only row in clear text, which is the disclosure the userinfo rule exists to prevent. The name is read rather than matched against a list. An exact-name version of this rule refused token and accepted auth_token, api_token, x-api-key and X-Amz-Signature, and an operator has no way to know which spellings the check happens to hold. Reading the name over-refuses in one direction on purpose: a misread parameter costs a rename, a missed one cannot be deleted afterwards. The fragment is split at the first question mark first, because a hash route or an OAuth-style callback puts a path in front of the parameters and reading the whole fragment as one query string turns all of it into a single name that matches nothing. metadata.goog is refused too, by asking the list browsing already uses rather than keeping a second copy here. It is Google's own short alias for the metadata server and it carries a dot and none of the suffixes this check lists, so it read as an ordinary vendor name, while the long spelling was refused only incidentally by the .internal test. * Spend an MCP token only for its own server, and only at its own address (#238) * Point a curated MCP server only at a credential of its own kind Adding a server by URL checks which credential it is being pointed at. Adding one from the catalogue took the same field from the same request and stored it unread, so a credential of any kind could be attached to a curated server and spent by the refresh that runs before the add returns. The reach is narrower than the path beside it and worth saying so. The column is a foreign key, so an id naming nothing was already refused by the database, and the one entry in the catalogue is reached with each person's own account, whose OAuth client is registered through its own call and sent to a pinned address. What was reachable is a credential of the wrong kind being accepted and spent on behalf of somebody who never agreed to it, a malformed id arriving as a database error where a refusal belongs, and the whole shape returning with the first deployment-bearer entry a fork re-adds, which the catalogue invites. Which kind an entry takes is decided beside the entry, because it is a property of the vendor's auth rather than of the request. Both add paths then ask one function the same question, so a credential that does not exist and one of the wrong kind are still refused in the same words and the endpoint cannot be asked which ids are real. The curated route maps that refusal to a 400 rather than letting it surface as a 500. Re-adding a curated server no longer clears the credential it points at. That column holds the OAuth client registering one put there, and a re-add to change an instance host said nothing about it while clearing it anyway, leaving the row orphaned and everybody who had connected told there is no client registered. * Spend an MCP token only for its own server, and only at its own address Attaching a credential to a server is the one place this deployment accepts a reference to a stored secret rather than the secret itself. Everywhere else the value arrives in the request that stores it, and the id it gets is nobody's to choose: storeAgentAuth mints its own row from the key an administrator typed. So this is the field where which secret and which address can be made to disagree, and the add is what settles it, because refreshTools runs before the call returns and sends what it decrypts to the URL from the same request. Both ways they could disagree are now refused. A credential has to belong to the server it is attached to, which the vault already records: storeMcpToken sets the provider to the server it mints for and is the only way the plugins screen makes one, so nothing a deployment can reach through the UI is refused by this. And a server that already holds a credential cannot be re-added at a different address, which is the case a check on ownership cannot see: the token does belong to that server, and only the address moved. The second is why the first is not enough alone. Both delivered a stored token to a host the caller named, before any Bot, grant or policy check existed, and a stored credential is otherwise unreadable by design. Refused rather than repaired, because both harmless readings are served by something else. Correcting a title or retrying an interrupted add sends the same URL and is untouched, a server holding no credential can still be re-addressed, and moving one that does means removing it and adding it again with the token the new address is meant to have. Curated servers are unaffected: their URL comes from the catalogue rather than the request, and an instance hostname is matched against the vendor's anchored pattern before anything is stored. The upsert test from #214 now mints its own token. It had reused one credential across two server ids, which is a shape storeMcpToken cannot produce. * Show a dot on a channel a Bot has spoken in unseen (#259) * Give a membership a memory of when its channel was last read * Stamp the caller's membership read, and say so in the roster * Let a member say they have read a channel * Carry the read marker to the app and let it be stamped * Draw a dot on a channel a Bot has spoken in unseen * Mark a channel read the moment it is the one on screen * Narrow the admin and settings rails to the width their labels earn * Keep a fast clock elsewhere from turning mark-read into a storm * Say in the changelog what the unread dot is and is not * Stamp a read against the message clock, not just this one * Assert the clamped stamp without reaching through an optional chain * Regenerate the read-marker migration behind the queue and the frames * Refuse to mark a deleted channel read, matching the pin * Update azure/setup-helm action to v5 (#260) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Say what a strict content-security-policy has to allow (#225) * Point the test at the database the project actually has (#234) * Let the API reach Intelligence and sign-in when a NetworkPolicy is on (#257) * Refuse a credential written into the rest of the address (#230) Refusing a credential in the userinfo closed one spelling and left the two beside it open. A token in the query string or the fragment was accepted, and addCustomServer writes the address it was given into mcp_servers.url and into the configuration.changed audit payload verbatim. Redaction keys on the field name and url is not a sensitive one, so the secret landed in an append-only row in clear text, which is the disclosure the userinfo rule exists to prevent. The name is read rather than matched against a list. An exact-name version of this rule refused token and accepted auth_token, api_token, x-api-key and X-Amz-Signature, and an operator has no way to know which spellings the check happens to hold. Reading the name over-refuses in one direction on purpose: a misread parameter costs a rename, a missed one cannot be deleted afterwards. The fragment is split at the first question mark first, because a hash route or an OAuth-style callback puts a path in front of the parameters and reading the whole fragment as one query string turns all of it into a single name that matches nothing. metadata.goog is refused too, by asking the list browsing already uses rather than keeping a second copy here. It is Google's own short alias for the metadata server and it carries a dot and none of the suffixes this check lists, so it read as an ordinary vendor name, while the long spelling was refused only incidentally by the .internal test. --------- Co-authored-by: Guido Vizoso <guido.vizoso9@gmail.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Vaibhav Zope <121456155+zopeVaibhav@users.noreply.github.com> Co-authored-by: David McKay <davidmckayv@users.noreply.github.com> --------- Co-authored-by: Guido Vizoso <guido.vizoso9@gmail.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Vaibhav Zope <121456155+zopeVaibhav@users.noreply.github.com> Co-authored-by: David McKay <davidmckayv@users.noreply.github.com> --------- Co-authored-by: Guido Vizoso <guido.vizoso9@gmail.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Vaibhav Zope <121456155+zopeVaibhav@users.noreply.github.com> Co-authored-by: David McKay <davidmckayv@users.noreply.github.com> * Stop grant queries polling the placeholder Bot (#240) * fix: stop grant queries polling the placeholder Bot Every surface polls which components its Bot holds, so a revoked grant leaves an open conversation within seconds. On a screen with no conversation, the Bot it polled about was the placeholder id the routing holder falls back to — which no package registers and the server 404s. An admin page left open asked a guaranteed miss every five seconds, forever. Nothing looked broken: an absent grant list and an empty one render identically. The cost is that a request log where the same 404 repeats indefinitely is one where a 404 that matters is invisible. `declaredBotId` names the distinction the holder already had but nothing could ask about: the placeholder exists so a handler always has something to route with, but it is not a Bot. The grant queries now take the declared id and their existing `enabled` guard does the rest — undefined simply does not run. Conversation surfaces declare a real Bot and are unchanged, as are the call-time checks, which never trusted the poll anyway. * Channel pin and soft delete, and a Notion connector over hosted MCP (#242) * Let a member pin a channel and soft-delete it, from a right-click menu * Land the caret in the composer once a coworker is chosen * Calm the screen panel down and make the full-size view a card * Let a Bot's message take the whole transcript column * Put Notion in the catalogue, and let a vendor register its OAuth client dynamically * Rotate refresh tokens in place, serialised per connection, and recover an evicted client * Introduce the deployment to a dynamic vendor on first connect * Show Notion in the plugin screens, without a client form it does not need * Say what the Notion connector is, everywhere the catalogue is described * Grant a batch of tools to Bots from the vendor page * Hold the vault row while a rotating token is spent, so replicas take turns * Refuse to mint a second client inside the re-registration window * Tell every member's roster when a channel is deleted, again * Leave a who-and-when behind a soft delete, again * Carry a pin across one person's own tabs * Say the classification direction right everywhere a person reads it * Refuse to whisper into a deleted channel `get` and `list` filter on `deleted_at`; `recordActivity` and `setPinned` did not. Activity POSTed to a soft-deleted channel returned 204, bumped `last_message`, and announced it to every member, each of whom then refetched a roster for a row it cannot show; a pin on one succeeded the same way. Both now join the channel and require it undeleted, throwing ChannelNotFoundError to match `get`, which also keeps the notify off the refused path since it is written inside the transaction. The roster's second query repeats the same filter. It selects the page and then joins the agents to it in a separate statement on a separate snapshot, so a delete committing between the two would hand back a channel this person can no longer see. * Hold a pinned channel at the top of the roster, not the page The roster ordered by recency alone and the client lifted pinned rows at render, so a pin only reached the top of whatever pages were loaded: a channel somebody pinned and then did not talk to for a month sat on page three and never appeared above anything. The promise is about the roster, so the ordering belongs in the query. The page now orders by the pin first and the cursor carries it as the leading element. Every part of the sort descends — a pin is 1 and no pin is 0 — which keeps the keyset predicate a single row comparison rather than a nest of ORs, and a cursor minted before the pin existed reads as the first page, like any other cursor describing an ordering this query no longer has. `pinnedFirst` stays in the sidebar as the render-level mirror, for the window between refetches: the socket patches a pin onto a loaded row without moving it, and re-sorts a page by recency alone. Its comment now says that is what it is for, rather than claiming to be where the rule lives. * Read a vendor's garbage as a refusal, not a crash * Keep the wheel reachable when the screen has nothing to show Take control and Hand back live in the full-size view, and the only way in was disabled unless there was a picture to open. So a blank browser, a screenshot that had not arrived, or a computer that could not be reached left a person with no way to take the wheel at all - the three states where they most want it. The frame now opens whatever is in it, and with nothing to draw the full-size view reserves the same shape and says the same words the card does, with the wheel underneath them. Somebody already driving keeps the live socket, whatever is on the page: once a person holds the wheel the stream is the truth about it. The Bot ASKING for the wheel comes back to the card as its own amber row with the reason on it, which is what the rework dropped. It is not the persistent footer that was deliberately removed - it is there only while the request is, next to the credential form, which is the other thing a stuck Bot needs. * Answer pin and delete failures where they happened Three things this row did quietly. A refused delete stayed on the mutation, so reopening the confirm showed a stale 409 about an attempt nobody had made yet; the menu resets it on the way in. A failed pin said nothing at all - the menu closed, the pin did not move, and that reads as the app ignoring the click - so the sentence now lands on the row, there being no toast in this app. And a delete of the channel on screen navigated home after the write. The roster invalidates the moment it lands, which unmounts this row and the dialog inside it, so the navigate belonged to a component that was already gone. Leaving first is safe in the other direction: a refusal puts them on the roster with the channel still in it, and says why. * Grant a batch with one refetch and a progress count Two Bots and twelve tools is twenty-four writes, and every one of them went through the grant mutation - which invalidates every plugin query and waits for the refetch. Most of the wait was re-reading a list hidden behind the dialog. The write is now its own function with no refetch attached, and the dialog invalidates once when the loop is done, including after a refusal, because the grants before it landed. The button says which of the N is in flight rather than only "Granting", so a slow batch can be told from a stuck one, and each set of tickboxes is a fieldset named by the heading already above it - "Changes things" is the whole warning on those tools, and a listener would otherwise never hear it. * Sweep the code the screen rework orphaned `hasBrowsed` had no callers left once the screen and the activity log stopped being tabs that had to guess which one to open, and the placeholder artwork went with the blank-browser strip it decorated. The note itself stays: the tool handler is the only place the fact exists, and a screenshot cannot answer it. The composer's autofocus is a mount-time courtesy, claimed once. Keyed off the editor becoming interactive, it re-fired on every disabled or busy transition, so a completed turn yanked the caret back from wherever the person had moved it. A send of their own still returns it - that one they asked for. * Stop pretending a new client can spend an old grant * Let two first connects race to one client * Cap, revoke and say what refresh saw * Seal the consent state, not just sign it * Refuse a consent that outlived the person's access * Run OpenBot on Kubernetes: Bots and all, proven on EKS (#235) * Run OpenBot on Kubernetes: a Helm chart, and what installing it found One chart for EKS, GKE, AKS and somebody's own cluster, with nothing but values between them. No cloud branching in any template: every place the clouds differ is a value whose default is what a plain self-hosted cluster does. Identity is one annotations map, because that is all IRSA, Workload Identity and AKS workload identity are. Secrets are a plain Secret by default and an ExternalSecret against any backend when asked. Two replicas by default, because horizontal is the point and one hides every bug that is not. A bad install is refused at helm install naming the value to change, rather than found in a crash loop. Three things only a real install could find: drizzle-kit cannot migrate in the shipped image. It reads a TypeScript config, which needs the esbuild that bun install --production leaves out, so it printed one line, exited 1 and said nothing. EMBEDDED_POSTGRES=on was starting containers whose database was never migrated. The migrator inside drizzle-orm is a runtime dependency already and keeps the same journal. sessionOf answered from a map in the process that started the computer, which is right until there are two of them. The replica taking a snapshot is usually not the one handling the click, and an unknown session skips the generation check rather than failing it, so the check that stops a ref from a replaced computer resolving against a live one was silently absent on the shape it was written for. It now asks by listing, never by ensuring, so asking cannot start a computer that had stopped. A browser in an API pod cannot be replicated, so the image's computer gets the same switch its database has. * Give Bots computers on Kubernetes, and suspend them when idle The chart had no computer, so no Bot could do anything on a cluster. It has one now, and a Bot has driven a real browser on real EKS with the decision in the audit trail. computers.mode picks the shape. shared runs one browser for every Bot and needs nothing installed. sandbox gives each Bot its own as a Sandbox from kubernetes-sigs/agent-sandbox, which is built for exactly this: an isolated stateful singleton with a stable identity and persistent storage, where suspending is a field that keeps the volumes, so a computer comes back with its logins rather than signed out. What decides a computer is idle is the audit trail, not the browser. Asking the browser wakes it, so every computer anything asked about would come back up and the bill would never fall. The work is claimed and leased out of Postgres with for update skip locked. Three features need that one mechanism, so it is written once with all three in view: the culler here, routines, and a hop from one Bot to another. A CronJob runs the sweep rather than a timer in the API, because a timer fires in every replica and suspending a browser somebody just started using is not something to do five times. Also: a fresh EKS cluster very often has no default StorageClass. eksctl creates gp2, unmarked and on the in-tree provisioner current Kubernetes no longer has, so a volume asking for the default never binds and nothing says why. Found on a real 1.34 cluster and written down where somebody configuring one will read it. * Refuse a sandbox install on a cluster that cannot make one computers.mode: sandbox creates Sandbox objects, which exist only once the agent-sandbox controller is installed. Without it the install succeeds, every pod is healthy, and the deployment looks finished right up until the first Bot asks for a browser and the API server answers 404. That is the worst moment to learn it. The check reads the cluster rather than a value somebody has to remember to set, and the message carries the one command that fixes it. Proven both ways: refused on a cluster with no CRD, installs on the EKS cluster that has one. Also from driving it on real EKS: lost+found was listed as a Bot, because an EBS volume is ext4 and arrives with that directory, which a bind mount never does. The allow-list that stops a hostile id becoming a path answers the other half of the question too. The migration Job named a ServiceAccount that does not exist yet, since a pre-install hook runs before the chart's own resources. It talks to a database and never to the cluster, so it needs no account at all. The API pod gets a cluster token only in sandbox mode, the pods roll when the computer template changes, the Sandbox asks for a Service so it has an address that survives a resume, and the cluster CA is actually used when talking to the API server. * Tell one run of a computer from the next across a suspend A resumed browser counts snapshot generations from one again, so a ref the model still holds from before the suspend matches a row nothing has overwritten, and the boundary decides about an element on a page that no longer exists. The first answer used the node and the pod address. Resuming a real computer on EKS disproved it: a suspended sandbox is very often rescheduled onto the same node and handed the same address back, and both were identical across the cycle, so the check would have said same run for the exact case it exists to catch. The Ready condition's transition time moves whenever a computer starts serving again, needs no permission beyond the sandbox already read, and is precisely the question. Driven on EKS: a ref taken before a suspend is refused after the resume, naming why, and a fresh ref from a new snapshot clicks through. * Let the policy reach the computers, and refuse one that fences off the database The NetworkPolicy allowed DNS and the bundled database. Nothing let the API reach a Bot's computer, which it does for every browser action, and nothing let it reach a managed database, whose address this chart cannot know. On a cluster that enforces policy both are outages that read as something else: the API looks broken rather than fenced. The computers and the API server are allowed now, and turning the policy on with an external database and no rule for it is refused with the shape of the rule to add. None of this showed up by installing it, because EKS runs its CNI with --enable-network-policy=false and the policy is inert there. That is worth knowing on its own, so it is written down: a policy that installs, looks right, and does nothing is worse than one that is off. Also driven on EKS: reset takes the volumes with it and the Bot gets a clean profile afterwards, and the HPA reads real metrics. * Keep the browsing that produced an answer Every turn in which a Bot used a tool vanished from the transcript on reload. The sentence the Bot wrote stayed, the browsing that produced it did not, the inline screen went with it, and the footer said some messages could not be read. The history store writes a tool call as {id, name, args}; AG-UI describes {id, type: function, function: {name, arguments}}. The reader validated against the second and treated the first as damage from an interrupted run. It is not damage, it is how every tool call is stored, so a guard written against one bad turn was deleting all the real ones. Found by driving a real conversation on the EKS deployment rather than by reading: two browsing turns, both counted unreadable, both well formed in the store's own dialect. Both spellings now read as the same thing. A mixed or unrecognised array is still refused rather than half-translated, because a reader that rewrites what it does not recognise is worse than one that refuses it. * Show the page a finished turn opened, not the one open now Reopening a conversation made every past turn fetch the screen as it is now, so an answer about Hacker News from an hour ago sat under a picture of whatever the Bot had open since. The frame was live and the caption was not, and the turn read as though it had browsed somewhere it never went. A turn that has finished is history, and history is not polled. It names the page that turn actually left open, which the tool result already carried. Nothing changes while a turn runs: those frames are its own and freeze where it left them. It names the page rather than showing it, because nothing stored the picture and fetching one now would show a different page. Naming it stays true however many times the Bot has browsed since. Driven on EKS: three turns, three different pages, each holding its own across a reload. * Keep the frame a browsing turn ended on Reopening a conversation made every past turn fetch the screen as it is now, so an answer about one page sat under a picture of whatever the Bot had open since. A browsing turn keeps its last frame in computer_turn_frame, filed under the tool call and written once, because a turn that has happened does not happen differently later. Three things had to be true together and each was wrong on its own first. The frame is read at the moment the turn ends, since a short turn finishes before the tile has polled anything. Restoring a kept frame must not make the turn look live again, which the first version did: it counted a turn as history only while it had no picture, so restoring one restarted the polling that then replaced it. And a turn is over when it has a result rather than when its status says so, because a restored tool call arrives with its result in hand and a status that is briefly something else. Found by watching the network on the deployed cluster rather than by reading: two live screenshot reads before the restore, on every reload. * Keep a turn's frame only when it is a frame of that turn's page The capture ran at the end of a turn and took whatever the screen showed then. That is usually right and sometimes badly wrong: the same computer is driven by other conversations, a resumed one starts blank, and a short turn finishes before the tile has polled anything. So an answer about one page could be filed with a picture of another, which is worse than having no picture at all. A frame is now kept only when its own url is the page the turn opened. Unknown counts as no match, because storing on unknown is how the wrong picture gets kept. Also folds the restore and the capture into one effect asked in order: what is stored first, the live screen only if nothing is. Two effects racing is what made a reopened turn restore the right frame and then overwrite it with a fresh screenshot one render later, which the console showed plainly once I stopped guessing and logged it. * Photograph the page where it is opened, not where it is read back The transcript's inline screen used to capture its own frame after the turn ended, and file it under the tool call. That is a race it cannot win. A reopened turn and one that has just finished look identical from inside the component, the same computer is driven by other conversations in between, and a resumed computer starts blank, so the picture filed was routinely of somewhere the turn never went, or of nothing. The frame is now taken on the server the moment a navigation succeeds, which is the one moment the screen is certainly showing…
1 parent 50949d6 commit c6ed702

6 files changed

Lines changed: 188 additions & 2 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,22 @@ to be able to say what it was doing.
5050

5151
Nothing to configure. A Bot that acts during a takeover gets the refusal it already got for a click,
5252
and the trail records the attempt and the failure the same way.
53+
### A computer that was suspended once suspends again
54+
55+
Scale-to-zero worked once per Bot. A computer suspended, resumed, used and then left alone again was
56+
offered for suspension on every sweep after that and never suspended, and stayed awake until the next
57+
day. Nothing reported it, because a sweep that offers work and suspends nothing looks exactly like a
58+
fleet that is busy.
59+
60+
The queue keys a suspension on the Bot id and keeps the finished row so that a late offer of the same
61+
key collides with it rather than running the work twice. Both are right. What was wrong is that the
62+
finished row was kept for a day, which is the window the other half of the sweep needs: a suspension
63+
that keeps failing is held back that long before anything tries it again. One number could not be
64+
both, so there are now two, and a finished suspension is kept for the idle window instead. That is
65+
the same clock the offer runs on, so a Bot cannot come back round as idle until its row has gone.
66+
67+
Nothing to configure, and the sweep already runs on a schedule. A deployment where each Bot has its
68+
own computer stops paying for browsers that were used once.
5369

5470
### A finished turn shows the page it opened, not the one open now
5571

‎server/scripts/cull-idle-computers.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,20 @@ try {
7171
const purged = await queue.purge({
7272
kind: CULL_KIND,
7373
olderThanMs: 24 * 60 * 60 * 1000,
74+
/*
75+
* A FINISHED SUSPENSION IS KEPT FOR THE IDLE WINDOW, NOT FOR A DAY.
76+
*
77+
* The key is the Bot id, so the row left behind by a suspension is what the next one collides
78+
* with: a computer resumed, used, and left alone again was offered every sweep and swallowed
79+
* every time, and stayed awake until the row aged out a day later. Nobody saw it, because a
80+
* sweep that offers work and suspends nothing looks exactly like a fleet that is busy.
81+
*
82+
* The idle window is the right length because it is the same clock the offer runs on: a Bot
83+
* cannot qualify as idle again until this long after its last action, by which point its row has
84+
* gone. A day is still right for the other half, where the window is the backoff before a
85+
* suspension that keeps failing is tried again.
86+
*/
87+
finishedOlderThanMs: config.computer.idleAfterMs,
7488
});
7589
/*
7690
* And the screenshots, which had a reaper and nothing calling it.

‎server/src/work/queue.ts‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -87,10 +87,18 @@ export type WorkQueue = {
8787
*
8888
* Both kinds of done: finished, and given up on. An item at its attempt cap is not finished and was
8989
* reaped by nothing, so its key stayed occupied for ever and the work could never be offered again.
90+
*
91+
* TWO KINDS OF DONE, TWO WINDOWS. They are the same length only by coincidence. A finished row is
92+
* kept so a late offer of the same key collides with it, which needs to outlast a sweep; a row that
93+
* gave up is kept because the window is also the backoff before anything tries again, which wants
94+
* to be long. Kept as one number, a queue whose work repeats has to choose which of those to be
95+
* wrong about. `finishedOlderThanMs` defaults to `olderThanMs`, so a caller that has only one
96+
* answer keeps the behaviour it had.
9097
*/
9198
purge: (input: {
9299
kind: string;
93100
olderThanMs: number;
101+
finishedOlderThanMs?: number;
94102
maxAttempts?: number;
95103
}) => Promise<number>;
96104
};
@@ -250,15 +258,21 @@ export function createWorkQueue(database: Database): WorkQueue {
250258
return Boolean(released);
251259
},
252260

253-
async purge({ kind, olderThanMs, maxAttempts = DEFAULT_MAX_ATTEMPTS }) {
261+
async purge({
262+
kind,
263+
olderThanMs,
264+
finishedOlderThanMs = olderThanMs,
265+
maxAttempts = DEFAULT_MAX_ATTEMPTS,
266+
}) {
254267
const cutoff = fromNow(-olderThanMs);
268+
const finishedCutoff = fromNow(-finishedOlderThanMs);
255269
const gone = await database
256270
.delete(workItems)
257271
.where(
258272
and(
259273
eq(workItems.kind, kind),
260274
or(
261-
lt(workItems.finishedAt, cutoff),
275+
lt(workItems.finishedAt, finishedCutoff),
262276
/*
263277
* AND THE ONES THAT GAVE UP, which is the half this forgot.
264278
*

‎server/tests/computer-culler.integration.test.ts‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,4 +219,57 @@ describe("suspending computers nobody is using", () => {
219219
// Each exactly once, which is the point of claiming rather than sweeping.
220220
expect(new Set(stopped).size).toBe(stopped.length);
221221
});
222+
223+
/*
224+
* The second idle window, which is the one that pays for scale-to-zero.
225+
*
226+
* A computer is suspended once and then resumed, used, and left alone again. Every sweep after the
227+
* first was offering work that the finished row silently swallowed, so the Bot stayed awake until
228+
* that row aged out a day later. The queue is right to keep the row: it is what stops the same key
229+
* running twice. It is the retention window for a finished suspension that has to be the idle
230+
* window rather than a day, which is what the sweep below passes.
231+
*/
232+
test("a computer used again after it was suspended is suspended again", async () => {
233+
const botId = botOf("recycled");
234+
const { provider, stopped } = providerWith([
235+
{ botId, status: "running", url: "http://c" },
236+
]);
237+
const at = (iso: string) => () => new Date(iso);
238+
const sweep = async (whenIso: string) => {
239+
const options = {
240+
database,
241+
queue,
242+
provider,
243+
idleAfterMs,
244+
owner: "replica-1",
245+
now: at(whenIso),
246+
};
247+
await offerIdleComputers(options);
248+
const report = await suspendClaimedComputers(options);
249+
/*
250+
* What the CronJob does at the end of every sweep. Zero here rather than the idle window
251+
* because these rows are finished seconds ago in real time and `purge` reads the database's
252+
* clock, not this test's: the window being separate from the give-up one is the property
253+
* under test, not its length.
254+
*/
255+
await queue.purge({
256+
kind: CULL_KIND,
257+
olderThanMs: 24 * 60 * 60_000,
258+
finishedOlderThanMs: 0,
259+
});
260+
return report;
261+
};
262+
263+
await database.insert(auditEvents).values(ran(botId, minutesAgo(60)));
264+
expect((await sweep("2026-08-24T12:00:00Z")).suspended).toEqual([botId]);
265+
266+
// Somebody comes back at 13:00, and it is quiet again by 14:00.
267+
await database
268+
.insert(auditEvents)
269+
.values(ran(botId, new Date("2026-08-24T13:00:00Z")));
270+
const second = await sweep("2026-08-24T14:00:00Z");
271+
272+
expect(second.suspended).toEqual([botId]);
273+
expect(stopped).toEqual([botId, botId]);
274+
});
222275
});
Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
import { expect, test } from "bun:test";
2+
import { readFileSync } from "node:fs";
3+
import { join } from "node:path";
4+
5+
/**
6+
* What the sweep passes, asserted against the script's own source.
7+
*
8+
* The script cannot be imported to be tested: it opens a database and a computer provider at the top
9+
* level and runs a sweep as a side effect of loading. So the retention window it hands `purge` is the
10+
* one line in this fix that nothing executes, and every test around it passes the window itself,
11+
* which means they would all still be green with this line deleted.
12+
*
13+
* That is the failure this whole change is about, one level up: a value the caller never names
14+
* reaches nothing, and the sweep goes on looking like it worked. Read as text, the way
15+
* `tests/compose.test.ts` pins the variables Compose has to name, because the alternative is a fix
16+
* whose only load-bearing line is the untested one.
17+
*
18+
* WHAT THIS CANNOT SEE. It proves the argument is written and where its value comes from, not that
19+
* the sweep behaves. `computer-culler.integration.test.ts` owns the behaviour and runs the real
20+
* queue against a real database; this is only the wire between the two.
21+
*/
22+
const script = readFileSync(
23+
join(import.meta.dir, "..", "scripts", "cull-idle-computers.ts"),
24+
"utf8",
25+
);
26+
27+
test("the cull sweep keeps a finished suspension for the configured idle window", () => {
28+
expect(script).toContain("finishedOlderThanMs: config.computer.idleAfterMs");
29+
});
30+
31+
/**
32+
* And that the other half still gets its day.
33+
*
34+
* The two windows exist because they are different questions. A sweep that passed the idle window for
35+
* both would turn a suspension that keeps failing into one retried every few minutes, which is the
36+
* regression this fix could most easily cause and the one nothing else would report.
37+
*/
38+
test("and gives a suspension that gave up the longer window before it is tried again", () => {
39+
expect(script).toContain("olderThanMs: 24 * 60 * 60 * 1000");
40+
});

‎server/tests/work-queue.integration.test.ts‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -323,4 +323,53 @@ describe("claiming durable work", () => {
323323
expect(row?.attempts).toBe(3);
324324
expect(row?.why).toBe("the cluster said no");
325325
});
326+
327+
/*
328+
* The two kinds of done, on their own clocks.
329+
*
330+
* They were one number, which a queue whose work repeats cannot afford: a finished row only has to
331+
* outlast a sweep, and a row that gave up is kept because that window is also the backoff before
332+
* anything tries the work again. Sharing it meant the culler either wedged its own key for a day
333+
* or retried a broken suspension every few minutes.
334+
*/
335+
test("a finished row and one that gave up are swept on separate windows", async () => {
336+
await queue.offer({ kind, key: "done" });
337+
await queue.claim({ kind, owner: "replica-1", leaseMs: 30_000 });
338+
await queue.finish({ kind, key: "done", owner: "replica-1" });
339+
340+
await queue.offer({ kind, key: "gave-up" });
341+
for (let attempt = 0; attempt < 3; attempt += 1) {
342+
await queue.claim({
343+
kind,
344+
owner: "replica-1",
345+
leaseMs: 30_000,
346+
maxAttempts: 3,
347+
});
348+
await queue.release({
349+
kind,
350+
key: "gave-up",
351+
owner: "replica-1",
352+
delayMs: 0,
353+
reason: "the cluster said no",
354+
});
355+
}
356+
357+
// The finished one goes on its own short window; the one that gave up keeps its long one.
358+
expect(
359+
await queue.purge({
360+
kind,
361+
olderThanMs: 60_000,
362+
finishedOlderThanMs: 0,
363+
maxAttempts: 3,
364+
}),
365+
).toBe(1);
366+
const [left] = await database
367+
.select({ key: workItems.key })
368+
.from(workItems)
369+
.where(eq(workItems.kind, kind));
370+
expect(left?.key).toBe("gave-up");
371+
372+
// And omitting it leaves both halves on the one window, which is what every other caller does.
373+
expect(await queue.purge({ kind, olderThanMs: 0, maxAttempts: 3 })).toBe(1);
374+
});
326375
});

0 commit comments

Comments
 (0)