test: mutate the consent and confinement decisions too

packages/harness/src/policy.ts

@@ -3,13 +3,13 @@ import { createRequire } from "node:module";
 import type { SandboxPolicy } from "@microsoft/mxc-sdk";
 import { z } from "zod";
 
-import { absolutePath } from "./schema.js";
+import { absolutePath, positiveIntegerSetting } from "./schema.js";
 
 const policySettings = z.object({
 	workspace: absolutePath,
 	readonlyPaths: z.array(absolutePath).optional(),
 	allowedHosts: z.array(z.string().trim()).optional(),
-	timeoutMs: z.number().int().positive("timeoutMs must be a positive integer").optional(),
+	timeoutMs: positiveIntegerSetting("timeoutMs must be a positive integer").optional(),
 	version: z.string().optional(),
 });
 

packages/harness/src/schema.ts

@@ -14,7 +14,20 @@ export type MessageKind = z.output<typeof messageKind>;
 export const messageId = z.string().trim().min(1, "agent message id must be non-empty").brand<"MessageId">();
 export type MessageId = z.output<typeof messageId>;
 
-export const sequenceNumber = z.number().int().positive("sequence must be a positive integer").brand<"SequenceNumber">();
+/** A positive-integer setting whose message holds however the value is wrong.
+ * `z.number().int().positive(message)` attaches `message` to the positivity
+ * check alone, so a fractional or non-numeric value failed with zod's generic
+ * "Invalid input: expected int, received number" -- which never says what the
+ * setting actually needs. Every constraint carries the same message instead.
+ *
+ * Deliberately duplicated in packages/training/src/errors.ts rather than shared
+ * through a new package for three lines, as `attempt`/`errorMessage` are; keep
+ * the copies identical. */
+export function positiveIntegerSetting(message: string) {
+	return z.number({ error: message }).int(message).positive(message);
+}
+
+export const sequenceNumber = positiveIntegerSetting("sequence must be a positive integer").brand<"SequenceNumber">();
 export type SequenceNumber = z.output<typeof sequenceNumber>;
 
 export const agentMessage = z.object({
@@ -39,7 +52,7 @@ export const absolutePath = z.string()
 	.brand<"AbsolutePath">();
 export type AbsolutePath = z.output<typeof absolutePath>;
 
-export const roundLimit = z.number().int().positive("maxRounds must be a positive integer").brand<"RoundLimit">();
+export const roundLimit = positiveIntegerSetting("maxRounds must be a positive integer").brand<"RoundLimit">();
 export type RoundLimit = z.output<typeof roundLimit>;
 
 export const rubricText = z.string().trim().min(1, "judge rubric must be non-empty").brand<"Rubric">();

packages/harness/test/harness.test.ts

@@ -339,11 +339,18 @@ describe("training harness", () => {
 		it.each([
 			["a relative workspace", { workspace: "relative/path" }],
 			["a relative readonly path", { workspace: tmpdir(), readonlyPaths: ["relative/path"] }],
-			["a zero timeout", { workspace: tmpdir(), timeoutMs: 0 }],
-			["a fractional timeout", { workspace: tmpdir(), timeoutMs: 1.5 }],
 		])("refuses %s rather than building a policy around it", (_label, settings) => {
 			expect(() => createSandboxPolicy(settings as Parameters<typeof createSandboxPolicy>[0])).toThrow();
 		});
+
+		it.each([
+			["zero", 0],
+			["negative", -1],
+			["fractional", 1.5],
+		])("refuses a %s timeout, saying what a timeout must be", (_label, timeoutMs) => {
+			expect(() => createSandboxPolicy({ workspace: tmpdir(), timeoutMs }))
+				.toThrow(/timeoutMs must be a positive integer/);
+		});
 	});
 
 	it("refuses symlinked paths that resolve outside the workspace", async () => {

packages/rewrite/src/emit.ts

@@ -46,11 +46,21 @@ export function emitInstrumentation(targets: readonly InstrumentTarget[]): strin
 			[factory.createArrayLiteralExpression(targets.map(entryLiteral), true)],
 		),
 	);
-	const printer = ts.createPrinter({ newLine: ts.NewLineKind.LineFeed });
-	const container = ts.createSourceFile("instrumentation.js", "", ts.ScriptTarget.Latest, false, ts.ScriptKind.JS);
-	return printer.printNode(ts.EmitHint.Unspecified, statement, container);
+	return printer.printNode(ts.EmitHint.Unspecified, statement, printContainer);
 }
 
+// The printer and the source file it prints against. Every node here is
+// synthesized, so the container's name, text and parent linkage never reach the
+// output, and the explicit LineFeed only pins what TypeScript's default already
+// resolves to -- belt and braces, so instrumentation appended to an LF file
+// stays LF. None of it is observable from the emitted string, so it is excluded
+// from mutation rather than pinned by a test that could not tell the
+// difference. What the emitted text *is* has its own approved snapshot.
+// Stryker disable all
+const printer = ts.createPrinter({ newLine: ts.NewLineKind.LineFeed });
+const printContainer = ts.createSourceFile("instrumentation.js", "", ts.ScriptTarget.Latest, false, ts.ScriptKind.JS);
+// Stryker restore all
+
 function isInstrumentable(target: InstrumentTarget): boolean {
 	return isIdentifierName(target.methodName) && (target.className === undefined || isIdentifierName(target.className));
 }

packages/rewrite/test/instrument.test.ts

@@ -105,19 +105,52 @@ describe("instrumentation emission", () => {
 	});
 
 	it("registers working accessors when evaluated in the module's scope", () => {
+		// The `wrap` handler returns a *different* function on purpose. Returning
+		// the original made the assertions pass whether or not the emitted setter
+		// worked at all, and rebinding the module's own name is the entire reason
+		// the setter is emitted -- it is how a promoted candidate replaces a
+		// directive-marked free function.
 		const { handlers, methods, wrapped } = recordingInstrumentation();
-		installInstrumentation(handlers);
+		const replacement = (input: string) => input.toUpperCase();
+		installInstrumentation({
+			...handlers,
+			wrap: (fn, id) => { wrapped.push(id); return replacement as unknown as typeof fn; },
+		});
 		const evaluate = new Function(`class Router { route(input) { return input; } }
 function normalize(input) { return input; }
 ${emitInstrumentation(targets)}
 return { Router, normalize };`) as () => { Router: unknown; normalize: unknown };
 		const scope = evaluate();
 		expect(methods).toEqual([{ owner: scope.Router, methodName: "route", id: "Router.route" }]);
 		expect(wrapped).toEqual(["normalize"]);
+		expect(scope.normalize).toBe(replacement);
 	});
 
-	it("rejects names that are not plain identifiers", () => {
-		expect(() => emitInstrumentation([{ id: "bad", methodName: "not a name" }])).toThrow(TypeError);
+	it("emits the entries one per line, so a rewritten module stays readable", () => {
+		expect(emitInstrumentation(targets).split("\n")).toHaveLength(4);
+	});
+
+	it("appends nothing that would change the file's line endings", () => {
+		// This text is appended to the user's own module. Emitting CRLF into an
+		// LF file is diff noise the library has no business creating.
+		expect(emitInstrumentation(targets)).not.toContain("\r");
+	});
+
+	it("rejects names that are not plain identifiers, naming the offender", () => {
+		expect(() => emitInstrumentation([{ id: "bad", methodName: "not a name" }]))
+			.toThrow(/instrument target must be a plain identifier: not a name/);
+	});
+
+	it.each([
+		["a leading space", " normalize"],
+		["a trailing space", "normalize "],
+		["a reserved word", "class"],
+		["a member expression", "obj.normalize"],
+		["an empty name", ""],
+	])("rejects %s as a target name", (_label, methodName) => {
+		// Anything but a plain identifier would be emitted straight into the
+		// user's module and turn a valid file into a syntax error at load time.
+		expect(() => emitInstrumentation([{ id: "bad", methodName }])).toThrow(TypeError);
 	});
 });
 
@@ -150,6 +183,18 @@ describe("createRewriter", () => {
 		expect(createRewriter(() => [computed], "use audit")(marked, "/app/f.js")).toBe(marked);
 	});
 
+	it("skips a target whose class name cannot be referenced, keeping the method's own name valid", () => {
+		// Only the method name was ever checked here. A class name that does not
+		// scan as an identifier would be emitted as `owner: () => Not-A-Class`,
+		// which is a syntax error in the file the library just rewrote.
+		const marked = '"use audit"; class C { m() {} }';
+		const bad: InstrumentTarget = { id: "weird", methodName: "m", className: "Not-A-Class" };
+		expect(createRewriter(() => [bad], "use audit")(marked, "/app/c.js")).toBe(marked);
+
+		const good: InstrumentTarget = { id: "C.m", methodName: "m", className: "C" };
+		expect(createRewriter(() => [good], "use audit")(marked, "/app/c.js")).not.toBe(marked);
+	});
+
 	it("only accepts \"use <name>\" markers", () => {
 		// @ts-expect-error markers must be `use ${string}` directives
 		expect(() => createRewriter(() => [], "audit")).toThrow(TypeError);

packages/training/src/errors.ts

@@ -257,6 +257,19 @@ export class InvalidSettingsError extends TsAutocodeTypeError {
 	}
 }
 
+/** A positive-integer setting whose message holds however the value is wrong.
+ * `z.number().int().positive(message)` attaches `message` to the positivity
+ * check alone, so a fractional or non-numeric value failed with zod's generic
+ * "Invalid input: expected int, received number" -- which never says what the
+ * setting actually needs. Every constraint carries the same message instead.
+ *
+ * Deliberately duplicated in packages/harness/src/schema.ts rather than shared
+ * through a new package for three lines, as `attempt`/`errorMessage` are; keep
+ * the copies identical. */
+export function positiveIntegerSetting(message: string) {
+	return z.number({ error: message }).int(message).positive(message);
+}
+
 /** Parse with a Zod schema, surfacing failures as `InvalidSettingsError`
  * instead of leaking `ZodError` to consumers. The first issue's message is used
  * verbatim, so the schemas' hand-written messages still read as before. */

packages/training/src/loop.ts

@@ -1,7 +1,7 @@
 import { z } from "zod";
 
 import type { CandidatePatch } from "./engine.js";
-import { parseSetting } from "./errors.js";
+import { parseSetting, positiveIntegerSetting } from "./errors.js";
 import type { TrainableEvalRun } from "./evaluation.js";
 import type { PromotionDecision } from "./promotion.js";
 import type { TrainableId } from "./token.js";
@@ -87,8 +87,8 @@ export interface RoundSequence {
  * in order; within a round up to `fanOut` candidate pipelines run
  * concurrently. A round that reviews nothing new (every slot proposed an
  * already-seen candidate) completes the sequence as `"stalled"`. */
-const roundLimit = z.number().int().positive("maxRounds must be a positive integer");
-const fanOutWidth = z.number().int().positive("fanOut must be a positive integer");
+const roundLimit = positiveIntegerSetting("maxRounds must be a positive integer");
+const fanOutWidth = positiveIntegerSetting("fanOut must be a positive integer");
 
 export function trainingRounds(input: TrainingLoopInput): RoundSequence {
 	const maxRounds = parseSetting(roundLimit, input.maxRounds ?? defaultMaxRounds);

packages/training/src/training.ts

@@ -12,6 +12,7 @@ import {
 	ExecutorNotConfiguredError,
 	InsufficientTracesError,
 	parseSetting,
+	positiveIntegerSetting,
 	PromotionApplierNotConfiguredError,
 	PromotionRejectedError,
 	TraceNotFoundError,
@@ -52,7 +53,7 @@ import {
 
 const trainableAttribute = "ts_autocode.trainable.id";
 const tracerName = "ts-autocode";
-const traceMinimum = z.number().int().positive("minTraces must be a positive integer");
+const traceMinimum = positiveIntegerSetting("minTraces must be a positive integer");
 const executionTimeout = z.number().positive("execution.timeoutMs must be a positive number of milliseconds").finite("execution.timeoutMs must be a positive number of milliseconds");
 
 export interface CaptureSettings {

packages/training/test/errors.test.ts

@@ -15,6 +15,7 @@ import {
 	MissingSecretError,
 	OperationInterruptedError,
 	parseSetting,
+	positiveIntegerSetting,
 	PromotionApplierNotConfiguredError,
 	PromotionRejectedError,
 	SourceDiscoveryError,
@@ -180,3 +181,31 @@ describe("parseSetting", () => {
 		expect(parseSetting(z.string().transform((value) => value.length), "abcd")).toBe(4);
 	});
 });
+
+describe("positiveIntegerSetting", () => {
+	// `z.number().int().positive(message)` attaches the message to the
+	// positivity check alone, so every setting written that way reported zod's
+	// "Invalid input: expected int, received number" for a fractional value --
+	// a message that names neither the setting nor what it needs. Six settings
+	// across two packages were written that way. This is the shape that fixes
+	// them, so the message has to hold for every way the value can be wrong.
+	const schema = positiveIntegerSetting("minTraces must be a positive integer");
+
+	it("accepts a positive integer", () => {
+		expect(parseSetting(schema, 3)).toBe(3);
+	});
+
+	it.each([
+		["zero", 0],
+		["negative", -1],
+		["fractional", 2.5],
+		["a negative fraction", -0.5],
+		["Infinity", Number.POSITIVE_INFINITY],
+		["NaN", Number.NaN],
+		["a string", "many"],
+		["null", null],
+		["undefined", undefined],
+	])("reports the same message for %s", (_label, value) => {
+		expect(() => parseSetting(schema, value)).toThrow("minTraces must be a positive integer");
+	});
+});

packages/training/test/loop.test.ts

@@ -3,6 +3,7 @@ import { describe, expect, it } from "vitest";
 import {
 	createCandidateReview,
 	createPromotionDecision,
+	defineTrainable,
 	sequentialLoop,
 	trainingRounds,
 	type CandidatePatch,
@@ -116,6 +117,27 @@ describe("training loop fan-out", () => {
 	});
 });
 
+describe("round settings", () => {
+	// A fractional round or fan-out count used to fail with zod's generic
+	// "expected int, received number", naming neither the setting nor its rule.
+	it.each([
+		["maxRounds", { maxRounds: 1.5 }, "maxRounds must be a positive integer"],
+		["maxRounds at zero", { maxRounds: 0 }, "maxRounds must be a positive integer"],
+		["fanOut", { fanOut: 2.5 }, "fanOut must be a positive integer"],
+		["fanOut at zero", { fanOut: 0 }, "fanOut must be a positive integer"],
+	])("refuses a bad %s, naming the setting and its rule", async (_label, overrides, message) => {
+		await expect(sequentialLoop({
+			trainableId: defineTrainable("Router.route").id,
+			objective: "improve",
+			rubric: "must pass",
+			outputDir: "test/output/loop-settings",
+			propose: async () => { throw new Error("never proposed"); },
+			review: async () => { throw new Error("never reviewed"); },
+			...overrides,
+		})).rejects.toThrow(message);
+	});
+});
+
 describe("observable round sequence", () => {
 	it("emits each reviewed round in order before completing", async () => {
 		const emitted: Array<TrainingRound | string> = [];

stryker.config.json

@@ -24,20 +24,30 @@
     "packages/training/src/digest.ts",
     "packages/training/src/builders.ts",
     "packages/rewrite/src/apply.ts",
-    "packages/rewrite/src/canonical.ts"
+    "packages/rewrite/src/canonical.ts",
+    "packages/rewrite/src/emit.ts",
+    "packages/harness/src/policy.ts",
+    "src/evolve.ts"
   ],
   "thresholds": {
     "high": 100,
     "low": 100,
     "break": 100
   },
   "_comment": [
-    "Scoped to the modules where a surviving mutant is alarming rather than merely untidy:",
-    "the promotion gate decides whether generated code is written to a user's source file,",
-    "and the rewrite guard decides whether it lands on the body it was verified against.",
-    "A mutant that survives in either means a test asserts the happy path without pinning",
-    "the decision. Mutating everything would take hours and mostly re-measure line coverage,",
-    "which vitest already enforces.",
+    "Scoped to the modules where a surviving mutant is alarming rather than merely untidy.",
+    "Each one decides something the library does to a user's machine, not merely something",
+    "it computes: the promotion gate decides whether generated code is written to a source",
+    "file at all; the rewrite guard decides whether it lands on the body it was verified",
+    "against; evolve.ts is the kill switch deciding whether the library may rewrite that",
+    "source without being asked, and must fail closed on a value it does not recognize;",
+    "policy.ts is the sandbox's filesystem and network confinement, where the allowlist",
+    "failing open would be the worst outcome available; emit.ts appends generated code to",
+    "the user's own module, where a name that is not an identifier turns a valid file into",
+    "a syntax error at load time.",
+    "A mutant that survives in any of these means a test asserts the happy path without",
+    "pinning the decision. Mutating everything would take hours and mostly re-measure line",
+    "coverage, which vitest already enforces.",
     "The break threshold is a ratchet, like the coverage thresholds: raise it as suites",
     "improve, never lower it to get a build green. It reached 100 by killing the last",
     "survivors in the rewrite formatter; a genuinely equivalent mutant -- one no test",

test/tier1.test.ts

@@ -46,6 +46,26 @@ describe("evolution kill switch", () => {
 		expect(() => evolutionEnabled("nope")).toThrow(evolveVariable);
 		expect(() => evolutionEnabled("maybe")).toThrow(/must be one of/);
 	});
+
+	it("lists every value it would have accepted, and quotes the one it got", () => {
+		// Failing closed is only half of it: a user who typed the wrong thing has
+		// to be told what the right thing is, or the kill switch is a dead end.
+		// Asserting only /must be one of/ let the list itself go unchecked.
+		const message = (() => {
+			try {
+				evolutionEnabled("nope");
+			} catch (error) {
+				return (error as Error).message;
+			}
+			throw new Error("evolutionEnabled accepted an unrecognized value");
+		})();
+
+		for (const accepted of ["1", "true", "on", "yes", "enabled", "0", "false", "off", "no", "disabled"]) {
+			expect(message).toContain(accepted);
+		}
+		expect(message).toContain(", ");
+		expect(message).toContain('received "nope"');
+	});
 });
 
 describe("promotion thresholds", () => {

vitest.mutation.config.ts

@@ -31,6 +31,10 @@ export default defineConfig({
 			"packages/training/test/builders.test.ts",
 			"packages/rewrite/test/apply.test.ts",
 			"packages/rewrite/test/canonical.test.ts",
+			"packages/rewrite/test/instrument.test.ts",
+			"packages/harness/test/harness.test.ts",
+			"test/tier1.test.ts",
+			"test/register.test.ts",
 		],
 	},
 });