Conversation
|
Independent verification on the PR head (9bdbd7f): skills auto-load plus pinned-skills wire suites 10/10 green on Linux. Preloading pinned guidance without skills tools keeps startup behavior intact. No findings. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to merged #92048. Configured
skills.auto_loadis startup guidance, not a callable capability. Include its full body when skills tools are unavailable, including sessions with zero tools. The skills index remains tool-gated; this change enables no tools.Remove only the tool-availability condition in
_auto_load_parts. Preserve internal-fork suppression, ignore-rules handling, existing profile resolution, missing/disabled-skill behavior, resolve-once caching and canonical-name explicit-preload deduplication.Configured skill text is sent to the chosen model even with skills tools disabled. Operators should not pin secrets or untrusted instructions. This does not change skill text's established prompt authority or any tool permissions.
Regression coverage
httpx.MockTransport. Both zero-tools and unrelated-tools cases verify full pinned content in the existing system/developer prefix, stable prefix bytes and unchanged callable schemas. Socket connections are blocked; auxiliary metadata, credentials, MCP discovery and security installation are stubbed as unrelated boundaries.Verification
Reviewed snapshot base:
4fda3bbdca947ebbff3f0c573881a578c3673d5d.Test-first on unchanged production source: auto-load tests produced 2 failed, 4 passed, specifically missing full pinned content with zero/unrelated tools.
Independent review ran the canonical runner with a clean temporary HOME and isolated dependency interpreter:
Result: 267 passed, 0 failed, 1 skipped across eight files. Four serialized provider requests were captured in-process; no real model/authentication endpoint was contacted.
git diff --checkpassed.Full repository suite and lint checks on the new request-capture test were not run. Earlier checks on the two original changed files passed
ruff check; their formatting checks also fail on unchanged upstream, so broad formatting churn is excluded.This is a single-profile in-process integration test, not an installed executable/argparse smoke test. Concurrent CLI ContextVar propagation is a separate unresolved issue and is not fixed or validated here. Independent review found no material correctness issue in this minimal production delta; this is not release certification. Maintainer agreement on treating configured guidance independently of tool availability remains a policy decision.