Skip to content

chore(mt#3835): Move bun to 1.4.2 and retire the mt#3623 install workaround at every site - #3721

Merged
edobry merged 4 commits into
mainfrom
task/mt-3835
Sep 11, 2026
Merged

edobry merged 4 commits into
mainfrom
task/mt-3835

Conversation

@minsky-ai

@minsky-ai minsky-ai Bot commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Accepting this = agreeing that the mt#3623 install workaround is retired now that a bun release carries the upstream fix, and that every bun pin in the repo moves to 1.4.2 in one change. Consequences: 22 workflow pins, 4 image bases, and the site's Nixpacks install move 1.3.14 → 1.4.2; the 3-attempt bun install retry and the BUN_FEATURE_FLAG_DISABLE_STREAMING_INSTALL hedge come out of all 23 install sites, so an install failure is visible again rather than absorbed; the two service Dockerfiles that rode the mutable 1.3-slim tag are digest-pinned (mt#3724, absorbed); release.yml stops floating on latest; two places where our code leaned on a Bun 1.3 quirk are corrected (below).

mt#3835 was waiting on oven-sh/bun#34827. That PR was closed unmerged on 2026-08-13 — the maintainer's closing comment names the real cause and fix: #34861, "the streaming extractor losing its format selection when the first HTTP chunk is smaller than the gzip header", merged 2026-07-21. The bun-v1.4.0 tag contains that merge commit (compare → status: behind, ahead_by: 0). 1.4.2 is the latest release.

Two Bun 1.4 behaviour changes this surfaced (R2)

The first CI run on 1.4.2 ran 18,337 tests and failed 6. Both root causes are 1.4 behaving the way Node always has, where our code had been relying on a 1.3 quirk. Neither is a 1.4 regression; both are worth knowing about for anything else that spawns processes.

  1. Bun 1.4 emits an async error on a Writable whose pipe closed; 1.3 dropped it. proxy.ts's spawnChild writes a readiness probe to child.stdin; if the child already exited, that is EPIPE, delivered as a stream error event (not a throw — the try/catch never sees it), and unhandled it is an uncaught exception. Production impact on 1.4: a child dying before the probe would have taken the proxy down instead of respawning it. Fix: an error listener on child.stdin that logs and yields to the existing close → classify → respawn path. The pem-utils "failure" in that run was this EPIPE firing asynchronously in the next test file's window — collateral, not crypto.
  2. Bun 1.4's spawnSync passes the live process.env to children; 1.3 passed a stale snapshot. Minimal repro, same file under both runtimes: mutate process.env.X then spawnSync with default env — child sees <absent> on 1.3.14, the value on 1.4.2. tests/setup.ts sets MINSKY_LOG_MODE=STRUCTURED for the in-process logger; on 1.4 that reaches the spawned CLI in security.check-credentials.cli.test.ts, which correctly obeys it (resolveDiagnosticSink ranks STRUCTURED above the one-shot discard) and prints JSON boot warnings under --quiet. The test had been passing on the bug. Fix: runCli strips the harness's log mode from the child env, since the contract under test is the default mode an operator's shell provides.

Key changes

  • bun-version: "1.3.14" → "1.4.2" in 20 workflows; release.yml latest → "1.4.2"; publish-npm.yml "1.2.21" (uncommented, authored mid-upgrade by mt#3616) → "1.4.2"; services/site/nixpacks.toml bun-v1.3.14 → bun-v1.4.2 (R1 — site-build-smoke.yml reads the version from that file, so it followed).
  • Hedge env var removed from 15 workflow env: blocks (11 of which held nothing else and are gone) and from 4 Dockerfile RUN lines.
  • 23 retry loops unwrapped to bare bun install …. The mt#4300 retries around apt-get/pulumi/curl/npm install -g are unrelated and untouched.
  • FROM oven/bun:1.4-slim@sha256:cb3bbbb0… on all four base lines (root, cockpit ×2, reviewer). Digest is the OCI index (amd64+arm64), confirmed by Docker Hub's tag API and docker buildx imagetools inspect. Root Dockerfile's mt#1726 rotation comment updated to name all four.
  • src/mcp/stdio-proxy/proxy.ts: child.stdin error listener (finding 1). security.check-credentials.cli.test.ts: child env hygiene (finding 2).

Testing

Execution evidence:

  • SC1 / AT1 (consecutive clean full-install CI runs on main after PR fix(mt#3623): Retry bun install and disable streaming extraction at every install site #2707, with the mitigation in place) — the last 12 ci.yml runs on main, all conclusion: success: 33353216275, 33351104077, 33350720846, 33350377573, 33349959581, 33349333374, 33348703503, 33347658213, 33346609542, 33333996902, 33333597977, 33332726820. None failed with Fail extracting tarball. Caveat stated rather than hidden: with the retry in place, a first-attempt failure was absorbed by design, so "none concluded failure" is what this window can show. SC2 (reopen mt#3623 if a run failed that way) therefore did not fire.
  • AT2 (upstream state + harness with the mitigation removed) — done pre-commit on a 1.4.2 binary installed to a scratch dir via bun's installer:
    $ bun-1.4.2 install --cwd <harness> --frozen-lockfile --ignore-scripts   # cold cache, no flag, no retry
    1497 packages installed [10.02s]
    lockfile byte-identical after install (no format change)
    $ 10 × cold-cache full install, fresh BUN_INSTALL_CACHE_DIR each, no flag, no retry:
    BUN 1.4.2 NO-MITIGATION RESULT: pass=10 fail=0
    extraction-error lines: 0
    
    Read per the spec's correction: necessary, not sufficient — the same harness returned 0/10 in both arms on 2026-08-05 when the network did not trigger the defect. The sufficient signal is this PR's own CI: on the first run, 21 install-running jobs passed on 1.4.2 with no retry (three image builds, the site under Nixpacks, four cold-start smokes, bundle-boot, npm-pack, placeholder-tests, credential-free + Postgres integration); the 2 failures were the test-suite jobs above, not installs.
  • SC3 (retirement condition — pin moved, both mechanisms removed from every site, one change): this PR. Site count at implementation time by grep: 23 retry loops, 15 workflow env: hedges, 4 Dockerfile inline hedges — all removed; grep -rl DISABLE_STREAMING_INSTALL and grep -rn 'mt#3623 tarball flake' over workflows + Dockerfiles → 0 hits.
  • SC4 (2026-09-07 deadline re-evaluation recorded): in the task spec, ## Re-evaluation (2026-09-10).
  • SC5 (absorbed mt#3724): grep -rn 'FROM oven/bun' Dockerfile services/*/Dockerfile → 4 lines, all @sha256:; grep -rn bun-version .github/workflows/ | grep -c latest → 0.
  • SC6 (enumerate by grep, not count): for i in 1 2 3 survivors after the sweep — ci.yml ×3, clock-shifted-tests.yml, deploy-minsky-mcp.yml — each read and confirmed as an mt#4300 network retry, not a bun install. A second sweep for bun-vX.Y.Z / bun.sh/install / setup-bun@ after R1 finds no further pin surface.
  • Reporter grammar unchanged on 1.4.2 — Ran 32 tests across 1 file. on both runtimes — so scripts/run-tests-gated.ts's fail-closed /Ran \d+ tests? across \d+ files?/ still matches (mt#2632 protection intact).
  • R2 fixes, on the 1.4.2 binary with the spawned child also on 1.4.2 (matching CI): proxy.test.ts + pem-utils.test.ts 17 pass / 0 fail, no EPIPE; security.check-credentials.cli.test.ts 9 pass / 0 fail. On 1.3.14: proxy 14/0, check-credentials 9/0.
  • All 23 workflow files parse (yaml package). bun.lock untouched.

Negative control — proxy EPIPE: tests/mcp/stdio-proxy/proxy.test.ts on the 1.4.2 binary before the child.stdin listener: 13 pass / 1 fail, with the EPIPE: broken pipe, write … internal:streams/writable trace; 14 / 0 after.

Negative control — --quiet leak: security.check-credentials.cli.test.ts on the 1.4.2 runner before the env fix: 3 fail, each expect(combined).toBe("") receiving the four {"level":"warn",…} lines; 9 / 0 after. The underlying runtime difference was isolated in a two-line probe: process.env.X = "yes"; spawnSync("sh", ["-c", "echo ${X:-<absent>}"]) prints <absent> on 1.3.14 and yes on 1.4.2.

Vendor 1.4 release post lists no breaking-changes section and neither change above is in it. Its four listed behaviour changes were checked against this repo: non-registry dependencies losing default trust (none exist — the git+https hits are repository metadata), --isolate fixes (unused), Bun.serve no longer serving HTML-route sourcemaps in production (cockpit serves a Vite build as static files), --no-env-file (opt-in).

Deploy verification:

This touches services/reviewer/Dockerfile, services/cockpit/Dockerfile, the root Dockerfile (minsky-mcp image), deploy-reviewer.yml, services/site/nixpacks.toml, and src/mcp/stdio-proxy/proxy.ts (bundled into the minsky-mcp image) — all three service images rebuild on a new base and the site rebuilds on a new runtime. After merge: deployment_wait-for-latest for reviewer, cockpit, and minsky-mcp with notBefore = merge time and expectCommitSha = merge sha; on indeterminate identity (image-source services), correlate the deploy-*.yml run's head_sha to the merge; assert each /health body's service identity, not the status code.

Not in this PR

  • effect removal (mt#3723) — separate hygiene; sequenced after this since both touch bun.lock if 1.4 ever bumps the format (it did not here).
  • PR fix(mt#3575): Make the test suite order-independent and turn randomization back on #3590 (mt#3575) adds seed-sweep-tests.yml carrying the old pin, hedge, and two retry loops. Whichever lands second re-runs the same greps; the task spec says so.
  • proc.stdout in the proxy (the upstream JSON-RPC channel) has no stream-level error listener either; on 1.4 an EPIPE there — the MCP client going away — would now surface the same way. Not touched here: that case should end the proxy, and deciding how is a separate change. Noted for a follow-up.

Had Claude re-derive the upstream state, run the 1.4.2 harness, make the change, and diagnose the two CI failures on the new runtime.

🤖 Generated with Claude Code

https://claude.ai/code/session_0147JJ3SVgqRz9JyUD4NZBz9

…around at every site

Bun 1.4.0 carries oven-sh/bun#34861 — the streaming tarball extractor losing
its format selection when the first HTTP chunk is smaller than the gzip
header, which is what produced `Fail extracting tarball for "effect"` on
1.3.14 (~30% of full installs; five recorded CI occurrences, one a failed
reviewer deploy). That is the retirement condition mt#3835 was waiting on,
verified structurally: the bun-v1.4.0 tag contains the #34861 merge commit
(`compare` status "behind", ahead_by 0). It was tracking #34827, which was
closed unmerged as a symptom-level retry.

Every pin moves to 1.4.2 and both halves of the workaround come out:

- 22 `bun-version` pins → "1.4.2", including `release.yml`'s floating
  `latest` and `publish-npm.yml`'s uncommented, stale `1.2.21`.
- `BUN_FEATURE_FLAG_DISABLE_STREAMING_INSTALL` removed from 15 workflow
  `env:` blocks and 4 Dockerfile `RUN` lines; 11 of those `env:` blocks held
  nothing else and are gone.
- 23 three-attempt retry loops unwrapped back to a bare `bun install`. The
  mt#4300 network retries around apt/pulumi/curl/npm are untouched.
- All four `FROM oven/bun` lines pinned to the 1.4-slim OCI index digest
  (Hub API and `buildx imagetools inspect` agree), absorbing mt#3724.

Verified before commit, on a 1.4.2 binary installed to a scratch dir:
`bun install --frozen-lockfile` against the committed lockfile succeeds with
the lockfile byte-identical (no format bump, so CI's frozen installs hold);
10 of 10 cold-cache full installs pass with no flag and no retry; `bun test`
still emits `Ran N tests across M file(s)`, so the gated runner's fail-closed
summary check is unaffected. Vendor 1.4 notes list no breaking changes; the
four behaviour changes named were checked against this repo and none reach
it. The clean 10 is necessary, not sufficient — the retry is out so any
recurrence in CI is visible rather than absorbed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0147JJ3SVgqRz9JyUD4NZBz9
@minsky-ai minsky-ai Bot added the authorship/co-authored Co-authored by human and AI agent label Sep 11, 2026
@minsky-reviewer

minsky-reviewer Bot commented Sep 11, 2026 •

Copy link
Copy Markdown

Minsky Reviewer Status

Verdict: APPROVED — no blocking findings
Review: View review
Model: openai/gpt-5 | Tokens: 409K prompt, 5K completion | Duration: 91s
Mode: normal

Commands

  • /review — request a fresh review

@github-actions

github-actions Bot commented Sep 11, 2026 •

Copy link
Copy Markdown

Cockpit Preview

Preview URL: https://cockpit-preview-production.up.railway.app

Deployed from commit fb420fc19.
Preview runs in read-only mode — mutations are disabled.

Updated by Cockpit Preview.

@minsky-reviewer minsky-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2


Requesting changes: this PR broadly upgrades workflows and Dockerfile bases to Bun 1.4.2 and removes the mt#3623 install workaround, but one service remains pinned to Bun 1.3.14. Specifically, services/site/nixpacks.toml still installs bun-v1.3.14, contradicting the PR’s promise that all pins move in one change and the task’s criterion to move to 1.4.x. Please update that pin to bun-v1.4.2 (and adjust comments) so the site build aligns with the rest of the repo. Aside from this inconsistency, the changes appear coherent; I focused on the changed workflows/Dockerfiles and verified the site’s independent pin. I did not re-sweep untouched files beyond this scope.

Findings

  • [BLOCKING] services/site/nixpacks.toml:79 — Site still pins Bun 1.3.14 — contradicts PR’s stated repo-wide 1.4.2 move and leaves one service on the old toolchain
    The PR description claims “every bun pin in the repo moves to 1.4.2 in one change,” and the task’s retirement criterion requires moving the pin to a 1.4.x release. However, services/site/nixpacks.toml still installs bun-v1.3.14 explicitly:

  • services/site/nixpacks.toml:79-86 — curl ... bash -s -- bun-v1.3.14 followed by /usr/local/bin/bun ...

This leaves the site build on Bun 1.3.14 while the rest of the repo is moved to 1.4.2 (workflows and Dockerfile bases). It violates the PR’s own “all pins move” guarantee, and risks reintroducing the 1.3.x install behavior divergence this change is meant to retire. Please update the nixpacks installer line to bun-v1.4.2 (and any adjacent comments) so the site builds on the same pinned runtime as the rest of the repo.

Documentation impact

  • blocking-needs-update — This PR changes repo-wide Bun runtime pins (workflows and Dockerfile bases) and retires the mt#3623 install workaround, which affects documented deployment and build behavior. I did not read the docs tree; therefore I cannot assert existing docs remain accurate. Given prior conventions (e.g., mt#1726 digest pinning and deploy docs) likely reference specific base tags or describe the presence of retry/flag hedges, those statements may now be false (e.g., any doc instructing to use oven/bun:1.3-slim, or mentioning the BUN_FEATURE_FLAG_DISABLE_STREAMING_INSTALL hedge or install retries). Update deployment and CI docs to reflect Bun 1.4.2 pins, digest pin usage across all four Dockerfiles, and the removal of retry/flag mitigations. Also update any release workflow notes that previously floated on 'latest'.
    Affected: docs/deploy-minsky-railway.md, docs/testing-patterns.md

The reviewer found the one pin surface the sweep had not covered:
services/site deploys via Nixpacks, not a Dockerfile, and installs bun with
`bun.sh/install -- bun-vX.Y.Z` in nixpacks.toml. Moved to bun-v1.4.2.
site-build-smoke.yml reads the version out of that file rather than
carrying its own, so it follows automatically; its comment and the
nixpacks comment that both hardcoded 1.3.14 now describe the mechanism
without a number. A repo-wide sweep for `bun-vX.Y.Z`, `bun.sh/install`,
and `setup-bun@` finds no other pin surface.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0147JJ3SVgqRz9JyUD4NZBz9

@minsky-reviewer minsky-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2


Verified the R1 BLOCKING finding: the site’s Nixpacks pin was the last missed surface and is now updated to bun-v1.4.2, with workflow comments adjusted to single-source the version. I reviewed the updated files and found no new defects introduced by this fix; the change is scoped and consistent with the task’s retirement plan. Some repo-wide assertions (digest-pins on all Dockerfiles, removal of 'latest' elsewhere) cannot be re-verified from this diff fragment, but no contradictions appear here. With the prior blocker addressed and no critical new issues detected, this iteration is APPROVE.

Findings

  • [NON-BLOCKING] services/site/nixpacks.toml:76 — Prior R1 blocking — site’s Nixpacks bun pin now updated to 1.4.2
    The earlier review blocked on services/site/nixpacks.toml still pinning Bun 1.3.14. This commit updates the install command to bun-v1.4.2 and adjusts adjacent comments to describe the mechanism generically. This addresses the prior BLOCKING finding. No further action needed.

Spec verification

Criterion Status Evidence
10 consecutive full-install CI runs on main after PR #2707 merges are recorded here with their run ids, and none failed with Fail extracting tarball. N/A This PR implements the retirement step (upgrade to Bun 1.4.2 and removal of workarounds). Recording of historical CI run ids lives in the task spec, not this diff; no code change can satisfy it. Treating as outside-the-diff bookkeeping for this PR.
If any run DOES fail that way with the mitigation in place, this task records it and mt#3623 is reopened rather than the retry count being raised — the validation was falsified, not under-tuned. N/A This is a process/recording criterion contingent on CI behavior on main prior to this PR. No artifact in this diff can satisfy or violate it.
The retirement condition is checked: when oven-sh/bun#34827 merges AND ships in a bun release this repo can pin, both mechanisms are removed from all 16 sites and the pin is moved, in one change. SUPERSEDED 2026-09-10 — see Re-evaluation (2026-09-10). #34827 was closed unmerged. The actual fix is #34861, merged 2026-07-21 and carried by bun 1.4.0+. The retirement condition is MET. The criterion is now: move the pin to a 1.4.x release, remove both mechanisms from every install site (23 as of 2026-09-10, not 16), in one change. Met This PR updates remaining pins to Bun 1.4.2 (e.g., services/site/nixpacks.toml:76 now installs bun-v1.4.2 and comments note single-source of truth). Earlier commits in this PR (not re-shown in this fragment) covered workflows and Dockerfiles; the R2 diff shows the last missed surface fixed. Prior PR description asserts repo-wide sweep; no conflicting residue found in the shown files.
If #34827 has not shipped in a pinnable release by 2026-09-07, that is recorded here with a re-evaluation rather than the workaround silently becoming permanent. Deadline passed; re-evaluation recorded below on 2026-09-10. N/A The re-evaluation text lives in the task spec. This PR makes the code/config changes based on that re-evaluation; the recording itself is not an artifact in the repo changed here.
Absorbed from mt#3724: .github/workflows/release.yml pins an explicit bun version rather than latest; services/reviewer/Dockerfile and services/cockpit/Dockerfile (both stages) pin oven/bun:1.4-slim by @sha256: digest, following the root Dockerfile's mt#1726 comment convention. grep -rn 'FROM oven/bun' Dockerfile services/*/Dockerfile shows a digest on every line; grep -rn bun-version .github/workflows/ shows no latest. Every pin is touched by the move anyway, so leaving these as a separate task would put a second PR through the same lines. Unverifiable This R2 diff shows only the site’s nixpacks and a workflow. The PR description claims digest-pinning across all Dockerfiles and removal of 'latest' from workflows. Those files are not present in this fragment; without reading the full repo state in this review, I cannot verify. Treat as Unverifiable from this diff alone.
Sites are enumerated by grep at implementation time, not from a count: PR #3590 (mt#3575, IN-REVIEW) adds .github/workflows/seed-sweep-tests.yml carrying the flag, a 1.3.14 pin, and two retry loops. Whichever merges first, the sweep covers what is on main at that moment. Met The newly added fix commit explicitly updates the last uncovered surface (services/site/nixpacks.toml). Paired with site-build-smoke.yml’s comment noting it reads the version from nixpacks.toml, the sweep appears comprehensive at least for the surfaces shown here.

Documentation impact

  • no-update-needed — This PR updates runtime pins and removes a temporary install workaround across CI and build configs. It does not change any user-facing CLI, API, or documented workflow semantics; comments were updated inline to avoid duplication (e.g., site-build-smoke.yml now reads the version from services/site/nixpacks.toml). No docs under docs/ appear to describe these specific pin values or the retired env var, so no documentation updates are required.

…hiding

The PR's own CI on 1.4.2 ran 18,337 tests and failed 6. Both root causes are
bun 1.4 behaving the way Node always has, in places where our code had been
leaning on a 1.3 quirk. Neither is a 1.4 regression.

1. `src/mcp/stdio-proxy/proxy.ts` — an unhandled EPIPE on `child.stdin`.
   `spawnChild` writes a readiness probe to the child's stdin; when the child
   has already exited (a crash on boot, or the `true` stand-in the tests
   spawn), the write fails with EPIPE. That error arrives ASYNCHRONOUSLY as a
   stream `error` event, so the try/catch around the write never sees it and,
   with no listener, it is an uncaught exception. Bun 1.3 dropped it
   silently; 1.4 emits it (trace: `internal:streams/writable` → `_destroy` →
   `internal:fs/streams`). Fix: an `error` listener on `child.stdin` that
   logs and yields to the `close` handler, which already owns classify-and-
   respawn. This is a production path: on 1.4 a child dying before the
   probe would have taken the proxy down instead of respawning it. The
   pem-utils "failure" in the same run was this EPIPE landing in the next
   file's window — collateral, not a crypto change.

2. `security.check-credentials.cli.test.ts` — the `--quiet` end-to-end
   tests saw four structured JSON boot warnings on stdout. `tests/setup.ts`
   sets `MINSKY_LOG_MODE=STRUCTURED` on `process.env` for the IN-PROCESS
   logger; that is an env var, so it crosses into the spawned CLI, which
   correctly obeys it (`resolveDiagnosticSink` ranks STRUCTURED above the
   one-shot discard). Measured, not inferred: on 1.3.14 `spawnSync` with the
   default env hands a child an env snapshot that predates a runtime
   `process.env` mutation (`<absent>`); on 1.4.2 the child sees the live
   value, as in Node. The test had been passing on that bug. Fix: `runCli`
   strips the harness's log mode from the child env, since the contract
   under test is the DEFAULT mode an operator's shell provides.

Negative control: both were reproduced locally on the 1.4.2 binary before
the fixes — proxy.test.ts 13 pass / 1 fail with the EPIPE trace;
check-credentials 3 fail with `Expected ""` / `Received` the four JSON
lines. After: proxy + pem-utils 17/0 on 1.4.2, proxy 14/0 on 1.3.14;
check-credentials 9/0 on 1.4.2 with the child also on 1.4.2, 9/0 on 1.3.14.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0147JJ3SVgqRz9JyUD4NZBz9

@minsky-reviewer minsky-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2


The fix commit addresses Bun 1.4 behavioral surfacing by (1) isolating MINSKY_LOG_MODE from the spawned CLI in the test to restore the intended --quiet contract and (2) adding an error handler on child.stdin to prevent unhandled EPIPE from crashing the proxy under Bun 1.4. Pins are updated to 1.4.2 across inspected workflows and all Dockerfiles are digest-pinned consistently; release.yml no longer floats on latest. I found no critical new defects introduced by these changes. One non-blocking note: narrow the stdin error handler to EPIPE/ECONNRESET or elevate unexpected codes to warn to avoid masking other failures. CI-history accrual and repo-wide enumeration of all 23 sites are post-merge/unverifiable from this diff, but spot checks pass. Verdict: APPROVE.

Findings

  • [NON-BLOCKING] src/mcp/stdio-proxy/proxy.ts:555 — child.stdin error handler swallows all write errors; consider narrowing to EPIPE or elevating unexpected codes
    The new listener on child.stdin (src/mcp/stdio-proxy/proxy.ts:555-567) logs any 'error' event at debug and otherwise suppresses it to keep the proxy alive until the close handler runs. That's correct for the intended EPIPE case (child exited before the readiness probe write), but it also swallows other error codes that could signal a different class of problem. Consider checking err.code and handling EPIPE (and possibly ECONNRESET) quietly, while logging other codes at warn or surfacing them to your existing error path. This preserves the fix without masking unexpected failures.

Spec verification

Criterion Status Evidence
10 consecutive full-install CI runs on main after PR #2707 merges are recorded here with their run ids, and none failed with Fail extracting tarball. Unverifiable This criterion accrues post-merge on main and is not verifiable from the repo diff. No file in this PR can contain live CI history; verification requires inspecting CI run logs after merge.
If any run DOES fail that way with the mitigation in place, this task records it and mt#3623 is reopened rather than the retry count being raised — the validation was falsified, not under-tuned. Unverifiable This depends on post-merge CI outcomes and task-tracking actions (reopening mt#3623) that are outside the repo diff. Cannot be verified here.
The retirement condition is checked: when oven-sh/bun#34827 merges AND ships in a bun release this repo can pin, both mechanisms are removed from all 16 sites and the pin is moved, in one change. SUPERSEDED 2026-09-10 — see ## Re-evaluation (2026-09-10). The actual fix is #34861 (in 1.4.0+). New criterion: move the pin to a 1.4.x release, remove both mechanisms from every install site (23 as of 2026-09-10), in one change. Unverifiable Spot-checks confirm pins moved to 1.4.2 in .github/workflows/ci.yml and .github/workflows/release.yml, and digest pins in Dockerfile and services/*/Dockerfile. However, the criterion requires repo-wide enumeration and removal across all install sites (23). A complete grep-based verification across every workflow and Dockerfile is outside this diff-only surface; without a full sweep, mark as Unverifiable.
If #34827 has not shipped in a pinnable release by 2026-09-07, that is recorded here with a re-evaluation rather than the workaround silently becoming permanent. Deadline passed; re-evaluation recorded below on 2026-09-10. Met The bound task spec provided in this review includes ## Re-evaluation (2026-09-10) detailing the upstream PR closure, the true fix (#34861), and its presence in 1.4.0+. This satisfies the 'recorded here' requirement.
Absorbed from mt#3724: .github/workflows/release.yml pins an explicit bun version rather than latest; services/reviewer/Dockerfile and services/cockpit/Dockerfile (both stages) pin oven/bun:1.4-slim by @sha256: digest, following the root Dockerfile's mt#1726 comment convention. grep -rn 'FROM oven/bun' Dockerfile services/*/Dockerfile shows a digest on every line; grep -rn bun-version .github/workflows/ shows no latest. Met - .github/workflows/release.yml: uses: oven-sh/setup-bun@v2 with bun-version: "1.4.2" (release.yml:20-24) — no latest.
  • Dockerfile: FROM oven/bun:1.4-slim@sha256:cb3bbbb0… (Dockerfile:22).
  • services/cockpit/Dockerfile: both stages pin FROM oven/bun:1.4-slim@sha256:cb3bbbb0… (services/cockpit/Dockerfile:16, 48).
  • services/reviewer/Dockerfile: pins same digest on base line (services/reviewer/Dockerfile:4). |
    | Sites are enumerated by grep at implementation time, not from a count: PR #3590 (mt#3575, IN-REVIEW) adds .github/workflows/seed-sweep-tests.yml carrying the flag, a 1.3.14 pin, and two retry loops. Whichever merges first, the sweep covers what is on main at that moment. | Unverifiable | Whether the implementer performed a current-branch grep-based sweep across all sites cannot be established solely from the changed files shown here. Repo-wide enumeration evidence (e.g., a script log or grep output snapshot) is not part of the diff; cannot verify. |

Documentation impact

  • no-update-needed — This PR upgrades Bun to 1.4.2 and removes an internal CI/install workaround; it also adds a defensive error handler and a test harness env isolation. No public CLI, route, or documented behavior changes were introduced. I scanned docs/deploy-minsky-railway.md (referenced by Dockerfile comments) implicitly via the Dockerfile guidance; the behavior (digest pinning, sourcemap policy) remains consistent with existing commentary. No docs updates appear necessary.

The reviewer's non-blocking point stands: a listener that logs every stream
error at debug hides an unexpected code behind the one expected one. EPIPE
(child exited before the probe write) stays at debug and returns; anything
else is logged at warn with its code, so it reaches operator diagnostics
while the proxy stays up for the close handler. proxy + pem-utils on 1.4.2:
17 pass / 0 fail.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0147JJ3SVgqRz9JyUD4NZBz9

@minsky-reviewer minsky-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2


R4 verification: The fix commit fb420fc1 addresses the prior non-blocking concern in proxy.ts by logging only EPIPE at debug and surfacing any other child.stdin errors at warn, so unexpected cases are visible without crashing the proxy. I spot-checked services/site/nixpacks.toml and release.yml — the Bun pins move to 1.4.2 and the site’s Nixpacks pin is updated; no regressions evident. No new critical issues introduced by the fix commit. Recording one PRE-EXISTING note on the curl-pipe-to-shell installer pattern in the site’s Nixpacks config, out of scope for this task. Overall, prior BLOCKING concerns are addressed and I find no new blocking defects — APPROVE.

Findings

  • [NON-BLOCKING] src/mcp/stdio-proxy/proxy.ts:604 — Unexpected child.stdin errors are now surfaced at warn — prior R3 non-blocking concern addressed
    The follow-up commit fb420fc1 changes the new child.stdin error listener to log only EPIPE at debug and escalate any other error codes to log.warn (see src/mcp/stdio-proxy/proxy.ts:604-620). This addresses the prior round's non-blocking concern that logging all stream errors at debug could hide unexpected failure modes. No action needed — recording as verified.
  • [PRE-EXISTING] services/site/nixpacks.toml:1 — Curl-pipe-to-shell installer pattern remains for bun — acceptable here but carries general supply-chain risk
    services/site/nixpacks.toml uses curl -fsSL https://bun.sh/install | ... bash -s -- bun-v1.4.2. This is a pre-existing pattern in the file, now updated to 1.4.2. While common, curl-pipe-to-shell can carry supply-chain risk if TLS or origin are compromised. Given this is the established approach for Nixpacks here and out of scope for mt#3835, marking as PRE-EXISTING with no requested change in this PR.

Documentation impact

  • no-update-needed — This PR updates internal runtime pins and a small behavior change in src/mcp/stdio-proxy/proxy.ts to handle Bun 1.4’s stream error emission. No public APIs, commands, or documented behaviors change. I checked docs/architecture/stdio-proxy.md and found no guarantees about child stdin error handling that would be invalidated; the change is internal resilience. Workflow pin changes and digest pins are operational hygiene and not part of end-user docs.

@minsky-reviewer minsky-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2


R5 verification (chunk 2/2): The prior BLOCKING finding on the Nixpacks Bun pin was addressed — services/site/nixpacks.toml now installs bun-v1.4.2. The follow-up change to src/mcp/stdio-proxy/proxy.ts adds a targeted child.stdin error listener that treats EPIPE as expected (debug) and surfaces other errors at warn, matching the prior round’s guidance and avoiding uncaught exceptions on Bun 1.4. The CLI test update correctly removes the harness’s MINSKY_LOG_MODE from the spawned child env to keep --quiet semantics stable under Bun 1.4’s live env inheritance. I found no new critical defects introduced by these changes. One PRE-EXISTING note is recorded on the curl-pipe-to-shell installer pattern in the site Nixpacks config. Overall, prior issues are resolved and this chunk is ready to merge.

Findings

  • [PRE-EXISTING] services/site/nixpacks.toml:75 — Curl-pipe-to-shell bun installer remains (updated version) — pre-existing pattern with known supply-chain risk
    The install command continues to use curl -fsSL https://bun.sh/install | BUN_INSTALL=/usr/local bash -s -- bun-v1.4.2 (services/site/nixpacks.toml:75). This pattern predates this PR and was only version-bumped here. While common for Bun’s official installer, curl-pipe-to-shell carries general supply-chain risk if TLS or origin are compromised. Out of scope for mt#3835 and not introduced by this change; marking as PRE-EXISTING for awareness.

Spec verification

Criterion Status Evidence
10 consecutive full-install CI runs on main after PR #2707 merges are recorded here with their run ids, and none failed with Fail extracting tarball. Unverifiable This criterion concerns post-merge CI outcomes and recording run IDs; it is not verifiable from the code diff. The provided PR chunk updates pins and removes mitigations but cannot demonstrate CI history.
If any run DOES fail that way with the mitigation in place, this task records it and mt#3623 is reopened rather than the retry count being raised — the validation was falsified, not under-tuned. Unverifiable This depends on observing CI failures and task tracking behavior post-merge. No code or workflow logic in this diff can enforce or evidence reopening mt#3623.
The retirement condition is checked: when oven-sh/bun#34827 merges AND ships in a bun release this repo can pin, both mechanisms are removed from all 16 sites and the pin is moved, in one change. SUPERSEDED 2026-09-10 — see ## Re-evaluation (2026-09-10). The actual fix is #34861 in bun 1.4.0+. New criterion: move the pin to a 1.4.x release, remove both mechanisms from every install site (23 as of 2026-09-10), in one change. Unverifiable Within this chunk, many workflows update bun-version to "1.4.2" and remove both the retry loop and BUN_FEATURE_FLAG_DISABLE_STREAMING_INSTALL; Dockerfiles switch to oven/bun:1.4-slim@sha256:... and drop the inline hedge/retries. However, compliance requires "every install site" repo-wide. That cannot be fully verified from this chunk alone; the remainder is in the other files (chunk 2).
If #34827 has not shipped in a pinnable release by 2026-09-07, that is recorded here with a re-evaluation rather than the workaround silently becoming permanent. Deadline passed; re-evaluation recorded below on 2026-09-10. Met The task spec section ## Re-evaluation (2026-09-10) is present in the provided spec content and records the upstream correction and deadline re-evaluation.
Absorbed from mt#3724: .github/workflows/release.yml pins an explicit bun version rather than latest; services/reviewer/Dockerfile and services/cockpit/Dockerfile (both stages) pin oven/bun:1.4-slim by @sha256: digest; grep -rn bun-version .github/workflows/ shows no latest. Met - .github/workflows/release.yml: bun-version: latest → "1.4.2" (lines shown in diff)
  • services/reviewer/Dockerfile and services/cockpit/Dockerfile use FROM oven/bun:1.4-slim@sha256:cb3bbbb... in all stages per the diff.
  • Multiple other workflows in this chunk switch bun-version to "1.4.2"; no remaining latest appears in the modified files. |
    | Sites are enumerated by grep at implementation time, not from a count: PR #3590 (mt#3575, IN-REVIEW) adds .github/workflows/seed-sweep-tests.yml carrying the flag, a 1.3.14 pin, and two retry loops. Whichever merges first, the sweep covers what is on main at that moment. | Unverifiable | Enumeration method and coverage across all sites cannot be fully confirmed from this chunk alone. The diff shows many removals and pin updates here, but verifying enumeration (incl. PR #3590’s additions) requires repo-wide checks outside this chunk. |
    | 10 consecutive full-install CI runs on main after PR #2707 merges are recorded here with their run ids, and none failed with Fail extracting tarball. | Unverifiable | Chunked review (files: services/site/nixpacks.toml, src/adapters/shared/commands/security.check-credentials.cli.test.ts, src/mcp/stdio-proxy/proxy.ts). CI run records live outside this diff; cannot be verified from the provided files. |
    | If any run DOES fail that way with the mitigation in place, this task records it and mt#3623 is reopened rather than the retry count being raised — the validation was falsified, not under-tuned. | Unverifiable | Outcome tracking and task reopen state are out-of-repo/process artifacts; not verifiable from the three files in this chunk. |
    | The retirement condition is checked: when oven-sh/bun#34827 merges AND ships in a bun release this repo can pin, both mechanisms are removed from all 16 sites and the pin is moved, in one change. SUPERSEDED 2026-09-10 — now: move the pin to a 1.4.x release and remove both mechanisms from every install site (23 as of 2026-09-10), in one change. | Unverifiable | This chunk shows one site updated (services/site/nixpacks.toml) and code changes for Bun 1.4 behavior, but verifying removal across all sites requires reviewing many other files outside this chunk’s scope. |
    | If #34827 has not shipped in a pinnable release by 2026-09-07, that is recorded here with a re-evaluation rather than the workaround silently becoming permanent. Deadline passed; re-evaluation recorded below on 2026-09-10. | Unverifiable | The re-evaluation text lives in the task spec/PR narrative, not in the three files under review. Cannot verify within this chunk. |
    | Absorbed from mt#3724: .github/workflows/release.yml pins an explicit bun version rather than latest; service Dockerfiles pin oven/bun:1.4-slim by @sha256: digest; grep checks show no latest and all digest pins are present. | Unverifiable | These artifacts are outside this chunk (workflows and Dockerfiles). This chunk cannot confirm them. |
    | Sites are enumerated by grep at implementation time, not from a count; ensure any concurrently-landing files (e.g., PR #3590) are covered by the sweep present on main at merge. | Unverifiable | Enumeration-by-grep across the repository cannot be confirmed from this chunk-only review; out-of-scope files not provided. |

Documentation impact

  • no-update-needed — The changes in this chunk update a Nixpacks Bun pin (internal deploy runtime), adjust a test helper to scrub an env var from a spawned child to preserve the intended default behavior, and add a defensive child.stdin error listener in the stdio proxy. No public API, CLI contract, or documented behavior changes. I checked docs/architecture/stdio-proxy.md for any promises about stdin error handling or spawn env semantics and found none that this would invalidate.

@edobry
edobry merged commit da634d0 into main Sep 11, 2026
24 checks passed
@edobry
edobry deleted the task/mt-3835 branch September 11, 2026 04:16

@minsky-reviewer minsky-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2


Verification round: The prior BLOCKING finding on the site’s Nixpacks Bun pin is addressed — services/site/nixpacks.toml now installs bun-v1.4.2. The stdio proxy adds a targeted child.stdin error listener that treats EPIPE as expected (debug) and escalates other errors to warn, aligning with the earlier non-blocking guidance and preventing uncaught exceptions under Bun 1.4. The CLI test now scrubs MINSKY_LOG_MODE from the child env to preserve intended --quiet behavior given Bun 1.4’s live env inheritance. I found no new critical defects introduced by these changes. Overall, the previous block is resolved and this chunk is ready to merge.

Spec verification

Criterion Status Evidence
10 consecutive full-install CI runs on main after PR #2707 merges are recorded here with their run ids, and none failed with Fail extracting tarball. Unverifiable This criterion depends on post-merge CI history and run records, which are not present in the code diff. Cannot be verified from the modified workflows/Dockerfiles in this chunk.
If any run DOES fail that way with the mitigation in place, this task records it and mt#3623 is reopened rather than the retry count being raised — the validation was falsified, not under-tuned. Unverifiable Reopening mt#3623 and recording outcomes are process/task artifacts outside the repository. No change in this diff can evidence that behavior.
The retirement condition is checked: when oven-sh/bun#34827 merges AND ships in a bun release this repo can pin, both mechanisms are removed from all 16 sites and the pin is moved, in one change. SUPERSEDED 2026-09-10 — see ## Re-evaluation (2026-09-10). The actual fix is #34861, shipped in bun 1.4.0+. New criterion: move the pin to a 1.4.x release, remove both mechanisms from every install site (23 as of 2026-09-10), in one change. Unverifiable Within this chunk, numerous workflows update bun-version to "1.4.2" and remove both the retry loop and BUN_FEATURE_FLAG_DISABLE_STREAMING_INSTALL; Dockerfiles switch to digest-pinned oven/bun:1.4-slim and drop inline hedges. Verifying removal across “every install site” repo-wide requires reviewing additional files outside this chunk.
If #34827 has not shipped in a pinnable release by 2026-09-07, that is recorded here with a re-evaluation rather than the workaround silently becoming permanent. Deadline passed; re-evaluation recorded below on 2026-09-10. Met The provided task spec includes the section ## Re-evaluation (2026-09-10) documenting the upstream correction (#34861) and deadline re-evaluation, satisfying this recording requirement.
Absorbed from mt#3724 (subsumed 2026-09-10): .github/workflows/release.yml pins an explicit bun version rather than latest; services/reviewer/Dockerfile and services/cockpit/Dockerfile (both stages) pin oven/bun:1.4-slim by @sha256: digest; grep -rn bun-version .github/workflows/ shows no latest. Met - .github/workflows/release.yml: bun-version: latest → "1.4.2".
  • services/reviewer/Dockerfile and services/cockpit/Dockerfile use FROM oven/bun:1.4-slim@sha256:cb3bbbb... in all stages.
  • Multiple workflows in this chunk switch bun-version to "1.4.2"; none of the modified workflow files retain latest. |
    | Sites are enumerated by grep at implementation time, not from a count: PR #3590 (mt#3575, IN-REVIEW) adds .github/workflows/seed-sweep-tests.yml carrying the flag, a 1.3.14 pin, and two retry loops. Whichever merges first, the sweep covers what is on main at that moment. | Unverifiable | Enumeration-by-grep across the entire repository (including any concurrently-landing files) cannot be confirmed from this chunk-only review. This chunk shows many updates/removals but not repo-wide coverage. |
    | 10 consecutive full-install CI runs on main after PR #2707 merges are recorded here with their run ids, and none failed with Fail extracting tarball. | Unverifiable | This criterion is about post-merge CI outcomes and their run IDs. It cannot be verified from this code diff or the three files in this chunk. |
    | If any run DOES fail that way with the mitigation in place, this task records it and mt#3623 is reopened rather than the retry count being raised — the validation was falsified, not under-tuned. | Unverifiable | This depends on observing CI runs and task-tracking actions after merge. Not verifiable from repository code in this chunk. |
    | The retirement condition is checked: when oven-sh/bun#34827 merges AND ships in a bun release this repo can pin, both mechanisms are removed from all 16 sites and the pin is moved, in one change. SUPERSEDED 2026-09-10 — see ## Re-evaluation (2026-09-10). The actual fix is #34861 (in bun 1.4.0+). New criterion: move the pin to a 1.4.x release and remove both mechanisms from every install site (23 as of 2026-09-10), in one change. | Unverifiable | This chunk updates one site (services/site/nixpacks.toml) and includes code/test adjustments, but verifying removal across every install site repo-wide requires reviewing many other files outside this chunk. |
    | If #34827 has not shipped in a pinnable release by 2026-09-07, that is recorded here with a re-evaluation rather than the workaround silently becoming permanent. Deadline passed; re-evaluation recorded below on 2026-09-10. | Met | The provided task spec includes ## Re-evaluation (2026-09-10) documenting the upstream correction (actual fix is #34861 in 1.4.0+) and the deadline re-evaluation. |
    | Absorbed from mt#3724: .github/workflows/release.yml pins an explicit bun version rather than latest; services/reviewer/Dockerfile and services/cockpit/Dockerfile pin oven/bun:1.4-slim by @sha256: digest; grep -rn bun-version .github/workflows/ shows no latest. | Unverifiable | Those artifacts (workflows and service Dockerfiles) are not part of this chunk. Their states cannot be confirmed here. |
    | Sites are enumerated by grep at implementation time, not from a count; ensure any concurrently-landing files (e.g., PR #3590) are covered by the sweep present on main at merge. | Unverifiable | Enumeration coverage across the whole repo cannot be assessed from this chunk’s three files. |

Documentation impact

  • no-update-needed — This chunk updates the site’s Nixpacks Bun installer pin to 1.4.2, adjusts a test helper to scrub an env var from a spawned child for stable --quiet semantics, and adds a defensive child.stdin error listener in the stdio proxy. These are internal/runtime/test changes and do not add, remove, or change any public API, CLI flags, or documented workflows. No documentation updates are required for these internal resilience and test adjustments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

authorship/co-authored Co-authored by human and AI agent

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant