[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