From 6d5d71fab5e135c687f7aa2f797da19b4c2a1c1e Mon Sep 17 00:00:00 2001 From: Ysopwnable <161784452+N0zoM1z0@users.noreply.github.com> Date: Sun, 11 Oct 2026 16:16:21 +0800 Subject: [PATCH] fix(javascript): index repeated graph observations before final sorting Retain stable live nodes and observation arrays while avoiding quadratic collection rebuilds. Preserve duplicate handling and canonical output. Fixes #1748 Assisted-by: Suika (gensokyo-skills:suika-scatter-gather-planning) --- ...JavaScriptArtifactGraphAccumulator.test.ts | 97 +++++++++++++++++++ .../JavaScriptArtifactGraphAccumulator.ts | 91 ++++++++++++----- 2 files changed, 162 insertions(+), 26 deletions(-) create mode 100644 src/application/javascript/JavaScriptArtifactGraphAccumulator.test.ts diff --git a/src/application/javascript/JavaScriptArtifactGraphAccumulator.test.ts b/src/application/javascript/JavaScriptArtifactGraphAccumulator.test.ts new file mode 100644 index 0000000000..47a8945f67 --- /dev/null +++ b/src/application/javascript/JavaScriptArtifactGraphAccumulator.test.ts @@ -0,0 +1,97 @@ +import { expect, it } from "vitest"; +import { completeApplicationCoverage } from "../../domain/javascript/javascriptApplicationEvidenceSchemas.js"; +import { createJavaScriptApplicationNode } from "../../domain/javascript/javascriptApplicationGraph.js"; +import { JavaScriptArtifactGraphAccumulator } from "./JavaScriptArtifactGraphAccumulator.js"; +import { artifactObservationEvidence } from "./JavaScriptArtifactGraphEvidence.js"; + +const nodeInput = (value: string, sha256 = "a".repeat(64)) => ({ + kind: "javascript-asset", + identity: { + strategy: "content-digest", + stability: "global-exact", + sha256, + }, + observations: [ + { + label: value, + properties: { value }, + evidence: artifactObservationEvidence({ + sha256, + path: "main.js", + operation: "accumulator-test", + coverage: completeApplicationCoverage(), + }), + }, + ], +}); + +it("retains live references, exact deduplication and canonical output across later additions", () => { + const accumulator = new JavaScriptArtifactGraphAccumulator(); + const inputs = ["雪😀", "alpha", "beta"].map((value) => nodeInput(value)); + const firstInput = inputs[0]; + if (firstInput === undefined) throw new Error("Missing first input"); + const node = accumulator.addNode(firstInput); + const observations = node.observations; + const firstObservation = observations[0]; + expect(accumulator.addNode(firstInput)).toBe(node); + expect(node.observations).toBe(observations); + expect(observations).toHaveLength(1); + expect(observations[0]).not.toBe(firstObservation); + for (const input of inputs.slice(1)) + expect(accumulator.addNode(input)).toBe(node); + const secondNode = accumulator.addNode(nodeInput("other", "b".repeat(64))); + const firstSnapshot = accumulator.nodes(); + const expectedNode = createJavaScriptApplicationNode({ + ...firstInput, + observations: inputs.flatMap((input) => input.observations), + }); + expect(firstSnapshot).toEqual([expectedNode, secondNode]); + expect(firstSnapshot[0]).toBe(node); + expect(node.observations).toBe(observations); + const finalizedObservations = [...observations]; + expect(accumulator.nodes()).not.toBe(firstSnapshot); + expect(accumulator.nodes()).toEqual(firstSnapshot); + for (const [index, observation] of finalizedObservations.entries()) + expect(node.observations[index]).toBe(observation); + + const later = nodeInput("later"); + expect(accumulator.addNode(later)).toBe(node); + // Updating a duplicate after final sorting must use its new array index. + expect(accumulator.addNode(firstInput)).toBe(node); + expect(accumulator.nodes()).toEqual([ + createJavaScriptApplicationNode({ + ...firstInput, + observations: [...inputs, later].flatMap((input) => input.observations), + }), + secondNode, + ]); + expect(node.observations).toBe(observations); +}); + +it("preserves first-input duplicates and retains the last occurrence when later merged", () => { + const accumulator = new JavaScriptArtifactGraphAccumulator(); + const first = nodeInput("same"); + const distinct = nodeInput("distinct"); + const node = accumulator.addNode({ + ...first, + observations: [...first.observations, ...first.observations], + }); + const observations = node.observations; + const lastInitial = observations[1]; + expect(accumulator.nodes()[0]?.observations).toHaveLength(2); + expect(accumulator.addNode(distinct)).toBe(node); + expect(accumulator.nodes()).toEqual([ + createJavaScriptApplicationNode({ + ...first, + observations: [...first.observations, ...distinct.observations], + }), + ]); + expect(node.observations).toBe(observations); + expect(observations).toContain(lastInitial); + // A repeated observation inside an incoming batch also collapses to one. + accumulator.addNode({ + ...distinct, + observations: [...distinct.observations, ...distinct.observations], + }); + expect(accumulator.nodes()[0]?.observations).toHaveLength(2); +}); diff --git a/src/application/javascript/JavaScriptArtifactGraphAccumulator.ts b/src/application/javascript/JavaScriptArtifactGraphAccumulator.ts index c492f05c6c..25378a9f15 100644 --- a/src/application/javascript/JavaScriptArtifactGraphAccumulator.ts +++ b/src/application/javascript/JavaScriptArtifactGraphAccumulator.ts @@ -6,40 +6,62 @@ import type { ApplicationEdge, ApplicationNode, } from "../../domain/javascript/javascriptApplicationGraphSchemas.js"; +import { compareUnicodeCodePoints } from "../../domain/unicodeCodePointOrder.js"; -/** Deduplicate graph entities while retaining distinct evidence observations. */ +/** + * Own live construction nodes and deduplicate their evidence observations. + * Consumers retain references but must not mutate the owned observations; + * nodes() finalizes their canonical order before graph validation/output. + */ export class JavaScriptArtifactGraphAccumulator { - readonly #nodes = new Map(); + readonly #nodes = new Map(); readonly #edges = new Map(); - /** Create or merge one canonical node, retaining a stable live construction reference. */ + /** Merge observations into a stable live node; order is finalized by nodes(). */ addNode(input: Input): ApplicationNode { const created = createJavaScriptApplicationNode(displayableNode(input)); const existing = this.#nodes.get(created.node_id); if (existing === undefined) { - this.#nodes.set(created.node_id, created); + this.#nodes.set(created.node_id, { + node: created, + observationIndices: new Map( + created.observations.map((observation, index) => [ + observation.observation_id, + index, + ]), + ), + dirty: false, + }); return created; } - const bySemanticId = new Map( - [...existing.observations, ...created.observations].map((observation) => [ - observation.observation_id, - observation, - ]), - ); - const observations = [...bySemanticId.values()].sort((left, right) => - left.observation_id < right.observation_id - ? -1 - : left.observation_id > right.observation_id - ? 1 - : 0, - ); - // Both factories already validated and identified these observations. - // Replacing the node retained every previous observation array through - // path/chunk/IPC indexes when multiple paths shared an entity identity. - existing.observations.length = 0; - for (const observation of observations) - existing.observations.push(observation); - return existing; + const observations = existing.node.observations; + // The first factory input can contain duplicate IDs. As before, a later + // merge retains only their last occurrences without replacing the array. + if (existing.observationIndices.size !== observations.length) { + let retained = 0; + for (const [index, observation] of observations.entries()) { + if ( + existing.observationIndices.get(observation.observation_id) !== index + ) + continue; + observations[retained] = observation; + existing.observationIndices.set(observation.observation_id, retained); + retained += 1; + } + observations.length = retained; + } + for (const observation of created.observations) { + const index = existing.observationIndices.get(observation.observation_id); + if (index === undefined) { + existing.observationIndices.set( + observation.observation_id, + observations.length, + ); + observations.push(observation); + existing.dirty = true; + } else observations[index] = observation; + } + return existing.node; } /** Create or replace one canonical edge by semantic identifier. */ @@ -49,9 +71,20 @@ export class JavaScriptArtifactGraphAccumulator { return edge; } - /** Return all accumulated canonical nodes. */ + /** Finalize observation order in place and return all canonical nodes. */ nodes(): readonly ApplicationNode[] { - return [...this.#nodes.values()]; + return [...this.#nodes.values()].map((entry) => { + if (entry.dirty) { + entry.node.observations.sort((left, right) => + compareUnicodeCodePoints(left.observation_id, right.observation_id), + ); + entry.observationIndices.clear(); + for (const [index, observation] of entry.node.observations.entries()) + entry.observationIndices.set(observation.observation_id, index); + entry.dirty = false; + } + return entry.node; + }); } /** Return all accumulated canonical edges. */ @@ -60,6 +93,12 @@ export class JavaScriptArtifactGraphAccumulator { } } +interface AccumulatedNode { + readonly node: ApplicationNode; + readonly observationIndices: Map; + dirty: boolean; +} + /** Node fields whose display form is normalized before validation. */ interface DisplayableNodeInput { readonly observations: readonly { readonly label: string | null }[];