fix(cli): make actor NAME argument optional, derive from YAML config #10974
Labels
No labels
auto/needs-reevaluation
controller-managed
auto/blocked-by-deps
auto/ci-timeout
auto/claimed-implementer
auto/claimed-merge
auto/claimed-reviewer
auto/driver-down
auto/invariant-violation
auto/last-attempt-tier-0
auto/last-attempt-tier-1
auto/last-attempt-tier-2
auto/last-attempt-tier-min
Automation Tracking
auto/needs-conflict-resolution
auto/needs-implementer
auto/postmortem
auto/ready-to-merge
auto/restart-throttled
auto/revert
auto/sentinel
auto/stale-inactivity
auto/unstable
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs Feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
CI Blocker
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Automation
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Refactor
Type
Support
Type
Task
Type
Testing
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Blocks
#4186 agents actor add command signature contradicts specification
cleveragents/cleveragents-core
Reference
cleveragents/cleveragents-core!10974
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bugfix/actor-add-name-arg"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Issue
Fixes issue #4186 - CLI signature contradiction with specification
Changes
Testing
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 #4186CI Status — BLOCKING
No CI checks have been reported for this PR yet. All individual check statuses are pending (null).
CI / lint (pull_request)— PENDINGCI / typecheck (pull_request)— PENDINGCI / security (pull_request)— PENDINGCI / unit_tests (pull_request)— PENDINGCI / coverage (pull_request)— PENDINGCI / e2e_tests (pull_request)— PENDINGCI / status-check (pull_request)— PENDINGPer 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.
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
i am still working on this ticket
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— FAILINGCI / integration_tests— FAILINGCI / benchmark-regression— FAILINGCI / 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-argcontains zero commits implementing this fix.The 4 unique commits in this branch relative to
masterare:5e016cbc—feat(a2a): implement JSON-RPC 2.0 wire format and method routing86133e61—fix(a2a): resolve reviewer blocking issues in JSON-RPC facade PRb20711ce—fix(a2a): update operation count from 42 to 53 in tests and docs4c415567—fix(a2a): fix lint/format failures and remove incorrect tdd_expected_fail tagsAll 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 difffeatures/actor_add_name_positional.feature— not present in the difffeatures/steps/actor_add_name_positional_steps.py— not present in the diffThe actual diff contains: A2A facade module refactoring (
facade.pysplit into_facade.py,_handlers.py,_handlers_extension.py,_handlers_standard.py,_operations.py), a newagent_card.pymodule, new BDD feature file for A2A agent card, and Robot Framework integration tests.What to do: The branch needs to be rebased onto
masterand 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:
v3.5.0; this PR must also be assigned tov3.5.0.Type/label — exactly oneType/label is required (e.g.Type/Bug). Currently onlyNeeds Feedbackis present.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 theType/Buglabel, and ensure each commit footer containsISSUES CLOSED: #4186.Summary
This PR requires significant rework before it can be reviewed properly:
NAMEoptional inagents actor add, derive from YAMLnamefield, raiseBadParameterif neither is provided.v3.5.0,Type/Buglabel, andISSUES CLOSED: #4186commit 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
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:
CI / lintCI / typecheckCI / securityCI / unit_testsCI / integration_testsCI / coverageCI / benchmark-regressionCI / status-checkPer 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_testsandnox -s integration_testslocally and fix all failures before resubmitting.2. Missing
ISSUES CLOSED:footer in commit messagesPer CONTRIBUTING.md, every commit footer must include
ISSUES CLOSED: #N. The implementation commit (4e8f0da6) usesFixes issue #4186in the body — this is the wrong format and location. The correct form is a dedicated footer line: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 assignv3.5.0.4. Missing
Type/Buglabel on PRPer CONTRIBUTING.md, every PR must have exactly one
Type/label. Issue #4186 isType/Bug, so this PR requires aType/Buglabel. The current labelNeeds Feedbackis a process label, not a type label. Please addType/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.mdnot updatedAcceptance criterion #3 in issue #4186 explicitly requires: "The specification in
docs/specification.mdis updated to match the actual CLI behavior." This PR updates the CLI and adds tests but does not updatedocs/specification.mdto reflect the newagents actor addcommand signature whereNAMEis optional. The spec at lines 4938-4948 and line 279 must be updated to reflect the corrected signatureagents 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 namedbugfix/mN-<descriptive-name>where N is the milestone number. For milestone v3.5.0 (M5), this should bebugfix/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 separatetdd/mN-branch that proves the bug exists (with@tdd_expected_failtag) before the fix is applied. The new scenarioactor add without NAME positional argument uses config nametagged with@tdd_issue @tdd_issue_4186is the passing test for the fix — it should be on atdd/branch as a@tdd_expected_failscenario 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.pyis correct:name: str | None = None, derivation fromconfig_blob.get("name"), andBadParameterif 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
BLOCKING — Missing
ISSUES CLOSED:footer in commit message.The commit message for
4e8f0da6usesFixes issue #4186in the body text. Per CONTRIBUTING.md, the correct format is a dedicated footer line:This applies to ALL four commits in this PR — none have a proper
ISSUES CLOSED: #Nfooter. Please interactive-rebase to correct the commit footers before re-submitting.Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
6a74f996ad4c62ca8579Re-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
actor.pycorrectly makesNAMEoptional (str | None = None), derives fromconfig_blob.get("name"), and raisesBadParameterif neither is provided.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.)environment.pyfixes replacingtempfile.mktemp()withtempfile.mkstemp()and addingfcntllocking are correct and well-implemented.@tdd_expected_failremoval 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 / lintis a required merge gate. Please runnox -s lintlocally, fix all lint/format errors, and push a corrected commit.🔴 BLOCKER 2 — CI coverage is SKIPPED (Required Gate)
The
CI / coveragejob shows statusHas been skippedon 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.BadParameterwhen neither the NAME argument nor the confignamefield is provided. However, there is no Behave scenario testing this error path. The only scenario tagged@tdd_issue_4186tests 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:
🔴 BLOCKER 4 — Missing
ISSUES CLOSED:footer on all commitsNone of the five commits in this PR have the required
ISSUES CLOSED: #Nfooter format:4c62ca85Fixes issue #4186in body — wrong formatfca8f5ccc7f2a807b3e575092d68c7a2Per CONTRIBUTING.md, every commit footer must include:
(or
Refs: #Nfor 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 assignv3.5.0.🔴 BLOCKER 6 — Missing
Type/BuglabelPer CONTRIBUTING.md, every PR must carry exactly one
Type/label. Issue #4186 isType/Bug, so this PR requiresType/Bug. The only label currently on the PR isNeeds Feedback(a process label). Please addType/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.mdfile 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_failbelong to a different issueThe feature file
actor_add_name_positional.featurecontains 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_failscenarios should live on atdd/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 owntdd/branch.Branch name missing milestone prefix
The branch is
bugfix/actor-add-name-arg. Per CONTRIBUTING.md, it should bebugfix/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 = Nonesignature, config derivation logic, andBadParameterraise 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:
nox -s lint)nox -s coverage_reportruns and reports ≥97%)ISSUES CLOSED: #4186to all commitsv3.5.0to the PRType/Buglabel to the PRAutomated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
BLOCKING — Missing
ISSUES CLOSED:footer.The commit message for
4c62ca85usesFixes issue #4186in the body. Per CONTRIBUTING.md, the required format is a dedicated footer line: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: #Nfooters).Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
BLOCKING — Missing Behave scenario for the
BadParametererror path.The implementation at line 604 raises
BadParameterwhen neither the NAME argument nor the confignamefield is present. This error path has no Behave test coverage. The only scenario tagged@tdd_issue_4186tests 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.featuresuch as:Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
4c62ca8579b79f7872aaRe-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
Type/Buglabel — now applied ✅❌ Items Still Outstanding (Blocking)
🔴 BLOCKER 1 — CI unit_tests Still Failing
CI / unit_testsis FAILING after 5m54s on headb79f7872aa5af1566ce137ba75f93dfcd6520f38. As a direct consequence,CI / coverageis SKIPPED and cannot verify the ≥97% threshold.CI / status-checkis also FAILING as the aggregate gate.Current CI state:
CI / lintCI / typecheckCI / securityCI / unit_testsCI / integration_testsCI / e2e_testsCI / coverageCI / benchmark-regressionCI / status-checkPer company policy,
unit_testsandcoverageare required merge gates. Please runnox -s unit_testslocally and fix all failures. Once unit_tests pass, coverage will run and the ≥97% threshold must be verified withnox -s coverage_report.🔴 BLOCKER 2 — Missing Behave Scenario for BadParameter Error Path
The inline comment from review #7767 requested a Behave scenario testing the
BadParametererror path (when neither NAME argument nor confignamefield is provided). This scenario is still absent fromfeatures/actor_add_name_positional.feature.The current feature file contains only:
@tdd_issue_4230 @tdd_expected_failscenarios (happy path — positional NAME)@tdd_issue_4186scenario (happy path — config-derived name)The error path at
actor.pylines 601–605 (raisesBadParameterwhenconfig_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:
And implement the corresponding step definition for the
Thenassertion.🔴 BLOCKER 3 — Missing
ISSUES CLOSED:Footer on CommitsThe implementation commit (
b79f7872) usesFixes issue #4186embedded in the commit body. Per CONTRIBUTING.md, the required format is a dedicated footer line (after a blank line separating it from the body):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 eitherISSUES CLOSED: #4186orRefs: #4186in 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 assignv3.5.0to the PR.Non-Blocking Observations
Implementation correctness
The core implementation in
actor.pyis correct and aligns with the specification:name: str | None = Nonecorrectly makes the argument optionalconfig_blob.get("name")is correctBadParameterraised when neither source provides a name is correctenvironment.py improvements
The
tempfile.mktemp()→tempfile.mkstemp()replacements andfcntl.flocklocking 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:
nox -s unit_testslocally and fix all failures; then verifynox -s coverage_report≥97%actor addwhen config has nonamefieldISSUES CLOSED: #4186as a dedicated footer line on all commitsv3.5.0— via the PR sidebarOnce 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_4186Scenario: actor add without NAME positional argument uses config nameGiven an actor CLI runnerBLOCKING — Missing Behave scenario for the
BadParametererror path.The implementation in
actor.pyraisestyper.BadParameterwhen neither the positionalNAMEargument nor the confignamefield 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:
Note: The
Given I have an actor JSON config file without a name fieldstep already exists in the step file (it was present before this PR). TheThenstepthe actor command should fail with a BadParameter error about missing actor namealso 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
ISSUES CLOSED:footer on implementation commit.This commit (
b79f7872) containsFixes issue #4186inline in the body text. Per CONTRIBUTING.md, the required format is a dedicated footer line (separated from the body by a blank line):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
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
b79f7872aac4298589a9Re-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
Type/Buglabel — applied ✅ (already resolved in prior review)Note on benchmark-regression:
CI / benchmark-regressionis still failing (Failing after 1m25s), but this job is not part of the required merge gate (status-checkdoes 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
BadParametererror path when neither NAME argument nor confignamefield is provided. This scenario is still absent fromfeatures/actor_add_name_positional.feature.The current feature file has three scenarios:
@tdd_issue_4230 @tdd_expected_fail— actor add accepts NAME as positional (expected fail, issue #4230 not fixed yet)@tdd_issue_4230 @tdd_expected_fail— NAME positional argument takes precedence (expected fail, issue #4230 not fixed yet)@tdd_issue_4186— actor add without NAME uses config name (happy path)The error path at
actor.pylines 600–607 (raisesBadParameterwhenconfig_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:The step definitions for all four steps above already exist in
features/steps/actor_add_name_positional_steps.pyfrom the pre-existing implementation — no new step implementations are required.🔴 BLOCKER 2 — Missing
ISSUES CLOSED:Footer on All CommitsNone of the six commits in this PR have the required
ISSUES CLOSED: #Nfooter format:c4298589Fixes issue #4186embedded in body — wrong format and wrong location90757a08c085a45edd1499404c40d0211ba1f168Per CONTRIBUTING.md, every commit footer must include a dedicated footer line (separated from the body by a blank line):
For commits that do not directly close the issue (the supporting test/fix commits), use:
Please interactive-rebase all six commits to add the correct footer format.
Full Review Checklist Assessment
NAMEis optional, derives from config, raisesBadParameterwhen neither provided. All acceptance criteria in issue #4186 met except error path test.BadParameterwhen no name in config) has no Behave scenario (BLOCKER 1).str | Noneannotation correct; no# type: ignore.ISSUES CLOSED: #4186footer 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:
features/actor_add_name_positional.feature(no new step definitions needed)ISSUES CLOSED: #4186(orRefs: #4186for 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
BLOCKING — Missing Behave scenario for the
BadParametererror path.The implementation at
actor.pylines 600–607 raisestyper.BadParameterwhen neither the positionalNAMEargument nor the confignamefield 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: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) usesFixes issue #4186embedded 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):The other 5 commits (
90757a08,c085a45e,dd149940,4c40d021,1ba1f168) have no issue reference at all. Each should include eitherISSUES CLOSED: #4186(for the main fix commit) orRefs: #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
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.
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
c4298589a985643d6e12@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.
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
Scenario: actor add without NAME and without config name field raises BadParameternow present infeatures/actor_add_name_positional.featurewith corresponding@thenstep inactor_add_name_positional_steps.py)ISSUES CLOSED: #4186footer — Fixed ✅ (main implementation commit85643d6enow has a properISSUES CLOSED: #4186footer line)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 requiredstatus-checkgate; appears to be a pre-existing infrastructure issue unrelated to this PRv3.5.0— assigned ✅Type/Buglabel — applied ✅Full Review Checklist
NAMEoptional, derives from confignamefield, raisesBadParameterwhen neither provided. All acceptance criteria in issue #4186 met.agents actor add --config|-c <FILE> [<NAME>].str | Noneannotation correct. No# type: ignore.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@thenstep for theBadParametererror path has the sameassert "Actor name is required" in context.result.outputstatement 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
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
85643d6e12f975223b10f975223b105cb0f7e1125cb0f7e1120358100a2e0358100a2e398d59d71e