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

[maruiz93]

Summary

• Fetch full contents of changed files at PR head SHA and pass them inline to review sub-agents, eliminating redundant disk reads (5-6 reads of the same file across 4 agents) and false positives from reading base-branch code
• Add source_files field to sub-agent context packages and a "Source files (PR head)" section to the prompt template
• Include a size guard for large PRs (>20 files or >5000 lines): defer file selection to per-dimension context assembly; sub-agents fall back to GitHub contents API reads (not disk) for omitted files

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

Test plan

• Verify review agent run on a small PR (<20 files) includes source files in sub-agent context
• Verify sub-agents make ≤1 Read call per changed file (down from 3-5)
• Verify no false positives from base-branch vs PR-head confusion
• Verify large PR (>20 files) selectively includes dimension-relevant files
• Verify sub-agents use GitHub contents API fallback (not disk) for omitted files in large PRs

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Perf: pass PR-head source files to review sub-agents

✨ Enhancement 📝 Documentation 🕐 10-20 Minutes

AI Description

• Fetch changed-file contents at PR head SHA and include them in sub-agent context.
• Prevent redundant disk reads and base-branch/PR-head mismatch false positives.
• Add size guard to selectively include files for large PRs with API fallback.

Diagram

graph TD
O["Review orchestrator"] --> D["Fetch PR diff"] --> P["PR files list"] --> C["Fetch file contents @ HEAD_SHA"] --> S["Context package (source_files)"] --> A["Review sub-agent"]
C --> G["GitHub Contents API"]
A -->|"only if missing (large PR)"| G
A -->|"avoid for changed files"| L["Local repo disk (base branch)"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Sub-agents fetch PR-head sources themselves (API-only)

• ➕ Simpler orchestrator (no source_files packaging)
• ➕ Each agent can pull only what it needs
• ➖ Repeated API calls across sub-agents reintroduce redundancy
• ➖ Harder to enforce consistent filtering (binary/removed/size limits)
• ➖ More variability in review latency and token usage

2. Use git plumbing locally (git show HEAD_SHA:path) instead of Contents API

• ➕ Avoids GitHub Contents API 1MB/403 and rate-limit concerns
• ➕ Works offline given a full fetch of the PR head SHA
• ➖ Requires guaranteeing the PR head commit and blobs are present locally
• ➖ More complex setup in CI runners; still needs careful binary/removed handling

3. Central content cache keyed by (HEAD_SHA, path) shared across agents

• ➕ Best of both worlds: no duplication and no prompt bloat for very large PRs
• ➕ Can support streaming/partial inclusion policies
• ➖ More infrastructure/complexity (cache lifecycle, invalidation, serialization)
• ➖ Still needs a policy for what gets embedded vs referenced

Recommendation: Keep the PR’s approach: embed PR-head source contents into sub-agent context for small PRs, with a size guard and API fallback for large PRs. This provides the biggest reduction in redundant reads and eliminates base-branch confusion while keeping large-PR prompts bounded; the alternatives either reintroduce duplication (agent-driven fetching) or add operational complexity (local git plumbing / shared cache).

Files changed (3) +90 / -2

Enhancement (1) +80 / -0

SKILL.mdAdd PR-head source fetching + source_files context contract +80/-0

Add PR-head source fetching + source_files context contract

• Introduces a new step to fetch full contents of changed files at the PR head SHA via the GitHub contents API, including filtering and failure/skip guidance. Extends sub-agent context packages with a new source_files field and updates the prompt template to include a dedicated "Source files (PR head)" section plus large-PR fallback instructions.

skills/pr-review/SKILL.md

Other (2) +10 / -2

meta-prompt.mdUpdate review constraints to prefer provided PR-head sources +8/-1

Update review constraints to prefer provided PR-head sources

• Replaces the generic instruction to read full source files with explicit guidance 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 stale-code risk.

skills/pr-review/meta-prompt.md

challenger.mdAlign challenger constraints with PR-head source passthrough +2/-1

Align challenger constraints with PR-head source passthrough

• Updates challenger sub-agent constraints to use provided source files rather than reading changed files from disk, only pulling extra context when needed beyond what is included.

skills/pr-review/sub-agents/challenger.md

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:00 AM UTC · Completed 7:18 AM UTC
Commit: 6a4ecc8 · View workflow run →

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (1)

Context used

✅ Compliance rules (platform): 55 rules

✅ Skills: 4 invoked

  code-review

  code-implementation

  pr-review

  docs-review

1. Source files lack delimiters ✓ Resolved 🐞 Bug ≡ Correctness

Description

The step 2b fetch loop outputs decoded file contents without emitting filenames/delimiters, so
multiple files become an undifferentiated stream and the orchestrator cannot reliably build the
per-file source_files section. This directly conflicts with the context contract that requires
#### <relative-path> headers and fenced blocks per file, risking misattributed code during review.

Code

skills/pr-review/SKILL.md[R160-166]

+echo "$FETCH_FILES" | while IFS= read -r FILE; do
+  RESP=$(gh api "repos/${REPO_FULL_NAME}/contents/${FILE}?ref=${HEAD_SHA}" 2>&1) || {
+    echo "::warning::Skipping ${FILE}: contents API error" >&2
+    continue
+  }
+  echo "$RESP" | jq -r '.content' | base64 -d
+done

Relevance

⭐⭐ Medium

No historical evidence found about requiring #### headers/delimiters when emitting multiple file
contents.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The step 2b snippet decodes and prints only file contents (no filename headers/delimiters), while
the later context-package contract explicitly requires each file to be preceded by a `####
<relative-path>` header and wrapped in a fenced code block.

skills/pr-review/SKILL.md[152-166]
skills/pr-review/SKILL.md[350-353]

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 step 2b example fetch loop prints raw decoded contents for each file without any boundary markers, but later steps require `source_files` to be structured per file with `#### <relative-path>` headers and fenced code blocks.

## Issue Context
Sub-agents are instructed to rely on `Source files (PR head)` for correctness. If the orchestrator can’t map decoded content back to its path, the section becomes unusable or misleading.

## Fix Focus Areas
- skills/pr-review/SKILL.md[152-167]
- skills/pr-review/SKILL.md[350-353]

## Suggested change
Update the step 2b fetch example (and corresponding orchestrator guidance) to emit a header per file (e.g., `#### ${FILE}`) and wrap decoded output in a fenced block. At minimum, add an unambiguous delimiter before/after each file so the orchestrator can reconstruct a per-file map.

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

---

2. Unsanitized ${FILE} in ::warning:: ✓ Resolved 📜 Skill insight ⛨ Security

Description

The new bash snippet emits a GitHub Actions workflow command (::warning::) with an interpolated
value (${FILE}) that is not sanitized. A crafted filename containing ::, %0A/%0D, or control
characters could inject additional workflow commands into logs.

Code

skills/pr-review/SKILL.md[R160-163]

+echo "$FETCH_FILES" | while IFS= read -r FILE; do
+  RESP=$(gh api "repos/${REPO_FULL_NAME}/contents/${FILE}?ref=${HEAD_SHA}" 2>&1) || {
+    echo "::warning::Skipping ${FILE}: contents API error" >&2
+    continue

Relevance

⭐⭐ Medium

Mixed history: sanitize workflow-command interpolation accepted (PR #90) but similar sanitization
rejected (PR #148).

PR-#90
PR-#148

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1538382 requires every interpolated value in workflow commands like ::warning::
to be sanitized. The added snippet echoes ::warning::Skipping ${FILE}: ... without sanitizing
FILE.

skills/pr-review/SKILL.md[160-163]
Skill: pr-review

Agent prompt

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

## Issue description
A GitHub Actions workflow command (`::warning::...`) is emitted with an unsanitized interpolated variable (`FILE`). This can allow workflow command injection via `::`, `%0A/%0D`, ANSI/control characters.

## Issue Context
The snippet in `skills/pr-review/SKILL.md` is intended to be executed in environments where GitHub Actions command parsing may apply. The compliance rule requires sanitizing **each** interpolated variable used in workflow commands.

## Fix Focus Areas
- skills/pr-review/SKILL.md[160-163]

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

---

3. HEAD_SHA not passed ✓ Resolved 🐞 Bug ≡ Correctness

Description

The skill requires sub-agents to fetch omitted changed files via the contents API using HEAD_SHA,
but the context package field list does not explicitly include HEAD_SHA or REPO_FULL_NAME as
provided context fields. This leaves the large-PR fallback underspecified and can prevent sub-agents
from retrieving PR-head code for omitted changed files.

Code

skills/pr-review/SKILL.md[R350-357]

+- `source_files`: full contents of changed files at the PR head revision,
+  fetched by the orchestrator in step 2b. Each file is preceded by a
+  `#### <relative-path>` header and wrapped in a fenced code block with
+  the appropriate language identifier. For large PRs (>20 files or >5000
+  lines), include only the files most relevant to the sub-agent's
+  dimension; the sub-agent may fetch additional changed files via the
+  GitHub contents API using `HEAD_SHA` (not from disk, which contains
+  base-branch code).

Relevance

⭐⭐ Medium

No historical evidence found on enforcing HEAD_SHA/REPO_FULL_NAME inclusion in sub-agent context
packages.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Step 2b explicitly says to include HEAD_SHA and REPO_FULL_NAME for sub-agents’ fallback fetch,
but the context package field list that defines what gets passed to sub-agents doesn’t include those
values.

skills/pr-review/SKILL.md[181-185]
skills/pr-review/SKILL.md[341-367]

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 doc now instructs sub-agents to fetch additional changed files via the contents API using `HEAD_SHA`, but the context package schema bullets don’t list `HEAD_SHA`/`REPO_FULL_NAME` as fields to include.

## Issue Context
In large PRs, some changed files may be omitted from `source_files`. Without explicit `HEAD_SHA` (and repo identifier) in the sub-agent context, the documented fallback becomes unreliable.

## Fix Focus Areas
- skills/pr-review/SKILL.md[181-185]
- skills/pr-review/SKILL.md[341-367]

## Suggested change
Add explicit context fields (e.g., `head_sha` and `repo_full_name`) to the context package list and ensure the spawn prompt template includes them in a clearly labeled section so sub-agents can deterministically construct contents API calls.

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

---

4. Contents API fetch brittle ✓ Resolved 🐞 Bug ☼ Reliability

Description

The suggested contents fetch interpolates ${FILE} directly into the API path and merges stderr
into the captured response, making parsing fragile for filenames with URL-significant characters and
for any diagnostic output emitted alongside a successful response. The pipeline also doesn’t
validate that .content exists before base64-decoding, so non-file responses (or unexpected API
shapes) can silently produce bad/empty source_files.

Code

skills/pr-review/SKILL.md[R154-165]

+FETCH_FILES=$(echo "$PR_FILES" \
+  | jq -r '.[] | select(.status != "removed") | .filename' \
+  | grep -v -E '\.(png|jpg|jpeg|gif|ico|svg|woff2?|ttf|eot|pdf|zip|tar|gz|bin|exe|dll|so|dylib|wasm|pb\.go|lock)$')
+
+# For small PRs (≤20 files and ≤5000 lines), fetch all; for large PRs,
+# select a subset per dimension in step 3d.
+echo "$FETCH_FILES" | while IFS= read -r FILE; do
+  RESP=$(gh api "repos/${REPO_FULL_NAME}/contents/${FILE}?ref=${HEAD_SHA}" 2>&1) || {
+    echo "::warning::Skipping ${FILE}: contents API error" >&2
+    continue
+  }
+  echo "$RESP" | jq -r '.content' | base64 -d

Relevance

⭐⭐⭐ High

Team tends to harden gh/jq parsing and null-guard API responses (accepted/partially accepted in PR
#94).

PR-#94

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The snippet shows ${FILE} is embedded directly in the contents/ path and stderr is merged into
the captured response before jq extraction and base64 decode, with no validation of .content.

skills/pr-review/SKILL.md[154-166]

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 step 2b example uses `gh api ".../contents/${FILE}?ref=${HEAD_SHA}" 2>&1` and then pipes the captured string into `jq`/`base64 -d`. This is brittle for URL/path edge cases and for any mixed stderr/stdout output, and it doesn’t verify `.content` is present/decodable.

## Issue Context
This fetch is the new primary source of truth for sub-agents. When it fails or produces malformed output, sub-agents either review stale code or proceed without authoritative source.

## Fix Focus Areas
- skills/pr-review/SKILL.md[152-166]

## Suggested change
- Avoid `2>&1` in the JSON capture; keep stderr separate and include the error message in the warning.
- URL-encode `${FILE}` for the API path (or use a method that safely encodes path components).
- Use `gh api --jq '.content'` (or `jq -er '.content'`) and explicitly handle missing/empty content before `base64 -d`.
- Guard against empty `${FILE}` rows in the loop (`[ -z "$FILE" ] && continue`).

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

---

5. Protected skills/ files modified 📜 Skill insight § Compliance

Description

This PR modifies files under skills/, which is a protected governance/infrastructure path
requiring explicit human review (must not be auto-approved). Ensure appropriate reviewer/approval
flow is enforced for these changes.

Code

skills/pr-review/meta-prompt.md[R45-52]

+- **Use the source files provided in the "Source files (PR head)"
+  section below.** These are the full contents of changed files at the
+  PR head commit — they reflect the actual code being reviewed, not
+  the base branch. Only read additional files from disk if you need
+  context beyond the changed files provided (e.g., call sites,
+  dependencies, or sibling files for pattern comparison).
+- Do not re-read files that are already provided in the source files
+  section. This wastes tokens and risks reading stale base-branch code.

Relevance

⭐⭐ Medium

Protected-path skills/ review-gating suggestion exists but outcome undetermined (PR #59).

PR-#59

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1538392 lists skills/ as a protected path and requires raising a finding whenever
such files are modified. This PR changes skills/pr-review/meta-prompt.md (and other files under
skills/pr-review/).

skills/pr-review/meta-prompt.md[45-52]
Skill: pr-review

Agent prompt

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

## Issue description
Protected governance/infrastructure paths were modified (under `skills/`). These changes must not be auto-approved and should receive explicit human review.

## Issue Context
This PR is authorized by linked issue #1924, but compliance still requires raising a protected-path finding and ensuring review routing/controls are applied.

## Fix Focus Areas
- skills/pr-review/meta-prompt.md[45-52]

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

---

6. Untrusted sources not stated ✓ Resolved 🐞 Bug ⛨ Security

Description

The meta-prompt declares the diff and PR metadata as untrusted input, but it doesn’t extend that
warning to the new inline Source files (PR head) payload. Since source files can contain
instruction-like text (comments/strings), this weakens prompt-injection defense-in-depth for
sub-agents.

Code

skills/pr-review/meta-prompt.md[R45-52]

+- **Use the source files provided in the "Source files (PR head)"
+  section below.** These are the full contents of changed files at the
+  PR head commit — they reflect the actual code being reviewed, not
+  the base branch. Only read additional files from disk if you need
+  context beyond the changed files provided (e.g., call sites,
+  dependencies, or sibling files for pattern comparison).
+- Do not re-read files that are already provided in the source files
+  section. This wastes tokens and risks reading stale base-branch code.

Relevance

⭐⭐ Medium

No historical evidence found that prompts must explicitly mark inline source-file payloads as
untrusted.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The meta-prompt’s untrusted-input warning only names the diff and PR metadata, while the updated
constraints introduce reliance on a new Source files (PR head) section without adding it to the
untrusted-input framing.

skills/pr-review/meta-prompt.md[3-8]
skills/pr-review/meta-prompt.md[45-52]

Agent prompt

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

## Issue description
meta-prompt.md warns that the diff and PR metadata are untrusted input, but the PR adds a new `Source files (PR head)` section that sub-agents are instructed to rely on, without explicitly marking it as untrusted.

## Issue Context
Source files are authored by the PR submitter and can contain instruction-like content (comments/strings). Sub-agents should treat them as data to analyze, not directives.

## Fix Focus Areas
- skills/pr-review/meta-prompt.md[3-8]
- skills/pr-review/meta-prompt.md[45-52]

## Suggested change
Update the untrusted-input paragraph to explicitly include the `Source files (PR head)` section (and any other inline PR-authored content) as untrusted input, with the same instruction-like-text warning.

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

---

[fullsend-ai-review]

Review

Findings

Medium

• [workflow-permission] skills/pr-review/SKILL.md — The SAFE_FILE sanitization for GHA workflow commands strips literal newlines (tr -d '\n\r') and :: sequences (sed 's/:://g') but does not strip URL-encoded newlines (%0A, %0a, %0D, %0d). The established sanitize_gha() function in scripts/post-scribe.sh strips both patterns. A malicious filename containing %0A::set-env name=FOO::bar would survive the current sanitization because %0A is interpreted by the GHA log processor as a newline before :: stripping can act on the injected workflow command.
  Remediation: Add %0A/%0a/%0D/%0d stripping to the SAFE_FILE sanitization, consistent with sanitize_gha() in scripts/post-scribe.sh. For example: sed -e 's/%0[aAdD]//g' -e 's/:://g'.

• [protected-path] skills/pr-review/SKILL.md, skills/pr-review/meta-prompt.md, skills/pr-review/sub-agents/challenger.md — This PR modifies files under the protected skills/ path. The PR links to Review sub-agents should receive source file contents in prompt instead of re-reading from disk fullsend#1924 and provides rationale for the changes. Human approval is always required for protected-path changes, regardless of context.

Low

• [logic-error] skills/pr-review/SKILL.md — Step 6d prose states the challenger "receives only the raw findings and the diff," but the challenger template now also includes a ### Source files (PR head) section. The Part 3 description similarly omits source files from its enumeration even though the template includes them. The template is authoritative so the risk of actual omission is low, but the prose should match.
  Remediation: Update the step 6d introduction to mention source files alongside findings and diff; update the Part 3 description to include source files.

• [injection-vuln] skills/pr-review/SKILL.md — The raw FILE variable is interpolated into markdown output on the primary path (echo "#### ${FILE}") without the SAFE_FILE sanitization applied on the error paths. Additionally, base64-decoded file content could contain triple backticks that prematurely close the fenced code block. Exploitation requires repo write access and the meta-prompt.md untrusted-input framing provides defense-in-depth.

• [edge-case] skills/pr-review/SKILL.md — The step 2b bash script passes file paths directly to the GitHub contents API without URL-encoding. Files with special URL characters (spaces, #, ?) would produce malformed API requests. The error is caught by the || { continue } block so the failure is graceful, but affected files would be silently omitted.

Previous run

Review

Findings

Medium

• [workflow-permission] skills/pr-review/SKILL.md — The SAFE_FILE sanitization for GHA workflow commands strips literal newlines (tr -d '\n\r') and :: sequences (sed 's/:://g') but does not strip URL-encoded newlines (%0A, %0a, %0D, %0d). The established sanitize_gha() function in scripts/post-scribe.sh strips both patterns. A malicious filename containing %0A::set-env name=FOO::bar would survive the current sanitization because %0A is interpreted by the GHA log processor as a newline before :: stripping can act on the injected workflow command.
  Remediation: Add %0A/%0a/%0D/%0d stripping to the SAFE_FILE sanitization, consistent with sanitize_gha() in scripts/post-scribe.sh. For example: sed -e 's/%0[aAdD]//g' -e 's/:://g'.

• [protected-path] skills/pr-review/SKILL.md, skills/pr-review/meta-prompt.md, skills/pr-review/sub-agents/challenger.md — This PR modifies files under the protected skills/ path. The PR links to Review sub-agents should receive source file contents in prompt instead of re-reading from disk fullsend#1924 and provides rationale for the changes. Human approval is always required for protected-path changes, regardless of context.

Low

• [logic-error] skills/pr-review/SKILL.md — Step 6d prose states the challenger "receives only the raw findings and the diff," but the challenger template now also includes a ### Source files (PR head) section. The Part 3 description similarly omits source files from its enumeration even though the template includes them. The template is authoritative so the risk of actual omission is low, but the prose should match.
  Remediation: Update the step 6d introduction to mention source files alongside findings and diff; update the Part 3 description to include source files.

• [injection-vuln] skills/pr-review/SKILL.md — The raw FILE variable is interpolated into markdown output on the primary path (echo "#### ${FILE}") without the SAFE_FILE sanitization applied on the error paths. Additionally, base64-decoded file content could contain triple backticks that prematurely close the fenced code block. Exploitation requires repo write access and the meta-prompt.md untrusted-input framing provides defense-in-depth.

• [edge-case] skills/pr-review/SKILL.md — The step 2b bash script passes file paths directly to the GitHub contents API without URL-encoding. Files with special URL characters (spaces, #, ?) would produce malformed API requests. The error is caught by the || { continue } block so the failure is graceful, but affected files would be silently omitted.

---

Labels: PR modifies review agent skill definitions

Previous run (2)

Review

Findings

Medium

• [workflow-permission] skills/pr-review/SKILL.md — The SAFE_FILE sanitization for GHA workflow commands strips literal newlines (tr -d '\n\r') and :: sequences (sed 's/:://g') but does not strip URL-encoded newlines (%0A, %0a, %0D, %0d). The established sanitize_gha() function in scripts/post-scribe.sh strips both patterns. A malicious filename containing %0A::set-env name=FOO::bar would survive the current sanitization because %0A is interpreted by the GHA log processor as a newline before :: stripping can act on the injected workflow command.
  Remediation: Add %0A/%0a/%0D/%0d stripping to the SAFE_FILE sanitization, consistent with sanitize_gha() in scripts/post-scribe.sh. For example: sed -e 's/%0[aAdD]//g' -e 's/:://g'.

• [protected-path] skills/pr-review/SKILL.md, skills/pr-review/meta-prompt.md, skills/pr-review/sub-agents/challenger.md — This PR modifies files under the protected skills/ path. The PR links to Review sub-agents should receive source file contents in prompt instead of re-reading from disk fullsend#1924 and provides rationale for the changes. Human approval is always required for protected-path changes, regardless of context.

Low

• [logic-error] skills/pr-review/SKILL.md — Step 6d prose states the challenger "receives only the raw findings and the diff," but the challenger template now also includes a ### Source files (PR head) section. The Part 3 description similarly omits source files from its enumeration even though the template includes them. The template is authoritative so the risk of actual omission is low, but the prose should match.
  Remediation: Update the step 6d introduction to mention source files alongside findings and diff; update the Part 3 description to include source files.

• [logic-error] skills/pr-review/SKILL.md — Step 3d describes head_sha and repo_full_name as fields in the context package, and the large-PR note in the Source files template references ${REPO_FULL_NAME} and ${HEAD_SHA} as shell variable syntax. The step 4 template does not include dedicated sections providing these concrete values to sub-agents. The step 4 template should make the expected format explicit.
  Remediation: Add ### Head SHA and ### Repo full name sections to the step 4 template, or replace shell variable references with placeholder syntax the orchestrator fills in.

• [injection-vuln] skills/pr-review/SKILL.md — The raw FILE variable is interpolated into markdown output on the primary path (echo "#### ${FILE}") without the SAFE_FILE sanitization applied on the error paths. Additionally, base64-decoded file content could contain triple backticks that prematurely close the fenced code block. Exploitation requires repo write access and the meta-prompt.md untrusted-input framing provides defense-in-depth.

• [edge-case] skills/pr-review/SKILL.md — The step 2b bash script passes file paths directly to the GitHub contents API without URL-encoding. Files with special URL characters (spaces, #, ?) would produce malformed API requests. The error is caught by the || { continue } block so the failure is graceful, but affected files would be silently omitted.

• [pattern-inconsistency] skills/pr-review/meta-prompt.md — The new constraint bullets mix imperative directives with explanatory rationale and conditional clauses. Other constraints in this file use terse imperatives like "Stay within your owned dimension" and "Do not write any files".

• [pattern-inconsistency] skills/pr-review/sub-agents/challenger.md — The new constraint is more verbose and conditional than the terse imperative style used by other constraints in this file.

Previous run (3)

Review

Findings

Medium

• [workflow-permission] skills/pr-review/SKILL.md:165 — The SAFE_FILE sanitization for GHA workflow commands strips literal newlines (tr -d '\n\r') and :: sequences (sed 's/:://g') but does not strip URL-encoded newlines (%0A, %0a, %0D, %0d). The established sanitize_gha() function in scripts/post-scribe.sh strips both patterns. A malicious filename containing %0A::set-env name=FOO::bar would survive the current sanitization because %0A is interpreted by the GHA log processor as a newline before :: stripping can act on the injected workflow command.
  Remediation: Add %0A/%0a/%0D/%0d stripping to the SAFE_FILE sanitization, consistent with sanitize_gha() in scripts/post-scribe.sh. For example: sed -e 's/:://g' -e 's/%0[aAdD]//g'.

• [protected-path] skills/pr-review/SKILL.md, skills/pr-review/meta-prompt.md, skills/pr-review/sub-agents/challenger.md — This PR modifies files under the protected skills/ path. The PR links to Review sub-agents should receive source file contents in prompt instead of re-reading from disk fullsend#1924 and provides rationale for the changes. Human approval is always required for protected-path changes, regardless of context.

Low

• [logic-error] skills/pr-review/SKILL.md:620 — Step 6d prose states the challenger "receives only the raw findings and the diff," but the template at lines 636-654 now also includes source files (line 645). The Part 3 description at lines 632-634 similarly omits source files from its enumeration even though the template includes a ### Source files (PR head) section. The template is authoritative so the risk of actual omission is low, but the prose should match.
  Remediation: Update line 620 to mention source files alongside findings and diff; update lines 632-634 to include source files in the Part 3 description.

• [injection-vuln] skills/pr-review/SKILL.md:175 — The raw FILE variable is interpolated into markdown output on the primary path (echo "#### ${FILE}") without the SAFE_FILE sanitization applied on the error paths (lines 165, 170). Additionally, base64-decoded file content could contain triple backticks that prematurely close the fenced code block. Exploitation requires repo write access and the meta-prompt.md untrusted-input framing provides defense-in-depth.

• [pattern-inconsistency] skills/pr-review/sub-agents/challenger.md:74 — The new constraint is more verbose and conditional than the terse imperative style used by other constraints in this file (e.g., "Do not add new findings - only adjudicate existing ones").

Previous run (4)

Review

Findings

High

• [logic-error] skills/pr-review/sub-agents/challenger.md:75 — The challenger sub-agent constraint is changed to "Use provided source files instead of reading from disk," but the challenger dispatch in SKILL.md step 6d does not include source_files in the challenger's context package. Step 6d Part 3 only includes: Findings to challenge, Diff, Changed files, and PR metadata - no "Source files (PR head)" section was added. Additionally, meta-prompt.md (included as Part 2 in the challenger dispatch) now instructs sub-agents to use the source files section, reinforcing a reference to content the challenger will never receive. The challenger's ability to verify findings against actual source code will be degraded.
  Remediation: Either (a) add a ### Source files (PR head) section to the challenger's context package template in step 6d Part 3 of SKILL.md, or (b) revert the challenger.md constraint change and keep the original "Read full source files, not just the diff hunks" wording so the challenger continues to read from disk.

Medium

• [edge-case] skills/pr-review/SKILL.md:160 — The bash snippet in step 2b outputs decoded file contents to stdout with no delimiter, file-path header, or code fence between files. All file contents are concatenated into an undifferentiated stream. However, step 3d specifies that source_files should have each file preceded by a #### <relative-path> header and wrapped in a fenced code block, and the step 4 template shows the expected format with headers and fences. The snippet does not produce output matching either specification.
  Remediation: Add file headers and code fence delimiters to the bash snippet output, or add a note that the orchestrator must format the raw output into the expected structure.

• [protected-path] skills/pr-review/SKILL.md — This PR modifies files under the protected skills/ path: skills/pr-review/SKILL.md, skills/pr-review/meta-prompt.md, skills/pr-review/sub-agents/challenger.md. The PR links to Review sub-agents should receive source file contents in prompt instead of re-reading from disk fullsend#1924 and provides rationale. Human approval is always required for protected-path changes, regardless of context.

Low

• [api-contract] skills/pr-review/SKILL.md:165 — The bash snippet uses base64 -d which is not portable to macOS (BSD base64 requires -D or --decode). The same pattern appears in the large-PR fallback instructions in step 4. While this is an LLM instruction and the primary deployment is Linux containers, using base64 --decode would be more portable.

• [api-contract] skills/pr-review/SKILL.md:149 — Step 2b states the GitHub contents API "returns 403 for files exceeding 1 MB." The API actually returns a 200 with an empty content field and encoding: "none" for files between 1-100 MB. A 403 is returned only above 100 MB. Checking for 403 would miss the 1-100 MB case where content is silently empty.

• [doc-style] skills/pr-review/meta-prompt.md:46 — The new constraint uses bold formatting (**Use the source files provided**) not present in other constraints in this file. Existing constraints use plain imperative text without bold.

Previous run (5)

Review

Findings

Medium

• [workflow-permission] skills/pr-review/SKILL.md — The SAFE_FILE sanitization for GHA workflow commands strips literal newlines (tr -d '\n\r') and :: sequences (sed 's/:://g') but does not strip URL-encoded newlines (%0A, %0a, %0D, %0d). The established sanitize_gha() function in scripts/post-scribe.sh strips both patterns. A malicious filename containing %0A::set-env name=FOO::bar would survive the current sanitization because %0A is interpreted by the GHA log processor as a newline before :: stripping can act on the injected workflow command.
  Remediation: Add %0A/%0a/%0D/%0d stripping to the SAFE_FILE sanitization, consistent with sanitize_gha() in scripts/post-scribe.sh. For example: sed -e 's/%0[aAdD]//g' -e 's/:://g'.

• [protected-path] skills/pr-review/SKILL.md, skills/pr-review/meta-prompt.md, skills/pr-review/sub-agents/challenger.md — This PR modifies files under the protected skills/ path. The PR links to Review sub-agents should receive source file contents in prompt instead of re-reading from disk fullsend#1924 and provides rationale for the changes. Human approval is always required for protected-path changes, regardless of context.

Low

• [logic-error] skills/pr-review/SKILL.md — Step 6d prose states the challenger "receives only the raw findings and the diff," but the challenger template now also includes a ### Source files (PR head) section. The Part 3 description similarly omits source files from its enumeration even though the template includes them. The template is authoritative so the risk of actual omission is low, but the prose should match.
  Remediation: Update the step 6d introduction to mention source files alongside findings and diff; update the Part 3 description to include source files.

• [logic-error] skills/pr-review/SKILL.md — Step 3d describes head_sha and repo_full_name as fields in the context package, and the large-PR note in the Source files template references ${REPO_FULL_NAME} and ${HEAD_SHA} as shell variable syntax. The step 4 template does not include dedicated sections providing these concrete values to sub-agents. The orchestrator could substitute them at assembly time, but the template should make the expected format explicit.
  Remediation: Add ### Head SHA and ### Repo full name sections to the step 4 template, or replace shell variable references with placeholder syntax the orchestrator fills in.

• [injection-vuln] skills/pr-review/SKILL.md — The raw FILE variable is interpolated into markdown output on the primary path (echo "#### ${FILE}") without the SAFE_FILE sanitization applied on the error paths. Additionally, base64-decoded file content could contain triple backticks that prematurely close the fenced code block. Exploitation requires repo write access and the meta-prompt.md untrusted-input framing provides defense-in-depth.

• [edge-case] skills/pr-review/SKILL.md — The step 2b bash script passes file paths directly to the GitHub contents API without URL-encoding. Files with special URL characters (spaces, #, ?) would produce malformed API requests. The error is caught by the || { continue } block so the failure is graceful, but affected files would be silently omitted.

• [pattern-inconsistency] skills/pr-review/meta-prompt.md — The new constraint bullets mix imperative directives with explanatory rationale and conditional clauses. Other constraints in this file use terse imperatives like "Stay within your owned dimension" and "Do not write any files".

• [pattern-inconsistency] skills/pr-review/sub-agents/challenger.md — The new constraint is more verbose and conditional than the terse imperative style used by other constraints in this file.

Previous run (6)

Review

Findings

Medium

• [workflow-permission] skills/pr-review/SKILL.md:165 — The SAFE_FILE sanitization for GHA workflow commands strips literal newlines (tr -d '\n\r') and :: sequences (sed 's/:://g') but does not strip URL-encoded newlines (%0A, %0a, %0D, %0d). The established sanitize_gha() function in scripts/post-scribe.sh strips both patterns. A malicious filename containing %0A::set-env name=FOO::bar would survive the current sanitization because %0A is interpreted by the GHA log processor as a newline before :: stripping can act on the injected workflow command.
  Remediation: Add %0A/%0a/%0D/%0d stripping to the SAFE_FILE sanitization, consistent with sanitize_gha() in scripts/post-scribe.sh. For example: sed -e 's/:://g' -e 's/%0[aAdD]//g'.

• [protected-path] skills/pr-review/SKILL.md, skills/pr-review/meta-prompt.md, skills/pr-review/sub-agents/challenger.md — This PR modifies files under the protected skills/ path. The PR links to Review sub-agents should receive source file contents in prompt instead of re-reading from disk fullsend#1924 and provides rationale for the changes. Human approval is always required for protected-path changes, regardless of context.

Low

• [logic-error] skills/pr-review/SKILL.md:620 — Step 6d prose states the challenger "receives only the raw findings and the diff," but the template at lines 636–654 now also includes source files (line 645). The Part 3 description at lines 632–634 similarly omits source files from its enumeration even though the template includes a ### Source files (PR head) section. The template is authoritative so the risk of actual omission is low, but the prose should match.
  Remediation: Update line 620 to mention source files alongside findings and diff; update lines 632–634 to include source files in the Part 3 description.

• [injection-vuln] skills/pr-review/SKILL.md:175 — The raw FILE variable is interpolated into markdown output on the primary path (echo "#### ${FILE}") without the SAFE_FILE sanitization applied on the error paths (lines 165, 170). Additionally, base64-decoded file content could contain triple backticks that prematurely close the fenced code block. Exploitation requires repo write access and the meta-prompt.md untrusted-input framing provides defense-in-depth.

• [pattern-inconsistency] skills/pr-review/sub-agents/challenger.md:74 — The new constraint is more verbose and conditional than the terse imperative style used by other constraints in this file (e.g., "Do not add new findings — only adjudicate existing ones").

Previous run (7)

Review

Findings

High

• [logic-error] skills/pr-review/sub-agents/challenger.md:75 — The challenger sub-agent constraint is changed to "Use provided source files instead of reading from disk," but the challenger dispatch in SKILL.md step 6d does not include source_files in the challenger's context package. Step 6d Part 3 only includes: Findings to challenge, Diff, Changed files, and PR metadata — no "Source files (PR head)" section was added. Additionally, meta-prompt.md (included as Part 2 in the challenger dispatch) now instructs sub-agents to use the source files section, reinforcing a reference to content the challenger will never receive. The challenger's ability to verify findings against actual source code will be degraded.
  Remediation: Either (a) add a ### Source files (PR head) section to the challenger's context package template in step 6d Part 3 of SKILL.md, or (b) revert the challenger.md constraint change and keep the original "Read full source files, not just the diff hunks" wording so the challenger continues to read from disk.

Medium

• [edge-case] skills/pr-review/SKILL.md:160 — The bash snippet in step 2b outputs decoded file contents to stdout with no delimiter, file-path header, or code fence between files. All file contents are concatenated into an undifferentiated stream. However, step 3d specifies that source_files should have each file preceded by a #### <relative-path> header and wrapped in a fenced code block, and the step 4 template shows the expected format with headers and fences. The snippet does not produce output matching either specification.
  Remediation: Add file headers and code fence delimiters to the bash snippet output, or add a note that the orchestrator must format the raw output into the expected structure.

• [protected-path] skills/pr-review/SKILL.md — This PR modifies files under the protected skills/ path: skills/pr-review/SKILL.md, skills/pr-review/meta-prompt.md, skills/pr-review/sub-agents/challenger.md. The PR links to Review sub-agents should receive source file contents in prompt instead of re-reading from disk fullsend#1924 and provides rationale. Human approval is always required for protected-path changes, regardless of context.

Low

• [api-contract] skills/pr-review/SKILL.md:165 — The bash snippet uses base64 -d which is not portable to macOS (BSD base64 requires -D or --decode). The same pattern appears in the large-PR fallback instructions in step 4. While this is an LLM instruction and the primary deployment is Linux containers, using base64 --decode would be more portable.

• [api-contract] skills/pr-review/SKILL.md:149 — Step 2b states the GitHub contents API "returns 403 for files exceeding 1 MB." The API actually returns a 200 with an empty content field and encoding: "none" for files between 1–100 MB. A 403 is returned only above 100 MB. Checking for 403 would miss the 1–100 MB case where content is silently empty.

• [doc-style] skills/pr-review/meta-prompt.md:46 — The new constraint uses bold formatting (**Use the source files provided**) not present in other constraints in this file. Existing constraints use plain imperative text without bold.

[rh-hemartin]

Wasn't this already ported by @ben-alkov ?

[ben-alkov]

Wasn't this already ported by @ben-alkov ?

I don't think so... I can't find it if I did.

[rh-hemartin]

No worries then, my mistake.

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 11:25 AM UTC · Completed 11:42 AM UTC
Commit: bfad1f9 · View workflow run →

Superseded by updated review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:31 PM UTC · Completed 3:50 PM UTC
Commit: c7b640b · View workflow run →

[ben-alkov]

LGTM

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:45 AM UTC · Completed 10:55 AM UTC
Commit: c0bae51 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:57 AM UTC · Completed 11:05 AM UTC
Commit: e02264e · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 11:09 AM UTC · Completed 11:18 AM UTC
Commit: e02264e · View workflow run →

[fullsend-ai-retro]

Retro: PR #172 — pass source file contents to review sub-agents

PR: fullsend-ai/agents#172
Outcome: Merged after ~52 hours, 2 commits, 5 review bot runs, addressing 10 of 13 bot inline findings.

What went well

• The review bots (qodo-code-review and fullsend-ai-review) caught a high-severity logic error on the first review: the challenger sub-agent was told to use provided source files but never received them. This was a genuine correctness bug that would have broken the challenger for every PR.
• Both bots independently found the missing file delimiters issue, which would have produced an undifferentiated content stream incompatible with the context contract.
• The author (maruiz93) diligently addressed all findings with traceable commit references.
• Human reviewer (ben-alkov) approved after the substantive issues were fixed.

Improvement opportunities

Three proposals filed:

1. Sub-agent tool gap for large PRs — SKILL.md instructs sub-agents to fetch additional files via gh api for large PRs, but sub-agents only have Read/Grep/Glob tools (no Bash). This instruction is unexecutable.
2. Unresolved sanitization and prose issues from review — The review bot found a medium-severity SAFE_FILE sanitization gap (missing URL-encoded newline stripping) plus several low-severity issues that persisted through merge without being addressed.
3. Issue Review agent sub-agents re-read files already loaded by parent orchestrator #140 should be closed by PR perf(#1924): pass source file contents to review sub-agents #172 — Issue Review agent sub-agents re-read files already loaded by parent orchestrator #140 describes exactly the redundant sub-agent file reads that PR perf(#1924): pass source file contents to review sub-agents #172 fixes, but remains open with no cross-reference.

Proposals filed

• Review sub-agents cannot execute large-PR file-fetch fallback (no Bash tool) (in fullsend-ai/agents)
• fix(review): address unresolved review findings from PR #172 (sanitization gap, stale prose) (in fullsend-ai/agents)
• Close issue #140: sub-agent redundant file reads resolved by PR #172 (in fullsend-ai/agents)