github: make the review link in the PR-body stack footer configurable by netproteus · Pull Request #1410 · facebook/sapling

netproteus · 2026-08-09T20:56:54Z

Closes #1409.

sl pr submit hard-codes https://reviewstack.dev/... as the "Best reviewed with" link in the stack footer. Teams on GitHub Enterprise (#512), running a self-hosted ReviewStack fork (#304), or using another stack-aware review tool get a wrong or dead link in every PR. The ReviewStack source itself anticipates alternate hosts (saplingStack.ts: the URL "can change over time (particularly if someone wants to run their own fork of ReviewStack)").

This PR adds two configs:

[github]
pull-request-review-url-template=https://review.example.com/{owner}/{repo}/pull/{number}
pull-request-review-tool-name=MyReview

producing:

Stack created with Sapling. Best reviewed with MyReview.

Design notes

Unset ⇒ byte-for-byte today's output. The three existing test-ext-github-pr-submit-*.t tests pass unchanged, which doubles as the regression proof.
The URL template uses str.format placeholders ({owner}, {repo}, {number}, {hostname}) rather than the full Sapling templater: it keeps create_pull_request_title_and_body() a pure, doctest-able function and matches how the URL is already written. Happy to switch to the templater (as github.pr.branch-name-template does) if you'd prefer.
The sentence shape is deliberately not configurable. parse_stack_information() only tolerates the intro line because it starts with the literal Stack created with [Sapling]; free-form text would also risk corrupting ReviewStack's bullet parsing (* -prefixed lines, bare ---, __->__). Keeping the fixed sentence means even old sl binaries parse customized bodies correctly.
An invalid URL template aborts at submit time with a message naming the config.
github.pull-request-include-reviewstack=false still suppresses the whole line; the new configs are no-ops in that case.
Config resolution happens at the call boundary (submit.py: ui.config(...) or DEFAULT_...), so the library function and the test mocks take non-Optional values; the defaults are exported as DEFAULT_REVIEW_URL_TEMPLATE / DEFAULT_REVIEW_TOOL_NAME.

Follow-up

The footer-parser hardening that was originally a second commit here (tolerating free-text introduction lines after the modern footer marker) is now its own PR per review feedback: #1419. The two are independent.

Test plan

New doctests in pull_request_body.py (custom URL, custom name, both, invalid-template abort, round-trip parse of a customized footer) — module doctests all pass.
New eden/scm/tests/test-ext-github-pr-submit-footer.t + tests/github/mock_create_prs_footer.py: the mock server computes the expected footer from the same configs the test sets, so it only matches if the templates were applied. mock_utils.expect_update_pr_request() now takes required keyword-only review_url/review_tool args; all mock callers construct them from the exported defaults via _format_review_url().
Existing test-ext-github-pr-submit-{single,overlap,open}.t pass unchanged.
All four .t tests ran green against an OSS make oss-equivalent build (build.py --oss sl) in a manylinux_2_34 container on the initial submission; module doctests re-verified after each review revision.

🤖 Generated with Claude Code

https://claude.ai/code/session_01UaQdFpGUAEQSBSqHwtQCZ1

meta-codesync · 2026-08-09T21:01:12Z

This pull request has been imported. If you are a Meta employee, you can view this in D115372413. (Because this pull request was imported automatically, there will not be any future comments.)

zzl0

Thanks for contributing to Sapling. Looks good overall, I just have a few comments on the implementation details.

zzl0 · 2026-08-18T15:44:31Z

+pull-request-review-tool-name=MyReview
+```
+
+The URL template supports the `{owner}`, `{repo}`, `{number}`, and `{hostname}` placeholders. When these configs are unset, the footer links to reviewstack.dev as before. Setting `github.pull-request-include-reviewstack=false` removes the review link line entirely.


pull-request-review-url-template=https://review.example.com/{owner}/{repo}/pull/{number}
The URL template supports the {owner}, {repo}, {number}, and {hostname} placeholders.

Should we also mention {hostname} (add its default value) in the example url template?

Good idea — documented {hostname} with its default expansion (the repo's GitHub host; the upstream's host when submitting from a fork) and added a second example template that uses it.

zzl0 · 2026-08-18T15:56:26Z

        owner: str = OWNER,
        name: str = REPO_NAME,
        stack_pr_ids: Optional[List[int]] = None,
+        review_url: Optional[str] = None,


How do you think about making both review_url and review_tool required values? Callers should construct them before calling this function:

On the caller side, we can do the following to make sure url_template is always not None: use

url_template = ui.config("github", "pull-request-review-url-template") or <default_tempalte>

instead of

url_template = ui.config("github", "pull-request-review-url-template")

Done — review_url and review_tool are now required keyword-only args on expect_update_pr_request with the internal defaulting removed; all four mock callers construct them explicitly, and mock_create_prs_footer.py uses exactly the ui.config(...) or DEFAULT_... pattern you suggested (the defaults are now public constants exported from pull_request_body). I applied the same pattern at the real call site in submit.py.

One nuance: create_pull_request_title_and_body's params became non-Optional str with the constants as keyword defaults rather than fully required — that removes all the None handling while keeping the existing doctests unchanged. Happy to make them required there too if you'd prefer.

zzl0 · 2026-08-18T16:05:29Z

+            hostname = (
+                repository.upstream.hostname
+                if repository.upstream
+                else repository.hostname
+            )


Could you help add a comment for this logic?

Added — it mirrors get_upstream_owner_and_name() just above: PRs live in the upstream repository, so {hostname} expands from the upstream when submitting from a fork; for non-fork clones it's the same host.

zzl0 · 2026-08-18T16:13:00Z

+    Text after the bullet list still ends the list:
+    >>> body = (
+    ...     '[//]: # (BEGIN SAPLING FOOTER)\n' +
+    ...     'Introduction.\n' +
+    ...     '* #1\n' +
+    ...     'Not a stack entry.\n' +
+    ...     '* #2\n')
+    >>> parse_stack_information(body)
+    [(False, 1)]


Why do you want to support the free-text case? This looks a bit confusing.

Two reasons: (a) it fixes an existing silent failure — if anyone edits the footer prose on github.com today, parse_stack_information returns [] and sl pr pull silently stops linking stack ancestors; (b) it matches the grammar ReviewStack's parser already implements — eden/contrib/reviewstack/src/saplingStack.ts treats non-bulleted lines after the marker as introduction prose, and its design comment anticipates this text changing over time ("particularly if someone wants to run their own fork of ReviewStack"), which is exactly what the first commit enables.

The skip only activates after the modern [//]: # (BEGIN SAPLING FOOTER) marker; legacy marker-less bodies keep the conservative behavior (there's a doctest pinning that).

That said, it's deliberately a separate commit — happy to split it into its own PR if you'd rather land the config change alone.

it fixes an existing silent failure — if anyone edits the footer prose on github.com today, parse_stack_information returns [] and sl pr pull silently stops linking stack ancestors;

Could you explain this a bit more about the "if anyone edits the footer prose on github.com"?

That said, it's deliberately a separate commit — happy to split it into its own PR if you'd rather land the config change alone.

Yeah, let's split it into its own PR. Sapling prefer small PRs: one PR focus on one thing.

Sure — the footer is part of the PR description, so anything that edits the description on github.com can perturb it: an author adding a test plan from the web UI, a reviewer fixing a typo, a bot that rewrites PR bodies. If that leaves a non-bullet line between the footer marker and the bullet list, parse_stack_information returns [] and the only symptom is that sl pr pull silently stops linking the stack — no error. (GitHub also rewrites line endings to \r\n on web edits, which the parser already tolerates; this is the same class of problem one level up.)

Agreed on the split — I've dropped that commit from this PR (now just the config change) and opened it as #1419.

facebook-github-tools · 2026-08-18T17:32:24Z

@netproteus has updated the pull request. You must reimport the pull request before landing.

netproteus · 2026-08-18T17:36:20Z

@zzl0 thanks for the review, I've addressed your comments please let me know if you have any follow ups

zzl0 · 2026-08-18T18:52:45Z

+
+```ini
+[github]
+pull-request-review-url-template=https://review.example.com/{hostname}/{owner}/{repo}/pull/{number}


Is this example correct? I thought it should behttps://{hostname}/{owner}/{repo}/pull/{number}?

The host in the template is the review tool itself rather than GitHub — the intent is a single review-tool instance serving repositories on more than one GitHub host, using {hostname} in the path to tell them apart. I've reworded the docs and renamed the example host to reviewstack.example.com to make that clearer. (https://{hostname}/{owner}/{repo}/pull/{number} also works if a team wants the footer to link straight back to the PR page itself.)

