cleveragents/cleveragents-core
4
0
Fork 3
Code Issues 6.0k Pull Requests 115 Actions Packages Projects Releases Wiki Activity

fix(cli): make actor NAME argument optional, derive from YAML config #10974

Merged
CoreRasurae merged 6 commits from bugfix/actor-add-name-arg into master 2026-05-07 20:05:35 +00:00
Conversation 16 Commits 6 Files Changed 24
+205 -70
CoreRasurae commented 2026-05-05 17:08:30 +00:00
Member
Copy Link
Copy Source

Summary

  • Make NAME positional argument optional in agents actor add command
  • When NAME is omitted, derive actor name from config file name field
  • Raises BadParameter if neither NAME argument nor config name field is provided
  • Updated CLI help and docstring signature to reflect optional NAME

Issue

Fixes issue #4186 - CLI signature contradiction with specification

Changes

  • src/cleveragents/cli/commands/actor.py: Implementation changes
  • features/actor_add_name_positional.feature: Test scenarios
  • features/steps/actor_add_name_positional_steps.py: Step definitions

Testing

  • Lint: All checks passed
  • Typecheck: 0 errors, 3 warnings (unresolved imports - pre-existing)
  • Behave tests: All 3 scenarios pass
## Summary - Make NAME positional argument optional in agents actor add command - When NAME is omitted, derive actor name from config file name field - Raises BadParameter if neither NAME argument nor config name field is provided - Updated CLI help and docstring signature to reflect optional NAME ## Issue Fixes issue #4186 - CLI signature contradiction with specification ## Changes - src/cleveragents/cli/commands/actor.py: Implementation changes - features/actor_add_name_positional.feature: Test scenarios - features/steps/actor_add_name_positional_steps.py: Step definitions ## Testing - Lint: All checks passed - Typecheck: 0 errors, 3 warnings (unresolved imports - pre-existing) - Behave tests: All 3 scenarios pass
HAL9000 requested changes 2026-05-05 17:56:36 +00:00
HAL9000 left a comment
Owner
Copy Link
Copy Source

CI Status — BLOCKING

No CI checks have been reported for this PR yet. All individual check statuses are pending (null).

  • CI / lint (pull_request) — PENDING
  • CI / typecheck (pull_request) — PENDING
  • CI / security (pull_request) — PENDING
  • CI / unit_tests (pull_request) — PENDING
  • CI / coverage (pull_request) — PENDING
  • CI / e2e_tests (pull_request) — PENDING
  • CI / status-check (pull_request) — PENDING

Per company policy, all CI gates (lint, typecheck, security, unit_tests, coverage) must pass before a PR can be approved and merged. Please ensure CI is configured and all required checks complete successfully.

Note: Once CI checks have completed and are passing, a full code review will be conducted.

## CI Status — BLOCKING No CI checks have been reported for this PR yet. All individual check statuses are pending (null). - `CI / lint (pull_request)` — PENDING - `CI / typecheck (pull_request)` — PENDING - `CI / security (pull_request)` — PENDING - `CI / unit_tests (pull_request)` — PENDING - `CI / coverage (pull_request)` — PENDING - `CI / e2e_tests (pull_request)` — PENDING - `CI / status-check (pull_request)` — PENDING Per company policy, all CI gates (lint, typecheck, security, unit_tests, coverage) must pass before a PR can be approved and merged. Please ensure CI is configured and all required checks complete successfully. Note: Once CI checks have completed and are passing, a full code review will be conducted.
HAL9000 commented 2026-05-05 17:58:34 +00:00
Owner
Copy Link
Copy Source

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

--- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
CoreRasurae added the Needs Feedback label 2026-05-05 22:29:38 +00:00
CoreRasurae commented 2026-05-05 22:29:56 +00:00
Author
Member
Copy Link
Copy Source

i am still working on this ticket

i am still working on this ticket
HAL9001 requested changes 2026-05-06 14:08:12 +00:00
Dismissed
HAL9001 left a comment
Owner
Copy Link
Copy Source

Re-Review: PR #10974 — fix(cli): make actor NAME argument optional

Previous Feedback Addressed?

