Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 110 additions & 0 deletions .github/PULL_REQUEST_TEMPLATE/EXAMPLE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
<!-- WORKED EXAMPLE — the standard, not a template. Reference it; do not copy the content. -->

## 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

<!-- FILES-TOUCHED:START -->
**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)_
<!-- FILES-TOUCHED:END -->
73 changes: 73 additions & 0 deletions .github/PULL_REQUEST_TEMPLATE/infra.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
## Problem

<!-- The operational symptom, cost line, alert, or capability gap driving this change. -->

```
paste the alert, error, or cost/quota evidence here
```

Closes #

## Environments

- [ ] dev
- [ ] staging
- [ ] prod

## Plan

<details><summary><code>terraform plan</code></summary>

```
paste plan output here
```

</details>

- [ ] 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

<!-- `path — why`. Paths in backticks. Reconciled against the diff by CI. -->

**Added**
**Modified**
**Deleted**

## Files touched

<!-- FILES-TOUCHED:START -->
_pending — the bot fills this in on push_
<!-- FILES-TOUCHED:END -->
54 changes: 54 additions & 0 deletions .github/PULL_REQUEST_TEMPLATE/release.md
Original file line number Diff line number Diff line change
@@ -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

<!-- FILES-TOUCHED:START -->
_pending — the bot fills this in on push_
<!-- FILES-TOUCHED:END -->
76 changes: 76 additions & 0 deletions .github/pull_request_template.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
## Problem

<!-- REQUIRED. The error, bug, or gap this fixes. Lead with the symptom a human saw.
Paste the actual traceback, failing assertion, alert, or log line. -->

```
paste the error / failing output here, or delete this block and describe the gap
```

Closes #

## Fix

<!-- What you changed to make the problem above go away. Note alternatives you rejected and why. -->

## Risk

<!-- Pick exactly one. This routes how hard reviewers look. -->

- [ ] 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

<!-- Show the problem is gone. Pasted output or a CI link. Not "tests pass". -->

```
$ pytest -q
$ ruff check . && pyright
```

## Gates

<!-- Leave unchecked if it does not apply, and say why on the line. An unchecked box
with no reason blocks merge (see .github/workflows/pr-gates.yml). -->

- [ ] 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

<!-- Where to look hardest. Trade-offs accepted. Deferred follow-ups, with issue links. -->

## Changes by intent

<!-- YOU write this. One line per file you meant to touch, with the reason.
This is the contract; the bot-generated list below is the actual diff.
Any mismatch is flagged by CI — an unexplained file is usually a stray
debug edit, a committed artifact, or scope creep.
Delete the ADDED or MODIFIED heading if empty. Use `path — why`. -->

**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

<!-- Auto-filled by .github/workflows/pr-files.yml on every push. Do not edit by hand. -->

<!-- FILES-TOUCHED:START -->
_pending — the bot fills this in on push_
<!-- FILES-TOUCHED:END -->
Loading
Loading