Skip to content

fix(agent-lightning): report the applied penalty in multiplicative PolicyReward mode - #4418

Merged
MohammadHaroonAbuomar merged 4 commits into
microsoft:mainfrom
ShivamSharma43:fix/lightning-multiplicative-penalty-stats
Oct 26, 2026
Merged

MohammadHaroonAbuomar merged 4 commits into
microsoft:mainfrom
ShivamSharma43:fix/lightning-multiplicative-penalty-stats

Conversation

@ShivamSharma43

Copy link
Copy Markdown
Contributor

Related Issue

None. This is a bug I found in agent_lightning_gov.reward.PolicyReward; the sections below are filled in as the template asks.

Problem & Solution

Problem. PolicyReward.__call__ always computed the additive severity penalty, then picked the reward formula:

penalty = self._calculate_penalty(violations)
if self.config.multiplicative and violations:
    final_reward = base_reward * self.config.multiplicative_factor
else:
    final_reward = base_reward + penalty
...
self._total_penalties += penalty
self._emit_reward(final_reward, base_reward, penalty, violations)

In multiplicative mode the severity penalty is not used for the reward, but it is still what goes into get_stats() (total_penalties, avg_penalty) and into the policy_penalty value emitted to Agent-Lightning next to final and base:

reward = PolicyReward(kernel, config=RewardConfig(multiplicative=True))
reward(rollout_with_one_critical_violation)   # 0.5  (1.0 * 0.5)
reward.get_stats()["total_penalties"]         # -100.0, but the reward lost 0.5

So the emitted record says final=0.5, base=1.0, policy_penalty=-100, which do not add up, and a dashboard of the penalty shows the additive table rather than what training saw.

Solution. In multiplicative mode the penalty is now the amount the scaling took off the base reward (base * factor - base), taken before the clean bonus and the min/max bounds, as the additive penalty already is. Additive mode, and the reward itself in both modes, are unchanged.

Impact on Your Work

Anyone training with multiplicative=True and reading get_stats() or the emitted policy_penalty gets numbers that do not describe the rewards that were used.

Timeline

None.

Alternatives Considered

  • Report final - base. That would fold the clean bonus and the min/max clamp into the penalty, unlike additive mode, where policy_penalty is the severity sum before them.
  • Report 0 in multiplicative mode. The violations did lower the reward, so 0 would also be wrong.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)

Package(s) Affected

  • agent-lightning

Testing

Unit Testing

In agent-lightning/tests/test_lightning_comprehensive.py, TestPolicyRewardMultiplicative gains test_multiplicative_stats_record_applied_penalty (a critical violation at factor 0.5 records total_penalties == -0.5) and test_multiplicative_emits_applied_penalty (base 8.0 at factor 0.25 emits policy_penalty == -6.0); both fail on main. test_additive_stats_record_severity_penalty pins that additive mode still records the severity penalty.

Manual Testing

  • agent-lightning/tests: 206 passed.
  • ruff check --select E,F,W --ignore E501 (as in CI): clean on the changed source file; the test file's only findings are the four existing E731 lambdas in TestCompositeReward, unchanged on main.
  • Spell check: ran cspell with .cspell.json on the changed files; no issues on added lines.

Checklist

  • I have linked a related issue above, or completed "Problem & Solution", "Impact on Your Work", and "Alternatives Considered"
  • My code follows the project style guidelines (ruff check)
  • I have added tests that prove my fix/feature works
  • All new and existing tests pass (pytest)
  • I have updated documentation as needed (CHANGELOG)
  • I have signed the Microsoft CLA

Attribution & Prior Art

  • This contribution does not contain code copied or derived from other projects without attribution
  • Any external projects that inspired this design are credited in code comments or documentation
  • If this PR implements functionality similar to an existing open-source project, I have listed it below

None.

AI Assistance

  • I can explain every meaningful change in this PR: what it does, why, and what tradeoffs were considered
  • I have run tests and verification appropriate for this change
  • No part of this PR was autonomously submitted by an AI agent without my review
  • I have not used AI to generate review comments on others' PRs

I used Claude Code (Claude Opus) to help find the bug and draft the fix and tests. I reviewed the full diff and will handle review comments myself.

IP, Patents, and Licensing

  • This contribution does not implement patent-pending or patent-encumbered techniques
  • This contribution does not require an NDA or licensing agreement to understand or use
  • Any AI tools used have terms compatible with the MIT License

🤖 Generated with Claude Code

…licyReward mode

