[snyaggarwal]

Part of OpenConceptLab/ocl_issues#3010 (web: OpenConceptLab/oclweb3#96). Reworked for the ACs Jon rewrote on Nov 5.

Collection version create takes an optional expansion: lock, latest or none; any other value is a 400.

• With EXPANSION_LOCKED_REPO_VERSION_TOGGLE on and no expansion, the default is lock when HEAD has an expansion, else latest. autoexpand: false alone still means no expansion.
• lock and latest start from HEAD's expansion parameters and evaluate every reference again (force_reevaluate, _async on each reference). Nothing is copied from HEAD's expansion.
• lock adds <url>|<version> to system-version for each source HEAD's expansion locked, unless HEAD's parameters already name it. The URL is the canonical URL when it resolves back to that version, else the OCL URL. It also starts from HEAD's evaluated collection versions, since system-version doesn't reach value sets.
• latest resolves unversioned references to each repo's latest release, else its HEAD.
• none matches autoexpand: false. With the toggle off, nothing changes.

Deploys. seed_children_to_new_version keeps its four arguments. The choice is recorded in the task's kwargs, so a worker on older code takes the message as before and a new worker reads it back; the task hooks now keep recorded kwargs when a message carries none.

Tests check member and evaluated versions: after a new release, lock keeps the locked release's member and a rebuild gives the same, and latest gives the new release's. Also the default, the task-record path, every API choice with the toggle on and off, and the 400. Pylint 10/10; collections, sources and tasks suites pass (773).

Known limit (as on the ticket): a later rebuild resolves unversioned value set references again.

🤖 Generated with Claude Code

[snyaggarwal]

Verdict: Approve

P1–P5 findings: none.

Nits: none.

The optional expansion contract matches the issue: omission preserves existing behavior; copy, generate, and none reach the seeding path with the intended precedence over autoexpand; invalid values fail validation. The new task argument defaults to the old behavior, so already-queued four-argument tasks and existing callers remain compatible. Coverage exercises every choice with the toggle on and off, plus invalid input. No open-core boundary concern.

Verification: reviewed the serializer/model/task flow and affected callers/tests with the code graph; Pylint and Snyk are green. The GitHub Tests check was still running at review time.

[paynejd]

The ACs on OpenConceptLab/ocl_issues#3010 changed after Jon's decision on Nov 5 (comment there). For the API:

1. Choices lock, latest and none, replacing copy and generate. With the lock toggle on and no expansion, the default is lock when HEAD has an expansion, else latest.
2. Both build choices evaluate every reference again: force_reevaluate=True, with _async = True on each reference, as the busy-HEAD branch does (models.py:459). Without _async, cascades stop at max_results (get_concept_cascade_params). Nothing is copied from HEAD's expansion for now.
3. Both start from HEAD's expansion parameters. Today the generate path creates the expansion with default parameters (Expansion.persist gets none).
4. lock adds <url>|<version> to system-version for each source HEAD's expansion locked: the version HEAD's unversioned references to that source resolve to. Use the source's canonical URL, or its OCL URL when it has none, and skip sources HEAD's parameters already name.
   • system-version only reaches unversioned references (can_compute_against_other_system_version), so versioned references keep their versions and a rebuild gives the same members.
   • It doesn't reach value sets: resolve_valueset_version reads the expansion's evaluated collection versions. So lock also starts the new expansion from HEAD's evaluated collection versions before evaluating. The busy-HEAD branch of seed_from_head_expansion_in_snapshot already does most of this.
5. Pass the choice down through cascade_children_to_expansion and Expansion.persist to seed_children_to_expansion(expansion_id, index, force_reevaluate), which already takes it. Capture the requested value before tasks.py:421 replaces seed_from_head.
6. Deploys. Production rolls out the API before the celery workers, so an old worker can receive the new five-argument seed_children_to_new_version message (models.py:1298). It fails after the API has returned 201, and the version gets no content. Send new arguments only when they're needed (for example only with the lock toggle on), or pass the choice as data.
7. Tests: check member versions. Release v1, add a reference to HEAD, change the concept and release v2, then create versions. lock gives v1 as the evaluated version and v1's concept version as the member, and a rebuild gives the same. latest gives v2 for both. On this head, generate gives v2 as the evaluated version with v1's concept version as the member.

[snyaggarwal]

Reworked in 7653a83 for the new ACs. Against the notes above:

