Repository navigation
refactor(router): a request's kind and a provider's capabilities are each answered in one place, and the Windows suite is fixed - #98
Conversation
…ompts reach every provider unaltered Whether a request was an Auto mode classifier stage, a web search, a web fetch or a turn was worked out on several lines of proxy.ts and never reached the adapters, so every translated provider added to the classifier's security prompt: ChatGPT put an identity line in front of it and the image-generation hint after it, and the OpenAI-compatible, Google, Grok and Anthropic-compatible paths added the identity line and the addendum. ARCHITECTURE §2 promises that prompt stays intact. requestKind() now decides the kind once, and the route carries it with the model the client asked for. decide() returns the route or the reason to refuse it in one pass, replacing unroutableReason, which re-implemented resolve's rules to explain them; resolve() stays for callers without a path. promptAdditions() is the one rule for what a routed system prompt may gain, and a classifier request gains nothing. Failover names `asked` instead of splitting the log tag. Behaviour is otherwise unchanged. The ChatGPT path now trims the addendum as the others did. A prefix rule naming an ingress-only provider still lets a marker to a model nothing can take be refused by name; a test pins that case, which folding the two functions together had briefly lost. Implementation by GPT Daybreak Blue to a fixed design; reviewed here. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Five deepening candidates with a before/after flow graph each, a top recommendation, and what should be left alone. The router change before this commit is its candidate 1; candidate 3 comes next. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Brings PBJ-2#91's review fixes and current upstream main. The two conflicts kept both sides: upstream's conversation key on the request record and the SSE parser tests, beside this branch's request kind. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Brings the comment on the first-stage bound from PBJ-2#91. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Brings current upstream main through PBJ-2#91: provider-qualified model ids and the fix that keeps a Claude request with a delegation marker passing through (PBJ-2#94). Upstream made that fix in resolve() and unroutableReason() side by side. Here both live in decide(): the qualified-id branches are placed as upstream placed them, and every way of finding no route ends in one noRoute() that applies upstream's refusal rules in their order. Checked against PBJ-2#91's resolve() and unroutableReason() over 41,664 combinations of config, model and body (16,124 routed, 10,545 passed through, the rest refused with all five messages): no difference. The upstream tests that called unroutableReason() now read decide(...).refusal. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The 19 tests that fail on main's Windows job, and why:
- Same folder, different spelling. The runner's TEMP is the 8.3 alias
C:\Users\RUNNER~1\...; the product records the native long form, and on
Windows the lowercased installation home. desktop-legacy, direct-ownership,
the Codex block and the git creation tests now build their fixture roots with
fs.realpathSync.native and compare through the product's own normalizeHome.
The desktop-legacy fixture also sets USERPROFILE, which os.homedir() reads on
Windows, so its default-home case stops seeing the runner's real home.
- A file name Windows forbids. The launcher simulations for darwin and linux
made a home named with a double quote; on a Windows host it keeps the other
hostile characters (Korean, an apostrophe, spaces) and drops only the quote.
The three that need symlinks skip where the account cannot make one.
- The browse test expected the listing without the drive entries the server puts
first on Windows (folders.test.ts already pins those); it now expects them.
Product fixes the failures exposed:
- images.ts: the session temp scope and the allowed-root checks mixed TEMP's
8.3 spelling with the long form Windows returns, and compared paths case by
case, so a legitimate pasted image came back "blocked". ACL and identity
checks now run on the native long form, Windows paths compare without case,
and TEMP itself is still refused when it is a junction. The ACL, junction,
symlink and identity re-checks are unchanged.
- tray.ts: on Windows renameSync replaces an existing file, so a backup path
already taken let the swap replace the working runtime. The swap now refuses
an occupied backup path and leaves both copies whole.
- launcher.ts: removing the tray launcher uses unlinkSync with ENOENT ignored;
fs.rmSync({ force: true }) on that hostile file name ended the Node 24.1
process with 0xC0000409 on Windows.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
targets.ts answers the questions the router asked in a dozen places:
- isTargetProvider / ingressOnly: whether a provider can take routed
requests. It was spelled two ways ("anthropic without accountPool" and
"accountPool with claude-code auth") across routing, validate, the proxy,
agent files, model scores and the admin status. One spelling now, the
stricter one, which a valid config never tells apart.
- declaredBy / qualifiedModel / rawDeclaredBy / soleOwner, moved from
routing.ts (still re-exported there).
- effortLevels, moved out of the HTTP module the proxy imported it from
(still re-exported from admin.ts).
- runsServerTools / answersWebSearch: the proxy's canRunServerTools and the
admin's hand-copied mirror of it become one implementation.
- targetWarnings: a primary route or a direct rule naming an ingress-only
provider was accepted by validate and then dropped by routing without a word.
It is still accepted (refusing it would break configs that load today), but
the config save answers with a warning per rule and the router logs it when
it reads the config.
The dashboard asks the same question of providers not saved yet, so it keeps
one copy, isTargetProvider in app.js, used where it had three inline checks;
ui-targets.test.ts holds that copy to the router's for every provider shape.
One difference that comes with the single spelling: a config with accountPool
true and an auth other than claude-code is refused by validate but adopted at
startup. It is now ingress-only everywhere; before, routing treated it as a
target and the proxy then answered 400.
decide() still matches the previous rules over the 41,664-case comparison.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- The replacement race in images.test.ts swapped the folder only when the reader opened one exact spelling of the path. The reader now opens the native long form, so on a runner whose TEMP is an 8.3 alias the swap never happened and the file read as "ready". The swap now triggers on either spelling, and on Windows it uses a junction, which needs no symlink privilege, so the test also runs on accounts without it (it passes here: the swap is blocked). - The darwin/linux launcher simulation checked that a symlinked home is the same installation. normalizeHome resolves links for POSIX homes only; on a Windows host the simulated homes are Windows paths, compared as text, so that one step runs off Windows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Windows session temp check required each folder's owner to be the current user. Folders made by an elevated administrator token belong to the Administrators group instead, so on such a machine (GitHub's Windows runners among them) every pasted image and scratchpad file was blocked from the phone. The owner may now be the user, SYSTEM or Administrators: the principals the same check already trusts in its access rules. A folder another ordinary user owns, or one writable by anyone else, is still refused. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Port the isolated Windows fixes from PBJ-2#98 commits d4d5736, c69dc7b and ffe90c8, without the routing refactor. Preserve legacy ownership coverage on restricted Windows accounts. Harden the port so rejected candidate paths are never canonicalized before authorization. Add native-root spelling and ancestor-junction regressions; isolate session-temp ACL tests from the real user's temporary directory. Validation: typecheck and mod checks pass; Windows repair suites 161 passed, 6 skipped; image boundary suite 27 passed, 1 skipped. Full local suite: 2414 passed, 5 existing EPERM symlink failures, 47 skipped. Co-Authored-By: 115dkk <qjrzbasd@naver.com> Co-Authored-By: Claude Code <noreply@anthropic.com>
Two conflicts, both kept from each side: - admin.ts: the config save passes the agent dir to the model-slot sync, as main now does, and still appends the target warnings. - proxy.ts: main's pool-affinity capture and credential logging stay; the web-search query is still read from the request kind decided once. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The test added in 3f908e7 moved HOME to a temporary folder, but on Windows os.homedir() reads USERPROFILE, so the reader looked in the real home and the test failed there. It now sets both and restores each, unset ones included. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Three conflicts, each kept from both sides: - agents.ts: main moved the worker loop into agentTargets(); the loop there now asks isTargetProvider, as this branch's copy did. - providers/google: handle() takes main's credential handoff and this branch's request kind, in the order the OpenAI adapter takes them. - proxy.ts: the route still comes from decide() once, and main's worker ladder fallbacks then apply to it. canRunServerTools stays removed (targets.ts answers it); main's new ladder and model-health helpers are kept. The Google call passes both the credential and the kind. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PBJ-2
left a comment
There was a problem hiding this comment.
Thank you for tracking down the classifier prompt additions and for consolidating both the routing decision and the provider capability rules. I reviewed what #98 adds on top of #91 and tested a merge with current main (6158026). I found no blocking issues in the refactor itself.
- The prompt-integrity defect is real, and the fix works. On current main the translated paths add identity/addendum text, and ChatGPT adds an image hint, in front of the classifier's security prompt. The merged tree keeps the classifier system text intact. I also checked real local HTTP upstream requests through both OpenAI wires, Google and Grok; ChatGPT and Anthropic-compatible are covered by the existing adapter and proxy tests.
- The merge and regression checks are green. The merge is conflict-free; typecheck,
check:modand the full suite pass (2,695 tests: 2,679 passed, 16 skipped, 0 failed). A 35,840-case routing/refusal comparison against main found no differences, and 90 serialization comparisons for ordinary, search and fetch requests matched apart from the existing random per-turn ID. That checks prefix stability, not a live cache-hit measurement. - Two optional test improvements. In
packages/cli/test/launcher.test.ts:56, the symlink-capability skip also skips the unrelated legacy-ownership assertions; splitting them would keep that coverage on accounts without symlink privileges. The classifier tests inopenai-translate.test.ts:228andgoogle-translate.test.ts:44callpromptAdditionsdirectly, so an adapter-level test would better protect the hand-off of the request kind.
The Windows CI test step genuinely succeeds here, not just under continue-on-error.
One thing gates the merge: this branch contains #91, which still has the save race described in my review there. Once that fix lands (in #91, then merged into this branch), this is ready to merge. The test improvements above are non-blocking.
The legacy-evidence test was skipped wherever file symlinks cannot be made, though only its linked-home check makes one, and that check was already limited to POSIX hosts. The linked-home check is now a test of its own, skipped on Windows, and the legacy and unknown-file assertions run on every account. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
#91 is merged now. Could you merge current main into this branch so the diff shows only the refactor and the Windows fixes? Once that's green, I'll merge this as reviewed. |
…dle() The classifier tests for the OpenAI-compatible and Google translations called promptAdditions() directly, so they would not notice the request kind being lost on its way into the adapter. The OpenAI (both wires), Google and Grok adapter tests now pass the kind from requestKind() into handle(), as the ChatGPT adapter test already did, and send a classifier request and an ordinary turn through an adapter with an addendum: the classifier's text arrives exactly as sent, and the turn still gets the identity line and the addendum. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
No conflicts. Main now holds PBJ-2#91, so the PR's diff against main is the refactor and the Windows fixes alone. Main's new code repeats none of the target, server-tool or prompt rules this branch moved into targets.ts and identity.ts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Main's harness (495df86) now runs the page's own renderAutoClassifier, which on this branch asks isTargetProvider whether a Claude provider is an account pool. The harness slices that function from app.js too, so the redraw test runs the page's code instead of failing on a missing name. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merged current main (0.12.21, with #91) in d2a9197, so the diff against main is now the refactor and the Windows fixes only. The merge had no textual conflicts, but main's harness from 495df86 runs the page's own Both optional test improvements are in:
Typecheck and |
This PR builds on PBJ-2#98, which fixes the Windows tests that fail on main. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This PR builds on PBJ-2#98, which fixes the Windows tests that fail on main. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The deferred cleaner's PowerShell did not write "running" within 5 s on a Windows CI runner (the run of PBJ-2#116, 2026-10-29), and uninstall --purge reported that cleanup could not start. A PowerShell start took over 5 s on a loaded desktop the same day. The cleaner now has 20 s to be launched and 15 s to answer; a working one still returns as soon as it answers. The test's child gets 60 s so that the longer waits fit inside it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rt wait Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rt wait Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The deferred cleaner writes its receipt and then exits; until it has, Windows keeps its redirected logs open. Removing the folder at once failed with EPERM in the self-purge test's after hook on a CI runner (2026-10-29), although every assertion had passed. Both cleaner tests now remove it through one helper that retries for up to 5 s. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…p retry Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…p retry Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The snapshot test makes directOn's backup fail by taking the backup's name
first. The name carries the time to the second, and on a Windows CI runner
the second turned over between the test's name and directOn's, so nothing
failed ("Missing expected exception", 2026-10-29). The test now takes this
second's name and the next two.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… a direct test Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… a direct test Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Repeating the purge tests 20 times on 2026-10-29 failed in three ways, each also reachable by `uninstall --purge` itself: - The CLI read the receipt while the cleaner's Set-Content was writing it, got EBUSY, and reported a cleaner that never started. A busy or empty receipt is now read again on the next round. - After Wait-Process returned, Get-Process by id still listed the exited Node process, and the receipt said "uninstall process has not exited". The cleaner now holds that process's handle and waits on HasExited. - Set-Content could not write the final receipt while a reader had it open, so a purge that had removed the folder recorded itself as failed. The receipt is now written with WriteAllText, which shares the file with readers, and a refused write is tried again. The tests read a receipt that is mid-write as not yet there, and remove the cleaner's folder with their own retry: rmSync's maxRetries does not cover the EPERM of its still-open logs on Node 24. 30 runs in a row passed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ver tests On PBJ-2#98's Windows run at f5b9cbd, two permission tests hit their 4.5 s limit and a socket retry test ran out of waitFor's 2 s while the host was still answering; the same tests had passed on the runs before. The limits are for a stalled test, and a passing one never waits them out, so each file now uses TEST_TIMEOUT (15 s) and WAIT_LIMIT (10 s). 211 tests pass locally. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…Windows CI Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…Windows CI Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…home The Windows task lock lives under os.homedir(), which reads USERPROFILE on Windows. ownership.test.ts and service-races.test.ts set HOME only, so both took the real home's lock for the default task name, in parallel: on PBJ-2#117's Windows run, service-races found ownership's lock before its owner file was written and failed with "서비스 작업 잠금의 소유자를 확인할 수 없습니다" instead of the race it tests (2026-10-29). Both now point USERPROFILE at their temp folder too, as desktop-legacy.test.ts does; neither touches ~/.clauderipple/service-locks any more. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n tests Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n tests Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What this is for
Two refactors from an architecture review of
mainplus #91, and a fix for the Windows CI job.A request's kind is decided once (review candidate 1). Whether a request is an Auto mode classifier stage, a web search, a web fetch or an ordinary turn used to be worked out on several lines of
proxy.ts, and the answer stopped there: the provider adapters never saw it. Now it is decided once, carried on the route, and read by the adapters. That fixes a defect. ARCHITECTURE §2 promises that the classifier's security prompt reaches its target intact, but every translated provider added to it. A classifier request routed to ChatGPT got this in front of the security prompt, and the image-generation hint (acurlrecipe) after it:The OpenAI-compatible, Google, Grok and Anthropic-compatible paths added the identity line and
instructionsAppend. Nothing in the logs showed it.What a provider can do is answered in one module (candidate 3). "Can this provider take routed requests", "who declares this model id", "which efforts does it take" and "can it run web search" were each answered in several places, in two spellings for the first.
targets.tsanswers all four.validateand then dropped by routing without a word. It is still accepted, because refusing it would break configs that load today. But the config save now answers with a warning for each such rule, and the router logs it when it reads the config.The suite passes on
windows-latest.main's Windows job fails 19 tests, and the causes are listed below. Fixing them turned up four product bugs on Windows, also fixed here.Depends on #91. This branch is built on #91's and merges it again after each of #91's updates, so #91's commits and the
mainthey merge show here too.Review document
The review, with a before/after flow graph for each candidate, is in this PR at
docs/reviews/2026-10-24-architecture-review.md. This PR carries its candidates 1 and 3.Before
flowchart TB REQ([Messages request]) --> H["proxy.ts handle()"] H -->|"purpose check"| REC[(requests.jsonl)] H --> RES["resolve()"] H -->|"the same rules again"| UR["unroutableReason()"] RES -->|"Resolved: provider, model, tag"| AD{adapter} AD --> CG["chatgpt: adds identity line + image hint"] AD --> OG["other translated providers: add identity + addendum"] RES -.->|"splits the tag string"| FO[failover] H & AD & RT2[agents · model scores · validate · admin] --> Q{{"routable target? two spellings, a dozen places"}} classDef leak stroke:#c62f2f,stroke-width:2px,color:#c62f2f; class UR,CG,OG,FO,Q leakAfter
flowchart TB REQ([Messages request]) --> K["requestKind()"] K -->|kind| D["decide()"] D -->|"route: kind, asked"| AD{adapter} D -->|refusal| X[400 refused] D -->|"no route"| PASS[passed to Anthropic as is] AD --> PA["promptAdditions(kind)"] PA -->|"classifier: nothing added"| CG[chatgpt · translated providers] D -->|asked| FO[failover] D & AD & RT2[agents · model scores · validate · admin] --> T["targets.ts"] classDef deep fill:#15243a,stroke:#15243a,color:#e8eef7,stroke-width:3px; class K,D,PA,T deepWhat changes
Request kind (candidate 1)
routing.tsRequestKind(web-search,web-fetch,auto-mode-classifier,turn) andrequestKind(json, pathname), whose order is the precedence.Resolvedcarrieskindandasked(the model id the client sent).decide(model, body, cfg, kind)returns the route, or the reason to refuse it, in one pass.resolve()stays for its other callers asdecide(…).route.unroutableReason, which re-implementedresolve's rules to explain them, is gone. Every way of finding no route ends in onenoRoute(), which applies the refusal rules in the orderunroutableReasonapplied them, including 모델명 중복으로 호출이 안 되는 문제(Opencode Go 관련) #94's qualified ids and the pass-through for Claude requests with a delegation marker.identity.ts:promptAdditions(kind, provider, imageHint?)is the one rule for what a routed system prompt may gain. A classifier request gains nothing.proxy.tscomputes it once, callsdecideonce, and failover namesaskedinstead of splitting the log tag.Provider capabilities (candidate 3)
targets.tsisTargetProvider/ingressOnly: one spelling of the target rule,accountPool: truewithauth: "claude-code".declaredBy,qualifiedModel,rawDeclaredBy,soleOwner: moved fromrouting.ts, still re-exported there.effortLevels: moved out of the HTTP module the proxy imported it from, still re-exported fromadmin.ts.runsServerTools/answersWebSearch: the proxy'scanRunServerToolsand the admin's hand-copied mirror of it become one implementation.targetWarnings: the save warning and the load-time log line described above.validate, the proxy, agent files, model scores, the admin status) use it instead of spelling the rule themselves.isTargetProviderinapp.js, in place of three inline checks.ui-targets.test.tsholds that copy to the router's for every provider shape.Windows CI
Same folder, different spelling. The runner's TEMP is the 8.3 alias
C:\Users\RUNNER~1\…, while the product records the native long form, and on Windows the installation home in lowercase. The fixtures now resolve their roots withfs.realpathSync.nativeand compare through the product'snormalizeHome. The desktop-legacy fixture also setsUSERPROFILE, whichos.homedir()reads on Windows.Other test-side causes:
normalizeHomeresolves links for POSIX homes and compares Windows paths as text.folders.test.tsalready covers those).Product bugs on Windows the failures exposed:
images.ts, two causes of a wrongly blocked image.tray.ts: on WindowsrenameSyncreplaces an existing file, so an occupied backup path let the swap replace the working runtime. The swap now refuses an occupied backup path.launcher.ts: removing the tray launcher usesunlinkSync, becausefs.rmSync({ force: true })on a hostile file name ended the Node 24.1 process with0xC0000409.The workflow's comment says to drop
continue-on-errorfor Linux and Windows once they pass. I left that to you.Behaviour
Unchanged apart from the fixes above, plus four notes:
instructionsAppendthe way the other providers already did. An addendum with leading or trailing whitespace changes once, which costs one prompt-cache miss.accountPool: trueand an auth other thanclaude-codeis refused byvalidatebut adopted at startup. It is now ingress-only everywhere; before, routing treated it as a target and the proxy then answered 400.warningmay now hold several lines: the model-slot warning first, then one line per ignored route or direct rule.Found, not changed
Reading every call site for candidate 3 turned up places where two parts of the code answer the same question differently. Changing them changes behaviour, so they are listed here rather than changed:
ConfigStore.reloadadopts an invalid config on first load, so the guaranteesvalidategives do not hold at startup.provider:modelids, and writes direct prefix rules instead.ultraforgpt-6.1-sol.validateaccepts any provider aswebSearch.openai-compatiblebackend is served through its web plugin, though the status says it cannot search.serveWebSearchgives it no backend role.normalizeHomecompares Windows paths as text, so a junction or an 8.3 spelling of the same home reads as another installation.Verification
windows-latestat ffe90c8On this machine, seven tests fail, and none of them fails on the runners:
Windows self-purge waits for its Node process…depends on timing. It passes when its file runs alone.loopback idle timeout closes a stalled response bodyanswers 503 here onmaintoo, with or without proxy variables, and passes on all three runners. I did not dig further.On the runners, all three jobs pass at ffe90c8. The Windows job went from 19 failures (
main) to 5 at d4d5736 and to none at ffe90c8, which confirms the elevated-owner cause of the last two image failures.npm run typecheckandgit diff --checkpass.decide()was compared with #91'sresolve()andunroutableReason()over 41,664 combinations of config, model and body: 16,124 routed, 10,545 passed through, and the rest refused with all five messages. There were no differences.New tests:
requestKindprecedence andkind/askedon every kind of route (routing.test.ts).promptAdditions(identity.test.ts).targets.test.ts,ui-targets.test.ts, and the warning on a config save (admin.test.ts).🤖 Generated with Claude Code