Skip to content

perf(#1924): pass source file contents to review sub-agents - #1926

Closed
fullsend-ai-coder[bot] wants to merge 3 commits into
mainfrom
agent/1924-sub-agent-source-files
Closed

perf(#1924): pass source file contents to review sub-agents#1926
fullsend-ai-coder[bot] wants to merge 3 commits into
mainfrom
agent/1924-sub-agent-source-files

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor

The review orchestrator already fetches full source files of changed paths but did not include them in sub-agent context packages. Sub-agents independently re-read the same files from disk, causing redundant token usage (5-6 reads of the same file across 4 agents) and false positives from reading base-branch code instead of PR head.

Changes:

  • SKILL.md step 2: add orchestrator logic to fetch source file
    contents at PR head SHA via the GitHub contents API
  • SKILL.md section 3d: add source_files field to context packages
  • SKILL.md step 4 Part 4: add "Source files (PR head)" section to
    the sub-agent prompt template with inline instructions
  • meta-prompt.md: replace "read full source files" constraint with
    instruction to use provided source files and avoid redundant reads
  • Add size guard: for large PRs (>20 files or >5000 lines), include
    only dimension-relevant files; sub-agents fall back to disk reads

Closes #1924

Post-script verification

  • Branch is not main/master (agent/1924-sub-agent-source-files)
  • Secret scan passed (gitleaks — 4939eb52904be897ccf76b36a5f18f1130f51cb7..HEAD)
  • Pre-commit hooks passed (authoritative run on runner)
  • Tests ran inside sandbox

@github-actions

github-actions Bot commented Jun 5, 2026

Copy link
Copy Markdown

Site preview

Preview: https://7b3f3da0-site.fullsend-ai.workers.dev

Commit: 19603cbea7524a8daddb5f1b3e8586cb8ab9218a

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 5, 2026

Copy link
Copy Markdown

Review

Findings

Low

  • [logic-error] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md:155 — The bash snippet in step 2b outputs decoded file contents without filename headers or code fences, while steps 3d and 4 expect each file preceded by a #### <relative-path> header and wrapped in a fenced code block. Since this is instructional pseudocode consumed by an LLM orchestrator (not a directly-executed pipeline), the format mismatch is a clarity issue rather than a runtime bug — the orchestrator reads both the fetch script and the formatting instructions in steps 3d/4. Consider adding a note in the snippet that the orchestrator must format the output per steps 3d/4.

  • [edge-case] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md:152 — The binary-file extension filter includes .pb.go, which are protobuf-generated Go source files (plain text, not binary). The surrounding prose describes the filter as excluding "binary files (images, compiled artifacts)", but .pb.go files are human-readable Go code. While excluding generated files is reasonable, conflating them with binary exclusion is inaccurate.

  • [pattern-inconsistency] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md:350 — File boundary heading level inconsistency between diff and source_files context package fields. The diff field uses ### File: <relative-path> (h3) for per-file boundaries, while source_files uses #### <relative-path> (h4). The #### pattern is hierarchically correct; the existing ### File: pattern creates sibling sections that break heading hierarchy.

Info


Labels: PR modifies scaffolded review orchestrator skill definitions (SKILL.md, meta-prompt.md, challenger sub-agent)

Previous run

Review

Findings

Medium

  • [edge-case] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md:145 — The bash loop for FILE in $(echo "$PR_FILES" | jq -r '.[].filename') iterates all changed filenames without filtering by file status. Files with status removed do not exist at the PR head SHA, so the GitHub contents API will return 404 for them. The script has no error handling for this case.
    Remediation: Filter the file list to exclude removed files: jq -r '.[] | select(.status != "removed") | .filename'

  • [incomplete-implementation] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md:145 — The GitHub REST API contents endpoint returns base64-encoded content only for files under 1 MB. Files at or above 1 MB return a 403 error. The bash snippet will fail for large files without any fallback. Additionally, binary files (images, compiled artifacts) will be base64-decoded and included as noise, wasting tokens for sub-agents.
    Remediation: Add filtering to skip binary files (by extension or MIME type) and either add a fallback to the git blobs API for large files, or document the 1 MB limitation and skip oversized files with a warning.

  • [stale-constraint] internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/challenger.md:74 — The challenger sub-agent's Constraints section contains Read full source files, not just the diff hunks, which is the old wording from meta-prompt.md. The PR updates meta-prompt.md to instruct sub-agents to use the provided source files instead of reading from disk, but does not update challenger.md's own Constraints section. When the orchestrator composes the challenger prompt (SKILL.md step 6d), both the challenger's own constraints and meta-prompt's updated constraints will be present, creating contradictory instructions.
    Remediation: Update line 74 in challenger.md to align with the new meta-prompt.md constraint, or remove it since meta-prompt.md already provides the constraint.

Low

  • [redundant-api-call] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md:143 — The new code extracts HEAD_SHA from PR_META, but HEAD_SHA was already extracted from PR_DATA in step 1. Both PR_DATA and PR_META are fetched from the same API endpoint, making this a redundant variable reassignment.
    Remediation: Reuse the HEAD_SHA variable from step 1 instead of re-extracting it from PR_META.

  • [doc-style] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md:138 — New section #### Fetch source file contents (PR head) uses #### heading level under ### 2. Fetch PR context, while peer subsection ### 2a. Prior review context uses ### level. This creates heading hierarchy inconsistency within section 2.
    Remediation: Change to ### 2b. Fetch source file contents (PR head) to match the existing ### 2a pattern.

  • [incomplete-implementation] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md:151 — The size guard for large PRs specifies ">20 changed files or 5000 total changed lines" as the threshold but does not provide concrete guidance on how to determine "most relevant files" for each sub-agent dimension.

Info

  • [missing-error-handling] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md:145 — The bash snippet for fetching file contents does not include error handling for API failures (rate limits, network errors). A failed gh api call will result in empty or malformed content being passed to sub-agents.

  • [validation-gap] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — Issue Review sub-agents should receive source file contents in prompt instead of re-reading from disk #1924 specifies validation criteria including "≥20% reduction in cache-creation tokens", but the diff does not add any mechanism to measure or log token usage.


Labels: PR modifies agent harness skill definitions for the review orchestrator.

Previous run (2)

Review

Findings

Medium

  • [edge-case] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md:145 — The bash loop in "Fetch source file contents (PR head)" iterates all filenames from PR_FILES without filtering by file status. PR_FILES includes deleted files (status: "removed"). Calling gh api repos/.../contents/<file>?ref=HEAD_SHA for a file deleted in the PR head commit will return a 404, causing errors in the fetch loop. The script has no error handling for this case.
    Remediation: Filter PR_FILES to exclude deleted files before the loop: for FILE in $(echo "$PR_FILES" | jq -r '.[] | select(.status != "removed") | .filename'); do. Alternatively, add explicit 404 handling to the gh api call.

Low

  • [edge-case] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md:143 — The GitHub contents API returns a 403 for files exceeding 1 MB. The fetch loop does not handle this case, so large binary or generated files in a small PR would cause silent failures. The size guard addresses large PRs by file/line count, but individual large files within a small PR would still hit this limit.
    Remediation: Add a note that the contents API has a 1 MB limit and that files exceeding it should be skipped or fetched via the git blobs API with the raw media type.

  • [doc-style] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md:135 — New section #### Fetch source file contents (PR head) uses #### heading level under ### 2. Fetch PR context, while peer subsections like ### 2a. Prior review context use ### level. This breaks the established heading hierarchy where numbered step subsections use the same level as the parent.
    Remediation: Change heading to ### 2b. Fetch source file contents (PR head) to match the existing pattern, or restructure as a continuation of step 2.

Info

  • [logic-error] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md:151 — The size guard threshold (>20 files or >5000 lines) differs from step 2's existing triage thresholds (<50 files/<3000 lines for small, 50-200/3000-10000 for large). These serve different purposes — diff handling vs. source file inclusion — but the rationale for the different thresholds is not documented, which could cause confusion for the LLM executing the skill.

@fullsend-ai-review fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Jun 5, 2026
@ben-alkov
ben-alkov requested a review from ralphbean June 10, 2026 17:56
@fullsend-ai-coder

fullsend-ai-coder Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ❌ Failure · Started 6:23 PM UTC · Completed 6:30 PM UTC
Commit: 4e21a60 · View workflow run →

@ben-alkov
ben-alkov marked this pull request as draft June 15, 2026 19:34
@ben-alkov
ben-alkov force-pushed the agent/1924-sub-agent-source-files branch from 15408db to 9d13f07 Compare June 18, 2026 17:48
@github-actions

github-actions Bot commented Jun 18, 2026

Copy link
Copy Markdown

E2E tests did not run

E2E tests run automatically for org/repo members and collaborators on pull requests.

For other contributors, a maintainer must add the ok-to-test label after the latest push.

See E2E testing guide for details.

@ben-alkov ben-alkov added agent/review Review agent ok-to-test Allow e2e CI to run after maintainer review (must be re-applied after each push) labels Jun 18, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 18, 2026

Copy link
Copy Markdown

🤖 Review · ❌ Terminated · Started 5:51 PM UTC · Ended 6:06 PM UTC
Commit: 4e21a60 · View workflow run →

@codecov

codecov Bot commented Jun 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@fullsend-ai-review fullsend-ai-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the review comment for full details.

Comment thread internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md
Comment thread internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md
Comment thread internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md
Comment thread internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md Outdated
Comment thread internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md Outdated
Comment thread internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md
@fullsend-ai-review fullsend-ai-review Bot added component/harness Agent harness, config, and skills loading and removed requires-manual-review Review requires human judgment labels Jun 18, 2026
@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 5:51 PM UTC · Completed 6:06 PM UTC
Commit: 9d13f07 · View workflow run →

@ben-alkov
ben-alkov force-pushed the agent/1924-sub-agent-source-files branch from 9d13f07 to 8b7635d Compare June 18, 2026 18:43
@github-actions github-actions Bot removed the ok-to-test Allow e2e CI to run after maintainer review (must be re-applied after each push) label Jun 18, 2026
@ben-alkov

Copy link
Copy Markdown
Member

/fs-fix

@ben-alkov
ben-alkov marked this pull request as ready for review June 18, 2026 18:44
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Pass PR-head source file contents into PR review sub-agent context
📝 Documentation ✨ Enhancement 🕐 10-20 Minutes

Grey Divider

Description

• Fetch full changed-file contents at PR head SHA and share with sub-agents.
• Add source_files to sub-agent context packages and prompt template guidance.
• Introduce large-PR size guard to limit included files and allow fallback disk reads.
Diagram

graph TD
  O["Review orchestrator"] --> D[("PR diff")]
  O --> G{{"GitHub Contents API"}} --> S[("PR-head source files")]
  O --> Z{"Large PR?"}
  Z -- "No" --> C[("Context package (all files)")]
  Z -- "Yes" --> C2[("Context package (selected files)")]
  C --> A["Review sub-agents"]
  C2 --> A --> R[("Repo disk reads (fallback)")]

  subgraph Legend
    direction LR
    _svc["Component"] ~~~ _data[("Data")]
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Ensure sub-agents read from a PR-head checkout (no source_files transfer)
  • ➕ Avoids embedding large file blobs into prompts/context packages
  • ➕ Keeps a single source of truth on disk and reduces prompt churn
  • ➖ Still repeats token/tool calls across sub-agents (each re-reads files)
  • ➖ Requires strong guarantees that each sub-agent runs in the exact PR-head workspace; failures reintroduce stale base-branch reads
2. Shared orchestrator-side file cache + on-demand fetch by sub-agents
  • ➕ Avoids eagerly sending all file contents; sub-agents request only what they need
  • ➕ Eliminates redundant GitHub API calls and disk reads across agents
  • ➖ Adds protocol/IPC complexity between orchestrator and sub-agents
  • ➖ Harder to keep prompts deterministic; more moving parts to debug
3. Send only structured excerpts (e.g., top-N largest hunks + surrounding context)
  • ➕ Lower token footprint than full-file contents
  • ➕ Often sufficient for localized changes
  • ➖ Increases false positives/negatives when understanding requires full-file context (imports, helpers, invariants)
  • ➖ Still encourages follow-up disk reads, reintroducing redundancy/staleness risk

Recommendation: The PR’s approach (embed PR-head full file contents into each sub-agent context, with a size guard) is the best tradeoff for correctness and token efficiency. It directly addresses the key failure mode (sub-agents reading base-branch files) while reducing redundant I/O. If follow-up iteration is needed, consider an orchestrator-side on-demand cache to further reduce prompt size for large PRs, but it’s not required to realize the main benefit.

Files changed (2) +58 / -1

Documentation (2) +58 / -1
SKILL.mdDocument PR-head source file fetching and 'source_files' context field +50/-0

Document PR-head source file fetching and 'source_files' context field

• Adds an orchestrator step to fetch full contents of changed files at the PR head SHA via the GitHub contents API. Extends sub-agent context package schema with a 'source_files' field and adds a prompt-template section instructing sub-agents to prefer these contents, including a large-PR size guard and fallback guidance.

internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md

meta-prompt.mdUpdate sub-agent constraints to use provided PR-head source files +8/-1

Update sub-agent constraints to use provided PR-head source files

• Replaces the generic directive to read full source files with explicit instructions to use the orchestrator-provided "Source files (PR head)" section. Adds a constraint to avoid re-reading already-provided files to reduce token waste and prevent base-branch/stale reads.

internal/scaffold/fullsend-repo/skills/pr-review/meta-prompt.md

@fullsend-ai-coder

fullsend-ai-coder Bot commented Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 6:47 PM UTC · Completed 6:56 PM UTC
Commit: 4e21a60 · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 18, 2026

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 6:48 PM UTC · Ended 6:57 PM UTC
Commit: 4e21a60 · View workflow run →

@qodo-code-review

qodo-code-review Bot commented Jun 18, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 58 rules
✅ Skills: writing-user-docs, writing-adrs

Grey Divider


Action required

1. Disk fallback reads base ✓ Resolved 🐞 Bug ☼ Reliability
Description
The new size-guard and prompt text say sub-agents may read omitted changed files “from disk,” but
the review workflow checks out the target repo without a PR-head ref, so disk reads will see
default-branch (base) code and can reintroduce stale-context false positives on large PRs.
Code

internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md[R154-160]

+**Size guard for large PRs:** If the PR exceeds 20 changed files or
+5000 total changed lines, selectively include only the files most
+relevant to each sub-agent's dimension (files with the most changes,
+files touching security-sensitive paths for the security agent, test
+files for the correctness agent, etc.). Let sub-agents read remaining
+files from disk as needed. For PRs within the threshold, include all
+changed file contents.
Evidence
SKILL.md’s new guidance explicitly notes disk reads would be base-branch, and the review workflow’s
checkout step does not set ref to PR head; therefore any “read remaining changed files from disk”
fallback is base-vs-head unsafe.

internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md[140-143]
internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md[154-160]
internal/scaffold/fullsend-repo/skills/pr-review/meta-prompt.md[35-42]
.github/workflows/reusable-review.yml[120-127]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The docs introduce a fallback for large PRs: sub-agents may read remaining files from disk. In this repo’s review workflow, the target repo is checked out without specifying the PR head SHA, so disk reads are base-branch code. This undermines the core goal (PR-head-accurate context) specifically for large PRs.

## Issue Context
- SKILL.md explicitly warns disk reads would be base-branch.
- The workflow checkout omits `ref`, so it’s not pinned to PR head.

## Fix Focus Areas
- internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md[154-160]
- internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md[367-386]
- internal/scaffold/fullsend-repo/skills/pr-review/meta-prompt.md[35-42]

Suggested direction (pick one and document consistently):
1) **Make disk reads safe**: update the review workflow (or orchestrator step) to checkout PR head SHA before sub-agent reads.
2) **Keep base checkout but remove unsafe fallback**: change the size-guard wording so sub-agents only read from disk for files that are *not* in `changed_files`; for omitted changed files, instruct the orchestrator to fetch PR-head contents via the GitHub contents API instead of telling sub-agents to read from disk.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Unsafe filename iteration ✓ Resolved 🐞 Bug ≡ Correctness
Description
The new SKILL.md bash example uses for FILE in $(...), which performs word-splitting and glob
expansion; changed paths containing whitespace or glob characters can be split/mutated, causing
wrong gh api .../contents/... requests and missing/garbled source content.
Code

internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md[R148-151]

+for FILE in $(echo "$PR_FILES" | jq -r '.[].filename'); do
+  gh api "repos/${REPO_FULL_NAME}/contents/${FILE}?ref=${HEAD_SHA}" \
+    --jq '.content' | base64 -d
+done
Evidence
The snippet introduces an unsafe shell iteration pattern; elsewhere in this repo, scripts avoid this
by reading line-by-line with while IFS= read -r ... to preserve exact file paths.

internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md[145-151]
internal/scaffold/fullsend-repo/scripts/post-review.sh[103-118]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The SKILL.md example uses `for FILE in $(...)`, which is unsafe for valid git paths containing whitespace/newlines or glob characters. This can corrupt the `FILE` value and lead to incorrect GitHub contents API calls.

## Issue Context
This snippet is intended to be followed as orchestrator guidance; it should demonstrate robust, copy-paste-safe shell patterns.

## Fix Focus Areas
- internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md[145-152]

Suggested direction:
- Replace the `for FILE in $(...)` loop with a line-safe read loop:
 - `echo "$PR_FILES" | jq -r '.[].filename' | while IFS= read -r FILE; do ...; done`
- Quote `${FILE}` usage everywhere.
- (Optional) mention URL-encoding if paths may contain `#`/spaces.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Size guard example mismatch ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
SKILL.md describes a size guard for large PRs (selectively include only dimension-relevant files),
but the example fetch loop shown still fetches every changed file; this mismatch can mislead
implementers into doing the expensive/unbounded behavior the guard is meant to avoid.
Code

internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md[R145-160]

+```bash
+HEAD_SHA=$(echo "$PR_META" | jq -r '.head.sha')
+# For each changed file, read its contents at the PR head
+for FILE in $(echo "$PR_FILES" | jq -r '.[].filename'); do
+  gh api "repos/${REPO_FULL_NAME}/contents/${FILE}?ref=${HEAD_SHA}" \
+    --jq '.content' | base64 -d
+done
+```
+
+**Size guard for large PRs:** If the PR exceeds 20 changed files or
+5000 total changed lines, selectively include only the files most
+relevant to each sub-agent's dimension (files with the most changes,
+files touching security-sensitive paths for the security agent, test
+files for the correctness agent, etc.). Let sub-agents read remaining
+files from disk as needed. For PRs within the threshold, include all
+changed file contents.
Evidence
The file shows an unconditional per-file fetch loop, immediately followed by prose requiring
selective inclusion for large PRs; as written, the example and the guard contradict each other.

internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md[145-160]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The documentation introduces a size guard but the immediately-adjacent example still loops over all changed files. This is internally inconsistent and can lead to copying an implementation that defeats the guard.

## Issue Context
The size guard is meant to prevent prompt bloat and redundant API calls on large PRs.

## Fix Focus Areas
- internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md[145-160]

Suggested direction:
- Update the example to demonstrate selecting a filtered file list first (e.g., `SELECTED_FILES=...`) and looping over that.
- Or explicitly label the snippet as “for small PRs” and add a second snippet showing the large-PR selective fetch pattern.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md Outdated
fullsend-ai-coder Bot added a commit that referenced this pull request Jun 18, 2026
- Filter removed files from contents API fetch loop (404 prevention)
- Replace unsafe `for FILE in $(...)` with `while IFS= read -r` loop
- Add binary file filtering and API error handling for 1MB limit
- Remove redundant HEAD_SHA extraction (reuse from step 1)
- Fix heading hierarchy: `####` → `### 2b.` to match `### 2a.` pattern
- Update size guard to use API fallback instead of unsafe disk reads
- Add per-dimension file selection guidance for large PRs
- Align challenger.md constraint with updated meta-prompt.md wording

Addresses review feedback on #1926
```bash
# Filter to non-removed, non-binary files
FETCH_FILES=$(echo "$PR_FILES" \
| jq -r '.[] | select(.status != "removed") | .filename' \

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] logic-error

The bash snippet in step 2b outputs decoded file contents without filename headers or code fences, while steps 3d and 4 expect each file preceded by a #### header and fenced code block. Since this is instructional pseudocode consumed by an LLM orchestrator, the format mismatch is a clarity issue — the orchestrator reads both the fetch script and the formatting instructions.

Suggested fix: Add a note in the snippet that the orchestrator must format the output per steps 3d/4, or add headers to the bash script for clarity.

limit (the API returns 403 for these); log a warning so the orchestrator
knows which files were omitted.

```bash

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] edge-case

The binary-file extension filter includes .pb.go, which are protobuf-generated Go source files (plain text, not binary). The prose describes the filter as excluding binary files, but .pb.go files are human-readable Go code. Conflating generated files with binary exclusion is inaccurate.

Suggested fix: Either move .pb.go to a separate generated-files filter or update the prose to say binary and generated files.

by a `### File: <relative-path>` header so sub-agents can identify file
boundaries. Generated files (lockfiles, vendor/, protobuf output) are
excluded from the concatenation.
- `source_files`: full contents of changed files at the PR head revision,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] pattern-inconsistency

File boundary heading level inconsistency between diff and source_files context package fields. The diff field uses ### File: (h3) for per-file boundaries, while source_files uses #### (h4). The h4 pattern is hierarchically correct; the existing h3 pattern creates sibling sections that break heading hierarchy.

@fullsend-ai-review fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label Jun 18, 2026
ben-alkov pushed a commit that referenced this pull request Jun 29, 2026
- Filter removed files from contents API fetch loop (404 prevention)
- Replace unsafe `for FILE in $(...)` with `while IFS= read -r` loop
- Add binary file filtering and API error handling for 1MB limit
- Remove redundant HEAD_SHA extraction (reuse from step 1)
- Fix heading hierarchy: `####` → `### 2b.` to match `### 2a.` pattern
- Update size guard to use API fallback instead of unsafe disk reads
- Add per-dimension file selection guidance for large PRs
- Align challenger.md constraint with updated meta-prompt.md wording

Addresses review feedback on #1926
@ben-alkov
ben-alkov force-pushed the agent/1924-sub-agent-source-files branch from 385900f to 31d45e2 Compare June 29, 2026 19:11
@ben-alkov ben-alkov added the ok-to-test Allow e2e CI to run after maintainer review (must be re-applied after each push) label Jun 29, 2026

@ralphbean ralphbean left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an automated comment. We are moving agent content from internal/scaffold/fullsend-repo/ to https://github.com/fullsend-ai/agents -- changes should be made to agent definitions there going forwards.

@maruiz93

Copy link
Copy Markdown
Contributor

/fs-fix rebase and fix the comments of the reviewers

@fullsend-ai-coder

fullsend-ai-coder Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 10:34 AM UTC · Completed 10:55 AM UTC
Commit: e8381e3 · View workflow run →

fullsend-ai-coder Bot added a commit that referenced this pull request Jul 10, 2026
- Add size-guard conditional to bash example so it only fetches files
  for small PRs (<=20 files, <=5000 lines); large PRs defer to step 3d
- Tighten disk-read guidance in meta-prompt.md: distinguish unchanged
  files (safe to read from disk) from omitted changed files (must use
  GitHub contents API)
- Apply same clarification to challenger.md constraints
- Strengthen SKILL.md large-PR fallback text: "must" instead of
  "should", explicit note that unchanged files are safe from disk

Addresses review feedback on #1926
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 2 (human-triggered)

Addressed 2 of 3 Qodo review findings (disk fallback wording and size guard mismatch). The third finding (unsafe iteration) was a false positive — the code already uses the safe while-read pattern. Noted ralphbean's repo-migration comment and the rebase request as outside the fix agent's scope.

Fixed (2):

  1. disk fallback reads base-branch code for omitted changed files (internal/scaffold/fullsend-repo/skills/pr-review/meta-prompt.md): Tightened disk-read guidance to explicitly distinguish unchanged files (safe to read from disk) from omitted changed files in large PRs (must use GitHub contents API). Applied same clarification to challenger.md.
  2. size guard example mismatch — code fetches all files but text says to be selective (internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md): Added conditional guard (if FILE_COUNT<=20 and LINE_COUNT<=5000) to the bash example so it only runs the fetch loop for small PRs. Updated the size-guard text below to reference the conditional. Strengthened the large-PR fallback text from 'should' to 'must'.

