[Refactor] Review comments from #12902

Open

#12,951 opened on May 13, 2026 0 comments 0 reactions 0 assignees View on GitHub

engine:v1 type:docs

Dominant language: Rust
Stars: 13.8k
Forks: 2.6k
Avg merge: 21h 31m
Merged PRs (30d): 56

Description

### Housekeeping

- [x] I am a maintainer of dbt-core

### Short description

[Copilot AI had a few code suggestions](https://github.com/dbt-labs/dbt-core/pull/12902#pullrequestreview-4245672637) while reviewing https://github.com/dbt-labs/dbt-core/pull/12902.

The code was lifted-and-shifted, so we intentionally wanted to keep it as-is rather than make any changes. This issue is to review those suggestions and adopt any that make sense.

---

[`core/dbt/deps/private_package.py`](https://github.com/dbt-labs/dbt-core/pull/12902/files/15470c0de1fc2c8d8025392a1af07b7748e0d8c4#diff-dd3069c35e5cdcd2327ef867845b8ec3f966d617e2259148484471289181ba59)

`GitProvider` overrides `__eq__` to make ADO variants equal, but keeps the default `Enum`/`StrEnum` hashing semantics. This violates the Python hash/eq contract (e.g., `GitProvider.ADO == GitProvider.AZURE_DEVOPS` but they will not behave as the same key in dicts/sets). Consider avoiding custom `__eq__` (use a normalization/matching helper instead) or implement a compatible `__hash__` based on the normalized provider value.

---

Comment on line R115

https://github.com/dbt-labs/dbt-core/pull/12902/changes#r3202933839

`PrivatePackageName._parts` will raise a `ValueError` if the input does not contain at least `org/repo` (e.g. missing `/`). Since `PackageConfig.validate` does not validate `private` values, this will surface as an unhandled exception. Recommend validating the format here (or in contract validation) and raising a `PrivatePackageResolutionError` with a clear message.

`core/dbt/deps/private_package.py`

```
head, *groups, tail = self.split("/")
--
parts = self.split("/")
if len(parts) < 2 or not parts[0] or not parts[-1]:
raise PrivatePackageResolutionError(
f"Invalid private package name '{self}'. Expected format 'org/repo' or "
"'org/group/.../repo'."
)
head, *groups, tail = parts
```

---

Comment on lines R360 to R366

https://github.com/dbt-labs/dbt-core/pull/12902/changes#r3202933959

For the `ado`/`azure_devops` SSH fallback, the Azure DevOps SSH URL format is `org/project/repo` (exactly 3 path segments). The current check only enforces a minimum of 3 parts, so inputs like `org/a/b/repo` will produce an invalid SSH URL without an error. Consider validating the part count is exactly 3 for these providers and raising a clear `PrivatePackageResolutionError` otherwise.

---

Comment on lines R383 to R388

https://github.com/dbt-labs/dbt-core/pull/12902/changes#r3202933896

`PrivatePinnedPackage.to_dict()` drops the `provider` field entirely. Since `package-lock.yml` is used as the source of truth on subsequent `dbt deps` runs, omitting `provider` means a locked private dependency can be re-resolved with `provider=None` (defaulting to GitHub SSH) and break installs for GitLab/Azure DevOps packages. The lock file should persist `provider` (when set) so re-installs are reproducible.

---

Comment on lines R413 to R425

https://github.com/dbt-labs/dbt-core/pull/12902/changes#r3202933925

`PrivateUnpinnedPackage.from_contract()` sets `git_unrendered=contract.private` and does not retain `contract.provider`. For consistency with `GitUnpinnedPackage.from_contract`, this should use the unrendered value (if present) and carry `provider` through to the pinned/lock representation; otherwise secrets/templating and provider-specific SSH fallback behavior can be lost when creating/using `package-lock.yml`.

---

Comment on lines R50 to R61

https://github.com/dbt-labs/dbt-core/pull/12902/changes#r3202934002

This test asserts the entire `package-lock.yml` contents including an exact commit SHA (`revision`) and `sha1_hash`. That makes the test brittle if the upstream repo/tag ever moves (or if lockfile formatting/hash inputs change). Consider parsing the YAML and asserting the important invariants instead (single package entry, `private` value, `revision` looks like a 40-char SHA, no token leakage) without hard-coding the exact SHA/hash.

---