diff --git a/.agents/skills/custom-codereview-guide.md b/.agents/skills/custom-codereview-guide.md new file mode 100644 index 0000000000000000000000000000000000000000..38697679b2f10abc165cc678e475ff39360c2d7b --- /dev/null +++ b/.agents/skills/custom-codereview-guide.md @@ -0,0 +1,201 @@ +--- +name: custom-codereview-guide +description: Repository-specific review rules for the OpenHands Agent Canvas frontend. +triggers: + - /codereview +--- + +# OpenHands Agent Canvas Code Review Guidelines + +This guide supplements the public `code-review` skill with rules specific to +`OpenHands/OpenHands`, the Agent Canvas frontend. Read `AGENTS.md` first; it is +the detailed source of truth for current architecture and test conventions. + +Be direct and constructive. Review correctness and architecture, not formatting +that lint or the compiler already checks. + +## Review Decision + +- Submit exactly one review: **APPROVE** or **COMMENT**. Never use + **REQUEST_CHANGES**. +- Default to **APPROVE** when there are no important findings. Nitpicks and + optional cleanup are not reasons to withhold approval. +- Use **COMMENT** for correctness, security, architecture, missing evidence, or + unmet acceptance criteria. Let a human maintainer make the blocking decision. +- Do not approve changes that can affect agent or benchmark behavior—prompts, + tool selection, conversation payloads, terminal behavior, planning, memory, + or evaluation paths—without human review and appropriate lightweight evals. +- Read the linked issue and include a compact checklist covering each acceptance + criterion. Meeting the checklist is necessary but does not replace review for + regressions, security, or maintainability. + +## Repository Ownership + +Put behavior in the repository that owns it: + +| Repository | Owns | +| ------------------------------ | --------------------------------------------------------------------------------------------------------------- | +| `OpenHands/OpenHands` | Agent Canvas UI, frontend state, backend selection, frontend service integration, and local-stack orchestration | +| `OpenHands/software-agent-sdk` | Agent Server, agents, tools, conversations, events, workspaces, and the canonical server API | +| `OpenHands/typescript-client` | Browser-compatible typed access to the Agent Server API | +| `OpenHands/extensions` | Reusable skills, plugins, and integrations | +| `OpenHands/automation` | Scheduling, webhooks, run history, and automation dispatch | + +The normal dependency direction is Agent Server contract → TypeScript client → +Canvas. Flag raw endpoint reimplementations, Canvas-local copies of server +contracts, and changes opened in the wrong repository. + +## Architecture That Guides Agents + +Agents tend to copy the nearest pattern and choose the shortest compiling path. +Review the codebase as part of the product surface that guides those choices: + +1. **Make the conventional path cheapest.** New work should naturally reuse a + named hook, service, store, or feature module instead of adding another branch + to a shared root. +2. **Fail forbidden dependencies mechanically.** Repeated review guidance should + become a lint rule, compiler boundary, or architecture test. Do not grow this + document when a small executable guard would be clearer. +3. **Give durable state one obvious writer.** A backend setting, consent value, + conversation cache entry, or persisted browser value should have one named + owner. Flag second writers and component-local mirrors of authoritative state. +4. **Prefer owned feature files over shared switches.** Product work should + usually extend a feature-owned module. Shared registries and root conditionals + need a concrete reason. +5. **Keep exceptions narrow and visible.** Exceptions belong in a small allowlist + next to the guard that enforces the rule and should be reviewed as architecture + changes. + +Treat “deep module” as a design heuristic, not a line-count target. A good module +has a narrow, stable interface and hides cohesive complexity. Do not split a file +merely because it is long, and do not create layers that only rename or forward +arguments. Prefer a small pure seam when it removes duplicated decisions, makes +ownership explicit, or enables focused tests. + +### React effects + +`useEffect` is for synchronizing React with an external system. Flag effects used +to: + +- derive render data from props or state; +- respond to a user action that can run in the event handler; +- initialize a value that belongs in a lazy state initializer; +- mirror one store or cache into another component state value; or +- repair ordering created by competing writers. + +An effect is not automatically wrong. Subscription, browser API, timer, and +network synchronization still belong in effects when cleanup and dependency +semantics are explicit. + +## Blocking Architecture Checkpoints + +### Agent Server and Cloud API access + +`src/api/no-direct-agent-server-calls.test.ts` is the executable source of truth. +Do not approve new raw `fetch`, `axios`, shared `openHands`, or low-level HTTP +client access to Agent Server endpoints. Use `@openhands/typescript-client` with +the options from `src/api/agent-server-client-options.ts`. + +Cloud and runtime-sandbox requests must go through `callCloudProxy`; runtime +requests must provide the correct `hostOverride` and authentication mode. Review +changes to the guard's allowlist as architecture changes. Do not copy its current +entries into this guide—the test should remain the one authoritative list. + +### Event wire contracts + +The SDK event model is the wire authority, the TypeScript client mirrors it, and +Canvas consumes the published client type. Do not approve Canvas-local +redeclarations, partial intersections, module augmentation, or presentation +fields added to wire-event interfaces. + +A contract change should land in this order: + +1. SDK model/schema and serialization coverage. +2. TypeScript-client mirror derived from the SDK payload. +3. Published client release. +4. Canvas consumption and rendering/telemetry coverage. + +Canvas-only presentation state belongs in a separate view model keyed by event +identity. + +### Telemetry and durable frontend state + +- `src/services/telemetry.ts` is the only owner of the Canvas PostHog client. +- React events go through typed functions in `src/hooks/use-tracking.ts`; components + must not call PostHog directly. +- Consent rendering uses the telemetry consent external store, not mirrored local + state. `setTelemetryConsent` remains the single consent controller. +- A business milestone has one canonical capture. Flag duplicate conditional + captures. +- For other durable values, prefer the existing named service/store/hook and flag + new storage writes from arbitrary components. + +## Dependencies and Releases + +- Direct dependencies are exact-pinned. Keep `package.json` and + `package-lock.json` synchronized through npm; do not hand-edit one side only. +- Treat changes to dependency exemptions, git pins, and security overrides as + reviewable policy changes. `__tests__/package-library.test.ts` is the executable + source of truth for allowed specs. +- Scrutinize newly published third-party dependency versions for supply-chain + risk. First-party OpenHands packages are exempt from a waiting period but not + from contract and release-order review. +- Package version changes belong in explicit release PRs and must match the + release workflow expectations. + +## Testing and Evidence + +- Require evidence proportional to the behavior changed. For UI behavior, use a + screenshot or video from the real app. For CLI, API, or scripts, require the + exact runtime command and observed result. +- Runtime and user-visible bug fixes require before-and-after evidence through + the real production-facing path. The before evidence must reproduce the bug on + the base branch or released version; the after evidence must repeat the same + setup on the PR head and show the corrected behavior. Include exact commands, + relevant output, and screenshots or video when the behavior is visual. +- For lifecycle fixes, evidence must also verify the resulting process or resource + state—for example, the parent exit code and whether child services or listening + ports remain after shutdown. +- Unit and integration tests are regression proof, not a substitute for live + evidence. If required live evidence is missing, submit **COMMENT**, not + **APPROVE**, and identify the exact production-facing verification still needed. +- Prefer tests that exercise real logic and observable state. Do not reward mocks + that only prove another mock was called. +- Keep tests focused: one meaningful assertion path per behavior, no duplicated + coverage of library behavior, and no brittle presentation-only snapshots. +- Follow the test routing in `AGENTS.md`. The mock-LLM, Docker mock-LLM, and + live LLM-backed E2E suites run after changes reach `main`, not from PR labels. + If a risky PR needs pre-merge E2E evidence, recommend manually dispatching the + relevant workflow against the PR branch. +- Never broaden live E2E triggers or secret exposure for convenience. + +## Review Context Integrity + +Before submitting the review, compare its summary and every finding against the +current PR title, changed-file manifest, linked issues, and acceptance criteria. +If the review describes files, behavior, issues, or release paths that are not in +that context, stop and re-read the PR rather than submitting stale or mismatched +feedback. Do not approve until this final context check passes. + +## What Not to Comment On + +Do not leave review comments for: + +- formatting or minor style that tooling handles; +- optional “nice to have” refactors unrelated to the change; +- praise-only observations—approve instead; +- extra tests for straightforward data/config changes when existing checks cover + the risk; or +- temporary `.pr/` artifacts, which are cleaned up by repository automation. + +When raising a finding, trace the relevant call or data flow far enough to show +the concrete failure mode. Prefer one high-signal comment over several symptoms +of the same ownership problem. + +## Communication Style + +- Be concise, specific, and friendly. +- Explain the user-visible or architectural consequence. +- Suggest the smallest viable correction. +- Use GitHub suggestion syntax for local fixes. +- If the PR is sound, approve it without manufacturing feedback. diff --git a/.agents/skills/pr-design-doc/SKILL.md b/.agents/skills/pr-design-doc/SKILL.md new file mode 100644 index 0000000000000000000000000000000000000000..62f02c6a675a1c851f906bf180c177cf0aea5865 --- /dev/null +++ b/.agents/skills/pr-design-doc/SKILL.md @@ -0,0 +1,168 @@ +--- +name: pr-design-doc +description: > + For a non-trivial pull request, write a self-contained HTML design doc under the + temporary `.pr/` directory and link a visibility-appropriate preview in the PR + description, so maintainers grasp the proposal at a glance - code/API design, and the + before/after of the change, grounded to real code. Use when opening or updating a + non-trivial PR, or when the user says "add a design doc", "document this PR for + reviewers", "show the before/after", "make the design reviewable", or "write the .pr/ + page". +triggers: +- /pr-design-doc +- /design-doc +license: MIT +metadata: + tags: pull-request, design-doc, html, review, before-after, htmlpreview +--- + +# pr-design-doc - a reviewable design doc for a non-trivial PR + +A diff shows *what changed line by line*. It does not show *the design*: the shape of the +change, the API before and after, and why this approach. Reviewers reconstruct that by +hand, slowly. The scarce resource is the maintainer's attention and trust budget - not the +agent's effort. Spend extra effort to hand them **one self-contained HTML page** that +conveys the **big picture** and the **before → after core difference**, with every claim +**clickable back to the real code**, then link it from the PR description. + +This is the same craft as a "show me this change" explainer, aimed at one job: making a +non-trivial PR easy to review. + +## When to use it + +- Opening or updating a **non-trivial** PR: new/changed public API, a new module or + subsystem, a behavior change in core logic, a migration, or anything a reviewer can't + fully judge from the diff in a couple of minutes. +- **Skip it** for trivial PRs - a typo, a one-line guard, a dependency bump, a docs tweak, a simple bug fix. + A design doc adds more to review. Use judgment; if the diff *is* the explanation, don't add a + page. + +## The `.pr/` workflow + +Use the temporary **`.pr/`** directory for PR-only artifacts. Before relying on automatic +cleanup, verify that the target repository has an enabled +`.github/workflows/pr-artifacts.yml` workflow that removes `.pr/` after approval. + +- Same-repository PR with verified cleanup workflow: the workflow removes `.pr/` after + approval. +- Fork PR, or repository without a verified cleanup workflow: remove `.pr/` manually before + merge. + +The design doc is a review aid that lives with the branch while the PR is open. It must not +ship in the merged tree. + +## Workflow + +1. **Check out and verify the PR head.** Do not write or commit the design doc from the base + branch or an unrelated checkout. Start with a clean worktree, then inspect and check out + the PR: + ```bash + gh pr view --json title,body,url,baseRefName,baseRefOid,headRefName,headRefOid,headRepository,headRepositoryOwner,isCrossRepository,files,additions,deletions + gh pr checkout + git rev-parse HEAD + gh pr view --json headRefOid --jq .headRefOid + ``` + The final two SHAs must match before you continue. If they do not, stop and fix the + checkout. Compute the merge-base SHA with + `git merge-base `. Group changed files by area and keep both the + merge-base SHA and head SHA for source links. + +2. **Read both sides of each logical file.** Compare + `git show :` with the verified head. Capture the + **function-level** behavioral difference - what the code *did* vs *does now*. + - new file → no "before"; one "after" diagram + a line on the role it adds. + - deleted file → "before" diagram + who/what takes over. + - edited file → a before/after pair, with the delta highlighted. + +3. **Classify each file.** *Logic* change (behavior moved) → draw before/after. *Mechanical* + change (rename, constant, config, import move) → a one-line `before → after` row, no + diagram. Don't dilute the signal by drawing mechanical edits. + +4. **If the change is an API change, lead with the API.** Show the signature/schema/type + **before and after** side by side (function signature, endpoint + payload, config field, + event shape). Name the compatibility impact plainly: additive, breaking, or behind a flag. + +5. **Find the cross-file story.** If one call chain threads several files, draw a single + **overview** before/after at the top; per-file cards drill in. + +6. **Build the page** per [`references/html-craft.md`](references/html-craft.md) - one + self-contained, offline, editorial HTML file with hand-drawn SVG figures. Save it to + the repo's `.pr/` directory, e.g. `.pr/design.html` (or `.pr/.html`). Before + writing, reject a symlink at `.pr` or at the exact output path; never follow a + branch-controlled symlink outside the worktree. + ```bash + test ! -L .pr && test ! -L .pr/design.html + mkdir -p .pr + ``` + +7. **Commit under `.pr/`, push to the verified PR head, and link it.** Confirm that the push + remote resolves to `headRepository.nameWithOwner`; never push the artifact to the base + repository's default branch. + ```bash + git add .pr/design.html + git commit -m "docs(.pr): design doc for " + git push HEAD: + ``` + Query the base repository's visibility before choosing the link: + ```bash + gh repo view / --json visibility,url + ``` + - **Public repository:** add an htmlpreview link near the top of the PR description, + pointing at the **fork and branch the PR is opened from** (it renders before merge): + ``` + 📄 Design doc: https://htmlpreview.github.io/?https://github.com///blob//.pr/design.html + ``` + - **Private or internal repository:** link the access-controlled GitHub blob and include + local download/open instructions, or use an existing access-controlled artifact + service. Never send the document through htmlpreview or another public host. + +## What the page contains + +1. **What changed (decision first)** - one paragraph: the intent, net effect, and why the + reviewer should care. Put the highest-impact conclusion, risk, or API-compat note in a + `★` callout, with the most important changed `path:line` nearby. Stats (`N files · + +A / −D`) are context, not the lead. If there's a cross-file flow, the **overview + before/after SVG** goes here. +2. **API before → after** (when the PR changes an interface) - signatures/schemas/types side + by side, with the compatibility verdict stated. +3. **Left rail / index** - changed files grouped by area, each tagged (🟢 added · 🔴 removed · + ✏️ changed · ⚙️ mechanical) with +/− counts; click to jump. +4. **Per-file cards** - for each logical file: a claim-carrying title, a one-line summary of + how its behavior changed, **before/after** diagrams with real symbol names + `file:line` + (changed nodes in orange), and the diff in a collapsed `
`. Mechanical files get a + small `before → after` table, no diagram. +5. **(optional) Risk / follow-ups** - only if grounded in what you read. + +## Non-negotiable principles + +1. **Optimize for scarce reviewer attention.** The first screen answers, in ~15 seconds: + what this PR does, whether it's risky, where to look first, and what evidence backs the + claim. Lead with the conclusion, not your process. +2. **Show the difference, not just the after.** For any logic or API change, draw **before** + and **after** and make the *delta* visually loud (color + line style). The contrast is + the product. +3. **Ground everything to code, beside the claim.** Every box, node, and sentence names a + real symbol + `path:line`, and links to the correct source revision where possible: the + merge-base SHA for before-state evidence and the verified head SHA for after-state + evidence. One click from "this changed" to the exact code. +4. **Hand-draw the carrying diagrams.** Prefer bespoke inline SVG for the before/after that + makes the argument; Mermaid is fine only for quick auxiliary graphs. +5. **Self-contained & offline.** One HTML file, inline CSS/SVG, no external scripts or + assets, opens by double-click, and survives being copied to another machine. +6. **`.pr/` only, and temporary.** The doc is a review aid, not project docs. Keep it in + `.pr/` and ensure it is removed before merge. Rely on automatic cleanup only when the + repository's workflow has been verified; otherwise remove it manually. Do not move design + HTML into `docs/` or ship it in the merged tree. + +## Anti-patterns + +- ❌ Dumping the raw diff / file tree and calling it a "design doc" - adds nothing over the + PR page. +- ❌ Empty nodes ("process data", "handle request") - every node is a real symbol + + location. +- ❌ Only the after-state when something changed - reviewers want the *contrast*. +- ❌ A design doc on a trivial PR - noise. Skip it. +- ❌ Committing the HTML outside `.pr/` (e.g. `docs/`), where it would merge into `main`. +- ❌ Publishing a private-repository design doc through htmlpreview, GitHub Pages, or another + public host. Use the private/local preview path in the craft reference. Use GitHub Pages + only with explicit user authorization after verifying private Pages access control. diff --git a/.agents/skills/pr-design-doc/references/html-craft.md b/.agents/skills/pr-design-doc/references/html-craft.md new file mode 100644 index 0000000000000000000000000000000000000000..974a9c649061d8124df6b2a6ec699c8c23c495e2 --- /dev/null +++ b/.agents/skills/pr-design-doc/references/html-craft.md @@ -0,0 +1,322 @@ +# html-craft - how to build the page + +Shared craft for every show-me dimension. One self-contained, offline, editorial HTML +page with hand-drawn SVG figures and code-grounded claims. + +## The look (editorial document, light theme) + +Calm, readable, document-like - not a dark dashboard. Skeleton: + +```html + + +{{Title}} + +
+ +
+
{{kind}}

