Skip to content

Commit f86fba1

Browse files
committed
Fix argdump mutex group variant names and empty descriptors
Argdump's mutex code synthesized inner names from `dest` (underscored), but the path terminal already carried a name from `preferredName` (hyphenated long flag). The mismatch caused the solver's variant struct field key to diverge from the path binding's name, and the Boutiques backend emitted empty variants. Use the existing deep terminal name when available so the seq, path, and binding stay aligned. The solver also stripped literal alternatives unconditionally (`--fs-no-reconall` -> `fs-no-reconall`), ignoring any meta.name attached by the mutex code. Prefer alt.meta.name first, fall back to literal stripping or `variant_${i}`. This avoids value-key collisions when an argparse dest differs from the flag.
1 parent df32b10 commit f86fba1

4 files changed

Lines changed: 150 additions & 3 deletions

File tree

packages/core/src/backend/boutiques/boutiques.test.ts

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -645,6 +645,76 @@ describe("argdump -> Boutiques validity", () => {
645645
expect(variantNames).toContain("input_file");
646646
});
647647

648+
it("emits a populated descriptor for store-action variants in a mutex group", () => {
649+
// Regression: when the mutex code synthesized seq.meta.name from `dest`
650+
// (underscored), it diverged from the path terminal's name (hyphenated,
651+
// from preferredName), so findStructNode could not match the binding and
652+
// the variant emitted empty (just name+id, no command-line/inputs).
653+
const bt = emitFromArgdump({
654+
prog: "mytool",
655+
actions: [
656+
{
657+
option_strings: ["--input-file"],
658+
dest: "input_file",
659+
action_type: "store",
660+
type_info: { name: "Path", module: "pathlib" },
661+
},
662+
{
663+
option_strings: ["--no-input"],
664+
dest: "no_input",
665+
action_type: "store_true",
666+
},
667+
],
668+
mutually_exclusive_groups: [
669+
{ required: false, actions: ["input_file", "no_input"] },
670+
],
671+
});
672+
const inputs = bt.inputs as Record<string, unknown>[];
673+
const parent = inputs.find((i) => i.id === "input_file_or_no_input");
674+
expect(parent).toBeDefined();
675+
const variants = parent!.type as Record<string, unknown>[];
676+
const storeVariant = variants.find((v) => v.id === "input_file");
677+
expect(storeVariant).toBeDefined();
678+
expect(storeVariant!["command-line"]).toBe("--input-file [INPUT_FILE]");
679+
const subInputs = storeVariant!.inputs as Record<string, unknown>[];
680+
expect(subInputs).toHaveLength(1);
681+
expect(subInputs[0]?.id).toBe("input_file");
682+
expect(subInputs[0]?.type).toBe("File");
683+
});
684+
685+
it("uses meta.name for literal alternatives (e.g. mutex store_false dest)", () => {
686+
// Regression: solver always stripped the literal value (`--fs-no-reconall`
687+
// -> `fs-no-reconall`) and ignored alt.meta.name set by the mutex code,
688+
// which caused value-key collisions when the dest differed from the flag.
689+
const bt = emitFromArgdump({
690+
prog: "petprep",
691+
actions: [
692+
{
693+
option_strings: ["--fs-subjects-dir"],
694+
dest: "fs_subjects_dir",
695+
action_type: "store",
696+
type_info: { name: "Path", module: "pathlib" },
697+
},
698+
{
699+
option_strings: ["--fs-no-reconall"],
700+
dest: "run_reconall",
701+
action_type: "store_false",
702+
},
703+
],
704+
mutually_exclusive_groups: [
705+
{ required: false, actions: ["fs_subjects_dir", "run_reconall"] },
706+
],
707+
});
708+
const inputs = bt.inputs as Record<string, unknown>[];
709+
const parent = inputs.find((i) => i.id === "fs_subjects_dir_or_run_reconall");
710+
expect(parent).toBeDefined();
711+
const variants = parent!.type as Record<string, unknown>[];
712+
// The literal variant should use the dest from meta.name, not the
713+
// stripped-flag fallback.
714+
const variantIds = variants.map((v) => v.id).sort();
715+
expect(variantIds).toEqual(["fs_subjects_dir", "run_reconall"]);
716+
});
717+
648718
it("sanitizes ids when source names contain illegal characters", () => {
649719
// Boutiques requires id ~ /^[0-9A-Za-z_]+$/. Argparse subparser command
650720
// names commonly contain hyphens (e.g. `do-thing`).

packages/core/src/frontend/argdump/parser.test.ts

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -935,6 +935,60 @@ describe("ArgdumpParser", () => {
935935
expect(nodes[0]?.meta?.name).toBe("output_format");
936936
});
937937

938+
it("inner store action keeps the deep terminal name (matches solver binding)", () => {
939+
// Regression: setting inner.meta.name to dest (underscored) instead of
940+
// the existing terminal name (hyphenated, from preferredName) caused the
941+
// variant struct field key to drift from the path binding's name, which
942+
// broke findStructNode in the Boutiques backend (variants emitted empty).
943+
const result = parse(
944+
minimalDescriptor({
945+
actions: [
946+
{
947+
dest: "fs_subjects_dir",
948+
action_type: "store",
949+
option_strings: ["--fs-subjects-dir"],
950+
type_info: { name: "Path", module: "pathlib" },
951+
},
952+
{
953+
dest: "run_reconall",
954+
action_type: "store_false",
955+
option_strings: ["--fs-no-reconall"],
956+
},
957+
],
958+
mutually_exclusive_groups: [
959+
{ required: false, actions: ["fs_subjects_dir", "run_reconall"] },
960+
],
961+
}),
962+
);
963+
const opt = actionNodes(result)[0] as Optional;
964+
const alt = opt.attrs.node as Alternative;
965+
// First member is the seq for --fs-subjects-dir; its name should match
966+
// the inner terminal's name ("fs-subjects-dir") rather than dest.
967+
const seqAlt = alt.attrs.alts[0] as Sequence;
968+
expect(seqAlt.kind).toBe("sequence");
969+
expect(seqAlt.meta?.name).toBe("fs-subjects-dir");
970+
const pathTerminal = seqAlt.attrs.nodes[1];
971+
expect(pathTerminal?.meta?.name).toBe("fs-subjects-dir");
972+
});
973+
974+
it("falls back to dest when no deep terminal name exists", () => {
975+
// Two store_true alts: the inner is a bare literal, so there is no
976+
// deeper name to inherit and we should still synthesize one from dest.
977+
const result = parse(
978+
minimalDescriptor({
979+
actions: [
980+
{ dest: "json", action_type: "store_true", option_strings: ["--json"] },
981+
{ dest: "xml", action_type: "store_true", option_strings: ["--xml"] },
982+
],
983+
mutually_exclusive_groups: [{ required: false, actions: ["json", "xml"] }],
984+
}),
985+
);
986+
const opt = actionNodes(result)[0] as Optional;
987+
const alt = opt.attrs.node as Alternative;
988+
expect(alt.attrs.alts[0]?.meta?.name).toBe("json");
989+
expect(alt.attrs.alts[1]?.meta?.name).toBe("xml");
990+
});
991+
938992
it("required group -> alt() without optional wrapper", () => {
939993
const result = parse(
940994
minimalDescriptor({

packages/core/src/frontend/argdump/parser.ts

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,24 @@ import type {
1919
SourceLocation,
2020
} from "../frontend.js";
2121

22+
// Find the deepest existing name in a subtree, mirroring solver semantics.
23+
// Used by the mutex code so the synthesized inner name matches the binding
24+
// name the solver will produce for the same subtree.
25+
function findDeepName(node: Expr): string | undefined {
26+
if (node.meta?.name) return node.meta.name;
27+
if (node.kind === "optional" || node.kind === "repeat") {
28+
return findDeepName(node.attrs.node);
29+
}
30+
if (node.kind === "sequence") {
31+
for (const child of node.attrs.nodes) {
32+
if (child.kind === "literal") continue;
33+
const name = findDeepName(child);
34+
if (name) return name;
35+
}
36+
}
37+
return undefined;
38+
}
39+
2240
// Type guards
2341

2442
function isObject(x: unknown): x is Record<string, unknown> {
@@ -744,7 +762,11 @@ export class ArgdumpParser implements Frontend {
744762
inner.meta = {
745763
...outerMeta,
746764
...inner.meta,
747-
name: inner.meta?.name ?? dest,
765+
// Prefer the deepest existing name in the subtree so the synthesized
766+
// name matches the binding the solver derives for the same node.
767+
// Otherwise findDeepName short-circuits on the inner's new name and
768+
// the variant struct's field key drifts from the binding name.
769+
name: inner.meta?.name ?? findDeepName(inner) ?? dest,
748770
};
749771
altMembers.push(inner);
750772
excluded.add(dest);

packages/core/src/solver/solver.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -144,9 +144,10 @@ export function solve(expr: Expr, options?: SolveOptions): SolveResult {
144144
(alt.kind === "literal" ? literalFromNode(alt) : { kind: "bool" as const });
145145

146146
const name =
147-
alt.kind === "literal"
147+
alt.meta?.name ??
148+
(alt.kind === "literal"
148149
? alt.attrs.str.replace(/^-+/, "")
149-
: (alt.meta?.name ?? `variant_${i}`);
150+
: `variant_${i}`);
150151

151152
return { name, type: childType, node: alt };
152153
});

0 commit comments

Comments
 (0)