diff --git a/.gitignore b/.gitignore index 91840b5..f487961 100644 --- a/.gitignore +++ b/.gitignore @@ -89,7 +89,9 @@ build/android/overlay.json # into scripts/gitea-release.sh; the release page is the changelog. .release-notes.md -# Agent session log: local scratch, not repo memory (that is CLAUDE.md -# and .planning/). Written by the scheduled backlog runs. +# Agent session log and loop state: local scratch, not repo memory +# (that is CLAUDE.md and .planning/). journal is written by the +# scheduled backlog runs; loop/ is the autonomous loop's index and flags. .pi/journal.md .pi/schedule-prompts.json +.pi/loop/ diff --git a/.pi/agents/yj-loop/diffreview.md b/.pi/agents/yj-loop/diffreview.md new file mode 100644 index 0000000..38b822a --- /dev/null +++ b/.pi/agents/yj-loop/diffreview.md @@ -0,0 +1,25 @@ +--- +name: diffreview +package: yj-loop +description: Scope-tight review of a loop PR's diff for correctness within the plan's stated scope. The understood-diff half of the critique fan-out. +model: qwen/deepseek-v4-pro-0813 +thinking: medium +tools: read, bash, grep, find +systemPromptMode: replace +inheritProjectContext: true +defaultContext: fresh +--- + +You review a backlog-loop branch's diff for correctness within the +scope the plan claimed. This is the tight review: does the code do what +the plan said, correctly, without grabbing anything it said it would +not. + +Read the issue, the plan comment, and the diff itself. Check each hunk: +correctness of the logic, the repo's conventions as `CLAUDE.md` states +them, tests added or extended, and whether the changed surface matches +its own documented contracts (bindings generated when signatures +changed, events emitted through `events.Emit`, lint grammar). Report: +**blockers**, **fix-worthy**, **optional**, with file and line, and the +smallest safe fix per item. Do not modify files. Do not re-litigate the +plan's scope choices — flag a scope creep, do not redesign it. \ No newline at end of file diff --git a/.pi/agents/yj-loop/escalate.md b/.pi/agents/yj-loop/escalate.md new file mode 100644 index 0000000..231abdb --- /dev/null +++ b/.pi/agents/yj-loop/escalate.md @@ -0,0 +1,29 @@ +--- +name: escalate +package: yj-loop +description: The loop's ceiling — re-runs a leg the two lower tiers failed, seeded with their written failure summaries. Fresh session, never parallel, once a day. +model: go/kimi-k3 +thinking: max +systemPromptMode: replace +inheritProjectContext: true +defaultContext: fresh +skills: + - yellowjacket-dev +--- + +You are the escalation tier of the YellowJacket backlog loop. Both +lower tiers already failed at the leg you are here for; you receive +their written summaries (what each tried, what failed, what was +observed) plus the original leg contract from the orchestrator. + +Start from the summaries, not from the original problem — they exist so +you are not anchored on the failed approaches. Read `CLAUDE.md` and +`.planning/NOTES.md` yourself: the trap that defeated them is usually +written in one of those two. `yellowjacket-dev` tells you how to run +the harness tiers. + +You may delegate mechanical subtasks, never the leg. You produce the +same output the original leg contract demands — this is a re-run of the +leg, not a report about it. The loop spends you once per day; make the +evidence count: name exactly what was different this time and why it +cannot regress. \ No newline at end of file diff --git a/.pi/agents/yj-loop/inspect.md b/.pi/agents/yj-loop/inspect.md new file mode 100644 index 0000000..f9c15f7 --- /dev/null +++ b/.pi/agents/yj-loop/inspect.md @@ -0,0 +1,33 @@ +--- +name: inspect +package: yj-loop +description: Mechanical gatherer for the backlog loop — dumps tracker, PR, CI and branch state verbatim into a digest. No judgement, no writes beyond the digest. +model: go/mimo-v2.5 +thinking: off +tools: read, bash, grep, find +systemPromptMode: replace +inheritProjectContext: true +defaultContext: fresh +progress: true +--- + +You gather state for the YellowJacket backlog loop. You are the eyes of +the orchestrator: nothing you produce may be an opinion, and you never +edit the repo or the tracker. + +Given a request for state, produce a digest with exactly these sections, +verbatim where the source is machine output: + +- **Issues** — `scripts/issue.sh list | search` output as relevant. +- **Pull requests** — from the REST API, open PRs with head sha and + status. +- **CI** — latest runs for the branch/PR requested (REST API; the + `gitea_ci` tool's job_logs 404s on this instance, the REST endpoints + answer). +- **Branches** — `git ls-remote --heads origin`, grepped as asked. +- **State file** — `.pi/loop/state.json` contents, untouched. + +Conventions: env `GITEA_TOKEN` is required; API base +`https://git.ljones.me/api/v1/repos/yonlu/yellowjacket`. If a source +fails, report the failure exactly — never guess its contents. Keep the +digest compact; raw output over prose. \ No newline at end of file diff --git a/.pi/agents/yj-loop/plan.md b/.pi/agents/yj-loop/plan.md new file mode 100644 index 0000000..24e8297 --- /dev/null +++ b/.pi/agents/yj-loop/plan.md @@ -0,0 +1,33 @@ +--- +name: plan +package: yj-loop +description: Writes the implementation plan for a claimed backlog issue, as a tracker comment. Designs on the repo's real shape, not from first principles. +model: glm/glm-5.3 +thinking: high +tools: read, bash, grep, find, write +systemPromptMode: replace +inheritProjectContext: true +defaultContext: fresh +skills: + - yellowjacket-dev +--- + +You write the implementation plan for one claimed YellowJacket issue. +The plan becomes a comment on the issue; you do not push, claim, or +implement. + +Read in order: `CLAUDE.md` (the constraints are load-bearing; where it +explains *why* a shape exists there is usually a test pinning it), +`.planning/NOTES.md` (rejected approaches are rejected forever — do not +resurrect one), `.planning/plans/active/`, `.pi/journal.md`, then the +issue and any comments on it. Skip nothing on the grounds that the +issue looks small: most of this repo's traps are written in exactly one +of those places. + +The plan states: the change in one sentence; the files and components +it touches; the verification tiers the change demands (per the +`yellowjacket-dev` skill's table — name them all, a skipped tier is a +claim not a hope); what is deliberately out of scope; and the risks you +actually see. If the work is materially larger than the issue reports, +say so instead of planning around it. Keep it to a screen; the worker +reads this cold. \ No newline at end of file diff --git a/.pi/agents/yj-loop/review.md b/.pi/agents/yj-loop/review.md new file mode 100644 index 0000000..501fe5f --- /dev/null +++ b/.pi/agents/yj-loop/review.md @@ -0,0 +1,28 @@ +--- +name: review +package: yj-loop +description: Fresh-context consequences review of a loop PR — what breaks that the diff did not say. Advisory only; findings, never edits. +model: glm/glm-5.3 +thinking: medium +tools: read, bash, grep, find +systemPromptMode: replace +inheritProjectContext: true +defaultContext: fresh +--- + +You review a backlog-loop change for unintended consequences, from a +cold read of the repo. Parameterize nothing on the worker's own +reasoning; you inspect the diff itself. + +Read: the issue, its plan comment, `CLAUDE.md`'s load-bearing shapes, +and the branch diff against origin/main. Then enumerate, each with file +and line: **blockers** (wrong, or breaks something the issue did not +ask to break), **fix-worthy** (would not ship with it if it were yours), +**optional**. For every fix-worthy item, the smallest safe change. + +Your angles: does it violate a shape `CLAUDE.md` calls load-bearing; do +other call sites of the same surface break; do the tests assert the +behaviour or the plumbing; does any event's cost change (events carry +meaning in this app — an expensive event reused cheaply is a defect); +did anything non-obvious change owners. Do not modify files. Ignore +style dust unless it hides a bug. \ No newline at end of file diff --git a/.pi/agents/yj-loop/scribe.md b/.pi/agents/yj-loop/scribe.md new file mode 100644 index 0000000..a5310e5 --- /dev/null +++ b/.pi/agents/yj-loop/scribe.md @@ -0,0 +1,27 @@ +--- +name: scribe +package: yj-loop +description: The loop's clerk — commit messages, PR bodies, journal and changelog-sized entries, written from supplied facts. Prose only. +model: go/mimo-v2.5 +thinking: off +tools: read, bash, write, edit +systemPromptMode: replace +inheritProjectContext: true +defaultContext: fresh +--- + +You write the loop's prose. The orchestrator supplies the facts; you +shape them; you decide nothing. + +Forms you produce: Conventional Commit messages (imperative subject, +≤72 chars, body explains *why*, `Closes #n` one per line as instructed +— exactly the lines you are given), PR bodies (what the issue was, what +changed and why, which verification tiers ran with results, what was +deliberately not done, commit-to-issue table), `.pi/journal.md` entries +(facts: what was done, verified, left open), and `CLAUDE.md` updates +when told a shape changed (in that file's voice — load-bearing +paragraphs, never bullet lists of trivia). + +Never invent a fact: a tier result you were not given is not run. Never +rephrase a `Closes` line. Keep every form compact; this repo's prose +density is a feature. \ No newline at end of file diff --git a/.pi/agents/yj-loop/select.md b/.pi/agents/yj-loop/select.md new file mode 100644 index 0000000..3dccc4c --- /dev/null +++ b/.pi/agents/yj-loop/select.md @@ -0,0 +1,34 @@ +--- +name: select +package: yj-loop +description: Picks the single next issue the backlog loop should take. Judgment leg on the tracker state; writes nothing to the tracker itself. +model: glm/glm-5.3 +thinking: medium +tools: read, bash, grep, find +systemPromptMode: replace +inheritProjectContext: true +defaultContext: fresh +skills: + - yj-loop + - yellowjacket-dev +--- + +You choose which one issue the YellowJacket backlog loop works next. You +are given a fresh tracker digest. You write nothing to the tracker; the +orchestrator claims. + +Read the selection rules in the `yj-loop` skill (priority order, #73's +sequence, busy states, collisions, verifiability, flakes, emulator +flag), then answer with exactly one of: + +- `#n — ` and five lines of why this one beats the runner-up + (mentioning #73's phase if it speaks); +- `nothing qualifies` with the reason, if the open list is genuinely + empty of actionable work. + +Rules that decide, in order of weight: `Priority/*` tier; #73's +explicit sequence; `Reviewed/Confirmed`; `Kind/Bug` over Enhancement +over Feature; verifiable in the tiers available (the emulator flag in +`.pi/loop/state.json` widens the ladder; device-only never reaches it); +no existing branch or open PR for it; nobody holds the claim. Pick one. +Uncertainty about the tracker state is a reason to say so, not to guess. \ No newline at end of file diff --git a/.pi/agents/yj-loop/validate.md b/.pi/agents/yj-loop/validate.md new file mode 100644 index 0000000..baf3ca4 --- /dev/null +++ b/.pi/agents/yj-loop/validate.md @@ -0,0 +1,30 @@ +--- +name: validate +package: yj-loop +description: Checks that the implemented work actually answers the issue's claim, against the acceptance evidence. Claim-first validation before any review. +model: glm/glm-5.3 +thinking: medium +tools: read, bash, grep, find +systemPromptMode: replace +inheritProjectContext: true +defaultContext: fresh +skills: + - yellowjacket-dev +--- + +You validate one issue's implemented work — the branch diff, the +worker's handoff, and the issue itself — before review and merge. + +Method: read the issue first and write down what would have to be true +for it to be answered. Then read the diff and the handoff, and check +each item against real evidence: command output, test names, files +touched. Green suites that never touch the reported surface are +findings, not passes. A tier the change demands but the handoff +does not show is a gap, regardless of what else is green. Anything +visual was checked by a model that can see; if no screenshot evidence +exists for a cosmetic change, say so. + +Output: a verdict — `pass`, `pass with nits` (nits listed), `fail` — +with each acceptance item marked met/unmet/unevidenced and the reason +in one line. You do not edit files. You do not trust the diff's self +description; you read it. \ No newline at end of file diff --git a/.pi/agents/yj-loop/visual.md b/.pi/agents/yj-loop/visual.md new file mode 100644 index 0000000..aa866ed --- /dev/null +++ b/.pi/agents/yj-loop/visual.md @@ -0,0 +1,27 @@ +--- +name: visual +package: yj-loop +description: Reads screenshots of the app for the loop — the only leg allowed to judge pixels. What the image actually shows, not what the change claims. +model: glm/glm-5.3-flash +thinking: minimal +tools: read, bash +systemPromptMode: replace +inheritProjectContext: true +defaultContext: fresh +skills: + - yellowjacket-dev +--- + +You are the loop's eyes. You look at screenshots the orchestrator gives +you (paths, or the running app's captures) and say what is actually in +them. + +Report, per image: the view and state shown, whether the element the +issue is about is present and correct, anything clipped, misaligned, +missing or contradictory — measured against the issue's description, +not against the change's claim. Where the harness provides before/after +pairs, read the difference. Be specific in pixels. + +You never edit code and never run the app tier yourself; you read +images and report. If an image is missing or cannot be read, say so — +that is evidence the validator needs, not a reason to guess. \ No newline at end of file diff --git a/.pi/agents/yj-loop/work.md b/.pi/agents/yj-loop/work.md new file mode 100644 index 0000000..33fea41 --- /dev/null +++ b/.pi/agents/yj-loop/work.md @@ -0,0 +1,35 @@ +--- +name: work +package: yj-loop +description: The loop's implementer — builds the claimed issue from its plan comment, in the loop worktree, runs the tiers the change demands, and hands off with evidence. The single writer. +model: qwen/deepseek-v4-pro-0813 +thinking: high +systemPromptMode: replace +inheritProjectContext: true +defaultContext: fresh +skills: + - yellowjacket-dev +--- + +You implement one YellowJacket issue from its plan comment, in the loop +worktree, on the claimed branch. You are the only writer. You do not +claim issues, do not open or merge PRs, do not push without being told +the PR contract is next. + +Read in order: `CLAUDE.md`, `.planning/NOTES.md`, then the issue, its +plan comment, and the claim comment (which names the branch). Implement +what the plan says and nothing else. Match surrounding style. Follow +`CLAUDE.md`'s shapes rather than reasoning from first principles. + +Verification is the `yellowjacket-dev` skill's tier table, all of the +tiers the change demands, run by you in this worktree. Before the e2e +tier check the harness port is free; if it is not, stop and say so — +never attach to another tree's app. Anything you discover that the +issue did not ask for becomes a new issue (`scripts/issue.sh new`), +never a bigger diff. If the work turns out materially larger than the +issue and plan say, stop and write what you found; do not hail-mary. + +Hand off with: changed files, what was left undone and why, every +command run with its exit code, the verification evidence, surprises, +and any decision that needs the orchestrator. A handoff missing any of +that is a failed leg; the orchestrator cannot act on prose alone. \ No newline at end of file diff --git a/.pi/chains/loop-critique.chain.json b/.pi/chains/loop-critique.chain.json new file mode 100644 index 0000000..28527dc --- /dev/null +++ b/.pi/chains/loop-critique.chain.json @@ -0,0 +1,28 @@ +{ + "context": "fresh", + "chain": [ + { + "parallel": [ + { + "agent": "yj-loop.review", + "phase": "Critique", + "label": "Consequences", + "as": "consequences", + "task": "Fresh-context consequences review of the loop's pending change. Issue, plan comment and branch: {task}. Read the issue, the plan comment, CLAUDE.md's load-bearing shapes, and the branch diff against origin/main. Enumerate blockers / fix-worthy / optional with file and line, smallest safe fix per item. Do not modify project/source files; returning findings through the configured output artifact is allowed.", + "output": "critique/consequences.md", + "outputMode": "file-only" + }, + { + "agent": "yj-loop.diffreview", + "phase": "Critique", + "label": "Scope", + "as": "scope", + "task": "Scope-tight review of the loop's pending change. Issue, plan comment and branch: {task}. Read the issue, the plan comment and the diff. Does the code do what the plan said, correctly, within its claimed scope? Blockers / fix-worthy / optional with file and line, smallest safe fix per item. Do not modify project/source files; returning findings through the configured output artifact is allowed.", + "output": "critique/scope.md", + "outputMode": "file-only" + } + ], + "concurrency": 2 + } + ] +} \ No newline at end of file diff --git a/.pi/prompts/loop-tick.md b/.pi/prompts/loop-tick.md new file mode 100644 index 0000000..ab22c45 --- /dev/null +++ b/.pi/prompts/loop-tick.md @@ -0,0 +1,26 @@ +--- +description: One tick of the autonomous YellowJacket backlog loop +--- + +You are the orchestrator of the YellowJacket backlog loop, waking for +one tick. Work in this directory. Read `.pi/skills/yj-loop/SKILL.md` +first — it is the operating procedure and it binds you. The design +questions are answered in `.planning/plans/active/020-autonomous-backlog-loop.md`; +the skill is what you run. + +One tick means: + +1. Take the lock, reconcile, pick exactly one leg, execute it, journal, + release the lock. +2. Delegate every deliberative leg to its `yj-loop.*` agent by name — + the model is pinned in the agent file, never an argument. You hold + only claim, shipping polls, merge, housekeep. +3. Touch only what the loop created. If any rail in the skill is + untestable right now, the tick stops before acting, not after. +4. If the scheduler fires while you are mid-answer, finish this tick + only. Two ticks never overlap; the lock is yours. + +Then report in three lines: the issue taken or continued, its state +after this tick, and any anomaly. Stop. Do not start another tick, do +not re-schedule, do not merge anything that is not in the state file as +this loop's own. \ No newline at end of file diff --git a/.pi/skills/yellowjacket-dev/references/fixtures.md b/.pi/skills/yellowjacket-dev/references/fixtures.md index 52d96f7..de0f32b 100644 --- a/.pi/skills/yellowjacket-dev/references/fixtures.md +++ b/.pi/skills/yellowjacket-dev/references/fixtures.md @@ -42,11 +42,16 @@ strings and identical specs produce different bytes on different builds. playback and then clicks pause races the track ending and fails against a correct UI. Use `LONG_TRACK` (90 s, `edge-lengths`) exported from `e2e/support/fixtures.ts`. -- **WAV tracks scan in untitled.** `backend/tagwriter` writes WAV tags - into a RIFF `id3 ` chunk and `dhowden/tag` has no RIFF parser, so - there is no "Field Recordings" artist in the Artists view. This is a - known open bug pinned by `TestWAVTagsAreNotReadableYet`; do not - "fix" a spec by asserting the broken behaviour elsewhere. +- **WAV tracks scan like every other format.** #104 added + `backend/riff`, so the scan reads the `id3 ` chunk `backend/tagwriter` + writes and both WAVs come in fully tagged: "Field Recordings" is an + ordinary artist in the Artists view, with a "Test Tones" album and a + cover. They are therefore not an example of an untitled or albumless + track — the only two tracks with no album are + `unsorted/no-tags-at-all.mp3` and `unsorted/title-only.mp3`. Prose + written before #104 says the opposite and names + `TestWAVTagsAreNotReadableYet`, a test that change deleted; that is + dated history rather than a description of the app. ## Seeds diff --git a/.pi/skills/yj-loop/SKILL.md b/.pi/skills/yj-loop/SKILL.md new file mode 100644 index 0000000..b6cea17 --- /dev/null +++ b/.pi/skills/yj-loop/SKILL.md @@ -0,0 +1,273 @@ +--- +name: yj-loop +description: Operating the autonomous backlog loop — the crank that works the YellowJacket tracker one issue at a time (tick mechanics, the state machine in Gitea, which agent and model take each leg, the escalation ladder, merge authority and the rails that stop it doing damage). Use whenever a scheduled tick fires, and when piloting or debugging the loop. +--- + +# The YellowJacket backlog loop + +Design and arguments: `.planning/plans/active/020-autonomous-backlog-loop.md`. +This skill is the **operating procedure**; the plan is the reasoning. +`yellowjacket-dev` is the harness doctrine (tiers, seeds, traps); this +skill is the loop doctrine (who acts, on what model, with what authority). +Read the plan first, once. Then this file every tick. + +## The one-sentence discipline + +**Every leg is a fresh subagent session on a pinned tier; the token, the +tracker and the loop worktree are the only things passed between legs. +Never switch a model mid-session, never let two writers exist at once, +never keep state in a conversation.** + +## Tick skeleton + +A tick is one leg of the state machine, and the leg is picked by +reconciling first. Execute in this order: + +1. **Lock.** `/tmp/yj-loop.lock` holds `pid + start-iso`. If a live + process owns it and is younger than 2 h: exit immediately, report + "tick skipped (lock held)". If the PID is dead, take the lock. + Remove it before every exit. +2. **Reconcile.** Fresh reads, never cached: open issues + (`scripts/issue.sh list`), PRs and CI via the REST API, branches via + `git ls-remote --heads origin`, `.pi/loop/state.json`. GITEA_TOKEN + refusing = the tick reports and exits; the identity rails below are + not optional. +3. **Pick the leg.** See the state machine below; the leg follows the + issue's lifecycle (claim→plan→…→merge→…→housekeep). Exactly one leg. +4. **Execute** — the leg table below says who acts and what they must + return. +5. **Journal** — one line per tick in the state file (issue, leg, result, + tick cost if leg reports it). +6. **Report** — three lines: issue taken or continued, its state now, + anomalies. Then stop. A tick that reports is a tick that can leave a + conversation behind. + +## The state machine + +The tracker is the truth. The state file (`.pi/loop/state.json`, +gitignored) is an index plus flags (`emulator`, `drain`); the tracker +wins every disagreement. + +| Stage | Where it lives | Leg → actor | +|---|---|---| +| selected | nothing written until claim is possible | select | +| in flight | `Status/In Progress`, assignee, comment with branch+approach | claim (orchestrator, `scripts/issue.sh`) | +| plan done | plan as an issue comment | plan | +| implemented | commits on `origin/<branch>` | work | +| validated | handoff + a comment on the issue summarizing evidence | validate (+ visual) | +| critiqued | review findings applied or argued; fix commits on the branch | review + diffreview, fix round by work | +| shipped | PR open, body per the contract, CI green | ship (orchestrator + scribe) | +| merged | PR merged, issue closed (footer verified) | merge (orchestrator) | +| done | diary entries, unclaim happened | diary (scribe) | +| cleaned | stale own branches/PRs handled | housekeep (orchestrator, daily) | + +## Legs and their agents + +Delegation is by agent name; the model is pinned in the agent file and is +**not** an argument. Every leg prompt names: the issue, the evidence so +far (plan comment, handoffs), what the leg must produce, and its stop +rules. Never "go fix it" — the leg contract is in this file. + +| Leg | Agent | Model (tier) | Produces | +|---|---|---|---| +| gather/mechanical dump | `yj-loop.inspect` | go/mimo-v2.5 (T0) | tracker/PR/CI/branch digest, verbatim | +| select next issue | `yj-loop.select` | glm/glm-5.3 (T2) | one issue + reasons, or "nothing qualifies" | +| plan | `yj-loop.plan` | glm/glm-5.3 (T2) | a plan comment on the issue | +| implement | `yj-loop.work` | qwen/deepseek-v4-pro-0813 (T1) | commits + a handoff (see contract below) | +| validate | `yj-loop.validate` | glm/glm-5.3 (T2) | pass/fail with evidence per acceptance item | +| visual evidence | `yj-loop.visual` | glm/glm-5.3-flash (T2) | what the screenshot actually shows | +| consequences review | `yj-loop.review` | glm/glm-5.3 (T2) | blockers / fix-worthy / optional findings | +| understood-diff review | `yj-loop.diffreview` | qwen/deepseek-v4-pro-0813 (T1) | same shape, scope-tight | +| escalation | `yj-loop.escalate` | go/kimi-k3 (T3) | same leg re-run, seeded with failure summary | +| prose (PR body, commit msgs, journal) | `yj-loop.scribe` | go/mimo-v2.5 (T0) | text only, from supplied facts | + +Orchestrator-only legs: **claim** (`issue.sh claim --branch` — atomic, +refuses if held), **ship's PR/CI polling** (REST API below — `gitea_ci` +job_logs 404s on this Gitea; the REST endpoints are the way), **merge** +(API below), **housekeep**. + +## Selection rules (`select`) + +The rules from `.pi/prompts/next-issue.md` stay — priority order, #73's +sequence overriding labels where it speaks, skipping `Status/*` states +that mean busy, branch-collision check, verifiability, flakes. The +emulator flag **adds** emulator-verifiable Android issues; it never +reaches device-only ones. A "nothing qualifies" answer is a correct +tick, not a failure — report it and stop. + +## The implementation contract (`work`) + +The worker implements **from the plan comment**, in the loop worktree, +on the claimed branch, and nothing else: + +- runs the tiers the change demands (`yellowjacket-dev` decides which — + the loop never outvotes it), including `npx tsc --noEmit`; +- e2e only if `ss -ltn | grep 34115` is empty; `make dev-headless + SEED=default` before and `make dev-stop` after; +- discoveries outside the issue become new issues (`issue.sh new`), never + bigger diffs; a materially-larger-than-implied issue stops the leg with + a comment and a label removal, not a hail-mary; +- handoff must state: changed files, what was left undone, commands run + with exit codes, verification evidence, surprises, decisions needing + approval. A handoff without that list is a failed leg. + +## Validate and critique + +Validation is **claim-first**: re-read the issue, then check each piece +of evidence against the acceptance items; a green suite that never +touched the reported surface is a finding. Screenshots go to `visual`, +never to a text-only tier. + +Critique is the standing fan-out (`subagent` parallel: `yj-loop.review` +consequences + `yj-loop.diffreview` scope-tight, both fresh). The +orchestrator synthesizes: blockers and fix-worthy findings go back to +`work` as one bounded fix round (maximum three rounds total; then the +issue gets a `⟦loop⟧` comment stating what will not be fixed and why, +and the ship leg proceeds unless a finding is a blocker). Reviewers do +not edit files. + +## Escalation ladder + +When a leg fails twice on its tier, do not re-prompt bigger: + +1. The failing session writes its summary: what it tried, what failed, + what it observed. +2. A **new** session on the next tier up is seeded with that summary and + the original leg contract. +3. T3 is the ceiling: fresh session, never parallel, **once per day**. + A day's escalation is spent — the issue waits until tomorrow. + +Routing down is free; routing up is the budget. + +## Ship and the PR body contract + +Push the branch (SSH; never to `main`, never force). The PR body — +written by `scribe` from the validator's and reviewers' output — states: +what the issue was, what changed and why, **which verification tiers ran +and their results**, what was deliberately not done, the commit-to-issue +table, and `Closes #n`. `Closes` also sits one-per-line in a commit body +**inside the branch** — both, regardless of merge strategy, because the +pairing was measured. + +Poll CI until `check` and `e2e` finish. On failure: read the log via +`GET /api/v1/repos/yonlu/yellowjacket/actions/runs/<run>/jobs` (per-step) +and `…/actions/jobs/<id>/logs` (full). Fix on the branch. **Two +consecutive identical failures = stop**: comment what is known on the +PR and the issue, leave both, report. Do not burn ticks on a red wall. + +## Merge authority + +Merge when, and only when, **all** hold: + +- the PR was opened by this loop (it is in the state file's index); +- the protection contexts `CI / check` and `CI / e2e` are green on the + PR's head, read from the API, not from the PR page's badge; +- the PR reports mergeable; +- the critique leg ran and no open blocker stands; +- the branch is **not behind `origin/main`** — the protection's + `block_on_outdated_branch: true` refuses it anyway; never + `force_manually_merged` around it. + +**Refresh before every merge.** In the loop worktree: fetch, then +`git merge origin/main` on the PR branch, push. A textual conflict +stops the leg there — as diff text, not as a failed merge click: hunks +the loop authored are resolved by the loop; anything else is left with +`⟦loop⟧` comment for a human, never forced. After any refresh push, +re-poll the PR's own required contexts on the **new head** before +merging. + +**Merges happen one at a time**, each re-reading state — the previous +merge moved `main`, and the next PR's mergeability is recomputed at +its own turn. + +``` +curl -sS -X POST -H "Authorization: token $GITEA_TOKEN" \ + -H "Content-Type: application/json" \ + https://git.ljones.me/api/v1/repos/yonlu/yellowjacket/pulls/<n>/merge \ + -d '{"Do":"merge","merge_message_field":"default","force_manually_merged":false}' +``` + +**Afterwards watch the `push` run on `main`** — the CI the merge +started. A red main after a loop merge is a **halt**: comment what is +known on the offending PR, mark the state file, stop taking new issues. +That run is the only thing between a clean textual merge of +independently-written PRs and a self-contradicting main; no +mergeability check sees it. Only a green main lets the tick proceed (to +footer verification, below). + +Footer verification: `scripts/issue.sh list --state open` and check +the footer took. Close stragglers with `issue.sh close`, naming the +merge commit. `unclaim.yml` handles the label; it is not instant; +reopening does not restore it. Merging fans out to nothing (releases +are the manual `release.yml`, which the loop never runs) — the +criticism stands before the merge because nothing stands after it. + +## Rails — the loop's absolute rules + +1. **Touch only its own.** Issues it claimed, branches it made, PRs it + opened. `issue.sh claim` enforces the front gate; never work around a + refusal. +2. **One writer, one issue.** The loop worktree is the only dirty tree. +3. **Never merge a PR it did not open.** Any merge that violates this is + a hard stop. +4. **Human work is holy.** Human branches, PRs, assignees: leave exactly + as found. Cleanup never names them. +5. **The token is identity.** If GITEA_TOKEN misbehaves, the tick stops. +6. **New findings are new issues**, never scope creep. The tracker + vocabulary (`Kind/`, `Area/`, `Priority/`) stays intact in one + taxonomy; use `scripts/issue.sh new` with correct labels. +7. **Conventional Commits**, enforced by `scripts/commit-check.sh`; the + type list and `.releaserc.yml`'s must agree — a loop commit is a + release grammar token even after months of no manual releases. +8. **Tiers over vibes.** `yellowjacket-dev`'s tier table decides what a + change must pass; a skipped tier is stated, never silent. +9. **Two strikes on CI, three rounds of critique, one kimi a day.** The + loop's patience is finite on purpose. +10. **Every leg writes its evidence.** A leg that leaves nothing behind + is indistinguishable from a leg that did not run — which is how the + next tick re-does it. +11. **The loop may not re-schedule itself** (the scheduler refuses it + anyway — treat as an invariant, not a limitation). +12. **Drain means drain.** `drain: true` = finish in flight, take + nothing new, then stop. + +## Emulator mode + +Flag `emulator: true` in the state file **and** an already-booted +emulator (`adb devices` answers) opts in: `make android` (build), `make +android-install`, `make android-smoke` (crash check — the same pid +surviving is the only signal that means started), `make +android-screenshot` and `make android-eval` as evidence for `visual`. +The loop never boots or stops an emulator; that is the user's machine. +Device-only issues stay open under either setting. One-time setup the +user performs: `make android-setup` (~3.5 GB, creates the `yj-test` +AVD), then `make android-emulator` per session. + +## ON / OFF / drain + +- **Worktree:** `git worktree add ~/.paseo/worktrees/loop/jumpy-hound + origin/main` (from any clone; branch from origin/main in the loop + tree, never `git checkout main`). +- **Session:** pi in that worktree, `/name loop`. Add the job via + `/schedule-prompt` (name `yj-loop`, cron + `0 0 10-18 * * 1-5`, prompt: "Read `.pi/skills/yj-loop/SKILL.md` and + run exactly one tick. Stop.") — session-bound by default. +- **OFF:** toggle the job, or close the session. **ON:** `pi --resume + loop` in the worktree, job enabled. Courses of the tick appear in + that session's transcript. +- **Tune in:** the same resume. Talk to it only between; a tick is + atomic. + +## Troubleshooting + +- `issue.sh: GITEA_TOKEN is not set` or a 401 — the token is the whole + identity (rails 5). Stop, do not fall back to anything. +- `gitea_ci`'s job log 404s — the REST endpoints above answer; this is + a Gitea build, not a fault. +- A spec fails that the tier doc says can fail from stale backend state + — restart the app tier before believing it (`yellowjacket-dev`). +- A tick that "did nothing" — reconcile again; the tracker usually says + which leg it really is. +- The job did not fire — the scheduler fires only while a session is + open in its directory (documented); "the loop is off" is the correct + reading, not a bug. \ No newline at end of file diff --git a/.planning/plans/active/020-autonomous-backlog-loop.md b/.planning/plans/active/020-autonomous-backlog-loop.md new file mode 100644 index 0000000..d857d6c --- /dev/null +++ b/.planning/plans/active/020-autonomous-backlog-loop.md @@ -0,0 +1,242 @@ +# 020 — The autonomous backlog loop + +**Issue:** #236 (`Kind/Enhancement`, `Priority/Low`) +**Status:** active — phase 0, supervised pilot +**Relates:** #73 (the roadmap the loop follows), plan 005 (the harness the +loop drives). Cost and model-tier doctrine is the `pi-session-reference` +card handed to the session that designed this; the loop's copies of it +are deliberate one-paragraph summaries, not the authority. + +A pi coding-agent configuration that, toggled on, works the Gitea tracker +one issue at a time — triage, claim, plan, implement, validate, critique, +PR, CI, merge, verify-close, diary — and then does it again. The tracker is +the state machine: whoever reads Gitea sees exactly where the loop is, +which is the property this document's rails exist to protect. + +--- + +## The shape: a crank, not a resident brain + +Half the design is that **nothing lives in a conversation**. Each tick is a +fresh, bounded unit of work; every transition writes evidence to Gitea +(label, comment, branch, PR) or to the loop's own state file; a tick that +dies mid-leg loses nothing, because the next tick resumes from what Gitea +says. + +The other half is that **no leg trusts the one before it**. The worker +implements from the plan, not from the issue alone; the validator checks +the *claim*, not the green CI row; the merger merges only after reading the +protection contexts itself; the diary leg is what makes the next issue's +triage cheaper. + +One issue in flight at a time. That is a pacing decision, not a +concurrency limit of the tooling — CI has a capacity-1 runner and the e2e +tier owns one headless port on this machine, so two writers would serialize +on infrastructure they cannot see and appear to be doing fine. + +## The state machine + +| Leg | Writes | Actor / model | +|---|---|---| +| reconcile | — | orchestrator + `inspect` (mimo-v2.5) | +| select | nothing on the tracker; decision logged in the tick transcript | `select` (glm-5.3) | +| claim | assignee + `Status/In Progress` + comment naming branch & approach | `scripts/issue.sh claim` | +| plan | plan as an issue comment | `plan` (glm-5.3) | +| implement | commits on the issue branch, in the loop worktree | `work` (qwen/deepseek-v4-pro-0813) | +| validate | verification evidence in the handoff | `validate` (glm-5.3), `visual` (glm-5.3-flash) for screenshots | +| critique | review findings; fix commits | `review` (glm-5.3) + `diffreview` (qwen) + fix round by `work` | +| ship | push, PR with body contract, CI read + fixes | orchestrator + `scribe` (mimo-v2.5) | +| merge | the merge; post-merge issue verification | orchestrator | +| diary | `.pi/journal.md`, `CLAUDE.md` if structural | `scribe` | +| housekeep | stale-branch/PR cleanup, state-file prune | orchestrator | + +### Legs that are the orchestrator's alone + +The orchestrator (the loop session) delegates every deliberative leg and +keeps three for itself because they are script-shaped and must not be +re-implemented by a model: claim (`issue.sh claim`, which refuses when +someone else holds the issue — the backstop), merge (API calls below), and +housekeep (branch deletion). If a tick does nothing else, it reconciles. + +## Model routing + +The routing authority is the card's four tiers, reproduced here as the +loop's assignment, not as an argument: + +- **T0 `go/mimo-v2.5`** — mechanical gathering, commit/PR/journal prose, + any fan-out. Effectively free; wrong only where wrongness costs a + debugging session, so nothing above takes its word for a *fact*. +- **T1 `qwen/deepseek-v4-pro-0813`** — implement-from-a-written-plan, + understood-diff review, the orchestrator itself. The default session + model; half price 10:00–20:00 EDT, which the cron is shaped around. +- **T2 `glm/glm-5.3`** — repo-scale reasoning: selection, planning, + consequences review, validation judgement. Weekly credits with no + rollover: the loop draws them every week by construction, which is the + correct posture. **`glm-5.3-flash`** for anything multimodal + (screenshots, UI inspection). +- **T3 `go/kimi-k3`** — escalation only: two lower tiers already failed, + or the issue is a named gnarly one. A fresh session seeded with the + failing tier's own summary, never a mid-session switch, never parallel, + at most once per day. + +The invariant behind all four, from the card: **routing down is cheap, +routing up is expensive.** An implementation that stalls is escalated by +having the T1 session write *what it tried, what failed, what it observed* +and handing that to a new session one tier up. Escalating a session in +place is forbidden in both directions. + +Fan-out is allowed on T0 and T1 only (the Go plan's $12/5 h constraint +makes T3 fan-out self-defeating). Critique is the one standing fan-out: +two reviewers, two angles, one synthesis. + +## Scheduling + +`0 0 10-18 * * 1-5` (local = EDT): hourly on weekdays inside Qwen's +half-price window, clear of the card's ⚠ 2–6am band (DeepSeek peaks, GLM +loses its off-peak discount — the window the old `yj-backlog` cron sat in, +which this replaces as the loop supersedes it). + +- A tick takes a lock (`/tmp/yj-loop.lock`, PID + timestamp). An overrun + tick makes the next fire exit immediately; serialization survives + whatever the scheduler does with overlapping fires. +- ~9 ticks/day; an issue is 2–5 ticks; **one to two issues per day** is + the natural rate. That also paces the bills without a budget flag. +- The port check is part of reconcile: if `34115` is occupied, the tick + refuses any leg that needs the headless app and defers to the next + tick, without complaint. A human's interactive tier always wins. + +## Runtime and ON/OFF + +The scheduler (`pi-schedule-prompt`) fires only while a pi session is open +in the job's directory — that limitation is the switch: + +- **Worktree:** `git worktree add` a dedicated clone at + `~/.paseo/worktrees/loop/jumpy-hound`. Loop edits happen only there; a + dirty tree there is the loop's business and nobody else's. +- **Session:** pi in that worktree, `/name loop`. The job is bound to that + session, so another pi elsewhere in the same directory does not + double-fire it. +- **ON:** resume the loop session (`pi --resume loop`) and enable the job. + **OFF:** toggle the job off in `/schedule-prompt`, or close the session. + **Drain** (stop taking new work, finish in flight): set `drain: true` in + the state file. +- **Tune in:** the same `pi --resume loop` — the chat transcript *is* the + loop's log, each tick's reasoning inline, each leg reporting in. + +## Identity, claims, and what the loop may touch + +The loop operates **as the owner** via `GITEA_TOKEN` (scopes: `read:user`, +`write:issue`, `write:pull`, `write:repository`); pushes ride SSH and need +no token. Every tracker comment the loop writes is prefixed `⟦loop⟧`, so +the collaborator reads it as the pump and not as a person. + +It may only ever touch work it created: issues it claimed, branches it +made, PRs it opened. Two mechanisms make that enforced rather than +intentional: `issue.sh claim` refuses an issue somebody else holds, and +reconcile checks `git ls-remote --heads origin` so a branch name collision +from a concurrent session is caught before the first edit. + +## Merge lifecycle + +- **Only PRs the loop opened.** A collaborator's PR is never merged, never + commented on for pressure, never touched. +- **Every branch is refreshed against main before its merge**, in the + loop worktree — the refresh is where a textual conflict surfaces, as + diff text: hunks the loop authored are resolved there, anything else + is left to a human with a `⟦loop⟧` comment. The protection's + `block_on_outdated_branch` makes the refresh mandatory for adopted + (pre-loop) branches: behind `main`, a PR cannot merge at all. + Required contexts are re-polled on the refreshed head. +- **Merges are one at a time**, each re-reading state — the previous + merge moved `main`, and the next PR's mergeability is recomputed at + its own turn. +- **Post-merge, the `push` run on `main` is watched.** A red main after + a loop merge halts the loop. That run is the only guard against the + class no mergeability check sees: two PRs touching the same file, + merging cleanly, contradicting each other. +- The gate is the protection rule itself, read from the API: contexts + `CI / check*` and `CI / e2e*` green, PR mergeable. (Required approvals + is 0 today; if a second person changes protection rules, the merge + endpoint refuses and the tick stops and reports — human business.) +- `Closes #n` goes **in a commit body inside the branch, one line per + issue, and in the PR body**. Both, because a squash route and a merge + route parse different texts, and this pairing was measured: a comma + list partially matched, five of ten issues. +- After merging: verify against `issue.sh list --state open` that the + issue actually closed; close any straggler naming the merge commit. + `unclaim.yml` strips `Status/In Progress` automatically; it is not + instant, and a re-open does not restore it — the verification is + against the open list, not against the label. +- Merging to `main` fans out to nothing: releases are the manual + `release.yml`, which this loop never runs. The blast radius of a + merge is the main branch's CI, and the critique leg is what stands + before it. + +## Verification contract + +The tier table is `yellowjacket-dev`'s; the loop re-states nothing above +it except the *division of duty*: the worker runs the tiers the change +demands, and the validator re-reads the issue and checks that the tier +evidence actually answers the claim — a green suite that never touched +the reported surface is a finding, not a pass. Cosmetics are read by a +model that can see (`visual`, the multimodal tier); a change that moves +geometry refreshes its `ui-visual` baseline in the same commit. +`tsc --noEmit` is part of the gate and nothing else runs it. The e2e app +is seeded (`SEED=default`) and stopped after. + +## Android / emulator mode + +The loop is **device-free by default**: issues whose verification is +physical-device behaviour stay open for humans (the repo's own tags say +which those are). One step of the ladder exists for the rest: + +- `{"emulator": true}` in `.pi/loop/state.json` **plus an already-booted + emulator** (`adb devices` answers) opts the loop into building the APK + and using `android-smoke` (crash verification), and `android-screenshot` + / `android-eval` as rendering evidence for `visual`. +- The loop **never boots or stops an emulator** — that is the user's + machine and their gesture. Boot it with `make android-emulator` + (one-time `make android-setup`, ~3.5 GB, creates the AVD), and + `make android-emulator-stop` when done. +- Real-device-only issues are skipped under either setting. + +## Budgets and pacing + +Expected spend: dominated by the T1 implementation leg inside the +half-price window (pennies to tens of cents) and T2 on weekly credits; +T3 bounded at one fresh call per day. The card's numbers ($12 per rolling +5 h, $30/week as burst headroom not allowance, GLM reset weekly) are the +sanity cells; the loop's own weekly check compares against them rather +than against the month. + +## Cleanup (housekeep leg, once per day) + +- Loop-owned branches whose commits are in `origin/main`: deleted, local + and remote. +- Loop-owned PRs open >7 days or red on a second identical CI cause: + commented with what is known (`⟦loop⟧`), and left — never silently + deleted. +- Anything not the loop's (assignee, branch, PR): left exactly as found. + +## Pilot phases + +- **P0 — supervised.** One tick, user watching the transcript: reconcile, + select, claim, plan. No merge. +- **P1 — observed.** Two ticks ending in the loop's first merge, watched + through CI → merge → verify-close. +- **P2 — unattended.** The schedule left on. Weekly check against the + card's two-minute ritual. +- **Hard stops** (any of these halts the loop and leaves a comment, never + a silent retry): a tick dies twice with no explanation; a merge happens + for a PR the loop did not open; spend outside the cells above by 2×. + +## Not now, on purpose + +- **Parallel worktrees** — blocked on e2e's exclusive port; viable only + with per-worktree headless ports or CI-only e2e. The shape ( + supervisor + per-issue worktrees) is the target, not the first cut. +- **Weekend batch refactors** — DeepSeek off-peak is real but is a + scheduling knob on top of a working pump. +- **More chain files** — the critique fan-out is a chain; the rest stay + orchestrator-legs until two weeks of unattended runs say which legs + are actually fixed-shape. \ No newline at end of file diff --git a/CLAUDE.md b/CLAUDE.md index dc3057d..265c5b2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -138,6 +138,17 @@ has started is a second, staler answer to "what are we doing next". Numbering is sequential and stable across status moves (a plan keeps its `NNN-` prefix). Abandoned plans are deleted. +**The autonomous loop** (plan 020, `.pi/skills/yj-loop/`) is the pi +configuration that works the tracker one issue at a time — a cron tick +in a dedicated worktree and session, with the tracker labels as its +state machine. It claims with `issue.sh` like anyone, merges only PRs +it opened once the protection contexts are green, and files what it +finds. Its switch is `.pi/schedule-prompts.json` (gitignored): it runs +only while that pi session is open, and that limitation is the whole +on/off design. Where a loop discovery contradicts this file, this file +is wrong and should be fixed by the diary leg — the loop never quietly +decides otherwise. + ## Commands ```bash @@ -3297,6 +3308,27 @@ its own duplicates apart) — and changing either is invisible against an existing `YJ_HOME`, whose `config.toml` already holds the old list, so `make sandbox-seed NAME=default` before believing the app. +**And the *valid* columns are declared twice too, which is the pair +that drifted.** `tracklist.AllColumnIDs` is what the backend accepts; +`COLUMN_DEFS` is what the frontend knows how to draw, and they are not +the same set — `titleArtist` is a definition and not a choice, since it +is the phone's stacked column and is picked by width in +`PHONE_COLUMN_IDS`. Settings built its list from `Object.keys( +COLUMN_DEFS)` and so offered it: **two rows both called "Track Name"** +(#197), the second unselectable, because ticking it sends a column set +Go rejects with `unknown track-list column ID` and `config-page` +swallows that into a `console.error`. `CONFIGURABLE_COLUMN_IDS` is what +the configurator reads now, derived from a `configurable` flag on the +definition, and `settings-column-list.test.ts` reads Go's own list out +of the source rather than writing it down a third time — the rule being +about every column, so checking one checks nothing. + +One thing it does **not** fix, because it is reachable from any invalid +input rather than from that row: `SetTrackListColumns` assigns before it +validates, so a rejected list stays in memory and `Save()` validates the +whole config — one tick and **no setting saves for the rest of the +session**, silently. That is #231. + **Event-driven communication**: Backend emits events via Wails runtime; frontend stores subscribe to them. Event names are constants in `backend/events/`. `frontend/src/events.ts` is **generated** from `backend/events/events.go` diff --git a/backend/download/provider_ytdlp_test.go b/backend/download/provider_ytdlp_test.go index 34a2720..8f84499 100644 --- a/backend/download/provider_ytdlp_test.go +++ b/backend/download/provider_ytdlp_test.go @@ -7,6 +7,7 @@ import ( "path/filepath" "runtime" "strings" + "syscall" "testing" ) @@ -17,6 +18,21 @@ import ( // stubYtDlp writes an executable script that echoes the given stdout // and returns it as a provider config binary path. +// +// The write is held under syscall.ForkLock, and that is not tidiness: +// the kernel refuses to exec a file that is open for writing anywhere +// in the process, and these tests are parallel, so a *sibling* test's +// fork can duplicate this descriptor in the moment it is open and +// carry it past our close — the exec a moment later then fails with +// ETXTBSY, "text file busy". That is #146, seen once in CI and once +// locally, on trees containing no Go at all. Closing sooner is not +// available (os.WriteFile has already closed the file before anything +// execs it) and O_CLOEXEC does not help, because the window is between +// another goroutine's fork and its own exec. ForkLock is the lock +// syscall.forkExec takes across that fork, so holding it here means no +// child can exist while the descriptor does. Measured on this helper +// under 12 concurrent writers: 176-189 of 2400 execs refused without +// it, 0 of 2400 with it. func stubYtDlp(t *testing.T, script string) string { t.Helper() @@ -26,9 +42,11 @@ func stubYtDlp(t *testing.T, script string) string { path := filepath.Join(t.TempDir(), "yt-dlp") - if err := os.WriteFile( - path, []byte("#!/bin/sh\n"+script), 0o700, - ); err != nil { + syscall.ForkLock.Lock() + err := os.WriteFile(path, []byte("#!/bin/sh\n"+script), 0o700) + syscall.ForkLock.Unlock() + + if err != nil { t.Fatalf("write stub: %v", err) } diff --git a/frontend/src/components/config-page/config-page.ts b/frontend/src/components/config-page/config-page.ts index 522ac2c..502c557 100644 --- a/frontend/src/components/config-page/config-page.ts +++ b/frontend/src/components/config-page/config-page.ts @@ -51,7 +51,7 @@ import type { BackgroundShade } from '@store/theme-store'; import type { IconStyle } from '@store/favorites-store'; import { COLUMN_DEFS, - ALL_COLUMN_IDS, + CONFIGURABLE_COLUMN_IDS, } from '@components/track-list/columns'; import './config-field'; @@ -1640,7 +1640,7 @@ export class ConfigPage extends ViewLifecycleMixin(LitElement) { ...this.trackListCtrl.columnIds, ]; - const disabledIds = ALL_COLUMN_IDS.filter( + const disabledIds = CONFIGURABLE_COLUMN_IDS.filter( (id) => !enabledIds.includes(id), ); diff --git a/frontend/src/components/first-run-wizard/first-run-wizard.ts b/frontend/src/components/first-run-wizard/first-run-wizard.ts index 447ac6b..cdc7072 100644 --- a/frontend/src/components/first-run-wizard/first-run-wizard.ts +++ b/frontend/src/components/first-run-wizard/first-run-wizard.ts @@ -2,12 +2,14 @@ import { LitElement, html, css, nothing } from 'lit'; import { customElement, state, query } from 'lit/decorators.js'; import '@awesome.me/webawesome/dist/components/dialog/dialog.js'; import '@awesome.me/webawesome/dist/components/icon/icon.js'; +import { EventsOn } from '@runtime/runtime'; import { AddLibrary, GetAllLibrariesWithTrackCounts, } from '@go/library/library.js'; import { describeError, explainError } from '@utils/describe-error'; import { nameDialogsIn } from '@utils/name-dialog'; +import { Events } from '../../events'; import { pickDirectory } from '../../utils/pick-directory'; /** @@ -19,6 +21,13 @@ import { pickDirectory } from '../../utils/pick-directory'; * prompting the user to pick their music folder, registers it through the * library CRUD API, and dismisses itself. AddLibrary emits LibraryAdded * and kicks off the initial scan automatically. + * + * **The dismissal follows the library existing, not the button being + * pressed.** `AddLibrary` emits `LibraryAdded` whoever calls it, so the + * wizard waits on the state it exists to wait for rather than on a step + * in its own flow — a library arriving by any other route (Settings, a + * direct call) leaves a full-screen modal up otherwise, intercepting + * every pointer event. */ @customElement('first-run-wizard') export class FirstRunWizard extends LitElement { @@ -37,9 +46,18 @@ export class FirstRunWizard extends LitElement { /** Error message from a failed pick/save, if any. */ @state() private errorMessage = ''; + /** Unsubscribe from LibraryAdded, while this element is connected. */ + private cancelLibraryAdded?: () => void; + override async connectedCallback(): Promise<void> { super.connectedCallback(); + // Subscribed before the read below, so a library arriving while + // that call is in flight is not answered with a stale empty list. + this.cancelLibraryAdded = EventsOn(Events.LibraryAdded, () => { + this.dismiss(); + }); + try { const existing = await GetAllLibrariesWithTrackCounts(); @@ -54,6 +72,8 @@ export class FirstRunWizard extends LitElement { return; } + if (this.finished) return; + this.active = true; await this.updateComplete; @@ -61,6 +81,13 @@ export class FirstRunWizard extends LitElement { if (this.dialog) this.dialog.open = true; } + override disconnectedCallback(): void { + this.cancelLibraryAdded?.(); + this.cancelLibraryAdded = undefined; + + super.disconnectedCallback(); + } + static override styles = css` wa-dialog { --width: 480px; @@ -239,6 +266,20 @@ export class FirstRunWizard extends LitElement { if (!this.finished) e.preventDefault(); }; + /** + * Close, and stay closed: a library exists, so setup is over. + * + * `finished` is set first, or `preventClose` cancels the hide this + * asks for. + */ + private dismiss(): void { + this.finished = true; + + if (this.dialog) this.dialog.open = false; + + this.active = false; + } + private handleChoose = async (): Promise<void> => { this.errorMessage = ''; @@ -264,11 +305,7 @@ export class FirstRunWizard extends LitElement { try { await AddLibrary(this.selectedDirectory); - this.finished = true; - - if (this.dialog) this.dialog.open = false; - - this.active = false; + this.dismiss(); } catch (err) { this.errorMessage = explainError( err, diff --git a/frontend/src/components/track-list/columns.ts b/frontend/src/components/track-list/columns.ts index 059863a..a0f8a38 100644 --- a/frontend/src/components/track-list/columns.ts +++ b/frontend/src/components/track-list/columns.ts @@ -32,6 +32,18 @@ export interface ColumnDef { id: string; /** Human-readable header label. */ label: string; + /** + * Whether Settings may offer this column. Defaults to true. + * + * A definition is not the same thing as a *choice*. `titleArtist` + * is the phone's stacked column, picked by width in + * `PHONE_COLUMN_IDS`, and `tracklist.AllColumnIDs` in Go does not + * list it — so a tick in the configurator sends a column set the + * backend rejects with `unknown track-list column ID`, the tick + * reverts on the next render, and the only trace is a + * `console.error` (#197). + */ + configurable?: boolean; /** Extracts the display value from a track. */ accessor: (track: library.Track) => string; /** Default CSS width (used when no saved width exists). */ @@ -98,11 +110,16 @@ export const COLUMN_DEFS: Record<string, ColumnDef> = { }, titleArtist: { id: 'titleArtist', - // Named for what it sorts by, since that is the only place the - // label is user-visible: the phone has no column headers, and - // the page header's sort list is built from the *configured* - // columns rather than the drawn ones. + // Named for what it sorts by. That label is drawn nowhere + // today: the phone has no column headers, and the page header's + // sort list is built from the *configured* columns, which this + // one can never be — see `configurable` below. label: 'Track Name', + // Chosen by width, never by the user, and rejected by the + // backend if it ever were. #197: Settings listed it anyway, so + // there were two rows called "Track Name" and the second one + // could not be selected. + configurable: false, accessor: (t) => t.TrackName, defaultWidth: '1fr', comparator: (a, b) => compareStr(a.TrackName, b.TrackName), @@ -266,10 +283,15 @@ export const COLUMN_DEFS: Record<string, ColumnDef> = { }; /** - * All column IDs in default display order. - * Used by the settings UI to list available columns. + * The column IDs Settings may offer, in default display order. + * + * Not every definition is one: a column the user cannot choose has no + * row in the configurator, because a checkbox that cannot change + * anything is worse than an absent one — see `ColumnDef.configurable`. */ -export const ALL_COLUMN_IDS: string[] = Object.keys(COLUMN_DEFS); +export const CONFIGURABLE_COLUMN_IDS: string[] = Object.keys( + COLUMN_DEFS, +).filter((id) => COLUMN_DEFS[id]?.configurable !== false); /** * Column IDs that are always searched regardless of visibility. diff --git a/frontend/test/components/first-run-wizard.test.ts b/frontend/test/components/first-run-wizard.test.ts new file mode 100644 index 0000000..c2d6244 --- /dev/null +++ b/frontend/test/components/first-run-wizard.test.ts @@ -0,0 +1,123 @@ +/** + * #175: the first-run wizard's dismissal follows the library existing, + * not its own button being pressed. + * + * The wizard is a modal that blocks every pointer event, so a library + * arriving by another route — Settings, a direct call — used to leave + * it up over an app that was already set up. `LibraryAdded` is emitted + * by `AddLibrary` whoever calls it, which is what makes one + * subscription the whole fix. + */ +import { beforeEach, describe, expect, it } from 'vitest'; + +import { Events } from '../../src/events'; +import { emit, stub } from '../support/harness'; +import { fixture, shadow, shadowAll } from '../support/render'; +import { wails } from '../support/wails-fake'; + +import '@components/first-run-wizard/first-run-wizard'; + +import type { FirstRunWizard } from '@components/first-run-wizard/first-run-wizard'; + +/** A library row, as `GetAllLibrariesWithTrackCounts` returns one. */ +const aLibrary = { + id: 1, + name: 'Music', + path: '/home/logan/Music', + trackCount: 9, +}; + +/** Mount the wizard on a fresh install: no libraries yet. */ +async function wizardOnAFreshInstall(): Promise<FirstRunWizard> { + stub('library.Library.GetAllLibrariesWithTrackCounts', []); + + return fixture<FirstRunWizard>('first-run-wizard'); +} + +/** Whether the wizard is rendering its modal at all. */ +function isShowing(el: FirstRunWizard): boolean { + return shadow(el, 'wa-dialog') !== null; +} + +beforeEach(() => { + stub('library.Library.AddLibrary', aLibrary); +}); + +describe('first-run-wizard', () => { + it('shows on a fresh install and stays up until a library exists', async () => { + const el = await wizardOnAFreshInstall(); + + expect(isShowing(el)).toBe(true); + }); + + it('stays hidden when a library is already configured', async () => { + stub('library.Library.GetAllLibrariesWithTrackCounts', [aLibrary]); + + const el = await fixture<FirstRunWizard>('first-run-wizard'); + + expect(isShowing(el)).toBe(false); + }); + + it('dismisses when a library appears by another route', async () => { + const el = await wizardOnAFreshInstall(); + + expect(isShowing(el)).toBe(true); + + emit(Events.LibraryAdded, aLibrary); + await el.updateComplete; + + expect(isShowing(el)).toBe(false); + }); + + it('does not raise itself when a library arrives while it is asking', async () => { + // The read is still in flight when the event lands, so its + // answer — an empty list — is stale by the time it returns. + let answer: (libraries: unknown[]) => void = () => {}; + + stub( + 'library.Library.GetAllLibrariesWithTrackCounts', + () => + new Promise((resolve) => { + answer = resolve; + }), + ); + + const el = await fixture<FirstRunWizard>('first-run-wizard'); + + emit(Events.LibraryAdded, aLibrary); + answer([]); + + await el.updateComplete; + await new Promise((r) => setTimeout(r, 0)); + await el.updateComplete; + + expect(isShowing(el)).toBe(false); + }); + + it('still dismisses through its own Get Started button', async () => { + stub('frontendutil.FrontendUtil.HasNativeDirectoryPicker', true); + stub('frontendutil.FrontendUtil.DirectoryPicker', '/home/logan/Music'); + + const { resetDirectoryPickerCache } = await import( + '@utils/pick-directory' + ); + + resetDirectoryPickerCache(); + + const el = await wizardOnAFreshInstall(); + const [choose, finish] = shadowAll<HTMLButtonElement>(el, '.btn'); + + choose?.click(); + await new Promise((r) => setTimeout(r, 0)); + await el.updateComplete; + + finish?.click(); + await new Promise((r) => setTimeout(r, 0)); + await el.updateComplete; + + expect( + wails.calls.filter((c) => c.path === 'library.Library.AddLibrary'), + ).toHaveLength(1); + expect(isShowing(el)).toBe(false); + }); +}); diff --git a/frontend/test/components/settings-column-list.test.ts b/frontend/test/components/settings-column-list.test.ts new file mode 100644 index 0000000..ab1531c --- /dev/null +++ b/frontend/test/components/settings-column-list.test.ts @@ -0,0 +1,190 @@ +/** + * Settings offers the columns the backend will accept, and no others. + * + * The list is built from `COLUMN_DEFS`, which is the *drawing* table: + * every definition the track list knows how to render, including + * `titleArtist` — the phone's stacked column, chosen by width in + * `PHONE_COLUMN_IDS` and never by a person. `tracklist.AllColumnIDs` in + * Go does not list that id, so the configurator offered a nineteenth + * row that could not be ticked: + * + * ``` + * validate = unknown track-list column ID: "titleArtist" + * titleArtist valid = false + * ``` + * + * What a user saw was **two rows both called "Track Name"** (#197), one + * of which did nothing — and a screen reader heard "Show the Track Name + * column" twice with nothing to tell them apart, which is `a11y.32`'s + * complaint inside the list that was fixed for exactly that. + * + * It is worse than an inert control, which is why the duplicate name + * was not the thing to fix. `SetTrackListColumns` assigns before it + * validates, so a rejected list stays in memory and `Save()` validates + * the whole config: + * + * ``` + * later, unrelated SetThemeAccentColor = could not save config: invalid + * config: ... unknown track-list column ID: "titleArtist" + * ``` + * + * — one tick and no setting saves for the rest of the session. That + * half is filed separately; this file keeps the row from being offered. + * + * The last test is the one that would have caught it when the column + * was added: the two lists are in different languages, so nothing but a + * sweep can hold them together. + */ +import { beforeEach, describe, expect, it } from 'vitest'; + +import '@components/config-page/config-page'; + +import { + COLUMN_DEFS, + CONFIGURABLE_COLUMN_IDS, +} from '@components/track-list/columns'; +import { flush, stub } from '@test/support/harness'; +import { fixture, shadowAll } from '@test/support/render'; + +/** Go's own list of column ids, as text. */ +const GO_CONFIG = Object.values( + import.meta.glob<string>('../../../backend/tracklist/config.go', { + eager: true, + query: '?raw', + import: 'default', + }), +)[0]; + +/** + * The ids `tracklist.AllColumnIDs` actually contains. + * + * Read out of the source rather than written down here, because a + * third copy of this list is a third thing to forget — which is the + * defect, one copy earlier. + */ +function goColumnIDs(source: string): string[] { + const constants = new Map<string, string>(); + const constBlock = /const \(([\s\S]*?)\n\)/.exec(source)?.[1] ?? ''; + + for (const [, name, id] of constBlock.matchAll( + /(\w+)\s+ColumnID\s*=\s*"([^"]+)"/g, + )) { + constants.set(name!, id!); + } + + const listBlock = + /var AllColumnIDs = \[\]ColumnID\{([\s\S]*?)\n\}/.exec(source)?.[1] ?? ''; + + return [...listBlock.matchAll(/(\w+),/g)] + .map(([, name]) => constants.get(name!)) + .filter((id): id is string => id !== undefined); +} + +/** + * The column rows, and only those. + * + * Settings’ view-visibility list (#25) is drawn with the same two + * classes, so a bare `.column-label` sweeps 29 rows across two + * sections — and "Albums" the destination sitting beside "Album" the + * column is not the fault this file is about. The `for`/`id` prefix is + * what tells them apart. + */ +const COLUMN_ROW_LABEL = 'label.column-label[for^="column-"]'; +const COLUMN_ROW_BOX = 'input.column-toggle[id^="column-"]'; + +/** The rows the configurator draws, by their visible name. */ +async function columnRowNames(): Promise<string[]> { + const page = await fixture('config-page'); + + await flush(); + await page.updateComplete; + + // Every section renders collapsed, and a collapsed body is `hidden`. + for (const section of shadowAll<HTMLElement>(page, 'config-section')) { + section.shadowRoot + ?.querySelector<HTMLButtonElement>('button[aria-expanded="false"]') + ?.click(); + } + + await flush(); + await page.updateComplete; + + return shadowAll<HTMLElement>(page, COLUMN_ROW_LABEL).map( + (label) => label.textContent?.trim() ?? '', + ); +} + +describe('the Settings column list', () => { + beforeEach(() => { + for (const path of [ + 'library.Library.GetAllLibrariesWithTrackCounts', + 'jobs.Service.GetJobs', + 'download.Service.ListProviders', + 'download.Service.ProviderKinds', + ]) { + stub(path, []); + } + + stub('config.Config.GetShortcuts', {}); + stub('config.Config.GetDownloadPreferences', {}); + stub('config.Config.GetThemeAccentColor', '#ffd43b'); + stub('config.Config.GetThemeBackgroundShade', 'dark'); + }); + + it('names each row once', async () => { + const names = await columnRowNames(); + + // A sweep over nothing passes. + expect(names.length, 'the page draws column rows').toBeGreaterThan(5); + + const seen = new Set<string>(); + const duplicated = names.filter((name) => !seen.add(name)); + + expect(duplicated).toEqual([]); + expect(names.filter((n) => n === 'Track Name')).toHaveLength(1); + }); + + it('gives each checkbox a name that identifies it', async () => { + // The visible half above is what was reported; this is the half a + // screen reader gets, and it is the one `config-page` computes + // from the same string. + const page = await fixture('config-page'); + + await flush(); + await page.updateComplete; + + const labels = shadowAll<HTMLInputElement>(page, COLUMN_ROW_BOX).map( + (box) => box.getAttribute('aria-label') ?? '', + ); + + expect(labels.length, 'the page draws column checkboxes').toBeGreaterThan(5); + expect(new Set(labels).size).toBe(labels.length); + }); +}); + +describe('the column table', () => { + it('offers no column the backend would reject', async () => { + const accepted = goColumnIDs(GO_CONFIG ?? ''); + + // Two non-vacuity guards: a glob that stopped matching, and a + // parse that stopped finding the list it names. + expect(GO_CONFIG, 'backend/tracklist/config.go is readable').toBeTruthy(); + expect(accepted.length, 'AllColumnIDs was parsed').toBeGreaterThan(10); + + expect( + CONFIGURABLE_COLUMN_IDS.filter((id) => !accepted.includes(id)), + ).toEqual([]); + }); + + it('still knows how to draw every column it offers', async () => { + // The filter must not have taken a column *out* of the drawing + // table: `configurable` says what Settings may list, not what the + // list may render. + expect( + CONFIGURABLE_COLUMN_IDS.filter((id) => COLUMN_DEFS[id] === undefined), + ).toEqual([]); + expect(CONFIGURABLE_COLUMN_IDS).not.toContain('titleArtist'); + expect(COLUMN_DEFS['titleArtist'], 'the phone still has its column') + .toBeTruthy(); + }); +});