Skip to content

feat: package SkillSpector as a Codex skill - #371

Open
edjshen wants to merge 4 commits into
NVIDIA:mainfrom
edjshen:codex/skill-scanner-closeout-20260812
Open

edjshen wants to merge 4 commits into
NVIDIA:mainfrom
edjshen:codex/skill-scanner-closeout-20260812

Conversation

@edjshen

@edjshen edjshen commented Aug 13, 2026

Copy link
Copy Markdown

What changed

  • packages SkillSpector as a reusable Codex skill-scanner skill
  • adds the skill contract in SKILL.md, OpenAI UI metadata, and a portable launcher
  • runs the repository's locked production environment through uv --frozen --no-dev
  • defaults guidance to static --no-llm inspection and preserves scanner exit codes

Why

This makes the existing SkillSpector engine directly usable as an agent skill while keeping target skills untrusted and avoiding execution of their scripts or dependencies.

Validation

  • skill-creator quick_validate.py: valid
  • launcher --help: successful against SkillSpector 2.9.4
  • safe fixture: exit 0, score 0, SAFE, 0 findings
  • malicious fixture: exit 1, score 93, CRITICAL / DO_NOT_INSTALL, 6 findings
  • branch reconciled cleanly with upstream main at SkillSpector 2.9.4

Static --no-llm mode intentionally reports disabled semantic analyzers as a limitation; the launcher does not hide that completeness signal.

Comment thread agents/openai.yaml
@@ -0,0 +1,4 @@
interface:
display_name: "Skill Scanner"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to have it as SkillSpector itself.

Comment thread SKILL.md
@@ -0,0 +1,90 @@
---
name: skill-scanner

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have it SkillSpector itself IMHO. The description anyways highlights it as a skill scanner

@rng1995

rng1995 commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

@edjshen - Please address the review comments.

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes for the recursive workflow bug below. The existing metadata naming threads also remain unresolved (display name, skill name), and the current head still fails the DCO check.

Comment thread SKILL.md
5. Report the scan mode, score, severity, recommendation, highest-impact findings with
file/line evidence, execution completeness, and material limitations.

Use `--recursive` when the target contains multiple immediate subdirectories that each

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Make the recursive workflow produce the JSON that step 4 requires. Adding --recursive to the canonical command without --output does not emit combined JSON: _scan_multi_skill prints human summaries unless an output path is supplied. An agent following these instructions therefore has no execution_successful/completeness fields to inspect. Require --output <path> for recursive scans and instruct the agent to read that file, or change the launcher to honor --format json on stdout.

@mohgupta-ship-it

mohgupta-ship-it commented Aug 25, 2026 •

Copy link
Copy Markdown
Member

Powered by Codex: PR council review result.

This is a triage signal, not a maintainer approval.

  • Rating: new feature
  • Confidence: high
  • Status read: Changes requested, DCO fail, behind
  • Review method: fresh GitHub metadata/body/files/reviews/checks plus selected diffs; council lenses were spec fit, dead-code/reachability, YAGNI/scope, design/coupling, and code standards/tests.
  • Council assessment: Codex skill packaging is useful, but recursive workflow/naming issues and DCO failure remain unresolved.
  • Recommended action: Fix skill workflow/naming, sign/rebase, and request re-review.

@rng1995

rng1995 commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

@edjshen - Please address my review comments, resolve merge conflicts and ping me for approval

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SkillSpector Review]

Re-review: still requesting changes. The current head only merges upstream changes and leaves every requested PR-owned edit unchanged: the display/skill names remain Skill Scanner/skill-scanner, recursive JSON guidance still makes --output optional even though no JSON contract is emitted without it, and DCO still fails for 7d7b985 and ae7d8a2. In an exact-head reproduction, recursive JSON with no output file printed only the summary table and contained neither execution_successful nor analysis_completeness. Please address the three open threads and restore green DCO.

@yashrajp22 yashrajp22 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes for the static-mode privacy guarantee: enabled tracing can transmit scanned file contents even with --no-llm. The new finding is inline; existing review threads are not duplicated.

Verification covered 147 selected tests, 48 source/wheel corpus reports and four actual launcher cases, including concurrent outputs and Unicode paths. Testing was offline on Python 3.12; partial corpus coverage and finding-ID differences remain recorded. Live providers and cold network installation were not tested.

Comment thread SKILL.md

## Choose static or semantic analysis

Default to `--no-llm`. Static mode keeps file contents local, is fast, and does not require

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we disable inherited tracing for static scans, or make the tracing disclosure and consent requirement explicit here? When LangSmith tracing is enabled, --no-llm still allows scanned file contents in graph state to be sent to the configured tracing endpoint. I reproduced this on the installed PR package using a synthetic loopback receiver and flushing queued traces. The local-only guarantee can therefore lead an agent to transmit contents without the approval required below.

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SkillSpector Review]

@edjshen thank you for packaging SkillSpector as an agent skill with a locked, production-only launcher, and sorry this has sat for so long.

Value and readiness

Still not ready. Main has resolved one earlier blocker, but the naming, privacy-claim and DCO items are unchanged. There has been no author activity on the PR since it was opened on Aug 13.

Material findings

Prior findings

  1. Display name Skill Scanner (agents/openai.yaml:2): still open (maintainer asked for SkillSpector).
  2. Skill name skill-scanner (SKILL.md:2, and $skill-scanner in default_prompt): still open.
  3. Recursive JSON requires --output (SKILL.md:30): resolved by main. #576 (fcfbcd8b, Sep 21) makes _scan_multi_skill write the combined JSON to stdout when --output is absent, with progress on stderr. Step 4's fields will therefore be present after merge, and this thread can be resolved.
  4. DCO: still open. 7d7b985 and ae7d8a2 have no Signed-off-by. The two update-branch merges are exempt under ci.yml's rule.
  5. yashrajp22's tracing finding (SKILL.md:37, "Static mode keeps file contents local"): still open. On main, _build_trace_config still applies to --no-llm scans, and nothing disables inherited LangSmith tracing.

New (optional)

  • SKILL.md:46 cites a README section Security Considerations that does not exist on main. The closest section is "Trust model and data egress".
  • scripts/scan_skill.py lacks the SPDX header that the other scripts/ files carry.

PIC tradeoffs

  • Main already ships skills/skill-inspector/SKILL.md (#253, Aug 12), an agent skill that runs skillspector scan --no-llm --format json and then reviews the source. This PR adds a second, root-level skill that turns the whole repository into a skill bundle. The PIC should decide whether both are wanted. If the author does not return, closing this in favor of skill-inspector is reasonable.
  • The README makes the same "--no-llm keeps contents local" claim. Disabling tracing for --no-llm in the product may therefore be a better fix than a caveat in SKILL.md.

Verification and gaps

  • Read the 142-line diff at da2b83d7 and all rng1995 and yashrajp22 reviews and threads. Commits after ae7d8a2 are update merges only.
  • git merge-tree against 530d3c5c is clean.
  • Compared _scan_multi_skill at the PR head with main, checked _build_trace_config and the README headings on main, and checked sign-offs against the DCO rule in ci.yml.
  • Last CI ran Aug 26 (DCO failed). Nothing was executed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants