Improve packaged build, skill ACL, and config compatibility#34
Conversation
ZhiXiao-Lin
left a comment
There was a problem hiding this comment.
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-toolsas "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
- Land the `a3s-common` fix on its own, using `{ path, version }` together.
- Land the legacy-format parsing for `allowed-tools` on its own, with a deprecation warning.
- Open a separate discussion issue for the "omitted = allow" change before merging it.
- 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.
Summary
a3s-commoncrate so standaloneCodecheckouts and Python wheel builds do not require a sibling../../commoncheckout.allowed-toolshandling compatible with omitted values, whitespace-only legacy values such asRead Write Edit Bash, and YAML list values.limitblocks, 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_labelscargo test -p a3s-code-core test_parse_legacy_whitespace_allowed_toolscargo test -p a3s-code-core test_parse_allowed_tools_yaml_listcargo test -p a3s-code-core test_skill_permission_policy_allows_when_unspecifiedcargo test -p a3s-code-core test_skill_permission_policy_accepts_legacy_allowed_toolscargo test -p a3s-code-core test_config_parses_acl_model_token_limits