mirror of
https://github.com/alirezarezvani/claude-skills.git
synced 2026-08-28 04:24:58 +00:00
fix(skill-doctor): address automated review — repo-match transparency, exact-id error dedup
- collect_sessions.py: name-only repo matches (worktree/basename fallback, which can match an unrelated same-named repo) are now recorded per session (repo_match), counted in inventory.json, called out in the summary, and excludable via a new --strict-repo flag - score_aggregator.py: never-scored detection tracks session ids in a set instead of substring-matching error text, so a prefix-style id can no longer suppress another session's never-scored error - redact_secrets(): broader patterns no longer stack markers on top of an already-redacted value - session files read through the MAX_FILE_BYTES cap instead of slurped whole - README deviations 21-22 recorded; count trued to 22 in CLAUDE.md and authoring-notes; prefix-id regression + single-marker redaction verified Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017yFdbrdBnFL8Gw7DLrdg19
This commit is contained in:
Claude
parent
67972e0a61
commit
d12e2586d0
5 changed files with 59 additions and 17 deletions
|
|
@ -178,7 +178,7 @@ Vendors `engineering/skillopt-sleep/` — a byte-for-byte start from [microsoft/
|
|||
|
||||
Rebuild of [warpdotdev/common-skills](https://github.com/warpdotdev/common-skills)' `skill-doctor` (MIT, © Denver Technologies, Inc., pinned at `f3b58c81`) as a full plugin. Harvests the last N days of local Claude Code / Codex sessions scoped to one repo, has the agent judge each condensed transcript against two verbatim-preserved rubrics (efficiency, code quality — labels only, closed tables), measures which installed skills actually fired, and proposes only the skill edits the evidence justifies — one local, self-contained HTML report; nothing is ever uploaded.
|
||||
|
||||
- **20 numbered deviations from upstream** — `engineering/skill-doctor/README.md`'s list is authoritative; `authoring-notes.json` summarizes it. Headline additions: **`score_aggregator.py`, a deterministic gate with no upstream counterpart** — upstream let the scoring LLM average its own scores and assemble `report.json`; here labels map to scores via embedded tables mirroring the rubrics, scores for unsampled sessions are rejected as fabrications, reasons are length-checked, and a suggestion that cites no scored session id (or carries no diff) fails at exit 4. **Collector hardening:** always-on 12-pattern secret redaction before any transcript touches disk (no off switch; per-label counts land in the inventory and report), `chmod 0700/0600` artifacts, plugin-layout skill discovery (`*/skills/*/SKILL.md`), slash-command usage detection mined from `<command-name>` markers before the injection filter drops them. **Renderer replaced:** zero JavaScript (upstream embeds a 1,531-line prebuilt diff bundle + canvas share-image) — pure-CSS diff coloring, native `<details>` collapse, `prefers-color-scheme`, print-to-PDF; Warp branding and the `warp.dev/factories` CTA removed. **Dropped:** the Warp `warp.sqlite` source and its 388-line hand-rolled protobuf decoder (unauditable vendored binary-format code; Warp users should run upstream).
|
||||
- **22 numbered deviations from upstream** — `engineering/skill-doctor/README.md`'s list is authoritative; `authoring-notes.json` summarizes it. Headline additions: **`score_aggregator.py`, a deterministic gate with no upstream counterpart** — upstream let the scoring LLM average its own scores and assemble `report.json`; here labels map to scores via embedded tables mirroring the rubrics, scores for unsampled sessions are rejected as fabrications, reasons are length-checked, and a suggestion that cites no scored session id (or carries no diff) fails at exit 4. **Collector hardening:** always-on 12-pattern secret redaction before any transcript touches disk (no off switch; per-label counts land in the inventory and report), `chmod 0700/0600` artifacts, plugin-layout skill discovery (`*/skills/*/SKILL.md`), slash-command usage detection mined from `<command-name>` markers before the injection filter drops them. **Renderer replaced:** zero JavaScript (upstream embeds a 1,531-line prebuilt diff bundle + canvas share-image) — pure-CSS diff coloring, native `<details>` collapse, `prefers-color-scheme`, print-to-PDF; Warp branding and the `warp.dev/factories` CTA removed. **Dropped:** the Warp `warp.sqlite` source and its 388-line hand-rolled protobuf decoder (unauditable vendored binary-format code; Warp users should run upstream).
|
||||
- Preserved verbatim: both scoring rubrics (`scorers/`) and the skill-improvement filing bar ("would a competent agent with the current instructions still fail this way?"), now §1–§2 of `references/skill_edit_governance.md`. Zero suggestions clearing the bar is a valid, reportable success; proposed edits stay staged under the run's scratch dir until an explicit per-skill yes, and proposals for this repo's skills must pass write-a-skill's 6-item checklist.
|
||||
- 3 stdlib tools (all `--help`/`--sample`/`--output json`, typed exit codes; the pipeline smoke-tests end-to-end from `--sample` fixtures including a planted fake secret), 3 references citing 7 sources each, 3 handoff-shape assets, `cs-skill-doctor` agent, `/cs:skill-doctor` command. SKILL.md is a full 6/6 PASS on the write-a-skill checklist. Distinct from `skillopt-sleep` (automated nightly loop; this is one interactive graded pass), `write-a-skill` (authors from expertise; this improves from evidence), and self-eval (grades the current session; this grades a window). **Counters** (this branch merged `dev` after `agent-memory` landed, so these deltas sit on top of it): skills 378 → 379; tools 703 → 706; refs 820 → 823; agents 110 → 111; commands 130 → 131; plugins 95 → 96 (verified via `scripts/derive_counters.py --check`).
|
||||
|
||||
|
|
|
|||
|
|
@ -3,6 +3,6 @@
|
|||
"derived_from": "https://github.com/warpdotdev/common-skills/tree/f3b58c81d1cfd5d8eabf2e32edb32db2b0573923/.agents/skills/skill-doctor",
|
||||
"upstream_license": "MIT",
|
||||
"upstream_copyright": "Denver Technologies, Inc. (Warp)",
|
||||
"derivation_note": "Rebuild, not a verbatim vendor — README.md's numbered 'Deviations from upstream' list is authoritative. Preserved verbatim: both scoring rubrics and the skill-improvement filing bar. Rebuilt: the collector (adds secret redaction with per-label counts, 0700/0600 artifact permissions, plugin-layout and slash-command skill detection, --sample/--output json house CLI contract; drops the Warp sqlite/protobuf path — Warp users should use upstream). Added: score_aggregator.py, a deterministic gate that owns all scoring arithmetic (upstream had the LLM compute Step 3) and enforces evidence traceability on suggestions. Replaced: the report renderer — zero JavaScript instead of a 1,531-line prebuilt diff bundle and canvas share-image, native <details> collapse, prefers-color-scheme, print CSS; Warp branding, pixel-mark and factories CTA removed."
|
||||
"derivation_note": "Rebuild, not a verbatim vendor — README.md's numbered 'Deviations from upstream' list is authoritative. Preserved verbatim: both scoring rubrics and the skill-improvement filing bar. Rebuilt: the collector (adds secret redaction with per-label counts, 0700/0600 artifact permissions, plugin-layout and slash-command skill detection, --sample/--output json house CLI contract; drops the Warp sqlite/protobuf path — Warp users should use upstream). Added: score_aggregator.py, a deterministic gate that owns all scoring arithmetic (upstream had the LLM compute Step 3) and enforces evidence traceability on suggestions. Replaced: the report renderer — zero JavaScript instead of a 1,531-line prebuilt diff bundle and canvas share-image, native <details> collapse, prefers-color-scheme, print CSS; Warp branding, pixel-mark and factories CTA removed. Post-review hardening: per-session repo_match transparency with a --strict-repo flag (upstream's name-only fallback could pull in an unrelated same-named repo), and bounded session-file reads."
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -139,6 +139,19 @@ in `.claude-plugin/authoring-notes.json` summarizes it; if the two ever disagree
|
|||
`suggestions` fields refuse with exit 3 and point to the aggregator, instead
|
||||
of upstream's KeyError traceback.
|
||||
|
||||
**Post-review hardening (added during PR review)**
|
||||
|
||||
21. **Repo-scoping transparency + `--strict-repo`.** Upstream's worktree/basename
|
||||
fallback silently treats any directory named like the repo as the repo — an
|
||||
unrelated project sharing a common name (`backend`, `app`) could leak its
|
||||
sessions into the run. Each session now records how it matched
|
||||
(`repo_match: "path" | "name"`), name-only matches are counted in
|
||||
`inventory.json` (`sessions_matched_by_name_only`) and called out in the
|
||||
collector's summary, and `--strict-repo` disables the fallback entirely.
|
||||
22. **Bounded session reads.** Session files are read through a
|
||||
`MAX_FILE_BYTES` cap instead of upstream's slurp-then-truncate, so a
|
||||
pathological multi-hundred-MB JSONL never lands in memory whole.
|
||||
|
||||
**Docs & governance**
|
||||
|
||||
19. **References rewritten as cited canon.** Upstream ships one reference
|
||||
|
|
|
|||
|
|
@ -74,6 +74,10 @@ def redact_secrets(text, counter=None):
|
|||
return text
|
||||
for label, pattern in REDACTION_PATTERNS:
|
||||
def _sub(match, _label=label):
|
||||
# A broader pattern (e.g. env-secret) re-matching an already-redacted
|
||||
# value would just stack markers; leave prior redactions alone.
|
||||
if "[REDACTED:" in match.group(0):
|
||||
return match.group(0)
|
||||
if counter is not None:
|
||||
counter[_label] = counter.get(_label, 0) + 1
|
||||
groups = match.groups()
|
||||
|
|
@ -105,6 +109,9 @@ def parse_args(argv=None):
|
|||
p.add_argument("--no-skill", type=int, default=4, help="max sampled sessions that used no skill (default 4)")
|
||||
p.add_argument("--skills-dir", action="append", default=[], help="extra skills directory to scan (repeatable)")
|
||||
p.add_argument("--include-subagents", action="store_true", help="include subagent/sidechain sessions")
|
||||
p.add_argument("--strict-repo", action="store_true",
|
||||
help="only include sessions whose cwd resolves inside the repo "
|
||||
"(disables the worktree/basename fallback, which can match an unrelated repo of the same name)")
|
||||
p.add_argument("--out", default=None, help="output directory (default: a fresh dir under the system temp dir)")
|
||||
p.add_argument("--output", choices=("text", "json"), default="text", help="summary format on stdout")
|
||||
p.add_argument("--sample", action="store_true",
|
||||
|
|
@ -245,11 +252,10 @@ def new_stats():
|
|||
def parse_claude_session(path, skill_names, include_subagents):
|
||||
"""Normalize one Claude Code JSONL session to the shared transcript shape."""
|
||||
try:
|
||||
raw = path.read_text(errors="replace")
|
||||
with open(path, "rb") as fh:
|
||||
raw = fh.read(MAX_FILE_BYTES).decode("utf-8", errors="replace")
|
||||
except OSError:
|
||||
return None
|
||||
if len(raw) > MAX_FILE_BYTES:
|
||||
raw = raw[:MAX_FILE_BYTES]
|
||||
|
||||
meta = {}
|
||||
stats = new_stats()
|
||||
|
|
@ -385,11 +391,10 @@ def parse_claude_session(path, skill_names, include_subagents):
|
|||
def parse_codex_session(path, skill_names, include_subagents):
|
||||
"""Normalize one Codex rollout JSONL session to the shared transcript shape."""
|
||||
try:
|
||||
raw = path.read_text(errors="replace")
|
||||
with open(path, "rb") as fh:
|
||||
raw = fh.read(MAX_FILE_BYTES).decode("utf-8", errors="replace")
|
||||
except OSError:
|
||||
return None
|
||||
if len(raw) > MAX_FILE_BYTES:
|
||||
raw = raw[:MAX_FILE_BYTES]
|
||||
|
||||
meta = {}
|
||||
stats = new_stats()
|
||||
|
|
@ -508,18 +513,26 @@ def render_transcript(meta, stats, skills_used, entries, redaction_counter):
|
|||
return "\n".join(lines)
|
||||
|
||||
|
||||
def session_matches_repo(cwd, repo):
|
||||
"""True when a session's recorded cwd belongs to this repo (prefix or
|
||||
worktree-style basename match; see upstream rationale)."""
|
||||
def repo_match_mode(cwd, repo):
|
||||
"""How a session's recorded cwd relates to the target repo.
|
||||
|
||||
Returns "path" when cwd resolves inside the repo root (a certain match),
|
||||
"name" when only the worktree/basename heuristic matches (the directory
|
||||
name equals the repo's — this can false-positive on an unrelated repo that
|
||||
shares the name, so callers record it and --strict-repo disables it), or
|
||||
None for no match.
|
||||
"""
|
||||
if not cwd:
|
||||
return False
|
||||
return None
|
||||
p = Path(cwd)
|
||||
try:
|
||||
if p.resolve().is_relative_to(repo):
|
||||
return True
|
||||
return "path"
|
||||
except (OSError, ValueError):
|
||||
pass
|
||||
return p.name == repo.name or repo.name in p.parts
|
||||
if p.name == repo.name or repo.name in p.parts:
|
||||
return "name"
|
||||
return None
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
|
|
@ -536,6 +549,7 @@ def sample_sessions(repo):
|
|||
"first_ts": fixed_ts, "last_ts": fixed_ts, "has_code_edits": True},
|
||||
"skills_used": ["sample-skill"],
|
||||
"file": "(synthetic)",
|
||||
"repo_match": "path",
|
||||
"modified_at": fixed_ts,
|
||||
"_entries": [
|
||||
("user", "Fix the failing date parser test"),
|
||||
|
|
@ -556,6 +570,7 @@ def sample_sessions(repo):
|
|||
"first_ts": fixed_ts, "last_ts": fixed_ts, "has_code_edits": False},
|
||||
"skills_used": [],
|
||||
"file": "(synthetic)",
|
||||
"repo_match": "path",
|
||||
"modified_at": fixed_ts,
|
||||
"_entries": [
|
||||
("user", "What does the release script do?"),
|
||||
|
|
@ -658,13 +673,15 @@ def main(argv=None):
|
|||
if parsed is None:
|
||||
continue
|
||||
meta, stats, entries, skills_used = parsed
|
||||
if not session_matches_repo(meta.get("cwd"), repo):
|
||||
match = repo_match_mode(meta.get("cwd"), repo)
|
||||
if match is None or (args.strict_repo and match != "path"):
|
||||
continue
|
||||
in_repo_count += 1
|
||||
if stats["assistant_turns"] < 1 or stats["tool_calls"] < 1:
|
||||
continue
|
||||
sessions.append({"harness": "claude", "meta": meta, "stats": stats,
|
||||
"skills_used": skills_used, "file": str(path),
|
||||
"repo_match": match,
|
||||
"modified_at": mtime.isoformat(), "_entries": entries})
|
||||
elif args.harness == "claude":
|
||||
print(f"error: Claude Code project history not found at {claude_projects}", file=sys.stderr)
|
||||
|
|
@ -682,13 +699,15 @@ def main(argv=None):
|
|||
if parsed is None:
|
||||
continue
|
||||
meta, stats, entries, skills_used = parsed
|
||||
if not session_matches_repo(meta.get("cwd"), repo):
|
||||
match = repo_match_mode(meta.get("cwd"), repo)
|
||||
if match is None or (args.strict_repo and match != "path"):
|
||||
continue
|
||||
in_repo_count += 1
|
||||
if stats["assistant_turns"] < 1 or stats["tool_calls"] < 1:
|
||||
continue
|
||||
sessions.append({"harness": "codex", "meta": meta, "stats": stats,
|
||||
"skills_used": skills_used, "file": str(path),
|
||||
"repo_match": match,
|
||||
"modified_at": mtime.isoformat(), "_entries": entries})
|
||||
elif args.harness == "codex":
|
||||
print(f"error: Codex home not found at {codex_home}", file=sys.stderr)
|
||||
|
|
@ -723,6 +742,7 @@ def main(argv=None):
|
|||
"sessions_sampled": len(sampled_keys),
|
||||
"skills_found": len(skills),
|
||||
"skills_used": sum(1 for v in skill_usage.values() if v > 0),
|
||||
"sessions_matched_by_name_only": sum(1 for s in sessions if s.get("repo_match") == "name"),
|
||||
},
|
||||
"sessions": sessions,
|
||||
}
|
||||
|
|
@ -742,6 +762,9 @@ def main(argv=None):
|
|||
f"{st['sessions_in_repo']} in repo, {st['sessions_considered']} scoreable")
|
||||
print(f"sessions sampled: {st['sessions_sampled']} -> {transcripts_dir}")
|
||||
print(f"secrets redacted: {sum(redaction_counter.values())}")
|
||||
if st["sessions_matched_by_name_only"]:
|
||||
print(f"note: {st['sessions_matched_by_name_only']} session(s) matched only by "
|
||||
"directory name (worktree heuristic) — pass --strict-repo to exclude them")
|
||||
print(f"inventory: {out_dir / 'inventory.json'}")
|
||||
return 0
|
||||
|
||||
|
|
|
|||
|
|
@ -120,6 +120,7 @@ def validate(inventory, scores, suggestions):
|
|||
return (["scores file has no 'sessions' list"], warnings, {})
|
||||
|
||||
per_session = {}
|
||||
sids_with_errors = set()
|
||||
for i, entry in enumerate(entries):
|
||||
sid = entry.get("session_id")
|
||||
if not sid:
|
||||
|
|
@ -131,6 +132,7 @@ def validate(inventory, scores, suggestions):
|
|||
if sid in per_session:
|
||||
errors.append(f"duplicate score entry for '{sid}'")
|
||||
continue
|
||||
errors_before = len(errors)
|
||||
record = {}
|
||||
for scorer, table in (("efficiency", EFFICIENCY_LABELS), ("code_quality", CODE_QUALITY_LABELS)):
|
||||
block = entry.get(scorer)
|
||||
|
|
@ -148,6 +150,8 @@ def validate(inventory, scores, suggestions):
|
|||
f"({len(reason)} chars < {MIN_REASON_CHARS}) — cite specifics from the transcript")
|
||||
continue
|
||||
record[scorer] = {"label": label, "score": table[label], "reason": reason}
|
||||
if len(errors) > errors_before:
|
||||
sids_with_errors.add(sid)
|
||||
if len(record) == 2:
|
||||
if (record["code_quality"]["label"] == "insufficient_evidence"
|
||||
and sampled[sid].get("stats", {}).get("has_code_edits")):
|
||||
|
|
@ -155,8 +159,10 @@ def validate(inventory, scores, suggestions):
|
|||
"confirm the transcript truly hid the diff")
|
||||
per_session[sid] = record
|
||||
|
||||
# Exact-id tracking, not substring matching against error text: one session
|
||||
# id being a prefix of another must not suppress its never-scored error.
|
||||
for sid in sampled:
|
||||
if sid not in per_session and not any(sid in e for e in errors):
|
||||
if sid not in per_session and sid not in sids_with_errors:
|
||||
errors.append(f"sampled session '{sid}' was never scored")
|
||||
|
||||
findings = scores.get("top_findings")
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue