feat(audit): incidents source collector for a measured MTTR (DF-07) - #170
feat(audit): incidents source collector for a measured MTTR (DF-07)#170dustyo-O wants to merge 3 commits into
Conversation
MTTR (DF-07, category 1103) had no real incident source — the "incident" slot in standards.toml (sources = [tracker, incident]) had no collector, so MTTR only ever reported a git branch-lifetime proxy and 1103 never scored. Git recovery signals are already covered (change_failure_rate + this proxy); what was missing is ingesting real incident sources. Add that. - collectors/incidents.ts: connector-passed (mirrors tracker/ci/docs). The orchestrator fetches incidents from a real source — PagerDuty/OpsGenie/ incident.io, Statuspage/Atlassian, or code-host incident labels — normalises them to IncidentRecord[], and passes them; the collector computes per-incident recovery spans and their median. No connector → available:false (SKIP), so git-only repos are unaffected. - metrics/mttr.ts: a real-source branch takes precedence — median recovery time, reliability "maximal", category 1103 awarded when >=1 incident is resolved. The git-proxy path is preserved unchanged as the fallback. - references/connector-shapes.md: full Incidents recipe (IncidentRecord / IncidentsConnector shapes + per-platform mapping for all three source families). SKILL.md already lists "incident" among fetchable connectors. - audit_core: collect incidents in the base pass; has_incident_source is satisfied by a resolved incident (or a tracker incident_source — the .raw read is also corrected). artifact_types registers the 'incidents' source. - standards.toml + delivery-flow.md: DF-07 reflects the measured-from-real- source derivation with the git-proxy fallback. Tests: new incidents-collector + met-mttr-incidents suites; the existing git-proxy tests stay untouched and pass. Full engine suite: 1331 pass. Rebuilt dist/. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Follow-ups agreed during review of the incidents source collector:
- connector-shapes.md: reword the incidents recipe to map semantically from
the live tool response ("open time → started_at") rather than a fixed vendor
field list — the field names are demoted to typical-as-of-writing hints, so a
vendor API rename can't mislead the orchestrator (which reads the live MCP/CLI
response anyway).
- lint: pin connector-shapes.md to collectors/incidents.ts — every field on the
IncidentRecord / IncidentsConnector interfaces must be documented, so the
recipe can't silently drift from the code contract.
- tests/det-incidents-mttr.test.ts: commit the end-to-end check as a real
integration test — build a git repo, run audit-core (DF-07 SKIP), write a
connector-shaped collected/incidents.json, run enrich, assert DF-07 becomes a
measured (2h), awarded, maximal-reliability result.
Co-Authored-By: Claude Opus 4.8 <[email protected]>
📝 WalkthroughWalkthroughThe PR adds an incidents collector and artifact schema, integrates resolved incident medians into DF-07 MTTR, preserves git-proxy fallback behavior, updates standards and documentation, and adds collector, metric, enrichment, and documentation tests. ChangesIncident-based MTTR
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AuditCore
participant IncidentsCollector
participant IncidentsArtifact
participant MTTRMetric
AuditCore->>IncidentsCollector: collect incident records
IncidentsCollector->>IncidentsArtifact: write normalized incident data
MTTRMetric->>IncidentsArtifact: read incident records
MTTRMetric-->>AuditCore: return incident MTTR or git proxy
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: Ping-pong health check failed Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
plugins/awos/skills/ai-readiness-audit/metrics/mttr.ts (1)
69-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a shared helper for the "measurable incident source" predicate.
metrics/mttr.tsandaudit_core.tseach independently re-implement the same check — an available incidents artifact withresolved_count > 0— to decide, respectively, which value MTTR reports and whether category 1103 is awarded. They agree today, but duplicated logic like this can silently drift if only one side is updated later, decoupling the awarded category from the value actually computed.
plugins/awos/skills/ai-readiness-audit/metrics/mttr.ts#L69-L111: extract theavailable && median_duration_hours is number && resolved_count > 0check into a shared helper (e.g. exported fromcollectors/incidents.tsor a small shared module) and call it here.plugins/awos/skills/ai-readiness-audit/audit_core.ts#L605-L621: call the same shared helper here instead of re-derivingresolved_count > 0inline, sohas_incident_sourceand mttr.ts's branch decision can never diverge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/awos/skills/ai-readiness-audit/metrics/mttr.ts` around lines 69 - 111, Extract and export a shared measurable-incident predicate from the incidents collector or a suitable shared module, requiring an available incidents artifact, numeric median_duration_hours, and resolved_count > 0. Update plugins/awos/skills/ai-readiness-audit/metrics/mttr.ts lines 69-111 to use it for the measured MTTR branch, and plugins/awos/skills/ai-readiness-audit/audit_core.ts lines 605-621 to use it for has_incident_source instead of duplicating the resolved-count check.tests/lint-prompts.test.js (1)
4079-4096: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse a word-boundary match instead of a raw substring check.
doc.includes(f)at line 4090 can produce false negatives for short field names.idis a substring of "incident," which appears throughout this document, sodoc.includes('id')is always true regardless of whether theidfield itself is documented. This defeats the drift guard for exactly the fields most likely to silently drift unnoticed.Match on a word boundary so the check reflects whether the field name itself appears, not any substring occurrence.
🔍 Proposed fix
- const missing = fields.filter((f) => !doc.includes(f)); + const missing = fields.filter((f) => !new RegExp(`\\b${f}\\b`).test(doc));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/lint-prompts.test.js` around lines 4079 - 4096, In the fields.filter call that creates the missing array, replace the substring check doc.includes(f) with a word-boundary regex match to ensure field names are matched as complete words rather than substrings. This prevents false positives where short field names like 'id' incorrectly match within unrelated words like 'incident', ensuring the drift guard actually detects undocumented fields.plugins/awos/skills/ai-readiness-audit/collectors/incidents.ts (1)
63-91: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard
connector.incidentsbefore mapping durations.Connector-passed artifacts can omit or mis-map
incidents, soconnector.incidents ?? []does not protect non-array values. UseArray.isArraybefore.map(durationHours)and add a test for a malformed non-arrayincidents; run the tests if available.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/awos/skills/ai-readiness-audit/collectors/incidents.ts` around lines 63 - 91, Update collect to validate connector.incidents with Array.isArray before mapping durations, falling back to an empty array for malformed non-array values while preserving valid incident handling. Add a test covering a connector with non-array incidents and run the available tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@plugins/awos/skills/ai-readiness-audit/collectors/incidents.ts`:
- Around line 63-91: Update collect to validate connector.incidents with
Array.isArray before mapping durations, falling back to an empty array for
malformed non-array values while preserving valid incident handling. Add a test
covering a connector with non-array incidents and run the available tests.
In `@plugins/awos/skills/ai-readiness-audit/metrics/mttr.ts`:
- Around line 69-111: Extract and export a shared measurable-incident predicate
from the incidents collector or a suitable shared module, requiring an available
incidents artifact, numeric median_duration_hours, and resolved_count > 0.
Update plugins/awos/skills/ai-readiness-audit/metrics/mttr.ts lines 69-111 to
use it for the measured MTTR branch, and
plugins/awos/skills/ai-readiness-audit/audit_core.ts lines 605-621 to use it for
has_incident_source instead of duplicating the resolved-count check.
In `@tests/lint-prompts.test.js`:
- Around line 4079-4096: In the fields.filter call that creates the missing
array, replace the substring check doc.includes(f) with a word-boundary regex
match to ensure field names are matched as complete words rather than
substrings. This prevents false positives where short field names like 'id'
incorrectly match within unrelated words like 'incident', ensuring the drift
guard actually detects undocumented fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dd65a89a-3357-4baf-acab-cbc7120e041b
⛔ Files ignored due to path filters (1)
plugins/awos/skills/ai-readiness-audit/dist/cli.jsis excluded by!**/dist/**
📒 Files selected for processing (11)
plugins/awos/skills/ai-readiness-audit/artifact_types.tsplugins/awos/skills/ai-readiness-audit/audit_core.tsplugins/awos/skills/ai-readiness-audit/collectors/incidents.tsplugins/awos/skills/ai-readiness-audit/dimensions/delivery-flow.mdplugins/awos/skills/ai-readiness-audit/metrics/mttr.tsplugins/awos/skills/ai-readiness-audit/references/connector-shapes.mdplugins/awos/skills/ai-readiness-audit/references/standards.tomlplugins/awos/skills/ai-readiness-audit/tests/det-incidents-mttr.test.tsplugins/awos/skills/ai-readiness-audit/tests/incidents-collector.test.tsplugins/awos/skills/ai-readiness-audit/tests/met-mttr-incidents.test.tstests/lint-prompts.test.js
AlexanderMakarov
left a comment
AlexanderMakarov
left a comment
There was a problem hiding this comment.
This lands the right architecture for a measured MTTR — a connector-passed collector mirroring tracker/docs, the git proxy preserved as fallback, and an e2e through the shipped bundle; the mechanics the roadmap item lists (new collector, COLLECTOR_SOURCES, the metric, standards.toml, dist/ rebuilt) are all here. But the item's core ask is MTTR
computed deterministically by the engine from actual incident records instead of being an orchestrator-transcribed display string
and as it stands, that's the one part that didn't land - the engine only trusts derived aggregates that nothing in the shipped flow computes, so the median is still orchestrator-computed and republished verbatim at maximal reliability (inline on connector-shapes.md). Add to that the newly-live tracker gate awarding the category against the PR's own definition (inline on audit_core.ts), and the measured number isn't yet trustworthy end-to-end. Three things that don't map to changed lines:
The executive headline still denies the feature. computeDerivedDelivery (audit_core.ts:79-140) only reads the tracker, and DerivedDelivery.mttr has no value slot (unlike its sibling cycle_time), so after a successful enrich the headline row still renders "— (needs incident connector)" — or the now-false "incident source declared — no incident data mapped" — while the DF-07 detail right below shows a measured, maximal-reliability median. That's the headline/detail contradiction the 5bca168 fix was for, quoted in computeDerivedDelivery's own docstring ("deriving both the value and the honest gated note from the SAME artifact makes that contradiction impossible"). SKILL.md and output-format.md also still say the MTTR headline is computed from collected/tracker.json. If threading the incidents artifact through the three call sites is out of scope here, a tracking note would help — as shipped, the report's most visible row contradicts its own detail.
The plugin version isn't bumped. marketplace.json, plugin.json, and the EXPECTED_PLUGIN_VERSION pin are all still 2.4.3, identical to main. This PR changes scoring behavior, and the engine stamps the plugin version into audit.json provenance and the report footer, so audits run after merge get mislabeled — the exact scenario the three-file rule exists for (#145/#150/#154 all bumped in-PR). If a separate release commit is the plan, worth saying so on the PR.
CONNECTABLE_SOURCES / COLLECTED_ARTIFACT_SOURCES (audit_core.ts:1049-1066) never gained 'incidents'. Masked today — 1103 also lists tracker and is topology-gated, so neither buildSkipReason nor canReuse misbehaves yet — but the first category that lists sources = ["incidents"] alone gets a wrong skip reason and is wrongly treated as reusable on enrich, silently ignoring freshly-fetched connector data. The 'incident' singular in those two sets is dead and can go (render.ts's gated: 'incident' union is a different, live concept — leave that alone).
|
|
||
| ### IncidentsConnector | ||
|
|
||
| The object the orchestrator assembles and writes to `collected/incidents.json`: |
There was a problem hiding this comment.
This recipe and the engine disagree about who computes the median, and the gap is silent in the worst case. The metric's new branch trusts only the derived fields — raw.resolved_count / raw.median_duration_hours (metrics/mttr.ts:81), and the topology gate reads raw.resolved_count too — but nothing in the shipped flow computes them: audit-core calls the collector with no connector, enrich skips collection entirely, and no CLI verb takes one, so collect()'s median logic is unreachable outside the unit tests. An orchestrator following this section literally writes {incidents, source_label}: as a bare object it at least trips the stranded-payload warning, but wrapped in the proper {available: true, raw: {…}} envelope with no derived fields, nothing fires — mttr silently falls back to the git proxy and DF-07 reads "Not applicable" while valid PagerDuty data sits on disk. And when the orchestrator does hand-write median_duration_hours, the engine republishes that number verbatim as maximal-reliability "measured from N resolved incidents" — an arithmetic slip in the orchestrator becomes a measured DORA number. The tracker already solved this shape problem: collectors/tracker.ts exports buildTypeCounts/countResolved so the metrics derive aggregates themselves, and its recipe says the orchestrator "does not need to produce this directly". This is also the roadmap item's central requirement — MTTR "computed deterministically by the engine from actual incident records instead of being an orchestrator-transcribed display string" — and hand-written median_duration_hours is exactly an orchestrator-transcribed number, just relocated into an artifact. I'd do what the tracker does — export durationHours/median from the collector, derive from raw.incidents[] in mttr.ts and the topology gate when the aggregates are absent (treating orchestrator-supplied ones as advisory), and warn when an available incidents artifact has records but no measurable span. That also makes this recipe truthful as written, and gives the e2e a variant worth adding: feed the envelope with only incidents[] and assert it still scores. (Small tell: det-incidents-mttr.test.ts:4 calls the hand-written full-envelope artifact "connector-shaped" — it isn't; it's the derived shape the doc never mentions.)
There was a problem hiding this comment.
Done — this is the main fix. The engine now derives the median itself from raw.incidents[] (deriveIncidentAggregates, shared by the metric, the gate, and the collector); any hand-written median_duration_hours is advisory and ignored. Reworked the recipe to the records-only envelope, and the e2e now feeds incidents[] only and asserts it still scores — the 'connector-shaped' mislabel is gone too.
| has_tracker: Boolean(trackerArt?.available), | ||
| has_docs_connector: Boolean(docsArt?.available), | ||
| // A real, measurable incident source: the connector-passed incidents | ||
| // artifact with at least one resolved incident, or a tracker that names an | ||
| // incident source. Gates DF-07 (category 1103); the git proxy stays SKIP. | ||
| has_incident_source: Boolean( | ||
| trackerArt?.available && trackerArt?.incident_source | ||
| (incidentsArt?.available && (incidentsRaw?.resolved_count ?? 0) > 0) || | ||
| (trackerArt?.available && trackerRaw?.incident_source) | ||
| ), |
There was a problem hiding this comment.
The .raw fix is right — the old top-level read meant this branch never fired in any audit to date — but making it live changes scoring in a way the PR's own docs disavow: a tracker that merely names an incident system now flips has_incident_source, and the git-proxy fallback still calls awardCategories, so 1103's full weight 5 is granted on a not-reliable proxy with zero incident data. That contradicts the new standards.toml definition ("awarded when at least one incident has a resolved recovery span"), the DF-07 What-line, and the comment two lines up. There's also no test pinning either leg of this expression — reverting the .raw fix fails nothing. I'd either drop the tracker disjunct (award only on measured spans) or keep it deliberately and align the three doc strings; either way a couple of pinning tests would guard it (tracker-declared path → flag true; incidents available with zero resolved → flag false).
There was a problem hiding this comment.
Dropped the tracker disjunct — 1103 is awarded only on a measured recovery span. standards.toml sources are incidents-only now, and I added the pinning tests: tracker-declared-only leaves DF-07 SKIP (e2e), plus hasMeasurableIncidents true/false legs (unit).
| ### DF-07: Mean time to recovery | ||
|
|
||
| - **What:** Mean time to recovery from incidents; computed from git as a proxy by default (merge/revert/hotfix cadence), upgraded when a real incident source is present in the tracker artifact. Always included — never omitted from the artifact. SKIP only if even git is unavailable. | ||
| - **What:** Median time to recovery from incidents. Measured from a real incident source — the incidents collector (`collectors/incidents.ts`), fed by the orchestrator from PagerDuty/OpsGenie/incident.io, Statuspage, or code-host incident labels (see `references/connector-shapes.md`). Category 1103 is awarded when at least one incident has a resolved recovery span. Without a source it falls back to a git proxy (merge branch-lifetime) and stays SKIP. Always included — never omitted from the artifact. |
There was a problem hiding this comment.
The new What-line and the retained Skip bullet now contradict each other: here, no source → git proxy "and stays SKIP"; three lines down, SKIP is only valid when git is unavailable, and Pass explicitly includes the git-proxy value. Two different SKIPs are conflated — the DF-07 check (SKIP whenever has_incident_source is false; your own e2e asserts that baseline) and the metric (never returns SKIP). I'd write "…and category 1103 stays SKIP" here and rewrite the Skip bullet to name the check, not the metric. Same wording slip in the audit_core.ts:617 comment ("the git proxy stays SKIP").
There was a problem hiding this comment.
Rewrote it — Pass/Skip now describe the DF-07 check via has_incident_source, and spell out that the mttr metric itself never SKIPs (it still reports the git proxy for context). Fixed the same slip in the audit_core comment.
| // --- Real incident source (connector-passed) takes precedence over the git | ||
| // proxy. When the orchestrator has fetched a real incident source and at | ||
| // least one incident has a measurable started→resolved span, MTTR is the | ||
| // median of those spans — a first-class measurement, reliability maximal. | ||
| const incidentsRead = readArtifact(collectedDir, 'incidents'); | ||
| if (!('error' in incidentsRead) && incidentsRead.artifact?.available) { | ||
| const iraw = (incidentsRead.artifact.raw ?? {}) as { | ||
| resolved_count?: number; | ||
| median_duration_hours?: number | null; | ||
| source_label?: string | null; | ||
| }; | ||
| const resolved = iraw.resolved_count ?? 0; | ||
| if (typeof iraw.median_duration_hours === 'number' && resolved > 0) { |
There was a problem hiding this comment.
The module header above still states the opposite of this branch — "no MTTR is ever computed from incident data here … reliability/confidence stay at the proxy's minimal level" — and its "Source shapes" list omits incidents.json, now the primary source. Worth rewriting for the two-path contract while you're in here. The header also carries two pre-existing self-contradictions worth fixing in the same pass: the proxy is uniform branch-lifetime over all first-parent merges, not "intervals between consecutive revert/hotfix/rollback merge commits", and "(incident_source field upgrades reliability)" is denied by the body itself.
There was a problem hiding this comment.
Rewrote the header for the two-path contract, and fixed the two pre-existing contradictions: the proxy is a uniform branch-lifetime over all first-parent merges, and the incident_source line no longer claims a reliability upgrade. incidents.json is listed as the primary source now.
| function durationHours(inc: IncidentRecord): number | null { | ||
| if (!inc.resolved_at) return null; | ||
| const start = Date.parse(inc.started_at); | ||
| const end = Date.parse(inc.resolved_at); | ||
| if (Number.isNaN(start) || Number.isNaN(end) || end < start) return null; | ||
| return (end - start) / 3_600_000; | ||
| } | ||
|
|
There was a problem hiding this comment.
Every malformed record lands here as null and silently becomes "unresolved": a mis-mapped started_at, epoch-millis instead of ISO, or swapped fields all vanish without a trace, so a whole-batch mapping mistake reads as "no measurable incidents → Not applicable" with zero signal. code_host got a dedicated "N of M records lack a parseable timestamp" warning for the identical failure shape — incidents deserve the same: an invalid_count in the raw plus an artifact warning, and a "measured from N of M" note when only part of the sample is usable. Related edge: end === start counts a 0h span as measured, so auto-resolved alert flaps can band "elite". No test currently covers reversed spans or garbage dates.
There was a problem hiding this comment.
Added it — invalid_count in the raw plus a reliability note, and a 'measured from N of M' style note when only part of the sample is usable. Also fixed the 0h edge: end <= start is now invalid, so an auto-resolved flap can't band elite. Tests cover reversed spans, garbage dates, and the flap.
| ); | ||
| } | ||
|
|
||
| const incidents = connector.incidents ?? []; |
There was a problem hiding this comment.
Nothing clamps these to the audit window: the recipe asks the orchestrator to query the same window, but unlike CI runs and tracker tickets — which the engine clamps defensively regardless of what it's sent — incidents are accepted as-is, including future-dated or all-time history. An over-fetched source silently shifts the "audit-window" median at maximal reliability and makes the number non-reproducible across differently-scoped fetches. A started_at within window_anchor − lookback_days filter wherever the derivation ends up (see the connector-shapes comment) would close it.
There was a problem hiding this comment.
Clamped — deriveIncidentAggregates drops incidents whose started_at falls outside window_anchor - lookback_days, anchored to the newest record the same way clampToWindow does for CI/tracker. Test covers an over-fetched all-time history being dropped to the window.
| ...fieldsOf('IncidentRecord'), | ||
| ...fieldsOf('IncidentsConnector'), | ||
| ]; | ||
| const missing = fields.filter((f) => !doc.includes(f)); |
There was a problem hiding this comment.
Building on CodeRabbit's word-boundary point — it's a bit worse than a nitpick: doc.includes('id') is satisfied by the word "incident", and source/incidents/count by ordinary prose, so about half the guarded fields are checked vacuously. And the interfaces it checks are the input shapes; IncidentsRaw — whose resolved_count/median_duration_hours are what the engine actually reads — isn't covered, which is exactly the contract the recipe currently omits (see the connector-shapes comment). Matching identifiers inside the jsonc fences (or \b-bounded) and including IncidentsRaw would make the guard catch the drift that matters.
There was a problem hiding this comment.
Fixed — the guard now matches each field as a standalone identifier (id is no longer satisfied by 'incident', nor count by 'resolved_count') and covers IncidentsRaw, so the derived fields the engine actually reads are both guarded and documented in the recipe.
Address review: the connector shipped, but the engine trusted an orchestrator-transcribed median. Now the engine derives it deterministically from raw.incidents[], so a transcription slip can't become a measured DORA number. - collectors/incidents.ts: shared deriveIncidentAggregates (window-clamped, invalid_count) + durationHours/hasMeasurableIncidents; end<=start rejected - mttr.ts + topology gate derive from raw.incidents[]; advisory aggregates ignored; rewrite stale header for the two-path contract - gate: award 1103 only on a measured recovery span (drop tracker disjunct); standards sources -> ["incidents"] - headline: DerivedDelivery.mttr gains a value slot; computeDerivedDelivery reads the incidents artifact independently of the tracker; render + docs - fix dead 'incident' -> 'incidents' in the two source sets - clamp incidents to the audit window; count/flag unparseable spans - lint drift guard: word-boundary match + cover IncidentsRaw - bump plugin 2.4.3 -> 2.4.4 (marketplace, plugin.json, flow.md, pin) - rebuild dist Engine 1343/0, lint 148/0. Co-Authored-By: Claude Opus 4.8 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/awos/skills/ai-readiness-audit/metrics/mttr.ts (1)
72-131: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winScale reliability/confidence with sample adequacy, not just
resolved_count > 0.This branch awards
tag: 'maximal',confidence: 'HIGH', and a fixedconfidence: 0.9wheneveragg.resolved_count > 0(Lines 87-101, 115), with no regard for how many incidents wereinvalidrelative toresolved, or how small the resolved sample is. A single resolved span out of 99 invalid ones, or a median computed from exactly one incident, gets the same "maximal/HIGH/0.9" signal as a robust sample. The git-proxy fallback below (Lines 219-223) does scale confidence with the number of intervals available (MEDvsLOW); the incidents path should apply an equivalent adequacy check so the reported reliability actually reflects data quality.🐛 Proposed fix to scale confidence with sample adequacy
const measured = agg.resolved_count; const total = measured + agg.invalid_count; const ofNote = agg.invalid_count > 0 ? ` (of ${total} resolved; ${agg.invalid_count} lacked a parseable span)` : ''; + const dataQualityOk = total === 0 || measured / total >= 0.5; + const sampleOk = measured >= 3; const reliability: Reliability = { - tag: 'maximal', - confidence: 'HIGH', + tag: dataQualityOk && sampleOk ? 'maximal' : 'not-reliable', + confidence: dataQualityOk && sampleOk ? 'HIGH' : 'LOW', note: `measured from ${measured} resolved incident${measured === 1 ? '' : 's'}${iraw.source_label ? ` (${iraw.source_label})` : ''}${ofNote}`, };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/awos/skills/ai-readiness-audit/metrics/mttr.ts` around lines 72 - 131, Update the incident-derived branch in the MTTR calculation around deriveIncidentAggregates and makeMetricResult so reliability and numeric confidence reflect sample adequacy instead of always using maximal/HIGH/0.9. Account for both the resolved sample size and the proportion of invalid incidents, applying an equivalent tiered confidence policy to the git-proxy fallback while preserving the existing median, scoring, and fallback behavior.
🧹 Nitpick comments (1)
plugins/awos/skills/ai-readiness-audit/collectors/incidents.ts (1)
107-145: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPersist
dropped_out_of_window(and a warning) in the incidents artifact.
deriveIncidentAggregatescomputesdropped_out_of_window(Line 143), butIncidentsRawhas no field for it, andcollect()discards it when buildingraw(Lines 179-186). The artifact message is also alwaysnull(Line 187), even wheninvalid_count > 0. Since the past review specifically flagged reproducibility risk from over-fetched/out-of-window incidents, persisting how many records were dropped (and surfacing a short warning) inincidents.jsonitself would let anyone inspecting the raw artifact — not just the MTTR metric — diagnose a mismatched fetch window or a batch mapping problem.♻️ Proposed fix to surface drop/invalid counts on the artifact
export interface IncidentsRaw { incidents: IncidentRecord[]; count: number; /** Incidents with a valid started→resolved span (the only ones MTTR can measure). */ resolved_count: number; /** Resolved incidents whose span could not be parsed (bad/zero/reversed timestamps). */ invalid_count: number; /** Median recovery time in hours over resolved incidents; null when none. */ median_duration_hours: number | null; source_label: string | null; + /** Incidents dropped for falling outside the audit window. */ + dropped_out_of_window: number; }const raw: IncidentsRaw = { incidents: agg.incidents, count: agg.count, resolved_count: agg.resolved_count, invalid_count: agg.invalid_count, median_duration_hours: agg.median_duration_hours, source_label: connector.source_label ?? null, + dropped_out_of_window: agg.dropped_out_of_window, }; - return makeArtifact('incidents', true, null, period, raw); + const warning = + agg.invalid_count > 0 + ? `${agg.invalid_count} of ${agg.resolved_count + agg.invalid_count} resolved incidents lacked a parseable span` + : null; + return makeArtifact('incidents', true, warning, period, raw);Also applies to: 160-188
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/awos/skills/ai-readiness-audit/collectors/incidents.ts` around lines 107 - 145, Update IncidentsRaw and the collect() artifact-building flow to persist deriveIncidentAggregates().dropped_out_of_window in incidents.json, rather than discarding it when constructing raw. Populate the artifact message with a concise warning whenever invalid_count > 0 or dropped_out_of_window > 0, while retaining the existing null message when neither condition occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/awos/skills/ai-readiness-audit/SKILL.md`:
- Line 171: Align the Merges display format in the relevant documentation so it
consistently uses "<n> / week (per active contributor)" everywhere, including
the output-format guidance and the SKILL.md prompt. Preserve the existing
null-value omission behavior and all other metric requirements.
- Line 170: Update the nonexistent “SKILL.md Step 6.4” reference to “SKILL.md
Step 5.4” in plugins/awos/skills/ai-readiness-audit/SKILL.md at line 170 and
plugins/awos/skills/ai-readiness-audit/output-format.md at line 77, ensuring
both prompt documents point to the report-authoring workflow in Step 5, item 4.
---
Outside diff comments:
In `@plugins/awos/skills/ai-readiness-audit/metrics/mttr.ts`:
- Around line 72-131: Update the incident-derived branch in the MTTR calculation
around deriveIncidentAggregates and makeMetricResult so reliability and numeric
confidence reflect sample adequacy instead of always using maximal/HIGH/0.9.
Account for both the resolved sample size and the proportion of invalid
incidents, applying an equivalent tiered confidence policy to the git-proxy
fallback while preserving the existing median, scoring, and fallback behavior.
---
Nitpick comments:
In `@plugins/awos/skills/ai-readiness-audit/collectors/incidents.ts`:
- Around line 107-145: Update IncidentsRaw and the collect() artifact-building
flow to persist deriveIncidentAggregates().dropped_out_of_window in
incidents.json, rather than discarding it when constructing raw. Populate the
artifact message with a concise warning whenever invalid_count > 0 or
dropped_out_of_window > 0, while retaining the existing null message when
neither condition occurs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1b317628-926a-4e19-8ebb-41af20874ec1
⛔ Files ignored due to path filters (1)
plugins/awos/skills/ai-readiness-audit/dist/cli.jsis excluded by!**/dist/**
📒 Files selected for processing (18)
.claude-plugin/marketplace.jsonplugins/awos/.claude-plugin/plugin.jsonplugins/awos/commands/flow.mdplugins/awos/skills/ai-readiness-audit/SKILL.mdplugins/awos/skills/ai-readiness-audit/artifact_types.tsplugins/awos/skills/ai-readiness-audit/audit_core.tsplugins/awos/skills/ai-readiness-audit/collectors/incidents.tsplugins/awos/skills/ai-readiness-audit/dimensions/delivery-flow.mdplugins/awos/skills/ai-readiness-audit/metrics/mttr.tsplugins/awos/skills/ai-readiness-audit/output-format.mdplugins/awos/skills/ai-readiness-audit/references/connector-shapes.mdplugins/awos/skills/ai-readiness-audit/references/standards.tomlplugins/awos/skills/ai-readiness-audit/render.tsplugins/awos/skills/ai-readiness-audit/tests/derived-delivery.test.tsplugins/awos/skills/ai-readiness-audit/tests/det-incidents-mttr.test.tsplugins/awos/skills/ai-readiness-audit/tests/incidents-collector.test.tsplugins/awos/skills/ai-readiness-audit/tests/met-mttr-incidents.test.tstests/lint-prompts.test.js
🚧 Files skipped from review as they are similar to previous changes (5)
- plugins/awos/skills/ai-readiness-audit/dimensions/delivery-flow.md
- tests/lint-prompts.test.js
- plugins/awos/skills/ai-readiness-audit/artifact_types.ts
- plugins/awos/skills/ai-readiness-audit/references/connector-shapes.md
- plugins/awos/skills/ai-readiness-audit/audit_core.ts
| ``` | ||
|
|
||
| - `headline` — the executive band. Transcribe values verbatim from the dimension checks (cite the `check_id`); never invent numbers. Row 1 of the headline (capability Points + Coverage cap-score block) is emitted by the renderer directly from `audit_total`/`coverage` — do not add it as a `delivery[]` entry, and the two connector-gated rows (Cycle time In-Progress→Done, MTTR) are computed by the engine from the tracker artifact and appended by the renderer — do not author them either (an authored gated row is ignored). `delivery[]` carries only rows 2–7, each a `DeliveryMetric` object `{label, display_value?, band?, check_id?}`. Author them in this order, reading DORA bands from each check's `hint` field ("DORA-banded (high)"), and transcribing all values verbatim — never invent numbers: | ||
| - `headline` — the executive band. Transcribe values verbatim from the dimension checks (cite the `check_id`); never invent numbers. Row 1 of the headline (capability Points + Coverage cap-score block) is emitted by the renderer directly from `audit_total`/`coverage` — do not add it as a `delivery[]` entry, and the two connector-gated rows (Cycle time In-Progress→Done from the tracker artifact, MTTR from the incidents artifact) are computed by the engine and appended by the renderer — do not author them either (an authored gated row is ignored). `delivery[]` carries only rows 2–7, each a `DeliveryMetric` object `{label, display_value?, band?, check_id?}`. Author them in this order, reading DORA bands from each check's `hint` field ("DORA-banded (high)"), and transcribing all values verbatim — never invent numbers: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Replace the nonexistent Step 6.4 reference.
Both changed prompt documents refer to SKILL.md Step 6.4, but the current workflow places report authoring in Step 5, item 4. This can direct the orchestrator to the wrong section.
plugins/awos/skills/ai-readiness-audit/SKILL.md#L170-L170: changeSKILL.md Step 6.4toSKILL.md Step 5.4.plugins/awos/skills/ai-readiness-audit/output-format.md#L77-L77: changeSKILL.md Step 6.4toSKILL.md Step 5.4.
As per coding guidelines, “Treat framework and plugin markdown files as prompts: prioritize clarity, structure, and explicit role, task, and process sections.”
📍 Affects 2 files
plugins/awos/skills/ai-readiness-audit/SKILL.md#L170-L170(this comment)plugins/awos/skills/ai-readiness-audit/output-format.md#L77-L77
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/awos/skills/ai-readiness-audit/SKILL.md` at line 170, Update the
nonexistent “SKILL.md Step 6.4” reference to “SKILL.md Step 5.4” in
plugins/awos/skills/ai-readiness-audit/SKILL.md at line 170 and
plugins/awos/skills/ai-readiness-audit/output-format.md at line 77, ensuring
both prompt documents point to the report-authoring workflow in Step 5, item 4.
Source: Coding guidelines
|
|
||
| - `headline` — the executive band. Transcribe values verbatim from the dimension checks (cite the `check_id`); never invent numbers. Row 1 of the headline (capability Points + Coverage cap-score block) is emitted by the renderer directly from `audit_total`/`coverage` — do not add it as a `delivery[]` entry, and the two connector-gated rows (Cycle time In-Progress→Done, MTTR) are computed by the engine from the tracker artifact and appended by the renderer — do not author them either (an authored gated row is ignored). `delivery[]` carries only rows 2–7, each a `DeliveryMetric` object `{label, display_value?, band?, check_id?}`. Author them in this order, reading DORA bands from each check's `hint` field ("DORA-banded (high)"), and transcribing all values verbatim — never invent numbers: | ||
| - `headline` — the executive band. Transcribe values verbatim from the dimension checks (cite the `check_id`); never invent numbers. Row 1 of the headline (capability Points + Coverage cap-score block) is emitted by the renderer directly from `audit_total`/`coverage` — do not add it as a `delivery[]` entry, and the two connector-gated rows (Cycle time In-Progress→Done from the tracker artifact, MTTR from the incidents artifact) are computed by the engine and appended by the renderer — do not author them either (an authored gated row is ignored). `delivery[]` carries only rows 2–7, each a `DeliveryMetric` object `{label, display_value?, band?, check_id?}`. Author them in this order, reading DORA bands from each check's `hint` field ("DORA-banded (high)"), and transcribing all values verbatim — never invent numbers: | ||
| 1. **Merges** — put the unit in the value, not the label: `label: "Merges"`, `display_value` from `report-context` → `window_stats.merges_per_active_per_week` rendered as a per-week rate `"<n> / week (per active contributor)"` (e.g. `"1.5 / week (per active contributor)"`); no `band`; no `check_id`; source: git artifact. If the value is null (zero active contributors), omit `display_value`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep the Merges display format consistent.
Line 171 requires "<n> / week (per active contributor)", but output-format.md Line 85 still shows "3.2 / active contributor". Update one document so both prompts define the same format.
As per coding guidelines, “Treat framework and plugin markdown files as prompts: prioritize clarity, structure, and explicit role, task, and process sections.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/awos/skills/ai-readiness-audit/SKILL.md` at line 171, Align the
Merges display format in the relevant documentation so it consistently uses "<n>
/ week (per active contributor)" everywhere, including the output-format
guidance and the SKILL.md prompt. Preserve the existing null-value omission
behavior and all other metric requirements.
Source: Coding guidelines
|
Thanks — right call to push on this. All landed in 593e7c0, and I bumped the plugin to 2.4.4 (marketplace + plugin.json + the flow.md generator stamp + pin) as you flagged. The core one: the engine now derives the MTTR median itself from CodeRabbit's three nitpicks folded in too: the shared measurable-incident predicate ( Engine 1343/0, lint 148/0, dist rebuilt. |
2 participants
What
Makes the audit's
incidentssource real so MTTR (DF-07 / category 1103) is measured from actual incidents instead of only a git proxy. Git recovery signals were already covered (change_failure_rate+ the existing git-proxy MTTR); the gap was ingesting a real incident source — thesources = ["tracker", "incident"]slot instandards.tomlhad no collector, so 1103 never scored.Roadmap item: Audit deterministic incidents collector (Improvement).
How
collectors/incidents.ts(new) — connector-passed, mirroringtracker/ci/docs. The orchestrator fetches incidents from a real source — PagerDuty/OpsGenie/incident.io, Statuspage/Atlassian, or code-host incident labels — normalizes them toIncidentRecord[], and passes them; the collector computes per-incident recovery spans and their median. No connector →available:false(SKIP), so git-only repos are unaffected.metrics/mttr.ts— a real-source branch takes precedence (median recovery time, reliabilitymaximal, awards 1103 when ≥1 incident is resolved). The git-proxy path is preserved unchanged as the fallback.references/connector-shapes.md— the Incidents recipe:IncidentRecord/IncidentsConnectorshapes + per-platform mapping. Maps semantically from the live tool response (open time →started_at, restore →resolved_at); vendor field names are typical-as-of-writing hints only.SKILL.mdalready listsincidentamong fetchable connectors.audit_corebase-pass collection +has_incident_source(a resolved incident, or a tracker incident source — the.rawread is also corrected);artifact_typesregisters theincidentssource;standards.toml+dimensions/delivery-flow.mdupdate DF-07.Design notes for review
available:false→ git proxy → DF-07 stays SKIP). Only a repo with a connected incident source changes — which is the point.has_incident_sourcepreviously readtrackerArt.incident_source(top-level, alwaysundefined); corrected to.raw.incident_source. A tracker-declared incident source now actually flips the flag.Testing
tests/incidents-collector.test.ts— connector shaping (spans, median, open-incident exclusion, SKIP-without-connector).tests/met-mttr-incidents.test.ts— the real-source metric branch + git-proxy fallback preserved.tests/det-incidents-mttr.test.ts— end-to-end:audit-core(DF-07 SKIP) → write connector-shapedincidents.json→enrich→ DF-07 becomes measured 2h / awarded / maximal.connector-shapes.mdpinned to the collector's TS interfaces.dist/rebuilt & committed.Follow-up (separate)
Extending the onboarding
configure-external-sourcesskill with an "incident management" category (to wire up a PagerDuty/OpsGenie MCP the audit then auto-detects) — intentionally out of scope here; the audit assesses reachable connectors, it doesn't configure them.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation