Repository navigation
refactor(http): extract Agent and native-admin handlers - #873
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex review: needs maintainer review before merge. Reviewed October 2, 2026, 7:15 AM ET / 11:15 UTC (Revision 2). ClawSweeper reviewWhat this changesThe 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 Review scores
Verification
How this fits togetherThe 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
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest 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. LabelsLabel changes: No label changes. Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
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.
|
Thanks for this refactor. I merged origin/main into the branch (9170fed) to clear the conflicts. Main changed |
The controller HTTP entry point mixed shared admission with Agent resource handlers and native-admin proxy lifecycle. Extract Agent handlers into
http/agents.tsand native-admin admission, availability, sockets, and shutdown auditing intohttp/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
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, andpnpm check:modulespassed.pnpm openapi:checkpassed with no generated contract changes.pnpm docs:checkand structural validation of all four changed flow documents passed; Manual Notes preserved.Independent Codex review of
6ba4cceaaagainst7a6cc931dcompleted 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.