fix!: one authority for which hooks exist - #38
Conversation
Four places describe which hooks the engine dispatches and they disagree. Nothing consumes the enumerations, so the drift went unnoticed. The plan makes the metadata table the authority and co-declares each hook's constant with its row, so a constant without a row cannot be written. Also adds cmf.http_response, and makes config validation reachable from the two load paths that skip it today. Refs: praxis-proxy#37 Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
A hook's name constant and its metadata row now come from one `define_hooks!` declaration, so a constant without a row is unrepresentable. That gap is what let `cmf.http_request` ship unregistered: every phase consumer read it as Unphased, and no test could catch it because Rust has no reflection and any list a test walks is a second hand-maintained list. Completes the authority at thirteen hooks. `cmf.http_request` gains Pre / http, `elicit` gains Unphased, and `cmf.http_response` is new so the L7 path has a return half. Each HTTP constant now states what its own half carries: admission on the way in, response filtering on the way out. `BUILTIN_METADATA` is the concatenation of the per-module tables rather than a literal, and keeps its name and type. Every constant resolves at the path it did before. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
`lookup` returned a wildcard for a name the registry did not hold, so an absent hook and a deliberately unphased one both read as Unphased. A caller reading phase to decide something could not tell a missing entry from a real one, which is how a hook with no row still dispatched and looked intentional. It now returns `Option<HookMetadata>`, and `unknown()` becomes `permissive()`: with absence expressed by `None`, the wildcard is a default a caller opts into rather than the shape of a failed lookup. `dispatch_plan` asks for it explicitly, so its behavior is unchanged and the substitution is visible where it happens instead of hidden in the lookup. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
`builtin_hook_types()` restated the set by hand and had six of sixteen entries right: six named hooks nothing dispatches, two used a spelling identity and delegation do not answer to, and one dispatched hook was missing. Nothing called it, which is why the drift survived. A list nothing reads cannot fail. Both it and `hook_type_from_str` are now projections over the metadata table, so divergence is unrepresentable, and the count assertion reads the table's length instead of restating a number. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
`hook_names` and `cmf_hook_names` held sixteen public constants no dispatch site read. Six of `hook_names` shadowed CMF hooks under names nothing fires, and two spelled identity and delegation with underscores, which no handler answers to. `cmf_hook_names` duplicated `cmf::constants` and got the prompt pair wrong, teaching `cmf.prompt_pre_fetch` where the dispatched name is `cmf.prompt_pre_invoke`. Deleted rather than re-exported: the only reference in the tree was a doctest naming one of the removed constants, so a shim would serve no caller and would leave a second place a spelling could reappear. The doctest and the `PluginConfig` example now name hooks that dispatch, and the removed spellings are gone from fixtures and prose. `cmf/constants` and the three family constants keep their paths and their values. The plugin demo owned two hooks under the shadowed spellings. They become `demo.tool_pre_invoke` / `demo.tool_post_invoke`, declared with `define_hooks!` and registered at startup, which is what a host with its own hooks has to do. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
`load_config` and `from_config` take a pre-built `PolicyConfig` and ran neither the top-level `groups:` merge nor `validate_config`, both of which the YAML paths apply. A host assembling its config in Rust got no duplicate-plugin-name check, no route-shape check, and no group resolution, so a route joining a group came up without the plugins and `authentication:` that group was meant to supply and nothing said so. Both now normalize and validate through one helper. Idempotent, so `load_config_yaml` keeps normalizing early for its visitors and pays a map lookup to repeat it. A programmatic config that previously loaded with an inert piece now refuses, which is the point. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
`hooks:` carried free strings and nothing checked them, so a typo loaded clean and the plugin never fired for the rest of the deployment's life. The one thing worth validating in a plugin declaration was the one thing that was not. An unknown name now refuses the config, naming the plugin, the name, and the nearest name that does dispatch. `tool_pre_invoke` suggests `cmf.tool_pre_invoke`, which is what the removed constants and the old example taught; a name close to nothing gets no suggestion rather than the least-bad match in the table. The suggestion candidates are the derived enumeration, so a wrong projection produces a wrong suggestion a test catches. Validation reads the runtime registry rather than the built-in table, so a host's own hook passes once its metadata is registered. That pins the ordering: registration before the config that names it. The engine's own fixtures declare such a hook and now register it the same way. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
The entity-less HTTP path had a request half and no return half, so a `global.apl` with `result:` or `post_invocation:` steps compiled them and nothing ever ran them. Authorization genuinely has nothing to say once the upstream has answered, but response filtering does. Each half installs only when the policy declares steps for it, so a policy that only authorizes gains nothing. The pair and which half is Pre come from `hook_pair_for_entity`, the same mapping the entity routes read, so the phase a hook installs under is decided in one place. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Nothing checked that a hook's recorded phase matched the phase the dispatcher installs it under, which is how the HTTP request hook came to be installed as Pre while the table held no row for it and every phase consumer read it as unphased. The test walks the metadata table for entity types rather than naming hooks, so a new entity-typed hook joins it without an edit. Both directions are checked: each pair's halves carry the recorded phase, and no entity-typed hook sits outside its type's pair. A hook with no entity type has no install site to disagree with, so the unphased families are not a gap. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
The `hooks:` field is validated now, so its rustdoc says which names dispatch, where their constants live, and that a host registers its own hook metadata before loading config that names them. Registering afterwards is too late and the load refuses, which is the one ordering a reader can get wrong without any warning. `register_hook_metadata` states the same constraint from the other side, and the CHANGELOG records the breaking removals, the two load-path behavior changes, and that a host is free never to fire the new HTTP response hook. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
The install guard read as a filter over an Option where it means "both halves have to be wanted and the pair has to resolve"; a let-chain says that. Adds a test that no two modules declare the same hook name, the mirror of the gap the concatenated table already guards, and fixes two garbled doc sentences. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Review of the hook-registry work turned up ten findings; these are the seven worth acting on. The `global.response` warning said the response could never fire whenever the request half did not install, but the new post handler renders that same response, so a post-only policy got a warning contradicting what it does. Confirmed against a throwaway test before changing the guard. The suggestion threshold compared a character-counted edit distance against a byte length, giving a multi-byte name a looser bound than an ASCII one. Inactive-settings warnings were lost whenever validation rejected a config, because validation had moved ahead of them; an operator who sets a dead knob and has a validation error should see both. `hook_type_from_str` scanned the table for a match it then discarded and its doc claimed to return a canonical instance, which it never did: `HookType` owns its string and compares by value, so there was nothing to canonicalize. Reverted to the wrap it always was, with a doc that says so. The two enumeration drift tests compared a projection against the table it projects, which cannot fail. Replaced with assertions anchored on the names the old hand-maintained list got wrong, so the list this work removed would fail them. Two branches this work introduced had no coverage: the permissive fallback for a hook with no metadata row, and the entity-less HTTP path installing both halves at once, where a dropped request handler would be an authorization bypass. Both new tests were checked against a deliberately broken tree first. The notes overclaimed in three places. Adding an entity type to `cmf.http_request` narrows dispatch as well as correcting a phase, since a row-less hook matched every entity; a `hooks:` typo cost nothing for a plugin whose factory hardcodes its hook name, so "never fired" held only for the factories that read the field; and a repeated validation pass is a full walk, not the map lookup a comment claimed. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
`BUILTIN_METADATA` did not say metadata of what. `BUILTIN_HOOK_METADATA` matches the four per-module slices it concatenates, all of which already read `*_HOOK_METADATA`, and the `HOOK_TABLES` / `HOOK_COUNT` consts beside it. Crate-internal, so no public surface moves. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
praxis-bot
left a comment
There was a problem hiding this comment.
Review: fix!: one authority for which hooks exist
Well-structured fix. The define_hooks! macro makes the drift between name constants and metadata rows unrepresentable at compile time, which is the right solution for a class of bugs that tests cannot fully catch (any test walking the table is itself a second hand-maintained list). Config-time hook name validation, the Option return from lookup, and the normalization of every load path are all clean.
One test gap noted below.
| Severity | Count |
|---|---|
| Medium | 1 |
| assert!( | ||
| mgr.has_hooks_for(HOOK_CMF_HTTP_RESPONSE), | ||
| "a post-phase global policy must install the response handler", | ||
| ); |
There was a problem hiding this comment.
[Medium] The test verifies the response handler installs for a post-only config but does not assert the request handler is absent. Without the negative check, a regression that installs both handlers whenever either set of phases is declared would pass this test (the both_http_halves_install_and_evaluate_independently test checks both install, but nothing checks that a single-phase config installs only one).
Add a companion assertion:
assert!(
!mgr.has_hooks_for(HOOK_CMF_HTTP_REQUEST),
"a post-only global policy must not install the request handler",
);The symmetrical gap exists for the pre-only test above this PR's changes, but establishing the pattern here prevents it from widening.
OverallThe core idea is great. Specific things that hold up well:
Finding:
|
terylt
left a comment
There was a problem hiding this comment.
Some small change requests above.. nothing big.
Description
BUILTIN_HOOK_METADATAis now the single authority for the hooks the engine dispatches. It held ten of thirteen,builtin_hook_types()restated the set by hand with six of sixteen entries right, and two more constant modules named hooks nothing fires. Nothing consumed either enumeration, so the drift was never caught.define_hooks!emits a hook's name constant and its metadata row from one declaration, making a constant without a row unrepresentable. Addscmf.http_responseso the L7 path has a return half.Closes: #37
Breaking
hooks::types::hook_namesandcmf_hook_namesremoved — 16 constants with no dispatch site. Usecmf::constantsplusidentity::HOOK_IDENTITY_RESOLVE,delegation::HOOK_TOKEN_DELEGATE,elicitation::HOOK_ELICIT, whose paths and values are unchanged.hooks::lookupreturnsOption<HookMetadata>;unknown()is nowpermissive(). Migrate with.unwrap_or_else(HookMetadata::permissive).hooks:entry now refuses the config, andload_config/from_configvalidate where they previously did not. A host with its own hooks must callregister_hook_metadatabefore loading config naming them.cmf.http_requestgaining an entity type narrows dispatch as well as correcting its phase: a row-less hook matched every entity type.Full notes in
CHANGELOG.md.