Fix stale skill docs and repo-path guard for the action-based JS architecture - #57579
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the implementation label and has <=100 new lines of code in business logic directories.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped: No test files were added or modified in this PR. Changes are limited to skills documentation (.md), Makefile, and shell scripts (not in analyzed scope). Scope: Go (_test.go), JavaScript (*.test.cjs, *.test.js), Python, and Rust.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
The new skill-path guard does not actually cover all of the stale path shapes this PR introduces, so the repo can still merge invalid skill guidance while the new lint target reports success.
Blocking theme
The guard skips placeholder-style backticked paths instead of treating them as invalid or forcing a different notation, and this PR still adds one of those skipped pseudo-paths in temporary-id-safe-output/SKILL.md. That undercuts the main value of the change: catching stale documentation before it drifts further.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 34.4 AIC · ⌖ 7.43 AIC · ⊞ 23.5K
Comment /review to run again
Comments that could not be inline-anchored
.github/skills/temporary-id-safe-output/SKILL.md:101
This guard is too narrow to catch the stale placeholder path this PR still adds, so it creates a false sense of coverage while letting invalid skill guidance merge.
<details><summary>💡 Why this blocks the change</summary>
scripts/check-skill-file-paths.sh only validates a small allowlist of prefixes (pkg/, scripts/, internal/, cmd/, .github/skills/, actions/...) and silently skips everything else. But this PR still adds pkg/workflow/js/<job_name>.cjs in `.github/skills/tempor…
There was a problem hiding this comment.
One simplification is worth making in the new skill-path guard; it can be made smaller without changing behavior. net: -20 lines possible.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #57579 · codex · mai10 · 5.89 AIC · ⌖ 0.322 AIC · ⊞ 12.8K
Comment /ponytail to run again
| exit 1 | ||
| fi | ||
|
|
||
| # Extract backtick-delimited strings from SKILL.md files and validate the ones that |
There was a problem hiding this comment.
scripts/check-skill-file-paths.sh:L26-L84: shrink: the guard splits token extraction, path filtering, and existence checks across Bash, embedded Python, and a long whitelist. One smaller parser that emits the candidate paths and checks them directly would cut the duplicated logic and make the guard easier to follow.
There was a problem hiding this comment.
Review: Fix stale skill docs and repo-path guard
The changes are well-targeted and correct.
What was reviewed
- Skill doc updates (
developer-internals,error-pattern-safety,javascript-refactoring,messages,temporary-id-safe-output) replacing stale(go/redacted):embed-based patterns with current action-generation/build pipeline references - New
scripts/check-skill-file-paths.shguard +scripts/check-skill-file-paths_test.sh+ Makefile wiring
Findings
No blocking issues.
Minor: mixed indentation in JSON example (messages/SKILL.md)
The JSON schema example uses a mix of tabs and two-space indentation inconsistent with the surrounding file style. Low priority — JSON parsers are indifferent, but it may confuse readers.
Observation: Python subprocess per file in check-skill-file-paths.sh
The guard spawns a python3 subprocess per SKILL.md file for backtick extraction. With ~50 skill files this is fine today; a grep -oP or awk alternative would avoid overhead at scale. Not a blocker.
The guard script logic is sound: the bash and Python filters are slightly redundant but consistently correct. The three test scenarios (valid, invalid, missing directory) give solid baseline coverage.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 41 AIC · ⌖ 14.7 AIC · ⊞ 6.2K
There was a problem hiding this comment.
🟡 Changes recommended
Markdown fences bypass the new guard, and several rewritten skills still describe nonexistent paths or incorrect build behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates contributor skills for the action-based JavaScript architecture and adds validation for stale repository paths.
Changes:
- Revises JavaScript, message, validation, and temporary-ID skill guidance.
- Adds a skill-path checker with shell tests.
- Integrates the checker into linting and script tests.
File summaries
| File | Description |
|---|---|
scripts/check-skill-file-paths.sh |
Adds the path validation guard. |
scripts/check-skill-file-paths_test.sh |
Tests guard behavior and errors. |
Makefile |
Integrates the guard into lint and tests. |
.github/skills/temporary-id-safe-output/SKILL.md |
Corrects setup JavaScript paths. |
.github/skills/messages/SKILL.md |
Reworks message-authoring guidance. |
.github/skills/javascript-refactoring/SKILL.md |
Reframes JavaScript architecture guidance. |
.github/skills/error-pattern-safety/SKILL.md |
Updates validation guidance. |
.github/skills/developer-internals/SKILL.md |
Corrects validation file references. |
Review details
Suppressed comments (7)
.github/skills/messages/SKILL.md:68
- This parser guidance is inaccurate:
parseMessagesConfiglives inpkg/workflow/safe_outputs_messages_config.goand explicitly assigns every string field withextractStringFromMap; it is not reflection-wired. Following this instruction would leave a new field unset at runtime.
If the message needs custom parsing logic, update the workflow parser in `pkg/workflow/safe_outputs.go` or the relevant config block. Most simple string fields will be wired automatically by the existing reflection-based parser.
.github/skills/messages/SKILL.md:72
- Safe-output message modules are sourced from
actions/setup/js/;pkg/workflow/js/contains neither the current message modules nor their tests. This still directs the contributor into the dead source tree the PR is intended to remove.
Create the new module in the current shared JS location, typically `pkg/workflow/js/`:
.github/skills/messages/SKILL.md:142
- Both referenced files are actually under
actions/setup/js/; thepkg/workflow/js/versions do not exist. Update these exact paths so the core type and export steps are actionable.
Update the `SafeOutputMessages` typedef and the return object in `pkg/workflow/js/messages_core.cjs`, and re-export the message helper from `pkg/workflow/js/messages.cjs`.
.github/skills/messages/SKILL.md:152
actions/setup/js/is explicitly source-of-truth content copied bysetup.sh;buildSetupActiononly checks/counts these files and does not rebuild a bundle. Requiringmake actions-buildhere misstates how message changes ship and may make contributors think generated output was refreshed when none exists.
- keep the JS module in `pkg/workflow/js/` or the relevant action folder,
- update the action dependency map or action source if needed,
- rebuild the action bundle with `make actions-build`.
.github/skills/error-pattern-safety/SKILL.md:110
- No current validation helper implements these three protections: the repository has no
lastIndex === lastIndexguard or iteration warning/limit, and current agent-error detection uses non-globalRegExp.test()checks. Presenting the sample as an existing safety mechanism gives contributors a false guarantee.
The repo’s validation helpers include built-in protections for dangerous regex patterns:
1. **Zero-width detection**: Checks whether a regex stops advancing across iterations
2. **Iteration warning**: Warns when repeated runs approach a hang threshold
3. **Hard limit**: Stops execution before runaway loops can lock the process
.github/skills/messages/SKILL.md:109
- The matching tests are also under
actions/setup/js/; this example path does not exist underpkg/workflow/js/. Keeping it here would recreate the same dead-layer mistake for test placement.
Create a matching test file, for example `pkg/workflow/js/messages_my_new.test.cjs`:
.github/skills/messages/SKILL.md:185
- These references preserve the stale source paths, and
actions/README.mddescribes conventional generated actions rather than the setup action's directly copied scripts. Point this guide at the setup-specific source-of-truth documentation and files.
- `actions/README.md` - current action-generation/build workflow
- `pkg/workflow/js/messages_core.cjs` - shared safe-output message helpers
- `pkg/workflow/js/messages.cjs` - message exports
- Files reviewed: 8/8 changed files
- Comments generated: 5
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| import pathlib, re, sys | ||
| path = pathlib.Path(sys.argv[1]) | ||
| text = path.read_text(encoding='utf-8', errors='ignore') | ||
| for match in re.findall(r'`([^`]+)`', text): | ||
| if not match or '://' in match: | ||
| continue | ||
| if any(ch.isspace() for ch in match): | ||
| continue | ||
| if any(ch in match for ch in "()[]{}'\"<>*?"): | ||
| continue | ||
| if '...' in match: | ||
| continue | ||
| if match.startswith('@'): | ||
| match = match[1:] | ||
| if '/' in match or match.startswith('.'): | ||
| print(match) |
| if [[ "$candidate" == pkg/* ]]; then | ||
| : | ||
| elif [[ "$candidate" == scripts/* ]]; then | ||
| : | ||
| elif [[ "$candidate" == internal/* || "$candidate" == cmd/* || "$candidate" == eslint-factory/* ]]; then | ||
| : | ||
| elif [[ "$candidate" == .github/skills/* ]]; then | ||
| : | ||
| elif [[ "$candidate" =~ ^actions/.+\.(cjs|js|mjs|ts|md|yaml|yml)$ || "$candidate" =~ ^actions/.+/src/.+ || "$candidate" =~ ^actions/.+/index\.(cjs|js|mjs)$ ]]; then | ||
| : | ||
| else | ||
| continue | ||
| fi |
| ## Step 2: Update Go Struct | ||
|
|
||
| Add the new field to `SafeOutputMessagesConfig` in `pkg/workflow/compiler.go`: | ||
| Add the field to `SafeOutputMessagesConfig` in `pkg/workflow/compiler.go`: |
| - Shared JS modules live under `pkg/workflow/js/*.cjs` and `actions/setup/js/*.cjs` | ||
| - Action source files live under `actions/<action-name>/src/` | ||
| - Generated action bundles are committed under `actions/<action-name>/index.js` | ||
| - Shipping is driven by the action build pipeline (`make actions-build`, `gh aw actions-build`) and dependency maps such as `pkg/cli/actions_build_command.go` |
| ## Validation Tests | ||
|
|
||
| All error patterns must pass these tests: | ||
| All error patterns must pass the same safety checks used by the repo’s unit suite: |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — requesting minor changes on test accuracy and a maintenance risk in the guard script.
📋 Key Themes & Highlights
Key Themes
- Test fixture accuracy: The valid-paths test includes a
./README.mdline that the guard never validates (it falls through the allowlist), creating false confidence in the coverage. - Dual filter duplication: The bash loop and the Python subprocess both implement the same exclusion rules independently — a future rule change needs to be applied in two places.
- Directory vs file semantics: The guard uses
-e(exists) not-f(regular file); no test documents whether directory references are intentionally allowed.
Positive Highlights
- ✅ Excellent motivation: deleting 323 lines of dead
(go/redacted):embedguidance and replacing it with 76 lines of accurate action-centric guidance is a net win for contributors. - ✅ The guard is narrow by design — the allowlist-prefix approach avoids false positives from package names and example snippets.
- ✅ The test script uses isolated
mktemp -dfixture roots, which is the right pattern for bash integration tests. - ✅ Adding the guard to
lintandtest-scriptsmeans it runs in CI without requiring a separate opt-in step.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 70.8 AIC · ⌖ 15.3 AIC · ⊞ 7.6K
Comment /matt to run again
Comments that could not be inline-anchored
scripts/check-skill-file-paths_test.sh:18
[/diagnosing-bugs] The ./README.md fixture in the valid-paths test creates false confidence: the bash allowlist only validates paths starting with pkg/, scripts/, internal/, cmd/, eslint-factory/, .github/skills/, or the actions/ pattern. A ./-relative path silently falls through to the continue branch and is never checked, so the test passes for the wrong reason.
<details>
<summary>💡 Suggestion</summary>
Remove ./README.md from the valid fixture or add a comment cl…
scripts/check-skill-file-paths.sh:58
[/diagnosing-bugs] The guard's Python extraction and the bash filter apply the same exclusion rules twice but in slightly different ways — the Python script strips @ prefixes and also skips '"<>*? characters, whereas the bash loop above duplicates most of those same checks. This dual-filter approach creates a maintenance hazard: future rule changes must be applied to both layers to stay in sync.
<details>
<summary>💡 Suggestion</summary>
Consider making the Python script the single so…
scripts/check-skill-file-paths.sh:83
[/tdd] The test suite covers valid, invalid, and missing-directory scenarios but has no test for a path that names a directory rather than a file within an allowlisted prefix (e.g. pkg/workflow/js with no filename). The guard uses -e (exists), not -f (regular file), so a directory reference silently passes; it is worth verifying that is intentional and adding a test that documents the behaviour.
@copilot please address this.
Makefile:4
[/codebase-design] The check-skill-file-paths Makefile target is wired into lint but is not included in agent-report-progress-no-test, which is the fast pre-push target contributors use most often. A stale skill path will only be caught at full lint time, not on every intermediate save. If the script is fast enough (it is a grep + python pass over .github/skills/), consider adding it to the fast target.
@copilot please address this.
|
🎉 This pull request is included in a new release. Release: |
The repo still documented a dead JavaScript embedding/bundling architecture in a few high-traffic skills, and some references pointed at renamed validation files and stale paths. That drift could steer contributors toward edits in the wrong runtime layer, where they would never affect shipped assets.
What changed
actions/andpkg/workflow/js/, generated bundles rebuilt through the action build pipeline, and no guidance that suggests//go:embed-style runtime shipping.strict_mode_*.go,expression_safety_validation.go,expression_safety_test.go).SKILL.mdfiles for backticked repo-like paths and fails when they resolve to missing files, while ignoring package names and placeholder examples.Why it matters
Example