Repository navigation
fix(plugins): the role rules hold at every door, not one each - #633
Conversation
Two consequences of one shape, both found on the merged branch: a rule
enforced at one door while another door reaches the same operation.
`clear_role` never consulted `REQUIRED_ROLES`. The RPC door did, and refused
to clear `llm` or `embedding`; the wizard reached the same function through
its own older answer, whose table says `optional: true` for embedding --
which answers "may be left unset", not "may be erased". Clicking Skip on a
configured embedding role called `set_embedding_endpoint({"model": "",
"provider": ""})` and emptied raven's own top-level block, the one endpoint
every knowledge base embeds with. The rule now lives with the operation:
`clear_role` raises `RoleRequiredError`, the RPC translates it rather than
restating it, and the wizard stops offering Skip for a role that is set and
required. A test that asserted the old behaviour is replaced -- it pinned
the defect as the contract.
`restart_for_config_change` never asked `everos_owned`. `_require_owned`
sits on the write primitives "so a new caller cannot opt out"; the restart
is a new caller and is not a write primitive, so it did. The embedding role
is exempt from the ownership gate by design, so a save of it on a
user-managed root fell through to `stop_for_reload` and SIGTERMed the
server that root belongs to -- against `everos_owned`'s own docstring
("never start or stop the process"), the file header, and C13. It now
refuses before it stops anything and says the root is theirs to restart.
Also, smaller, all named in the same review: the design document used
"role block", which the CONTEXT entry it ships beside puts on its `_Avoid_`
list; the vendor table spelled the rerank protocol `rerank_provider`, the
word that entry says to avoid, while EverOS's own `rerank.provider` field
and the `EVEROS_RERANK__PROVIDER` variable keep their names; and A18 stated
a criterion that is false about the code, since the protocol legitimately
reads `deepinfra` and `dashscope` where it belongs.
Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the full github/main...HEAD diff and the surrounding role configuration, onboarding, restart, RPC, and provider-table paths. Also checked the relevant history, callers, backward compatibility of the internal rerank_protocol rename, test-strength changes, and the repository ownership and domain-term constraints.
Verification:
uv run --all-extras pytest tests/test_everos_config.py tests/test_everos_server.py tests/test_cli_onboard_commands.py tests/test_rpc_settings.py -q: 589 passed.- Ruff check and format-check on all changed Python files: passed.
- The first test attempt without
--all-extrascould not collect because the fresh environment did not include theraven_everosdistribution; rerunning with the repository plugin extras produced the result above. - GitHub checks were green except two unit shards still pending when reviewed.
No inline findings.
…o the diff
Renaming the vendor table's key to `rerank_protocol` made seven lines of
`onboard.py` changed lines that had never been covered, and the diff gate
reads changed lines. They are worth covering on their own terms rather than
worth renaming around.
Four are the dispatch that decides what a rerank probe posts and how it
reads the reply: `deepinfra` to `{base}/{model}` reading `scores`,
`dashscope` to its own service path reading `output.results`, and anything
else to `{base}/rerank`. That is the fact the rerank half of this feature
exists for -- borrowing the chat address or the wrong shape is why
reranking configured from the settings page never reached DeepInfra -- and
it was observed on the wire during acceptance, so a unit case is its cheap
twin.
Three are the branch that asks the operator for that shape when the table
cannot answer: a self-hosted box is the case no table knows, and the
settings page has nowhere to ask, so the wizard is the only door.
Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the new test-only delta and rechecked the cumulative diff. The added cases meaningfully cover the three rerank wire shapes and the self-hosted protocol prompt, are additive rather than weakened replacements, and follow the repository CLI test-file rule. The delta does not change runtime compatibility or architecture; affected callers and the commit history remain consistent with the previously reviewed implementation.
Verification:
uv run --all-extras pytest tests/test_cli_onboard_commands.py tests/test_everos_config.py tests/test_everos_server.py tests/test_rpc_settings.py -q: 595 passed.- Ruff check and format-check for the changed test file: passed.
- GitHub lint, repository, page, installer, bridge, and trajectory checks were green; remaining test/UI jobs were still pending when reviewed.
No inline findings.
## Summary Follow-up to #614, which merged with a blocking finding open. Two consequences of one shape, both confirmed by reproducing them on the merged branch before anything was touched: a rule enforced at one door while another door reaches the same operation. **`clear_role` never consulted `REQUIRED_ROLES`.** The RPC door did, and refused to clear `llm` or `embedding`. The wizard reached the same function through its own older answer, whose table says `optional: true` for embedding -- which answers "may be left unset", not "may be erased". Clicking Skip on a configured embedding role called `set_embedding_endpoint({"model": "", "provider": ""})` and emptied raven's own top-level block, the one endpoint every knowledge base embeds with. Reproduced: ``` before {'model': 'text-embedding-3-small', 'provider': 'openrouter'} CONTROL clear_role(rerank) block unchanged SUBJECT clear_role(embedding) not refused; block = None log: "config/update: embedding endpoint cleared" ``` The rule now lives with the operation. `clear_role` raises `RoleRequiredError`; the RPC translates it instead of restating it, so there is one rule rather than two copies that drifted; the wizard stops offering Skip for a role that is set and required, while still offering it for one that is merely unset. A test that asserted the old behaviour is replaced -- it pinned the defect as the contract. **`restart_for_config_change` never asked `everos_owned`.** `_require_owned` sits on the write primitives "so a new caller cannot opt out"; the restart is a new caller and is not a write primitive, so it did. The embedding role is exempt from the ownership gate by design, so a save of it on a user-managed root fell through to `stop_for_reload` -> `stop_pid`, which SIGTERMs whatever `lock_holder` names for that root: the user's own server. Reproduced as far as the exemption, which is the part that can be driven without harming a running service: ``` owned=False llm rerank multimodal -> REFUSED (EverosRootNotOwnedError) embedding -> got past the ownership gate ``` That contradicts this feature's own written rule three times: `everos_owned`'s docstring ("never start or stop the process"), the file header, and C13 in the design document. The chain now refuses before it stops anything, and says the root is theirs to restart. Also, smaller, all named in the same review: the design document used "role block", which the CONTEXT entry it ships beside puts on its `_Avoid_` list; the vendor table spelled the rerank protocol `rerank_provider`, the word that entry says to avoid, while EverOS's own `rerank.provider` field and the `EVEROS_RERANK__PROVIDER` variable keep their names; and A18 stated a criterion that is false about the code, since the protocol legitimately reads `deepinfra` and `dashscope` where it belongs. ## Type - [x] Fix - [ ] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification - `uv run --frozen --python 3.12 --all-extras pytest tests/test_everos_config.py tests/test_everos_backend.py tests/test_everos_server.py tests/test_everos_discover.py tests/test_config_update_everos.py tests/test_config_update_providers.py tests/test_plugin_boundary.py tests/test_rpc_contract_shapes.py tests/test_rpc_settings.py tests/test_cli_onboard_commands.py -q` -- 1018 passed, 1 skipped - `ruff check raven/ tests/ plugins-dist/` -- all checks passed - `ruff format --check raven/ tests/ plugins-dist/` -- 1839 files already formatted - `make check-source-language` over this range -- clean Each fix was mutated back to the defect to confirm the new cases bite: - `clear_role` stops consulting `REQUIRED_ROLES` -- reddens the config case and the RPC case, which is what says the RPC now depends on the one rule rather than carrying a second copy - the wizard offers Skip for a required role again -- reddens the wizard case - the restart stops asking `everos_owned` -- reddens the unowned-root case Each mutation was reverted and the file hash compared afterwards. ## Risk User-visible: Skip disappears from the wizard menu for an embedding role that is already configured, and a save of the embedding role against a user-managed root now reports that the server was not restarted instead of restarting it. Both are the behaviour the feature's own documents already describe. Rollback: one commit, reverts cleanly. ## Related Issues Follow-up to #614. --------- Co-authored-by: gloryfromca <23442919+gloryfromca@users.noreply.github.com> Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
Summary
Follow-up to #614, which merged with a blocking finding open. Two consequences
of one shape, both confirmed by reproducing them on the merged branch before
anything was touched: a rule enforced at one door while another door reaches
the same operation.
clear_rolenever consultedREQUIRED_ROLES. The RPC door did, and refusedto clear
llmorembedding. The wizard reached the same function through itsown older answer, whose table says
optional: truefor embedding -- whichanswers "may be left unset", not "may be erased". Clicking Skip on a configured
embedding role called
set_embedding_endpoint({"model": "", "provider": ""})and emptied raven's own top-level block, the one endpoint every knowledge base
embeds with. Reproduced:
The rule now lives with the operation.
clear_roleraisesRoleRequiredError;the RPC translates it instead of restating it, so there is one rule rather than
two copies that drifted; the wizard stops offering Skip for a role that is set
and required, while still offering it for one that is merely unset. A test that
asserted the old behaviour is replaced -- it pinned the defect as the contract.
restart_for_config_changenever askedeveros_owned._require_ownedsits on the write primitives "so a new caller cannot opt out"; the restart is a
new caller and is not a write primitive, so it did. The embedding role is exempt
from the ownership gate by design, so a save of it on a user-managed root fell
through to
stop_for_reload->stop_pid, which SIGTERMs whateverlock_holdernames for that root: the user's own server. Reproduced as far as the exemption,
which is the part that can be driven without harming a running service:
That contradicts this feature's own written rule three times:
everos_owned'sdocstring ("never start or stop the process"), the file header, and C13 in the
design document. The chain now refuses before it stops anything, and says the
root is theirs to restart.
Also, smaller, all named in the same review: the design document used "role
block", which the CONTEXT entry it ships beside puts on its
_Avoid_list; thevendor table spelled the rerank protocol
rerank_provider, the word that entrysays to avoid, while EverOS's own
rerank.providerfield and theEVEROS_RERANK__PROVIDERvariable keep their names; and A18 stated a criterionthat is false about the code, since the protocol legitimately reads
deepinfraand
dashscopewhere it belongs.Type
Verification
uv run --frozen --python 3.12 --all-extras pytest tests/test_everos_config.py tests/test_everos_backend.py tests/test_everos_server.py tests/test_everos_discover.py tests/test_config_update_everos.py tests/test_config_update_providers.py tests/test_plugin_boundary.py tests/test_rpc_contract_shapes.py tests/test_rpc_settings.py tests/test_cli_onboard_commands.py -q-- 1018 passed, 1 skippedruff check raven/ tests/ plugins-dist/-- all checks passedruff format --check raven/ tests/ plugins-dist/-- 1839 files already formattedmake check-source-languageover this range -- cleanEach fix was mutated back to the defect to confirm the new cases bite:
clear_rolestops consultingREQUIRED_ROLES-- reddens the config case andthe RPC case, which is what says the RPC now depends on the one rule rather
than carrying a second copy
everos_owned-- reddens the unowned-root caseEach mutation was reverted and the file hash compared afterwards.
Risk
User-visible: Skip disappears from the wizard menu for an embedding role that is
already configured, and a save of the embedding role against a user-managed root
now reports that the server was not restarted instead of restarting it. Both are
the behaviour the feature's own documents already describe.
Rollback: one commit, reverts cleanly.
Related Issues
Follow-up to #614.