fix: improve API key error messages and add tests for credential checks

[n1colasf]

Description

Improves the error messages shown when API credentials are missing across chat, importagent, and migrate management commands, and in the core authentication client. Previous messages were vague and only referenced COGSOL_API_BASE; the new messages correctly check for COGSOL_API_KEY (or COGSOL_AUTH_CLIENT_ID as fallback) and guide the user on how to obtain and configure their credentials.

Type of Change

• Bug fix (non-breaking change that fixes an issue)

Related Issues

Fixes #40

Changes Made

• Replaced generic COGSOL_API_BASE is required messages with descriptive errors that check for the actual credential variables (COGSOL_API_KEY / COGSOL_AUTH_CLIENT_ID)
• Added step-by-step instructions pointing to https://onboarding.cogsol.ai in the error output for chat, importagent, and migrate commands
• Improved the COGSOL_AUTH_SECRET error in cogsol/core/api.py with the same onboarding URL guidance
• Added tests/test_api_key_errors.py with unit tests covering all missing-credential scenarios for the three commands and the API client

Testing Done

• Unit tests pass locally
• Added new tests for new functionality
• Tested manually

[MicaPerdomo]

Review: fix: improve API key error messages and add tests for credential checks

1. PR Summary

• What it changes: Replaces not api_base checks with not api_key and not COGSOL_AUTH_CLIENT_ID checks in all 3 commands (chat, migrate, importagent), improves error messages with onboarding instructions, and adds tests.
• What it solves: Issue #40 — error messages when credentials were missing were confusing and referenced COGSOL_API_BASE which is no longer required.
• Areas affected: 3 management commands, CogSolClient._refresh_bearer_token, README, new tests.
• Risk level: Medium

2. Findings

Important — Check semantics change but don't cover both cases

The PR replaces not api_base with not api_key and not COGSOL_AUTH_CLIENT_ID. The old check was dead code (get_cognitive_api_base_url() always returns a string due to its hardcoded fallbacks), so the change improves the situation.

However, api_base is still used after the check (passed to CogSolClient). If someone explicitly set COGSOL_API_BASE="", they'd pass the credentials check but get a cryptic error from the client. Unlikely thanks to the fallback, but ideally both validations should be kept.

Important — importagent.py has import os inside handle()

In importagent.py line 294, there's a local import os inside handle(). The other two commands import it at module level. The new code adds os.environ.get("COGSOL_AUTH_CLIENT_ID") which depends on this import. Should be moved to top-level for consistency.

Important — Output inconsistency: print_error vs print

• chat.py uses print_error() (with ANSI formatting)
• migrate.py uses print() with a manual "Error:" prefix
• importagent.py uses print() with a manual "Error:" prefix

Since print_error already exists, all 3 commands should use it for a consistent user experience.

Minor — Tests are overly granular

293 lines of tests for a simple change. Many tests verify the same thing split into individual assertions (one test for "No API credentials found", another for "COGSOL_API_KEY", another for "onboarding URL" — all on the same output from the same command). Could be consolidated: one test per command validating the important parts of the message + one test for the happy path.

Minor — _bare_client() accesses internal attributes

The helper uses CogSolClient.__new__ and sets attributes manually. If __init__ changes, these tests won't break when they should. Fragile but acceptable for this scope.

3. Regression risks

• Low: The old check never triggered, so replacing it doesn't change the actual flow.
• Medium: If any user was running commands without COGSOL_API_KEY or COGSOL_AUTH_CLIENT_ID (only with a custom COGSOL_API_BASE), they'd now be blocked. Worth checking if that use case exists.

4. Missing or recommended tests

• Test for COGSOL_API_KEY="" (empty string) — does it pass the check or not?
• Test for both variables present simultaneously
• Consider consolidating tests to reduce verbosity

5. Final verdict

Approvable with minor changes

The change significantly improves the user experience. Recommended adjustments before merge:

1. Unify print_error vs print across all 3 commands
2. Move the import os in importagent.py to top-level
3. Consider consolidating tests