Pre-publish review of the 2026-09-01 main changes: eight fixes before v3.0.0 - #143
Conversation
…gistrations setup and doctor could not finish on any machine that had Codex or Claude Code installed. Both clients report a missing registration as a failed command, and every rejection was mapped to `unknown` without reading it, so setup stopped at Step 5 and doctor said "unrecognized client entry". The Claude Code probe (`claude mcp get -s user google --json`) is rejected by every claude version with `unknown option '-s'`, so it was `unknown` everywhere. A rejection whose text says "missing" now means missing, and the Claude Code user-scope entry is read from ~/.claude.json (the file `claude mcp add -s user` writes), with its `type: 'stdio'` and empty `env` stripped so a correct entry compares equal to the desired one. doctor also reported every README-documented registration (bare bin, npx, clone path) as "entry differs from recommended configuration", because its recommended entry is the absolute path of the copy running doctor. An entry that launches this package is now `configured` with a note; Codex without CODEX_MCP_PROTOCOL_VERSION stays a problem and names the variable. Co-Authored-By: Claude Code <noreply@anthropic.com>
checkForUpdate took injectable readFile/writeFile/mkdir with no defaults, and index.js passes none, so in production every cache read threw (swallowed as "no cache") and every write threw on mkdir (swallowed as "best effort"). The registry was fetched on every launch and update-check.json never written, while every test injected fakes and passed. A real-temp-dir test now proves a second launch inside the TTL makes no network request. Co-Authored-By: Claude Code <noreply@anthropic.com>
help used Zod's bare toJSONSchema(), whose default is the output schema, so every .optional().default(x) field came back as required. For 50 of the 160 tools that contradicted tools/list. Now io:'input', draft 2020-12, pinned to the SDK's conversion for every registered tool. Co-Authored-By: Claude Code <noreply@anthropic.com>
An abandoned flow could clear a newer latch after logout (identity guard), a flow running at logout could install its client afterwards (authGeneration counter, bumped by resetClients; the stale flow throws "Logged out while authorization was in progress"), and a cold request arriving mid re-authorization opened a second consent screen (ensureAuth now joins reauthInFlight). Nine tests; removing each guard fails exactly its test. Co-Authored-By: Claude Code <noreply@anthropic.com>
… [Circular] doctor --json printed "args": "[Circular]" for the second client because the two recommended entries share one args array and `seen` never released a node. A true cycle still prints [Circular]. Co-Authored-By: Claude Code <noreply@anthropic.com>
One shared cleanup loop (scripts/live-smoke/cleanup.mjs) replaces the two copies in live-mission and live-smoke, both of which reported an already-deleted file as left behind (containment check before existence check) and crashed on a null raw Drive handle after an invalid_grant rebuild. Blocked outcomes are counted, both runners list the test folder and leftover drafts after cleanup and fail on anything this run left, --keep prints each kept id with the live-call --cleanup command, and track() dedupes ids the runner already registered. live-coverage credits ctx.createDoc()/createFolder() helpers, accepts any quote style, skips live/missions/archive/ (loop-1 and loop-2 are frozen transcripts that hard-fail on fixed code), fails on zero coverage, and derives the guard-denied set from guard.mjs (MUTATING_VERB exported; the only guard change). verify-preserve-heading probe 3 states that it expects the heading gone. live-mission's report `account` was always null. The created-resource pin test now executes all eight creating tools. Co-Authored-By: Claude Code <noreply@anthropic.com>
CHANGELOG entries for every fix on this branch; README Step 3 Codex commands now pass --env CODEX_MCP_PROTOCOL_VERSION=2026-07-28, which the breaking-change section already said Codex needs; SESSION-STATE reflects main=a957677, the merged #140, this pass, and the current gate numbers; inventory snapshot regenerated. Co-Authored-By: Claude Code <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d6ad46d19
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ElliotDrel
left a comment
ElliotDrel
left a comment
There was a problem hiding this comment.
Adversarial Review — issue
The draft cleanup audit also fails open on any getDraft error. listLeftoverDrafts() treats every exception as proof that the draft is gone, but getDraft can fail for auth, network, quota, or malformed-response reasons while the draft still exists. Both runners then see an empty leftoverDrafts array and can exit successfully after an unverifiable cleanup.
Failure scenario: cleanup deletes a draft, then the verification getDraft call hits a transient 401/429/5xx or transport failure. The catch block silently classifies that as “gone,” so a draft that survived cleanup is indistinguishable from a real 404.
Smallest fix: only treat a confirmed not-found response as deleted. Propagate or return an explicit verification failure for every other error, and make both runners fail closed on that state. Add a test where getDraft throws a non-404 error and assert the run cannot report cleanup success.
…ries in doctor Review findings on #143, all four confirmed: - listLeftovers returned an empty array when the listing threw, read a truncated listing as complete, and only looked one level deep. It now scans the whole tree (depth 'all', up to 5000 items) and reports UNVERIFIED with the reason when the listing failed, was cut short, or skipped an unreadable folder. Both runners fail on that state. - listLeftoverDrafts treated every getDraft error as "deleted". Only an error carrying a real 404 status counts as gone; anything else is reported per draft in `unverified` and fails the run. - launchesThisPackage accepted `google-tools-mcp doctor`, `npx google-tools-mcp setup`, and `dist/index.js auth` as server launches. dist/index.js dispatches on argv[2], so anything after the target is a subcommand, not the server. Env entries selecting an HTTP transport are rejected too. tests/liveHarnessCleanupAudit.test.js covers each state, including the non-404 getDraft case, and asserts the runner predicate cannot report success on an unverified audit. Live harness-selftest re-run: 3/3 trashed, sandbox 0 item(s) after cleanup, exit 0. Co-Authored-By: Claude Code <noreply@anthropic.com>
|
Addressed in 66d4e18. Tests in |
1 participant
Pre-publish review of everything merged to
mainon 2026-09-01 (06f4ef0..a957677: #139, 73ca178, #140), with fixes for what the review found. Seven commits, one finding each, every fix has a test that was mutation-checked.Findings fixed
setupanddoctorcould not finish on any machine with Codex or Claude Code installed. A "not found" CLI failure was mapped tounknown, and the Claude Code probe used-s user --json, which noclaudeversion accepts. CI never saw it because CI has neither client.doctorreported every README-documented registration (bare bin,npx -y google-tools-mcp, clone path) as "entry differs from recommended configuration", exit 1. Nowconfigured+ note when the entry launches this package; Codex withoutCODEX_MCP_PROTOCOL_VERSIONstays a problem and names it.readFile/writeFile/mkdirhad no defaults, so the cache never read or wrote.help tool=Xrendered the output-io JSON Schema; 50 of 160 tools showed.default()fields as required.invalid_grant, blocked outcomes uncounted, no post-cleanup listing,--keepprinted no ids,track()double-counted. Plus frozen missions archived, false friction in probe 3, coverage scanner gaps, hand-maintained guard-denied list.doctor --jsonprinted"args": "[Circular]"for any shared reference.--env CODEX_MCP_PROTOCOL_VERSION=2026-07-28. CHANGELOG entries for all of the above. SESSION-STATE rewritten (it said main=73ca178 and #140 open).Evaluated and left alone: Sheets read-state seeding in
createSpreadsheet/copyFileis request-scoped on HTTP. Neither tool claims the Sheet is mutable afterwards and the guard's HTTP branch already says Sheets edits are stdio-only.Gates at the head of this branch
npm run test:ci: 94 suites, 1381 passed, 2 skippednpm audit --omit=dev: 0npm pack --dry-run: 184 files, no tests/live/scripts leakagedist/**/*.jsimports (181 modules)npm run live-coverage: 29 / 131 / 2, exit 0harness-selftest3/3,verify-created-resource-tracking2/2,agent-loop-2-fixes19 calls 5/5,live-smoke22/22 with 28/28 trashed and an empty folder after.--keepandlive-call --cleanupexercised.Still open before the tag
npm-publishenvironmentprotection_rulesis[]. Repo settings, Elliot only.2026-08-31) should be set to the publish day.🤖 Generated with Claude Code