Skip to content

docs(#136): memory distillation pipeline first-run post-mortem#137

Open
alexosugo wants to merge 2 commits into
mainfrom
review/memory-pipeline-postmortem
Open

docs(#136): memory distillation pipeline first-run post-mortem#137
alexosugo wants to merge 2 commits into
mainfrom
review/memory-pipeline-postmortem

Conversation

@alexosugo

@alexosugo alexosugo commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds docs/memory-pipeline-postmortem.md - the consolidated post-mortem of the first production
run
of the memory distillation pipeline (scraper -> filter -> distiller -> open-review-pr), run
against merged medic/cht-core PRs and surfaced as the 8 open chore(memory): promotion PRs
(#120, #121, #122, #123, #129, #130, #131, #132).

~107 distilled memories were reviewed, spot-checked against
the live cht-core API.

Key finding

Distillation prose is good; the failures are in plumbing. One defect dominates:

PR-number-as-issue-number (distiller.ts:267). The scraper only parses body
Fixes/Closes/Resolves #N, but CHT encodes the issue in the PR title type(#N):, which is
never parsed - so the ?? pr.prNumber fallback fires and id/issueNumber/issueUrl name the
merge PR. ~60/107 drafts affected. The /issues/N -> /pull/N redirect masks it, so
validate-schema passed while shipping wrong data. This defeats the planned #135 de-dup consumer.

What's in the report

  • Overall patterns + recurring failure classes across all 8 PRs
  • Root-cause analysis mapped to specific prompt lines / truncation limits / schema fields
  • Prompt + pipeline improvements, each mapped to the feedback it addresses
  • An 8-item prioritized roadmap (by impact)
  • A 3-layer evaluation strategy + golden-set plan
  • A 20-scenario regression catalog (each row = a reviewer-caught defect as a permanent test)
  • Documentation updates for the next iteration
  • Per-PR appendix

Related

Test plan

  • Docs-only change; no code affected. Review the report for accuracy against the source PRs.

Consolidated findings from reviewing the 8 chore(memory): promotion PRs
(#120-#132): dominant PR-number-as-issue-number defect (~60/107 drafts),
missing de-duplication, static frontmatter, and prose grounding gaps.
Includes root-cause analysis, prioritized roadmap, a 3-layer evaluation
strategy, and a 20-scenario regression catalog.

@sugat009 sugat009 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comprehensive review of the post-mortem, every claim re-checked against the live GitHub API and source, unbiased. The core is solid and this is a valuable doc: the prose-vs-plumbing thesis, every source-code reference, all 15 PR->issue resolutions, and the backport/cross-domain analysis all hold up. A handful of inline corrections before it becomes the reference gating #135/#104: two factual errors (the resolution priority chain is inverted vs the shipped code; the 10623 hallucination example is wrong), and three staleness/labeling items (#129's shipped scope, the roadmap already living in #138, and the "60/107 corpus-wide" scope). All non-blocking, all inline.

Comment thread docs/memory-pipeline-postmortem.md Outdated
## 4. Pipeline improvements that address multiple issues

1. **Resolve the canonical issue with a priority chain; stop aliasing the PR number.**
`(1)` body `Fixes/Closes/Resolves` -> `(2)` CHT title `type(#N):` -> `(3)` GraphQL

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: The recommended resolution order here (body Fixes/Closes/Resolves -> title type(#N): -> closingIssuesReferences) is inverted relative to the shipped code. issue-linkage.ts (byte-identical on #129 and #138) orders closing-ref -> title -> body as explicit "descending authority", with closingIssuesReferences the most authoritative and linkedIssues[0] canonical. A closing-ref is GitHub's authoritative link, so it should win over an author-typed body keyword, not lose to it. In the divergence case (title #N, empty closing-refs, body Closes #M) this doc picks #M while the code correctly picks the authoritative signal. The same ordering flows into the Part 2 eval chain (L213-214) and the Part 3 title vs body precedence fixture (L254), which would then encode the wrong canonical issue. Suggest flipping to closingIssuesReferences -> title -> body throughout to match the implementation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — verified against issue-linkage.ts on the #129/#138 branches (collectLinkedIssueRefs pushes closing-ref → title → body, "descending authority", first entry canonical). Flipped the chain to closingIssuesReferences > title > body throughout: section 4.1, the Part 2 unit chain, the Part 3 fixture (now "title outranks body", issueNumber=8026), and the Part 4 precedence line. Fixed in ba5b5ce.

Comment thread docs/memory-pipeline-postmortem.md Outdated
> `relatedFiles` and `entities` MUST be chosen only from the Files changed list above. Do not
> infer, guess, or invent paths. If a file is not in that list, do not include it.

*Fixes:* hallucinated paths (#123/10623 - `message.pipe.ts`, `reducers/tasks.ts` not in the PR).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: This example does not hold up. PR 10623 does change webapp/src/ts/pipes/message.pipe.ts and webapp/src/ts/reducers/tasks.ts (plus reducers/global.ts and their specs) per gh api repos/medic/cht-core/pulls/10623/files, so the draft's file list is accurate, not hallucinated. Since 10623 is the flagship "hallucinated paths" case (also cited in the Part 3 fixture at L258 and the appendix at L399-400), it should be dropped or swapped for a real case. The other prose examples (Slack L341, "cf. Seed 4" L382) do check out, so the point survives, just not this instance.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed via gh api .../pulls/10623/files: message.pipe.ts and reducers/tasks.ts are both in the real PR. But the draft (10623-feat10577-…​.md) also lists services/user-contact.service.ts, which is not in the PR — so 10623 is still a valid hallucination, just misdescribed. Kept it as the exemplar and corrected the file facts at L118/L267/L409. Fixed in ba5b5ce.

Comment thread docs/memory-pipeline-postmortem.md Outdated

PR #129 (data-sync) already shipped a **partial fix** (`gh-classify.ts` using the `pull_request`
API key to tell issues from PRs, plus `issue-linkage.ts` and a one-off `relink-issues.ts`), but
its "affected" predicate (`issueNumber === source_pr`) was narrower than the bug (missed the

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (stale): This understates what #129 already shipped. On the #129 branch: the nested 10399 -> 10182(PR) -> 10183 case is handled and regression-tested (relink-issues.spec.ts:96 asserts to === 10183); title parsing is wired into the forward path (scraper.ts:305 collectLinkedIssueRefs(prTitle, ...)); and the ?? pr.prNumber alias is gone from the forward write path (distiller.ts:268 uses linkedIssues[0]?.number and throws/flags when absent). Of the three gaps listed, only deduplication is genuinely still open in #129. Suggest narrowing this to dedup.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified on #129 head (58e1e0c): scraper.ts wires collectLinkedIssueRefs(prTitle, …), distiller.ts:268 dropped the ?? pr.prNumber alias and flags no-issue PRs, and relink-issues.spec.ts:96 covers the nested 10399→10182→10183 case. Narrowed the paragraph to deduplication as the one open gap. Fixed in ba5b5ce.


| Rank | Item | Impact | Effort | Rationale |
|:---:|---|:---:|:---:|---|
| **1** | Complete the issue-resolution chain (title `type(#N)` + `closingIssuesReferences`) and remove the `?? pr.prNumber` alias; set `issueNumber` null + flag when unresolved | high | med | Repairs the ~60/107 mislink that corrupts the corpus key and blocks #135. Extends #129's existing `gh-classify.ts`/`issue-linkage.ts`. |

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: Most of this roadmap is already implemented in the open #138 (same author). Ranks 1-3 and 5-8 map to dedup.ts (ciGuardReason, dedupeByIssueId + source_prs[]), reconcile.ts (hallucinationRate), the CONSTRAINTS block, stripBoilerplate + ISSUE_BODY_LIMIT_EXPANDED, and computeConfidence. Worth reframing as "tracking #138" so it reads as in-flight rather than net-new. One caveat: rank 4's specific design (nullable issueNumber, /pull URLs, cht-core-pr-<n>) is contradicted by #138, which keeps issueNumber required (distiller.ts:329 still emits /issues/${issueNumber}) and flags/skips no-issue PRs instead. Rank 4 should be rewritten to match the require-an-issue-or-skip approach that actually shipped.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Added a status note framing the roadmap as tracking #138 (ranks 1-3, 5-8 → dedup.ts/reconcile.ts/CONSTRAINTS/stripBoilerplate/computeConfidence). Rewrote rank 4 and section 4.2 to the shipped require-an-issue-or-skip approach (issueNumber stays required, no-issue PRs flagged), and fixed the Part 4 invariant. Fixed in ba5b5ce.

Comment thread docs/memory-pipeline-postmortem.md Outdated
> `fixes|closes|resolves #N`, but CHT encodes the issue in the **PR title** `type(#N):`, which is
> never parsed. So for the common case `linkedIssues` is empty, the `?? pr.prNumber` fallback
> fires, and the memory's primary key names the **merge PR** instead of the resolved issue.
> **~60 of 107 drafts corpus-wide** are affected. The `/issues/N -> /pull/N` GitHub redirect makes

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: "~60 of 107 drafts corpus-wide" is really the four fully-reviewed domains, not the corpus. The 8-domain table above sums to 255 memories; 60/107 is exactly contacts+auth+config+data-sync (44+39+10+14 = 107; mislinks 31+19+4+6 = 60). Section 1.1 (L63) then lists tasks (15/35) in the same sentence but still totals 60/107. Suggest relabeling "60/107 across the four fully-reviewed domains" (corpus-wide would be higher, out of 255) to keep the headline stat precise.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relabeled: "~60 of 107 across the four fully-reviewed domains (contacts/auth/config/data-sync)" at L41, and reworked the 1.1 line so 60/107 is the four domains with tasks/messaging/forms/infra called out as additional. Fixed in ba5b5ce.

Comment thread docs/memory-pipeline-postmortem.md Outdated
#121; #135 (de-dup consumer); cross-domain dupes with #131 (#9835/#9065/#6543).
- **[incorrect | sugat009]** 31/44 drafts: `issueNumber`/`issueUrl`/`id` name the merge PR. e.g.
10043 stores `issueNumber:10043` (PR `feat(#10036):...`) but the real issue is #10036.
- **[duplication | sugat009]** #10038 x5, #10036 x3, #8985 x2 (9027 + its 4.7.x backport 9098), etc.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick (minor, non-blocking): A few small accuracy items in the appendix/fixtures: #10036 is x4 not x3 (merged PRs 10070/10061/10056/10043). "all 44 ... confidence: medium / related_issues: []" (L345) holds for the distilled drafts but not 4 legacy subDomain-schema drafts that carry neither field. Two Part 3 fixture titles (8843, 8675 at L252/255) are paraphrased rather than the verbatim PR titles. And CouchDB 9960/10014 (L421) close different issues (9882 vs 10027), so they are duplicated effort, not a single-issue backport pair.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All four fixed in ba5b5ce: #10036 → x4 (10070/10061/10056/10043); noted the 4 legacy subDomain-schema drafts lack confidence/related_issues; used the verbatim 8675/8843 titles; and clarified CouchDB #9960/#10014 close different issues (#9882 vs #10027) — duplicated effort, not a backport pair.

- Flip issue-resolution precedence to match shipped issue-linkage.ts
  (closingIssuesReferences > title > body, descending authority)
- Fix PR 10623 hallucination facts: only user-contact.service.ts is
  invented; message.pipe.ts/reducers/tasks.ts are real
- Narrow #129 open gaps to deduplication (title-parse + forward-write
  path + nested-chain case already shipped and tested)
- Reframe roadmap as tracking #138; rewrite rank 4 to the shipped
  require-an-issue-or-skip approach
- Relabel 60/107 as the four fully-reviewed domains, not corpus-wide
- Appendix: #10036 x4, subDomain-schema draft caveat, verbatim 8675/8843
  titles, CouchDB 9960/10014 close different issues
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

2 participants