docs(triage-bot): as-built reference + shared architecture proposal (PP-4858) - #1355
Draft
mauricecarrier7 wants to merge 6 commits into
Draft
docs(triage-bot): as-built reference + shared architecture proposal (PP-4858)#1355mauricecarrier7 wants to merge 6 commits into
mauricecarrier7 wants to merge 6 commits into
Conversation
…PP-4858) Two documents for the PP-4858 spike. triage-bot-v1-as-built.md describes the bot as it works today, written as the behavioral contract another platform reproduces: the classifier's normalization, scoring and guards, the 15 redaction patterns with their order and replacement tokens, the corpus schema, and the escalation state machine. Platform-specific detail is labeled so nobody ports it by mistake. triage-bot-shared-architecture-proposal.md designs the server and Android halves. It opens with a decision memo naming what is being asked for, what is deliberately not being approved yet, and the risks that still need an accepter. Also corrects the package README, which claimed 26 tests and named the clipboard as the ticket transport, and adds a pointer from CLAUDE.md so anyone changing the package reads the contract first. **Scope:** documentation only, plus a README correction and a CLAUDE.md pointer. No production code in this commit. **Not done:** the docs describe several gaps rather than fixing them, most notably that the clipboard transport does not honor the patron's field omissions. Those are tracked under PP-4882. **Deferred:** cross-platform conformance fixtures are specified but not built; they are a Phase 2 deliverable and the Android estimate depends on them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
When the bot recognised a problem already fixed in a later release, it offered the patron a "Notify me" option. Tapping it produced a friendly confirmation and did nothing else: no notification was ever scheduled and no part of the app could have delivered one. It was a promise we had no way to keep, so the affordance is hidden rather than shown. The rule lives in a pure KBMatchActionPolicy in Core, following the existing HelpEntryPointPolicy pattern, so the decision is testable without a SwiftUI host. The reducer's handler stays behind it with its existing coverage, which makes restoring the option a one-line change once real delivery exists. The test pins the case that used to render the chip, so a regression that restores the old entry.fixedInVersion != nil rule fails rather than shipping a false promise. Mutation-verified: flipping the policy to return entryHasFixVersion fails the suite. **Scope:** one new pure policy type, its test, and the call site in the match card. No behavior change to the reducer or the classifier. **Not done:** actual notification delivery. Deciding what "notify me" should mean, local reminder, push, or dropping it entirely, is a product call tracked by PP-4886. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
scripts/triage-corpus-check.sh describes itself as a release-time credential-leak gate, and RedactionCorpusTests already runs inside the package suite. The script itself had no caller: not verify-pr.sh, not release-gates.yml, not any workflow, despite three files describing it as running at release time. The part that was therefore never running is --self-test, which disables redaction to prove the guard goes red on an un-redacted corpus. That is the check that catches a guard which has silently stopped guarding, and the suite alone cannot catch it. Wired into both places an engineer actually runs: the CI job, immediately after the package tests so build products are warm, and verify-pr.sh as a triage_redaction_gate check. Costs seconds. Dry-run clean on the current tree: guard passes, and the self-test correctly goes red when redaction is disabled. **Scope:** two call sites. The gate script and the tests it runs are unchanged. **Not done:** release-gates.yml still does not run it. That workflow is ubuntu-only and the gate needs a Swift toolchain, so adding it there means either a macOS runner or a container, which is a bigger change than this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The proposal carried a named escalation about unredacted logs in the shipped Android app, addressed to the android-core maintainer, with the affected fields and log level enumerated. Raising it was right; the venue was not. A shared architecture proposal that circulates to leadership is the wrong place for a maintainer to first encounter a finding about their own codebase, and an iOS-side document enumerating another team's credential handling reads as an audit published over their head rather than a colleague's note. The specifics belong in a direct conversation with the owner, who then has the chance to assess, confirm, or say it is already handled. What the document needs is the planning fact, which is unchanged and still stated: the existing Android report path attaches unredacted logs, the bot would use that path, so redaction is a prerequisite workstream for the Android client and is plausibly larger than the bot module itself. **Scope:** removes the escalation section and its contents entry, restates section 8.4 without the field-level detail, and neutralizes the supporting footnote. No change to any recommendation, decision, or estimate. **Not done:** the finding itself is unaffected and still needs an owner on the Android side. That conversation happens directly, not through this document. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🏗️ CodeAtlas Ledger Analysis✅ All Checks Passed♿ Accessibility (via AccessLint)
✅ No accessibility issues detected 🏛️ Architecture Analysis
🔄 Dependency CyclesSee full report for cycle details. 📋 Architecture FindingsSee full report for detailed findings. 🔍 Reachability Analysis
ℹ️ No architecture-relevant files in this diff — reachability not evaluated. 📊 0 files analyzed | 📦 Download Full Report Powered by CodeAtlas Ledger |
🧪 Unit Test Results📊 View Full Interactive Report ❌ 3 TESTS FAILED8187 tests | 8043 passed | 3 failed | 139 skipped | ⏱️ 15m 7s | 📊 98.2% | 📈 45.1% coverage All 942 classes — 3 with failures (full matrix)
📊 Full interactive matrix: report ❌ Failed testsNames only — open the interactive report above for messages + stack frames. 📊 Testing Coverage BreakdownUnit Test Line Coverage (testable surfaces): 45.1% Total coverage incl. UI/lifecycle: 44.1% (17 files excluded from testable denominator — see
📈 What changed vs. baseTest count: +8
|
| Artifact | Description |
|---|---|
| test-report | 📄 Markdown + HTML reports |
| test-data | 📊 JSON data for tooling |
| test-results | 🔍 Full xcresult (open in Xcode) |
Final editing pass before review. No facts, recommendations, numbers, or citations changed. Roles replace people throughout. The cover memo's accountable owner is now the iOS dev rather than a named individual, and every "maintainer" is now the Android dev or a phrase about the work itself. Where a sentence needed a role word to carry meaning, it was rephrased around the work: "the maintainer's to assess" became "this sits with the Android dev". Factual properties of a codebase kept the fact and dropped the title, so "single-maintainer repo" reads "single-contributor repo" and states the commit distribution instead. Documents that circulate should describe roles and work, not individuals. Voice moves from argued to documented. Removed rhetorical closers, moral framing about discipline, bold used for emphasis rather than scanning, empty intensifiers, and phrasing that praised the analysis. Reasons are stated once, factually, instead of built toward. Kept deliberately: the UNVERIFIED markers, the retractions, the "both numbers are proposals, not findings" note, and the steelmanned counter to the engine recommendation. Those read as hedging only if you mistake accuracy for timidity; three reviewers named them the documents' most useful property. One real gap closed in the recommendation path: the cover memo asked approval for the first two delivery phases without mentioning that the iOS launch gate precedes them, so a reader tracing "what happens first" hit a contradiction between the memo and the plan. Fixed with a pointer rather than more text. **Scope:** voice, role language, and one cross-reference. Both documents got shorter. **Not done:** the style is now recorded outside this repo so it applies by default to future documentation; nothing in this commit enforces it here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… toggle, telemetry decision (#1359) * fix(triage-bot): honor field omissions on the clipboard send path (PP-4883) The patron reviews the support ticket field-by-field and can switch pieces off (barcode, library, network, logs). Only the email path stripped those fields; the clipboard/copy path (the shipping default, submission flag off) JSON-encoded the raw draft, so anything the patron switched off was still in what got copied — the payload even listed the fields it was supposed to have removed (as-built gap 12). Content-level redaction still ran on both paths, so no secret leaked; the narrow failure was that the patron said "not this" and we copied it anyway. Centralize ticket serialization in Core (TicketWirePayload), which always encodes sanitizedForSubmission(). Route both the clipboard gateway and the email diagnostics attachment through it, so the two paths produce byte-identical bytes and cannot drift apart. Document the serialization contract on the TicketGateway protocol so a future send path can't reintroduce the gap. Tests: TicketWirePayloadTests (Core, runs in the fast swift-test gate that already guards email) pins that omitted fields are absent, non-omitted fields survive, and the copy payload is byte-identical to the email attachment. ClipboardTicketGatewayTests pins the gateway itself via an injected write sink (no global UIPasteboard). Full Core suite: 289 passed. **Scope:** clipboard/email serialization parity only. No change to what the redactor strips or to the email path's behavior (byte-identical output). **Not done:** full Palace test suite (runs on PR CI); SoD critical-path review markers (privacy/redaction path). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(triage-bot): patron-facing diagnostics toggle + privacy review record (PP-4884) The bot had a working "don't send diagnostics" preference (DiagnosticsGatingContextProvider) with tests, but setIncludeDiagnostics had zero callers — no screen, no row, no toggle — so every patron was on the default (ON). The choice was written and never given to anyone (as-built gap 5). Add an "Include diagnostics" toggle to the Support settings section, shown only when the bot is active. It binds via @AppStorage to the exact key the gating context provider reads, now exposed as UserDefaultsDiagnosticsPreference.defaultsKey, so flipping the switch changes what the bot collects with no other wiring and no key drift. A caption explains the trade honestly: off means support has only app + device version to work with. Also records the privacy review as a durable decision document (docs/architecture/triage-bot-privacy-review.md): what the bot collects, what leaves the device and when, on-device retention, the consent model, and open items for the reviewing roles (identifier hash strength, default-on posture, support-side retention). The engineering facts are cited to code; the answer itself is a product/legal call recorded in the doc's Decision block. Test: DiagnosticsPreferenceTests pins that a toggle write under defaultsKey is read back by the default-key preference the factory builds (guards the toggle and provider against drifting apart). Full Core suite: 289 passed; app builds on iPhone 17 Pro sim. **Scope:** engineering wiring for the toggle + the recorded review doc. **Not done / needs product-legal (story Done-when, not code):** the toggle COPY needs product sign-off (marked in-code); the privacy review needs a recorded answer in the Decision block before the bot is enabled for patrons. **Deferred:** none. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(triage-bot): telemetry destination decision + measurement yardstick (PP-4885) The bot reports what happens as patrons use it — conversations started, answers offered, guided-step outcomes, escalations — through a fixed 12-key allow-list carrying no patron text. In release builds the events reach Firebase, and nothing on the other end reads them (as-built gap 6). So no one can say whether the bot helps, and the whole shared-architecture plan assumes we learn from iOS in the field first. Record the decision and the yardstick (resolves open decision 4 of the shared-architecture proposal): - Recommend Firebase for the pilot (the data already flows there, zero new infrastructure) with the Circulation Manager ingestion endpoint as the follow-on once the feature earns the larger investment. - Define the three questions as concrete metrics over the real event vocabulary: auto-resolution rate, per-category miss rate, guided-flow abandonment by step. Names the honest limit — missed-input phrasings are deliberately not in telemetry, so category miss rate points where to look and the escalated support tickets supply the actual phrasings. - Propose starting thresholds to ratify so the rollout has a number, not a vibe. Docs only. Producing an actual number needs no app change (events already reach Firebase); it needs the destination decision and the queries, owned by whoever holds that console. **Scope:** the written decision + metric definitions + proposed thresholds. **Not done / needs product-infra (story Done-when, not code):** ratify the destination and thresholds in the doc's Decision block; assign a query owner. No in-app view is built — it would front-run the destination decision and would not measure field usage, which is what the rollout needs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(triage-bot): guard the omit-guarantee in the reducer, in the fast CI gate (PP-4883) Addresses the qa_test SoD block. The original fix routed the clipboard gateway through the sanitizing serializer, but the gateway-level test that pinned it (ClipboardTicketGatewayTests) runs in NEITHER CI job: macOS `swift test` compiles it away under `#if canImport(UIKit)`, and the Palace xcodebuild scheme runs only PalaceTests/TenPrintCoverTests, not the package's IOS test target. So the exact gap-12 mutation — a gateway reverting to encode the raw draft — would ship the leak again with CI green. Move the guarantee to the pure-Core reducer: the draft handed to the `.submitTicket` effect is now `sanitizedForSubmission()`, at both the confirm and retry sites, so the wire draft is omit-stripped before ANY gateway sees it, independent of which gateway runs. `.submitting(ticket:)` keeps the original draft so the failure/retry/persist path still shows what the patron reviewed. sanitize is idempotent, so the gateway/composition serialization sanitize becomes harmless defense-in-depth. SubmitEffectSanitizationTests drives the reducer and asserts the emitted effect's draft is wire-ready (confirm path, retry path, and nothing-omitted control). It runs in the fast `swift test` gate; reverting the reducer sanitize fails it with 5 assertion failures (verified by mutation). Also applies the architect's non-blocking note: the email JSON attachment now passes the raw draft to TicketWirePayload.jsonData (sole sanitize owner) instead of a pre-sanitized draft, removing the double-sanitize idempotency reliance. Byte-identical output preserved (TicketWirePayloadTests pins it). Core suite: 292 passed. **Scope:** reducer submit-effect sanitize + its Core test + email single-owner cleanup. No behavior change to the failure/retry/persist UX (state draft unchanged). **Not done:** wiring the package IOS/UI test targets into CI is a separate, board-stability-sensitive change (deferred; the reducer guard covers the privacy regression in the meantime). 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Output of the PP-4858 spike, plus two small changes it surfaced.
Draft because these are for reading and comment first. Review inline, on the sentence.
The two documents
docs/architecture/triage-bot-v1-as-built.mddescribes the triage bot as it works today, written as the behavioral contract another platform reproduces: normalization, scoring and the four suggest guards, the redaction rules and their order, the corpus schema, and the escalation state machine. Platform-specific detail is labeled so nobody ports it by mistake.docs/architecture/triage-bot-shared-architecture-proposal.mddesigns the server and Android halves. It opens with a decision memo naming what is being asked for, what is deliberately not being approved yet, and three risks that still need someone's name against them.If you have fifteen minutes and a decision to make: read the cover memo, then the delivery plan and organizational dependencies. Everything else is reference.
Recommendation
A hybrid. The corpus ships as a versioned CDN artifact rather than a new service, because the org already distributes versioned content that way in two places. Ticket submission becomes a per-library endpoint on the existing Circulation Manager. The AI key proxy stays a hard prerequisite for the AI fallback but is sequenced last, since it is the only piece that may need new infrastructure. Classification and redaction stay on the phones. Android gets a small hand-written Kotlin module kept in step by shared test fixtures rather than shared code.
Three findings worth reading even if nothing else is
Also in this PR
scripts/triage-corpus-check.shdescribed itself as a release gate but had no caller anywhere. Its--self-test, which proves the guard goes red when redaction is disabled, was therefore never running. Wired into CI andverify-pr.sh.How it was checked
Four grounded passes over
android-core,circulation,library-registryand the analytics tooling. Three independent reviews from backend, Android and iOS perspectives. A source audit of every factual claim. And one experiment: the matching engine was rebuilt in Python from the document alone, without reading the Swift, then run against the real conformance corpora. It matched 85 of 85. The claim that another platform can build from this document is demonstrated rather than asserted.Verification
Related: PP-4882 and sub-tasks PP-4883 to PP-4886 cover the gaps that need closing before the bot's flag is turned on for patrons. The bot ships in 3.3.0 with its master switch off, so nothing is patron-facing yet.
🤖 Generated with Claude Code