feat(policy-engine)!: remove the deprecated Decision.applies_effects - #4058
david-long1 wants to merge 2 commits into
Conversation
The property has been a deprecated alias of applies_transform since the AGT D1 effects removal, and the DeprecationWarning it emitted on every access fails any consumer suite run with -W error, including through third-party code the host does not control. Remove it; applies_transform and permits are the two questions it conflated. Refs microsoft#4048 Signed-off-by: David Long <davidlong@berkeley.edu>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
github-actions
Bot
added
documentation
|
🟡 Contributor Check: MEDIUM
Automated check by AGT Contributor Check. |
PR Review Summary
Verdict: AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims. |
github-actions
Bot
added
the
needs-review:MEDIUM
MohammadHaroonAbuomar
left a comment
MohammadHaroonAbuomar
left a comment
There was a problem hiding this comment.
- Cross-SDK parity: Python drops the alias while Node keeps
appliesEffects(sdk/node/src/adapter-helpers.ts:88, with a test) and .NET keepsAppliesEffects(Primitives.cs:123, [Obsolete], with a harness). The body does not mention them. Either state that this is a Python-only removal and why, or open follow-ups for the other two so the SDKs stay in step. Also: this conflicts with your #4059 in BREAKING_CHANGES.md and CHANGELOG.md (both insert a top entry at the same lines); whichever lands second needs a trivial rebase. - On the removal itself: the alias shipped deprecated in 0.3.1b0 and 0.3.1b1 with a warning naming applies_transform and permits, which meets the one-cycle shim precedent in docs/acs-retarget.md, and the SDK is already on an unreleased breaking 0.4.0b0. The deprecation was never announced in CHANGELOG or BREAKING_CHANGES, though, so the maintainers will confirm the removal before this merges; nothing for you to do on that point.
| target; ``allow``, ``warn`` and ``escalate`` never do. | ||
| is moved to the canonical ``applies_transform`` predicate. An explicit | ||
| ``transformed_policy_target_applied`` flag preserves upstream support for |
There was a problem hiding this comment.
the docstring rewrite leaves a fragment with no subject: "target; allow, warn and escalate never do.\n is moved to the canonical applies_transform predicate. An explicit ...". Delete that line or restore the leading clause.
There was a problem hiding this comment.
Fixed: deleted the orphaned line. The paragraph now ends at "never do" and continues straight into the null-transform flag note; the gate itself is visible in the code two lines down.
|
This is a Python-only removal; I've added that to the body. The failure the PR fixes is a runtime one: the Python alias emitted On the #4059 overlap: both insert at the top of the same two sections, so I'll rebase whichever lands second. |
MohammadHaroonAbuomar
left a comment
MohammadHaroonAbuomar
left a comment
There was a problem hiding this comment.
- Commit 5edd95a ("docs(policy-engine): repair the _transformed_or docstring") has no Signed-off-by trailer and the Developer Certificate of Origin check fails on it; 1993d97 is signed. Amend with
-sand force-push, or squash it into 1993d97. The docstring fix and the Python-only scope statement in the body are both in; the removal itself still waits on the maintainers' confirmation, nothing further for you there.
2 participants
Refs #4048, item (c).
Decision.applies_effectshas been a deprecated alias ofapplies_transformsince the AGT D1 effects removal, and it emitted aDeprecationWarningon every access. Any consumer suite run with-W error(common in CI) fails on the first code path that still touches it, including third-party code the host does not control; emitting once per process would not change that. The issue offered "drop it" or "warn once"; this PR drops it.Removed the property and its deprecation test, reworded the two comments that referred to it, and recorded the removal in
BREAKING_CHANGES.md(date TBD, as the other entries) and the changelog.applies_transformandpermitsremain the two questions it conflated.python -W error -c "import agent_control_specification"is clean; the fullpytest sdk/pythonrun is unchanged apart from the removed test.This is a Python-only removal. The Node
appliesEffects(JSDoc@deprecated) and .NETAppliesEffects([Obsolete]) aliases are unchanged: neither warns at runtime, so neither can fail a host through code it does not control the way the PythonDeprecationWarningdoes under-W error. Dropping them for cross-SDK parity is a separate maintainer call.