refactor(e2e): extract Brev branch-validation lifecycle into trusted tools/e2e helpers#6983
refactor(e2e): extract Brev branch-validation lifecycle into trusted tools/e2e helpers#6983souvikDevloper wants to merge 8 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:
📝 WalkthroughWalkthroughThe PR extracts Brev lifecycle operations into trusted TypeScript tooling, adds pure lifecycle logic and tests, and updates the E2E workflow to invoke trusted installation, debugging, teardown, and reporting commands. ChangesBrev lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Workflow
participant BrevLifecycle
participant BrevCLI
participant GitHubAPI
Workflow->>BrevLifecycle: Run install, collect-debug, delete-instance, or report-pr
BrevLifecycle->>BrevCLI: Install, inspect, refresh, collect, or delete instance
BrevLifecycle->>GitHubAPI: Validate head SHA and publish check-run/comment
Possibly related issues
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 @.github/workflows/e2e-branch-validation.yaml:
- Around line 297-309: Require the teardown deletion step to run only after the
trusted checkout succeeds: assign the checkout step an ID and add that step’s
successful outcome to the deletion step’s existing always() and keep_alive
condition in .github/workflows/e2e-branch-validation.yaml (lines 297-309).
Update test/brev-nightly-workflow.test.ts (lines 168-174) to assert that the
deletion condition references the trusted checkout outcome.
In `@test/brev-nightly-workflow.test.ts`:
- Around line 168-174: Extend the test around the trusted checkout and cleanup
assertions to verify the cleanup step is conditionally gated on successful
completion of “Checkout trusted lifecycle tooling for teardown.” Assert the
relevant cleanup condition or expression reflects checkout success, while
retaining only behavior-focused checks for the trusted checkout and cleanup
command.
In `@test/e2e/support/brev-lifecycle.test.ts`:
- Line 45: Update the affected test titles in the describe blocks and related
cases, including selectBrevLogin, so issue reference `#6962` appears only as the
final standalone suffix “(`#6962`)”; preserve the existing descriptive title text
and apply the same format to all referenced occurrences.
In `@tools/e2e/brev-lifecycle.mts`:
- Around line 354-359: Update the sshCollect and scpBundle callbacks used by
realRunner() to capture the run() result and propagate its nonzero exit status
instead of discarding it. Preserve the existing SSH/SCP arguments while ensuring
failed remote collection is returned to the caller so it can emit a warning and
report failure.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7b27ec35-beb1-45e1-9dd4-51850292eb2c
📒 Files selected for processing (5)
.github/workflows/e2e-branch-validation.yamltest/brev-nightly-workflow.test.tstest/e2e/support/brev-lifecycle.test.tstools/e2e/brev-lifecycle-core.mtstools/e2e/brev-lifecycle.mts
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Thanks for the substantial extraction work and the direct tests around the pure lifecycle decisions. The intended separation is good, but the workflow does not yet enforce the claimed trust boundary securely.
-
Blocking: target-branch code newly receives the raw Brev API key and organization values. The workflow checks out the requested branch and then invokes that branch's
tools/e2e/brev-lifecycle.mts install-cliwithBREV_API_KEY,BREV_ORG_ID, and the token. Previously the API-key login was trusted inline workflow code. Although the validation data plane already receives a Brev token, this unnecessarily widens the credential material available to tested code. Please load the install/auth implementation from an immutable trusted source or avoid passing the additional credentials to branch-controlled code. -
Blocking: teardown is not reliably isolated from previously executed target code. Checking
github.workflow_shaout under.trusted-e2efixes the source revision, but the directory remains writable in the same job after arbitrary tested code has run. A process left by the target can rewrite the helper before deletion executes. Please run cleanup in a separate constrained job, or install and verify it in a location the tested workload cannot mutate. -
Blocking: the write-capable reporter uses mutable
actions/setup-node@v6. This job haschecks: writeandpull-requests: write; pin setup-node to an immutable commit as the repository does for other actions. -
The branch is conflicting and currently drops behavior already merged in #6978. Main's reporter now has
actions: readand resolves the validation job URL for the check'sdetails_url. Please rebase and preserve that behavior in the extracted reporter and its tests. This also needs the coordination with #6145 requested by #6962. -
Debug-command failures are discarded. The real runners return statuses, but the
collect-debugcallbacks ignore them, so the surrounding warning path does not observe ordinary SSH/SCP failures. Please preserve the former best-effort diagnostics behavior and add direct CLI-wiring coverage.
The pure decision layer is a solid start; the remaining work is to make the effectful wiring uphold the same security and behavior guarantees.
…helpers The e2e-branch-validation workflow embedded four executable programs in YAML: pinned CLI install/auth, failure-bundle collection, retrying instance deletion, and a write-capable PR/check reporter. None were directly testable, and the cleanup and publication logic could drift with the tested branch. Move them into tools/e2e, keeping YAML responsible for secrets, instance identity, keep_alive, permissions, and always-run ordering. - Add tools/e2e/brev-lifecycle-core.mts: pure login selection, checksum/URL pinning, brev ls --json shape handling, deletion-presence classification, result mapping, PR comment rendering, and PR-number/stale-SHA guards. - Add tools/e2e/brev-lifecycle.mts: install-cli, collect-debug, delete-instance, and report-pr subcommands driving that logic through injected runners. - Rewire the workflow to invoke the helper. install-cli and collect-debug run from the tested checkout in the secret-bearing data-plane job; delete-instance and report-pr, being cleanup and publication, load the helper only from the trusted workflow revision (github.workflow_sha), never the tested branch. The reporter job still receives no Brev/inference secret and checks out no target code. Behavior is preserved: CLI checksum verification, API-key/org login with the legacy refresh-token fallback, onboarding suppression and readiness, best-effort debug collection before deletion, retrying/idempotent deletion that validates brev ls --json and fails visibly after the final attempt, and publication that rejects invalid PR numbers and stale tested SHAs before any write. Tests in test/e2e/support/brev-lifecycle.test.ts cover the install/auth branches, list JSON variants, deletion outcomes, result mapping, and stale-head rejection directly. The brev-nightly workflow contract test now asserts the extracted structure and trusted-load boundary instead of inline script text. Refs NVIDIA#6962 Signed-off-by: Souvik Ghosh <138186578+souvikDevloper@users.noreply.github.com>
Addresses the blocking review on NVIDIA#6983 and rebases onto main. - Teardown moves to its own cleanup-brev-instance job. Pinning .trusted-e2e to github.workflow_sha fixed the source revision, but the directory stayed writable in the validation job after arbitrary tested code had run, so a process left behind by the target could rewrite the helper before deletion executed. The new job checks out only the trusted revision, never the tested branch, and installs its own Brev CLI. - The Brev CLI install stays inline in the validation job rather than calling the branch's helper. Extracting it there newly handed BREV_API_KEY and BREV_ORG_ID to target-controlled code; the install/auth implementation is now only invoked from the trusted cleanup job, where no target code is present. - The write-capable reporter pins actions/setup-node to an immutable commit. - Preserve NVIDIA#6978: the reporter resolves the validation job and links the check through details_url, falling back to the run URL. resolveValidationJobUrl ports the jq guard's rules — bounded, internally consistent payload, jobs belonging to this exact run and attempt, and exactly one match — and treats the listing as untrusted input. - Debug collection no longer discards command failures: the ssh/scp runners return their status and a non-zero result reaches the warning path. Tests cover the job-link matching rules and every fallback, the debug-failure warnings, and the workflow contract now asserts the isolated cleanup job, the trusted-only checkouts, and pinned actions. Refs NVIDIA#6962 Signed-off-by: Souvik Ghosh <138186578+souvikDevloper@users.noreply.github.com>
a292b04 to
4e51596
Compare
|
Thanks @cv — every one of these was fair, and two were security regressions I introduced rather than inherited. Rebased onto Blocking: target code newly received Fix: the CLI install stays inline in the validation job. The extracted install/auth implementation is now only invoked from the new trusted cleanup job, where no target code is present. This is the same reasoning I applied to Docker-auth in #6961 — secret-bearing auth should not become branch-controlled just to satisfy an extraction. Blocking: teardown not isolated. Also right, and a subtle one I'd missed: pinning Blocking: mutable Dropping #6978. Rebased and preserved. Debug failures discarded. Fixed — On the #6145 coordination you and #6962 both asked for: this remains behavior-preserving and touches only executable bodies plus job topology for the trust boundary, so it composes with the trigger work rather than conflicting.
Signed-off-by: Souvik Ghosh 138186578+souvikDevloper@users.noreply.github.com |
The changed-test if-statement guardrail rejects conditional test bodies. Assert the reporter's action pins by filtering to the unpinned set and expecting it to be empty, instead of branching per step. Refs NVIDIA#6962 Signed-off-by: Souvik Ghosh <138186578+souvikDevloper@users.noreply.github.com>
|
@cv can i get the ci now |
wscurran
added
area: ci
|
✨ Thanks for the refactor. Extracting the Brev branch-validation lifecycle into tested helpers improves maintainability and security by removing executable logic from branch-influenced YAML. Ready for maintainer review. Related open issues:
Related open issues: |
|
@wscurran This PR's CI is stalled on run vetting rather than failing on content: because the diff modifies Could a maintainer/vetter approve the workflow runs for this PR so PR CI and the trusted coordination can complete? Happy to rebase onto latest |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Addresses the static-checks, source-shape, and CodeQL findings on the PR CI run for NVIDIA#6983. - Write the Brev CLI tarball into a private mkdtemp directory instead of a fixed name in the shared temp dir, and remove it after extraction; a predictable path could be pre-created or symlinked by another local user before tar extracts into /usr/local/bin (CodeQL js/insecure-temporary-file). - Send the PR comment body to gh over stdin (--body-file -) instead of a predictable temp file (CodeQL js/insecure-temporary-file). - Validate environment-supplied argv inputs before they reach ssh/scp/brev/gh: instance names and destination directories must not start with '-' (ssh option injection such as -oProxyCommand), the repository must be an owner/name slug, and run identifiers must be decimal (CodeQL js/indirect-command-line-injection). - Mark tools/e2e/brev-lifecycle.mts executable to match its shebang (check-shebang-scripts-are-executable). - Move local issue references in brev-lifecycle test titles to a final '(#NNNN)' suffix (test-title-style). - Record the renamed brev-nightly reporter test in the source-shape contract-exception allowlist under its new title. Refs NVIDIA#6962 Signed-off-by: Souvik Ghosh <138186578+souvikDevloper@users.noreply.github.com>
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 `@tools/e2e/brev-lifecycle-core.mts`:
- Around line 291-295: Update assertRepository to reject repository values whose
owner or name component is exactly "." or ".." before returning, while
preserving the existing owner/name format validation for all other values.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1b962f3a-afde-47b0-9b23-5c6b91cda36d
📒 Files selected for processing (4)
ci/source-shape-test-budget.jsontest/e2e/support/brev-lifecycle.test.tstools/e2e/brev-lifecycle-core.mtstools/e2e/brev-lifecycle.mts
🚧 Files skipped from review as they are similar to previous changes (2)
- tools/e2e/brev-lifecycle.mts
- test/e2e/support/brev-lifecycle.test.ts
Addresses the CodeRabbit review on NVIDIA#6983: "../target" and "owner/.." passed the owner/name pattern but would traverse to a different GitHub API endpoint once spliced into repos/<repo>/... paths. Reject any dot-only path component and cover the shapes in the argv-guard tests. Refs NVIDIA#6962 Signed-off-by: Souvik Ghosh <138186578+souvikDevloper@users.noreply.github.com>
|
@cv can i get the ci now? |
Co-authored-by: Souvik Ghosh <138186578+souvikDevloper@users.noreply.github.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Dismissed as outdated after exact-head verification. Later commits moved teardown into a separate trusted-workflow-SHA job, removed Brev API key/org exposure from target-controlled helpers, pinned the write-capable reporter action, preserved #6978 actions:read and job details_url behavior, propagated SSH/SCP failures, hardened repository/path/ID inputs, and replaced shared temp files. Maintainer follow-up 5db7c99 additionally bounds cleanup and archive download waits. Fresh CI, CodeQL, advisors, and E2E are now running; no original blocking item remains.
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Blocking on exact head 5db7c99: the secret-bearing e2e-branch-validation job still invokes mutable actions/setup-node@v6. This was part of the earlier trust-boundary request and remains exploitable as a tag-retarget supply-chain path to the Brev/inference secrets. Pin this invocation to the same immutable setup-node commit already used by the isolated cleanup and reporter jobs, and add a workflow contract assertion so target-data-plane actions cannot regress to mutable refs. The separate trusted cleanup job, reporter permissions/link behavior, and observable debug failures otherwise address the prior findings.
Pin every action in the secret-bearing validation job to an immutable commit and guard that trust boundary with a workflow contract test. Co-authored-by: Souvik Ghosh <138186578+souvikDevloper@users.noreply.github.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 341b5e5 after the requested trusted-workflow hardening. Every action in the secret-bearing validation job is now pinned to an immutable 40-hex commit and the workflow contract enforces that invariant. Focused lifecycle/workflow coverage passes locally (51/51), the fix commit is GitHub Verified with DCO/co-author credit, there are no unresolved threads, and all ordinary CI plus both advisors are green. Credentialed E2E coordination is still running and remains the final gate.
Co-authored-by: Souvik Ghosh <138186578+souvikDevloper@users.noreply.github.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Current-main refresh pushed as
Local validation passed:
Fresh exact-head CI and advisor checks are now running. |
|
@apurvvkumaria thanks for your review also i am quite interseted to join the nemoClaw team and become its legit collabrator, if there is a open position or something love to know also ready for interviews as per companys protocol . @cv nothing more on my side in these repo these are infra issues if u permit to skip those tests then all green and ready to merge. @apurvvkumaria love to work with you in person let me know you guys hiring process |
5 participants
Refs #6962 (parent epic #6952).
Problem
.github/workflows/e2e-branch-validation.yamlembedded four executable programs in YAML: pinned Brev CLI install/auth, failure-bundle collection, retrying instance deletion, and a 54-line write-capable PR/check reporter. None were directly testable, and — the part that matters for security — the cleanup and publication logic sat in the same file that a tested branch can influence.What this does
Moves the logic into
tools/e2e, leaving YAML responsible for triggers, secrets, instance identity,keep_alive, permissions, and always-run ordering.tools/e2e/brev-lifecycle-core.mts— pure decision/rendering: login selection (API-key/org vs legacy token vs fail-closed), pinned CLI version/checksum/URL,brev ls --jsonshape normalization (array vs{workspaces}vs unverifiable), deletion-presence classification, result→conclusion/status/emoji mapping, PR-comment rendering, and the PR-number / stale-SHA guards.tools/e2e/brev-lifecycle.mts—install-cli,collect-debug,delete-instance,report-prsubcommands driving that logic through injected runners (thinbrev/ssh/ghwrappers).The workflow now invokes the helper:
install-cli,collect-debugdelete-instancegithub.workflow_sha(checked out to.trusted-e2e)report-prgithub.workflow_sha(reporter job, no target checkout)Behavior preserved
CLI checksum verification, API-key/org login with legacy refresh-token fallback, onboarding suppression + readiness check, best-effort debug collection before deletion, retrying/idempotent deletion that validates
brev ls --json, detects already-absent instances, and fails visibly after the final attempt, and publication that rejects invalid PR numbers and stale tested SHAs before any write.Tests
test/e2e/support/brev-lifecycle.test.ts(28) covers install/auth branches,brev ls --jsonvariants, every deletion outcome (no-CLI, first-try, retry-success, confirmed-absent, unverifiable→retry-to-failure, still-present→fail), result mapping, comment rendering, and stale-head/invalid-PR rejection.test/brev-nightly-workflow.test.ts— cv'ssource-shape-contract: securityguards, updated to assert the extracted structure and the trusted-load boundary (deletion loaded from.trusted-e2e, reporter checks out onlygithub.workflow_shaand carries no secrets) instead of inline script text.typecheck:cliand biome are clean. I verified against a cleanmainbaseline that this introduces no new test failures (tools/**/*.mtsis covered bytsconfig.cli.json).Coordination
Per the issue, this coordinates with #6145 (auto-retrigger branch-validation E2E / advisors on fork PRs): this change is behavior-preserving and touches only the executable bodies, not triggers, job topology, or permissions, so it composes with the trigger changes #6145 makes rather than conflicting.
Acceptance criteria
brev ls --json, detects already-absent instances, and fails visibly after the final attempt.keep_aliveguidance and instance naming unchanged.Signed-off-by: Souvik Ghosh 138186578+souvikDevloper@users.noreply.github.com
Summary by CodeRabbit