fix(detect): expand copilot harness markers to file-based signals (#1435)#1440
Conversation
There was a problem hiding this comment.
Pull request overview
Expands the Copilot harness auto-detection logic to recognize file-based Copilot harness directory markers under .github/, in addition to the existing .github/copilot-instructions.md file signal.
Changes:
- Add
.github/instructions,.github/agents,.github/prompts, and.github/hooksas Copilot detection signals. - Add unit tests asserting each of the new
.github/*directories triggers Copilot detection.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/apm_cli/core/target_detection.py |
Extends SIGNAL_WHITELIST with additional Copilot directory markers for auto-detection. |
tests/unit/core/test_target_resolution_v2.py |
Adds 4 unit tests verifying the new .github/* directory signals map to the copilot target. |
sergio-sisternes-epam
force-pushed
the
fix/1435-copilot-harness-detection
branch
from
951a8b9 to
35308fd
Compare
sergio-sisternes-epam
added
panel-review
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 1 | Clean data-only whitelist extension; no architectural issues. One nit on false-positive risk for .github/hooks/. |
| CLI Logging Expert | 0 | 0 | 1 | _SIGNAL_LIST in errors.py correctly mirrors all 4 new SIGNAL_WHITELIST entries; dir-suffix convention is consistent. No output regressions. |
| DevX UX Expert | 0 | 1 | 1 | Three new signals are well-scoped; .github/hooks/ risks false-positive Copilot detection on non-Copilot repos and should be narrowed or dropped. |
| Supply Chain Security Expert | 0 | 0 | 1 | No supply-chain threats introduced; false-positive auto-detect risk is low and scoped to user-controlled repos only. |
| OSS Growth Hacker | 0 | 1 | 1 | Auto-detect fix unlocks Copilot file-based-instructions users; small but real adoption delta worth a CHANGELOG mention. |
| Doc Writer | 0 | 3 | 0 | Two reference pages document copilot detection as .github/copilot-instructions.md only -- the PR's four new file-based signals are not reflected, and no CHANGELOG entry exists. |
| Test Coverage Expert | 0 | 1 | 2 | Four happy-path unit tests are present and sufficient at unit tier; the integration tier gap for new dir-based Copilot signals is the only substantive finding. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Test Coverage Expert] Add integration-with-fixtures test: repo containing only .github/instructions/ auto-detects as copilot target via real CLI pipeline -- Unit tests pass even if the whitelist type flips from 'dir' to 'file'; only an integration test proves the user-visible promise holds end-to-end
- [Doc Writer] Update targets.md and targets-matrix.md copilot rows to list all five detection signals (not just .github/copilot-instructions.md) -- Reference docs now describe an incomplete and misleading detection surface; stale docs erode trust faster than missing features
- [OSS Growth Hacker] Add CHANGELOG entry under [Unreleased] for the four new Copilot file-based harness detection signals -- User-observable behavior change with no changelog entry; missed share moment for a Copilot-ecosystem alignment story
- [DevX UX Expert] Open follow-up issue: drop .github/hooks/ or implement compound-signal logic requiring co-presence of a confirmed Copilot-specific marker -- .github/hooks/ is not Copilot-exclusive and will false-positive in repos using it for Git or CI hook scripts, triggering AmbiguousHarnessError
- [Test Coverage Expert] Add unit test: .github/copilot-instructions.md file + .github/instructions/ dir co-present deduplicate to single copilot entry without AmbiguousHarnessError -- Same-harness deduplication is untested; low risk today but a silent regression trap as whitelist grows
Architecture
classDiagram
direction LR
class Signal {
<<ValueObject>>
+target str
+source str
}
class ResolvedTargets {
<<ValueObject>>
+targets list[str]
+source str
+auto_create bool
}
class SIGNAL_WHITELIST {
<<RegistryData>>
+list~tuple~ entries
}
class detect_signals {
<<PureFunction>>
+detect_signals(root) list[Signal]
}
class resolve_targets {
<<Facade>>
+resolve_targets(root, flag, yaml) ResolvedTargets
}
class NoHarnessError {
<<DomainError>>
}
class AmbiguousHarnessError {
<<DomainError>>
}
class _SIGNAL_LIST {
<<StringConstant>>
}
SIGNAL_WHITELIST <.. detect_signals : iterates
detect_signals ..> Signal : produces
resolve_targets ..> detect_signals : calls
resolve_targets ..> ResolvedTargets : returns
resolve_targets ..> NoHarnessError : raises
resolve_targets ..> AmbiguousHarnessError : raises
_SIGNAL_LIST <.. render_no_harness_error : embeds in message
class SIGNAL_WHITELIST:::touched
class _SIGNAL_LIST:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A(["apm run / apm install"]) --> B["resolve_targets()\ntarget_detection.py:694"]
B --> C{"--target flag?"}
C -- yes --> D["Return ResolvedTargets\nsource='--target flag'"]
C -- no --> E{"apm.yml targets?"}
E -- yes --> F["Return ResolvedTargets\nsource='apm.yml'"]
E -- no --> G["detect_signals(project_root)\ntarget_detection.py:672"]
G --> H["Iterate SIGNAL_WHITELIST\n[FS] is_dir / is_file checks"]
H --> H1["[FS] .github/copilot-instructions.md"]
H --> H2["[FS] .github/instructions/ (NEW)"]
H --> H3["[FS] .github/agents/ (NEW)"]
H --> H4["[FS] .github/prompts/ (NEW)"]
H --> H5["[FS] .github/hooks/ (NEW)"]
H1 & H2 & H3 & H4 & H5 --> I["Collect Signal objects"]
I --> J{"Dedupe by target"}
J -- 0 targets --> K["raise NoHarnessError\nerrors.py: _SIGNAL_LIST in message"]
J -- 2+ targets --> L["raise AmbiguousHarnessError"]
J -- 1 target --> M["Return ResolvedTargets\nsource='auto-detect from ...'"]
Recommendation
Ship this PR. The three unambiguous Copilot file-based instruction signals (.github/instructions/, .github/agents/, .github/prompts/) are correctly implemented, tested at unit tier, and reflect where the Copilot ecosystem is heading. The .github/hooks/ entry carries a false-positive risk that should be tracked as a follow-up issue rather than a revert -- the blast radius is limited to AmbiguousHarnessError scenarios, not silent data corruption. Before the next release cut, the author or a follow-up PR should add the CHANGELOG entry, update the two stale reference doc pages, and open a tracking issue for the .github/hooks/ compound-signal decision. The integration test gap is the only finding that carries structural weight beyond opinion; it should be filed as a fast-follow, not a merge gate.
Full per-persona findings
Python Architect
- [nit] .github/hooks/ may false-positive on non-Copilot git hooks dirs at
src/apm_cli/core/target_detection.py:629
The .github/hooks/ path is not an official Copilot VS Code file-based instruction directory -- it is sometimes used for custom git hooks stored in .github/. If a repo keeps non-Copilot hooks there, APM will wrongly auto-detect copilot and block apm run with AmbiguousHarnessError when another harness is also present.
Suggested: Add inline comment noting that .github/hooks/ is Copilot-specific only when co-located with other Copilot dirs.
CLI Logging Expert
- [nit] _SIGNAL_LIST now has 14 entries across 4 lines; wraps mid-sentence at 80-char terminals at
src/apm_cli/core/errors.py:62
_SIGNAL_LIST in errors.py is used in the 'no harness detected' error. With 14 comma-separated entries, it wraps mid-sentence on narrow terminals. Pre-existing issue made slightly worse.
Suggested: Group markers by harness name or move to bullet list in render_no_harness_error().
DevX UX Expert
- [recommended] .github/hooks/ is not a Copilot-exclusive directory and risks false-positive auto-detection at
src/apm_cli/core/target_detection.py:629
Teams commonly use .github/hooks/ for Git hook scripts or CI hook configs; matching it as a Copilot harness signal will falsely auto-select 'copilot' in those repos. The other three directories (.github/instructions/, .github/agents/, .github/prompts/) are directly documented in VS Code/Copilot file-based instructions and carry low false-positive risk.
Suggested: Remove the ('copilot', 'dir', '.github/hooks') entry, or implement compound-signal logic so hooks/ only triggers Copilot detection alongside a confirmed Copilot-specific marker. - [nit] _SIGNAL_LIST error message grouping clarity reduced with 5 Copilot paths inline at
src/apm_cli/core/errors.py:63
Updated error string lists all five Copilot paths inline, making it visually harder to parse which tool each marker belongs to.
Suggested: Minor formatting pass on _SIGNAL_LIST to group markers by harness name.
Supply Chain Security Expert
- [nit] .github/hooks/ name could be confused with Git or GH Actions hooks; add an inline comment at
src/apm_cli/core/target_detection.py:629
GitHub does not use .github/hooks/ (Git hooks live in .git/hooks/). No privilege-escalation risk, but a comment prevents future maintainer confusion.
Suggested: Add comment: # Copilot harness hook definitions (not Git/.git/hooks or GH Actions)
OSS Growth Hacker
- [recommended] No CHANGELOG entry for this behavior-changing fix at
CHANGELOG.md
File-based Copilot instructions is the modern pattern VS Code Copilot pushes users toward; without detection, APM silently falls back to manual --target copilot. A changelog entry converts that confusion into a share moment.
Suggested: Add to CHANGELOG under Fixes: 'apm auto-detects Copilot file-based instruction directories (.github/instructions/, .github/agents/, .github/prompts/, .github/hooks/) so projects using the multi-file layout are recognised without --target copilot. ([BUG] No harness detected - Copilot #1435)' - [nit] .github/agents/ and .github/prompts/ are also APM-native dirs -- a dogfooding story angle worth noting in release copy
APM dogfoods these directories itself. APM repos themselves now self-detect -- a nice story angle.
Auth Expert -- inactive
This PR only adds file-based harness directory signals to the detection whitelist and error signal list -- no authentication behavior, token management, credential resolution, or remote-host fallback semantics are touched.
Doc Writer
- [recommended] No CHANGELOG entry for the four new Copilot harness detection signals at
CHANGELOG.md
This is a user-observable behavior change (four new auto-detection signals) with no CHANGELOG entry under [Unreleased].
Suggested: Add: 'apm targets / apm install auto-detection now recognises .github/instructions/, .github/agents/, .github/prompts/, and .github/hooks/ as Copilot harness signals. ([BUG] No harness detected - Copilot #1435)' - [recommended] docs/src/content/docs/reference/cli/targets.md copilot row lists only .github/copilot-instructions.md -- now stale at
docs/src/content/docs/reference/cli/targets.md
After this fix, any of five paths activates the copilot target. The reference doc only lists one.
Suggested: Update the copilot row to list all five signals. - [recommended] docs/src/content/docs/reference/targets-matrix.md has two stale copilot signal references at
docs/src/content/docs/reference/targets-matrix.md
Two places list .github/copilot-instructions.md as the sole copilot signal; both are now stale after the PR.
Suggested: Expand the copilot row in the detection signal whitelist table and update the ## copilot section prose to name all five signals.
Test Coverage Expert
- [recommended] No integration-with-fixtures test exercises the new .github/instructions (or agents/prompts/hooks) dir as a Copilot autodetect signal via the real CLI pipeline at
tests/integration/test_target_resolution_e2e.py
The new signal entries feed the full apm compile/apm install command path. Per the tier-floor matrix, integration-with-fixtures evidence is needed. A unit test on _signal_targets() is necessary but not sufficient: if the whitelist type flips from 'dir' to 'file' for the new entries, all four unit tests still pass but the CLI would stop detecting these directories.
Proof (missing at integration-with-fixtures):tests/integration/test_target_resolution_e2e.py::test_s22b_github_instructions_dir_detects_copilot-- proves: A repo containing only .github/instructions/ is auto-detected as a Copilot target by the CLI without the user passing --target [multi-harness-support]
assert result.exit_code == 0 and 'copilot' in result.output.lower() - [nit] No test for multi-signal deduplication: .github/instructions dir + .github/copilot-instructions.md file both present simultaneously at
tests/unit/core/test_target_resolution_v2.py
No test confirms that two Copilot signals from the same harness (one file, one dir) are de-duplicated to a single 'copilot' entry rather than raising a spurious AmbiguousHarnessError. Risk is lower since this is same-harness not cross-harness.
Proof (missing at unit):tests/unit/core/test_target_resolution_v2.py::test_signal_whitelist_github_instructions_dir_plus_file_deduplicates_to_single_copilot-- proves: A repo with both .github/copilot-instructions.md and .github/instructions/ does not raise an ambiguity error and resolves to exactly one copilot target
assert _signal_targets(tmp_path) == {'copilot'} - [nit] Existing negative-case unit test (bare .github/ not a signal) confirmed present -- no gap at
tests/unit/core/test_target_resolution_v2.py
test_signal_whitelist_github_dir_alone_NOT_signal exists and correctly defends the promise.
Proof (passed at unit):tests/unit/core/test_target_resolution_v2.py::test_signal_whitelist_github_dir_alone_NOT_signal-- proves: A bare .github/ directory without any recognized sub-path is NOT treated as a Copilot signal
assert "copilot" not in _signal_targets(tmp_path)
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Generated by PR Review Panel for issue #1440 · ● 2M · ◷
github-actions
Bot
removed
the
panel-review
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 1 | Clean data-only whitelist extension; follows existing (harness, kind, path) pattern exactly. Ship. |
| CLI Logging Expert | 0 | 0 | 1 | User-facing _SIGNAL_LIST string accurate and consistent with new whitelist; flat 14-entry string is a follow-up DRY opportunity. |
| DevX UX Expert | 0 | 0 | 2 | Correct first-run detection fix; consider AGENTS.md and error-string length as follow-ups. |
| Supply Chain Security Expert | 0 | 0 | 0 | Pure local file-existence sniff; no remote fetch, no token use, no integrity surface. No security regression. |
| OSS Growth Hacker | 0 | 0 | 2 | Strong adoption signal from community contributor; unblocks first-run for the growing Copilot file-based-instructions cohort. Ship and amplify. |
| Test Coverage Expert | 0 | 0 | 1 | All four new copilot dir-markers have positive-detection tests using real filesystem fixtures; negative guard preserved. No coverage gap. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [CLI Logging Expert] Generate _SIGNAL_LIST programmatically from SIGNAL_WHITELIST to eliminate dual-maintenance drift risk -- 14 entries hand-synced across two files will eventually drift on a future whitelist addition. DRY now while the surface is small.
- [DevX UX Expert] Evaluate adding ('copilot', 'file', 'AGENTS.md') as a root-level Copilot marker -- emerging convention for Copilot Coding Agent; omitting it leaves a detection gap for a small but vocal user segment.
- [OSS Growth Hacker] CHANGELOG entry crediting @sergio-sisternes-epam and framing as "detect modern Copilot setups" -- rewards the contributor publicly and plants an SEO/discoverability keyword for the file-based-instructions audience.
- [OSS Growth Hacker] Add one-line mention of file-based signals in the quickstart harness-detection section -- power users adopting file-based instructions are high-propensity evangelists; validating their setup reduces "will this work for me" friction.
- [DevX UX Expert] Consider collapsing the error-message signal list to avoid wall-of-text as whitelist grows -- at 14 entries the parenthetical is ~180 chars; npm/pip keep hints short and link to docs for exhaustive lists.
Architecture
classDiagram
direction LR
class ResolvedTargets {
<<DataClass>>
+targets list~str~
+source str
+auto_create bool
}
class Signal {
<<DataClass>>
+target str
+source str
}
class SIGNAL_WHITELIST {
<<ModuleConstant>>
list~tuple~ entries
}
class detect_signals {
<<Pure>>
+detect_signals(project_root) list~Signal~
}
class resolve_targets {
<<IOBoundary>>
+resolve_targets(...) ResolvedTargets
}
detect_signals ..> SIGNAL_WHITELIST : iterates
detect_signals ..> Signal : produces
resolve_targets ..> detect_signals : calls
resolve_targets ..> ResolvedTargets : returns
flowchart TD
A["apm install / apm run"] --> B["resolve_targets()"]
B --> C{"--target flag provided?"}
C -- yes --> D["validate_canonical_v2()"]
C -- no --> E["detect_signals(project_root)"]
E --> F["[I/O] iterate SIGNAL_WHITELIST<br/>check is_dir / is_file"]
F --> G{"any Signal found?"}
G -- yes --> H["build ResolvedTargets from signals"]
G -- no --> I["raise EmptyTargetsListError<br/>with _SIGNAL_LIST hint"]
H --> J["continue command execution"]
D --> J
Recommendation
Merge as-is. CI green, full test coverage on the new signals, zero security surface, all panelists agree. The DRY follow-up (_SIGNAL_LIST generation) and AGENTS.md gap are real but non-blocking -- track them as issues post-merge. Add the CHANGELOG credit line at merge time to close the contributor loop.
Full per-persona findings
Python Architect
- [nit] SIGNAL_WHITELIST entries use bare paths without trailing slash; existing convention at
src/apm_cli/core/target_detection.py:626
Existing convention is consistent (no trailing slash in the tuple, slash appended at detect_signals line 674). The new entries follow that convention correctly. Noting for future contributors.
CLI Logging Expert
- [nit] _SIGNAL_LIST and SIGNAL_WHITELIST are now two sources of truth that must be hand-kept in sync at
src/apm_cli/core/errors.py:62
_SIGNAL_LIST is rendered inside EmptyTargetsListError. With 14 entries in a flat comma-separated string it could drift from SIGNAL_WHITELIST on a future change. Consider generating it programmatically from SIGNAL_WHITELIST in a follow-up. Current string is accurate.
DevX UX Expert
- [nit] Error message signal list is approaching wall-of-text territory after the addition at
src/apm_cli/core/errors.py:62
The _SIGNAL_LIST is shown verbatim when no harness is detected. Adding 4 dir paths makes the parenthetical ~180 chars. npm and pip keep the "what we looked for" hint short and link to docs for the full list. Not blocking; the error is still correct and actionable.
Suggested: Consider collapsing copilot entries to ".github/copilot-instructions.md + 4 file-based dirs" or linking to docs. - [nit] AGENTS.md (repo root) is an emerging Copilot convention not covered by this PR
Some repos use a root AGENTS.md for the Copilot Coding Agent. Omitting it creates a gap where a user thinks "I use Copilot" but auto-detection misses them. Four directories in this PR cover the documented file-based instructions paths and the majority case; AGENTS.md is a fine follow-up.
Suggested: Open a follow-up issue to evaluate adding ('copilot', 'file', 'AGENTS.md').
Supply Chain Security Expert
No findings.
OSS Growth Hacker
- [nit] Release note should name the contributor and frame the ecosystem trend
First community contribution that fixes a first-run blocker is a story-shaped change. Naming the contributor in CHANGELOG/release notes rewards community-spotted gaps. The file-based instructions pattern is where Copilot is heading; framing this as "APM now detects modern Copilot setups" plants a discoverability keyword.
Suggested: CHANGELOG: "fix(detect): recognize file-based Copilot instruction directories -- thanks @sergio-sisternes-epam (fix(detect): expand copilot harness markers to file-based signals (#1435) #1440)". - [nit] Mention file-based signals in quickstart for discoverability
Users adopting file-based instructions are power users likely to evangelize. A one-line mention in the quickstart validates their setup choice and reduces "will this work for me" friction.
Suggested: Add a parenthetical in docs/src/content/docs/getting-started/quick-start.md where harness detection is described.
Auth Expert -- inactive
PR touches src/apm_cli/core/errors.py and src/apm_cli/core/target_detection.py only -- no auth, token, AuthResolver, or remote-host surface in diff.
Doc Writer -- inactive
PR touches only src/apm_cli/core/errors.py, src/apm_cli/core/target_detection.py, and tests/unit/core/test_target_resolution_v2.py -- no docs, README, CHANGELOG, MANIFESTO, .apm/, .github/ instructions, or workflows in diff.
Test Coverage Expert
- [nit] Coverage closes the loop: 4 positive tests + 1 preserved negative test guard the contract end-to-end at the detection unit
Mirror tests for each new whitelist entry follow the same fixture pattern as the existing test_signal_whitelist_copilot_instructions_is_signal. The negative regression-trap test_signal_whitelist_github_dir_alone_NOT_signal still holds. tmp_path with real mkdir + the real _signal_targets() call qualifies as integration-with-fixtures tier (no mocking of the filesystem boundary). This is the right floor for a harness-detection signal sniff.
Proof (passed):tests/unit/core/test_target_resolution_v2.py::test_signal_whitelist_github_*_dir_is_copilot_signal (4 tests) + test_signal_whitelist_github_dir_alone_NOT_signal-- proves: When any of the four file-based Copilot marker directories exists, apm auto-detects copilot as a target [multi-harness-support, devx]
assert 'copilot' in _signal_targets(tmp_path)
Performance Expert -- inactive
PR touches only target_detection.py (a small whitelist tuple iteration) and errors.py and a unit test file -- no cache/, deps/, install/phases, install/pipeline, install/resolve, scripts/perf, or perf-instrumentation surface in diff. Adds 4 path-existence checks to detect_signals, an O(1) extension of an already-O(n) sniff.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
sergio-sisternes-epam
force-pushed
the
fix/1435-copilot-harness-detection
branch
from
2309b69 to
3a0ef37
Compare
sergio-sisternes-epam
force-pushed
the
fix/1435-copilot-harness-detection
branch
from
3a0ef37 to
2a11ff4
Compare
Description
Expand Copilot harness auto-detection to recognise file-based instruction directories, not just
.github/copilot-instructions.md.Fixes #1435
Type of change
Testing
What changed
Added four new entries to
SIGNAL_WHITELISTinsrc/apm_cli/core/target_detection.py:.github/instructions/(file-based instructions).github/agents/(agent definitions).github/prompts/(prompt files).github/hooks/(hook definitions)These directories are standard Copilot harness markers that users create when using file-based instructions instead of the monolithic
copilot-instructions.md.Tests added
4 new tests in
tests/unit/core/test_target_resolution_v2.py:test_signal_whitelist_github_instructions_dir_is_copilot_signaltest_signal_whitelist_github_agents_dir_is_copilot_signaltest_signal_whitelist_github_prompts_dir_is_copilot_signaltest_signal_whitelist_github_hooks_dir_is_copilot_signalExisting test
test_signal_whitelist_github_dir_alone_NOT_signalconfirms a bare.github/without these subdirs does NOT trigger Copilot detection.All 31 tests pass. Lint clean.