Skip to content

fix: opt-in OpenCode discovery of hidden Spec Kit files - #5032

Open
mvanhorn wants to merge 16 commits into
github:mainfrom
mvanhorn:fix/5028-opencode-hidden-discovery
Open

mvanhorn wants to merge 16 commits into
github:mainfrom
mvanhorn:fix/5028-opencode-hidden-discovery

Conversation

@mvanhorn

Copy link
Copy Markdown
Contributor

Description

OpenCode's file search reports that the hidden Spec Kit project directory contains no files. Local and open-weight models then miss the constitution, templates, and scripts the Spec Kit commands need. Models that retry by reading a known path directly often continue anyway. OpenCode 1.18.31 and 1.19.3 run that glob through ripgrep without --hidden, so dot-directories are skipped. Re-including the tree on that path takes both !.specify/ and !.specify/** in the project-root .ignore; the contents rule alone still returns no .specify/**/* matches. The OpenCode integration only installed command files and never wrote those exceptions.

OpenCode install and upgrade accept an opt-in --configure-ignore-file flag that appends whichever of !.specify/ and !.specify/** is not already an exact line, keeps existing .ignore bytes as a prefix (including non-ASCII text, CRLF, and a missing final newline), and records the root file as partial ownership. Symlinks, non-files, and unwritable paths are rejected before any write. Uninstall, including with --force, leaves .ignore in place, and installs without the flag, along with other integrations, leave it untouched. Upgrade replays the stored flag, a new --integration-options value replaces the whole stored set, and the integrations reference documents the option and the checked ripgrep behavior.

Fixes #5028

Testing

  • Tested locally with uv run specify --help
    Ran uv run python -m pytest tests/test_agent_config_consistency.py -q locally on macOS for the spec-kit OpencodeIntegration change; it fails the same way on the base branch, so the failure predates this change (it fails the same way on upstream/main).
  • Ran existing tests with uv sync && uv run pytest
    uv sync && uv run pytest was not run locally for the spec-kit OpencodeIntegration change either.
  • Tested with a sample project (if applicable)
    No one has checked the spec-kit OpencodeIntegration change with a sample project yet.

Ran uv run python -m pytest tests/test_agent_config_consistency.py -q locally on macOS for the spec-kit OpencodeIntegration change; it fails the same way on the base branch, so the failure predates this change (it fails the same way on upstream/main). Tests for the spec-kit OpencodeIntegration change live in tests/integrations/test_integration_opencode.py.

AI Disclosure

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

AI disclosure

AI was used for assistance.

  • Extent: AI wrote the code changes and drafted this description.

OpenCode install and upgrade accept an opt-in --configure-ignore-file
flag that appends whichever of !.specify/ and !.specify/** is not
already an exact line, keeps existing .ignore bytes as a prefix
(including non-ASCII text, CRLF, and a missing final newline), and
records the root file as partial ownership. Symlinks, non-files, and
unwritable paths are rejected before any write. Uninstall, including
with --force, leaves .ignore in place, and installs without the flag,
along with other integrations, leave it untouched. Upgrade replays the
stored flag, a new --integration-options value replaces the whole stored
set, and the integrations reference documents the option and the checked
ripgrep behavior.

Fixes github#5028

Assisted-by: AI
@mvanhorn
mvanhorn requested a review from mnriem as a code owner October 20, 2026 07:45
@mnriem mnriem added the triage-must-have Verdict: high-value, important work for Spec Kit — do first label Oct 20, 2026
@mnriem
mnriem requested a max review from Copilot October 20, 2026 13:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Write failures can destroy existing ignore rules, and rejected destinations can leave integration updates partially applied.

5 open issues
What changed in this PR

Adds an opt-in OpenCode integration option so file search can discover hidden Spec Kit project files.

Changes:

  • Adds .ignore exceptions with partial ownership tracking.
  • Tests preservation, upgrade persistence, uninstall, and failure handling.
  • Documents usage and checked OpenCode behavior.

Validation used isolated checks; project tests and baseline comparison were not run.

AI-generated for the requester by GitHub Copilot (model: unknown; autonomous review).

File Description
tests/​integrations/​test_integration_opencode.py Adds option and lifecycle coverage.
src/​specify_cli/​integrations/​opencode/​__init__.py Implements opt-in ignore configuration.
docs/​reference/​integrations.md Explains discovery behavior and usage.

Review effort: Max

Comment thread docs/reference/integrations.md Resolved
Comment thread docs/reference/integrations.md Resolved
@mnriem

mnriem commented Oct 20, 2026

Copy link
Copy Markdown
Collaborator

Please address Copilot feedback and fix test & lint errors

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Failed writes can destroy existing .ignore rules, and non-UTF-8 comments abort configuration.

4 open issues
4 previously missed issues

In code that hasn't changed since last review

Medium severity Deleting rules does not persistently opt out while OpenCode is installed

docs/​reference/​integrations.md:518

Deleting these lines is not a persistent opt-out while OpenCode remains installed. Partial ownership still hashes the entire .ignore, so any user edit blocks a normal upgrade (manifest.py:340–364, command_upgrade.py:80–87). Adding --force replays the stored flag and restores the rules; it also permits overwriting customized shared scripts and templates. Document the hash-tracking consequence and the safe order: first upgrade with options that omit this flag while preserving other options, then delete the lines. After uninstall, deleting them directly is sufficient.

AI-generated by GitHub Copilot for the review requester (model: unknown; autonomous).

Medium severity Non-UTF-8 .ignore content aborts install or upgrade

src/​specify_cli/​integrations/​opencode/​__init__.py:153

An existing .ignore with a non-UTF-8 comment, such as Latin-1 # café, raises UnicodeDecodeError here and aborts the opted-in install or upgrade. Only ASCII rules need recognition. Use lossless decoding for this membership check and add a non-UTF-8 preservation case; the original bytes can then remain untouched.

AI-generated by GitHub Copilot for the review requester (model: unknown; autonomous).

Medium severity Non-atomic .ignore writes can destroy existing user rules

src/​specify_cli/​integrations/​opencode/​__init__.py:192

record_file() calls Path.write_bytes() (manifest.py:208), which truncates the existing .ignore before writing. A write failure after opening can destroy user rules, and built-in-only installs/upgrades have no lifecycle journal to restore them (_lifecycle.py:468–469). The new disk-full mock raises before opening, so it misses this failure. Use an atomic replacement that preserves permissions, safe-path checks, and lifecycle notifications, and test a failure after output begins.

AI-generated by GitHub Copilot for the review requester (model: unknown; autonomous).

Medium severity Platform-dependent symlink and permission tests fail unexpectedly

tests/​integrations/​test_integration_opencode.py:341

Direct symlink creation can fail on Windows without the required privilege, and the permission-rejection assertions fail under root, which can still read and write these files. Either condition fails this combined test before its remaining cases run. Split the cases so only unsupported symlink and permission checks are skipped, following tests/integrations/test_integration_kimi.py:17–30 and tests/extensions/github/test_github_extension.py:1408–1411. Keep directory and write-failure checks platform-independent.

AI-generated by GitHub Copilot for the review requester (model: unknown; autonomous).

1 resolved since last review

Review effort: Max

@mnriem

mnriem commented Oct 20, 2026

Copy link
Copy Markdown
Collaborator

Please address Copilot feedback. If not applicable please explain why

Address the rest of the Copilot review on github#5032. The code fence fix
already landed in b395863; this covers the other four comments:

- Document that upgrades still hash-check .ignore, that --force also
  overwrites edited command files, and that the stored flag must be
  cleared before removing the two rules.
- Check .ignore (symlink, directory, unreadable, unwritable) before
  command files are rendered, and check it again before the write, so
  a rejected destination leaves existing command files unchanged.
- Match the two rules as ASCII bytes so a non-UTF-8 .ignore is kept
  byte for byte instead of raising UnicodeDecodeError.
- Write .ignore through the journal-aware atomic writer, keeping its
  file mode, and record partial ownership afterward, so a failed write
  can no longer truncate existing user rules.

Refs github#5028
Bring the branch up to date with main (505781d). github#5034 split
docs/reference/integrations.md into per-agent guides under
docs/integrations/ and deleted the old page, which conflicted with this
branch's edits to it.

Resolution: accept the deletion and move this branch's "OpenCode:
hidden Spec Kit files" section into docs/integrations/opencode.md as
"Hidden Spec Kit Files", and list it in the Integration-Specific
Options table in docs/integrations/cli.md, which is where main now
points readers for agent-specific options. The old catalog table rows
and options table row are dropped with the deleted page.
@mvanhorn

Copy link
Copy Markdown
Contributor Author

Addressed the Copilot review: the .ignore section now documents that upgrades still hash-check the file, that --force also overwrites edited command files, and that the stored flag must be cleared before removing the rules. .ignore is checked before command files are rendered and again before the write, the two rules are matched as bytes so a non-UTF-8 file is kept as-is, and the write goes through the journal-aware atomic writer with the file mode kept. Also merged main; since #5034 moved agent docs, the OpenCode section now lives in docs/integrations/opencode.md. ruff, markdownlint and the full pytest suite pass locally.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mnriem

mnriem commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator

Please address Copilot feedback. If not applicable, please explain why. And fix test & lint errors

- Require a writable project root whenever .ignore must change, including
  replacing an existing writable file, so setup fails before any command
  files are written. Add a read-only-root regression test.
- Compare post-setup .ignore modes with the mode chmod() actually set, so
  the mode-preservation test passes on Windows.
- Match the atomic writer's ..ignore. temp prefix in the cleanup assertion.
@mvanhorn

Copy link
Copy Markdown
Contributor Author

Addressed the three Copilot comments in 76d791d (the fix plus a merge of main):

  • The .ignore preflight now requires a writable project root whenever .ignore has to change, including replacing an existing writable file, so setup fails before any command files are written. Added a regression test for a writable .ignore in a read-only root (skipped on Windows, where directories always report writable).
  • The mode-preservation test now compares against the mode recorded right after chmod(), the same pattern as tests/test_merge.py. That was the assert 438 == 416 failure on the Windows jobs.
  • The cleanup assertion now matches the atomic writer's ..ignore. temp prefix.

Ran locally on macOS: uvx ruff@0.15.0 check src tests and uv run pytest tests/integrations -q -p no:cacheprovider, both passing. The Windows jobs could not run here.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Existing ignore-file content can still prevent discovery after configuration reports success.

2 open issues
3 resolved since last review

Review effort: Max

@mnriem

mnriem commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator

Please address Copilot feedback

- Evaluate the final .ignore in file order with pathspec's GitIgnoreSpec
  and fail setup when a later exclusion, such as .specify/** after the
  !.specify/ and !.specify/** rules, still hides a Spec Kit path. A later
  exception that restores visibility is accepted. Add ordering regression
  cases.
- Validate UTF-8 during preflight, before command files are written, and
  reject other encodings with a conversion hint, since ripgrep stops at
  the first invalid byte. Replace the windows-1252 positive fixture with a
  rejection test and update the docstring.
@mvanhorn

Copy link
Copy Markdown
Contributor Author

@mnriem both Copilot notes from the last review are handled in 5aaaeab:

  • The .ignore checks now run before install, switch or upgrade write any shared templates or scripts, so a forced upgrade with a bad .ignore leaves the project as it was.
  • Script probes only cover the --script variant being installed, plus scripts already in .specify/scripts.

Both have regression tests. Locally: ruff check src tests passes, markdownlint-cli2 on the OpenCode doc is clean, and the opencode, install/switch/upgrade, shared-infra and related suites pass (1186 passed, 27 skipped).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Preflight omits planned manifest and event-dispatcher outputs, allowing built-in-only installs or upgrades to fail after shared files have been created or overwritten without rollback.

1 open issue
2 resolved since last review

Review effort: Max


return content

def preflight_setup(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split out into #5198 as the prerequisite PR. It carries the preflight_setup() hook, the install/switch/upgrade wiring and shared_infra.script_variant_dirs(), with tests that use only existing built-in integrations: a refusal from each command exits 1 and leaves every project file byte-identical, and an accepted setup calls the hook once with the resolved options and script type.

The source changes in #5198 are identical to the shared part of this PR, so once it merges I'll merge main here and this diff drops to the OpenCode package, its tests and docs.

@mnriem

mnriem commented Oct 29, 2026

Copy link
Copy Markdown
Collaborator

Please separate the shared changes into its own PR as per the guidance

…idden-discovery

# Conflicts:
#	src/specify_cli/integrations/command_switch.py
mnriem pushed a commit that referenced this pull request Nov 2, 2026
…red writes (#5198)

* feat(integrations): add a preflight_setup() hook before shared writes

Install, switch and upgrade now call IntegrationBase.preflight_setup()
after options and the script type are resolved and before any shared
template, script or integration file is written. A ValueError or
OSError from the hook ends the command with exit 1 and "No files were
changed." The default accepts every setup, so no integration changes.

Also expose shared_infra.script_variant_dirs() so a hook can see which
script variants install_shared_infra() will copy.

Split out of #5032 per the integration delivery boundary.

* Assert the exact parsed options reach the preflight hook

---------

Co-authored-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
kevinnguyenhoang91 pushed a commit to kevinnguyenhoang91/spec-kit that referenced this pull request Nov 10, 2026
…red writes (github#5198)

* feat(integrations): add a preflight_setup() hook before shared writes

Install, switch and upgrade now call IntegrationBase.preflight_setup()
after options and the script type are resolved and before any shared
template, script or integration file is written. A ValueError or
OSError from the hook ends the command with exit 1 and "No files were
changed." The default accepts every setup, so no integration changes.

Also expose shared_infra.script_variant_dirs() so a hook can see which
script variants install_shared_infra() will copy.

Split out of github#5032 per the integration delivery boundary.

* Assert the exact parsed options reach the preflight hook

---------

Co-authored-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
Copilot AI max review requested due to automatic review settings November 11, 2026 00:51
@mvanhorn

Copy link
Copy Markdown
Contributor Author

@mnriem the shared preflight hook went in separately and is now on main, and I merged main into this branch in 7882944. This PR now only touches the OpenCode integration, its docs and its tests.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Blanket ignore exceptions can expose private configuration and API keys, while incomplete preflight checks can leave project files overwritten or the existing integration removed after failure.

9 open issues

Review effort: Max

Comment thread docs/integrations/opencode.md Resolved
assert ignore.read_bytes() == b"keep me\n!.specify/\n!.specify/**\n"
result = _run_in_project(project, ["integration", "upgrade", "opencode", "--force"])
assert result.exit_code == 0, result.output
assert ignore.read_bytes() == b"keep me\n!.specify/\n!.specify/**\n"
ripgrep ranks .ignore above .gitignore, so !.specify/** also exposed
.specify/extensions/*/local-config.yml, which can hold credentials.
Append an exclusion for it after the exceptions, and let the
later-exclusion check accept exclusions of those files.
Copilot AI max review requested due to automatic review settings November 11, 2026 02:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

- skip ignore lines pathspec rejects with re.error, not just ValueError
- read the root .ignore through FileHelper so a link swapped in after
  the checks is refused
- re-read .ignore right before the atomic write and abort if it changed
- reject .specify/.ignore and .specify/.rgignore rules that hide Spec Kit
  files, and document it
- skip the chmod-based permission tests when running as root
- write opencode in the Hidden Spec Kit Files docs, module comments and
  --configure-ignore-file help text
Copilot AI max review requested due to automatic review settings November 11, 2026 02:54
Exclude .specify/**/local-config.yml and .specify/**/*-config.local.yml
so extension removal backups and *-config.local.yml overrides stay
hidden, append them after the last negation line so a later ! rule
cannot re-include them, and leave .specify/extensions/.backup/ out of
the hidden-path probe set.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot AI max review requested due to automatic review settings November 11, 2026 03:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Credential-file protection remains unverified on opencode’s native glob discovery path, and the documented flag-clearing flow lacks regression coverage.

3 open issues
2 resolved since last review

Review effort: Max

Run rg on the .ignore that --configure-ignore-file writes. A plain
rg --files omits local-config.yml and *-config.local.yml, but a
positive --glob that matches them (.specify/**/*, **/*.yml) overrides
every ignore file and lists them. Scope the docs, module comment and
option help to searches without a matching glob, and point credentials
at SPECKIT_{EXTENSION}_* environment variables.
Copilot AI max review requested due to automatic review settings November 11, 2026 04:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

A race between the .ignore change check and atomic replacement can silently overwrite concurrent edits or a newly created file.

2 open issues
1 resolved since last review

Review effort: Max

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author-awaiting Waiting on author response triage-must-have Verdict: high-value, important work for Spec Kit — do first

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: .specify ignored with other dot-files by opencode glob

3 participants