chore(memory): promote strong-fit data-sync drafts for review#129
chore(memory): promote strong-fit data-sync drafts for review#129Hareet wants to merge 10 commits into
Conversation
Hareet
force-pushed
the
seeding-claude-cli-v2
branch
from
1979c05 to
6a2742a
Compare
sugat009
left a comment
sugat009
left a comment
There was a problem hiding this comment.
Requesting changes, though to be clear the fix is upstream and not something to patch by hand here. This seeder faithfully promotes what the distillation pipeline generated, so nothing is wrong with the promotion itself. The problem is in the drafts the pipeline produced, and I verified it against the live medic/cht-core API for every draft in this PR.
The defect: issueNumber / issueUrl name the merge PR, not the resolved issue
One precise note up front, because the intuitive version of this is not what is happening: issueNumber and issueUrl always agree with each other (the URL is literally /issues/<issueNumber>). The bug is that in 6 of 14 drafts here, that shared number is a pull request, so /issues/N silently redirects to /pull/N. The real resolved issue survives only in the PR-title slug. For example:
8773-fix6299-trigger-sync...storesissueNumber: 8773/issueUrl: .../issues/8773, but 8773 is the merge PR ("fix(#6299): trigger sync if something is left to sync"). The real issue is #6299 (closed): "Sync status sometimes says all reports synced when there are changes yet to sync".10776-perf10749-move-tasksbycontact...storesissueNumber: 10776/issueUrl: .../issues/10776, but 10776 is the merge PR ("perf(10749): move tasks_by_contact view to offline-only design document"). The real issue is #10749 (closed): "Move tasks_by_contact to offline clients only".
This is the same scraper behaviour flagged on #121, and it is pipeline-wide: across the four clean seeders, 60 of 107 drafts are affected.
Duplicates in this PR. Because each draft is keyed by its PR, some resolved issues appear more than once:
- Issue #10792 is promoted 3 times (
10793,10798,10799), one draft per merge PR for the single fix.
Why request-changes rather than merge-and-fix-later
This corpus is the agent's memory of resolved issues (consumed by the Context Analysis Agent, see #135). A reference that resolves to a PR instead of the issue is simply wrong data, and it specifically defeats #135's planned de-duplication by issue id, since the "id" ends up being the PR id. Regenerating the drafts after the upstream fix will rewrite most of these files anyway, so merging now would churn the corpus twice.
Suggested fix (at the source, not file by file)
- Fix the distiller/scraper to take the resolved issue from the
type(#N):PR title (the filename slug already extracts it correctly), and keep the PR number insource_prwhere it belongs. - Regenerate and re-promote this domain's drafts.
- Add de-duplication by real issue id (also a #135 acceptance item).
Open to discussing the approach. Once the drafts carry the real issue references, this should be a quick re-review.
|
Good catch! I noted that the pipeline is PR-centric but the schema is issue-based. Thanks for the dive and verification Root causeThe scraper only harvests linked issues from
When that body search returned nothing, the distiller silently aliased the PR number as the issue number: // src/scripts/distiller.ts:267
const issueNumber = pr.linkedIssues[0]?.number ?? pr.prNumber; // ← the bugSo Detection rule: a draft is affected when frontmatter What data we retainedtwo retained signals are sufficient to re-link deterministically:
The frontmatter Blast radius (all 261 promoted drafts)
136 of 261 are affected; 130 (96%) are fixable from retained data with zero network/LLM. This matches the reviewer's observed ratio (~60 of 107). Per-domain (affected count = token-fixable shown; tokenless 6 are spread across domains, e.g.
OptionsOption 1 — Offline re-link from filename tokenScript parses
Option 2 — Deterministic re-fetch (no LLM)Keyed by
Option 3 — Fix distiller/scraper + re-run full pipeline (reviewer's literal ask)Fix the source, then re-scrape and re-distill the affected PRs.
Recommendation — hybrid (no pipeline re-run)The body content is independent of this bug, so re-running distillation (Option 3) is unnecessary and risky.
I'm going to run 1 on this domain and verify a few of them. re-running the pipeline isn't bad, but uses my session limit in an hour and ends my weekly limit early |
…; flag no-issue PRs; share link
…ross-check, collision-aware)
…tadata-only, data-sync)
Hareet
force-pushed
the
memory/promote-data-sync
branch
from
2976cbb to
7a9b3b0
Compare
…ref filter, Sonar fixes Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…SonarCloud S6594) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
This is ready for another look, @sugat009 What changed Forward fix: the scraper now resolves linked issues from GitHub's Existing drafts: relinked deterministically rather than re-distilled — the body content was already faithful, so the one-off tool rewrites only the three identity fields ( This PR (data-sync): 5 drafts relinked to their real issues (e.g. The remaining domains follow in their own PRs using the same tool (after this one is approved); their edge cases (suspect mismatches, PRs that close no issue) are flagged for manual confirmation rather than guessed. Re-requesting review — thanks again for catching this. 🙏 |
sugat009
left a comment
sugat009
left a comment
There was a problem hiding this comment.
Re-reviewed at a0ae253b against the live cht-core API and the code. Strong fix: the distiller no longer aliases the PR number, the shared resolver and relink tool are clean, and 13 of 14 drafts are correctly relinked. One systemic gap I think is worth closing before merge (inline), the rest are notes and suggestions. All of it is open to discussion, please push back wherever I've read it wrong.
| for (const ref of closingRefs) push(ref.number, 'closing-ref'); | ||
|
|
||
| const titleIssue = parseTitleIssue(prTitle); | ||
| if (titleIssue !== null) push(titleIssue, 'title'); |
There was a problem hiding this comment.
issue (blocking): a title/body-derived number is trusted as an issue without confirming it isn't a PR
parseTitleIssue (:24) and this push(titleIssue, 'title') accept any positive integer; nothing checks that N is an issue rather than a pull request. So when a PR title is fix(#N): with N itself a PR (a follow-up PR) and closingIssuesReferences is empty, a PR number flows into issueNumber/issueUrl/id. Since this resolver is shared, the forward scraper reproduces it too (see the note on fetchLinkedIssues).
One way to close it: after resolving a title/body candidate, verify it's a real issue (gh api repos/<repo>/issues/N -> a pull_request field means PR, 404 means missing); if it's a PR, follow one hop to that PR's closingIssuesReferences, else flag. Keeping this file pure and putting the gh check in the callers that already shell out would preserve the scraper/relink symmetry. Open to other approaches, what's your read?
|
|
||
| return Array.from(seen) | ||
| .map((issueNumber): LinkedIssue | null => { | ||
| function fetchLinkedIssues(refs: IssueRef[], repo: string): LinkedIssue[] { |
There was a problem hiding this comment.
note: this is the path that lets the above reach the corpus
gh issue view <N> succeeds on a PR number (GitHub serves PRs through the issues endpoint), so a PR-number ref isn't dropped by the catch below. Just flagging where an is-it-an-issue check would naturally live.
| function classify(ctx: FileCtx, online: boolean, exec: ExecFn): Classification { | ||
| const src = ctx.src; | ||
| if (!src) return unchanged(ctx.file, ctx.issueNumber); // old-convention file, not the alias bug | ||
| if (ctx.issueNumber === src.prNumber) return classifyAffected(ctx, src, online, exec); |
There was a problem hiding this comment.
issue (blocking): the relink's "affected" test is narrower than the bug
classify treats a draft as affected only when issueNumber === src.prNumber. The 10399 draft has issueNumber 10182 != source_pr 10399, so it falls to classifySuspect, which passes it because issueNumber matches the filename token (:272) -> neither relinked nor flagged. Broadening this to "issueNumber doesn't resolve to a real issue" (the same gh check as above) would catch both the original alias and this second-order case, and a re-run picks up the stragglers. Does that seem right to you?
| category: bug | ||
| domain: data-sync | ||
| domainFit: strong | ||
| issueNumber: 10182 |
There was a problem hiding this comment.
issue (blocking): this issueNumber resolves to a PR, not an issue
10182 is a pull request (fix(#10183):); on GitHub PR 10399 closes nothing, and the real issue reads as 10183 ("Replication fails to filter out reports with needs_signoff set to false"), which matches this draft's summary. So this looks like the one draft the relink missed. After the resolver change it should re-stamp to 10183 (id/issueNumber/issueUrl). Does that match your read of it?
| @@ -0,0 +1,83 @@ | |||
| --- | |||
| id: cht-core-10792 | |||
There was a problem hiding this comment.
note (non-blocking): id isn't unique here, and that's probably fine, just worth a decision
10793/10798/10799 all resolve to issue 10792, so all three carry id: cht-core-10792. Harmless today (nothing keys on id), but schema.json:62 and TEMPLATE.md:81 call id a "Unique identifier", which the corpus can't honor once multiple PRs fix one issue. Lightest fix is just softening that wording, plus making sure the #135 consumer dedups on issueNumber rather than id. A composite cht-core-<issue>-pr<pr> is the honest long-term shape if anything ever needs an id-keyed map, but that's a corpus-wide re-stamp and probably not worth it now. Curious which way you'd lean.
| /** Single sidebar closing-ref is ground truth; use it even if the token disagrees. */ | ||
| function resolveFromSidebar(authoritative: number, token: Token | null): Resolution { | ||
| const tokenMismatch = token ? token.issueNumber !== authoritative : undefined; | ||
| return relink(authoritative, 'gh', tokenMismatch); |
There was a problem hiding this comment.
suggestion (non-blocking): cover the tokenMismatch=true path
This branch (sidebar wins over a disagreeing filename token) is the safety-critical override and the only one that sets the TOKEN-MISMATCH audit flag, but the spec only asserts tokenMismatch === false. A case where the sidebar closing-ref and the filename token disagree would lock it in.
| let fm = block[2]; | ||
| const repoName = repo.split('/')[1]; // medic/cht-core -> cht-core | ||
| const edits: Array<[RegExp, string]> = [ | ||
| [/^id: cht-(?:core|interoperability)-\d+$/m, `id: ${repoName}-${newIssue}`], |
There was a problem hiding this comment.
nitpick (non-blocking): not quite repo-agnostic despite the doc
The id/issueUrl match regexes hardcode owner medic and the two repo names, so a non-medic repo would throw rather than rewrite. Either generalize or tweak the "repo-agnostic" comment. (No CRLF / non-medic test exists either.)
| const raw: Array<{ number: number; url?: string }> = Array.isArray(meta.closingIssuesReferences) | ||
| ? meta.closingIssuesReferences | ||
| : []; | ||
| return raw.filter(r => typeof r.url === 'string' && r.url.includes(`/${repo}/issues/`)); |
There was a problem hiding this comment.
nitpick (non-blocking): unanchored substring match
url.includes('/<repo>/issues/') also passes for a non-host occurrence (e.g. https://github.com/attacker/medic/cht-core/issues/1). Not exploitable on trusted GitHub data, but anchoring to the host would be tidier.
… — addresses #129 review Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @sugat009 — all seven points addressed. Rather than patch just the 10399 case, we enumerated the full "a number could be an issue OR a PR" space and handled it systematically. All checks green (611 tests, coverage, SonarCloud). Root causeGitHub serves PRs through the issues endpoint, so Blocking
Non-blocking
Extra cases the enumeration caught (beyond 10399)
Re-requesting review 🙏 |
sugat009
left a comment
sugat009
left a comment
There was a problem hiding this comment.
Re-reviewed at a152324 against the live cht-core API, the code, and the tests. The code fix is excellent and fully closes the issue-vs-PR gap, including the nested 10399 case, with a direct unit test for it; the schema id wording, the tokenMismatch=true test gap, and the substring/doc nitpicks are all resolved. One data step remains: the relink wasn't re-applied, so the committed 10399 draft still carries a PR number (inline). Requesting changes just for that re-run.
| * Returns issue:null with a reason for missing / no-issue / multi-issue. | ||
| * @throws {GhTransientError} on a transient gh failure. | ||
| */ | ||
| export function resolveRealIssue(repo: string, n: number, exec: ExecFn, cache?: ClassifyCache): ResolveResult { |
There was a problem hiding this comment.
praise: clean resolution of the gap. Classifying via the pull_request key, following a PR to its sole closingIssuesReferences (one authoritative hop), and surfacing GhTransientError so a throttled lookup can't demote a real issue, all exactly right, and more robust than the review asked for.
| * Resolve an affected draft's true issue: try the source PR's closing-refs first | ||
| * (most authoritative), then follow the stored issueNumber if it is itself a PR. | ||
| */ | ||
| function resolveAffectedIssue(src: SourcePr, issueNumber: number | undefined, gh: GhCtx): ResolveResult { |
There was a problem hiding this comment.
note: verified this recovers the nested case. Trying the source PR first, then following the stored issueNumber when the source closes nothing, is what turns 10399 into 10183 (source 10399 has empty closing-refs, so it falls to issueNumber 10182 [a PR], whose closing issue is 10183). Multi-issue correctly short-circuits to a flag rather than guessing off issueNumber. Nicely done.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Done. Ran Once merged into main, I'll rebase the other memory/promote-* branches and relink the issues per domain before re-requesting the review on the PR. |
sugat009
left a comment
sugat009
left a comment
There was a problem hiding this comment.
Deep re-review at 58e1e0c. The data is now whole: all 14 data-sync drafts resolve to real issues (checked against GitHub closing-refs), the 10399 -> 10183 relink is correct, and the id-collision is now documented in schema.json with the #135 dedup contract. The resolution core (gh-classify / issue-linkage / the relink tool) is sound: frontmatter-only rewrites verified, regexes linear, fails closed. Approving. A few non-blocking refinements inline. The fail-loud gap in hydrateIssue is the one I'd most want your read on, here or as a follow-up. Nothing blocks on data grounds.
| raw = exec('gh', ['api', `repos/${repo}/issues/${n}`]); | ||
| } catch (err) { | ||
| const text = errText(err); | ||
| if (/HTTP 404/i.test(text)) return null; // 404 only — never treat a transient "not found" as missing |
There was a problem hiding this comment.
praise: The GhTransientError design here is the right backbone for this PR. Treating only HTTP 404 as missing and raising on everything else means a rate-limited lookup can never demote a real issue to missing, the exact failure that would silently mis-attribute a draft. resolveRealIssue's one-hop, single-closing-issue conservatism (0 or >1 both yield null) is nicely defensive too.
| const parsed = JSON.parse(raw); | ||
| const comments: string[] = (parsed.comments ?? []).map((c: { body: string }) => c.body); | ||
| return { number: n, body: parsed.body ?? '', comments }; | ||
| } catch { |
There was a problem hiding this comment.
issue (non-blocking): hydrateIssue swallows every gh issue view failure into null, transient (rate-limit / 5xx) as well as a real 404. That undoes the fail-loud guarantee the rest of the module works for: resolveRef (L87) trusts closing-ref numbers directly without a classify call, so for the highest-authority source this is the only gh touch and has no transient protection. Under bulk-run throttling, a closing-ref issue can be dropped silently and a lower-authority body ref promoted into linkedIssues[0], the same silent mis-attribution this PR exists to prevent. resolveRef's own docstring (L80-84) even promises transient errors propagate rather than drop a linkage. Could hydrateIssue mirror fetchIssueRecord, return null only on HTTP 404 and throw GhTransientError otherwise? Open to whether that's worth doing here or as a follow-up, since the pattern predates this PR.
|
|
||
| function readCtx(file: string): FileCtx { | ||
| const content = fs.readFileSync(file, 'utf8'); | ||
| const fm = matter(content).data as Record<string, unknown>; |
There was a problem hiding this comment.
issue (non-blocking): matter(content) isn't guarded, so a single draft with malformed YAML frontmatter throws out of planFile and aborts the whole relinkIssues run with a raw stack and no report. It fails closed (all planning precedes applyRelinks, so nothing is half-written), which is the important part, but as a corpus-maintenance tool it'd be friendlier to catch per-file and flag it as unparseable alongside the other flagged files. Minor, since real drafts are schema-validated on the way in.
| /** Resolve + dedup + hydrate one ref; marks `seen` only on a successful new issue. */ | ||
| function linkOneRef(ref: IssueRef, repo: string, cache: ClassifyCache, seen: Set<number>): LinkedIssue | null { | ||
| const issueNum = resolveRef(ref, repo, cache); | ||
| if (issueNum === null || seen.has(issueNum)) return null; |
There was a problem hiding this comment.
suggestion (non-blocking): This resolveRef -> null -> drop line is the safety valve that stops a PR number being recorded as the issue, but it isn't pinned by a scrapePR-level test. The PR-following tests cover the single-closing-issue happy path; none drives a title/body ref whose resolveRealIssue returns null (multi-issue / no-issue / 404). If this regressed to ... ?? ref.number, the original PR-as-issue bug returns and every scraper test still passes (it's only unit-tested in gh-classify.spec). A scrapePR case where a title ref points at a PR closing 0 or >1 issues, asserting the ref is dropped, would lock it down.
| */ | ||
|
|
||
| /** The source that contributed a linked-issue number, by descending authority. */ | ||
| export type IssueSource = 'closing-ref' | 'title' | 'body'; |
There was a problem hiding this comment.
thought (non-blocking): IssueSource is a nice model, but the resolved source is used only for ordering and isn't persisted onto the draft. In the current corpus 4 of 14 links are title-derived (the PR closes nothing per GitHub's closing-refs, so the number comes from the fix(#N) title, the exact error-prone signal this PR fixes). They all resolve to real issues here, but they're indistinguishable from authoritative links once written. Persisting a linkSource/confidence field, or at least flagging title-only links, would let a human audit the low-confidence ones. Future-auditing thought, not a blocker.
2 participants
Promotes 14 strong-fit
data-syncdrafts fromagent-memory/_pending/intoagent-memory/domains/data-sync/issues/for squad content review.Categories: bug (6), improvement (4), feature (4)
Themes: replication keys & primary-contact depth, purging/pre-purge counts, Nouveau/offline view design docs, service-worker UI extensions.
All 14 carry
domainFit: strong+ a## Domain Rationalesection. 38 weak-fit drafts deferred to Stream C —data-syncwas the largest catch-all in this run (73% weak), so this PR is the distilled minority; the over-broad-domain question is for later review.seeding-claude-cli-v2(feat(#108): seeding pipeline - CLI provider, domain-rationale, infrastructure domain, concurrency #119) — retarget tomainafter the schema lands.validate-schema: passing, 0 failures.