joshtronic/igor

Watch

feat: player-feedback triage (agent.json .feedback.csv -> drop/file) #258

Merged

joshtronic merged 2 commits from feat/feedback-triage into master

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

igor commented

2026-06-26 02:58:52 +00:00

Collaborator

Turns porksicle's Google-Form feedback CSV into reviewed work tickets you greenlight — the feedback loop, built to the spec we locked.

Opt-in: agent.json .feedback.csv (the 2nd agent.json consumer; porksicle's already set).

Per tick, one row (do_feedback_tick): takes the oldest unprocessed CSV row, and ONE claude_call on AGENT_MODEL_REVIEW reads it — as clearly-fenced untrusted data, never instructions — plus context (recent closed issues, recent commits, the game list), then decides:

DROP — spam / too vague / already-worked (judged from the closed-issues/commits context). The model may silently drop confident junk/dupes (your call).
FILE — real + new → an UNLABELED issue assigned to you. Greenlight = add Agent label + unassign; reject = close.

Safety: the human label gate means a prompt-injected row can at worst become a ticket you reject — never code. Processed rows are stamped in a local seen-set (.feedback.seen); nothing is written back to the sheet (the tracker is the status, no status column). Below the Claude health gate (model work). Robust quoted-CSV parsing via python3.

Validated live on your 8 real rows (dry-run, no filing):

Slam Pig "bigger instructions" → DROP: "already worked — #53 / PR #54" ✅ the dedup you asked for
Boar Dungeon won't-load, Ham Jam controls, Pork Chops idea → FILE (real)

28 unit checks; bash -n + check-sync green. No reviewer.

To arm: merge this, and the next ticks will start triaging the 8-row backlog (one per tick) into tickets for you to greenlight/reject. The already-worked ones should drop themselves.

Turns porksicle's Google-Form feedback CSV into **reviewed work tickets you greenlight** — the feedback loop, built to the spec we locked. **Opt-in:** `agent.json` `.feedback.csv` (the 2nd `agent.json` consumer; porksicle's already set). **Per tick, one row** (`do_feedback_tick`): takes the oldest unprocessed CSV row, and ONE `claude_call` on `AGENT_MODEL_REVIEW` reads it — as **clearly-fenced untrusted data, never instructions** — plus context (recent **closed issues**, recent **commits**, the **game list**), then decides: - **DROP** — spam / too vague / **already-worked** (judged from the closed-issues/commits context). The model may silently drop confident junk/dupes (your call). - **FILE** — real + new → an **UNLABELED issue assigned to you**. Greenlight = add `Agent` label + unassign; reject = close. **Safety:** the human label gate means a prompt-injected row can at worst become a ticket you reject — never code. Processed rows are stamped in a local seen-set (`.feedback.seen`); nothing is written back to the sheet (the tracker is the status, no status column). Below the Claude health gate (model work). Robust quoted-CSV parsing via `python3`. **Validated live on your 8 real rows** (dry-run, no filing): - Slam Pig "bigger instructions" → **DROP**: *"already worked — #53 / PR #54"* ✅ the dedup you asked for - Boar Dungeon won't-load, Ham Jam controls, Pork Chops idea → **FILE** (real) 28 unit checks; `bash -n` + `check-sync` green. No reviewer. **To arm:** merge this, and the next ticks will start triaging the 8-row backlog (one per tick) into tickets for you to greenlight/reject. The already-worked ones should drop themselves.

igor added 1 commit

2026-06-26 02:58:52 +00:00

feat(feedback): player-feedback triage from agent.json .feedback.csv

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

Details

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

Details

30613832f0

Turns the Google-Form feedback CSV into reviewed work tickets. Opt-in by
convention: a repo's agent.json .feedback.csv (the second agent.json consumer
after auto-merge).

- PER-TICK, one row: do_feedback_tick takes the oldest unprocessed CSV row across
  the analysis set; ONE claude_call on AGENT_MODEL_REVIEW reads it (as fenced
  UNTRUSTED data) + repo context (recent closed issues, recent commits, the game
  list) and decides DROP (spam/vague/already-worked) or FILE (real+new).
- FILE -> UNLABELED issue assigned to FORGEJO_REVIEWER; greenlight = add Agent
  label + unassign, reject = close. Human label gate bounds prompt-injection.
- DROP latitude is the operator's call (chosen): the model may silently drop
  confident spam/dupes; the seen-set still records the row.
- State: local .feedback.seen (FIFO-capped); nothing written back to the sheet.
- Robust CSV parsing via python3 (free text has commas + newlines).
- Below the health gate (model work). Verdict is a DECISION:/REASON:/TITLE:/
  ===BODY=== sentinel, parsed harness-side.

Validated live on porksicle's 8 real rows: correctly DROPPED an already-worked
item ('done in #53/#54' from the context) and FILED real bugs + a game idea.
28 unit checks (CSV parse, seen-set, response parse, file payload, tick
decision); 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:02:56 +00:00

Author

Collaborator

🤖 Review — `COMMENT` (automated)

CI for 30613832: success

Summary: Adds a player-feedback triage pass (lib/feedback.sh, bin/lib/feedback-directive.md, wired into bin/tick.sh below the health gate) that reads a Google-Form CSV from agent.json .feedback.csv, runs one claude_call per tick to DROP/FILE the oldest unprocessed row, and files unlabeled issues for the human to greenlight. Well-scoped (~384 added lines), CI green, no workflow changes, and the security model (untrusted data fenced + human label gate) is sound. I'm landing on COMMENT because of a few things I cannot verify from the diff alone plus one robustness concern.

Things I can't verify from the diff (worth a human glance):

claude_call signature. do_feedback_tick calls claude_call "$AGENT_MODEL_REVIEW" "feedback-triage" 4000 "$directive" "$prompt" 0 (lib/feedback.sh, in the retry loop). The test stubs claude_call to ignore all args, so the test passes regardless of whether this arg order/arity matches the real claude_call. If it doesn't match, the feature breaks at runtime while tests stay green. Please confirm against the real definition.
ANALYSIS_REPOS_JSON shape. do_feedback_tick iterates with while IFS= read -r repo_line ... done <<<"$ANALYSIS_REPOS_JSON", treating it as newline-delimited JSON objects (one .full_name per line). The test feeds a single bare object. If the rest of the harness stores this as a JSON array, the loop silently matches nothing and the feature is a no-op. The "validated live on 8 rows (dry-run)" claim suggests it works, but I can't confirm the dry-run exercised this exact path vs. calling functions directly.

Robustness concern (not a hard block, but consider):

Queue-head livelock. On parse failure (both attempts) or issue-file failure, the function return 1 without marking the row seen — correct for not losing data, but it means a single row that the model persistently can't produce parseable output for will sit at the head forever and block every later row from ever being triaged. Clearing .feedback.seen won't help since the bad row is still oldest-unseen. Worth a guard or an escape hatch (e.g. a per-row attempt counter that eventually drops-with-log).

Minor / description accuracy:

The description says "28 unit checks"; I count 27 ok/no/eq/has assertions in bin/test-feedback.sh (3 CSV + 4 seen-set + 1 next + 7 parse + 2 csv_url + 4 file_issue + 6 tick). The tests genuinely exist, so this is a miscount, not a fabricated claim — just fix the number.
feedback_parse_response runs the decision through tr -dc 'A-Z', so a model line like DECISION: DROP (spam) collapses to DROPSPAM and fails the case. Acceptable given the strict directive + 2 retries, but slightly brittle.
No issue is linked ("built to the spec we locked"); the change is coherent and self-contained, just noting the contract expects a linked issue.

Positives: good test isolation (all network/forgejo boundaries stubbed, skip-safe on missing tools), proper jq -n --arg payload construction (no injection), heredoc prompt interpolation is safe (variable values aren't re-evaluated), and the unlabeled-issue + human-greenlight design correctly bounds prompt injection to "a ticket Josh rejects, never code."

No blocking defect I can point to with certainty, but enough unverifiable runtime assumptions that I'd want the human to confirm the claude_call signature and ANALYSIS_REPOS_JSON shape before merge rather than rubber-stamp it.

_{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 `30613832`: **success** **Summary:** Adds a player-feedback triage pass (`lib/feedback.sh`, `bin/lib/feedback-directive.md`, wired into `bin/tick.sh` below the health gate) that reads a Google-Form CSV from `agent.json` `.feedback.csv`, runs one `claude_call` per tick to DROP/FILE the oldest unprocessed row, and files unlabeled issues for the human to greenlight. Well-scoped (~384 added lines), CI green, no workflow changes, and the security model (untrusted data fenced + human label gate) is sound. I'm landing on COMMENT because of a few things I cannot verify from the diff alone plus one robustness concern. **Things I can't verify from the diff (worth a human glance):** - **`claude_call` signature.** `do_feedback_tick` calls `claude_call "$AGENT_MODEL_REVIEW" "feedback-triage" 4000 "$directive" "$prompt" 0` (lib/feedback.sh, in the retry loop). The test stubs `claude_call` to ignore all args, so the test passes regardless of whether this arg order/arity matches the real `claude_call`. If it doesn't match, the feature breaks at runtime while tests stay green. Please confirm against the real definition. - **`ANALYSIS_REPOS_JSON` shape.** `do_feedback_tick` iterates with `while IFS= read -r repo_line ... done <<<"$ANALYSIS_REPOS_JSON"`, treating it as newline-delimited JSON objects (one `.full_name` per line). The test feeds a single bare object. If the rest of the harness stores this as a JSON *array*, the loop silently matches nothing and the feature is a no-op. The "validated live on 8 rows (dry-run)" claim suggests it works, but I can't confirm the dry-run exercised this exact path vs. calling functions directly. **Robustness concern (not a hard block, but consider):** - **Queue-head livelock.** On parse failure (both attempts) or issue-file failure, the function `return 1` *without* marking the row seen — correct for not losing data, but it means a single row that the model persistently can't produce parseable output for will sit at the head forever and block every later row from ever being triaged. Clearing `.feedback.seen` won't help since the bad row is still oldest-unseen. Worth a guard or an escape hatch (e.g. a per-row attempt counter that eventually drops-with-log). **Minor / description accuracy:** - The description says **"28 unit checks"**; I count **27** `ok`/`no`/`eq`/`has` assertions in `bin/test-feedback.sh` (3 CSV + 4 seen-set + 1 next + 7 parse + 2 csv_url + 4 file_issue + 6 tick). The tests genuinely exist, so this is a miscount, not a fabricated claim — just fix the number. - `feedback_parse_response` runs the decision through `tr -dc 'A-Z'`, so a model line like `DECISION: DROP (spam)` collapses to `DROPSPAM` and fails the `case`. Acceptable given the strict directive + 2 retries, but slightly brittle. - No issue is linked ("built to the spec we locked"); the change is coherent and self-contained, just noting the contract expects a linked issue. **Positives:** good test isolation (all network/forgejo boundaries stubbed, skip-safe on missing tools), proper `jq -n --arg` payload construction (no injection), heredoc prompt interpolation is safe (variable values aren't re-evaluated), and the unlabeled-issue + human-greenlight design correctly bounds prompt injection to "a ticket Josh rejects, never code." No blocking defect I can point to with certainty, but enough unverifiable runtime assumptions that I'd want the human to confirm the `claude_call` signature and `ANALYSIS_REPOS_JSON` shape before merge rather than rubber-stamp it. --- <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:02:56 +00:00

2 minutes 25 seconds

igor requested review from joshtronic

2026-06-26 03:02:57 +00:00

igor referenced this pull request from a commit

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

fix(feedback): anti-livelock escape hatch + robust DECISION parse (review #258)

igor added 1 commit

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

fix(feedback): anti-livelock escape hatch + robust DECISION parse (review #258 )

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

Details

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

Details

63fc77399f

Addresses the shadow review's substantive point on #258. (The two 'can't verify
from the diff' concerns were verified CORRECT, not bugs: claude_call's arg order
matches the real signature exactly, and the ANALYSIS_REPOS_JSON iteration is
byte-identical to the in-prod do_ceo_tick loop over the same jq -c '.[]' stream.)

- Queue-head livelock: a row that persistently failed to parse/file used to
  return 1 without marking seen, sitting at the head forever and blocking every
  later row (and clearing .feedback.seen didn't help -- still oldest-unseen). Now
  a per-row attempt counter (.feedback.attempts) gives up after
  FEEDBACK_MAX_ATTEMPTS (3) ticks: mark-seen-and-warn so the queue advances. The
  counter is cleared when a row resolves normally.
- DECISION parse hardened: extract the first alpha word, so 'DECISION: DROP --
  already worked' no longer mangles to DROPALREADYWORKED and fails the case.
- Tests: bump/give-up escape hatch + trailing-text decision. 36 checks total.

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:19:02 +00:00

Author

Collaborator

🤖 Review — `COMMENT` (automated)

CI for 63fc7739: success

Adds a player-feedback triage pass: lib/feedback.sh + directive + unit tests, wired into bin/tick.sh below the CEO pass. Well-structured, security-conscious (untrusted-data fencing + human label gate), and unit-tested. My reservations are about integration boundaries I can't verify from the diff and one description inaccuracy — net COMMENT, not a clear block, but I wouldn't rubber-stamp it for autonomous issue-filing without a human sanity-check.

What's good

Prompt-injection posture is sound: feedback is fenced as untrusted DATA, the model output is parsed harness-side (feedback_parse_response), issues are filed UNLABELED + assigned to the human, and code never runs off a feedback row. The directive reinforces this.
Anti-livelock give-up (_feedback_fail + attempt counter capped at 3) is a nice touch so a poison row can't block the queue head forever; tested directly.
No CI-config changes; CI is success.

Things a human should verify (stubbed-out in tests, so green CI proves none of them)

The unit tests stub every external boundary — claude_call, _fj, forgejo_repo_get_file, feedback_* overrides. So the green suite validates internal logic only, not that the real call signatures match. In particular claude_call "$AGENT_MODEL_REVIEW" "feedback-triage" 4000 "$directive" "$prompt" 0 (lib/feedback.sh, do_feedback_tick) and the _fj GET/POST shapes can't be confirmed from the diff. Please confirm these match the existing helpers' contracts.
ANALYSIS_REPOS_JSON is consumed as newline-delimited JSON objects (while read repo_line; ... jq -r '.full_name' <<<"$repo_line"). The test feeds a single one-line object. If the real env var is a JSON array (single line or pretty-printed), this loop silently processes zero repos. Worth confirming it's NDJSON to match other passes.
The PR says it was "Validated live on your 8 real rows (dry-run, no filing)" — so feedback_file_issue/_fj POST (the actual write path) was not exercised live, only via the payload-shape unit test. The first real FILE will be the first live exercise of the POST.
Placement claim "sits BELOW the Claude health gate" can't be verified — the diff inserts the block after do_ceo_tick at ~line 3622, but the health gate isn't shown. Confirm the gate is above this point.

Description accuracy

"28 unit checks" is an undercount — I count 36 ok/no/eq/has assertions in bin/test-feedback.sh. Not a fabrication (work is over-delivered, not missing), but the description and reality disagree; worth fixing so the checklist stays trustworthy.

Minor

Doc wording drifts: the give-up log says "clear .feedback to retry" while CLAUDE.md says "Clear .feedback.seen to re-triage" (the state actually lives under .feedback.seen/.feedback.attempts in discretionary-state.json). Harmless, but align the two.
feedback_fetch_rows uses curl -sL (follows redirects) on the .feedback.csv URL. Operator-controlled config, so not a real SSRF vector, but noting it.

Nothing here is a concrete correctness/security defect I can pin to a line, so I'm not requesting changes — but the integration assumptions and the unvalidated filing path are exactly the kind of thing the human should glance at before this starts opening real tickets.

_{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 `63fc7739`: **success** Adds a player-feedback triage pass: `lib/feedback.sh` + directive + unit tests, wired into `bin/tick.sh` below the CEO pass. Well-structured, security-conscious (untrusted-data fencing + human label gate), and unit-tested. My reservations are about integration boundaries I can't verify from the diff and one description inaccuracy — net **COMMENT**, not a clear block, but I wouldn't rubber-stamp it for autonomous issue-filing without a human sanity-check. **What's good** - Prompt-injection posture is sound: feedback is fenced as untrusted DATA, the model output is parsed harness-side (`feedback_parse_response`), issues are filed UNLABELED + assigned to the human, and code never runs off a feedback row. The directive reinforces this. - Anti-livelock give-up (`_feedback_fail` + attempt counter capped at 3) is a nice touch so a poison row can't block the queue head forever; tested directly. - No CI-config changes; CI is `success`. **Things a human should verify (stubbed-out in tests, so green CI proves none of them)** - The unit tests stub *every* external boundary — `claude_call`, `_fj`, `forgejo_repo_get_file`, `feedback_*` overrides. So the green suite validates internal logic only, not that the real call signatures match. In particular `claude_call "$AGENT_MODEL_REVIEW" "feedback-triage" 4000 "$directive" "$prompt" 0` (lib/feedback.sh, `do_feedback_tick`) and the `_fj GET/POST` shapes can't be confirmed from the diff. Please confirm these match the existing helpers' contracts. - `ANALYSIS_REPOS_JSON` is consumed as newline-delimited JSON objects (`while read repo_line; ... jq -r '.full_name' <<<"$repo_line"`). The test feeds a single one-line object. If the real env var is a JSON *array* (single line or pretty-printed), this loop silently processes zero repos. Worth confirming it's NDJSON to match other passes. - The PR says it was "Validated live on your 8 real rows (dry-run, **no filing**)" — so `feedback_file_issue`/`_fj POST` (the actual write path) was *not* exercised live, only via the payload-shape unit test. The first real FILE will be the first live exercise of the POST. - Placement claim "sits BELOW the Claude health gate" can't be verified — the diff inserts the block after `do_ceo_tick` at ~line 3622, but the health gate isn't shown. Confirm the gate is above this point. **Description accuracy** - "28 unit checks" is an undercount — I count **36** `ok`/`no`/`eq`/`has` assertions in `bin/test-feedback.sh`. Not a fabrication (work is over-delivered, not missing), but the description and reality disagree; worth fixing so the checklist stays trustworthy. **Minor** - Doc wording drifts: the give-up log says "clear .feedback to retry" while CLAUDE.md says "Clear `.feedback.seen` to re-triage" (the state actually lives under `.feedback.seen`/`.feedback.attempts` in `discretionary-state.json`). Harmless, but align the two. - `feedback_fetch_rows` uses `curl -sL` (follows redirects) on the `.feedback.csv` URL. Operator-controlled config, so not a real SSRF vector, but noting it. Nothing here is a concrete correctness/security defect I can pin to a line, so I'm not requesting changes — but the integration assumptions and the unvalidated filing path are exactly the kind of thing the human should glance at before this starts opening real tickets. --- <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:19:02 +00:00

2 minutes 9 seconds