feat: AUT-2166 add reusable isolated review components

[svcnemo-autobot]

Background

Megatron-Bridge review requires auditable isolation between untrusted PR content, model analysis, and GitHub publication without maintaining a custom inference loop.

What changed

• Use an unmodified, full-SHA-pinned Claude Code Base Action only in the isolated analyzer.
• Preserve immutable MCP context, audit-backed validation, authorization and revision gates, and model-free publication.
• Focus the independent test workflow on Python modules and essential isolation contracts.
• Require human-readable Napoleon-style docstrings across the added Python code and tests.

Details

flowchart LR
  C[Immutable context] --> A[Permissionless Base Action]
  A --> V[Audit-bound validation]
  V --> P[Model-free publisher]

Loading

Analysis runs in an empty trusted directory with permissions: {}, no PR-head checkout, no project settings or plugins, and only bounded review_context MCP tools. The pinned Base Action checkout remains unmodified. Shell, filesystem, web, and mutation tools are denied. Only the validated structured result and retrieval audit are uploaded.

Tested

• 36 Python module and isolation tests
• Ruff component check
• Python 3.11 compilation
• Actionlint on affected workflows
• Clean pinned Base Action checkout verification
• git diff --check and repository secrets detection deferred to CI because the typed workstation materialization lacks .git metadata

[chtruong814]

@svcnemo-autobot Requesting changes. The isolation architecture is directionally correct, but this does not yet satisfy the security and compatibility contract for Megatron-Bridge.

1. The analyzer cannot obtain the review context it needs. review.diff is stored but no MCP operation exposes its text; diff_hunks returns only line coordinates, and read returns base64. Reads and searches are also restricted to changed-file snapshots, so the reviewer cannot load trusted BASE_SHA instructions/skills or inspect unchanged definitions, callers, tests, and performance configurations. The validator accepts model-reported coverage counts without proving them from the retrieval audit, so an incomplete review can still be marked complete. Please expose bounded textual diff/source retrieval, permit bounded trusted-base repository reads/searches, capture governing base instructions and skills, and derive completeness from audited retrieval.

2. The existing Megatron-Bridge prompt is passed through unchanged even though its tools no longer exist. It requires Read, trusted skills, gh pr comment, and inline-comment tools, none of which are available in the analysis job. The old publication instructions should not be retained, but they need to be replaced by a trusted, tool-compatible wrapper that explains the immutable context, treats PR data as untrusted, requires trusted-base instructions and complete changed-file accounting, defines incomplete behavior, and asks only for structured output. Repository-specific review policy can remain caller-owned.

3. Automatic Megatron-Bridge review remains on the original combined privileged path. This PR provides only the manual issue_comment composition, while companion PR NVIDIA-NeMo/Megatron-Bridge#5854 changes only the manual workflow pin. The current automatic same-repository review still gives the model OIDC/GitHub publication capability. Please provide and adopt an isolated automatic-review entrypoint, or a generic orchestrator that supports both modes, before treating the original concern as resolved.

4. Publication can leave partial findings. Inline findings are posted one at a time. Revision drift or an API error after some posts leaves partial comments and may omit the final status. The composed top-level body is not bounded to the GitHub comment limit either. Please preflight all output and use one COMMENT review request containing the body and inline comments so publication is atomic to the extent GitHub supports it.

5. Existing lifecycle behavior regresses. The eyes acknowledgment was removed; context/analysis failures do not produce a visible incomplete status because publish requires successful dependencies; concurrency covers only publication, allowing an older analysis to publish after a newer run; and the prior 45-minute timeout has no replacement. Please restore acknowledgment and deterministic failure status, apply concurrency to the complete orchestration, and add explicit time/turn budgets.

6. Trigger matching is still not exact. startsWith(...) accepts arbitrary suffixes and there is no optional expected-head SHA parsing. Please use an exact normalized first-line command grammar and validate any supplied 40-character SHA against the captured head.

7. Copyright/license headers are incomplete. review_components.py has the full Apache header, but both new Python test files use only a two-line abbreviated license, and the three new reusable YAML workflows have no header. Please use the full repository-standard NVIDIA copyright and Apache 2.0 block. The JSON schema can remain exempt because JSON does not support comments.

The focused tests pass when Git signing is explicitly disabled, but the fixture inherits global commit.gpgsign and otherwise fails before exercising 12 tests; please disable signing in the fixture. The current tests also do not demonstrate an end-to-end analyzer, publisher, authorized-fork/OIDC flow, stale revision, partial-publication prevention, or automatic-review migration.

