feat(xtest): support platform-embedded otdfctl for migration to monorepo#434
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 selectable otdfctl source (auto/standalone/platform), detects platform-embedded otdfctl in CI, and threads the source through resolver, registry, installers, setup action, and CLI scripts so Go CLI resolution, installation, and generated links use the correct repository/module and version format. Changes
Sequence DiagramsequenceDiagram
participant WF as GitHub Workflow
participant Detect as Detect Step
participant Setup as Setup Action
participant Resolve as Resolver (otdf-sdk-mgr)
participant Install as Installer (otdf-sdk-mgr)
participant Script as CLI Script
WF->>Detect: probe checkout for embedded otdfctl (auto or explicit)
Detect-->>WF: outputs {otdfctl-source, otdfctl-dir, otdfctl-sha}
WF->>Resolve: resolve(sdk="go", go_source=otdfctl-source)
Resolve-->>WF: ResolveSuccess {version, sha, source?}
WF->>Install: install_go_release(version, source)
Install-->>WF: writes .version (module@tag) / installs binary
WF->>Setup: provide platform-otdfctl-dir/sha to action
Setup-->>WF: checkout or symlink platform sources per outputs
WF->>Script: run cli.sh / otdfctl.sh
Script->>Script: read .version -> choose module@version vs legacy repo@tag
Script-->>WF: CLI output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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}
There was a problem hiding this comment.
Code Review
This pull request introduces support for resolving and installing the Go otdfctl CLI from the platform monorepo in addition to the standalone repository. Key changes include adding a source parameter to resolution and installation logic, updating shell wrappers to support dynamic module paths, and modifying the GitHub Action to allow symlinking local platform-embedded source code. Feedback highlights a missing propagation of the source parameter during installation, a potential IndexError when resolving the main branch, and security risks associated with direct shell interpolation of JSON outputs in the GitHub Action.
X-Test Results✅ java-main |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
xtest/sdk/go/otdfctl.sh (1)
20-28: Use[[instead of[for conditional tests.The SonarCloud analysis correctly flags this. Since the script uses
#!/usr/bin/env bash, using[[provides safer string comparisons (no word splitting or glob expansion issues).♻️ Proposed fix
if [ ! -f "$SCRIPT_DIR"/otdfctl ]; then MODULE_PATH="github.com/opentdf/otdfctl" - if [ -f "$SCRIPT_DIR/.module-path" ]; then + if [[ -f "$SCRIPT_DIR/.module-path" ]]; then MODULE_PATH=$(tr -d '[:space:]' <"$SCRIPT_DIR/.module-path") fi - if [ -f "$SCRIPT_DIR/.version" ]; then + if [[ -f "$SCRIPT_DIR/.version" ]]; then OTDFCTL_VERSION=$(tr -d '[:space:]' <"$SCRIPT_DIR/.version") cmd=(go run "${MODULE_PATH}@${OTDFCTL_VERSION}") else🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xtest/sdk/go/otdfctl.sh` around lines 20 - 28, Replace the POSIX [ tests with Bash [[ tests in the otfdctl script to avoid word-splitting and globbing issues: change each if [ -f "$SCRIPT_DIR/.module-path" ] and if [ -f "$SCRIPT_DIR/.version" ] to if [[ -f "$SCRIPT_DIR/.module-path" ]] and if [[ -f "$SCRIPT_DIR/.version" ]], keeping the same bodies that read into MODULE_PATH and OTDFCTL_VERSION and assign cmd (MODULE_PATH, SCRIPT_DIR, OTDFCTL_VERSION, cmd are the identifiers to locate).otdf-sdk-mgr/src/otdf_sdk_mgr/installers.py (1)
44-45: Consider usinggo_module_path(source)for consistency.Line 49 uses
go_install_prefix(source)to dynamically select the module prefix, but line 45 directly references theGO_MODULE_PATH_PLATFORMconstant. Usinggo_module_path(source)here would be more consistent and encapsulate the platform/standalone selection logic in one place.♻️ Suggested consistency improvement
-from otdf_sdk_mgr.config import ( - GO_MODULE_PATH_PLATFORM, - LTS_VERSIONS, - get_sdk_dir, - get_sdk_dirs, - go_install_prefix, -) +from otdf_sdk_mgr.config import ( + LTS_VERSIONS, + get_sdk_dir, + get_sdk_dirs, + go_install_prefix, + go_module_path, +)if source == "platform": - (dist_dir / ".module-path").write_text(f"{GO_MODULE_PATH_PLATFORM}\n") + (dist_dir / ".module-path").write_text(f"{go_module_path(source)}\n")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@otdf-sdk-mgr/src/otdf_sdk_mgr/installers.py` around lines 44 - 45, The code writes GO_MODULE_PATH_PLATFORM directly when source == "platform", which duplicates selection logic; replace the direct constant with a call to go_module_path(source) so the module path selection is centralized (match the existing pattern used by go_install_prefix(source)), i.e., use go_module_path(source) when writing to the ".module-path" file and preserve the trailing newline.xtest/sdk/go/cli.sh (1)
25-33: Use[[instead of[for conditional tests.Same as
otdfctl.sh- the SonarCloud analysis flags this. Since the script uses Bash,[[is the safer choice.♻️ Proposed fix
if [ ! -f "$SCRIPT_DIR"/otdfctl ]; then MODULE_PATH="github.com/opentdf/otdfctl" - if [ -f "$SCRIPT_DIR/.module-path" ]; then + if [[ -f "$SCRIPT_DIR/.module-path" ]]; then MODULE_PATH=$(tr -d '[:space:]' <"$SCRIPT_DIR/.module-path") fi - if [ -f "$SCRIPT_DIR/.version" ]; then + if [[ -f "$SCRIPT_DIR/.version" ]]; then OTDFCTL_VERSION=$(tr -d '[:space:]' <"$SCRIPT_DIR/.version") cmd=(go run "${MODULE_PATH}@${OTDFCTL_VERSION}") else🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xtest/sdk/go/cli.sh` around lines 25 - 33, The conditional file-existence tests use the POSIX single-bracket form; change them to Bash's safer double-bracket tests: replace instances of [ -f "$SCRIPT_DIR/.module-path" ] and [ -f "$SCRIPT_DIR/.version" ] with [[ -f "$SCRIPT_DIR/.module-path" ]] and [[ -f "$SCRIPT_DIR/.version" ]] respectively in the code that assigns MODULE_PATH, OTDFCTL_VERSION and builds the cmd array so the checks are executed with Bash's [[ ... ]] semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@xtest/setup-cli-tool/action.yaml`:
- Around line 150-173: The run block uses interpolated inputs (${ { inputs.path
}} and ${ { inputs.sdk }}) directly causing script-injection risk; move those
into environment variables (e.g., INPUT_PATH and INPUT_SDK) in the step's env
and reference them inside the bash script (use "$INPUT_PATH" and "$INPUT_SDK")
when constructing src_dir and mkdir/ln operations (symbols to update: the step
named "symlink platform-embedded otdfctl for head versions", variables
version_json/needs_source/tag/src_dir, and the ln -sfn invocation), and ensure
all expansions are quoted to prevent word-splitting.
---
Nitpick comments:
In `@otdf-sdk-mgr/src/otdf_sdk_mgr/installers.py`:
- Around line 44-45: The code writes GO_MODULE_PATH_PLATFORM directly when
source == "platform", which duplicates selection logic; replace the direct
constant with a call to go_module_path(source) so the module path selection is
centralized (match the existing pattern used by go_install_prefix(source)),
i.e., use go_module_path(source) when writing to the ".module-path" file and
preserve the trailing newline.
In `@xtest/sdk/go/cli.sh`:
- Around line 25-33: The conditional file-existence tests use the POSIX
single-bracket form; change them to Bash's safer double-bracket tests: replace
instances of [ -f "$SCRIPT_DIR/.module-path" ] and [ -f "$SCRIPT_DIR/.version" ]
with [[ -f "$SCRIPT_DIR/.module-path" ]] and [[ -f "$SCRIPT_DIR/.version" ]]
respectively in the code that assigns MODULE_PATH, OTDFCTL_VERSION and builds
the cmd array so the checks are executed with Bash's [[ ... ]] semantics.
In `@xtest/sdk/go/otdfctl.sh`:
- Around line 20-28: Replace the POSIX [ tests with Bash [[ tests in the otfdctl
script to avoid word-splitting and globbing issues: change each if [ -f
"$SCRIPT_DIR/.module-path" ] and if [ -f "$SCRIPT_DIR/.version" ] to if [[ -f
"$SCRIPT_DIR/.module-path" ]] and if [[ -f "$SCRIPT_DIR/.version" ]], keeping
the same bodies that read into MODULE_PATH and OTDFCTL_VERSION and assign cmd
(MODULE_PATH, SCRIPT_DIR, OTDFCTL_VERSION, cmd are the identifiers to locate).
🪄 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: 20c9d9d4-32c8-4359-a320-f53c627cf8a4
📒 Files selected for processing (8)
.github/workflows/xtest.ymlotdf-sdk-mgr/src/otdf_sdk_mgr/cli_versions.pyotdf-sdk-mgr/src/otdf_sdk_mgr/config.pyotdf-sdk-mgr/src/otdf_sdk_mgr/installers.pyotdf-sdk-mgr/src/otdf_sdk_mgr/resolve.pyxtest/sdk/go/cli.shxtest/sdk/go/otdfctl.shxtest/setup-cli-tool/action.yaml
X-Test Failure Report |
3 similar comments
X-Test Failure Report |
X-Test Failure Report |
X-Test Failure Report |
60373eb to
bf739dc
Compare
X-Test Failure Report |
X-Test Results✅ java-v0.13.0 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
otdf-sdk-mgr/src/otdf_sdk_mgr/resolve.py (1)
148-159: Consider using exact match for main branch detection.The condition
"refs/heads/main" in tagis a substring match that could unintentionally match branches likerefs/heads/main-featureorrefs/heads/main2. An exact match would be safer:sha, _ = next(tag for tag in all_heads if tag[1] == "refs/heads/main")However, since the tuple is
(sha, ref), the current logic checks if the ref string contains the substring which should work for the exact refrefs/heads/main. Ifall_headscontains["sha", "refs/heads/main"]tuples, thentag[1]would be the ref. The current"refs/heads/main" in tagchecks if the substring exists anywhere in the tuple (including the sha), which is less precise.♻️ Proposed fix for exact match
all_heads = [r.split("\t") for r in repo.ls_remote(sdk_url, heads=True).split("\n")] - sha, _ = next(tag for tag in all_heads if "refs/heads/main" in tag) + sha, _ = next((s, r) for s, r in all_heads if r == "refs/heads/main")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@otdf-sdk-mgr/src/otdf_sdk_mgr/resolve.py` around lines 148 - 159, The branch detection uses a substring search against the tuple (all_heads) which can mis-match names like refs/heads/main-feature; update the generator in the resolve.py block (where version == "main" or "refs/heads/main", all_heads, repo.ls_remote, and the next(...) call) to unpack each tuple into (sha, ref) and match ref exactly (e.g., next((sha for sha, ref in all_heads if ref == "refs/heads/main"))), then use that sha and ref to construct the returned dict with "tag": "main" as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@otdf-sdk-mgr/src/otdf_sdk_mgr/resolve.py`:
- Around line 148-159: The branch detection uses a substring search against the
tuple (all_heads) which can mis-match names like refs/heads/main-feature; update
the generator in the resolve.py block (where version == "main" or
"refs/heads/main", all_heads, repo.ls_remote, and the next(...) call) to unpack
each tuple into (sha, ref) and match ref exactly (e.g., next((sha for sha, ref
in all_heads if ref == "refs/heads/main"))), then use that sha and ref to
construct the returned dict with "tag": "main" as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 54ee9642-05f8-4986-a32c-5e0d698b8236
📒 Files selected for processing (10)
.github/workflows/xtest.ymlotdf-sdk-mgr/README.mdotdf-sdk-mgr/src/otdf_sdk_mgr/cli_install.pyotdf-sdk-mgr/src/otdf_sdk_mgr/config.pyotdf-sdk-mgr/src/otdf_sdk_mgr/installers.pyotdf-sdk-mgr/src/otdf_sdk_mgr/registry.pyotdf-sdk-mgr/src/otdf_sdk_mgr/resolve.pyxtest/sdk/go/cli.shxtest/sdk/go/otdfctl.shxtest/setup-cli-tool/action.yaml
✅ Files skipped from review due to trivial changes (1)
- otdf-sdk-mgr/README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- xtest/sdk/go/cli.sh
- .github/workflows/xtest.yml
- otdf-sdk-mgr/src/otdf_sdk_mgr/config.py
- xtest/sdk/go/otdfctl.sh
- otdf-sdk-mgr/src/otdf_sdk_mgr/installers.py
X-Test Failure Report |
3 similar comments
X-Test Failure Report |
X-Test Failure Report |
X-Test Failure Report |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
xtest/setup-cli-tool/action.yaml (1)
338-348:⚠️ Potential issue | 🔴 CriticalFix
.envfile path: write to SDK root, not tosrc/subdirectory.The
save env from version-infostep writes.envfiles tosrc/${tag}.env, but the Java Makefile expects them at the SDK root directory (MAKEFILE_DIR). This causes version-specific environment variables likePLATFORM_BRANCHto be ignored during builds. Go and JS Makefiles do not consume version-specific.envfiles.Change the path from
${{ inputs.path }}/${{ inputs.sdk }}/src/${tag}.envto${{ inputs.path }}/${{ inputs.sdk }}/${tag}.env.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xtest/setup-cli-tool/action.yaml` around lines 338 - 348, The "save env from version-info" step currently writes per-version .env files to "${{ inputs.path }}/${{ inputs.sdk }}/src/${tag}.env" which is wrong for the Java Makefile; update the run loop so it writes to the SDK root instead by changing the target path to "${{ inputs.path }}/${{ inputs.sdk }}/${tag}.env" (keep the loop, tag/env extraction and the conditional intact, only replace the "/src/" segment in the echo output path).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@xtest/setup-cli-tool/action.yaml`:
- Around line 338-348: The "save env from version-info" step currently writes
per-version .env files to "${{ inputs.path }}/${{ inputs.sdk }}/src/${tag}.env"
which is wrong for the Java Makefile; update the run loop so it writes to the
SDK root instead by changing the target path to "${{ inputs.path }}/${{
inputs.sdk }}/${tag}.env" (keep the loop, tag/env extraction and the conditional
intact, only replace the "/src/" segment in the echo output path).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fb9ecb89-5294-4f93-960b-ad80764c535e
📒 Files selected for processing (8)
.github/workflows/xtest.ymlotdf-sdk-mgr/README.mdotdf-sdk-mgr/src/otdf_sdk_mgr/cli_versions.pyotdf-sdk-mgr/src/otdf_sdk_mgr/config.pyotdf-sdk-mgr/src/otdf_sdk_mgr/installers.pyotdf-sdk-mgr/src/otdf_sdk_mgr/registry.pyotdf-sdk-mgr/src/otdf_sdk_mgr/resolve.pyxtest/setup-cli-tool/action.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- otdf-sdk-mgr/src/otdf_sdk_mgr/cli_versions.py
- otdf-sdk-mgr/README.md
- otdf-sdk-mgr/src/otdf_sdk_mgr/resolve.py
X-Test Results✅ java-v0.13.0 |
X-Test Results✅ java-main |
|
|
||
| - name: Replace otdfctl go.mod packages, but only at head version of platform | ||
| if: fromJson(steps.configure-go.outputs.heads)[0] != null && env.FOCUS_SDK == 'go' && contains(fromJSON(needs.resolve-versions.outputs.heads), matrix.platform-tag) | ||
| if: >- |
There was a problem hiding this comment.
So in default auto mode, detect-otdfctl reports platform if the otdfctl dir exists. However, resolve-versions still pulls the Go head from the standalone repo (which skips the replace step and the build ends with the published platform modules). Should this condition be on the per-version source from resolve-versions instead?
There was a problem hiding this comment.
I don't think so, it will check out a new copy of the platform repo at that tag so the go.work should be correct. I could be wrong; there are a lot of moving parts
There was a problem hiding this comment.
latest_stable_version returns just the version string, so cmd_stable calls install_release with no source. I believe this will fail once the latest stable only exists in opentdf/platform/otdfctl bc go install github.com/opentdf/otdfctl@... will not point to anything. Should we use source here?
otdfctl is moving from opentdf/otdfctl into opentdf/platform. This updates the test infrastructure to auto-detect when the platform checkout contains otdfctl/ and build from there instead of the standalone repo. Key changes: - xtest.yml: new otdfctl-source input (auto/standalone/platform) and detection step that checks for otdfctl/go.mod in the platform dir - setup-cli-tool: new platform-otdfctl-dir input; symlinks platform source into sdk/go/src/ for head builds instead of separate checkout - otdf-sdk-mgr: resolve.py supports go_source="platform" to resolve against the platform repo with otdfctl/ tag infix; installers write .module-path file for the new Go module path - cli.sh/otdfctl.sh: read .module-path to use the correct module path (github.com/opentdf/platform/otdfctl) for go run fallback Backward compatible: old releases still resolve from the standalone repo; .module-path absence defaults to the original module path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… migration - artifactLink in xtest.yml now uses the correct pkg.go.dev module path based on the source field (platform vs standalone) - registry.py list_go_versions() queries both the standalone otdfctl repo and the platform repo (otdfctl/ prefixed tags), deduplicating with platform entries taking precedence - README.md documents both module paths for Go release installs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ersion file The Go SDK version config now uses a single .version file with format `module-path@version` (e.g., `github.com/opentdf/otdfctl@v0.24.0`) instead of separate .version and .module-path files. Shell wrappers fall back to the standalone module path for legacy bare-version files. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Allows specifying the source repo for Go CLI installs (e.g., --source platform) to support the otdfctl migration from standalone repo to the platform monorepo. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…uts and step outputs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Replace global sdk_source/sdk_repo with per-version checkout decisions
so that multiple Go head versions from opentdf/platform at different
SHAs can be tested against each other (e.g. a platform PR vs main).
- Add platform-otdfctl-sha input for SHA-based matching of the existing
platform checkout; versions with a different SHA get a fresh checkout
of opentdf/platform into platform-src/{tag}/ with a symlink from
otdfctl/ into src/{tag}/
- Wire OTDFCTL_SOURCE to the resolve-versions job when otdfctl-source
is explicitly 'platform', so the resolver returns platform SHAs
- Preserve backward-compatible auto-detect behavior: when platform-
otdfctl-dir is set but version-info lacks source:"platform", all
head Go versions still symlink to the existing dir
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Pass --source flag through artifact install step so platform-sourced Go versions get the correct module path in .version files - Strip tag infix (e.g., otdfctl/v0.24.0 → v0.24.0) before normalizing in install_go_release to avoid producing invalid version strings - Move step output expansion to env: block in detect-otdfctl to prevent script injection (consistent with 0c1b428) - Narrow except Exception to GitCommandError in registry.py platform tag query so programming bugs propagate instead of being swallowed - Handle StopIteration explicitly in resolve.py with a clear "main branch not found" error message - Validate source parameter in go_module_path() to reject typos - Add symlink target existence checks after ln -sfn in action.yaml - Add ::warning:: annotation when auto-detect fallback skips SHA verification - Fix typo and minor comment/doc improvements Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The workflow input and CLI advertise "standalone" as a valid source, but go_module_path rejected it with a ValueError. Add "standalone" to the valid set so it maps to the default standalone module path, consistent with go_git_url, go_tag_infix, and go_install_prefix. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The otdfctl-source description implied auto detects from the platform checkout, but release resolution runs before that checkout exists. Update the input description and inline comment to accurately reflect the two-phase behavior: releases resolve from standalone, platform detection applies only to head builds. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
In auto mode, setup-cli-tool previously symlinked all head Go versions to the platform checkout without verifying the commit SHA. This meant otdfctl-ref=my-feature could silently test the platform-embedded otdfctl instead of opentdf/otdfctl@my-feature. Now the fallback only reuses the platform checkout when the resolved SHA matches; otherwise it falls through to a standalone checkout. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Validate PLATFORM_DIR before using it in detect-otdfctl to prevent wrong-repo SHA when the directory is missing. Remove dead cross-repo SHA comparison in auto-detect fallback (standalone and platform repos have different commit histories). Add input validation to all go_* config helpers and the OTDFCTL_SOURCE env var. Treat pre-warm failure as a hard error for the platform module path. Use array pattern for source_args to prevent shell word-splitting. Improve observability with ::warning:: annotations and version-override logging. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Narrow except Exception in resolve() to (GitCommandError, ValueError); move config validation before try block so programming errors propagate - Make unrecognized OTDFCTL_SOURCE a hard error (exit 2) instead of silent fallback to standalone - Fix install_method display for platform versions (use stripped version, not prefixed tag) - Propagate source through latest_stable_version/cmd_stable so platform Go releases use the correct module path - Fix script injection: move inputs.path/sdk from run blocks to env vars in all action.yaml steps (SonarCloud/CodeRabbit finding) - Make freshly checked-out symlink target absolute (alkalescent review) - Check git rev-parse exit code in detect-otdfctl step - Use [[ instead of [ for bash file tests in cli.sh/otdfctl.sh Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2c9bfeb to
7c4db05
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
otdf-sdk-mgr/src/otdf_sdk_mgr/installers.py (1)
245-252:⚠️ Potential issue | 🟡 Minor
cmd_releasecannot target platform source.
cmd_releaseparsessdk:versionspecs and callsinstall_releasewithoutsource, so there's no path for a user invokinginstall release go:v0.24.0to select the platform module. With the migration, once a version only exists on platform, this command will silently resolve to the standalone module path and (perinstall_go_release) warn-and-continue rather than fail, leaving a.versionpointing at a nonexistent module.Consider extending the spec syntax (e.g.,
go@platform:v0.24.0) or auto-detecting fromlist_go_versions()which source owns the requested version. This is the same class of concern raised onlatest_stable_versionin a prior review and is still reachable viacmd_release.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@otdf-sdk-mgr/src/otdf_sdk_mgr/installers.py` around lines 245 - 252, cmd_release currently splits specs as sdk:version and calls install_release(sdk, version) without specifying source, so platform-only versions can be misresolved; update cmd_release to accept an optional source token (syntax like sdk@source:version) or auto-detect the correct source before calling install_release: parse specs for an '@' (e.g., split into sdk, source, version) and if no source is provided, query the per-SDK version list function (e.g., list_go_versions or similar list_*_versions) to determine whether the requested version lives on 'platform' vs 'standalone', then call install_release(sdk, version, source=...) (or pass the resolved source through to install_go_release where applicable) so the installer targets the proper module and avoids creating .version entries pointing at nonexistent modules.
🧹 Nitpick comments (4)
otdf-sdk-mgr/src/otdf_sdk_mgr/cli_install.py (1)
77-86: Consider validatingsourceat the CLI boundary.Unlike
resolve_versionsincli_versions.py(which validatesOTDFCTL_SOURCEagainst{"standalone", "platform"}and exits 2 on unknown), this command forwardssourceverbatim intocmd_install. A typo (e.g.,--source platfrm) will silently fall through togo_module_path(...)'s default behavior. Consider validating here for a consistent UX, or relying ongo_module_pathto raise on unknown values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@otdf-sdk-mgr/src/otdf_sdk_mgr/cli_install.py` around lines 77 - 86, The CLI handler for the install command accepts the source parameter but doesn't validate it before calling cmd_install, so typos (e.g., "--source platfrm") silently propagate to go_module_path; add explicit validation in the CLI boundary (mirroring resolve_versions) to check that source is either "standalone" or "platform" and exit with status 2 on unknown values, and only call cmd_install(sdk, version, dist_name=dist_name, source=source) if validation passes; reference the cli_install.py handler's source parameter, the resolve_versions validation logic, and go_module_path/cmd_install as the places to align behavior.otdf-sdk-mgr/src/otdf_sdk_mgr/installers.py (2)
45-50: Infix stripping is unconditional and source-agnostic.
if "/" in version: version = version.rsplit("/", 1)[-1]strips any prefix before/, regardless of whether it matches the expected infix for the chosensource. In practice this is fine today since callers pass already-bare semvers for platform (seeregistry.pyline 112), but if a caller ever forwardsotdfctl/v0.24.0withsource=None, the.versionfile will recordgithub.com/opentdf/otdfctl@v0.24.0— which happens to work but silently hides a source/tag mismatch. Consider validating that the stripped prefix matches the expected infix for the selectedsource, or logging when stripping occurs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@otdf-sdk-mgr/src/otdf_sdk_mgr/installers.py` around lines 45 - 50, The current unconditional infix strip in the installer (the block that checks if "/" in version and does version.rsplit("/",1)[-1] before calling normalize_version and go_module_path and writing dist_dir / ".version") can hide mismatches between the provided source and the version prefix; update this logic to detect and validate the prefix when a slash is present: extract the prefix and compare it against the expected infix for the given source (or the canonical module path produced by go_module_path) and only strip when they match, otherwise either leave the version as-is and/or emit a warning log indicating a source/tag mismatch; ensure normalize_version is still called on the final version and that the .version file continues to be written using module and tag only after validation.
60-66: Asymmetric pre-warm failure handling — confirm intent.Platform pre-warm failures raise
InstallError, while standalone failures only warn and defer to runtime retry. This is defensible (platform module path is new and a miss likely means "not published yet"), but it means a transient network/proxy blip during a platform install aborts the whole install, whereas the same blip on standalone is tolerated. If that asymmetry is intentional for the migration window, consider a code comment noting it; otherwise consider distinguishing "module not found" (fail fast) from transient errors (warn + retry) by inspectingresult.stderr.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@otdf-sdk-mgr/src/otdf_sdk_mgr/installers.py` around lines 60 - 66, Current code treats go install failures for GO_MODULE_PATH_PLATFORM by raising InstallError while other modules only warn and defer, which is asymmetric; either document the intentional behavior or change logic to distinguish "module not found" vs transient errors by inspecting result.stderr. Update the block around the go install check (use variables result, msg, GO_MODULE_PATH_PLATFORM, and InstallError) to: if the platform-special-case behavior is intentional, add a brief inline comment explaining the migration/publishing rationale; otherwise parse result.stderr (or result.returncode) to detect a not-found/publish-related message and raise InstallError only for that case while treating other transient errors like the non-platform path (print warning and retry at runtime). Ensure the message includes the module and tag context as currently constructed.otdf-sdk-mgr/src/otdf_sdk_mgr/registry.py (1)
115-122: Overlap notice may be noisy during migration window.Once platform starts publishing the same versions as standalone, this note will print for every shared tag on every
list/resolvecall. Consider gating it behind a verbose/debug flag, or emitting a single summary line at the end.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@otdf-sdk-mgr/src/otdf_sdk_mgr/registry.py` around lines 115 - 122, The overlap notice printing for every shared version is noisy; modify the logic around the "if version in seen" branch (the print to sys.stderr) to either: a) respect a verbosity/debug flag (e.g., check an existing verbose/debug parameter or add one to the surrounding function and only emit the message when verbose is true, or switch to using a logger and call logger.debug instead of print), or b) collect overlapping versions into a list (e.g., overlaps or duplicated_versions) during the loop and, after the loop completes, emit a single summary line to stderr/log (e.g., "Note: versions X found in both...") if that list is non-empty. Update the surrounding function signature and callers if you add a verbose flag, or switch to the module logger for debug-level output so callers can control noise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@otdf-sdk-mgr/src/otdf_sdk_mgr/installers.py`:
- Around line 245-252: cmd_release currently splits specs as sdk:version and
calls install_release(sdk, version) without specifying source, so platform-only
versions can be misresolved; update cmd_release to accept an optional source
token (syntax like sdk@source:version) or auto-detect the correct source before
calling install_release: parse specs for an '@' (e.g., split into sdk, source,
version) and if no source is provided, query the per-SDK version list function
(e.g., list_go_versions or similar list_*_versions) to determine whether the
requested version lives on 'platform' vs 'standalone', then call
install_release(sdk, version, source=...) (or pass the resolved source through
to install_go_release where applicable) so the installer targets the proper
module and avoids creating .version entries pointing at nonexistent modules.
---
Nitpick comments:
In `@otdf-sdk-mgr/src/otdf_sdk_mgr/cli_install.py`:
- Around line 77-86: The CLI handler for the install command accepts the source
parameter but doesn't validate it before calling cmd_install, so typos (e.g.,
"--source platfrm") silently propagate to go_module_path; add explicit
validation in the CLI boundary (mirroring resolve_versions) to check that source
is either "standalone" or "platform" and exit with status 2 on unknown values,
and only call cmd_install(sdk, version, dist_name=dist_name, source=source) if
validation passes; reference the cli_install.py handler's source parameter, the
resolve_versions validation logic, and go_module_path/cmd_install as the places
to align behavior.
In `@otdf-sdk-mgr/src/otdf_sdk_mgr/installers.py`:
- Around line 45-50: The current unconditional infix strip in the installer (the
block that checks if "/" in version and does version.rsplit("/",1)[-1] before
calling normalize_version and go_module_path and writing dist_dir / ".version")
can hide mismatches between the provided source and the version prefix; update
this logic to detect and validate the prefix when a slash is present: extract
the prefix and compare it against the expected infix for the given source (or
the canonical module path produced by go_module_path) and only strip when they
match, otherwise either leave the version as-is and/or emit a warning log
indicating a source/tag mismatch; ensure normalize_version is still called on
the final version and that the .version file continues to be written using
module and tag only after validation.
- Around line 60-66: Current code treats go install failures for
GO_MODULE_PATH_PLATFORM by raising InstallError while other modules only warn
and defer, which is asymmetric; either document the intentional behavior or
change logic to distinguish "module not found" vs transient errors by inspecting
result.stderr. Update the block around the go install check (use variables
result, msg, GO_MODULE_PATH_PLATFORM, and InstallError) to: if the
platform-special-case behavior is intentional, add a brief inline comment
explaining the migration/publishing rationale; otherwise parse result.stderr (or
result.returncode) to detect a not-found/publish-related message and raise
InstallError only for that case while treating other transient errors like the
non-platform path (print warning and retry at runtime). Ensure the message
includes the module and tag context as currently constructed.
In `@otdf-sdk-mgr/src/otdf_sdk_mgr/registry.py`:
- Around line 115-122: The overlap notice printing for every shared version is
noisy; modify the logic around the "if version in seen" branch (the print to
sys.stderr) to either: a) respect a verbosity/debug flag (e.g., check an
existing verbose/debug parameter or add one to the surrounding function and only
emit the message when verbose is true, or switch to using a logger and call
logger.debug instead of print), or b) collect overlapping versions into a list
(e.g., overlaps or duplicated_versions) during the loop and, after the loop
completes, emit a single summary line to stderr/log (e.g., "Note: versions X
found in both...") if that list is non-empty. Update the surrounding function
signature and callers if you add a verbose flag, or switch to the module logger
for debug-level output so callers can control noise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 346508b2-a96f-4ce8-8a8d-e8e18536d4e2
📒 Files selected for processing (11)
.github/workflows/xtest.ymlotdf-sdk-mgr/README.mdotdf-sdk-mgr/src/otdf_sdk_mgr/cli_install.pyotdf-sdk-mgr/src/otdf_sdk_mgr/cli_versions.pyotdf-sdk-mgr/src/otdf_sdk_mgr/config.pyotdf-sdk-mgr/src/otdf_sdk_mgr/installers.pyotdf-sdk-mgr/src/otdf_sdk_mgr/registry.pyotdf-sdk-mgr/src/otdf_sdk_mgr/resolve.pyxtest/sdk/go/cli.shxtest/sdk/go/otdfctl.shxtest/setup-cli-tool/action.yaml
🚧 Files skipped from review as they are similar to previous changes (7)
- otdf-sdk-mgr/README.md
- xtest/sdk/go/cli.sh
- xtest/setup-cli-tool/action.yaml
- otdf-sdk-mgr/src/otdf_sdk_mgr/resolve.py
- .github/workflows/xtest.yml
- otdf-sdk-mgr/src/otdf_sdk_mgr/config.py
- xtest/sdk/go/otdfctl.sh
X-Test Results✅ java-main |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
otdf-sdk-mgr/src/otdf_sdk_mgr/resolve.py (1)
333-337:⚠️ Potential issue | 🟠 MajorNarrowed exception clause may swallow previously-handled errors from
list_java_github_releases().The switch from
except Exceptiontoexcept (git.exc.GitCommandError, ValueError)is generally good for specificity, but the Java "latest" branch at line 337 callslist_java_github_releases()within the try block. That function internally catches onlyurllib.error.URLError, allowing other exceptions to escape:
json.JSONDecodeError(if GitHub API returns invalid JSON)UnicodeDecodeError(if response encoding fails)socket.timeout(from the 30-second timeout)These exceptions are not in the narrowed except tuple and will propagate out of
resolve()instead of being converted to aResolveErrordict, breaking the previous behavior.Consider either:
- Adding exception types to the except tuple (e.g.,
json.JSONDecodeError,socket.timeout,OSError), or- Having
list_java_github_releases()catch and normalize these errors toValueErrorbefore they escape.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@otdf-sdk-mgr/src/otdf_sdk_mgr/resolve.py` around lines 333 - 337, The narrowed except clause in resolve() around the Java branch can let exceptions from list_java_github_releases() escape (e.g., json.JSONDecodeError, UnicodeDecodeError, socket.timeout, OSError), so update the exception tuple in the try/except that surrounds list_java_github_releases() to also catch json.JSONDecodeError, UnicodeDecodeError, socket.timeout and OSError (or alternatively have list_java_github_releases() normalize those into ValueError), ensuring resolve() converts those failures into the existing ResolveError dict instead of letting them propagate.
🧹 Nitpick comments (1)
otdf-sdk-mgr/src/otdf_sdk_mgr/resolve.py (1)
134-155:go_sourceis only validated on the platform path.
_go_platformis only true whensdk == "go" and go_source == "platform". For any other combination (e.g.sdk == "go"with a typo likego_source="standaloen", orsdk != "go"withgo_source="platform"), the value is silently ignored and the code falls through toSDK_GIT_URLS[sdk]. Sinceconfig._validate_go_sourcealready exists and raises on unknown values, calling it unconditionally (or at least whenevergo_source is not None) would catch caller mistakes instead of resolving from the wrong repo.Suggested tweak
+ from otdf_sdk_mgr.config import _validate_go_source + _validate_go_source(go_source) _go_platform = sdk == "go" and go_source == "platform"(or expose a public validator rather than importing the underscore-prefixed helper)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@otdf-sdk-mgr/src/otdf_sdk_mgr/resolve.py` around lines 134 - 155, The code only validates go_source when _go_platform is true; instead call the existing validator (config._validate_go_source) whenever go_source is not None (or otherwise expose/use a public validator) before computing _go_platform so typos like go_source="standaloen" are rejected; then compute _go_platform = (sdk == "go" and go_source == "platform") and proceed to use go_git_url/go_tag_infix for the platform path or SDK_GIT_URLS[sdk] for others, returning the same unknown-SDK error on KeyError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@otdf-sdk-mgr/src/otdf_sdk_mgr/resolve.py`:
- Around line 333-337: The narrowed except clause in resolve() around the Java
branch can let exceptions from list_java_github_releases() escape (e.g.,
json.JSONDecodeError, UnicodeDecodeError, socket.timeout, OSError), so update
the exception tuple in the try/except that surrounds list_java_github_releases()
to also catch json.JSONDecodeError, UnicodeDecodeError, socket.timeout and
OSError (or alternatively have list_java_github_releases() normalize those into
ValueError), ensuring resolve() converts those failures into the existing
ResolveError dict instead of letting them propagate.
---
Nitpick comments:
In `@otdf-sdk-mgr/src/otdf_sdk_mgr/resolve.py`:
- Around line 134-155: The code only validates go_source when _go_platform is
true; instead call the existing validator (config._validate_go_source) whenever
go_source is not None (or otherwise expose/use a public validator) before
computing _go_platform so typos like go_source="standaloen" are rejected; then
compute _go_platform = (sdk == "go" and go_source == "platform") and proceed to
use go_git_url/go_tag_infix for the platform path or SDK_GIT_URLS[sdk] for
others, returning the same unknown-SDK error on KeyError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6c6c70e9-4ca8-455c-aabf-7b4cf956a121
📒 Files selected for processing (2)
otdf-sdk-mgr/README.mdotdf-sdk-mgr/src/otdf_sdk_mgr/resolve.py
🚧 Files skipped from review as they are similar to previous changes (1)
- otdf-sdk-mgr/README.md
X-Test Results✅ java-main |
|
X-Test Results✅ java-v0.14.0 |
2 participants
otdfctlis moving from opentdf/otdfctl into opentdf/platform. This change updates the test infrastructure to auto-detect when the platform checkout containsotdfctl/and updates builds to use that path instead of the standalone repo.xtest.ymlworkflowotdfctl-sourceinput {auto|standalone|platform}otdfctl/go.modin theplatformdirsetup-cli-toolactionplatform-otdfctl-dirinput./sdk/go/src/for head builds instead of separate checkoutotdf-sdk-mgrcommand line toolresolve.pysupportsgo_source="platform"to resolve against the platform repo with otdfctl/ tag infix.versionfilecli.sh,otdfctl.sh.versionto use the correct module path (github.com/opentdf/platform/otdfctl) for go run fallbackBackward compatible!
Old releases still resolve from the standalone repo;
.module-pathabsence defaults to the original module path.Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
Summary by CodeRabbit
New Features
Refactor
Documentation
Bug Fix / UX