diff --git a/.github/workflows/platform.yml b/.github/workflows/platform.yml index 745b7ca3..5d80a24d 100644 --- a/.github/workflows/platform.yml +++ b/.github/workflows/platform.yml @@ -99,14 +99,13 @@ jobs: # http://localhost:4200 for a local SPA pointed at this deployment. # Empty on prod. Mirrors CDK_MCP_SANDBOX_EXTRA_FRAME_ANCESTORS below. CDK_ARTIFACTS_EXTRA_FRAME_ANCESTORS: ${{ vars.CDK_ARTIFACTS_EXTRA_FRAME_ANCESTORS }} - # "Shared with you" inbox on the artifact library. Default OFF, - # opt-in — the reverse of the flags above, because the surface ships - # ahead of the product decision about it. Unset resolves to an empty - # string, which config.ts treats as off; set the - # `CDK_ARTIFACT_SHARE_INBOX_ENABLED` variable to "true" in an - # environment to reveal it there. The fan-out rows the inbox reads are - # written regardless of this flag, so turning it on shows a complete - # inbox with no backfill. + # "Shared with you" inbox on the artifact library. Default ON with a + # kill switch, like the flags above: unset resolves to an empty string, + # which config.ts treats as "use the default (on)". Set the + # `CDK_ARTIFACT_SHARE_INBOX_ENABLED` variable to "false" in an + # environment to keep it dark there. The fan-out rows the inbox reads + # are written regardless of this flag, so toggling it never needs a + # backfill. CDK_ARTIFACT_SHARE_INBOX_ENABLED: ${{ vars.CDK_ARTIFACT_SHARE_INBOX_ENABLED }} CDK_FRONTEND_CERTIFICATE_ARN: ${{ vars.CDK_FRONTEND_CERTIFICATE_ARN }} # MCP Apps sandbox-proxy origin (mcp-sandbox.{domain}). Without the diff --git a/backend/src/apis/shared/feature_flags.py b/backend/src/apis/shared/feature_flags.py index 5681a5e6..d9e8e974 100644 --- a/backend/src/apis/shared/feature_flags.py +++ b/backend/src/apis/shared/feature_flags.py @@ -197,13 +197,23 @@ def artifact_share_inbox_enabled() -> bool: """Whether a recipient can *discover* artifacts shared with them. Covers the ``GET /shared-artifacts`` inbox endpoint and, through it, - the library page's "Shared with you" tab. **Default OFF, opt-in** - (the deferred-feature pattern, mirroring the long-deleted - ``FINE_TUNING_ENABLED``): only the literal ``"true"`` - (case-insensitive) enables it. Every other flag in this module ships - default-on with a kill switch; this one is deliberately the other - way round, because the surface it gates lands before the product - decision about it does. + the library page's "Shared with you" tab. **Default ON with a kill + switch** (house style, mirroring ``announcements_enabled`` and + ``scheduled_runs_enabled``): unset or empty resolves to enabled; only + the literal ``"false"`` (case-insensitive) disables. + + It shipped the other way round — default off, opt-in — because the + surface landed before the product decision about it did. That + decision was made in 1.18.0 and the inbox went live; carrying an + opt-in default past it would mean every institution forking this + repo silently loses a finished feature, and has to discover a + variable to get it back. Default-on is the right answer for a fork, + and ``"false"`` still turns it off for anyone who wants it dark. + + Note the empty-string case is load-bearing in the *opposite* + direction now: an unset GitHub Actions variable forwards ``""``, + which under this flag means **on**. That is deliberate — a fork that + never sets the variable is exactly who this default is for. ############################################################ # This flag gates the READ ONLY. The recipient fan-out rows the @@ -222,5 +232,5 @@ def artifact_share_inbox_enabled() -> bool: """ return ( os.environ.get("ARTIFACT_SHARE_INBOX_ENABLED", "").strip().lower() - == "true" + != "false" ) diff --git a/backend/tests/apis/app_api/artifacts/test_artifact_share_inbox.py b/backend/tests/apis/app_api/artifacts/test_artifact_share_inbox.py index 47a350c0..87cbf5de 100644 --- a/backend/tests/apis/app_api/artifacts/test_artifact_share_inbox.py +++ b/backend/tests/apis/app_api/artifacts/test_artifact_share_inbox.py @@ -464,13 +464,29 @@ def test_inbox_404s_while_the_flag_is_off( env, monkeypatch: pytest.MonkeyPatch ) -> None: make_client, ddb = env - monkeypatch.delenv("ARTIFACT_SHARE_INBOX_ENABLED", raising=False) + monkeypatch.setenv("ARTIFACT_SHARE_INBOX_ENABLED", "false") _put_version(ddb) _share_with(make_client(), [FRIEND_EMAIL]) assert make_client(_friend()).get("/shared-artifacts").status_code == 404 +def test_inbox_is_on_when_the_flag_is_unset( + env, monkeypatch: pytest.MonkeyPatch +) -> None: + """The point of the default-on flip. + + A fork that never sets ``CDK_ARTIFACT_SHARE_INBOX_ENABLED`` should get + the finished feature, not lose it silently and have to discover a + variable to get it back.""" + make_client, ddb = env + monkeypatch.delenv("ARTIFACT_SHARE_INBOX_ENABLED", raising=False) + _put_version(ddb) + _share_with(make_client(), [FRIEND_EMAIL]) + + assert len(_inbox(make_client(_friend()))["artifacts"]) == 1 + + def test_fan_out_rows_are_written_while_the_flag_is_off( env, monkeypatch: pytest.MonkeyPatch ) -> None: @@ -492,22 +508,25 @@ def test_fan_out_rows_are_written_while_the_flag_is_off( assert len(_inbox(make_client(_friend()))["artifacts"]) == 1 -def test_only_the_literal_true_enables_the_inbox( +def test_only_the_literal_false_disables_the_inbox( env, monkeypatch: pytest.MonkeyPatch ) -> None: - """Opt-in, not a kill switch. An unset GitHub Actions variable - forwards an empty string, which must resolve to off rather than - revealing the surface by accident.""" + """A kill switch, not opt-in — and the empty string is the case that + matters. An unset GitHub Actions variable forwards ``""``, which must + resolve to ON. Getting this backwards is how a default-on flag ships + silently disabled to every fork that never sets the variable.""" make_client, ddb = env _put_version(ddb) _share_with(make_client(), [FRIEND_EMAIL]) friend = make_client(_friend()) - for value in ("", " ", "false", "1", "yes", "TRUE!"): + for value in ("false", "FALSE", " False "): monkeypatch.setenv("ARTIFACT_SHARE_INBOX_ENABLED", value) assert friend.get("/shared-artifacts").status_code == 404, value - for value in ("true", "TRUE", " True "): + # Everything else — including the empty string, and including junk — + # leaves the feature on. A typo must not silently disable a surface. + for value in ("", " ", "true", "TRUE", " True ", "1", "yes", "FALSE!"): monkeypatch.setenv("ARTIFACT_SHARE_INBOX_ENABLED", value) assert friend.get("/shared-artifacts").status_code == 200, value diff --git a/docs-site/src/content/docs/features/artifacts.md b/docs-site/src/content/docs/features/artifacts.md index c54448a2..37f0164a 100644 --- a/docs-site/src/content/docs/features/artifacts.md +++ b/docs-site/src/content/docs/features/artifacts.md @@ -173,18 +173,22 @@ consumed by minting a render token, so it cannot be useful without artifacts being on. The "Shared with you" inbox has a flag of its own: `ARTIFACT_SHARE_INBOX_ENABLED` -(CDK: `CDK_ARTIFACT_SHARE_INBOX_ENABLED`). Unlike the kill-switch flags elsewhere -in the platform it is **default off and opt-in** — only the literal `"true"` -enables it — because the surface shipped ahead of the product decision about it. -While off, `GET /shared-artifacts` 404s and the SPA renders the library without -tabs. +(CDK: `CDK_ARTIFACT_SHARE_INBOX_ENABLED`). Like the other flags in the platform +it is **default on with a kill switch** — only the literal `"false"` disables it, +and an unset variable resolves to on. While off, `GET /shared-artifacts` 404s and +the SPA renders the library without tabs. + +It shipped default-off and opt-in in 1.18.0, because the surface landed ahead of +the product decision about it. That decision was made and the inbox went live, so +the default flipped: a deployment that never sets the variable should get the +finished feature rather than silently lose it. The flag gates the **read only**. Fan-out rows are written by every share regardless of it. That asymmetry is deliberate: if the writes were gated too, turning the flag on would reveal an inbox missing every share created while it was off — a wrong answer rather than an empty one, and one nobody could see was -wrong. Writing the rows regardless makes the flip complete and instant, with no -backfill to sequence. +wrong. Writing the rows regardless makes the toggle complete and instant in +either direction, with no backfill to sequence. ## Artifacts inside a shared conversation diff --git a/infrastructure/lib/config.ts b/infrastructure/lib/config.ts index 9a875560..44fd6745 100644 --- a/infrastructure/lib/config.ts +++ b/infrastructure/lib/config.ts @@ -106,10 +106,9 @@ export interface ArtifactsConfig { extraFrameAncestors: string[]; // Whether recipients can *discover* artifacts shared with them (the // library's "Shared with you" tab, backed by GET /shared-artifacts). - // Default OFF and opt-in, unlike the kill-switch flags elsewhere in this - // file: the surface lands before the product decision about it. Gates the - // read only — the fan-out rows behind it are written unconditionally, so - // enabling this never needs a backfill. + // Default ON with a kill switch, like the other flags in this file. + // Gates the read only — the fan-out rows behind it are written + // unconditionally, so toggling this never needs a backfill. shareInboxEnabled: boolean; } @@ -821,14 +820,18 @@ export function loadConfig(scope: cdk.App): AppConfig { .map((s) => s.trim()).filter(Boolean) || scope.node.tryGetContext('artifacts')?.extraFrameAncestors || [], - // Default OFF, opt-in — the inverse of the kill-switch ternary used by - // scheduledRuns/memorySpaces/skills above. Only the literal "true" - // enables, so the empty string an unset GitHub Actions variable - // forwards resolves to off, which is the intended default rather than - // an accident. + // Default ON with a kill switch: the recipient inbox is a complete + // feature and ships enabled for every deployer (opt-out, not opt-in). + // It shipped opt-in in 1.18.0 because the surface landed ahead of the + // product decision; that decision is made, so a fork should get the + // finished feature without having to discover a variable. + // The workflow forwards `${{ vars.CDK_ARTIFACT_SHARE_INBOX_ENABLED }}`, + // which is an EMPTY STRING when the variable is unset — so treat + // empty/unset as "use the default (on)" and only the literal "false" + // as the off switch. (Same ternary as scheduledRuns above — keep in sync.) shareInboxEnabled: process.env.CDK_ARTIFACT_SHARE_INBOX_ENABLED - ? process.env.CDK_ARTIFACT_SHARE_INBOX_ENABLED === 'true' - : scope.node.tryGetContext('artifacts')?.shareInboxEnabled ?? false, + ? process.env.CDK_ARTIFACT_SHARE_INBOX_ENABLED !== 'false' + : scope.node.tryGetContext('artifacts')?.shareInboxEnabled ?? true, }, mcpSandbox: { certificateArn: process.env.CDK_MCP_SANDBOX_CERTIFICATE_ARN || scope.node.tryGetContext('mcpSandbox')?.certificateArn, diff --git a/infrastructure/lib/constructs/app-api/app-api-service-construct.ts b/infrastructure/lib/constructs/app-api/app-api-service-construct.ts index f142d11d..e681a25a 100644 --- a/infrastructure/lib/constructs/app-api/app-api-service-construct.ts +++ b/infrastructure/lib/constructs/app-api/app-api-service-construct.ts @@ -182,11 +182,11 @@ export class AppApiServiceConstruct extends Construct { environment['DYNAMODB_ARTIFACTS_TABLE_NAME'] = props.refs.artifactsTable.tableName; environment['ARTIFACTS_ORIGIN'] = props.artifactsOrigin; environment['ARTIFACTS_RENDER_TOKEN_SECRET_ARN'] = props.refs.artifactRenderTokenSecret.secretArn; - // "Shared with you" inbox. Default off; read by + // "Shared with you" inbox. Default on with a kill switch; read by // apis/shared/feature_flags.py::artifact_share_inbox_enabled, which gates // the GET /shared-artifacts route ONLY. The recipient fan-out rows are - // written by every share regardless, so flipping this on reveals a - // complete inbox rather than one that begins at the flip. + // written by every share regardless, so toggling this never reveals an + // inbox that begins at the flip. environment['ARTIFACT_SHARE_INBOX_ENABLED'] = config.artifacts.shareInboxEnabled ? 'true' : 'false'; // Skill reference-file bucket (admin-managed Skills, PR-4). Read by diff --git a/infrastructure/test/config.test.ts b/infrastructure/test/config.test.ts index 99c00c78..784933c7 100644 --- a/infrastructure/test/config.test.ts +++ b/infrastructure/test/config.test.ts @@ -455,6 +455,49 @@ describe('RAG Ingestion Configuration', () => { }); }); + // ============================================================ + // Artifact share inbox feature flag — default ON with a kill switch + // (flipped from opt-in once the surface shipped; the empty workflow + // var is the case that matters — see the 1.18.0 release notes) + // ============================================================ + + describe('Artifact share inbox feature flag', () => { + test('defaults to enabled when CDK_ARTIFACT_SHARE_INBOX_ENABLED is unset', () => { + delete process.env.CDK_ARTIFACT_SHARE_INBOX_ENABLED; + + expect(loadConfig(app).artifacts.shareInboxEnabled).toBe(true); + }); + + test('treats empty string (unset GitHub Actions variable) as enabled', () => { + // `${{ vars.CDK_ARTIFACT_SHARE_INBOX_ENABLED }}` renders to "" when + // unset. Under the previous opt-in default this resolved to OFF; the + // whole point of the flip is that a fork which never sets the variable + // now gets the finished feature. + process.env.CDK_ARTIFACT_SHARE_INBOX_ENABLED = ''; + + expect(loadConfig(app).artifacts.shareInboxEnabled).toBe(true); + }); + + test('CDK_ARTIFACT_SHARE_INBOX_ENABLED="false" is the kill switch', () => { + process.env.CDK_ARTIFACT_SHARE_INBOX_ENABLED = 'false'; + + expect(loadConfig(app).artifacts.shareInboxEnabled).toBe(false); + }); + + test('CDK_ARTIFACT_SHARE_INBOX_ENABLED="true" stays enabled', () => { + process.env.CDK_ARTIFACT_SHARE_INBOX_ENABLED = 'true'; + + expect(loadConfig(app).artifacts.shareInboxEnabled).toBe(true); + }); + + test('cdk.json context artifacts.shareInboxEnabled=false disables when env is unset', () => { + delete process.env.CDK_ARTIFACT_SHARE_INBOX_ENABLED; + app.node.setContext('artifacts', { shareInboxEnabled: false }); + + expect(loadConfig(app).artifacts.shareInboxEnabled).toBe(false); + }); + }); + // ============================================================ // Memory Spaces feature flag — default ON with a kill switch // (complete feature; ships enabled for forkers, empty var must not disable)