fix(review): agent tool invocation is fundamentally wrong by ben-alkov · Pull Request #114 · fullsend-ai/agents

ben-alkov · 2026-07-10T19:15:39Z

Summary

Copies the scaffold-file portion of fullsend-ai/fullsend#2098 into this repo, at the paths that resulted from last week's split (internal/scaffold/fullsend-repo/** → repo root).

subagent_type and run_in_background were hallucinated fields — there's no such thing. Subagent restriction is via tools/disallowedTools; permissionMode: dontAsk blocks actions that would normally require approval.
Review agents should not modify the target repository; adds readonly_repo: true to harness/review.yaml.

The Go engine changes from the source PR (internal/cli/run.go, internal/harness/harness.go) that implement readonly_repo enforcement stay in fullsend-ai/fullsend — there's no Go code in this repo, and that PR remains open there for those changes.

harness/review.yaml diverged from fullsend-ai/fullsend's copy since the split (now uses forge.github blocks), so the readonly_repo: true line was added by hand rather than via patch; content is equivalent.

Test plan

Confirm readonly_repo: true is picked up correctly once fix(agents): There are no subagent "types"; they don't compose fullsend#2098's engine changes merge
Verify review sub-agent frontmatter (tools, permissionMode, background) matches current Claude Code docs

fullsend-ai-review · 2026-07-10T19:16:35Z

🤖 Review · ⚠️ Cancelled · Started 7:16 PM UTC · Ended 7:31 PM UTC
Commit: b8a817e · View workflow run →

qodo-code-review · 2026-07-10T19:16:55Z

PR Summary by Qodo

Fix review sub-agent invocation docs and enforce read-only review harness

🐞 Bug fix ✨ Enhancement ⚙️ Configuration changes 📝 Documentation 🕐 20-40 Minutes

AI Description

• Remove hallucinated Agent tool fields (subagent_type, run_in_background) from review docs.
• Standardize sub-agent frontmatter to use tools, permissionMode: dontAsk, and background.
• Make the review harness read-only (readonly_repo: true) to prevent repo mutation.

Diagram

graph TD
  A["harness/review.yaml"] --> B[Review harness]
  B --> C[Review agent] --> D["pr-review skill"] --> E[Spawn sub-agents] --> F[Synthesize findings]
  G["sub-agents/*.md"] --> E

High-Level Assessment

The following are alternative approaches to this PR:

1. Rely only on tool allow-lists (no `readonly_repo`)

➕ No engine/runtime support needed; works wherever tool filtering is enforced
➕ Keeps security posture entirely in agent definitions
➖ More brittle: a future config change could re-introduce write-capable tools
➖ Doesn’t protect against unexpected tool exposure or orchestration bugs

2. Centralize shared sub-agent frontmatter via templating

➕ Avoids repetition of tools/permissionMode/background across many files
➕ Reduces drift when updating agent configuration semantics
➖ Requires additional build/render step or conventions not currently present
➖ Less transparent for quick, manual edits in-repo

Recommendation: Keep the PR’s approach: document the real Agent tool controls (tools/disallowedTools, permissionMode, background) and add readonly_repo: true as a defense-in-depth guardrail. Tool allow-lists alone are helpful but easier to accidentally weaken; readonly_repo makes the intended “review must not mutate” invariant explicit at the harness level.

Files changed (10) +79 / -76

Documentation (2) +31 / -62

review.mdSimplify review orchestration docs and remove invalid sub-agent fields +6/-24

Simplify review orchestration docs and remove invalid sub-agent fields

• Removes detailed sub-agent dimension descriptions and deletes references to nonexistent dispatch fields like 'subagent_type'. Clarifies that 'pr-review' orchestrates triage/dispatch/synthesis without prescribing invalid Agent tool parameters.

agents/review.md

SKILL.mdCorrect sub-agent dispatch instructions and roster documentation +25/-38

Correct sub-agent dispatch instructions and roster documentation

• Updates the sub-agent roster/table to focus on dispatch mode and dimensions rather than model wiring. Removes references to hallucinated Agent tool fields ('subagent_type', 'run_in_background') and clarifies challenger sequencing vs parallel dimension sub-agents.

skills/pr-review/SKILL.md

Other (8) +48 / -14

review.yamlEnable read-only execution for the review harness +1/-0

Enable read-only execution for the review harness

• Adds 'readonly_repo: true' to prevent review runs from mutating the target repository. This enforces the intended non-destructive behavior for review agents.

