Hi! While preparing the two runner MRs (album-solution-api!16 and !290) I read through the whole of `src/` on `dev` (`66c90ef`) and kept notes of everything that looked off. Nothing here is fixed or changed; this is just the list so you can decide what is worth tackling, in which order, or not at all. Where I could, I confirmed a point by running it; the rest is from reading, and I have tried to say which is which. Happy to turn any item into an MR if you want one. Line numbers refer to `dev` at `66c90ef`. ## 1. Things that look like real bugs ### 1.1 `album deploy --force-deploy` is inverted `src/album/argument_parsing.py:281-287` declares `--force-deploy` with `action="store_false"`. So `album deploy` without the flag passes `force_deploy=True` (existing solutions in a direct catalog get overwritten silently), and adding `--force-deploy` turns force *off*. The help text describes the opposite. The API tests only exercise `force_deploy=True` through `Album.deploy`, which is why nothing catches it. (From reading; the test in `test/unit/test_argument_parsing.py:48` only checks that the subcommand parses.) ### 1.2 Resolving a solution by DOI fails with today's Zenodo URLs `resolve_operations._parse_zenodo_url` (`src/album/core/utils/operations/resolve_operations.py:206-216`) requires `zenodo.org/record/<id>`. `retrieve_redirect_url` follows all redirects and returns the *final* URL, which Zenodo now serves as `/records/<id>` (plural). I ran it: ``` final url after redirects: https://zenodo.org/records/5571504 parse_doi_service_url raised: ValueError Unknown zenodo URL format!... ``` So `album install 10.5281/zenodo.5571504` (the example DOI in the code comment) cannot work at the moment. Accepting `records?` in the regex would be enough. ### 1.3 `_handle_multiple_solution_matches` raises `IndexError` when nothing is installed `CollectionManager._get_latest_solution` (`collection_manager.py:596-606`) does `solutions[0]` unguarded, and `_get_latest_installed_solution` (588-594) filters down to installed ones before calling it. In `_handle_multiple_solution_matches` (523-571) both the `len(cache_matches) > 1` branch (543-552) and the `non_cache_matches` branch (553-562) call it first, so `album info <name>` for a name that exists in two versions with none installed dies with `IndexError: list index out of range` instead of falling through to "pick the latest". (From reading.) ### 1.4 Cleaning an unfinished *child* installation removes the *parent's* environment `InstallManager.clean_unfinished_installations` (`install_manager.py:433-467`) decides whether to remove an environment with `if not get_parent_dict(resolve.loaded_solution())` (line 459). But `resolve.loaded_solution()` here is built from the collection DB entry (`resolve_result.py:225-230`), and `get_deploy_dict` deliberately drops `dependencies` (`solution_operations.py:26`), so `get_parent_dict` always returns `None` for DB-loaded solutions. The guard therefore passes for children too, `_clean_unfinished_installations_environment` calls `set_environment`, which resolves the *parent's* environment through `db_entry.internal()["parent"]` (`environment_manager.py:178-201`), and that environment is removed although the parent is fully installed and possibly shared. Scenario: a child's `install()` routine fails after the parent link was written (that happens before `set_installed`), the next `album install` runs the cleanup. (From reading, fairly confident; worth a test.) The same `get_parent_dict`-on-DB-entry problem makes `album uninstall --uninstall-deps` a silent no-op when the solution file itself could not be loaded (`install_manager.py:369-370, 426-431`). ### 1.5 `uninstall` crashes when the solution file cannot be loaded `InstallManager.uninstall` (`install_manager.py:290-302`) logs "Cannot load solution ... Proceed without" and continues with `environment = None`. A few lines later (345-346) it calls `remove_environment(None)` for non-parent solutions; album-environments does `environment.path()` on it, so this ends in `AttributeError` rather than the promised graceful path. Only `LookupError` and `SubProcessError` are caught. (From reading; I checked the album-environments side.) ### 1.6 Plugin activation closes the shared collection index `RunManager.load_plugins` (`run_manager.py:92-98`) wraps the controller in a throw-away `Album(self.album)` for the plugin call. `Album.__del__` calls `close()` (`api.py:78-80, 279-283`), which calls `AlbumController.close()` → `CollectionManager.close()` (`collection_manager.py:43-53`) and sets `catalog_collection = None`. CPython collects the temporary right after the plugin returns, so from then on anything that needs the index in the same process raises `LookupError("No collection loaded!")`: e.g. `set_environment` for a solution with a parent (`environment_manager.py:195` → `catalogs().get_by_id`), or any later API call in a long-lived process. (From reading.) ### 1.7 Argument `description` is optional in the schema but mandatory in the code The solution schema only requires `name` for an argument (`definitions/Argument/required == ["name"]`), but several places index `argument["description"]` directly: `catalog_index.py:290-294`, `collection_index.py:559-567` and `619-629`, `view_operations.py:57-59`. The catalog index SQL also has `description TEXT not null` (`catalog_index_schema.sql`). A solution with `{"name": "x"}` therefore validates fine but `album info` raises `KeyError` and deploy/install fails when writing the index. Either the schema should require it or the code should use `get_dict_entry`. (From reading.) ### 1.8 `album clone template:<unknown>` succeeds silently, and the error it would raise is malformed `CloneManager._clone_catalog_template` returns `False` when the template URL is not downloadable (`clone_manager.py:123`), and `clone` ignores the return value, so the command exits 0 having done nothing. The `LookupError` in `clone_manager.py:51-55` is built with a comma instead of `%`, so if it is ever raised the message is the tuple `('Cannot resolve %s ...', (path, url))`. (From reading.) ### 1.9 Resource download error path itself errors `DownloadManager._retrieve_resources_from_dict` (`shared_downloads_manager.py:178-204`): `resource_value["hash"]` is read directly on line 184, so a resource *without* a hash raises `KeyError` before the friendly "has no hash provided" message on line 195 can appear; and after a failed download `fpath` is `None`, so `pooch.file_hash(None)` on line 202 fails. The whole resource feature is currently never called (`install_manager.py:48-49` TODO), so this is dormant. (From reading.) ### 1.10 `last_execution` records the last DB update, not the last run `CollectionIndex.update_solution` (`collection_index.py:1366-1373`) unconditionally sets `last_execution = now` on every call. It is called by `set_installed`, `set_uninstalled`, `set_installation_unfinished` and catalog upgrades, while nothing sets it on `run`. `get_recently_launched_solutions` (1323-1338) therefore orders by something else. (From reading.) ### 1.11 Versions are ordered as strings `CatalogIndex.get_all_solution_versions` uses `ORDER BY version DESC` (`catalog_index.py:568`), so `0.9.0` sorts above `0.10.0`. Consumers that rely on "first is newest": `_carry_forward_doi` (`deploy_manager.py:395-409`) and the generated `CHANGELOG.md` order in `get_changelog_content` (`changelog.py:29-68`). Related: `undeploy` finds a solution's tags with `tag.startswith(as_tag_unversioned(...))` (`deploy_manager.py:455-459`), so `grp-seg` also collects the tags of `grp-seg-tools`; and the "current"/"previous" version is then taken from a commit-date ordering (`git_operations.get_tags`). (From reading.) ### 1.12 Anything containing one slash is treated as a DOI `get_doi_from_input` (`resolve_operations.py:92-97`) matches `^(doi:)?([^:/]*/[^:/]*)$`. In `_resolve` (`collection_manager.py:262-297`) a non-existing path such as a typo'd `mygroup/solution.py` therefore goes down the DOI branch and produces a network error from `doi.org` rather than "file not found". (From reading.) ### 1.13 Framework sanity check looks at the current working directory `EnvironmentManager._append_framework_to_dependencies` (`environment_manager.py:361-369`) does `Path(DefaultValues.runner_api_package_name.value).is_dir()`, i.e. `Path("album-solution-api").is_dir()` relative to the CWD. Running `album install ...` from a directory that contains a checkout of `album-solution-api` (developers will) fails every install with "Framework is not properly defined". (From reading.) ### 1.14 Every `Album` instance wipes the shared `tmp` directory `Configuration.setup()` calls `_empty_tmp()` (`configuration.py:90`, `151-156`), which `force_remove`s `<base>/tmp`. Every process that builds an `Album` on the same base path (a solution that itself uses the Python API, `allow_recursive`, two CLI invocations at once) deletes the other process's temporary directories while they may be mid-clone or mid-unzip (`TemporaryDirectory(dir=tmp_path)` is used all over `catalog_handler`, `migration_manager`, `solution_handler`, `check_doi`). (From reading.) ## 2. Robustness and platform ### 2.1 Text files are opened without `encoding=` About 20 sites, most importantly the solution file itself in `StateManager.load` (`state_manager.py:21`), the JSON helpers (`file_operations.py:67, 77`), the legacy-runner wrapper (`script_manager.py:127-136`, including the generated `exec(open(r"...").read())`), `commandline._merge_scripts:148`, `changelog.py:85`, `keepachangelog/__init__.py:63`, `resource_manager.py:135, 152, 174`, `shared_downloads_manager.py` (several), and `read_text()` in `migration_manager.py:230, 247`. On Windows this means cp1252, so a solution with an umlaut in its description fails to load with `UnicodeDecodeError`, and JSON/YAML gets written in the wrong encoding. `pyproject.toml` lists Windows as supported. ### 2.2 Context managers that do not close on error `Catalog.retrieve_catalog` (`catalog.py:248-270`), `clone_repository_sparse` (`git_operations.py:532-566`) and `clone_repository` (569-594) do `yield repo; repo.close()` without `try/finally`. If the body raises, the `Repo` (and its git processes) stays open; on Windows that is the usual cause of the `PermissionError` from the following `force_remove`. ### 2.3 `url_operations.download` leaks file descriptors and never checks the status `tempfile.mkstemp` returns an open fd that is never closed, twice (`url_operations.py:61, 65`). There is also no `raise_for_status`, so a 404 HTML page is saved as the "solution" and later `exec`'d. ### 2.4 `Database.is_created(close=True)` and `next_id(close=True)` leave a closed connection in the cache Both close `cursor.connection` directly (`database.py:99-100, 114-115`) instead of going through `close_current_connection`, so the closed connection stays in `self.connections` and the next `get_connection()` hands it back. Production code only uses `close=False` today; `test_catalog_index.py:67` uses the default. ### 2.5 IDs are `MAX+1` and the `catalog` table has no key `Database.next_id` (`database.py:81-102`) computes the next id in Python without a transaction, and `catalog` has neither a primary key nor a `UNIQUE` on `name`/`src` (`catalog_collection_schema.sql`). Two processes on the same collection can therefore produce duplicate ids. Same family as 1.14. ### 2.6 Migration failures are swallowed `migrate_catalog_collection_db` / `migrate_catalog_index_db` (`migration_manager.py:113-124, 148-159`) log the error, restore the backup and *return normally*, so `migrate_collection_index` continues with the next step on an un-migrated DB. The restore also hard-codes `catalog_collection.db` / `album_catalog_index.db` instead of using the real file name, and the index variant's message says "catalog collection database". Dormant today because `src/album/core/schema/migrations/*` contain only `__init__.py`. ### 2.7 Zenodo client details - `ZenodoAPI.validate_response` does `ResponseStatus(response.status_code)` (`zenodo_api.py:894`); any code not in the enum (301, 302, 410, 502, 503, 504 during Zenodo maintenance) becomes a bare `ValueError` instead of `InvalidResponseStatusError`, and `response.json()["message"]` (923-924) raises `KeyError` on non-JSON error bodies. - `ZenodoEntry.to_dict()` (`zenodo_api.py:152-159`) works on `self.__dict__` directly and pops `params`/`base_url`/`entry_dict` from the *live* object. - A leftover `print(...)` in `zenodo_manager.py:282`. - `album-catalog-admin --force-retrieve` uses `type=bool` (`ci/argument_parsing.py:208-214`), so `--force-retrieve False` is `True`. ### 2.8 Small git-operation slips - `retrieve_files_from_head_last_commit` logs `parent.summary` (`git_operations.py:144`) before the `if not parent` check (146), so the single-commit case is an `AttributeError` rather than the intended `RuntimeError`. - `download_repository(force_download=True)` never removes an existing checkout: the `force_remove` sits inside the "no `.git` yet" branch (`git_operations.py:501-512`), so the flag is a no-op whenever it would matter. ## 3. Smaller things and cleanups - Deprecated `Logger.warn` at seven sites in `collection_manager.py:490-566` and `backwards_compatibility_0_6_1.py:645`. (I checked: it still exists on 3.14, so this is only a `DeprecationWarning`, not a break.) `Thread.setDaemon` in `task_manager.py:38` is deprecated too. - `i is len(...) - 1` identity comparisons in `view_operations.py:116` and `161`; correct only for small ints (the `└─` marker disappears after the 257th entry). - `InstallManager._install_loaded_resolve_result`: the "Solution already installed. Uninstall solution first!" raise (`install_manager.py:95-99`) is unreachable, the earlier `elif` (74-79) has already returned. `install()`/`uninstall()` accept `argv` but never use it. - `TestManager.test` (`test_manager.py:26-35`): the condition reduces to "`test` is callable". - `CollectionManager.close()` (`collection_manager.py:43-53`) deletes and immediately re-creates both handlers. - `Catalog.is_cache()` (`catalog.py:126-132`) with the default `src=""` reaches `os.path.samefile("", ...)`. - `CatalogHandler.create_new_metadata` (`catalog_handler.py:179-193`) builds JSON by string concatenation; `json.dumps` would survive quotes in names. `add_by_src` says "Cannot add catalog twice" for a *name* clash with a different source (75-80), which is confusing. - `ResourceManager._handle_env_file_string` (`resource_manager.py:138-142`) accepts `.yml` but rejects `.yaml`; `_append_setuptools_to_yml` (213-222) only skips an exact `"setuptools"` entry, so `setuptools>=60` gets a second pin. - `SearchManager._find_matches` (`search_manager.py:52-58`) is case-sensitive substring matching. - `DefaultValues.server_port` / `server_host` (`default_values.py:130-132`) look like leftovers from the old server. - Test aggregators `test/unit/run_all_unit_micromamba.py:122` and `test/integration/run_all_integration_micromamba.py` call `main()` at import time and `exit()`, so `python -m unittest <file>` never actually reaches `unittest`; importing the module from anywhere runs the whole suite. The legacy runner module had no tests at all until !290. - `ruff --select B904` flags four `raise ... inside except` without `from` (`clone_manager.py:52`, `migration_manager.py:301, 322`, `file_operations.py:55`). ## 4. Tooling and docs - `.pre-commit-config.yaml` lists `pyupgrade` twice with conflicting `--py38-plus` and `--py36-plus`, and `default_stages: [commit, push]` are the old stage names (`pre-commit`/`pre-push` since pre-commit 3.2). - `CHANGELOG.md` stops at 0.12.1 while `pyproject.toml` is 0.12.2, and the `[0.10.4]` link compares `v0.10.0...v0.10.1`. - `Dockerfile` uses an unpinned `FROM ubuntu`, and `album index` at build time needs network access to add the default catalog. ## Related - album-solution-api!16 and !290 fix the "required argument in `pre_test()`" ordering bug in both runners; they are separate from everything above. Thanks for maintaining album! Tell me which of these you would like as MRs and I will open them one at a time so they stay easy to review. 🤖 Generated with [Claude Code](https://claude.com/claude-code)