Skip to content

Fix magentic ledger bug - #14372

Merged
westey (westey-m) merged 2 commits into
microsoft:mainfrom
westey-m:magentic-ledger-fix
Sep 4, 2026
Merged

westey (westey-m) merged 2 commits into
microsoft:mainfrom
westey-m:magentic-ledger-fix

Conversation

@westey-m

Copy link
Copy Markdown
Contributor

Motivation and Context

Description

Contribution Checklist

Copilot AI lite review requested due to automatic review settings September 3, 2026 18:37
@westey-m
westey (westey-m) requested a review from a team as a code owner September 3, 2026 18:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The change is small, well-scoped, and the updated/new unit tests align with the intended role-safety behavior.

Pull request overview

This PR adjusts Magentic’s “task ledger” message to avoid using the System role (which can over-trust interpolated/untrusted content) by emitting it as a User message instead, and updates/adds unit tests to reflect and guard the behavior.

Changes:

  • Change StandardMagenticManager ledger output role from AuthorRole.System to AuthorRole.User.
  • Update existing unit tests to expect the new ledger role.
  • Add a regression test ensuring task content is not surfaced in any System-role message.
File summaries
File Description
dotnet/src/Agents/Magentic/StandardMagenticManager.cs Emits the rendered ledger as a User-role message to avoid elevating interpolated content to System.
dotnet/src/Agents/UnitTests/Magentic/StandardMagenticManagerTests.cs Updates role assertions and adds a regression test covering System-role injection/placement concerns.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Lite

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

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

MAF Automated Review — Iteration 1

Result: No findings
Scope: full PR (1 commit(s)): caa1338d8649
Model: claude-opus-4.8

Overview

This PR changes the Magentic task ledger message from AuthorRole.System to AuthorRole.User in StandardMagenticManager.PrepareTaskLedgerAsync, so that task/fact/plan content derived from agent and tool output is no longer presented to the model as a trusted system instruction. The change is confined to one internal, private return site reached symmetrically by PlanAsync and ReplanAsync, and no downstream orchestration code branches on the System role (the only role predicate selects Assistant for partial results), so the shipped control flow is unaffected. Unit tests were updated and a regression test was added asserting no System-role message reaches the chat service; the suite passes. No publishable Critical/High/Medium defect was substantiated: the residual observations (cross-SDK role divergence from Python, first-evaluation message shape, and test framing) are either pre-existing, unverified, or Low-priority polish.

Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
No publishable findings remained after source verification for this scope.

@westey-m
westey (westey-m) added this pull request to the merge queue Sep 4, 2026
Merged via the queue into microsoft:main with commit f8c5ba7 Sep 4, 2026
18 checks passed
@westey-m
westey (westey-m) deleted the magentic-ledger-fix branch September 4, 2026 10:30

This branch was previously deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants