feat(xtest): Lets otdf-sdk-mgr manage platform too#451
Open
dmihalcik-virtru wants to merge 14 commits into
Open
feat(xtest): Lets otdf-sdk-mgr manage platform too#451dmihalcik-virtru wants to merge 14 commits into
dmihalcik-virtru wants to merge 14 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds platform install tooling (git-bare + worktrees), git-ref helpers, ref-aware checkout/build plumbing, a scenario-driven install CLI that emits installed manifests, CLI updates ( ChangesPlatform Installation and Scenario Orchestration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces support for installing the OpenTDF platform service and scenario-driven installations. Key changes include the addition of a platform_installer module that manages source builds via git worktrees, a new cli_scenario module for manifest-based installs, and updates to existing CLI commands to handle the "platform" target. Review feedback highlights a bug in updating git worktrees from bare repositories, identifies code duplication in the CLI logic, suggests optimizing YAML parsing to avoid redundant reads, and recommends allowing real-time output for long-running build processes to improve user experience.
This was referenced May 15, 2026
X-Test Results✅ go-v0.15.0 |
dmihalcik-virtru
force-pushed
the
DSPX-3302-02-platform-installer
branch
from
c6a7895 to
ebc0c15
Compare
dmihalcik-virtru
force-pushed
the
DSPX-3302-02-platform-installer
branch
from
ebc0c15 to
14e5c1e
Compare
This was referenced May 18, 2026
dmihalcik-virtru
added a commit
that referenced
this pull request
May 21, 2026
#450) ## Summary First PR in a five-part stack that introduces a multi-instance test harness and a Claude plugin for OpenTDF bug reproduction. This PR adds *only* the shared Pydantic schema in `otdf-sdk-mgr` — no consumers yet. - Adds `otdf_sdk_mgr.schema` with v2 models: `Scenario`, `Instance`, `PlatformPin`, `KasPin`, `SdkPin`, `ScenarioSdks`, `Suite`, etc. - `ScenarioSdks.encrypt` / `.decrypt` mirror xtest's existing `--sdks-encrypt` / `--sdks-decrypt` convention so a→b-only scenarios are first-class. - `python -m otdf_sdk_mgr.schema validate <path>` validates either a Scenario or an Instance file based on its `kind:`. - Adds `pydantic` + `ruamel.yaml` to `otdf-sdk-mgr/pyproject.toml`. - 6 unit tests covering round-trips, pin invariants, and unknown-field rejection. ## Stack 1. [**This PR**](#450) — Shared schema 2. [Platform installer + `install scenario`](#451) in `otdf-sdk-mgr` (builds on this) 3. `otdf-local` [multi-instance refactor](#452) + new CLI subcommands 4. `xtest/conftest.py` [integration](#453) (`--scenario`, `--instance`) 5. [Claude plugin](#454) (`.claude/skills/`, settings, plugin manifest) 6. #455 ## Test plan - [x] `cd otdf-sdk-mgr && uv run pytest tests/test_schema.py` — all 6 pass - [x] `uv run python -m otdf_sdk_mgr.schema validate <path>` accepts a valid scenarios.yaml and rejects unknown fields Jira: https://virtru.atlassian.net/browse/DSPX-3302 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added schema validation for OpenTDF Scenario and Instance YAML configurations with a new CLI command. * Introduced strict validation with cross-field constraints for SDK and platform configurations. * **Documentation** * Updated supported container formats from `nano` to `ztdf-ecwrap`. * **Dependencies** * Updated core package dependencies to support enhanced validation capabilities. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/opentdf/tests/pull/450?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dmihalcik-virtru
force-pushed
the
DSPX-3302-02-platform-installer
branch
from
14e5c1e to
9993b12
Compare
dmihalcik-virtru
changed the title
X-Test Failure Report✅ java@main-main |
X-Test Failure Report |
dmihalcik-virtru
changed the title
X-Test Failure Report |
X-Test Failure Report |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
otdf-sdk-mgr/tests/test_cli_scenario.py (1)
107-126: ⚡ Quick winAdd a validation-error CLI test case.
Please add one test for schema-invalid (but syntactically valid) YAML to assert
install_scenario_cmdexits cleanly withtyper.Exit(1)and no traceback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@otdf-sdk-mgr/tests/test_cli_scenario.py` around lines 107 - 126, Add a new test in tests/test_cli_scenario.py that submits a syntactically valid but schema-invalid YAML to install_scenario_cmd and asserts it exits cleanly with typer.Exit(status=1) and no traceback; specifically, create a tmp_path scenario file containing SCENARIO_YAML with a deliberate schema violation, call install_scenario_cmd(scenario_path, ...) in a pytest.raises(typer.Exit) context and assert the raised exception has exit_code == 1 and that no exception other than typer.Exit is propagated (mirroring the pattern used in test_install_scenario_writes_partial_manifest_on_failure), referencing install_scenario_cmd and the tmp_path-based scenario file to locate where to add the test.otdf-sdk-mgr/tests/test_platform_installer.py (1)
8-23: ⚡ Quick winAdd a regression case for container-image-style refs.
Please add a parametrized case asserting container-image inputs are rejected (clear v1-not-supported error), so this behavior stays locked.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@otdf-sdk-mgr/src/otdf_sdk_mgr/cli_scenario.py`:
- Around line 57-72: The code only catches YAMLError from load_yaml_mapping but
lets other parse/validation errors escape; update the block around
load_yaml_mapping, the raw kind extraction, and the calls to
Scenario.model_validate and Instance.model_validate to also handle non-YAML
failures by validating that raw is a mapping (isinstance(raw, dict)) and
wrapping model_validate calls in a try/except that catches validation/parse/type
errors (e.g., ValueError/TypeError and the validation error your pydantic layer
raises) and in the except branch call typer.echo with a clear message including
the exception and then raise typer.Exit(1); ensure instance and scenario
variables are only used after successful validation.
---
Nitpick comments:
In `@otdf-sdk-mgr/tests/test_cli_scenario.py`:
- Around line 107-126: Add a new test in tests/test_cli_scenario.py that submits
a syntactically valid but schema-invalid YAML to install_scenario_cmd and
asserts it exits cleanly with typer.Exit(status=1) and no traceback;
specifically, create a tmp_path scenario file containing SCENARIO_YAML with a
deliberate schema violation, call install_scenario_cmd(scenario_path, ...) in a
pytest.raises(typer.Exit) context and assert the raised exception has exit_code
== 1 and that no exception other than typer.Exit is propagated (mirroring the
pattern used in test_install_scenario_writes_partial_manifest_on_failure),
referencing install_scenario_cmd and the tmp_path-based scenario file to locate
where to add the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7eb48abc-654d-4040-9cfe-925dec493035
📒 Files selected for processing (7)
AGENTS.mdotdf-sdk-mgr/src/otdf_sdk_mgr/cli_install.pyotdf-sdk-mgr/src/otdf_sdk_mgr/cli_scenario.pyotdf-sdk-mgr/src/otdf_sdk_mgr/platform_installer.pyotdf-sdk-mgr/src/otdf_sdk_mgr/schema.pyotdf-sdk-mgr/tests/test_cli_scenario.pyotdf-sdk-mgr/tests/test_platform_installer.py
3 tasks
dmihalcik-virtru
added a commit
that referenced
this pull request
May 22, 2026
## Summary Improves the agent-facing documentation across the repository so future Claude Code sessions land on accurate context faster. All changes are docs-only — no code touched. - **Root `AGENTS.md`**: new Repository Layout table at the top; new "Before Committing Python Changes" section requiring `uv run ruff check`, `uv run ruff format`, and `uv run pyright`, with the rationale that `uvx` strips the project venv and breaks pyright import resolution; trimmed the duplicated "Summary → Preferred Workflow" block. - **`otdf-local/AGENTS.md`**: flagged the manual-keys YAML block as an emergency fallback that may drift from the current platform schema. - **`xtest/AGENTS.md`** (new): test-suite layout, custom pytest options reference, audit-log fixture quick reference (`--no-audit-logs` vs `DISABLE_AUDIT_ASSERTIONS`), and test-authoring guidance. `xtest/CLAUDE.md` symlinks to it, matching the root convention. Scoped intentionally to content that's accurate on `main` today. A follow-up PR will add `otdf-sdk-mgr/AGENTS.md` (covering the platform installer and scenario workflows) once #451 merges. ## Test plan - [x] No source files modified — verified with `git diff --stat` - [x] `grep` for forward references to feature-branch-only code (`platform_installer`, `cli_scenario`, `install platform`, `install scenario`) returns no matches in these files - [x] All command snippets are commands that work against `main` today 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Updated onboarding guidance for the test framework, including repository layout and required pre-commit linting and type-checking procedures * Added comprehensive documentation for the cross-client integration test suite with pytest modules, fixtures, and custom options * Enhanced Golden Key Auto-Configuration documentation with emergency fallback configuration details <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/opentdf/tests/pull/459?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
dmihalcik-virtru
force-pushed
the
DSPX-3302-02-platform-installer
branch
from
ec1f655 to
13b5c96
Compare
`install scenario` could not run as written: it iterated `ScenarioSdks.union()` as a dict (it returns a list) and passed a `source=` kwarg `install_release` does not accept. The emitted `installed.json` shape also did not match what `scenario_to_pytest_sdks` reads (per-role lists, not sdk-name-keyed dict), so even the platform-only path produced a manifest no downstream tool could consume. Source fixes: - cli_scenario.py: iterate `union()` as the list it is, cache installs by (sdk, version, source), emit role-keyed lists matching the reader's expected shape; on failure write a partial manifest with `status=partial` so half-installed dist trees are diagnosable. Catch YAMLError in `_peek_kind` to surface a clean typer error. - platform_installer.py: `_git_rev_parse` raises on failure instead of silently writing an empty `sha=` into `.version`. Missing `scripts/` raises instead of warning-and-continuing. SHA passthrough heuristic tightened from `>=7` chars to exactly 40 (SHA-1) or 64 (SHA-256), so ambiguous short tags like `abc1234` no longer skip the `service/` prefix. Dropped a docstring fragment pointing to a planning doc that won't exist post-merge. - cli_install.py: dropped a docstring whose "deferred import" claim was false (the registration runs at module import). `lts platform` with no pinned version now exits 1 instead of warning-and-exit-0. Tests: - test_platform_installer.py: parametrized cases for `_resolve_platform_ref` covering version normalization, branch passthrough, the tightened hex heuristic, and SHA-1/SHA-256 passthrough. - test_cli_scenario.py: end-to-end smoke that mocks the installers and asserts the produced manifest is round-trip consumable by `scenario_to_pytest_sdks`. This is the gating test that would have caught the original bug. 79 passing (was 67). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- platform_installer: fix worktree update from bare repo (no `origin` remote exists), use `git reset --hard <branch>` instead of `git pull` - platform_installer: stop swallowing subprocess output so long-running `go build`/`git clone` progress is visible to the user - cli_install: extract `_install_platform_or_exit` to dedupe platform handling across `lts`, `tip`, and `release` - cli_scenario: parse manifest YAML once and dispatch by `kind`, instead of peeking + re-parsing in each loader Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…typing - AGENTS.md: add "Before Committing Python Changes" section requiring `uv run ruff check`, `uv run ruff format`, and `uv run pyright` on any touched Python package before commit. Explicitly call out that `uvx` must NOT be used for pyright (isolated env can't see project deps, so every project import becomes a spurious "could not be resolved" error). - cli_scenario: split the single `dict[str, object]` install record into per-section typed containers (`installed_platform`, `installed_kas`, `installed_sdks`) assembled at write time via a `_snapshot()` helper. Fixes pre-existing pyright `__setitem__ ... not defined on object` errors at the nested writes; on-disk JSON shape is unchanged. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Root AGENTS.md: add a Repository Layout table near the top, correct the `platform/` description (it's installed by `otdf-sdk-mgr install`, not committed source), and trim the duplicated "Summary → Preferred Workflow" block that restated the body. - otdf-local/AGENTS.md: lead with the dependency on `otdf-sdk-mgr` (otdf-local launches the binaries the installer produces). Mark the manual-keys YAML block as an emergency fallback that may drift. - otdf-sdk-mgr/AGENTS.md (new): operational guide for the installer — subcommand layout, bare-clone-worktree gotchas (no `origin` remote, namespaced `service/vX.Y.Z` tags, unbuffered subprocess output), pattern for adding a new subcommand. - xtest/AGENTS.md (new): test-suite layout, custom pytest options, audit-log fixture quick reference, authoring guidance. - otdf-sdk-mgr/CLAUDE.md, xtest/CLAUDE.md: symlinks to AGENTS.md to match the repo convention. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds a `--ref` option to `install tip` so platform and SDK source builds can target any git ref (branches, tags, SHAs, raw `refs/...`, or the `pr:N` shorthand that expands to `refs/pull/N/head`). Mutable refs (branches, PR heads) re-fetch the bare repo and rebuild on each invocation; immutable refs (tags, full SHAs) reuse the cached dist. Also fetches `refs/...` refs explicitly into the bare repo before `git worktree add` — the default bare-clone refspec doesn't include `refs/pull/*`, so PR installs were dying with `invalid reference`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
dmihalcik-virtru
force-pushed
the
DSPX-3302-02-platform-installer
branch
from
a3dea66 to
04f9cac
Compare
X-Test Failure Report |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
xtest/AGENTS.md (1)
18-18:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove the unresolved merge conflict marker.
Line 18 contains a Git merge conflict marker (
<<<<<<< HEAD) that breaks the Markdown table structure. This was previously flagged and must be removed before merging.🔧 Proposed fix
-<<<<<<< HEAD | `sdk/{go,java,js}/dist/<version>/` | SDK CLI builds. Installed by `otdf-sdk-mgr install` (see `../otdf-sdk-mgr/AGENTS.md`). |As per coding guidelines: "
xtest/**/AGENTS.md: Review AGENTS.md for coding standards and guidelines".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xtest/AGENTS.md` at line 18, Remove the unresolved Git merge marker "<<<<<<< HEAD" from AGENTS.md (it currently breaks the Markdown table); simply delete the conflict marker and any corresponding conflict markers ("=======", ">>>>>>> ...") if present so the table is valid, then verify the Markdown renders correctly.
🧹 Nitpick comments (2)
otdf-sdk-mgr/src/otdf_sdk_mgr/platform_installer.py (1)
263-263: 💤 Low valueLexicographic sort may yield unexpected ordering for semver versions.
out.sort()sorts strings lexicographically, sov0.10.0will appear beforev0.9.0. If callers expect version-ordered results (e.g., to find the latest), consider sorting by parsed semver.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@otdf-sdk-mgr/src/otdf_sdk_mgr/platform_installer.py` at line 263, The code currently calls out.sort() which performs a lexicographic string sort and will mis-order semantic versions (e.g., v0.10.0 before v0.9.0); change the sort to use a semantic-version-aware key by parsing versions (e.g., use packaging.version.parse or semantic_version.Version) and sort with out.sort(key=parsed_version) or equivalent, adding the necessary import (from packaging.version import parse or from semantic_version import Version) and ensuring any leading "v" is stripped before parsing so version-ordering (latest first/last as desired) is correct.otdf-sdk-mgr/src/otdf_sdk_mgr/checkout.py (1)
79-86: 💤 Low valueMisleading comment: worktrees do have access to the bare repo's remotes.
The comment says "Worktrees from a bare clone have no
originremote", but line 83 then successfully fetches fromorigin. Worktrees created from a bare repo share its remote configuration. The actual reasongit pullfails is that detached worktrees or bare-clone worktrees may not have a tracking branch set, not thatoriginis missing.Suggested comment clarification
if is_mutable_ref(branch): print(f"Worktree exists at {worktree_path}; resetting to '{branch}'.") - # Worktrees from a bare clone have no `origin` remote, so reset - # to the just-fetched ref in the bare repo instead of `git pull`. + # `git pull` may fail in worktrees from a bare clone (no tracking + # branch configured), so explicitly fetch + reset instead. _run(["git", "-C", str(worktree_path), "fetch", "origin", branch]) _run(["git", "-C", str(worktree_path), "reset", "--hard", "FETCH_HEAD"])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@otdf-sdk-mgr/src/otdf_sdk_mgr/checkout.py` around lines 79 - 86, The inline comment wrongly claims "Worktrees from a bare clone have no `origin` remote" while the code calls _run(["git", "-C", str(worktree_path), "fetch", "origin", branch]); update the comment near the is_mutable_ref branch handling to state that worktrees may lack a configured tracking branch or be detached (so git pull can fail), and that we explicitly fetch the ref from origin and hard-reset to FETCH_HEAD to avoid relying on a tracking branch; reference is_mutable_ref, worktree_path and the two _run(["git", ... "fetch", ...]) / _run(["git", ... "reset", ...]) calls to locate the block to edit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@otdf-sdk-mgr/src/otdf_sdk_mgr/platform_installer.py`:
- Around line 189-192: install_platform_source() currently sets dist_name using
normalize_version(ref) for immutable refs which incorrectly normalizes a
namespaced tag like "service/v0.9.0" into "vservice/v0.9.0"; instead, after
resolving full_ref via _resolve_platform_ref(ref) compute the immutable branch
dist_name by normalizing only the tail (the semver part) — e.g. tail =
full_ref.rsplit("/", 1)[-1] — and pass that to normalize_version(tail); keep the
mutable branch using ref_slug(full_ref) when is_mutable_ref(full_ref) is True
and then use that dist_name for dist_dir as before so namespaced immutable tags
reuse the same dist directory as their non-namespaced equivalents.
---
Duplicate comments:
In `@xtest/AGENTS.md`:
- Line 18: Remove the unresolved Git merge marker "<<<<<<< HEAD" from AGENTS.md
(it currently breaks the Markdown table); simply delete the conflict marker and
any corresponding conflict markers ("=======", ">>>>>>> ...") if present so the
table is valid, then verify the Markdown renders correctly.
---
Nitpick comments:
In `@otdf-sdk-mgr/src/otdf_sdk_mgr/checkout.py`:
- Around line 79-86: The inline comment wrongly claims "Worktrees from a bare
clone have no `origin` remote" while the code calls _run(["git", "-C",
str(worktree_path), "fetch", "origin", branch]); update the comment near the
is_mutable_ref branch handling to state that worktrees may lack a configured
tracking branch or be detached (so git pull can fail), and that we explicitly
fetch the ref from origin and hard-reset to FETCH_HEAD to avoid relying on a
tracking branch; reference is_mutable_ref, worktree_path and the two
_run(["git", ... "fetch", ...]) / _run(["git", ... "reset", ...]) calls to
locate the block to edit.
In `@otdf-sdk-mgr/src/otdf_sdk_mgr/platform_installer.py`:
- Line 263: The code currently calls out.sort() which performs a lexicographic
string sort and will mis-order semantic versions (e.g., v0.10.0 before v0.9.0);
change the sort to use a semantic-version-aware key by parsing versions (e.g.,
use packaging.version.parse or semantic_version.Version) and sort with
out.sort(key=parsed_version) or equivalent, adding the necessary import (from
packaging.version import parse or from semantic_version import Version) and
ensuring any leading "v" is stripped before parsing so version-ordering (latest
first/last as desired) is correct.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 204485b8-c732-4efe-8d05-c8d322d9b4ab
📒 Files selected for processing (17)
.gitignoreAGENTS.mdotdf-local/AGENTS.mdotdf-sdk-mgr/AGENTS.mdotdf-sdk-mgr/CLAUDE.mdotdf-sdk-mgr/README.mdotdf-sdk-mgr/src/otdf_sdk_mgr/checkout.pyotdf-sdk-mgr/src/otdf_sdk_mgr/cli_install.pyotdf-sdk-mgr/src/otdf_sdk_mgr/cli_scenario.pyotdf-sdk-mgr/src/otdf_sdk_mgr/installers.pyotdf-sdk-mgr/src/otdf_sdk_mgr/platform_installer.pyotdf-sdk-mgr/src/otdf_sdk_mgr/refs.pyotdf-sdk-mgr/src/otdf_sdk_mgr/schema.pyotdf-sdk-mgr/tests/test_cli_scenario.pyotdf-sdk-mgr/tests/test_platform_installer.pyotdf-sdk-mgr/tests/test_refs.pyxtest/AGENTS.md
✅ Files skipped from review due to trivial changes (6)
- .gitignore
- otdf-local/AGENTS.md
- AGENTS.md
- otdf-sdk-mgr/CLAUDE.md
- otdf-sdk-mgr/AGENTS.md
- otdf-sdk-mgr/tests/test_refs.py
🚧 Files skipped from review as they are similar to previous changes (4)
- otdf-sdk-mgr/tests/test_cli_scenario.py
- otdf-sdk-mgr/src/otdf_sdk_mgr/cli_scenario.py
- otdf-sdk-mgr/src/otdf_sdk_mgr/schema.py
- otdf-sdk-mgr/src/otdf_sdk_mgr/cli_install.py
…refs
For immutable refs (tags, SHAs), derive dist_name by normalizing only the
semver tail after the last `/`. This ensures namespaced tags like
`service/v0.9.0` produce the same dist_name (`v0.9.0`) as plain tags
(`v0.9.0`, `0.9.0`), enabling immutable ref dist-dir reuse.
Before: `normalize_version(ref)` on `service/v0.9.0` → `vservice/v0.9.0`
After: `normalize_version(ref.rsplit("/", 1)[-1])` → `v0.9.0`
Also add `list_platform_versions()` to registry and expose platform versions
via `versions list platform`.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@otdf-sdk-mgr/src/otdf_sdk_mgr/cli_versions.py`:
- Line 45: Validate the incoming sdk value against the allowed set instead of
silently producing an empty list: check the variable sdks computation (the
ternary that sets sdks = ["go", "js", "java", "platform"] if sdk == "all" else
[sdk]) and ensure sdk is one of {"go","js","java","platform","all"}; if not, log
an error and call sys.exit(2). Apply the same explicit validation where similar
logic appears around the other branch (the code at the block referenced by lines
61-63) so unsupported sdk values consistently fail fast with exit code 2.
In `@otdf-sdk-mgr/src/otdf_sdk_mgr/registry.py`:
- Line 230: The install_method string is using the wrong command format; update
the value for the "install_method" key in registry.py (where install_method is
built) to match the platform release flow by using the "install release
platform:{version_normalized}" form (i.e., produce "otdf-sdk-mgr install release
platform:{version_normalized}" instead of the current "otdf-sdk-mgr install
platform {version_normalized}").
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 071f4bd4-1d1c-4393-abe3-991b3d8fab2e
📒 Files selected for processing (4)
otdf-sdk-mgr/src/otdf_sdk_mgr/cli_versions.pyotdf-sdk-mgr/src/otdf_sdk_mgr/platform_installer.pyotdf-sdk-mgr/src/otdf_sdk_mgr/registry.pyotdf-sdk-mgr/tests/test_platform_installer.py
🚧 Files skipped from review as they are similar to previous changes (1)
- otdf-sdk-mgr/src/otdf_sdk_mgr/platform_installer.py
- Remove stray <<<<<<< HEAD merge marker from xtest/AGENTS.md
- Disambiguate 7-39 char hex refs in platform_installer via
`git rev-parse --verify`; ambiguous prefixes raise PlatformInstallError,
unresolvable hex falls through as a branch/tag name
- Make `install_go_release` fail loudly on `go install` pre-warm errors;
no more silent .version writes after a broken install
- Add `RegistryUnreachableError` and raise it from npm/Maven/GitHub
URLError paths so network outages no longer look like "no versions
available"; CLI wrappers translate to clean typer.Exit(1)
- Fix `versions {list,latest}` typo in AGENTS.md (subcommands are
`list` and `resolve`)
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Widen `install scenario` exception net to subprocess.CalledProcessError, ValidationError, and OSError so a failed build, bad YAML, or missing helper script still produces a typer.Exit(1) plus a partial manifest instead of an unhandled traceback - Delete duplicate `list_platform_versions` from platform_installer.py (registry.py has the canonical version returning dict entries) - Preserve KasPin.mode and KasPin.features in the installed.json manifest so downstream tooling can read them back without re-parsing YAML - Add `.complete` marker to platform builds; reuse requires both the binary and the marker, surviving Ctrl-C mid-build - checkout._run now captures stderr and includes cwd in the raised CalledProcessError; platform_installer._run wraps FileNotFoundError with the executable name - Move scenario subcommand registration out of `_register_scenario_cmd` side-effect wrapper Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Tests: - Rewrite `test_dist_name_derivation` to call `install_platform_source` instead of re-implementing its dist-name logic in the test body - Regression tests for mutable-vs-immutable rebuild, .complete marker semantics, PR ref fetch via explicit refspec, and short-SHA expansion - New `test_registry.py` covering RegistryUnreachableError propagation, _github_headers with/without GITHUB_TOKEN, ls_remote tag parsing, and GitHub rate-limit warning - Assert KasPin.mode and KasPin.features round-trip into installed.json Polish: - `install_java_release` switches the BaseException catch to try/finally so KeyboardInterrupt/SystemExit retain their normal semantics - README documents the dist-naming convention as a table Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
X-Test Failure Report |
- Reject container-image refs in _resolve_platform_ref with a clear PlatformInstallError, instead of letting strings like ghcr.io/opentdf/platform:v0.9.0 fall through to git and fail with a generic "invalid reference" message. - Use "install release platform:<version>" in registry.install_method so copy-paste from `versions list` lands on the actual subcommand signature. - Drop unused boom() helper flagged by Sonar in test_registry.py. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Member
Author
|
Latest push (8b5de7d) addresses the two remaining bot suggestions:
Also dropped the unused Earlier rounds of Gemini / CodeRabbit suggestions ( |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@otdf-sdk-mgr/src/otdf_sdk_mgr/registry.py`:
- Around line 181-182: The code currently treats HTTPError (403/429 rate-limit)
as a URLError and raises RegistryUnreachableError; update the exception handling
so that fetch_json() (or list_java_github_releases()) catches
urllib.error.HTTPError separately and maps 403 and 429 to a distinct rate-limit
error (e.g., raise a new RateLimitError with the response/code and message or
re-raise the original HTTPError) while only converting genuine connection
failures (urllib.error.URLError excluding HTTPError) into
RegistryUnreachableError; adjust references in list_java_github_releases(),
fetch_json(), and the RegistryUnreachableError usage accordingly so callers can
distinguish rate-limited responses from unreachable network errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 21390981-6807-4a77-a5e8-eda2313ee964
📒 Files selected for processing (13)
otdf-sdk-mgr/AGENTS.mdotdf-sdk-mgr/README.mdotdf-sdk-mgr/src/otdf_sdk_mgr/checkout.pyotdf-sdk-mgr/src/otdf_sdk_mgr/cli_install.pyotdf-sdk-mgr/src/otdf_sdk_mgr/cli_scenario.pyotdf-sdk-mgr/src/otdf_sdk_mgr/cli_versions.pyotdf-sdk-mgr/src/otdf_sdk_mgr/installers.pyotdf-sdk-mgr/src/otdf_sdk_mgr/platform_installer.pyotdf-sdk-mgr/src/otdf_sdk_mgr/registry.pyotdf-sdk-mgr/tests/test_cli_scenario.pyotdf-sdk-mgr/tests/test_platform_installer.pyotdf-sdk-mgr/tests/test_registry.pyxtest/AGENTS.md
💤 Files with no reviewable changes (1)
- xtest/AGENTS.md
✅ Files skipped from review due to trivial changes (2)
- otdf-sdk-mgr/README.md
- otdf-sdk-mgr/AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (4)
- otdf-sdk-mgr/src/otdf_sdk_mgr/cli_scenario.py
- otdf-sdk-mgr/src/otdf_sdk_mgr/checkout.py
- otdf-sdk-mgr/src/otdf_sdk_mgr/cli_install.py
- otdf-sdk-mgr/src/otdf_sdk_mgr/cli_versions.py
Comment on lines
181
to
+182
| except urllib.error.URLError as e: | ||
| print(f"Warning: failed to fetch GitHub releases: {e}", file=sys.stderr) | ||
| break | ||
| raise RegistryUnreachableError(f"failed to fetch GitHub releases ({url}): {e}") from e |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python3 -c "import urllib.error; print('HTTPError subclasses URLError:', issubclass(urllib.error.HTTPError, urllib.error.URLError))"Repository: opentdf/tests
Length of output: 93
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "def fetch_json|fetch_json\(|list_java_github_releases|RegistryUnreachableError|URLError|HTTPError" otdf-sdk-mgr/src/otdf_sdk_mgr/registry.py
echo "---- registry.py excerpt around line 150-220 ----"
sed -n '150,220p' otdf-sdk-mgr/src/otdf_sdk_mgr/registry.pyRepository: opentdf/tests
Length of output: 3423
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- registry.py excerpt around fetch_json (lines 35-90) ----"
sed -n '35,95p' otdf-sdk-mgr/src/otdf_sdk_mgr/registry.pyRepository: opentdf/tests
Length of output: 2386
Rate-limit (403/429) should not be wrapped as “unreachable”.
otdf-sdk-mgr/src/otdf_sdk_mgr/registry.py lines 181-182: fetch_json() catches GitHub urllib.error.HTTPError with codes 403/429, prints a rate-limit warning, and then re-raises. Since HTTPError is a subclass of URLError, list_java_github_releases() catches it and converts it into RegistryUnreachableError, misleading users into treating a reachable rate-limited endpoint as a network outage. Catch HTTPError separately (or map 403/429 to a distinct rate-limit error) and only convert genuine URLError connection failures.
🛠️ Possible approach
try:
releases = fetch_json(url)
+ except urllib.error.HTTPError:
+ # 403/429 (rate limit) already surfaced a warning in fetch_json;
+ # the endpoint is reachable, so don't mask it as "unreachable".
+ raise
except urllib.error.URLError as e:
raise RegistryUnreachableError(f"failed to fetch GitHub releases ({url}): {e}") from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| except urllib.error.URLError as e: | |
| print(f"Warning: failed to fetch GitHub releases: {e}", file=sys.stderr) | |
| break | |
| raise RegistryUnreachableError(f"failed to fetch GitHub releases ({url}): {e}") from e | |
| try: | |
| releases = fetch_json(url) | |
| except urllib.error.HTTPError: | |
| # 403/429 (rate limit) already surfaced a warning in fetch_json; | |
| # the endpoint is reachable, so don't mask it as "unreachable". | |
| raise | |
| except urllib.error.URLError as e: | |
| raise RegistryUnreachableError(f"failed to fetch GitHub releases ({url}): {e}") from e |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@otdf-sdk-mgr/src/otdf_sdk_mgr/registry.py` around lines 181 - 182, The code
currently treats HTTPError (403/429 rate-limit) as a URLError and raises
RegistryUnreachableError; update the exception handling so that fetch_json() (or
list_java_github_releases()) catches urllib.error.HTTPError separately and maps
403 and 429 to a distinct rate-limit error (e.g., raise a new RateLimitError
with the response/code and message or re-raise the original HTTPError) while
only converting genuine connection failures (urllib.error.URLError excluding
HTTPError) into RegistryUnreachableError; adjust references in
list_java_github_releases(), fetch_json(), and the RegistryUnreachableError
usage accordingly so callers can distinguish rate-limited responses from
unreachable network errors.
This was referenced Jun 1, 2026
|
](https://app.coderabbit.ai/change-stack/opentdf/tests/pull/450?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
](https://app.coderabbit.ai/change-stack/opentdf/tests/pull/459?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Promotes the OpenTDF platform service to a first-class managed package in
otdf-sdk-mgr, mirroring the existing Go/Java/JS SDK CLI flow, and adds a one-shotinstall scenarioentry point.Summary
Second PR in the five-part stack.
platform_installer.py: resolvesv0.9.0to theservice/v0.9.0tag in theopentdf/platformmonorepo, creates a git worktree, and runsgo build -o xtest/platform/dist/<version>/service ./service.install_helper_scripts(main): mirrorsplatform/scripts/intoxtest/platform/scripts/. Helper scripts are shared across instances and refreshed on demand.otdf-sdk-mgr install release platform:<version>(alongsidego:,js:,java:)otdf-sdk-mgr install lts platform/install tip platformotdf-sdk-mgr install scriptsotdf-sdk-mgr install scenario <path>— installs platform pin + per-KAS pins + encrypt/decrypt SDK union from a scenarios.yaml or instance.yaml, writes<path>.installed.jsonotdf-sdk-mgr versionsStack
Test plan
cd otdf-sdk-mgr && uv run pytest tests/→ 57 passing (existing 51 + new 6 from PR 1)uv run otdf-sdk-mgr install --helpshows the newscenarioandscriptscommandsJira: https://virtru.atlassian.net/browse/DSPX-3302
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements / Bug Fixes
Documentation
Tests
Stack (
a60d3302):Generated by
wgo stack. Edit text above or below this block, not inside it.