Feat/agent context instructions preset

[TheovanKraay]

Description

Part of #4200, and a follow-up to the discussion on #4259.

This is the delivery mechanism the maintainer proposed on #4259: instead of an extension manifest field that changes the agent's always-on behavior implicitly on install, always-on rules are delivered through an explicit, opt-in preset over agent-context. Core validates metadata only; the opt-in agent-context extension owns the writes; nothing touches the agent's context unless the user explicitly enables a preset.

• Presets (src/specify_cli/presets/__init__.py): preset.yml may declare provides.instructions (list of { file, description? }), path-safe. An instructions-only preset (no templates) is valid. Metadata validation only.
• agent-context (all three twins): each installed and enabled preset's instruction block is composed into the managed section as a namespaced <!-- SPECKIT PRESET:<id> START/END --> sub-block. The Python twin is the single source of truth (--emit-preset-blocks); the bash and PowerShell twins delegate to it for byte-identical output. Path-unsafe, non-UTF-8, and marker-colliding payloads are skipped fail-closed. The PowerShell twin warns when no Python 3 with PyYAML is available and presets are installed, instead of silently omitting the blocks.
• presets/example-always-on-rules/: example instructions-only preset.
• Docs: docs/reference/presets.md, presets/PUBLISHING.md.

Ownership and consent: the preset is the standalone unit (specify preset add / disable / remove), so installing an extension does not by itself change the agent. Enabling the preset is the explicit opt-in. An extension author can also distribute the preset together with the extension and agent-context as a bundle, so specify bundle install sets everything up in one previewable, consented step; that uses the existing bundle mechanism and lives with the extension, so it is out of scope here.

Testing

• Tested locally with uv run specify --help
• Ran existing tests with uv sync && uv run pytest
• Tested with a sample project (if applicable)

New tests/extensions/test_preset_instructions.py (16) and preset-parity tests in tests/extensions/test_update_agent_context_python_parity.py (Python-vs-Bash on CI, Python-vs-PowerShell locally, non-ASCII payload). No regressions across the preset and extension suites (771 passed, 144 skipped). Verified end to end via both the Python and PowerShell twins: specify init -> specify preset add --dev presets/example-always-on-rules -> specify extension add --dev extensions/agent-context -> agent-context update composes the block into .github/copilot-instructions.md.

AI Disclosure

• I did not use AI assistance for this contribution
• I did use AI assistance (describe below)

Implemented with GitHub Copilot (agentic): preset validation, the agent-context composition and its bash/powershell twins, the example preset, the tests, and the docs were written with AI assistance and reviewed by me.

[Copilot]

🟡 Changes recommended

Registry path containment and manifest validation gaps must be resolved before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds opt-in preset-provided always-on instructions composed by the agent-context extension.

Changes:

• Validates provides.instructions in preset manifests.
• Composes enabled preset rules across Python, Bash, and PowerShell scripts.
• Adds an example preset, documentation, and parity tests.

File summaries

File	Description
src/specify_cli/presets/__init__.py	Adds instruction metadata validation.
extensions/agent-context/scripts/python/update_agent_context.py	Collects and renders preset rules.
extensions/agent-context/scripts/bash/update-agent-context.sh	Delegates composition to Python.
extensions/agent-context/scripts/powershell/update-agent-context.ps1	Delegates composition with dependency warnings.
tests/extensions/test_preset_instructions.py	Tests validation and composition.
tests/extensions/test_update_agent_context_python_parity.py	Tests cross-script output parity.
docs/reference/presets.md	Documents always-on instructions.
presets/PUBLISHING.md	Documents manifest syntax.
presets/example-always-on-rules/preset.yml	Defines an example preset.
presets/example-always-on-rules/instructions/best-practices.md	Supplies example rules.

Review details

• Files reviewed: 10/10 changed files
• Comments generated: 3
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[TheovanKraay]

Good catch. The registry is untrusted input, so joining the preset id straight onto .specify/presets let an absolute key or a ../separator id (or a symlinked directory) point the manifest read outside the presets root.

Fixed in f660e11: the collector now rejects any id that is not a simple name (must match ^[a-z0-9][a-z0-9._-]*$, so no separators, .., or absolute/drive forms) and, after resolving, confirms the preset directory still lives inside the resolved presets root via relative_to before it opens preset.yml. A crafted key or symlink is skipped instead of read.

Added test_unsafe_registry_preset_id_skipped, which injects ../../evil and /abs-evil into the registry and asserts the good preset still composes while nothing from the escaped paths appears in the section.

[TheovanKraay]

Agreed. An empty provides.instructions list passed validation but contributed nothing, which is inconsistent with how we already reject empty templates.

Fixed in f660e11: validation now raises provides.instructions must not be empty when the list is present but empty, mirroring the empty-templates rejection. Added test_empty_instructions_list_rejected.

