Skip to content

docs: add AGENTS.md guidance for AI agents#1117

Open
nielspardon wants to merge 2 commits into
substrait-io:mainfrom
nielspardon:docs/add-agent-guidance
Open

docs: add AGENTS.md guidance for AI agents#1117
nielspardon wants to merge 2 commits into
substrait-io:mainfrom
nielspardon:docs/add-agent-guidance

Conversation

@nielspardon

@nielspardon nielspardon commented Jul 3, 2026

Copy link
Copy Markdown
Member

Proposed change

Add an AGENTS.md documenting the conventions AI agents (and new contributors) need when changing this specification repository, and a CLAUDE.md that imports it (@AGENTS.md) so Claude Code picks it up automatically.

The guidance is distilled from recurring patterns and captures:

  • Cross-SDK impact analysis — proto changes ripple into substrait-java/-go/-python/-rs, so deprecated-field removals should be analyzed against all four SDKs first, with wire/source/semantic compatibility classified.
  • Pixi-based toolingpixi run format / lint / test / generate, so versions match CI.
  • Generated codegen/ is gitignored; the ANTLR parsers under tests/*/antlr_parser/ are committed and must be regenerated when grammars change.
  • CI gates — the conventional-commit PR-title check and the BREAKING CHANGE: footer requirement when buf breaking trips.
  • Docs — semantic changes usually need matching site/docs updates and externally-validated examples.

AGENTS.md is the emerging cross-tool convention; CLAUDE.md is kept to a one-line import so there is a single source of truth.

🤖 Generated with AI


This change is Reviewable

Add AGENTS.md documenting the conventions, tooling, and cross-SDK impact
analysis that changes to this specification repository require, plus a
CLAUDE.md that imports it so Claude Code loads the guidance automatically.

The guidance covers: the pixi-based build/test/lint tasks, which generated
code is committed vs. gitignored, the conventional-commit and breaking-change
CI checks that gate every PR, and PR-description conventions.
Comment thread AGENTS.md
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)

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Member Author

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.md as 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)

Comment thread AGENTS.md Outdated
3. Only then propose the change, often as a **draft PR** pending community
discussion, with companion PRs to the SDKs where needed.

Wire-compatibility details matter and are expected in PR descriptions: reserve

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this... I don't know whether it is worth to encode more precisely... or just link to a page in protobuf compatibility doc in protobuf official documentation.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — no point re-deriving protobuf's rules here. Trimmed this down to the substrait-specific bit (reserve removed field numbers/names, enforced by buf breaking, so old plans carry unknown fields rather than failing to parse) and linked out to protobuf's Updating A Message Type guide for the general wire-compatibility rules. Dropped the dissolved-oneof note. (c241d19)

Comment thread AGENTS.md Outdated
companion PRs" once that's implicit in the PR being a draft.

Do include: the rationale, the SDK impact analysis, and wire/source/semantic
compatibility analysis for proto changes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where these should go? always create issue per PR?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarified this in the doc: the impact/migration/compatibility analysis goes in the PR (or draft PR) body — no requirement to open a separate issue per PR. Issues are for surfacing design discussion on larger or contentious changes before the design is settled (the URI→URN cookbook is a good example of that flow). Does that match the intended process? Happy to adjust the wording if you'd frame it differently. (c241d19)

Address review feedback on the deprecation/breaking-change guidance:
reference active_libraries.md, breaking_change_policy.md, versioning.md,
and governance.md instead of re-deriving them, reorder the workflow to
match the breaking-change policy, add external consumers as non-blocking
usage signal, trim the wire-compat prose to a protobuf link, and clarify
that impact analysis goes in the PR body rather than a per-PR issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants