Skip to content

Improve packaged build, skill ACL, and config compatibility#34

Merged
ZhiXiao-Lin merged 4 commits into
AI45Lab:mainfrom
Lixtt:fix/config-token-limits
May 22, 2026
Merged

Improve packaged build, skill ACL, and config compatibility#34
ZhiXiao-Lin merged 4 commits into
AI45Lab:mainfrom
Lixtt:fix/config-token-limits

Conversation

@Lixtt
Copy link
Copy Markdown
Contributor

@Lixtt Lixtt commented May 20, 2026

Summary

  • Use the published a3s-common crate so standalone Code checkouts and Python wheel builds do not require a sibling ../../common checkout.
  • Make skill allowed-tools handling compatible with omitted values, whitespace-only legacy values such as Read Write Edit Bash, and YAML list values.
  • Parse ACL model token limits from flat model fields and nested limit blocks, including snake_case and camelCase aliases.

Why

These changes make packaged installs and benchmark/task catalogs work without relying on local build patches. They preserve explicit skill allowlists while avoiding accidental deny-all behavior for skills that omit allowed-tools, and they allow long-context OpenAI-compatible model deployments to express output/context limits in ACL config.

Validation

  • cargo test -p a3s-code-core test_config_supports_acl_style_provider_labels
  • cargo test -p a3s-code-core test_parse_legacy_whitespace_allowed_tools
  • cargo test -p a3s-code-core test_parse_allowed_tools_yaml_list
  • cargo test -p a3s-code-core test_skill_permission_policy_allows_when_unspecified
  • cargo test -p a3s-code-core test_skill_permission_policy_accepts_legacy_allowed_tools
  • cargo test -p a3s-code-core test_config_parses_acl_model_token_limits

@Lixtt Lixtt marked this pull request as ready for review May 20, 2026 11:25
Copy link
Copy Markdown
Contributor

@ZhiXiao-Lin ZhiXiao-Lin left a comment

Choose a reason for hiding this comment

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

Thanks for tackling these — the underlying pain points are real. That said, I'd push back on bundling these together and on a couple of the specific approaches. Suggesting we split this into separate PRs and revisit two of the three changes.

1. a3s-common dependency (commit 3f19952)

Switching to a published version fixes packaging, but dropping the path entry breaks the monorepo dev loop — local edits to common won't be picked up by Code anymore. Cargo supports both at once:

a3s-common = { path = "../../common", version = "x.y" }

Path wins locally, version is used when published. Could we use that form instead?

2. Skill allowed-tools parsing (commit 6f93640) — needs to be split

This commit conflates two changes with very different risk profiles:

  • Accepting legacy whitespace-separated strings and YAML lists — fine in principle, but please emit a warn! for the legacy form so we don't carry both shapes forever. Without a deprecation path this becomes permanent surface area.

  • Treating an omitted allowed-tools as "allow" — this is a security-posture change, not a compatibility fix. `allowed-tools` is a permission field; fail-secure (deny when unspecified) is the correct default for a permission system. Flipping it to allow-by-default means existing skills that simply forgot to declare the field will silently gain broader privileges on upgrade, with nothing in the changelog to surface it.

    If the goal is to reduce friction for authors, I'd suggest the opposite direction: keep fail-secure, but log a clear warning at load time pointing at the skill and the migration path. Could we pull this part out into a separate PR and discuss the security implications explicitly?

3. ACL model token limits (commit 4dca502)

Supporting flat fields and nested `limit` blocks, and both snake_case and camelCase, gives us four ways to express the same thing. That's a lot of permanent config surface for one field. Could we pick one canonical shape, accept at most one alias for migration with a deprecation warning, and document the canonical form? Otherwise this tends to drift — six months from now someone files "why doesn't `maxTokens` work?" because one code path only matched `max_tokens`.

Suggested path forward

  1. Land the `a3s-common` fix on its own, using `{ path, version }` together.
  2. Land the legacy-format parsing for `allowed-tools` on its own, with a deprecation warning.
  3. Open a separate discussion issue for the "omitted = allow" change before merging it.
  4. For the ACL limits, pick one canonical form + at most one alias, and document it.

Happy to review again once these are split. Thanks again for the work.

@ZhiXiao-Lin ZhiXiao-Lin merged commit 529986f into AI45Lab:main May 22, 2026
1 check passed
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