The dropped-upstream warning hedges about a table it already has (#676) - #678
The dropped-upstream warning hedges about a table it already has (#676)#678philcunliffe wants to merge 5 commits into
Conversation
`gateway_upstreams_dropped` told the operator that traffic for a dropped upstream is not proxied "unless an adapter preset already covers the same name". True on every branch, but the daemon publishes the answer: `details.registered_presets` is the list of presets adapter plugins registered at activation, and `mergeUpstreams` backfills one into exactly the names the compiled config table is missing - which is what a dropped entry's name is, by definition. Intersecting the dropped names with that list decides, per name, between two very different faults that wanted different fixes: - no preset covers it: requests match no route, nothing is proxied and nothing is captured. - a preset covers it: traffic still flows, to the preset's default endpoint rather than the `base_url` the operator's entry meant to set. The warning still has to fire, because the override silently did not take effect, but calling that silence was wrong. The hedge is confined rather than deleted, to the two shapes that still withhold the answer: a status file with no `registered_presets` key at all (absent is not empty, and reading a missing field as "no presets" would manufacture a confident claim of silence), and a drop this build could attach no name to (`name` is one of the two keys whose absence drops an entry, so an unnamed drop has nothing to intersect; the same guard covers the deduped case, where two same-named entries drop and yield one name for two drops). Also moves the names off the configured-upstreams noun on the bound branch. "1 of its 2 configured upstreams (openai)" lists the *dropped* name against the *configured* phrase, which reads for a moment as the configured set; the names now sit at the consequence they belong to, and the surviving hedge labels its parenthetical `(dropped: openai)`. The idle branch keeps its wording untouched: there every configured entry is also a dropped one, so the parenthetical states a fact. No change to when either kind fires, to their non-degrading severity, or to LLP 0195, which decides the configured-vs-compiled comparison and not what the resulting sentence says. Issue #676 item 3 (whether a fully broken gateway-only install should flip `overall`) is untouched: LLP 0195 §consequences records it as an open product-policy question, and it needs a human, not a patch. Tests: the silent drop, the preset-covered drop and a mixed one each assert their own definitive sentence and the absence of the other's; both withholding shapes assert the hedge survives; and one test pins that no parenthetical hangs off "configured upstreams". The existing preset-backfill test, which pinned the hedge, now pins the definitive sentence instead. Co-Authored-By: Claude <noreply@anthropic.com>
…y can settle
The commit under review replaced a hedge with two per-name assertions. Both
overreached past what the status file knows, because the gateway routes by
`path_prefix` and `match()` while the attribution is an intersection of *names*:
- "nothing is proxied or captured for openai" is false whenever a surviving
upstream written with no `path_prefix` compiles to the `/` catch-all, which
`pathMatchesPrefix` matches every path against. The dropped entry's traffic
is then proxied and recorded under the other upstream's name. The gateway
source already says so where it logs this very fault ("falls through to
whatever the remaining routes match (or nothing)"). Now scoped to the name,
with the fall-through spelled out.
- "at the preset's default endpoint rather than the base_url this config meant
to set" names the wrong single field. `mergeUpstreams` backfills the preset's
*whole* entry, so its `path_prefix` is in force too and a client still
pointed at the operator's prefix gets a 404 from a gateway this line calls
healthy. Now names both fields.
- The covered clause pluralised only its verb, describing two presets at two
endpoints as "the adapter preset ... under the same name".
Three tests, each failing on e482b56: they pin the routing facts (the `/`
catch-all match, and `mergeUpstreams` reverting `path_prefix`) rather than the
sentence alone, so a later edit that reaches back for the stronger claim fails
here instead of in front of an operator.
npm test 3798 pass / 0 fail / 6 skipped, typecheck clean.
Co-Authored-By: Claude <noreply@anthropic.com>
Review round 1 (neutral, own-PR) - findings, fixed and pushedReviewed Verdict: findings (2 major, 1 minor). All fixed on the branch at The change is right in kind: the daemon does publish the answer, 1. MAJOR - the "silent" clause is false whenever a surviving catch-all takes the traffic
[[upstreams]]
name = "anthropic"
base_url = "https://api.anthropic.com" # no path_prefix -> the '/' catch-all
[[upstreams]]
name = "openai"
url = "https://api.openai.com" # typo -> dropped
path_prefix = "/openai"emits the sentence above, while The text was equally wrong before this commit - but the hedge was the escape hatch, and this commit removed it and added Fixed: the clause is now scoped to the name, which is what the intersection actually settles, and names the fall-through: 2. MAJOR - the "covered" clause names the wrong single lost field
Fixed: 3. MINOR - covered clause pluralised only its verb
With two covered names it read Fixed: Tests addedThree, in
Evidence check on the PR bodyVerified rather than trusted. The six new tests were run against Item 3 (the
|
…and asserted the traffic one Round 1 removed "traffic meant for it is not proxied and nothing is captured" from the definitive branch because a surviving upstream with no `path_prefix` compiles to the `/` catch-all and takes the dropped entry's traffic. It left the identical sentence in the hedged branch, whose JSDoc then called it "a weaker sentence, not a wrong one". It is a wrong one, and on a current build rather than only on an old status file: `name` is one of the two keys whose absence drops an entry, so a nameless drop is an ordinary config that reaches the hedged branch (nothing to intersect with `registered_presets`). Not knowing which preset covers a name says nothing about whether a catch-all takes the path. The hedged sentence is now bounded to the name the same way the definitive one is, and pluralises with the drop count. Co-Authored-By: Claude <noreply@anthropic.com>
…ch cannot reach
"X is still proxied by the adapter preset registered under the same name" is
a claim about routing, and the only evidence behind it is an intersection of
two name lists. Two ordinary configs falsify it:
- The backfilled preset can be shadowed outright. `mergeUpstreams` appends
presets after the config entries and proxy.js's `compileUpstreams` breaks a
rank tie on that order, so a surviving config upstream at an equal
`path_prefix` wins every path the preset would have taken. The preset is in
the table and routes nothing.
- "the preset's own base_url and path_prefix are what is in force" is wrong
for every bundled adapter preset, because they all carry a `match()` and
`matchUpstream` never consults `prefix` when one is present. The claude
preset takes `/v1/complete` and any anthropic-headered path; naming
`path_prefix` understates its reach as much as it overstates the
operator's.
The clause now states the table fact it can settle ("in the routing table
only as the adapter preset registered under the same name"), names the
routing surface rather than one field of it, and does not deny the
outranking. Tests assert `matchUpstream` over the real sorted table, which
is the thing the sentence is about, rather than the preset entry's presence.
Also: the hedged branch drove its name plurals off the dropped-entry count,
which the dedupe path makes disagree with the names it prints.
Co-Authored-By: Claude <noreply@anthropic.com>
… 181-char line Both are the same edit as the clause change: a test named "... is still proxied" is a contract statement, and that is the contract this round removed. Co-Authored-By: Claude <noreply@anthropic.com>
Review round 2 (neutral, own-PR) - findings, fixed and pushed. LAST ROUND.Reviewed Verdict: findings (3 major, 1 minor). All fixed. Head is now Round 1's fix did not hold. It correctly identified that a status message must not assert more than the status file supports, then applied that rule to exactly one of the three sentences that violate it. Two of the three findings below are the same defect class as round 1's own finding 1, in the sibling branch it declared sound and in the clause it rewrote. 1. MAJOR - the
|
| branch | message tail |
|---|---|
| hedge, no names | that entry is not in the routing table, so unless an adapter preset already covers the same name, nothing is proxied or captured under that name, and a request aimed at it gets a 404 or falls through to whatever surviving route its path matches |
| hedge, 2 names | those entries are not in the routing table (dropped: x, y), so unless ... the same names, nothing is proxied or captured under those names, and requests aimed at them get a 404 or fall through to whatever surviving route their paths match |
| silent x1 | nothing is proxied or captured under the name openai (no adapter preset covers that name), so a request aimed at it gets a 404 or falls through ... |
| silent x2 | ... under the names openai, gemini (no adapter preset covers those names), so requests aimed at them get a 404 or fall through ... |
| covered x1 | anthropic is in the routing table only as the adapter preset registered under the same name, so that preset's own base_url and routing rules are in force, nothing this config set for it took effect, and a surviving upstream can still outrank the preset on any path |
| mixed | silent clause ; covered clause |
Tests
Five new, plus five existing ones updated to the new contract (a test named ... is still proxied ... is a contract statement, and that is the contract this round removes). They assert matchUpstream over the real sorted routing table, which is what the sentence is about, rather than the preset entry's mere presence in mergeUpstreams's output - which is precisely the gap that let findings 1 and 2 through round 1.
a covered name is not claimed to be proxied, because a surviving upstream can shadow the preset(finding 1) - builds the real table, assertsmirrorwins both paths, then asserts the messagea covered name does not claim path_prefix is in force, which a match()-carrying preset never routes on(finding 2) - uses the realanthropicUpstreamPreset(), asserts it takes/totally/elsewherethe hedged branch does not claim the traffic is dead either, because a catch-all still takes it(finding 3)the hedged branch pluralises for a multi-entry dropand... names off the names, not off the entry count(finding 4)
All five fail against a0a9633/013b500 and pass on b0a9e41. Verified by checking out only status.js at the earlier sha with the new tests in place.
Checked and sound: round 1's guard reasoning
Verified against the code rather than accepted. The two withholding guards cannot be defeated:
Array.isArray(details.registered_presets)- the source publishes it asArray.from(state.presets.keys())(source.js:101); thenumbershape atsource.js:195/203is a log attribute, not status details, and is correctly rejected.names.length !== dropped-droppedNames(source.js:418-446) is the set of distinct names for which no entry compiled, so each contributes >= 1 dropped entry:|droppedNames| <= droppedalways, with equality iff every dropped entry is named and all names are distinct. Checked the three cancellation candidates explicitly (duplicates where one compiles ->keptswallows the name -> mismatch -> hedge; duplicates where neither compiles -> dedupe -> mismatch -> hedge; non-object array entries -> hedge). Sound.registerUpstreamPreset(api.js:47-53) does reject a preset with neithermatch()norpath_prefix, andstate.presets.setappears only there (api.js:55), somergeUpstreams's filter is unreachable via the capability API. Round 1 was right about this.- Preset registration timing: all bundled adapters register in
activate(), and the daemon starts sources only after every plugin activates (daemon/runtime.js:91). Confirmed.
The guards were never the problem. The sentences downstream of them were.
Not fixed, deliberately
- Item 3 of Follow-up: deferred review findings from PR #658 #676 (should
overallflip for a fully broken gateway-only install). Untouched: no code, no test, no LLP edit. Reserved product-policy question per Follow-up: deferred review findings from PR #650 #653 and LLP 0195 §consequences. Confirmed by inspection. - "that name" has no printed antecedent on a nameless drop. The hedge branch can fire with no name in the message at all, so "the same name" / "that name" refer to a name the operator never sees, and the hedge is arguably vacuous there (an entry with no name cannot collide with a preset). Real but not false, and closing it needs the status file to distinguish a nameless drop from a deduped one, which it cannot: both surface only as
names.length !== dropped. That is a source-side change (upstreams_dropped_unnamed, say), not a wording one. registered_presetsis snapshotted atstatus()time, the table atlaunchListenertime. A preset registered after source start would reportcoveredwhile absent from the table. Unreachable with bundled plugins (all register inactivate()), same class as finding 1, and not fixable from this side.- Dropped names are still not run through
sanitizeLabel/ capped the wayrecentEntrypointsFromSourcesis, though both read the same untrusted file. Round 1 deferred this; it predates the whole change and still belongs to a separate one. - PR body is stale. Its before/after table and quoted strings describe
e482b56, three commits back. Worth a refresh before merge; not blocking.
LLP 0195 gloss erratum: I agree with the disposition
Verified independently: git cat-file -e origin/master:llp/0195-... fails - LLP 0195 does not exist on master. It was minted inside #658, which is open, so its Accepted status is branch-local and nothing in the merged corpus is contradicted yet.
The gloss in §visible-when-unintended - "traffic for the dropped upstream is silently unrouted and uncaptured" - is wrong for the no-preset case (the / catch-all takes it) and the preset-covered case (a preset backfills it), and after finding 1 it is wrong in a third way: even a preset-covered name may be shadowed and route nothing. Three ways, one clause.
#658 should carry the one-clause erratum before it merges, and #678 should not touch the doc. A stacked PR must not rewrite a doc its base introduces - the edit would land in the wrong PR and dirty the eventual retarget to master. And the window matters: while #658 is open the doc is still a proposal, so an erratum costs nothing; once it merges the gloss is a record and correcting it needs a new LLP or at minimum an Extended-by: forward-ref. The erratum settles nothing new - which comparison fires which kind, and that both are non-degrading, are untouched.
Conventions
No semicolons, no U+2014 anywhere in the three files, JSDoc-only types, no inline import('...'), no @typedef. DroppedUpstreamAttribution is an interface in types.d.ts reached through the existing root-anchored '../../../src/core/daemon/types.js' specifier. Both @ref anchors resolve to live targets (llp/0195-...md:67 and :89). One 181-char line in the round-1 tests split.
Checks on b0a9e41
npm test: 3803 pass / 0 fail / 6 skipped (was 3798 ata0a9633; +5 new tests)npm run typecheck: cleannpm run smoke -- status_diagnostics: ok- CI: all 9 checks green
For the human disposing of this
The ladder caps at 2 rounds, so this is the last automated pass. What is left for you:
- Item 3 of Follow-up: deferred review findings from PR #658 #676 still needs an answer (confirm the open question stands, or settle it in a follow-up LLP). A partially dropped gateway upstream is visible in status, and LLP 0193 records why the gateway idles (#653) #658 left the seam:
gateway_idle_no_upstreamsandgateway_upstreams_droppedare already distinct kinds. - A partially dropped gateway upstream is visible in status, and LLP 0193 records why the gateway idles (#653) #658 needs the LLP 0195 erratum before it merges.
- Retarget The dropped-upstream warning hedges about a table it already has (#676) #678 to
masteronce A partially dropped gateway upstream is visible in status, and LLP 0193 records why the gateway idles (#653) #658 merges. - The PR body wants a refresh against
b0a9e41.
Triage (neutral): PR can ship - no unresolved blocking findings at
|
philcunliffe
added
the
neutral:approved
1 participant
Deferred review findings from PR #658, per issue #676. Two of the three are fixed here; the third is a human decision and is deliberately untouched.
Item 1 (fixed): the partial-drop warning hedges where it could be definitive
The message said traffic for a dropped upstream is not proxied "unless an adapter preset already covers the same name". The daemon already publishes the answer.
details.registered_presetslists what adapter plugins registered at activation, andmergeUpstreamsbackfills a preset into exactly the names the compiled config table is missing - which is what a dropped entry's name is, by definition. The intersection now decides per name:unless an adapter preset already covers the same name, traffic meant for it is not proxied and nothing is capturednothing is proxied or captured for openai (no adapter preset covers that name)anthropic is still proxied by the adapter preset registered under the same name, at the preset's default endpoint rather than the base_url this config meant to setThose are two different faults wanting two different fixes, and the preset-covered one is not silence: the warning still fires (the operator's
base_urlsilently did not take effect) but no longer miscalls it.The hedge is confined, not deleted. It survives verbatim for the two shapes that genuinely withhold the answer, because this reads a file another build may have written:
registered_presetskey. Absent is not empty; reading a missing field as "no presets registered" would manufacture a confident claim of silence.nameis one of the two keys whose absence drops an entry, so an unnamed drop has nothing to intersect. The samenames.length === droppedguard covers the deduped case (two same-named entries dropping yield one name for two drops).Item 2 (fixed): the
(names)parenthetical is ambiguous1 of its 2 configured upstreams (openai)hung the dropped name off the configured noun. On the bound branch the names now sit at the consequence they belong to, and the surviving hedge labels its parenthetical(dropped: openai).The idle branch is deliberately untouched, avoiding the churn the issue flagged: there every configured entry is also a dropped one, so
2 upstreams (anthropic, openai) are configuredstates a fact.Item 3 (NOT fixed - human decision, by design)
Whether
overallshould flip todegradedfor a fully broken gateway-only install. Issue #653 reserved this for a human; LLP 0195 §consequences records it as an open product-policy question that "this document does not settle". That is a decision, not a defect: inventing a behaviour change would settle it by patch, and editing an Accepted LLP to close it is exactly what the LLP conventions forbid. Left entirely alone - no code, no test, and no LLP edit. It still needs a human to either confirm the open question is the intended record or answer it with a follow-up LLP, using the seam #658 left (gateway_idle_no_upstreamsandgateway_upstreams_droppedare already distinct kinds).Evidence
Six new tests, written before the fix and failing on
fix/issue-653's head (a62c75c):All pass after. The existing
a dropped upstream whose name matches a registered adapter preset...test pinned the hedge, so it is updated to pin the definitive sentence instead - that assertion was the old contract this change replaces.They drive the real gateway source and the real
compileUpstreams/mergeUpstreams(viarealGatewayDetails), not hand-written details, so they fail if the drop rule and the published preset list ever disagree.npm test: 3795 pass / 0 fail / 6 skipped (was 3789 pass; +6 new tests)npm run typecheck: clean@refanchors resolve (#visible-when-unintended,#consequencesin LLP 0195)LLP
No LLP change. LLP 0195 is Accepted and decides when the two kinds fire and that they are non-degrading; neither moves here. The new
@refis[constrained-by]and says so. Note that the flat gloss in §visible-when-unintended ("traffic for the dropped upstream is silently unrouted and uncaptured") was already imprecise relative to shipped code on this branch - commiteee845cin #658 introduced the hedge for exactly the preset-backfill case. This moves further in the direction #658 already went rather than opening a new gap.Fixes #676