From e377831a31d9c4cb68ba8e7e1f9747a2c755e6bf Mon Sep 17 00:00:00 2001 From: Jeff Yaw Date: Wed, 10 Jun 2026 21:17:49 -0700 Subject: [PATCH] fix: address 10 confirmed findings from review of f7b7cb3..a736200 release.sh: - handle git ls-remote failure in the tag-drift guard explicitly instead of dying on the ERR trap with git's real error suppressed - non-interactive runs now require RELEASE_YES=1 or a piped 'y' instead of proceeding unconditionally; a piped decline aborts again - correct the SKIP_LINT comment (no CI lint gate exists in this repo) and warn loudly when the hatch is active - warn() writes to stderr so warnings survive stdout redirects (the 'npm run lint:fix >/dev/null' path swallowed the SKIP_LINT notice) - mcp-publisher login failures name the actual token source (gh CLI session vs MCP_REGISTRY_TOKEN) and stop hiding the publisher's stderr src: - api.ts: fix getPool() doc claim that an operator can flip ALLOW_WRITES without a restart -- a stdio server's env is fixed at spawn - api.ts: fix typeNameCache concurrency comment to describe the real race (empty-but-non-null map -> duplicate targeted miss-fill) - explain.ts: check the raw table string for double-quotes before the dot-split so pre-quoted names with embedded dots get the actionable "remove the quotes" message; re-tighten the loosened test assertion tests: - tools.test.ts: drop the always-true round-trip loop and the comment's unbacked schema/handler-drift claim changelog: - backfill [0.6.17]-[0.6.19] sections for published versions with none Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 51 ++++++++++++++++++++++++++++++++++++ release.sh | 54 ++++++++++++++++++++++++++++++++------- src/api.ts | 21 +++++++++------ src/tools/explain.test.ts | 8 +++--- src/tools/explain.ts | 14 ++++++---- src/tools/tools.test.ts | 30 ++++++++++------------ 6 files changed, 135 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6bdbbe0..e6414c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -88,6 +88,57 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 a local-only pre-tag gate; this is just an opt-in fail-fast. - `package.json` `package-lock.json` and `server.json` all bumped to 0.6.20. +## [0.6.19] - 2026-06-02 + +Release-flow hardening; no library behavior changes shipped in this version. + +### Fixed +- `release.sh` refuses to push when origin already has `v` pointing + at a different commit (rewound tag elsewhere, parallel release race) -- + previously `git push --follow-tags` silently skipped the stale tag and the + GitHub release linked the wrong commit. Compares tag-object SHAs so resume + runs don't false-abort. +- README "Add to Yaw MCP" badge points at the https forwarder so it renders + as a link on github.com (raw `yaw://` hrefs are stripped). + +### Added +- `SKIP_LINT=1` escape hatch in `release.sh` for hosts where the npm + run-script wrapper segfaults on exit-cleanup (MINGW64-ARM64). +- `wrapToolHandler` extracted from `index.ts` for testability, with unit + coverage of the MCP result wrapper and the connect-failure path (expanded + further in 0.6.20). + +## [0.6.18] - 2026-05-28 + +### Changed +- Release publishing consolidated into `release.sh`: the MCP Registry publish + moved into the script and `release.yml` (plus the non-release CI workflows) + was dropped. The script hands off to CI when a CI publish path exists and + publishes from the workstation otherwise. + +### Fixed +- `release.sh` syncs `server.json` unconditionally, not only inside the bump + branch, so a resume run no longer asks mcp-publisher to re-publish the + previous version (400 duplicate-version). +- `release.sh` falls back to the gh CLI session token when + `MCP_REGISTRY_TOKEN` is unset. +- The release confirmation prompt is tty-gated so non-interactive runs don't + hang on `read`. + +### Docs +- README install badge swapped to the "Add to Yaw MCP" deep link; `npx` + spawn examples pinned to `@latest` for auto-update. + +## [0.6.17] - 2026-05-19 + +### Added +- `release.sh` accepts an optional pre-release commit message as a second + argument: runs the pre-commit checklist, commits tracked changes, then + proceeds with the release. +- Post-publish smoke script (`scripts/post-publish-smoke.sh`) wired into the + release flow -- exercises the published tarball via a real `npx` install + instead of trusting `npm view` registry metadata. + ## [0.6.16] - 2026-05-18 ### Tests diff --git a/release.sh b/release.sh index 327d6fc..752cc1f 100644 --- a/release.sh +++ b/release.sh @@ -30,14 +30,20 @@ NC='\033[0m' # corrupted bytes then get copy-pasted into bug reports. step() { echo -e "\n${CYAN}=== [$1/$TOTAL_STEPS] $2 ===${NC}"; } info() { echo -e "${GREEN} [OK] $1${NC}"; } -warn() { echo -e "${YELLOW} [!] $1${NC}"; } +# warn() writes to stderr so warnings survive stdout redirects -- e.g. the +# pre-commit path runs `npm run lint:fix >/dev/null`, which would otherwise +# swallow the SKIP_LINT noop notice and silently skip the formatting gate. +warn() { echo -e "${YELLOW} [!] $1${NC}" >&2; } fail() { echo -e "${RED} [X] $1${NC}"; exit 1; } # SKIP_LINT=1 escape hatch -- wraps `npm`/`pnpm` so lint-related runs are # no-ops. Workaround for the MINGW64-ARM64 npm-run-script wrapper that -# segfaults on exit-cleanup (platform-windows.md). Apply only when the -# lint runner is broken on the host; CI catches lint regressions anyway. +# segfaults on exit-cleanup (platform-windows.md). NOTE: this repo has no CI +# lint gate (release.yml was dropped when registry publish moved into this +# script), so with SKIP_LINT=1 lint runs NOWHERE for this release -- run +# `npx biome check src/` on a working runner before tagging. if [ "${SKIP_LINT:-}" = "1" ]; then + warn "SKIP_LINT=1 -- lint will not run anywhere this release (no CI lint gate exists); run 'npx biome check src/' on a working runner before tagging" npm() { if [ "$1" = "run" ] && [[ "$2" == lint* ]]; then warn "SKIP_LINT=1 -- noop 'npm run $2'" @@ -143,8 +149,20 @@ if [ "$IS_CI" != "true" ] && [ "$RESUMING" != "true" ]; then echo "Aborted." exit 0 fi + elif [ "${RELEASE_YES:-}" = "1" ]; then + info "RELEASE_YES=1 -- proceeding without confirmation" else - info "Non-interactive shell -- proceeding without confirmation" + # stdin is not a tty: honor a piped reply (`echo y |` proceeds, `echo n |` + # aborts) instead of releasing unconditionally -- a piped decline used to + # be read and honored before the tty-gate. -t 5 bounds the wait so an + # open-but-silent pipe can't hang the release. + REPLY="" + IFS= read -r -t 5 -n 1 REPLY || true + if [[ $REPLY =~ ^[Yy]$ ]]; then + info "Non-interactive stdin -- proceeding on piped 'y'" + else + fail "Non-interactive stdin and no 'y' confirmation -- pipe 'y' or set RELEASE_YES=1 to release non-interactively." + fi fi fi @@ -260,7 +278,12 @@ else # reports success, but origin's tag stays at the old SHA -- and the later # `gh release create` step then creates a GitHub release linked to that # stale commit while npm carries the new one. - ORIGIN_TAG_SHA=$(git ls-remote --tags origin "refs/tags/v${VERSION}" 2>/dev/null | awk '{print $1}') + # No stderr suppression and an explicit failure branch: under `set -euo + # pipefail` a failing ls-remote (network blip, auth) inside the command + # substitution would otherwise kill the script with only the generic + # ERR-trap line and git's actual error discarded. + ORIGIN_TAG_SHA=$(git ls-remote --tags origin "refs/tags/v${VERSION}" | awk '{print $1}') \ + || fail "Could not query origin for tag v${VERSION} (network or auth failure -- see git's error above). Resolve and re-run." if [ -n "$ORIGIN_TAG_SHA" ]; then LOCAL_TAG_SHA=$(git rev-parse "v${VERSION}") if [ "$ORIGIN_TAG_SHA" != "$LOCAL_TAG_SHA" ]; then @@ -441,12 +464,25 @@ else # io.github.YawLabs/* namespace. # Fall back to gh CLI's session token if MCP_REGISTRY_TOKEN is unset -- # gh auth login (admin:org or read:org scope) covers the namespace claim. - : "${MCP_REGISTRY_TOKEN:=$(gh auth token 2>/dev/null || true)}" + # Track which source supplied the token so a login failure names the thing + # the operator actually controls instead of an env var they never set. + TOKEN_SOURCE="env" + if [ -z "${MCP_REGISTRY_TOKEN:-}" ]; then + MCP_REGISTRY_TOKEN="$(gh auth token 2>/dev/null || true)" + TOKEN_SOURCE="gh-cli" + fi if [ -z "${MCP_REGISTRY_TOKEN:-}" ]; then - fail "MCP_REGISTRY_TOKEN unset -- set it to a GitHub PAT with read:org for YawLabs (or run '$MP login github' once interactively to cache the session)." + fail "MCP_REGISTRY_TOKEN unset and 'gh auth token' returned nothing -- set a GitHub PAT with read:org for YawLabs, or 'gh auth login' first (or run '$MP login github' once interactively to cache the session)." + fi + # stdout stays quiet; stderr passes through so mcp-publisher's own + # diagnostic isn't hidden behind the generic fail message. + if ! "$MP" login github -token "$MCP_REGISTRY_TOKEN" >/dev/null; then + if [ "$TOKEN_SOURCE" = "gh-cli" ]; then + fail "mcp-publisher login failed using the gh CLI session token (MCP_REGISTRY_TOKEN was unset). The gh token likely lacks read:org for YawLabs -- run 'gh auth refresh -h github.com -s read:org' and re-run." + else + fail "mcp-publisher login failed -- check MCP_REGISTRY_TOKEN scopes (needs read:org for YawLabs)." + fi fi - "$MP" login github -token "$MCP_REGISTRY_TOKEN" >/dev/null 2>&1 \ - || fail "mcp-publisher login failed -- check MCP_REGISTRY_TOKEN scopes (needs read:org for YawLabs)" "$MP" publish \ || fail "mcp-publisher publish failed -- npm + GitHub release succeeded, but the MCP Registry did not. Retry the step (re-run the script) once the cause is identified." info "Published to MCP Registry" diff --git a/src/api.ts b/src/api.ts index 296e994..a9e997e 100644 --- a/src/api.ts +++ b/src/api.ts @@ -106,10 +106,12 @@ export function getSslConfig(): { rejectUnauthorized: boolean } | undefined { * until `shutdown()` runs and a subsequent call rebuilds the pool. * * Two values are intentionally re-read on every request and are NOT snapshotted - * here: `getMaxRows()` (so a one-off bump to `POSTGRES_MAX_ROWS` takes effect - * immediately, no restart) and `isWritesAllowed()` (so flipping `ALLOW_WRITES` - * on or off between tool calls is observed without a process restart -- the - * operator uses this when they want to briefly run a write, then flip back). + * here: `getMaxRows()` and `isWritesAllowed()`. The per-request re-read matters + * for in-process callers (tests set `process.env.ALLOW_WRITES` between calls) + * and keeps the flag out of the pool snapshot above. It does NOT give an + * operator a live toggle: a stdio MCP server's environment is fixed at spawn + * by the host's config, so changing `ALLOW_WRITES` (or `POSTGRES_MAX_ROWS`) + * on a running server requires the MCP host to restart it. */ export function getPool(): pg.Pool { if (pool) return pool; @@ -152,10 +154,13 @@ export interface QueryResult { // but is never read back (no result row references the dead oid), so it's // wasted memory, not a correctness bug. // -// Concurrency: the bootstrap read is unsynchronized, so with POSTGRES_POOL_MAX>1 -// two clients can both observe a null cache and each run the full pg_type -// SELECT before either assigns -- harmless duplicate work, last-writer-wins on -// the shared map. +// Concurrency: the null check and the `new Map()` assignment in +// resolveTypeNames are synchronous (no await between them), so the bootstrap +// SELECT runs at most once per cache lifetime. The actual race with +// POSTGRES_POOL_MAX>1: a second concurrent caller sees an empty-but-non-null +// map, skips the bootstrap, and runs the targeted `WHERE oid = ANY($1)` +// miss-fill for its oids -- harmless duplicate targeted work; both callers +// merge entries into the same shared Map. let typeNameCache: Map | null = null; async function resolveTypeNames(client: pg.PoolClient, oids: number[]): Promise> { diff --git a/src/tools/explain.test.ts b/src/tools/explain.test.ts index e702c3c..b67cdcf 100644 --- a/src/tools/explain.test.ts +++ b/src/tools/explain.test.ts @@ -58,10 +58,10 @@ describe("pg_explain hypothetical_indexes identifier validation", () => { hypothetical_indexes: [{ table: 'public."odd.name"', columns: ["x"], using: "btree" }], })) as { ok: boolean; error?: string }; assert.equal(result.ok, false); - // The embedded `.` inside the quoted segment splits this into 3 pieces, so - // the over-qualified guard fires first; the double-quote guard would catch - // it otherwise. Either rejection is correct -- accept both forms. - assert.match(result.error ?? "", /double-quote|pre-quoting|over-qualified/i); + // The raw-string double-quote check runs before the dot-split, so a + // pre-quoted name with an embedded dot gets the actionable "remove the + // quotes" message, not the over-qualified one. + assert.match(result.error ?? "", /double-quote|pre-quoting/i); }); it("rejects an over-qualified (3+ part) table name", async () => { diff --git a/src/tools/explain.ts b/src/tools/explain.ts index c84e4e8..f81b8b8 100644 --- a/src/tools/explain.ts +++ b/src/tools/explain.ts @@ -64,6 +64,15 @@ function quoteQualifiedTable(name: string): string { * default re-application in buildHypopgHooks. */ function validateHypoIndex(idx: { table: string; columns: string[] }): string | null { + // Check the RAW string for pre-quoting BEFORE splitting on `.`: a pre-quoted + // name with an embedded dot (`public."odd.name"`) splits into 3+ pieces, and + // the over-qualified message alone gives the caller no hint that removing + // the quotes is the fix. JSON.stringify the offending value so a name + // containing `"` renders unambiguously instead of producing a broken + // nested-quote message. + if (idx.table.includes('"')) { + return `Hypothetical index table ${JSON.stringify(idx.table)} contains a double-quote; pass plain identifier names without pre-quoting.`; + } const pieces = idx.table.split("."); // Only `schema.table` (1 or 2 pieces) is a legal dotted shape. A 3+ part // name like `a.b.c` passes the per-piece checks below but renders as @@ -73,11 +82,6 @@ function validateHypoIndex(idx: { table: string; columns: string[] }): string | return `Hypothetical index table ${JSON.stringify(idx.table)} is over-qualified; use only \`schema.table\` or \`table\`.`; } for (const piece of pieces) { - if (piece.includes('"')) { - // JSON.stringify the offending value so a name containing `"` renders - // unambiguously instead of producing a broken nested-quote message. - return `Hypothetical index table ${JSON.stringify(idx.table)} contains a double-quote; pass plain identifier names without pre-quoting.`; - } if (Buffer.byteLength(piece, "utf8") > 63) { return `Hypothetical index table piece ${JSON.stringify(piece)} exceeds PostgreSQL's 63-byte NAMEDATALEN limit (multi-byte characters count as multiple bytes).`; } diff --git a/src/tools/tools.test.ts b/src/tools/tools.test.ts index 16c0ef3..146531e 100644 --- a/src/tools/tools.test.ts +++ b/src/tools/tools.test.ts @@ -55,17 +55,18 @@ describe("Tool definitions", () => { assert.equal(typeof tool.annotations.openWorldHint, "boolean"); }); - // Schema/handler-drift guard. Every tool's inputSchema is a ZodObject; - // the handler then does `const { ... } = input as { ... }` and reads - // the destructured fields without re-validating. A regression that - // (a) drops the ZodObject wrapper, (b) leaves the schema empty when - // the handler destructures input, or (c) has a schema whose shape - // doesn't match the keys the handler reads would compile cleanly - // and only surface as `undefined` at runtime -- caught by integration - // tests, but months later and at the call site. The cheap probe: - // the schema is a ZodObject (already asserted above), the schema - // actually parses a sample input built to satisfy each field's - // type, and the parsed output exposes the shape's declared keys. + // Schema sanity guard. Every tool's inputSchema is a ZodObject; the + // handler then does `const { ... } = input as { ... }` and reads the + // destructured fields without re-validating. The cheap probe here: + // the schema is a ZodObject (already asserted above) and it actually + // parses a sample input built to satisfy each field's type -- catching + // a dropped ZodObject wrapper or an impossible constraint (a bound or + // refine no type-correct input can satisfy). It deliberately does NOT + // try to assert that the schema's keys match what the handler + // destructures: Zod's parsed output always echoes supplied declared + // keys, so a key round-trip assertion can never fail once safeParse + // succeeds, and a real handler-key check would need a per-tool fixture + // of the keys each handler reads. // // Note: a tool with `z.object({})` (no declared fields, e.g. // pg_list_schemas / pg_list_extensions / pg_replication_status) is @@ -73,7 +74,7 @@ describe("Tool definitions", () => { // and these tools take no input. The "must have at least one field" // assertion is intentionally NOT made; the test below still runs // and passes (parses {}, parsed object is {}) for those tools. - it("inputSchema parses a type-correct sample input and round-trips declared keys", () => { + it("inputSchema parses a type-correct sample input", () => { const shape = tool.inputSchema.shape as Record; const fieldNames = Object.keys(shape); @@ -134,11 +135,6 @@ describe("Tool definitions", () => { true, `${tool.name} inputSchema rejected a type-correct sample: ${result.success ? "" : result.error.message}`, ); - if (result.success) { - for (const name of fieldNames) { - assert.ok(name in result.data, `${tool.name} inputSchema parsed output missing declared field "${name}"`); - } - } }); }); }