63786101 - Add issue-filing scaffolding for the log-error skill - #48
63786101 - Add issue-filing scaffolding for the log-error skill#48Danswar wants to merge 43 commits into
Conversation
432662f to
14a61e5
Compare
|
EN: DE: DetailsReview passes25 passes, each two parallel lanes (conformance and logic) over the full diff at a fresh head, plus five cross-vendor gate runs. Passes 3, 7, 16 and one lane of 20 ended Findings that changed the code, by pass:
Reasoned rejections, each against a verified repo fact: case-insensitive masking (three existing tests require lowercase The CLI wiring is a declared deviation from end-to-end completeness, granted by a human reviewer and recorded in Gates at this head
|
TaprootFreakAI
left a comment
There was a problem hiding this comment.
EN:
Do not file GitHub issues for grouped errors; keep the grouping in the local store.
DE:
Keine GitHub-Issues für gruppierte Fehler anlegen; die Gruppierung gehört in den lokalen Store.
Details
The useful part of this change is template_fingerprint: variants that differ only by chain or asset belong to one template. That identity already has a home — error.seen in the local store, which already tracks fingerprint, count, and last_seen.
GitHub issues are the wrong second store. error_issue_act exists to treat an issue body as a database (delimited variant table, hidden markers, salted public digest, burst issue, cooldown against local history). That complexity is the symptom: issue text is untrusted, public, and not a row. Cooldown and variant counting already live locally; the issue only mirrors them, with a large surface (log excerpts in public bodies, marker splicing, a truncated gh list read as "no issue exists").
The human-facing next step on an eligible error is already specified: error.fix → draft pull request (DESIGN §21.5). An extra issue in between is not a work item the store and the draft PR do not already provide.
Please keep template_fingerprint on error.seen if grouping is still wanted, and drop the GitHub issue-filing path (error_issue_act, error.issue, DESIGN §21.6 filing behaviour). Do not wire agent watch error-issue.
Remove error_issue_act.py and its tests along with the related design notes. Filing a GitHub issue per grouped error added an unnecessary external dependency with no real benefit over the existing local history and draft-pull-request path; the template_fingerprint grouping itself stays.
17c36b3 to
cc91a88
Compare
Section 21.3 still described the deleted issue-marker mechanism and pointed at section 21.6 for detail that no longer exists there.
Three docstrings still described the deleted GitHub-issue mechanism; the functions and tests they document are unrelated leftovers that stay exactly as they are.
The module comment above _KNOWN_CHAINS still described the deleted issue-filing feature.
|
EN: DE: DetailsAddresses the CHANGES_REQUESTED review directly: Review rounds (Grok then Codex, quality + logic each):
Local suite: 693 passed, 1 skipped throughout (was 771 before this branch removed |
EN:
Four additive pieces toward letting the
log-errorskill actually take its "open an Issue with repro" branch, all deterministic — no model judgment in any of the grouping or throttling logic. Atemplate_fingerprintgroups error variants that differ only by chain and/or asset ticker under one template, masking those names only as whole tokens so ordinary prose like "Based" or "RESOLVE" is never mistaken for one. A newerror_issue_actmodule finds-or-creates a GitHub issue per template, handles every pending row of one template in a single pass, tracks the variants in a machine-owned delimited section of the issue body, and announces genuinely new ones in one comment. Two throttles sit in front: burst detection, counted over templates never filed before so a backlog draining after downtime is not mistaken for a burst, and a cooldown checked against local history that adry_runpreview never opens.DE:
Vier additive Bausteine, damit die
log-error-Skill ihren Zweig "Issue mit Repro öffnen" tatsächlich nutzen kann, durchgehend deterministisch — keine Modell-Beurteilung in der Gruppierungs- oder Drossel-Logik. Eintemplate_fingerprintgruppiert Fehler-Varianten, die sich nur durch Chain und/oder Asset-Ticker unterscheiden, unter einer Vorlage und maskiert diese Namen nur als ganze Tokens, damit gewöhnlicher Text wie "Based" oder "RESOLVE" nie dafür gehalten wird. Ein neues Modulerror_issue_actfindet oder legt ein GitHub-Issue pro Vorlage an, verarbeitet alle offenen Zeilen einer Vorlage in einem Durchgang, führt die Varianten in einem maschinenverwalteten, abgegrenzten Abschnitt des Issue-Bodys nach und kündigt wirklich neue in einem Kommentar an. Davor liegen zwei Drosseln: eine Burst-Erkennung, gezählt über noch nie gemeldete Vorlagen, damit ein nach einer Störung ablaufender Rückstau nicht als Burst gilt, und eine Abklingzeit gegen die lokale Historie, die eindry_runnie eröffnet.Details
Grouping
template_fingerprint()/template_signature()inerrors.pyare additive — the existingfingerprint()/stack_sig()identity used forerror.seencount/last_seen tracking is unchanged, so per-variant dedup stays exact. The chain and payment-rail names and asset tickers masked in the template signature come from the platform's own live enum (checked against production, not guessed from log samples); the asset list is cut to tickers seen on 2+ chains plus two single-chain tickers confirmed present in real production errors.Both lists are matched longest-first and only at word boundaries. Without those anchors
Basematched inside "Based",SOLinside "RESOLVE",COMPinside "COMPLETE" andDAIinside "DAILY", which both mangled the template hash of unrelated errors and labelled them with a token that had nothing to do with them. Ordering still matters for a name containing punctuation:USDCinsideUSDC.epasses a word boundary because.is not a word character, so the longer name is tried first and a gluedUSDC.e_balanceyields nothing at all — the same answer asUSDC_balance. Matching is deliberately case-sensitive: lowercaseusdin "token tether -> usd" is prose, not a ticker, and three tests pin that.service,classandenvironmentare percent-escaped before the join so two different field tuples cannot serialize to one fingerprint.Issue handling
error_issue_act.pymirrorserror_fix_act.py's deterministic pattern (find-or-create, runner-injectedghcalls for testability). Dedup uses a label plus a hidden marker carrying a salted digest of the template fingerprint, matched against the issue body rather than handed togh --search, whose full-text matching can both miss the marker and return an issue that does not carry it. The marker is re-checked on the body that is actually written, so a marker a hand edit moved inside the machine-owned section is never spliced away. The digest is salted with the device id because it sits in a public issue while the fingerprint behind it stays raw and injective — grouping must not merge two tenants, and an unsalted digest would let a reader confirm a guessed stream label. The same salting and the same encode guard apply to the burst row digest.The variant table lives between delimited markers and is spliced in place — nothing outside that section is ever touched. A body with a lone or duplicated marker is treated as damaged and fails loud rather than gaining a second section, and every body sent to
ghpasses one shared ceiling check before the call. The table is capped, so normal growth cannot walk an issue into that ceiling and wedge every later update; the dropped count stays visible.All pending rows of one template are processed together, so two variants seen in one run land in one issue write. Handled row by row, a second variant seen in the same run was silently dropped — exactly the
Arbitrum/USDCvsArbitrum/WBTCcase this change exists to group. The edit is written before the comment so the durable record lands first and a retry can never double-file a variant; a failed comment is reported on the row rather than discarding a successful edit.The burst issue reuses the same splice mechanism with its own marker. Only templates with no prior filing count toward the threshold — a backlog draining after downtime is a volume spike, not a burst — and a template already folded into an open burst is not filed again, judged from the issue body and from local fold history keyed by issue number so a fold into a since-closed burst is not credited to an unrelated open one. The cooldown reads local history rather than calling
gh, is scoped to the issue repo, ignores rows that were themselves skipped, ignoresdry_runpreviews, ignores rows that name no issue, and treats a future-dated timestamp as not in cooldown.Validation
Verified end-to-end against real production data (not just synthetic tests): replayed a batch of real
error.seenrows from a live instance through this code locally indry_runmode. Before the asset-ticker masking, 30 raw incidents grouped into 25 templates; after, into 11 — mostly collapsing repeated low-balance alerts that only varied by token. Separately measured real volume over a 7-day production window: ~353 raw error lines/day collapsing to a steady state of ~14–29 new templates/day (a single very-noisy known issue accounted for 26% of all raw lines but correctly collapsed to one template) — this is the basis for the burst-detection default and the reason a cooldown was worth building before this goes anywhere near live.Local suite green: 665 passed, 1 skipped. Every fix in this branch carries a mutation check confirming its test actually catches the regression rather than passing by construction.
Out of scope
Not included, intentionally: CLI wiring (adding
error.issueto theagent activity addallowlist, anagent watch error-issuecommand, and theissue_repo/dry_run/cooldown_minutes/storm_thresholdconfig fields) and the human-already-filed-a-duplicate fuzzy-match check. Both are separate, larger surfaces that deserve their own focused review. The deferral is recorded inDESIGN.md§21.6, and §21.3 documents theerror.seenpayload this writes.Also deliberately left for follow-up: the pre-existing
fingerprint()joins its fields without escaping, the same ambiguity fixed here fortemplate_fingerprint; changing it would re-key already-storederror.seenrows, so it does not belong in this change.