-
Notifications
You must be signed in to change notification settings - Fork 192
docs: add AGENTS.md guidance for AI agents #1117
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
nielspardon
wants to merge
2
commits into
substrait-io:main
Choose a base branch
from
nielspardon:docs/add-agent-guidance
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+158
−0
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,157 @@ | ||
| # AGENTS.md | ||
|
|
||
| Guidance for AI agents making changes to the Substrait specification repository. | ||
|
|
||
| ## What this repository is | ||
|
|
||
| Substrait is a **cross-language specification** for relational algebra (query | ||
| plans). This repo holds the specification itself, not an implementation. The | ||
| canonical artifacts are: | ||
|
|
||
| - **Protobuf definitions** — [`proto/substrait/*.proto`](proto/substrait) (the | ||
| serialized plan format; the source of truth). | ||
| - **Text/grammar** — [`grammar/*.g4`](grammar) (ANTLR grammar for the type and | ||
| test-case text format) and [`text/`](text) JSON schemas for extensions and | ||
| dialects. | ||
| - **Extensions** — [`extensions/*.yaml`](extensions) (standard function | ||
| definitions). | ||
| - **Docs site** — [`site/`](site) (MkDocs; published spec documentation). | ||
|
|
||
| Because this is a spec, changes here ripple into the downstream SDKs. Treat a | ||
| change as an API change to an ecosystem, not a local edit. | ||
|
|
||
| ## The downstream SDKs (critical context) | ||
|
|
||
| Any change to `proto/` affects the SDKs that implement the spec. The maintained | ||
| list is [`active_libraries.md`](site/docs/community/active_libraries.md), which | ||
| separates **active libraries** (currently substrait-go, -java, -python, -rs) from | ||
| inactive ones — treat that doc as the source of truth rather than hardcoding the | ||
| set here. External projects also consume Substrait directly (e.g. Apache | ||
| DataFusion, DuckDB's substrait extension); they are useful *usage signal* when | ||
| gauging a change's blast radius, but they are not release-blocking — the active | ||
| libraries are the gate. | ||
|
|
||
| ### Removing or changing a deprecated field | ||
|
|
||
| This is the single most common task in this repo's history, and it is governed by | ||
| the **[breaking-change policy](site/docs/spec/breaking_change_policy.md)** and the | ||
| **[versioning policy](site/docs/spec/versioning.md)**: compatibility is maintained | ||
| via deprecation, and a breaking change must ship an explicit **migration strategy | ||
| that is implemented in all active libraries before the breaking change lands**. | ||
| (Format/specification deprecations also require PMC votes — see | ||
| [`governance.md`](site/docs/governance.md).) The expected workflow: | ||
|
|
||
| 1. Propose the change with an explicit migration strategy. The canonical pattern | ||
| is dual-write → prefer-consume-new → remove after a reasonable soak — see the | ||
| **URI→URN cookbook** in the breaking-change policy for a worked example. | ||
| 2. Determine whether each active library still *produces* the old field and | ||
| whether it has migrated to *consuming* the new one. Search the repos with | ||
| whatever GitHub access is available — the `gh` CLI or a GitHub MCP server — or | ||
| clone them locally if the user has them checked out (often sibling dirs like | ||
| `../substrait-go`). Classify the change per library: *wire-compatible*, | ||
| *source-breaking*, and/or *semantically breaking*. | ||
| 3. Land the migration in the active libraries (companion PRs), then remove the | ||
| field here — often proposed as a **draft PR** pending community discussion. | ||
|
|
||
| Proto mechanics: reserve removed field numbers and names | ||
| (`reserved 3; reserved "microseconds";`) — `buf breaking` enforces this, and old | ||
| plans then carry *unknown fields* rather than failing to parse. See protobuf's | ||
| [updating message types](https://protobuf.dev/programming-guides/proto3/#updating) | ||
| guide for the general wire-compatibility rules. | ||
|
|
||
| ## Environment & tooling | ||
|
|
||
| Everything runs through **[Pixi](https://pixi.prefix.dev)**. Do not invoke | ||
| `buf`/`antlr`/`ruff`/`pytest` from a global install — use the pixi tasks so | ||
| versions match CI. | ||
|
|
||
| ```bash | ||
| pixi run format # ruff format + buf format -w | ||
| pixi run lint # buf lint, buf format check, ruff, editorconfig, yamllint, jsonschema checks | ||
| pixi run test # pytest (regenerates protobuf bindings first) | ||
| pixi run generate # regenerate ANTLR parsers + protobuf python bindings | ||
| ``` | ||
|
|
||
| Individual tasks worth knowing (see `[tool.pixi.tasks]` in | ||
| [`pyproject.toml`](pyproject.toml)): `pixi run buf <args>`, | ||
| `pixi run generate-antlr`, `pixi run generate-protobuf`, `pixi run mkdocs serve`. | ||
|
|
||
| For GitHub work (reading issues/PRs, creating PRs, searching the SDK repos), use | ||
| whatever access the user has configured — the `gh` CLI or a GitHub MCP server. | ||
| Commands below are shown with `gh` for concreteness; the MCP equivalents work | ||
| just as well. | ||
|
|
||
| ## Generated code | ||
|
|
||
| - **`gen/`** (protobuf Python bindings) is **gitignored** — never commit it. | ||
| `pixi run test` regenerates it via `buf generate`. | ||
| - **ANTLR parsers under `tests/*/antlr_parser/`** **are committed** and carry a | ||
| license header. If you change a `.g4` grammar, run `pixi run generate-antlr` | ||
| and commit the regenerated parsers (the `make` target prepends the license). | ||
| - Never hand-edit generated files. | ||
|
|
||
| ## Verification before proposing a change | ||
|
|
||
| - Proto edits: `pixi run buf` for `lint`, `format --diff --exit-code`, and | ||
| `breaking --against 'https://github.com/substrait-io/substrait.git#branch=main'`. | ||
| - Grammar edits: regenerate parsers, then `pixi run test` (e.g. the type-grammar | ||
| and coverage tests under [`tests/`](tests)). | ||
| - YAML (extensions/examples/dialects): validated against schemas via | ||
| `check-jsonschema` (part of `pixi run lint`). | ||
|
|
||
| ## Commit & PR conventions (read carefully — these repeatedly trip up agents) | ||
|
|
||
| The repo uses **[Conventional Commits](https://www.conventionalcommits.org/)** | ||
| and semantic-release. Two CI checks gate every PR: | ||
|
|
||
| 1. **PR title check** (`pr_title.yml`): the **PR title + body together must be a | ||
| valid conventional commit message**, because they become the squash-merge | ||
| commit message. commitlint validates it. Denote breaking changes with `!` | ||
| after the type/scope, e.g. `feat(protos)!: remove deprecated field`. | ||
| 2. **Breaking-change check** (`pr_breaking.yml`): if `buf breaking` detects a | ||
| breaking proto change, the **PR body must contain a line starting with | ||
| `BREAKING CHANGE: `**, or the check fails. | ||
|
|
||
| ### PR description style (based on maintainer feedback) | ||
|
|
||
| Keep descriptions high-signal. The maintainer has repeatedly asked to remove: | ||
|
|
||
| - **Lists of files touched** — obvious from the diff. | ||
| - **Claims that CI-verified things pass** — e.g. "buf lint passes", "tests | ||
| pass". These run as PR checks; if they failed the checks would be red. | ||
| - **Obvious process notes** — e.g. "draft pending review", "coordinating | ||
| companion PRs" once that's implicit in the PR being a draft. | ||
|
|
||
| Do include: the rationale, the migration strategy, and the per-library plus | ||
| wire/source/semantic compatibility analysis for proto changes. This analysis | ||
| belongs in the PR (or draft PR) body — there is no requirement to open a separate | ||
| issue per PR. Issues are for surfacing design discussion on larger or contentious | ||
| changes before a design is settled (see *When in doubt*). | ||
|
|
||
| ### Commit hygiene | ||
|
|
||
| - Keep commit bodies **clean of git trailers**. The changelog pipeline was | ||
| specifically configured to strip trailers (`Signed-off-by`, etc.), and recent | ||
| history has no `Co-authored-by`/tool-attribution trailers. Match that. | ||
| - Don't reference issue numbers inside test-case *descriptions* (use `Closes #N` | ||
| in the PR body instead). | ||
| - Commit-message linting can be checked locally with `npx commitlint` or the | ||
| `commitlint` pre-commit hook (install with | ||
| `pre-commit install --hook-type commit-msg`). | ||
|
|
||
| ## Docs go with the change | ||
|
|
||
| Proto/grammar changes that alter semantics usually need matching updates under | ||
| [`site/docs/`](site/docs) (e.g. `types/type_classes.md`) and sometimes the | ||
| dialect/extension schemas in [`text/`](text). When adding doc examples, use | ||
| **external validated example files** (`--8<-- "examples/..."`), not inline code | ||
| blocks — see [`site/examples/README.md`](site/examples). Check the docs when the | ||
| user proposes a behavioral/semantic change even if they don't explicitly ask. | ||
|
|
||
| ## When in doubt | ||
|
|
||
| - Spec changes are decided by maintainer consensus (community sync). For | ||
| anything beyond a trivial fix, prefer opening a **draft PR or an issue** to | ||
| surface the discussion rather than assuming the design. | ||
| - See [`CONTRIBUTING.md`](CONTRIBUTING.md) for the human-facing version of the | ||
| build/test/release setup. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| @AGENTS.md |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think deprecation preference or guidance may be useful here? Also, it would be nice if we can include the usage survey -- like blast radius analysis for a change in other open source projects like... data fusion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call — rather than inventing new guidance, I've grounded this in the existing policy docs. It now links the breaking-change policy, versioning policy, and governance (PMC votes for spec deprecations), and reorders the workflow to match the policy's rule that the migration must land in all active libraries before the breaking change. For the library list, it now points to
active_libraries.mdas the source of truth instead of hardcoding it. And on blast radius: added external consumers (Apache DataFusion, DuckDB's substrait extension) as non-blocking usage signal, with the active libraries as the actual gate. (c241d19)