From fb622fe0d2928967b621c33eae7beb210cbaec37 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Fri, 21 Aug 2026 08:58:39 -0400 Subject: [PATCH 01/11] Add jira-issue skill and compact style for issues and PR bodies Agent-authored LT tickets put good analysis in the wrong shape. The reference failure opens with a heading, runs past a thousand words, and states that it contains four separate user-visible problems, so a triager scanning a queue cannot act on it. Jira Data Center has no expand macro, so nothing in a description can be folded away. The skill keeps the description short and moves the depth into the first comment. The skill interviews for who, when, where and how under a cap of six questions, hunts duplicates before drafting rather than before posting, and gets a three-line lede approved before anything else is written. Unknowns are recorded rather than guessed, and nothing enters a description that the reporter did not say or that we did not verify. compact-style.md is a shared reference adapting the MIT-licensed i-have-adhd skill from chat turns to written artifacts. pr-pitch now points at it and gains a Start here line, numbered verification steps, a closing Next line, an optional status line for long-lived PRs, and the pre-send check. jira-bugfix gains the LT-XXXXX-short-slug branch convention, a relaxed worktree rule that asks rather than refuses, and a note that it is entered at Step 3 when jira-issue hands off. Evidence and screenshot handling is deliberately absent; it follows in a stacked branch. Co-Authored-By: Claude Opus 5 --- .claude/references/compact-style.md | 104 ++++++++ .claude/skills/jira-bugfix/SKILL.md | 32 ++- .claude/skills/jira-issue/SKILL.md | 223 ++++++++++++++++++ .../skills/jira-issue/references/bug-track.md | 91 +++++++ .../skills/jira-issue/references/examples.md | 96 ++++++++ .../jira-issue/references/feature-track.md | 79 +++++++ .../skills/jira-issue/references/format.md | 102 ++++++++ .claude/skills/pr-pitch/SKILL.md | 46 +++- .gitignore | 3 + .../2026-08-21-jira-issue-skill-design.md | 83 +++++++ 10 files changed, 852 insertions(+), 7 deletions(-) create mode 100644 .claude/references/compact-style.md create mode 100644 .claude/skills/jira-issue/SKILL.md create mode 100644 .claude/skills/jira-issue/references/bug-track.md create mode 100644 .claude/skills/jira-issue/references/examples.md create mode 100644 .claude/skills/jira-issue/references/feature-track.md create mode 100644 .claude/skills/jira-issue/references/format.md create mode 100644 Docs/workflows/2026-08-21-jira-issue-skill-design.md diff --git a/.claude/references/compact-style.md b/.claude/references/compact-style.md new file mode 100644 index 0000000000..902cb70855 --- /dev/null +++ b/.claude/references/compact-style.md @@ -0,0 +1,104 @@ +# Compact style for issues and PR bodies + +Shared reference. Pointed at by `.claude/skills/jira-issue/SKILL.md` and +`.claude/skills/pr-pitch/SKILL.md`. Adapted from the MIT-licensed +`i-have-adhd` skill (https://github.com/ayghri/i-have-adhd), which shapes chat +turns; these rules shape written artifacts instead. + +## What changes when the reader is not in the conversation + +A chat reader has just asked you something. A Jira reader is scanning a queue +of forty tickets, and a PR reader has eleven other reviews open. Both arrive +cold, months after the work, with none of your context and no way to ask a +follow-up. Four consequences: + +1. The first line is the only line guaranteed to be read. It carries the + decision, not the setup. +2. Nothing can be held in the reader's head from a previous paragraph. +3. There is no "let me know if you need anything else". Whatever is unanswered + stays unanswered. +4. Length is not a cost the reader chooses to pay. Jira Data Center has no + collapsible sections, so every word in a description is on the screen. + +## The rules + +### 1. Lead with what the reader must decide + +Not context, not history, not the investigation. For a triager: what breaks, +who it hits, how bad. For a reviewer: what the change does and what it risks. + +Bad: `h3. The underlying problem` / "While investigating LT-22710 we noticed" +Good: "Deleting a feature-based natural class empties every rule that used it." + +### 2. Number every multi-step sequence + +Repro steps, verification steps, migration steps. One bounded action per step. +**No step contains "and then" twice.** If a step needs a sub-list, it is two +steps. + +### 3. End with one named next action + +Every description ends with a single `*Next:*` line naming who does what. +Tickets and PRs that end without one stall, because nobody is named. + +Good: `*Next:* reporter to confirm the FLEx version (see comment).` +Good: `Next: approve, or tell me to split the installer change out.` + +### 4. One artifact, one problem + +A ticket describing four problems is four tickets, linked. A PR doing three +unrelated things is three PRs, stacked. Splitting is cheap at filing time and +expensive at review time. + +### 5. State cause, not concern + +No "seems to", no "there may be an issue with", no apology. Name the symptom +and, if known, the mechanism. If the mechanism is inferred rather than +observed, say "inferred" -- that is information, whereas hedging is noise. + +### 6. Cost shape, not hours + +Hours on a ticket read as a commitment nobody made. Size the work by what it +touches. + +Bad: "This will take some work." / "About two days." +Good: "One branch in one file." / "Needs a liblcm release and a package bump." + +### 7. Cap any list at five + +Past five, split into "must" and "nice to have", or accept that the artifact +is really an epic. Five ranked beats ten unranked. + +### 8. No preamble, no recap, no closer + +Banned openers: "This issue describes", "This PR refactors", "While +investigating", "As a note", "I have been looking into". Banned closers: +"Please let me know", "Hope this helps", "Happy to provide more detail". + +Start with the answer. Stop when the answer is done. + +### 9. Say what is not known + +An explicit `*Not known:*` line is worth more than a confident guess. It tells +the next reader where to dig and stops a fabrication becoming folklore. + +### 10. Never assert what was not verified + +Everything above the fold is either something a reporter said or something we +observed. Analysis, inference and reconstruction go in a comment or an +accordion, labelled as such. A screenshot from a headless test is not a +screenshot of the product; say which it is. + +## Pre-send check + +Delete before publishing: + +1. The first sentence, if it announces what the artifact is about to do. +2. The last sentence, if it recaps or asks for further questions. +3. Any "by the way" sidebar. It is a separate ticket. +4. Hedging adverbs carrying no information. Keep a hedge that carries real + uncertainty; deleting that one manufactures confidence. +5. Any idiom. Replace with the literal action. + +Then verify: **reading only the title and the last line, does the reader know +what is wrong and what happens next?** If yes, publish. diff --git a/.claude/skills/jira-bugfix/SKILL.md b/.claude/skills/jira-bugfix/SKILL.md index daa0de36d3..d43378101d 100644 --- a/.claude/skills/jira-bugfix/SKILL.md +++ b/.claude/skills/jira-bugfix/SKILL.md @@ -128,20 +128,37 @@ git branch --show-current > The current branch is ``. This doesn't match > LT-XXXXX. Options: - > - Create a new branch `LT-XXXXX` from `main` + > - Create a new branch `LT-XXXXX-` from `main` > - Continue on the current branch > - Switch to an existing branch (specify name) If creating a new branch: ```powershell git fetch origin - git checkout -b LT-XXXXX origin/main + git checkout -b LT-XXXXX- origin/main ``` -> **Important**: Do NOT create worktrees automatically. This -> repo uses worktrees but creating them involves workspace -> setup scripts. If a worktree is needed, tell the user to -> run the "Worktree: Create/Open from branch" VS Code task. +**Branch naming**: `LT-XXXXX-short-slug`, for example +`LT-22715-nc-delete-warning`. The number keeps this step's +"contains the LT number" check working and makes the branch +greppable; the slug is what makes a list of a dozen worktrees +readable. A bare `LT-XXXXX` tells you nothing at a glance. + +> **Worktrees**: do not create one without asking, and do not +> refuse to create one either. The repo has +> `scripts/Worktree-CreateFromBranch.ps1` and a set of +> `Worktree:` VS Code tasks. Ask which the user wants; if the +> `jira-issue` skill already recorded a `workspace` preference +> in `.claude/.jira-issue-prefs.json`, follow it and say so. +> Note that the script places worktrees under +> `../.worktrees/` while existing worktrees on disk are +> under `.tmp/worktrees/` -- match what is already there. + +> **Arriving from `jira-issue`**: if that skill just filed the +> ticket and the user chose "start now", Steps 0-2 are already +> done -- the issue is fetched, assigned, In Progress, and the +> branch and worktree exist and are named in a ticket comment. +> Begin at Step 3. ## Step 3: Reproduce the Bug (TDD) @@ -410,6 +427,9 @@ All other steps proceed automatically. ## Integration with Other Skills This skill composes with: +- `jira-issue` — files the ticket in the first place, and + hands off to this skill at Step 3 when the user chooses + "start now" - `atlassian-readonly-skills` — reading JIRA issues - `atlassian-skills` — writing to JIRA (assign, comment) - `session-workflow` — session management and handoff diff --git a/.claude/skills/jira-issue/SKILL.md b/.claude/skills/jira-issue/SKILL.md new file mode 100644 index 0000000000..f112907b61 --- /dev/null +++ b/.claude/skills/jira-issue/SKILL.md @@ -0,0 +1,223 @@ +--- +name: jira-issue +description: "Write and file a well-shaped LT Jira issue -- bug or feature. Use whenever asked to file, raise, report, or create a Jira issue or LT ticket, to turn a user report or a finding into a ticket, or to restructure a ticket that buries its point. Interviews for who/when/where/how, hunts duplicates before drafting, gets the three-line lede approved, keeps the description short because Jira Data Center cannot collapse anything, and moves the analysis to a comment." +argument-hint: "Optional: the raw report, a finding, or an existing LT-XXXXX to restructure" +user-invocable: true +--- + +# Jira Issue + +Read `.claude/references/compact-style.md` first. Every rule below assumes it. + +Related skills: `atlassian-readonly-skills` (search, read), `atlassian-skills` +(create, comment, link, transition), `jira-bugfix` (takes over at Phase 8). + +## The constraint that shapes everything + +Jira Data Center has no `{expand}` macro. Nothing in a description can be +folded away, so length in the description is length on the screen. The +description stays short; the depth goes in the first comment. + +## Preferences + +Read `.claude/.jira-issue-prefs.json` (gitignored). On first run, ask the two +questions once and write it: + +```json +{ + "jiraUsername": "", + "workspace": "worktree | branch", + "branchStyle": "LT-XXXXX-slug" +} +``` + +Thereafter do not ask. State which preference was used when acting on it, so a +wrong one is visible and correctable. + +## Phase 0 -- Type, and the one-problem test + +Bug or feature. Then count the problems in the raw material. + +**More than one problem means more than one ticket.** LT-22715 announces that +it contains "four separate user-visible problems"; that is four tickets and a +set of links, and splitting is far cheaper now than after triage. Propose the +split, name each ticket in one line, and get agreement before drafting. + +## Phase 1 -- Interview + +One question at a time. **Hard cap of six.** Quote the answers; do not +paraphrase them into confidence. + +| | Bug | Feature | +| --- | --- | --- | +| Who | Which user, what role, how many affected | Who wants it, and what they are actually trying to accomplish | +| When | FLEx version and build, date, first time or recurring, did it work before | How often it comes up | +| Where | Tool, window, menu path, which project | Which area of FLEx | +| How | Exact actions -- typed, pasted, dragged, IME, Send/Receive | What they do today instead | +| How bad | Data loss, workaround, blocking | What "done" looks like | + +Two rules outrank the questions: + +- **"I don't know" is an answer and it goes in the ticket** as a `*Not known:*` + line. A recorded gap tells the next reader where to dig; a guess becomes + folklore. +- **Nothing enters the description that the reporter did not say or that we + did not verify.** Inferred mechanism goes to the analysis comment, labelled + inferred. + +If the developer is relaying a second-hand report, ask which parts they +witnessed. Second-hand detail is recorded as second-hand. + +## Phase 2 -- Duplicates and related issues, before drafting + +Search first. If the ticket exists, the work is a comment on it, and finding +that out after drafting wastes the drafting. + +Four passes, ten results each: + +```powershell +python -c @' +import sys; sys.path.insert(0, ".claude/skills/atlassian-readonly-skills/scripts") +from jira_search import jira_search +jql = "project = LT AND text ~ \"natural class\" AND status != Closed ORDER BY updated DESC" +print(jira_search(jql, fields="key,summary,status,updated", limit=10)) +'@ +``` + +1. **Symptom words** -- the user's vocabulary, not ours. +2. **Area** -- `project = LT AND component = "..." AND status != Closed`. +3. **Link walk** -- for every ticket already cited, read its links and follow + one hop. +4. **Mechanism** -- the type or method name, when the code location is known. + +Present at most five candidates: + +| Key | Summary | Why it might be the same | Verdict | +| --- | --- | --- | --- | + +Verdicts are duplicate, related, or unrelated. **Never file without showing +this table and getting a yes.** If a duplicate exists, stop and offer to +comment on it instead; if the developer still wants a new ticket, file it and +link it as a duplicate. + +## Phase 3 -- The lede, approved before anything else is written + +Draft exactly three lines and stop: + +``` +*What happens:* Deleting a feature-based natural class empties every rule +formula that used it. No warning, no prompt. +*Who hits it:* Anyone whose phonological rules reference Natural Classes. +*How bad:* Silent data loss. Rules must be rebuilt by hand. +``` + +For a feature track the labels are `*What they want:*`, `*Who wants it:*`, +`*Why it matters:*`. + +Show it rendered. The developer approves, edits, or adds detail. Loop at most +three times; if it is still wrong, ask which line is wrong rather than guessing +a fourth. **Nothing else is drafted until these three lines are theirs.** + +## Phase 4 -- The rest of the ticket + +Follow `references/bug-track.md` or `references/feature-track.md`. Field +budgets and Jira markup are in `references/format.md`. Worked before-and-after +examples are in `references/examples.md`. + +Budgets, since nothing can be folded: summary 80 characters, lede 60 words, +whole description 250 words ending in one `*Next:*` line. Overflow goes to the +analysis comment. + +## Phase 5 -- Permission + +**Hard stop.** Before any attachment, sample project, log, or screenshot of +real data leaves the machine: + +> Do you have permission to post this? + +Language data is frequently unpublished and community-owned -- vernacular +text, speaker names, unreleased lexicons. Never attach a project or capture +the agent found on disk without being told to. If the answer is no or unclear, +describe the data instead of attaching it and record that in the ticket. + +## Phase 6 -- Publish + +Order matters: issue, then comment, then links. + +Write the description and the comment to files first; never inline multi-line +Jira markup into a command. + +```powershell +python -c @' +import sys; sys.path.insert(0, ".claude/skills/atlassian-skills/scripts") +from jira_issues import jira_create_issue +desc = open("desc.txt", encoding="utf-8").read() +print(jira_create_issue("LT", "", "Bug", description=desc)) +'@ +``` + +Then the analysis comment via `jira_add_comment(issue_key, comment)`, then one +link per related ticket. Read the real link type names rather than guessing: + +```powershell +python -c @' +import sys; sys.path.insert(0, ".claude/skills/atlassian-readonly-skills/scripts") +from jira_links import jira_get_link_types +print(jira_get_link_types()) +'@ +``` + +Then `jira_create_issue_link(link_type, inward_issue_key, outward_issue_key)`. + +SIL Jira is Data Center: `assignee` takes a username, not an account ID, +despite what the docstring says. If assignment fails, fall back to +`custom_fields={"assignee": {"name": ""}}`. + +## Phase 7 -- Report + +Three lines: the key, the URL, and one `Next:` line. Nothing else. + +## Phase 8 -- "Do you want to start this now?" + +Ask. Four exits: + +| Answer | Actions | +| --- | --- | +| Start now | Assign, transition to In Progress, comment the branch and worktree, create the workspace, hand to `jira-bugfix` at its Step 3 | +| Mine, later | Assign only. Status untouched, no branch | +| Leave for triage | Nothing. This is the default | +| Someone else's | Assign to the named person, comment, stop | + +On "start now": + +1. Branch `LT-XXXXX-short-slug` off fresh `origin/main`. +2. Workspace per the saved preference -- worktree or branch in place. Say + which preference was used. +3. Comment on the ticket, naming both: + + > Taken. Working on branch `LT-22715-nc-delete-warning`, worktree + > `.tmp/worktrees/nc-delete-warning`. + + This is what makes the ticket the index into a worktree list of sixteen. +4. Hand off to `jira-bugfix` at Step 3. Do not repeat its Steps 0-2; the issue + is already in hand, assigned and in progress. + +Resolve transitions by ID from `jira_get_transitions`, never by guessing a +name. If the transition is refused for permissions, say "assigned, please move +it to In Progress yourself" and continue rather than aborting. **Never +transition to Done or Resolved** -- that follows a merged PR. + +If Phase 2 found the work already exists, Phase 8 targets that existing key. + +## Before finishing + +- [ ] Summary is 80 characters or fewer and names the symptom, not the cause. +- [ ] The three lede lines are the developer's, not the first draft. +- [ ] Description is 250 words or fewer and ends in one `*Next:*` line. +- [ ] Every claim above the fold was said by the reporter or verified by us. +- [ ] Every unknown is a `*Not known:*` line rather than a guess. +- [ ] The duplicate table was shown and answered. +- [ ] Links created for every candidate marked duplicate or related. +- [ ] Nothing was attached without the permission question being answered. +- [ ] The reader knows what is wrong and what happens next from the title and + the last line alone. diff --git a/.claude/skills/jira-issue/references/bug-track.md b/.claude/skills/jira-issue/references/bug-track.md new file mode 100644 index 0000000000..bb66fe55d1 --- /dev/null +++ b/.claude/skills/jira-issue/references/bug-track.md @@ -0,0 +1,91 @@ +# Bug track + +What a FLEx bug report needs, from a real reporter's list: a brief description +of the problem, a sample project, and the steps to reproduce. Everything below +serves those three. + +## The interview + +Six questions maximum, one at a time. Stop early if the answers are already in +the raw report. + +**Who** -- which user, what role (linguist, consultant, translation advisor), +and how many people are affected. "One user" and "every project on the team" +are different tickets. + +**When** -- FLEx version *and* build number, the date it happened, whether it +is the first time or recurring, and whether it worked before. Version is +usually the decisive fact: a guard that shipped in 9.2.5 changes the whole +diagnosis depending on which side of it the reporter is on. + +**Where** -- the tool and window, the exact menu path, and which project. Ask +whether the project can be shared before assuming it can. + +**How** -- the exact actions. For text-entry bugs this is where the answer +hides: typed directly, pasted, dragged, typed with a vernacular keyboard or +IME, or arrived through Send/Receive. "Just typed it" and "pasted it" are +different bugs with different fixes. + +**How bad** -- data loss, a workaround, or blocked work. Silent data loss +outranks a visible error. Say whether Undo recovers it. + +## Steps to reproduce + +Numbered, one bounded action per step, no step containing "and then" twice. +Each step is something the reader can do without knowing the codebase. + +``` +# Open Grammar > Natural Classes. +# Create a natural class from phonological features and name it. +# Insert that class into a phonological rule formula. +# Return to Natural Classes and delete the class. +# Open the rule again. +``` + +Rules: + +- Start from a state the reader can reach: a new project, or a named sample. +- Never start at "with the corrupted project open". +- If a step needs specific data, say exactly what data. +- If reproduction is unreliable, say how many attempts out of how many. An + intermittent bug reported as reliable wastes the first hour of the fix. +- If nobody has reproduced it, say so in one line and put the inferred path in + the comment. A ticket that claims a reproduction it does not have is worse + than one that admits the gap. + +## Expected and Actual + +One line each, both observable. "It should work" is neither. + +## Environment + +FLEx version and build, Windows version, and anything unusual: Send/Receive in +use, a non-default keyboard or IME, a project migrated from an older version. + +## Sample project + +**Ask before attaching. Always.** + +> Do you have permission to post this? + +FLEx projects contain unpublished lexical data, vernacular text, and often +speaker names. The reporter may not own the data, and a Jira attachment is a +publication to everyone with project access. + +If the answer is no, or unclear: + +- Describe the shape of the data instead: how many entries, which writing + systems, which fields populated. +- Ask whether a minimal synthetic project reproduces it. +- Record in the ticket that a sample exists but was not attached, so nobody + re-asks. + +Never attach a project the agent found on disk. Never attach a screenshot of a +live project without the same question. + +## Priority + +Do not set a priority number. Give the triager the facts that determine one: +whether data is lost, whether Undo recovers it, whether a workaround exists, +and how many users are affected. Those four lines are worth more than a guess +at a field value, and the lede already carries them. diff --git a/.claude/skills/jira-issue/references/examples.md b/.claude/skills/jira-issue/references/examples.md new file mode 100644 index 0000000000..655a7fad7e --- /dev/null +++ b/.claude/skills/jira-issue/references/examples.md @@ -0,0 +1,96 @@ +# Worked example: LT-22715 + +A real ticket, filed before this skill existed. The analysis in it is good. +The shape is the failure. + +## Before + +Summary: `No distinction between user-created and auto-generated natural classes` + +Description opened with: + +``` +h3. The underlying problem + +FLEx has two kinds of feature-based natural class and no way to tell them apart. +``` + +and ran past a thousand words through `h3. Symptoms`, `h3. Why the LT-22576 +approach cannot be extended`, `h3. Proposed signal: presence of an +Abbreviation`, a five-column table, `h3. Open challenges`, and `h3. Three ways +to resolve them`. + +Four things went wrong: + +1. **The first rendered line is a heading**, so the first thing a triager reads + is the word "problem" and nothing else. +2. **It contains four problems.** The description says so outright: "that + single missing distinction produces four separate user-visible problems." + None of the four can be triaged, prioritised or closed on its own. +3. **Analysis sits above the fold.** The comparison with LT-22576, the + proposed signal, and the three resolution options are all real and useful, + and none of them is what a triager needs in ten seconds. +4. **The blocking question is buried.** Four open challenges needing a team + decision appear after roughly eight hundred words. + +## After + +Summary: `Natural Classes: generated classes are indistinguishable from real ones` + +``` +*What they want:* A rule should show the natural class the user picked, not a +stack of features, and the Natural Classes list should not fill with entries +nobody created. +*Who wants it:* Anyone building phonological rules from features. Surfaced by +LT-22576. +*Why it matters:* Editing a shared class from inside one rule silently changes +every other rule using it. + +h3. The cause + +FLEx has two kinds of feature-based natural class and nothing in the model +separates them: ones created deliberately in Grammar > Natural Classes, and +ones fabricated silently when features are inserted into a rule. Both are +{{PhNCFeatures}}. + +h3. Symptoms, filed separately + +# LT-AAAAA -- a shared class is rewritten from inside a rule, with nothing on +screen saying so +# LT-BBBBB -- a named class renders as a decomposed feature list +# LT-CCCCC -- generated classes accumulate in the user's list, never cleaned up +# LT-DDDDD -- no way to promote a feature bundle into a real class + +h3. Ideas to resolve + +# Treat a filled Abbreviation as "this is a real class" -- one branch in +{{RuleFormulaVcBase.Display}}, no model change +# Migrate, filling Abbreviation from Name -- needs a liblcm release and a +package bump, so no longer FieldWorks-only +# Stop naming generated classes at all -- no migration, but legacy data stays +ambiguous + +h3. Open question + +Existing real classes with no Abbreviation become indistinguishable from +generated ones. A team decision is needed before any of the three are built. + +_Trade-offs, the LT-22576 comparison and the migration detail are in the first +comment._ + +*Next:* team decision on the migration question. +``` + +228 words. The four symptom tickets are linked, each triageable on its own. + +## What moved to the comment + +Everything cut is still on the ticket, one scroll down: + +- Why the LT-22576 display-name heuristic cannot be extended +- The five-column rendering table +- Full detail on all three options +- All four open challenges, not just the blocking one +- The implementation note about the existing label-rendering path + +Nothing was lost. It stopped being the first thing a triager reads. diff --git a/.claude/skills/jira-issue/references/feature-track.md b/.claude/skills/jira-issue/references/feature-track.md new file mode 100644 index 0000000000..b865506947 --- /dev/null +++ b/.claude/skills/jira-issue/references/feature-track.md @@ -0,0 +1,79 @@ +# Feature track + +What a FLEx feature request needs: the user story -- what does the user want to +do -- what has been tried including workarounds, and ideas to resolve it. + +## The interview + +**Who wants it, and what are they actually trying to accomplish.** Users +request a widget when they have a goal. "Add a button that clears generated +natural classes" is a widget; "stop my rule list filling with entries I never +created" is the goal. File the goal, mention the widget as an idea. A ticket +filed as a widget forecloses every better solution. + +**How often it comes up.** Once, in one project, is a different ticket from +every workshop for three years. + +**What they do today.** The workaround is the single most useful thing in a +feature request. It shows the shape of the gap, proves the need is real, and +sometimes turns out to be good enough with one small change. + +**What "done" looks like.** In the user's terms, not ours. This is what the +reporter will check when a build ships. + +## Description skeleton + +``` +*What they want:* +*Who wants it:* +*Why it matters:* + +h3. What they do today + + +h3. Ideas to resolve +# -- +# -- + +h3. Open questions + + +*Not known:* + +_Detail in the first comment._ + +*Next:* +``` + +## Ideas to resolve + +Cap at five, ranked, one line each. Each idea carries a **cost shape**, not +hours: + +- "One branch in one file." +- "Needs a liblcm release and a package bump, so no longer a FieldWorks-only + change." +- "New model field, so a data migration." + +Cost shape is what lets a triager sequence the work. Hours are a commitment +nobody in the conversation is authorised to make. + +Detail belongs in the comment. The description gets the one-liners. + +## Open questions + +If the feature needs a decision that is not the implementer's to make -- what +happens to existing data, whether a field becomes mandatory, whether a +migration runs once or repeatedly -- say so, in one line each, under a heading +that says a team decision is needed. Burying a blocking question inside a +paragraph of analysis is how a ticket sits untouched for a year. + +## Scope + +**One ticket, one problem**, and features break this more often than bugs. A +request that reads "and while we are there we should also" is two tickets. + +When one underlying cause produces several user-visible problems, file the +problems separately and link them to one ticket describing the cause. Each +problem can then be triaged, prioritised and fixed on its own, which is the +whole point of separating them. diff --git a/.claude/skills/jira-issue/references/format.md b/.claude/skills/jira-issue/references/format.md new file mode 100644 index 0000000000..7bd3580e9c --- /dev/null +++ b/.claude/skills/jira-issue/references/format.md @@ -0,0 +1,102 @@ +# Field contract, budgets, and Jira markup + +## Budgets + +| Field | Budget | Why | +| --- | --- | --- | +| Summary | 80 characters | Truncates in queue views past roughly that | +| Lede | 60 words, three labelled lines | One glance, no scrolling | +| Description | 250 words total, ending in one `*Next:*` line | Nothing can be collapsed | +| Analysis comment | As long as the reasoning deserves | Nobody is forced to scroll past it | + +Count words before publishing. Over budget means the overflow was comment +material, not that the budget was wrong. + +## Summary + +Shape: `Area: what goes wrong`. + +- Names the **symptom**, not the cause. The cause is often wrong at filing + time; the symptom is what a duplicate search will match. +- No ticket references, no "Bug:", no trailing punctuation. +- Uses the reporter's vocabulary so the next person searching finds it. + +Good: `Natural Classes: deleting a feature class empties rules that use it` +Bad: `PhNCFeatures.DeletionTextTSS override suppresses the delete warning` +Bad: `Issue with natural classes` + +## Description skeleton + +``` +*What happens:* +*Who hits it:* +*How bad:* + +h3. Steps to reproduce +# +# + +h3. Expected + + +h3. Actual + + +h3. Environment +FLEx , . + +*Not known:* + +_Analysis in the first comment._ + +*Next:* +``` + +Feature track replaces Steps/Expected/Actual with the sections in +`feature-track.md`. Everything else is identical. + +## Jira Data Center wiki markup + +There is **no `{expand}`**. It is a Confluence macro. Do not write one, and do +not plan a description around content being hidden. + +| Need | Markup | +| --- | --- | +| Heading | `h3. Text` | +| Bold | `*text*` | +| Italic | `_text_` | +| Monospace | `{{text}}` | +| Numbered list | `# item` | +| Bullet list | `* item` | +| Code block | `{code:java}...{code}` or `{noformat}...{noformat}` | +| Quote block | `{quote}...{quote}` | +| Table | `||head||head||` then `|cell|cell|` | +| Link | `[text|https://example.com]` | +| Ticket reference | `LT-12345` -- linkifies automatically | +| Attached image | `!name.png!` or `!name.png\|thumbnail!` | + +Two traps: + +- `*` at the start of a line is a bullet, not bold. The lede labels work + because `*What happens:*` is followed by text on the same line. +- Underscores inside identifiers turn on italics. Wrap any identifier in + `{{...}}`. + +## What goes in the analysis comment + +Everything true and useful that a triager does not need in the first ten +seconds: + +- Root-cause analysis and the code path +- Evidence, probes, test output +- Inferred mechanism, labelled inferred +- Options considered and their trade-offs +- Cost shape for each option +- Anything second-hand, labelled second-hand + +Open the comment with one line saying what it is, so a reader scrolling past +knows whether to stop: + +``` +h3. Analysis (inferred unless marked verified) +``` diff --git a/.claude/skills/pr-pitch/SKILL.md b/.claude/skills/pr-pitch/SKILL.md index 961e2e66f2..579af0abac 100644 --- a/.claude/skills/pr-pitch/SKILL.md +++ b/.claude/skills/pr-pitch/SKILL.md @@ -14,6 +14,10 @@ Related skills this one calls: `fieldworks-code-commenting` (when doc text is inlined into source), `fieldworks-migration-scope-review` (when the PR is a large migration branch and the pitch must justify its scope). +Read `.claude/references/compact-style.md` before writing the pitch. It is the +shared style contract for issues and PR bodies, and it is where the banned +openers, the five-item list cap and the pre-send check live. + ## What this produces Two artifacts, always together, never one without the other: @@ -137,6 +141,16 @@ deleting the qualifiers that make a claim honest. The top zone, in this order. +### 0. Status, for a PR that has been open a while (one line, optional) + +A reviewer returning to a long-lived PR cannot hold its state between visits. +Give them one line at the top: what it is waiting on, and anything red. + +> Ready for review. CI green except the known-flaky interlinear test. + +Skip it on a PR opened today. It exists so nobody has to reconstruct the state +from a comment thread. + ### 1. Lead with what it does (2-3 sentences) Open with the concrete thing, not the framing. A screenshot or GIF if the @@ -152,7 +166,15 @@ not make them read to find it. ### 3. Where to look (at most five bullets, one line each) -The load-bearing section. The failure points a domain expert would +Open with one `**Start here:**` line naming the first file to read and why. +That is the *entry point*, not the riskiest thing -- a reviewer who is not told +where to begin reads the diff in alphabetical order, which is nobody's reading +order. + +> **Start here:** `ViewDefinitionOverrideApplier.cs` -- everything else is +> plumbing around what it does. + +Then the load-bearing part. The failure points a domain expert would anticipate, ordered by what would sink the PR -- not by what was hardest to build. One line each: the risk, and the thing that pins it -- the gate, the test, the invariant. @@ -176,6 +198,23 @@ tests, manual checks -- and what was *not* run. Anything currently red or known-broken goes here in plain words; a reviewer must never learn of a red job from the checks tab after reading a body that implied green. +When the reviewer needs to run something themselves, give it as numbered +steps, one bounded action each, copy-pasteable: + +> 1. `.\build.ps1 -CommentHygiene` +> 2. `.\test.ps1 -TestProject '' -TestFilter ''` +> 3. Open Grammar > Natural Classes and delete a feature-based class. + +Prose verification instructions get skipped. Numbered ones get run. + +### 6. What you want from the reviewer (one line) + +Close the top zone with a single line naming the next action and who takes it. +A pitch that ends on a verification paragraph leaves the reviewer guessing +whether they are approving, splitting, or blocking. + +> Next: approve, or tell me to split the installer change out. + Pitch rules: - No process narration. "We then discovered...", "after several @@ -314,6 +353,11 @@ Before finishing, confirm: - [ ] Every count was recounted. - [ ] The pitch does not repeat what an accordion already says. - [ ] Working notes are gitignored (`Docs/migration/working/`), not merged. +- [ ] The top zone opens with a `**Start here:**` line and closes with a + `Next:` line. +- [ ] The pre-send check in `.claude/references/compact-style.md` passes: + reading only the title and the last line, does the reviewer know what + this does and what is wanted from them? Do not mark this complete on unverified claims. If a claim could not be checked, say so in the report rather than asserting it. diff --git a/.gitignore b/.gitignore index a4c7c89ff4..6db7bd9207 100644 --- a/.gitignore +++ b/.gitignore @@ -223,3 +223,6 @@ DataTreeTimingBaselines.json Docs/migration/working/ Build/Agent/comment-hygiene-report.json .review/ + +# Per-developer preferences for the jira-issue skill +.claude/.jira-issue-prefs.json diff --git a/Docs/workflows/2026-08-21-jira-issue-skill-design.md b/Docs/workflows/2026-08-21-jira-issue-skill-design.md new file mode 100644 index 0000000000..881c8b417f --- /dev/null +++ b/Docs/workflows/2026-08-21-jira-issue-skill-design.md @@ -0,0 +1,83 @@ +# Design: the `jira-issue` skill + +Working document. When this becomes a PR, `pr-pitch` triages it as RESEARCH and +evicts it into the PR body. It is not meant to merge. + +## Problem + +Agent-authored LT tickets carry good analysis in the wrong shape. LT-22715 is +the reference failure: its first rendered line is `h3. The underlying problem`, +its description runs past a thousand words, and it announces that it contains +"four separate user-visible problems". A triager scanning a queue cannot use it. + +Jira Data Center has no `{expand}` macro, so nothing can be folded away. Length +in the description is length on the screen. That single constraint drives the +whole design: the description stays short and the depth moves to a comment. + +## Shape + +One skill, `.claude/skills/jira-issue/`, two tracks (bug and feature) sharing +one format contract, one style reference, one publish path. Nine phases. + +| Phase | Name | Gate | +| --- | --- | --- | +| 0 | Type and one-problem test | Split before drafting | +| 1 | Interview: who/when/where/how/how-bad | Cap of six questions | +| 2 | Duplicate and related hunt | Candidate table, developer says yes | +| 3 | Lede | Developer approves three lines | +| 4 | Track sections | - | +| 5 | Permission | Hard stop on user data | +| 6 | Publish | Issue, analysis comment, links | +| 7 | Report | Key, URL, one `Next:` line | +| 8 | Start now? | Four exits, hands off to `jira-bugfix` | + +## Decisions + +**Search before drafting, not before posting.** If the ticket already exists, +the work is a comment on it. Discovering that after twenty minutes of drafting +wastes the drafting. + +**The lede is approved before anything else is written.** Three labelled lines, +rendered and shown. Up to three revisions, then the skill asks which line is +wrong rather than guessing a fourth time. + +**"I don't know" is recorded, not resolved.** Missing facts go into the ticket +as `*Not known:*`. Invented ones are damage. Nothing enters the description +that the reporter did not say or that we did not verify; inferred mechanism +goes to the analysis comment, labelled as inferred. + +**Preferences are a file, not memory.** `.claude/.jira-issue-prefs.json`, +gitignored, written on first run. Portable to any agent this repo supports, +per-clone, and one developer's `worktree` never becomes another's default. + +**Branches are `LT-XXXXX-short-slug`.** Greppable by ticket and readable in a +worktree list of sixteen. `jira-bugfix`'s existing "branch name contains the LT +number" check keeps working. + +## Rejected + +**A second skill pair mirroring `pr-preflight` / `pr-pitch`.** Two entrypoints +to keep straight for a workflow that is mostly linear. The PR pair earns its +split because the write-up is re-run on existing PRs; a Jira description is +rewritten far less often. + +**Collapsible sections in the description.** `{expand}` is a Confluence macro. +Verified absent before relying on the comment split. + +**A shared `compact-writing` skill.** A fourth skill in the chain, loadable +when nobody asked for it. A reference file both skills point at costs less. + +## Sources + +`compact-style.md` adapts the MIT-licensed `i-have-adhd` skill +(https://github.com/ayghri/i-have-adhd) from chat turns to written artifacts. + +## Split + +PR 1, this branch: skill, style reference, prefs, `jira-bugfix` and `pr-pitch` +edits. Touches no image behaviour. + +PR 2, stacked: evidence framework, `jira_add_attachment`, `pr-pitch` evidence +section, `fieldworks-avalonia-ui` capture step. Its publish step probes for +`gh --attach`, falls back to `gh image`, then to an orphan evidence branch, so +it does not depend on the gh release landing. From 9a7ca75aeccc5bba7266187ca1811aed8d39f8fd Mon Sep 17 00:00:00 2001 From: John Lambert Date: Fri, 21 Aug 2026 09:12:37 -0400 Subject: [PATCH 02/11] Skip ticket sections that do not apply to the subject Not every LT ticket is about FLEx the product. One about developer tooling, an agent skill, the build or documentation has no FLEx version, no project file, no menu path and nothing to reproduce inside the application. Phase 0b now decides what kind of thing the ticket is about before the interview starts, and maps that to the environment questions worth asking. Asking which FLEx build was running, for a ticket about a Markdown reference file, spends one of six questions and signals that the ticket was generated rather than written. The output rule is the same: never emit a section that does not apply. A template dutifully filled with N/A costs the reader the same scan and returns nothing, so the heading goes instead. Co-Authored-By: Claude Opus 5 --- .claude/skills/jira-issue/SKILL.md | 26 ++++++++++++++++++- .../skills/jira-issue/references/format.md | 5 ++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/.claude/skills/jira-issue/SKILL.md b/.claude/skills/jira-issue/SKILL.md index f112907b61..f6b819f258 100644 --- a/.claude/skills/jira-issue/SKILL.md +++ b/.claude/skills/jira-issue/SKILL.md @@ -43,10 +43,32 @@ it contains "four separate user-visible problems"; that is four tickets and a set of links, and splitting is far cheaper now than after triage. Propose the split, name each ticket in one line, and get agreement before drafting. +## Phase 0b -- Relevance + +**Ask what kind of thing this ticket is about before asking anything else.** +Not every LT ticket is about FLEx the product. A ticket about developer +tooling, an agent skill, the build, CI, or documentation has no FLEx version, +no project file, no menu path, and no reproduction inside the application. + +| Subject | Environment questions that apply | +| --- | --- | +| FLEx product | Version and build, OS, project, menu path, keyboard/IME | +| Developer tooling, agent skills, docs | Repo, branch, which skill or script. No FLEx version | +| Build, CI, installer | Branch, runner, toolchain, which script. No project | + +**Never emit a section that does not apply.** A template dutifully filled with +"N/A" is worse than a short ticket: it costs the reader the same scan and +returns nothing. Drop the heading instead. + +The same test governs the interview. Asking a developer which FLEx build they +were running, for a ticket about a Markdown reference file, wastes one of the +six questions and signals that the ticket was generated rather than written. + ## Phase 1 -- Interview One question at a time. **Hard cap of six.** Quote the answers; do not -paraphrase them into confidence. +paraphrase them into confidence. Skip any row Phase 0b ruled irrelevant -- +the cap is a budget, and a wasted question is one you do not get back. | | Bug | Feature | | --- | --- | --- | @@ -216,6 +238,8 @@ If Phase 2 found the work already exists, Phase 8 targets that existing key. - [ ] Description is 250 words or fewer and ends in one `*Next:*` line. - [ ] Every claim above the fold was said by the reporter or verified by us. - [ ] Every unknown is a `*Not known:*` line rather than a guess. +- [ ] No section was emitted that does not apply -- no "N/A" environment, no + empty repro steps on a ticket with nothing to reproduce. - [ ] The duplicate table was shown and answered. - [ ] Links created for every candidate marked duplicate or related. - [ ] Nothing was attached without the permission question being answered. diff --git a/.claude/skills/jira-issue/references/format.md b/.claude/skills/jira-issue/references/format.md index 7bd3580e9c..0b2caea1a1 100644 --- a/.claude/skills/jira-issue/references/format.md +++ b/.claude/skills/jira-issue/references/format.md @@ -55,6 +55,11 @@ _Analysis in the first comment._ Feature track replaces Steps/Expected/Actual with the sections in `feature-track.md`. Everything else is identical. +This is a skeleton, not a form. Drop any heading that does not apply to the +subject -- a ticket about a build script has no `h3. Environment` worth +writing, and a ticket about a documentation file has nothing to reproduce. +See Phase 0b in `../SKILL.md`. + ## Jira Data Center wiki markup There is **no `{expand}`**. It is a Confluence macro. Do not write one, and do From 15eb19cc627a06d1d6c4891a16734f6eb25203d8 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Fri, 21 Aug 2026 10:35:49 -0400 Subject: [PATCH 03/11] Move the design record from the tree into the PR body The design document was a working artifact, not something a future maintainer needs in order to change this code correctly. Under the pr-pitch triage it is RESEARCH: a one-time investigation whose conclusions are now carried by the skill itself. Its content survives in the pull request body, where the decisions, the rejected alternatives and the reasoning stay recoverable without shipping scaffolding into the repository. Refs LT-22723 Co-Authored-By: Claude Opus 5 --- .../2026-08-21-jira-issue-skill-design.md | 83 ------------------- 1 file changed, 83 deletions(-) delete mode 100644 Docs/workflows/2026-08-21-jira-issue-skill-design.md diff --git a/Docs/workflows/2026-08-21-jira-issue-skill-design.md b/Docs/workflows/2026-08-21-jira-issue-skill-design.md deleted file mode 100644 index 881c8b417f..0000000000 --- a/Docs/workflows/2026-08-21-jira-issue-skill-design.md +++ /dev/null @@ -1,83 +0,0 @@ -# Design: the `jira-issue` skill - -Working document. When this becomes a PR, `pr-pitch` triages it as RESEARCH and -evicts it into the PR body. It is not meant to merge. - -## Problem - -Agent-authored LT tickets carry good analysis in the wrong shape. LT-22715 is -the reference failure: its first rendered line is `h3. The underlying problem`, -its description runs past a thousand words, and it announces that it contains -"four separate user-visible problems". A triager scanning a queue cannot use it. - -Jira Data Center has no `{expand}` macro, so nothing can be folded away. Length -in the description is length on the screen. That single constraint drives the -whole design: the description stays short and the depth moves to a comment. - -## Shape - -One skill, `.claude/skills/jira-issue/`, two tracks (bug and feature) sharing -one format contract, one style reference, one publish path. Nine phases. - -| Phase | Name | Gate | -| --- | --- | --- | -| 0 | Type and one-problem test | Split before drafting | -| 1 | Interview: who/when/where/how/how-bad | Cap of six questions | -| 2 | Duplicate and related hunt | Candidate table, developer says yes | -| 3 | Lede | Developer approves three lines | -| 4 | Track sections | - | -| 5 | Permission | Hard stop on user data | -| 6 | Publish | Issue, analysis comment, links | -| 7 | Report | Key, URL, one `Next:` line | -| 8 | Start now? | Four exits, hands off to `jira-bugfix` | - -## Decisions - -**Search before drafting, not before posting.** If the ticket already exists, -the work is a comment on it. Discovering that after twenty minutes of drafting -wastes the drafting. - -**The lede is approved before anything else is written.** Three labelled lines, -rendered and shown. Up to three revisions, then the skill asks which line is -wrong rather than guessing a fourth time. - -**"I don't know" is recorded, not resolved.** Missing facts go into the ticket -as `*Not known:*`. Invented ones are damage. Nothing enters the description -that the reporter did not say or that we did not verify; inferred mechanism -goes to the analysis comment, labelled as inferred. - -**Preferences are a file, not memory.** `.claude/.jira-issue-prefs.json`, -gitignored, written on first run. Portable to any agent this repo supports, -per-clone, and one developer's `worktree` never becomes another's default. - -**Branches are `LT-XXXXX-short-slug`.** Greppable by ticket and readable in a -worktree list of sixteen. `jira-bugfix`'s existing "branch name contains the LT -number" check keeps working. - -## Rejected - -**A second skill pair mirroring `pr-preflight` / `pr-pitch`.** Two entrypoints -to keep straight for a workflow that is mostly linear. The PR pair earns its -split because the write-up is re-run on existing PRs; a Jira description is -rewritten far less often. - -**Collapsible sections in the description.** `{expand}` is a Confluence macro. -Verified absent before relying on the comment split. - -**A shared `compact-writing` skill.** A fourth skill in the chain, loadable -when nobody asked for it. A reference file both skills point at costs less. - -## Sources - -`compact-style.md` adapts the MIT-licensed `i-have-adhd` skill -(https://github.com/ayghri/i-have-adhd) from chat turns to written artifacts. - -## Split - -PR 1, this branch: skill, style reference, prefs, `jira-bugfix` and `pr-pitch` -edits. Touches no image behaviour. - -PR 2, stacked: evidence framework, `jira_add_attachment`, `pr-pitch` evidence -section, `fieldworks-avalonia-ui` capture step. Its publish step probes for -`gh --attach`, falls back to `gh image`, then to an orphan evidence branch, so -it does not depend on the gh release landing. From 0f1f0d85aad9461c9ec5f1e1a05d3ffa5ec7f6f3 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Fri, 21 Aug 2026 10:39:28 -0400 Subject: [PATCH 04/11] Require Affects Version FW 9.3 on every new ticket Every new LT ticket carries Affects Version FW 9.3, with no exception. It is a filing convention that keeps the queue filterable rather than a claim about which build the reporter was running, so the Phase 0b relevance rule does not exempt a tooling, build or documentation ticket from it. A specific point release the reporter names is added alongside FW 9.3, never in place of it. Neither jira_create_issue nor jira_update_issue exposes the versions field, so it goes through custom_fields. The same applies to assignee, which those helpers send as a Cloud-style accountId that SIL's Data Center rejects; both fallbacks are now written down where the publish step needs them. Refs LT-22723 Co-Authored-By: Claude Opus 5 --- .claude/skills/jira-issue/SKILL.md | 30 +++++++++++++++++++ .../skills/jira-issue/references/format.md | 7 +++++ 2 files changed, 37 insertions(+) diff --git a/.claude/skills/jira-issue/SKILL.md b/.claude/skills/jira-issue/SKILL.md index f6b819f258..80d09c3202 100644 --- a/.claude/skills/jira-issue/SKILL.md +++ b/.claude/skills/jira-issue/SKILL.md @@ -60,6 +60,9 @@ no project file, no menu path, and no reproduction inside the application. "N/A" is worse than a short ticket: it costs the reader the same scan and returns nothing. Drop the heading instead. +One exception, and it is not a section: **Affects Version is always set**, even +on a ticket with no FLEx version in its body. See Phase 6. + The same test governs the interview. Asking a developer which FLEx build they were running, for a ticket about a Markdown reference file, wastes one of the six questions and signals that the ticket was generated rather than written. @@ -164,6 +167,32 @@ describe the data instead of attaching it and record that in the ticket. ## Phase 6 -- Publish +### Required on every new issue + +**Affects Version is `FW 9.3` on every new ticket, without exception.** It is +a filing convention that keeps the queue filterable, not a claim about which +build the reporter was running -- so Phase 0b does **not** exempt a tooling, +build or documentation ticket from it. If the reporter names a specific point +release, add that version as well; never in place of `FW 9.3`. + +The field is `versions` (Affects Version), not `fixVersions`. Neither +`jira_create_issue` nor `jira_update_issue` exposes it directly, so it goes +through `custom_fields`: + +```powershell +python -c @' +import sys; sys.path.insert(0, ".claude/skills/atlassian-skills/scripts") +from jira_issues import jira_update_issue +print(jira_update_issue("LT-XXXXX", custom_fields={"versions": [{"name": "FW 9.3"}]})) +'@ +``` + +`assignee` is the same story: `jira_create_issue` sends a Cloud-style +`accountId`, which SIL's Data Center rejects. Use +`custom_fields={"assignee": {"name": ""}}`. + +### Order + Order matters: issue, then comment, then links. Write the description and the comment to files first; never inline multi-line @@ -240,6 +269,7 @@ If Phase 2 found the work already exists, Phase 8 targets that existing key. - [ ] Every unknown is a `*Not known:*` line rather than a guess. - [ ] No section was emitted that does not apply -- no "N/A" environment, no empty repro steps on a ticket with nothing to reproduce. +- [ ] Affects Version is set to `FW 9.3`. Every new ticket, no exceptions. - [ ] The duplicate table was shown and answered. - [ ] Links created for every candidate marked duplicate or related. - [ ] Nothing was attached without the permission question being answered. diff --git a/.claude/skills/jira-issue/references/format.md b/.claude/skills/jira-issue/references/format.md index 0b2caea1a1..94ea1929a2 100644 --- a/.claude/skills/jira-issue/references/format.md +++ b/.claude/skills/jira-issue/references/format.md @@ -9,6 +9,13 @@ | Description | 250 words total, ending in one `*Next:*` line | Nothing can be collapsed | | Analysis comment | As long as the reasoning deserves | Nobody is forced to scroll past it | +Required fields, regardless of subject: + +| Field | Value | +| --- | --- | +| Affects Version (`versions`) | `FW 9.3` on every new ticket, plus any specific point release the reporter names | +| Assignee | Only when someone is taking it now. Data Center wants `{"name": ""}` | + Count words before publishing. Over budget means the overflow was comment material, not that the budget was wrong. From 34b639ed733c06db9ef7e01fa2567a71b2b3eab9 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Fri, 21 Aug 2026 10:57:55 -0400 Subject: [PATCH 05/11] Point the worked example at the tickets it actually produced The example carried placeholder keys for the four symptom tickets that splitting LT-22715 would create. Those tickets now exist, filed on 2026-08-21 as LT-22724 through LT-22727 and linked back with Issue split, so the example names them. A worked example that cites real tickets can be checked by a reader. One that cites LT-AAAAA cannot, and quietly invites the reader to treat the whole example as hypothetical. Refs LT-22715, LT-22723 Co-Authored-By: Claude Opus 5 --- .claude/skills/jira-issue/references/examples.md | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/.claude/skills/jira-issue/references/examples.md b/.claude/skills/jira-issue/references/examples.md index 655a7fad7e..863d7cf1b3 100644 --- a/.claude/skills/jira-issue/references/examples.md +++ b/.claude/skills/jira-issue/references/examples.md @@ -55,11 +55,10 @@ ones fabricated silently when features are inserted into a rule. Both are h3. Symptoms, filed separately -# LT-AAAAA -- a shared class is rewritten from inside a rule, with nothing on -screen saying so -# LT-BBBBB -- a named class renders as a decomposed feature list -# LT-CCCCC -- generated classes accumulate in the user's list, never cleaned up -# LT-DDDDD -- no way to promote a feature bundle into a real class +# LT-22724 -- editing a class from one rule changes every rule using it +# LT-22725 -- a named feature class renders as a feature list inside rules +# LT-22726 -- generated classes accumulate and are never cleaned up +# LT-22727 -- no way to promote a generated feature bundle into a real class h3. Ideas to resolve @@ -81,7 +80,9 @@ comment._ *Next:* team decision on the migration question. ``` -228 words. The four symptom tickets are linked, each triageable on its own. +271 words with the Environment line. The four symptom tickets were filed on +2026-08-21 as LT-22724 through LT-22727, linked to this one with "Issue split", +and each is triageable on its own. ## What moved to the comment From 03f686e4021fa7ae3bb8a9b635e1e61cd718dc25 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Fri, 21 Aug 2026 11:12:31 -0400 Subject: [PATCH 06/11] Halve the always-loaded skill and record the traps that bit us SKILL.md goes from 224 lines to 95. It is the only file loaded every time the skill triggers, so every line it holds is context spent whether or not the reader needs it. Mechanics move to references, which load when the phase that needs them is reached. Moved out: the four duplicate-search passes to a new duplicates.md, the publish calls to publish.md, the relevance table to format.md. What stays is the phase table, the two gates, the budgets and the traps. Recorded from the retroactive rewrite of 28 tickets: - Link types must be read, never guessed. There is no Relates in this Jira, and falling back to the first name in the list produced four bogus Cloners links between a cause ticket and its children. - resolution cannot be set by an update, only by a transition. - The read-only skill's jira_workflow.py and jira_projects.py raise NameError on import; use the atlassian-skills copies. - A private Gmail or Drive URL in a description is broken evidence. - Rewriting a ticket posts the original as a comment first. Task-type tickets gain their own lede labels. Two of the rewritten tickets were Tasks and had to improvise them. Refs LT-22723 Co-Authored-By: Claude Opus 5 --- .claude/skills/jira-issue/SKILL.md | 314 ++++-------------- .../jira-issue/references/duplicates.md | 36 ++ .../skills/jira-issue/references/format.md | 22 +- .../skills/jira-issue/references/publish.md | 98 ++++++ 4 files changed, 218 insertions(+), 252 deletions(-) create mode 100644 .claude/skills/jira-issue/references/duplicates.md create mode 100644 .claude/skills/jira-issue/references/publish.md diff --git a/.claude/skills/jira-issue/SKILL.md b/.claude/skills/jira-issue/SKILL.md index 80d09c3202..f06dc240fc 100644 --- a/.claude/skills/jira-issue/SKILL.md +++ b/.claude/skills/jira-issue/SKILL.md @@ -1,277 +1,95 @@ --- name: jira-issue -description: "Write and file a well-shaped LT Jira issue -- bug or feature. Use whenever asked to file, raise, report, or create a Jira issue or LT ticket, to turn a user report or a finding into a ticket, or to restructure a ticket that buries its point. Interviews for who/when/where/how, hunts duplicates before drafting, gets the three-line lede approved, keeps the description short because Jira Data Center cannot collapse anything, and moves the analysis to a comment." +description: "Write and file an LT Jira issue -- bug, feature or task -- that a triager can act on from the first line. Use whenever asked to file, raise, report or create a Jira issue or LT ticket, to turn a user report or a finding into a ticket, or to restructure a ticket that buries its point. Interviews, hunts duplicates before drafting, gets a three-line lede approved, and moves the analysis to a comment." argument-hint: "Optional: the raw report, a finding, or an existing LT-XXXXX to restructure" user-invocable: true --- # Jira Issue -Read `.claude/references/compact-style.md` first. Every rule below assumes it. +Jira Data Center has no `{expand}`. Nothing folds away, so length in the +description is length on the screen for every reader, permanently. Keep the +description short and put the depth in the first comment. -Related skills: `atlassian-readonly-skills` (search, read), `atlassian-skills` -(create, comment, link, transition), `jira-bugfix` (takes over at Phase 8). +Style contract: `.claude/references/compact-style.md`. Read it first. -## The constraint that shapes everything +## Phases -Jira Data Center has no `{expand}` macro. Nothing in a description can be -folded away, so length in the description is length on the screen. The -description stays short; the depth goes in the first comment. - -## Preferences - -Read `.claude/.jira-issue-prefs.json` (gitignored). On first run, ask the two -questions once and write it: - -```json -{ - "jiraUsername": "", - "workspace": "worktree | branch", - "branchStyle": "LT-XXXXX-slug" -} -``` - -Thereafter do not ask. State which preference was used when acting on it, so a -wrong one is visible and correctable. - -## Phase 0 -- Type, and the one-problem test - -Bug or feature. Then count the problems in the raw material. - -**More than one problem means more than one ticket.** LT-22715 announces that -it contains "four separate user-visible problems"; that is four tickets and a -set of links, and splitting is far cheaper now than after triage. Propose the -split, name each ticket in one line, and get agreement before drafting. - -## Phase 0b -- Relevance - -**Ask what kind of thing this ticket is about before asking anything else.** -Not every LT ticket is about FLEx the product. A ticket about developer -tooling, an agent skill, the build, CI, or documentation has no FLEx version, -no project file, no menu path, and no reproduction inside the application. - -| Subject | Environment questions that apply | -| --- | --- | -| FLEx product | Version and build, OS, project, menu path, keyboard/IME | -| Developer tooling, agent skills, docs | Repo, branch, which skill or script. No FLEx version | -| Build, CI, installer | Branch, runner, toolchain, which script. No project | - -**Never emit a section that does not apply.** A template dutifully filled with -"N/A" is worse than a short ticket: it costs the reader the same scan and -returns nothing. Drop the heading instead. - -One exception, and it is not a section: **Affects Version is always set**, even -on a ticket with no FLEx version in its body. See Phase 6. - -The same test governs the interview. Asking a developer which FLEx build they -were running, for a ticket about a Markdown reference file, wastes one of the -six questions and signals that the ticket was generated rather than written. - -## Phase 1 -- Interview - -One question at a time. **Hard cap of six.** Quote the answers; do not -paraphrase them into confidence. Skip any row Phase 0b ruled irrelevant -- -the cap is a budget, and a wasted question is one you do not get back. - -| | Bug | Feature | +| # | Phase | Rule | | --- | --- | --- | -| Who | Which user, what role, how many affected | Who wants it, and what they are actually trying to accomplish | -| When | FLEx version and build, date, first time or recurring, did it work before | How often it comes up | -| Where | Tool, window, menu path, which project | Which area of FLEx | -| How | Exact actions -- typed, pasted, dragged, IME, Send/Receive | What they do today instead | -| How bad | Data loss, workaround, blocking | What "done" looks like | - -Two rules outrank the questions: - -- **"I don't know" is an answer and it goes in the ticket** as a `*Not known:*` - line. A recorded gap tells the next reader where to dig; a guess becomes - folklore. -- **Nothing enters the description that the reporter did not say or that we - did not verify.** Inferred mechanism goes to the analysis comment, labelled - inferred. - -If the developer is relaying a second-hand report, ask which parts they -witnessed. Second-hand detail is recorded as second-hand. - -## Phase 2 -- Duplicates and related issues, before drafting - -Search first. If the ticket exists, the work is a comment on it, and finding -that out after drafting wastes the drafting. - -Four passes, ten results each: - -```powershell -python -c @' -import sys; sys.path.insert(0, ".claude/skills/atlassian-readonly-skills/scripts") -from jira_search import jira_search -jql = "project = LT AND text ~ \"natural class\" AND status != Closed ORDER BY updated DESC" -print(jira_search(jql, fields="key,summary,status,updated", limit=10)) -'@ -``` - -1. **Symptom words** -- the user's vocabulary, not ours. -2. **Area** -- `project = LT AND component = "..." AND status != Closed`. -3. **Link walk** -- for every ticket already cited, read its links and follow - one hop. -4. **Mechanism** -- the type or method name, when the code location is known. - -Present at most five candidates: - -| Key | Summary | Why it might be the same | Verdict | -| --- | --- | --- | --- | +| 0 | Type | Bug, feature or task. **More than one problem means more than one ticket** | +| 0b | Relevance | Not every ticket is about FLEx. Drop sections that do not apply; never fill them with "N/A" | +| 1 | Interview | Who / when / where / how / how bad. One at a time, **max 6**. "I don't know" is recorded, not guessed | +| 2 | Duplicates | Search **before** drafting. Show at most 5 candidates as a table with verdicts | +| 3 | Lede | Three labelled lines, **approved before anything else is written**. Max 3 revisions, then ask which line is wrong | +| 4 | Body | Track file, plus the budgets in `references/format.md` | +| 5 | Permission | "Do you have permission to post this?" Hard stop before anything leaves the machine | +| 6 | Publish | `references/publish.md` | +| 7 | Report | Key, URL, one `Next:` line. Nothing else | +| 8 | Start now? | Assign, transition, comment the branch and worktree, hand to `jira-bugfix` at its Step 3 | -Verdicts are duplicate, related, or unrelated. **Never file without showing -this table and getting a yes.** If a duplicate exists, stop and offer to -comment on it instead; if the developer still wants a new ticket, file it and -link it as a duplicate. +## The two gates -## Phase 3 -- The lede, approved before anything else is written +Nothing is filed until the developer has answered both: -Draft exactly three lines and stop: +1. the duplicate table (Phase 2) +2. the three lede lines (Phase 3) -``` -*What happens:* Deleting a feature-based natural class empties every rule -formula that used it. No warning, no prompt. -*Who hits it:* Anyone whose phonological rules reference Natural Classes. -*How bad:* Silent data loss. Rules must be rebuilt by hand. -``` +If either can be skipped quietly, the skill is decoration. -For a feature track the labels are `*What they want:*`, `*Who wants it:*`, -`*Why it matters:*`. +## Non-negotiable -Show it rendered. The developer approves, edits, or adds detail. Loop at most -three times; if it is still wrong, ask which line is wrong rather than guessing -a fourth. **Nothing else is drafted until these three lines are theirs.** +- **Affects Version `FW 9.3` on every new ticket.** A filing convention, not a + claim about a build, so Phase 0b does not exempt tooling or docs tickets. +- **Nothing above the fold that the reporter did not say or you did not + verify.** Inference goes in the comment, labelled inferred. +- Budgets: summary 80 characters, lede 60 words, description 250 words ending + in one `*Next:*` line. Overflow goes to the comment. -## Phase 4 -- The rest of the ticket +## Lede labels -Follow `references/bug-track.md` or `references/feature-track.md`. Field -budgets and Jira markup are in `references/format.md`. Worked before-and-after -examples are in `references/examples.md`. - -Budgets, since nothing can be folded: summary 80 characters, lede 60 words, -whole description 250 words ending in one `*Next:*` line. Overflow goes to the -analysis comment. - -## Phase 5 -- Permission - -**Hard stop.** Before any attachment, sample project, log, or screenshot of -real data leaves the machine: - -> Do you have permission to post this? - -Language data is frequently unpublished and community-owned -- vernacular -text, speaker names, unreleased lexicons. Never attach a project or capture -the agent found on disk without being told to. If the answer is no or unclear, -describe the data instead of attaching it and record that in the ticket. - -## Phase 6 -- Publish - -### Required on every new issue - -**Affects Version is `FW 9.3` on every new ticket, without exception.** It is -a filing convention that keeps the queue filterable, not a claim about which -build the reporter was running -- so Phase 0b does **not** exempt a tooling, -build or documentation ticket from it. If the reporter names a specific point -release, add that version as well; never in place of `FW 9.3`. - -The field is `versions` (Affects Version), not `fixVersions`. Neither -`jira_create_issue` nor `jira_update_issue` exposes it directly, so it goes -through `custom_fields`: - -```powershell -python -c @' -import sys; sys.path.insert(0, ".claude/skills/atlassian-skills/scripts") -from jira_issues import jira_update_issue -print(jira_update_issue("LT-XXXXX", custom_fields={"versions": [{"name": "FW 9.3"}]})) -'@ -``` - -`assignee` is the same story: `jira_create_issue` sends a Cloud-style -`accountId`, which SIL's Data Center rejects. Use -`custom_fields={"assignee": {"name": ""}}`. - -### Order - -Order matters: issue, then comment, then links. - -Write the description and the comment to files first; never inline multi-line -Jira markup into a command. - -```powershell -python -c @' -import sys; sys.path.insert(0, ".claude/skills/atlassian-skills/scripts") -from jira_issues import jira_create_issue -desc = open("desc.txt", encoding="utf-8").read() -print(jira_create_issue("LT", "", "Bug", description=desc)) -'@ -``` - -Then the analysis comment via `jira_add_comment(issue_key, comment)`, then one -link per related ticket. Read the real link type names rather than guessing: - -```powershell -python -c @' -import sys; sys.path.insert(0, ".claude/skills/atlassian-readonly-skills/scripts") -from jira_links import jira_get_link_types -print(jira_get_link_types()) -'@ -``` - -Then `jira_create_issue_link(link_type, inward_issue_key, outward_issue_key)`. - -SIL Jira is Data Center: `assignee` takes a username, not an account ID, -despite what the docstring says. If assignment fails, fall back to -`custom_fields={"assignee": {"name": ""}}`. - -## Phase 7 -- Report - -Three lines: the key, the URL, and one `Next:` line. Nothing else. - -## Phase 8 -- "Do you want to start this now?" - -Ask. Four exits: - -| Answer | Actions | +| Type | Labels | | --- | --- | -| Start now | Assign, transition to In Progress, comment the branch and worktree, create the workspace, hand to `jira-bugfix` at its Step 3 | -| Mine, later | Assign only. Status untouched, no branch | -| Leave for triage | Nothing. This is the default | -| Someone else's | Assign to the named person, comment, stop | - -On "start now": +| Bug | `*What happens:*` `*Who hits it:*` `*How bad:*` | +| Feature | `*What they want:*` `*Who wants it:*` `*Why it matters:*` | +| Task | `*What this is:*` `*Who it affects:*` `*Why it matters:*` | + +## Traps that have already bitten + +- **Read link types; never guess.** There is no `Relates` here. Falling back to + the first name in the list once produced four bogus `Cloners` links. +- **`resolution` cannot be set by an update** -- only by a transition. +- **The read-only skill's `jira_workflow.py` and `jira_projects.py` raise + `NameError: Optional` on import.** Use the `atlassian-skills` copies. +- **A private URL is broken evidence.** A Gmail or Drive link renders for + nobody. Attach the file. +- **Rewriting a ticket: post the original as a comment first**, then replace + the description. -1. Branch `LT-XXXXX-short-slug` off fresh `origin/main`. -2. Workspace per the saved preference -- worktree or branch in place. Say - which preference was used. -3. Comment on the ticket, naming both: - - > Taken. Working on branch `LT-22715-nc-delete-warning`, worktree - > `.tmp/worktrees/nc-delete-warning`. +## Preferences - This is what makes the ticket the index into a worktree list of sixteen. -4. Hand off to `jira-bugfix` at Step 3. Do not repeat its Steps 0-2; the issue - is already in hand, assigned and in progress. +`.claude/.jira-issue-prefs.json` (gitignored): `jiraUsername`, `workspace` +(`worktree` or `branch`), `branchStyle`. Ask the two questions once on first +run, then act on them silently -- but say which preference was used, so a +wrong one is visible. -Resolve transitions by ID from `jira_get_transitions`, never by guessing a -name. If the transition is refused for permissions, say "assigned, please move -it to In Progress yourself" and continue rather than aborting. **Never -transition to Done or Resolved** -- that follows a merged PR. +## References -If Phase 2 found the work already exists, Phase 8 targets that existing key. +| File | For | +| --- | --- | +| `references/bug-track.md` | Interview, repro rules, sample-project permission | +| `references/duplicates.md` | The four search passes and the candidate table | +| `references/feature-track.md` | User story, workarounds, cost shape | +| `references/format.md` | Budgets, required fields, Jira markup | +| `references/publish.md` | The API calls and their gotchas | +| `references/examples.md` | LT-22715 before and after | ## Before finishing -- [ ] Summary is 80 characters or fewer and names the symptom, not the cause. -- [ ] The three lede lines are the developer's, not the first draft. -- [ ] Description is 250 words or fewer and ends in one `*Next:*` line. -- [ ] Every claim above the fold was said by the reporter or verified by us. +- [ ] Both gates answered by the developer. +- [ ] Affects Version set. +- [ ] Description 250 words or fewer, ending in one `*Next:*` line. - [ ] Every unknown is a `*Not known:*` line rather than a guess. -- [ ] No section was emitted that does not apply -- no "N/A" environment, no - empty repro steps on a ticket with nothing to reproduce. -- [ ] Affects Version is set to `FW 9.3`. Every new ticket, no exceptions. -- [ ] The duplicate table was shown and answered. -- [ ] Links created for every candidate marked duplicate or related. -- [ ] Nothing was attached without the permission question being answered. -- [ ] The reader knows what is wrong and what happens next from the title and - the last line alone. +- [ ] No section emitted that does not apply. +- [ ] The title and the last line alone tell the reader what is wrong and what + happens next. diff --git a/.claude/skills/jira-issue/references/duplicates.md b/.claude/skills/jira-issue/references/duplicates.md new file mode 100644 index 0000000000..73ca30af0d --- /dev/null +++ b/.claude/skills/jira-issue/references/duplicates.md @@ -0,0 +1,36 @@ +# Duplicate and related search (Phase 2) + +**Search before drafting, not before posting.** If the ticket exists, the work +is a comment on it, and finding that out after twenty minutes wastes the +twenty minutes. + +```powershell +python -c @' +import sys; sys.path.insert(0, ".claude/skills/atlassian-skills/scripts") +from jira_search import jira_search +jql = "project = LT AND text ~ \"natural class\" AND status != Closed ORDER BY updated DESC" +print(jira_search(jql, fields="key,summary,status,updated", limit=10)) +'@ +``` + +Four passes, ten results each: + +1. **Symptom words** -- the reporter's vocabulary, not ours. +2. **Area** -- `project = LT AND component = "..." AND status != Closed`. +3. **Link walk** -- for every ticket already cited, read its links, follow one hop. +4. **Mechanism** -- the type or method name, when the code location is known. + +`text ~` searches summary, description, comments and environment, so a common +word like "triage" returns hundreds of irrelevant hits. Say so when reporting a +noisy pass rather than listing its results. + +Present at most five candidates: + +| Key | Summary | Why it might be the same | Verdict | +| --- | --- | --- | --- | + +Verdicts are duplicate, related or unrelated. **Never file without showing this +table and getting a yes.** If a duplicate exists, offer to comment on it +instead; if the developer still wants a new ticket, file it and link it. + +Link types are in `publish.md` -- read them, never guess. diff --git a/.claude/skills/jira-issue/references/format.md b/.claude/skills/jira-issue/references/format.md index 94ea1929a2..0d970bee07 100644 --- a/.claude/skills/jira-issue/references/format.md +++ b/.claude/skills/jira-issue/references/format.md @@ -62,10 +62,24 @@ _Analysis in the first comment._ Feature track replaces Steps/Expected/Actual with the sections in `feature-track.md`. Everything else is identical. -This is a skeleton, not a form. Drop any heading that does not apply to the -subject -- a ticket about a build script has no `h3. Environment` worth -writing, and a ticket about a documentation file has nothing to reproduce. -See Phase 0b in `../SKILL.md`. +This is a skeleton, not a form. Drop any heading that does not apply. + +## Relevance (Phase 0b) + +Not every LT ticket is about FLEx the product. Decide the subject before the +interview, because it decides which questions are worth one of your six. + +| Subject | Environment questions that apply | +| --- | --- | +| FLEx product | Version and build, OS, project, menu path, keyboard/IME | +| Developer tooling, agent skills, docs | Repo, branch, which skill or script. No FLEx version | +| Build, CI, installer | Branch, runner, toolchain, which script. No project | + +A template dutifully filled with "N/A" is worse than a short ticket: same scan, +no information. Drop the heading instead. + +One exception, and it is a field rather than a section: **Affects Version is +always set**, whatever the subject. ## Jira Data Center wiki markup diff --git a/.claude/skills/jira-issue/references/publish.md b/.claude/skills/jira-issue/references/publish.md new file mode 100644 index 0000000000..0200812320 --- /dev/null +++ b/.claude/skills/jira-issue/references/publish.md @@ -0,0 +1,98 @@ +# Publishing: the calls, and what bites + +Order: issue, comment, attachments, links. Write description and comment to +files first; never inline multi-line Jira markup into a command. + +## Create + +```powershell +python -c @' +import sys; sys.path.insert(0, ".claude/skills/atlassian-skills/scripts") +from jira_issues import jira_create_issue +desc = open("desc.txt", encoding="utf-8").read() +print(jira_create_issue("LT", "", "Bug", description=desc, + custom_fields={"versions": [{"name": "FW 9.3"}]})) +'@ +``` + +Then `jira_add_comment(key, comment)`, then `jira_add_attachment(key, paths)` +once Phase 5 is answered, then one link per related ticket. + +## Fields that need the custom_fields back door + +`jira_create_issue` and `jira_update_issue` do not expose these, and their +docstrings describe Jira Cloud rather than SIL's Data Center. + +| Field | Pass | +| --- | --- | +| Affects Version | `custom_fields={"versions": [{"name": "FW 9.3"}]}` | +| Assignee | `custom_fields={"assignee": {"name": "John_Lambert"}}` -- a username, **not** an accountId | +| Resolution | **Not settable by update at all.** It is not on the edit screen; only a transition sets it | + +Usernames are not email addresses. Read the caller's own with +`client.get(client.api_path("myself"))["name"]`. + +## Link types -- read them, never guess + +```powershell +python -c @' +import sys; sys.path.insert(0, ".claude/skills/atlassian-skills/scripts") +from jira_links import jira_get_link_types +print(jira_get_link_types()) +'@ +``` + +**There is no `Relates` in this Jira.** As of 2026-08-21 the types are +`Cloners, Depends on, Duplicate, Issue split, partially implements, Redesign, +Related, Requires, Solution, Story/Task, Test`. + +- Splitting one ticket into several -> **`Issue split`** +- Merely related -> **`Related`** +- Same defect -> **`Duplicate`** + +**Never fall back to the first name in the list.** Doing that once produced +four `Cloners` links between a cause ticket and its children, which reads as a +claim nobody made. If the intended type is absent, stop and ask. + +Then `jira_create_issue_link(link_type, inward_issue_key, outward_issue_key)`. + +## Broken scripts to route around + +`.claude/skills/atlassian-readonly-skills/scripts/jira_workflow.py` and +`jira_projects.py` both raise `NameError: name 'Optional' is not defined` on +import -- a missing typing import. The `atlassian-skills` copies work. Use +those, for reads as well as writes, until the read-only ones are fixed. + +## Rewriting an existing ticket + +**Post the original as a comment before replacing the description.** Someone +has already read that text and may have replied to it; replacing it outright +destroys the record. + +``` +h3. Full detail (original description, preserved ) + +The description above was shortened so a triager can act on the first line. +Nothing was deleted -- the original text follows verbatim. + +---- +``` + +Then update the description. Every edit notifies watchers, so a bulk pass is a +mail burst: do it in one sitting and tell the team it is coming. + +## Phase 8 -- starting work + +1. Branch `LT-XXXXX-short-slug` off fresh `origin/main`. +2. Workspace per the saved preference. Say which one was used. +3. Comment on the ticket naming both, so the ticket indexes the worktree list: + + > Taken. Working on branch `LT-22715-nc-delete-warning`, worktree + > `.tmp/worktrees/nc-delete-warning`. + +4. Hand to `jira-bugfix` at its Step 3. Its Steps 0-2 are already done. + +Resolve transitions by ID from `jira_get_transitions`, never by guessing a +name. A permissions failure degrades to "assigned, please move it to In +Progress yourself" rather than aborting. **Never transition to Done or +Resolved** -- that follows a merged PR. From de88efee4dce14f6e95fb7e83461feea6f5c8351 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Fri, 21 Aug 2026 09:03:21 -0400 Subject: [PATCH 07/11] Add evidence framework for screenshots in PRs and Jira issues A visible change should ship with a picture, and the repo already has the hard half: headless Skia capture for Avalonia, MCP capture for WinForms, and an Output/ManualEvidence naming convention. What was missing is the last mile -- trimming, captioning, provenance labelling, and any route at all from a PNG to a PR body or a ticket. The rule evidence.md turns on is that the test is the evidence and the screenshot is the courtesy. A control-level headless capture is not a screenshot of the product, and every image now says which it is. Publishing probes three routes and reports the one it used: native gh attach once it ships, gh image where a session cookie is reachable, and an orphan evidence branch with sha-pinned raw URLs otherwise. Only the third works under an Actions token, which is why it stays. jira_add_attachment uploads to Jira. It drives client.session directly because AtlassianClient.post sends JSON only, while multipart needs the XSRF header and no Content-Type. Its validation paths are exercised; the upload path has not yet been run against a live issue. A screenshot of a live project is a data disclosure exactly as a project file is, so the permission gate covers both. Co-Authored-By: Claude Opus 5 --- .claude/references/evidence.md | 140 ++++++++++++++++++ .claude/skills/atlassian-skills/SKILL.md | 9 ++ .../scripts/jira_attachments.py | 139 +++++++++++++++++ .../skills/fieldworks-avalonia-ui/SKILL.md | 23 +++ .claude/skills/jira-issue/SKILL.md | 4 +- .../skills/jira-issue/references/publish.md | 19 ++- .claude/skills/pr-pitch/SKILL.md | 12 ++ 7 files changed, 343 insertions(+), 3 deletions(-) create mode 100644 .claude/references/evidence.md create mode 100644 .claude/skills/atlassian-skills/scripts/jira_attachments.py diff --git a/.claude/references/evidence.md b/.claude/references/evidence.md new file mode 100644 index 0000000000..65a17db77f --- /dev/null +++ b/.claude/references/evidence.md @@ -0,0 +1,140 @@ +# Screenshots and visual evidence + +Shared reference. Pointed at by `.claude/skills/pr-pitch/SKILL.md`, +`.claude/skills/jira-issue/SKILL.md` and +`.claude/skills/fieldworks-avalonia-ui/SKILL.md`. + +Three stages, and skipping the middle one is the usual failure: +**capture, curate, publish.** + +## The rule that outranks the rest + +**The test is the evidence. The screenshot is the courtesy.** + +A headless capture proves nothing a reviewer can re-run. The assertion does. +So a PR carries the test name *and* the picture, and the picture never carries +a claim the test does not. + +The corollary: **a control-level headless capture is not a screenshot of the +product.** If nothing writes the operation at runtime yet, the image shows a +renderer, not a feature. Say which it is, in the caption, every time. That +distinction evaporates the moment an image is pasted without one. + +## Capture + +| Surface | Who captures | How | +| --- | --- | --- | +| Avalonia | The agent, automated | Headless Skia. See `fieldworks-avalonia-ui/references/visual-snapshot-testing.md` | +| WinForms, live project | The developer | Real scenarios need real data, and real data needs permission | +| WinForms, throwaway project | Either | `fieldworks-winapp/navigation/screenshot-evidence.md`, MCP-driven | + +Captures land in `Output/ManualEvidence//NN-name.png`. That directory +is gitignored, which is correct -- captures are working output, not artifacts. + +## Curate + +An uncaptured curation step is why evidence reads as decoration. Three things: + +**Trim to content.** Renderer captures are mostly background. A 520x180 capture +with content in the top quarter reads as an empty box at thumbnail size, which +is the size it is first seen at in both GitHub and Jira. Crop to the content +bounds plus about 8 pixels. + +**Caption every image.** What to look at, not what it is. "Before" is not a +caption; "Before -- the Lexeme field lists both seh and pt" is. + +**Label provenance in the caption.** One of: headless control-level capture, +live FLEx desktop, or mockup. Never leave it to be inferred. + +Name files `NN-state-subject.png` so they sort into reading order: +`01-before-writing-systems.png`, `02-after-writing-systems.png`. + +## Publish -- GitHub + +Probe for a route in this order and say which one was used. + +**1. `gh --attach`, once it ships.** Native upload on six commands (issue and +PR create, edit, comment), tracked by `github/roadmap#1324`. It uses the +ordinary `gh` token, so no cookie and no committed file. Constraints: write +access required, **Actions tokens excluded** so CI cannot use it, nine file +types, images under 10 MB. Detect it rather than assuming a version: + +```powershell +if ((gh pr comment --help 2>&1 | Out-String) -match '--attach') { "native upload available" } +``` + +**2. `gh image`** (`drogers0/gh-image`, MIT). Drives the web UI's own upload +flow and returns a real `user-attachments` URL. It needs a GitHub **session +cookie**, not the `gh` token: `--token`, `GH_SESSION_TOKEN`, or extraction +from a browser cookie store. Chrome 127 and later encrypt cookies in a way +that defeats extraction on Windows, so a Chrome-only machine will report +`session token is empty`. + +- Check availability with `gh image check-token`, which prints a username. +- **Never run `gh image extract-token` in an agent session.** It prints a + full-account credential to stdout, and stdout becomes conversation context. +- A `user_session` cookie grants complete account access and bypasses 2FA. If + a developer chooses this route, they set `GH_SESSION_TOKEN` in their own + shell before starting the session -- never pasted into a prompt. + +**3. Orphan `evidence` branch.** Always available, no credential, and the only +route in CI. The branch is created once, never merged, and never appears in a +diff: + +```powershell +git switch --orphan evidence +``` + +Reference the file by a **sha-pinned** raw URL so a merged PR's images can +never change under it: + +```markdown +![Before -- Lexeme lists seh and pt (headless capture)](https://raw.githubusercontent.com/sillsdev/FieldWorks//LT-22691d/01-before.png) +``` + +`sillsdev/FieldWorks` is public, so these render for everyone with no auth. +The branch must be protected: deleting it breaks every image in every PR that +ever referenced it. + +## Publish -- Jira + +Jira takes native attachments, which is better than a URL there because they +outlive any branch: + +```powershell +python -c @' +import sys; sys.path.insert(0, ".claude/skills/atlassian-skills/scripts") +from jira_attachments import jira_add_attachment +print(jira_add_attachment("LT-22715", ["01-before.png", "02-after.png"])) +'@ +``` + +Then reference them from the description or comment with `!01-before.png!`, +or `!01-before.png|thumbnail!` to keep a long description scannable. Images +belong in the analysis comment unless the picture *is* the bug report. + +## Permission + +**Hard stop, every time, before anything leaves the machine:** + +> Do you have permission to post this? + +A screenshot of a live project is a data disclosure exactly as a sample +project is: vernacular text, speaker names, unpublished lexical data, +community-owned material. Jira attachments are visible to everyone with +project access, and a GitHub attachment on a public repo is public. + +- Never publish a capture the agent found on disk without being told to. +- Agent-captured WinForms evidence comes from a throwaway test project only. +- If permission is unclear, describe the image instead and say in the ticket + that a capture exists but was not attached, so nobody re-asks. + +## Checklist + +- [ ] The claim the image supports is also pinned by a test, or the image is + labelled as the only evidence. +- [ ] Trimmed to content. +- [ ] Captioned with what to look at. +- [ ] Provenance named: headless, live, or mockup. +- [ ] Permission asked and answered before upload. +- [ ] The publish route used is stated, and any URL is sha-pinned. diff --git a/.claude/skills/atlassian-skills/SKILL.md b/.claude/skills/atlassian-skills/SKILL.md index ed05cb53b7..7f44b60592 100644 --- a/.claude/skills/atlassian-skills/SKILL.md +++ b/.claude/skills/atlassian-skills/SKILL.md @@ -281,6 +281,15 @@ from scripts.jira_issues import ( jira_add_comment # Add comment to issue ) +# Attachments live in their own module because they need a multipart POST +from scripts.jira_attachments import ( + jira_add_attachment # Upload one or more files to an issue +) + +# Attachments are visible to everyone who can see the issue. Confirm +# permission before uploading user data -- see .claude/references/evidence.md +jira_add_attachment("LT-22715", ["01-before.png", "02-after.png"]) + # Create issue with full options jira_create_issue( project_key="PROJ", diff --git a/.claude/skills/atlassian-skills/scripts/jira_attachments.py b/.claude/skills/atlassian-skills/scripts/jira_attachments.py new file mode 100644 index 0000000000..e51e8921dd --- /dev/null +++ b/.claude/skills/atlassian-skills/scripts/jira_attachments.py @@ -0,0 +1,139 @@ +"""Jira attachment tools. + +Tools: + - jira_add_attachment: Upload one or more files to an issue + +Attachments need a multipart POST, which AtlassianClient.post cannot do -- +it only sends JSON. This module therefore drives client.session directly, +reusing the client's base URL, auth, SSL setting and error handling. +""" + +import mimetypes +import os +import sys +from pathlib import Path +sys.path.insert(0, str(Path(__file__).parent)) + +from typing import Any, Dict, List, Optional, Union + +from _common import ( + AtlassianCredentials, + get_jira_client, + format_json_response, + format_error_response, + ConfigurationError, + AuthenticationError, + ValidationError, + NotFoundError, + APIError, + NetworkError, +) + +# Jira Data Center's default ceiling. A larger file fails server-side with a +# message that does not name the limit, so check it here where we can say so. +DEFAULT_MAX_BYTES = 10 * 1024 * 1024 + + +def jira_add_attachment( + issue_key: str, + file_paths: Union[str, List[str]], + credentials: Optional[AtlassianCredentials] = None, + max_bytes: int = DEFAULT_MAX_BYTES +) -> str: + """Attach one or more files to a Jira issue. + + Args: + issue_key: Issue key (e.g., 'LT-22715') + file_paths: A path, or a list of paths, to upload + credentials: Optional AtlassianCredentials for Agent environments. + If not provided, uses environment variables. + max_bytes: Reject any file larger than this before uploading + + Returns: + JSON string with one entry per attachment, each carrying id, + filename, size and the content URL, or error information + + Note: + Attachments are visible to everyone who can see the issue. Confirm + permission to publish before calling this with user data -- + FieldWorks projects, screenshots of live data, and logs frequently + contain unpublished language material. + """ + handles = [] + try: + client = get_jira_client(credentials) + + if not issue_key: + raise ValidationError('issue_key is required') + if not file_paths: + raise ValidationError('at least one file path is required') + + if isinstance(file_paths, str): + file_paths = [file_paths] + + for path in file_paths: + if not os.path.isfile(path): + raise ValidationError(f'file not found: {path}') + size = os.path.getsize(path) + if size == 0: + raise ValidationError(f'file is empty: {path}') + if size > max_bytes: + raise ValidationError( + f'file is {size} bytes, over the {max_bytes} byte limit: {path}' + ) + + files = [] + for path in file_paths: + name = os.path.basename(path) + mime = mimetypes.guess_type(name)[0] or 'application/octet-stream' + handle = open(path, 'rb') + handles.append(handle) + files.append(('file', (name, handle, mime))) + + url = f"{client.config.url}{client.api_path(f'issue/{issue_key}/attachments')}" + + # X-Atlassian-Token defeats Jira's XSRF check, which otherwise rejects + # the upload. Content-Type must be cleared so requests can set the + # multipart boundary; the session sets application/json for every + # other call, and a None value here removes it for this one. + response = client.session.post( + url, + files=files, + headers={'X-Atlassian-Token': 'no-check', 'Content-Type': None}, + timeout=120, + verify=client.ssl_verify + ) + client._handle_error(response) + + uploaded: List[Dict[str, Any]] = [] + for item in (response.json() if response.content else []): + uploaded.append({ + 'id': item.get('id', ''), + 'filename': item.get('filename', ''), + 'size': item.get('size', 0), + 'mimeType': item.get('mimeType', ''), + 'content': item.get('content', ''), + 'thumbnail': item.get('thumbnail', '') + }) + + return format_json_response({ + 'issue_key': issue_key, + 'count': len(uploaded), + 'attachments': uploaded + }) + + except ConfigurationError as e: + return format_error_response('ConfigurationError', str(e)) + except AuthenticationError as e: + return format_error_response('AuthenticationError', str(e)) + except ValidationError as e: + return format_error_response('ValidationError', str(e)) + except NotFoundError as e: + return format_error_response('NotFoundError', str(e)) + except (APIError, NetworkError) as e: + return format_error_response(type(e).__name__, str(e)) + except Exception as e: + return format_error_response('UnexpectedError', f'Unexpected error: {str(e)}') + finally: + for handle in handles: + handle.close() diff --git a/.claude/skills/fieldworks-avalonia-ui/SKILL.md b/.claude/skills/fieldworks-avalonia-ui/SKILL.md index 09c5b64e6b..be0bbc9a3a 100644 --- a/.claude/skills/fieldworks-avalonia-ui/SKILL.md +++ b/.claude/skills/fieldworks-avalonia-ui/SKILL.md @@ -173,6 +173,29 @@ Rules specific to dialogs: `../fieldworks-winforms-to-avalonia-migration/references/parity-evidence.md` §"Evidence language"). +## Evidence for a PR or a ticket + +Avalonia is the surface where capture is automated, so a visible change ships +with a picture. Do not leave it to the reviewer to imagine the before and +after. + +1. Capture both states from a permanent headless test, not a throwaway + fixture, so the evidence regenerates. `references/visual-snapshot-testing.md` + has the harness. +2. Assert the behaviour deterministically in that same test. **The test is the + evidence; the screenshot is the courtesy.** Keep PNGs as subjective + evidence rather than pixel-golden tests. +3. Trim, caption and label before publishing, then upload by the routes in + `.claude/references/evidence.md`. + +Label every capture **control-level headless**, never "screenshot of FLEx", +unless the product actually drives the code path. When an operation exists but +nothing writes it at runtime yet, say so beside the image; a reader who +assumes otherwise believes a feature has shipped. + +Captures belong in `Output/ManualEvidence//`, which is gitignored. +Do not commit files from `Output`. + ## Handoff Report Avalonia docs consulted, tests run, remaining prototype gaps, diff --git a/.claude/skills/jira-issue/SKILL.md b/.claude/skills/jira-issue/SKILL.md index f06dc240fc..e1b1b0604a 100644 --- a/.claude/skills/jira-issue/SKILL.md +++ b/.claude/skills/jira-issue/SKILL.md @@ -12,6 +12,7 @@ description is length on the screen for every reader, permanently. Keep the description short and put the depth in the first comment. Style contract: `.claude/references/compact-style.md`. Read it first. +Screenshots: `.claude/references/evidence.md`, before publishing any image. ## Phases @@ -23,7 +24,7 @@ Style contract: `.claude/references/compact-style.md`. Read it first. | 2 | Duplicates | Search **before** drafting. Show at most 5 candidates as a table with verdicts | | 3 | Lede | Three labelled lines, **approved before anything else is written**. Max 3 revisions, then ask which line is wrong | | 4 | Body | Track file, plus the budgets in `references/format.md` | -| 5 | Permission | "Do you have permission to post this?" Hard stop before anything leaves the machine | +| 5 | Permission | "Do you have permission to post this?" Hard stop. A screenshot of a live project counts | | 6 | Publish | `references/publish.md` | | 7 | Report | Key, URL, one `Next:` line. Nothing else | | 8 | Start now? | Assign, transition, comment the branch and worktree, hand to `jira-bugfix` at its Step 3 | @@ -91,5 +92,6 @@ wrong one is visible. - [ ] Description 250 words or fewer, ending in one `*Next:*` line. - [ ] Every unknown is a `*Not known:*` line rather than a guess. - [ ] No section emitted that does not apply. +- [ ] Every image trimmed, captioned, and labelled headless / live / mockup. - [ ] The title and the last line alone tell the reader what is wrong and what happens next. diff --git a/.claude/skills/jira-issue/references/publish.md b/.claude/skills/jira-issue/references/publish.md index 0200812320..fe348ec5a7 100644 --- a/.claude/skills/jira-issue/references/publish.md +++ b/.claude/skills/jira-issue/references/publish.md @@ -15,8 +15,23 @@ print(jira_create_issue("LT", "", "Bug", description=desc, '@ ``` -Then `jira_add_comment(key, comment)`, then `jira_add_attachment(key, paths)` -once Phase 5 is answered, then one link per related ticket. +Then `jira_add_comment(key, comment)`, then attachments once Phase 5 is +answered, then one link per related ticket. + +## Attachments + +```powershell +python -c @' +import sys; sys.path.insert(0, ".claude/skills/atlassian-skills/scripts") +from jira_attachments import jira_add_attachment +print(jira_add_attachment("LT-XXXXX", ["01-before.png", "02-after.png"])) +'@ +``` + +Reference them with `!01-before.png!`, or `!01-before.png|thumbnail!` to keep a +long description scannable. Images belong in the analysis comment unless the +picture *is* the report. Trimming, captioning and provenance labelling are in +`.claude/references/evidence.md`; they are not optional. ## Fields that need the custom_fields back door diff --git a/.claude/skills/pr-pitch/SKILL.md b/.claude/skills/pr-pitch/SKILL.md index 579af0abac..c899110402 100644 --- a/.claude/skills/pr-pitch/SKILL.md +++ b/.claude/skills/pr-pitch/SKILL.md @@ -18,6 +18,10 @@ Read `.claude/references/compact-style.md` before writing the pitch. It is the shared style contract for issues and PR bodies, and it is where the banned openers, the five-item list cap and the pre-send check live. +Read `.claude/references/evidence.md` before publishing any screenshot. It +covers trimming, captioning, provenance labelling, the three upload routes, +and the permission question that precedes all of them. + ## What this produces Two artifacts, always together, never one without the other: @@ -157,6 +161,14 @@ Open with the concrete thing, not the framing. A screenshot or GIF if the change is visible; otherwise one sentence naming what a user or caller can now do that they could not before. Never open with "This PR refactors...". +**If the change is visible, a picture is expected, not optional.** For +Avalonia work the capture is automated and there is no excuse for its absence; +for WinForms work the author supplies it. Every image is trimmed, captioned +with what to look at, and labelled with its provenance -- a headless +control-level capture is not a screenshot of the product, and a body that +blurs the two misleads the reviewer about what has been proven. The image +supports the claim; the test pins it. See `.claude/references/evidence.md`. + ### 2. The unknown the reviewer starts with (one paragraph) State the question the reviewer will actually have on opening a diff this From 0b1a07649e1ce1be6117d9cdf71fa3c3b643cacc Mon Sep 17 00:00:00 2001 From: John Lambert Date: Fri, 21 Aug 2026 11:41:14 -0400 Subject: [PATCH 08/11] Fix a reference path that does not resolve from its own file evidence.md sits in .claude/references/ and pointed at fieldworks-avalonia-ui/references/visual-snapshot-testing.md as if it were a sibling. The file is under .claude/skills/, so the pointer resolved from nowhere. Made absolute from the repo root. Co-Authored-By: Claude Opus 5 --- .claude/references/evidence.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.claude/references/evidence.md b/.claude/references/evidence.md index 65a17db77f..99214d3590 100644 --- a/.claude/references/evidence.md +++ b/.claude/references/evidence.md @@ -24,7 +24,7 @@ distinction evaporates the moment an image is pasted without one. | Surface | Who captures | How | | --- | --- | --- | -| Avalonia | The agent, automated | Headless Skia. See `fieldworks-avalonia-ui/references/visual-snapshot-testing.md` | +| Avalonia | The agent, automated | Headless Skia. See `.claude/skills/fieldworks-avalonia-ui/references/visual-snapshot-testing.md` | | WinForms, live project | The developer | Real scenarios need real data, and real data needs permission | | WinForms, throwaway project | Either | `fieldworks-winapp/navigation/screenshot-evidence.md`, MCP-driven | From d9890cbcd64bec5ff6d967b529c0e6546da358f5 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Fri, 21 Aug 2026 11:47:24 -0400 Subject: [PATCH 09/11] Drop the orphan evidence branch from the publish routes The third upload route proposed creating a never-merged orphan branch to host screenshots and referencing them by sha-pinned raw URL. It was speculative, nothing had been built, and it solved a problem lasting one review by putting binaries in history permanently. Route three is now the honest one: when no upload path is available, say which was tried and hand the file to the author to drag in. The skill also states plainly that images are not committed to the repository and no side branch is created to host them. Refs LT-22723 --- .claude/references/evidence.md | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/.claude/references/evidence.md b/.claude/references/evidence.md index 99214d3590..fa7888a724 100644 --- a/.claude/references/evidence.md +++ b/.claude/references/evidence.md @@ -51,7 +51,7 @@ Name files `NN-state-subject.png` so they sort into reading order: ## Publish -- GitHub -Probe for a route in this order and say which one was used. +Try these in order and say which one was used. **1. `gh --attach`, once it ships.** Native upload on six commands (issue and PR create, edit, comment), tracked by `github/roadmap#1324`. It uses the @@ -77,24 +77,17 @@ that defeats extraction on Windows, so a Chrome-only machine will report a developer chooses this route, they set `GH_SESSION_TOKEN` in their own shell before starting the session -- never pasted into a prompt. -**3. Orphan `evidence` branch.** Always available, no credential, and the only -route in CI. The branch is created once, never merged, and never appears in a -diff: +**3. Ask the author to drag it in.** When neither route above is available -- +no `--attach` yet, no session cookie, or a CI run, where the official flag +excludes Actions tokens anyway -- say so and hand the file over. Name the exact +path to drop into the comment box, then splice the returned URL into the body. -```powershell -git switch --orphan evidence -``` - -Reference the file by a **sha-pinned** raw URL so a merged PR's images can -never change under it: - -```markdown -![Before -- Lexeme lists seh and pt (headless capture)](https://raw.githubusercontent.com/sillsdev/FieldWorks//LT-22691d/01-before.png) -``` +That is a real answer, not a failure. An agent that cannot upload should say +which route it tried and stop, rather than inventing somewhere to put the file. -`sillsdev/FieldWorks` is public, so these render for everyone with no auth. -The branch must be protected: deleting it breaks every image in every PR that -ever referenced it. +**Do not commit images to the repository** to work around this, and do not +create a side branch to host them. Both put binaries in history permanently to +solve a problem that lasts one review. ## Publish -- Jira @@ -137,4 +130,4 @@ project access, and a GitHub attachment on a public repo is public. - [ ] Captioned with what to look at. - [ ] Provenance named: headless, live, or mockup. - [ ] Permission asked and answered before upload. -- [ ] The publish route used is stated, and any URL is sha-pinned. +- [ ] The publish route used is stated, including when it was you. From 68e01e79d588919e8ba0719a80d922610d0b3e47 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Fri, 21 Aug 2026 11:20:51 -0400 Subject: [PATCH 10/11] Compress the three workflow skills that carry the most prose jira-bugfix, pr-preflight and pr-pitch drop from 1107 lines to 374, about two thirds. A SKILL.md is loaded whole every time its skill triggers, so every line it holds is context spent whether the reader needs it or not. Procedure that only one phase needs now lives in a reference that loads when that phase is reached. Moved out: the review-summary template, the accordion catalogue, and the publish-and-verify mechanics. Deduplicated rather than shortened in place. jira-bugfix carried its own copy of the JIRA API calls, the assignee and version workarounds, and the transition rules; it now points at the jira-issue skill's publish.md. It also delegates commit messages, PR creation and test coverage to the skills that own them instead of restating them. Nothing about the guidance changed. The pitch structure, the triage buckets, the four analysis passes, the interview discipline and every gate survive verbatim or tightened. Refs LT-22723 Co-Authored-By: Claude Opus 5 --- .claude/skills/jira-bugfix/SKILL.md | 485 +++--------------- .claude/skills/pr-pitch/SKILL.md | 411 ++++----------- .../skills/pr-pitch/references/accordions.md | 43 ++ .../skills/pr-pitch/references/publishing.md | 67 +++ .claude/skills/pr-preflight/SKILL.md | 315 +++--------- .../references/summary-template.md | 79 +++ 6 files changed, 428 insertions(+), 972 deletions(-) create mode 100644 .claude/skills/pr-pitch/references/accordions.md create mode 100644 .claude/skills/pr-pitch/references/publishing.md create mode 100644 .claude/skills/pr-preflight/references/summary-template.md diff --git a/.claude/skills/jira-bugfix/SKILL.md b/.claude/skills/jira-bugfix/SKILL.md index d43378101d..7ca016a80f 100644 --- a/.claude/skills/jira-bugfix/SKILL.md +++ b/.claude/skills/jira-bugfix/SKILL.md @@ -1,4 +1,3 @@ -```skill --- name: jira-bugfix description: > @@ -10,445 +9,105 @@ license: MIT compatibility: Requires atlassian-skills, atlassian-readonly-skills. metadata: author: FieldWorks team - version: "1.0" + version: "2.0" --- -# JIRA Bugfix Workflow +# JIRA Bugfix -End-to-end skill for fixing bugs sourced from SIL's JIRA -(LT-prefixed tickets). Orchestrates the full lifecycle from -issue triage through PR creation and JIRA update. +Fixing a bug sourced from an LT ticket, from triage to PR. -## When to Use +Tell the user the steps before starting, then work them in order. Pause at the +four decision points below; everything else proceeds. -Activate this skill when: -- The user says "fix LT-XXXXX" or "work on LT-XXXXX" -- The user references a JIRA bug they want resolved -- You are starting a bugfix session for a known defect +## Steps -## Announce the Plan +| # | Step | Notes | +| --- | --- | --- | +| 0 | Fetch the issue | Summary, status, components, comments. Comments often carry the repro | +| 1 | Assign, move to In Progress | Skip if already there | +| 2 | Branch | `LT-XXXXX-short-slug` off fresh `origin/main` | +| 3 | **Reproduce with a failing test** | The most important step. See below | +| 4 | Fix | The minimal change that makes the test pass | +| 5 | Widen coverage | Edge cases, other call sites of the changed code, backward compatibility | +| 6 | Devil's advocate | Challenge the fix and the tests before anyone else does | +| 7 | AGENTS.md | Update only if a contract or behaviour documented there changed | +| 8 | Commit | `commit-messages` skill. gitlint runs in CI | +| 9 | PR | `pr-preflight` -- the required entrypoint. Never hand-write a PR body | +| 10 | Comment on the ticket | Root cause, fix, PR link, tests added | -Before starting any work, **always tell the user** the steps -you will follow, in order: +**Arriving from `jira-issue`?** If that skill just filed the ticket and the +user chose "start now", steps 0-2 are already done -- the issue is fetched, +assigned, In Progress, and the branch and worktree are named in a ticket +comment. Begin at step 3. -> I will follow the JIRA bugfix workflow: -> -> 1. Fetch the issue details from JIRA -> 2. Ensure the issue is assigned to you and in progress -> 3. Verify or create a branch named after the ticket -> 4. Reproduce the bug with a failing test (TDD) -> 5. Implement the minimal fix to make the test pass -> 6. Assess whether additional test coverage is needed -> 7. Devil's advocate & code review -> 8. Check and update relevant AGENTS.md files -> 9. Commit, push, and create a PR -> 10. Update the JIRA ticket with a comment and PR link -> -> I will pause for your input at key decision points. +## Step 3 -- TDD, and the escape hatch -Then proceed through the steps below. +Write a failing test that captures the bug before writing any fix. Name it +`MethodName_Scenario_ExpectedBehavior`. Confirm it fails, then fix, then +confirm it passes. -## Step 0: Fetch Issue from JIRA - -Use the `atlassian-readonly-skills` scripts to get the issue: - -```powershell -python -c "import sys; sys.path.insert(0, '.claude/skills/atlassian-readonly-skills/scripts'); from jira_issues import jira_get_issue; print(jira_get_issue('LT-XXXXX'))" -``` - -Extract and present to the user: -- **Summary** and **description** -- **Status**, **priority**, **assignee** -- **Components** and **affected versions** -- **Comments** (recent ones may contain reproduction steps) - -If the issue is not a Bug type, note this and ask the user -whether to proceed with the bugfix workflow anyway. - -## Step 1: Assign and Transition to In Progress - -### Check current assignment - -If the issue is unassigned or assigned to someone else, ask -the user for their JIRA username and assign it: - -```powershell -# Assign the issue (Data Center uses 'name' field) -python -c " -import sys, json -sys.path.insert(0, '.claude/skills/atlassian-skills/scripts') -from jira_issues import jira_update_issue -print(jira_update_issue('LT-XXXXX', assignee='username')) -" -``` - -> **Note**: SIL JIRA is Data Center, so the assignee field -> uses `name` (username string), not `accountId`. If the -> `jira_update_issue` call fails with assignee, fall back to -> setting it via `custom_fields`: -> `custom_fields={'assignee': {'name': 'username'}}` - -### Transition to In Progress - -```powershell -# Get available transitions -python -c " -import sys -sys.path.insert(0, '.claude/skills/atlassian-readonly-skills/scripts') -from jira_workflow import jira_get_transitions -print(jira_get_transitions('LT-XXXXX')) -" - -# Transition (use the ID for "In Progress" from above) -python -c " -import sys -sys.path.insert(0, '.claude/skills/atlassian-skills/scripts') -from jira_workflow import jira_transition_issue -print(jira_transition_issue('LT-XXXXX', 'TRANSITION_ID')) -" -``` - -If the issue is already In Progress, skip this step. - -## Step 2: Branch Management - -### Check current branch - -```powershell -git branch --show-current -``` - -**Decision tree:** - -1. If the current branch name contains the LT number - (e.g., `LT-22427`), you are already on the right branch. - Proceed. - -2. If not, **ask the user**: - - > The current branch is ``. This doesn't match - > LT-XXXXX. Options: - > - Create a new branch `LT-XXXXX-` from `main` - > - Continue on the current branch - > - Switch to an existing branch (specify name) - - If creating a new branch: - ```powershell - git fetch origin - git checkout -b LT-XXXXX- origin/main - ``` - -**Branch naming**: `LT-XXXXX-short-slug`, for example -`LT-22715-nc-delete-warning`. The number keeps this step's -"contains the LT number" check working and makes the branch -greppable; the slug is what makes a list of a dozen worktrees -readable. A bare `LT-XXXXX` tells you nothing at a glance. - -> **Worktrees**: do not create one without asking, and do not -> refuse to create one either. The repo has -> `scripts/Worktree-CreateFromBranch.ps1` and a set of -> `Worktree:` VS Code tasks. Ask which the user wants; if the -> `jira-issue` skill already recorded a `workspace` preference -> in `.claude/.jira-issue-prefs.json`, follow it and say so. -> Note that the script places worktrees under -> `../.worktrees/` while existing worktrees on disk are -> under `.tmp/worktrees/` -- match what is already there. - -> **Arriving from `jira-issue`**: if that skill just filed the -> ticket and the user chose "start now", Steps 0-2 are already -> done -- the issue is fetched, assigned, In Progress, and the -> branch and worktree exist and are named in a ticket comment. -> Begin at Step 3. - -## Step 3: Reproduce the Bug (TDD) - -**This is the most important step.** Default to test-driven -development: write a failing test that captures the bug -before writing any fix. - -### Process - -1. **Analyze the bug**: From the JIRA description and code - exploration, understand the root cause. - -2. **Find the right test file**: Locate existing tests for - the affected component. Follow the conventions in - `.github/instructions/testing.instructions.md`. - -3. **Write a failing test** that demonstrates the bug: - - Name it descriptively: - `MethodName_Scenario_ExpectedBehavior` - - The test should FAIL with the current code - - The test should PASS after the fix - -4. **Run the test** to confirm it fails: - ```powershell - .\test.ps1 -TestFilter "Name~TestMethodName" - ``` - -5. **If a test is impossible**, explain to the user WHY: - - The bug is purely visual/UI and untestable in NUnit - - The bug requires external services not available in - test harness - - The bug is in build/packaging infrastructure - - Then ask the user: - > I cannot write an automated test for this bug because - > [reason]. Would you like me to proceed with the fix - > anyway, or would you prefer to explore alternative - > verification approaches? - - Wait for confirmation before proceeding. - -## Step 4: Implement the Fix - -1. Apply the **minimal change** needed to make the failing - test pass. -2. Follow repo conventions: - - `.github/instructions/managed.instructions.md` for C# - - `.github/instructions/native.instructions.md` for C++ - - `.github/instructions/testing.instructions.md` for tests -3. Run the previously-failing test to confirm it passes: - ```powershell - .\test.ps1 -TestFilter "Name~TestMethodName" - ``` - -## Step 5: Assess Additional Test Coverage - -After the fix passes, evaluate whether additional tests are -needed. Ask yourself: - -- **Are there related edge cases** the fix might affect? -- **Are there other code paths** that use the same logic? -- **Is the existing test coverage** for this component - adequate? -- **Could a devil's advocate** argue the fix is incomplete? - -If gaps exist, add tests. Common high-value additions: -- Backward compatibility tests (old behavior still works) -- Isolation tests (fix doesn't leak to unrelated paths) -- Boundary/edge-case tests -- Tests for other call sites of modified code - -Run the full component tests: ```powershell -.\test.ps1 -TestProject "path/to/TestProject" +.\test.ps1 -TestFilter "Name~TestMethodName" -CommentHygiene ``` -## Step 6: Devil's Advocate & Code Review - -Before finalizing, critically review your own work. Play -devil's advocate against the fix and the tests. - -### Challenge the fix - -Ask yourself and present findings to the user: - -- **Is this the best solution?** Are there simpler or more - robust alternatives? If multiple reasonable approaches - exist and the best choice is unclear, **present the - options to the user** with trade-offs and ask which they - prefer. -- **Does the fix introduce new risks?** Could it regress - other behavior, cause performance issues, or break - backward compatibility? -- **Is the scope right?** Is the fix too narrow (misses - related cases) or too broad (changes more than needed)? -- **Are there subtle edge cases** not covered by the tests? -- **Does the fix match the codebase style** and patterns? - Check naming, error handling, null safety, threading. - -### Clean code review - -Review the diff as if you were a code reviewer: - -- No dead code, commented-out code, or debug artifacts -- No unintended whitespace or formatting changes -- Method/variable names are clear and consistent -- Comments explain *why*, not *what* -- No accidental scope expansion beyond the bug fix -- Error paths are handled correctly - -### Decision point - -If the review surfaces **any uncertainties**, present them -to the user: - -> **Devil's advocate findings:** -> -> 1. [Finding and why it matters] -> 2. [Alternative approach and trade-offs] -> -> Would you like me to adjust the approach, or proceed -> as-is? +A filter that matches nothing still exits 0 and prints PASS. Check +`Total tests: N` is greater than zero. -Wait for the user's response before continuing. If no -uncertainties exist, briefly summarize why the fix is -solid and proceed. +If a test is genuinely impossible -- purely visual, needs an external service, +or lives in build/packaging -- say **which** of those it is, then ask whether +to proceed without one. Do not skip silently, and do not invent a test that +passes either way. -## Step 7: Update AGENTS.md +## Step 6 -- Devil's advocate -Check whether the fix changes any contracts, behaviors, or -architecture documented in AGENTS.md files: +Before finalising, argue against your own work: -1. Find relevant AGENTS.md files: - ```powershell - # Check for AGENTS.md in affected directories - Get-ChildItem -Path "Src/" -Filter "AGENTS.md" -Recurse - ``` +- Is there a simpler or more robust fix? If several are reasonable, present + them with trade-offs and let the user choose. +- Does it regress anything, or change performance or compatibility? +- Is the scope right -- not so narrow it misses sibling cases, not so broad it + becomes a refactor? +- Does the diff carry dead code, debug artefacts, or stray whitespace? -2. If behavior or contracts changed, update the AGENTS.md - to reflect the new state. +Surface any uncertainty to the user rather than resolving it silently. -3. If no AGENTS.md exists for the affected area and the - change is architecturally significant, note this but - do not create one unless the user requests it. +## Branching and worktrees -## Step 8: Commit and Push +`LT-XXXXX-short-slug`, for example `LT-22715-nc-delete-warning`. The number +keeps step 2's "contains the LT number" check working and makes the branch +greppable; the slug is what makes a list of a dozen worktrees readable. -### Pre-commit checks +Do not create a worktree without asking -- and do not refuse to create one +either. `scripts/Worktree-CreateFromBranch.ps1` and the `Worktree:` VS Code +tasks exist. If `.claude/.jira-issue-prefs.json` records a `workspace` +preference, follow it and say so. The script places worktrees under +`../.worktrees/` while the ones on disk are under `.tmp/worktrees/` -- +match what is already there. -```powershell -.\build.ps1 -.\test.ps1 -.\Build\Agent\check-and-fix-whitespace.ps1 -``` - -### Commit - -Follow `.github/instructions/commit-messages.instructions.md`: -- Subject: max 72 characters, imperative mood, no trailing - punctuation -- Body: wrap at 80 characters, explain what and why - -Pattern: -``` -Fix LT-XXXXX: +## JIRA mechanics - - - - -``` - -### Push - -```powershell -git add -A -git commit -m "" -git push -u origin LT-XXXXX -``` +All of it -- create, comment, attach, link, transition, and the `custom_fields` +back doors for assignee and Affects Version -- is in +`.claude/skills/jira-issue/references/publish.md`. Do not duplicate it here. -If the branch already exists on the remote, just `git push`. +**Never transition to Done or Resolved.** That follows a merged PR, and it is +not this skill's call. -## Step 9: Create a Pull Request +## Pause for the user -Use the GitHub MCP tools (or the `mcp_github_create_pull_request` -tool) to create a PR: +1. The current branch does not match the ticket. +2. A failing test cannot be written. +3. Devil's advocate surfaced a real alternative or uncertainty. +4. The fix reveals a larger problem than the ticket describes. -- **Title**: `Fix LT-XXXXX: ` -- **Base**: `main` (or the appropriate target branch) -- **Body** should include: - - Problem description (from JIRA) - - Root cause analysis - - Fix description with rationale - - Files changed table - - All tests added with descriptions - - Test results summary - - Design rationale for non-obvious choices +## When things fail -Present the PR URL to the user. +- **JIRA unreachable** -- ask for the details, carry on, update the ticket at the end. +- **Tests fail after the fix** -- show the failures and ask; do not tune the test to pass. +- **Push rejected** -- pull with rebase, resolve, retry. -## Step 10: Update JIRA Ticket +## Composes with -Add a comment to the JIRA ticket with the fix summary and -PR link: - -```powershell -python -c " -import sys -sys.path.insert(0, '.claude/skills/atlassian-skills/scripts') -from jira_issues import jira_add_comment -comment = '''Fix implemented and PR created. - -*Root cause*: - -*Fix*: - -*PR*: https://github.com/sillsdev/FieldWorks/pull/NNN - -*Tests added*: -- : -- : -''' -print(jira_add_comment('LT-XXXXX', comment)) -" -``` - -Do NOT transition the ticket to "Done" or "Resolved" — that -happens after the PR is merged and verified. - -## IDE-Specific Notes - -### VS Code -- Use the terminal for all Python/git/build commands -- Use the GitHub MCP tools for PR creation -- Use the `mcp_github_*` tools for PR review operations - -### Visual Studio -- Use the Package Manager Console or Developer PowerShell - for Python/git commands -- The JIRA Python scripts work the same way -- For PR creation, use the GitHub CLI (`gh pr create`) or - the VS GitHub extension - -## Decision Points (Pause for User) - -This workflow pauses for user input at these points: -1. **Branch**: If current branch doesn't match the ticket -2. **Untestable bug**: If a failing test cannot be written -3. **Devil's advocate**: If the review surfaces - uncertainties or alternative approaches -4. **Scope expansion**: If the fix reveals larger issues - -All other steps proceed automatically. - -## Error Handling - -- **JIRA unreachable**: Ask user for issue details manually, - continue with the workflow, and update JIRA at the end -- **Tests fail after fix**: Present failures, ask user - whether to investigate or revert -- **Build fails**: Present errors, attempt to fix, or ask - user for guidance -- **Push rejected**: Pull with rebase, resolve conflicts, - retry - -## Integration with Other Skills - -This skill composes with: -- `jira-issue` — files the ticket in the first place, and - hands off to this skill at Step 3 when the user chooses - "start now" -- `atlassian-readonly-skills` — reading JIRA issues -- `atlassian-skills` — writing to JIRA (assign, comment) -- `session-workflow` — session management and handoff -- `execute-implement` — implementation conventions -- `verify-test` — test verification -- `rubric-verify` — weighted rubric scoring + hard-gate checks -- `review` — self-review before PR - -## Quick Reference - -``` -Skill chain: - jira-bugfix - ├── atlassian-readonly-skills (Step 0: read) - ├── atlassian-skills (Steps 1, 10: write) - ├── execute-implement (Step 4: fix) - ├── verify-test (Steps 3, 5: test) - ├── review (Step 6: devil's advocate) - ├── commit-messages.instructions (Step 8: commit) - └── mcp_github / gh CLI (Step 9: PR) -``` -``` +`jira-issue` (files the ticket, hands off here at step 3) · +`atlassian-skills` (JIRA writes) · `commit-messages` (step 8) · +`pr-preflight` (step 9) · `fieldworks-test-coverage` (step 5) diff --git a/.claude/skills/pr-pitch/SKILL.md b/.claude/skills/pr-pitch/SKILL.md index c899110402..94905a0401 100644 --- a/.claude/skills/pr-pitch/SKILL.md +++ b/.claude/skills/pr-pitch/SKILL.md @@ -6,370 +6,135 @@ argument-hint: "Optional PR number (defaults to the PR for the current branch)" # PR Pitch -**Not an entrypoint.** `pr-preflight` is the single entrypoint for making a -PR; it calls this skill for the write-up step. Invoke this directly only to -redo the write-up on a PR that already exists. +**Not an entrypoint.** `pr-preflight` calls this for the write-up. Invoke it +directly only to redo the write-up on an existing PR. -Related skills this one calls: `fieldworks-code-commenting` (when doc text -is inlined into source), `fieldworks-migration-scope-review` (when the PR is -a large migration branch and the pitch must justify its scope). - -Read `.claude/references/compact-style.md` before writing the pitch. It is the -shared style contract for issues and PR bodies, and it is where the banned -openers, the five-item list cap and the pre-send check live. - -Read `.claude/references/evidence.md` before publishing any screenshot. It -covers trimming, captioning, provenance labelling, the three upload routes, -and the permission question that precedes all of them. +Read `.claude/references/compact-style.md` before writing, and +`.claude/references/evidence.md` before publishing any screenshot. ## What this produces -Two artifacts, always together, never one without the other: +Two artifacts, always together: -1. **The PR body**, in two zones: - - **The pitch** -- 200-400 words, above the fold, uncollapsed. - - **The accordions** -- collapsed `
` sections below it, holding - the decisions, evidence, and paths not taken. -2. **A commit** that deletes the provenance sources from the branch, so - research and working notes inform the reviewer without merging. +1. **The PR body** -- a pitch of 200-400 words above the fold, then collapsed + `
` accordions holding decisions, evidence and paths not taken. +2. **A commit deleting the provenance sources** from the branch, so research + and working notes inform the reviewer without merging. -**Everything goes in the body -- never a separate comment.** The PR -description is the one place a reader always looks, it is inherently sticky -(editing it is always in place, the URL never changes), and it is the only -part of the PR that survives being read a year later without scrolling a -thread. A second comment splits the record for no gain: nothing here needs a -comment's affordances, and no CI job posts alongside it. +Do both or neither. Accordions without the deletion merges the scaffolding; +deletion without accordions loses the reasoning. -If you write the accordions but skip the deletion, the branch merges its own -scaffolding. If you delete without the accordions, the reasoning is lost. Do -both or do neither. +**Everything goes in the body, never a separate comment.** The description is +the one place a reader always looks, editing is in place, and it survives being +read a year later without scrolling a thread. ## The rule that drives everything -A reviewer arrives at the PR with the **same unknowns the author started -with**, and no time to rediscover them. The pitch closes that gap; the -accordions hold what a future maintainer -- not a reviewer -- will want when -they ask "why is it like this?" a year from now. +A reviewer arrives with the **same unknowns the author started with**, and no +time to rediscover them. So, for every sentence: -So the test for every sentence: - -- A reviewer needs it to say yes or no today -> **the pitch**. -- A reviewer would want it only to *check* a claim the pitch makes -> - **an accordion**. State the claim above the fold, put the proof below it. - This is where most over-long bodies go wrong: the proof is real and - interesting and belongs on the PR, just not above the fold. -- Nobody needs it today, but someone will in a year -> **an accordion**. -- Neither -> **delete it**. +| Test | Zone | +| --- | --- | +| A reviewer needs it to say yes or no today | The pitch | +| They would want it only to *check* a claim the pitch makes | An accordion | +| Nobody needs it today, but someone will in a year | An accordion | +| Neither | Delete it | ## Phase 1 -- Triage every markdown file on the branch -List what the branch adds or changes: - ``` git diff --name-status main...HEAD -- '*.md' ``` -Classify each file into exactly one bucket. Judge by what the file **is**, -not where it lives. +Judge by what the file **is**, not where it lives. | Bucket | Test | Destination | | --- | --- | --- | -| DURABLE | Someone changing this code next year must read it to change it correctly | Stays in repo -- align it in Phase 2 | -| RESEARCH | A one-time investigation, audit, census, or survey whose conclusion is now baked into the code | Provenance comment, then delete | -| NOT-TAKEN | Options considered and rejected, alternatives, tradeoffs, abandoned approaches | Provenance comment, then delete | -| PROCESS | Task checklists, burn-down guides, review checklists, working notes tied to *doing* the work | Provenance comment (only the durable findings), then delete | -| STALE | Describes code that no longer exists | Delete. Salvage to provenance only if it records a real reversal | - -Two traps: - -- **A spec is not automatically durable.** A spec that merely narrates what - the code now plainly says is RESEARCH. Durable means it carries - constraints the code cannot express: invariants, contracts with other - subsystems, rejected designs that will otherwise be re-proposed. -- **Skills are usually durable, references are usually not.** A - `SKILL.md` that guides future work stays. A `references/*.md` capturing - one migration's findings is RESEARCH. - -Present the triage to the developer as a table and get confirmation before -deleting anything. Deletion is theirs to approve. - -## Phase 2 -- Align what stays with the code as it is - -**Always trust the written code.** When a doc and the code disagree, the -code is right and the doc is wrong -- never the reverse, and never -"reconcile" by softening the doc into vagueness. - -For each DURABLE file, verify every concrete noun it names -- type, -interface, method, namespace, file path, setting key, test name -- against -the tree: - -``` -git grep -l "" -- 'Src/*' -``` - -Zero hits means the doc is wrong. Find what the thing is called now and fix -the doc. Renames, folder moves, and split interfaces are the usual cause. - -Also re-check every **count** a doc asserts ("~8 call sites", "12 markers"). -Counts rot silently. Recount from the tree and correct. - -Do not rewrite prose that is still accurate. This phase is a correction -pass, not a rewrite. - -## Phase 3 -- Write the pitch (the top zone) - -**Budget: 200-400 words, and it must fit on one screen without scrolling.** -That is the binding constraint, not a target. Reviewers skim the description -and leave for the diff; a body that runs past a screen buries the one -paragraph that would have saved them an hour, and the length itself reads as -"the author could not say what this does." - -Scale by risk, not by diff size. An 82,000-line branch whose whole story is -"one flag, defaulted off" gets a *shorter* body than a 200-line change to a -payment path, because there is less a reviewer must hold in their head. - -The budget applies to the top zone only. The accordions below it are as long -as the branch's reasoning deserves -- a closed `
` costs a reader -nothing. There is no tension between "short" and "complete" here; there is -only the question of which zone a sentence belongs in. - -A reviewer arrives with two kinds of gap. **Known unknowns** -- the questions -they already know they have on opening this diff -- and **unknown unknowns**, -the things only you can see because you built it. The top zone answers those -two and nothing else. Everything you happen to know that answers neither goes -below the fold, or nowhere. - -The overflow rule: when a section will not fit the budget, that section was -accordion material. Move it down and leave one line up top. Never compress by -deleting the qualifiers that make a claim honest. - -The top zone, in this order. - -### 0. Status, for a PR that has been open a while (one line, optional) - -A reviewer returning to a long-lived PR cannot hold its state between visits. -Give them one line at the top: what it is waiting on, and anything red. - -> Ready for review. CI green except the known-flaky interlinear test. +| DURABLE | Someone changing this code next year must read it to change it correctly | Stays -- align it in Phase 2 | +| RESEARCH | A one-time investigation whose conclusion is now baked into the code | Accordion, then delete | +| NOT-TAKEN | Options considered and rejected | Accordion, then delete | +| PROCESS | Checklists and working notes tied to *doing* the work | Accordion (durable findings only), then delete | +| STALE | Describes code that no longer exists | Delete. Salvage only a real reversal | -Skip it on a PR opened today. It exists so nobody has to reconstruct the state -from a comment thread. +Two traps: **a spec is not automatically durable** -- one that narrates what +the code plainly says is RESEARCH; durable means constraints the code cannot +express. And **skills are usually durable, references usually not.** -### 1. Lead with what it does (2-3 sentences) +Present the triage as a table and get confirmation. **Deletion is the +developer's to approve.** -Open with the concrete thing, not the framing. A screenshot or GIF if the -change is visible; otherwise one sentence naming what a user or caller can -now do that they could not before. Never open with "This PR refactors...". +## Phase 2 -- Align what stays -**If the change is visible, a picture is expected, not optional.** For -Avalonia work the capture is automated and there is no excuse for its absence; -for WinForms work the author supplies it. Every image is trimmed, captioned -with what to look at, and labelled with its provenance -- a headless -control-level capture is not a screenshot of the product, and a body that -blurs the two misleads the reviewer about what has been proven. The image -supports the claim; the test pins it. See `.claude/references/evidence.md`. +**Always trust the written code.** Where a doc and the code disagree, the doc +is wrong -- never the reverse, and never "reconcile" by going vague. -### 2. The unknown the reviewer starts with (one paragraph) +Verify every concrete noun a DURABLE file names -- type, method, path, setting +key, test name -- with `git grep -l "" -- 'Src/*'`. Zero hits means the +doc is wrong; find the current name. Recount every **count** it asserts; counts +rot silently. This is a correction pass, not a rewrite. -State the question the reviewer will actually have on opening a diff this -size -- usually "what breaks?" or "why is this so big?" -- answer it, and -then reframe the review around the question that *is* worth their time. Do -not make them read to find it. +## Phase 3 -- The pitch -### 3. Where to look (at most five bullets, one line each) +**200-400 words, fitting one screen without scrolling.** That is binding, not +a target. Scale by *risk*, not diff size: an 82,000-line branch whose story is +"one flag, defaulted off" gets a shorter body than a 200-line payment change. -Open with one `**Start here:**` line naming the first file to read and why. -That is the *entry point*, not the riskiest thing -- a reviewer who is not told -where to begin reads the diff in alphabetical order, which is nobody's reading -order. +When a section will not fit, it was accordion material -- move it down and +leave one line up top. Never compress by deleting the qualifiers that make a +claim honest. -> **Start here:** `ViewDefinitionOverrideApplier.cs` -- everything else is -> plumbing around what it does. +In order: -Then the load-bearing part. The failure points a domain expert would -anticipate, ordered by what would sink the PR -- not by what was hardest to -build. One line each: the risk, and the thing that pins it -- the gate, the -test, the invariant. +**0. Status** (optional, one line) -- only for a PR open a while. What it is +waiting on, and anything red. *"Ready for review. CI green except the +known-flaky interlinear test."* -The evidence for each line goes in an accordion below. A reviewer who wants -to check rather than trust opens it; one who trusts the summary never pays -for it. Do not inline the proof here -- that is what blew the budget on every -over-long body this skill exists to prevent. +**1. What it does** (2-3 sentences) -- the concrete thing, not the framing. +Never open with "This PR refactors". If the change is visible, **a picture is +expected, not optional**: trimmed, captioned with what to look at, and +labelled headless / live / mockup. The image supports the claim; the test pins +it. -### 4. What is deliberately not here (one list, one line each) +**2. The unknown they start with** (one paragraph) -- usually "what breaks?" +or "why is this so big?". Answer it, then reframe the review around the +question worth their time. -Deferrals, parity gaps, known-narrower paths. A reviewer who finds an -unlisted gap stops trusting the whole pitch, so listing them buys more than -it costs. Name the in-code marker or the follow-up PR, not the reasoning -- -reasoning goes in an accordion. +**3. Where to look** (at most five bullets) -- open with one +`**Start here:**` line naming the first file to read and why. That is the +*entry point*, not the riskiest thing; a reviewer not told where to begin +reads the diff alphabetically, which is nobody's reading order. Then the +failure points a domain expert would anticipate, ordered by what would sink +the PR -- one line each: the risk, and the thing that pins it. **The proof goes +in an accordion**; inlining it is what blows the budget. -### 5. Stack and verification (a few lines) +**4. What is deliberately not here** -- deferrals, parity gaps, narrower +paths. A reviewer who finds an unlisted gap stops trusting the whole pitch. +Name the marker or follow-up PR, not the reasoning. -If the PR is stacked, say what it merges into and in what order. Then build, -tests, manual checks -- and what was *not* run. Anything currently red or -known-broken goes here in plain words; a reviewer must never learn of a red -job from the checks tab after reading a body that implied green. +**5. Stack and verification** -- what it merges into and in what order. Build, +tests, manual checks, and what was *not* run. Anything red goes here in plain +words; a reviewer must never learn of a red job from the checks tab. Give +anything they must run themselves as numbered, copy-pasteable steps -- prose +verification instructions get skipped. -When the reviewer needs to run something themselves, give it as numbered -steps, one bounded action each, copy-pasteable: +**6. What you want from them** (one line) -- *"Next: approve, or tell me to +split the installer change out."* A pitch ending on a verification paragraph +leaves the reviewer guessing whether they are approving, splitting or blocking. -> 1. `.\build.ps1 -CommentHygiene` -> 2. `.\test.ps1 -TestProject '' -TestFilter ''` -> 3. Open Grammar > Natural Classes and delete a feature-based class. +### Pitch rules -Prose verification instructions get skipped. Numbered ones get run. - -### 6. What you want from the reviewer (one line) - -Close the top zone with a single line naming the next action and who takes it. -A pitch that ends on a verification paragraph leaves the reviewer guessing -whether they are approving, splitting, or blocking. - -> Next: approve, or tell me to split the installer change out. - -Pitch rules: - -- No process narration. "We then discovered...", "after several - iterations..." -- cut. The reviewer is approving a result, not a journey. +- No process narration. "We then discovered", "after several iterations" -- cut. - No apology, no hedging, no "should be fine". -- No section that exists only to demonstrate rigor. Depth belongs below the - fold, where it costs the reviewer nothing. +- No section that exists only to demonstrate rigor. - Every claim with a name in it must be true of the current tree. Re-verify - claims carried over from an earlier version of the body; long-lived PRs - accumulate stale ones. -- Word-count the result before publishing. Over 400, cut -- do not rationalize. - -## Phase 4 -- Write the accordions (the bottom zone) - -**Synthesize; do not paste.** Dumping the specs into `
` blocks is -the failure mode this skill exists to prevent. The accordions carry what -someone will care about later, in your words, with the reasoning intact and -the scaffolding gone. - -They go directly below the pitch in the same PR body, after a `---` rule. -Open with a short orienting accordion that says what this record is and why -it exists outside the tree -- a reader a year out needs to know the working -documents were deliberately deleted, not lost. - -```markdown ---- - -
-Reading this a year from now -- start here - -What this record is, and why the reasoning lives here instead of in the -tree. - -
+ claims carried over from an earlier version of the body. +- Word-count before publishing. Over 400, cut -- do not rationalize. -
-Decisions, and why +## Phases 4 and 5 -... - -
-``` - -Sections worth writing, when the branch has them: - -- **Reading this a year from now** -- the orienting preamble. First. -- **The layer cake** -- for a branch that introduces an architecture, the - path from input to output with the real type at each hop. This is the - single most useful thing for someone arriving cold. -- **Decisions, and why** -- the choice, the alternatives, what tipped it. - Prefer decisions where the code looks arbitrary until you know the reason. -- **Paths not taken** -- what was tried or seriously considered and - rejected, and the reason. The single most valuable section; it is what - stops the next person re-proposing a dead end. -- **Reversals** -- things built and then removed, with why. Note which ones - are invisible in `git log` because they happened inside a squash. -- **Surprising findings** -- what the investigation turned up that - contradicted the initial assumption. -- **What this does NOT authorize** -- for a foundational branch, the limits. - A later reader will otherwise cite the branch as precedent for more than - it decided. -- **Deferred, and what would unblock it** -- what was scoped out and what - would need to be true to pick it up. -- **Evidence** -- the proof behind the pitch's "Where to look" bullets: the - predicate quoted, the call-site count, the equivalence argument, the test - that pins it. Written for a reviewer rather than a maintainer, so it can - sit last; the pitch already carries the claim. - -Rules: - -- Each section stands alone. Nobody reads these top to bottom. -- Attribute nothing to a person; describe the decision, not the deciders. -- GitHub caps a PR body at 65,536 characters. If you are near it, you are - pasting, not synthesizing. Cut harder. -- Reasoning recoverable *only* from git history -- because the branch - deleted the document that argued it -- is the highest-value content here. - Prefer it over anything a reader could derive by reading the tree. - -## Phase 5 -- Apply - -In this order: - -1. Delete the RESEARCH / NOT-TAKEN / PROCESS / STALE files (`git rm`), after - the developer has confirmed the Phase 1 triage. -2. Commit the Phase 2 alignment edits and the deletions together, with a - message saying the reasoning moved to the PR. -3. Push. -4. Update the PR body -- pitch and accordions, one write. - -Write the body to a file first -- never pass it inline. Then: - -```powershell -# gh pr edit is fine when the token has the scope; when it does not -# (it wants org:read), go straight to REST. -gh pr edit --body-file body.md -gh api -X PATCH repos///pulls/ -F body=@body.md --jq '.body|length' -``` - -The body is inherently sticky: editing it is always in place and the PR URL -never changes, so nothing needs marker-matching or an existence check. That -is the main reason the record lives here rather than in a comment. - -If an earlier run of this skill left a separate provenance comment, fold its -content into the accordions and delete it, so there is exactly one record: - -```powershell -gh api -X DELETE repos///issues/comments/ -``` - -Deleting is destructive and public -- confirm with the developer first, and -only after its content is verifiably in the published body. - -Anything in the tree that pointed at that comment now dangles. Check before -you delete, and repoint at the PR description: - -``` -git grep -n "provenance comment\|issuecomment" -- openspec/ Docs/ Src/ -``` +Accordions: `references/accordions.md`. Applying and the final checklist: +`references/publishing.md`. -Note that a wrapped line will defeat a naive grep for a two-word phrase -- -search for each word. - -Before finishing, confirm: - -- [ ] The pitch zone is 200-400 words and fits one screen. Count, do not - estimate. -- [ ] Every `
` is closed -- count `
` against `
`. -- [ ] The whole body is under 65,536 characters. -- [ ] No content lives in a PR comment; the description is the only record. -- [ ] Nothing in the tree references a comment that was deleted. -- [ ] No deleted file's content was lost -- each is represented in an - accordion or was deliberately dropped as STALE. -- [ ] Every name in the body resolves in the current tree -- accordions rot - the same way the pitch does, and a rename sweep late in a branch will - have stranded names the earlier reasoning used. -- [ ] Every count was recounted. -- [ ] The pitch does not repeat what an accordion already says. -- [ ] Working notes are gitignored (`Docs/migration/working/`), not merged. -- [ ] The top zone opens with a `**Start here:**` line and closes with a - `Next:` line. -- [ ] The pre-send check in `.claude/references/compact-style.md` passes: - reading only the title and the last line, does the reviewer know what - this does and what is wanted from them? - -Do not mark this complete on unverified claims. If a claim could not be -checked, say so in the report rather than asserting it. +Related skills this calls: `fieldworks-code-commenting` (when doc text is +inlined into source), `fieldworks-migration-scope-review` (large migration +branches). diff --git a/.claude/skills/pr-pitch/references/accordions.md b/.claude/skills/pr-pitch/references/accordions.md new file mode 100644 index 0000000000..13af123331 --- /dev/null +++ b/.claude/skills/pr-pitch/references/accordions.md @@ -0,0 +1,43 @@ +# The accordions (the bottom zone) + +Everything below the pitch, in collapsed `
` blocks after a `---`. +A closed `
` costs a reader nothing, so length here is free -- but +**synthesise, do not paste.** Dumping the branch's specs into `
` +blocks is the failure mode this skill exists to prevent. + +Open with a short orienting block, so a reader a year out knows the working +documents were deliberately deleted rather than lost. + +```markdown +--- + +
+Reading this a year from now -- start here + +What this record is, and why the reasoning lives here instead of in the tree. + +
+``` + +## Sections worth writing, when the branch has them + +| Section | Carries | +| --- | --- | +| **Reading this a year from now** | The orienting preamble. Always first | +| **The layer cake** | For a branch introducing an architecture: input to output, with the real type at each hop. The most useful thing for someone arriving cold | +| **Decisions, and why** | The choice, the alternatives, what tipped it. Prefer decisions where the code looks arbitrary until you know the reason | +| **Paths not taken** | What was tried or seriously considered and rejected. The highest-value section -- it stops the next person re-proposing a dead end | +| **Reversals** | Built then removed, and why. Note which are invisible in `git log` because they happened inside a squash | +| **Surprising findings** | What contradicted the initial assumption | +| **What this does NOT authorize** | For a foundational branch, the limits. Otherwise it gets cited as precedent for more than it decided | +| **Deferred, and what would unblock it** | Scoped out, and what would need to be true to pick it up | +| **Evidence** | The proof behind the pitch's "Where to look" bullets. Written for a reviewer rather than a maintainer, so it sits last | + +## Rules + +- Each section stands alone. Nobody reads these top to bottom. +- Attribute nothing to a person. Describe the decision, not the deciders. +- GitHub caps a body at 65,536 characters. Near it means you are pasting. +- **Reasoning recoverable only from git history** -- because the branch + deleted the document that argued it -- is the highest-value content here. + Prefer it over anything a reader could derive by reading the tree. diff --git a/.claude/skills/pr-pitch/references/publishing.md b/.claude/skills/pr-pitch/references/publishing.md new file mode 100644 index 0000000000..71d83bfa6b --- /dev/null +++ b/.claude/skills/pr-pitch/references/publishing.md @@ -0,0 +1,67 @@ +# Applying the pitch + +Order: + +1. `git rm` the RESEARCH / NOT-TAKEN / PROCESS / STALE files, **after** the + developer has confirmed the triage. +2. Commit the alignment edits and the deletions together, with a message + saying the reasoning moved to the PR. +3. Push. +4. Update the PR body -- pitch and accordions, one write. + +Write the body to a file first. Never pass it inline. + +```powershell +gh pr edit --body-file body.md +``` + +`gh pr edit` needs `read:org` on the token and will fail with a GraphQL scope +error without it. Go straight to REST when it does: + +```powershell +gh api -X PATCH repos///pulls/ -F body=@body.md --jq '.body|length' +``` + +The body is sticky: editing is in place and the PR URL never changes, so +nothing needs marker-matching or an existence check. That is the main reason +the record lives here rather than in a comment. + +## Folding in an old provenance comment + +If an earlier run left a separate comment, move its content into the +accordions and delete it, so there is exactly one record: + +```powershell +gh api -X DELETE repos///issues/comments/ +``` + +Deleting is destructive and public -- confirm first, and only once the content +is verifiably in the published body. Anything in the tree pointing at that +comment now dangles: + +``` +git grep -n "provenance comment\|issuecomment" -- openspec/ Docs/ Src/ +``` + +A wrapped line defeats a naive two-word grep. Search for each word. + +## Before finishing + +- [ ] The pitch zone is 200-400 words and fits one screen. Count, do not estimate. +- [ ] It opens with `**Start here:**` and closes with a `Next:` line. +- [ ] Every `
` is closed -- count `
` against `
`. +- [ ] Whole body under 65,536 characters. +- [ ] No content lives in a PR comment; the description is the only record. +- [ ] Nothing in the tree references a comment that was deleted. +- [ ] No deleted file's content was lost -- each is in an accordion, or was + deliberately dropped as STALE. +- [ ] Every name in the body resolves in the current tree. Accordions rot the + same way the pitch does, and a late rename sweep strands names the + earlier reasoning used. +- [ ] Every count was recounted. +- [ ] The pitch does not repeat what an accordion already says. +- [ ] Working notes are gitignored (`Docs/migration/working/`), not merged. +- [ ] The pre-send check in `.claude/references/compact-style.md` passes. + +Do not mark this complete on unverified claims. If a claim could not be +checked, say so in the report rather than asserting it. diff --git a/.claude/skills/pr-preflight/SKILL.md b/.claude/skills/pr-preflight/SKILL.md index f032ea6f1d..098acf72d1 100644 --- a/.claude/skills/pr-preflight/SKILL.md +++ b/.claude/skills/pr-preflight/SKILL.md @@ -7,269 +7,112 @@ user-invocable: true # PR Preflight -Use this skill when the user wants an interactive FieldWorks branch review before posting or updating a PR. +Interactive branch review before a PR is posted or updated. This is the +orchestration layer: review policy lives in +`.github/instructions/review-analyzer.instructions.md`, shared terminology in +`CONTEXT.md`, and the write-up belongs to `pr-pitch`. -This is the orchestration layer. The review policy lives in `.github/instructions/review-analyzer.instructions.md`, shared terminology lives in `CONTEXT.md`, and specialized reviewer agents may be used for independent read-only passes. - -## Goals - -- Analyze the branch diff from `origin/main` using FieldWorks review policy. -- Use specialist agents for deep read-only review where they fit the changed files. -- Challenge the author on risks, assumptions, validation gaps, and design understanding. -- Record author explanations, dismissed findings, unresolved concerns, and in-review fixes. -- Write a fresh `.review/summary.md` that a reviewer can use as a meeting agenda. -- Optionally commit, push, and create or update a PR only after the author confirms readiness, delegating the write-up to `pr-pitch`. - -## Start Here - -First tell the author what will happen: - -> **Here's what this preflight will do:** -> -> 1. **Setup** - Check your branch and ask a couple of quick questions -> 2. **Analysis** - Review contracts/correctness, managed UI/localization, native interop/COM, build/test/installer risk, and validation evidence -> 3. **Interview** - Walk through findings and challenge the reasoning -> 4. **Output** - Write `.review/summary.md` and optionally create or update a PR -> -> During the interview, you can explain changes, dismiss findings with reasons, ask me to fix something, or say you are unsure. I will record all of that for the reviewer. +Tell the author what will happen before starting: setup, analysis, interview, +then `.review/summary.md` and optionally the PR. During the interview they can +explain, dismiss a finding with a reason, ask for a fix, or say they are +unsure -- all of which get recorded. ## Setup -1. Determine the review model name. Use `GitHub Copilot` when running in Copilot. Do not invent AI co-author trailers. -2. Validate the current branch: - - Run `git branch --show-current`. - - If the branch is `main`, stop and tell the author to run this from a feature branch. -3. Check working tree state with `git status --porcelain`. - - If there are uncommitted changes, explain that any fixes made during the review will be staged and may be committed with existing changes at the end. - - Ask whether the author wants to commit existing work first or continue with it included. -4. Compute the diff range against `origin/main`. - - Run `git fetch origin --quiet`. - - Compute `MERGE_BASE` with `git merge-base origin/main HEAD`. - - Compute `HEAD_SHA`, `FILE_COUNT`, and `INITIAL_COMMIT_COUNT`. - - List changed files with `git diff --name-only MERGE_BASE`. - - If this fails, ask the author which base branch to use. -5. Check whether `.review` is ignored. - - If `.review` is missing from `.gitignore`, ask whether to add it now. - - Do not combine this question with the purpose question. -6. Determine the branch purpose. - - Use any purpose supplied in the prompt invocation. - - If none was supplied, ask: "What is the overall purpose of these changes? Please describe it in your own words." - -## Context And Language Check - -Before analysis, load `CONTEXT.md` and `.github/context/codebase.context.md`. - -If the branch purpose, PR title, plan, or spec uses overloaded FieldWorks terms such as `project`, `model`, `view`, `app`, `context`, `review`, or `validation`, apply the `grill-with-docs` discipline before writing the summary: +1. `git branch --show-current`. **Stop if it is `main`.** +2. `git status --porcelain`. If dirty, ask whether to commit first; in-review + fixes will otherwise be committed alongside. +3. `git fetch origin --quiet`, then merge-base against `origin/main`. Record + the file count and commit count. +4. If `.review` is not gitignored, ask to add it -- as its own question. +5. Ask the branch purpose in the author's own words, unless supplied. -- Clarify the term with the author. -- Ground the term in code, docs, tests, or build files. -- Update `CONTEXT.md` only for durable shared language decisions. -- Carry the clarified terms into findings, interview notes, and PR copy. +Load `CONTEXT.md` and `.github/context/codebase.context.md`. If the purpose or +title uses an overloaded FieldWorks term -- `project`, `model`, `view`, `app`, +`context`, `review`, `validation` -- apply `grill-with-docs` before writing +the summary, and carry the clarified term into the findings and PR copy. ## Analysis -Load `.github/instructions/review-analyzer.instructions.md` and run all four required passes: - -- Contracts, compatibility, and correctness. -- Managed UI, C#, and localization. -- Native, COM, and boundary safety. -- Build, tests, CI, dependencies, and installer. - -Use specialist agents as independent read-only reviewers when the changed files justify them and the agent tooling is available. Keep them scoped; the final synthesis remains your responsibility. - -Recommended agents: - -- `FieldWorks C# Expert` for managed `*.cs`, `.csproj`, config, resources, or net48 behavior. -- `FieldWorks WinForms Expert` for WinForms UI, designer, layout, event-handler, resource, or localization changes. -- `FieldWorks C++ Expert` for native, C++/CLI-adjacent, COM, Views, FwKernel, ViewsInterfaces, or ABI-sensitive changes. -- `FieldWorks Avalonia UI Expert` only for Avalonia/XAML work; do not use it for existing WinForms UI. -- `devils-advocate` for large architecture, scope, or risk arguments where a skeptical pass would sharpen the interview. - -If specialist agents are unavailable or would add friction for a small diff, run the passes directly using the review policy. - -For each pass: - -- Compare against `MERGE_BASE`. -- Verify findings against actual code before reporting. -- Record findings as Critical, Important, or Minor. -- Record positive observations. -- Record required validation and evidence gaps. - -After all passes: - -- Merge findings into one list ordered by severity. -- Deduplicate only when two passes flagged the same file for the same concern. -- Keep distinct concerns about the same file as separate findings. -- Keep a factual Contract/API Changes summary. -- Keep Required Validation separate from findings. - -## Author Interview - -Aim for 5-15 questions total. Ask one Critical or Important finding at a time unless multiple findings share one root cause. - -For each Critical and Important finding: - -- Ask directly why the change is safe or intentional. -- Ask what validation covers it. -- If the answer is vague, ask one follow-up. -- If it remains unclear after the follow-up, record it as unresolved. - -If the changes are large, cross native/managed/build/installer boundaries, involve non-obvious design decisions, or the author's answers reveal uncertainty, ask separately: - -> "Can you walk me through the most complex or non-obvious part of these changes? I want to make sure I understand the reasoning." - -Watch for lack-of-understanding signals: - -- "The AI did it" or similar deferrals. -- "I'm not sure" or "I don't know". -- Vague explanations that do not describe the mechanism. -- Inability to explain a changed section. - -Record those explicitly as `Author does not understand: `. Do not soften them into acceptance or satisfaction. - -For Minor findings: - -- Print the full list first. -- If there are 3 or fewer, ask whether the author wants to respond to all at once or one at a time. -- If there are more than 3, go one at a time. - -End the interview by asking: - -> "Is there anything else you want to flag or discuss before I write up the summary? Any tradeoffs you made, things you're uncertain about, or context a reviewer should know?" - -## In-Review Fixes - -If the author asks you to fix a finding, implement the fix unless it is ambiguous. Ask one clarifying question only when needed. - -Rules: - -- Keep fixes minimal and scoped to the finding. -- Stage fixes with `git add`; do not commit yet. -- Record each fix as `INTERVIEW_CHANGES` with the finding and what changed. -- Do not remove fixed findings from the summary; mark them `[x]` with a fixed-during-review note. - -After in-review fixes, run relevant FieldWorks checks: - -- Use repository scripts and tasks, not ad-hoc `msbuild`, `dotnet build`, `vstest.console`, or `nmake`. -- Prefer the VS Code task `CI: Whitespace check` for whitespace. -- Run `./build.ps1` when build-affecting, managed, native, resource, or project files changed. -- Run `./test.ps1` with the narrowest reliable `-TestProject` or `-TestFilter` for managed behavior. -- Run `./test.ps1 -Native` with `-TestProject` when native code or tests changed. -- Run `./Build/Agent/Setup-InstallerBuild.ps1 -ValidateOnly` for installer/WiX/helper-script changes. -- Do not mark manual validation complete unless you directly performed it or the author explicitly confirms it. - -Report checks that were skipped and why. - -## Summary File - -Always write a fresh `.review/summary.md`. Do not merge with an existing summary. - -Use this structure: - -```markdown -# Code Review Summary - -**Branch**: - -**Base**: - -**Date**: - -**Review model**: - -**Files changed**: - -## Overview - -[One or two paragraphs combining the author's purpose with the analysis result.] - -## Contract/API Changes - -[Factual Contract/API Changes summary. Write "None." if none.] - -## Findings - -Finding states: -- `- [ ] **Description**` - open -- `- [ ] ~~Description~~ _(author's explanation)_` - dismissed -- `- [x] **Description** _(fixed during review: what changed)_` - fixed - -### Critical - Must address before merge - -[All Critical findings, or "None."] - -### Important - Should address before merge - -[All Important findings, or "None."] - -### Minor - Consider - -[All Minor findings, or "None."] - -## Required Validation / Evidence - -[Commands run, commands still needed, manual validation gaps, or "None."] +Run all four passes from `review-analyzer.instructions.md`: -## Positive Observations +1. Contracts, compatibility, correctness +2. Managed UI, C#, localization +3. Native, COM, boundary safety +4. Build, tests, CI, dependencies, installer -[Positive observations, or "None."] +Use specialist read-only agents where the changed files justify one -- +`FieldWorks C# Expert`, `WinForms Expert`, `C++ Expert`, `Avalonia UI Expert` +(Avalonia work only), `devils-advocate` for large scope or risk arguments. Run +the passes directly for a small diff. The synthesis is yours either way. -## Interview Notes +Per pass: compare against the merge base, **verify each finding against the +actual code before reporting it**, and grade Critical / Important / Minor. +Record positive observations and validation gaps too. Merge into one severity +-ordered list; deduplicate only when two passes flagged the same file for the +same concern. -[Author explanations, decisions, unresolved items, and explicit lack-of-understanding notes.] +## Interview -## In-Review Quality Check +5-15 questions. One Critical or Important finding at a time, unless several +share a root cause. -[Only include if in-review changes were made.] +Per finding: why is this safe or intentional, and what validation covers it? +One follow-up if the answer is vague; if still unclear, record it unresolved. -## Suggested Review Focus +For large, cross-boundary or non-obvious changes, ask separately: -- [ ] [High-priority review meeting agenda item] -- [ ] [High-priority review meeting agenda item] -``` +> "Can you walk me through the most complex or non-obvious part of these +> changes? I want to make sure I understand the reasoning." -## PR Offer +**Record lack of understanding literally.** "The AI did it", "I'm not sure", +or an explanation that never describes the mechanism becomes +`Author does not understand: `. Never soften it into acceptance. -After writing the summary, tell the author: +Minor findings: print them all first. Three or fewer, ask whether to take them +together; more than three, go one at a time. -> "Review summary written to `.review/summary.md`. -> -> Please review it, make changes where appropriate, and run `/pr-preflight` again until you are ready to post the PR. -> -> If you do not want to make any changes and are ready for review, would you like me to commit any uncommitted changes, push, and post the PR? I will check whether one already exists for this branch and update it, or create a new one if not. The write-up runs through `pr-pitch`, which will also triage any research or working markdown on the branch into collapsed sections in the PR body and out of the tree -- you approve that triage before anything is deleted." +Close with: "Anything else to flag -- trade-offs, uncertainties, context a +reviewer should know?" -Only create or update a PR after the author confirms. +## In-review fixes -## PR Description +Keep them minimal and scoped to the finding. `git add`, do not commit yet. +Record each as `INTERVIEW_CHANGES`. **Do not delete a fixed finding** -- mark +it `[x]` with a fixed-during-review note. -This skill is the single entrypoint for making a PR, but it does not compose -the description itself. Once the author confirms readiness, invoke the -`pr-pitch` skill and let it own the write-up. It produces two artifacts -together: the PR body (a pitch above the fold, provenance in collapsed -accordions below it) and a commit evicting the branch's research and working -markdown from the tree. +Then run the repo scripts, never ad-hoc `msbuild` / `dotnet build` / +`vstest.console` / `nmake`: -Hand `pr-pitch` the branch purpose, the findings, and `.review/summary.md`. +| Changed | Run | +| --- | --- | +| Anything build-affecting | `./build.ps1 -CommentHygiene` | +| Managed behaviour | `./test.ps1` with the narrowest reliable `-TestProject` or `-TestFilter` | +| Native code or tests | `./test.ps1 -Native -TestProject

` | +| Installer, WiX, helper scripts | `./Build/Agent/Setup-InstallerBuild.ps1 -ValidateOnly` | +| Whitespace | VS Code task `CI: Whitespace check` | -The preflight record still belongs on the PR, but as a collapsed section -below the pitch, not as the opening content: +A `-TestFilter` that matches nothing still exits 0 and prints PASS -- check +`Total tests: N` is above zero. **Never mark manual validation complete unless +you performed it or the author explicitly confirms it.** Report skipped checks +and why. -```markdown -

-Preflight review details +## Summary and PR - -[summary] - +Write `.review/summary.md` per `references/summary-template.md`. -
-``` +Then offer -- and only act on confirmation: -Keep the markers so a re-run replaces the section instead of appending a -second copy. Do not duplicate findings, interview notes, or validation logs -into the pitch itself -- the pitch states validation status in a sentence -and leaves the detail here. +> "Summary written to `.review/summary.md`. Review it, make changes where +> appropriate, and re-run until you are ready. When you are, shall I commit, +> push and post the PR? I will update an existing one if there is one. The +> write-up runs through `pr-pitch`, which also triages the branch's research +> and working markdown into collapsed sections in the PR body and out of the +> tree -- you approve that triage before anything is deleted." -For branch names like `lt-1234-anything`, prefix the PR title with `LT-1234:`. Use a sentence-case title based on the actual change, not just the branch slug. +**This skill never composes the description itself.** Hand `pr-pitch` the +branch purpose, the findings and the summary. For a branch named +`lt-1234-anything`, prefix the PR title `LT-1234:` and write a sentence-case +title from the actual change, not the branch slug. -After Copilot or human reviewers leave comments, use `.github/prompts/respond-to-review-comments.prompt.md` to work through the review response loop: evaluate comments, fix sensible requests, ask about ambiguity, verify, commit, push, reply, resolve, and summarize. +After reviewers comment, use `respond-to-review-comments`. diff --git a/.claude/skills/pr-preflight/references/summary-template.md b/.claude/skills/pr-preflight/references/summary-template.md new file mode 100644 index 0000000000..250e6785dd --- /dev/null +++ b/.claude/skills/pr-preflight/references/summary-template.md @@ -0,0 +1,79 @@ +# `.review/summary.md` template + +Always write it fresh. Never merge into an existing summary. + +```markdown +# Code Review Summary + +**Branch**: +**Base**: +**Date**: +**Review model**: +**Files changed**: + +## Overview + +[One or two paragraphs combining the author's stated purpose with what the +analysis found.] + +## Contract/API Changes + +[Factual. "None." if none.] + +## Findings + +### Critical - Must address before merge +### Important - Should address before merge +### Minor - Consider + +## Required Validation / Evidence + +[Commands run, commands still needed, manual gaps, or "None."] + +## Positive Observations + +## Interview Notes + +[Author explanations, decisions, unresolved items, and explicit +lack-of-understanding notes.] + +## In-Review Quality Check + +[Only if in-review changes were made.] + +## Suggested Review Focus + +- [ ] [agenda item] +``` + +Finding states: + +| Markup | Means | +| --- | --- | +| `- [ ] **Description**` | Open | +| `- [ ] ~~Description~~ _(author's explanation)_` | Dismissed, with the reason | +| `- [x] **Description** _(fixed during review: what changed)_` | Fixed | + +A dismissed finding keeps its strikethrough and its reason. Deleting it hides +that the question was ever asked. + +## Carrying the summary onto the PR + +The preflight record belongs on the PR as a **collapsed section below the +pitch**, never as the opening content: + +```markdown +
+Preflight review details + + +[summary] + + +
+``` + +Keep the markers so a re-run replaces the section instead of appending a +second copy. Do not duplicate findings, interview notes or validation logs +into the pitch -- the pitch states validation status in a sentence and leaves +the detail here. From 50ac8505f154b32e4b3d83b5345cd5fffe3be09d Mon Sep 17 00:00:00 2001 From: John Lambert Date: Fri, 21 Aug 2026 11:56:40 -0400 Subject: [PATCH 11/11] Repoint nine skill references that resolved from nowhere Nine cross-references in the Avalonia migration and WinApp skills were written relative to the skill root while the citing file lives inside references/, so they resolved to references/references/.md and pointed at nothing. Every target exists; only the paths were wrong. Seven become plain sibling names, which is what a file inside references/ needs to reach the file next to it. Two genuinely cross skill boundaries and become repo-root paths: winforms-avalonia-parity cites parity-evidence in the migration skill, and architecture-patterns cites style-system in the Avalonia UI skill. Nothing else changed. No prose, no guidance, no file moved or renamed. Verified by resolving every reference in every .claude markdown file from the directory it is written in: nine dangling before, none after. Co-Authored-By: Claude Opus 5 --- .../fieldworks-winapp/navigation/winforms-avalonia-parity.md | 2 +- .claude/skills/fieldworks-winapp/references/research.md | 2 +- .../references/architecture-patterns.md | 2 +- .../references/control-exemplar-map.md | 2 +- .../references/lessons-learned.md | 4 ++-- .../references/migration-checklist.md | 4 ++-- 6 files changed, 8 insertions(+), 8 deletions(-) diff --git a/.claude/skills/fieldworks-winapp/navigation/winforms-avalonia-parity.md b/.claude/skills/fieldworks-winapp/navigation/winforms-avalonia-parity.md index 4bcc24c8a4..90d696e715 100644 --- a/.claude/skills/fieldworks-winapp/navigation/winforms-avalonia-parity.md +++ b/.claude/skills/fieldworks-winapp/navigation/winforms-avalonia-parity.md @@ -17,7 +17,7 @@ a dialog, a browse table. The WinForms side needs the live app (this skill); the - A project with the relevant data (e.g. Words ▸ Analyses parity needs **parsed wordform analyses** — a bare project shows an empty interlinear). Restore per `project-loading.md`. - Evidence folders and parity-bundle naming: see `screenshot-evidence.md` (canonical parity - layout defined in the migration skill's `references/parity-evidence.md` §6). + layout defined in the migration skill's `.claude/skills/fieldworks-winforms-to-avalonia-migration/references/parity-evidence.md` §6). ## The two captures diff --git a/.claude/skills/fieldworks-winapp/references/research.md b/.claude/skills/fieldworks-winapp/references/research.md index 7d50737a67..03644c587e 100644 --- a/.claude/skills/fieldworks-winapp/references/research.md +++ b/.claude/skills/fieldworks-winapp/references/research.md @@ -51,7 +51,7 @@ sequence, cues, and safety notes to perform that route. - One reusable navigation destination or workflow per `navigation/*.md` file. - Shared safety and triggering rules stay in `SKILL.md`. -- Skill maintenance rules stay in `references/how-to-update.md`. +- Skill maintenance rules stay in `how-to-update.md`. - Research rationale stays in this file so future edits can revisit the design without bloating the active route instructions. diff --git a/.claude/skills/fieldworks-winforms-to-avalonia-migration/references/architecture-patterns.md b/.claude/skills/fieldworks-winforms-to-avalonia-migration/references/architecture-patterns.md index 29ba03d9d8..7d61375935 100644 --- a/.claude/skills/fieldworks-winforms-to-avalonia-migration/references/architecture-patterns.md +++ b/.claude/skills/fieldworks-winforms-to-avalonia-migration/references/architecture-patterns.md @@ -367,7 +367,7 @@ delta in the region manifest). > intentionally diverge. The visual-parity evidence type therefore checks > density/layout, not pixel-for-pixel appearance. The density tokens and > per-view border/font rules that this parity is measured against live in -> `fieldworks-avalonia-ui/references/style-system.md`, even where styling +> `.claude/skills/fieldworks-avalonia-ui/references/style-system.md`, even where styling > intentionally diverges. **Canonical code.** `Src/Common/FwAvalonia/FwAvaloniaDensity.cs`; diff --git a/.claude/skills/fieldworks-winforms-to-avalonia-migration/references/control-exemplar-map.md b/.claude/skills/fieldworks-winforms-to-avalonia-migration/references/control-exemplar-map.md index 1fa613fc38..80762f0351 100644 --- a/.claude/skills/fieldworks-winforms-to-avalonia-migration/references/control-exemplar-map.md +++ b/.claude/skills/fieldworks-winforms-to-avalonia-migration/references/control-exemplar-map.md @@ -80,7 +80,7 @@ headless tests), and then PROMOTES it: citations. 2. Add a numbered subsection to dialog-conversion.md (the §2c/§2d shape: what the legacy behavior was, the shared design, the test names). -3. Record any surprise in `references/lessons-learned.md` per its update +3. Record any surprise in `lessons-learned.md` per its update protocol. Until a gap's exemplar exists, conversions that need it stay on the Legacy diff --git a/.claude/skills/fieldworks-winforms-to-avalonia-migration/references/lessons-learned.md b/.claude/skills/fieldworks-winforms-to-avalonia-migration/references/lessons-learned.md index c7662b1bff..81b3f20fc1 100644 --- a/.claude/skills/fieldworks-winforms-to-avalonia-migration/references/lessons-learned.md +++ b/.claude/skills/fieldworks-winforms-to-avalonia-migration/references/lessons-learned.md @@ -248,8 +248,8 @@ requirements it fed are synced to `openspec/specs/lexical-edit-parity-automation `xWorksTests` (~1400 tests share the host); the restored test base holds the undoable task open (no nested `NonUndoableUnitOfWorkHelper`); `OnChangeFilter` takes an (added, removed) delta that `RecordList` composes into its `AndFilter`. -- Skill files changed: `references/architecture-patterns.md` (§13), - `references/parity-evidence.md` (§2a, §3), `references/migration-checklist.md` +- Skill files changed: `architecture-patterns.md` (§13), + `parity-evidence.md` (§2a, §3), `migration-checklist.md` (Phase 7), `SKILL.md` (quick map + workflow step 7), this ledger. ### 2026-06 — Lexical Edit (full entry view), phases 1–2 (seed entry) diff --git a/.claude/skills/fieldworks-winforms-to-avalonia-migration/references/migration-checklist.md b/.claude/skills/fieldworks-winforms-to-avalonia-migration/references/migration-checklist.md index e312758cfd..c5b5cdec1d 100644 --- a/.claude/skills/fieldworks-winforms-to-avalonia-migration/references/migration-checklist.md +++ b/.claude/skills/fieldworks-winforms-to-avalonia-migration/references/migration-checklist.md @@ -47,7 +47,7 @@ Use this when collapsing a large multi-view derisk branch into a landable PR (se ## Phase 3 — Seams - [ ] Existing seams reused from `Src/Common/FwAvalonia/Seams/` -- [ ] Any new seam added to `references/seam-catalog.md` with purpose, +- [ ] Any new seam added to `seam-catalog.md` with purpose, rules, and pivot trigger - [ ] No region code reaches directly into PropertyTable/mediator/LCModel outside a seam @@ -125,7 +125,7 @@ Use this when collapsing a large multi-view derisk branch into a landable PR (se ## Phase 10 — Retrospective (updates this skill set) - [ ] New patterns/gotchas/pivots recorded per the protocol in - `references/lessons-learned.md` + `lessons-learned.md` - [ ] New plugins added to the canonical examples in architecture-patterns.md §5 - [ ] Stale file pointers in any fieldworks-* skill fixed