The problem is in build/mkupdates/listpromotable, starts around line 207, see the following 20 lines. Also related, bug 5545.

The following few lines of comments fully explain the logic.

    # all of these tflags force publication;
    # include "net", since otherwise this script has to be aware [...]

  # rule was from a file marked with "#testrules" (bug 5545)
  # note: this is after "tflags publish" support, so you can override
  # it on a rule-by-rule basis anyway

Of course, the rules autopromoted unintentionally are net rules.


Suggest the following change in the autopromotion / exclusion logic.

* ONLY tflags publish comes first and can actually override #testrules.
* Then chicken out, if #testrules has been set.
* Last, check for the other tflags as currently done early.

* Go on with the rest of the autopromotion magic...

NOTE: The only reason #testrules was used in this particular sandbox file was Bug #6527 where "tflags nopublish" not only excluded it from auto-promotion but also from nightly masscheck.

Created attachment 4858 [details]
proposed fix

This rather trivial patch moves tflags (userconf|learn|net) handling *after* checking for #testrules. Only an explicit tflag publish can override testrules.

Please note that the patch is entirely UNTESTED. Don't have an environment to run the code. Please review carefully. ;)

Didn't commit to trunk either, since I don't know which branch actually is used for autopromotion. Doe this need to be committed to both, 3.3 and trunk?

I looked into if this change will cause anything else unexpected.

http://svn.apache.org/repos/asf/spamassassin/trunk/rulesrc/sandbox/jm/20_bug_5984.cf

RCVD_IN_BRBL_LASTEXT was never meant to escape testing? =)

(In reply to comment #4)
> Didn't commit to trunk either, since I don't know which branch actually is used
> for autopromotion. Doe this need to be committed to both, 3.3 and trunk?

autopromotion runs off of trunk.  The code will update itself to the latest rev.  So just commit to trunk.  If it breaks its no big deal.  Rule updates are disabled.  Try to do it ASAP though just in case it breaks so that it can be fixed before the weekly mass-check is tagged.

May I propose that we insert a temporary "short-circuit" into the auto-update mechanism to allow for manual inspection prior to it being pushed live?

Let all but the DNS update happen then post the URL somewhere public so many eyes can download and examine diffs from the previous rules.

(In reply to comment #6)
> autopromotion runs off of trunk.  The code will update itself to the latest
> rev.  So just commit to trunk.  If it breaks its no big deal.  Rule updates are
> disabled.  Try to do it ASAP though just in case it breaks so that it can be
> fixed before the weekly mass-check is tagged.

Great, let me break it then. :)

Sending listpromotable
Committed revision 1085175.

  Bug 6560, specifically declared #testrules may only be overridden on a
  rule-by-rule basis by tflags publish, not tflags net.


Keeping this bug open, until the untested code at least has proven not to break the process.


(In reply to comment #7)
> May I propose that we insert a temporary "short-circuit" into the auto-update
> mechanism to allow for manual inspection prior to it being pushed live?

You may, though I suggest to do it somewhere else and not hijack this #testrules bug. ;)  The dev list seems appropriate for kicking around ideas.

Daryl provided a generated but un-released sa-update for testing this:

http://mail-archives.apache.org/mod_mbox/spamassassin-dev/201105.mbox/%3C4DD482AD.10304@dostech.ca%3E

Warren thought he tested it, but was apparently looking at the wrong tarball (said in IRC today).  Kevin also tested it, but presumably didn't specifically look for the SEM rules that were being a problem.  

(In case anybody else has been thinking "Wait, didn't we test this?")

(In reply to comment #9)
> Daryl provided a generated but un-released sa-update for testing this:
> 
> http://mail-archives.apache.org/mod_mbox/spamassassin-dev/201105.mbox/%3C4DD482AD.10304@dostech.ca%3E
> 
> Warren thought he tested it, but was apparently looking at the wrong tarball
> (said in IRC today).  Kevin also tested it, but presumably didn't specifically
> look for the SEM rules that were being a problem.  
> 
> (In case anybody else has been thinking "Wait, didn't we test this?")

As they were flagged as T_ which is what the cf file said to do, I wouldn't have flagged it as wrong.  A few DNS queries don't bug me and it won't swing ham/spam scores excep by almost negligible amounts.

(In reply to comment #10)
> 
> As they were flagged as T_ which is what the cf file said to do, I wouldn't
> have flagged it as wrong.  A few DNS queries don't bug me and it won't swing
> ham/spam scores excep by almost negligible amounts.

They are wrong as in we never intended for any T_ rule to become active?

(In reply to comment #11)
> (In reply to comment #10)
> > 
> > As they were flagged as T_ which is what the cf file said to do, I wouldn't
> > have flagged it as wrong.  A few DNS queries don't bug me and it won't swing
> > ham/spam scores excep by almost negligible amounts.
> 
> They are wrong as in we never intended for any T_ rule to become active?

#Testrules and nopublish are not the same thing.  I worry people are confusing the two OR the code doesn't implement what people thought it implemented. 

Considering this bug resolved and outstanding issues duplicate of bug 6527.