Skip to content

fix: trim security heuristics to policy directives#2046

Merged
ben-alkov merged 2 commits into
mainfrom
fix/trim-security-heuristics
Jun 11, 2026
Merged

fix: trim security heuristics to policy directives#2046
ben-alkov merged 2 commits into
mainfrom
fix/trim-security-heuristics

Conversation

@ralphbean

Copy link
Copy Markdown
Member

Summary

  • Trim security sub-agent heuristics from feat(#898): add security heuristics to review sub-agent #2038 to policy directives only, removing domain-knowledge explanations Claude already knows (fail-open definitions, least-privilege explanations)
  • Merge the redundant "Permission manifest changes" and "Workflow permission and role auditing" sections into a single "Permission and role changes" section
  • Deduplicate the permission file-type list between the Own block and the dedicated section

These commits were pushed to #2038's branch but the merge queue picked up the prior head before they landed.

Toward #898

Test plan

  • make lint passes
  • CI passes (markdown-only change, no Go code affected)

@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown

Site preview

Preview: https://ab67c02e-site.fullsend-ai.workers.dev

Commit: 51c0f75a7fcd9648e9f1a0749c046153b6bd6e7b

@fullsend-ai-review

Copy link
Copy Markdown

🤖 Review · Started 8:22 PM UTC
Commit: d0ac11b · View workflow run →

@ralphbean ralphbean closed this Jun 8, 2026
@fullsend-ai-retro

Copy link
Copy Markdown

🤖 Retro · Started 8:25 PM UTC
Commit: d0ac11b · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #2046fix: trim security heuristics to policy directives

What happened: PR #2038 (by fullsend-ai-coder[bot]) added security heuristics to the review sub-agent to address issue #898. The code agent produced verbose heuristics that re-explained security concepts (fail-open definitions, least-privilege explanations) that Claude already knows. A human (ralphbean) had to open PR #2046 to trim the output down to just policy directives — what to flag and at what severity — and to merge redundant sections. PR #2046 was opened and closed within 3 minutes without merging (the changes were likely applied via another path).

The review agent started a run on PR #2046 but was wasted since the PR was closed ~90 seconds later. This is already tracked by issues #1870 and #1439.

One new proposal is included below, addressing the code agent's tendency to over-explain domain knowledge when writing agent prompts, which caused human rework.

Skipped proposals (already tracked):

Proposals filed

@fullsend-ai-retro

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 8:25 PM UTC · Completed 8:30 PM UTC
Commit: d0ac11b · View workflow run →

@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 8:22 PM UTC · Completed 8:32 PM UTC
Commit: d0ac11b · View workflow run →

@ben-alkov ben-alkov reopened this Jun 11, 2026
@github-actions

github-actions Bot commented Jun 11, 2026

Copy link
Copy Markdown

E2E tests are running

Authorization passed for this commit. See the E2E Tests workflow for results.

@codecov

codecov Bot commented Jun 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

The heuristics were re-teaching security concepts (fail-open, least
privilege) that Claude already knows. Trim to policy directives and
severity thresholds — what to always flag and at what severity — which
is the novel content the model needs.

Also merge the redundant "Permission manifest changes" and "Workflow
permission and role auditing" sections into a single "Permission and
role changes" section, eliminating duplicate permissions: block
guidance that could cause the sub-agent to produce duplicate findings.

Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
The parenthetical enumeration in the Own block duplicated the
canonical list in the "Permission and role changes" section.
Keep one list to prevent drift.

Assisted-by: Claude claude-opus-4-6 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
@ben-alkov ben-alkov force-pushed the fix/trim-security-heuristics branch from af94b99 to 51c0f75 Compare June 11, 2026 16:25
@ben-alkov

Copy link
Copy Markdown
Member

/fs-review

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 11, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 4:29 PM UTC · Completed 4:39 PM UTC
Commit: 6bfb10b · View workflow run →

@ben-alkov ben-alkov 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.

Good stuff

  • Dedupe and eliminate redundancy
  • Remove the parenthetical file-type list from the "Own:" paragraph; it's misplaced in this section
  • Provide policy thresholds and severity assignments without re-teaching the concept

@fullsend-ai-review

Copy link
Copy Markdown

Review

Findings

Medium

  • [stale-reference] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md:198 — The diff removes the workflow-permission category from security.md by deleting the "Workflow permission and role auditing" section and merging it into "Permission and role changes" (which lists permission-expansion, permission-reduction, role-escalation, secret-exposure). However, SKILL.md line 198 still includes workflow-permission in the security dimension's category routing table. This creates a cross-file inconsistency: the orchestrator will route prior workflow-permission findings to the security sub-agent for re-review anchoring, but the sub-agent will never produce new findings with that category.
    Remediation: Remove workflow-permission from the security categories list in SKILL.md, or re-add it to the merged section in security.md.

Low

  • [fail-open] internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security.md:14 — The "Own" scope line removed the parenthetical examples (GitHub App manifests, workflow permissions blocks, IAM policies, OAuth scopes) that previously disambiguated "permission manifest changes." The merged "Permission and role changes" section still enumerates all these file types, so the operative detection instructions are intact — but the scope declaration is now vaguer than its subsections.

Info

  • [permission-reduction] internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security.md:95 — The workflow-permission finding category was dropped when merging the two permission sections. Workflow permissions: block changes will now be categorized under permission-expansion or permission-reduction. This is a taxonomy simplification, not a security gap.

  • [design-direction] internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security.md — The condensation brings security.md closer to the length of other sub-agents (~120 lines vs correctness at 75, intent-coherence at 76). The tradeoff is whether the condensed policy directives maintain the same finding quality as the verbose checklists. The proof point will be empirical — whether the condensed version catches the same findings on future security-relevant PRs.

@fullsend-ai-review fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Jun 11, 2026
@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 11, 2026
@ben-alkov ben-alkov added this pull request to the merge queue Jun 11, 2026
Merged via the queue into main with commit a029a60 Jun 11, 2026
16 checks passed
@ben-alkov ben-alkov deleted the fix/trim-security-heuristics branch June 11, 2026 17:35
@fullsend-ai-retro

fullsend-ai-retro Bot commented Jun 11, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 5:39 PM UTC · Completed 5:47 PM UTC
Commit: 51c0f75 · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #2046fix: trim security heuristics to policy directives

Context: PR #2038 (code agent) added verbose security heuristics to the review sub-agent for issue #898. A human opened PR #2046 to trim those heuristics to concise policy directives, removing domain-knowledge explanations Claude already knows, and merging redundant sections. The PR was needed because the merge queue on #2038 picked up the prior HEAD before the human's cleanup commits landed.

Workflow observations:

  1. Retro raced review on PR open. When PR fix: trim security heuristics to policy directives #2046 was created, both the review agent (20:22 UTC) and retro agent (20:25 UTC) dispatched nearly simultaneously. The retro completed at 20:30 — two minutes before the review finished at 20:32. This means the retro analyzed the PR without access to the review's findings. The retro filed issue Code agent over-explains domain knowledge in agent prompt files, causing rework #2047 based solely on the PR description and parent PR context, missing the review's later stale-reference finding.

  2. Valid medium finding merged unresolved. The second review (triggered by /fs-review on 2026-06-11) found a genuine cross-file inconsistency: SKILL.md:198 still references a workflow-permission category that was removed from security.md. Verified as accurate. The PR was approved and merged without addressing this. Already tracked by Auto-file tracking issues for unresolved medium+ severity review findings #1956 and Review agent medium findings should trigger fix agent or block merge #870.

  3. Code agent verbosity causing rework. Already tracked by Code agent over-explains domain knowledge in agent prompt files, causing rework #2047 (filed by the first retro run on this same PR).

  4. Duplicate retro runs. This is the second retro on PR fix: trim security heuristics to policy directives #2046 — the first ran on PR open (2026-06-08), this one on merge (2026-06-11). Partially overlaps with existing skip-guard issues (Skip retro dispatch on PRs with no agent involvement #939, Refine retro dispatch guard in issue #939 to account for agent involvement on non-agent branches #942, Skip retro dispatch for unmerged PRs with no agent interaction #1411, Extend retro skip guard to human-authored PRs with no-finding reviews #1675) but none specifically address the retro-before-review race.

One new proposal below addresses the retro→review sequencing gap. Other findings are already tracked by existing issues.

Proposals filed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Allow e2e CI to run after maintainer review (must be re-applied after each push) requires-manual-review Review requires human judgment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants