Repository navigation
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Amejuma <johnsonamaju@gmail.com>
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Reviewed current head 1fa896d98d7e525f3c3ccdd8c5b3e063050003ee. The new CLAUDE.md guidance matches the repository's current commands, provider architecture, analyzer layout, and contribution conventions. I found no required documentation changes.
All hosted required checks pass. The branch is behind main, so it must be updated and revalidated before merging.
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Corrective re-review of current head 9ecbf2615918fcdaa21ecfc077b8960782312a3b. The new contributor guide still has required factual corrections: it omits the implemented TT6 rule and points to a nonexistent LLM analyzer module while mislocating LLMMetaAnalyzer. Please correct the two architecture statements identified inline.
| adding a node needs **no** `graph.py` change. Categories: `static_patterns_*` (regex, one `analyze(content, | ||
| file_path, file_type) -> list[AnalyzerFinding]` per module, built on `static_runner.run_static_patterns` + | ||
| `pattern_defaults` for category/remediation), `static_yara.py` (YARA), `behavioral_ast.py` (AST1-9: exec/eval/ | ||
| subprocess/os.system/compile/dynamic-import/getattr), `behavioral_taint_tracking.py` (TT1-5: source→sink |
There was a problem hiding this comment.
This analyzer already includes TT6 (it is registered in both the severity and confidence maps), so TT1-5 understates the implemented rule set. Please update this to TT1-6 and describe TT6 if this list is intended to summarize coverage.
| `providers/<name>/` is one subpackage per provider (own `provider.py` + bundled `model_registry.yaml`); | ||
| `providers/registry.py` exposes context-length/max-output lookups. CLI providers (`claude_cli`, `codex_cli`) | ||
| implement `AgentCLICapable` and shell out through the hardened `providers/_agent_cli.py` (no shell, stdin-only | ||
| untrusted content, env scrubbed of API keys, tools/MCP disabled, per-call timeout). `nodes/llm_analyzer_base.py` |
There was a problem hiding this comment.
This path does not exist at this head, and the two classes are not co-located: LLMAnalyzerBase is in src/skillspector/llm_analyzer_base.py, while LLMMetaAnalyzer is in src/skillspector/nodes/meta_analyzer.py. Please correct the path and class ownership so the contributor guide does not send readers to a missing module.
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Re-reviewed current head b2cafc124ab0ce218db990a592f3c4f77f5cb326 after the latest main synchronization. The PR-owned CLAUDE.md blob is byte-identical to the prior head, so both factual blockers remain: the guide still says TT1-5 although TT6 is implemented, and it still points to a nonexistent nodes/llm_analyzer_base.py while mislocating LLMMetaAnalyzer. The prior inline comments contain the exact corrections, so I have not duplicated them.
test-unit is still running and GitHub reports mergeStateStatus=BLOCKED.
| 3. Register the node id/callable in `nodes/analyzers/__init__.py`'s `ANALYZER_NODE_IDS` / `ANALYZER_NODES` — | ||
| `graph.py` wires the edges automatically. |
There was a problem hiding this comment.
Please describe defining ANALYZER_ID and node in the analyzer module, then updating the registry test. These collections are populated automatically. Pre-populating ANALYZER_NODE_IDS makes discovery return early, which can leave existing analyzers unwired.
| 1. Implement a node: input `state: SkillspectorState`, output `AnalyzerNodeResponse` (`{"findings": | ||
| list[Finding]}`). | ||
| 2. For a regex-pattern analyzer, write `analyze(content, file_path, file_type) -> list[AnalyzerFinding]` and run | ||
| it through `static_runner.run_static_patterns`, sourcing category/remediation text from `pattern_defaults`. |
There was a problem hiding this comment.
Please use run_static_patterns_with_ledger and preserve its ledger and status output in this recipe. During a normal scan with producer records, a registered analyzer returning only findings leaves those findings unaccounted for, so finalization marks the scan as failed. The node wrapper does not add these records. The architecture flow should also include finalize_inspection_ledger.
| `providers/registry.py` exposes context-length/max-output lookups. CLI providers (`claude_cli`, `codex_cli`) | ||
| implement `AgentCLICapable` and shell out through the hardened `providers/_agent_cli.py` (no shell, stdin-only | ||
| untrusted content, env scrubbed of API keys, tools/MCP disabled, per-call timeout). `nodes/llm_analyzer_base.py` |
There was a problem hiding this comment.
Please qualify the restrictions by provider. Claude receives an empty tool allowlist and strict MCP configuration; Codex uses a read-only sandbox and still permits model-generated read commands. Describing both as having tools disabled gives maintainers the wrong security contract.
| - **State**: `SkillspectorState` (`state.py`, `TypedDict, total=False`) is threaded through every node. Findings | ||
| accumulate via an `operator.add` reducer on `state["findings"]`. See the field table in | ||
| `docs/DEVELOPMENT.md` §4 before touching state shape. |
There was a problem hiding this comment.
Please describe the findings reducer as merge_findings_by_id. Updates with an existing finding_id replace that finding in place; only new IDs append. This matters when changing state or meta-analyzer enrichment, because operator.add would have different behavior.
yashrajp22
left a comment
There was a problem hiding this comment.
Please address the four issues noted in my inline comments before merging.
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Re-reviewed current head b4d5a5ec5baded33c35b6f2394370d985c994571 after the latest main synchronization. The PR-owned CLAUDE.md blob is byte-for-byte identical to reviewed head b2cafc124ab0ce218db990a592f3c4f77f5cb326, so the outstanding documentation corrections remain:
- update the behavioral-taint coverage from TT1-5 to TT1-6;
- point
LLMAnalyzerBasetosrc/skillspector/llm_analyzer_base.pyandLLMMetaAnalyzertosrc/skillspector/nodes/meta_analyzer.py; - describe analyzer discovery through
ANALYZER_ID/nodeinstead of pre-populating the registry collections; - use
run_static_patterns_with_ledgerand include final ledger handling in the architecture recipe; - distinguish Claude's empty tool/MCP allowlist from Codex's read-only command sandbox; and
- describe the findings reducer as
merge_findings_by_id, notoperator.add.
The six existing inline threads contain the exact locations, so I have not duplicated them. All current hosted checks pass, but these required documentation changes, the unresolved threads, and GitHub's BEHIND merge state block approval and merge.
Summary
CLAUDE.md, guidance for Claude Code (and future instances) working in this repo: common commands (install, test targets, lint/format, running a single test), and a summary of the LangGraph architecture (state, findings, analyzer registration, provider/LLM plumbing, suppression, entry points).README.mdanddocs/DEVELOPMENT.md; no functional code changes.🤖 Generated with Claude Code