diff --git a/.github/PULL_REQUEST_TEMPLATE/EXAMPLE.md b/.github/PULL_REQUEST_TEMPLATE/EXAMPLE.md new file mode 100644 index 0000000..5d0b7ba --- /dev/null +++ b/.github/PULL_REQUEST_TEMPLATE/EXAMPLE.md @@ -0,0 +1,110 @@ + + +## Problem + +Long agent runs aborted at roughly 70% of the real context limit. Operators saw: + +``` +agentkit.budget.BudgetExceeded: 198,004 / 200,000 tokens at turn 42 + (actual measured context: 122,311 tokens) +``` + +Tool-result messages were counted twice, so the tracker reported ~45% high. + +Closes #1184 + +## Fix + +`BudgetTracker.add()` was called by both the transport layer and the message +reducer. Removed the transport-side call and made the reducer the single writer, +since it already owns message identity and can dedupe on `message_id`. + +Rejected: dedupe inside `add()` by hashing content. That hides the double-call +instead of fixing it, and content hashes are not stable across tool-result +serialization. + +## Risk + +- [x] Medium — touches shared code, config, or a public interface + +Blast radius: all four agents on `agentkit>=0.9`. Budgets now report ~30% lower, +so downstream thresholds tuned to inflated numbers will fire later than before. +Rollback: revert this commit; no state or schema involved. Pin consumers to +`agentkit==0.9.3` if the revert lands after their next deploy. + +## Evidence + +``` +$ pytest -q tests/test_budget.py +.................... 20 passed in 1.9s + +$ pytest -q +1,204 passed, 3 skipped in 48.2s + +$ ruff check . && pyright +All checks passed. 0 errors, 0 warnings, 0 informations +``` + +CI: https://github.com/acme/agentkit/actions/runs/1029384756 + +Replay of the captured 42-turn production trace: + +| Turn | Before | After | Actual (tiktoken) | +| --- | --- | --- | --- | +| 10 | 41,208 | 28,905 | 28,905 | +| 42 | 178,442 | 122,310 | 122,311 | + +## Gates + +- [x] Regression test added that fails without this fix — `test_add_is_idempotent_per_message_id` +- [x] No secrets, tokens, or customer data — trace fixture scrubbed by `scripts/scrub_trace.py`; only role, message_id, token counts retained +- [x] `semgrep` clean +- [ ] New IAM / workflow permissions — n/a, no infrastructure or workflow changes +- [ ] Third-party actions pinned — n/a, no workflow files touched +- [x] Public interface change documented — `add()` now requires `message_id`; CHANGELOG under Changed, minor bump to 0.10.0 +- [x] Observability — `budget.tokens.counted` now tags `source=reducer`, so a regression appears as a second source label + +## Reviewer focus + +Hardest look at `reducer.py:88-140`, the dedupe boundary. I accepted a +session-bounded in-memory `set` of seen `message_id`s: a small leak on very long +sessions traded for simplicity, measured at 3.2 MB over 10k turns. + +Deferred: the transport layer still constructs a `BudgetTracker` it no longer +writes to. Removing it is a wider refactor — #1191. + +## Changes by intent + +**Added** +- `tests/fixtures/trace_42turn.json` — scrubbed production trace, the regression fixture +- `scripts/scrub_trace.py` — strips content from captured traces so fixtures are safe to commit + +**Modified** +- `src/agentkit/budget.py` — `add()` requires `message_id` and dedupes on it +- `src/agentkit/reducer.py` — becomes the single writer to the tracker +- `src/agentkit/transport.py` — removed the duplicate `add()` call +- `tests/test_budget.py` — idempotency and missing-id cases +- `CHANGELOG.md` — 0.10.0 entry + +**Deleted** +- none + +## Files touched + + +**6 files** — 6 files changed, 214 insertions(+), 31 deletions(-) + +`src/agentkit/` +- `budget.py` — modified +- `reducer.py` — modified +- `transport.py` — modified + +`tests/` +- `test_budget.py` — modified + +`tests/fixtures/` +- `trace_42turn.json` — added + +`(root)/` +- `CHANGELOG.md` — modified _(generated)_ + diff --git a/.github/PULL_REQUEST_TEMPLATE/infra.md b/.github/PULL_REQUEST_TEMPLATE/infra.md new file mode 100644 index 0000000..fb59c3e --- /dev/null +++ b/.github/PULL_REQUEST_TEMPLATE/infra.md @@ -0,0 +1,73 @@ +## Problem + + + +``` +paste the alert, error, or cost/quota evidence here +``` + +Closes # + +## Environments + +- [ ] dev +- [ ] staging +- [ ] prod + +## Plan + +
terraform plan + +``` +paste plan output here +``` + +
+ +- [ ] Plan reviewed; no unexpected destroy or replace +- [ ] No drift against live state +- [ ] State backend and locking unchanged, or migration documented below + +## Risk + +- [ ] Low — additive resource, no traffic path, trivially destroyable +- [ ] Medium — modifies an in-use resource, brief or zero downtime +- [ ] High — destroy/replace, data store, IAM, network boundary, or DNS + +Resources created: +Resources modified: +Resources destroyed or replaced: +Expected downtime: +Blast radius: + +## Rollback + +Procedure: +Data-loss risk on rollback: +Backup or snapshot taken: + +## Gates + +- [ ] IAM least privilege; no wildcard actions or resources +- [ ] Secrets from Secrets Manager or SSM; no literals, no tfvars in git +- [ ] Network exposure unchanged, or new ingress justified below +- [ ] Encryption at rest and in transit enforced +- [ ] Tagging and cost allocation applied +- [ ] Monitoring and alerting cover the new resources +- [ ] Module version pinned; provider constraints unchanged or bumped deliberately + +## Reviewer focus + +## Changes by intent + + + +**Added** +**Modified** +**Deleted** + +## Files touched + + +_pending — the bot fills this in on push_ + diff --git a/.github/PULL_REQUEST_TEMPLATE/release.md b/.github/PULL_REQUEST_TEMPLATE/release.md new file mode 100644 index 0000000..dc91453 --- /dev/null +++ b/.github/PULL_REQUEST_TEMPLATE/release.md @@ -0,0 +1,54 @@ +## Release + +Version: `vX.Y.Z` (previous: `vX.Y.Z`) +Bump rationale: patch / minor / major — because + +## Changelog + +### Added +### Changed +### Fixed +### Removed or deprecated +### Breaking + +## Risk + +- [ ] Low — patch, no interface change +- [ ] Medium — minor, additive interface change +- [ ] High — breaking change or migration required + +Downstream consumers affected: +Rollback tag: `vX.Y.Z` + +## Evidence + +``` +$ pytest -q +$ ruff check . && pyright +``` + +CI run: + +## Gates + +- [ ] Version bumped in every manifest (pyproject / package.json / chart / action.yml) +- [ ] CHANGELOG.md updated and dated +- [ ] Full CI green on the release branch +- [ ] Upgrade or migration notes written for every breaking change +- [ ] Artifacts build reproducibly (wheel, image digest, tag) +- [ ] Docs and README reflect the new behavior +- [ ] Consumers notified or bump PRs opened + +## Reviewer focus + +## Changes by intent + +**Added** +**Modified** +**Deleted** + +## Files touched + + +_pending — the bot fills this in on push_ + diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md new file mode 100644 index 0000000..f50cf4d --- /dev/null +++ b/.github/pull_request_template.md @@ -0,0 +1,76 @@ +## Problem + + + +``` +paste the error / failing output here, or delete this block and describe the gap +``` + +Closes # + +## Fix + + + +## Risk + + + +- [ ] Low — additive, reversible, no data or contract change +- [ ] Medium — touches shared code, config, or a public interface +- [ ] High — breaking change, migration, IAM/network, or irreversible + +Blast radius: +Rollback: + +## Evidence + + + +``` +$ pytest -q +$ ruff check . && pyright +``` + +## Gates + + + +- [ ] Regression test added that fails without this fix +- [ ] No secrets, tokens, or customer data in code, tests, fixtures, or logs +- [ ] `semgrep` clean, or findings triaged below +- [ ] New IAM / workflow permissions are least privilege and enumerated +- [ ] Third-party actions pinned to a full commit SHA +- [ ] Public interface change is documented and versioned +- [ ] Observability exists for the new path (metric, log, trace, or alert) + +## Reviewer focus + + + +## Changes by intent + + + +**Added** +- `path/to/new_file.py` — why this file needs to exist + +**Modified** +- `path/to/existing.py` — what changed in it and why + +**Deleted** +- `path/to/dead.py` — why it is safe to remove + +## Files touched + + + + +_pending — the bot fills this in on push_ + diff --git a/.github/workflows/pr-files.yml b/.github/workflows/pr-files.yml new file mode 100644 index 0000000..3cf32a1 --- /dev/null +++ b/.github/workflows/pr-files.yml @@ -0,0 +1,96 @@ +name: PR files touched + +on: + pull_request: + types: [opened, synchronize, reopened, edited] + +permissions: + contents: read + pull-requests: write + +concurrency: + group: pr-files-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + annotate: + if: github.event.pull_request.draft == false + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + with: + fetch-depth: 0 + + - env: + BASE: ${{ github.event.pull_request.base.sha }} + HEAD: ${{ github.event.pull_request.head.sha }} + run: | + git diff --name-status "$BASE...$HEAD" > /tmp/changed.txt + git diff --shortstat "$BASE...$HEAD" > /tmp/shortstat.txt + + - uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + with: + script: | + const fs = require('fs'); + const START = ''; + const END = ''; + const SYM = { A: 'added', M: 'modified', D: 'deleted', R: 'renamed', C: 'copied', T: 'typechange' }; + const GEN = /^(poetry\.lock|package-lock\.json|uv\.lock|.*\.snap|CHANGELOG\.md)$|(^|\/)(dist|__snapshots__)\//; + + const pr = context.payload.pull_request; + const body = pr.body || ''; + + const rows = fs.readFileSync('/tmp/changed.txt', 'utf8') + .split('\n').filter(Boolean) + .map(l => { const p = l.split('\t'); return { st: SYM[p[0][0]] || p[0], path: p.slice(1).join(' -> ') }; }); + const shortstat = fs.readFileSync('/tmp/shortstat.txt', 'utf8').trim(); + + // paths the author declared under "## Changes by intent" + const intentSec = (body.match(/##\s*Changes by intent([\s\S]*?)(?=\n##\s|$)/i) || [,''])[1] + .replace(//g, ''); + const declared = new Set([...intentSec.matchAll(/`([^`]+)`/g)] + .map(m => m[1].trim()).filter(p => p.includes('/') || p.includes('.'))); + + const undeclared = rows.filter(r => !declared.has(r.path) && !GEN.test(r.path)); + const phantom = [...declared].filter(d => !rows.some(r => r.path === d) && !d.startsWith('path/to/')); + + const group = {}; + for (const r of rows) { + const dir = r.path.includes('/') ? r.path.split('/').slice(0, -1).join('/') : '(root)'; + (group[dir] ||= []).push(r); + } + + let out = `**${rows.length} file${rows.length === 1 ? '' : 's'}** — ${shortstat}\n`; + if (undeclared.length) out += `\n> [!WARNING]\n> ${undeclared.length} file(s) not declared under **Changes by intent**: ` + + undeclared.slice(0, 10).map(r => `\`${r.path}\``).join(', ') + (undeclared.length > 10 ? ', …' : '') + '\n'; + if (phantom.length) out += `\n> [!NOTE]\n> Declared but not in the diff: ` + + phantom.map(p => `\`${p}\``).join(', ') + '\n'; + + const many = rows.length > 60; + if (many) out += `\n
Expand file list\n`; + for (const dir of Object.keys(group).sort()) { + out += `\n\`${dir}/\`\n`; + for (const r of group[dir]) { + const name = r.path.split('/').pop(); + const mark = declared.has(r.path) ? '' : (GEN.test(r.path) ? ' _(generated)_' : ' ⚠️'); + out += r.st === 'deleted' + ? `- ~~\`${name}\`~~ — deleted${mark}\n` + : `- \`${name}\` — ${r.st}${mark}\n`; + } + } + if (many) out += `\n
\n`; + + const block = `${START}\n${out}${END}`; + const next = body.includes(START) && body.includes(END) + ? body.replace(new RegExp(`${START}[\\s\\S]*?${END}`), block) + : `${body}\n\n## Files touched\n\n${block}\n`; + + if (next !== body) { + await github.rest.pulls.update({ ...context.repo, pull_number: pr.number, body: next }); + } + + if (undeclared.length) { + core.summary.addHeading('Undeclared files', 2) + .addList(undeclared.map(r => `${r.path} (${r.st})`)).write(); + core.setFailed(`${undeclared.length} file(s) changed but not declared under "Changes by intent".`); + } diff --git a/.github/workflows/pr-gates.yml b/.github/workflows/pr-gates.yml new file mode 100644 index 0000000..d2264ff --- /dev/null +++ b/.github/workflows/pr-gates.yml @@ -0,0 +1,50 @@ +name: PR gates + +on: + pull_request: + types: [opened, edited, synchronize, reopened, ready_for_review] + +permissions: + pull-requests: read + +jobs: + check: + if: github.event.pull_request.draft == false && github.event.pull_request.user.login != 'dependabot[bot]' + runs-on: ubuntu-latest + steps: + - uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + with: + script: | + const body = context.payload.pull_request.body || ''; + const fail = []; + + const section = name => { + const m = body.match(new RegExp(`##\\s*${name}([\\s\\S]*?)(?=\\n##\\s|$)`, 'i')); + return m ? m[1] : ''; + }; + + const problem = section('Problem') + .replace(//g, '') + .replace(/```[\s\S]*?```/g, m => m.includes('paste the error') ? '' : m) + .replace(/Closes #\d*/i, '').trim(); + if (problem.length < 30) fail.push('**Problem** section is empty or still boilerplate. Describe the error this fixes.'); + + const risk = [...section('Risk').matchAll(/^\s*-\s*\[([ xX])\]/gm)].filter(m => m[1] !== ' '); + if (risk.length !== 1) fail.push(`**Risk**: check exactly one level (found ${risk.length}).`); + + if (!/```[\s\S]*?```/.test(section('Evidence')) && !/actions\/runs\/\d+/.test(section('Evidence'))) + fail.push('**Evidence**: paste command output or link a CI run.'); + + for (const line of section('Gates').split('\n')) { + const m = line.match(/^\s*-\s*\[ \]\s*(.+)$/); + if (!m) continue; + const item = m[1].trim(); + // an unchecked box needs a stated reason: "n/a", "no ...", or a trailing "— because" + if (!/(n\/a|not applicable|—|--|:)\s*\S{4,}/i.test(item)) + fail.push(`**Gate** unchecked with no reason: _${item.slice(0, 70)}_`); + } + + if (fail.length) { + core.summary.addHeading('PR gates failed', 2).addList(fail).write(); + core.setFailed(fail.length + ' gate issue(s). See the job summary.'); + } diff --git a/docs/DEPLOY.md b/docs/DEPLOY.md new file mode 100644 index 0000000..85cc469 --- /dev/null +++ b/docs/DEPLOY.md @@ -0,0 +1,89 @@ +# Deploy + +## 1. Place the files + +If your org already has a `.github` repository: + +```bash +./scripts/install.sh /path/to/org/.github +cd /path/to/org/.github +git checkout -b feat/pr-templates +git add .github docs +git commit -m "feat(pr): org-wide PR template with intent reconciliation and gates" +git push -u origin feat/pr-templates +``` + +If it does not exist, create a **public** repository named exactly `.github` in the +org first, then run the same steps. A private `.github` repo will not serve PR +templates org-wide. + +Merge to the default branch. Templates take effect on the next PR opened in any +repo that has no local override. + +## 2. Grant the workflows what they need + +`pr-files.yml` edits the PR body, so it needs `pull-requests: write`. Confirm at +**Org → Settings → Actions → General**: + +- Workflow permissions: *Read repository contents and packages permissions* + (the workflows declare their own elevated scopes explicitly) +- Allow GitHub Actions to create and approve pull requests: not required + +Both workflows are already scoped with a minimal top-level `permissions:` block and +pin third-party actions to full commit SHAs. + +## 3. Distribute the workflows + +Workflows in the `.github` repo do **not** run for other repositories. Only the +templates propagate. Pick one: + +**Option A — reusable workflow (recommended).** Move the job bodies into +`workflow_call` workflows here, then each repo adds a three-line caller: + +```yaml +name: PR hygiene +on: + pull_request: + types: [opened, edited, synchronize, reopened, ready_for_review] +jobs: + hygiene: + uses: YOUR_ORG/.github/.github/workflows/pr-gates.yml@v1 +``` + +**Option B — sync.** Copy `.github/workflows/pr-*.yml` into each repo with a +scheduled sync job or a tool like `repo-file-sync-action`. + +Tag this repo `v1` and let callers pin the tag, so you can iterate on `main` +without breaking every repo at once. + +## 4. Make the checks required + +Per repo, or org-wide via a **ruleset** on the default branch: + +- Require a pull request before merging +- Required status checks: `PR gates / check`, `PR files touched / annotate` +- Require branches to be up to date before merging + +Roll out in warn-only mode first: comment out the `core.setFailed(...)` lines in +both workflows, watch a week of real PRs, then re-enable. Turning hard failures on +day one trains people to bypass rather than comply. + +## 5. Validate + +```bash +# from a repo with the workflows installed +gh pr create --fill --draft # drafts are skipped by both workflows +gh pr ready +gh pr view --json body -q .body # confirm the Files touched block was written +gh run list --workflow "PR gates" +``` + +Expected: an untouched template fails gates with an empty **Problem**, no **Risk** +box checked, and no **Evidence**. That failure is the smoke test. + +## Rollback + +Revert the merge commit on this repo's default branch. Templates stop being applied +immediately for new PRs; existing PR bodies are unaffected. Remove the required +status checks from any ruleset first, otherwise open PRs will block on a check that +no longer runs. diff --git a/docs/DESIGN.md b/docs/DESIGN.md new file mode 100644 index 0000000..999e81b --- /dev/null +++ b/docs/DESIGN.md @@ -0,0 +1,56 @@ +# Design notes + +## Problem first + +The first section is `## Problem`, with a fenced block prompting the real traceback, +alert, or failing assertion. Reviewers arrive knowing what hurt before they see how +it was fixed. A restatement of the diff is not a problem statement. + +## Checkboxes carry weight or they are cut + +Most PRs are approved with superficial review, so unenforced checkboxes teach +reflexive ticking. Two kinds survive here: + +- **Risk** — exactly one of three levels. Mutually exclusive, and it routes review + depth rather than acting as a to-do item. +- **Gates** — seven items. Check it, or leave it unchecked *with a reason on the + same line*. `pr-gates.yml` fails an unchecked box that has no justification. + +Mixing "all must be checked" with "pick exactly one" is what breaks naive task-list +validators. Separating them into two sections lets each be enforced correctly. + +## Two file lists, on purpose + +`## Changes by intent` is written by the author: `path — why`, grouped by +added/modified/deleted. `## Files touched` is written by the bot from +`git diff --name-status base...head`. + +The value is the comparison. A file in the diff that nobody declared is usually a +stray debug edit, a committed artifact, or scope creep. `pr-files.yml` marks those +inline with a warning callout and fails the job. Declared-but-absent paths surface +as a note — typically a typo or a dropped change. Lockfiles, snapshots, `dist/`, +and `CHANGELOG.md` are exempt via the `GEN` regex; extend it for your stacks. + +Triple-dot diff (`base...head`) is deliberate: it yields the full PR change set +rather than only the latest commit, which is the usual bug in file-listing jobs. + +## Evidence, not assertion + +The `Evidence` gate requires a fenced block or a link matching `actions/runs/`. +"Tests pass" is unverifiable; pasted output is. + +## Idempotent body edits + +The bot rewrites the PR body between `` and `:END` +sentinels instead of posting comments, so the description stays current and the +thread stays clean. The template ships the sentinels with a `_pending_` placeholder +so the bot has an anchor on the very first push. It writes only when content +changes, avoiding an edit → `edited` event → edit loop. + +## The example is the standard + +`PULL_REQUEST_TEMPLATE/EXAMPLE.md` is a complete, realistic PR: measured +before/after table, a named rejected alternative, two gates deliberately unchecked +*with reasons*, an accepted trade-off with a measured cost, and a deferred refactor +linked to its own issue. Link it from `CONTRIBUTING.md` and from onboarding. +Contributors imitate the best precedent they can find; give them one.