fix: preserve base URL path prefix in record proxy - #57
Merged
Conversation
The URL constructor drops the base path when the pathname is absolute:
new URL("/v1/chat/completions", "https://openrouter.ai/api") resolves to
https://openrouter.ai/v1/chat/completions — losing /api.
Extract resolveUpstreamUrl() that normalizes inputs for RFC 3986 relative
resolution (trailing slash on base, strip leading slash from pathname).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Merged
jpr5
added a commit
that referenced
this pull request
Mar 31, 2026
## Summary Patch release capturing recorder fixes that landed since v1.6.0. ### Patch Changes - Fix record proxy to preserve upstream URL path prefixes (#57) - Fix record proxy to forward all request headers to upstream (#58) - Fix recorder to decode base64-encoded embeddings with `encoding_format: "base64"` (#64) - Guard base64 decode against corrupted data - Update CHANGELOG, skill docs, competitive matrix script ### Files changed - `package.json` — version 1.6.0 → 1.6.1 - `CHANGELOG.md` — new 1.6.1 section - `skills/write-fixtures/SKILL.md` — recorder docs updated All 1,327 tests pass. Build clean.
jpr5
added a commit
that referenced
this pull request
Apr 3, 2026
## Problem
The record proxy loses the base URL path prefix when constructing
upstream URLs.
`new URL("/v1/chat/completions", "https://openrouter.ai/api")` resolves
to
`https://openrouter.ai/v1/chat/completions` — the `/api` prefix is
dropped.
This is standard `URL` constructor behavior (absolute pathname replaces
the
base path), but breaks providers like OpenRouter whose API lives at a
non-root path (`/api/v1/...`).
Providers with root-path APIs (OpenAI, Anthropic) are unaffected.
## Fix
Extract URL joining into a `resolveUpstreamUrl()` helper that normalizes
inputs for RFC 3986 relative resolution:
- Ensure base URL has a trailing slash (marks it as a "directory")
- Strip leading slash from pathname (makes it relative, not absolute)
This preserves any path prefix while keeping existing behavior for
root-path providers unchanged. Unit tests cover all combinations.
## Examples
| Base URL | Pathname | Before | After |
|---|---|---|---|
| `https://openrouter.ai/api` | `/v1/chat/completions` |
`https://openrouter.ai/v1/chat/completions` |
`https://openrouter.ai/api/v1/chat/completions` |
| `https://api.openai.com` | `/v1/chat/completions` |
`https://api.openai.com/v1/chat/completions` |
`https://api.openai.com/v1/chat/completions` |
| `https://api.anthropic.com` | `/v1/messages` |
`https://api.anthropic.com/v1/messages` |
`https://api.anthropic.com/v1/messages` |
jpr5
added a commit
that referenced
this pull request
Apr 3, 2026
## Summary Patch release capturing recorder fixes that landed since v1.6.0. ### Patch Changes - Fix record proxy to preserve upstream URL path prefixes (#57) - Fix record proxy to forward all request headers to upstream (#58) - Fix recorder to decode base64-encoded embeddings with `encoding_format: "base64"` (#64) - Guard base64 decode against corrupted data - Update CHANGELOG, skill docs, competitive matrix script ### Files changed - `package.json` — version 1.6.0 → 1.6.1 - `CHANGELOG.md` — new 1.6.1 section - `skills/write-fixtures/SKILL.md` — recorder docs updated All 1,327 tests pass. Build clean.
jpr5
added a commit
that referenced
this pull request
Jul 15, 2026
## Problem
The record proxy loses the base URL path prefix when constructing
upstream URLs.
`new URL("/v1/chat/completions", "https://openrouter.ai/api")` resolves
to
`https://openrouter.ai/v1/chat/completions` — the `/api` prefix is
dropped.
This is standard `URL` constructor behavior (absolute pathname replaces
the
base path), but breaks providers like OpenRouter whose API lives at a
non-root path (`/api/v1/...`).
Providers with root-path APIs (OpenAI, Anthropic) are unaffected.
## Fix
Extract URL joining into a `resolveUpstreamUrl()` helper that normalizes
inputs for RFC 3986 relative resolution:
- Ensure base URL has a trailing slash (marks it as a "directory")
- Strip leading slash from pathname (makes it relative, not absolute)
This preserves any path prefix while keeping existing behavior for
root-path providers unchanged. Unit tests cover all combinations.
## Examples
| Base URL | Pathname | Before | After |
|---|---|---|---|
| `https://openrouter.ai/api` | `/v1/chat/completions` |
`https://openrouter.ai/v1/chat/completions` |
`https://openrouter.ai/api/v1/chat/completions` |
| `https://api.openai.com` | `/v1/chat/completions` |
`https://api.openai.com/v1/chat/completions` |
`https://api.openai.com/v1/chat/completions` |
| `https://api.anthropic.com` | `/v1/messages` |
`https://api.anthropic.com/v1/messages` |
`https://api.anthropic.com/v1/messages` |
jpr5
added a commit
that referenced
this pull request
Jul 15, 2026
## Summary Patch release capturing recorder fixes that landed since v1.6.0. ### Patch Changes - Fix record proxy to preserve upstream URL path prefixes (#57) - Fix record proxy to forward all request headers to upstream (#58) - Fix recorder to decode base64-encoded embeddings with `encoding_format: "base64"` (#64) - Guard base64 decode against corrupted data - Update CHANGELOG, skill docs, competitive matrix script ### Files changed - `package.json` — version 1.6.0 → 1.6.1 - `CHANGELOG.md` — new 1.6.1 section - `skills/write-fixtures/SKILL.md` — recorder docs updated All 1,327 tests pass. Build clean.
jpr5
added a commit
that referenced
this pull request
Aug 6, 2026
…artifact is pinned (#359) Two follow-ups on `main` from the adversarial review of #357. ## 1. The rejection/dedup query is already saturated — silently `gh pr list --limit N` is a window, not a ceiling: it returns the newest N rows and drops the rest with no flag, no warning and no count. The marker self-heal — the thing that makes a human's rejection survive a body edit — was a plain `--state all --limit 200` whose state filter is applied *client-side*, so every PR in the repo competed for those 200 slots and the merged ones won. Measured live on this repo, 2026-08-05: ``` gh pr list --state all --limit 200 -> 200 [MERGED=184 CLOSED=14 OPEN=2] #125..#358 gh pr list --state closed --limit 200 -> 200 [MERGED=186 CLOSED=14] gh pr list --state all --limit 200 --search "is:unmerged" -> 28 [CLOSED=26 OPEN=2] #1..#358 ``` Two things the review did not have: - **`--state closed` is not a fix.** gh maps it to CLOSED-**or**-MERGED — 186 merged against 14 closed, just as full. - **Twelve closed PRs are already invisible.** The repo has 26 unmerged-closed PRs; the current listing sees 14. The lost set is `#1, 30, 50, 56, 59, 61, 63, 94, 95, 96, 119, 120`, each confirmed CLOSED by `gh pr view`. None is a drift PR, so no rejection is lost *today* — but the mechanism is running now, and MERGED is the population that grows daily. **Fix.** Both self-heal listings additionally query `--search "is:unmerged"` and union the two views. That excludes merged PRs server-side: 200/200 slots consumed becomes 28/200, and 28 is the *complete* unmerged population back to #1. `is:unmerged` is a STATE predicate, so unlike `<key> in:body` it cannot be defeated by the very body edit the self-heal repairs, and a just-closed PR matches under either indexed state. The plain listing is unioned rather than replaced, so the index-free view stays and plain entries win on collision. Headroom is not a proof, so saturation is also made loud: `assert_listing_complete` refuses any listing that comes back full at its `--limit` rather than deciding on it. `--limit 200` also stopped being a magic number — each step binds `PR_LIST_LIMIT` once and the audit reads the same variable. ## 2. "the one unpinned executable in the job" was false `89e5a07` pinned `ollama.com/install.sh`. That script then streams `ollama.com/download/ollama-linux-<arch>.tar.zst` — mutable, unversioned, no digest — through `zstd -d` into `sudo tar -x` under `/usr/local`. Run verbatim out of the pinned script's own bytes, with curl serving attacker content: ``` STEP_EXIT=0 SUDO_RAN argv=tar -xf - -C ./dest TAR_RAN argv=-xf - -C ./dest what tar was handed: ATTACKER-CONTROLLED TARBALL BYTES ``` A compromised tarball plants a root-owned `git`/`gh`/`node` on PATH exactly as well as a compromised script would, and every later step holds a `contents:write` + `pull-requests:write` app token those binaries can reach. It cannot be fixed in place — install.sh pipes the download straight into `tar`, so it never holds the file and has nothing to verify; `OLLAMA_VERSION` only appends `?version=`, a version pin rather than a byte pin. **Fix.** install.sh is not used. The release artifact is fetched from an immutable release tag and sha256-checked before anything unpacks it. Digest agreed by three independent sources: the v0.32.6 release's `sha256sum.txt`, the GitHub release API's asset `digest` field, and sha256 of the 1,420,686,963 downloaded bytes. Archive layout confirmed by listing it (`bin/ollama` + `lib/ollama/*`, so `-C /usr/local` puts the binary on PATH). Dropping install.sh also drops its NVIDIA CUDA repo adds and `$PACKAGE_MANAGER -y install`; the step already ran `ollama serve` itself rather than using its systemd unit. The false claim is corrected in the workflow comment, the suite's header comment, its describe name and its failure messages. Two further stale claims about the self-heal listing `--state open` (false since #357) are corrected too. The commit message that first asserted it is in history and cannot be rewritten. ## Red-green The `gh` stub could not previously express item 1's bug — it ignored `--limit` and returned the whole population, so a saturated window looked exactly like a healthy one. It now models what real gh does. **RED (watched).** The existing rejection scenario plus 200 newer MERGED PRs — both PR-open steps re-proposed the rejected changeset: ``` FAIL a rejection is still found once MERGED PRs have filled the listing window stdio: Pushed branch main-1111222233334444 … Matched PR #56 FAIL needs_human_pr: a rejection survives a full window there too stdio: Pushed needs-human branch … Opened needs-human PR #57 ``` **GREEN.** 154 passed, exit 0. **RE-BREAK (watched), each mutation restored and md5-verified:** | mutation | tests red | |---|---| | neuter the union so unmerged rows are dropped | 2 | | delete all four `assert_listing_complete` calls | 2 | | raise the audit threshold out of reach | 1 | | replace the tarball digest comparison with `if false` | 1 | | point the fetch back at `ollama.com/install.sh` | 1 | Item 2's guard now watches `tar`, not `sh`, and the positive control asserts `tar` was handed exactly the verified bytes rather than merely that it ran. ## Gates `pnpm build` 0 · `pnpm test` 0 (**5048 passed / 171 files**) · `pnpm test:drift` 0 · `tsc --noEmit` 0 · `pnpm lint` 0 · `pnpm test:exports` 0 · prettier clean · `actionlint` 0 · `zizmor` 0 findings · `bash -n` on all 16 `run:` bodies 0 · commitlint 0. ## Not fixed here `.github/workflows/test-drift.yml:177` does the same `sh install.sh` with **no pin at all**, in a job holding five provider API keys, with provisioning ahead of the steps that use them. Same defect, different file — flagged, not touched.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The record proxy loses the base URL path prefix when constructing upstream URLs.
new URL("/v1/chat/completions", "https://openrouter.ai/api")resolves tohttps://openrouter.ai/v1/chat/completions— the/apiprefix is dropped.This is standard
URLconstructor behavior (absolute pathname replaces thebase path), but breaks providers like OpenRouter whose API lives at a
non-root path (
/api/v1/...).Providers with root-path APIs (OpenAI, Anthropic) are unaffected.
Fix
Extract URL joining into a
resolveUpstreamUrl()helper that normalizesinputs for RFC 3986 relative resolution:
This preserves any path prefix while keeping existing behavior for
root-path providers unchanged. Unit tests cover all combinations.
Examples
https://openrouter.ai/api/v1/chat/completionshttps://openrouter.ai/v1/chat/completionshttps://openrouter.ai/api/v1/chat/completionshttps://api.openai.com/v1/chat/completionshttps://api.openai.com/v1/chat/completionshttps://api.openai.com/v1/chat/completionshttps://api.anthropic.com/v1/messageshttps://api.anthropic.com/v1/messageshttps://api.anthropic.com/v1/messages