fix(feedback): generic tracker context + targeted search (no repo catalog) #259

Merged

joshtronic merged 1 commit from feat/feedback-dedup-search into master

2026-06-26 03:49:04 +00:00

igor commented

2026-06-26 03:44:33 +00:00

Collaborator

Fixes the garbage tickets from the first live run (the "no such game" / name-detective noise on #63–67).

Root cause (my bug, not the model's): feedback_gather_context fed the model src/_data/games.json — a 20-game curated subset — as "the games." The real ~100 games are folders under src/games/, so the model saw none of the feedback's games and concluded they don't exist.

The fix is NOT to read src/games/ — that hardcodes porksicle's file layout into the generic harness, which is exactly wrong (your call). It's to drop the catalog entirely and go generic:

feedback_gather_context → recent closed issues + commits only. No catalog, no repo structure.
feedback_search_prior → a generic keyword search of issues/commits for the subject the feedback names. This reaches older fixes the recent-N window misses (e.g. Boar Dungeon → #41) — the real dedup fix.
directive → take the named subject as-given; never claim "no such game" or do name-detective work; judge the feedback's substance + the already-worked signals only.

Zero src/games/games.json references remain. Tests + check-sync green. No reviewer.

Follow-up (separate, after this deploys): wipe the 5 misleading tickets on porksicle and re-triage the backlog clean.

**Fixes the garbage tickets from the first live run** (the "no such game" / name-detective noise on #63–67). **Root cause (my bug, not the model's):** `feedback_gather_context` fed the model `src/_data/games.json` — a **20-game curated subset** — as "the games." The real ~100 games are folders under `src/games/`, so the model saw none of the feedback's games and concluded they don't exist. **The fix is NOT to read `src/games/`** — that hardcodes porksicle's file layout into the generic harness, which is exactly wrong (your call). It's to **drop the catalog entirely** and go generic: - `feedback_gather_context` → recent closed issues + commits only. No catalog, no repo structure. - `feedback_search_prior` → a **generic keyword search** of issues/commits for the subject the feedback names. This reaches older fixes the recent-N window misses (e.g. Boar Dungeon → #41) — the real dedup fix. - directive → take the named subject **as-given**; never claim "no such game" or do name-detective work; judge the feedback's substance + the already-worked signals only. Zero `src/games`/`games.json` references remain. Tests + check-sync green. No reviewer. Follow-up (separate, after this deploys): wipe the 5 misleading tickets on porksicle and re-triage the backlog clean.

igor added 1 commit

2026-06-26 03:44:33 +00:00

fix(feedback): generic tracker context + targeted search, no repo-specific catalog

Lint / check-sync (pull_request) Successful in 5s

Details

Lint / check-sync (push) Successful in 4s

Details

a04c9a2cde

The first live run filed garbage tickets ('no such game', name-detective work)
because feedback_gather_context fed the model src/_data/games.json -- a 20-game
CURATED subset -- as 'the games', while the real ~100 games are folders under
src/games/. So the model thought every feedback game didn't exist. The fix is
NOT to read src/games/ (that hardcodes porksicle's layout into the generic
harness -- wrong); it's to drop the catalog entirely:

- feedback_gather_context: recent CLOSED issues + commits only. No catalog, no
  file layout. The harness has no business knowing how a repo organizes itself.
- feedback_search_prior: a GENERIC keyword search of issues/commits for the
  subject the feedback names -- reaches older fixes the recent-N window misses
  (e.g. Boar Dungeon, fixed in #41). This is the real dedup fix.
- directive: take the named subject AS-GIVEN; never speculate about whether it
  'exists' or do name-detective work; judge only the feedback's substance + the
  already-worked signals. An unfamiliar name is just a name.

Tests: feedback_search_prior (name match / empty); zero src/games or games.json
refs remain. bash -n + check-sync green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011KXPafSYJPY6XkWHHk1Lfs

igor commented

2026-06-26 03:46:18 +00:00

Author

Collaborator

🤖 Review — `COMMENT` (automated)

CI for a04c9a2c: success

Well-scoped fix that drops the porksicle-specific games.json catalog from feedback triage and replaces it with a generic feedback_search_prior keyword search over closed issues/commits. Code is clean, tested, and CI is green. One reservation I'd want a human to weigh before merge, plus a claim I can't fully verify from the diff.

Findings

Untrusted subject echoed into the non-fenced "context" section (lib/feedback.sh, feedback_search_prior + do_feedback_tick). The subject is jq -r '.Game // ""' straight off the player-submitted CSV row, then printed verbatim into a context header: printf '### Prior work mentioning "%s" ...' "$subject". That context is assembled into the prompt alongside feedback_gather_context output and presented as harness-generated (i.e. trusted) data, separate from the explicitly-fenced UNTRUSTED feedback block. A crafted Game value (embedded newline + fake ##/SYSTEM: markdown) could inject into what the model reads as trusted context. This is a marginal escalation of a pre-existing vector (the feedback row already reaches the model) and is backstopped by FORGEJO_REVIEWER greenlighting every filed issue, so I'm not blocking — but the human should decide whether the named subject deserves the same untrusted-fencing as the rest of the feedback. Other interpolations are safe: the URL query is @uri-encoded and the grep term goes through jq --arg.
"Zero src/games/games.json references remain" — unverifiable from the diff. The diff removes the only reference I can see (feedback_gather_context) and updates CLAUDE.md + the directive, but a repo-wide "zero remain" claim can't be confirmed from the touched files alone. Likely true given the focused scope; flagging that I can't confirm it.

Correctness / tests

feedback_search_prior logic looks right: empty/(unknown) subject short-circuits with return 0; issues path defensively re-filters PRs with select(.pull_request == null); commit grep lowercases both sides via ascii_downcase/tr. The two new tests (commit-grep hit on "Boar Dungeon", empty-subject → no output) match the mock _fj and pass through cleanly.
Minor, non-blocking: commits?limit=120 may be silently capped by the Forgejo API page size (commonly 50), so the "reaches past the recent-N window" guarantee could be smaller than advertised. Not a bug, just narrower than the comment implies. The issues-search path (q=) isn't exercised by a test — the mock returns [] — so that branch is untested.

No CI-config changes, diff is ~60 lines and on-topic, CI is success. The injection-surface question is the only thing I'd want eyes on before this merges.

_{Independent review by the harness on claude-opus-4-8. The human reviewer is requested once Igor has reviewed; a human still merges.}

### 🤖 Review — `COMMENT` _(automated)_ CI for `a04c9a2c`: **success** Well-scoped fix that drops the porksicle-specific `games.json` catalog from feedback triage and replaces it with a generic `feedback_search_prior` keyword search over closed issues/commits. Code is clean, tested, and CI is green. One reservation I'd want a human to weigh before merge, plus a claim I can't fully verify from the diff. **Findings** - **Untrusted subject echoed into the non-fenced "context" section (`lib/feedback.sh`, `feedback_search_prior` + `do_feedback_tick`).** The subject is `jq -r '.Game // ""'` straight off the player-submitted CSV row, then printed verbatim into a context header: `printf '### Prior work mentioning "%s" ...' "$subject"`. That context is assembled into the prompt alongside `feedback_gather_context` output and presented as harness-generated (i.e. *trusted*) data, separate from the explicitly-fenced UNTRUSTED feedback block. A crafted `Game` value (embedded newline + fake `##`/`SYSTEM:` markdown) could inject into what the model reads as trusted context. This is a marginal escalation of a pre-existing vector (the feedback row already reaches the model) and is backstopped by `FORGEJO_REVIEWER` greenlighting every filed issue, so I'm not blocking — but the human should decide whether the named subject deserves the same untrusted-fencing as the rest of the feedback. Other interpolations are safe: the URL query is `@uri`-encoded and the grep term goes through `jq --arg`. - **"Zero `src/games`/`games.json` references remain" — unverifiable from the diff.** The diff removes the only reference I can see (`feedback_gather_context`) and updates CLAUDE.md + the directive, but a repo-wide "zero remain" claim can't be confirmed from the touched files alone. Likely true given the focused scope; flagging that I can't confirm it. **Correctness / tests** - `feedback_search_prior` logic looks right: empty/`(unknown)` subject short-circuits with `return 0`; issues path defensively re-filters PRs with `select(.pull_request == null)`; commit grep lowercases both sides via `ascii_downcase`/`tr`. The two new tests (commit-grep hit on "Boar Dungeon", empty-subject → no output) match the mock `_fj` and pass through cleanly. - Minor, non-blocking: `commits?limit=120` may be silently capped by the Forgejo API page size (commonly 50), so the "reaches past the recent-N window" guarantee could be smaller than advertised. Not a bug, just narrower than the comment implies. The issues-search path (`q=`) isn't exercised by a test — the mock returns `[]` — so that branch is untested. No CI-config changes, diff is ~60 lines and on-topic, CI is `success`. The injection-surface question is the only thing I'd want eyes on before this merges. --- <sub>Independent review by the harness on `claude-opus-4-8`. The human reviewer is requested once Igor has reviewed; a human still merges.</sub>

igor added spent time

2026-06-26 03:46:18 +00:00

1 minute 25 seconds

igor requested review from joshtronic

2026-06-26 03:46:19 +00:00