Responsibilities: opt-in score smoothing - #32
tydinjarman wants to merge 2 commits into
Conversation
Semantic assessment scores are produced independently per evaluation and can oscillate while the underlying work is stable. This adds an opt-in exponential moving average over the scores each responsibility thresholds on, so threshold decisions track genuine movement instead of noise. Each responsibility lazily owns its ExponentialSmoother (per-check-key state, never shared between responsibilities, never written back onto the shared ForemanResult): enabling it cannot mutate assessment results globally or couple one responsibility's decisions to another's. score_smoothing_alpha defaults to 1.0 (FOREMAN_SCORE_SMOOTHING_ALPHA), which disables smoothing -- values pass through untouched and no state is recorded, preserving upstream behavior.
JARosen
left a comment
There was a problem hiding this comment.
The smoother is called inside short-circuiting boolean expressions. Consequently, some keys are not sampled on every assessment: if ready_to_finish fails, the requirements, tests, and verification smoothers may retain values from an older iteration. A later decision can then combine EMAs representing different assessment histories.
Please read and smooth every relevant score exactly once per evaluation before applying threshold logic. I would also like explicit safety semantics for smoothing needs_human and work_off_track, since damping a newly high safety signal is materially different from smoothing completion noise.
…s never damped Sample-once: every directives() now reads and smooths each thresholded score exactly once into a local at the top of the evaluation, before any state gates or boolean logic - no smoother calls inside short-circuiting expressions. Previously CompletionResponsibility's chained `and` skipped smoothing requirements_satisfied/tests_sufficient/needs_verification whenever ready_to_finish failed (and VerificationResponsibility skipped implementation_complete when needs_verification failed), so later decisions could combine EMAs built from different assessment histories. All threshold logic now operates purely on the sampled locals. Safety semantics: new ExponentialSmoother.smooth_safety() - fast-attack, slow-release. On a rising edge the raw value wins, so a newly high needs_human or work_off_track is never damped and escalation fires on the very assessment that reports it; falling edges still ease through the EMA to avoid flapping. HumanEscalation and WorkerHealth.work_off_track use it via _safety_probability(); completion-style scores keep the symmetric EMA. The distinction (damping a newly high safety signal is a safety regression; damping completion noise is the intended use) is documented in code comments and docs/routing.md. Tests: 188 passed (8 new) - smoother unit tests, a spy-count test proving all 4 completion keys are sampled exactly once per assessment across a failing-then-passing sequence, a numeric no-stale-mixing test (old code yields 0.9 where correct is 0.725), immediate-escalation tests for needs_human/work_off_track spikes (old code damped 0.9->0.5, below threshold), and a completion-noise smoothing test. Ruff clean.
tydinjarman
left a comment
There was a problem hiding this comment.
Addressed both review points (pushed as commit 83647dc).
Sample-once per assessment: every directives() now reads and smooths
each thresholded score exactly once into a local at the top of the
evaluation, before any state gates or boolean logic — no smoother calls
inside short-circuiting expressions. Previously,
CompletionResponsibility's chained and skipped smoothing
requirements_satisfied / tests_sufficient / needs_verification
whenever ready_to_finish failed (and VerificationResponsibility skipped
implementation_complete when needs_verification failed), so later
decisions could combine EMAs built from different assessment histories. All
threshold logic now operates purely on the sampled locals.
Safety semantics: new ExponentialSmoother.smooth_safety() —
fast-attack, slow-release. On a rising edge the raw value wins, so a newly
high needs_human or work_off_track is never damped: escalation fires on
the very assessment that reports it. Falling edges still ease through the
EMA to avoid flapping. HumanEscalation and
WorkerHealth.work_off_track use it via _safety_probability();
completion-style scores keep the symmetric EMA. The distinction — damping a
newly high safety signal is a safety regression, damping completion noise
is the intended use — is documented in code comments and docs/routing.md.
Test evidence: full suite 188 passed (8 new), including a spy-count test
proving all 4 completion keys are sampled exactly once per assessment
across a failing-then-passing sequence, a numeric no-stale-mixing test (old
code yields 0.9 where the correct EMA is 0.725), immediate-escalation tests
for needs_human/work_off_track spikes (the old code damped 0.9 → 0.5,
below threshold), and a completion-noise smoothing test. Ruff clean.
JARosen
left a comment
There was a problem hiding this comment.
The sample-once change and fast-attack handling for needs_human and work_off_track address the previous review well. A terminal-safety case remains around completion and verification.
With alpha=0.5, an initial assessment at readiness, requirements, and tests = 0.9 with needs_verification=0.1, followed by an assessment where all three positive signals fall to 0.6 and needs_verification rises to 0.9, still returns FINISH. The positive EMAs remain at 0.75 while needs_verification is damped to 0.5. Thus every current completion signal can regress and verification risk can rise sharply, yet the terminal action still fires.
Please make terminal smoothing conservative. Rising needs_verification should take effect immediately, and falling positive evidence such as tests_sufficient, requirements_satisfied, and ready_to_finish should not be hidden from a terminal decision. Reasonable designs include asymmetric slow-attack/fast-release smoothing for positive completion gates, or requiring both raw and smoothed values to satisfy terminal conditions. Please add the two-assessment sequence above as a regression test.
Supersedes part of #19, per your review: the "score smoothing" change, rebuilt from current
mainthrough the responsibility architecture — addressing your concern that smoothing "should not mutate assessment results globally or implicitly couple unrelated responsibilities" — with no helper scripts.What it does
src/foreman/responsibilities/smoothing.py:ExponentialSmoother, an EMA over one responsibility's noisy assessment scores (alphain(0, 1],smooth(key, value),reset())._smoothed_probability()on the shared responsibility base: per-check-key state, never shared between responsibilities, never written back onto the sharedForemanResult. The old PR'ssetattr(assessment, ...)global mutation is gone.score_smoothing_alpha(default1.0,FOREMAN_SCORE_SMOOTHING_ALPHA):alpha=1.0disables smoothing — values pass through untouched and no state is recorded, preserving upstream behavior exactly.Validation
tests/test_score_smoothing.py(alpha=1.0 passthrough, EMA behavior, per-responsibility isolation, noForemanResultmutation); full suite 180 passed,ruff checkclean.builtin.pycall sites across responsibilities, so if the sibling Policy: cold-start grace, sticky finish thresholds, optional EMA score smoothing #19 PRs merge first it will need a rebase — happy to rebase on request.