[snyaggarwal]

OpenConceptLab/ocl_issues#2943 (part of the expansion-health epic OpenConceptLab/ocl_issues#2942)

What changes

1. system-version resolves in the collection owner's namespace. Expansion.add_references resolves each entry with Source.resolve_reference_expression(entry, namespace), as a collection's own references resolve. The namespace is the collection version's owner, used only while URL_REGISTRY_IN_RESOLVE_REFERENCE_TOGGLE is on: without the registries a namespace would limit canonical URLs to the owner's own repositories (the same rule $resolveReference follows for ?namespace=), so toggle-off behaviour is unchanged.

   • Toggle on: owner registry → owner's repositories → global registry. A canonical URL only the owner's registry knows now resolves instead of landing in unresolved_repo_versions.
   • An entry the global registry knows resolves as before.

2. Two different versions of one source in system-version is a 400 on expansion create (POST …/expansions/), and no expansion is created. This follows FHIR (Jon, Nov 3). Entries are compared by the source they resolve to, so a canonical URL and an OCL version URL naming the same version don't conflict. A value set whose own references name both versions stays valid; add_references itself still accepts both, so existing expansions with such parameters keep re-evaluating as before. A non-string system-version (which add_references couldn't split) is also a 400.

   • FHIR $expand already rejects system-version as not supported, and expansion create is the only OCL endpoint that accepts expansion parameters, so no other entry point needed the check.

3. References without a namespace of their own resolve in the collection owner's namespace while URL_REGISTRY_IN_RESOLVE_REFERENCE_TOGGLE is on, as OCL Docs' $resolveReference page says (Jon, Nov 5). CollectionReference.resolution_namespace is the reference's own namespace if it has one, else the same owner namespace system-version uses; it's worked out when resolving and never saved, so nothing changes with the toggle off. It applies wherever a reference resolves its system or value sets: resolve_expression_system_version, resolve_valueset_version, the reference cache key in get_ref_system_version, and the two paths that show a reference's resolved versions. Unresolved entries now record the namespace the lookup used.

Example error:

{"parameters": {"system-version": ["Names more than one version of /orgs/X/sources/S/: https://s.org|v1, https://s.org|v2. Use only one version of a source in system-version."]}}

Tests

• ExpansionSystemVersionNamespaceTest: owner-registry resolution with the toggle on, the owner registry ignored with the toggle off, unresolved without any entry, the global registry entry (unchanged), and the conflict helper.
• CollectionVersionExpansionsViewTest.test_post_system_version_with_two_versions_of_one_source_400.
• Jon's probe: the owner's registry and the global registry map one URL to different repos, and a <url>|v1 entry applies whether the reference names the owner's namespace or none (it failed for no namespace before). Plus resolution_namespace with the toggle on and off.
• Ran: core.collections, core.integration_tests.tests_collections, core.url_registry, core.repos, core.sources (770 tests). All pass. pylint 10/10 on the changed files.

Sibling PRs

• OpenConceptLab/ocl_issues#2943 | Expansion system-version: owner namespace and one version per source ocl-docs#81 (docs: collectionReferenceEvaluation.md "How OCL picks a source version", and the 400 in expansions.md)

🤖 Generated with Claude Code

[snyaggarwal]

Verdict: Request changes

Issue complete: not yet. The main namespace-resolution and duplicate-version behavior are implemented cleanly, but the new validation still accepts falsey non-string system-version values ([], {}, 0, false) and creates the expansion. I left one inline P2 finding with the fix and regression-test suggestion.

Security blockers: none. Open-core boundary concerns: none. No additional nits.

Verification: CI Pylint and Snyk pass; CI tests are still running. Host-side syntax validation passed. I could not complete an independent container test run because the existing container has stale/truncated copies of three changed files.

(snyaggarwal • Codex review)

[snyaggarwal]

Verdict: Approve

Re-reviewed at 2c8713f8. The prior P2 is fixed: only None and the empty string are treated as absent, every non-string value (including falsey JSON values) returns 400, and the regression test covers a non-empty list, [], {}, 0, and false. The owner-namespace resolution and duplicate-source-version rejection still match OpenConceptLab/ocl_issues#2943 and the sibling docs.

Findings/nits: none. Security blockers: none. Open-core boundary concerns: none.

Verification: host-side syntax validation passed; Snyk passes. CI Pylint is still running and tests have not started, so merge after CI is green. The existing local API container remains unusable for independent tests because its mounted copies of three changed files are stale/truncated.

(snyaggarwal • Codex re-review)

[paynejd]

