Skip to content

feat(extensions): add bundled github extension for taskstoissues - #4488

Merged
mnriem merged 23 commits into
github:mainfrom
Yash-Chindam:feat/4421-github-extension
Sep 29, 2026
Merged

mnriem merged 23 commits into
github:mainfrom
Yash-Chindam:feat/4421-github-extension

Conversation

@Yash-Chindam

@Yash-Chindam Yash-Chindam commented Sep 9, 2026 •

Copy link
Copy Markdown
Contributor

Description

Partially addresses #4421 — stage 1 of the three-stage migration (stage 2 deprecates the core command, stage 3 removes it). Generic add-on registration remains a prerequisite before core removal.

Adds a bundled, opt-in github extension providing speckit.github.taskstoissues. The core speckit.taskstoissues command is untouched and nothing here claims its name, so the two coexist for the whole of stage 1.

specify extension add github
/speckit.github.taskstoissues

Shape

The issue proposed a github-issues extension. Per @mnriem's scoping comment this lands as github instead — the home for GitHub-platform functionality, with git staying the local-VCS-workflow domain. Milestone grouping (#4370) gets a natural home in the same extension rather than spawning a second narrow one.

The extension owns its feature-resolution script rather than reaching back into core check-prerequisites through ../../scripts/. That matters for more than tidiness: an extension that escapes its own directory re-couples it to core internals, which is the exact thing stage 3 removes. Because the extension now ships resolve-tasks, the plain scripts: spelling is the correct one — it renders to .specify/extensions/github/scripts/…, a path that exists — and the escape hatch disappears instead of being tested around.

resolve-tasks is a trimmed twin of core check-prerequisites, not a copy. It resolves the project root (honouring SPECIFY_INIT_DIR) and the active feature directory (SPECIFY_FEATURE_DIRECTORY, else .specify/feature.json), requires plan.md and tasks.md, and reports the design docs beside them. It mirrors the invocation the core command makes (--require-tasks --include-tasks): plan.md is required, spec.md is not, and by default a SPECIFY_FEATURE_DIRECTORY override is persisted to feature.json exactly as core persists it, so a later run without the variable resolves to the same feature. Resolution without an override writes nothing. With SPECIFY_FEATURE_NO_PERSIST=1 or true, an override selects the feature for this run but does not create or overwrite feature.json. (Earlier revisions of this branch skipped the plan gate and the persistence; both were divergences from core, reported in review and fixed in 850430e, with per-runtime parity tests.)

Why the script path deserved a test

A verbatim copy of the core command's frontmatter renders .specify/extensions/github/scripts/bash/check-prerequisites.sh — a file no extension ships. That install validates, registers the command, auto-registers the skill, and prints ✓ Extension installed successfully!. The failure only surfaces when a user runs the command. TestScriptPathResolution installs the extension, renders it, and asserts the resolved path is a file that exists on disk — in command mode, in skills mode, and for all three script runtimes.

Acceptance criteria

Criterion Evidence
Bundled extension installs via specify extension add … ✅ catalog.json (bundled: true) + pyproject.toml force-include; TestCatalogEntry, TestExtensionInstall. Installed as github, not github-issues, per the scoping comment.
Provides the command for integrations with add-on registration ⚠️ test_every_extension_registrar_integration_renders_the_command covers every entry in AGENT_CONFIGS; test_generic_installs_sources_without_registering_an_artifact confirms the supported generic integration cannot invoke the replacement in either commands or skills mode. Generic support remains follow-up work before core removal.
Preserves issue creation, remote validation, pagination, deduplication ✅ test_body_differs_from_core_only_in_the_script_invocation diffs the command against core and requires the only changed lines to be the script invocation plus the two that read the new TASKS value. test_preserves_remote_validation and test_preserves_deduplication_and_pagination additionally pin the GitHub-URL guard, the both-states listing, cursor paging (perPage, after, endCursor, the early exit), and the four-digit-safe \bT\d{3,}\b pattern.
before_taskstoissues / after_taskstoissues hooks execute as before ✅ The keys are literal strings the command body reads out of .specify/extensions.yml, so copying the body preserves the contract. TestCommandBody pins both keys and asserts the live consumers — the git extension's auto-commit hooks — still target them.
Core speckit.taskstoissues remains available and unchanged ✅ git diff main -- templates/ scripts/ src/ is empty. Verified end to end: after installing the extension, the core command and its check-prerequisites invocation both still resolve and run.
No legacy alias conflicting with the core command ✅ test_no_alias_claims_the_core_command. Worth noting this is author discipline, not something the system enforces — see "Split out" below.
Tests cover installation, invocation artifacts, hooks, uninstall, and resolver parity ✅ tests/extensions/github/ covers all three script runtimes, including the plan.md gate, default override persistence, no-persist opt-out, and Windows PowerShell 5.1 UTF-8 feature paths.
Documentation explains installation, usage, migration ✅ extensions/github/README.md (install, removal, command table with per-integration invocation syntax, behaviour, hooks, requirements, scripts, and a migration section with the three stages and a before/after table), plus pointers from README.md and docs/installation.md.
Included in a minor release ⏳ Maintainer's call — nothing in the diff pins a version.

Answering the assessment's blocking questions

The stage-5 decision returned needs-clarification. Four of its six blocking questions are settled by the scoping comment and this implementation:

  • Is the extension responsible for correcting the helper script path, and what exact rendered path must each runtime use? Yes — and by owning the script rather than by correcting a path into core. All three resolve under .specify/extensions/github/scripts/{bash,powershell,python}/, asserted per-runtime.
  • Is scripts: frontmatter stripping part of this request? No — split out, see below.
  • Which integrations and layouts are required for parity? Every integration with static add-on registration are covered by a parametrised test; the supported generic integration is an explicit exception in both commands and skills layouts pending shared registration support.
  • Is "no legacy alias" a local constraint or a repo-wide guarantee? Local to this extension here; the repo-wide guard is a separate concern, see below.

The remaining two — usage/adoption baselines for gating stage 2, and which release carries stage 1 — are product decisions rather than implementation ones, and nothing here forecloses either.

Split out, as requested

Two findings from the scoping investigation are defects in shared paths rather than in this extension, and are deliberately not touched here so stage 1 stays additive:

  1. Extension rendering does not strip scripts: from the agent-facing frontmatter, while the core render does (step 3 of process_command_template, integrations/base.py). Visible in this PR's own output: the generated .github/agents/speckit.github.taskstoissues.agent.md retains the key, where core's does not. No bundled extension declared scripts: before this one, so nothing had exercised it.
  2. _validate_install_conflicts overstates what it checks. It is documented as rejecting installs that "would shadow core or installed extension commands", but _get_installed_command_name_map only walks self.registry — installed extensions. Core command names are never in that map, so an extension declaring an alias of speckit.taskstoissues is accepted today, with the core command present.

Both are ready to file with the reproductions. A third cross-cutting limitation—generic add-on registration—requires separate shared-infrastructure work before the core command can be removed; the current stage documents it instead of expanding the shared registrar.

Files

Path
extensions/github/extension.yml Manifest — id: github, one namespaced command, and the requires.speckit_version block (absent from the issue body's proposed manifest, which install rejects)
extensions/github/commands/speckit.github.taskstoissues.md Command; body differs from core only in the script invocation
extensions/github/scripts/{bash,powershell,python}/resolve-tasks.* Vendored feature/tasks resolver, three runtimes
extensions/github/README.md Install, usage, hooks, requirements, migration
extensions/catalog.json, pyproject.toml Bundled registration and wheel packaging
docs/installation.md, docs/reference/agentic-sdd.md Point at the extension, migration, and generic limitation
tests/extensions/github/ Registration and resolver behavior tests, including the generic exception in both layouts
tests/test_ps1_encoding.py New .ps1 directory added to the ASCII-only (PowerShell 5.1) guard

Testing

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

Full suite, this branch vs. a main worktree on the same machine:

passed skipped failed errors
main (0c8e31f) 7055 566 68 38
this branch 7086 568 68 38

Identical failure and error counts; the delta is exactly the tests added. Those 68/38 are pre-existing on this Windows box, not regressions — they sit in test_setup_tasks.py, test_check_prerequisites_python_parity.py, test_setup_plan_no_overwrite.py and test_setup_plan_feature_json.py, which exercise core scripts this PR does not touch. The box has no pwsh, and bare bash resolves to the WSL launcher rather than Git Bash, so those PowerShell and parity tests error the same way in both trees; running those four files against the baseline worktree directly gives byte-identical results (5 failed / 8 passed / 40 skipped / 27 errors).

That table predates the later commits. Current state, re-run after the fixes below — tests/extensions, tests/test_ps1_encoding.py, tests/test_extension_registration.py, tests/test_extension_skills.py, tests/test_extensions.py → 836 passed, 237 skipped, plus the same two symlink-privilege failures that fail identically on main (test_scaffold_config_rejects_symlink_template, test_scaffold_config_rejects_symlinked_config_root).

CI caught two bugs this box could not. The json_escape parity tests wrote into a tmp subdirectory nothing created, so every case died with FileNotFoundError before asserting — they had never executed, because bare bash resolves to the WSL launcher here and requires_bash skipped them. And the parity test then flagged real drift: upstream #4456 changed core's taskstoissues hook-error handling after this branch was cut, so the extension copy went stale. Both fixed in 36a4745 (rebased onto main, both hook blocks ported, diff back to the intended 10 lines).

To execute the bash-gated tests locally I ran pytest with a small plugin that rewrites only argv[0] from bash to Git Bash — System32 precedes PATH in Windows' CreateProcess search, so no PATH ordering can redirect a bare bash. With that, all 92 github tests run: 91 passed, 1 skipped (NTFS rejects the backslash-in-filename case, which the test skips explicitly).

The two bash-twin tests skip here for the WSL reason above; I ran both scenarios by hand against Git Bash and confirmed output and exit codes match the assertions.

End-to-end, against scratch projects scaffolded from this branch:

  • Copilot, commands mode — specify extension add github installs cleanly; .github/agents/speckit.github.taskstoissues.agent.md renders {SCRIPT} to .specify/extensions/github/scripts/bash/resolve-tasks.sh --json; that file exists and returns the expected FEATURE_DIR / TASKS / AVAILABLE_DOCS. Core speckit.taskstoissues.agent.md and .specify/scripts/bash/check-prerequisites.sh are still present and still work.
  • Copilot, skills mode (default) — .github/skills/speckit-github-taskstoissues/SKILL.md resolves the same path. specify extension remove github removes the skill and the extension directory, leaving the core skill intact.
  • All three script twins were run against the same fixture and agree on FEATURE_DIR, TASKS, and AVAILABLE_DOCS, including the single-element-array case and the missing-tasks.md error path (exit 1, same message). The PowerShell twin is ASCII-only for PowerShell 5.1 and reads feature.json as UTF-8 explicitly (fix: decode feature.json as UTF-8 in Windows PowerShell #4359).

Manual test results

Agent: GitHub Copilot (scaffolded via specify init) | OS/Shell: Windows 11 / Git Bash + PowerShell 5.1

Command tested Notes
specify extension add github Installs in both commands and skills mode; command/skill registered; scripts land under .specify/extensions/github/scripts/.
specify extension remove github Removes command/skill and extension directory; core taskstoissues untouched.
/speckit.github.taskstoissues Script step verified by running the rendered {SCRIPT} invocation directly (bash, PowerShell, Python) against a fixture feature. The issue-creation steps need a GitHub MCP server and a GitHub remote, which I did not exercise against a live repository — that portion of the body is carried over from the core command unchanged, and the diff test above pins that it stays that way.
/speckit.taskstoissues (core) Still installed and unchanged; its check-prerequisites invocation still resolves.

AI Disclosure

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

I used Claude Code (model Claude Opus 5) to help write the extension, the vendored scripts, the tests, and this description. I reviewed the changes and ran the verification described above myself.

GitHub Copilot (GPT-5.6 Sol Fast (Internal only), autonomous mode) later narrowed the extension registrar acceptance wording in response to review and updated this description on behalf of @mnriem.

🤖 Generated with Claude Code

@Yash-Chindam
Yash-Chindam requested a review from mnriem as a code owner September 9, 2026 04:52
@mnriem
mnriem requested a balanced review from Copilot September 9, 2026 17:05
@mnriem mnriem added the triage-must-have Verdict: high-value, important work for Spec Kit — do first label Sep 9, 2026

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

Resolver escaping and Windows output issues can break execution, and PowerShell behavior lacks parity coverage.

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

Pull request overview

Adds an opt-in bundled github extension as the stage-one replacement for core task-to-issue functionality.

Changes:

  • Adds the namespaced command and three resolver runtimes.
  • Registers, packages, documents, and tests the extension.
  • Preserves the existing core command and hook contract.
File summaries
File Description
extensions/github/extension.yml Defines extension metadata and command.
extensions/github/commands/speckit.github.taskstoissues.md Implements task-to-issue workflow.
extensions/github/scripts/bash/resolve-tasks.sh Adds Bash task resolver.
extensions/github/scripts/powershell/resolve-tasks.ps1 Adds PowerShell task resolver.
extensions/github/scripts/python/resolve_tasks.py Adds Python task resolver.
extensions/github/README.md Documents usage and migration.
extensions/catalog.json Registers the bundled extension.
pyproject.toml Includes extension in wheels.
README.md Announces the migration.
docs/installation.md Updates installed-command guidance.
tests/extensions/github/__init__.py Marks the test package.
tests/extensions/github/test_github_extension.py Tests layout, installation, rendering, and behavior.
tests/test_ps1_encoding.py Extends PowerShell encoding coverage.
Review details
  • Files reviewed: 12/13 changed files
  • Comments generated: 5
  • Review effort level: Balanced

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread extensions/github/scripts/bash/resolve-tasks.sh
Comment thread extensions/github/scripts/python/resolve_tasks.py Outdated
Comment thread extensions/catalog.json
Comment thread extensions/github/README.md Outdated
Comment thread tests/extensions/github/test_github_extension.py
@mnriem mnriem added the author-awaiting Waiting on author response label Sep 9, 2026
@mnriem

mnriem commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Thanks — the shape is exactly right (github namespace, vendored scripts, plain frontmatter). The re-review found real bugs in the vendored scripts to fix before merge: (1) resolve-tasks.sh json_escape doesn't escape backslashes and collapses newlines/tabs to letters instead of JSON escapes; (2) resolve_tasks.py emits U+2713 which raises UnicodeEncodeError under Windows cp1252 — use an ASCII marker or force UTF-8; (3) catalog.json top-level updated_at is stale; (4) the README invocation guide omits Command Code (also $speckit- skills syntax). Re-request once addressed.

Yash-Chindam added a commit to Yash-Chindam/spec-kit that referenced this pull request Sep 9, 2026
… scripts

Four review findings on github#4488, all confirmed against the code before
changing it.

resolve-tasks.sh json_escape had lost one level of backslash quoting
when the file was authored, so the escape table read `${s//\/\}` rather
than core's `${s//\/\\}`. Backslashes passed through unescaped and
\n/\t/\r collapsed to the bare letters n/t/r. A Windows feature path
such as C:\Users\dev\specs emitted JSON the agent cannot parse
("Invalid \escape"). Replaced with core's implementation from
scripts/bash/common.sh verbatim, which also adds \b, \f and the
\uXXXX control-character pass, and is now asserted equal to core's
output case by case.

resolve_tasks.py printed U+2713 unconditionally. On Windows stdout falls
back to the ANSI code page whenever it is not a console -- a pipe or a
redirect, which is how agents invoke these scripts -- so the report
died with UnicodeEncodeError right after "AVAILABLE_DOCS:". Adopted
core's _status_marker idiom: emit the glyph when stdout can encode it,
fall back to "[OK]", which is what the PowerShell twin already prints.

catalog.json updated_at was left at the assess extension's date; bumped
to this change's date, matching what the bug and assess additions did.

The README invocation guide named only Codex and ZCode for the
`$speckit-` syntax, omitting Command Code. A test now derives the
expected set from DOLLAR_SKILLS_AGENTS so a newly added agent fails
until the guide is updated.

Regression tests added for each: the json_escape cases round-trip
through a JSON parser and are compared against core's output, an
end-to-end run covers a feature directory containing a backslash, and
the text-mode tests pin both the cp1252 fallback and the UTF-8 glyph.
All were confirmed to fail against the pre-fix scripts.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Yash-Chindam

Copy link
Copy Markdown
Contributor Author

All four confirmed and fixed in 64915df. I reproduced each against the code before changing anything, and every fix has a regression test that I verified fails against the pre-fix scripts.

1. json_escape — you're right, and it's worse than a style issue.

The escape table had lost one level of backslash quoting when I authored the file. What shipped was:

s="${s//\/\\}"          # pattern is an escaped nothing; replacement collapses to \
s="${s//$'\n'/\n}"      # replacement \n means literal "n"

against core's:

s="${s//\\/\\\\}"
s="${s//$'\n'/\\n}"

So backslashes passed through unescaped and \n/\t/\r became the bare letters. The user-visible failure is a Windows feature path:

C:\Users\dev\specs  ->  "C:\Users\dev\specs"   json.JSONDecodeError: Invalid \escape

The agent gets unparseable JSON from step 1 of the command. I'd missed it because every fixture I tested used an MSYS forward-slash path, so no input ever contained a backslash.

Replaced with core's implementation from scripts/bash/common.sh verbatim — which also handles \b, \f and the \uXXXX control-character pass I had omitted entirely. The tests now assert each case round-trips through a JSON parser and is byte-identical to core's output, so the two can't drift.

Worth noting for the record: I had "verified" this function earlier by eyeballing a Python repr() of the source line and reading '\\' as two backslashes when it means one. That's what let a real bug through a check that looked like it passed.

2. U+2713 under cp1252 — confirmed, and core had already solved it.

print(f" \u2713 {doc}") dies with UnicodeEncodeError whenever stdout isn't a console, which is exactly how agents invoke these scripts:

$ PYTHONIOENCODING=cp1252 python resolve_tasks.py | cat
AVAILABLE_DOCS:
UnicodeEncodeError: 'charmap' codec can't encode character '\u2713'

The report dies right after AVAILABLE_DOCS:. I adopted core's _status_marker idiom from scripts/python/check_prerequisites.py — emit the glyph when stdout can encode it, fall back to [OK] otherwise. That also aligns the Python twin with the PowerShell one, which was already printing [OK]. Both branches are pinned by tests.

3. updated_at — bumped to 2026-09-10T00:00:00Z. I checked the history: the bug and assess additions each bumped it to their own date, so this matches the convention rather than guessing.

4. Command Code — right, DOLLAR_SKILLS_AGENTS is {codex, zcode, command-code} and the guide named only two. Fixed. Rather than just adding the string, the test now derives the expected set from DOLLAR_SKILLS_AGENTS, so if a fourth dollar-skills agent is added the test fails until the guide is updated.

One adjacent thing I did not touch, to avoid scope creep: extensions/agent-context/README.md:43 has the identical omission — my note was copied from it. Happy to fix it here if you'd prefer, or leave it for a separate change.


Verification. 92 tests in tests/extensions/github/. Extension suites (tests/extensions, test_ps1_encoding, test_extension_registration, test_extension_skills, test_extensions) → 806 passed, 237 skipped, plus the two test_scaffold_config_rejects_symlink* failures that also fail on main here (they need a symlink privilege this box doesn't grant).

One caveat I want to be explicit about rather than let CI discover: the 17 bash-gated tests skip on my machine and I could not execute them under pytest. Bare bash resolves to the WSL launcher through a Windows App Execution Alias that wins over PATH, so requires_bash correctly skips them. I verified those assertions by running the identical logic against Git Bash directly — all json_escape cases round-trip and match core, and the end-to-end backslash-path run produces parseable JSON — but their first run as pytest tests will be in CI.

Re-requesting review.

Disclosure: I used an AI assistant (Claude Code, model Claude Opus 5) to investigate and fix these and to draft this reply. Each finding was reproduced against the pre-fix code and each fix re-verified, including confirming the new tests fail without the fixes.

@Yash-Chindam
Yash-Chindam force-pushed the feat/4421-github-extension branch from 64915df to 36a4745 Compare September 12, 2026 08:30
Yash-Chindam added a commit to Yash-Chindam/spec-kit that referenced this pull request Sep 12, 2026
… scripts

Four review findings on github#4488, all confirmed against the code before
changing it.

resolve-tasks.sh json_escape had lost one level of backslash quoting
when the file was authored, so the escape table read `${s//\/\}` rather
than core's `${s//\/\\}`. Backslashes passed through unescaped and
\n/\t/\r collapsed to the bare letters n/t/r. A Windows feature path
such as C:\Users\dev\specs emitted JSON the agent cannot parse
("Invalid \escape"). Replaced with core's implementation from
scripts/bash/common.sh verbatim, which also adds \b, \f and the
\uXXXX control-character pass, and is now asserted equal to core's
output case by case.

resolve_tasks.py printed U+2713 unconditionally. On Windows stdout falls
back to the ANSI code page whenever it is not a console -- a pipe or a
redirect, which is how agents invoke these scripts -- so the report
died with UnicodeEncodeError right after "AVAILABLE_DOCS:". Adopted
core's _status_marker idiom: emit the glyph when stdout can encode it,
fall back to "[OK]", which is what the PowerShell twin already prints.

catalog.json updated_at was left at the assess extension's date; bumped
to this change's date, matching what the bug and assess additions did.

The README invocation guide named only Codex and ZCode for the
`$speckit-` syntax, omitting Command Code. A test now derives the
expected set from DOLLAR_SKILLS_AGENTS so a newly added agent fails
until the guide is updated.

Regression tests added for each: the json_escape cases round-trip
through a JSON parser and are compared against core's output, an
end-to-end run covers a feature directory containing a backslash, and
the text-mode tests pin both the cp1252 fallback and the UTF-8 glyph.
All were confirmed to fail against the pre-fix scripts.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Yash-Chindam

Copy link
Copy Markdown
Contributor Author

CI was red on all five platforms — 8 failures, both causes mine. Fixed in 36a4745.

1. The json_escape parity tests had never actually run.

FileNotFoundError: .../test_matches_core_json_escape_0/ours/value.txt

The helper wrote its payload into a per-script subdirectory of tmp_path that nothing created, so all seven parametrised cases died before reaching an assertion. They passed locally only because they never executed: bare bash resolves to the WSL launcher on my machine, so requires_bash skipped them and CI was their first real run. I flagged that risk in my last comment — this is exactly it landing. One-line fix (tmp_path.mkdir(parents=True, exist_ok=True)).

2. The parity test caught real drift, which is what it's for.

assert 14 == 10

After this branch was cut, #4456 changed core's taskstoissues to report an unreadable extensions.yml instead of skipping hooks silently — in both the before and after hook blocks. The extension copy still carried the old wording, so the diff against core grew from 10 lines to 14.

I ported both lines rather than relaxing the assertion. That's the duplicate-behaviour-surface risk called out in the assessment, and the test is the thing that stops it drifting silently — so it should stay strict. Rebased onto main; the diff is back to the intended 10 lines (three script paths, plus the two that read the new TASKS value).

Closing the gap that let #1 through. Windows' CreateProcess searches System32 before PATH, so no PATH ordering can point a bare bash at Git Bash — which is why requires_bash exists and why my earlier "verified by hand" check ran a standalone harness rather than the test bodies, and so missed a bug in the fixture setup. I now run pytest with a small local plugin that rewrites only argv[0] from bash to Git Bash, leaving the test bodies, fixtures and assertions untouched. With that, all 92 github tests execute: 91 passed, 1 skipped — the skip is the backslash-in-a-filename case, which NTFS rejects and the test skips explicitly.

Extension suites under normal gating: 836 passed, 237 skipped, plus the two test_scaffold_config_rejects_symlink* failures that fail identically on main here (they need a symlink privilege this box doesn't grant).

PR body updated with the current numbers.

One thing needing your hand: the workflow runs for 36a4745 are sitting at action_required — fork PRs need a maintainer to approve each run, so CI hasn't re-run yet. Could you kick it off when you get a moment?

Disclosure: I used an AI assistant (Claude Code, model Claude Opus 5) to diagnose and fix these and to draft this reply.

@mnriem

mnriem commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Thanks—the four requested corrections and the subsequent fixture/core-parity fixes are present. The branch now conflicts with main in README.md; please rebase.

The remaining review gap is committed PowerShell behavioral coverage. Please turn the manually verified JSON output, single-element document array, and missing-tasks.md scenarios into automated parity tests, using the existing PowerShell detection pattern. No broader resolver audit is needed.

Your current CI runs do require maintainer approval. After the update, the fresh-head runs will need approval before final re-review.

Drafted for @mnriem by GitHub Copilot (model: GPT-6 Astra).

@mnriem mnriem added author-needs-tests Real change but missing a regression test — add one that fails before / passes after author-needs-rebase Branch conflicts with main — rebase/resolve before merge labels Sep 15, 2026
Yash-Chindam added a commit to Yash-Chindam/spec-kit that referenced this pull request Sep 15, 2026
… scripts

Four review findings on github#4488, all confirmed against the code before
changing it.

resolve-tasks.sh json_escape had lost one level of backslash quoting
when the file was authored, so the escape table read `${s//\/\}` rather
than core's `${s//\/\\}`. Backslashes passed through unescaped and
\n/\t/\r collapsed to the bare letters n/t/r. A Windows feature path
such as C:\Users\dev\specs emitted JSON the agent cannot parse
("Invalid \escape"). Replaced with core's implementation from
scripts/bash/common.sh verbatim, which also adds \b, \f and the
\uXXXX control-character pass, and is now asserted equal to core's
output case by case.

resolve_tasks.py printed U+2713 unconditionally. On Windows stdout falls
back to the ANSI code page whenever it is not a console -- a pipe or a
redirect, which is how agents invoke these scripts -- so the report
died with UnicodeEncodeError right after "AVAILABLE_DOCS:". Adopted
core's _status_marker idiom: emit the glyph when stdout can encode it,
fall back to "[OK]", which is what the PowerShell twin already prints.

catalog.json updated_at was left at the assess extension's date; bumped
to this change's date, matching what the bug and assess additions did.

The README invocation guide named only Codex and ZCode for the
`$speckit-` syntax, omitting Command Code. A test now derives the
expected set from DOLLAR_SKILLS_AGENTS so a newly added agent fails
until the guide is updated.

Regression tests added for each: the json_escape cases round-trip
through a JSON parser and are compared against core's output, an
end-to-end run covers a feature directory containing a backslash, and
the text-mode tests pin both the cp1252 fallback and the UTF-8 glyph.
All were confirmed to fail against the pre-fix scripts.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Yash-Chindam
Yash-Chindam force-pushed the feat/4421-github-extension branch from 36a4745 to 0d91825 Compare September 15, 2026 17:27
@Yash-Chindam

Copy link
Copy Markdown
Contributor Author

Both done in 0d91825:

  • Rebased onto main. The README.md conflict came from docs: simplify README around three processes #4591 removing the commands table. I took main's README as it is, and moved the migration note next to /speckit.taskstoissues in docs/reference/agentic-sdd.md.
  • PowerShell parity tests. I added TestResolveTasksPowerShellParity, gated on HAS_POWERSHELL from tests.parity_helpers. It runs resolve-tasks.ps1 -Json next to the Python twin and covers:
    • JSON output: same keys, key order, paths and docs.
    • A single-document AVAILABLE_DOCS: it stays the JSON array ["tasks.md"].
    • A missing tasks.md: both exit 1 with tasks.md not found, and PowerShell prints nothing on stdout.

Locally, tests/extensions/github plus tests/test_ps1_encoding.py gives 106 passed and 1 skipped (the NTFS-only case). The new PowerShell tests ran against Windows PowerShell 5.1. The workflow runs for the new head will need your approval before CI starts.

@mnriem
mnriem requested a balanced review from Copilot September 17, 2026 10:50
@mnriem mnriem removed author-awaiting Waiting on author response author-needs-rebase Branch conflicts with main — rebase/resolve before merge labels Sep 17, 2026
@mnriem mnriem removed the author-needs-tests Real change but missing a regression test — add one that fails before / passes after label Sep 17, 2026

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

Python JSON output fails for some non-ASCII Windows paths, and dependency metadata and uninstall coverage need correction.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 12/13 changed files
  • Comments generated: 4
  • Review effort level: Balanced (auto)

Note

Copilot is running an experiment and ran this review at Balanced.

Comment thread extensions/github/extension.yml Outdated
Comment thread extensions/github/scripts/python/resolve_tasks.py Outdated
Comment thread tests/extensions/github/test_github_extension.py Outdated
Comment thread extensions/github/README.md Outdated
@Yash-Chindam

Copy link
Copy Markdown
Contributor Author

All four Copilot findings addressed in d1559e2. I checked each against the code before changing anything, and all four were correct.

Tool requirements understated the command's dependencies. The command always runs git config --get remote.origin.url and cannot list or create issues without the GitHub MCP server, yet git was required: false and the MCP server was not declared at all. Both are now required: true, and the MCP server entry includes an install_url. requires.tools is only displayed by specify extension info and is not enforced at install, so install behaviour does not change. test_declares_its_mandatory_external_tools pins both entries.

--json crashed on a non-ASCII feature path under a legacy stdout. Reproduced: with a specs/001-功能 feature and PYTHONIOENCODING=cp1252, which is what a redirected pipe gets on Windows, the resolver raised UnicodeEncodeError. Because the rendered command always passes --json, this was the real invocation path. JSON mode now uses the default ensure_ascii=True. The escapes decode to the same path, so the parsed payload is unchanged. I added a JSON-mode regression next to the existing text-mode encoding test. It fails on the previous script and passes now.

The uninstall test could not catch leftover artifacts. It installed with register_commands=False, so no command or skill file ever existed to be left behind. test_remove_deletes_registered_artifacts_and_keeps_core now installs with registration in both layouts:

  • Copilot command mode: .agent.md and .prompt.md
  • Claude skills mode: SKILL.md

It asserts each namespaced artifact is created and then deleted on remove, and that a coexisting core speckit.taskstoissues artifact is left byte-for-byte intact.

The README claimed full behavioural parity. That contradicted the Scripts section: the resolver deliberately drops core's plan.md gate, so this command can proceed where the core one stops. The README now claims only that issue creation is unchanged (remote validation, deduplication, titles, hook contract) and states the feature-resolution difference, with a link to the Scripts section.

tests/extensions/github + tests/test_ps1_encoding.py: 110 passed, 1 skipped (the NTFS-only case). ruff@0.15.0 check src tests is clean. main has moved 19 commits since the rebase, including changes to extensions/__init__.py and agents.py. I test-merged it rather than assume: no conflicts, and tests/extensions/github plus tests/test_extensions.py came out at 639 passed. The only 2 failures were the WinError 1314 symlink-privilege cases from my Windows environment, which also fail on unmodified main.

The workflow runs for d1559e2 are at action_required, so they have not run yet. Could you approve them when you get a chance?

Disclosure: this comment and d1559e2 were written by Claude Code (model: Claude Opus 5), acting on behalf of @Yash-Chindam.

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 review overview

🔵 Needs a closer look

The cross-platform resolver duplication and broad integration surface warrant final human review despite extensive coverage.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Resolved since last review (1)

Assisted-by: GitHub Copilot (model: GPT-5.6 Sol Fast (Internal only), autonomous)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@mnriem

mnriem commented Sep 28, 2026

Copy link
Copy Markdown
Collaborator

Addressed this review round in 73c718ae:

  • Narrowed the helper, constant, test name, and acceptance wording to the extension command registrar scope.
  • Retained the explicit assertion that generic is excluded because it has no static extension registrar configuration; the extension README and integration reference already disclose that add-ons are not registered for generic projects.

Validation: tests/extensions/github/test_github_extension.py passes with 198 passed and 1 skipped; the full suite collects 8,810 tests.

AI disclosure: Posted on behalf of @mnriem by GitHub Copilot (GPT-5.6 Sol Fast (Internal only), autonomous mode). Copilot authored the scoped test wording changes and ran the reported validation.

Resolve the extensions catalog conflict by retaining the latest catalog metadata and the bundled GitHub extension entry.

Assisted-by: GitHub Copilot (model: GPT-5.6 Sol Fast (Internal only), autonomous)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@mnriem

mnriem commented Sep 28, 2026

Copy link
Copy Markdown
Collaborator

Resolved the main merge conflict in 04efb714:

  • Merged current upstream main.
  • Resolved extensions/catalog.json by retaining main's newer catalog timestamp and agent-context version while preserving this PR's bundled github entry.
  • Removed the hard-coded integration count from the PR description because main added the mcode integration; the acceptance statement now tracks every entry in AGENT_CONFIGS.

Validation: the GitHub extension and agent configuration suites pass with 230 passed and 1 skipped; the repository collects 8,944 tests.

AI disclosure: Posted on behalf of @mnriem by GitHub Copilot (GPT-5.6 Sol Fast (Internal only), autonomous mode). Copilot performed the merge, resolved the catalog conflict, updated the description, and ran the reported validation.

Yash-Chindam added a commit to Yash-Chindam/spec-kit that referenced this pull request Sep 28, 2026
… scripts

Four review findings on github#4488, all confirmed against the code before
changing it.

resolve-tasks.sh json_escape had lost one level of backslash quoting
when the file was authored, so the escape table read `${s//\/\}` rather
than core's `${s//\/\\}`. Backslashes passed through unescaped and
\n/\t/\r collapsed to the bare letters n/t/r. A Windows feature path
such as C:\Users\dev\specs emitted JSON the agent cannot parse
("Invalid \escape"). Replaced with core's implementation from
scripts/bash/common.sh verbatim, which also adds \b, \f and the
\uXXXX control-character pass, and is now asserted equal to core's
output case by case.

resolve_tasks.py printed U+2713 unconditionally. On Windows stdout falls
back to the ANSI code page whenever it is not a console -- a pipe or a
redirect, which is how agents invoke these scripts -- so the report
died with UnicodeEncodeError right after "AVAILABLE_DOCS:". Adopted
core's _status_marker idiom: emit the glyph when stdout can encode it,
fall back to "[OK]", which is what the PowerShell twin already prints.

catalog.json updated_at was left at the assess extension's date; bumped
to this change's date, matching what the bug and assess additions did.

The README invocation guide named only Codex and ZCode for the
`$speckit-` syntax, omitting Command Code. A test now derives the
expected set from DOLLAR_SKILLS_AGENTS so a newly added agent fails
until the guide is updated.

Regression tests added for each: the json_escape cases round-trip
through a JSON parser and are compared against core's output, an
end-to-end run covers a feature directory containing a backslash, and
the text-mode tests pin both the cp1252 fallback and the UTF-8 glyph.
All were confirmed to fail against the pre-fix scripts.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Yash-Chindam
Yash-Chindam force-pushed the feat/4421-github-extension branch from 04efb71 to b3186a6 Compare September 28, 2026 16:00
@mnriem
mnriem requested a balanced review from Copilot September 28, 2026 16:01
@Yash-Chindam
Yash-Chindam force-pushed the feat/4421-github-extension branch from b3186a6 to 04efb71 Compare September 28, 2026 16:01
@Yash-Chindam

Copy link
Copy Markdown
Contributor Author

Nothing outstanding from me this round, but one thing on the branch needs disclosing.

I briefly force-pushed over your merge commit, and then restored it. I saw the PR conflicting with main, rebased locally, and pushed with --force-with-lease. In the minutes between my fetch and that push you had pushed 04efb71 (the upstream merge), so the lease was satisfied against a ref I had only just learned about and the rebase replaced your merge. I checked before doing anything else: git diff 04efb71 <my rebase> was empty — identical trees, same conflict resolution on extensions/catalog.json (updated_at kept at main's newer value), and every one of your commits present. No content was lost. I have force-pushed 04efb71 back, so the branch head is exactly the commit you pushed, and I have nothing of my own outstanding on top of it. My apologies for the churn — the rebase was unnecessary given you had already merged, and I should have re-checked the remote immediately before pushing rather than relying on the lease.

Verified on the restored head 04efb71: level with main, no conflicts, floor still >=0.12.17, tests/extensions/github + tests/test_ps1_encoding.py at 207 passed / 5 skipped (the NTFS case and the POSIX-only permission cases), ruff@0.15.0 check src tests clean.

On the review state: c7812d1 had a full green CI run before the merge, including all six pytest matrix jobs. The remaining Copilot thread is the generic one, which your 019fc2c and 73c718a addressed in docs and tests, so it looks like an unresolved thread rather than outstanding work. The merge head needs its own CI approval.

Also, for the record on a process point I raised earlier: the eight oldest commits on this branch carry only Co-Authored-By: and not the Assisted-by: trailer. Rewriting them is blocked in my sandbox, and after this round I would rather not touch the branch history again unless you ask for it.

Disclosure: this comment was written by Claude Code (model: Claude Opus 5, autonomous), acting on behalf of @Yash-Chindam.

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 review overview

🔵 Needs a closer look

The catalog’s top-level update timestamp must be refreshed for the new bundled entry.

Review effort: Balanced
Findings: None

Resolved since last review (1)

Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@mnriem

mnriem commented Sep 28, 2026

Copy link
Copy Markdown
Collaborator

Updated extensions/catalog.json’s updated_at to 2026-09-28T16:20:28Z in commit 04030cf. The JSON parses and the commit changes only that timestamp; the bundled github entry remains intact.

AI disclosure: Posted on behalf of @mnriem by GitHub Copilot (model: GPT-6 Sol, autonomous mode). The agent authored and pushed the timestamp-only commit, verified the diff and JSON, and wrote this comment. No human verification is claimed.

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 review overview

🔵 Needs a closer look

The published documentation has a broken link, and several resolver tests inherit environment overrides that make them nondeterministic.

Review effort: Balanced
Findings: None

Previously missed (2)

In code that hasn't changed since last review

Medium severity Resolver tests inherit caller environment overrides

tests/​extensions/​github/​test_github_extension.py:654

This helper inherits SPECIFY_FEATURE_DIRECTORY and SPECIFY_INIT_DIR from the pytest process, so its fixture-based tests can resolve an unrelated caller feature and fail (or modify the fixture's feature.json). Use the sanitizer already defined below for deterministic resolver tests.

This issue also appears in the following locations of the same file:

  • line 686
  • line 722
  • line 744
Low severity Relative link points to a file excluded from DocFX output

docs/​installation.md:155

This relative link targets a file that DocFX does not include in the published site (docs/docfx.json:3-24), so it resolves to a 404 after deployment. Use the repository URL, as the other extension links in docs/reference/agentic-assessment.md:101 and docs/reference/agentic-sdd.md:176 do.

Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@mnriem

mnriem commented Sep 28, 2026

Copy link
Copy Markdown
Collaborator

Addressed the review in 849b309. All resolver invocations in the GitHub extension tests now strip inherited feature/project overrides, with a regression test demonstrating the previous failure. The installation guide now links to the extension README in the repository rather than a file excluded from DocFX. The extension suite passed for all non-PowerShell cases (163 passed); the affected resolver tests also passed with hostile inherited overrides (86 passed). Local pwsh crashes on startup even for Write-Output probe, so its cases could not be verified here.

On behalf of @mnriem, GitHub Copilot (model: GPT-6 Sol, autonomous mode) authored the test and documentation changes, ran the checks, and committed and pushed the fix. No human line-by-line review is claimed.

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 review overview

🔵 Needs a closer look

Positive cross-runtime coverage is missing for several optional-document reporting branches.

Review effort: Balanced
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Low severity Expand parity test coverage to all optional artifacts

tests/​extensions/​github/​test_github_extension.py:1166

This cross-runtime parity test only exercises research.md and tasks.md, so the new data-model.md, non-empty contracts/, and quickstart.md reporting branches can drift in any twin without failing the suite. Populate those optional artifacts here and assert the complete ordered list.

Assisted-by: GitHub Copilot (model: GPT-6 Sol, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@mnriem

mnriem commented Sep 28, 2026

Copy link
Copy Markdown
Collaborator

On behalf of @mnriem, GitHub Copilot (GPT-6 Sol, autonomous mode) authored the test change and this comment without human line-by-line review. Commit 25f36fe extends the cross-runtime complete-feature case to create data-model.md, a non-empty contracts/ directory, and quickstart.md, then assert the full ordered AVAILABLE_DOCS list. The extension suite passed 163 cases with PowerShell cases excluded; this host’s pwsh crashes on a trivial Write-Output command before it can run any script, so PowerShell validation remains unavailable 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.

Copilot review overview

🔵 Needs a closer look

Cross-platform resolver behavior and broad integration registration warrant final human validation despite comprehensive tests.

Review effort: Balanced
Findings: None

@mnriem

mnriem commented Sep 29, 2026

Copy link
Copy Markdown
Collaborator

Thank you!

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.

3 participants