Skip to content

Add career-coach scenario: Career Coach autopilot using Node.js SDK - #338

Open
Akshit-MSFT (Akshit0022) wants to merge 6 commits into
microsoft:mainfrom
Akshit0022:scenario/career-coach
Open

Akshit-MSFT (Akshit0022) wants to merge 6 commits into
microsoft:mainfrom
Akshit0022:scenario/career-coach

Conversation

@Akshit0022

@Akshit0022 Akshit-MSFT (Akshit0022) commented Aug 23, 2026 •

Copy link
Copy Markdown

Add career-coach scenario: Career Coach autopilot using Node.js SDK

What this adds

A scenario extension under scenarios/career-coach/ - a private AI Career Coach built on the Microsoft Agent 365 SDK + OpenAI Agents SDK that runs an employee's growth journey end-to-end as an Agent 365 AI Teammate (its own M365 identity and mailbox), in Microsoft Teams and by email: goal setting, skill-gap mapping, course recommendations, proactive post-course quizzes, milestone nudges, and a manager wrap-up email. Complements the Chief-of-Staff (#333) and Scrum-Master (#334) scenarios by covering the talent-development / employee-growth side of leader / squad workflows.

Capabilities

Each flow is a deterministic handler in src/handlers.ts, with focused LLM sub-calls in src/llm-tasks.ts:

  • Set goals - user names a target role, then gets a skill-path Adaptive Card with per-skill self-assessment dropdowns
  • Map skills - deterministic gap analysis against CompetencyFramework_v2 + course recommendations from LearningCatalog_v2 (planReview card)
  • Save plan - writes the plan (goals, skills, course list) to UserState, one row per goal (Active / Completed / Replaced) so finished goals are kept as history
  • Sync progress - diffs LearningPortalStatus against the plan and recomputes progress
  • Proactive quiz - a Microsoft Graph change-notification subscription fires when the portal marks a course complete, and the agent DMs a 5-question quiz (3 MCQ graded in code, 2 short-answer graded by a focused LLM call)
  • 80% milestone - one-shot card surfacing the weakest topic areas aggregated across quiz attempts
  • 100% completion - completion email from the agent's mailbox (To: user, Cc: manager); the goal is marked Completed only after a successful send
  • 1:1 prep brief - STAR-format wins + talking points on demand, including for finished goals
  • Email channel - the same coach by email: thread-scoped history, plans as HTML tables, reply "Save it" to save (same code as the Teams button); org members only
  • WorkIQ MCP tools - Mail and Calendar (V2) for user-requested actions ("email me my plan", "find time for my 1:1")

Full flows, data model and design decisions: scenarios/career-coach/docs/design.md and AGENT-CODE-WALKTHROUGH.md.

Design principles

  1. The LLM talks; code decides. The LLM is gated to the paths that genuinely need language understanding (conversation, quiz-question generation, short-answer grading, completion-email prose). Every card submit, data write, MCQ grade, milestone rule, and email dispatch is plain TypeScript.
  2. Rules are enforced, not requested. Tool-level guards reject disallowed writes: the model cannot mark a course Complete, record a passed quiz, raise a skill level, edit a finished goal, create a second active goal, or switch role on a goal with progress. SharePoint tools are scoped to the caller's own rows.
  3. One source of truth. State lives in five SharePoint lists; Teams and email read and write the same rows.
  4. Card actions never double-fire. Every Action.Execute handler acks fast and runs deferred; cards lock after use and in-flight claims absorb double clicks.
  5. Never go silent; degrade, don't die. A failed background step tells the user what failed and unlocks the card only when retrying is safe. Optional capabilities (MCP servers, telemetry, the webhook) can fail without stopping a turn; A365 lifecycle events are consumed by a top-priority route.
  6. Least privilege. The agent acts as its own agentic user; WorkIQ SharePoint is read-only; email goes only to the requesting user (and their manager on request or on completion).

What's included

scenarios/career-coach/
  README.md                     - Prereqs, install, SharePoint setup, dev tunnel, register/publish, E2E walkthrough, troubleshooting, auth, deploy
  AGENT-CODE-WALKTHROUGH.md      - Source-level tour in request order
  docs/
    design.md                   - Principles, architecture, data model + goal lifecycle, flows, security, reliability, observability, decisions
  src/                          - agent, cards, handlers, service layer, llm-tasks, graph-service, sharepoint tools + guards, email channel
    telemetry.ts                - @microsoft/opentelemetry bootstrap (imported first)
    observability/
      app-token-resolver.ts     - App-only S2S export token for the runtime agent identity
      conversation-id.ts        - Stable span conversation id (email threads)
    scripts/
      setup-sharepoint.ts       - Idempotent provisioning of all 5 lists (adds new lifecycle columns on re-run)
      seed-reference-data.ts    - Competency framework + learning catalog from CSVs
      mark-courses-complete.ts  - Simulate portal completions (drives Feature 1)
      reset-milestones.ts       - Re-arm the 80% / 100% cards
      backup-and-reset-user.ts  - Snapshot + clear a user's write-lists for a clean re-run
      clear-list.ts             - Empty a list
  ToolingManifest.json          - WorkIQ MCP servers (Mail, Calendar)
  SharePoint Data/              - Seed CSVs (19 competencies + 31 courses)
  manifest/                     - Teams app + agentic-user templates (placeholder GUIDs)
  images/thumbnail.png
  package.json                  - Scripts: dev, build, test-tool, setup:sharepoint, seed:reference, mark:complete, reset:milestones, backup:user
  tsconfig.json
  .env.template                 - Every env var with WHY comments
  .gitignore

Local testing

The conversational path (goal setting, skill ratings, gap analysis, 1:1 prep) runs locally against the Agents Playground: npm install, npm run dev, npm run test-tool. The full data-backed loop (save plan, proactive quiz, milestones, completion email, email channel) requires:

  • Azure OpenAI or OpenAI API key (gpt-4o recommended)
  • A SharePoint site with the five lists (npm run setup:sharepoint), and the agent user added as a site Member
  • A public dev-tunnel URL for the messaging endpoint and the Graph change-notification webhook (Feature 1)
  • An Agent 365 registration: a365 setup all --aiteammate, a365 setup blueprint --update-endpoint <url>/api/messages --m365, a365 publish --aiteammate, then an approved, licensed agent instance

Auth model

  • Runtime workload - agentic auth: the A365 platform hands the agent a delegated Graph token for its own agentic user per turn (/me is the Career Coach agent user and mailbox). SharePoint, the completion email and WorkIQ MCP use this identity. No application-permission client secret is used for Graph.
  • Telemetry - exported over the S2S route with an app-only token for the runtime agent identity (blueprint FMI assertion → agent identity → Observability API), validated and never logged. gen_ai.agent.id is the runtime identity from the turn; the blueprint id is metadata only.
  • Setup / seed scripts - MSAL device-code + delegated scopes (Sites.Manage.All, Sites.ReadWrite.All, User.Read.All, Mail.Send, offline_access). Sites.Manage.All and User.Read.All are admin-restricted, so a tenant/SharePoint admin performs - or pre-consents - the one-time sign-in. The blueprint's Sites.ReadWrite.All grant and the WorkIQ MCP grants (a365 setup permissions mcp) also need a Global Administrator.

Observability

@microsoft/opentelemetry with the Agent 365 exporter on the S2S route. Every path - LLM turns, card actions, typed shortcuts, email and proactive turns - emits invoke_agent (input/output messages), chat (model/provider, finish reason) and execute_tool spans with caller, channel, session and conversation id. OpenAI Agents auto-instrumentation adds token usage; no fabricated token counts.

Verification

  • Set up on a fresh M365 tenant (new blueprint, AI Teammate instance, SharePoint site); the agent responds in Teams over a dev tunnel with SharePoint state, WorkIQ MCP and the S2S telemetry token working. Live-tenant evidence follows in the validation-evidence comment.
  • Clean npm install (no lockfile) → npm run build and npx tsc --noEmit pass.
  • a365-code-validator static scan: 0 findings.
  • Secret scan clean - no tenant IDs, no client secrets, no PATs, no persona bleed. All placeholders are 00000000-..., contoso.sharepoint.com, <CHANGE_ME_...> etc.
  • All source has MIT copyright headers.
  • CLA signed.

Placement - why scenarios/

Placed at the repo root under scenarios/career-coach/ rather than under a language tier (e.g. nodejs/openai/) because a career-coach workflow doesn't map cleanly to a single SDK - it composes the Agent 365 SDK, OpenAI Agents SDK, SharePoint, and Microsoft Graph. This mirrors how the Chief-of-Staff (#333) and Scrum-Master (#334) samples are placed. Happy to relocate to nodejs/openai/scenarios/career-coach/ if reviewers prefer a language-tier scoping.

Not included / future work

  • Shared storage for multi-instance hosting. The quiz answer-key cache is in memory, and the proactive map, email-plan store and subscription state are local files - fine for a single instance; move them to shared storage (e.g. Redis / Cosmos DB) before scaling out.
  • Production Key Vault-backed MSAL cache for the setup scripts. Local dev uses a file cache (.mstoken-cache.json, gitignored).
  • E2E test suite under tests/e2e/. Recommend a follow-up PR once initial review lands.

Related