1. Choices are lock, latest and none. With the toggle on and no expansion, the default is lock when HEAD has an expansion, else latest.
2. Both build choices evaluate every reference again with force_reevaluate=True and _async set. The copy path is gone, along with copy_members_from and has_pending_readd, which nothing else used.
3. Both start from HEAD's expansion parameters.
4. lock adds <url>|<version> for each source HEAD locked, skipping sources HEAD's parameters already name. It uses the canonical URL only when that resolves back to the same version, else the OCL URL. It also starts from HEAD's evaluated collection versions.
5. Different from the note: the build runs inside seed_children_to_new_version, as the busy-HEAD branch did, rather than through Expansion.persist and seed_children_to_expansion. seed_children with force_reevaluate clears every lock first, so it would drop lock's value set lock, or rebuild would have to change.
6. Deploys: the message keeps four arguments. The choice goes in the task record's kwargs, which an old worker ignores; the task hooks now keep recorded kwargs when a message has none.
7. Tests check member versions with the v1/v2 setup: lock gives v1 and v1's concept version, a rebuild gives the same, and latest gives v2 for both.

Members that resolve to a source's HEAD still become their latest concept version in the new version, as the copy path did.

🤖 Generated with Claude Code

[snyaggarwal]

Supersedes my review of f9266a5.

Verdict: Request changes.

P1: none.

P2 — Rebuilding a version that resolved a source HEAD does not reproduce the members created here (core/collections/models.py:467). Initial creation calls replace_head_resources, converting HEAD concept/mapping rows to their latest immutable resource versions. The normal rebuild path (expansion.seed_children(..., force_reevaluate=True)) never calls that normalization; with a saved system-version ending in |HEAD, reevaluation selects HEAD rows again. Extend test_seed_children_task_swaps_head_resources_in_references by rebuilding and asserting the expansion/reference still contain latest: they currently flip back to HEAD. This violates the primary reproducibility AC, and the ticket’s known exception covers value-set references only. Put the normalization in the reusable rebuild path or persist enough information for rebuild to select the same resource versions.

P3 — The omitted expansion default is resolved in the worker instead of when the version-create request locks HEAD (core/common/models.py:1304-1309). Only explicit choices are saved in Task.kwargs; an omitted choice is saved as None, so seed_children_to_new_version later rechecks whether HEAD has an expansion. If HEAD gains or loses its expansion while the task waits, the documented default silently flips between latest and lock. Resolve the default after select_for_update() and record the resolved choice with the task; add a non-TEST_MODE regression that changes HEAD before running the queued task.

P4: none.

P5: none beyond the regression coverage requested above.

Nits: none.

Verification: reviewed the rewritten ticket and both PR discussions; traced the model/task/rebuild flows with the code graph; inspected the member-version, rebuild, default and task-record tests; changed Python files compile; Pylint is green. GitHub Tests was still running at review time. No open-core boundary concern.

[snyaggarwal]

Both findings from the review of 7653a83 checked and fixed in 8662e1a.

P2: rebuild flips HEAD-resolved members back to HEAD rows. Valid. A test reproduced it: after creating a version whose source resolves to HEAD, seed_children(force_reevaluate=True) put the HEAD concept row back in place of its latest resource version. The copy path on master had the same gap.

• Fix: a rebuild of a non-HEAD version's default expansion now swaps HEAD rows for their latest resource versions, as creating the version does, and indexes after the swap. With the lock toggle off, or for other expansions, a rebuild is unchanged. dry_run_rebuild goes through the same path, so its preview matches.
• Test: test_rebuild_keeps_the_resource_versions_a_head_resolved_version_was_created_with, for both lock and latest, plus the toggle-off case.

P3: the omitted default was worked out in the worker. Valid. With no expansion, None was recorded and the task checked HEAD again when it ran.

• Fix: persist_new_version settles the choice right after it locks HEAD and records the result with the task.
• Test: test_persist_new_version_settles_the_default_before_the_task_runs records lock, removes HEAD's expansion, runs the task, and it still builds with lock. The sync-path test now expects the settled latest.

Pylint 10/10; core.collections, core.integration_tests.tests_collections, core.sources and core.tasks pass (775).

Still to do: this PR conflicts with #981 in one hunk of Expansion.add_references, and is behind master. Whichever merges second also resolves lock's entries in the owner namespace.

🤖 Generated with Claude Code

[snyaggarwal]

Re-review of 8662e1a against the rewritten Nov 5 ACs and the current PR description.

Verdict: Approve.

Findings:

• P1: none
• P2: none
• P3: none
• P4: none
• P5: none
• Nits: none

The prior rebuild/member-version finding is fixed: forced rebuilds of default version expansions now normalize HEAD resources again, with a regression test proving the locked member survives a rebuild. The prior queued-default race is also fixed: the default choice is settled while HEAD is locked, recorded on the task, and covered by a task-path regression test. The rewritten expansion semantics and old-worker-compatible four-argument task message match the posted ACs.

Validation: changed Python files compile with Python 3; Snyk is green. The latest Pylint job is still pending, so this verdict assumes that required check completes successfully.

This supersedes my earlier request-changes review.

[snyaggarwal]