zzl0 · 2026-08-18T18:55:22Z

+        review_url_template=ui.config("github", "pull-request-review-url-template")
+        or DEFAULT_REVIEW_URL_TEMPLATE,


Could we have a separate variable for ui.config("github", "pull-request-review-url-template") or DEFAULT_REVIEW_URL_TEMPLATE?

Done — extracted review_url_template and review_tool_name as locals before the call.

zzl0 · 2026-08-18T18:58:12Z

+            review_url=f"https://reviewstack.dev/{OWNER}/{REPO_NAME}/pull/{num}",
+            review_tool="ReviewStack",


How do you think about reusing DEFAULT_REVIEW_URL_TEMPLATE and DEFAULT_REVIEW_TOOL_NAME constants, and the _format_review_url() function?

Done — the mock callers now build the expected URL with _format_review_url(DEFAULT_REVIEW_URL_TEMPLATE, ...) and pass DEFAULT_REVIEW_TOOL_NAME; the footer mock routes its configured template through _format_review_url as well.

The footer that `sl pr submit` writes for stacked PRs hard-codes https://reviewstack.dev as the "Best reviewed with" link. Teams running their own ReviewStack instance, another stack-aware review tool, or GitHub Enterprise (where reviewstack.dev cannot access the repo) end up with a wrong or dead link in every PR they create. Add two configs: [github] pull-request-review-url-template=https://review.example.com/{owner}/{repo}/pull/{number} pull-request-review-tool-name=MyReview The URL template supports {owner}, {repo}, {number} and {hostname} placeholders. When both configs are unset the output is byte-for-byte identical to today's. The fixed "Stack created with [Sapling]..." sentence prefix is deliberately kept so parse_stack_information() (used by `sl pr pull`), ReviewStack's client-side parser, and legacy pre-marker bodies all keep parsing unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Change-Id: I96ab2d9dd8bba9921ca897521bc4fd5d24acbce3

facebook-github-tools · 2026-08-19T00:36:08Z

@netproteus has updated the pull request. You must reimport the pull request before landing.

netproteus · 2026-08-19T00:59:19Z

@zzl0 thanks again, let me know if i'm on the right track?

zzl0

Thanks for updating the PR, LGTM. Just one minor comment. Once you update it, I will land the diff internally.

zzl0 · 2026-08-26T13:47:51Z

+
+```ini
+[github]
+pull-request-review-url-template=https://reviewstack.example.com/{hostname}/{owner}/{repo}/pull/{number}


Should the example be pull-request-review-url-template=https://{hostname}/{owner}/{repo}/pull/{number}?

meta-cla Bot added the CLA Signed label Aug 9, 2026

zzl0 self-requested a review August 18, 2026 15:38

zzl0 reviewed Aug 18, 2026

View reviewed changes

netproteus force-pushed the footer-config branch from 3181bcf to 062ef11 Compare August 18, 2026 17:32

zzl0 reviewed Aug 18, 2026

View reviewed changes

netproteus force-pushed the footer-config branch from 062ef11 to 5db1206 Compare August 19, 2026 00:36

netproteus mentioned this pull request Aug 19, 2026

github: tolerate free-text introduction lines in the stack footer parser #1419

Open

zzl0 reviewed Aug 26, 2026

View reviewed changes

		review_url_template=ui.config("github", "pull-request-review-url-template")
		or DEFAULT_REVIEW_URL_TEMPLATE,

		review_url=f"https://reviewstack.dev/{OWNER}/{REPO_NAME}/pull/{num}",
		review_tool="ReviewStack",

Uh oh!

Conversation

netproteus commented Aug 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Design notes

Follow-up

Test plan

Uh oh!

meta-codesync Bot commented Aug 9, 2026

Uh oh!

zzl0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-tools Bot commented Aug 18, 2026

Uh oh!

netproteus commented Aug 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-tools Bot commented Aug 19, 2026

Uh oh!

netproteus commented Aug 19, 2026

Uh oh!

zzl0 left a comment

Choose a reason for hiding this comment

Uh oh!

zzl0 Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

netproteus commented Aug 9, 2026 •

edited

Loading

zzl0 Aug 26, 2026 •

edited

Loading