cleveragents/cleveragents-core
cleveragents/cleveragents-core
4
0
Fork 3
Code Issues 6.1k Pull requests 489 Projects Releases Packages Wiki Activity Actions

docs(spec): fix custom resource type YAML example to match implementation format #5717

Closed
HAL9000 wants to merge 1 commit from spec/fix-resource-type-yaml-format-5622 into master
pull from: spec/fix-resource-type-yaml-format-5622
merge into: cleveragents:master
Conversation 4 Commits 1 Files changed 1 +44 -48
HAL9000 commented 2026-04-09 08:47:40 +00:00
Owner
Copy link

Summary

This PR fixes the custom resource type YAML example in the §Custom Resource Types section to use the correct format that matches the implementation.

Problem

Issue #5622 identified that the spec's §Custom Resource Types section showed an incorrect YAML format that is incompatible with ResourceTypeSpec.from_config() in resource_type.py.

Incompatibilities Fixed

Spec (wrong) Implementation (correct)
resource_type: wrapper Flat format (no wrapper)
cleveragents: version: "3.0" header Not supported
physical_or_virtual: physical resource_kind: physical
cli_arguments: cli_args:
allowed_parent_types: parent_types:
handler: DatabaseHandler (string) handler: {class: ..., module: ...} (object)
capabilities.readable/writable/sandboxable/checkpointable capabilities.read/write/sandbox/checkpoint

What Changed

Updated the database.yaml example in §Custom Resource Types (~line 24504) to use the correct flat format.

Authoritative Reference

The §Resource Type Configuration Files section (~line 34780) already uses the correct format and serves as the authoritative reference. This PR aligns the §Custom Resource Types example with that section.

Scope

  • Change type: Documentation fix — example YAML updated to correct format
  • Risk: None — documentation only
  • Breaking changes: None

Relates to #5622.

Automated by CleverAgents Bot
Supervisor: Architecture | Agent: architect | Instance: architect-1

## Summary This PR fixes the custom resource type YAML example in the §Custom Resource Types section to use the correct format that matches the implementation. ## Problem Issue #5622 identified that the spec's §Custom Resource Types section showed an incorrect YAML format that is incompatible with `ResourceTypeSpec.from_config()` in `resource_type.py`. ### Incompatibilities Fixed | Spec (wrong) | Implementation (correct) | |---|---| | `resource_type:` wrapper | Flat format (no wrapper) | | `cleveragents: version: "3.0"` header | Not supported | | `physical_or_virtual: physical` | `resource_kind: physical` | | `cli_arguments:` | `cli_args:` | | `allowed_parent_types:` | `parent_types:` | | `handler: DatabaseHandler` (string) | `handler: {class: ..., module: ...}` (object) | | `capabilities.readable/writable/sandboxable/checkpointable` | `capabilities.read/write/sandbox/checkpoint` | ## What Changed Updated the `database.yaml` example in §Custom Resource Types (~line 24504) to use the correct flat format. ## Authoritative Reference The §Resource Type Configuration Files section (~line 34780) already uses the correct format and serves as the authoritative reference. This PR aligns the §Custom Resource Types example with that section. ## Scope - **Change type**: Documentation fix — example YAML updated to correct format - **Risk**: None — documentation only - **Breaking changes**: None Relates to #5622. --- **Automated by CleverAgents Bot** Supervisor: Architecture | Agent: architect | Instance: architect-1
docs(spec): fix custom resource type YAML example to match implementation format
All checks were successful
CI / lint (pull_request) Successful in 22s
Details
CI / security (pull_request) Successful in 56s
Details
CI / typecheck (pull_request) Successful in 1m17s
Details
CI / quality (pull_request) Successful in 30s
Details
CI / build (pull_request) Successful in 26s
Details
CI / helm (pull_request) Successful in 25s
Details
CI / push-validation (pull_request) Successful in 15s
Details
CI / e2e_tests (pull_request) Successful in 3m16s
Details
CI / integration_tests (pull_request) Successful in 7m36s
Details
CI / unit_tests (pull_request) Successful in 8m30s
Details
CI / docker (pull_request) Successful in 2m31s
Details
CI / coverage (pull_request) Successful in 12m19s
Details
CI / status-check (pull_request) Successful in 1s
Details
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / benchmark-regression (pull_request) Successful in 57m40s
Details
6175e4aba0 
Relates to #5622

The §Custom Resource Types section showed an incorrect YAML format with:
- resource_type: wrapper (not supported by implementation)
- cleveragents: version header (not supported)
- physical_or_virtual: (implementation uses resource_kind:)
- cli_arguments: (implementation uses cli_args:)
- allowed_parent_types: (implementation uses parent_types:)
- handler: DatabaseHandler (string, implementation expects object with class/module)
- capabilities.readable/writable/sandboxable/checkpointable (implementation uses read/write/sandbox/checkpoint)

Updated the database.yaml example to use the correct flat format that matches
ResourceTypeSpec.from_config() in resource_type.py:
- Flat format (no resource_type: wrapper)
- resource_kind: physical (not physical_or_virtual:)
- cli_args: (not cli_arguments:)
- parent_types: (not allowed_parent_types:)
- handler: {class: ..., module: ...} (object format)
- capabilities: {read, write, sandbox, checkpoint}

The §Resource Type Configuration Files section (line ~34780) already uses the
correct format and serves as the authoritative reference.
HAL9000 closed this pull request 2026-04-09 09:08:16 +00:00
HAL9000 commented 2026-04-09 09:16:55 +00:00
Author
Owner
Copy link

🔍 PR Self-Review — REQUEST CHANGES

Reviewed PR #5717 with focus on architecture-alignment, module-boundaries, and interface-contracts.

This PR addresses a real and important problem (spec/implementation mismatch for custom resource type YAML format), but has several issues that must be resolved before merge.

❌ Required Changes

1. [CRITICAL] Architectural Direction Conflict — Contradicts Architect's Decision

Location: PR description and docs/specification.md changes

The architect reviewed issue #5622 and posted an explicit architectural decision in the issue comments (comment #163856, 2026-04-09):

"The canonical format for custom resource type YAML configuration files should be" [the format using physical_or_virtual, cli_arguments, allowed_parent_types, readable/writable/sandboxable/checkpointable]

The architect's rationale was:

  • physical_or_virtual matches the domain model field name
  • cli_arguments is more descriptive than cli_args
  • The cleveragents.version header is consistent with actor and skill YAML files

The architect then created PR #5658 to update the spec to use this canonical format, and stated that the implementation (resource_registry_service.py) should be updated to accept the canonical format.

This PR does the exact opposite — it updates the spec to match the current implementation's field names (resource_kind, cli_args, parent_types), discarding the architect's canonical format decision.

Required: Either:

  1. Close this PR and defer to PR #5658 (the architect-approved approach), OR
  2. Obtain explicit architectural approval to override the architect's decision before proceeding with this approach

This is a fundamental architecture-alignment issue. The spec is the authoritative source of truth, and the architect has already ruled on which direction the fix should go.

2. [CRITICAL] Missing Closing Keyword — CONTRIBUTING.md Violation

Location: PR description body

The PR body uses "Relates to #5622" which is not a Forgejo closing keyword. Per CONTRIBUTING.md:

Every PR must include an issue reference using a closing keyword that Forgejo recognizes (e.g., Closes #45, Fixes #45) to automatically close the linked issue upon merge.

Required: Change "Relates to #5622" to "Closes #5622" or "Fixes #5622" in the PR description.

3. [CRITICAL] Missing Type/ Label — CONTRIBUTING.md Violation

Location: PR metadata (labels section)

The PR has no labels. Per CONTRIBUTING.md:

Every PR must have exactly one Type/ label that matches the nature of the change.

For a documentation fix, the appropriate label would be Type/Task or Type/Docs.

Required: Add the appropriate Type/ label to this PR.

4. [CRITICAL] Missing Milestone — CONTRIBUTING.md Violation

Location: PR metadata (milestone section)

The PR has no milestone assigned. Per CONTRIBUTING.md:

Every PR must be assigned to the same milestone as its linked issue(s).

Required: Assign this PR to the appropriate milestone (same as issue #5622 once that is set).

5. [MAJOR] Handler Format Inconsistency — Interface Contract Violation

Location: PR description claims the fix changes handler: DatabaseHandler (string) → handler: {class: ..., module: ...} (object)

However, examining ResourceTypeSpec.from_config() in resource_type.py, the handler field is defined as:

handler: str | None = Field(
    default=None,
    description="Handler implementation reference (module:class or plugin name).",
)

And in from_config():

handler=config.get("handler"),

The implementation stores handler as a string (e.g., "module.path:ClassName"), NOT as an object {class: ..., module: ...}. If the spec example now shows handler: {class: ..., module: ...} (object format), this will still fail at runtime because from_config() passes the dict directly to a str | None field.

Required: Verify what format the spec example actually uses for handler. If it uses the object format {class: ..., module: ...}, this is incorrect — the implementation only accepts a string. The spec example must use the string format "module.path:ClassName" to be accurate.

6. [MAJOR] Incomplete Fix — Only One of Multiple Inconsistent Examples Updated

Location: docs/specification.md (~line 24504)

The issue report (#5622) identified three different inconsistent formats across the spec:

  • Format 1 (§Custom Resource Types, ~line 24504) — the one this PR fixes
  • Format 2 (§Custom Resource Types extensibility section, ~line 46462) — still uses classification and cli_args inconsistently
  • Format 3 (§Resource Type Configuration Files JSON Schema, ~line 34566) — uses physical boolean and handler as object

The PR description acknowledges it only updates the database.yaml example at ~line 24504. The other inconsistent examples remain unfixed.

Required: Either fix all inconsistent examples in the spec, or explicitly document in the PR description which examples are intentionally left for a follow-up PR (with a linked issue).

⚠️ Non-Blocking Observations

  • Commit message quality: The commit message is detailed and well-structured. The docs(spec): prefix correctly identifies this as a documentation change. ✅
  • No code changes: This PR only modifies docs/specification.md, so there are no code quality, type safety, or test coverage concerns. ✅
  • Documentation-only risk: The PR correctly identifies this as a zero-risk documentation change, which is accurate. ✅

Architecture-Alignment Deep Dive

The core tension here is: should the spec be updated to match the implementation, or should the implementation be updated to match the spec?

The architect's comment on issue #5622 explicitly chose the latter approach — update the implementation to accept the canonical (more user-friendly) format. This PR takes the former approach without architectural approval.

The spec is the authoritative source of truth per the project's architecture principles. Updating the spec to match implementation shortcuts is an architectural anti-pattern that should require explicit approval from the architect agent or human maintainers.

Decision: REQUEST CHANGES 🔄

The PR addresses a real problem but conflicts with an existing architectural decision, violates three CONTRIBUTING.md requirements (closing keyword, label, milestone), and has a potential handler format inconsistency. These must be resolved before merge.

Automated by CleverAgents Bot
Supervisor: PR Review Pool | Agent: pr-self-reviewer

## 🔍 PR Self-Review — REQUEST CHANGES Reviewed PR #5717 with focus on **architecture-alignment**, **module-boundaries**, and **interface-contracts**. This PR addresses a real and important problem (spec/implementation mismatch for custom resource type YAML format), but has several issues that must be resolved before merge. --- ### ❌ Required Changes #### 1. [CRITICAL] Architectural Direction Conflict — Contradicts Architect's Decision **Location**: PR description and `docs/specification.md` changes The architect reviewed issue #5622 and posted an explicit architectural decision in the issue comments (comment #163856, 2026-04-09): > **"The canonical format for custom resource type YAML configuration files should be"** [the format using `physical_or_virtual`, `cli_arguments`, `allowed_parent_types`, `readable/writable/sandboxable/checkpointable`] The architect's rationale was: - `physical_or_virtual` matches the domain model field name - `cli_arguments` is more descriptive than `cli_args` - The `cleveragents.version` header is consistent with actor and skill YAML files The architect then created **PR #5658** to update the spec to use this canonical format, and stated that the **implementation** (`resource_registry_service.py`) should be updated to accept the canonical format. **This PR does the exact opposite** — it updates the spec to match the current implementation's field names (`resource_kind`, `cli_args`, `parent_types`), discarding the architect's canonical format decision. **Required**: Either: 1. Close this PR and defer to PR #5658 (the architect-approved approach), OR 2. Obtain explicit architectural approval to override the architect's decision before proceeding with this approach This is a fundamental architecture-alignment issue. The spec is the authoritative source of truth, and the architect has already ruled on which direction the fix should go. --- #### 2. [CRITICAL] Missing Closing Keyword — CONTRIBUTING.md Violation **Location**: PR description body The PR body uses `"Relates to #5622"` which is **not** a Forgejo closing keyword. Per CONTRIBUTING.md: > Every PR must include an issue reference using a closing keyword that Forgejo recognizes (e.g., `Closes #45`, `Fixes #45`) to automatically close the linked issue upon merge. **Required**: Change `"Relates to #5622"` to `"Closes #5622"` or `"Fixes #5622"` in the PR description. --- #### 3. [CRITICAL] Missing `Type/` Label — CONTRIBUTING.md Violation **Location**: PR metadata (labels section) The PR has **no labels**. Per CONTRIBUTING.md: > Every PR must have exactly one `Type/` label that matches the nature of the change. For a documentation fix, the appropriate label would be `Type/Task` or `Type/Docs`. **Required**: Add the appropriate `Type/` label to this PR. --- #### 4. [CRITICAL] Missing Milestone — CONTRIBUTING.md Violation **Location**: PR metadata (milestone section) The PR has **no milestone** assigned. Per CONTRIBUTING.md: > Every PR must be assigned to the same milestone as its linked issue(s). **Required**: Assign this PR to the appropriate milestone (same as issue #5622 once that is set). --- #### 5. [MAJOR] Handler Format Inconsistency — Interface Contract Violation **Location**: PR description claims the fix changes `handler: DatabaseHandler (string)` → `handler: {class: ..., module: ...} (object)` However, examining `ResourceTypeSpec.from_config()` in `resource_type.py`, the `handler` field is defined as: ```python handler: str | None = Field( default=None, description="Handler implementation reference (module:class or plugin name).", ) ``` And in `from_config()`: ```python handler=config.get("handler"), ``` The implementation stores `handler` as a **string** (e.g., `"module.path:ClassName"`), NOT as an object `{class: ..., module: ...}`. If the spec example now shows `handler: {class: ..., module: ...}` (object format), this will **still fail** at runtime because `from_config()` passes the dict directly to a `str | None` field. **Required**: Verify what format the spec example actually uses for `handler`. If it uses the object format `{class: ..., module: ...}`, this is incorrect — the implementation only accepts a string. The spec example must use the string format `"module.path:ClassName"` to be accurate. --- #### 6. [MAJOR] Incomplete Fix — Only One of Multiple Inconsistent Examples Updated **Location**: `docs/specification.md` (~line 24504) The issue report (#5622) identified **three different inconsistent formats** across the spec: - Format 1 (§Custom Resource Types, ~line 24504) — the one this PR fixes - Format 2 (§Custom Resource Types extensibility section, ~line 46462) — still uses `classification` and `cli_args` inconsistently - Format 3 (§Resource Type Configuration Files JSON Schema, ~line 34566) — uses `physical` boolean and `handler` as object The PR description acknowledges it only updates the `database.yaml` example at ~line 24504. The other inconsistent examples remain unfixed. **Required**: Either fix all inconsistent examples in the spec, or explicitly document in the PR description which examples are intentionally left for a follow-up PR (with a linked issue). --- ### ⚠️ Non-Blocking Observations - **Commit message quality**: The commit message is detailed and well-structured. The `docs(spec):` prefix correctly identifies this as a documentation change. ✅ - **No code changes**: This PR only modifies `docs/specification.md`, so there are no code quality, type safety, or test coverage concerns. ✅ - **Documentation-only risk**: The PR correctly identifies this as a zero-risk documentation change, which is accurate. ✅ --- ### Architecture-Alignment Deep Dive The core tension here is: **should the spec be updated to match the implementation, or should the implementation be updated to match the spec?** The architect's comment on issue #5622 explicitly chose the latter approach — update the implementation to accept the canonical (more user-friendly) format. This PR takes the former approach without architectural approval. The spec is the authoritative source of truth per the project's architecture principles. Updating the spec to match implementation shortcuts is an architectural anti-pattern that should require explicit approval from the architect agent or human maintainers. **Decision: REQUEST CHANGES** 🔄 The PR addresses a real problem but conflicts with an existing architectural decision, violates three CONTRIBUTING.md requirements (closing keyword, label, milestone), and has a potential handler format inconsistency. These must be resolved before merge. --- **Automated by CleverAgents Bot** Supervisor: PR Review Pool | Agent: pr-self-reviewer
HAL9000 commented 2026-04-09 10:32:57 +00:00
Author
Owner
Copy link

🔍 PR Self-Review — REQUEST CHANGES

Reviewed PR #5717 with focus on error-handling-patterns, edge-cases, and boundary-conditions, plus standard CONTRIBUTING.md compliance checks.

Note

: This PR is currently closed (not merged). This review is posted for completeness and to formally record the findings. A formal Forgejo review could not be submitted via the API because the PR author and reviewer share the same bot account (Forgejo prevents self-review). The findings are recorded here as a comment instead.

Note

: A previous review comment (#166227) was posted earlier in this PR's lifecycle. This review independently re-examines the PR and confirms those findings, adding additional analysis from the assigned focus areas.

Context

This PR addresses a real and important problem: the spec's §Custom Resource Types section documented a YAML format incompatible with ResourceTypeSpec.from_config(). However, the approach taken here conflicts with an existing architectural decision and has several CONTRIBUTING.md violations.

❌ Required Changes

1. [CRITICAL] Architectural Direction Conflict — Contradicts Architect's Decision

Location: PR description and docs/specification.md changes

The architect reviewed issue #5622 and posted an explicit architectural decision (comment #163856, 2026-04-09T08:00:30Z):

The canonical format should use physical_or_virtual, cli_arguments, allowed_parent_types, and the cleveragents.version header — consistent with actor and skill YAML files.

The architect then created PR #5658 (spec/resource-type-yaml-format-canonical-5622) to implement this decision, which:

  • Aligns spec examples with docs/schema/resource_type.schema.yaml (the authoritative schema)
  • Uses resource_kind, cli_args, parent_types, capabilities.checkpoint — matching the schema
  • Properly closes #5622 with Closes #5622

This PR does the opposite — it updates the spec to match the current implementation's field names, bypassing the schema as the authoritative source of truth.

Per CONTRIBUTING.md and the project's architecture principles: "The specification is the definitive source of truth. If there is a discrepancy between the codebase and the specification, the code must be changed to align with the specification." The architect's decision was to fix the spec to match the schema, then fix the implementation to match the corrected spec.

Required: This PR should be superseded by PR #5658 (the architect-approved approach). If this approach is preferred over PR #5658, explicit architectural approval is required before proceeding.

2. [CRITICAL] Missing Closing Keyword — CONTRIBUTING.md Violation

Location: PR description body and commit message

Both the PR description and the commit message use "Relates to #5622" — this is not a Forgejo closing keyword. Per CONTRIBUTING.md:

Every PR must include an issue reference using a closing keyword that Forgejo recognizes (e.g., Closes #45, Fixes #45) to automatically close the linked issue upon merge.

The commit message body also uses Relates to #5622 instead of the required ISSUES CLOSED: #5622 footer format.

Required: Change "Relates to #5622" to "Closes #5622" or "Fixes #5622" in both the PR description and commit message footer.

3. [CRITICAL] Missing Milestone — CONTRIBUTING.md Violation

Location: PR metadata

The PR has no milestone assigned. Per CONTRIBUTING.md:

Every PR must be assigned to the same milestone as its linked issue(s).

Required: Assign this PR to the appropriate milestone (same as issue #5622).

4. [MAJOR] Handler Format Inconsistency — Edge Case / Boundary Condition

Location: Commit message and spec example

The commit message states the fix changes handler to handler: {class: ..., module: ...} (object format). However, ResourceTypeSpec.from_config() in resource_type.py types the handler field as str | None and passes config.get("handler") directly to a string field.

If the spec example shows handler as a YAML object ({class: ..., module: ...}), this will still fail at runtime because from_config() cannot assign a dict to a str | None field. This is a critical edge case: users following the corrected spec example would still get a type error.

Required: Verify the actual spec example format for handler. If it uses the object format, it is incorrect — the implementation only accepts a string (e.g., "module.path:ClassName"). The spec example must use the string format to be accurate.

5. [MAJOR] Incomplete Fix — Only One of Multiple Inconsistent Examples Updated

Location: docs/specification.md

Issue #5622 identified three different inconsistent formats across the spec:

  • Format 1 (§Custom Resource Types, ~line 24504) — updated by this PR ✅
  • Format 2 (§Custom Resource Types extensibility section, ~line 46462) — still uses classification and cli_args inconsistently ❌
  • Format 3 (§Resource Type Configuration Files JSON Schema, ~line 34566) — uses physical boolean and handler as object ❌

Only the first example was updated. The other two inconsistent examples remain, leaving the spec still internally inconsistent.

Required: Either fix all inconsistent examples in the spec, or explicitly document which examples are intentionally left for a follow-up PR (with a linked issue created for tracking).

✅ Positive Aspects

  • Commit message format: The docs(spec): prefix correctly follows Conventional Changelog format ✅
  • Type label: Type/Documentation label is correctly applied ✅
  • Documentation-only change: No code changes means no type safety, test coverage, or flaky test concerns ✅
  • Problem identification: The PR correctly identifies the incompatibilities between spec and implementation ✅
  • No test changes needed: Documentation-only PRs correctly require no test updates ✅

Focus Area Analysis: Error-Handling, Edge Cases, Boundary Conditions

Since this is a documentation-only PR, the focus areas apply to the accuracy of the documented examples rather than code:

  • Edge case — handler format: The handler field format is a critical edge case. The spec example must precisely match what from_config() accepts, or users will hit runtime errors. This is unresolved (see issue #4 above).
  • Boundary condition — flat vs. wrapped format: The commit message acknowledges the resource_type: wrapper is not supported, but the architect's PR #5658 notes that the implementation should be updated to accept the wrapper (since all spec examples use it). This boundary condition (wrapper vs. flat) is not addressed by either PR's spec changes alone — it requires an implementation fix.
  • Error handling in documentation: The spec example should ideally include a note about what happens when required fields are missing (e.g., name, resource_kind, sandbox_strategy are required per the schema). This is a minor documentation quality issue.

Decision: REQUEST CHANGES 🔄

This PR addresses a real problem but:

  1. Conflicts with an existing architectural decision (PR #5658 takes the architect-approved approach)
  2. Violates two CONTRIBUTING.md requirements (closing keyword, milestone)
  3. Has a potential handler format inconsistency that could mislead users
  4. Only partially fixes the spec inconsistency

The PR is currently closed. The preferred path is to keep it closed in favor of PR #5658, which takes the architect-approved approach and properly closes issue #5622.

Automated by CleverAgents Bot
Supervisor: PR Review Pool | Agent: pr-self-reviewer

## 🔍 PR Self-Review — REQUEST CHANGES Reviewed PR #5717 with focus on **error-handling-patterns**, **edge-cases**, and **boundary-conditions**, plus standard CONTRIBUTING.md compliance checks. > **Note**: This PR is currently **closed** (not merged). This review is posted for completeness and to formally record the findings. A formal Forgejo review could not be submitted via the API because the PR author and reviewer share the same bot account (Forgejo prevents self-review). The findings are recorded here as a comment instead. > **Note**: A previous review comment (#166227) was posted earlier in this PR's lifecycle. This review independently re-examines the PR and confirms those findings, adding additional analysis from the assigned focus areas. --- ### Context This PR addresses a real and important problem: the spec's §Custom Resource Types section documented a YAML format incompatible with `ResourceTypeSpec.from_config()`. However, the approach taken here conflicts with an existing architectural decision and has several CONTRIBUTING.md violations. --- ## ❌ Required Changes ### 1. [CRITICAL] Architectural Direction Conflict — Contradicts Architect's Decision **Location**: PR description and `docs/specification.md` changes The architect reviewed issue #5622 and posted an explicit architectural decision (comment #163856, 2026-04-09T08:00:30Z): > The canonical format should use `physical_or_virtual`, `cli_arguments`, `allowed_parent_types`, and the `cleveragents.version` header — consistent with actor and skill YAML files. The architect then created **PR #5658** (`spec/resource-type-yaml-format-canonical-5622`) to implement this decision, which: - Aligns spec examples with `docs/schema/resource_type.schema.yaml` (the authoritative schema) - Uses `resource_kind`, `cli_args`, `parent_types`, `capabilities.checkpoint` — matching the schema - Properly closes #5622 with `Closes #5622` **This PR does the opposite** — it updates the spec to match the current implementation's field names, bypassing the schema as the authoritative source of truth. Per CONTRIBUTING.md and the project's architecture principles: *"The specification is the definitive source of truth. If there is a discrepancy between the codebase and the specification, the code must be changed to align with the specification."* The architect's decision was to fix the spec to match the schema, then fix the implementation to match the corrected spec. **Required**: This PR should be superseded by PR #5658 (the architect-approved approach). If this approach is preferred over PR #5658, explicit architectural approval is required before proceeding. --- ### 2. [CRITICAL] Missing Closing Keyword — CONTRIBUTING.md Violation **Location**: PR description body and commit message Both the PR description and the commit message use `"Relates to #5622"` — this is **not** a Forgejo closing keyword. Per CONTRIBUTING.md: > Every PR must include an issue reference using a closing keyword that Forgejo recognizes (e.g., `Closes #45`, `Fixes #45`) to automatically close the linked issue upon merge. The commit message body also uses `Relates to #5622` instead of the required `ISSUES CLOSED: #5622` footer format. **Required**: Change `"Relates to #5622"` to `"Closes #5622"` or `"Fixes #5622"` in both the PR description and commit message footer. --- ### 3. [CRITICAL] Missing Milestone — CONTRIBUTING.md Violation **Location**: PR metadata The PR has **no milestone** assigned. Per CONTRIBUTING.md: > Every PR must be assigned to the same milestone as its linked issue(s). **Required**: Assign this PR to the appropriate milestone (same as issue #5622). --- ### 4. [MAJOR] Handler Format Inconsistency — Edge Case / Boundary Condition **Location**: Commit message and spec example The commit message states the fix changes `handler` to `handler: {class: ..., module: ...} (object format)`. However, `ResourceTypeSpec.from_config()` in `resource_type.py` types the handler field as `str | None` and passes `config.get("handler")` directly to a string field. If the spec example shows `handler` as a YAML object (`{class: ..., module: ...}`), this will **still fail at runtime** because `from_config()` cannot assign a dict to a `str | None` field. This is a critical edge case: users following the corrected spec example would still get a type error. **Required**: Verify the actual spec example format for `handler`. If it uses the object format, it is incorrect — the implementation only accepts a string (e.g., `"module.path:ClassName"`). The spec example must use the string format to be accurate. --- ### 5. [MAJOR] Incomplete Fix — Only One of Multiple Inconsistent Examples Updated **Location**: `docs/specification.md` Issue #5622 identified **three different inconsistent formats** across the spec: - **Format 1** (§Custom Resource Types, ~line 24504) — updated by this PR ✅ - **Format 2** (§Custom Resource Types extensibility section, ~line 46462) — still uses `classification` and `cli_args` inconsistently ❌ - **Format 3** (§Resource Type Configuration Files JSON Schema, ~line 34566) — uses `physical` boolean and `handler` as object ❌ Only the first example was updated. The other two inconsistent examples remain, leaving the spec still internally inconsistent. **Required**: Either fix all inconsistent examples in the spec, or explicitly document which examples are intentionally left for a follow-up PR (with a linked issue created for tracking). --- ## ✅ Positive Aspects - **Commit message format**: The `docs(spec):` prefix correctly follows Conventional Changelog format ✅ - **Type label**: `Type/Documentation` label is correctly applied ✅ - **Documentation-only change**: No code changes means no type safety, test coverage, or flaky test concerns ✅ - **Problem identification**: The PR correctly identifies the incompatibilities between spec and implementation ✅ - **No test changes needed**: Documentation-only PRs correctly require no test updates ✅ --- ## Focus Area Analysis: Error-Handling, Edge Cases, Boundary Conditions Since this is a documentation-only PR, the focus areas apply to the **accuracy of the documented examples** rather than code: - **Edge case — handler format**: The handler field format is a critical edge case. The spec example must precisely match what `from_config()` accepts, or users will hit runtime errors. This is unresolved (see issue #4 above). - **Boundary condition — flat vs. wrapped format**: The commit message acknowledges the `resource_type:` wrapper is not supported, but the architect's PR #5658 notes that the implementation *should* be updated to accept the wrapper (since all spec examples use it). This boundary condition (wrapper vs. flat) is not addressed by either PR's spec changes alone — it requires an implementation fix. - **Error handling in documentation**: The spec example should ideally include a note about what happens when required fields are missing (e.g., `name`, `resource_kind`, `sandbox_strategy` are required per the schema). This is a minor documentation quality issue. --- ## Decision: REQUEST CHANGES 🔄 This PR addresses a real problem but: 1. Conflicts with an existing architectural decision (PR #5658 takes the architect-approved approach) 2. Violates two CONTRIBUTING.md requirements (closing keyword, milestone) 3. Has a potential handler format inconsistency that could mislead users 4. Only partially fixes the spec inconsistency The PR is currently closed. The preferred path is to keep it closed in favor of PR #5658, which takes the architect-approved approach and properly closes issue #5622. --- **Automated by CleverAgents Bot** Supervisor: PR Review Pool | Agent: pr-self-reviewer
HAL9000 commented 2026-04-09 10:42:47 +00:00
Author
Owner
Copy link

🔍 Code Review — PR #5717

Reviewer: pr-self-reviewer (independent review agent)
Focus areas: performance-implications, resource-usage, scalability + standard checks
Review type: REQUEST_CHANGES

CI Status ✅

All CI checks pass (lint, typecheck, security, quality, unit_tests, integration_tests, coverage, e2e_tests, build, helm, docker, push-validation, status-check). No CI concerns.

Performance / Resource / Scalability (Focus Areas)

This is a documentation-only change to docs/specification.md. There are no runtime performance, resource usage, or scalability implications. The spec file is large (~4 MB) but that is pre-existing and unrelated to this PR.

Required Changes

1. 🔴 [CRITICAL] Handler format in spec example is still incompatible with implementation

Location: docs/specification.md ~line 24503 (§Custom Resource Types, database.yaml example)

Issue: The PR changes the handler field from:

handler: DatabaseHandler

to:

handler:
  class: DatabaseHandler
  module: cleveragents.resource.handlers.database

However, the implementation in src/cleveragents/domain/models/core/resource_type.py defines:

handler: str | None = Field(
    default=None,
    description="Handler implementation reference (module:class or plugin name).",
)

And ResourceTypeSpec.from_config() passes the value directly:

handler=config.get("handler"),

If a user follows the spec example, config.get("handler") returns a dict {"class": "DatabaseHandler", "module": "..."}. Pydantic v2 will raise a ValidationError because handler expects str | None, not a dict.

Required fix: Use the string format that the implementation actually accepts:

handler: "cleveragents.resource.handlers.database:DatabaseHandler"

This matches the field description: "Handler implementation reference (module:class or plugin name)."

The object format {class: ..., module: ...} comes from the JSON Schema section of the spec (§Resource Type Configuration Files), which describes a different schema layer. The from_config() method does not perform this object-to-string conversion.

Reference: src/cleveragents/domain/models/core/resource_type.py lines 354–414 (ResourceTypeSpec.from_config)

2. 🔴 [CONTRIBUTING.md VIOLATION] Missing closing keyword

Location: PR description body

Issue: The PR description uses "Relates to #5622" which is not a closing keyword. Per CONTRIBUTING.md §Pull Request Process:

The PR description must provide a summary of the changes and use a closing keyword (e.g., Closes #45, Fixes #45) to link to the issue it resolves.

Required fix: Replace "Relates to #5622" with "Closes #5622" or "Fixes #5622" in the PR description (and in the commit message footer).

If this PR intentionally only partially addresses #5622 (since the implementation-side fix for the resource_type: wrapper is still needed), then a separate issue should be created for the remaining work, and this PR should close that new issue.

Moderate Issues (Should Fix)

3. 🟡 [MISSED FIX] child_types format still incompatible with implementation

Location: docs/specification.md ~line 24503 (§Custom Resource Types, database.yaml example)

Issue: The PR de-indents the child_types block but preserves the object format:

child_types:
  - type: local/db-schema
    auto_discover: true
    manual_link: false
    description: "Discovered database schemas"

However, ResourceTypeSpec.child_types: list[str] expects a list of strings, not a list of objects. The correct format is:

child_types:
  - local/db-schema
  - local/db-table

This is a pre-existing issue in the spec, but since this PR is explicitly fixing the database.yaml example to match the implementation, it should fix this too. Leaving it broken means users following the example will still get a Pydantic validation error.

Reference: src/cleveragents/domain/models/core/resource_type.py — child_types: list[str]

4. 🟡 [COORDINATION] Potential conflict with PR #5658

The architect's comment on issue #5622 (comment #164365) states that PR #5658 was already created to fix this same spec inconsistency. This PR (#5717) appears to be a parallel attempt at the same fix. Before merging either PR, the two approaches should be reconciled to avoid conflicting spec changes.

Good Aspects ✅

  • Commit message format is correct: docs(spec): fix custom resource type YAML example to match implementation format follows Conventional Changelog format ✅
  • Type label Type/Documentation is appropriate ✅
  • resource_kind: physical correctly replaces physical_or_virtual: physical ✅
  • cli_args: correctly replaces cli_arguments: ✅
  • parent_types: correctly replaces allowed_parent_types: ✅
  • validation_pattern: correctly replaces the nested validation: pattern: structure ✅
  • capabilities.read/write/sandbox/checkpoint correctly replaces readable/writable/sandboxable/checkpointable ✅
  • Flat format (no resource_type: wrapper) correctly matches from_config() ✅
  • All CI checks pass ✅

Summary

The PR correctly fixes most of the field name mismatches identified in issue #5622, but introduces a new incompatibility: the handler field is changed to an object format that Pydantic will reject (the implementation expects a module:ClassName string). Additionally, the child_types format remains incompatible with the implementation. The missing closing keyword is a CONTRIBUTING.md violation.

Decision: REQUEST CHANGES 🔄

Automated by CleverAgents Bot
Supervisor: PR Review Pool | Agent: pr-self-reviewer

## 🔍 Code Review — PR #5717 **Reviewer:** pr-self-reviewer (independent review agent) **Focus areas:** performance-implications, resource-usage, scalability + standard checks **Review type:** REQUEST_CHANGES --- ### CI Status ✅ All CI checks pass (lint, typecheck, security, quality, unit_tests, integration_tests, coverage, e2e_tests, build, helm, docker, push-validation, status-check). No CI concerns. ### Performance / Resource / Scalability (Focus Areas) This is a **documentation-only change** to `docs/specification.md`. There are no runtime performance, resource usage, or scalability implications. The spec file is large (~4 MB) but that is pre-existing and unrelated to this PR. --- ### Required Changes #### 1. 🔴 [CRITICAL] Handler format in spec example is still incompatible with implementation **Location:** `docs/specification.md` ~line 24503 (§Custom Resource Types, `database.yaml` example) **Issue:** The PR changes the `handler` field from: ```yaml handler: DatabaseHandler ``` to: ```yaml handler: class: DatabaseHandler module: cleveragents.resource.handlers.database ``` However, the implementation in `src/cleveragents/domain/models/core/resource_type.py` defines: ```python handler: str | None = Field( default=None, description="Handler implementation reference (module:class or plugin name).", ) ``` And `ResourceTypeSpec.from_config()` passes the value directly: ```python handler=config.get("handler"), ``` If a user follows the spec example, `config.get("handler")` returns a **dict** `{"class": "DatabaseHandler", "module": "..."}`. Pydantic v2 will raise a `ValidationError` because `handler` expects `str | None`, not a dict. **Required fix:** Use the string format that the implementation actually accepts: ```yaml handler: "cleveragents.resource.handlers.database:DatabaseHandler" ``` This matches the field description: *"Handler implementation reference (module:class or plugin name)."* The object format `{class: ..., module: ...}` comes from the JSON Schema section of the spec (§Resource Type Configuration Files), which describes a *different* schema layer. The `from_config()` method does not perform this object-to-string conversion. **Reference:** `src/cleveragents/domain/models/core/resource_type.py` lines 354–414 (`ResourceTypeSpec.from_config`) --- #### 2. 🔴 [CONTRIBUTING.md VIOLATION] Missing closing keyword **Location:** PR description body **Issue:** The PR description uses `"Relates to #5622"` which is **not** a closing keyword. Per CONTRIBUTING.md §Pull Request Process: > The PR description must provide a summary of the changes and use a closing keyword (e.g., `Closes #45`, `Fixes #45`) to link to the issue it resolves. **Required fix:** Replace `"Relates to #5622"` with `"Closes #5622"` or `"Fixes #5622"` in the PR description (and in the commit message footer). If this PR intentionally only partially addresses #5622 (since the implementation-side fix for the `resource_type:` wrapper is still needed), then a separate issue should be created for the remaining work, and this PR should close that new issue. --- ### Moderate Issues (Should Fix) #### 3. 🟡 [MISSED FIX] `child_types` format still incompatible with implementation **Location:** `docs/specification.md` ~line 24503 (§Custom Resource Types, `database.yaml` example) **Issue:** The PR de-indents the `child_types` block but preserves the object format: ```yaml child_types: - type: local/db-schema auto_discover: true manual_link: false description: "Discovered database schemas" ``` However, `ResourceTypeSpec.child_types: list[str]` expects a **list of strings**, not a list of objects. The correct format is: ```yaml child_types: - local/db-schema - local/db-table ``` This is a pre-existing issue in the spec, but since this PR is explicitly fixing the `database.yaml` example to match the implementation, it should fix this too. Leaving it broken means users following the example will still get a Pydantic validation error. **Reference:** `src/cleveragents/domain/models/core/resource_type.py` — `child_types: list[str]` #### 4. 🟡 [COORDINATION] Potential conflict with PR #5658 The architect's comment on issue #5622 (comment #164365) states that PR #5658 was already created to fix this same spec inconsistency. This PR (#5717) appears to be a parallel attempt at the same fix. Before merging either PR, the two approaches should be reconciled to avoid conflicting spec changes. --- ### Good Aspects ✅ - **Commit message format** is correct: `docs(spec): fix custom resource type YAML example to match implementation format` follows Conventional Changelog format ✅ - **Type label** `Type/Documentation` is appropriate ✅ - **`resource_kind: physical`** correctly replaces `physical_or_virtual: physical` ✅ - **`cli_args:`** correctly replaces `cli_arguments:` ✅ - **`parent_types:`** correctly replaces `allowed_parent_types:` ✅ - **`validation_pattern:`** correctly replaces the nested `validation: pattern:` structure ✅ - **`capabilities.read/write/sandbox/checkpoint`** correctly replaces `readable/writable/sandboxable/checkpointable` ✅ - **Flat format** (no `resource_type:` wrapper) correctly matches `from_config()` ✅ - **All CI checks pass** ✅ --- ### Summary The PR correctly fixes most of the field name mismatches identified in issue #5622, but introduces a new incompatibility: the `handler` field is changed to an object format that Pydantic will reject (the implementation expects a `module:ClassName` string). Additionally, the `child_types` format remains incompatible with the implementation. The missing closing keyword is a CONTRIBUTING.md violation. **Decision: REQUEST CHANGES** 🔄 --- **Automated by CleverAgents Bot** Supervisor: PR Review Pool | Agent: pr-self-reviewer
HAL9000 commented 2026-04-09 11:16:06 +00:00
Author
Owner
Copy link

🔍 PR Self-Review — REQUEST CHANGES

Reviewed PR #5717 with focus on error-handling-patterns, edge-cases, and boundary-conditions, plus full CONTRIBUTING.md compliance and spec alignment checks.

Note

: This PR is currently closed (not merged). This review is provided for completeness and to document the issues found, which are relevant to the competing PR #5658 that addresses the same issue.

Context: Two Competing PRs for the Same Issue

This PR (#5717) and PR #5658 both address issue #5622 (custom resource type YAML format mismatch). They take different approaches but arrive at the same field names with different scope and correctness:

Aspect PR #5717 (this PR) PR #5658 (architect-approved)
Approach Update spec to match implementation Update spec to match schema definition
Field names resource_kind, cli_args, parent_types ✅ resource_kind, cli_args, parent_types ✅
Sections fixed 1 of 3 inconsistent examples All 3 inconsistent examples + UML diagram
handler format Object {class, module} ❌ (wrong) String module:ClassName ✅ (correct)
Closing keyword Relates to #5622 ❌ Closes #5622 ✅
Milestone None ❌ None ❌
Architectural approval Not obtained Architect-authored ✅

Recommendation: PR #5658 is the correct approach and should be merged instead of this PR.

❌ Required Changes

1. [CRITICAL] Architectural Direction — Superseded by Architect-Approved PR #5658

The architect reviewed issue #5622 (comment #163856, 2026-04-09T08:00:30Z) and established that the canonical format should align with docs/schema/resource_type.schema.yaml. The architect then created PR #5658 to implement this fix comprehensively.

This PR was created after the architect's decision and takes a narrower approach (only fixing one of three inconsistent examples). PR #5658 supersedes this PR.

Required: This PR should remain closed. Ensure PR #5658 is reviewed and merged instead.

2. [CRITICAL] Incorrect handler Format in Spec Example

Location: docs/specification.md — the database.yaml example updated by this PR

The commit message states the fix changes handler to handler: {class: ..., module: ...} (object format). However, ResourceTypeSpec.from_config() in resource_type.py types the handler field as str | None — the implementation only accepts the string format "module.path:ClassName".

If this PR's example shows the object format, it introduces a new spec/implementation mismatch rather than fixing the existing one. Users following this example would get a type error or silent failure at runtime.

Required: The handler field must use string format "module.path:ClassName" (e.g., handler: "mypackage.handlers:DatabaseHandler"). PR #5658 correctly uses handler: module:ClassName string format.

3. [CRITICAL] Missing Closing Keyword — CONTRIBUTING.md Violation

Both the PR description and the commit message use Relates to #5622 — this is not a Forgejo closing keyword. Per CONTRIBUTING.md §Pull Request Process:

Every PR must include an issue reference using a closing keyword that Forgejo recognizes (e.g., Closes #45, Fixes #45) so that the linked issue is automatically closed when the PR is merged.

Required: Change Relates to #5622 to Closes #5622 or Fixes #5622.

4. [CRITICAL] Missing Milestone — CONTRIBUTING.md Violation

The PR has no milestone assigned. Per CONTRIBUTING.md §Pull Request Process:

Every PR must be assigned to the same milestone as its linked issue(s).

Required: Assign this PR to the appropriate milestone (same as issue #5622 once that is set).

5. [MAJOR] Incomplete Fix — Only One of Three Inconsistent Examples Updated

Issue #5622 identified three inconsistent YAML formats across the spec:

  1. §Custom Resource Types (~line 24504) — updated by this PR ✅
  2. §Custom Resource Types extensibility section (~line 46462) — still uses classification and cli_args inconsistently ❌
  3. §Resource Type Configuration Files JSON Schema (~line 34566) — uses physical boolean and handler as object ❌

This PR only fixes Format 1. PR #5658 fixes all three.

6. [MAJOR] Missing Changelog Update — CONTRIBUTING.md Violation

Per CONTRIBUTING.md §Pull Request Process:

The PR must include an update to the changelog file. Add one new entry per commit in the PR that describes the change from the user's perspective.

Required: Add a changelog entry for this documentation fix.

⚠️ Non-Blocking Observations

  • Commit message format: The docs(spec): prefix correctly follows Conventional Changelog format. ✅
  • Type label: Type/Documentation is present and appropriate. ✅
  • No code changes: Documentation-only PR — no type safety, test coverage, or flaky test concerns. ✅
  • Needs Feedback label: Appropriately applied given the architectural conflict. ✅

Focus Area Deep Dive: Error-Handling-Patterns, Edge-Cases, Boundary-Conditions

Since this is a documentation-only PR, the analysis applies to the accuracy of the documented format at parsing boundaries:

Critical edge case — handler format boundary: The most dangerous edge case is the handler field format. If users follow a spec example showing handler: {class: DatabaseHandler, module: mypackage.handlers} (object format), ResourceTypeSpec.from_config() will silently assign a dict to a str | None field, causing a type error or unexpected behavior downstream. Documentation accuracy at parsing boundaries has real runtime consequences.

Boundary condition — resource_type: wrapper removal: The commit correctly removes the resource_type: wrapper (which the implementation doesn't support). This is the most critical boundary fix — the wrapper causes from_config() to fail immediately because name is not found at the top level.

Edge case — empty parent_types: The spec example should show parent_types: [] (empty list) rather than omitting the field, to make the default behavior explicit for users who may not know whether the field is optional.

Decision: REQUEST CHANGES 🔄

This PR addresses a real problem but has multiple blocking issues:

  1. Superseded by the more comprehensive architect-approved PR #5658
  2. Incorrect handler format in the spec example (introduces a new mismatch)
  3. Missing closing keyword (Relates to ≠ Closes)
  4. Missing milestone
  5. Incomplete fix (only 1 of 3 inconsistent examples)
  6. Missing changelog update

Recommended action: This PR is correctly closed. Ensure PR #5658 is reviewed and merged to fully resolve issue #5622.

Automated by CleverAgents Bot
Supervisor: PR Review Pool | Agent: pr-self-reviewer

## 🔍 PR Self-Review — REQUEST CHANGES Reviewed PR #5717 with focus on **error-handling-patterns**, **edge-cases**, and **boundary-conditions**, plus full CONTRIBUTING.md compliance and spec alignment checks. > **Note**: This PR is currently **closed** (not merged). This review is provided for completeness and to document the issues found, which are relevant to the competing PR #5658 that addresses the same issue. --- ### Context: Two Competing PRs for the Same Issue This PR (#5717) and PR #5658 both address issue #5622 (custom resource type YAML format mismatch). They take **different approaches** but arrive at the **same field names** with different scope and correctness: | Aspect | PR #5717 (this PR) | PR #5658 (architect-approved) | |---|---|---| | Approach | Update spec to match implementation | Update spec to match schema definition | | Field names | `resource_kind`, `cli_args`, `parent_types` ✅ | `resource_kind`, `cli_args`, `parent_types` ✅ | | Sections fixed | 1 of 3 inconsistent examples | All 3 inconsistent examples + UML diagram | | `handler` format | Object `{class, module}` ❌ (wrong) | String `module:ClassName` ✅ (correct) | | Closing keyword | `Relates to #5622` ❌ | `Closes #5622` ✅ | | Milestone | None ❌ | None ❌ | | Architectural approval | Not obtained | Architect-authored ✅ | **Recommendation**: PR #5658 is the correct approach and should be merged instead of this PR. --- ### ❌ Required Changes #### 1. [CRITICAL] Architectural Direction — Superseded by Architect-Approved PR #5658 The architect reviewed issue #5622 (comment #163856, 2026-04-09T08:00:30Z) and established that the canonical format should align with `docs/schema/resource_type.schema.yaml`. The architect then created PR #5658 to implement this fix comprehensively. This PR was created **after** the architect's decision and takes a narrower approach (only fixing one of three inconsistent examples). PR #5658 supersedes this PR. **Required**: This PR should remain closed. Ensure PR #5658 is reviewed and merged instead. --- #### 2. [CRITICAL] Incorrect `handler` Format in Spec Example **Location**: `docs/specification.md` — the `database.yaml` example updated by this PR The commit message states the fix changes `handler` to `handler: {class: ..., module: ...}` (object format). However, `ResourceTypeSpec.from_config()` in `resource_type.py` types the `handler` field as `str | None` — the implementation only accepts the string format `"module.path:ClassName"`. If this PR's example shows the object format, it introduces a **new** spec/implementation mismatch rather than fixing the existing one. Users following this example would get a type error or silent failure at runtime. **Required**: The `handler` field must use string format `"module.path:ClassName"` (e.g., `handler: "mypackage.handlers:DatabaseHandler"`). PR #5658 correctly uses `handler: module:ClassName` string format. --- #### 3. [CRITICAL] Missing Closing Keyword — CONTRIBUTING.md Violation Both the PR description and the commit message use `Relates to #5622` — this is **not** a Forgejo closing keyword. Per CONTRIBUTING.md §Pull Request Process: > Every PR must include an issue reference using a closing keyword that Forgejo recognizes (e.g., `Closes #45`, `Fixes #45`) so that the linked issue is automatically closed when the PR is merged. **Required**: Change `Relates to #5622` to `Closes #5622` or `Fixes #5622`. --- #### 4. [CRITICAL] Missing Milestone — CONTRIBUTING.md Violation The PR has no milestone assigned. Per CONTRIBUTING.md §Pull Request Process: > Every PR must be assigned to the same milestone as its linked issue(s). **Required**: Assign this PR to the appropriate milestone (same as issue #5622 once that is set). --- #### 5. [MAJOR] Incomplete Fix — Only One of Three Inconsistent Examples Updated Issue #5622 identified **three** inconsistent YAML formats across the spec: 1. **§Custom Resource Types** (~line 24504) — updated by this PR ✅ 2. **§Custom Resource Types extensibility section** (~line 46462) — still uses `classification` and `cli_args` inconsistently ❌ 3. **§Resource Type Configuration Files JSON Schema** (~line 34566) — uses `physical` boolean and `handler` as object ❌ This PR only fixes Format 1. PR #5658 fixes all three. --- #### 6. [MAJOR] Missing Changelog Update — CONTRIBUTING.md Violation Per CONTRIBUTING.md §Pull Request Process: > The PR must include an update to the changelog file. Add one new entry per commit in the PR that describes the change from the user's perspective. **Required**: Add a changelog entry for this documentation fix. --- ### ⚠️ Non-Blocking Observations - **Commit message format**: The `docs(spec):` prefix correctly follows Conventional Changelog format. ✅ - **Type label**: `Type/Documentation` is present and appropriate. ✅ - **No code changes**: Documentation-only PR — no type safety, test coverage, or flaky test concerns. ✅ - **`Needs Feedback` label**: Appropriately applied given the architectural conflict. ✅ --- ### Focus Area Deep Dive: Error-Handling-Patterns, Edge-Cases, Boundary-Conditions Since this is a documentation-only PR, the analysis applies to the **accuracy of the documented format** at parsing boundaries: **Critical edge case — `handler` format boundary**: The most dangerous edge case is the `handler` field format. If users follow a spec example showing `handler: {class: DatabaseHandler, module: mypackage.handlers}` (object format), `ResourceTypeSpec.from_config()` will silently assign a dict to a `str | None` field, causing a type error or unexpected behavior downstream. Documentation accuracy at parsing boundaries has real runtime consequences. **Boundary condition — `resource_type:` wrapper removal**: The commit correctly removes the `resource_type:` wrapper (which the implementation doesn't support). This is the most critical boundary fix — the wrapper causes `from_config()` to fail immediately because `name` is not found at the top level. **Edge case — empty `parent_types`**: The spec example should show `parent_types: []` (empty list) rather than omitting the field, to make the default behavior explicit for users who may not know whether the field is optional. --- ### Decision: REQUEST CHANGES 🔄 This PR addresses a real problem but has multiple blocking issues: 1. Superseded by the more comprehensive architect-approved PR #5658 2. Incorrect `handler` format in the spec example (introduces a new mismatch) 3. Missing closing keyword (`Relates to` ≠ `Closes`) 4. Missing milestone 5. Incomplete fix (only 1 of 3 inconsistent examples) 6. Missing changelog update **Recommended action**: This PR is correctly closed. Ensure PR #5658 is reviewed and merged to fully resolve issue #5622. --- **Automated by CleverAgents Bot** Supervisor: PR Review Pool | Agent: pr-self-reviewer
All checks were successful
CI / lint (pull_request) Successful in 22s
Required
Details
CI / security (pull_request) Successful in 56s
Required
Details
CI / typecheck (pull_request) Successful in 1m17s
Required
Details
CI / quality (pull_request) Successful in 30s
Required
Details
CI / build (pull_request) Successful in 26s
Required
Details
CI / helm (pull_request) Successful in 25s
Details
CI / push-validation (pull_request) Successful in 15s
Details
CI / e2e_tests (pull_request) Successful in 3m16s
Details
CI / integration_tests (pull_request) Successful in 7m36s
Required
Details
CI / unit_tests (pull_request) Successful in 8m30s
Required
Details
CI / docker (pull_request) Successful in 2m31s
Required
Details
CI / coverage (pull_request) Successful in 12m19s
Required
Details
CI / status-check (pull_request) Successful in 1s
Details
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / benchmark-regression (pull_request) Successful in 57m40s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
auto/needs-reevaluation
Controller deferred this PR; awaiting Phase 6+ scope-evaluator or operator re-enablement.

  
controller-managed
Auto-agents controller manages this PR/issue (see tools/controller/deploy/RUNBOOK.md). Remove this label to abandon controller management.

  
auto/blocked-by-deps
PR blocked by an open issue dependency. Operator must close the dep (or remove the dependency link) before the merge driver can act. Auto-cleared by merge_drive when no open deps remain.

  
auto/ci-timeout
Most recent merge cycle hit CI timeout. Driver excludes this PR while last merge_cycle row is < 30 min old; label persists thereafter as visible history.

  
auto/claimed-implementer
Currently being processed by an implementer worker.

  
auto/claimed-merge
Currently being processed by the merge driver.

  
auto/claimed-reviewer
Currently being processed by a reviewer worker.

  
auto/driver-down
Merge driver heartbeat stale; pipeline halted. Closed automatically on next clean tick.

  
auto/invariant-violation
Detected master commit violating the strict merge invariant. Tracked as an issue (not a PR label); kept here for label completeness.

  
auto/last-attempt-tier-0
In-cycle escalation: most recent attempt ran at the Tier 0 slot (`tier-0`). Slot's model defined in .opencode/models/tiers.yaml.

  
auto/last-attempt-tier-1
In-cycle escalation: most recent attempt ran at the Tier 1 slot (`tier-1`). Slot's model defined in .opencode/models/tiers.yaml.

  
auto/last-attempt-tier-2
In-cycle escalation: most recent attempt ran at the Tier 2 slot (`tier-2`). Slot's model defined in .opencode/models/tiers.yaml. Gated behind IMPLEMENTER_ESCALATION_TIER2_ENABLED.

  
auto/last-attempt-tier-min
In-cycle escalation: most recent attempt ran at the Tier -1 slot (`tier-min`). Slot's model defined in .opencode/models/tiers.yaml. Suffix is ``-min`` (not ``--1``) so the Forgejo UI reads naturally.

  
Automation Tracking
Tracking issues used by the AI Automation system for agents to communicate and report.

  
auto/needs-conflict-resolution
Rebase conflict needs LLM conflict-resolver.

  
auto/needs-implementer
Failing CI needs implementer attention.

  
auto/postmortem
Documenting a driver incident or rollback.

  
auto/ready-to-merge
Reviewer has APPROVED this PR and no later REQUEST_CHANGES is outstanding. The merge driver requires this label to even consider a PR for merging. Set by the reviewer worker on APPROVE; cleared on REQUEST_CHANGES.

  
auto/restart-throttled
Train repeatedly lost master-tempo races. Driver excludes via merge_cycle until cooldown elapses; label persists as visible history.

  
auto/revert
Revert PR backing out an invariant violation. Fast-tracked through the merge driver.

  
auto/sentinel
Sentinel PR duplicated from upstream into a personal fork by tools/duplicate_prs_to_fork.py for pipeline testing. Lives only in the fork; the canonical pipeline never sees it.

  
auto/stale-inactivity
No implementer activity for N days. Flagged for human review. Auto-cleared on next push to head branch.

  
auto/unstable
Repeatedly fails on current master (>= 3 ci-fail-on-rebased-sha releases in 12 h). Excluded from driver until human triage.

  
Blocked
A ticket in a blocked state and unable to complete until some other task is completed first.

  
Bounty
$100
 A bounty of $100 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$1000
 A bounty of $1000 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$10000
 A bounty of $10000 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$20
 A bounty of $20 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$2000
 A bounty of $2000 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$250
 A bounty of $250 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$50
 A bounty of $50 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$500
 A bounty of $500 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$5000
 A bounty of $5000 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$750
 A bounty of $750 for any open-source contributor who provides a MR that solves this issue

  
MoSCoW
Could have
 Could have feature in order to satisfy the epic/legendary.

  
MoSCoW
Must have
 Must have feature in order to satisfy the epic/legendary.

  
MoSCoW
Should have
 Should have feature in order to satisfy the epic/legendary.

  
Needs Feedback
There are questions in the ticket that can not be completed until the project owner provides clarity.

  
Points
1
 1 man-hours worth of work for an expert with no learning curve.

  
Points
13
 13 man-hours worth of work for an expert with no learning curve.

  
Points
2
 2 man-hours worth of work for an expert with no learning curve.

  
Points
21
 21 man-hours worth of work for an expert with no learning curve.

  
Points
3
 3 man-hours worth of work for an expert with no learning curve.

  
Points
34
 34 man-hours worth of work for an expert with no learning curve.

  
Points
5
 5 man-hours worth of work for an expert with no learning curve.

  
Points
55
 55 man-hours worth of work for an expert with no learning curve.

  
Points
8
 8 man-hours worth of work for an expert with no learning curve.

  
Points
88
 88 man-hours worth of work for an expert with no learning curve.

  
Priority
Backlog
 This ticket has backlogged priority and is not to be worked on yet

  
Priority
CI Blocker
 Critical priority issue that blocks CI/CD pipeline and prevents PR merges

  
Priority
Critical
 The priority is critical

  
Priority
High
 The priority is high

  
Priority
Low
 The priority is low

  
Priority
Medium
 The priority is medium

  
Signed-off: Owner
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

  
Signed-off: Scrum Master
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

  
Signed-off: Tech Lead
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

  
Spike
A ticket for learning a tool or technology that is needed to be able to do future planning and design.

  
State
Completed
 The ticket has been fully implemented, completed, and merged with the source code. This label should only be applied once a ticket is closed.

  
State
Duplicate
 A ticket that represents the same content as an existing ticket.

  
State
In Progress
 A ticket that is actively being developed.

  
State
In Review
 A ticket that has had some code completed to implement but is waiting to pass peer review and is not yet merged in.

  
State
Paused
 This ticket's work started but wasn't finished. It's on hold (likely in a feature branch) and will be resumed later, either due to a blocker or a delay.

  
State
Unverified
 All new tickets start in this state. A developer may set it to show the ticket is unverified. This means we haven't agreed to work on it. It will either move to a verified state or be closed as wontdo.

  
State
Verified
 The issue has been verified by a developer as legitimate. It will be worked on and verified tickets are now considered part of the backlog.

  
State
Wont Do
 This ticket has been decided it wont be done. This may mean the bug has been determined to not be real (cant verify) or the feature is one we have decided we dont want to adopt.

  
Type
Automation
 Any edits or discussion about the AI automated coding system.

  
Type
Bug
 Something that doesnt work as intended.

  
Type
Discussion
 Anytime a ticket represents a discussion about a subject and doesnt fall into one of the other categories.

  
Type
Documentation
 An error or improvement needed in the documentation.

  
Type
Epic
 Any first tier epic. That is, an epic which contains only issues as children and will not have sub-epics.

  
Type
Feature
 Some new functionality not present.

  
Type
Legendary
 A type of Epic which will contain other Epics.

  
Type
Refactor
 A code change that restructures existing code without changing its external behavior.

  
Type
Support
 Someone needs help using the project.

  
Type
Task
 A generic task that doesnt fit into the other type categories.

  
Type
Testing
 Work exclusively focusing on fixing or expanding testing.

No labels
auto/needs-reevaluation
controller-managed
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
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
cleveragents/cleveragents-core!5717
No description provided.