docs: sandbox layout, skill precedence, and agent rule layering#2584
Conversation
PR Summary by QodoDocs: clarify sandbox layout and skill/agent rule precedence Description
Diagram
High-Level Assessment
Files changed (3)
|
|
🤖 Review · |
Code Review by Qodo
Context used✅ Tickets:
🎫 Publish scaffold skills and sub-agent definitions as a standalone Claude Code plugin 🎫 Skills loading policy: explicit harness list vs. org+repo union with scanning 🎫 Define protected vs. freely overridable harness fields at org/repo inheritance layers✅ Compliance rules (platform):
51 rules✅ Skills:
writing-user-docs, writing-adrs 1. Misstated security pipeline
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| All repo context files (CLAUDE.md, AGENTS.md, SKILL.md) are scanned for | ||
| prompt injection via `scanRepoContextFiles()` before the agent starts. The | ||
| scan uses the `InputPipeline` (context injection scanner, SSRF validator, | ||
| secret redactor). Critical findings block the run in `fail_mode: closed`. |
There was a problem hiding this comment.
3. Misstated security pipeline 🐞 Bug ⛨ Security
cli-internals.md states scanRepoContextFiles() uses InputPipeline including SSRF validation and secret redaction, but InputPipeline() is only unicode normalization plus prompt-injection scanning. This can create false confidence about what security checks actually run over repo context files.
Agent Prompt
## Issue description
Docs claim the repo context scan uses an `InputPipeline` that includes SSRF validation and secret redaction. In code, `security.InputPipeline()` only runs Unicode normalization and the context injection scanner; SSRF validation is not part of that pipeline, and secret redaction is an output concern.
## Issue Context
`run.go`’s host-side `scanRepoContextFiles()` uses `security.InputPipeline()` directly, so the docs should match that behavior. If SSRF/secret redaction are desired here, that’s a code change; otherwise, fix the docs text.
## Fix Focus Areas
- docs/guides/dev/cli-internals.md[468-474]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
waynesun09
force-pushed
the
docs-sandbox-skill-layering
branch
from
e2b29da to
6572f48
Compare
|
🤖 Review · |
- customizing-with-skills.md: fix skill overloading section to distinguish extend (.claude/skills/) from override (customized/skills/), add skill precedence explanation (personal > project) - customizing-with-agents-md.md: add section on how AGENTS.md composes with agent definitions (different layers, not competing) - runtimes.md: add sandbox workspace layout diagram, agent rule layering diagram, AGENTS.md injection logic, and context file security scanning - cli-internals.md: add cross-reference to runtimes.md for sandbox details Assisted-by: Claude (fix), Gemini (review), Codex (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
waynesun09
force-pushed
the
docs-sandbox-skill-layering
branch
from
6572f48 to
c4ee5f0
Compare
|
🤖 Finished Review · ✅ Success · Started 11:38 PM UTC · Completed 11:53 PM UTC |
ReviewFindingsMedium
Labels: PR modifies only documentation files covering sandbox layout, skill precedence, and agent rule layering. |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| these by providing your own skill with the same name. Your version replaces | ||
| the built-in one at runtime — no other configuration needed. | ||
| Skills you add to your repository are available to all fullsend agents | ||
| alongside the built-in skills. This is the primary way to give agents |
There was a problem hiding this comment.
[medium] architectural-misalignment
The PR reverses the documented skill precedence model: the old text says repo skills with the same name as built-in skills replace the built-in; the new text says built-in skills take precedence and repo skills are silently ignored. ADR 0035 describes the customized/ overlay mechanism (which the new docs preserve correctly), but the claim about project-level .claude/skills/ being shadowed by personal-level CLAUDE_CONFIG_DIR/skills/ needs verification against Claude Code actual skill resolution behavior.
Suggested fix: Verify Claude Code actual skill precedence (personal > project). If confirmed, add a brief note explaining that this precedence is a Claude Code runtime behavior, distinct from the ADR 0035 overlay mechanism.
| Repo skills **extend** the agent's skill set. They do not replace built-in | ||
| skills. If a repo skill has the same name as a built-in skill, the built-in | ||
| version takes precedence and the repo version is silently ignored. Use a | ||
| unique name to ensure your skill is discoverable. |
There was a problem hiding this comment.
If this precedence behavior is accurate, i.e. repo skills with colliding names are silently ignored, then I feel like this should also track a follow-up to log a warning when a repo skill is shadowed, otherwise users have no way to know their skill isn't running.
|
🤖 Finished Retro · ✅ Success · Started 2:54 PM UTC · Completed 3:04 PM UTC |
Retro: PR #2584 — docs: sandbox layout, skill precedence, and agent rule layeringTimeline:
Review agent accuracy: 2/4 findings valid.
The valid stale-reference findings were not addressed before merge but are tracked in #2662. Existing issues cover most improvement vectors:
One novel proposal below addresses the root cause of the two false positives: the review agent treating existing (incorrect) documentation as ground truth rather than verifying against source code. Proposals filed
|
3 participants
Summary
customized/skills/(ADR-0035 overlay). A repo.claude/skills/with a colliding name is silently shadowed by fullsend's personal-level version. Separates into "Extending agents with repo skills" (additive) and "Overriding built-in skills" (via customized/) with skill precedence explanation./sandbox/claude-config/(personal level) vs/sandbox/workspace/<repo>/(project level), agent rule layering diagram (3 layers: agent definition > project instructions > skills), and AGENTS.md injection logic.Related issues: #237, #236, #2135, #1901
Test plan
internal/runtime/claude.goBootstrap() andinternal/cli/run.gorunAgent()internal/sandbox/sandbox.go