feat: FastAPI app skeleton for sync-engine deployment - #4
Conversation
Wires InMemoryStore and WebhookHandler behind a minimal HTTP surface so the engine has a runnable artifact: /health for liveness, /webhooks for HMAC-verified event reception. Uses an app factory (create_app) rather than a module-level app instance so tests can inject a store and secret without touching WEBHOOK_SECRET; production entrypoint runs via `uvicorn sync_engine.app:create_app --factory`. Also refreshes README/testing.md, which still described webhook/ and the package as "upcoming" despite both having landed in PR #3.
Adds uvicorn as a runtime dependency (needed to actually serve the FastAPI app in the container) and a workflow that builds/pushes to ghcr.io/koydas/sync-engine on merges to main, mirroring the pattern already used in fullstack-pilot's build-backend.yml.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7488471293
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
|
|
||
| def create_app(store: SyncStore | None = None, secret: str | None = None) -> FastAPI: | ||
| store = store or InMemoryStore() |
There was a problem hiding this comment.
When create_app() is invoked as the no-argument deployment factory, every process silently constructs the test-only InMemoryStore. A successful webhook response then acknowledges data that is lost on any restart and is not shared across multiple workers, defeating the documented persistent-queue and at-least-once guarantees; production startup should inject a persistent backend or fail rather than defaulting to this store.
Useful? React with 👍 / 👎.
| try: | ||
| payload = await request.json() | ||
| except ValueError as exc: | ||
| raise HTTPException(status_code=400, detail="Invalid JSON body") from exc |
There was a problem hiding this comment.
A validly signed JSON array, scalar, or null is accepted here and passed to WebhookHandler, even though both the handler and SyncStore.enqueue_webhook require a dict. This places values outside the store contract into the queue and can break downstream processing; validate that the decoded payload is an object and return a 400 otherwise.
Useful? React with 👍 / 👎.
|
|
||
| def create_app(store: SyncStore | None = None, secret: str | None = None) -> FastAPI: | ||
| store = store or InMemoryStore() | ||
| secret = secret if secret is not None else os.environ["WEBHOOK_SECRET"] |
There was a problem hiding this comment.
If WEBHOOK_SECRET exists but is empty, or a caller explicitly supplies secret="", the application starts and authenticates HMACs using the publicly guessable empty key. In that misconfiguration, anyone can forge accepted webhooks; startup should reject empty secrets instead of checking only for None or a missing environment key.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eef8aa3905
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
|
|
||
| def create_app(store: SyncStore | None = None, secret: str | None = None) -> FastAPI: | ||
| store = store or InMemoryStore() |
There was a problem hiding this comment.
When the new Dockerfile runs uvicorn ... --factory, it invokes create_app() without arguments, so every deployed container takes this branch and uses InMemoryStore, which is explicitly test-only. The API therefore reports webhooks as accepted even though the queue is lost on every restart and cannot be shared across replicas, breaking the engine's at-least-once delivery guarantee; the production factory needs to construct a configured persistent SyncStore instead.
Useful? React with 👍 / 👎.
| try: | ||
| payload = await request.json() | ||
| except ValueError as exc: | ||
| raise HTTPException(status_code=400, detail="Invalid JSON body") from exc |
There was a problem hiding this comment.
For a correctly signed body such as [], null, or a JSON string, request.json() succeeds and the endpoint returns accepted, even though WebhookHandler.handle() and SyncStore.enqueue_webhook() require a dictionary payload. This places values outside the store contract into the queue and can make downstream processing fail when it treats them as mappings; validate that the decoded value is an object before calling the handler.
Useful? React with 👍 / 👎.
1 participant
Summary
src/sync_engine/app.py: a FastAPI app factory (create_app) wiringInMemoryStore+WebhookHandlerbehind/healthand/webhooks.uvicorn sync_engine.app:create_app --factory(factory pattern avoids constructing the app — and requiringWEBHOOK_SECRET— at import time, so tests can inject their own store/secret).webhook/and the package itself as "upcoming" despite both landing in PR feat: webhook verifier and handler with idempotency guard #3.This is the first runnable artifact for the engine — the piece that will get containerized and deployed via ArgoCD in a follow-up. More commits (Dockerfile, image-publish CI workflow, Helm chart) are landing on this same branch before merge, so this PR becomes the full "deployable skeleton".
Test plan
pytest— 28/28 passing locallyruff check ./black --check .— cleandocker build+ local run once the Dockerfile lands