fix: print server advisories on --dry-run (#1124731)

CONTRACT.md

@@ -124,6 +124,21 @@ The API base is `.../api/v1`. Shapes the CLI parses:
   issues can hit it. The CLI renders one scrubbed `note:` line per
   entry naming the code, the story count and the floor year.
 
+  A `dry_run` preview reports the same advisories as the committing import of
+  the same payload, except `cross_reference_links_failed` (agile-tracker
+  `import-mapping.md` §2.4.3, and §3 "Dry-run preview"). A preview never runs
+  the post-commit PR↔issue link attach that emits that code: in agile-tracker
+  `handlers/import.rs`, only a committing GitHub import takes
+  `execute_github_streamed_import`, and that function holds the attach. `--dry-run`
+  renders the preview's `warnings` through the same `note:` renderer: one
+  indented `note:` line per entry inside the plan block, after the `would` lines
+  and before the closing `No changes made.`. For a preview that the server
+  confirmed (`dry_run: true` in the result), no write took place, so an advisory
+  above that close cannot read as a report of a write. The direct engine's local dry
+  run renders its plan's `warnings` through the same block; that engine reports
+  none today. The local-preview fallback for an older server has no server
+  result, so it prints no `note:` line.
+
   **`unmatched` is always empty for a GitHub import.** The lists are built from
   the *email/name actor cells* a source can carry — `owner_emails`,
   `follower_emails`, `reviewers[].email`, `requester_email`,

src/cli.js

@@ -246,6 +246,7 @@ function reportDryRunPlan(plan, { stdout, stderr, owner, repo, project, projectT
       (created.length
         ? `  would create ${created.length} placeholder owner(s): ${placeholderOwnersTail(created)}`
         : "") +
+      (plan.warnings ?? []).map((w) => `  note: ${warningLine(w)}\n`).join("") +
       "No changes made.\n",
   );
   writeRowErrors(stderr, plan.errors);

tests/cli.test.js

@@ -442,6 +442,90 @@ test("--engine direct with --dry-run renders the same plan block as the server p
   );
 });
 
+/** An older server sends no `warnings` key, and spreading `warnings: undefined` would keep it.
+ * @param {Partial<import("../src/importer.js").ImportOutcome>} plan */
+async function directDryRun(plan) {
+  const out = capture();
+  await inTempDir(() =>
+    withEnv({ EAT_AGENT_KEY: "key" }, async () => {
+      const code = await main(
+        [
+          "--token",
+          "ghp_test",
+          "--project",
+          "91",
+          "--repo",
+          "o/r",
+          "--engine",
+          "direct",
+          "--dry-run",
+        ],
+        {
+          stdout: out,
+          stderr: capture(),
+          preflight: async () => preflightResult(),
+          runDirect: async () => {
+            /** @type {any} */
+            const result = { ...outcome({ dryRun: true }), ...plan };
+            if (plan.warnings === undefined) delete result.warnings;
+            return result;
+          },
+        },
+      );
+      assert.equal(code, 0);
+    }),
+  );
+  return out.buf;
+}
+
+test("a direct dry run reports each warning after the placeholder line, before the close", async () => {
+  const out = await directDryRun({
+    importedStories: 4,
+    externalMembersCreated: ["alice"],
+    warnings: [
+      { code: "dependencies_fetch_failed", count: 1 },
+      { code: "dependencies_truncated", count: 2 },
+    ],
+  });
+  assert.ok(
+    out.includes(
+      "  would import 4 stories (0 labels), would skip 0, 0 error(s).\n" +
+        "  would create 1 placeholder owner(s): @alice — external members outside the " +
+        "project roster; auto-linked when the matching GitHub account signs in.\n" +
+        "  note: dependencies_fetch_failed (1 story)\n" +
+        "  note: dependencies_truncated (2 stories)\n" +
+        "No changes made.\n",
+    ),
+    out,
+  );
+  assert.equal(out.trimEnd().split("\n").at(-1), "No changes made.", out);
+});
+
+test("a direct dry-run warning cannot write control characters to the terminal", async () => {
+  const out = await directDryRun({
+    warnings: [{ code: "history\u001b[2K\u202eclamped", count: 1, floor_year: 2024 }],
+  });
+  assert.ok(out.includes("  note: history[2Kclamped (1 story, floor year 2024)\n"), out);
+  assert.ok(!out.includes("\u001b"), JSON.stringify(out));
+  assert.ok(!out.includes("\u202e"), JSON.stringify(out));
+});
+
+for (const [label, warnings] of /** @type {[string, unknown[] | undefined][]} */ ([
+  ["an empty list", []],
+  ["no warnings key", undefined],
+])) {
+  test(`a direct dry run with ${label} prints no note`, async () => {
+    const out = await directDryRun({ importedStories: 4, warnings });
+    assert.ok(
+      out.includes(
+        "  would import 4 stories (0 labels), would skip 0, 0 error(s).\nNo changes made.\n",
+      ),
+      out,
+    );
+    assert.ok(!out.includes("note:"), out);
+  });
+}
+
 test("the direct engine prompts for confirmation like the server engine", async () => {
   await inTempDir(() =>
     withEnv({ EAT_AGENT_KEY: "key" }, async () => {

tests/direct.test.js

@@ -154,11 +154,14 @@ test("dry-run computes the plan locally and writes nothing", async () => {
     assert.equal(plan.skipped, 0);
     assert.equal(plan.dryRun, true);
     assert.deepEqual(plan.errors, []);
+    // CONTRACT says this engine reports no warnings; #386282 changes this pin and that sentence together.
+    assert.deepEqual(plan.warnings, []);
     assert.equal((mock.state.stories[91] ?? []).length, 0);
     assert.equal((mock.state.labels[91] ?? []).length, 0);
 
     // After a real import, a dry-run re-run reports everything as would-skip.
-    await runDirect(client, 91, "o", "r", options);
+    const committed = await runDirect(client, 91, "o", "r", options);
+    assert.deepEqual(committed.warnings, []);
     const rerun = await runDirect(client, 91, "o", "r", { ...options, dryRun: true });
     assert.equal(rerun.importedStories, 0);
     assert.equal(rerun.importedLabels, 0);

tests/import.test.js

@@ -404,6 +404,92 @@ test("a warning cannot write control characters to the terminal", async () => {
   assert.ok(!out.includes("\u001b"), JSON.stringify(out));
 });
 
+const DRY_RUN_ARGS = ["--project", "91", "--repo", "o/r", "--dry-run"];
+
+test("a server dry run reports each warning inside the plan block", async () => {
+  const { code, out } = await runCliWith(
+    {
+      dry_run: true,
+      imported: { stories: 4, labels: 0 },
+      skipped: 0,
+      errors: [],
+      warnings: [
+        { code: "history_floor_clamped", count: 3, floor_year: 2024 },
+        { code: "dependencies_truncated", count: 2 },
+      ],
+    },
+    DRY_RUN_ARGS,
+  );
+  assert.equal(code, 0);
+  assert.ok(
+    out.includes(
+      "Dry run plan for o/r into project 91 (Mock Project):\n" +
+        "  would import 4 stories (0 labels), would skip 0, 0 error(s).\n" +
+        "  note: history_floor_clamped (3 stories, floor year 2024)\n" +
+        "  note: dependencies_truncated (2 stories)\n" +
+        "No changes made.\n",
+    ),
+    out,
+  );
+  assert.equal(out.trimEnd().split("\n").at(-1), "No changes made.", out);
+});
+
+test("a server dry-run warning cannot write control characters to the terminal", async () => {
+  const { code, out } = await runCliWith(
+    {
+      dry_run: true,
+      imported: { stories: 1, labels: 0 },
+      skipped: 0,
+      errors: [],
+      warnings: [{ code: "history\u001b[2K\rclamped", count: 1, floor_year: 2024 }],
+    },
+    DRY_RUN_ARGS,
+  );
+  assert.equal(code, 0);
+  assert.ok(out.includes("  note: history[2Kclamped (1 story, floor year 2024)\n"), out);
+  assert.ok(!out.includes("\u001b"), JSON.stringify(out));
+  assert.ok(!out.includes("\r"), JSON.stringify(out));
+});
+
+test("a dry-run note renders each warning exactly as the committing run's note", async () => {
+  const result = {
+    imported: { stories: 4, labels: 0 },
+    skipped: 0,
+    errors: [],
+    warnings: [
+      { code: "dependencies_truncated", count: 0 },
+      { code: "history_floor_clamped", floor_year: 2024 },
+      "bare_string_advisory",
+      { code: "history_floor_clamped", count: 3, floor_year: 2024 },
+    ],
+  };
+  /** @param {string} out */
+  const notes = (out) =>
+    out
+      .split("\n")
+      .filter((line) => /^ {0,2}note: /.test(line))
+      .map((line) => line.replace(/^ {2}/, ""));
+  const committed = notes((await runCliWith(result)).out);
+  const previewed = notes((await runCliWith({ ...result, dry_run: true }, DRY_RUN_ARGS)).out);
+  assert.equal(committed.length, 4, committed.join("\n"));
+  assert.deepEqual(previewed, committed);
+});
+
+test("a server dry run without warnings prints no note", async () => {
+  const { code, out } = await runCliWith(
+    { dry_run: true, imported: { stories: 4, labels: 0 }, skipped: 0, errors: [] },
+    DRY_RUN_ARGS,
+  );
+  assert.equal(code, 0);
+  assert.ok(
+    out.includes(
+      "  would import 4 stories (0 labels), would skip 0, 0 error(s).\nNo changes made.\n",
+    ),
+    out,
+  );
+  assert.ok(!out.includes("note:"), out);
+});
+
 test("a non-array unmatched member does not break the actor note", async () => {
   const { code, out } = await runCliWith({
     imported: { stories: 1, labels: 0 },
@@ -907,6 +993,7 @@ test("--dry-run falls back to the local preview on older servers", async () => {
     await mock.close();
   }
   assert.ok(out.buf.includes("Dry run: would import o/r into project 91"));
+  assert.ok(!out.buf.includes("note:"), out.buf);
   assert.equal(mock.state.imports.length, 0); // no import call at all
 });