fix: contain and bound skill supporting file reads - #11342
Conversation
Signed-off-by: Jasper Hugo <jasper@spiral.xyz>
Signed-off-by: Jasper Hugo <jasper@spiral.xyz>
![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: 50e3037812
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Jasper Hugo <jasper@spiral.xyz>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76216b95a7
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Jasper Hugo <jasper@spiral.xyz>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c412b8f18
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Jasper Hugo <jasper@spiral.xyz>
|
This currently protects only supporting files loaded through |
michaelneale
left a comment
michaelneale
left a comment
There was a problem hiding this comment.
I think makes sense. Small chance this may surprise some users of skills but the way to resolve that is to put things in sensible places! nice.
* origin/main: (59 commits) chore(deps): bump the ui-minor-and-patch group across 1 directory with 53 updates (#11386) fix(security): bound call graph traversal (#11193) fix: pin arrayref to known-good commit (#11389) feat(providers): add SayGM as declarative OpenAI-compatible provider (#11267) feat(providers): custom provider cost fields drive cost tracking (config-declared pricing fallback) (#11220) fix(deps): repair dangling syn reference in Cargo.lock (#11385) fix(flake): add cudaforge hash for git dependency (#10910) feat: auto-focus chat input when user starts typing (#11184) fix(security): fail closed on invalid Codex ACP mode (#11362) fix(mcp): keep stdio extensions alive across worker exits (#10364) feat(ui): collapse scheduled job sessions into accordion in chat history (#11265) fix: bound retry command diagnostics (#11365) Disable thinking for tool call labels (#11207) fix(review): contain REVIEW.md discovery (#11367) fix(acp): preserve tool result audience metadata (#11375) test(providers): isolate environment-proxy test in its own binary (#11262) fix(security): bind Foundry API keys to request origin (#11347) fix: canonicalize mangled tool names before permission inspection (follow-up to #10230) (#10285) feat(providers): add Lynkr as a declarative OpenAI-compatible provider (#11372) fix: sanitize Pi imported output (#10990) ...
|
Addressed in I audited each file-backed
Added symlink regressions for Recipe and Agent discovery. Verification includes the focused supporting-file, summon, recipe, and source suites (20 source tests), — AI-generated by Codex |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62f3743115
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let content = crate::skills::read_source_file_with_limit( | ||
| &parent_dir, | ||
| Path::new(file_name), | ||
| crate::agents::max_tool_response_size(), |
There was a problem hiding this comment.
When GOOSE_MAX_TOOL_RESPONSE_SIZE is lowered to control when tool output is offloaded, this now also rejects any recipe whose source exceeds that character count, so an otherwise valid recipe can no longer be discovered or run before any tool response exists. The setting is specifically a tool-response threshold, and this path previously read recipes without that cap; use a separate source-file safety limit rather than coupling recipe loading to this configuration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid finding. I missed this unresolved thread before #11342 entered the merge queue.
Corrected in 7bac7a9a73 and follow-up PR #11391: trusted source reads now use a fixed 1 MiB byte limit grounded in the existing scheduled-recipe ceiling, while skill supporting-file responses retain the independent character limit from GOOSE_MAX_TOOL_RESPONSE_SIZE. Added regressions for a recipe above the default tool-response threshold and an over-limit source file; focused tests and clippy pass.
I am leaving this thread unresolved until the follow-up merges.
— AI-generated by Codex
…bined * origin/main: (85 commits) feat(desktop): sort configured providers to the top of the provider list (#11409) fix(cli): refuse symlink diagnostics outputs (#11398) test(plugins): isolate GOOSE_PATH_ROOT in discovery tests (#11407) fix(config): serialize secret mutations (#11388) fix: decouple source file and tool response limits (#11391) chore(deps): bump pctx_code_mode from 0.4.1 to 0.5.0 (#11245) fix(security): suppress sensitive OTLP traces (#11381) feat(openrouter): forward session_id and add app category header (#10868) feat(acp): derive and forward thinking effort from the ACP harness (#10949) fix(aws_bedrock): replace flat model list with routing table, add Gemma 4 Mantle support (#10297) Add GPT-5.6 follow-up support for Codex and Responses API (#10460) fix(update): fetch attestation bundles from bundle_url (#10557) fix(security): fail closed on invalid default GCP credentials (#11363) fix(codex): reject socket-backed MCP extensions (#11304) fix: pass complete response to stop hooks (#11366) fix: contain and bound skill supporting file reads (#11342) chore(deps): bump the ui-minor-and-patch group across 1 directory with 53 updates (#11386) fix(security): bound call graph traversal (#11193) fix: pin arrayref to known-good commit (#11389) feat(providers): add SayGM as declarative OpenAI-compatible provider (#11267) ... # Conflicts: # crates/goose/src/agents/agent.rs
…rn's context; skill file reads are bounded A local 27B rarely decides to call search_memories or load_skill. The new `recall` platform extension (default on, no tools) takes the user's own words on each request turn — never during a tool loop — drops function words, searches the memory store (when the memory extension is on) and the skill catalogue (when skills is on), and contributes a turn-context part: `<recalled-memories>` with the matching entries in full (an entry must share two terms with the request or carry one in its name; at most three) and `<relevant-skills>` naming skills to load (name match, or two description terms; at most three). No match, no block; an unreadable store is logged, never faked. Its one system-prompt instruction explains the two blocks; moim's template is untouched, so every other agent's prompt is byte-identical — swarm workers get only `developer` plus the swarm config's extensions, so they never carry it. Skills: a supporting file is cut at GOOSE_MAX_TOOL_RESPONSE_SIZE with the cut announced in the result (upstream aaif-goose#11342's intent, ported by hand around our rmcp 1.4 client); tracing::info on every skill and file load. Tests: 5 for recall's pure functions (request-turn detection, stopwords, hit selection, skill ranking, rendering); prompt_manager snapshot carries the new section. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VvuBB2cVVZ8Herdq3zyuMQ
…rn's context; skill file reads are bounded A local 27B rarely decides to call search_memories or load_skill. The new `recall` platform extension (default on, no tools) takes the user's own words on each request turn — never during a tool loop — drops function words, searches the memory store (when the memory extension is on) and the skill catalogue (when skills is on), and contributes a turn-context part: `<recalled-memories>` with the matching entries in full (an entry must share two terms with the request or carry one in its name; at most three) and `<relevant-skills>` naming skills to load (name match, or two description terms; at most three). No match, no block; an unreadable store is logged, never faked. Its one system-prompt instruction explains the two blocks; moim's template is untouched, so every other agent's prompt is byte-identical — swarm workers get only `developer` plus the swarm config's extensions, so they never carry it. Skills: a supporting file is cut at GOOSE_MAX_TOOL_RESPONSE_SIZE with the cut announced in the result (upstream aaif-goose#11342's intent, ported by hand around our rmcp 1.4 client); tracing::info on every skill and file load. Tests: 5 for recall's pure functions (request-turn detection, stopwords, hit selection, skill ranking, rendering); prompt_manager snapshot carries the new section. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VvuBB2cVVZ8Herdq3zyuMQ
3 participants
Summary
Security invariant
File-backed trusted-source reads must stay bound to their discovered source root and must not allocate or return more than the configured response budget.
Verification
cargo fmt --all -- --checkcargo test -p goose skills::supporting_files::tests --lib— 10 passedcargo test -p goose agents::platform_extensions::summon::tests --lib— 52 passedcargo test -p goose recipe::read_recipe_file_content::tests --lib— 3 passedcargo test -p goose sources:: --lib— 20 passedcargo build -p goosecargo clippy -p goose --all-targets -- -D warningsgit diff origin/main...HEAD --checkWindows-specific handle traversal was reviewed against Goose's existing
NtCreateFileimplementation; the available validation host was macOS/aarch64.Addresses https://github.com/project-loupe/audit-goose/issues/860
Addresses https://github.com/project-loupe/audit-goose/issues/862
This finding was discovered by Project Loupe