diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 68f4704..fd2bb1f 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -113,13 +113,16 @@ jobs: fi go test -run 'TestE2E' -v . - # Reports the port's own mechanical gate. Advisory here, BLOCKING inside the - # sync job (scripts/upstream) where it gates whether state.yaml advances. + # The port's own mechanical gate, blocking in both places it runs: here on every + # PR, and inside the sync job (scripts/upstream) where it decides whether + # state.yaml advances. # - # Advisory on purpose: the port is currently a minor version behind upstream, so - # the required-API check fails by design until the first sync lands. Making that - # a red required check on every unrelated PR just teaches people to ignore CI. - # The signal still shows up in the job summary. + # It was advisory while the port sat a minor version behind upstream and the + # required-API check failed by design. That gap closed with the 0.8.0 port and + # the verifier now passes clean, so continue-on-error only bought the ability to + # regress the parity floor with a green PR. The upstream-dependent sections + # (release-tag position, test-cluster parity) SKIP without an upstream checkout + # rather than fail, so a plain actions/checkout is enough to run this. verify-port: runs-on: ubuntu-latest timeout-minutes: 15 @@ -127,9 +130,8 @@ jobs: steps: - uses: actions/checkout@v4 - uses: ./.github/actions/port-toolchain - - name: Port verifier (advisory) + - name: Port verifier id: verify - continue-on-error: true run: | set -o pipefail ./.upstreamer/scripts/verify.sh 2>&1 | tee /tmp/verify.log @@ -143,8 +145,8 @@ jobs: if [ "${{ steps.verify.outcome }}" = "success" ]; then echo "Port is in sync with its parity floor." else - echo "Parity gaps below. Expected until the port catches up to upstream —" - echo "advisory here, blocking inside the sync job." + echo "Parity floor violated — see the failures below. This blocks the PR:" + echo "fix the gap rather than lowering the gate." fi echo echo '```' diff --git a/.github/workflows/upstreamer-port.yaml b/.github/workflows/upstreamer-port.yaml index 17317c6..54d157e 100644 --- a/.github/workflows/upstreamer-port.yaml +++ b/.github/workflows/upstreamer-port.yaml @@ -1,10 +1,28 @@ name: Upstreamer Port -# Ports @openrouter/agent into this repo. Two triggers: -# 1. repository_dispatch from typescript-agent's publish.yaml on a new npm release -# (event type: openrouter-agent-published) — the intended path. Ports track -# published releases, not every commit to upstream main. -# 2. Weekly cron as a safety net for missed dispatches, plus manual dispatch. +# Ports @openrouter/agent into this repo. +# +# The port tracks upstream's **default-branch HEAD**, not the latest published npm +# release. Release tracking sounds safer but produces exactly the failure this +# pipeline exists to prevent: upstream can sit for weeks with large unreleased +# work on main (doom-loop detection, #73, was ~7.5k lines) and the port stays +# blind to it, then absorbs the whole delta in one automated run touching the most +# load-bearing modules. Tracking HEAD keeps each delta small enough to review. +# +# Consequence to keep in mind: the port is then routinely AHEAD of the latest +# release, so the release tag matching its ported surface legitimately lags +# upstream's package.json. The verifier reports that rather than failing — Go +# modules carry no version field, so there is nothing to drift, but tagging a +# release from an ahead-of-tag tree would still misrepresent it. +# +# Three triggers, all resolving to HEAD unless given an explicit ref: +# 1. Weekly cron — the primary path now that releases are not the trigger. +# 2. repository_dispatch from typescript-agent's publish.yaml on a new npm +# release. Still useful as a "something just shipped, sync promptly" nudge, +# but it no longer pins the ref to that release tag: doing so would port +# BACKWARDS once the port is ahead of the release. scripts/upstream refuses +# an ancestor ref outright. +# 3. Manual dispatch, optionally with an explicit ref. # # Opens a PR. Never pushes to main. A failed parity eval leaves # .upstreamer/state.yaml unchanged, so the next run retries the same delta. @@ -39,6 +57,13 @@ jobs: port: runs-on: ubuntu-latest timeout-minutes: 150 + env: + # Surfaced as env because the `secrets` context is NOT available in a + # step-level `if:` (only github/needs/strategy/matrix/job/runner/env/vars/ + # steps/inputs are). Referencing secrets.* there evaluates to empty and the + # condition silently never matches — so the App-token gate below tests this + # variable instead. Only ever compared against '' ; never echoed. + HAS_APP_KEY: ${{ secrets.PORT_BOT_PRIVATE_KEY != '' }} steps: - uses: actions/checkout@v4 with: @@ -52,21 +77,29 @@ jobs: - name: Set up language toolchain uses: ./.github/actions/port-toolchain - # Ports track published releases, not upstream main. When no ref arrives - # (cron, or a manual dispatch with the input left blank), resolve the - # latest published @openrouter/agent version from the public npm registry - # and port its release tag. This makes the cron fully equivalent to the - # repository_dispatch fast path — same tag either way — so the pipeline - # works with no cross-repo token at all if the dispatch is unavailable. + # Blank ref = upstream default-branch HEAD, which scripts/upstream resolves + # itself. That is the normal case for both the cron and a publish dispatch. + # + # Only an EXPLICIT manual `ref` input is honored. The publish dispatch's + # client_payload.ref is deliberately ignored: it carries the release tag, + # which is an ancestor of HEAD once the port is ahead of the release, so + # honoring it would revert landed work. The dispatch still does its real + # job — waking the pipeline promptly after a release — it just syncs to HEAD + # like every other trigger. (scripts/upstream also refuses an ancestor ref + # outright, so this is defense in depth, not the only guard.) - name: Resolve target ref id: target run: | set -euo pipefail - REF="${{ inputs.ref || github.event.client_payload.ref }}" - if [ -z "$REF" ]; then - VERSION="$(curl -fsSL 'https://registry.npmjs.org/@openrouter%2Fagent/latest' | python3 -c 'import json,sys; print(json.load(sys.stdin)["version"])')" - REF="@openrouter/agent@${VERSION}" - echo "No ref provided — resolved latest npm release: $REF" + REF="${{ inputs.ref }}" + if [ -n "$REF" ]; then + echo "Explicit ref requested: $REF" + else + PAYLOAD_REF="${{ github.event.client_payload.ref }}" + if [ -n "$PAYLOAD_REF" ]; then + echo "::notice::Ignoring dispatch payload ref '$PAYLOAD_REF' — this port tracks upstream HEAD, and a release tag is an ancestor once the port is ahead of it. Syncing to HEAD instead." + fi + echo "No explicit ref — porting upstream default-branch HEAD." fi echo "ref=$REF" >> "$GITHUB_OUTPUT" @@ -84,9 +117,14 @@ jobs: echo "::error::OPENROUTER_API_KEY secret is not set. See .upstreamer/port.env.example." exit 1 fi - args=(--ref "${{ steps.target.outputs.ref }}") + # Only pass --ref when there is actually a ref. `--ref ""` is not the + # same as omitting it: the arg parser consumes the empty value and the + # script would target an empty ref instead of defaulting to HEAD. + args=() + REF="${{ steps.target.outputs.ref }}" + [ -n "$REF" ] && args+=(--ref "$REF") [ "${{ inputs.force }}" = "true" ] && args+=(--force) - ./scripts/upstream "${args[@]}" + ./scripts/upstream ${args[@]+"${args[@]}"} - name: Check for changes id: diff @@ -112,12 +150,52 @@ jobs: echo "passed=true" >> "$GITHUB_OUTPUT" fi + # Mint a GitHub App installation token so the PR is created by the App + # rather than by the native GITHUB_TOKEN. + # + # Why this exists: GitHub does not trigger workflows from events created + # with GITHUB_TOKEN (recursion guard), so a PR opened with it gets no + # pull_request-event checks. Measured on python-agent PR #24, which runs the + # same pipeline: the commit had 14 check-runs while the PR's rollup showed + # 7 — the workflow_dispatch half was invisible to the PR. So the "dispatch + # ci.yaml explicitly" fallback below produces a green run that is not + # attached to the PR, which is materially worse here than on python-agent: + # main is not branch-protected on this repo, so nothing stops a reviewer + # from merging an automated port PR whose checks appear absent or, worse, + # reading a dispatched green run as if it had gated this diff. + # + # An App installation token is not recursion-guarded, so the PR gets real + # pull_request checks. Preferred over a PAT: scoped to this repo, not tied to + # a person's account, and independently revocable. + # + # Optional by design — see the fallback below. + - name: Mint App token + id: app-token + # Gate on BOTH halves. Gating on the App ID alone is a trap: the ID is a + # variable and the key is a secret, so they are added in separate places + # and one routinely lands first (that is the state right now — the ID is + # set on this repo, the key is not). With only the ID set, this step would + # run and fail on the missing key, turning a working fallback into a + # broken pipeline — worse than no App at all. + if: >- + steps.diff.outputs.changed == 'true' + && vars.PORT_BOT_APP_ID != '' + && env.HAS_APP_KEY == 'true' + uses: actions/create-github-app-token@v1 + with: + app-id: ${{ vars.PORT_BOT_APP_ID }} + private-key: ${{ secrets.PORT_BOT_PRIVATE_KEY }} + - name: Open PR id: open-pr if: steps.diff.outputs.changed == 'true' uses: peter-evans/create-pull-request@v6 with: - token: ${{ secrets.GITHUB_TOKEN }} + # App token when configured; GITHUB_TOKEN otherwise. With the fallback + # the PR still opens, but its checks will not attach — the guard step + # below says so loudly rather than leaving a PR that looks unchecked + # (or falsely checked) with no explanation. + token: ${{ steps.app-token.outputs.token || secrets.GITHUB_TOKEN }} branch: upstreamer/sync delete-branch: true title: >- @@ -142,16 +220,23 @@ jobs: `.upstreamer/state.yaml` did not advance, the eval did not pass and this PR must not be merged as-is. - # Events created with the native GITHUB_TOKEN deliberately do not trigger - # other workflows (GitHub's recursion guard), so the PR opened above gets - # no CI checks on its own. workflow_dispatch is exempt from that guard: - # kick ci.yaml at the PR branch explicitly. This keeps the whole pipeline - # on the native token — no PAT anywhere in this repo. - - name: Trigger CI on the port PR - if: steps.diff.outputs.changed == 'true' && steps.open-pr.outputs.pull-request-operation != 'none' + # Fallback path only. With the App configured, the PR above already has real + # pull_request checks and nothing here runs. + # + # Without it, the dispatched run below does NOT attach to the PR. It gives a + # human something to read, but it is not a check on that diff — and since + # main here is unprotected, an unattached green run is easy to mistake for + # one. Say that plainly rather than letting the PR look gated. + - name: Trigger CI on the port PR (no App token — checks will not attach) + if: >- + steps.diff.outputs.changed == 'true' + && steps.open-pr.outputs.pull-request-operation != 'none' + && steps.app-token.outputs.token == '' env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - run: gh workflow run ci.yaml --repo "$GITHUB_REPOSITORY" --ref upstreamer/sync + run: | + echo "::warning::PORT_BOT_APP_ID / PORT_BOT_PRIVATE_KEY are not configured, so this PR was opened with GITHUB_TOKEN and will receive NO pull_request-event checks. The dispatched run below is informational only and does not gate this diff — do not read it as a passing check. Configure the App (see PORTING.md) or close and reopen the PR by hand to generate real checks." + gh workflow run ci.yaml --repo "$GITHUB_REPOSITORY" --ref upstreamer/sync - name: Upload logs if: always() diff --git a/.upstreamer/scripts/verify.sh b/.upstreamer/scripts/verify.sh index 818ab50..cf177bf 100755 --- a/.upstreamer/scripts/verify.sh +++ b/.upstreamer/scripts/verify.sh @@ -148,6 +148,92 @@ grep -rq 'service_tier\|ServiceTier' --include='*.go' . 2>/dev/null \ || fail "service_tier=auto workaround missing (breaks real request serialization)" echo +# A Go module declares no version, so there is no downstream version field to +# drift against upstream's package.json — the drift check the Python port needs +# has no analogue here and is deliberately absent. +# +# What still applies is position relative to the release tag. This port tracks +# upstream HEAD, so it routinely contains work upstream has not released, and +# `git tag v0.8.0` on that tree would publish post-0.8.0 behavior under a +# released number. Consumers pin go-agent by tag, and a pushed tag is immutable +# in the module proxy's cache, so this has to be visible before tagging rather +# than discovered after. Not a failure: being ahead is the intended state. +echo "-- Release-tag position" +upstream_git="tmp/upstreamer/upstream" +upstream_pkg="$upstream_git/packages/agent/package.json" +if [ -d "$upstream_git/.git" ] && [ -f "$upstream_pkg" ]; then + target=$(grep -m1 '"version"' "$upstream_pkg" | sed 's/.*"version": *"\([^"]*\)".*/\1/') + ported_sha="$(git -C "$upstream_git" rev-parse HEAD 2>/dev/null || true)" + rel_tag="$(git -C "$upstream_git" tag -l "@openrouter/agent@$target" | head -1)" + if [ -n "$ported_sha" ] && [ -n "$rel_tag" ]; then + ahead="$(git -C "$upstream_git" rev-list --count "$rel_tag^{commit}..$ported_sha" 2>/dev/null || echo 0)" + if [ "${ahead:-0}" -gt 0 ]; then + echo " NOTE: ported tree is $ahead commit(s) ahead of the $target release tag." + echo " Correct for a HEAD-tracking port, but do NOT cut a v$target tag" + echo " from this state — it would ship unreleased upstream work under a" + echo " released version number, and module tags cannot be recalled." + echo " Tag only from a commit level with a release tag, or after" + echo " upstream releases." + else + pass "ported tree is level with the $target release tag (taggable)" + fi + else + echo " SKIP: no @openrouter/agent@$target tag in the upstream checkout" + fi +else + # Only present during a sync run. Standalone/CI invocations legitimately have no + # upstream checkout; not a failure, but say so rather than passing silently. + echo " SKIP: no upstream checkout — release-tag position unchecked" +fi +echo + +# The suite is what makes "a version behind on real behavior" visible or +# invisible, so upstream's test surface is mechanically compared. Advisory: +# which gaps are acceptable is a judgment call, and .upstreamer/eval.md makes it. +# +# Compared at FEATURE-CLUSTER granularity, not file-for-file. The Python port can +# map 1:1 because pytest keeps upstream's file split; this port does not, and +# should not — Go consolidates by type under test, so `hooks-matchers.test.ts` +# lands as TestHooksManagerToolMatcherFailsClosed in hooks_manager_test.go and +# `run-cancellation.test.ts` as TestResultCancelStopsRunWithoutPanic. A per-file +# check was measured against this tree and reported 35-40 of 44 files "missing" +# while nearly all were in fact ported under Go names: noise that loud trains +# people to skip the section. Clustering on the leading path token and searching +# the whole test corpus reports only clusters with no coverage at all, which is +# the signal worth acting on. +echo "-- Test parity with upstream (advisory)" +upstream_tests="tmp/upstreamer/upstream/packages/agent/tests/unit" +if [ -d "$upstream_tests" ]; then + # Test function names plus bodies plus filenames: a cluster counts as covered + # if it is mentioned anywhere in the Go suite, not only in a function name. + corpus=$(mktemp) + { cat ./*_test.go 2>/dev/null; ls ./*_test.go 2>/dev/null | tr -d '_'; } \ + | tr 'A-Z' 'a-z' > "$corpus" + absent="" + # .test-d.ts files are compile-time type assertions with no runtime analogue in + # Go's type system; excluded rather than reported as perpetual gaps. + clusters=$(ls "$upstream_tests" | grep '\.test\.ts$' | sed 's/\.test\.ts$//' \ + | awk -F- '{print $1}' | sort -u) + for c in $clusters; do + grep -q "$c" "$corpus" || absent="$absent $c" + done + rm -f "$corpus" + if [ -z "${absent// /}" ]; then + pass "every upstream test cluster is represented in the Go suite" + else + count=$(printf '%s' "$absent" | wc -w | tr -d ' ') + echo " NOTE: $count upstream test cluster(s) with no mention anywhere in the Go suite:" + for name in $absent; do + echo " ${name}-*.test.ts — no Go test references '${name}'" + done + echo " Not a mechanical failure — see the Test Quality section of" + echo " .upstreamer/upstreamer.md and let the eval judge severity." + fi +else + echo " SKIP: no upstream checkout — test parity unchecked" +fi +echo + echo "-- No leaked TypeScript artifacts" leaked=$(find . -path ./tmp -prune -o -type f \( -name '*.ts' -o -name '*.js' \ -o -name 'package.json' -o -name 'tsconfig*.json' -o -name 'pnpm-lock.yaml' \) -print 2>/dev/null) diff --git a/PORTING.md b/PORTING.md index 774ebad..bcb436e 100644 --- a/PORTING.md +++ b/PORTING.md @@ -9,27 +9,58 @@ listed in the Idiomatic Divergences section of `.upstreamer/upstreamer.md`. ## How it works +The port tracks upstream's **default-branch HEAD**, not the latest published npm +release. + ``` -typescript-agent publishes @openrouter/agent to npm - │ - │ repository_dispatch: openrouter-agent-published + weekly cron · publish dispatch · manual dispatch + │ (a nudge, (optional + │ not a ref) explicit ref) ▼ .github/workflows/upstreamer-port.yaml - │ + │ resolve ref: explicit input, else upstream HEAD ▼ scripts/upstream │ 1. fetch upstream, resolve target commit - │ 2. compare against .upstreamer/state.yaml — skip if unchanged - │ 3. opencode runs the port against .upstreamer/upstreamer.md - │ 4. .upstreamer/scripts/verify.sh (mechanical gate) - │ 5. .upstreamer/eval.md (parity gate, fresh context) - │ 6. advance state.yaml — ONLY if both gates pass + │ 2. REFUSE if target is behind state.yaml (would revert work) + │ 3. compare against .upstreamer/state.yaml — skip if unchanged + │ 4. opencode runs the port against .upstreamer/upstreamer.md + │ 5. .upstreamer/scripts/verify.sh (mechanical gate) + │ 6. .upstreamer/eval.md (parity gate, fresh context) + │ 7. advance state.yaml — ONLY if both gates pass ▼ - Pull request (never a direct push to main) + Pull request, opened by the GitHub App so it gets real + pull_request-event CI checks (never a direct push to main) ``` -A weekly cron backs up the dispatch in case one is missed, and -`workflow_dispatch` allows a manual run against any ref. +### Why HEAD and not the latest release + +Release tracking sounds more conservative and is worse in practice. Upstream can +sit for weeks with large unreleased work on `main` — doom-loop detection (#73) was +~7,500 lines, ~4,700 of it tests, and rewrote a big part of `model-result.ts`. A +release-tracking port stays blind to that, then absorbs the entire delta in one +automated run touching the most load-bearing module in the package. Tracking HEAD +keeps each delta small enough that a human can actually review it. + +Two consequences follow, and both are handled rather than ignored: + +**The port is routinely ahead of the latest release.** So a release ref is now +*dangerous*: it resolves to an ancestor of what is already ported, and the +converter would faithfully "port" the older tree, reverting landed work. +`scripts/upstream` refuses a target that is behind `state.yaml` (exit 3) unless +`--force` is given, and the workflow ignores the publish dispatch's +`client_payload.ref` for the same reason. + +**A release tag cut from that tree would misrepresent it.** A Go module declares no +version, so there is no version field to drift — but the ported tree can sit ahead +of the release tag matching its surface, and `git tag v0.8.0` there would ship +unreleased upstream behavior under a released number. Module tags are immutable in +the proxy's cache, so the verifier reports how many commits ahead the tree is +before a tag gets cut. Tag from a commit level with a release tag. + +The weekly cron is the primary trigger. The publish dispatch still fires on a new +npm release — useful as "something shipped, sync promptly" — but it syncs to HEAD +like everything else. `workflow_dispatch` allows a manual run against any ref. ## The contract is the product @@ -107,6 +138,48 @@ Two values, same names locally and in CI: | `OPENROUTER_API_KEY` | local: `.upstreamer/port.env` · CI: repo **secret** | `sk-or-…` key opencode uses for inference | | `OPENCODE_MODEL` | local: `.upstreamer/port.env` · CI: repo **variable** | e.g. `openrouter/~anthropic/claude-opus-latest` | +Two more are needed in CI only, for the bot that opens port PRs: + +| Name | Kind | What | +|------|------|------| +| `PORT_BOT_APP_ID` | repo **variable** | The GitHub App's App ID | +| `PORT_BOT_PRIVATE_KEY` | repo **secret** | The App's generated private key (full PEM, including the BEGIN/END lines) | + +### Why the port PR needs a GitHub App + +GitHub does not trigger workflows from events created with the native +`GITHUB_TOKEN` (its recursion guard), so a PR opened with that token gets no +`pull_request`-event checks at all. + +Measured on python-agent PR #24, which runs this same pipeline: the commit carried +**14 check-runs while the PR's rollup showed 7** — a `workflow_dispatch` run of the +same workflow on the same commit was completely invisible to the PR. That is why +"just dispatch `ci.yaml` at the branch" does not substitute for it; the run exists +but gates nothing. + +`main` is not branch-protected on this repo, so unlike python-agent nothing +mechanically blocks a merge — which makes this *more* important here, not less. +An automated port PR with no attached checks, sitting next to a green dispatched +run, is exactly the thing a reviewer merges on the assumption CI covered it. + +An App installation token is not recursion-guarded, so the PR it opens gets real +checks. Preferred over a PAT: scoped to this repo, not tied to anyone's personal +account, and revocable on its own. + +**Setup** — create a GitHub App (org Settings → Developer settings → GitHub Apps): + +- Repository permissions: **Contents: Read and write**, **Pull requests: Read and + write**. Nothing else. +- Install it on `OpenRouterTeam/go-agent`. +- Generate a private key, then add `PORT_BOT_APP_ID` (variable) and + `PORT_BOT_PRIVATE_KEY` (secret). + +Until both exist the pipeline still runs and still opens a PR, but emits a +`::warning::` saying the PR will receive no checks. That is deliberate — an +unconfigured bot should degrade loudly, not look healthy. Both halves are required +by the gate: the ID alone would make the mint step run and fail on the missing +key, breaking a pipeline that otherwise works. + The wrapper writes the key into `~/.local/share/opencode/auth.json` so headless runs work without the interactive `opencode /connect` flow. diff --git a/scripts/upstream b/scripts/upstream index e110791..d6fe930 100755 --- a/scripts/upstream +++ b/scripts/upstream @@ -149,6 +149,35 @@ echo "target: $target_commit${ref:+ ($ref)}" >&2 echo "last port: ${last_upstream_commit:-none}" >&2 echo "model: ${model:-}" >&2 +# Refuse to port backwards. +# +# The port tracks upstream's default branch, so it is routinely AHEAD of the +# latest published release. That makes a release ref actively dangerous: a +# publish dispatch (or a manual --ref v0.8.0) resolves to a commit that is an +# ancestor of what we already ported, and the converter would dutifully "port" +# the older tree — silently reverting everything landed since. +# +# Ancestor of last-ported, and not equal to it, means strictly behind. Unrelated +# histories (no merge-base) are not caught here; they surface as a normal diff +# for review rather than a silent revert. +if [ "$force" -eq 0 ] && [ -n "$last_upstream_commit" ] && [ "$target_commit" != "$last_upstream_commit" ] \ + && git -C "$upstream_dir" merge-base --is-ancestor "$target_commit" "$last_upstream_commit" 2>/dev/null; then + behind_by="$(git -C "$upstream_dir" rev-list --count "$target_commit..$last_upstream_commit" 2>/dev/null || echo "?")" + cat >&2 <