fix(memory-driven-chief-of-staff): make two setup refusals say something true - #146
Conversation
Both found by installing this recipe into a fresh profile on a NemoClaw host that already runs another Outlook recipe, and following the README as written. `setup-graph.sh` refuses when another provider already supplies `MS_GRAPH_ACCESS_TOKEN` to the same sandbox — correctly, because two providers cannot both supply one key and this recipe's endpoint policy is not the other one's. It then advised setting `GRAPH_PROVIDER_NAME` to a different name. That does nothing: the check reads the credential keys of every attached provider and never consults the variable. Run with and without it, the output is identical. The message now names the two things that do work, and says the part a user has to know — refusing here stops a second registration, not the reading. Inside the sandbox a credential names the sandbox and the key and not the provider, so the collector cannot tell which provider handed it over and will go on collecting mail through the other one. `install.sh` offers a real key or `ALLOW_NO_API_KEY=1`. Behind a gateway that substitutes the credential at the boundary, neither is right: a real key is a secret written into a second config file for no reason, and no key sends `no-key-required`, which the endpoint rejects — so the opt-out produces a profile that passes this check and fails to authenticate on every scheduled run. The working profile beside it held the gateway's rewrite marker. That marker is a public constant, so it is now named as a third option, and only when the profile these settings came from is already using exactly it. A real key never matches and is never printed, which has its own regression. Both are tested by running the scripts against a stubbed `openshell` and a stubbed `hermes`, because the earlier shape of this — reading the source for the message — is what let the advice stay wrong. Reverting the refusal turns two red; reverting the installer hint turns one red. The test that proves the old advice was wrong stays green either way by design: it compares the outcome with and without `GRAPH_PROVIDER_NAME`, which is the fact, not the wording. The README's test count is the only line touched there; its prose is being revised separately. 571 tests.
apurvvkumaria
left a comment
There was a problem hiding this comment.
Blocking documentation gap: this pull request changes credential onboarding and documents an important runtime limitation only in script error output, while the pull request description explicitly defers the README work. CONTRIBUTING.md requires setup, credential, permission, and security-boundary documentation to change with the behavior.
Please update the README and docs/set-up-graph.md in this pull request to cover both cases before a user runs the affected commands:
- For a gateway-backed inference endpoint, explain when the public rewrite marker is the correct profile credential and how to configure it, alongside the existing real-key and no-key paths.
- If the sandbox already has another provider exposing
MS_GRAPH_ACCESS_TOKEN, explain thatsetup-graph.shrefuses to register this recipe’s provider but cannot stop the in-sandbox collector from reading the existing credential. State that the existing provider may have a different egress policy, and give the supported choices: detach it or use a separate sandbox.
The implementation, 97 relevant tests, repository checks, DCO, and SPDX are otherwise clean at c9fc2782d1eeadda954fb5b8b28d4117f81900a7.
|
The documentation you are asking for is written — it is #147, opened alongside this one, which rewrites the recipe README and I split them because the two changes want different review: this one is a behaviour change with script-level regressions, and #147 is a thousand-line rewrite of the onboarding prose. Folding a paragraph into this pull request now would collide with that rewrite in the same lines, and the paragraph would be the weaker of the two. There is an ordering point worth naming, and it argues for landing this one first. #147 currently says "The current error text may suggest changing If you would rather have something in this pull request regardless, say so and I will add the minimum — the third credential option and the provider-conflict consequence — and rebase #147 over it. |
Related Issue
Related: #122
Description
Two messages that sent the user somewhere useless, both found by installing this recipe into a fresh profile on a NemoClaw host that already runs another Outlook recipe, and following the README as written.
setup-graph.shgave advice that provably does nothing. It refuses when another provider already suppliesMS_GRAPH_ACCESS_TOKENto the same sandbox — correctly, because two providers cannot both supply one key and this recipe's endpoint policy is not the other one's. It then advised settingGRAPH_PROVIDER_NAMEto a different name. The check reads the credential keys of every attached provider and never consults that variable; run with and without it, the output is identical. The message now names the two things that do work — detach the other provider, or attach this recipe to a sandbox that does not have it — and quotes the commands.It also now says the part a user has to know: the refusal does not bind the collector. Inside the sandbox a credential is a placeholder naming the sandbox and the key and nothing else, so
ingest_graph.pycannot tell which provider handed it over. On the host I tested, refusing to register stopped a second provider and not the reading: the first intake tick collected fifty real messages through the other recipe's provider, whose profile isaccess: read-writewhere this recipe declares read-only. Refusing at setup and continuing at runtime is worth saying out loud rather than leaving to be discovered.I did not try to make the collector enforce it, because on a sandboxed install it cannot. Setup runs on the host, where
openshellis; the collector runs in the sandbox, where the profile is;setup-graph.shwrites nothing into the profile and has nowhere to write it. A gate that cannot see what it is gating is worse than none, so this change makes the message true instead of pretending the boundary holds. The limitation belongs in the README, which is being revised separately.install.shoffered two answers, neither of which is right behind a gateway. A real key is a secret written into a second config file for no reason, and no key at all sendsno-key-required, which the endpoint rejects — soALLOW_NO_API_KEY=1produces a profile that passes this check and then fails to authenticate on every scheduled run. The working profile beside it held the gateway's substitution marker. That marker is a public constant, so it is now named as a third option, and only when the profile these settings came from is already using exactly it. A real key never matches the constant and is never printed, which has its own regression.Both changes are tested by running the scripts against a stubbed
openshell, a stubbedhermesand a stubbeduname, because the earlier shape of this — reading the source for the message — is what let the advice stay wrong in the first place. One test stays green either way by design: it compares the outcome with and withoutGRAPH_PROVIDER_NAME, which is the fact the message has to describe rather than the wording.Verification
python3 scripts/check_license_headers.py --check— 590 filespython3 scripts/check_label_taxonomy.py --check— not applicable, no governance metadata changedgit diff --checkOKhermes-direct-outlookattached,setup-graph.shrefuses identically with and withoutGRAPH_PROVIDER_NAMEset, and the intake tick afterwards collected 50 messages through that providerMS_GRAPH_ACCESS_TOKEN=openshell:resolve:env:v<sandbox>_MS_GRAPH_ACCESS_TOKENDocumentation Writer Review
no-doc-change-neededRelease And Compliance
.envfiles, private certificates, or token caches.THIRD-PARTY-NOTICES— none; standard library only.Signed-off-by: Xuan Wu xuwu@nvidia.com