From 482fd601dd2854b81091cf82fd208237555b69bb Mon Sep 17 00:00:00 2001 From: Amit Gazal Date: Tue, 21 Jul 2026 09:49:54 +0300 Subject: [PATCH 1/8] feat: add dependabot-triage skill Adds a playbook skill for handling the repo's open Dependabot PRs as a single test-gated batch: survey the queue by ecosystem, consolidate every uv bump onto one local branch, prove it with the full suite (including crewai in its own venv), approve the PRs that pass with a signed comment, and leave excluded bumps open with a Linear follow-up. Motivated by an a2a-sdk major upgrade that got starved: its bump PR was closed unmerged, and the uv open-PR limit then saturated with untriaged green PRs, so Dependabot never re-proposed the newer version. The skill keeps the queue drained so the per-ecosystem limit can't silently starve future upgrades. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/skills/dependabot-triage/SKILL.md | 234 ++++++++++++++++++++++ 1 file changed, 234 insertions(+) create mode 100644 .github/skills/dependabot-triage/SKILL.md diff --git a/.github/skills/dependabot-triage/SKILL.md b/.github/skills/dependabot-triage/SKILL.md new file mode 100644 index 000000000..dc9825d2e --- /dev/null +++ b/.github/skills/dependabot-triage/SKILL.md @@ -0,0 +1,234 @@ +--- +name: dependabot-triage +description: Triage this repo's open Dependabot PRs in one batch run — consolidate every bump onto one branch, prove it with the full test suite, approve the PRs that pass (so a maintainer can merge them quickly), and open Linear issues for what needs manual work. Keeps the per-ecosystem open-PR limit from silently starving new upgrades. Use when asked to review, clear, batch, or "handle the Dependabot PRs", or when a dependency upgrade never showed up as a PR. +--- + +# Dependabot PR Triage + +Triage the whole Dependabot queue in a single, test-gated batch: consolidate all +the bumps onto one local branch, run the full suite, **approve** the PRs that +pass so a maintainer can merge them quickly, and file follow-ups for the rest. +The skill never merges — approval is its verdict; a human does the merge. The +goal is that after a run every mergeable bump carries the skill's sign-off and +every excluded bump has a paper trail — never a silent close. + +## Why this skill exists (the failure it prevents) + +Dependabot has a per-`(ecosystem, directory)` **`open-pull-requests-limit`** +(see `.github/dependabot.yml`: the root `uv` config caps at **10**, the +echo-agent `uv` config at **5**, GitHub Actions is grouped into one weekly PR). +When that limit is hit, Dependabot **will not open another PR for that +ecosystem** — including for a genuinely important upgrade — until an existing one +is merged or closed. No error, no notification; the new upgrade is simply never +proposed. + +That is how an important major upgrade got lost once: its PR was opened, then +**closed unmerged** (a breaking major nobody was ready for). Dependabot honored +the close ("I won't notify about *this release* again"), a newer version shipped +later, but by then the queue was full of green-but-unmerged PRs — the limit was +saturated, so the newer version was never re-proposed. A closed major PR plus a +saturated limit = a silently starved upgrade. + +So a full queue is an alarm, not a steady state. Drain it, and give every +excluded bump a tracked reason so nothing is starved by neglect. + +## Step 1 — Survey the queue + +List every open Dependabot PR, split by ecosystem, with CI status, age, and bump +size. GitHub Actions PRs are detected by the `/` in the dependency name (their +`github-actions` label is not always applied). + +```bash +gh pr list --state open --author "app/dependabot" --limit 100 \ + --json number,title,labels,createdAt,statusCheckRollup > /tmp/ddb-prs.json +python3 <<'PY' +import json, re, datetime as dt +prs = json.load(open("/tmp/ddb-prs.json")) +now = dt.datetime.now(dt.timezone.utc) +def dep(title): + m = re.search(r"bump ([^ ]+) from", title) + return m.group(1) if m else "?" +def bump(title): + m = re.search(r"from (\d+)[.\d]* to (\d+)[.\d]*", title) + return "?" if not m else ("MAJOR" if m.group(1) != m.group(2) else "minor/patch") +def eco(title, labels): + if "github-actions" in {l["name"] for l in labels}: return "github-actions" + return "github-actions" if "/" in dep(title) else "uv" +def ci(rollup): + st = [c.get("conclusion") or c.get("state") for c in (rollup or [])] + if any(s in ("FAILURE","ERROR","CANCELLED","TIMED_OUT") for s in st): return "RED" + if any(s in ("PENDING","IN_PROGRESS","QUEUED",None) for s in st): return "pending" + return "green" if st else "no-checks" +buckets = {} +for p in prs: + age = (now - dt.datetime.fromisoformat(p["createdAt"].replace("Z","+00:00"))).days + row = (p["number"], ci(p["statusCheckRollup"]), bump(p["title"]), dep(p["title"]), age, p["title"]) + buckets.setdefault(eco(p["title"], p["labels"]), []).append(row) +LIMITS = {"uv": 10, "github-actions": "grouped"} +for e, rows in sorted(buckets.items()): + print(f"\n== {e}: {len(rows)} open (limit {LIMITS.get(e,'?')}) ==") + for n, c, b, d, age, t in sorted(rows, key=lambda r: r[0]): + print(f" #{n:<5} {c:<8} {b:<11} {age:>3}d {d}") +PY +``` + +If an ecosystem's open count meets or exceeds its limit, new upgrades for it are +being starved right now — draining is urgent. + +**Handle the two ecosystems separately.** The consolidation below covers the +**`uv` (Python)** bumps, which all edit `uv.lock` and conflict with each other. +**GitHub Actions** bumps edit workflow YAML, never the lock, and Dependabot +already groups them — review and approve that grouped PR on its own once green; +don't fold it into the lock branch. + +## Step 2 — Open a Linear tracking issue + +Create one issue in the Integrations team for this triage run before touching +git, so the batch PR and any follow-ups link back to it. Record the surveyed +queue (the table from Step 1) in the description. Use the Linear MCP tools +(`create_issue` / `save_issue`). Keep the whole run under this issue. + +## Step 3 — Branch off `dev` + +Dependabot targets `dev`. Consolidate there. + +```bash +git fetch origin +git checkout -b chore/deps-batch-$(date +%Y%m%d) origin/dev +``` + +## Step 4 — Apply all the `uv` bumps at once + +Do **not** merge the individual branches (they all collide on `uv.lock`). Instead +reproduce their intent in one clean resolution: upgrade every Python package from +the survey in a single `uv lock`, pinning each to its PR's target version. + +```bash +uv lock \ + -P claude-agent-sdk==0.2.123 \ + -P openai==2.45.0 \ + -P langgraph==1.2.9 \ + -P uvicorn==0.51.0 \ + -P pytest-rerunfailures==16.4 \ + -P crewai==1.15.4 \ + -P pydantic-ai-slim==2.13.0 \ + -P langchain-community==0.4.2 \ + -P pytest-asyncio==1.4.0 \ + -P starlette==1.3.1 +# (fill -P flags from the survey; drop the version to just take latest-resolvable) +git add uv.lock +``` + +For the rare PR that **raises a constraint floor** in `pyproject.toml` (title +reads "update … requirement from >=X to >=Y", not "bump … from X to Y"), also +apply that `pyproject.toml` edit and `git add` it — a lock bump alone won't +reproduce it. Then commit: + +```bash +git commit -m "chore(deps): batch Dependabot bumps" +``` + +## Step 5 — Run the full suite + +Prove the consolidated set. The lint/type gate and unit tests are the floor; +integration and e2e run when their credentials are present (see CLAUDE.md's +Environment Variables — they need Band + LLM keys, and e2e needs +`E2E_TESTS_ENABLED=true`). + +```bash +uv sync --extra dev +uv run pre-commit run --all-files # ruff + pyrefly gate +uv run pytest tests/ --ignore=tests/integration/ --ignore=tests/e2e/ # unit +uv run pytest tests/integration/ -v -s --no-cov # needs BAND_API_KEY_USER +E2E_TESTS_ENABLED=true uv run pytest tests/e2e/baseline/ -v -s --no-cov # needs live platform + LLM keys +``` + +**crewai must be tested in its own venv** — it conflicts with parlant/pydantic-ai +and is absent from the `dev` extra, so the run above never exercises the crewai +bump: + +```bash +uv sync --extra dev-crewai +uv run pytest tests/adapters/ -k crewai -v +``` + +## Step 6 — Decide which bumps to keep (bisect on failure) + +If everything is green, the whole set is good — keep it all. + +If a check fails, the failing area usually names the culprit (a `pydantic_ai` +test failing ⇒ the `pydantic-ai-slim` bump; a crewai test ⇒ the crewai bump). A +bump whose **own** Dependabot PR was already RED in the survey (e.g. a breaking +major) is the first suspect. Drop the suspect from the upgrade set and re-lock +without it, then re-test: + +```bash +# Re-run Step 4's uv lock omitting the suspect's -P flag (so it stays at its +# current version), then re-run Step 5. Repeat until green. +``` + +Each dropped package is a bump you are **not** approving — it goes to Step 8. + +## Step 7 — Approve the passing PRs + +Don't merge. The consolidation branch was only the test vehicle (keep it local, +don't push it). For **each** individual Dependabot PR whose bump was in the green +set, post an approving review with a comment recording that the skill validated +it — a maintainer then does the actual merge. + +```bash +gh pr review --approve --body "$(cat <<'MSG' +✅ Approved by the dependabot-triage skill. + +Validated in a consolidated batch run: this bump was applied together with the +other passing Dependabot bumps on a local branch and the full suite (ruff + +pyrefly, unit, crewai in dev-crewai, and integration/e2e where credentials were +available) passed with all of them in place — so it's compatible with the rest +of the batch, not just green on its own. Safe to merge. +MSG +)" +``` + +Report the approved set to the maintainer so they can merge them. Merging is +what actually drains the queue and reopens the open-PR limit — the skill's +approval is the green light for that, not a substitute for it. + +## Step 8 — File follow-ups for the excluded bumps + +Every dropped/failing bump gets a Linear issue (sub-issue of Step 2's), stating +the package, the target version, and the failure it caused, so a breaking upgrade +is tracked work rather than something forgotten. + +**Leave the excluded PRs open and unapproved.** Don't close them and don't add an +`ignore` entry — the open PR is the live reminder that the upgrade is pending, and +it carries Dependabot's own changelog/compatibility notes for whoever picks up the +migration. Once the maintainer merges the approved set, most slots reopen; the few +excluded PRs staying in place is intended. + +## Step 9 — Recover an upgrade that was already starved or closed + +If a dependency's upgrade never appeared (the incident above): + +1. Confirm the ecosystem limit was saturated (Step 1). Approving (Steps 3–7) and + getting the maintainer to merge drains it, which lets the next weekly run + re-propose on schedule. +2. If the upgrade's PR was previously **closed unmerged**, Dependabot won't + re-propose that exact version, only a newer one. To pull it now, just include + it in the batch: add its `-P package==version` to Step 4 and let the test gate + judge it like any other bump. + +## Guardrails + +- **The test gate is the arbiter — never approve a bump that wasn't in a green + batch**, and never drop a *passing* bump just to shrink the diff. The skill + approves; it never merges. +- **Test crewai in `dev-crewai`.** A crewai bump validated only in the `dev` venv + is untested — the default resolution excludes it. +- **Do not raise `open-pull-requests-limit` as the fix.** A higher ceiling only + defers triage and hides the alarm; the queue still needs draining. +- **Every excluded bump gets a Linear issue and stays open as a PR** — never a + silent close, and never an `ignore` entry. The open PR is the reminder that the + upgrade is still pending. +- **Majors get human sign-off** even when green, especially framework/adapter + dependencies where a major can change behavior the SDK relies on. Surface them + in the report rather than approving them unremarked. From d313e27cfdbd0b3f047772d08c959cc00fd82ec3 Mon Sep 17 00:00:00 2001 From: Amit Gazal Date: Tue, 21 Jul 2026 16:13:40 +0300 Subject: [PATCH 2/8] docs: target main in dependabot-triage skill (single-trunk flow) Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/skills/dependabot-triage/SKILL.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/skills/dependabot-triage/SKILL.md b/.github/skills/dependabot-triage/SKILL.md index dc9825d2e..3526c699f 100644 --- a/.github/skills/dependabot-triage/SKILL.md +++ b/.github/skills/dependabot-triage/SKILL.md @@ -88,13 +88,13 @@ git, so the batch PR and any follow-ups link back to it. Record the surveyed queue (the table from Step 1) in the description. Use the Linear MCP tools (`create_issue` / `save_issue`). Keep the whole run under this issue. -## Step 3 — Branch off `dev` +## Step 3 — Branch off `main` -Dependabot targets `dev`. Consolidate there. +Dependabot targets `main` (single-trunk GitHub flow). Consolidate there. ```bash git fetch origin -git checkout -b chore/deps-batch-$(date +%Y%m%d) origin/dev +git checkout -b chore/deps-batch-$(date +%Y%m%d) origin/main ``` ## Step 4 — Apply all the `uv` bumps at once From 263447564cf0ddb045c70a3dbb922d29ec38f57a Mon Sep 17 00:00:00 2001 From: Amit Gazal Date: Thu, 23 Jul 2026 08:55:08 +0300 Subject: [PATCH 3/8] refactor: drop claude-config submodule, inline rules into AGENTS.md, relocate skill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove the `claude-config` git submodule (band-ai/claude-config) mounted at `.claude`, along with its `.gitmodules` entry. - Inline its shared rules (`.claude/rules/00-05,07`) into AGENTS.md under a new "## Engineering Rules" section. CLAUDE.md symlinks to AGENTS.md, so both Claude Code and Codex load them from one file. Dropped the obsolete `06-claude-config-management` rule and the now-false "clone with --recurse-submodules" instructions. - Move the dependabot-triage skill from `.github/skills/` (not a discovery path) to `.claude/skills/dependabot-triage/` so Claude discovers it as a project skill — now repo-local instead of living in the shared submodule. - Keep `.claude/settings.local.json` machine-local (gitignored). Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude | 1 - .../skills/dependabot-triage/SKILL.md | 118 ++- .gitignore | 3 + .gitmodules | 3 - AGENTS.md | 721 ++++++++++++++++++ 5 files changed, 836 insertions(+), 10 deletions(-) delete mode 160000 .claude rename {.github => .claude}/skills/dependabot-triage/SKILL.md (60%) delete mode 100644 .gitmodules diff --git a/.claude b/.claude deleted file mode 160000 index 462101565..000000000 --- a/.claude +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 46210156573e6d0b9194d0ddccc906a77a77e510 diff --git a/.github/skills/dependabot-triage/SKILL.md b/.claude/skills/dependabot-triage/SKILL.md similarity index 60% rename from .github/skills/dependabot-triage/SKILL.md rename to .claude/skills/dependabot-triage/SKILL.md index 3526c699f..891f344a1 100644 --- a/.github/skills/dependabot-triage/SKILL.md +++ b/.claude/skills/dependabot-triage/SKILL.md @@ -8,9 +8,12 @@ description: Triage this repo's open Dependabot PRs in one batch run — consoli Triage the whole Dependabot queue in a single, test-gated batch: consolidate all the bumps onto one local branch, run the full suite, **approve** the PRs that pass so a maintainer can merge them quickly, and file follow-ups for the rest. -The skill never merges — approval is its verdict; a human does the merge. The -goal is that after a run every mergeable bump carries the skill's sign-off and -every excluded bump has a paper trail — never a silent close. +By default the skill stops at approval — approval is its verdict and a human +merges; when you are *explicitly asked to drive the merges too*, follow the +"Merging the approved set" section below (it is a serial grind with real +gotchas, not a batch button). The goal is that after a run every mergeable bump +carries the skill's sign-off and every excluded bump has a paper trail — never a +silent close. ## Why this skill exists (the failure it prevents) @@ -88,6 +91,11 @@ git, so the batch PR and any follow-ups link back to it. Record the surveyed queue (the table from Step 1) in the description. Use the Linear MCP tools (`create_issue` / `save_issue`). Keep the whole run under this issue. +Set the tracking issue (and each Step 8 sub-issue) to **Todo**, **assign it to +the triager**, and put it in the **current cycle** — resolve "current" via +`list_cycles(type="current")` for the Integrations team and pass that cycle +number, rather than assuming a name. + ## Step 3 — Branch off `main` Dependabot targets `main` (single-trunk GitHub flow). Consolidate there. @@ -128,6 +136,21 @@ reproduce it. Then commit: git commit -m "chore(deps): batch Dependabot bumps" ``` +**Exact-pinned deps can't be moved with `-P`.** `crewai` is pinned exactly +(`crewai==X`) in *two* places in `pyproject.toml` (the `crewai` and `dev-crewai` +extras). `uv lock -P crewai==` fails as unsatisfiable ("depends on +crewai==X and crewai==Y"). Edit both pins to the target version and re-lock, and +update any nearby comment that cites the old version's transitive pins (the +`tool.uv.conflicts` rationale) so it stays accurate. Because crewai lives in its +own conflict fork, commit the crewai pin+lock change on its own once its +`dev-crewai` tests pass (below). + +**Drop a known-breaking major from the lock up front.** A bump whose own PR is +already RED in the survey (typically a `1.x → 2.x` major like `pydantic-ai-slim`) +will usually make the whole `uv lock` fail to resolve, or fail the suite — so +omit its `-P` flag from Step 4 from the start rather than discovering it in +Step 6. It goes to Step 8 as an excluded bump. + ## Step 5 — Run the full suite Prove the consolidated set. The lint/type gate and unit tests are the floor; @@ -143,13 +166,33 @@ uv run pytest tests/integration/ -v -s --no-cov # needs BA E2E_TESTS_ENABLED=true uv run pytest tests/e2e/baseline/ -v -s --no-cov # needs live platform + LLM keys ``` +**Actually run integration and e2e — don't pre-declare them "env-blocked."** If +they fail, isolate the cause (Step 6) rather than assuming; a real config bug +hides behind that assumption. The e2e baseline provisions its own agents (only +needs `BAND_API_KEY_USER`) and can take a while. + +**Stream long runs to a log file, never pipe through `tail`/`head`.** A pipe +buffers the whole run until the process exits, so a hang looks identical to +progress. Redirect to a file and watch that instead: + +```bash +E2E_TESTS_ENABLED=true uv run pytest tests/e2e/baseline/ -v --no-cov > /tmp/e2e.log 2>&1 & +# then tail -f /tmp/e2e.log (watch for PASSED|FAILED|ERROR|403|429|Traceback and the final tally) +``` + **crewai must be tested in its own venv** — it conflicts with parlant/pydantic-ai and is absent from the `dev` extra, so the run above never exercises the crewai -bump: +bump. Target the crewai test **files** explicitly: a bare `-k crewai` over +`tests/adapters/` still tries to *collect* every adapter module, and the +non-crewai ones fail to import in the crewai-only venv (collection error). ```bash uv sync --extra dev-crewai -uv run pytest tests/adapters/ -k crewai -v +uv run pytest tests/adapters/test_crewai_adapter.py \ + tests/adapters/test_crewai_flow_adapter.py \ + tests/adapters/test_crewai_flow_phase3.py tests/adapters/test_crewai_flow_phase4.py \ + tests/adapters/test_crewai_flow_phase5.py tests/adapters/test_crewai_flow_state_source.py \ + tests/adapters/test_crewai_adapter_soak.py tests/converters/test_crewai.py -v ``` ## Step 6 — Decide which bumps to keep (bisect on failure) @@ -167,7 +210,26 @@ without it, then re-test: # current version), then re-run Step 5. Repeat until green. ``` -Each dropped package is a bump you are **not** approving — it goes to Step 8. +**Attribute each failure before dropping a bump — most integration/e2e failures +are environmental, not the bumps.** These run against a live platform and a +multi-lane matrix, so isolate with a minimal probe before blaming a package. +Signatures seen in practice that are **not** bump regressions: + +- WebSocket `403` on connect **plus** a `422` "Expected :uuid" on a mention ⇒ a + stale `TEST_AGENT_ID` / `BAND_AGENT_ID` in the test env that doesn't match the + agent `BAND_API_KEY` owns. The SDK appends `&agent_id=` to the socket URL and + the platform rejects an unauthorized pairing. Confirm by connecting the WS with + vs. without `&agent_id=`; fix the env, don't drop a bump. +- WebSocket `429` "reconnect rate-limited after recent supersede" ⇒ too many + connect/disconnect cycles on one agent from repeated runs — back off, not a bump. +- e2e errors for out-of-lane adapters (gemini/codex/opencode/letta reporting + "not set" / "no server", crewai absent from the `dev` venv) and memory + `403 plan_required` (Enterprise-plan-gated API) ⇒ lane / creds / plan, not bumps. + A `copilot_acp` narration assertion is a known flaky out-of-process bridge. + +Only a failure that survives this triage **and** points at a bumped package +justifies dropping that bump. Each dropped package is a bump you are **not** +approving — it goes to Step 8. ## Step 7 — Approve the passing PRs @@ -217,6 +279,44 @@ If a dependency's upgrade never appeared (the incident above): it in the batch: add its `-P package==version` to Step 4 and let the test gate judge it like any other bump. +## Merging the approved set (only when explicitly asked to drive it) + +Approval is the default verdict — a maintainer merges. If you *are* asked to +merge, know that this repo's setup makes it a **serial grind**, not a batch: + +- **All root-`uv` PRs conflict on `uv.lock`.** Merge one and the rest instantly + go stale; Dependabot must rebase each before it can merge. So: merge one → wait + for the next to rebase + go green → merge it → repeat, one at a time. Poll + `gh pr view --json mergeStateStatus` for `CLEAN`. The echo-agent lock (own + file) and the gh-actions group (YAML) are independent — they don't collide with + the root set. Nudge a lagging rebase with a `@dependabot rebase` comment. +- **`main`'s ruleset dismisses stale approvals and requires branches up to date + (strict).** Every Dependabot rebase/force-push wipes the approval, so + **re-approve as the last step immediately before merging**, not before CI — + otherwise the PR sits `BLOCKED` on "review required" despite green checks. + Auto-merge is disabled repo-wide; do **not** `--admin`-bypass required checks. +- **The `uv lock --check` specifier-drift trap.** CI's lint job runs + `uv lock --check`. Dependabot's version-only lock bump does *not* reconcile + specifier metadata that has drifted on `main` (e.g. an `openai>=1.0.0 → >=2.0.0` + floor raised across extras), so the PR's lint fails and **`@dependabot rebase` + and `recreate` never fix it** (it loops). Fix it yourself: rebuild the lock on + current `main` and force-push it to the Dependabot branch (needs push + permission), then re-approve + merge: + + ```bash + git fetch origin main + git checkout -B lockfix origin/main + uv lock -P == # rebuilds the whole lock on current main + uv lock --check # must pass now + git commit -am "chore(deps): rebuild uv.lock for on current main" + git push --force origin "HEAD:$(gh pr view --json headRefName -q .headRefName)" + ``` + +- **Never merge a stale-based lock PR.** Its `uv.lock` predates the other merges + and a squash-merge would silently *revert* them (a PR based on pre-openai-merge + `main` drops openai back down). Always rebase/rebuild onto current `main` first. +- Merge each with `gh pr merge --squash --delete-branch`. + ## Guardrails - **The test gate is the arbiter — never approve a bump that wasn't in a green @@ -232,3 +332,9 @@ If a dependency's upgrade never appeared (the incident above): - **Majors get human sign-off** even when green, especially framework/adapter dependencies where a major can change behavior the SDK relies on. Surface them in the report rather than approving them unremarked. +- **Run integration and e2e for real, and attribute every failure** (Step 6) + before dropping a bump — live-platform/lane/plan/env failures are common and + are not the bumps. Never call a suite "env-blocked" on assumption. +- **If you drive merges: re-approve last, never `--admin`-bypass CI, and never + merge a stale-based lock PR** (it silently reverts already-merged bumps). See + "Merging the approved set." diff --git a/.gitignore b/.gitignore index 9f23a3a33..4f2be8950 100644 --- a/.gitignore +++ b/.gitignore @@ -177,3 +177,6 @@ uv.toml # Scripts config (contains local paths) scripts/config.yaml .a5c/ + +# Local Claude Code settings (machine-specific, not shared) +.claude/settings.local.json diff --git a/.gitmodules b/.gitmodules deleted file mode 100644 index 53c0612ea..000000000 --- a/.gitmodules +++ /dev/null @@ -1,3 +0,0 @@ -[submodule ".claude"] - path = .claude - url = git@github.com:band-ai/claude-config.git diff --git a/AGENTS.md b/AGENTS.md index ecd3bc426..97c7a0c6b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -650,3 +650,724 @@ uv run ruff format . uv run pyrefly check uv run pytest tests/ --ignore=tests/integration/ --ignore=tests/e2e/ -v ``` + + +## Engineering Rules + +_Inlined from the former shared `claude-config` submodule (`.claude/rules/`), now removed. These apply repo-wide. (The old `06-claude-config-management` rule is intentionally dropped — it described managing that submodule.)_ + +### Coding Standards + +#### Type Hints + +- Always use type hints for function parameters and return types +- Use `from __future__ import annotations` as the first import in every file +- Use `TypeVar` and `Generic` for typed generic classes +- Use `Protocol` classes for interfaces (not ABC) + +#### Logging + +##### Core Rule + +- **NEVER use `print()` statements** - always use logging instead +- When encountering `print()` in existing code, replace it with appropriate logging + +##### Logger Setup + +Every module should have a module-level logger: + +```python +import logging + +logger = logging.getLogger(__name__) +``` + +##### Log Levels + +Use the appropriate level for each message: + +| Level | Use For | +|-------|---------| +| `logger.debug()` | Detailed diagnostic info, variable values, flow tracing | +| `logger.info()` | Normal operations, startup messages, successful completions | +| `logger.warning()` | Unexpected but handled situations, deprecations | +| `logger.error()` | Failures that prevented an operation from completing | +| `logger.exception()` | Errors with full traceback (use inside except blocks) | + +##### Converting Print to Logging + +```python +# Bad +print(f"Processing {item}") +print(f"Error: {e}") + +# Good +logger.info("Processing %s", item) +logger.error("Failed to process: %s", e) +``` + +##### String Formatting + +Use `%s` placeholders instead of f-strings for log messages: + +```python +# Preferred - lazy evaluation, better performance +logger.debug("User %s performed action %s", user_id, action) + +# Acceptable but less efficient +logger.debug(f"User {user_id} performed action {action}") +``` + +##### Structured Context + +Include relevant context in log messages: + +```python +logger.info("Request completed", extra={"user_id": user_id, "duration_ms": duration}) +logger.error("API call failed: %s", error, extra={"endpoint": url, "status": status_code}) +``` + +#### Imports + +- Use absolute imports from the package root +- Sort imports with isort (configured via ruff) +- Group imports: stdlib, third-party, local + +#### Data Models + +- Use Pydantic v2 for data models and validation +- Define models with proper field types and validators +- Use `model_dump()` instead of deprecated `dict()` + +#### Python Version + +- Target Python 3.10+ (match statements are OK) +- Use modern syntax: `list[str]` instead of `List[str]` +- Use `|` for union types: `str | None` instead of `Optional[str]` + +#### Async/Await + +- Use async/await everywhere in async codebases +- No sync code in async adapters or handlers +- Use `asyncio.gather()` for concurrent operations +- Use `AsyncMock` for testing async methods + +#### Documentation + +- Use Context7 MCP to fetch up-to-date documentation when needed +- Follow existing patterns in the codebase for new code + + +### Error Handling + +#### Pydantic ValidationError + +- Catch `pydantic.ValidationError` separately from generic `Exception` +- Format validation errors for LLM readability: `"Invalid arguments for tool_name: field: message"` +- Handle ValidationError at the lowest common point to avoid duplication +- Log full error details but return concise messages to LLM + +Example: +```python +from pydantic import ValidationError + +try: + result = Model(**data) +except ValidationError as e: + # Log full details for debugging + logger.error(f"Validation failed: {e}") + # Return concise message for LLM + errors = "; ".join(f"{err['loc'][0]}: {err['msg']}" for err in e.errors()) + return f"Invalid arguments for {tool_name}: {errors}" +except Exception as e: + logger.exception(f"Unexpected error: {e}") + raise +``` + +#### Exception Hierarchy + +- Use specific exceptions over generic ones +- Create custom exception classes for domain-specific errors +- Always include context in exception messages + +#### Error Messages + +- Make error messages actionable and clear +- Include relevant context (what failed, why, what to do) +- Avoid exposing internal implementation details to end users + +#### Required Configuration + +- Use `raise ValueError(...)` for missing required configuration +- Do NOT use `logger.error()` + `sys.exit()` pattern +- Fail fast with clear error messages + +Example: +```python +# Good +if not api_key: + raise ValueError("OPENAI_API_KEY environment variable is required") + +# Bad +if not api_key: + logger.error("Missing API key") + sys.exit(1) +``` + + +### Git Workflow + +#### Branch Naming + +Branch names should match the Linear issue: + +- Format: `/-<ISSUE-ID>` +- Example: `feat/add-user-auth-ENG-123` + +Prefixes: + +- `feat/` - New features +- `fix/` - Bug fixes +- `refactor/` - Code refactoring +- `docs/` - Documentation changes +- `chore/` - Maintenance tasks + +##### Creating Branches from Linear Issues + +Use `git lb` to create properly named branches from Linear issues: + +```bash +git lb INT-84 +``` + +This automatically fetches the issue title from Linear and creates a branch with the correct naming convention. + +If `git lb` is not installed, ask the developer for the proper branch name. + +#### Commit Messages + +Follow conventional commits format for all commits: + +``` +<type>: <description> + +[optional body] + +[optional footer] +``` + +Types: +- `feat:` - New feature +- `fix:` - Bug fix +- `docs:` - Documentation only +- `refactor:` - Code refactoring +- `test:` - Adding or updating tests +- `chore:` - Maintenance tasks + +#### Pull Request Titles + +PR titles MUST use conventional commits format: + +- `feat:` - New features +- `fix:` - Bug fixes +- `docs:` - Documentation changes + +Examples: +- `feat: Add custom tools support to all adapters` +- `fix: Handle validation errors in execute_tool_call` +- `docs: Update README with new adapter examples` + +#### Pre-Commit Checklist + +- Run tests before committing +- Run linting and formatting +- Ensure type checking passes +- Review changes with `git diff` + +#### Code Review + +- Keep PRs focused and reasonably sized +- Respond to review comments promptly +- Squash commits when merging if history is messy + + +### Code Quality + +#### Linting with Ruff + +Ruff is used for linting and import sorting. + +```bash +# Check for linting issues +uv run ruff check . + +# Auto-fix issues +uv run ruff check . --fix +``` + +##### Common Ruff Rules + +- `E` - pycodestyle errors +- `F` - Pyflakes +- `I` - isort (import sorting) +- `UP` - pyupgrade (modern Python syntax) +- `B` - flake8-bugbear (common bugs) + +##### Typical Ruff Configuration + +```toml +# pyproject.toml +[tool.ruff] +line-length = 88 +target-version = "py310" + +[tool.ruff.lint] +select = ["E", "F", "I", "UP", "B"] +ignore = [ + "E501", # Line too long (handled by formatter) +] + +[tool.ruff.lint.isort] +known-first-party = ["thenvoi"] # Replace with your package name +``` + +#### Formatting with Ruff + +```bash +# Format code +uv run ruff format . + +# Check formatting without changes +uv run ruff format . --check +``` + +##### Formatting Standards + +- Line length: 88 characters (Black default) +- Use double quotes for strings +- Trailing commas in multi-line structures + +#### Type Checking with Pyrefly + +Pyrefly is used for type checking (not mypy). + +```bash +# Run type checker +uv run pyrefly check +``` + +##### Type Checking Notes + +- Fix type errors before committing +- Use `# type: ignore` sparingly with explanation +- Prefer proper typing over ignoring errors + +##### When to Use `# type: ignore` + +Use type ignores only when: +- Third-party library has incomplete type stubs +- Dynamic behavior that can't be expressed in types +- Workaround for known type checker limitations + +Always include a comment explaining why: + +```python +# type: ignore[arg-type] # langgraph returns untyped dict +result = some_dynamic_call() + +# type: ignore[assignment] # Pydantic model validates at runtime +self.value = untrusted_input +``` + +##### Known Typing Limitations + +- Some async patterns may need explicit type annotations +- Generic callbacks across frameworks may need `Any` +- Dynamic tool registration may require runtime validation + +#### Package Management with uv + +After cloning a repository, always install dev dependencies: + +```bash +# Install with dev dependencies (required for development) +uv sync --extra dev +``` + +Other common commands: + +```bash +# Install production dependencies only +uv sync + +# Add a dependency +uv add <package> + +# Add optional dependency to a group +uv add --optional <group> <package> +``` + +#### Pre-Commit Workflow + +Run these before every commit: + +```bash +uv run ruff check . +uv run ruff format . +uv run pyrefly check +uv run pytest tests/ --ignore=tests/integration/ -v +``` + + +### Testing Conventions + +#### Test Framework + +- Use pytest as the test framework +- Use pytest-asyncio for async tests + +#### Running Tests + +```bash +# Run unit tests +uv run pytest tests/ --ignore=tests/integration/ -v + +# Run single test by name +uv run pytest tests/ -k "test_name" + +# Run with coverage +uv run pytest tests/ --ignore=tests/integration/ --cov=src/<package> + +# Run integration tests (requires API credentials) +uv run pytest tests/integration/ -v -s --no-cov +``` + +#### Async Tests + +- Use `@pytest.mark.asyncio` decorator for async tests +- Use `AsyncMock` for mocking async methods +- Use `MagicMock` for sync methods + +```python +import pytest +from unittest.mock import AsyncMock, MagicMock + +@pytest.mark.asyncio +async def test_async_function(): + mock_client = AsyncMock() + mock_client.fetch.return_value = {"data": "value"} + + result = await some_async_function(mock_client) + + assert result == expected + mock_client.fetch.assert_called_once() +``` + +#### Test Organization + +``` +tests/ +├── conftest.py # Shared fixtures +├── fixtures.py # Test data factories +├── unit/ # Unit tests (mocked dependencies) +├── integration/ # Real API tests (skipped in CI) +└── e2e/ # End-to-end tests +``` + +#### Fixtures + +- Define shared fixtures in `conftest.py` +- Use `MockDataFactory` or similar for creating test data +- Scope fixtures appropriately (function, class, module, session) + +```python +# conftest.py +import pytest +from unittest.mock import AsyncMock + +@pytest.fixture +def mock_client(): + return AsyncMock() + +@pytest.fixture +def sample_message(): + return {"role": "user", "content": "Hello"} +``` + +#### Test Data Factory Pattern + +Create a factory class for generating consistent test data: + +```python +# tests/fixtures.py +from dataclasses import dataclass + +@dataclass +class MockDataFactory: + @staticmethod + def make_message(role: str = "user", content: str = "test"): + return {"role": role, "content": content} + + @staticmethod + def make_tool_call(name: str = "test_tool", args: dict | None = None): + return {"name": name, "arguments": args or {}} + + @staticmethod + def make_event(event_type: str = "message", **kwargs): + return {"type": event_type, **kwargs} +``` + +Use in tests: + +```python +from tests.fixtures import MockDataFactory + +def test_message_handling(): + message = MockDataFactory.make_message(role="assistant", content="Hello") + # test code +``` + +#### Skip Markers + +Use markers to categorize tests: + +```python +@pytest.mark.requires_api +async def test_real_api_call(): + """Skipped in CI, requires real API credentials.""" + pass + +@pytest.mark.slow +def test_performance(): + """Skipped by default, run with --slow flag.""" + pass +``` + +Configure in `pyproject.toml`: +```toml +[tool.pytest.ini_options] +markers = [ + "requires_api: marks tests as requiring real API (skipped in CI)", + "slow: marks tests as slow (skipped by default)", +] +``` + +#### Test Performance + +- Override slow defaults for faster tests (e.g., `_max_iterations = 3`) +- Use `pytest-xdist` for parallel test execution when needed +- Keep unit tests fast (< 100ms each) + +#### Integration Tests + +- Store test credentials in `.env.test` +- Never commit real credentials +- Skip integration tests in CI by default +- Use separate test accounts/environments + + +### Optional Dependencies Pattern + +#### Overview + +Use optional dependencies to keep the core package lightweight while supporting multiple frameworks. + +#### pyproject.toml Configuration + +```toml +[project.optional-dependencies] +langgraph = ["langgraph", "langchain-openai"] +anthropic = ["anthropic"] +crewai = ["crewai"] +dev = ["pytest", "pytest-asyncio", "ruff", "pyrefly"] +``` + +#### Lazy Imports in Adapters + +Adapters should import framework dependencies lazily to avoid import errors when the optional dependency is not installed. + +```python +# Good - lazy import inside the adapter +class LangGraphAdapter: + def __init__(self): + try: + from langgraph.graph import StateGraph + except ImportError: + raise ImportError( + "langgraph is required for LangGraphAdapter. " + "Install with: pip install thenvoi[langgraph]" + ) + self.graph = StateGraph() + +# Bad - top-level import +from langgraph.graph import StateGraph # Fails if langgraph not installed + +class LangGraphAdapter: + pass +``` + +#### Testing with Optional Dependencies + +Skip tests when optional dependencies are missing: + +```python +import pytest + +try: + import langgraph + HAS_LANGGRAPH = True +except ImportError: + HAS_LANGGRAPH = False + +@pytest.mark.skipif(not HAS_LANGGRAPH, reason="langgraph not installed") +def test_langgraph_adapter(): + from mypackage.adapters.langgraph import LangGraphAdapter + # test code +``` + +Or use a marker: + +```python +requires_langgraph = pytest.mark.skipif( + not HAS_LANGGRAPH, + reason="langgraph not installed" +) + +@requires_langgraph +def test_langgraph_feature(): + pass +``` + +#### Installation Commands + +```bash +# Install core only +uv add thenvoi + +# Install with specific adapter +uv add thenvoi[langgraph] +uv add thenvoi[anthropic] + +# Install with multiple adapters +uv add thenvoi[langgraph,anthropic] + +# Install with dev dependencies +uv add thenvoi[dev] +``` + + +### GitHub PR Inline Comments + +#### Adding Inline Review Comments + +To add inline comments at specific lines in a PR, use the GitHub Reviews API with `gh api`: + +```bash +cat << 'EOF' | gh api repos/{owner}/{repo}/pulls/{pr_number}/reviews --method POST --input - +{ + "commit_id": "<commit_sha>", + "event": "COMMENT", + "body": "Review summary", + "comments": [ + { + "path": "src/path/to/file.py", + "line": 42, + "body": "Your comment here" + } + ] +} +EOF +``` + +#### Getting the Correct Line Numbers + +**Important:** Line numbers must be from the NEW version of the file, not diff line numbers. + +1. Get the commit SHA: + ```bash + gh pr view {pr_number} --json headRefOid -q .headRefOid + ``` + +2. Find correct line numbers in the actual file: + ```bash + # Get the file content at the PR's HEAD commit + curl -s "https://raw.githubusercontent.com/{owner}/{repo}/{commit_sha}/path/to/file.py" | grep -n "pattern" + ``` + +3. Alternatively, use the diff with grep: + ```bash + gh pr diff {pr_number} | grep -n "pattern_to_find" + ``` + Note: These are diff line numbers, not file line numbers. Use the actual file method above for accuracy. + +#### Common Mistakes to Avoid + +- **Don't use `gh pr review --comment`** - This adds a general comment, not inline comments +- **Don't use diff line numbers** - Use actual file line numbers from the new version +- **Don't use `-f` flag for JSON arrays** - Pass JSON via stdin with `--input -` +- **Don't guess line numbers** - Always verify by checking the actual file content + +#### Example: Full Workflow + +```bash +# 1. Get commit SHA +COMMIT=$(gh pr view 83 --json headRefOid -q .headRefOid) + +# 2. Find the line number for a specific pattern +curl -s "https://raw.githubusercontent.com/owner/repo/${COMMIT}/src/file.py" | grep -n "def my_function" + +# 3. Add inline comment at that line +cat << 'EOF' | gh api repos/owner/repo/pulls/83/reviews --method POST --input - +{ + "commit_id": "abc123...", + "event": "COMMENT", + "body": "Code review", + "comments": [ + { + "path": "src/file.py", + "line": 25, + "body": "Consider renaming this function for clarity" + } + ] +} +EOF +``` + +#### Multiple Comments + +Add multiple inline comments in a single review: + +```bash +cat << 'EOF' | gh api repos/owner/repo/pulls/83/reviews --method POST --input - +{ + "commit_id": "abc123...", + "event": "COMMENT", + "body": "Review with multiple comments", + "comments": [ + { + "path": "src/file.py", + "line": 14, + "body": "First comment" + }, + { + "path": "src/file.py", + "line": 42, + "body": "Second comment" + }, + { + "path": "src/other_file.py", + "line": 10, + "body": "Comment on different file" + } + ] +} +EOF +``` + +#### Review Events + +The `event` field can be: +- `"COMMENT"` - Submit general feedback without approval +- `"APPROVE"` - Approve the PR +- `"REQUEST_CHANGES"` - Request changes before merging + From f45ebba46affdc332f9f06a3cc5432c8cfb9689d Mon Sep 17 00:00:00 2001 From: Amit Gazal <amit.gazal@thenvoi.com> Date: Thu, 23 Jul 2026 09:05:17 +0300 Subject: [PATCH 4/8] docs: mark embedded rule code fences as notest The rules inlined into AGENTS.md (## Engineering Rules) carry illustrative python snippets (undefined logger/pytest/api_key/MockDataFactory, etc.). In the former claude-config submodule these were never executed by pytest-markdown-docs (submodule files aren't in the main repo's git ls-files); inlined, CI ran them and 18 fences failed. Mark the 15 in-section python fences `notest` to match their illustrative intent, matching the repo's Documentation Testing convention. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --- AGENTS.md | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index be5ceff82..3ba07b490 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -678,7 +678,7 @@ _Inlined from the former shared `claude-config` submodule (`.claude/rules/`), no Every module should have a module-level logger: -```python +```python notest import logging logger = logging.getLogger(__name__) @@ -698,7 +698,7 @@ Use the appropriate level for each message: ##### Converting Print to Logging -```python +```python notest # Bad print(f"Processing {item}") print(f"Error: {e}") @@ -712,7 +712,7 @@ logger.error("Failed to process: %s", e) Use `%s` placeholders instead of f-strings for log messages: -```python +```python notest # Preferred - lazy evaluation, better performance logger.debug("User %s performed action %s", user_id, action) @@ -724,7 +724,7 @@ logger.debug(f"User {user_id} performed action {action}") Include relevant context in log messages: -```python +```python notest logger.info("Request completed", extra={"user_id": user_id, "duration_ms": duration}) logger.error("API call failed: %s", error, extra={"endpoint": url, "status": status_code}) ``` @@ -770,7 +770,7 @@ logger.error("API call failed: %s", error, extra={"endpoint": url, "status": sta - Log full error details but return concise messages to LLM Example: -```python +```python notest from pydantic import ValidationError try: @@ -805,7 +805,7 @@ except Exception as e: - Fail fast with clear error messages Example: -```python +```python notest # Good if not api_key: raise ValueError("OPENAI_API_KEY environment variable is required") @@ -973,7 +973,7 @@ Use type ignores only when: Always include a comment explaining why: -```python +```python notest # type: ignore[arg-type] # langgraph returns untyped dict result = some_dynamic_call() @@ -1050,7 +1050,7 @@ uv run pytest tests/integration/ -v -s --no-cov - Use `AsyncMock` for mocking async methods - Use `MagicMock` for sync methods -```python +```python notest import pytest from unittest.mock import AsyncMock, MagicMock @@ -1082,7 +1082,7 @@ tests/ - Use `MockDataFactory` or similar for creating test data - Scope fixtures appropriately (function, class, module, session) -```python +```python notest # conftest.py import pytest from unittest.mock import AsyncMock @@ -1100,7 +1100,7 @@ def sample_message(): Create a factory class for generating consistent test data: -```python +```python notest # tests/fixtures.py from dataclasses import dataclass @@ -1121,7 +1121,7 @@ class MockDataFactory: Use in tests: -```python +```python notest from tests.fixtures import MockDataFactory def test_message_handling(): @@ -1133,7 +1133,7 @@ def test_message_handling(): Use markers to categorize tests: -```python +```python notest @pytest.mark.requires_api async def test_real_api_call(): """Skipped in CI, requires real API credentials.""" @@ -1188,7 +1188,7 @@ dev = ["pytest", "pytest-asyncio", "ruff", "pyrefly"] Adapters should import framework dependencies lazily to avoid import errors when the optional dependency is not installed. -```python +```python notest # Good - lazy import inside the adapter class LangGraphAdapter: def __init__(self): @@ -1212,7 +1212,7 @@ class LangGraphAdapter: Skip tests when optional dependencies are missing: -```python +```python notest import pytest try: @@ -1229,7 +1229,7 @@ def test_langgraph_adapter(): Or use a marker: -```python +```python notest requires_langgraph = pytest.mark.skipif( not HAS_LANGGRAPH, reason="langgraph not installed" From fd2efc02dc10f652a74a1be314f33605c3d0c4a5 Mon Sep 17 00:00:00 2001 From: Amit Gazal <amit.gazal@thenvoi.com> Date: Thu, 23 Jul 2026 09:15:53 +0300 Subject: [PATCH 5/8] docs: fix inlined-rules drift and skill Linear tool reference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - AGENTS.md: align inlined Engineering Rules with the repo — Python 3.11+ (not 3.10+), and reframe the ruff config as a generic example (repo has no [tool.ruff] block) with py311 and the real package name. - dependabot-triage skill: reference the actual Linear MCP save_issue tool instead of a nonexistent create_issue. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --- .claude/skills/dependabot-triage/SKILL.md | 5 +++-- AGENTS.md | 13 +++++++++---- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/.claude/skills/dependabot-triage/SKILL.md b/.claude/skills/dependabot-triage/SKILL.md index 891f344a1..4ca8b2a1e 100644 --- a/.claude/skills/dependabot-triage/SKILL.md +++ b/.claude/skills/dependabot-triage/SKILL.md @@ -88,8 +88,9 @@ don't fold it into the lock branch. Create one issue in the Integrations team for this triage run before touching git, so the batch PR and any follow-ups link back to it. Record the surveyed -queue (the table from Step 1) in the description. Use the Linear MCP tools -(`create_issue` / `save_issue`). Keep the whole run under this issue. +queue (the table from Step 1) in the description. Use the Linear MCP +`save_issue` tool (it both creates and updates). Keep the whole run under this +issue. Set the tracking issue (and each Step 8 sub-issue) to **Todo**, **assign it to the triager**, and put it in the **current cycle** — resolve "current" via diff --git a/AGENTS.md b/AGENTS.md index 3ba07b490..4ee0d5e8c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -743,7 +743,7 @@ logger.error("API call failed: %s", error, extra={"endpoint": url, "status": sta #### Python Version -- Target Python 3.10+ (match statements are OK) +- Target Python 3.11+ (match statements are OK) - Use modern syntax: `list[str]` instead of `List[str]` - Use `|` for union types: `str | None` instead of `Optional[str]` @@ -915,13 +915,18 @@ uv run ruff check . --fix - `UP` - pyupgrade (modern Python syntax) - `B` - flake8-bugbear (common bugs) -##### Typical Ruff Configuration +##### Ruff Configuration + +This repo has **no `[tool.ruff]` block** in `pyproject.toml` — it relies on +ruff's defaults. The following is a generic example of the shape such a block +takes, not this repo's config; don't add it unless you intend to override the +defaults. ```toml # pyproject.toml [tool.ruff] line-length = 88 -target-version = "py310" +target-version = "py311" [tool.ruff.lint] select = ["E", "F", "I", "UP", "B"] @@ -930,7 +935,7 @@ ignore = [ ] [tool.ruff.lint.isort] -known-first-party = ["thenvoi"] # Replace with your package name +known-first-party = ["band"] ``` #### Formatting with Ruff From b2d90bb1563ca21be9636aa16b25aa1296a760c0 Mon Sep 17 00:00:00 2001 From: Amit Gazal <amit.gazal@thenvoi.com> Date: Thu, 23 Jul 2026 09:38:26 +0300 Subject: [PATCH 6/8] docs: trim duplicative inlined Engineering Rules from AGENTS.md Address PR review nits: the inlined generic claude-config rules overlapped or contradicted the file's existing repo-specific sections. Drop the generic Coding Standards (dup of ## Coding Standards, incl. the stale Context7 line), Code Quality (dup of ## Commands), Testing Conventions (its tests/ tree wrongly showed tests/unit/), and Optional Dependencies (used a bogus thenvoi/mypackage name) subsections. Keep only what the sections above lack: Error Handling, Git Workflow, and the GitHub PR inline-comment recipe. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --- AGENTS.md | 474 +----------------------------------------------------- 1 file changed, 1 insertion(+), 473 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 4ee0d5e8c..ac91ecea8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -656,109 +656,7 @@ uv run pytest tests/ --ignore=tests/integration/ --ignore=tests/e2e/ -v ## Engineering Rules -_Inlined from the former shared `claude-config` submodule (`.claude/rules/`), now removed. These apply repo-wide. (The old `06-claude-config-management` rule is intentionally dropped — it described managing that submodule.)_ - -### Coding Standards - -#### Type Hints - -- Always use type hints for function parameters and return types -- Use `from __future__ import annotations` as the first import in every file -- Use `TypeVar` and `Generic` for typed generic classes -- Use `Protocol` classes for interfaces (not ABC) - -#### Logging - -##### Core Rule - -- **NEVER use `print()` statements** - always use logging instead -- When encountering `print()` in existing code, replace it with appropriate logging - -##### Logger Setup - -Every module should have a module-level logger: - -```python notest -import logging - -logger = logging.getLogger(__name__) -``` - -##### Log Levels - -Use the appropriate level for each message: - -| Level | Use For | -|-------|---------| -| `logger.debug()` | Detailed diagnostic info, variable values, flow tracing | -| `logger.info()` | Normal operations, startup messages, successful completions | -| `logger.warning()` | Unexpected but handled situations, deprecations | -| `logger.error()` | Failures that prevented an operation from completing | -| `logger.exception()` | Errors with full traceback (use inside except blocks) | - -##### Converting Print to Logging - -```python notest -# Bad -print(f"Processing {item}") -print(f"Error: {e}") - -# Good -logger.info("Processing %s", item) -logger.error("Failed to process: %s", e) -``` - -##### String Formatting - -Use `%s` placeholders instead of f-strings for log messages: - -```python notest -# Preferred - lazy evaluation, better performance -logger.debug("User %s performed action %s", user_id, action) - -# Acceptable but less efficient -logger.debug(f"User {user_id} performed action {action}") -``` - -##### Structured Context - -Include relevant context in log messages: - -```python notest -logger.info("Request completed", extra={"user_id": user_id, "duration_ms": duration}) -logger.error("API call failed: %s", error, extra={"endpoint": url, "status": status_code}) -``` - -#### Imports - -- Use absolute imports from the package root -- Sort imports with isort (configured via ruff) -- Group imports: stdlib, third-party, local - -#### Data Models - -- Use Pydantic v2 for data models and validation -- Define models with proper field types and validators -- Use `model_dump()` instead of deprecated `dict()` - -#### Python Version - -- Target Python 3.11+ (match statements are OK) -- Use modern syntax: `list[str]` instead of `List[str]` -- Use `|` for union types: `str | None` instead of `Optional[str]` - -#### Async/Await - -- Use async/await everywhere in async codebases -- No sync code in async adapters or handlers -- Use `asyncio.gather()` for concurrent operations -- Use `AsyncMock` for testing async methods - -#### Documentation - -- Use Context7 MCP to fetch up-to-date documentation when needed -- Follow existing patterns in the codebase for new code - +_Inlined from the former shared `claude-config` submodule (`.claude/rules/`), now removed. Only the rules the sections above don't already cover are kept — error handling, git workflow, and the GitHub PR inline-comment recipe; the generic coding-standards, code-quality, testing, and packaging rules were dropped as duplicative of (or contradicting) the repo-specific sections above. (The old `06-claude-config-management` rule is intentionally dropped — it described managing that submodule.)_ ### Error Handling @@ -893,376 +791,6 @@ Examples: - Squash commits when merging if history is messy -### Code Quality - -#### Linting with Ruff - -Ruff is used for linting and import sorting. - -```bash -# Check for linting issues -uv run ruff check . - -# Auto-fix issues -uv run ruff check . --fix -``` - -##### Common Ruff Rules - -- `E` - pycodestyle errors -- `F` - Pyflakes -- `I` - isort (import sorting) -- `UP` - pyupgrade (modern Python syntax) -- `B` - flake8-bugbear (common bugs) - -##### Ruff Configuration - -This repo has **no `[tool.ruff]` block** in `pyproject.toml` — it relies on -ruff's defaults. The following is a generic example of the shape such a block -takes, not this repo's config; don't add it unless you intend to override the -defaults. - -```toml -# pyproject.toml -[tool.ruff] -line-length = 88 -target-version = "py311" - -[tool.ruff.lint] -select = ["E", "F", "I", "UP", "B"] -ignore = [ - "E501", # Line too long (handled by formatter) -] - -[tool.ruff.lint.isort] -known-first-party = ["band"] -``` - -#### Formatting with Ruff - -```bash -# Format code -uv run ruff format . - -# Check formatting without changes -uv run ruff format . --check -``` - -##### Formatting Standards - -- Line length: 88 characters (Black default) -- Use double quotes for strings -- Trailing commas in multi-line structures - -#### Type Checking with Pyrefly - -Pyrefly is used for type checking (not mypy). - -```bash -# Run type checker -uv run pyrefly check -``` - -##### Type Checking Notes - -- Fix type errors before committing -- Use `# type: ignore` sparingly with explanation -- Prefer proper typing over ignoring errors - -##### When to Use `# type: ignore` - -Use type ignores only when: -- Third-party library has incomplete type stubs -- Dynamic behavior that can't be expressed in types -- Workaround for known type checker limitations - -Always include a comment explaining why: - -```python notest -# type: ignore[arg-type] # langgraph returns untyped dict -result = some_dynamic_call() - -# type: ignore[assignment] # Pydantic model validates at runtime -self.value = untrusted_input -``` - -##### Known Typing Limitations - -- Some async patterns may need explicit type annotations -- Generic callbacks across frameworks may need `Any` -- Dynamic tool registration may require runtime validation - -#### Package Management with uv - -After cloning a repository, always install dev dependencies: - -```bash -# Install with dev dependencies (required for development) -uv sync --extra dev -``` - -Other common commands: - -```bash -# Install production dependencies only -uv sync - -# Add a dependency -uv add <package> - -# Add optional dependency to a group -uv add --optional <group> <package> -``` - -#### Pre-Commit Workflow - -Run these before every commit: - -```bash -uv run ruff check . -uv run ruff format . -uv run pyrefly check -uv run pytest tests/ --ignore=tests/integration/ -v -``` - - -### Testing Conventions - -#### Test Framework - -- Use pytest as the test framework -- Use pytest-asyncio for async tests - -#### Running Tests - -```bash -# Run unit tests -uv run pytest tests/ --ignore=tests/integration/ -v - -# Run single test by name -uv run pytest tests/ -k "test_name" - -# Run with coverage -uv run pytest tests/ --ignore=tests/integration/ --cov=src/<package> - -# Run integration tests (requires API credentials) -uv run pytest tests/integration/ -v -s --no-cov -``` - -#### Async Tests - -- Use `@pytest.mark.asyncio` decorator for async tests -- Use `AsyncMock` for mocking async methods -- Use `MagicMock` for sync methods - -```python notest -import pytest -from unittest.mock import AsyncMock, MagicMock - -@pytest.mark.asyncio -async def test_async_function(): - mock_client = AsyncMock() - mock_client.fetch.return_value = {"data": "value"} - - result = await some_async_function(mock_client) - - assert result == expected - mock_client.fetch.assert_called_once() -``` - -#### Test Organization - -``` -tests/ -├── conftest.py # Shared fixtures -├── fixtures.py # Test data factories -├── unit/ # Unit tests (mocked dependencies) -├── integration/ # Real API tests (skipped in CI) -└── e2e/ # End-to-end tests -``` - -#### Fixtures - -- Define shared fixtures in `conftest.py` -- Use `MockDataFactory` or similar for creating test data -- Scope fixtures appropriately (function, class, module, session) - -```python notest -# conftest.py -import pytest -from unittest.mock import AsyncMock - -@pytest.fixture -def mock_client(): - return AsyncMock() - -@pytest.fixture -def sample_message(): - return {"role": "user", "content": "Hello"} -``` - -#### Test Data Factory Pattern - -Create a factory class for generating consistent test data: - -```python notest -# tests/fixtures.py -from dataclasses import dataclass - -@dataclass -class MockDataFactory: - @staticmethod - def make_message(role: str = "user", content: str = "test"): - return {"role": role, "content": content} - - @staticmethod - def make_tool_call(name: str = "test_tool", args: dict | None = None): - return {"name": name, "arguments": args or {}} - - @staticmethod - def make_event(event_type: str = "message", **kwargs): - return {"type": event_type, **kwargs} -``` - -Use in tests: - -```python notest -from tests.fixtures import MockDataFactory - -def test_message_handling(): - message = MockDataFactory.make_message(role="assistant", content="Hello") - # test code -``` - -#### Skip Markers - -Use markers to categorize tests: - -```python notest -@pytest.mark.requires_api -async def test_real_api_call(): - """Skipped in CI, requires real API credentials.""" - pass - -@pytest.mark.slow -def test_performance(): - """Skipped by default, run with --slow flag.""" - pass -``` - -Configure in `pyproject.toml`: -```toml -[tool.pytest.ini_options] -markers = [ - "requires_api: marks tests as requiring real API (skipped in CI)", - "slow: marks tests as slow (skipped by default)", -] -``` - -#### Test Performance - -- Override slow defaults for faster tests (e.g., `_max_iterations = 3`) -- Use `pytest-xdist` for parallel test execution when needed -- Keep unit tests fast (< 100ms each) - -#### Integration Tests - -- Store test credentials in `.env.test` -- Never commit real credentials -- Skip integration tests in CI by default -- Use separate test accounts/environments - - -### Optional Dependencies Pattern - -#### Overview - -Use optional dependencies to keep the core package lightweight while supporting multiple frameworks. - -#### pyproject.toml Configuration - -```toml -[project.optional-dependencies] -langgraph = ["langgraph", "langchain-openai"] -anthropic = ["anthropic"] -crewai = ["crewai"] -dev = ["pytest", "pytest-asyncio", "ruff", "pyrefly"] -``` - -#### Lazy Imports in Adapters - -Adapters should import framework dependencies lazily to avoid import errors when the optional dependency is not installed. - -```python notest -# Good - lazy import inside the adapter -class LangGraphAdapter: - def __init__(self): - try: - from langgraph.graph import StateGraph - except ImportError: - raise ImportError( - "langgraph is required for LangGraphAdapter. " - "Install with: pip install thenvoi[langgraph]" - ) - self.graph = StateGraph() - -# Bad - top-level import -from langgraph.graph import StateGraph # Fails if langgraph not installed - -class LangGraphAdapter: - pass -``` - -#### Testing with Optional Dependencies - -Skip tests when optional dependencies are missing: - -```python notest -import pytest - -try: - import langgraph - HAS_LANGGRAPH = True -except ImportError: - HAS_LANGGRAPH = False - -@pytest.mark.skipif(not HAS_LANGGRAPH, reason="langgraph not installed") -def test_langgraph_adapter(): - from mypackage.adapters.langgraph import LangGraphAdapter - # test code -``` - -Or use a marker: - -```python notest -requires_langgraph = pytest.mark.skipif( - not HAS_LANGGRAPH, - reason="langgraph not installed" -) - -@requires_langgraph -def test_langgraph_feature(): - pass -``` - -#### Installation Commands - -```bash -# Install core only -uv add thenvoi - -# Install with specific adapter -uv add thenvoi[langgraph] -uv add thenvoi[anthropic] - -# Install with multiple adapters -uv add thenvoi[langgraph,anthropic] - -# Install with dev dependencies -uv add thenvoi[dev] -``` - - ### GitHub PR Inline Comments #### Adding Inline Review Comments From 6a9a2727a9d5cd05b2169dc31c17eada9685442b Mon Sep 17 00:00:00 2001 From: Amit Gazal <amit.gazal@thenvoi.com> Date: Thu, 23 Jul 2026 09:43:23 +0300 Subject: [PATCH 7/8] docs: drop changelog note from Engineering Rules heading MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The provenance note narrated what was removed and why, which is PR/git history — not something a reader of the rules needs. Comments should describe the content as it is. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --- AGENTS.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index ac91ecea8..98b80893c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -656,8 +656,6 @@ uv run pytest tests/ --ignore=tests/integration/ --ignore=tests/e2e/ -v ## Engineering Rules -_Inlined from the former shared `claude-config` submodule (`.claude/rules/`), now removed. Only the rules the sections above don't already cover are kept — error handling, git workflow, and the GitHub PR inline-comment recipe; the generic coding-standards, code-quality, testing, and packaging rules were dropped as duplicative of (or contradicting) the repo-specific sections above. (The old `06-claude-config-management` rule is intentionally dropped — it described managing that submodule.)_ - ### Error Handling #### Pydantic ValidationError From 25c13286ac472cb5bc78d64c1049b32e5fbe03e3 Mon Sep 17 00:00:00 2001 From: Amit Gazal <amit.gazal@thenvoi.com> Date: Thu, 23 Jul 2026 09:45:21 +0300 Subject: [PATCH 8/8] docs: drop Engineering Rules wrapper heading, promote subsections Error Handling, Git Workflow, and GitHub PR Inline Comments now stand as top-level sections instead of nesting under an Engineering Rules wrapper. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --- AGENTS.md | 40 +++++++++++++++++++--------------------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 98b80893c..d4d572d4e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -654,11 +654,9 @@ uv run pytest tests/ --ignore=tests/integration/ --ignore=tests/e2e/ -v ``` -## Engineering Rules +## Error Handling -### Error Handling - -#### Pydantic ValidationError +### Pydantic ValidationError - Catch `pydantic.ValidationError` separately from generic `Exception` - Format validation errors for LLM readability: `"Invalid arguments for tool_name: field: message"` @@ -682,19 +680,19 @@ except Exception as e: raise ``` -#### Exception Hierarchy +### Exception Hierarchy - Use specific exceptions over generic ones - Create custom exception classes for domain-specific errors - Always include context in exception messages -#### Error Messages +### Error Messages - Make error messages actionable and clear - Include relevant context (what failed, why, what to do) - Avoid exposing internal implementation details to end users -#### Required Configuration +### Required Configuration - Use `raise ValueError(...)` for missing required configuration - Do NOT use `logger.error()` + `sys.exit()` pattern @@ -713,9 +711,9 @@ if not api_key: ``` -### Git Workflow +## Git Workflow -#### Branch Naming +### Branch Naming Branch names should match the Linear issue: @@ -730,7 +728,7 @@ Prefixes: - `docs/` - Documentation changes - `chore/` - Maintenance tasks -##### Creating Branches from Linear Issues +#### Creating Branches from Linear Issues Use `git lb` to create properly named branches from Linear issues: @@ -742,7 +740,7 @@ This automatically fetches the issue title from Linear and creates a branch with If `git lb` is not installed, ask the developer for the proper branch name. -#### Commit Messages +### Commit Messages Follow conventional commits format for all commits: @@ -762,7 +760,7 @@ Types: - `test:` - Adding or updating tests - `chore:` - Maintenance tasks -#### Pull Request Titles +### Pull Request Titles PR titles MUST use conventional commits format: @@ -775,23 +773,23 @@ Examples: - `fix: Handle validation errors in execute_tool_call` - `docs: Update README with new adapter examples` -#### Pre-Commit Checklist +### Pre-Commit Checklist - Run tests before committing - Run linting and formatting - Ensure type checking passes - Review changes with `git diff` -#### Code Review +### Code Review - Keep PRs focused and reasonably sized - Respond to review comments promptly - Squash commits when merging if history is messy -### GitHub PR Inline Comments +## GitHub PR Inline Comments -#### Adding Inline Review Comments +### Adding Inline Review Comments To add inline comments at specific lines in a PR, use the GitHub Reviews API with `gh api`: @@ -812,7 +810,7 @@ cat << 'EOF' | gh api repos/{owner}/{repo}/pulls/{pr_number}/reviews --method PO EOF ``` -#### Getting the Correct Line Numbers +### Getting the Correct Line Numbers **Important:** Line numbers must be from the NEW version of the file, not diff line numbers. @@ -833,14 +831,14 @@ EOF ``` Note: These are diff line numbers, not file line numbers. Use the actual file method above for accuracy. -#### Common Mistakes to Avoid +### Common Mistakes to Avoid - **Don't use `gh pr review --comment`** - This adds a general comment, not inline comments - **Don't use diff line numbers** - Use actual file line numbers from the new version - **Don't use `-f` flag for JSON arrays** - Pass JSON via stdin with `--input -` - **Don't guess line numbers** - Always verify by checking the actual file content -#### Example: Full Workflow +### Example: Full Workflow ```bash # 1. Get commit SHA @@ -866,7 +864,7 @@ cat << 'EOF' | gh api repos/owner/repo/pulls/83/reviews --method POST --input - EOF ``` -#### Multiple Comments +### Multiple Comments Add multiple inline comments in a single review: @@ -897,7 +895,7 @@ cat << 'EOF' | gh api repos/owner/repo/pulls/83/reviews --method POST --input - EOF ``` -#### Review Events +### Review Events The `event` field can be: - `"COMMENT"` - Submit general feedback without approval