mirror of
https://github.com/OpenHands/OpenHands.git
synced 2026-10-07 13:38:55 +08:00
docs: add API access rules to AGENTS.md and code review skill (#506)
Document the two mandatory API access conventions that are enforced by the CI test src/api/no-direct-agent-server-calls.test.ts: 1. All agent-server calls must use typed @openhands/typescript-client classes (ConversationClient, FileClient, VSCodeClient, ServerClient, RemoteWorkspace, RemoteEventsList) instantiated via getAgentServerClientOptions() -- never raw axios/fetch. 2. All cloud SaaS and runtime-sandbox calls must go through callCloudProxy() in src/api/cloud/proxy.ts to avoid CORS, using hostOverride for runtime-sandbox URLs and authMode='session-api-key' for those endpoints. AGENTS.md gets a full '## API Access Rules' section with client listings, option helper references, CORRECT/WRONG code examples, and the allowed-exceptions list. The custom-codereview-guide.md skill gets a '## Frontend API Access Conventions' section with DO NOT APPROVE triggers, forbidden pattern lists, correct examples, and a note about the silent hostOverride bug. Co-authored-by: openhands <openhands@all-hands.dev>
This commit is contained in:
Rohit Malhotra
committed by
GitHub
GitHub
co-authored by
openhands
openhands
parent
acb04fd9ba
commit
43e6db6919
@@ -168,6 +168,78 @@ pydantic_core.ValidationError: Extra inputs are not permitted
|
||||
|
||||
**This is a production-breaking change.** Do not approve PRs that modify event types without proper backward compatibility handling and tests.
|
||||
|
||||
## Frontend API Access Conventions
|
||||
|
||||
These two rules are enforced by the CI test `src/api/no-direct-agent-server-calls.test.ts`.
|
||||
**Flag any PR that introduces a violation** -- these are correctness bugs, not style nits.
|
||||
|
||||
### Rule 1 -- All agent-server calls must use `@openhands/typescript-client`
|
||||
|
||||
**DO NOT APPROVE** a PR that introduces raw `axios`, `fetch`, or the shared `openHands`
|
||||
axios instance to call an agent-server endpoint (`/api/*`, `/server_info`). All such
|
||||
calls must go through typed client classes from `@openhands/typescript-client`,
|
||||
instantiated with options from `getAgentServerClientOptions()` or
|
||||
`getAgentServerHttpClientOptions()` in `src/api/agent-server-client-options.ts`.
|
||||
|
||||
Forbidden patterns (caught by the CI guard):
|
||||
- `openHands.<method>(...)` -- shared axios instance
|
||||
- `createHttpClient(...)` -- creates a raw HTTP client
|
||||
- `axios(...)` / `axios.get/post/etc.(...)` (except in the two allowed files)
|
||||
- `fetch('/api/...')` or `fetch(\`${host}/api/...\`)`
|
||||
|
||||
Correct pattern:
|
||||
```ts
|
||||
new ConversationClient(getAgentServerClientOptions()).getConversation(id)
|
||||
new FileClient(getAgentServerClientOptions()).downloadTextFile(path)
|
||||
new ServerClient(getAgentServerHttpClientOptions()).getServerInfo()
|
||||
new RemoteWorkspace(getAgentServerClientOptions()).gitChanges({ ref: "HEAD" })
|
||||
```
|
||||
|
||||
Allowed exceptions (explicitly listed in `ALLOWED_AD_HOC_HTTP_FILES`):
|
||||
- `src/api/automation-service/automation-service.api.ts`
|
||||
- `src/api/cloud/proxy.ts`
|
||||
|
||||
If a PR adds a new file to `ALLOWED_AD_HOC_HTTP_FILES` without a strong reason,
|
||||
flag it -- the allowlist should not grow casually.
|
||||
|
||||
### Rule 2 -- All cloud backend calls must go through `callCloudProxy`
|
||||
|
||||
**DO NOT APPROVE** a PR that issues a direct browser `fetch` or `axios` call to the
|
||||
cloud SaaS backend (`app.all-hands.dev`) or a cloud runtime sandbox
|
||||
(`*.prod-runtime.all-hands.dev`). Both origins block CORS from `localhost`. Cloud calls
|
||||
must go through `callCloudProxy()` in `src/api/cloud/proxy.ts`, which routes them
|
||||
server-side through `/api/cloud-proxy` on the local agent-server.
|
||||
|
||||
Correct pattern -- cloud SaaS:
|
||||
```ts
|
||||
callCloudProxy({ backend, method: "GET", path: "/api/v1/app-conversations/search?..." })
|
||||
```
|
||||
|
||||
Correct pattern -- cloud runtime sandbox (use `hostOverride` + `authMode: "session-api-key"`):
|
||||
```ts
|
||||
callCloudProxy({
|
||||
backend,
|
||||
method: "GET",
|
||||
hostOverride: buildHttpBaseUrl(conversationUrl),
|
||||
path: `/api/conversations/${id}`,
|
||||
authMode: "session-api-key",
|
||||
sessionApiKey,
|
||||
})
|
||||
```
|
||||
|
||||
Standard branch structure every cloud-aware service method should follow:
|
||||
```ts
|
||||
if (getActiveBackend().backend.kind === "cloud") {
|
||||
return callCloudProxy({ backend: active, ... });
|
||||
}
|
||||
// local path: typed typescript-client
|
||||
return new ConversationClient(getAgentServerClientOptions()).someMethod(...);
|
||||
```
|
||||
|
||||
Missing the `hostOverride` on a runtime-sandbox call is a silent bug: the proxy
|
||||
will target `backend.host` (the SaaS API) instead of the actual runtime URL.
|
||||
Flag any `callCloudProxy` call that targets a runtime URL without `hostOverride`.
|
||||
|
||||
## SDK Architecture Conventions
|
||||
|
||||
These conventions codify patterns that are easy to violate when adding new features. Each was learned from a real bug.
|
||||
|
||||
@@ -159,6 +159,94 @@ you are running inside of — NOT the automation backend.
|
||||
|
||||
- `@openhands/typescript-client` is currently pinned to commit `ef62e82fc3dfb03991a1c8025429caf354427263` because the package metadata needed by this PR has not been published as a consistent npm/tagged release yet. That commit ships the needed typed clients plus subpath exports for `client/http-client`, `events/remote-events-list`, and `workspace/remote-workspace`. `RemoteWorkspace.gitChanges`/`gitDiff` accept an optional `{ ref }` option; agent-canvas passes `'HEAD'` so the changes panel reflects working-tree + index versus the latest commit (i.e. staged + unstaged) instead of a diff against the upstream/default branch.
|
||||
- The `@openhands/typescript-client` git dep must be expressed as a `git+https://github.com/...` URL in both `package.json` and the top-level dep entry of `package-lock.json`; the `github:OpenHands/...` shorthand normalizes to `git+ssh://` inside the lockfile, and Vercel's build environment has no GitHub SSH key, so an ssh-pinned lockfile makes Vercel fall back to a stale cached tarball and the bundler then fails with `[MISSING_EXPORT] ConversationClient/FileClient/SharedClient is not exported by .../dist/clients.js`. `scripts/vercel-install.sh` (wired up via `vercel.json`'s `installCommand`) defensively rewrites any leftover `git+ssh://git@github.com/` resolved URLs to `git+https://github.com/` and adds matching `git config --global url..insteadOf` aliases before invoking `npm ci`, so a future regression that re-introduces an ssh-pinned lockfile entry still builds on Vercel. See GitHub issue #384 for the original failure and PR #382 for the prior single-shot lockfile fix that this generalizes.
|
||||
## API Access Rules
|
||||
|
||||
Two strict conventions govern every REST call in the frontend. Violations break CI
|
||||
via `src/api/no-direct-agent-server-calls.test.ts`.
|
||||
|
||||
### Rule 1 -- Agent-server calls must use `@openhands/typescript-client`
|
||||
|
||||
All calls that target the local agent-server (`/api/*`, `/server_info`, `/sockets`)
|
||||
**must** go through typed client classes from `@openhands/typescript-client`, **never**
|
||||
raw `axios`, `fetch`, or the legacy shared `openHands` axios instance.
|
||||
|
||||
Available clients and their subpath imports:
|
||||
- `ConversationClient` -- `@openhands/typescript-client/clients`
|
||||
- `FileClient` -- `@openhands/typescript-client/clients`
|
||||
- `VSCodeClient` -- `@openhands/typescript-client/clients`
|
||||
- `ServerClient` -- `@openhands/typescript-client/clients`
|
||||
- `HttpClient` -- `@openhands/typescript-client/client/http-client`
|
||||
- `RemoteWorkspace` -- `@openhands/typescript-client/workspace/remote-workspace`
|
||||
- `RemoteEventsList` -- `@openhands/typescript-client/events/remote-events-list`
|
||||
|
||||
Client options are always assembled via helpers in `src/api/agent-server-client-options.ts`:
|
||||
- `getAgentServerClientOptions(overrides?)` -- for SDK client constructors
|
||||
- `getAgentServerHttpClientOptions(overrides?)` -- for `HttpClient`-based callers
|
||||
|
||||
These helpers read host, session API key, and working directory from the active backend
|
||||
registry and env config, so callers never hardcode URLs or auth tokens.
|
||||
|
||||
```ts
|
||||
// CORRECT
|
||||
const data = await new ConversationClient(getAgentServerClientOptions()).getConversation(id);
|
||||
const file = await new FileClient(getAgentServerClientOptions()).downloadTextFile(path);
|
||||
|
||||
// WRONG -- raw axios/fetch calls fail the no-direct-agent-server-calls.test.ts guard
|
||||
const data = await axios.get(`${host}/api/conversations/${id}`);
|
||||
const data = await fetch(`/api/conversations/${id}`);
|
||||
```
|
||||
|
||||
**Allowed exceptions** (files that may use axios directly for infrastructure reasons):
|
||||
- `src/api/automation-service/automation-service.api.ts`
|
||||
- `src/api/cloud/proxy.ts` -- the proxy envelope POST itself
|
||||
|
||||
### Rule 2 -- Cloud backend routes must go through `callCloudProxy`
|
||||
|
||||
Any call from the browser to the cloud SaaS backend (`app.all-hands.dev`) or a cloud
|
||||
runtime sandbox (`*.prod-runtime.all-hands.dev`) **must** go through `callCloudProxy()`
|
||||
in `src/api/cloud/proxy.ts`. These origins do not permit CORS from `localhost`;
|
||||
`callCloudProxy` POSTs the request envelope to `/api/cloud-proxy` on the local
|
||||
agent-server, which forwards it server-side.
|
||||
|
||||
```ts
|
||||
import { callCloudProxy } from "../cloud/proxy";
|
||||
|
||||
// CORRECT -- cloud SaaS endpoint
|
||||
const result = await callCloudProxy<ResponseType>({
|
||||
backend,
|
||||
method: "GET",
|
||||
path: `/api/v1/app-conversations/search?${params}`,
|
||||
});
|
||||
|
||||
// CORRECT -- cloud runtime sandbox, auth via session key
|
||||
const result = await callCloudProxy<ResponseType>({
|
||||
backend,
|
||||
method: "GET",
|
||||
hostOverride: buildHttpBaseUrl(conversationUrl),
|
||||
path: `/api/git/changes?path=${path}`,
|
||||
authMode: "session-api-key",
|
||||
sessionApiKey,
|
||||
});
|
||||
|
||||
// WRONG -- direct fetch/axios to a cloud host is blocked by CORS in the browser
|
||||
const result = await axios.get(`${backend.host}/api/v1/app-conversations`);
|
||||
```
|
||||
|
||||
`callCloudProxy` key options:
|
||||
- `backend` -- the cloud `Backend` object (provides host and bearer token)
|
||||
- `hostOverride` -- override for runtime-sandbox calls; replaces `backend.host`
|
||||
- `authMode` -- `"bearer"` (default, cloud SaaS) | `"session-api-key"` (runtime sandbox) | `"none"`
|
||||
- `sessionApiKey` -- required when `authMode === "session-api-key"`
|
||||
|
||||
Standard cloud/local branch pattern used throughout the service layer:
|
||||
|
||||
```ts
|
||||
if (getActiveBackend().backend.kind === "cloud") {
|
||||
return callCloudProxy({ backend: active, ... });
|
||||
}
|
||||
return new ConversationClient(getAgentServerClientOptions()).someMethod(...);
|
||||
```
|
||||
|
||||
- Use `@openhands/typescript-client` classes directly for agent-server-backed REST/workspace/event/VS Code calls. Centralize host/session API key/working-directory option assembly through `src/api/agent-server-client-options.ts`; the backend fallback policy itself lives in `src/api/backend-registry/active-store.ts`.
|
||||
- Local verification/build gotchas:
|
||||
- `npm run typecheck` assumes generated translation types exist; run `npm run make-i18n` first if `src/i18n/declaration.ts` is missing.
|
||||
|
||||
Reference in New Issue
Block a user