proposal: add SecurityClass pipeline checks - #8
Conversation
72f4fd2 to
4ac4fdd
Compare
|
Proposal validation failed:
See proposal process for requirements. |
What? and Why? were agreed in the linked discussion. |
praxis-bot
left a comment
There was a problem hiding this comment.
PR Review
Summary: The proposal is well-structured and clearly motivates the need for SecurityClass-based pipeline checks instead of hardcoded name lists. The design approach of stamping is_security during pipeline build is sound. However, one critical edge case around registry lookup failure is not specified, and there is a minor inconsistency in the Non-goals section.
| Severity | Count |
|---|---|
| Large | 1 |
| Medium | 1 |
| `build_with_chains` already have the registry. | ||
| When constructing each `PipelineFilter`, set | ||
| `is_security` from | ||
| `registry.is_security_filter(&entry.filter_type)`. |
There was a problem hiding this comment.
[Large] Lines 154-161 describe stamping is_security during pipeline build by reading registry.is_security_filter(&entry.filter_type), but the proposal never specifies what happens if the filter type is not found in the registry.
This is a critical edge case because:
- Custom filters might be referenced in config before registration completes (race condition during startup)
- Config reload might reference a filter that was unregistered
- Malformed config might reference a non-existent filter type
Possible behaviors:
- Default to
false— treat unknown filters as non-security (could create security gaps) - Fail pipeline build — safer, but might break existing configs
- Log warning and default — middle ground but still requires a default choice
Specify the intended behavior explicitly, e.g., "If registry.is_security_filter returns None for a filter type, pipeline build fails with an error indicating the filter is not registered." or document that this cannot happen because config validation already ensures all referenced filters exist in the registry.
| - Adding new `SecurityClass` variants. | ||
| - Registering `ai_guardrails` as Security in | ||
| praxis-ai. That is a downstream follow-up after | ||
| this change is in a `praxis-filter` release. |
There was a problem hiding this comment.
[Medium] Line 140-142 states "Registering ai_guardrails as Security in praxis-ai" is a non-goal, but lines 231-234 in the Implementation PRs section explicitly describe a "Downstream (not this repo)" PR that registers ai_guardrails as SecurityClass::Security.
This is inconsistent: if downstream registration is covered in Implementation PRs, it's part of the coordinated delivery plan, not truly a non-goal. Either:
- Remove the non-goal bullet (since the proposal does coordinate the downstream change), or
- Move the Implementation PRs note about
ai_guardrailsto a separate "Downstream follow-up" section that clarifies it's outside this proposal's direct scope but enabled by it.
Signed-off-by: Liav Weiss <lweiss@redhat.com>
4ac4fdd to
245fdb4
Compare
|
We have a certain proposal filename naming issue here after we consolidated every proposal into a single repo; In the old process we should create an Epic in "this" repo to track this formal proposal and the how etc and use that Epic number as a filename prefix.. But I'm not sure it makes sense to run Epics out of the enhancements repo, especially not for a rather simple change like this. Then again, if it's cross repo concerns that's proposed it makes sense to have the Epic outside e.g. Core. We might want to adjust the process definition here some how. @shaneutt ? Strictly speaking I don't even think this PR is required by the process: Beyond that, the content of the PR looks good, I would close it(no bad feelings), write a short issue in Praxis for it.. and I'm looking forward to the impl. :) |
|
Thank you @aslakknutsen. |
Summary
SecurityClassinstead of the hardcodedSECURITY_FILTERSname list, so out-of-tree filters get the same fail-open, condition, and SkipTo checks as builtins.Tested
proposals/template.md(frontmatter, What / Why / How).Context