Skip to content

feat(announcements): data layer, admin CRUD, and table (PR-1) - #966

Merged
philmerrell merged 1 commit into
developfrom
feature/announcements-data-layer
Sep 5, 2026
Merged

philmerrell merged 1 commit into
developfrom
feature/announcements-data-layer

Conversation

@philmerrell

Copy link
Copy Markdown
Contributor

PR-1 of docs/specs/feature-announcements.md — the storage layer, the admin authoring API, the delegated admin scope, and the DynamoDB table.

This ships dark

There is no user-facing surface in this PR. Admins can author drafts; users see nothing. GET /announcements, the ack endpoint, and the What's-New panel land in PR-2.

⚠️ Deploy order

This PR adds a CDK table, so platform.yml must deploy before backend.yml or app-api will have no DYNAMODB_ANNOUNCEMENTS_TABLE_NAME and no IAM grant. That failure mode is benign — the repository logs a warning and disables itself rather than crashing (the same posture AuditService takes for the same reason), so a backend-first deploy degrades to "the admin API returns empty" rather than taking anything down. But the admin surface is not usable until the platform deploy lands.

No GSI, so none of the GSI deploy-ordering hazards apply.

What's here

Storage — backend/src/apis/shared/announcements/{models,repository,service}.py

  • Announcement items on the fixed ANNOUNCEMENTS partition, SK: ANNOUNCEMENT#<uuid> — the same shape user_menu_links uses.
  • Acknowledgement items under PK: USER#<user_id>, SK: ACK#<announcementId>#R<revision> — the per-user partition user_settings established, revision-keyed per spec D4.
  • The whole field set is modelled now, including revision / showToNewUsers / requiresAck / targetRoles that later PRs consume. The table is the expensive thing to change; the routes are not.

Admin API — POST/GET/PATCH/DELETE /admin/announcements plus /publish, /archive, /revise. Guarded package-wide by the new delegable admin.announcements scope, in both the backend registry and the duplicated SPA literal union. (/stats is deliberately PR-6.)

Feature flag — ANNOUNCEMENTS_ENABLED, default ON with a kill switch (!= "false"), matching skills_enabled / scheduled_runs_enabled. Explicitly false unmounts the admin router so the surface 404s.

CDK — AnnouncementsTable beside UserMenuLinksTable with timeToLiveAttribute: 'ttl', SSM param, PlatformComputeRefs threading, DYNAMODB_ANNOUNCEMENTS_TABLE_NAME, an IAM grant, DDB throttle-alarm coverage, and the backup/restore table lists.

Three things worth reading closely

The ack write is monotonic at the database (spec D2). record_ack is a conditional UpdateExpression guarded by attribute_not_exists(actionRank) OR actionRank < :rank, and a ConditionalCheckFailedException is swallowed as success. seen is written the moment a surface renders, so it races the user's click on the ✕; without the guard the late seen overwrites dismissed and the banner comes back on the next load. This is the same failure class as #741 and #751 — per-user state moving backwards — so it gets the same discipline: the guard is in the condition expression, not in application ordering. The write path is built here even though PR-2 is what calls it, and TestMonotonicAck is the first class in the test file.

targetRoles is a display filter, not an RBAC grant (spec D9). CLAUDE.md's rule that a role list on a resource must be written through to each AppRole.granted* governs tools, models, and skills — things with a can_access_* predicate behind them. Announcement visibility confers no capability and inherits nothing, so the list lives only on the announcement item and apis/shared/rbac/ is untouched beyond adding the scope. There's a comment on the field saying so, and a test that pins it, because "fixing" this into the role service would put display metadata into the access-decision path.

One thing not in the spec, added here. state is absent from the AnnouncementUpdate body. If PATCH accepted it, an archived announcement could be put back in front of every user by a request that looks like an ordinary body edit, and the /publish guard would be decorative. For the same reason create accepts only draft / scheduled — going live is its own call. Both are covered by tests. Flagging it as a deviation in case you'd rather PATCH stay fully general.

Also deferred deliberately

  • Audit logging of announcement writes (spec D10). AuditAction is a closed constant namespace that today names only role actions, and extending it touches the non-delegable admin.audit area. Better as its own change than smuggled into this one — worth doing before the scope is actually delegated to comms staff.
  • config.announcements.enabled CDK threading (spec §8.4). Not wired, matching MID_TURN_STEERING_ENABLED, which also relies on default-ON with no env var. Nothing is needed for the flag to resolve enabled.

Testing

cd backend && uv run python -m pytest tests/ -q

7399 passed, 3 skipped. 70 of those are new, covering:

  • the monotonic-ack regression (seen after dismissed leaves dismissed intact), plus idempotency, user scoping, and that a stronger action still raises the rank;
  • revision keying — ack at R1, bump, the R2 slot is un-acked while the R1 history stays readable;
  • ack TTLs, including that upgrading seen → acknowledged on a requiresAck announcement removes the TTL rather than letting the compliance record expire on the earlier schedule;
  • expiresAt required for banner/modal, enforced on create and on a PATCH whose merged result is invalid;
  • ctaUrl rejecting javascript: at the API layer, on both POST and PATCH;
  • flag off → the admin router is unmounted and the path 404s.

tests/architecture/test_admin_scope_coverage.py picks up the new admin package — verified by running it, not assumed; its EXPECTED_MODULE_SCOPES map needed the new entry, which is the mechanism working as designed.

cd infrastructure && npx tsc --noEmit && npx jest && npx cdk synth

784 infra tests pass, tsc clean, synth clean. Four table-count assertions needed updating (26 → 27 tables, AdminTablesConstruct 3 → 4, the DDB alarm count) and gsi-inventory.json was regenerated — it now records announcements: [], confirming no index was added. Added a construct test asserting the TTL attribute is on the announcements table only, since a TTL that spread to a sibling would silently delete admin-authored content.

cd frontend/ai.client && npm test

205 files / 2234 tests pass — the SPA change is the one-line scope-union addition.

🤖 Generated with Claude Code

…nouncements

PR-1 of docs/specs/feature-announcements.md. Ships dark: admins can author
drafts, users see nothing until PR-2 adds the user-facing surface.

Storage (apis/shared/announcements/): announcement items on the fixed
`ANNOUNCEMENTS` partition, acknowledgement items under `USER#<id>` with
revision-keyed sort keys (`ACK#<id>#R<n>`). The full field set is modelled now,
including the fields later PRs consume — the table is the expensive thing to
change, the routes are not.

The ack write is a conditional UpdateExpression guarded by
`attribute_not_exists(actionRank) OR actionRank < :rank`, and a
ConditionalCheckFailedException is swallowed as success. `seen` is written on
render and races the user's dismiss click; without the guard the late `seen`
clobbers `dismissed` and the banner comes back. Same failure class as #741 /
#751 — per-user state moving backwards — so the guard lives in the database,
not in application ordering.

`targetRoles` is a display filter, not an RBAC grant (spec D9). It is written
only to the announcement item; apis/shared/rbac/ is untouched beyond the new
scope. Commented on the field so it does not get "fixed" into the role service.

Also: `state` is absent from the PATCH body, so the publish/archive state
machine cannot be walked around by a request that looks like a body edit, and
create accepts only `draft` / `scheduled` — going live is its own call.

- admin CRUD at /admin/announcements, guarded package-wide by the new
  delegable `admin.announcements` scope (backend registry + the duplicated
  SPA literal union)
- `ANNOUNCEMENTS_ENABLED`, default ON with a kill switch; the admin router
  unmounts when explicitly false
- CDK: AnnouncementsTable in AdminTablesConstruct with `timeToLiveAttribute`,
  SSM param, PlatformComputeRefs threading, app-api env var + IAM grant, DDB
  alarm coverage, and the backup/restore table lists. No GSI.

Tests: 70 backend cases covering the monotonic-ack regression, revision
keying, expiresAt validation for loud surfaces, `javascript:` rejection at the
API layer, and the flag-off 404. Infra table-count assertions and the GSI
inventory updated.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@philmerrell
philmerrell merged commit 8f8ffa6 into develop Sep 5, 2026
4 checks passed
@philmerrell
philmerrell deleted the feature/announcements-data-layer branch September 5, 2026 19:33
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