From ea0bf43862f376a5848c3b6f360e8831faf55c16 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 20 May 2026 19:46:34 -0400 Subject: [PATCH 1/4] chore: add AI review customization workflows --- .github/context/codebase.context.md | 3 +- .../review-analyzer.instructions.md | 170 +++++++++++++ .github/prompts/challenge-pr-author.prompt.md | 9 + .../respond-to-review-comments.prompt.md | 22 ++ .github/skills/challenge-pr-author/SKILL.md | 231 ++++++++++++++++++ .github/skills/grill-with-docs/SKILL.md | 49 ++++ .../respond-to-review-comments/SKILL.md | 143 +++++++++++ CONTEXT.md | 99 ++++++++ 8 files changed, 725 insertions(+), 1 deletion(-) create mode 100644 .github/instructions/review-analyzer.instructions.md create mode 100644 .github/prompts/challenge-pr-author.prompt.md create mode 100644 .github/prompts/respond-to-review-comments.prompt.md create mode 100644 .github/skills/challenge-pr-author/SKILL.md create mode 100644 .github/skills/grill-with-docs/SKILL.md create mode 100644 .github/skills/respond-to-review-comments/SKILL.md create mode 100644 CONTEXT.md diff --git a/.github/context/codebase.context.md b/.github/context/codebase.context.md index e399dac48d..cd9f885ca9 100644 --- a/.github/context/codebase.context.md +++ b/.github/context/codebase.context.md @@ -2,8 +2,10 @@ Use these entry points to load context efficiently without scanning the entire repo. +- Shared language and glossary: `CONTEXT.md` - Onboarding: `.github/AGENTS.md` - Src catalog (overview of major folders): `.github/src-catalog.md` +- PR review response workflow: `.github/skills/respond-to-review-comments/SKILL.md` - Component guides: `Src//AGENTS.md` (and subfolder AGENTS.md where present) - Build system: `build.ps1`, `FieldWorks.proj`, `FieldWorks.sln`, `Build/InstallerBuild.proj` - Installer: `FLExInstaller/` @@ -15,4 +17,3 @@ Use these entry points to load context efficiently without scanning the entire r Tips - Prefer top-level scripts or FieldWorks.sln over ad-hoc project builds - Respect CI checks (commit messages, whitespace) before pushing - diff --git a/.github/instructions/review-analyzer.instructions.md b/.github/instructions/review-analyzer.instructions.md new file mode 100644 index 0000000000..e5ff2d3175 --- /dev/null +++ b/.github/instructions/review-analyzer.instructions.md @@ -0,0 +1,170 @@ +--- +name: "FieldWorks Review Analyzer" +description: "Use when performing PR or branch review analysis for FieldWorks. Defines required checks for contracts, managed UI, native interop, build/test, installer, localization, and validation evidence." +--- + +# FieldWorks Review Analyzer + +Use these instructions whenever Copilot analyzes a FieldWorks branch, PR, or diff for review. This is review policy, not an interactive workflow; the interactive author interview lives in the `challenge-pr-author` skill. + +FieldWorks is a Windows/x64 desktop application with .NET Framework 4.8 managed code, native C++, C++/CLI-adjacent boundaries, registration-free COM, WiX installer flows, `.resx` localization, and repo scripts for build/test validation. + +## Accuracy Requirement + +Before reporting any finding, verify it against actual code. Read the relevant file and, when necessary, compare against the before-state with `git show :`. Do not flag issues based only on file names, assumptions, or grep hits. + +A small number of accurate findings is better than a long list with errors. If you cannot confirm a concern from the actual code, omit it or explicitly note that it could not be verified. + +## Context to Apply + +Use the repository guidance that applies to changed files, especially: + +- `AGENTS.md` +- `.github/copilot-instructions.md` +- `.github/instructions/build.instructions.md` +- `.github/instructions/testing.instructions.md` +- `.github/instructions/managed.instructions.md` +- `.github/instructions/native.instructions.md` +- `.github/instructions/fieldworks-build-installer-review.instructions.md` +- `.github/instructions/fieldworks-interop-review.instructions.md` +- `.github/instructions/fieldworks-parser-utilities-review.instructions.md` +- `.github/instructions/fieldworks-ui-review.instructions.md` + +Load only the files needed for the changed areas. For structural or hidden-dependency risk, such as inheritance, interfaces, factories, DI/composition, native/managed boundaries, COM, parser pipelines, installer chains, or build graph changes, use symbol/reference/navigation tools if available. If not available, inspect definitions, callers, project references, and relevant grep results before concluding there is no dependency. + +## Severity Rubric + +- **Critical**: Blocks merge. Examples: correctness bug, security issue, native/managed boundary safety issue, breaking public contract or ABI change without compatibility plan, global COM registration/registry hack, build ordering break, CI silently skipping tests, installer behavior that can corrupt install/upgrade/uninstall. +- **Important**: Should fix before merge. Examples: missing meaningful tests for risky behavior, .NET Framework/C# 7.3 incompatibility, localization gap, legacy project file omission, likely UI crash path, installer/build validation gap, significant reuse/architecture concern. +- **Minor**: Nice to have. Examples: naming, small simplification, low-risk consistency issue, dependency bump note, documentation polish. + +When a verified finding's severity is unclear, prefer the higher severity and explain the risk. + +## FieldWorks Hot Spots + +Be especially careful when changes touch `Src/Utilities`, parser utilities, Allomorph Generator, interlinear display, installer/build workflows, dependency versions, WinForms click handlers, stale UI state, layout/display refresh, media-line behavior, or localization resources. Recent regressions in these areas often involve null checks, collection bounds, selection state, refresh/invalidation, layout anchoring, resource keys, and missing regression-test evidence. + +## Required Analysis Passes + +Run all four passes for PR/branch review. They may be done by separate read-only subagents or by Copilot directly, but every pass must be represented in the final analysis. + +### Pass 1: Contracts, Compatibility, and Correctness + +Analyze compatibility-relevant surfaces: + +- Public or protected C# types and members in changed assemblies, especially under `Src/` and shared libraries. +- Native headers, exported functions, `.def` files, COM interfaces, MIDL-generated interfaces, GUIDs, interface layout, vtable order, manifests, and registration-free COM configuration. +- Serialized data, project data, migration formats, settings/config schemas, XML formats, resource keys whose meaning changed, and file formats consumed across versions. +- PowerShell/MSBuild/script parameters and outputs that developers, CI, or installer workflows depend on. +- Installer product/upgrade codes, component IDs, feature layout, bundle/MSI command-line behavior, registry/file footprint, and upgrade/uninstall contracts. + +Steps: + +1. Get changed files with `git diff --name-only `. +2. For candidate contract files, compare before/after using `git show :` and current content. +3. Inspect callers/consumers when behavior or signatures change. +4. For legacy `.csproj` changes or new C# files, verify source files are explicitly included where required and test sources are not accidentally included in production projects. +5. For COM/native interfaces, verify GUIDs, layout, registration-free COM assumptions, and managed wrappers remain compatible or are explicitly coordinated. + +Flag Critical when confirmed: + +- Public C# API removal, parameter removal/reordering, type narrowing, behavior change for valid inputs, or undocumented default change that can break existing callers. +- COM interface, GUID, vtable, marshaling, or manifest change without a compatibility plan and validation. +- Native exported surface or ABI change that can break managed/native consumers. +- Serialized format or migration behavior change that risks existing projects. +- Build/test/installer script contract change that breaks documented repo workflows. + +Also read changed files and assess whether the implementation accomplishes the branch purpose. Flag incomplete implementations, stubs, new TODOs, dead code, null/bounds bugs, stale state, missed refresh/invalidation, behavior mismatches, security risks, and data-loss risks. + +### Pass 2: Managed UI, C#, and Localization + +Review changed managed files (`*.cs`, `*.csproj`, `*.resx`, `*.config`, `*.xaml`) and related UI resources. + +Check: + +- FieldWorks targets .NET Framework 4.8 and C# 7.3. Flag C# 8+ syntax/APIs such as nullable reference types, switch expressions, `using var`, ranges, records, target-typed `new`, file-scoped namespaces, or `required` members. +- Legacy `.csproj` files require explicit source includes. New `.cs` files must be included, and tests must not leak into production projects. +- NuGet/reference changes are coordinated with `Directory.Packages.props`, binding redirects, app configs, and .NET Framework compatibility. +- Event handlers and callbacks guard `sender`, selected items, selected indices, model objects, cast results, and collection bounds. +- Deleted/replaced model objects cannot remain in chooser lists, cached selections, mediator state, or reference collections. +- UI updates from background work marshal back to the UI thread. +- Interlinear, preview pane, dictionary, media-line, and tree/list display changes preserve refresh/invalidation, persistence, and selection behavior. +- Designer/layout changes preserve `TableLayoutPanel`, `FlowLayoutPanel`, anchoring, docking, minimum size, tab order, resizable dialogs, disposal, component initialization, and resource manager usage. +- User-visible UI strings belong in `.resx` resources, not hardcoded in C#. +- Resource keys are stable and descriptive. If a string's meaning changed, flag that a new key may be needed for Crowdin/localization safety. +- `.resx`, designer-generated resource accessors, and consuming code stay synchronized. + +### Pass 3: Native, COM, and Boundary Safety + +Review native and boundary files, especially `*.cpp`, `*.h`, `*.hpp`, `*.ixx`, `*.def`, `Src/Common/ViewsInterfaces/**`, `Src/views/**`, `Src/Generic/**`, `Src/Kernel/**`, and managed wrappers around native code. + +Check: + +- Untrusted input is validated before native, COM, file-system, installer, or process boundaries. +- String encoding conversions are explicit and do not rely on locale defaults. +- Buffer lengths, array counts, pointer ownership, lifetime, nullability, and out parameters are correct. +- RAII or deterministic cleanup is used for native resources. +- SAL annotations and checked APIs are used where existing patterns call for them. +- Exceptions do not cross managed/native boundaries; failures are translated through existing error-reporting patterns. +- COM GUID, interface layout, vtable order, manifest, or registration-free COM changes are explicit and compatible. +- No global COM registration or registry hacks are introduced. +- Managed wrapper changes remain consistent with native contracts. +- Native changes do not hide compiler/MSBuild warnings. +- Boundary failures produce actionable diagnostics without excessive normal logging. +- Native tests or managed boundary tests cover risky changes. + +### Pass 4: Build, Tests, CI, Dependencies, and Installer + +Review build scripts, project files, tests, CI workflows, package/dependency files, installer files, and validation evidence. + +Check: + +- Normal build/test validation uses `./build.ps1` and `./test.ps1`; avoid ad-hoc `msbuild`, `dotnet build`, `vstest.console`, or `nmake` unless debugging build infrastructure. +- Native C++ still builds before managed projects through `FieldWorks.proj` and `build.ps1` ordering. +- Build/test process cleanup remains scoped to the current worktree and does not terminate other worktrees' processes. +- CI changes do not skip tests or quality gates silently. +- Hardcoded absolute paths, machine-specific assumptions, hidden downloads, or untracked prerequisites are not introduced. +- Bug fixes, parser/morphology/interlinear/media-line/display changes, interop changes, installer changes, build workflow changes, and dependency alignment changes have meaningful validation evidence. +- Version changes in `Directory.Packages.props`, `Build/SilVersions.props`, app configs, binding redirects, local-library scripts, and installer/runtime packaging agree. +- New dependencies are compatible with .NET Framework 4.8 and Windows x64. +- WiX and installer changes preserve WiX 3 and WiX 6 flows unless explicitly scoped. +- Bundle/MSI install, upgrade, uninstall, logs, and expected footprint have validation evidence when behavior changes. +- PowerShell scripts propagate exit codes, quote paths with spaces, avoid unsafe pipelines where repo wrappers exist, and avoid name-based process termination. + +Do not mark manual smoke tests complete unless they were directly performed or the author explicitly confirms them. + +## Analysis Output Format + +Use this format for each pass or for the combined analyzer result: + +```markdown +## Contract/API Changes Summary + +[Factual list of additions, removals, and modifications to public or compatibility-relevant surfaces. Write "None." if no such surfaces changed.] + +## Findings + +### Critical (must address) + +- [ ] `path/to/file.ext`: Specific verified finding with explanation and suggested fix if applicable. + +### Important (should address) + +- [ ] `path/to/file.ext`: Specific verified finding with explanation. + +### Minor (consider) + +- [ ] `path/to/file.ext`: Specific verified finding. + +### Positive Observations + +- Things done well: good tests, safe boundary handling, localized strings, clear diagnostics, minimal scope, etc. + +### Required Validation + +- [ ] `./build.ps1` - needed because [reason] +- [ ] `./test.ps1 -TestProject "..."` - needed because [reason] +- [n/a] No additional validation beyond normal review identified. +``` + +If a section has no entries, write `None.` instead of leaving it blank. diff --git a/.github/prompts/challenge-pr-author.prompt.md b/.github/prompts/challenge-pr-author.prompt.md new file mode 100644 index 0000000000..210916735a --- /dev/null +++ b/.github/prompts/challenge-pr-author.prompt.md @@ -0,0 +1,9 @@ +--- +description: "Challenge a FieldWorks PR author before posting a PR; runs interactive branch review, writes .review/summary.md, and optionally prepares the PR." +agent: "agent" +argument-hint: "Optional branch purpose or PR goal" +--- + +Use the `challenge-pr-author` skill from `.github/skills/challenge-pr-author/SKILL.md` for the current branch. + +Treat any text supplied with this prompt as the optional branch purpose or PR goal. If no purpose is available, ask for it during the skill setup. Load `.github/instructions/review-analyzer.instructions.md` for the analysis policy, then follow the skill workflow through `.review/summary.md` creation. diff --git a/.github/prompts/respond-to-review-comments.prompt.md b/.github/prompts/respond-to-review-comments.prompt.md new file mode 100644 index 0000000000..8d609da1b2 --- /dev/null +++ b/.github/prompts/respond-to-review-comments.prompt.md @@ -0,0 +1,22 @@ +--- +description: "Respond to Copilot and human PR reviewer comments; fix sensible requests, ask about ambiguity, verify, commit, push, reply, resolve, and summarize." +agent: "agent" +argument-hint: "Optional PR URL, reviewer name, comment ID, or focus area" +--- + +Use the `respond-to-review-comments` skill from `.github/skills/respond-to-review-comments/SKILL.md`. + +Treat any text supplied with this prompt as optional input: a PR URL, PR number, reviewer name, comment ID, pasted review comments, or a focus area. + +Before changing files, load `CONTEXT.md` and use the `grill-with-docs` workflow to challenge ambiguous language in the review feedback. Load relevant staged docs and repo instructions before making decisions. + +Work through review comments as follows: + +1. Collect unresolved Copilot and human reviewer comments from the active PR, supplied PR details, or pasted comments. +2. Classify each comment as `Fix`, `Clarify`, `Reply only`, or `Defer`. +3. Fix sensible, unambiguous requests with minimal scoped changes. +4. Ask the user one focused question for ambiguous or risky requests. +5. Verify with the narrowest appropriate FieldWorks checks. +6. Commit and push only the intended changes, preserving unrelated user work. +7. Reply to each comment and resolve addressed threads where possible. +8. Report a summary of fixes, replies, unresolved comments, commit/push status, and verification. diff --git a/.github/skills/challenge-pr-author/SKILL.md b/.github/skills/challenge-pr-author/SKILL.md new file mode 100644 index 0000000000..ff9456de53 --- /dev/null +++ b/.github/skills/challenge-pr-author/SKILL.md @@ -0,0 +1,231 @@ +--- +name: challenge-pr-author +description: "Interactively challenge a FieldWorks PR author before posting a PR. Use for pre-PR review, author interview, branch review, PR readiness, review summary generation, and surfacing gaps in understanding or validation." +argument-hint: "Optional branch purpose or PR goal" +user-invocable: false +--- + +# Challenge PR Author + +Use this skill when the user wants an interactive pre-PR review that challenges the author, records their explanations, and writes `.review/summary.md`. + +The analyzer policy is in `.github/instructions/review-analyzer.instructions.md`; load and apply it for every analysis pass in this workflow. + +## Goals + +- Analyze the branch diff from `origin/main` using FieldWorks review policy. +- 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. + +## Start Here + +First tell the author what will happen: + +> **Here's what this review 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, and build/test/installer risk +> 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. + +## 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 `. + - 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." + +## 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. + +You may use read-only subagents for independent passes when available, but do not rely on a custom `review-analyzer` agent existing. If subagents are unavailable or would add friction, run the passes directly. + +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."] + +## Positive Observations + +[Positive observations, or "None."] + +## Interview Notes + +[Author explanations, decisions, unresolved items, and explicit lack-of-understanding notes.] + +## In-Review Quality Check + +[Only include if in-review changes were made.] + +## Suggested Review Focus + +- [ ] [High-priority review meeting agenda item] +- [ ] [High-priority review meeting agenda item] +``` + +## PR Offer + +After writing the summary, tell the author: + +> "Review summary written to `.review/summary.md`. +> +> Please review it, make changes where appropriate, and run `/challenge-pr-author` 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, using this summary as the description." + +Only create or update a PR after the author confirms. + +When creating or updating a PR, wrap the summary in: + +```markdown + +[summary] + +``` + +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. + +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. diff --git a/.github/skills/grill-with-docs/SKILL.md b/.github/skills/grill-with-docs/SKILL.md new file mode 100644 index 0000000000..852b25cb43 --- /dev/null +++ b/.github/skills/grill-with-docs/SKILL.md @@ -0,0 +1,49 @@ +--- +name: grill-with-docs +description: Grill the user against a repo-local CONTEXT.md, sharpen ambiguous terminology, ground terms in code, and update shared language before implementation. Use when shaping a feature, bugfix, refactor, or repo workflow in an existing codebase. +model: haiku +--- + + +You are a context-refinement agent. You interrogate fuzzy language until the user, the codebase, and the planning artifacts all use the same terms. + + + +You will receive: +- A proposed feature, bug, refactor, workflow, or planning topic +- A codebase or bounded context to inspect +- Existing shared-language docs, if any + + + +1. **Load context first** + - Look for a `CONTEXT.md` in the current folder or nearest parent context. + - If none exists, bootstrap one from the repo's docs, folder guides, and code terminology before continuing. +2. **Grill the language** + - Ask focused questions that expose ambiguous nouns, overloaded terms, missing relationships, and hidden constraints. + - Challenge wording that conflicts with the existing glossary or repo conventions. +3. **Ground terms in the codebase** + - Cross-reference code, docs, tests, and build files so terms match reality, not just conversation. + - Prefer established names that already appear in the repo over invented synonyms. +4. **Refine shared language** + - Update `CONTEXT.md` with clarified definitions, relationships, invariants, and open questions. + - Keep the document thin and practical: only include language that improves planning, naming, navigation, or implementation. +5. **Capture decision pressure** + - When a non-obvious choice is hard to reverse or surprising without explanation, record it under ADR candidates or the repo's ADR convention if one exists. +6. **Close with alignment** + - Summarize resolved terms, unresolved questions, and the next best step (for example `to-prd`, `tdd`, or implementation). + + + +- Ask one high-leverage question at a time when actively grilling the user. +- Do not let overloaded words like `project`, `app`, `model`, or `context` pass without disambiguation when precision matters. +- Prefer repo terms over generic replacements unless the repo term is itself the problem. +- Treat `CONTEXT.md` as a shared glossary plus invariants, not as a full architecture manual. +- If the repo contains multiple bounded contexts, keep the current edit scoped to the nearest relevant context and use the root `CONTEXT.md` only as a fallback. + + + +- Strong triggers: `grill me`, `clarify terminology`, `what should we call this`, `bootstrap CONTEXT.md`, `align language before implementation`, `sharpen the glossary`. +- In FieldWorks, likely sources of truth include `AGENTS.md`, `.github/context/codebase.context.md`, `.github/src-catalog.md`, `Docs/`, folder-level `AGENTS.md`, and the code symbols themselves. +- If the conversation reveals durable terminology changes, update `CONTEXT.md` before ending the session. + diff --git a/.github/skills/respond-to-review-comments/SKILL.md b/.github/skills/respond-to-review-comments/SKILL.md new file mode 100644 index 0000000000..29473c3c2a --- /dev/null +++ b/.github/skills/respond-to-review-comments/SKILL.md @@ -0,0 +1,143 @@ +--- +name: respond-to-review-comments +description: "Respond to Copilot and human PR reviewer comments: evaluate feedback, fix sensible requests, ask about ambiguity, verify, commit, push, reply to threads, resolve addressed comments, and summarize results. Use when handling review comments, Copilot review feedback, requested changes, unresolved PR threads, or reviewer follow-up." +argument-hint: "Optional PR URL, reviewer name, comment ID, or focus area" +--- + +# Respond to Review Comments + +Use this skill when the user wants a guided workflow for addressing Copilot or human reviewer comments on a FieldWorks pull request. + +This workflow starts from technical evaluation, not automatic agreement. External feedback is input to check against the codebase and repository rules. + +## Goals + +- Collect unresolved Copilot and human reviewer comments from the active pull request, a supplied PR URL, or comments pasted in chat. +- Decide whether each comment needs a code change, a reply, a clarifying question, or technical pushback. +- Fix sensible, unambiguous comments with minimal scoped edits. +- Ask the user before guessing when feedback is ambiguous, risky, contradictory, or policy-heavy. +- Run appropriate FieldWorks validation through repo scripts or narrow editor diagnostics. +- Commit and push the resulting code when it is ready. +- Reply to review comments, resolve threads when possible, and report a concise summary. + +## Required Context + +Before working comments: + +1. Load `CONTEXT.md` and apply the shared language rules. +2. Load the repo instructions relevant to touched files. +3. Use the `grill-with-docs` discipline for ambiguous terminology: challenge fuzzy terms, ground terms in code, and update `CONTEXT.md` if the workflow exposes durable language decisions. +4. Apply the code-review reception rule: verify feedback before implementing it. + +Useful repo anchors: + +- `AGENTS.md` +- `.github/context/codebase.context.md` +- `.github/instructions/navigation.instructions.md` +- `.github/instructions/testing.instructions.md` +- `.github/instructions/fieldworks-ui-review.instructions.md` +- `.github/instructions/fieldworks-interop-review.instructions.md` +- `.github/instructions/fieldworks-build-installer-review.instructions.md` + +## Comment Intake + +Use the richest available source, in this order: + +1. Active pull request tools in the editor or GitHub extension. +2. A PR URL, PR number, reviewer filter, or comment ID supplied by the user. +3. Review comments pasted into chat. + +Collect: + +- Unresolved inline review threads. +- Copilot review comments. +- Human reviewer comments and change requests. +- General PR comments that request action. + +If no comments are available, ask the user for the PR, comment text, or review thread details. + +## Classify Each Comment + +Build a short working ledger in chat, grouped by file or thread. For each comment, classify it as one of: + +- **Fix**: The request is technically sound, unambiguous, scoped, and compatible with FieldWorks conventions. +- **Clarify**: The request is ambiguous, too broad, conflicts with another requirement, or needs a product/architecture decision. +- **Reply only**: The request is already satisfied, incorrect for this codebase, a YAGNI expansion, or better handled by explanation. +- **Defer**: The request is valid but outside the current PR or requires a separate planned change. + +Ask the user one focused question before changing code for any **Clarify** item. Do not partially implement unclear multi-item feedback while unresolved ambiguity remains. + +## Fix Workflow + +For each **Fix** item: + +1. Read the comment, the referenced code, and enough nearby context to verify the issue. +2. Use symbol/reference navigation when the change crosses interfaces, inheritance, build graph, installer topology, native/managed boundaries, or parser/interlinear pipelines. +3. Apply the smallest change that satisfies the reviewer without unrelated refactoring. +4. Preserve FieldWorks rules: + - Use `build.ps1` and `test.ps1` for build/test work. + - Keep managed code compatible with .NET Framework 4.8 / C# 7.3. + - Keep user-visible strings in `.resx`. + - Preserve native-before-managed build order. + - Preserve registration-free COM behavior. +5. Record what changed for the eventual reply. + +## Verification + +Choose validation based on touched files and risk: + +- Documentation, prompt, instruction, or skill-only changes: run editor diagnostics on touched files and consider `CI: Whitespace check` before committing. +- Managed code changes: run the narrowest reliable `./test.ps1 -TestProject` or `./test.ps1 -TestFilter`, and `./build.ps1` when project/resource/build inputs changed. +- Native or interop changes: run `./test.ps1 -Native` or the relevant native test project, plus build validation when needed. +- Installer or PowerShell changes: run the relevant `Build/Agent` validation script and avoid ad-hoc pipelines. + +Do not claim manual validation unless it was directly performed or the user explicitly confirms it. + +## Git Workflow + +Before staging or committing: + +1. Run `git status --short`. +2. Identify pre-existing user changes and changes made by this workflow. +3. Stage only the intended files explicitly. Do not use broad staging if unrelated or staged-but-deleted files are present. +4. If unrelated changes are already staged, ask whether to include them in the review-comment commit or leave them staged for a separate commit. + +Commit after fixes are validated and no ambiguity remains. Use a concise message that describes the review-response work, for example: + +```text +fix: address PR review comments +``` + +Push the current branch after committing. If push requires credentials or a force push, stop and ask the user; do not force-push without explicit approval. + +## Reply and Resolve + +For each thread or comment: + +- **Fixed**: Reply with the concrete change and verification result. +- **Reply only**: Reply with concise technical reasoning. +- **Clarify**: Ask the specific question needed to proceed. +- **Deferred**: State why it belongs in follow-up work. + +Resolve a review thread only when all of these are true: + +- The thread is unresolved. +- The tool/API reports it can be resolved. +- The code change or reply fully addresses the comment. +- There is no open question for the user or reviewer. + +Do not resolve threads that are ambiguous, disputed, blocked by missing verification, or intentionally deferred. + +When using GitHub APIs directly, reply to inline review comments in their thread, not as unrelated top-level PR comments. + +## Final Report + +End with: + +- Comments fixed, grouped by reviewer/thread. +- Comments replied to without code changes and why. +- Comments left unresolved and the blocker. +- Commit SHA and pushed branch, if committed and pushed. +- Verification commands run and any gaps. + +Keep the report concise, but include enough detail for the user to see which comments still need attention. diff --git a/CONTEXT.md b/CONTEXT.md new file mode 100644 index 0000000000..cfb72c5a42 --- /dev/null +++ b/CONTEXT.md @@ -0,0 +1,99 @@ +# FieldWorks Context + +This file captures the thin layer of shared language that helps humans and agents talk about this repository without re-explaining the same concepts every session. + +It is intentionally not a full architecture manual. It should stay biased toward terminology, relationships, invariants, and naming choices that affect planning, navigation, implementation, and reviews. + +## Scope + +- This is the root context for the whole repository. +- It covers repo-wide language shared across product, code, build, test, and installer work. +- If a subtree develops materially different language, add a more local `CONTEXT.md` there instead of bloating this file. + +## Canonical Product Terms + +- **FieldWorks**: The preferred default name for the product suite, the repository, and root-level planning language. +- **FLEx**: Legacy shorthand for **FieldWorks Language Explorer**. Use it when matching existing code, folders, UI text, integrations, or historical documentation, not as the default root-level product term. +- **Language Explorer**: The spelled-out legacy product name behind FLEx that still appears in repo paths, UI assets, and integrations. +- **FieldWorks.exe**: The current host executable for the main application shell. + +## Disambiguation Rules + +- **Treat `project` as requiring a qualifier almost always.** The word is overloaded in this repo. +- **Language project**: A user/data project inside FieldWorks or FLEx. +- **Git repository** or **repo**: The source tree you are editing now. +- **MSBuild project**: A `.csproj`, `.vcxproj`, `.wixproj`, or similar build unit. +- **Installer project**: WiX authoring and packaging work under `FLExInstaller/`. +- **Worktree**: A git worktree for isolated builds and edits. + +## Core Domain and Product Language + +- **Language project**: A user-managed linguistic dataset opened and edited in FieldWorks. +- **Writing system**: A configured language/script/orthography used to store and display text. +- **Vernacular writing system**: A writing system used for source-language data. +- **Analysis writing system**: A writing system used for glosses, definitions, translated labels, and analysis-oriented text. +- **Lexicon**: The lexical data and editing experience in FLEx. +- **Interlinear text**: Text annotated with multiple aligned linguistic analysis lines. +- **Morphology**: The part of the system and data model concerned with morphemes, rules, and word analysis. +- **Parser**: Morphological analysis tooling such as HermitCrab or XAmple. +- **Paratext integration**: Scripture and lexicon interoperability with Paratext. +- **Send/Receive**: The user-facing synchronization workflow for sharing project data. +- **FLEx Bridge**: The tool/integration layer used by Send/Receive and related LIFT-based exchange workflows. + +## Architecture and Codebase Language + +- **LCModel**: The underlying managed data model, caches, services, and related packages used for FieldWorks data access. +- **xWorks**: The shared application framework and shell infrastructure that hosts major work areas. +- **LexText**: The FLEx application layer and related lexicon/text-analysis functionality. +- **Common**: Shared infrastructure, controls, dialogs, utilities, and framework code used across the suite. +- **FwKernel** and **Views**: Native rendering and view infrastructure. +- **ViewsInterfaces**: The managed interface layer generated from native IDL and used across the managed/native boundary. +- **Traversal build**: The ordered build driven by `FieldWorks.proj` and invoked via `build.ps1`. +- **DistFiles**: Runtime assets copied into outputs or installers. + +## Repo-Wide Invariants + +- Native C++ builds before managed code generation and managed projects. +- `build.ps1` is the canonical build entry point. +- `test.ps1` is the canonical test entry point. +- Registration-free COM is a core deployment/runtime assumption; do not introduce global COM registration behavior. +- User-visible UI strings belong in `.resx`, not hardcoded source. +- Installer work lives under `FLExInstaller/`. +- Integration tests often depend on deterministic sample data such as `TestLangProj/`. +- Worktree-aware scripts are preferred because concurrent work across git worktrees is supported. + +## Key Relationships + +- FieldWorks the repository contains the FLEx application, supporting tools, shared libraries, installer authoring, and docs. +- FLEx/Language Explorer is built on shared infrastructure such as xWorks, Common, LCModel, and the native Views/FwKernel stack. +- Native build artifacts feed managed code generation through ViewsInterfaces. +- Send/Receive is a workflow; FLEx Bridge is the underlying integration/tooling layer behind that workflow. +- Writing systems are first-class project configuration, with vernacular and analysis writing systems playing different roles. + +## Good Naming Pressure + +- Prefer established repo names over generic synonyms. +- If a change concerns user data, say **language project**, not just **project**. +- If a change concerns the source tree, say **repo**, **worktree**, or the specific build/test project. +- If a change touches synchronization, distinguish the user concept (**Send/Receive**) from the implementation/tooling concept (**FLEx Bridge**) unless the distinction is intentionally irrelevant. +- If a change touches the managed/native boundary, call out **ViewsInterfaces**, **Views**, **FwKernel**, or **registration-free COM** explicitly. + +## Review Workflow Language + +- **Review comment**: Any actionable feedback from Copilot or a human reviewer on a pull request. +- **Review thread**: A GitHub inline conversation anchored to code. Resolve only after the thread is fully addressed and no question remains. +- **Copilot reviewer comment**: Automated review feedback from GitHub Copilot. Evaluate it like external reviewer feedback; do not treat it as authoritative without checking the code. +- **Human reviewer comment**: Feedback from a person. Treat it seriously, but still verify the requested change against FieldWorks conventions and existing behavior. +- **Sensible fix**: A reviewer request that is technically sound, unambiguous, scoped, and compatible with repo rules. +- **Ambiguous feedback**: A reviewer request that lacks enough context, conflicts with another requirement, or would require a product or architecture decision. Ask the user before changing code. +- **Reply**: A response in the review conversation explaining a fix, asking a question, or giving technical reasoning for no code change. +- **Resolve**: Marking a review thread addressed in GitHub. Do this only when the code or reply fully answers the thread. + +## Remaining Open Question + +- Should this root file stay mixed product-plus-architecture, or should lower-level developer terms move into narrower subtree contexts later? + +## ADR Candidates + +- No repository-wide ADR location is established yet. +- If a terminology decision becomes hard to reverse or affects naming across many files, record it here first and promote it to a formal ADR only if the repo adopts a dedicated ADR convention. From 6dc95bac2a2cfc2c80b465d35abc2e21b8b417fd Mon Sep 17 00:00:00 2001 From: John Lambert Date: Thu, 21 May 2026 13:47:44 -0400 Subject: [PATCH 2/4] Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .github/context/codebase.context.md | 2 +- .github/skills/challenge-pr-author/SKILL.md | 2 +- .github/skills/respond-to-review-comments/SKILL.md | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/context/codebase.context.md b/.github/context/codebase.context.md index cd9f885ca9..2def04835a 100644 --- a/.github/context/codebase.context.md +++ b/.github/context/codebase.context.md @@ -3,7 +3,7 @@ Use these entry points to load context efficiently without scanning the entire repo. - Shared language and glossary: `CONTEXT.md` -- Onboarding: `.github/AGENTS.md` +- Onboarding (canonical anchor): `.github/AGENTS.md` - Src catalog (overview of major folders): `.github/src-catalog.md` - PR review response workflow: `.github/skills/respond-to-review-comments/SKILL.md` - Component guides: `Src//AGENTS.md` (and subfolder AGENTS.md where present) diff --git a/.github/skills/challenge-pr-author/SKILL.md b/.github/skills/challenge-pr-author/SKILL.md index ff9456de53..96d9faf797 100644 --- a/.github/skills/challenge-pr-author/SKILL.md +++ b/.github/skills/challenge-pr-author/SKILL.md @@ -2,7 +2,7 @@ name: challenge-pr-author description: "Interactively challenge a FieldWorks PR author before posting a PR. Use for pre-PR review, author interview, branch review, PR readiness, review summary generation, and surfacing gaps in understanding or validation." argument-hint: "Optional branch purpose or PR goal" -user-invocable: false +user-invocable: true --- # Challenge PR Author diff --git a/.github/skills/respond-to-review-comments/SKILL.md b/.github/skills/respond-to-review-comments/SKILL.md index 29473c3c2a..b4c25865ff 100644 --- a/.github/skills/respond-to-review-comments/SKILL.md +++ b/.github/skills/respond-to-review-comments/SKILL.md @@ -22,7 +22,7 @@ This workflow starts from technical evaluation, not automatic agreement. Externa ## Required Context -Before working comments: +Before working on comments: 1. Load `CONTEXT.md` and apply the shared language rules. 2. Load the repo instructions relevant to touched files. From fa7afd01323d208d880e95681829c62bc98b657c Mon Sep 17 00:00:00 2001 From: John Lambert Date: Thu, 21 May 2026 14:49:45 -0400 Subject: [PATCH 3/4] chore: refine AI review workflows --- .github/context/codebase.context.md | 4 +- .github/copilot-instructions.md | 5 +- .../dotnet-framework.instructions.md | 70 +++++++++---------- .../review-analyzer.instructions.md | 14 +++- .github/prompts/challenge-pr-author.prompt.md | 9 --- .github/prompts/pr-preflight.prompt.md | 9 +++ .github/skills/execute-implement/SKILL.md | 1 + .../skills/openspec-continue-change/SKILL.md | 2 + .github/skills/openspec-explore/SKILL.md | 2 + .github/skills/openspec-ff-change/SKILL.md | 2 + .github/skills/openspec-new-change/SKILL.md | 2 + .../skills/openspec-propose-change/SKILL.md | 4 +- .github/skills/plan-design/SKILL.md | 2 + .../SKILL.md | 44 +++++++++--- .../respond-to-review-comments/SKILL.md | 2 +- CONTEXT.md | 3 + Src/AGENTS.md | 4 +- 17 files changed, 114 insertions(+), 65 deletions(-) delete mode 100644 .github/prompts/challenge-pr-author.prompt.md create mode 100644 .github/prompts/pr-preflight.prompt.md rename .github/skills/{challenge-pr-author => pr-preflight}/SKILL.md (76%) diff --git a/.github/context/codebase.context.md b/.github/context/codebase.context.md index 2def04835a..3e017168ab 100644 --- a/.github/context/codebase.context.md +++ b/.github/context/codebase.context.md @@ -5,7 +5,8 @@ Use these entry points to load context efficiently without scanning the entire r - Shared language and glossary: `CONTEXT.md` - Onboarding (canonical anchor): `.github/AGENTS.md` - Src catalog (overview of major folders): `.github/src-catalog.md` -- PR review response workflow: `.github/skills/respond-to-review-comments/SKILL.md` +- AI review workflow: `grill-with-docs` for planning language, `review-analyzer.instructions.md` for review policy, `pr-preflight` for branch/PR readiness, and `respond-to-review-comments` for reviewer feedback. +- Specialist review agents: `.github/agents/fieldworks.csharp-expert.agent.md`, `.github/agents/fieldworks.winforms-expert.agent.md`, `.github/agents/fieldworks.cpp-expert.agent.md`, and `.github/agents/fieldworks.avalonia-expert.agent.md`. - Component guides: `Src//AGENTS.md` (and subfolder AGENTS.md where present) - Build system: `build.ps1`, `FieldWorks.proj`, `FieldWorks.sln`, `Build/InstallerBuild.proj` - Installer: `FLExInstaller/` @@ -17,3 +18,4 @@ Use these entry points to load context efficiently without scanning the entire r Tips - Prefer top-level scripts or FieldWorks.sln over ad-hoc project builds - Respect CI checks (commit messages, whitespace) before pushing +- Prefer FieldWorks-specific agents over generic language/framework agents when reviewing this repo diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 5cc5108cd5..b378214a8b 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -16,8 +16,9 @@ desktop application. validation, encoding assumptions, buffer lengths, and ownership rules. - Verify user-visible UI strings are placed in `.resx` resources and remain compatible with Crowdin localization. -- For C# code, flag C# 8+ syntax and APIs that are not compatible with .NET - Framework 4.8 / C# 7.3. +- For C# code, flag language features newer than the repo default C# 8.0, + nullable-reference-type assumptions when nullable is disabled, and APIs that + are not compatible with .NET Framework 4.8. - For legacy `.csproj` changes, verify new source files are explicitly included and test sources are explicitly excluded where production and test trees mix. - Require meaningful test or validation evidence for bug fixes, regressions, diff --git a/.github/instructions/dotnet-framework.instructions.md b/.github/instructions/dotnet-framework.instructions.md index 9b796f612a..5555fbf8eb 100644 --- a/.github/instructions/dotnet-framework.instructions.md +++ b/.github/instructions/dotnet-framework.instructions.md @@ -6,50 +6,52 @@ applyTo: '**/*.csproj, **/*.cs' # .NET Framework Development ## Build and Compilation Requirements -- Always use `msbuild /t:rebuild` to build the solution or projects instead of `dotnet build` +- Use `./build.ps1` for normal builds and `./test.ps1` for normal test runs. +- Do not use `dotnet build` for the main repo workflow. +- Avoid direct `msbuild` or project-only builds unless you are explicitly debugging build infrastructure. ## Project File Management -### Non-SDK Style Project Structure -.NET Framework projects use the legacy project format, which differs significantly from modern SDK-style projects: +### SDK-style net48 projects are common +Most managed projects in this repo use SDK-style `.csproj` files while still targeting .NET Framework `net48`. -- **Explicit File Inclusion**: All new source files **MUST** be explicitly added to the project file (`.csproj`) using a `` element - - .NET Framework projects do not automatically include files in the directory like SDK-style projects - - Example: `` +- **Implicit file inclusion is common**: SDK-style projects usually include new `.cs` files automatically. +- **Check the touched project before editing**: Some projects still carry explicit includes, linked files, designer items, generated code, or custom metadata that must be preserved. +- **Assembly metadata is managed explicitly**: Keep `GenerateAssemblyInfo` disabled where the project links `CommonAssemblyInfo.cs`. +- **Preserve project-specific settings**: Keep existing settings for x64, WinExe, WindowsDesktop, COM, resources, warnings-as-errors, and custom MSBuild targets. +- **Do not normalize projects unnecessarily**: Avoid converting project structure, import style, or target declarations unless that is the task. -- **No Implicit Imports**: Unlike SDK-style projects, .NET Framework projects do not automatically import common namespaces or assemblies - -- **Build Configuration**: Contains explicit `` sections for Debug/Release configurations +### Legacy project caveat +Some non-SDK-style or tool-specific projects may still exist. When you encounter one: -- **Output Paths**: Explicit `` and `` definitions - -- **Target Framework**: Uses `` instead of `` - - Example: `v4.7.2` +- Keep explicit `` items and other legacy structure intact. +- Update the project file only as much as the change requires. +- Do not assume you can migrate it to SDK-style as part of routine code edits. ## NuGet Package Management - Installing and updating NuGet packages in .NET Framework projects is a complex task requiring coordinated changes to multiple files. Therefore, **do not attempt to install or update NuGet packages** in this project. - Instead, if changes to NuGet references are required, ask the user to install or update NuGet packages using the Visual Studio NuGet Package Manager or Visual Studio package manager console. - When recommending NuGet packages, ensure they are compatible with .NET Framework or .NET Standard 2.0 (not only .NET Core or .NET 5+). -## C# Language Version is 7.3 -- This project is limited to C# 7.3 features only. Please avoid using: +## C# Language Version +- The repo-wide default language version for C# projects is 8.0 via `Directory.Build.props`. +- Do not introduce features that require a newer language version unless the specific project or repo policy is updated first. +- Prefer syntax already used in the touched area so edits remain consistent with surrounding code. + +### C# 8.0 features available by default +- Switch expressions and pattern matching enhancements. +- Using declarations where disposal scope remains clear. +- Null-coalescing assignment and range/index operators when they improve clarity. -### C# 8.0+ Features (NOT SUPPORTED): - - Using declarations (`using var stream = ...`) - - Await using statements (`await using var resource = ...`) - - Switch expressions (`variable switch { ... }`) - - Null-coalescing assignment (`??=`) - - Range and index operators (`array[1..^1]`, `array[^1]`) - - Default interface methods - - Readonly members in structs - - Static local functions - - Nullable reference types (`string?`, `#nullable enable`) +### Features not safe to assume repo-wide +- Nullable reference types are not enabled repo-wide. Do not introduce `string?`, `#nullable enable`, or nullable-flow assumptions unless the project explicitly opts in. +- File-scoped namespaces are not available under the repo default language version and should not be introduced. ### C# 9.0+ Features (NOT SUPPORTED): - Records (`public record Person(string Name)`) - Init-only properties (`{ get; init; }`) - Top-level programs (program without Main method) - - Pattern matching enhancements + - Pattern matching enhancements beyond C# 8 - Target-typed new expressions (`List list = new()`) ### C# 10+ Features (NOT SUPPORTED): @@ -58,17 +60,15 @@ applyTo: '**/*.csproj, **/*.cs' - Record structs - Required members -### Use Instead (C# 7.3 Compatible): - - Traditional using statements with braces - - Switch statements instead of switch expressions - - Explicit null checks instead of null-coalescing assignment - - Array slicing with manual indexing - - Abstract classes or interfaces instead of default interface methods +### Use instead when staying within repo defaults + - Block-scoped namespaces + - Explicit null checks unless the project has enabled nullable analysis + - Established project patterns over newer syntax for its own sake ## Environment Considerations (Windows environment) -- Use Windows-style paths with backslashes (e.g., `C:\path\to\file.cs`) -- Use Windows-appropriate commands when suggesting terminal operations -- Consider Windows-specific behaviors when working with file system operations +- FieldWorks builds and managed test runs are Windows-only. +- On non-Windows hosts, limit work to editing, code search, docs, and specs. +- When suggesting build or test commands, prefer the repo PowerShell scripts and Windows-oriented workflows. ## Common .NET Framework Pitfalls and Best Practices diff --git a/.github/instructions/review-analyzer.instructions.md b/.github/instructions/review-analyzer.instructions.md index e5ff2d3175..b5a86d3770 100644 --- a/.github/instructions/review-analyzer.instructions.md +++ b/.github/instructions/review-analyzer.instructions.md @@ -5,7 +5,7 @@ description: "Use when performing PR or branch review analysis for FieldWorks. D # FieldWorks Review Analyzer -Use these instructions whenever Copilot analyzes a FieldWorks branch, PR, or diff for review. This is review policy, not an interactive workflow; the interactive author interview lives in the `challenge-pr-author` skill. +Use these instructions whenever Copilot analyzes a FieldWorks branch, PR, or diff for review. This is review policy, not an interactive workflow; the interactive author interview lives in the `pr-preflight` skill. FieldWorks is a Windows/x64 desktop application with .NET Framework 4.8 managed code, native C++, C++/CLI-adjacent boundaries, registration-free COM, WiX installer flows, `.resx` localization, and repo scripts for build/test validation. @@ -35,7 +35,7 @@ Load only the files needed for the changed areas. For structural or hidden-depen ## Severity Rubric - **Critical**: Blocks merge. Examples: correctness bug, security issue, native/managed boundary safety issue, breaking public contract or ABI change without compatibility plan, global COM registration/registry hack, build ordering break, CI silently skipping tests, installer behavior that can corrupt install/upgrade/uninstall. -- **Important**: Should fix before merge. Examples: missing meaningful tests for risky behavior, .NET Framework/C# 7.3 incompatibility, localization gap, legacy project file omission, likely UI crash path, installer/build validation gap, significant reuse/architecture concern. +- **Important**: Should fix before merge. Examples: missing meaningful tests for risky behavior, .NET Framework/C# language policy incompatibility, nullable-reference-type policy mismatch, localization gap, legacy project file omission, likely UI crash path, installer/build validation gap, significant reuse/architecture concern. - **Minor**: Nice to have. Examples: naming, small simplification, low-risk consistency issue, dependency bump note, documentation polish. When a verified finding's severity is unclear, prefer the higher severity and explain the risk. @@ -48,6 +48,13 @@ Be especially careful when changes touch `Src/Utilities`, parser utilities, Allo Run all four passes for PR/branch review. They may be done by separate read-only subagents or by Copilot directly, but every pass must be represented in the final analysis. +When specialist agents are available, use them as focused reviewers for matching areas and synthesize their output against this policy: + +- `FieldWorks C# Expert` for managed code, project files, config, net48 behavior, and test failures. +- `FieldWorks WinForms Expert` for WinForms designer, layout, event-handler, UI-thread, resource, and localization changes. +- `FieldWorks C++ Expert` for native, COM, C++/CLI-adjacent, Views, FwKernel, ViewsInterfaces, ABI, and memory-safety changes. +- `FieldWorks Avalonia UI Expert` only for Avalonia/XAML changes. + ### Pass 1: Contracts, Compatibility, and Correctness Analyze compatibility-relevant surfaces: @@ -82,7 +89,8 @@ Review changed managed files (`*.cs`, `*.csproj`, `*.resx`, `*.config`, `*.xaml` Check: -- FieldWorks targets .NET Framework 4.8 and C# 7.3. Flag C# 8+ syntax/APIs such as nullable reference types, switch expressions, `using var`, ranges, records, target-typed `new`, file-scoped namespaces, or `required` members. +- FieldWorks targets .NET Framework 4.8 and defaults C# projects to C# 8.0 via `Directory.Build.props`, with nullable reference types disabled by default. Flag features that require C# 9+ or nullable-flow assumptions unless a specific project explicitly opts in. +- C# 8 syntax such as switch expressions, using declarations, null-coalescing assignment, and range/index operators is allowed by the repo default, but should still match the surrounding code style. Do not introduce file-scoped namespaces, records, init-only properties, target-typed `new`, global usings, `required` members, `string?`, or `#nullable enable` unless the project policy changes first. - Legacy `.csproj` files require explicit source includes. New `.cs` files must be included, and tests must not leak into production projects. - NuGet/reference changes are coordinated with `Directory.Packages.props`, binding redirects, app configs, and .NET Framework compatibility. - Event handlers and callbacks guard `sender`, selected items, selected indices, model objects, cast results, and collection bounds. diff --git a/.github/prompts/challenge-pr-author.prompt.md b/.github/prompts/challenge-pr-author.prompt.md deleted file mode 100644 index 210916735a..0000000000 --- a/.github/prompts/challenge-pr-author.prompt.md +++ /dev/null @@ -1,9 +0,0 @@ ---- -description: "Challenge a FieldWorks PR author before posting a PR; runs interactive branch review, writes .review/summary.md, and optionally prepares the PR." -agent: "agent" -argument-hint: "Optional branch purpose or PR goal" ---- - -Use the `challenge-pr-author` skill from `.github/skills/challenge-pr-author/SKILL.md` for the current branch. - -Treat any text supplied with this prompt as the optional branch purpose or PR goal. If no purpose is available, ask for it during the skill setup. Load `.github/instructions/review-analyzer.instructions.md` for the analysis policy, then follow the skill workflow through `.review/summary.md` creation. diff --git a/.github/prompts/pr-preflight.prompt.md b/.github/prompts/pr-preflight.prompt.md new file mode 100644 index 0000000000..6607873667 --- /dev/null +++ b/.github/prompts/pr-preflight.prompt.md @@ -0,0 +1,9 @@ +--- +description: "Run FieldWorks PR preflight for the current branch: review readiness, author interview, validation evidence, .review/summary.md, and optional PR preparation." +agent: "agent" +argument-hint: "Optional branch purpose or PR goal" +--- + +Use the `pr-preflight` skill from `.github/skills/pr-preflight/SKILL.md` for the current branch. + +Treat any text supplied with this prompt as the optional branch purpose or PR goal. If no purpose is available, ask for it during the skill setup. Load `CONTEXT.md`, `.github/context/codebase.context.md`, and `.github/instructions/review-analyzer.instructions.md`, then follow the skill workflow through `.review/summary.md` creation. diff --git a/.github/skills/execute-implement/SKILL.md b/.github/skills/execute-implement/SKILL.md index ffd9226297..3d33158ffa 100644 --- a/.github/skills/execute-implement/SKILL.md +++ b/.github/skills/execute-implement/SKILL.md @@ -24,6 +24,7 @@ You will receive: - Apply the minimal change set needed to satisfy acceptance signals. 3. **Self-check** - Review for style, safety, and regression risk. + - For FieldWorks branches that will become PRs, use the review policy in `.github/instructions/review-analyzer.instructions.md` or run `pr-preflight` before handoff when risk or scope warrants it. 4. **Update status** - Record what changed and any follow-up notes. diff --git a/.github/skills/openspec-continue-change/SKILL.md b/.github/skills/openspec-continue-change/SKILL.md index 4f2c3dcf4b..3656ad630b 100644 --- a/.github/skills/openspec-continue-change/SKILL.md +++ b/.github/skills/openspec-continue-change/SKILL.md @@ -40,6 +40,8 @@ Continue working on a change by creating the next artifact. 3. **Act based on status**: + Before creating proposal, design, or task artifacts from ambiguous user language, apply `grill-with-docs` so new artifact wording uses clarified repo terms from `CONTEXT.md`. + --- **If all artifacts are complete (`isComplete: true`)**: diff --git a/.github/skills/openspec-explore/SKILL.md b/.github/skills/openspec-explore/SKILL.md index 6858d3f693..cfcf593152 100644 --- a/.github/skills/openspec-explore/SKILL.md +++ b/.github/skills/openspec-explore/SKILL.md @@ -13,6 +13,8 @@ Enter explore mode. Think deeply. Visualize freely. Follow the conversation wher **IMPORTANT: Explore mode is for thinking, not implementing.** You may read files, search code, and investigate the codebase, but you must NEVER write code or implement features. If the user asks you to implement something, remind them to exit explore mode first and create a change proposal. You MAY create OpenSpec artifacts (proposals, designs, specs) if the user asks—that's capturing thinking, not implementing. +When exploration turns into planning/spec markdown or exposes overloaded repo terms, apply `grill-with-docs` before capturing durable artifact wording so OpenSpec text matches `CONTEXT.md` and real codebase terminology. + **This is a stance, not a workflow.** There are no fixed steps, no required sequence, no mandatory outputs. You're a thinking partner helping the user explore. --- diff --git a/.github/skills/openspec-ff-change/SKILL.md b/.github/skills/openspec-ff-change/SKILL.md index 43f2632ea4..5819bf4438 100644 --- a/.github/skills/openspec-ff-change/SKILL.md +++ b/.github/skills/openspec-ff-change/SKILL.md @@ -24,6 +24,8 @@ Fast-forward through artifact creation - generate everything needed to start imp **IMPORTANT**: Do NOT proceed without understanding what the user wants to build. + If the request uses overloaded repo terms or unclear planning language, apply `grill-with-docs` before creating artifacts so proposal/spec/design/task wording matches `CONTEXT.md` and real codebase terminology. + 2. **Create the change directory** ```bash openspec new change "" diff --git a/.github/skills/openspec-new-change/SKILL.md b/.github/skills/openspec-new-change/SKILL.md index 1af41c7d73..6688dcb5de 100644 --- a/.github/skills/openspec-new-change/SKILL.md +++ b/.github/skills/openspec-new-change/SKILL.md @@ -24,6 +24,8 @@ Start a new change using the experimental artifact-driven approach. **IMPORTANT**: Do NOT proceed without understanding what the user wants to build. + If the request uses overloaded repo terms or unclear planning language, apply `grill-with-docs` before selecting names or creating artifacts so OpenSpec wording matches `CONTEXT.md` and real codebase terminology. + 2. **Determine the workflow schema** Use the default schema (omit `--schema`) unless the user explicitly requests a different workflow. diff --git a/.github/skills/openspec-propose-change/SKILL.md b/.github/skills/openspec-propose-change/SKILL.md index 44743f354b..5992f35c06 100644 --- a/.github/skills/openspec-propose-change/SKILL.md +++ b/.github/skills/openspec-propose-change/SKILL.md @@ -26,6 +26,8 @@ This is the preferred user-facing alias of the fast-forward workflow. **IMPORTANT**: Do NOT proceed without understanding what the user wants to build. + If the request uses overloaded repo terms or unclear planning language, apply `grill-with-docs` before creating artifacts so proposal/spec/design wording matches `CONTEXT.md` and real codebase terminology. + 2. **Create the change directory** ```bash openspec new change "" @@ -100,4 +102,4 @@ After completing all artifacts, summarize: - Always read dependency artifacts before creating a new one - If context is critically unclear, ask the user - but prefer making reasonable decisions to keep momentum - If a change with that name already exists, suggest continuing that change instead -- Verify each artifact file exists after writing before proceeding to next \ No newline at end of file +- Verify each artifact file exists after writing before proceeding to next diff --git a/.github/skills/plan-design/SKILL.md b/.github/skills/plan-design/SKILL.md index 8a8dd5575b..4d24e6363b 100644 --- a/.github/skills/plan-design/SKILL.md +++ b/.github/skills/plan-design/SKILL.md @@ -18,6 +18,7 @@ You will receive: 1. **Understand the issue** - Restate scope and expected outcome. + - If the request uses overloaded repo terms, planning/spec language, or unclear ownership boundaries, apply `grill-with-docs` before finalizing the plan. 2. **Identify constraints and risks** - Note dependencies, compatibility requirements, and edge cases. 3. **Design the approach** @@ -30,6 +31,7 @@ You will receive: - Keep scope limited to the issue. - Record assumptions explicitly. - Avoid implementation details that belong in execution. +- Use clarified terms from `CONTEXT.md`; update that context only when the conversation creates durable shared language. diff --git a/.github/skills/challenge-pr-author/SKILL.md b/.github/skills/pr-preflight/SKILL.md similarity index 76% rename from .github/skills/challenge-pr-author/SKILL.md rename to .github/skills/pr-preflight/SKILL.md index 96d9faf797..93c474fdfe 100644 --- a/.github/skills/challenge-pr-author/SKILL.md +++ b/.github/skills/pr-preflight/SKILL.md @@ -1,19 +1,20 @@ --- -name: challenge-pr-author -description: "Interactively challenge a FieldWorks PR author before posting a PR. Use for pre-PR review, author interview, branch review, PR readiness, review summary generation, and surfacing gaps in understanding or validation." +name: pr-preflight +description: "Use when preparing a FieldWorks branch or pull request for review: pre-PR review, branch readiness, author interview, review summary generation, validation evidence, or PR description preparation." argument-hint: "Optional branch purpose or PR goal" user-invocable: true --- -# Challenge PR Author +# PR Preflight -Use this skill when the user wants an interactive pre-PR review that challenges the author, records their explanations, and writes `.review/summary.md`. +Use this skill when the user wants an interactive FieldWorks branch review before posting or updating a PR. -The analyzer policy is in `.github/instructions/review-analyzer.instructions.md`; load and apply it for every analysis pass in this workflow. +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. @@ -23,10 +24,10 @@ The analyzer policy is in `.github/instructions/review-analyzer.instructions.md` First tell the author what will happen: -> **Here's what this review will do:** +> **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, and build/test/installer risk +> 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 > @@ -54,6 +55,17 @@ First tell the author what will happen: - 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: + +- 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. + ## Analysis Load `.github/instructions/review-analyzer.instructions.md` and run all four required passes: @@ -63,7 +75,17 @@ Load `.github/instructions/review-analyzer.instructions.md` and run all four req - Native, COM, and boundary safety. - Build, tests, CI, dependencies, and installer. -You may use read-only subagents for independent passes when available, but do not rely on a custom `review-analyzer` agent existing. If subagents are unavailable or would add friction, run the passes directly. +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: @@ -212,7 +234,7 @@ After writing the summary, tell the author: > "Review summary written to `.review/summary.md`. > -> Please review it, make changes where appropriate, and run `/challenge-pr-author` again until you are ready to post the PR. +> 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, using this summary as the description." @@ -221,9 +243,9 @@ Only create or update a PR after the author confirms. When creating or updating a PR, wrap the summary in: ```markdown - + [summary] - + ``` 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. diff --git a/.github/skills/respond-to-review-comments/SKILL.md b/.github/skills/respond-to-review-comments/SKILL.md index b4c25865ff..54f4fc5ce9 100644 --- a/.github/skills/respond-to-review-comments/SKILL.md +++ b/.github/skills/respond-to-review-comments/SKILL.md @@ -76,7 +76,7 @@ For each **Fix** item: 3. Apply the smallest change that satisfies the reviewer without unrelated refactoring. 4. Preserve FieldWorks rules: - Use `build.ps1` and `test.ps1` for build/test work. - - Keep managed code compatible with .NET Framework 4.8 / C# 7.3. + - Keep managed code compatible with .NET Framework 4.8, the repo default C# 8.0 language policy, and nullable reference types disabled unless a project explicitly opts in. - Keep user-visible strings in `.resx`. - Preserve native-before-managed build order. - Preserve registration-free COM behavior. diff --git a/CONTEXT.md b/CONTEXT.md index cfb72c5a42..e55fa8ba76 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -80,6 +80,9 @@ It is intentionally not a full architecture manual. It should stay biased toward ## Review Workflow Language +- **PR preflight**: An interactive branch-readiness workflow before posting or updating a PR. It applies FieldWorks review policy, may use specialist review agents, interviews the author about risks and validation, and writes `.review/summary.md`. +- **Review analyzer**: The FieldWorks review policy in `.github/instructions/review-analyzer.instructions.md`. It defines what to check; it is not the interactive workflow. +- **Specialist review agent**: A focused read-only reviewer such as the FieldWorks C#, WinForms, C++, or Avalonia agent. Specialist output is evidence for the final synthesis, not a replacement for verifying findings against code. - **Review comment**: Any actionable feedback from Copilot or a human reviewer on a pull request. - **Review thread**: A GitHub inline conversation anchored to code. Resolve only after the thread is fully addressed and no question remains. - **Copilot reviewer comment**: Automated review feedback from GitHub Copilot. Evaluate it like external reviewer feedback; do not treat it as authoritative without checking the code. diff --git a/Src/AGENTS.md b/Src/AGENTS.md index ae29073fbf..20f76702e9 100644 --- a/Src/AGENTS.md +++ b/Src/AGENTS.md @@ -10,10 +10,10 @@ Minimal guidance for source-tree work. ## Constraints - Preserve native-first build order. -- Keep managed code compatible with .NET Framework 4.8 / C# 7.3. +- Keep managed code compatible with .NET Framework 4.8, the repo default C# 8.0 language policy, and nullable reference types disabled unless a project explicitly opts in. - Keep interop boundaries explicit; do not introduce COM registration side-effects. - Keep user-visible strings in resources. ## Discoverability -Use repo and folder AGENTS guidance plus Serena symbol navigation for project/dependency/test discovery. \ No newline at end of file +Use repo and folder AGENTS guidance plus Serena symbol navigation for project/dependency/test discovery. From f69a8e38762613c4805ba23af1f3e445ff47861c Mon Sep 17 00:00:00 2001 From: John Lambert Date: Thu, 21 May 2026 16:47:37 -0400 Subject: [PATCH 4/4] docs: add AI PR workflow onboarding --- .github/pull_request_template.md | 1 + Docs/CONTRIBUTING.md | 5 + Docs/core-developer-setup.md | 17 +- Docs/workflows/ai-pr-workflow.md | 241 ++++++++++++++++++++++++ Docs/workflows/pull-request-workflow.md | 2 + ReadMe.md | 1 + 6 files changed, 266 insertions(+), 1 deletion(-) create mode 100644 Docs/workflows/ai-pr-workflow.md diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 6650186cbf..e56fef9bdb 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -11,6 +11,7 @@ git diff --check --cached ``` - [ ] Builds/tests pass locally (or I've run the CI-style build via Bash script or MSBuild). +- [ ] If this is core-developer AI-assisted work, I followed `Docs/workflows/ai-pr-workflow.md` and ran `pr-preflight` or the equivalent branch-readiness review before requesting review. - [ ] For any `Src/**` folders touched, corresponding `AGENTS.md` files are updated or explicitly confirmed still accurate. ## Notes for reviewers (optional) diff --git a/Docs/CONTRIBUTING.md b/Docs/CONTRIBUTING.md index f44760c00f..21fbe0a6f0 100644 --- a/Docs/CONTRIBUTING.md +++ b/Docs/CONTRIBUTING.md @@ -142,6 +142,8 @@ Default recommendation: - **C# Dev Kit is discouraged** in this workspace. - Use repo scripts/tasks as source of truth for build/test: `./build.ps1` and `./test.ps1`. +If you are a core developer using GitHub Copilot or Claude Code, follow [AI-Assisted PR Workflow](workflows/ai-pr-workflow.md) for the Jira-to-PR path: create a dedicated worktree, validate with repo tasks/scripts, run `pr-preflight`, and then work review comments through the repo review-response workflow. + Switch to **Visual Studio 2022** when you need: - WinForms designer workflows - Mixed managed/native debugging across interop boundaries @@ -184,6 +186,8 @@ We welcome any contribution! To get started: See [workflows/pull-request-workflow.md](workflows/pull-request-workflow.md) for detailed PR guidelines. +Core developers doing AI-assisted work should start with [AI-Assisted PR Workflow](workflows/ai-pr-workflow.md) instead of jumping directly to the generic PR checklist. + ### Becoming a Core Developer People we know well might be asked to join the core development team. Core developers get additional privileges including the ability to make branches directly in the main repository and contribute in additional ways. @@ -198,5 +202,6 @@ People we know well might be asked to join the core development team. Core devel - [Visual Studio Setup](visual-studio-setup.md) - Detailed VS configuration - [Core Developer Setup](core-developer-setup.md) - Additional setup for core developers +- [AI-Assisted PR Workflow](workflows/ai-pr-workflow.md) - Canonical Jira-to-PR workflow for core developers using GitHub Copilot or Claude Code - [Pull Request Workflow](workflows/pull-request-workflow.md) - How to submit changes - [Build Instructions](../.github/instructions/build.instructions.md) - Detailed build guide diff --git a/Docs/core-developer-setup.md b/Docs/core-developer-setup.md index e21f51dcff..d57285af86 100644 --- a/Docs/core-developer-setup.md +++ b/Docs/core-developer-setup.md @@ -150,6 +150,20 @@ Recommended Git tools: - **GitKraken** or **SourceTree** - For visual branch management - **VS Code** - Has excellent Git integration +## AI-Assisted PR Workflow + +For core developers, the canonical AI-assisted path is now [AI-Assisted PR Workflow](workflows/ai-pr-workflow.md). + +Use that guide when you want the full FieldWorks workflow from Jira through merge-ready review: + +1. Pull the Jira ticket through approved Atlassian tooling. +2. Create a dedicated branch worktree with the repo task. +3. Build and test with `./build.ps1`, `./test.ps1`, or the matching VS Code tasks. +4. Run `pr-preflight` before opening or updating the PR. +5. Use the review-response workflow for Copilot and human comments. + +If you use Claude Code, create the worktree with the repo task first, then launch Claude inside that worktree. + ### 4. Working with Branches #### Branch Naming Conventions @@ -181,7 +195,7 @@ git checkout -b feature/my-feature-name 4. After approval and CI passes, merge the PR -See [Pull Request Workflow](workflows/pull-request-workflow.md) for detailed guidelines. +See [AI-Assisted PR Workflow](workflows/ai-pr-workflow.md) for the canonical core-developer workflow, and [Pull Request Workflow](workflows/pull-request-workflow.md) for the generic GitHub PR mechanics. ### 5. Release Management @@ -238,5 +252,6 @@ git branch -a # List all branches including remote ## See Also - [CONTRIBUTING.md](CONTRIBUTING.md) - Basic setup for all contributors +- [AI-Assisted PR Workflow](workflows/ai-pr-workflow.md) - Canonical Jira-to-PR workflow for core developers using GitHub Copilot or Claude Code - [Pull Request Workflow](workflows/pull-request-workflow.md) - How to submit changes - [Release Process](workflows/release-process.md) - Release workflow documentation diff --git a/Docs/workflows/ai-pr-workflow.md b/Docs/workflows/ai-pr-workflow.md new file mode 100644 index 0000000000..170051cb0e --- /dev/null +++ b/Docs/workflows/ai-pr-workflow.md @@ -0,0 +1,241 @@ +# AI-Assisted PR Workflow + +This is the canonical core-developer workflow for AI-assisted FieldWorks work. +It starts from a Jira ticket, uses a dedicated branch and git worktree, and +ends after PR preflight, reviewer feedback, and Jira follow-up. + +Use this workflow with either: + +- **GitHub Copilot** in VS Code +- **Claude Code** running inside the same FieldWorks worktree + +## Guiding Rules + +- Keep one ticket, one branch, one worktree, and one PR together. +- Include the Jira key in the branch name, commit messages, and PR title. +- Prefer repo tasks and scripts over ad hoc commands. +- Use agent-safe Jira access. Do not point agents at Jira URLs directly. +- Keep PRs small enough that a reviewer can understand them in one sitting. + +## Tool Map + +| Goal | GitHub Copilot | Claude Code | Repo anchor | +|------|----------------|-------------|-------------| +| Read Jira issue | Atlassian VS Code extension or ask Copilot to use the Atlassian helpers | Use the Atlassian helper scripts or ask Claude to follow them | [../.github/copilot-jira-setup.md](../../.github/copilot-jira-setup.md), [../.github/skills/atlassian-readonly-skills/SKILL.md](../../.github/skills/atlassian-readonly-skills/SKILL.md) | +| Create isolated worktree | VS Code task: `Worktree: Create/Open from branch` | Use the same VS Code task, then start Claude inside that worktree | [../.vscode/tasks.json](../../.vscode/tasks.json), [../scripts/Worktree-CreateFromBranch.ps1](../../scripts/Worktree-CreateFromBranch.ps1) | +| Preflight branch before PR | `/pr-preflight` | Ask Claude to follow [../.github/skills/pr-preflight/SKILL.md](../../.github/skills/pr-preflight/SKILL.md) for the current branch | [../.github/prompts/pr-preflight.prompt.md](../../.github/prompts/pr-preflight.prompt.md) | +| Respond to review comments | `/respond-to-review-comments` | Ask Claude to follow [../.github/skills/respond-to-review-comments/SKILL.md](../../.github/skills/respond-to-review-comments/SKILL.md) | [../.github/prompts/respond-to-review-comments.prompt.md](../../.github/prompts/respond-to-review-comments.prompt.md) | +| Build and test | VS Code `Build`, `Test`, and `CI: Full local check` tasks | Same tasks or `./build.ps1` and `./test.ps1` from the worktree root | [../ReadMe.md](../../ReadMe.md), [../AGENTS.md](../../AGENTS.md) | + +## Phase 1: Pull The Jira Issue + +1. Start from a real Jira ticket, usually an `LT-` issue. +2. Read the summary, description, recent comments, and acceptance signals before you ask an AI tool to edit code. +3. If you want agent access to Jira, use the approved service-user path described in [../.github/copilot-jira-setup.md](../../.github/copilot-jira-setup.md). +4. Keep the Jira key uppercase. Jira development linking depends on the exact key format. + +Recommended human-first path: + +- Use the **Atlassian VS Code** extension to browse assigned issues and copy the ticket key. + +Recommended scriptable path: + +```powershell +# Read one Jira issue through the repo's read-only helper +python -c "import sys; sys.path.insert(0, '.github/skills/atlassian-readonly-skills/scripts'); from jira_issues import jira_get_issue; print(jira_get_issue('LT-22382'))" +``` + +Best practice from Atlassian's linked-development docs: + +- Put the Jira key in the branch name. +- Put the Jira key in commit messages. +- Put the Jira key in the PR title. + +For example: + +- Branch: `bugfix/LT-22382-fix-crash` +- Commit: `LT-22382 guard deleted item selection` +- PR title: `LT-22382: guard deleted item selection` + +## Phase 2: Create A Dedicated Worktree + +FieldWorks supports concurrent work across git worktrees. Use them instead of stacking unrelated work on one checkout. + +Preferred path: + +1. Open the Command Palette and run **Tasks: Run Task**. +2. Run **`Worktree: Create/Open from branch`**. +3. Enter the branch name you chose for the Jira issue. +4. Let the new VS Code window open for that worktree. + +Useful worktree tasks: + +- `Worktree: Create/Open from branch` +- `Worktree: Create/Open from branch (picker in terminal)` +- `Worktree: Create/Open from branch (dry run)` +- `Worktree: Rename to branch` +- `Worktree: Open (picker)` + +Important repo-specific guidance: + +- Prefer the repo task over raw `git worktree add` when you want the normal FieldWorks setup. +- `Setup: Colorize Worktree` runs on folder open and gives each worktree a distinct VS Code window color. +- Open one VS Code window per worktree. +- If Serena or MCP feels stale after switching worktrees, run **MCP: Reset Cached Tools**. + +Claude Code users should still create the worktree with the repo task first, then start Claude inside that worktree. That keeps the workspace layout, colorization, and MCP setup consistent with the rest of the team. + +## Phase 3: Implement Inside Repo Guardrails + +Before editing: + +1. Read [../AGENTS.md](../../AGENTS.md) and [../CONTEXT.md](../../CONTEXT.md). +2. Load the instructions that match the touched area. +3. If the ticket language is fuzzy, use the `grill-with-docs` discipline before implementation. + +During implementation: + +- Keep changes scoped to the Jira ticket. +- Prefer a plan-first pass for larger or riskier changes. +- Use repo build and test entrypoints, not ad hoc `msbuild` or `dotnet build` commands. +- Keep validation notes as you go so the PR description is easy to write later. + +Standard validation surfaces: + +```powershell +./build.ps1 +./test.ps1 +./test.ps1 -TestProject "Src/Common/FwUtils/FwUtilsTests" +``` + +Useful VS Code tasks: + +- `Build` +- `Build Release` +- `Test` +- `Test (with filter)` +- `Test (specific project)` +- `CI: Full local check` + +Best practices from Claude Code and GitHub review guidance also apply here: + +- Plan before editing when the change is not obvious. +- Delegate broad investigation to subagents or focused review passes instead of mixing everything into one long conversation. +- Verify each risky change with the narrowest reliable test before widening scope. + +## Phase 4: Run Branch Preflight Before Posting Or Updating The PR + +Do a branch-level review before you ask humans to review the PR. + +### GitHub Copilot + +Run: + +```text +/pr-preflight +``` + +### Claude Code + +Ask Claude to follow the repo skill directly. A good prompt is: + +```text +Follow .github/skills/pr-preflight/SKILL.md for the current branch. +Use CONTEXT.md, .github/context/codebase.context.md, and .github/instructions/review-analyzer.instructions.md. +Write .review/summary.md. +``` + +`pr-preflight` is the multi-phase branch review. It is expected to: + +- review the branch diff from `main` +- apply the FieldWorks review policy +- challenge unclear reasoning and validation gaps +- write `.review/summary.md` + +Review the generated summary before you open or update the PR. Fix obvious issues first; do not outsource that judgment completely to the agent. + +## Phase 5: Create Or Update The PR + +When the branch is ready: + +1. Push the branch. +2. Create a PR against the correct base branch. +3. Use a title that starts with the Jira key. +4. Include testing and risk notes in the description. +5. Use a draft PR if you want feedback before the change is ready to merge. + +Recommended PR description content: + +- What changed +- Why the Jira issue required it +- Validation you ran +- Remaining risks or manual checks + +If the work also tracks a GitHub issue, link it in the PR description with the normal closing keywords. + +GitHub best practices worth keeping: + +- request specific reviewers +- re-request review after significant updates +- keep follow-up commits on the same branch so the discussion stays attached to the PR + +## Phase 6: Work The Review Loop + +Once Copilot or humans leave feedback, use the review-response workflow instead of replying ad hoc. + +### GitHub Copilot + +Run: + +```text +/respond-to-review-comments +``` + +### Claude Code + +Ask Claude to follow the repo skill directly. A good prompt is: + +```text +Follow .github/skills/respond-to-review-comments/SKILL.md for this PR. +Classify each comment as Fix, Clarify, Reply only, or Defer. +Verify each change with the narrowest FieldWorks check. +``` + +Expected behavior during review response: + +- verify comments against the code before changing anything +- make minimal scoped fixes +- ask one focused question when feedback is ambiguous +- reply in-thread with what changed or why no change is needed +- resolve a thread only after code and validation fully address it + +## Phase 7: Close The Jira Loop + +Before merge or immediately after it, make sure Jira reflects the actual state of the work. + +- Because the Jira key is in the branch, commits, and PR title, Jira's development panel can show linked branches, commits, and PRs. +- If you need to add a Jira comment or transition the issue, use the write-enabled Atlassian helpers rather than improvising API calls. + +Example: + +```powershell +python -c "import sys; sys.path.insert(0, '.github/skills/atlassian-skills/scripts'); from jira_issues import jira_add_comment; print(jira_add_comment('LT-22382', 'PR ready for review: https://github.com/sillsdev/FieldWorks/pull/905'))" +``` + +## Short Checklist + +- [ ] Jira ticket understood and key copied exactly +- [ ] Branch name includes the Jira key +- [ ] Dedicated worktree created with the repo task +- [ ] Changes built and tested with repo scripts or tasks +- [ ] `pr-preflight` run and `.review/summary.md` reviewed +- [ ] PR title starts with the Jira key +- [ ] Review comments handled through the review-response workflow +- [ ] Jira updated with the final branch or PR state when needed + +## See Also + +- [Core Developer Setup](../core-developer-setup.md) +- [Pull Request Workflow](pull-request-workflow.md) +- [MCP Helpers](../mcp.md) +- [Agent playbook](../../AGENTS.md) \ No newline at end of file diff --git a/Docs/workflows/pull-request-workflow.md b/Docs/workflows/pull-request-workflow.md index 1b6e96df01..6664c11b3d 100644 --- a/Docs/workflows/pull-request-workflow.md +++ b/Docs/workflows/pull-request-workflow.md @@ -2,6 +2,8 @@ This guide describes the process for submitting code changes to FieldWorks via GitHub Pull Requests. +> **Core developer note**: If you are using GitHub Copilot or Claude Code for FieldWorks work, start with [AI-Assisted PR Workflow](ai-pr-workflow.md). That guide covers the canonical path from Jira to worktree setup, branch preflight, reviewer feedback, and Jira follow-up. This document remains the generic GitHub PR mechanics reference. + ## Overview All code changes to FieldWorks go through a pull request (PR) workflow: diff --git a/ReadMe.md b/ReadMe.md index 9d908aaf1f..8593f191ff 100644 --- a/ReadMe.md +++ b/ReadMe.md @@ -6,6 +6,7 @@ New to FieldWorks development? Start here: - **[VS Code Stability Profile](Docs/vscode-stability-profile.md)** - ReSharper-first VS Code setup and when to switch to Visual Studio - **[Visual Studio Setup](Docs/visual-studio-setup.md)** - Detailed VS 2022 configuration - **[Core Developer Setup](Docs/core-developer-setup.md)** - Additional setup for team members +- **[AI-Assisted PR Workflow](Docs/workflows/ai-pr-workflow.md)** - Canonical core-developer path from Jira to worktree setup, branch preflight, and review response > **Note**: We are migrating documentation from the [FwDocumentation wiki](https://github.com/sillsdev/FwDocumentation/wiki) into this repository. Some wiki content may be more recent until migration is complete.