Skip to content

docs: record the spec as the source of truth, add a PR template - #1073

Open
nielspardon wants to merge 1 commit into
substrait-io:mainfrom
nielspardon:docs/spec-source-of-truth-and-pr-template
Open

docs: record the spec as the source of truth, add a PR template#1073
nielspardon wants to merge 1 commit into
substrait-io:mainfrom
nielspardon:docs/spec-source-of-truth-and-pr-template

Conversation

@nielspardon

@nielspardon nielspardon commented Aug 6, 2026

Copy link
Copy Markdown
Member

Two gaps in the contributor docs.

Nothing stated that this repository implements the Substrait specification rather than defining it, or how to proceed when the spec is ambiguous. CONTRIBUTING.md now leads with that and gives an escalation order: the sibling language bindings under Active Libraries are the closest comparison for modeling questions, since they solve the same problem at the same layer; the engines under Powered by Substrait are the better reference for runtime semantics; and a clarification issue upstream comes only when neither shows a consensus. AGENTS.md carries the agent-specific half, because filling a spec gap with something plausible and then describing it as spec-defined is a failure mode worth naming outright.

There was also no PR template, even though the title and body are linted together and the body becomes the squash-merge commit body. That makes the description changelog input rather than a review scratchpad — easy to miss, and behind most malformed release notes. The template states only that and links to the new CONTRIBUTING.md section; the detail stays there, where it cannot be pasted into a commit body by accident. An earlier revision of this PR also carried a copy-paste BREAKING CHANGE: placeholder; that is dropped, because a placeholder inside an HTML comment survives a partial edit and reaches the release notes, and guarding it well is worse than not shipping one.

The gh CLI line is removed from AGENTS.md deliberately, not as collateral: it describes the agent's environment rather than anything about this repository, so it belongs in whatever configuration an agent is run with.

🤖 Generated with AI

@alexandrefimov alexandrefimov left a comment

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.

Useful change, and the CONTRIBUTING sections read well. One thing in the template is worth fixing before it ships, noted inline.

Checked the TOC anchors against the new headings -- those all resolve.

Minor: the AGENTS.md rewrite drops For GitHub work (issues, PRs), use the gh CLI. from the intro. Unrelated to the spec-source-of-truth point, so it looks accidental rather than intended.

Comment thread .github/pull_request_template.md Outdated
Comment on lines +31 to +35
Un-indent the line below -- while indented it is ignored by the parser, so a
forgotten placeholder cannot reach the release notes:

BREAKING CHANGE: <what breaks, and what consumers should do instead>
-->

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 block re-creates the trap it warns about, with --> standing in for the git trailer.

The placeholder sits inside the HTML comment and the instruction is to un-indent it. If someone does that but leaves the enclosing <!-- / --> -- easy to miss, since the whole block is invisible in GitHub's preview -- the squash-merge body ends up as:

BREAKING CHANGE: <what breaks, and what consumers should do instead>
-->

GitHub copies the PR description into the squash commit verbatim, comment markers included, and the parser ends the note only at another note keyword or a reference -- exactly as the text above this line says. So --> gets absorbed into the note and published in the release notes.

stripTrailers in .releaserc.mjs won't catch it either: it pops only blank lines and lines matching TRAILER_KEYS, so it breaks on the first --> and leaves it in place.

Either fix works:

  • Move the breaking-change block outside the comment, so there is no --> left to trail the footer.
  • Add a trailing --> to what stripTrailers strips -- the same belt-and-braces rationale as the git trailers it already handles.

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.

Confirmed, and fixed by deleting the placeholder rather than guarding it.

Both mechanics you describe hold. I had already reproduced the note absorbing the marker while testing this very template — the parse came out as text: "…do instead.\n-->" — and stripTrailers stops at the first line that is neither blank nor a recognized trailer, so --> ends the scan instead of being popped.

Two things I checked while deciding between the options:

  • A #-prefixed placeholder is inert with no config change. The parser anchors the note keyword at line start, and # isn't it — so commentChar isn't needed to make a commented placeholder safe.
  • commentChar: '#' does filter lines before note extraction, so it would also strip a trailing # --> out of a live note. That makes it a cleaner primitive than a bespoke --> regex if we want the belt-and-braces version later.

I went with neither. Every guard I tried lived inside the comment block and just created another half-edited state to reason about; the placeholder itself was the hazard. The template is now a single comment block stating that the description is changelog input, and the footer syntax lives in CONTRIBUTING.md#breaking-changes where it can't be pasted into a commit body by accident.

Happy to do the stripTrailers hardening as a separate PR — as commentChar rather than an --> match — though given that only committers and PMC squash-merge, rendering the prospective note back onto the PR from pr_title.yaml is probably the higher-value half of that.

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.

Deleting the placeholder closes it — with no line left to un-indent, the enclosing --> stays inside the comment. Nothing further from me here.

@nielspardon
nielspardon force-pushed the docs/spec-source-of-truth-and-pr-template branch from bfdb01b to ff93e03 Compare August 7, 2026 06:37
@nielspardon

Copy link
Copy Markdown
Member Author

On the gh CLI line — that removal is intentional, not collateral. It states that gh is installed and authenticated, which is a fact about the environment an agent happens to run in rather than anything about this repository, so it belongs in that agent's own configuration and not in a file every contributor shares. My PR description should have said so up front; it now does.

The template is trimmed to a single comment block carrying only the fact that is easy to miss (title + description become the squash-merge commit message), with the BREAKING CHANGE: placeholder removed per your inline finding.

@alexandrefimov

Copy link
Copy Markdown
Contributor

Removing the placeholder is the better fix — with it gone there's no half-edited state left to reason about.

Two things while you're in this file, neither blocking. AGENTS.md now contradicts itself on attribution: the existing line says to keep commit bodies free of git trailers "(Signed-off-by, Co-authored-by, tool-attribution lines)", while the new bullet says the BREAKING CHANGE: footer goes "after any tool-attribution line". For a file meant to be followed literally that wants settling one way or the other.

And it isn't hypothetical: 🤖 Generated with AI is already in the 0.98.0 release notes, absorbed into #1044's breaking note under ### ⚠ BREAKING CHANGES. So if you do the stripTrailers hardening, commentChar: '#' won't catch that line — it'd need its own entry.

@nielspardon
nielspardon force-pushed the docs/spec-source-of-truth-and-pr-template branch from ff93e03 to f7a979e Compare August 7, 2026 08:03
State that this repo implements the Substrait specification rather than
defining it, and give the escalation order for ambiguous spec points:
compare the sibling language bindings, then the engines, and raise a
clarification issue upstream only when neither shows a consensus.

Add a pull request template carrying the one fact that is easy to miss
and sits behind most malformed release notes: the PR title and body
become the squash-merge commit message, so the description is changelog
input rather than a review scratchpad. The detail stays in CONTRIBUTING,
where it cannot be pasted into a commit body by accident.

State the breaking-change rule as an invariant -- nothing may follow the
footer -- rather than as an ordering relative to the attribution lines
the same docs tell you not to write. The parser ends a note only at
another footer keyword or an issue reference, so whatever trails it is
published verbatim; the v0.98.0 release notes already carry a
tool-attribution line that way.

Drop the gh CLI instruction from AGENTS.md; it describes the agent's
environment rather than anything about this repository.
@nielspardon
nielspardon force-pushed the docs/spec-source-of-truth-and-pr-template branch from f7a979e to 09c249a Compare August 7, 2026 08:04
@nielspardon

Copy link
Copy Markdown
Member Author

Both points taken, and the second one verified — the note on #1044 parsed as:

BREAKING CHANGE: the isthmus fat jar (…) instead.

🤖 Generated with AI

stripTrailers pops trailing blanks and ^(Key):\s matches, so it stops on the attribution line and leaves it. You're right that commentChar wouldn't help, and TRAILER_KEYS is also the wrong home for it: that list is interpolated into a Key: value pattern and the attribution line has no colon, so it needs a separate pattern rather than a new entry.

On the contradiction — settled toward the invariant rather than the ordering. The bullet no longer orders the footer against a tool-attribution line, since doing so quietly sanctioned what the line above forbids. It now says the footer goes last with nothing after it, and names attribution only as an example of what gets absorbed. That leaves one rule to follow instead of two that disagree, and it holds regardless of what any given harness appends. CONTRIBUTING.md gets the same treatment plus the v0.98.0 precedent, which argues the point better than a hypothetical does.

Worth recording that the trap caught me in the commit message for this very PR: I wrote "nothing may follow a / BREAKING CHANGE: footer" across a line break, which put the keyword at column 0 and turned a docs: commit into a breaking one. The tell was cheap — commitlint emits footer-leading-blank when it recognizes a footer, so that warning on a commit you don't consider breaking is a reliable signal. Rewrapped, and it's a fair demonstration that the ordering framing was too subtle to follow reliably.

I'd still keep the stripTrailers change out of this PR — it turns a docs change into a release-config one, and matching prose is whack-a-mole in a way the Key: value trailers aren't. Happy to open it as fix(release): with the attribution pattern plus rendering the prospective note onto the PR, unless you'd rather have the pattern here.

@alexandrefimov

Copy link
Copy Markdown
Contributor

Agreed on keeping it out, and on TRAILER_KEYS — the colon is baked into the pattern, so no entry there could match a colonless line.

One thing that may lower the cost when you open it: pr_title.yaml already assembles title + body, runs on edited, and updates its comment in place via a hidden marker — so the rendering has a home in a job that already parses the message.

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