Disagreed (3):

  1. unsafe filename iteration with for FILE in $(...): The code already uses the safe 'while IFS= read -r FILE' pattern (line 160 of SKILL.md). The Qodo finding was a false positive against the current code.
  2. move agent content from internal/scaffold/fullsend-repo/ to agents repo: ralphbean's automated comment about repo migration is an organizational process concern, not a code fix. The fix agent cannot move files to another repository. This should be handled by the PR author or maintainers as part of the migration effort.
  3. rebase the branch on main: The fix agent cannot run git rebase per its tool constraints. Rebasing must be done manually by a human or by the CI/post-script infrastructure.

Tests: passed

Updated by fullsend fix agent

@fullsend-ai-coder
fullsend-ai-coder Bot requested a review from a team as a code owner July 10, 2026 10:55
@github-actions github-actions Bot removed the ok-to-test Allow e2e CI to run after maintainer review (must be re-applied after each push) label Jul 10, 2026
@maruiz93

Copy link
Copy Markdown
Contributor

/fs-fix DCO is incorrect, rebase and fix it

@fullsend-ai-coder

fullsend-ai-coder Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 10:31 AM UTC · Completed 10:43 AM UTC
Commit: 5cd495a · View workflow run →

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 3 (human-triggered)

Rebased branch onto origin/main with corrected author email on d08cae5. The commit previously used fullsend-code@users.noreply.github.com which the DCO app did not recognize as a bot. Changed to 278716306+fullsend-ai-coder[bot]@users.noreply.github.com to match the other commits. Strategy change at iteration 3: previous iterations likely attempted code-level fixes; this iteration addresses the root cause (commit metadata) via rebase. A force push is required to update the remote branch.

Fixed (1):

  1. DCO check failing on commit d08cae5 due to incorrect author email: Rebased branch onto origin/main and corrected the author email on commit d08cae5 from fullsend-code@users.noreply.github.com to 278716306+fullsend-ai-coder[bot]@users.noreply.github.com so the Probot DCO app correctly auto-skips it as a bot commit. No Signed-off-by trailers added per AGENTS.md policy.

Tests: passed

Strategy change: Previous iterations attempted code-level fixes. This iteration (at escalation threshold) takes a fundamentally different approach: fixing the commit metadata via rebase rather than modifying code.

Decision points
  • Fixed author email identity instead of adding Signed-off-by trailer (alternatives: Add Signed-off-by trailer to commit, Change committer identity only; rationale: AGENTS.md explicitly prohibits Signed-off-by on agent commits. The Probot DCO app auto-skips commits with [bot] in the author email, so fixing the email is the correct approach per repo policy.)

Updated by fullsend fix agent

fullsend-ai-coder Bot added a commit that referenced this pull request Jul 14, 2026
- Filter removed files from contents API fetch loop (404 prevention)
- Replace unsafe `for FILE in $(...)` with `while IFS= read -r` loop
- Add binary file filtering and API error handling for 1MB limit
- Remove redundant HEAD_SHA extraction (reuse from step 1)
- Fix heading hierarchy: `####` → `### 2b.` to match `### 2a.` pattern
- Update size guard to use API fallback instead of unsafe disk reads
- Add per-dimension file selection guidance for large PRs
- Align challenger.md constraint with updated meta-prompt.md wording

Addresses review feedback on #1926
fullsend-ai-coder Bot added a commit that referenced this pull request Jul 14, 2026
- Add size-guard conditional to bash example so it only fetches files
  for small PRs (<=20 files, <=5000 lines); large PRs defer to step 3d
- Tighten disk-read guidance in meta-prompt.md: distinguish unchanged
  files (safe to read from disk) from omitted changed files (must use
  GitHub contents API)
- Apply same clarification to challenger.md constraints
- Strengthen SKILL.md large-PR fallback text: "must" instead of
  "should", explicit note that unchanged files are safe from disk

Addresses review feedback on #1926
@fullsend-ai-coder
fullsend-ai-coder Bot force-pushed the agent/1924-sub-agent-source-files branch from 19603cb to f5d28d2 Compare July 14, 2026 10:43
The review orchestrator already fetches full source files of changed
paths but did not include them in sub-agent context packages.
Sub-agents independently re-read the same files from disk, causing
redundant token usage (5-6 reads of the same file across 4 agents)
and false positives from reading base-branch code instead of PR head.

Changes:
- SKILL.md step 2: add orchestrator logic to fetch source file
  contents at PR head SHA via the GitHub contents API
- SKILL.md section 3d: add source_files field to context packages
- SKILL.md step 4 Part 4: add "Source files (PR head)" section to
  the sub-agent prompt template with inline instructions
- meta-prompt.md: replace "read full source files" constraint with
  instruction to use provided source files and avoid redundant reads
- Add size guard: for large PRs (>20 files or >5000 lines), include
  only dimension-relevant files; sub-agents fall back to disk reads

Closes #1924
- Filter removed files from contents API fetch loop (404 prevention)
- Replace unsafe `for FILE in $(...)` with `while IFS= read -r` loop
- Add binary file filtering and API error handling for 1MB limit
- Remove redundant HEAD_SHA extraction (reuse from step 1)
- Fix heading hierarchy: `####` → `### 2b.` to match `### 2a.` pattern
- Update size guard to use API fallback instead of unsafe disk reads
- Add per-dimension file selection guidance for large PRs
- Align challenger.md constraint with updated meta-prompt.md wording

Addresses review feedback on #1926
- Add size-guard conditional to bash example so it only fetches files
  for small PRs (<=20 files, <=5000 lines); large PRs defer to step 3d
- Tighten disk-read guidance in meta-prompt.md: distinguish unchanged
  files (safe to read from disk) from omitted changed files (must use
  GitHub contents API)
- Apply same clarification to challenger.md constraints
- Strengthen SKILL.md large-PR fallback text: "must" instead of
  "should", explicit note that unchanged files are safe from disk

Addresses review feedback on #1926
@maruiz93

Copy link
Copy Markdown
Contributor

Superseeded by fullsend-ai/agents#172

@maruiz93 maruiz93 closed this Jul 15, 2026
maruiz93 added a commit to maruiz93/agents that referenced this pull request Jul 16, 2026
The review orchestrator already fetches full source files of changed
paths but did not include them in sub-agent context packages. Sub-agents
independently re-read the same files from disk, causing redundant token
usage (5-6 reads of the same file across 4 agents) and false positives
from reading base-branch code instead of PR head.

Changes:
- SKILL.md step 2b: fetch source file contents at PR head SHA via the
  GitHub contents API
- SKILL.md section 3d: add source_files field to context packages
- SKILL.md step 4 Part 4: add "Source files (PR head)" section to the
  sub-agent prompt template with inline instructions
- meta-prompt.md: replace "read full source files" constraint with
  instruction to use provided source files and avoid redundant reads
- challenger.md: align constraint with meta-prompt.md
- Size guard: for large PRs (>20 files or >5000 lines), include only
  dimension-relevant files; sub-agents fall back to API reads

Port of fullsend-ai/fullsend#1926.
Closes fullsend-ai/fullsend#1924

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Marta Anon <manon@redhat.com>
maruiz93 added a commit to maruiz93/agents that referenced this pull request Jul 17, 2026
The review orchestrator already fetches full source files of changed
paths but did not include them in sub-agent context packages. Sub-agents
independently re-read the same files from disk, causing redundant token
usage (5-6 reads of the same file across 4 agents) and false positives
from reading base-branch code instead of PR head.

Changes:
- SKILL.md step 2b: fetch source file contents at PR head SHA via the
  GitHub contents API
- SKILL.md section 3d: add source_files field to context packages
- SKILL.md step 4 Part 4: add "Source files (PR head)" section to the
  sub-agent prompt template with inline instructions
- meta-prompt.md: replace "read full source files" constraint with
  instruction to use provided source files and avoid redundant reads
- challenger.md: align constraint with meta-prompt.md
- Size guard: for large PRs (>20 files or >5000 lines), include only
  dimension-relevant files; sub-agents fall back to API reads

Port of fullsend-ai/fullsend#1926.
Closes fullsend-ai/fullsend#1924

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Marta Anon <manon@redhat.com>
@github-actions
github-actions Bot deleted the agent/1924-sub-agent-source-files branch August 16, 2026 03:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent/review Review agent component/harness Agent harness, config, and skills loading ready-for-merge All reviewers approved — ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Review sub-agents should receive source file contents in prompt instead of re-reading from disk

3 participants