fix(analyse): select the chained ABCI skill deterministically + release 0.21.28 - #2543
Merged
Merged
Conversation
`_get_chained_abci_skill` iterated `AgentConfig.skills`, which is a
`Set[PublicId]`, and returned the first skill that was non-abstract and
declared `abstract_round_abci` as a dependency. When an agent ships more
than one qualifying skill, set iteration order decided the winner, so
`autonomy analyse service` returned a different answer run to run
depending on `PYTHONHASHSEED`.
On valory-xyz/trader both `trader_abci` and `funds_manager` qualify --
each is `is_abstract: false` and each declares `abstract_round_abci`:
PYTHONHASHSEED 0,1 -> funds_manager
PYTHONHASHSEED 2,3 -> trader_abci
That coin flip is why trader carries `analyse-service` commented out of
its `common_checks` workflow with the note that it "is checking funds
manager skill which is not an ABCI skill" -- a description of one face
of the coin, not a stable property.
Selection now collects every qualifying candidate and picks the one that
nothing else in the agent declares as a dependency: the root of the
dependency DAG, which is what a chained app is. `trader_abci` lists
`funds_manager` among its `skills`, so it is the unique root. Where
there is no single root, the command now raises a named `ClickException`
listing the candidates instead of silently picking one.
The replaced code carried a comment recording the assumption it broke:
"makes an assumption skills other than the chained/main abci are
defined as abstract".
The regression test's fixtures are named `aaa_dependency` and
`zzz_chained` so that the dependency sorts *before* the root, mirroring
`funds_manager` before `trader_abci`. A `sorted()`-only fix would pass a
test whose names sort the other way while still picking the wrong skill
in trader -- consistently, which is worse than the coin flip because it
looks stable.
Dependency edges are collected from every skill the agent overrides,
not just from the candidates. Reading them off the candidates alone
misses composition through an abstract intermediate: for `A -> B -> C`
with `B` abstract, the `B -> C` edge is invisible, so `A` and `C` both
look like roots and a chain with one obvious root is rejected as
ambiguous.
Candidates are keyed on the versioned identifier so two versions of one
skill remain two candidates and are reported, rather than one silently
overwriting the other. The edge comparison itself is version-
insensitive, so a dependency pinned to a different version than the
agent's skill list still registers as the same skill; a version-
sensitive comparison would see no edge and abort as ambiguous, which is
a worse failure than the bug being fixed.
Verified deterministic across six `PYTHONHASHSEED` values against
trader's packages, and across all four services in trader and optimus;
optimus's `basius` service, previously blocked by this bug, now reports
its real findings.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cuts 0.21.28 with the deterministic chained-ABCI-skill selection fix. Bumps the version across all eight pin sites: `pyproject.toml`, `autonomy/__version__.py`, the assertion in `tests/test_base.py`, `deployments/Dockerfiles/autonomy-user/requirements.txt`, both plugin `setup.py` files, `MATCHING_FRAMEWORK_VERSION` in `plugins/aea-test-autonomy/aea_test_autonomy/configurations.py`, and the `open-aea-test-autonomy` pin in `packages/valory/skills/transaction_settlement_abci/skill.yaml`. That YAML pin cascades through `autonomy packages lock` into eight dependent package hashes and on into `docs/package_list.md`. `abstract_round_abci` does not depend on the bumped package, so its hash is unchanged and `ABSTRACT_ROUND_ABCI_SKILL_WITH_HASH` in `autonomy/constants.py` stays in sync without edit. The two `poetry.lock` version fields are updated in place rather than by regenerating: `poetry lock --regenerate` rewrites ~2200 lines of unrelated dependency metadata and still records the stale plugin version, because Poetry reads installed dist-info for `develop` path dependencies. `poetry check --lock` passes. `docs/upgrading.md` documents the one operator-visible change: an agent with several non-abstract skills depending on `abstract_round_abci` and no dependency relating them now fails with a named error instead of selecting one arbitrarily. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses review feedback from @DIvyaNautiyal07. `_get_chained_abci_skill` reported two structurally different faults through one message. `len(roots) != 1` catches both "no root at all" -- every candidate is declared as a dependency by another skill, so nothing composes the rest -- and "several roots", the genuinely ambiguous case. The advice to mark the non-chained skills abstract or declare a dependency between them is right for several roots and misleading for none, where the problem is that a dependency already exists. Split into two branches, each naming the skills relevant to its own fault: the no-root message lists the candidates, the several-roots message lists only the roots. The `:raises:` clause claimed the exception covers any failure to identify the skill. It does not: zero candidates returns `None`, which the caller turns into its own message. Reworded so a reader guarding only on the exception does not miss the `None` path. The comment above the root filter said the chained app is the one nothing else "in the agent" declares. The set is built from every overridden skill, candidates and abstract intermediates alike, and that wider scope is what makes composition through an abstract intermediate resolve. Says so now. Three tests, each mutation-verified against the class alone: - A non-abstract skill that does not depend on `abstract_round_abci` is not a candidate. Removing that gate previously failed only `TestVerifySkillConfig::test_abci_skill_not_found`, by way of the CLI; every test in `TestChainedAbciSkillSelection` passed, because every fixture there inherits `abstract_round_abci` from `get_dummy_skill_config`. - Zero candidates returns `None` rather than raising, pinning the contract the caller depends on. - An abstract skill declaring the chained app leaves no root at all and reports the no-root fault. This is the case that makes the single-candidate fast path load-bearing: were the two paths merged, a lone candidate declared by any abstract skill would raise here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
DIvyaNautiyal07
approved these changes
Aug 27, 2026
DIvyaNautiyal07
left a comment
Contributor
There was a problem hiding this comment.
All five previous review findings addressed cleanly in ec2926ef. Bonus mutation test pins the split-branches invariant. LGTM.
Adamantios
approved these changes
Aug 27, 2026
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.
Proposed changes
autonomy analyse servicepicked the chained ABCI skill non-deterministically._get_chained_abci_skilliteratedAgentConfig.skills— aSet[PublicId]— and returned the first non-abstract skill declaringabstract_round_abcias a dependency. With more than one qualifying skill the winner was decided by set iteration order, so the command's answer changed withPYTHONHASHSEED.On valory-xyz/trader both
trader_abciandfunds_managerqualify (eachis_abstract: false, each declaringabstract_round_abci):PYTHONHASHSEEDfunds_managertrader_abciThat coin flip is why trader carries
analyse-servicecommented out of itscommon_checksworkflow with the note that it "is checking funds manager skill which is not an ABCI skill" — a description of one face of the coin rather than a stable property.The fix
Collect every qualifying candidate, then pick the one nothing else in the agent declares as a dependency — the root of the dependency DAG, which is what "chained app" means.
trader_abcilistsfunds_manageramong itsskills, so it is the unique root. Where there is no single root, the command raises a namedClickExceptionlisting the candidates instead of silently picking one.The code this replaces documented the assumption it was breaking:
Three details are load-bearing, each covered by a test:
A → B → CwithBabstract, theB → Cedge is invisible,AandCboth look like roots, and a chain with one obvious root is rejected as ambiguous.The pattern in all three: a hole in the edge model turns into a spurious hard error on a valid config, which is a worse failure than the coin flip being fixed.
Why not just
sorted()Sorting alone is deterministic but wrong: in trader it would consistently select
funds_manager, which is worse than the coin flip because it looks stable. The regression test's fixtures are therefore namedaaa_dependency/zzz_chained, so the dependency sorts before the root exactly asfunds_managerdoes beforetrader_abci. Stripping the dependency logic fails the test withassert 'aaa_dependency' == 'zzz_chained'; please keep that naming.Note for reviewers: the single-candidate fast path is load-bearing
if len(candidates) > 1:guards the DAG logic, and the lone-candidate case returns directly. That looks redundant — a single candidate cannot depend on itself — but it is not, because the dependency set now includes edges from non-candidate skills. Collapsing the two paths would mean a single candidate that any abstract skill happens to declare resolves to zero roots and raises: a spurious hard failure on the most common configuration there is. Please don't merge the branches.Release
Also cuts 0.21.28, bumping the version across all eight pin sites. The
open-aea-test-autonomypin intransaction_settlement_abci/skill.yamlcascades throughautonomy packages lockinto eight dependent package hashes and on intodocs/package_list.md.abstract_round_abcidoes not depend on the bumped package, soABSTRACT_ROUND_ABCI_SKILL_WITH_HASHinautonomy/constants.pystays in sync without edit.The two
poetry.lockversion fields are updated in place rather than regenerated:poetry lock --regeneraterewrites ~2200 lines of unrelated dependency metadata and still records the stale plugin version, because Poetry reads installed dist-info fordeveloppath dependencies.poetry check --lockpasses.Fixes
Unblocks
analyse-serviceintraderandoptimus(separate downstream PRs).optimus'sbasiusservice, previously blocked by this bug, now reports its real findings.Types of changes
One operator-visible behaviour change, documented in
docs/upgrading.md: an agent with several non-abstract skills depending onabstract_round_abciand no dependency relating them now fails with a named error instead of returning an arbitrary skill. That is the intended trade, but it can surface as a new error on a service that previously "passed" by luck.Checklist
mainbranch (left side). Also you should start your branch off ourmain.Further comments
Verification. Deterministic across six
PYTHONHASHSEEDvalues against trader's real packages, and across all four services intraderandoptimus. Six new tests; all five behaviours above are mutation-verified — each dies without at least one test.mypy,pylint(10.00/10),flake8,black,isort,vulture,check-hash,check-packages,check-dependencies,check-doc-links-hashes,check-third-party-hashes,check-abci-docstrings,check-abciapp-specs,check-handlers,check-dialogues,liccheck,banditandpoetry check --lockall pass. Full framework unit suite: 633 passed.Two pre-existing local failures, neither caused by this PR:
tox -e darglintexits 129 on six files inautonomy/,plugins/andpackages/valory/that this PR does not touch.darglint 1.8.1breaks on Python 3.14, which is whymain_workflow.yml:109pins that job to 3.13 with the comment "use 3.14 after darglint2 issue chore(deps-dev): bump tox from 3.23.1 to 3.24.3 #44 is fixed". The two changed files pass darglint cleanly.test_build_image.py,test_images/) fail locally without the images built; they are unrelated to this change.Unrelated loose end spotted while working, not addressed here:
tox -e check-api-docsregenerates an untrackeddocs/api/data/, whichf21680f99("skip data packages from generate api documentation script") deliberately deleted. The generator still emits it and the check does not catch it, so the fix looks incomplete.