diff --git a/.planning/phases/08-github-action-readme/08-DEMO-COMPLETE-RISK-REVIEW.md b/.planning/phases/08-github-action-readme/08-DEMO-COMPLETE-RISK-REVIEW.md new file mode 100644 index 0000000..2d25c28 --- /dev/null +++ b/.planning/phases/08-github-action-readme/08-DEMO-COMPLETE-RISK-REVIEW.md @@ -0,0 +1,139 @@ +# Risk Review: `08-DEMO-COMPLETE.md` + +## 1. Plan Summary + +**Purpose.** Prove a complete, user-visible Phase 8 demo in `Galzi1/github-pr-kb-demo`, ending with a merged feature PR, a generated KB PR containing at least one real article, a stable post-merge workflow run, and a clean final repo state. + +**Key components touched.** + +| Component | Role in the plan | +| --- | --- | +| Demo repository `Galzi1/github-pr-kb-demo` | Consumer repo where the Phase 8 workflow is exercised end to end | +| GitHub Actions workflow | Triggers on merge events, runs extract/classify/generate, opens KB PRs, and persists cursor state | +| Extraction / classification / generation pipeline | Converts merged PR comments into KB artifacts | +| Anthropic-backed classifier | Converts raw comment text into one of the supported categories or review-needed output | +| KB output (`kb//`, `kb/INDEX.md`, `kb/.manifest.json`) | Observable proof that knowledge was generated and published | +| Human review / merge steps | Provide the source review comments, validate article fidelity, and close the loop by merging the KB PR | + +**Plan-stated assumptions.** + +1. Previous workflow hardening means the plumbing path is already sound enough that the remaining gap is content generation. +2. Existing logs and artifacts from PR `#1` are sufficient to diagnose why all 10 comments failed classification. +3. A new, deliberately small feature PR with high-signal review comments is likely to produce at least one publishable article. +4. One successful curated run is enough to justify calling the demo "100% confidence" complete. + +**Theory of success.** The plan succeeds if the current classification failure mode is first understood, then a fresh PR with clearly knowledge-bearing comments is merged, the workflow produces a real KB article, humans confirm the article is faithful, and the system reaches steady state without opening another automation PR. + +## 2. Assumptions & Evidence + +This plan is strongest where it is explicit about the current gap and weakest where it assumes a single successful rerun will convert uncertainty into confidence. + +| ID | Assumption | Explicit / Implicit | Class | Justification status | Blast radius if wrong | How to test before committing fully | +| --- | --- | --- | --- | --- | --- | --- | +| A1 | The PR `#1` failures are diagnosable from existing logs/artifacts without reproducing the run in a different way. | Explicit | Foundational | **Partial.** The plan names this as the first todo, but does not confirm artifact depth or whether failed model outputs were retained. | If false, the team may start a new demo without understanding whether the problem is content quality, classifier robustness, caching, or API behavior. | Pull the exact workflow logs/artifacts for the failed run and verify they reveal concrete failure reasons rather than only aggregate counts. | +| A2 | Workflow mechanics are already sufficiently proven, so the remaining uncertainty is mainly in the content/classifier path. | Implicit | Structural | **Mostly justified.** Multiple runs reportedly validated branch creation, persistence, and steady state. Still, content-path failures can expose workflow-path weaknesses hidden by zero-article runs. | If false, a second demo may fail for both workflow and content reasons, making diagnosis slower and conclusions weaker. | Run a preflight checklist on permissions, refs, secrets, and branch protections immediately before the next live demo. | +| A3 | Better-crafted review comments will classify cleanly. | Explicit | Foundational | **Weak.** The previous outcome was "10 failed" rather than merely "low confidence" or "other," which suggests a possibly systemic issue, not just low-signal input. | If false, the next demo repeats the same failure and consumes manual effort without increasing confidence. | Dry-run representative review comments through the classifier outside the live demo path or inspect the exact prior failure mode first. | +| A4 | Supported categories and prompt behavior are predictable enough that humans can intentionally author comments that land in `gotcha`, `code_pattern`, or `domain_knowledge`. | Explicit | Structural | **Partial.** This is plausible, but only if classifier output is stable and parsing is robust. The current evidence does not show that. | If false, the demo becomes dependent on prompt luck rather than reproducible behavior. | Create 2-3 example comments and validate local classification results before the merge-triggered workflow run. | +| A5 | A single curated successful run is enough to claim "100% confidence in the demo." | Explicit | Structural | **Unjustified.** One scripted success proves the happy path, not robustness across comment phrasing, timing, API variability, or cache state. | If false, the plan may overclaim readiness and hide brittleness that appears immediately after the demo. | Reframe the claim as "happy-path demo complete" unless at least one additional variant or replay also succeeds. | +| A6 | Manual reviewers will provide comments that are both product-realistic and classifier-friendly, then judge article fidelity consistently. | Implicit | Structural | **Weak.** The plan requires manual precision but does not define an authoring rubric or acceptance rubric. | If false, the demo outcome may hinge on ad hoc human choices rather than product behavior. | Provide explicit comment-writing examples and a short article-acceptance checklist before starting the live run. | +| A7 | External dependencies will behave consistently during the next run: GitHub permissions, workflow refs, model availability, API quotas, and repository state. | Implicit | Structural | **Partial.** Prior runs worked, but these are live dependencies outside the plan's control. | If false, a demo failure may be misread as a product failure when it is an environment failure. | Do a same-day preflight against secrets, permissions, tool ref reachability, and API readiness. | +| A8 | "No open PRs" is a valid final exit criterion for the demo repo. | Explicit | Peripheral | **Weak.** It is precise for automation churn, but too broad if unrelated human PRs can exist. | If false, the plan can fail its own exit criteria even when the automation loop worked correctly. | Narrow the criterion to "no open automation PRs created by github-pr-kb." | + +**Secrets vs. mysteries.** + +- **Secrets:** the exact cause of the 10 failed classifications, whether cache state affects replays, whether example comments classify locally, whether the workflow still has correct permissions and refs. +- **Mysteries:** whether a human reviewer will judge the generated article "useful" enough, whether future non-curated PR comments will produce similarly good output. + +The current plan is strongest when it treats unresolved classifier behavior as a **secret** to investigate. It becomes weaker when it treats long-term confidence as if it can be resolved by one curated run, which is closer to a **mystery** that must be managed with bounded claims. + +## 3. Ipcha Mistabra - Devil's Advocacy + +### 3a. The Inversion Test + +**Claim:** A fresh PR with better comments will likely complete the content demo. + +**Inversion:** Better comments may not matter at all. "10 failed" classifications points to a parser, prompt, API, schema, or cache defect more than a simple comment-quality issue. In that world, the next PR adds more manual work while reproducing the same opaque failure. + +**Why this inversion is compelling:** The prior evidence is not "0 useful comments" but "0 classified, 10 failed." That smells like system behavior, not merely weak content. + +--- + +**Claim:** Once one curated run succeeds, the demo can be called complete with 100% confidence. + +**Inversion:** A single curated success could actually reduce epistemic rigor by creating false confidence. It would prove that one carefully staged path worked once under current conditions, not that the system is stable, repeatable, or resilient to minor variation. + +**Why this inversion is compelling:** Demo success and product confidence are not the same thing. A curated demo is a marketing-quality proof, not an operational-quality proof. + +--- + +**Claim:** The workflow path is already validated, so the remaining work is mostly content-path verification. + +**Inversion:** Content-path verification may be exactly where workflow-path weaknesses reappear. A run that generates real articles can trigger branch, manifest, dedup, PR diff, and post-merge behavior that zero-article runs never exercised in the same way. + +**Why this inversion is compelling:** "Real content exists" is not just more of the same path; it can activate new edge cases. + +### 3b. The Little Boy from Copenhagen + +**A new engineer joining next month** would likely ask: "What exact failure signature distinguishes bad input from broken classification?" The current plan does not define that boundary. It assumes the first diagnosis task will discover it, but the execution plan after that still relies heavily on intuition. + +**An SRE or on-call maintainer at 3 AM** would likely ask: "If the next run produces no article, how do I tell in five minutes whether extraction, classification, generation, dedup, or permissions caused it?" The plan has outcome checks, but not a stage-specific observability checklist. + +**A repo maintainer** would likely ask: "Why is 'no open PRs' part of success?" If another unrelated PR exists, the plan fails its own exit criteria for a reason that has nothing to do with Phase 8. + +**A skeptical user of the product** would likely ask: "How do I know the generated article is actually faithful rather than superficially plausible?" The plan includes human review, which is good, but it does not define what fidelity means. + +### 3c. Failure of Imagination Check + +1. **Cache poisoning / stale-state scenario.** The next run may reuse cached classification or manifest state in a way that masks improvement or suppresses article generation, making a good comment set look like a product failure. +2. **Parseable-but-wrong scenario.** The classifier may stop "failing" but still return low-confidence or `other` results that technically pass through the pipeline while still not producing the proof the demo needs. +3. **Article-generated-but-filtered scenario.** Classification may succeed, but generation may deduplicate or skip the article because of manifest state, leading to another KB PR that looks empty in the place that matters. +4. **Environment-drift scenario.** The plan assumes the same repo settings, token scopes, action refs, and external model behavior remain stable between prior validation and the next demo. Live systems drift. +5. **Success-but-not-credible scenario.** The demo may succeed only because comments were written in a classifier-optimized way that real reviewers would never write, making the demo impressive but strategically misleading. + +## 4. Risk Register + +| Risk ID | Category | Description | Trigger | Probability | Severity | Priority | Detection | Mitigation | Contingency | Assumption link | Knowledge class | +| --- | --- | --- | --- | --- | --- | --- | --- | --- | --- | --- | --- | +| R1 | Technical | The real root cause of the PR `#1` "10 failed" outcome is not isolated before running the next demo. | A new feature PR is merged before the failure signature is understood. | High - the plan still lacks the actual cause. | High - the next demo can repeat the same failure and waste the main proving attempt. | High | Another run shows `0 classified` and failed counts without actionable diagnosis. | Make Todo #1 a hard gate: inspect logs/artifacts and, if needed, reproduce locally until the failure mode is concrete. | Pause the demo-completion claim and switch scope to classifier-path repair. | A1, A3 | Unknown known | +| R2 | Technical | The next review comments are human-meaningful but still do not map cleanly to the classifier's expected prompt/schema/category behavior. | Comments are merged without first validating example phrasing. | Medium | High - no article means the demo is still incomplete. | High | Low-confidence, `other`, empty, or failed classifications on otherwise good comments. | Create a short comment-authoring rubric and validate 2-3 samples before the live PR merge. | Open another small PR or adjust classifier robustness before retrying. | A3, A4, A6 | Known unknown | +| R3 | Operational | Observability is too weak to separate extraction, classification, generation, dedup, and permission failures quickly during the live demo. | The next run again produces no article or partial output. | Medium | High - diagnosis becomes slow and confidence claims remain fuzzy. | High | Logs show counts but not decisive per-stage reasons or retained failing payloads. | Add a pre-demo diagnostic checklist and preserve the exact artifacts/log lines needed for stage-level attribution. | Download artifacts immediately and perform a post-mortem before any retry. | A1, A2 | Known unknown | +| R4 | Organizational | A single curated successful run is over-interpreted as "100% confidence," masking fragility and shrinking future learning. | The first successful article-producing run is treated as full proof. | High | Medium - the product story becomes stronger than the evidence. | High | The final write-up contains categorical confidence claims unsupported by repeatability evidence. | Reframe the target as "complete happy-path demo" unless at least one additional variant also works. | Document confidence boundaries explicitly in the completion note. | A5 | Known known | +| R5 | Operational | External dependencies drift between the prior validated runs and the next live demo. | Token scopes, action permissions, refs, branch protections, or model/API behavior change. | Medium | High - a live demo can fail for reasons unrelated to the product logic. | High | Auth errors, checkout/ref errors, API failures, or PR creation failures appear in the workflow. | Perform a same-day environment preflight before merging the new feature PR. | Repair environment issues and rerun without counting the failed attempt as product evidence. | A2, A7 | Known unknown | +| R6 | Organizational | Manual steps are underspecified, so comment authoring and article-fidelity review vary too much between runs or reviewers. | Humans write comments or assess generated content without a shared rubric. | Medium | Medium | Medium | Comments are inconsistent in style/specificity, or reviewers disagree on whether the article is faithful. | Provide exact examples of acceptable review comments and a short fidelity checklist. | Re-review the KB PR or create a fresh demo PR with better source comments. | A6 | Mystery | +| R7 | Technical | Cache or manifest state suppresses article generation even when classification improves. | Prior cache entries or manifest mappings are unexpectedly reused during the next run. | Medium | High - the team may misdiagnose success as failure or failure as success. | High | Nonzero extraction/classification activity but `0 new` generation or unexplained skips. | Inspect cache and manifest behavior explicitly as part of pre-demo readiness. | Reset or isolate demo state, or rerun in a fresh environment with documented cache behavior. | A2, A3 | Unknown unknown | +| R8 | Operational | The exit criterion "no open PRs" blocks success for irrelevant reasons. | Any unrelated PR exists during or after the demo. | Medium | Low | Medium | Core demo behavior succeeds, but the repo still has an open non-automation PR. | Narrow the criterion to automation PRs created by the workflow. | Snapshot repo state and declare success relative to automation artifacts only. | A8 | Known known | + +## 5. Verdict & Recommendations + +**Overall risk level: High.** The plan is honest, structured, and close to a viable execution checklist, but it rests on one load-bearing uncertainty: whether the prior all-failed classification result was caused by input quality or by a systemic defect. Until that is resolved, the proposed next demo attempt is at meaningful risk of being an expensive repetition rather than a completion. + +**Top 3 risks.** + +1. **R1 - unresolved classifier failure mode** +2. **R3 - insufficient stage-level observability during the live run** +3. **R4 - overclaiming confidence from one curated success** + +**Recommended actions before the next live demo attempt.** + +1. **Promote diagnosis to a hard gate.** Do not merge a new feature PR for the "complete demo" path until the PR `#1` failure mode is written down concretely: malformed model output, parse mismatch, API failure, cache interaction, prompt mismatch, or something else. +2. **Add a comment-input contract.** Prepare 2-3 example review comments that are both realistic and intentionally aligned to the supported categories, then validate them before using them in the live demo. +3. **Add a five-minute triage checklist.** Define what evidence distinguishes extraction failure, classification failure, generation failure, dedup/manifest suppression, and permissions/environment failure. +4. **Reduce claim scope.** Replace "100% confidence" with a bounded statement unless you also demonstrate repeatability or at least one meaningful variant. +5. **Tighten exit criteria.** Change "no open PRs" to "no open automation PRs created by the workflow," and define a brief article-fidelity checklist so the manual review step is less subjective. + +**Open questions.** + +1. What exact error produced the "10 failed" classification result on the prior run? +2. Are the failing classifier outputs retained anywhere inspectable, or do logs only expose aggregate counts? +3. Could cache or manifest state suppress article creation even when classification succeeds? +4. What specific rubric will be used to decide that a generated article is faithful enough to merge? +5. Is the next demo intended to prove only the happy path, or also repeatability? + +**What the plan does well.** + +- It is unusually honest about the current gap: the missing proof is not workflow plumbing, but real article generation. +- It includes a non-negotiable gate that rejects false completion if the next run still produces no articles. +- It separates automatic and manual responsibilities clearly enough to execute. +- It defines a concrete desired end state in the repo rather than relying on vague notions of "done." + +**Bottom line.** This is a credible near-final demo checklist, but not yet a low-risk completion plan. Treat the prior classifier failure as the critical unknown, narrow the confidence language, and add sharper observability and manual rubrics before running the next "complete demo" attempt. diff --git a/.planning/phases/08-github-action-readme/08-DEMO-COMPLETE.md b/.planning/phases/08-github-action-readme/08-DEMO-COMPLETE.md new file mode 100644 index 0000000..77f12f2 --- /dev/null +++ b/.planning/phases/08-github-action-readme/08-DEMO-COMPLETE.md @@ -0,0 +1,303 @@ +# Phase 8 demo completion status for `github-pr-kb-demo` + +## Goal + +Reach a **complete end-to-end demo** of Phase 8 in `Galzi1/github-pr-kb-demo` and stop only when we have evidence for the full user-visible loop: + +1. a normal feature PR is opened in the demo repo, +2. review comments are added on that PR, +3. the PR is merged, +4. the Phase 8 workflow automatically extracts/classifies/generates KB output, +5. the workflow opens a KB PR that contains at least one real article under `kb//`, +6. that KB PR is reviewed and merged, +7. the follow-up workflow run succeeds and does **not** create another KB PR, +8. `main` ends with the generated article, updated `kb/INDEX.md`, updated `kb/.manifest.json`, no open automation PRs, and a successful latest workflow run. + +## What is already done + +| Status | Item | Owner | Evidence | +| --- | --- | --- | --- | +| Done | Public demo repo `Galzi1/github-pr-kb-demo` was created and scaffolded with the copied workflow. | Copilot | Repo exists and workflow is installed on `main`. | +| Done | Source workflow was hardened for real-world demo use. | Copilot | Fixed parse-safe secret checks, corrected `KB_TOOL_REPOSITORY` to `Galzi1/github-pr-kb`, and fixed cursor persistence. | +| Done | Demo PR `#1` was opened, reviewed, updated, and merged. | Copilot + user flow | PR `#1` merged successfully. | +| Done | Manual no-op path was validated. | Automatic GitHub Actions flow | Run `24242554122` succeeded. | +| Done | Real merged-PR publish path was validated. | Automatic GitHub Actions flow | Run `24245335864` reached KB publication and opened PR `#2`; later fixes removed its final cursor failure. | +| Done | Full publish-and-persist path was validated after fixes. | Automatic GitHub Actions flow | Run `24245538002` succeeded. | +| Done | Steady-state loop after merging the generated KB PR was validated. | Automatic GitHub Actions flow | Run `24245767479` succeeded and did not open another KB PR. | +| Done | New demo PR `#3` was opened, reviewed with explicit plain-text comments, and merged. | Copilot | PR `#3` merged at `2026-04-10T21:58:37Z`. | +| Blocked | Fresh content-demo attempt after PR `#3` did **not** produce a KB PR or article. | Automatic GitHub Actions flow + Copilot diagnosis | Run `24266078977` succeeded mechanically, but classification logged `0 new / 18 failed`, generation logged `0 new`, and no automation PR was opened. | + +## Important current limitation + +The demo is **not yet a complete content demo**. + +What we proved so far is: + +- the workflow boots correctly in a consumer repo, +- merged PR events trigger the pipeline, +- the automation branch / rolling KB PR flow works, +- cursor persistence works, +- the workflow settles cleanly after a generated KB PR is merged. + +What we **did not** prove yet is: + +- a merged demo PR with review comments results in **at least one generated KB article** under `kb//`. + +The latest attempt with PR `#3` did **not** close that gap. It reproduced the same high-level failure mode in a cleaner demo setup: + +- PR `#3` was merged successfully, +- the merged-PR workflow run `24266078977` started automatically and finished `success`, +- no KB PR was opened, +- `main` still contains only `kb/.manifest.json` and `kb/INDEX.md`, +- the run logs showed `Extracted 2 PRs, 8 comments cached.`, `Classified 0 new, 0 cached, 0 need review, 18 failed.`, and `Generated 0 new, 0 skipped, 0 filtered, 0 failed.`. + +So the demo is now blocked not on setup or review-comment quality alone, but on the classifier/content path itself. + +For the PR `#1` demo data in workflow run `24245335864`, the preserved logs and debug artifact showed: + +- `Extracted 1 PRs, 10 comments cached.` +- nine `Could not parse classification JSON for comment ...` warnings during classification, +- `Classified 1 new, 0 cached, 0 need review, 9 failed.` +- `Generated 1 new, 0 skipped, 0 filtered, 0 failed.` + +However, the rolling KB PR `#2` that was ultimately merged still only contained: + +- `kb/.manifest.json` +- `kb/INDEX.md` + +So the workflow mechanics were validated, but the repo still did **not** finish with a published article on `main`, which is why the content demo remains incomplete. + +## Remaining work required for a COMPLETE demo + +### Mandatory gates before the next live demo attempt + +The next feature-PR demo attempt must **not** start until all of the following are true: + +1. **Done:** we have a short written diagnosis of the PR `#1` classification failure that names the concrete failure mode from logs/artifacts. +2. **Done:** we know which prior evidence is still inspectable and what fallback to use if deeper payload-level evidence is needed. +3. **Done:** we have a **comment-input contract** with realistic example review comments intentionally aligned to `gotcha`, `code_pattern`, and `domain_knowledge`. +4. **Done:** we have a **five-minute triage checklist** that distinguishes: + - extraction failure, + - classification failure, + - generation failure, + - dedup / manifest suppression, + - permissions / environment failure. +5. **Done:** we have a brief **article-fidelity checklist** for the human review step so "looks good" is not purely ad hoc. + +### PR `#1` classification diagnosis + +The concrete failure mode for the PR `#1` demo data was **malformed model output / parse mismatch at the classifier boundary**, not extraction failure and not an Anthropic API outage. + +Evidence from run `24245335864` and its uploaded `github-pr-kb-debug` artifact: + +- extraction succeeded and cached all 10 comments for `pr-1.json`, +- the classifier emitted nine log lines of the form `Could not parse classification JSON for comment `, +- the current classifier only increments `failed` for two paths: `anthropic.APIError` or `json.loads(...)` failure, and the logs showed the JSON-parse path rather than the API-error path, +- one plain-text issue comment (`4223530384`) classified successfully as `domain_knowledge` and produced a generated article in the failure artifact, +- the nine failed comments therefore represent responses that came back from the model but were not valid bare JSON for the current parser contract. + +Short diagnosis: **the dominant PR `#1` classification failure was non-parseable classifier output, especially across the longer/more formatted comment set, so the next demo should not assume better review comments alone will fix the problem.** + +What is still unknown from existing evidence is the exact raw malformed payload for each failed comment, because the workflow logs did not preserve the model response text at info level. If we need payload-level examples, the fastest fallback is a focused reproduction against the saved `pr-1.json` comment bodies with response capture enabled. + +### Artifact and fallback status + +The prior failure evidence is still inspectable enough to support the next step. + +What is still available right now: + +- the workflow job logs for run `24245335864` are still downloadable and still show the stage-level evidence, including the nine `Could not parse classification JSON` warnings, +- the uploaded debug artifact `github-pr-kb-debug` (artifact `6371844189`, not expired as of this update) is still downloadable, +- that artifact still contains the key preserved files we need for replay and inspection: `.github-pr-kb/cache/pr-1.json`, `.github-pr-kb/cache/classified-pr-1.json`, `.github-pr-kb/cache/classification-index.json`, and the generated `kb/` output snapshot. + +What is **not** preserved in the current artifact set: + +- the raw malformed model responses for the nine failed comments, +- enough logging detail to tell whether the malformed outputs were wrapped in prose, fenced code blocks, partial JSON, or some other near-miss shape. + +Fastest reliable fallback if we need deeper evidence: + +- replay classification locally or in a one-off debug run against the saved `pr-1.json` comments, +- capture the raw Claude response text before `json.loads(...)`, +- compare the successful plain-text comment path with one or two failed heavily formatted comments to isolate the output-shape mismatch quickly. + +### Comment-input contract for the next demo PR + +Use **plain human review comments** for the live demo. Each comment should be: + +- plain text only, with no HTML, screenshots, copied bot templates, or fenced code blocks, +- 1-3 sentences focused on one idea, +- tied to a concrete code behavior or business rule, +- explicit about **why** the point matters, not just what changed, +- written before merge as normal review feedback, not as a post-fix status update like "fixed in commit ...". + +Recommended example comments for the next demo run: + +1. **Gotcha** + `We should reject negative discount_amount values explicitly. Otherwise this helper turns a "discount" path into a surcharge path, which is easy to miss in tests because the math still looks valid at a glance.` + +2. **Code pattern** + `I would keep a small pattern here: validate discount inputs first, normalize the discounted subtotal once, then delegate to calculate_total for tax and rounding. That keeps every pricing path using the same calculation flow instead of duplicating rounding rules in multiple helpers.` + +3. **Domain knowledge** + `Please document that this helper applies an absolute discount before tax, not after tax and not as a percentage. That ordering is a pricing rule the business will care about, so it should be explicit in the code or docstring rather than implied by the implementation.` + +These examples are intentionally shaped to be: + +- realistic code-review comments a human could naturally leave on a small pricing helper PR, +- short enough to stay readable in GitHub, +- knowledge-bearing enough to publish if the classifier and generator path works correctly. + +### Five-minute triage checklist for the next run + +When the next merged-PR workflow runs, inspect the latest `update-kb` job in this order and stop as soon as one row matches the evidence: + +| Area | Check in the run | Positive signal | Failure signal | Next action | +| --- | --- | --- | --- | --- | +| 1. Permissions / environment | Look first at `Validate repository-variable auth configuration`, `Checkout github-pr-kb tool`, `Install tool dependencies`, `Create or reuse rolling KB pull request`, and `Persist KB_LAST_SUCCESSFUL_CURSOR`. | These steps are `success` or intentionally `skipped`. | Missing secrets, checkout failure, dependency install failure, `gh` auth failure, push/PR creation failure, or cursor-persist failure. | Treat as workflow/environment breakage before reasoning about content quality. | +| 2. Extraction | Read the `Extract PR comments` step summary and, if needed, the uploaded debug artifact's `.github-pr-kb/cache/pr-.json`. | Expected merged PR is present and the log shows nonzero comments cached. | Step fails, target PR is absent from cache, or comment count is unexpectedly zero. | Diagnose extractor input scope, repo/token access, or comment filtering before looking at classify/generate. | +| 3. Classification | Read the `Classify extracted comments` step summary and warnings. | Nonzero `new` or `cached`, with failures at zero or low enough to still leave publishable output. | `failed > 0`, repeated `Could not parse classification JSON`, Anthropic/API errors, or `0 classified` when extracted comments were clearly substantive. | Treat as classifier/content-path failure; inspect `classified-pr-.json`, `classification-index.json`, and raw-response fallback if needed. | +| 4. Generation | Read the `Generate knowledge base` step summary and inspect `kb/` output or the KB PR diff. | At least one article file appears under `kb//`, plus matching `kb/INDEX.md` and `kb/.manifest.json` updates. | Generate step fails, or classify succeeded but no article file is produced. | Treat as generation failure unless the next row proves it was intentional dedup suppression. | +| 5. Dedup / manifest suppression | Compare generate output with `.manifest.json`, staged diff, and whether `Stage generated KB output` reports changes. | Existing manifest mapping explains why a repeated comment produced no new article. | `Generated 0 new` / no staged changes even though the PR introduced truly new review comments and no manifest entry already maps them. | If the manifest does not already explain the skip, escalate back to generation or classification diagnosis. | +| 6. Publication outcome | Check whether `Stage generated KB output`, `Commit and push KB update branch`, and `Create or reuse rolling KB pull request` actually produce a KB PR with article files. | Open or updated KB PR contains one or more `kb//*.md` files. | Only `kb/.manifest.json` and `kb/INDEX.md`, or no KB PR despite expected content. | Do **not** claim demo success; capture the artifact and continue diagnosis from the earliest failed stage above. | + +Fast rule of thumb for this demo: + +- **Extraction failure:** wrong or missing PR/comment data. +- **Classification failure:** comments were extracted, but classifier warnings/errors prevent usable classifications. +- **Generation failure:** classifications exist, but article files are not created from them. +- **Dedup / manifest suppression:** the system is behaving as designed because the comment was already published. +- **Permissions / environment failure:** the workflow cannot read, write, authenticate, or persist state even before content logic is trustworthy. + +### Article-fidelity checklist for human review of the KB PR + +When the generated KB PR is open, review each article against this checklist before merging: + +| Check | What to confirm | Merge only if... | +| --- | --- | --- | +| 1. Factual fidelity | The article stays grounded in the PR title and source review comment. | It does **not** invent causes, decisions, implications, or fixes that are missing from the source. | +| 2. Category fit | The article category matches the original comment's intent. | A warning/pitfall lands in `gotcha`, a reusable implementation approach lands in `code_pattern`, and a business/project rule lands in `domain_knowledge` (or another category only if clearly justified). | +| 3. Useful summary | The `#` heading and article framing capture the actual insight. | The title is specific and useful to a future reader, not vague, misleading, or broader than the source comment. | +| 4. Required structure | The article body uses the expected section headings for its category and fills unsupported sections honestly. | Missing evidence is rendered as `Not stated in the source comment.` rather than hallucinated prose. | +| 5. Source handling | The article paraphrases rather than echoing or bloating the original review text. | It preserves the meaning without copying large spans of the source comment verbatim. | +| 6. Metadata integrity | Frontmatter and cross-file references match the source. | `pr_url`, `comment_url`, `author`, `category`, `comment_id`, `needs_review`, `kb/INDEX.md`, and `kb/.manifest.json` all point to the same article and source comment. | +| 7. Review flag visibility | Low-confidence content stays visibly reviewable. | If `needs_review: true`, the article and `kb/INDEX.md` make that visible rather than hiding it. | +| 8. Diff hunk relevance | Review-comment articles include code context only when it helps. | Any included fenced diff block is the source diff hunk for that comment and is relevant to the article's point. | + +Practical merge rule for this demo: + +- merge if the article is faithful, correctly categorized, structurally complete, and useful to a teammate who did not read the original PR, +- do **not** merge if the article hallucinates, miscategorizes the insight, or turns a narrow review comment into a broader claim than the source supports. + +### Prepared next demo PR + +The next small feature PR is now ready for review in `github-pr-kb-demo`. + +- PR: `#3` — `feat: add percentage discount helper for demo review` +- URL: `https://github.com/Galzi1/github-pr-kb-demo/pull/3` +- Branch: `demo/percentage-discount-helper` +- Change scope: one small change to `demo_app.py` adding `calculate_total_with_percentage_discount(...)` +- Why this is a good demo candidate: it is small, easy to review on one screen, and naturally invites the exact kinds of comments we want for the next step — bounds/validation (`gotcha`), reuse of calculation flow (`code_pattern`), and business-rule clarity about tax/stacking semantics (`domain_knowledge`). + +### Review comments posted on the demo PR + +PR `#3` now contains the intended human-written review comments aligned to the comment-input contract: + +- `gotcha`: `https://github.com/Galzi1/github-pr-kb-demo/pull/3#discussion_r3066907088` +- `code_pattern`: `https://github.com/Galzi1/github-pr-kb-demo/pull/3#discussion_r3066907115` +- `domain_knowledge`: `https://github.com/Galzi1/github-pr-kb-demo/pull/3#discussion_r3066907152` + +These are plain-text review comments on the changed lines in `demo_app.py`, and they are ready for the merge step that will trigger the workflow. + +### Current live status after merging PR `#3` + +- PR `#3` merged successfully. +- Automatic workflow run `24266078977` started from that merge and completed `success`. +- `Commit and push KB update branch` and `Create or reuse rolling KB pull request` were both skipped because the workflow staged no KB changes. +- There is currently **no open automation PR** in `github-pr-kb-demo`. +- The latest run therefore failed the content-demo goal even though the workflow itself stayed green. + +### PR `#3` run diagnosis + +The new demo attempt confirms that the next task is **not** another review/merge exercise. It is a classifier/content-path fix. + +Evidence from run `24266078977`: + +- extraction worked (`Extracted 2 PRs, 8 comments cached.`), +- the classifier emitted repeated `Could not parse classification JSON for comment ...` warnings, +- classification ended at `Classified 0 new, 0 cached, 0 need review, 18 failed.`, +- generation then had nothing publishable to work with (`Generated 0 new, 0 skipped, 0 filtered, 0 failed.`), +- no KB files changed, so no KB PR was opened. + +Short conclusion: **the next demo should not continue until the classifier/content path is fixed or hardened enough to turn these comments into parseable classifications.** + +### Current remediation status in `github-pr-kb` + +The classifier/content-path fix is now in progress in the tool repository itself: + +- the local `github-pr-kb` classifier has been hardened to recover JSON objects from bare JSON, fenced ```json``` output, and prose-wrapped output instead of only accepting a raw top-level JSON string, +- regression tests were added for fenced JSON and prose-wrapped JSON classifier responses, +- local validation passed (`ruff check src tests` and the full pytest suite). + +Important scope note: run `24266078977` still reflects the **pre-fix** shipped tool behavior because the demo workflow is pinned to the older `KB_TOOL_REF`. The next live demo attempt should use the hardened classifier build, not the previous pinned commit. + +### Exit criteria + +We can claim the **happy-path Phase 8 demo is complete** only after all of the following are true at the same time: + +1. A new merged feature PR in `github-pr-kb-demo` has review comments that carry knowledge worth publishing. +2. The automatic merged-PR workflow succeeds. +3. The resulting KB PR contains: + - at least one article file under `kb//`, + - an updated `kb/INDEX.md` linking to that article, + - an updated `kb/.manifest.json` mapping the comment id to the article path. +4. The article content is reviewed and accepted as faithful to the source PR comment. +5. The KB PR is merged. +6. The automatic post-KB-merge workflow succeeds and does not open another KB PR. +7. The demo repo ends with no open **automation** PRs and the generated article committed on `main`. + +This is a **bounded claim**: it proves the intended end-to-end demo path on a real example. By itself it does **not** prove broad repeatability across varied comment styles, repo states, or external API conditions. + +### Todo checklist + +| # | Todo | Owner | Why this is required | Completion signal | +| --- | --- | --- | --- | --- | +| 1 | **Done:** inspect the current classification failures from the PR `#1` demo data and identify the exact failure mode (for example: malformed model output, empty output, API issue, parse mismatch, cache interaction, or prompt mismatch). Treat this as a **hard gate**, not a nice-to-have. | Copilot | We need to know whether the next demo should focus on better review-comment input, classifier robustness, cache handling, or both. | We have a short written diagnosis with the concrete failure reason from logs/artifacts, or a focused reproduction that isolates it. | +| 2 | **Done:** confirm whether the failing classifier outputs and related artifacts are still inspectable. If not, decide the fastest reliable fallback for reproducing the failure signal. | Copilot | Aggregate counts alone are not enough for a confidence-grade next step. | We know exactly what evidence we have and what fallback we will use if logs are insufficient. | +| 3 | **Done:** prepare a **comment-input contract**: 2-3 example review comments that are realistic, clearly knowledge-bearing, and intentionally aligned to supported categories such as `gotcha`, `code_pattern`, or `domain_knowledge`. | Copilot | The next live demo should not depend on vague human intuition about what "good comments" look like. | We have a short set of example comments ready to use as the review standard. | +| 4 | **Done:** define a **five-minute triage checklist** for the next run that distinguishes extraction failure, classification failure, generation failure, dedup / manifest suppression, and permissions / environment failure. | Copilot | If the next run still produces no article, we need rapid stage-level diagnosis instead of another ambiguous post-mortem. | We have a compact checklist or decision tree for reading the next run. | +| 5 | **Done:** define a brief **article-fidelity checklist** for human review of the generated KB PR. | Copilot + you | The final merge decision should use a consistent standard rather than a vague "looks good." | We have a small rubric covering factual fidelity, category fit, and usefulness. | +| 6 | **Done:** prepare a **new small feature PR** in `github-pr-kb-demo` that is easy to review and likely to produce one or more high-signal review comments. | Copilot | We need a fresh PR because PR `#1` has already been merged and its comments produced no article. | A new demo PR exists and is ready for review. | +| 7 | **Done:** add **clear, explicit review comments** on that new PR using the comment-input contract. | You (manual) | This is the actual user input the product is supposed to learn from. For a confidence-grade demo, the review comments should be intentionally high-signal and human-readable in GitHub itself. | The PR contains at least 2-3 substantive review comments that match the agreed examples/rubric. | +| 8 | **Done:** merge the new feature PR after the review comments are in place. | You (manual) | The workflow triggers from a merged PR, so this is the start of the real automatic demo path. | PR `#3` merged into `main`. | +| 9 | **Done:** run the Phase 8 workflow automatically from that merge. | Automatic GitHub Actions flow | This is the core Phase 8 behavior under demo. | Run `24266078977` started automatically from the PR `#3` merge and completed successfully. | +| 10 | **Failed / diagnosed:** verify that the automatic workflow produces a KB PR containing at least one real article file under `kb//`. If it does not, stop and diagnose instead of pretending the demo is complete. | Copilot | This is the missing proof from the current demo. Without an actual article, we only proved plumbing, not the content path. | Run `24266078977` produced no KB PR, no article files, and a classification-failure-only result (`18 failed`). | +| 11 | Review the generated article(s) against the article-fidelity checklist. | You (manual) | A complete demo needs human confirmation that the published knowledge is actually correct and useful, not just mechanically generated. | You are satisfied that the article reflects the source review comment accurately enough to merge. | +| 12 | Merge the generated KB PR. | You (manual) | This proves the publication loop closes successfully on real article content. | The KB PR is merged into `main`. | +| 13 | Let the post-KB-merge workflow run automatically and verify that it succeeds without opening another KB PR. | Automatic GitHub Actions flow | This proves the system reaches a stable steady state after publication. | Latest workflow run succeeds and no new automation PR is opened. | +| 14 | Perform the final repo-state verification: article exists on `main`, `kb/INDEX.md` links to it, `.manifest.json` maps it, latest workflow is green, and there are no open automation PRs created by this workflow. | Copilot | This is the final proof that the demo finished cleanly and left the repo in the expected end state. | All exit criteria above are satisfied simultaneously. | + +## Non-negotiable gate + +If the next merged demo PR again produces: + +- no article files, +- only `kb/.manifest.json` and `kb/INDEX.md`, or +- another classification-failure-only run, + +then the demo is **not complete** and we must not claim the happy-path demo is done. In that case, the next task is to diagnose and fix the classifier/content path before continuing. + +## Expected final repo state when the demo is truly complete + +`github-pr-kb-demo` should end with all of the following: + +- the feature PR merged, +- the generated KB PR merged, +- at least one article file under `kb//`, +- `kb/INDEX.md` linking to the article, +- `kb/.manifest.json` containing the article mapping, +- no open automation PRs created by this workflow, +- latest relevant workflow runs green, +- no follow-up automation churn after the KB PR merge. + +Only then should this demo be considered fully complete as a **happy-path end-to-end demonstration**. diff --git a/src/github_pr_kb/classifier.py b/src/github_pr_kb/classifier.py index 0d3ff94..861ce2d 100644 --- a/src/github_pr_kb/classifier.py +++ b/src/github_pr_kb/classifier.py @@ -4,6 +4,7 @@ import json import logging import os +import re import tempfile from datetime import datetime, timezone from pathlib import Path @@ -35,6 +36,8 @@ "other", } +_JSON_FENCE_RE = re.compile(r"```(?:json)?\s*(.*?)\s*```", re.IGNORECASE | re.DOTALL) + SYSTEM_PROMPT = """You are a technical knowledge classifier for GitHub PR comments. Classify the given comment into exactly one of these 5 categories: @@ -72,6 +75,62 @@ def _write_atomic(path: Path, data: str) -> None: raise +def _parse_classification_response(text: str) -> dict | None: + """Extract a JSON object from bare, fenced, or prose-wrapped model output.""" + stripped = text.strip() + if not stripped: + return None + + decoder = json.JSONDecoder() + candidates: list[str] = [stripped] + candidates.extend(match.group(1).strip() for match in _JSON_FENCE_RE.finditer(stripped)) + + seen: set[str] = set() + for candidate in candidates: + if not candidate or candidate in seen: + continue + seen.add(candidate) + try: + parsed = json.loads(candidate) + except json.JSONDecodeError: + continue + if _is_valid_classification_payload(parsed): + return parsed + + for idx, char in enumerate(stripped): + if char != "{": + continue + try: + parsed, _ = decoder.raw_decode(stripped[idx:]) + except json.JSONDecodeError: + continue + if _is_valid_classification_payload(parsed): + return parsed + + return None + + +def _is_valid_classification_payload(parsed: object) -> bool: + """Return True only for dicts that satisfy the classifier response contract.""" + if not isinstance(parsed, dict): + return False + + category = parsed.get("category") + summary = parsed.get("summary") + if not isinstance(category, str) or not isinstance(summary, str): + return False + + if "confidence" not in parsed: + return False + + try: + float(parsed["confidence"]) + except (TypeError, ValueError): + return False + + return True + + class PRClassifier: """Reads cached PR comment files, classifies via Claude API, deduplicates by SHA-256 hash.""" @@ -210,9 +269,8 @@ def _classify_comment(self, comment: CommentRecord) -> ClassifiedComment | None: return None text = response.content[0].text - try: - result: dict = json.loads(text) - except json.JSONDecodeError: + result = _parse_classification_response(text) + if result is None: logger.warning( "Could not parse classification JSON for comment %d", comment.comment_id ) diff --git a/tests/test_classifier.py b/tests/test_classifier.py index 1afc5f5..58b26d1 100644 --- a/tests/test_classifier.py +++ b/tests/test_classifier.py @@ -226,6 +226,96 @@ def test_parse_failure_returns_none(cache_dir_with_pr): assert body_hash(comment.body) not in classifier._index +def test_markdown_fenced_json_is_parsed(cache_dir_with_pr): + from github_pr_kb.classifier import PRClassifier + + response_text = """```json + {"category": "gotcha", "confidence": 0.88, "summary": "Validate discount rate bounds."} + ```""" + mock_message = make_mock_message(response_text) + + with patch("github_pr_kb.classifier.Anthropic") as MockAnthropic: + mock_client = MagicMock() + mock_client.messages.create.return_value = mock_message + MockAnthropic.return_value = mock_client + + classifier = PRClassifier(cache_dir=cache_dir_with_pr, api_key="sk-ant-fake") + result = classifier.classify_pr(1) + + assert len(result.classifications) == 1 + assert result.classifications[0].category == "gotcha" + assert classifier._failed_count == 0 + + +def test_prose_wrapped_json_is_parsed(cache_dir_with_pr): + from github_pr_kb.classifier import PRClassifier + + response_text = ( + 'Here is the classification result:\n' + '{"category": "domain_knowledge", "confidence": 0.91, ' + '"summary": "Discount order is a business rule."}\n' + "Thanks." + ) + mock_message = make_mock_message(response_text) + + with patch("github_pr_kb.classifier.Anthropic") as MockAnthropic: + mock_client = MagicMock() + mock_client.messages.create.return_value = mock_message + MockAnthropic.return_value = mock_client + + classifier = PRClassifier(cache_dir=cache_dir_with_pr, api_key="sk-ant-fake") + result = classifier.classify_pr(1) + + assert len(result.classifications) == 1 + assert result.classifications[0].category == "domain_knowledge" + assert classifier._failed_count == 0 + + +def test_unrelated_json_dict_is_skipped_before_valid_classification(cache_dir_with_pr): + from github_pr_kb.classifier import PRClassifier + + response_text = ( + 'Noise: {"status": "ok"}\n' + '{"category": "code_pattern", "confidence": 0.83, ' + '"summary": "Reuse one validation flow for pricing helpers."}' + ) + mock_message = make_mock_message(response_text) + + with patch("github_pr_kb.classifier.Anthropic") as MockAnthropic: + mock_client = MagicMock() + mock_client.messages.create.return_value = mock_message + MockAnthropic.return_value = mock_client + + classifier = PRClassifier(cache_dir=cache_dir_with_pr, api_key="sk-ant-fake") + result = classifier.classify_pr(1) + + assert len(result.classifications) == 1 + assert result.classifications[0].category == "code_pattern" + assert classifier._failed_count == 0 + + +def test_schema_less_json_object_is_rejected(cache_dir_with_pr): + from github_pr_kb.classifier import PRClassifier, body_hash + + mock_message = make_mock_message("{}") + + with patch("github_pr_kb.classifier.Anthropic") as MockAnthropic: + mock_client = MagicMock() + mock_client.messages.create.return_value = mock_message + MockAnthropic.return_value = mock_client + + classifier = PRClassifier(cache_dir=cache_dir_with_pr, api_key="sk-ant-fake") + comment = PRFile.model_validate_json( + (cache_dir_with_pr / "pr-1.json").read_text(encoding="utf-8") + ).comments[0] + + result = classifier._classify_comment(comment) + + assert result is None + assert classifier._failed_count == 1 + assert body_hash(comment.body) not in classifier._index + + def test_load_index_filters_failed(tmp_path): from github_pr_kb.classifier import PRClassifier