DSPX 3382 mlkem scenarios#463
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 implements a multi-instance test harness refactor, enabling concurrent local platform and KAS instances on disjoint port ranges, alongside Claude agent skills for scenario and feature design. It also adds support for pure ML-KEM key wrapping (mlkem:768 and mlkem:1024) and allows installing platform binaries at specific git refs using git worktrees. The review feedback highlights opportunities to improve robustness, such as handling missing git executables or network errors during remote tag queries, verifying worktree paths exist before setting them as working directories, removing redundant git fetch operations, and consistently specifying encoding="utf-8" on file read/write operations to avoid platform-dependent decoding issues.
| from git import Git | ||
|
|
||
| results: list[dict[str, Any]] = [] | ||
| raw = Git().ls_remote(SDK_GIT_URLS["platform"], tags=True) |
There was a problem hiding this comment.
If git is not installed on the system, or if there is a network outage, Git().ls_remote will raise git.exc.GitCommandNotFound or git.exc.GitCommandError. These exceptions are not caught in cli_versions.py (which only catches RegistryUnreachableError), leading to an unhandled traceback and crash. We should wrap the Git().ls_remote call in a try...except block and raise RegistryUnreachableError.
| raw = Git().ls_remote(SDK_GIT_URLS["platform"], tags=True) | |
| from git.exc import GitCommandError, GitCommandNotFound | |
| try: | |
| raw = Git().ls_remote(SDK_GIT_URLS["platform"], tags=True) | |
| except GitCommandNotFound as e: | |
| raise RegistryUnreachableError(f"git executable not found: {e}") from e | |
| except GitCommandError as e: | |
| raise RegistryUnreachableError(f"failed to query remote git platform tags: {e}") from e |
| worktree = binary.parent | ||
| version_file = binary.parent / ".version" | ||
| if version_file.exists(): | ||
| for line in version_file.read_text().splitlines(): | ||
| if line.startswith("worktree="): | ||
| worktree = Path(line.split("=", 1)[1].strip()) | ||
| break |
There was a problem hiding this comment.
If the repository has been moved to another location on disk, the absolute path recorded in .version will point to a non-existent directory. Setting cwd=worktree in subprocess.run or ProcessManager.start will then crash with a FileNotFoundError. We should check if the candidate worktree path exists before using it, falling back to binary.parent if it doesn't. Also, read_text() should specify encoding="utf-8".
| worktree = binary.parent | |
| version_file = binary.parent / ".version" | |
| if version_file.exists(): | |
| for line in version_file.read_text().splitlines(): | |
| if line.startswith("worktree="): | |
| worktree = Path(line.split("=", 1)[1].strip()) | |
| break | |
| worktree = binary.parent | |
| version_file = binary.parent / ".version" | |
| if version_file.exists(): | |
| for line in version_file.read_text(encoding="utf-8").splitlines(): | |
| if line.startswith("worktree="): | |
| candidate = Path(line.split("=", 1)[1].strip()) | |
| if candidate.exists(): | |
| worktree = candidate | |
| break |
| worktree = binary.parent # safe fallback | ||
| version_file = binary.parent / ".version" | ||
| if version_file.exists(): | ||
| for line in version_file.read_text().splitlines(): | ||
| if line.startswith("worktree="): | ||
| worktree = Path(line.split("=", 1)[1].strip()) | ||
| break |
There was a problem hiding this comment.
If the repository has been moved to another location on disk, the absolute path recorded in .version will point to a non-existent directory, causing a crash when setting cwd=worktree. We should check if the candidate worktree path exists before using it, falling back to binary.parent if it doesn't. Also, read_text() should specify encoding="utf-8".
| worktree = binary.parent # safe fallback | |
| version_file = binary.parent / ".version" | |
| if version_file.exists(): | |
| for line in version_file.read_text().splitlines(): | |
| if line.startswith("worktree="): | |
| worktree = Path(line.split("=", 1)[1].strip()) | |
| break | |
| worktree = binary.parent # safe fallback | |
| version_file = binary.parent / ".version" | |
| if version_file.exists(): | |
| for line in version_file.read_text(encoding="utf-8").splitlines(): | |
| if line.startswith("worktree="): | |
| candidate = Path(line.split("=", 1)[1].strip()) | |
| if candidate.exists(): | |
| worktree = candidate | |
| break |
| # Worktrees from a bare clone have no `origin` remote, so reset | ||
| # to the just-fetched ref in the bare repo instead of `git pull`. | ||
| _run(["git", "-C", str(worktree_path), "fetch", "origin", branch]) | ||
| _run(["git", "-C", str(worktree_path), "reset", "--hard", "FETCH_HEAD"]) |
There was a problem hiding this comment.
The comment states that worktrees from a bare clone have no origin remote, but the code immediately runs git fetch origin. Since the bare repository has already fetched the updates (at line 72), the ref is already updated in the shared database. Running git fetch again inside the worktree is redundant and slow. We can simplify this by resetting directly to the 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`. | |
| _run(["git", "-C", str(worktree_path), "fetch", "origin", branch]) | |
| _run(["git", "-C", str(worktree_path), "reset", "--hard", "FETCH_HEAD"]) | |
| # Worktrees from a bare clone share the bare repo's database, so we | |
| # reset to the bare repo's just-fetched ref instead of fetching again. | |
| _run(["git", "-C", str(worktree_path), "reset", "--hard", branch]) |
| _run(["git", f"--git-dir={bare_repo_path}", "fetch", "origin", ref, "--tags"]) | ||
| _run(["git", "-C", str(worktree_path), "checkout", "--force", "FETCH_HEAD"]) |
There was a problem hiding this comment.
The bare repository has already fetched the updates (at line 140 or 145). Running git fetch again is redundant and slow. We can simplify this by checking out the ref directly.
| _run(["git", f"--git-dir={bare_repo_path}", "fetch", "origin", ref, "--tags"]) | |
| _run(["git", "-C", str(worktree_path), "checkout", "--force", "FETCH_HEAD"]) | |
| _run(["git", "-C", str(worktree_path), "checkout", "--force", ref]) |
| from ruamel.yaml import YAML | ||
|
|
||
| y = YAML(typ="safe") | ||
| raw = y.load(scenario_path.read_text()) |
There was a problem hiding this comment.
read_text() is called without specifying an encoding, which can lead to platform-dependent decoding errors (e.g., on Windows). It is safer to specify encoding="utf-8".
| raw = y.load(scenario_path.read_text()) | |
| raw = y.load(scenario_path.read_text(encoding="utf-8")) |
| print(f" Warning: {msg} (will retry at runtime)") | ||
| raise InstallError(msg) | ||
| dist_dir.mkdir(parents=True, exist_ok=True) | ||
| (dist_dir / ".version").write_text(f"{module}@{tag}\n") |
There was a problem hiding this comment.
write_text() is called without specifying an encoding, which can lead to platform-dependent encoding errors. It is safer to specify encoding="utf-8".
| (dist_dir / ".version").write_text(f"{module}@{tag}\n") | |
| (dist_dir / ".version").write_text(f"{module}@{tag}\n", encoding="utf-8") |
| def _record_version(dist_dir: Path, ref: str, worktree: Path) -> None: | ||
| """Write a `.version` metadata file alongside the binary.""" | ||
| sha = _git_rev_parse(worktree, "HEAD") | ||
| (dist_dir / ".version").write_text(f"ref={ref}\nsha={sha}\nworktree={worktree}\n") |
There was a problem hiding this comment.
write_text() is called without specifying an encoding, which can lead to platform-dependent encoding errors. It is safer to specify encoding="utf-8".
| (dist_dir / ".version").write_text(f"ref={ref}\nsha={sha}\nworktree={worktree}\n") | |
| (dist_dir / ".version").write_text(f"ref={ref}\nsha={sha}\nworktree={worktree}\n", encoding="utf-8") |
| subprocess.CalledProcessError, | ||
| OSError, | ||
| ) as e: | ||
| out.write_text(json.dumps(_snapshot(status="partial"), indent=2) + "\n") |
There was a problem hiding this comment.
write_text() is called without specifying an encoding, which can lead to platform-dependent encoding errors. It is safer to specify encoding="utf-8".
| out.write_text(json.dumps(_snapshot(status="partial"), indent=2) + "\n") | |
| out.write_text(json.dumps(_snapshot(status="partial"), indent=2) + "\n", encoding="utf-8") |
| typer.echo(f" Wrote partial manifest to {out}", err=True) | ||
| raise typer.Exit(1) | ||
|
|
||
| out.write_text(json.dumps(_snapshot(), indent=2) + "\n") |
There was a problem hiding this comment.
write_text() is called without specifying an encoding, which can lead to platform-dependent encoding errors. It is safer to specify encoding="utf-8".
| out.write_text(json.dumps(_snapshot(), indent=2) + "\n") | |
| out.write_text(json.dumps(_snapshot(), indent=2) + "\n", encoding="utf-8") |
dmihalcik-virtru
force-pushed
the
DSPX-3302-05-claude-plugin
branch
from
26cafa1 to
962c03a
Compare
dmihalcik-virtru
force-pushed
the
DSPX-3382-mlkem-scenarios
branch
from
2970620 to
1949128
Compare
X-Test Failure Report |
1 similar comment
X-Test Failure Report |
dmihalcik-virtru
force-pushed
the
DSPX-3382-mlkem-scenarios
branch
2 times, most recently
from
3601aa4 to
b12abe7
Compare
dmihalcik-virtru
force-pushed
the
DSPX-3302-05-claude-plugin
branch
from
d787998 to
ad4b4a3
Compare
X-Test Failure Report |
Scenario YAML files can now configure KAS preview settings via the features dict, avoiding manual edits to generated opentdf.yaml files that don't persist through service restarts. - Documents KasPin.features field with open-ended description for forward compatibility with new platform preview settings - Updates pure-mlkem scenario to enable hybrid_tdf_enabled for ML-KEM - Adds FEATURES.md guide with examples and precedence rules - Regenerates JSON schemas from Pydantic models Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
dmihalcik-virtru
force-pushed
the
DSPX-3382-mlkem-scenarios
branch
from
b12abe7 to
7ec8c8c
Compare
dmihalcik-virtru
force-pushed
the
DSPX-3302-05-claude-plugin
branch
from
ad4b4a3 to
256b593
Compare
X-Test Failure Report |
|
X-Test Failure Report |
1 participant
Stack (
a60d3302):Generated by
wgo stack. Edit text above or below this block, not inside it.