fix(test-suite): freeze orchestrated fhevm baseline#2278
fix(test-suite): freeze orchestrated fhevm baseline#2278
Conversation
c09bfac to
d7cf1f2
Compare
54df93f to
249ed2d
Compare
|
@claude /pr-review |
There was a problem hiding this comment.
[AUTOMATED]
Code Review
Summary
This PR freezes orchestrated fhevm e2e baselines by resolving from the PR base SHA instead of floating latest-main. Key changes:
- New
resolve-baselinejob in the orchestration workflow that resolves and uploads a lock artifact from the PR base SHA - New
git.tsmodule replacing GitHub API calls with local git history for sha resolution --refparameter support enabling branch-aware baseline resolution (including release branches)- Differentiated build-result handling for relayer/test-suite (empty on skip vs base-tag on skip) to let the frozen lock provide baseline versions
- Guard steps ensuring orchestrated non-manual runs require a frozen lock artifact
Review Findings
Bugs & Security: No issues found. Reviewed for null/undefined dereferences, logic errors, command injection, and path traversal. The SAFE_REF regex in git.ts and requireRef validation provide adequate input sanitization for git commands.
Guideline Compliance: No violations. The code follows established project patterns — error types (GitHubApiError, PreflightError), CLI argument parsing via parseUpInput, and test structure are all consistent with the existing codebase.
Error Handling: Adequate. All error paths throw with descriptive messages. The catch → re-throw as GitHubApiError pattern matches existing code in the same files.
Nit (non-blocking)
Unused ref parameter in applyReleaseBaselineDefaults:
export const applyReleaseBaselineDefaults = (
bundle: VersionBundle, defaults: Record<string, string>, ref: string
): VersionBundle => {The ref parameter is accepted but never used in the function body. Consider prefixing with _ or removing it if it's not needed for future use.
Notes
- The removal of the
missingRepoPackagescheck from theshatarget path is intentional — the frozen lock artifact now ensures image availability, andassertCommitOnRef+assertSupportedShaBundleprovide the necessary validation. - The
floor >= 0guard inassertSupportedShaBundlecorrectly handles release branches where anchor commits (SIMPLE_ACL_MIN_SHA, SHA_RUNTIME_COMPAT_MIN_SHA) may not exist in the branch history — skipping the check rather than incorrectly failing. - Good test coverage: new tests for
--refvalidation, release baseline defaults, stale sha lock rejection, and branch history without main cutover anchors.
No blocking issues found. ✅
|
The review has been posted successfully. Here's a summary of what was done: PR Review CompletePR #2278: Review Process
ResultNo blocking issues found. The PR is well-structured with:
One non-blocking nit was noted: an unused |
Summary
Freeze orchestrated
fhevme2e baselines from the PR base SHA instead of resolving floatinglatest-mainduring the reusable workflow boot path.Closes zama-ai/fhevm-internal#1263.
Why
Merge-queue / orchestrated e2e was still resolving
latest-mainlive when no lock artifact was provided. That made the boot path depend on live GitHub and GHCR state and caused flaky failures from:latest-mainpoints at a commit whose full orchestrated stack is not available yetThe goal of this PR is narrow:
github.event.pull_request.base.shaChange
resolve-baselinejob to.github/workflows/test-suite-orchestrate-e2e-tests.yml./fhevm-cli resolve --target sha --sha "$BASE_SHA" --resetlock-artifact-nameinto the reusable e2e workflowlatest-mainworkflow_dispatchorchestrated runs able to resolvelatest-mainmanually when no artifact is suppliedsharesolution so it constructs the repo-owned image tags from the requested SHA without branch/ref/release logicTesting
cd test-suite/fhevm && bun test src/resolve.test.ts src/cli.test.tscd test-suite/fhevm && bun run checkruby -e 'require "yaml"; YAML.load_file(ARGV.fetch(0)); puts "ok"' .github/workflows/test-suite-orchestrate-e2e-tests.ymlruby -e 'require "yaml"; YAML.load_file(ARGV.fetch(0)); puts "ok"' .github/workflows/test-suite-e2e-tests.ymlcd test-suite/fhevm && bun run src/cli.ts resolve --target sha --sha $(git rev-parse --short=7 origin/main) --reset