docs: update mcp docs for IHalClient pivot, scoped HalToolCollectionManager, and gap resolutions#82
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>
![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: c5b5097d6d
ℹ️ 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".
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>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 617602bcd7
ℹ️ 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".
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>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3bf4a8a263
ℹ️ 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".
…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>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9fa41ad34
ℹ️ 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".
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>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0bd3a1bcc
ℹ️ 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".
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>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9010fb790e
ℹ️ 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".
…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>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70d006aebb
ℹ️ 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".
…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
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a96b4aa58
ℹ️ 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".
… example Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6db5a314e0
ℹ️ 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".
…hrefs with URL query params Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7974655682
ℹ️ 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".
…l.md Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4a3cabd91
ℹ️ 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".
…raphql.md Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c14f898d9
ℹ️ 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".
…tch to run-codex-review-loop step 2 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f07e2afe7
ℹ️ 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".
…phql.md Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28a049e2b6
ℹ️ 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".
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53c9408932
ℹ️ 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".
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 811757a4ee
ℹ️ 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".
…eview-graphql.md Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…design fixes (#84) * docs: update mcp docs for IHalClient pivot, scoped HalToolCollectionManager, and gap resolutions Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs: fix remaining stale GetHalAsync and HttpClient references in mcp 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> * docs: fix Codex review findings in mcp requirements and architecture 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> * docs: fix case normalization order in tool-name sanitization algorithm 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> * docs: fix relative URI handling and NavigateToRootTool DI contradiction 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> * docs: enforce ScopeRequests=true in WithHalApi registration (REQ-46) 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> * docs: fix IHalClient DI registration in Integration Story example 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> * docs: add query-strip, single-session constraint, and empty-LinkObjects 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> * docs: add HalClientResult envelope and GetResultAsync to client/MCP docs 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> * docs: add counter-based tool name collision dedup to mcp requirements 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> * update agent framework * update agent framework * docs: fix HalClientResult type ambiguity and enforce href sanitization 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> * docs: chain .AddHalOptions() to IHalClient registration in MCP integration 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> * docs: fix AddHalOptions missing configure delegate in MCP integration example Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs: anchor REQ-05 scheme-strip to string start to protect relative hrefs with URL query params Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs: add pageInfo to comments connections in github-pr-review-graphql.md Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs: add pageInfo to reviewThreads connections in github-pr-review-graphql.md Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs: add after cursor args to GraphQL queries; add review summary fetch to run-codex-review-loop step 2 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs: add per-thread comment pagination query to github-pr-review-graphql.md Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs: add review summary fetch to watch-pr-feedback detection path Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs: add Fetch Reviews query to github-pr-review-graphql.md Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs: make REQ-05 scheme-strip regex case-insensitive Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs: fix per-thread comment pagination cursor wording in github-pr-review-graphql.md Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs(mcp): add SwapTools tuple return and NavigateToRootTool InvokeAsync 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 * docs(mcp): fix McpServerOptions resolution and removedCount calculation 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. * docs(mcp): fix addedCount hardcoded subtraction in SwapTools algorithm Use navigateToRoot.Count instead of constant 1 for addedCount, matching the same fix applied to removedCount. Addresses Codex P2 on PR #84. --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
1 participant
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com