feat(e2e): expand org pool from 6 to 12#2766
Conversation
PR Summary by QodoExpand e2e org pool and fix URL skill dir naming in resolver
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Site previewPreview: https://85cbba5b-site.fullsend-ai.workers.dev Commit: |
|
🤖 Finished Review · ✅ Success · Started 8:12 PM UTC · Completed 8:32 PM UTC |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Code Review by Qodo
1. Skill symlink path traversal
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| skillName := filepath.Base(forgeInfo.Path) | ||
| if skillName == "" || skillName == "." { | ||
| skillName = "tree" | ||
| } | ||
| namedPath := filepath.Join(filepath.Dir(treePath), skillName) | ||
| if namedPath != treePath { | ||
| // Idempotent: only create if it doesn't already exist. | ||
| if _, err := os.Lstat(namedPath); os.IsNotExist(err) { | ||
| if err := os.Symlink("tree", namedPath); err != nil { | ||
| return Dependency{}, "", fmt.Errorf("creating named symlink for %s: %w", field, err) | ||
| } | ||
| } | ||
| treePath = namedPath | ||
| } |
There was a problem hiding this comment.
2. Skill symlink path traversal 🐞 Bug ⛨ Security
resolveSkillDirURL builds a “named” cache path from filepath.Base(forgeInfo.Path) but does not reject ".."; a URL ending in "/.." will make namedPath resolve to the cache parent and treePath will be redirected outside the intended cached tree/ directory. Downstream consumers will then read/upload the wrong directory.
Agent Prompt
## Issue description
`resolveSkillDirURL` derives `skillName := filepath.Base(forgeInfo.Path)` and then sets `treePath = filepath.Join(filepath.Dir(treePath), skillName)`. If `skillName` is `".."` (possible because forge URL paths are not normalized), `filepath.Join` will resolve outside the cache entry directory and the resolver returns a path that is not the cached skill tree.
## Issue Context
- Forge URLs are parsed into `forgeInfo.Path` by joining URL path segments; `..` segments are preserved.
- The cache layout stores the skill directory under `<cacheEntry>/tree/`, so returning a parent/sibling path is incorrect and can expand what gets read/uploaded.
## Fix Focus Areas
- internal/resolve/resolve.go[372-388]
### Suggested fix approach
- Treat URL paths as URL paths: compute the directory name safely (e.g., use `path.Base` on a slash-separated path, or explicitly split on `/`).
- Validate the derived name is a safe single directory name:
- Reject/replace `""`, `"."`, and **`".."`**.
- Optionally, restrict to a conservative charset (e.g., `[A-Za-z0-9._-]`) and fall back to `"tree"` if it doesn’t match.
- After computing `namedPath`, ensure it remains within `filepath.Dir(treePath)` (e.g., clean it and verify it has the expected prefix) before assigning to `treePath`.
- If validation fails, skip symlink creation and keep `treePath` as the original `.../tree` path.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
ReviewFindingsHigh
Labels: PR modifies e2e test org pool infrastructure in e2e/admin/testutil.go. Previous runReviewFindingsMedium
Low
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| fetchedAt = dirEntry.FetchTime | ||
| } | ||
|
|
||
| // Create a symlink named after the skill directory so downstream consumers |
There was a problem hiding this comment.
[medium] scope-mismatch-unauthorized-work
PR contains two unrelated changes without linking to any authorizing issue. The PR title describes only the e2e test infrastructure change, but the diff also includes a production code change adding named symlink creation for skill directories with corresponding test updates.
Suggested fix: Split this PR into two separate PRs or update the PR title and description to cover both changes.
| fetchedAt = dirEntry.FetchTime | ||
| } | ||
|
|
||
| // Create a symlink named after the skill directory so downstream consumers |
There was a problem hiding this comment.
[low] path traversal
The skillName guard checks for empty string and '.' but does not check for '..'. If forgeInfo.Path ends with a '..' segment, filepath.Base would return '..', causing namedPath to escape the cache entry directory. Exploitation is heavily constrained but the fix is trivial defense-in-depth.
Suggested fix: Add '..' to the guard condition: if skillName == "" || skillName == "." || skillName == ".." { skillName = "tree" }
| // directory name from the URL ("review"), not the cache-internal "tree". | ||
| info, err := os.Stat(h.Skills[0]) | ||
| require.NoError(t, err) | ||
| assert.True(t, info.IsDir()) |
There was a problem hiding this comment.
[low] test-coverage-gap
The test updates verify happy-path skill path basenames but do not test symlink creation edge cases: existing symlink with different target, regular file at symlink path, os.Symlink failure, or skillName extraction producing empty string or '.'.
Suggested fix: Add test cases for idempotent symlink creation and edge cases in filepath.Base extraction.
With 6 orgs, concurrent e2e runs from multiple PRs starve each other for test orgs, causing cancellations after the 10-minute lock timeout. Double the pool to 12 orgs to support higher parallelism. New orgs (halfsend-07 through halfsend-12) must be provisioned before merging — run hack/setup-new-e2e-org.sh for each. Signed-off-by: Robin Bender Ginn <rbenderg@redhat.com> Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
ralphbean
force-pushed
the
expand-e2e-org-pool
branch
from
8189412 to
a6c4bd4
Compare
|
🤖 Finished Review · ✅ Success · Started 11:56 AM UTC · Completed 12:05 PM UTC |
|
🤖 Finished Retro · ✅ Success · Started 12:16 PM UTC · Completed 12:24 PM UTC |
Retro: PR #2766 —
|
| Friction | Existing issue |
|---|---|
| Second review dispatched on a SHA already approved by a human, creating noise | #963 |
CHANGES_REQUESTED for a single style finding on a human-authored PR |
#2115 |
| Review bot overriding human approval | #1922 |
| "Previous run findings" section displayed findings about code no longer in the diff | #2116, #1155 |
| Commit prefix classified as High severity, bypassing verdict dampening for human PRs | #2105 |
Conclusion
No new proposals. All identified improvement opportunities are tracked by existing open issues. The highest-impact fix for this class of friction would be implementing #963 (skip review dispatch when the HEAD SHA already has a human approval) — this single change would have prevented the second review run entirely, avoiding the noisy CHANGES_REQUESTED override and saving token cost.
2 participants
Summary
halfsend-07throughhalfsend-12to the e2e org pool, doubling capacity from 6 to 12 concurrent runsPrerequisites before merging
Each new org must be provisioned using the setup script:
This creates the org, adds
botsendas owner, createstest-repo, installs the GitHub Apps, and checks mint enrollment.Test plan
hack/setup-new-e2e-org.sh🤖 Generated with Claude Code