One more change, from Jon on Nov 5 (now a to-do on OpenConceptLab/ocl_issues#2943): a reference without a namespace of its own should resolve in the collection owner's namespace, as OCL Docs' $resolveReference page says ("when expanding a ValueSet, the namespace is set to the owner"). Today references resolve in their own stored namespace (resolve_expression_system_version), and nearly all have none. So this PR's owner-namespace system-version entries can disagree with them.

A probe with the registry toggle on, where the owner's URL registry and the global URL registry map one canonical URL to different repos, and the system-version entry is <url>|v1:

Reference's namespace	master	this PR
owner	entry ignored	entry applied
none	entry applied	entry ignored, with nothing in unresolved_repo_versions

Fix: one namespace for everything an expansion resolves. That's a reference's own namespace if it has one, else get_system_version_namespace(<owner url>) (models.py:1777).

• Apply it everywhere a reference resolves its system or value sets: resolve_expression_system_version, resolve_valueset_version (models.py:1275), and the reference cache key in get_ref_system_version.
• Work it out when resolving rather than saving it on the reference, so nothing changes with the toggle off. There, a namespace limits resolution to the owner's repos, so references to other orgs' repos would stop resolving.
• With this, OpenConceptLab/ocl_issues#2943 | Expansion system-version: owner namespace and one version per source ocl-docs#81's sentence about the collection's references is right as written.

Test: the case above, with the entry applied in both cases.

[snyaggarwal]

Done in a9be572, following the note above:

• CollectionReference.resolution_namespace is the reference's own namespace if it has one, else get_system_version_namespace(<owner url>). It's worked out when resolving and never saved, so with the toggle off nothing changes.
• Used in resolve_expression_system_version, resolve_valueset_version and the cache key in get_ref_system_version, and also in the two paths that show a reference's resolved versions (_get_expression_system_version, _get_resolved_valueset_versions), so the API shows what the expansion resolved.
• Unresolved entries now record the namespace the lookup used, so one existing expectation in test_add_references_task changed from None to the owner's namespace. Nothing reads that field to match on.
• Test: your probe, with the entry applied for both an owner-namespace and a no-namespace reference. On the previous head it failed for no namespace, as in your table.

Pylint 10/10; core.collections, core.integration_tests.tests_collections, core.url_registry, core.repos and core.sources pass (770).

Heads-up for #980 (#3010): the two PRs conflict in one hunk of Expansion.add_references (the system-version loop), and #980's lock helper resolves its entries without a namespace. Whichever merges second keeps this PR's loop and resolves lock's entries in the owner namespace too, so they match what the build resolves.

🤖 Generated with Claude Code

[snyaggarwal]

Verdict: Request changes — do not merge yet

The new owner-namespace fallback is correct for stored/direct expansion references, but one P2 remains in collection reference creation: cascade+transform generation receives the collection context only when the expansion-lock toggle is on. With URL registries on and the lock toggle off, a namespace-less reference can still resolve globally before its collection is assigned and generate children from the wrong repository. I left the reproduction conditions, fix direction, and test gap inline.

Security blockers: none. Open-core boundary concerns: none.

Merge recommendation: fix the P2 and wait for CI to pass. Also reconcile the overlapping Expansion.add_references hunk with #980 so whichever PR merges second preserves owner-namespace resolution for both system-version and lock.

(snyaggarwal • Codex re-review)

[snyaggarwal]

Review fixes in 9f92485, each checked first (probe tests for the first two):

1. Unresolved system-version entry crashed a matching reference. With a reference whose system is the same string as an unresolved entry, the cache handed back an unsaved Source and evaluation raised ValueError: Cannot use None as a query value. It did so for a reference without a namespace on master too, and since a9be572 also for owner-namespace references. Now an unresolved entry is cached as None, as resolve_expression_system_version returns, and stays listed in unresolved_repo_versions.
2. Conflicts got past the 400 when a version was missing. url|v1,url|v2 with v2 not installed (or neither installed) found no conflict. Entries that don't resolve now count under their URL without the version, so those are a 400. A canonical URL and an OCL URL for the same version still don't conflict, nor does the same entry twice.
3. Generated references got their collection context only with the lock toggle on (the inline P2). Fixed as replied there.
4. Generated references dropped an explicit namespace. Cascade-and-transform built them without namespace, so an explicit global / now fell back to the owner. They now keep the input reference's namespace.

Red build on a9be572: test_posting_value_sets expected the old resolution. Its CodeSystem is in the value set owner's org: with the registry toggle on it used to be looked up globally and not found, and now it resolves in the owner's namespace, so compose.include names the version it resolved to (0.2.0). The test now expects that.

Tests for each; pylint 10/10; collections, collection and value set integration suites pass (418). Locally 7 other tests (GraphQL global search, an importer test, OpenID auth) fail the same way without these changes, and didn't fail in CI.

OpenConceptLab/ocl-docs#81 needs no change for these.

🤖 Generated with Claude Code

[snyaggarwal]

Verdict: Request changes — do not merge yet

The prior cascade/transform owner-context finding is fixed correctly, including lock-toggle-off coverage and preservation of an explicit namespace on generated references. One new P2 remains in the added unresolved-version conflict logic: when both missing versions use different URL forms (canonical and OCL relative) for the same registered source, neither produces version.id, so they are never mapped into one repository bucket and the conflicting request is accepted. I left the concrete case, fix direction, and regression cases inline.

Findings: one P2. Nits: none. Security blockers: none. Open-core boundary concerns: none.

Merge recommendation: do not merge until the cross-form unresolved conflict is fixed and CI is green. When reconciling with #980, preserve owner-namespace resolution for both system-version and lock.

(snyaggarwal • Codex re-review)

[snyaggarwal]

Verdict: Approve — merge when CI is green

Re-reviewed at d7971302. The prior P2 is fixed: unresolved canonical and OCL URL forms are mapped to the same repository and requested-version identity, with regression coverage for different missing versions, the same missing version, and the resolved same-version case. The earlier cascade/transform owner-context fix remains correct, and the mutable-default test cleanup is sound.

Findings: none. Nits: none. Security blockers: none. Open-core boundary concerns: none.

Merge recommendation: merge once Pylint and tests pass. #980 is still open and overlaps Expansion.add_references; whichever PR merges second must rebase and preserve owner-namespace resolution for both system-version and lock, then rerun its checks.

Verification: Snyk passes and the changed Python files parse successfully; Pylint is currently running.

(snyaggarwal • Codex re-review)

[snyaggarwal]

Red Pylint on 4988742 fixed in d797130: the rewritten lock-context test passed a dict as a default argument (W0102). It now uses a helper with no mutable default. pylint -j0 core/ exits 0, and the collections, collection and value set integration, URL registry and sources suites pass (724).

🤖 Generated with Claude Code