feat(#2096): add two-pass review strategy for large PRs#57
Conversation
Add a new subsection under "CI pipeline for agent configurations" elaborating on Step 1 (static analysis). Covers component-level checks (structural integrity, security patterns, token budget), setup-level analysis (redundancy detection, dependency validation, token budget distribution, trigger overlap, dimension scoring), and optional LLM-based rubric scoring. Presents similarity techniques as options (TF-IDF, embeddings, LLM-based) rather than prescribing a single approach. Adds three open questions on thresholds, lint rule universality, and token budgets. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Benjamin Kapner <bkapner@redhat.com>
Introduce --vendor to install vendored binaries, reusable workflows, actions, and agent content. Vendored upstream mirror content is committed under .defaults/ (same layout as runtime sparse checkout); layered installs fetch fullsend-ai/fullsend@v0 into .defaults when the marker file is absent. Reusable workflows use inline workspace preparation and reference infra from ./.defaults/, matching the pre-vendor layered design. Thin callers render local reusable paths when --vendor is set. --fullsend-source pins the source tree for both content and binary cross-compile; --fullsend-binary remains an explicit ELF override. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Write vendor-manifest.yaml on --vendor installs so cleanup and analyze work without a local fullsend checkout. Workflows analyze stays embed-only; vendor layer reports presence, manifest alignment, and optional source alignment via admin analyze --fullsend-source. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Consolidate thin-stage caller registry, reuse resolved source root for binary vendoring, reject oversized tar members during extraction, restore workflows scope comment, fix testing-workflows prose, and introduce InstallFiles as the canonical collector return type. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Re-add the full download_test.go suite and append extractSourceTree size limit coverage. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Delete vendored paths atomically via forge.DeleteFiles, reuse resolved source root for cross-compile, preserve extracted file modes, and tighten WouldFix deduplication to exact path matches. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Document intentional breaking change: old flag callers should use --vendor; only known usage was e2e, already updated in this branch. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Document VendorBinaryLayer legacy naming, restore Uninstall/Analyze comments, and use Title Case for stale-cleanup progress messages. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Batch binary, content, and manifest in one CommitFiles call; validate manifest version on read; trim leading slash in extractSourceTree; wrap DeleteFiles ref PATCH in retryOnTransient. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Use the existing blob mode from the recursive tree and set type blob so deletion entries match GitHub Trees API expectations. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Guard against regressions in delete-entry construction per review. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # internal/forge/fake.go # internal/forge/forge.go Signed-off-by: Barak Korren <bkorren@redhat.com>
Encode CommitFiles tree entries as base64 to preserve ELF binaries, add tar extract containment check, consolidate stale cleanup with a manifest/binary quick-check, and deduplicate cleanup between CLI and layer. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # action.yml # docs/guides/dev/testing-workflows.md Signed-off-by: Barak Korren <bkorren@redhat.com>
Clarify removed distribution-mode artifacts, drop e2e vendor line, and document action.yml source-build fallback. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Empty commit to re-dispatch review; prior synchronize dispatch was cancelled. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Keep enumerateVendoredPaths aligned with CollectVendoredAssets after main added the composite action (fullsend-ai#2106); fixes CI parity test. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…t dispatch GitHub Actions may return 422 when repo-maintenance is dispatched immediately after a separate vendor CommitFiles on a fresh .fullsend repo. Merge scaffold and vendored assets into one atomic commit and retry dispatch on indexing lag. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…nance Poll GitHub until repo-maintenance.yml is active before dispatch, re-touch config.yaml after scaffold so the push trigger can run enrollment when dispatch is still rejected, and fall back to awaiting a push-triggered run. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…nary Tree entries with encoding:base64 stored base64 text literally on GitHub, corrupting YAML workflows and vendor-manifest.yaml. Restore UTF-8 inline content for text and upload binary via the Git Blob API instead. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Design for a new `prerequisites` triage action that replaces `blocked`. The agent can now express both existing blockers and new issues that need to be created upstream before progress can happen. Includes allowlist configuration for cross-repo issue creation and a degraded path when targets are not authorized. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
…nd-ai#401) Seven-task plan covering config structs, JSON schema, agent prompt, post-script, user docs, and caller updates. TDD approach with exact file paths and code blocks. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Add CreateIssuesConfig and AllowTargets types to both OrgConfig and PerRepoConfig. NewOrgConfig populates defaults with the org and fullsend-ai/fullsend. NewPerRepoConfig populates with the target repo and fullsend-ai/fullsend. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
…ues (fullsend-ai#401) Pass org name and target repo to config constructors so create_issues defaults are populated at install time. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
…pt (fullsend-ai#401) The triage agent can now recommend creating upstream issues via the prerequisites action's create array, in addition to referencing existing blockers. Adds hard constraint against emitting sufficient when prerequisites exist. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
…d-ai#401) Update triage agent docs to explain the new prerequisites action and the create_issues.allow_targets configuration surface. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
…#401) Replace the blocked handler with prerequisites. The post-script reads the create_issues allowlist from config.yaml, creates permitted upstream issues via gh, and includes collapsed draft bodies for disallowed or failed creates so humans can file them manually. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
…ullsend-ai#401) The agent prompt referenced a nonexistent `prerequisites` label when checking for prior blockers — the post-script actually applies the `blocked` label. Also removed unused SOURCE_ORG variable from post-triage.sh. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
…script-diagnostic-errors fix(fullsend-ai#2393): add diagnostic stderr output to post-script failure paths
…-await-and-enqueue feat(merge-queue): add await-and-enqueue script
Two call sites in commitFilesTo were missed during the rename, causing build failures. Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
…pdate
The mergeEnrollmentPR function in the e2e test calls
MergeChangeProposal once without handling GitHub's 409 "Head branch
is out of date" response. When the reconcile workflow pushes to the
default branch between PR creation and the merge attempt, the
enrollment PR's base falls behind and the merge is rejected.
Add an UpdatePullRequestBranch method to the forge.Client interface
(wrapping GitHub's PUT /repos/{owner}/{repo}/pulls/{number}/update-branch)
and implement it in the GitHub LiveClient and FakeClient. In
mergeEnrollmentPR, wrap the merge call in a retry loop (up to 3
attempts) that detects 409 errors via the APIError status code,
calls UpdatePullRequestBranch to bring the PR branch up to date,
waits 5 seconds for GitHub to process, and retries the merge.
Note: pre-commit could not run in sandbox (shellcheck install
failed due to network restrictions). The post-script runs it
authoritatively.
Closes fullsend-ai#2432
fix(forge): retry 5xx server errors at the HTTP client level
docs(problems): add static analysis layer to testing-agents
…-merge-409 fix(fullsend-ai#2432): retry enrollment PR merge on 409 with branch update
For PRs with 30+ files, the review orchestrator now runs a lightweight security-triage pre-pass before dispatching dimension sub-agents. The triage pass uses a haiku-model sub-agent to classify changed files as security-critical or standard based on path patterns (e.g., **/mint/**, **/auth/**, **/oidc/**) and diff content heuristics (auth logic, token handling, permission changes). Security-critical files identified by the triage pass receive prioritized context in the security and correctness sub-agent context packages — their full diffs appear first with explicit classification headers, ensuring they get dedicated reasoning budget rather than competing with boilerplate changes. Changes: - New sub-agent definition: sub-agents/security-triage.md (haiku model, read-only classifier) - New orchestrator step 3c-1 in SKILL.md: security-critical file triage, runs synchronously before context package assembly - Updated step 3d in SKILL.md: security-prioritized context package assembly for security and correctness sub-agents when triage results are available - Updated sub-agent roster table with security-triage entry The 30-file threshold is a starting point that may need tuning. Triage failures fall back to uniform attention (all files treated as security-critical) to preserve existing behavior as a safe default. Closes fullsend-ai#2096
- Raise security triage threshold from 30 to 50 files to align with step 2's per-file diff boundary, resolving ambiguity in the 30-49 file range where per-file diffs were not available [edge-case] - Add clarifying note to roster table documenting that security-triage and challenger use non-standard dispatch and are excluded from step 4's parallel loop [logic-error, design-direction] - Clarify step 4 heading to explicitly scope dispatch to dimension sub-agents only [logic-error] - Remove parenthetical from security-triage sub-agent title to match naming convention of other sub-agents [naming-convention] Addresses review feedback on fullsend-ai#2303
|
/fs-qf |
|
🤖 Finished Review · ✅ Success · Started 8:00 AM UTC · Completed 8:21 AM UTC |
ReviewFindingsHigh
Medium
Low
Info
Previous runReviewReason: stale-head The review agent reviewed commit Previous run (2)ReviewReason: stale-head The review agent reviewed commit |
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 8:24 AM UTC · Completed 9:05 AM UTC |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
QualityFlow Pipeline Summary
Test Output
Issue: GH-57 Generated by QualityFlow |
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 9:08 AM UTC · Completed 9:50 AM UTC |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| default: "" | ||
| status-token: | ||
| description: Token for status comments (defaults to GH_TOKEN env var). | ||
| mint-url: |
There was a problem hiding this comment.
[high] github-action-input-breaking-change
The status-token input has been renamed to mint-url, breaking all downstream workflow calls that pass status-token. The failure mode is silent degradation of status comment functionality.
Suggested fix: Keep both inputs with deprecation mapping (read mint-url with fallback to status-token), or document as a breaking change with a major version bump.
| @@ -24,19 +25,6 @@ on: | |||
| - 'scripts/check-e2e-authorization.sh' | |||
| pull_request_target: | |||
There was a problem hiding this comment.
[high] workflow-trigger-broadening
Removed paths: filter from pull_request_target trigger. Every PR now triggers the e2e workflow. While the gate job enforces authorization, removing the path filter widens the trigger surface for pull_request_target which runs with base-repo privileges and secret access.
Suggested fix: Keep paths: filter as defense-in-depth, or document the intentional trade-off.
| source "${SCRIPT_DIR}/openshell-version.sh" | ||
|
|
||
| echo "Installing OpenShell ${OPENSHELL_VERSION} (${OPENSHELL_SHA})" | ||
| curl -LsSf "https://raw.githubusercontent.com/NVIDIA/OpenShell/${OPENSHELL_SHA}/install.sh" \ |
There was a problem hiding this comment.
[medium] curl-pipe-to-shell
New script downloads and executes code from GitHub via curl | sh, pinned to a commit SHA but without checksum verification of the downloaded content.
Suggested fix: Add SHA-256 checksum verification of the downloaded install.sh before piping to sh.
| @@ -719,17 +753,8 @@ func (p *Provisioner) provisionSelfManaged(ctx context.Context) (map[string]stri | |||
| } | |||
There was a problem hiding this comment.
[medium] logic-error
Legacy org/role keys in ROLE_APP_IDS are never cleaned up during code deploys. Dead data accumulates indefinitely.
Suggested fix: Add a cleanup step during code deploy that strips legacy org/role keys.
| @@ -760,7 +759,7 @@ func runPerRepoInstall(ctx context.Context, c perRepoInstallConfig) error { | |||
| agentAppIDs = make(map[string]string, len(roles)) | |||
There was a problem hiding this comment.
[medium] logic-error
When resolveSharedRoleAppIDs finds no role-only keys but legacy org/role keys exist, the error message says install the app first which is misleading.
Suggested fix: Include guidance about running migration to convert org/role keys to role-only keys.
| The mint URL is stable across redeploys within the same project and region — updating the Cloud Function does not change its URL. Adding a new org to an existing mint only updates `ALLOWED_ORGS` (and WIF configuration) without redeploying the function. Shared `ROLE_APP_IDS` are managed at deploy/bootstrap time (`mint deploy --pem-dir`) or per-role via `mint add-role` / `remove-role` — not during enrollment. Existing enrolled repos continue working with no changes when orgs are added. | ||
|
|
||
| Deploying to a **different region** (e.g., changing `--region` from `us-central1` to `us-east5`) creates a new Cloud Run service with a different URL. All enrolled repos store the mint URL in a repo or org variable (`FULLSEND_MINT_URL`), so changing the region requires updating every enrolled repo's variable. Avoid changing `--region` after initial deployment unless you plan to update all consumers. | ||
|
|
There was a problem hiding this comment.
[medium] stale-doc
Enrollment flags table includes removed flags (--app-set, --role-app-ids, --roles).
Suggested fix: Remove stale flags and update enrollment documentation.
9 participants
Mirror of upstream fullsend-ai#2303
For PRs with 30+ files, the review orchestrator now runs a lightweight security-triage pre-pass before dispatching dimension sub-agents. Security-critical files get prioritized context in sub-agent packages.