@Akshit0022
Akshit-MSFT (Akshit0022) marked this pull request as ready for review August 23, 2026 21:58
@Akshit0022
Akshit-MSFT (Akshit0022) requested a review from a team as a code owner August 23, 2026 21:58
Copilot AI lite review requested due to automatic review settings August 23, 2026 21:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new Career Coach scenario under scenarios/career-coach/, expanding the repo’s scenario samples with a Node.js/TypeScript Agent 365 agent that uses SharePoint Lists + Microsoft Graph for durable plan/progress state and proactive flows.

Changes:

  • Introduces a full Career Coach agent implementation (LLM path + deterministic handlers) with SharePoint-backed state, quizzes, milestones, and completion email flow.
  • Adds SharePoint provisioning/seed/reset scripts plus reference CSV datasets for competency framework and learning catalog.
  • Adds scenario documentation and Teams manifest/assets for sideloading/testing.

Reviewed changes

Copilot reviewed 36 out of 39 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
scenarios/career-coach/.env.template Environment variable template for OpenAI/Azure OpenAI, A365 identity/auth, SharePoint, and webhook settings
scenarios/career-coach/.gitignore Scenario-local ignore rules for build artifacts, caches, state, and logs
scenarios/career-coach/AGENT-CODE-WALKTHROUGH.md Walkthrough documentation for the scenario code
scenarios/career-coach/README.md End-to-end setup and testing instructions for the scenario
scenarios/career-coach/ToolingManifest.json MCP server manifest for tooling integration
scenarios/career-coach/package.json Node/TS dependencies and scripts for dev/build and SharePoint helpers
scenarios/career-coach/tsconfig.json TypeScript compiler configuration
scenarios/career-coach/verify-userstate.ps1 PowerShell helper to inspect UserState list contents via Graph
scenarios/career-coach/SharePoint Data/CompetencyFramework_v2.csv Seed data: role → competency framework
scenarios/career-coach/SharePoint Data/LearningCatalog_v2.csv Seed data: course catalog mapped to competencies
scenarios/career-coach/docs/design.md Design/architecture documentation for the scenario
scenarios/career-coach/images/thumbnail.png Scenario thumbnail asset
scenarios/career-coach/manifest/manifest.json Teams app manifest (placeholder IDs)
scenarios/career-coach/manifest/agenticUserTemplateManifest.json Agentic user template manifest
scenarios/career-coach/manifest/color.png Teams icon asset
scenarios/career-coach/manifest/outline.png Teams icon asset
scenarios/career-coach/src/index.ts Express host: health check, /api/messages, proactive /api/portal-event, startup wiring
scenarios/career-coach/src/agent.ts AgentApplication wiring, lifecycle routing guard, message + notification handlers, adaptive card action handlers
scenarios/career-coach/src/client.ts OpenAI Agents client wrapper, system prompt, SharePoint function-tools integration, observability scopes
scenarios/career-coach/src/cards.ts Adaptive Card rendering utilities and payload helpers
scenarios/career-coach/src/career-coach-types.ts Shared data model types + SharePoint configuration constants
scenarios/career-coach/src/career-coach-service.ts Deterministic service layer: SharePoint CRUD, progress logic, grading logic, milestone aggregation
scenarios/career-coach/src/handlers.ts Deterministic handlers for card submits, progress sync, quiz flows, milestones, and completion email
scenarios/career-coach/src/llm-tasks.ts Focused LLM subcalls for quiz generation, short-answer grading, and completion email prose
scenarios/career-coach/src/openai-config.ts OpenAI vs Azure OpenAI client configuration
scenarios/career-coach/src/graph-service.ts Graph client setup (MSAL device-code for scripts + agentic token exchange for runtime), subscriptions, mail helpers
scenarios/career-coach/src/sharepoint-tools.ts Graph-backed SharePoint tools exposed as OpenAI function tools (site/list ID resilience, column mapping)
scenarios/career-coach/src/sharepoint-column-map.ts SharePoint display-name ↔ internal-name mapping and defensive field coercions
scenarios/career-coach/src/subscription-manager.ts Create/renew Graph list change-notification subscription with local persisted state
scenarios/career-coach/src/proactive-refs.ts Disk-backed mapping of AAD object IDs to proactive conversation IDs
scenarios/career-coach/src/file-storage.ts Disk-backed Storage implementation for Proactive subsystem restart resilience
scenarios/career-coach/src/quiz-cache.ts In-memory cache of generated quiz questions/answer keys for deterministic grading
scenarios/career-coach/src/token-cache.ts Custom in-memory token cache + token resolver for observability exporter
scenarios/career-coach/src/scripts/setup-sharepoint.ts Script: idempotent provisioning of SharePoint lists + columns
scenarios/career-coach/src/scripts/seed-reference-data.ts Script: seed reference lists from CSVs
scenarios/career-coach/src/scripts/mark-courses-complete.ts Script: simulate portal completions by inserting LearningPortalStatus rows
scenarios/career-coach/src/scripts/reset-milestones.ts Script: reset milestone guards for re-demos
scenarios/career-coach/src/scripts/backup-and-reset-user.ts Script: snapshot + delete a user’s rows across write-lists
scenarios/career-coach/src/scripts/clear-list.ts Script: delete all rows from a specified list

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread scenarios/career-coach/src/agent.ts Resolved
Comment thread scenarios/career-coach/package.json Resolved
Comment thread scenarios/career-coach/docs/design.md Resolved

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 36 out of 39 changed files in this pull request and generated 1 comment.

Suppressed comments (8)

Previously missed (4) — in code that hasn't changed since the last review.

scenarios/career-coach/verify-userstate.ps1:40

  • The Graph list-items request uses expand=fields, but Microsoft Graph expects $expand=fields. As written, item.fields won't be expanded and the script will fail when it tries to read $item.fields.
    scenarios/career-coach/src/token-cache.ts:30
  • The TokenCache comment says it has "expiration handling", but the implementation never expires entries. This is misleading for readers and callers (especially since the resolver may be hit frequently).
    scenarios/career-coach/src/token-cache.ts:42
  • TokenCache.set() logs on every cache write. Since the observability SDK may refresh tokens frequently, this can create noisy logs and unnecessary overhead. Gate this behind an env var (or remove it).
    scenarios/career-coach/src/token-cache.ts:53
  • TokenCache.get() logs on every cache miss, which can be very frequent and flood logs in normal operation. Gate the miss log behind an env var (or remove it).

scenarios/career-coach/src/career-coach-types.ts:4

  • This new TypeScript source file is missing the required Microsoft copyright header, and the opening comment still references an MCP server even though the scenario now uses Graph-backed tools.
// Career Coach data types
// These types define the structure of data in SharePoint Lists
// accessed via the SharePoint MCP Server