harness/review.yaml

challenger.mdAlign challenger sub-agent frontmatter with supported Agent tool controls +7/-2

Align challenger sub-agent frontmatter with supported Agent tool controls
• Renames the sub-agent to 'challenger', expands description formatting, and adds explicit 'tools', 'permissionMode: dontAsk', and 'background: false'. This matches the intended “no writes, no prompts for approval” execution mode.
skills/pr-review/sub-agents/challenger.md

correctness.mdStandardize correctness sub-agent frontmatter (tools/permission/background) +7/-2

Standardize correctness sub-agent frontmatter (tools/permission/background)
• Renames the sub-agent to 'correctness', improves description formatting, and sets 'tools: Read, Grep, Glob' plus 'permissionMode: dontAsk' and 'background: true'. This removes reliance on invalid dispatch fields and keeps the agent read-only.
skills/pr-review/sub-agents/correctness.md

cross-repo-contracts.mdStandardize cross-repo-contracts sub-agent frontmatter +6/-2

Standardize cross-repo-contracts sub-agent frontmatter
• Renames the sub-agent to 'cross-repo-contracts', updates the description, and adds explicit 'tools', 'permissionMode: dontAsk', and 'background: true'. This documents the supported mechanism for restricting sub-agent capabilities.
skills/pr-review/sub-agents/cross-repo-contracts.md

docs-currency.mdStandardize docs-currency sub-agent frontmatter +6/-2

Standardize docs-currency sub-agent frontmatter
• Renames the sub-agent to 'docs-currency' and adds explicit 'tools', 'permissionMode: dontAsk', and 'background: true'. Keeps the sub-agent constrained to read/search operations during review.
skills/pr-review/sub-agents/docs-currency.md

intent-coherence.mdStandardize intent-coherence sub-agent frontmatter +7/-2

Standardize intent-coherence sub-agent frontmatter

• Renames the sub-agent to 'intent-coherence', refines the dimension description, and adds 'tools: Read, Grep, Glob' plus 'permissionMode: dontAsk' and 'background: true'. This aligns with current Agent tool configuration semantics.

skills/pr-review/sub-agents/intent-coherence.md

security.mdStandardize security sub-agent frontmatter +7/-2

Standardize security sub-agent frontmatter

• Renames the sub-agent to 'security', reformats its description, and adds explicit tool restrictions plus 'permissionMode: dontAsk' and 'background: true'. Removes any implication of unsupported sub-agent invocation parameters.

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

style-conventions.mdStandardize style-conventions sub-agent frontmatter +7/-2

Standardize style-conventions sub-agent frontmatter

• Renames the sub-agent to 'style-conventions', improves description formatting, and adds explicit 'tools', 'permissionMode: dontAsk', and 'background: true'. Ensures style review is performed in a read-only, non-interactive mode.

skills/pr-review/sub-agents/style-conventions.md

qodo-code-review · 2026-07-10T19:23:14Z

Code Review by Qodo

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

Context used

✅ Compliance rules (platform): 55 rules

✅ Skills: 4 invoked

code-review

code-implementation

pr-review

docs-review

1. ~~Git-history check lacks tools~~ ✗ Dismissed 🐞 Bug ☼ Reliability

Description

The correctness sub-agent is configured with tools: Read, Grep, Glob, but its own instructions
require checking git history of modified test files; without Bash/git access it cannot perform that
required step. This can reduce review integrity, and if the sub-agent errors/returns empty output,
the orchestrator procedure escalates that as a high-severity failure for Opus-tier sub-agents.

Code

skills/pr-review/sub-agents/correctness.md[R6-9]
model: opus
+tools: Read, Grep, Glob
+permissionMode: dontAsk
+background: true

Relevance

⭐⭐ Medium
No historical evidence for toolset mismatch vs git-history instruction in sub-agent configs.
ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The correctness sub-agent is explicitly configured without Bash, but the body explicitly requires
git-history inspection; the orchestrator procedure defines Opus-tier sub-agent missing output as a
high-severity failure.
skills/pr-review/sub-agents/correctness.md[1-10]
skills/pr-review/sub-agents/correctness.md[24-27]
skills/pr-review/SKILL.md[422-433]

Agent prompt

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

## Issue description
`skills/pr-review/sub-agents/correctness.md` instructs the sub-agent to check git history of modified test files, but the sub-agent’s frontmatter restricts tools to `Read, Grep, Glob` (no `Bash`/`git`). This is an internal contradiction: the sub-agent cannot execute the workflow it is told to perform.

Additionally, `skills/pr-review/SKILL.md` specifies that if an Opus-tier sub-agent (including `correctness`) fails to return findings (timeout/error/empty), the orchestrator must emit a **high-severity** `sub-agent-failure` finding, which can make reviews systematically fail if the sub-agent hits tool denials.

## Issue Context
- Correctness sub-agent requires git-history inspection for test-integrity checks.
- Correctness sub-agent tool allowlist does not include `Bash`.
- Orchestrator escalates Opus-tier sub-agent failures.

## Fix Focus Areas
- skills/pr-review/sub-agents/correctness.md[1-30]
- skills/pr-review/SKILL.md[422-433]

## Proposed fix direction
Pick one:
1) **Enable the required capability**: add a restricted Bash allowance to the correctness sub-agent (e.g., `tools: Read, Grep, Glob, Bash(git)` or equivalent) so it can run `git log`/`git blame` for modified test files.

2) **Remove/replace the requirement**: if sub-agents must remain `Read/Grep/Glob` only, edit the correctness sub-agent instructions to remove the git-history requirement (and adjust any orchestrator expectations accordingly), so the procedure is actually executable under the allowed toolset.

Ensure the final state has no “required steps” that are impossible under the declared tool allowlist.

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

2. readonly_repo not enforced 📜 Skill insight ≡ Correctness

Description

The PR introduces readonly_repo: true, but there is no in-repo consumer/enforcement of this flag,
so it may be a no-op at runtime and create a false sense of safety. Guard/contract flags must be
traceable from producer to consumer to verify they actually trigger.

Code

harness/review.yaml[7]
+readonly_repo: true

Relevance

⭐⭐ Medium
No precedent on “flag has no in-repo consumer”; PR text says enforcement is in fullsend repo.
ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The diff adds readonly_repo: true as a guard/contract flag, but repository search shows it only
exists in this config and has no in-repo consumer, so runtime functionality cannot be verified and
may not trigger.

harness/review.yaml[1-10]
Skill: pr-review
Skill: code-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
`readonly_repo: true` is introduced as a guard mechanism, but there is no evidence in this repository that anything reads/enforces it; this can silently fail to provide the intended protection.

## Issue Context
Compliance requires tracing guard/contract flags from where they are set to where they are read/enforced. If enforcement lives outside this repo, add a local enforcement mechanism or a hard-fail compatibility check so the flag cannot silently do nothing.

## Fix Focus Areas
- harness/review.yaml[1-10]

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

3. Config dropped on spawn 🐞 Bug ≡ Correctness

Description

pr-review now instructs composing sub-agent prompts from the markdown body after frontmatter and
spawning sub-agents using only the prompt, without specifying how to apply frontmatter fields like
model, tools, and background. This makes the new sub-agent frontmatter configuration likely
ineffective, so sub-agents may run with unintended model/tool/background settings.

Code

skills/pr-review/SKILL.md[R335-338]

+1. Compose the spawn prompt from:

   **Part 0 — Scope constraint (conditional):** If `scope_constraint`
   from step 3e is not `"none"`, prepend:

Relevance

⭐⭐ Medium
No historical evidence for sub-agent frontmatter being dropped/required in pr-review orchestrator.
ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The orchestrator instructions explicitly exclude frontmatter from the sub-agent prompt and no longer
describe extracting/passing any frontmatter-derived config into the Agent spawn, while sub-agent
files now declare execution config exclusively in frontmatter.
skills/pr-review/SKILL.md[331-351]
skills/pr-review/SKILL.md[396-398]
skills/pr-review/sub-agents/correctness.md[1-10]

Agent prompt

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

## Issue description
`skills/pr-review/SKILL.md` dispatch instructions currently (a) exclude sub-agent frontmatter from the composed prompt and (b) do not instruct extracting/passing frontmatter config (`model`, `tools`, `permissionMode`, `background`) into the Agent tool invocation. Meanwhile, sub-agent files now define those fields in frontmatter.

This creates a high-risk mismatch: the orchestrator procedure no longer clearly applies the intended per-sub-agent configuration.

## Issue Context
- The dispatch procedure explicitly says Part 1 is “everything after the frontmatter”, which would drop frontmatter unless the orchestrator separately parses and applies it.
- Sub-agent files now include significant execution config in frontmatter (`model`, `tools`, `permissionMode`, `background`).

## Fix Focus Areas
- skills/pr-review/SKILL.md[331-398]

## Proposed fix direction
Choose one clear, consistent approach and document it precisely:
1) **Parse and pass frontmatter explicitly**: update Step 4 to instruct reading each sub-agent file, extracting `model`, `tools`, `permissionMode`, and `background`, and passing those as parameters to the Agent tool call.

or

2) **Include frontmatter in the Agent prompt** (only if the runtime actually parses frontmatter from the provided prompt): update Step 4 so Part 1 includes the *entire* sub-agent markdown file, including frontmatter, and explicitly state that the Agent tool consumes it.

Whichever approach is correct for this repo’s runtime, ensure the SKILL describes it unambiguously so future edits don’t silently break dispatch configuration.

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

4. No linked authorizing issue 📜 Skill insight § Compliance

Description

This PR makes non-trivial, structural changes across multiple governance/config paths but does not
provide an explicit linked authorizing issue/ADR per the checklist requirement. Without an
authorizing issue link, the change lacks the required audit trail for approval.

Code

skills/pr-review/SKILL.md[R36-47]

+Sub-agent discovery: The sub-agents' definitions are in `sub-agents/`
+relative to this file.
+
+| Sub-agent              | Dispatch   | Dimensions                                                                                                              |
+|------------------------|------------|-------------------------------------------------------------------------------------------------------------------------|
+| `correctness`          | parallel   | Logic errors, edge cases, nil handling, API contracts, test adequacy/integrity                                          |
+| `security`             | parallel   | Security vulnerabilities, auth/access control, data exposure, injection defense, privilege escalation, content security |
+| `intent-coherence`     | parallel   | Architectural coherence & fit, design coherence, intent alignment, PR scope, scope authorization, tier matching         |
+| `style-conventions`    | parallel   | Repo-specific naming, error-handling idioms, API shape, code organization                                               |
+| `docs-currency`        | parallel   | Documentation staleness (follows docs-review skill inline)                                                              |
+| `cross-repo-contracts` | parallel   | API contract breakage affecting other repos (conditional)                                                               |
+| `challenger`           | sequential | Adversarial challenge of findings, false-positive removal, deduplication                                                |

Relevance

⭐⭐ Medium
Governance-authorization suggestions appear, but acceptance not evidenced (PR #29).
PR-#29

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The compliance checklist requires explicit authorization via a linked issue for non-trivial changes;
the diff demonstrates substantial, structural changes to the PR review orchestrator
documentation/config surfaces.

skills/pr-review/SKILL.md[11-48]
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
Non-trivial changes require an explicit linked authorizing issue/ADR for auditability.

## Issue Context
If the authorization lives outside this repo (e.g., upstream work), create/link a tracking issue or ADR in this repo and reference it in the PR description.

## Fix Focus Areas
- skills/pr-review/SKILL.md[34-48]

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

5. Protected paths modified 📜 Skill insight § Compliance

Description

This PR modifies protected governance/infrastructure paths (agents/, harness/, skills/), which
must not be auto-approved and requires explicit human review. These changes should be explicitly
called out for governance review even if the intent is valid.

Code

harness/review.yaml[7]
+readonly_repo: true

Relevance

⭐⭐ Medium
Similar protected-path review suggestions exist but outcomes unclear (PR #59, #29).
PR-#59
PR-#29

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The compliance checklist requires raising a finding whenever protected governance/infrastructure
paths are modified. The diff shows changes under harness/, agents/, and skills/.
harness/review.yaml[1-10]
agents/review.md[74-100]
skills/pr-review/sub-agents/correctness.md[1-10]
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 in this PR, which requires explicit human review and should not be implicitly treated as low-risk.

## Issue Context
This rule is about ensuring changes under protected directories receive the right review/approval posture (e.g., CODEOWNERS, governance reviewers, or an explicit authorization link).

## Fix Focus Areas
- harness/review.yaml[1-10]
- agents/review.md[74-100]
- skills/pr-review/sub-agents/correctness.md[1-10]

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

There is no such thing as "subagent_type" or "run_in_background" - both are hallucinated fields. Subagent restriction is via tools/disallowedTools, and permissionMode: dontAsk blocks actions that would normally require approval. Assisted-by: Claude Code (Sonnet 5) Signed-off-by: Ben Alkov <ben.alkov@redhat.com>

Review agents should not modify the target repository. Assisted-by: Claude Code (Sonnet 5) Signed-off-by: Ben Alkov <ben.alkov@redhat.com>

fullsend-ai-review · 2026-07-10T19:32:25Z

🤖 Finished Review · ✅ Success · Started 7:32 PM UTC · Completed 7:52 PM UTC
Commit: 35162f8 · View workflow run →

fullsend-ai-review · 2026-07-10T19:52:16Z

Review

Findings

High

[protected-path] agents/review.md — All 10 files in this PR are under protected paths (agents/, harness/, skills/): agents/review.md, harness/review.yaml, skills/pr-review/SKILL.md, and 7 files under skills/pr-review/sub-agents/. The PR has no linked issue in this repository. While the PR body provides detailed technical rationale and references upstream PR fix(agents): There are no subagent "types"; they don't compose fullsend#2098, human approval is always required for protected-path changes, and the authorization chain requires a formally linked issue.
Remediation: Link an issue in this repository that authorizes the changes to protected paths, or obtain explicit human approval.

Low

[api-contract] skills/pr-review/SKILL.md — The dispatch instructions in step 4 and section 6d no longer specify how the model parameter reaches the Agent tool when spawning sub-agents. The old instructions explicitly extracted model from sub-agent frontmatter and passed it as an Agent tool parameter. The new instructions say only "Spawn the subagents with their prompt argument." Since the PR adds tools, permissionMode, and background to frontmatter without corresponding dispatch instructions, these fields are likely auto-consumed by the engine — and model presumably works the same way. A brief clarifying note would remove the ambiguity.
[stale-reference] agents/review.md — The Identity section no longer enumerates the six review dimensions or describes sub-agent frontmatter format. The Skill routing section no longer mentions where sub-agent definitions live. This information was redundant (SKILL.md and sub-agent files are the source of truth), so the reduction is not a logic error, but it reduces the agent definition's self-contained context.

fullsend-ai-review

See the review comment for full details.

ben-alkov · 2026-07-10T19:53:43Z

@rh-hemartin; It's like déjà vu all over again 😁. Same diff as fullsend-ai/fullsend/pull/2098

ben-alkov requested a review from a team as a code owner July 10, 2026 19:15

qodo-code-review Bot reviewed Jul 10, 2026

View reviewed changes

Comment thread harness/review.yaml

Comment thread harness/review.yaml

Comment thread skills/pr-review/SKILL.md

Comment thread skills/pr-review/SKILL.md

Comment thread skills/pr-review/sub-agents/correctness.md

ben-alkov added 2 commits July 10, 2026 15:29

feat(review): harden review agent with read-only repo access

35162f8

Review agents should not modify the target repository. Assisted-by: Claude Code (Sonnet 5) Signed-off-by: Ben Alkov <ben.alkov@redhat.com>

ben-alkov force-pushed the copy-pr-2098 branch from dbe1681 to 35162f8 Compare July 10, 2026 19:31

ben-alkov requested a review from rh-hemartin July 10, 2026 19:51

fullsend-ai-retro Bot mentioned this pull request Jul 10, 2026

Review agent dispatch instructions are underspecified after removing hallucinated Agent tool parameters #122

Open

fullsend-ai-review Bot suggested changes Jul 10, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(review): agent tool invocation is fundamentally wrong#114

fix(review): agent tool invocation is fundamentally wrong#114
ben-alkov wants to merge 2 commits into
mainfrom
copy-pr-2098

ben-alkov commented Jul 10, 2026

Uh oh!

fullsend-ai-review Bot commented Jul 10, 2026 •

edited

Loading

Uh oh!

qodo-code-review Bot commented Jul 10, 2026

Uh oh!

qodo-code-review Bot commented Jul 10, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fullsend-ai-review Bot commented Jul 10, 2026 •

edited

Loading

Uh oh!

fullsend-ai-review Bot commented Jul 10, 2026

Uh oh!

fullsend-ai-review Bot left a comment

Uh oh!

ben-alkov commented Jul 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

ben-alkov commented Jul 10, 2026

Summary

Test plan

Uh oh!

fullsend-ai-review Bot commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qodo-code-review Bot commented Jul 10, 2026

PR Summary by Qodo

Uh oh!

qodo-code-review Bot commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fullsend-ai-review Bot commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fullsend-ai-review Bot commented Jul 10, 2026

Review

Findings

High

Low

Uh oh!

fullsend-ai-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

ben-alkov commented Jul 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

fullsend-ai-review Bot commented Jul 10, 2026 •

edited

Loading

qodo-code-review Bot commented Jul 10, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jul 10, 2026 •

edited

Loading