Skip to content

Commit 00b78cb

Browse files
committed
ci(observability-map): write scan reports with --out instead of capturing stdout
Both scan steps redirected pnpm --filter ... exec stdout into files the renderer JSON.parses. pnpm takes its recursive path under --filter and some versions announce 'Scope: N of M workspace projects' on it; one such line in head.json fails the parse and degrades every run to the stale-report comment, which is a permanent quiet failure rather than a loud one. It does not reproduce on the 10.33.2 the workflow pins, which was checked, so this closes the class rather than a reproduction: the scanner writes its own file and stdout is left to be log output. The -s guard keeps the partial dance honest now the redirect no longer creates the file, so a scanner that exits 0 without writing takes the stale-report branch instead of failing the mv and turning the job red. The render step still captures stdout, since prCommentCli has no --out and a banner there puts a stray line in a markdown comment rather than breaking a parse. Reported by Devin on #4455.
1 parent 5ad6026 commit 00b78cb

2 files changed

Lines changed: 53 additions & 4 deletions

File tree

.github/workflows/observability-map.yml

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -149,10 +149,24 @@ jobs:
149149
# Guarded rather than allowed to fail: this job must never block a pull request. The failure
150150
# is not swallowed either, the render step below turns a missing head report into a comment
151151
# saying so, because a swallowed failure with no comment is the outcome nobody wants.
152+
#
153+
# `--out` rather than a stdout redirect, so nothing a tool decides to print can end up inside
154+
# the document `prCommentCli` parses. `pnpm --filter` takes its recursive path and some
155+
# versions announce `Scope: N of M workspace projects` on the way; that line landing in
156+
# head.json would fail the parse and degrade every run to the stale-report comment, which is
157+
# a permanent quiet failure rather than a loud one. It does not reproduce on the 10.33.2
158+
# pinned above, so this closes the class rather than a reproduction: the file is written by
159+
# the process that owns it and stdout is left to be log output. Held by
160+
# `it("let the scanner write its own report rather than capturing stdout")` in
161+
# `internal-packages/observability-map/src/integration.test.ts`.
162+
#
163+
# `-s` keeps the partial dance honest now the redirect no longer creates the file: a scanner
164+
# that exits 0 without writing takes the else branch and the stale-report comment, instead of
165+
# failing the `mv` and turning the job red.
152166
- name: 🔎 Scan head
153167
run: |
154-
if pnpm --filter @internal/observability-map exec tsx src/cli.ts --json --no-write \
155-
> /tmp/head.json.partial; then
168+
if pnpm --filter @internal/observability-map exec tsx src/cli.ts \
169+
--out=/tmp/head.json.partial && [ -s /tmp/head.json.partial ]; then
156170
mv /tmp/head.json.partial /tmp/head.json
157171
else
158172
rm -f /tmp/head.json /tmp/head.json.partial
@@ -168,8 +182,9 @@ jobs:
168182
- name: 🔎 Scan base with the head's scanner
169183
run: |
170184
if git worktree add /tmp/base-tree ${{ github.event.pull_request.base.sha }} \
171-
&& pnpm --filter @internal/observability-map exec tsx src/cli.ts --json --no-write \
172-
--routes=/tmp/base-tree/apps/webapp/app/routes > /tmp/base.json; then
185+
&& pnpm --filter @internal/observability-map exec tsx src/cli.ts \
186+
--routes=/tmp/base-tree/apps/webapp/app/routes --out=/tmp/base.json \
187+
&& [ -s /tmp/base.json ]; then
173188
:
174189
else
175190
echo "-" > /tmp/base.json || true

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

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,40 @@ describe("the report workflow's two readers of the comment lookup", () => {
117117
});
118118
});
119119

120+
/**
121+
* Both scan steps used to capture the scanner's stdout with a shell redirect, into files the
122+
* renderer then `JSON.parse`s. Anything else reaching stdout therefore corrupted the report:
123+
* `pnpm --filter` takes its recursive path, and some versions of pnpm announce
124+
* `Scope: N of M workspace projects` on it. A single line of that in head.json fails the parse,
125+
* and the workflow degrades to the stale-report comment on every run, quietly and permanently.
126+
*
127+
* It does not reproduce on the 10.33.2 the workflow pins, which was checked. What is asserted here
128+
* is the shape that cannot have the bug at all rather than the version that happens not to: the
129+
* scanner writes its own file through `--out`, so stdout carries log output and nothing else.
130+
* The same is not yet true of the render step, which has no `--out` to reach for; a banner there
131+
* puts a stray line in a markdown comment instead of breaking a parse, so it is left alone.
132+
*/
133+
describe("the report workflow's two scan steps", () => {
134+
const WORKFLOW = resolve(__dirname, "../../../.github/workflows/observability-map.yml");
135+
136+
it("let the scanner write its own report rather than capturing stdout", () => {
137+
const scans = readFileSync(WORKFLOW, "utf8")
138+
.split(/^ {6}- name: /m)
139+
.slice(1)
140+
.filter((step) => step.startsWith("🔎 Scan"));
141+
expect(scans).toHaveLength(2);
142+
143+
for (const step of scans) {
144+
// The `if ...; then` condition only, which is where the scanner runs. The else branch writes
145+
// `echo "-" > /tmp/base.json`, a redirect of the workflow's own making that has nothing to
146+
// do with capturing the scanner, and an earlier version of this test failed on it.
147+
const command = step.split("; then")[0]!;
148+
expect(command).toMatch(/--out=\S+\.json/);
149+
expect(command).not.toMatch(/>\s*\S*\.json/);
150+
}
151+
});
152+
});
153+
120154
/**
121155
* The gating half of the same problem. A test job that nothing waits for is decoration, and the
122156
* first attempt at this was exactly that: a job inside `observability-map.yml`, which reads well

0 commit comments

Comments
 (0)