Convert policy module-level state (D, #181) to an explicit run(bf, policy) argument

Filed as the concrete follow-through for #182, since an "interim, convert later" decision only means something if there's a real trigger rather than a comment that fades into a closed thread.

Background

#182 asked how a loaded Policy reaches a check, given run(bf: Session)'s signature is fixed by all four (docs/design/pipeline-feature-shapes.md). #181 answered it with module-level state (analyse(policy=...) installs it, a check reads it via policy.active_policy()), argued explicitly as an interim rather than the permanent answer -- see #182's thread for the full reasoning and costs.

The trigger

Convert to passing policy as an explicit argument (run(bf, policy), the option run(bf)'s own contract would need all four to ratify) when a second check starts reading the policy. Right now only policy_compliance does. The moment access_control or routing also needs it, module-level state stops being "one check reads one hidden global" and becomes "multiple checks implicitly share state nobody's signature admits to" -- exactly the cost #182's thread named, just multiplied.

Why this is a small change when it happens

Checked while reviewing #181 (see #182's thread): every read of the policy in policy_compliance.py goes through one function, rules_in_use(), called once. Converting means giving it a policy parameter instead of a global lookup, and widening run(bf) to run(bf, policy=None) to pass it through. Contained, not a rewrite -- but it's still a shared-contract change under docs/design/pipeline-feature-shapes.md, so it needs the same kind of team sign-off #159 got, not a solo PR.

Acceptance

whichever of access_control / routing is about to read a policy second is the moment this gets picked up, not before
run(bf) widens to run(bf, policy=None), ratified the way Decision request: the five policy-format questions — five letters, one sitting (#87) #159 ratified D1-D5
policy.active_policy() / set_active_policy() / clear_active_policy() retired once nothing calls them

Give each request its own active policy, per Samika's #182 finding

@ARSH871-bot

This is a decision request, not a discussion. Five questions, each with a recommended answer. Agreeing costs one line.

Reply with five letters. 1A 2A 3A 4A 5A means "I agree with all five."
Disagree with any and just say which and why - that's the useful reply, not silence.

The trigger hasn't just fired, it's overshot

#191 said: convert when a SECOND check starts reading policy.active_policy(). Measured just now, on main:

policy_compliance.py:415   active = policy.active_policy()         (always has)
access_control.py:163      active = policy_module.active_policy()  (#316)
routing.py:254              active = policy_module.active_policy()  (#319)

Three checks, not two. And change_impact.py (not a check, but a real caller) installs the same global via set_active_policy()/clear_active_policy() in analyse_change() so its own policy-coverage stage can read it too. Four places implicitly share one hidden global, exactly the cost #182's thread named, now multiplied by four instead of two.

Question 1: proceed now that the trigger has overshot?

1A (recommended). Yes - converting later only gets more expensive as more callers attach to the global.
1B. No, not yet - say why.

Question 2: what exactly changes

Widens docs/design/pipeline-feature-shapes.md's CHECKS contract:

before   run(bf) -> list[dict]
after    run(bf, policy=None) -> list[dict]

pipeline.run_check() passes policy through: CHECKS[name](bf, policy).
pipeline.analyse() stops calling set_active_policy/clear_active_policy and threads policy through run_check instead.
Each of the three checks takes policy as a parameter and hands it to its own reading helper (policy_compliance.rules_in_use(policy) and whatever access_control/routing call), instead of reading the global.
change_impact.analyse_change() already takes policy explicitly (added for US-31 — Check a proposed change against my whole policy #323) - it drops its own set_active_policy/clear_active_policy calls and passes policy straight into the same reading helpers the checks now use.
Once nothing calls active_policy() / set_active_policy() / clear_active_policy(), they're deleted from analysis/policy.py in the same PR, not left as dead code someone finds later.

2A (recommended). Ratify this shape, the same way #159 ratified the policy format's own D1-D5.
2B. A different shape - say what.

Question 3: who does the mechanical pass

It touches pipeline.py, all three checks, change_impact.py, analysis/policy.py and the shapes doc - five files, no new logic, same reasoning the issue itself gave for calling this contained rather than a rewrite.

3A (recommended). I do the whole pass as one PR, each owner reviews their own file's section (Arsh: access_control.py, Shubham: policy_compliance.py + change_impact.py, Samika: nothing here currently, flag if that's wrong).
3B. Split by owner, four PRs.

Question 4: timing

#405 and #406 both still touch change_impact.py's policy handling and are not yet merged. Starting this conversion now stacks a third source of conflict on the same file.

4A (recommended). Start after both #405 and #406 have merged, whichever order.
4B. Start now anyway - say why it's worth the extra conflict.

Question 5: retiring the old mechanism

5A (recommended). active_policy() / set_active_policy() / clear_active_policy() deleted in the same PR that removes their last caller - matching #191's own acceptance criteria.
5B. Keep them around for a transition period - say why.

@ARSH871-bot @shubhamkataria2005 @SamikaPerera

1A, 2A, 4A, 5A agreed. 3A agreed with one correction.

Question 3 says "Samika: nothing here currently". That isn't accurate: tests/test_web_policy_applied.py uses clear_active_policy() and active_policy() at lines 87, 89, 146 and 201. I had Claude Code re-check each line on main at dc93ccb. Arsh wrote that file on 25 August and I changed it for #242 on 31 August, so ownership is shared.

Lines 87, 89 and 146 are fixture teardown and a spy, so they port easily. Line 201 doesn't: it asserts the global is empty after a request, which is how it pins that analyse()'s finally block clears the policy. Under 5A there is no global left to assert about, so it needs rewriting, not renaming. What should replace it? The property worth keeping is that a policy from one request can't reach the next analysis. If passing it as a parameter makes that impossible, I'd rather delete the test with a note saying why than leave a weaker assertion.

Checked Samika's correction on Question 3 directly against tests/test_web_policy_applied.py on main at dc93ccb - he's right, exactly. The three global-policy functions are called at lines 87, 89 (clear_active_policy() in the fixture's setup and teardown), 146 (seen["active"] = policy.active_policy(), the spy inside the stubbed _analyse), and 201 (assert policy.active_policy() is None, the one that pins analyse()'s finally clearing the global). "Samika: nothing here currently" was wrong - fixing that for the record.

On his actual question - what replaces line 201 once there's no global to assert about:

Lines 87/89 just go. Nothing left to reset.
Line 146's spy stops reading a global and instead captures the policy value the now-explicit _analyse/check call actually received as a parameter. Lines 167 and 186, which assert on seen["active"], don't need to change - same dict key, different source.
Line 201 itself has no direct replacement, because under explicit-parameter passing the property it guards ("a policy can't leak into the next analysis") stops being something analyse() has to clean up after itself and becomes true by construction - there's no shared mutable state between calls, so there's nothing left to leak.

That points at Samika's second option, sharpened rather than just deleted-with-a-note: replace test_the_policy_is_cleared_again_after_the_request with a test that drives two sequential requests - one with a policy uploaded, then a fresh one without - and asserts the second request's captured policy is None. That's a stronger test of the real property than the old one: the old test only checked the global got reset, never that a second, different request actually got independent treatment. A bug where analyse() accidentally memoized or reused the previous call's policy object would pass the old test and fail this one.

Proposing this as the concrete answer under 5A, to be written when the conversion PR touches this file.

Navigation Menu

Repository navigation

Convert policy module-level state (D, #181) to an explicit run(bf, policy) argument #191

Background

The trigger

Why this is a small change when it happens

Acceptance

The trigger hasn't just fired, it's overshot

Question 1: proceed now that the trigger has overshot?

Question 2: what exactly changes

Question 3: who does the mechanical pass

Question 4: timing

Question 5: retiring the old mechanism

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Convert policy module-level state (D, #181) to an explicit run(bf, policy) argument #191

Description

Background

The trigger

Why this is a small change when it happens

Acceptance

Activity

patelankeet2 commented on Oct 22, 2026

The trigger hasn't just fired, it's overshot

Question 1: proceed now that the trigger has overshot?

Question 2: what exactly changes

Question 3: who does the mechanical pass

Question 4: timing

Question 5: retiring the old mechanism

SamikaPerera commented on Oct 27, 2026

patelankeet2 commented on Oct 27, 2026

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions