Repository navigation
Add career-coach scenario: Career Coach autopilot using Node.js SDK - #338
Open
Akshit-MSFT (Akshit0022) wants to merge 9 commits into
Open
Akshit-MSFT (Akshit0022) wants to merge 9 commits into
Akshit-MSFT (Akshit0022) wants to merge 9 commits into
Conversation
Contributor
There was a problem hiding this comment.
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.
Contributor
There was a problem hiding this comment.
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.fieldswon'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:53TokenCache.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/expressto 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
Contributor
There was a problem hiding this comment.
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.
Contributor
There was a problem hiding this comment.
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_SECRETkeeps the old subscription and its oldclientState. 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.tscreates.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; comparesrc/agent.ts:1-2andsrc/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_OBSERVABILITYswitch 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; customSP_SITE_PATHandSP_LIST_USER_STATEvalues 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.AllandUser.Read.All, andgraph-service.tsrequests 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
LastUpdatedwatermark/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()andgetListIdByName(), 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
backupAndClearListcall 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
uncaughtExceptionis 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);
});
Akshit-MSFT (Akshit0022)
marked this pull request as draft
August 25, 2026 10:44
Akshit-MSFT (Akshit0022)
force-pushed
the
scenario/career-coach
branch
from
August 25, 2026 10:52
9a73e6b to
179ba6f
Compare
Akshit-MSFT (Akshit0022)
marked this pull request as ready for review
August 25, 2026 11:02
Contributor
There was a problem hiding this comment.
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.jsontherefore 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
UserStateorQuizResponsesrow. Prompt instructions are not an authorization boundary, so a manipulated model call can expose private career data. Derive the caller's AAD ID fromRunCtxand 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
UserStaterecords; 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)
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.
Akshit-MSFT (Akshit0022)
force-pushed
the
scenario/career-coach
branch
from
August 26, 2026 15:01
72ea808 to
bd4e262
Compare
Akshit-MSFT (Akshit0022)
left a comment
Akshit-MSFT (Akshit0022)
left a comment
Author
There was a problem hiding this comment.
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>
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)
2. New / upgraded features
3. Package upgrades
The npm 4. Docs and config
5. Note on an earlier review threadThe email-injection thread ( Not included (local / generated)
Verification
Live-tenant validation evidence (E2E, Defender, Purview, admin center) will follow in a separate comment. |
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 1. E2E playbook (AutoQAgent): 5 / 5 passed
Run finished 2026-10-22 16:42:23Z (duration 8m 46s).
2. Runtime exporter evidence (agent log, redacted aggregates)Playbook window (≈16:33:37Z → 16:42:23Z):
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: Five 3. A365 code validator: ✅ PASS
4. Build and repository checks on
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved write-authorization gaps and concurrent state-update risks can expose private reports or corrupt saved progress.
11 open issues
7 resolved since last review
uncaughtExceptioncan leave Node.js state corrupted while the health endpoint….sp-subscription.jsonis not ignored even though it contains tenant-specific…
Review effort: Balanced
…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>
Author
|
Addressed all 11 open threads from the review of Review threads
|
Rick Brighenti (rbrighenti)
left a comment
Rick Brighenti (rbrighenti)
left a comment
Contributor
There was a problem hiding this comment.
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.
Krishnadheeraj (DheerajPannala)
left a comment
Krishnadheeraj (DheerajPannala)
left a comment
Contributor
There was a problem hiding this comment.
The layering is clear and the SharePoint write guards are in the right place.
18 things to address before this merges; details inline.
Integrity: quiz only counts for courses on the current plan; grading outages abort instead of recording a failed attempt; graded marker set before the result card; stale model writes can no longer undo earned progress; plan courses must carry a catalog courseId; completion email claimed once per goal and closes only the goal it was sent for; minutes summed for the plan's courses only; cached quizzes cleared when a goal is saved/replaced. Security: Mail/Calendar recipient allowlist enforced in code (mail-guard); email text and quoted history fenced as untrusted; email thread key includes agent instance and verified sender; failed-write logs carry column names only; PORTAL_EVENT_SECRET validated (placeholder/short rejected) and shipped empty; BEARER_TOKEN shipped empty. Identity/reliability: background Graph work (subscription, notification scan, warm-up) runs as the agent user via getAgenticUserToken with ids captured from the first turn; bounded-backoff subscription retry; server-side LastUpdated filter; production fails fast without a valid secret and service connection. Scripts/docs: backup writes the file before deleting; helper scripts honour TEST_USER_AAD_ID; prompt course-match rule aligned with the matcher; README, design and walkthrough updated. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Krishnadheeraj (DheerajPannala)
left a comment
Krishnadheeraj (DheerajPannala)
left a comment
Contributor
There was a problem hiding this comment.
The scenario is well structured: deterministic handlers own the state transitions, SharePoint writes are guarded in one place, and Teams and email share that path. The critical flows are implemented carefully and the sample is close to something operators can run.
13 things to address before this merges; details inline.
Since my last review:
- Fixed: Reject missing course IDs instead of skipping the progress guard (
scenarios/career-coach/src/sharepoint-tools.ts) - Fixed: Do not record a grading outage as incorrect answers (
scenarios/career-coach/src/handlers.ts) - Fixed: Mark an attempt recorded before sending its result card (
scenarios/career-coach/src/handlers.ts) - Fixed: Limit completion time totals to courses in the completed goal (
scenarios/career-coach/src/handlers.ts) - Fixed: Remove private career payloads from production error logs (
scenarios/career-coach/src/sharepoint-tools.ts) - Fixed: Use the same course-matching rule for text ratings and cards (
scenarios/career-coach/src/client.ts) - Fixed: Include the authenticated sender in email conversation keys (
scenarios/career-coach/src/agent.ts) - Fixed: A quiz for a course that is not on the active goal still raises skill levels (
scenarios/career-coach/src/handlers.ts) - Fixed: Shipped PORTAL_EVENT_SECRET placeholder is accepted as a valid webhook secret (
scenarios/career-coach/.env.template) - Fixed: BEARER_TOKEN placeholder replaces the agentic MCP token, so WorkIQ tools never attach (
scenarios/career-coach/.env.template) - Fixed: Webhook validation test in Troubleshooting sends a GET, which the endpoint never answers (
scenarios/career-coach/README.md) - Fixed: TEST_USER_* is documented as the default for all helper scripts, but only mark:complete reads it (
scenarios/career-coach/README.md) - Still open: Preserve earned progress when applying conversational updates (
scenarios/career-coach/src/sharepoint-tools.ts). The new merge at scenarios/career-coach/src/sharepoint-tools.ts:396-401 matches earned records by courseId alone, although handlers.ts:314-325 stores one record per course/skill pair. For passed recor - Still open: Retry subscription creation after an initial startup failure (
scenarios/career-coach/src/subscription-manager.ts). The new backoff does not cover list-lookup failures. scenarios/career-coach/src/graph-service.ts:379-386 catches every lookup error and substitutes an empty list; subscription-manager.ts:143-148 then - Still open: Proactive subscription and portal webhook run as the developer, not the agent (
scenarios/career-coach/src/index.ts). The agentic background path is implemented, but scenarios/career-coach/src/graph-service.ts:240-247 still falls back to acquireTokenSilentForGraph whenever the provider or captured identity is missing - Still open: Identity section says runtime Graph is agentic, but the portal subscription is not (
scenarios/career-coach/docs/design.md). scenarios/career-coach/docs/design.md:173-176 now describes agentic background authentication but restricts the developer-cache fallback to local use. graph-service.ts:240-247 has no production restri - Still open: Email body is executable input to Mail tools, with no recipient check in code (
scenarios/career-coach/src/agent.ts). A recipient wrapper now exists, but its authorization check remains bypassable. In scenarios/career-coach/src/mail-guard.ts:29 and :72-80, the valid address mallory!alice@contoso.com is matched as the - Still open: Deployment docs omit that the real-time trigger runs on the developer's device-code token cache (
scenarios/career-coach/README.md). A developer cache is no longer required for deployment: scenarios/career-coach/src/index.ts:28-33 wires the hosting connection's agentic-user token provider, and README.md:163-166, :239-241, and :547
Completion/goals: the completion is recorded before the summary card is rendered, so a card failure can't resend the email; the manager is looked up from the directory at send time (never the cached address) and the card only says the manager was copied if they were; replacing a goal creates the new row first and rolls it back if the old one can't be retired; plans with missing or unknown course ids are rejected before anything changes. Identity/subscription: background Graph work never uses the developer cache in production (waits for the agent ids and retries); subscription setup is single-flight, each attempt is pinned to one identity, and a developer-made subscription is replaced by an agent-user one (deleted with its creator) once the ids are known; portal notifications are de-duplicated across batches. Privacy/config: no user message text or display name in logs; observability export is opt-in (template ships false, only "true" enables); seed script adds only missing rows by key and fails non-zero on errors; README/design/walkthrough corrected (device-code prompt, production behaviour). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Author
|
Thanks Rick Brighenti (@rbrighenti) and Krishnadheeraj (@DheerajPannala). All 35 review threads are addressed: 22 in Round 1: 22 threads, fixed in
|
| Topic | What was fixed |
|---|---|
| Backup script deleted rows before writing the backup | Reads all lists, writes the backup file, then deletes; exits non-zero if any delete fails |
| Completion could close a replacement goal | The originating goal id is carried through and re-verified before the goal is marked Completed |
| Quiz for a course not on the plan raised skills (2 threads) | A quiz counts only for a course on the current plan, checked before any write; saving or replacing a goal clears cached quizzes |
| Duplicate completion emails from overlapping syncs | In-process claim per user and goal; fresh state re-read; claim released in finally so failures can retry |
Empty courseId bypassed the progress guard |
Id-less records count as unearned; every plan course needs a catalog courseId (fails closed); progress ignores id-less records |
| Stale model write could undo a passed quiz | Passed courses are merged from the fresh row, skill levels never decrease, goals and overall are recomputed |
| Grading outage recorded a wrong attempt | Grading throws on a missing result; nothing is persisted and no attempt is counted; the card is restored with the answers |
markGraded ran after the card was sent |
Marked right after persistence, before sending |
| Failed first subscription was never retried | Bounded backoff (30 s to 10 min) for retryable failures only, plus an immediate retry when the agent identity is captured |
| Completion minutes summed every portal row | Summed for the completing plan's courses only, latest row per course |
| Failed-write logs dumped field contents | Logs operation, status, error code and column names only; user text removed from logs |
| Prompt course-match rule contradicted the matcher | Prompt aligned to the matcher's range-overlap rule and ranking |
| Email thread key had no sender | Key is now agent instance + verified sender + conversation id |
| Developer-identity subscription and scan (3 threads) | Background Graph work runs as the agent user via getAgenticUserToken, with ids captured from the first turn or env; server-side LastUpdated filter |
| Mail recipient limits were only in the prompt; dead summarizer | Recipient allowlist enforced in code (mail-guard); email text and quoted history fenced as untrusted; dead code removed |
PORTAL_EVENT_SECRET placeholder shipped |
Shipped empty; placeholder, empty and short values are rejected; production fails fast |
BEARER_TOKEN placeholder shipped |
Shipped empty, with a local-dev-only note |
README curl check used GET |
Replaced with Invoke-RestMethod -Method Post |
TEST_USER_* claim was untrue |
Helper scripts honour it; mark:complete fails fast if it is unset |
Round 2: 13 threads, fixed in def7eae
| Topic | What was fixed |
|---|---|
| Email resent if the completion card failed | Completion is recorded before the card is rendered, so a card failure can't make the email resendable |
| Completion copied a stale manager | Manager is looked up from the directory at send time; no cached fallback; if the lookup fails, only the employee is mailed |
| Card claimed the manager was emailed | The footer reflects who actually received the mail |
| Old goal retired before the replacement existed | New goal is created first; rolled back if retiring the old one fails |
| Plan saved with missing or unknown course ids | Rejected before any write, with an explicit message (Teams and email) |
| Subscription setup could overlap or stay on the developer token | Single-flight; each attempt pinned to one identity; a developer-made subscription is deleted with its creator and recreated as the agent |
| Production gate didn't stop the developer fallback (3 threads) | Enforced in code: in production the background client waits for the agent ids and retries, never using the developer cache; docs corrected |
| Portal updates re-sent across webhook batches | Per-user checkpoint; the checkpoint advances only after successful handling |
| Message text and display name in logs | Removed |
| Observability export was on in the template | Template ships false; only the exact string true enables export |
| Partial seed treated as complete | Seed is keyed per row, inserts only missing rows, exits non-zero on failure |
| README overstated the device-code prompt | Only setup:sharepoint signs in; the other scripts reuse its cache |
Validation
tscandnpm run buildpass; secret scan clean.- Fake-Graph behaviour checks pass for both rounds.
- Live: after a restart with
NODE_ENV=production, the agent renewed its Graph subscription as the agent identity, with no developer sign-in.
Krishnadheeraj (DheerajPannala)
left a comment
Krishnadheeraj (DheerajPannala)
left a comment
Contributor
There was a problem hiding this comment.
The deterministic-handler design fits this workflow, and the lifecycle and concurrency code is careful. I would not merge it yet.
Seven things to address before this merges; details inline.
Since my last review:
- Fixed: Proactive subscription and portal webhook run as the developer, not the agent (
scenarios/career-coach/src/index.ts) - Fixed: Identity section says runtime Graph is agentic, but the portal subscription is not (
scenarios/career-coach/docs/design.md) - Fixed: Record successful completion email delivery before sending its card (
scenarios/career-coach/src/handlers.ts) - Fixed: Keep the old goal active until its replacement has been saved (
scenarios/career-coach/src/handlers.ts) - Fixed: Refresh the manager recipient before sending the completion report (
scenarios/career-coach/src/handlers.ts) - Fixed: Subscription setup can overlap, and a developer-token success skips the agent retry (
scenarios/career-coach/src/subscription-manager.ts) - Fixed: Reject incomplete or unknown courses before saving a plan (
scenarios/career-coach/src/handlers.ts) - Fixed: Remove private message text from production logs (
scenarios/career-coach/src/agent.ts) - Fixed: Completion card says the manager was emailed even when they were not (
scenarios/career-coach/src/handlers.ts) - Fixed: Production startup does not stop the developer Graph fallback (
scenarios/career-coach/docs/design.md) - Fixed: Checked-in observability config exports career spans unless the flag is the string false (
scenarios/career-coach/.env.template) - Fixed: Production gate doesn't stop background Graph work from running on the developer's cached sign-in (
scenarios/career-coach/README.md) - Fixed: Only
setup:sharepointshows the device-code prompt; the other scripts need its cache (scenarios/career-coach/README.md) - Still open: Preserve earned progress when applying conversational updates (
scenarios/career-coach/src/sharepoint-tools.ts). sharepoint-tools.ts:393-401 still merges earned records by courseId alone. handlers.ts:333-346 creates one record per course/skill pair. For a passed course covering two skills, the second earned reco - Still open: Retry subscription creation after an initial startup failure (
scenarios/career-coach/src/subscription-manager.ts). graph-service.ts:412-419 still converts every list-lookup exception into an empty result. subscription-manager.ts:186-189 consequently returns retry:false for a transient lookup failure, bypassing the - Still open: Email body is executable input to Mail tools, with no recipient check in code (
scenarios/career-coach/src/agent.ts). The recipient wrapper remains bypassable: mail-guard.ts:29 and :72-80 parse mallory!alice@contoso.com as the allowed substring alice@contoso.com. recipientViolation then accepts it, and :116 forwards - Still open: Deployment docs omit that the real-time trigger runs on the developer's device-code token cache (
scenarios/career-coach/README.md). README.md:164-167, :241-249 and :551-557 now describe background agent-user authentication, the identity prerequisites, and the non-production-only developer fallback. Production background work no lo - Still open: Deduplicate portal updates across webhook deliveries (
scenarios/career-coach/src/index.ts). Sequential deliveries are now deduplicated, but overlapping deliveries are not. index.ts:208 selects against lastNotified before awaiting delivery at :217, and advances the checkpoint only at :225. Tw - Still open: A partial seed is treated as complete on the next run (
scenarios/career-coach/src/scripts/seed-reference-data.ts). scripts/seed-reference-data.ts:123-134 reads existing row keys, and :165-179 inserts only missing keys rather than skipping a nonempty list. Failed row inserts also produce a nonzero exit status at :2
Comment on lines
+761
to
+762
| const convId = await this.proactive.storeConversation(turnContext); | ||
| proactiveRefs.setRef(aad, convId); |
Contributor
There was a problem hiding this comment.
Should fix: Keep group conversations out of the private proactive-reference map
Any message from an employee replaces their proactive destination with the current conversation, including a group chat. If Alice first establishes a personal chat and subsequently messages the coach in a group, her next portal update resumes that group conversation and posts her course or career-progress information to everyone there. Checking the employee's identity does not establish that the destination is private.
Evidence
agent.ts:757-762 checks only the sender's aadObjectId before calling storeConversation(turnContext) and setRef(aad, convId). proactive-refs.ts:49-55 overwrites the existing reference for that employee. agent.ts:781 and :801-819 subsequently resumes exactly that stored conversation and calls handleSyncProgress for the employee. Thus personal conversation D -> Alice sends a message in group G -> portal event produces Alice's progress/quiz in G. There is no conversation-type restriction in this path or the scenario manifests.
Suggested fix: Store proactive references only from personal conversations, and never replace them with group or channel references. Establish a personal conversation before sending career data when no private reference exists.
| args: unknown, | ||
| resolveAllowed: () => Promise<Set<string>>, | ||
| ): Promise<string | null> { | ||
| if (isReadToolName(toolName)) return null; |
Contributor
There was a problem hiding this comment.
Should fix: Restrict reads from the shared coach mailbox to the current employee
Read/search operations bypass authorization here, although every employee uses the same Career Coach mailbox. That mailbox contains other employees' incoming coaching messages and outgoing completion reports. A mailbox search in Bob's conversation can therefore retrieve Alice's private career content; the per-sender conversation key and SharePoint ownership checks do not protect this separate data path.
Evidence
ToolingManifest.json:3-9 registers the Mail server, and client.ts:427-447 attaches it to each conversation with only guardMailRecipients as its restriction. mail-guard.ts:93 returns immediately for read operations, before resolving the caller's allowed addresses; :97 also permits address-free arguments. The shared-mailbox identity is explicit in client.ts:379-382 and README.md:553. graph-service.ts:392-402 sends completion reports through /me/sendMail with saveToSentItems:true. After Alice completes a plan, an unrestricted Mail search/read during Bob's turn can return Alice's saved report without any employee-ownership check.
Suggested fix: Expose only the Mail operations this scenario requires. If mailbox reads are retained, enforce employee ownership on both the requested messages and returned content before passing that content into the conversation.
Comment on lines
+465
to
+470
| async invokeAgent(prompt: string, ctx: RunCtx): Promise<string> { | ||
| // Add user message to history (once, before any retries) | ||
| this.conversationHistory.push({ role: 'user', content: prompt }); | ||
|
|
||
| // Build input: format as AgentInputItem array | ||
| const input = this.conversationHistory.map(msg => { |
Contributor
There was a problem hiding this comment.
Minor: Serialize invocations of each cached conversation client
Overlapping messages in one conversation share this history and the same mutable connection collection. One invocation can replace or clear connections while another is still running. Its failure cleanup can also remove the other invocation's successful reply rather than its own unanswered message, leaving subsequent turns with incorrect context.
Evidence
client.ts:96-101 returns the same cached Client for a conversation; agent.ts:628-629 and :1053-1054 invoke it without per-client serialization. client.ts:428, :447 and :456 replace or clear the shared agent.mcpServers. Independently, the history race is deterministic: A pushes its message at :467; B pushes its message and successful reply at :494; A exhausts its retries and pop() at :516 removes B's reply. The locks in handlers.ts and sharepoint-tools.ts cover SharePoint writes, not this client lifecycle.
Suggested fix: Queue invocations per cached client, covering history updates, connection attachment, execution, cleanup, and rollback. Use a separate conversation-level queue rather than nesting these runs inside the existing non-reentrant UserState lock.
Comment on lines
+132
to
+136
| res.status(202).json({ ok: true }); | ||
| // Fire-and-forget: read recently-changed rows and DM affected users. | ||
| void handleGraphNotification(body).catch((err) => { | ||
| console.error('[Proactive] Async Graph notification handling failed:', (err as any)?.message ?? err); | ||
| }); |
Contributor
There was a problem hiding this comment.
Minor: Retain failed notification work after returning 202
The webhook acknowledges delivery before performing any SharePoint reads or proactive sends, but failures afterward are only logged. There is no queued retry. A final course-completion notification received during a temporary Graph or Teams outage can therefore disappear permanently, even after the dependency recovers; leaving the checkpoint unchanged only helps if another notification happens to arrive.
Evidence
index.ts:132 returns HTTP 202 and :134 starts handleGraphNotification asynchronously. Site/list failures return at :185-188, exhausted row-read failures return at :203-205, and unsuccessful proactive deliveries are only logged at :225-228. No retry is scheduled for any of these paths. Graph has already received a successful acknowledgement, and :173 limits later scans to ten minutes, so an otherwise isolated completion is not recovered automatically.
Suggested fix: Queue notification work before acknowledging it, and retry transient read or delivery failures with bounded backoff and a stable deduplication key. Preserve pending work independently of whether another list update occurs.
Comment on lines
+499
to
+503
| if (OpenAIClient.isTransientNetworkError(error) && attempt < maxAttempts) { | ||
| const delayMs = 1000 * attempt; | ||
| console.warn(`Transient network error on attempt ${attempt}/${maxAttempts}, retrying in ${delayMs}ms:`, (error as any)?.message || error); | ||
| await new Promise(resolve => setTimeout(resolve, delayMs)); | ||
| continue; |
Contributor
There was a problem hiding this comment.
Minor: Do not replay an entire conversation run after an external write
This retry restarts the original request even when an earlier step already sent an email or created a calendar event. A network failure while producing the final reply can therefore execute the same outbound action again, up to three times. Restricting recipients does not make these writes idempotent.
Evidence
client.ts:470-480 constructs input once, and :487-503 repeatedly calls run(this.agent, input, { context: ctx }) with that unchanged input. Only a successful final output is added to history at :494; intermediate successful actions are not retained for a retry. The Mail/Calendar capabilities attached at :427-447 include the user-requested sends described at :379-384. Scenario: 'email me my plan' successfully sends, the following inference request fails with ECONNRESET, and the retry starts again without the successful send in its input history.
Suggested fix: Limit retries to operations that are safe to repeat, or resume from recorded run state. Do not restart the whole run after an external write may have executed unless that write has a durable idempotency guard.
Comment on lines
+783
to
+788
| // WRITE #2 — UserState update. | ||
| const previousOverall = userRecord.state.OverallProgress; | ||
| const nextState = summary.passed | ||
| ? applyQuizPass(userRecord.state, input.courseId, quizResult, course) | ||
| : applyQuizFail(userRecord.state, input.courseId, quizResult); | ||
| await upsertUserState(graph, nextState, userRecord); |
Contributor
There was a problem hiding this comment.
Minor: Reuse the recorded quiz attempt when retrying a failed plan update
The audit row is created before the plan update, but the submitted marker is set only after both operations. If the audit POST succeeds and the UserState PATCH fails, the restored card allows the same answers to be submitted again. That creates another audit row and increments the attempt number for one logical submission, double-counting its assessment and weak-topic history.
Evidence
handlers.ts:755 increments attempts, :768-778 appends QuizResponses, and :788 updates UserState. A failure at :788 skips markGraded at :799. agent.ts:249-262 restores the cached quiz whenever wasGraded is false, preserving its answers. Retrying therefore reaches another append and another bumpAttempts. For a 4/5 submission whose first audit POST succeeds but plan PATCH returns 503, recovery records attempts 1 and 2 for the same submission.
Suggested fix: Give each submission a stable attempt ID and persist its audit record idempotently. On recovery, reuse the recorded assessment and retry the pending plan update instead of appending and grading it again.
| Portal row complete, or the user says "I completed X" → server generates 5 questions and keeps the answer key **server-side** → card without answers → submit → deterministic grading (short answers by one narrow LLM call) → pass = 4/5 → course Complete, skill level raised, progress recomputed → milestone / completion cascade → next queued quiz. | ||
|
|
||
| ### 6.3 Completion | ||
| 100% → LLM writes the email text, **code** sends it (To: user, Cc: manager) → on success the goal is marked Completed → the next conversation starts with the welcome card. The chat model is instructed never to send this email itself. |
Contributor
There was a problem hiding this comment.
Minor: Completion email can go only to the manager and still close the goal
Section 6.3 says code sends the completion email To the employee and Cc the manager, and only then marks the goal Completed. If the employee address cannot be resolved, the send path moves the manager from Cc to To, treats that as success, and closes the goal. The employee never receives the wrap-up, and a later progress check will not retry it.
Evidence
design.md:122 says code sends the email "(To: user, Cc: manager)" and "on success the goal is marked Completed". handlers.ts:998-1000 calls getMyProfile and swallows any error, leaving userEmail unset (an empty mail and UPN also leaves it unset). handlers.ts:1009-1012 then runs if (to.length === 0 && cc.length > 0) { to.push(cc[0]); cc.length = 0; }. A manager lookup that already succeeded puts the manager in cc, so this makes them the only recipient. sendMail then sets sent = true (handlers.ts:1018-1019), and handlers.ts:1033 writes Completion100Fired and PlanStatus Completed. The card at handlers.ts:1070 has a branch for "sent to your manager" when userEmail is missing. Example: getMyManager returns manager@contoso.com, getMyProfile throws 429, the career wrap-up is mailed only to the manager, and the goal is closed so "check my progress" will not send the employee their copy.
Suggested fix: Either do not retarget To and do not mark the goal Completed when the employee address is missing, or change section 6.3 and the "manager copied" wording so they describe this fallback and that it still completes the goal.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 insrc/llm-tasks.ts:CompetencyFramework_v2+ course recommendations fromLearningCatalog_v2(planReview card)UserState, one row per goal (Active/Completed/Replaced) so finished goals are kept as historyLearningPortalStatusagainst the plan and recomputes progressFull flows, data model and design decisions:
scenarios/career-coach/docs/design.mdandAGENT-CODE-WALKTHROUGH.md.Design principles
Action.Executehandler acks fast and runs deferred; cards lock after use and in-flight claims absorb double clicks.What's included
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:gpt-4orecommended)npm run setup:sharepoint), and the agent user added as a site Membera365 setup all --aiteammate,a365 setup blueprint --update-endpoint <url>/api/messages --m365,a365 publish --aiteammate, then an approved, licensed agent instanceAuth model
/meis 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.getAgenticUserToken(...)with the tenant / agent instance / agentic-user ids captured from the first message (orAGENT_INSTANCE_ID/AGENT_USER_ID). The developer device-code cache is only a local fallback for scripts and for the moments before the agent has seen its own ids. WithNODE_ENV=productionand a webhook configured, the agent refuses to start without a validPORTAL_EVENT_SECRETand the service connection. The notification read is filtered server-side to recently updated rows.gen_ai.agent.idis the runtime identity from the turn; the blueprint id is metadata only.Sites.Manage.All,Sites.ReadWrite.All,User.Read.All,Mail.Send,offline_access).Sites.Manage.AllandUser.Read.Allare admin-restricted, so a tenant/SharePoint admin performs - or pre-consents - the one-time sign-in. The blueprint'sSites.ReadWrite.Allgrant and the WorkIQ MCP grants (a365 setup permissions mcp) also need a Global Administrator.Observability
@microsoft/opentelemetrywith the Agent 365 exporter on the S2S route. Every path - LLM turns, card actions, typed shortcuts, email and proactive turns - emitsinvoke_agent(input/output messages),chat(model/provider, finish reason) andexecute_toolspans with caller, channel, session and conversation id. OpenAI Agents auto-instrumentation adds token usage; no fabricated token counts.Verification
npm install(no lockfile) →npm run buildandnpx tsc --noEmitpass.a365-code-validatorstatic scan: 0 findings.00000000-...,contoso.sharepoint.com,<CHANGE_ME_...>etc.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 tonodejs/openai/scenarios/career-coach/if reviewers prefer a language-tier scoping.Not included / future work
.mstoken-cache.json, gitignored).tests/e2e/. Recommend a follow-up PR once initial review lands.Related
nodejs/openai/sample-agent