Skip to content

DO NOT MERGE: Extract API composition root#1355

Draft
simple-agent-manager[bot] wants to merge 2 commits into
mainfrom
sam/task-api-composition-root-01kvcz
Draft

DO NOT MERGE: Extract API composition root#1355
simple-agent-manager[bot] wants to merge 2 commits into
mainfrom
sam/task-api-composition-root-01kvcz

Conversation

@simple-agent-manager

Copy link
Copy Markdown
Contributor

DO NOT MERGE / DO NOT DEPLOY TO STAGING

This PR is intentionally draft/do-not-merge per human instruction. Do not deploy this branch to staging, do not mark ready for review, and do not merge unless Raphaël explicitly authorizes it later.

Summary

  • Extracted apps/api/src/index.ts into explicit app modules under apps/api/src/app/ while keeping Durable Object runtime exports in index.ts.
  • Added named app owners for error handling, Pages proxy, workspace proxy, global middleware, public routes, well-known endpoints, API route groups, MCP CORS/route registration, 404 handling, and scheduled handling.
  • Encoded route-order constraints in named registration functions, including auth precedence routes and project callback routes before session-auth project routes.
  • Split MCP CORS policy from MCP route mounting so /mcp preflight is handled before global credentialed CORS while normal MCP requests still pass through global middleware.

Validation

  • pnpm --filter @simple-agent-manager/api test — passed: 330 files, 5470 tests.
  • pnpm --filter @simple-agent-manager/api typecheck — passed.
  • pnpm --filter @simple-agent-manager/api lint — passed with existing warnings.
  • Focused composition tests — passed.
  • pnpm --filter @simple-agent-manager/api test:workers — attempted twice; blocked before test execution by workerd signal 11 segmentation faults. Vitest reported 32 Cloudflare pool errors and 0 tests executed.

Staging Verification

  • Staging deployment green — NOT RUN. Human explicitly forbade staging deployment.
  • Live app verified via Playwright — NOT RUN. Human explicitly forbade staging deployment.
  • Existing workflows confirmed working — NOT RUN on staging. Local API tests passed.
  • New feature/fix verified on staging — NOT RUN. Human explicitly forbade staging deployment.
  • Infrastructure verification completed — N/A: no infrastructure paths changed.
  • Mobile and desktop verification notes added — N/A: no UI changes.

Staging Verification Evidence

No staging deployment was performed. This is intentional and required by the task instructions.

UI Compliance Checklist

  • N/A: no UI changes.

End-to-End Verification

  • Composition data/control flow traced through local behavior tests and source-contract tests.
  • Capability tests exercise app-level route registration for well-known endpoints, 404, CORS, proxy pass-through/intercept, and scheduled delegation.
  • Existing route precedence tests continue covering auth and callback ordering.

Data Flow Trace

  • Cloudflare entrypoint: apps/api/src/index.ts exports DO classes and delegates fetch to createApiApp() and scheduled to handleScheduled().
  • App construction: apps/api/src/app/create-api-app.ts registers proxy, middleware, public/well-known routes, ordered API route groups, MCP, and 404.
  • Route ordering: apps/api/src/app/register-routes.ts owns auth precedence and project callback-before-session-auth groups.
  • Scheduled dispatch: apps/api/src/app/scheduled.ts owns cron classification and job delegation.

Untested Gaps

Worker runtime tests could not execute because workerd crashed with signal 11 on two attempts. No staging verification was performed by explicit human constraint.

Post-Mortem

N/A: refactor plus behavior-preserving extraction, not a bug fix.

Specialist Review Evidence

  • All local specialist checklist reviews completed.
  • If any reviewer did NOT complete: N/A, no SAM-dispatched reviewers were launched.
Reviewer Status Outcome
task-completion-validator PASS Checklist maps to diff and tests; Worker test gap documented.
cloudflare-specialist PASS Worker entrypoint exports preserved; scheduled handler extracted; no D1/KV/R2 binding changes.
security-auditor PASS CORS/auth/proxy behavior preserved; MCP bearer CORS fixed for actual /mcp preflight without credential headers.
test-engineer PASS Added app composition and scheduled delegation tests; updated source-contract tests to new owners.

Agent Preflight

  • Preflight completed before code changes

Classification

  • external-api-change
  • cross-component-change
  • business-logic-change
  • public-surface-change
  • docs-sync-change
  • security-sensitive-change
  • ui-change
  • infra-change

External References

N/A: no external API/library behavior changes. Used SAM MCP get_instructions, SAM MCP get_idea, local task/audit context, and codebase rules.

Codebase Impact Analysis

Affected apps/api composition root, route registration tests, scheduled source-contract tests, and task record. No route module splitting and no API contract changes intended.

Documentation & Specs

N/A: internal refactor; task record updated. Public docs are not changed because user-facing behavior should be unchanged.

Constitution & Risk Check

Checked route ordering, auth/CORS behavior, and hardcoded-value risk. No new env vars or config values added. Main risk is Worker-runtime verification gap due workerd crash; local API test/type/lint coverage passed.

Exceptions

  • Scope: Worker runtime tests and staging verification.
  • Rationale: Worker tests crashed in workerd; staging deployment explicitly forbidden by human.
  • Expiration: Before merge readiness is considered.

@sonarqubecloud

Copy link
Copy Markdown

@simple-agent-manager

Copy link
Copy Markdown
Contributor Author

Behavioral Equivalence Audit — PR #1355 (API Composition Root Extraction)

This PR was reviewed for behavioral equivalence as part of the code elegance audit. Below is a summary of changes that go beyond pure restructuring.

MCP CORS Registration Ordering (Intentional Improvement)

Change: MCP CORS is now registered before global CORS middleware, whereas the original index.ts had MCP CORS registered after global CORS.

Why this is correct: In Hono, the first cors() middleware to handle an OPTIONS preflight short-circuits with a 204 response. If global CORS (which is credentialed and domain-scoped) runs first on /mcp paths, it rejects unknown origins and returns 204 with no CORS headers — preventing MCP's origin: '*' policy from ever running. By placing MCP CORS first, /mcp preflight requests correctly get origin: '*' with credentials: false.

The removeCredentialedCors middleware still strips any Access-Control-Allow-Credentials header that global CORS might add on non-preflight requests (POST, GET), so the belt-and-suspenders approach works correctly.

Verdict: This is a deliberate fix for a latent bug in the original ordering. The test at create-api-app.test.ts:84-97 validates this behavior.

All Other Changes

The remaining changes are pure structural extraction — splitting the monolithic index.ts into focused modules (errors.ts, middleware.ts, mcp.ts, public-routes.ts, well-known.ts, pages-proxy.ts, workspace-proxy.ts, register-routes.ts). Registration ordering within each group is preserved. No other behavioral changes detected.

Recommendation: Accept as-is. No fixes needed.

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