diff --git a/README.md b/README.md index 3039d465..a405dea3 100644 --- a/README.md +++ b/README.md @@ -13,6 +13,7 @@ Pipelines  ·  How It Works  ·  Providers  ·  + Review Guide  ·  Agent Guide

diff --git a/docs/PR_REVIEW_GUIDE.md b/docs/PR_REVIEW_GUIDE.md new file mode 100644 index 00000000..9090a507 --- /dev/null +++ b/docs/PR_REVIEW_GUIDE.md @@ -0,0 +1,508 @@ +# Pull Request Review Guide + +This guide is for community reviewers, maintainers, and AI assistants reviewing +OpenMontage pull requests. It is a review framework, not a closed checklist. +Use it to structure the review, then keep looking for issues that are specific +to the PR in front of you. + +OpenMontage is an agent-orchestrated video production system. Many PRs affect +more than the file they edit: a provider change can affect tool discovery, +selector routing, setup instructions, pipeline decisions, cost reporting, and +the user-visible production flow. Good reviews protect those contracts. + +## Review Mindset + +Start with these questions: + +- Does this PR move OpenMontage in the right direction? +- Is the scope focused, or is it mixing unrelated changes? +- Can a user, maintainer, or agent understand the behavior after this lands? +- Does the PR preserve the agent-first architecture? +- Does it introduce regressions in provider discovery, pipeline artifacts, or + rendering behavior? +- Are tests and docs updated at the same level as the behavior change? +- Are there security, privacy, dependency, or supply-chain concerns? + +The answer can be "useful, but not merge-ready." That is a good review outcome +when the idea is aligned but the implementation needs cleanup. + +## Review Outputs + +A useful review should usually produce one of these outcomes: + +- **Approve**: the PR is useful, focused, tested, and low-risk enough to merge. +- **Comment**: the PR is promising but needs cleanup before approval. +- **Request changes**: the PR has blockers that would create regressions, + break contracts, or mislead users. +- **Close or redirect**: the PR is not aligned, mostly noise, or belongs in an + issue/discussion before code. + +Avoid rubber-stamp approvals. Also avoid turning every concern into a blocker. +Name the actual severity and explain the impact. + +## General Review Areas + +### Project Direction + +Check whether the PR solves a real OpenMontage problem. + +- Does it improve video production quality, reliability, speed, portability, + provider coverage, local execution, docs, tests, or contributor experience? +- Does it duplicate an existing path without improving it? +- Is it a speculative feature with no clear user workflow? +- Does it introduce maintenance burden disproportionate to the value? + +### Scope Hygiene + +Noise makes reviews unsafe. + +- Are unrelated files changed? +- Are lockfiles changed only when dependencies actually changed? +- Are generated files, screenshots, binary assets, or local artifacts included + without reason? +- Are docs/test fixes bundled with feature work in a way that hides risk? +- Is the branch stale against latest `main`? + +If a PR is useful but noisy, ask for a narrower diff before deep approval. + +### Regression Risk + +Look beyond the changed file. + +- Does the change affect existing users or only add a new optional path? +- Does it alter default provider selection, fallback behavior, or setup menus? +- Does it change schemas, artifacts, or pipeline stage contracts? +- Does it change rendering output, timing, audio, captions, or file paths? +- Does it create silent fallback behavior where the user should be told? + +### Security and Supply Chain + +Review security issues factually and with evidence. Do not make public claims +about a contributor's intent. If you see suspicious code, describe the behavior +and risk. + +Check for: + +- New network calls, uploads, telemetry, or background processes +- Secret exfiltration risks, `.env` reads, token logging, or unsafe debug output +- Shell execution, `subprocess`, dynamic imports, `eval`, `exec`, or generated + code execution +- Dependency additions, install scripts, package-lock churn, or broad version + ranges +- Unsafe file deletion, path traversal, archive extraction, or writes outside + the expected project directory +- Remote model downloads or provider calls that are not surfaced in metadata +- Prompt injection surfaces where external content could instruct the agent + +Use language like: + +- "I found a security blocker: this command executes user-controlled input." +- "This dependency change needs justification before merge." +- "I did not find an actionable security issue in the reviewed diff." + +Avoid public phrasing like: + +- "No malicious intent." +- "This author is safe." +- "This is definitely harmless." + +### Performance and Resource Use + +OpenMontage works with video, audio, image generation, and local models. Small +code changes can create large runtime costs. + +- Does the PR add repeated model loads instead of reusing state? +- Does it download large files without clear setup/status messaging? +- Does it increase render time, memory, VRAM, disk, or network use? +- Does it change frame extraction, composition, encoding, or audio processing + in a way that could slow common paths? +- Are resource profiles and cost estimates realistic? + +## OpenMontage Architecture Checks + +### Agent-First Architecture + +OpenMontage's control plane is the agent following markdown skills and YAML +manifests. Python should provide tools and persistence, not hidden orchestration. + +Flag PRs that: + +- Add Python orchestrators for creative decisions, stage transitions, or review + policy +- Hide provider/model/runtime decisions inside code without user visibility +- Bypass pipeline manifests, stage director skills, checkpoints, or review +- Move quality policy into ad hoc code instead of documented instructions + +Good PRs keep intelligence in instructions and contracts, with Python handling +well-bounded execution. + +### Tool Contract + +Every tool should inherit from `tools/base_tool.py` and satisfy the `BaseTool` +contract. + +For new or changed tools, review: + +- `name`, `version`, `tier`, `capability`, `provider` +- `runtime`, `stability`, `execution_mode`, `determinism` +- `dependencies` using supported prefixes such as `cmd:`, `env:`, `python:` +- `install_instructions` +- `input_schema`, `output_schema`, and artifact behavior +- `supports`, `best_for`, `not_good_for` +- `resource_profile` +- `retry_policy` +- `fallback` and `fallback_tools` +- `agent_skills` +- `user_visible_verification` +- `estimate_cost()` and `estimate_runtime()` where relevant +- `execute()` returning a `ToolResult` + +Metadata is user-facing. If setup, offline behavior, cost, model downloads, or +hardware requirements are inaccurate, the provider menu and agent planning will +mislead users. + +### Tool Registry and Discovery + +Tool discovery flows through `tools/tool_registry.py`. Avoid hardcoded tool +lists unless there is a strong reason. + +Check: + +- Does the tool register through normal package discovery? +- Does it use the right `capability` so selectors can find it? +- Does `get_status()` report availability accurately? +- Does an import failure in one optional provider break unrelated discovery? +- Does the provider menu show useful setup instructions? +- Does the PR accidentally make unavailable tools look configured? + +Useful commands: + +```bash +python -c "from tools.tool_registry import registry; import json; registry.discover(); print(json.dumps(registry.capability_catalog(), indent=2))" +python -c "from tools.tool_registry import registry; import json; registry.discover(); print(json.dumps(registry.provider_menu_summary(), indent=2))" +``` + +### Selectors and Providers + +Selectors route capability-level requests to provider tools: + +- `tts_selector` +- `image_selector` +- `video_selector` + +When reviewing selector or provider changes: + +- Confirm the provider has the correct `capability`. +- Confirm selector input names map to provider input names. +- Confirm provider-specific options do not get silently ignored. +- Confirm user preference is respected when explicit. +- Confirm unavailable providers do not block available alternatives. +- Confirm ranking/fallback changes are tested. +- Confirm the selector still returns useful alternatives and reasoning. + +Adding a provider should usually not require selector code changes. If it does, +the PR should explain why. + +### Pipeline Manifests and Stage Skills + +Pipeline manifests live in `pipeline_defs/`. Stage instructions live in +`skills/pipelines/`. + +For pipeline changes, check: + +- Manifest schema validity +- Stage order and stage names +- `produces` artifacts +- `tools_available` +- `review_focus` +- `success_criteria` +- `checkpoint_required` +- `human_approval_default` +- Matching stage director skills +- Tests in `tests/contracts/` or `tests/pipelines/` + +The manifest and the stage skill must agree. If the manifest says a stage +produces `scene_plan`, the director skill should actually guide creation of a +valid `scene_plan`. + +### Canonical Artifacts and Schemas + +Artifacts in `schemas/artifacts/` are contracts between stages. + +When reviewing schema or artifact changes: + +- Does every producer still write valid artifacts? +- Does every consumer still understand the artifact? +- Are required fields justified? +- Are migrations or backward compatibility needed? +- Do tests cover valid and invalid examples? +- Does the checkpoint protocol still work with the changed artifact? + +Schema changes are high-impact. Treat them as cross-pipeline changes unless the +PR proves otherwise. + +### Checkpoints and Review Policy + +OpenMontage uses checkpoints for resume, human approval, audit trails, and +stage gating. + +Check: + +- Does the PR preserve checkpoint status semantics? +- Does it avoid skipping required human approval? +- Does it preserve cost snapshots and review metadata where expected? +- Does it avoid writing incomplete canonical artifacts as completed stages? +- Does it align with `skills/meta/checkpoint-protocol.md`? +- Does it align with `skills/meta/reviewer.md`? + +Review logic should remain instruction-driven unless the PR is adding a narrow +mechanical validator. + +### Composition Runtimes + +OpenMontage can compose with Remotion, HyperFrames, or FFmpeg. Runtime choices +are user-visible production decisions. + +For render/composition PRs, check: + +- Does `video_compose` preserve explicit `render_runtime` routing? +- Are Remotion, HyperFrames, and FFmpeg paths considered where relevant? +- Does the PR avoid silent runtime swaps? +- Are runtime availability errors surfaced as blockers? +- Are Node, FFmpeg, `npx`, and package requirements checked accurately? +- Do render reports contain enough evidence to debug failures? +- Are browser previews treated as QA/debug artifacts when the production render + path is different? + +Silent downgrades from motion-led production to still-led fallback are review +findings, not harmless implementation details. + +### Cost, Budget, and Paid Providers + +OpenMontage should not silently spend money or imply a paid provider is free. + +Check: + +- Does the tool estimate cost accurately enough for planning? +- Are first-time paid provider uses visible to the user? +- Does the provider doc mention pricing/free-tier caveats? +- Does fallback from paid to free, or free to paid, require user visibility? +- Are model downloads, hosted endpoints, or cloud GPU usage described honestly? + +### Docs and User-Facing Claims + +Docs are part of the product. Provider setup docs often drive agent behavior. + +Check: + +- Does the PR update `docs/PROVIDERS.md` when adding a provider? +- Does `docs/ARCHITECTURE.md` need an update for architectural changes? +- Are setup instructions accurate on macOS, Windows, and Linux where claimed? +- Do docs distinguish API keys, local installs, model downloads, and cached + offline operation? +- Are package versions and model versions clearly separated? +- Are limitations stated plainly? + +## Scenario-Specific Review Prompts + +### New Provider or Tool + +Ask: + +- Is this provider useful for OpenMontage workflows? +- Does it add real coverage or just duplicate an existing provider? +- Is the tool discoverable through the registry? +- Does the matching selector discover and route to it? +- Are provider inputs compatible with selector inputs? +- Does status checking reflect real availability? +- Are setup docs, dependencies, cost, network, and cache behavior accurate? +- Does it have focused tests? +- Does it avoid importing heavyweight dependencies at module import time? +- Are generated artifacts written to expected paths? + +Minimum expected coverage: + +- Tool metadata/contract test +- Registry discovery test +- Status behavior test with dependencies mocked when needed +- Selector/routing test if it joins a selector-backed capability +- Docs update for user-visible providers + +### Selector Change + +Ask: + +- Does it still auto-discover providers? +- Does it avoid hardcoded provider lists? +- Does it preserve explicit user preference? +- Does it handle unavailable providers cleanly? +- Does it explain the selected provider? +- Does it preserve alternatives considered? +- Does it map shared inputs to provider-specific inputs? + +### Local GPU or Model Runtime Change + +Ask: + +- Is the hardware requirement accurate? +- Does CPU/MPS/CUDA behavior match the implementation? +- Are dtype choices safe for each device? +- Are model downloads and cache behavior documented? +- Does the resource profile reflect realistic RAM, VRAM, and disk needs? +- Does setup avoid pretending local GPU install is a one-minute API-key fix? + +### Render Runtime Change + +Ask: + +- Does it preserve runtime choice in `edit_decisions.render_runtime`? +- Does it avoid fallback without user-visible approval? +- Are Remotion and HyperFrames contracts respected? +- Are smoke tests or render probes included? +- Does final output get validated with ffprobe/frame/audio checks where relevant? + +### Pipeline or Skill Change + +Ask: + +- Does the manifest still validate? +- Do stage skills exist and match stage names? +- Are `review_focus` and `success_criteria` meaningful? +- Are canonical artifacts valid? +- Are human approval gates correct? +- Are tests updated? + +### Schema Change + +Ask: + +- Which producers and consumers are affected? +- Is the field required or optional? +- Do existing checkpoints/artifacts still work? +- Are tests updated for both valid and invalid data? +- Does the change need a migration note? + +### Dependency or Lockfile Change + +Ask: + +- Is the dependency necessary for the PR? +- Is the lockfile change proportional? +- Are install scripts or transitive packages risky? +- Is the package maintained and appropriately licensed? +- Does it work on the supported Python/Node versions? +- Are version bounds too loose or too strict? + +### Docs-Only Change + +Ask: + +- Is the doc technically accurate? +- Does it match current code and registry behavior? +- Does it overpromise provider quality, cost, offline operation, or platform + support? +- Does it point users to the right setup and troubleshooting path? + +Docs-only PRs can still create regressions by teaching users or agents the +wrong behavior. + +## Testing Expectations + +Pick tests based on risk. Do not require expensive integration tests for every +small doc fix, but do require evidence for changed behavior. + +Common checks: + +```bash +python -m pytest tests/contracts -q +python -m pytest tests/tools -q +python -m pytest tests/qa -q +python -m py_compile path/to/changed_file.py +``` + +For provider PRs, focused mocked tests are often better than expensive live API +tests. Live-provider QA is useful when credentials and cost are acceptable, but +the contract should not depend on a maintainer having every provider configured. + +If tests cannot be run, the review should say why and what risk remains. + +## PR Comment Guidance + +Good review comments are specific, evidence-based, and actionable. + +Prefer: + +- "This test fails with the current branch: `...`" +- "This provider will not be discovered because `capability` is set to `...`." +- "This setup claim is misleading because first run downloads model weights." +- "Please split the lockfile churn from this provider change." + +Avoid: + +- Vague comments like "clean this up" +- Personal comments about the contributor +- Public speculation about intent +- Overclaiming safety +- Long lists of nits when there is a clear blocker + +When a PR has several issues, prefer one consolidated comment. It helps the +author fix everything in one pass. Use inline comments for exact line-level +bugs that need code context. + +Suggested public language for security review: + +- "I found an actionable security issue in this diff." +- "This area needs a security review before merge." +- "I did not find an actionable security issue in the reviewed diff." + +Do not state or imply that a contributor has or lacks malicious intent. Review +the code and behavior. + +## Maintainer and AI-Agent Workflow + +This section is public so contributors understand how reviews are performed. +It is a workflow, not a limit on reviewer judgment. + +For a full review: + +1. Fetch latest `main`. +2. Check out the PR in a clean worktree. +3. Inspect PR metadata: title, linked issues, changed files, author notes, and + prior comments. +4. Review the diff for scope, usefulness, and architecture fit. +5. Identify the PR scenario: provider, selector, pipeline, schema, runtime, + docs, tests, dependency, or mixed. +6. Apply the relevant scenario prompts from this guide. +7. Run focused tests or explain why they were not run. +8. Check docs and user-facing claims against implementation. +9. Check security, dependency, and supply-chain risk. +10. Write findings ordered by severity. +11. Decide: approve, comment, request changes, or close/redirect. + +For AI-assisted review, ask the agent to report evidence, not just conclusions: + +- Commands run and their results +- Files inspected +- Architecture contracts touched +- Risks that remain unverified +- Exact merge blockers + +Do not approve from a summary alone. The reviewer or agent should inspect the +diff and relevant project contracts. + +## Merge-Readiness Rubric + +Before approving, confirm: + +- The PR is useful and aligned with project direction. +- The diff is focused and free of unrelated churn. +- Architecture contracts are preserved. +- Tests cover the behavior at the right level. +- Docs match user-visible behavior. +- Security and supply-chain risks have been considered. +- Performance, cost, and resource claims are realistic. +- The branch is current enough that review findings are still valid. +- Remaining risks are acceptable and stated. + +Approving does not mean the PR is perfect. It means the change is useful, +understood, appropriately tested, and safe enough to land.