scenarios/career-coach/package.json:45

  • This package uses Express v5 (express: ^5.1.0) but pins @types/express to v4. That mismatch can cause incorrect typings and TypeScript compile errors. Use the Express v5 type package version (as other samples in this repo do).
  "devDependencies": {
    "@microsoft/m365agentsplayground": "^0.2.18",
    "@types/express": "^4.17.21",
    "@types/node": "^20.14.9",

scenarios/career-coach/docs/design.md:5

  • This design doc appears to be the generic OpenAI sample-agent template (title and overview) rather than Career Coach–specific content. Since the PR description points readers here for Career Coach architecture/flows, the opening section should be updated to match this scenario.
# OpenAI Sample Agent Design (Node.js/TypeScript)

## Overview

This sample demonstrates an agent built using the official OpenAI Agents SDK for Node.js. It showcases TypeScript patterns, MCP server integration, notification handling, and Microsoft Agent 365 observability.

scenarios/career-coach/AGENT-CODE-WALKTHROUGH.md:22

  • The file-structure diagram is still for sample-agent/ and omits many Career Coach modules (handlers/service layer/SharePoint scripts). This makes the walkthrough misleading for contributors trying to navigate the scenario code.
sample-agent/
├── src/
│   ├── agent.ts               # Main agent implementation (~60 lines)
│   ├── client.ts              # OpenAI client wrapper with observability
│   └── index.ts               # Express server entry point

Comment thread scenarios/career-coach/src/index.ts Resolved

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 35 out of 38 changed files in this pull request and generated 16 comments.

Suppressed comments (2)

scenarios/career-coach/src/career-coach-types.ts:3

  • This new TypeScript source file is missing the required Microsoft copyright and MIT license header.
// Career Coach data types
// These types define the structure of data in SharePoint Lists
// accessed via Microsoft Graph (agentic delegated auth)

scenarios/career-coach/src/scripts/seed-reference-data.ts:147

  • Skipping whenever any item exists makes the seeder non-resumable. If one row fails after earlier rows were created, the next run sees a non-empty list and skips it, leaving the reference catalog permanently incomplete unless the developer deletes all successful rows. Seed missing rows by the stable role/course key, or verify the full expected key set before declaring the list complete.

Comment thread scenarios/career-coach/src/llm-tasks.ts Resolved
Comment thread scenarios/career-coach/src/handlers.ts Resolved
Comment thread scenarios/career-coach/src/handlers.ts Resolved
Comment thread scenarios/career-coach/src/index.ts Resolved
Comment thread scenarios/career-coach/src/handlers.ts Resolved
Comment thread scenarios/career-coach/src/client.ts Resolved

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 35 out of 38 changed files in this pull request and generated 1 comment.

Suppressed comments (14)

Previously missed (9) — in code that hasn't changed since the last review.

scenarios/career-coach/src/handlers.ts:623

  • This automatically Cc's the manager with the employee's target role, courses, and time investment as soon as progress reaches 100%, without any consent state or confirmation action. That contradicts the agent's own rule in src/client.ts:101 (“Never share with managers without explicit consent”) and exposes private career data. Gate manager delivery on an explicit, persisted user choice; otherwise send only to the employee.
    if (managerEmail && managerEmail.toLowerCase() !== (userEmail ?? '').toLowerCase()) cc.push(managerEmail);
    // Fallback: if we somehow lack a userEmail, still send to the manager only.
    if (to.length === 0 && cc.length > 0) { to.push(cc[0]); cc.length = 0; }

    if (to.length === 0) {
        note = 'No manager or user email found — the email was drafted but not sent.';
    } else {
        try {
            await sendMail({ to, cc, subject, htmlBody, fromUserId: userAADId, graph });

scenarios/career-coach/src/agent.ts:147

  • The quiz work is started without an in-flight/idempotency guard. Two rapid clicks or a retried invoke can both read the cache before clearQuiz, then append duplicate QuizResponses, update UserState concurrently, send duplicate cards, and potentially dispatch the completion email twice. Claim an idempotency key (for example user/course plus activity ID) before launching the deferred handler and reject or join duplicates.
        // Fire-and-forget so the invoke returns fast (LLM short-answer grading + writes
        // can take ~5-10s; Teams' invoke timeout is ~10s and shows a red banner if we exceed it).
        handleQuizSubmit(context, this.authorization, {
          userAADId,
          courseId,
          skillId,
          answersByQuestionId,
        }).catch((err) => {
          console.error('[Quiz] Deferred handleQuizSubmit failed:', (err as any)?.message ?? err);
        });

scenarios/career-coach/src/subscription-manager.ts:99

  • Renewal matches only URL and resource, so changing PORTAL_EVENT_SECRET keeps the old subscription and its old clientState. Graph then continues sending the old value while the endpoint compares against the new secret, causing every notification to return 401 until the subscription expires or is manually removed. Persist a non-reversible fingerprint of the configured clientState and recreate the subscription when it changes.
    scenarios/career-coach/.gitignore:24
  • subscription-manager.ts creates .sp-subscription.json, but this generated file is not ignored. It contains the subscription ID, SharePoint resource identifiers, and tunnel URL, so a normal local run can accidentally commit tenant-specific runtime state. Add it to the runtime-state ignore list.
# runtime state + backups
.proactive-storage.json
.proactive-refs.json
backups/

scenarios/career-coach/src/career-coach-types.ts:3

  • This TypeScript source file is the only file in src/ missing the required Microsoft copyright and MIT license header; compare src/agent.ts:1-2 and src/handlers.ts:1-2. Add the standard header before the file description.
// Career Coach data types
// These types define the structure of data in SharePoint Lists
// accessed via Microsoft Graph (agentic delegated auth)

scenarios/career-coach/.env.template:26

  • The Node.js sample configuration is missing the required ENABLE_A365_OBSERVABILITY switch and leaves the required observability log variables blank/commented. The sample template requires active defaults for all four observability settings so local and deployed behavior is explicit.
# --- A365 observability ------------------------------------------------------
# Enable the observability exporter. Default false = console-only exporter.
# MUST be true in any deployed / dev-tunnel environment or MAC Activity stays empty.
ENABLE_A365_OBSERVABILITY_EXPORTER=false
# When true, the sample demos a custom token resolver + token cache; otherwise it
# uses the built-in AgenticTokenCache from the observability hosting package.
Use_Custom_Resolver=true
# Optional exporter logging. One of 'info' | 'warn' | 'error'. Defaults to silent.
# Uncomment OTEL_LOG_LEVEL too when debugging export failures.
A365_OBSERVABILITY_LOG_LEVEL=
# OTEL_LOG_LEVEL=INFO

scenarios/career-coach/verify-userstate.ps1:10

  • The comment says this configuration matches .env, but only the host does; custom SP_SITE_PATH and SP_LIST_USER_STATE values are ignored. Users following the README's supported rename/custom-site path will query the wrong site or list. Read all three values from the corresponding environment variables.
    scenarios/career-coach/README.md:50
  • The PR description says no admin consent is required, but this README correctly requires admin consent for delegated Sites.ReadWrite.All and User.Read.All, and graph-service.ts requests those scopes. These are materially different setup requirements; update the PR description/auth claims to match the implementation and README.
**Microsoft Entra (Azure AD) app registration for the agent**
- The A365 platform provisions one for you when you create a hosted-agent project. That app's client ID + secret + tenant ID go into `.env` under `connections__service_connection__settings__*`.
- **Required delegated Graph scopes (with admin consent):**
  - `Sites.ReadWrite.All`
  - `User.Read.All`

scenarios/career-coach/src/index.ts:155

  • Every notification rescans a ten-minute window and treats all rows in that window as affected. If user A completes a course and user B completes one five minutes later, B's notification causes both A and B to be messaged again; Graph retries produce the same duplication. Track the actual changed item/resource data when available, or persist a per-item LastUpdated watermark/idempotency key so each portal update is processed once.
  const usersToNotify = new Map<string, any>();
  for (const row of rows) {
    const f = row.fields || {};
    const lastUpdated = f.LastUpdated ? new Date(f.LastUpdated) : null;
    if (!lastUpdated || isNaN(lastUpdated.valueOf())) continue;
    if (lastUpdated < cutoff) continue;
    const aad = String(f.UserAADId || '').trim();
    if (!aad) continue;
    // Keep the newest row per user (avoid double-DM if a user has multiple recent changes).
    const existing = usersToNotify.get(aad);
    if (!existing || new Date(existing.LastUpdated ?? 0) < lastUpdated) usersToNotify.set(aad, f);

scenarios/career-coach/src/career-coach-service.ts:53

  • These runtime read helpers receive an agentic Graph client, but site/list resolution calls getSiteId() and getListIdByName(), which each create the file-backed MSAL developer client. Consequently normal and proactive turns still require .mstoken-cache.json, fail after that cache expires or in deployment, and resolve metadata under the developer rather than the current agentic identity. Thread the supplied client through all site/list lookup helpers so the runtime path is actually agentic-only.
export async function readCompetencyFramework(graph: MsGraphClient, siteId?: string): Promise<CompetencyFrameworkRow[]> {
    const site = siteId ?? await getSiteId();
    const listId = await getListIdByName(site, SP_CONFIG.lists.competencyFramework);

scenarios/career-coach/src/llm-tasks.ts:100

  • The prompt requests five questions, but this code only truncates the response; it accepts zero to four questions, invalid MCQ choice counts, and missing answer keys. Since passing is hard-coded as score >= 4, a short model response can make the quiz impossible to pass. Validate exactly five questions (including the 3/2 type split, four choices, and nonempty keys) and retry or use a deterministic fallback before caching/rendering it.
    const json = await chatJson<{ questions: QuizQuestionWithKey[] }>(system, user, { temperature: 0.7 });
    const questions = Array.isArray(json?.questions) ? json.questions : [];
    // Normalize/fill missing fields defensively.
    return questions.slice(0, 5).map((q, i) => ({

scenarios/career-coach/src/scripts/backup-and-reset-user.ts:106

  • Each backupAndClearList call deletes rows immediately, while the backup file is not written until all three calls finish. A later Graph error, process interruption, or filesystem failure therefore leaves already-deleted data with no backup, contrary to the command's stated guarantee. Read all three datasets and atomically persist the snapshot first; only then begin deletions, reporting any partial cleanup separately.
        lists: {
            userState:            await backupAndClearList(graph, siteId, SP_CONFIG.lists.userState,            userAADId),
            learningPortalStatus: await backupAndClearList(graph, siteId, SP_CONFIG.lists.learningPortalStatus, userAADId),
            quizResponses:        await backupAndClearList(graph, siteId, SP_CONFIG.lists.quizResponses,        userAADId),

scenarios/career-coach/README.md:17

  • The sample README does not follow the repository README template: it starts with a table of contents and omits or reorders required sections such as “What This Sample Demonstrates,” “Authentication + Identity,” “Environment Configuration,” “Deploying the Agent,” “Observability,” and “Additional Resources.” Restructure it to the mandated section order while preserving the scenario-specific setup and walkthrough details.
## Table of contents

1. [Prerequisites](#1-prerequisites)
2. [Clone + install](#2-clone--install)
3. [Configure `.env`](#3-configure-env)

scenarios/career-coach/src/index.ts:21

  • Continuing after an uncaughtException is unsafe because Node may be left with partially mutated application or storage state; this handler also prevents a process supervisor from restarting the service cleanly. Log the exception and terminate with a nonzero status instead of claiming the server remains usable.
process.on('uncaughtException', (err) => {
  console.error('Uncaught exception (ignored, server stays up):', err?.message ?? err);
});

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 35 out of 38 changed files in this pull request and generated 5 comments.

@Akshit0022 Akshit-MSFT (Akshit0022) left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 35 out of 38 changed files in this pull request and generated 4 comments.

Suppressed comments (8)

Previously missed (1) — in code that hasn't changed since the last review.

scenarios/career-coach/src/subscription-manager.ts:176

  • After a subscription expires or is deleted, renewal returns 404 forever because each interval retries the same persisted ID. The proactive flow remains disabled until the process restarts. Detect terminal renewal failures, discard the stale state, and create a fresh subscription while retaining retry behavior for transient failures.

scenarios/career-coach/src/career-coach-service.ts:53

  • This read path receives an agentic Graph client, but resolves both IDs through helpers that construct the file-cached MSAL client. A deployed runtime without .mstoken-cache.json therefore fails before the agentic client is used. Pass the supplied client through ID resolution (and do the same in the other runtime CRUD helpers) so runtime access is actually agentic-only.
export async function readCompetencyFramework(graph: MsGraphClient, siteId?: string): Promise<CompetencyFrameworkRow[]> {
    const site = siteId ?? await getSiteId();
    const listId = await getListIdByName(site, SP_CONFIG.lists.competencyFramework);

scenarios/career-coach/src/sharepoint-tools.ts:238

  • These optional filters make the generic tool capable of returning every employee's UserState or QuizResponses row. Prompt instructions are not an authorization boundary, so a manipulated model call can expose private career data. Derive the caller's AAD ID from RunCtx and enforce owner scoping for sensitive-list reads and writes in code.
    scenarios/career-coach/src/agent.ts:270
  • The save action has no in-flight or idempotency guard. Two clicks can run concurrent read-then-create operations, both observe no existing row, and create duplicate UserState records; later reads then select an arbitrary duplicate. Add a server-side operation/idempotency key and disable or deduplicate repeated submissions before starting this detached work.
        handleSavePlanSubmit(context, this.authorization, {
          userAADId, displayName, roleTitle, targetRoleId, groups, ratings,
        }).catch((err) => {
          console.error('[SavePlan] Deferred save failed:', (err as any)?.message ?? err);

scenarios/career-coach/src/index.ts:150

  • Every notification received during this ten-minute window selects the same recently updated rows again. Graph retries or another user's update therefore resend quizzes to users already notified, while their plan remains quiz-pending. Persist a per-row/version watermark (or otherwise deduplicate notification processing) instead of using a sliding wall-clock window.
    const lastUpdated = f.LastUpdated ? new Date(f.LastUpdated) : null;
    if (!lastUpdated || isNaN(lastUpdated.valueOf())) continue;
    if (lastUpdated < cutoff) continue;

scenarios/career-coach/src/scripts/backup-and-reset-user.ts:74

  • Rows are deleted before any backup file is written. If fetching a later list, deleting a later row, creating the directory, or writing the JSON fails, earlier data has already been removed with no snapshot on disk. Fetch all three lists and atomically persist the complete backup before beginning deletion.
    // Delete each row.
    let ok = 0, fail = 0;
    for (const row of mine) {
        try {
            await graph.api(`/sites/${siteId}/lists/${listId}/items/${row.id}`).delete();

scenarios/career-coach/src/llm-tasks.ts:100

  • Slicing only caps the response; it does not enforce five valid questions, three MCQs, four choices, unique IDs, or nonempty answer keys. A malformed/short LLM response can render an impossible quiz while passing still requires four correct answers. Validate the complete schema and retry or fail gracefully before caching/rendering it.
    const json = await chatJson<{ questions: QuizQuestionWithKey[] }>(system, user, { temperature: 0.7 });
    const questions = Array.isArray(json?.questions) ? json.questions : [];
    // Normalize/fill missing fields defensively.
    return questions.slice(0, 5).map((q, i) => ({

scenarios/career-coach/README.md:18

  • The sample README does not follow the repository README template: it omits the required standalone “What This Sample Demonstrates,” “Authentication + Identity,” “Environment Configuration,” “Deploying the Agent,” “Observability,” “Additional Resources,” “Trademarks,” and “License” sections and their prescribed order. Restructure the document while preserving the scenario-specific setup and walkthrough details.
## Table of contents

1. [Prerequisites](#1-prerequisites)
2. [Clone + install](#2-clone--install)
3. [Configure `.env`](#3-configure-env)
4. [First-time SharePoint setup](#4-first-time-sharepoint-setup)

Comment thread scenarios/career-coach/src/agent.ts Resolved
Comment thread scenarios/career-coach/.gitignore Resolved
Comment thread scenarios/career-coach/src/index.ts Resolved
Comment thread scenarios/career-coach/src/agent.ts Resolved
An Agent 365 AI Teammate that helps employees set a target role, maps skill gaps, recommends courses, quizzes them proactively via Graph change notifications, and drafts the manager wrap-up email. Hybrid pro-code: deterministic TypeScript for all card submits/writes/business rules; focused LLM sub-calls for conversation, quiz generation, grading, and email prose. SharePoint Lists for state, Microsoft Graph (agentic delegated) for triggers + mail. Includes Copilot review fixes: idempotent quiz submit, no-tools email summarization (prompt-injection safe), real token usage, admin-consent scope docs. Placeholder GUIDs; no secrets or tenant-specific values.

@Akshit0022 Akshit-MSFT (Akshit0022) left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve updated the PR based on all the review points and addressed the comments.

…, WorkIQ tools

Brings the career-coach scenario up to the current Agent 365 SDK and
observability pattern (matches microsoft#339), and adds the features built since
the first revision.

Observability
- Migrate from the preview @microsoft/agents-a365-observability* packages
  to the @microsoft/opentelemetry distro (src/telemetry.ts).
- Export over the S2S route with an app-only token for the runtime agent
  identity (src/observability/app-token-resolver.ts: blueprint FMI
  assertion -> agent identity -> Observability API; validated, cached,
  never logged). Removes the delegated token cache (src/token-cache.ts).
- gen_ai.agent.id comes from activity.recipient.agenticAppId; blueprint id
  is metadata only. invoke_agent / chat / execute_tool spans on every
  path (LLM turns, card actions, typed shortcuts, email, proactive),
  with channel, caller and a stable conversation id (email threads).

Features
- Email channel: agent converses by email (org members only, bounces
  ignored), thread-scoped history, "Save it" saves the proposed plan via
  the same code as the Teams button (email-conversation.ts,
  email-plan-store.ts).
- Goal lifecycle: one UserState row per goal (Active / Completed /
  Replaced) with history; completion marks a goal Completed only after the
  email is sent; "show my previous goals" tool.
- WorkIQ MCP (Mail, Calendar V2 servers in ToolingManifest.json); WorkIQ
  SharePoint is read-only so list writes stay behind the code guards.
- Write guards: LLM cannot mark progress, edit finished goals, create a
  second active goal, or switch role on a goal with progress; SharePoint
  tools are owner-scoped to the caller.
- Reliability: card actions run deferred with user-visible failure
  handling and safe card unlock; typed shortcuts for sync / completions;
  LLM retries with history rollback.

Packages: @microsoft/agents-a365-* 2.x, @microsoft/opentelemetry,
@openai/agents 0.7, openai 6, zod 4; npm overrides no longer needed.

Docs: README keeps the template sections and is updated for the new
architecture (auth, deploy, troubleshooting); docs/design.md and the
code walkthrough rewritten; .env.template regenerated.

Removed: verify-userstate.ps1 (debug helper no longer used).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Akshit0022

Akshit-MSFT (Akshit0022) commented Oct 22, 2026 •

Copy link
Copy Markdown
Author

The recent update brings the Career Coach up to the current Agent 365 SDK and observability pattern, and adds the features built since the first revision. The PR description has been refreshed to match.

1. Observability — S2S route with an app-only token (matches #339)

Change Where
Migrated from the preview @microsoft/agents-a365-observability* packages to the @microsoft/opentelemetry distro, initialised before the agent/LLM modules load so OpenAI Agents auto-instrumentation can patch them. src/telemetry.ts, src/index.ts
Export uses the S2S route with an app-only token for the runtime agent identity: the blueprint credential issues an FMI assertion for the agent identity, which exchanges it for an Observability API token. The token is validated (no scp, matching client / tenant / audience), cached until near expiry, shared across concurrent calls, backs off after failure, and is never logged. src/observability/app-token-resolver.ts
Removed the delegated token cache. src/token-cache.ts (deleted)
gen_ai.agent.id = runtime agent identity from activity.recipient.agenticAppId; the blueprint id is carried only as blueprint metadata, and an env fallback equal to the blueprint id is ignored. src/agent.ts, src/client.ts
invoke_agent / chat / execute_tool spans on every path — LLM turns, card actions, typed shortcuts, email and proactive turns — with input/output messages, caller, channel, session label and a stable conversation id (one email thread = one conversation). src/agent.ts (runObserved, startInvokeAgentScope), src/client.ts (InferenceScope), src/observability/conversation-id.ts

2. New / upgraded features

  • Email channel — the AI Teammate converses by email. Only organisation members get a reply (guests, external senders and bounces are ignored); history is per email thread; replying "Save it" saves the proposed plan through the same savePlan code as the Teams button. Quizzes stay in Teams. (src/email-conversation.ts, src/email-plan-store.ts
  • Goal lifecycle — one UserState row per goal (Active / Completed / Replaced), so finished goals are kept as history instead of being overwritten. A goal is marked Completed only after the completion email is sent, so a failed send can be retried. New listPreviousGoals tool and 1:1-prep support for finished goals. Legacy rows map automatically; npm run setup:sharepoint adds the new columns.
  • WorkIQ MCP — Mail and Calendar (V2 servers) in ToolingManifest.json, attached per turn and never blocking it. The WorkIQ SharePoint server, if attached, is filtered to read-only tools so every list write still goes through the guarded tools.
  • Write guards (src/sharepoint-tools.ts — the model cannot mark a course Complete, record a passed quiz or raise a skill level (only the quiz grader can); lifecycle columns are stripped from model writes; finished goals are read-only; no second active goal; no role switch on a goal with progress. SharePoint tools are scoped to the caller's own rows (activity.from.aadObjectId).
  • Reliability — card actions acknowledge immediately and run deferred; on failure the user is told what failed and the card is unlocked only when resubmitting is safe. Typed shortcuts (check my progress, I completed …) run deterministically. LLM calls retry transient network errors and roll back the unanswered message from history.

3. Package upgrades

Package Before Now
@microsoft/agents-a365-notifications, -runtime 0.1.0-preview.125 ^2.0.0
@microsoft/agents-a365-tooling, -tooling-extensions-openai — ^2.0.0 (WorkIQ MCP)
@microsoft/agents-a365-observability, -extensions-openai, -hosting 0.1.0-preview.125 replaced by @microsoft/opentelemetry ^1.4.0
@openai/agents ^0.1.11 ^0.7.2
openai ^4.77.0 ^6.26.0
zod, axios, @opentelemetry/resources — added

The npm overrides block is no longer needed and was removed.

4. Docs and config

  • README keeps the template sections added in review (What this sample demonstrates, Authentication + identity, Deploying the agent, Additional resources, Support · Contributing · Trademarks · License) and is updated for the new architecture: three-credential auth model (agentic workload / app-only telemetry / device-code scripts), registration with a365 setup all --aiteammate, endpoint update with --update-endpoint … --m365, agent-user licensing, email walkthrough (§8.8), goal lifecycle (§8.7) and new troubleshooting rows. Package links point at npm (per Replace links to SDK GitHub repositories with links to specific packages used by samples #347); Learn links have no language code (per Don't include language code to links to LMC #348).
  • docs/design.md rewritten: purpose, principles, components, data model + goal lifecycle, key flows, security and trust boundaries, reliability, observability, identity, deployment, decisions.
  • AGENT-CODE-WALKTHROUGH.md rewritten in request order.
  • .env.template regenerated: S2S observability settings and the runtime-agent-id vs blueprint-id distinction documented; stale Use_Custom_Resolver and BEARER_TOKEN_MCP_SHAREPOINTREMOTESERVER removed.
  • .gitignore also ignores .email-plans.json and .a365-workspace-detection.local.json.
  • Removed verify-userstate.ps1 (debug helper no longer used).

5. Note on an earlier review thread

The email-injection thread (src/agent.ts, "email body is untrusted") was resolved with a no-tools summarisation path. Email is now a full conversation channel, so that path is replaced by these controls: sender must be an organisation Member (Graph lookup), bounces ignored, all SharePoint tools owner-scoped to the sender, progress/lifecycle writes enforced by code guards rather than the prompt, and Mail/Calendar rules that forbid acting on instructions embedded in emails, documents or tool results and limit recipients to the requesting user (plus their manager only when explicitly asked). Happy to tighten further if reviewers prefer.

Not included (local / generated)

.env, a365.config.json, a365.generated.config.json, .a365-workspace-detection.local.json, runtime state (.mstoken-cache.json, .proactive-*.json, .sp-subscription.json, .email-plans.json), logs, package-lock.json, manifest/manifest.zip, editor/agent-skill folders. manifest/ keeps placeholder IDs.

Verification

  • ✅ Clean npm install (no lockfile, no overrides, no --legacy-peer-deps) → npm run build and npx tsc --noEmit pass.
  • ✅ a365-code-validator static scan: 0 findings (exporter on, S2S route, app-only token resolver, runtime identity binding, invoke_agent / chat / execute_tool / output messages).
  • ✅ Docs: 12/12 relative links and all README TOC anchors resolve.
  • ✅ git diff --check clean.
  • ✅ Diff scan: no tenant / blueprint / agent / user identifiers, secrets or tokens. The only GUIDs are public Microsoft resource IDs (Graph, Messaging Bot API, Observability API, WorkIQ Mail/Calendar audiences).

Live-tenant validation evidence (E2E, Defender, Purview, admin center) will follow in a separate comment.

@Akshit0022

Copy link
Copy Markdown
Author

Validation evidence: Career Coach scenario (A365 observability + WorkIQ MCP)

The agent under test ran locally behind a dev tunnel from a build of the same src/ that is in fc93651 (byte-for-byte; verified before writing this comment), registered as an AI Teammate on a fresh blueprint and instance.


1. E2E playbook (AutoQAgent): 5 / 5 passed

image

Run finished 2026-10-22 16:42:23Z (duration 8m 46s).

Test Result Evidence
Agent exists ✅ PASS Teams people search found the Career Coach AI agent and opened the chat.
Agent responds ✅ PASS After "hi", the agent replied "Welcome back, …!" with a career progress summary (active goal read from SharePoint).
Tool execution (playbook label: MCP working) ✅ PASS The agent listed its tools and queried the LearningCatalog list, returning two Natural Language Processing courses. This exercised the agent's guarded SharePoint function tools; the WorkIQ Mail and Calendar MCP servers were attached on every turn of the run but were not invoked by this test.
O11y → Defender ✅ PASS CloudAppEvents query filtered on the runtime Agent Identity returned 84 results.
O11y → Purview ✅ PASS Audit search for the agent UPN (2026-10-21 00:00 → 2026-10-22 23:30 UTC) completed with 135 results. The search is by UPN over a two-day window, and an earlier instance of this agent (deleted the same day) used the same UPN, so not every result is necessarily from the current instance.

2. Runtime exporter evidence (agent log, redacted aggregates)

Playbook window (≈16:33:37Z → 16:42:23Z):

Signal Count
User turns received 3
WorkIQ MCP servers attached per turn (mcp_MailTools, mcp_CalendarTools) 3 / 3 turns, 0 connect failures
SharePoint tool failures 0
export-group succeeded / All spans exported successfully 9 / 9
GenAI spans delivered 30
Export failures (fail, insufficient_scope) 0

Whole agent session (2026-10-22 15:30Z → 18:00Z): 10 user turns; 27 / 27 export groups succeeded, all for the same runtime Agent Identity (no blueprint-tagged groups); 101 genAI spans delivered; 0 export failures; no app-only token acquisition failures. In every batch (36 / 36) the skipped spans are exactly the non-genAI spans the exporter filters out (log: N non-genAI spans filtered out); batches with no genAI spans log No eligible spans to export.

Five AADSTS700016 (application not found) rejections at 15:58Z–16:30Z referenced the identity of the previously deleted first instance (residual platform traffic), not the current Agent Identity. None occurred during the playbook window and none affected export.


3. A365 code validator: ✅ PASS

Check Result
Static validator ✅ 0 findings
Exporter activation ✅ enabled: true + enableObservabilityExporter (on unless ENABLE_A365_OBSERVABILITY_EXPORTER=false), @microsoft/opentelemetry initialised before the agent/LLM modules load
Export transport ✅ useS2SEndpoint: true
Observability token ✅ App-only token for the runtime Agent Identity via the blueprint FMI assertion → agent identity exchange; rejects scp tokens, checks client / tenant / audience, cached with refresh skew and failure back-off, never logged. No delegated (AgenticTokenCache) telemetry path
Identity binding ✅ gen_ai.agent.id = activity.recipient.agenticAppId; Blueprint ID only as blueprint metadata; an env fallback equal to the Blueprint ID is ignored
Semantic spans ✅ invoke_agent (input/output messages), chat (model, provider, finish reasons), execute_tool (code-handled steps) and output messages, on LLM turns, card actions, typed shortcuts, email and proactive turns — with caller, channel, session and conversation id (one email thread = one conversation)
Runtime export evidence ✅ 27 / 27 export groups succeeded for one runtime identity; 0 failures (section 2)
WorkIQ V2 ✅ Mail and Calendar V2 servers attach each turn; WorkIQ SharePoint (if attached) is filtered to read-only tools
Live Blueprint grants ✅ Read-only a365 query-entra inheritance: effective inheritance OK on 6 / 6 resources (Microsoft Graph, Agent Tools, WorkIQ Mail, WorkIQ Calendar, Messaging Bot API, Power Platform API). blueprint-scopes shows the Graph delegated scopes the sample needs, including Sites.ReadWrite.All
Local setup state ℹ️ a365.generated.config.json reports completed: true (local only — not committed)

4. Build and repository checks on fc93651

  • ✅ Clean npm install (no lockfile, no overrides, no --legacy-peer-deps)
  • ✅ npm run build and npx tsc --noEmit
  • ✅ Documentation: 12 / 12 relative links and all README TOC anchors resolve
  • ✅ git diff --check
  • ✅ Diff scan: no credentials, tokens, tenant / blueprint / agent / user identifiers, .env, logs, local state or build output. The only GUIDs are public Microsoft resource IDs (Graph, Messaging Bot API, Observability API, WorkIQ Mail/Calendar audiences)
  • ✅ PR CI: all completed checks pass (26); CLA signed

Defender / Purview CSV exports and Microsoft 365 admin-center Activity metrics are not claimed in this comment. The original screenshot, agent logs and any raw exports contain tenant and user identifiers and are not attached; only aggregate, redacted evidence is included here.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

…tions)

SharePoint tools (sharepoint-tools.ts, sharepoint-column-map.ts)
- Restrict chat writes to the configured UserState list; the data tools only
  operate on the configured Career Coach site and only read the five Career
  Coach lists. Reference data, portal completions and quiz audit rows are
  written only by provisioning, the portal and the quiz grader.
- Scope personal lists (UserState, LearningPortalStatus, QuizResponses) to the
  caller's own rows in code; bind new plans to the authenticated caller and
  reject a different UserAADId; verify row ownership fail-closed before an
  update and never allow the owner to change.
- Canonicalize model field names to display names (case-insensitive, internal
  field_N included) before every guard; reject conflicting aliases.
- Make ManagerName / ManagerEmail app-managed so the model cannot redirect the
  completion email.

Concurrency (user-lock.ts, handlers.ts, quiz-cache.ts)
- Serialise every UserState read-modify-write per user: save, sync, quiz
  grading, milestone / completion persistence and the model's create / update.
- Share one in-flight quiz generation per user/course so a card is always
  graded against its own answer key.

Runtime auth and data (graph-service.ts, career-coach-service.ts)
- Pass the runtime Graph client through site / list discovery; the developer
  device-code client is now only used by scripts and background jobs.
- Preserve intentional clears on trusted app updates (null for optional
  columns; required columns are never cleared).

Subscriptions (subscription-manager.ts)
- Store a hash of PORTAL_EVENT_SECRET with the saved subscription and replace
  (delete + create) it when the secret, URL or resource changes.
- Re-create the subscription when renewal reports it gone or its saved expiry
  has passed, instead of retrying the dead id forever.

Prompt (client.ts): the model is told chat writes are limited to UserState and
that the app records quiz results, audit rows and manager details.

Docs: guard tables, security table, file maps and trade-offs updated.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The dependency-review check flags @azure/msal-node 5.x: "authorization code
flow may disclose parameters to an attacker-controlled endpoint" (high; fixed
in 7.1.0). The sample only uses PublicClientApplication with the device-code
and silent flows for the setup / seed scripts; both work unchanged on 7.1.1
(type-check clean, silent flow and API surface verified).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Akshit0022

Copy link
Copy Markdown
Author

Addressed all 11 open threads from the review of fc93651 in 52223e5, and fixed the dependency-review failure in e8f6d35.

Review threads

# File Concern Fix
1 career-coach-service.ts Unconditional plan PATCH; concurrent turns lose each other's changes Every UserState read-modify-write now runs under a per-user lock (src/user-lock.ts): save, sync, quiz grading, milestone / completion persistence, and the model's own create / update. The read happens inside the lock, so a sync can't overwrite a freshly persisted quiz result and two courses can't clobber each other. The lock is in-process (single-instance); docs/design.md notes that multi-instance needs a shared lock or row-version conditional writes
2 sharepoint-tools.ts ManagerEmail / ManagerName writable by the model, so the completion email could be redirected Both are now app-managed (APP_MANAGED_USERSTATE_FIELDS) and stripped from model writes, including when sent under an internal column name. They are populated only from the directory
3 sharepoint-tools.ts Conversational writes allowed on any list createListItem / updateListItem refuse any list but the configured UserState, and any site but the configured one. Reads are limited to the five Career Coach lists. Reference data, portal completions and quiz audit rows are written only by provisioning, the portal and the deterministic quiz grader. The system prompt no longer tells the model to write QuizResponses
4 sharepoint-tools.ts Guards inspect display names; field_N internal names bypass them Both write tools first rewrite every key to the column's display name (canonicalizeFields: case-insensitive, matches display and internal names), then run the strip / ownership / lifecycle / progress guards. Two keys addressing the same column are rejected
5 sharepoint-tools.ts Model-supplied UserAADId takes precedence on create A new plan is always created for the authenticated caller; a different UserAADId is rejected and the caller's id is persisted (no orphaned rows). With no authenticated user, personal data is refused
6 sharepoint-tools.ts Row ownership not verified before update updateListItem reads the row first and fails closed if it can't be read or isn't the caller's; UserAADId can never be changed; itemId must be numeric
7 graph-service.ts Site / list discovery uses the developer's MSAL client even with a runtime client getSiteId(graph?) and getListIdByName(site, name, graph?) accept the runtime client, and every runtime path in career-coach-service.ts passes it, so save / sync / quiz / previous-goal flows no longer depend on .mstoken-cache.json. The device-code client stays the default only for scripts and explicitly developer-authenticated background jobs (subscription manager, startup cache warm-up, the portal-event handler)
8 quiz-cache.ts Overlapping turns can send different questions; the first card is graded against the second's answers getOrCreateQuiz shares one in-flight generation per user/course and keeps only the first result, so every card sent is graded against its own key. A failed generation doesn't poison the next attempt
9 sharepoint-column-map.ts Dropping every empty value also drops explicit clears (reset:milestones, re-saving a plan) toInternalFields takes { preserveClears }. Trusted app updates send null for an explicit '' / null on known, non-required columns (read from the list schema), so reopening a goal clears PlanCompletedDate and re-saving can clear ManagerAsks. Creates never send clears; model writes keep the old drop-empties behaviour
10 subscription-manager.ts A rotated PORTAL_EVENT_SECRET is ignored because the old subscription is renewed The saved subscription stores a SHA-256 of the secret (never the secret itself). A mismatch, URL / resource change, or a state file with no fingerprint deletes the old subscription and creates a new one instead of renewing it
11 subscription-manager.ts An expired or deleted subscription is retried forever When renewal reports 404 / ResourceNotFound, or the saved expiry has passed, the timer re-enters ensureSubscription() to create a fresh one (guarded against re-entry). Transient errors still retry on the next tick

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found four correctness issues: two P1 issues involving backup data loss and goal lifecycle races, and two P2 issues involving stale quiz submissions and duplicate completion emails. Details are inline. All four were reproduced against unchanged source with mocked I/O; live tenant behavior was not verified.

for (const row of mine) {
try {
await graph.api(`/sites/${siteId}/lists/${listId}/items/${row.id}`).delete();
ok++;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Persist the complete backup before deleting any rows

backupAndClearList deletes rows immediately, but main processes all three lists before writing the snapshot. If a later Graph read or the final local backup write fails, already-deleted rows have no persisted backup. Reproduced with all three lists deleted followed by an EACCES from writeFileSync. Collect and successfully persist the complete snapshot before issuing any DELETE requests.

await upsertUserState(graph, {
...record.state,
Completion100Fired: true,
PlanStatus: 'Completed',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Complete the originating goal rather than whichever goal is active

The completion email is composed/sent outside the user lock, after which this path re-reads the currently active goal and marks it Completed. If the user saves a new role while that email is in flight, the new goal is closed instead of the original. Reproduced by pausing composition, saving a replacement role, then releasing composition: the new goal became Completed at 0% and the old goal remained Replaced. Carry the originating UserState item ID/version through the operation and verify it before updating.

// Counted only once the reads succeed, so a network failure above isn't an attempt.
const attempts = bumpAttempts(input.userAADId, input.courseId) || 1;
const lp = userRecord.state.LearningProgress.find((l) => l.courseId === input.courseId);
const courseTitle = lp?.courseTitle ?? course.Title;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Reject quiz submissions that do not belong to the current goal

The optional lp?.courseTitle fallback lets a submission proceed even when no matching LearningProgress entry exists. Cached quizzes are keyed only by user/course and survive goal replacement, so submitting an old quiz after saving another goal can call applyQuizPass against the new goal and raise shared skills even though the course is absent from its plan. Reproduced a skill increase from 0 to 3 while the only current course remained Recommended. Bind attempts to their originating goal and reject absent plan courses before any writes.

await fireMilestone80(context, authorization, userAADId, updatedState);
}
if (updatedState.OverallProgress === 100 && !updatedState.Completion100Fired) {
await fireCompletion100(context, authorization, userAADId, updatedState);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Atomically claim completion before sending the email

The completion flag is checked on a snapshot outside the lock and only persisted after email delivery; there is no in-flight completion claim. Overlapping progress checks or portal syncs for the same 100%-complete, unsent goal can therefore each send its completion email. Reproduced two concurrent handleSyncProgress calls producing two sendMail calls for one goal. Claim completion atomically per goal across all entry points, allowing retries after a failed send.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The layering is clear and the SharePoint write guards are in the right place.

18 things to address before this merges; details inline.

Comment on lines +310 to +311
const courseId = String(lp?.courseId ?? '');
if (!courseId || courses.some((c) => c.courseId === courseId)) continue;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should fix: Reject missing course IDs instead of skipping the progress guard

A learning record without a course ID bypasses the completion check, but it is still written to SharePoint. The progress calculation subsequently counts that record solely by skill and Complete status. This permits an unvalidated record to drive a goal to 100% and trigger its completion email without a passed quiz.

Evidence

scenarios/career-coach/src/sharepoint-tools.ts:311 executes 'if (!courseId ... ) continue' before checking status; updateListItem then writes the original fields at 611-614. For an active plan with one ai-engineering goal, LearningProgress=[{"courseId":"","skillId":"ai-engineering","courseTitle":"Unvalidated","status":"Complete"}] passes this guard. scenarios/career-coach/src/career-coach-service.ts:336-338 computes total=1, done=1, progressPct=100 without checking courseId or quizResult. The next handleSyncProgress recomputes that state and calls fireCompletion100 at scenarios/career-coach/src/handlers.ts:420-421.

Suggested fix: Require every learning record to have a valid plan/catalog course ID before evaluating or writing progress. Reject malformed records rather than silently excluding them from the guard.

Comment on lines +611 to +614
guardUserStateWrite(ctx, displayFields, stored);
const internalFields = toInternalFields(displayFields, colMap);
try {
const updated = await graph.api(`/sites/${siteId}/lists/${listId}/items/${itemId}/fields`).update(internalFields);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Preserve earned progress when applying conversational updates

The lock protects this PATCH, but not the earlier read used to construct its replacement arrays. If a quiz passes between that read and this update, the stale LearningProgress array can replace Complete with Recommended and remove the persisted quiz result. Re-reading the row for ownership and guard checks does not help because the freshly read progress is never merged into the write.

Evidence

scenarios/career-coach/src/sharepoint-tools.ts:574-579 reads the current row under the lock, but 612-614 serializes and writes displayFields supplied earlier. At 312-314, an already validated stored course is explicitly skipped by the guard; decreasing skill levels is also allowed by the comparison at 325-328. Sequence: listListItems returns course A as Recommended; handleQuizSubmit persists A as Complete with a passed quiz at scenarios/career-coach/src/handlers.ts:754-759; a later updateListItem submits the earlier array. The update passes its checks and overwrites the earned completion.

Suggested fix: Merge only permitted changes into the freshly loaded row and preserve server-owned quiz results, completion states, and earned levels. Alternatively, require the originating row version and reject stale replacement snapshots.

Comment on lines +708 to +709
} catch (err) {
console.warn('[QuizSubmit] Short-answer grading failed; marking short answers wrong:', (err as any)?.message ?? err);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Do not record a grading outage as incorrect answers

When short-answer grading fails, the handler continues with both answers marked incorrect and persists a failed attempt. For the normal three-MCQ/two-short quiz, even perfect answers can then score at most 3/5. This turns a dependency outage into a false assessment and adds spurious weak-topic history instead of leaving the attempt retryable.

Evidence

scenarios/career-coach/src/handlers.ts:684-689 initializes short-answer results with correct:false. The catch at 708-710 logs the grading failure but continues to summarizeGrading at 713, appendQuizResponse at 736-747, and the UserState update at 754-759. With three correct MCQs and a timeout from gradeShortAnswers, score=3 and passed=false regardless of the submitted short answers. Missing grading results are similarly converted to incorrect in scenarios/career-coach/src/llm-tasks.ts:181-188.

Suggested fix: Abort the attempt when grading is unavailable or incomplete, preserve the submitted answers for retry, and only increment attempts or persist assessment results after all required grades are valid.

footer,
});

markGraded(input.userAADId, input.courseId);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Mark an attempt recorded before sending its result card

The attempt and plan are already persisted when the result card is sent, but the submitted marker is set only afterward. If card delivery fails, the recovery path treats the recorded attempt as safe to resubmit and unlocks it. Retrying then appends another attempt and grades the same submission again.

Evidence

scenarios/career-coach/src/handlers.ts:736-759 writes QuizResponses and UserState before awaiting sendCard at 777-786. A rejection there skips markGraded at 788 and clearQuiz at 794. scenarios/career-coach/src/agent.ts:249-251 restores the quiz whenever wasGraded is false and cached questions remain, which is exactly this state. The gradingInFlight claim is released in handlers.ts:657-659, so the restored submit enters the write path again.

Suggested fix: Set the recorded-attempt marker immediately after successful persistence, before any outbound card delivery. A delivery failure must offer recovery of the result, not permission to record the same attempt again.

Comment on lines +179 to +183
} catch (err: any) {
const msg = err?.message ?? String(err);
console.error(`[sub-mgr] ✗ Failed to create subscription: ${msg}`);
console.error('[sub-mgr] Hint: make sure PORTAL_WEBHOOK_URL is reachable from the internet and the /api/portal-event route is running (Graph validates it during CREATE).');
return null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Retry subscription creation after an initial startup failure

A transient failure during initial subscription creation disables proactive notifications until the process is restarted. The server continues serving normally, but no recovery timer has been started. Restoring Graph connectivity or bringing the tunnel online later cannot recover this feature automatically.

Evidence

scenarios/career-coach/src/index.ts:215-218 calls ensureSubscription once during startup. In scenarios/career-coach/src/subscription-manager.ts, startRenewTimer is called only after successful renewal or creation at 132 and 177; the initial create failure returns null at 179-183. Even starting the existing timer alone would not recover an empty installation because its callback returns immediately when no state file exists at 198-199. A first POST /subscriptions returning 503 therefore leaves no subscription and no subsequent creation attempt.

Suggested fix: Start a bounded-backoff recovery loop independently of initial creation success. When no usable subscription state exists, the loop should retry ensureSubscription rather than returning without work.

Comment on lines +69 to +71
# Random shared secret. Generate one with:
# [Guid]::NewGuid().ToString() + [Guid]::NewGuid().ToString()
PORTAL_EVENT_SECRET=<CHANGE_ME_random_guid_pair>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Shipped PORTAL_EVENT_SECRET placeholder is accepted as a valid webhook secret

The template ships PORTAL_EVENT_SECRET=<CHANGE_ME_random_guid_pair>, which is a non-empty value. The webhook (index.ts) and ensureSubscription only fail closed when the secret is empty. A .env copied from the template therefore runs normally with a secret that is published in this repo, and nothing tells the developer it was never changed.

/api/portal-event is registered before authorizeJWT and is exposed through the --allow-anonymous dev tunnel (README §5.1). Anyone who learns the tunnel URL can send the placeholder as X-Portal-Secret. The agent then DMs any employee who has chatted with it, using preface text built from attacker-chosen CourseId/Status values. The response also reveals whether a given AAD object ID has a cached conversation. The same value works as a Graph clientState, so it can be used to trigger notification processing again and again.

README line 164 understates the risk by describing the value as 'Any random string (used for a manual POST test path)'. It is also the only check on Graph notifications. The repo guidance asks to 'Confirm required secrets are validated at startup with fail-fast behavior (no silent defaults for security-critical config)'.

Evidence
  • .env.template:71 contains PORTAL_EVENT_SECRET=<CHANGE_ME_random_guid_pair>. No code in the sample checks for CHANGE_ME; a grep over the career-coach .ts, .js, .json and .md files returns no matches.
  • src/index.ts:82-87: const expected = process.env.PORTAL_EVENT_SECRET || ''; if (!expected) { ... res.status(500) ... }. Only an empty value is rejected.
  • src/index.ts:111-124: compares req.headers['x-portal-secret'] with expected, then calls agentApplication.handleProactivePortalEvent(aad, body) and returns its {ok, reason} as JSON.
  • src/index.ts:95-108: accepts a Graph-shaped body when first.clientState === expected and runs handleGraphNotification.
  • src/subscription-manager.ts:79-87: also rejects only an empty clientState, so the Graph subscription is created with the placeholder.
  • src/agent.ts:788-792: builds preface from payload.CourseId and payload.Status, checking only that they are strings.
  • src/agent.ts:813-816: passes preface to sendPreface, which sends it verbatim with ctx.sendActivity(MessageFactory.text(text)) (agent.ts:46-47).
  • src/agent.ts:779-783: returns No cached conversation for aadObjectId=… for unknown users.
  • README.md:221: devtunnel create career-coach-tunnel --allow-anonymous.
  • PACKET.md:955 (security-compliance guidance): 'no silent defaults for security-critical config'. PACKET.md:1026: 'All samples should fail fast on missing required security configuration'.
  • Sibling samples reject placeholder secrets explicitly, e.g. nodejs/openai/sample-agent/src/observability-token-service.ts:52-58.

Scenario:

  1. A developer fills in the Azure, SharePoint and connection values but leaves line 71 as shipped. Everything works.
  2. An attacker sends POST https://<tunnel>/api/portal-event with header X-Portal-Secret: <CHANGE_ME_random_guid_pair> and body {"UserAADId":"<victim-oid>","CourseId":"<arbitrary text or link>"}.
  3. The victim receives a Teams DM from Career Coach containing that text, and a sync run follows.

Suggested fix: Ship PORTAL_EVENT_SECRET= empty, so the existing empty-secret guards stay closed until a real value is generated. Alternatively, reject placeholder values (for example, anything containing CHANGE_ME) in both index.ts and ensureSubscription. Also update README line 164 to say the value is the Graph clientState as well as the header for manual tests.

Suggested change
# Random shared secret. Generate one with:
# [Guid]::NewGuid().ToString() + [Guid]::NewGuid().ToString()
PORTAL_EVENT_SECRET=<CHANGE_ME_random_guid_pair>
# Random shared secret (required). Used as the Graph subscription clientState and as the
# X-Portal-Secret header for manual tests; the webhook refuses requests while it is empty.
# Generate one with:
# [Guid]::NewGuid().ToString() + [Guid]::NewGuid().ToString()
PORTAL_EVENT_SECRET=

# =============================================================================
# 2. MCP + OBSERVABILITY
# =============================================================================
BEARER_TOKEN=<CHANGE_ME_bearer_token_optional_for_MCP_tools>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: BEARER_TOKEN placeholder replaces the agentic MCP token, so WorkIQ tools never attach

The template ships BEARER_TOKEN=<CHANGE_ME_bearer_token_optional_for_MCP_tools>. attachWorkIqTools passes process.env.BEARER_TOKEN || '' to addToolServersToAgent, so on every turn this non-empty placeholder is used as the MCP auth token. The agentic user token is only obtained when that argument is empty, so it is never fetched.

The README describes the variable as an optional dev override ('Leave empty normally'), so most people will leave the shipped value alone. Tool-server registration or connection then fails. The error is caught and only logged as a warning, and the Mail and Calendar tools silently never attach. As a result, 'email me my plan' and 'find time for my 1:1' (README §8) don't work. The sibling Node templates ship this key empty.

Evidence
  • .env.template:28: BEARER_TOKEN=<CHANGE_ME_bearer_token_optional_for_MCP_tools>.
  • src/client.ts:413: await toolService.addToolServersToAgent(this.agent, ctx.authorization, this.authHandlerName, ctx.turnContext, process.env.BEARER_TOKEN || '');. A non-empty value is passed through as the token.
  • src/client.ts:414-418: catches the failure, logs [WorkIQ] Failed to register MCP tool servers (continuing without them) and sets this.agent.mcpServers = [].
  • README.md:149: 'Token from a365 develop get-token, only for running MCP tools without the platform token. Leave empty normally. Tokens expire.'
  • AGENT-CODE-WALKTHROUGH.md:224: 'WorkIQ (dev only) | BEARER_TOKEN'.
  • Sibling templates use BEARER_TOKEN=: nodejs/openai/sample-agent/.env.template:21 and nodejs/claude/sample-agent/.env.template:11.

Scenario: copy the template, fill in the values the README calls required, then ask the agent 'email me my plan'. No Mail tool is available, and the only sign of the problem is the WorkIQ warning in the log.

Suggested fix: Ship BEARER_TOKEN= empty, with a comment that it is an optional, dev-only override from a365 develop get-token.

Suggested change
BEARER_TOKEN=<CHANGE_ME_bearer_token_optional_for_MCP_tools>
# Optional, local dev only: token from `a365 develop get-token`. Leave empty to use the agentic token.
BEARER_TOKEN=

3. Reconcile the messaging endpoint: `a365 setup blueprint --update-endpoint <url>/api/messages --m365`.
4. Package with `a365 publish --agent-name "Career Coach" --aiteammate`, upload the package in the Microsoft 365 admin center, confirm the Teams Developer Portal configuration, and request an instance ([create an instance](https://learn.microsoft.com/microsoft-agent-365/developer/create-instance)).

Set the cloud env vars (Azure OpenAI, agentic auth, observability, SharePoint) at the platform level — not just in a local `.env`. The quiz answer-key cache is in memory and the proactive map, email-plan store and subscription state are local files, which suits a single instance; move them to shared storage before running several instances.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Deployment docs omit that the real-time trigger runs on the developer's device-code token cache

README §12 (line 541) says the MSAL device-code credential is used only by the setup and seed scripts. §4.4 (line 207) says that at runtime the agent works with SharePoint as its own agent user, not as the developer.

In fact, the running server uses the developer's cached device-code token (.mstoken-cache.json) for all of Feature 1:

  • creating and renewing the Graph subscription, at startup and on the renewal timer;
  • resolving the site and list, and reading LearningPortalStatus rows, for every change notification.

§13 points people to App Service, Container Apps or Functions and lists the local-state caveats, but not this one. A hosted instance has no cache file: it is gitignored and only created by an interactive sign-in. As a result:

  • ensureSubscription only logs a non-fatal warning;
  • notifications can't be processed;
  • the real-time proactive quiz silently never fires.

The same failure happens locally once the developer's refresh token lapses. The server also holds an admin's delegated Sites.Manage.All / User.Read.All / Mail.Send token, which the 'three separate credentials' description doesn't mention.

Evidence
  • src/index.ts:214-219: runs ensureSubscription() on listen; a failure only logs [startup] ensureSubscription failed (non-fatal).
  • src/subscription-manager.ts:94-96: calls getSiteId() and getListIdByName(siteId, listName) with no client, so both fall back to getGraphClient() (graph-service.ts:222 and graph-service.ts:325).
  • createSubscription, renewSubscription, listSubscriptions and deleteSubscription always use getGraphClient() (graph-service.ts:362-393).
  • src/index.ts:131-160 (handleGraphNotification): calls getSiteId(), getListIdByName(siteId, …) and getListItems(). getListItems hard-codes getGraphClient() (graph-service.ts:338-339).
  • graph-service.ts:125-131: throws 'No cached Microsoft account found. Run npm run setup:sharepoint once to sign in.' when the cache is missing.
  • .gitignore:15: ignores .mstoken-cache.json.
  • graph-service.ts:62: 'Simple file-backed MSAL cache. Not encrypted — for local dev only.'
  • README.md:541: 'Setup / seed scripts use the MSAL device-code flow…'.
  • README.md:207: 'At runtime the agent reads/writes the lists as its own agent user … not as the developer who ran the setup scripts.'
  • README.md:554: lists only the quiz cache, proactive map, email-plan store and subscription state.
  • By contrast, AGENT-CODE-WALKTHROUGH.md:40 correctly says 'MSAL device-code for scripts and background jobs'.

Scenario: deploy per §13 with the platform env vars set, then run mark:complete. The startup log shows ensureSubscription failing on the missing cache, so no subscription exists and no proactive quiz DM ever arrives.

Suggested fix: In §4.4 and §12, state that the Feature 1 webhook (subscription lifecycle and notification row reads) runs as the developer who completed the device-code sign-in. In §13, add that a hosted instance needs that token cache, provisioned securely, or app-only/agentic replacements for these calls. Without either, only the manual POST path and user-initiated syncs work.

| Feature 4 email "Access is denied" | `sendMail` using `/users/{id}/sendMail` on a delegated token | Should be fixed — `graph-service.ts` uses `/me/sendMail` whenever a graph client is passed in. |
| `Failed to acquire token silently` on script start | Cached MSAL token expired (60 days) or was for a different tenant | Delete `.mstoken-cache.json` and re-run — a fresh device-code prompt will appear. |
| SharePoint 500 on write with no error detail | Column-name mismatch between display and internal | Check the `[createListItem] Known columns:` log line — the field you sent must be in there. |
| Graph subscription creation fails with "Notification URL invalid" | `PORTAL_WEBHOOK_URL` isn't reachable | Verify the dev tunnel is running and the URL in `.env` matches; test with `curl <URL>?validationToken=hi` (should echo `hi`). |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Webhook validation test in Troubleshooting sends a GET, which the endpoint never answers

curl <URL>?validationToken=hi sends a GET, and so does Windows PowerShell's curl alias (Invoke-WebRequest). /api/portal-event is registered only for POST. A GET falls through to authorizeJWT and gets a 401, or a 404 when NODE_ENV=development. It never echoes hi, even when the tunnel and URL are correct, so this check sends people looking for a problem that isn't there. Graph's own validation request is a POST, as line 236 already says.

Evidence
  • src/index.ts:72: server.post('/api/portal-event', …) is the only handler for that path, and the echo happens inside it (index.ts:74-80).
  • src/index.ts:194: server.use(authorizeJWT(authConfig)) handles everything else, and there is no server.get('/api/portal-event').
  • README.md:236: 'Graph validates by POSTing ?validationToken=X'.

Suggested fix: Use a POST in the test command.

Suggested change
| Graph subscription creation fails with "Notification URL invalid" | `PORTAL_WEBHOOK_URL` isn't reachable | Verify the dev tunnel is running and the URL in `.env` matches; test with `curl <URL>?validationToken=hi` (should echo `hi`). |
| Graph subscription creation fails with "Notification URL invalid" | `PORTAL_WEBHOOK_URL` isn't reachable | Verify the dev tunnel is running and the URL in `.env` matches; test with `Invoke-RestMethod -Method Post "<URL>?validationToken=hi"` (should return `hi`). |


You'll need each test user's **UPN** and **AAD Object ID** (Entra ID → Users → select the user → *Object ID*).

> **Set the default user for scripts:** every helper script (`mark:complete`, `reset:milestones`, `backup:user`) accepts a user AAD ID. To avoid retyping it, set env vars **once** per terminal session:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: TEST_USER_* is documented as the default for all helper scripts, but only mark:complete reads it

§7 says every helper script (mark:complete, reset:milestones, backup:user) accepts a user AAD ID, and that setting TEST_USER_AAD_ID / TEST_USER_NAME saves retyping it. .env.template:125-126 makes the same claim. It actually works the other way round:

  • reset:milestones and backup:user read only process.argv, and exit with a usage error if the ID is missing.
  • mark:complete has no user argument at all; its arguments are course IDs. It reads only TEST_USER_AAD_ID, and silently falls back to the all-zero GUID when that is unset.

Someone who treats the env vars as an optional convenience and runs §8.3's npm run mark:complete -- CS007560 writes completion rows for 00000000-…. The script reports success, but no quiz DM ever arrives.

Evidence
  • src/scripts/reset-milestones.ts:23-27: const userAADId = (process.argv[2] || '').trim();, then on a missing value prints 'Usage: npm run reset:milestones -- …' and calls process.exit(2).
  • src/scripts/backup-and-reset-user.ts:86-90: same pattern; it never reads TEST_USER_*.
  • src/scripts/mark-courses-complete.ts:34: const USER_AAD_ID = process.env.TEST_USER_AAD_ID || '00000000-0000-0000-0000-000000000000';
  • src/scripts/mark-courses-complete.ts:56: const courseIds = process.argv.slice(2)….
  • README.md:347: npm run mark:complete -- CS007560 passes no user.
  • src/index.ts:163-188: proactively notifies the row's UserAADId, which logs 'No cached conversation for aadObjectId=00000000-…'.

Suggested fix: Say that mark:complete requires TEST_USER_AAD_ID (from .env or the shell), and that the other two scripts take the ID as an argument. Update the .env.template comment to match, or make the scripts fall back to the env vars.

Suggested change
> **Set the default user for scripts:** every helper script (`mark:complete`, `reset:milestones`, `backup:user`) accepts a user AAD ID. To avoid retyping it, set env vars **once** per terminal session:
> **Set the test user for `mark:complete`:** `mark:complete` has no user argument — it writes rows for `TEST_USER_AAD_ID` / `TEST_USER_NAME` (from `.env` or the shell), so set them before §8.3. `reset:milestones` and `backup:user` take the AAD ID as their first argument instead:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants