refactor: filter pipeline DRY, procmail DSL consolidation, token substitution#137
Open
denzuko wants to merge 2 commits into
Open
refactor: filter pipeline DRY, procmail DSL consolidation, token substitution#137denzuko wants to merge 2 commits into
denzuko wants to merge 2 commits into
Conversation
…titution
Six fixes from a codebase-wide milter/plugin architecture audit.
## Fix 1 -- invoke-filter-chain space-split bug
Filter paths with arguments (e.g. '/path/neural.sh --model llama3')
were silently broken: the chain split on whitespace and probe-file
checked each token as a separate filter, dropping the arguments.
Fix: invoke-filter-chain now disambiguates via a probe-file heuristic.
A string where every whitespace token is an existing file path is
treated as a legacy space-separated multi-filter list (backward
compatible). Otherwise the string is one shell command with arguments,
passed verbatim to sh -c.
Also found and fixed a related bug in src/main.lisp: both the
pre-filter and post-filter call sites independently wrapped the
:pre-filter/:post-filter value in (list value) BEFORE calling
invoke-filter-chain, which made the value always satisfy listp and
bypassed the new string-disambiguation logic entirely. Removed the
redundant wrapping at both call sites -- invoke-filter-chain now
receives the raw configured value and handles all three cases
(nil/string/list) itself.
## Fix 2 -- procmail-gen.lisp hanging TODO
cmd-install-procmail and cmd-install-bugs-procmail each had ad-hoc
format-string recipe generation duplicating the procmail-gen.lisp DSL.
The file header self-documented this as 'not yet refactored.'
Added list-recipes/bugs-recipes builder functions to procmail-gen.lisp
returning :recipe plists. Both admin commands now build recipes via
these builders and write via the existing write-procmail-recipes.
Removed the old procmail-recipe/procmailrc-has-list-p ad-hoc functions
and ~50 lines of dead orphaned code left over from a prior partial
edit in this session. TODO comment removed from procmail-gen.lisp.
## Fix 3 -- header parsing duplication
requests.lisp's read-message-headers re-implemented colon-position
header parsing (no folded-header support) instead of using parser.lisp's
parse-headers (the canonical RFC 5322 parser already used elsewhere).
Now delegates to parse-headers. filters.lisp's string-to-message was
already rewritten this session to delegate to parse-headers as well.
bounce.lisp's DSN Final-Recipient:/X-Failed-Recipients: parsing and
bugs.lisp's pseudo-header (Severity:/Tags:/Owner:) extraction are
narrower, semantically distinct patterns -- not RFC 5322 header
parsing -- and correctly remain separate.
## Fix 4 -- mta.lisp misleading docstring
File contains audit logging, sendmail delivery, RFC 2369 headers, and
subscriber lifecycle handlers -- none of which are strictly 'MTA'
concerns. Renaming the file is too invasive for this PR; docstring
corrected to accurately describe contents instead.
## Fix 5 -- pipe-through-command / invoke-single-filter DRY
Both functions wrote stdin to a temp file, ran sh -c 'cmd < in > out',
read stdout, cleaned up -- ~40 lines duplicated almost verbatim.
Extracted with-temp-io-files macro (accepts &body) handling the temp
file lifecycle. Both functions now call shared run-shell-command,
which uses the macro. Security-boundary distinction (admin-generated
vs untrusted-email input) preserved in docstrings only.
## Fix 6 -- render-template extra-bindings TODO
(declare (ignore extra-bindings)) ; future: token substitution
troff.lisp's templates/*.sexp files hardcode list addresses/names per
file rather than using a substitution mechanism. Implemented:
substitute-tokens (str bindings) -- {{key}} -> value in a string
substitute-sexp-tokens (form bindings) -- recursive over DSL sexp tree
render-template now builds default bindings from list state
({{list-address}}, {{list-name}}, {{list-id}}), merges with caller
extra-bindings (caller bindings take precedence), and substitutes
before compiling to troff. Existing templates are unaffected (no
{{tokens}} present yet) -- this is purely additive.
## Verification
FiveAM: 78/78 (53+25)
BATS: 402/402 (400 baseline + FLT-7/FLT-8 new specs for the
argument-preservation fix)
All existing PM-1..25 (install-procmail), PMG-1..8 (procmail-gen
binary), BUG-1..33 (mlisp-bugs incl. BUG-29 procmail recipes),
FLT-1..6 (existing filter pipeline specs) pass unchanged against
the refactored code -- confirms backward compatibility.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Six fixes from a codebase-wide milter/plugin architecture audit, following the lesson learned in #126/#127 that filters are complete autonomous actors.
Fix 1 —
invoke-filter-chainspace-split bugFilter paths with arguments were silently broken — split on whitespace, arguments dropped. Fixed via a probe-file heuristic: if every whitespace token is an existing file, treat as a legacy multi-filter list (backward compatible); otherwise treat as one command with arguments preserved.
Also found and fixed a related bug in
main.lisp: both filter call sites independently pre-wrapped the configured value in(list value), bypassing the new disambiguation logic entirely. Removed the redundant wrapping.Fix 2 —
procmail-gen.lisphanging TODOcmd-install-procmail/cmd-install-bugs-procmailnow use the existing DSL (list-recipes/bugs-recipes+write-procmail-recipes) instead of ad-hoc format strings. Removed ~50 lines of dead orphaned code left from a prior partial edit.Fix 3 — header parsing duplication
read-message-headersnow delegates toparse-headers(the canonical RFC 5322 parser) instead of a colon-position reimplementation with no folded-header support.Fix 4 —
mta.lispdocstringCorrected to accurately describe contents (audit log, sendmail, RFC 2369 headers, subscriber lifecycle) rather than implying pure MTA concerns.
Fix 5 —
pipe-through-command/invoke-single-filterDRYExtracted
with-temp-io-filesmacro (accepts&body) for the shared temp-file lifecycle. Both functions now sharerun-shell-command.Fix 6 —
render-templatetoken substitutionImplemented
substitute-tokens/substitute-sexp-tokens. Templates can now use{{list-address}},{{list-name}},{{list-id}}instead of hardcoding per-list values.Verification: 480/480 (402 BATS + 78 FiveAM)
All existing specs (PM-1..25, PMG-1..8, BUG-1..33, FLT-1..6) pass unchanged against the refactored code. New FLT-7/FLT-8 cover the argument-preservation fix.