Skip to content

refactor(http): extract Agent and native-admin handlers - #873

Merged
freeqaz merged 2 commits into
mainfrom
dev/kevinlin/http-modular-handlers
Oct 2, 2026
Merged

freeqaz merged 2 commits into
mainfrom
dev/kevinlin/http-modular-handlers

Conversation

@kevinlin-openai

@kevinlin-openai kevinlin-openai commented Oct 2, 2026 •

Copy link
Copy Markdown
Collaborator

The controller HTTP entry point mixed shared admission with Agent resource handlers and native-admin proxy lifecycle. Extract Agent handlers into http/agents.ts and native-admin admission, availability, sockets, and shutdown auditing into http/native-admin.ts, using the existing resource-handler dispatch.

The entry point drops from 4,948 to 3,553 lines. Removing repeated input mapping, workspace validation, response projection, and dependency errors makes the complete runtime change 40 lines smaller (28 fewer lines including documentation). Closed request schemas still reject unknown fields, route parameters supply resource scope, and omitted/null settings retain their meaning. Deployment audit events retain the admitted authorization decision; the native-admin module reads the current controller after bootstrap. Update the four affected source-backed flows.

Verification

  • 104 tests passed, zero skipped: node --test tests/integration/occ-api.test.mjs tests/conformance/occ-api-security.test.mjs tests/integration/native-admin-access.test.mjs tests/integration/controller-lifecycle.test.mjs.
  • pnpm lint, pnpm format:check, pnpm typecheck, pnpm check:workspace, and pnpm check:modules passed.
  • pnpm openapi:check passed with no generated contract changes.
  • pnpm docs:check and structural validation of all four changed flow documents passed; Manual Notes preserved.
  • These suites exercise Fastify, in-memory platform state, native-admin proxy admission, and lifecycle behavior. Live PostgreSQL/Kubernetes/model execution was not run for this HTTP-only refactor.

Independent Codex review of 6ba4cceaa against 7a6cc931d completed as scoped-clean through P2, with no findings. The review inspected the complete change bundle and schema/source context; it did not rerun tests.

The existing long flow pages remain together because each documents one coherent lifecycle; this change refreshes their source ownership. Full GitHub CI is running separately, including the PostgreSQL-backed native-admin WebSocket lease and shutdown tests.

@kevinlin-openai
kevinlin-openai requested a review from a team as a code owner October 2, 2026 00:21
@clawsweeper

clawsweeper Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-02T00:24:35.826460Z 6ba4cce PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@clawsweeper clawsweeper Bot added P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Oct 2, 2026
@clawsweeper

clawsweeper Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed October 2, 2026, 7:15 AM ET / 11:15 UTC (Revision 2).

ClawSweeper review

What this changes

The PR moves Agent API handlers and native-admin proxy lifecycle management into separate HTTP modules and updates four runtime flow documents.

Merge readiness

✅ Ready for maintainer review

Keep open for normal maintainer triage. The extraction remains distinct from current main, and source comparison found no concrete introduced defect. The repository policy and collaborator authorship also preclude automatic closure.

Priority: P3
Reviewed head: 9170fed9621a94248b2cce9ec42b5c9a2ecc44e3

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused extraction with preserved contracts, relevant existing coverage, and no actionable correctness finding.
Proof confidence 🌊 off-meta tidepool Not applicable: The collaborator-authored refactor is exempt from ordinary external-contributor proof. Source comparison establishes no material authority change or stored-data contract change; reported Fastify and proxy integration results are supplemental and do not establish PostgreSQL, Kubernetes, or model execution.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The collaborator-authored refactor is exempt from ordinary external-contributor proof. Source comparison establishes no material authority change or stored-data contract change; reported Fastify and proxy integration results are supplemental and do not establish PostgreSQL, Kubernetes, or model execution.
Evidence reviewed 7 items Pinned patch and current-main comparison: The introduced delta contains five TypeScript files and four flow documents. Main still keeps Agent and native-admin handling in the entry point; its HTTP directory lacks the two extracted modules. The patch therefore remains useful as a structural refactor.
Native-admin behavior preserved: Compared the extracted functions with the original main blob. Session admission, identity resolution, exact-Agent authorization, proxy dispatch, WebSocket lease renewal, and audit functions retain their implementations. Controller, request-context, and admission lookups now use getters wired to the original live state.
Request scope and input semantics: Create, provision, and update schemas reject additional properties. Fastify disables removal of unknown properties, coercion, and defaults. The extracted handlers spread validated bodies before supplying route-owned namespace and Agent identifiers, preserving omitted versus explicit-null settings.
Findings None None.
Security None None.

How this fits together

The controller HTTP layer admits callers and routes Agent operations into the control plane. Native-admin requests pass through session and exact-Agent authorization checks before reaching the Agent gateway.

flowchart TB
A[HTTP requests] --> B[Caller admission]
B --> C[Agent handlers]
C --> D[Control plane operations]
D --> E[Audit evidence]
B --> F[Native admin authorization]
F --> G[Agent gateway proxy]
G --> E
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Entry-point reduction 5,009 → 3,604 lines The pinned comparison moves substantial handler ownership out of the controller entry point.
Production versus test delta production +1,498/-1,535 (net -37); tests +0/-0 The structural refactor reduces production code and relies on existing contract coverage.

Technical review

Best possible solution:

Keep shared admission in the HTTP entry point while the extracted modules own Agent handling and native-admin lifecycle without changing public contracts.

Do we have a high-confidence way to reproduce the issue?

Not applicable: this PR proposes a refactor rather than reporting a failing behavior; no concrete introduced failure was found.

Is this the best way to solve the issue?

Yes: extending the existing resource-handler dispatch and extracting native-admin ownership is a bounded solution that preserves shared admission and existing API contracts.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 9deec40f59fb.

Labels

Label changes:

No label changes.

Label justifications:

  • P3: This is a behavior-preserving structural cleanup with no demonstrated urgent user-facing failure.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The collaborator-authored refactor is exempt from ordinary external-contributor proof. Source comparison establishes no material authority change or stored-data contract change; reported Fastify and proxy integration results are supplemental and do not establish PostgreSQL, Kubernetes, or model execution.

Evidence

What I checked:

  • Pinned patch and current-main comparison: The introduced delta contains five TypeScript files and four flow documents. Main still keeps Agent and native-admin handling in the entry point; its HTTP directory lacks the two extracted modules. The patch therefore remains useful as a structural refactor. (apps/controller/src/index.ts:1314, 9170fed9621a)
  • Native-admin behavior preserved: Compared the extracted functions with the original main blob. Session admission, identity resolution, exact-Agent authorization, proxy dispatch, WebSocket lease renewal, and audit functions retain their implementations. Controller, request-context, and admission lookups now use getters wired to the original live state. (apps/controller/src/http/native-admin.ts:697, 9170fed9621a)
  • Request scope and input semantics: Create, provision, and update schemas reject additional properties. Fastify disables removal of unknown properties, coercion, and defaults. The extracted handlers spread validated bodies before supplying route-owned namespace and Agent identifiers, preserving omitted versus explicit-null settings. (packages/contracts/src/api/common.ts:604, 9170fed9621a)
  • Deployment and deletion audit preservation: The handler callback forwards the admitted deployment authorization into the existing audit factory. Agent deletion retains main's accessBindingsRemovedOnCompletion details. The existing deployment-audit regression checks decision provenance and transaction rollback after audit failure. (apps/controller/src/http/agents.ts:337, 9170fed9621a)
  • Validation scope and continuity: The captured PR body reports 104 passing tests with zero skips across API, security, native-admin, and lifecycle suites, plus static and documentation checks. Inspected existing assertions for exact-Agent permissions, forbidden requests not reaching the upstream, credential stripping, and deployment audit rollback. Tests were not executed during this read-only review. The previous completed review retained no findings or published rank-up moves. (tests/integration/native-admin-access.test.mjs:559, 9170fed9621a)
  • Verified merge identities: Raw commit records confirm the original head has parents 6ba4cce and pinned main. The test merge has pinned main followed by the exact PR head and shares the head's result tree; this is distinct from a merged fixing PR. (ce2128dc21dc)

Likely related people:

  • Kevin Lin: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • freeqaz: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (1 earlier review cycle)
  • reviewed 2026-10-02T00:28:36.753Z sha 6ba4cce :: needs maintainer review before merge. :: none

Resolve conflicts with main:
- Carry main's deleteAgent audit change (accessBindingsRemovedOnCompletion,
  from accessBindingsTargeting) into the moved handler in http/agents.ts,
  passing it as mutationEvent details; drop the now-unused import from index.ts.
- http/types.ts imports both DeployAgentAuthorization and RemovedAccessBinding
  and keeps removedAccessBindingDetails.
- controller-worker.md keeps both changelog entries.
@freeqaz

freeqaz commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Thanks for this refactor. I merged origin/main into the branch (9170fed) to clear the conflicts. Main changed deleteAgent in the meantime to record accessBindingsRemovedOnCompletion in its audit event, so I carried that into the moved handler in http/agents.ts (passed as mutationEvent details). http/types.ts now keeps both imports and removedAccessBindingDetails, and the controller-worker changelog keeps both entries. Typecheck and lint pass, the occ-api, occ-api-security, native-admin-access and controller-lifecycle suites pass locally, and CI Required is green on the new head.

@freeqaz
freeqaz merged commit e1e329b into main Oct 2, 2026
30 checks passed
@freeqaz
freeqaz deleted the dev/kevinlin/http-modular-handlers branch October 2, 2026 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants