foundation: create the modular-monolith API and worker skeleton (#6) - #33
Conversation
|
Caution Review failedFailed to post review comments. GitHub was unavailable or timed out while CodeRabbit was posting the review. Please request a new review later if the pull request still needs one. This happened while posting 1 inline comment. Use ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (17)
🧰 Additional context used📓 Path-based instructions (4)**/*📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{py,ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/tests/**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
🪛 ast-grep (0.44.1)applications/worker.py[info] 16-24: use jsonify instead of json.dumps for JSON output (use-jsonify) tests/process/test_processes.py[error] 21-35: Command coming from incoming request (subprocess-from-request) [error] 64-70: Command coming from incoming request (subprocess-from-request) [error] 81-87: Command coming from incoming request (subprocess-from-request) [warning] 40-40: Request-controlled URL passed to urlopen; validate against an allowlist to prevent SSRF. (urlopen-unsanitized-data) 🪛 checkmake (0.3.2)Makefile[warning] 2-2: Required target "all" is missing from the Makefile. (minphony) [warning] 2-2: Required target "clean" is missing from the Makefile. (minphony) 🪛 Ruff (0.15.21)tests/process/test_processes.py[error] 22-22: (S603) [error] 23-31: Starting a process with a partial executable path (S607) [error] 66-66: Starting a process with a partial executable path (S607) [error] 83-83: Starting a process with a partial executable path (S607) 🪛 zizmor (1.26.1).github/workflows/ci.yml[warning] 12-12: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false (artipacked) 📝 WalkthroughWalkthroughThe PR establishes an M0 Python package with build metadata, common verification commands, fail-closed runtime construction, FastAPI and worker entrypoints, shared build identification, CI, and unit/process tests. ChangesM0 runtime skeleton
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant APIProcess
participant FastAPI
participant Runtime
Client->>APIProcess: start API and request /health
APIProcess->>FastAPI: create application
FastAPI->>Runtime: validate required kernel dependencies
FastAPI-->>Client: readiness payload with BUILD_IDENTIFIER
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: b1503f0ea0
ℹ️ 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".
| sort_keys=True, | ||
| ) | ||
| ) | ||
| return 0 |
There was a problem hiding this comment.
When the worker is launched normally as context-engine-worker without --test-mode, this unconditional return lets the process print ready and exit immediately. That contradicts the separate --test-mode lifecycle and leaves deployments or smoke scripts with no running Supply worker process; normal mode should block/run its service loop even while job behavior is NOT_ACTIVE.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
will-fix — fixed in e740392. Normal mode now prints readiness with flush and blocks at applications/worker.py:28-29; --test-mode alone exits deterministically. Regression coverage at tests/process/test_processes.py:81-101 proves the installed worker remains alive until terminated.
| if process.poll() is not None or time.monotonic() >= deadline: | ||
| output = process.stdout.read() if process.stdout else "" |
There was a problem hiding this comment.
If the API subprocess is still alive but never serves /health before the deadline, this read() waits for EOF on a live stdout pipe, so the smoke test can hang indefinitely instead of failing after 10 seconds. This can happen for a startup deadlock or a server that stays running but binds incorrectly; terminate or non-blockingly drain the process before reading its output.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
will-fix — fixed in e740392. The timeout/dead-process path now terminates first and uses communicate(timeout=5) at tests/process/test_processes.py:45-50, so it cannot perform a blocking read against a live child. The installed API-script smoke remains bounded by the existing readiness deadline.
|
Verdict: request-changes Per-criterion review of issue #6 at
Convention blockers/risks:
Local verification: Bypass reproduction: Accepted choice: Request changes before #6 establishes these construction and module-boundary conventions for downstream issues. Rejected alternatives:
Constraint: #6 is the convention-setting foundation, and its explicit acceptance criteria require a non-disableable construction guard plus runnable process entry points, not merely green tests around alternate seams. Re-run verification after these are fixed. |
|
Verdict: approve What I checked:
Verification re-run locally: |
1 participant
Originating Multica issue: STO-388
Original author: @hao
Summary
Adds the first runnable ContextEngine vertical skeleton: a Python 3.13 project with one FastAPI API process, one long-lived independent Supply worker process, one shared build identity, fail-closed Runtime construction, locked dependencies, repository-owned verification commands, and CI. The API health and worker lifecycle explicitly report Runtime delivery and job behavior as
NOT_ACTIVE.Why
The greenfield repository needs one load-bearing layout and toolchain before downstream M0 work can safely fan out. This implements and closes #6 while preserving the settled API/worker/shared-domain dependency direction and refusing to imply any database, authorization, or ContextPackage behavior that does not exist yet. It is the first implementation step under STO-388.
Closes #6
Approach
The API adapter remains at
adapters/http/app.py; process composition roots live outward inapplications/api.pyandapplications/worker.py. Both import the sharedenginepackage and build identifier, whileengine/has no transport/application imports. The installed scripts accept process arguments and are the exact seams exercised by smoke tests.engine/runtime/construction.pymodels policy, audit, budget, and provenance as four explicit mandatory inputs. Runtime itself rejects subclasses/duck types and inspects every exact enum identity, so caller-controlled validation cannot disable the guard. It deliberately exposes noresolve()behavior.Accepted choice: Use the ADR-selected Python 3.13/FastAPI modular monolith with outer process roots,
uv.lock, and a small Makefile façade shared by local development and CI.Rejected alternatives:
dependencies.validate()— rejected because subclass/duck-type overrides bypassed the security guard.engine— rejected because the accepted dependency direction keeps transport/composition outside the shared domain.uvsupplies interpreter selection, frozen sync, locking, running and build.Constraint: ADR-0005 fixes Python 3.13/FastAPI, ADR-0008 fixes API plus independent worker sharing one domain package, and issue #6 requires reproducible locked setup plus non-disableable policy/audit/budget/provenance startup guards.
How I Tested
End-to-end test cases
tests/process/test_processes.py:20context-engine-apiwith an injected port, waits for HTTP readiness, and checks shared version plusruntime_delivery: NOT_ACTIVE.tests/process/test_processes.py:64context-engine-worker --test-mode, requires exit 0, and checks shared version plusjob_behavior: NOT_ACTIVE.tests/process/test_processes.py:81tests/unit/test_runtime_construction.py:13tests/unit/test_runtime_construction.py:25tests/unit/test_runtime_construction.py:38KernelDependenciessubclass with no-opvalidate()and all-None fields is rejected.tests/unit/test_runtime_construction.py:54validate()is rejected.Verbatim test output
Red-before regression run against the pre-fix implementation:
The API failure showed the shipped script ignored the injected port and bound
127.0.0.1:8000; the worker failure showed exit code 0 instead of remaining alive. Both bypass tests reportedDID NOT RAISE RuntimeConfigurationError.Green-after complete matrix:
Installed console-script smoke:
Domain import-boundary scan:
Existing tests
New tests added
tests/unit/test_runtime_construction.py:38— rejects subclass override plus all-None inputs.tests/unit/test_runtime_construction.py:54— rejects duck-typed no-op validation.tests/process/test_processes.py:20— runs the installed API script with injected host/port.tests/process/test_processes.py:64— runs the installed worker test lifecycle.tests/process/test_processes.py:81— proves normal worker remains alive until terminated.Lint / typecheck
Rollback Plan
Revert this PR. Maximum blast radius is the repository development/build skeleton and the two not-yet-deployed process entry points; there is no database, migration, persistent data, Runtime delivery, external effect, or tenant state to unwind. Time-to-rollback is under five minutes once the revert is merged.
Out of Scope
ContextRuntime.resolve, transport authentication, ContextPackage schemas, or any successful authorization behavior.Summary by CodeRabbit
New Features
Documentation
Chores