The prior review (#7617) requested that CI checks be configured and passing. CI is now running, which is a step forward. However, CI is still failing on three required gates, so this PR cannot be approved.

🔴 BLOCKER 1 — CI is Failing on Required Gates

The following required-for-merge CI checks are failing:

  • CI / unit_tests — FAILING
  • CI / integration_tests — FAILING
  • CI / benchmark-regression — FAILING
  • CI / coverage — SKIPPED (cannot run when unit_tests fail)
  • CI / status-check — FAILING (aggregate gate)

Per company policy, all CI gates (lint, typecheck, security, unit_tests, coverage) must pass before a PR can be approved and merged. The unit_tests and coverage failures are hard merge blockers.

What to do: Fix the failing unit tests and integration tests, then ensure the benchmark regression check passes. Only once all required gates are green can a full approval be granted.

🔴 BLOCKER 2 — Branch Contains Wrong Commits (No Actor NAME Fix Implemented)

This is the most critical finding. The PR is titled "fix(cli): make actor NAME argument optional, derive from YAML config" and claims to fix issue #4186, but the branch bugfix/actor-add-name-arg contains zero commits implementing this fix.

The 4 unique commits in this branch relative to master are:

  1. 5e016cbc — feat(a2a): implement JSON-RPC 2.0 wire format and method routing
  2. 86133e61 — fix(a2a): resolve reviewer blocking issues in JSON-RPC facade PR
  3. b20711ce — fix(a2a): update operation count from 42 to 53 in tests and docs
  4. 4c415567 — fix(a2a): fix lint/format failures and remove incorrect tdd_expected_fail tags

All 4 commits are A2A-related (JSON-RPC wire format, Agent Card generation, standard A2A method routing). None of them touch src/cleveragents/cli/commands/actor.py, and none of them implement the actor NAME argument changes described in issue #4186.

The PR description claims changes to:

  • src/cleveragents/cli/commands/actor.py — not present in the diff
  • features/actor_add_name_positional.feature — not present in the diff
  • features/steps/actor_add_name_positional_steps.py — not present in the diff

The actual diff contains: A2A facade module refactoring (facade.py split into _facade.py, _handlers.py, _handlers_extension.py, _handlers_standard.py, _operations.py), a new agent_card.py module, new BDD feature file for A2A agent card, and Robot Framework integration tests.

What to do: The branch needs to be rebased onto master and the actor NAME argument fix (as specified in issue #4186) must be implemented. If the A2A work belongs to a different issue/PR, it should be extracted to its own appropriately named branch. The author confirmed they are still working on this ticket — please complete the implementation and update the PR when ready.

🔴 BLOCKER 3 — PR Metadata is Incomplete

Several required PR metadata fields are missing:

  • No milestone assigned — the linked issue #4186 is on milestone v3.5.0; this PR must also be assigned to v3.5.0.
  • No Type/ label — exactly one Type/ label is required (e.g. Type/Bug). Currently only Needs Feedback is present.
  • Commit footers missing ISSUES CLOSED: #4186 — per CONTRIBUTING.md, every commit footer must reference the issue it closes.

What to do: Assign the PR to milestone v3.5.0, apply the Type/Bug label, and ensure each commit footer contains ISSUES CLOSED: #4186.

Summary

This PR requires significant rework before it can be reviewed properly:

  1. Implement the actual fix — make NAME optional in agents actor add, derive from YAML name field, raise BadParameter if neither is provided.
  2. Fix failing CI — unit_tests, integration_tests, and coverage must all be green.
  3. Fix PR metadata — add milestone v3.5.0, Type/Bug label, and ISSUES CLOSED: #4186 commit footers.

Note: The A2A work currently in this branch appears to be misplaced. If it is intended for a different issue, it should be moved to its own branch with an appropriate name and linked to the correct issue.

A full code review against the review checklist will be conducted once the actor NAME fix is implemented and CI is passing.

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

## Re-Review: PR #10974 — fix(cli): make actor NAME argument optional ### Previous Feedback Addressed? The prior review (#7617) requested that CI checks be configured and passing. CI is now **running**, which is a step forward. However, CI is still failing on three required gates, so this PR cannot be approved. --- ### 🔴 BLOCKER 1 — CI is Failing on Required Gates The following required-for-merge CI checks are **failing**: - `CI / unit_tests` — **FAILING** - `CI / integration_tests` — **FAILING** - `CI / benchmark-regression` — **FAILING** - `CI / coverage` — **SKIPPED** (cannot run when unit_tests fail) - `CI / status-check` — **FAILING** (aggregate gate) Per company policy, all CI gates (lint, typecheck, security, unit_tests, coverage) must pass before a PR can be approved and merged. The unit_tests and coverage failures are hard merge blockers. **What to do:** Fix the failing unit tests and integration tests, then ensure the benchmark regression check passes. Only once all required gates are green can a full approval be granted. --- ### 🔴 BLOCKER 2 — Branch Contains Wrong Commits (No Actor NAME Fix Implemented) This is the most critical finding. The PR is titled **"fix(cli): make actor NAME argument optional, derive from YAML config"** and claims to fix issue #4186, but the branch `bugfix/actor-add-name-arg` contains **zero commits** implementing this fix. The 4 unique commits in this branch relative to `master` are: 1. `5e016cbc` — `feat(a2a): implement JSON-RPC 2.0 wire format and method routing` 2. `86133e61` — `fix(a2a): resolve reviewer blocking issues in JSON-RPC facade PR` 3. `b20711ce` — `fix(a2a): update operation count from 42 to 53 in tests and docs` 4. `4c415567` — `fix(a2a): fix lint/format failures and remove incorrect tdd_expected_fail tags` All 4 commits are A2A-related (JSON-RPC wire format, Agent Card generation, standard A2A method routing). **None of them touch `src/cleveragents/cli/commands/actor.py`**, and none of them implement the actor NAME argument changes described in issue #4186. The PR description claims changes to: - `src/cleveragents/cli/commands/actor.py` — **not present in the diff** - `features/actor_add_name_positional.feature` — **not present in the diff** - `features/steps/actor_add_name_positional_steps.py` — **not present in the diff** The actual diff contains: A2A facade module refactoring (`facade.py` split into `_facade.py`, `_handlers.py`, `_handlers_extension.py`, `_handlers_standard.py`, `_operations.py`), a new `agent_card.py` module, new BDD feature file for A2A agent card, and Robot Framework integration tests. **What to do:** The branch needs to be rebased onto `master` and the actor NAME argument fix (as specified in issue #4186) must be implemented. If the A2A work belongs to a different issue/PR, it should be extracted to its own appropriately named branch. The author confirmed they are still working on this ticket — please complete the implementation and update the PR when ready. --- ### 🔴 BLOCKER 3 — PR Metadata is Incomplete Several required PR metadata fields are missing: - **No milestone assigned** — the linked issue #4186 is on milestone `v3.5.0`; this PR must also be assigned to `v3.5.0`. - **No `Type/` label** — exactly one `Type/` label is required (e.g. `Type/Bug`). Currently only `Needs Feedback` is present. - **Commit footers missing `ISSUES CLOSED: #4186`** — per CONTRIBUTING.md, every commit footer must reference the issue it closes. **What to do:** Assign the PR to milestone `v3.5.0`, apply the `Type/Bug` label, and ensure each commit footer contains `ISSUES CLOSED: #4186`. --- ### Summary This PR requires significant rework before it can be reviewed properly: 1. **Implement the actual fix** — make `NAME` optional in `agents actor add`, derive from YAML `name` field, raise `BadParameter` if neither is provided. 2. **Fix failing CI** — unit_tests, integration_tests, and coverage must all be green. 3. **Fix PR metadata** — add milestone `v3.5.0`, `Type/Bug` label, and `ISSUES CLOSED: #4186` commit footers. Note: The A2A work currently in this branch appears to be misplaced. If it is intended for a different issue, it should be moved to its own branch with an appropriate name and linked to the correct issue. A full code review against the review checklist will be conducted once the actor NAME fix is implemented and CI is passing. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
HAL9001 commented 2026-05-06 14:30:06 +00:00
Owner
Copy Link
Copy Source

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

--- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
HAL9001 requested changes 2026-05-06 16:18:11 +00:00
Dismissed
HAL9001 left a comment
Owner
Copy Link
Copy Source

Re-Review: fix(cli): make actor NAME argument optional, derive from YAML config

Prior Feedback Status

The previous review flagged that CI checks were not running. CI is now running — partial progress. The core implementation logic is correct and aligns with the specification. However, several blocking issues remain that must be resolved before this PR can be approved.

CI Status — BLOCKING

CI is now running, but the following gates are failing:

Check Status
CI / lint ✅ Passing
CI / typecheck ✅ Passing
CI / security ✅ Passing
CI / unit_tests ❌ FAILING
CI / integration_tests ❌ FAILING
CI / coverage ⏭️ Skipped (unit_tests failed)
CI / benchmark-regression ❌ FAILING
CI / status-check ❌ FAILING

Per company policy, all CI gates (lint, typecheck, security, unit_tests, coverage) must pass before a PR can be approved. The unit_tests failure also means coverage cannot be verified against the required ≥97% threshold. Please investigate and fix the test failures.

Blocking Issues

1. Unit tests and integration tests failing (CI gate)

The PR adds AmbiguousStep collision fixes across many feature files and step modules — these were pre-existing issues but the changes need to resolve the failures entirely, not partially. The CI/unit_tests and CI/integration_tests jobs are both failing. Since unit_tests is failing, coverage is also skipped, making it impossible to verify the ≥97% coverage threshold. Please run nox -s unit_tests and nox -s integration_tests locally and fix all failures before resubmitting.

2. Missing ISSUES CLOSED: footer in commit messages

Per CONTRIBUTING.md, every commit footer must include ISSUES CLOSED: #N. The implementation commit (4e8f0da6) uses Fixes issue #4186 in the body — this is the wrong format and location. The correct form is a dedicated footer line:

ISSUES CLOSED: #4186

The other three commits (6a74f996, 0f7592f7, 2d68c7a2) have no issue reference at all. Every commit must reference its issue in the footer. Please squash/rebase the commit history to fix the footers.

3. No milestone assigned to the PR

Issue #4186 is in milestone v3.5.0. Per CONTRIBUTING.md, the PR must be assigned to the same milestone as the linked issue. Currently the PR has no milestone. Please assign v3.5.0.

4. Missing Type/Bug label on PR

Per CONTRIBUTING.md, every PR must have exactly one Type/ label. Issue #4186 is Type/Bug, so this PR requires a Type/Bug label. The current label Needs Feedback is a process label, not a type label. Please add Type/Bug.

5. No Forgejo dependency linking (PR must block issue)

Per CONTRIBUTING.md (critical rule): the PR must be set to block issue #4186 (not the reverse). This creates the correct dependency: issue #4186 depends on this PR. Currently no dependency is set. Please open the PR → Sidebar → "This pull request blocks" → add issue #4186.

6. docs/specification.md not updated

Acceptance criterion #3 in issue #4186 explicitly requires: "The specification in docs/specification.md is updated to match the actual CLI behavior." This PR updates the CLI and adds tests but does not update docs/specification.md to reflect the new agents actor add command signature where NAME is optional. The spec at lines 4938-4948 and line 279 must be updated to reflect the corrected signature agents actor add --config|-c <FILE> [<NAME>].

7. Missing CHANGELOG entry

Per CONTRIBUTING.md, every PR must include a CHANGELOG update with one entry describing the user-facing change. No CHANGELOG entry is present in any of the four commits.

Non-Blocking Observations

Branch name missing milestone prefix

The branch is named bugfix/actor-add-name-arg. Per CONTRIBUTING.md, bug fix branches must be named bugfix/mN-<descriptive-name> where N is the milestone number. For milestone v3.5.0 (M5), this should be bugfix/m5-actor-add-name-arg. While the branch cannot be renamed post-PR-creation without complications, note this for future issues.

TDD workflow for bug fixes

Issue #4186 is Type/Bug. The TDD workflow requires a companion TDD issue with a separate tdd/mN- branch that proves the bug exists (with @tdd_expected_fail tag) before the fix is applied. The new scenario actor add without NAME positional argument uses config name tagged with @tdd_issue @tdd_issue_4186 is the passing test for the fix — it should be on a tdd/ branch as a @tdd_expected_fail scenario first. Please verify the TDD companion issue/branch was created and submitted per the TDD workflow.

Correctness of the implementation

The core implementation logic in actor.py is correct: name: str | None = None, derivation from config_blob.get("name"), and BadParameter if neither is provided. This aligns with the specification. Good work on the implementation itself.

Summary

The implementation approach is correct and aligns with the specification. The primary blockers are: failing CI tests, missing commit footer format, missing PR metadata (milestone, Type label, dependency link), missing spec documentation update, and missing CHANGELOG entry. The author has noted they are still working on this ticket — please address all blocking items above before requesting re-review.

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

## Re-Review: fix(cli): make actor NAME argument optional, derive from YAML config ### Prior Feedback Status The previous review flagged that CI checks were not running. CI is now running — partial progress. The core implementation logic is correct and aligns with the specification. However, several **blocking issues** remain that must be resolved before this PR can be approved. --- ## CI Status — BLOCKING CI is now running, but the following gates are **failing**: | Check | Status | |---|---| | `CI / lint` | ✅ Passing | | `CI / typecheck` | ✅ Passing | | `CI / security` | ✅ Passing | | `CI / unit_tests` | ❌ **FAILING** | | `CI / integration_tests` | ❌ **FAILING** | | `CI / coverage` | ⏭️ Skipped (unit_tests failed) | | `CI / benchmark-regression` | ❌ **FAILING** | | `CI / status-check` | ❌ **FAILING** | Per company policy, all CI gates (lint, typecheck, security, unit_tests, coverage) must pass before a PR can be approved. The unit_tests failure also means coverage cannot be verified against the required ≥97% threshold. Please investigate and fix the test failures. --- ## Blocking Issues ### 1. Unit tests and integration tests failing (CI gate) The PR adds AmbiguousStep collision fixes across many feature files and step modules — these were pre-existing issues but the changes need to resolve the failures entirely, not partially. The CI/unit_tests and CI/integration_tests jobs are both failing. Since unit_tests is failing, coverage is also skipped, making it impossible to verify the ≥97% coverage threshold. Please run `nox -s unit_tests` and `nox -s integration_tests` locally and fix all failures before resubmitting. ### 2. Missing `ISSUES CLOSED:` footer in commit messages Per CONTRIBUTING.md, every commit footer **must** include `ISSUES CLOSED: #N`. The implementation commit (`4e8f0da6`) uses `Fixes issue #4186` in the body — this is the wrong format and location. The correct form is a dedicated footer line: ``` ISSUES CLOSED: #4186 ``` The other three commits (`6a74f996`, `0f7592f7`, `2d68c7a2`) have **no issue reference at all**. Every commit must reference its issue in the footer. Please squash/rebase the commit history to fix the footers. ### 3. No milestone assigned to the PR Issue #4186 is in milestone `v3.5.0`. Per CONTRIBUTING.md, the PR must be assigned to the same milestone as the linked issue. Currently the PR has no milestone. Please assign `v3.5.0`. ### 4. Missing `Type/Bug` label on PR Per CONTRIBUTING.md, every PR must have exactly one `Type/` label. Issue #4186 is `Type/Bug`, so this PR requires a `Type/Bug` label. The current label `Needs Feedback` is a process label, not a type label. Please add `Type/Bug`. ### 5. No Forgejo dependency linking (PR must block issue) Per CONTRIBUTING.md (critical rule): the PR must be set to **block** issue #4186 (not the reverse). This creates the correct dependency: issue #4186 depends on this PR. Currently no dependency is set. Please open the PR → Sidebar → "This pull request blocks" → add issue #4186. ### 6. `docs/specification.md` not updated Acceptance criterion #3 in issue #4186 explicitly requires: _"The specification in `docs/specification.md` is updated to match the actual CLI behavior."_ This PR updates the CLI and adds tests but does not update `docs/specification.md` to reflect the new `agents actor add` command signature where `NAME` is optional. The spec at lines 4938-4948 and line 279 must be updated to reflect the corrected signature `agents actor add --config|-c <FILE> [<NAME>]`. ### 7. Missing CHANGELOG entry Per CONTRIBUTING.md, every PR must include a CHANGELOG update with one entry describing the user-facing change. No CHANGELOG entry is present in any of the four commits. --- ## Non-Blocking Observations ### Branch name missing milestone prefix The branch is named `bugfix/actor-add-name-arg`. Per CONTRIBUTING.md, bug fix branches must be named `bugfix/mN-<descriptive-name>` where N is the milestone number. For milestone v3.5.0 (M5), this should be `bugfix/m5-actor-add-name-arg`. While the branch cannot be renamed post-PR-creation without complications, note this for future issues. ### TDD workflow for bug fixes Issue #4186 is `Type/Bug`. The TDD workflow requires a companion TDD issue with a separate `tdd/mN-` branch that proves the bug exists (with `@tdd_expected_fail` tag) before the fix is applied. The new scenario `actor add without NAME positional argument uses config name` tagged with `@tdd_issue @tdd_issue_4186` is the **passing** test for the fix — it should be on a `tdd/` branch as a `@tdd_expected_fail` scenario first. Please verify the TDD companion issue/branch was created and submitted per the TDD workflow. ### Correctness of the implementation The core implementation logic in `actor.py` is correct: `name: str | None = None`, derivation from `config_blob.get("name")`, and `BadParameter` if neither is provided. This aligns with the specification. Good work on the implementation itself. --- ## Summary The implementation approach is correct and aligns with the specification. The primary blockers are: failing CI tests, missing commit footer format, missing PR metadata (milestone, Type label, dependency link), missing spec documentation update, and missing CHANGELOG entry. The author has noted they are still working on this ticket — please address all blocking items above before requesting re-review. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
src/cleveragents/cli/commands/actor.py Outdated
HAL9001 commented 2026-05-06 16:18:11 +00:00
Owner
Copy Link
Copy Source

BLOCKING — Missing ISSUES CLOSED: footer in commit message.

The commit message for 4e8f0da6 uses Fixes issue #4186 in the body text. Per CONTRIBUTING.md, the correct format is a dedicated footer line:

ISSUES CLOSED: #4186

This applies to ALL four commits in this PR — none have a proper ISSUES CLOSED: #N footer. Please interactive-rebase to correct the commit footers before re-submitting.

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

**BLOCKING** — Missing `ISSUES CLOSED:` footer in commit message. The commit message for `4e8f0da6` uses `Fixes issue #4186` in the body text. Per CONTRIBUTING.md, the correct format is a dedicated footer line: ``` ISSUES CLOSED: #4186 ``` This applies to ALL four commits in this PR — none have a proper `ISSUES CLOSED: #N` footer. Please interactive-rebase to correct the commit footers before re-submitting. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
HAL9001 commented 2026-05-06 16:38:40 +00:00
Owner
Copy Link
Copy Source

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

--- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
CoreRasurae force-pushed bugfix/actor-add-name-arg from 6a74f996ad to 4c62ca8579 2026-05-06 18:21:57 +00:00 Compare
HAL9001 requested changes 2026-05-06 20:43:55 +00:00
Dismissed
HAL9001 left a comment
Owner
Copy Link
Copy Source

Re-Review: fix(cli): make actor NAME argument optional, derive from YAML config

Prior Feedback Addressed?

Good progress since the last review. The implementation is now present and correct, and the unit_tests / integration_tests CI gates are now passing. Several blockers from the previous review have been resolved. However, a number of required items remain unaddressed, and a new CI regression has appeared. This PR cannot be approved until all blockers below are fixed.

✅ Items Resolved from Previous Review

  • Implementation now present — actor.py correctly makes NAME optional (str | None = None), derives from config_blob.get("name"), and raises BadParameter if neither is provided.
  • CI unit_tests — now PASSING ✅
  • CI integration_tests — now PASSING ✅
  • CI typecheck — PASSING ✅
  • CI security — PASSING ✅
  • Specification update — the spec at line 279 and lines 5048–5050 already shows agents actor add --config|-c <FILE> [--update] without a required NAME argument; the spec was already aligned with the desired behavior. No spec update is needed. (This blocker from the previous review was overstated — it is resolved.)
  • AmbiguousStep collisions — resolved across multiple step files. The environment.py fixes replacing tempfile.mktemp() with tempfile.mkstemp() and adding fcntl locking are correct and well-implemented.
  • @tdd_expected_fail removal for issue #2609 — correct; those scenarios are already passing in master.

🔴 BLOCKER 1 — CI lint is NOW FAILING (Regression)

CI lint was passing in the previous review cycle but is now failing on the current head 4c62ca8579620fd491fba67e5d97d775e2bdd4bd. This is a regression introduced by the new commits.

Per company policy, CI / lint is a required merge gate. Please run nox -s lint locally, fix all lint/format errors, and push a corrected commit.

🔴 BLOCKER 2 — CI coverage is SKIPPED (Required Gate)

The CI / coverage job shows status Has been skipped on the current head. Coverage is a hard merge gate (≥97% required). When coverage is skipped, there is no guarantee the threshold is met. Please investigate why the coverage job is being skipped even though unit_tests is now passing, and ensure it runs and reports ≥97%.

🔴 BLOCKER 3 — Missing Behave scenario for BadParameter error path

The implementation correctly raises typer.BadParameter when neither the NAME argument nor the config name field is provided. However, there is no Behave scenario testing this error path. The only scenario tagged @tdd_issue_4186 tests the happy path (config name derived successfully). Per CONTRIBUTING.md, all new behavior — including error/failure paths — must be covered by Behave BDD scenarios.

Please add a scenario such as:

Scenario: actor add without NAME and without config name field raises BadParameter
  Given an actor CLI runner
  And I have an actor JSON config file without a name field
  When I run actor add with config but no NAME positional argument
  Then the actor command should fail with a BadParameter error about missing actor name

🔴 BLOCKER 4 — Missing ISSUES CLOSED: footer on all commits

None of the five commits in this PR have the required ISSUES CLOSED: #N footer format:

Commit Footer present?
4c62ca85 Uses Fixes issue #4186 in body — wrong format
fca8f5cc No issue reference at all
c7f2a807 No issue reference at all
b3e57509 No issue reference at all
2d68c7a2 No issue reference at all

Per CONTRIBUTING.md, every commit footer must include:

ISSUES CLOSED: #4186

(or Refs: #N for commits that do not close the issue). Please interactive-rebase to add the correct footers to all five commits.

🔴 BLOCKER 5 — No milestone assigned

Issue #4186 is assigned to milestone v3.5.0. Per CONTRIBUTING.md, the PR must be assigned to the same milestone as its linked issue. This PR currently has no milestone. Please assign v3.5.0.

🔴 BLOCKER 6 — Missing Type/Bug label

Per CONTRIBUTING.md, every PR must carry exactly one Type/ label. Issue #4186 is Type/Bug, so this PR requires Type/Bug. The only label currently on the PR is Needs Feedback (a process label). Please add Type/Bug.

🔴 BLOCKER 7 — No Forgejo dependency link (PR must block issue)

Per CONTRIBUTING.md (critical rule): the PR must be set to block issue #4186, creating the dependency issue #4186 depends on PR #10974. This prevents the issue from being closed until the PR is merged. Currently no dependency is set on this PR. Please open the PR sidebar → "This pull request blocks" → add issue #4186.

🔴 BLOCKER 8 — No CHANGELOG entry

Per CONTRIBUTING.md, every PR must include a CHANGELOG update with at least one entry describing the user-facing change. The CHANGELOG.md file is unchanged in this PR. Please add an entry under the appropriate unreleased section describing this fix.

Non-Blocking Observations

Scenarios tagged @tdd_issue_4230 @tdd_expected_fail belong to a different issue

The feature file actor_add_name_positional.feature contains two scenarios tagged @tdd_issue @tdd_issue_4230 @tdd_expected_fail. These belong to issue #4230 (not #4186) and are still expected to fail. Per the TDD workflow, @tdd_expected_fail scenarios should live on a tdd/ branch. They are present here presumably because this branch was branched from a state that included them. This is acceptable if issue #4230 is tracked separately — no action required on this PR, but verify that issue #4230 has its own tdd/ branch.

Branch name missing milestone prefix

The branch is bugfix/actor-add-name-arg. Per CONTRIBUTING.md, it should be bugfix/m5-actor-add-name-arg (milestone v3.5.0 = m5). The branch cannot be renamed post-PR-creation without complications. Note this for future issues. No action required on this PR.

Implementation correctness

The core implementation is correct and aligns with the specification. The str | None = None signature, config derivation logic, and BadParameter raise are all well-implemented. Good work.

Summary

Good progress overall — the implementation is correct and several major CI blockers have been resolved. To get this PR to approval, address the following:

  1. Fix lint regression (nox -s lint)
  2. Fix coverage skip (ensure nox -s coverage_report runs and reports ≥97%)
  3. Add Behave scenario for the BadParameter error path (no name in arg AND no name in config)
  4. Fix commit footers — add ISSUES CLOSED: #4186 to all commits
  5. Assign milestone v3.5.0 to the PR
  6. Add Type/Bug label to the PR
  7. Set PR to block issue #4186 via Forgejo dependency sidebar
  8. Add CHANGELOG entry

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

## Re-Review: fix(cli): make actor NAME argument optional, derive from YAML config ### Prior Feedback Addressed? Good progress since the last review. The implementation is now present and correct, and the unit_tests / integration_tests CI gates are now passing. Several blockers from the previous review have been resolved. However, a number of required items remain unaddressed, and a **new CI regression** has appeared. This PR cannot be approved until all blockers below are fixed. --- ### ✅ Items Resolved from Previous Review - **Implementation now present** — `actor.py` correctly makes `NAME` optional (`str | None = None`), derives from `config_blob.get("name")`, and raises `BadParameter` if neither is provided. - **CI unit_tests** — now **PASSING** ✅ - **CI integration_tests** — now **PASSING** ✅ - **CI typecheck** — **PASSING** ✅ - **CI security** — **PASSING** ✅ - **Specification update** — the spec at line 279 and lines 5048–5050 already shows `agents actor add --config|-c <FILE> [--update]` without a required NAME argument; the spec was already aligned with the desired behavior. No spec update is needed. (This blocker from the previous review was overstated — it is resolved.) - **AmbiguousStep collisions** — resolved across multiple step files. The `environment.py` fixes replacing `tempfile.mktemp()` with `tempfile.mkstemp()` and adding `fcntl` locking are correct and well-implemented. - **`@tdd_expected_fail` removal for issue #2609** — correct; those scenarios are already passing in master. --- ## 🔴 BLOCKER 1 — CI lint is NOW FAILING (Regression) CI lint was **passing** in the previous review cycle but is **now failing** on the current head `4c62ca8579620fd491fba67e5d97d775e2bdd4bd`. This is a regression introduced by the new commits. Per company policy, `CI / lint` is a required merge gate. Please run `nox -s lint` locally, fix all lint/format errors, and push a corrected commit. --- ## 🔴 BLOCKER 2 — CI coverage is SKIPPED (Required Gate) The `CI / coverage` job shows status `Has been skipped` on the current head. Coverage is a hard merge gate (≥97% required). When coverage is skipped, there is no guarantee the threshold is met. Please investigate why the coverage job is being skipped even though unit_tests is now passing, and ensure it runs and reports ≥97%. --- ## 🔴 BLOCKER 3 — Missing Behave scenario for BadParameter error path The implementation correctly raises `typer.BadParameter` when neither the NAME argument nor the config `name` field is provided. However, there is **no Behave scenario testing this error path**. The only scenario tagged `@tdd_issue_4186` tests the happy path (config name derived successfully). Per CONTRIBUTING.md, all new behavior — including error/failure paths — must be covered by Behave BDD scenarios. Please add a scenario such as: ```gherkin Scenario: actor add without NAME and without config name field raises BadParameter Given an actor CLI runner And I have an actor JSON config file without a name field When I run actor add with config but no NAME positional argument Then the actor command should fail with a BadParameter error about missing actor name ``` --- ## 🔴 BLOCKER 4 — Missing `ISSUES CLOSED:` footer on all commits None of the five commits in this PR have the required `ISSUES CLOSED: #N` footer format: | Commit | Footer present? | |--------|----------------| | `4c62ca85` | Uses `Fixes issue #4186` in body — wrong format | | `fca8f5cc` | No issue reference at all | | `c7f2a807` | No issue reference at all | | `b3e57509` | No issue reference at all | | `2d68c7a2` | No issue reference at all | Per CONTRIBUTING.md, every commit footer **must** include: ``` ISSUES CLOSED: #4186 ``` (or `Refs: #N` for commits that do not close the issue). Please interactive-rebase to add the correct footers to all five commits. --- ## 🔴 BLOCKER 5 — No milestone assigned Issue #4186 is assigned to milestone `v3.5.0`. Per CONTRIBUTING.md, the PR must be assigned to the same milestone as its linked issue. This PR currently has no milestone. Please assign `v3.5.0`. --- ## 🔴 BLOCKER 6 — Missing `Type/Bug` label Per CONTRIBUTING.md, every PR must carry exactly one `Type/` label. Issue #4186 is `Type/Bug`, so this PR requires `Type/Bug`. The only label currently on the PR is `Needs Feedback` (a process label). Please add `Type/Bug`. --- ## 🔴 BLOCKER 7 — No Forgejo dependency link (PR must block issue) Per CONTRIBUTING.md (critical rule): the PR must be set to **block** issue #4186, creating the dependency `issue #4186 depends on PR #10974`. This prevents the issue from being closed until the PR is merged. Currently no dependency is set on this PR. Please open the PR sidebar → "This pull request blocks" → add issue #4186. --- ## 🔴 BLOCKER 8 — No CHANGELOG entry Per CONTRIBUTING.md, every PR must include a CHANGELOG update with at least one entry describing the user-facing change. The `CHANGELOG.md` file is unchanged in this PR. Please add an entry under the appropriate unreleased section describing this fix. --- ## Non-Blocking Observations ### Scenarios tagged `@tdd_issue_4230 @tdd_expected_fail` belong to a different issue The feature file `actor_add_name_positional.feature` contains two scenarios tagged `@tdd_issue @tdd_issue_4230 @tdd_expected_fail`. These belong to issue #4230 (not #4186) and are still expected to fail. Per the TDD workflow, `@tdd_expected_fail` scenarios should live on a `tdd/` branch. They are present here presumably because this branch was branched from a state that included them. This is acceptable if issue #4230 is tracked separately — no action required on this PR, but verify that issue #4230 has its own `tdd/` branch. ### Branch name missing milestone prefix The branch is `bugfix/actor-add-name-arg`. Per CONTRIBUTING.md, it should be `bugfix/m5-actor-add-name-arg` (milestone v3.5.0 = m5). The branch cannot be renamed post-PR-creation without complications. Note this for future issues. No action required on this PR. ### Implementation correctness The core implementation is correct and aligns with the specification. The `str | None = None` signature, config derivation logic, and `BadParameter` raise are all well-implemented. Good work. --- ## Summary Good progress overall — the implementation is correct and several major CI blockers have been resolved. To get this PR to approval, address the following: 1. **Fix lint regression** (`nox -s lint`) 2. **Fix coverage skip** (ensure `nox -s coverage_report` runs and reports ≥97%) 3. **Add Behave scenario for the BadParameter error path** (no name in arg AND no name in config) 4. **Fix commit footers** — add `ISSUES CLOSED: #4186` to all commits 5. **Assign milestone `v3.5.0`** to the PR 6. **Add `Type/Bug` label** to the PR 7. **Set PR to block issue #4186** via Forgejo dependency sidebar 8. **Add CHANGELOG entry** --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
src/cleveragents/cli/commands/actor.py
HAL9001 commented 2026-05-06 20:43:55 +00:00
Owner
Copy Link
Copy Source

BLOCKING — Missing ISSUES CLOSED: footer.

The commit message for 4c62ca85 uses Fixes issue #4186 in the body. Per CONTRIBUTING.md, the required format is a dedicated footer line:

ISSUES CLOSED: #4186

The footer line must appear as the last section of the commit message, separated by a blank line from the body. Please interactive-rebase to correct this commit (and all other commits in this PR that lack ISSUES CLOSED: #N footers).

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

**BLOCKING** — Missing `ISSUES CLOSED:` footer. The commit message for `4c62ca85` uses `Fixes issue #4186` in the body. Per CONTRIBUTING.md, the required format is a dedicated footer line: ``` ISSUES CLOSED: #4186 ``` The footer line must appear as the last section of the commit message, separated by a blank line from the body. Please interactive-rebase to correct this commit (and all other commits in this PR that lack `ISSUES CLOSED: #N` footers). --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
src/cleveragents/cli/commands/actor.py
HAL9001 commented 2026-05-06 20:43:55 +00:00
Owner
Copy Link
Copy Source

BLOCKING — Missing Behave scenario for the BadParameter error path.

The implementation at line 604 raises BadParameter when neither the NAME argument nor the config name field is present. This error path has no Behave test coverage. The only scenario tagged @tdd_issue_4186 tests the happy path (config name derived successfully). Per CONTRIBUTING.md, all error/failure paths must be covered by BDD scenarios.

Please add a scenario to features/actor_add_name_positional.feature such as:

Scenario: actor add without NAME and without config name field raises BadParameter
  Given an actor CLI runner
  And I have an actor JSON config file without a name field
  When I run actor add with config but no NAME positional argument
  Then the actor command should fail with a BadParameter error about missing actor name

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

**BLOCKING** — Missing Behave scenario for the `BadParameter` error path. The implementation at line 604 raises `BadParameter` when neither the NAME argument nor the config `name` field is present. This error path has **no Behave test coverage**. The only scenario tagged `@tdd_issue_4186` tests the happy path (config name derived successfully). Per CONTRIBUTING.md, all error/failure paths must be covered by BDD scenarios. Please add a scenario to `features/actor_add_name_positional.feature` such as: ```gherkin Scenario: actor add without NAME and without config name field raises BadParameter Given an actor CLI runner And I have an actor JSON config file without a name field When I run actor add with config but no NAME positional argument Then the actor command should fail with a BadParameter error about missing actor name ``` --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
HAL9001 commented 2026-05-06 20:44:13 +00:00
Owner
Copy Link
Copy Source

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

--- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
CoreRasurae force-pushed bugfix/actor-add-name-arg from 4c62ca8579 to b79f7872aa 2026-05-06 20:52:56 +00:00 Compare
CoreRasurae added
Type
Bug
 and removed Needs Feedback labels 2026-05-06 21:18:53 +00:00
HAL9000 was assigned by CoreRasurae 2026-05-06 21:19:02 +00:00
HAL9001 requested changes 2026-05-06 22:20:46 +00:00
Dismissed
HAL9001 left a comment
Owner
Copy Link
Copy Source

Re-Review: fix(cli): make actor NAME argument optional, derive from YAML config

Prior Feedback Status (Review #7767)

Good progress from the previous round. Several blockers have been resolved. However, a number of required items remain unaddressed and this PR cannot be approved until they are fixed.

✅ Items Resolved from Previous Review

  • CI lint — now PASSING ✅ (regression from previous review is fixed)
  • Type/Bug label — now applied ✅
  • Forgejo dependency link — PR now blocks issue #4186 ✅
  • CHANGELOG entry — now present ✅

❌ Items Still Outstanding (Blocking)

🔴 BLOCKER 1 — CI unit_tests Still Failing

CI / unit_tests is FAILING after 5m54s on head b79f7872aa5af1566ce137ba75f93dfcd6520f38. As a direct consequence, CI / coverage is SKIPPED and cannot verify the ≥97% threshold. CI / status-check is also FAILING as the aggregate gate.

Current CI state:

Check Status
CI / lint ✅ Passing
CI / typecheck ✅ Passing
CI / security ✅ Passing
CI / unit_tests ❌ FAILING
CI / integration_tests ✅ Passing
CI / e2e_tests ✅ Passing
CI / coverage ⏭️ Skipped (unit_tests failed)
CI / benchmark-regression ❌ FAILING
CI / status-check ❌ FAILING

Per company policy, unit_tests and coverage are required merge gates. Please run nox -s unit_tests locally and fix all failures. Once unit_tests pass, coverage will run and the ≥97% threshold must be verified with nox -s coverage_report.

🔴 BLOCKER 2 — Missing Behave Scenario for BadParameter Error Path

The inline comment from review #7767 requested a Behave scenario testing the BadParameter error path (when neither NAME argument nor config name field is provided). This scenario is still absent from features/actor_add_name_positional.feature.

The current feature file contains only:

  1. Two @tdd_issue_4230 @tdd_expected_fail scenarios (happy path — positional NAME)
  2. One @tdd_issue_4186 scenario (happy path — config-derived name)

The error path at actor.py lines 601–605 (raises BadParameter when config_blob.get("name") is falsy) has no test coverage. Per CONTRIBUTING.md, all new behavior — including error/failure paths — must be covered by Behave BDD scenarios.

Please add a scenario such as:

@tdd_issue @tdd_issue_4186
Scenario: actor add without NAME and without config name field raises BadParameter
  Given an actor CLI runner
  And I have an actor JSON config file without a name field
  When I run actor add with config but no NAME positional argument
  Then the actor command should fail with a BadParameter error about missing actor name

And implement the corresponding step definition for the Then assertion.

🔴 BLOCKER 3 — Missing ISSUES CLOSED: Footer on Commits

The implementation commit (b79f7872) uses Fixes issue #4186 embedded in the commit body. Per CONTRIBUTING.md, the required format is a dedicated footer line (after a blank line separating it from the body):

ISSUES CLOSED: #4186

The current format does not meet the project standard. The other four commits (b587bdd2, f7858800, ad8e023f, a92b8cd7) have no issue reference at all — each should include either ISSUES CLOSED: #4186 or Refs: #4186 in their footers.

Please interactive-rebase to add the correct footer format to all five commits.

🔴 BLOCKER 4 — No Milestone Assigned

Issue #4186 is in milestone v3.5.0. Per CONTRIBUTING.md, the PR must be assigned to the same milestone as its linked issue. This PR still has no milestone assigned. Please assign v3.5.0 to the PR.

Non-Blocking Observations

Implementation correctness

The core implementation in actor.py is correct and aligns with the specification:

  • name: str | None = None correctly makes the argument optional
  • Derivation from config_blob.get("name") is correct
  • BadParameter raised when neither source provides a name is correct
  • Updated docstring and examples are clear and accurate

environment.py improvements

The tempfile.mktemp() → tempfile.mkstemp() replacements and fcntl.flock locking in _ensure_template_db() are correct, well-implemented improvements that eliminate TOCTOU race conditions in parallel test runs. Good work.

AmbiguousStep collision fixes

The step renaming across multiple step files is correct and necessary. The changes are minimal and well-scoped.

Summary

Four blockers remain:

  1. Fix failing CI unit_tests — run nox -s unit_tests locally and fix all failures; then verify nox -s coverage_report ≥97%
  2. Add BadParameter error path Behave scenario — add a scenario and step definition testing actor add when config has no name field
  3. Fix commit message footers — use ISSUES CLOSED: #4186 as a dedicated footer line on all commits
  4. Assign milestone v3.5.0 — via the PR sidebar

Once these are addressed, the PR is close to approval. The implementation itself is already correct.

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

## Re-Review: fix(cli): make actor NAME argument optional, derive from YAML config ### Prior Feedback Status (Review #7767) Good progress from the previous round. Several blockers have been resolved. However, a number of required items remain unaddressed and this PR cannot be approved until they are fixed. --- ### ✅ Items Resolved from Previous Review - **CI lint** — now **PASSING** ✅ (regression from previous review is fixed) - **`Type/Bug` label** — now applied ✅ - **Forgejo dependency link** — PR now blocks issue #4186 ✅ - **CHANGELOG entry** — now present ✅ --- ### ❌ Items Still Outstanding (Blocking) --- ## 🔴 BLOCKER 1 — CI unit_tests Still Failing `CI / unit_tests` is **FAILING** after 5m54s on head `b79f7872aa5af1566ce137ba75f93dfcd6520f38`. As a direct consequence, `CI / coverage` is **SKIPPED** and cannot verify the ≥97% threshold. `CI / status-check` is also **FAILING** as the aggregate gate. **Current CI state:** | Check | Status | |---|---| | `CI / lint` | ✅ Passing | | `CI / typecheck` | ✅ Passing | | `CI / security` | ✅ Passing | | `CI / unit_tests` | ❌ **FAILING** | | `CI / integration_tests` | ✅ Passing | | `CI / e2e_tests` | ✅ Passing | | `CI / coverage` | ⏭️ Skipped (unit_tests failed) | | `CI / benchmark-regression` | ❌ **FAILING** | | `CI / status-check` | ❌ **FAILING** | Per company policy, `unit_tests` and `coverage` are required merge gates. Please run `nox -s unit_tests` locally and fix all failures. Once unit_tests pass, coverage will run and the ≥97% threshold must be verified with `nox -s coverage_report`. --- ## 🔴 BLOCKER 2 — Missing Behave Scenario for BadParameter Error Path The inline comment from review #7767 requested a Behave scenario testing the `BadParameter` error path (when neither NAME argument nor config `name` field is provided). This scenario is **still absent** from `features/actor_add_name_positional.feature`. The current feature file contains only: 1. Two `@tdd_issue_4230 @tdd_expected_fail` scenarios (happy path — positional NAME) 2. One `@tdd_issue_4186` scenario (happy path — config-derived name) The error path at `actor.py` lines 601–605 (raises `BadParameter` when `config_blob.get("name")` is falsy) has **no test coverage**. Per CONTRIBUTING.md, all new behavior — including error/failure paths — must be covered by Behave BDD scenarios. Please add a scenario such as: ```gherkin @tdd_issue @tdd_issue_4186 Scenario: actor add without NAME and without config name field raises BadParameter Given an actor CLI runner And I have an actor JSON config file without a name field When I run actor add with config but no NAME positional argument Then the actor command should fail with a BadParameter error about missing actor name ``` And implement the corresponding step definition for the `Then` assertion. --- ## 🔴 BLOCKER 3 — Missing `ISSUES CLOSED:` Footer on Commits The implementation commit (`b79f7872`) uses `Fixes issue #4186` embedded in the commit body. Per CONTRIBUTING.md, the required format is a dedicated **footer line** (after a blank line separating it from the body): ``` ISSUES CLOSED: #4186 ``` The current format does not meet the project standard. The other four commits (`b587bdd2`, `f7858800`, `ad8e023f`, `a92b8cd7`) have no issue reference at all — each should include either `ISSUES CLOSED: #4186` or `Refs: #4186` in their footers. Please interactive-rebase to add the correct footer format to all five commits. --- ## 🔴 BLOCKER 4 — No Milestone Assigned Issue #4186 is in milestone `v3.5.0`. Per CONTRIBUTING.md, the PR must be assigned to the same milestone as its linked issue. This PR still has **no milestone assigned**. Please assign `v3.5.0` to the PR. --- ## Non-Blocking Observations ### Implementation correctness The core implementation in `actor.py` is correct and aligns with the specification: - `name: str | None = None` correctly makes the argument optional - Derivation from `config_blob.get("name")` is correct - `BadParameter` raised when neither source provides a name is correct - Updated docstring and examples are clear and accurate ### environment.py improvements The `tempfile.mktemp()` → `tempfile.mkstemp()` replacements and `fcntl.flock` locking in `_ensure_template_db()` are correct, well-implemented improvements that eliminate TOCTOU race conditions in parallel test runs. Good work. ### AmbiguousStep collision fixes The step renaming across multiple step files is correct and necessary. The changes are minimal and well-scoped. --- ## Summary Four blockers remain: 1. **Fix failing CI unit_tests** — run `nox -s unit_tests` locally and fix all failures; then verify `nox -s coverage_report` ≥97% 2. **Add BadParameter error path Behave scenario** — add a scenario and step definition testing `actor add` when config has no `name` field 3. **Fix commit message footers** — use `ISSUES CLOSED: #4186` as a dedicated footer line on all commits 4. **Assign milestone `v3.5.0`** — via the PR sidebar Once these are addressed, the PR is close to approval. The implementation itself is already correct. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
features/actor_add_name_positional.feature
@@ -20,2 +20,3 @@
Scenario: actor add without NAME positional argument fails
@tdd_issue @tdd_issue_4186
Scenario: actor add without NAME positional argument uses config name
Given an actor CLI runner
HAL9001 commented 2026-05-06 22:20:46 +00:00
Owner
Copy Link
Copy Source

BLOCKING — Missing Behave scenario for the BadParameter error path.

The implementation in actor.py raises typer.BadParameter when neither the positional NAME argument nor the config name field is provided. This failure path has no test coverage in this feature file. Only the happy path (config-derived name) is tested under @tdd_issue_4186.

Per CONTRIBUTING.md, all error/failure paths must be covered by Behave BDD scenarios. Please add a scenario such as:

@tdd_issue @tdd_issue_4186
Scenario: actor add without NAME and without config name field raises BadParameter
  Given an actor CLI runner
  And I have an actor JSON config file without a name field
  When I run actor add with config but no NAME positional argument
  Then the actor command should fail with a BadParameter error about missing actor name

Note: The Given I have an actor JSON config file without a name field step already exists in the step file (it was present before this PR). The Then step the actor command should fail with a BadParameter error about missing actor name also already exists from the previously-removed scenario. So only the scenario declaration needs to be added — no new step definitions required.

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

**BLOCKING** — Missing Behave scenario for the `BadParameter` error path. The implementation in `actor.py` raises `typer.BadParameter` when neither the positional `NAME` argument nor the config `name` field is provided. This failure path has **no test coverage** in this feature file. Only the happy path (config-derived name) is tested under `@tdd_issue_4186`. Per CONTRIBUTING.md, all error/failure paths must be covered by Behave BDD scenarios. Please add a scenario such as: ```gherkin @tdd_issue @tdd_issue_4186 Scenario: actor add without NAME and without config name field raises BadParameter Given an actor CLI runner And I have an actor JSON config file without a name field When I run actor add with config but no NAME positional argument Then the actor command should fail with a BadParameter error about missing actor name ``` Note: The `Given I have an actor JSON config file without a name field` step already exists in the step file (it was present before this PR). The `Then` step `the actor command should fail with a BadParameter error about missing actor name` also already exists from the previously-removed scenario. So only the scenario declaration needs to be added — no new step definitions required. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
src/cleveragents/cli/commands/actor.py
HAL9001 commented 2026-05-06 22:20:46 +00:00
Owner
Copy Link
Copy Source

BLOCKING — Missing ISSUES CLOSED: footer on implementation commit.

This commit (b79f7872) contains Fixes issue #4186 inline in the body text. Per CONTRIBUTING.md, the required format is a dedicated footer line (separated from the body by a blank line):

ISSUES CLOSED: #4186

The other four commits in this PR (b587bdd2, f7858800, ad8e023f, a92b8cd7) have no issue reference at all. Each commit must reference its issue in the footer. Please interactive-rebase to correct all five commit footers.

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

**BLOCKING** — Missing `ISSUES CLOSED:` footer on implementation commit. This commit (`b79f7872`) contains `Fixes issue #4186` inline in the body text. Per CONTRIBUTING.md, the required format is a **dedicated footer line** (separated from the body by a blank line): ``` ISSUES CLOSED: #4186 ``` The other four commits in this PR (`b587bdd2`, `f7858800`, `ad8e023f`, `a92b8cd7`) have no issue reference at all. Each commit must reference its issue in the footer. Please interactive-rebase to correct all five commit footers. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
HAL9001 commented 2026-05-06 22:20:53 +00:00
Owner
Copy Link
Copy Source

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

--- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
CoreRasurae force-pushed bugfix/actor-add-name-arg from b79f7872aa to c4298589a9 2026-05-06 22:45:56 +00:00 Compare
CoreRasurae added this to the v3.5.0 milestone 2026-05-06 22:47:16 +00:00
CoreRasurae added the Needs Feedback label 2026-05-06 23:28:09 +00:00
HAL9000 requested changes 2026-05-06 23:28:12 +00:00
HAL9000 left a comment
Owner
Copy Link
Copy Source

Re-Review: fix(cli): make actor NAME argument optional, derive from YAML config

Prior Feedback Status (Review #7810)

Good progress from the previous round. All four blockers from review #7810 have been partially addressed. Two have been fully resolved; two remain outstanding and block approval.

✅ Items Resolved from Previous Review

  • CI unit_tests — now PASSING ✅ (Successful in 4m59s)
  • CI integration_tests — now PASSING ✅
  • CI e2e_tests — now PASSING ✅
  • CI coverage — now PASSING ✅ (Successful in 10m36s)
  • CI lint — now PASSING ✅
  • CI typecheck — now PASSING ✅
  • CI security — now PASSING ✅
  • CI status-check — now PASSING ✅ (aggregate gate passed, all required checks green)
  • Milestone v3.5.0 — now assigned ✅
  • Type/Bug label — applied ✅ (already resolved in prior review)
  • Forgejo dependency link — PR blocks issue #4186 ✅ (already resolved)
  • CHANGELOG entry — present ✅ (already resolved)

Note on benchmark-regression: CI / benchmark-regression is still failing (Failing after 1m25s), but this job is not part of the required merge gate (status-check does not depend on it). This appears to be a pre-existing infrastructure issue unrelated to this PR. No action required.

❌ Items Still Outstanding (Blocking)

🔴 BLOCKER 1 — Missing Behave Scenario for BadParameter Error Path

The inline comment from review #7810 requested a Behave scenario covering the BadParameter error path when neither NAME argument nor config name field is provided. This scenario is still absent from features/actor_add_name_positional.feature.

The current feature file has three scenarios:

  1. @tdd_issue_4230 @tdd_expected_fail — actor add accepts NAME as positional (expected fail, issue #4230 not fixed yet)
  2. @tdd_issue_4230 @tdd_expected_fail — NAME positional argument takes precedence (expected fail, issue #4230 not fixed yet)
  3. @tdd_issue_4186 — actor add without NAME uses config name (happy path)

The error path at actor.py lines 600–607 (raises BadParameter when config_blob.get("name") is falsy) has no test coverage. Per CONTRIBUTING.md, all new behavior — including error/failure paths — must be covered by Behave BDD scenarios.

Please add the following scenario to features/actor_add_name_positional.feature:

  @tdd_issue @tdd_issue_4186
  Scenario: actor add without NAME and without config name field raises BadParameter
    Given an actor CLI runner
    And I have an actor JSON config file without a name field
    When I run actor add with config but no NAME positional argument
    Then the actor command should fail with a BadParameter error about missing actor name

The step definitions for all four steps above already exist in features/steps/actor_add_name_positional_steps.py from the pre-existing implementation — no new step implementations are required.

🔴 BLOCKER 2 — Missing ISSUES CLOSED: Footer on All Commits

None of the six commits in this PR have the required ISSUES CLOSED: #N footer format:

Commit Current state
c4298589 Uses Fixes issue #4186 embedded in body — wrong format and wrong location
90757a08 No issue reference at all
c085a45e No issue reference at all
dd149940 No issue reference at all
4c40d021 No issue reference at all
1ba1f168 No issue reference at all

Per CONTRIBUTING.md, every commit footer must include a dedicated footer line (separated from the body by a blank line):

ISSUES CLOSED: #4186

For commits that do not directly close the issue (the supporting test/fix commits), use:

Refs: #4186

Please interactive-rebase all six commits to add the correct footer format.

Full Review Checklist Assessment

Category Status Notes
CORRECTNESS ✅ Pass NAME is optional, derives from config, raises BadParameter when neither provided. All acceptance criteria in issue #4186 met except error path test.
SPECIFICATION ALIGNMENT ✅ Pass CLI now matches spec.
TEST QUALITY ❌ Fail Happy path covered; error path (BadParameter when no name in config) has no Behave scenario (BLOCKER 1).
TYPE SAFETY ✅ Pass str | None annotation correct; no # type: ignore.
READABILITY ✅ Pass Clear naming, good inline comment explaining derivation logic.
PERFORMANCE ✅ Pass No performance concerns.
SECURITY ✅ Pass No security concerns.
CODE STYLE ✅ Pass SOLID, well-structured, ruff-compliant.
DOCUMENTATION ✅ Pass Docstring updated, CHANGELOG updated.
COMMIT AND PR QUALITY ❌ Fail Missing ISSUES CLOSED: #4186 footer on all 6 commits (BLOCKER 2). All other PR metadata now correct.

Summary

This PR has made substantial progress — the required CI gates are all green, all PR metadata is correct, and the implementation is well-executed. Only two blockers remain:

  1. Add BadParameter error path Behave scenario — one scenario declaration in features/actor_add_name_positional.feature (no new step definitions needed)
  2. Fix commit footers — interactive-rebase all 6 commits to add ISSUES CLOSED: #4186 (or Refs: #4186 for supporting commits)

Once these two items are addressed, this PR is ready for approval.

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

## Re-Review: fix(cli): make actor NAME argument optional, derive from YAML config ### Prior Feedback Status (Review #7810) Good progress from the previous round. All four blockers from review #7810 have been partially addressed. Two have been fully resolved; two remain outstanding and block approval. --- ### ✅ Items Resolved from Previous Review - **CI unit_tests** — now **PASSING** ✅ (Successful in 4m59s) - **CI integration_tests** — now **PASSING** ✅ - **CI e2e_tests** — now **PASSING** ✅ - **CI coverage** — now **PASSING** ✅ (Successful in 10m36s) - **CI lint** — now **PASSING** ✅ - **CI typecheck** — now **PASSING** ✅ - **CI security** — now **PASSING** ✅ - **CI status-check** — now **PASSING** ✅ (aggregate gate passed, all required checks green) - **Milestone v3.5.0** — now assigned ✅ - **`Type/Bug` label** — applied ✅ (already resolved in prior review) - **Forgejo dependency link** — PR blocks issue #4186 ✅ (already resolved) - **CHANGELOG entry** — present ✅ (already resolved) **Note on benchmark-regression:** `CI / benchmark-regression` is still failing (Failing after 1m25s), but this job is **not** part of the required merge gate (`status-check` does not depend on it). This appears to be a pre-existing infrastructure issue unrelated to this PR. No action required. --- ### ❌ Items Still Outstanding (Blocking) --- ## 🔴 BLOCKER 1 — Missing Behave Scenario for BadParameter Error Path The inline comment from review #7810 requested a Behave scenario covering the `BadParameter` error path when neither NAME argument nor config `name` field is provided. This scenario is **still absent** from `features/actor_add_name_positional.feature`. The current feature file has three scenarios: 1. `@tdd_issue_4230 @tdd_expected_fail` — actor add accepts NAME as positional (expected fail, issue #4230 not fixed yet) 2. `@tdd_issue_4230 @tdd_expected_fail` — NAME positional argument takes precedence (expected fail, issue #4230 not fixed yet) 3. `@tdd_issue_4186` — actor add without NAME uses config name (happy path) The error path at `actor.py` lines 600–607 (raises `BadParameter` when `config_blob.get("name")` is falsy) has **no test coverage**. Per CONTRIBUTING.md, all new behavior — including error/failure paths — must be covered by Behave BDD scenarios. Please add the following scenario to `features/actor_add_name_positional.feature`: ```gherkin @tdd_issue @tdd_issue_4186 Scenario: actor add without NAME and without config name field raises BadParameter Given an actor CLI runner And I have an actor JSON config file without a name field When I run actor add with config but no NAME positional argument Then the actor command should fail with a BadParameter error about missing actor name ``` The step definitions for all four steps above already exist in `features/steps/actor_add_name_positional_steps.py` from the pre-existing implementation — no new step implementations are required. --- ## 🔴 BLOCKER 2 — Missing `ISSUES CLOSED:` Footer on All Commits None of the six commits in this PR have the required `ISSUES CLOSED: #N` footer format: | Commit | Current state | |--------|---------------| | `c4298589` | Uses `Fixes issue #4186` embedded in body — **wrong format and wrong location** | | `90757a08` | No issue reference at all | | `c085a45e` | No issue reference at all | | `dd149940` | No issue reference at all | | `4c40d021` | No issue reference at all | | `1ba1f168` | No issue reference at all | Per CONTRIBUTING.md, every commit footer **must** include a dedicated footer line (separated from the body by a blank line): ``` ISSUES CLOSED: #4186 ``` For commits that do not directly close the issue (the supporting test/fix commits), use: ``` Refs: #4186 ``` Please interactive-rebase all six commits to add the correct footer format. --- ## Full Review Checklist Assessment | Category | Status | Notes | |---|---|---| | **CORRECTNESS** | ✅ Pass | `NAME` is optional, derives from config, raises `BadParameter` when neither provided. All acceptance criteria in issue #4186 met except error path test. | | **SPECIFICATION ALIGNMENT** | ✅ Pass | CLI now matches spec. | | **TEST QUALITY** | ❌ **Fail** | Happy path covered; error path (`BadParameter` when no name in config) has no Behave scenario (BLOCKER 1). | | **TYPE SAFETY** | ✅ Pass | `str \| None` annotation correct; no `# type: ignore`. | | **READABILITY** | ✅ Pass | Clear naming, good inline comment explaining derivation logic. | | **PERFORMANCE** | ✅ Pass | No performance concerns. | | **SECURITY** | ✅ Pass | No security concerns. | | **CODE STYLE** | ✅ Pass | SOLID, well-structured, ruff-compliant. | | **DOCUMENTATION** | ✅ Pass | Docstring updated, CHANGELOG updated. | | **COMMIT AND PR QUALITY** | ❌ **Fail** | Missing `ISSUES CLOSED: #4186` footer on all 6 commits (BLOCKER 2). All other PR metadata now correct. | --- ## Summary This PR has made substantial progress — the required CI gates are all green, all PR metadata is correct, and the implementation is well-executed. Only two blockers remain: 1. **Add BadParameter error path Behave scenario** — one scenario declaration in `features/actor_add_name_positional.feature` (no new step definitions needed) 2. **Fix commit footers** — interactive-rebase all 6 commits to add `ISSUES CLOSED: #4186` (or `Refs: #4186` for supporting commits) Once these two items are addressed, this PR is ready for approval. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
features/actor_add_name_positional.feature Outdated
HAL9000 commented 2026-05-06 23:28:12 +00:00
Owner
Copy Link
Copy Source

BLOCKING — Missing Behave scenario for the BadParameter error path.

The implementation at actor.py lines 600–607 raises typer.BadParameter when neither the positional NAME argument nor the config name field is provided. This failure path has no test coverage in this feature file.

Only the happy path (config-derived name) is tested under @tdd_issue_4186. Per CONTRIBUTING.md, all error/failure paths must be covered by Behave BDD scenarios.

All four required step definitions already exist in actor_add_name_positional_steps.py. Please add the following scenario to this feature file:

  @tdd_issue @tdd_issue_4186
  Scenario: actor add without NAME and without config name field raises BadParameter
    Given an actor CLI runner
    And I have an actor JSON config file without a name field
    When I run actor add with config but no NAME positional argument
    Then the actor command should fail with a BadParameter error about missing actor name

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

**BLOCKING** — Missing Behave scenario for the `BadParameter` error path. The implementation at `actor.py` lines 600–607 raises `typer.BadParameter` when neither the positional `NAME` argument nor the config `name` field is provided. This failure path has **no test coverage** in this feature file. Only the happy path (config-derived name) is tested under `@tdd_issue_4186`. Per CONTRIBUTING.md, all error/failure paths must be covered by Behave BDD scenarios. All four required step definitions already exist in `actor_add_name_positional_steps.py`. Please add the following scenario to this feature file: ```gherkin @tdd_issue @tdd_issue_4186 Scenario: actor add without NAME and without config name field raises BadParameter Given an actor CLI runner And I have an actor JSON config file without a name field When I run actor add with config but no NAME positional argument Then the actor command should fail with a BadParameter error about missing actor name ``` --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
src/cleveragents/cli/commands/actor.py
HAL9000 commented 2026-05-06 23:28:12 +00:00
Owner
Copy Link
Copy Source

BLOCKING — Missing ISSUES CLOSED: footer on all commits.

This PR has 6 commits. The implementation commit (c4298589) uses Fixes issue #4186 embedded in the body text — this is the wrong format. Per CONTRIBUTING.md, the correct form is a dedicated footer line (after a blank line separating it from the body):

ISSUES CLOSED: #4186

The other 5 commits (90757a08, c085a45e, dd149940, 4c40d021, 1ba1f168) have no issue reference at all. Each should include either ISSUES CLOSED: #4186 (for the main fix commit) or Refs: #4186 (for supporting commits) as a dedicated footer line.

Please interactive-rebase to fix all 6 commit footers.

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

**BLOCKING** — Missing `ISSUES CLOSED:` footer on all commits. This PR has 6 commits. The implementation commit (`c4298589`) uses `Fixes issue #4186` embedded in the body text — this is the wrong format. Per CONTRIBUTING.md, the correct form is a **dedicated footer line** (after a blank line separating it from the body): ``` ISSUES CLOSED: #4186 ``` The other 5 commits (`90757a08`, `c085a45e`, `dd149940`, `4c40d021`, `1ba1f168`) have no issue reference at all. Each should include either `ISSUES CLOSED: #4186` (for the main fix commit) or `Refs: #4186` (for supporting commits) as a dedicated footer line. Please interactive-rebase to fix all 6 commit footers. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
CoreRasurae commented 2026-05-06 23:38:35 +00:00
Author
Member
Copy Link
Copy Source

Issues were addressed, except for the 5 additional commits that fix previously written tests issues, just for the CI to pass, They do not actually relate to the current ticket. So allow those to be without Refs as i cannot identify their original PRs which are closed anyway.

Issues were addressed, except for the 5 additional commits that fix previously written tests issues, just for the CI to pass, They do not actually relate to the current ticket. So allow those to be without Refs as i cannot identify their original PRs which are closed anyway.
HAL9000 commented 2026-05-06 23:28:18 +00:00
Owner
Copy Link
Copy Source

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

--- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
CoreRasurae force-pushed bugfix/actor-add-name-arg from c4298589a9 to 85643d6e12 2026-05-06 23:35:07 +00:00 Compare
CoreRasurae removed the Needs Feedback label 2026-05-06 23:35:23 +00:00
CoreRasurae commented 2026-05-06 23:39:01 +00:00
Author
Member
Copy Link
Copy Source

@HAL9000 @HAL9001 This PR code can be re-reviewed. if you are not happy with the changes yet, you can modify it @HAL9000 .
Just not that the following E2E Test is unreliable in the sense it that it sometimes fails, other times it passes, but it is a test that is unrelated to the current feature and exists on master.

==============================================================================
E2E.Wf10 Batch :: E2E test for Workflow Example 10: Full-Auto Batch Operati...
==============================================================================
Workflow 10 Full-Auto Batch Formatting :: End-to-end test for full... | FAIL |
Expected at least 2 plans (of 3 healthy) to reach 'applied' state but found 1
------------------------------------------------------------------------------
E2E.Wf10 Batch :: E2E test for Workflow Example 10: Full-Auto Batc... | FAIL |
1 test, 0 passed, 1 failed
==============================================================================
E2E                                                                   | FAIL |
1 test, 0 passed, 1 failed
==============================================================================
@HAL9000 @HAL9001 This PR code can be re-reviewed. if you are not happy with the changes yet, you can modify it @HAL9000 . Just not that the following E2E Test is unreliable in the sense it that it sometimes fails, other times it passes, but it is a test that is unrelated to the current feature and exists on master. ``` ============================================================================== E2E.Wf10 Batch :: E2E test for Workflow Example 10: Full-Auto Batch Operati... ============================================================================== Workflow 10 Full-Auto Batch Formatting :: End-to-end test for full... | FAIL | Expected at least 2 plans (of 3 healthy) to reach 'applied' state but found 1 ------------------------------------------------------------------------------ E2E.Wf10 Batch :: E2E test for Workflow Example 10: Full-Auto Batc... | FAIL | 1 test, 0 passed, 1 failed ============================================================================== E2E | FAIL | 1 test, 0 passed, 1 failed ============================================================================== ```
HAL9001 approved these changes 2026-05-07 00:07:08 +00:00
HAL9001 left a comment
Owner
Copy Link
Copy Source

Re-Review: fix(cli): make actor NAME argument optional, derive from YAML config

Prior Feedback Status (Reviews #7811 / #7810)

All prior blockers have been resolved. This PR is ready to merge.

✅ All Items Resolved

  • BadParameter error path Behave scenario — Added ✅ (Scenario: actor add without NAME and without config name field raises BadParameter now present in features/actor_add_name_positional.feature with corresponding @then step in actor_add_name_positional_steps.py)
  • ISSUES CLOSED: #4186 footer — Fixed ✅ (main implementation commit 85643d6e now has a proper ISSUES CLOSED: #4186 footer line)
  • CI all required gates — All PASSING ✅
    • lint ✅, typecheck ✅, security ✅, quality ✅, unit_tests ✅, integration_tests ✅, e2e_tests ✅, coverage ✅, build ✅, docker ✅, helm ✅, push-validation ✅, status-check ✅
    • benchmark-regression ❌ — still failing, but is not part of the required status-check gate; appears to be a pre-existing infrastructure issue unrelated to this PR
  • Milestone v3.5.0 — assigned ✅
  • Type/Bug label — applied ✅
  • Forgejo dependency (PR blocks issue #4186) — set ✅
  • CHANGELOG entry — present and comprehensive ✅

Full Review Checklist

Category Status Notes
CORRECTNESS ✅ Pass NAME optional, derives from config name field, raises BadParameter when neither provided. All acceptance criteria in issue #4186 met.
SPECIFICATION ALIGNMENT ✅ Pass CLI now matches spec: agents actor add --config|-c <FILE> [<NAME>].
TEST QUALITY ✅ Pass Happy path and error path both covered. Unit tests, integration tests, e2e tests all passing. Coverage passing.
TYPE SAFETY ✅ Pass str | None annotation correct. No # type: ignore.
READABILITY ✅ Pass Clear naming, good inline comments.
PERFORMANCE ✅ Pass No concerns.
SECURITY ✅ Pass No concerns.
CODE STYLE ✅ Pass SOLID, ruff-compliant (lint passes).
DOCUMENTATION ✅ Pass Docstring updated. CHANGELOG comprehensive.
COMMIT AND PR QUALITY ✅ Pass Main commit has ISSUES CLOSED: #4186. Milestone, label, and dependency all correct. Supporting commits lack issue refs — author explained these fix pre-existing bugs from closed PRs with no traceable issue number; accepted.

Non-Blocking Suggestion

In features/steps/actor_add_name_positional_steps.py, the @then step for the BadParameter error path has the same assert "Actor name is required" in context.result.output statement copy-pasted three times identically. Only one assertion is needed. The duplicates add no verification value and reduce readability. Consider cleaning this up in a follow-up commit — no action required to merge.

Summary

The implementation is correct and complete. All required CI gates are green. All PR metadata is in order. This PR is approved and ready to merge.

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

## Re-Review: fix(cli): make actor NAME argument optional, derive from YAML config ### Prior Feedback Status (Reviews #7811 / #7810) All prior blockers have been resolved. This PR is ready to merge. --- ### ✅ All Items Resolved - **BadParameter error path Behave scenario** — Added ✅ (`Scenario: actor add without NAME and without config name field raises BadParameter` now present in `features/actor_add_name_positional.feature` with corresponding `@then` step in `actor_add_name_positional_steps.py`) - **`ISSUES CLOSED: #4186` footer** — Fixed ✅ (main implementation commit `85643d6e` now has a proper `ISSUES CLOSED: #4186` footer line) - **CI all required gates** — All PASSING ✅ - `lint` ✅, `typecheck` ✅, `security` ✅, `quality` ✅, `unit_tests` ✅, `integration_tests` ✅, `e2e_tests` ✅, `coverage` ✅, `build` ✅, `docker` ✅, `helm` ✅, `push-validation` ✅, `status-check` ✅ - `benchmark-regression` ❌ — still failing, but is not part of the required `status-check` gate; appears to be a pre-existing infrastructure issue unrelated to this PR - **Milestone `v3.5.0`** — assigned ✅ - **`Type/Bug` label** — applied ✅ - **Forgejo dependency (PR blocks issue #4186)** — set ✅ - **CHANGELOG entry** — present and comprehensive ✅ --- ## Full Review Checklist | Category | Status | Notes | |---|---|---| | **CORRECTNESS** | ✅ Pass | `NAME` optional, derives from config `name` field, raises `BadParameter` when neither provided. All acceptance criteria in issue #4186 met. | | **SPECIFICATION ALIGNMENT** | ✅ Pass | CLI now matches spec: `agents actor add --config\|-c <FILE> [<NAME>]`. | | **TEST QUALITY** | ✅ Pass | Happy path and error path both covered. Unit tests, integration tests, e2e tests all passing. Coverage passing. | | **TYPE SAFETY** | ✅ Pass | `str \| None` annotation correct. No `# type: ignore`. | | **READABILITY** | ✅ Pass | Clear naming, good inline comments. | | **PERFORMANCE** | ✅ Pass | No concerns. | | **SECURITY** | ✅ Pass | No concerns. | | **CODE STYLE** | ✅ Pass | SOLID, ruff-compliant (lint passes). | | **DOCUMENTATION** | ✅ Pass | Docstring updated. CHANGELOG comprehensive. | | **COMMIT AND PR QUALITY** | ✅ Pass | Main commit has `ISSUES CLOSED: #4186`. Milestone, label, and dependency all correct. Supporting commits lack issue refs — author explained these fix pre-existing bugs from closed PRs with no traceable issue number; accepted. | --- ## Non-Blocking Suggestion In `features/steps/actor_add_name_positional_steps.py`, the `@then` step for the `BadParameter` error path has the same `assert "Actor name is required" in context.result.output` statement copy-pasted **three times identically**. Only one assertion is needed. The duplicates add no verification value and reduce readability. Consider cleaning this up in a follow-up commit — no action required to merge. --- ## Summary The implementation is correct and complete. All required CI gates are green. All PR metadata is in order. This PR is **approved** and ready to merge. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
HAL9001 commented 2026-05-07 00:07:15 +00:00
Owner
Copy Link
Copy Source

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

--- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
CoreRasurae force-pushed bugfix/actor-add-name-arg from 85643d6e12 to f975223b10 2026-05-07 18:16:49 +00:00 Compare
CoreRasurae force-pushed bugfix/actor-add-name-arg from f975223b10 to 5cb0f7e112 2026-05-07 18:20:02 +00:00 Compare
CoreRasurae force-pushed bugfix/actor-add-name-arg from 5cb0f7e112 to 0358100a2e 2026-05-07 18:23:10 +00:00 Compare
CoreRasurae scheduled this pull request to auto merge when all checks succeed 2026-05-07 18:23:44 +00:00
CoreRasurae force-pushed bugfix/actor-add-name-arg from 0358100a2e to 398d59d71e 2026-05-07 19:47:00 +00:00 Compare
CoreRasurae merged commit 23e9848f95 into master 2026-05-07 20:05:35 +00:00
CoreRasurae deleted branch bugfix/actor-add-name-arg 2026-05-07 20:05:35 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
HAL9000
HAL9001
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.
 overdue
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 Label
Type
Bug
Milestone
No items
No Milestone v3.5.0
Projects
Clear projects
No project
Assignees
Clear assignees
aditya (Aditya Chhabra) aleenaumair (Aleena Umair) brent.edwards (Brent Edwards) CoreRasurae (Luis Mendes) drew (Drew Morris) eugen.thaci (Eugen Thaci) freemo (Jeffrey Phillips Freeman) HAL9000 (HAL 9000) HAL9001 (HAL9001) hamza.khyari (Hamza Khyari) hurui200320 (Rui Hu) justin.morris khird (Kyle Hird) org.cleveragents
No Assignees HAL9000
3 Participants
Notifications
Due Date
No due date set.
Blocks
#4186 agents actor add command signature contradicts specification
cleveragents/cleveragents-core
Reference: cleveragents/cleveragents-core#10974
Delete Branch "bugfix/actor-add-name-arg"

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?