From c0d066985e866ae362aaf45382e67a79e70b950e Mon Sep 17 00:00:00 2001 From: Mridul Pathak Date: Sun, 9 Aug 2026 11:37:38 +0530 Subject: [PATCH 1/2] Added: manage-contribution-workflow skill; renamed precommit-readiness to prepush-readiness Adds a manage-contribution-workflow skill covering the OFBiz contribution lifecycle - fork/upstream sync, branching, optional worktree setup, the framework/plugins repo pairing, testing against real data, backporting, and PR conventions - that wasn't covered by the existing domain-pattern skills. Also corrects precommit-readiness, which mischaracterized the repository's git hook as pre-commit when it's actually pre-push (Checkstyle and CodeNarc only); renamed to prepush-readiness with accurate config paths and commands, and cross-referenced it with coding-standards. --- ai-agent-skills/README.md | 3 +- ai-agent-skills/SKILLS_SUMMARY.md | 3 +- ai-agent-skills/coding-standards/SKILL.md | 10 +- .../manage-contribution-workflow/SKILL.md | 258 ++++++++++++++++++ .../SKILL.md | 69 +++-- 5 files changed, 323 insertions(+), 20 deletions(-) create mode 100644 ai-agent-skills/manage-contribution-workflow/SKILL.md rename ai-agent-skills/{precommit-readiness => prepush-readiness}/SKILL.md (65%) diff --git a/ai-agent-skills/README.md b/ai-agent-skills/README.md index 4ef96b8b0..cb69b9a87 100644 --- a/ai-agent-skills/README.md +++ b/ai-agent-skills/README.md @@ -100,7 +100,8 @@ The toolkit currently covers: - **Integrations**: `manage-api-integration`, `manage-email-services`. - **Advanced Management**: `manage-security-advanced`, `manage-localization-advanced`, `manage-webapps`, `manage-cache-and-performance`. - **Strategies**: `manage-strategies` (Xml vs Java/Groovy, Dos and Donts). -- **Quality Gates**: `precommit-readiness` (pre-commit hooks, CodeNarc, Groovy/Java compilation, XML validation, and focused tests). +- **Quality Gates**: `prepush-readiness` (the pre-push hook — Checkstyle, CodeNarc — plus compilation, XML validation, and focused tests). +- **Contribution Workflow**: `manage-contribution-workflow` (worktree setup, the framework/plugins repo pairing, pre-test data loading, backporting, and the PR template). ## Deployment and Updates This plugin is linked to the GitHub repository. To update the skills available to your agent, pull the latest changes from the repository. diff --git a/ai-agent-skills/SKILLS_SUMMARY.md b/ai-agent-skills/SKILLS_SUMMARY.md index 1057ac4ac..c46faed03 100644 --- a/ai-agent-skills/SKILLS_SUMMARY.md +++ b/ai-agent-skills/SKILLS_SUMMARY.md @@ -79,7 +79,8 @@ This document provides a high-level summary of the specialized skills developed | **create-component** | Scaffold new OFBiz plugins using standardized directory structures. | | **manage-component** | Manage component-level dependencies and `ofbiz-component.xml` registrations. | | **coding-standards** | General guidelines for clean code, commenting, and OFBiz contribution standards. | -| **precommit-readiness** | Write changes in a hook-ready shape for CodeNarc, Groovy/Java compilation, XML validation, and focused tests before committing. | +| **prepush-readiness** | Write changes in a hook-ready shape for the pre-push hook (Checkstyle, CodeNarc), plus compilation, XML validation, and focused tests before pushing. | +| **manage-contribution-workflow** | Take a change from branch to merge-ready PR: worktree setup, the framework/plugins repo pairing, pre-test data loading, backporting, and the community PR template. | ## 🧠 Strategic Thinking | Skill | Description | diff --git a/ai-agent-skills/coding-standards/SKILL.md b/ai-agent-skills/coding-standards/SKILL.md index 2099449d6..314794309 100644 --- a/ai-agent-skills/coding-standards/SKILL.md +++ b/ai-agent-skills/coding-standards/SKILL.md @@ -19,7 +19,11 @@ under the License. --- name: coding-standards -description: Guidelines for writing production-quality code, focusing on commenting style, cleanliness, and professionalism. +description: +- Writing or reviewing Java, Groovy, or XML changes in OFBiz. +- Choosing naming conventions for components, entities, services, or classes. +- Deciding whether logic belongs in XML actions vs. Groovy. +- Applying UtilValidate, UtilMisc, EntityQuery, or UI-label (i18n) patterns. --- # Skill: coding-standards @@ -73,3 +77,7 @@ GenericValue price = EntityQuery.use(delegator).from("ProductPrice") .filterByDate() .queryFirst(); ``` + +## See Also +- `prepush-readiness` — the mechanical checks (Checkstyle, CodeNarc, compilation) that enforce a subset of these standards before a push. +- `manage-contribution-workflow` — branching, testing, commit, and PR conventions once the code itself is ready. diff --git a/ai-agent-skills/manage-contribution-workflow/SKILL.md b/ai-agent-skills/manage-contribution-workflow/SKILL.md new file mode 100644 index 000000000..050bac623 --- /dev/null +++ b/ai-agent-skills/manage-contribution-workflow/SKILL.md @@ -0,0 +1,258 @@ + + +--- +name: manage-contribution-workflow +description: Take an OFBiz change from branch to merge-ready PR — branching and worktree setup, the framework/plugins repo pairing, pre-test data loading, commit hygiene, backporting, and the community PR template. +--- + +# Skill: manage-contribution-workflow + +## Goal + +Get a change through the actual mechanics of an OFBiz contribution — not the +code itself (see `manage-entities`, `manage-services`, `manage-groovy`, etc. +for that), but everything around it: branch setup, running tests against real +data, keeping the paired `plugins/` repo in sync, and shaping commits/PRs the +way the project expects. Most of this workflow is invisible in the diff, but +skipping it produces failures and review friction that have nothing to do +with the code change itself. + +## Triggers + +**ALWAYS** read this skill when: +- Starting work on an OFBiz bug, improvement, or feature (with or without a + filed JIRA issue). +- Creating a branch or a linked `git worktree` in an `ofbiz-framework` (or + paired `ofbiz-plugins`) checkout. +- About to run `ofbiz --test component=...` for regression verification. +- Preparing a commit message or a pull request. +- Backporting a fix from `trunk` to a release branch. + +## Fork And Upstream Sync + +Most OFBiz contributor checkouts are a personal fork (`origin`) of the +canonical Apache repo (`upstream`). This sync is step zero — it happens +**before** any branch is created or pushed, empty or not, and before JIRA +questions are even relevant: + +1. **Confirm `upstream` exists.** `git remote -v` should list both `origin` + (your fork) and `upstream` (`https://github.com/apache/ofbiz-framework.git` + or `https://github.com/apache/ofbiz-plugins.git`, matching whichever repo + this is). If `upstream` is missing, add it: + ```bash + git remote add upstream https://github.com/apache/.git + ``` +2. **Sync fully, then branch, then push.** Fetch and fast-forward the local + base branch (`trunk`, or the release branch you're backporting to) from + `upstream`: + ```bash + git fetch upstream + git checkout trunk + git merge --ff-only upstream/trunk + git push origin trunk + ``` + Only cut the new branch and push it — whether pushed empty right away or + already carrying changes — after this completes. Branching from a stale + fork risks conflicts, duplicate fixes, or missing context that landed + upstream since the fork's `trunk` was last updated; syncing *after* the + branch already exists doesn't fix that, since the branch's base is fixed + at the moment it's cut. +3. **Re-check before opening the PR.** If time has passed since the branch + was cut, re-fetch `upstream` and confirm the branch still applies cleanly + against current `trunk` — don't assume nothing has changed. This is a + check, not an automatic rebase: if the feature branch has already been + pushed, don't rewrite its history to "catch up" — a real conflict is a + sign to resolve it deliberately (merge or rebase, as the situation calls + for), not to silently force-sync. + +## JIRA And Branching + +- A JIRA issue (`OFBIZ-XXXXX`) is standard practice — it gives the change a + tracking number and is what the PR title's `(OFBIZ-XXXXX)` suffix refers + to — but it is **not mandatory**. Committers in particular may branch and + PR directly for small or obvious fixes. +- **Never pick, reuse, or infer an issue ID on your own.** Don't pattern-match + git log/PR history to reuse an existing ticket ("prior commits touching + this file used OFBIZ-NNNNN, so I'll use that too"), and don't invent one. + Ask the contributor which issue ID to use, or whether to file a new one, or + whether to skip JIRA entirely — before creating the branch. This applies + even when precedent looks obvious and reusing it would seem like the + efficient, consistent choice; consistency with history is the contributor's + call, not an inference to make for them. +- Once confirmed (and only after the Fork And Upstream Sync above), branch as + `OFBIZ-XXXXX` or `OFBIZ-XXXXX-short-slug` when an issue exists, otherwise a + short descriptive name. +- One branch per issue/change. Don't stack unrelated work on the same branch. + +## Worktree Setup + +Using a linked `git worktree` is **optional**, not a requirement of this +workflow — branching and working directly in the main checkout (on `trunk`, +or any release branch) is equally valid, and skips every setup step below +entirely. Reach for a worktree when isolation actually matters (e.g. keeping +several issues in flight at once without stashing between them); otherwise +default to working in place. + +`git worktree add` only checks out tracked files, which leaves several things +missing in a fresh linked worktree of this repo: + +1. **`gradle/wrapper/gradle-wrapper.jar`** — intentionally gitignored (see + `gradle/init-gradle-wrapper.sh`), so `./gradlew` fails immediately with + "Unable to access jarfile" until it's restored. +2. **`plugins/`** — a separate git repository in its own right, not a + submodule, so a new worktree gets an empty (or wrong-branch) `plugins/` + directory. +3. **Agent skill directories** (`.claude/skills/`, `.gemini/skills/`, + `.github/skills/`, `.cursor/skills/`, `.agents/skills/`) — these are + untracked, generated copies of `plugins/ai-agent-skills/` (see that + plugin's own README), so a fresh worktree has none of them until + regenerated. Without this step, whatever coding agent is working in the + worktree has no skills available at all — not this one, not the + domain-specific ones (`manage-entities`, `manage-services`, etc.). + +From the repo root, after creating a new worktree, run in order before the +first build: + +```bash +./setup-worktree-gradle-wrapper.sh +./setup-worktree-plugins.sh +./gradlew syncAgentSkills -Pagent= +``` + +`setup-worktree-gradle-wrapper.sh` copies the jar from the main checkout when +the Gradle version matches, or falls back to the verified download path. +`setup-worktree-plugins.sh` adds a linked worktree of the `plugins/` repo +(inferring the matching branch from the framework branch's ancestry) — +`plugins/` must exist first, since `syncAgentSkills` is defined inside +`plugins/ai-agent-skills/build.gradle`. All three are idempotent — safe to +re-run. + +**`` is not a placeholder to ask the user about — infer it from the +path you loaded this very skill file from**, since that path already +identifies which agent is running this step: + +| You loaded this skill from | Run with | +| :--- | :--- | +| `.claude/skills/...` | `-Pagent=claude` | +| `.gemini/skills/...` | `-Pagent=gemini` | +| `.github/skills/...` | `-Pagent=github` | +| `.cursor/skills/...` | `-Pagent=cursor` | +| `.agents/skills/...` (or unclear) | `-Pagent=agents` | + +This syncs only the directory the current agent actually reads from — +skip `all`, which would regenerate every agent's directory in the worktree +regardless of whether that agent is in use here. If a human runs this step +manually and multiple agents are expected to work in the same worktree, +`-Pagent=all` (or a comma-separated list) is still fine — the per-agent +inference above is for an agent acting on its own behalf. + +## The `plugins/` Repo Pairing + +`ofbiz-framework` and `ofbiz-plugins` are independent git repositories that +must be built together. Whenever the framework repo's branch changes — +switching to a release branch, back to `trunk`, or onto a new branch — check +`plugins/`'s branch too: + +```bash +git branch --show-current +git -C plugins branch --show-current +``` + +Do this **before** any Gradle build and before `git push` (the pre-push hook +builds the project too). A mismatch doesn't fail with an obvious +branch-mismatch error — it surfaces as a misleading Groovy-version dependency +conflict or a cascading "No hooks found" Gradle configuration failure. If a +full build or push fails that way, check `plugins/`'s branch before assuming +it's a real dependency problem. + +Uncommitted `package-lock.json` changes in either repo (e.g. under +`plugins/*/webapp/*/`) are npm-regenerated lockfile churn from local +builds/installs, not intentional changes — safe to discard without asking. + +## Testing Against Real Data + +Before running any `./gradlew "ofbiz --test component=X"` command, run +`./gradlew cleanAll loadAll` first. Don't trust the embedded H2 dev +database's existing state — it may be empty, partially loaded, or stale. + +Skipping this doesn't fail loudly — it produces failures like +`ServiceAuthException: User login is missing` that look like real bugs (or +"pre-existing flakiness") but are actually missing seed data +(`UserLogin`/permission records most service-engine tests depend on). +`cleanAll loadAll` first, every time, before treating test failures as +meaningful signal. + +## Backporting To A Release Branch + +When a fix committed on `trunk` also needs to land on a release branch (e.g. +`release24.09`), use: + +```bash +git cherry-pick +``` + +rather than manually re-applying the same diff and writing a fresh commit. +Cherry-pick preserves the original message and authorship and is far less +error-prone than hand-editing twice. + +Cherry-pick is the default, not a guarantee — if the surrounding code has +drifted enough between `trunk` and the release branch that it conflicts, +resolve conflicts in place if they're small and mechanical, or fall back to +manually re-applying the change's intent on the release branch if the code +shapes have genuinely diverged (e.g. a method was refactored/renamed/moved +after the release branch was cut). A manual backport is still expected to +produce the same behavioral fix, just adapted to the release branch's +version of the code — it's not license to reinterpret the fix. + +In the backport PR description, say "Backported from trunk (#)" +either way, and note if it required manual adaptation rather than a clean +cherry-pick. + +## Pull Request Conventions + +Title: one of `Improved:` / `Implemented:` / `Documented:` / `Completed:` / +`Reverted:` / `Fixed:`, followed by a short description, ending with +`(OFBIZ-XXXXX)` when an issue exists. + +Body: a single concise paragraph — what was wrong or needed, and what +changed. + +- Don't repeat the title as the first line of the body. +- Don't add an "Explanation" (or similar) heading — just the paragraph. +- Only add a `Thanks:` line when someone other than the PR author actually + helped (reported the issue, reviewed, co-designed). Self-filed, + self-authored changes don't get one. +- Keep it short by default; expand only if more detail is genuinely needed. + +## Guardrails + +- Don't build or push from a checkout where the framework and `plugins/` + branches disagree — fix the mismatch first, don't debug around it. +- Don't treat `ofbiz --test` failures as pre-existing/environmental without + first confirming `cleanAll loadAll` was run. +- Don't hand-reapply a diff for a backport when cherry-pick is available. +- Don't block a change on filing a JIRA issue when the contributor (e.g. a + committer) doesn't need one — but do suggest filing one for anything + non-trivial, since it's what ties the PR title, commit history, and + release notes together. + +## See Also +- `coding-standards` — OFBiz style and idiom rules to follow while writing the change. +- `prepush-readiness` — the pre-push hook (Checkstyle, CodeNarc) and other checks to clear before pushing. diff --git a/ai-agent-skills/precommit-readiness/SKILL.md b/ai-agent-skills/prepush-readiness/SKILL.md similarity index 65% rename from ai-agent-skills/precommit-readiness/SKILL.md rename to ai-agent-skills/prepush-readiness/SKILL.md index 0a82e66f0..d3b722c16 100644 --- a/ai-agent-skills/precommit-readiness/SKILL.md +++ b/ai-agent-skills/prepush-readiness/SKILL.md @@ -18,28 +18,59 @@ under the License. --> --- -name: precommit-readiness -description: Write OFBiz changes so they pass local commit hooks, CodeNarc, Groovy compilation, Java checks, XML validation, and focused tests before the commit step. +name: prepush-readiness +description: Write OFBiz changes so they pass the pre-push hook (Checkstyle for Java, CodeNarc for Groovy) and are in committable shape — clean compilation, valid XML/widgets, and focused tests — before you push. --- -# Skill: precommit-readiness +# Skill: prepush-readiness ## Goal Prevent the "functional code first, hook fixes later" loop. Code should be -written in the shape this repository's pre-commit checks, CodeNarc rules, -Groovy compilation, Java compilation, XML parsers, and focused tests will -accept. +written in the shape this repository's pre-push hook (Checkstyle, CodeNarc), +Groovy/Java compilation, XML parsers, and focused tests will accept — checked +early, not discovered at push time. + +**There is no pre-commit hook in this repo.** The `com.github.jakemarsden.git-hooks` +Gradle plugin wires a single **pre-push** hook (`build.gradle`) that runs +`checkstyleMain checkstyleTest codenarcMain codenarcTest`. Compilation, XML +validity, and tests are not hook-enforced at all — they're this skill's own +recommended quality bar, worth checking before you commit even though nothing +stops you from committing broken code locally. ## Triggers **ALWAYS** read this skill when: -- Writing or modifying code that may be committed. +- Writing or modifying code that may be committed or pushed. - Editing Groovy, Java, XML, FreeMarker, properties, data, tests, or Gradle files. -- Preparing a commit or responding to pre-commit/build failures. +- Preparing a commit, preparing to push, or responding to a pre-push hook failure. - Adding logic that affects service contracts, entity models, widgets, events, data loading, or tests. +## The Pre-Push Gate + +The hook runs exactly two Gradle checks, both zero-tolerance: + +| Check | Language | Config | Command | +| :--- | :--- | :--- | :--- | +| Checkstyle | Java | `config/checkstyle/checkstyle.xml` (`maxErrors = 0`) | `./gradlew checkstyleMain checkstyleTest` | +| CodeNarc | Groovy | `config/codenarc/codenarc.groovy` (`maxPriority1/2/3Violations = 0`) | `./gradlew codenarcMain codenarcTest` | + +Mirror the hook exactly before pushing: + +```bash +./gradlew checkstyleMain checkstyleTest codenarcMain codenarcTest +``` + +This is the only command that reproduces what will actually block the push — +running a broader `check`/`build` task also compiles and tests, which is +useful, but don't mistake that for the hook itself. + +If working in a linked `git worktree`, the hook still fires there (the plugin +resolves the real `.git` common dir so this works even though `.git` is a +gitlink file in a worktree) — see `manage-contribution-workflow` for the +other worktree setup steps. + ## Core Workflow 1. **Load the domain skill first** @@ -49,22 +80,22 @@ accept. `manage-controller`, `manage-java`, or `manage-tests`. 2. **Design for hook compatibility before editing** - - Identify likely checks for the touched files: CodeNarc for Groovy, - compilation for Java/Groovy, XML parsing/schema expectations for OFBiz XML, - and focused tests for behavior. + - Identify likely checks for the touched files: Checkstyle for Java, + CodeNarc for Groovy, compilation for both, XML parsing/schema + expectations for OFBiz XML, and focused tests for behavior. - Prefer existing local patterns in nearby files over introducing a new style. - Keep the diff narrow so validation failures are easy to isolate. 3. **Run focused validation before finishing** - Prefer the narrowest command that exercises the changed area. - - If a pre-commit hook exists and the user is preparing to commit, run or - mirror the hook checks before the commit step when practical. + - When preparing to push, run the exact hook command above first — don't + rely on IDE linting or memory of the rules. - If validation is too expensive or blocked, explain exactly what was not run and why. 4. **Capture reusable failures** - - When a hook or build fails for a pattern likely to recur, update this skill - with the bad pattern and preferred replacement. + - When the hook or build fails for a pattern likely to recur, update this + skill with the bad pattern and preferred replacement. ## Groovy Readiness @@ -120,8 +151,8 @@ Choose the smallest useful validation set: existing Gradle task that validates the touched area. - Behavior changes: add or update focused tests when the risk is more than mechanical. -- Commit preparation: run the same checks the pre-commit hook will run when - practical. +- Push preparation: run `./gradlew checkstyleMain checkstyleTest codenarcMain + codenarcTest` — the exact pre-push hook command — before pushing. ## Common Failure Patterns To Avoid @@ -148,3 +179,7 @@ Before handing back a code change: - Imports, locals, and labels are clean. - Focused validation was run, or any skipped validation is clearly reported. - Any new recurring hook failure pattern is added back to this skill. + +## See Also +- `coding-standards` — the broader style and OFBiz-idiom rules this skill's checks partially enforce. +- `manage-contribution-workflow` — worktree setup (including why the pre-push hook still works there), testing against real data, and PR conventions. From 21516b684b45a49ed77e43349016ca33f52a3386 Mon Sep 17 00:00:00 2001 From: Mridul Pathak Date: Sun, 9 Aug 2026 14:06:50 +0530 Subject: [PATCH 2/2] Fixed: manage-contribution-workflow didn't cover resuming an existing branch/PR The skill's triggers only fired on starting or creating new work, so resuming a branch/PR from an earlier session could skip loading it entirely; and the upstream-sync step only applied at branch creation, with re-checking before a PR framed as conditional ("if time has passed"). Made both unconditional: added explicit triggers for resuming existing work and for any git push, and require an upstream fetch/drift-check before every push to an existing branch, not just the first one - this gap caused a real PR to land with a conflict. --- .../manage-contribution-workflow/SKILL.md | 43 ++++++++++++++----- 1 file changed, 32 insertions(+), 11 deletions(-) diff --git a/ai-agent-skills/manage-contribution-workflow/SKILL.md b/ai-agent-skills/manage-contribution-workflow/SKILL.md index 050bac623..5562d2969 100644 --- a/ai-agent-skills/manage-contribution-workflow/SKILL.md +++ b/ai-agent-skills/manage-contribution-workflow/SKILL.md @@ -41,8 +41,15 @@ with the code change itself. filed JIRA issue). - Creating a branch or a linked `git worktree` in an `ofbiz-framework` (or paired `ofbiz-plugins`) checkout. +- **Resuming work on a branch or PR that already existed before this + session** — this applies exactly as much as starting fresh. A branch + cut days or weeks ago is more likely to be stale against `upstream`, not + less, so picking up old work is a stronger reason to re-sync, not a reason + to skip it because "setup already happened." +- About to `git push` for any reason — a new branch, an update to an + existing one, or a fix requested on an open PR. - About to run `ofbiz --test component=...` for regression verification. -- Preparing a commit message or a pull request. +- Preparing a commit message or a pull request, or updating one already open. - Backporting a fix from `trunk` to a release branch. ## Fork And Upstream Sync @@ -59,9 +66,9 @@ questions are even relevant: ```bash git remote add upstream https://github.com/apache/.git ``` -2. **Sync fully, then branch, then push.** Fetch and fast-forward the local - base branch (`trunk`, or the release branch you're backporting to) from - `upstream`: +2. **Sync fully, then branch, then push — for a brand new branch.** Fetch + and fast-forward the local base branch (`trunk`, or the release branch + you're backporting to) from `upstream`: ```bash git fetch upstream git checkout trunk @@ -74,13 +81,27 @@ questions are even relevant: upstream since the fork's `trunk` was last updated; syncing *after* the branch already exists doesn't fix that, since the branch's base is fixed at the moment it's cut. -3. **Re-check before opening the PR.** If time has passed since the branch - was cut, re-fetch `upstream` and confirm the branch still applies cleanly - against current `trunk` — don't assume nothing has changed. This is a - check, not an automatic rebase: if the feature branch has already been - pushed, don't rewrite its history to "catch up" — a real conflict is a - sign to resolve it deliberately (merge or rebase, as the situation calls - for), not to silently force-sync. +3. **Re-sync before every push — not just the first one, unconditionally.** + Before pushing to a branch that already exists — the second commit, the + tenth, a fix requested on review, a session picking the branch back up — + re-fetch `upstream` and check whether its target branch (`trunk`, or the + release branch) has moved in a way that would conflict: + ```bash + git fetch upstream + git log HEAD..upstream/trunk --oneline + ``` + Do this **every single push**, with no exceptions for "I just did this + earlier," "this is the same session," or "it's probably still fine" — + none of those are things an agent can actually verify without doing the + fetch, so they're not valid reasons to skip it. A branch resumed after a + gap (a new session, a different day) is *more* likely to be stale, not + less — resuming old work is a stronger reason to check, never a reason to + assume the earlier sync still covers you. + This is a check, not an automatic rebase: if the feature branch has + already been pushed, don't rewrite its history to "catch up" — a real + conflict is a sign to resolve it deliberately (merge or rebase, as the + situation calls for), not to silently force-sync or push anyway and let + the PR surface the conflict. ## JIRA And Branching