codegen: support body/manual operation kinds and nested namespaces#54
codegen: support body/manual operation kinds and nested namespaces#54jadenfix wants to merge 1 commit into
Conversation
Groundwork for migrating the hand-written facades (agents.py, policies.py,
users.py) to contract-generated code:
- kind `body`: any single HTTP call — path/query/body parameter locations,
UUID coercion, pass_unset_when_none, or_empty list/dict defaults,
inject_organization_id body insertion, return_shape list with typed
error label, refetch_with_retrieve (create-then-retrieve), no-content
returns. Conventions mirror the hand-written facades exactly
(request_raw/from_dict for GETs, request_json/.parsed for mutations).
- kind `manual`: never generated; codegen asserts the method exists in the
hand-written facade and fails loudly listing all missing methods.
- namespaces: generated nested classes proxy _raw/_org_id to the parent,
wired via ctor + property, mirroring AgentVersionsAPI.
- split mode: APIs with manual ops emit base classes to
src/roe/api/_{api}_generated.py (public class stays hand-written);
all-manual APIs (users) are parity-checked only; whole-file APIs
(discovery, tables) are byte-identical to before.
scripts/fixtures/candidate_wrappers.yml is the migration parity fixture:
every signature derived from the hand-written facades; AST comparison of
generated vs hand-written shows 22/22 body methods signature-identical.
Production no-op: regenerating from the committed openapi/wrappers.yml
produces zero diff.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Greptile SummaryThis PR extends
Confidence Score: 3/5Safe to merge today (no production code changes), but the candidate fixture carries a return-type mismatch that will silently change the public API surface when B2 activates it. The generator itself is well-structured and currently a no-op against scripts/fixtures/candidate_wrappers.yml — verify Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[main] --> B[_generate_modules]
B --> C[_check_manual_parity]
C -->|facade file missing or method absent| E[ManualWrapperParityError]
C -->|all present| D{_spec_has_manual?}
D -->|yes + has generated ops| F[_render_module split=True\n→ _api_generated.py]
D -->|yes + all manual| G[no file emitted\nparity-checked only]
D -->|no| H[_render_module split=False\n→ api.py]
F --> I{operation kind}
H --> I
I -->|body| J[_scan_body_operation\n+ _body_kind_method]
I -->|manual| K[skip generation]
I -->|simple / table_upload| L[existing handlers]
J --> M{refetch_with_retrieve?}
M -->|yes| N[request_raw + hardcoded version_id\n→ self.retrieve]
M -->|no + body_type| O[request_json → resp.parsed]
M -->|no + return_shape=list| P[request_raw → list comprehension]
M -->|no + return_type| Q[request_raw → .from_dict]
M -->|no + none| R[request_raw void]
B --> S[_render_registry\nwhole-file APIs only]
Reviews (1): Last reviewed commit: "codegen: support body/manual operation k..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return_import: roe._generated.models.agent_job_status.AgentJobStatus | ||
| parameters: | ||
| - name: job_id | ||
| location: path | ||
| annotation: str | ||
| coerce: uuid | ||
| - kind: body | ||
| method_name: retrieve_result | ||
| docstring: '' | ||
| method: GET | ||
| path: /v1/agents/jobs/{agent_job_id}/result/ | ||
| endpoint_module: roe._generated.api.agents.agents_jobs_result_retrieve | ||
| return_type: AgentJobResultResponse | ||
| return_import: roe._generated.models.agent_job_result_response.AgentJobResultResponse | ||
| parameters: | ||
| - name: job_id | ||
| location: path | ||
| annotation: str | ||
| coerce: uuid | ||
| - kind: body | ||
| method_name: cancel | ||
| docstring: '' | ||
| method: POST | ||
| path: /v1/agents/jobs/{job_id}/cancel/ | ||
| endpoint_module: roe._generated.api.agents.agents_jobs_cancel_create | ||
| parameters: |
There was a problem hiding this comment.
retrieve_status return type contradicts the hand-written facade
The fixture declares return_type: AgentJobStatus, but AgentJobsAPI.retrieve_status in agents.py (line 234-241) returns AgentJobSingleStatus and calls AgentJobSingleStatus.from_dict(response.json()). The fixture's own header states "swapping this file into openapi/wrappers.yml and running scripts/generate_wrappers.py must reproduce the existing public surface exactly." Activating this in B2 will silently change the method's return type, breaking any caller that type-checks against AgentJobSingleStatus.
| refetch_args = ", ".join(param["name"] for param in path_params) | ||
| lines.append(_request_call("response = ", "request_raw")) | ||
| lines.append( | ||
| " data = response.json()\n" | ||
| ' version_id = data.get("id") if isinstance(data, dict) else None\n' | ||
| " if version_id is None:\n" | ||
| " raise RoeAPIException(\n" | ||
| ' f"Unexpected response from server: status={response.status_code}"\n' | ||
| " )\n" | ||
| " # POST returns a partial create payload; re-fetch to get the full version.\n" | ||
| f" return self.retrieve({refetch_args}, str(version_id))\n" | ||
| ) |
There was a problem hiding this comment.
version_id local variable can shadow a path parameter
The refetch path always introduces a local variable named version_id = data.get("id"). Any future refetch_with_retrieve operation that includes a path parameter also named version_id will have that parameter silently shadowed: the generated method will receive version_id as an argument, then immediately overwrite it with the server response value before the self.retrieve(...) call. The current fixture avoids this only because agents.versions.create has agent_id as its sole path parameter, but the generator itself provides no guard against the collision.
| elif return_shape == "list": | ||
| label = operation["list_error_label"] |
There was a problem hiding this comment.
list_error_label produces a cryptic KeyError
When return_shape: list is specified but list_error_label is absent from the operation spec, operation["list_error_label"] raises a bare KeyError with no useful diagnostic. The generator already validates other constraints (e.g. return_type required, body_type forbidden) before reaching this line; adding an explicit check here keeps error messages consistent.
| elif return_shape == "list": | |
| label = operation["list_error_label"] | |
| elif return_shape == "list": | |
| if "list_error_label" not in operation: | |
| raise ValueError( | |
| f"return_shape: list requires list_error_label " | |
| f"(operation {method_name!r})" | |
| ) | |
| label = operation["list_error_label"] |
1 participant
Phase B1 of the SDK facade centralization (roe-main #3456 is the contract-side groundwork). Teaches
scripts/generate_wrappers.pyto consume the upcoming wrapper shapes:bodykind — any single HTTP call (path/query/body params, optional request model, optional empty response), withor_empty,return_shape: list,refetch_with_retrieve,coerce: uuid, andinject_organization_idconventions matching the existing hand-written facades.manualkind — generator skips it but asserts the method still exists in the hand-written facade source (drift check).namespaces— nested accessors (client.agents.versions.*)._{api}_generated.pybase classes; the hand-written facade subclasses them. All-manual APIs are parity-checked only.scripts/fixtures/candidate_wrappers.ymlis the parity-verified rendering of the unified contract for this repo — roe-main's full-cycle test (test_b2_draft_contract_renders_the_language_repo_fixtures) asserts its renderer reproduces it exactly.No-op today:
openapi/wrappers.ymlstill only carriessimple/table_uploadops, so generated output is unchanged until roe-main flips the contract (B2). Rebased on v1.1.0 (kept the release-banner sync inmain()).Test Plan
uv run pytest— 43/43 (includes newtests/unit/test_generate_wrappers.pycodegen suite against the candidate fixture)openapi/wrappers.yml(existing goldens unchanged)bodymethods are signature-identical to the 22 hand-written facade methods they will replace🤖 Generated with Claude Code