From b4e2a3132af2f5f0675d4566593c85ef80db4fa6 Mon Sep 17 00:00:00 2001 From: Shahzaib Jameel Date: Thu, 9 Jul 2026 16:05:42 -0700 Subject: [PATCH] s360-reporter: fix broken item links and enforce full pagination MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes AB#3683197 — two first-user bugs reported by Sowmya Malayanur (Jun 2026). Bug 1 — Broken item links - generate-report.js rendered every item title as an anchor around item.s360Url unconditionally, so a missing S360 URL shipped as literal href="undefined" (404 on click). - Added safeHref() and linkOrText() helpers: only wrap the title in an anchor when s360Url is a non-empty http(s) URL. Otherwise render plain text so the reviewer notices before sending. - Added a red banner and stderr WARN listing every item that ended up without a link, so a broken run is visible in-report rather than shipping silently. Bug 2 — Incomplete item coverage - Pagination was only a manual instruction in SKILL.md, so a first-time user could ship the report using only page 1 of the MCP response (up to 50 items). - New fetch-items.js consolidator loops through paginated response files and hard-fails if the last page still has a populated nextCursor. - merge-items.js now refuses to run against any input JSON that still has a populated nextCursor (defense-in-depth). - SKILL.md Steps 1a/1b/1c updated to require fetch-items.js and document both guards. Self-check (AC#3) - Coverage summary printed to stderr on every run (item counts by SLA plus resolved and new-this-week counts). - In-report banner + stderr WARN when any item has no valid link. Verified end-to-end against a synthetic fixture: - fetch-items.js exits 1 when last page has nextCursor; exits 0 when exhausted. - merge-items.js exits 1 on unspent-cursor input. - generate-report.js emits banner and WARN for the broken URLs, valid URLs still render as anchors, and no href="undefined" or href="null" appears in the output HTML. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/skills/s360-reporter/SKILL.md | 38 +++++++ .github/skills/s360-reporter/fetch-items.js | 105 ++++++++++++++++++ .../skills/s360-reporter/generate-report.js | 77 ++++++++++++- .github/skills/s360-reporter/merge-items.js | 15 +++ 4 files changed, 232 insertions(+), 3 deletions(-) create mode 100644 .github/skills/s360-reporter/fetch-items.js diff --git a/.github/skills/s360-reporter/SKILL.md b/.github/skills/s360-reporter/SKILL.md index 198e1d82..8a72c814 100644 --- a/.github/skills/s360-reporter/SKILL.md +++ b/.github/skills/s360-reporter/SKILL.md @@ -143,6 +143,25 @@ request: { If more than 50 items, paginate using the `nextCursor` field. +**Pagination is mandatory** — save each page's raw MCP response to a separate +file (e.g. `s360-service-page1.json`, `s360-service-page2.json`, ...) and keep +calling with the returned cursor until `nextCursor` is empty. Then consolidate +with `fetch-items.js`: + +```powershell +node .github/skills/s360-reporter/fetch-items.js ` + --input "$env:TEMP\s360-service-page1.json" "$env:TEMP\s360-service-page2.json" ` + --output "$env:TEMP\s360-service.json" +``` + +`fetch-items.js` exits non-zero if the last input page still has `nextCursor` +set. This is a defense against the first-user bug reported in AB#3683197 +where a single-page fetch silently dropped every item past page 1. + +If your first page's response has no `nextCursor`, you can pass it directly as +`--input` (single file) — the consolidator will still verify pagination +completed and produce the same output shape. + #### 1b: Person-targeted items Using the aliases discovered in Step 0, call `mcp_s360-breeze-m_search_active_s360_kpi_action_items` @@ -158,6 +177,11 @@ request: { This captures person-targeted items like on-call readiness checklists and certifications that are tied to individuals rather than service tree IDs. +**Same pagination rule applies** — the person query also paginates at +`pageSize=50`. Save each page separately and consolidate with +`fetch-items.js` before running the merger. `merge-items.js` refuses to run +against a JSON file that still has a populated `nextCursor` (see 1c). + **Important**: The `assignedTo` search returns ALL items for those aliases across Microsoft, including items from other team memberships. After fetching, filter results to only include items where one of these conditions is met: @@ -198,6 +222,11 @@ The script accepts MCP envelopes (`{ result: { resources: [...] } }`), trimmed envelopes (`{ resources: [...] }`), or bare arrays for the `--service` / `--person` inputs. +**Coverage guard**: `merge-items.js` refuses to run if either input still has a +populated `nextCursor` — this is defense-in-depth against skipping the +pagination loop in 1a/1b. If it errors, go back and consolidate with +`fetch-items.js`. + **Filter logic** (enforced by the script — do not duplicate ad-hoc): - `TargetType == "Person"` AND `TargetId` is a team alias → keep - `TargetId` is one of the three service tree GUIDs → keep @@ -774,6 +803,15 @@ Write a JSON file to a temp location (e.g., `$env:TEMP/s360_data.json`) with thi - `pbi`: This field is the ADO work item ID and accepts **either a PBI or a Bug** ID (the generator's `pbiUrl()` builds a type-agnostic ADO URL that works for both). When the matched item is a Bug, still put its ID here — do not leave it null. +- `s360Url`: The item's title-link target. Populated from the S360 API `URL` + field (remediation / action link — may be `aka.ms/…`, IcM URL, or an ADO + work-item URL). May legitimately be missing for some KPIs. The generator + validates it — non-empty `http(s)` URLs render as an anchor; anything else + (null, empty, `undefined`, malformed) renders as plain text and the item is + logged to a `⚠ N item(s) have no S360 link` banner at the top of the report + and to stderr. **Do not** substitute a placeholder like `https://s360.msftcloudes.com/` + just to get an anchor — plain text is the correct fallback so the human + reviewer notices the missing link before sending. #### 5b: Run the generator diff --git a/.github/skills/s360-reporter/fetch-items.js b/.github/skills/s360-reporter/fetch-items.js new file mode 100644 index 00000000..f2e7bd57 --- /dev/null +++ b/.github/skills/s360-reporter/fetch-items.js @@ -0,0 +1,105 @@ +#!/usr/bin/env node +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. +// +// fetch-items.js +// ---------------------------------------------------------------------------- +// Consolidates one or more paginated `search_active_s360_kpi_action_items` MCP +// responses into a single JSON file and verifies pagination was completed. +// +// Background: the S360 MCP paginates at pageSize=50 and returns a `nextCursor` +// on every page except the last. Before this script existed, SKILL.md relied +// on a manual "loop until nextCursor is empty" instruction — a first-time user +// (Sowmya Malayanur, Jun 2026) missed it and shipped an incomplete report. +// This script + the guard in merge-items.js make that failure mode loud. +// +// Usage: +// node fetch-items.js --input page1.json page2.json [pageN.json ...] \ +// --output consolidated.json +// +// Each input file must be one of: +// • Full MCP envelope: { result: { resources: [...], nextCursor?: "..." } } +// • Mid envelope: { resources: [...], nextCursor?: "..." } +// • Bare array: [...] (assumed to be a complete final page) +// +// Exit behavior: +// • Exits non-zero if the LAST input page still has a non-empty nextCursor. +// The user must fetch the next page(s) and re-run. +// • Emits a coverage summary to stderr (per-page counts + total). +// +// The output JSON is a bare array of items — same shape merge-items.js +// already accepts. + +'use strict'; + +const fs = require('fs'); +const path = require('path'); + +// ── CLI args ────────────────────────────────────────────────────────────────── +const args = process.argv.slice(2); +const inputs = []; +let outputPath = null; +for (let i = 0; i < args.length; i++) { + if (args[i] === '--output') { + outputPath = args[++i]; + } else if (args[i] === '--input') { + while (i + 1 < args.length && !args[i + 1].startsWith('--')) { + inputs.push(args[++i]); + } + } +} + +if (inputs.length === 0) { + console.error('Usage: node fetch-items.js --input [page2.json ...] --output '); + console.error(''); + console.error('Save each MCP `search_active_s360_kpi_action_items` response to a separate file,'); + console.error('then pass them all in page order. This script verifies pagination completed.'); + process.exit(2); +} + +// ── Envelope unwrap ─────────────────────────────────────────────────────────── +// Returns { resources, nextCursor } regardless of envelope shape. A bare array +// input is treated as a complete final page (no nextCursor). +function unwrap(p) { + const j = JSON.parse(fs.readFileSync(p, 'utf8')); + if (Array.isArray(j)) return { resources: j, nextCursor: null }; + if (j && Array.isArray(j.resources)) return { resources: j.resources, nextCursor: j.nextCursor || null }; + if (j && j.result && Array.isArray(j.result.resources)) { + return { resources: j.result.resources, nextCursor: j.result.nextCursor || null }; + } + throw new Error(`Could not find a resources array in ${p}. Expected a top-level array, { resources: [...] }, or { result: { resources: [...] } }.`); +} + +// ── Consolidate ─────────────────────────────────────────────────────────────── +const all = []; +let lastCursor = null; +for (let i = 0; i < inputs.length; i++) { + const { resources, nextCursor } = unwrap(inputs[i]); + const isLast = i === inputs.length - 1; + console.error(`Page ${i + 1} (${path.basename(inputs[i])}): ${resources.length} items${nextCursor ? ` [nextCursor present]` : ''}`); + all.push(...resources); + if (isLast) lastCursor = nextCursor; +} + +// ── Hard-fail if the caller forgot to fetch every page ──────────────────────── +if (lastCursor) { + console.error(''); + console.error('ERROR: pagination incomplete.'); + console.error(`The last input page (${path.basename(inputs[inputs.length - 1])}) still has a nextCursor set.`); + console.error(`Call \`mcp_s360-breeze-m_search_active_s360_kpi_action_items\` again with cursor="${lastCursor}",`); + console.error('save the response as the next page file, then re-run this script with the additional --input file.'); + console.error(''); + console.error('Not enforcing this is how S360 reports have silently under-counted before (AB#3683197).'); + process.exit(1); +} + +console.error(`Consolidated ${all.length} items across ${inputs.length} page(s). Pagination complete (last nextCursor is empty).`); + +// ── Write ───────────────────────────────────────────────────────────────────── +const out = JSON.stringify(all, null, 2); +if (outputPath) { + fs.writeFileSync(outputPath, out); + console.error(`Wrote ${all.length} items to ${outputPath}`); +} else { + process.stdout.write(out + '\n'); +} diff --git a/.github/skills/s360-reporter/generate-report.js b/.github/skills/s360-reporter/generate-report.js index 664f7131..2be3f798 100644 --- a/.github/skills/s360-reporter/generate-report.js +++ b/.github/skills/s360-reporter/generate-report.js @@ -77,6 +77,37 @@ function esc(s) { return s.replace(/&/g, '&').replace(//g, '>').replace(/"/g, '"'); } +// ── Link safety ─────────────────────────────────────────────────────────────── +// safeHref returns a rendered href attribute value only when the input is a +// non-empty http(s) URL. Anything else (null, undefined, empty string, the +// literal string "undefined", a bare path, javascript:, mailto:, etc.) returns +// null so callers can fall back to plain text. +// +// This exists because before this fix, `` rendered +// as `href="undefined"` when the S360 API's URL field was missing — producing +// visibly broken links on hover and 404s on click. (Reported by first user +// Sowmya Malayanur, Jun 2026; PBI AB#3683197.) +function safeHref(url) { + if (typeof url !== 'string') return null; + const s = url.trim(); + if (!s || s.toLowerCase() === 'undefined' || s.toLowerCase() === 'null') return null; + if (!/^https?:\/\//i.test(s)) return null; + return s; +} + +// linkOrText wraps `innerHtml` in an anchor when the URL is safe, otherwise +// returns the inner HTML unwrapped. Callers pass the CSS for the anchor. +function linkOrText(url, innerHtml, anchorStyle) { + const href = safeHref(url); + if (!href) return innerHtml; + return `${innerHtml}`; +} + +// Track items whose title link had to be rendered as plain text because the +// S360 API returned no usable URL. Surfaced in the self-check banner + stderr +// so a broken run is visible in-report instead of shipping silently. +const itemsWithoutLinks = []; + function slaStyle(sla) { if (sla === 'OutOfSla') return { rowBg: '#fff5f5', rowBgAlt: '#fff5f5', leftBorder: '#cf222e', @@ -175,7 +206,8 @@ html += ` Services: AuthN SDK - MSAL Android • AuthN SDK - ADAL Android • Microsoft Authenticator - Android

-`; + +`; // Summary cards const cards = [ @@ -314,7 +346,10 @@ if (outOfSlaItems.length > 0) {

- ${esc(item.title)} + ${(() => { + if (!safeHref(item.s360Url)) itemsWithoutLinks.push({ title: item.title, ownerAlias: item.ownerAlias }); + return linkOrText(item.s360Url, esc(item.title), 'color:#1a1a1a; text-decoration:none;'); + })()}

@@ -388,7 +423,10 @@ sortedPrograms.forEach(([progName, prog]) => { html += ` @@ -516,6 +554,39 @@ html += ` `; // ── Output ──────────────────────────────────────────────────────────────────── + +// Self-check: surface any items whose title link had to be rendered as plain +// text. If items are missing links, prepend an in-report banner so the human +// reviewer sees it immediately rather than shipping a broken-looking report. +if (itemsWithoutLinks.length > 0) { + const preview = itemsWithoutLinks.slice(0, 5).map(i => `${esc(i.title)}${i.ownerAlias ? ` (${esc(i.ownerAlias)})` : ''}`).join(' • '); + const more = itemsWithoutLinks.length > 5 ? ` • +${itemsWithoutLinks.length - 5} more` : ''; + const banner = ` + + +`; + html = html.replace('', banner); + console.error(`WARN: ${itemsWithoutLinks.length} item(s) have no valid S360 link (rendered as plain text):`); + itemsWithoutLinks.forEach(i => console.error(` - ${i.title}${i.ownerAlias ? ` (${i.ownerAlias})` : ''}`)); +} else { + html = html.replace('', ''); +} + +// Coverage summary — helps the human running the skill eyeball item count vs +// the S360 portal without having to hunt through the report. +console.error(`Coverage summary: ${total} active items rendered (${outCount} out-of-SLA, ${nearCount} approaching, ${inCount} in SLA, ${noEta} missing ETA). ${resolved.length} resolved. ${newItems.length} new this week.`); + if (outputPath) { fs.writeFileSync(outputPath, html, 'utf8'); console.log('Report saved to ' + outputPath); diff --git a/.github/skills/s360-reporter/merge-items.js b/.github/skills/s360-reporter/merge-items.js index 419d1699..741e0d94 100644 --- a/.github/skills/s360-reporter/merge-items.js +++ b/.github/skills/s360-reporter/merge-items.js @@ -67,6 +67,21 @@ if (!servicePath || !personPath || !teamPath) { // ── Load inputs ─────────────────────────────────────────────────────────────── function loadResources(p) { const j = JSON.parse(fs.readFileSync(p, 'utf8')); + // Coverage guard: refuse to proceed if the input still carries an unspent + // nextCursor. A populated nextCursor means the caller only fetched one page + // — merging that partial data into the report silently under-counts items. + // This is the failure mode reported in AB#3683197 (first-user bug, Jun 2026). + // Use fetch-items.js to consolidate all pages before running the merger. + const cursor = (j && j.nextCursor) || (j && j.result && j.result.nextCursor) || null; + if (cursor) { + throw new Error( + `Refusing to merge ${p}: input still has a populated nextCursor ("${cursor}"). ` + + `This means only one page (up to pageSize=50) was fetched — the S360 items past that page ` + + `would be silently dropped from the report. Fetch every page via ` + + `\`mcp_s360-breeze-m_search_active_s360_kpi_action_items\` (looping until nextCursor is empty) ` + + `and consolidate with fetch-items.js before running the merger.` + ); + } if (Array.isArray(j)) return j; if (j && j.resources && Array.isArray(j.resources)) return j.resources; if (j && j.result && j.result.resources && Array.isArray(j.result.resources)) return j.result.resources;
- ${esc(it.shortTitle || it.title)}${it.subtitle ? `
${esc(it.subtitle)}` : ''} + ${(() => { + if (!safeHref(it.s360Url)) itemsWithoutLinks.push({ title: it.title, ownerAlias: it.ownerAlias }); + return linkOrText(it.s360Url, esc(it.shortTitle || it.title), 'color:#24292f; font-weight:600; text-decoration:none;'); + })()}${it.subtitle ? `
${esc(it.subtitle)}` : ''} 		${esc(it.service)} ${esc(it.ownerName)}
(${esc(it.ownerAlias)})
+ + + + + +
  +

⚠ ${itemsWithoutLinks.length} item(s) have no S360 link

+

The S360 API returned no usable URL for these items — their titles render as plain text. Investigate before sending this report externally.

+

${preview}${more}

+
+