Repository navigation
chore(loop): mine the uploaded agent sessions and feed them to retro - #82
github-actions[bot] wants to merge 4 commits into
Conversation
The workflow uploads every pi session as a `loop-sessions-<run_id>` artifact (90 days), but nothing read them back: `retro-scheduled` aggregated only the run `end` rows, so engine faults, failing tool calls, retry loops and reads of skills that do not exist were written and never consumed (#77). `scripts/loop/sessions.mjs` now lists/downloads those artifacts through the job's existing `gh` credential, parses each pi session (stage, runId, task, per-call usage, tool calls and their results), joins it to the run `end` row by runId, and renders an evidence report: processed tokens vs the recorded `tokens` value, engine faults, a tool-failure taxonomy, retry loops, and the stage skills the opening ritual names but the repo lacks. `entry.mjs` runs the miner before a `retro-scheduled` agent starts and passes the report path in its prompt (fail-open, and an unavailable report is stated in the prompt so "no report" is never read as "no signals"). `skills/retro/SKILL.md` scheduled mode makes reading it step 0 and requires each signal to become a proposal or be dismissed with a reason. The parser/aggregate/renderer is dependency-free and unit-tested in the browser vitest suite; the CLI (`pnpm loop:sessions`, also `--dir` for an offline tree) does the gh/unzip/fs work. First run over the 100 newest artifacts (103 sessions, 2026-09-10..10-21): 148,246,119 processed tokens vs 7,120,313 recorded (20.8x), 148 failed tool calls (3.3%), 2 engine faults in runs still recorded as `completed`, 18 shallow-clone git failures, 9 reads of the two missing stage skills. The resulting proposals are in the run report; none are applied here.
|
Loop pr-review — request-changes Reviewer a — correctness & regression (model Reviewer A — Correctness & regression — verdict: request-changes (confidence: high) PR-contract gate: pass (What/why, Proof it works, Risk tier + AI role, Review focus all present;
No regression surface: no Reviewer b — standards, safety & maintainability (model Reviewer B — Standards, safety & maintainability: approve Contract gate: all four author sections present. Test-integrity gate: the only test change is additive ( No L1/L2 issues from this lens. AGENTS.md conformance, docs, secrets, dangerous patterns and silent-change review are all clean (
|
Review findings — resolutionBoth verdicts came from the loop's own reviewers. Every blocking item from Reviewer A and
Reproduce the verificationpnpm exec vitest run test/sessions.spec.ts # 7 passed
node scripts/loop/sessions.mjs --dir <tree> --limit 1 --out /tmp/s.md
node scripts/loop/sessions.mjs --limit # exit 1: "--limit needs a value"The review re-run is enabled by #83 (D26: a new commit on a loop PR re-opens its review); |
Coverage
|
|
Loop pr-review — request-changes Reviewer a — correctness & regression (model Reviewer A — Correctness & regression — verdict: request-changes (confidence: high) PR-contract gate: pass (Closes #77, loop-task: #77, all four sections). Test-integrity gate: pass —
Regression surface: no Reviewer b — standards, safety & maintainability (model Reviewer B — standards, safety & maintainability: approve. Gates: PR-contract (all four sections + No L1/L2 problems. Optional nits:
Verified: |
Second-round review — resolution and corrected figuresCorrected numbers. The figures in the body and in the earlier resolution table were
The 38 rows the old build reported and this one does not are phantom: successful A1 (this round). Other nits fixed. Parsed-but-unkept files are now reported Verification. Reviewer B's remaining notes are accepted as documented limitations: redaction is |
|
Loop pr-review — approve ✅ — two isolated reviewers Reviewer a — correctness & regression (model Reviewer A — Correctness & regression: approveHead What I re-ran on head (chromium installed locally; integration specs excluded as the body states): The central claim holds, measured rather than assumed. For artifact No L1/L2 problems. Eight L3 nits, none of which I would block on:
Merging remains the owner's call; Reviewer b — standards, safety & maintainability (model Reviewer B (standards, safety & maintainability) — verdict: approve. Contract gate: pass (all four author sections present, non-empty). Test-integrity gate: pass (only a new test file added; no test deleted/skipped, no assertion or gate config weakened; signer oracles untouched). Re-ran the same non-integration suite as the body: 20 files, 263 passed. Problems (L3 only, optional):
Safety/standards checks that passed: redaction covers the loop's real credential shapes before any text reaches the report/JSON; no shell interpolation (spawnSync argv only, no |
This branch has not been deployed
1 participant
Closes #77
loop-task: #77
What/why
The loop already uploads every agent session as a
loop-sessions-<run_id>artifact, but nothing read them back —retro-scheduledaggregated only runendrows, so the facts that explain a run (engine faults, failing tool calls, retry loops, reads of skills that do not exist) were written and never consumed (#77; owner: "do 1 now, then 2"). This adds a session miner and makes the weekly retro consume it. The mine at this branch's head over the 100 newest artifacts: 105 sessions, 160,399,055 processed tokens, 4,723 tool calls, 110 failed tool calls (2.3%), 2 engine faults in runs still recorded ascompleted, 16 git failures in 12 sessions, 11 reads of the two missing stage skills. These supersede the pre-fix figures in the issue comment (148 failures / 3.3%, 29 git / 19 sessions, 9 skill reads): the classifier now counts only failed tool results, so the 38 rows that disappeared were phantom — successful reads of this spec, of reports and of logs quoting failure text.Proof it works
(
pnpm testalso picks up the five integration specs, which needpnpm serve+ a.envwith credentials; this sandbox has none, so they fail withTypeError: Failed to fetchbefore any assertion — unchanged by this PR.)The miner against the real artifact store:
Unit coverage of the parser (synthetic session, browser vitest): token split, failure classification, retry loop, missing-skill path, report rendering (
test/sessions.spec.ts, 4 tests). The recorded-token column needs the runendrows from the R2 state layer (a local checkout carries only the 2026-09 rows, so 7 sessions join there); when nothing joins, the report saysRecorded-token comparison unavailableinstead of printing a multiplier, and oneendrow is counted once per run however many sessions share its runId (pr-review= two reviewers, one row).Risk tier + AI role
Medium-low, loop-host only; the agent produced the whole change. The miner is read-only, writes only under
state/, and uses the credential the job already holds. The behavioural change is on the weekly retro timer: it now downloads up to 50 artifacts (LOOP_SESSION_ARTIFACTS, default 50) before the agent starts, and is fail-open — a failed download is logged and stated in the prompt, so "no report" can never be read as "no signals". If the classifier is wrong, the effect is a wrong finding in a retro report, not a wrong GitHub write.skills/retro/SKILL.mdis a norms-layer proposal; per AGENTS.md the owner merges it.Review focus
scripts/loop/shared/sessions.mjs— the token split (billedTokens= sum of every assistant call;contextTokens= last call; the end row stores the latter) and the failure taxonomy's line-anchored matching. A misclassification here propagates into retro's proposals.scripts/loop/entry.mjsretro hook — 28 lines insidedoRun: fail-open degradation and the prompt wording that prevents an absent report from being treated as a clean corpus.