From bf124b88de5143864ab840a149fb94c301db4db1 Mon Sep 17 00:00:00 2001 From: Aleksandr Filippov Date: Tue, 21 Jul 2026 00:36:53 +0300 Subject: [PATCH] fix: merge files-to-download by final file path and make downloads atomic The inheritance merge keyed files-to-download entries by the raw dest string, so distinct files sharing a directory-form dest (trailing separator) collapsed into one identity and all but the last entry were silently dropped. The merge identity is now the normalized final file path: a directory-form dest is combined with the source filename, derived by a helper shared with the download-time destination resolution so the two stay in lockstep. The shared helper decodes GitLab API raw-file URLs, so a parent entry resolved to such a URL still matches a child override and deploys under its real filename instead of the literal raw. Duplicate identities within one mcp-servers, skills, or files-to-download list now collapse to the last entry with a warning instead of silently. Entries resolving to the same final path are deduplicated before the parallel download phase, and downloads are written atomically via a temp file and rename so a race or interruption can never leave a partially-written file. --- docs/environment-configuration-guide.md | 29 +-- scripts/setup_environment.py | 202 +++++++++++++++-- tests/e2e/fixtures/merge_dirdest_child.yaml | 14 ++ tests/e2e/fixtures/merge_dirdest_parent.yaml | 8 + tests/e2e/test_merge_keys.py | 44 ++++ tests/test_setup_environment.py | 205 +++++++++++++++++- .../test_setup_environment_file_downloads.py | 169 +++++++++++++++ 7 files changed, 633 insertions(+), 38 deletions(-) create mode 100644 tests/e2e/fixtures/merge_dirdest_child.yaml create mode 100644 tests/e2e/fixtures/merge_dirdest_parent.yaml diff --git a/docs/environment-configuration-guide.md b/docs/environment-configuration-guide.md index 5c2f19d..29e78ef 100644 --- a/docs/environment-configuration-guide.md +++ b/docs/environment-configuration-guide.md @@ -461,7 +461,7 @@ Skill configurations. Each skill is a set of files placed in `~/.claude/skills/{ - **Type:** `list[Skill] | None` - **Default:** `[]` -- **Inheritance:** Standard override (child replaces parent) by default. When listed in `merge-keys`: identity-based merge by `name` field. Child skills with the same name replace the parent skill in-position (at the parent's original index). New child skills are appended at the end. +- **Inheritance:** Standard override (child replaces parent) by default. When listed in `merge-keys`: identity-based merge by `name` field. Child skills with the same name replace the parent skill in-position (at the parent's original index). New child skills are appended at the end. Duplicate names within one list are collapsed to the last entry with a warning. - **Skill fields:** - `name` (str, required): Skill identifier - `base` (str, required): Base URL or local path for skill files @@ -488,7 +488,8 @@ Arbitrary files to download during setup. Each entry specifies a source and a de - `dest` (str, required): Destination path (supports `~` expansion) - **Validation:** Paths cannot be empty or contain null bytes - **Security:** Destinations matching sensitive path prefixes (for example, `~/.ssh/`, `~/.bashrc`) are flagged with `[!]` in the installation summary -- **Inheritance:** Standard override (child replaces parent) by default. When listed in `merge-keys`: identity-based merge by `dest` field. Child entries with the same destination replace the parent entry in-position. New child entries are appended at the end. +- **Inheritance:** Standard override (child replaces parent) by default. When listed in `merge-keys`: identity-based merge by the normalized final file path. A `dest` ending with `/` or `\` is a directory destination, so its identity is `dest` plus the source filename (query parameters stripped; for GitLab API raw-file URLs the real filename is decoded from the URL-encoded path segment) -- the trailing-separator form is the only directory form the merge identity recognizes, and the normalization is purely lexical (no filesystem checks). The filename derivation is stable across source resolution, so a parent whose sources were already resolved to absolute URLs matches a child entry written with a relative source. Distinct files sharing a directory dest therefore keep distinct identities and all survive the merge. Child entries whose final file path matches a parent entry replace it in-position; new child entries are appended at the end. Duplicate identities within one list are collapsed to the last entry with a warning. +- **Download deduplication:** After merging, entries that still resolve to the same final file (for example, a directory-form dest and an explicit file dest naming the same path) are deduplicated before the parallel download phase: the last entry wins and each skipped entry is reported with a warning. Files are written atomically (temp file plus rename), so an interrupted or concurrent write never leaves a partially-written destination. - **Example:** ```yaml @@ -504,7 +505,7 @@ MCP (Model Context Protocol) servers extend Claude Code with additional capabili - **Type:** `list[dict] | None` - **Default:** `[]` - **Note:** Each server must have a `name` field -- **Inheritance:** Standard override (child replaces parent) by default. When listed in `merge-keys`: identity-based merge by `name` field. Child servers with the same name replace the parent server in-position (at the parent's original index). New child servers are appended at the end. +- **Inheritance:** Standard override (child replaces parent) by default. When listed in `merge-keys`: identity-based merge by `name` field. Child servers with the same name replace the parent server in-position (at the parent's original index). New child servers are appended at the end. Duplicate names within one list are collapsed to the last entry with a warning. #### HTTP Transport @@ -1391,16 +1392,18 @@ If all levels 2-4 use merge: `[A, B, C, D, E]`. #### Merge Strategies by Key Type -| Type | Keys | Strategy | -|------------------------|-------------------------------------|---------------------------------------------------------------------------------------| -| String list | `agents`, `slash-commands`, `rules` | Concatenate parent + child; deduplicate by string equality; parent items first | -| Named list (by `name`) | `mcp-servers`, `skills` | Identity-based: child overrides parent in-position; new items appended | -| Named list (by `dest`) | `files-to-download` | Identity-based: child overrides parent in-position; new items appended | -| Per-platform dict | `dependencies` | Per-platform sub-key list concatenation with deduplication | -| Composite | `hooks` | `files`: concat + dedup by full path; `events`: concat (no dedup) | -| Deep dict | `global-config` | `deep_merge_settings()` with `array_union_keys=set()` (YAML inheritance layer only) | -| Deep dict | `user-settings` | `deep_merge_settings()` with `DEFAULT_ARRAY_UNION_KEYS` (YAML inheritance layer only) | -| Shallow dict | `os-env-variables` | Shallow merge; child overrides; `null` deletes (RFC 7396) | +| Type | Keys | Strategy | +|---------------------------------|-------------------------------------|----------------------------------------------------------------------------------------| +| String list | `agents`, `slash-commands`, `rules` | Concatenate parent + child; deduplicate by string equality; parent items first | +| Named list (by `name`) | `mcp-servers`, `skills` | Identity-based: child overrides parent in-position; new items appended | +| Named list (by final file path) | `files-to-download` | Identity-based: child overrides parent in-position; new items appended | +| Per-platform dict | `dependencies` | Per-platform sub-key list concatenation with deduplication | +| Composite | `hooks` | `files`: concat + dedup by full path; `events`: concat (no dedup) | +| Deep dict | `global-config` | `deep_merge_settings()` with `array_union_keys=set()` (YAML inheritance layer only) | +| Deep dict | `user-settings` | `deep_merge_settings()` with `DEFAULT_ARRAY_UNION_KEYS` (YAML inheritance layer only) | +| Shallow dict | `os-env-variables` | Shallow merge; child overrides; `null` deletes (RFC 7396) | + +The `files-to-download` identity is the normalized final file path: a `dest` ending with `/` or `\` is combined with the source filename (query parameters stripped) before matching, so distinct files sharing a directory dest keep distinct identities. See [`files-to-download`](#files-to-download) for details. > **Note:** The `global-config` and `user-settings` rows above describe the YAML inheritance layer only -- how `merge-keys` composes parent and child configurations before the writer touches disk. The `user-settings` deep merge covers its nested `env` block (Claude-session environment variables). The on-disk writers (`write_global_config()`, `write_user_settings()`, `write_profile_settings_to_settings()`) use the universal array-union contract: every list at every depth is unioned with structural dedupe, independent of `DEFAULT_ARRAY_UNION_KEYS`. See [Profile-Level Settings Routing](#profile-level-settings-routing) for the on-disk write contract. diff --git a/scripts/setup_environment.py b/scripts/setup_environment.py index bc3abc6..ba3df4e 100644 --- a/scripts/setup_environment.py +++ b/scripts/setup_environment.py @@ -5301,44 +5301,163 @@ def _merge_string_list( return result +def _name_identity(item: dict[str, Any]) -> str | None: + """Compute the merge identity of a named entry (mcp-servers, skills). + + Args: + item: An entry dict with a 'name' key. + + Returns: + The name as a string, or None when the entry has no name. + """ + name = item.get('name') + return None if name is None else str(name) + + +def _source_filename(source: str) -> str: + """Derive the deployed filename a directory-form dest receives for a source. + + Strips query parameters and takes the last path segment. GitLab API + raw-file URLs ({base}/api/v4/projects/{id}/repository/files/{encoded + path}/raw?ref=...) carry the real filename inside the URL-encoded path + segment while their last path segment is the literal 'raw', so for them + the encoded segment is decoded and its basename used instead. Purely + lexical (no filesystem or network access), so the result is identical + whether the source is still relative or already resolved to a URL. + + Args: + source: Source path or URL from a files-to-download entry. + + Returns: + The filename to append to a directory-form dest. + """ + clean_source = source.split('?')[0] + if ( + clean_source.endswith('/raw') + and '/api/v4/projects/' in clean_source + and '/repository/files/' in clean_source + ): + encoded_path = clean_source.split('/repository/files/')[-1].removesuffix('/raw') + return Path(urllib.parse.unquote(encoded_path)).name + return Path(clean_source).name + + +def _files_download_identity(item: dict[str, Any]) -> str | None: + """Compute the merge identity of a files-to-download entry. + + The identity is the final deployed file path in lexical form. A dest + ending with a path separator ('/' or '\\') is a directory destination, + so the filename from _source_filename() is appended, mirroring the + destination resolution in process_file_downloads(), which uses the same + helper. The check is purely lexical (no filesystem access) so that + inheritance resolution behaves identically on machines whose filesystem + does not match the target: a dest without a trailing separator is + treated as a file path even if a directory exists at that path at + download time. + + Args: + item: An entry dict with 'source' and 'dest' keys. + + Returns: + The identity string, or None when the entry has no dest. + """ + dest = item.get('dest') + if dest is None: + return None + dest_str = str(dest) + if dest_str.endswith(('/', '\\')): + return dest_str + _source_filename(str(item.get('source', ''))) + return dest_str + + +def _dedupe_by_identity( + items: list[dict[str, Any]], + identity_fn: Callable[[dict[str, Any]], str | None], + section: str, +) -> list[dict[str, Any]]: + """Drop entries sharing an identity, keeping only the last occurrence. + + Entries with the same identity resolve to the same final artifact, so + only one can take effect; the last one is kept to match the + later-overrides-earlier merge semantics, and every dropped entry is + reported with a warning instead of disappearing silently. + + Args: + items: List of entry dicts. + identity_fn: Callable computing an entry's identity (None means the + entry has no identity and always survives). + section: Configuration section name used in warning messages. + + Returns: + The list with earlier duplicate-identity entries removed. + """ + last_index: dict[str, int] = {} + for idx, item in enumerate(items): + key = identity_fn(item) + if key is not None: + last_index[key] = idx + + result: list[dict[str, Any]] = [] + for idx, item in enumerate(items): + key = identity_fn(item) + if key is not None and last_index[key] != idx: + warning( + f"Duplicate identity '{key}' in {section}: " + f'ignoring an earlier entry in favor of the last one', + ) + continue + result.append(item) + return result + + def _merge_named_list( parent_list: list[dict[str, Any]], child_list: list[dict[str, Any]], - identity_key: str, + identity_fn: Callable[[dict[str, Any]], str | None], + section: str, ) -> list[dict[str, Any]]: """Merge named lists with in-position replacement for matching identities. - Child items sharing a parent item's identity replace it at the parent's + Identities are computed by identity_fn (the 'name' field for mcp-servers + and skills, the normalized final file path for files-to-download). Child + items sharing a parent item's identity replace it at the parent's original position. New child items (no matching parent) are appended. + Duplicate identities within either input list are collapsed to the last + occurrence with a warning before merging. Args: parent_list: Base list of dicts. child_list: Override list of dicts. - identity_key: Dict key used as the identity for matching. + identity_fn: Callable computing an entry's identity (None means the + entry has no identity, never matches, and is always kept). + section: Configuration section name used in duplicate warnings. Returns: Merged list preserving parent ordering with child overrides and appends. """ + parent_list = _dedupe_by_identity(parent_list, identity_fn, section) + child_list = _dedupe_by_identity(child_list, identity_fn, section) + child_by_id: dict[str, dict[str, Any]] = {} for item in child_list: - key = item.get(identity_key) + key = identity_fn(item) if key is not None: - child_by_id[str(key)] = item + child_by_id[key] = item consumed: set[str] = set() result: list[dict[str, Any]] = [] for parent_item in parent_list: - parent_key = str(parent_item.get(identity_key, '')) - if parent_key in child_by_id: + parent_key = identity_fn(parent_item) + if parent_key is not None and parent_key in child_by_id: result.append(child_by_id[parent_key]) consumed.add(parent_key) else: result.append(parent_item) for item in child_list: - key = str(item.get(identity_key, '')) - if key not in consumed: + key = identity_fn(item) + if key is None or key not in consumed: result.append(item) return result @@ -5424,13 +5543,13 @@ def _merge_config_key( if key in ('mcp-servers', 'skills'): p_named = cast(list[dict[str, object]], parent_value) if isinstance(parent_value, list) else [] c_named = cast(list[dict[str, object]], child_value) if isinstance(child_value, list) else [] - return _merge_named_list(p_named, c_named, 'name') + return _merge_named_list(p_named, c_named, _name_identity, key) - # Named list key with identity by 'dest' + # Named list key with identity by the normalized final file path if key == 'files-to-download': p_files = cast(list[dict[str, object]], parent_value) if isinstance(parent_value, list) else [] c_files = cast(list[dict[str, object]], child_value) if isinstance(child_value, list) else [] - return _merge_named_list(p_files, c_files, 'dest') + return _merge_named_list(p_files, c_files, _files_download_identity, key) # Dependencies: per-platform merge if key == 'dependencies': @@ -8089,6 +8208,36 @@ def extract_front_matter(file_path: Path) -> dict[str, Any] | None: return None +def _write_file_atomic(destination: Path, write_to: Callable[[Path], object]) -> None: + """Write a file atomically via a same-directory temp file and os.replace(). + + The content is written to a uniquely-named temporary file in the + destination's directory and then moved into place with os.replace(), so + a reader (or a concurrent writer targeting the same path) can never + observe a partially-written destination: it sees either the old file or + the complete new one. + + Args: + destination: Final file path; its parent directory must exist. + write_to: Callable that writes the content to the given temp path. + """ + fd, tmp_name = tempfile.mkstemp(dir=str(destination.parent), prefix=f'.{destination.name}.', suffix='.tmp') + os.close(fd) + tmp_path = Path(tmp_name) + try: + # mkstemp creates the file with restrictive 0o600 permissions; align + # with the destination's existing mode (or the conventional 0o644 for + # new files) before writing. shutil.copy2 writers subsequently apply + # the source file's mode on top, matching a direct copy's semantics. + if os.name != 'nt': + mode = destination.stat().st_mode & 0o777 if destination.exists() else 0o644 + os.chmod(tmp_path, mode) + write_to(tmp_path) + os.replace(tmp_path, destination) + finally: + tmp_path.unlink(missing_ok=True) + + def handle_resource( resource_path: str, destination: Path, @@ -8130,13 +8279,13 @@ def handle_resource( content_bytes = fetch_url_bytes_with_auth( resolved_path, auth_param=auth_param, rate_limiter=rate_limiter, auth_cache=auth_cache, ) - destination.write_bytes(content_bytes) + _write_file_atomic(destination, lambda p: p.write_bytes(content_bytes)) else: # Text file - fetch as text and write text content = fetch_url_with_auth( resolved_path, auth_param=auth_param, rate_limiter=rate_limiter, auth_cache=auth_cache, ) - destination.write_text(content, encoding='utf-8') + _write_file_atomic(destination, lambda p: p.write_text(content, encoding='utf-8')) success(f'Downloaded: {filename}') else: # Copy from local path @@ -8146,7 +8295,7 @@ def handle_resource( return False # Copy the file - shutil.copy2(source_path, destination) + _write_file_atomic(destination, lambda p: shutil.copy2(source_path, p)) success(f'Copied: {filename} from {source_path}') return True @@ -8280,13 +8429,28 @@ def process_file_downloads( # If dest ends with separator or is existing directory, append source filename dest_str = str(dest) if dest_str.endswith(('/', '\\')) or (dest_path.exists() and dest_path.is_dir()): - # Extract filename from source (remove query params if present) - clean_source = str(source).split('?')[0] - filename = Path(clean_source).name - dest_path = dest_path / filename + dest_path = dest_path / _source_filename(str(source)) valid_downloads.append((str(source), dest_path)) + # Entries resolving to the same final file would race in the parallel + # download phase; keep only the last one (later-overrides-earlier + # semantics) and warn about each skipped entry. + last_by_dest: dict[Path, int] = {} + for idx, (_, dest_path) in enumerate(valid_downloads): + last_by_dest[dest_path] = idx + if len(last_by_dest) < len(valid_downloads): + deduped_downloads: list[tuple[str, Path]] = [] + for idx, (source_str, dest_path) in enumerate(valid_downloads): + if last_by_dest[dest_path] != idx: + warning( + f"Multiple entries resolve to the same destination '{dest_path}': " + f"skipping earlier source '{source_str}'", + ) + continue + deduped_downloads.append((source_str, dest_path)) + valid_downloads = deduped_downloads + # Per-batch coordinator shares rate-limit state across download threads rate_limiter = RateLimitCoordinator() diff --git a/tests/e2e/fixtures/merge_dirdest_child.yaml b/tests/e2e/fixtures/merge_dirdest_child.yaml new file mode 100644 index 0000000..c59b805 --- /dev/null +++ b/tests/e2e/fixtures/merge_dirdest_child.yaml @@ -0,0 +1,14 @@ +# Child config composing shared libraries into a directory-form dest +name: "DirDest Child" + +inherit: merge_dirdest_parent.yaml +merge-keys: + - files-to-download + +files-to-download: + - source: "configs/hook_json_output.py" + dest: "~/.claude/hooks/" + - source: "configs/hook_bypass_detection.py?raw=true" + dest: "~/.claude/hooks/" + - source: "configs/patched/hook_config_loader.py" + dest: "~/.claude/hooks/hook_config_loader.py" diff --git a/tests/e2e/fixtures/merge_dirdest_parent.yaml b/tests/e2e/fixtures/merge_dirdest_parent.yaml new file mode 100644 index 0000000..8d7250b --- /dev/null +++ b/tests/e2e/fixtures/merge_dirdest_parent.yaml @@ -0,0 +1,8 @@ +# Base config for directory-form dest merge E2E testing +name: "DirDest Parent" + +files-to-download: + - source: "configs/hook_config_loader.py" + dest: "~/.claude/hooks/" + - source: "configs/parent-only.txt" + dest: "~/.claude/parent-only.txt" diff --git a/tests/e2e/test_merge_keys.py b/tests/e2e/test_merge_keys.py index b8c0dfe..d66cd51 100644 --- a/tests/e2e/test_merge_keys.py +++ b/tests/e2e/test_merge_keys.py @@ -288,3 +288,47 @@ def test_mixed_within_single_child(self, fixtures_dir): assert len(resolved['agents']) == 2 # Replaced key (name is not in merge-keys) assert resolved['name'] == 'Merge Child' + + +class TestFilesToDownloadDirectoryDestMerge: + """Test files-to-download merge identity for directory-form destinations. + + Merge identity is the normalized final file path: a dest ending with a + path separator is combined with the source basename (query parameters + stripped), so distinct files sharing a directory dest keep distinct + identities and all survive composition. + """ + + def _resolved_files(self, fixtures_dir: Path) -> list[dict[str, Any]]: + child_path = fixtures_dir / 'merge_dirdest_child.yaml' + config = _load_yaml(child_path) + resolved, _ = _resolve(config, str(child_path)) + return resolved['files-to-download'] + + def test_all_directory_dest_entries_survive(self, fixtures_dir): + """Distinct libraries sharing a directory-form dest all survive the merge.""" + files = self._resolved_files(fixtures_dir) + identities = [setup_environment._files_download_identity(f) for f in files] + assert '~/.claude/hooks/hook_json_output.py' in identities + assert '~/.claude/hooks/hook_bypass_detection.py' in identities + assert '~/.claude/hooks/hook_config_loader.py' in identities + assert len(files) == 4 + + def test_explicit_child_dest_replaces_parent_directory_entry_in_position(self, fixtures_dir): + """A child explicit file dest replaces the parent's matching directory-form entry in-position.""" + files = self._resolved_files(fixtures_dir) + assert files[0]['dest'] == '~/.claude/hooks/hook_config_loader.py' + assert files[0]['source'].replace('\\', '/').endswith('configs/patched/hook_config_loader.py') + + def test_parent_explicit_entry_preserved(self, fixtures_dir): + """The parent's unrelated explicit-file entry is preserved at its position.""" + files = self._resolved_files(fixtures_dir) + assert files[1]['dest'] == '~/.claude/parent-only.txt' + + def test_query_param_source_keeps_own_identity(self, fixtures_dir): + """A query-param source contributes its clean basename to the identity.""" + files = self._resolved_files(fixtures_dir) + query_entries = [f for f in files if '?' in f['source']] + assert len(query_entries) == 1 + identity = setup_environment._files_download_identity(query_entries[0]) + assert identity == '~/.claude/hooks/hook_bypass_detection.py' diff --git a/tests/test_setup_environment.py b/tests/test_setup_environment.py index 7b1fa72..7f838a4 100644 --- a/tests/test_setup_environment.py +++ b/tests/test_setup_environment.py @@ -5855,38 +5855,172 @@ def test_merge_named_list_in_position_replacement(self): """Named list: child replaces parent item at parent's position.""" parent = [{'name': 'srv1', 'url': 'old'}, {'name': 'srv2', 'url': 'keep'}] child = [{'name': 'srv1', 'url': 'new'}] - result = setup_environment._merge_named_list(parent, child, 'name') + result = setup_environment._merge_named_list(parent, child, setup_environment._name_identity, 'mcp-servers') assert result == [{'name': 'srv1', 'url': 'new'}, {'name': 'srv2', 'url': 'keep'}] def test_merge_named_list_new_items_appended(self): """Named list: new child items are appended at the end.""" parent = [{'name': 'srv1'}] child = [{'name': 'srv2'}] - result = setup_environment._merge_named_list(parent, child, 'name') + result = setup_environment._merge_named_list(parent, child, setup_environment._name_identity, 'mcp-servers') assert result == [{'name': 'srv1'}, {'name': 'srv2'}] def test_merge_named_list_mixed_replace_and_append(self): """Named list: some replaced in-position, some appended.""" parent = [{'name': 'A', 'v': 1}, {'name': 'B', 'v': 2}] child = [{'name': 'B', 'v': 20}, {'name': 'C', 'v': 3}] - result = setup_environment._merge_named_list(parent, child, 'name') + result = setup_environment._merge_named_list(parent, child, setup_environment._name_identity, 'mcp-servers') assert result == [{'name': 'A', 'v': 1}, {'name': 'B', 'v': 20}, {'name': 'C', 'v': 3}] def test_merge_named_list_empty_lists(self): """Named list: empty parent and child.""" - result = setup_environment._merge_named_list([], [], 'name') + result = setup_environment._merge_named_list([], [], setup_environment._name_identity, 'mcp-servers') assert result == [] def test_merge_named_list_missing_identity_key(self): """Named list: items missing identity key are kept and appended independently.""" parent = [{'v': 1}] child = [{'v': 2}] - result = setup_environment._merge_named_list(parent, child, 'name') + result = setup_environment._merge_named_list(parent, child, setup_environment._name_identity, 'mcp-servers') # Items without identity key are not matched; both are kept assert len(result) == 2 assert result[0] == {'v': 1} assert result[1] == {'v': 2} + def test_merge_named_list_duplicate_identity_in_child_warns_keeps_last(self): + """Named list: duplicate identities within the child list collapse to the last with a warning.""" + parent = [{'name': 'A', 'v': 1}] + child = [{'name': 'A', 'v': 2}, {'name': 'A', 'v': 3}] + with patch('setup_environment.warning') as mock_warning: + result = setup_environment._merge_named_list( + parent, child, setup_environment._name_identity, 'mcp-servers', + ) + assert result == [{'name': 'A', 'v': 3}] + assert mock_warning.call_count == 1 + assert "Duplicate identity 'A' in mcp-servers" in mock_warning.call_args[0][0] + + def test_merge_named_list_duplicate_identity_in_parent_warns_keeps_last(self): + """Named list: duplicate identities within the parent list collapse to the last with a warning.""" + parent = [{'name': 'A', 'v': 1}, {'name': 'A', 'v': 2}] + child = [{'name': 'B', 'v': 3}] + with patch('setup_environment.warning') as mock_warning: + result = setup_environment._merge_named_list( + parent, child, setup_environment._name_identity, 'skills', + ) + assert result == [{'name': 'A', 'v': 2}, {'name': 'B', 'v': 3}] + assert mock_warning.call_count == 1 + assert "Duplicate identity 'A' in skills" in mock_warning.call_args[0][0] + + # === Identity function tests === + + def test_name_identity_returns_name(self): + """_name_identity returns the name as a string.""" + assert setup_environment._name_identity({'name': 'srv1'}) == 'srv1' + + def test_name_identity_missing_name_returns_none(self): + """_name_identity returns None when no name is present.""" + assert setup_environment._name_identity({'url': 'x'}) is None + + def test_files_download_identity_file_dest_passthrough(self): + """File-form dest (no trailing separator) is the identity as-is.""" + item = {'source': 'configs/a.txt', 'dest': '~/.claude/a.txt'} + assert setup_environment._files_download_identity(item) == '~/.claude/a.txt' + + def test_files_download_identity_directory_dest_appends_basename(self): + """Directory-form dest (trailing slash) appends the source basename.""" + item = {'source': 'hooks/hook_config_loader.py', 'dest': '~/.claude/hooks/'} + assert setup_environment._files_download_identity(item) == '~/.claude/hooks/hook_config_loader.py' + + def test_files_download_identity_backslash_directory_dest(self): + """Directory-form dest (trailing backslash) appends the source basename.""" + item = {'source': 'hooks/loader.py', 'dest': '~\\.claude\\hooks\\'} + assert setup_environment._files_download_identity(item) == '~\\.claude\\hooks\\loader.py' + + def test_files_download_identity_strips_query_params(self): + """Query parameters are stripped from the source before taking the basename.""" + item = {'source': 'https://example.com/hooks/loader.py?ref=main&raw=true', 'dest': '~/.claude/hooks/'} + assert setup_environment._files_download_identity(item) == '~/.claude/hooks/loader.py' + + def test_files_download_identity_missing_dest_returns_none(self): + """Entries without dest have no identity.""" + assert setup_environment._files_download_identity({'source': 'a.txt'}) is None + + def test_files_download_identity_matches_download_resolution(self): + """Merge identity mirrors the download-time destination resolution for URL sources.""" + item = {'source': 'https://raw.githubusercontent.com/org/repo/main/libs/util.py', 'dest': '~/.claude/libs/'} + assert setup_environment._files_download_identity(item) == '~/.claude/libs/util.py' + + # === _source_filename tests === + + def test_source_filename_plain_path(self): + """A plain relative path yields its basename.""" + assert setup_environment._source_filename('configs/settings.json') == 'settings.json' + + def test_source_filename_strips_query_params(self): + """Query parameters are stripped before taking the basename.""" + assert setup_environment._source_filename('libs/util.py?raw=true&token=x') == 'util.py' + + def test_source_filename_gitlab_api_url_decodes_encoded_path(self): + """GitLab API raw-file URLs yield the decoded real filename, not the literal 'raw'.""" + url = 'https://gitlab.com/api/v4/projects/123/repository/files/configs%2Fsettings.json/raw?ref=main' + assert setup_environment._source_filename(url) == 'settings.json' + + def test_source_filename_gitlab_api_url_unencoded_path(self): + """GitLab API URLs with an unencoded path segment also yield the real filename.""" + url = 'https://gitlab.com/api/v4/projects/123/repository/files/configs/settings.json/raw?ref=main' + assert setup_environment._source_filename(url) == 'settings.json' + + def test_source_filename_non_gitlab_raw_suffix_kept(self): + """A non-GitLab source whose last segment is 'raw' keeps that basename.""" + assert setup_environment._source_filename('https://example.com/data/raw') == 'raw' + + def test_files_download_identity_stable_across_gitlab_resolution(self): + """A raw relative source and its GitLab-API-resolved form produce the same identity.""" + raw_entry = {'source': 'configs/settings.json', 'dest': '~/.claude/'} + resolved_entry = { + 'source': 'https://gitlab.com/api/v4/projects/123/repository/files/configs%2Fsettings.json/raw?ref=main', + 'dest': '~/.claude/', + } + raw_identity = setup_environment._files_download_identity(raw_entry) + resolved_identity = setup_environment._files_download_identity(resolved_entry) + assert raw_identity == resolved_identity == '~/.claude/settings.json' + + def test_merge_config_key_files_to_download_gitlab_resolved_parent_overridden(self): + """A child raw-source entry overrides a parent whose source was resolved to a GitLab API URL.""" + parent = [{ + 'source': 'https://gitlab.com/api/v4/projects/123/repository/files/configs%2Fsettings.json/raw?ref=main', + 'dest': '~/.claude/', + }] + child = [{'source': 'configs/settings.json', 'dest': '~/.claude/'}] + result = setup_environment._merge_config_key('files-to-download', parent, child) + assert result == [{'source': 'configs/settings.json', 'dest': '~/.claude/'}] + + # === _dedupe_by_identity tests === + + def test_dedupe_by_identity_no_duplicates_unchanged(self): + """Lists without duplicate identities pass through unchanged and silently.""" + items = [{'name': 'A'}, {'name': 'B'}] + with patch('setup_environment.warning') as mock_warning: + result = setup_environment._dedupe_by_identity(items, setup_environment._name_identity, 'skills') + assert result == items + mock_warning.assert_not_called() + + def test_dedupe_by_identity_keeps_last_and_warns_per_dropped_entry(self): + """Each dropped earlier duplicate produces one warning; the last entry wins.""" + items = [{'name': 'A', 'v': 1}, {'name': 'A', 'v': 2}, {'name': 'A', 'v': 3}] + with patch('setup_environment.warning') as mock_warning: + result = setup_environment._dedupe_by_identity(items, setup_environment._name_identity, 'skills') + assert result == [{'name': 'A', 'v': 3}] + assert mock_warning.call_count == 2 + + def test_dedupe_by_identity_none_identities_always_survive(self): + """Entries without an identity are never treated as duplicates.""" + items = [{'v': 1}, {'v': 2}] + with patch('setup_environment.warning') as mock_warning: + result = setup_environment._dedupe_by_identity(items, setup_environment._name_identity, 'skills') + assert result == items + mock_warning.assert_not_called() + def test_merge_hooks_files_dedup_events_concat(self): """Hooks merge: files deduped, events concatenated.""" parent = {'files': ['a.py', 'b.py'], 'events': [{'event': 'E1'}]} @@ -5942,12 +6076,71 @@ def test_merge_config_key_skills(self): assert result == [{'name': 'sk1', 'base': '/a'}, {'name': 'sk2', 'base': '/b'}] def test_merge_config_key_files_to_download(self): - """Dispatch: files-to-download uses named list merge by 'dest'.""" + """Dispatch: files-to-download uses named list merge by final file path.""" parent = [{'source': 'a', 'dest': '~/.claude/a.txt'}] child = [{'source': 'b', 'dest': '~/.claude/a.txt'}] result = setup_environment._merge_config_key('files-to-download', parent, child) assert result == [{'source': 'b', 'dest': '~/.claude/a.txt'}] + def test_merge_config_key_files_to_download_directory_dest_all_survive(self): + """Distinct files sharing a directory-form dest keep distinct identities and all survive.""" + parent = [{'source': 'hooks/hook_config_loader.py', 'dest': '~/.claude/hooks/'}] + child = [ + {'source': 'hooks/hook_json_output.py', 'dest': '~/.claude/hooks/'}, + {'source': 'hooks/hook_bypass_detection.py', 'dest': '~/.claude/hooks/'}, + ] + result = setup_environment._merge_config_key('files-to-download', parent, child) + assert result == [ + {'source': 'hooks/hook_config_loader.py', 'dest': '~/.claude/hooks/'}, + {'source': 'hooks/hook_json_output.py', 'dest': '~/.claude/hooks/'}, + {'source': 'hooks/hook_bypass_detection.py', 'dest': '~/.claude/hooks/'}, + ] + + def test_merge_config_key_files_to_download_directory_dest_same_basename_overrides(self): + """A child directory-form entry with the same source basename replaces the parent in-position.""" + parent = [ + {'source': 'hooks/hook_config_loader.py', 'dest': '~/.claude/hooks/'}, + {'source': 'configs/other.txt', 'dest': '~/.claude/other.txt'}, + ] + child = [{'source': 'patched/hook_config_loader.py', 'dest': '~/.claude/hooks/'}] + result = setup_environment._merge_config_key('files-to-download', parent, child) + assert result == [ + {'source': 'patched/hook_config_loader.py', 'dest': '~/.claude/hooks/'}, + {'source': 'configs/other.txt', 'dest': '~/.claude/other.txt'}, + ] + + def test_merge_config_key_files_to_download_directory_vs_explicit_dest_match(self): + """A parent directory-form dest and a child explicit file dest resolving to the same path match.""" + parent = [{'source': 'hooks/hook_config_loader.py', 'dest': '~/.claude/hooks/'}] + child = [{'source': 'patched/hook_config_loader.py', 'dest': '~/.claude/hooks/hook_config_loader.py'}] + result = setup_environment._merge_config_key('files-to-download', parent, child) + assert result == [ + {'source': 'patched/hook_config_loader.py', 'dest': '~/.claude/hooks/hook_config_loader.py'}, + ] + + def test_merge_config_key_files_to_download_query_param_source_matches(self): + """Query-param sources normalize to the same identity as the clean source.""" + parent = [{'source': 'hooks/loader.py', 'dest': '~/.claude/hooks/'}] + child = [{'source': 'https://example.com/hooks/loader.py?token=x', 'dest': '~/.claude/hooks/'}] + result = setup_environment._merge_config_key('files-to-download', parent, child) + assert result == [ + {'source': 'https://example.com/hooks/loader.py?token=x', 'dest': '~/.claude/hooks/'}, + ] + + def test_merge_config_key_files_to_download_duplicate_directory_dest_in_one_list_warns(self): + """Two same-final-path entries within one list collapse to the last with a warning.""" + parent = [] + child = [ + {'source': 'v1/hook_config_loader.py', 'dest': '~/.claude/hooks/'}, + {'source': 'v2/hook_config_loader.py', 'dest': '~/.claude/hooks/'}, + ] + with patch('setup_environment.warning') as mock_warning: + result = setup_environment._merge_config_key('files-to-download', parent, child) + assert result == [{'source': 'v2/hook_config_loader.py', 'dest': '~/.claude/hooks/'}] + assert mock_warning.call_count == 1 + warning_text = mock_warning.call_args[0][0] + assert "Duplicate identity '~/.claude/hooks/hook_config_loader.py' in files-to-download" in warning_text + def test_merge_config_key_dependencies(self): """Dispatch: dependencies uses per-platform merge.""" parent = {'common': ['cmd1']} diff --git a/tests/test_setup_environment_file_downloads.py b/tests/test_setup_environment_file_downloads.py index b6fde6f..762ddeb 100644 --- a/tests/test_setup_environment_file_downloads.py +++ b/tests/test_setup_environment_file_downloads.py @@ -1,10 +1,15 @@ """Tests for file download functionality in setup_environment.py.""" import os +import sys +import tempfile from pathlib import Path from unittest.mock import MagicMock from unittest.mock import patch +import pytest + +from scripts.setup_environment import _write_file_atomic from scripts.setup_environment import process_file_downloads @@ -184,6 +189,23 @@ def test_process_file_downloads_query_params_removed( # Filename should be 'file.txt', not 'file.txt?raw=true' assert dest_path.name == 'file.txt' + @patch('scripts.setup_environment.handle_resource') + def test_process_file_downloads_gitlab_api_source_real_filename( + self, mock_handle: MagicMock, + ) -> None: + """A GitLab API raw-file source deploys under its decoded filename, not 'raw'.""" + mock_handle.return_value = True + + file_specs = [{ + 'source': 'https://gitlab.com/api/v4/projects/123/repository/files/configs%2Fsettings.json/raw?ref=main', + 'dest': '~/dest/', + }] + process_file_downloads(file_specs, 'config.yaml') + + call_args = mock_handle.call_args[0] + dest_path = call_args[1] + assert dest_path.name == 'settings.json' + @patch('scripts.setup_environment.success') @patch('scripts.setup_environment.handle_resource') def test_process_file_downloads_multiple_files( @@ -322,3 +344,150 @@ def test_process_file_downloads_uses_normalize_tilde_path( process_file_downloads(file_specs, 'config.yaml') mock_normalize.assert_called_once_with('~/test.txt') + + +class TestProcessFileDownloadsSameDestDedupe: + """Entries resolving to the same final file are deduplicated before the parallel phase.""" + + @patch('scripts.setup_environment.warning') + @patch('scripts.setup_environment.handle_resource') + def test_same_explicit_dest_downloads_last_only( + self, mock_handle: MagicMock, mock_warning: MagicMock, + ) -> None: + """Two entries with the same explicit dest download only the last source.""" + mock_handle.return_value = True + + file_specs = [ + {'source': 'v1/file.txt', 'dest': '~/file.txt'}, + {'source': 'v2/file.txt', 'dest': '~/file.txt'}, + ] + result = process_file_downloads(file_specs, 'config.yaml') + + assert result is True + assert mock_handle.call_count == 1 + assert mock_handle.call_args[0][0] == 'v2/file.txt' + assert mock_warning.call_count == 1 + warning_text = mock_warning.call_args[0][0] + assert 'same destination' in warning_text + assert 'v1/file.txt' in warning_text + + @patch('scripts.setup_environment.warning') + @patch('scripts.setup_environment.handle_resource') + def test_directory_and_explicit_dest_resolving_same_path_deduped( + self, mock_handle: MagicMock, mock_warning: MagicMock, + ) -> None: + """A directory-form dest and an explicit file dest resolving to one path race no more.""" + mock_handle.return_value = True + + file_specs = [ + {'source': 'v1/file.txt', 'dest': '~/target/'}, + {'source': 'v2/file.txt', 'dest': '~/target/file.txt'}, + ] + result = process_file_downloads(file_specs, 'config.yaml') + + assert result is True + assert mock_handle.call_count == 1 + assert mock_handle.call_args[0][0] == 'v2/file.txt' + assert mock_handle.call_args[0][1].name == 'file.txt' + assert mock_warning.call_count == 1 + + @patch('scripts.setup_environment.warning') + @patch('scripts.setup_environment.handle_resource') + def test_distinct_dests_not_deduped( + self, mock_handle: MagicMock, mock_warning: MagicMock, + ) -> None: + """Entries with distinct final paths all download without warnings.""" + mock_handle.return_value = True + + file_specs = [ + {'source': 'hooks/hook_config_loader.py', 'dest': '~/hooks/'}, + {'source': 'hooks/hook_json_output.py', 'dest': '~/hooks/'}, + {'source': 'hooks/hook_bypass_detection.py', 'dest': '~/hooks/'}, + ] + result = process_file_downloads(file_specs, 'config.yaml') + + assert result is True + assert mock_handle.call_count == 3 + mock_warning.assert_not_called() + + +class TestWriteFileAtomic: + """Test the temp-file-plus-rename atomic write helper.""" + + def test_writes_content_and_leaves_no_temp_files(self) -> None: + """Content lands at the destination with no temp file left behind.""" + with tempfile.TemporaryDirectory() as tmpdir: + dest = Path(tmpdir) / 'out.txt' + _write_file_atomic(dest, lambda p: p.write_text('payload', encoding='utf-8')) + + assert dest.read_text(encoding='utf-8') == 'payload' + assert [f.name for f in Path(tmpdir).iterdir()] == ['out.txt'] + + def test_overwrites_existing_destination(self) -> None: + """An existing destination is replaced with the new content.""" + with tempfile.TemporaryDirectory() as tmpdir: + dest = Path(tmpdir) / 'out.txt' + dest.write_text('old', encoding='utf-8') + + _write_file_atomic(dest, lambda p: p.write_text('new', encoding='utf-8')) + + assert dest.read_text(encoding='utf-8') == 'new' + + def test_failed_write_preserves_existing_destination(self) -> None: + """A writer failure leaves the previous content intact and cleans up the temp file.""" + def failing_writer(path: Path) -> None: + path.write_text('partial', encoding='utf-8') + raise OSError('simulated mid-write failure') + + with tempfile.TemporaryDirectory() as tmpdir: + dest = Path(tmpdir) / 'out.txt' + dest.write_text('old', encoding='utf-8') + + with pytest.raises(OSError, match='simulated mid-write failure'): + _write_file_atomic(dest, failing_writer) + + assert dest.read_text(encoding='utf-8') == 'old' + assert [f.name for f in Path(tmpdir).iterdir()] == ['out.txt'] + + def test_failed_write_creates_no_destination(self) -> None: + """A writer failure for a new file leaves no destination and no temp file.""" + def failing_writer(_path: Path) -> None: + raise OSError('simulated failure') + + with tempfile.TemporaryDirectory() as tmpdir: + dest = Path(tmpdir) / 'out.txt' + + with pytest.raises(OSError, match='simulated failure'): + _write_file_atomic(dest, failing_writer) + + assert not dest.exists() + assert list(Path(tmpdir).iterdir()) == [] + + def test_writes_bytes(self) -> None: + """Binary content is written intact.""" + with tempfile.TemporaryDirectory() as tmpdir: + dest = Path(tmpdir) / 'out.bin' + _write_file_atomic(dest, lambda p: p.write_bytes(b'\x00\x01\x02')) + + assert dest.read_bytes() == b'\x00\x01\x02' + + @pytest.mark.skipif(sys.platform == 'win32', reason='POSIX file mode semantics') + def test_new_file_gets_default_mode(self) -> None: + """A newly created destination receives the conventional 0o644 mode.""" + with tempfile.TemporaryDirectory() as tmpdir: + dest = Path(tmpdir) / 'out.txt' + _write_file_atomic(dest, lambda p: p.write_text('x', encoding='utf-8')) + + assert dest.stat().st_mode & 0o777 == 0o644 + + @pytest.mark.skipif(sys.platform == 'win32', reason='POSIX file mode semantics') + def test_existing_file_mode_preserved(self) -> None: + """An existing destination keeps its mode across an atomic overwrite.""" + with tempfile.TemporaryDirectory() as tmpdir: + dest = Path(tmpdir) / 'out.txt' + dest.write_text('old', encoding='utf-8') + os.chmod(dest, 0o600) + + _write_file_atomic(dest, lambda p: p.write_text('new', encoding='utf-8')) + + assert dest.stat().st_mode & 0o777 == 0o600