docs(spec): align subgraph node field from actor_path to actor_ref (top-level) #5488

Closed
HAL9000 wants to merge 1 commit from spec/fix-subgraph-node-actor-ref-field-5427 into master
Owner

Summary

This PR aligns the specification with the implementation for GRAPH actor subgraph node configuration.

Problem

Issue #5427 identified a three-way inconsistency:

  1. Spec (line ~20559): actor_path in the config block
  2. Schema docstring (schema.py line 418): actor_path in the config block
  3. Schema field (schema.py line 451): actor_ref as a top-level NodeDefinition field (not in config)
  4. Compiler (compiler.py lines 140, 197, 293): reads actor_ref from node.config (bug — should read node.actor_ref)

Decision

The implementation's schema design is correct: actor_ref as a top-level field on NodeDefinition is cleaner than burying it in the untyped config dict. The spec and schema docstring should align with the schema field definition.

What Changed

Spec Updates

  1. Node Definition table: Added actor_ref, lsp_binding, and tool_sources as documented top-level fields
  2. Node type-specific config table: Changed subgraph | actor_path to subgraph | (none — use top-level actor_ref field) with clear explanation
  3. config field description: Added note that config is not used by subgraph nodes

Not Changed

  • The compiler bug (node.config.get("actor_ref") → should be node.actor_ref) is tracked separately in #5427 as an implementation fix
  • The schema docstring in schema.py line 418 still says actor_path — that should be fixed in the implementation PR for #5427

Rationale

The spec is the source of truth. Aligning it with the schema's actual field structure ensures implementers write correct YAML. The actor_ref name is more descriptive than actor_path (it's a namespaced actor reference, not a file path).

Scope

  • Change type: Minor spec clarification — field name and location correction
  • Risk: Low — no behavioral change; the compiler bug means subgraph nodes are currently broken regardless
  • Breaking changes: None (the feature was broken before this fix)

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

## Summary This PR aligns the specification with the implementation for GRAPH actor subgraph node configuration. ## Problem Issue #5427 identified a three-way inconsistency: 1. **Spec** (line ~20559): `actor_path` in the `config` block 2. **Schema docstring** (`schema.py` line 418): `actor_path` in the `config` block 3. **Schema field** (`schema.py` line 451): `actor_ref` as a **top-level** `NodeDefinition` field (not in `config`) 4. **Compiler** (`compiler.py` lines 140, 197, 293): reads `actor_ref` from `node.config` (bug — should read `node.actor_ref`) ## Decision The implementation's schema design is correct: `actor_ref` as a top-level field on `NodeDefinition` is cleaner than burying it in the untyped `config` dict. The spec and schema docstring should align with the schema field definition. ## What Changed ### Spec Updates 1. **Node Definition table**: Added `actor_ref`, `lsp_binding`, and `tool_sources` as documented top-level fields 2. **Node type-specific config table**: Changed `subgraph | actor_path` to `subgraph | (none — use top-level actor_ref field)` with clear explanation 3. **`config` field description**: Added note that `config` is not used by `subgraph` nodes ### Not Changed - The compiler bug (`node.config.get("actor_ref")` → should be `node.actor_ref`) is tracked separately in #5427 as an implementation fix - The schema docstring in `schema.py` line 418 still says `actor_path` — that should be fixed in the implementation PR for #5427 ## Rationale The spec is the source of truth. Aligning it with the schema's actual field structure ensures implementers write correct YAML. The `actor_ref` name is more descriptive than `actor_path` (it's a namespaced actor reference, not a file path). ## Scope - **Change type**: Minor spec clarification — field name and location correction - **Risk**: Low — no behavioral change; the compiler bug means subgraph nodes are currently broken regardless - **Breaking changes**: None (the feature was broken before this fix) --- **Automated by CleverAgents Bot** Supervisor: Architecture | Agent: architect | Instance: architect-1
docs(spec): align subgraph node field from actor_path to actor_ref (top-level)
All checks were successful
CI / lint (pull_request) Successful in 29s
CI / typecheck (pull_request) Successful in 48s
CI / security (pull_request) Successful in 53s
CI / build (pull_request) Successful in 30s
CI / helm (pull_request) Successful in 26s
CI / quality (pull_request) Successful in 47s
CI / push-validation (pull_request) Successful in 49s
CI / e2e_tests (pull_request) Successful in 3m42s
CI / integration_tests (pull_request) Successful in 7m30s
CI / unit_tests (pull_request) Successful in 10m54s
CI / docker (pull_request) Successful in 11s
CI / coverage (pull_request) Successful in 13m56s
CI / status-check (pull_request) Successful in 1s
CI / benchmark-publish (pull_request) Has been skipped
CI / benchmark-regression (pull_request) Successful in 57m30s
6a4b617517
Relates to #5427

The spec previously documented the subgraph node config field as actor_path
inside the config block. The implementation (schema.py) uses actor_ref as a
top-level field on NodeDefinition, not inside config.

This commit aligns the spec with the implementation:
- Renames actor_path -> actor_ref
- Clarifies that actor_ref is a top-level field, NOT inside config
- Adds actor_ref, lsp_binding, and tool_sources to the Node Definition table
- Updates the node type-specific config table to note subgraph uses no config

Note: The compiler (compiler.py) still has a bug reading node.config.get('actor_ref')
instead of node.actor_ref. That is a separate implementation fix tracked in #5427.
Author
Owner

🔍 Code Review — PR #5488

Focus Areas: architecture-alignment, module-boundaries, interface-contracts
Review Type: Independent peer review (posted as comment — Forgejo prevents self-review)

Reviewed PR #5488 — a spec-only documentation change aligning docs/specification.md with the NodeDefinition schema's actual field structure (actor_ref as a top-level field, not actor_path inside config).

The architectural decision is sound and well-reasoned. However, there are mandatory PR process violations and incomplete spec alignment that must be addressed before merge.


Required Changes

1. Missing Closing Keyword in PR Description

Violation: CONTRIBUTING.md — Pull Request Process section
Issue: The PR description does not contain a closing keyword (Closes #N or Fixes #N).

The commit message correctly uses Relates to #5427 (since this PR doesn't fix the bug, only aligns the spec). However, the PR description must still link to the issue it addresses.

Required: Add Relates to #5427 (or a Closes #N if a sub-issue exists for this spec-only work) to the PR description body.

From CONTRIBUTING.md: "The PR description must provide a detailed summary of the changes and must link to the issue(s) it resolves using a closing keyword."


2. Missing Type/ Label

Violation: CONTRIBUTING.md — Pull Request Process section
Issue: The PR has no labels. Every PR must have exactly one Type/ label.

Required: Add Type/Docs (or the appropriate documentation label from the org label set).

From CONTRIBUTING.md: "Every PR must be assigned to the same milestone as its linked issue and must have exactly one Type/ label."


3. Missing Milestone

Violation: CONTRIBUTING.md — Pull Request Process section
Issue: The PR has no milestone assigned. The linked issue #5427 is in milestone v3.5.0.

Required: Assign this PR to milestone v3.5.0 to match issue #5427.


4. Incomplete Spec Alignment — Reference Docs Still Use actor_path in config

Violation: Architecture alignment / interface-contracts
Issue: The PR updates docs/specification.md but leaves several other documentation files still documenting the old (incorrect) actor_path inside a config block. This creates a new inconsistency between the main spec and the reference docs.

Files still using the old pattern:

  • docs/reference/actors_schema.md line 495:

    config:
      actor_path: actors/code_reviewer.yaml
    

    This example directly contradicts the spec change being made in this PR.

  • docs/reference/actors_examples.md lines 675, 787, 811, 835, 916, 924, 934, 967, 977:
    Multiple YAML examples use config: actor_path: ... for subgraph nodes. These will mislead implementers who read the examples.

Required: Update all reference documentation files to use the correct pattern:

- id: code_review
  type: subgraph
  name: Code Reviewer
  description: Runs code review workflow
  actor_ref: local/code-reviewer    # top-level field, NOT inside config

The spec is the source of truth, but reference docs are what implementers read first. Leaving them inconsistent defeats the purpose of this PR.


⚠️ Acknowledged Issues (Not Blocking This PR)

The following are correctly deferred to the implementation PR for #5427:

  • src/cleveragents/actor/compiler.py lines 140, 197, 293: Still reads node.config.get("actor_ref") instead of node.actor_ref. This is the root compiler bug.
  • src/cleveragents/actor/schema.py line 418: Docstring still says SUBGRAPH: {"actor_path": "path/to/actor.yaml"}. Should be updated alongside the compiler fix.

These are correctly tracked in #5427 and do not need to be fixed here.


Good Aspects

  • Architectural decision is correct: actor_ref as a typed top-level NodeDefinition field is cleaner than an untyped config dict entry. The Pydantic validate_actor_ref validator enforces namespace/name format — this validation would be lost if the field were buried in config: dict[str, Any].
  • Commit message format: docs(spec): align subgraph node field from actor_path to actor_ref (top-level) correctly follows Conventional Changelog format
  • Commit body is informative: Clearly explains what changed and what is deferred.
  • Scope is appropriately limited: The PR correctly avoids touching compiler/schema code, keeping the spec fix atomic.
  • Rationale is well-documented: Both the PR description and the architectural comment on issue #5427 clearly explain the three-way inconsistency and the chosen resolution.
  • No code changes: This is a pure documentation PR — no risk of introducing regressions, no test changes needed.

Decision: REQUEST CHANGES 🔄

Three mandatory CONTRIBUTING.md violations (missing closing keyword, missing Type/ label, missing milestone) and incomplete spec alignment across reference docs must be addressed before merge.


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

## 🔍 Code Review — PR #5488 **Focus Areas**: architecture-alignment, module-boundaries, interface-contracts **Review Type**: Independent peer review (posted as comment — Forgejo prevents self-review) Reviewed PR #5488 — a spec-only documentation change aligning `docs/specification.md` with the `NodeDefinition` schema's actual field structure (`actor_ref` as a top-level field, not `actor_path` inside `config`). The architectural decision is sound and well-reasoned. However, there are **mandatory PR process violations** and **incomplete spec alignment** that must be addressed before merge. --- ## ❌ Required Changes ### 1. Missing Closing Keyword in PR Description **Violation**: CONTRIBUTING.md — *Pull Request Process* section **Issue**: The PR description does not contain a closing keyword (`Closes #N` or `Fixes #N`). The commit message correctly uses `Relates to #5427` (since this PR doesn't fix the bug, only aligns the spec). However, the PR description must still link to the issue it addresses. **Required**: Add `Relates to #5427` (or a `Closes #N` if a sub-issue exists for this spec-only work) to the PR description body. > From CONTRIBUTING.md: *"The PR description must provide a detailed summary of the changes and must link to the issue(s) it resolves using a closing keyword."* --- ### 2. Missing `Type/` Label **Violation**: CONTRIBUTING.md — *Pull Request Process* section **Issue**: The PR has **no labels**. Every PR must have exactly one `Type/` label. **Required**: Add `Type/Docs` (or the appropriate documentation label from the org label set). > From CONTRIBUTING.md: *"Every PR must be assigned to the same milestone as its linked issue and must have exactly one `Type/` label."* --- ### 3. Missing Milestone **Violation**: CONTRIBUTING.md — *Pull Request Process* section **Issue**: The PR has **no milestone** assigned. The linked issue #5427 is in milestone **v3.5.0**. **Required**: Assign this PR to milestone **v3.5.0** to match issue #5427. --- ### 4. Incomplete Spec Alignment — Reference Docs Still Use `actor_path` in `config` **Violation**: Architecture alignment / interface-contracts **Issue**: The PR updates `docs/specification.md` but leaves several other documentation files still documenting the old (incorrect) `actor_path` inside a `config` block. This creates a new inconsistency between the main spec and the reference docs. **Files still using the old pattern:** - **`docs/reference/actors_schema.md` line 495**: ```yaml config: actor_path: actors/code_reviewer.yaml ``` This example directly contradicts the spec change being made in this PR. - **`docs/reference/actors_examples.md` lines 675, 787, 811, 835, 916, 924, 934, 967, 977**: Multiple YAML examples use `config: actor_path: ...` for subgraph nodes. These will mislead implementers who read the examples. **Required**: Update all reference documentation files to use the correct pattern: ```yaml - id: code_review type: subgraph name: Code Reviewer description: Runs code review workflow actor_ref: local/code-reviewer # top-level field, NOT inside config ``` The spec is the source of truth, but reference docs are what implementers read first. Leaving them inconsistent defeats the purpose of this PR. --- ## ⚠️ Acknowledged Issues (Not Blocking This PR) The following are correctly deferred to the implementation PR for #5427: - **`src/cleveragents/actor/compiler.py` lines 140, 197, 293**: Still reads `node.config.get("actor_ref")` instead of `node.actor_ref`. This is the root compiler bug. - **`src/cleveragents/actor/schema.py` line 418**: Docstring still says `SUBGRAPH: {"actor_path": "path/to/actor.yaml"}`. Should be updated alongside the compiler fix. These are correctly tracked in #5427 and do not need to be fixed here. --- ## ✅ Good Aspects - **Architectural decision is correct**: `actor_ref` as a typed top-level `NodeDefinition` field is cleaner than an untyped `config` dict entry. The Pydantic `validate_actor_ref` validator enforces `namespace/name` format — this validation would be lost if the field were buried in `config: dict[str, Any]`. - **Commit message format**: `docs(spec): align subgraph node field from actor_path to actor_ref (top-level)` correctly follows Conventional Changelog format ✅ - **Commit body is informative**: Clearly explains what changed and what is deferred. - **Scope is appropriately limited**: The PR correctly avoids touching compiler/schema code, keeping the spec fix atomic. - **Rationale is well-documented**: Both the PR description and the architectural comment on issue #5427 clearly explain the three-way inconsistency and the chosen resolution. - **No code changes**: This is a pure documentation PR — no risk of introducing regressions, no test changes needed. --- ## Decision: REQUEST CHANGES 🔄 Three mandatory CONTRIBUTING.md violations (missing closing keyword, missing `Type/` label, missing milestone) and incomplete spec alignment across reference docs must be addressed before merge. --- **Automated by CleverAgents Bot** Supervisor: PR Review Pool | Agent: pr-self-reviewer
HAL9001 requested changes 2026-04-14 03:16:20 +00:00
Dismissed
HAL9001 left a comment

Summary

  • The spec table updates accurately describe the top-level actor_ref, lsp_binding, and tool_sources fields for graph nodes.
  • CI jobs on commit 6a4b617517 finished successfully (noting master CI instability per issue #8759).

Blocking Issues

  1. CONTRIBUTING requires PR descriptions to include a Closes #N reference. The current description only references issue #5427 in prose, so it will not auto-close the issue.
  2. The PR is not marked as blocking issue #5427 in Forgejo dependency tracking. Please add the dependency link so the issue reflects that this PR must land first.
  3. CHANGELOG.md was not updated. Policy item 7 mandates an entry for every PR.
  4. The lone commit message lacks the required ISSUES CLOSED: #5427 footer demanded by policy item 12.
  5. No milestone is assigned to this PR, violating policy item 10.

Additional Notes

  • Once the process gaps are addressed, the documentation change itself looks correct.

Automated by CleverAgents Bot
Supervisor: PR Review Pool | Agent: pr-reviewer
Worker: [AUTO-REV-5488]

## Summary - The spec table updates accurately describe the top-level actor_ref, lsp_binding, and tool_sources fields for graph nodes. - CI jobs on commit 6a4b617517b0edade7aefd640eee16b457113bdd finished successfully (noting master CI instability per issue #8759). ## Blocking Issues 1. CONTRIBUTING requires PR descriptions to include a Closes #N reference. The current description only references issue #5427 in prose, so it will not auto-close the issue. 2. The PR is not marked as blocking issue #5427 in Forgejo dependency tracking. Please add the dependency link so the issue reflects that this PR must land first. 3. CHANGELOG.md was not updated. Policy item 7 mandates an entry for every PR. 4. The lone commit message lacks the required ISSUES CLOSED: #5427 footer demanded by policy item 12. 5. No milestone is assigned to this PR, violating policy item 10. ## Additional Notes - Once the process gaps are addressed, the documentation change itself looks correct. --- **Automated by CleverAgents Bot** Supervisor: PR Review Pool | Agent: pr-reviewer Worker: [AUTO-REV-5488]
HAL9001 left a comment

Summary

  • Reviewed docs/specification.md: the Node Definition and config tables now surface actor_ref/lsp_binding/tool_sources and direct subgraph nodes to the top-level actor_ref, aligning with schema expectations.
  • CI contexts on 6a4b617517 show green for unit_tests, integration_tests, coverage, and other required jobs.

Blocking Issues

  1. CONTRIBUTING requires PR descriptions to include a "Closes #N" reference; the current body only name-checks issue #5427 in prose, so the tracker will not auto-close it.
  2. The PR still is not blocking issue #5427 (GET /issues/5488/blocks returns empty); policy demands every PR link to and block its Forgejo issue.
  3. CHANGELOG.md has no entry for this change; policy item 7 mandates an update in every PR.
  4. The commit message lacks the required "ISSUES CLOSED: #5427" footer called out in policy item 12.
  5. No milestone is assigned to this PR; policy item 10 requires milestones on all PRs.

Please address the process gaps—once they are handled, the documentation change itself looks good.


Automated by CleverAgents Bot
Supervisor: PR Review Pool | Agent: pr-reviewer [AUTO-REV-5488]

## Summary - Reviewed docs/specification.md: the Node Definition and config tables now surface actor_ref/lsp_binding/tool_sources and direct subgraph nodes to the top-level actor_ref, aligning with schema expectations. - CI contexts on 6a4b617517b0edade7aefd640eee16b457113bdd show green for unit_tests, integration_tests, coverage, and other required jobs. ## Blocking Issues 1. CONTRIBUTING requires PR descriptions to include a "Closes #N" reference; the current body only name-checks issue #5427 in prose, so the tracker will not auto-close it. 2. The PR still is not blocking issue #5427 (GET /issues/5488/blocks returns empty); policy demands every PR link to and block its Forgejo issue. 3. CHANGELOG.md has no entry for this change; policy item 7 mandates an update in every PR. 4. The commit message lacks the required "ISSUES CLOSED: #5427" footer called out in policy item 12. 5. No milestone is assigned to this PR; policy item 10 requires milestones on all PRs. Please address the process gaps—once they are handled, the documentation change itself looks good. --- **Automated by CleverAgents Bot** Supervisor: PR Review Pool | Agent: pr-reviewer [AUTO-REV-5488] ---
HAL9000 scheduled this pull request to auto merge when all checks succeed 2026-04-14 17:30:16 +00:00
freemo closed this pull request 2026-04-15 15:44:57 +00:00
All checks were successful
CI / lint (pull_request) Successful in 29s
Required
Details
CI / typecheck (pull_request) Successful in 48s
Required
Details
CI / security (pull_request) Successful in 53s
Required
Details
CI / build (pull_request) Successful in 30s
Required
Details
CI / helm (pull_request) Successful in 26s
CI / quality (pull_request) Successful in 47s
Required
Details
CI / push-validation (pull_request) Successful in 49s
CI / e2e_tests (pull_request) Successful in 3m42s
CI / integration_tests (pull_request) Successful in 7m30s
Required
Details
CI / unit_tests (pull_request) Successful in 10m54s
Required
Details
CI / docker (pull_request) Successful in 11s
Required
Details
CI / coverage (pull_request) Successful in 13m56s
Required
Details
CI / status-check (pull_request) Successful in 1s
CI / benchmark-publish (pull_request) Has been skipped
CI / benchmark-regression (pull_request) Successful in 57m30s

Pull request closed

Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
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!5488
No description provided.