Red build on 8662e1a fixed in 3bc047d. The new test_persist_new_version_settles_the_default_before_the_task_runs ran its setup under TEST_MODE=False, so add_expressions queued index_expansion_concepts, whose celery_once lock needs Redis, which GitHub Actions does not have. Now only the version-create call runs as outside tests, and both task-record tests mock the index signal tasks (handle_save, handle_m2m_changed). Checked locally with Redis unreachable: both pass, and the collections and tasks suites fail no more tests than master does without Redis. With Redis, 775 tests pass; pylint 10/10.

🤖 Generated with Claude Code

[snyaggarwal]

Two more fixes in ec5f893, each reproduced first with a test that fails without it:

1. A rebuild capped cascades that creation didn't. Creating a version with lock or latest sets _async on each reference, so cascades run without the 1,000-result cap, but seed_children(force_reevaluate=True) loaded the references without it, so a rebuild could drop members past the cap. A rebuild of a version's default expansion (lock toggle on, the same case that swaps HEAD rows) now sets _async too. Test: test_rebuild_cascades_without_a_result_cap_as_creation_does. Master's rebuild has the same cap; with the toggle off, and for other expansions, nothing changes.
2. lock could add a second version of a source. If HEAD's system-version names a source with a version that doesn't resolve (S|v9 or /S/v9/), the helper didn't treat that source as named and appended S|v1. It now works out the repository from the entry's URL without the version, as OpenConceptLab/ocl_issues#2943 | Expansion system-version resolves in the owner namespace and rejects two versions of one source #981's conflict check does, so it keeps the single entry. Test: test_locked_system_version_parameter_keeps_a_source_named_with_a_version_that_doesnt_resolve.

pylint -j0 core/ exits 0; collections, collection and value set integration, sources and tasks suites pass (778).

For the merge with #981: lock's helper resolves entries globally here, while #981 resolves them in the owner namespace. Whichever merges second makes resolve_system_version_entry resolve in the owner namespace too, so the URL lock writes is the one the build resolves to the locked version.

🤖 Generated with Claude Code

[snyaggarwal]

Merged master (with #981) in 16879b6, taking #981's side of the one conflicting hunk in Expansion.add_references: system-version entries resolve in the collection owner's namespace, and an unresolved entry is cached as None. Then, in 96cad6a:

• F3.1, rebuild cascade cap. The fix from ec5f893 stands (a rebuild that swaps HEAD rows sets _async, as creation does). New test test_rebuild_keeps_members_past_the_cascade_cap_as_creation_does: a root with 1,000 children, one of which has a child. Creation keeps the grandchild, and so does a rebuild; without the fix the rebuild drops it.
• FX.1, lock resolved its entries globally. get_locked_system_version_parameter now resolves in the owner's namespace, the same one the build uses: to find which sources HEAD's parameter already names, and to decide whether a source's canonical URL comes back to the locked version (else it writes the OCL URL). Test test_lock_keeps_heads_version_when_the_owner_registry_maps_its_canonical_url_elsewhere: the owner's registry and the global registry map the canonical URL to different repos, and the new version keeps HEAD's locked version and member. Resolving globally, it writes the canonical URL and the build moves to the other repo.
• FX.2, a second version of one source. The lock helper and the conflict check now share one rule for which source an entry names (get_system_version_entry_repos), so S|v9 with v9 missing names S and the helper adds nothing for S. The helper also runs the conflict check on what it generates, and if that would add a conflict HEAD's own parameter doesn't have, it keeps HEAD's parameter. Tests: test_locked_system_version_parameter_keeps_a_source_named_with_a_version_that_doesnt_resolve and test_locked_system_version_parameter_never_adds_a_conflict.

pylint -j0 core/ exits 0; collections, collection and value set integration, URL registry, sources and tasks suites pass (834).

🤖 Generated with Claude Code

[snyaggarwal]

Delta re-review of 96cad6a since the previously approved 8662e1a, including the master/#981 merge and its follow-up resolution.

Verdict: Approve.

Findings:

• P1: none
• P2: none
• P3: none
• P4: none
• P5: none
• Nits: none

The post-approval changes resolve the remaining edge cases cleanly:

• rebuild now gives cascades the same uncapped evaluation as version creation, so members beyond the former 1,000-result boundary are preserved;
• lock generation uses #981's owner-scoped system-version resolution, falls back to the OCL URL when a canonical URL resolves elsewhere, and retains unresolved entries as naming their source;
• the shared repository-identification/conflict logic prevents lock generation from adding a second version of a source;
• the task-record test adjustment limits TEST_MODE=False to the queueing path and mocks only the indexing signal handlers, preserving the behavior under test.

Validation: the seven new/adjusted focused regressions pass, and the two affected test classes pass locally (55 tests). GitHub Pylint and Snyk are green. The full GitHub Tests job is still running, so this verdict assumes that required check completes successfully. No open-core boundary concern.

This supersedes my review of 8662e1a for the current head.