From f20b9780757041b3b34096993fc061274fb0334e Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 31 Aug 2026 11:29:34 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=8E=AF=20ci:=20Retarget=20Pull=20Requests?= =?UTF-8?q?=20Opened=20Against=20`main`=20to=20`dev`=20(#15416)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ๐ŸŽฏ ci: Retarget Pull Requests Opened Against `main` to `dev` Contributions land on `dev`; `main` only moves at release time. With `main` as the repository's default branch, pull requests default to it, and the mismatch has been resolved by hand at merge time. Adds a `pull_request_target` hook that switches a newly opened or reopened pull request's base from `main` to `dev` and posts a one-time explanation, plus a manual `workflow_dispatch` sweep (dry-run by default) for the existing backlog. Release-bound upstream branches keep `main` as their base: `dev`, `release/*`, `hotfix/*`, `backport/*` and `*-main`. The `target: main` label exempts any pull request. The exemptions apply only to branches on this repository, so a fork branch that happens to match one of those names is still retargeted. The job checks out the trusted base commit rather than the pull request head, so the write-scoped token never runs contributor code. `pull_request_target` reads its workflow definition from the base branch, so this must exist on `main` to take effect. * ๐Ÿ“ docs: Record the `dev`-First Branch Policy for Agents `CLAUDE.md` and `AGENTS.md` never stated which branch work targets, and the default branch is `main` โ€” so `gh pr create` and the GitHub UI point at `main` unless told otherwise, and an agent following the defaults gets it wrong. States the policy, the reason a backport to `main` is never needed (`main` is kept as a strict ancestor of `dev`), and the exemptions the retarget workflow honors. * ๐Ÿ“ docs: Record Typecheck, Formatting and Issue-Closing Gotchas for Agents Four repeat cycle-wasters that neither file stated, each verified against the current tree: - `tsdown` emits without typechecking, so `npm run build` proves nothing about types in `packages/api`, `packages/client` or `packages/data-schemas`; only `packages/data-provider` runs `tsc` in its build. - `packages/client/tsconfig.json` excludes `*.spec.ts(x)` and `*.test.ts(x)`, so type errors in its specs surface only at run time. - `npm run sort-imports` with no arguments rewrites all six source roots. - `Fixes #N` never closes an issue here, because closing keywords are honored only on merges into the default branch and everything merges to `dev`. Also notes that worktrees share one stash stack, and points at `npm run static-checks -- --against origin/dev` as the local mirror of CI. * ๐Ÿ” fix: Address Codex Review on the Retarget Workflow - **Backport branches are no longer exempt.** Exempting `backport/*` and `*-main` contradicted the invariant the policy rests on: `main` is a fast-forward of `dev`, so a backport merged straight to `main` is precisely what breaks it. `target: main` remains the deliberate, explicit escape hatch; `dev`, `release/*` and `hotfix/*` stay exempt as release-bound branches. - **`npm run update` does not sync `dev`.** `config/update.js` hard-codes `git checkout main` and `git pull origin main` โ€” it is the self-host deployment updater, not a branch-sync tool. Telling contributors to refresh `dev` with it would have branched them off `main` and recreated the very flow this change exists to prevent. Replaced with the explicit git commands. - **A dry run no longer reports work it did not do.** The summary said "Retargeted N" on a dispatch that wrote nothing, which could read as a completed sweep. - **A failed explanation comment is recorded, not swallowed.** The base edit has already landed at that point, so a later run would skip the pull request and the contributor would never receive the note. It now counts, fails the run, and names the retry: `EXPLAIN_MISSING=true`, which posts a missing explanation onto an already-retargeted pull request. - **The sweep paginates.** `gh pr list --limit 500` is a cap, not a page size, and would have silently ignored the remainder while reporting success. Verified against the stubbed `gh` boundary: `*-main` now retargets, the dry run performs zero writes and says so, a failing comment yields a non-zero exit, and `EXPLAIN_MISSING` posts onto a pull request already based on `dev`. The paginated query returns all 258 open pull requests based on `main`. * ๐Ÿงท fix: Address the Second Codex Round on the Retarget Workflow - **`already_explained` now separates "lookup failed" from "no marker".** A transient comments-API failure returned the same status as a clean miss, so the run would post a second explanation and report success โ€” defeating the deduplication it exists for. A failed read now declines to post and is recorded. - **`CONTRIBUTING.md` syncs from the canonical remote.** The previous fix pulled `dev` from `origin`, which in the fork-based flow described two sections later is the contributor's own fork โ€” so it would fail outright or silently sync a stale branch, landing them on the wrong base again. - **The hook retries on `synchronize`.** If the `opened` run failed before `gh pr edit` landed, nothing retried and the pull request stayed on `main`. A retargeted pull request no longer matches `branches: [main]`, so this adds no redundant runs โ€” and the existing backlog now drains on contributor activity rather than only by sweep. - **Sweeps serialize against each other** via a shared concurrency group, while the hook stays keyed per pull request. Deliberately not one global group: GitHub keeps a single pending job per group, so a burst of openings would cancel queued retargets โ€” trading a rare duplicate comment for dropped retargets is the wrong way round. - **`main` is an ancestor of `dev`, not a strict one.** Right after a sync the two refs are equal, which the previous wording described as a violated invariant. Verified: a failing comments lookup edits the base but posts nothing and exits non-zero. --- .github/CONTRIBUTING.md | 13 ++- .github/scripts/retarget-prs.sh | 155 ++++++++++++++++++++++++++ .github/workflows/pr-retarget-dev.yml | 79 +++++++++++++ AGENTS.md | 18 +++ CLAUDE.md | 41 +++++++ 5 files changed, 302 insertions(+), 4 deletions(-) create mode 100755 .github/scripts/retarget-prs.sh create mode 100644 .github/workflows/pr-retarget-dev.yml diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index 1574efbcd1..1b43ce895d 100644 --- a/.github/CONTRIBUTING.md +++ b/.github/CONTRIBUTING.md @@ -43,7 +43,12 @@ Project maintainers have the right and responsibility to remove, edit, or reject ## 2. Development Notes -1. Before starting work, make sure your main branch has the latest commits with `npm run update`. +1. Before starting work, sync `dev` from this repository. You are working in a fork, so `origin` is + your fork โ€” add the canonical remote once and sync from it: + - `git remote add upstream https://github.com/danny-avila/LibreChat.git` + - `git fetch upstream dev && git checkout -B dev upstream/dev` + - `npm run update` is the self-host deployment updater โ€” it checks out `main` and rebuilds your + containers. Do not use it to refresh a development branch. 2. Run linting command to find errors: `npm run lint`. Alternatively, ensure husky pre-commit checks are functioning. - `npm install` sets the hooks up for you; set `HUSKY=0` to opt out. - The pre-commit hook runs the Static Checks CI job locally, scoped to the files in the commit. Run it by hand with `npm run static-checks`, against a base ref with `npm run static-checks -- --against origin/dev`, or with the slow gates (TypeScript, config migration tests, unused i18n keys, unused npm packages) via `npm run static-checks:full`. @@ -60,11 +65,11 @@ Project maintainers have the right and responsibility to remove, edit, or reject We utilize a GitFlow workflow to manage changes to this project's codebase. Follow these general steps when contributing code: -1. Fork the repository and create a new branch with a descriptive slash-based name (e.g., `new/feature/x`). +1. Fork the repository and branch off `dev` with a descriptive slash-based name (e.g., `new/feature/x`). All contributions target `dev`; `main` only moves at release time, and pull requests opened against it are retargeted automatically. 2. Implement your changes and ensure that all tests pass. 3. Commit your changes using conventional commit messages with GitFlow flags. Begin the commit message with a tag indicating the change type, such as "feat" (new feature), "fix" (bug fix), "docs" (documentation), or "refactor" (code refactoring), followed by a brief summary of the changes (e.g., `feat: Add new feature X to the project`). -4. Submit a pull request with a clear and concise description of your changes and the reasons behind them. -5. We will review your pull request, provide feedback as needed, and eventually merge the approved changes into the main branch. +4. Submit a pull request against `dev` with a clear and concise description of your changes and the reasons behind them. +5. We will review your pull request, provide feedback as needed, and eventually merge the approved changes into the `dev` branch. ## 4. Commit Message Format diff --git a/.github/scripts/retarget-prs.sh b/.github/scripts/retarget-prs.sh new file mode 100755 index 0000000000..f9358b4efc --- /dev/null +++ b/.github/scripts/retarget-prs.sh @@ -0,0 +1,155 @@ +#!/usr/bin/env bash +# Retarget pull requests opened against the release branch onto the development branch. +# Used by .github/workflows/pr-retarget-dev.yml for both the on-open hook and the manual sweep. +set -euo pipefail + +REPO="${REPO:?REPO is required (owner/name)}" +RELEASE_BASE="${RELEASE_BASE:-main}" +TARGET_BASE="${TARGET_BASE:-dev}" +DRY_RUN="${DRY_RUN:-false}" +EXPLAIN_MISSING="${EXPLAIN_MISSING:-false}" +KEEP_LABEL="${KEEP_LABEL:-target: main}" +THROTTLE_SECONDS="${THROTTLE_SECONDS:-0}" +MARKER="" + +if [ "$#" -eq 0 ]; then + echo "usage: REPO=owner/name $0 [pr-number...]" >&2 + exit 64 +fi + +# Branches on the upstream repository that legitimately merge into the release branch. +# Backport branches are deliberately absent: `main` is kept as a fast-forward of `dev`, so a +# backport merged straight to `main` would break that invariant. Use the label to exempt one. +keeps_release_base() { + local head_repo="$1" head_ref="$2" + [ "$head_repo" = "$REPO" ] || return 1 + case "$head_ref" in + "$TARGET_BASE" | release/* | hotfix/*) return 0 ;; + *) return 1 ;; + esac +} + +# 0 = already explained, 1 = not explained, 2 = the lookup itself failed. The third status +# matters: treating a failed read as "not explained" would post a duplicate. Bodies are collected +# before grepping because piping into `grep -q` lets SIGPIPE fail the pipeline under `pipefail`. +already_explained() { + local bodies + bodies="$(gh api "repos/$REPO/issues/$1/comments" --paginate --jq '.[].body')" || return 2 + grep -qF "$MARKER" <<<"$bodies" +} + +comment_body() { + cat </dev/null; then + echo "#$number: skipped โ€” labelled '$KEEP_LABEL'" + skipped=$((skipped + 1)) + continue + fi + + if keeps_release_base "$head_repo" "$head_ref"; then + echo "#$number: skipped โ€” $head_repo:$head_ref is a release-bound branch" + skipped=$((skipped + 1)) + continue + fi + + if [ "$DRY_RUN" = "true" ]; then + echo "#$number: would retarget $RELEASE_BASE -> $TARGET_BASE ($head_repo:$head_ref)" + matched=$((matched + 1)) + continue + fi + + echo "#$number: retargeting $RELEASE_BASE -> $TARGET_BASE ($head_repo:$head_ref)" + if ! gh pr edit "$number" --repo "$REPO" --base "$TARGET_BASE"; then + echo "#$number: FAILED to change base branch" + failed=$((failed + 1)) + continue + fi + post_explanation "$number" + matched=$((matched + 1)) + [ "$THROTTLE_SECONDS" = "0" ] || sleep "$THROTTLE_SECONDS" +done + +if [ "$DRY_RUN" = "true" ]; then + echo "DRY RUN โ€” no pull request was modified. would_retarget=$matched skipped=$skipped failed=$failed" + summary="**Dry run โ€” nothing was modified.** Would retarget **$matched**, skip **$skipped**, failed to read **$failed**." +else + echo "retargeted=$matched skipped=$skipped failed=$failed unexplained=$unexplained" + summary="Retargeted **$matched** pull request(s) onto \`$TARGET_BASE\`, skipped **$skipped**, failed **$failed**, missing an explanation **$unexplained**." +fi + +echo "$summary" +[ -z "${GITHUB_STEP_SUMMARY:-}" ] || echo "$summary" >> "$GITHUB_STEP_SUMMARY" + +[ "$failed" -eq 0 ] && [ "$unexplained" -eq 0 ] diff --git a/.github/workflows/pr-retarget-dev.yml b/.github/workflows/pr-retarget-dev.yml new file mode 100644 index 0000000000..da4cf02638 --- /dev/null +++ b/.github/workflows/pr-retarget-dev.yml @@ -0,0 +1,79 @@ +name: Retarget PRs to dev + +on: + pull_request_target: + types: [opened, reopened, synchronize] + branches: [main] + workflow_dispatch: + inputs: + dry_run: + description: 'Report what would change without editing any pull request' + type: boolean + default: true + pr_numbers: + description: 'Space-separated PR numbers (default: every open pull request based on main)' + required: false + default: '' + +permissions: + contents: read + pull-requests: write + +# Per-pull-request for the hook so a burst of openings runs in parallel; GitHub keeps only one +# pending job per group, so a single shared group would cancel queued retargets. Every sweep shares +# one group so two of them cannot process the same pull request at once. +concurrency: + group: pr-retarget-dev-${{ github.event.pull_request.number || 'sweep' }} + cancel-in-progress: false + +jobs: + on-open: + name: Retarget on open + if: github.event_name == 'pull_request_target' + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - uses: actions/checkout@v5 + with: + # pull_request_target runs with a write token: check out the trusted base + # commit only, never the pull request head. + ref: ${{ github.event.pull_request.base.sha }} + persist-credentials: false + sparse-checkout: .github/scripts + + - name: Retarget onto dev + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + REPO: ${{ github.repository }} + PR_NUMBER: ${{ github.event.pull_request.number }} + run: .github/scripts/retarget-prs.sh "$PR_NUMBER" + + sweep: + name: Sweep open pull requests + if: github.event_name == 'workflow_dispatch' + runs-on: ubuntu-latest + timeout-minutes: 60 + steps: + - uses: actions/checkout@v5 + with: + persist-credentials: false + sparse-checkout: .github/scripts + + - name: Retarget onto dev + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + REPO: ${{ github.repository }} + DRY_RUN: ${{ inputs.dry_run }} + PR_NUMBERS: ${{ inputs.pr_numbers }} + THROTTLE_SECONDS: '2' + run: | + numbers="$PR_NUMBERS" + if [ -z "$numbers" ]; then + numbers="$(gh api --paginate "repos/$REPO/pulls?base=main&state=open&per_page=100" --jq '.[].number')" + fi + if [ -z "$numbers" ]; then + echo "No open pull requests based on main." + exit 0 + fi + # shellcheck disable=SC2086 + .github/scripts/retarget-prs.sh $numbers diff --git a/AGENTS.md b/AGENTS.md index 287d5357f2..13a1570a26 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,5 +1,23 @@ See CLAUDE.md. +## Branching and pull requests + +Branch off `dev` and target `dev` with every pull request; `gh pr create` defaults to `main`, so +pass `--base dev` explicitly. `main` is the released branch, kept as a fast-forward of `dev` and +synced as-is โ€” never open a backport pull request to `main`, because anything merged to `dev` +reaches it at the next sync. Pull requests opened against `main` are retargeted automatically. +`Fixes #N` does not close the issue on a `dev` merge โ€” GitHub honors closing keywords only on the +default branch, so close linked issues by hand. Worktrees share one stash stack, so never use a bare +`git stash pop`. See the detailed policy in `CLAUDE.md` under "Branching and Pull Requests". + +## Verification + +A green build is not a typecheck: `packages/api`, `packages/client` and `packages/data-schemas` build +with `tsdown`, which emits without checking types. Run `npx tsc --noEmit` in the workspace you +changed. `packages/client` excludes `*.spec.ts(x)` and `*.test.ts(x)` from typechecking entirely. +`npm run sort-imports` with no arguments rewrites every source root โ€” pass the paths you touched. See +`CLAUDE.md` under "Typechecking" and "Formatting". + ## Frontend theming and styling For frontend work, compose existing `@librechat/client` primitives and variants before adding diff --git a/CLAUDE.md b/CLAUDE.md index bff3a7ebd1..7594b29027 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -28,6 +28,29 @@ The source code for `@librechat/agents` (major backend dependency, same team) li --- +## Branching and Pull Requests + +- **Branch off `dev`, and target `dev` with every pull request.** All work lands on `dev` first. +- **`main` is the released branch.** It is kept as a fast-forward of `dev` and synced as-is, so it + is always an ancestor of `dev` โ€” equal to it right after a sync, behind it otherwise. It never + carries a commit that `dev` does not have. +- **Never open a backport pull request to `main`.** Anything merged to `dev` reaches `main` at the + next sync; a second pull request for the same change is redundant. +- **The repository's default branch is `main`**, so `gh pr create` and the GitHub UI target it + unless told otherwise โ€” always pass `--base dev` explicitly. +- Pull requests opened against `main` are retargeted to `dev` automatically by + `.github/workflows/pr-retarget-dev.yml`. The `target: main` label exempts one, as do release-bound + upstream branches (`dev`, `release/*`, `hotfix/*`). Backport branches are deliberately not exempt โ€” + a backport merged straight to `main` is what breaks the fast-forward invariant. +- **`Fixes #N` does not close the issue.** GitHub honors closing keywords only when a pull request + merges into the default branch (`main`). Merging to `dev` does not close anything, and the later + fast-forward of `main` is not a merge event either โ€” close linked issues by hand. +- **Git worktrees share one stash stack.** `refs/stash` lives in the common `.git` directory, so a + bare `git stash pop` in one worktree can take work stashed in another. Prefer a throwaway WIP + commit; if you must stash, `git stash push -m ` and `apply` that specific entry. + +--- + ## Code Style ### Naming and File Organization @@ -207,6 +230,20 @@ Without it, OpenID JWT request burst caching can serve a stale `req.user` until - Frontend tests: `__tests__` directories alongside components; use `test/layout-test-utils` for rendering. - Cover loading, success, and error states for UI/data flows. +### Typechecking + +- **A green build is not a typecheck.** `packages/api`, `packages/client` and `packages/data-schemas` + build with `tsdown` alone, which emits without checking types. Only `packages/data-provider` runs + `tsc` as part of its build. +- Run `npx tsc --noEmit` in the workspace you changed before calling it done. `client` also exposes + it as `npm run typecheck`. +- `packages/client/tsconfig.json` excludes `*.spec.ts(x)` and `*.test.ts(x)`, so test files there are + never typechecked โ€” a type error in a spec surfaces only when the test runs. +- `npm run static-checks` runs the Static Checks CI job locally against your staged files; + `npm run static-checks -- --against origin/dev` reproduces what CI sees for a pull request, and + `npm run static-checks:full` adds the slow gates (TypeScript, config migration tests, unused i18n + keys, unused npm packages). + ### Philosophy - **Real logic over mocks.** Exercise actual code paths with real dependencies. Mocking is a last resort. @@ -221,3 +258,7 @@ Without it, OpenID JWT request burst caching can serve a stale `req.user` until ## Formatting Fix all formatting lint errors (trailing spaces, tabs, newlines, indentation) using auto-fix when available. All TypeScript/ESLint warnings and errors **must** be resolved. + +`npm run sort-imports` with no arguments rewrites every file under `api/`, `client/src` and the four +`packages/*/src` roots โ€” far beyond what you touched. Always pass explicit paths: +`npm run sort-imports -- path/to/file.ts`.