Skip to content

Commit a3c9d1f

Browse files
mauricecarrier7tclaude
authored
feat(reporting): world-class PR report contract + decision-first automated comments (#1324)
* fix(triage): exonerate the reducer for the stuck-Send + clear input on Start-over (PP-4846) PP-4846 (repro-first): after Discard → pick a different category → type, the Send arrow intermittently stayed disabled. It did not reproduce deterministically. Pinned the REDUCER side of that flow — the source of truth the Send button's `disabled` predicate reads (`!isFollowUpStep && inputText.trimmed.isEmpty`). `StuckSendReproTests` drives Discard → re-category → type and proves inputText is set and the step is a composition step, i.e. Send is enabled at the reducer level. So the intermittent field report is localized to the SwiftUI TextField binding (consistent with the "UIKit gesture-timeout" log + the non-determinism), NOT the state machine. Writing that test surfaced a real, separate latent bug: `.userTappedStartOver` returned to `.awaitingCategory` but did NOT clear `inputText`, so a half-typed composer survived "start fresh" — unlike Discard, which lands empty. In practice Start-over is reached with inputText already empty (post-submit), but the two reset paths should have identical composer semantics. Now both clear it. Verified: TriageBotCore reducer suites green (StuckSendReproTests + CategoryChipDebounce + StructuredEscalation, 13/13). **Scope:** the Start-over reset clears inputText; reducer-level tests for both reset paths. No SwiftUI change. **Not done:** the intermittent SwiftUI TextField(axis:.vertical) binding re-sync itself — reducer is exonerated; recommended fix is the local-@State mirror pattern already used by TicketPreviewCard, but it can't be verified without a deterministic repro, so it is not shipped blind here. **Deferred:** a simdrive repro harness for the SwiftUI race (needs a dedicated run; the field bug is intermittent). * feat(reporting): world-class PR report contract + decision-first automated comments Root cause: The PR record had an inverted signal budget. The authored layer (bodies + commit messages) was world-class, but the automated layer buried it — a ~900-row all-green test table dwarfed the human body, the delta engine that computes what-changed was silent, the ledger comment printed constant numbers on every PR, and a disabled QAAtlas integration shipped a standing "set OPENAI_API_KEY" banner. For an agent-authored / human-orchestrated codebase, the load-bearing claims were also author-attested prose with no machine anchor. Solution: Establish one PR Report Contract that every reporting surface serves, and make the automated surfaces obey it. - docs/architecture/pr-report-contract.md — the cohesion anchor: 4 goals (orchestrator decision speed, grounded verifiability, durable forensic reconstruction, fleet legibility) + 4 principles (decision-first, delta-or-silent, grounded-verifiability, fleet-legible) + surface map + the standing gap-question list. - .github/PULL_REQUEST_TEMPLATE.md — the authored-layer contract (there was no template): adds Evidence / Repro / Class / Obligations to Root-cause/Solution. - docs/regressions/recurrence-classes.md — greppable Class taxonomy, seeded from the classes agents already name in prose. - unit-testing.yml — decision-first: full per-class matrix ALWAYS collapsed (never dumped open); failures OPEN; deltas relabelled "what changed vs base" with new-failures open; Skipped column + reconciliation icon so all-skipped classes stop rendering as false-green 7|0|0; provenance footer. - parse-xcresult.py — emit per-class `skipped` so the comment can reconcile rows. - ledger.yml — delta-or-silent: drop the dead QAAtlas banner (qa domain is off), name the layer violation instead of showing a bare count, and stop printing a green "no dead code" over 0 analyzed files. Evidence: py_compile (3 scripts) OK; YAML parse OK; actionlint clean on edited regions (only pre-existing info-level shellcheck notes on untouched run: steps); node --check on both github-script bodies OK; render-logic smoke test green for 6 cases incl. all-skipped→green, non-reconciling→warn, legacy 5-field tolerance. The PR's own CI run is the machine anchor, dogfooding the contract. **Scope:** dev-tooling + docs only. No Palace/ production code, no test-affecting change. Comment-generator JS validated structurally (node --check) + render smoke test; the live rendering is proven by this PR's own CI comments. **Not done:** the .test-history develop cache-warm job (so deltas populate on PR branches); freeze-red-before-overwrite in the sticky comment; a package-suite (TriageBotCore) CI job to reproduce swift test counts; the cross-PR roll-up; the ~2 unmapped-status tests in parse-xcresult (the off-by-2) — the new ⚠️ row now flags them but the parser normalization is not fixed here. All tracked as Obligations on the PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(reporting): adopt ledger 0.9.7 (real monolith blast-radius) + ship componentRoots Root cause: the Ledger engine treated the single Palace app target as one file-less component, so app files were owned by nothing and blast-radius on widely-imported packages (e.g. PalaceAuth on the sign-in path) falsely read "nobody depends / Risk LOW" — the reason the ledger comment carried no trustworthy signal on PRs. Solution: adopt CodeAtlas Ledger 0.9.7 (SyncTek-LLC/ledger-dist), which indexes Xcode app-target file membership, turns config componentRoots into real file-owning components, and adds conservative symbol-reference edges. Ship the 32 validated componentRoots so the model decomposes Palace into its real modules (Network, MyBooks, SignInLogic, …). Validated pre-merge: impact AuthCoordinator → 8 real dependents at CRITICAL; TPPNetworkExecutor resolves to Network; context --focus MyBooks returns real files/symbols/deps. **Scope:** ledger.yml version pin 0.9.6 -> 0.9.7 + tools/ledger/ledger-config.json componentRoots. The install URL is unchanged (it redirects to SyncTek-LLC). **Not done:** ledger job stays continue-on-error (advisory) for this dogfood cycle before any consideration of blocking; the tool's own swift test has 12 pre-existing (non-regression) failures to clear first. accesslint artifact for 0.9.7 not added (matches 0.9.6; a11y behavior unchanged). impact display polish (adjacent-folder/dup rows) deferred. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: t <t@t.io> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 6934c13 commit a3c9d1f

8 files changed

Lines changed: 344 additions & 44 deletions

File tree

.github/COMMIT_AND_PR_FOR_JIRA.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,11 @@
11
# Commit and PR format for JIRA
22

3+
> This is the **authored layer** of the [PR Report Contract](../docs/architecture/pr-report-contract.md).
4+
> The contract explains *why* the format exists (durable forensic record + fleet telemetry for an
5+
> agent-authored, human-orchestrated codebase); this file is the concrete commit/PR format. The PR
6+
> **body** is now driven by [`.github/PULL_REQUEST_TEMPLATE.md`](./PULL_REQUEST_TEMPLATE.md), which adds
7+
> `Evidence` / `Repro` / `Class` / `Obligations` to the Root-cause/Solution/How-to-verify below.
8+
39
When commits or PRs are linked to JIRA, the default post often shows only hash, author, date, one-line message, and file list. To make JIRA posts **useful** (root cause, what changed, how to verify), use the formats below.
410

511
---

.github/PULL_REQUEST_TEMPLATE.md

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
<!--
2+
Palace iOS PR body — the durable record + fleet telemetry surface.
3+
Contract: docs/architecture/pr-report-contract.md Format: .github/COMMIT_AND_PR_FOR_JIRA.md
4+
Fill every section. Delete a section ONLY if it genuinely does not apply, and say why.
5+
The goal is that six months from now, git + this page alone reconstruct the whole change.
6+
-->
7+
8+
## Summary
9+
<!-- One or two sentences. What changes, in patron-visible terms where possible. -->
10+
11+
## Root cause
12+
<!-- WHY this change is needed: the mechanism of the bug, or the gap being filled.
13+
Describe behavior and the call path — not a file list. -->
14+
15+
## Solution
16+
<!-- WHAT this PR does: the behavior change, and the key technical decision. -->
17+
18+
## Evidence
19+
<!-- Grounded-verifiability (contract §3). Every weight-bearing claim → where CI or an
20+
artifact reproduces it. A number with no pointer is a hypothesis, not a verification.
21+
Mark each as CI-reproduced or self-attested — do not hide the difference. -->
22+
- <claim, e.g. "PalaceTests full suite green"> → <CI job / artifact / exact command + exit status>
23+
- <claim, e.g. "TriageBotCore swift test 258/258"> → <CI job, or "self-attested — not yet CI-reproduced (Obligation below)">
24+
25+
## Repro
26+
<!-- Durable reconstruction (contract §3). How the scenario was reproduced, so a future
27+
reader can re-run it. Link the simdrive replay / fixture in-repo, not a dead run URL. -->
28+
- Scenario: <one sentence, patron-visible> Jira: PP-XXXX
29+
- Replay / fixture: <.simdrive path or in-repo test> | none — <why>
30+
31+
## Class
32+
<!-- Fleet-legibility (contract §4). Is this a recurrence of a known failure class?
33+
Use an id from docs/regressions/recurrence-classes.md, or declare a NEW class there. -->
34+
- Recurrence-of: <registry-id, e.g. unbounded-live-dependency | debounce-missing | none — new class: <slug> (added to registry)>
35+
36+
## Obligations
37+
<!-- Anything promised-but-not-done becomes a tracked checkbox with an owner, so a
38+
roll-up can audit un-discharged promises. "will verify before merge" lives here. -->
39+
- [ ] <deferred / promised follow-up> — owner: <@handle or workstream>
40+
41+
## Scope
42+
<!-- What this PR touches, and what it deliberately does not. -->
43+
44+
## Not done / Deferred
45+
<!-- Hook-enforced for changes ≥50 prod LOC. Honest boundaries of the change. -->
46+
47+
---
48+
🤖 Generated with [Claude Code](https://claude.com/claude-code)

.github/workflows/ledger.yml

Lines changed: 42 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -59,9 +59,11 @@ jobs:
5959
6060
- name: Install ledger
6161
run: |
62-
# v0.9.6: Ledger 0.9.6 + QAAtlas 2.3.0
62+
# v0.9.7: monolith app-target indexing (real blast-radius on the app
63+
# target; sign-in path no longer false-LOW). See CodeAtlas
64+
# fix/monolith-target-indexing. Install URL redirects to SyncTek-LLC.
6365
curl -fsSL https://raw.githubusercontent.com/mauricecarrier7/ledger-dist/main/install.sh \
64-
| bash -s -- --version 0.9.6 --dir ./tools/bin
66+
| bash -s -- --version 0.9.7 --dir ./tools/bin
6567
6668
# Clear macOS quarantine attributes (fixes exit code 11)
6769
xattr -cr ./tools/bin/ 2>/dev/null || true
@@ -409,16 +411,16 @@ jobs:
409411
// ═══════════════════════════════════════════════════════════════
410412
// TEST COVERAGE SECTION (QAAtlas) - only if qa domain enabled
411413
// ═══════════════════════════════════════════════════════════════
412-
body += '---\n';
413-
body += '### 🧪 Test Coverage (via QAAtlas)\n\n';
414-
415-
// Check if we have valid QA data
414+
// TEST COVERAGE (QAAtlas): the `qa` domain is disabled
415+
// (DOMAINS="arch,reach,a11y" at the top of this file), so this
416+
// section stays SILENT rather than posting a standing "set
417+
// OPENAI_API_KEY" banner on every PR — a constant banner is noise
418+
// that trains readers to skim past warnings
419+
// (pr-report-contract.md §2, delta-or-silent).
416420
const hasQAData = qaCoverage && parseFloat(qaCoverage) > 0 || (parseInt(qaTested) > 0 || parseInt(qaTotal) > 0);
417-
418-
if (!hasQAData) {
419-
body += '> ⚠️ **No coverage data available** - QAAtlas requires an OpenAI API key.\n';
420-
body += '> Set `OPENAI_API_KEY` in repository secrets to enable AI-powered test coverage analysis.\n\n';
421-
} else {
421+
if (hasQAData) {
422+
body += '---\n';
423+
body += '### 🧪 Test Coverage (via QAAtlas)\n\n';
422424
const coverageNum = parseFloat(qaCoverage);
423425
const coverageIcon = coverageNum >= 50 ? '🟢' : coverageNum >= 30 ? '🟡' : '🔴';
424426
body += `**Coverage Estimate:** ${coverageIcon} **${qaCoverage}%**\n\n`;
@@ -519,15 +521,31 @@ jobs:
519521
body += `| Layer Violations | ${violations} | Dependencies that break architectural boundaries |\n`;
520522
body += `| Hotspots | ${hotspots} | Files with high complexity + frequent changes |\n`;
521523
body += `| Avg Coupling | ${avgCoupling} | How interconnected modules are (lower is better, <1.0 is good) |\n\n`;
522-
524+
525+
// Honesty note: these are repo-BASELINE metrics, not diff deltas —
526+
// they read identically on most PRs. Don't misread a constant as
527+
// PR-caused. A true base-vs-head delta is a tracked Obligation
528+
// (pr-report-contract.md §2).
529+
if (changedFiles === '0') {
530+
body += `> ℹ️ This diff changed **0** architecture-relevant files — the numbers above are the repo baseline, unchanged by this PR.\n\n`;
531+
} else {
532+
body += `> ℹ️ Baseline metrics (whole-repo), not a diff delta. Watch for a value that *moves* vs. \`develop\`, not its absolute level.\n\n`;
533+
}
534+
523535
if (parseInt(archIssues) === 0 && cycles === '0' && violations === '0') {
524536
body += '✅ **Architecture looks healthy!**\n\n';
525537
} else {
526538
// Show issues summary
527539
if (parseInt(archIssues) > 0) {
528540
body += `⚠️ Found **${archIssues}** architectural issue(s).\n\n`;
529541
}
530-
542+
// A layer violation with no issue count still gets NAMED, not left
543+
// as a bare number the reader can't act on (pr-report-contract.md
544+
// gap #8). Surfaces the same findings file below.
545+
if (parseInt(violations) > 0) {
546+
body += `⚠️ **${violations}** layer violation(s) — see **Architecture Findings** below for the offending dependency edge(s).\n\n`;
547+
}
548+
531549
// Expandable: Dependency Cycles
532550
if (parseInt(cycles) > 0) {
533551
body += '<details>\n<summary><strong>🔄 Dependency Cycles</strong></summary>\n\n';
@@ -544,8 +562,9 @@ jobs:
544562
body += '\n</details>\n\n';
545563
}
546564
547-
// Expandable: All Findings
548-
if (parseInt(archIssues) > 0) {
565+
// Expandable: All Findings — open when a violation exists so the
566+
// offending edge is named, not just counted (gap #8).
567+
if (parseInt(archIssues) > 0 || parseInt(violations) > 0) {
549568
body += '<details>\n<summary><strong>📋 Architecture Findings</strong></summary>\n\n';
550569
try {
551570
const findings = fs.readFileSync('/tmp/arch_findings.md', 'utf8');
@@ -569,7 +588,14 @@ jobs:
569588
body += '> 💡 *Detects code that cannot be reached from entry points (dead code).*\n\n';
570589
571590
if (parseInt(reachIssues) === 0) {
572-
body += '✅ **No dead code detected**\n\n';
591+
// Don't present a vacuous truth as a green check: "no dead code"
592+
// over ZERO analyzed files is meaningless, and a fake ✅ trains
593+
// readers to trust the real ones less (pr-report-contract.md §2).
594+
if (changedFiles === '0') {
595+
body += 'ℹ️ No architecture-relevant files in this diff — reachability not evaluated.\n\n';
596+
} else {
597+
body += '✅ **No dead code detected**\n\n';
598+
}
573599
} else {
574600
body += `Found **${reachIssues}** potentially unreachable symbol(s).\n`;
575601
body += `> These functions/classes may be unused and could be candidates for removal.\n\n`;

.github/workflows/unit-testing.yml

Lines changed: 57 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -762,25 +762,42 @@ jobs:
762762
}
763763
body += `**${tests} tests** | **${passed} passed** | **${failed} failed** | **${skipped} skipped** | ⏱️ ${duration} | 📊 ${passRate}${coverageDisplay}\n\n`;
764764
765-
// Class breakdown table — collapsed by default. Per-class
766-
// rows are valuable when triaging a failure but noise on a
767-
// green PR. Reviewers click through to expand.
765+
// Full per-class matrix — ALWAYS collapsed, never dumped open
766+
// (pr-report-contract.md §1, decision-first). ~900 green rows are
767+
// noise a reviewer must not scroll past; failures are surfaced
768+
// separately below. Skipped column added so an all-skipped class
769+
// reconciles instead of rendering as a false-green `7|0|0`;
770+
// ⚠️ flags any row where passed+failed+skipped != total.
768771
if (classSummary && classSummary.trim()) {
769772
const classLines = classSummary.trim().split('\n').filter(l => l.split('|')[0]);
770-
const failedClasses = classLines.filter(l => l.split('|')[3] !== '0').length;
773+
const failedClasses = classLines.filter(l => (l.split('|')[3] || '0') !== '0').length;
771774
const summaryLabel = failedClasses > 0
772-
? `<strong>Tests by Class</strong> — ${classLines.length} classes, <strong>${failedClasses} with failures</strong>`
773-
: `<strong>Tests by Class</strong> — ${classLines.length} classes (click to expand)`;
774-
body += `<details${failedClasses > 0 ? ' open' : ''}>\n<summary>${summaryLabel}</summary>\n\n`;
775-
body += `| Class | Tests | Passed | Failed | Duration |\n`;
776-
body += `|-------|-------|--------|--------|----------|\n`;
775+
? `<strong>All ${classLines.length} classes</strong>${failedClasses} with failures (full matrix)`
776+
: `<strong>All ${classLines.length} classes</strong> (full matrix — click to expand)`;
777+
body += `<details>\n<summary>${summaryLabel}</summary>\n\n`;
778+
body += `| Class | Tests | Passed | Failed | Skipped | Duration |\n`;
779+
body += `|-------|-------|--------|--------|---------|----------|\n`;
777780
classLines.forEach(line => {
778-
const [cls, total, pass, fail, dur] = line.split('|');
779-
const icon = fail === '0' ? '✅' : '❌';
781+
// 6-field format (cls|total|pass|fail|skip|dur); tolerate the
782+
// legacy 5-field (cls|total|pass|fail|dur) during rollout.
783+
const parts = line.split('|');
784+
const cls = parts[0];
785+
const total = parts[1] || '0';
786+
const pass = parts[2] || '0';
787+
const fail = parts[3] || '0';
788+
const hasSkip = parts.length >= 6;
789+
const skip = hasSkip ? (parts[4] || '0') : '0';
790+
const dur = hasSkip ? parts[5] : (parts[4] || '-');
791+
const reconciles = (parseInt(pass) + parseInt(fail) + parseInt(skip)) === parseInt(total);
792+
const icon = parseInt(fail) > 0 ? '❌' : (reconciles ? '✅' : '⚠️');
780793
const failCell = fail === '0' ? fail : `**${fail}**`;
781-
body += `| ${icon} ${cls} | ${total} | ${pass} | ${failCell} | ${dur} |\n`;
794+
body += `| ${icon} ${cls} | ${total} | ${pass} | ${failCell} | ${skip} | ${dur} |\n`;
782795
});
783-
body += `\n</details>\n\n`;
796+
body += `\n`;
797+
if (reportUrl) {
798+
body += `📊 Full interactive matrix: **[report](${reportUrl})**\n`;
799+
}
800+
body += `</details>\n\n`;
784801
}
785802
} else {
786803
if (testOutcome === 'success') {
@@ -795,10 +812,16 @@ jobs:
795812
}
796813
}
797814
798-
// Failed tests details
815+
// Failing detail — OPEN when red so the reviewer lands on the
816+
// actionable rows, not the collapsed matrix (contract §1).
799817
if (failedTests && failedTests.trim()) {
800-
body += `<details>\n<summary><strong>Failed Tests (click to expand)</strong></summary>\n\n`;
818+
body += `<details open>\n<summary><strong>Failed tests</strong></summary>\n\n`;
801819
body += '```\n' + failedTests.trim() + '\n```\n';
820+
// Names only — parse-xcresult emits identifiers, not messages.
821+
// Inlining the failure message + first stack frame is a tracked
822+
// Obligation (pr-report-contract.md gap #3); until then, the
823+
// interactive report carries the detail.
824+
body += `\n_Names only — open the interactive report above for messages + stack frames._\n`;
802825
body += `</details>\n\n`;
803826
}
804827
@@ -833,28 +856,33 @@ jobs:
833856
body += `not that we shipped less UI. Total coverage is kept for continuity. `;
834857
body += `The excluded paths are covered by simdrive E2E journeys (see \`chaos-replay-on-pr.yml\`).\n\n`;
835858
836-
// Trends
837-
if (newFailures || fixedTests || flakyTests || testCountChange) {
838-
body += `### 📈 Trends\n\n`;
839-
859+
// What changed vs. base — the orchestrator's first question
860+
// (contract §1). Delta-or-silent: this whole section renders only
861+
// when the .test-history cache hits (populating deltas). When it's
862+
// empty the section is silent by design, not a green claim — the
863+
// develop cache-warm job that makes deltas reliable is a tracked
864+
// Obligation (pr-report-contract.md gap #4).
865+
if ((newFailures && newFailures.trim()) || (fixedTests && fixedTests.trim()) || (flakyTests && flakyTests.trim()) || testCountChange) {
866+
body += `### 📈 What changed vs. base\n\n`;
867+
840868
if (testCountChange) {
841-
body += `Test count change: **${testCountChange}**\n\n`;
869+
body += `Test count: **${testCountChange}**\n\n`;
842870
}
843-
871+
844872
if (newFailures && newFailures.trim()) {
845-
body += `<details>\n<summary>⚠️ <strong>New Failures</strong></summary>\n\n`;
873+
body += `<details open>\n<summary>⚠️ <strong>New failures</strong> — passing on base, failing on this branch</summary>\n\n`;
846874
body += '```\n' + newFailures.trim() + '\n```\n';
847875
body += `</details>\n\n`;
848876
}
849-
877+
850878
if (fixedTests && fixedTests.trim()) {
851-
body += `<details>\n<summary>✅ <strong>Fixed Tests</strong></summary>\n\n`;
879+
body += `<details>\n<summary>✅ <strong>Fixed</strong> — failing on base, passing on this branch</summary>\n\n`;
852880
body += '```\n' + fixedTests.trim() + '\n```\n';
853881
body += `</details>\n\n`;
854882
}
855-
883+
856884
if (flakyTests && flakyTests.trim()) {
857-
body += `<details>\n<summary>⚡ <strong>Flaky Tests Detected</strong></summary>\n\n`;
885+
body += `<details>\n<summary>⚡ <strong>Flaky</strong> — inconsistent across recent runs</summary>\n\n`;
858886
body += '```\n' + flakyTests.trim() + '\n```\n';
859887
body += `</details>\n\n`;
860888
}
@@ -866,12 +894,14 @@ jobs:
866894
body += `Download the **snapshot-failures** artifact to view visual differences.\n\n`;
867895
}
868896
869-
// Links section
897+
// Links + provenance. The counts above are produced by THIS CI run
898+
// parsing the xcresult — not author-attested (contract §2,§3).
870899
body += `---\n`;
871900
if (reportUrl) {
872901
body += `🔗 **[Interactive HTML Report](${reportUrl})** | `;
873902
}
874903
body += `**[CI Run Details](${runUrl})**\n\n`;
904+
body += `_Counts above were produced by this CI run's xcresult parse — reproduce via the run link._\n\n`;
875905
876906
// Artifacts table
877907
body += `<details>\n<summary>📦 <strong>Downloadable Artifacts</strong></summary>\n\n`;

0 commit comments

Comments
 (0)