feat: add authenticated HTTPS control plane - #20
Conversation
|
Warning Review limit reached
Next review available in: 24 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThe pull request adds an authenticated HTTPS-only ChangesControl plane
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (6)
web/src/content/docs/cli.mdx-12-18 (1)
12-18: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the
Last updatedmetadata on all changed pages.Each page still states March 2025, but this PR adds the HTTPS control-plane and explicit local-execution guidance. Use the actual publication date.
web/src/content/docs/cli.mdx#L12-L18: update the page metadata.web/src/content/docs/docker.mdx#L24-L24: update the page metadata.web/src/content/docs/introduction.mdx#L17-L17: update the page metadata.web/src/content/docs/quickstart.mdx#L51-L51: update the page metadata.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/content/docs/cli.mdx` around lines 12 - 18, Update the Last updated metadata to the actual publication date in web/src/content/docs/cli.mdx lines 12-18, web/src/content/docs/docker.mdx line 24, web/src/content/docs/introduction.mdx line 17, and web/src/content/docs/quickstart.mdx line 51.tests/e2e/test_cli_run.py-203-208 (1)
203-208: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winIsolate the environment so the remote-configuration guard is deterministic.
enqueuebinds--api-url,--auth-url, and--workspacetoLOAFER_API_URL,LOAFER_AUTH_URL, andLOAFER_WORKSPACE_IDthrough Typerenvvar(loafer/cli.py Lines 114-116). If the runner exports those variables, or exportsLOAFER_ACCESS_TOKEN, the guard does not trigger and this test fails or issues a real request. Clear the variables in the test.💚 Proposed fix
- def test_enqueue_never_falls_back_without_remote_configuration(self) -> None: + def test_enqueue_never_falls_back_without_remote_configuration( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + for name in ( + "LOAFER_API_URL", + "LOAFER_AUTH_URL", + "LOAFER_WORKSPACE_ID", + "LOAFER_ACCESS_TOKEN", + ): + monkeypatch.delenv(name, raising=False) result = runner.invoke(app, ["enqueue", "/nonexistent/path.yaml"])Import
pytestif the module does not already import it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/test_cli_run.py` around lines 203 - 208, Update test_enqueue_never_falls_back_without_remote_configuration to isolate the environment with pytest’s monkeypatch fixture, removing LOAFER_API_URL, LOAFER_AUTH_URL, LOAFER_WORKSPACE_ID, and LOAFER_ACCESS_TOKEN before invoking enqueue. Preserve the existing assertions and import pytest if needed.loafer/cli.py-168-201 (1)
168-201: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDefensively read Better Auth device-flow response fields.
Better Auth accepts these device endpoints as JSON, but the authorization response may omit RFC 8628’s optional
verification_uri_complete. Read device fields with.get()and fall back toverification_uriplususer_codeso a sparse response raises a clear error instead ofCLI login failed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@loafer/cli.py` around lines 168 - 201, The device-flow handling in the CLI login function should defensively read authorization response fields using `.get()`. Use `verification_uri_complete` when present, otherwise construct the displayed link from `verification_uri` and `user_code`, and validate required fields so sparse responses raise a clear error rather than an incidental key lookup failure. Preserve the existing authentication and polling behavior.loafer/control_plane/app.py-421-424 (1)
421-424: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Last-Event-IDaccepts a negative sequence.
int(last_event_id or "0")accepts-5. Theafterquery parameter on the siblingeventsandlogsroutes enforcesQuery(ge=0). Reject a negative value here so the resume path matches the polling path.🐛 Proposed fix
try: after = int(last_event_id or "0") except ValueError as exc: raise ValueError("Last-Event-ID must be an integer sequence") from exc + if after < 0: + raise ValueError("Last-Event-ID must not be negative")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@loafer/control_plane/app.py` around lines 421 - 424, Validate the parsed `after` value in the `Last-Event-ID` handling block so negative sequence IDs are rejected, matching the non-negative constraint used by the sibling `events` and `logs` routes. Preserve the existing integer parsing error and raise an appropriate `ValueError` when `after` is below zero.loafer/control_plane/auth.py-69-79 (1)
69-79: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStrip whitespace in the list branch of the
roleclaim.The string branch applies
.strip().lower(). The list branch applies only.lower(). A list entry of" admin"therefore becomes" admin"and never matches an"admin"permission check. The mismatch fails closed, so it denies access rather than granting it, but the two branches should normalize identically.🐛 Proposed fix
elif isinstance(role_claim, list): global_roles = frozenset( - role.lower() for role in role_claim if isinstance(role, str) and role + role.strip().lower() + for role in role_claim + if isinstance(role, str) and role.strip() )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@loafer/control_plane/auth.py` around lines 69 - 79, Update the list-handling branch for role_claim in the claims normalization logic to strip surrounding whitespace before lowercasing each string role, matching the normalization performed in the string branch. Preserve filtering of non-string and empty entries so both input forms produce equivalent global_roles values.loafer/control_plane/app.py-146-166 (1)
146-166: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake problem responses CORS-safe.
CORSMiddlewareis registered beforesecurity_boundary, butsecurity_boundaryis the outermost middleware. When it returns a_problemresponse directly,allow_originsprocessing does not run, so clients onLOAFER_ALLOWED_ORIGINScannot read 400/429 responses despite the allowed origin. Add CORS/Accessibility-Control-Expose-Headershandling for short-circuited_problemresponses, or addCORSMiddlewarelast.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@loafer/control_plane/app.py` around lines 146 - 166, Update security_boundary’s short-circuited _problem responses so they include the appropriate CORS headers for request origins in selected.allowed_origins, including Access-Control-Expose-Headers as needed. Ensure this applies consistently to 400/403/429 responses returned before downstream CORSMiddleware, without changing rejection behavior for untrusted origins.
🧹 Nitpick comments (15)
tests/unit/test_control_plane_api.py (1)
52-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate the generator fixture with
Iterator.The
apifixture usesyield, so it is a generator function. The declared return typetuple[ASGIClient, SqlMetadataStore]does not describe it. A type checker overtestsreports this as an error.♻️ Proposed fix
+from collections.abc import Iterator + `@pytest.fixture`() -def api(tmp_path: Path) -> tuple[ASGIClient, SqlMetadataStore]: +def api(tmp_path: Path) -> Iterator[tuple[ASGIClient, SqlMetadataStore]]:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_control_plane_api.py` around lines 52 - 53, Update the return annotation of the generator fixture api to use the appropriate Iterator type, with the yielded tuple of ASGIClient and SqlMetadataStore as its element type.scripts/generate_control_plane_contract.py (1)
33-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin UTF-8 for artifact reads and writes.
read_text()andwrite_text()use the platform default encoding. On a runner with a non-UTF-8 locale,--checkcan report a stale artifact or write a differently encoded file, which breaks the CI contract gate. Set the encoding explicitly.♻️ Proposed fix
stale = [ path for path, content in outputs.items() - if not path.exists() or path.read_text() != content + if not path.exists() or path.read_text(encoding="utf-8") != content ] if args.check: if stale: rendered = ", ".join(str(path.relative_to(ROOT)) for path in stale) parser.error(f"generated control-plane artifacts are stale: {rendered}") return for path, content in outputs.items(): path.parent.mkdir(parents=True, exist_ok=True) - path.write_text(content) + path.write_text(content, encoding="utf-8")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/generate_control_plane_contract.py` around lines 33 - 45, Update the artifact comparison and generation logic around outputs in the control-plane contract script to pass encoding="utf-8" to both path.read_text() and path.write_text(). Keep the existing stale detection, check-mode error handling, and output directory creation unchanged.openapi/control-plane-v1.json (1)
1649-1657: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDeclare the SSE response as
text/event-stream.The
event_streamroute returnsmedia_type="text/event-stream"(loafer/control_plane/app.py:414-439), but the contract advertisesapplication/jsonwith an empty schema. Generated clients then expect JSON for a stream endpoint. Set the response class and content type on the route decorator, then regenerate this artifact.`@app.get`( "/api/v1/workspaces/{workspace_id}/runs/{run_id}/stream", response_class=StreamingResponse, responses={200: {"content": {"text/event-stream": {}}}}, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openapi/control-plane-v1.json` around lines 1649 - 1657, Update the OpenAPI response for the SSE stream route to advertise text/event-stream instead of application/json, matching the route’s StreamingResponse configuration. Regenerate openapi/control-plane-v1.json so the 200 response content type and schema reflect the streaming contract.loafer/adapters/metadata_schema.py (1)
335-349: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd indexes for the expected
loafer_audit_eventsquery patterns.The table has only a primary key on
id. Audit reads are normally filtered by tenant and time. Without a supporting index, every organization- or workspace-scoped audit query becomes a sequential scan that grows without bound.♻️ Proposed indexes
Column("metadata_json", JSON, nullable=False), Column("occurred_at", DateTime(timezone=True), nullable=False), + Index("ix_loafer_audit_events_org_time", "organization_id", "occurred_at"), + Index("ix_loafer_audit_events_workspace_time", "workspace_id", "occurred_at"), )
Indexmust be imported fromsqlalchemy, and_up_v3must create the indexes after the table.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@loafer/adapters/metadata_schema.py` around lines 335 - 349, Update the audit_events schema and _up_v3 migration to import SQLAlchemy’s Index and create indexes after the loafer_audit_events table for organization_id with occurred_at, and workspace_id with occurred_at, matching the expected tenant-and-time query patterns.loafer/adapters/metadata.py (1)
665-671: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefine the workspace-scope helper once.
loafer/control_plane/repository.pylines 368-373 contains a byte-identical copy of this method, including the GUC nameloafer.workspace_idand theis_localflag. The GUC name is the contract that the RLS policies inloafer/adapters/metadata_schema.pydepend on. If one copy changes, the tenant predicate silently stops matching in that code path.Export one helper (for example a module-level function in
loafer/adapters/metadata.py) and let the repository call it.♻️ Proposed refactor
+WORKSPACE_SCOPE_SETTING = "loafer.workspace_id" + + +def set_workspace_scope(connection: Connection, workspace_id: str) -> None: + """Bind the tenant predicate for the current transaction on PostgreSQL.""" + if connection.dialect.name == "postgresql": + connection.execute( + text(f"SELECT set_config('{WORKSPACE_SCOPE_SETTING}', :workspace_id, true)"), + {"workspace_id": workspace_id}, + ) + + class SqlMetadataStore:Then reduce the method to a delegation:
def _set_workspace_scope(self, connection: Connection, workspace_id: str) -> None: - if self.profile == "postgresql": - connection.execute( - text("SELECT set_config('loafer.workspace_id', :workspace_id, true)"), - {"workspace_id": workspace_id}, - ) + set_workspace_scope(connection, workspace_id)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@loafer/adapters/metadata.py` around lines 665 - 671, Define a single exported workspace-scope helper in loafer/adapters/metadata.py containing the existing PostgreSQL set_config behavior, including the loafer.workspace_id GUC and local-setting flag. Update _set_workspace_scope and the corresponding repository implementation to delegate to this shared helper, removing the byte-identical duplicate while preserving non-PostgreSQL behavior.loafer/control_plane/repository.py (2)
452-461: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not import private row mappers from another module.
_runand_scheduleimport_run_recordand_schedulefromloafer.adapters.metadata. Both names are private to that module, so this repository depends on a surface that carries no compatibility guarantee. A rename insideloafer/adapters/metadata.pybreaks the control plane at runtime, and the deferred imports hide the break until the call executes.Promote the two mappers to public names in
loafer/adapters/metadata.pyand import them at module top level.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@loafer/control_plane/repository.py` around lines 452 - 461, Promote the row mappers currently named _run_record and _schedule in loafer.adapters.metadata to public names, then update control_plane.repository’s _run and _schedule wrappers to import those public names at module top level instead of using deferred private imports. Preserve the existing mapping behavior and return types.
401-403: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the list index in the rejection path.
The list branch passes the parent
pathunchanged. An error inside a list therefore reportspayload.sources.passwordwith no index, so a caller with many sources cannot tell which entry failed.♻️ Proposed change
if isinstance(value, list): - return [_reject_embedded_secrets(item, path) for item in value] + return [ + _reject_embedded_secrets(item, f"{path}[{index}]") + for index, item in enumerate(value) + ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@loafer/control_plane/repository.py` around lines 401 - 403, The list branch in _reject_embedded_secrets must include each element’s index when recursively building the rejection path. Update the list comprehension to pass a path extended with the current index, preserving the existing parent path for non-list values and accurately identifying entries such as sources[0].password.tests/integration/test_metadata_store.py (1)
47-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the policy enforces the predicate, not only that it exists.
The test proves the two policies were created. It does not prove they filter rows. A policy can exist and still be inert, for example when the connecting role owns the table.
Add one behavioral case: insert runs for two workspaces, call
set_config('loafer.workspace_id', <workspace-a>, true)in a transaction, then assert the unfilteredSELECTreturns only workspace A rows. That test would also detect the owner-bypass and missingWITH CHECKproblems raised onloafer/adapters/metadata_schema.py.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/test_metadata_store.py` around lines 47 - 54, The metadata store integration test currently checks only policy existence; add a behavioral case that inserts runs for two workspaces, sets loafer.workspace_id to workspace A with set_config(..., true) inside a transaction, and verifies an unfiltered SELECT returns only workspace A rows. Extend the test around the existing policy assertions and use the established run/workspace symbols and database connection flow.loafer/control_plane/auth.py (2)
37-44: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a small
leewayfor clock skew.The algorithm allowlist, bound issuer and audience, and required claims are all correct.
jwt.decodeuses zero leeway by default, so any clock drift between the Better Auth issuer andloaferdrejects a freshly minted token oniat, or expires one early onexp. The PR describes these tokens as short-lived, which makes the margin thin. Pass a few seconds ofleeway.♻️ Proposed change
issuer=self._issuer, audience=self._audience, + leeway=30, options={"require": ["sub", "iss", "aud", "exp", "iat"]},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@loafer/control_plane/auth.py` around lines 37 - 44, Update the jwt.decode call in the authentication flow to pass a small, explicit leeway of a few seconds for clock skew while preserving the existing algorithm, issuer, audience, and required-claims validation.
50-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
StaticTokenVerifierinto the test tree.The docstring states this class serves in-process contract tests, but it ships in the production
auth.pymodule.create_appaccepts anyTokenVerifier, so a future change could wire this class into a deployment and bypass JWT verification.daemon.pynever passesverifier, so no production path reaches it today.Move the class into
tests/so the production package cannot supply a bypass. If any non-test code imports it, keep it here and add a guard that refuses to construct it whenenforce_httpsis set.#!/bin/bash # Check which modules import StaticTokenVerifier. rg -n -C 2 'StaticTokenVerifier'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@loafer/control_plane/auth.py` around lines 50 - 60, Move the StaticTokenVerifier class and its production import/export references out of loafer/control_plane/auth.py into the tests tree, updating test imports to use the new location. Ensure production code exposes only the TokenVerifier contract and cannot construct this static bypass; if a non-test import must remain, add construction-time rejection when enforce_https is enabled..github/workflows/ci.yml (2)
100-103: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
web-authomitsneeds: [lint].Every other job in this workflow declares
needs: [lint].web-authruns unconditionally. Thelintjob only checks Python, so this is defensible for a Node-only job. If you want the workflow to fail fast on a stale generated contract, add the dependency, because thelintjob now also verifies the control-plane contract that the web client consumes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 100 - 103, Add needs: [lint] to the web-auth job so it waits for the lint job, including its control-plane contract verification, before running. Keep the existing web-auth steps and configuration unchanged.
117-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
npm auditcan block unrelated pull requests.
npm audit --omit=dev --audit-level=highdepends on live registry advisory data. A newly published high advisory with no released fix fails this required job for every open pull request, including changes unrelated to the dependency. Consider running the audit on a schedule, or allow it to report without failing the merge path.Also split
npm run lint && npm run typecheckinto two steps. The combined step short-circuits, so a lint failure hides typecheck errors and the failed step name is ambiguous.♻️ Proposed step split
- - name: Lint and typecheck + - name: Lint working-directory: web - run: npm run lint && npm run typecheck + run: npm run lint + + - name: Typecheck + if: ${{ !cancelled() }} + working-directory: web + run: npm run typecheck🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 117 - 123, Update the CI workflow steps around “Check production advisories” and “Lint and typecheck”: prevent the live npm audit from blocking the required pull-request merge path, either by moving it to a scheduled workflow or allowing it to report without failing. Split the combined lint/typecheck command into separate named steps, each running its respective npm script so both checks execute and failures identify the responsible check.loafer/control_plane/app.py (2)
283-283: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConstrain the
Idempotency-Keyheader.The header is typed as a bare
str, so a client can send an arbitrarily long value that reaches the idempotency store as a key. Add length and pattern bounds. The same annotation is repeated at Line 311, Line 349, Line 371, and Line 480, so extract one alias.♻️ Proposed refactor
IdempotencyKey = Annotated[ str, Header(alias="Idempotency-Key", min_length=1, max_length=255, pattern=r"^[A-Za-z0-9._:-]+$") ]Then each route declares
idempotency_key: IdempotencyKey.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@loafer/control_plane/app.py` at line 283, Define a shared IdempotencyKey Annotated alias with the Idempotency-Key header, requiring 1–255 characters and the pattern ^[A-Za-z0-9._:-]+$. Replace the repeated bare-string idempotency_key annotations in all affected route declarations with this alias, including the declarations near lines 283, 311, 349, 371, and 480.
525-529: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
@app.on_event("shutdown")with a lifespan handler.With
fastapi>=0.116,<1,on_eventis deprecated and emits aDeprecationWarning. Define anasynccontextmanagerlifespan that closesmetadataon shutdown, passlifespan=lifespantoFastAPI(...), and remove theon_eventblock.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@loafer/control_plane/app.py` around lines 525 - 529, Replace the shutdown `@app.on_event` handler around close_metadata with an asynccontextmanager lifespan handler that closes metadata during shutdown, pass it via lifespan=lifespan when constructing FastAPI, and remove the deprecated event block. Preserve the owned_store condition so the handler is only configured when metadata is owned.loafer/control_plane/schemas.py (1)
100-107: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winValidate
timezoneandtrigger_specat the contract boundary.
timezoneaccepts any 1-64 character string.trigger_specaccepts any string for bothcronandinterval. An invalid value is persisted and then fails inside the scheduler, far from the request that caused it. Validate the timezone againstzoneinfo.available_timezones()and validatetrigger_specpertrigger_kind.♻️ Proposed validator
from zoneinfo import ZoneInfo `@field_validator`("timezone") `@classmethod` def _known_timezone(cls, value: str) -> str: try: ZoneInfo(value) except Exception as exc: # ZoneInfoNotFoundError, ValueError raise ValueError("timezone must be a valid IANA zone") from exc return value🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@loafer/control_plane/schemas.py` around lines 100 - 107, Update ScheduleUpsertRequest to validate timezone as a known IANA zone using zoneinfo, and add trigger_spec validation that applies the appropriate cron or interval format based on trigger_kind. Reject invalid values at model validation time while preserving the existing field constraints and accepted trigger kinds.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@loafer/control_plane/app.py`:
- Around line 448-458: Require an Idempotency-Key header for the
create_connection endpoint, matching the existing command routes and dependency
pattern. Apply the same requirement to register_pipeline so both
resource-creating POST handlers reject requests without the header.
- Around line 162-166: Update the rate-limiter key in the request handling path
around limiter.allow so requests behind --behind-tls-proxy are identified per
client rather than by the proxy’s address. Reuse the existing trusted-proxy
handling to derive the client identity from X-Forwarded-For, or use the
authenticated subject when available, while preserving the current
request.client.host fallback for direct connections.
- Around line 637-649: Update the SSE generator loop around service.events to
offload the synchronous call with asyncio.to_thread or run_in_threadpool,
preventing database queries from blocking the event loop. After processing
events, fetch or reuse the run record and break when it reaches a terminal
state, and enforce a maximum stream duration so connected clients must
eventually reconnect while preserving disconnect and heartbeat behavior.
- Around line 100-112: Remove the metadata.migrate() call from create_app so
application construction remains stateless. Add an explicit loaferd migrate
command that invokes migration once before rollout, and update create_app to
verify the metadata schema version instead, failing clearly when it is not
current.
- Around line 169-175: Update the response-header middleware around the
Content-Security-Policy assignment so the strict default-src 'none' policy is
not applied to the Swagger UI route at /api/v1/docs, while preserving it for
other API responses. Keep the existing security headers unchanged and ensure the
documentation page can load its CDN JavaScript and CSS.
- Around line 534-548: Update _RateLimiter.allow to delete each key from
_requests after expired timestamps are trimmed and its deque becomes empty,
while preserving the existing return False behavior when the per-key limit is
reached. Add a bounded maximum for tracked keys and enforce it when admitting
new keys, using the limiter’s existing configuration pattern where appropriate.
- Around line 178-183: The async authenticate dependency currently calls
synchronous token_verifier.verify directly, potentially blocking the event loop
during JWKS retrieval. Update authenticate to run verification via
asyncio.to_thread or the established threadpool mechanism, and configure an
explicit request timeout on the PyJWKClient used by BetterAuthJWTVerifier in
auth.py.
In `@loafer/control_plane/auth.py`:
- Line 30: Update the PyJWKClient initialization in the authentication class to
pass an explicit short timeout for JWKS network fetches, while preserving the
existing caching and lifespan settings.
In `@loafer/control_plane/client.py`:
- Around line 102-127: Update _raise_for_status to read the response body before
parsing JSON, including streamed responses from iter_events. Catch the
unread-body condition as needed, and only call .get on a JSON object; otherwise
fall back to response.reason_phrase while preserving request_id extraction and
ControlPlaneClientError behavior.
In `@loafer/control_plane/daemon.py`:
- Around line 32-44: Update the behind_tls_proxy configuration path around
ControlPlaneSettings and security_boundary so forwarded headers are trusted only
from an explicitly configured proxy peer or network allowlist. Ensure direct
clients cannot set X-Forwarded-Proto to bypass HTTPS enforcement, and enforce
the reverse proxy’s host/network separation while preserving trusted-proxy HTTPS
behavior.
In `@loafer/control_plane/repository.py`:
- Around line 376-387: Remove the "url" entry from the _SECRET_KEYS denylist so
register_pipeline accepts normal source and target URL fields. Preserve the
existing rejection behavior for actual credential keys and rely on the existing
secret_reference contract for connection credentials.
In `@loafer/control_plane/service.py`:
- Around line 330-350: Update the mutation flows and
ControlPlaneRepository.audit so audit rows are written using the same
transaction/connection as the corresponding change, rather than opening a
separate transaction after commit. Have _audit accept or reuse the active
connection, pass it to audit, and ensure organization validation occurs before
the mutation commits so MetadataError also rolls back the action.
- Around line 131-141: Update the run creation and retry flows around
repository.store.create_run to handle concurrent duplicate command inserts as
idempotency conflicts: catch the relevant MetadataError, re-read the existing
run and preserve the conflict response, or translate the uq_loafer_run_command
IntegrityError into IdempotencyConflictError before it becomes a 503. Apply this
consistently in create_run and retry_run while leaving unrelated metadata errors
unchanged.
In `@web/app/api/auth/`[...all]/route.ts:
- Line 3: Fix the `@/`* path mapping in web/tsconfig.json so `@/src/lib/auth`
resolves to web/src/lib/auth.ts, and ensure include covers src/**/*.ts. Keep the
dynamic import in web/app/api/auth/[...all]/route.ts unchanged;
web/app/api/control/[...path]/route.ts requires no direct change. Re-run npm run
typecheck to verify both imports resolve.
In `@web/app/api/control/`[...path]/route.ts:
- Around line 34-45: Validate the token returned by auth.api.getToken before
constructing the proxy request in the control route. When token is absent,
return a clear 401 problem response and do not set the Authorization header or
forward the request; preserve the existing path validation and authenticated
flow for valid tokens.
- Around line 53-67: Update the proxy response handling around the outgoing
Headers construction to delete both content-encoding and content-length before
returning the decoded response. Add an AbortSignal.timeout(...) to the fetch
options, using a longer or disabled timeout when the target path is /stream
while retaining a bounded timeout for other requests.
In `@web/tests/auth.integration.mjs`:
- Around line 42-44: Update the web test runner configuration in package.json so
Node enables TypeScript stripping for the dynamic import in
auth.integration.mjs, preferably by adding --experimental-strip-types without
raising the supported Node engine floor.
---
Minor comments:
In `@loafer/cli.py`:
- Around line 168-201: The device-flow handling in the CLI login function should
defensively read authorization response fields using `.get()`. Use
`verification_uri_complete` when present, otherwise construct the displayed link
from `verification_uri` and `user_code`, and validate required fields so sparse
responses raise a clear error rather than an incidental key lookup failure.
Preserve the existing authentication and polling behavior.
In `@loafer/control_plane/app.py`:
- Around line 421-424: Validate the parsed `after` value in the `Last-Event-ID`
handling block so negative sequence IDs are rejected, matching the non-negative
constraint used by the sibling `events` and `logs` routes. Preserve the existing
integer parsing error and raise an appropriate `ValueError` when `after` is
below zero.
- Around line 146-166: Update security_boundary’s short-circuited _problem
responses so they include the appropriate CORS headers for request origins in
selected.allowed_origins, including Access-Control-Expose-Headers as needed.
Ensure this applies consistently to 400/403/429 responses returned before
downstream CORSMiddleware, without changing rejection behavior for untrusted
origins.
In `@loafer/control_plane/auth.py`:
- Around line 69-79: Update the list-handling branch for role_claim in the
claims normalization logic to strip surrounding whitespace before lowercasing
each string role, matching the normalization performed in the string branch.
Preserve filtering of non-string and empty entries so both input forms produce
equivalent global_roles values.
In `@tests/e2e/test_cli_run.py`:
- Around line 203-208: Update
test_enqueue_never_falls_back_without_remote_configuration to isolate the
environment with pytest’s monkeypatch fixture, removing LOAFER_API_URL,
LOAFER_AUTH_URL, LOAFER_WORKSPACE_ID, and LOAFER_ACCESS_TOKEN before invoking
enqueue. Preserve the existing assertions and import pytest if needed.
In `@web/src/content/docs/cli.mdx`:
- Around line 12-18: Update the Last updated metadata to the actual publication
date in web/src/content/docs/cli.mdx lines 12-18,
web/src/content/docs/docker.mdx line 24, web/src/content/docs/introduction.mdx
line 17, and web/src/content/docs/quickstart.mdx line 51.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 100-103: Add needs: [lint] to the web-auth job so it waits for the
lint job, including its control-plane contract verification, before running.
Keep the existing web-auth steps and configuration unchanged.
- Around line 117-123: Update the CI workflow steps around “Check production
advisories” and “Lint and typecheck”: prevent the live npm audit from blocking
the required pull-request merge path, either by moving it to a scheduled
workflow or allowing it to report without failing. Split the combined
lint/typecheck command into separate named steps, each running its respective
npm script so both checks execute and failures identify the responsible check.
In `@loafer/adapters/metadata_schema.py`:
- Around line 335-349: Update the audit_events schema and _up_v3 migration to
import SQLAlchemy’s Index and create indexes after the loafer_audit_events table
for organization_id with occurred_at, and workspace_id with occurred_at,
matching the expected tenant-and-time query patterns.
In `@loafer/adapters/metadata.py`:
- Around line 665-671: Define a single exported workspace-scope helper in
loafer/adapters/metadata.py containing the existing PostgreSQL set_config
behavior, including the loafer.workspace_id GUC and local-setting flag. Update
_set_workspace_scope and the corresponding repository implementation to delegate
to this shared helper, removing the byte-identical duplicate while preserving
non-PostgreSQL behavior.
In `@loafer/control_plane/app.py`:
- Line 283: Define a shared IdempotencyKey Annotated alias with the
Idempotency-Key header, requiring 1–255 characters and the pattern
^[A-Za-z0-9._:-]+$. Replace the repeated bare-string idempotency_key annotations
in all affected route declarations with this alias, including the declarations
near lines 283, 311, 349, 371, and 480.
- Around line 525-529: Replace the shutdown `@app.on_event` handler around
close_metadata with an asynccontextmanager lifespan handler that closes metadata
during shutdown, pass it via lifespan=lifespan when constructing FastAPI, and
remove the deprecated event block. Preserve the owned_store condition so the
handler is only configured when metadata is owned.
In `@loafer/control_plane/auth.py`:
- Around line 37-44: Update the jwt.decode call in the authentication flow to
pass a small, explicit leeway of a few seconds for clock skew while preserving
the existing algorithm, issuer, audience, and required-claims validation.
- Around line 50-60: Move the StaticTokenVerifier class and its production
import/export references out of loafer/control_plane/auth.py into the tests
tree, updating test imports to use the new location. Ensure production code
exposes only the TokenVerifier contract and cannot construct this static bypass;
if a non-test import must remain, add construction-time rejection when
enforce_https is enabled.
In `@loafer/control_plane/repository.py`:
- Around line 452-461: Promote the row mappers currently named _run_record and
_schedule in loafer.adapters.metadata to public names, then update
control_plane.repository’s _run and _schedule wrappers to import those public
names at module top level instead of using deferred private imports. Preserve
the existing mapping behavior and return types.
- Around line 401-403: The list branch in _reject_embedded_secrets must include
each element’s index when recursively building the rejection path. Update the
list comprehension to pass a path extended with the current index, preserving
the existing parent path for non-list values and accurately identifying entries
such as sources[0].password.
In `@loafer/control_plane/schemas.py`:
- Around line 100-107: Update ScheduleUpsertRequest to validate timezone as a
known IANA zone using zoneinfo, and add trigger_spec validation that applies the
appropriate cron or interval format based on trigger_kind. Reject invalid values
at model validation time while preserving the existing field constraints and
accepted trigger kinds.
In `@openapi/control-plane-v1.json`:
- Around line 1649-1657: Update the OpenAPI response for the SSE stream route to
advertise text/event-stream instead of application/json, matching the route’s
StreamingResponse configuration. Regenerate openapi/control-plane-v1.json so the
200 response content type and schema reflect the streaming contract.
In `@scripts/generate_control_plane_contract.py`:
- Around line 33-45: Update the artifact comparison and generation logic around
outputs in the control-plane contract script to pass encoding="utf-8" to both
path.read_text() and path.write_text(). Keep the existing stale detection,
check-mode error handling, and output directory creation unchanged.
In `@tests/integration/test_metadata_store.py`:
- Around line 47-54: The metadata store integration test currently checks only
policy existence; add a behavioral case that inserts runs for two workspaces,
sets loafer.workspace_id to workspace A with set_config(..., true) inside a
transaction, and verifies an unfiltered SELECT returns only workspace A rows.
Extend the test around the existing policy assertions and use the established
run/workspace symbols and database connection flow.
In `@tests/unit/test_control_plane_api.py`:
- Around line 52-53: Update the return annotation of the generator fixture api
to use the appropriate Iterator type, with the yielded tuple of ASGIClient and
SqlMetadataStore as its element type.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1fd509ae-b169-40d3-8567-2ef057d2a05c
⛔ Files ignored due to path filters (2)
uv.lockis excluded by!**/*.lockweb/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (44)
.github/workflows/ci.ymlCHANGELOG.mdPRODUCTION_READINESS.mdREADME.mdloafer/adapters/metadata.pyloafer/adapters/metadata_schema.pyloafer/cli.pyloafer/control_plane/__init__.pyloafer/control_plane/app.pyloafer/control_plane/auth.pyloafer/control_plane/client.pyloafer/control_plane/daemon.pyloafer/control_plane/domain.pyloafer/control_plane/repository.pyloafer/control_plane/schemas.pyloafer/control_plane/service.pyopenapi/control-plane-v1.jsonpyproject.tomlscripts/generate_control_plane_contract.pyskills/loafer-api-design/SKILL.mdskills/loafer-auth/SKILL.mdskills/loafer-cli-tui/SKILL.mdskills/loafer-engineering/SKILL.mdskills/loafer-engineering/references/architecture.mdskills/loafer-engineering/references/platform-architecture.mdskills/loafer-self-hosting/references/deployment-contract.mdskills/loafer-web-ui/SKILL.mdtests/e2e/test_cli_run.pytests/integration/test_metadata_store.pytests/unit/test_control_plane_api.pytests/unit/test_metadata_store.pytests/unit/test_packaging.pyweb/.env.exampleweb/app/api/auth/[...all]/route.tsweb/app/api/control/[...path]/route.tsweb/package.jsonweb/public/search-index.jsonweb/src/content/docs/cli.mdxweb/src/content/docs/control-plane.mdxweb/src/content/docs/docker.mdxweb/src/content/docs/index.tsweb/src/content/docs/introduction.mdxweb/src/content/docs/quickstart.mdxweb/tests/auth.integration.mjs
| @@ -0,0 +1,8 @@ | |||
| async function handler(request: Request) { | |||
| // Runtime-only initialization keeps database credentials out of the build step. | |||
| const { auth } = await import('@/src/lib/auth') | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔴 Critical | ⚡ Quick win
Unresolved @/src/lib/auth import breaks the CI typecheck in both route files. npm run typecheck reports Cannot find module '@/src/lib/auth' or its corresponding type declarations. The module exists, because web/tests/auth.integration.mjs loads it as ../src/lib/auth.ts. The shared root cause is the @/* path mapping or the program includes in web/tsconfig.json, so one configuration change resolves both sites.
web/app/api/auth/[...all]/route.ts#L3-L3: keep the dynamic import, and correct the@/*mapping inweb/tsconfig.jsonso@/src/lib/authresolves toweb/src/lib/auth.ts. Confirm thatincludecoverssrc/**/*.ts.web/app/api/control/[...path]/route.ts#L25-L25: no change is needed at this line once the mapping resolves; re-runnpm run typecheckto confirm this specifier also resolves.
🧰 Tools
🪛 GitHub Actions: CI / 4_Web & Authentication Contracts.txt
[error] 3-3: TypeScript typecheck failed: Cannot find module '@/src/lib/auth' or its corresponding type declarations. Command: npm run typecheck
🪛 GitHub Actions: CI / Web & Authentication Contracts
[error] 3-3: TypeScript typecheck failed: Cannot find module '@/src/lib/auth' or its corresponding type declarations. Command: npm run typecheck.
📍 Affects 2 files
web/app/api/auth/[...all]/route.ts#L3-L3(this comment)web/app/api/control/[...path]/route.ts#L25-L25
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/app/api/auth/`[...all]/route.ts at line 3, Fix the `@/`* path mapping in
web/tsconfig.json so `@/src/lib/auth` resolves to web/src/lib/auth.ts, and ensure
include covers src/**/*.ts. Keep the dynamic import in
web/app/api/auth/[...all]/route.ts unchanged;
web/app/api/control/[...path]/route.ts requires no direct change. Re-run npm run
typecheck to verify both imports resolve.
Source: Pipeline failures
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@loafer/adapters/metadata.py`:
- Around line 602-609: Update upsert_schedule to detect an existing schedule ID
outside the authorized workspace before attempting the insert, using an
authorized workspace-scoped existence or uniqueness check. If such a conflict is
found, raise or translate it to IdempotencyConflictError instead of allowing the
global schedules.id IntegrityError to escape.
In `@loafer/application/durable.py`:
- Line 128: Update the worker composition flow around _get_ready_metadata_store
and DurableWorker so the metadata store is closed when get_object_storage or
DurableWorker construction raises before ownership transfers to the worker.
Ensure successful construction preserves the existing ownership and lifecycle
behavior.
In `@tests/unit/test_control_plane_api.py`:
- Around line 410-426: Update BetterAuthJWTVerifier’s JWKS retrieval path to
reject redirects that change the configured URL’s transport or authority before
following the next request, including HTTPS-to-HTTP and
HTTPS-to-different-authority cases; ensure PyJWT’s default fetcher cannot bypass
this validation. Add a regression test covering an HTTPS JWKS endpoint
redirecting to HTTP, while preserving existing authentication error behavior.
In `@web/src/lib/auth.ts`:
- Around line 145-149: Add a deadline to the email request in
web/src/lib/auth.ts:145-149, using the surrounding auth flow to catch timeout
failures and return its existing controlled error result. Also update the fetch
path in web/src/lib/control-plane-client.ts:56-59 to apply a default request
timeout while preserving any caller-provided cancellation signal; use the
relevant auth and control-plane client methods as implementation anchors.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c241a69-ae0e-4d02-a5c0-b0450ec0637c
⛔ Files ignored due to path filters (2)
web/package-lock.jsonis excluded by!**/package-lock.jsonweb/src/lib/control-plane-types.generated.tsis excluded by!**/*.generated.*
📒 Files selected for processing (33)
.github/workflows/ci.yml.gitignore.pre-commit-config.yamlCHANGELOG.mdREADME.mdloafer/adapters/metadata.pyloafer/adapters/metadata_schema.pyloafer/application/durable.pyloafer/cli.pyloafer/control_plane/app.pyloafer/control_plane/auth.pyloafer/control_plane/client.pyloafer/control_plane/daemon.pyloafer/control_plane/repository.pyloafer/control_plane/service.pyloafer/ports/metadata.pyopenapi/control-plane-v1.jsonscripts/generate_control_plane_contract.pyscripts/git-hooks/README.mdscripts/git-hooks/pre-commitscripts/smoke_test.shskills/loafer-self-hosting/SKILL.mdskills/loafer-self-hosting/references/deployment-contract.mdtests/e2e/test_cli_run.pytests/integration/test_metadata_store.pytests/unit/test_control_plane_api.pytests/unit/test_control_plane_transactions.pytests/unit/test_metadata_store.pyweb/package.jsonweb/src/content/docs/control-plane.mdxweb/src/lib/auth-client.tsweb/src/lib/auth.tsweb/src/lib/control-plane-client.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- web/package.json
- .github/workflows/ci.yml
- web/src/content/docs/control-plane.mdx
- loafer/control_plane/daemon.py
- scripts/generate_control_plane_contract.py
- loafer/control_plane/client.py
- openapi/control-plane-v1.json
- README.md
- loafer/adapters/metadata_schema.py
- loafer/control_plane/auth.py
- loafer/control_plane/repository.py
- loafer/control_plane/service.py
- loafer/control_plane/app.py
| def upsert_schedule( | ||
| self, | ||
| schedule: ScheduleRecord, | ||
| *, | ||
| connection: Connection | None = None, | ||
| ) -> ScheduleRecord: | ||
| with self._transaction(connection) as connection: | ||
| self._set_workspace_scope(connection, schedule.workspace_id) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 12 'schedules|workspace_id|UniqueConstraint|PrimaryKeyConstraint' \
loafer/adapters/metadata_schema.py
rg -n -C 10 'upsert_schedule|schedule_id' loafer/control_plane loafer testsRepository: lupppig/loafer
Length of output: 50370
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo '--- adapter metadata upsert_schedule context ---'
sed -n '1,40p;560,660p' loafer/adapters/metadata.py
echo '--- metadata exceptions/import context ---'
sed -n '1,120p;200,280p' loafer/adapters/metadata.py
echo '--- service scheduling / replace path context ---'
sed -n '188,216p' loafer/scheduler.py
sed -n '382,412p' loafer/control_plane/service.py
echo '--- tests around duplicate schedules and replace/upsert behavior ---'
rg -n -C 8 'test_.*schedule|schedule_id|IdempotencyConflict|Replace|replace|IntegrityError|upsert_schedule' tests loafer | head -n 240Repository: lupppig/loafer
Length of output: 30838
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo '--- control_plane app exception handling context ---'
sed -n '1,110p' loafer/control_plane/app.py
echo '--- control_plane service imports and conflict handling ---'
sed -n '1,90p' loafer/control_plane/service.py
rg -n -C 12 'IdempotencyConflictError|Conflic|upsert_schedule|schedule.upsert|except' loafer/control_plane/app.py loafer/control_plane/service.py
echo '--- behavioral probe of query shape under PostgreSQL RLS policy ---'
python3 - <<'PY'
from pathlib import Path
p = Path("loafer/adapters/metadata.py")
text = p.read_text()
lines = text.splitlines()
print("upsert_schedule_workspace_scope_before_existing_lookup", any("_set_workspace_scope(connection, schedule.workspace_id)" in line for line in lines))
print("select_loafer_schedules_by_id_inside_upsert_schedule", any("select(schema.schedules).where(schema.schedules.c.id == schedule.id)" in line for line in lines))
print("workspaces_scope_RLS_in_schema", any("loafer_schedules" in line and "workspace_id = current_setting" in next((lines[i+j] for j in range(5) if i+j < len(lines) and "loafer_schedules" in line or "policy_name" in lines[i+j]), "") for i,line in enumerate(lines) if "loafer_schedules" in line and "CREATE POLICY" in line))
PYRepository: lupppig/loafer
Length of output: 35582
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo '--- full upsert_schedule and exception handling in control_plane/service.py and app.py exception mapping ---'
sed -n '120,200p' loafer/control_plane/service.py
sed -n '206,222p' loafer/control_plane/app.py
sed -n '160,182p' loafer/control_plane/service.py
sed -n '232,244p' loafer/control_plane/service.pyRepository: lupppig/loafer
Length of output: 5072
Handle non-authorized schedule ID conflicts before insert.
upsert_schedule sets the current workspace idempotently, so Select * loafer_schedules WHERE id = $1 hides same-ID rows owned by another workspace. The existing row is unread, the insert runs, and the global schedules.id primary key raises an uncaught IntegrityError. Use an authorized workspace-scoped existence check/unique key or translate the violation to IdempotencyConflictError.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@loafer/adapters/metadata.py` around lines 602 - 609, Update upsert_schedule
to detect an existing schedule ID outside the authorized workspace before
attempting the insert, using an authorized workspace-scoped existence or
uniqueness check. If such a conflict is found, raise or translate it to
IdempotencyConflictError instead of allowing the global schedules.id
IntegrityError to escape.
| """Compose a worker process; callers own its long-running lifecycle.""" | ||
| return DurableWorker( | ||
| get_metadata_store(metadata_url), | ||
| _get_ready_metadata_store(metadata_url), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Close the metadata store when worker composition fails.
Line [128] opens the metadata store before Python evaluates get_object_storage(object_root) and constructs DurableWorker. If either operation raises, no worker owns the store, so its engine is not disposed.
Proposed cleanup
- return DurableWorker(
- _get_ready_metadata_store(metadata_url),
+ metadata = _get_ready_metadata_store(metadata_url)
+ try:
+ return DurableWorker(
+ metadata,
get_object_storage(object_root),
worker_id=worker_id,
- )
+ )
+ except Exception:
+ metadata.close()
+ raise📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _get_ready_metadata_store(metadata_url), | |
| metadata = _get_ready_metadata_store(metadata_url) | |
| try: | |
| return DurableWorker( | |
| metadata, | |
| get_object_storage(object_root), | |
| worker_id=worker_id, | |
| ) | |
| except Exception: | |
| metadata.close() | |
| raise |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@loafer/application/durable.py` at line 128, Update the worker composition
flow around _get_ready_metadata_store and DurableWorker so the metadata store is
closed when get_object_storage or DurableWorker construction raises before
ownership transfers to the worker. Ensure successful construction preserves the
existing ownership and lifecycle behavior.
| const response = await fetch(authEmailEndpoint, { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ kind, email, action_url: actionUrl }), | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound all new fetch operations with deadlines.
Both fetch paths can remain pending indefinitely when an upstream service stalls.
web/src/lib/auth.ts#L145-L149: add an email-delivery timeout and return a controlled error.web/src/lib/control-plane-client.ts#L56-L59: add a default request timeout while preserving caller cancellation.
📍 Affects 2 files
web/src/lib/auth.ts#L145-L149(this comment)web/src/lib/control-plane-client.ts#L56-L59
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/lib/auth.ts` around lines 145 - 149, Add a deadline to the email
request in web/src/lib/auth.ts:145-149, using the surrounding auth flow to catch
timeout failures and return its existing controlled error result. Also update
the fetch path in web/src/lib/control-plane-client.ts:56-59 to apply a default
request timeout while preserving any caller-provided cancellation signal; use
the relevant auth and control-plane client methods as implementation anchors.
1 participant
Summary
loaferdcontrol plane with versioned/api/v1resources, durable commands, RFC 9457-style errors, sequenced SSE events, OpenAPI, and generated browser typesloafer enqueueusesloaferd, while embedded enqueue and inline execution require explicit--localWhy
Loafer needs one authenticated point of control for the CLI, web application, and automation without coupling HTTP availability to pipeline execution. This establishes the Docker-daemon-like interface boundary discussed in the architecture: every remote client uses the same HTTPS API, while schedulers and workers remain separate processes over authoritative metadata.
The implementation intentionally does not add a Unix socket, browser-specific control API, direct browser-to-worker path, or silent fallback to embedded execution.
Developer and operator impact
loaferdis available as a new package entry point and requires direct TLS certificates or an explicitly trusted TLS proxy.loafer run ... --local.LOAFER_API_URL,LOAFER_WORKSPACE_ID, and eitherLOAFER_AUTH_URLor a short-livedLOAFER_ACCESS_TOKEN.Security boundaries
loaferdaccepts signed bearer tokens with pinned issuer, audience, expiry, and supported asymmetric algorithms.loaferdmutations require bearer credentials.Validation
uv run ruff check .uv run ruff format --check .uv run pytest tests/unit tests/e2e -q- 742 passed, 11 skippedDeferred work
loaferd.Summary by CodeRabbit
New Features
Documentation
Bug Fixes
--local, preventing unintended fallback behavior.