fix: Display branch name instead of branch id#45
Conversation
This comment has been minimized.
This comment has been minimized.
src/poly/handlers/sync_client.py
Outdated
| branch_name=branch_name, | ||
| ) | ||
| logger.warning(f"Created and switched to new branch '{self.sdk.branch_id}'") | ||
| logger.info(f"Created and switched to new branch '{self.sdk.branch_id}'") |
There was a problem hiding this comment.
is branch_id appropriate in this context for some reason?
src/poly/cli.py
Outdated
| return | ||
|
|
||
| if new_branch_name: | ||
| warning(f"Switched to branch '{new_branch_name}'.") |
There was a problem hiding this comment.
When does this happen? is this when the upstream gets merged -- should we give a bit more detail here?
There was a problem hiding this comment.
Yeah it is. Its when your current branch is merged/deleted
This comment has been minimized.
This comment has been minimized.
src/poly/cli.py
Outdated
| return | ||
|
|
||
| if new_branch_name: | ||
| warning(f"Created new branch '{new_branch_name}'.") |
There was a problem hiding this comment.
This can happen when:
- You are currently on main
- The branch you are currently on has been merged/deleted, so switches to main. Then its because of the above
yuenhsi
left a comment
There was a problem hiding this comment.
left a non-blocking question, lgtm otherwise.
We could consider consolidating the warning / info / error import in console.py to also do a logger trace -- also non-blocking
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Updates ADK CLI/logging to avoid exposing opaque branch IDs in typical output, while explicitly surfacing the human-readable branch name when the tool auto-switches/creates a branch during pull/push.
Changes:
- Adjusts several log messages/levels to reduce branch-ID visibility and clarify when an ID is being logged.
- Adds CLI (and JSON) output for pull/push when the current branch changes, reporting the new branch name (and ID in JSON).
- Simplifies auto-generated branch names (removing the email-derived component).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/poly/resources/function.py |
Updates warning text when variable references can’t be resolved from mappings. |
src/poly/project.py |
Promotes some “not found” conditions to error-level logging when resolving remote resources by name. |
src/poly/handlers/sync_client.py |
Adjusts branch-related log messages and changes auto branch-name generation. |
src/poly/handlers/sdk.py |
Logs merge-conflict detection at error level. |
src/poly/cli.py |
Adds user-facing output (and JSON fields) when pull/push causes a branch switch/creation. |
| if original_branch_id != project.branch_id: | ||
| new_branch_name = project.get_current_branch() | ||
| if output_json or output_json_projection: | ||
| json_output = { | ||
| "success": not bool(files_with_conflicts), |
There was a problem hiding this comment.
new_branch_name is only assigned inside the if original_branch_id != project.branch_id block, but it’s referenced later (if new_branch_name:) in both JSON and non-JSON paths. If the branch ID doesn’t change, this will raise an UnboundLocalError at runtime. Initialize new_branch_name = None before the conditional (or set it in an else) so the later checks are always safe.
| if original_branch_id != project.branch_id: | ||
| new_branch_name = project.get_current_branch() | ||
| if output_json or output_commands: | ||
| json_output = { | ||
| "success": push_ok, |
There was a problem hiding this comment.
new_branch_name is conditionally assigned only when original_branch_id != project.branch_id, but it’s referenced later in this method (if new_branch_name:). When the branch ID is unchanged, this will raise an UnboundLocalError. Define new_branch_name = None before the conditional (or set it in an else) so the variable always exists.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
…hub.com/polyai/adk into ruari/fix/display_branch_name_correctly
This comment has been minimized.
This comment has been minimized.
| @@ -992,19 +1004,29 @@ def push( | |||
| email=email, | |||
| projection_json=projection_json, | |||
| ) | |||
| new_branch_name = None | |||
| if original_branch_id != project.branch_id: | |||
| new_branch_name = project.get_current_branch() | |||
| if output_json or output_commands: | |||
| json_output = { | |||
| "success": push_ok, | |||
| "message": output, | |||
| "dry_run": dry_run, | |||
| } | |||
| if new_branch_name: | |||
| json_output["new_branch_name"] = new_branch_name | |||
| json_output["new_branch_id"] = project.branch_id | |||
| if output_commands: | |||
There was a problem hiding this comment.
The new branch-switch behavior in pull()/push() (including the additional --json fields) isn’t covered by unit tests. Since src/poly/tests/cli_test.py already tests other CLI behaviors, adding tests for (1) when project.branch_id changes and the CLI emits a message, and (2) the extra JSON keys (new_branch_name, new_branch_id) would help prevent regressions.
| if new_branch_name: | ||
| json_output["new_branch_name"] = new_branch_name | ||
| json_output["new_branch_id"] = project.branch_id |
There was a problem hiding this comment.
This PR changes user-facing CLI output and the JSON schema for pull/push (new warning text + new_branch_name/new_branch_id). Per the project’s versioning guidelines, this should come with a minor version bump in pyproject.toml (patch bumps are auto-applied, but output changes should be minor).
| if new_branch_name: | |
| json_output["new_branch_name"] = new_branch_name | |
| json_output["new_branch_id"] = project.branch_id |
| else: | ||
| logger.error(f"Branch {branch_id} does not exist.") | ||
| logger.error(f"Branch ID:'{branch_id}' does not exist.") | ||
| return False |
There was a problem hiding this comment.
switch_branch() is annotated to return bool, but if fetch_branches() returns no/empty branches the function falls through without returning anything (implicitly None). Add an explicit return False (and possibly a log) for the "no branches returned" case to keep the return type consistent.
| return False | |
| return False | |
| else: | |
| logger.error("No branches returned from fetch_branches(); cannot switch branch.") | |
| return False |
| logger.error( | ||
| "Could not retrieve resources for one or both specified names: " | ||
| f"before={before_name}, after={after_name}" | ||
| ) |
There was a problem hiding this comment.
diff_remote_named_versions() now logs an error when get_remote_resources_by_name() returns empty, but get_remote_resources_by_name() also logs an error for the same failure paths. This will produce duplicated error logs for a single user mistake (unknown name / missing deployment). Consider having only one layer log (e.g., let get_remote_resources_by_name() be silent and raise/return a structured error, or keep logging only at the caller).
| logger.error( | |
| "Could not retrieve resources for one or both specified names: " | |
| f"before={before_name}, after={after_name}" | |
| ) |
src/poly/handlers/sdk.py
Outdated
| # Check if this is a conflict response | ||
| if "conflicts" in response_data or "hasConflicts" in response_data: | ||
| logger.warning( | ||
| logger.error( |
There was a problem hiding this comment.
This conflict path is also logged at a higher level (e.g., SyncClientHandler.merge_branch() logs an error when hasConflicts is true). With this changed to logger.error, a single merge-with-conflicts event will likely emit duplicate error-level logs. Consider logging conflicts at only one layer (or keeping this as warning/info and letting the caller handle user-facing severity).
| logger.error( | |
| logger.warning( |
src/poly/cli.py
Outdated
| warning( | ||
| f"Current branch no longer exists in Agent Studio. Created and switched to new branch '{new_branch_name}'." |
There was a problem hiding this comment.
The user-facing warning shown after push_project() triggers any time project.branch_id changes, including the normal/expected case of pushing from main (where a new branch is created). In that common scenario, the message is misleading because it claims the current branch no longer exists. Consider distinguishing between (a) switching because the local branch ID was invalid/deleted vs (b) creating a new draft branch from main, and adjust the wording and/or severity accordingly (e.g., only warn for the invalid/deleted case).
| warning( | |
| f"Current branch no longer exists in Agent Studio. Created and switched to new branch '{new_branch_name}'." | |
| info( | |
| f"Created and switched to new branch '{new_branch_name}' in Agent Studio." |
Coverage Report
Changed file coverage
|
Summary
Display new branch name in CLI when the tool switches branch
Motivation
On push when creating a new branch, users would be shown branch ID not new branch name
Changes
sdk-userTest strategy
poly <command>)Checklist
ruff check .andruff format --check .passpytestpassespolyCLI interface (or migration path documented)Screenshots / Logs