From d12e2586d05e39c58a811d54e3f3c39d6077b2ca Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 24 Aug 2026 20:03:06 +0000 Subject: [PATCH] =?UTF-8?q?fix(skill-doctor):=20address=20automated=20revi?= =?UTF-8?q?ew=20=E2=80=94=20repo-match=20transparency,=20exact-id=20error?= =?UTF-8?q?=20dedup?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 Claude-Session: https://claude.ai/code/session_017yFdbrdBnFL8Gw7DLrdg19 --- CLAUDE.md | 2 +- .../.claude-plugin/authoring-notes.json | 2 +- engineering/skill-doctor/README.md | 13 +++++ .../skill-doctor/scripts/collect_sessions.py | 51 ++++++++++++++----- .../skill-doctor/scripts/score_aggregator.py | 8 ++- 5 files changed, 59 insertions(+), 17 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index c5573c2b..b1828ebd 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -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 `` 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 `
` 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 `` 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 `
` 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`). diff --git a/engineering/skill-doctor/.claude-plugin/authoring-notes.json b/engineering/skill-doctor/.claude-plugin/authoring-notes.json index 043d8ddb..62a16ccb 100644 --- a/engineering/skill-doctor/.claude-plugin/authoring-notes.json +++ b/engineering/skill-doctor/.claude-plugin/authoring-notes.json @@ -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
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
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." } } diff --git a/engineering/skill-doctor/README.md b/engineering/skill-doctor/README.md index f6eee464..eed392ae 100644 --- a/engineering/skill-doctor/README.md +++ b/engineering/skill-doctor/README.md @@ -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 diff --git a/engineering/skill-doctor/skills/skill-doctor/scripts/collect_sessions.py b/engineering/skill-doctor/skills/skill-doctor/scripts/collect_sessions.py index 0d1d8e28..155ac792 100644 --- a/engineering/skill-doctor/skills/skill-doctor/scripts/collect_sessions.py +++ b/engineering/skill-doctor/skills/skill-doctor/scripts/collect_sessions.py @@ -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 diff --git a/engineering/skill-doctor/skills/skill-doctor/scripts/score_aggregator.py b/engineering/skill-doctor/skills/skill-doctor/scripts/score_aggregator.py index 3b3b5fd6..3458a96b 100644 --- a/engineering/skill-doctor/skills/skill-doctor/scripts/score_aggregator.py +++ b/engineering/skill-doctor/skills/skill-doctor/scripts/score_aggregator.py @@ -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")