Skip to content

Ohif Test Agent Skills#5993

Draft
diattamo wants to merge 16 commits into
OHIF:masterfrom
diattamo:feat/ohif-test-agent-skills
Draft

Ohif Test Agent Skills#5993
diattamo wants to merge 16 commits into
OHIF:masterfrom
diattamo:feat/ohif-test-agent-skills

Conversation

@diattamo
Copy link
Copy Markdown
Contributor

@diattamo diattamo commented May 5, 2026

Summary

Adds an ohif-test-agent skill that teaches AI coding agents how to generate correct, runnable Playwright E2E tests for the OHIF Viewer using its custom fixture system, page objects, normalized WebGL viewport coordinates, and visual-regression conventions.

This PR landed iteratively. The original proposal published the skill under tests/ohif-test-agent/ and required each contributor to manually copy or symlink the folder into their agent client's skill directory. The current shape replaces that with a single canonical location that all major clients discover automatically, plus reviewer-driven hardening of the rules the skill enforces.

Changes

The skill is at .agents/skills/ohif-test-agent/ and contains:

  • SKILL.md — the entire behavior contract. Covers the environment model, test-authoring workflow, architecture (custom fixtures, page objects, normalized coordinates, WebGL viewport), study loading lifecycle, render-cycle waits vs sleeps, visual regression rules, Playwright config facts, canonical study UIDs, a hard rules list, and a mandatory pre-output self-check.
  • assets/spec-template.ts — a ready-to-copy test template with the correct imports, fixture destructuring, viewport interaction patterns, prompt handling, and assertion strategies.
  • references/patterns-by-feature.md — feature-area-indexed seed specs (length / freehand / SEG hydration / RTSTRUCT / TMTV / MPR / crosshairs / overlays / tag browser / etc.) plus the canonical study UID table.
  • references/page-objects.md — stable structural rules for the page object system; deliberately does not enumerate methods (the source is authoritative).
  • references/utilities.md — stable rules for tests/utils/ (barrel vs deep imports, the @playwright/test trap, object-param convention, visitStudy delays, checkForScreenshot direction).
  • references/failure-triage.md — categorize-then-debug guide for the failures the suite actually produces.

Discovery by agents: zero-config for three clients, one-liner for Claude Code

Client Discovery Setup required
GitHub Copilot / VS Code Agent Scans .agents/skills/ natively none
OpenAI Codex (CLI / IDE per docs) Scans .agents/skills/ natively none
Cursor Scans .agents/skills/ natively (per Cursor docs) none
Claude Code Scans only .claude/skills/ one-line symlink, documented in AGENTS.md

A root AGENTS.md was added in another commit. This has been updated to include a short "Skills" section describing the test skill and the Claude Code setup command (Windows variant included).

Working examples:

Client Screenshot
gh Copilot image
Cursor image
Claude image
Codex /<skill> works after putting the skill files in a .cursor director similar to .claude. However, unlike claude, in the current .agents directory, it is discovered and used during agent task performance
(even) Antigravity image

Test plan

  • Pull branch
  • Confirm .agents/skills/ohif-test-agent/SKILL.md exists
  • In VS Code with Copilot Agent: type /skills in chat and confirm ohif-test-agent is listed
  • In Cursor: type /skills in chat and confirm ohif-test-agent is listed
  • In Claude Code: run the documented ln -s once, then start a new session and confirm ohif-test-agent is in the discovered-skills list
  • Prompt: "Write an OHIF Playwright test for the Length tool" — verify the generated test
  • On Windows (if available): confirm core.symlinks=true allows the .claude/skills/ohif-test-agent symlink to materialize correctly

@netlify
Copy link
Copy Markdown

netlify Bot commented May 5, 2026

Deploy Preview for ohif-dev canceled.

Name Link
🔨 Latest commit f87a492
🔍 Latest deploy log https://app.netlify.com/projects/ohif-dev/deploys/6a077ddd57966500095843fe

@jbocce jbocce self-requested a review May 6, 2026 15:41
Comment thread tests/ohif-test-agent/instructions.md Outdated

1. Locate your Claude skills directory:
- Common location: `~/.claude/skills/`
2. Copy this folder into that directory:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way for Claude to just find it in the worktree's directory? I apologize if this is a stupid question.

Copy link
Copy Markdown
Contributor Author

@diattamo diattamo May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the overall configuration to not have to perform this anymore.
But to answer your question. I have now made it so that claude finds it in the worktree. Just need to run a simple one line command.

Comment thread tests/ohif-test-agent/instructions.md Outdated

## Codex setup

Point your Codex runtime at `.github/agents/ohif-test-agent/`.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need instructions prior to this for copying ohif-test-agent to the .github directory?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed anymore

Comment thread tests/ohif-test-agent/instructions.md Outdated

---

## Codex setup
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add something for Cursor too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Cursor should now work out of the box.

| `1.3.6.1.4.1.14519.5.2.1.1706.8374.643249677828306008300337414785` | `viewer` | CT volume for 3D/MPR/crosshairs |
| `1.3.6.1.4.1.14519.5.2.1.256467663913010332776401703474716742458` | `viewer` or `segmentation` | CT + SEG for labelmap tests |
| `1.2.840.113619.2.290.3.3767434740.226.1600859119.501` | `viewer` / `segmentation` / `tmtv` | CT + RTSTRUCT + PET, used for contour and TMTV |
| `1.3.6.1.4.1.14519.5.2.1.7695.4007.324475281161490036195179843543` | `viewer` | SR structured report |
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should consider adding more here.

| `1.2.840.113619.2.290.3.3767434740.226.1600859119.501` | `viewer` / `segmentation` / `tmtv` | CT + RTSTRUCT + PET, used for contour and TMTV |
| `1.3.6.1.4.1.14519.5.2.1.7695.4007.324475281161490036195179843543` | `viewer` | SR structured report |

Do not invent UIDs — they must exist on the e2e data server.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call.

- **Object form** (preferred in newer specs): `checkForScreenshot({ page, screenshotPath, maxDiffPixelRatio?, threshold?, normalizedClip?, fullPage? })`
- **Positional form** (still in use across older specs): `checkForScreenshot(page, locator, screenshotPath, ...)`

The object form exposes tuning knobs the positional form doesn't — `maxDiffPixelRatio`, `threshold`, `normalizedClip`. For 3D content, bump `maxDiffPixelRatio` to around `0.04` because GPU rendering is noisier. Check the current signature in [tests/utils/checkForScreenshot.ts](tests/utils/checkForScreenshot.ts) (or wherever the barrel points) if something looks off.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather AI not monkey with maxDiffPixelRatio. That should be something rarely touched.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the instruction to not mess with this. I added instruction to have it reviewed instead.


### `checkForScreenshot` has two call forms

- **Object form** (preferred in newer specs): `checkForScreenshot({ page, screenshotPath, maxDiffPixelRatio?, threshold?, normalizedClip?, fullPage? })`
Copy link
Copy Markdown
Collaborator

@jbocce jbocce May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only reason object form is preferred because we want to stop taking the screenshot of the entire app and instead focus on the viewport or viewport grid. This should somehow be worked in here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated this basically default to not using screenshots going forward. Take a look at the wording.

diattamo added 3 commits May 14, 2026 12:20
Copilot/Cursor/Codex now auto-discover via .agents/skills/. Claude Code
needs a one-line symlink documented in AGENTS.md. .claude/ and .cursor/ added
in .gitignore. Drops the obsolete AGENTS.md wrapper and instructions.md install guide; updates to tests/* markdown links in skill content
@diattamo diattamo requested a review from jbocce May 14, 2026 19:34
Comment thread AGENTS.md Outdated

## Skills

The `ohif-test-agent` skill (Playwright E2E test guidance) lives at `.agents/skills/ohif-test-agent/`. GitHub Copilot/VS Code Agent, OpenAI Codex, and Cursor scan `.agents/skills/` natively — no setup required. Claude Code scans only `.claude/skills/` so set up the symlink locally once after cloning:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be tied into the preinstall.js file like what is done for (this) existing AGENTS.md file? I believe there is already a (similar) auto link for the AGENTS.md file in there.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented so that it should work with
yarn install (or preinstall)

@diattamo diattamo requested a review from jbocce May 16, 2026 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants