fix: stop the exclusion scan writing the rendered list's report (#1909)
Copilot round 10. The scan no longer publishes its malformed entries into the tools/list report at all. Round 9 only stopped it overwriting the aggregate's; the remaining case is unsalvageable for a stronger reason than 'two different requests': - a FAILED strict aggregate runs salvage, which writes the report itself; - a SUCCESSFUL one means every entry parsed, so the rendered list contains no malformed entries. So by the time the scan finds something, either the aggregate already reported it with indices matching the screen, or the aggregate succeeded and what the scan found is provably not in the list being displayed. Publishing could only ever name entries the user cannot see. Nothing is lost: listToolsForScan still marks its own refused response, so the finding surfaces on the Protocol entry for the exchange that carried it, where it is true. The per-page accumulation and raw-entry offset existed only to feed the removed publish, and go with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RDXCSrnSBV7vZfxycG5Z6E Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
efd0bf2b7c
commit
77ff30bbc9
2 changed files with 34 additions and 51 deletions
|
|
@ -535,10 +535,12 @@ describe("InspectorClient list salvage (#1909)", () => {
|
||||||
log.destroy();
|
log.destroy();
|
||||||
});
|
});
|
||||||
|
|
||||||
it("reports scan-dropped tools at their position in the whole list", async () => {
|
it("indexes a dropped tool against the whole aggregate, across pages", async () => {
|
||||||
// The exclusion scan walks pages of its own, so its report has to be
|
// The reported index has to be offset by the RAW entries of PRIOR pages —
|
||||||
// offset by the RAW entries of PRIOR pages. An unlabeled entry's index is
|
// valid and malformed alike, not just the kept ones. An unlabeled entry's
|
||||||
// all the user has to go on, so a wrong offset points at nothing.
|
// index is all the user has to go on, so a wrong offset points at nothing.
|
||||||
|
// This is the aggregate salvage's walk: it owns the `tools/list` report,
|
||||||
|
// because it is the walk that produced the list being rendered.
|
||||||
const server = await startMalformedServer(
|
const server = await startMalformedServer(
|
||||||
{
|
{
|
||||||
tools: [
|
tools: [
|
||||||
|
|
@ -610,12 +612,12 @@ describe("InspectorClient list salvage (#1909)", () => {
|
||||||
expect(connected.getMalformedListItems()).toEqual([]);
|
expect(connected.getMalformedListItems()).toEqual([]);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("does not let the scan overwrite the aggregate's report", async () => {
|
it("keeps the aggregate's report when the scan sees a different list", async () => {
|
||||||
// Both walks run on a modern connection and they are separate, uncached
|
// Both walks run on a modern connection and they are separate, uncached
|
||||||
// exchanges. The aggregate is the list being RENDERED, so its report owns
|
// exchanges. The aggregate is the list being RENDERED, so it owns the
|
||||||
// the indices; the scan must not replace them with its own — the server can
|
// report; the scan never writes it. Here the server answers the scan's
|
||||||
// answer the second request differently, and then the warning would point
|
// request with a different malformed shape — which, if the scan published,
|
||||||
// into a list nobody is looking at.
|
// would leave the warning pointing into a list nobody is looking at.
|
||||||
const server = await startMalformedServer(
|
const server = await startMalformedServer(
|
||||||
{
|
{
|
||||||
tools: [
|
tools: [
|
||||||
|
|
|
||||||
|
|
@ -3579,10 +3579,6 @@ export class InspectorClient extends InspectorClientEventTarget {
|
||||||
metadata?: Record<string, string>,
|
metadata?: Record<string, string>,
|
||||||
): Promise<ExcludedTool[]> {
|
): Promise<ExcludedTool[]> {
|
||||||
const excluded: ExcludedTool[] = [];
|
const excluded: ExcludedTool[] = [];
|
||||||
const malformed: MalformedListItem[] = [];
|
|
||||||
// Raw entries seen so far, across pages — the frame the reported indices
|
|
||||||
// are relative to (see the mapping below).
|
|
||||||
let rawOffset = 0;
|
|
||||||
// Gated to connections that actually exclude; otherwise this is a pure
|
// Gated to connections that actually exclude; otherwise this is a pure
|
||||||
// no-op (no round trip). The raw `listTools` below guards the connection.
|
// no-op (no round trip). The raw `listTools` below guards the connection.
|
||||||
if (this.excludesInvalidXMcpHeaderTools()) {
|
if (this.excludesInvalidXMcpHeaderTools()) {
|
||||||
|
|
@ -3597,18 +3593,6 @@ export class InspectorClient extends InspectorClientEventTarget {
|
||||||
if (pages >= LIST_MAX_PAGES) throw listPaginationExceeded("tools/list");
|
if (pages >= LIST_MAX_PAGES) throw listPaginationExceeded("tools/list");
|
||||||
pages += 1;
|
pages += 1;
|
||||||
const page = await this.listToolsForScan(cursor, metadata);
|
const page = await this.listToolsForScan(cursor, metadata);
|
||||||
malformed.push(
|
|
||||||
...page.malformed.map((entry) => ({
|
|
||||||
...entry,
|
|
||||||
// Offset by the RAW entries of PRIOR pages only. `excluded.length`
|
|
||||||
// would be wrong twice over: it counts just the invalid-header
|
|
||||||
// tools, and on page one it would also fold in this page's own
|
|
||||||
// valid count — putting an unlabeled entry, whose index is all the
|
|
||||||
// user has to go on, at a position that doesn't exist.
|
|
||||||
index: entry.index + rawOffset,
|
|
||||||
})),
|
|
||||||
);
|
|
||||||
rawOffset += page.tools.length + page.malformed.length;
|
|
||||||
for (const tool of page.tools) {
|
for (const tool of page.tools) {
|
||||||
const scan = scanXMcpHeaderDeclarations(tool.inputSchema);
|
const scan = scanXMcpHeaderDeclarations(tool.inputSchema);
|
||||||
if (!scan.valid) excluded.push({ tool, reason: scan.reason });
|
if (!scan.valid) excluded.push({ tool, reason: scan.reason });
|
||||||
|
|
@ -3621,28 +3605,24 @@ export class InspectorClient extends InspectorClientEventTarget {
|
||||||
}
|
}
|
||||||
} while (cursor !== undefined);
|
} while (cursor !== undefined);
|
||||||
}
|
}
|
||||||
// Report what the scan itself had to drop — but only when the aggregate
|
// The scan deliberately does NOT publish what it dropped into the
|
||||||
// salvage left nothing to say.
|
// `tools/list` report. That report is rendered above the Tools list as
|
||||||
|
// "entries dropped from this list", and the aggregate is what that list
|
||||||
|
// is — so only the aggregate's salvage may write it.
|
||||||
//
|
//
|
||||||
// The aggregate OWNS this report, because the aggregate is the list the
|
// The reason is stronger than "these are two different requests". A
|
||||||
// user is looking at. This walk is a second, deliberately un-cached
|
// successful strict aggregate means every entry parsed, so the rendered
|
||||||
// exchange, so its entries need not be the rendered ones: the server can
|
// list has no malformed entries in it at all; and a FAILED aggregate runs
|
||||||
// change between the two requests, and a cache-served aggregate is a
|
// salvage, which writes the report itself. So by the time this walk finds
|
||||||
// different list version entirely. Overwriting an aggregate report with
|
// something, either the aggregate already reported it (with the indices
|
||||||
// this one would then replace accurate indices with indices into a list
|
// that match what is on screen), or the aggregate succeeded and whatever
|
||||||
// nobody is seeing.
|
// this walk just found is provably not in the list being displayed — the
|
||||||
|
// server changed, or the aggregate came from cache. Publishing here can
|
||||||
|
// therefore only ever name entries the user cannot see.
|
||||||
//
|
//
|
||||||
// It still publishes when the aggregate said nothing, which is the case
|
// The finding is not lost: `listToolsForScan` marks the scan's own refused
|
||||||
// this exists for: a cache-served aggregate succeeds without ever running
|
// response, so it surfaces on the Protocol entry for the exchange that
|
||||||
// salvage, so without this a malformed tool would be dropped from the scan
|
// actually carried it, which is where it is true.
|
||||||
// with no explanation anywhere. `setMalformedListItems` is not called at
|
|
||||||
// all when empty, so a clean scan cannot clear what the aggregate wrote.
|
|
||||||
if (
|
|
||||||
malformed.length > 0 &&
|
|
||||||
this.malformedListItems.get("tools/list") === undefined
|
|
||||||
) {
|
|
||||||
this.setMalformedListItems("tools/list", malformed);
|
|
||||||
}
|
|
||||||
this.excludedTools = excluded;
|
this.excludedTools = excluded;
|
||||||
this.dispatchTypedEvent("excludedToolsChange", excluded);
|
this.dispatchTypedEvent("excludedToolsChange", excluded);
|
||||||
return excluded;
|
return excluded;
|
||||||
|
|
@ -3657,8 +3637,12 @@ export class InspectorClient extends InspectorClientEventTarget {
|
||||||
* which would blank the excluded set and leave a tool that vanished for an
|
* which would blank the excluded set and leave a tool that vanished for an
|
||||||
* invalid header with no explanation at all. That is the same failure #1909
|
* invalid header with no explanation at all. That is the same failure #1909
|
||||||
* is about, one level down. So on a decode rejection the page is re-fetched
|
* is about, one level down. So on a decode rejection the page is re-fetched
|
||||||
* leniently and scanned entry by entry; the malformed ones are not "excluded
|
* leniently and scanned entry by entry.
|
||||||
* tools" and are reported through the salvage channel instead.
|
*
|
||||||
|
* The malformed ones are not "excluded tools" and are not returned: they
|
||||||
|
* describe THIS exchange, not the aggregate the Tools panel renders, so they
|
||||||
|
* surface as the rejection reason on this response's own Protocol entry and
|
||||||
|
* go no further (see `refreshExcludedTools`).
|
||||||
*/
|
*/
|
||||||
private async listToolsForScan(
|
private async listToolsForScan(
|
||||||
cursor: string | undefined,
|
cursor: string | undefined,
|
||||||
|
|
@ -3666,11 +3650,9 @@ export class InspectorClient extends InspectorClientEventTarget {
|
||||||
): Promise<{
|
): Promise<{
|
||||||
tools: Tool[];
|
tools: Tool[];
|
||||||
nextCursor?: string;
|
nextCursor?: string;
|
||||||
malformed: MalformedListItem[];
|
|
||||||
}> {
|
}> {
|
||||||
try {
|
try {
|
||||||
const page = await this.listTools(cursor, metadata);
|
return await this.listTools(cursor, metadata);
|
||||||
return { ...page, malformed: [] };
|
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
if (!isSalvageableRejection(err)) throw err;
|
if (!isSalvageableRejection(err)) throw err;
|
||||||
// Capture before the re-fetch, for the reason `salvageList` documents:
|
// Capture before the re-fetch, for the reason `salvageList` documents:
|
||||||
|
|
@ -3750,7 +3732,6 @@ export class InspectorClient extends InspectorClientEventTarget {
|
||||||
return {
|
return {
|
||||||
tools: valid,
|
tools: valid,
|
||||||
...(nextCursor !== undefined && { nextCursor }),
|
...(nextCursor !== undefined && { nextCursor }),
|
||||||
malformed,
|
|
||||||
};
|
};
|
||||||
} catch (fallbackErr) {
|
} catch (fallbackErr) {
|
||||||
// The fallback could not explain the rejection — the re-fetch itself
|
// The fallback could not explain the rejection — the re-fetch itself
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue