Repository navigation
Conversation
github-actions
Bot
added
documentation
PR Summary by QodoMove pstack to a root-level Hermes plugin and fix a TS scan trigger
AI Description
Diagram
High-Level Assessment
Files changed (150)
|
Code Review by Qodo
🔴 High 1. Release workflow fails before publishing
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| ``` | ||
|
|
||
| Strengthen when the loose type forces a lie at a use site. The tells are `!`, `arr[0] as T`, and a "should never happen" throw: | ||
| Strengthen when the loose type forces a lie at a use site. The tells are non-null assertions, `arr[0] as T`, and a "should never happen" throw: |
There was a problem hiding this comment.
🔴 High
5. Generated typescript guidance can drift from its converter 📎 Requirement gap ⚙ Maintainability
patterns.md changes the warning by hand, but tools/convert.py has no anchored transform or audited provenance entry for the phrase. When CI, publish-dist, or release rebuilds from the pinned source, the root-mode copy replaces skills/ with upstream content, restoring the ! phrase that triggers Hermes’ inline-shell DSL and causing the tracked-file check to fail.
Agent Prompt
## Issue description
The generated TypeScript reference was hand-edited to remove a phrase that triggers Hermes’ inline-shell DSL. Rebuilds from the pinned source restore that phrase because the converter does not implement or audit the adaptation.
## Fix Focus Areas
- skills/typescript-best-practices/references/patterns.md[94-94]
- tools/convert.py[1750-1767]
- .build-provenance.txt[8-33]
## Recommended Fix
Add an exact, anchor-audited transform in `convert.py` that replaces the upstream text ``The tells are `!`, `arr[0] as T`,`` with ``The tells are non-null assertions, `arr[0] as T`,``. Record a `fixes:` entry in `.build-provenance.txt`, rebuild from pin c47b128, and commit the generated output. Keep the remainder of the warning, the cast example, and the TypeScript samples byte-for-byte unchanged. Add a validator check that fails when a `!` code span is followed by another backtick span in skill content.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| ``` | ||
|
|
||
| Strengthen when the loose type forces a lie at a use site. The tells are `!`, `arr[0] as T`, and a "should never happen" throw: | ||
| Strengthen when the loose type forces a lie at a use site. The tells are non-null assertions, `arr[0] as T`, and a "should never happen" throw: |
There was a problem hiding this comment.
🔴 High
1. The inline-shell fix has no parser-safety proof 📎 Requirement gap ⛨ Security
tools/validate.py only parses the YAML manifest and checks package layout, with no cases for accidental prose, intentional inline-shell syntax, TypeScript postfix assertions, or isolated operator mentions. A validation run therefore cannot demonstrate that the original phrase reaches the real parser without execution while the adapted phrase does not.
Agent Prompt
## Issue description
The PR adapts a phrase that reportedly triggers Hermes inline-shell parsing, but the validator and test changes provide no parser proof or execution guard for the affected content.
## Fix Focus Areas
- skills/typescript-best-practices/references/patterns.md[94-94]
- tools/validate.py[159-177]
- tools/tests/test_bans.py[16-24]
## Recommended Fix
Add validator tests covering accidental prose, intentional inline-shell DSL, ordinary TypeScript postfix assertions, and an isolated operator mention. Invoke the real upstream parser on the original and adapted phrases, assert only the original matches, and ensure the proof uses a harmless stub or equivalent guard so no command can execute.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+159
to
+161
| p = pkg / "plugin.yaml" | ||
| if not p.is_file(): | ||
| p = pkg / "plugin.yaml" |
There was a problem hiding this comment.
🔴 High
2. Rebuilds leave published metadata stale 🐞 Bug ≡ Correctness
check_manifest() now reads plugin.yaml, but convert.py still generates plugin.json and does not regenerate the checked-in YAML file. On a root rebuild, validation and publication consume the old YAML metadata even if the pinned upstream manifest changes; a build into a fresh directory instead fails validation because YAML is absent.
Agent Prompt
## Issue description
The converter writes `plugin.json`, while the validator and publisher consume an unrelated, checked-in `plugin.yaml`.
## Fix Focus Areas
- tools/validate.py[158-173]
- tools/convert.py[1805-1809]
- .github/workflows/publish-dist.yml[49-52]
## Recommended Fix
Use the same Hermes-supported manifest file for conversion, validation, and publication. Verify both a fresh-directory build and a root rebuild after changing upstream manifest metadata.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| pstack-core: | ||
| - changed-files: | ||
| - any-glob-to-any-file: "pstack/plugin.json" | ||
| - any-glob-to-any-file: "pstack/plugin.yml" |
There was a problem hiding this comment.
🟠 Medium
8. Package changes lose their review labels 🐞 Bug ⚙ Maintainability
The labeler's manifest glob now names pstack/plugin.yml, while the migrated manifest is plugin.yaml at the repository root and the other package globs still start with pstack/. When a later pull request changes root skills, config, or the manifest, the corresponding package, skills, and principles labels do not match.
Agent Prompt
## Issue description
The automatic package labels still point to deleted subdirectory paths and a nonexistent manifest name.
## Fix Focus Areas
- .github/labeler.yml[8-19]
## Recommended Fix
Match the root manifest and root `config/**` and `skills/**` paths, including the principles subset. Verify label matches against representative changed-file paths.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
61
to
63
| cp -a plugin.yaml skills config assets README.md after-install.md LICENSE .build-provenance.txt "$RUNNER_TEMP/release-built" | ||
| rm -f "$RUNNER_TEMP/release-built/.build-provenance.txt" | ||
| diff -qr "$RUNNER_TEMP/release-built" "$published" |
There was a problem hiding this comment.
🔴 High
3. Release workflow fails before publishing 🐞 Bug ☼ Reliability
The release verification step runs cp -a with multiple sources but never creates its destination, $RUNNER_TEMP/release-built; unlike the previous single-source copy, this requires an existing directory. On a fresh runner, cp fails under set -euo pipefail, so tagged releases stop before the comparison, ZIP creation, or GitHub release.
Agent Prompt
## Issue description
Release verification copies multiple package entries into `$RUNNER_TEMP/release-built` without creating that directory first, causing the release job to stop before comparison and packaging.
## Fix Focus Areas
- .github/workflows/release.yml[60-63]
## Recommended Fix
Add `mkdir -p "$RUNNER_TEMP/release-built"` before the copy, and include the real manifest (`plugin.json`) in the source list. Run the verification script with a fresh temporary directory to confirm the copy and subsequent steps succeed.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| env: | ||
| UPSTREAM_CLONE: ${{ steps.build.outputs.upstream-clone-path }} | ||
| PACKAGE_PATH: pstack | ||
| PACKAGE_PATH: . |
There was a problem hiding this comment.
🔴 High
6. Ci reproducibility gate can never pass 🐞 Bug ☼ Reliability
With PACKAGE_PATH: ., hash_git_tree() runs git ls-tree -r HEAD -- . and hashes the entire committed repository, while hash_dir_tree() hashes only the converter output, which contains plugin.json rather than plugin.yaml. On a clean PR or push run, tracked tooling, workflows, and documentation have no counterpart in the rebuild, so the comparison fails on both OS legs regardless of package contents.
Agent Prompt
## Issue description
The reproducibility gate compares a whole-repository committed-tree hash with a package-only rebuild hash, causing clean runs to fail.
## Fix Focus Areas
- .github/workflows/ci.yml[64-105]
## Recommended Fix
Enumerate the same explicit package paths on both sides (`plugin.json`, `skills`, `config`, `assets`, `README.md`, `after-install.md`, and `LICENSE`), excluding `.build-provenance.txt`. Pass the committed package paths to `git ls-tree -r HEAD -- <paths>`, account for the `plugin.yaml`/`plugin.json` name difference, and normalize names identically before hashing the corresponding rebuilt paths. Exercise the gate on an otherwise clean checkout.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| # cron/loop jobs in Phase 4. | ||
| agents_n = copy_tree(source / "agents", out / "agents", st) | ||
| comment_sicko = out / "agents" / "comment-sicko.md" | ||
| comment_sicko = source / "agents" / "comment-sicko.md" |
There was a problem hiding this comment.
🟠 Medium
9. Comment sicko reference can ship with crlf 🐞 Bug ≡ Correctness
comment_sicko is now read from the upstream source and written with write_bytes(comment_sicko.read_bytes()), skipping copy_file/normalize_text. Before, it came from the normalized out/agents copy. On a checkout with core.autocrlf=true (the Windows CI leg; the provenance shows this build fixed 124 CRLF files), skills/no-comments/references/comment-sicko.md keeps CRLF/BOM, which fails check_encoding and diverges from the Linux build.
Agent Prompt
## Issue description
The Comment Sicko persona is copied byte-for-byte from the source, with no LF/BOM normalization.
## Fix Focus Areas
- tools/convert.py[1764-1771]
## Recommended Fix
Replace the `write_bytes(comment_sicko.read_bytes())` call with `copy_file(comment_sicko, no_comments_refs / "comment-sicko.md", st)`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| import argparse | ||
| import hashlib | ||
| import json | ||
| import yaml |
There was a problem hiding this comment.
🟠 Medium
10. Lint fails on an unused import in the converter 🐞 Bug ⚙ Maintainability
import yaml was added to convert.py among the stdlib imports but is never used, since the converter still emits JSON. Ruff's select = ["E4","E7","E9","F","I","UP","B"] flags it as F401 (unused) and I001 (third-party import inside the stdlib block), so the ruff check tools CI step fails.
Agent Prompt
## Issue description
An unused third-party import in the stdlib block breaks ruff (F401, I001).
## Fix Focus Areas
- tools/convert.py[33-42]
## Recommended Fix
Delete `import yaml` from `tools/convert.py`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| VOCAB_EXEMPT_PREFIXES: tuple[str, ...] = ("agents/", ".cursor-plugin/", ".build-provenance.txt") | ||
| GENERATED_DEPENDENCY_DIRS: frozenset[str] = frozenset({"node_modules"}) | ||
| VOCAB_EXEMPT_PREFIXES: tuple[str, ...] = (".build-provenance.txt",) | ||
| GENERATED_DEPENDENCY_DIRS: frozenset[str] = frozenset({"node_modules", "tools", "docs", ".github"}) |
There was a problem hiding this comment.
🟠 Medium
11. Security ban scan skips any tools or docs folder 🐞 Bug ⛨ Security
is_generated_dependency_path() treats tools, docs, and .github as generated dependency directories wherever they appear in a path, rather than exempting only repository-level directories. When recursively copied upstream skills contain paths such as skills/<name>/tools/install.sh or skills/<name>/docs/reference.md, those packaged files bypass security-ban checks in validation and the scanner gate, as well as encoding and conflict-marker checks.
Agent Prompt
## Issue description
Directory-name exemptions for `tools`, `docs`, and `.github` also suppress checks of files inside packaged skills.
## Fix Focus Areas
- tools/bans.py[32-57]
## Recommended Fix
Keep `GENERATED_DEPENDENCY_DIRS = {"node_modules"}` and add a separate `EXEMPT_TOP_LEVEL_DIRS = {"tools", "docs", ".github", ".git", ".venv"}` that applies only when `rel.parts[0]` matches. Add a regression case proving nested skill references remain in the scanned file set. Consider running the scan on the package file list rather than the repository root.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
-24
to
-27
|
|
||
| Port modifications for the Hermes Agent platform (converter, validator, CI, | ||
| fork patches, discovery and delegation rewrites): | ||
| Copyright (c) 2026 the pstack-hermes-port contributors |
There was a problem hiding this comment.
🟠 Medium
12. Port contributors' copyright removed from license 🐞 Bug ⚙ Maintainability
Because DEFAULT_OUT is now the repo root, the converter's generated LICENSE (upstream MIT plus ADAPTATION_LINE) overwrote the repository LICENSE. The paragraph granting "Copyright (c) 2026 the pstack-hermes-port contributors" for the converter, validator, CI and fork patches is gone. The repo's own hand-written tooling now carries only the upstream author's notice, and any future manual restore would be wiped on the next rebuild.
Agent Prompt
## Issue description
The in-place root build overwrites the repository LICENSE and drops the port contributors' copyright.
## Fix Focus Areas
- tools/convert.py[1812-1822]
- LICENSE[21-27]
## Recommended Fix
Have the converter append the port-modifications copyright paragraph after `ADAPTATION_LINE` so the generated LICENSE keeps it, then rebuild.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| |---|---|---| | ||
| | `pstack/` | **generated** (`linguist-generated`) | never hand-edit; review as build output | | ||
| | `pstack/.cursor-plugin/`, `pstack/agents/` | generated; inert on hermes | dual-load surface; leave as-is | | ||
| | `agents/` | generated; inert on hermes | agent personas; leave as-is | |
There was a problem hiding this comment.
🟠 Medium
13. Agent rules contradict the new root layout 🐞 Bug ⚙ Maintainability
AGENTS.md still tells agents that pstack/ is the generated never-hand-edit tree and that agents/ is generated and should be left as-is. Now the generated tree is skills/, config/, etc. at the root, and validate.py fails if agents/ exists. The PR also deletes the hard rules "Anchors audited", "Converter changes are tracked in issues" and "Excluded upstreams stay excluded" and leaves a dangling "this migration merges." fragment, so agents lose the guardrail against hand-editing generated skills (the exact mistake in this PR).
Agent Prompt
## Issue description
Agent-governing docs reference the old `pstack/` and `agents/` layout and drop the rules that prevent hand-editing generated content.
## Fix Focus Areas
- AGENTS.md[130-140]
- AGENTS.md[183-192]
## Recommended Fix
Mark the root `skills/`, `config/`, `assets/`, `plugin.json`, `README.md`, `after-install.md`, `LICENSE` and `.build-provenance.txt` as generated (never hand-edit). Remove the `agents/` row. Restore the Anchors-audited, converter-change-tracking and excluded-upstreams rules, and fix the dangling sentence.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1
to
+2
| $schema: https://agent-plugins.org/schemas/1.0.0/plugin.schema.json | ||
| name: pstack |
There was a problem hiding this comment.
🔴 High
4. Hermes cannot install the plugin 🔗 Cross-repo conflict ≡ Correctness
The root plugin.yaml sends this skills-only package through Hermes’ native plugin loader, which requires a loadable entry point, while the portable skills loader expects a root plugin.json. Installs from the checked-in root, published dist tree, or release ZIP cannot register the skills; even if conversion adds plugin.json, the root YAML takes precedence and reaches the same native-loader failure.
Agent Prompt
## Issue description
The skills-only package is published with a root `plugin.yaml`, which selects Hermes’ native plugin loader rather than the portable loader that discovers skills through `plugin.json`. The converter still generates JSON, but publication omits it, and keeping YAML alongside it would still select the wrong loader.
## Fix Focus Areas
- plugin.yaml[1-18]
- tools/convert.py[1805-1809]
- tools/validate.py[158-176]
- tools/validate.py[301-303]
- .github/workflows/publish-dist.yml[49-53]
- .github/workflows/release.yml[61-69]
## Recommended Fix
Remove the root `plugin.yaml` and commit the converter-generated root `plugin.json`. Restore JSON parsing in `check_manifest` and `check_layout`, remove the unused `yaml` import, and make dist publishing and release copy and ZIP lists include `plugin.json` instead of `plugin.yaml`. Test a normal Hermes install without force and confirm skill discovery from the exact published tree.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 8c19fa4 |
iap
force-pushed
the
fix/ts-pattern-inline-shell-v2
branch
from
October 31, 2026 15:09
8c19fa4 to
336cc57
Compare
|
Code review by qodo was updated up to the latest commit 336cc57 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes issue #107 - replace problematic ! pattern with non-null assertions.