mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
Ignacy Łątka
9ce76824f6
## Defect
`VariantProposalStore`
(packages/tool-server/src/utils/variant-proposals.ts) could leave a
round in the `completed && !consumed` state, and a variant proposed in
that state was silently dropped from the next selection outcome.
`submitSelection()` sets `completed = true, consumed = false`, and only
sets `consumed = true` when there is a parked waiter to settle
(`toSettle.length > 0`). When the preview window POSTs a selection while
no `await_user_selection` is currently parked (the window lingers open
after a submit, so the user can complete a selection during the next
proposing phase, or after a pending-timeout before the agent re-awaits),
the round stays `completed && !consumed`.
The next `proposeVariant()` for a NEW element called
`autoRollIfConsumed()`, which only reset the store when `completed &&
consumed`. So it did not roll: the new element was appended to the
already-completed round. The subsequent `awaitSelection()` then hit the
fast path `if (this.completed && !this.consumed)` and returned the
frozen `lastOutcome` captured at submit time — which predates the new
element. `proposeVariant` reported success (`totalElements` incremented,
"staged"), but the element never appeared in any outcome and was never
presented for selection.
### Repro (dropped element)
- `proposeVariant("Alpha")` -> `submitSelection(Alpha)` with no awaiter
parked -> `proposeVariant("Beta")` (returns `totalElements: 2`) ->
`await awaitSelection()` resolves a completed outcome whose
`selections`/`unselected` contain "Alpha" but NOT "Beta". Beta is
silently dropped.
## Fix
Roll a fresh round whenever a proposal lands on an already-`completed`
round, not only when `completed && consumed`. The helper (renamed
`autoRollIfCompleted`) now calls `reset()` on `this.completed`, so a
post-completed proposal begins a new round and the new element is staged
into it. The legitimate submit-then-await fast path (submit with no
waiter, then await with no new proposal) is unchanged — `completed`
stays true with no intervening propose, so the frozen outcome is still
handed back and the round consumed.
With the fix, the repro above starts round 2 with "Beta" as a live
proposal; the next await presents Beta (as a pending proposal) instead
of dropping it.
## Tests
- New regression test
`packages/tool-server/test/variant-store-propose-after-submit.test.ts` —
fails before the fix (Beta dropped from the completed outcome), passes
after (Beta presented in the fresh round).
- All existing variant-proposals tests pass, including the
submit-then-await fast path and the await-then-submit consumed path.
- Full tool-server suite: 1622 passed. `tsc --build` clean; eslint 0
problems on changed files.
61 lines
2.7 KiB
TypeScript
61 lines
2.7 KiB
TypeScript
import { describe, it, expect } from "vitest";
|
|
import { VariantProposalStore } from "../src/utils/variant-proposals";
|
|
|
|
// The unauthenticated submit route must not retain an unbounded number of
|
|
// selections nor an unbounded variantId in `this.submitted` — mirroring the
|
|
// annotation caps, "bound memory regardless of caller".
|
|
describe("VariantProposalStore — selection retention is bounded regardless of caller", () => {
|
|
it("dedups selections per element and caps the variantId", () => {
|
|
const s = new VariantProposalStore();
|
|
const a = s.proposeVariant({ element: "Alpha", variant: { name: "v1", summary: "s" } });
|
|
const sels: Array<{ elementId: string; variantId: string | null }> = [];
|
|
for (let i = 0; i < 50_000; i++)
|
|
sels.push({ elementId: a.elementId, variantId: "x".repeat(100) });
|
|
sels.push({ elementId: a.elementId, variantId: "y".repeat(5_000_000) });
|
|
s.submitSelection({ selections: sels });
|
|
|
|
const submitted = (
|
|
s as unknown as {
|
|
submitted: Array<{ elementId: string; variantId: string | null }>;
|
|
}
|
|
).submitted;
|
|
// Bounded to the number of distinct real proposals (1), not 50_001.
|
|
expect(submitted.length).toBe(1);
|
|
// The retained variantId is capped.
|
|
expect((submitted[0]!.variantId ?? "").length).toBeLessThanOrEqual(200);
|
|
});
|
|
|
|
it("is behavior-preserving: the first selection per element still wins", () => {
|
|
const s = new VariantProposalStore();
|
|
const a = s.proposeVariant({ element: "Alpha", variant: { name: "v1", summary: "s" } });
|
|
// First dup picks the real variant; later dups (skip / bogus) are dead weight.
|
|
s.submitSelection({
|
|
selections: [
|
|
{ elementId: a.elementId, variantId: a.variantId },
|
|
{ elementId: a.elementId, variantId: null },
|
|
{ elementId: a.elementId, variantId: "nonexistent" },
|
|
],
|
|
});
|
|
const out = s.getLastOutcome();
|
|
expect(out?.status).toBe("completed");
|
|
const alpha = out?.selections.find((x) => x.element === "Alpha");
|
|
expect(alpha?.chosenVariant?.id).toBe(a.variantId);
|
|
});
|
|
|
|
it("keeps a selection for every distinct element (does not drop later elements)", () => {
|
|
const s = new VariantProposalStore();
|
|
const a = s.proposeVariant({ element: "Alpha", variant: { name: "v1", summary: "s" } });
|
|
const b = s.proposeVariant({ element: "Beta", variant: { name: "v1", summary: "s" } });
|
|
s.submitSelection({
|
|
selections: [
|
|
{ elementId: a.elementId, variantId: a.variantId },
|
|
{ elementId: b.elementId, variantId: b.variantId },
|
|
],
|
|
});
|
|
const out = s.getLastOutcome();
|
|
const picked = out?.selections.map((x) => x.element) ?? [];
|
|
expect(picked).toContain("Alpha");
|
|
expect(picked).toContain("Beta");
|
|
});
|
|
});
|