feat: use SystemMessageConfig with SectionOverride for worker prompts (fixes #496)#822
feat: use SystemMessageConfig with SectionOverride for worker prompts (fixes #496)#822github-actions[bot] wants to merge 8 commits intomainfrom
Conversation
Refactor worker prompt construction from string concatenation in BuildWorkerPrompt() to use the SDK's SystemMessageConfig with Customize mode and SectionOverride. This places worker-specific content in the appropriate system prompt sections instead of the user message: - Identity/charter → SystemPromptSections.Identity (Append) - Tool honesty policy → SystemPromptSections.ToolEfficiency (Append) - Worktree note → SystemPromptSections.EnvironmentContext (Append) - Shared context → SystemPromptSections.CustomInstructions (Append) Key changes: - Add BuildWorkerSystemMessageSections() to construct section overrides - Add systemMessageSections parameter to CreateSessionAsync - Update BuildFreshSessionConfig to detect workers and add sections - Simplify BuildWorkerPrompt to only contain task-specific content - Update group creation flows to pass sections at session creation time Fixes #496 Co-authored-by: copilot-agentic-workflow[bot] <224017+copilot-agentic-workflow[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cross-Platform Verification — PR #822Build Results
✅ All platforms verified Triggered by: verify-build run |
🧪 Integration Test Report — PR #822
✅ All platforms passed |
Cross-Platform Verification — PR #822Build Results
✅ All platforms verified Triggered by: verify-build run |
🧪 Integration Test Report — PR #822
✅ All platforms passed |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Found 2 issues — one regression and one unverified SDK behavior risk.
🔴 Worker identity/context frozen at session creation time (regression)
CopilotService.Organization.cs:2506-2512
The old ExecuteWorkerAsync read meta.SystemPrompt, group.SharedContext, and worktree info dynamically on every dispatch. The refactored code bakes these into SystemMessageConfig sections at session creation time, and the SDK has no API to update the system message post-creation. This means:
SetSessionSystemPrompt()updatesmeta.SystemPrompt(used by the orchestrator's planning prompt), but the worker's own system message is stale → orchestrator routes work assuming a persona the worker doesn't reflect.group.SharedContextchanges afterCreateMultiAgentGroupAsyncare invisible to existing workers.- Only session revival (error recovery) refreshes sections via
BuildWorkerSectionsForSession.
🟡 SystemMessageMode.Customize + Content interaction undocumented
CopilotService.cs:2829-2834
First use of Customize mode in the codebase. SDK skill reference (v0.2.2) doesn't document this mode (new in 0.3.0). If Content is ignored in Customize mode, MCP server guidance and relaunch instructions are silently lost for worker sessions.
Warning
⚠️ Firewall blocked 4 domains
The following domains were blocked by the firewall during workflow execution:
api.nuget.orgdc.services.visualstudio.compkgs.dev.azure.comwww.nuget.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "api.nuget.org"
- "dc.services.visualstudio.com"
- "pkgs.dev.azure.com"
- "www.nuget.org"See Network Configuration for more information.
Generated by Expert Code Review · ● 67.9M
| var sw = System.Diagnostics.Stopwatch.StartNew(); | ||
| await EnsureSessionModelAsync(workerName, cancellationToken); | ||
|
|
||
| // Use per-worker system prompt if set, otherwise generic. | ||
| // Note: .github/copilot-instructions.md is auto-loaded by the SDK for each session's working directory, | ||
| // so workers already inherit repo-level copilot instructions without explicit injection here. | ||
| var meta = GetSessionMeta(workerName); | ||
| var identity = !string.IsNullOrEmpty(meta?.SystemPrompt) | ||
| ? meta.SystemPrompt | ||
| : "You are a worker agent. Complete the following task thoroughly."; | ||
|
|
||
| // Inject shared context (e.g., Squad decisions.md) if the group has it | ||
| var group = meta != null ? Organization.Groups.FirstOrDefault(g => g.Id == meta.GroupId) : null; | ||
| var sharedPrefix = !string.IsNullOrEmpty(group?.SharedContext) | ||
| ? $"## Team Context (shared knowledge)\n{group.SharedContext}\n\n" | ||
| : ""; | ||
|
|
||
| // Inject worktree awareness if the worker has an isolated worktree | ||
| var wtInfo = meta?.WorktreeId != null | ||
| ? _repoManager.Worktrees.FirstOrDefault(wt => wt.Id == meta.WorktreeId) : null; | ||
| var worktreeNote = wtInfo != null && group?.WorktreeStrategy != WorktreeStrategy.Shared | ||
| ? $"\n\n## Your Worktree\nYou have an isolated git worktree at `{wtInfo.Path}` (branch: {wtInfo.Branch}). " + | ||
| "You can safely run any git operations without affecting other workers. " + | ||
| "To check out a PR: `git fetch origin pull/<N>/head:pr-<N> && git checkout pr-<N>`\n" | ||
| : ""; | ||
|
|
||
| var workerPrompt = BuildWorkerPrompt(identity, worktreeNote, sharedPrefix, originalPrompt, task); | ||
| // Worker identity, worktree note, shared context, and tool honesty policy are now | ||
| // delivered via SystemMessageConfig sections (set at session creation/revival time). | ||
| // The user prompt contains only the task-specific content. | ||
| var workerPrompt = BuildWorkerPrompt(originalPrompt, task); |
There was a problem hiding this comment.
🔴 Regression: SetSessionSystemPrompt changes silently ignored for running workers
The old code read meta.SystemPrompt and group.SharedContext at dispatch time (every ExecuteWorkerAsync call). The new code bakes these values into SystemMessageConfig sections at session creation time. The SDK provides no API to update the system message after session creation, so these values are frozen for the session's lifetime.
Concrete failing scenario:
- User creates a group via
CreateMultiAgentGroupAsync→ workers get generic "You are a worker agent" identity in sections - User calls
SetSessionSystemPrompt("GroupName-Worker-1", "You are a security auditor.")— meta is updated - Orchestrator plans tasks and sees "security auditor" persona (reads
meta.SystemPromptinBuildOrchestratorPlanningPrompt) — so it routes security-related work there ExecuteWorkerAsyncsends only the task prompt; the worker's system message still says "You are a worker agent" — the persona mismatch causes the worker to ignore its specialization
The same applies to group.SharedContext changes after creation (e.g., updating team decisions mid-session). The revival path (BuildWorkerSectionsForSession) reads live values, but revival only triggers on error recovery — not on every dispatch.
Suggested fix: Either (a) keep identity/context in the user prompt so it's always fresh (mirror the old behavior alongside sections), or (b) add a session recreation or SDK system message update when SetSessionSystemPrompt or SharedContext changes for workers.
| SystemMessage = systemMessageSections != null | ||
| ? new SystemMessageConfig | ||
| { | ||
| Mode = SystemMessageMode.Customize, | ||
| Sections = systemMessageSections, | ||
| Content = systemContent.ToString() |
There was a problem hiding this comment.
🟡 Risk: Content behavior under SystemMessageMode.Customize is unverified
The SDK reference (v0.2.2) documents modes as "append/prepend/replace" — Customize is new in SDK 0.3.0 and not documented in the skill reference. This code sets both Sections and Content in Customize mode, expecting both to be used. If the SDK ignores Content in Customize mode, worker sessions silently lose:
- MCP server guidance ("
If an MCP tool call fails... suggest /mcp reload") - The relaunch script instructions (when working directory matches ProjectDir)
This is the first use of SystemMessageMode.Customize anywhere in the codebase, so there's no existing precedent to validate against.
Suggested fix: Add an integration test or manual verification that confirms Content is included in the final system prompt when Mode = Customize with Sections set. Alternatively, move the MCP guidance and relaunch instructions into an additional section override (e.g., SystemPromptSections.EnvironmentContext or a custom section key) so there's no dependency on Content being used alongside Sections.
| var meta = GetSessionMeta(sessionName); | ||
| if (meta?.Role != MultiAgentRole.Worker) return null; | ||
|
|
||
| var identity = !string.IsNullOrEmpty(meta.SystemPrompt) | ||
| ? meta.SystemPrompt | ||
| : "You are a worker agent. Complete the following task thoroughly."; | ||
|
|
||
| var group = Organization.Groups.FirstOrDefault(g => g.Id == meta.GroupId); |
There was a problem hiding this comment.
🟡 MODERATE — Organization.Groups/Sessions read without lock from background thread (2/3 reviewers)
BuildWorkerSectionsForSession is called from BuildFreshSessionConfig, which runs on background reconnect threads (lines ~4046, ~4078, ~4900 — all inside Task.Run or async continuations). GetSessionMeta (line 4424) reads Organization.Sessions.FirstOrDefault and line 4431 reads Organization.Groups.FirstOrDefault — both without synchronization.
Concrete scenario: A concurrent CreateMultiAgentGroupAsync on the UI thread calls Organization.Sessions.Add(...) while a worker revival is running BuildWorkerSectionsForSession on a background thread → InvalidOperationException: Collection was modified from FirstOrDefault, crashing the reconnect and orphaning the session.
Fix: Use the existing thread-safe snapshot helpers:
private Dictionary<string, SectionOverride>? BuildWorkerSectionsForSession(string sessionName)
{
var metas = SnapshotSessionMetas();
var meta = metas.FirstOrDefault(m => m.SessionName == sessionName);
if (meta?.Role != MultiAgentRole.Worker) return null;
// ...
var groups = SnapshotGroups();
var group = groups.FirstOrDefault(g => g.Id == meta.GroupId);
// ...
}| // Worker identity, worktree note, shared context, and tool honesty policy are now | ||
| // delivered via SystemMessageConfig sections (set at session creation/revival time). | ||
| // The user prompt contains only the task-specific content. | ||
| var workerPrompt = BuildWorkerPrompt(originalPrompt, task); |
There was a problem hiding this comment.
🟡 MODERATE — Behavioral regression: dynamic→static worker context binding (3/3 reviewers)
The old ExecuteWorkerAsync re-read meta.SystemPrompt, group.SharedContext, and meta.WorktreeId at every dispatch. The new code bakes these into SystemMessageConfig.Sections at session creation time.
Concrete scenario: A user creates a Squad team, dispatches initial work, then edits decisions.md (updating SharedContext) or changes a worker's charter via SetSessionSystemPrompt. The next dispatch to the same worker still uses the original values — the update is invisible until the session dies and gets fresh-recreated.
This also affects the simple CreateMultiAgentGroupAsync path (line 131), which hardcodes empty worktree and shared context. If a user later assigns a worktree or adds shared context to the group, those values are never reflected.
Fix options:
- Document this as a known behavioral change ("charter/context changes take effect on next session revival")
- On
SetSessionSystemPromptorSharedContextchange, trigger session recreation for affected workers - Keep charter in the user prompt for the current-dispatch path (old approach) and use sections only for fresh/revived sessions
| /// </summary> | ||
| [Collection("PolyPilot")] | ||
| [Trait("Category", "WorkerSystemMessage")] | ||
| public class WorkerSystemMessageTests : IntegrationTestBase |
There was a problem hiding this comment.
🟢 MINOR — Integration tests don't validate worker system message behavior (2/3 reviewers)
WorkerSystemMessageTests contains two generic smoke tests:
Dashboard_ShowsMultiAgentGroupCreation— verifies the dashboard has contentApp_RespondsToApiStatus— verifies the/api/statusendpoint
Neither test validates that CreateSessionAsync correctly uses SystemMessageMode.Customize with section overrides for worker sessions. Consider either renaming the class to match what it actually tests (e.g., AppSmoke_WorkerRefactorTests), or adding a test that creates a multi-agent group and verifies the worker session's configuration includes expected sections.
| SystemMessage = systemMessageSections != null | ||
| ? new SystemMessageConfig | ||
| { | ||
| Mode = SystemMessageMode.Customize, | ||
| Sections = systemMessageSections, | ||
| Content = systemContent.ToString() |
There was a problem hiding this comment.
🔴 CRITICAL — Content may be silently dropped in Customize mode (3/3 reviewers)
When systemMessageSections != null, the config uses SystemMessageMode.Customize with both Sections and Content. The SDK reference documents Mode as "append/prepend/replace" — Customize is not documented (possibly a newer SDK addition).
The risk: If Customize mode ignores the Content field (only using Sections), every worker session silently loses:
- The "CRITICAL BUILD INSTRUCTION: ALWAYS run the relaunch script" warning (built into
systemContentwhensessionDir == ProjectDir) - All MCP server guidance appended by
AppendMcpServerGuidance
Concrete scenario: A PolyPilot-directory worker makes code changes but the relaunch instruction is missing from its system prompt, so it attempts dotnet build + open separately — exactly the failure the relaunch instruction exists to prevent.
The same pattern appears in BuildFreshSessionConfig (line 4397), so revived workers are equally affected.
Fix: Before merging, empirically verify that Customize mode delivers both Content and Sections to the model. If Content is ignored, move the relaunch instructions and MCP guidance into a dedicated SectionOverride entry (e.g., appended to SystemPromptSections.EnvironmentContext). Also update the copilot-sdk-reference skill with Customize mode semantics.
| { | ||
| Mode = SystemMessageMode.Customize, | ||
| Sections = systemMessageSections, | ||
| Content = systemContent.ToString() |
There was a problem hiding this comment.
🟡 MODERATE · 2/3 reviewers · Content field behavior in Customize mode unverified
When systemMessageSections is provided, Content (containing MCP server failure guidance built by AppendMcpServerGuidance — the /mcp reload instructions) is set alongside Mode = Customize and Sections. The SDK docs don't explicitly document whether Content is honored when Mode = Customize. If the SDK ignores Content in Customize mode, all worker sessions lose MCP failure guidance.
Scenario: Worker session with MCP servers configured. An MCP tool fails. The worker has no /mcp reload guidance because Content was silently dropped.
Suggestion: Verify empirically that Customize mode honors both Sections and Content. If not, pipe MCP guidance into an additional named section entry in the sections dictionary.
| // Worker identity, worktree note, shared context, and tool honesty policy are now | ||
| // delivered via SystemMessageConfig sections (set at session creation/revival time). | ||
| // The user prompt contains only the task-specific content. | ||
| var workerPrompt = BuildWorkerPrompt(originalPrompt, task); |
There was a problem hiding this comment.
🟡 MODERATE · 3/3 reviewers · Stale sections: dynamic state → static binding
Old ExecuteWorkerAsync re-read meta.SystemPrompt, group.SharedContext, and worktree info on every dispatch call, so mid-flight decisions.md updates were automatically picked up. The new code bakes these into SystemMessageConfig sections at session creation time only.
Scenario: A squad group is created, the user edits decisions.md to add a critical constraint (e.g., "Do NOT touch the auth module"), then triggers a multi-agent dispatch. Workers' CustomInstructions section still reflects the pre-edit decisions. Similarly, worktree branch changes after session creation aren't reflected in the EnvironmentContext section.
Suggestion: Either document this as intentional (creation-time binding), re-derive sections before each dispatch if the SDK supports updating them, or fall back to including dynamic fields (SharedContext) in the user prompt alongside task content when they've changed.
| ? meta.SystemPrompt | ||
| : "You are a worker agent. Complete the following task thoroughly."; | ||
|
|
||
| var group = Organization.Groups.FirstOrDefault(g => g.Id == meta.GroupId); |
There was a problem hiding this comment.
🟡 MODERATE · 3/3 reviewers (via follow-up) · Thread safety: unsynchronized Organization.Groups read
BuildWorkerSectionsForSession reads Organization.Groups (a plain List<T>) via FirstOrDefault without synchronization. Per project conventions (confirmed by the snapshot pattern at ~line 3766: var groupSnapshots = Organization.Groups.ToList()), these lists must only be enumerated from the UI thread. However, BuildFreshSessionConfig → BuildWorkerSectionsForSession is called from reconnect/revival paths that run on background threads (Task.Run, EnsureSessionConnectedAsync).
Scenario: UI thread mutates Organization.Groups (adding/removing a group) concurrently with a background reconnect calling BuildWorkerSectionsForSession → list enumerator corruption.
Suggestion: Snapshot the needed values (group.SharedContext, group.WorktreeStrategy) before calling from background threads, or use SnapshotGroups() inside this method.
|
|
||
| [Fact] | ||
| public async Task Dashboard_ShowsMultiAgentGroupCreation() | ||
| { |
There was a problem hiding this comment.
🟢 MINOR · 3/3 reviewers · Integration tests don't validate the feature
Both test methods are pure smoke tests — Dashboard_ShowsMultiAgentGroupCreation checks the body has content and takes a screenshot; App_RespondsToApiStatus checks agentReady. Neither creates a multi-agent group, creates a worker session, nor inspects whether the session uses SystemMessageMode.Customize with section overrides. These tests pass identically whether the PR's changes are present or reverted.
Suggestion: Either rename to reflect what's actually tested (e.g., AppBootstrapTests) or replace with tests that create a worker group and verify the session's SystemMessageConfig includes the expected sections.
MergeDynamicContentIntoSections pipes relaunch instructions and MCP guidance into EnvironmentContext section overrides instead of relying on Content being honored alongside Sections in Customize mode. Also uses thread-safe SnapshotSessionMetas/SnapshotGroups in BuildWorkerSectionsForSession to prevent collection-modified crashes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
BuildWorkerPrompt now accepts optional freshIdentity and freshSharedContext params. ExecuteWorkerAsync re-reads current meta.SystemPrompt and group.SharedContext at dispatch time, ensuring mid-session changes (e.g., edited decisions.md or SetSessionSystemPrompt) are reflected even though system message sections are baked at creation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…oke tests - Add 5 new tests: BuildWorkerPrompt with fresh identity/SharedContext, MergeDynamicContentIntoSections merge/create/no-op behaviors - ConnectionRecoveryTests: add Customize mode + BuildWorkerSectionsForSession assertions to catch regressions in the worker branch - Rename integration tests to AppBootstrap to honestly reflect scope Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Commit pushed:
|
Review-Fix Loop — Round 1 of 3Findings Addressed: 5/5
Skipped: 0Test Results✅ 3682 passed, 0 failed, 0 skipped Next StepsExpert review round 2 and verify-build dispatched. Warning
|
Cross-Platform Verification — PR #822Build Results
✅ All platforms verified Previous Review HistoryFound 3 automated review(s) on this PR. Build verification validates that all review-driven fixes compile and pass tests across platforms. Triggered by: verify-build run |
… reads ExecuteWorkerAsync runs on background threads (dispatched via Task.WhenAll from orchestration). It was reading Organization.Groups and Organization.Sessions directly via GetSessionMeta/FirstOrDefault without synchronization, risking InvalidOperationException if the UI thread mutates these lists concurrently. Round 1 fixed this in BuildWorkerSectionsForSession but missed the same pattern in ExecuteWorkerAsync. Now uses SnapshotSessionMetas() and SnapshotGroups() for thread-safe reads. Co-authored-by: copilot-agentic-workflow[bot] <224017+copilot-agentic-workflow[bot]@users.noreply.github.com>
Addresses review finding: integration tests are smoke tests that verify app bootstrap, not worker system message behavior. Renaming the class and file to honestly reflect what they test. Co-authored-by: copilot-agentic-workflow[bot] <224017+copilot-agentic-workflow[bot]@users.noreply.github.com>
|
Commit pushed:
|
Review-Fix Loop — Round 2 of 3Findings Addressed: 2/2
Skipped: 0Test Results✅ 3682 passed, 0 failed, 0 skipped Next StepsExpert review round 3 and verify-build dispatched. Warning
|
Cross-Platform Verification — PR #822Build Results
✅ All platforms verified Previous Review HistoryFound 3 automated review(s) on this PR. Build verification validates that all review-driven fixes compile and pass tests across platforms. Triggered by: verify-build run |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Expert Code Review — PR #822Methodology: 3 independent reviewers with adversarial consensus 4 findings posted as inline comments (3 moderate, 1 minor)
Discarded Findings (single reviewer only, failed consensus)
CI StatusNo check runs found for the head commit. Test CoverageThe PR includes comprehensive test updates:
|
There was a problem hiding this comment.
Expert Code Review: 4 findings posted inline (3 moderate, 1 minor). See summary comment below for full details.
Generated by Expert Code Review · ● 20M
| /// provided, ensuring dispatch-time changes are picked up even when sections are stale. | ||
| /// </summary> | ||
| internal static string BuildWorkerPrompt(string originalPrompt, string task, | ||
| string? freshIdentity = null, string? freshSharedContext = null) |
There was a problem hiding this comment.
🟡 MODERATE · 3/3 reviewers · Worktree info no longer refreshed at dispatch time
The old ExecuteWorkerAsync re-read meta.WorktreeId and injected the worktree note into the user prompt at every dispatch. The refactored code removes this entirely — worktree context now exists only in system message sections baked at session creation time.
If the worktree's branch changes between session creation and dispatch (e.g., git checkout), or if a worktree is assigned to a simple-path worker after creation (CreateMultiAgentGroupAsync creates workers with worktreeNote: ""), the worker sees stale or missing worktree instructions with no user-prompt fallback.
Suggestion: Mirror the approach used for identity/shared context — add freshWorktreeNote as an optional parameter to BuildWorkerPrompt and recompute it in ExecuteWorkerAsync from the current meta.WorktreeId.
| @@ -2710,7 +2744,7 @@ await FinalizeResumedSessionUiStateAsync( | |||
| private static Task<PermissionRequestResult> AutoApprovePermissions(PermissionRequest request, PermissionInvocation invocation) | |||
There was a problem hiding this comment.
🟡 MODERATE · 2/3 reviewers · Other CreateSessionAsync callers for workers don't pass systemMessageSections
The new systemMessageSections parameter is only passed from 3 call sites (simple CreateMultiAgentGroupAsync, Squad setup, and BuildFreshSessionConfig revival). But at least two other paths create worker sessions without sections:
-
RecreateSessionAsync(~line 3307) — called when a user changes a worker's model on a session with zero history. It callsCreateSessionAsyncwithoutsystemMessageSections, so the recreated session usesAppendmode and loses the tool honesty policy thatExecuteWorkerAsyncno longer injects via the user prompt. -
CreateMultiAgentGroupwith existingsessionNames— adopts already-created sessions as workers without recreating them. These sessions were created without sections, so they lack tool honesty, identity, and worktree sections.
Suggestion: In RecreateSessionAsync, call BuildWorkerSectionsForSession(name) before CloseSessionAsync (while meta is still available) and pass the result to CreateSessionAsync. For adopted sessions, consider recreating them with sections or ensuring tool honesty is still delivered via the user prompt as a fallback.
| /// This avoids relying on <c>Content</c> being honored alongside <c>Sections</c> in | ||
| /// <see cref="SystemMessageMode.Customize"/> mode. | ||
| /// </summary> | ||
| internal static Dictionary<string, SectionOverride> MergeDynamicContentIntoSections( |
There was a problem hiding this comment.
🟢 MINOR · 3/3 reviewers · MergeDynamicContentIntoSections mutates the input dictionary in-place
This method modifies sections[EnvironmentContext] in the passed-in dictionary and returns the same reference. Currently safe because each call site creates a fresh dictionary via BuildWorkerSystemMessageSections. However, the method name ("Merge...Into...") and the return sections pattern imply functional composition, not mutation — a future caller that caches or reuses the dictionary across retries or sessions would silently accumulate duplicate environment content.
Suggestion: Either clone the dictionary before mutating (new Dictionary<string, SectionOverride>(sections)), or add a // Mutates sections in-place remark to the XML doc to signal the side-effect.
| var freshSharedContext = group?.SharedContext ?? ""; | ||
| var freshIdentity = meta?.SystemPrompt; | ||
|
|
||
| var workerPrompt = BuildWorkerPrompt(originalPrompt, task, freshIdentity, freshSharedContext); |
There was a problem hiding this comment.
🟡 MODERATE · 3/3 reviewers · Duplicate identity/shared context in system message + user prompt
When meta.SystemPrompt or group.SharedContext are non-empty (the common Squad case), the model receives the same content twice: once from the system message sections (baked at session creation) and once from the user prompt (injected here at dispatch time).
This creates two problems:
- Token waste proportional to charter +
decisions.mdsize — can be significant for large team contexts. - Conflicting instructions — if the values changed after session creation, the system section has the stale creation-time value and the user prompt has the fresh dispatch-time value. Models are inconsistent about which they honor when system vs. user messages diverge.
Suggestion: Pick one source of truth. If dispatch-time freshness matters, only include changed values in the user prompt (compare against what was baked into sections). Alternatively, skip duplicating identity entirely (system section always has it) and only pass shared context in the user prompt when it has actually changed.
…nding 13) Identity and shared context are now delivered exclusively via SystemMessageConfig sections. Removes freshIdentity/freshSharedContext from BuildWorkerPrompt and ExecuteWorkerAsync to avoid token waste and conflicting instructions when system sections contain stale creation-time values. Adds fresh worktree note to BuildWorkerPrompt instead (finding 10) since worktree branch/path can change between session creation and dispatch. Updates tests to match new BuildWorkerPrompt signature. Co-authored-by: copilot-agentic-workflow[bot] <224017+copilot-agentic-workflow[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ding 11) RecreateSessionAsync now calls BuildWorkerSectionsForSession before closing the session, preserving worker identity, tool honesty, worktree, and shared context sections when a worker's model is changed on a zero-history session. Also adds mutation warning to MergeDynamicContentIntoSections XML doc (finding 12). Co-authored-by: copilot-agentic-workflow[bot] <224017+copilot-agentic-workflow[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Commit pushed:
|
Review-Fix Loop — Round 2 of 3Findings Addressed: 4 of 5
Test Results✅ 3683 passed, 0 failed, 0 skipped Next StepsExpert review and verify-build dispatched for round 3. Warning
|
Cross-Platform Verification — PR #822Build Results
✅ All platforms verified Previous Review HistoryFound 3 automated review(s) on this PR. Build verification validates that all review-driven fixes compile and pass tests across platforms. Triggered by: verify-build run |
There was a problem hiding this comment.
Expert Code Review: 1 finding posted inline (1 moderate). See summary comment for full details.
Warning
⚠️ Firewall blocked 2 domains
The following domains were blocked by the firewall during workflow execution:
api.nuget.orgdc.services.visualstudio.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "api.nuget.org"
- "dc.services.visualstudio.com"See Network Configuration for more information.
Generated by Expert Code Review · ● 72.6M
| // For worker sessions, build system message sections BEFORE closing (meta is still available). | ||
| // Without this, the recreated session loses identity, tool honesty, worktree, and shared context. | ||
| var workerSections = BuildWorkerSectionsForSession(name); | ||
|
|
||
| await CloseSessionAsync(name); | ||
|
|
||
| return await CreateSessionAsync(name, newModel, workingDir, groupId: groupId); | ||
| return await CreateSessionAsync(name, newModel, workingDir, groupId: groupId, | ||
| systemMessageSections: workerSections); |
There was a problem hiding this comment.
🟡 MODERATE · 2/2 reviewers agreed · RecreateSessionAsync loses worker metadata on subsequent restarts
Worker sections are correctly captured before CloseSessionAsync (line 3349), so the immediate recreation works. However, CloseSessionAsync removes the SessionMeta (including Role = Worker, SystemPrompt, WorktreeId). After recreation, the new session's meta only has GroupId restored.
Failing scenario: User changes a worker's model → RecreateSessionWithModelAsync runs → initial session works with correct sections. App restarts → BuildFreshSessionConfig → BuildWorkerSectionsForSession sees meta.Role != Worker → returns null → session revives with SystemMessageMode.Append instead of Customize, losing identity, tool honesty policy, and shared context.
Suggested fix: Preserve and restore critical worker meta fields after recreation:
var savedRole = meta?.Role;
var savedSystemPrompt = meta?.SystemPrompt;
var savedWorktreeId = meta?.WorktreeId;
await CloseSessionAsync(name);
var result = await CreateSessionAsync(name, newModel, workingDir, groupId: groupId,
systemMessageSections: workerSections);
var newMeta = GetSessionMeta(name);
if (newMeta != null && savedRole != null)
{
newMeta.Role = savedRole.Value;
newMeta.SystemPrompt = savedSystemPrompt;
newMeta.WorktreeId = savedWorktreeId;
}
return result;
|
The latest expert review found 4 findings (3 moderate, 1 minor) that were not addressed in the 3 automated rounds:
These require manual review and resolution.
|
Summary
Refactors worker prompt construction from string concatenation in
BuildWorkerPrompt()to use the SDK'sSystemMessageConfigwithCustomizemode andSectionOverride. This places worker-specific system content in the appropriate system prompt sections instead of concatenating everything into the user message.What Changed
Section Mapping
SystemPromptSections.IdentitySystemPromptSections.ToolEfficiencySystemPromptSections.EnvironmentContextSystemPromptSections.CustomInstructionsCode Changes
CopilotService.Organization.cs:BuildWorkerSystemMessageSections()— constructsDictionary<string, SectionOverride>for worker-specific content using the SDK's structured section systemBuildWorkerPrompt()— now only contains task-specific content (Original User Request+Assigned Task)ExecuteWorkerAsync()— no longer builds identity/worktree/shared context inline; these are delivered via system message sections set at session creationCreateMultiAgentGroupAsyncand the detailed Squad setup) to pass worker sections toCreateSessionAsyncCopilotService.cs:systemMessageSectionsparameter toCreateSessionAsync— when provided, usesSystemMessageMode.Customizewith sections instead ofSystemMessageMode.AppendBuildFreshSessionConfig— detects worker sessions viaBuildWorkerSectionsForSession()and includes sections for session revival/reconnectionBuildWorkerSectionsForSession()— looks up worker meta (charter, worktree, shared context) and delegates toBuildWorkerSystemMessageSectionsTest Changes
WorkerToolHonestyTests.cs— updated from reflection-based tests on old 5-paramBuildWorkerPromptto direct tests on:BuildWorkerPrompt()— verifies only task content (no system-level content)BuildWorkerSystemMessageSections()— verifies each section (Identity, ToolEfficiency, EnvironmentContext, CustomInstructions) with correct action and contentWorkerSystemMessageTests.cs(integration) — verifies app initializes correctly with refactoredCreateSessionAsyncWhy This Is Better
BuildFreshSessionConfigre-derives worker sections from meta, so revived sessions get correct system messagesRisk
Low — same content, different delivery mechanism. The prompt text is identical, just delivered through SDK sections instead of string concatenation in the user message. Backward compatible: sessions created without sections fall through to the existing
Appendmode path.Test Results
Unit tests: 3676 passed, 0 failed (1 flaky timing test unrelated to changes)
Integration tests: build verified
Fixes feat: Use SystemMessageConfig with SectionOverride for worker prompts #496
Warning
The following domain was blocked by the firewall during workflow execution:
192.0.2.1To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.