Repository navigation
docs(p12): README + ARCHITECTURE polish — close out issues #95 + #96 - #147
Merged
Merged
Conversation
Two things the runbook (and the Dockerfile) were missing, both discovered while doing the first end-to-end CDK + image deploy on top of the merged hardening work. 1. Backend Dockerfile: pre-create /app/local_images writable LocalImageStorage tries to mkdir /app/local_images on first write. WORKDIR /app is owned by root (default 755), and the container drops to the unprivileged `app` user before runtime — so the mkdir failed with PermissionError. The seed Fargate task surfaced this first; the App Runner instance would have hit it on the first live image-gen request. Fix: explicit `mkdir -p /app/local_images` + chown to `app` before USER swap. Goes away when the S3 adapter ships. 2. DEPLOY.md step 6: pass the fargate-egress SG to ecs run-task The VPC default SG has no egress rules (CDK locks it down). Without an explicit `securityGroups=[...]` on the network config, the Fargate task can't reach Secrets Manager and dies with `ResourceInitializationError: ... context deadline exceeded`. The dedicated `magnacms-dev-fargate-egress` SG exists for this; runbook now references it. 3. DEPLOY.md step 8: drop the stale "leave IMAGES_CDN_BASE_URL empty" instruction PR #145 made `CORS_ORIGINS` and `IMAGES_CDN_BASE_URL` CDK- context-driven, so console edits get clobbered on next deploy. Step 8 now points at `cdk deploy magnacms-dev-compute -c ...` as the only supported path, with a paragraph on why the image base URL has to be set (LocalImageStorage concatenates it with the storage key; empty produces 404 paths). 4. DEPLOY.md adds step 6a: optional seed task Documents `python -m app.scripts.seed` as a command override on the migration task definition, plus the known caveat that seeded image bytes don't reach App Runner's filesystem (cross- container; only DB rows survive). The runbook is now honest about the limitation rather than implying seeded thumbnails work end-to-end.
… list tests backend-ci on commit a517f5e (squash of PR #145) failed on test_list_returns_paginated_envelope_newest_first; the same test passed on the PR's last pre-merge run (9ee04d4). Same code, same test — different CI runner timing. Root cause: Postgres `now()` is microsecond-resolution and three sequential mock-LLM API calls on a fast CI runner can all land in the same microsecond. The dashboard repository orders by `created_at DESC, id DESC` (the tiebreaker added in PR #145); when all three timestamps tie, the tiebreaker decides ordering by random UUID byte values rather than insert order. The test asserts strict `[third, second, first]` insert-order ordering, which is only satisfied when the runner happens to produce distinct microseconds. Two options to fix: (a) Add a strictly-monotonic ordering column (BIGSERIAL) to ContentPiece / Improvement. Schema change + migration. Best long-term but big blast radius. (b) Stagger the test creates by 20ms so timestamps are reliably distinct, matching real-world usage (humans don't click microsecond-fast). Going with (b): 20ms × 2 sleeps = 40ms added to each test, vs the intermittent CI red. Two affected tests: tests/integration/test_content_dashboard.py test_list_returns_paginated_envelope_newest_first tests/integration/test_improve_routes.py test_list_returns_newest_first_with_previews The dashboard list contract ("newest-first, deterministic ties broken by UUID DESC") is unchanged; only the test fixture relaxes its timing assumption. When the S3-backed storage adapter ships and a serial column comes with it, these sleeps can go.
The 20ms-sleep approach in 1caa44e didn't work — backend-ci still failed with `[first, third, second]` actual vs `[third, second, first]` expected. Root cause is in tests/integration/conftest.py, not in the test: the fixture wraps every test in ONE outer transaction and binds every request's session to that connection with `join_transaction_mode=create_savepoint`. All requests share a single Postgres transaction, and `now()` / `transaction_timestamp()` is constant within a transaction — so every row a test inserts has the SAME `created_at` regardless of wall-clock gaps between requests. The dashboard repo orders by `created_at DESC, id DESC` (PR #145). With timestamps tied, the tiebreaker (UUID DESC) decides — which is deterministic but obviously doesn't match insert order. Fix: stop asserting insert order. Assert what the contract actually guarantees: 1. All inserted IDs appear in the response (set equality). 2. When timestamps tie, the order is `id DESC` (lexicographic UUID compared as DESC, which is what Postgres `id DESC` produces on a UUID column). Two tests updated: tests/integration/test_content_dashboard.py test_list_returns_paginated_envelope_newest_first tests/integration/test_improve_routes.py test_list_returns_newest_first_with_previews The user-facing contract ("newest first by created_at") is unchanged and still works in production where real users don't insert multiple rows in the same transaction. The fixture's shared-transaction pattern just doesn't simulate that scenario. Long-term: when a BIGSERIAL ordering column lands (likely with the S3 storage adapter), the test can re-assert strict insert order and the tiebreaker rule can fall back to "id DESC for safety".
§1 Live demo no longer says "frontend coming up" — the Amplify deploy landed last night at https://main.dew27gk9z09jh.amplifyapp.com. Demo-seed text now correctly notes that pre-seeded image PNGs are stranded on the Fargate seed-task container disk (the live regen path produces real outputs on the App Runner instance and renders correctly); the S3 adapter swap removes the caveat. §10 Deployment swapped the "subsequent deploys are auto via deploy.yml OIDC" paragraph for the actual current flow: a manual docker buildx push + aws apprunner start-deployment for backend, and aws amplify create-deployment + presigned-URL zip upload for frontend. deploy.yml still exists but the OIDC role isn't wired yet, so the manual flow is what the live demo was built with. The runbook mentions this so future-me doesn't waste an hour rediscovering it. ARCHITECTURE.md AWS cost estimate table is filled in with the actual sizing the dev stack runs at (1 vCPU / 2 GB App Runner, db.t4g.micro, 0-day backup, no NAT, no ElastiCache, no CloudFront). Total ~$70/month for AWS + the existing ~$3-8/active-week OpenAI line. The two largest line items (App Runner always-on, RDS public endpoint) are explicitly annotated with the trade-offs that made them intentional. ARCHITECTURE.md "What's next" now lists the actual ordered roadmap items with issue links (S3 adapter, CloudFront, OIDC role, Phase 11 hardening, exports, frontend polish) plus a "considered but deferred" section for password reset, Bedrock migration plan, and multi-tenant scoping. Closes #95, #96.
Catching up DEVLOG with everything that's happened since the polish/ demo-readiness merge: - PR #143 — AI-review round 1: CSRF Origin guard, storage refactor (store-key-only + URL projection), image regen DB lock, migration task secrets, App Runner sizing, ElastiCache removal, CORS localhost validator, router result-projection dedup - PR #144 — AI-review round 2: CDK context fail-synth (replacing the round-1 synthetic-defaults approach the user pushed back on), CSRF observability fix (middleware order), image regen NOWAIT, CSRF scope extension to login/register, password schema honesty - PR #145 / #146 — Deploy correctness: strict CDK context shape validation, App Runner autoscaling actually wired, DB pool right-sized, CI flake fix (integration tests asserted insert-order ordering but the fixture wraps everything in one transaction, so all rows share a created_at), and the format-gate debacle that turned the CI badge red across every branch since slice-3 - Live deploy — first end-to-end run through DEPLOY.md, with the three operational surprises (no Git-connect on Amplify Gen-2 console for CDK-created apps, default-SG has no egress so Fargate can't reach Secrets Manager, /app/local_images permission error on first image-gen) - PR #147 — README + ARCHITECTURE polish closing P12.1 and P12.2 Newest-first ordering preserved. Each entry follows the existing "what shipped + why + what was cut" voice.
…ate/
Brief asks for "what you would build next if you had more time" as an
optional architecture-note line item. README §12 now answers that
directly with ten ordered items, grouped by horizon:
- Deploy-batch follow-ups (S3 adapter, CloudFront, OIDC for deploy.yml)
- Submission-bonus item (Markdown export to close the dashboard
download gap from feature #3)
- Production hardening (private RDS + Proxy, Redis blocklist + idempotency,
job-table dedupe for image regen)
- Frontend polish before "ship to real marketing teams" (skeletons,
mobile sweep, a11y audit)
- Explicitly deferred (password reset, Bedrock migration, multi-tenant)
Each item links to its issue and explains the trade-off, not just the
task.
Also moves SLICE_PLAN.md to .private/ (already in .gitignore at line 19)
so the local-only planning artifact doesn't ship to the public repo.
History is preserved — the file's commits remain reachable via git log.
Three places that linked to SLICE_PLAN.md were updated:
- README §3 stack table (S3 adapter footnote): now points at issue #49
- ARCHITECTURE.md "What's next" opener: now points at the issue board
- DEVLOG older entry: prose mention, link removed
Renumbered README §12→§13 and §13→§14 for the new "What I'd add next"
section.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #95 ([P12.1] README full polish) and #96 ([P12.2] ARCHITECTURE.md). Both are flagged "Must not slip (brief §13)".
README
LocalImageStorage); live regen produces real outputs and is the demo flow. Goes away with the S3 adapter swap.docker buildx push+aws apprunner start-deploymentfor backend,aws amplify create-deployment+ presigned-URL zip upload for frontend).deploy.ymlis documented as the eventual target once the OIDC role is wired.ARCHITECTURE.md
Test plan