[TheovanKraay]

Right, the ad-hoc check let non-portable forms through (empty, whitespace, ., directory-only, and Windows drive-relative like C:rules.md).

Fixed in f660e11: instruction file paths now go through the shared relative_extension_path_violation policy in _utils.py, the same one the extension path validation uses, so all of those forms are rejected at metadata-validation time with a specific reason. Added a parametrized test_instruction_path_non_portable_rejected covering empty, whitespace, surrounding whitespace, ., directory-only, drive-relative, and backslash forms.

[Copilot]

🟡 Changes recommended

Instruction payloads need size limits, and the refresh lifecycle and traversal regression test need clarification or strengthening.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

docs/reference/presets.md:219

• This reads as though composition happens as soon as agent-context is installed and the preset is enabled, but the implementation only recomposes when the update command/hook next runs. In particular, adding or enabling a preset and then working outside a Spec Kit workflow leaves its rules absent indefinitely. Document that refresh requirement for add/enable as well as disable/remove, and point users to the update command.

This is opt-in and owned by the `agent-context` extension: nothing is written unless `agent-context` is installed and the preset is enabled. When both hold, `agent-context` composes each enabled preset's block into the routed context file (for example `.github/copilot-instructions.md`) inside a namespaced `<!-- SPECKIT PRESET:<id> START/END -->` block, and drops it again on `preset disable`/`remove` at the next refresh. Enabling the preset is the explicit opt-in; installing an extension does not by itself change the agent's context.

• Files reviewed: 10/10 changed files
• Comments generated: 2
• Review effort level: Balanced

[TheovanKraay]

Good call. The composed managed section is re-sent as agent context on every request, so an oversized instruction file (a bundled archive member or an unbounded --dev source) could bloat it without bound.

Fixed in a899821: added a deliberately small budget in the collector. Any single file over a per-file cap (32 KiB) is skipped with a warning, and the on-disk size is checked with stat() before the file is read so a huge member is never allocated into memory. A running aggregate cap (64 KiB across all presets) stops composition once reached, and each skip logs which preset and why.

Boundary coverage added: test_instruction_file_at_limit_included (a file exactly at the per-file cap is kept, since the check is strictly greater), test_oversized_instruction_file_skipped (over-cap file skipped, other presets still compose), and test_aggregate_instruction_budget_enforced (three under-cap presets where the third crosses the aggregate cap and is dropped in id order). The bash/ps1 twins delegate to this collector via --emit-preset-blocks, so the budget applies uniformly.

[TheovanKraay]

Right, the previous version only injected registry keys, so collection stopped at manifest.is_file() and the test would have passed even without the guards.

Fixed in a899821: the test now materializes a real out-of-root preset (a preset.yml plus an instruction payload tagged PWNED_TRAVERSAL_PAYLOAD) at exactly the location the resolved ../../evil key points at, and asserts that payload is not composed. So if the id-format or containment guard is removed, collection resolves and reads it and the test fails.

I also added test_symlinked_preset_dir_escaping_root_skipped: a valid simple id (linked) whose directory is a symlink pointing outside .specify/presets. That case passes the id regex, so it specifically exercises the resolved-containment check; it asserts the out-of-root payload (PWNED_SYMLINK_PAYLOAD) is not composed, and skips only where the platform disallows symlink creation.

[Copilot]

🟡 Changes recommended

Shell wrappers currently suppress composer failures and can erase previously composed instruction blocks.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

presets/example-always-on-rules/preset.yml:7

• This 242-character description violates the preset publishing checklist’s “under 200 characters” rule (presets/PUBLISHING.md:92). Since this preset is the new reference example, shorten it so it models the documented publishing contract.
  src/specify_cli/presets/init.py:529
• This shared validator’s traversal reason says the path must remain within the “extension directory” (src/specify_cli/_utils.py:95-96), so preset authors now receive an incorrect extension-specific error for unsafe instruction paths. Make the shared wording package-neutral or translate it to “preset directory” here.

• Files reviewed: 10/10 changed files
• Comments generated: 3
• Review effort level: Balanced

[TheovanKraay]

Good catch, and this was a real destructive-failure bug. The 2>/dev/null || true around the emit call both hid the composer's warnings and turned any nonzero exit into an empty result, which then rewrote the managed section with previously composed preset blocks silently dropped.

