loki/docs/implementation/PHASE-1-STEP-16b-NOTES.md

Phase 1 Step 16b — Implementation Notes

Status

Done.

Plan reference

• Parent plan: docs/implementation/PHASE-1-STEP-16-NOTES.md
• Sub-phase goal: "Extract install_builtins() as top-level function"

Summary

Extracted Agent::install_builtin_agents() and Macro::install_macros() from inside Config::init into a new top-level free function config::install_builtins(). Called once from main.rs before any config-loading path.

Both functions are Config-independent — they just copy embedded agent/macro assets from the binary into the user's config directory. Extracting them clears the way for Config::init's eventual deletion in Step 16f.

What was changed

src/config/mod.rs

Added:

pub fn install_builtins() -> Result<()> {
    Agent::install_builtin_agents()?;
    Macro::install_macros()?;
    Ok(())
}

Placed after the Config::Default impl, before the impl Config block. Module-level pub fn (not a method on any type).

Removed from Config::init (inside the async setup closure):

• Agent::install_builtin_agents()?; (was at top of setup block)
• Macro::install_macros()?; (was at bottom of setup block)

src/main.rs

Added:

• install_builtins to the use crate::config::{...} import list
• install_builtins()?; call after setup_logger()? and before any of the three config-loading paths (oauth, vault flags, main config)

Placement rationale

The early-return paths (cli.completions, cli.tail_logs) legitimately don't need builtins — they return before touching any config. Those skip the install.

The three config paths (oauth via Config::init_bare, vault flags via Config::init_bare, main via Config::init) all benefit from builtins being installed once at startup. install_builtins() is idempotent — it checks file existence and skips if already present — so calling it unconditionally in the common path is safe.

Behavior parity

• install_builtin_agents and install_macros are static methods with no &self or Config arguments. Nothing observable changes about their execution.
• The two functions ran on every Config::init call before. Now they run once per main.rs invocation, which is equivalent for the REPL and CLI paths.
• Config::init_bare() no longer triggers the installs transitively. The oauth and vault-flag paths now rely on main.rs having called install_builtins()? first. This is a minor behavior shift — those paths previously installed builtins as a side effect of calling Config::init_bare. Since we now call install_builtins() unconditionally in main.rs before those paths, the observable behavior is identical.

Files modified

• src/config/mod.rs — added install_builtins() free function; removed 2 calls from Config::init.
• src/main.rs — added import; added install_builtins()? call after logger setup.

Assumptions made

1. install_builtins should always run unconditionally. Even if the user is only running --completions or --tail-logs (early-return paths), those return before the install call. The three config-using paths all benefit from it. No downside to running it early.

2. Module-level pub fn is the right API surface. Could have made it a method on AppState or AppConfig, but:

   • It's called before any config/state exists
   • It has no self parameter
   • It's a static side-effectful operation (filesystem) A free function at the module level is the honest signature.

3. No tests added. install_builtins is a thin wrapper around two side-effectful functions that write files. Testing would require filesystem mocking or temp dirs, which is disproportionate for a 3-line function. The underlying install_builtin_agents and install_macros functions have existing behavior in the codebase; the extraction doesn't change their contracts.

Open questions

1. Should install_builtins accept a "skip install" flag? Currently it always runs. For a server/REST API deployment, you might want to skip this to avoid writing to the user's config dir at startup. Deferring this question until REST API path exists — can add a flag or a _skip_install() variant later.

2. Do CI/test environments break because of the filesystem write? The install functions already existed in the codebase and ran on every Config::init. No new risk introduced. Watch for flaky tests after this change, but expected clean.

Verification

• cargo check — clean, zero warnings
• cargo clippy — clean, zero warnings
• cargo test — 122 passing, zero failures
• Grep confirmation:
  • install_builtin_agents only defined in src/config/agent.rs and called only via install_builtins
  • install_macros only defined in src/config/macros.rs and called only via install_builtins
  • install_builtins has one caller (main.rs)

Remaining work for Step 16

• 16c: Migrate Vault::init(&Config) → Vault::init(&AppConfig); eventually move vault ownership to AppState.
• 16d: Build AppState::init(app_config, ...).await.
• 16e: Switch main.rs and all 15 Config::init() callers to the new flow.
• 16f: Delete Config runtime fields, bridge.rs, Config::init, duplicated methods.

Migration direction preserved

After 16b, Config::init no longer handles builtin-asset installation. This is a small but meaningful piece of responsibility removal — when Config::init is eventually deleted in 16f, we don't need to worry about orphaning the install logic.

Startup flow now:

main()
    → install_builtins()?        [NEW: extracted from Config::init]
    → if oauth: Config::init_bare → oauth flow
    → if vault flags: Config::init_bare → vault handler
    → else: Config::init → to_app_config → AppState → ctx → run

The Config::init calls still do:

• load_envs
• set_wrap
• load_functions
• load_mcp_servers
• setup_model
• setup_document_loaders
• setup_user_agent

Those move to AppConfig::from_config (already built in 16a but unused) and AppState::init (16d) in later sub-phases.