OpenConceptLab/ocl_online#247 | repo version-create --match-algorithms, and a warning on version-update --match-algorithms

[paynejd]

Linked Issue

Part of OpenConceptLab/ocl_online#247 (the API side is OpenConceptLab/oclapi2#926).

Summary

• ocl repo version-create --match-algorithms es,llm sends the new version's match algorithms in the create request, so a release can be created vectorized in one step. Without the flag the request leaves them out and the server decides. Once OpenConceptLab/ocl_online#247 | Release-level vectorization: vectors for every semantic repo version, reused instead of re-encoded oclapi2#926 is deployed, a new source version is vectorized when HEAD is.
• ocl repo version-update --match-algorithms now warns on stderr before sending the change. Changing an existing version's match algorithms makes the server reindex its concepts: adding llm embeds them, which can take hours for a large repository, and removing it can drop their vectors. The warning points to version-create --match-algorithms.
• Sources only, never empty. --match-algorithms is refused for --type collection, which has no match algorithms, and refused when empty (an unset shell variable would otherwise clear them). Opting out is --match-algorithms es; opting in, for example when HEAD isn't vectorized, is --match-algorithms es,llm.
• README: the examples opt in (es,llm) and out (es) with version-create, and a short note explains why to set it there rather than with version-update.

The API already accepts match_algorithms in the version-create body, so the flag works against today's server too.

Test Plan

• New tests/test_repo_commands.py (8 tests):
  • the flag reaches the client as a list, and is left out by default;
  • the client body includes match_algorithms only when it's given;
  • version-update warns with --match-algorithms, stays quiet without it, and keeps --json stdout parseable;
  • --match-algorithms is refused for collections, and so is an empty value.
  • They capture stderr separately on Click 8.1 as well as 8.2+ (both satisfy click>=8.1.0).
• pytest: 9 passed, on Click 8.3.1 and on 8.1.8.
• ruff check src tests: no new findings (the same ones as main).
• Codex adversarial review: 3 passes posted on this PR. Pass 3 is clean.
• Codex review after the wording update for the revised server rule (HEAD only): pass 4 found the Click 8.1 test issue (P2), fixed in 76d98de; pass 5 is clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VRGfMH9ZR6TGNMSfiPyUaF

[paynejd]

Codex adversarial review, pass 1 (codex-cli 0.160.0, commit 8622165)

1. Medium — src/ocl_cli/commands/repo.py:206–209; src/ocl_cli/api_client.py:929–930: source-only option is forwarded to collections.
   version-create O R v1 --type collection --match-algorithms es,llm sends match_algorithms to /orgs/O/collections/R/versions/, although collections have no such field. The update command likewise forwards it and prints an inapplicable reindexing warning. Depending on server validation, the request either fails or succeeds without enabling the requested matching behavior. Fix: reject this flag with --type collection before making a request, identify it as source-only in help/README, and test both commands. Consider guarding direct API-client calls too. The exact server response could not be verified offline.

2. Low — tests/test_repo_commands.py:62–70: the warning test does not protect JSON stdout.
   It checks that stderr contains a warning but never parses stdout. An implementation that prints the warning to both streams would still pass while breaking ocl --json … | jq. Fix: assert that json.loads(result.stdout) equals the expected response, alongside the stderr assertions. The current implementation passes this stronger check in my independent reproduction.

What I checked and found correct:

• Ran git diff origin/main...HEAD and inspected command dispatch, request construction, and output handling.
• Whitespace is stripped and empty comma-separated items are discarded: " es, , llm, " becomes ["es", "llm"].
• An explicit empty string or comma/whitespace-only value becomes [] and is sent. This is distinct from omission; callers using an empty shell variable will explicitly override server defaults. Clearing behavior is undocumented, but the supplied requirements do not establish that [] must be rejected.
• Omitting the flag actually omits match_algorithms from the HTTP body. The client-level test correctly verifies this.
• Update warnings go to stderr; stdout remains parseable JSON. Updates without the flag produce no warning.
• Source creation uses the correct versions POST endpoint; update uses the version PATCH endpoint.
• README’s default-vectorization and reindexing explanations match the supplied server behavior. The diff introduces no credentials or private tracker references into the public repository.
• All 7 tests passed with bytecode writes disabled. Mocked HTTP reproductions confirmed the edge cases above. No files were modified and no network access was used.

No high-severity findings.

[paynejd]

Codex pass 1: dispositions (fixed in c808ed7)

1. Medium: the source-only option is forwarded for collections. Fixed. version-create and version-update now refuse --match-algorithms with --type collection as a usage error (exit 2), and send nothing. Help and README say "sources only". Test: test_match_algorithms_are_refused_for_collections.
2. Low: the warning test didn't protect JSON on stdout. Fixed. The test now parses stdout as the JSON response.

