Conversation
…PR path A Forgejo pull request URL carries the plural /pulls/ segment, which fm_pr_url_parse did not accept, so bin/fm-pr-check.sh and bin/fm-pr-merge.sh both refused one outright. Nothing could record pr= or pr_head=, no merge poll could be armed, and merging had to happen outside the guard entirely. Add forgejo as a third provider across the PR path: - fm_pr_url_parse gets its own host, owner, and repository rules rather than a loosened GitHub or GitLab rule. The shared lowercase-DNS host shape moves into fm_pr_dns_host_valid so it is stated once; GitLab's accepted set is unchanged. github.com and gitlab.com are refused as Forgejo hosts for the same reason github.com is already refused as a GitLab one. - bin/fm-pr-poll.sh reads forgejo-axi pr merged and wakes only on an exact merged proof. The poll source stays byte-for-byte identical for every task and the identity stays in the private sidecar. - bin/fm-pr-merge.sh verifies one live mergeability read, reports every failing condition plus the forge's own reasons, and binds the merge to the verified head with forgejo-axi pr merge --expected-head. Both the poll and the merge pass the host from the validated record as --base-url. forgejo-axi reads only owner/repository/number out of a pull request URL and still sends the request to whatever host its own configuration resolves, so passing the URL alone would let an ambient default answer for the host the record names. An absent forgejo-axi refuses at arming and at merge rather than watching nothing, matching how the GitLab path handles an absent glab. docs/forgejo-merge-watch.md records the live evidence against a public Forgejo instance.
…unmerged Forgejo proof
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Reviews (2): Last reviewed commit: "fix(bin): refuse Forgejo repository name..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| fm_pr_forgejo_repo_valid() { | ||
| local repo=${1-} | ||
| local LC_ALL=C | ||
| [ "${#repo}" -ge 1 ] && [ "${#repo}" -le 100 ] || return 1 | ||
| case "$repo" in | ||
| .|..|*[!A-Za-z0-9._-]*|*.git|*.wiki|*.rss|*.atom) return 1 ;; | ||
| esac | ||
| } |
There was a problem hiding this comment.
When a Forgejo URL uses - as its repository name or contains consecutive dots, the new validator accepts the identity and arms a poll for a repository Forgejo cannot host, causing a watch that can never wake. Apply the same rejection in the duplicated poll validation.
|
Thanks - this is a good catch, and it is half right. I checked both halves against Forgejo's own source before acting, so here is the reasoning on the record.
reservedRepoNames = []string{".", "..", "-"}
reservedRepoPatterns = []string{"*.git", "*.wiki", "*.rss", "*.atom"}I reject Consecutive dots: I do not think this one holds, so I am not applying it. Repository names are checked with // AlphaDashDotPattern characters prohibited in a user name (anything except A-Za-z0-9_.-)
AlphaDashDotPattern = regexp.MustCompile(`[^\w-\.]`)That prohibits out-of-set characters only; there is no consecutive-character rule for repositories. The One more in the same class, which I found while checking the above. name = strings.TrimSpace(strings.ToLower(name))The reserved suffixes are therefore case-insensitive on the forge, while my Both fixes land in One deliberate omission, flagged so it reads as a decision rather than an oversight: I am not encoding Forgejo's reserved username list for the owner segment. That list is forge policy that varies by version, so baking it in would rot into refusing owners the forge accepts. |
An upstream review flagged that a Forgejo URL naming "-" as its repository
parsed and armed a merge poll. Forgejo reserves that name outright, so the
poll watched something the forge cannot host and could never wake - the
silent failure this validation exists to prevent.
Forgejo's own rules, checked against its source rather than inferred:
reservedRepoNames = []string{".", "..", "-"}
reservedRepoPatterns = []string{"*.git", "*.wiki", "*.rss", "*.atom"}
"." and ".." were already refused; "-" was not. IsUsableName also lowercases
before comparing, so the reserved suffixes are case-insensitive on the forge
while the patterns here were not: "repo.GIT" and "repo.Wiki" were accepted.
Both gaps are fixed, in bin/fm-pr-lib.sh and in bin/fm-pr-poll.sh, which
re-validates rather than trusting its sidecar. Each side now points at the
other so a later change cannot fix one and miss it.
Two things deliberately left alone, with the reasoning in the comments:
The same review claimed consecutive dots are invalid. They are not.
AlphaDashDotPattern is [^\w-\.] and carries no consecutive-character rule for
repositories; that restriction is invalidUsernamePattern's, and it already
applies to the owner segment here. Refusing "a..b" would make this stricter
than the forge and reject a repository it can genuinely host, which is a worse
defect than the one reported. A test now pins that, so the claim cannot be
adopted later by accident.
The forge's reserved USERNAME list stays unencoded for the owner. It is
version-varying policy that would rot into refusing owners the forge accepts,
unlike "-", which can never name a hostable repository.
|
Fixed in
The new rejections were confirmed to fail against the pre-fix code and pass after it, rather than only passing once the change was in. Thanks for the review - the |
|
Speaking as Kun's firstmate: Reviewed HEAD Class: opt-in. Adds VISION.md per-rule (inspected
Attestation: mismatch. Body Fork CI: approved this pass on this HEAD only: Overlap hold: open #3068 (Gitea/Forgejo forge support) and #3009 (Gitea |
2 participants
Why
firstmate's guarded PR path speaks GitHub and GitLab.
fm_pr_url_parseacceptsgithub.com/OWNER/REPO/pull/Nand GitLab's.../-/merge_requests/N, and anything else is refused.That leaves self-hosted Forgejo out. A Forgejo pull request URL is
https://HOST/OWNER/REPO/pulls/N- note the pluralpulls- sobin/fm-pr-check.shandbin/fm-pr-merge.shboth refuse it outright. For anyone whose projects live on a Forgejo instance, that means nopr=orpr_head=can be recorded, no merge poll can be armed, and merging has to happen by calling the forge CLI directly, outside the guarded path entirely.This adds
forgejoas a third provider across that path, following the shape the GitLab provider already established.What this adds
Parsing (
bin/fm-pr-lib.sh). Aforgejobranch infm_pr_url_parsewith its own host, owner, and repository rules rather than a loosened GitHub or GitLab rule. The owner and repository rules follow Forgejo's own validation (owner: at most 40 characters, starts alphanumeric, allows-,_,., no run of two of those and no trailing one; repository: at most 100 characters over the same set, never.or.., never ending in a reserved.git/.wiki/.rss/.atomsuffix).The shared lowercase-DNS host shape that GitLab and Forgejo both need moves into one
fm_pr_dns_host_validhelper so it is stated once. GitLab's accepted set is unchanged by that extraction, and the parser matrix asserts it.The three providers are told apart by the segment the others cannot carry: GitHub's own host plus
/pull/, GitLab's/-/merge_requests/, and Forgejo's plural/pulls/.github.comandgitlab.comare refused as Forgejo hosts for the same reasongithub.comis already refused as a GitLab one - they are those forges' own hosts and never a Forgejo instance, sohttps://github.com/o/r/pulls/1would otherwise arm a watch that can never succeed.Polling (
bin/fm-pr-poll.sh). Aforgejobranch readingforgejo-axi pr merged, waking only on an exact merged proof. The poll source stays byte-for-byte identical for every task and the identity stays in the private sidecar, exactly as the existing contract requires. The sidecar and registration formats were already provider-generic (provider,url,host,path,number), so a third provider needed no format change and none was made.Merging (
bin/fm-pr-merge.sh). Aforgejobranch built onforgejo-axi pr merge --expected-head, which re-reads the pull request before merging, sends the verified commit as the merge request'shead_commit_id, and refuses to report a merge it cannot prove landed at that exact commit - the same head-binding guaranteeglab mr merge --shagives the GitLab path.Before that, one live
forgejo-axi pr mergeabilityread has to report the forge's own mergeable verdict and passing checks at the current head. Every failing condition is reported rather than only the first, along with the forge's own reasons. A recordedpr_head=that disagrees with the live head is reported, not trusted, because a rebase leaves it stale.Two details worth calling out
The instance is passed explicitly, never inferred from the URL.
forgejo-axiaccepts a pull request URL, but it reads only the owner, repository, and number out of one and still sends the request to whatever host its own configuration resolves. Aforgejo.exampleURL answering fromcodeberg.orgdemonstrates it:Both the poll and the merge therefore pass the validated record's host as
--base-urland the parsed pair as--repo, and never hand a URL to the CLI. This mirrors how the GitLab path usesGITLAB_HOSTplus-R <project URL>, and it is asserted by tests.A merge method is named here, unlike on GitLab. GitLab's merge API applies the project's own merge method, so imposing one there would override that convention. Forgejo's merge API takes the method in the request body and has no such fallback, so some method is always chosen. This path names the same
--squashdefault GitHub gets rather than inheritingforgejo-axi's ownmergedefault, and a caller who wants another passes-- --method mergeor-- --method rebase.GitHub's
--squash,--mergeand--rebasespellings are not flagsforgejo-axitakes, so on the Forgejo path they are refused by name before anything is recorded, naming the--methodform instead. Forwarding them would first suppress this path's own--method squashdefault and then fail at the CLI as an unknown flag, after the live pre-merge read had already run.Degrading honestly
The poll is silent on every error by design, so a missing
forgejo-axiwould be indistinguishable from a pull request that never merges. Arming is the one point where that can be reported, sobin/fm-pr-check.shrefuses there rather than arming a watch that sees nothing - the same stance the GitLab path takes whenglabis absent. Merging additionally needsjq, and either tool missing is reported together before anything is recorded.One consequence is deliberate and documented: a repository whose pull requests report no passing checks cannot merge through this path, exactly as a GitLab project that runs no pipeline cannot. Passing checks at the head is a condition, and "there are no checks" does not satisfy it.
forgejo-axi pr mergeabilityreads commit statuses and branch protection, never the Actions runs API, so an instance that does not serve that API is fully supported.Scope
This stays inside the PR path. The only shared change is the small
fm_pr_dns_host_validextraction described above, made to keep the DNS host rule stated once rather than to restructure the provider abstraction.bin/fm-teardown.sh'spr_is_mergedstill usesgh pr viewand falls back to its provider-agnostic content check for a Forgejo URL. That is the same pre-existing shape GitLab already has, and it is left alone here.How it is tested
Colocated with the existing suites, no new runner:
tests/fm-pr-check-security.test.shgains Forgejo rows in the parser matrix, the plural-versus-singular boundary asserted in both directions, both cross-provider spoof refusals, around thirty new malformed-URL rejection rows covering the owner and repository rules, and atest_forgejo_merge_watchmirroring the existing GitLab one: exact sidecar bytes, only an exact merged proof wakes, silence on CLI failure and on an absent CLI, the arming refusal, doctored-sidecar refusals including a nested project path, addressing by--base-urland never by a URL, andpr_head=recording.tests/fm-pr-merge.test.shgains fourteen Forgejo merge tests covering the merge invocation, the instance coming from the URL, the squash default and an explicit method, the refusal of GitHub's method spellings, each pre-merge condition independently plus all of them together with the forge's reasons, a view that answered for another pull request, a stale recorded head, unreadable state, an invalid head, merge failure propagation, unconfirmed landings leaving the poll armed, missing tools, and rejected override arguments.Every new guard was mutation-tested: each one was deliberately broken, the specific test confirmed to turn red, and the code restored to green.
docs/forgejo-merge-watch.mdrecords the live evidence, alongside the existingdocs/gitlab-merge-watch.md. It reads only public pull requests oncodeberg.org, so every command in it can be rerun without a credential.bin/fm-lint.sh(ShellCheck 0.11.0, actionlint 1.7.12) andbin/fm-doc-audience-check.share clean, andtests/fm-pr-check-security.test.sh(42 ok),tests/fm-pr-merge.test.sh(75 ok), andtests/fm-teardown.test.sh(58 ok) all pass.Happy to adjust any of the choices above if you would rather they went a different way.
Updates from git push no-mistakes