Conversation
Completed Working on "Code Review"✅ Workflow completed successfully. |
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
![overcut-ai[bot]](https://avatars.githubusercontent.com/in/1367736?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Found 4 total review comments:
- 3 MAJOR
- 1 MINOR
- 0 BLOCKER, 0 CRITICAL, 0 SUGGESTION, 0 PRAISE
Affected files: 1 (AGENTS.md)
Key themes
- Documentation accuracy issues that could misdirect contributor workflows (plugin paths/commands).
- Safety guidance gaps around integration test execution and sandbox constraints.
- Overstated architectural claims (sync/async parity) that may lead to incorrect assumptions.
Actionable next steps
- Correct path and command guidance to match current repo workflow (
mcp/.claude-plugin/marketplace.json,make pluginexecution context). - Strengthen safety rules to explicitly cover direct execution of integration write scripts.
- Reword architecture claims to avoid guaranteeing one-to-one sync/async API parity.
No blocking correctness/security defects were identified; changes are requested as documentation fixes and clarifications.
AGENTS.md
Outdated
| Install for development: | ||
|
|
||
| ```bash | ||
| python -m pip install -e ".[dev]" |
There was a problem hiding this comment.
[major]: The safety section points to sandbox rules but the task examples include directly running tests/integration/test_sdk_writes.py, which bypasses pytest marker defaults and can execute write operations immediately. AGENTS.md should explicitly warn that direct script execution must follow the same sandbox-only constraints and confirmation practices to avoid accidental production writes.
Suggested fix: In Critical Rules, add a rule such as: 'Never execute integration write scripts directly unless sandbox credentials are loaded and sandbox tenant validation passes; prefer pytest -m integration for gated execution.' Also reference tests/integration/README.md standalone-script warnings.
AGENTS.md
Outdated
| - `affinity/` — Core SDK package (client, services, models, types, CLI modules). | ||
| - `tests/` — Unit/integration/CLI/service tests. Integration tests are sandbox-gated and skipped by default. | ||
| - `docs/public/` — Documentation source for MkDocs (guides, CLI docs, API reference, MCP docs). | ||
| - `mcp/` — Bash-based MCP server implementation, provider scripts, prompts, tools, and plugin assembly artifacts. |
There was a problem hiding this comment.
[major]: The document states that the repository marketplace metadata is under .claude-plugin/, but contributor docs and plugin flow indicate marketplace metadata is under mcp/.claude-plugin/marketplace.json while .claude-plugin/ at repo root is plugin source metadata. This can misdirect release/packaging work.
Suggested fix: Update AGENTS.md to distinguish root .claude-plugin/ (source metadata) from mcp/.claude-plugin/marketplace.json (repository marketplace manifest used in plugin workflow), matching CONTRIBUTING.md.
| ```bash | ||
| cd mcp | ||
| make plugin | ||
| ``` |
There was a problem hiding this comment.
[minor]: make plugin is documented as a common task, but CONTRIBUTING.md notes this assembles files into mcp/.claude-plugin/ and assumes execution from mcp/. Without that context, contributors may run it from repo root or expect committed outputs.
Suggested fix: Add one sentence that make plugin must be run from mcp/ and that generated files in mcp/.claude-plugin/ are assembly artifacts (git-ignored).
AGENTS.md
Outdated
| - **Dual API surface support** (V1 + V2 routing behavior documented in README/docs). | ||
| - **Sync + async parity** for client operations where supported. | ||
| - **CLI + SDK cohesion**: CLI behavior should map cleanly to SDK capabilities. | ||
| - **MCP Bash framework integration** in `mcp/` for AI client interoperability. |
There was a problem hiding this comment.
[major]: The architecture section states there is sync+async parity for client operations, but the implementation contains async-only operations in service modules (e.g., async methods in services/tasks.py and others) rather than strict one-to-one parity. This can mislead contributors into assuming every sync API has an async equivalent and vice versa.
Suggested fix: Rephrase to: 'Sync and async APIs are both supported across major workflows, but parity is not guaranteed for every operation; verify per-service methods before relying on one-to-one equivalents.'
1 participant
🔄 Overcut automatically updated AGENTS.md with latest repository analysis.