With RewardConfig(multiplicative=True) a violation scales the base
reward by multiplicative_factor, but get_stats() and the emitted
policy_penalty still used the additive severity table, so a critical
violation that halved a reward of 1.0 was recorded as -100. The penalty
is now the amount the scaling took off the base reward.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Shivam Sharma <192242476+ShivamSharma43@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 23, 2026 17:31

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.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@github-actions github-actions Bot added documentation Improvements or additions to documentation tests labels Oct 23, 2026
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Shivam Sharma <192242476+ShivamSharma43@users.noreply.github.com>
@github-actions github-actions Bot added the size/S Small PR (< 50 lines) label Oct 23, 2026
@github-actions

Copy link
Copy Markdown

PR Review Summary

Check Status Details
🔍 Code Review ⚠️ Missing No current-run comment
🛡️ Security Scan ⚠️ Missing No current-run comment
🔄 Breaking Changes ⚠️ Missing No current-run comment
📝 Docs Sync ⚠️ Missing No current-run comment
🧪 Test Coverage ⚠️ Missing No current-run comment

Verdict: ⚠️ AI review incomplete; ready for human review

AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims.

@github-actions

Copy link
Copy Markdown

🟡 Contributor Check: MEDIUM

Check Result
Profile MEDIUM
Credential LOW
Overall MEDIUM

Automated check by AGT Contributor Check.

@github-actions github-actions Bot added the needs-review:MEDIUM Contributor check flagged MEDIUM risk label Oct 23, 2026

@MohammadHaroonAbuomar MohammadHaroonAbuomar left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

  • docs/specs/AGENT-LIGHTNING-FAST-PATH-1.0.md:423 The fix is verified: in multiplicative mode the recorded and emitted penalty is now the applied scaling (two of the three new tests fail on main and pass here; 205 passed). One item: the normative spec still describes the old behaviour. Section 6.5 steps 3 to 5 say the total penalty comes from the severity mapping before the mode branch, and line 447 describes policy_penalty as the total penalty value. Please reword them so the penalty is the severity-table sum in additive mode and base_reward * multiplicative_factor - base_reward in multiplicative mode, taken before the clean bonus and the clamp, and say policy_penalty is the penalty applied in the active mode. The sibling #4419 updated the spec for its change, so this keeps the two in step. Approve follows.

…putation spec

Signed-off-by: Shivam Sharma <192242476+ShivamSharma43@users.noreply.github.com>
…icative-penalty-stats

Signed-off-by: Shivam Sharma <192242476+ShivamSharma43@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
@ShivamSharma43

Shivam Sharma (ShivamSharma43) commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor Author

Reworded the spec in 019cdb7. Section 6.5 steps 3–5 now say the penalty is base_reward * multiplicative_factor - base_reward in multiplicative mode and the severity-table sum in additive mode. They say it is taken before the clean bonus and the clamp, and that this value is the one recorded in the statistics and emitted. The 6.6 table now describes policy_penalty as the penalty applied in the active mode. I also merged main (CHANGELOG: main's lines first, then this PR's bullet). agent-lightning tests: 206 passed.

@github-actions github-actions Bot added size/M Medium PR (< 200 lines) and removed size/S Small PR (< 50 lines) labels Oct 24, 2026
@MohammadHaroonAbuomar
MohammadHaroonAbuomar dismissed their stale review October 26, 2026 14:26

Verified: in multiplicative mode the recorded and emitted penalty is the applied scaling; two of the three new tests fail on main and pass here; the spec section 6.5 and the policy_penalty description now match the code; 205 passed.

@MohammadHaroonAbuomar MohammadHaroonAbuomar left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Verified: in multiplicative mode the recorded and emitted penalty is the applied scaling; two of the three new tests fail on main and pass here; the spec section 6.5 and the policy_penalty description now match the code; 205 passed. CI is green on this head.

@MohammadHaroonAbuomar
MohammadHaroonAbuomar merged commit 6dc2093 into microsoft:main Oct 26, 2026
135 checks passed
San-Hsien (SanHsien) added a commit to SanHsien/agent-governance-toolkit that referenced this pull request Oct 26, 2026
…oft#4426, advance watermarks

- 6dc2093 (microsoft#4418): code and tests already in the fork; sync the reward
  computation spec so it matches the applied-penalty behaviour
- 46b370e (microsoft#4259 --sync-registry): not adopted; generate_workflows.py
  --check is already red in the fork (hand-added repository gates) and the
  new symlink tests raise WinError 1314 on unprivileged Windows
- PR microsoft#4427 (Parmana verifier integration): not adopted; head
  bc596b0 scanned for hidden content, clean
- issue microsoft#4426 (ApprovalResolution signature RFC): WATCH
- baseline: commit 46b370e, PR 4427, issue 4426

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation needs-review:MEDIUM Contributor check flagged MEDIUM risk size/M Medium PR (< 200 lines) tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants