[Eduard van Valkenburg (eavanvalkenburg)]

Motivation & Context

_RestrictedUnpickler (used when FileCheckpointStorage and other checkpoint
storages decode persisted workflow checkpoints) allowlisted builtins:type as
a safe built-in. Pickle's REDUCE opcode can call any allowlisted global;
calling type(qualname, bases, namespace) constructs a brand-new class whose
__module__/__qualname__ are chosen entirely by the payload being
deserialized, so the allowlist check on the global (find_class) does not
constrain what class actually gets instantiated. The integrity check that
compares the unpickled object's recorded type against its expected type
(_verify_type) only compares those same payload-chosen strings, so it does
not catch this either.

The allowlist's own stated purpose is to restrict which classes may be
instantiated, and handing back the class-factory type itself does not meet
that purpose.

Description & Review Guide

• What are the major changes?
  • Remove "builtins:type" from the default allowlist
    (_BUILTIN_ALLOWED_TYPE_KEYS).
  • Add a small _is_metaclass helper and use it everywhere
    _RestrictedUnpickler resolves a class (_is_allowed_type, the explicit
    allowed_types branch, and the agent_framework.*/openai.types. prefix
    branch of find_class), so the unpickler never hands back a metaclass
    (a subclass of type) regardless of which allowlisted name or module
    prefix was used to reach it. This also covers restricted
    getattr(dict, "__class__"), and module attributes that re-export type
    under another name inside the auto-allowed agent_framework.* prefix
    (there are two such aliases in the codebase today, both from builtins import type as builtin_type), with a single invariant instead of
    patching each case individually.
  • Add regression tests covering the direct global, both existing aliases,
    the enum.EnumMeta metaclass re-export, the getattr route, an
    explicitly-configured metaclass entry, and an end-to-end
    FileCheckpointStorage test that edits a saved checkpoint file on disk
    and asserts load() rejects it.
• What is the impact of these changes? Ordinary classes (dataclasses,
  enums, dict/list subclasses, named tuples, nested classes, etc.) are not
  subclasses of type and continue to round-trip exactly as before —
  verified by running the full existing core (tests/workflow) and
  orchestrations suites unchanged. The only values that stop being
  restorable by default are ones that are themselves the class objects
  type/NoneType (storing a class as checkpoint data, not an instance
  of one) — an exceedingly unusual thing to put in workflow state, and
  FileCheckpointStorage.save() already proactively rejects anything it
  could not restore under its own allowlist, so this surfaces as a clear
  save-time error rather than silent data loss or a later resume failure.
• What do you want reviewers to focus on? Whether the _is_metaclass
  check is applied at every point _RestrictedUnpickler can produce a class
  (the three call sites touched here), and whether any other legitimate
  checkpoint payload could plausibly need to reconstruct type itself as
  data rather than as a value's class.

Related Issue

N/A - trivial change. This narrows an existing allowlist (introduced to
restrict which classes a persisted checkpoint can instantiate) so that it
also excludes the universal class-factory type and its equivalents,
consistent with the allowlist's own documented purpose; the desired result —
this allowlist should not be able to construct arbitrary new classes — is not
reasonably in dispute, and the change is behavior-preserving for every
legitimate checkpoint value exercised by the existing test suites.

AI Assistance

• No material AI assistance was used.
• This is an AI-assisted contribution. I reviewed, understood, and verified all submitted content and accept responsibility for it.

AI assistance details:

GitHub Copilot assisted with the implementation, tests, and validation under
my direction: it implemented the fix, added the regression tests, confirmed
those tests fail without the fix and pass with it, and ran the full existing
core and orchestrations test suites plus the repository's lint/type-check
tooling (ruff, strict Pyright, and the mypy/pyrefly/ty/zuban/relaxed-
Pyright test-typing checks) with no regressions. I reviewed and verified all
submitted content.

Contribution Checklist

• The code builds clean without any errors or warnings
• All unit tests pass, and I have added new tests where possible
• The PR follows the Contribution Guidelines
• This PR links to an agreed issue with no competing open PR, or the Related Issue section documents a trivial-change or repository-automation exception.
• This is not a breaking change. If it is a breaking change, add the breaking change label (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.

[Copilot]

🟡 Changes recommended

A reusable pickle builder remains defined in the test file, violating the coding standard requiring non-fixture test helpers to live in a separate helper module.

1 open issue

• Move reusable pickle builder to a helper module · New

What changed in this PR

Hardens Python workflow checkpoint deserialization by excluding metaclasses from restricted unpickling.

Changes:

• Removes builtins:type from the default allowlist.
• Rejects metaclasses reached through explicit allowlists, framework re-exports, and restricted attribute traversal.
• Adds regression coverage for class-fabrication payloads, including a tampered checkpoint file.

File	Description
python/​packages/​core/​agent_framework/​_workflows/​_checkpoint_encoding.py	Adds metaclass detection and rejection across restricted type-resolution paths.
python/​packages/​core/​tests/​workflow/​test_checkpoint_unrestricted_pickle.py	Tests direct and indirect metaclass access and file-storage tampering.

_{Review effort: Balanced}

[github-actions]

MAF Automated Review — Iteration 1

Result: Findings reported
Scope: full PR (1 commit(s)): c962ccb2011f
Model: gpt-5.6-sol

Overview

The PR removes the direct builtins:type primitive and consistently rejects metaclasses reached through nested traversal, explicit allowlisting, and framework-prefix aliases. Its regression coverage exercises those routes and the file-storage load path. Two residual risks remain: the prefix allowlist still exposes the functional Enum class factory, and the unconditional rejection makes some checkpoints written by prior releases impossible to resume without migration.

Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
2 verified findings remained after source verification (2 medium) across 1 file. Details are attached to the affected lines below.

Affected areas: python/packages/core/agent_framework/_workflows/_checkpoint_encoding.py