Add the repin bot - #56
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e3fecc99b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| repin: | ||
| name: Repin against the current base tag | ||
| # A preflight that passed has nothing to fix. | ||
| if: ${{ github.event_name == 'workflow_dispatch' || github.event.workflow_run.conclusion == 'failure' }} |
There was a problem hiding this comment.
The automatic path never runs for the pin conflicts it is intended to repair. .github/workflows/unsloth-pin-preflight.yml records a status=failure output and opens an issue, but neither that step nor prebuilt-alert exits nonzero, so the workflow conclusion remains success; this condition therefore skips the repin job on every detected merge conflict. Trigger based on the preflight's reported status or make the preflight workflow actually fail.
Useful? React with 👍 / 👎.
| m = run(["git", "-c", "merge.conflictStyle=diff3", *git_id, "merge", "--no-ff", | ||
| "--no-edit", "-m", f"Merge {base} into {head_ref}", f"refs/tags/{base}"], |
There was a problem hiding this comment.
Each branch is merged only with the base tag, whereas the checked preflight merges pins sequentially and reports a failure against the base plus all earlier pins. If two pinned PRs conflict with each other but each merges cleanly with the base, this code creates new SHAs without resolving the actual conflict and can report a successful proposal that still fails the nightly. The repin operation must reproduce the cumulative merge topology used by the preflight.
Useful? React with 👍 / 👎.
| + ", ".join(sorted(shared)[:3]) | ||
| ) | ||
| # Upstream first, then ours: the same order a human repin produces. | ||
| return list(theirs) + list(ours) |
There was a problem hiding this comment.
An empty diff3 base and disjoint line text do not prove that two insertions can safely be concatenated. For example, branches that insert mode = 1; and mode = 2; at the same location satisfy these checks, but the fixed theirs-then-ours order silently selects one behavior; mutually exclusive control-flow additions have the same problem. Since the workflow commits and pushes this resolution directly to owned branches, the resolver should restrict automation to a structurally validated pattern rather than unioning every insertion-only conflict.
Useful? React with 👍 / 👎.
| set -uo pipefail | ||
| python3 scripts/unsloth/repin.py \ |
There was a problem hiding this comment.
Because this step enables -u and pipefail but not -e, a nonzero exit from repin.py does not fail the step: the subsequent jq reads can also fail, and the final successful echo leaves steps.repin.outcome as success. The Report step explicitly relies on that outcome to detect crashes, so malformed input or an unexpected runtime error is instead reported as a successful run with no actionable alert.
Useful? React with 👍 / 👎.
| git push -q --force \ | ||
| "https://x-access-token:${REPIN_TOKEN}@github.com/${GITHUB_REPOSITORY}.git" \ | ||
| "HEAD:refs/heads/${REPIN_BRANCH}" 2>&1 | sed "s/${REPIN_TOKEN}/***/g" |
There was a problem hiding this comment.
If this push fails because of token permissions, branch protection, or a transient remote error, pipefail exposes the failure but the absence of set -e lets execution continue. The workflow can then edit or create a PR against a missing or stale unsloth/auto-repin branch and finally emit mode=pushed, causing the Report step to declare success even though the proposed pin changes were never published.
Useful? React with 👍 / 👎.
| CHANGED="$(jq -r '.changed' "${RUNNER_TEMP}/repin.json")" | ||
| BLOCKED="$(jq -r '[.results[] | select(.action == "conflict" or .action == "third-party")] | length' "${RUNNER_TEMP}/repin.json")" |
There was a problem hiding this comment.
The blocked count excludes every skip, even when skipping is the condition that caused preflight to fail and explicitly requires human action. For example, a required pin whose PR was closed, a force-pushed-away commit, or a branch head that moved past the reviewed SHA is returned as action="skip"; with no automatic changes this produces changed=0, blocked=0, and a successful Report status instead of alerting that the pin remains unusable. Expected no-op cases and refusal/error skips need distinct classifications.
Useful? React with 👍 / 👎.
| repo = work / f"r{pin['num']}" | ||
| run(["git", "clone", "-q", "--filter=blob:none", "--no-checkout", | ||
| f"https://github.com/{pin['src']}.git", str(repo)]) |
There was a problem hiding this comment.
Scratch clone names use only the PR number, although pr-set.json permits PRs from both ggml-org/llama.cpp and unslothai/llama.cpp. If both repositories have a pinned PR with the same number, the first clone creates r<N> and the second clone fails because that directory already exists, so a valid pin is skipped solely due to a cross-repository number collision. Incorporate the source owner or repository into the directory name.
Useful? React with 👍 / 👎.
| while read -r path head_repo head_ref new_sha; do | ||
| echo "pushing ${new_sha:0:10} to ${head_repo}:${head_ref}" | ||
| # No -x anywhere in this step; the URL carries the token. | ||
| if ! git -C "$path" push \ | ||
| "https://x-access-token:${REPIN_TOKEN}@github.com/${head_repo}.git" \ | ||
| "HEAD:refs/heads/${head_ref}" 2>&1 | sed "s/${REPIN_TOKEN}/***/g"; then | ||
| FAILED="${FAILED} ${head_repo}:${head_ref}" | ||
| fi |
There was a problem hiding this comment.
Branch pushes occur sequentially, but any later failure causes the workflow to exit before proposing any pin updates. Branches pushed earlier in the loop remain advanced; on the next run their API head no longer equals the old pinned SHA, so repin_one refuses them and the bot has lost the new SHA needed to update pr-set.json. A transient failure on one repository can therefore strand successfully resolved pins and require manual recovery; retain and propose successful push results or avoid leaving them unrecorded.
Useful? React with 👍 / 👎.
| if not head_repo.startswith(OWNED): | ||
| out["action"] = "third-party" | ||
| out["note"] = f"`{head_repo}` is not ours; ask the author to merge master" | ||
| return out |
There was a problem hiding this comment.
The only authorization check is ownership of the head repository; it does not verify that head_ref is a disposable feature branch. If an owned PR is opened from unslothai/llama.cpp:master or danielhanchen/llama.cpp:master, the later push fast-forwards that default branch directly with the generated merge commit, despite the workflow's stated guarantee that master is touched only through a human-reviewed PR. Compare the head ref with the repository's default branch or use an explicit branch allowlist before permitting the push.
Useful? React with 👍 / 👎.
1 participant
Four nightlies failed in a week, every one of them a pinned PR that merged yesterday and stopped merging today because the base tag moved under it. Preflight (#53) tells us that a few hours early. This does the other half: the merge itself.
What it does
On a preflight failure, for each pin in
scripts/unsloth/pr-set.json:It opens the PR and stops. It does not merge it.
What it refuses to do
The pin file exists so that only reviewed code ships, so the bot is written to be unable to widen it:
pwilkin/llama.cpp(model: add Kimi-K3 text model ggml-org/llama.cpp#26185) gets a report line, never a push. Onlyunslothai/*anddanielhanchen/*are writable.The add/add rule
scripts/unsloth/additive_merge.pyre-reads the conflict in diff3 style and resolves a hunk only when the merge base section is empty, which means both sides added text where there was none and neither edited the other's. That is exactly the recurringcase LLM_ARCH_FOO:fallthrough collision. It additionally refuses when the two sides share a line, because that is one change made twice rather than two independent additions.Anything else, including the common case of two PRs each appending a term to the same
ifcondition, is left conflicted and reported. It never picks a side.Decisions are made for every conflicted file before any of them is written, so a refusal on the second file cannot leave the first one half-resolved.
Verification
scripts/unsloth/test_additive_merge.pybuilds real git conflicts and covers the resolve, the refusals, the partial-write guard and the dry run. 14 assertions, all passing.The one that matters is the replay of the actual 08-02 breakage: base
b10229, Inkling pinned at02142bbc, colliding with upstream's newLLM_ARCH_DEEPSEEK4. The resolver reproduces the resolution I made by hand in1e6f9e4abyte for byte.Ran end to end against the live repos on today's base
b10236:ggml-org#24423ggml-org#25731unslothai#48src/llama-arch.cpp,DEEPSEEK4/INKLING), repinnedggml-org#26185REPIN_TOKEN
The plan was for the bot to work without a
workflow-scoped token. It cannot: pushing todanielhanchen/llama.cppis cross-repo, whichGITHUB_TOKENcannot do at all, and the merges carry upstream's own.github/workflowschanges, which needs workflow write. Dropping those paths from the merge would leave the branch permanently diverged there, which is worse.So it needs a
REPIN_TOKENsecret with contents and workflow write onunslothai/llama.cppanddanielhanchen/llama.cpp. Until that exists the bot still runs every step except the push, and files the report as an issue, so nothing here is blocked on the secret. Note it only ever pushes to feature branches and to its own proposal branch;masteris only ever touched through a PR a human merges.