{{Title}}

+

{{one-paragraph mental model - the big picture in 2-3 sentences}}

+

1…

…
+
+
+``` + +Numbered sticky TOC + one-paragraph mental model up top + sectioned body. No JS needed +for this shell. + +## First screen: 15-second orientation + +The reader's attention is the budget. The first viewport should answer four questions +without requiring a full read: + +1. **What is this?** A one-paragraph mental model in the title subtitle. +2. **Why does it matter?** The highest-impact conclusion, risk, or action in a `★` + callout near the top. +3. **Where should I jump?** A numbered sticky TOC whose labels carry information, not + just categories. +4. **Why should I trust it?** Nearby source links (`path:line`, test, command, commit, + or fixture) for the first substantive claim. + +Do not open with "I read these files" or a chronological work log. Start with the +reader's decision: what changed, how the system works, what path matters, or where to +look next. Put methods, command output, and raw diffs behind `
` unless they +are the point of the report. + +## Skimmable structure + +Design the page so a busy reader can scan headings, captions, callouts, and tables +before choosing where to dive: + +- **Headings make claims.** Prefer "Writes only cross the queue" over "Architecture", + when the section has a specific finding. Generic labels are acceptable only when the + title/subtitle already carries the finding. +- **One paragraph, one judgment.** Keep paragraphs short; split when a sentence starts + proving a different point. +- **Number parallel points.** If you say "three rules" or "two risks", number them so + the reader can reconcile the claim with the list. +- **Use tables only for stable comparison axes.** A table should reduce cognitive + work, not force subtle judgments into neat boxes. +- **Make captions do work.** A caption states the takeaway of the figure, not merely + its type. + +## Callouts (give them semantic types) + +The `.callout` / `.callout.warn` / `.callout.note` styles aren't interchangeable - assign +each a fixed job and the reader learns to skim by them: + +- **`★` key takeaway** (accent) - the one load-bearing sentence of a section. At most one per + section; it's what the reader should remember if they read nothing else. +- **`ⓘ` note** (`.note`, muted) - a reading hint for a figure, an aside, a "why we did it this + way" that isn't on the critical path. +- **`⚠` warning** (`.warn`) - a boundary, a risk, a gotcha, a guardrail: trust boundaries, + "this does NOT do X", "never commit the secret". Reserve it for things that bite. + +Don't let callouts become wallpaper - if every paragraph is a colored box, none of them carry +weight. A glyph prefix (`★ ⓘ ⚠`) makes the type legible before the reader parses the text. + +## Hand-drawn SVG figures (the diagrams that carry the argument) + +Draw bespoke inline `` with a `` for arrow markers and a +scoped `