From c6cad533a1d14ce500367495f15b67141dc5723e Mon Sep 17 00:00:00 2001 From: crisperity Date: Fri, 6 Nov 2026 11:08:15 +0100 Subject: [PATCH] code-walkthrough: before/after overview concepts, thread glossary, bump to 1.4.0 --- .claude-plugin/marketplace.json | 2 +- .../assets/diff-review-template.html | 107 +++++++++++++++++- skills/code-walkthrough/prompts/comment.md | 6 + skills/code-walkthrough/prompts/visuals.md | 15 ++- skills/code-walkthrough/scripts/cw_ask.py | 5 + skills/code-walkthrough/scripts/cw_llm.py | 2 +- skills/code-walkthrough/scripts/cw_mcp.py | 63 ++++++++++- skills/code-walkthrough/scripts/cw_run.py | 83 +++++++++++--- skills/code-walkthrough/scripts/render.py | 34 +++++- .../code-walkthrough/scripts/test_cw_ask.py | 59 ++++++++++ .../code-walkthrough/scripts/test_cw_mcp.py | 34 +++++- .../code-walkthrough/scripts/test_cw_run.py | 98 +++++++++++++++- .../code-walkthrough/scripts/test_prompts.py | 2 +- .../code-walkthrough/scripts/test_render.py | 39 ++++++- .../code-walkthrough/scripts/test_visuals.py | 24 ++++ skills/code-walkthrough/scripts/visuals.py | 57 +++++++++- 16 files changed, 587 insertions(+), 43 deletions(-) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 8aa6886..e32c8c9 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -8,7 +8,7 @@ { "name": "code-walkthrough", "description": "code-walkthrough: turn a diff, branch, PR, or existing code into a top-down walkthrough page: overview, story map, call graph, annotated diff, and line comments you hand back to the agent", - "version": "1.3.0", + "version": "1.4.0", "source": "./", "author": { "name": "crisperity" diff --git a/skills/code-walkthrough/assets/diff-review-template.html b/skills/code-walkthrough/assets/diff-review-template.html index 77a4b5a..13bbf70 100644 --- a/skills/code-walkthrough/assets/diff-review-template.html +++ b/skills/code-walkthrough/assets/diff-review-template.html @@ -906,6 +906,12 @@ .cw-turn{margin:8px 0} .cw-turn-q{margin:0;color:var(--muted);font-style:italic;overflow-wrap:anywhere;white-space:pre-wrap} .cw-turn-a{margin:4px 0 0;overflow-wrap:anywhere} + .cw-term{text-decoration:underline dotted;text-underline-offset:3px;cursor:help;-webkit-touch-callout:none} + .cw-term:focus-visible{outline:2px solid var(--accent);outline-offset:1px;border-radius:2px} + .cw-term-pop{position:fixed;z-index:1000;max-width:min(18rem,calc(100vw - 16px));padding:8px 10px; + background:var(--panel2);color:var(--text);border:1px solid var(--border);border-radius:8px; + box-shadow:0 4px 14px rgba(0,0,0,.25);font-size:var(--fs-sm);line-height:1.45;pointer-events:none} + .cw-term-pop[hidden]{display:none} .cw-md>*{margin:6px 0} .cw-md>:first-child{margin-top:0} .cw-md pre{white-space:pre-wrap;max-width:100%;overflow-x:auto} @@ -1197,6 +1203,17 @@ .cw-vis img{max-width:100%} .cw-vis-group{margin:8px 0 14px} + /* First-glance concepts (render.py concepts_html): a card per idea, picture then Before/After/Why. */ + .cw-concept{margin:12px 0;padding:12px;background:var(--panel);border:1px solid var(--border);border-radius:10px} + .cw-concept-title{margin:0 0 4px;font-weight:700;font-size:var(--fs-md)} + .cw-concept .cw-vis{margin:8px 0;padding:8px;background:var(--bg)} + .cw-concept-ba{margin:8px 0 0;display:grid;grid-template-columns:repeat(auto-fit,minmax(200px,1fr));gap:8px} + .cw-concept-ba div{padding:8px 10px;background:var(--bg);border:1px solid var(--border);border-left:3px solid var(--muted);border-radius:6px} + .cw-concept-ba .cw-concept-before{border-left-color:var(--red)} .cw-concept-ba .cw-concept-after{border-left-color:var(--green)} + .cw-concept-ba .cw-concept-why{border-left-color:var(--accent)} + .cw-concept-ba dt{margin:0 0 2px;font-size:var(--fs-sm);font-weight:700;color:var(--muted);text-transform:uppercase;letter-spacing:.04em} + .cw-concept-ba dd{margin:0;font-size:var(--fs-md);line-height:1.4} + /* System change diagram (render.py overview_svg). Kind colour is one custom property per kind so node, chip, legend swatch and edge all read the same theme variable. */ .cw-ov-sub{margin:-4px 0 8px;color:var(--muted);font-size:var(--fs-sm)} @@ -2643,7 +2660,7 @@ return wrap; } - function mdEl(text){ + function mdEl(text,terms){ const frag=document.createDocumentFragment(); const listEl=l=>{ const e=document.createElement(l.ordered?'ol':'ul'); @@ -2661,9 +2678,92 @@ else if(b.kind==='list') frag.append(listEl(b)); else frag.append(richEl('p',b.text)); } + if(Array.isArray(terms)&&terms.length) markTerms(frag,terms); return frag; } + // Glossary: the first whole-word, case-insensitive occurrence of each term outside code and + // links becomes a focusable .cw-term; the definition shows in one shared tooltip. The boundary + // rule mirrors cw_mcp.clean_terms; longer terms go first so "order book" beats "order". + const termDefs=new WeakMap(); + const termRe=term=>new RegExp('(?w.replace(/[.*+?^${}()|[\]\\]/g,'\\$&')).join('\\s+') + +'(?![\\p{L}\\p{N}_])','iu'); + function markTerms(root,terms){ + const valid=terms.filter(t=>t&&typeof t.term==='string'&&typeof t.def==='string'&&t.term.trim()&&t.def); + for(const t of valid.sort((a,b)=>b.term.length-a.term.length)){ + const re=termRe(t.term); + const walker=document.createTreeWalker(root,NodeFilter.SHOW_TEXT); + for(let n=walker.nextNode();n;n=walker.nextNode()){ + const par=n.parentElement; + if(!par||par.closest('code,pre,a,.cw-term')) continue; + const m=re.exec(n.data); + if(!m) continue; + const hit=n.splitText(m.index); + hit.splitText(m[0].length); + const el=mk('span','cw-term',m[0]); + el.tabIndex=0; + termDefs.set(el,t.def); + hit.replaceWith(el); + break; + } + } + } + + const termPop={el:null,anchor:null,timer:0,watch:null}; + function hideTerm(){ + clearTimeout(termPop.timer); + if(termPop.watch) termPop.watch.disconnect(); + if(termPop.el) termPop.el.hidden=true; + if(termPop.anchor) termPop.anchor.removeAttribute('aria-describedby'); + termPop.anchor=null; + } + function showTerm(el){ + if(termPop.anchor===el) return; + hideTerm(); + if(!termPop.el){ + const p=mk('div','cw-term-pop'); p.id='cw-term-pop'; p.setAttribute('role','tooltip'); p.hidden=true; + document.body.append(p); + termPop.el=p; + termPop.watch=new MutationObserver(()=>{ if(termPop.anchor&&!termPop.anchor.isConnected) hideTerm(); }); + } + const p=termPop.el; + p.textContent=termDefs.get(el)||''; + p.style.left=p.style.top='0px'; + p.hidden=false; + el.setAttribute('aria-describedby',p.id); + termPop.anchor=el; + termPop.watch.observe(document.body,{childList:true,subtree:true}); + const r=el.getBoundingClientRect(), w=document.documentElement.clientWidth; + const left=Math.max(8,Math.min(r.left,w-p.offsetWidth-8)); + const below=r.bottom+6+p.offsetHeight<=window.innerHeight||r.tope.target.closest?e.target.closest('.cw-term'):null; + document.addEventListener('pointerover',e=>{ const el=termOf(e); if(el&&e.pointerType==='mouse') showTerm(el); }); + document.addEventListener('pointerout',e=>{ + if(e.pointerType==='mouse'&&termOf(e)&&!(e.relatedTarget&&e.relatedTarget.closest&&e.relatedTarget.closest('.cw-term'))) hideTerm(); + }); + // :focus-visible keeps a touch or mouse tap from opening it; those use hover or the long press. + document.addEventListener('focusin',e=>{ const el=termOf(e); if(el&&el.matches(':focus-visible')) showTerm(el); }); + document.addEventListener('focusout',e=>{ if(termOf(e)) hideTerm(); }); + document.addEventListener('pointerdown',e=>{ + const el=termOf(e); + if(el&&e.pointerType!=='mouse'){ clearTimeout(termPop.timer); termPop.timer=setTimeout(()=>showTerm(el),400); } + else if(!el) hideTerm(); + }); + for(const ev of ['pointerup','pointercancel','pointermove']) document.addEventListener(ev,e=>{ + if(e.pointerType!=='mouse'&&!termPop.anchor) clearTimeout(termPop.timer); + }); + document.addEventListener('contextmenu',e=>{ if(termOf(e)&&termPop.anchor) e.preventDefault(); }); + document.addEventListener('keydown',e=>{ + if(e.key==='Escape'&&termPop.anchor){ hideTerm(); e.stopPropagation(); } + },true); + window.addEventListener('scroll',hideTerm,true); + } + // Pinned locale: the page's own text is English, so a browser-locale relative/absolute // date (e.g. Polish "2 dni temu") would be the odd one out. const RTF=new Intl.RelativeTimeFormat('en-GB',{numeric:'auto'}); @@ -5153,6 +5253,7 @@ } function outcomeEl(t,o){ + if(o.outcome==='terms') return null; if(o.outcome==='draft_comments') return draftCommentsCard(o); if(o.outcome==='task') return taskCard(t,o); if(o.outcome==='page_edit') return pageEditCard(t,o); @@ -5235,7 +5336,7 @@ d.append(mk('p','cw-turn-a cw-turn-err','Error: '+(r.error||'unknown')+(r.remedy?' ('+r.remedy+')':''))); } else { const a=mk('div','cw-turn-a cw-md'); - a.append(mdEl(r.answer||'')); + a.append(mdEl(r.answer||'',r.terms)); if(r.status==='cancelled') a.append(mk('span','cw-stopped','(stopped)')); d.append(a); } @@ -5264,7 +5365,7 @@ } } if((r.status==='error'||r.status==='cancelled')&&isLast) d.append(retryBtn(t,r)); - for(const o of (outcomesByQid.get(r.qid)||new Map()).values()) d.append(outcomeEl(t,o)); + for(const o of (outcomesByQid.get(r.qid)||new Map()).values()){ const oe=outcomeEl(t,o); if(oe) d.append(oe); } return d; } diff --git a/skills/code-walkthrough/prompts/comment.md b/skills/code-walkthrough/prompts/comment.md index 0bedac1..f303628 100644 --- a/skills/code-walkthrough/prompts/comment.md +++ b/skills/code-walkthrough/prompts/comment.md @@ -52,6 +52,12 @@ Call `post_draft_comments` when the reviewer asks you to review the change and l items of `{path, line, side, end_line?, body, suggestion?}`, each on a line inside the diff. They become local drafts the user edits and submits later; nothing reaches GitHub until they press Submit review. +When your reply uses words that are specific to this project or its domain (an internal concept, a +product or ticket term, a house abbreviation) and a newcomer to the codebase would not know them, call +`define_terms` once with up to 5 `{term, def}` pairs, each term spelled as it appears in your reply and +each `def` one or two short sentences. Skip common programming vocabulary (function, API, cache, async) +and anything the reply already explains. Most replies need no glossary. + A comment that begins "Triage this review thread" is the app's fixed triage request, not a request to reword or rewrite. Judge the thread against the head code and use the matching tool or reply. diff --git a/skills/code-walkthrough/prompts/visuals.md b/skills/code-walkthrough/prompts/visuals.md index 9ca3352..abc3f33 100644 --- a/skills/code-walkthrough/prompts/visuals.md +++ b/skills/code-walkthrough/prompts/visuals.md @@ -1,7 +1,16 @@ -You write the visuals for a code walkthrough page: one overview graph of the change, and one -picture for each walkthrough group. Deliver once, by calling `submit_visuals`: +You write the visuals for a code walkthrough page: the concepts of the change, an overview graph, and +one picture for each walkthrough group. Deliver once, by calling `submit_visuals`: - {"overview": {...} | omitted, "groups": [{"index": <0-based group index>, "html": ""}]} + {"concepts": [...] | omitted, "overview": {...} | omitted, + "groups": [{"index": <0-based group index>, "html": ""}]} + +## Concepts + +The change in the fewest ideas: one concept, two only for distinct novelties. Plain sentences, no file +names: `{"title": "1-80 chars", "before": "how it worked", "after": "how it works now", "why": "what that +buys"}`, each at most 200 characters. The task says which half to do. Text: all four fields, no `html`. +Picture: for the concept you are handed, one entry with only `html`, a before -> after diagram with the +difference highlighted. Docs-, tooling- or config-only change: no concepts. ## Overview graph diff --git a/skills/code-walkthrough/scripts/cw_ask.py b/skills/code-walkthrough/scripts/cw_ask.py index ab09118..ff05318 100644 --- a/skills/code-walkthrough/scripts/cw_ask.py +++ b/skills/code-walkthrough/scripts/cw_ask.py @@ -1270,6 +1270,11 @@ def on_usage(usage): record.update(status="error", error=str(e), remedy=None) sweep_outcomes(d, qid, record["thread_id"], on_event) + if record["status"] == "ok": + defined = next((o for o in read_outcomes(d) if o["qid"] == qid and o["outcome"] == "terms"), None) + terms = cw_mcp.clean_terms((defined or {}).get("payload", {}).get("terms"), record.get("answer") or "") + if terms: + record["terms"] = terms if live.get("queued_behind"): record["queued_behind"] = live["queued_behind"] if after_turn: diff --git a/skills/code-walkthrough/scripts/cw_llm.py b/skills/code-walkthrough/scripts/cw_llm.py index 4dbb698..fc69991 100644 --- a/skills/code-walkthrough/scripts/cw_llm.py +++ b/skills/code-walkthrough/scripts/cw_llm.py @@ -338,7 +338,7 @@ def claude_call(profile, system, prompt, *, cwd, tools, schema=None, timeout=300 # cw_mcp imports this module, so the names are literal here; test_cw_task pins them to outcome_tools. THREAD_ALLOWED = ",".join(f"mcp__cw__{n}" for n in ( - "propose_resolve", "propose_page_edit", "propose_github_draft", "propose_task", "post_draft_comments")) + "propose_resolve", "propose_page_edit", "define_terms", "propose_github_draft", "propose_task", "post_draft_comments")) def thread_argv(profile, system, *, mcp_config, add_dir, session_id=None, resume=None): diff --git a/skills/code-walkthrough/scripts/cw_mcp.py b/skills/code-walkthrough/scripts/cw_mcp.py index 1f2fec8..237d791 100644 --- a/skills/code-walkthrough/scripts/cw_mcp.py +++ b/skills/code-walkthrough/scripts/cw_mcp.py @@ -252,6 +252,59 @@ def rpc(tool, args, *, timeout=300): }) +TERMS_MAX, TERM_MAX, TERM_DEF_MAX = 5, 40, 240 + +OUTCOME_TOOLS.append({ + "name": "define_terms", + "description": ("Attach a glossary to your reply: up to 5 domain-specific words or phrases from your reply " + "text, each with a 1-2 sentence definition. The page underlines them and shows the " + "definition on hover. Only terms specific to this project or its domain; never common " + "programming vocabulary (function, API, cache). A term must appear verbatim in your reply. " + "Call it once per turn, and not at all when your reply has no such term."), + "inputSchema": { + "type": "object", "required": ["terms"], + "properties": {"terms": {"type": "array", "maxItems": TERMS_MAX, "items": { + "type": "object", "required": ["term", "def"], + "properties": {"term": {"type": "string", "maxLength": TERM_MAX}, + "def": {"type": "string", "maxLength": TERM_DEF_MAX}}}}}, + }, +}) + + +def _term_re(term): + return re.compile(r"(? (clean, problems, calls).""" messages = [{"role": "system", "content": system_prompt}, {"role": "user", "content": _visuals_user_text( - d, meta, analysis, symdelta, need_calls and group is None) + "\n\n" + scope}] + d, meta, analysis, symdelta, need_calls and name == "overview") + + "\n\n" + scope}] state: dict = {"attempt": 0} def handler(args): state["attempt"] += 1 args = args if isinstance(args, dict) else {} - if group is None: - scoped = {**args, "groups": []} + if name == "concepts": + cs = args.get("concepts") + scoped = {"concepts": [{k: v for k, v in c.items() if k != "html"} for c in cs + if isinstance(c, dict)] if isinstance(cs, list) else []} + elif concept is not None: + cs = args.get("concepts") + pic = next((c.get("html") for c in cs if isinstance(c, dict) + and c.get("index") in (concept[0], None)), None) if isinstance(cs, list) else None + scoped = {"concepts": [{**concept[1], "index": concept[0], "html": pic}]} + elif group is None: + scoped = {**args, "groups": [], "concepts": []} else: gs = args.get("groups") scoped = {"groups": [g for g in gs if isinstance(g, dict) and g.get("index") == group] @@ -1010,8 +1030,11 @@ def handler(args): clean, problems = visuals.check_visuals(scoped, n_groups, ctx) if group is not None and group not in clean["groups"] and not problems: problems = [f"visuals.groups: no picture for group {group}"] + if concept is not None and not (clean.get("concepts") and clean["concepts"][0].get("html")) \ + and not problems: + problems = [f"visuals.concepts: no picture for concept {concept[0]}"] calls = [] - if group is None and need_calls: + if name == "overview" and need_calls: calls, call_problems = visuals.check_calls(args.get("calls"), ctx) problems = problems + call_problems if problems and state["attempt"] < 3: @@ -1024,7 +1047,7 @@ def handler(args): error = None try: cw_llm.run_tools( - profile, messages, tools + [_visuals_submit_tool(need_calls and group is None)], + profile, messages, tools + [_visuals_submit_tool(need_calls and name == "overview")], local_handlers, max_rounds=8, max_tokens=config["max_conversation_tokens"], timeout=config["timeout_s"], on_usage=on_usage, cwd=d / "head") except Exception as e: @@ -1032,7 +1055,7 @@ def handler(args): finally: _write_transcript(d, f"visuals-{name}", profile, messages, result="clean" in state, error=error) if "clean" not in state: - return {"pr": None, "overview": None, "groups": {}}, [error or f"{name}: nothing submitted"], [] + return _empty_visuals(), [error or f"{name}: nothing submitted"], [] return state["clean"], state["problems"] + ([error] if error else []), state["calls"] jobs = [] @@ -1040,33 +1063,57 @@ def handler(args): jobs.append(("overview", "Author only the overview" + (" (and `calls`)." if need_calls else "."), None)) jobs += [(f"group-{i}", f"Author only group {i}: submit one entry in `groups` with index {i}, " "and no overview.", i) for i in range(min(n_groups, visuals.MAX_GROUPS))] - with ThreadPoolExecutor(max_workers=4) as pool: - results = list(pool.map(lambda j: author(*j), jobs)) - clean = {"pr": None, "overview": None, "groups": {}} + clean = _empty_visuals() problems, calls = [], [] if n_groups > visuals.MAX_GROUPS: problems.append(f"visuals.groups: at most {visuals.MAX_GROUPS} pictures, " f"{n_groups - visuals.MAX_GROUPS} dropped") + with ThreadPoolExecutor(max_workers=4) as pool, \ + ThreadPoolExecutor(max_workers=visuals.MAX_CONCEPTS) as pic_pool: + concepts_job = pool.submit(author, "concepts", "Author only `concepts`, text only: no `html`, " + "no overview, no groups.") + futures = [pool.submit(author, *j) for j in jobs] + texts, probs, _ = concepts_job.result() + problems += probs + concepts = texts.get("concepts", []) + pic_futures = [pic_pool.submit( + author, f"concept-{i}", f"Author only the picture for concept {i} ({c['title']}): submit one " + "entry in `concepts` with only its `html`, a before -> after diagram. No overview, no groups.", + None, (i, c)) for i, c in enumerate(concepts)] + results = [f.result() for f in futures] + pic_results = [f.result() for f in pic_futures] for c, probs, cs in results: clean["pr"] = clean["pr"] or c["pr"] clean["overview"] = clean["overview"] or c["overview"] clean["groups"].update(c["groups"]) problems += probs calls += cs - size = {i: len(h.encode("utf-8", "replace")) for i, h in clean["groups"].items()} + for text, (c, probs, _) in zip(concepts, pic_results): + pic = c["concepts"][0].get("html") if c.get("concepts") else None + clean["concepts"].append({**text, "html": pic} if pic else text) + problems += probs + size = {("g", i): len(h.encode("utf-8", "replace")) for i, h in clean["groups"].items()} + size.update({("c", i): len(c["html"].encode("utf-8", "replace")) + for i, c in enumerate(clean["concepts"]) if c.get("html")}) while sum(size.values()) > visuals.TOTAL_MAX: - big = max(size, key=size.get) - del size[big], clean["groups"][big] - problems.append(f"visuals.groups: picture {big} dropped, over the {visuals.TOTAL_MAX // 1024} KB total") + kind, i = big = max(size, key=size.get) + del size[big] + if kind == "g": + del clean["groups"][i] + else: + del clean["concepts"][i]["html"] + problems.append(f"visuals.{'groups' if kind == 'g' else 'concepts'}: picture {i} dropped, " + f"over the {visuals.TOTAL_MAX // 1024} KB total") if calls: _write_model_symdelta(d, meta, symdelta, calls) else: (d / "symdelta.model.json").unlink(missing_ok=True) - if not clean["overview"] and not clean["pr"] and not clean["groups"]: + if not (clean["overview"] or clean["pr"] or clean["groups"] or clean["concepts"]): raise RuntimeError("no usable pictures; dropped: " + "; ".join(problems)[:500]) cw_store.write_json(out, { "overview": clean["overview"], "pr": clean["pr"], + **({"concepts": clean["concepts"]} if clean["concepts"] else {}), "groups": [{"index": i, "html": h} for i, h in sorted(clean["groups"].items())]}) _emit_step(d, on_event, "visuals", "ok", **({"lines": problems} if problems else {})) return True diff --git a/skills/code-walkthrough/scripts/render.py b/skills/code-walkthrough/scripts/render.py index a4655f6..eac4b90 100644 --- a/skills/code-walkthrough/scripts/render.py +++ b/skills/code-walkthrough/scripts/render.py @@ -254,6 +254,30 @@ def overview_html(ov, explain, target_of=lambda item: "", symbols=""): f'
{chips}
{symbols}') +_CONCEPT_ROWS = {"before": "Before", "after": "After", "why": "Why"} + + +def concepts_html(concepts, explain): + """First on the page: per concept its picture (if any) and a Before / After / Why block. The text + is the fallback when there is no picture; malformed entries are skipped.""" + cards = [] + for c in concepts if isinstance(concepts, list) else []: + c = c if isinstance(c, dict) else {} + title, html = c.get("title"), c.get("html") + if not all(isinstance(v, str) and v for v in (title, *(c.get(k) for k in _CONCEPT_ROWS))): + continue + pic = (f'
{html}
' + if isinstance(html, str) and html else "") + rows = "".join(f'
{label}
{_html_prose(c[k])}
' + for k, label in _CONCEPT_ROWS.items()) + cards.append(f'

{escape(title)}

' + f'{pic}
{rows}
') + if not cards: + return "" + return (f'
' + f'

{"The idea" if explain else "What changes"}

{"".join(cards)}
') + + _CTX_SOURCES = ("jira", "github", "linear", "other") _CTX_FOUND_IN = {"branch": "branch name", "pr_body": "PR description", "commit": "a commit message", "link": "a pasted link"} _CTX_STATUS = {"covered": ("ok", "\u2713"), "partial": ("pt", "\u25d0"), "missing": ("no", "\u2717"), "extra": ("ex", "+")} @@ -448,6 +472,10 @@ def render_html(analysis, files, template, walkthrough="", links=None, title=Non if not explain: body.append(context_html(context, files, (links or {}).get("head_sha", ""), building, ticket_hint)) + clean = visuals or {} + if concepts_block := concepts_html(clean.get("concepts"), explain): + body.append(concepts_block) + overview = _paragraphs(analysis.get("overview")) if overview: # One heading in both modes: the topic a cold reader needs is the same shape whether @@ -461,7 +489,6 @@ def render_html(analysis, files, template, walkthrough="", links=None, title=Non symbols_block = ('
All symbols' f'{all_symbols.rstrip(chr(10))}
') if all_symbols else "" - clean = visuals or {} pr_picture = clean.get("pr") ov = clean.get("overview") if ov: @@ -527,8 +554,9 @@ def load_visuals(path, n_groups, placed=None, ctx=None): del clean["groups"][idx] problems.append(f"visuals.groups[{idx}]: group has no files on this page") raw = raw if isinstance(raw, dict) else {} - total = int(bool(raw.get("pr"))) + int(raw.get("overview") is not None) + (len(raw["groups"]) if isinstance(raw.get("groups"), list) else 0) - kept = int(bool(clean["pr"])) + int(bool(clean["overview"])) + len(clean["groups"]) + total = int(bool(raw.get("pr"))) + int(raw.get("overview") is not None) + (len(raw["groups"]) if isinstance(raw.get("groups"), list) else 0) \ + + (len(raw["concepts"]) if isinstance(raw.get("concepts"), list) else 0) + kept = int(bool(clean["pr"])) + int(bool(clean["overview"])) + len(clean["groups"]) + len(clean.get("concepts", [])) return clean, [f"visuals: {kept} kept, {max(total - kept, 0)} dropped"] + [ f"visuals: {problem}" for problem in problems] diff --git a/skills/code-walkthrough/scripts/test_cw_ask.py b/skills/code-walkthrough/scripts/test_cw_ask.py index d0992b0..2986c16 100644 --- a/skills/code-walkthrough/scripts/test_cw_ask.py +++ b/skills/code-walkthrough/scripts/test_cw_ask.py @@ -1274,6 +1274,65 @@ def _forge_outcomes(d, qid, records): path.write_text("".join(json.dumps(r) + "\n" for r in records)) +def test_answer_stores_defined_terms_that_occur_in_the_answer(monkeypatch): + with cw_testlib.temp_home() as home: + d = _claude_done(home) + + def fake_turn(d, record, *a, **kw): + cw_ask.write_turn_anchor(d, record["qid"], record["thread_id"], dict(ANCHOR_LINE), "x") + ok, text = cw_mcp.accept_outcome(d, record["qid"], "define_terms", {"terms": [ + {"term": "Order Book", "def": "Open bids."}, {"term": "ghost", "def": "Not in the answer."}]}) + assert ok, text + record["answer"] = "The order book is rebuilt on start." + + monkeypatch.setattr(cw_ask, "_claude_turn", fake_turn) + record = cw_ask.answer(d, "q1", dict(ANCHOR_LINE), "x") + assert record["status"] == "ok" and record["terms"] == [{"term": "Order Book", "def": "Open bids."}] + assert cw_ask.read_qa(d)[-1]["terms"] == record["terms"] + + +def test_define_terms_second_call_is_rejected_and_discards_are_reported(): + with cw_testlib.temp_home() as home: + d = _claude_done(home) + cw_ask.write_turn_anchor(d, "q1", "q1", dict(ANCHOR_LINE), "x") + ok, text = cw_mcp.accept_outcome(d, "q1", "define_terms", {"terms": [ + {"term": "alpha", "def": "a"}, {"term": "ALPHA", "def": "dup"}, {"term": "x"}]}) + assert ok and "kept 1 of 3" in text + ok, text = cw_mcp.accept_outcome(d, "q1", "define_terms", {"terms": [{"term": "beta", "def": "b"}]}) + assert not ok and "already" in text + + +def test_failed_or_cancelled_turn_stores_no_terms(monkeypatch): + with cw_testlib.temp_home() as home: + d = _claude_done(home) + + def fake_turn(d, record, *a, **kw): + cw_ask.write_turn_anchor(d, record["qid"], record["thread_id"], dict(ANCHOR_LINE), "x") + cw_mcp.accept_outcome(d, record["qid"], "define_terms", {"terms": [{"term": "alpha", "def": "a"}]}) + record["answer"] = "alpha is here" + raise cw_store.CWError("boom") + + monkeypatch.setattr(cw_ask, "_claude_turn", fake_turn) + record = cw_ask.answer(d, "q1", dict(ANCHOR_LINE), "x") + assert record["status"] == "error" and "terms" not in record + + +def test_sweep_drops_forged_define_terms(): + with cw_testlib.temp_home() as home: + d = _claude_done(home) + cw_ask.write_turn_anchor(d, "q1", "q1", cw_ask.validate_anchor(dict(ANCHOR_LINE)), "x", "page") + _forge_outcomes(d, "q1", [{"oid": "o-t1", "name": "define_terms", "arguments": {"terms": "nope"}}]) + cw_ask.sweep_outcomes(d, "q1", "q1") + assert cw_ask.read_outcomes(d) == [] + + +def test_answer_without_defined_terms_has_no_terms_key(monkeypatch): + with cw_testlib.temp_home() as home: + d = _claude_done(home) + monkeypatch.setattr(cw_ask, "_claude_turn", lambda d, record, *a, **kw: record.update(answer="plain")) + assert "terms" not in cw_ask.answer(d, "q1", dict(ANCHOR_LINE), "x") + + def test_sweep_drops_forged_outcome_records(): with cw_testlib.temp_home() as home: d = _claude_done(home) diff --git a/skills/code-walkthrough/scripts/test_cw_mcp.py b/skills/code-walkthrough/scripts/test_cw_mcp.py index d9fa860..450e58f 100644 --- a/skills/code-walkthrough/scripts/test_cw_mcp.py +++ b/skills/code-walkthrough/scripts/test_cw_mcp.py @@ -280,7 +280,8 @@ def test_outcomes_server_accepts_and_rejects(): rpc = _Rpc("outcomes", "--dir", str(d), "--qid", "q1") try: assert [t["name"] for t in rpc.call("tools/list")["result"]["tools"]] == [ - "propose_resolve", "propose_page_edit", "propose_github_draft", "propose_task", "post_draft_comments"] + "propose_resolve", "propose_page_edit", "define_terms", "propose_github_draft", "propose_task", + "post_draft_comments"] rpc.proc.stdin.write("garbage{\n") rpc.proc.stdin.flush() assert rpc.call("ping")["result"] == {} @@ -301,8 +302,9 @@ def test_outcomes_server_accepts_and_rejects(): def test_outcome_server_lists_tools_by_mode(): - review = ["propose_resolve", "propose_page_edit", "propose_github_draft", "propose_task", "post_draft_comments"] - full = ["propose_resolve", "propose_page_edit", "propose_github_draft", "post_draft_comments", + review = ["propose_resolve", "propose_page_edit", "define_terms", "propose_github_draft", "propose_task", + "post_draft_comments"] + full = ["propose_resolve", "propose_page_edit", "define_terms", "propose_github_draft", "post_draft_comments", "set_context", "reload_walkthrough"] assert [t["name"] for t in cw_mcp.outcome_tools()] == review assert [t["name"] for t in cw_mcp.outcome_tools("full")] == full @@ -735,6 +737,32 @@ def test_set_context_outcome_cleans_and_needs_no_reference(): assert "16384" in cw_mcp.check_outcome(anchor, "set_context", {"context": {"purpose": "y" * 17000}}) +def test_define_terms_validates_dedupes_caps_and_needs_no_reference(): + anchor = {"anchors": [{"kind": "free", "quote": ""}]} + pair = lambda t, d="A thing.": {"term": t, "def": d} + assert cw_mcp.check_outcome(anchor, "define_terms", {"terms": [pair("Order Book")]}) is None + for bad in (None, "x", [], [1], [{"term": "x"}], [pair("a")], [pair("ok", " ")], [pair("n" * 41)], [{"term": 3, "def": "d"}]): + assert "terms must be" in cw_mcp.check_outcome(anchor, "define_terms", {"terms": bad}) + assert "already" in cw_mcp.check_outcome(anchor, "define_terms", {"terms": [pair("Order Book")]}, {"terms"}) + many = [pair(f"term{i}") for i in range(8)] + [pair("TERM0", "dup")] + cleaned = cw_mcp.clean_terms(many + [pair(" spaced out ", "d" * 300)]) + assert [t["term"] for t in cleaned] == [f"term{i}" for i in range(5)] + assert cw_mcp.clean_terms([pair(" spaced out ", "d" * 300)]) == [{"term": "spaced out", "def": "d" * 240}] + assert cw_mcp.OUTCOME_KINDS["define_terms"]["payload"]({"terms": many}, None) == {"terms": cleaned} + + +def test_clean_terms_keeps_only_whole_words_present_in_the_text(): + text = "The Order Book feeds the matcher; ordering is unrelated. Uses C++ (x) too." + pairs = [{"term": t, "def": "d"} for t in ("order book", "order", "match", "absent", "C++ (x)")] + assert [t["term"] for t in cw_mcp.clean_terms(pairs, text)] == ["order book", "order", "C++ (x)"] + + +def test_clean_terms_ignores_code_spans_and_matches_across_whitespace(): + pairs = [{"term": t, "def": "d"} for t in ("matcher", "order book", "fenced", "Élan")] + text = "Use `matcher` here.\n\n```\nfenced()\n```\nThe order\n book and élan; xÉlan no." + assert [t["term"] for t in cw_mcp.clean_terms(pairs, text)] == ["order book", "Élan"] + + def test_mcp_switches_to_a_newer_plugin_install_and_replays_the_request(): import shutil skill = Path(cw_mcp.__file__).resolve().parent.parent diff --git a/skills/code-walkthrough/scripts/test_cw_run.py b/skills/code-walkthrough/scripts/test_cw_run.py index cf909ad..aff854e 100644 --- a/skills/code-walkthrough/scripts/test_cw_run.py +++ b/skills/code-walkthrough/scripts/test_cw_run.py @@ -308,12 +308,17 @@ def _submit(args): def _scope_of(body): text = cw_testlib.last_user_text(body) + if "Author only `concepts`" in text: + return "concepts" + if "Author only the picture for concept " in text: + return "concept-" + text.rsplit("Author only the picture for concept ", 1)[1].split(" ")[0] return "overview" if "Author only the overview" in text else "group-" + text.rsplit("Author only group ", 1)[1].split(":")[0] def _scoped(**replies): """Scripted replies per visuals call (overview=[...], group_0=[...]); each call walks its own list.""" seen, lock = {}, threading.Lock() + replies.setdefault("concepts", [_submit({"concepts": []})]) def script(body, _n): scope = _scope_of(body) @@ -361,7 +366,7 @@ def _overview(ref="foo.py:X"): def test_visuals_submit_tool_schema_has_overview_and_no_pr(): props = cw_run._visuals_submit_tool()["function"]["parameters"]["properties"] - assert "pr" not in props and set(props) == {"overview", "groups"} + assert "pr" not in props and set(props) == {"overview", "concepts", "groups"} node = props["overview"]["properties"]["nodes"]["items"]["properties"] assert node["kind"]["enum"] == ["same", "new", "changed", "guard", "removed"] assert node["label"]["maxLength"] == 22 and props["overview"]["properties"]["nodes"]["maxItems"] == 12 @@ -390,7 +395,7 @@ def test_visuals_pass_prefers_the_visuals_role_over_prose(): {"analysis": "p", "prose": "p", "visuals": "v"}) stub.script = {"vis-m": [_submit({"pr": None, "groups": [{"index": 0, "html": "

n

"}]})]} assert cw_run._run_visuals(d, meta, cw_store.load_config(), None) - assert stub.count("vis-m") == 2 and stub.count("prose-m") == 0 + assert stub.count("vis-m") == 3 and stub.count("prose-m") == 0 def test_visuals_pass_keeps_the_clean_part_on_the_final_attempt(): @@ -403,7 +408,7 @@ def test_visuals_pass_keeps_the_clean_part_on_the_final_attempt(): assert cw_run._run_visuals(d, meta, cw_store.load_config(), None) saved = json.loads((d / "visuals.json").read_text()) assert saved["pr"] is None and saved["groups"][0]["index"] == 0 - assert stub.count("prose-m") == 4 + assert stub.count("prose-m") == 5 def test_visuals_pass_fails_when_nothing_clean_survives_the_final_attempt(): @@ -418,6 +423,86 @@ def test_visuals_pass_fails_when_nothing_clean_survives_the_final_attempt(): assert step["status"] == "failed" and " is not allowed" in step["error"] +CONCEPT = {"title": "Live config", "before": "Constants, redeploy to change", + "after": "Config is read live", "why": "No deploy to tune a limit"} + + +def _concept_dir(home, tmp, stub, texts=(CONCEPT,), pics=("

pic

",)): + d, meta = _visuals_dir(home, tmp, stub) + stub.script = _scoped( + concepts=[_submit({"concepts": [{**t, "html": "

ignored

"} for t in texts]})], + group_0=[G0], + concept_0=[_submit({"concepts": [{"index": 0, "html": pics[0]}]})] if pics else [_submit({})], + concept_1=[_submit({"concepts": [{"index": 1, "html": pics[-1]}]})] if pics else [_submit({})]) + return d, meta + + +def test_concepts_are_drawn_after_their_text_and_stored_with_the_picture(): + with cw_testlib.temp_home() as home, tempfile.TemporaryDirectory() as tmp: + with cw_testlib.StubLLM({}) as stub: + d, meta = _concept_dir(home, tmp, stub, texts=(CONCEPT, {**CONCEPT, "title": "Two"}), + pics=("

pic0

", "

pic1

")) + assert cw_run._run_visuals(d, meta, cw_store.load_config(), None) + saved = json.loads((d / "visuals.json").read_text()) + assert saved["concepts"] == [{**CONCEPT, "html": "

pic0

"}, + {**CONCEPT, "title": "Two", "html": "

pic1

"}] + assert "Live config" in cw_testlib.last_user_text(_requests_for(stub, "concept-0")[0]) + assert {p.name for p in (d / "runs").glob("visuals-concept-*")} == { + "visuals-concept-0-1.json", "visuals-concept-1-1.json"} + + +def test_a_failed_concept_picture_keeps_the_text_as_fallback(): + with cw_testlib.temp_home() as home, tempfile.TemporaryDirectory() as tmp: + with cw_testlib.StubLLM({}) as stub: + d, meta = _concept_dir(home, tmp, stub, pics=("",)) + assert cw_run._run_visuals(d, meta, cw_store.load_config(), None) + saved = json.loads((d / "visuals.json").read_text()) + assert saved["concepts"] == [CONCEPT] + assert len(_requests_for(stub, "concept-0")) == 3 + + +def test_bad_concept_text_leaves_the_groups_intact(): + with cw_testlib.temp_home() as home, tempfile.TemporaryDirectory() as tmp: + with cw_testlib.StubLLM({}) as stub: + d, meta = _visuals_dir(home, tmp, stub) + bad = _submit({"concepts": [{**CONCEPT, "why": ""}]}) + stub.script = _scoped(concepts=[bad], group_0=[G0]) + assert cw_run._run_visuals(d, meta, cw_store.load_config(), None) + saved = json.loads((d / "visuals.json").read_text()) + assert "concepts" not in saved and saved["groups"] == [{"index": 0, "html": "

g

"}] + assert not _requests_for(stub, "concept-0") + + +def test_a_concept_picture_is_taken_from_the_entry_with_its_own_index(): + with cw_testlib.temp_home() as home, tempfile.TemporaryDirectory() as tmp: + with cw_testlib.StubLLM({}) as stub: + d, meta = _concept_dir(home, tmp, stub) + stub.script = _scoped( + concepts=[_submit({"concepts": [CONCEPT]})], group_0=[G0], + concept_0=[_submit({"concepts": [{"index": 1, "html": "

wrong

"}, + {"index": 0, "html": "

right

"}]})]) + assert cw_run._run_visuals(d, meta, cw_store.load_config(), None) + assert json.loads((d / "visuals.json").read_text())["concepts"][0]["html"] == "

right

" + + +def test_total_cap_evicts_the_biggest_picture_and_the_saved_file_reloads_clean(monkeypatch): + import render + monkeypatch.setattr(cw_run.visuals, "TOTAL_MAX", 15 * 1024) + with cw_testlib.temp_home() as home, tempfile.TemporaryDirectory() as tmp: + with cw_testlib.StubLLM({}) as stub: + d, meta = _visuals_dir(home, tmp, stub) + big, small = "

" + "x" * 9000 + "

", "

" + "y" * 7000 + "

" + stub.script = _scoped( + concepts=[_submit({"concepts": [CONCEPT]})], + concept_0=[_submit({"concepts": [{"index": 0, "html": big}]})], + group_0=[_submit({"groups": [{"index": 0, "html": small}]})]) + assert cw_run._run_visuals(d, meta, cw_store.load_config(), None) + saved = json.loads((d / "visuals.json").read_text()) + assert saved["concepts"] == [CONCEPT] and saved["groups"][0]["html"] == small + clean, lines = render.load_visuals(d / "visuals.json", 1) + assert clean["concepts"] == [CONCEPT] and lines == ["visuals: 2 kept, 0 dropped"] + + def test_visuals_pass_skips_when_visuals_json_exists_unless_forced(): with cw_testlib.temp_home() as home, tempfile.TemporaryDirectory() as tmp: with cw_testlib.StubLLM({}) as stub: @@ -460,13 +545,14 @@ def test_visuals_groups_are_authored_in_parallel_calls_and_merged(): stub.script = _scoped(overview=[_submit({"overview": _overview()})], group_0=[_group(0)], group_1=[_group(1)]) assert cw_run._run_visuals(d, meta, cw_store.load_config(), None) - assert stub.peak_in_flight == 3 + assert stub.peak_in_flight == 4 saved = json.loads((d / "visuals.json").read_text()) assert set(saved) == {"overview", "pr", "groups"} and saved["overview"]["title"] == "Foo changes" assert saved["groups"] == [{"index": 0, "html": "

g0

"}, {"index": 1, "html": "

g1

"}] assert {p.name for p in (d / "runs").glob("visuals-*")} == { - "visuals-overview-1.json", "visuals-group-0-1.json", "visuals-group-1-1.json"} - assert cw_store.read_meta(d)["usage"]["prose"]["calls"] == 3 + "visuals-concepts-1.json", "visuals-overview-1.json", "visuals-group-0-1.json", + "visuals-group-1-1.json"} + assert cw_store.read_meta(d)["usage"]["prose"]["calls"] == 4 def test_a_failing_group_is_dropped_and_the_rest_still_written(): diff --git a/skills/code-walkthrough/scripts/test_prompts.py b/skills/code-walkthrough/scripts/test_prompts.py index e589a12..8fd32c2 100644 --- a/skills/code-walkthrough/scripts/test_prompts.py +++ b/skills/code-walkthrough/scripts/test_prompts.py @@ -85,7 +85,7 @@ def test_visuals_prompt_has_an_overview_example_and_no_pr_picture(): def test_visuals_prompt_stays_small_and_examples_are_phone_sized(): text = (PROMPTS_DIR / "visuals.md").read_text() - assert len(text.encode()) <= 11264 + assert len(text.encode()) <= 11776 for vb in re.findall(r"viewBox='0 0 (\d+) \d+'", text): assert 360 <= int(vb) <= 560 diff --git a/skills/code-walkthrough/scripts/test_render.py b/skills/code-walkthrough/scripts/test_render.py index 66c6e0c..8de2a5a 100644 --- a/skills/code-walkthrough/scripts/test_render.py +++ b/skills/code-walkthrough/scripts/test_render.py @@ -326,7 +326,7 @@ def test_live_ui_never_uses_html_injection_sinks(): assert "e.textContent=text" in mk assert "window.open(r.url,'_blank','noopener')" in wire_ask turn = _between(wire_ask, "function turnEl(", "function upsertOutcome(") - assert "mdEl(r.answer||'')" in turn and "mk('p','cw-turn-a',r.answer" not in turn + assert "mdEl(r.answer||'',r.terms)" in turn and "mk('p','cw-turn-a',r.answer" not in turn assert "pre-wrap" not in re.search(r"\.cw-turn-a\{[^}]*\}", template).group(0) assert "mdEl(n.body)" in template @@ -1710,3 +1710,40 @@ def test_resolved_thread_without_resolution_posts_to_resolve_thread_and_listens_ assert "'/resolve-thread'" in tpl and "JSON.stringify({thread_id:root.gh_thread_id})" in tpl assert "addEventListener('thread_resolution'" in tpl and "__cwOnThreadResolution" in tpl assert "b.status==='done'" in tpl and ".catch(()=>fail(null))" in tpl and "es.addEventListener('open'" in tpl and "__cwResetHow" in tpl and "r.status===409" in tpl and "r.status===404" in tpl + + +def test_concepts_render_first_with_text_fallback_and_legacy_pages_are_unchanged(): + c = {"title": "Live ", "before": "Constants", "after": "Read `live`", "why": "No deploy"} + out = pics({"pr": None, "groups": {}, "concepts": [{**c, "html": "

pic

"}, {**c, "title": "Two"}]}) + + assert out.index('id="cw-concepts"') < out.index("

Overview

") + assert out.count('
') == 2 + assert 'Live <config>' in out and "live" in out + assert out.count('class="cw-vis cw-concept-vis"') == 1 and "

pic

" in out + assert "
Before
Constants
" in out and "
Why
No deploy
" in out + assert "cw-concepts" not in pics({"pr": PR_PIC, "groups": {}}) + + +def test_load_visuals_counts_and_sanitises_concepts(tmp_path): + c = {"title": "T", "before": "b", "after": "a", "why": "w"} + raw = {"concepts": [{**c, "html": ""}], "groups": []} + clean, lines = load_visuals(_visuals_file(tmp_path, raw), 1) + + assert clean["concepts"] == [c] and lines[0] == "visuals: 1 kept, 0 dropped" + + +def test_concept_text_is_escaped_and_malformed_concepts_are_skipped(): + hostile = {"title": "", "before": "", "after": "a", "why": "w"} + out = pics({"pr": None, "groups": {}, "concepts": [hostile, {"title": "no rows"}, "junk", None]}) + + assert out.count('
') == 1 + assert "