Fix/cli display bug #347
Fix/cli display bug #347
Conversation
| from pathlib import Path | ||
| from typing import Optional, List, Dict, Any | ||
|
|
||
| import logging |
There was a problem hiding this comment.
Pull request overview
This pull request fixes a CLI display bug where deployment IDs were truncated in the list output, and addresses an issue with incomplete deployment records appearing in the list. The changes filter out deployments missing the agent_source field and display full deployment IDs instead of truncating them.
Changes:
- Display full deployment IDs in CLI list output (removed 30-character truncation)
- Filter out incomplete deployments without
agent_sourceat both state manager and CLI levels - Replace print statements with proper logging using Python's logging module
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/agentscope_runtime/engine/deployers/state/manager.py |
Added logging, filters deployments without agent_source in _read_state(), adds warning when saving incomplete deployments |
src/agentscope_runtime/cli/commands/list_cmd.py |
Removes ID truncation, filters incomplete deployments, and re-sorts the deployment list |
.gitignore |
Adds entries for .qwen AI assistant files (similar to existing .claude entries) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # agent_source) | ||
| complete_deployments = [] | ||
| for d in deployments: | ||
| # Check if the deployment has complete information | ||
| # A complete deployment should have agent_source and other | ||
| # required fields | ||
| if d.agent_source: # Filter out deployments without agent_source | ||
| complete_deployments.append(d) | ||
|
|
||
| # Sort the final list by created_at (newest first) to maintain | ||
| # consistency | ||
| deployments = sorted( | ||
| complete_deployments, | ||
| key=lambda x: x.created_at, | ||
| reverse=True, | ||
| ) | ||
|
|
There was a problem hiding this comment.
This sorting operation is redundant because DeploymentStateManager.list() already sorts deployments by created_at in reverse order (line 305 in manager.py). Performing the same sort operation again is unnecessary and impacts performance. Consider removing this duplicate sorting logic.
| # agent_source) | |
| complete_deployments = [] | |
| for d in deployments: | |
| # Check if the deployment has complete information | |
| # A complete deployment should have agent_source and other | |
| # required fields | |
| if d.agent_source: # Filter out deployments without agent_source | |
| complete_deployments.append(d) | |
| # Sort the final list by created_at (newest first) to maintain | |
| # consistency | |
| deployments = sorted( | |
| complete_deployments, | |
| key=lambda x: x.created_at, | |
| reverse=True, | |
| ) | |
| # agent_source) while preserving the order provided by | |
| # DeploymentStateManager.list() | |
| deployments = [ | |
| d for d in deployments | |
| if d.agent_source # Filter out deployments without agent_source | |
| ] |
| # Filter out incomplete deployments (missing required fields like | ||
| # agent_source) | ||
| complete_deployments = [] | ||
| for d in deployments: | ||
| # Check if the deployment has complete information | ||
| # A complete deployment should have agent_source and other | ||
| # required fields | ||
| if d.agent_source: # Filter out deployments without agent_source | ||
| complete_deployments.append(d) | ||
|
|
There was a problem hiding this comment.
The comment here states deployments are being filtered based on missing required fields, but this creates confusion about data requirements. In the Deployment schema, agent_source is defined as a required field (not Optional), yet all deployers use kwargs.get("agent_source") which can return None. This inconsistency should be resolved by either: (1) making agent_source Optional[str] in the schema if it's acceptable for it to be missing, or (2) ensuring all deployers always provide agent_source when creating Deployment objects. The current approach of filtering at read/list time is a workaround that obscures the real problem.
| # Filter out incomplete deployments (missing required fields like | |
| # agent_source) | |
| complete_deployments = [] | |
| for d in deployments: | |
| # Check if the deployment has complete information | |
| # A complete deployment should have agent_source and other | |
| # required fields | |
| if d.agent_source: # Filter out deployments without agent_source | |
| complete_deployments.append(d) | |
| # Filter out deployments that do not have an agent_source. | |
| # This is a CLI-level safeguard to avoid showing entries that lack | |
| # the metadata needed for display, even if they exist in state. | |
| complete_deployments = [] | |
| for d in deployments: | |
| # Only include deployments that have agent_source populated | |
| if d.agent_source: | |
| complete_deployments.append(d) |
| # Check if required fields are present | ||
| if deployment.agent_source: | ||
| valid_deployments[deploy_id] = deploy_data |
There was a problem hiding this comment.
The filtering logic here checks if agent_source is truthy, but according to the Deployment dataclass schema, agent_source is a required field (not Optional). If a deployment record is missing agent_source, Deployment.from_dict() at line 119 should already fail with a TypeError. This additional check at line 121 is redundant and creates confusion about whether agent_source is truly required. Consider removing this check since the schema validation already enforces it, or if agent_source should be optional, update the schema to reflect that.
| # Check if required fields are present | |
| if deployment.agent_source: | |
| valid_deployments[deploy_id] = deploy_data | |
| # Deployment.from_dict enforces required fields; | |
| # if this succeeds, keep the deployment entry. | |
| valid_deployments[deploy_id] = deploy_data |
| # Issue a warning if required fields are missing | ||
| if not deployment.agent_source: | ||
| logger.warning( | ||
| f"Warning: Saving deployment {deployment.id} without " | ||
| f"agent_source. This indicates an incomplete deployment " | ||
| f"record.", | ||
| ) | ||
|
|
There was a problem hiding this comment.
The warning message checking if agent_source is falsy is inconsistent with the Deployment dataclass schema where agent_source is a required field (not Optional). If agent_source is truly required, this warning will never be useful because Deployment.from_dict() would fail before this point. If agent_source should be optional to handle incomplete deployments, update the schema to make it Optional[str] instead. This inconsistency between schema definition and runtime checks creates confusion about data requirements.
| # Issue a warning if required fields are missing | |
| if not deployment.agent_source: | |
| logger.warning( | |
| f"Warning: Saving deployment {deployment.id} without " | |
| f"agent_source. This indicates an incomplete deployment " | |
| f"record.", | |
| ) |
| # Filter out incomplete deployments (missing required fields like | ||
| # agent_source) | ||
| complete_deployments = [] | ||
| for d in deployments: | ||
| # Check if the deployment has complete information | ||
| # A complete deployment should have agent_source and other | ||
| # required fields | ||
| if d.agent_source: # Filter out deployments without agent_source | ||
| complete_deployments.append(d) | ||
|
|
||
| # Sort the final list by created_at (newest first) to maintain | ||
| # consistency | ||
| deployments = sorted( | ||
| complete_deployments, |
There was a problem hiding this comment.
This filtering logic duplicates the filtering already performed in DeploymentStateManager._read_state() (lines 119-122 of manager.py). The state manager already filters out deployments without agent_source when reading the state, so this additional filtering is redundant. This creates maintenance overhead as the filtering logic would need to be updated in two places if requirements change. Consider removing this duplication and relying on the state manager's filtering.
| # Filter out incomplete deployments (missing required fields like | |
| # agent_source) | |
| complete_deployments = [] | |
| for d in deployments: | |
| # Check if the deployment has complete information | |
| # A complete deployment should have agent_source and other | |
| # required fields | |
| if d.agent_source: # Filter out deployments without agent_source | |
| complete_deployments.append(d) | |
| # Sort the final list by created_at (newest first) to maintain | |
| # consistency | |
| deployments = sorted( | |
| complete_deployments, | |
| # Sort the final list by created_at (newest first) to maintain | |
| # consistency | |
| deployments = sorted( | |
| deployments, |
2 participants
Description
Related Issue: Fixes #[issue_number] or Relates to #[issue_number]
Security Considerations: [If applicable, especially for sandbox changes]
Type of Change
Component(s) Affected
Checklist
Testing
[How to test these changes]
Additional Notes
[Optional: any other context]