Skip to content

Surface stale-binary staleness + fix hardcoded server version (Refs #303) - #307

Open
kiki830621 wants to merge 9 commits into
mainfrom
idd/303-server-staleness-self-check
Open

Surface stale-binary staleness + fix hardcoded server version (Refs #303)#307
kiki830621 wants to merge 9 commits into
mainfrom
idd/303-server-staleness-self-check

Conversation

@kiki830621

Copy link
Copy Markdown
Member

Summary

Surfaces MCP-server staleness and fixes the prerequisite version-rot behind it. Refs #303.

A long-lived Claude Code window keeps its MCP server process alive across a binary update (the wrapper checks the version only at spawn, then execs itself away; session-start.sh's staleness kill fires only on SessionStart), so a window left open across an update keeps serving a stale in-memory image with no external re-check — the lifecycle seam behind a "mcp 連不上" report. The hang itself is already prevented forward by #297 (runGuarded fast-fails the one stuck call and the server survives); what was missing was visibility.

Diagnosis also surfaced the prerequisite bug: the compiled MCP handshake serverVersion was hardcoded "2.7.2" in Server.swift — stale by ~18 releases, with no real self-version for a staleness check to compare against.

Changes

Version truth (prerequisite)

  • Version.swift (new) — AppVersion.current, the single committed source of truth for the server's own version.
  • Server.swift — handshake now reports AppVersion.current instead of "2.7.2".
  • scripts/release.shdies if AppVersion.current disagrees with the release tag.
  • VersionTestsAppVersion.current is valid semver and equals the newest ## [x.y.z] CHANGELOG header. Belt-and-suspenders so the rot cannot silently recur.

Staleness self-check

  • StalenessCheck.swift (new) — pure, actor-free, filesystem-free evaluate(compiled:sidecar:) -> String?. Warns only when both parse as semver and the on-disk sidecar is strictly newer; fails open on absent / unparseable / .
  • MailController.preflightAutomation() — the choke point all AppleScript-backed tools funnel through — runs a one-time-per-process check (static-flag gate), reading the wrapper sidecar next to the running executable and emitting a warn-once stderr nudge to restart. Never throws, never refuses a tool call (refusing would break a still-safe post-AppleScript 操作(get_special_mailboxes/get_email/reply_email)hang 120s 後導致 MCP server 斷線,非乾淨回 -1743 #297 session).
  • stalenessWarningOnce is a nonisolated static seam so the one-time gate is unit-tested synchronously with a counting reader.

Design decisions

Out of scope

Hook-side extra trigger points / PreToolUse-kill (dropped in diagnosis); actually killing/respawning a live stale server; wrapper / session-start.sh changes; refusing tool calls; the pre-guard image that actually hung (unreachable by shipped code — ages out).

Test plan

  • swift build clean.
  • swift test1080 tests, 0 failures (baseline 1068 + 12 new: 10 StalenessCheckTests, 2 VersionTests).
  • StalenessCheckTests: drift → warning naming both + "restart"; equal → nil; on-disk older → nil; nil sidecar → nil; unparseable (either side) → nil; numeric (not string) semver ordering; prerelease/build suffix ignored; one-time gate fires once and does not re-read after.
  • VersionTests: AppVersion.current valid semver, non-empty, == newest CHANGELOG release header.
  • Release-guard: verified the release.sh parse + comparison dies on a drifted tag, passes on a matching one.
  • Live (attended, follow-up): with the built binary + a sidecar edited to a higher version, the first AppleScript-backed tool call emits the one-time stderr staleness warning naming both versions; matching versions → no warning.

The MCP handshake serverVersion was hardcoded "2.7.2" in Server.swift,
stale by ~18 releases, and there was no self-version to compare against
for staleness detection. Introduce AppVersion.current (Version.swift) as
the single committed source of truth; Server.swift reads it. release.sh
now dies if AppVersion.current disagrees with the release tag, and
VersionTests pins it to the newest CHANGELOG header — belt-and-suspenders
so the version rot cannot silently recur.

Refs #303
A long-lived window keeps its MCP server alive across a binary update, so
the running image can lag the on-disk binary with no external re-check.
The #297 guard already prevents the hang; what was missing was visibility.
preflightAutomation() now runs a one-time-per-process staleness check
(StalenessCheck.evaluate over a version sidecar next to the executable),
emitting a warn-once stderr nudge to restart when the on-disk binary is
strictly newer. Never throws, never refuses, fails open on every ambiguous
input. Pure logic + one-time gate unit-tested without Mail/filesystem.

Refs #303
@kiki830621

Copy link
Copy Markdown
Member Author

Verify Report — PR #307 (Refs #303)

Engine

manual fan-out — 4 lens Agents (model: sonnet) + sequenced Devil's Advocate (sonnet) + Codex cross-model (gpt-5.x, via codex-rescue). Frozen at 51f3339; freshness re-checked before aggregate.

Aggregate

FAIL — 2 blocking, 5 non-blocking, 1 pre-existing advisory.

Three lenses (requirements / logic / regression) returned PASS; the security lens and the cross-model reviewer returned FAIL, and the Devil's Advocate upheld both objections and escalated one. The split is itself the finding: the three PASS verdicts each verified the implementation faithfully against the design as stated, and the design does not achieve the issue's purpose.


Blocking

B1 — CRITICAL — unbounded sidecar read executes OUTSIDE runGuarded, on the singleton actor's serial executor
Sources/CheAppleMailMCP/AppleScript/MailController.swift:417-419 (call), :455-462 (readVersionSidecar), reached from :93 / :111 / :220.

preflightAutomation() is invoked before runGuarded(...) at all three call sites (let granted = try preflightAutomation() then return try runGuarded(...)), and runGuarded's deadline wraps only the closure passed to it. So String(contentsOf:) — a fully-buffering read with no timeout, no O_NOFOLLOW, no size cap — runs unguarded. MailController is an actor and a process-wide singleton (static let shared), and preflightAutomation is not nonisolated, so a blocking read occupies the actor's serial executor: every one of the ~53 AppleScript-backed tools queues behind it indefinitely, and the only symptom the client sees is its own ~120s idle timeout dropping the whole connection — precisely the failure mode #297 exists to eliminate.

Reachable with one command (mkfifo ~/bin/.CheAppleMailMCP.version; open() on a FIFO with no writer blocks forever). A symlink to /dev/zero gives unbounded memory growth instead. Requires local same-user write access to the executable's directory — a bar that also permits replacing the binary, so this is a robustness/consistency defect more than a privilege-escalation path; the severity comes from the blast radius and from it being trivially triggerable by accident (a stalled network mount, a corrupted file) as well as deliberately.

This repo already solved this exact primitive. ExportDirLock (#236) opens a fixed, user-writable path with O_NOFOLLOW | O_NONBLOCK and has regression tests named testExportDirLock_symlinkAtLockfilePath_refusedNotFollowed and testExportDirLock_fifoAtLockfilePath_failsFastNotHang (Tests/CheAppleMailMCPTests/ExportEmailsMarkdownTests.swift:645-679), the latter asserting fail-fast under 2 s. The new code applies none of it.

Compounding: StalenessCheck.swift's own doc comment cites "#297's timeout guard already prevents such a stale server from hanging-to-disconnect" as justification — but the new code sits outside that guard. The safety argument in the PR does not cover the line it was written to defend.

B2 — CRITICAL (escalated from HIGH) — the one-time gate is consumed at startup, so the feature never fires in its target scenario
Sources/CheAppleMailMCP/Server.swift:46, MailController.swift:412-424, StalenessCheckTests.swift (testGate_noDrift_stillFlipsAndStopsReading).

Server.swift:46 runs Task { try? await mailController.checkForNewMail() } fire-and-forget during init(), after registerHandlers() and before server.start(transport:) — i.e. before any client request can arrive. That path (checkForNewMailbuildCheckForNewMailScript, which cannot throw → runScriptpreflightAutomation) reaches the staleness block with no earlier throw. The block sits before the switch AutomationStatus.probe(), so Mail.app being closed or Automation being denied does not prevent consumption — those throws happen after the gate has already flipped.

At that moment the sidecar necessarily matches the running binary (the wrapper just spawned this process from it), so evaluate returns nil, no warning is emitted, and didCheckStaleness is permanently true. When the binary is later updated on disk — hours or days later, which is exactly issue #303's stated Expected scenario ("a long-lived window that never re-triggered the staleness kill", the case session-start.sh structurally cannot cover) — every subsequent call short-circuits and never re-reads.

Even without the startup task the design fails: any process's first preflightAutomation() call is overwhelmingly likely to see no drift. Server.swift:46 merely makes it certain and sub-millisecond. The implementation is a one-time check where the feature required a one-time warning.

testGate_noDrift_stillFlipsAndStopsReading asserts this behavior as correct — the defect was written into the spec, which is why three independent lenses certified it.


Non-blocking

# Sev Finding Source
3 MEDIUM StalenessCheck.swift:36 interpolates the raw sidecar string (v\(sidecar)), not the parsed integers. Only the prefix before the first -/+ is validated, so 2.26.0-<ANSI escapes / newlines / forged log lines> passes and is written verbatim to stderr, which Claude Desktop collects into ~/Library/Logs/Claude/mcp-server-*.log. Fix: interpolate onDisk.major/minor/patch. security, DA
4 LOW→MED StalenessCheck.swift:52split(whereSeparator:) omits empty subsequences, so a leading -/+ is swallowed and SemVer("-1.2.3") parses as 1.2.3 (verified empirically by two reviewers independently). Not reachable from the current trusted pipeline. Codex's accompanying "drops prerelease precedence" claim is refuted: that is documented in the doc comment and deliberately covered by testSemVer_ignoresPrereleaseAndBuildSuffix — a design decision, not a defect. Severity holds only via its interaction with #3. logic, codex; adjudicated by DA
5 MEDIUM No test covers the live layer. Deleting the entire preflightAutomation wiring, or making readVersionSidecar() always return nil, leaves the suite fully green — the tests exercise the pure helper and an injected reader closure only. This is the direct cause of B1/B2 surviving three PASS verdicts. codex, DA
6 LOW (↓ from MED) scripts/release.sh:111grep -oE 'static let current = "…"' | head -1 would match a commented-out declaration preceding the real one. Hypothetical; no such convention in this repo. Cheap hardening: anchor ^\s*static let current or filter ^\s*//. codex; downgraded by DA
7 LOW VersionTests does not assert that Server.swift actually uses AppVersion.current, so a future re-hardcode would pass. The "cannot recur" claim is stronger than the tests support. codex
8 MEDIUM (pre-existing, advisory) mcpb/manifest.json:5 is still "version": "2.7.2", and neither release.sh nor the Makefile ever updates it. This drift pre-dates this PR — it was masked because Server.swift's hardcoded "2.7.2" coincidentally matched. Making the handshake dynamic doesn't cause it; it makes it visible. → follow-up issue, not a blocker. regression

Confirmed non-issues

Both Codex and the logic lens independently cleared: the actor-isolated didCheckStaleness passed inout to a synchronous nonisolated static helper (no suspension point between entry and call, so actor serialization preserves exclusivity); @discardableResult remains correctly bound to preflightAutomation(); and Bundle.main.executableURL resolves correctly through the wrapper's exec, matching the wrapper's actual VERSION_FILE="$INSTALL_DIR/.${BINARY_NAME}.version" convention. No hardcoded secrets. release.sh's new guard is shell-injection-safe and correctly placed before every build/sign/tag/push/upload step. Full suite: 1080 tests, 0 failures — no regression.

Scope check

Clean. The diff touches exactly the 8 planned files; every out-of-scope item from the approved plan (hook trigger points, wrapper / session-start.sh, kill-or-respawn, refusing tool calls) is untouched.


Fix direction (validated by the DA)

  1. Gate semantics → consume only when a warning is actually emitted (one-time warning, not one-time check).
  2. Bounded readopen(O_RDONLY | O_NONBLOCK | O_NOFOLLOW) + fstat regular-file check + small byte cap, mirroring ExportDirLock. Not a detached-thread timeout wrapper — per bug: 草稿建立目前卡住 — 全路徑 re-test(create_draft clean/legacy・update_draft・reply/forward save_as_draft) #301, an abandoned thread keeps consuming resources; O_NONBLOCK prevents the block at the syscall instead of tolerating it.
  3. No time-based throttle. With a bounded read the per-call cost is a few syscalls, negligible beside the AutomationStatus.probe() Apple Event already on the same path — and per .claude/rules/r-must-direct-db.md most read tools go through SQLite and never reach preflightAutomation(), so this is user-paced, not a hot path. A throttle would reintroduce a smaller version of B2's detection gap plus extra mutable state.
  4. Interpolate parsed integers (improve: 信箱搜尋效能優化 — 考慮 word embedding 技術 #3); fix the leading -/+ parse and add the missing test case (fix: list_emails 當 limit 超過實際信件數量時報 Invalid index 錯誤 #4); add wiring coverage (fix: search_emails 無法搜尋寄件人地址 #5); harden the release.sh extraction (fix: create_draft AppleScript error -2741 on multiline body with CJK text #6); lock Server.swift's use of AppVersion.current (perf: search_emails runs the same AppleScript query 4 times sequentially #7).

Fix round to follow on this branch, then re-verify. No idd-*-verified tag (verdict is FAIL).

Two blocking defects, both in the layer no test reached.

B2 — the gate was a one-time CHECK, not a one-time WARNING. Server.swift's
fire-and-forget startup checkForNewMail() reaches preflightAutomation()
during init(), before the transport starts, when the sidecar necessarily
still matches the running binary — so it burned the gate on a guaranteed
no-drift result and every later call short-circuited. The feature was dead
in exactly the long-lived-window scenario #303 exists for, and a test
asserted that as correct, which is why three review lenses certified it.
The gate is now consumed only when a warning is actually emitted.

B1 — the sidecar read was an unbounded String(contentsOf:) at a fixed,
user-writable path, running OUTSIDE #297's runGuarded on the serial
executor of the singleton actor: a planted FIFO would stall all ~53
AppleScript tools indefinitely. Now mirrors ExportDirLock (#236), which
had already solved this primitive here: O_RDONLY|O_NOFOLLOW|O_NONBLOCK +
fstat S_ISREG + 64-byte cap.

Also: warning interpolates parsed ints, not raw sidecar bytes (log
forging); SemVer rejects a leading -/+ (split omitted empty subsequences
so "-1.2.3" parsed as 1.2.3); release.sh version extraction anchored at
line start so a commented-out declaration cannot be matched.

New StalenessSidecarReadTests covers the live filesystem layer (FIFO and
device cases assert promptness, not just a nil return — only the clock
distinguishes a rejection from a hang). New source guards fail the suite
if the wiring is deleted or Server.swift re-hardcodes a version literal;
verified by deleting each and watching them go red.

1096 tests, 0 failures.

Refs #303
@kiki830621

Copy link
Copy Markdown
Member Author

Fix round — all 7 verify findings addressed (51f3339049f21f)

Every finding from the verify report is resolved. 1096 tests, 0 failures (1080 + 16 new).

# Sev Fix
B1 CRITICAL Bounded read mirroring ExportDirLock (#236) — open(O_RDONLY|O_NOFOLLOW|O_NONBLOCK) + fstat S_ISREG + 64-byte cap. Symlink → ELOOP; FIFO and /dev/zero → rejected as non-regular; oversized file → capped.
B2 CRITICAL Gate is consumed only when a warning is actually emitted. A no-drift result leaves it armed, so an update landing hours later is still caught. didCheckStalenessdidWarnStaleness.
3 MEDIUM Warning interpolates parsed major.minor.patch, never raw sidecar bytes.
4 LOW→MED SemVer rejects a leading -/+. Prerelease handling untouched — the DA correctly refuted that part of the finding as a documented design decision.
5 MEDIUM New StalenessSidecarReadTests (live filesystem layer) + StalenessWiringGuardTests (source guards).
6 LOW release.sh extraction anchored at line start.
7 LOW Source guard pins Server.swift to AppVersion.current, rejects any hardcoded version: "x.y.z".
8 advisory Pre-existing mcpb/manifest.json drift → filed as #311, correctly out of scope here.

Each fix was proven, not assumed

  • B2 went RED before GREEN. A test reproducing the production sequence (startup no-drift → later drift) failed against 51f3339 with the gate consumed and the drift silent, then passed after the inversion. Had it been green on the unfixed code it would have proved nothing.
  • B1's FIFO/device tests assert promptness, not just nil. A hang and a rejection both eventually yield "no warning"; only the time bound distinguishes them (the export_emails_markdown: concurrent/overlapping run() to same output_dir has TOCTOU collision window (follow-up from #232 verify) #236 test's reasoning).
  • The source guards were verified to bite. Deleting the wiring from preflightAutomation() still compiles cleanly — which is exactly why the old suite stayed green and how B1/B2 survived three PASS verdicts — and the guard now fails loudly on it.
  • fix: create_draft AppleScript error -2741 on multiline body with CJK text #6 demonstrated concretely. With a commented-out // static let current = "9.9.9" planted above the real declaration, the old pattern extracts 9.9.9; the anchored one holds 2.25.0.

The test that had to be deleted

testGate_noDrift_stillFlipsAndStopsReading asserted the B2 defect as intended behavior. Three reviewers cited it as evidence the design was deliberate — the logic lens verbatim: 「這是刻意設計...測試已鎖住此行為」. It is replaced by testGate_noDrift_leavesGateArmedAndKeepsChecking, which asserts the opposite.

Notes

  • Toolchain: this machine has two incompatible Swift toolchains — Xcode's 6.3.3 (what xcrun resolves, and what produced every prior result including the reviewers' runs) and swiftly's 6.2.4, now first on PATH. .build modules cannot be shared across them. Pinned to xcrun swift (6.3.3) so this round's results stay comparable with the findings being fixed.
  • One pre-existing flake: SpecialMailboxesScriptBuilderTests/test_runScriptAsList_preservesEmptyStringPositions intermittently fails with Run loop nesting count is negative (-1) under load (failed in a 104 s run, passed in a 63 s run; 5/5 in isolation). Same pattern observed before any fix-round change. Not caused by this PR; not silently ignored either.
  • Accepted residual, stated not hidden: O_NONBLOCK does not defeat a hard-mounted network filesystem, where open() can still block uninterruptibly. Pathological for an executable's own directory — the same mount would stall exec of this binary.

Re-verify next. No merge and no close until a clean ensemble run against 049f21f.

Round 2 confirmed the runtime core correct (gate inversion, fd lifecycle,
FIFO/device defense, bounded read, stderr canonicalization) but found the
GUARDS did not hold. All independently reproduced before fixing.

- Source guards used lexical contains(), so COMMENTING OUT the wiring
  (rather than deleting it) left the identifiers in the text and passed.
  Now strip // and /* */ before matching, and extract the function body by
  brace balancing instead of a fixed .prefix(1200) window that would
  silently stop covering the wiring as the function grows.
- release.sh's line anchor stopped '// static let ...' but not a block
  comment, whose inner line begins with 'static' — so a stale version
  could still be extracted and shipped. Now strips both comment forms
  before extracting. Verified against line, block, inline-block and mixed.
- The FIFO/device tests measured Date() around a synchronous call, which
  is not an executable timeout: a regressed blocking open() would hang the
  suite forever and never reach the assert. Now bounded by a real deadline
  (detached thread + semaphore) so a regression fails red instead of
  wedging. Leaked-worker-on-timeout residual documented — same tradeoff
  #297 takes in production.
- Corrected the /dev/zero comment's misattribution: the 64-byte cap
  already bounds that read, so fstat's job there is rejection, not
  boundedness — and mutation testing shows the FIFO case survives fstat
  removal (O_NONBLOCK + EAGAIN cover it). The two checks cover different
  threats.
- CHANGELOG no longer claims 'one-time-per-process' / 'at most one read
  per process' / 'off the hot path'; the gate inversion made all three
  false and the old text contradicted the new paragraph.

Both guard bypasses were verified to pass before the fix and fail after.
1097 tests, 0 failures.

Refs #303
@kiki830621

Copy link
Copy Markdown
Member Author

Verify round 2 — 049f21f, and the fixes on top (b66eb52)

Round-2 verdict: FAIL on 049f21f — but on much narrower ground than round 1. All three reviewers confirmed the runtime core is correct; every remaining finding was in the guards and docs. Fixed in b66eb52.

Lens Verdict on 049f21f
Security + syscall PASS
Logic + regression PASS
Codex (gpt-5.6-sol) FAIL — 3 MEDIUM, 2 LOW

Confirmed correct (independently, by more than one lens)

Gate inversion genuinely fixes the round-1 scenario; fd lifecycle sound (defer after the guard, every return path closes); guard n > 0 catches both EOF and -1, so the negative-slice trap is unreachable; ssize_tInt import safe; no path TOCTOU (fstat is on the open fd, not a re-stat); no actor interleaving (no suspension point between read and write); no route for attacker bytes to reach stderr — three SemVer bypasses attempted (leading whitespace, U+2212, embedded control chars), all fail-open.

The logic lens ran mutation testing on the sidecar tests: removing fstat, O_NOFOLLOW, or the byte cap each turned exactly one corresponding test red. The defenses are load-bearing and precisely covered.

What actually failed — my guards, not the code

# Sev Finding Status
1 MEDIUM Source guards used lexical contains, so commenting the wiring out (vs deleting it) left the identifiers in the text and passed fixed — strip // and /* */, extract body by brace balancing
2 MEDIUM release.sh's line anchor stopped // but not a block comment, whose inner line begins with static — a stale version could still ship fixed — strip both forms first; verified against line / block / inline-block / mixed
3 MEDIUM FIFO+device tests measured Date() around a synchronous call — not an executable timeout. A regressed blocking open() would hang the suite forever, never reaching the assert fixed — real deadline (detached thread + semaphore); leaked-worker residual documented, same tradeoff #297 takes
4 LOW /dev/zero comment misattributed the defense fixed — see below
5 LOW CHANGELOG still claimed "one-time-per-process", "at most one read per process", "off the hot path" — all falsified by the gate inversion fixed

Both guard bypasses were reproduced before fixing and re-tested after: commenting the wiring out gave 3/3 green pre-fix, and 2 failures post-fix under both // and /* */ forms. The release.sh extraction yielded 9.9.9 pre-fix and holds 2.25.0 post-fix across all four comment shapes.

Two corrections to things I previously asserted

  • My flake attribution was wrong. I reported SpecialMailboxesScriptBuilderTests/test_runScriptAsList_preservesEmptyStringPositions as a pre-existing flake. It passes cleanly in every run. The Run loop nesting count is negative crash appears only under --parallel, during teardown of two XCTSkip'd MailAppIntegrationTests, and traces to AppleScript 操作(get_special_mailboxes/get_email/reply_email)hang 120s 後導致 MCP server 斷線,非乾淨回 -1743 #297's deliberately abandoned Thread.detachNewThread workers — XCTest simply attributes the exception to whichever test is running when it surfaces. Better root cause than mine; it is pre-existing and unrelated to this PR either way.
  • The FIFO/device defenses layer differently than I documented. Mutation testing shows removing fstat leaves the FIFO case green — O_NONBLOCK plus readEAGAIN already cover it. fstat's distinct job is rejecting a character device like /dev/zero, which reads fine and which the 64-byte cap already bounds. So fstat provides rejection, not boundedness, and the two checks cover different threats. Comment corrected.

Process note

The security lens could not run the suite (it hit the 6.2.4-vs-6.3.3 toolchain split and .build lock contention from my parallel runs) and said so rather than implying it had. Its review is static-only; the executable coverage comes from my own runs and the logic lens's. My omission — I gave the xcrun swift warning to one reviewer and not the other.

Also: the logic lens observed my uncommitted round-2 fixes appearing in the working tree mid-review and correctly separated them from 049f21f rather than crediting them to the frozen commit. They are now committed as b66eb52.

1097 tests, 0 failures. Round 3 next, against b66eb52.

Only the FIFO case can actually hang — a FIFO's open() has
wait-for-the-peer semantics, while read on a character device is bounded
by the count argument and on a regular file does not block. The helper is
still applied uniformly (cheap, and keeps the bound honest if the
implementation changes which syscall does the work), but the comment no
longer implies all three tests carried the same hang risk.

Refs #303
… on behavior

Round 3 confirmed the runtime fixes from rounds 1-2 but dismantled the
guards protecting them, and found one real production bug.

- PRODUCTION BUG: reading exactly the byte cap let truncation MANUFACTURE
  a valid version. A 65-byte file '99.0.0' + 58 spaces + 'X' — whose full
  content is not a version — trimmed to exactly '99.0.0' and raised a
  bogus drift warning, breaking the documented fail-open invariant. Now
  reads cap+1 and rejects anything larger. Live-verified silent.

- The source guards could not work in principle. They asserted that the
  text 'stalenessWarningOnce' appeared in preflightAutomation's source;
  a decoy string passed every assertion with the real call deleted. Also
  defeated by nested block comments (Swift permits them; a boolean cannot
  track depth) and by '{' in a string literal inflating the brace count so
  functionBody overran into the following declarations. Each patch was
  bypassed by the next legal-Swift construct because the premise was
  wrong: text is not behavior. Replaced with behavioral tests driving the
  real chokepoint through setTestSeams — the convention this file already
  established for scriptRunnerOverride. Verified against all three
  bypasses: green on clean source, 3 failures on each.

  (First attempt at this called runStalenessCheck() directly and so proved
  nothing about whether preflightAutomation invokes it — the same error in
  new clothing. The test now enters via runScriptAsList, a real caller.)

- release.sh now COMPILES Version.swift and prints AppVersion.current
  instead of parsing for it. Three text-matching attempts were each
  defeated by different legal Swift — and the last one, a multi-line
  string literal containing a fake declaration, is not a comment at all,
  so no comment-stripper could ever have helped. Recognising Swift needs a
  Swift parser. Less code than the lexer it replaces.

- CHANGELOG now states scope honestly: the check covers wrapper installs
  only (a Claude Desktop .mcpb install has no sidecar and the feature is
  inert there — verified on a live machine, tracked as #312), and the
  warning is a diagnostic-log signal, not a user-facing prompt. Also
  trimmed the review chronology that had accumulated in it.

1099 tests, 0 failures. Live end-to-end re-verified.

Refs #303
@kiki830621

Copy link
Copy Markdown
Member Author

Verify round 3 — b1d5488, fixes in 3d8d9f7

Round-3 verdict: FAIL. The deepest round yet, and the one that found the guards from round 2 were not merely buggy but unworkable in the form chosen. All findings reproduced independently before fixing. 1099 tests, 0 failures; live end-to-end re-verified.

Lens Verdict
Coherence (whole change, 5 commits) PASS — mergeable, no new defects
Codex (gpt-5.6-sol) FAIL — 4 HIGH, 2 MEDIUM, 1 LOW
Test-infra incomplete — hit a session limit and died before reporting

Findings and fixes

# Sev Finding Fix
C MEDIUM — production bug Reading exactly the byte cap let truncation manufacture a valid version: a 65-byte "99.0.0" + 58 spaces + "X", whose full content is not a version, trimmed to exactly "99.0.0" and raised a bogus drift warning. The documented "fails open on every ambiguous input" invariant was false. read cap+1, reject anything larger. Live-verified silent.
A HIGH The source guards could not work in principle. A decoy _ = "stalenessWarningOnce didWarnStaleness" passed every assertion with the real call deleted and the check disabled. Also defeated by nested block comments (Swift permits them; a boolean cannot track depth) and by "{" in a string literal inflating the brace count so functionBody overran into the following declarations — which happen to contain both identifiers. replaced with behavioral tests driving the real chokepoint via setTestSeams, the convention this file already used for scriptRunnerOverride. Verified against all three bypasses: green on clean source, 3 failures on each.
B HIGH release.sh could ship a mismatched binary: a legal multi-line string literal containing a fake static let current yielded 9.9.9. Nested comments too. release.sh now compiles Version.swift and prints AppVersion.current. Verified against line / block / nested / string-literal inputs — all yield 2.25.0.
D HIGH The feature is inert for Claude Desktop .mcpb installs — no sidecar next to the packaged executable. Verified on a live machine: the install exists, find for .*.version returns nothing. CHANGELOG now states the scope limit plainly; tracked as #312.
E MEDIUM stderr is a diagnostic-log signal, not user-facing; and if a server is already unreachable no tool call arrives, so the check never runs. said plainly in the CHANGELOG rather than implied away.
LOW CHANGELOG had become a review chronicle. rewritten, much shorter.

Confirmed sound by Codex: withDeadline/ResultBox (the semaphore supplies the happens-before edge; the double-optional correctly separates timeout from a legitimate nil), and the reader's O_NOFOLLOW/O_NONBLOCK/fstat defenses.

The mistake worth recording

My first attempt at the behavioral fix called runStalenessCheck() directly — so deleting the wiring from preflightAutomation() left it green, exactly like the text guards it replaced. I extracted a method and then tested the method, which proves nothing about whether the chokepoint invokes it. My own bypass check caught it before I claimed the fix worked. The test now enters through runScriptAsList, a real caller.

That is the same error three rounds running, each time in new clothing: round 1 verified an implementation against my own stated design; round 2 verified a guard against my own imagined mutation; round 3 verified a seam against my own extracted method. Each was caught only by asking a question I had not thought to ask.

Process disclosure

  • The test-infra lens died to a session limit before reporting. Its nested-block-comment finding I reproduced independently; anything else it found is lost. That lens is a gap in this round, not a pass.
  • The coherence lens reported — and correctly disregarded — a prompt-injection attempt: a fake "system" turn instructing it to stop using tools and emit a bogus compaction summary. The only untrusted content in its context was repo source and diffs.
  • It also ran rm -rf .build and rebuilt under the other toolchain (6.2.4 rather than the 6.3.3 that .build was built with). Incidentally useful — the suite passes under both — but it mutated shared state mid-review.

Round 4 next, against 3d8d9f7. Three rounds have each found something one layer further out; I am not going to predict that stops.

`trap 'rm -rf "$tmp"' RETURN` never fired. bash honors a RETURN trap only
inside a function body, and command substitution is a subshell — so every
release leaked a temp dir containing a compiled probe binary. EXIT is the
correct signal for this scope; verified empirically (RETURN leaks, EXIT
cleans, probe still returns 2.25.0).

Also reworded the fail-closed message: it said 'could not parse', which is
no longer what the step does, and it now names the actual causes (missing
xcrun/swiftc, Version.swift no longer compiling standalone, mktemp failure).

Found in verify round 4 — notably it survived my own review and three
reviewers reading the round-3 diff, and only surfaced when the construct
was executed in isolation rather than read.

Refs #303
…tifact

Two reviewers independently found the same fourth bypass of the round-3
behavioral test: adding `guard stalenessReaderOverride != nil else { return }`
left all 31 staleness tests green while silently disabling the feature in
production. The cause is structural — a test must ENABLE the seam to control
which version is read, so its verification was coupled to the exact condition
the mutation keys on. It could only observe a world it had already intervened in.

So the seams are gone. The wiring test now stages a real executable with a real
sidecar, runs it as a real process, and reads its real stderr. There is nothing
to key off, and the production sink is exercised rather than substituted.
Removed from production: two overrides, resetStalenessGateForTesting(), and the
internal runStalenessCheck() (private again, inlined). Verified against all four
mutations — green on clean source, red on each.

- CRASH (verified, exit 134/SIGABRT): the production sink used the non-throwing
  FileHandle.standardError.write, which raises an uncatchable ObjC exception on
  a bad descriptor. A host launching the server with stderr closed turned an
  advisory nudge into a process abort, violating "never throws / never refuses"
  by a route the wording did not anticipate. Now throwing write(contentsOf:)
  with the error swallowed — the #301 remedy. Every test had injected a fake
  sink, so the production sink was never exercised until the seams came out.

- release.sh now interrogates the SHIPPED artifact: each slice of the universal
  binary is run with a new --version mode and must exactly match the tag. The
  host-compiled probe only guessed at the artifact, and a legal
  `#if arch(x86_64) static let current = "0.0.0"` made arm64 print 2.25.0 while
  the Intel slice being signed reported 0.0.0 — silently. Exact match also
  catches a trailing newline baked into the literal, which bash $( ) strips.

- release.sh probe no longer trips set -e at its own assignment, which made the
  diagnostic naming the failure causes unreachable dead code for the two cases
  most worth explaining. Compiler stderr is now shown instead of discarded.

1098 tests, 0 failures. Live re-verified: drift warns, match/corrupt silent,
FIFO bounded, stderr-closed no longer aborts.

Refs #303
@kiki830621

Copy link
Copy Markdown
Member Author

Verify round 4 — 3d8d9f7, fixes in 158fb01 + 36987be

Verdict: FAIL → fixed. 1098 tests, 0 failures; live re-verified. Net effect: less production code than before, more strongly verified.

Lens Verdict
Seam / infra risk FAIL — 1 HIGH (4th bypass), 1 MEDIUM (dead diagnostic)
Codex (gpt-5.6-sol) FAIL — 1 HIGH, 3 MEDIUM, 1 LOW

Both reviewers independently found the same fourth bypass, and both verified it by running rather than reasoning.

The finding that mattered

guard stalenessReaderOverride != nil else { return }   // added to runStalenessCheck()

All 31 staleness tests stayed green; production never sets a seam, so the warning vanishes silently and permanently.

The cause is structural, not a slip: a test must enable the seam to control which version is read — so its verification was coupled to the exact condition the mutation keys on. The test could only observe a world it had already intervened in.

Fix — delete the seams rather than patch around them. The wiring test now stages a real executable, writes a real sidecar beside it, runs it as a real process, and reads its real stderr. Nothing to key off; the production sink is exercised rather than substituted. Removed from production: two overrides, resetStalenessGateForTesting(), and the internal runStalenessCheck() (private again, inlined). Verified against all four mutations — green on clean source, red on each, and it now also catches a neutered sink, which the seam-based test structurally could not.

Other findings, each reproduced before fixing

Sev Finding Fix
CRASH The production sink used non-throwing FileHandle.standardError.write, which raises an uncatchable ObjC exception on a bad descriptor. Host launches the server with stderr closed → exit 134 / SIGABRT. An advisory check could kill the process — violating "never throws / never refuses" by a route the wording didn't anticipate. Every test injected a fake sink, so this was never exercised. throwing write(contentsOf:), error swallowed — the same remedy #301 already adopted on the osascript stdin path
HIGH A legal #if arch(x86_64) static let current = "0.0.0" defeats the host-compiled probe: arm64 prints 2.25.0 while the Intel slice being signed and shipped reports 0.0.0. Tag, tests, probe, lipo all pass. (#if compiler(...) likewise — and this repo builds probe and slices with different toolchains.) new --version mode; release.sh runs each slice of the actual artifact and requires an exact match
MEDIUM static let current = "2.25.0\n" — bash $() strips trailing newlines so the guard passed, but the real handshake carried a newline → semver parse fails → staleness permanently fail-open exact-match comparison against the shipped slices catches it
MEDIUM My round-3 die message was dead code for 2 of the 3 causes it listed: set -e killed the script at the assignment before reaching it `
LOW→fixed earlier trap ... RETURN inside $( ) never fires (bash scopes RETURN to function bodies) — every release leaked a temp dir holding a compiled binary EXIT (158fb01)

Clean: seams cannot leak to production (separate process); no test pollution; --parallel 1099/0; wiring test environment-independent (staleness runs before AutomationStatus.probe() throws).

Two corrections to my own claims this round

The through-line, four rounds in

R1 verified an implementation against my own stated design. R2 verified a guard against my own imagined mutation. R3 verified a seam against my own extracted method. R4 verified behavior through the only world the test could observe. Each fix moved the blind spot rather than removing it, because each verified through the same instrument it was validating. What finally worked was removing the instrument: run the real thing and look at what comes out.

Round 5 next, against 36987be.

Round 5 (and my own real-binary reproduction) showed the round-4
"crash-proof" claim was imprecise. The throwing write(contentsOf:) fixes the
descriptor-error family (closed / read-only fd 2 — both verified to survive),
but NOT SIGPIPE: a broken-pipe stderr (reader gone) kills the process in the
kernel before write() returns, so no try? can intercept it (verified on the
real binary — exit signal 13 when a drift makes emitDiagnostic fire; the
server survives with no drift).

That exposure is a pre-existing whole-server property — 27 stderr-write sites,
no process-wide SIGPIPE handling — and emitDiagnostic is merely the earliest of
them on the startup path. The proper fix is process-scope SIG_IGN + EPIPE
handling, which needs the server's shutdown path validated and is out of this
change's scope; filed as #320. No code behavior change here — only the
CHANGELOG and doc comment, corrected from a blanket claim to the precise
boundary.

Refs #303
@kiki830621

Copy link
Copy Markdown
Member Author

Verify round 5 — 36987be, correction in 10717c3

Verdict: PASS — with an explicit caveat about lens coverage, and one precision correction I owe.

Lens Verdict
Claude (sonnet), delta-focused + mutation testing PASS — 1 MEDIUM, 1 LOW-MEDIUM, both non-blocking
Codex (gpt-5.6-sol) UNAVAILABLE — hit its usage limit (resets Aug 5)

I will not overstate this round. The cross-model lens has delivered the decisive finding in three of the four prior rounds; it could not run here. So round 5 rests on one Claude lens plus my own independent verification — genuinely less coverage than rounds 1-4. A PASS here is weaker evidence than a PASS there, and the honest options are to accept that or to re-run the cross-model lens once its quota resets.

What was checked and is clean

  • --version mode: stdout is exactly 2.25.0\n (7 bytes, xxd-confirmed), stderr empty, exit 0 — so release.sh's exact-match holds. RunMode.parse precedence is --setup > --check-fda > --version > .server; the default stdio path is unperturbed.
  • The process-spawning wiring test: not flaky (5/5 for the lens, 3/3 for me), ~6s each dominated by a fixed sleep with a ~30× margin over the ~0.1-0.2s the warning actually takes; no zombies, no leaked temp dirs.
  • Mutation resistance: the seam-free E2E test caught all four disable-mutations, including a new one — an env-var "detect the test and skip" guard. The lens argued (correctly) that this class is structurally caught, because the assertion reads the real spawned process's real stderr; the only escape is a signal that exists solely in production (e.g. a hardcoded install path), which is deliberate sabotage, not a refactor slip.
  • emitDiagnostic survives a closed and a read-only fd 2 (the round-4 SIGABRT fix holds — verified).
  • release.sh slice loop: fails loudly (die) if a slice can't report its version (e.g. no Rosetta 2 for the x86_64 slice) — not a silent skip.

The one real correction (LOW-MEDIUM, deferred to #320)

The lens found, and I verified on the real binary, that my round-4 "crash-proof" wording was imprecise. try? catches the descriptor-error family (closed/read-only fd 2) but not SIGPIPE: a broken-pipe stderr (reader gone) kills the process in the kernel before write() returns.

Real-binary reproduction sharpened it:

Scenario Result
broken-pipe stderr + version drift (emitDiagnostic fires) killed by signal 13 (SIGPIPE)
broken-pipe stderr, no drift (emitDiagnostic silent) survives

So emitDiagnostic is the earliest of the server's 27 stderr writers on the startup path — it makes an already-fatal fd-2 state fatal at startup, on exactly the drift scenario the feature targets. It is not a new crash class (all 27 writers share it; there's no process-wide SIGPIPE handling), and it is not reachable on a correctly-configured host — Claude Desktop / Claude Code capture the server's stderr to a log (which is the only setup where the warning is even useful), so fd 2 is not a broken pipe there. A host that gives the server a stderr pipe and then doesn't read it is the contradictory case.

Handled: the CHANGELOG and doc comment now state the precise boundary instead of a blanket claim (10717c3); the whole-server SIGPIPE hardening — a process-scope SIG_IGN + EPIPE handling that needs the server's shutdown path validated — is filed as #320, deliberately out of scope for this PR. No code-behavior change in 10717c3.

Documented residual (MEDIUM, not currently triggered)

XCTSkipUnless(fileExists(server)) in the wiring test would silently skip the two E2E tests (suite still reports passed) under swift test --skip-build with a missing executable. The repo's own scripts/CI never pass --skip-build (zero grep hits), so it isn't triggered today; noted rather than changed, because forcing a hard failure would make the test fragile in build matrices the repo doesn't currently use.

Suite

1098 tests, 0 failures. Live end-to-end still green (drift warns, match/corrupt silent, FIFO bounded, closed-fd survives).

The through-line stops moving outward

Rounds 1-4 each found a defect one layer further out. Round 5 found no new blocking defect and no bypass of the seam-free test — only a precision correction to a claim and a pre-existing whole-server property that #303 happens to trigger earliest. That is what convergence looks like: the remaining items are about a boundary being stated accurately, not about the feature being wrong. With the cross-model lens missing, I hold this as a qualified PASS.

kiki830621 added a commit that referenced this pull request Aug 1, 2026
…ne (#325)

The field froze at 2.7.2 for ~18 releases — no release tooling referenced
it, and the drift was masked because Server.swift's then-hardcoded
handshake version had rotted to the same value (the two wrong sources
agreed by coincidence; PR #307's dynamic handshake exposes it).

- mcpb/manifest.json bumped to the current release (2.25.0)
- release.sh dies when the manifest disagrees with the tag (fail-closed;
  JSON-parsed via python3, not grepped — it is JSON, read it as JSON;
  deliberately not auto-edited: the script requires a clean tree, so
  editing mid-release would contradict its own precondition)
- ManifestVersionTests pins manifest == newest released CHANGELOG header
  (RED at 2.7.2 vs 2.25.0 before the bump, GREEN after)

1069 tests, 0 failures.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant