fix(observability-map): let the renderer write its own comment file

Author: 1stvamp

.github/workflows/observability-map.yml

@@ -300,15 +300,21 @@ jobs:
           COMPARE_URL: ${{ github.server_url }}/${{ github.repository }}/compare/${{ github.event.pull_request.base.sha }}...${{ github.event.pull_request.head.sha }}
         run: |
           rm -f /tmp/comment.md
+          # `--out` rather than a stdout redirect, for the reason the scan steps above give, and with a
+          # worse failure mode than theirs: the marker has to be the comment's first line for the
+          # lookup to find it, so a line printed ahead of the document makes every push post a new
+          # comment instead of updating the one already there. Held by
+          # `it("let the renderer write its own comment rather than capturing stdout")`.
           render() {
             pnpm --filter @internal/observability-map exec tsx src/report/prCommentCli.ts \
-              --commit-sha="$HEAD_SHA" --commit-url="$COMPARE_URL" "$@"
+              --commit-sha="$HEAD_SHA" --commit-url="$COMPARE_URL" --out=/tmp/comment.md.partial "$@"
           }
 
           # Every write goes through this, so a renderer that exits non-zero never leaves a 0-byte
           # comment.md for the upsert to skip in silence.
           emit() {
-            if render "$@" > /tmp/comment.md.partial; then
+            rm -f /tmp/comment.md.partial
+            if render "$@"; then
               mv /tmp/comment.md.partial /tmp/comment.md
               return 0
             fi

internal-packages/observability-map/INTERNALS.md

@@ -489,6 +489,12 @@ because
 `JSON.parse` and degrades the workflow to the stale-report comment permanently. What is asserted is
 the shape that cannot have the bug rather than the pinned 10.33.2 that happens not to.
 
+The render step writes through `--out` for the same reason, and its failure mode is the worse of the
+two. `renderPrComment` puts the marker on the first line and the lookup finds the comment with
+`startswith` on it, so a line printed ahead of the document does not degrade the comment, it hides it:
+the next push finds no id and posts a second comment, and no later run can reconcile either. The scan
+steps degrade to a stale report, which at least stays one comment.
+
 The corpus runs on the package's own paths and on a schedule rather than on every route pull request,
 because it measures the tool's resistance to laundering, which only an edit to the tool can weaken,
 and it costs four and a half minutes. The nightly covers tree drift late rather than not at all.

internal-packages/observability-map/src/integration.test.ts

@@ -177,9 +177,18 @@ describe("the report workflow reconciles a comment the paths no longer reach", (
 
 /**
  * Asserts the shape that cannot have the stdout-capture bug rather than the pnpm version that happens
- * not to. Why, and why the render step is left alone: INTERNALS.md, "Tests, timeouts and CI".
+ * not to. Why: INTERNALS.md, "Tests, timeouts and CI".
  */
-describe("the report workflow's two scan steps", () => {
+describe("the report workflow's scan and render steps", () => {
+  it("let the renderer write its own comment rather than capturing stdout", () => {
+    const render = steps(read(REPORT)).find((step) => step.startsWith("📝 Render"))!;
+    expect(render).toBeDefined();
+    expect(render).toMatch(/--out=\S+\.md\S*/);
+    // The marker has to be the comment's first line for the lookup to find it, so a redirect that
+    // could put a package-manager banner ahead of the document costs the upsert, not just tidiness.
+    expect(render).not.toMatch(/render[^\n]*>\s*\S*\.md/);
+  });
+
   it("let the scanner write its own report rather than capturing stdout", () => {
     const scans = steps(read(REPORT)).filter((step) => step.startsWith("🔎 Scan"));
     expect(scans).toHaveLength(2);

internal-packages/observability-map/src/report/prCommentCli.test.ts

@@ -1,4 +1,4 @@
-import { mkdtempSync, rmSync, writeFileSync } from "node:fs";
+import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
 import { main, type Io } from "./prCommentCli.js";
@@ -197,6 +197,43 @@ describe("prCommentCli", () => {
     expect(r.err).not.toContain(" at ");
   });
 
+  describe("--out", () => {
+    it("writes the comment to the file and leaves stdout empty", () => {
+      const outPath = join(dir, "out-delta.md");
+      const r = run(headPath, basePath, `--out=${outPath}`);
+      expect(r.code).toBe(0);
+      expect(r.out).toBe("");
+
+      const written = readFileSync(outPath, "utf8");
+      expect(written.split("\n")[0]).toBe("<!-- observability-map-report -->");
+    });
+
+    it("writes an empty file when there is nothing to post, rather than no file", () => {
+      const outPath = join(dir, "out-nothing.md");
+      const r = run(unchangedPath, unchangedPath, `--out=${outPath}`);
+      expect(r.code).toBe(0);
+      expect(existsSync(outPath)).toBe(true);
+      expect(readFileSync(outPath, "utf8")).toBe("");
+    });
+
+    it("writes the resolved and scan-failed comments too", () => {
+      for (const mode of ["--resolved", "--scan-failed"]) {
+        const outPath = join(dir, `out${mode}.md`);
+        expect(run(mode, `--out=${outPath}`).code).toBe(0);
+        expect(readFileSync(outPath, "utf8").split("\n")[0]).toBe(
+          "<!-- observability-map-report -->"
+        );
+      }
+    });
+
+    it("truncates a file left by an earlier run", () => {
+      const outPath = join(dir, "out-stale.md");
+      writeFileSync(outPath, "stale content from a previous push\n");
+      expect(run(unchangedPath, unchangedPath, `--out=${outPath}`).code).toBe(0);
+      expect(readFileSync(outPath, "utf8")).toBe("");
+    });
+  });
+
   it("exits 1 with a one-line message when base.json is malformed", () => {
     const malformedBasePath = join(dir, "malformed-base.json");
     writeFileSync(malformedBasePath, "not json at all");

internal-packages/observability-map/src/report/prCommentCli.ts

@@ -1,4 +1,4 @@
-import { readFileSync } from "node:fs";
+import { readFileSync, writeFileSync } from "node:fs";
 import { resolve } from "node:path";
 import { fileURLToPath } from "node:url";
 import type { MapReport } from "../score.js";
@@ -72,6 +72,21 @@ export function main(argv: string[], io: Io = processIo): number {
   const headPath = positional[0];
   const basePath = positional[1];
 
+  /**
+   * The marker has to be the document's first line or the workflow's lookup cannot find the comment it
+   * left, and every later push posts a new one instead of updating it. `--out` keeps stdout free for
+   * whatever a tool decides to announce, the same reason `src/cli.ts` has it. An empty write is a real
+   * outcome and not a failure: it is how "post nothing" reaches the workflow's `-s` check.
+   */
+  const outPath = flag(args, "out");
+  const write = (text: string) => {
+    if (outPath === undefined) {
+      if (text) io.out(text);
+      return;
+    }
+    writeFileSync(outPath, text);
+  };
+
   let commit: CommitContext | undefined;
   try {
     commit = commitFrom(args);
@@ -81,12 +96,12 @@ export function main(argv: string[], io: Io = processIo): number {
   }
 
   if (scanFailed) {
-    io.out(`${renderScanFailedComment(commit)}\n`);
+    write(`${renderScanFailedComment(commit)}\n`);
     return 0;
   }
 
   if (resolved) {
-    io.out(`${renderResolvedComment(commit)}\n`);
+    write(`${renderResolvedComment(commit)}\n`);
     return 0;
   }
 
@@ -106,12 +121,10 @@ export function main(argv: string[], io: Io = processIo): number {
   }
 
   if (hasDelta(head, base)) {
-    io.out(`${renderPrComment(head, base, commit)}\n`);
+    write(`${renderPrComment(head, base, commit)}\n`);
     return 0;
   }
-  if (existingComment) {
-    io.out(`${renderResolvedComment(commit)}\n`);
-  }
+  write(existingComment ? `${renderResolvedComment(commit)}\n` : "");
   return 0;
 }