Internalize Compete and React Doctor as two Beta features - #11
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:
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 on-demand advisory evidence providers (compete and react-doctor) and experimental Beta features (competitive-intelligence and react-diagnostics) to the Code Intel pipeline, along with strict 'fail-closed' trust contracts for Hospital scoring and scoped Repowise egress validation. Feedback on these changes highlights a performance bottleneck in Run-ScopedRepowiseDocs.py caused by spawning git subprocesses in a loop, a compatibility issue in Invoke-EvidenceProvider.ps1 with Windows PowerShell 5.1, and a potential strict-mode runtime crash if JSON parsing returns null.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def _validate_file_entries( | ||
| repo_path: Path, | ||
| entries: object, | ||
| policy: str, | ||
| label: str, | ||
| ) -> dict[str, str]: | ||
| if not isinstance(entries, list): | ||
| raise RuntimeError(f"egress manifest {label} list is missing") | ||
|
|
||
| seen: dict[str, str] = {} | ||
| ordered_paths: list[str] = [] | ||
| for entry in entries: | ||
| if not isinstance(entry, dict): | ||
| raise RuntimeError(f"egress manifest contains an invalid {label} entry") | ||
| relative = entry.get("path") | ||
| expected_hash = entry.get("sha256") | ||
| if not isinstance(relative, str) or not isinstance(expected_hash, str): | ||
| raise RuntimeError(f"egress manifest {label} entry is incomplete") | ||
| if len(expected_hash) != 64 or any(char not in "0123456789abcdefABCDEF" for char in expected_hash): | ||
| raise RuntimeError(f"egress manifest contains an invalid SHA-256: {relative}") | ||
| posix_path = PurePosixPath(relative) | ||
| if posix_path.is_absolute() or ".." in posix_path.parts or relative in {"", "."}: | ||
| raise RuntimeError(f"egress manifest contains unsafe path: {relative}") | ||
| try: | ||
| candidate = (repo_path / Path(*posix_path.parts)).resolve(strict=True) | ||
| except OSError as exc: | ||
| raise RuntimeError(f"egress manifest path is unavailable: {relative}") from exc | ||
| if not candidate.is_relative_to(repo_path) or not candidate.is_file(): | ||
| raise RuntimeError(f"egress manifest path escapes the scoped repository: {relative}") | ||
| actual_hash = hashlib.sha256(candidate.read_bytes()).hexdigest() | ||
| if actual_hash != expected_hash.lower(): | ||
| raise RuntimeError(f"egress manifest hash mismatch: {relative}") | ||
| if relative in seen: | ||
| raise RuntimeError(f"egress manifest contains duplicate path: {relative}") | ||
| seen[relative] = expected_hash.lower() | ||
| ordered_paths.append(relative) | ||
|
|
||
| if policy == "head-tracked-only": | ||
| try: | ||
| head_blob_oid = subprocess.check_output( | ||
| ["git", "-C", str(repo_path), "rev-parse", f"HEAD:{relative}"], | ||
| text=True, | ||
| stderr=subprocess.STDOUT, | ||
| ).strip() | ||
| working_blob_oid = subprocess.check_output( | ||
| [ | ||
| "git", | ||
| "-C", | ||
| str(repo_path), | ||
| "hash-object", | ||
| f"--path={relative}", | ||
| str(candidate), | ||
| ], | ||
| text=True, | ||
| stderr=subprocess.STDOUT, | ||
| ).strip() | ||
| except (OSError, subprocess.CalledProcessError) as exc: | ||
| raise RuntimeError(f"egress manifest file is not tracked at HEAD: {relative}") from exc | ||
| if working_blob_oid != head_blob_oid: | ||
| raise RuntimeError(f"egress manifest file does not match HEAD blob: {relative}") | ||
|
|
||
| if ordered_paths != sorted(ordered_paths): | ||
| raise RuntimeError(f"egress manifest {label} list must be sorted by path") | ||
| return seen |
There was a problem hiding this comment.
Spawning two git subprocesses (git rev-parse and git hash-object) inside a loop for every file in the egress manifest is a significant performance bottleneck, especially on Windows where process creation overhead is high.\n\nWe can optimize this to O(1) subprocess calls by:\n1. Running git ls-tree -r HEAD once before the loop to retrieve all tracked file OIDs at HEAD.\n2. Computing the Git blob OID directly in Python using hashlib.sha1 on the file content, which we already read in memory.\n\nThis completely eliminates 2 * N process spawns and file reads.
def _validate_file_entries(\n repo_path: Path,\n entries: object,\n policy: str,\n label: str,\n) -> dict[str, str]:\n if not isinstance(entries, list):\n raise RuntimeError(f"egress manifest {label} list is missing")\n\n head_blobs: dict[str, str] = {}\n if policy == "head-tracked-only":\n try:\n ls_tree_out = subprocess.check_output(\n ["git", "-C", str(repo_path), "ls-tree", "-r", "HEAD"],\n text=True,\n )\n for line in ls_tree_out.splitlines():\n if line:\n parts = line.split(None, 2)\n if len(parts) >= 3:\n oid_path = parts[2].split('\t', 1)\n if len(oid_path) == 2:\n head_blobs[oid_path[1]] = oid_path[0]\n except (OSError, subprocess.CalledProcessError) as exc:\n raise RuntimeError(f"cannot read HEAD tree: {exc}") from exc\n\n seen: dict[str, str] = {}\n ordered_paths: list[str] = []\n for entry in entries:\n if not isinstance(entry, dict):\n raise RuntimeError(f"egress manifest contains an invalid {label} entry")\n relative = entry.get("path")\n expected_hash = entry.get("sha256")\n if not isinstance(relative, str) or not isinstance(expected_hash, str):\n raise RuntimeError(f"egress manifest {label} entry is incomplete")\n if len(expected_hash) != 64 or any(char not in "0123456789abcdefABCDEF" for char in expected_hash):\n raise RuntimeError(f"egress manifest contains an invalid SHA-256: {relative}")\n posix_path = PurePosixPath(relative)\n if posix_path.is_absolute() or ".." in posix_path.parts or relative in {"", "."}:\n raise RuntimeError(f"egress manifest contains unsafe path: {relative}")\n try:\n candidate = (repo_path / Path(*posix_path.parts)).resolve(strict=True)\n except OSError as exc:\n raise RuntimeError(f"egress manifest path is unavailable: {relative}") from exc\n if not candidate.is_relative_to(repo_path) or not candidate.is_file():\n raise RuntimeError(f"egress manifest path escapes the scoped repository: {relative}")\n content = candidate.read_bytes()\n actual_hash = hashlib.sha256(content).hexdigest()\n if actual_hash != expected_hash.lower():\n raise RuntimeError(f"egress manifest hash mismatch: {relative}")\n if relative in seen:\n raise RuntimeError(f"egress manifest contains duplicate path: {relative}")\n seen[relative] = expected_hash.lower()\n ordered_paths.append(relative)\n\n if policy == "head-tracked-only":\n head_blob_oid = head_blobs.get(relative)\n if head_blob_oid is None:\n raise RuntimeError(f"egress manifest file is not tracked at HEAD: {relative}")\n hasher = hashlib.sha1()\n hasher.update(f'blob {len(content)}\x00'.encode("utf-8"))\n hasher.update(content)\n working_blob_oid = hasher.hexdigest()\n if working_blob_oid != head_blob_oid:\n raise RuntimeError(f"egress manifest file does not match HEAD blob: {relative}")\n\n if ordered_paths != sorted(ordered_paths):\n raise RuntimeError(f"egress manifest {label} list must be sorted by path")\n return seen| function Get-Sha256Text([string]$Text) { | ||
| $bytes = [Text.Encoding]::UTF8.GetBytes($Text) | ||
| $hash = [Security.Cryptography.SHA256]::HashData($bytes) | ||
| return [Convert]::ToHexString($hash).ToLowerInvariant() | ||
| } |
There was a problem hiding this comment.
The static method [Security.Cryptography.SHA256]::HashData and the [Convert]::ToHexString method are only available in .NET Core / .NET 5+ (PowerShell 6+). If this script is executed under Windows PowerShell 5.1 (which uses .NET Framework 4.8), it will fail with a MethodNotFound exception.\n\nTo maintain compatibility with Windows PowerShell 5.1 (as indicated by the fallback to powershell in install-code-intel-pipeline.ps1), use the standard .NET Framework compatible APIs.
function Get-Sha256Text([string]$Text) {\n $bytes = [Text.Encoding]::UTF8.GetBytes($Text)\n $sha = [System.Security.Cryptography.SHA256]::Create()\n $hash = $sha.ComputeHash($bytes)\n $sha.Dispose()\n return [System.BitConverter]::ToString($hash).Replace('-', '').ToLowerInvariant()\n}
| $report = Get-Content -LiteralPath $reportPath -Raw | ConvertFrom-Json | ||
| if ($report.schemaVersion -ne 3) { throw "unsupported JSON schema" } | ||
| $notApplicable = $report.ok -eq $false -and | ||
| $null -ne $report.error -and | ||
| $report.error.name -eq "ProjectNotFoundError" |
There was a problem hiding this comment.
In PowerShell strict mode (Set-StrictMode -Version Latest), referencing properties on a null value throws a runtime exception. If ConvertFrom-Json returns $null (e.g., if the report file is empty or invalid), accessing $report.schemaVersion or $report.ok will crash the script.\n\nAdding an explicit $null -eq $report check prevents this crash.
$report = Get-Content -LiteralPath $reportPath -Raw | ConvertFrom-Json\n if ($null -eq $report) { throw 'React Doctor report is empty or invalid' }\n if ($report.schemaVersion -ne 3) { throw 'unsupported JSON schema' }\n $notApplicable = $report.ok -eq $false -and\n $null -ne $report.error -and\n $report.error.name -eq 'ProjectNotFoundError'
|
Closing after a merge-readiness review at main HEAD 384617f (2026-07-28). Why close rather than merge:
Salvageable idea: React Doctor as a Beta feature is still a reasonable concept — if wanted, re-spec it against main's current capability contract, with a pinned/verified toolchain instead of runtime |
1 participant
What changed
competitive-intelligenceandreact-diagnosticsBeta feature surfacesnormalorfullexecutionWhy
The two upstream projects are implementation engines for two Code Intel product features, not the product-facing feature model itself. This change makes the Code Intel report contract and CLI the public surface while retaining upstream provenance, diagnostic IDs, coverage, freshness, and snapshot validation.
Validation
cargo test --workspace— 35 passedtest-evidence-providers.ps1— 2 providers, 2 Beta features, 4 schemas, zero target-repo writestest-integration-orchestration.ps1— passedtest-atomic-capability-contract.ps1— passedsentrux gate .— no degradationtest-code-intel-pipeline.ps1 -RepoPath .— normal pipeline exit code 0