Restore old system prompt behavior around subagents#7576
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cdb7617c3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let skills: Vec<&Source> = sources | ||
| .iter() | ||
| .filter(|s| s.kind == SourceKind::Skill || s.kind == SourceKind::BuiltinSkill) | ||
| .collect(); |
There was a problem hiding this comment.
The new instruction builder takes sources as-is and immediately formats skill bullets, but those sources come from read_dir traversal whose entry order is not guaranteed; with multiple skills, the summon instruction text can reorder between runs. Because this text is embedded in the system prompt, that introduces avoidable prompt nondeterminism and cache misses. Please sort the selected skills (for example by name) before appending them to instructions.
Useful? React with 👍 / 👎.
| } else { | ||
| None |
There was a problem hiding this comment.
This branch drops summon instructions entirely when no session object is present, so the extension contributes no usage guidance to the system prompt. That can happen on runtime enable flows that call ExtensionManager::add_extension without a session id (for example from platform_extensions/ext_manager.rs), which means enabling summon in-session can silently produce an empty summon instructions section. Keeping a default summon instruction string and only appending dynamic skill text when a session exists avoids that regression.
Useful? React with 👍 / 👎.
DOsinga
left a comment
DOsinga
left a comment
There was a problem hiding this comment.
I think we could add a test to verify that we actually find the skills, but that can be done later.
|
|
||
|
|
||
| You have these skills at your disposal, when it is clear they can help you solve a problem or you are asked to use them: | ||
| • goose-doc-guide - Reference goose documentation to create, configure, or explain goose-specific features like recipes, extensions, sessions, and providers. You MUST fetch relevant goose docs before answering. You MUST NOT rely on training data or assumptions for any goose-specific fields, values, names, syntax, or commands. |
There was a problem hiding this comment.
not for now, but maybe where this is generated we can throw in some skills so they actually show up here. hmm, actually, actually, should we even show this if there are no skills?
There was a problem hiding this comment.
this goose-doc-guide is a skill that currently always appears, because it's built into the binary
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e432b708d4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| instructions.push_str("\n\nYou have these skills at your disposal, when it is clear they can help you solve a problem or you are asked to use them:"); | ||
| for skill in &skills { | ||
| instructions.push_str(&format!("\n• {} - {}", skill.name, skill.description)); |
There was a problem hiding this comment.
This constructor now builds InitializeResult.instructions by interpolating skill.description values from discovered SKILL.md files, which includes session working-directory skill folders, so a malicious repository can inject arbitrary prompt text at system-instruction priority as soon as summon is enabled. That is a behavior regression from the previous static summon guidance and can silently steer model behavior before any explicit load/delegate call; treat filesystem skill metadata as untrusted and avoid embedding it directly in system instructions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a good point, but I think in general we trust the working directory in goose. WDYT @DOsinga @alexhancock ?
There was a problem hiding this comment.
I think this is how skills are supposed to work - you install an evil skill and your agent becomes evil. YOLO all the way
| if !skills.is_empty() { | ||
| instructions.push_str("\n\nYou have these skills at your disposal, when it is clear they can help you solve a problem or you are asked to use them:"); | ||
| for skill in &skills { | ||
| instructions.push_str(&format!("\n• {} - {}", skill.name, skill.description)); |
There was a problem hiding this comment.
Should we put a limit on the max number of skills in the description? The skill monorepos can have dozens or hundreds
…m-extension-pr * origin/main: Restore old system prompt behavior around subagents (#7576)
PlatformExtensionContext gained a session field from #7576. Add session: None to the test helper. Update snapshot.
PlatformExtensionContext gained a session field from #7576. Add session: None to the test helper. Update snapshot.
3 participants
with @alexhancock