Skip to content

Conversation

@jayhack
Copy link
Contributor

@jayhack jayhack commented Aug 15, 2025

Motivation

Content

Testing

Please check the following before marking your PR as ready for review

  • I have added tests for my changes
  • I have updated the documentation or added new documentation as needed

@jayhack jayhack requested review from a team and codegen-team as code owners August 15, 2025 00:49
@codecov
Copy link

codecov bot commented Aug 15, 2025

Codecov Report

❌ Patch coverage is 20.72072% with 88 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/codegen/cli/commands/agents/main.py 18.07% 68 Missing ⚠️
src/codegen/cli/commands/integrations/main.py 16.66% 10 Missing ⚠️
src/codegen/cli/commands/tools/main.py 10.00% 9 Missing ⚠️
src/codegen/cli/commands/login/main.py 0.00% 1 Missing ⚠️

@jayhack jayhack merged commit 91fcbce into develop Aug 15, 2025
18 checks passed
@jayhack jayhack deleted the jay/cli-tune-up branch August 15, 2025 00:54

# Base directories
CONFIG_DIR = Path("~/.config/codegen-sh").expanduser()
CONFIG_DIR = Path("~/.codegen").expanduser()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Logic bug]: CONFIG_DIR path change breaks consistency
Other modules (e.g. src/codegen/configs/constants.py) still reference the old ~/.config/codegen-sh directory, so credentials/config written here will no longer be found elsewhere.

Suggested change
CONFIG_DIR = Path("~/.codegen").expanduser()
# Re-align with the existing global constants – keep the old location or update all usages consistently
CONFIG_DIR = Path("~/.config/codegen-sh").expanduser()

rich.print(f"[green]✓ Stored token to:[/green] {token_manager.token_file}")
rich.print("[cyan]📊 Hey![/cyan] We collect anonymous usage data to improve your experience 🔒")
rich.print("To opt out, set [green]telemetry_enabled = false[/green] in [cyan]~/.config/codegen-sh/analytics.json[/cyan] ✨")
return token
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Logic bug]: Removed consent message may hide telemetry notice
Deleting the opt-out instructions lines means users are no longer told how to disable telemetry.

Suggested change
return token
rich.print("[cyan]📊 Hey![/cyan] We collect anonymous usage data to improve your experience 🔒")
rich.print("To opt out, set [green]telemetry_enabled = false[/green] in [cyan]~/.config/codegen-sh/analytics.json[/cyan] ✨")

# Import config command (still a Typer app)
from codegen.cli.commands.config.main import config_command
from codegen.cli.commands.init.main import init
from codegen.cli.commands.integrations.main import integrations_app
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Syntax error]: extraneous import removed in diff but still referenced
codegen/cli/cli.py still imports codegen.cli.commands.mcp.main which has been deleted, causing ImportError at runtime.

Suggested change
from codegen.cli.commands.integrations.main import integrations_app
# from codegen.cli.commands.mcp.main import mcp # removed command

main = typer.Typer(name="codegen", help="Codegen - the Operating System for Code Agents.", rich_markup_mode="rich")

# Add individual commands to the main app
main.command("claude", help="Run Claude Code with OpenTelemetry monitoring and logging.")(claude)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Logic error]: Command registration for removed mcp still present
Since mcp function is no longer imported, these lines will raise NameError when CLI module is imported.

Suggested change
main.command("claude", help="Run Claude Code with OpenTelemetry monitoring and logging.")(claude)
# main.command("mcp", help="Start the Codegen MCP server.")(mcp) # command removed


# Should contain basic help text
assert "Codegen CLI - Transform your code with AI" in result.stdout
assert "Commands" in result.stdout
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Test failure]: Unit tests still assert old help string
Help message string changed; update tests or revert help text to keep tests passing.

Suggested change
assert "Commands" in result.stdout
assert "Codegen - the Operating System for Code Agents" in result.stdout

rich.print("[cyan]📊 Hey![/cyan] We collect anonymous usage data to improve your experience 🔒")
rich.print("To opt out, set [green]telemetry_enabled = false[/green] in [cyan]~/.config/codegen-sh/analytics.json[/cyan] ✨")
return token
except AuthError as e:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Logic error]: Telemetry opt-out path hard-coded to old config location
If CONFIG_DIR path is changed, this message must also reflect new location.

Suggested change
except AuthError as e:
rich.print("To opt out, set [green]telemetry_enabled = false[/green] in [cyan]~/.codegen/analytics.json[/cyan] ✨")

@codegen-sh
Copy link
Contributor

codegen-sh bot commented Aug 15, 2025

Found 7 issues. Please review my inline comments above.

🔍 View my analysis

@github-actions
Copy link
Contributor

🎉 This PR is included in version 0.56.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants