Skip to content

systemd/web-search: user-bus fallback + brave key canonical path#34531

Closed
webdevtodayjason wants to merge 2 commits intoopenclaw:mainfrom
webdevtodayjason:codex/systemd-is-enabled-user-bus
Closed

systemd/web-search: user-bus fallback + brave key canonical path#34531
webdevtodayjason wants to merge 2 commits intoopenclaw:mainfrom
webdevtodayjason:codex/systemd-is-enabled-user-bus

Conversation

@webdevtodayjason
Copy link
Copy Markdown
Contributor

@webdevtodayjason webdevtodayjason commented Mar 4, 2026

Summary

  • treat systemctl --user is-enabled user-bus-unavailable failures as not enabled instead of hard-failing
  • canonicalize Brave web search key path to tools.web.search.brave.apiKey while keeping legacy tools.web.search.apiKey as a fallback alias
  • update configure/onboarding/runtime secret collection and credential-surface docs for the canonical Brave key path

Testing

  • pnpm test src/daemon/systemd.test.ts
  • pnpm test src/config/config.web-search-provider.test.ts src/secrets/runtime.test.ts src/secrets/target-registry.test.ts
  • pnpm format:check

Closes #34464
Closes #34507

@openclaw-barnacle openclaw-barnacle bot added docs Improvements or additions to documentation gateway Gateway runtime commands Command implementations agents Agent runtime and tooling size: M labels Mar 4, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 4, 2026

Greptile Summary

This PR addresses two separate bug fixes: (1) it prevents isSystemdServiceEnabled from hard-failing when the systemd user bus is unavailable (e.g., in containers or non-systemd environments) by treating those conditions as "not enabled", and (2) it canonicalizes the Brave Search API key path to tools.web.search.brave.apiKey while keeping tools.web.search.apiKey as a legacy fallback alias.

Key changes:

  • New isSystemdUserBusUnavailable helper in systemd.ts correctly identifies D-Bus connection failures and returns false instead of throwing.
  • resolveSearchApiKey in web-search.ts now prefers tools.web.search.brave.apiKey over the legacy tools.web.search.apiKey, with BRAVE_API_KEY env var as final fallback.
  • configure.wizard.ts migrates users from the legacy path on next key entry by writing the canonical path and stripping apiKey.
  • Secret resolution in runtime-config-collectors-core.ts correctly bridges both paths: if only the legacy key is configured, the resolved value is propagated to search.brave.apiKey as well.
  • Schema, type definitions, labels, help text, registry, docs, and credential surface docs are all consistently updated.

The changes are well-tested, additive, and backward-compatible. The legacy alias is preserved at every layer (schema, runtime resolution, configure wizard, audit, docs), so existing installs won't break.

Confidence Score: 4/5

  • Safe to merge with a minor style cleanup recommended for test naming clarity
  • The PR is well-tested, additive, and maintains backward compatibility. The two functional changes (systemd bus fallback and Brave key canonicalization) are well-scoped with comprehensive tests. The legacy alias is preserved at every layer (schema, runtime resolution, configure wizard, audit, docs), so existing installs won't break. A single minor style issue was identified: the test name at line 82 of systemd.test.ts contradicts its assertion and should be updated for clarity, but this does not affect correctness or safety.
  • src/daemon/systemd.test.ts — minor test name update needed

Last reviewed commit: d5d5f4f

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 4, 2026

Additional Comments (1)

src/daemon/systemd.test.ts, line 90
Stale test description after behavior change

This test was updated to assert that isSystemdServiceEnabled returns false when it encounters a "Failed to connect to bus" error, but the test name was not updated. The name "throws when systemctl is-enabled fails for non-state errors" now contradicts what the test actually asserts, which can mislead future developers.

  it("returns false when systemctl cannot connect to the user bus", async () => {
    const { isSystemdServiceEnabled } = await import("./systemd.js");
    execFileMock.mockImplementationOnce((_cmd, _args, _opts, cb) => {
      const err = new Error("Failed to connect to bus") as Error & { code?: number };
      err.code = 1;
      cb(err, "", "Failed to connect to bus");
    });
    await expect(isSystemdServiceEnabled({ env: {} })).resolves.toBe(false);
  });

chatgpt-codex-connector[bot]
chatgpt-codex-connector bot reviewed Mar 4, 2026
View reviewed changes
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d5d5f4f441

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

src/agents/tools/web-search.ts
Comment on lines 385 to 388
search && "apiKey" in search
? normalizeResolvedSecretInputString({
value: search.apiKey,
path: "tools.web.search.apiKey",
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot Mar 4, 2026

Choose a reason for hiding this comment

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

P1 Badge Skip legacy key resolution when canonical Brave key is present

resolveSearchApiKey eagerly normalizes search.apiKey even when search.brave.apiKey is already set, so a leftover legacy SecretRef can still throw assertSecretInputResolved and break Brave web search despite a valid canonical key. This can happen during migration when users keep an old tools.web.search.apiKey ref while setting tools.web.search.brave.apiKey; the canonical key should short-circuit the legacy alias instead of failing on it.

Useful? React with 👍 / 👎.

@vincentkoc vincentkoc mentioned this pull request Mar 4, 2026
Merged
18 tasks
@vincentkoc vincentkoc self-assigned this Mar 4, 2026
@vincentkoc
Copy link
Copy Markdown
Member

vincentkoc commented Mar 4, 2026

Thanks for the contribution here. The systemd user-bus/is-enabled fix path is now covered by merged #33634 and #34884, so this mixed-scope PR is superseded on the daemon side and I am closing it. If you still want to land the non-systemd portion, please open a focused follow-up PR and reference this thread.

@vincentkoc vincentkoc closed this Mar 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chatgpt-codex-connector chatgpt-codex-connector[bot] chatgpt-codex-connector[bot] left review comments

Assignees

@vincentkoc vincentkoc

Labels

agents Agent runtime and tooling commands Command implementations docs Improvements or additions to documentation gateway Gateway runtime size: M

Projects

None yet

Milestone

No milestone

2 participants

@webdevtodayjason @vincentkoc