Repository navigation
Add scrum-master scenario: Scrum Master autopilot using Node.js SDK - #334
keshav Keshari (keshavk-msft) wants to merge 13 commits into
Conversation
Scenario extension on top of the OpenAI + Agent 365 base sample: an autonomous Scrum Master that runs standups, reconciles the Jira board, chases blockers via MCP Calendar, warns on sprint risk, answers grounded Q&A, and posts a management-ready sprint close report. Runs out of the box with JIRA_MODE=mock (no Atlassian account required). Live mode uses Jira Cloud REST + delegated Microsoft Graph (Sites.ReadWrite.All + Sites.Manage.All) via device-code flow. * nodejs / Microsoft Agent 365 SDK + OpenAI Agents SDK + MCP tools * Adaptive Card DMs, per-user rolling context, deterministic classifier * Sibling azure-functions/ package hosts the nightly/weekly timers
Enables reviewers to try live mode (JIRA_MODE=live) without hand-crafting issues: * src/scripts/seed-jira-sample.ts — creates 2 stories + 5 sub-tasks and a future sprint on a pre-existing Jira Scrum project. Idempotent by summary. Reuses accountIds from team.sample.json so a single edit propagates through the whole seed. * src/scripts/sprint.sample.json — topology (edit to customise). * docs/sharepoint-schema.md — human-readable reference for every SMA_* list column, matching LIST_SCHEMAS in services/sharepoint.ts. * package.json — new npm script: seed:jira
Consolidates a single top-level H1 (was two) and reorders content for a first-time reader: intro paragraph -> features (7 MVPs) -> architecture diagram -> prerequisites -> quick start in mock mode -> full setup in live mode -> capability try-it table -> reference sections. Adds: * Mermaid architecture diagram * Quick start (mock mode, no external deps) as the fast path * Full configuration reference table with defaults and required-for * Try-each-capability table with trigger + expected + handler link * Link to docs/sharepoint-schema.md Drops: * Duplicated base-sample content (identity, install, typing, multi-message) now referenced via a link at the top * Redundant POC-flavoured framing
Additive-only improvements ported from patterns in the Chief-of-Staff sample (PR microsoft#333). Zero behaviour change to existing handlers. * src/util/logger.ts (new) — level-based logger with LOG_LEVEL, timestamped scope tags, and automatic secret redaction. Available for future use; existing console.log calls left in place. * src/util/httpLogger.ts (new) — global axios interceptor for outbound Jira / Graph / MCP tracing. Off by default, gated behind LOG_HTTP=true. * src/startup-check.ts (new) — one-shot boot banner printing every relevant env var with [MISSING] markers so misconfig surfaces before the first handler runs. * src/index.ts — wire installHttpLogging + printStartupBanner immediately after configDotenv(); add unhandledRejection and uncaughtException handlers so a stray connector 502 no longer tears down the scheduler. * docs/design.md — replace stub with full design doc: 12 numbered sections including per-flow mermaid sequence diagrams, determinism boundary, concurrency guarantees, and extension points. * README.md — trim architecture / reconcile rules / warn thresholds / calendar path (moved to design.md); add TOC, First live proof smoke test, Troubleshooting matrix, and Deploy to Azure sections. * .env.template — document LOG_LEVEL and LOG_HTTP toggles.
There was a problem hiding this comment.
Pull request overview
Adds a new Scrum Master autonomous scenario under scenarios/scrum-master/, built on the Microsoft Agent 365 SDK + OpenAI Agents SDK, with integrations for Jira (live + mock), SharePoint Lists state, proactive Teams messaging via Adaptive Cards, and optional Azure Functions timers to drive scheduled ceremonies.
Changes:
- Introduces the Scrum Master scenario agent (message routing, slash commands, standup/reconcile/chase/warn/sprint-summary flows) plus supporting services (Jira, SharePoint, Graph delegated auth, MCP Calendar).
- Adds provisioning + seed scripts for SharePoint lists, helper roster, and optional Jira sample data.
- Adds scenario packaging/config (ToolingManifest, env template, manifests, Azure Functions sibling project, build tooling).
Reviewed changes
Copilot reviewed 59 out of 63 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| scenarios/scrum-master/tsconfig.json | TypeScript compiler configuration for the scenario package. |
| scenarios/scrum-master/ToolingManifest.json | Declares MCP servers (Mail/Calendar) used by the scenario. |
| scenarios/scrum-master/src/util/logger.ts | Scenario-level log helper with redaction/truncation. |
| scenarios/scrum-master/src/util/httpLogger.ts | Optional global axios request/response tracing. |
| scenarios/scrum-master/src/token-cache.ts | In-memory token cache + token resolver for observability export. |
| scenarios/scrum-master/src/startup-check.ts | Startup banner summarizing key env configuration. |
| scenarios/scrum-master/src/services/team-roster.ts | SharePoint-backed team roster with cached ConversationReferences. |
| scenarios/scrum-master/src/services/sharepoint.ts | Graph CRUD wrapper for SharePoint lists + report upload helper. |
| scenarios/scrum-master/src/services/session-store.ts | In-memory standup session tracking for fan-in/cutoffs. |
| scenarios/scrum-master/src/services/proactive.ts | Proactive messaging helper (continueConversation wrapper). |
| scenarios/scrum-master/src/services/jira.ts | Jira client abstraction (live REST + mock implementation). |
| scenarios/scrum-master/src/services/jira-tool.ts | OpenAI Agents function-tools for grounded Jira Q&A. |
| scenarios/scrum-master/src/services/issue-labels.ts | Helpers to map Jira keys to “Task-N” labels and clean titles. |
| scenarios/scrum-master/src/services/helperMatcher.ts | Keyword-based helper matching for blocker chase flow. |
| scenarios/scrum-master/src/services/graph.ts | Delegated Graph auth via MSAL device-code + file token cache. |
| scenarios/scrum-master/src/services/calendar.ts | Calendar ops via MCP CalendarTools using a scenario-specific agent. |
| scenarios/scrum-master/src/scripts/team.sample.json | Sample team roster seed data (placeholder identities). |
| scenarios/scrum-master/src/scripts/sprint.sample.json | Sample sprint topology for optional Jira seeding. |
| scenarios/scrum-master/src/scripts/setup-sharepoint.ts | Idempotent SharePoint list + library provisioning script. |
| scenarios/scrum-master/src/scripts/seed-team.ts | Seeds TeamMembers list (optionally patches current user AAD id). |
| scenarios/scrum-master/src/scripts/seed-jira-sample.ts | Optional live Jira sample data seeding (stories/subtasks/sprint). |
| scenarios/scrum-master/src/scripts/seed-helper-roster.ts | Seeds HelperRoster list rows used by blocker chase. |
| scenarios/scrum-master/src/openai-config.ts | OpenAI/Azure OpenAI client configuration wiring for agents SDK. |
| scenarios/scrum-master/src/mock/jira-mock.ts | In-memory mutable mock sprint for offline demo mode. |
| scenarios/scrum-master/src/index.ts | Express host + internal endpoints + scheduler kickoff. |
| scenarios/scrum-master/src/handlers/warn.ts | Nightly sprint risk detection + SM DM. |
| scenarios/scrum-master/src/handlers/sprint-summary.ts | T-2 mid-sprint markdown summary report builder + delivery. |
| scenarios/scrum-master/src/handlers/reconcile.ts | Deterministic status classifier + safe Jira transitions + SM approval card. |
| scenarios/scrum-master/src/handlers/config.ts | /config channel capture of channel ConversationReference. |
| scenarios/scrum-master/src/handlers/commands.ts | Slash-command router for scenario commands. |
| scenarios/scrum-master/src/handlers/answer.ts | Grounded Jira Q&A agent with per-user rolling history. |
| scenarios/scrum-master/src/cron/local-scheduler.ts | In-process node-cron scheduler for dev mode. |
| scenarios/scrum-master/src/config.ts | Central env-var parsing for scenario configuration. |
| scenarios/scrum-master/src/client.ts | OpenAI Agents + MCP tool registration + observability scope wrapper. |
| scenarios/scrum-master/src/cards/transition-confirm.card.ts | Adaptive Card for SM approval of non-safe Jira transitions. |
| scenarios/scrum-master/src/cards/standup-summary.card.ts | Adaptive Card for consolidated standup summary posting. |
| scenarios/scrum-master/src/cards/standup-request.card.ts | Adaptive Card DM for standup collection + “submitted” confirmation. |
| scenarios/scrum-master/src/cards/meeting-propose.card.ts | Adaptive Card for SM meeting-slot selection and booking. |
| scenarios/scrum-master/src/cards/blocker-escalation.card.ts | Adaptive Card DM escalating a blocker to the SM. |
| scenarios/scrum-master/src/agent.ts | AgentApplication wiring: message routing, card submits, notifications, token preload. |
| scenarios/scrum-master/package.json | Scenario package metadata, dependencies, and scripts. |
| scenarios/scrum-master/manifest/manifest.json | Teams manifest template for the scenario. |
| scenarios/scrum-master/manifest/agenticUserTemplateManifest.json | Agentic user template manifest for the scenario. |
| scenarios/scrum-master/docs/sharepoint-schema.md | Human-readable SharePoint list schema reference. |
| scenarios/scrum-master/azure-functions/tsconfig.json | TS config for the Azure Functions sibling project. |
| scenarios/scrum-master/azure-functions/src/index.ts | Azure Functions timers that POST to agent internal endpoints. |
| scenarios/scrum-master/azure-functions/README.md | Azure Functions setup/run/deploy documentation. |
| scenarios/scrum-master/azure-functions/package.json | Azure Functions package metadata and deps. |
| scenarios/scrum-master/azure-functions/local.settings.sample.json | Local settings template for Functions (callback URL + token). |
| scenarios/scrum-master/azure-functions/host.json | Azure Functions host configuration. |
| scenarios/scrum-master/azure-functions/.gitignore | Ignores dist, node_modules, and local.settings.json secrets. |
| scenarios/scrum-master/.gitignore | Ignores build output, env files, MSAL cache, and logs. |
| scenarios/scrum-master/.env.template | Comprehensive env template covering OpenAI, Agent 365, Jira, SharePoint, scheduling. |
Comments suppressed due to low confidence (2)
scenarios/scrum-master/src/util/httpLogger.ts:50
- The response log line includes
res.config.urlverbatim, which may contain query strings. Strip?…/#…before logging.
scenarios/scrum-master/src/util/httpLogger.ts:60 - The error log line includes
cfg?.urlverbatim, which may contain query strings. Strip?…/#…before logging.
Applies 7 of the 8 review comments; the 8th (client.ts:140 MCP transport close-in-finally) is deferred because it changes runtime behaviour on a codepath inherited from the base sample. * config.ts — default JIRA_MODE to 'mock' (was 'live') so a fresh clone runs offline in mock mode as the README documents, instead of throwing on missing Jira credentials. * index.ts — remove duplicate unhandledRejection/uncaughtException handlers (kept the more-detailed pair at top of file). * index.ts — emit a one-shot boot warning when INTERNAL_TRIGGER_TOKEN is empty so production deployments notice open /api/internal/* endpoints. * token-cache.ts — replace 'All rights reserved' header with the repo-standard MIT license header. * client.ts — rename observability service from 'TypeScript Claude Sample Agent' to 'Scrum Master Sample Agent'. * client.ts — fix fallback agentId/agentName (was 'typescript-compliance-agent' / 'TypeScript Compliance Agent'). * util/httpLogger.ts — add safePath() helper and strip query strings + URL fragments from request/response/error log lines so credentials in ?token=… never leak. * AGENT-CODE-WALKTHROUGH.md — sync doc snippet to new agentId strings.
|
Thanks copilot-pull-request-reviewer for the thorough pass. Pushed
Deferred: comment #7 (
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 59 out of 63 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (4)
scenarios/scrum-master/src/services/sharepoint.ts:127
- findByField() has the same issue as findByTitle(): URL-encoding the value inside the OData literal can break matches (e.g., spaces become %20). Encode the entire $filter expression and keep escapeODataString limited to OData single-quote escaping.
scenarios/scrum-master/src/client.ts:139 - invokeAgent() always closes MCP servers in finally. In this same scenario, calendar.ts documents that closing Streamable-HTTP MCP transports can break subsequent calls with "Session not found" because McpToolRegistrationService can reuse an already-closed transport. Consider not closing MCP servers here (or only closing on process shutdown) to avoid intermittent MCP failures across turns.
await this.closeServers();
scenarios/scrum-master/azure-functions/README.md:24
- This second link to
../openai/sample-agentis also incorrect for this scenario (the agent is..). Updating it avoids sending readers to a non-existent path.
- The agent already running somewhere reachable — either a dev tunnel URL, App Service, or Container Apps endpoint that exposes `/api/internal/*` from [`../openai/sample-agent`](../openai/sample-agent).
scenarios/scrum-master/src/index.ts:94
- When INTERNAL_TRIGGER_TOKEN is empty, /api/internal/* endpoints become unauthenticated even if the server is bound to 0.0.0.0 (non-development). Logging a warning is easy to miss in deployment logs; it's safer to reject requests unless you're explicitly in development mode.
| const graph = getGraphClient(); | ||
| const res = await graph | ||
| .api(`/sites/${siteId}/lists/${listName(key)}/items?$expand=fields&$filter=fields/Title eq '${escapeODataString(title)}'`) | ||
| .header('Prefer', 'HonorNonIndexedQueriesWarningMayFailRandomly') | ||
| .get(); |
| // inside our filter value as URL syntax. Our IDs use `#` as a separator (e.g. | ||
| // `<sprintId>#<yyyy-mm-dd>`), which without encoding gets treated as a URL fragment | ||
| // and truncates the filter — silently returning zero rows. | ||
| return encodeURIComponent(s.replace(/'/g, "''")); |
| export function getWarnConfig(): WarnConfig { | ||
| return { | ||
| todoPct: Number(optional('WARN_TODO_PCT', '0.40')), | ||
| sprintProgressPct: Number(optional('WARN_SPRINT_PROGRESS_PCT', '0.50')), | ||
| }; | ||
| } |
| scopes: getObservabilityAuthenticationScope() | ||
| }); | ||
|
|
||
| console.log(`Preloaded Observability token for agentId=${agentId}, tenantId=${tenantId} token=${aauToken?.token?.substring(0, 10)}...`); |
| const response = await client.invokeAgentWithScope( | ||
| `You have received the following email. Please follow any instructions in it. ${emailContent}` | ||
| ); |
| }, | ||
| "devDependencies": { | ||
| "@microsoft/m365agentsplayground": "^0.2.18", | ||
| "@types/express": "^4.17.21", |
| @@ -0,0 +1,121 @@ | |||
| # Scrum Master Assistant — Azure Functions | |||
|
|
|||
| Two timer-triggered functions that drive the SMA scheduled ceremonies. The agent process (in [`../openai/sample-agent`](../openai/sample-agent)) owns all state, tokens, and Adaptive Card logic — these functions only *nudge* it via HTTP. | |||
| /** | ||
| * Simple custom in-memory token cache with expiration handling | ||
| * In production, use a more robust caching solution like Redis | ||
| */ |
cce56fe to
aa27069
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 59 out of 63 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
scenarios/scrum-master/src/agent.ts:238
- The prompt
Please follow any instructions in itmakes the agent treat email content as executable instructions, which is a classic prompt-injection vector. Even with general safety guidance elsewhere, it’s better to frame email content as untrusted data and ask for summarization + a drafted response.
// Then process the email
const response = await client.invokeAgentWithScope(
`You have received the following email. Please follow any instructions in it. ${emailContent}`
);
scenarios/scrum-master/src/client.ts:199
closeServers()closes every MCP server after each agent invocation. This conflicts with the calendar MCP guidance inservices/calendar.ts/ README troubleshooting: streamable HTTP MCP sessions are intended to be long-lived, and closing can lead to-32001 Session not foundon subsequent turns when the registration service reuses a closed transport.
private async closeServers(): Promise<void> {
if (this.agent.mcpServers && this.agent.mcpServers.length > 0) {
for (const server of this.agent.mcpServers) {
await server.close();
}
}
}
| const internalToken = getInternalTriggerToken(); | ||
| if (!internalToken) { | ||
| // Loud one-shot warning so production deployments don't accidentally ship | ||
| // open internal endpoints. Dev convention still allows an empty token so a | ||
| // solo developer can curl the endpoints without setting a secret. | ||
| console.warn( | ||
| '[security] INTERNAL_TRIGGER_TOKEN is empty — /api/internal/* is UNAUTHENTICATED. ' + | ||
| 'Set INTERNAL_TRIGGER_TOKEN in .env before deploying anywhere non-local.', | ||
| ); | ||
| } | ||
| function requireInternalToken(req: express.Request, res: Response): boolean { | ||
| const provided = req.get('x-internal-token') ?? ''; | ||
| if (!internalToken) { | ||
| // Convention: empty token in .env => endpoint is open (dev-only). The one-shot | ||
| // warning above logs this condition; each request stays quiet. | ||
| return true; | ||
| } | ||
| if (provided !== internalToken) { | ||
| res.status(401).json({ error: 'invalid internal token' }); | ||
| return false; | ||
| } | ||
| return true; | ||
| } |
| const adapter = agentApplication.adapter as CloudAdapter; | ||
| const botAppId = getBotAppId(); | ||
| try { | ||
| // Cast: stored references always come from `activity.getConversationReference()` | ||
| // which returns a fully-populated ConversationReference, but our storage layer | ||
| // deserializes as `Partial<>`. Adapter throws at runtime on truly-incomplete refs. | ||
| await adapter.continueConversation(botAppId, reference as ConversationReference, async (context) => { | ||
| await logic(context); | ||
| }); |
| type: 'Input.Toggle', | ||
| id: `approve_${t.issueKey}`, | ||
| title: 'Approve this change', | ||
| value: 'true', |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 59 out of 63 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (6)
scenarios/scrum-master/src/agent.ts:191
- Logging even a token prefix can leak credentials via console logs (often shipped to centralized log stores). Avoid printing any part of the observability token; log only metadata like agentId/tenantId.
console.log(`Preloaded Observability token for agentId=${agentId}, tenantId=${tenantId} token=${aauToken?.token?.substring(0, 10)}...`);
scenarios/scrum-master/src/agent.ts:238
- This prompt tells the model to "follow any instructions" inside an email, which is a direct prompt-injection vector (email content is untrusted). Ask the model to summarize/extract action items instead, without executing instructions from the email body.
const response = await client.invokeAgentWithScope(
`You have received the following email. Please follow any instructions in it. ${emailContent}`
);
scenarios/scrum-master/src/client.ts:140
- This closes MCP server sessions after every run(). In this same scenario (see calendar.ts), closing Streamable-HTTP MCP transports can break subsequent tool calls with "Session not found" because McpToolRegistrationService may reuse the underlying transport across agents. Consider keeping MCP sessions long-lived here as well.
} finally {
await this.closeServers();
}
scenarios/scrum-master/src/index.ts:94
- If INTERNAL_TRIGGER_TOKEN is empty, /api/internal/* becomes unauthenticated for all environments (including production misconfig). It would be safer to only allow the "open" behavior in development and fail closed otherwise.
scenarios/scrum-master/azure-functions/README.md:3 - This README links to
../openai/sample-agent, but the agent process for this scenario is the parentscenarios/scrum-masterpackage (and its README). The current link is broken/misleading in this repo layout.
Two timer-triggered functions that drive the SMA scheduled ceremonies. The agent process (in [`../openai/sample-agent`](../openai/sample-agent)) owns all state, tokens, and Adaptive Card logic — these functions only *nudge* it via HTTP.
scenarios/scrum-master/azure-functions/README.md:24
- Same as above: this link points at
../openai/sample-agent, but the internal endpoints live in the parent scenario package. Update the reference so readers can find the right code/docs.
- The agent already running somewhere reachable — either a dev tunnel URL, App Service, or Container Apps endpoint that exposes `/api/internal/*` from [`../openai/sample-agent`](../openai/sample-agent).
| scope.recordInputTokens(45); | ||
| scope.recordOutputTokens(78); |
| console.log('[OpenAI Config] Using Azure OpenAI'); | ||
| console.log(`[OpenAI Config] Endpoint: ${process.env.AZURE_OPENAI_ENDPOINT}`); | ||
| console.log(`[OpenAI Config] Deployment: ${process.env.AZURE_OPENAI_DEPLOYMENT}`); |
The MCP calendar tool loop occasionally succeeds at creating the event but the LLM returns a garbled response (e.g. webLink="onlineMeetingUrl" as a literal string, or id="errorEvent verification failed..." from a mis-formed rule-microsoft#6 error path). The existing !out.webLink check misses this because the values are truthy strings that pass Zod validation. * services/calendar.ts — after the existing empty-webLink guard, check that webLink is actually a URL (^https?://) and that id doesn't start with "error". On mismatch, throw a distinct CALENDAR_VERIFY_FAILED: error with an actionable "verify in Outlook" message. * handlers/chase.ts — catch the CALENDAR_VERIFY_FAILED: prefix separately; mark the blocker as booked (event was almost certainly created) and DM the SM a warning that asks them to verify in Outlook, instead of the scary "couldn't book the meeting" error. Tested in the POC with real MCP calendar output that reproduced the shape described.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 59 out of 63 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (7)
scenarios/scrum-master/src/agent.ts:191
- This log line prints a prefix of the exchanged observability token. Even partial token material can be used for correlation/attacks and shouldn’t be emitted to logs.
console.log(`Preloaded Observability token for agentId=${agentId}, tenantId=${tenantId} token=${aauToken?.token?.substring(0, 10)}...`);
scenarios/scrum-master/src/agent.ts:238
- The email-notification flow explicitly instructs the model to “follow any instructions” found in the email content. That creates a prompt-injection path where untrusted email content can drive unintended tool use or actions. The agent should treat email as untrusted data: summarize it, extract requests, and ask for user confirmation before acting.
// Then process the email
const response = await client.invokeAgentWithScope(
`You have received the following email. Please follow any instructions in it. ${emailContent}`
);
scenarios/scrum-master/src/token-cache.ts:37
- The comments describe “expiration handling” and “store a token with expiration”, but the implementation is a plain Map with no TTL/expiry logic. This is misleading for readers and for anyone reusing the sample code.
scenarios/scrum-master/src/client.ts:199 - This closes all MCP server transports after every agent invocation. In this scenario, calendar.ts documents that closing a Streamable-HTTP MCP transport can break subsequent tool calls with “Session not found” when McpToolRegistrationService reuses the closed transport across turns. To avoid intermittent failures, consider not closing MCP servers here (or ensure each agent gets a non-shared transport instance).
private async closeServers(): Promise<void> {
if (this.agent.mcpServers && this.agent.mcpServers.length > 0) {
for (const server of this.agent.mcpServers) {
await server.close();
}
}
}
scenarios/scrum-master/src/handlers/report.ts:268
- The action-items table hardcodes the target sprint as "Sprint 2". That will be incorrect for most real sprint names; use a neutral label like "Next sprint" (or derive it from sprint metadata if available).
scenarios/scrum-master/src/handlers/chase.ts:17 - The header comment describes the blocker-chase flow as using direct Graph calls (/me/findMeetingTimes, POST /events). The implementation now drives mcp_CalendarTools via services/calendar.ts, so this comment is outdated/misleading.
scenarios/scrum-master/src/handlers/report.ts:260 - The action-items table hardcodes the target sprint as "Sprint 2". That will be incorrect for most real sprint names (and even for mock data) and can confuse readers; consider using a neutral label like "Next sprint".
This issue also appears on line 268 of the same file.
| const internalToken = getInternalTriggerToken(); | ||
| if (!internalToken) { | ||
| // Loud one-shot warning so production deployments don't accidentally ship | ||
| // open internal endpoints. Dev convention still allows an empty token so a | ||
| // solo developer can curl the endpoints without setting a secret. | ||
| console.warn( | ||
| '[security] INTERNAL_TRIGGER_TOKEN is empty — /api/internal/* is UNAUTHENTICATED. ' + | ||
| 'Set INTERNAL_TRIGGER_TOKEN in .env before deploying anywhere non-local.', | ||
| ); | ||
| } | ||
| function requireInternalToken(req: express.Request, res: Response): boolean { | ||
| const provided = req.get('x-internal-token') ?? ''; | ||
| if (!internalToken) { | ||
| // Convention: empty token in .env => endpoint is open (dev-only). The one-shot | ||
| // warning above logs this condition; each request stays quiet. | ||
| return true; | ||
| } | ||
| if (provided !== internalToken) { | ||
| res.status(401).json({ error: 'invalid internal token' }); | ||
| return false; | ||
| } | ||
| return true; | ||
| } |
Ports the observability pattern from PR microsoft#333 (Chief-of-Staff) so both scenarios share the same MAC Activity wiring convention. Also addresses the five outstanding Copilot review comments from Jul 27 and Jul 29. Observability: * src/observability.ts (new) — shared module: getAgentId (agentic instance), getAgentBlueprintId (metadata only), getTenantId, buildAgentDetails, buildRequest, ensureObservabilityToken (both identities), runWithObservabilityContext. Runtime agent id comes from activity.recipient.agenticAppId — not the blueprint id — which is what the exporter needs for spans to bind to MAC Activity. * src/agent.ts — call ensureObservabilityToken at the top of handleAgentMessageActivity so per-turn spans have identity. * src/services/proactive.ts — wrap continueConversation in runWithObservabilityContext so proactive DMs and channel posts carry the same tenant+agent baggage as inbound /api/messages turns. * .env.template — add agent365Observability__agentInstanceId with guidance distinguishing runtime instance vs blueprint id. Review-comment fixes: * src/index.ts — fail-shut on empty INTERNAL_TRIGGER_TOKEN when NODE_ENV=production (was warn-only). Keeps dev convention intact. * src/services/proactive.ts — getBotAppId throws with a clear message when connections__service_connection__settings__clientId is unset instead of letting continueConversation fail with an opaque error. * src/cards/transition-confirm.card.ts — approval toggles default to 'false' so risky/non-whitelisted transitions require explicit opt-in. * src/client.ts — remove hard-coded 45 / 78 input/output token counts that were leaking into telemetry. Instrumentor populates real values. * src/openai-config.ts — mask AZURE_OPENAI_ENDPOINT to scheme+host and log only [set]/(unset) for the deployment so internal resource names never appear in logs.
|
Updated in 1. Wired A365 observability (matches PR #333 pattern)Ran the
2. Addressed the 5 open review comments
Not yet in this commit
Verification pending on our tenant — will run the agent locally and confirm the boot log shows both identities being warmed. |
Two things landed after live-tenant verification with the a365-skills
'a365-code-validator' + 'instrument-observability' flow:
Observability (from running the instrument-observability skill against
the sample):
* src/observability.ts — add withInvokeAgentScope() and
withExecuteToolScope() helpers plus a per-turn AsyncLocalStorage so
tool definitions can pick up the current TurnContext without threading
it through every signature. buildUserDetails / buildCallerDetails
populate user.id / userName / userEmail (UPN only when the from.id
actually carries one). runWithTurnBaggage() replaces the old
runWithObservabilityContext export.
* src/agent.ts — hoist ensureObservabilityToken +
preloadObservabilityToken above the dispatch decision so cards,
slash commands, Answer, and the generic LLM path all run inside one
identity-tagged baggage scope. Same treatment for the
agent-notification (email) branch.
* src/client.ts — bind AgentDetails / Request via buildAgentDetails() /
buildRequest() from the shared module, so gen_ai.agent.id gets the
runtime agentic instance id rather than the blueprint id. Drops the
hard-coded conversationId literal.
* src/handlers/answer.ts — wrap the Q&A turn in withInvokeAgentScope so
MAC Activity gets an invoke_agent parent above the auto-instrumented
chat inference span.
* src/services/jira-tool.ts — wrap jira_get_issue,
jira_list_sprint_issues, jira_get_issue_comments in
withExecuteToolScope so tool calls become named execute_tool spans
bound to the agent identity.
* .env.template — small doc tweak clarifying the exporter default.
Bug fix (found while live-testing the Q&A path):
* src/services/issue-labels.ts — toJiraKey() checked the canonical
^PROJ-N regex before the taskish ^task-N regex, so 'Task-9' matched
the canonical branch and returned 'TASK-9' (a made-up project). Only
bare numbers reached the correct branch. Reorder so the task/# prefix
match runs first; canonical falls through only for real project keys
like 'DEMO-14' or 'EDP-10'.
Ignore hygiene:
* .gitignore — exclude TS compile artefacts under src/**/*.{js,d.ts,
js.map}, azure-functions/src/**/*.{js,d.ts,js.map}, and the workspace-
local skill install directories (.agents/, .claude/,
.github/copilot-instructions.md) so those don't accidentally join the
next commit.
Live-tenant verification highlights (agentId f5a59964-... on tenant
1fe41f4b-...):
* Boot: Agent365Exporter enabled: true, both blueprint + agentic-
instance identity tokens cached.
* MVP 1 standup: sentTo=4, per-card submit round-trip succeeded; span
export-group succeeded in ~1.3s (9/10 spans bound on Answer turn,
6/10 on standup fan-out).
* MVP 3 chase: end-to-end unblock meeting booked; 4/4 spans bound on
the calendar tool loop.
* MVP 4 warn / MVP 6 mid-sprint RAG / MVP 7 sprint close: all fired
through HTTP triggers, all export-group succeeded.
* MVP 5 Q&A: withInvokeAgentScope + withExecuteToolScope both fire and
bind. Task-9 lookup succeeds after the label fix.
* M365 admin center Agents view now shows the Scrum Master Blueprint
with a MarioR@... session on today's date.
|
Live-tenant verification complete. Ran the Observability instrumentation is now in placeThe skill flow surfaced 5 gaps in what we had before, all fixed in this commit:
Bug fix caught during live testing
Live-tenant verification resultsTenant
M365 admin center Agents view now shows the Scrum Master Blueprint with Known cosmetic warnings (not from this change, pre-existing)
Worth filing as follow-up issues on the samples repo but neither blocks this PR. Not verified in this session (deferred, not blocking)
|
Closes the 7 review threads still open after the observability wire-up landed: * src/config.ts - getWarnConfig() switches from Number(optional(...)) to the numeric() helper so non-numeric WARN_TODO_PCT / WARN_SPRINT_PROGRESS_PCT values fall back to defaults instead of silently returning NaN and disabling every risk check. * src/agent.ts - preloadObservabilityToken no longer logs a substring of the AAU token; the agentId + tenantId are still logged for correlation. Even partial tokens can aid replay. * src/agent.ts - handleEmailNotification's second LLM prompt no longer says "follow any instructions in it". Email bodies are untrusted user-controlled data; the new prompt asks the model to summarize and draft a response while treating the body strictly as data (no execution of instructions, no privileged actions). * src/services/sharepoint.ts - escapeODataString now only doubles single quotes (the OData string-literal rule). findByTitle / findByField URL-encode the full $filter query value so spaces stay real spaces inside the literal and URL syntax chars (#, ?, &, +) still survive transport. Previously URL-encoding was applied inside the literal, so titles containing spaces silently returned zero rows. * src/token-cache.ts - Docstring no longer claims "expiration handling". The class is a plain Map with no TTL / eviction; the comment now says so and points at Redis / App Service secrets cache as the production replacement. * package.json - @types/express bumped from ^4.17.21 to ^5.0.0 to match the runtime express ^5.1.0. * src/client.ts - invokeAgent() no longer closes MCP servers in finally. services/calendar.ts already documents that closing Streamable-HTTP MCP sessions returns -32001 "Session not found" on the next tool call when McpToolRegistrationService reuses the transport. Removed the unused closeServers() helper. * azure-functions/README.md - Both `../openai/sample-agent` links now point at `..` (the parent scenarios/scrum-master/ folder where the agent actually lives). The stale links were broken for anyone cloning from the samples repo.
|
Reviewed all 22 Copilot review comments across the five review passes. Full sweep resolved in Newly resolved in this commit
Already resolved by prior commits — verified in current tree
Verified locally
|
Limit the sample TypeScript build to src so the separate Azure Functions project is not compiled by the root configuration. Clarify SharePoint reuse and consent requirements, and document observability export, Defender collection, and inventory troubleshooting. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Validation evidence: Scrum Master scenario (A365 observability + Jira)1. E2E playbook (AutoQAgent): 5 / 5 passed
The user-provided completed playbook screenshot was inspected; the playbook was not independently rerun for this comment.
2. Defender advanced hunting: exported CloudAppEventsThe locally supplied Defender CSV contains 840 records, all with Workload = Agent365, across 2026-10-21 16:42:41 to 2026-10-22 09:36:56 as displayed in the CSV (timezone not specified).
SDK tool records: Each of the populated non-zero The 30-row playbook result and the 840-row CSV are different evidence sets; they should not be treated as the same query window or scope. 3. Purview DSPM for AI: Activity ExplorerThe locally supplied Activity Explorer CSV contains 27 AI Interaction records:
The diagnostic row is not presented as a normal user interaction. The 122 audit-search results in the playbook and these 27 Activity Explorer rows come from different queries/exports and are not expected to have identical totals. The CSV's 4. A365 code validator: PASS WITH RISKS (migration pending)
Important distinction: the successful playbook and exported telemetry demonstrate the currently working deployment. They do not establish compliance with the newer app-only-token/S2S export pattern. The coordinated resolver + S2S migration has not been applied or committed. No tenant permissions, blueprint, messaging endpoint, or workload authentication were changed during this validation. No Microsoft 365 admin-center Activity metrics are claimed here because that evidence was not supplied for this validation comment. Raw Defender/Purview CSVs and the original screenshot contain tenant/user identifiers and are intentionally not attached to this public comment. Only aggregate, redacted evidence is included. The exports cover deployed activity and are not an isolated regression run of every change in the linked commit. Linux deployment regression follow-up
|
|
Hi Krishnadheeraj (@DheerajPannala), |
Krishnadheeraj (DheerajPannala)
left a comment
Krishnadheeraj (DheerajPannala)
left a comment
There was a problem hiding this comment.
I would not merge it yet; the end-to-end behavior still needs to match what this package claims.
38 things to address before this merges; details inline.
| let transitions: ProposedTransition[] = []; | ||
| try { transitions = JSON.parse(String(value?.transitions ?? '[]')); } | ||
| catch { transitions = []; } | ||
|
|
||
| const approvedKeys = new Set<string>(); | ||
| for (const t of transitions) { | ||
| const approvedRaw = String(value?.[`approve_${t.issueKey}`] ?? 'false'); | ||
| if (approvedRaw === 'true') approvedKeys.add(t.issueKey); |
There was a problem hiding this comment.
Should fix: Authorize reconciliation approvals against server-side state
Sending the approval card to the Scrum Master does not authorize its subsequent submission. This handler accepts both the proposed transitions and approval flags from the caller without checking the caller's role or looking up a pending proposal. An authenticated non-SM can consequently request arbitrary transitions using the shared Jira credentials, including for issues outside the standup.
Evidence
scenarios/scrum-master/src/agent.ts:108-110 dispatches any reconcile.* submission to this handler. reconcile.ts:210 parses value.transitions, lines 215-216 trust approve_, and lines 248-251 pass the supplied issueKey and transitionId directly to Jira. For example, a non-SM submission with action='reconcile.apply', transitions='[{"issueKey":"DEMO-42","transitionId":"31"}]', and approve_DEMO-42='true' reaches transitionIssue without any caller, standup, or proposal lookup. services/jira.ts:85-90 configures the shared Basic-auth credentials.
Suggested fix: Require the authenticated caller to be the configured Scrum Master and resolve approved transitions from a stored pending proposal. Accept only identifiers and approval selections from the submitted card, not authoritative issue/transition pairs.
| if (!internalToken) { | ||
| // Fail-shut in production so open /api/internal/* can never ship by mistake. | ||
| if (process.env.NODE_ENV === 'production') { |
There was a problem hiding this comment.
Should fix: Fail closed outside explicitly enabled development mode
The missing-secret guard protects only the literal NODE_ENV=production case. With NODE_ENV unset or set to staging, the internal endpoints remain unauthenticated even though the rest of the server treats those environments as authenticated deployments. An unauthenticated caller can trigger standup fan-out, forced risk notifications, and reports.
Evidence
scenarios/scrum-master/src/index.ts:52-53 requires normal authentication unless NODE_ENV is development, but lines 79-98 allow an empty internal token in every environment except production. The internal routes are registered before authorizeJWT at line 165, and line 176 defaults non-development deployments to 0.0.0.0. With valid service configuration, NODE_ENV=staging, HOST unset, and INTERNAL_TRIGGER_TOKEN unset, an unauthenticated POST to /api/internal/nightly-check?force=warn&forceAlert=true passes requireInternalToken.
Suggested fix: Allow an empty internal token only when isDevelopment is true. Fail startup or reject internal requests in every other environment when the secret is missing.
| // Load roster and issues in parallel. | ||
| const [members, issues] = await Promise.all([ | ||
| listTeamMembers(), |
There was a problem hiding this comment.
Should fix: Provide an offline standup path instead of requiring SharePoint in mock mode
The documented mock quick start cannot run /standup with only the language-service credentials. Although Jira is mocked, this unconditional roster read still requires a SharePoint site and Graph sign-in. The command fails before generating any standup card, contradicting the primary offline onboarding path.
Evidence
README.md:52-76 describes the no-external-services setup and directs the user to run /standup. scenarios/scrum-master/src/handlers/standup.ts:120-123 calls listTeamMembers regardless of Jira mode. services/team-roster.ts:64-65 calls listItems; services/sharepoint.ts:133-136 calls getSiteId; getSiteId at line 37 calls getSharePointConfig. config.ts:79 requires SHAREPOINT_SITE_URL, and lines 12-15 throw 'Missing required env var: SHAREPOINT_SITE_URL' for the supplied empty default.
[grok-4.7] README.md:73 tells the playground user to DM /standup after copying .env.template and leaving JIRA_MODE=mock, and the PR description says this works without a SharePoint site. triggerStandup always calls listTeamMembers() (standup.ts:121). That calls listItems → getSharePointConfig() (sharepoint.ts:37), and required('SHAREPOINT_SITE_URL') (config.ts:79, config.ts:14) throws Missing required env var: SHAREPOINT_SITE_URL when the template value is empty. If the operator seeds src/scripts/team.sample.json instead, those rows use JiraAccountId values 557058:00000000-0000-0000-0000-000000000001 and siblings (team.sample.json:6), while mock assignees are acc-alex, acc-priya, acc-arjun, acc-sam, acc-chetan (jira-mock.ts:26). The join at standup.ts:128-132 skips every issue, so sentTo stays 0.
Suggested fix: Add an explicit offline standup path using an in-memory roster/state and the current Playground conversation. Keep Graph-backed roster, persistence, and proactive delivery behind the live configuration path.
| } catch (e) { | ||
| console.error('[cron] Standup failed:', (e as Error).message); | ||
| } | ||
| }, { timezone: cfg.timezone }); |
There was a problem hiding this comment.
Should fix: Interpret the UTC cron expressions in UTC
The configured cron expressions already encode UTC, but node-cron is told to interpret them in the display timezone. With the defaults, standup runs at 03:30 IST rather than 09:00 IST, and its four-hour cutoff expires at 07:30 before the documented standup time. The nightly job is likewise shifted to 19:00 IST instead of 00:30 IST.
Evidence
scenarios/scrum-master/src/config.ts:106-109 defaults to standupCron='0 30 3 * * 1-5', nightlyCron='0 0 19 * * *', and timezone='Asia/Kolkata'. local-scheduler.ts:44 and 52 pass that timezone to cron.schedule, so the hour fields are interpreted as IST. README.md's configuration table explicitly calls these UTC cron expressions and labels TIMEZONE as display-only; azure-functions/src/index.ts:31-34 also identifies 03:30 UTC as 09:00 IST.
[grok-4.7] Defaults are STANDUP_CRON=0 30 3 * * 1-5 and NIGHTLY_CRON=0 0 19 * * * (config.ts:106-107) with TIMEZONE=Asia/Kolkata (config.ts:109). README.md:218-219 call those expressions UTC (03:30 UTC = 09:00 IST, 19:00 UTC = 00:30 IST) and README.md:221 says TIMEZONE is display-only. cron.schedule is called with { timezone: cfg.timezone } (local-scheduler.ts:44 and local-scheduler.ts:52), so node-cron fires at 03:30 and 19:00 Asia/Kolkata, not 09:00 / 00:30 IST. Azure Functions uses the same strings as NCRONTAB (azure-functions/src/index.ts:35 and :56), which is UTC unless WEBSITE_TIME_ZONE is set. azure-functions/README.md:88 sets WEBSITE_TIME_ZONE='India Standard Time' while line 91 says that setting does not change the schedule; line 107 says the opposite. With that app setting, Functions also fire at 03:30 IST. Local cron and Functions therefore neither match each other under the documented defaults nor match the 09:00 IST claim.
Suggested fix: Schedule the existing expressions with timezone='UTC' and reserve TIMEZONE for display, matching the documented contract and Azure timer expressions.
| cron.schedule(cfg.nightlyCron, async () => { | ||
| console.log('[cron] Nightly tick — warn + report'); | ||
| try { await runWarnCheck(); } | ||
| catch (e) { console.error('[cron] warn failed:', (e as Error).message); } | ||
| try { await runSprintCloseReport(); } | ||
| catch (e) { console.error('[cron] report failed:', (e as Error).message); } |
There was a problem hiding this comment.
Minor: Wire the mid-sprint report into an automatic trigger
The T-2 report is implemented but never invoked by a scheduled path. This nightly callback runs only Warn and the close report, and the Azure nightly endpoint does the same. A correctly configured deployment therefore never posts the advertised automatic mid-sprint RAG report unless someone manually calls its separate endpoint.
Evidence
scenarios/scrum-master/src/cron/local-scheduler.ts:46-51 invokes only runWarnCheck and runSprintCloseReport. src/index.ts:118-149 likewise omits runSprintSummary from /api/internal/nightly-check. The only invocation of runSprintSummary is index.ts:157 in the manually addressed /api/internal/sprint-summary route, while azure-functions/src/index.ts:50-56 schedules only /api/internal/nightly-check. No scheduled caller reaches the T-2 gate in handlers/sprint-summary.ts:259-265.
[grok-4.7] README.md:35 says the RAG report fires two days before sprint end. sprint-summary.ts:16-17 says the nightly cron calls runSprintSummary() and only emits on sprintEnd-2. design.md:315 says the handler is called by the sprint-summary endpoint or the T-2 timer. The PR description says the Functions package includes mid-sprint timers. The nightly job only calls runWarnCheck and runSprintCloseReport (local-scheduler.ts:48-50). /api/internal/nightly-check does the same (index.ts:133 and index.ts:142) and never calls runSprintSummary (that call exists only on the manual route at index.ts:157). azure-functions/src/index.ts registers only StandupTimer and NightlyTimer, and NightlyTimer POSTs /api/internal/nightly-check. A deployed timer installation therefore never posts the mid-sprint table.
Suggested fix: Invoke the non-forced runSprintSummary from both nightly runners, or add the advertised dedicated timer. Preserve its date gate so ordinary nightly calls produce a report only at the intended time.
|
|
||
| ### Enable HTTP tracing | ||
|
|
||
| If a live call fails silently, set `LOG_HTTP=true` in `.env` and restart. Every outbound axios call prints `method host path status latency`. Credentials are redacted automatically. |
There was a problem hiding this comment.
Minor: LOG_HTTP=true troubleshooting step doesn't trace Jira calls: interceptors are on the default axios, Jira uses axios.create()
The troubleshooting section says LOG_HTTP=true prints every outbound call. .env.template:44-45 and README.md:207 go further and say it covers every Jira/Graph/MCP call.
- The tracer registers its interceptors on the default
axiosexport. - The Jira client is an
axios.create()instance, and axios instances get their own interceptor chains. So Jira calls, the documented use case, are never logged. - Graph (
@microsoft/microsoft-graph-clientover fetch) and MCP don't use axios at all.
As a result, the advice 'if a live call fails silently, set LOG_HTTP=true' gives the operator nothing beyond the 'outbound tracing enabled' banner.
Evidence
- README.md:292: 'set
LOG_HTTP=true… Every outbound axios call printsmethod host path status latency'. .env.template:44: 'prints every outbound Jira/Graph/MCP call'.- src/util/httpLogger.ts:33
axios.interceptors.request.use(...)and :43axios.interceptors.response.use(...)apply to the default instance only. - src/services/jira.ts:85:
this.http = axios.create({ baseURL: cfg.baseUrl, ... })is the scenario's only axios client at runtime. seed-jira-sample.ts:84 also usesaxios.create. - axios
create()builds a newAxioswith freshInterceptorManagers, so default-instance interceptors don't apply. - src/services/graph.ts:18/32 use isomorphic-fetch and
@microsoft/microsoft-graph-client.
Suggested fix: Attach the interceptors to the Jira instance, e.g. export attachHttpLogging(instance) from httpLogger and call it in LiveJiraClient's constructor. Then narrow the docs to 'Jira REST calls', since Graph and MCP aren't axios-based.
| - **`GRAPH_CLIENT_ID` defaults to the well-known "Microsoft Graph Command Line Tools" public client** for zero-setup device-code sign-in. Register your own multi-tenant public client for a real deployment. | ||
| - **Single-team by design.** The sample assumes one scrum team per process (single project key, single board, single channel). Multi-team support (per-team config, isolated Jira credentials, sharded timers) is called out as future work in [`docs/design.md#12-known-limitations--hardening-roadmap`](docs/design.md#12-known-limitations--hardening-roadmap). | ||
| - **Proactive DMs require prior interaction.** Every squad member must have said "hi" to the agent at least once so their conversation reference is captured in `SMA_TeamMembers`. | ||
| - **Running local `node-cron` and Azure Function timers simultaneously is safe** (`standupId = <sprintId>#<yyyy-mm-dd>` provides idempotency) but does two Jira reads per tick. Set `LOCAL_CRON=false` once the Function is deployed. |
There was a problem hiding this comment.
Minor: README says standup triggers are idempotent per day and safe to run with both schedulers, but the guard is in-memory and cleared after the summary
README.md:233 says the standup trigger is 'Idempotent per calendar day', and README.md:329 says running local node-cron and the Azure Function timers together 'is safe'.
The only guard is an in-memory lookup of the current standupId, and it doesn't last the day:
summarizeStandupevicts the session once the summary posts, on cutoff or when everyone has responded.- A process restart empties the map.
Any second trigger later the same UTC day re-sends every standup card and later posts a second summary. That trigger could be the other scheduler, a manual /standup, or the curl example. The README's own troubleshooting table (line 288) lists 'Duplicate standup summary cards' from exactly this setup.
Evidence
- standup.ts:107-117 is the only check:
const standupId = makeStandupId(sprint.id); ... const existingMem = getSession(standupId); if (existingMem) {...return}. - standup.ts:156-168 inserts the StandupSessions row without checking for an existing Title, and swallows errors.
- standup.ts:382 and :190-194 call
summarizeStandup, which ends withremoveSession(session.standupId)(standup.ts:524). - session-store.ts:54:
const sessions = new Map<...>(), process memory only. - session-store.ts:56-61: the standupId uses the UTC date.
Scenario:
- The SM runs
/standupat 09:05 and everyone answers by 10:00. - The summary posts and the session is evicted.
- The other scheduler, or a manual retry, fires later the same UTC day.
getSessionreturns undefined, so the cards fan out again and a second summary follows.
Suggested fix: Either make the guard durable, or correct the docs:
- Durable guard: check
findByTitle('standupSessions', standupId)before fanning out, and keep summarized sessions marked rather than evicted for the rest of the day. - Docs: say that re-triggering after a summary or a restart re-sends the cards, and recommend running only one scheduler.
| - **Running local `node-cron` and Azure Function timers simultaneously is safe** (`standupId = <sprintId>#<yyyy-mm-dd>` provides idempotency) but does two Jira reads per tick. Set `LOCAL_CRON=false` once the Function is deployed. | |
| - **Run only one scheduler.** The standup guard is in-memory (`standupId = <sprintId>#<UTC yyyy-mm-dd>`) and is cleared once the summary posts or the process restarts, so a second trigger later the same day re-sends the cards. Set `LOCAL_CRON=false` once the Function is deployed. |
| Minimum runtime configuration: | ||
|
|
||
| - Azure App Service (Linux, Node 20 LTS). Set `WEBSITES_PORT=3978`. | ||
| - Application Settings: all `.env` vars mapped one-to-one. |
There was a problem hiding this comment.
Minor: Deploy guidance maps dev-only .env values (NODE_ENV=development, HOST=127.0.0.1) to App Service
'Application Settings: all .env vars mapped one-to-one' copies the template's local-dev settings into Azure.
NODE_ENV=developmentis the switch the server uses to turn off JWT validation on/api/messages.- It also skips the fail-shut check for an empty
INTERNAL_TRIGGER_TOKEN. The template leaves that token blank, so/api/internal/*stays open. HOST=127.0.0.1binds the server to loopback, which the App Service front end can't reach.
The deploy section should list the production overrides explicitly.
Evidence
- README.md:301: '- Application Settings: all
.envvars mapped one-to-one.' .env.template:35NODE_ENV=development,.env.template:36HOST=127.0.0.1,.env.template:100INTERNAL_TRIGGER_TOKEN=.- src/index.ts:50-53: the comment '// Only NODE_ENV=development explicitly disables authentication', then
const authConfig = isDevelopment ? {} : loadAuthConfigFromEnv();. That config is passed toserver.use(authorizeJWT(authConfig))at :165. - src/index.ts:79-92 exits on an empty token only
if (process.env.NODE_ENV === 'production'); otherwiserequireInternalTokenreturns true (:95-98). - src/index.ts:176:
const host = process.env.HOST ?? (...), so the server binds 127.0.0.1 when HOST is mapped.
Suggested fix: Replace the one-to-one instruction with explicit production values:
NODE_ENV=production- Omit
HOST, so the server binds 0.0.0.0 - Set a strong
INTERNAL_TRIGGER_TOKEN - Fill in the service-connection settings
| - Application Settings: all `.env` vars mapped one-to-one. | |
| - Application Settings: copy your `.env` values, but set `NODE_ENV=production` (enables JWT validation and requires `INTERNAL_TRIGGER_TOKEN`), remove `HOST` so the server binds `0.0.0.0`, and set a strong `INTERNAL_TRIGGER_TOKEN`. |
| // Level-based logger for the scrum-master scenario. Every module should prefer | ||
| // this over raw `console.*` so verbosity can be controlled centrally through | ||
| // the `LOG_LEVEL` env var and secrets never leak into logs. | ||
| // | ||
| // LOG_LEVEL env values (case-insensitive): error | warn | info | debug | trace | ||
| // Default: info | ||
| // | ||
| // Usage: | ||
| // import { log } from '../util/logger'; |
There was a problem hiding this comment.
Minor: LOG_LEVEL logger is never used
The level-based logger and the LOG_LEVEL setting do nothing. Every module logs with console.*, so changing LOG_LEVEL does not change output.
Evidence
logger.ts:4-12 says every module should import log and that LOG_LEVEL gates output. README.md:206 and design.md:410 document that contract. No file under scenarios/scrum-master imports ../util/logger or the log export. startup-check.ts:26 only prints the env value. Setting LOG_LEVEL=error still prints [standup], [cron], and [security] lines via console.log.
Suggested fix: Either route scenario logs through log or remove the unused logger and stop documenting LOG_LEVEL as effective.
| actions.push({ | ||
| item: `Carry-over: ${cleanUserStorySummary(m.summary)} (${m.key})`, | ||
| owner: m.assignee?.displayName ?? 'Unassigned', | ||
| sprint: 'Sprint 2', |
There was a problem hiding this comment.
Minor: Sprint-close action items always say Sprint 2
Every generated action item is labeled Sprint 2, regardless of which sprint just closed. The management report therefore names the wrong next sprint.
Evidence
buildActionItems hardcodes sprint: 'Sprint 2' for carry-over stories (report.ts:260), open bugs (report.ts:268), and the cross-cutting suggestions (report.ts:273-282). The function receives the closed sprint's issues but never reads the sprint name or computes the next one.
Suggested fix: Use the closed sprint's name, or a computed next-sprint label, instead of the literal Sprint 2.
Krishnadheeraj (DheerajPannala)
left a comment
Krishnadheeraj (DheerajPannala)
left a comment
There was a problem hiding this comment.
I would not merge it yet; the end-to-end behavior still needs to match what this package claims.
38 things to address before this merges; details inline.
| let transitions: ProposedTransition[] = []; | ||
| try { transitions = JSON.parse(String(value?.transitions ?? '[]')); } | ||
| catch { transitions = []; } | ||
|
|
||
| const approvedKeys = new Set<string>(); | ||
| for (const t of transitions) { | ||
| const approvedRaw = String(value?.[`approve_${t.issueKey}`] ?? 'false'); | ||
| if (approvedRaw === 'true') approvedKeys.add(t.issueKey); |
There was a problem hiding this comment.
Should fix: Authorize reconciliation approvals against server-side state
Sending the approval card to the Scrum Master does not authorize its subsequent submission. This handler accepts both the proposed transitions and approval flags from the caller without checking the caller's role or looking up a pending proposal. An authenticated non-SM can consequently request arbitrary transitions using the shared Jira credentials, including for issues outside the standup.
Evidence
scenarios/scrum-master/src/agent.ts:108-110 dispatches any reconcile.* submission to this handler. reconcile.ts:210 parses value.transitions, lines 215-216 trust approve_, and lines 248-251 pass the supplied issueKey and transitionId directly to Jira. For example, a non-SM submission with action='reconcile.apply', transitions='[{"issueKey":"DEMO-42","transitionId":"31"}]', and approve_DEMO-42='true' reaches transitionIssue without any caller, standup, or proposal lookup. services/jira.ts:85-90 configures the shared Basic-auth credentials.
Suggested fix: Require the authenticated caller to be the configured Scrum Master and resolve approved transitions from a stored pending proposal. Accept only identifiers and approval selections from the submitted card, not authoritative issue/transition pairs.
| if (!internalToken) { | ||
| // Fail-shut in production so open /api/internal/* can never ship by mistake. | ||
| if (process.env.NODE_ENV === 'production') { |
There was a problem hiding this comment.
Should fix: Fail closed outside explicitly enabled development mode
The missing-secret guard protects only the literal NODE_ENV=production case. With NODE_ENV unset or set to staging, the internal endpoints remain unauthenticated even though the rest of the server treats those environments as authenticated deployments. An unauthenticated caller can trigger standup fan-out, forced risk notifications, and reports.
Evidence
scenarios/scrum-master/src/index.ts:52-53 requires normal authentication unless NODE_ENV is development, but lines 79-98 allow an empty internal token in every environment except production. The internal routes are registered before authorizeJWT at line 165, and line 176 defaults non-development deployments to 0.0.0.0. With valid service configuration, NODE_ENV=staging, HOST unset, and INTERNAL_TRIGGER_TOKEN unset, an unauthenticated POST to /api/internal/nightly-check?force=warn&forceAlert=true passes requireInternalToken.
Suggested fix: Allow an empty internal token only when isDevelopment is true. Fail startup or reject internal requests in every other environment when the secret is missing.
| // Load roster and issues in parallel. | ||
| const [members, issues] = await Promise.all([ | ||
| listTeamMembers(), |
There was a problem hiding this comment.
Should fix: Provide an offline standup path instead of requiring SharePoint in mock mode
The documented mock quick start cannot run /standup with only the language-service credentials. Although Jira is mocked, this unconditional roster read still requires a SharePoint site and Graph sign-in. The command fails before generating any standup card, contradicting the primary offline onboarding path.
Evidence
README.md:52-76 describes the no-external-services setup and directs the user to run /standup. scenarios/scrum-master/src/handlers/standup.ts:120-123 calls listTeamMembers regardless of Jira mode. services/team-roster.ts:64-65 calls listItems; services/sharepoint.ts:133-136 calls getSiteId; getSiteId at line 37 calls getSharePointConfig. config.ts:79 requires SHAREPOINT_SITE_URL, and lines 12-15 throw 'Missing required env var: SHAREPOINT_SITE_URL' for the supplied empty default.
[grok-4.7] README.md:73 tells the playground user to DM /standup after copying .env.template and leaving JIRA_MODE=mock, and the PR description says this works without a SharePoint site. triggerStandup always calls listTeamMembers() (standup.ts:121). That calls listItems → getSharePointConfig() (sharepoint.ts:37), and required('SHAREPOINT_SITE_URL') (config.ts:79, config.ts:14) throws Missing required env var: SHAREPOINT_SITE_URL when the template value is empty. If the operator seeds src/scripts/team.sample.json instead, those rows use JiraAccountId values 557058:00000000-0000-0000-0000-000000000001 and siblings (team.sample.json:6), while mock assignees are acc-alex, acc-priya, acc-arjun, acc-sam, acc-chetan (jira-mock.ts:26). The join at standup.ts:128-132 skips every issue, so sentTo stays 0.
Suggested fix: Add an explicit offline standup path using an in-memory roster/state and the current Playground conversation. Keep Graph-backed roster, persistence, and proactive delivery behind the live configuration path.
| } catch (e) { | ||
| console.error('[cron] Standup failed:', (e as Error).message); | ||
| } | ||
| }, { timezone: cfg.timezone }); |
There was a problem hiding this comment.
Should fix: Interpret the UTC cron expressions in UTC
The configured cron expressions already encode UTC, but node-cron is told to interpret them in the display timezone. With the defaults, standup runs at 03:30 IST rather than 09:00 IST, and its four-hour cutoff expires at 07:30 before the documented standup time. The nightly job is likewise shifted to 19:00 IST instead of 00:30 IST.
Evidence
scenarios/scrum-master/src/config.ts:106-109 defaults to standupCron='0 30 3 * * 1-5', nightlyCron='0 0 19 * * *', and timezone='Asia/Kolkata'. local-scheduler.ts:44 and 52 pass that timezone to cron.schedule, so the hour fields are interpreted as IST. README.md's configuration table explicitly calls these UTC cron expressions and labels TIMEZONE as display-only; azure-functions/src/index.ts:31-34 also identifies 03:30 UTC as 09:00 IST.
[grok-4.7] Defaults are STANDUP_CRON=0 30 3 * * 1-5 and NIGHTLY_CRON=0 0 19 * * * (config.ts:106-107) with TIMEZONE=Asia/Kolkata (config.ts:109). README.md:218-219 call those expressions UTC (03:30 UTC = 09:00 IST, 19:00 UTC = 00:30 IST) and README.md:221 says TIMEZONE is display-only. cron.schedule is called with { timezone: cfg.timezone } (local-scheduler.ts:44 and local-scheduler.ts:52), so node-cron fires at 03:30 and 19:00 Asia/Kolkata, not 09:00 / 00:30 IST. Azure Functions uses the same strings as NCRONTAB (azure-functions/src/index.ts:35 and :56), which is UTC unless WEBSITE_TIME_ZONE is set. azure-functions/README.md:88 sets WEBSITE_TIME_ZONE='India Standard Time' while line 91 says that setting does not change the schedule; line 107 says the opposite. With that app setting, Functions also fire at 03:30 IST. Local cron and Functions therefore neither match each other under the documented defaults nor match the 09:00 IST claim.
Suggested fix: Schedule the existing expressions with timezone='UTC' and reserve TIMEZONE for display, matching the documented contract and Azure timer expressions.
| cron.schedule(cfg.nightlyCron, async () => { | ||
| console.log('[cron] Nightly tick — warn + report'); | ||
| try { await runWarnCheck(); } | ||
| catch (e) { console.error('[cron] warn failed:', (e as Error).message); } | ||
| try { await runSprintCloseReport(); } | ||
| catch (e) { console.error('[cron] report failed:', (e as Error).message); } |
There was a problem hiding this comment.
Minor: Wire the mid-sprint report into an automatic trigger
The T-2 report is implemented but never invoked by a scheduled path. This nightly callback runs only Warn and the close report, and the Azure nightly endpoint does the same. A correctly configured deployment therefore never posts the advertised automatic mid-sprint RAG report unless someone manually calls its separate endpoint.
Evidence
scenarios/scrum-master/src/cron/local-scheduler.ts:46-51 invokes only runWarnCheck and runSprintCloseReport. src/index.ts:118-149 likewise omits runSprintSummary from /api/internal/nightly-check. The only invocation of runSprintSummary is index.ts:157 in the manually addressed /api/internal/sprint-summary route, while azure-functions/src/index.ts:50-56 schedules only /api/internal/nightly-check. No scheduled caller reaches the T-2 gate in handlers/sprint-summary.ts:259-265.
[grok-4.7] README.md:35 says the RAG report fires two days before sprint end. sprint-summary.ts:16-17 says the nightly cron calls runSprintSummary() and only emits on sprintEnd-2. design.md:315 says the handler is called by the sprint-summary endpoint or the T-2 timer. The PR description says the Functions package includes mid-sprint timers. The nightly job only calls runWarnCheck and runSprintCloseReport (local-scheduler.ts:48-50). /api/internal/nightly-check does the same (index.ts:133 and index.ts:142) and never calls runSprintSummary (that call exists only on the manual route at index.ts:157). azure-functions/src/index.ts registers only StandupTimer and NightlyTimer, and NightlyTimer POSTs /api/internal/nightly-check. A deployed timer installation therefore never posts the mid-sprint table.
Suggested fix: Invoke the non-forced runSprintSummary from both nightly runners, or add the advertised dedicated timer. Preserve its date gate so ordinary nightly calls produce a report only at the intended time.
|
|
||
| ### Enable HTTP tracing | ||
|
|
||
| If a live call fails silently, set `LOG_HTTP=true` in `.env` and restart. Every outbound axios call prints `method host path status latency`. Credentials are redacted automatically. |
There was a problem hiding this comment.
Minor: LOG_HTTP=true troubleshooting step doesn't trace Jira calls: interceptors are on the default axios, Jira uses axios.create()
The troubleshooting section says LOG_HTTP=true prints every outbound call. .env.template:44-45 and README.md:207 go further and say it covers every Jira/Graph/MCP call.
- The tracer registers its interceptors on the default
axiosexport. - The Jira client is an
axios.create()instance, and axios instances get their own interceptor chains. So Jira calls, the documented use case, are never logged. - Graph (
@microsoft/microsoft-graph-clientover fetch) and MCP don't use axios at all.
As a result, the advice 'if a live call fails silently, set LOG_HTTP=true' gives the operator nothing beyond the 'outbound tracing enabled' banner.
Evidence
- README.md:292: 'set
LOG_HTTP=true… Every outbound axios call printsmethod host path status latency'. .env.template:44: 'prints every outbound Jira/Graph/MCP call'.- src/util/httpLogger.ts:33
axios.interceptors.request.use(...)and :43axios.interceptors.response.use(...)apply to the default instance only. - src/services/jira.ts:85:
this.http = axios.create({ baseURL: cfg.baseUrl, ... })is the scenario's only axios client at runtime. seed-jira-sample.ts:84 also usesaxios.create. - axios
create()builds a newAxioswith freshInterceptorManagers, so default-instance interceptors don't apply. - src/services/graph.ts:18/32 use isomorphic-fetch and
@microsoft/microsoft-graph-client.
Suggested fix: Attach the interceptors to the Jira instance, e.g. export attachHttpLogging(instance) from httpLogger and call it in LiveJiraClient's constructor. Then narrow the docs to 'Jira REST calls', since Graph and MCP aren't axios-based.
| - **`GRAPH_CLIENT_ID` defaults to the well-known "Microsoft Graph Command Line Tools" public client** for zero-setup device-code sign-in. Register your own multi-tenant public client for a real deployment. | ||
| - **Single-team by design.** The sample assumes one scrum team per process (single project key, single board, single channel). Multi-team support (per-team config, isolated Jira credentials, sharded timers) is called out as future work in [`docs/design.md#12-known-limitations--hardening-roadmap`](docs/design.md#12-known-limitations--hardening-roadmap). | ||
| - **Proactive DMs require prior interaction.** Every squad member must have said "hi" to the agent at least once so their conversation reference is captured in `SMA_TeamMembers`. | ||
| - **Running local `node-cron` and Azure Function timers simultaneously is safe** (`standupId = <sprintId>#<yyyy-mm-dd>` provides idempotency) but does two Jira reads per tick. Set `LOCAL_CRON=false` once the Function is deployed. |
There was a problem hiding this comment.
Minor: README says standup triggers are idempotent per day and safe to run with both schedulers, but the guard is in-memory and cleared after the summary
README.md:233 says the standup trigger is 'Idempotent per calendar day', and README.md:329 says running local node-cron and the Azure Function timers together 'is safe'.
The only guard is an in-memory lookup of the current standupId, and it doesn't last the day:
summarizeStandupevicts the session once the summary posts, on cutoff or when everyone has responded.- A process restart empties the map.
Any second trigger later the same UTC day re-sends every standup card and later posts a second summary. That trigger could be the other scheduler, a manual /standup, or the curl example. The README's own troubleshooting table (line 288) lists 'Duplicate standup summary cards' from exactly this setup.
Evidence
- standup.ts:107-117 is the only check:
const standupId = makeStandupId(sprint.id); ... const existingMem = getSession(standupId); if (existingMem) {...return}. - standup.ts:156-168 inserts the StandupSessions row without checking for an existing Title, and swallows errors.
- standup.ts:382 and :190-194 call
summarizeStandup, which ends withremoveSession(session.standupId)(standup.ts:524). - session-store.ts:54:
const sessions = new Map<...>(), process memory only. - session-store.ts:56-61: the standupId uses the UTC date.
Scenario:
- The SM runs
/standupat 09:05 and everyone answers by 10:00. - The summary posts and the session is evicted.
- The other scheduler, or a manual retry, fires later the same UTC day.
getSessionreturns undefined, so the cards fan out again and a second summary follows.
Suggested fix: Either make the guard durable, or correct the docs:
- Durable guard: check
findByTitle('standupSessions', standupId)before fanning out, and keep summarized sessions marked rather than evicted for the rest of the day. - Docs: say that re-triggering after a summary or a restart re-sends the cards, and recommend running only one scheduler.
| - **Running local `node-cron` and Azure Function timers simultaneously is safe** (`standupId = <sprintId>#<yyyy-mm-dd>` provides idempotency) but does two Jira reads per tick. Set `LOCAL_CRON=false` once the Function is deployed. | |
| - **Run only one scheduler.** The standup guard is in-memory (`standupId = <sprintId>#<UTC yyyy-mm-dd>`) and is cleared once the summary posts or the process restarts, so a second trigger later the same day re-sends the cards. Set `LOCAL_CRON=false` once the Function is deployed. |
| Minimum runtime configuration: | ||
|
|
||
| - Azure App Service (Linux, Node 20 LTS). Set `WEBSITES_PORT=3978`. | ||
| - Application Settings: all `.env` vars mapped one-to-one. |
There was a problem hiding this comment.
Minor: Deploy guidance maps dev-only .env values (NODE_ENV=development, HOST=127.0.0.1) to App Service
'Application Settings: all .env vars mapped one-to-one' copies the template's local-dev settings into Azure.
NODE_ENV=developmentis the switch the server uses to turn off JWT validation on/api/messages.- It also skips the fail-shut check for an empty
INTERNAL_TRIGGER_TOKEN. The template leaves that token blank, so/api/internal/*stays open. HOST=127.0.0.1binds the server to loopback, which the App Service front end can't reach.
The deploy section should list the production overrides explicitly.
Evidence
- README.md:301: '- Application Settings: all
.envvars mapped one-to-one.' .env.template:35NODE_ENV=development,.env.template:36HOST=127.0.0.1,.env.template:100INTERNAL_TRIGGER_TOKEN=.- src/index.ts:50-53: the comment '// Only NODE_ENV=development explicitly disables authentication', then
const authConfig = isDevelopment ? {} : loadAuthConfigFromEnv();. That config is passed toserver.use(authorizeJWT(authConfig))at :165. - src/index.ts:79-92 exits on an empty token only
if (process.env.NODE_ENV === 'production'); otherwiserequireInternalTokenreturns true (:95-98). - src/index.ts:176:
const host = process.env.HOST ?? (...), so the server binds 127.0.0.1 when HOST is mapped.
Suggested fix: Replace the one-to-one instruction with explicit production values:
NODE_ENV=production- Omit
HOST, so the server binds 0.0.0.0 - Set a strong
INTERNAL_TRIGGER_TOKEN - Fill in the service-connection settings
| - Application Settings: all `.env` vars mapped one-to-one. | |
| - Application Settings: copy your `.env` values, but set `NODE_ENV=production` (enables JWT validation and requires `INTERNAL_TRIGGER_TOKEN`), remove `HOST` so the server binds `0.0.0.0`, and set a strong `INTERNAL_TRIGGER_TOKEN`. |
| // Level-based logger for the scrum-master scenario. Every module should prefer | ||
| // this over raw `console.*` so verbosity can be controlled centrally through | ||
| // the `LOG_LEVEL` env var and secrets never leak into logs. | ||
| // | ||
| // LOG_LEVEL env values (case-insensitive): error | warn | info | debug | trace | ||
| // Default: info | ||
| // | ||
| // Usage: | ||
| // import { log } from '../util/logger'; |
There was a problem hiding this comment.
Minor: LOG_LEVEL logger is never used
The level-based logger and the LOG_LEVEL setting do nothing. Every module logs with console.*, so changing LOG_LEVEL does not change output.
Evidence
logger.ts:4-12 says every module should import log and that LOG_LEVEL gates output. README.md:206 and design.md:410 document that contract. No file under scenarios/scrum-master imports ../util/logger or the log export. startup-check.ts:26 only prints the env value. Setting LOG_LEVEL=error still prints [standup], [cron], and [security] lines via console.log.
Suggested fix: Either route scenario logs through log or remove the unused logger and stop documenting LOG_LEVEL as effective.
| actions.push({ | ||
| item: `Carry-over: ${cleanUserStorySummary(m.summary)} (${m.key})`, | ||
| owner: m.assignee?.displayName ?? 'Unassigned', | ||
| sprint: 'Sprint 2', |
There was a problem hiding this comment.
Minor: Sprint-close action items always say Sprint 2
Every generated action item is labeled Sprint 2, regardless of which sprint just closed. The management report therefore names the wrong next sprint.
Evidence
buildActionItems hardcodes sprint: 'Sprint 2' for carry-over stories (report.ts:260), open bugs (report.ts:268), and the cross-cutting suggestions (report.ts:273-282). The function receives the closed sprint's issues but never reads the sprint name or computes the next one.
Suggested fix: Use the closed sprint's name, or a computed next-sprint label, instead of the literal Sprint 2.
Krishnadheeraj (DheerajPannala)
left a comment
Krishnadheeraj (DheerajPannala)
left a comment
There was a problem hiding this comment.
I would not merge it yet; the end-to-end behavior still needs to match what this package claims.
38 things to address before this merges; details inline.
| let transitions: ProposedTransition[] = []; | ||
| try { transitions = JSON.parse(String(value?.transitions ?? '[]')); } | ||
| catch { transitions = []; } | ||
|
|
||
| const approvedKeys = new Set<string>(); | ||
| for (const t of transitions) { | ||
| const approvedRaw = String(value?.[`approve_${t.issueKey}`] ?? 'false'); | ||
| if (approvedRaw === 'true') approvedKeys.add(t.issueKey); |
There was a problem hiding this comment.
Should fix: Authorize reconciliation approvals against server-side state
Sending the approval card to the Scrum Master does not authorize its subsequent submission. This handler accepts both the proposed transitions and approval flags from the caller without checking the caller's role or looking up a pending proposal. An authenticated non-SM can consequently request arbitrary transitions using the shared Jira credentials, including for issues outside the standup.
Evidence
scenarios/scrum-master/src/agent.ts:108-110 dispatches any reconcile.* submission to this handler. reconcile.ts:210 parses value.transitions, lines 215-216 trust approve_, and lines 248-251 pass the supplied issueKey and transitionId directly to Jira. For example, a non-SM submission with action='reconcile.apply', transitions='[{"issueKey":"DEMO-42","transitionId":"31"}]', and approve_DEMO-42='true' reaches transitionIssue without any caller, standup, or proposal lookup. services/jira.ts:85-90 configures the shared Basic-auth credentials.
Suggested fix: Require the authenticated caller to be the configured Scrum Master and resolve approved transitions from a stored pending proposal. Accept only identifiers and approval selections from the submitted card, not authoritative issue/transition pairs.
| if (!internalToken) { | ||
| // Fail-shut in production so open /api/internal/* can never ship by mistake. | ||
| if (process.env.NODE_ENV === 'production') { |
There was a problem hiding this comment.
Should fix: Fail closed outside explicitly enabled development mode
The missing-secret guard protects only the literal NODE_ENV=production case. With NODE_ENV unset or set to staging, the internal endpoints remain unauthenticated even though the rest of the server treats those environments as authenticated deployments. An unauthenticated caller can trigger standup fan-out, forced risk notifications, and reports.
Evidence
scenarios/scrum-master/src/index.ts:52-53 requires normal authentication unless NODE_ENV is development, but lines 79-98 allow an empty internal token in every environment except production. The internal routes are registered before authorizeJWT at line 165, and line 176 defaults non-development deployments to 0.0.0.0. With valid service configuration, NODE_ENV=staging, HOST unset, and INTERNAL_TRIGGER_TOKEN unset, an unauthenticated POST to /api/internal/nightly-check?force=warn&forceAlert=true passes requireInternalToken.
Suggested fix: Allow an empty internal token only when isDevelopment is true. Fail startup or reject internal requests in every other environment when the secret is missing.
| // Load roster and issues in parallel. | ||
| const [members, issues] = await Promise.all([ | ||
| listTeamMembers(), |
There was a problem hiding this comment.
Should fix: Provide an offline standup path instead of requiring SharePoint in mock mode
The documented mock quick start cannot run /standup with only the language-service credentials. Although Jira is mocked, this unconditional roster read still requires a SharePoint site and Graph sign-in. The command fails before generating any standup card, contradicting the primary offline onboarding path.
Evidence
README.md:52-76 describes the no-external-services setup and directs the user to run /standup. scenarios/scrum-master/src/handlers/standup.ts:120-123 calls listTeamMembers regardless of Jira mode. services/team-roster.ts:64-65 calls listItems; services/sharepoint.ts:133-136 calls getSiteId; getSiteId at line 37 calls getSharePointConfig. config.ts:79 requires SHAREPOINT_SITE_URL, and lines 12-15 throw 'Missing required env var: SHAREPOINT_SITE_URL' for the supplied empty default.
[grok-4.7] README.md:73 tells the playground user to DM /standup after copying .env.template and leaving JIRA_MODE=mock, and the PR description says this works without a SharePoint site. triggerStandup always calls listTeamMembers() (standup.ts:121). That calls listItems → getSharePointConfig() (sharepoint.ts:37), and required('SHAREPOINT_SITE_URL') (config.ts:79, config.ts:14) throws Missing required env var: SHAREPOINT_SITE_URL when the template value is empty. If the operator seeds src/scripts/team.sample.json instead, those rows use JiraAccountId values 557058:00000000-0000-0000-0000-000000000001 and siblings (team.sample.json:6), while mock assignees are acc-alex, acc-priya, acc-arjun, acc-sam, acc-chetan (jira-mock.ts:26). The join at standup.ts:128-132 skips every issue, so sentTo stays 0.
Suggested fix: Add an explicit offline standup path using an in-memory roster/state and the current Playground conversation. Keep Graph-backed roster, persistence, and proactive delivery behind the live configuration path.
| } catch (e) { | ||
| console.error('[cron] Standup failed:', (e as Error).message); | ||
| } | ||
| }, { timezone: cfg.timezone }); |
There was a problem hiding this comment.
Should fix: Interpret the UTC cron expressions in UTC
The configured cron expressions already encode UTC, but node-cron is told to interpret them in the display timezone. With the defaults, standup runs at 03:30 IST rather than 09:00 IST, and its four-hour cutoff expires at 07:30 before the documented standup time. The nightly job is likewise shifted to 19:00 IST instead of 00:30 IST.
Evidence
scenarios/scrum-master/src/config.ts:106-109 defaults to standupCron='0 30 3 * * 1-5', nightlyCron='0 0 19 * * *', and timezone='Asia/Kolkata'. local-scheduler.ts:44 and 52 pass that timezone to cron.schedule, so the hour fields are interpreted as IST. README.md's configuration table explicitly calls these UTC cron expressions and labels TIMEZONE as display-only; azure-functions/src/index.ts:31-34 also identifies 03:30 UTC as 09:00 IST.
[grok-4.7] Defaults are STANDUP_CRON=0 30 3 * * 1-5 and NIGHTLY_CRON=0 0 19 * * * (config.ts:106-107) with TIMEZONE=Asia/Kolkata (config.ts:109). README.md:218-219 call those expressions UTC (03:30 UTC = 09:00 IST, 19:00 UTC = 00:30 IST) and README.md:221 says TIMEZONE is display-only. cron.schedule is called with { timezone: cfg.timezone } (local-scheduler.ts:44 and local-scheduler.ts:52), so node-cron fires at 03:30 and 19:00 Asia/Kolkata, not 09:00 / 00:30 IST. Azure Functions uses the same strings as NCRONTAB (azure-functions/src/index.ts:35 and :56), which is UTC unless WEBSITE_TIME_ZONE is set. azure-functions/README.md:88 sets WEBSITE_TIME_ZONE='India Standard Time' while line 91 says that setting does not change the schedule; line 107 says the opposite. With that app setting, Functions also fire at 03:30 IST. Local cron and Functions therefore neither match each other under the documented defaults nor match the 09:00 IST claim.
Suggested fix: Schedule the existing expressions with timezone='UTC' and reserve TIMEZONE for display, matching the documented contract and Azure timer expressions.
| cron.schedule(cfg.nightlyCron, async () => { | ||
| console.log('[cron] Nightly tick — warn + report'); | ||
| try { await runWarnCheck(); } | ||
| catch (e) { console.error('[cron] warn failed:', (e as Error).message); } | ||
| try { await runSprintCloseReport(); } | ||
| catch (e) { console.error('[cron] report failed:', (e as Error).message); } |
There was a problem hiding this comment.
Minor: Wire the mid-sprint report into an automatic trigger
The T-2 report is implemented but never invoked by a scheduled path. This nightly callback runs only Warn and the close report, and the Azure nightly endpoint does the same. A correctly configured deployment therefore never posts the advertised automatic mid-sprint RAG report unless someone manually calls its separate endpoint.
Evidence
scenarios/scrum-master/src/cron/local-scheduler.ts:46-51 invokes only runWarnCheck and runSprintCloseReport. src/index.ts:118-149 likewise omits runSprintSummary from /api/internal/nightly-check. The only invocation of runSprintSummary is index.ts:157 in the manually addressed /api/internal/sprint-summary route, while azure-functions/src/index.ts:50-56 schedules only /api/internal/nightly-check. No scheduled caller reaches the T-2 gate in handlers/sprint-summary.ts:259-265.
[grok-4.7] README.md:35 says the RAG report fires two days before sprint end. sprint-summary.ts:16-17 says the nightly cron calls runSprintSummary() and only emits on sprintEnd-2. design.md:315 says the handler is called by the sprint-summary endpoint or the T-2 timer. The PR description says the Functions package includes mid-sprint timers. The nightly job only calls runWarnCheck and runSprintCloseReport (local-scheduler.ts:48-50). /api/internal/nightly-check does the same (index.ts:133 and index.ts:142) and never calls runSprintSummary (that call exists only on the manual route at index.ts:157). azure-functions/src/index.ts registers only StandupTimer and NightlyTimer, and NightlyTimer POSTs /api/internal/nightly-check. A deployed timer installation therefore never posts the mid-sprint table.
Suggested fix: Invoke the non-forced runSprintSummary from both nightly runners, or add the advertised dedicated timer. Preserve its date gate so ordinary nightly calls produce a report only at the intended time.
|
|
||
| ### Enable HTTP tracing | ||
|
|
||
| If a live call fails silently, set `LOG_HTTP=true` in `.env` and restart. Every outbound axios call prints `method host path status latency`. Credentials are redacted automatically. |
There was a problem hiding this comment.
Minor: LOG_HTTP=true troubleshooting step doesn't trace Jira calls: interceptors are on the default axios, Jira uses axios.create()
The troubleshooting section says LOG_HTTP=true prints every outbound call. .env.template:44-45 and README.md:207 go further and say it covers every Jira/Graph/MCP call.
- The tracer registers its interceptors on the default
axiosexport. - The Jira client is an
axios.create()instance, and axios instances get their own interceptor chains. So Jira calls, the documented use case, are never logged. - Graph (
@microsoft/microsoft-graph-clientover fetch) and MCP don't use axios at all.
As a result, the advice 'if a live call fails silently, set LOG_HTTP=true' gives the operator nothing beyond the 'outbound tracing enabled' banner.
Evidence
- README.md:292: 'set
LOG_HTTP=true… Every outbound axios call printsmethod host path status latency'. .env.template:44: 'prints every outbound Jira/Graph/MCP call'.- src/util/httpLogger.ts:33
axios.interceptors.request.use(...)and :43axios.interceptors.response.use(...)apply to the default instance only. - src/services/jira.ts:85:
this.http = axios.create({ baseURL: cfg.baseUrl, ... })is the scenario's only axios client at runtime. seed-jira-sample.ts:84 also usesaxios.create. - axios
create()builds a newAxioswith freshInterceptorManagers, so default-instance interceptors don't apply. - src/services/graph.ts:18/32 use isomorphic-fetch and
@microsoft/microsoft-graph-client.
Suggested fix: Attach the interceptors to the Jira instance, e.g. export attachHttpLogging(instance) from httpLogger and call it in LiveJiraClient's constructor. Then narrow the docs to 'Jira REST calls', since Graph and MCP aren't axios-based.
| - **`GRAPH_CLIENT_ID` defaults to the well-known "Microsoft Graph Command Line Tools" public client** for zero-setup device-code sign-in. Register your own multi-tenant public client for a real deployment. | ||
| - **Single-team by design.** The sample assumes one scrum team per process (single project key, single board, single channel). Multi-team support (per-team config, isolated Jira credentials, sharded timers) is called out as future work in [`docs/design.md#12-known-limitations--hardening-roadmap`](docs/design.md#12-known-limitations--hardening-roadmap). | ||
| - **Proactive DMs require prior interaction.** Every squad member must have said "hi" to the agent at least once so their conversation reference is captured in `SMA_TeamMembers`. | ||
| - **Running local `node-cron` and Azure Function timers simultaneously is safe** (`standupId = <sprintId>#<yyyy-mm-dd>` provides idempotency) but does two Jira reads per tick. Set `LOCAL_CRON=false` once the Function is deployed. |
There was a problem hiding this comment.
Minor: README says standup triggers are idempotent per day and safe to run with both schedulers, but the guard is in-memory and cleared after the summary
README.md:233 says the standup trigger is 'Idempotent per calendar day', and README.md:329 says running local node-cron and the Azure Function timers together 'is safe'.
The only guard is an in-memory lookup of the current standupId, and it doesn't last the day:
summarizeStandupevicts the session once the summary posts, on cutoff or when everyone has responded.- A process restart empties the map.
Any second trigger later the same UTC day re-sends every standup card and later posts a second summary. That trigger could be the other scheduler, a manual /standup, or the curl example. The README's own troubleshooting table (line 288) lists 'Duplicate standup summary cards' from exactly this setup.
Evidence
- standup.ts:107-117 is the only check:
const standupId = makeStandupId(sprint.id); ... const existingMem = getSession(standupId); if (existingMem) {...return}. - standup.ts:156-168 inserts the StandupSessions row without checking for an existing Title, and swallows errors.
- standup.ts:382 and :190-194 call
summarizeStandup, which ends withremoveSession(session.standupId)(standup.ts:524). - session-store.ts:54:
const sessions = new Map<...>(), process memory only. - session-store.ts:56-61: the standupId uses the UTC date.
Scenario:
- The SM runs
/standupat 09:05 and everyone answers by 10:00. - The summary posts and the session is evicted.
- The other scheduler, or a manual retry, fires later the same UTC day.
getSessionreturns undefined, so the cards fan out again and a second summary follows.
Suggested fix: Either make the guard durable, or correct the docs:
- Durable guard: check
findByTitle('standupSessions', standupId)before fanning out, and keep summarized sessions marked rather than evicted for the rest of the day. - Docs: say that re-triggering after a summary or a restart re-sends the cards, and recommend running only one scheduler.
| - **Running local `node-cron` and Azure Function timers simultaneously is safe** (`standupId = <sprintId>#<yyyy-mm-dd>` provides idempotency) but does two Jira reads per tick. Set `LOCAL_CRON=false` once the Function is deployed. | |
| - **Run only one scheduler.** The standup guard is in-memory (`standupId = <sprintId>#<UTC yyyy-mm-dd>`) and is cleared once the summary posts or the process restarts, so a second trigger later the same day re-sends the cards. Set `LOCAL_CRON=false` once the Function is deployed. |
| Minimum runtime configuration: | ||
|
|
||
| - Azure App Service (Linux, Node 20 LTS). Set `WEBSITES_PORT=3978`. | ||
| - Application Settings: all `.env` vars mapped one-to-one. |
There was a problem hiding this comment.
Minor: Deploy guidance maps dev-only .env values (NODE_ENV=development, HOST=127.0.0.1) to App Service
'Application Settings: all .env vars mapped one-to-one' copies the template's local-dev settings into Azure.
NODE_ENV=developmentis the switch the server uses to turn off JWT validation on/api/messages.- It also skips the fail-shut check for an empty
INTERNAL_TRIGGER_TOKEN. The template leaves that token blank, so/api/internal/*stays open. HOST=127.0.0.1binds the server to loopback, which the App Service front end can't reach.
The deploy section should list the production overrides explicitly.
Evidence
- README.md:301: '- Application Settings: all
.envvars mapped one-to-one.' .env.template:35NODE_ENV=development,.env.template:36HOST=127.0.0.1,.env.template:100INTERNAL_TRIGGER_TOKEN=.- src/index.ts:50-53: the comment '// Only NODE_ENV=development explicitly disables authentication', then
const authConfig = isDevelopment ? {} : loadAuthConfigFromEnv();. That config is passed toserver.use(authorizeJWT(authConfig))at :165. - src/index.ts:79-92 exits on an empty token only
if (process.env.NODE_ENV === 'production'); otherwiserequireInternalTokenreturns true (:95-98). - src/index.ts:176:
const host = process.env.HOST ?? (...), so the server binds 127.0.0.1 when HOST is mapped.
Suggested fix: Replace the one-to-one instruction with explicit production values:
NODE_ENV=production- Omit
HOST, so the server binds 0.0.0.0 - Set a strong
INTERNAL_TRIGGER_TOKEN - Fill in the service-connection settings
| - Application Settings: all `.env` vars mapped one-to-one. | |
| - Application Settings: copy your `.env` values, but set `NODE_ENV=production` (enables JWT validation and requires `INTERNAL_TRIGGER_TOKEN`), remove `HOST` so the server binds `0.0.0.0`, and set a strong `INTERNAL_TRIGGER_TOKEN`. |
| // Level-based logger for the scrum-master scenario. Every module should prefer | ||
| // this over raw `console.*` so verbosity can be controlled centrally through | ||
| // the `LOG_LEVEL` env var and secrets never leak into logs. | ||
| // | ||
| // LOG_LEVEL env values (case-insensitive): error | warn | info | debug | trace | ||
| // Default: info | ||
| // | ||
| // Usage: | ||
| // import { log } from '../util/logger'; |
There was a problem hiding this comment.
Minor: LOG_LEVEL logger is never used
The level-based logger and the LOG_LEVEL setting do nothing. Every module logs with console.*, so changing LOG_LEVEL does not change output.
Evidence
logger.ts:4-12 says every module should import log and that LOG_LEVEL gates output. README.md:206 and design.md:410 document that contract. No file under scenarios/scrum-master imports ../util/logger or the log export. startup-check.ts:26 only prints the env value. Setting LOG_LEVEL=error still prints [standup], [cron], and [security] lines via console.log.
Suggested fix: Either route scenario logs through log or remove the unused logger and stop documenting LOG_LEVEL as effective.
| actions.push({ | ||
| item: `Carry-over: ${cleanUserStorySummary(m.summary)} (${m.key})`, | ||
| owner: m.assignee?.displayName ?? 'Unassigned', | ||
| sprint: 'Sprint 2', |
There was a problem hiding this comment.
Minor: Sprint-close action items always say Sprint 2
Every generated action item is labeled Sprint 2, regardless of which sprint just closed. The management report therefore names the wrong next sprint.
Evidence
buildActionItems hardcodes sprint: 'Sprint 2' for carry-over stories (report.ts:260), open bugs (report.ts:268), and the cross-cutting suggestions (report.ts:273-282). The function receives the closed sprint's issues but never reads the sprint name or computes the next one.
Suggested fix: Use the closed sprint's name, or a computed next-sprint label, instead of the literal Sprint 2.
Address Dheeraj's review comments on reconciliation approvals, offline standups, scheduling, reports, Jira pagination, and documentation. Add focused regression coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Require Zod 4.6.6 or later for GHSA-mrhx-c7xc-m3w5. Align Agents SDK 0.7 with the A365 adapters and OpenAI 6, document Node.js 22+, and add offline real-SDK compatibility tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Krishnadheeraj (DheerajPannala)
left a comment
Krishnadheeraj (DheerajPannala)
left a comment
There was a problem hiding this comment.
The package and the operator-facing contract still need to describe one consistent runtime before this is safe to follow.
11 things to address before this merges; details inline.
Since my last review:
- Fixed: Make meeting booking idempotent for repeated card submissions (
scenarios/scrum-master/src/handlers/chase.ts) - Still open: Do not interpret negated completion statements as Done (
scenarios/scrum-master/src/handlers/reconcile.ts). The original examples are now guarded, but the fix is incomplete. scenarios/scrum-master/src/handlers/reconcile.ts:88 omits "aren't" and "weren't". classifyUpdateText("We aren't done yet", false) stil
| - Azure App Service (Linux, Node 20 LTS). Set `WEBSITES_PORT=3978`. | ||
| - Configure application settings explicitly; do **not** copy the local `.env` blindly. Required cloud overrides: `NODE_ENV=production`, `HOST=0.0.0.0`, and a nonempty random `INTERNAL_TRIGGER_TOKEN`. Supply live Jira, Graph, LLM and hosting/auth connection settings securely; never commit secrets. | ||
| - Health check path: `/api/health`. | ||
| - Always-On: enabled for the long-lived agent. Use the sibling [`azure-functions/`](azure-functions) package for timer triggers and set `LOCAL_CRON=false` on the agent so only one scheduler is active. | ||
| - `.mstoken-cache.json` — for local dev only. In production, replace `graph.ts`'s file-backed MSAL cache with an Azure Key Vault-backed one (out of scope for this sample). | ||
|
|
||
| Example deployment commands (replace placeholders; no deployment validation is implied): | ||
|
|
||
| ```powershell | ||
| # From the scenario folder | ||
| npm run build | ||
| az webapp up --name <app-name> --resource-group <rg> --runtime "NODE|20-lts" |
There was a problem hiding this comment.
Minor: Deploy steps create a Node 20 App Service the package no longer supports
The same Deploy section that requires Node 22 then tells operators to provision Node 20. Following those commands creates an App Service runtime this package and the Agents SDK 0.7 pin refuse to support.
Evidence
package.json:7 sets engines.node to >=22. README.md:339 says the target is Azure App Service (Node 22 or later). README.md:343 then says Linux, Node 20 LTS, and README.md:354 runs az webapp up --runtime "NODE|20-lts". The azure-functions host can stay on Node 20 because it only posts to the agent; the agent process is the one that loads @openai/agents 0.7. An operator who copies the deploy commands gets a runtime the package.json engines field and the prerequisite table both reject.
[claude-opus-5.5] - scenarios/scrum-master/package.json:6-8 has "engines": { "node": ">=22" }, added in the latest commit.
- package.json:45 has
"@openai/agents": "^0.7.2". - README.md:46 has
| **Node.js ≥ 22** | Agent host | Required by the OpenAI Agents SDK; use a current LTS |. - README.md:339 has
Recommended target: **Azure App Service (Node 22 or later) + Azure Functions timers**.The same commit changed it from 'Node 18/20'. - README.md:343 has
- Azure App Service (Linux, Node 20 LTS). Set WEBSITES_PORT=3978.It is unchanged. - README.md:354 has
az webapp up --name <app-name> --resource-group <rg> --runtime "NODE|20-lts". It is unchanged.
Scenario: following 'Deploy to Azure' as written runs az webapp up ... NODE|20-lts, which provisions a Node 20 Linux App Service. engine-strict is off by default, so the install and deploy go through. The agent then runs on Node 20, which line 339 and the package's engines field say is unsupported.
Suggested fix: Change the App Service bullet and the az webapp up runtime to Node 22 LTS (NODE|22-lts), matching engines and the Node 22 sentence above. Leave the Functions host on its own runtime.
| const now = DateTime.now().setZone(tz); | ||
| const end = DateTime.fromISO(sprint.endDate).setZone(tz); | ||
| const daysRemaining = Math.ceil(end.diff(now, 'days').days); |
There was a problem hiding this comment.
Minor: Compare local calendar dates for the T-2 gate
This rounds an elapsed-time interval rather than calculating the documented local T-2 calendar day. With the default 00:30 IST nightly run and a sprint ending at 17:00, it skips the date two days before sprint end and sends the report the following day instead, reducing the intended planning window.
Evidence
scenarios/scrum-master/src/config.ts:107-109 configures 19:00 UTC nightly with Asia/Kolkata, and scenarios/scrum-master/src/cron/local-scheduler.ts:47-55 evaluates that schedule in UTC. For now=2026-11-11T00:30:00+05:30 and end=2026-11-13T17:00:00+05:30, scenarios/scrum-master/src/handlers/sprint-summary.ts:262 computes ceil(2.6875)=3 and skips. At 2026-11-12T00:30:00+05:30 it computes ceil(1.6875)=2 and sends, although that is only one calendar day before the end date.
Suggested fix: Compare the local calendar dates, or calculate the difference between their startOf('day') values in the configured timezone, so eligibility is the date of sprint end minus two days.
| if (!opts?.forceAlert) { | ||
| const existing = await findByTitle<SprintRiskFields>('sprintRisks', riskId).catch(() => null); | ||
| if (existing) { | ||
| console.log(`[warn] Risk already recorded today (${riskId}) — skipping notify.`); | ||
| return assessment; |
There was a problem hiding this comment.
Minor: Do not suppress a risk alert before its DM succeeds
The daily deduplication record is written before roster lookup and notification delivery. A transient failure after that write leaves a record that makes every normal retry return here without notifying the Scrum Master. This loses the day's alert even when the nightly request correctly fails and is retried.
Evidence
scenarios/scrum-master/src/handlers/warn.ts:113-120 creates the SprintRisks row, then :124 calls listTeamMembers and :143-145 sends the DM. If the roster lookup throws after the insert, the retry finds the same sprint/date key and returns at :107-109 before reaching delivery. A failed send has the same suppression problem because its result is ignored. The Functions retry at scenarios/scrum-master/azure-functions/src/index.ts:58 cannot recover that alert.
Suggested fix: Track notification delivery separately from risk detection, and suppress retries only after confirmed successful delivery. Keep failed notifications retryable and propagate delivery failure to the caller.
| function historyKey(context: TurnContext): string { | ||
| const from = context.activity.from; | ||
| return from?.aadObjectId ?? from?.id ?? 'anon'; |
There was a problem hiding this comment.
Minor: Keep Q&A history isolated to its conversation
The same user's personal, group and channel conversations share one history entry. A follow-up in a group therefore inherits messages from that user's private DM, and the resulting answer is posted to the group. Preserving personal proactive references elsewhere does not prevent this separate disclosure path.
Evidence
scenarios/scrum-master/src/handlers/answer.ts:50-52 keys history only by aadObjectId/from.id. Lines 69-75 prepend that history to the next request, and :78 sends the reply through the current conversation. scenarios/scrum-master/src/agent.ts:134-136 dispatches free text without a personal-conversation restriction. Thus two contexts with the same caller ID but different conversation IDs deterministically share private input and replies; a group-chat 'provide more details' can continue the private discussion.
Suggested fix: Include tenant, channel, conversation/thread and caller identifiers in the history key. Start with separate history when the conversation or audience changes.
| execute: async ({ issueKey }) => withExecuteToolScope('jira_get_issue', { issueKey }, async () => { | ||
| const jiraKey = toJiraKey(issueKey); | ||
| console.log(`[jira-tool] jira_get_issue received=${JSON.stringify(issueKey)} mapped=${jiraKey}`); | ||
| try { | ||
| const issue = await getJiraClient().getIssue(jiraKey); |
There was a problem hiding this comment.
Should fix: Restrict Jira Q&A reads to the configured project
JIRA_PROJECT_KEY changes friendly-label expansion but does not restrict these reads. A Teams caller can request an issue such as PRIVATE-123 and its comments, and both requests use the configured personal Jira token rather than that caller's Jira permissions. When the token owner has access to another restricted project, this exposes its content through a deployment intended for one project.
Evidence
scenarios/scrum-master/src/services/issue-labels.ts:40-42 passes arbitrary canonical project keys through unchanged. scenarios/scrum-master/src/services/jira-tool.ts:29-33 and :85-89 pass those keys directly to getIssue/getComments. scenarios/scrum-master/src/services/jira.ts:85-92 constructs Basic authentication from cfg.email/cfg.apiToken, and :170-174/:207-212 perform unrestricted issue reads. With JIRA_PROJECT_KEY=SCRUM and a configured token that can read PRIVATE-123, requesting PRIVATE-123 returns that project's fields/comments without any caller or project authorization check.
Suggested fix: At the shared Jira read boundary, verify that the resolved issue belongs to JIRA_PROJECT_KEY before returning its fields or comments. Cross-project access should require explicit authorization rather than key normalization alone.
| const res = await graph | ||
| .api(`/sites/${siteId}/lists/${listName(key)}/items?$expand=fields&$top=200`) | ||
| .get(); | ||
| return (res.value ?? []).map((v: any) => ({ id: v.id, fields: v.fields })); |
There was a problem hiding this comment.
Minor: Read all pages from SharePoint collection queries
The live collection readers return only the first Graph page. Once a list exceeds that page, later records silently disappear from the roster, helper matching or report data. This is especially relevant to the blocker list, which accumulates historical records without cleanup.
Evidence
scenarios/scrum-master/src/services/sharepoint.ts:171-174 requests $top=200 and returns res.value without following @odata.nextLink; findByField at :158-162 has the same omission. A valid response containing 200 records plus a continuation for record 201 produces a 200-record result. scenarios/scrum-master/src/services/team-roster.ts:102 uses that result as the complete roster, and scenarios/scrum-master/src/handlers/report.ts:320-326 uses it as the complete blocker collection. The mock branch returns all records and does not reproduce this truncation.
Suggested fix: Follow @odata.nextLink and aggregate every page in listItems and findByField, preserving the required query headers on subsequent requests.
| const kws = (row.Keywords ?? '') | ||
| .split(',') | ||
| .map(k => k.trim().toLowerCase()) | ||
| .filter(k => k.length >= 3); |
There was a problem hiding this comment.
Minor: Honor the bundled two-letter UX helper keyword
The bundled Design/UX roster explicitly includes 'ux', but this filter discards it before matching. A normal blocker such as 'Need UX input' therefore finds no helper, and the unblock meeting omits the configured UX contact.
Evidence
scenarios/scrum-master/src/scripts/seed-helper-roster.ts:58-61 seeds the Design/UX helper with the keyword 'ux'. scenarios/scrum-master/src/services/helperMatcher.ts:79-82 removes every keyword shorter than three characters, so 'Need UX input' matches none of the bundled rows and returns null at :95. scenarios/scrum-master/src/handlers/chase.ts:230-235 adds the helper attendee only when that result is non-null.
Suggested fix: Allow explicitly configured two-character keywords such as UX, using whole-word matching for short terms to avoid accidental substring matches.
| - **AMBER** — `dueDate <= today + AMBER_WINDOW_DAYS` and status not | ||
| `Done`/`In Review` |
There was a problem hiding this comment.
Minor: Documented amber rule excludes In Review, but classify() does not
Section 5.6 says an item is amber only when its due date is inside the window and its status is not Done or In Review. classify() treats every non-Done status the same, so an In Review item due today or within AMBER_WINDOW_DAYS is amber. Readers of the design will mis-explain the T-2 table.
Evidence
design.md:347-348: "AMBER — dueDate <= today + AMBER_WINDOW_DAYS and status not Done/In Review". sprint-summary.ts:60-72 sets Green only when status === 'Done'; otherwise daysFromEta <= AMBER_WINDOW_DAYS (2) is Amber. The report footer at sprint-summary.ts:230 says amber is "ETA within 2 days & pending", with no In Review exclusion. Scenario: status In Review, dueDate = today. The design says green; the posted table marks it amber.
Suggested fix: Drop the In Review exclusion so the design matches classify(), unless you also change classify() to skip In Review.
| - **AMBER** — `dueDate <= today + AMBER_WINDOW_DAYS` and status not | |
| `Done`/`In Review` | |
| - **AMBER** — `dueDate <= today + AMBER_WINDOW_DAYS` and status not `Done` |
| fanout, but this check is not atomic across processes. Failed finalization | ||
| stays `summarizing` to avoid an ambiguous duplicate post; inspect existing | ||
| records before manually recovering a run. |
There was a problem hiding this comment.
Minor: Design says a failed standup finalization stays summarizing
Section 9 tells operators that a failed standup finalization stays summarizing, so they should inspect existing records before recovering the run. That is not what a failed summary send does. claimSessionFinalization sets the in-memory state to summarizing, but summarizeStandup ignores deliverStandup's {ok:false} and then writes SharePoint State: 'summarized' and calls completeSession. SharePoint never has a summarizing value (open or summarized only). After a connector failure, or after a restart, there is no summarizing record to inspect, and the daily guard blocks another trigger.
Evidence
design.md:450-452: "Failed finalization stays summarizing to avoid an ambiguous duplicate post; inspect existing records before manually recovering a run." session-store.ts:64 sets session.state = 'summarizing' only in memory. standup.ts:534 and :544 call deliverStandup and discard {ok}. standup.ts:553-570 then sets session.state = 'summarized', updates the SharePoint row to State: 'summarized', and calls completeSession. sendProactive returns {ok:false} instead of throwing, so the common channel failure takes that path. Scenario: the channel connector fails the summary post; the operator looks for summarizing and instead finds a completed daily guard, so the summary is not retried.
Suggested fix: Replace the recovery sentence with the actual states. If the intended design is to hold summarizing and allow a retry, summarizeStandup must not mark the session or SharePoint row summarized when delivery returns {ok:false}.
| fanout, but this check is not atomic across processes. Failed finalization | |
| stays `summarizing` to avoid an ambiguous duplicate post; inspect existing | |
| records before manually recovering a run. | |
| fanout, but this check is not atomic across processes. A thrown failure | |
| before the summary write can leave the in-memory session `summarizing`; | |
| SharePoint only stores `open` or `summarized`. A connector failure that | |
| returns `{ok:false}` still marks the session summarized, so the daily guard | |
| blocks a retry. |
|
|
||
| | Column | Type | Description | | ||
| |---|---|---| | ||
| | `Title` | text | Friendly channel name | |
There was a problem hiding this comment.
Minor: Schema describes TeamsConfig.Title as a channel name
The TeamsConfig column reference says Title is a friendly channel name. The handler never stores the channel name there. It writes the literal primary and looks the row up by that title. Someone provisioning or inspecting the list from this doc will not find the row the summaries and reports actually use.
Evidence
sharepoint-schema.md:55: "Title | text | Friendly channel name". config.ts:21 sets PRIMARY = 'primary'. config.ts:67 writes Title: PRIMARY, and config.ts:75 loads the row with findByField('teamsConfig', 'Title', PRIMARY). standup.ts:531 and report.ts use the same lookup. A row titled General is invisible to those handlers.
Suggested fix: Document Title as the single-row lookup key primary. Channel identity belongs in TeamId and ChannelId.
| | `Title` | text | Friendly channel name | | |
| | `Title` | text | Single-row lookup key. Handlers write and read the literal `primary`, not the channel display name | |
Add scrum-master scenario: Scrum Master autopilot using Node.js SDK
What this adds
A scenario extension under
scenarios/scrum-master/— an autonomous Scrum Master built on the Microsoft Agent 365 SDK + OpenAI Agents SDK that runs a scrum team's ceremonies end-to-end: daily standups, board reconciliation, blocker chase, mid-sprint risk warnings, grounded Q&A, and the sprint close report. Complements the existing Chief-of-Staff scenario (#333) by covering the delivery-team side of leader / squad workflows.Capabilities
Seven flows, each a handler in
src/handlers/:SMA_HelperRoster→ MCP calendar tool books an unblock meeting on the agent's mailboxFull per-flow mermaid sequence diagrams:
scenarios/scrum-master/docs/design.md.Design principles
Verbatim from
docs/design.md:setImmediate.SMA_*lists survive restarts.What's included
Runs offline
JIRA_MODE=mock(the default) drives the sample against a mutable in-memory Jira sprint insrc/mock/jira-mock.ts. Contributors can clone,npm install,npm run dev,npm run test-tooland try/standupin the Agents Playground without an Atlassian account, without a SharePoint site, and without a Teams tenant.Live mode (
JIRA_MODE=live) requires:Auth model — delegated only
All Microsoft Graph calls use MSAL device-code + delegated scopes (
Sites.ReadWrite.All,Sites.Manage.All,Files.ReadWrite.All). Onboarding is a single interactive sign-in. No application-permission client credentials, no admin consent, no Teams policies to provision.Verification
00000000-…,<your-org>.atlassian.net,alice@contoso.cometc.Placement — why
scenarios/Placed at the repo root under
scenarios/scrum-master/rather than under a language tier (e.g.nodejs/openai/) because a scrum-master workflow doesn't map cleanly to a single SDK — it composes Agent 365 SDK, OpenAI Agents SDK, MCP Calendar, and multiple external services. This mirrors how the Chief-of-Staff sample (#333) is placed. Happy to relocate tonodejs/openai/scenarios/scrum-master/if reviewers prefer a language-tier scoping.Not included / future work
SMA_Teamslist +TeamIdcolumn + per-team Jira credentials in Key Vault is documented indocs/design.md..mstoken-cache.json, gitignored). Production swap-in is called out indocs/design.md.tests/e2e/. Recommend follow-up PR once initial review lands.Commits
d33aecc— initial import of the scenario code (57 files)feb98d9— Jira sample-data seed script + SharePoint schema doc1c5a4af— README rewrite (intro-features-prereqs-setup structure)62b0cad— observability + docs polish (logger, http-logger, startup banner, process safety nets, design.md, TOC, troubleshooting, deploy-to-Azure)Related
nodejs/openai/sample-agent