diff --git a/AGENTS.md b/AGENTS.md index 2377a52..75cad7e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -44,7 +44,7 @@ gh pr merge --rebase --delete-branch ### Core Layer (`core.py`) - `Phabfive` class: central configuration and API client management -- Loads config from environment variables or `~/.config/phabfive.yaml` +- Loads config from `.arcconfig`, `~/.arcrc`, `~/.config/phabfive.yaml`, and environment variables - Uses the `phabricator` library for Conduit API calls - Output formatting: supports `rich` (terminal), `yaml`, and `strict` (machine-readable) modes @@ -62,7 +62,18 @@ Complex features use a consistent subpackage structure: - `transitions/` - state machine for task status/priority/column changes ### Configuration -Required: `PHAB_TOKEN` and `PHAB_URL` (via environment or config file) + +Required: `PHAB_TOKEN` and `PHAB_URL` + +Config precedence (later overrides earlier): +1. Hard-coded defaults +2. `/etc/phabfive.yaml` +3. `/etc/phabfive.d/*.yaml` +4. `~/.config/phabfive.yaml` (PHAB_URL/PHAB_TOKEN deprecated here, use for PHAB_SPACE/PHAB_FALLBACK/PHABFIVE_DEBUG) +5. `~/.config/phabfive.d/*.yaml` +6. `.arcconfig` in git root (provides PHAB_URL from `phabricator.uri`) +7. `~/.arcrc` (provides PHAB_TOKEN for matched URL) +8. Environment variables ## AI Agent Usage diff --git a/phabfive/constants.py b/phabfive/constants.py index 0091d1c..fd0345c 100644 --- a/phabfive/constants.py +++ b/phabfive/constants.py @@ -77,8 +77,8 @@ class LogLevel(str, Enum): } VALIDATION_HINTS = {"PHAB_URL": "example: https://we.phorge.it/api/"} MISSING_CONFIG_HINTS = { - "PHAB_TOKEN": "example: export PHAB_TOKEN=cli-RANDOMRANDOMRANDOMRANDOMRAND", - "PHAB_URL": "example: echo PHAB_URL: https://we.phorge.it/api/ >> ~/.config/phabfive.yaml", + "PHAB_TOKEN": "add token to ~/.arcrc or run: phabfive user setup", + "PHAB_URL": 'create .arcconfig with: {"phabricator.uri": "https://we.phorge.it/"} or run: phabfive user setup', } PRIORITY_DEFAULT = "normal" diff --git a/phabfive/core.py b/phabfive/core.py index 19816ac..26b8745 100644 --- a/phabfive/core.py +++ b/phabfive/core.py @@ -8,6 +8,7 @@ import os import re import stat +import sys from urllib.parse import urlparse # phabfive imports @@ -437,6 +438,64 @@ def _load_arcrc(self, current_conf): return result + def _load_arcconfig(self): + """ + Load PHAB_URL from .arcconfig in the git repository root. + + Walks up from the current working directory looking for a .git directory, + then reads .arcconfig in that directory. Extracts `phabricator.uri` and + normalizes it to an API URL. + + Returns + ------- + dict + Configuration dict with PHAB_URL if found, empty dict otherwise + """ + # Walk up from cwd looking for .git directory + current = os.getcwd() + git_root = None + + while True: + if os.path.isdir(os.path.join(current, ".git")): + git_root = current + break + parent = os.path.dirname(current) + if parent == current: + break + current = parent + + if git_root is None: + log.debug("Not in a git repository, skipping .arcconfig") + return {} + + arcconfig_path = os.path.join(git_root, ".arcconfig") + + if not os.path.exists(arcconfig_path): + log.debug(f"No .arcconfig found at {arcconfig_path}") + return {} + + log.debug(f"Loading configuration from {arcconfig_path}") + + try: + with open(arcconfig_path, "r") as f: + arcconfig_data = json.load(f) + except json.JSONDecodeError as e: + log.warning(f"Failed to parse {arcconfig_path}: {e}") + return {} + except IOError as e: + log.warning(f"Failed to read {arcconfig_path}: {e}") + return {} + + uri = arcconfig_data.get("phabricator.uri") + + if not uri: + log.debug("No phabricator.uri found in .arcconfig") + return {} + + normalized = self._normalize_url(uri) + log.debug(f"Using PHAB_URL from .arcconfig: {normalized}") + return {"PHAB_URL": normalized} + def load_config(self): """ Load configuration from configuration files and environment variables. @@ -448,8 +507,9 @@ def load_config(self): 3. `/etc/phabfive.d/*.yaml` 4. `~/.config/phabfive.yaml` 5. `~/.config/phabfive.d/*.yaml` - 6. `~/.arcrc` (Arcanist configuration) - 7. environment variables + 6. `.arcconfig` in git root + 7. `~/.arcrc` (Arcanist configuration) + 8. environment variables """ environ = os.environ.copy() @@ -487,6 +547,10 @@ def load_config(self): }, ) + # Track config state before user yaml files to detect deprecated keys + pre_user_url = conf.get("PHAB_URL", "") + pre_user_token = conf.get("PHAB_TOKEN", "") + user_conf_file = os.path.join(f"{appdirs.user_config_dir('phabfive')}.yaml") log.debug(f"Loading configuration file: {user_conf_file}") self._check_secure_permissions(user_conf_file) @@ -520,8 +584,33 @@ def load_config(self): }, ) + # Warn if PHAB_URL or PHAB_TOKEN were set from user yaml config files + deprecated_keys = [] + if conf.get("PHAB_URL", "") != pre_user_url and conf.get("PHAB_URL", ""): + deprecated_keys.append("PHAB_URL") + if conf.get("PHAB_TOKEN", "") != pre_user_token and conf.get("PHAB_TOKEN", ""): + deprecated_keys.append("PHAB_TOKEN") + if deprecated_keys: + keys_str = "/".join(deprecated_keys) + print( + f"WARNING: ~/.config/phabfive.yaml contains {keys_str} which is deprecated. " + "Migrate credentials to ~/.arcrc using: phabfive user setup", + file=sys.stderr, + ) + + # Load from .arcconfig in git repository root + arcconfig_conf = self._load_arcconfig() + if arcconfig_conf: + log.debug("Merging configuration from .arcconfig") + anyconfig.merge(conf, arcconfig_conf) + # Load from Arcanist .arcrc file (supports single or multiple hosts) - arcrc_conf = self._load_arcrc(conf) + # Include PHAB_URL from environment so .arcrc can match the right host + # even though env vars are formally merged later + arcrc_lookup_conf = dict(conf) + if "PHAB_URL" in environ and environ["PHAB_URL"]: + arcrc_lookup_conf["PHAB_URL"] = environ["PHAB_URL"] + arcrc_conf = self._load_arcrc(arcrc_lookup_conf) if arcrc_conf: log.debug("Merging configuration from ~/.arcrc") anyconfig.merge(conf, arcrc_conf) diff --git a/phabfive/setup.py b/phabfive/setup.py index c82b2b0..abcb78f 100644 --- a/phabfive/setup.py +++ b/phabfive/setup.py @@ -1,6 +1,7 @@ # -*- coding: utf-8 -*- """Interactive first-run configuration setup for phabfive.""" +import json import logging import os import re @@ -9,7 +10,6 @@ from phabricator import Phabricator, APIError from rich.console import Console from rich.prompt import Prompt, Confirm -from ruamel.yaml import YAML from phabfive.constants import VALIDATORS @@ -92,7 +92,7 @@ def _read_password_with_dots(prompt: str = "") -> str: class SetupWizard: """Interactive setup wizard for phabfive configuration.""" - CONFIG_PATH = os.path.expanduser("~/.config/phabfive.yaml") + CONFIG_PATH = os.path.expanduser("~/.arcrc") def __init__(self): self.console = Console() @@ -278,29 +278,26 @@ def _verify_connection(self) -> bool: return False def _save_config(self) -> bool: - """Save configuration to file with secure permissions.""" + """Save credentials to ~/.arcrc in Arcanist-compatible JSON format.""" try: - # Ensure ~/.config directory exists - config_dir = os.path.dirname(self.CONFIG_PATH) - os.makedirs(config_dir, mode=0o700, exist_ok=True) - - # Load existing config or create new - yaml = YAML() - yaml.default_flow_style = False - + # Load existing .arcrc or create new if os.path.exists(self.CONFIG_PATH): with open(self.CONFIG_PATH, "r") as f: - config = yaml.load(f) or {} + arcrc = json.load(f) else: - config = {} + arcrc = {} + + # Ensure hosts key exists + if "hosts" not in arcrc: + arcrc["hosts"] = {} - # Update with new values - config["PHAB_URL"] = self.phab_url - config["PHAB_TOKEN"] = self.phab_token + # Add/update the host entry + arcrc["hosts"][self.phab_url] = {"token": self.phab_token} - # Write config file + # Write JSON with indentation with open(self.CONFIG_PATH, "w") as f: - yaml.dump(config, f) + json.dump(arcrc, f, indent=2) + f.write("\n") # Set secure permissions (Unix only) if os.name != "nt": @@ -332,9 +329,105 @@ def _normalize_url(self, url: str) -> str: return url +def _find_git_root(): + """Find the git repository root by walking up from cwd. + + Returns: + str or None: Path to git root, or None if not in a git repo + """ + current = os.getcwd() + while True: + if os.path.isdir(os.path.join(current, ".git")): + return current + parent = os.path.dirname(current) + if parent == current: + return None + current = parent + + +def _setup_arcconfig(console) -> bool: + """Interactive setup to create .arcconfig in the git repo root. + + Args: + console: Rich Console instance for output + + Returns: + bool: True if .arcconfig was created successfully, False otherwise + """ + git_root = _find_git_root() + + if git_root is None: + console.print( + "[yellow]Not inside a git repository. Cannot create .arcconfig.[/yellow]\n" + ) + console.print("Either:") + console.print(" 1. Run phabfive from inside a git repository") + console.print(" 2. Set PHAB_URL environment variable") + console.print( + " 3. Run [bold]phabfive user setup[/bold] to configure ~/.arcrc\n" + ) + return False + + arcconfig_path = os.path.join(git_root, ".arcconfig") + + if os.path.exists(arcconfig_path): + console.print( + f"[yellow].arcconfig already exists at {arcconfig_path}[/yellow]\n" + ) + try: + with open(arcconfig_path, "r") as f: + data = json.load(f) + uri = data.get("phabricator.uri", "(not set)") + console.print(f" Current phabricator.uri: [bold]{uri}[/bold]\n") + except (json.JSONDecodeError, IOError): + pass + + if not Confirm.ask("Do you want to overwrite it?", default=False): + return False + + console.print("[bold]Create .arcconfig[/bold]") + console.print(f"This will create .arcconfig in: {git_root}\n") + + while True: + url = Prompt.ask( + "Enter your Phabricator URL (e.g., https://phorge.example.com)" + ) + + if not url: + console.print("[red]URL cannot be empty[/red]") + continue + + # Strip trailing slashes and /api/ suffix for .arcconfig + # .arcconfig stores the base URL, not the API URL + url = url.rstrip("/") + if url.endswith("/api"): + url = url[:-4].rstrip("/") + + console.print(f"[green]> Using URL: {url}[/green]\n") + break + + try: + arcconfig_data = {"phabricator.uri": url + "/"} + with open(arcconfig_path, "w") as f: + json.dump(arcconfig_data, f, indent=2) + f.write("\n") + + console.print(f"[green].arcconfig created at {arcconfig_path}[/green]") + console.print("[dim]Remember to commit .arcconfig to your repository.[/dim]\n") + return True + + except Exception as e: + console.print(f"[red]Failed to create .arcconfig: {e}[/red]") + return False + + def offer_setup_on_error(error_message: str) -> bool: """Offer to run setup when configuration error occurs. + Routes to the appropriate wizard based on what's missing: + - Missing PHAB_URL: offer to create .arcconfig + - Missing PHAB_TOKEN or other errors: offer full setup wizard (~/.arcrc) + Args: error_message: The configuration error message to display @@ -350,8 +443,17 @@ def offer_setup_on_error(error_message: str) -> bool: console = Console() console.print(f"\n[red]ERROR: {error_message}[/red]\n") - if Confirm.ask("Would you like to run interactive setup now?", default=True): - wizard = SetupWizard() - return wizard.run() + is_url_error = "PHAB_URL" in error_message and "PHAB_TOKEN" not in error_message + + if is_url_error: + if Confirm.ask( + "Would you like to create .arcconfig for this repository?", + default=True, + ): + return _setup_arcconfig(console) + else: + if Confirm.ask("Would you like to run interactive setup now?", default=True): + wizard = SetupWizard() + return wizard.run() return False diff --git a/tests/test_arcconfig.py b/tests/test_arcconfig.py new file mode 100644 index 0000000..45060bd --- /dev/null +++ b/tests/test_arcconfig.py @@ -0,0 +1,171 @@ +# -*- coding: utf-8 -*- + +"""Tests for .arcconfig file support (issue #171).""" + +import json +from unittest import mock + +from phabfive.core import Phabfive + + +class TestLoadArcconfig: + """Tests for the _load_arcconfig method.""" + + def setup_method(self): + """Create a Phabfive instance without initialization for testing.""" + self.phabfive = object.__new__(Phabfive) + + def test_arcconfig_found_with_valid_uri(self, tmp_path): + """Test .arcconfig found at git root with valid phabricator.uri.""" + # Create fake git repo structure + git_dir = tmp_path / ".git" + git_dir.mkdir() + arcconfig = tmp_path / ".arcconfig" + arcconfig.write_text( + json.dumps({"phabricator.uri": "https://phorge.example.com/"}) + ) + + with mock.patch("os.getcwd", return_value=str(tmp_path)): + result = self.phabfive._load_arcconfig() + + assert result == {"PHAB_URL": "https://phorge.example.com/api/"} + + def test_arcconfig_missing(self, tmp_path): + """Test .arcconfig missing returns empty dict.""" + # Create git repo without .arcconfig + git_dir = tmp_path / ".git" + git_dir.mkdir() + + with mock.patch("os.getcwd", return_value=str(tmp_path)): + result = self.phabfive._load_arcconfig() + + assert result == {} + + def test_no_phabricator_uri_key(self, tmp_path): + """Test .arcconfig with no phabricator.uri key returns empty dict.""" + git_dir = tmp_path / ".git" + git_dir.mkdir() + arcconfig = tmp_path / ".arcconfig" + arcconfig.write_text(json.dumps({"other.key": "value"})) + + with mock.patch("os.getcwd", return_value=str(tmp_path)): + result = self.phabfive._load_arcconfig() + + assert result == {} + + def test_invalid_json(self, tmp_path): + """Test .arcconfig with invalid JSON returns empty dict.""" + git_dir = tmp_path / ".git" + git_dir.mkdir() + arcconfig = tmp_path / ".arcconfig" + arcconfig.write_text("{ invalid json }") + + with mock.patch("os.getcwd", return_value=str(tmp_path)): + result = self.phabfive._load_arcconfig() + + assert result == {} + + def test_not_in_git_repo(self, tmp_path): + """Test not in a git repo returns empty dict.""" + # No .git directory anywhere + subdir = tmp_path / "subdir" + subdir.mkdir() + + with mock.patch("os.getcwd", return_value=str(subdir)): + result = self.phabfive._load_arcconfig() + + assert result == {} + + def test_git_root_found_in_parent(self, tmp_path): + """Test .arcconfig found when .git is in parent directory.""" + git_dir = tmp_path / ".git" + git_dir.mkdir() + arcconfig = tmp_path / ".arcconfig" + arcconfig.write_text( + json.dumps({"phabricator.uri": "https://phorge.example.com/"}) + ) + + subdir = tmp_path / "src" / "deep" + subdir.mkdir(parents=True) + + with mock.patch("os.getcwd", return_value=str(subdir)): + result = self.phabfive._load_arcconfig() + + assert result == {"PHAB_URL": "https://phorge.example.com/api/"} + + +class TestArconfigUrlNormalization: + """Tests for URL normalization from .arcconfig.""" + + def setup_method(self): + self.phabfive = object.__new__(Phabfive) + + def test_url_without_api_suffix(self, tmp_path): + """Test URL without /api/ gets normalized.""" + git_dir = tmp_path / ".git" + git_dir.mkdir() + arcconfig = tmp_path / ".arcconfig" + arcconfig.write_text( + json.dumps({"phabricator.uri": "https://phorge.example.com"}) + ) + + with mock.patch("os.getcwd", return_value=str(tmp_path)): + result = self.phabfive._load_arcconfig() + + assert result["PHAB_URL"] == "https://phorge.example.com/api/" + + def test_url_with_trailing_slash(self, tmp_path): + """Test URL with trailing slash gets normalized.""" + git_dir = tmp_path / ".git" + git_dir.mkdir() + arcconfig = tmp_path / ".arcconfig" + arcconfig.write_text( + json.dumps({"phabricator.uri": "https://phorge.example.com/"}) + ) + + with mock.patch("os.getcwd", return_value=str(tmp_path)): + result = self.phabfive._load_arcconfig() + + assert result["PHAB_URL"] == "https://phorge.example.com/api/" + + def test_url_with_api_already(self, tmp_path): + """Test URL already ending with /api/ is preserved.""" + git_dir = tmp_path / ".git" + git_dir.mkdir() + arcconfig = tmp_path / ".arcconfig" + arcconfig.write_text( + json.dumps({"phabricator.uri": "https://phorge.example.com/api/"}) + ) + + with mock.patch("os.getcwd", return_value=str(tmp_path)): + result = self.phabfive._load_arcconfig() + + assert result["PHAB_URL"] == "https://phorge.example.com/api/" + + def test_url_with_port(self, tmp_path): + """Test URL with port gets normalized.""" + git_dir = tmp_path / ".git" + git_dir.mkdir() + arcconfig = tmp_path / ".arcconfig" + arcconfig.write_text( + json.dumps({"phabricator.uri": "https://phorge.example.com:8443"}) + ) + + with mock.patch("os.getcwd", return_value=str(tmp_path)): + result = self.phabfive._load_arcconfig() + + assert result["PHAB_URL"] == "https://phorge.example.com:8443/api/" + + def test_url_with_api_no_trailing_slash(self, tmp_path): + """Test URL ending with /api (no trailing slash) gets normalized.""" + git_dir = tmp_path / ".git" + git_dir.mkdir() + arcconfig = tmp_path / ".arcconfig" + arcconfig.write_text( + json.dumps({"phabricator.uri": "https://phorge.example.com/api"}) + ) + + with mock.patch("os.getcwd", return_value=str(tmp_path)): + result = self.phabfive._load_arcconfig() + + assert result["PHAB_URL"] == "https://phorge.example.com/api/" diff --git a/tests/test_arcrc.py b/tests/test_arcrc.py index 8d28c8c..d3c66df 100644 --- a/tests/test_arcrc.py +++ b/tests/test_arcrc.py @@ -532,3 +532,79 @@ def test_legacy_cert_format_no_token(self, tmp_path): # Should provide URL but not token (cert-based auth not supported) assert result["PHAB_URL"] == "https://phorge.example.com/api/" assert "PHAB_TOKEN" not in result + + +class TestDeprecationWarning: + """Tests for deprecation warning when PHAB_URL/PHAB_TOKEN in phabfive.yaml.""" + + def setup_method(self): + self.phabfive = object.__new__(Phabfive) + + def test_deprecation_warning_phab_url_in_yaml(self, tmp_path, capsys): + """Test that PHAB_URL in user yaml config prints warning to stderr.""" + user_conf = tmp_path / "phabfive.yaml" + user_conf.write_text("PHAB_URL: https://phorge.example.com/api/\n") + os.chmod(user_conf, 0o600) + + with ( + mock.patch("appdirs.site_config_dir", return_value=str(tmp_path / "site")), + mock.patch( + "appdirs.user_config_dir", + return_value=str(user_conf).replace(".yaml", ""), + ), + mock.patch.object( + os.path, "expanduser", return_value=str(tmp_path / ".arcrc") + ), + mock.patch.dict(os.environ, {}, clear=True), + ): + self.phabfive.load_config() + + captured = capsys.readouterr() + assert "PHAB_URL" in captured.err + assert "deprecated" in captured.err + assert "phabfive user setup" in captured.err + + def test_deprecation_warning_phab_token_in_yaml(self, tmp_path, capsys): + """Test that PHAB_TOKEN in user yaml config prints warning to stderr.""" + user_conf = tmp_path / "phabfive.yaml" + user_conf.write_text("PHAB_TOKEN: cli-abcdefghijklmnopqrstuvwxyz12\n") + os.chmod(user_conf, 0o600) + + with ( + mock.patch("appdirs.site_config_dir", return_value=str(tmp_path / "site")), + mock.patch( + "appdirs.user_config_dir", + return_value=str(user_conf).replace(".yaml", ""), + ), + mock.patch.object( + os.path, "expanduser", return_value=str(tmp_path / ".arcrc") + ), + mock.patch.dict(os.environ, {}, clear=True), + ): + self.phabfive.load_config() + + captured = capsys.readouterr() + assert "PHAB_TOKEN" in captured.err + assert "deprecated" in captured.err + + def test_no_deprecation_warning_without_credentials(self, tmp_path, capsys): + """Test no warning when yaml has only non-credential keys.""" + user_conf = tmp_path / "phabfive.yaml" + user_conf.write_text("PHAB_SPACE: S42\n") + os.chmod(user_conf, 0o600) + + with ( + mock.patch("appdirs.site_config_dir", return_value=str(tmp_path / "site")), + mock.patch( + "appdirs.user_config_dir", + return_value=str(user_conf).replace(".yaml", ""), + ), + mock.patch.object( + os.path, "expanduser", return_value=str(tmp_path / ".arcrc") + ), + mock.patch.dict(os.environ, {}, clear=True), + ): + self.phabfive.load_config() + + captured = capsys.readouterr() + assert "deprecated" not in captured.err diff --git a/tests/test_setup.py b/tests/test_setup.py index 7542065..5c8fcba 100644 --- a/tests/test_setup.py +++ b/tests/test_setup.py @@ -7,7 +7,12 @@ import pytest -from phabfive.setup import SetupWizard, offer_setup_on_error +from phabfive.setup import ( + SetupWizard, + offer_setup_on_error, + _find_git_root, + _setup_arcconfig, +) # Skip marker for tests that require Unix-style permissions @@ -87,16 +92,128 @@ def test_interactive_declines_returns_false(self): result = offer_setup_on_error("Test error") assert result is False + def test_phab_url_error_offers_arcconfig(self): + """Test that PHAB_URL error offers .arcconfig setup, not full wizard.""" + with ( + mock.patch("sys.stdin.isatty", return_value=True), + mock.patch( + "phabfive.setup.Confirm.ask", return_value=False + ) as mock_confirm, + mock.patch("phabfive.setup.Console"), + ): + offer_setup_on_error("PHAB_URL is not configured") + + # Should ask about .arcconfig, not generic setup + call_args = mock_confirm.call_args[0][0] + assert ".arcconfig" in call_args + + def test_phab_token_error_offers_full_setup(self): + """Test that PHAB_TOKEN error offers full setup wizard.""" + with ( + mock.patch("sys.stdin.isatty", return_value=True), + mock.patch( + "phabfive.setup.Confirm.ask", return_value=False + ) as mock_confirm, + mock.patch("phabfive.setup.Console"), + ): + offer_setup_on_error("PHAB_TOKEN is not configured") + + call_args = mock_confirm.call_args[0][0] + assert "interactive setup" in call_args + + +class TestSetupArcconfig: + """Tests for .arcconfig creation.""" + + def test_find_git_root(self, tmp_path): + """Test finding git root from subdirectory.""" + git_dir = tmp_path / ".git" + git_dir.mkdir() + subdir = tmp_path / "src" / "deep" + subdir.mkdir(parents=True) + + with mock.patch("os.getcwd", return_value=str(subdir)): + result = _find_git_root() + + assert result == str(tmp_path) + + def test_find_git_root_not_in_repo(self, tmp_path): + """Test returns None when not in a git repo.""" + with mock.patch("os.getcwd", return_value=str(tmp_path)): + result = _find_git_root() + + assert result is None + + def test_setup_arcconfig_creates_file(self, tmp_path): + """Test that .arcconfig is created with correct content.""" + git_dir = tmp_path / ".git" + git_dir.mkdir() + + console = mock.MagicMock() + + with ( + mock.patch("os.getcwd", return_value=str(tmp_path)), + mock.patch( + "phabfive.setup.Prompt.ask", + return_value="https://phorge.example.com", + ), + ): + result = _setup_arcconfig(console) + + assert result is True + + import json + + arcconfig_path = tmp_path / ".arcconfig" + assert arcconfig_path.exists() + + with open(arcconfig_path) as f: + data = json.load(f) + + assert data["phabricator.uri"] == "https://phorge.example.com/" + + def test_setup_arcconfig_strips_api_suffix(self, tmp_path): + """Test that /api/ suffix is stripped from URL for .arcconfig.""" + git_dir = tmp_path / ".git" + git_dir.mkdir() + + console = mock.MagicMock() + + with ( + mock.patch("os.getcwd", return_value=str(tmp_path)), + mock.patch( + "phabfive.setup.Prompt.ask", + return_value="https://phorge.example.com/api/", + ), + ): + _setup_arcconfig(console) + + import json + + with open(tmp_path / ".arcconfig") as f: + data = json.load(f) + + assert data["phabricator.uri"] == "https://phorge.example.com/" + + def test_setup_arcconfig_not_in_git_repo(self, tmp_path): + """Test that setup fails gracefully outside a git repo.""" + console = mock.MagicMock() + + with mock.patch("os.getcwd", return_value=str(tmp_path)): + result = _setup_arcconfig(console) + + assert result is False + class TestSetupWizardSaveConfig: - """Tests for configuration saving.""" + """Tests for configuration saving to ~/.arcrc.""" def setup_method(self): self.wizard = SetupWizard() def test_save_creates_file(self, tmp_path): - """Test that save creates config file.""" - config_path = tmp_path / "phabfive.yaml" + """Test that save creates .arcrc file.""" + config_path = tmp_path / ".arcrc" self.wizard.CONFIG_PATH = str(config_path) self.wizard.phab_url = "https://phorge.example.com/api/" @@ -107,9 +224,9 @@ def test_save_creates_file(self, tmp_path): assert result is True assert config_path.exists() - def test_save_writes_correct_content(self, tmp_path): - """Test that save writes correct YAML content.""" - config_path = tmp_path / "phabfive.yaml" + def test_save_writes_arcrc_json_format(self, tmp_path): + """Test that save writes correct .arcrc JSON format.""" + config_path = tmp_path / ".arcrc" self.wizard.CONFIG_PATH = str(config_path) self.wizard.phab_url = "https://phorge.example.com/api/" @@ -117,14 +234,22 @@ def test_save_writes_correct_content(self, tmp_path): self.wizard._save_config() - content = config_path.read_text() - assert "PHAB_URL: https://phorge.example.com/api/" in content - assert "PHAB_TOKEN: cli-abcdefghijklmnopqrstuvwxyz12" in content + import json + + with open(config_path) as f: + data = json.load(f) + + assert "hosts" in data + assert "https://phorge.example.com/api/" in data["hosts"] + assert ( + data["hosts"]["https://phorge.example.com/api/"]["token"] + == "cli-abcdefghijklmnopqrstuvwxyz12" + ) @skip_on_windows def test_save_creates_secure_permissions(self, tmp_path): - """Test that saved config file has secure permissions.""" - config_path = tmp_path / "phabfive.yaml" + """Test that saved .arcrc file has secure permissions.""" + config_path = tmp_path / ".arcrc" self.wizard.CONFIG_PATH = str(config_path) self.wizard.phab_url = "https://phorge.example.com/api/" @@ -136,10 +261,19 @@ def test_save_creates_secure_permissions(self, tmp_path): mode = os.stat(config_path).st_mode & 0o777 assert mode == 0o600 - def test_save_preserves_existing_config(self, tmp_path): - """Test that save preserves existing configuration values.""" - config_path = tmp_path / "phabfive.yaml" - config_path.write_text("PHAB_SPACE: S42\n") + def test_save_merges_into_existing_arcrc(self, tmp_path): + """Test that save merges into existing .arcrc with other hosts.""" + import json + + config_path = tmp_path / ".arcrc" + existing = { + "hosts": { + "https://other.example.com/api/": { + "token": "cli-existingtokenexistingtoken12" + } + } + } + config_path.write_text(json.dumps(existing)) self.wizard.CONFIG_PATH = str(config_path) self.wizard.phab_url = "https://phorge.example.com/api/" @@ -147,23 +281,48 @@ def test_save_preserves_existing_config(self, tmp_path): self.wizard._save_config() - content = config_path.read_text() - assert "PHAB_SPACE: S42" in content - assert "PHAB_URL: https://phorge.example.com/api/" in content - assert "PHAB_TOKEN: cli-abcdefghijklmnopqrstuvwxyz12" in content - - def test_save_creates_parent_directory(self, tmp_path): - """Test that save creates parent directory if needed.""" - config_path = tmp_path / "subdir" / "phabfive.yaml" + with open(config_path) as f: + data = json.load(f) + + # Both hosts should be present + assert "https://other.example.com/api/" in data["hosts"] + assert ( + data["hosts"]["https://other.example.com/api/"]["token"] + == "cli-existingtokenexistingtoken12" + ) + assert "https://phorge.example.com/api/" in data["hosts"] + assert ( + data["hosts"]["https://phorge.example.com/api/"]["token"] + == "cli-abcdefghijklmnopqrstuvwxyz12" + ) + + def test_save_updates_existing_host(self, tmp_path): + """Test that save updates token for existing host.""" + import json + + config_path = tmp_path / ".arcrc" + existing = { + "hosts": { + "https://phorge.example.com/api/": { + "token": "cli-oldtokenoldtokenoldtokenold1" + } + } + } + config_path.write_text(json.dumps(existing)) self.wizard.CONFIG_PATH = str(config_path) self.wizard.phab_url = "https://phorge.example.com/api/" - self.wizard.phab_token = "cli-abcdefghijklmnopqrstuvwxyz12" + self.wizard.phab_token = "cli-newtokennewtokennewtokennew1" - result = self.wizard._save_config() + self.wizard._save_config() - assert result is True - assert config_path.exists() + with open(config_path) as f: + data = json.load(f) + + assert ( + data["hosts"]["https://phorge.example.com/api/"]["token"] + == "cli-newtokennewtokennewtokennew1" + ) class TestSetupWizardVerifyConnection: