Skip to content

🏗️🔧:fix four faults in landing a pull request - #914

Merged
openinf-commit-queue[bot] merged 1 commit into
mainfrom
infra/commit-queue-defects
Sep 10, 2026
Merged

openinf-commit-queue[bot] merged 1 commit into
mainfrom
infra/commit-queue-defects

Conversation

@DerekNonGeneric

@DerekNonGeneric DerekNonGeneric commented Sep 10, 2026 •

Copy link
Copy Markdown
Member

Summary

Porting this machinery into OpenINF/sdk put it under review again, and four defects came out of that. They were written here, so they are fixed here. Each was reproduced before being fixed, and each has regression cases.

A branch of more than thirty commits could not land at all. land-pull-request.mts asks the API for the branch's commits with gh api --paginate and a --jq filter. Those two together filter each page and print the results one after another, so anything past the first page hands JSON.parse several arrays in a row:

$ gh api --paginate 'repos/OpenINF/sdk/commits?per_page=2' --jq '[.[] | .sha]'
["8851fc8…","b7ee361…"]
["be50c61…","4b49126…"]

JSON.parse then fails with Unexpected non-whitespace character after JSON at position 88, the outer catch reports a parse error rather than a reason, and nothing lands. It now uses --paginate --slurp without --jq, which gathers the pages into one array, and does the merge-dropping filter in the task.

A long enough sign-off was impossible to write. The 72-column limit was applied to every line after the subject, trailers included. Signed-off-by: Christopher Alexander Montgomery <christopher.montgomery@example.org> is 84 characters and was rejected as a body-width error. Folding it is no escape, because readTrailers keeps only the token line, so the address moves out of reach of checkSignOff and that fails instead. Anyone whose name and address run past 57 characters was stuck between the two. The trailer block is now exempt from the limit. Prose is not, including a last paragraph that only looks like trailers and so holds none.

A folded trailer lost half of itself when landing. partsOfMessage read each line on its own, so the token line went to the trailers while an indented continuation stayed in the body. Co-authored-by: written over two lines landed with the address left behind, and the composed message still validated, so the queue merged it and the attribution was quietly lost. Continuations now attach to the trailer above them rather than being pushed as entries of their own, which would have let rank sort them away from what they belong to.

The pull request template pointed at the wrong field. It said to paste the emoji into the description; it belongs in the title, which is the subject that lands and the only place anything reads it. Following the template as written produced a valid description and an invalid title.

Validation

  • nps test — every verify task passes, including verify.commits on this commit
  • 69 unit tests in the two modules, 6 of them new
  • the pagination fix checked against the live API for shape, parents included so the merge filter still works

Note

The portal carries the same four, and the same fix, on its own infra/commit-queue-defects. OpenINF/sdk already has them, since porting this there is where review found them.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Commit messages now correctly allow long trailer lines, while still enforcing the line-length limit on regular text.
    • Folded trailer lines are now preserved and handled properly in commit and landing messages.
    • Pull request generation now handles commits spread across multiple pages without failing.
  • Documentation

    • Updated the pull request template so emoji guidance points to the title instead of the description.

Porting this machinery into OpenINF/sdk put it under review again, and
four things came out of that. All of them are here as well, which is
where they were written, so they are fixed here first.

A branch of more than thirty commits could not land at all. The commits
are asked of the API with `--paginate` and a `--jq` filter, and those
two together filter each page and print the results one after another,
so anything past the first page handed `JSON.parse` several arrays in a
row. What came back was a parse error rather than a reason. `--slurp`
gathers the pages into one array instead, and the filtering that `--jq`
was doing happens in the task.

The 72-column limit was applied to trailers, which cannot be wrapped to
meet it: git would read a folded value, but `readTrailers` keeps only
the token line, so folding a long `Signed-off-by:` moves the author out
of reach of `checkSignOff` and fails a different way. Anyone whose name
and address ran past 57 characters could not write a commit this would
accept. The trailer block is exempt now; prose is not, including a last
paragraph that only looks like trailers and so holds no trailers at all.

A folded trailer was split while a landing message was composed. Each
line was read on its own, so the token line went to the trailers and
the indented continuation stayed in the body: `Co-authored-by:` written
over two lines landed with the address left behind. The message still
validated, so the queue merged it and the attribution was quietly lost.
A continuation is attached to the trailer above it now, rather than
pushed as an entry of its own, which would have let the sort move it
away from what it belongs to.

The pull request template told contributors to paste the emoji into the
description. It belongs in the title, which is the subject that lands
and the only place anything reads it.

Signed-off-by: Derek Lewis <DerekNonGeneric@inf.is>
Assisted-by: Claude-Code:claude-opus-5
@coderabbitai

