diff --git a/.claude/commands/create-issue.md b/.claude/commands/create-issue.md index 30502c3..7a60c41 100644 --- a/.claude/commands/create-issue.md +++ b/.claude/commands/create-issue.md @@ -1,94 +1,99 @@ +--- +description: Create a GitHub issue from the repo's templates, with the right type and labels. +argument-hint: '[what the issue is about]' +--- + Create a GitHub issue for the current repository based on the user's input. ## Instructions -1. **Determine Issue Type** - Based on the user's description, determine which type to use: +1. **Check you're in the right repo first** - This is a **generated** SDK. `robosystems_client/api/` and `robosystems_client/models/` come from the RoboSystems API's OpenAPI document via `openapi-python-client`, and `robosystems_client/graphql/generated/` comes from `ariadne-codegen` over the checked-in `schema.graphql` plus the operation documents. A large share of apparent SDK bugs are really API bugs. Before filing here, work out which: + - A wrong or missing **type, field, or endpoint** almost always originates in the API's OpenAPI schema — file it in `RoboFinSystems/robosystems`. Regenerating here would only reproduce the same output. + - A bug in the **generation pipeline** — `bin/generate-sdk.sh` and its post-generation patches, `bin/generate-graphql.sh`, `bin/refresh-schema.py`, the `[tool.ariadne-codegen]` config in `pyproject.toml` — belongs here. + - A bug in **hand-written surface** — the typed facades under `robosystems_client/clients/`, `client.py`, `errors.py`, auth and token handling, the dataframe helpers — belongs here. + - A stale **GraphQL schema snapshot** is its own class: `schema.graphql` is checked in, so a drifted snapshot is a repo problem even though the schema itself lives in the API. + + When it's ambiguous, say which layer you think it is and why; a misfiled SDK issue costs a full round trip. + +2. **Determine Issue Type** - Based on the user's description, pick one: - **Bug**: Defects or unexpected behavior - **Task**: Specific, bounded work items that can be completed in one PR - **Feature**: Request a new capability (no design required) - - **RFC**: Propose a design for discussion before implementation (if available) - - **Spec**: Approved implementation plan ready for execution (if available) + - **RFC**: Propose a design for discussion before implementation + - **Spec**: Approved implementation plan ready for execution - Note: Not all repos have RFC/Spec templates. Check `.github/ISSUE_TEMPLATE/` first. + Confirm what this repo actually offers before assuming — `ls .github/ISSUE_TEMPLATE/` for the templates and `gh issue create --help` for whether `--type` is supported. -2. **Gather Context** - If the user provides a file path or references existing code: +3. **Gather Context** - If the user provides a file path or references existing code: - Read the relevant files to understand the current implementation - - Check related configuration files + - Check whether the file is generated (`robosystems_client/api/`, `robosystems_client/models/`, `robosystems_client/graphql/generated/`) before proposing a fix in it - Review any referenced documentation -3. **Draft the Issue** - Use the YAML templates in `.github/ISSUE_TEMPLATE/`: - - `bug.yml` - Include reproduction steps, impact, environment - - `task.yml` - Be specific about scope and acceptance criteria - - `feature.yml` - Capture the need and why it matters - - `spec.yml` - Fill in all sections with technical detail (if available) - - `rfc.yml` - Comprehensive design with alternatives considered (if available) - -4. **Sanitize for Public Visibility** - Before creating: - - Remove any internal pricing, margins, or cost details - - Remove specific customer names or data - - Generalize any sensitive business metrics - - Keep technical implementation details (these are fine to share) - -5. **Create the Issue** - Use `gh issue create` with: - - Clear, concise title (no prefixes like [SPEC] - types handle categorization) - - Well-formatted markdown body matching the template structure - - Appropriate metadata labels (see below) - -6. **Set Issue Type** - After creation, set the issue type via GraphQL: - ```bash - # Get repo info from current directory - REPO=$(gh repo view --json nameWithOwner -q .nameWithOwner) - OWNER=$(echo $REPO | cut -d'/' -f1) - NAME=$(echo $REPO | cut -d'/' -f2) +4. **Draft the Issue** - Read the matching YAML template in `.github/ISSUE_TEMPLATE/` and mirror its structure. Each template declares its own `type:` in frontmatter and marks which fields are required — read the file rather than guessing the sections. Fill the optional fields too where you have the information; they're the ones that make an issue actionable later. + + Note `gh issue create --title/--body` **bypasses templates entirely** — nothing prefills and nothing validates. That's exactly why the body has to be hand-matched to the template structure. + + For an SDK bug, the reproduction needs what a consumer report usually omits: the **installed version** (`pip show robosystems-client`), the **Python version** (the package supports 3.11–3.13), a **minimal call** showing the arguments passed, and the **actual vs expected** result. Include tracebacks verbatim, and say whether the sync or async path was used — they're separate code paths and a bug in one often isn't in the other. - # Get issue ID - gh api graphql -f query="{ repository(owner: \"$OWNER\", name: \"$NAME\") { issue(number: NUMBER) { id } } }" +5. **Say whether it's a compatibility break** - This package is **post-1.0 and published on PyPI**, so external integrators pin against it. If the issue implies changing an existing signature, model field, or exported name, say so explicitly — that turns the fix into a **major** and forces coordination with the API and every consumer. Issues that quietly imply a break are the expensive ones. - # Get available issue types for this repo - gh api graphql -f query="{ repository(owner: \"$OWNER\", name: \"$NAME\") { issueTypes(first: 10) { nodes { id name } } } }" +6. **Sanitize for Public Visibility** - This repo is public and the issue is world-readable immediately. Before creating: + - Remove API keys and JWTs — SDK repro snippets carry credentials more often than any other kind of issue. Check pasted tracebacks and request/response dumps line by line; an httpx traceback can include headers. + - Remove customer names, graph IDs, and real financial payloads; reconstruct with dummy values. + - Remove internal pricing, margins, or cost details. + - For anything security-adjacent, keep the text terse and non-actionable — no exploit mechanics, no endpoint enumerations, no payloads. For coordinated disclosure use a private GitHub Security Advisory, never a public issue. + - Keep ordinary technical implementation details (these are fine to share) - # Set type (use correct type ID from above) - gh api graphql -f query='mutation { updateIssue(input: { id: "ISSUE_ID", issueTypeId: "TYPE_ID" }) { issue { number } } }' +7. **Create the Issue** - One command, with the type set inline: + + ```bash + gh issue create \ + --type \ + --title "" \ + --body-file /tmp/issue-body.md \ + --label "" ``` + No prefixes like `[SPEC]` in the title — the type handles categorization. Write the body to a file rather than inlining it, to avoid shell-escaping problems. + + To change the type on an **existing** issue: `gh issue edit --type ` (or `--remove-type`). + ## Labels -Issue types handle primary categorization. Use labels for metadata (varies by repo): +Issue types handle primary categorization; labels carry the metadata. Always enumerate what actually exists rather than working from memory — and raise the limit, since the default truncates at 30: + +```bash +gh label list --limit 100 +``` -**Priority** (when to do it): -- `priority:critical` - Drop everything -- `priority:high` - Next up -- `priority:low` - Backlog +The families to expect in this repo: -**Size** (how long): -- `size:small` - < 1 day -- `size:medium` - 1-3 days -- `size:large` - > 3 days +- **`area:*`** — the primary routing dimension: `client` (the hand-written facades), `api` (the generated endpoint surface), `types`, `auth`, `errors`, `packaging` (build, wheel contents, dependency pins), `docs`, `testing`, `ci-cd`. **Always apply one.** +- **`priority:*`** — when to do it. Note the ladder is `critical` / `high` / `low` — there is **no `priority:medium`**. +- **`size:*`** — rough effort: `small` (< 1 day), `medium` (1–3 days), `large` (> 3 days). +- **Status** — `blocked`, `needs-review`. -**Status**: -- `blocked` - Waiting on something -- `needs-review` - Ready for review +## Questions vs issues -Check `gh label list` for available labels in the current repo. +`.github/ISSUE_TEMPLATE/config.yml` disables blank issues and routes open-ended questions to the org's GitHub Discussions. `gh issue create` bypasses that chooser entirely, so apply the intent yourself: if the user's input is a question ("how do I authenticate?") rather than actionable work, say so and suggest a Discussion instead of filing it. ## Example Usage -User: "We need to add export functionality" +User: "The graph query response model is missing the row count field" -Response: I'll create a feature issue for export functionality. Let me first understand the current state... +Response: Let me check whether that model is generated... -[Read relevant files to understand current implementation] -[Draft issue matching the template structure] -[Create issue with gh issue create] -[Set issue type via GraphQL] -[Add appropriate labels] +[Read the model — if it's under `robosystems_client/models/`, it comes from the API's OpenAPI schema, so the fix belongs in robosystems unless the post-generation patches are dropping it] +[Read bug.yml and draft a body matching its structure, with version, Python version, minimal call, and the traceback] +[Create with `gh issue create --type Bug --label area:api,size:small`] ## Output Format After creating the issue, provide: + 1. The issue URL 2. Brief summary of what was created 3. Issue type and labels applied -4. Any suggested follow-up tasks or related issues to create +4. Whether the fix implies a semver break, and any companion issue that should be filed against the API $ARGUMENTS diff --git a/.claude/commands/create-pr.md b/.claude/commands/create-pr.md index 12d3d03..7d21c0e 100644 --- a/.claude/commands/create-pr.md +++ b/.claude/commands/create-pr.md @@ -1,10 +1,17 @@ +--- +description: Open a pull request for the current branch, writing the description from the work actually done. +argument-hint: '[target-branch] [review]' +--- + Create a GitHub pull request for the current branch, writing the title and description from the actual work done in this session — not reconstructed from the diff. ## Why this command exists The previous flow outsourced PR-description authoring to a GitHub Action that only saw the diff and commit messages. It could not know _why_ the changes were made, so it frequently described things that weren't true. Those inaccurate descriptions then fed `@claude` reviews, compounding the bad information. This command fixes that at the root: **you author the description here, where the full context of what was done and why is available.** -This is the RoboSystems Python client — a type-safe, async-ready Python SDK generated against the RoboSystems API. Tooling is driven by `just`, not npm. +This is `robosystems-client` — a **published, post-1.0 Python SDK**, largely generated from the RoboSystems API's OpenAPI document and GraphQL schema. Its description is read by integrators deciding whether an upgrade is safe, so precision about the public surface matters more here than in an application repo. + +**This repository is public.** The PR title and body are world-readable the moment they're pushed — and, because publishing is triggered by a push to `release/**` rather than by a merge, the text is often public well before the version that carries it. Treat the description as a publication. ## Instructions @@ -18,11 +25,13 @@ CURRENT=$(git branch --show-current) TARGET=${1:-main} # override target via the first argument ``` -- **Never PR from the default branch.** If `CURRENT` is `main` (or `master`/`staging`), stop and tell the user to switch to a feature branch first. +- **Never PR from the default branch.** If `CURRENT` is `main` (or `master`/`staging`), stop and tell the user to switch to a feature branch first. New branches are created via `just create-feature `, not by hand. +- **Never target a release branch.** `release/**` is what `publish.yml` watches; a PR into one is a publish trigger, not a code review. Target `main`. - **Source ≠ target.** If `CURRENT == TARGET`, stop. -- **Uncommitted changes.** Run `git status --porcelain`. If there are uncommitted/staged changes, surface them and ask whether to commit them (respecting the repo's commit rules — never on `main`, no `git add -A`, stage files by name) or proceed without them. The PR description must reflect committed state. +- **Uncommitted changes.** Run `git status --porcelain`. If there are uncommitted/staged changes, surface them and ask whether to commit them (respecting the repo's commit rules — never on `main`, stage files by name, no `git add -A`) or proceed without them. The PR description must reflect committed state. - **Existing PR.** Check `gh pr list --head "$CURRENT" --base "$TARGET" --json url,number`. If a PR already exists, do **not** create a duplicate — offer to update its title/body with `gh pr edit` instead. -- **Push the branch.** `gh pr create` requires the branch on the remote. Ensure it's pushed: `git push -u origin "$CURRENT"` (the user invoking `/create-pr` is the explicit, in-the-moment request that authorizes pushing _this feature branch_ — this is the one push allowed without a separate ask; never push `main`). +- **Security fixes — check what's published.** A security-fix commit discloses the bug through its diff the moment it's pushed, and the vulnerable version stays installable from PyPI regardless. If this branch carries one, say which published versions are affected so the user can sequence a patch release with the disclosure. +- **Push the branch.** `gh pr create` requires the branch on the remote. Ensure it's pushed: `git push -u origin "$CURRENT"` (the user invoking `/create-pr` is the explicit, in-the-moment request that authorizes pushing _this feature branch_ — never push `main` or `release/*`). ### 2. Gather the real change context @@ -35,19 +44,33 @@ This is the whole point — ground the description in what actually happened: git diff --stat "$TARGET"..."$CURRENT" # files + churn git diff "$TARGET"..."$CURRENT" # full diff — read it, don't guess ``` -- **Hard rule — no confabulation.** Every claim in the description must be supported by the diff. If you didn't touch the auth client, don't write "auth improvements." If a behavior isn't in the diff, don't mention it. When the session context and the diff disagree, the diff wins and you investigate the discrepancy. -- **Generated code.** Much of `robosystems_client/` is generated from the OpenAPI spec via `just generate-sdk`. If a change is a regeneration, say so plainly rather than narrating individual model edits as if hand-written. +- **Separate generated churn from real change.** A regeneration touches `robosystems_client/api/`, `robosystems_client/models/`, and `robosystems_client/graphql/generated/` wholesale; the meaningful diff is usually a handful of lines inside it plus whatever was hand-written. Summarize the generated part ("regenerated against API build X; net effect: two new endpoints, one widened union") rather than enumerating it, and never describe generated churn as if it were authored work. +- **Hard rule — no confabulation.** Every claim must be supported by the diff. If you didn't change the public API surface, don't write "new client methods." When the session context and the diff disagree, the diff wins and you investigate the discrepancy. ### 3. Compose the PR -- **Type** — derive from the branch prefix (`feature/` → feat, `bugfix/`/`fix/` → fix, `hotfix/` → fix, `chore/` → chore, `refactor/` → refactor, `release/` → release). Default to `feat` if unprefixed. -- **Title** — concise (~50–72 chars), conventional-commit style, e.g. `feat(clients): add streaming query helper`. Match the style in `git log`. -- **Body** — markdown, only sections that apply: +- **Type** — derive from the branch prefix (`feature/` → feat, `bugfix/`/`fix/` → fix, `hotfix/` → fix, `chore/` → chore, `refactor/` → refactor). Default to `feat` if unprefixed. +- **Title** — concise (~50–72 chars), conventional-commit style with a scope, matching `git log` (e.g. `feat(ledger): add report bundle download support`, `chore(sdk): regenerate against report-bundle endpoint`). +- **Body** — markdown. **Match the headings in `.github/PULL_REQUEST_TEMPLATE.md`**, because `--body-file` bypasses template prefill entirely and a hand-written body silently drops whatever sections it omits: - **Summary** — 1–3 sentences: what this PR does and why. - - **Changes** — bullets grouped by area/file/module, describing real edits. - - **Testing** — state truthfully what was run. If `just test-all` (or a subset like `just test` / `just lint` / `just typecheck`) was run this session, say so and give the result. If nothing was run, say "Not run" — never claim passing tests that weren't executed. - - **Notes / Follow-ups** — optional: deferred items, risks, related issues, SDK-regeneration implications. -- **Attribution** — attribute to the user only. Do **not** add a "Generated with Claude Code" footer or a `Co-Authored-By: Claude` trailer (per `CLAUDE.local.md`). Include such a line only if the user explicitly asks. + - **Changes** — bullets grouped by area: regenerated SDK vs. hand-written extensions vs. tooling/packaging. + - **Compatibility** — the section that matters most here. See below. + - **Testing** — state truthfully what was run. The gate is `just test-all` (pytest → `ruff format` → `ruff check` → `basedpyright`); `just test`, `just lint`, `just format`, and `just typecheck` run standalone. SDK regeneration needs a reachable API (`just generate-sdk`), so it often isn't runnable in-session — if you couldn't, say so plainly. If nothing was run, say "Not run" — never claim passing tests that weren't executed. + + The template has no Related Issues section — put `Closes #123` / `Fixes #456` as the last line of the Summary. GitHub links it from anywhere in the body. + +- **Compatibility is a required judgment, not an optional section.** This package is post-1.0 with external integrators, so classify the change explicitly and say which it is: + - **Breaking** — a removed or renamed export, a changed signature or return type, a narrowed input type, or changed runtime semantics. This forces a **major** and has to be coordinated with the API and with every consuming app. Say it plainly in the body; don't bury it in a bullet. + - **Additive** — new endpoints, new optional fields, new exports. Free, but worth naming so consumers know what they gain. + - **Internal** — generation tooling, tests, packaging that doesn't alter emitted types. + + A regeneration is not automatically additive: an API schema change can narrow a type or drop a field, and that reaches consumers as a break even though no hand-written line changed. Diff the emitted types before classifying. + +- **Version and publish are not this PR's job.** `create-release.yml` bumps the version on `main` and cuts `release/`; the push to that branch is what triggers `publish.yml`. Never bump the version in `pyproject.toml` in a feature PR and never imply the PR publishes anything. + +- **Security-fix disclosure.** If the PR fixes a security issue, the prose is often _more_ actionable than the diff — keep it terse and non-actionable. Name the area hardened, never the mechanism. No exploit mechanics, attack scenarios, endpoint enumerations, or payloads. For coordinated disclosure use a private GitHub Security Advisory, never a public issue. + +- **Attribution** — attribute to the user only. Do **not** add a "🤖 Generated with Claude Code" footer or a `Co-Authored-By: Claude` trailer. Include such a line only if the user explicitly asks. ### 4. Create the PR @@ -71,7 +94,7 @@ Only if the user explicitly asks (e.g. passes `review` / `--review` in arguments gh pr comment --body "@claude please review this PR" ``` -Otherwise leave it off — the description is now accurate, and the user can run `/pr-review` locally (full context) or `@claude` manually when ready. Do not request review by default. +`claude.yml` only fires on an `@claude` mention from an `OWNER`/`MEMBER`/`COLLABORATOR`, so nothing happens automatically. Leave it off by default. ## Output @@ -80,7 +103,8 @@ After creating the PR, report: 1. The PR URL. 2. A one-line summary of the title. 3. Target ← source branches. -4. Whether a Claude review was requested. +4. The compatibility classification (breaking / additive / internal), and if breaking, what a consumer has to change. +5. Whether a Claude review was requested. ## Arguments diff --git a/.claude/commands/pr-review.md b/.claude/commands/pr-review.md index f922d02..198580f 100644 --- a/.claude/commands/pr-review.md +++ b/.claude/commands/pr-review.md @@ -1,3 +1,8 @@ +--- +description: Review a pull request — gather metadata, diff, and existing feedback, then give a verdict. +argument-hint: '[pr-number-or-url]' +--- + Review a pull request by gathering all PR metadata, diff, and review comments, then provide a comprehensive review summary. ## Instructions @@ -6,8 +11,8 @@ Review a pull request by gathering all PR metadata, diff, and review comments, t The user may provide a PR URL, number, or nothing: -- **URL provided** (e.g., `https://github.com/RoboFinSystems/robosystems/pull/577`): Extract the repo and PR number -- **Number provided** (e.g., `577`): Use the current repository +- **URL provided** (e.g., `https://github.com/RoboFinSystems/robosystems-python-client/pull/42`): Extract the repo and PR number +- **Number provided** (e.g., `42`): Use the current repository - **Nothing provided**: Detect from the current branch using `gh pr view --json number,url` — if no open PR exists for the current branch, ask the user which PR to review ### 2. Gather PR Data @@ -15,23 +20,23 @@ The user may provide a PR URL, number, or nothing: Run these `gh` commands to collect all context: ```bash -# PR metadata — use only valid fields -gh pr view --json title,body,author,state,labels,reviews,reviewRequests,statusCheckRollup,mergeStateStatus,headRefName,baseRefName,additions,deletions,changedFiles,createdAt,updatedAt +# PR metadata + conversation comments in one call +gh pr view --json number,url,title,body,author,state,isDraft,labels,comments,reviews,reviewDecision,latestReviews,reviewRequests,statusCheckRollup,mergeStateStatus,headRefName,headRefOid,baseRefName,additions,deletions,changedFiles,files,closingIssuesReferences,createdAt,updatedAt # PR diff (the actual code changes) gh pr diff -# Inline review comments (gh api needs owner/repo — use gh repo view to get it) +# Inline review comments — no --json equivalent exists, so this call is still required gh api repos/$(gh repo view --json nameWithOwner -q .nameWithOwner)/pulls//comments --paginate - -# Top-level PR conversation comments -gh api repos/$(gh repo view --json nameWithOwner -q .nameWithOwner)/issues//comments --paginate ``` -**Important `gh pr view --json` field reference** (common mistakes to avoid): -- Use `reviews` not `reviewers` (reviewers is not a valid field) -- Use `reviewRequests` for pending review requests -- Use `headRefOid` for the HEAD commit SHA +**Field notes:** + +- `reviews` not `reviewers` — `reviewers` is not a valid field and errors. +- `reviewDecision` is the single field that answers "has this been approved." +- `comments` covers the top-level conversation, so no separate `issues//comments` call is needed. +- `files` is essential here: a regeneration PR is mostly generated churn, and per-file add/delete counts are how you find the few hand-written files worth reading closely. +- Keep `--paginate` **bare**. Adding `-q`/`--jq` makes gh emit one JSON document _per page_ instead of a merged array, and `--slurp` can't be combined with `--jq`. Pipe to `jq` after the call, not through it. ### 3. Categorize Review Feedback @@ -39,21 +44,34 @@ Organize all comments and checks into categories: - **Human Reviews**: Comments from human reviewers (approve, request changes, general feedback) - **AI Reviews**: Comments from Claude, Copilot, or other AI review bots -- **Code Quality**: Comments from linters, formatters, type checkers (e.g., CodeRabbit, SonarCloud, Codacy) -- **Security**: Findings from security scanners (e.g., Snyk, Dependabot, CodeQL, GitGuardian) -- **CI/CD**: Build status, test results, deployment checks +- **Code Quality**: Comments from linters, formatters, type checkers +- **Security**: Findings from security scanners (Dependabot, CodeQL) +- **CI/CD**: Build status, test results + +**How feedback actually arrives in this repo** — don't read the categories too literally: + +- Formal `reviews` and inline `pulls//comments` are typically **empty**, and `reviewDecision` is usually blank. That's the norm here, not a signal that review was skipped. Don't report "no review feedback" on the strength of an empty `reviews` array. +- **AI review is opt-in.** `claude.yml` only fires on an explicit `@claude` mention from an `OWNER`/`MEMBER`/`COLLABORATOR` — there is no automatic review on PR open. When it has run, the findings are a **bot comment in the conversation `comments`**, not a formal review. +- In `statusCheckRollup`, checks expose `.name` while legacy statuses expose `.context`, and a `conclusion` of `NEUTRAL` or `SKIPPED` is not a failure. Read the conclusion, don't pattern-match on non-`SUCCESS`. +- Note what CI does **not** cover: it cannot regenerate against a live API, so a stale SDK passes every check. Green CI means "this code is internally consistent," not "this matches the API." ### 4. Review the Diff With the full PR diff in hand, perform your own review focusing on: -- **Correctness**: Does the code do what the PR description says? -- **Patterns**: Does it follow existing codebase patterns (check CLAUDE.md)? -- **Security**: Any OWASP top 10 concerns? -- **Multi-tenancy**: Are graph operations scoped to `graph_id`? -- **Error handling**: Appropriate for the context? -- **Tests**: Are changes covered by tests? -- **Missing changes**: Any files that should have been updated but weren't? +- **Compatibility first.** This is a published post-1.0 package with external integrators. Does the diff remove or rename an export, change a signature or return type, narrow an input type, or change runtime semantics? That's a **major** and needs coordination with the API and every consuming app — an uncoordinated break is a blocking issue, not a note. Additive fields and new optional model fields are free. Check the emitted model and signature surface, not just the diff shape: a regeneration can turn an optional field required, or drop one, without a single hand-written line changing. +- **Generated vs. hand-written.** Is anything under `robosystems_client/api/`, `robosystems_client/models/`, or `robosystems_client/graphql/generated/` edited by hand? That's always wrong — the next `just generate-sdk` / `just generate-graphql` erases it. The fix belongs in `bin/generate-sdk.sh`'s post-generation patches, in the `[tool.ariadne-codegen]` config in `pyproject.toml`, or in the API's schema. Flag it as blocking. +- **Schema snapshot drift.** `robosystems_client/graphql/schema.graphql` is a checked-in snapshot that the GraphQL operation tests validate against. If operations changed without a `just refresh-schema`, or the snapshot moved without the operations being re-checked, say so — a stale snapshot makes those tests assert against a schema the API no longer serves. +- **Does the regeneration match a real API state?** A regeneration PR should say what API version or build it was generated against. Types that don't correspond to any deployed API are worse than stale ones. +- **Correctness**: does the code do what the PR description says? +- **Auth and secrets**: token handling, header construction, and anything that could log a credential. A `console.log` of a request object is a credential leak in a consumer's terminal. +- **Error handling**: are API errors mapped to something a consumer can branch on, or swallowed into a generic throw? +- **Exports**: is new surface actually exported from the package `__init__.py` files, and are the models importable from where the README says they are? Unexported surface is invisible to consumers regardless of how well it's written. +- **Sync and async parity**: the client exposes both paths. A fix or a new method applied to only one of them is a half-fix — check that the other got the same treatment and the same test. +- **Packaging**: changes to `pyproject.toml` — dependency pins, optional extras, `requires-python`, included packages, `py.typed` — affect what ships and who can install it. A dropped `py.typed` or a too-narrow `requires-python` breaks consumers in ways no test here catches. +- **Tests**: are changes covered? Read the test, don't trust that it's green — a test that asserts the buggy behavior passes just as happily as a correct one. +- **Disclosure hygiene** (this repo is public): does the PR _text_ over-disclose? A security-fix description should name the area hardened, never the mechanism. Note also that the vulnerable version stays installable from PyPI after the fix merges — flag whether a patch release is needed. +- **Missing changes**: a new endpoint without an export, a new option without a type, a behavior change without a README update. ### 5. Output Format @@ -63,10 +81,13 @@ Provide a structured review: ## PR Summary **Title**: ... **Author**: ... | **Branch**: ... → ... -**Status**: ... | **Changes**: +X / -Y across Z files +**Status**: ... | **Changes**: +X / -Y across Z files (generated: A / hand-written: B) +## Compatibility + + ## Existing Review Feedback ### Human Reviews @@ -101,9 +122,9 @@ Provide a structured review: ### Notes -- If the PR diff is very large (>2000 lines), focus on the most important files and note which files were skimmed +- For a large regeneration diff, use the `files` array to separate generated paths from hand-written ones and review the latter line by line; summarize the former by net effect on the public surface - For security findings, always err on the side of flagging — false positives are better than missed vulnerabilities -- Cross-reference the PR description with the actual diff to catch scope creep or missing implementation -- If the PR references an issue, check that the issue requirements are met +- Cross-reference the PR description with the actual diff to catch scope creep or an unstated break +- If the PR references an issue (`closingIssuesReferences`), check that the issue requirements are met -$ARGUMENTS \ No newline at end of file +$ARGUMENTS diff --git a/.claude/commands/publish.md b/.claude/commands/publish.md new file mode 100644 index 0000000..a7b147d --- /dev/null +++ b/.claude/commands/publish.md @@ -0,0 +1,73 @@ +--- +description: Monitor a release/publish run — diagnose failures, verify the package actually landed on PyPI. +argument-hint: '[run-id]' +--- + +Monitor a release and publish run — pinpoint why it failed, and confirm the version actually landed on PyPI. Releases go through GitHub Actions; this command is about watching and diagnosing them, not replacing the pipeline. + +## How a release actually happens here + +Two workflows, and the trigger between them is the part that surprises people: + +1. **`create-release.yml`** (`workflow_dispatch`, or `bin/create-release.sh`) — reads the current version from `pyproject.toml`, computes the next one from the requested bump, commits the bump **to `main`**, cuts `release/` from that commit, and tags it. +2. **`publish.yml`** — triggered by **a push to `release/**`**, not by a merge and not by the tag. It reads the version from `pyproject.toml`, builds the distribution, checks the PyPI JSON API for that version, and if it isn't there, publishes over OIDC trusted publishing. + +So: **merging a PR to `main` publishes nothing.** The release branch push is the publishing event. And because `publish.yml` short-circuits when the version already exists on PyPI, a re-run of a successful publish is a no-op rather than an error — useful, but it also means "the run went green" is not by itself proof that _this_ run published anything. Note the build step runs *before* the existence check, so a green build tells you nothing about whether a publish happened. + +`tag-release.yml` writes the GitHub release body separately; see `/release-notes` for the curated-notes override. + +## Scope & guardrails + +- **`gh` reads are free; triggering a release is not.** Reading runs, jobs, and logs (`gh run list/view/watch`) needs no confirmation. **Dispatching `create-release.yml`** is an outward-facing and effectively irreversible action — a PyPI version can be yanked but **never** re-uploaded, so a bad publish burns that version number permanently. Confirm the bump type and the ref with the user, and default to watching a run they already started. +- **Never bump the version in `pyproject.toml` by hand.** The workflow owns the bump; a hand-bump collides with it and can produce a version that's tagged but never published. +- **Never push `main` or `release/*`.** Those are the user's. The pre-push hook blocks them. +- **The user owns the decision to publish a major.** A major reaches every consuming app and every external integrator. If the change set implies one, say so and stop — don't dispatch. + +## 1. Find the run + +```bash +gh run list --workflow=publish.yml --limit 5 +gh run list --workflow=create-release.yml --limit 5 +gh run view +gh run watch # live, if it's in flight +``` + +## 2. Pinpoint the failure + +```bash +gh run view --log-failed +``` + +Classify by stage: + +- **`create-release.yml` — branch already exists.** The workflow checks for `release/` before creating it. A failure here usually means a previous run got partway, and the fix is to resolve the leftover branch, not to re-dispatch blindly. +- **`create-release.yml` — push to `main` rejected.** The version bump commits directly to a protected branch and needs `ACTIONS_TOKEN`; a permissions failure here looks like an auth error at the push step. +- **`publish.yml` — "version exists on PyPI".** Not a failure. The upload step is skipped by condition. Read it as "nothing to do," and if you expected a publish, the version wasn't bumped. +- **`publish.yml` — build.** `pip install build twine` then `python -m build`. A build failure here is a packaging problem — usually `pyproject.toml` metadata or a missing file — that the test suite does not cover, since `just test-all` never builds a distribution. +- **`publish.yml` — the upload.** OIDC trusted publishing. Failures are usually the PyPI-side trusted-publisher configuration (environment or workflow name mismatch) rather than anything in the code. + +## 3. Verify it actually landed + +A green workflow is not proof. Check PyPI directly: + +```bash +curl -s https://pypi.org/pypi/robosystems-client/json | jq -r '.info.version' # latest +curl -s https://pypi.org/pypi/robosystems-client/json | jq -r '.releases | keys[]' # history +``` + +Then confirm the published artifact is usable, since packaging problems don't fail the upload: + +```bash +pip download robosystems-client== --no-deps -d /tmp/verify # fetch the wheel +unzip -l /tmp/verify/robosystems_client--*.whl | head -30 # what actually ships +``` + +Check `py.typed` is present in that listing — its absence turns the SDK untyped for every consumer and nothing in CI catches it. + +If the version is a major, downstream consumers need coordinated adoption — say so rather than treating the publish as the end of the task. External integrators pin against this package, so a major is a support event, not just a release. + +## Output + +A short status: which workflow, what failed and at which step, the root cause, the re-run link if any, and the verified published version from PyPI. If nothing failed, say so — don't manufacture work. + +$ARGUMENTS diff --git a/.claude/commands/release-notes.md b/.claude/commands/release-notes.md new file mode 100644 index 0000000..e37543a --- /dev/null +++ b/.claude/commands/release-notes.md @@ -0,0 +1,65 @@ +--- +description: Draft curated release notes for an SDK release. +argument-hint: '[version]' +--- + +Draft curated release notes for an upcoming release, following the convention in `.github/release-notes/README.md`. + +## Why this command exists + +`tag-release.yml` generates release bodies from the changes since the last tag. That suits routine releases but reads poorly for a milestone, where the story is what the version _is_. For a published SDK it is worse than poor: post-1.0 the notes **are** the compatibility contract that integrators read before upgrading, and a generated changelog does not state what is additive, what is deprecated, or when a deprecation is removed. This command encodes the review and hygiene checks that keep the notes accurate and safe to publish. + +## Instructions + +### 1. Decide whether to curate at all + +Unlike the application repos, curation here is not optional across the board. Curated notes are **mandatory for a major, and for any minor that deprecates public surface** — those notes carry the contract. A minor that is purely additive and a plain patch can keep the generated changelog; skipping is a normal outcome in those cases, not a failure. If the user invoked this command for a patch, confirm they still want curated notes. + +### 2. Establish the version and the range + +- The target version comes from the argument (e.g. `/release-notes 1.2.0`). If none was given, ask what version the user intends to tag — the filename must match the eventual tag exactly, and a mismatched file is silently ignored. Derive it from the current `pyproject.toml` version plus the bump type the user will dispatch (`1.1.0` + `minor` → `1.2.0`). +- **Never bump the version yourself.** `create-release.yml` bumps `pyproject.toml` on `main` as its first step and derives the tag from the result — a hand-bump collides with it. +- **The version is a promise, so sanity-check the bump type against the diff.** If the range contains a removal, a rename, or a semantic change to existing surface, a minor is the wrong bump — stop and raise it before writing a line of prose. +- **The range depends on the release kind.** A major or a surface-deprecating minor covers the span since the previous release of that significance; an ordinary curated release covers the span since the last tag: + +```bash +LAST=$(git tag --sort=-creatordate | head -1) # ordinary: last tag +# major/minor: the previous major or minor tag, e.g. v1.0.0 when cutting v1.2.0 +git log "$RANGE_START"..origin/main --merges --format='%s' +gh pr list --state merged --limit 30 --json number,title,mergedAt +``` + +Note the generated links section will still compare against the last tag; the prose should state the span it covers explicitly. + +### 3. Review the changes for real + +Do not write notes from commit subjects alone. Read the PR bodies (`gh pr view `) and spot-check diffs where the description is thin. Classify everything into public-surface changes, fixes, and internals, then check specifically: + +- **The compatibility contract.** This is the load-bearing check. The public surface is the one defined in `CLAUDE.local.md` — the facades (`clients/*`), root exports, documented models, error classes, and auth config; generated internals are exempt unless they show up in a facade signature or doc. For every change to it, decide which bucket it is in and say so in the notes: **added** (free, rides a minor), **deprecated** (must name the replacement and the earliest removal major), **removed** (majors only, and only after a deprecation shipped at least one further minor and 90 days earlier), or **changed semantics** (a break, even when the signature is untouched). The integration template pins `>=1,<2`, so anything in the last two buckets breaks real consumers. +- **Regeneration vs. hand edits.** Most of `robosystems_client/` is generated by `just generate-sdk` from the API's OpenAPI spec, and the typed GraphQL models by ariadne-codegen from the checked-in `schema.graphql`. A regeneration that widens the surface is worth a sentence about what the API added — not a per-model enumeration. Say plainly when a release is a regeneration, and don't dress generated-internal churn up as new capability. +- **Upstream API coupling.** A regeneration tracks a specific RoboSystems API version. If the new surface only works against an API that isn't deployed yet, the notes must say so — integrators will call it the day they upgrade. +- **Runtime and dependency floors.** A raised Python floor or a dependency major is an upgrade blocker for someone. Note it. + +### 4. Security disclosure review + +This repo is public and the release publishes to PyPI in the same run, so the notes are world-readable immediately and are read by everyone upgrading. For any security-adjacent change: + +- Keep the line at PR-title neutrality: what area was hardened, never how or against what. +- No exploit mechanics, no affected-endpoint enumerations, no detection signatures or thresholds, no "previously protected only by X" tells. +- Never paste content from private analysis documents into the notes. +- Say clearly that upgrading is recommended, without describing the exposure. +- When in doubt, terser. + +### 5. Write the file + +Write `.github/release-notes/v.md` — **body only**: + +- No `# RoboSystems Python SDK v` heading, no release-statistics section, no links section, no generated-with footer. The workflow supplies all of those. Start at the first line of prose. +- `v1.0.0.md` is a good model for the format. +- Lead with one or two sentences saying what the version is. Then, for anything touching the public surface, a section per contract bucket — added / deprecated / removed / changed — ahead of the ordinary fixes and internals. Ground every line in a change you actually reviewed. + +### 6. Hand off — sequencing matters + +The file must exist **at the tagged ref**, and there is no window to add it late: `create-release.yml` bumps the version on `main`, cuts `release/` from the result, and tags it in the same run. Pushing that release branch is also what triggers `publish.yml`, so by the time the package is on PyPI the notes are already fixed. They have to be **merged into `main` before the workflow is dispatched**. + +Write the draft on a feature branch (created via `just create-feature`), never on `main`. Present it for review and leave the merge and the dispatch to the user. diff --git a/.claude/commands/staged-review.md b/.claude/commands/staged-review.md index 3556b69..b1d9823 100644 --- a/.claude/commands/staged-review.md +++ b/.claude/commands/staged-review.md @@ -1,42 +1,78 @@ -Review all staged changes (`git diff --cached`) with focus on these contexts: +--- +description: Review the staged diff against this SDK's compatibility, generation, and packaging rules. +--- -## Client Implementation Context +Review all staged changes (`git diff --cached`) with focus on the contexts below. Read the diff first — if nothing is staged, say so rather than reviewing the working tree. -**API Methods:** -- Are new methods properly typed with Pydantic models? -- Is error handling consistent? -- Are async/sync variants implemented correctly? +This is `robosystems-client`: a **published, post-1.0 Python SDK**, largely generated from the RoboSystems API's OpenAPI document and GraphQL schema, consumed by internal tooling and external integrators. It is a **public repository**. -**Type Definitions:** -- Are Pydantic models properly defined? -- Are types properly exported in `__init__.py`? -- Is backwards compatibility maintained? +## Before anything else: is this file generated? -**Code Quality:** -- Does the code follow existing patterns? -- Is the code properly formatted (ruff)? -- Are type hints complete (basedpyright)? +```bash +git diff --cached --name-only +``` -## Testing Context +`robosystems_client/api/`, `robosystems_client/models/`, and `robosystems_client/graphql/generated/` are **generation output**. A hand edit there is erased by the next `just generate-sdk` / `just generate-graphql` — that's a blocking finding regardless of how correct the edit is. The fix belongs in one of: -- Do new methods have corresponding tests? -- Are edge cases covered? -- Is test coverage maintained? +- `bin/generate-sdk.sh` — the post-generation patches applied to `openapi-python-client` output +- `[tool.ariadne-codegen]` in `pyproject.toml`, or the operation documents under `robosystems_client/graphql/operations/` — GraphQL generation inputs +- `robosystems_client/graphql/schema.graphql` — the checked-in schema snapshot, refreshed with `just refresh-schema` against a running backend +- the API's OpenAPI or GraphQL schema, in `RoboFinSystems/robosystems` — where wrong types actually originate -## Documentation Context +If the staged diff mixes regenerated output with hand-written change, say which files are which; that distinction drives the rest of the review. -- Is README updated for new features? -- Are docstrings complete? -- Are examples provided for new methods? +## Compatibility (the section that decides the verdict) -## Packaging Context +Post-1.0, the emitted type surface **is** the contract. For anything staged here: -- Is `pyproject.toml` updated if needed? -- Are dependencies properly specified? +- Is an export removed or renamed? A signature or return type changed? An input type narrowed? Runtime semantics altered? Each is a **major**, requires coordination with the API and every consuming app, and must be stated explicitly rather than discovered by an integrator. +- Is it additive — new endpoints, new optional fields, new exports? Free, but name it. +- **A regeneration is not automatically safe.** An API schema change can turn an optional field required, or drop one, so the diff reaches consumers as a break with no hand-written line involved. Compare the emitted model and signature surface, not the diff shape. +- Does new surface appear in the package `__init__.py` exports, importable from where the README says it is? Unexported surface may as well not exist. +- Is the change applied to **both** the sync and async paths? A fix landed on one is a half-fix. + +## SDK implementation + +- Are new methods fully typed, with no `Any` used to silence `basedpyright`? +- Do the typed facades under `robosystems_client/clients/` follow the existing patterns rather than inventing a second style? +- Is error handling consistent — are API errors mapped to something a consumer can branch on, not swallowed into a generic throw? +- Are request/response types the generated models rather than hand-redeclared shapes that will drift? + +## Auth and secrets + +- Token and header handling: is anything logged, stringified into an exception message, or attached where it could surface in a consumer's traceback? An httpx error that carries request headers is a credential leak downstream. +- Are credentials read from configuration rather than defaulted to anything real? +- No API keys, JWTs, real graph IDs, or customer payloads in tests, fixtures, or comments. Fixtures should be invented. + +## Packaging + +- Changes to `pyproject.toml` — dependency pins, optional extras, `requires-python`, included packages, `py.typed` — change **what ships and who can install it**. A dropped `py.typed` silently turns a typed SDK into an untyped one for every consumer; a narrowed `requires-python` locks out supported versions (3.11–3.13). +- Never stage a version bump in `pyproject.toml` in a feature branch: `create-release.yml` owns the bump on `main`, and pushing `release/**` is what triggers `publish.yml`. + +## Testing + +- Do new methods have tests, including the error paths? +- Do tests exercise the public surface as a consumer would import it, rather than reaching into internals? +- Is the test asserting correct behavior, or just asserting what the code currently does? + +## Documentation + +- Is the README updated for new or changed surface? For a published package this is the primary integrator documentation. +- Are JSDoc comments present on new public methods, and accurate on changed ones? They surface in consumers' editors. +- Does a breaking change come with the migration line an integrator needs? + +## Public-repo hygiene + +- No customer names, graph IDs, internal cost/pricing detail, or real financial payloads in code, comments, or fixtures. +- If the change fixes a security issue, keep commit messages and comments terse and non-actionable — the area hardened, never the mechanism. Remember the vulnerable version stays installable from PyPI until a patch is published. ## Output Provide a summary with: -1. **Issues**: Problems that should be fixed before commit -2. **Suggestions**: Improvements that aren't blocking -3. **Questions**: Anything unclear that needs clarification + +1. **Compatibility**: BREAKING / ADDITIVE / INTERNAL, with what a consumer must change if breaking +2. **Issues**: Problems that should be fixed before commit +3. **Suggestions**: Improvements that aren't blocking +4. **Questions**: Anything unclear that needs clarification + +Anchor each finding to `file:line`. If the staged diff is clean, say so plainly rather than manufacturing findings. diff --git a/.claude/commands/test.md b/.claude/commands/test.md index 3d365c5..05bdffa 100644 --- a/.claude/commands/test.md +++ b/.claude/commands/test.md @@ -1,3 +1,8 @@ +--- +description: Run the full test and code-quality gate, fixing failures to green. +argument-hint: '[test-file-or-path]' +--- + Run `just test-all` and systematically fix all failures to achieve 100% completion. ## Timeouts @@ -40,7 +45,10 @@ For single-layer commands (below), output is short enough that `| tail -20` alon ## Notes -- The pre-commit hook runs check-only commands (`ruff check`, `ruff format --check`, `basedpyright`, `pytest`) — if the formatter would have changed a file, the hook fails. Run `just format` then re-stage. +- **`just test-all` mutates the working tree.** It runs `just format` (`ruff format .`, auto-write) between pytest and the lint check, so a green run can still leave modified files. Check `git status` afterwards and stage what it rewrote — the pre-commit hook runs check-only commands (`ruff check`, `ruff format --check`, `basedpyright`, `pytest`) and fails on exactly those files. +- **Never hand-fix a failure inside generated code.** `robosystems_client/api/`, `robosystems_client/models/`, and `robosystems_client/graphql/generated/` are generation output; an edit there is erased by the next `just generate-sdk` / `just generate-graphql`. The fix belongs in `bin/generate-sdk.sh`'s post-generation patches, in the `[tool.ariadne-codegen]` config in `pyproject.toml`, or in the API's schema. +- **Regeneration needs a reachable API; the GraphQL half doesn't.** `just generate-sdk` fetches the OpenAPI document from a running backend and also refreshes the checked-in `schema.graphql` snapshot. `just generate-graphql` is hermetic — it runs `ariadne-codegen` against that checked-in snapshot plus the operation documents, so it works offline but only reflects the API as of the last `just refresh-schema`. A GraphQL operation test failing after an API change usually means the snapshot is stale, not that the operation is wrong. +- **`just test-all` never builds a distribution.** Packaging problems (`pyproject.toml` metadata, missing `py.typed`, a wrong `requires-python`) pass the whole gate and fail at publish time. Use `just build-package` when the change touches packaging. ## Goal diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md new file mode 100644 index 0000000..bd55e63 --- /dev/null +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -0,0 +1,33 @@ +## Summary + + + +## Changes + + + +- + +## Compatibility + + + +ADDITIVE + +## Testing + + diff --git a/.github/release-notes/README.md b/.github/release-notes/README.md index dbbd692..54875c0 100644 --- a/.github/release-notes/README.md +++ b/.github/release-notes/README.md @@ -6,9 +6,11 @@ releases but reads poorly for a milestone, where the story is what the version _is_ rather than what changed since last Tuesday. To override it, commit the notes here as `v.md` **before** dispatching -`create-release.yml`. The file has to exist at the tagged ref, so it belongs in -release prep alongside the version bump — not added afterwards. When the file is -present the workflow uses it verbatim and skips the generated changelog, the +`create-release.yml`. The file has to exist at the tagged ref, and +`create-release.yml` bumps the version on `main`, cuts `release/` from +the result, and tags it in the same run — so the notes must be merged to `main` +before the dispatch, not added to the release branch afterwards. When the file +is present the workflow uses it verbatim and skips the generated changelog, the release-statistics section, and the generated-with footer. No file, no change: the release falls back to the generated changelog. Skipping