Repository navigation
Conversation
…to claude/project-thread-4ocdp6
…to claude/project-thread-4ocdp6
…to claude/project-thread-4ocdp6
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jke6dnq5CNkrDFWbJSHJWr
…ets failed to load Mirrors FastifyTransport: a not-ready bundle gets a 500 / -32603 on initialize and is dropped from the cache. Docs updated for 0.8.0. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jke6dnq5CNkrDFWbJSHJWr
TS review:
|
| # | Sev | Location | Finding |
|---|---|---|---|
| 1 | HIGH | CHANGELOG.md [0.8.0] Breaking, first bullet |
Says createPermissionBasedMcpServer clients with failing startup toolsets now get -32603 / HTTP 500. False: the permission orchestrator is built with toolsets: [], createPermissionAwareBundle enables toolsets itself, partial failure still connects (failedToolsets), total failure is 403, so isReady() never goes false there. |
| 2 | MEDIUM | CHANGELOG.md [0.8.0] Fixed (#30) |
Describes PermissionAwareFastifyTransport, which is not exported from src/index.ts, racing on background preload. Unreachable via the public factory, which awaits enableToolsets before returning. |
| 3 | MEDIUM | src/permissions/AGENTS.md flow; root AGENTS.md invariant 4 |
Imply the isReady / 500 path covers permission clients, and contradict "partial success continues with warning". |
| 4 | MEDIUM | tests/e2e/permissionBased.e2e.test.ts (new describes) |
Build the transport directly with hand-made bundles, so they pass while the public path is unchanged and hide finding 1. Label as transport-level, and pin the real behaviour through createPermissionBasedMcpServer. |
| 5 | LOW | README.md startup note |
"activates with no tools" overstates it; other modules in the toolset still register ("fewer tools"). |
{
"recommendation": "REQUEST_CHANGES",
"summary": "Version bump and most changelog/doc claims match the code, but the Breaking entry (and related docs/tests) claim createPermissionBasedMcpServer clients now get a 500/-32603 on failed startup toolsets, which cannot happen because that factory enables toolsets itself, tolerates partial failure, and leaves orchestrator.isReady() always true.",
"findings": [
{ "severity": "HIGH", "file": "CHANGELOG.md", "line": null, "issue": "Breaking entry claims permission-based clients get -32603/500 on failed startup toolsets; false for the public factory." },
{ "severity": "MEDIUM", "file": "CHANGELOG.md", "line": null, "issue": "Fixed entry for #30 describes an unexported class racing on preload; unreachable via createPermissionBasedMcpServer." },
{ "severity": "MEDIUM", "file": "src/permissions/AGENTS.md", "line": null, "issue": "Permission Flow and root invariant 4 imply the isReady/500 path covers permission clients and contradict partial-success behaviour." },
{ "severity": "MEDIUM", "file": "tests/e2e/permissionBased.e2e.test.ts", "line": null, "issue": "New e2e tests bypass the public factory and mask the changelog inaccuracy." },
{ "severity": "LOW", "file": "README.md", "line": null, "issue": "'activates with no tools' vs 'fewer tools' elsewhere." }
]
}Generated by Claude Code
createPermissionBasedMcpServer enables toolsets in its bundle factory and continues on partial failure, so the isReady() rejection does not apply. Pin that with a public-factory test and label the transport-level tests. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jke6dnq5CNkrDFWbJSHJWr
Fixes for the round 1 TS reviewAll five findings were correct (I confirmed the permission flow in
The guard itself stays: it is a small mirror of Generated by Claude Code |
…ening Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jke6dnq5CNkrDFWbJSHJWr
TS review:
|
| # | Sev | Location | Finding | Fix (1c58bb8) |
|---|---|---|---|---|
| 1 | LOW | CHANGELOG.md Fixed (#30) |
"hand-built createPermissionAwareBundle callback" implies a path consumers have; the class and helper are not exported. |
Reworded as internal hardening: the class is not public API and createPermissionBasedMcpServer is unchanged. |
| 2 | LOW | permissionBased.e2e.test.ts "all failed" |
Bare rejects.toThrow() passes on any error. |
Now asserts /Access denied/ (the 403 body). |
| 3 | LOW | permissionBased.e2e.test.ts partial failure |
Did not assert the failed toolset's tool is absent. | Now asserts dup.ping is not listed, which also pins the #31 rollback on the public path. |
{"recommendation":"COMMENT","summary":"All five round-1 findings fixed correctly and truthfully; only LOW polish remains.","findings":[{"severity":"LOW","file":"CHANGELOG.md","line":null,"issue":"#30 entry implies a user-reachable path; reword as internal hardening."},{"severity":"LOW","file":"tests/e2e/permissionBased.e2e.test.ts","line":null,"issue":"bare rejects.toThrow() in the all-failed test."},{"severity":"LOW","file":"tests/e2e/permissionBased.e2e.test.ts","line":null,"issue":"partial-failure test does not assert the failed toolset's tool is absent."}]}All three are addressed in 1c58bb8 (typecheck clean, 395 tests pass). Round 3 follows to confirm APPROVE.
Generated by Claude Code
TS review:
|
…-4ocdp6 # Conflicts: # CHANGELOG.md
TS review:
|
Requested by Ben · project thread
Before:
mainis at 0.7.0, and the finished changes for 0.8.0 (#29 to #33) have no release entry.After: version 0.8.0 with one changelog entry covering all five, plus one small addition:
PermissionAwareFastifyTransportnow rejects a client whose startup toolsets failed to load, the follow-up #33 and #30 asked for. Docs match the new behaviour. Nothing is tagged or published.Merge order (read first)
Base is
main. #29, #30, #32 and #33 have merged. #31 is still open, so this draft branch contains it merged in. Merge #31 first; this PR's diff then shrinks to the release commits and merges cleanly on top. Do not rebase or force-push the PR branches.enable_toolset/disable_toolsetadvertisedestructiveHint: falseconnect()disable_toolsetremoves the toolset's toolshttp.allowedOriginsOrigin check on/mcpinitializeand STATIC startup (Breaking)This release ships #33 as written (Strict semantics).
What the release adds beyond the five PRs
package.json/package-lock.json0.7.0 to 0.8.0, andCHANGELOG.md[0.8.0]replacing[Unreleased], with Breaking, Added, Changed and Fixed sections and migration notes covering fix: mark enable_toolset and disable_toolset as non-destructive #29 to fix: reject STATIC clients whose startup toolsets failed to load #33.PermissionAwareFastifyTransport:isReady()false gives 500 / JSON-RPC-32603and drops the cached bundle, asFastifyTransportdoes. This is internal hardening: the class is not public API, andcreatePermissionBasedMcpServer(which enables toolsets itself, starts its orchestrators with no toolsets and continues on partial failure) is unchanged. Two transport-level e2e tests fail without it, and two tests through the public factory pin the real behaviour.AGENTS.mdinvariant 4 (loader errors stay silent, registration failures no longer do) andsrc/permissions/AGENTS.md.allowedOrigins, destructive hints and disable-removes-tools were already documented by their PRs.Verification
npm run typecheck,npm run test:run(395 tests) andnpm run buildpass.npm publish --dry-run: toolception@0.8.0, 64 files, 107 kB; onlydist/,package.json,README.md,CHANGELOG.mdandLICENSE; nosrc/, tests or env files.npm run verify:toolceptionin the FMP server (main, 2.7.0) against the packed tarball: 19/21. Both failures are expected and are server work that must merge first:ALL_TOOLSdoes not start: thecalendarandearningstoolsets both list theearnings-transcriptmodule, sogetLatestEarningsTranscriptscollides (src/constants/toolSets.ts). The server needs a catalog fix, being done in a separate project thread.should disable toolset (state only, tools remain)asserts the old disable behaviour. Fixed by server #113. (The same overlap breaks the legacy/all-tools smoke tests and the installation checks.)pipeline-ts-reviewer): APPROVE in round 3; findings and fix mappings are in the comments below.Server prerequisites before the FMP server takes 0.8.0
earnings-transcriptmodule from one ofcalendar/earnings.npm run verify:toolception -- --source 0.8.0must be green, then the dependency bump PR, with the docs that say disable is "state tracking only" (docs/USAGE.md,src/endpoints/server-card.ts) changed together with it.Release steps (Ben)
After #31 and this PR are merged: tag
v0.8.0on main HEAD (release.yml publishes to npm and creates the GitHub Release). The command is in the project thread.🤖 Generated with Claude Code
https://claude.ai/code/session_01Jke6dnq5CNkrDFWbJSHJWr