mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-19 01:46:15 +02:00
SkilLab HR and TechandPaperclip
92bedaac4a
## Thinking Path
> - Paperclip is the open source app people use to manage AI agents for
work
> - At boot the server selects the origin that agents must dial for the
control plane. It exports that origin to every run as
`PAPERCLIP_API_URL`
> - The selection starts from `auth.publicBaseUrl`. An operator writes
this value by hand
> - If the operator writes `host:port` and omits the scheme, the server
does not reject the value. The value is a valid URL with an opaque
scheme. The `origin` of such a URL serializes to the *string* `"null"`
> - The string `"null"` is truthy. Thus it gets through the `if (value)`
guard. The enclosing `try/catch` does not help, because the parse did
not fail. A later `new URL("null")` then throws `TypeError: Invalid URL`
on the startup path
> - The result is that the server does not start. The error message
names neither the configuration key nor the bad value
> - This pull request parses the public base URL in one helper. The
helper treats an unusable origin as absent configuration. This is the
same treatment that input which fails to parse gets today
> - The benefit is that a missing `https://` falls back to the existing
derived candidates. It no longer stops the boot
## Linked Issues or Issue Description
Fixes: #13576 — that issue reports this defect and includes a standalone
reproduction.
**Related PRs found by the duplicate search (none fix this defect).** I
searched the open PR list
for `runtime-api`, `publicBaseUrl`, `public base url`,
`PAPERCLIP_API_URL` and `Invalid URL`. I then
read the diff of every close neighbour. Refs #10517, Refs #9916, Refs
#7342, Refs #12886,
Refs #11564, Refs #9228. No one of them contains a test for an opaque
origin, and no one of them
compares an origin against `"null"`. They change *which reachable origin
wins*. This pull request
changes *how one unusable value is rejected*. The two subjects do not
overlap.
Refs #10517 and Refs #9916 are duplicates of each other. Neither touches
`server/src/runtime-api.ts`.
**One neighbour needs attention.** Refs #11564 adds a new function
`resolveRuntimeApiUrl`. That
function contains this code:
```ts
try {
const origin = new URL(explicitPublicBaseUrl).origin;
return { url: origin, reason: "explicit-public-base-url", ... };
} catch {
// Fall through to derived candidates if config parsing drifted.
}
```
This is the same defect in a third location. A scheme-less value parses
correctly. Thus the `catch`
does not run, and the function returns the literal string `"null"` as
the URL. The lines are pure
additions, so there is no text conflict with this pull request. But if
#11564 merges after this one,
it puts the defect back in a new function. The helper in this pull
request is module-private and
available to that function. I can add the one-line change to this branch
if the maintainers prefer
that.
## What Changed
`server/src/runtime-api.ts` — one new module-private helper, used at the
two existing call sites:
```ts
function parsePublicOrigin(rawUrl: string | null | undefined): string | null {
const trimmed = rawUrl?.trim();
if (!trimmed) return null;
let origin: string;
try {
origin = new URL(trimmed).origin;
} catch {
return null;
}
return origin === "null" ? null : origin;
}
```
- `choosePrimaryRuntimeApiUrl` now calls the helper. Its inline
`try/catch` is removed.
- `buildRuntimeApiCandidateUrls` now calls the helper. Its inline IIFE
is removed.
- `server/src/__tests__/runtime-api.test.ts` gets two new cases. One
case covers the wrong `"null"`
result. The other case covers the startup exception.
Counts: **+20 / −17** in `runtime-api.ts`. Five of the added lines are
the comment that explains the
opaque scheme. The test file is **+23 / −0**. There is no new
dependency. There is no `any` type. No
exported signature changes. The duplicated parse logic at the two call
sites becomes one function.
## Verification
I ran every step below against `master` at commit
`c65fc9e3c81c41aafe421aa90a00514b84343285`. That
commit is the merge base of this branch.
- **Unit tests**, with the repository runner (`vitest`), on the two
added cases:
- Before the fix: `Tests 2 failed | 6 passed (8)`. The two failures are
exactly the two new cases.
The assertion error is
`AssertionError: expected 'null' to be
'http://runtime-host.example.test:3102'`.
- After the fix: `Tests 8 passed (8)`.
- Both new cases are pure additions. No existing expectation changes,
because no existing test
uses a scheme-less `authPublicBaseUrl`.
- **Standalone reproduction with 8 arms** (attached to #13576): **6/8**
on unpatched `c65fc9e3`, and
**8/8** with the patch. The 5 regression arms and the 1 control arm give
the same result on both
sides. The control arm uses input that truly fails to parse. Thus the
reproduction discriminates
between the two builds. It does not pass by accident.
- **Typecheck**: `tsc --noEmit --strict --noEmitOnError --target ES2023
--module NodeNext` exits 0 on
the patched file. It also exits 0 on the unmodified file.
## Risks
**Low risk.** Reasons:
- The change only affects input that makes the server crash today. No
operator can depend on the
present behaviour, because the present behaviour stops the boot.
- The new path is not new code. The helper returns `null`, and `null`
goes to the fallback that
already exists for input that fails to parse.
- The second call site previously put the string `"null"` into the
candidate list. It now omits that
entry. A `"null"` origin is not reachable, so no caller can lose a
usable candidate.
**The one behaviour change a reviewer must agree with.** A bad value now
degrades quietly instead of
failing loudly. An operator who writes `host:port` gets a server that
starts on a derived origin. The
operator does not get an error.
I did not add a warning log, for one reason: the existing parse-failure
path is also silent. A log
would change behaviour that this pull request does not target, and it
would enlarge the diff. If the
maintainers want the warning, the correct fix is one log line inside
`parsePublicOrigin`, which
covers both input classes at once. This is now possible only because the
two call sites share one
function. Tell me if you want that, and I will add it.
**Migration:** none. No schema change, no data change, no configuration
change.
## Model Used
- Provider and model: Anthropic Claude, Opus 5
- Exact model ID: `claude-opus-5[1m]`
- Context window: 1M tokens
- Mode: extended thinking
- Capabilities used: tool use, shell execution, file editing, and GitHub
API access
- The human operator reviewed and authorized this submission. A human
approved the publication of
this pull request before I opened it.
## Checklist
- [x] I have included a thinking path that traces from project context
to this change
- [x] I have specified the model used (with version and capability
details)
- [x] I have checked ROADMAP.md and confirmed this PR does not duplicate
planned core work — I read
the file. It has no item for the runtime API URL, for startup URL
validation, or for
configuration parsing.
- [x] I have searched GitHub for duplicate or related PRs and linked
them above — 5 queries, and I
read the diff of each close neighbour. See the linked-issues section.
- [x] I have either (a) linked existing issues with `Fixes: #` / `Closes
#` / `Refs #` OR (b)
described the issue in-PR following the relevant issue template
- [x] I have not referenced internal/instance-local Paperclip issues or
links (only public GitHub
`#NNN` / `github.com/paperclipai/paperclip` URLs)
- [x] My branch name describes the change (e.g. `docs/...`, `fix/...`)
and contains no internal
Paperclip ticket id or instance-derived details — the branch is
`fix/scheme-less-public-base-url`
- [x] I have run tests locally and they pass
- [x] I have added or updated tests where applicable
- [x] I have updated relevant documentation to reflect my changes — no
user-facing document
describes this internal fallback, so no document change applies.
- [x] I have considered and documented any risks above
- [ ] All Paperclip CI gates are green — not yet true, and I do not
check a box that is not true. On
commit `13ffb649`: 43 checks pass, 7 checks still run, 2 checks fail.
Both failures are the
PR-description gate that this update repairs. I will check this box when
the gates are green.
- [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups —
not yet true. Greptile gave
4/5 with one P2. That P2 is "Required PR sections missing", which this
update repairs.
- [x] I will address all Greptile and reviewer comments before
requesting merge — this update is
that work for the first review round.
---------
Co-authored-by: Paperclip <noreply@paperclip.ing>