OpenConceptLab/ocl_online#247 | repo version-create --match-algorithms, and a warning on version-update --match-algorithms - #12
Conversation
…s, and a warning on version-update --match-algorithms - `repo version-create --match-algorithms es,llm` sets the new version's match algorithms in the create request. Without it the request leaves them out, so the server decides (a new source version is vectorized when HEAD or the latest release is). - `repo version-update --match-algorithms` warns on stderr that changing an existing version's match algorithms makes the server reindex its concepts, and points to version-create instead. - README: the vectorized-release example uses version-create. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VRGfMH9ZR6TGNMSfiPyUaF
paynejd
left a comment
There was a problem hiding this comment.
Codex adversarial review, pass 1 (codex-cli 0.160.0, commit 8622165)
-
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,llmsendsmatch_algorithmsto/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 collectionbefore 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. -
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 breakingocl --json … | jq. Fix: assert thatjson.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...HEADand 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_algorithmsfrom 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.
…lections; the warning keeps --json output clean Codex review, pass 1: - `repo version-create` and `repo version-update` refuse `--match-algorithms` with `--type collection` (a usage error, nothing sent): collections have no match algorithms (Medium). Help and README say "sources only". - The version-update warning test now also checks stdout is the JSON response (Low). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VRGfMH9ZR6TGNMSfiPyUaF
|
Codex pass 1: dispositions (fixed in c808ed7)
An empty |
paynejd
left a comment
There was a problem hiding this comment.
Codex adversarial review, pass 2 (codex-cli 0.160.0, commit c808ed7)
- 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 usingclick.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_algorithmskey 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.
…d instead of clearing them Codex review, pass 2 (Medium): `--match-algorithms ""` (an unset shell variable, ",,", spaces) sent `[]`, which overrides inheritance on create and opts a version out on update. Both commands now refuse it as a usage error; opting out is `--match-algorithms es`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VRGfMH9ZR6TGNMSfiPyUaF
|
Codex pass 2: dispositions (fixed in 2d457cb)
|
paynejd
left a comment
There was a problem hiding this comment.
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 omitsmatch_algorithmsfrom 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.
Linked Issue
Part of OpenConceptLab/ocl_online#247 (the API side is OpenConceptLab/oclapi2#926).
Summary
ocl repo version-create --match-algorithms es,llmsends 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 or the latest release is.ocl repo version-update --match-algorithmsnow warns on stderr before sending the change. Changing an existing version's match algorithms makes the server reindex its concepts: addingllmembeds them, which can take hours for a large repository, and removing it can drop their vectors. The warning points toversion-create --match-algorithms.--match-algorithmsis 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.version-create, and a short note explains why.The API already accepts
match_algorithmsin the version-create body, so the flag works against today's server too.Test Plan
tests/test_repo_commands.py(8 tests):match_algorithmsonly when it's given;version-updatewarns with--match-algorithms, stays quiet without it, and keeps--jsonstdout parseable;--match-algorithmsis refused for collections, and so is an empty value.pytest: 9 passed.ruff check src tests: no new findings (the same 20 asmain).