An empty --match-algorithms "" still sends [], so an explicit empty value stays distinct from leaving the flag out. That's deliberate.

[paynejd]

Codex adversarial review, pass 2 (codex-cli 0.160.0, commit c808ed7)

1. Medium — src/ocl_cli/commands/repo.py:185: Empty input silently becomes an explicit empty override.
   --match-algorithms "", whitespace-only input, and ",," all produce []. Both commands forward that list; the API client includes it in the request body. For example, a script using --match-algorithms "$ALGORITHMS" with an unset variable sends {"match_algorithms":[]}. On creation this overrides server inheritance; on update it requests clearing the algorithms, potentially removing vectorized matching. The current tests pass without covering this case. Fix: reject values with no nonempty algorithms using click.UsageError; add tests for empty, whitespace-only, and comma-only inputs on both commands. If clearing is intentional, document and test it explicitly.

What I checked and found correct:

• Both pass-1 fixes hold: collections with the flag fail with exit code 2 before either client method runs, and the warning test parses JSON stdout.
• Collections without the flag remain usable.
• Whitespace is trimmed; empty items within "es,,llm," are discarded, yielding ["es", "llm"].
• Omitting the flag results in no match_algorithms key in the create POST body.
• Update warnings go to stderr; mocked JSON stdout remains parseable.
• README wording matches the supplied server behavior and source-only restriction. No secrets or private infrastructure details appeared in the diff.
• All 7 changed tests passed. Additional mocked probes confirmed the parsing edge cases above.

No files were modified and no network was used. Actual server handling of an empty list could not be verified offline.

[paynejd]

Codex pass 2: dispositions (fixed in 2d457cb)

1. Medium: an empty value silently becomes an explicit empty override. Fixed.
   • Both commands now refuse an empty, whitespace-only or comma-only --match-algorithms as a usage error (exit 2), and send nothing. The usual cause is an unset shell variable.
   • Opting a version out is --match-algorithms es.
   • Test: test_empty_match_algorithms_are_refused.

[paynejd]

Codex adversarial review, pass 3 (codex-cli 0.160.0, commit 2d457cb): clean

No significant findings in git diff origin/main...HEAD. The pass 2 empty-value issue is fixed.

Checked and found correct:

• Both commands reject empty, whitespace-only, and comma-only values before making an API call.
• Parsing trims whitespace and drops empty items: " es , , llm, " becomes ["es", "llm"].
• Omitting the create flag passes None, and the HTTP client omits match_algorithms from the POST body entirely.
• Update warnings go to stderr; stdout remains valid JSON with --json. Omitting the flag produces no warning or update field.
• Both commands reject --type collection --match-algorithms. Collections without the flag still work and omit the key.
• POST/PATCH endpoints and payloads are correct in mocked request checks.
• README matches the supplied server behavior and clearly states the source-only restriction. No secrets or private infrastructure details appear in the diff.

All 9 tests passed. Coverage could improve for mixed empty items and collection success without the flag; additional read-only checks confirmed both behaviors. Server behavior could not be independently verified offline.

No files modified; no network access used.

[paynejd]

Codex review, pass 4

codex review -c model_reasoning_effort=xhigh --base origin/main (codex-cli 0.160.1) on bef582e, after the wording update for the revised server rule (a new version is vectorized when HEAD is).

All nine tests pass with Click 8.3.1, but the new tests fail under the still-supported Click 8.1 series. No production-code regression was identified.

• [P2] Capture stderr separately on supported Click 8.1 releases — tests/test_repo_commands.py:43
  With Click 8.1.x, which satisfies click>=8.1.0 in pyproject.toml, CliRunner() defaults to mixing stderr into stdout rather than capturing it separately. Consequently, the new tests accessing result.stderr raise ValueError("stderr not separately captured"), and the warning also contaminates result.stdout. Use version-compatible separate-stderr capture or raise the minimum Click version to 8.2.

The wording changes themselves drew no findings.

[paynejd]

Codex pass 4: disposition

• [P2] Capture stderr separately on Click 8.1: fixed in 76d98de. Reproduced: with Click 8.1.8, 5 of the 9 tests failed with ValueError: stderr not separately captured. The tests now build their runner with CliRunner(mix_stderr=False) where that argument exists, and fall back to CliRunner() on Click 8.2+, which dropped it and always keeps the streams apart. That keeps the click>=8.1.0 floor rather than raising it. 9 passed on both Click 8.1.8 and 8.3.1; ruff reports nothing new.

[paynejd]

Codex review, pass 5

codex review -c model_reasoning_effort=xhigh --base origin/main (codex-cli 0.160.1) on 76d98de, after the pass 4 fix. Clean.

No actionable regressions were found in the diff against the supplied merge base. All 9 tests pass, and the new payload handling and stderr warning preserve existing command behavior and JSON output.