Repository navigation
Remove fork-specific instructions and define a single agent completion workflow - #88
Merged
Merged
Conversation
…completion workflow
Reviewer's GuideAGENTS.md is rewritten to reflect the repository’s canonical ownership, eliminate conflicting fork procedures, and provide one actionable agent workflow with reproducible setup, verification, specification boundaries, integration notes, versioning, changelog, and completion guidance. Flow diagram for the unified agent completion workflowflowchart TD
Understand[Understand the task] --> Plan[Plan affected files and verification]
Plan --> Implement[Implement focused changes]
Implement --> Test[Run pytest and spec checks]
Test --> Lint[Review style and formatting]
Lint --> Verify[Verify acceptance criteria]
Verify --> Finalize[Finalize report and PR target]
Finalize --> Complete[Completion criteria met]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
6 tasks
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 8 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="AGENTS.md" line_range="72" />
<code_context>
+
+## Reproducible setup
+
+- **Dependencies**: `pip install -r requirements.txt -r requirements-test.txt`. Host libraries (`pyguitarpro`, `PyYAML`) ship with the feedBack host in deployment; for local testing, ensure they are available. Tests that require ffmpeg will self-skip if `ffmpeg` isn't on PATH (imageio-ffmpeg can provide it).
+- **Test command**: `pytest` (from repo root). Host-gated tests may skip if host `lib/` or required deps aren't present; see learnings for running with host lib on `PYTHONPATH`.
+- **Specs**: `feedpak-spec/` and `feedBack-plugin-spec/` provide normative requirements referenced in this guide.
</code_context>
<issue_to_address>
**Test collection fails**
When an agent follows the listed install command in a clean plugin-only checkout without PyYAML, the requirements files do not install PyYAML, so `tests/conftest.py` imports `feedpakr_pipeline`, which imports `feedpakr_pack`; its module-level `import yaml` raises `ModuleNotFoundError` and pytest collection stops. The setup also gives no installation step for the host dependencies needed in a plugin-only checkout.
Add PyYAML and any required host dependencies to the documented setup, or give a clear installation step for them before running pytest.
Also at `AGENTS.md:73`, `AGENTS.md:78`.
</issue_to_address>
### Comment 2
<location path="AGENTS.md" line_range="68" />
<code_context>
+4. **Test**: Run the test suite with `pytest` to catch regressions. Verify changes against the relevant specs when touching manifest shape or validation logic.
+5. **Lint/format**: Follow repo conventions; no separate lint command is mandated beyond what tests/checks expect (see learnings). Ensure code matches surrounding style.
+6. **Verify**: Confirm the implementation meets acceptance criteria from the task/issue. Run any targeted checks relevant to your changes.
+7. **Finalize**: Ensure working tree is clean, summarize changes, and follow the run's reporting requirements. If the task involves creating a PR, ensure it targets `get-flashbacks/feedBack-plugin-feedpakr` (default `main`).
+
+## Reproducible setup
</code_context>
<issue_to_address>
**Requested edits disappear**
When an agent finishes a task with uncommitted task changes, the clean-working-tree requirement makes the agent treat its requested edits as unfinished; restoring the tree removes those edits, while committing them may be unrequested. The task is then delivered without the requested patch or with an unexpected commit.
Clarify that requested task edits may remain uncommitted and that only unrelated changes need to be cleaned up.
Also at `AGENTS.md:99`.
</issue_to_address>
### Comment 3
<location path="AGENTS.md" line_range="87" />
<code_context>
+- **Chordr**: Optional chord naming during import; failures degrade gracefully and emit warnings.
+- **FFmpeg**: Required only for certain audio operations; tests gated behind availability and self-skip when missing.
## Versioning
-Bump `version` in `plugin.json` whenever a change is user-visible — new
</code_context>
<issue_to_address>
**Required checks reject changes**
When a functional source change is considered not user-visible or notable, the versioning and changelog guidance does not require updates for that change, but CI requires both a `plugin.json` version bump and a `CHANGELOG.md` update for every functional source change. Following the guide therefore leaves the PR failing those checks.
Update both policies to require the version bump and changelog entry for every functional source change, including changes judged not user-visible or notable.
Also at `AGENTS.md:89`, `AGENTS.md:93`.
</issue_to_address>
### Comment 4
<location path="AGENTS.md" line_range="73" />
<code_context>
+## Reproducible setup
+
+- **Dependencies**: `pip install -r requirements.txt -r requirements-test.txt`. Host libraries (`pyguitarpro`, `PyYAML`) ship with the feedBack host in deployment; for local testing, ensure they are available. Tests that require ffmpeg will self-skip if `ffmpeg` isn't on PATH (imageio-ffmpeg can provide it).
+- **Test command**: `pytest` (from repo root). Host-gated tests may skip if host `lib/` or required deps aren't present; see learnings for running with host lib on `PYTHONPATH`.
+- **Specs**: `feedpak-spec/` and `feedBack-plugin-spec/` provide normative requirements referenced in this guide.
+
</code_context>
<issue_to_address>
**Required regression checks stay skipped**
When an agent runs the documented `pytest` command without the host checkout or FFmpeg and without setting `FEEDPAKR_REQUIRE`, `pytest` runs with `FEEDPAKR_REQUIRE` unset, so host- and FFmpeg-dependent regression tests can skip while the command exits successfully. The workflow treats that run as verification without requiring the release-critical tests to execute, leaving regressions undetected until CI.
Document the required host and FFmpeg setup and run pytest with `FEEDPAKR_REQUIRE=host,ffmpeg`, linking to `tests/SKIPS.md` for the skip policy.
</issue_to_address>
### Comment 5
<location path="AGENTS.md" line_range="74" />
<code_context>
+
+- **Dependencies**: `pip install -r requirements.txt -r requirements-test.txt`. Host libraries (`pyguitarpro`, `PyYAML`) ship with the feedBack host in deployment; for local testing, ensure they are available. Tests that require ffmpeg will self-skip if `ffmpeg` isn't on PATH (imageio-ffmpeg can provide it).
+- **Test command**: `pytest` (from repo root). Host-gated tests may skip if host `lib/` or required deps aren't present; see learnings for running with host lib on `PYTHONPATH`.
+- **Specs**: `feedpak-spec/` and `feedBack-plugin-spec/` provide normative requirements referenced in this guide.
+
+## Verification
</code_context>
<issue_to_address>
**Spec directories are absent**
When an agent tries to consult the referenced local spec directories, the guide sends agents to local `feedpak-spec/` and `feedBack-plugin-spec/` paths, but neither directory exists in the checkout, so agents cannot follow the stated normative requirements from those references.
Link to the actual spec repositories or explain where to obtain the specs and distinguish them from the vendored schemas.
</issue_to_address>
### Comment 6
<location path="AGENTS.md" line_range="62" />
<code_context>
+
+Follow this single completion workflow for all agent tasks:
+
+1. **Understand the task**: Read the task requirements and related context (issue, PR, or instructions). Check AGENTS.md for repo-specific guidance.
+2. **Plan**: Identify affected files, key changes, and verification steps. Create a concise todo list if the work spans multiple steps.
+3. **Implement**: Make minimal, focused changes that follow existing code style, naming, and patterns. Use `context['load_sibling']` for cross-module imports, `context['log']` for logging, and respect the plugin's architecture (blocking work in plain `def` handlers).
</code_context>
<issue_to_address>
**Architecture guidance gets missed**
When an agent task involves an area covered by the detailed architecture notes, the workflow tells agents to check `AGENTS.md` but does not link the detailed architecture notes in `README.md` or `CLAUDE.md`. Agents can miss constraints on areas such as audio synchronization and integrations.
Link the detailed architecture notes from the workflow so agents are directed to consult them when relevant.
</issue_to_address>
### Comment 7
<location path="AGENTS.md" line_range="85" />
<code_context>
+## Optional integrations
+
+- **Chordr**: Optional chord naming during import; failures degrade gracefully and emit warnings.
+- **FFmpeg**: Required only for certain audio operations; tests gated behind availability and self-skip when missing.
## Versioning
</code_context>
<issue_to_address>
**FFmpeg CI requirement is hidden**
When `FEEDPAKR_REQUIRE=host,ffmpeg` is set and FFmpeg is unavailable, `pytest_configure` aborts the session when CI requires FFmpeg and it is unavailable, rather than letting the tests self-skip; the guide therefore misrepresents a required CI capability as optional and can lead agents to overlook broken provisioning.
Clarify that these tests self-skip locally when capabilities are not required, but CI requires FFmpeg and fails if it is missing.
</issue_to_address>
### Comment 8
<location path="AGENTS.md" line_range="56" />
<code_context>
-everyone.
+## Repository & PR target
+
+This is the canonical `get-flashbacks/feedBack-plugin-feedpakr` repository. All changes should be opened as PRs against `get-flashbacks/feedBack-plugin-feedpakr` (default branch `main`). It is not a fork of another repo — `got-feedBack`'s plugin ecosystem (`feedpak-spec` and `feedBack-plugin-spec`) provides the specifications this plugin follows. Refer to those specs for normative requirements; host implementation details are noted where they differ.
+
+## Agent completion workflow
</code_context>
<issue_to_address>
**PR template still demands fork classification**
When an agent creates a PR using the repository template, the guide requires PRs to target the canonical repository, but the PR template still asks whether changes are for “this fork only” or should be upstreamed, leaving agents with contradictory PR guidance.
Remove the fork/upstream fields from the PR template so it matches the canonical-repository guidance.
</issue_to_address>Sourcery assessment
Approval pending. 8 findings to address first.
Blocking findings: AGENTS.md:72, AGENTS.md:68, AGENTS.md:87, AGENTS.md:73, AGENTS.md:74, and 3 more
Contributor
Author
|
Addressed all 8 Sourcery findings on PR #88 and pushed commit
Every thread has a reply and is resolved; the PR body now records the template removal and these follow-ups. Working tree is clean and the branch is pushed. Task list (5/5 completed)
|
Contributor
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Previous Review Summaries (2 snapshots, latest commit 3061c85)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 3061c85)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous review (commit 299a075)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Reviewed by efficient · Input: 55K · Output: 7.7K · Cached: 160K |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
Author
There was a problem hiding this comment.
Important
The AGENTS.md rewrite is a clear improvement, but two pieces of issue #83's scope are still open: CLAUDE.md was never reconciled (the duplication the issue cites persists, and the two files now disagree), and fork-oriented language still lives in .github/PULL_REQUEST_TEMPLATE.md.
Reviewed changes
Reviewed the single-commit, single-file diff against issue #83.
- Fork guidance removed — the
## PR Formatblock (fork/canonical/upstream/rebase procedures) is replaced with a## Repository & PR targetsection namingget-flashbacks/feedBack-plugin-feedpakrand default branchmain. - Single completion workflow added — a 7-step
## Agent completion workflow, plus## Reproducible setup,## Verification,## Optional integrations,## Changelog policy, and## Completion criteria. - Versioning section condensed — patch pattern changed from
0.1.xto0.x.y(correct:plugin.jsonis0.10.1), and the cache-busting rationale was dropped.
⚠️ Consolidation with CLAUDE.md is incomplete
Issue #83 names "duplicated guidance across CLAUDE.md" as part of the problem and lists "keep detailed architecture notes separate and linked" as an acceptance criterion. CLAUDE.md (the full agent guide) is untouched, so AGENTS.md still duplicates its Plugin-spec compliance, feedpak-spec compliance, and Versioning sections, and AGENTS.md links to it nowhere — the detailed architecture notes the criterion wants "separate and linked" stay unreferenced.
Technical details
# Reconcile AGENTS.md with CLAUDE.md
## Affected sites
- `AGENTS.md:9-52` — Plugin-spec/feedpak-spec compliance sections byte-near-identical to `CLAUDE.md:9-30` and `CLAUDE.md:244-264`.
- `CLAUDE.md:266-278` — Versioning section still says `0.1.x` and keeps the cache-busting rationale that `AGENTS.md:89` dropped; the two files now state different patch patterns.
- `AGENTS.md` — no reference to `CLAUDE.md` anywhere, so readers of the trimmed guide never reach the detailed notes.
## Required outcome
- Either point `AGENTS.md` at `CLAUDE.md` for the detailed architecture notes (satisfying "separate and linked") or reconcile the duplicated sections so the two files cannot drift.
## Open questions for the human
- Is `CLAUDE.md` intended to remain the full guide, or should it be trimmed to a pointer at `AGENTS.md`? That decides whether the fix is a link or a deletion.ℹ️ Fork-oriented checkboxes survive in the PR template
.github/PULL_REQUEST_TEMPLATE.md:13-18 still asks contributors to mark a change "relevant for this fork only", "core repository", or "both core and fork", and to say whether it "should be upstreamed to the core repository". These are the same fork/upstream assumptions this PR strips from AGENTS.md, so a contributor filling out a PR is still told the repo is a fork. It sits outside the issue's literal AGENTS.md scope, but it's cheap to fix in the same pass.
Technical details
# Stale fork guidance in the PR template
## Affected sites
- `.github/PULL_REQUEST_TEMPLATE.md:13-18` — "Repository Relevance" checklist and the "upstreamed to the core repository" sentence assume a fork/canonical split that does not exist.
## Required outcome
- Drop the fork/core relevance section (or rewrite it to reflect the single canonical repo), matching the new `AGENTS.md` `## Repository & PR target` wording.big-pickle (free) ｜ 𝕏
Contributor
Author
|
The failing check was The one genuinely outstanding review item was a Pullfrog review thread asking to reconcile the Working tree is clean;
|
Address review feedback on the canonical-repo guidance: - Reproducible setup now covers the host deps (PyYAML/pyguitarpro) whose absence aborts pytest collection, and the required `FEEDPAKR_REQUIRE=host,ffmpeg` test command linked to tests/SKIPS.md - Specs bullet links the upstream got-feedBack repos and distinguishes the vendored assets/schemas - Version and changelog policies now require a bump/entry for every functional source change (matching the live CI gates) - FFmpeg integration notes it is required in CI, not just optional - Workflow step 1 links CLAUDE.md/README.md architecture notes; step 7 and completion criteria keep requested edits instead of reverting them - Drop the fork/upstream "Repository Relevance" section from the PR template
cchacs
self-requested a review
October 17, 2026 12:50
cchacs
approved these changes
Oct 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
AGENTS.md stated this plugin is not a fork but still prescribed upstream-first PRs, linked fork PRs, and rebasing onto a canonical repository. This gave agents unnecessary/conflicting decisions.
Changes
get-flashbacks/feedBack-plugin-feedpakr, defaultmain).github/PULL_REQUEST_TEMPLATE.mdReview follow-ups (commit
3061c85):pyguitarpro,PyYAML) whose absence aborts pytest collection, and the requiredFEEDPAKR_REQUIRE=host,ffmpegtest command, linked totests/SKIPS.mdgot-feedBackrepos and distinguishes the repo's vendoredassets/schemas/*.jsonCLAUDE.md/README.mdarchitecture notes; step 7 and completion criteria keep requested edits instead of reverting themThis aligns with the repo's actual structure (canonical repo, not a fork).
Acceptance criteria
Fixes #83.
big-pickle(free) ｜ 𝕏Summary by Sourcery
Align repository and agent documentation with the canonical workflow and make development, verification, and completion requirements explicit.
Enhancements:
Documentation: