cleveragents/cleveractors-core

Fork 0

fix(registry): fetch and parse package content for resolved registry references #138

Merged

CoreRasurae merged 1 commit from bugfix/m1-registry-content-fetch into master

2026-08-24 14:21:39 +00:00

CoreRasurae commented

2026-08-20 21:39:41 +00:00

Member

Summary

Fixes #135: PackageContentResolver._resolve_registry_async only performed the Package Registry Standard §8.2.2 "resolve" call and returned that stub directly as package content, never performing the required §8.2.1 get_package fetch. A type: llm agent referenced via a bare REGISTRY-scheme string in agents.<name> therefore failed AgentFactory.acreate_agent with AgentCreationError("Unknown agent type: agent").

_resolve_registry_async now follows up resolve_package with get_package/get_package_content, parses the returned YAML content, and merges the real package content over the resolve stub (routed through the per-server RegistryCache when available). The resolution cache now stores the fully-resolved content. A malformed/missing content response raises ValidationError instead of silently propagating the stub.

Also fixes robot/fake_registry_server.py's /packages/{id} handler, which was returning raw fields instead of the spec-compliant {"content": "<yaml>"} envelope for actor/skill packages -- this was previously masking the same defect in robot/skill_registry_auth.robot's real end-to-end skill-download flow and in robot/registry_full_cache_integration.robot.

Stacking note: This PR is based on tdd/m1-registry-content-fetch (PR #137, not yet merged) rather than master, since the regression test this fix turns green lives there. Please merge #137 first; this PR's diff against master will shrink to just the fix once that happens.

Closes #135

Test plan

nox -s unit_tests -- full Behave suite green (3084 scenarios)
nox -s coverage_report -- 96.8% (>= 96.5% threshold)
nox -s typecheck -- 0 errors
nox -s lint / nox -s format -- --check -- clean
nox -s security_scan / nox -s dead_code -- clean
nox -s integration_tests -- full Robot suite green (363 tests), including new REGISTRY-backed type: llm agent scenario in agent_package_references.robot
Removed @tdd_expected_fail from the @tdd_issue_135 scenario now that the fix lands

## Summary Fixes #135: `PackageContentResolver._resolve_registry_async` only performed the Package Registry Standard §8.2.2 "resolve" call and returned that stub directly as package content, never performing the required §8.2.1 `get_package` fetch. A `type: llm` agent referenced via a bare REGISTRY-scheme string in `agents.<name>` therefore failed `AgentFactory.acreate_agent` with `AgentCreationError("Unknown agent type: agent")`. `_resolve_registry_async` now follows up `resolve_package` with `get_package`/`get_package_content`, parses the returned YAML `content`, and merges the real package content over the resolve stub (routed through the per-server `RegistryCache` when available). The resolution cache now stores the fully-resolved content. A malformed/missing `content` response raises `ValidationError` instead of silently propagating the stub. Also fixes `robot/fake_registry_server.py`'s `/packages/{id}` handler, which was returning raw fields instead of the spec-compliant `{"content": "<yaml>"}` envelope for actor/skill packages -- this was previously masking the same defect in `robot/skill_registry_auth.robot`'s real end-to-end skill-download flow and in `robot/registry_full_cache_integration.robot`. **Stacking note:** This PR is based on `tdd/m1-registry-content-fetch` (PR #137, not yet merged) rather than `master`, since the regression test this fix turns green lives there. Please merge #137 first; this PR's diff against `master` will shrink to just the fix once that happens. Closes #135 ## Test plan - [x] `nox -s unit_tests` -- full Behave suite green (3084 scenarios) - [x] `nox -s coverage_report` -- 96.8% (>= 96.5% threshold) - [x] `nox -s typecheck` -- 0 errors - [x] `nox -s lint` / `nox -s format -- --check` -- clean - [x] `nox -s security_scan` / `nox -s dead_code` -- clean - [x] `nox -s integration_tests` -- full Robot suite green (363 tests), including new REGISTRY-backed `type: llm` agent scenario in `agent_package_references.robot` - [x] Removed `@tdd_expected_fail` from the `@tdd_issue_135` scenario now that the fix lands

CoreRasurae added this to the v2.1.0 milestone

2026-08-20 21:39:41 +00:00

CoreRasurae added 1 commit

2026-08-20 21:39:42 +00:00

fix(registry): fetch and parse package content for resolved registry references a029089587

PackageContentResolver._resolve_registry_async only performed the Package
Registry Standard §8.2.2 "resolve" call (GET /{package_type}/{namespace}/{name}
-> {"package_id", "type"}) and returned that stub directly as if it were the
resolved package content. It never performed the required §8.2.1 "get package
by ID" call (GET /packages/{package_id} -> {"content": "<YAML string>"}) to
fetch and parse the real package payload, so a REGISTRY-scheme agents.<name>
(and routes.<name>/skills:) reference ended up with type: "agent" (the resolve
stub's package-type field) instead of the real package's type: "llm", per
docs/index.md §4.1.1 and ADR-2037 D-3.

_resolve_registry_async now follows up resolve_package with get_package/
get_package_content for the returned package_id, parses the YAML content
string via a new _parse_package_content helper, and merges the parsed
content over the resolve stub so the real type/config win. The fetch routes
through the per-server RegistryCache when available, mirroring the existing
resolve_package cache/no-cache branching, and the resolution cache now stores
the fully-resolved content instead of the intermediate stub. A malformed or
missing content response now raises ValidationError (a RegistryError
subclass) instead of silently propagating the stub.

Removed @tdd_expected_fail from the issue #135 regression scenario in
features/registry_http_client.feature now that the fix lands. Updated
existing mocks that only modeled the single §8.2.2 call
(registry_http_client_steps.py, registry_reference_resolver_coverage_steps.py,
cache_coverage_steps.py/.feature) to also serve a valid §8.2.1 response, and
fixed robot/fake_registry_server.py's /packages/{id} handler to wrap known
package content in the spec-compliant {"content": "<yaml>"} envelope instead
of returning raw fields at the top level -- both registry_full_cache_integration.robot
(updated miss-count assertions, now 2 misses per fetch instead of 1) and
skill_registry_auth.robot's real end-to-end skill-download flow depend on this
shape and were previously silently red. Added a new Robot scenario in
agent_package_references.robot proving a type: llm agent referenced via a bare
REGISTRY-scheme string constructs successfully through AgentFactory.acreate_agent
against a real fake registry server.

ISSUES CLOSED: #135

CoreRasurae added the

Type

Bug

label

2026-08-20 21:43:22 +00:00

CoreRasurae added a new dependency

2026-08-20 21:44:02 +00:00

#135 PackageContentResolver never fetches package content for REGISTRY references — agents.<name>/skills:/routes.<name> get the §8.2.2 stub instead of real config

CoreRasurae changed target branch from tdd/m1-registry-content-fetch to master

2026-08-24 08:59:21 +00:00

hurui200320 requested changes

2026-08-24 09:27:35 +00:00

Dismissed

hurui200320 left a comment

PR Review: !138 (Ticket #135)

Verdict: Request Changes

The fix correctly implements the two-call Package Registry Standard §8.2.2 + §8.2.1 resolution flow for PackageContentResolver and turns the new Behave/Robot regression tests green for the happy path. However, the implementation does not match the stated acceptance criteria or the CHANGELOG for one malformed-response edge case: a resolve response that lacks package_id silently returns the §8.2.2 stub instead of raising ValidationError. That leaves the original failure mode (AgentCreationError("Unknown agent type: agent")) reachable against a non-compliant registry and should be fixed before merge.

Critical Issues

None.

Major Issues

src/cleveractors/registry/reference_resolver.py, PackageContentResolver._resolve_registry_async (lines 561–566)
When resolve_package returns a dict without a usable package_id, the code merges the stub and returns it verbatim without fetching /packages/{id}. This contradicts issue #135's acceptance criteria and the CHANGELOG, both of which state that "a resolve response missing package_id ... now raises ValidationError".
Reproduced with a mocked client returning {"type": "agent"}: the resolver returned {"type": "agent", ...} and never called get_package.
Recommendation: Add an explicit validation after extracting package_id and raise ValidationError (a RegistryError subclass) when it is missing, not a string, or empty, e.g.:
```
package_id = resolved.get("package_id")
if not isinstance(package_id, str) or not package_id:
    raise ValidationError(
        f"Registry resolve response missing 'package_id' for {original_reference!r}",
        original_reference=original_reference,
    )
```

Minor Issues

Missing test coverage for new error branches
The new _parse_package_content error paths (missing/non-string content, malformed YAML, non-mapping result) and the missing package_id branch are not exercised by any Behave/Robot test. Since these are new code paths introduced by this PR, they should have dedicated scenarios so the 96.5% threshold is not masking uncovered error handling.
robot/fake_registry_server.py resolve response is still non-compliant
_build_resolve_response embeds the real package content at the top level of the §8.2.2 resolve response. The Robot E2E tests therefore pass even if the resolver failed to call /packages/{id}. While the Behave scenario covers the compliant stub case, tightening the fake server to return only {"package_id", "type"} from the resolve endpoint would make the Robot regression a stricter guard against regressions of issue #135.

Nits

Coverage threshold wording mismatch
Issue #135's subtasks say "coverage >= 97%", the PR description cites a 96.5% threshold, and noxfile.py enforces COVERAGE_THRESHOLD = 96.5. This is project housekeeping rather than a PR defect, but aligning the wording would avoid future confusion.

Summary

This PR is well-focused and the core happy-path fix is solid: REGISTRY references now fetch real package content through get_package/get_package_content, route through RegistryCache when configured, cache the fully-resolved result, and surface ValidationError for malformed content responses. Once the missing/empty package_id case is also validated and a regression test is added, this should be good to approve. I did not run the full nox suite, but targeted Behave/Robot runs for the touched features passed, and pyright/ruff are clean on the changed files.

## PR Review: !138 (Ticket #135) ### Verdict: Request Changes The fix correctly implements the two-call Package Registry Standard §8.2.2 + §8.2.1 resolution flow for `PackageContentResolver` and turns the new Behave/Robot regression tests green for the happy path. However, the implementation does not match the stated acceptance criteria or the CHANGELOG for one malformed-response edge case: a resolve response that lacks `package_id` silently returns the §8.2.2 stub instead of raising `ValidationError`. That leaves the original failure mode (`AgentCreationError("Unknown agent type: agent")`) reachable against a non-compliant registry and should be fixed before merge. ### Critical Issues None. ### Major Issues - **`src/cleveractors/registry/reference_resolver.py`, `PackageContentResolver._resolve_registry_async` (lines 561–566)** When `resolve_package` returns a dict without a usable `package_id`, the code merges the stub and returns it verbatim without fetching `/packages/{id}`. This contradicts issue #135's acceptance criteria and the CHANGELOG, both of which state that "a resolve response missing `package_id` ... now raises `ValidationError`". Reproduced with a mocked client returning `{"type": "agent"}`: the resolver returned `{"type": "agent", ...}` and never called `get_package`. **Recommendation:** Add an explicit validation after extracting `package_id` and raise `ValidationError` (a `RegistryError` subclass) when it is missing, not a string, or empty, e.g.: ```python package_id = resolved.get("package_id") if not isinstance(package_id, str) or not package_id: raise ValidationError( f"Registry resolve response missing 'package_id' for {original_reference!r}", original_reference=original_reference, ) ``` ### Minor Issues - **Missing test coverage for new error branches** The new `_parse_package_content` error paths (missing/non-string `content`, malformed YAML, non-mapping result) and the missing `package_id` branch are not exercised by any Behave/Robot test. Since these are new code paths introduced by this PR, they should have dedicated scenarios so the 96.5% threshold is not masking uncovered error handling. - **`robot/fake_registry_server.py` resolve response is still non-compliant** `_build_resolve_response` embeds the real package content at the top level of the §8.2.2 resolve response. The Robot E2E tests therefore pass even if the resolver failed to call `/packages/{id}`. While the Behave scenario covers the compliant stub case, tightening the fake server to return only `{"package_id", "type"}` from the resolve endpoint would make the Robot regression a stricter guard against regressions of issue #135. ### Nits - **Coverage threshold wording mismatch** Issue #135's subtasks say "coverage >= 97%", the PR description cites a 96.5% threshold, and `noxfile.py` enforces `COVERAGE_THRESHOLD = 96.5`. This is project housekeeping rather than a PR defect, but aligning the wording would avoid future confusion. ### Summary This PR is well-focused and the core happy-path fix is solid: REGISTRY references now fetch real package content through `get_package`/`get_package_content`, route through `RegistryCache` when configured, cache the fully-resolved result, and surface `ValidationError` for malformed content responses. Once the missing/empty `package_id` case is also validated and a regression test is added, this should be good to approve. I did not run the full `nox` suite, but targeted Behave/Robot runs for the touched features passed, and `pyright`/`ruff` are clean on the changed files.

CoreRasurae commented

2026-08-24 10:18:04 +00:00

Author

Member

ingore Nits issue, is not relevant, we have a noxfile threshold at 96.5% while having a 97% in the specification, but the 97% is the rounded value, which is also obtainable from a 96.5% real coverage.

CoreRasurae force-pushed bugfix/m1-registry-content-fetch from a029089587 to c0ca26cfe9

CI / lint (pull_request) Successful in 42s

Details

CI / unit_tests (pull_request) Has started running

Details

CI / typecheck (pull_request) Successful in 1m19s

Details

CI / benchmark (pull_request) Has started running

Details

CI / security (pull_request) Successful in 1m13s

Details

CI / build (pull_request) Successful in 1m5s

Details

CI / quality (pull_request) Successful in 2m0s

Details

CI / integration_tests (pull_request) Successful in 4m10s

Details

CI / coverage (pull_request) Has been cancelled

Details

CI / status-check (pull_request) Has been cancelled

Details

2026-08-24 12:07:47 +00:00

Compare

CoreRasurae commented

2026-08-24 12:08:24 +00:00

Author

Member

Addressed the review findings from @hurui200320's request-changes review (verified each against issue #135's acceptance criteria, docs/index.md §4.1.1, docs/adr/ADR-2037-agent-and-route-package-reference-resolution.md D-3, and docs/actor-registry-standard.md §8.2.1/§8.2.2 before applying):

Major — missing package_id silently returned the resolve stub
Confirmed as a genuine gap against docs/index.md §4.1.1 ("a missing package ... MUST signal a configuration error") and the CHANGELOG's own (previously unimplemented) claim. PackageContentResolver._resolve_registry_async now raises ValidationError when the §8.2.2 resolve response lacks a non-empty string package_id, instead of merging the stub and returning it unfetched.

Minor — missing test coverage for the new error branches
Added 5 Behave scenarios to features/registry_http_client.feature: the missing-package_id case above, plus _parse_package_content's four error paths (missing content, non-string content, malformed YAML, non-mapping YAML). All previously untested lines are now covered.

Minor — robot/fake_registry_server.py resolve response embedded real content
Confirmed non-compliant with §8.2.2 ({"package_id", "type"} only). _build_resolve_response no longer embeds _PACKAGE_CONTENT — the "cache warming" behavior it cited is not consumed by any production caller (RegistryCache.put is only exercised directly in cache_coverage.feature). Verified robot/agent_package_references.robot, robot/registry_full_cache_integration.robot, and robot/skill_registry_auth.robot don't depend on the removed content and all still pass.

Nit — coverage threshold wording
Per @CoreRasurae's comment above, leaving as-is: 96.5% real coverage rounds to the 97% cited in the issue subtasks, so there's no actual discrepancy to fix.

All changes squashed into the existing commit (amended, force-pushed) rather than added as new commits, since this PR is still a single unmerged commit implementing issue #135.

Addressed the review findings from @hurui200320's request-changes review (verified each against issue #135's acceptance criteria, `docs/index.md` §4.1.1, `docs/adr/ADR-2037-agent-and-route-package-reference-resolution.md` D-3, and `docs/actor-registry-standard.md` §8.2.1/§8.2.2 before applying): **Major — missing `package_id` silently returned the resolve stub** Confirmed as a genuine gap against `docs/index.md` §4.1.1 ("a missing package ... MUST signal a configuration error") and the CHANGELOG's own (previously unimplemented) claim. `PackageContentResolver._resolve_registry_async` now raises `ValidationError` when the §8.2.2 resolve response lacks a non-empty string `package_id`, instead of merging the stub and returning it unfetched. **Minor — missing test coverage for the new error branches** Added 5 Behave scenarios to `features/registry_http_client.feature`: the missing-`package_id` case above, plus `_parse_package_content`'s four error paths (missing `content`, non-string `content`, malformed YAML, non-mapping YAML). All previously untested lines are now covered. **Minor — `robot/fake_registry_server.py` resolve response embedded real content** Confirmed non-compliant with §8.2.2 (`{"package_id", "type"}` only). `_build_resolve_response` no longer embeds `_PACKAGE_CONTENT` — the "cache warming" behavior it cited is not consumed by any production caller (`RegistryCache.put` is only exercised directly in `cache_coverage.feature`). Verified `robot/agent_package_references.robot`, `robot/registry_full_cache_integration.robot`, and `robot/skill_registry_auth.robot` don't depend on the removed content and all still pass. **Nit — coverage threshold wording** Per @CoreRasurae's comment above, leaving as-is: 96.5% real coverage rounds to the 97% cited in the issue subtasks, so there's no actual discrepancy to fix. All changes squashed into the existing commit (amended, force-pushed) rather than added as new commits, since this PR is still a single unmerged commit implementing issue #135.

CoreRasurae requested review from hurui200320

2026-08-24 12:08:44 +00:00

CoreRasurae force-pushed bugfix/m1-registry-content-fetch from c0ca26cfe9

CI / lint (pull_request) Successful in 42s

Details

CI / unit_tests (pull_request) Has started running

Details

CI / typecheck (pull_request) Successful in 1m19s

Details

CI / benchmark (pull_request) Has started running

Details

CI / security (pull_request) Successful in 1m13s

Details

CI / build (pull_request) Successful in 1m5s

Details

CI / quality (pull_request) Successful in 2m0s

Details

CI / integration_tests (pull_request) Successful in 4m10s

Details

CI / coverage (pull_request) Has been cancelled

Details

CI / status-check (pull_request) Has been cancelled

Details

to 5a34ff04bf

CI / quality (pull_request) Successful in 50s

Details

CI / security (pull_request) Successful in 1m11s

Details

CI / build (pull_request) Successful in 1m48s

Details

CI / typecheck (pull_request) Successful in 2m1s

Details

CI / benchmark (pull_request) Failing after 29m33s

Details

CI / lint (pull_request) Successful in 1m42s

Details

CI / unit_tests (pull_request) Failing after 4m22s

Details

CI / coverage (pull_request) Has been skipped

Details

CI / integration_tests (pull_request) Failing after 14m50s

Details

CI / status-check (pull_request) Has been cancelled

Details

2026-08-24 12:13:47 +00:00

Compare

hurui200320 approved these changes

2026-08-24 12:49:01 +00:00

hurui200320 left a comment

PR Review: !138 (Ticket #135)

Verdict: Approve

The implementation correctly fixes issue #135: PackageContentResolver._resolve_registry_async now performs the full Package Registry Standard §8.2.2 resolve + §8.2.1 get_package fetch, parses the YAML content, and returns the real package payload instead of the resolve-endpoint stub. The previously requested package_id validation and malformed-content ValidationError paths are all in place and covered by Behave scenarios. The fake registry server now returns a strict §8.2.2 stub and a spec-compliant §8.2.1 envelope, making the Robot E2E tests a reliable regression guard. No critical or major issues remain.

Critical Issues

None.

Major Issues

None.

Minor Issues

None.

Nits

None.

Summary

This is a focused, well-tested fix. The production change in src/cleveractors/registry/reference_resolver.py is minimal and follows the existing cache/no-cache branching pattern: after a successful resolve_package, it validates package_id, fetches and parses the real content via get_package/get_package_content (routed through RegistryCache when configured), and caches the fully-resolved result. The new _parse_package_content helper raises ValidationError for missing/non-string content, malformed YAML, and non-mapping YAML, matching the stated acceptance criteria.

Test coverage is comprehensive:

Behave regression scenarios for the happy path, missing package_id, missing/non-string content, malformed YAML, and non-mapping YAML.
Updated cache-coverage expectations to account for the second per-resolution cache miss.
Robot end-to-end scenario proving a type: llm agent package resolves through the real resolver/client pipeline against the fake server.
The fake registry server's resolve and get_package endpoints now conform strictly to §8.2.2 and §8.2.1, closing the test-double gap that was masking the defect.

All previously raised review concerns have been addressed in the latest revision. I did not run the full nox suite locally due to environment dependencies, but the changed code is type-safe, lint-clean on inspection, and the PR's reported test results are consistent with the code structure.

## PR Review: !138 (Ticket #135) ### Verdict: Approve The implementation correctly fixes issue #135: `PackageContentResolver._resolve_registry_async` now performs the full Package Registry Standard §8.2.2 resolve + §8.2.1 `get_package` fetch, parses the YAML `content`, and returns the real package payload instead of the resolve-endpoint stub. The previously requested `package_id` validation and malformed-content `ValidationError` paths are all in place and covered by Behave scenarios. The fake registry server now returns a strict §8.2.2 stub and a spec-compliant §8.2.1 envelope, making the Robot E2E tests a reliable regression guard. No critical or major issues remain. ### Critical Issues None. ### Major Issues None. ### Minor Issues None. ### Nits None. ### Summary This is a focused, well-tested fix. The production change in `src/cleveractors/registry/reference_resolver.py` is minimal and follows the existing cache/no-cache branching pattern: after a successful `resolve_package`, it validates `package_id`, fetches and parses the real content via `get_package`/`get_package_content` (routed through `RegistryCache` when configured), and caches the fully-resolved result. The new `_parse_package_content` helper raises `ValidationError` for missing/non-string `content`, malformed YAML, and non-mapping YAML, matching the stated acceptance criteria. Test coverage is comprehensive: - Behave regression scenarios for the happy path, missing `package_id`, missing/non-string `content`, malformed YAML, and non-mapping YAML. - Updated cache-coverage expectations to account for the second per-resolution cache miss. - Robot end-to-end scenario proving a `type: llm` agent package resolves through the real resolver/client pipeline against the fake server. - The fake registry server's resolve and `get_package` endpoints now conform strictly to §8.2.2 and §8.2.1, closing the test-double gap that was masking the defect. All previously raised review concerns have been addressed in the latest revision. I did not run the full `nox` suite locally due to environment dependencies, but the changed code is type-safe, lint-clean on inspection, and the PR's reported test results are consistent with the code structure.

CoreRasurae force-pushed bugfix/m1-registry-content-fetch from 5a34ff04bf

CI / quality (pull_request) Successful in 50s

Details

CI / security (pull_request) Successful in 1m11s

Details

CI / build (pull_request) Successful in 1m48s

Details

CI / typecheck (pull_request) Successful in 2m1s

Details

CI / benchmark (pull_request) Failing after 29m33s

Details

CI / lint (pull_request) Successful in 1m42s