Skip to content

fix(core): relax active skill tool restrictions by default#72

Merged
ZhiXiao-Lin merged 2 commits into
AI45Lab:mainfrom
Aspdelus:codex/fix-active-skill-tool-locking
Jun 23, 2026
Merged

fix(core): relax active skill tool restrictions by default#72
ZhiXiao-Lin merged 2 commits into
AI45Lab:mainfrom
Aspdelus:codex/fix-active-skill-tool-locking

Conversation

@Aspdelus

Copy link
Copy Markdown
Contributor

Summary

  • Stop active skill allowed-tools from globally denying ordinary session tool calls by default.
  • Add enforceActiveSkillToolRestrictions / with_active_skill_tool_restrictions(true) as a compatibility switch for legacy behavior.
  • Keep skill-local allowed-tools enforced inside Skill child execution contexts.
  • Treat allowed-tools: "*" as a skill grant wildcard without bypassing permission policy, hooks, HITL, or AHP.

Tests

  • cargo fmt --all -- --check
  • rustfmt --edition 2021 --check sdk/node/src/lib.rs sdk/python/src/lib.rs
  • git diff --check
  • cargo test -p a3s-code-core
  • cargo test --manifest-path sdk/node/Cargo.toml
  • cargo test --manifest-path sdk/python/Cargo.toml

@ZhiXiao-Lin

Copy link
Copy Markdown
Contributor

Thanks for this — the design itself is sound. Reframing an active skill's allowed-tools from a global session lockdown into a scope on the skill's own execution is a cleaner, more composable model: the permission policy / hooks / HITL / AHP gates stay authoritative (the permission Deny still denies test confirms the relaxation doesn't bypass them), restrictions remain enforced inside the skill's child context, and the opt-in enforce_active_skill_tool_restrictions / with_active_skill_tool_restrictions(true) flag is a good migration path. Test coverage is solid.

That said, I'd hold off on merging it as-is, for a few reasons:

1. Stale branch + no green CI signal. This PR predates the v4.0.0 release. mergeStateStatus is UNSTABLE and there are no reported checks, so there's no passing build/test signal against the current main.

2. It overlaps files that changed in v4.0.0. It touches core/src/agent_api.rs, core/src/agent_api/session_persistence.rs, core/src/store/session_data.rs, and sdk/node/generated.d.ts — all of which moved in v4.0.0. Even though GitHub currently reports MERGEABLE (no textual conflict), it likely won't compile/test-match current main without a rebase. Specifically:

  • sdk/node/generated.d.ts is generated by napi build (not hand-edited) and must be regenerated from the rebased Rust; the SDK manifests on this branch also predate the 4.0.0 version bump.
  • core/src/store/session_data.rs adds a persisted field — please confirm the saved-session snapshot stays backward-compatible.

3. It's a security-relevant default change. Flipping active-skill tool restrictions off by default is a deliberate behavior change (anyone relying on "activate skill ⇒ lock the session to its allowed-tools" loses that default). It should get explicit maintainer sign-off plus a CHANGELOG entry — ideally landing in a 4.1.0 minor, since v4.0.0 just shipped.

Suggested path to merge: rebase onto current main (v4.0.0) → regenerate generated.d.ts → get cargo fmt --all -- --check, cargo clippy --workspace -- -D warnings, and cargo test -p a3s-code-core (+ the Node/Python SDK test suites) green → add a CHANGELOG note → then merge as 4.1.0.

@Aspdelus Aspdelus force-pushed the codex/fix-active-skill-tool-locking branch from 5b41c71 to 8c8a29f Compare June 22, 2026 07:04
@Aspdelus

Copy link
Copy Markdown
Contributor Author

Updated after review.

Changes in this update:

  • Rebased the PR branch onto current main (v4.0.0 lineage).
  • Re-ran npm run build in sdk/node to regenerate generated.d.ts; the generated file had no diff on current main.
  • Added a 4.1.0 - Unreleased CHANGELOG entry for the security-relevant default semantic change.
  • Ran rustfmt over the Node/Python binding entry files so SDK binding fmt checks are clean.

Validation run locally:

  • cargo fmt --all -- --check
  • rustfmt --edition 2021 --check sdk/node/src/lib.rs sdk/python/src/lib.rs
  • cargo clippy --workspace -- -D warnings
  • cargo test -p a3s-code-core (1784 passed, 6 ignored in core lib tests; integration/doctests passed)
  • cargo test --manifest-path sdk/node/Cargo.toml (29 passed)
  • cargo test --manifest-path sdk/python/Cargo.toml (25 passed)
  • npm run build, npm run test:types, npm run test:helpers, npm test in sdk/node
  • Python SDK smoke scripts after maturin develop in a temp venv: budget guard, parallel budget, subagent query API, session close, serve, confirmation inheritance, and real_config_env SDK with real LLM send/delegate disabled.

GitHub currently reports no checks configured for this PR branch.

@ZhiXiao-Lin ZhiXiao-Lin merged commit 1353f15 into AI45Lab:main Jun 23, 2026
ZhiXiao-Lin added a commit that referenced this pull request Jun 23, 2026
Release the active-skill tool-restriction relaxation (#72). Bumps all packages
4.0.0 -> 4.1.0 (core, node/python SDKs, bootstrap, lockfiles) and dates the
CHANGELOG entry. Verified by scripts/check_release_versions.sh 4.1.0.

Co-authored-by: Claude <claude@anthropic.com>
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