docs(mcp): MCP gap analysis — IHalClient pivot, SwapTools tuple, and design fixes#84
Merged
Conversation
…anager, and gap resolutions Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…p docs Replace leftover GetHalAsync references with IHalClient.GetAsync in REQ-38, Non-HAL response scenario, CancellationToken threading, and async conventions test bullets. Update null-response error message to match the canonical format. Update Key Points bullet to reflect that IHalClient wraps the registered HttpClient internally. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fix three issues found during Codex review of PR #82: - REQ-05 truncation formula now subtracts 2 for the "__" separator, with a prefixBudget <= 0 guard in the architecture pseudocode - Sanitization step 1 now strips scheme+:// only, retaining the authority (host+port) consistent with the examples - REQ-24 clarifies HalToolCollectionManager uses ILoggerFactory for per-tool loggers only; swap logging is at call site Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Move the lowercase step before the [^a-z0-9] replacement in both requirements.md (REQ-05) and architecture.md (Sanitize pseudocode). Uppercase letters were being replaced with underscores instead of normalized to lowercase because the character class [a-z0-9] does not match A-Z. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…on in mcp docs Replace new Uri(x) with new Uri(x, UriKind.RelativeOrAbsolute) in all pseudocode call sites to support relative hrefs from HAL APIs. Remove ILogger<NavigateToRootTool> constructor parameter since DI container is not built during WithHalApi registration; logger is now resolved from RequestContext.Services inside InvokeAsync. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add REQ-46 requiring WithHalApi to explicitly set McpServerOptions.ScopeRequests = true, ensuring per-invocation scoped DI regardless of host defaults. Update architecture.md registration sequence (renumbered) and SDK Alignment bullet from "default" to "enforced". Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The code sample used a named HttpClient registration that never registered IHalClient, causing InvalidOperationException at runtime when tools resolve IHalClient from RequestContext.Services (REQ-40). Switch to AddHttpClient<IHalClient, HalClient>(...) and update the Key Points bullet to document the typed-client pattern. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ts guard to mcp docs Fix 1: Insert query-string/fragment stripping as sanitization step 2 (after scheme-strip) in REQ-05 and Sanitize pseudocode to prevent sensitive query parameters from leaking into MCP tool names. Update REQ-36 to require query/fragment stripping before href logging. Add `/orders?page=2` example to both naming tables. Fix 2: Add concurrent-navigation-session constraint to Out of Scope (requirements) and SwapTools Key behaviors (architecture). v1 is single-session only; the global tool collection is not safe for concurrent agents. Fix 3: Guard against empty/null LinkObjects in SwapTools step 4 to avoid IndexOutOfRangeException on rels with zero link objects. Document skip behavior in REQ-04. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Introduce HalClientResult (REQ-01a) and GetResultAsync (REQ-03a) to client requirements. Update MCP requirements and architecture to use GetResultAsync, enabling tools to report HTTP status codes in error messages and distinguish 404 from non-HAL responses. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… and architecture Tool name collisions are easy to produce (punctuation variants, truncation, CURIE-like rels). Add deterministic _2, _3, ... suffix dedup in SwapTools with seenNames set, update REQ-05 collision rule, collision examples table, HalNavigationTool _name override field, ProtocolTool.Name fallback, Key behaviors bullet, and test strategy. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…n in log call sites Commit sealed class for HalClientResult (REQ-01a) and propagate sanitized href convention through MCP architecture pseudocode and log template tables to close the gap between REQ-36 and REQ-28-32 log message templates. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ation story HalClient requires HalClientOptions via constructor injection (client REQ-01). Without .AddHalOptions(), IHalClient activation fails at runtime. Updated the Integration Story code block and key points bullet to show the required call from Chatter.Rest.Hal.Client.DependencyInjection. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
# Conflicts: # .claude/skills/run-codex-review-loop/github-pr-review-graphql.md
… example Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…hrefs with URL query params Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…l.md Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…raphql.md Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tch to run-codex-review-loop step 2 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…phql.md Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…eview-graphql.md Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ync flow - SwapTools returns (int removed, int added) instead of void; counts are computed inside the lock, eliminating post-lock ToolCollection reads and the associated race condition (F1) and count inaccuracy (F2) - Add ## InvokeAsync Flow (NavigateToRootTool) pseudocode section matching the existing HalNavigationTool section - Update HalNavigationTool InvokeAsync flow and call-site logging pattern to use tuple destructure - Update REQ-42 to document SwapTools return type semantics
![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: 211b8287d8
ℹ️ 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".
Addresses Codex P1 and P2 findings on PR #84. - P1: Resolve McpServerOptions from request.Services in HalNavigationTool InvokeAsync step 1; replace _mcpOptions.ToolCollection with mcpOptions.ToolCollection in trace-loop (step 6b). Update prose and call-site logging pattern comment to match. - P2: Replace collection.Count - 1 with collection.Count - navigateToRoot.Count in SwapTools algorithm; update key behaviors bullet to describe removedCount semantics accurately.
# Conflicts: # .claude/agents/coder.md # .claude/agents/designer.md # .claude/agents/orchestrator.md # .claude/agents/planner.md # .claude/skills/address-pr-feedback/SKILL.md # .claude/skills/checkpoint-commit/SKILL.md # .claude/skills/create-working-branch/SKILL.md # .claude/skills/open-plan-pr/SKILL.md # .claude/skills/request-codex-review/SKILL.md # .claude/skills/run-codex-review-loop/SKILL.md # .claude/skills/watch-pr-feedback/SKILL.md # CLAUDE.md # agent-system-policy.md # branching-pr-workflow.md # docs/mcp/architecture.md # docs/mcp/requirements.md # pr-review-remediation-loop.md # versioning.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90b8d0103a
ℹ️ 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".
Use navigateToRoot.Count instead of constant 1 for addedCount, matching the same fix applied to removedCount. Addresses Codex P2 on PR #84.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
HttpClientextension methods toIHalClient/HalClientResultenvelope (prerequisite IDEA-04 interface)HalClientResult { Resource?, StatusCode, ReasonPhrase }andGetResultAsynccontract to client and MCP docsHalToolCollectionManager,IHalToolCollectionManagerinterface,ScopeRequests = trueenforcement (REQ-46), and tool-collection singleton constraintsAddHalOptionsDI registration,IHalClientDI wiring, relative URI handling, and NavigateToRootTool DI contradictionSwapToolsreturn type fromvoidto(int removed, int added); counts computed inside lock, eliminating post-lockToolCollection.Countreads and associated race (F1) and inaccuracy (F2)## InvokeAsync Flow (NavigateToRootTool)pseudocode section matching the existingHalNavigationToolsectionHalNavigationToolInvokeAsync flow and call-site logging pattern to use tuple destructureSwapToolsreturn type semanticsTest plan
#invokeasync-flow-navigatetoroottool)SwapToolstuple return is consistent across interface, impl declaration, algorithm pseudocode, both call sites, and logging patternNavigateToRootToolInvokeAsync section matches inline steps in the type section