Skip to content

fix: stop shipping one institution's config as the fork default - #1205

Merged
philmerrell merged 2 commits into
developfrom
feature/fork-neutral-defaults
Sep 20, 2026
Merged

philmerrell merged 2 commits into
developfrom
feature/fork-neutral-defaults

Conversation

@philmerrell

Copy link
Copy Markdown
Contributor

Two defaults that a fork of this stack inherits and cannot discover. Both were found while auditing flag defaults ahead of the 1.23.0 release; neither is a new regression, and both are one-line-ish fixes with a guard so they cannot come back.

1. The browser URL blocklist was hardcoded, and the env var was never forwarded

The blocklist is the control that stops a human in a browser takeover reaching a system the agent must not act inside. config.ts seeded it with instructure.com, and platform.yml never forwarded CDK_BROWSER_URL_BLOCKLIST — so through the deploy pipeline the list was effectively unconfigurable, and every fork inherited BSU's LMS policy with no breadcrumb.

The hosts that matter are a per-deployment policy call, not a property of this stack, so the default is now empty and the value is supplied per environment — the same shape as domainName and corsOrigins. The operational guidance stays in the comment (match on HOST, prefer the registrable domain so .test./.beta. instances are covered, don't block a vendor's sign-in page).

The empty-string trap is the reason for parseListEnv. An unset GitHub Actions variable arrives as ''. The previous !== undefined check would have read that as a deliberate "block nothing" the moment the workflow started forwarding it, silently overriding a configured default. Empty now means "not configured" and falls through to context, consistent with every other variable in the file.

And because the list now comes entirely from outside the repo, a deploy that ships an empty one has to say so. The synth log prints the list, or warns that RBAC on browse_web / request_user_login is the only remaining control.

CDK_BROWSER_URL_BLOCKLIST=instructure.com is already set on both the production and development environments, so neither loses the control. Prod is covered either way until the release merges — it still gets the hardcoded default from main.

2. Two catalogued tools were never seeded

seed_bootstrap_data.py is the only writer of the TOOL#<id> rows that RBAC grants and the tool picker read, and nothing syncs it with the Python TOOL_CATALOG. list_spreadsheets and analyze_spreadsheet were catalogued but never seeded, so a freshly bootstrapped deployment got no rows and the tools could not be granted to any role — silently, because an absent row reads as "not in the catalog" rather than as an error. Our own environments only have them because the rows were created by hand.

Values mirror the live deployments (category: data, enabledByDefault: true, isPublic: true). Default-on is safe against the injected-tool cost trap because SPREADSHEET_TOOL_IDS is in KEY_DESCRIBED_INJECTED_TOOL_IDS — the factory closes over (session_id, user_id, assistant_id), all cache-key elements, so it does not force an agent-cache bypass. Seeding skips existing rows, so this cannot touch a deployed catalog.

test_seed_matches_tool_catalog pins the two lists together. It is one-directional — seeded-without-catalog is legitimate for context-bound tools — and document_read is excluded by name, because it is gated on the session having an attachment rather than on enabled_tools and must never get a row.

Verification

  • tsc --noEmit clean; 881/881 infra jest tests pass.
  • Backend 240 passed on the seed or tool_catalog or injected or tool_filter subset. One error in test_pbt_model_access_preservation.py is pre-existing (the real-AWS-call class) and reproduces on a clean tree.
  • The parity test is not vacuous: run against the v1.22.0 seeder it fails with exactly ['analyze_spreadsheet', 'list_spreadsheets', 'request_user_login'], so it would have caught both this gap and request_user_login before it was seeded.
  • The blocklist tests pin that '' and ' , , ' fall through to context rather than emptying the list.

On merge

This auto-deploys dev, and that is the deploy where dev's blocklist switches from the hardcoded default to the variable. Check the platform deploy log for:

   Browser URL blocklist (1): instructure.com

If the ⚠️ empty warning prints instead, the variable did not reach the synth.

🤖 Generated with Claude Code

philmerrell and others added 2 commits September 20, 2026 14:10
…environment

The blocklist is the control that stops a human in a browser takeover reaching
a system the agent must not act inside. It was hardcoded to `instructure.com`
in config.ts, and `platform.yml` never forwarded CDK_BROWSER_URL_BLOCKLIST — so
through the deploy pipeline the list was effectively unconfigurable, and every
fork of this stack inherited one institution's LMS policy with no breadcrumb.

The hosts that matter are a per-deployment policy call, not a property of this
stack, so the default is now empty and the value is supplied per environment,
following the `domainName` / `corsOrigins` convention. The operational guidance
stays in the comment: Chromium matches on HOST, prefer the registrable domain
so `.test.`/`.beta.` instances are covered, and don't block a vendor's sign-in
page — reaching a login page is what a VPAT review exists to do.

New `parseListEnv` treats empty as unset and falls through to context. An unset
GitHub Actions variable arrives as '', and the previous `!== undefined` check
would have read that as a deliberate "block nothing" the moment the workflow
started forwarding it — silently overriding a configured default.

Because the list now comes entirely from outside the repo, a deploy that ships
an empty one has to say so: the synth log prints the list, or warns that RBAC
on browse_web / request_user_login is the only remaining control.

The CDK_BROWSER_URL_BLOCKLIST variable is set to `instructure.com` on both the
production and development environments, so neither loses the control.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`seed_bootstrap_data.py` is the only writer of the `TOOL#<id>` rows that RBAC
grants and the tool picker read, and nothing syncs it with the Python
TOOL_CATALOG. `list_spreadsheets` and `analyze_spreadsheet` were catalogued but
never seeded, so a freshly bootstrapped deployment got no rows and the tools
could not be granted to any role — silently, because an absent row reads as
"not in the catalog" rather than as an error. The live environments only have
them because the rows were created by hand.

Values mirror the live deployments: category `data`, enabledByDefault true,
isPublic true. Default-on is safe against the injected-tool cost trap because
SPREADSHEET_TOOL_IDS is in KEY_DESCRIBED_INJECTED_TOOL_IDS — the factory closes
over (session_id, user_id, assistant_id), all cache-key elements, so it does
not force an agent-cache bypass. Seeding skips existing rows, so this cannot
touch a deployed catalog.

test_seed_matches_tool_catalog pins the two lists together. It is one-
directional: seeded-without-catalog is legitimate for context-bound tools, and
`document_read` is excluded by name because it is gated on the session having
an attachment rather than on enabled_tools, so it must never get a row. Run
against the v1.22.0 seeder it fails with exactly the three ids that were
missing, so it is not vacuous.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@philmerrell
philmerrell merged commit 56afff4 into develop Sep 20, 2026
6 checks passed
@philmerrell
philmerrell deleted the feature/fork-neutral-defaults branch September 20, 2026 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant