feat(#1309): add mandatory finding for agent behavior definition changes - #2133
feat(#1309): add mandatory finding for agent behavior definition changes#2133fullsend-ai-coder[bot] wants to merge 3 commits into
Conversation
Site previewPreview: https://a49a1013-site.fullsend-ai.workers.dev Commit: |
|
🤖 Finished Review · ✅ Success · Started 5:13 PM UTC · Completed 5:22 PM UTC |
ReviewFindingsMedium
Previous runReviewFindingsMedium
Low
Info
Previous run (2)ReviewFindingsMedium
Low
Info
Labels: PR modifies a scaffolded skill definition to address a security governance gap in agent self-review. Previous run (3)ReviewFindingsMedium
Low
Info
Previous run (4)ReviewFindingsLow
Info
Previous run (5)ReviewFindingsLow
Info
Previous run (6)ReviewFindingsLow
Info
|
| Examples of permission-declaring files: GitHub App manifest JSON, | ||
| `permissions:` blocks in `.github/workflows/*.yml`, token scoping | ||
| maps, IAM policy JSON/YAML, Kubernetes `Role`/`ClusterRole` YAML. | ||
| - **Agent behavior definition changes:** If the diff modifies any file |
There was a problem hiding this comment.
[low] logic-error
The security sub-agent Own section does not explicitly list agent behavior definition files. Adding them would improve clarity and consistency with the mandatory-finding directive in SKILL.md.
d29ca18 to
2506234
Compare
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
|
/fs-fix |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
🤖 Review · ❌ Terminated · Started 6:22 PM UTC · Ended 6:32 PM UTC |
|
🤖 Finished Fix · ✅ Success · Started 6:22 PM UTC · Completed 6:29 PM UTC |
🔧 Fix agent — iteration 1 (human-triggered)No fixes applied. The bot approved with no findings, the human provided no specific instruction, and the code correctly implements issue #1309 requirements. All CI checks pass. Disagreed (1):
Tests: passed Updated by fullsend fix agent |
| modifications warrant careful human review because agents have a | ||
| structural blind spot when evaluating changes to their own | ||
| instructions. Severity: **info** for additive changes (new rules, new |
There was a problem hiding this comment.
Why is there a structural blindspot here?
|
🤖 Finished Review · ✅ Success · Started 6:22 PM UTC · Completed 6:32 PM UTC |
|
/fs-fix Take care of the "low" findings |
|
🤖 Finished Fix · ✅ Success · Started 8:00 PM UTC · Completed 8:06 PM UTC |
|
I'm wondering if these file patterns are too specific to fullsend-developing-fullsend. Another team using fullsend might not have Should these patterns be configurable? Something like a If we went that way, the rule in SKILL.md would say "check |
|
Following up on the configurability question — what's the relationship between this list and the "protected paths" list in pr-review? From a user's perspective, are these two separate lists that produce different behavior, or are they essentially the same thing? If they do the same thing (flag files for human attention), we might be asking users to maintain two lists that serve one purpose. If they're meaningfully different, we should be explicit about how — otherwise it'll be confusing to configure. |
|
Put another way: the small version of this might just be adding these patterns to the existing protected paths list — no new rule needed. The large version might be making the protected paths list itself configurable, so we aren't hardcoding paths that only matter for fullsend-developing-fullsend. |
Add explicit finding category `agent-behavior-definition` to the agent behavior definition changes rule so the pr-review routing table can correctly route and anchor these findings to the security dimension. Fix formatting consistency: **Medium** → **medium** to match the canonical lowercase severity convention used elsewhere in SKILL.md. Addresses review feedback on #2133
🔧 Fix agent — iteration 1 (human-triggered)Addressed both low review findings: added explicit finding category Fixed (2):
Tests: passed Updated by fullsend fix agent |
|
🤖 Finished Review · ✅ Success · Started 8:10 PM UTC · Completed 8:21 PM UTC |
Add explicit finding category `agent-behavior-definition` to the agent behavior definition changes rule so the pr-review routing table can correctly route and anchor these findings to the security dimension. Fix formatting consistency: **Medium** → **medium** to match the canonical lowercase severity convention used elsewhere in SKILL.md. Addresses review feedback on #2133
44f9eb3 to
447dba2
Compare
|
🤖 Finished Review · ✅ Success · Started 12:48 PM UTC · Completed 12:58 PM UTC |
| @@ -146,6 +146,17 @@ dimension carry over to another — each requires its own scrutiny. | |||
| Examples of permission-declaring files: GitHub App manifest JSON, | |||
| `permissions:` blocks in `.github/workflows/*.yml`, token scoping | |||
| maps, IAM policy JSON/YAML, Kubernetes `Role`/`ClusterRole` YAML. | |||
There was a problem hiding this comment.
[medium] logic-error
The new rule introduces category agent-behavior-definition, but this category is absent from the pr-review routing table (step 3a in skills/pr-review/SKILL.md). The fallback rule routes unrecognized categories to the nearest matching dimension by keyword, or to correctness as a fallback. Since agent-behavior-definition does not keyword-match any dimension name, prior findings with this category will be misrouted to correctness during re-reviews instead of security. Additionally, the security sub-agent definition (sub-agents/security.md) does not receive code-review/SKILL.md content during pr-review orchestration — sub-agents receive only their own definition file, the meta-prompt, and the context package. Therefore, the security sub-agent will not see the mandatory-finding rule during pr-review orchestration and will not produce agent-behavior-definition findings. The rule works correctly when code-review is invoked standalone, but during pr-review orchestration there are two gaps: (1) the security sub-agent lacks instructions to produce agent-behavior-definition findings, and (2) prior findings with this category will route to correctness instead of security.
Suggested fix: Add agent-behavior-definition to the security row in the pr-review routing table at skills/pr-review/SKILL.md. Also add agent behavior definition file coverage to the security sub-agent's Own section in sub-agents/security.md.
Add explicit finding category `agent-behavior-definition` to the agent behavior definition changes rule so the pr-review routing table can correctly route and anchor these findings to the security dimension. Fix formatting consistency: **Medium** → **medium** to match the canonical lowercase severity convention used elsewhere in SKILL.md. Addresses review feedback on #2133
447dba2 to
7aad081
Compare
Add a new Platform Security rule to the code-review skill that instructs the review agent to always produce a finding when a diff modifies files that define agent behavior. This closes a governance gap where the review agent could silently approve changes to its own skill definitions, agent definitions, harness configs, or policy files without flagging them for human review. The rule covers four file patterns: skills/*/SKILL.md, agents/*.md, harness/*.yaml, and policies/*.yaml. Severity is info for additive changes and medium for modifications that relax existing checks or remove constraints. Note: make lint could not run due to Go module cache permission errors in the sandbox (infrastructure issue, not code-related). This is a Markdown-only change with no Go code modifications. Closes #1309
Add explicit finding category `agent-behavior-definition` to the agent behavior definition changes rule so the pr-review routing table can correctly route and anchor these findings to the security dimension. Fix formatting consistency: **Medium** → **medium** to match the canonical lowercase severity convention used elsewhere in SKILL.md. Addresses review feedback on #2133
7aad081 to
07cd5f0
Compare
|
/fs-fix |
|
🤖 Finished Fix · ✅ Success · Started 8:12 PM UTC · Completed 8:17 PM UTC |
Add `agent-behavior-definition` to the security row in the pr-review routing table so findings with this category are routed to the security sub-agent during re-reviews instead of falling back to correctness. Add agent behavior definition file coverage to the security sub-agent's Own section so it knows to produce these findings during pr-review orchestration. Addresses review feedback on #2133
🔧 Fix agent — iteration 2 (human-triggered)Addressed both parts of the medium-severity logic-error finding: (1) added agent-behavior-definition to the security routing table in pr-review/SKILL.md, and (2) added agent behavior definition file coverage to the security sub-agent's Own section in sub-agents/security.md. This closes both gaps identified by the reviewer. Fixed (2):
Tests: passed Updated by fullsend fix agent |
ralphbean
left a comment
There was a problem hiding this comment.
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.
|
Will move to agents repo |
|
Superseded by fullsend-ai/agents#445 |
Add a new Platform Security rule to the code-review skill that instructs the review agent to always produce a finding when a diff modifies files that define agent behavior. This closes a governance gap where the review agent could silently approve changes to its own skill definitions, agent definitions, harness configs, or policy files without flagging them for human review.
The rule covers four file patterns: skills//SKILL.md, agents/.md, harness/.yaml, and policies/.yaml. Severity is info for additive changes and medium for modifications that relax existing checks or remove constraints.
Note: make lint could not run due to Go module cache permission errors in the sandbox (infrastructure issue, not code-related). This is a Markdown-only change with no Go code modifications.
Closes #1309
Post-script verification
agent/1309-agent-behavior-review-rule)eda1e12306a7a4393c077260bcd5acde491b8649..HEAD)