fix(plugin): review fixes for plugin evaluation (stacked on #28)

[rng1995]

Summary

Bug fixes from a code-quality review of #28. This PR is stacked on #28: its base is naren/plugin-evaluation-all-tiers, and merging it lands the fixes on #28's branch.

Every fix is its own signed-off commit with a regression test that fails without the fix. Please merge with a merge commit or rebase (not squash) to keep the per-fix history.

Area	Fix
Tier 2 plugin dedup	A plugin with one bundled skill no longer crashes context dedup: its per-skill LLM budget was 100, above the validator's cap of 50.
	Unsafe content in a bundled skill (for example a hard link) now blocks standalone tier2 PLUGIN instead of passing as a MEDIUM advisory.
	Advisory capping keeps each capped finding once and keeps plain warnings.
MCP and hook static checks	A shell-command MCP server with non-list args no longer crashes validate.
	The shell inline-program check now also catches bash.exe -c, -lc/-ec/-xc, and "command": "bash -c …".
	Exactly pinned scoped packages such as @nextcloud/…@1.2.3 or @headlessui/… are no longer HIGH mcp_command_floating_version.
	./a/../b is no longer reported as leaving the hook's working directory.
Tier 3 plugin staging	environment/plugin_mcp_servers.toml is now reserved. Before, an authored copy in the evals source replaced the plugin's validated MCP servers and skipped the static checks and redaction.
	.ENV / .Env.local (any letter case) are no longer copied into native plugin containers. Tier 1 already flagged them.
	A missing Claude Code harness log returns None instead of raising.
Dependency and schema validators	npm and container findings in bundled skills are no longer double-prefixed (skills/foo/skills/foo/package.json).
	Python MCP-runner findings name the manifest folder (.claude-plugin/plugin.json).
	An undecodable or oversize selected manifest is HIGH manifest_unreadable, not manifest_unsafe with a security failure. It is now read leniently, so its components are still checked.
	declared_dependencies counts only component fields on both manifest paths.
Harbor results, reporting, CLI	An integer usage counter or reward too large for a float no longer aborts collect_harbor_results.
	The HTML Tier 3 dashboard no longer shows INCOMPLETE in green.
	Markdown no longer raises on an empty plugin block.
	validate . / validate SKILL.md reports and SARIF use the resolved content root, so they no longer show Target: . or a doubled SKILL.md/SKILL.md URI.
	Status-like numbers inside successful MCP answers (for example "#451 - Crash") no longer count as failed calls.

docs/plugin-evaluation.mdx is updated where it described the changed behavior (the shell -c and floating-version rows, the unreadable manifest paragraph, and the reserved MCP servers file).

For reviewers

Behavior changes worth a look:

• Shell -c check: "args": {"-c": "x"} (a dict) used to get CRITICAL mcp_command_dangerous_form, only because iterating the dict yields its keys. It now gets HIGH mcp_args_not_list, which still blocks.
• Unreadable manifest: manifest_unreadable is no longer a security failure, so a policy severity override can lower it.
• Verdict card colour: the dashboard Verdict card now follows the Tier 3 tier-card tone for every state, so a neutral verdict is warning-coloured.
• Floating-version marker: the marker must now be attached to a package or image name. pkg@main2 is no longer HIGH but still gets MEDIUM mcp_unpinned_package.

Known gaps, not addressed here:

• An authored evals/environment/mcp_servers.toml (the shared task environment) is still loaded into every arm without the MCP static checks. That applies when the evals source is the plugin's own evals/.
• The validate exit gate excludes advisory_tier2 results even when they are security failures. Tier 1's whole-tree check already rejects linked and hard-linked plugin files.
• The dependency audit and Tier 3 coverage still read an unreadable selected manifest strictly.

Testing

• Full suite on this branch: 13,061 passed, 26 skipped, 0 failed (uv run pytest -q -n auto). That includes the golden CLI surface; no snapshot was regenerated.
• uv run ruff check . passes, and git diff --check is clean.
• Each new regression test was confirmed to fail with its source change reverted.

🤖 Generated with Claude Code

[rng1995]

Review of the batch-1 fixes. All 9 inline comments are below; the first four are correctness issues to address before merging.

1. Blocking false positive: the shell -c check scans the script's own arguments (-config, -recursive → CRITICAL).
2. Policy can hide unchecked manifests: an unreadable selected manifest whose contents couldn't be parsed can now be downgraded by policy.
3. Missed MCP failures: errors like status 404: Not Found in the middle of a line are counted as successful calls.
4. Collection can abort: a harness-log lstat error can stop Harbor result collection.

The rest are a removed fail-closed backstop, SARIF path consistency, a KeyError on an unknown ecosystem, and two cleanups. Every correctness item except the backstop was reproduced against 7608e96f8.

[rng1995]

[correctness] The shell -c check now flags the script's own arguments.

shell_args includes every argument after the shell, so it covers the script path and the script's own options. _SHELL_INLINE_PROGRAM_FLAG_RE also matches any single-dash word containing c. Reproduced:

• {"command": "bash", "args": ["server.sh", "-config", "x.yml"]} → CRITICAL mcp_command_dangerous_form
• ["run.sh", "--watch", "-recursive"] → CRITICAL mcp_command_dangerous_form

That blocks safe plugins. bash and sh only read options until the first operand (the script) or --, so stop scanning there:

def _shell_runs_inline_program(args: list[str]) -> bool:
    for arg in args:
        token = arg.strip()
        if token == "--" or not token.startswith("-"):
            return False  # the script path (or end of options) ends the shell's options
        if not token.startswith("--") and _SHELL_INLINE_PROGRAM_FLAG_RE.fullmatch(token):
            return True
    return False

Option values such as -o pipefail or --rcfile x need care; a test with script arguments after the script path would have caught this.

[rng1995]

Fixed in 23b3bcd. A new _shell_runs_inline_program() reads shell options only up to the first operand (the script) or --/-. It skips the value of -o/-O/+o/+O (also inside a cluster such as -eo pipefail), --rcfile and --init-file. bash server.sh -config x.yml and bash run.sh --watch -recursive are no longer flagged. -c, -lc, -l -c, -o pipefail -c, --rcfile x -c, bash.exe -c and "bash -c node" still are. It also now catches +c, which bash, sh and zsh treat as -c. Tests are in tests/validators/test_mcp_static.py, and the docs row is updated.

[rng1995]

[correctness / security] A policy override can now hide a manifest that was never checked.

manifest_unreadable no longer sets security_failure, so apply_policy will apply severity overrides to it (PLUGIN_SCHEMA.manifest_unreadable or PLUGIN_SCHEMA.*). That is fine when the lenient parse succeeded and the components were checked.

It is not fine when nothing was parsed:

• _parse_unreadable returns None for a manifest over the 8 MiB lenient bound, or one that isn't JSON even when read leniently;
• the agent_plugin.yaml branch at line 522 never reads leniently.

In both cases none of the plugin's hooks, MCP servers or LSP servers were inventoried, so a downgrade lets Tier 1 pass a plugin with no component checks. Before this change, security_failure kept these blocking regardless of policy.

Suggested fix: set result.metadata["security_failure"] = True (or another marker that policy can't override) whenever the selected manifest yields no data. Keep it overridable only when the lenient parse produced data that was inventoried.

[rng1995]

Fixed in 4941acf. When nothing can be read from the selected manifest, it now sets security_failure, so a policy override cannot downgrade it and Tier 1 stops after the schema step, as it does for an unsafe manifest. This covers the agent_plugin.yaml content-error branch and the JSON case where even the lenient read returns nothing. A manifest that does parse leniently can still be downgraded. Tests in tests/validators/test_plugin_review_fixes_b.py cover the policy cases (3 parametrized cases), Tier 1 stopping, and the lenient-read path.

[rng1995]

[correctness] Anchoring the status code to line start drops real failures.

The old pattern caught these mid-line errors; the new one counts them as successful calls (reproduced):

• Request failed with status 404: Not Found
• Error calling tool: 404: not found
• GET /repos/x: 404 - Not Found

That overstates mcp_calls.success_rate and can wrongly give mcp_proof a used-successfully status. Keep the line-start form for bare codes, and also accept a code after an error word earlier on the same line, e.g.:

r"(?:^[ \t]*(?:(?:error|http/\d(?:\.\d)?|status(?:[ _]code)?)[ \t:=]*)?[45]\d{2}\b[ \t]*[:\-])"
r"|(?:\b(?:error|failed|status(?:[ _]code)?)\b[^\n]{0,80}?\b[45]\d{2}\b[ \t]*[:\-])"

Add these three strings to test_failure_markers_mark_failure, keeping the three new negative cases.

[rng1995]

Fixed in 5610ad9. The line-start form stays. Two new patterns now count as failures:

• a request line at the start of a line, e.g. GET /repos/x: 404 - Not Found;
• a 4xx/5xx code after an error word (error, failed, status) on the same line, e.g. Request failed with status 404: Not Found. The code has to follow a space, so src/app.py:404: still doesn't match.

Every gap in the pattern is bounded, so it stays linear: about 0.06 s on 210k characters of adversarial input. The three strings are added to test_failure_markers_mark_failure, and the existing negatives still pass.

[rng1995]

[correctness] A harness-log lstat error can abort all of Harbor collection.

Only FileNotFoundError maps to None. NotADirectoryError (an agent path component that is a file) and PermissionError still raise SecurePathError. The only caller, collector._trial_load_census, which is reached from _attach_load_census, doesn't catch it. So one trial's unreadable log stops collect_harbor_results for the whole run, even though the load census is advisory.

Suggest treating NotADirectoryError like a missing file here. Then either return None for any other OSError, or catch SecurePathError in _trial_load_census and keep the census without harness evidence.

[rng1995]

Fixed in f1d5f28. read_harness_log_prefix now returns None for NotADirectoryError as well as for a missing file. _trial_load_census catches SecurePathError/OSError and keeps the census without harness evidence. Links are still refused. Tests are in tests/tier3/test_plugin_native.py.

[rng1995]

[correctness] The fail-closed backstop for unsafe input is gone.

Unsafe input reported through SkillCollectionError is now covered by the security_failure metadata, but the except (SecurePathError, SkillCollectionError) branch was removed with it. Any SecurePathError that escapes validate() now falls into the generic except Exception. That branch builds a MEDIUM context_dedup_error without security_failure, and _make_advisory passes it. Examples are a code path that doesn't go through collect_files, or a future refactor.

Suggest keeping a narrow except SecurePathError as exc: skill_result = _unsafe_plugin_result(exc) ahead of the generic handler, so unsafe filesystem input stays blocking whichever path raises it.

[rng1995]

Fixed in 11c3c76. Restored a narrow except SecurePathError as exc: skill_result = _unsafe_plugin_result(exc) ahead of the generic handler. SkillCollectionError is still handled inside validate(), which marks only the unsafe-input codes as security failures. The new test test_plugin_context_scan_keeps_a_raised_unsafe_path_blocking covers it.

[rng1995]

[altitude] The resolved root fixes the label, but SARIF paths are still inconsistent.

cd parent && skillevaluator validate sample -r sarif still produces mixed artifact URIs for one file: ['SKILL.md', 'sample', 'sample/SKILL.md'] (reproduced). sample/SKILL.md resolved under the scan root .../sample points to a file that doesn't exist.

The root cause is upstream. run_validation receives the lexical relative resolved_target, so some validators emit paths relative to the working directory. Consider passing report_root (absolute) to the validators, or normalizing finding paths against report_root in one place before reporting.

[rng1995]

Fixed in 5ed62ab. I normalized the paths in one place instead of passing an absolute target to run_validation, which would have put absolute host paths into the JSON, HTML and Markdown reports and changed the folder-structure verdicts. The new _content_relative_finding_paths() rewrites finding paths that start with the typed relative target the way validate . reports them, keeping any [skill] label. A path that names a real entry under the content root is left alone. All safety checks still run on the lexical target. A real Tier 1 run of validate skills/sample now gives only SKILL.md as its SARIF URI. Note that JSON, HTML and Markdown file_path values for a relative target now match validate . as well.

[rng1995]

[correctness] An unknown ecosystem now raises KeyError.

_UNVERIFIED_SUGGESTIONS[ecosystem] raises for any ecosystem other than python, npm or container. The previous code fell back to a default suggestion. Suggest _UNVERIFIED_SUGGESTIONS.get(ecosystem, _DEFAULT_UNVERIFIED_SUGGESTION), so a new runner ecosystem (deno, pypi aliases, …) produces an INFO finding instead of aborting the audit.

[rng1995]

Fixed in 759774c. Any other ecosystem now gets a generic _DEFAULT_UNVERIFIED_SUGGESTION instead of a KeyError. Test: test_unverified_finding_for_another_ecosystem_gets_the_default_suggestion.

[rng1995]

[reuse] This duplicates PluginManifestCandidate.read_lenient_text (line 224) line for line.

Suggest one module-level helper, e.g. _read_lenient_manifest(secure_file, declared_path, max_bytes), called by both classes. That way the bound, decoding and error mapping can't drift apart.

[rng1995]

Fixed in fdca13f. Both classes now call one module-level _read_lenient_manifest(). No behaviour change.

[rng1995]

[simplification] Use a lookup instead of the nested conditional for the tone mapping.

Suggested change

	<div class="dashboard-card {{ 'success' if tier3_tone == 'pass' else ('danger' if tier3_tone == 'fail' else ('warning' if tier3_tone == 'warn' else '')) }}">
	<div class="dashboard-card {{ {'pass': 'success', 'fail': 'danger', 'warn': 'warning'}.get(tier3_tone, '') }}">

[rng1995]

Fixed in 620e816, as suggested. The partial-run test still renders the card as warning, and a complete run renders success.

[rng1995]

Re-review: all 9 review comments are addressed in 23b3bcd..620e816, one commit per comment, each with a regression test. Verified locally on 620e816: full suite 13,089 passed, 26 skipped, 0 failed; ruff check and git diff --check clean; all commits signed off. (GitHub does not allow the PR author to approve, so this is recorded as a comment review.)