Fixed in 7107d8d: the call now captures only stdout (so the composer's oversized/marker-colliding/skipped warnings reach stderr) and checks the exit code, aborting the update on a nonzero exit so the section is never rewritten on failure. I moved it out of the section-building block so the abort happens before the temp section is assembled.

Verified locally: the happy path composes the block; an oversized entry warns on stderr and exits 0 so the update proceeds without it (a skip is not a failure); a hard composer failure aborts before any rewrite.

[TheovanKraay]

Agreed, same destructive failure mode as the bash twin. 2>$null discarded the composer's warnings and $LASTEXITCODE was never checked, so an emitter error was treated as "no blocks" and the upsert dropped existing preset instructions.

Fixed in 7107d8d: stderr is now routed to a temp file, surfaced to the console after the call (so warnings are preserved separately from the captured stdout), and the exit code is checked. A nonzero exit writes an error and aborts before $Section is assembled, so the managed section is never rewritten on failure. The UTF-8 OutputEncoding wrapper for stdout capture is kept.

Verified locally on Windows via a direct run of the PowerShell twin: the block composes on the happy path, and the emit path returns 0 with a stderr warning when an entry is skipped.

[TheovanKraay]

Right, since core validates metadata only, a preset can install cleanly yet contribute nothing at runtime, and that was not documented.

Fixed in 7107d8d: presets/PUBLISHING.md now documents, right after the manifest example, the runtime constraints the agent-context composer applies to each provides.instructions file: it must exist inside the preset directory and be valid UTF-8, must not contain a managed-section marker, must be at or below 32 KiB per file, and must fit the 64 KiB aggregate budget across all enabled presets. It notes that entries failing any of these are dropped fail-closed and logged to stderr while the rest still compose, and explains the budget exists because the managed section is re-sent as agent context on every request.

[Copilot]

🟡 Changes recommended

Aggregate accounting can allow rendered instruction blocks to exceed the intended 64 KiB context budget.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 10/10 changed files
• Comments generated: 2
• Review effort level: Balanced

[TheovanKraay]

Good catch, the counter was undercounting. It summed only each stripped payload, so a preset that references a tiny file thousands of times rendered well past 64 KiB while the counter stayed low (your 22,000-references example).

Fixed in 764c705: the accounting now adds the bytes each entry actually contributes to the rendered section, not just its payload. The first entry of a preset adds the surrounding marker block (<!-- SPECKIT PRESET:<id> START/END --> plus its newlines), and every later entry adds the two-byte blank-line separator. So the cap now bounds the composed always-on context, not an under-estimate of it.

Added test_aggregate_budget_counts_wrapper_and_separators: a single preset referencing a 1-byte file 25,000 times (raw sum under the cap) and asserts the composed section stays within _MAX_INSTRUCTION_TOTAL_BYTES and is truncated below the full entry count, so it fails if the overhead is dropped from the accounting.

[TheovanKraay]

Right, the doc over-promised. The collector warned on oversized/over-budget/marker-collision but silently skipped missing, unreadable, non-UTF-8, and path-unsafe files.

Fixed in 764c705 by doing both things you suggested. The collector now emits an stderr warning for a missing or unreadable/non-UTF-8 instruction file (these are author errors worth surfacing). Path-unsafe entries (absolute, parent-traversal, or a target that escapes the preset directory) stay silent on purpose, since that is a security fail-closed decision and I did not want to give traversal attempts useful feedback. The doc now states exactly that split: which skips warn versus which are intentionally silent, and also notes the aggregate budget counts the rendered wrappers/separators.

[mnriem]

Note that you should be able to define this completely as a preset and you should not need to adjust the extension scripts. The preset can wrap/replace those scripts (what you pick depends on your need).

[Copilot]

🟡 Changes recommended

Markdown content is mutated and two script failure paths do not provide the intended safe handling.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

extensions/agent-context/scripts/bash/update-agent-context.sh:361

• Because this script runs with set -e, a nonzero emitter status exits on the assignment itself, before _emit_rc and this diagnostic run. Put the assignment in an if condition so the intended error handling is reachable.
  extensions/agent-context/scripts/python/update_agent_context.py:345
• .strip() removes significant Markdown whitespace, not just the file's final newline. For example, a first line indented by four spaces stops being a code block, and trailing spaces used for hard line breaks are lost. Trim only boundary newlines so the authored instruction content retains its semantics.

• Files reviewed: 10/10 changed files
• Comments generated: 1
• Review effort level: Balanced

[TheovanKraay]

Good edge case. If Python was found but the sibling composer file was gone (a partial or corrupt install), the combined guard was just false and the script proceeded to rewrite the managed section without any preset blocks, silently erasing existing instructions.

Fixed in 719d319: when Python is available the ps1 twin now treats a missing composer as a hard failure, writing an error and aborting before the section is rewritten, exactly like the nonzero-exit path. The bash twin already covers this case, since a missing composer file makes python exit nonzero and the exit-code check aborts. Verified the ps1 parses clean and the happy path still composes.

[Copilot]

🟡 Changes recommended

Bash errexit bypasses the newly added composer failure handler and its diagnostic.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 10/10 changed files
• Comments generated: 1
• Review effort level: Balanced