The good pieces should remain: id-token: write is correctly confined to the model-free publisher, analysis has no GitHub/OIDC permissions, publication has no model or proposed-code checkout, artifacts are revision/digest bound, and the publisher fails rather than falling back from claude[bot] identity.

[chtruong814]

@svcnemo-autobot The isolation direction remains good, but the reference composition is not ready to merge yet. I left focused comments on the artifact round trip, governing symlink handling, trustworthy search coverage, and making the implementation easier for humans to audit. Each comment includes a concrete suggested fix and the missing regression coverage.

[chtruong814]

This artifact cannot be consumed as currently packaged. Both files are under .github, which actions/upload-artifact excludes by default unless include-hidden-files: true is set. In addition, uploading these two paths uses their least common ancestor as the artifact root, so the downloaded files would be at trusted-tools/review_components.py and trusted-tools/review-output-v1.schema.json, while the publisher invokes a nested trusted-tools/components/.github/... path.

Suggested fix: copy the script and schema into a non-hidden staging directory, upload that directory, download it to trusted-tools, and invoke trusted-tools/review_components.py. Please add an artifact round-trip test that uploads, downloads, and executes the tool from the resulting layout.

[chtruong814]

This makes the intended consumer repositories permanently incomplete when CLAUDE.md is a symlink to AGENTS.md. Both names are classified as governing paths, but the symlink is marked unavailable and retrieval_coverage() later requires every governing path to be available and fully read.

Suggested fix: resolve trusted-base symlinks strictly through captured Git objects, allowing only safe relative targets with traversal rejection, cycle detection, and a small depth bound. A simpler alternative is to treat a safe CLAUDE.md -> AGENTS.md alias as satisfied after the captured AGENTS.md content is read. Please add the exact symlink arrangement to the component tests.

[chtruong814]

Unavailable trusted-base files are silently skipped here. Once the context budget is exhausted, a search with no matches is indistinguishable from a complete repository search with no matches, so the reviewer can draw an absence-based conclusion from partial evidence.

Suggested fix: include search-scope metadata such as files_total, files_searched, files_unavailable, and scope_complete. The analyzer prompt and validation contract should prevent absence-based conclusions when scope_complete is false. Please cover both a complete search and a budget-exhausted search in tests.

[chtruong814]

The workflow decomposition is clear, but this 1,293-line module is difficult for a human to audit because it combines Git parsing, snapshot construction, retrieval budgets, MCP protocol handling, output validation, OIDC exchange, GitHub publication, and CLI dispatch. That concentration makes the security boundaries and invariants harder to verify locally.

Suggested change: retain one small executable entrypoint, but split the implementation into focused modules such as context.py, retrieval.py, validation.py, publisher.py, and cli.py. Use typed records (dataclass or TypedDict) for manifests, tree entries, coverage, and validated output instead of passing broad dict[str, Any] structures across every boundary.

[chtruong814]

@svcnemo-autobot The latest commit satisfactorily fixes the trusted-tool artifact layout and the CLAUDE.md -> AGENTS.md governing-symlink case. The focused suite also passes locally (22 component tests, 8 workflow tests, and compilation). I left two remaining recommendations: keep incomplete-search metadata bounded under the retriever byte budget, and replace the refactor's wildcard imports with explicit dependencies so the modules are genuinely easy to audit and Ruff-clean.

[chtruong814]

The new scope metadata fixes the ambiguity, but files_unavailable is still unbounded: it can contain every unavailable entry from a tree of up to 50,000 paths. Serializing that list can exceed the retriever's 16 MiB output budget, causing a broad search to fail before it can report that its scope was incomplete.

Please return a bounded contract instead—for example files_unavailable_count, counts grouped by reason, and a capped files_unavailable_sample—along with files_total, files_searched, and scope_complete. Add a regression test with enough unavailable, long-path entries to prove the response remains within its byte budget.

[chtruong814]

Splitting the monolith is a good structural improvement, but the wildcard import keeps each module's dependencies implicit. contracts.py currently re-exports standard-library modules, operational helpers, constants, and types, so names such as Path, Any, git, and ReviewError appear without local imports. This makes the security boundaries harder to audit and currently causes ruff check .github/actions/isolated-claude-review to fail with 355 errors, predominantly F405.

Please replace the wildcard imports throughout reviewlib with explicit standard-library and project imports. Keep contracts.py focused on shared constants, errors, and accurate wire types; move general helpers to a clearly named utility module if necessary. Then run Ruff over this component and correct the TypedDict definitions so they represent the records actually exchanged between modules.

[chtruong814]

@svcnemo-autobot A few focused follow-ups from checking the template against the Megatron-LM light/strict caller:

1. The secrets-detector failure is an intentional test fixture, not a credential. Please keep the negative URL validation test and annotate only that literal with # pragma: allowlist secret:

"https://user:password@inference.example.invalid",  # pragma: allowlist secret

This exact change passes the repository detector command while preserving the test. Please avoid excluding the file, weakening the detector, or adding a broad baseline entry.

2. Please make concurrency profile-aware. The current group is shared by every review on a PR, so a light review and strict review can cancel each other. A small optional review_profile input, included in the concurrency group and set by callers to light or strict, would preserve cancellation of stale runs within one profile without cross-cancelling independent profiles. Keep review_mode for manual versus automatic.

3. There is an easy CI improvement: the new test_review_components.py and test_workflows.py suites are not currently invoked by pre-flight.yml. Add a small Ubuntu job that checks out the PR and runs:

python3 -m unittest discover -s .github/actions/isolated-claude-review -p "test_*.py"

Please also add a small Megatron-LM-shaped contract fixture covering two manual callers with distinct trigger phrases and review profiles. It can remain a no-network test that verifies the reusable input contract, mutually independent concurrency keys, and policy-only prompts; it does not need inference credentials or GitHub publication. If actionlint is already available in CI, running it over the reusable workflows and fixture would provide an additional inexpensive syntax check.

[chtruong814]

@svcnemo-autobot One clarification on the CI recommendation: please wire the isolated-review test job into this repository existing pre-flight.yml, which already uses the pull_request event. That makes the tests run automatically when a PR is opened, reopened, or updated, including this PR, without creating a live Claude review or requiring inference/GitHub App credentials.

A focused job is sufficient:

isolated-review-tests:
  name: Isolated review tests
  runs-on: ubuntu-latest
  steps:
    - name: Checkout
      uses: actions/checkout@<full-commit-sha>
    - name: Run isolated review tests
      run: python3 -m unittest discover -s .github/actions/isolated-claude-review -p "test_*.py"

Please keep it as a required PR check if repository policy supports that. This gives every proposed template change immediate, credential-free component and workflow-contract coverage.

[svcnemo-autobot]

ℹ️ The workflow consumed the already-terminal exact-head CI handoff: item fwci-components head c6f87242ca88b6a4a96f1259d337005a0859e7f5 is failure. No terminal head was left parked or registered; each retained verdict is driving its normal terminal continuation.

[chtruong814]

@svcnemo-autobot I'm not sure if the megatron-lm mock workflow fixture test is useful. In fact, all of the tests do not seem useful because they mostly test the Github Action config? Can we add tests that test the python modules themselves? Remove those megatron-lm workflow related tests and focus on testing the python code. Also, breakout the test from the preflight test to its own independent one.

Also, ensure all added Python code include tests have human readable docstrings following Napoleon style.

[chtruong814]

@svcnemo-autobot Please consider replacing the repository-owned direct inference loop with the pinned Claude Code Base Action, used only inside the isolated analysis job. This should reduce custom API/tool-loop code while preserving the current context and publication boundaries.

Recommended shape:

• Keep the existing authorize job. The Base Action does not validate the triggering actor, so the exact command, write-permission, and optional head-SHA checks must remain prerequisites for analysis.
• Keep immutable context construction, the bounded/audited mcp-server, output validation, revision checks, and the model-free publisher.
• Run the Base Action with permissions: {} in an empty trusted working directory. Do not check out the PR head into that directory.
• Expose only the review-context MCP tools. Explicitly disable shell, filesystem, web, and GitHub mutation tools, and do not install plugins or accept project-local settings.
• Continue treating only captured BASE_SHA AGENTS.md, CLAUDE.md, CODEOWNERS, CONTRIBUTING.md, and skills/*/SKILL.md as governing instructions. PR-head versions may be reviewed as untrusted changed content but must never be auto-loaded as configuration.
• Request schema-constrained structured_output, pass it through the existing audit-backed validator, and upload only the validated result plus retrieval audit with the separately pinned upload-artifact action. Do not upload the Base Action execution transcript, and keep full output disabled.

This should allow removal of most or all of reviewlib/analyzer.py, its CLI wiring, and analyzer-specific transport/protocol tests. The more substantial isolation code in context, retrieval, validation, authorization, and publication should remain. Please add focused tests proving that unauthorized triggers cannot reach the Base Action, only the bounded MCP tools are configured, proposed instruction files are never present in the action working directory, and invalid structured output is rejected before upload or publication.