test: add e2e test for repos lifecycle across GitHub and GitLab - #5656
test: add e2e test for repos lifecycle across GitHub and GitLab#5656ggallen wants to merge 1 commit into
Conversation
PR Summary by QodoAdd repos lifecycle e2e test spanning GitHub + GitLab
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
|
🤖 Finished Review · ✅ Success · Started 10:00 PM UTC · Completed 10:12 PM UTC |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
ReviewFindingsHigh
Low
Next steps:
Previous runReviewFindingsHigh
Low
Next steps:
Previous run (2)ReviewFindingsHigh
Low
Next steps:
Previous run (3)ReviewFindingsHigh
Low
Next steps:
Previous run (4)ReviewFindingsHigh
Low
Next steps:
Previous run (5)ReviewFindingsHigh
Low
Next steps:
Previous run (6)ReviewFindingsHigh
Low
Next steps:
Previous run (7)ReviewFindingsHigh
Low
Previous run (8)ReviewFindingsHigh
Low
Previous run (9)ReviewFindingsHigh
Low
Previous run (10)ReviewFindingsHigh
Low
Labels: PR adds Go e2e test code and helpers; go label matches the established convention for PRs modifying Go files. Previous run (11)ReviewFindingsHigh
Low
Previous run (12)ReviewFindingsHigh
Medium
Low
Labels: PR adds e2e test infrastructure for the repos CLI feature and modifies CI workflow |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
component/e2e
a07a65f to
b8a2d46
Compare
|
🤖 Review · ❌ Terminated · Started 10:24 PM UTC · Ended 10:38 PM UTC |
Site previewPreview: https://9c8cfc04-site.fullsend-ai.workers.dev Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Review · ✅ Success · Started 10:24 PM UTC · Completed 10:38 PM UTC |
b8a2d46 to
31cd0c6
Compare
|
🤖 Finished Review · ✅ Success · Started 10:46 PM UTC · Completed 11:01 PM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
31cd0c6 to
3a3dd6f
Compare
|
🤖 Finished Review · ✅ Success · Started 11:06 PM UTC · Completed 11:21 PM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review findings below (inline).
3a3dd6f to
2c90b34
Compare
|
🤖 Finished Review · ✅ Success · Started 1:52 AM UTC · Completed 2:07 AM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
be09b2a to
b144223
Compare
|
🤖 Finished Review · ✅ Success · Started 3:45 PM UTC · Completed 4:02 PM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
b144223 to
586f808
Compare
|
🤖 Finished Review · ✅ Success · Started 6:20 PM UTC · Completed 6:34 PM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Three additional findings from a review pass, all at the current head commit.
586f808 to
31e7827
Compare
|
🤖 Review · |
31e7827 to
6277a5d
Compare
|
🤖 Finished Review · ✅ Success · Started 7:55 PM UTC · Completed 8:12 PM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Add TestReposLifecycle in e2e/admin/ that exercises all fullsend repos CLI commands (init, add, status, install --dry-run, diff, remove, upgrade-mint) against ephemeral repos on both GitHub and GitLab forges. The test requires REPOS_E2E_GITHUB_TOKEN and GITLAB_TOKEN to run, making it fully opt-in. Stale ephemeral repos from crashed runs are cleaned up at test start using creation timestamp checks. Signed-off-by: Greg Allen <greg@fullsend.ai> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Greg Allen <gallen@redhat.com>
6277a5d to
5c38488
Compare
|
🤖 Finished Review · ✅ Success · Started 11:54 PM UTC · Completed 12:08 AM UTC |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Additional review findings (3 medium-severity), verified against the current head and cross-checked against existing review threads to avoid duplicates.
| return m | ||
| } | ||
|
|
||
| type statusJSON struct { |
There was a problem hiding this comment.
[MEDIUM] Hand-rolled status/diff JSON structs duplicate exported internal/repos types
statusJSON (here) and diffJSON (line 191) re-declare a strict subset of the fields already exported by internal/repos.RepoStatus/StatusResult and internal/repos.Change/DiffResult, in a file that already imports internal/repos for Manifest/MarshalWithHeader/LoadManifest. Checked against the current internal/repos/status.go and sync.go: RepoStatus has 11 fields (Owner, Repo, Installed, CurrentRef, ExpectedRef, MintURL, ExpectedMintURL, Region, ExpectedRegion, Drifts, Error) but statusJSON decodes only 5 (Owner, Repo, Installed, CurrentRef, Error); Change has 7 fields (Owner, Repo, Field, Type, Action, OldValue, NewValue) but diffJSON decodes only 3 (Owner, Repo, Field).
Because json.Unmarshal silently ignores JSON fields it doesn't recognize rather than failing to compile, a future rename/addition to the real CLI JSON schema won't cause a build break here — it will just silently stop being asserted on, weakening the regression-catching value this lifecycle test exists to provide.
Suggestion: unmarshal directly into repos.StatusResult and repos.DiffResult (already importable from the same package) instead of maintaining parallel, partially-overlapping struct definitions in the test helper.
| "github.com/fullsend-ai/fullsend/pkg/e2etest" | ||
| ) | ||
|
|
||
| func TestReposLifecycle(t *testing.T) { |
There was a problem hiding this comment.
[MEDIUM] New 13-phase sequential e2e test has no runtime budget against the shared 30-minute CI timeout
TestReposLifecycle runs 14 sequential fullsend CLI subprocess spawns (init x2, add x2, status x1, install x1, diff x1, remove x2, add x2, upgrade-mint x1, remove x1, status x1) plus 4 live forge repo creations/deletions and up to 30s-timeout visibility polling (waitForRepoVisible, repoVisibilityTimeout = 30s), all with no t.Parallel(). This lives in the same e2e/admin package as TestAdminInstallUninstall (admin_test.go:86), which itself calls AcquireOrg with a lock timeout defaulting to 10 minutes (E2E_LOCK_TIMEOUT, documented default 10m in e2e-testing.md). Both tests share one job's timeout-minutes: 30 (.github/workflows/e2e.yml:105). Nothing in this PR measures or reserves budget for the added wall-clock cost.
Suggestion: measure actual added runtime once a real CI run is available; if material, run the repos lifecycle test in its own job/step with an independent timeout rather than folding it into the existing 30-minute admin e2e budget, or mark phases safe for t.Parallel() where they don't share mutable state.
| | `FULLSEND_MINT_URL` | Override mint endpoint (default: hosted public mint, same as `fullsend admin --mint-url`) | | ||
| | `E2E_LOCK_TIMEOUT` | Max wait for a free pool org (default 10m) | | ||
| | `E2E_GCP_PROJECT_ID` | GCP project for inference setup (`github setup --inference-project`) | | ||
| | `REPOS_E2E_GITHUB_TOKEN` | GitHub PAT for repos lifecycle e2e tests; overrides `GH_TOKEN`/`GITHUB_TOKEN` for the repos e2e org (test falls back to general auth if unset) | |
There was a problem hiding this comment.
[MEDIUM] Docs claim a GitHub token auth fallback that the code does not implement
This row states that REPOS_E2E_GITHUB_TOKEN "overrides GH_TOKEN/GITHUB_TOKEN for the repos e2e org (test falls back to general auth if unset)". The current setupReposTest (e2e/admin/repos_helpers.go:79-82) has no fallback at all:
ghToken := os.Getenv("REPOS_E2E_GITHUB_TOKEN")
if ghToken == "" {
t.Skip("REPOS_E2E_GITHUB_TOKEN not set, skipping repos e2e test")
}This matches this PR's own review history — an earlier resolveGitHubToken fallback chain (GH_TOKEN -> GITHUB_TOKEN -> gh auth token) was deliberately removed in a later round specifically so the test is fully opt-in via the dedicated secret — but this doc line, added in the same PR, wasn't updated to match. The GITLAB_TOKEN row directly below (line 49) correctly says "test skips if unset"; only the GitHub row overclaims a fallback.
Suggestion: fix the doc line to match the code ("test skips if unset", same wording as the GITLAB_TOKEN row) — a fallback to a contributor's personal token wouldn't be useful anyway since it's unlikely to have admin/delete_repo rights on the dedicated fullsend-repos-e2e-gh org.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Automated review pass: 1 CRITICAL finding (PR no longer compiles against current main — targets a repos CLI/manifest surface removed by three subsequent migrations). See inline comment for details.
| func buildCombinedManifest(ghOrg, glGroup string, ghRepos, glRepos []string) *repos.Manifest { | ||
| m := &repos.Manifest{ | ||
| Version: 1, | ||
| Mint: repos.MintConfig{ |
There was a problem hiding this comment.
[CRITICAL] PR does not compile against current main — tests a repos CLI/manifest surface that no longer exists
This PR branched at merge-base eeb97439 (2026-07-27 or earlier) and was never rebased, while three schema/CLI migrations landed on main since then: 2477b780 (move mint config into per-forge section, removing top-level Manifest.Mint/MintConfig), 464747b6 (consolidate repos commands from 9 to 4), and b526e287 (add repos migrate, remove repos init).
Verified directly against origin/main (7999832c):
internal/repos/manifest.go—Manifestis now{Version, Forge, Defaults, Repos}with noMintfield, andMintConfigdoes not exist anywhere in the repo (git grep confirms);DefaultsConfigis now{Forge, AllowedRemoteResources}— none ofInferenceProject/InferenceRegion/FullsendRefexist on it.internal/cli/repos.goon main registers exactly 5 subcommands:migrate,install,uninstall,status,set-default—init,add,remove,diff, andupgrade-mint(all used by this PR's test, and named explicitly in its own PR description) do not exist.
This PR's buildCombinedManifest() (this file, lines 331-345) constructs exactly the now-removed repos.Manifest{Mint: repos.MintConfig{...}} / repos.DefaultsConfig{InferenceProject, InferenceRegion, FullsendRef} literal, and repos_test.go's TestReposLifecycle invokes exactly the five removed subcommands (repos init, add, remove, diff, upgrade-mint) via 14 sequential CLI subprocess calls. A git merge --no-ff of the PR branch into origin/main produces a real conflict (confirmed in docs/guides/dev/e2e-testing.md, internal/cli/lock.go, internal/cli/lock_test.go, internal/harnessdispatch/enumerate_test.go).
None of the review rounds on this PR so far — including the round completed at this exact head commit — raise this specific schema/CLI-surface break; existing findings are unrelated (duplicate JSON structs, runtime budget, docs wording). The PR's own test-plan checkboxes ("go build -tags e2e ./e2e/admin/ compiles cleanly", "TestReposLifecycle passes locally") were only true against the PR's stale base and are no longer accurate against main.
Suggestion: This needs more than a rebase — the new test file targets a CLI/manifest schema that has since been redesigned twice. Rewrite TestReposLifecycle and buildCombinedManifest/writeTestManifest against the current migrate / install / uninstall / status / set-default command set and the current Manifest{Version, Forge, Defaults, Repos} / per-forge mint_url schema before this can land. Do not merge as-is; re-verify go build -tags e2e ./e2e/admin/... against current main before requesting re-review.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Automated review sweep: 2 additional findings not previously flagged.
| t.Helper() | ||
| ctx, cancel := context.WithTimeout(context.Background(), repoVisibilityTimeout) | ||
| defer cancel() | ||
| for { |
There was a problem hiding this comment.
[MEDIUM] waitForRepoVisible retries on every GetRepo error, not just not-found
waitForRepoVisible polls client.GetRepo(ctx, owner, repo) in a loop and treats any non-nil error identically to "not yet visible", retrying every 2s for the full 30s repoVisibilityTimeout before t.Fatalf. Unlike the codebase's own retryOnNotFound helper (pkg/e2etest/testutil.go:536-554), which explicitly checks forge.IsNotFound(err) before retrying and returns immediately on other error classes, this new helper has no such check.
Suggestion: only retry when the error is a not-found condition (forge.IsNotFound(err)); propagate/fail fast on other error classes such as auth failures, rate limiting, or malformed org/group names, instead of masking them behind a generic 30s timeout message.
| if !strings.HasPrefix(r.Name, ephemeralRepoPrefix) { | ||
| continue | ||
| } | ||
| createdAt, ageErr := getGitLabProjectCreatedAt(ctx, glToken, glGroup, r.Name) |
There was a problem hiding this comment.
[MEDIUM] Stale GitLab cleanup uses display Name where a URL path/slug is required
ListOrgRepos for GitLab (internal/forge/gitlab/repo.go:107-123) populates forge.Repository.Name from the JSON name field (display name) and FullName from path_with_namespace (the actual URL slug). cleanupStaleEphemeralRepos iterates glRepos and passes r.Name directly into getGitLabProjectCreatedAt(ctx, glToken, glGroup, r.Name) and glClient.DeleteRepo(ctx, glGroup, r.Name) — both of which build the API path from that value as if it were the project's path/slug. createEphemeralGitLabRepo only ever sets the name field on creation (internal/forge/gitlab/repo.go:182-188), so GitLab currently auto-derives an identical path from the already slug-safe ephemeral repo names, masking the bug today. Any future change to naming (spaces, non-ASCII, or an explicit differing path) would silently break stale-repo age lookups and deletion targeting.
Suggestion: derive the path from the last segment of r.FullName (path_with_namespace) rather than relying on r.Name coinciding with the URL slug.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only sweep: 3 additional findings not covered by existing review comments (verified against origin/main and current PR diff).
| installOut := e2etest.RunCLIWithEnv(t, env.binary, env.cliEnv(), | ||
| "repos", "install", | ||
| "--dry-run", | ||
| "--skip-mint-check", |
There was a problem hiding this comment.
[CRITICAL] --skip-mint-check flag does not exist on repos install
Phase 6 of TestReposLifecycle invokes repos install --dry-run --skip-mint-check -f manifestPath. Verified directly against internal/cli/repos.go on origin/main: newReposInstallCmd()'s flag set is exactly --manifest/-f, --dry-run, --concurrency, --roles, --direct, --force, --forge, --inference-project, --inference-project-number, --inference-region, --fullsend-ref, --mint-url, --allowed-remote-resources, --gitlab-bot-token. --skip-mint-check only exists on the unrelated admin install command (internal/cli/admin.go:603), not on repos install. Unlike the already-flagged CRITICAL issue on this PR covering the removed init/add/remove/diff/upgrade-mint subcommands and Manifest/MintConfig schema drift, repos install itself does exist on main today — but this specific flag on it does not, so Phase 6 will fail immediately with an "unknown flag" cobra parse error even after the schema/subcommand issues elsewhere in the test are fixed.
Suggestion: Drop --skip-mint-check from the repos install invocation, or determine what current flag (if any) suppresses mint verification during a dry run and use that instead.
| env: | ||
| E2E_SCREENSHOT_DIR: ${{ runner.temp }}/e2e-screenshots | ||
| E2E_GCP_PROJECT_ID: ${{ secrets.E2E_GCP_PROJECT_ID }} | ||
| REPOS_E2E_GITHUB_TOKEN: ${{ secrets.REPOS_E2E_GITHUB_TOKEN }} |
There was a problem hiding this comment.
[MEDIUM] New GitHub secret uses a static long-lived credential pattern inconsistent with sibling jobs' short-lived tokens
This PR wires REPOS_E2E_GITHUB_TOKEN: ${{ secrets.REPOS_E2E_GITHUB_TOKEN }} into the e2e job's "Run e2e tests" step, which runs under pull_request_target with an allow-unsafe-pr-checkout PR-head checkout (gated by a separate gate job's authorization check). Elsewhere in this same workflow file, equivalent-power credentials are minted short-lived: GCP access uses google-github-actions/auth with E2E_GCP_WIF_PROVIDER/E2E_GCP_SERVICE_ACCOUNT (Workload Identity Federation), and the behaviour job mints GitHub App installation tokens from TEST_FULLSEND_PEM/TEST_TRIAGE_PEM/etc. rather than static PATs. REPOS_E2E_GITHUB_TOKEN (needed to create/delete repos in the external fullsend-repos-e2e-gh org) breaks that pattern by being a plain, presumably long-lived static secret. The gate job's authorization check mitigates but does not eliminate exposure risk for this new credential type.
Suggestion: If feasible, mint a short-lived GitHub token for the fullsend-repos-e2e-gh org via the same OIDC/App-installation-token pattern used for GCP and the behaviour job's GitHub Apps, instead of a static classic PAT; if that's not practical, at minimum scope the PAT as narrowly as possible (repo-only, no org-admin) and document its blast radius alongside the other secrets in docs/guides/dev/e2e-testing.md.
|
|
||
| binary := e2etest.BuildCLIBinary(t) | ||
|
|
||
| ghToken := os.Getenv("REPOS_E2E_GITHUB_TOKEN") |
There was a problem hiding this comment.
[MEDIUM] Test silently skips (rather than failing) when its required secrets are unset, masking lack of coverage in CI
setupReposTest does if ghToken == "" { t.Skip(...) } and the same for GITLAB_TOKEN. If REPOS_E2E_GITHUB_TOKEN/GITLAB_TOKEN are not yet provisioned as repo secrets (a real possibility immediately after this PR merges, before someone sets them up) or are otherwise unavailable, make e2e-test reports green with zero coverage of the internal/repos/** and internal/cli/repos.go code paths this test was specifically added to protect — even though the CI path filter added by this same PR explicitly triggers the e2e job on changes to those paths. This is distinct from the already-fixed issue about the CI step not wiring a GitHub token at all — this finding is about the ongoing risk of the test degrading to a silent no-op whenever either secret is transiently absent.
Suggestion: In CI (e.g. when CI=true), fail loudly (t.Fatal) instead of skipping when the required secrets are absent, or route this test through a separate required status check so a missing secret is visibly surfaced rather than silently passing.
3 participants
Summary
TestReposLifecyclee2e test exercising allreposCLI commands (init, add, remove, status, install dry-run, diff, upgrade-mint) against real GitHub and GitLab APIs with ephemeral reposRunCLIWithEnv/TryRunCLIWithEnvhelpers for multi-token subprocess executionGITLAB_TOKENinto CI workflow and addinternal/repos/**to e2e path filtersREPOS_E2E_GITLAB_TOKENsecret is provisioned (already done)Test plan
go build -tags e2e ./e2e/admin/compiles cleanlygo vet -tags e2e ./e2e/admin/passesTestReposLifecyclepasses locally with bothGITHUB_TOKENandGITLAB_TOKENsetGITLAB_TOKENis not setREPOS_E2E_GITLAB_TOKENsecret is active)🤖 Generated with Claude Code