Repository navigation
feat: Copilot skills, dynamic test matrix, validate-feature script, and dev tooling improvements - #9
Conversation
Agent-Logs-Url: https://github.com/savvy-web/devcontainers/sessions/3f4f4847-225d-44c2-b584-7f3f747c259a Co-authored-by: spencerbeggs <109114+spencerbeggs@users.noreply.github.com>
Agent-Logs-Url: https://github.com/savvy-web/devcontainers/sessions/afb28ecd-3954-4935-a44d-6dfd59470e5b Co-authored-by: spencerbeggs <109114+spencerbeggs@users.noreply.github.com>
Agent-Logs-Url: https://github.com/savvy-web/devcontainers/sessions/e92d4aa5-605b-46a1-b98a-80f9a101b946 Co-authored-by: spencerbeggs <109114+spencerbeggs@users.noreply.github.com>
…E instructions, validate-feature script, dynamic test matrix, fix copilot-setup-steps, rewrite CONTRIBUTING+CLAUDE Agent-Logs-Url: https://github.com/savvy-web/devcontainers/sessions/c91e2780-f131-4d44-b71b-debae75de5f5 Co-authored-by: spencerbeggs <109114+spencerbeggs@users.noreply.github.com>
Agent-Logs-Url: https://github.com/savvy-web/devcontainers/sessions/c91e2780-f131-4d44-b71b-debae75de5f5 Co-authored-by: spencerbeggs <109114+spencerbeggs@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds Copilot-native scaffolding + repo conventions for devcontainer feature authoring, and updates CI/dev tooling to validate and test features more reliably.
Changes:
- Introduces Copilot skills/instructions and reference docs to standardize feature scaffolding, version bumps, and debugging.
- Adds
validate-feature+test:featurescripts and switches CI to a dynamically discovered feature test matrix. - Adds/updates several feature install/test assets (including new global features) and refreshes repo documentation.
Reviewed changes
Copilot reviewed 34 out of 45 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| test/node/pnpm/test.sh | Adds [PASS] marker to pnpm feature test |
| test/global/zig/test.sh | Updates Zig test success marker |
| test/global/rust/test.sh | Adds Rust feature test script |
| test/global/outbound-firewall/test.sh | Adds outbound firewall feature test script |
| test/global/homebrew/test.sh | Adds Homebrew feature test script |
| test/global/homebrew/scenarios.json | Converts Homebrew scenarios to the repo’s array format |
| test/global/claude-code/test.sh | Adds [PASS] marker to Claude Code test |
| test/global/biome/test.sh | Adds Biome feature test script |
| test/global/act/test.sh | Adds act feature test script |
| test/global/act/scenarios.json | Adds act scenarios.json |
| scripts/validate-feature.sh | Adds feature structure and conventions validator |
| scripts/test-feature.sh | Adds local “test one feature via act” helper |
| package.json | Exposes test:feature and validate-feature scripts |
| features/node/pnpm/install.sh | Adds Node.js + pnpm install implementation |
| features/node/README.md | Adds scope documentation for Node feature group |
| features/global/zig/install.sh | Adds Zig installer |
| features/global/rust/install.sh | Adds Rust installer |
| features/global/outbound-firewall/install.sh | Adds outbound firewall installer |
| features/global/homebrew/install.sh | Adds Homebrew installer |
| features/global/homebrew/devcontainer-feature.json | Adds missing name field |
| features/global/claude-code/install.sh | Adds Claude Code installer |
| features/global/biome/install.sh | Adds Biome installer |
| features/global/act/install.sh | Adds act installer |
| features/global/act/devcontainer-feature.json | Adds act feature metadata |
| features/global/README.md | Adds global feature table + scope guidance |
| docs/features/act.md | Adds act end-user documentation |
| README.md | Replaces template README with devcontainer-features overview |
| CONTRIBUTING.md | Rewrites contribution guide for devcontainer features workflow |
| CLAUDE.md | Rewrites agent guidance for this repo’s feature conventions |
| .github/workflows/test.yml | Switches to dynamic test discovery and adds install+test steps |
| .github/workflows/test-feature.yml | Adds workflow used by act for local single-feature testing |
| .github/workflows/copilot-setup-steps.yml | Completes Copilot agent setup (devcontainer CLI, shellcheck, verification) |
| .github/skills/new-feature/SKILL.md | Adds guided 5-file feature scaffold skill |
| .github/skills/devcontainer/references/topic-map.md | Adds curated links map for containers.dev docs |
| .github/skills/devcontainer/references/test-patterns.md | Documents repo test conventions (test.sh + scenarios.json) |
| .github/skills/devcontainer/references/feature-anatomy.md | Documents feature JSON/install conventions + examples |
| .github/skills/devcontainer/references/doc-template.md | Adds docs template + style rules |
| .github/skills/devcontainer/SKILL.md | Adds devcontainer expert skill definition |
| .github/skills/debug-feature/references/common-failure-modes.md | Adds structured debugging reference |
| .github/skills/debug-feature/SKILL.md | Adds debug skill definition |
| .github/skills/bump-feature/SKILL.md | Adds version-bump checklist skill |
| .github/scripts/collect-test-dirs.js | Adds dynamic test directory discovery for CI matrix |
| .github/instructions/SHELL.instructions.md | Adds auto-applied shell script conventions |
| .github/instructions/FEATURE.instructions.md | Adds auto-applied feature editing/versioning rules |
| .actrc | Configures act runner image + bind mount + cleanup |
| zig version | ||
|
|
||
| echo "[SUCCESS] Zig validated." | ||
| echo "[PASS] Zig install test passed." |
There was a problem hiding this comment.
This test doesn’t assert the installed Zig version matches the pinned default (zigVersion in features/global/zig/devcontainer-feature.json). Adding a grep against the default version makes the test catch regressions where the download URL changes or a different version is installed.
| - name: Install feature | ||
| env: | ||
| FEATURE_SCOPE: ${{ inputs.scope }} | ||
| FEATURE_ID: ${{ inputs.id }} | ||
| run: | | ||
| INSTALL_SCRIPT="features/${FEATURE_SCOPE}/${FEATURE_ID}/install.sh" | ||
| if [[ ! -f "$INSTALL_SCRIPT" ]]; then | ||
| echo "[ERROR] Install script not found: $INSTALL_SCRIPT" >&2 | ||
| exit 1 | ||
| fi | ||
| chmod +x "$INSTALL_SCRIPT" | ||
| bash "$INSTALL_SCRIPT" | ||
|
|
||
| - name: Run tests | ||
| env: | ||
| FEATURE_SCOPE: ${{ inputs.scope }} | ||
| FEATURE_ID: ${{ inputs.id }} | ||
| run: | | ||
| TEST_SCRIPT="test/${FEATURE_SCOPE}/${FEATURE_ID}/test.sh" | ||
| if [[ ! -f "$TEST_SCRIPT" ]]; then | ||
| echo "[ERROR] Test script not found: $TEST_SCRIPT" >&2 | ||
| exit 1 | ||
| fi | ||
| chmod +x "$TEST_SCRIPT" | ||
| bash "$TEST_SCRIPT" |
There was a problem hiding this comment.
The install/test steps run scripts as the default runner user. Several features assume root (write to /usr/local, modify /etc, iptables). Run these steps with sudo (or use the devcontainer CLI to test inside a container) to avoid permission failures and better match real feature execution.
| if ! command -v pnpm &>/dev/null; then | ||
| echo "[FAIL] pnpm not found in PATH" | ||
| exit 1 | ||
| fi | ||
|
|
||
| pnpm --version | ||
|
|
||
| echo "[PASS] Node.js + pnpm install test passed." |
There was a problem hiding this comment.
The new repository test conventions expect tests to assert versions pinned in devcontainer-feature.json (see .github/skills/devcontainer/references/test-patterns.md). This test only checks pnpm exists and prints the version, so it would miss mismatches. Add a grep assertion for the default pnpm (and Node) versions, and send failure messages to stderr (>&2).
| - name: Run tests | ||
| env: | ||
| FEATURE_SCOPE: ${{ matrix.scope }} | ||
| FEATURE_ID: ${{ matrix.id }} | ||
| run: | | ||
| TEST_SCRIPT="test/${FEATURE_SCOPE}/${FEATURE_ID}/test.sh" | ||
| if [[ ! -f "$TEST_SCRIPT" ]]; then | ||
| echo "[ERROR] Test script not found: $TEST_SCRIPT" >&2 | ||
| exit 1 | ||
| fi | ||
| chmod +x "$TEST_SCRIPT" | ||
| bash "$TEST_SCRIPT" |
There was a problem hiding this comment.
Some feature tests need elevated privileges (e.g., iptables inspection) and/or depend on root-only installs. Consider running the test step under sudo as well (or run both install+test inside a container via the devcontainer CLI) so the CI environment matches how features run in devcontainers.
| if [[ -f "$TEST_SH" ]]; then | ||
| FIRST_LINE=$(head -1 "$TEST_SH") | ||
| if [[ "$FIRST_LINE" == "#!/usr/bin/env bash" ]]; then | ||
| pass "test.sh shebang is correct" | ||
| else | ||
| fail "test.sh first line must be '#!/usr/bin/env bash', got: ${FIRST_LINE}" | ||
| fi | ||
|
|
||
| if grep -q "set -euo pipefail" "$TEST_SH"; then | ||
| pass "test.sh has 'set -euo pipefail'" | ||
| else | ||
| fail "test.sh is missing 'set -euo pipefail'" | ||
| fi | ||
|
|
||
| if grep -q "\[PASS\]" "$TEST_SH"; then | ||
| pass "test.sh has a [PASS] marker" | ||
| else | ||
| fail "test.sh is missing a [PASS] marker at the end" | ||
| fi | ||
| fi |
There was a problem hiding this comment.
validate-feature.sh checks that install.sh is executable, but does not verify the executable bit for test.sh (even though CI and local tooling may execute it directly). Consider adding a -x check for test/<scope>/<id>/test.sh to catch permission regressions early.
There was a problem hiding this comment.
Already applied in e80cd8a — validate-feature.sh line 183 has [[ -x "$TEST_SH" ]] at the start of the test.sh structural checks block.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: C. Spencer Beggs <spencer@beg.gs>
|
Copilot apply changes based on the comments in this thread |
… validate test.sh exec bit, fix instructions Agent-Logs-Url: https://github.com/savvy-web/devcontainers/sessions/0a7a8179-654e-4aa5-923b-13b29b752a14 Co-authored-by: spencerbeggs <109114+spencerbeggs@users.noreply.github.com>
Applied all changes from the review in e80cd8a:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 45 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
.github/workflows/test.yml:42
actions/checkout@v6is not a valid release of actions/checkout, so this workflow will fail to load the action. Pin to an existing major version (e.g.actions/checkout@v4).
- name: Checkout repository
uses: actions/checkout@v6
|
Copilot apply changes based on the comments in this thread |
All three changes from that review are already in place from e80cd8a:
No further changes needed. |
All changes from that review are already in place from e80cd8a: version assertions in
All changes from that review are already in place from e80cd8a — |
…umentationURL convention Agent-Logs-Url: https://github.com/savvy-web/devcontainers/sessions/25fe07a5-bf6a-4aba-9591-6f6b48a0df56 Co-authored-by: spencerbeggs <109114+spencerbeggs@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 47 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
scripts/validate-feature.sh:1
- The validator currently accepts
set -euo pipefailanywhere in the file, but the repo convention (and SHELL.instructions.md) requires it to be exactly the second line. This can let structurally non-conformant scripts pass validation. Consider validating the second line explicitly (similar to the shebang check) rather than usinggrep.
scripts/validate-feature.sh:1 - Same issue as
install.sh: the validator checks forset -euo pipefailanywhere intest.sh, but the convention requires it on line 2. Enforcing the exact second line will makevalidate-featurea stronger guardrail and prevent drift.
scripts/validate-feature.sh:1 - After detecting invalid JSON (line 137–141), the script still proceeds to parse the JSON repeatedly for required-field checks, semver checks, and documentationURL checks. This creates noisy cascaded failures that obscure the root cause. A more maintainable approach is to only run the subsequent JSON-dependent checks if the JSON parse succeeded (e.g., set a
JSON_VALID=1flag or parse once and reuse).
All changes from that review are already in place from e80cd8a: version assertions in
All changes from that review are already in place from e80cd8a — |
The repo lacked Copilot-native scaffolding for the most repetitive and error-prone devcontainer workflows, had a hardcoded test matrix with two broken paths, and its
CONTRIBUTING.md/CLAUDE.mdstill contained Node.js package template boilerplate.New Copilot Skills
new-feature— guided 5-file scaffold (JSON manifest,install.sh,test.sh,scenarios.json,docs/) with per-file conventions and a completion checklistbump-feature— atomic version bump guide; ensuresdevcontainer-feature.json,test.shassertions,docs/snippets, and option defaults move togetherdebug-feature+references/common-failure-modes.md— 9-category triage guide (env var injection, arch mismatch, broken URLs, permissions, silent failures, idempotency, version mismatches, wrong scope, Docker/act) with diagnostic commands and canonical fixes per categoryNew Instruction Files
SHELL.instructions.md(applyTo: **/*.sh) — auto-appliesset -euo pipefail, log prefix conventions, arch normalization, and install validation whenever Copilot touches a shell scriptFEATURE.instructions.md(applyTo: features/**) — auto-enforces the version bump rule, atomic update rule, five-file completeness, anddocumentationURLconvention on every feature editWorkflow Fixes
test.yml— replaced hardcoded matrix (contained two broken paths:test/node/node-pnpm,test/node/claude-code) with dynamic discovery via newcollect-test-dirs.js; added the previously missing install + test execution steps; scopedpermissions: contents: readcopilot-setup-steps.yml— was truncated afterSetup Runtime; added@devcontainers/cli,shellcheck, and a verify stepNew Scripts
scripts/validate-feature.sh/pnpm run validate-feature <scope>/<id>— validates 5-file completeness, executable bit, shebang,set -euo pipefail, valid JSON, required fields, semver format,documentationURLresolves to an existing file,[PASS]marker presence,scenarios.jsonis a non-empty array.github/scripts/collect-test-dirs.js— discovers alltest/<scope>/<id>/test.shfiles and emits a JSON matrix consumed bytest.ymlDocumentation
CONTRIBUTING.md/CLAUDE.md— full rewrites replacing pnpm module template boilerplate with devcontainer-specific content: five-file rule, version bump rule, available scripts, publish pipeline overviewfeatures/global/README.md/features/node/README.md— feature tables with scope assignment rulesPre-existing Bug Fixes (surfaced by
validate-feature)[PASS]markers intest/global/claude-code/test.sh,test/global/zig/test.sh,test/node/pnpm/test.shnamefield infeatures/global/homebrew/devcontainer-feature.jsontest/global/homebrew/scenarios.jsonwas a{ "scenarios": [...] }object instead of an arrayinstall.shandtest.shfiles lacked the executable bit in the git index (100644→100755)