From ae847b5997f3d5a7edc9ffaa89ced3f45386e85d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gonzalo=20Pe=C3=B1a-Castellanos?= Date: Tue, 27 Oct 2026 21:40:58 -0500 Subject: [PATCH 1/4] chore: adopt the AI-native SDLC playbook and repo policies --- .claude/hooks/ruff.sh | 27 +++++ .claude/settings.json | 12 ++ .claude/skills/ai-native-sdlc/SKILL.md | 47 ++++++++ .claude/skills/sdlc-claude-md/SKILL.md | 63 +++++++++++ .claude/skills/sdlc-feedback-loop/SKILL.md | 41 +++++++ .claude/skills/sdlc-intent/SKILL.md | 52 +++++++++ .claude/skills/sdlc-plan/SKILL.md | 54 +++++++++ .claude/skills/sdlc-policy-skill/SKILL.md | 52 +++++++++ .claude/skills/sdlc-pr-review/SKILL.md | 62 +++++++++++ .claude/skills/sdlc-spec/SKILL.md | 59 ++++++++++ .claude/skills/sdlc-subagents/SKILL.md | 54 +++++++++ .editorconfig | 15 +++ .github/ISSUE_TEMPLATE/bug_report.yml | 46 ++++++++ .github/ISSUE_TEMPLATE/config.yml | 8 ++ .github/ISSUE_TEMPLATE/feature_request.yml | 24 ++++ .github/ISSUE_TEMPLATE/task.yml | 39 +++++++ .github/pull_request_template.md | 30 +++++ .gitignore | 3 +- AGENTS.md | 105 ++++++++++++++++++ CLAUDE.md | 1 + CODE_OF_CONDUCT.md | 63 +++++++++++ CONTRIBUTING.md | 36 ++++++ README.release.md | 3 - RELEASE.md | 57 ++++++++++ REVIEW.md | 74 ++++++++++++ SECURITY.md | 73 ++++++++++++ changelog.d/+ai-native-sdlc.infrastructure.md | 1 + docs/review-lessons.md | 65 +++++++++++ intent/README.md | 65 +++++++++++ intent/review-process/intent.md | 80 +++++++++++++ 30 files changed, 1307 insertions(+), 4 deletions(-) create mode 100755 .claude/hooks/ruff.sh create mode 100644 .claude/settings.json create mode 100644 .claude/skills/ai-native-sdlc/SKILL.md create mode 100644 .claude/skills/sdlc-claude-md/SKILL.md create mode 100644 .claude/skills/sdlc-feedback-loop/SKILL.md create mode 100644 .claude/skills/sdlc-intent/SKILL.md create mode 100644 .claude/skills/sdlc-plan/SKILL.md create mode 100644 .claude/skills/sdlc-policy-skill/SKILL.md create mode 100644 .claude/skills/sdlc-pr-review/SKILL.md create mode 100644 .claude/skills/sdlc-spec/SKILL.md create mode 100644 .claude/skills/sdlc-subagents/SKILL.md create mode 100644 .editorconfig create mode 100644 .github/ISSUE_TEMPLATE/bug_report.yml create mode 100644 .github/ISSUE_TEMPLATE/config.yml create mode 100644 .github/ISSUE_TEMPLATE/feature_request.yml create mode 100644 .github/ISSUE_TEMPLATE/task.yml create mode 100644 .github/pull_request_template.md create mode 100644 AGENTS.md create mode 100644 CLAUDE.md create mode 100644 CODE_OF_CONDUCT.md delete mode 100644 README.release.md create mode 100644 RELEASE.md create mode 100644 REVIEW.md create mode 100644 SECURITY.md create mode 100644 changelog.d/+ai-native-sdlc.infrastructure.md create mode 100644 docs/review-lessons.md create mode 100644 intent/README.md create mode 100644 intent/review-process/intent.md diff --git a/.claude/hooks/ruff.sh b/.claude/hooks/ruff.sh new file mode 100755 index 000000000..cd06b3f13 --- /dev/null +++ b/.claude/hooks/ruff.sh @@ -0,0 +1,27 @@ +#!/usr/bin/env bash +# PostToolUse: reject an edit that leaves a Python file unformatted or failing ruff; exit 2 hands the message back to Claude. +set -euo pipefail +command -v jq > /dev/null || { echo "ruff hook: jq not found, skipping (nox -s lint still runs it)." >&2; exit 0; } +file=$(jq -r '.tool_input.file_path // empty') +case "$file" in + *.py) ;; + *) exit 0 ;; +esac +[ -f "$file" ] || exit 0 +if command -v ruff > /dev/null; then + ruff_cmd=(ruff) +elif command -v uvx > /dev/null; then + ruff_cmd=(uvx --quiet ruff) +else + echo "ruff hook: neither ruff nor uvx found, skipping (nox -s lint still runs it)." >&2 + exit 0 +fi +cd "${CLAUDE_PROJECT_DIR:-.}" +if ! out=$("${ruff_cmd[@]}" format --check "$file" 2>&1); then + echo "ruff: $file is not formatted. Run 'nox -s format' (or 'ruff format $file') and re-check. $out" >&2 + exit 2 +fi +if ! out=$("${ruff_cmd[@]}" check "$file" 2>&1); then + echo "ruff check failed on $file: $out" >&2 + exit 2 +fi diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 000000000..e50e517cd --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,12 @@ +{ + "hooks": { + "PostToolUse": [ + { + "matcher": "Edit|Write", + "hooks": [ + { "type": "command", "command": "\"${CLAUDE_PROJECT_DIR}\"/.claude/hooks/ruff.sh" } + ] + } + ] + } +} diff --git a/.claude/skills/ai-native-sdlc/SKILL.md b/.claude/skills/ai-native-sdlc/SKILL.md new file mode 100644 index 000000000..3bcabb66a --- /dev/null +++ b/.claude/skills/ai-native-sdlc/SKILL.md @@ -0,0 +1,47 @@ +--- +name: ai-native-sdlc +description: Entry point for the AI-native SDLC workflow (intent.md -> spec.md -> plan.md -> build -> verify -> review -> release). Use when starting any new feature, change or incident, when asked "where do we start", "what's the next step", or "which play should we run", or when deciding which sdlc-* skill applies to the current state of a change. +--- + +# AI-native SDLC + +Humans stay accountable for every judgment call. Claude does the drafting, checking and plumbing, and every stage leaves a committed Markdown artifact that the next stage consumes. + +## The artifact chain + +Each change gets a folder in the repo: `intent//`. + +| Artifact | Written by | Approved by | Skill | +|---|---|---|---| +| `intent.md` | Originator + Claude | Maintainer | `sdlc-intent` | +| `spec.md` | Claude, from the intent | Maintainer (area owners for flagged concerns) | `sdlc-spec` | +| `plan.md` | Claude in plan mode | Engineer (tech lead if higher risk) | `sdlc-plan` | +| code + tests | Claude, verified by its own feedback loop | Code owner at PR | `sdlc-feedback-loop`, `sdlc-pr-review` | +| release | Maintainer, following `RELEASE.md` | Code owner on the release PR | none | + +## Pick the next step from what exists + +Look in `intent//` and the repo, then route: + +1. No folder, only an idea or a ticket: `sdlc-intent`. +2. `intent.md` approved, no `spec.md`: `sdlc-spec`. +3. `spec.md` approved, no `plan.md`: `sdlc-plan`. +4. `plan.md` approved: build. Use `sdlc-feedback-loop` so the session verifies its own work, and `sdlc-subagents` to parallelise. +5. Diff ready: `sdlc-pr-review`. +6. Releasing: follow `RELEASE.md`. + +Foundations that any stage leans on: `sdlc-claude-md` (shared context in `AGENTS.md`), `sdlc-policy-skill` (organization policy as skills). + +## Prerequisites + +`sdlc-spec` needs an approved `intent.md`; `sdlc-plan` works from a spec or, for small changes, a plain +description; `sdlc-subagents` and `sdlc-pr-review` lean on `AGENTS.md`, `REVIEW.md` and the local CI commands in `AGENTS.md`. If a +prerequisite is missing, say so and offer to run that skill first instead of working around it. + +## Ground rules for every stage + +- Put the artifact in the repo and name its author. Git history is the audit record. +- One source of truth per artifact. If Jira, ServiceNow or a requirements tool already holds the record, either declare it authoritative and write outcomes back through MCP, or link both ways (record ID in the file, commit SHA in the record). +- Advisory controls (skills, `CLAUDE.md`) get a deterministic backstop where the rule must always hold: a hook at the edit, a required check at the PR. +- A human makes each accept or reject decision that moves an artifact to the next stage. Never mark one approved on their behalf. +- Every play has a leading and a lagging indicator. Name them when you set the play up so the team can tell whether it worked. diff --git a/.claude/skills/sdlc-claude-md/SKILL.md b/.claude/skills/sdlc-claude-md/SKILL.md new file mode 100644 index 000000000..53a92bf8c --- /dev/null +++ b/.claude/skills/sdlc-claude-md/SKILL.md @@ -0,0 +1,63 @@ +--- +name: sdlc-claude-md +description: Create, trim or audit a repo's CLAUDE.md so it holds exactly what a new joiner needs on day one (commands, conventions, architecture, recurring mistakes, verification). Use when a repo has no CLAUDE.md, when it has grown stale or long, when Claude repeats a mistake, or when asked to "set up CLAUDE.md" or "update our CLAUDE.md". +--- + +# The CLAUDE.md + +`CLAUDE.md` is the context a new joiner would need, read by the agent at the start of every session. It is version controlled and reviewed like code. Keep it under a page: Claude reads all of it every session, and stale lines waste context. + +## Create + +1. Survey the repo: build files, `Makefile`/package scripts, CI config, README, directory layout, test setup. +2. Draft with `/init` if available, or by hand, then cut it down to day-one essentials. +3. Verify every command by running it (or asking the engineer). Never list a command you have not seen work, and note a healthy output for each. +4. Ask the engineer who knows the codebase best for the "things Claude gets wrong" list; pull more from recent PR review comments and reverted changes. +5. Place it at the repo root and commit, with code owners set so changes get reviewed. + +## Template + +```markdown +# + +## Commands +- Build: +- Test: (unit), (integration, needs ...) +- Lint: (runs in CI; fix before pushing) +- Run: () + +## Conventions +- +- + +## Architecture +- + +## Things Claude gets wrong +- + +## Verifying your work +- Build: (must finish with "") +- Test: (all green; never skip or delete a failing test) +- Lint: (zero warnings) +Run all three before reporting any task complete, and paste the output. +If a test fails, fix the code, not the test. +``` + +If artifacts live in `intent/`, add a line saying `intent/` holds `intent.md`, `spec.md` and `plan.md` per change. + +## Audit an existing file + +- Remove anything stale, obvious from the code, or already enforced by CI or a hook. +- Move multi-step procedures into a skill (`sdlc-policy-skill`); move must-always-hold rules behind a hook (`.claude/settings.json`) or a CI check. +- Flag missing sections, unverifiable commands, and contradictions with `README` or CI. +- Report the diff and why; do not silently drop lines someone may rely on. + +## Maintenance rule + +When Claude makes the same mistake twice, the correction goes into `CLAUDE.md` in the same PR. PR review should also flag changes that make `CLAUDE.md` outdated (`sdlc-pr-review`). Trimming changes agent behavior, so watch the next few PRs for regressions. + +## Measure + +- Leading: how often Claude repeats a mistake `CLAUDE.md` should have caught (track corrections in Git history). +- Lagging: time to first merged PR for a new team member. diff --git a/.claude/skills/sdlc-feedback-loop/SKILL.md b/.claude/skills/sdlc-feedback-loop/SKILL.md new file mode 100644 index 000000000..db2525b95 --- /dev/null +++ b/.claude/skills/sdlc-feedback-loop/SKILL.md @@ -0,0 +1,41 @@ +--- +name: sdlc-feedback-loop +description: Give Claude a way to verify its own work (tests, build, lint, screenshot diff) and protect that check from being weakened. Use when starting implementation or a bug fix, when asked to "set up verification", "fix this bug test-first", or when Claude reports work done without proof; also for UI changes that need a visual check. +--- + +# Give Claude a feedback loop + +Always give the session a way to check its own work before a person sees it. A late signal (CI minutes later, a tester days later) turns the engineer into the bottleneck. + +## Set up + +1. Wrap verification in one command per concern that exits non-zero on failure (`make test`, `npm test`, `make lint`, `make build`). Create the targets if only a command sequence exists today. +2. List them in `CLAUDE.md` (see `sdlc-claude-md`) with a sample of healthy output, plus the instruction: run the tests before reporting a task complete and show the output. +3. State a quantifiable target for the task so Claude can check without asking: "all tests in X pass", "screenshot matches the attached mock", "the endpoint returns 200 with the new field". Take targets from `plan.md` Proof when it exists. +4. For UI work, give Claude a browser or screenshot tool (e.g. via MCP) and the mock: implement, screenshot, compare, adjust. Two or three rounds is normal. + +## Bug fixes: failing test first + +1. Reproduce the bug as a test. Run it and confirm it fails for the reason expected. +2. Commit that test. +3. Only then fix the code, without editing the test. A test that existed before the fix, and that the agent could not rewrite, is proof the bug is gone. + +## Protect the loop + +An agent fixing code must not be able to weaken the check on that code. Either: + +- block edits to existing test files once the failing-test commit exists, with a `PreToolUse` hook (`sdlc-policy-skill` shows the pattern), or +- check the diff in review: new regression tests are expected, but reject any change after the failing-test commit that weakens, skips or rewrites a test (`sdlc-pr-review`). + +## Report format + +When finishing, paste the literal output of the build, test and lint commands (or the screenshot diff). Evidence comes from the toolchain, not from the agent's summary. + +## Governance + +Verification before "done" and the block on editing tests are enforced as hooks where the organization wants them guaranteed. Evidence lives in the session transcript and in the PR's check run; the code owner reviews intent and risk because mechanical evidence is already attached. + +## Measure + +- Leading: first-pass CI success rate of agent-written changes. +- Lagging: review time per PR (should fall) and change failure rate from the incident tracker. diff --git a/.claude/skills/sdlc-intent/SKILL.md b/.claude/skills/sdlc-intent/SKILL.md new file mode 100644 index 000000000..36c20fea9 --- /dev/null +++ b/.claude/skills/sdlc-intent/SKILL.md @@ -0,0 +1,52 @@ +--- +name: sdlc-intent +description: Turn an idea, ticket, pain point or incident into a committed intent.md proto-spec by interviewing the originator. Use when someone describes a problem or wants a new feature and there is no intent.md yet, when asked to "capture intent", "write up this idea", or "start a new change". +--- + +# Capture as intent.md + +`intent.md` is a short proto-spec in the originator's own terms: what is wanted, why, and under which constraints. It is human readable and machine actionable, and it kicks off the rest of the chain. + +## Steps + +1. Let the originator describe the problem in their own words. No formal language needed. If the source is a ticket or incident, read it (via MCP or the pasted text) and start from there. +2. Interview until the idea is concrete, asking what an analyst would: who is affected, what they cannot do today, what better looks like, what is out of scope, which systems are touched, which constraints apply (security, compliance, existing auth), how success is observed. Ask a few questions at a time, not a form. +3. Check for an agreed template (`intent/TEMPLATE.md` or an organization skill) and use it. Otherwise use the template below. +4. Write the draft to `intent//intent.md` (kebab-case slug, create the folder). +5. Read it back and have the originator correct anything misunderstood. Do not invent answers: unresolved items go under Open questions. +6. Commit when the originator is satisfied (follow the repo's commit conventions). The product owner picks it up from there and decides accept or close. + +## Template + +```markdown +# Intent: +Author: . Status: draft. + +## Problem + + +## Proposed outcome + + +## Affected users and systems + + +## Constraints + + +## Open questions + +``` + +## Rules + +- Stay in the problem space. No design, no implementation, no file names. +- If the originator is not a Git user, commit through the version-control connector on their behalf and record them as author in the header. +- Track an incident-sourced intent the same way, and say in the header that it came from monitoring (`Author: monitoring agent`) so the product owner knows to review harder. + +## Measure + +- Leading: time from first conversation to a committed `intent.md` (Git timestamps). +- Lagging: share of intents the product owner accepts into design instead of closing, and how many edits `intent.md` receives after the first `spec.md` commit. + +Next step once accepted: `sdlc-spec`. diff --git a/.claude/skills/sdlc-plan/SKILL.md b/.claude/skills/sdlc-plan/SKILL.md new file mode 100644 index 000000000..8c50f9915 --- /dev/null +++ b/.claude/skills/sdlc-plan/SKILL.md @@ -0,0 +1,54 @@ +--- +name: sdlc-plan +description: Produce and commit a plan.md before any code is written, working in plan mode from the approved spec.md and intent.md. Use when starting implementation of a change, when asked to "plan this", "make an implementation plan", or before editing code for a change that has an intent/spec; also use to keep plan.md in sync when implementation departs from it. +--- + +# Plan mode as the default starting point + +Work starts with a written, reviewable plan. Plan mode lets Claude read the codebase without changing anything, so course corrections are edits to a document. + +## Steps + +1. Work in plan mode (Shift+Tab, or start the session with `--permission-mode plan`). Do not edit files until the engineer accepts the plan. +2. Read `intent//intent.md`, `spec.md` (the requirement numbers R1.. and concerns), `CLAUDE.md` and the code the change touches. If a spec exists, a plan without it is a smell; if neither exists, ask whether to run `sdlc-intent` / `sdlc-spec` first or to plan from a plain description. +3. Draft the plan using the template below. +4. Let the engineer interrogate it. Be ready to answer: what could this break, which step is riskiest, which alternatives were rejected and why. +5. Iterate until someone who never saw the conversation could implement the change from the plan alone. +6. Commit the approved plan as `intent//plan.md`. Then implement; with a solid plan this is often one pass. +7. If implementation departs from the plan, update `plan.md` in the same commit. Consider a hook that flags source changes with no matching plan update. + +## Template + +```markdown +# Plan: (from intent.md and spec.md) + +## Files that change +<paths, marking new files> + +## Order of work +1. <step> (R1, R2). +2. <step> (R4). + +## Risks +<what could break; cite spec concerns by number> + +## Proof +<which tests or checks cover which requirements, and how UI changes are verified> +``` + +## Rules + +- Cite requirement numbers on every step and every proof line so a reviewer can check the plan against the spec line by line. +- The Proof section must be executable by Claude (tests, build, screenshot compare); see `sdlc-feedback-loop`. +- Independent steps (different files) can run in parallel sessions; mark them so `sdlc-subagents` can split the work. +- Plans for changes the organization classes as higher risk go to a tech lead or architect before acceptance. +- Existing records (Jira, requirements tools) stay authoritative when the organization says so: note the record ID in the plan header, and write outcomes back through the connector. Otherwise link both ways. + +## Auto mode + +Once guardrails mature (a tuned `CLAUDE.md`, policy skills, blocking hooks, a test suite Claude can run), routine work with a tight spec and a small blast radius can run in auto mode after the plan is approved. The plan is still reviewed first. + +## Measure + +- Leading: share of changes that merge from the first implementation pass; time from plan approval to merged PR. +- Lagging: rework cycles per change; how often the merged diff still matches `plan.md`. diff --git a/.claude/skills/sdlc-policy-skill/SKILL.md b/.claude/skills/sdlc-policy-skill/SKILL.md new file mode 100644 index 000000000..01be1fc6e --- /dev/null +++ b/.claude/skills/sdlc-policy-skill/SKILL.md @@ -0,0 +1,52 @@ +--- +name: sdlc-policy-skill +description: Write a Claude skill that encodes an organization policy or standard (security, API design, brand, compliance, UX), with a trigger test and a deterministic backstop. Use when asked to "turn this policy into a skill", "encode our standard", "make Claude follow our X guidelines", or when a rule is enforced inconsistently today. +--- + +# Skills as institutional knowledge + +A skill makes institutional knowledge operational: explicit, version controlled, applied broadly, updated centrally. It is an advisory control. Write one for knowledge that must be applied consistently; keep general project context in `CLAUDE.md` and one-off instructions in a prompt. + +## Decide where it belongs + +| Knowledge | Home | +|---|---| +| Conventions every session needs (commands, layout) | `CLAUDE.md` | +| A policy with an owner, applied when a certain task happens | **a skill** | +| A rule that must hold with no exceptions | skill + hook (+ required CI check) | +| A recurring job needing its own context and tools | subagent (`sdlc-subagents`) | + +## Steps + +1. Pick one policy that is enforced inconsistently today. Get its written source of truth and its named owner. +2. Create `.claude/skills/<name>/SKILL.md` (ships with the repo) or put it in a plugin for organization-wide distribution. +3. Frontmatter: `name`, and a `description` that says what the skill does and exactly when to trigger (task types, file types, verbs users would say). The description decides whether it ever loads. +4. Body: numbered, testable rules in imperative form; cite the policy section each rule comes from; name any script to run and require its output in the summary. +5. Test triggering: ask Claude for the relevant task in 4-5 different phrasings, including ones that never mention the policy, and confirm the skill loads each time. Tighten the description until it does. +6. Have the policy owner sign off. Policy changes become skill changes, reviewed like code; engineers pick up the new version in their next session. +7. Back it up (below). + +## Template + +```markdown +--- +name: <kebab-name> +description: <What it enforces>. Use whenever <task types and triggers>. +--- +# <Title> +When you <do the task>: +1. <Rule>. (Policy §x.y) +2. <Rule>. +Run <script> and include its output in your summary. +``` + +## Add a deterministic backstop for must-hold rules + +- **Hook at the edit**: a `PostToolUse` hook (matcher `Edit|Write`) that runs the same script the skill names, only for the files that changed. Exit code 2 feeds the script's message back to Claude so it fixes the line. Keep build-phase hooks fast and scoped; heavy checks belong at commit or PR. +- **Required check at the PR**: run the same script in CI as a required status check. It also catches changes that bypassed the hook (a shell command rewriting a file, a different tool). +- A hook that asks a human for approval is a gate, not a guardrail; keep those at release, not mid-build. + +## Measure + +- Leading: time from policy owner approving a change to the updated skill merging. +- Lagging: PR review findings that cite the policy. They should trend to zero; if not, the skill is not triggering or has drifted from the official text. diff --git a/.claude/skills/sdlc-pr-review/SKILL.md b/.claude/skills/sdlc-pr-review/SKILL.md new file mode 100644 index 000000000..db76d171c --- /dev/null +++ b/.claude/skills/sdlc-pr-review/SKILL.md @@ -0,0 +1,62 @@ +--- +name: sdlc-pr-review +description: Set up REVIEW.md and run structured AI review passes (bugs, security, compliance against spec.md/plan.md) on a diff or PR with severity-ranked findings, or address review comments on a PR. Use when asked to "review this PR/diff", "set up PR review", "write REVIEW.md", "address the review comments", or before a PR is marked ready. +--- + +# AI in the PR review loop + +Claude both gives and receives reviews. Every PR gets the same review passes with findings ranked by severity, so human attention moves up to whether the change does what the plan intended and whether the risk is acceptable. Separation of duties holds: the agent that wrote the code has no way to approve it. Approval comes from a human code owner through branch protection. + +## Mode A: set up `REVIEW.md` (tech lead) + +1. Ask which passes the organization cares about. Default: Bugs, Security, Compliance (change matches `spec.md`, `plan.md` and design principles). +2. Define what Important means (breaks behavior, leaks data, breaches policy) and treat style and naming as nits. +3. Cap nits (e.g. five per review, summarize the rest as a count) and list what not to report (generated paths, anything CI already enforces). +4. Commit `REVIEW.md` at the repo root. + +```markdown +# Review instructions +## Passes +Run three passes and tag each finding with its pass: +- Bugs: logic errors, broken edge cases, subtle regressions +- Security: injection risks, authentication gaps, PII in logs +- Compliance: the change matches spec.md, plan.md and our design principles +## What Important means here +Reserve Important for findings that would break behavior, leak data +or breach a policy. Style and naming are nits. +## Cap the nits +Report at most five nits per review; summarize the rest as a count. +## Do not report +Generated files and anything CI already enforces. +``` + +Hosting options: the managed Code Review service (admin enables it per repository) or the `claude-code-action` in your own CI when you need control of the pipeline or model access through your own cloud agreement. + +## Mode B: review a diff or PR + +1. Get the diff (`git diff <base>...HEAD`, or `gh pr diff`). Read `REVIEW.md`, `CLAUDE.md`, and the change's `intent/<slug>/spec.md` and `plan.md` if present. The compliance pass depends on the committed plan, which a reviewer looking at the diff alone would not have open. +2. Run each pass in `REVIEW.md` over the full diff, citing requirement numbers (R1..) and plan steps where relevant. +3. Check that tests were not weakened and that `plan.md` was updated if the implementation departed from it. +4. Report a table sorted by severity: + + | Severity | File:Line | Issue | + |---|---|---| + | Important | `path:22` | Compliance: <finding, cite plan/spec> | + | Nit | `path:8` | Bugs: <finding> | + +5. Findings do not approve or block on their own. State plainly that approval is for the code owner. + +## Mode C: address review comments + +When tagged on a comment, make the fix, push, and reply on the thread so it records both the request and the change. To shepherd your own PR: sweep unresolved comments and failing checks, fix, push, repeat until green and waiting only on code-owner approval. Never approve or merge your own PR. + +## Feedback into the system + +- A mistake flagged a second time goes into `CLAUDE.md` in that PR (`sdlc-claude-md`). +- Flag when a change makes `CLAUDE.md` outdated. +- Monthly: rate findings, cap noisy categories in `REVIEW.md`. + +## Measure + +- Leading: time to first review (minutes); share of review comments resolved without a human touching the branch. +- Lagging: defects and vulnerabilities caught before merge vs escaping to production. diff --git a/.claude/skills/sdlc-spec/SKILL.md b/.claude/skills/sdlc-spec/SKILL.md new file mode 100644 index 000000000..fc4df26e2 --- /dev/null +++ b/.claude/skills/sdlc-spec/SKILL.md @@ -0,0 +1,59 @@ +--- +name: sdlc-spec +description: Turn an approved intent.md into a requirements-and-design spec.md with numbered requirements, a constraint-coverage table and flagged areas of concern, applying the organization's policy skills. Use when an intent.md has been accepted and needs a spec, or when asked to "write the spec", "do requirements and design", or "convert this intent into requirements". +--- + +# Requirements and design: intent.md to spec.md + +Requirements and design happen in one session instead of two hand-offs. The product owner reviews the spec; they do not write it. The goal is a spec engineering can plan against, with every concern surfaced early. + +## Before you start + +- Locate `intent/<slug>/intent.md` and confirm it is approved (ask if unclear). No intent, run `sdlc-intent` first. +- List the policy skills available (`.claude/skills/`, plugins) for brand, security, compliance and UX, and apply every one that is relevant. If none exist, say so, because the spec will rest on stated constraints only. +- Read `CLAUDE.md` and the parts of the codebase the intent touches so the design integrates with what exists. + +## Produce `intent/<slug>/spec.md` + +```markdown +# Spec: <title> +From: intent.md (<author>). Status: ready for product owner review. +Skills applied: <skill names>. + +## Requirements +- R1. <testable statement> +- R2. ... + +## Design +<endpoints, components, data flow, where each piece lives in the existing system> + +## How each constraint in the intent is met +| Constraint | Met by | +| :---- | :---- | +| <from intent> | <requirement or design element> | + +## Open questions from the intent +- <question>. <answered here, or carried forward as concern N> + +## Areas of concern +1. <concern, why it matters>. Owner: <policy owner>. +``` + +## Rules + +- Number every requirement (R1, R2, ...). The plan and the review cite these numbers. +- Make requirements testable: observable behavior, a measurable limit, an exact field set. +- Every constraint in the intent must appear in the coverage table. If one cannot be met, it becomes a concern, not a silent omission. +- When two policies conflict, do not pick a winner. Describe the clash, propose a resolution, name the owner. +- Carry unanswered intent questions forward as concerns with an owner; never guess. +- Record which skills (and versions) were applied, so the audit trail shows the live policy was read. +- Do not write code in this stage. + +## Hand-off + +Tell the product owner to resolve flagged concerns with their policy owners first (these are the points an analyst would have escalated), then review the spec against the original idea. A human decides whether the spec and intent progress to build; ask a technical lead to weigh in for higher-risk changes. Commit `spec.md` next to `intent.md` once accepted. Next step: `sdlc-plan`. + +## Measure + +- Leading: elapsed time between the `intent.md` commit and the `spec.md` commit. +- Lagging: `spec.md` commits dated after the first `plan.md` commit for the same change (requirements rework). diff --git a/.claude/skills/sdlc-subagents/SKILL.md b/.claude/skills/sdlc-subagents/SKILL.md new file mode 100644 index 000000000..594c836e4 --- /dev/null +++ b/.claude/skills/sdlc-subagents/SKILL.md @@ -0,0 +1,54 @@ +--- +name: sdlc-subagents +description: Split a plan into independent streams for parallel Claude sessions in separate git worktrees, and define reusable subagents (verifier, simplifier, researcher) in .claude/agents/. Use when asked to "work on these in parallel", "set up worktrees", "create a verifier agent", or when one engineer has several independent tasks in flight. +--- + +# Parallel sessions and subagents + +A **parallel session** is a full Claude Code instance on its own task in its own git worktree; sessions share nothing except the engineer steering them. A **subagent** is a scoped helper inside one session, with its own context window and tool limits, for jobs that recur across tasks. Sessions raise how many tasks are in flight; subagents keep each session focused. The engineer's job becomes steering and reviewing. + +## Prerequisites + +`CLAUDE.md` exists (every session reads it). A feedback loop helps, since sessions that can verify themselves need less supervision (`sdlc-feedback-loop`). Permission settings should be tuned so safe commands do not stall sessions on prompts. + +## Split the work + +1. Read `plan.md` (or the task list). Group steps by the files they touch. +2. Independent groups become parallel sessions. Tasks that share files run in one session, sequentially. +3. Start with two or three sessions. The ceiling is how many streams the engineer can review properly; add more only while review keeps up. +4. Give each its own worktree and branch: + ```bash + claude --worktree feature-auth + claude --worktree fix-rate-limit + ``` +5. Give each session its slice of the plan and its own proof (tests or checks) so it can finish without the others. + +## Define subagents + +Markdown files in `.claude/agents/`, checked into Git so the team shares them. Each needs a name, a description of when to use it, and the minimal tools it may touch. + +```markdown +--- +name: verifier +description: Runs the app and checks the change works before the session reports done +tools: Bash, Read +--- +Start the app with <run command>. Exercise the changed behavior and the two +nearest neighboring flows. Report what you ran, what you saw, and any +behavior that does not match plan.md. Do not fix anything; report only. +``` + +Useful starting set: + +- **verifier**: fresh-context final check, so the verdict is not colored by the assumptions that produced the code. This complements, and does not replace, the feedback loop running throughout the task. +- **code-simplifier**: strips needless complexity after the main agent finishes. +- **researcher**: explores the codebase and reports back without flooding the main context. + +## Governance + +More sessions means more output, so controls must live in repo configuration (hooks, permission settings) that all sessions inherit. Activity is attributed to the engineer who ran it and exported via OpenTelemetry if configured. + +## Measure + +- Leading: concurrent sessions per engineer while review quality holds; share of the day spent steering vs waiting. +- Lagging: changes merged per engineer per week, read alongside the rework rate from PR history. diff --git a/.editorconfig b/.editorconfig new file mode 100644 index 000000000..065eceaaa --- /dev/null +++ b/.editorconfig @@ -0,0 +1,15 @@ +root = true + +[*] +end_of_line = lf +charset = utf-8 +trim_trailing_whitespace = true +insert_final_newline = true + +[*.py] +indent_style = space +indent_size = 4 + +[*.{json,toml,yml,yaml,md}] +indent_style = space +indent_size = 2 diff --git a/.github/ISSUE_TEMPLATE/bug_report.yml b/.github/ISSUE_TEMPLATE/bug_report.yml new file mode 100644 index 000000000..02fe61c11 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/bug_report.yml @@ -0,0 +1,46 @@ +name: Bug report +description: Something in b2sdk behaves incorrectly. +labels: [bug] +type: Bug +body: + - type: input + id: version + attributes: + label: b2sdk version + description: From `python -c "import b2sdk.version; print(b2sdk.version.VERSION)"` or `pip show b2sdk`. + validations: + required: true + - type: input + id: apiver + attributes: + label: API version you import + placeholder: b2sdk.v2 + validations: + required: true + - type: input + id: python + attributes: + label: Python version and OS + placeholder: CPython 3.12.7, Ubuntu 24.04 + validations: + required: true + - type: textarea + id: what + attributes: + label: What happened, and what did you expect? + validations: + required: true + - type: textarea + id: repro + attributes: + label: Minimal reproduction + description: Code and steps. Never paste application keys or auth tokens. + render: python + validations: + required: true + - type: textarea + id: logs + attributes: + label: Traceback or logs + description: Redact keys, tokens and bucket names you consider private. + render: text diff --git a/.github/ISSUE_TEMPLATE/config.yml b/.github/ISSUE_TEMPLATE/config.yml new file mode 100644 index 000000000..a7837f57b --- /dev/null +++ b/.github/ISSUE_TEMPLATE/config.yml @@ -0,0 +1,8 @@ +blank_issues_enabled: false +contact_links: + - name: Report a security vulnerability + url: https://github.com/Backblaze/b2-sdk-python/security/advisories/new + about: Report privately. Do not open a public issue. See SECURITY.md. + - name: B2 service or account problem + url: https://help.backblaze.com/hc/en-us/requests/new + about: Problems with the B2 service itself, billing or your account go to Backblaze support. diff --git a/.github/ISSUE_TEMPLATE/feature_request.yml b/.github/ISSUE_TEMPLATE/feature_request.yml new file mode 100644 index 000000000..0e9f7de4b --- /dev/null +++ b/.github/ISSUE_TEMPLATE/feature_request.yml @@ -0,0 +1,24 @@ +name: Feature request +description: A B2 capability or API improvement b2sdk should support. +labels: [enhancement] +type: Feature +body: + - type: textarea + id: problem + attributes: + label: Problem + description: What can you not do today, and who is affected? + validations: + required: true + - type: textarea + id: outcome + attributes: + label: Proposed outcome + description: What better looks like. Link the B2 API docs for the calls involved if you know them. + validations: + required: true + - type: textarea + id: constraints + attributes: + label: Constraints and alternatives + description: Compatibility, capabilities needed, apiver layers affected (`b2sdk.v2`, `b2sdk.v3`), behavior of the b2 CLI or other B2 SDKs, workarounds. diff --git a/.github/ISSUE_TEMPLATE/task.yml b/.github/ISSUE_TEMPLATE/task.yml new file mode 100644 index 000000000..cfcc2cadf --- /dev/null +++ b/.github/ISSUE_TEMPLATE/task.yml @@ -0,0 +1,39 @@ +name: Task +description: Maintenance, tooling, CI, docs or release work that is neither a bug nor a new feature. +type: Task +body: + - type: textarea + id: goal + attributes: + label: What needs doing, and why? + description: The outcome and the reason it matters now. + validations: + required: true + - type: textarea + id: done + attributes: + label: Done when + description: Checkable conditions, one per line. + value: | + - [ ] + validations: + required: true + - type: textarea + id: scope + attributes: + label: Scope and context + description: Files, workflows or settings involved; related issues and PRs; what is out of scope. + - type: dropdown + id: area + attributes: + label: Area + options: + - CI and workflows + - Docs + - Release + - Tests + - Dependencies + - Repository settings + - Other + validations: + required: true diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md new file mode 100644 index 000000000..597c49365 --- /dev/null +++ b/.github/pull_request_template.md @@ -0,0 +1,30 @@ +Summary: + +## Motivation + +Why this change is needed. Link the issue (`Fixes #123`) and, if one exists, `intent/<slug>/`. + +## Changes + +What changed, and which apiver layers (`b2sdk.v0`..`b2sdk.v3`) see it. If this PR is stacked on another, name it and the merge order. + +## Testing + +Paste the tail of the local CI commands from `AGENTS.md`. For each behavior this PR adds or fixes, name the test that fails when the change is reverted. List any live-B2 integration tests or `.sdkharness` checks you ran, and any you could not. + +## Risk + +What could break for existing callers of each apiver layer, and whether the request sent to B2 changes. Name the riskiest part of the diff. + +## How to revert + +Whether reverting this commit is enough, or whether callers (including the B2 CLI) would need to pin the previous release. + +## Checklist + +- [ ] `nox -s lint`, `nox -s unit`, `nox -s build` and `nox -s towncrier_check` pass locally. +- [ ] A fresh-context review (`/code-review`, `/security-review` or a subagent) ran on the diff; findings are fixed or answered below. +- [ ] A `changelog.d/` fragment exists, or the PR is labelled `-changelog`. +- [ ] New or changed behavior is pinned by a test that fails when the change is reverted, on every apiver it affects. +- [ ] Docstrings, `doc/source`, README and `AGENTS.md` are updated if needed. +- [ ] The description matches the final code. diff --git a/.gitignore b/.gitignore index 7ee7c81ae..e5ac53132 100644 --- a/.gitignore +++ b/.gitignore @@ -14,4 +14,5 @@ venv .vscode .pdm-build/ .pdm-python -.env \ No newline at end of file +.env.DS_Store +.claude/settings.local.json diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 000000000..381b83d45 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,105 @@ +# b2sdk + +Python SDK for Backblaze B2 Cloud Storage (`b2sdk` on PyPI, `Backblaze/b2-sdk-python`). The B2 CLI is +its largest user. This file is the map for coding agents and new contributors; `CLAUDE.md` points here. +Keep it under about 150 lines and link out for detail. + +## Commands + +Tooling is `nox` + `uv` (`pip install nox uv`). Sessions are defined in `noxfile.py`. + +- Local CI, run before every push and paste the tail in the PR: + `nox -s lint`, `nox -s towncrier_check`, `nox -s unit-3.14`, `nox -s build`, `nox -s doc`. +- Format: `nox -s format` (ruff). Lint: `nox -s lint` (ruff, `test/static`, liccheck). +- Unit tests run once per apiver: `nox -s unit-3.14` runs `pytest --api=v3`, then `v2`, `v1`, `v0`. + By keyword: `nox -s unit-3.14 -- -k keyword`. One apiver: `uv run --group test pytest --api=v2 test/unit`. +- Integration tests hit live B2 and need `B2_TEST_APPLICATION_KEY_ID` and `B2_TEST_APPLICATION_KEY`: + `nox -s integration-3.14`. Leftover test buckets: `nox -s cleanup_old_buckets`. +- Docs: `nox -s doc` (needs `graphviz` and `plantuml`) also runs `doc_cover`, which fails on undocumented API. +- Lock file: `uv lock --check`. Dependency changes go through `uv add`/`uv lock`, never by hand. +- Changelog fragment: `uv run --group release towncrier create -c 'Text.' +slug.fixed.md`. +- Release: [RELEASE.md](RELEASE.md) (`nox -s make_release_commit -- X.Y.Z`). + +## Architecture + +| Path | Role | +| --- | --- | +| `b2sdk/_internal/` | All implementation. Private: users never import it. | +| `b2sdk/v3/`, `v2/`, `v1/`, `v0/` | Public apiver layers. `v3` re-exports `_internal`; older layers wrap or subclass to keep their old signatures and behavior. | +| `_internal/api.py`, `bucket.py` | High-level `B2Api` and `Bucket`, the surface most callers use. | +| `_internal/session.py`, `b2http.py` | Auth, reauth, retries, `Retry-After`, exception translation. | +| `_internal/raw_api.py` | One method per B2 Native API call (`AbstractRawApi`, `B2RawHTTPApi`). | +| `_internal/raw_simulator.py` | In-memory B2 used by unit tests and by users; must match `raw_api.py`. | +| `_internal/transfer/`, `large_file/` | Upload, download, copy, emerger and large-file managers (thread pools). | +| `_internal/sync/`, `scan/` | Folder scan and sync (used by `b2 sync`). | +| `_internal/account_info/` | Credential and cache stores, including `SqliteAccountInfo`. | +| `test/unit/` | Unit tests; `test/unit/vN/apiver` and `v_all` hold apiver-specific tests, selected by `--api`. | +| `test/integration/` | Live-B2 tests, using `_internal/testing/helpers/bucket_manager.py`. | +| `.sdkharness/` | Contract checks the central SDK harness runs against the B2 simulator on loopback. | +| `changelog.d/` | towncrier fragments; `CHANGELOG.md` is built from them at release. | +| `intent/` | Per-change `intent.md`, `spec.md`, `plan.md` (see Workflow). | + +## Conventions + +- Python 3.10+ (`requires-python`); CI runs 3.10-3.14 and PyPy 3.10 on Linux, macOS and Windows. +- ruff, 100-column lines, single quotes. Every file keeps the Backblaze license header (`test/static` checks it). +- Semver on each apiver. A breaking change goes into a new apiver (or the newest, unreleased one), never + into an older layer. Deprecate with a warning before removing. +- New B2 calls follow the B2 docs and the + [Integration Checklist](https://www.backblaze.com/docs/cloud-storage-integration-checklist). +- Comments are short and explain non-obvious decisions. Public API gets docstrings that Sphinx renders. +- Detail: past review problems in [docs/review-lessons.md](docs/review-lessons.md), contributor setup in + [CONTRIBUTING.md](CONTRIBUTING.md), the harness in [.sdkharness/README.md](.sdkharness/README.md). + +## Things agents get wrong here + +Each line was raised by a reviewer, or fixed by a follow-up PR, on at least two PRs. + +1. **Missing or wrong changelog fragment.** Every PR adds `changelog.d/<issue>.<type>.md` or + `+slug.<type>.md` (types in `pyproject.toml`). Write it for SDK users, about the fix, not the test. +2. **Tests that do not pin the fix.** Revert the change and the new test must fail. Exercise the mechanism + (the real sleep in `_translate_and_retry`, a new upload URL fetched after a 408), not an attribute. +3. **Retrying uploads in the HTTP layer.** Uploads go out with `try_count=1` so the error reaches + `UploadManager`, which gets a fresh upload URL and token. Do not retry them in `b2http.py`. +4. **Retry parity.** Treat every retryable 5xx alike, and parse `Retry-After` through + `_parse_retry_after_seconds` so a non-numeric value (an HTTP date) never raises. +5. **Missing apiver exports.** A new public name is exported from `b2sdk/v3` (`__init__.py`, + `exception.py`, `testing/__init__.py` as relevant); older layers must not pick up v3 behavior or + rely on v3-only abstractions. Run the unit tests on every `--api`. +6. **`raw_simulator.py` drift.** Any change to `AbstractRawApi` or a B2 call's behavior lands in + `RawSimulator` in the same PR. +7. **Accidental public interface.** Do not call another class's `_private` members; keep tunables as + module-level `_CONSTANTS`, not new parameters; use SDK-style names and thread new options up to `Bucket`. +8. **Hidden failures.** No catch-all `except`, no `pytest.fail` that drops the real error, no async test + that is never awaited. A skipped check is NOT RUN, never PASS. +9. **Portability.** Windows and new Pythons: do not reopen a `NamedTemporaryFile` by name; handle + permissions and symlinks; check `scan/` on Windows paths. +10. **Integration tests without the helpers.** Use `bucket_manager.py` (seeded names, retries, cleanup) and + a temporary account-info dir; never leave buckets or keys behind. +11. **sdkharness safety.** Checks target only `http://127.0.0.1:<port>` with `test-key-id`/`test-key`; + never read or print ambient `B2_*` credentials. +12. **Duplicate and split PRs.** Search open PRs first; one change per PR, against `master`. +13. **Claiming green over a red CI.** The live-B2 stage has known failures (#614). Reproduce on `master` + and cite the issue; never call a failure "unrelated" without showing it. + +## Workflow (AI-native SDLC) + +- Non-trivial changes start in `intent/<slug>/`: `intent.md` (problem) → `spec.md` (numbered requirements + R1..) → `plan.md` (steps cite R numbers). A human approves each before the next. Templates in + [`intent/README.md`](intent/README.md). Bug fixes with a linked issue may skip straight to a failing test. +- Bug fixes: commit the failing regression test first, then the fix, without editing the test. +- Skills in `.claude/skills/` (`ai-native-sdlc` routes to the right one). A `PostToolUse` hook runs ruff on + every Python file Claude edits. +- Before opening a PR, review your own diff in a fresh context (`/code-review` and `/security-review`, or a + subagent that did not write the code) and fix or answer what it finds. Say in the PR what it reported. +- Review: [REVIEW.md](REVIEW.md). Every finding gets fixed or resolved with a reason. Agents never approve + or merge their own PRs; a maintainer approves. +- The person directing an agent owns its PR: they count as one of its reviewers and fix what it breaks. +- Stacked PRs: state the base PR and merge order in the description; retarget to `master` after it merges. +- Releases: [RELEASE.md](RELEASE.md). Security reports: [SECURITY.md](SECURITY.md). + +## Verifying your work + +Run the local CI commands above and paste the tail of their output. If a test fails, fix the code, not +the test. Say which apivers and Python versions you ran, and list any integration or `.sdkharness` checks +you ran and any you could not. diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 000000000..43c994c2d --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1 @@ +@AGENTS.md diff --git a/CODE_OF_CONDUCT.md b/CODE_OF_CONDUCT.md new file mode 100644 index 000000000..2e37e75ad --- /dev/null +++ b/CODE_OF_CONDUCT.md @@ -0,0 +1,63 @@ +# Contributor Covenant Code of Conduct + +## Our Pledge + +We as members, contributors, and leaders pledge to make participation in our +community a harassment-free experience for everyone, regardless of age, body +size, visible or invisible disability, ethnicity, sex characteristics, gender +identity and expression, level of experience, education, socio-economic status, +nationality, personal appearance, race, religion, or sexual identity and +orientation. + +We pledge to act and interact in ways that contribute to an open, welcoming, +diverse, inclusive, and healthy community. + +## Our Standards + +Examples of behavior that contributes to a positive environment include: + +- Demonstrating empathy and kindness toward other people +- Being respectful of differing opinions, viewpoints, and experiences +- Giving and gracefully accepting constructive feedback +- Accepting responsibility and apologizing to those affected by our mistakes, + and learning from the experience +- Focusing on what is best for the overall community + +Examples of unacceptable behavior include: + +- The use of sexualized language or imagery, and sexual attention or advances of + any kind +- Trolling, insulting or derogatory comments, and personal or political attacks +- Public or private harassment +- Publishing others' private information, such as a physical or email address, + without their explicit permission +- Other conduct which could reasonably be considered inappropriate in a + professional setting + +## Enforcement Responsibilities + +Project maintainers are responsible for clarifying and enforcing our standards of +acceptable behavior and will take appropriate and fair corrective action in +response to any behavior that they deem inappropriate, threatening, offensive, +or harmful. + +## Scope + +This Code of Conduct applies within all community spaces, and also applies when +an individual is officially representing the community in public spaces. + +## Enforcement + +Instances of abusive, harassing, or otherwise unacceptable behavior may be +reported to the project maintainers responsible for enforcement at +**conduct@backblaze.com**. All complaints will be reviewed and investigated +promptly and fairly. All maintainers are obligated to respect the privacy and +security of the reporter of any incident. + +## Attribution + +This Code of Conduct is adapted from the [Contributor Covenant][homepage], +version 2.1, available at +https://www.contributor-covenant.org/version/2/1/code_of_conduct.html. + +[homepage]: https://www.contributor-covenant.org diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index c561287bc..2d15338d0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -19,6 +19,23 @@ In order to make it easier to contribute, core developers of this project: * checks if the documentation can be built properly * maintain other Continuous Integration tools (coverage tracker) +## Before you start + +Read [AGENTS.md](AGENTS.md): it maps the codebase, lists the local CI commands, and collects the problems +reviewers keep finding. Larger changes start from an intent in [`intent/`](intent/README.md) (problem → +spec → plan, each approved by a maintainer). Bug fixes with a linked issue can start from a failing test. + +Reviews follow [REVIEW.md](REVIEW.md). Releases follow [RELEASE.md](RELEASE.md). Report vulnerabilities +privately as described in [SECURITY.md](SECURITY.md). Everyone taking part follows the +[Code of Conduct](CODE_OF_CONDUCT.md). + +## Coding agents + +PRs written with coding agents are welcome on the same terms as any other PR. The person directing the +agent owns the PR: they read the diff, answer review findings, and fix what it breaks. Before opening the +PR, have a fresh context (`/code-review`, `/security-review` or a subagent that did not write the code) +review the diff, and say in the PR what it found. Agents never approve or merge PRs. + ## Versioning This package's versions adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html) and the versions are @@ -53,6 +70,10 @@ These files can either be created manually, or using `towncrier` e.g. `towncrier create` also takes care of duplicates automatically (if there is more than 1 news fragment of one type for a given github issue). +Each entry is one or more full sentences, starting with a capital letter and ending with a full stop, written +for SDK users. `nox -s towncrier_check` validates names and content locally. PRs that need no entry (for example +a lockfile-only change) are labelled `-changelog` by a maintainer. + ## Developer Info You'll need to have [nox](https://github.com/theacodes/nox) and [uv](https://docs.astral.sh/uv/) installed: @@ -105,6 +126,21 @@ make sure that `uv.lock` file reflects those changes by using `uv add`, `uv lock commands - see [documentation](https://docs.astral.sh/uv/). You can verify that the lock file is up to date by running `uv lock --check`. +## Local CI + +Run these before every push; they mirror the `lint`, `build` and `test` jobs of +[ci.yml](.github/workflows/ci.yml) on one Python version: + + nox -s lint + nox -s towncrier_check + nox -s unit-3.14 + nox -s build + nox -s doc + +`doc` needs `graphviz` and `plantuml` installed. CI also runs the unit tests on every supported Python and +OS, the integration tests with B2 credentials, and the B2 CLI test suite against this checkout +(`cli-compat`). + ## Linting To run all available linters: diff --git a/README.release.md b/README.release.md deleted file mode 100644 index 9ebbd8303..000000000 --- a/README.release.md +++ /dev/null @@ -1,3 +0,0 @@ -# Release Process - -- Run `nox -s make_release_commit -- X.Y.Z` where `X.Y.Z` is the version you're releasing, and follow the instructions diff --git a/RELEASE.md b/RELEASE.md new file mode 100644 index 000000000..9c3c67964 --- /dev/null +++ b/RELEASE.md @@ -0,0 +1,57 @@ +# Releasing b2sdk + +b2sdk is versioned from git tags (`hatch-vcs`): a release is a `release X.Y.Z` commit on `master` and a +`vX.Y.Z` tag on that commit. Pushing the tag runs [`cd.yml`](.github/workflows/cd.yml), which builds the +distribution, creates the GitHub release from the changelog section and uploads to PyPI. + +## What a release commit contains + +Every recent release (for example 2.13.2, `17a6be76`) changed exactly these, in one commit made by +`nox -s make_release_commit`: + +| File | Change | Done by | +| --- | --- | --- | +| `CHANGELOG.md` | New `## [X.Y.Z](...) - YYYY-MM-DD` section under the towncrier marker | `towncrier build` | +| `changelog.d/*` | Every released fragment deleted (`.gitkeep` stays) | `towncrier build` | + +No version string lives in the code; `b2sdk/version.py` reads it from the installed package. + +## Versioning + +Semantic versioning. The apiver layers (`b2sdk.v0`..`b2sdk.v3`) let a minor release add a new interface +without breaking the old ones, so a breaking change normally ships as a new apiver in a minor release, +not as a major bump. Pick the version from the fragments: any `added`, `changed`, `deprecated` or +`removed` → minor; only `fixed`, `doc` or `infrastructure` → patch. + +## Steps + +1. Make sure `master` is green (including `cli-compat`) and every PR meant for the release has merged. + Check the [Security tab](https://github.com/Backblaze/b2-sdk-python/security) for open Dependabot, + code scanning or secret scanning alerts that have a fix, and fix them first. +2. Preview the changelog: `uv run --group release towncrier build --draft --version X.Y.Z`. +3. Create the release commit from an up-to-date, clean `master`: + + ```bash + nox -s make_release_commit -- X.Y.Z + ``` + +4. Check the diff touches only `CHANGELOG.md` and `changelog.d/`, then follow the commands the session + prints: commit as `release X.Y.Z` and push it. +5. Wait for the CI workflow on that commit to pass. Only then tag and push the tag: + + ```bash + git tag vX.Y.Z + git push upstream vX.Y.Z + ``` + +6. Wait for the CD workflow to pass. Check the [GitHub release](https://github.com/Backblaze/b2-sdk-python/releases) + and [PyPI](https://pypi.org/project/b2sdk/) show X.Y.Z. +7. If the release fixes something the B2 CLI needs, open a PR in + [`B2_Command_Line_Tool`](https://github.com/Backblaze/B2_Command_Line_Tool) bumping its `b2sdk` bound. + +## Never + +- Tag a commit whose CI has not passed. +- Move or delete a published tag, or re-upload a version to PyPI. Release a new patch version instead. +- Release from a branch other than `master` without a reason recorded in the release PR or commit. +- Hand-edit a released `CHANGELOG.md` section without a reason recorded in the PR. diff --git a/REVIEW.md b/REVIEW.md new file mode 100644 index 000000000..83b77248b --- /dev/null +++ b/REVIEW.md @@ -0,0 +1,74 @@ +# Review instructions + +For human reviewers and review agents. Approval always comes from a maintainer; an agent's findings never +approve or block on their own, and the agent that wrote a change never reviews it. + +## What failure looks like here + +b2sdk fails by sending the wrong request to B2 (a dropped, extra or mis-shaped field), by breaking a caller +of an older apiver layer (`b2sdk.v0`..`b2sdk.v2`) or the B2 CLI, by retrying what it should not (or not +retrying what it should), by leaving resources behind (unfinished large files, keys, buckets in +integration tests), by hanging a transfer thread, or by leaking a credential into a log or exception. A +test that passes either way is also a failure: it claims coverage it does not give. + +## Before the passes + +1. Read the linked issue and, if present, `intent/<slug>/spec.md` and `plan.md`. +2. Check out the PR on current `master` and run the local CI commands from `AGENTS.md`. Say what you ran. +3. For each behavior the PR claims to fix or add, revert that line and confirm a test fails. Name any + behavior no test pins. + +## Passes + +Tag each finding with its pass. + +- **Bugs**: logic errors, off-by-one in ranges and part sizes, error paths that leak resources or + unfinished large files, threads or executors that are never shut down, shared state touched without the + lock that guards it. +- **Wire**: the request matches the B2 docs for the API version used; omitted vs `None` vs `[]` is + deliberate; `raw_simulator.py` models the same behavior as `raw_api.py`; retry, reauth and `Retry-After` + behave like sibling calls in `b2http.py`/`session.py`. +- **Security**: secrets (application keys, auth tokens, SSE-C keys) never logged, put in `repr` or in an + exception message; capability checks fail closed; `SqliteAccountInfo` files are not world-readable. +- **Compatibility**: every apiver layer keeps its signatures and return shapes; new public names are + exported only from the layers that should see them; `b2sdk._internal` stays private; deprecations warn + before removal; behavior the B2 CLI depends on (the `cli-compat` CI job) still holds. A breaking change + belongs only in a new apiver and needs a `changed`/`removed` fragment that says what callers must do. +- **Compliance**: the change matches the issue, `spec.md` and `plan.md`; the PR description matches the + code; a `changelog.d/` fragment exists, reads well for users and uses the right type; docstrings and + `doc/source` cover new public API (`doc_cover` passes); `AGENTS.md` is updated when the change makes it + stale or breaks a convention in it. + +## Severity + +- **Important**: breaks existing callers, sends a wrong request to B2, loses or leaks data or + credentials, hangs, or leaves resources behind. A claimed behavior with no test pinning it is Important. +- **Nit**: style, naming, comments. At most five per review; summarize the rest as a count. +- **Pre-existing**: a real bug this PR did not introduce. Report it so it is not lost, but it does not + block this PR; suggest an issue. + +## Evidence + +Every finding cites `file:line` and says what goes wrong, with the input or sequence that triggers it. +Do not infer behavior from a function's name; read it. If you could not verify something (no live B2, no +simulator), say so. + +## Converging + +- Round 1 reviews the whole diff. From round 3 on, review only the commits since your last review. +- Name the class of bug, not one instance, so the author fixes every occurrence in one round. +- Never repeat a finding already posted on the PR, resolved or not. +- After three rounds with Important findings still open, stop and ask the author to reconsider the + approach instead of posting more line comments. + +## Answering findings + +Every finding gets an answer before merge: fix it, or resolve the thread with a one-line reason +(false positive, intentional, out of scope with an issue link). Do not resolve a thread silently. + +## Format + +Start with a count (`2 Important, 1 Nit, 0 Pre-existing`, or `No issues found`), then what you checked +and against what (issue number, live B2 or simulator, commands run). Then a table sorted by severity: +`Severity | File:Line | Pass: finding`. Do not report generated files or what CI already enforces +(ruff, codespell, liccheck, towncrier naming). diff --git a/SECURITY.md b/SECURITY.md new file mode 100644 index 000000000..82eb0419d --- /dev/null +++ b/SECURITY.md @@ -0,0 +1,73 @@ +# Security Policy + +Security-response owner: Backblaze Security. Repository maintainers triage reports with them. + +## Supported Versions + +The latest release of b2sdk (currently `2.13.x`) receives security fixes. Every apiver layer +(`b2sdk.v0`..`b2sdk.v3`) inside that release is covered. Earlier releases are not patched in place; +upgrade to the newest release to receive fixes. See [`RELEASE.md`](RELEASE.md). + +| Version | Supported | +|---|---| +| Latest release (`2.13.x`) | Yes | +| Older releases | No | + +## Reporting a Vulnerability + +**Do not file a public GitHub issue for a security vulnerability.** + +Instead, report it privately via GitHub Security Advisories: + +1. Go to the [Security tab](https://github.com/Backblaze/b2-sdk-python/security/advisories/new) of this repository +2. Click **Report a vulnerability** +3. Provide a clear description, the affected version, reproduction steps, and impact assessment + +You can also email **security@backblaze.com** if you cannot use GitHub Security Advisories. + +## What to expect + +- Acknowledgement within 3 business days +- Initial assessment within 7 business days +- Coordinated disclosure once a fix is available, shipped as a patch release with a changelog entry and a + GitHub security advisory + +## Scope + +In scope: + +- Credential-handling flaws in `b2sdk`: application keys, authorization tokens or SSE-C keys exposed in + logs, exception messages, `repr`, or sent to unintended hosts +- Account-info storage: `SqliteAccountInfo` files created readable by other users, or credentials cached + where the caller did not ask +- Transport flaws: TLS verification bypassed, or requests sent outside the key's bucket or prefix scope +- Sync and download path handling that writes outside the destination folder +- Denial of service reachable through the public API, such as unbounded retries or thread leaks triggered + by server responses + +Out of scope: + +- Vulnerabilities in the Backblaze B2 service itself — report those to Backblaze directly +- Misuse of credentials by the calling application, such as embedding a master key in client code +- Issues requiring root or local access on the machine running the application + +## Security-relevant areas + +- **Credentials**: application keys go to `B2Api.authorize_account` and only to the B2 authorize + endpoint; the token returned is kept in the account-info store and sent in the `Authorization` header. +- **Account info**: `SqliteAccountInfo` creates its database with owner-only permissions before writing + credentials to it. +- **SSE-C keys**: customer-provided keys travel in request headers on uploads, downloads and copies, and + are not persisted. If you log requests, redact headers. +- **Transport**: all B2 API traffic uses HTTPS through `requests`, with certificates from `certifi`. +- **Tests and CI**: integration tests read `B2_TEST_APPLICATION_KEY_ID` and `B2_TEST_APPLICATION_KEY` from + repository secrets. The `.sdkharness` checks refuse any non-loopback endpoint and ambient `B2_*` + credentials. + +## Best practices for users + +- Use application keys scoped to specific buckets, prefixes and capabilities. +- Never embed an application key in code shipped to end users; issue download authorizations from a + backend instead. +- Rotate application keys regularly. +- Use SSE-B2 or SSE-C for encryption at rest, and Object Lock for compliance-sensitive data. diff --git a/changelog.d/+ai-native-sdlc.infrastructure.md b/changelog.d/+ai-native-sdlc.infrastructure.md new file mode 100644 index 000000000..664b80c73 --- /dev/null +++ b/changelog.d/+ai-native-sdlc.infrastructure.md @@ -0,0 +1 @@ +Add `AGENTS.md`, `CLAUDE.md`, `REVIEW.md`, `RELEASE.md`, `SECURITY.md`, `CODE_OF_CONDUCT.md`, issue forms, a PR template and `intent/` templates for the AI-native SDLC, with its skills and a ruff hook in `.claude/`. diff --git a/docs/review-lessons.md b/docs/review-lessons.md new file mode 100644 index 000000000..629643f02 --- /dev/null +++ b/docs/review-lessons.md @@ -0,0 +1,65 @@ +# Review lessons + +Problems that earlier reviews of b2-sdk-python found (or that follow-up PRs had to fix), grouped by theme, +with the PRs where they came up. Use it as a checklist before asking for review, and add a line when a +review finds something that will matter again. Rules that apply to every change are in +[AGENTS.md](../AGENTS.md). + +## Retries, reauth and upload tokens + +- Uploads are sent with `try_count=1`; `UploadManager` retries with a new upload URL and token. Retrying + inside `b2http` reuses a stale URL. (#522, #548, #556, #563, #618) +- A change to `post_content_return_json` affects every POST, not only the call you meant to fix. (#548) +- `Retry-After` may be seconds or an HTTP date; `int()` on the date form raises. Parse it through + `_parse_retry_after_seconds`, and treat all retryable 5xx alike. (#550, #574, #583, #619) + +## Tests + +- Revert the fix and the test must fail; show it in the PR. Good examples: #607, #620, #633. + (#535, #548, #576, #618, #619) +- Assert the mechanism (the sleep actually taken, the new upload URL fetched), not an exception attribute. + (#548, #618) +- Do not mask failures: no catch-all `except`, no generic `pytest.fail` hiding the cause, no unawaited + async tests. (#550, #566, #574, #623) + +## apiver layers and public interface + +- Export new public names from `b2sdk/v3` (`__init__.py`, `exception.py`, `testing/__init__.py`). + (#547, #557, #569, #618) +- Older layers must keep their own behavior and must not rely on v3-only abstractions. (#538, #541, #542, + #559, #581) +- Do not call `_private` members across classes; keep tunables as module-level constants; name parameters + the SDK way (`content_disposition`) and thread them up to `Bucket`. (#576, #581) +- Avoid names that collide in meaning with existing ones (`RequestTimeout` vs `B2RequestTimeout`). (#618) + +## Simulator parity + +- `RawSimulator` changes in the same PR as `AbstractRawApi` and `B2RawHTTPApi`. (#543, #548, #576, #604) + +## Portability + +- Windows: do not reopen a `NamedTemporaryFile` by name; handle permission and symlink cases; prefer + `stat.S_ISDIR` on `stat` results in `scan/folder.py`. (#525, #604, #607) +- New Python versions surface warnings and API changes first; run the newest one locally. (#515, #560) + +## Integration tests and live B2 + +- Use `bucket_manager.py` and a temporary account-info dir; clean up buckets and keys. (#547, #554, #557, + #564, #574) +- The live-B2 stage has known failures tracked in #614; reproduce on `master` before calling a failure + unrelated. (#611, #613, #618, #619, #620) + +## sdkharness + +- Checks target only loopback simulators with the fixed test credentials, refuse ambient `B2_*` values, + report FAIL (not SKIP) on real errors, and refuse a simulator that already served requests. + (#620, #623, #624, #628) + +## Changelog and PR hygiene + +- Every PR adds a towncrier fragment about the user-facing change. (#514, #522, #560, #576, #583) +- Search for open PRs first; one change per PR, against `master`. Duplicates and splits: #516/#517, + #527/#530, #551/#552, #576/#578/#579, #608/#615/#618, #609/#616/#617/#619. +- Behavior changes hidden in a fix (a new SSE default, rejecting an option) need their own fragment and + a reviewer's attention. (#604) +- Docs: a Sphinx or theme upgrade must be checked on Read the Docs, not only locally. (#567, #577, #582) diff --git a/intent/README.md b/intent/README.md new file mode 100644 index 000000000..0f4b0119b --- /dev/null +++ b/intent/README.md @@ -0,0 +1,65 @@ +# Change intents + +Each non-trivial change gets a folder `intent/<change-slug>/` holding three files, written in order. A +human approves each one before the next is written; the approval is a commit or a PR review, not an +agent's edit to the status line. + +| File | Written by | Approved by | Holds | +| --- | --- | --- | --- | +| `intent.md` | Originator (with Claude) | Maintainer | Problem, outcome, constraints, open questions. No design. | +| `spec.md` | Claude, from the intent | Maintainer; area owner for flagged concerns | Numbered requirements R1.., design, constraint table, concerns. | +| `plan.md` | Claude in plan mode | Engineer doing the work | Files, ordered steps citing R numbers, risks, proof. | + +Small fixes that already have a GitHub issue with a clear reproduction can skip `intent/` and start from a +failing test. Link the GitHub issue in the `intent.md` header when one exists, and the intent folder in the +issue. + +The `ai-native-sdlc` skill in `.claude/skills/` looks at what exists and routes to the next step. + +## Templates + +### intent.md + +```markdown +# Intent: <short title> +Author: <name>. Issue: #<n>. Status: draft. + +## Problem +## Proposed outcome +## Affected users, apiver layers and B2 calls +## Constraints +## Open questions +``` + +### spec.md + +```markdown +# Spec: <title> +From: intent.md (<author>). Status: ready for review. + +## Requirements +- R1. <testable statement: request sent to B2, returned value, exception raised, apiver layer> + +## Design +<which b2sdk/_internal modules change, what each b2sdk/vN exports, raw_api and raw_simulator changes> + +## How each constraint is met +| Constraint | Met by | +| --- | --- | + +## Areas of concern +1. <concern>. Owner: <@handle>. +``` + +### plan.md + +```markdown +# Plan: <title> + +## Files that change +## Order of work +1. <step> (R1). +## Risks +## Proof +- R1: <test name, apiver it runs on, and whether it fails when the change is reverted> +``` diff --git a/intent/review-process/intent.md b/intent/review-process/intent.md new file mode 100644 index 000000000..7976a04d3 --- /dev/null +++ b/intent/review-process/intent.md @@ -0,0 +1,80 @@ +# Intent: review and merge process scaled by risk +Author: Gonzalo Peña-Castellanos (with Claude). Issue: #634. Status: draft, for the b2-sdk-python maintainers. +Source: [How the Claude Code team does code review](https://claude.dev/blog/solving-code-review-at-anthropic/) +(claude.dev, 2026-10-15), applied to b2-sdk-python. + +## Problem + +More b2-sdk-python PRs are written with agents, and review has not scaled with them. Across the last 80 +PRs (#514 to #633): + +- 27 of 61 merged PRs got no review or comment from anyone but the author, including the whole + sdkharness series (#611 to #628). Most approvals carry no text, so they do not show what was checked. +- The same findings recur (see `docs/review-lessons.md`): missing changelog fragments, tests that do not + fail when the fix is reverted, retry logic in the wrong layer, missing apiver exports, simulator drift. +- Work is duplicated or split: #608/#615/#618 and #609/#616/#617/#619 fixed the same problems, and + #615 and #617 merged unreviewed into `development` before being redone against `master`. +- The live-B2 CI stage has known failures (#614), so every PR argues its red checks are unrelated, and + `nox -s lint` runs `ruff format` in write mode rather than `--check`, so lint never fails on formatting. +- Every PR gets the same attention whatever its risk; a docs fix and a change to `b2http.py` retries are + treated alike. +- Nothing measures whether a reviewer change (REVIEW.md, AGENTS.md, model) catches more or fewer bugs. + +PR for #634 covers the parts that need no settings change: AGENTS.md, REVIEW.md severities, evidence and +convergence rules, Risk and How-to-revert in the PR template, and a fresh-context self-review before a +PR opens. + +## Proposed outcome + +Human review time goes where the risk is, with no rise in reverts or post-release fixes: + +1. **Risk tiers set by the change, not the author.** A first proposal to accept or change: + + | Tier | Paths | Human approvals | + | --- | --- | --- | + | Low | `*.md`, `doc/`, `changelog.d/`, `intent/`, `test/` only, `.sdkharness/` docs | One maintainer, light review | + | Medium | `b2sdk/_internal/` (except below), `.sdkharness/tests/` | One maintainer | + | High | `b2http.py`, `session.py`, `raw_api.py`, `account_info/`, `encryption/`, any `b2sdk/vN/` export, `.github/workflows/`, `pyproject.toml`, `uv.lock`, release commits | Two maintainers, or one plus a green `cli-compat` run reviewed by hand | + + Authors outside the maintainer list always need a person. A bot's approval never fills a human slot. +2. **Approvals cover one commit.** New commits pushed after an approval need re-approval for Medium and + High tiers. +3. **Every review thread resolved before merge** (GitHub "require conversation resolution"). +4. **Automated AI review and security review on every PR**, following REVIEW.md, with open Important + findings blocking merge after a trial period. +5. **Release commits through a PR** instead of a direct push to `master`, so CI and a second person see + them before tagging. +6. **A green baseline**: fix or quarantine the known live-B2 failures (#614) and make `nox -s lint` check + formatting, so a red check means something. +7. **Past bugs become reviewer evals.** Build a set from bugs reviewers caught or that shipped, for + example #548 (retry change that would break every POST), #550 (TypeError on a date `Retry-After`), + #541 and #542 (v2 relying on v3 abstractions), #543 (simulator bucket-name mapping), #620 (harness + check that could reach production). Score the reviewer against it whenever REVIEW.md, AGENTS.md or the + model changes. +8. **Measure it.** Share of PRs merged with a substantive review, review rounds to pass, time a PR waits + on a person; reverts and post-release fix PRs as the guardrail. + +## Affected users and systems + +- Contributors and agents opening PRs; the maintainers who review them. +- Repo settings: branch protection or rulesets on `master`, conversation resolution, required checks. +- Workflows: `ci.yml` (lint formatting check), a new review workflow, possibly a risk-tier check. +- The B2 CLI, which depends on this SDK's behavior (`cli-compat`). +- Library users only indirectly: fewer regressions and faster fixes. + +## Constraints + +- Settings changes need a repo admin; this intent proposes them, it does not apply them. +- Separation of duties: the agent that wrote a change never approves it, and the person directing it + counts as one reviewer. +- Workflow changes keep the Dependabot guard (`github.actor` and `pull_request.user.login`). +- Start strict and loosen with evidence; any tier can be tightened if reverts rise. +- Cost of running an AI reviewer per push in CI. + +## Open questions + +- Who are the maintainers for approval purposes, and should a `CODEOWNERS` file name them per path? +- Do the tier paths above match where the risk is? +- Should the changelog validation step become a required check on `master`? +- Who owns fixing the #614 baseline, and should the `development` branch be retired? +- Where should the reviewer eval set live and run: this repo, or the central SDK harness? From 9388943355cea83b16ff09738844464c1c2d7a45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gonzalo=20Pe=C3=B1a-Castellanos?= <goanpeca@gmail.com> Date: Tue, 27 Oct 2026 22:31:33 -0500 Subject: [PATCH 2/4] docs: address Copilot review on gitignore, SSE-C and doc session --- .gitignore | 3 ++- AGENTS.md | 5 +++-- CONTRIBUTING.md | 2 +- SECURITY.md | 6 ++++-- 4 files changed, 10 insertions(+), 6 deletions(-) diff --git a/.gitignore b/.gitignore index e5ac53132..83524bf70 100644 --- a/.gitignore +++ b/.gitignore @@ -14,5 +14,6 @@ venv .vscode .pdm-build/ .pdm-python -.env.DS_Store +.env +.DS_Store .claude/settings.local.json diff --git a/AGENTS.md b/AGENTS.md index 381b83d45..f603448b8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -9,13 +9,14 @@ Keep it under about 150 lines and link out for detail. Tooling is `nox` + `uv` (`pip install nox uv`). Sessions are defined in `noxfile.py`. - Local CI, run before every push and paste the tail in the PR: - `nox -s lint`, `nox -s towncrier_check`, `nox -s unit-3.14`, `nox -s build`, `nox -s doc`. + `nox -s lint`, `nox -s towncrier_check`, `nox -s unit-3.14`, `nox -s build`, `nox --non-interactive -s doc`. - Format: `nox -s format` (ruff). Lint: `nox -s lint` (ruff, `test/static`, liccheck). - Unit tests run once per apiver: `nox -s unit-3.14` runs `pytest --api=v3`, then `v2`, `v1`, `v0`. By keyword: `nox -s unit-3.14 -- -k keyword`. One apiver: `uv run --group test pytest --api=v2 test/unit`. - Integration tests hit live B2 and need `B2_TEST_APPLICATION_KEY_ID` and `B2_TEST_APPLICATION_KEY`: `nox -s integration-3.14`. Leftover test buckets: `nox -s cleanup_old_buckets`. -- Docs: `nox -s doc` (needs `graphviz` and `plantuml`) also runs `doc_cover`, which fails on undocumented API. +- Docs: `nox --non-interactive -s doc` (needs `graphviz` and `plantuml`) also runs `doc_cover`, which fails on + undocumented API. Without `--non-interactive` in a terminal it starts a live-reload server instead. - Lock file: `uv lock --check`. Dependency changes go through `uv add`/`uv lock`, never by hand. - Changelog fragment: `uv run --group release towncrier create -c 'Text.' +slug.fixed.md`. - Release: [RELEASE.md](RELEASE.md) (`nox -s make_release_commit -- X.Y.Z`). diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2d15338d0..41bcc021e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -135,7 +135,7 @@ Run these before every push; they mirror the `lint`, `build` and `test` jobs of nox -s towncrier_check nox -s unit-3.14 nox -s build - nox -s doc + nox --non-interactive -s doc `doc` needs `graphviz` and `plantuml` installed. CI also runs the unit tests on every supported Python and OS, the integration tests with B2 credentials, and the B2 CLI test suite against this checkout diff --git a/SECURITY.md b/SECURITY.md index 82eb0419d..7ca812de4 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -57,8 +57,10 @@ Out of scope: endpoint; the token returned is kept in the account-info store and sent in the `Authorization` header. - **Account info**: `SqliteAccountInfo` creates its database with owner-only permissions before writing credentials to it. -- **SSE-C keys**: customer-provided keys travel in request headers on uploads, downloads and copies, and - are not persisted. If you log requests, redact headers. +- **SSE-C keys**: customer-provided keys travel in request headers on uploads and downloads, and as + `customerKey` fields in JSON request bodies on large-file start (`serverSideEncryption`) and server-side + copy (`sourceServerSideEncryption`, `destinationServerSideEncryption`). They are not persisted. If you + log requests, redact encryption keys in both headers and bodies. - **Transport**: all B2 API traffic uses HTTPS through `requests`, with certificates from `certifi`. - **Tests and CI**: integration tests read `B2_TEST_APPLICATION_KEY_ID` and `B2_TEST_APPLICATION_KEY` from repository secrets. The `.sdkharness` checks refuse any non-loopback endpoint and ambient `B2_*` From b1ddca1b4f3b067462533a0951e16a7abd0a330d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gonzalo=20Pe=C3=B1a-Castellanos?= <goanpeca@gmail.com> Date: Tue, 27 Oct 2026 22:38:57 -0500 Subject: [PATCH 3/4] docs: add review lessons from all PRs and code review practices --- .github/workflows/claude-review.yml | 64 ++++++ AGENTS.md | 64 ++++-- CONTRIBUTING.md | 5 + REVIEW.md | 44 +++- changelog.d/+ai-native-sdlc.infrastructure.md | 2 +- contrib/review_metrics.py | 131 +++++++++++ docs/review-lessons.md | 204 ++++++++++++++---- intent/review-process/evals.md | 25 +++ intent/review-process/intent.md | 39 +++- 9 files changed, 489 insertions(+), 89 deletions(-) create mode 100644 .github/workflows/claude-review.yml create mode 100644 contrib/review_metrics.py create mode 100644 intent/review-process/evals.md diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml new file mode 100644 index 000000000..cdbed9f62 --- /dev/null +++ b/.github/workflows/claude-review.yml @@ -0,0 +1,64 @@ +name: Claude review + +on: + pull_request: + types: [opened, reopened, synchronize, ready_for_review] + +permissions: + contents: read + pull-requests: write + id-token: write + +concurrency: + group: claude-review-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + code-review: + if: ${{ github.actor != 'dependabot[bot]' && github.event.pull_request.user.login != 'dependabot[bot]' && !github.event.pull_request.draft }} + timeout-minutes: 30 + runs-on: ubuntu-latest + env: + ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} + steps: + - name: Skip without an API key + if: ${{ env.ANTHROPIC_API_KEY == '' }} + run: echo "ANTHROPIC_API_KEY is not set (or this is a fork PR); skipping the Claude review." + - uses: actions/checkout@v4 + if: ${{ env.ANTHROPIC_API_KEY != '' }} + with: + fetch-depth: 0 + - uses: anthropics/claude-code-action@d70510b174a1244958df0b03166a54912811f3e6 # v1 + if: ${{ env.ANTHROPIC_API_KEY != '' }} + with: + anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }} + prompt: | + Review pull request #${{ github.event.pull_request.number }} in ${{ github.repository }}. + Follow REVIEW.md exactly, using AGENTS.md and docs/review-lessons.md as context. Review only + what changed since your last review on this PR after round 2, and never repeat a finding + already posted. Post each finding as an inline comment tagged Important, Nit or Pre-existing, + with file:line evidence. Then post one summary comment that starts with the count. + claude_args: | + --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Read,Grep,Glob" + + security-review: + if: ${{ github.actor != 'dependabot[bot]' && github.event.pull_request.user.login != 'dependabot[bot]' && !github.event.pull_request.draft }} + timeout-minutes: 30 + runs-on: ubuntu-latest + env: + ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} + steps: + - name: Skip without an API key + if: ${{ env.ANTHROPIC_API_KEY == '' }} + run: echo "ANTHROPIC_API_KEY is not set (or this is a fork PR); skipping the security review." + - uses: actions/checkout@v4 + if: ${{ env.ANTHROPIC_API_KEY != '' }} + with: + ref: ${{ github.event.pull_request.head.sha }} + fetch-depth: 2 + - uses: anthropics/claude-code-security-review@0c6a49f1fa56a1d472575da86a94dbc1edb78eda # main + if: ${{ env.ANTHROPIC_API_KEY != '' }} + with: + claude-api-key: ${{ secrets.ANTHROPIC_API_KEY }} + comment-pr: true + exclude-directories: doc,.claude,intent,changelog.d diff --git a/AGENTS.md b/AGENTS.md index f603448b8..d1b712a2e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -54,34 +54,46 @@ Tooling is `nox` + `uv` (`pip install nox uv`). Sessions are defined in `noxfile ## Things agents get wrong here -Each line was raised by a reviewer, or fixed by a follow-up PR, on at least two PRs. +Each line was raised by reviewers on at least two merged PRs (all 370 were read, #1 to #632) or fixed by a +follow-up PR. Detail and PR numbers in [docs/review-lessons.md](docs/review-lessons.md). 1. **Missing or wrong changelog fragment.** Every PR adds `changelog.d/<issue>.<type>.md` or `+slug.<type>.md` (types in `pyproject.toml`). Write it for SDK users, about the fix, not the test. 2. **Tests that do not pin the fix.** Revert the change and the new test must fail. Exercise the mechanism (the real sleep in `_translate_and_retry`, a new upload URL fetched after a 408), not an attribute. -3. **Retrying uploads in the HTTP layer.** Uploads go out with `try_count=1` so the error reaches - `UploadManager`, which gets a fresh upload URL and token. Do not retry them in `b2http.py`. -4. **Retry parity.** Treat every retryable 5xx alike, and parse `Retry-After` through - `_parse_retry_after_seconds` so a non-numeric value (an HTTP date) never raises. -5. **Missing apiver exports.** A new public name is exported from `b2sdk/v3` (`__init__.py`, - `exception.py`, `testing/__init__.py` as relevant); older layers must not pick up v3 behavior or - rely on v3-only abstractions. Run the unit tests on every `--api`. -6. **`raw_simulator.py` drift.** Any change to `AbstractRawApi` or a B2 call's behavior lands in - `RawSimulator` in the same PR. -7. **Accidental public interface.** Do not call another class's `_private` members; keep tunables as - module-level `_CONSTANTS`, not new parameters; use SDK-style names and thread new options up to `Bucket`. -8. **Hidden failures.** No catch-all `except`, no `pytest.fail` that drops the real error, no async test - that is never awaited. A skipped check is NOT RUN, never PASS. -9. **Portability.** Windows and new Pythons: do not reopen a `NamedTemporaryFile` by name; handle - permissions and symlinks; check `scan/` on Windows paths. -10. **Integration tests without the helpers.** Use `bucket_manager.py` (seeded names, retries, cleanup) and - a temporary account-info dir; never leave buckets or keys behind. -11. **sdkharness safety.** Checks target only `http://127.0.0.1:<port>` with `test-key-id`/`test-key`; + Server-format fixes get a simulator unit test and an integration test. +3. **Reordered or inserted positional parameters.** New optional parameters go last (or keyword-only); + never reorder. Callers pass many positionals (`Bucket.upload_bytes`), and a shift is silent. +4. **apiver layering.** Change only the unreleased layer; a released `b2sdk/vN` keeps its behavior through + a wrapper in `b2sdk/vN/` that forwards `*args, **kwargs`. Export new public names from `b2sdk/v3` + (`__init__.py`, `exception.py`, `testing/__init__.py`). Compat shims kept for the CLI get existence and + signature tests. Run the unit tests on every `--api`. +5. **Raw API drift.** `AbstractRawApi`, `B2RawHTTPApi` and `RawSimulator` change together with matching + signatures and argument names. Check live B2 before changing simulator behavior; keep it deterministic + and bounded in memory. No `requests` types in the raw API interface. +6. **Retries in the wrong place.** Uploads go out with `try_count=1` so `UploadManager` retries with a + fresh upload URL and token; never retry them in `b2http.py`. Treat every retryable 5xx alike and parse + `Retry-After` through `_parse_retry_after_seconds`. +7. **Accidental public interface.** Do not call another class's `_private` members. Tunables are named + constants: module-level, or a class attribute (`DEFAULT_*_CLASS`, `B2Http.TIMEOUT`) when a subclass or + apiver layer may override it. Logic lives in `B2Api` and `Bucket` delegates; use `_by_id`/`_by_name`. +8. **Hidden failures.** No bare `Exception` (subclass in `exception.py`), no catch-all `except`, no + `pytest.fail` that drops the real error, no unawaited async test. A skipped check is NOT RUN, not PASS. +9. **Undocumented public API.** Every new public class gets a docstring and every new parameter a `:param`, + with units and whether ranges are inclusive. `doc_cover` fails on the first, reviewers on the second. +10. **Sync and scan safety.** Reject `..` traversal, decide absolute paths per platform, allow names legal on + the OS, and never silently drop user files. Windows: do not reopen a `NamedTemporaryFile` by name. +11. **Large files and transfers.** A new upload option flows through emerger, executor and the + unfinished-file matcher, and a mismatch must not resume. Cover zero length, the 10,000-part limit and + >5 GB. Thread pools: lazy init and `get` share a lock; one `max_workers` default in one place. +12. **Test and integration hygiene.** `parametrize`, not loops of asserts; no nested functions (use + `_method`); `@pytest.mark.apiver` only where behavior differs. Integration tests use + `bucket_manager.py` and never touch real account info or leave buckets behind. +13. **sdkharness safety.** Checks target only `http://127.0.0.1:<port>` with `test-key-id`/`test-key`; never read or print ambient `B2_*` credentials. -12. **Duplicate and split PRs.** Search open PRs first; one change per PR, against `master`. -13. **Claiming green over a red CI.** The live-B2 stage has known failures (#614). Reproduce on `master` - and cite the issue; never call a failure "unrelated" without showing it. +14. **PR scope.** Search open PRs first; one change per PR, against `master`; no unrelated reformatting; + file an issue for out-of-scope bugs. The live-B2 stage has known failures (#614): reproduce on `master` + before calling a failure unrelated. ## Workflow (AI-native SDLC) @@ -93,9 +105,15 @@ Each line was raised by a reviewer, or fixed by a follow-up PR, on at least two every Python file Claude edits. - Before opening a PR, review your own diff in a fresh context (`/code-review` and `/security-review`, or a subagent that did not write the code) and fix or answer what it finds. Say in the PR what it reported. +- Every non-draft PR gets an automated Claude review and security review (`claude-review.yml`, once the + `ANTHROPIC_API_KEY` secret is set). `python contrib/review_metrics.py` reports human review time. - Review: [REVIEW.md](REVIEW.md). Every finding gets fixed or resolved with a reason. Agents never approve or merge their own PRs; a maintainer approves. -- The person directing an agent owns its PR: they count as one of its reviewers and fix what it breaks. +- The person directing an agent owns its PR: they count as one of its reviewers and fix what it breaks + ("you land it, you own it"). Keep PRs small so they are easy to review and revert. +- Before changing an unfamiliar area, ask Claude for an explainer of that slice of the code as it is today. +- When reviewers flag the same thing twice, fix it at the source in the same PR: this file, `REVIEW.md`, + a skill, or a lint rule or test. - Stacked PRs: state the base PR and merge order in the description; retarget to `master` after it merges. - Releases: [RELEASE.md](RELEASE.md). Security reports: [SECURITY.md](SECURITY.md). diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 41bcc021e..05eea2e01 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -36,6 +36,11 @@ agent owns the PR: they read the diff, answer review findings, and fix what it b PR, have a fresh context (`/code-review`, `/security-review` or a subagent that did not write the code) review the diff, and say in the PR what it found. Agents never approve or merge PRs. +If you doubt a change should be made at all, say so early on the issue or a draft PR, not as a blocking +comment on a finished one. Whoever lands a change owns fixing it if it breaks something; keep changes +small so they are easy to review and revert. `python contrib/review_metrics.py` reports how much human +review merged PRs got. + ## Versioning This package's versions adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html) and the versions are diff --git a/REVIEW.md b/REVIEW.md index 83b77248b..72ab1f693 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -13,7 +13,9 @@ test that passes either way is also a failure: it claims coverage it does not gi ## Before the passes -1. Read the linked issue and, if present, `intent/<slug>/spec.md` and `plan.md`. +1. Read the linked issue and, if present, `intent/<slug>/spec.md` and `plan.md`. Ask: does the change + solve its problem, fit the system, and leave everything else working? On a bug fix, find the root + cause and check the fix is there, not at a symptom. 2. Check out the PR on current `master` and run the local CI commands from `AGENTS.md`. Say what you ran. 3. For each behavior the PR claims to fix or add, revert that line and confirm a test fails. Name any behavior no test pins. @@ -24,13 +26,18 @@ Tag each finding with its pass. - **Bugs**: logic errors, off-by-one in ranges and part sizes, error paths that leak resources or unfinished large files, threads or executors that are never shut down, shared state touched without the - lock that guards it. + lock that guards it, a new upload option the unfinished-file matcher ignores, scan or sync silently + dropping user files. - **Wire**: the request matches the B2 docs for the API version used; omitted vs `None` vs `[]` is deliberate; `raw_simulator.py` models the same behavior as `raw_api.py`; retry, reauth and `Retry-After` - behave like sibling calls in `b2http.py`/`session.py`. + behave like sibling calls in `b2http.py`/`session.py`; the B2 transaction class (and cost) of new calls + is deliberate (copy is class C). - **Security**: secrets (application keys, auth tokens, SSE-C keys) never logged, put in `repr` or in an - exception message; capability checks fail closed; `SqliteAccountInfo` files are not world-readable. -- **Compatibility**: every apiver layer keeps its signatures and return shapes; new public names are + exception message or request body logs; capability checks fail closed; `SqliteAccountInfo` files are + not world-readable; sync rejects `..` traversal; new dependencies (including test and dev ones) are + vetted. +- **Compatibility**: every apiver layer keeps its signatures and return shapes; new optional parameters + are last or keyword-only, never inserted or reordered; compat shims have signature tests; new public names are exported only from the layers that should see them; `b2sdk._internal` stays private; deprecations warn before removal; behavior the B2 CLI depends on (the `cli-compat` CI job) still holds. A breaking change belongs only in a new apiver and needs a `changed`/`removed` fragment that says what callers must do. @@ -39,6 +46,23 @@ Tag each finding with its pass. `doc/source` cover new public API (`doc_cover` passes); `AGENTS.md` is updated when the change makes it stale or breaks a convention in it. +## Effort by risk + +Match the depth of review to what a missed bug would cost: + +- **Quick**: docs, `changelog.d/`, `intent/`, test-only and `.claude/` changes. Check claims and links. +- **Standard**: ordinary changes in `b2sdk/_internal/`. All passes on the whole diff. +- **Deep**: `b2http.py`, `session.py`, `raw_api.py`, `account_info/`, `encryption/`, `transfer/`, + `sync/`, any `b2sdk/vN/` change, `pyproject.toml`, `uv.lock`, `.github/workflows/`. All passes, run the + tests on every `--api`, and say what the checks would miss. These always need a maintainer. + +## Skip + +Do not review generated or vendored content: `uv.lock` hunks (review only the `pyproject.toml` change +that caused them), `CHANGELOG.md` release sections, `.claude/skills/` copied from the playbook, and +`doc/source` output. Do not report what CI already enforces (ruff, codespell, liccheck, license headers, +towncrier naming, `doc_cover`). + ## Severity - **Important**: breaks existing callers, sends a wrong request to B2, loses or leaks data or @@ -64,11 +88,15 @@ simulator), say so. ## Answering findings Every finding gets an answer before merge: fix it, or resolve the thread with a one-line reason -(false positive, intentional, out of scope with an issue link). Do not resolve a thread silently. +(false positive, intentional, out of scope with an issue link). Do not resolve a thread silently. Start +the reply with one of `Fixed`, `Wrong`, `Intentional` or `Won't fix`, so reviewer noise can be measured +(see `intent/review-process/intent.md`). + +When the same finding appears on a second PR, add it to `docs/review-lessons.md` and, if it is a rule, +to `AGENTS.md`, or turn it into a lint rule or test. ## Format Start with a count (`2 Important, 1 Nit, 0 Pre-existing`, or `No issues found`), then what you checked and against what (issue number, live B2 or simulator, commands run). Then a table sorted by severity: -`Severity | File:Line | Pass: finding`. Do not report generated files or what CI already enforces -(ruff, codespell, liccheck, towncrier naming). +`Severity | File:Line | Pass: finding`. When there is nothing to fix, say so plainly. diff --git a/changelog.d/+ai-native-sdlc.infrastructure.md b/changelog.d/+ai-native-sdlc.infrastructure.md index 664b80c73..5bd110a03 100644 --- a/changelog.d/+ai-native-sdlc.infrastructure.md +++ b/changelog.d/+ai-native-sdlc.infrastructure.md @@ -1 +1 @@ -Add `AGENTS.md`, `CLAUDE.md`, `REVIEW.md`, `RELEASE.md`, `SECURITY.md`, `CODE_OF_CONDUCT.md`, issue forms, a PR template and `intent/` templates for the AI-native SDLC, with its skills and a ruff hook in `.claude/`. +Add `AGENTS.md`, `CLAUDE.md`, `REVIEW.md`, `RELEASE.md`, `SECURITY.md`, `CODE_OF_CONDUCT.md`, issue forms, a PR template, `intent/` templates and review lessons from every merged PR for the AI-native SDLC, with its skills and a ruff hook in `.claude/`, an automated Claude code and security review workflow, and `contrib/review_metrics.py`. diff --git a/contrib/review_metrics.py b/contrib/review_metrics.py new file mode 100644 index 000000000..a443fac6d --- /dev/null +++ b/contrib/review_metrics.py @@ -0,0 +1,131 @@ +###################################################################### +# +# File: contrib/review_metrics.py +# +# Copyright 2026 Backblaze Inc. All Rights Reserved. +# +# License https://www.backblaze.com/using_b2_code.html +# +###################################################################### +""" +Report how much human review merged PRs got, and how often changes were reverted. + +Usage: python contrib/review_metrics.py [--repo OWNER/NAME] [--days 30] + +Needs an authenticated `gh` CLI. Read-only. Bots (logins ending in `[bot]` or starting with +`copilot`) never count as human reviewers; the PR author never counts as their own reviewer. +""" + +from __future__ import annotations + +import argparse +import datetime +import json +import statistics +import subprocess + +QUERY = """ +query($q: String!, $endCursor: String) { + search(query: $q, type: ISSUE, first: 50, after: $endCursor) { + pageInfo { hasNextPage endCursor } + nodes { + ... on PullRequest { + number title createdAt mergedAt author { login } + reviews(first: 100) { nodes { author { login } state submittedAt } } + comments(first: 100) { nodes { author { login } } } + reviewThreads(first: 100) { nodes { comments(first: 1) { nodes { author { login } } } } } + } + } + } +} +""" + + +def _is_bot(login: str | None) -> bool: + return not login or login.endswith('[bot]') or login.lower().startswith('copilot') + + +def _parse_time(value: str) -> datetime.datetime: + return datetime.datetime.fromisoformat(value.replace('Z', '+00:00')) + + +def _fetch(repo: str, days: int) -> list[dict]: + since = (datetime.date.today() - datetime.timedelta(days=days)).isoformat() + search = f'repo:{repo} is:pr is:merged merged:>={since}' + out = subprocess.run( + ['gh', 'api', 'graphql', '--paginate', '-f', f'query={QUERY}', '-f', f'q={search}'], + check=True, + capture_output=True, + text=True, + ).stdout + decoder = json.JSONDecoder() + pages, index = [], 0 + while index < len(out): + if out[index].isspace(): + index += 1 + continue + page, index = decoder.raw_decode(out, index) + pages.append(page) + return [node for page in pages for node in page['data']['search']['nodes'] if node] + + +def _percentile(values: list[float], fraction: float) -> float: + ordered = sorted(values) + return ordered[min(len(ordered) - 1, int(fraction * len(ordered)))] + + +def main() -> None: + parser = argparse.ArgumentParser(description=__doc__.splitlines()[1]) + parser.add_argument('--repo', default='Backblaze/b2-sdk-python') + parser.add_argument('--days', type=int, default=30) + args = parser.parse_args() + + prs = _fetch(args.repo, args.days) + if not prs: + print(f'No PRs merged in {args.repo} in the last {args.days} days.') + return + + approvals, comments, waits, rounds = [], [], [], [] + unreviewed = 0 + for pr in prs: + author = (pr['author'] or {}).get('login') + reviews = [ + r + for r in pr['reviews']['nodes'] + if not _is_bot((r['author'] or {}).get('login')) and r['author']['login'] != author + ] + approvers = {r['author']['login'] for r in reviews if r['state'] == 'APPROVED'} + human_comments = [ + c + for c in pr['comments']['nodes'] + + [t['comments']['nodes'][0] for t in pr['reviewThreads']['nodes']] + if not _is_bot((c['author'] or {}).get('login')) and c['author']['login'] != author + ] + approvals.append(len(approvers)) + comments.append(len(human_comments)) + rounds.append(len(reviews)) + if reviews: + first = min(_parse_time(r['submittedAt']) for r in reviews if r['submittedAt']) + waits.append((first - _parse_time(pr['createdAt'])).total_seconds() / 3600) + else: + unreviewed += 1 + + reverts = [pr for pr in prs if pr['title'].lower().startswith('revert')] + print(f'{args.repo}: {len(prs)} PRs merged in the last {args.days} days') + print( + f' human approvals per PR: median {statistics.median(approvals)}, ' + f'p95 {_percentile(approvals, 0.95)}' + ) + print(f' human comments per PR: median {statistics.median(comments)}') + print(f' human review submissions per PR (rounds): mean {statistics.mean(rounds):.2f}') + print(f' merged with no human review: {unreviewed} ({unreviewed / len(prs):.0%})') + if waits: + print( + f' hours to first human review: median {statistics.median(waits):.1f}, ' + f'p95 {_percentile(waits, 0.95):.1f}' + ) + print(f' reverts (guardrail): {len(reverts)} {[pr["number"] for pr in reverts]}') + + +if __name__ == '__main__': + main() diff --git a/docs/review-lessons.md b/docs/review-lessons.md index 629643f02..7d671013c 100644 --- a/docs/review-lessons.md +++ b/docs/review-lessons.md @@ -1,65 +1,175 @@ # Review lessons -Problems that earlier reviews of b2-sdk-python found (or that follow-up PRs had to fix), grouped by theme, -with the PRs where they came up. Use it as a checklist before asking for review, and add a line when a -review finds something that will matter again. Rules that apply to every change are in -[AGENTS.md](../AGENTS.md). +What reviewers of b2-sdk-python have asked for, grouped by theme, with the PRs where it came up. Use it +as a checklist before asking for review, and add a line when a review finds something that will matter +again. The rules that apply to every change are in [AGENTS.md](../AGENTS.md); the review process is in +[REVIEW.md](../REVIEW.md). -## Retries, reauth and upload tokens +## Where this comes from -- Uploads are sent with `try_count=1`; `UploadManager` retries with a new upload URL and token. Retrying - inside `b2http` reuses a stale URL. (#522, #548, #556, #563, #618) -- A change to `post_content_return_json` affects every POST, not only the call you meant to fix. (#548) -- `Retry-After` may be seconds or an HTTP date; `int()` on the date form raises. Parse it through - `_parse_retry_after_seconds`, and treat all retryable 5xx alike. (#550, #574, #583, #619) +All 370 merged PRs (#1 to #632) were read, with every inline comment, review body and conversation comment +from people other than the author (528 comments on 120 PRs; bots excluded; Copilot had not reviewed any +merged PR). About 90% of that review content is on PRs before #514. Lessons marked *(follow-up)* come +instead from later PRs that had to fix or redo an earlier one, or from closed duplicates, not from a +review comment. Rules that no longer apply (Python 2, yapf, Travis, absolute-imports-only) are left out. -## Tests +## apiver layers and public interface -- Revert the fix and the test must fail; show it in the PR. Good examples: #607, #620, #633. - (#535, #548, #576, #618, #619) -- Assert the mechanism (the sleep actually taken, the new upload URL fetched), not an exception attribute. - (#548, #618) -- Do not mask failures: no catch-all `except`, no generic `pytest.fail` hiding the cause, no unawaited - async tests. (#550, #566, #574, #623) +- Breaking changes go only into the unreleased apiver. A released `b2sdk/vN` keeps its old signature + through a wrapper in `b2sdk/vN/`. (#229, #297, #456) +- Subclass the next layer explicitly (`class B2Api(v1.B2Api)`); wrappers forward `*args, **kwargs` so new + arguments reach every layer. (#43, #397) +- Swap internal classes in wrappers through `DEFAULT_*_CLASS` class attributes, never `mock.patch`. + Helpers that exist only for an old layer live in `b2sdk/vN/`, not `_internal`. (#297) +- Compat methods kept for the B2 CLI (for example v2 `set_thread_pool_size`) get existence and signature + tests and must not call private upstream members. (#297) +- Export new names, including exceptions, from `b2sdk/v3`. (#618; follow-up #547, #557, #569) +- Older layers must not depend on v3-only abstractions. *(follow-up #538, #541, #542, #559, #581)* +- Docs and docstrings reference `b2sdk.vN.Name`, never `_internal` paths; document differences between + layers (for example the v1 `list_keys` cap). (#152, #156, #270) +- Decide deliberately whether a new helper is public. (#398, #447) + +## Backward compatibility and deprecation + +- New optional parameters go last, or keyword-only; never reorder, including constructors of public value + classes like `FileVersionInfo`. (#233, #297, #383) +- Changing a positional signature is breaking: new apiver plus a wrapper in the old one. (#456) +- Deprecate with the `version_utils` decorators before removing. (#26, #28, #172) +- A behavior change that can drop user data (for example a symlink depth limit in scan) is Important, + never silent. (#390) +- A new default (for example SSE-B2) or a newly rejected option is a behavior change with its own + changelog entry. *(follow-up #604)* + +## Changelog + +- One fragment per PR, with the right type, named for the issue when there is one. (#120, #127, #132, + #163, #213, #293, #303, #459, #514) +- Describe the user-visible change, not the test. (#560) +- Dependency pins added for a security report still need a fragment. (#429) -## apiver layers and public interface +## Tests -- Export new public names from `b2sdk/v3` (`__init__.py`, `exception.py`, `testing/__init__.py`). - (#547, #557, #569, #618) -- Older layers must keep their own behavior and must not rely on v3-only abstractions. (#538, #541, #542, - #559, #581) -- Do not call `_private` members across classes; keep tunables as module-level constants; name parameters - the SDK way (`content_disposition`) and thread them up to `Bucket`. (#576, #581) -- Avoid names that collide in meaning with existing ones (`RequestTimeout` vs `B2RequestTimeout`). (#618) +- Cover every new branch and edge case. (#120, #128, #141, #383, #388) +- Server-format fixes get a simulator unit test and an integration test. (#299, #301, #303) +- Drive the real mechanism: the sleep taken, the new upload URL fetched. Revert the fix and the test must + fail. (#618, #619) +- `@pytest.mark.apiver` only on tests whose behavior differs by apiver; update `v0`/`v1` tests or move + the test to `v_all`. (#90, #172, #253, #361) +- `parametrize` instead of loops of asserts; no redundant asserts, duplicated fixtures or debug leftovers; + check the test calls the function its name says. (#310, #315, #352, #383, #388, #390) +- Tests never touch the developer's real account info or environment. (#314, #447) +- Do not hide failures: no catch-all `except`, no generic `pytest.fail`, no unawaited async tests. + *(follow-up #550, #566, #623)* +- Review new test dependencies as supply-chain risk. (#512) + +## Raw API and RawSimulator + +- `AbstractRawApi`, `B2RawHTTPApi` and `RawSimulator` keep matching signatures and argument names + (`bucket_id` vs `bucket_name`). (#89, #172) +- Fix simulator bugs in the PR that needs them, after checking what live B2 does. (#120) +- The simulator stays deterministic (seeded) and bounded in memory. (#89, #141) +- No `requests` types in the raw API interface; transport details stay in `b2http`. (#172, #279) +- Raw-layer knowledge (default token types) stays in the raw layer. (#84) + +## Retries, timeouts and upload tokens + +- Timeouts are named constants on `B2Http`, not exactly the server's timeout. (#33, #329) +- Uploads retry in `UploadManager` with a fresh URL and token, never in `b2http`; one token serves one + upload at a time. (#89, #618) +- Parse `Retry-After` safely (it may be an HTTP date) and treat all retryable 5xx alike. (#619) +- Do not build a new `random.Random` per call. (#329) +- Key-change detection must keep clearing caches between master and non-master keys. (#184) +- Verify the downloaded length against the expected size. (#32) + +## Threading and transfer managers + +- Lazy pool creation and `get` take the same lock; `super()` chains cooperatively. (#297) +- Each default worker count is set once, named `max_workers`; think through `workers=1` and how sync and + download pools add up. (#297, #352) +- Validate worker and stream counts; keep per-thread memory O(1). (#352, #359) + +## Large files, copy and encryption + +- Document whether range ends are inclusive. (#55) +- Zero-length files and copies; copy (class C transaction) vs re-upload. (#165) +- Respect `PART_COUNT_LIMIT` (10,000) and replan; files over 5 GB with one worker; use 200 vs 206 status, + not `Content-Range`. (#32, #247, #297, #309) +- A new upload option flows through emerger, executor and the unfinished-file matcher; a mismatch must + not resume that file. (#383, #388) +- Expose the verified flag next to a checksum. (#153, #299) +- Bucket lookups never confuse ids with names. (#229) + +## Sync, scan and paths + +- Reject `..` traversal; decide which absolute paths are allowed per platform; allow names legal on the + current OS (backslashes on Linux). (#125) +- Scan policies receive a `FileVersion`, not a raw mtime; comment why mtime is rounded. (#128, #132) +- Document that bucket-to-bucket sync copies only the newest version. (#165) +- Use `pathlib` in new code; iterate directories lazily. (#235, #390) +- Windows: do not reopen a `NamedTemporaryFile` by name; handle permissions and symlinks. + *(follow-up #525, #604, #607)* + +## Account info + +- Follow `platformdirs` semantics for XDG paths; keep helpers private. (#235, #447) +- `SqliteAccountInfo` creates its file owner-only before writing credentials. + +## Exceptions, docstrings and typing + +- Never raise bare `Exception`; subclass in `exception.py` and override `__str__`; never swallow errors. + (#89, #125, #172, #270, #550) +- Every new class gets a docstring and every new parameter a `:param` with units; prefer type hints over a + duplicate `:rtype:`; drop docstrings that add nothing. (#52, #55, #89, #165, #172, #270, #297, #361, #383) +- A "why" comment on non-obvious branches. (#132, #141, #234, #270) +- Type touched signatures; `Optional` when the default is `None`; abstract collection types; no return + annotation on `__init__`. (#172, #297, #329, #390, #475) + +## Naming and structure + +- Precise names (`parameter_name`, not `field_name`); one meaning per parameter name. (#52, #329, #618) +- `_by_id`/`_by_name` pairs; logic in `B2Api`, `Bucket` delegates in one line. (#172) +- Class constants and factories (`self.BUCKET_FACTORY`) so subclasses can override; module-level + constants otherwise. (#33, #270, #297, #299, #390) +- No nested functions; use private methods. Compile regexes at module level; module-level loggers. + (#125, #172, #297, #390) +- Parse opaque server strings into typed objects; duck typing in `__eq__`. (#90, #220) -## Simulator parity +## Integration tests and live B2 -- `RawSimulator` changes in the same PR as `AbstractRawApi` and `B2RawHTTPApi`. (#543, #548, #576, #604) +- Use `bucket_manager.py` and a temporary account-info dir; clean up buckets and keys. + *(follow-up #547, #554, #557, #564)* +- Do not depend on account-gated features (custom upload timestamps); check values round-trip. (#383) +- Integration tests cannot run from forks (no secrets); say what you ran. (#172) +- The live-B2 stage has known failures (#614); reproduce on `master` before calling one unrelated. -## Portability +## sdkharness -- Windows: do not reopen a `NamedTemporaryFile` by name; handle permission and symlink cases; prefer - `stat.S_ISDIR` on `stat` results in `scan/folder.py`. (#525, #604, #607) -- New Python versions surface warnings and API changes first; run the newest one locally. (#515, #560) +- Checks target only loopback simulators with fixed test credentials, refuse ambient `B2_*` values, + report FAIL (not SKIP) on real errors, and refuse a used simulator. *(follow-up #620, #623, #624, #628; + no merged sdkharness PR had a review)* -## Integration tests and live B2 +## CI, nox and packaging -- Use `bucket_manager.py` and a temporary account-info dir; clean up buckets and keys. (#547, #554, #557, - #564, #574) -- The live-B2 stage has known failures tracked in #614; reproduce on `master` before calling a failure - unrelated. (#611, #613, #618, #619, #620) +- Shell steps keep the exit status. (#35) +- Ship only what users need in the sdist; run the import test against the build. (#154, #337) +- Environment markers on backports; check what nox `reuse_venv` does before changing it. (#277, #561) +- Keep the CD release steps working when changing actions. (#392) +- A Sphinx or theme upgrade is checked on Read the Docs, not only locally. *(follow-up #567, #577, #582)* -## sdkharness +## PR process + +- Self-review before asking for review. (#383, #390) +- Split PRs that are too large to review (one at 2,000 lines was "barely reviewable"). (#184, #221) +- No unrelated reformatting; file an issue for out-of-scope bugs; squash noisy histories. + (#125, #165, #297, #412, #512) +- Search for duplicates first; one change per PR, against `master`. *(follow-up #576/#578/#579, + #608/#615/#618, #609/#616/#617/#619)* -- Checks target only loopback simulators with the fixed test credentials, refuse ambient `B2_*` values, - report FAIL (not SKIP) on real errors, and refuse a simulator that already served requests. - (#620, #623, #624, #628) +## Candidates for automation -## Changelog and PR hygiene +Checks reviewers keep making that a tool could make instead (see `intent/review-process/intent.md`): -- Every PR adds a towncrier fragment about the user-facing change. (#514, #522, #560, #576, #583) -- Search for open PRs first; one change per PR, against `master`. Duplicates and splits: #516/#517, - #527/#530, #551/#552, #576/#578/#579, #608/#615/#618, #609/#616/#617/#619. -- Behavior changes hidden in a fix (a new SSE default, rejecting an option) need their own fragment and - a reviewer's attention. (#604) -- Docs: a Sphinx or theme upgrade must be checked on Read the Docs, not only locally. (#567, #577, #582) +- A per-apiver signature snapshot test: catches reordered parameters, broken wrappers and missing exports. +- A raw API parity test across `AbstractRawApi`, `B2RawHTTPApi` and `RawSimulator`. +- ruff rules beyond `E`, `F`, `I`, `UP`: `BLE001`, `TRY002`, `B`, `PT006`/`PT007`, `T20`, `D417`. +- A changelog check scoped to `b2sdk/` changes. diff --git a/intent/review-process/evals.md b/intent/review-process/evals.md new file mode 100644 index 000000000..bbc1a8ad8 --- /dev/null +++ b/intent/review-process/evals.md @@ -0,0 +1,25 @@ +# Reviewer eval set (seed) + +Past changes where a bug shipped or nearly shipped, for scoring an automated reviewer. Each case is a +diff the reviewer should flag. A maintainer who knows the code confirms the bug and where it lived before +a case counts. Re-score the reviewer against the confirmed set whenever `REVIEW.md`, `AGENTS.md`, the +model or the review workflow changes; keep a change only if the score does not drop. Add a case for every +revert or post-release fix PR. + +Status: candidates, not yet confirmed. + +| Case | Diff to review | Bug the reviewer should find | Fixed or caught in | Confirmed by | +| --- | --- | --- | --- | --- | +| Retry-After as HTTP date | the change that introduced `int(retry_after)` | `int()` raises `TypeError`/`ValueError` on a date-form `Retry-After` | #550 | | +| Retry in `post_content_return_json` | first version of #548 | a retry change in the shared POST path alters every POST, not only uploads | review of #548 | | +| Upload retried in HTTP layer | #615 | uploads retried with the same URL and token instead of in `UploadManager` | #618 | | +| v2 uses v3 abstractions | the change before #541 | an older apiver depends on v3-only classes | #541, #542 | | +| Simulator bucket mapping | the change before #543 | `RawSimulator` maps bucket names differently from B2 | #543 | | +| Positional misalignment | first version of #297 | new parameter inserted mid-signature shifts callers' positional arguments | review of #297 | | +| Upload option vs unfinished file | first version of #383 | a new upload option is ignored when matching an unfinished large file to resume | review of #383 | | +| Path traversal in sync | first version of #125 | `..` in a B2 file name escapes the destination folder | review of #125 | | +| Silent file drop in scan | first version of #390 | a symlink depth limit silently skips user files | review of #390 | | +| Harness reaches production | the change before #620 | the health check accepts ambient `B2_TEST_APPLICATION_KEY*` and could hit production | #620 | | + +Also run the reviewer on known-clean merged PRs and count findings that would have led someone to make a +harmful change (the noise rate). Track rounds-to-pass alongside recall. diff --git a/intent/review-process/intent.md b/intent/review-process/intent.md index 7976a04d3..f0c30fe6d 100644 --- a/intent/review-process/intent.md +++ b/intent/review-process/intent.md @@ -20,9 +20,12 @@ PRs (#514 to #633): treated alike. - Nothing measures whether a reviewer change (REVIEW.md, AGENTS.md, model) catches more or fewer bugs. -PR for #634 covers the parts that need no settings change: AGENTS.md, REVIEW.md severities, evidence and -convergence rules, Risk and How-to-revert in the PR template, and a fresh-context self-review before a -PR opens. +PR #635 covers the parts that need no settings change: AGENTS.md and docs/review-lessons.md built from +the review history of all 370 merged PRs, REVIEW.md (failure modes, passes, effort by risk, skip list, +severities, evidence, convergence, reply codes), Risk and How-to-revert in the PR template, a +fresh-context self-review before a PR opens, a ruff hook for agents, a `claude-review.yml` workflow (AI +review and security review, inert until an `ANTHROPIC_API_KEY` secret exists), a seed eval set +([evals.md](evals.md)) and `contrib/review_metrics.py` for the metrics below. ## Proposed outcome @@ -32,7 +35,7 @@ Human review time goes where the risk is, with no rise in reverts or post-releas | Tier | Paths | Human approvals | | --- | --- | --- | - | Low | `*.md`, `doc/`, `changelog.d/`, `intent/`, `test/` only, `.sdkharness/` docs | One maintainer, light review | + | Low | `*.md`, `doc/`, `changelog.d/`, `intent/`, `test/` only, `.sdkharness/` docs | None once CI, AI review and security review are green, for trusted authors; otherwise one | | Medium | `b2sdk/_internal/` (except below), `.sdkharness/tests/` | One maintainer | | High | `b2http.py`, `session.py`, `raw_api.py`, `account_info/`, `encryption/`, any `b2sdk/vN/` export, `.github/workflows/`, `pyproject.toml`, `uv.lock`, release commits | Two maintainers, or one plus a green `cli-compat` run reviewed by hand | @@ -41,18 +44,32 @@ Human review time goes where the risk is, with no rise in reverts or post-releas High tiers. 3. **Every review thread resolved before merge** (GitHub "require conversation resolution"). 4. **Automated AI review and security review on every PR**, following REVIEW.md, with open Important - findings blocking merge after a trial period. + findings blocking merge after a trial period. `claude-review.yml` is in place; it needs the + `ANTHROPIC_API_KEY` secret, and then "require conversation resolution" makes open findings block. + Optionally a nightly review run over existing code to find old bugs. 5. **Release commits through a PR** instead of a direct push to `master`, so CI and a second person see - them before tagging. -6. **A green baseline**: fix or quarantine the known live-B2 failures (#614) and make `nox -s lint` check + them before tagging. Risky releases could go out first as a pre-release tried by the B2 CLI. Today `cd.yml` + treats a tag as a pre-release only when it ends in a non-digit, so `vX.Y.Zrc1` would be uploaded to + PyPI as a normal release; fix that check before relying on it. Rollback is a new patch release, never a moved tag. +6. **Merge queue**, so checks rerun on `master` plus the change and two PRs that pass alone cannot + combine into a broken `master`. +7. **A green baseline**: fix or quarantine the known live-B2 failures (#614) and make `nox -s lint` check formatting, so a red check means something. -7. **Past bugs become reviewer evals.** Build a set from bugs reviewers caught or that shipped, for +8. **Turn repeated review comments into checks** (see docs/review-lessons.md, "Candidates for + automation"): a per-apiver signature snapshot test, a raw API parity test, and more ruff rules. +9. **CODEOWNERS** so the Deep paths in REVIEW.md always need a maintainer, and branch protection that + dismisses stale approvals on new commits. +10. **Past bugs become reviewer evals.** Build a set from bugs reviewers caught or that shipped, for example #548 (retry change that would break every POST), #550 (TypeError on a date `Retry-After`), #541 and #542 (v2 relying on v3 abstractions), #543 (simulator bucket-name mapping), #620 (harness check that could reach production). Score the reviewer against it whenever REVIEW.md, AGENTS.md or the model changes. -8. **Measure it.** Share of PRs merged with a substantive review, review rounds to pass, time a PR waits - on a person; reverts and post-release fix PRs as the guardrail. + The seed set is [evals.md](evals.md). +11. **Measure it.** `python contrib/review_metrics.py --days 30` reports human approvals per PR (median, + p95), human comments, review rounds, hours to first human review, PRs merged with no human review, and + reverts as the guardrail. Over the last 120 days: 16 PRs merged, 14 (88%) with no human review, 0 + reverts. Reviewer noise is measured from the reply codes in REVIEW.md (`Fixed`, `Wrong`, + `Intentional`, `Won't fix`). ## Affected users and systems @@ -78,3 +95,5 @@ Human review time goes where the risk is, with no rise in reverts or post-releas - Should the changelog validation step become a required check on `master`? - Who owns fixing the #614 baseline, and should the `development` branch be retired? - Where should the reviewer eval set live and run: this repo, or the central SDK harness? +- Who adds the `ANTHROPIC_API_KEY` secret for `claude-review.yml`, and what per-PR cost is acceptable? +- Who confirms the candidate cases in `evals.md`? From b37a323012b2788f743aefa3c06ce1d3339b4256 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gonzalo=20Pe=C3=B1a-Castellanos?= <goanpeca@gmail.com> Date: Tue, 27 Oct 2026 22:52:11 -0500 Subject: [PATCH 4/4] fix: address Copilot review on security scan and review metrics --- .github/workflows/claude-review.yml | 20 +++++++++ contrib/review_metrics.py | 68 ++++++++++++++++++++--------- 2 files changed, 68 insertions(+), 20 deletions(-) diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index cdbed9f62..e530d4e87 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -57,8 +57,28 @@ jobs: ref: ${{ github.event.pull_request.head.sha }} fetch-depth: 2 - uses: anthropics/claude-code-security-review@0c6a49f1fa56a1d472575da86a94dbc1edb78eda # main + id: security if: ${{ env.ANTHROPIC_API_KEY != '' }} with: claude-api-key: ${{ secrets.ANTHROPIC_API_KEY }} comment-pr: true + run-every-commit: true exclude-directories: doc,.claude,intent,changelog.d + # The action posts inline comments only on its first report per PR, so surface every commit's + # findings here and fail the check (also when the scan produced no count). + - name: Report security findings for this commit + if: ${{ env.ANTHROPIC_API_KEY != '' && steps.security.outputs.findings-count != '0' }} + env: + COUNT: ${{ steps.security.outputs.findings-count }} + RESULTS: ${{ steps.security.outputs.results-file }} + run: | + { + echo "## Security review: ${COUNT:-unknown} finding(s) on ${GITHUB_SHA}" + if [ -n "$RESULTS" ] && [ -f "$RESULTS" ]; then + echo '```json' + cat "$RESULTS" + echo '```' + fi + } >> "$GITHUB_STEP_SUMMARY" + echo "Security review reported ${COUNT:-no count}; see the job summary." >&2 + exit 1 diff --git a/contrib/review_metrics.py b/contrib/review_metrics.py index a443fac6d..76547b6aa 100644 --- a/contrib/review_metrics.py +++ b/contrib/review_metrics.py @@ -13,7 +13,8 @@ Usage: python contrib/review_metrics.py [--repo OWNER/NAME] [--days 30] Needs an authenticated `gh` CLI. Read-only. Bots (logins ending in `[bot]` or starting with -`copilot`) never count as human reviewers; the PR author never counts as their own reviewer. +`copilot`) never count as human reviewers; the PR author never counts as their own reviewer. Only +reviews and comments made before the merge count; replies inside review threads count. """ from __future__ import annotations @@ -26,14 +27,17 @@ QUERY = """ query($q: String!, $endCursor: String) { - search(query: $q, type: ISSUE, first: 50, after: $endCursor) { + search(query: $q, type: ISSUE, first: 20, after: $endCursor) { pageInfo { hasNextPage endCursor } nodes { ... on PullRequest { number title createdAt mergedAt author { login } - reviews(first: 100) { nodes { author { login } state submittedAt } } - comments(first: 100) { nodes { author { login } } } - reviewThreads(first: 100) { nodes { comments(first: 1) { nodes { author { login } } } } } + reviews(first: 100) { totalCount nodes { author { login } state submittedAt } } + comments(first: 100) { totalCount nodes { author { login } createdAt } } + reviewThreads(first: 100) { + totalCount + nodes { comments(first: 50) { totalCount nodes { author { login } createdAt } } } + } } } } @@ -52,12 +56,15 @@ def _parse_time(value: str) -> datetime.datetime: def _fetch(repo: str, days: int) -> list[dict]: since = (datetime.date.today() - datetime.timedelta(days=days)).isoformat() search = f'repo:{repo} is:pr is:merged merged:>={since}' - out = subprocess.run( + result = subprocess.run( ['gh', 'api', 'graphql', '--paginate', '-f', f'query={QUERY}', '-f', f'q={search}'], - check=True, + check=False, capture_output=True, text=True, - ).stdout + ) + if result.returncode: + raise SystemExit(f'gh api graphql failed: {result.stderr.strip()}') + out = result.stdout decoder = json.JSONDecoder() pages, index = [], 0 while index < len(out): @@ -69,6 +76,27 @@ def _fetch(repo: str, days: int) -> list[dict]: return [node for page in pages for node in page['data']['search']['nodes'] if node] +def _human_before_merge( + nodes: list[dict], author: str | None, merged_at, time_key: str +) -> list[dict]: + """Keep items by people other than the author, submitted before the PR merged (pending reviews, + which have no timestamp, are dropped).""" + return [ + node + for node in nodes + if not _is_bot((node['author'] or {}).get('login')) + and node['author']['login'] != author + and node[time_key] + and _parse_time(node[time_key]) <= merged_at + ] + + +def _truncated(pr: dict) -> bool: + connections = [pr['reviews'], pr['comments'], pr['reviewThreads']] + connections += [thread['comments'] for thread in pr['reviewThreads']['nodes']] + return any(c['totalCount'] > len(c['nodes']) for c in connections) + + def _percentile(values: list[float], fraction: float) -> float: ordered = sorted(values) return ordered[min(len(ordered) - 1, int(fraction * len(ordered)))] @@ -87,25 +115,21 @@ def main() -> None: approvals, comments, waits, rounds = [], [], [], [] unreviewed = 0 + truncated = [pr['number'] for pr in prs if _truncated(pr)] for pr in prs: author = (pr['author'] or {}).get('login') - reviews = [ - r - for r in pr['reviews']['nodes'] - if not _is_bot((r['author'] or {}).get('login')) and r['author']['login'] != author - ] + merged_at = _parse_time(pr['mergedAt']) + reviews = _human_before_merge(pr['reviews']['nodes'], author, merged_at, 'submittedAt') approvers = {r['author']['login'] for r in reviews if r['state'] == 'APPROVED'} - human_comments = [ - c - for c in pr['comments']['nodes'] - + [t['comments']['nodes'][0] for t in pr['reviewThreads']['nodes']] - if not _is_bot((c['author'] or {}).get('login')) and c['author']['login'] != author - ] + thread_comments = [c for t in pr['reviewThreads']['nodes'] for c in t['comments']['nodes']] + human_comments = _human_before_merge( + pr['comments']['nodes'] + thread_comments, author, merged_at, 'createdAt' + ) approvals.append(len(approvers)) comments.append(len(human_comments)) rounds.append(len(reviews)) if reviews: - first = min(_parse_time(r['submittedAt']) for r in reviews if r['submittedAt']) + first = min(_parse_time(r['submittedAt']) for r in reviews) waits.append((first - _parse_time(pr['createdAt'])).total_seconds() / 3600) else: unreviewed += 1 @@ -125,6 +149,10 @@ def main() -> None: f'p95 {_percentile(waits, 0.95):.1f}' ) print(f' reverts (guardrail): {len(reverts)} {[pr["number"] for pr in reverts]}') + if truncated: + print( + f' note: more than 100 reviews, comments or threads; counts are lower bounds on {truncated}' + ) if __name__ == '__main__':