Repository navigation
PR #62 follow-up 20: document caller gates for execute_oss_tool helpers - #69
Merged
Merged
Conversation
execute_oss_tool and execute_oss_tool_with_query_headers are public and dispatch any tool in the full OSS catalog, including alert action tools, but unlike call_tool_registry they do not check session validity, tenant suspension or the global Query permission. Spell out these obligations in the doc comments so a future in-process caller applies them. No runtime behaviour change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… fix/pr62-20-execute-oss-tool-gates
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
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.
Stacked on #62 (base
upstream/tool-catalog-1831). Retarget tomainafter #62 merges.Problem
Follow-up 20 from the #62 review.
execute_oss_toolandexecute_oss_tool_with_query_headers(insrc/tool_executor.rs, inherited from upstream 32979cb) arepub. They look up tools in the fulloss_tool_specs(), which includes the alert action toolscreate_alert,enable_alert,disable_alert,evaluate_alertandcreate_alert_target. They then validate arguments and dispatch. Unlikecall_tool_registry, they skip theUsers.session_existscheck,check_suspension_for_tenant(tenant, Action::Query), and the globalauthorize(Query, None). They also don't restrict execution to the read catalog. Each tool's ownauthorize()still runs.I confirmed this against the current code. Nothing in the fork calls these helpers, so this is a hazard for future callers, not a live bug.
Fix
Doc comments only, with no runtime change.
execute_oss_toolgains a "Caller obligations" section listing the four gates a caller must apply: session validity, tenant suspension, globalQueryauthorization, and tool restriction plus confirmation for action tools.execute_oss_tool_with_query_headerspoints back to that list.Why documentation rather than a shared gate helper:
call_tool_registryinto a shared helper is the natural next step.docs/tool-api.mdis unchanged because nothing caller-visible changed.Tests
No code changed, so I added no new tests.
cargo clippy -- -D warnings: cleancargo clippy --all-targets: no warnings insrc/tool_executor.rsRUSTDOCFLAGS="-D rustdoc::broken_intra_doc_links" cargo doc --no-deps --lib: clean, so the new intra-doc links resolvecargo test --lib tool_: 38 passed, 0 failedUpstream fidelity
The diff only adds doc comments, about 24 lines, to two upstream functions, so the risk of merge conflicts is minimal. I've drafted an equivalent docs-only upstream change but haven't filed it.
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.