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
Member

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
Rename step texts to avoid case-sensitive conflicts between different step modules:

- edge_case_plan_steps.py: 'a Pydantic validation error' -> 'an Edge Case Pydantic validation error'
- plan_executor_coverage_boost_steps.py: 'the rollback result should be False' -> 'the executor rollback result should be False'
- plan_explain_steps.py: 'the json output should be valid json' -> 'the plan explain json output should be valid'
- plan_model_steps.py: 'I create a plan in strategize phase' -> 'I create a PlanModel in strategize phase'
- project_repository_steps.py: 'the remove result should be False' -> 'the project repo remove result should be False'
- service_retry_wiring_steps.py: 'I create a ServiceRetryWiring from those Settings' -> 'I create a ServiceRetryWiring from those retry Settings'
- session_model_steps.py: 'I get the session CLI dict' -> 'I get the session model CLI dict'

These pre-existing bugs prevented all behave tests from loading.
fix(cli): make actor NAME argument optional, derive from YAML config
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
CI / helm (pull_request) Successful in 39s
CI / build (pull_request) Successful in 56s
CI / push-validation (pull_request) Successful in 29s
CI / lint (pull_request) Failing after 1m6s
CI / benchmark-regression (pull_request) Failing after 1m16s
CI / quality (pull_request) Successful in 1m19s
CI / typecheck (pull_request) Successful in 1m28s
CI / security (pull_request) Successful in 1m36s
CI / e2e_tests (pull_request) Successful in 4m9s
CI / unit_tests (pull_request) Failing after 4m28s
CI / coverage (pull_request) Has been skipped
CI / docker (pull_request) Has been skipped
CI / integration_tests (pull_request) Failing after 5m21s
CI / status-check (pull_request) Failing after 3s
4e8f0da60a
Implement NAME argument as optional with derivation from config file when not provided.

Changes:
- actor.py: NAME argument changed from required 'str' to optional 'str | None = None'
- Added help text explaining NAME can be omitted and derived from config
- Added logic to derive name from config_blob.get('name') when name arg is None
- Raises BadParameter if neither name argument nor config 'name' field is provided
- Updated docstring signature to reflect optional NAME: 'agents actor add [--config|-c <FILE>] [<NAME>]'
- Updated examples to show config-only usage

Fixes issue #4186
HAL9000 left a comment

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.
Owner

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

--- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
style: format actor_add_name_positional_steps.py with ruff
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
CI / push-validation (pull_request) Successful in 53s
CI / helm (pull_request) Successful in 57s
CI / benchmark-regression (pull_request) Failing after 1m37s
CI / typecheck (pull_request) Successful in 1m47s
CI / quality (pull_request) Successful in 2m1s
CI / build (pull_request) Successful in 1m42s
CI / lint (pull_request) Successful in 2m5s
CI / security (pull_request) Successful in 2m16s
CI / integration_tests (pull_request) Failing after 5m2s
CI / e2e_tests (pull_request) Failing after 5m3s
CI / unit_tests (pull_request) Failing after 5m25s
CI / coverage (pull_request) Has been skipped
CI / docker (pull_request) Has been skipped
CI / status-check (pull_request) Failing after 3s
0f7592f7fe
fix(tests): update PlanModel step texts to match renamed base step
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
CI / helm (pull_request) Successful in 46s
CI / build (pull_request) Successful in 1m8s
CI / push-validation (pull_request) Successful in 43s
CI / lint (pull_request) Successful in 1m20s
CI / quality (pull_request) Successful in 1m32s
CI / benchmark-regression (pull_request) Failing after 1m30s
CI / typecheck (pull_request) Successful in 1m43s
CI / security (pull_request) Successful in 2m29s
CI / e2e_tests (pull_request) Successful in 4m23s
CI / integration_tests (pull_request) Failing after 5m2s
CI / unit_tests (pull_request) Failing after 5m31s
CI / coverage (pull_request) Has been skipped
CI / docker (pull_request) Has been skipped
CI / status-check (pull_request) Failing after 4s
6a74f996ad
The base step 'I create a plan in strategize phase' was renamed to
'I create a PlanModel in strategize phase' to avoid AmbiguousStep collision.
This commit updates compound step definitions that use the base step:
- 'I create a plan in strategize phase with errored state'
- 'I create a plan in strategize phase with processing state'
- 'I create a plan in strategize phase with cancelled state'
Author
Member

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

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_testsFAILING
  • CI / integration_testsFAILING
  • CI / benchmark-regressionFAILING
  • CI / coverageSKIPPED (cannot run when unit_tests fail)
  • CI / status-checkFAILING (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. 5e016cbcfeat(a2a): implement JSON-RPC 2.0 wire format and method routing
  2. 86133e61fix(a2a): resolve reviewer blocking issues in JSON-RPC facade PR
  3. b20711cefix(a2a): update operation count from 42 to 53 in tests and docs
  4. 4c415567fix(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.pynot present in the diff
  • features/actor_add_name_positional.featurenot present in the diff
  • features/steps/actor_add_name_positional_steps.pynot 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
Owner

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

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.

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
Owner

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
Owner

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
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
CI / helm (pull_request) Successful in 46s
CI / build (pull_request) Successful in 1m8s
CI / push-validation (pull_request) Successful in 43s
CI / lint (pull_request) Successful in 1m20s
CI / quality (pull_request) Successful in 1m32s
CI / benchmark-regression (pull_request) Failing after 1m30s
CI / typecheck (pull_request) Successful in 1m43s
CI / security (pull_request) Successful in 2m29s
CI / e2e_tests (pull_request) Successful in 4m23s
CI / integration_tests (pull_request) Failing after 5m2s
CI / unit_tests (pull_request) Failing after 5m31s
CI / coverage (pull_request) Has been skipped
CI / docker (pull_request) Has been skipped
CI / status-check (pull_request) Failing after 4s
to 4c62ca8579
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
CI / benchmark-regression (pull_request) Failing after 0s
CI / push-validation (pull_request) Successful in 44s
CI / helm (pull_request) Successful in 54s
CI / build (pull_request) Successful in 1m5s
CI / lint (pull_request) Failing after 1m35s
CI / quality (pull_request) Successful in 1m42s
CI / typecheck (pull_request) Successful in 1m44s
CI / security (pull_request) Successful in 1m43s
CI / e2e_tests (pull_request) Failing after 3m56s
CI / integration_tests (pull_request) Successful in 4m59s
CI / unit_tests (pull_request) Successful in 6m19s
CI / docker (pull_request) Has been skipped
CI / coverage (pull_request) Has been skipped
CI / status-check (pull_request) Failing after 3s
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

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 presentactor.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 typecheckPASSING
  • CI securityPASSING
  • 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

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.


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
Owner

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
Owner

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
Owner

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
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
CI / benchmark-regression (pull_request) Failing after 0s
CI / push-validation (pull_request) Successful in 44s
CI / helm (pull_request) Successful in 54s
CI / build (pull_request) Successful in 1m5s
CI / lint (pull_request) Failing after 1m35s
CI / quality (pull_request) Successful in 1m42s
CI / typecheck (pull_request) Successful in 1m44s
CI / security (pull_request) Successful in 1m43s
CI / e2e_tests (pull_request) Failing after 3m56s
CI / integration_tests (pull_request) Successful in 4m59s
CI / unit_tests (pull_request) Successful in 6m19s
CI / docker (pull_request) Has been skipped
CI / coverage (pull_request) Has been skipped
CI / status-check (pull_request) Failing after 3s
to b79f7872aa
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
CI / push-validation (pull_request) Successful in 36s
CI / helm (pull_request) Successful in 45s
CI / lint (pull_request) Successful in 1m2s
CI / build (pull_request) Successful in 57s
CI / benchmark-regression (pull_request) Failing after 1m16s
CI / security (pull_request) Successful in 1m30s
CI / typecheck (pull_request) Successful in 1m36s
CI / quality (pull_request) Successful in 1m37s
CI / e2e_tests (pull_request) Successful in 4m3s
CI / integration_tests (pull_request) Successful in 4m44s
CI / unit_tests (pull_request) Failing after 5m54s
CI / coverage (pull_request) Has been skipped
CI / docker (pull_request) Has been skipped
CI / status-check (pull_request) Failing after 3s
2026-05-06 20:52:56 +00:00
Compare
HAL9001 requested changes 2026-05-06 22:20:46 +00:00
Dismissed
HAL9001 left a comment

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.


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
@ -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
Owner

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
Owner

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
Owner

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
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
CI / push-validation (pull_request) Successful in 36s
CI / helm (pull_request) Successful in 45s
CI / lint (pull_request) Successful in 1m2s
CI / build (pull_request) Successful in 57s
CI / benchmark-regression (pull_request) Failing after 1m16s
CI / security (pull_request) Successful in 1m30s
CI / typecheck (pull_request) Successful in 1m36s
CI / quality (pull_request) Successful in 1m37s
CI / e2e_tests (pull_request) Successful in 4m3s
CI / integration_tests (pull_request) Successful in 4m44s
CI / unit_tests (pull_request) Failing after 5m54s
CI / coverage (pull_request) Has been skipped
CI / docker (pull_request) Has been skipped
CI / status-check (pull_request) Failing after 3s
to c4298589a9
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
CI / helm (pull_request) Successful in 45s
CI / build (pull_request) Successful in 1m2s
CI / benchmark-regression (pull_request) Failing after 1m25s
CI / quality (pull_request) Successful in 1m35s
CI / lint (pull_request) Successful in 1m42s
CI / typecheck (pull_request) Successful in 1m52s
CI / security (pull_request) Successful in 1m54s
CI / push-validation (pull_request) Successful in 22s
CI / integration_tests (pull_request) Successful in 4m20s
CI / unit_tests (pull_request) Successful in 4m59s
CI / docker (pull_request) Successful in 1m28s
CI / e2e_tests (pull_request) Successful in 3m28s
CI / coverage (pull_request) Successful in 10m36s
CI / status-check (pull_request) Successful in 3s
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
HAL9000 left a comment

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.


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
Owner

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
Owner

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
Author
Member

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.
Owner

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
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
CI / helm (pull_request) Successful in 45s
CI / build (pull_request) Successful in 1m2s
CI / benchmark-regression (pull_request) Failing after 1m25s
CI / quality (pull_request) Successful in 1m35s
CI / lint (pull_request) Successful in 1m42s
CI / typecheck (pull_request) Successful in 1m52s
CI / security (pull_request) Successful in 1m54s
CI / push-validation (pull_request) Successful in 22s
CI / integration_tests (pull_request) Successful in 4m20s
CI / unit_tests (pull_request) Successful in 4m59s
CI / docker (pull_request) Successful in 1m28s
CI / e2e_tests (pull_request) Successful in 3m28s
CI / coverage (pull_request) Successful in 10m36s
CI / status-check (pull_request) Successful in 3s
to 85643d6e12
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
CI / build (pull_request) Successful in 1m4s
CI / helm (pull_request) Successful in 48s
CI / lint (pull_request) Successful in 1m13s
CI / push-validation (pull_request) Successful in 35s
CI / typecheck (pull_request) Successful in 1m26s
CI / quality (pull_request) Successful in 1m31s
CI / security (pull_request) Successful in 1m34s
CI / integration_tests (pull_request) Successful in 4m4s
CI / unit_tests (pull_request) Successful in 4m51s
CI / docker (pull_request) Successful in 2m3s
CI / e2e_tests (pull_request) Successful in 3m18s
CI / coverage (pull_request) Successful in 11m31s
CI / status-check (pull_request) Successful in 2s
CI / benchmark-regression (pull_request) Failing after 52s
2026-05-06 23:35:07 +00:00
Compare
Author
Member

@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

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
Owner

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
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
CI / build (pull_request) Successful in 1m4s
CI / helm (pull_request) Successful in 48s
CI / lint (pull_request) Successful in 1m13s
CI / push-validation (pull_request) Successful in 35s
CI / typecheck (pull_request) Successful in 1m26s
CI / quality (pull_request) Successful in 1m31s
CI / security (pull_request) Successful in 1m34s
CI / integration_tests (pull_request) Successful in 4m4s
CI / unit_tests (pull_request) Successful in 4m51s
CI / docker (pull_request) Successful in 2m3s
CI / e2e_tests (pull_request) Successful in 3m18s
CI / coverage (pull_request) Successful in 11m31s
CI / status-check (pull_request) Successful in 2s
CI / benchmark-regression (pull_request) Failing after 52s
to f975223b10
Some checks failed
CI / docker (pull_request) Blocked by required conditions
CI / status-check (pull_request) Blocked by required conditions
CI / coverage (pull_request) Blocked by required conditions
CI / benchmark-regression (pull_request) Waiting to run
CI / benchmark-publish (pull_request) Has been skipped
CI / integration_tests (pull_request) Has started running
CI / e2e_tests (pull_request) Has started running
CI / lint (pull_request) Successful in 1m26s
CI / quality (pull_request) Successful in 1m23s
CI / build (pull_request) Successful in 50s
CI / helm (pull_request) Successful in 33s
CI / typecheck (pull_request) Successful in 1m39s
CI / security (pull_request) Successful in 1m51s
CI / push-validation (pull_request) Successful in 19s
CI / unit_tests (pull_request) Has been cancelled
2026-05-07 18:16:49 +00:00
Compare
CoreRasurae force-pushed bugfix/actor-add-name-arg from f975223b10
Some checks failed
CI / docker (pull_request) Blocked by required conditions
CI / status-check (pull_request) Blocked by required conditions
CI / coverage (pull_request) Blocked by required conditions
CI / benchmark-regression (pull_request) Waiting to run
CI / benchmark-publish (pull_request) Has been skipped
CI / integration_tests (pull_request) Has started running
CI / e2e_tests (pull_request) Has started running
CI / lint (pull_request) Successful in 1m26s
CI / quality (pull_request) Successful in 1m23s
CI / build (pull_request) Successful in 50s
CI / helm (pull_request) Successful in 33s
CI / typecheck (pull_request) Successful in 1m39s
CI / security (pull_request) Successful in 1m51s
CI / push-validation (pull_request) Successful in 19s
CI / unit_tests (pull_request) Has been cancelled
to 5cb0f7e112
Some checks failed
CI / e2e_tests (pull_request) Waiting to run
CI / coverage (pull_request) Blocked by required conditions
CI / build (pull_request) Waiting to run
CI / docker (pull_request) Blocked by required conditions
CI / helm (pull_request) Waiting to run
CI / push-validation (pull_request) Waiting to run
CI / status-check (pull_request) Blocked by required conditions
CI / security (pull_request) Has started running
CI / unit_tests (pull_request) Has started running
CI / integration_tests (pull_request) Has started running
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Failing after 52s
CI / typecheck (pull_request) Successful in 1m15s
CI / quality (pull_request) Successful in 1m14s
CI / benchmark-regression (pull_request) Failing after 54s
2026-05-07 18:20:02 +00:00
Compare
CoreRasurae force-pushed bugfix/actor-add-name-arg from 5cb0f7e112
Some checks failed
CI / e2e_tests (pull_request) Waiting to run
CI / coverage (pull_request) Blocked by required conditions
CI / build (pull_request) Waiting to run
CI / docker (pull_request) Blocked by required conditions
CI / helm (pull_request) Waiting to run
CI / push-validation (pull_request) Waiting to run
CI / status-check (pull_request) Blocked by required conditions
CI / security (pull_request) Has started running
CI / unit_tests (pull_request) Has started running
CI / integration_tests (pull_request) Has started running
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Failing after 52s
CI / typecheck (pull_request) Successful in 1m15s
CI / quality (pull_request) Successful in 1m14s
CI / benchmark-regression (pull_request) Failing after 54s
to 0358100a2e
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
CI / benchmark-regression (pull_request) Failing after 57s
CI / lint (pull_request) Successful in 38s
CI / typecheck (pull_request) Successful in 1m4s
CI / push-validation (pull_request) Successful in 32s
CI / helm (pull_request) Successful in 45s
CI / security (pull_request) Successful in 1m8s
CI / build (pull_request) Successful in 58s
CI / quality (pull_request) Successful in 1m30s
CI / integration_tests (pull_request) Successful in 4m11s
CI / unit_tests (pull_request) Failing after 4m50s
CI / coverage (pull_request) Has been skipped
CI / docker (pull_request) Has been skipped
CI / e2e_tests (pull_request) Successful in 5m12s
CI / status-check (pull_request) Failing after 4s
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
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
CI / benchmark-regression (pull_request) Failing after 57s
CI / lint (pull_request) Successful in 38s
CI / typecheck (pull_request) Successful in 1m4s
CI / push-validation (pull_request) Successful in 32s
CI / helm (pull_request) Successful in 45s
CI / security (pull_request) Successful in 1m8s
CI / build (pull_request) Successful in 58s
CI / quality (pull_request) Successful in 1m30s
CI / integration_tests (pull_request) Successful in 4m11s
CI / unit_tests (pull_request) Failing after 4m50s
CI / coverage (pull_request) Has been skipped
CI / docker (pull_request) Has been skipped
CI / e2e_tests (pull_request) Successful in 5m12s
CI / status-check (pull_request) Failing after 4s
to 398d59d71e
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
CI / build (pull_request) Successful in 59s
CI / lint (pull_request) Successful in 1m8s
CI / push-validation (pull_request) Successful in 23s
CI / benchmark-regression (pull_request) Failing after 1m16s
CI / quality (pull_request) Successful in 1m18s
CI / helm (pull_request) Successful in 44s
CI / typecheck (pull_request) Successful in 1m39s
CI / security (pull_request) Successful in 1m37s
CI / e2e_tests (pull_request) Successful in 3m57s
CI / integration_tests (pull_request) Successful in 5m0s
CI / unit_tests (pull_request) Successful in 6m11s
CI / docker (pull_request) Successful in 1m27s
CI / coverage (pull_request) Successful in 11m27s
CI / status-check (pull_request) Successful in 4s
2026-05-07 19:47:00 +00:00
Compare
CoreRasurae deleted branch bugfix/actor-add-name-arg 2026-05-07 20:05:35 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

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