Skip to content

Add test for var name collision with package name#1361

Open
jrray wants to merge 1 commit into
mainfrom
var-name-collision-test
Open

Add test for var name collision with package name#1361
jrray wants to merge 1 commit into
mainfrom
var-name-collision-test

Conversation

@jrray
Copy link
Copy Markdown
Collaborator

@jrray jrray commented Apr 30, 2026

Summary

  • Adds a test demonstrating a bug in the step-based solvers (cli, checks) where a namespaced var's base name collides with a package name in VarRequirementsValidator
  • When middle install-requires var: demo.samenameaspkg/true and the solver's option map contains samenameaspkg=~1.2.3 (from resolving the samenameaspkg package), the validator matches by base_name() and incorrectly rejects the build
  • The resolvo solver handles this correctly; the test expects success for resolvo and failure for cli/checks

Bug details

The root cause is in crates/spk-solve/crates/validation/src/validators/var_requirements.rs line 26:

let is_not_same_base = request.var.base_name() != name.base_name();

This causes demo.samenameaspkg (a namespaced var) to match the option samenameaspkg (a package version) because they share the base name samenameaspkg.

Test plan

  • cargo test -p spk-cmd-build test_build_var_name_collides_with_package_name passes (resolvo succeeds, cli/checks correctly fail)

🤖 Generated with Claude Code

@jrray jrray self-assigned this Apr 30, 2026
@jrray jrray added bug Something isn't working AI Code authored with AI assistance. labels Apr 30, 2026
When a namespaced var shares its base name with a separate
package (e.g. demo defines var 'samenameaspkg' and package
'samenameaspkg/1.2.3' also exists), the step solver's
VarRequirementsValidator incorrectly rejects builds. It
matches option keys by base_name, so
demo.samenameaspkg=true collides with samenameaspkg=~1.2.3.

The test passes for the resolvo solver and correctly
expects failure for the cli/checks (step-based) solvers.

Signed-off-by: J Robert Ray <jrray@imageworks.com>
@jrray jrray force-pushed the var-name-collision-test branch from d2cc499 to e81cd37 Compare April 30, 2026 21:27
@jrray
Copy link
Copy Markdown
Collaborator Author

jrray commented Apr 30, 2026

@rydrman I found a case where the solver implementations disagree, the PR is a little biased to claim that resolvo is correct but this is up for debate. I could use your input because it is about the treatment of namespaced vs non-namespaced vars.

If a package a defines a var b/true that happens to be the same name as the name of a package, a conflict arises in the event that a consumer package wants to include both packages a and b in the same environment. Resolving b causes the solver state to contain b = 1.2.3 and then that prevents a from being resolved because a wants a.b = true.

Do we say that packages are not allowed to define vars that have the same name as other packages, which requires a certain level of omniscience, or do we start treating b and a.b as distinct vars [in the step solver]?

Resolvo already treats them as distinct and this doesn't run afoul of any of the existing solver tests. But I chalk that up to there being missing tests that demonstrate the desired behavior.

My own two cents, I think if two different packages both define a var with the same name, it shouldn't be assumed that this was intentional and that the vars are sharing a meaning or trying to coordinate. I prefer a pattern where if two or more packages want to coordinate on the same var then there should be an additional package foo in the mix to define the var, and then all the packages trying to coordinate all use foo.varname making the coordination explicit.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 30, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@rydrman
Copy link
Copy Markdown
Collaborator

rydrman commented May 13, 2026

It definitely feels to me like there is a bug here, mainly in that I don't see how it's reasonable not to also consider the namespaces here when comparing the request to the var. Unless I'm not reading this code right or missing some key context about where this code is executed... Would considering the namespace as part of this resolve the conflict?

https://github.com/spkenv/spk/blob/main/crates/spk-solve/crates/validation/src/validators/var_requirements.rs#L26

@jrray
Copy link
Copy Markdown
Collaborator Author

jrray commented May 15, 2026

It definitely feels to me like there is a bug here, mainly in that I don't see how it's reasonable not to also consider the namespaces here when comparing the request to the var. Unless I'm not reading this code right or missing some key context about where this code is executed... Would considering the namespace as part of this resolve the conflict?

https://github.com/spkenv/spk/blob/main/crates/spk-solve/crates/validation/src/validators/var_requirements.rs#L26

Comparing the whole thing is how resolvo is wired up which is why it passes this test. If you consider this a bug in the step solver (I prefer this stance) then I can make them both consistent by taking the resolvo behavior as correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI Code authored with AI assistance. bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants