mirror of https://github.com/got-feedBack/feedBack.git synced 2026-09-12 18:48:33 +00:00

Files

T

Miguel_LZPFandBret Mogilefsky b45751164f fix: address PR #332 review feedback

Addresses 12 of 13 review comments from Copilot and CodeRabbit on
PR #332. One comment (no-manifests in validate-plugins.yml) is
declined and answered inline; the rest are applied here.

Substantive fixes:

- .github/workflows/validate-plugins.yml — add --noconftest to the
  schema-tests step. tests/conftest.py imports structlog at module
  level, but the CI job only installs requirements-test.txt
  (pytest/httpx/jsonschema), so pytest collection would fail at
  conftest import. The schema tests don't use shared fixtures, so
  skipping conftest is safe and avoids dragging the full runtime
  requirements into a 2 KB validation job. (Copilot)

- schema/plugin.schema.json — tighten the server_files regex on both
  settings.server_files and diagnostics.server_files to match the
  runtime _validate_relpath rules in plugins/__init__.py. The
  previous regex only blocked absolute paths, drive letters,
  backslashes, and "..". The runtime also rejects "//", "./",
  "/./", and leading-dotfile segments. Schema-valid manifests are
  now also load-time-valid. Verified the regex against 12 cases:
  the 3 in-tree manifests still validate. (Copilot)

- .claude/skills/plugin-validate/SKILL.md — add a per-iteration
  plugin_ok flag so we no longer print "OK <path>" after an earlier
  FAIL in the same manifest. Schema-pass + id-mismatch previously
  produced both FAIL and OK lines for one plugin. (CodeRabbit)

- docs/websocket-protocol.md — clarify song_info.tuning array length
  is source-dependent (typically 6 guitar, 4 bass, but extended-range
  GP imports can be 7/8/5/6). Recommend highway.getStringCount() for
  the authoritative count. Line 30 already said this; the table row
  on line 12 was the stale half. (CodeRabbit)

Trivial fixes:

- .claude/rules/plugin-author.md — "wants included" -> "wants to
  include" in the settings.server_files rule. (CodeRabbit)

- Markdown MD040 — add `text` language tags to 7 bare-fence code
  blocks across AGENTS.md, docs/PLUGIN_AUTHORING.md,
  docs/testing-plugins.md, docs/plugin-logging.md, .claude/README.md,
  .claude/agents/slopsmith-reviewer.md, and
  .claude/skills/plugin-validate/SKILL.md (two fences). (CodeRabbit)

Declined:

- .github/workflows/validate-plugins.yml no-manifests -> exit 0
  (CodeRabbit suggested exit 1). Plugins in this repo are in-tree,
  not submodules (no .gitmodules, git submodule status empty), and
  the workflow has a path filter on plugins/**/plugin.json so it
  only runs when a manifest actually changes. Exit 0 is correct.
  Answered inline on the PR.

Verification:
  pytest tests/test_plugin_schema.py -v --noconftest    # 8 passed
  python -c "import json,glob,jsonschema; s=json.load(open('schema/plugin.schema.json')); [jsonschema.validate(json.load(open(p)), s) for p in sorted(glob.glob('plugins/*/plugin.json'))]"
  # ok — all 3 in-tree manifests validate against tightened schema
Signed-off-by: Miguel_LZPF <mgcdreamer@gmail.com>

2026-06-18 00:38:51 -07:00

4.4 KiB

Raw Blame History

name, description, globs

name

description

globs

plugin-author

Rules that apply when editing files under plugins/**. Enforces the plugin contracts documented in docs/PLUGIN_AUTHORING.md.

plugins/**

Plugin authoring rules

These rules apply only when editing files under plugins/**. They encode the contracts described in docs/PLUGIN_AUTHORING.md so AI suggestions don't drift from them.

Manifest

plugin.json is required and must validate against schema/plugin.schema.json. Required fields: id, name. The id must match the parent directory name (the loader keys discovery by directory; drift breaks plugin lookup).
License must come from the curated allowlist if the plugin is intended for the curated list. See CONTRIBUTING.md "Plugin licensing".
type: "visualization" requires a script field exporting window.slopsmithViz_<id>. See docs/plugin-visualization-contracts.md.

Backend (`routes.py`)

Use context["log"], never print() or traceback.print_exc(). The CI workflow blocks print( and traceback.print_exc( in server.py / lib/; plugin code should follow the same rule. The provided logger is a stdlib logging.Logger namespaced to slopsmith.plugin.<id> with correlation IDs, JSON mode, and rotation already wired. See docs/plugin-logging.md.
Multi-file plugins must use context["load_sibling"]("<module>"), not bare from <module> import X. Two plugins shipping a same-named helper collide via sys.modules. See docs/plugin-sibling-imports.md.
setup(app, context) is the required entry. Don't run side effects at import time.

Frontend (`screen.js`)

Wrap in an IIFE — (function () { 'use strict'; ... })();. Frontend scripts share global scope; leaking variables collides with other plugins.
Hook window.playSong carefully — always call the original, always await it. Wrappers run outermost-first; awaiting yields to the event loop and WebSocket messages can arrive before the outer wrapper finishes setup. Use highway.getSongInfo() as a fallback rather than relying solely on _onReady.
Hook window.showScreen — clean up your plugin's state when the user leaves the player screen.
Use window.slopsmith.emit / on for cross-plugin communication. Don't poll other plugins' globals.
Register shortcuts with window.registerShortcut({ key, scope, handler }) and clean up with window.unregisterShortcut(key, scope) — pass the same scope you registered with (default 'global' won't match 'player' / 'plugin-*'). For panel-scoped registries, prefer panel.clearShortcuts(). See docs/plugin-keyboard-shortcuts.md.

State and config

localStorage keys must be prefixed with the plugin id to avoid collisions.
settings.server_files declares config-dir paths the plugin wants to include in the Settings export/import flow. Relpaths only — no .., no abs paths, no backslashes. See docs/plugin-manifest.md.
diagnostics.server_files / diagnostics.callable declares what enters the Export Diagnostics bundle. Keep payloads under 100 KB and don't include secrets. See docs/plugin-diagnostics.md.

Visualization specifics

When plugin.json declares "type": "visualization":

Factory must be window.slopsmithViz_<id> where <id> matches plugin.json.
Factory must return a fresh object on each call — splitscreen creates N instances.
The renderer owns its getContext() call. Declare contextType: '2d' or 'webgl2' on the returned object so the highway can swap the canvas element when needed (getContext is one-shot per canvas).
draw(bundle) receives difficulty-filtered arrays — never read from _filteredNotes or other internals.

See docs/plugin-visualization-contracts.md for the full lifecycle and the Overlay + Note-state-provider contracts.

Testing

When changing plugin internals, add or update a test under tests/ (Python) or tests/js/ (Node) or tests/browser/ (Playwright). See docs/testing-plugins.md for fixtures (isolate_logging, reset_plugin_state).

4.4 KiB Raw Blame History