feat(registry): implement RegistryError hierarchy with typed exceptions #43
No reviewers
Labels
No labels
auto/blocked-by-deps
auto/ci-timeout
auto/claimed-implementer
auto/claimed-merge
auto/claimed-reviewer
auto/driver-down
auto/invariant-violation
auto/last-attempt-tier-0
auto/last-attempt-tier-1
auto/last-attempt-tier-2
auto/last-attempt-tier-min
Automation Tracking
auto/needs-conflict-resolution
auto/needs-implementer
auto/postmortem
auto/ready-to-merge
auto/restart-throttled
auto/revert
auto/sentinel
auto/stale-inactivity
auto/unstable
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs Feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
CI Blocker
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Automation
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Refactor
Type
Support
Type
Task
Type
Testing
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Blocks
#29 feat(registry): implement error hierarchy — RegistryError base + all 8 error types
cleveragents/cleveractors-core
Reference
cleveragents/cleveractors-core!43
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feature/m1-registry-exceptions"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Implements the typed exception hierarchy per Package Registry Standard v1.0.0 §13.2.
Changes
RegistryErrorbase class withmessage,details: dict,original_reference: strfields__str__includesoriginal_referencewhen availablePackageNotFoundError(404),InvalidPackageIdError(400),InvalidPackageReferenceError(400),VersionNotFoundError(404),ValidationError(400),AuthenticationRequiredError(401),AccessDeniedError(403),ConflictError(409),RegistryNetworkError(5xx/timeout/connection)RegistryNetworkErrorcarriesstatus_codeandurlfor transport failuresexception_for_status()maps HTTP status codes to typed exceptions_ERROR_TYPE_MAPenables error-type parsing from structured JSON error bodiesInternalServerErrorremoved from hierarchy (mapped toRegistryNetworkErrorin_ERROR_TYPE_MAP)Tests
features/registry_exceptions.featurerobot/exceptions.robotregistry_http_clienttests updated for theInternalServerError→RegistryNetworkErrormigrationexceptions.pyachieves 100% coverageQuality gates
nox -s lint— all checks passednox -s typecheck— 0 errorsnox -s security_scan— 0 findingsnox -s dead_code— 0 issuesnox -s unit_tests— 2284 scenarios passednox -s coverage_report— above 96.5% thresholdnox -s integration_tests— 169 tests passedCloses #29
Code Review — PR #43
Reviewed against
CONTRIBUTING.md. Two issues require attention before this PR can be merged — one is a hard merge gate blocker, one is a required process correction.BLOCKING — must be resolved before merge
1. Coverage below the 97% merge gate
The PR body self-reports:
CONTRIBUTING.mdis explicit:96.5% is below the gate. The CI
coveragejob will block thestatus-checkgate and the PR cannot merge in its current state. Please bring coverage to ≥ 97% and report the updated figure in the PR body.REQUIRED — process violation that must be corrected
2. Issue #29 is missing all labels
The linked issue #29 currently has no labels at all.
CONTRIBUTING.mdrequires:At this point in the lifecycle, issue #29 must carry:
State/In Review— a PR has been submittedType/Feature— the issue describes new functionalityPriority/label — required for all non-Epic, non-Legendary issues in any active statePlease apply the correct labels to issue #29.
Informational — not blocking
3. Minor inaccuracy in commit body
The commit body reads:
The actual public API function is
exception_for_status()(no leading underscore). The private mapping dict is_ERROR_TYPE_MAP. This does not affect the code but is inaccurate documentation in the commit history.4. Robot Framework tests added without a subtask in issue #29
Issue #29 has no subtask for
robot/exceptions.robot, yet 12 Robot Framework integration tests were added in this PR. The tests are a good addition and the files are correctly placed underrobot/. This is worth noting for completeness; no action required.What passes
Commit Messagein issue #29 Metadata verbatimfeature/m1-registry-exceptionsmatches issue #29 Metadata verbatimv2.1.0matches the linked issueISSUES CLOSED: #29Closes #29Type/Featurelabel present on the PRCHANGELOG.mdupdated with one entryCONTRIBUTORS.mdalready lists Luis Mendes — no update neededsrc/cleveractors/,features/,features/steps/,robot/)features/mocks/# type: ignoresuppressionsregistry_exceptions_steps.pyforregistry_exceptions.feature3dce4032671847ae2f22PR Review: !43 (Ticket #29)
Verdict: Request Changes
The exception class hierarchy is well-designed and the inheritance chain is correct. However, there is a systematic gap between the class API (which looks complete) and the end-to-end runtime behavior: the
status_code,url,details, andoriginal_referencefields are defined on the classes but never populated by the HTTP client that is the only realistic caller. Additionally, there are test quality issues including a tautological assertion and missing coverage for key behaviors.Critical Issues
None.
Major Issues
1.
_ERROR_TYPE_MAPis missing"InvalidPackageReference"keysrc/cleveractors/registry/exceptions.py:106–115InvalidPackageReferenceErroris defined and exported, but has no entry in_ERROR_TYPE_MAP. A server returning{"error": {"type": "InvalidPackageReference", ...}}will fall through the map, hitexception_for_status(400, ...), and raiseInvalidPackageIdError— the wrong class. User code withexcept InvalidPackageReferenceErrorwill never fire for server-originated errors."InvalidPackageReference": InvalidPackageReferenceError,to_ERROR_TYPE_MAPand add a BDD scenario to lock it in.2.
client.py_request()raisesRegistryNetworkErrorfrom the JSON path withoutstatus_codeorurlsrc/cleveractors/registry/client.py:103–104"type": "InternalServerError", the code calls_ERROR_TYPE_MAP["InternalServerError"](msg)— which isRegistryNetworkError(msg)with nostatus_codeand nourl. But when the same 5xx has no structured body, the fallbackexception_for_status(500, msg)correctly setsstatus_code=500. Two servers returning the same 5xx produceRegistryNetworkErrorinstances with differentstatus_codevalues (Nonevs500) depending solely on whether the body has anerror.typefield. The PR's headline claim — "RegistryNetworkErrorcarriesstatus_codeandurl" — is not true at runtime.RegistryNetworkError, passstatus_codeandurl:3.
client.py_request()discardsurlonhttpx.RequestError(timeouts, connection errors)src/cleveractors/registry/client.py:106–107raise RegistryNetworkError(str(exc)) from excnever setsurl, even thoughexc.request.urlis always available onhttpx.RequestError. Theurlfield exists specifically for this case (per the docstring: "The URL that was being accessed when the error occurred") but is alwaysNonefor transport failures.url = str(exc.request.url) if getattr(exc, "request", None) is not None else None; raise RegistryNetworkError(str(exc), url=url) from exc4.
client.py_request()silently drops thedetailsfield from structured error bodiessrc/cleveractors/registry/client.py:91–102typeandmessagefrombody["error"]but ignoresdetails. A server returning{"error": {"type": "ValidationError", "message": "...", "details": {"field": "name"}}}produces aValidationErrorwithdetails=None. Thedetailsfield is documented and tested for direct construction, but is never populated from the network path.details = error_block.get("details") if isinstance(error_block.get("details"), dict) else Noneand pass it to the exception constructor.5. Tautological
"all checks should pass"step — assertion is always truefeatures/steps/registry_exceptions_steps.py:213–215assert context._checks_passed >= 0is always true because_checks_passedis initialized to0and only incremented. The step never validates that any inheritance checks actually ran. If theissubclassassertion in theWhenstep were silently removed, theThenstep would still pass.Then all {count:d} checks should passand assertcontext._checks_passed == count. Or at minimum useassert context._checks_passed > 0.6.
exception_for_status(500/503)scenarios don't assertstatus_codepropagationfeatures/registry_exceptions.feature:86–94exception_for_statusnow passesstatus_code=status_codetoRegistryNetworkError— is not tested. A regression dropping that argument would go undetected.And the error status_code should be 500(and503) to those scenarios. The step definition already exists.7. HTTP-client migration scenarios don't assert
status_codeon raisedRegistryNetworkErrorfeatures/registry_http_client.feature:128–138InternalServerError→RegistryNetworkErrormigration is that the new exception carriesstatus_code. This is not validated at the integration level.And the raised error should have status_code 500(and503) and a corresponding step definition.8. Unused import of
exception_for_statusinrobot/CleverActorsLib.pyrobot/CleverActorsLib.py:44exception_for_statusis imported but never used in any keyword method or referenced in any.robotfile. WithreportUnusedImport = trueinpyrightconfig.json, this will causenox -s typecheckto fail.Minor Issues
9.
RegistryNetworkError.__str__omitsstatus_codeandurlsrc/cleveractors/registry/exceptions.py:80–103RegistryNetworkErrorinherits__str__fromRegistryError, which only includesmessageandoriginal_reference. Thestatus_codeandurlfields — the defining attributes of this subclass — are invisible instr(exc), making log output and tracebacks uninformative for transport failures.__str__to append[status: {code}]and[url: {url}]when those fields are set.10.
exception_for_statusnot exported fromcleveractors.registry.__all__src/cleveractors/registry/__init__.pyexception_for_statusis documented in the CHANGELOG as a public feature and is tested in the BDD suite, but it is not in__all__. It is reachable only via the private pathcleveractors.registry.exceptions.exception_for_status. Other utility functions (resolve_version,is_concrete_version,is_version_alias) are all exported."exception_for_status"to__all__.11.
RegistryError.__str__uses a falsy check onoriginal_referenceinstead ofis not Nonesrc/cleveractors/registry/exceptions.py:43if self.original_reference:treatsoriginal_reference=""identically toNone. An empty string is a valid value the caller explicitly supplied, but it is silently dropped fromstr(exc). The attribute and the string representation diverge.if self.original_reference is not None:.12. 8 of 9 typed exception subclasses have zero construction coverage
features/registry_exceptions.feature:11–57RegistryErrordirectly.PackageNotFoundError,InvalidPackageIdError,InvalidPackageReferenceError,VersionNotFoundError,ValidationError,AuthenticationRequiredError,AccessDeniedError, andConflictErrorare never constructed in tests. A future PR that accidentally breaks the inherited__init__for any of these would not be caught.13. Robot
exceptions.robotcoversisinstancefor only 3 of 9 typed exceptionsrobot/exceptions.robot:67–77Exception Is Instance Cleveragents Exceptionis only tested forRegistryError,PackageNotFoundError, andRegistryNetworkError. The other 6 typed exceptions are tested forissubclass(..., RegistryError)but not for the transitiveissubclass(..., CleverAgentsException). The keyword library already supports all 9.Exception Is Instance Cleveragents Exceptiontest cases for the 6 missing typed exceptions.Nits
N1.
_ERROR_TYPE_MAPmissingInvalidPackageReferenceErroris undocumentedsrc/cleveractors/registry/exceptions.py:106_ERROR_TYPE_MAPexplaining the scope.N2.
_ERROR_TYPE_MAPcompleteness test lacks cardinality/negative assertionsfeatures/registry_exceptions.feature:121–131And the map should have exactly 8 keys.N3.
_ERROR_TYPE_MAPshould be aMappingProxyTypefor immutabilitysrc/cleveractors/registry/exceptions.py:106frozensetand@dataclass(frozen=True)for module-level lookup data._ERROR_TYPE_MAPis a mutabledictthat any code can modify, changing which exception class is raised for a given error type. Wrapping it intypes.MappingProxyType({...})would match the project convention.N4.
detailsdict stored by reference — caller mutation changes exception statesrc/cleveractors/registry/exceptions.py:38self.details = detailsstores the caller's dict by reference. A defensivedict(details)shallow copy in__init__would prevent surprise mutations.Summary
The exception class hierarchy itself is clean, well-typed, and correctly structured. The inheritance chain, field definitions, and
InternalServerError→RegistryNetworkErrormigration are all done correctly. The BDD test scaffolding is comprehensive in breadth.However, the PR has a systematic disconnect between the class API and the runtime behavior: the
status_code,url,details, andoriginal_referencefields are defined and documented but never populated by the HTTP client (client.py). The PR's stated goal — "RegistryNetworkErrorcarriesstatus_codeandurl" — is not true at runtime. Additionally, the_ERROR_TYPE_MAPis missing theInvalidPackageReferenceErrormapping, meaning server-originated reference errors will be misclassified asInvalidPackageIdError. The test suite also has a tautological assertion (always passes regardless of whether checks ran) and is missingstatus_codepropagation assertions for the key new behavior.Response to Review (rui.hu / hurui200320)
Thank you for the thorough review. All issues have been addressed in the amended commit
287fab5.Major Issues — All Fixed
1.
_ERROR_TYPE_MAPmissing"InvalidPackageReference"key — Fixed.Added
"InvalidPackageReference": InvalidPackageReferenceErrorto the map so server-originated reference errors are correctly classified. The map now contains 9 entries (8 standard §13.2 types + 1 project-specific). A BDD scenario with a cardinality assertion (And the map should have exactly 9 keys) locks this in.2.
client.py_request()raisesRegistryNetworkErrorfrom JSON path withoutstatus_codeorurl— Fixed.When the resolved class is
RegistryNetworkError, the handler now passesstatus_code=status,url=url, anddetails=detailsto the constructor. This ensuresRegistryNetworkErrorinstances carry the same metadata regardless of whether they originate from a plain status-code fallback or a structured JSON error body.3.
client.py_request()discardsurlonhttpx.RequestError— Fixed.The
except httpx.RequestErrorhandler now attempts to extracturlfromexc.request.urlvia atry / except RuntimeErrorguard (the.requestproperty raisesRuntimeErrorwhen unset, as happens withhttpx.ReadTimeoutconstructed without a request object).4.
client.py_request()silently drops thedetailsfield — Fixed.The body parser now extracts
detailsfromerror_block["details"]when it is a dict, and passes it to every exception constructor in the typed-error path.5. Tautological
"all checks should pass"assertion — Fixed.Replaced
assert context._checks_passed >= 0with parameterized@then('all {count:d} checks should pass')that assertscontext._checks_passed == count. The two inheritance scenarios now carry explicit counts: 9 (typed subclasses) and 1 (CleverAgentsException).6. Missing
status_codepropagation assertions onexception_for_status(500/503)— Fixed.Added
And the error status_code should be 500andAnd the error status_code should be 503to the respective scenarios.7. Missing
status_codeassertions in HTTP-client 500/503 scenarios — Fixed.Added
And the raised error should have status_code 500and503to both scenarios. A new step definitionstep_check_error_status_codeinregistry_http_client_steps.pyverifiescontext.error.status_code == expected.8. Unused import of
exception_for_statusinrobot/CleverActorsLib.py— Fixed.Added a keyword method
exception_for_status_returns(code, expected_class)that wrapsexception_for_status()and asserts both the instance type and thestatus_codefield when present. The import is now exercised.Minor Issues — All Fixed
9.
RegistryNetworkError.__str__omitsstatus_codeandurl— Fixed.Added a
__str__override that appends[status: {code}]and[url: {url}]when those fields are set, falling through tosuper().__str__()for the base message + reference format. Existingstrassertions in the BDD scenarios were updated to match: e.g."server error [status: 500] [url: https://...]"and"timeout [ref: pkg_act_x] [url: https://bad.example.com]".10.
exception_for_statusnot exported from__all__— Fixed.Added
"exception_for_status"to__all__insrc/cleveractors/registry/__init__.pyand updated the import block.11.
RegistryError.__str__uses falsy check onoriginal_reference— Fixed.Changed
if self.original_reference:toif self.original_reference is not None:.12. Zero construction coverage for 8 typed subclass errors — Fixed.
Added one construction scenario per typed subclass in
registry_exceptions.feature(PackageNotFoundError through ConflictError), each asserting the message and instance type via@then("the error should be an instance of {cls_name}").13. Robot
exceptions.robotmissingisinstancechecks for 6 typed exceptions — Fixed.Added test cases for
InvalidPackageIdError,InvalidPackageReferenceError,VersionNotFoundError,ValidationError,AuthenticationRequiredError,AccessDeniedError, andConflictError. Theexception_is_instance_cleveragents_exceptionmapping inCleverActorsLib.pywas extended accordingly.Nits — All Fixed
N1. Undocumented omission of
InvalidPackageReferenceError— Fixed.Replaced the bare dict with a comment documenting the scope: maps server-side error-type strings to exception classes per §13.2 plus the project-specific
InvalidPackageReferencetype.N2.
_ERROR_TYPE_MAPcompleteness test lacks cardinality assertion — Fixed.Added
And the map should have exactly 9 keyswith a new step definitionstep_map_cardinality.N3.
_ERROR_TYPE_MAPshould beMappingProxyType— Fixed.Wrapped the dict in
types.MappingProxyType({...})and importedtypes.MappingProxyType.N4.
detailsdict stored by reference — Fixed.Changed
self.details = detailstoself.details = dict(details) if details is not None else None.Quality Gates
All CI checks pass:
nox -s lint— all checks passednox -s typecheck— 0 errors (1 pre-existinglangchain_google_genaiwarning only)nox -s unit_tests— 2292 scenarios, 10920 steps, 0 failuresnox -s integration_tests— 176 tests, 0 failuresnox -s coverage_report— clean (above 97%)nox -s security_scan— 0 findings10 files changed, +230 / -13 lines.
1847ae2f22287fab54e7287fab54e78c6a05e15d8c6a05e15dd1573919b2PR Review: !43 (Ticket #29)
Verdict: Request Changes
The exception hierarchy is well-designed and all 13 items from the previous review round have been addressed. However, three new issues were found in the updated code — two are bugs in
client.pyand one is an incomplete fix from the previous round — that must be resolved before merging.Critical Issues
None.
Major Issues
1.
client.py—msgbecomesNonewhen server returns"message": nullsrc/cleveractors/registry/client.py, line 104if "message" in error_block:only verifies key presence, not value. If the server returns{"error": {"type": "PackageNotFound", "message": null}},msgis overwritten withNone. This violates themessage: strtype contract of every exception class, andstr(exc)will silently return the string"None"— corrupting the primary diagnostic field.2.
client.py—urlis silently lost on 5xx responses without a structured error bodysrc/cleveractors/registry/client.py, line 119urlvariable is computed at the top of theexcept httpx.HTTPStatusErrorblock and correctly propagated in the structured-error path (line 116). However, the final fallbackraise exception_for_status(status, msg) from excdoes not passurl, andexception_for_status()has nourlparameter. A 500/502/503/504 response that lacks a structurederror.typefield loses theurl— even though the same response with a structured body would preserve it. The 500/503 BDD scenarios only assertstatus_codeand never checkurl, so this gap is undetected by tests.exception_for_status()to accept an optionalurlparameter, or (b) inline theRegistryNetworkErrorconstruction at the fallback:3.
robot/CleverActorsLib.py—exception_for_status_returnskeyword is defined but never invokedrobot/CleverActorsLib.py, line 416;robot/CleverActorsLib.py, line 44exception_for_statusimport as unused (issue #8). The fix was to add theexception_for_status_returnskeyword method. However, no.robotfile in the repository calls this keyword — the import at line 44 remains effectively dead and the fix is incomplete.robot/exceptions.robotthat exercise this keyword, e.g.:Minor Issues
4.
exceptions.py—exception_for_status()crashes withTypeErroron non-integerstatus_codesrc/cleveractors/registry/exceptions.py, line 158500 <= status_code < 600raisesTypeErrorifstatus_codeisNoneor any non-integer. Python's type annotations are not enforced at runtime. While current callers always pass an integer, a defensive guard would prevent a confusingTypeErrorfrom masking the actual error in future callers or test stubs.5.
client.py— InconsistentOptional[...]vs| Noneunion syntaxsrc/cleveractors/registry/client.py, line 96details: dict[str, Any] | None = Noneuses PEP 604 union syntax, while the rest of_request()consistently usesOptional[...](lines 45, 52, 54, 81, 95). This introduces a style inconsistency within the same function.Optional[dict[str, Any]] = Noneto match the existing local convention.Nits
N1.
exceptions.py—RegistryNetworkError.__str__produces a leading space whenmessageis emptysrc/cleveractors/registry/exceptions.py, lines 108–114message=""andstatus_code=500,__str__returns" [status: 500]"(leading space). Cosmetic, but affects log readability.base = f"[status: {self.status_code}]"whenmessageis empty.N2.
client.py—KeyErrorin theexcept (ValueError, KeyError, TypeError)clause is unreachablesrc/cleveractors/registry/client.py, line 110.get()orinchecks — neither raisesKeyErroron a standard dict. TheKeyErrorcatch is dead code that slightly widens the silent-swallow surface.except (ValueError, TypeError):.N3.
registry_exceptions_steps.py—step_map_valuehas a single-entry class mapfeatures/steps/registry_exceptions_steps.py, lines 241–250class_mapinsidestep_map_valueonly containsRegistryNetworkError. Any future test asserting a different value class via the{value_cls}placeholder will raiseKeyErrorfrom inside aThenstep.class_mapwith all 9 registry exception classes, or reuse the module-level class map already used by other step definitions.N4.
registry_http_client.feature—urlpropagation from HTTP responses is never assertedfeatures/registry_http_client.feature, lines 128–140status_codebut never asserturlon the raisedRegistryNetworkError. Theurlpropagation fix (previous review item #3) has no test backing it in the HTTP-client scenarios.And the raised error should have url "..."assertions to the 500/503 scenarios.Summary
The implementation is well-structured and the author has done thorough work addressing the previous round of review feedback. The exception hierarchy,
_ERROR_TYPE_MAP,__str__methods,MappingProxyTypewrapping, anddetailscopy are all correctly implemented. Spec compliance against §13.2 is complete.Three issues require changes before merging: (1) a silent
Nonemessage corruption when the server sends"message": null; (2)urlbeing dropped in the 5xx fallback path of_request(); and (3) theexception_for_status_returnsRobot keyword being defined but never exercised — leaving theexception_for_statusimport still effectively dead. The remaining minor and nit items are defense-in-depth improvements and style consistency fixes.Response to Review (rui.hu / hurui200320 — Round 3)
All nine issues from the latest review have been addressed in the amended commit
089ac59. Below is a point-by-point breakdown.Major Issues — All Fixed
1.
client.py—msgbecomesNonewhen server returns"message": null— Fixed.The
"message" in error_blockkey-presence check has been replaced with a value guard:This ensures that
msgis never overwritten withNone, preserving themessage: strtype contract of all exception classes even when the server sends"message": null. The fallbackmsg = f"HTTP {status}"from line 94 remains as the default.2.
client.py—urlis silently lost on 5xx responses without a structured error body — Fixed.Added an inline 5xx guard before the
exception_for_status()fallback:This approach was chosen over extending
exception_for_status()because: (a) it keeps the public API ofexception_for_status()stable — that function is a general-purpose status-to-exception mapper and adding aurlparameter would change its contract for all callers; (b) it keeps theurlpropagation logic contained within_request()where theurlvariable is computed and available. Non-5xx status codes (401, 403, 404, etc.) continue to useexception_for_status()as before.3.
robot/CleverActorsLib.py—exception_for_status_returnskeyword is defined but never invoked — Fixed.Added 8 Robot test cases to
robot/exceptions.robotcovering the completeexception_for_status()routing table: 400 →InvalidPackageIdError, 401 →AuthenticationRequiredError, 403 →AccessDeniedError, 404 →PackageNotFoundError, 409 →ConflictError, 500 →RegistryNetworkError, 503 →RegistryNetworkError, and 418 (unknown) →RegistryError. The keyword method was also fixed to usegetattr(result, "status_code", None)instead ofresult.status_codeto handle exception classes (ConflictError,RegistryError) that legitimately do not carry astatus_codeattribute.Minor Issues — All Fixed
4.
exceptions.py—exception_for_status()crashes withTypeErroron non-integerstatus_code— Fixed.Added a type guard at the top of the function:
This prevents a confusing
TypeErrorfrom the chain comparison500 <= status_code < 600whenstatus_codeisNoneor any non-integer, falling back cleanly to a plainRegistryError.5.
client.py— InconsistentOptional[...]vs| Noneunion syntax — Fixed.Changed
details: dict[str, Any] | None = Nonetodetails: Optional[dict[str, Any]] = Noneto match the existing local convention used throughout the rest of_request().Nits — All Fixed
N1.
exceptions.py—RegistryNetworkError.__str__produces a leading space whenmessageis empty — Fixed.Replaced the unconditional space prefix with a conditional approach:
When
messageis empty, no leading space is added, producing"[status: 500]"instead of" [status: 500]". Whenmessageis non-empty, the existing format is preserved:"server error [status: 500]".N2.
client.py—KeyErrorin theexcept (ValueError, KeyError, TypeError)clause is unreachable — Fixed.Narrowed the except clause from
(ValueError, KeyError, TypeError)to(ValueError, TypeError). All dict accesses in the guarded block use.get()orinchecks, neither of which raisesKeyError. TheValueErrorcatch remains necessary for malformed JSON from.json(), andTypeErrorprotects against non-dict responses.N3.
registry_exceptions_steps.py—step_map_valuehas a single-entry class map — Fixed.Populated the
class_mapwith all 9 registry exception classes:PackageNotFoundError,InvalidPackageIdError,InvalidPackageReferenceError,VersionNotFoundError,ValidationError,AuthenticationRequiredError,AccessDeniedError,ConflictError, andRegistryNetworkError. Any future step asserting a value class via the{value_cls}placeholder will resolve correctly.N4.
registry_http_client.feature—urlpropagation from HTTP responses is never asserted — Fixed.Added
And the raised error should have url setassertions to both the 500 and 503 scenarios. A corresponding step definitionstep_check_error_url_setwas added toregistry_http_client_steps.pythat assertscontext.error.url is not None. A specific URL string assertion was not used because the mock HTTP responses instantiatehttpx.HTTPStatusError(request=MagicMock(), ...), which produces a MagicMock-derived URL string rather than a deterministic URL. Theurl setassertion correctly verifies thaturlpropagation was exercised without coupling the test to mock implementation details.Quality Gates
nox -s lint— all checks passednox -s typecheck— 0 errors (1 pre-existinglangchain_google_genaiwarning only)nox -s unit_tests— 2292 scenarios, 10922 steps, 0 failuresnox -s integration_tests— 184 tests, 0 failuresd1573919b2089ac59953089ac5995325b88da01cPR Review: !43 (Ticket #29)
Verdict: Request Changes
The exception hierarchy is well-designed and the author has done thorough work across three rounds of review. All 13 items from Round 1 and all 9 items from Round 2 have been correctly addressed. However, three new bugs remain in the updated
_request()method — two correctness issues and one that can cause an unhandledTypeErrorto escape the error-handling boundary — that must be fixed before merging.Critical Issues
None.
Major Issues
1.
client.py—detailssilently dropped in the 5xx fallback pathsrc/cleveractors/registry/client.py, lines 120–123error.type(e.g. a proxy returning{"error": {"message": "gateway error", "details": {"trace_id": "abc"}}}), thedetailsdict is correctly extracted on line 107 but then dropped by the fallback constructor: This is an internal inconsistency — two code paths for the same 5xx scenario produceRegistryNetworkErrorinstances with differentdetailsvalues depending solely on whether the body has anerror.typefield.detailsin the fallback:raise RegistryNetworkError(msg, details=details, status_code=status, url=url) from exc2.
client.py—msgnot guarded againstNonein the top-level-message fallback branchsrc/cleveractors/registry/client.py, line 109error_blockbranch (lines 103–105) correctly guards withif new_msg is not None:before overwritingmsg. The siblingelifbranch does not: If the server returns{"message": null}(noerrorblock),msgbecomesNone. Every exception class storesself.message = message(typed asstr), and__str__returnsself.messagedirectly — sostr(exc)returns the string"None", corrupting the primary diagnostic field. This is an asymmetry with the guard added in Round 3 for theerror_blockpath.elif "message" in body and body["message"] is not None: msg = body["message"]3.
client.py— Non-hashableerror_typefrom server causes unhandledTypeErrorsrc/cleveractors/registry/client.py, line 113error_typeis obtained viaerror_block.get("type")and can be any JSON value. Thetry/except (ValueError, TypeError)block on lines 97–112 does not cover line 113. If a hostile or buggy server returns{"error": {"type": ["PackageNotFound"]}}, thenerror_typeis a list (truthy), anderror_type in _ERROR_TYPE_MAPraisesTypeError: unhashable type: 'list'— which escapes_request()entirely, breaking the error-handling contract.strtype guard:if isinstance(error_type, str) and error_type in _ERROR_TYPE_MAP:Minor Issues
4.
registry_http_client.feature— No test formsg=Nonein the top-level-message fallback pathfeatures/registry_http_client.feature(missing scenario)"message": nullin theerror_blockpath has no BDD scenario. Theelif "message" in bodypath (Major #2 above) also has no test. A regression on either guard would go undetected.{"message": null}(noerrorblock) asserting the message falls back to"HTTP {status}".5.
registry_exceptions.feature— No test forexception_for_status()non-integer type guardfeatures/registry_exceptions.feature(missing scenario)if not isinstance(status_code, int): return RegistryError(message)has no BDD scenario exercising it. A regression removing the guard would not be caught.exception_for_statuswithNoneor a string and asserting the result is a plainRegistryError.6.
registry_http_client_steps.py— Negation tuple instep_check_registry_erroris incompletefeatures/steps/registry_http_client_steps.py, lines 410–424VersionNotFoundError,InvalidPackageReferenceError, andValidationErrorare missing from the tuple. A future change making 418 map to any of these three would silently pass.assert type(context.error) is RegistryErrorfor an exact-class check.Nits
N1.
exceptions.py—detailsshallow-copy semantics not documentedsrc/cleveractors/registry/exceptions.py, lines 39–41dict(details)is a shallow copy. Nested mutable values (e.g. a list insidedetails) can still be mutated by the caller after construction. This is acceptable, but worth a one-line note in thedetailsattribute docstring to set expectations.N2.
exceptions.py— Module docstring count is ambiguoussrc/cleveractors/registry/exceptions.py, lines 3–4InvalidPackageReferenceErrorandRegistryNetworkError" which reads as 10, but there are 9 typed subclasses (8 spec types whereInternalServerErrormaps toRegistryNetworkError, plusInvalidPackageReferenceError). Reword for clarity.N3.
registry_exceptions_steps.py—class_mapliteral duplicated in 4 step functionsfeatures/steps/registry_exceptions_steps.py, lines 170–178, 192–204, 243–253, 306–315_CLASS_MAPconstant to eliminate the duplication and reduce maintenance risk.N4.
robot/CleverActorsLib.py— 3 overlapping class-mapping dictsrobot/CleverActorsLib.py, lines 377–391, 398–411, 417–425_EXCEPTION_CLASS_MAPand reference it from all three.N5.
registry_http_client.feature—urlpropagation fromhttpx.RequestErrornot assertedfeatures/registry_http_client.feature, lines 102–106status_code is Noneorurl is None(sincehttpx.ReadTimeoutis constructed without a request object in the mock). The Round 1 fix for URL extraction from transport errors has no test backing it.And the error status_code should be NoneandAnd the error url should be Noneto the timeout scenario.Summary
The exception hierarchy itself is clean, well-typed, and fully spec-compliant against §13.2. All 13 items from Round 1 and all 9 items from Round 2 have been correctly addressed — the
_ERROR_TYPE_MAPcompleteness,MappingProxyTypeimmutability,RegistryNetworkError.__str__,detailspropagation,status_code/urlpropagation, andmessage: nullguard in theerror_blockpath are all correctly implemented.Three new bugs remain in the updated
_request()method:detailsis dropped in the 5xx fallback path (inconsistent with the_ERROR_TYPE_MAPpath),msgcan be set toNonevia the top-level"message"fallback (asymmetric with the Round 3 guard), and a non-hashableerror_typefrom the server can cause an unhandledTypeErrorto escape the error-handling boundary. All three are straightforward one-line fixes.Response to Review Round 3 — rui.hu (2026-06-11)
All issues from this review round have been addressed. The three major bugs, three minor issues, and five nits are resolved.
Major Issues — All Fixed
1.
detailssilently dropped in the 5xx fallback path_request()line 120-123 now passesdetails=detailsin the 5xx fallbackRegistryNetworkErrorconstructor, matching the_ERROR_TYPE_MAPcode path.2.
msgnot guarded againstNonein the top-level-message fallback branchelif "message" in body:branch now readselif "message" in body and body["message"] is not None:, mirroring thenew_msg is not Noneguard already present in theerror_blockpath.3. Non-hashable
error_typefrom server causes unhandledTypeErrorif isinstance(error_type, str) and error_type in _ERROR_TYPE_MAP:. Non-string values (lists, numbers, null) fall through to the status-code fallback instead of escaping asTypeError.Minor Issues — All Fixed
4. No test for
msg=Nonein the top-level-message fallback path"Response with null message preserves default HTTP status message"inregistry_http_client.featurethat sends{"message": null}(no error block) with status 500, and asserts the message falls back to"HTTP 500".5. No test for
exception_for_status()non-integer type guard"non-integer status_code falls back to RegistryError"scenario inregistry_exceptions.featurecallingexception_for_statuswith"not-a-number"and asserting the result is a plainRegistryErrorwith the original message preserved. The step definition usestyping.cast(int, code)to satisfy Pyright without# type: ignore.6. Negation tuple in
step_check_registry_errorincompleteVersionNotFoundError,InvalidPackageReferenceError, andValidationErrorto the negation tuple. The full import ofInvalidPackageReferenceErrorwas also added toregistry_http_client_steps.py.Nits — Addressed
N1.
detailsshallow-copy semantics — Added# shallow copycomment on thedict(details)line inRegistryError.__init__.N2. Module docstring count ambiguity — Reworded to clarify: 8 spec standard types, plus 1 project-specific, totals 9 typed subclasses.
N3.
class_mapduplicated in 4 step functions — Hoisted into a single module-level_CLASS_MAP: dict[str, type]inregistry_exceptions_steps.py, referenced by all four functions.N4. 3 overlapping class-mapping dicts in CleverActorsLib — Hoisted into a single module-level
_EXCEPTION_CLASS_MAP: dict[str, type]inrobot/CleverActorsLib.py, referenced by all three methods.N5.
urlpropagation fromhttpx.RequestErrornot asserted — Added"the raised error should have url None"assertion to the timeout scenario, with corresponding step definition inregistry_http_client_steps.py.Quality Gate Status
nox -s lintnox -s typechecknox -s unit_testsnox -s integration_testsnox -s coverage_reportexceptions.py100%All changes are in a single working-tree delta against commit
25b88da. Ready for re-review.25b88da01c73ea50454973ea504549e3847ce07aPR Review: !43 (Ticket #29)
Verdict: Request Changes
The exception hierarchy (
exceptions.py) is clean, fully spec-compliant against §13.2, and all 22 issues from the three previous review rounds have been correctly addressed. The_ERROR_TYPE_MAP,MappingProxyTypeimmutability,RegistryNetworkError.__str__,detailscopy,status_code/urlpropagation, andmessage: nullguard are all correctly implemented.Three issues require changes before merging: (1)
detailsis still dropped in the non-5xx fallback path — the same class of bug as the Round 3 fix, applied incompletely; (2)RegistryClient._get_client()is missing theasyncio.Lockthat the project standardized for this exact pattern inReferenceResolver; and (3) the "404 without error type" scenario is a no-op that asserts nothing while claiming coverage.Critical Issues
None.
Major Issues
1.
detailssilently dropped in the non-5xx fallback pathsrc/cleveractors/registry/client.py, line 124detailsin the 5xx fallback path (client.py:120–123). However, the same bug still exists for non-5xx codes:raise exception_for_status(status, msg)has nodetailsparameter, so anydetailsextracted from the structured error body (lines 106–108) are silently discarded for 4xx responses with unrecognizederror.typevalues. This is the same class of bug as the one fixed in Round 3 for 5xx — the fix was incomplete.exception_for_status()to accept an optionaldetailsargument, or (b) inline the non-5xx fallback symmetrically to the 5xx branch:raise RegistryError(msg, details=details) from exc.2.
RegistryClient._get_client()lacks theasyncio.Lockthat the project standardized for this patternsrc/cleveractors/registry/client.py, lines 69–79ReferenceResolver._get_client()(reference_resolver.py:64–75) uses anasyncio.Lockto protect the check-and-create sequence, explicitly introduced to fix a race condition. The newRegistryClient._get_client()uses the identical unprotected pattern (if self._client is None or self._client.is_closed: ... self._client = httpx.AsyncClient(...)). While single-threaded asyncio without anawaitbetween check and create is safe today, the project has chosen to standardize on lock-protected initialization for this exact pattern. Any future change introducing anawaitin the initialization path would silently create a real race that leakshttpx.AsyncClientinstances.ReferenceResolverpattern: addself._async_lock: Optional[asyncio.Lock] = Nonein__init__, a_get_async_lock()helper, and wrap the check-and-create body withasync with self._get_async_lock():.3. "404 without error type in body" scenario is a no-op — asserts nothing
features/registry_http_client.feature, lines 178–180Whensteps (create client, set up mock) and noThenstep at all. It never callsget_packageto trigger the response and never asserts thatPackageNotFoundErroris raised. Behave marks it as "passed" because no step failed, but it provides zero coverage of the named behavior. This is worse than no test because it gives a false sense of coverage.Minor Issues
4. HTTPS logging step definitions construct a new client, defeating test isolation
features/steps/registry_http_client_steps.py, lines 466–498step_client_logs_cleartext_warning,step_client_logs_https_info,step_client_no_https_warnings) instantiate a brand-newRegistryClientinside theThenstep'swith patch(...)block, never referencingcontext.clientcreated in the precedingWhenstep. The test only verifies that "a client constructed with these parameters in a fresh call would log" — which is tautological since the step itself performs the construction. The originalWhenstep's client is never tested.Whenstep (before construction) and assert against it in theThenstep, so the test actually validates the behavior of the client created by the scenario'sWhenclause.5. "Client with API key includes authorization header" scenario does not verify the header
features/registry_http_client.feature, lines 182–186the registry result body equals '{"content": "ok"}'— it verifies the response body, not theAuthorizationheader. A client that drops theAuthorizationheader entirely would still pass this scenario. The spec §9.2 authentication behavior is not actually tested.Authorization: Bearer my-api-keyis present.6.
detailspropagation from structured error body is never tested end-to-endfeatures/registry_http_client.feature(missing scenario)client.pylines 106–108 extractdetailsfrom the structured error body and pass it to the exception constructor. No BDD scenario exercises this path. A regression dropping thedetailspropagation would go undetected.7.
get_packagedoes not URL-encodepackage_idsrc/cleveractors/registry/client.py, line 165resolve_packagecarefully appliesquote(namespace, safe="")andquote(name, safe="")(lines 202–203), butget_packageinterpolatespackage_iddirectly:f"/packages/{package_id}". Apackage_idcontaining/,?, or#would produce an unintended URL. This is an inconsistency in input handling.encoded_id = quote(package_id, safe=""); return await self._request(f"/packages/{encoded_id}")8.
exc.request is not Nonecheck is misleading — the property raises, not returnsNonesrc/cleveractors/registry/client.py, line 128RequestError.requestis a@propertythat raisesRuntimeErrorwhen_requestis unset — it never returnsNone. Theif exc.request is not None:check therefore never evaluates toFalse; when_requestis unset, theRuntimeErroris caught by the outerexcept RuntimeError: pass. The code produces the correct result, but the check is misleading to readers.if getattr(exc, "_request", None) is not None:to make the intent explicit, or add a comment explaining the property's exception-raising behavior.Nits
N1. Type annotation convention inconsistent between
exceptions.pyandclient.pysrc/cleveractors/registry/exceptions.py(usesT | None) vssrc/cleveractors/registry/client.py(usesOptional[T])N2.
_CLASS_MAP/_EXCEPTION_CLASS_MAPtyped too generically asdict[str, type]features/steps/registry_exceptions_steps.py:29,robot/CleverActorsLib.py:58dict[str, type[Exception]](ortype[CleverAgentsException]) for better static type safety.N3.
typing.cast(int, code)in step definition is misleadingfeatures/steps/registry_exceptions_steps.py, line 189cast()does not convert values at runtime — it only silences the type checker. The value passed is still astr. Add a comment:# intentionally non-int to exercise the isinstance guard in exception_for_status.N4.
asyncio.get_event_loop().run_until_complete()is deprecated in Python 3.10+features/steps/registry_http_client_steps.py(lines 167, 179, 253, 268, and others)asyncio.run(_call())to avoid deprecation warnings.N5.
RegistryNetworkError.__str__leading-space logic is subtlesrc/cleveractors/registry/exceptions.py, lines 110–118prefix = " " if base else ""pattern is correct but requires translating string truthiness to understand. Aparts: list[str]+" ".join(parts)approach would be more readable.Response to Review Round 5 — rui.hu (2026-06-11)
All three major bugs, five minor issues, and four of five nits have been addressed. Below is a point-by-point breakdown of what was done, what was not, and the justification for each decision.
Major Issues — All Fixed
1.
detailssilently dropped in the non-5xx fallback pathFixed. The reviewer correctly identified that the Round 3 fix for
detailspropagation was incomplete —detailswere preserved in the 5xx path but dropped at theexception_for_status()fallback for non-5xx codes.Solution: Extended
exception_for_status()to accept an optionaldetailsparameter (def exception_for_status(status_code, message, details=None)). All typed-exception constructors inside the function now passdetails=details. This was chosen over the alternative of inliningRegistryError(msg, details=details)at the fallback site because:PackageNotFoundError) is the correct behaviour for all error paths. A rawRegistryErrorat the 4xx fallback would silently degrade the exception type for unrecognisederror.typevalues, breaking the “catch any 404” pattern.detailstoexception_for_status()makes the API consistent: all callers now have the same ability to propagate details, not just the single_request()call site.2.
RegistryClient._get_client()lacks theasyncio.Lockthat the project standardised for this patternFixed. Mirrored the
ReferenceResolver._get_client()pattern exactly:self._async_lock: Optional[asyncio.Lock] = Nonein__init__._get_async_lock()helper with lazy initialisation (avoids creating aLockoutside a running event loop).async with self._get_async_lock():.This prevents a real race condition if any future change introduces an
awaitbetween theis Nonecheck and thehttpx.AsyncClientconstruction — the exact scenario thatReferenceResolver’s lock was designed to guard against.3. “404 without error type in body” scenario is a no-op — asserts nothing
Fixed. The scenario originally had two
Whensteps (create client, set up mock) with noThenstep — it never calledget_packageand never asserted the raised exception. Behave marked it “passed” but it exercised zero code paths. Added:Minor Issues — All Fixed
4. HTTPS logging step definitions construct a new client, defeating test isolation
Fixed. The reviewer correctly identified that all three HTTPS logging
Thensteps instantiated a brand-newRegistryClientinside awith patch(...)block, never referencingcontext.clientfrom the precedingWhenstep. This made the tests tautological.Solution: Refactored the test structure:
Whenstep definitions (…and capture logsvariants) create the client whilewith patch(...)captures the logger mocks and stores them on the test context.Thensteps now assert againstcontext._log_warn_mock/context._log_info_mock— the very mocks that captured logging during client construction in theWhenstep.Thensteps were removed. The unusedstep_create_insecure_clientwas also removed (replaced by the capture-logs variant).5. “Client with API key includes authorization header” scenario does not verify the header
Fixed. The original scenario only asserted the response body
{"content": "ok"}— a client that silently drops theAuthorizationheader would pass. Changed the scenario:Whenstep now patcheshttpx.AsyncClientand capturescall_args, storing oncontext._captured_client_kwargs.Then the Authorization header should be "Bearer my-api-key"step definition assertsheaders["Authorization"] == expected.6.
detailspropagation from structured error body is never tested end-to-endFixed. Added a new BDD scenario:
The
And the error details should equalstep reuses the existing definition fromregistry_exceptions_steps.py:137, which assertscontext.error.details— proving thatdetailstravels from the mock HTTP response through_request()→exception_for_status()→ the raised exception.7.
get_packagedoes not URL-encodepackage_idFixed. Added
encoded_id = quote(package_id, safe=""); return await self._request(f"/packages/{encoded_id}")toget_package(), matching the URL-encoding already applied inresolve_package().8.
exc.request is not Nonecheck is misleading — the property raises, not returnsNoneFixed. Changed both occurrences in
_request():if getattr(exc, "request", None) is not None:if getattr(exc, "_request", None) is not None:if exc.request is not None:if getattr(exc, "_request", None) is not None:In httpx,
RequestError.requestis a@propertythat raisesRuntimeErrorwhen_requestis unset — it never returnsNone. Accessing the private_requestattribute directly prevents the misleading impression that the check might evaluate toFalse(it will either return the object or theexcept RuntimeError: passblock will catch the exception). The result is identical, but the intent is now explicit.Nits — 4 of 5 Addressed
N1. Type annotation convention inconsistent between
exceptions.pyandclient.pyNot addressed.
exceptions.pyuses PEP 604T | Noneunion syntax throughout (10 occurrences);client.pyusesOptional[T](15+ occurrences). Both conventions are valid, PEP-compliant, and functionally identical withfrom __future__ import annotations(present in both files). Changing either file would touch 10–15 lines purely for cosmetic consistency. Given both files were modified in the same commit, both conventions represent the current project state. I recommend deferring a unification decision to a separate style-only PR that can be discussed independently of this feature’s bug fixes.N2.
_CLASS_MAP/_EXCEPTION_CLASS_MAPtyped too generically asdict[str, type]Fixed. Tightened both:
features/steps/registry_exceptions_steps.py:29:dict[str, type]→dict[str, type[CleverAgentsException]]robot/CleverActorsLib.py:58:dict[str, type]→dict[str, type[CleverAgentsException]]N3.
typing.cast(int, code)in step definition is misleadingFixed. Added comment:
N4.
asyncio.get_event_loop().run_until_complete()is deprecated in Python 3.10+Fixed. Replaced all 11 occurrences in
features/steps/registry_http_client_steps.pywithasyncio.run().N5.
RegistryNetworkError.__str__leading-space logic is subtleFixed. Refactored from conditional
prefixlogic to aparts: list[str]+" ".join(parts)approach:This avoids the
" " if base else ""fnord and is trivially verifiable: whenmessageis empty,super().__str__()returns"", and" ".join(...)strips the leading space naturally.An unused
RegistryErrorimport inclient.pywas also removed (replaced by the extendedexception_for_statuscall).Quality Gate Status
nox -s lintnox -s typechecklangchain_google_genaiwarning)nox -s unit_testsnox -s integration_testsnox -s coverage_reportexceptions.py100%,client.py94.7%All changes amended into commit
ef0e709. Ready for re-review.e3847ce07aef0e7091b6ef0e7091b64288390e81Re-Review: PR #43 — Round 6
Reviewer: brent.edwards
Head commit:
4288390e81a4694921945ba7008776039aa4ed27All 3 major issues, all 5 minor issues, and 4 of 5 nits from Round 5 have been correctly addressed. The exception hierarchy,
_ERROR_TYPE_MAP,RegistryNetworkError.__str__,asyncio.Lock,detailspropagation, URL encoding, and test coverage improvements are all well done.However, two new blocking issues were found during this review — one of which is also the direct cause of the alarming 252 changed files / 74,639 lines figure this PR shows. These must be resolved before merge.
Round 5 issues: all resolved
All items verified as correctly fixed in the current head:
detailsdropped in non-5xx fallback):exception_for_status()now acceptsdetailsand it is passed at the call-site — fixedasyncio.Lockin_get_client()):_get_async_lock()helper andasync with self._get_async_lock():added — fixedThen a PackageNotFoundError should be raisedstep added — fixedcapturing headersstep andAuthorizationheader assertion added — fixeddetailspropagation never tested): end-to-end scenario withAnd the error details should equaladded — fixedget_packagedoes not URL-encode):quote(package_id, safe="")applied — fixedexc.request is not None): replaced withgetattr(exc, "_request", None) is not Nonein both exception handlers — fixed_CLASS_MAPtype tightened,typing.castcomment added,asyncio.run()in steps,RegistryNetworkError.__str__usespartslist — all fixedNEW BLOCKERS
BLOCKER 1 —
.opencode/skills/accidentally committed (root cause of 252 files / 74,639 lines)The overwhelming majority of the 252 changed files and 74,639 added lines are internal AI agent configuration files under
.opencode/skills/that were accidentally committed into this feature branch. These are not project code, tests, or documentation.Actual code changes in this PR: 12 files —
exceptions.py,client.py,registry/__init__.py,features/registry_exceptions.feature,features/registry_http_client.feature,features/steps/registry_exceptions_steps.py,features/steps/registry_http_client_steps.py,robot/CleverActorsLib.py,robot/exceptions.robot,robot/version.robot,CHANGELOG.md, andactor-registry-standard.md. The remaining 240 files are.opencode/skills/content.The
.opencode/skills/directory holds skill documentation forcleveractors-contributing,cleveragents-spec,cleverthis-guidelines,forgejo-api,programming-patterns, andtemplating-vault— all internal AI coding-assistant tooling. The.gitignorehas no entry for.opencode/, so git tracked all of it.Note: the PR is currently flagged
mergeable: falseby Forgejo — the massive unintended diff is a likely contributor.To fix:
.opencode/to.gitignore(or at minimum.opencode/skills/)git rm -r --cached .opencode/skills/ && git commit -m "chore: remove accidentally committed .opencode/skills from branch"BLOCKER 2 —
actor-registry-standard.mdplaced at repository root instead ofdocs/CONTRIBUTING.mdis explicit: all documentation goes in/docs/. The root is reserved for widely-recognised project conventions (pyproject.toml,noxfile.py,CHANGELOG.md, etc.).Please move to
docs/actor-registry-standard.md.CI status
CI is pending for the current head (
4288390). Most jobs show prior completions (lint, typecheck, security, unit_tests, build, integration_tests all green), butcoverageandstatus-checkare listed as "Blocked by required conditions" for this specific commit. The author self-reports 97.14% — which satisfies the gate — but this must be confirmed by CI completing on the current commit before merge.Remaining Nit N1 (acknowledged, not blocking)
The author stated in their Round 5 response that N1 was left intentionally. Acknowledged. For the record:
Optional[T]inclient.pyvsT | Noneinexceptions.pywithin the same subpackage remains inconsistent. Not blocking merge.Minor CHANGELOG inaccuracy (not blocking)
The Round 5 entry says: "Removed unused
exception_for_statusimport fromclient.py". This is incorrect —exception_for_statusIS imported and IS used inclient.py(raise exception_for_status(status, msg, details=details) from exc). This appears to be a copy-paste error referring to the Round 2robot/CleverActorsLib.pyimport issue. Please correct the CHANGELOG entry.Summary
The exception hierarchy code quality is excellent. All 22 items from Rounds 1–4 and all 8 items from Round 5 have been correctly resolved. Two new issues — one accidental commit of internal tooling and one misplaced documentation file — must be addressed before merge.
To unblock merge:
.opencode/skills/from the branch; add.opencode/to.gitignoreactor-registry-standard.md→docs/actor-registry-standard.mdexception_for_statusimport fromclient.py" is wrong)Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
@ -0,0 +1,2054 @@---BLOCKER — accidental commit of internal AI tooling files.
This entire
.opencode/skills/directory (240 files, ~74,400 lines) consists of internal opencode AI agent configuration — skill documents forcleveractors-contributing,cleveragents-spec,cleverthis-guidelines,forgejo-api,programming-patterns, andtemplating-vault. These are not project source, tests, or documentation.This is the direct cause of the PR showing 252 changed files and 74,639 lines. The
.gitignorehas no.opencode/entry, so git tracked these files.Fix:
.opencode/to.gitignoregit rm -r --cached .opencode/skills/ && git commit -m "chore: remove accidentally committed .opencode/skills"Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
@ -0,0 +1,523 @@# The Package Registry StandardBLOCKER — documentation file placed at repository root instead of
docs/.CONTRIBUTING.mdis explicit: documentation goes in/docs/. The repository root is reserved for widely-recognised project conventions (pyproject.toml,noxfile.py,CHANGELOG.md, etc.).Please move to
docs/actor-registry-standard.md.Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
Re-review submitted as review #9580 (REQUEST_CHANGES).
All Round 5 items confirmed resolved. Two new blockers found: (1)
.opencode/skills/accidentally committed into the branch — root cause of the 252 changed files / 74,639 lines; (2)actor-registry-standard.mdplaced at repo root instead ofdocs/. See the review for full details and fix instructions.Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
Response to Review (brent.edwards / #9580)
All three blocking issues have been addressed in the amended commit
4c82788.BLOCKER 1 —
.opencode/skills/accidentally committedFixed. The entire
.opencode/skills/directory has been removed from git tracking viagit rm -r --cached .opencode/skills/. The directory remains on disk (it is needed by the AI coding assistant tooling) but is no longer part of the committed history. The.gitignorewas intentionally left unchanged per instruction.Result: The PR now contains exactly 12 files / 1,493 insertions — the actual code, test, and documentation changes — instead of the 252 files / 74,639 lines that included the accidentally committed internal tooling. The
mergeable: falseflag should now be recalculated by Forgejo on the next push.BLOCKER 2 —
actor-registry-standard.mdat repository rootFixed. Moved from root to
docs/actor-registry-standard.mdviagit mv. This aligns withCONTRIBUTING.mdfile organization rules which state all documentation belongs in/docs/. The Package Registry Standard specification content is unchanged.CHANGELOG inaccuracy
Fixed. Removed the statement
Removed unused exception_for_status import from client.pyfrom the "Registry Error Hierarchy Fixes (PR Review rui.hu Round 5)" entry. This claim was incorrect —exception_for_statusis both imported (line 25) and called (line 132) insrc/cleveractors/registry/client.py. The review correctly identified this as a copy-paste error.What was NOT changed
Optional[T]vsT | Noneinconsistency betweenexceptions.pyandclient.py): Left intentionally as acknowledged in the prior round. Not blocking merge..gitignore: Intentionally not modified. The.opencode/directory remains untracked on disk without a.gitignoreentry.Verification
4288390e814c827888d1Re-Review: PR #43 — Round 7 — APPROVED
Reviewer: brent.edwards
Review commit:
7adee05afd37294c8a54e2dd0ad410233d03b4fdCurrent branch HEAD:
4c827888d1fd35daf30b2c9879427c1dc23307dcAll three Round 6 blockers have been correctly resolved. The PR is approved to merge once CI completes on the current head.
Round 6 blockers: all resolved
BLOCKER 1 —
.opencode/skills/accidentally committedFixed. The directory was removed from git tracking via
git rm -r --cached .opencode/skills/. The PR now shows exactly 12 changed files / 1,493 insertions — only the actual code, test, and documentation changes.BLOCKER 2 —
actor-registry-standard.mdat repository rootFixed. Moved to
docs/actor-registry-standard.md. No file exists at the root. ✓CHANGELOG inaccuracy
Fixed. The incorrect statement "Removed unused
exception_for_statusimport fromclient.py" has been removed from the Round 5 entry. The CHANGELOG is now accurate. ✓Full review summary
All 22 items from Rounds 1–4, all 8 items from Round 5, and all 3 items from Round 6 have been correctly resolved across this PR. The exception hierarchy itself is clean, well-typed, and spec-compliant:
RegistryErrorbase withmessage,details,original_reference, and__str__✓InvalidPackageReferenceError) ✓_ERROR_TYPE_MAPwithMappingProxyType, correct 9-entry mapping,InvalidPackageReferenceErrorincluded ✓RegistryNetworkErrorcarriesstatus_code,url, overrides__str__with parts-list join ✓exception_for_status()withdetailsparameter, non-integer guard, correct status routing ✓RegistryClient._get_client()protected byasyncio.Lockmatching theReferenceResolverpattern ✓_request()propagatesurl,details, andstatus_codethrough all error paths ✓get_package()URL-encodespackage_id✓__str__, status mapping, inheritance, map completeness, subclass construction, HTTP client paths) ✓issubclasschecks, 9isinstancechecks, 8exception_for_statusrouting tests ✓exception_for_statusexported fromcleveractors.registry.__all__✓InternalServerErrorremoved from hierarchy,InternalServerError→RegistryNetworkErrorin_ERROR_TYPE_MAP✓docs/actor-registry-standard.mdadded as the normative specification ✓CI
CI for commit
7adee05(identical code to current head):state: success, 9/9 checks passed — lint, typecheck, security, unit_tests, integration_tests, build, quality, coverage (97.14% ≥ 97% gate), status-check all green.CI for the current head
4c82788is pending — this commit contains onlygit rm -r --cached .opencode/skills/andgit mv actor-registry-standard.md docs/. No code or test changes. CI will pass.Recommendation (non-blocking, post-merge follow-up)
The
.opencode/skills/removal was done viagit rm -r --cachedwithout adding.opencode/to.gitignore. The files remain on disk as untracked and are not in the commit history — the PR blocker is resolved. However, without a.gitignoreentry, a futuregit add .could accidentally re-commit them. Consider creating a small follow-up issue to add.opencode/to.gitignore.Nit N1 (acknowledged, intentionally deferred)
Optional[T]inclient.pyvsT | Noneinexceptions.py— author has stated this is intentionally deferred. Acknowledged. The inconsistency does not affect correctness or CI.Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
APPROVED review submitted as review #9582. All blockers across all 6 rounds of review have been resolved. The exception hierarchy is clean, spec-compliant, and fully tested. CI is green on the identical-code commit (
7adee05). Ready to merge once CI completes on the current head (4c82788).Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
4c827888d1e8bd348c77