coderabbitai Bot commented Sep 10, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 0932b052-cee2-4303-b3ad-47d7254cf84d

📥 Commits

Reviewing files that changed from the base of the PR and between 1ac4cb4 and 99a268b.

📒 Files selected for processing (6)
  • .github/PULL_REQUEST_TEMPLATE.md
  • build/shared/commit-message.mts
  • build/shared/commit-message.test.mts
  • build/shared/landing.mts
  • build/shared/landing.test.mts
  • build/tasks/land-pull-request.mts

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The changes update commit trailer validation and folded trailer parsing, fix paginated commit retrieval, add related tests, and correct the pull request template instruction for emoji placement.

Changes

Commit Message Handling

Layer / File(s) Summary
Commit validation and trailer parsing
build/shared/commit-message.mts, build/shared/commit-message.test.mts, build/shared/landing.mts, build/shared/landing.test.mts
CONTINUATION_LINE is exported. Trailer continuations remain attached to their trailers. Trailer lines are exempt from the body width limit. Tests cover validation, composition, and mixed prose.

Paginated Commit Retrieval

Layer / File(s) Summary
Paginated commit fetching
build/tasks/land-pull-request.mts
Paginated results are combined before TypeScript flattens the pages, removes merge commits, and extracts commit messages.

Pull Request Guidance

Layer / File(s) Summary
Template instruction
.github/PULL_REQUEST_TEMPLATE.md
The emoji instruction now directs contributors to copy the emoji into the title.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 99a26

The landing workflow fixes are covered by regression tests, with no actionable merge-blocking risk identified.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the pull request by identifying four fixes in the pull-request landing workflow. The emojis add minor noise but do not make the title unclear or unrelated.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 5 files. (1 skipped: 1 …
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch infra/commit-queue-defects

Warning

Some tools did not complete. Review the errors below.

🔧 markdownlint-cli2 (0.23.2)
.github/PULL_REQUEST_TEMPLATE.md

markdownlint-cli2 v0.23.2 (markdownlint v0.41.1)
Finding: :.github/PULL_REQUEST_TEMPLATE.md !lib/ !node_modules/ !.pnpm-store/ !**/COPYING.md !LICENSE/
AggregateError: Unable to import module 'markdown-it-task-lists'.
at importModule (file:///usr/local/lib/node_modules/markdownlint-cli2/markdownlint-cli2.mjs:95:11)
at async Promise.all (index 0)
at async Promise.all (index 1)
at async createDirInfos (file:///usr/local/lib/node_modules/markdownlint-cli2/markdownlint-cli2.mjs:629:3)
at async main (file:///usr/local/lib/node_modules/markdownlint-cli2/markdownlint-cli2.mjs:1065:5)
at async file:///usr/local/lib/node_modules/markdownlint-cli2/markdownlint-cli2-bin.mjs:14:22 {
[errors]: [
Error: Cannot find module 'markdown-it-task-lists'
Require stack:
- /usr/local/lib/node_modules/markdownlint-cli2/node_modules/markdownlint/lib/resolve-module.cjs
at Module._resolveFilename (node:internal/modules/cjs/loader:1476:15)
at wrapResolveFilenam

... [truncated 1052 characters] ...

Resolution (node:internal/modules/esm/resolve:271:11)
at moduleResolve (node:internal/modules/esm/resolve:861:10)
at defaultResolve (node:internal/modules/esm/resolve:988:11)
at #cachedDefaultResolve (node:internal/modules/esm/loader:697:20)
at #resolveAndMaybeBlockOnLoaderThread (node:internal/modules/esm/loader:714:38)
at ModuleLoader.resolveSync (node:internal/modules/esm/loader:746:52)
at #resolve (node:internal/modules/esm/loader:679:17)
at ModuleLoader.getOrCreateModuleJob (node:internal/modules/esm/loader:599:35)
at node:internal/modules/esm/loader:628:32
at TracingChannel.tracePromise (node:diagnostics_channel:362:14) {
code: 'ERR_MODULE_NOT_FOUND',
url: 'file:///markdown-it-task-lists'
}
]
}


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@DerekNonGeneric DerekNonGeneric added the 🚀 Status: Commit Queue Land this pull request when its checks pass label Sep 10, 2026
@openinf-commit-queue
openinf-commit-queue Bot merged commit 87075c9 into main Sep 10, 2026
11 checks passed
@openinf-commit-queue openinf-commit-queue Bot removed the 🚀 Status: Commit Queue Land this pull request when its checks pass label Sep 10, 2026
@openinf-commit-queue
openinf-commit-queue Bot deleted the infra/commit-queue-defects branch September 10, 2026 23:32
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.

1 participant