.agents/skills/custom-codereview-guide.md
You are an expert code reviewer for the OpenHands/agent-canvas repository. This skill provides repo-specific review guidelines. Be direct but constructive.
You have permission to APPROVE or COMMENT on PRs. Do not use REQUEST_CHANGES.
Mandatory: Always submit exactly one PR review object before finishing. If you found no actionable issues, post a short APPROVE review rather than ending silently without posting a review. If you found actionable issues or concerns, post a COMMENT review.
Do NOT submit an APPROVE review when the PR changes agent behavior or anything that could plausibly affect benchmark/evaluation performance.
Examples include: prompt templates, tool calling/execution, planning/loop logic, memory/condenser behavior, terminal/stdin/stdout handling, or evaluation harness code.
If a PR is in this category (or you are uncertain), leave a COMMENT review and explicitly flag it for a human maintainer to decide after running lightweight evals.
Default to APPROVE: If your review finds no issues at "important" level or higher, approve the PR. Minor suggestions or nitpicks alone are not sufficient reason to withhold approval.
IMPORTANT: If you determine a PR is worth merging and it is not in the eval-risk category above, you should approve it. Don’t just say a PR is "worth merging" or "ready to merge" without actually submitting an approval. Your words and actions should be consistent.
Examples of straightforward and low-risk PRs you should approve (non-exhaustive):
DO NOT APPROVE PRs that have any of the following issues:
pyproject.toml file has changes to the version field (e.g., version = "1.12.0" → version = "1.13.0"), and the PR is NOT explicitly a release PR (title/description doesn't indicate it's a release), DO NOT APPROVE. Version numbers should only be changed in dedicated release PRs managed by maintainers.
version = "..." in any */pyproject.toml filestool.uv.exclude-newer caveat.Examples:
resolve_model_config.py or verified_models.py with corresponding test updatesUse COMMENT when you have feedback or concerns:
If there are significant issues, leave detailed comments explaining the concerns—but let a human maintainer decide whether to block the PR.
This repository intentionally uses a workspace-wide uv resolver guardrail:
pyproject.toml: [tool.uv] exclude-newer = "7 days"Important: Dependabot does not currently honor that uv guardrail when it opens uv.lock update PRs for this repo's workspace setup. A Dependabot PR can therefore bump to a version that was uploaded less than 7 days ago, even though a local uv lock would normally exclude it.
When reviewing dependency update PRs (uv.lock, pyproject.toml, requirements*.txt, etc.), explicitly check for too-new package uploads:
uv.lock, use the per-file upload-time metadata in the changed package entry.upload-time as the upload time of that specific distribution file to the package index (for example, the wheel uploaded to PyPI) — not the Git tag time or GitHub release time.If the updated package was uploaded within the last 7 days, treat it as a real security / supply-chain concern:
tool.uv.exclude-newer for this repo's workspace updates.Simplicity First: Question complexity. If something feels overcomplicated, ask "what's the use case?" and seek simpler alternatives. Features should solve real problems, not imaginary ones.
Pragmatic Testing: Test what matters. Avoid duplicate test coverage. Don't test library features (e.g., BaseModel.model_dump()). Focus on the specific logic implemented in this codebase.
Type Safety: Avoid # type: ignore - treat it as a last resort. Fix types properly with assertions, proper annotations, or code adjustments. Prefer explicit type checking over getattr/hasattr guards.
Backward Compatibility: Evaluate breaking change impact carefully. Consider API changes that affect existing users, removal of public fields/methods, and changes to default behavior.
# type: ignore usage, missing type annotations, getattr/hasattr guards, mocking non-existent argumentspyright not mypy, put fixtures in conftest.py, avoid sys.path.insert hacksLocalConversation that read or write self._state must use with self._state: — see the Concurrency section belowFor events received from the agent-server (REST history or WebSocket), the SDK Pydantic event model is the sole wire-contract authority. The TypeScript client must mirror that SDK contract, and Canvas must consume the client type.
Do not approve a PR when any of the following is true:
@openhands/typescript-client, including a partial redeclaration,
intersection type, module augmentation, or Canvas-only optional field.When an event contract changes, require this order and evidence in the PR:
Canvas-only presentation state belongs in a separate view-model, keyed by an event ID; it must never be appended to the wire-event interface.
When reviewing PRs that modify event types (e.g., TextContent, Message, Event, or any Pydantic model used in event serialization), DO NOT APPROVE until the following are verified:
Model validator present: If a field is being removed from an event type with extra="forbid", there MUST be a @model_validator(mode="before") that uses handle_deprecated_model_fields() to remove the deprecated field before validation. Otherwise, old events will fail to load.
Tests for backward compatibility: The PR MUST include tests that:
Test naming convention: The version in the test name should be the LAST version where a particular event structure exists. For example, if enable_truncation was removed in v1.11.1, the test should be named test_v1_10_0_... (the last version with that field), not test_v1_8_0_... (when it was introduced). This avoids duplicate tests and clearly documents when a field was last present.
Important: Deprecated field handlers are permanent and should never be removed. They ensure old conversations can always be loaded.
from openhands.sdk.utils.deprecation import handle_deprecated_model_fields
class MyModel(BaseModel):
model_config = ConfigDict(extra="forbid")
# Deprecated fields that are silently removed for backward compatibility
# when loading old events. These are kept permanently.
_DEPRECATED_FIELDS: ClassVar[tuple[str, ...]] = ("old_field_name",)
@model_validator(mode="before")
@classmethod
def _handle_deprecated_fields(cls, data: Any) -> Any:
"""Remove deprecated fields for backward compatibility with old events."""
return handle_deprecated_model_fields(data, cls._DEPRECATED_FIELDS)
Production systems resume conversations that may contain events serialized with older SDK versions. If the SDK can't load old events, users will see errors like:
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.
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.
@openhands/typescript-clientDO 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 instancecreateHttpClient(...) -- creates a raw HTTP clientaxios(...) / axios.get/post/etc.(...) (except in the two allowed files)fetch('/api/...') or fetch(\${host}/api/...`)`Correct pattern:
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.tssrc/api/cloud/proxy.tsIf a PR adds a new file to ALLOWED_AD_HOC_HTTP_FILES without a strong reason,
flag it -- the allowlist should not grow casually.
callCloudProxyDO NOT APPROVE a PR that issues a direct browser fetch or axios call to the
cloud 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:
callCloudProxy({ backend, method: "GET", path: "/api/v1/app-conversations/search?..." })
Correct pattern -- cloud runtime sandbox (use hostOverride + authMode: "session-api-key"):
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:
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 cloud API) instead of the actual runtime URL.
Flag any callCloudProxy call that targets a runtime URL without hostOverride.
These conventions codify patterns that are easy to violate when adding new features. Each was learned from a real bug.
LocalConversation protects mutable state with a FIFOLock accessed via with self._state:. Every method that reads or writes self._state.events, self._state.stats, self._state.agent_state, self._state.activated_knowledge_skills, or any other mutable field on ConversationState must hold this lock. There are currently ~13 call sites using this pattern.
When reviewing a PR that adds a new method to LocalConversation:
self._state.* field.with self._state: block.run().BaseConversation.get_persistence_dir(base, conversation_id) returns str(Path(base) / conversation_id.hex). The LocalConversation.__init__ constructor calls this automatically when persistence_dir is provided.
Rule: Callers that pass persistence_dir to LocalConversation() must pass only the base directory (e.g., /data/conversations/). The constructor appends the conversation hex. Passing a pre-constructed full path (e.g., /data/conversations/abc123) causes double-appending: /data/conversations/abc123/abc123.
When reviewing code that creates a new LocalConversation (fork, resume, migration):
persistence_dir.Server endpoints in conversation_service.py that create persistent state (writing directories, files, or calling fork() which writes to disk) and then perform follow-up operations (like _start_event_service) must handle partial failure.
Pattern: If the follow-up operation fails, clean up the already-written persistent state so it doesn't become an orphaned directory that confuses future startups.
# Good: rollback on failure
fork_dir = self.conversations_dir / fork_conv_id.hex
try:
fork_event_service = await self._start_event_service(fork_stored)
except Exception:
safe_rmtree(fork_dir)
raise
When reviewing server endpoints that create conversations or persistent artifacts:
The e2e-tests label triggers the mock-LLM E2E and Docker E2E test suites on a
PR. When reviewing, use your judgement to decide whether the changes could
benefit from full end-to-end testing. If the PR doesn't already have the label
and you think it should, add it:
gh pr edit <PR_NUMBER> --add-label "e2e-tests" --repo OpenHands/agent-canvas
Mention in your review body that you added the label (one sentence is enough). When in doubt, add it — running the tests is cheap, missing a regression is not. Skip it for obviously safe changes like docs-only, pure styling, or CI config tweaks.
If the PR touches an area that lacks mock-LLM E2E coverage and would benefit
from it, suggest adding a test in tests/e2e/mock-llm/ as part of the PR or a
follow-up.
Do not leave comments for:
.pr/ directory artifacts: Files in the .pr/ directory are temporary PR-specific documents (design notes, analysis, scripts) that are automatically cleaned up when the PR is approved. Do not comment on their presence or suggest removing them.If a PR is approvable, just approve it. Don't add "one small suggestion" or "consider doing X" comments that delay merging without adding real value.