Skip to content

fix(memory-driven-chief-of-staff): make two setup refusals say something true - #146

Open
rebelle5868 wants to merge 1 commit into
NVIDIA:mainfrom
rebelle5868:memory-driven-chief-of-staff-graph-credential
Open

fix(memory-driven-chief-of-staff): make two setup refusals say something true#146
rebelle5868 wants to merge 1 commit into
NVIDIA:mainfrom
rebelle5868:memory-driven-chief-of-staff-graph-credential

Conversation

@rebelle5868

Copy link
Copy Markdown
Contributor

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.sh gave advice that provably does nothing. It 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. 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.py cannot 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 is access: read-write where 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 openshell is; the collector runs in the sandbox, where the profile is; setup-graph.sh writes 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.sh offered 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 sends no-key-required, which the endpoint rejects — so ALLOW_NO_API_KEY=1 produces 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 stubbed hermes and a stubbed uname, 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 without GRAPH_PROVIDER_NAME, which is the fact the message has to describe rather than the wording.

Verification

  • python3 scripts/check_license_headers.py --check — 590 files
  • python3 scripts/check_label_taxonomy.py --check — not applicable, no governance metadata changed
  • git diff --check
  • Recipe suite: 610 tests across fourteen files, OK
  • Reproduced on a Linux NemoClaw host: with hermes-direct-outlook attached, setup-graph.sh refuses identically with and without GRAPH_PROVIDER_NAME set, and the intake tick afterwards collected 50 messages through that provider
  • Confirmed the sandbox placeholder carries no provenance: MS_GRAPH_ACCESS_TOKEN=openshell:resolve:env:v<sandbox>_MS_GRAPH_ACCESS_TOKEN
  • Confirmed on the same host that the profile which authenticates holds the gateway marker rather than a key
  • Each behaviour checked by reverting it — the refusal turns two red, the installer hint turns one red

Documentation Writer Review

  • Documentation writer review completed for the final changes
  • Result: no-doc-change-needed
  • Evidence or justification: the only README line touched is the test count, which the suite pins. The onboarding and egress-limitation prose this work surfaced is being revised in a separate change so the two do not collide.
  • Reviewer: @rebelle5868
  • Changed user-facing text follows the writing guide and controlled-word list.
  • A public contributor can understand the changed text without internal company context.
  • I reviewed any agent-generated text before submission.

Release And Compliance

  • No secrets or credentials are included, including API keys, access tokens, passwords, local .env files, private certificates, or token caches.
  • No nonpublic project names, environment names, hostnames, URLs, ticket identifiers, workspace paths, logs, screenshots, or configuration values are included.
  • Third-party dependency changes are reflected in THIRD-PARTY-NOTICES — none; standard library only.
  • Public content uses sanitized examples and placeholders instead of private values.
  • I added my DCO sign-off declaration to this pull request description.

Signed-off-by: Xuan Wu xuwu@nvidia.com

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 apurvvkumaria left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.
  2. If the sandbox already has another provider exposing MS_GRAPH_ACCESS_TOKEN, explain that setup-graph.sh refuses 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.

@rebelle5868

Copy link
Copy Markdown
Contributor Author

The documentation you are asking for is written — it is #147, opened alongside this one, which rewrites the recipe README and docs/set-up-graph.md. Both of your points are covered there: sk-OPENSHELL-PROXY-REWRITE is named as the profile credential for a gateway-backed endpoint with the config set command beside it, and the provider conflict has its own Runtime provenance limitation block stating that the effective boundary is the attached provider's policy rather than the YAML in the checkout, that ingest_graph.py trusts whichever provider supplies the key, that a foreign provider therefore bypasses the setup-time refusal, and that the supported choices are to detach it or use a dedicated sandbox.

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 GRAPH_PROVIDER_NAME, but renaming the provider does not change the scan of credentials already attached to the sandbox" — written against the message this pull request replaces. Once this lands, the error text no longer suggests that, so #147 can drop the workaround sentence entirely rather than carry a description of a defect that is gone. Merged the other way round, that sentence is wrong the moment this one follows, and needs a second fix.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants