Skip to content

fix(doc): replace getSuggestedRules with generationSuggestions in ToolBase - #3238

Open
Alanyaeer wants to merge 4 commits into
agentscope-ai:mainfrom
Alanyaeer:fix/doc-permission
Open

Alanyaeer wants to merge 4 commits into
agentscope-ai:mainfrom
Alanyaeer:fix/doc-permission

Conversation

@Alanyaeer

Copy link
Copy Markdown
Contributor

Method getSuggestedRules does not exist in ToolBase, use generationSuggestions() instead.
image

@oss-maintainer oss-maintainer left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Docs-only fix for a non-existent API in the permission-system examples: toolCall.getSuggestedRules() is replaced with tool.generateSuggestions(toolCall.getInput()). Verified against mainToolUseBlock indeed has no getSuggestedRules(), while ToolBase.generateSuggestions(Map<String, Object>) is public and is exactly what PermissionEngine.checkPermission uses to build suggested rules (PermissionDecision.withSuggestedRules(...)), and toolCall.getInput() is Map<String, Object>, so the type lines up with ConfirmResult(boolean, ToolUseBlock, List<PermissionRule>). Good catch, and both the en/zh copies stay in sync. CI is green so far (Check License, Check Module Sync, validate pass; build jobs still queued); mergeStateStatus=BLOCKED is required-review/check driven, not a conflict (mergeable=MERGEABLE), and CLA is signed.

Findings

  • [Warning] docs/v2/en/.../permission-system.md:211 (and zh:211) — tool is undefined in the snippet, so the example is still not directly runnable; worth a one-line declaration/comment.
  • [Warning] docs/v2/en/.../permission-system.md:210 (and zh:210) — the comment above the snippet still claims suggested rules sit "on the ToolUseBlock"; they live on PermissionDecision.

Neither blocks the fix — the API rename itself is correct. Approving with these two optional polish points.

Suggestions

If you extend the snippet, permission-system.md currently has no other place where a ToolBase instance is bound, so an explicit comment (see inline) is enough; no need to grow the example further.


Automated review by github-manager-bot

@@ -209,7 +209,7 @@ ConfirmResult result =
new ConfirmResult(
/* confirmed = */ true,
/* toolCall = */ toolCall,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Warning] tool is not defined in this snippet — the surrounding block only introduces toolCall, so the example is not copy-pasteable as it stands. Declaring the sender explicitly fixes it, and since generateSuggestions(Map<String, Object>) is a public ToolBase hook it also documents where the suggestions come from:

// `tool` is the ToolBase instance that received the ASK (the tool named in the pending call)
ConfirmResult result =
        new ConfirmResult(
                /* confirmed = */ true,
                /* toolCall  = */ toolCall,
                /* rules     = */ tool.generateSuggestions(toolCall.getInput()));

Same wording applies to the zh file.

@@ -209,7 +209,7 @@ ConfirmResult result =
new ConfirmResult(
/* confirmed = */ true,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Warning] The preceding comment is now stale: suggested rules are carried on PermissionDecision (PermissionDecision.getSuggestedRules(), attached by PermissionEngine.checkPermission via withSuggestedRules(...)), not on the ToolUseBlockToolUseBlock has no such field, which is exactly why the old toolCall.getSuggestedRules() could not compile. Suggested rewrite:

// ASK decisions carry suggestedRules on the PermissionDecision, but they are not
// re-exposed on the pending ToolUseBlock — regenerate them from the tool itself:

The zh version ("位于 ToolUseBlock 上") needs the same correction.

@codecov

codecov Bot commented Sep 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@oss-maintainer oss-maintainer left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Follow-up on the new commit b43cdab: the tool binding note you added (lines 207-208) does close my earlier point about the snippet having no tool in scope — thanks for that. The remaining issue is one level up: both copies still tell the reader that ASK decisions carry suggested rules on the ToolUseBlock, which is exactly the misconception that produced the original toolCall.getSuggestedRules() call this PR removes. The API rename is correct and CI/CLA look fine.

Findings

  • [Warning] docs/v2/en/.../permission-system.md:206 + docs/v2/zh/.../permission-system.md:206 — the "on the ToolUseBlock" comment is still wrong; suggested rules live on PermissionDecision.
  • [Info] docs/v2/zh/.../permission-system.md:201 — the prose refers to ConfirmResult.acceptedRules, but there is no such field/getter: ConfirmResult exposes the rules via its rules constructor parameter / getRules(). Same class of non-existent-API reference as the one being fixed here, and en:201 ("Pass the accepted rules in ConfirmResult") is already correct, so the two copies also disagree with each other.

Suggestions

One-line wording fixes in both languages, e.g.:

// ASK 决策(PermissionDecision)会带上基于本次调用自动生成的 suggestedRules。
// 接受建议时,把它们放进 ConfirmResult 的 rules 即可。

Not blocking the rename itself — tool.generateSuggestions(toolCall.getInput()) is the right call and matches how PermissionEngine builds the suggestion list.

@@ -204,12 +204,13 @@ PermissionContextState permCtx =
import io.agentscope.core.event.ConfirmResult;

// ASK decisions carry suggestedRules on the ToolUseBlock.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Warning] This sentence is still the stale claim from before the rename: suggested rules are not carried "on the ToolUseBlock" — ToolUseBlock has no such field (id / name / input / content / metadata / state only). They are attached to the PermissionDecision by PermissionEngine.checkPermission via askDecision.withSuggestedRules(tool.generateSuggestions(input)), and read back with PermissionDecision.getSuggestedRules(). Since the whole point of this snippet is now "re-derive the suggestions from the tool", the wording should match, e.g. // ASK decisions carry suggestedRules on the PermissionDecision; or simply // Suggested rules are auto-generated per invocation by the tool:. Same fix needed at docs/v2/zh/.../permission-system.md:206.

@@ -204,12 +204,13 @@ PermissionContextState permCtx =
import io.agentscope.core.event.ConfirmResult;

// ASK 决策中包含基于本次调用生成的 suggestedRules(位于 ToolUseBlock 上)。

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Warning] 同上:中文文案里的 “(位于 ToolUseBlock 上)” 也和代码不一致。suggestedRules 只存在于 PermissionDecisionPermissionDecision.getSuggestedRules()),由 PermissionEngine.checkPermission() 通过 withSuggestedRules(tool.generateSuggestions(input)) 挂载;ToolUseBlock 本身没有这个字段。建议改成 “ASK 决策(PermissionDecision)中包含基于本次调用生成的 suggestedRules”。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants