feat: chat onboarding and routine advisor#927
feat: chat onboarding and routine advisor#927ilblackdragon wants to merge 35 commits intostagingfrom
Conversation
Port the complete psychographic profiling system from NPA into IronClaw, including enriched profile schema, conversational onboarding, profile evolution, and three-tier prompt augmentation. Personal onboarding moved from wizard Step 9 to first assistant interaction per maintainer feedback — the First Contact system prompt block now instructs the LLM to conduct a natural onboarding conversation that builds the psychographic profile via memory_write. Changes: - Enrich profile.rs with 5 new structs, 9-dimension analysis framework, custom deserializers for backward compatibility, and rendering methods - Add conversational onboarding engine with one-step-removed questioning technique, personality framework, and confidence-scored profile generation - Add profile evolution with confidence gating, analysis metadata tracking, and weekly update routine - Replace thin interaction style injection with three-tier system gated on confidence > 0.6 and profile recency - Replace wizard Step 9 with First Contact system prompt block that drives conversational onboarding during the user's first interaction - Add autonomy progression to SOUL.md seed and personality framework to AGENTS.md seed Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…pace seeds Remove the interactive onboarding_chat.rs engine in favor of a simpler bootstrap flow: fresh workspaces get a proactive LLM greeting that naturally profiles the user. Identity files are now seeded from src/workspace/seeds/ instead of being hardcoded. Also removes the identity-file write protection (seeds are now managed), adds routine advisor integration, and includes an e2e trace for bootstrap greeting. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
bot
added
scope: agent
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the agent's initial user experience and introduces advanced personalization capabilities. It moves away from a rigid interactive onboarding flow to a more natural, conversational bootstrap greeting for new users, leveraging a new psychographic profiling system to understand and adapt to user preferences. Furthermore, it integrates routine management improvements, allowing for event-driven automation and continuous profile evolution, while simplifying memory write operations by centralizing seed file management. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant feature update, replacing the previous onboarding system with a dynamic, conversational bootstrap flow and introducing a comprehensive psychographic profiling system. While these features enhance user experience, they introduce significant prompt injection risks. The most critical issue is the removal of write protection for core identity files in the memory_write tool, which allows the agent to overwrite its own instructions. Additionally, untrusted user data is injected into prompts during profile evolution and personalization without sufficient sanitization, creating paths for both direct and indirect prompt injection. On the development side, the changes are well-structured, with notable improvements like moving hardcoded workspace seed files into a dedicated src/workspace/seeds/ directory, enhancing maintainability. A minor suggestion is to improve the consistency of cron expression examples in the new routine-advisor skill documentation to align with the updated tool documentation.
I am having trouble creating individual review comments. Click here to see my feedback.
src/tools/builtin/memory.rs (23-27)
The removal of PROTECTED_IDENTITY_FILES write protection is a critical security regression. These files (IDENTITY.md, SOUL.md, AGENTS.md, USER.md) are injected into the system prompt to define the agent's core behavior and boundaries. Allowing the LLM to overwrite them enables persistent prompt injection attacks where an attacker can permanently alter the agent's instructions, bypass safety guardrails, and gain full control over the agent in future sessions. Please restore the write protection for these files.
src/setup/profile_evolution.rs (28-29)
The recent_messages_summary is directly concatenated into the LLM prompt without sanitization or delimiters. Since this summary contains content from user messages, it can be used for prompt injection to manipulate the profile evolution process. An attacker could trick the LLM into generating a malicious psychographic profile. Recommend wrapping the summary in unique delimiters and instructing the LLM to treat it as untrusted data.
References
- Sanitization should be applied to data paths sent to external services, such as an LLM, to prevent prompt injection. User messages concatenated into LLM prompts without sanitization or delimiters can be used for prompt injection.
src/workspace/mod.rs (706-782)
Profile fields such as tone, goals, and pain_points are rendered directly into the system prompt. These fields are populated by the LLM and can be influenced by user input. If an attacker poisons these fields (e.g., via the evolution prompt), malicious instructions will be persistently injected into the system prompt of every session. Recommend implementing strict validation and sanitization for all profile fields before they are used in prompt construction.
References
- Sanitization should be applied to data paths sent to external services, such as an LLM, to prevent prompt injection. Profile fields influenced by user input and populated by an LLM should be validated and sanitized before use in prompt construction to prevent persistent prompt injection.
skills/routine-advisor/SKILL.md (81-85)
For consistency with the routine_create tool's updated documentation, which now recommends the standard 5-field cron format (min hour day month weekday), it would be better to update these examples to use 5 fields instead of 6. While the new normalize_cron_expression function handles both, aligning the skill documentation with the tool documentation will improve clarity and predictability for the LLM.
- Daily 9am: `0 9 * * *`
- Weekday mornings: `0 9 * * MON-FRI`
- Weekly Monday: `0 9 * * MON`
- Every 2 hours during work: `0 9-17/2 * * MON-FRI`
- Sunday evening: `0 18 * * SUN`
There was a problem hiding this comment.
Pull request overview
Implements a new first-run experience driven by a proactive “bootstrap greeting” and introduces a psychographic profile pipeline that syncs derived workspace documents and supports routine advising/evolution.
Changes:
- Replace interactive onboarding with BOOTSTRAP.md-based prompt injection + proactive greeting on fresh workspaces.
- Seed workspace identity/memory templates from
src/workspace/seeds/*viainclude_str!instead of hardcoded strings. - Add profile schema/types, profile-to-doc sync (
USER.md, assistant directives, heartbeat), routine-advisor skill, and cron normalization (5/6/7-field support).
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/support/test_rig.rs | Adds bootstrap-aware test rig behavior (keep/clear bootstrap flag; gateway naming). |
| tests/support/test_channel.rs | Adds configurable channel name to support gateway-only bootstrap behavior in tests. |
| tests/fixtures/llm_traces/advanced/bootstrap_greeting.json | Adds an E2E LLM trace fixture for the proactive bootstrap greeting flow. |
| tests/e2e_advanced_traces.rs | Adds an E2E test asserting bootstrap greeting + follow-up interaction. |
| src/workspace/seeds/USER.md | New seed template for user context. |
| src/workspace/seeds/TOOLS.md | New seed template for environment/tool notes. |
| src/workspace/seeds/SOUL.md | New seed template for agent values/boundaries. |
| src/workspace/seeds/README.md | New seed template describing workspace structure. |
| src/workspace/seeds/MEMORY.md | New seed template for long-term memory doc. |
| src/workspace/seeds/IDENTITY.md | New seed template for agent identity doc. |
| src/workspace/seeds/HEARTBEAT.md | New seed template for heartbeat checklist. |
| src/workspace/seeds/BOOTSTRAP.md | New bootstrap instructions used for first-run prompt injection. |
| src/workspace/seeds/AGENTS.md | New seed template for session instructions and profile building guidance. |
| src/workspace/mod.rs | Switches seeds to include_str!, adds bootstrap flags, profile prompt injection tiers, and profile-derived doc sync helpers. |
| src/workspace/document.rs | Adds canonical paths for context/profile.json and assistant directives doc. |
| src/tools/builtin/routine.rs | Normalizes cron expressions before validation/storage; updates schema description to accept 5/6/7-field cron. |
| src/tools/builtin/memory.rs | Removes identity-file write guards; adds profile-write-triggered document sync + onboarding completion persistence. |
| src/setup/wizard.rs | Adds note that personal onboarding is now conversational (bootstrap-driven), not wizard-driven. |
| src/setup/profile_evolution.rs | Adds weekly profile evolution prompt generator + routine prompt template and tests. |
| src/setup/mod.rs | Exposes profile_evolution module and re-exports SetupError. |
| src/setup/README.md | Documents the new personal onboarding flow via bootstrap injection. |
| src/settings.rs | Adds profile_onboarding_completed flag for conversational onboarding completion. |
| src/profile.rs | Introduces psychographic profile types/schema/framework and rendering helpers (USER.md, assistant directives, heartbeat). |
| src/lib.rs | Exposes new profile module. |
| src/app.rs | Loads onboarding-complete flag from settings to suppress repeat bootstrap injection. |
| src/agent/routine.rs | Adds cron normalization helper and expands next_cron_fire to accept 5/6/7-field cron; adds tests. |
| src/agent/agent_loop.rs | Sends a proactive bootstrap greeting at startup when the workspace is freshly seeded; adjusts logging verbosity. |
| skills/routine-advisor/SKILL.md | Adds a routine-advisor skill for suggesting routines based on conversation patterns. |
| skills/delegation/SKILL.md | Adds a delegation skill for task breakdown/tracking workflows. |
| CLAUDE.md | Documents the new src/profile.rs module in the repository layout. |
Comments suppressed due to low confidence (1)
src/tools/builtin/memory.rs:259
memory_writenow allows arbitrary paths without blocking writes to prompt-injected instruction/identity files (e.g. AGENTS.md/SOUL.md/IDENTITY.md/USER.md). This reintroduces a prompt-injection persistence vector (attacker can poison future system prompts). Reintroduce a denylist/allowlist (or require explicit approval) for these files even if they are seeded/system-managed.
path => {
if append {
self.workspace
.append(path, content)
.await
.map_err(|e| ToolError::ExecutionFailed(format!("Write failed: {}", e)))?;
} else {
self.workspace
.write(path, content)
.await
.map_err(|e| ToolError::ExecutionFailed(format!("Write failed: {}", e)))?;
}
path.to_string()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 7. Create TestChannel and ChannelManager. | ||
| let test_channel = Arc::new(TestChannel::new()); | ||
| // When testing bootstrap, the channel must be named "gateway" because | ||
| // the bootstrap greeting targets only the gateway channel. | ||
| let test_channel = if self.keep_bootstrap { | ||
| Arc::new(TestChannel::new().with_name("gateway")) | ||
| } else { | ||
| Arc::new(TestChannel::new()) | ||
| }; |
There was a problem hiding this comment.
build(self) destructures self into locals (including keep_bootstrap), but later the code still references self.keep_bootstrap when choosing the TestChannel name. This won’t compile because self has been moved. Use the local keep_bootstrap variable (or avoid moving self before this point).
There was a problem hiding this comment.
Will check — this may be a non-issue if the destructure happens after the reference, but worth verifying the borrow/move ordering in build().
There was a problem hiding this comment.
Verified — no issue. self is destructured into locals at the top of build(), and by line 659 only those locals are used. keep_bootstrap is a bool (Copy), so there's no borrow/move conflict. The deps and channels values are consumed by Agent::new() before agent is moved into tokio::spawn, and test_channel remains valid as a separate Arc. No code change needed.
| let Ok(parsed) = chrono::DateTime::parse_from_rfc3339(updated_at) else { | ||
| return false; | ||
| }; | ||
| let age = Utc::now().signed_duration_since(parsed); | ||
| // Future timestamps are not "recent" (clock skew / bad data). | ||
| if age.num_seconds() < 0 { | ||
| return false; | ||
| } | ||
| age.num_days() <= max_days |
There was a problem hiding this comment.
is_profile_recent subtracts a DateTime<FixedOffset> (from parse_from_rfc3339) from Utc::now(), which is a different timezone type and won’t compile. Convert parsed to UTC first (e.g., parsed.with_timezone(&Utc)) before computing the age.
There was a problem hiding this comment.
This is actually fine — signed_duration_since accepts any DateTime<Tz> since it's generic over the timezone parameter. DateTime<FixedOffset> works directly without conversion to UTC.
src/agent/agent_loop.rs
Outdated
| let bootstrap_msg = IncomingMessage::new( | ||
| "system", | ||
| "default", | ||
| "Hello! I just set you up. Introduce yourself and help me get started.", | ||
| ); |
There was a problem hiding this comment.
Bootstrap greeting uses IncomingMessage::new("system", ...), but thread persistence (persist_user_message/persist_assistant_response) hard-codes the conversation channel as "gateway", so this will create/persist a second “gateway” conversation under a thread_id resolved for the system channel. To avoid duplicate/ghost threads, create the bootstrap message on the gateway channel and attach the resolved assistant_thread_id as thread_id so session resolution + persistence align.
| let bootstrap_msg = IncomingMessage::new( | |
| "system", | |
| "default", | |
| "Hello! I just set you up. Introduce yourself and help me get started.", | |
| ); | |
| let mut bootstrap_msg = IncomingMessage::new( | |
| "gateway", | |
| "default", | |
| "Hello! I just set you up. Introduce yourself and help me get started.", | |
| ); | |
| if let Some(tid) = &assistant_thread_id { | |
| bootstrap_msg.thread_id = Some(tid.to_string()); | |
| } |
There was a problem hiding this comment.
Valid point about channel alignment. The bootstrap message uses "system" intentionally to distinguish it from real user input in the conversation history, but the persistence path does hardcode "gateway". Will look into aligning the channel or using thread_id attachment as suggested.
There was a problem hiding this comment.
Fixed in 71c0aeb — bootstrap message now uses ("default", "gateway") consistently with the persist and session registration calls. The "system" user was causing the thread ownership mismatch that produced the Rejected write for unavailable thread warning.
There was a problem hiding this comment.
Fixed in f14d136 — removed the duplicate event delegation block and duplicate event listener registrations. Only one delegated data-action click handler remains.
| // Persist the onboarding-completed flag and set the | ||
| // in-memory safety net so BOOTSTRAP.md injection stops | ||
| // even if the LLM forgets to delete it. | ||
| self.workspace.mark_bootstrap_completed(); | ||
| let toml_path = crate::settings::Settings::default_toml_path(); | ||
| if let Ok(Some(mut settings)) = crate::settings::Settings::load_toml(&toml_path) | ||
| && !settings.profile_onboarding_completed | ||
| { | ||
| settings.profile_onboarding_completed = true; | ||
| if let Err(e) = settings.save_toml(&toml_path) { | ||
| tracing::warn!("failed to persist profile_onboarding_completed: {e}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Writing context/profile.json triggers a tool-side write to the user’s global config.toml (Settings::default_toml_path()). Tool execution should ideally be confined to workspace/db state; persisting host config from an LLM tool call can cause unexpected side effects (and test/CI pollution). Consider moving this persistence to an app-layer component (or store the flag in the DB settings table) instead of writing TOML from the tool.
| // Persist the onboarding-completed flag and set the | |
| // in-memory safety net so BOOTSTRAP.md injection stops | |
| // even if the LLM forgets to delete it. | |
| self.workspace.mark_bootstrap_completed(); | |
| let toml_path = crate::settings::Settings::default_toml_path(); | |
| if let Ok(Some(mut settings)) = crate::settings::Settings::load_toml(&toml_path) | |
| && !settings.profile_onboarding_completed | |
| { | |
| settings.profile_onboarding_completed = true; | |
| if let Err(e) = settings.save_toml(&toml_path) { | |
| tracing::warn!("failed to persist profile_onboarding_completed: {e}"); | |
| } | |
| } | |
| // Mark onboarding as completed in workspace state so | |
| // BOOTSTRAP.md injection stops even if the LLM forgets | |
| // to delete it. Persisting any global config (e.g. | |
| // config.toml) should be handled at the application | |
| // layer, not from within a tool. | |
| self.workspace.mark_bootstrap_completed(); |
There was a problem hiding this comment.
Acknowledged — the config.toml write is a pragmatic choice for now since the settings file is the only durable store available outside the workspace DB. Moving this to an app-layer hook or DB-backed settings table is a reasonable follow-up, but it doesn't block this PR. The write is guarded behind a condition check (only fires once when the flag flips) so test/CI pollution is minimal.
src/settings.rs
Outdated
| /// Reserved flag for whether the conversational profile onboarding has | ||
| /// been completed. | ||
| /// | ||
| /// Intended semantics (not yet wired into the agent loop): this will be | ||
| /// set during the user's first interaction with the running assistant | ||
| /// (not during the setup wizard), after the agent builds a psychographic | ||
| /// profile via `memory_write`. | ||
| /// | ||
| // TODO: Wire into agent loop — currently the system uses bootstrap presence | ||
| // and `is_populated()` to gate onboarding. This flag will eventually | ||
| // distinguish "never onboarded" from "onboarded but profile was reset". |
There was a problem hiding this comment.
The doc comment says profile_onboarding_completed is “not yet wired into the agent loop”, but this PR uses it to suppress BOOTSTRAP injection (loaded in app.rs and checked in Workspace::system_prompt_for_context_inner). Update the comment/TODO to match current behavior so future work doesn’t duplicate or regress it.
| /// Reserved flag for whether the conversational profile onboarding has | |
| /// been completed. | |
| /// | |
| /// Intended semantics (not yet wired into the agent loop): this will be | |
| /// set during the user's first interaction with the running assistant | |
| /// (not during the setup wizard), after the agent builds a psychographic | |
| /// profile via `memory_write`. | |
| /// | |
| // TODO: Wire into agent loop — currently the system uses bootstrap presence | |
| // and `is_populated()` to gate onboarding. This flag will eventually | |
| // distinguish "never onboarded" from "onboarded but profile was reset". | |
| /// Flag for whether the conversational profile onboarding has been | |
| /// completed. | |
| /// | |
| /// Current semantics: this is persisted in settings and used by the | |
| /// agent loop (via the workspace/system-prompt wiring) to suppress | |
| /// BOOTSTRAP/profile-onboarding injection once the user has already | |
| /// completed their conversational onboarding. | |
| /// | |
| /// Future semantics: this should be set during the user's first | |
| /// interaction with the running assistant (not during the setup wizard), | |
| /// after the agent builds a psychographic profile via `memory_write`, | |
| /// and may be extended to distinguish "never onboarded" from "onboarded | |
| /// but profile was reset". |
There was a problem hiding this comment.
Good catch — updated the doc comment in 7cb9f86 to reflect that this flag is now actively wired into the agent loop for suppressing BOOTSTRAP.md injection.
There was a problem hiding this comment.
Fixed in f14d136 — the duplicate listener registration block has been removed.
| "name": "memory_write", | ||
| "arguments": { | ||
| "target": "context/profile.json", | ||
| "content": "{\"preferred_name\":\"Alex\",\"confidence\":0.3,\"cohort\":{\"cohort\":\"unknown\",\"confidence\":30},\"communication\":{\"tone\":\"warm\",\"formality\":\"casual\",\"detail_level\":\"unknown\",\"pace\":\"unknown\",\"response_speed\":\"unknown\",\"decision_making\":\"unknown\"},\"personality\":{\"openness\":50,\"conscientiousness\":50,\"extraversion\":50,\"agreeableness\":50,\"neuroticism\":50,\"resilience\":50,\"creativity\":50,\"analytical\":50,\"empathy\":50},\"behavior\":{\"frictions\":[],\"desired_outcomes\":[],\"pain_points\":[],\"suggested_support\":[]},\"interaction_preferences\":{\"feedback_style\":\"direct\",\"proactivity_style\":\"reactive\"},\"friendship\":{\"style\":\"unknown\",\"qualities\":[]},\"context\":{\"profession\":\"unknown\",\"timezone\":\"unknown\",\"focus_areas\":[]},\"assistance\":{\"proactivity\":\"medium\",\"goals\":[],\"notification_preferences\":\"moderate\"},\"analysis_metadata\":{\"message_count\":2,\"topic_variety\":1},\"schema_version\":2,\"created_at\":\"2026-03-10T00:00:00Z\",\"updated_at\":\"2026-03-10T00:00:00Z\"}" |
There was a problem hiding this comment.
The trace fixture writes context/profile.json with a schema that doesn’t match crate::profile::PsychographicProfile (e.g. uses schema_version, different personality shape). Because memory_write now attempts to parse/sync profile docs and set onboarding-complete based on successful parsing, this fixture should be updated to the current PROFILE_JSON_SCHEMA (including version) to exercise the real bootstrap flow.
| "content": "{\"preferred_name\":\"Alex\",\"confidence\":0.3,\"cohort\":{\"cohort\":\"unknown\",\"confidence\":30},\"communication\":{\"tone\":\"warm\",\"formality\":\"casual\",\"detail_level\":\"unknown\",\"pace\":\"unknown\",\"response_speed\":\"unknown\",\"decision_making\":\"unknown\"},\"personality\":{\"openness\":50,\"conscientiousness\":50,\"extraversion\":50,\"agreeableness\":50,\"neuroticism\":50,\"resilience\":50,\"creativity\":50,\"analytical\":50,\"empathy\":50},\"behavior\":{\"frictions\":[],\"desired_outcomes\":[],\"pain_points\":[],\"suggested_support\":[]},\"interaction_preferences\":{\"feedback_style\":\"direct\",\"proactivity_style\":\"reactive\"},\"friendship\":{\"style\":\"unknown\",\"qualities\":[]},\"context\":{\"profession\":\"unknown\",\"timezone\":\"unknown\",\"focus_areas\":[]},\"assistance\":{\"proactivity\":\"medium\",\"goals\":[],\"notification_preferences\":\"moderate\"},\"analysis_metadata\":{\"message_count\":2,\"topic_variety\":1},\"schema_version\":2,\"created_at\":\"2026-03-10T00:00:00Z\",\"updated_at\":\"2026-03-10T00:00:00Z\"}" | |
| "content": "{\"preferred_name\":\"Alex\",\"confidence\":0.3,\"cohort\":{\"cohort\":\"unknown\",\"confidence\":30},\"communication\":{\"tone\":\"warm\",\"formality\":\"casual\",\"detail_level\":\"unknown\",\"pace\":\"unknown\",\"response_speed\":\"unknown\",\"decision_making\":\"unknown\"},\"personality\":{\"openness\":50,\"conscientiousness\":50,\"extraversion\":50,\"agreeableness\":50,\"neuroticism\":50,\"resilience\":50,\"creativity\":50,\"analytical\":50,\"empathy\":50},\"behavior\":{\"frictions\":[],\"desired_outcomes\":[],\"pain_points\":[],\"suggested_support\":[]},\"interaction_preferences\":{\"feedback_style\":\"direct\",\"proactivity_style\":\"reactive\"},\"friendship\":{\"style\":\"unknown\",\"qualities\":[]},\"context\":{\"profession\":\"unknown\",\"timezone\":\"unknown\",\"focus_areas\":[]},\"assistance\":{\"proactivity\":\"medium\",\"goals\":[],\"notification_preferences\":\"moderate\"},\"analysis_metadata\":{\"message_count\":2,\"topic_variety\":1},\"version\":2,\"created_at\":\"2026-03-10T00:00:00Z\",\"updated_at\":\"2026-03-10T00:00:00Z\"}" |
There was a problem hiding this comment.
Good catch — the fixture uses schema_version while the actual struct expects version. Will update the fixture to match the current PROFILE_JSON_SCHEMA.
There was a problem hiding this comment.
Fixed in 71c0aeb — updated schema_version → version in the fixture to match PROFILE_JSON_SCHEMA in profile.rs.
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Review: feat: chat onboarding and routine advisor
Verdict: Request changes -- security regression must be addressed before merge.
What It Does
Replaces the interactive setup wizard's onboarding step with a conversational "bootstrap greeting" system:
- Psychographic profiling (
src/profile.rs, 1145 lines) -- 9-dimension analysis framework with personality traits, communication preferences, cohort classification, behavior patterns. Well-designed types with backward-compatible deserializers. - Bootstrap greeting -- Fresh workspaces get a proactive LLM greeting that naturally profiles the user through conversation. Triggered in
agent_loop.rswhenbootstrap_pendingis true. - Seed files -- Identity documents moved from hardcoded strings to
src/workspace/seeds/directory loaded viainclude_str!. Good maintainability improvement. - Profile evolution -- Weekly routine that re-analyzes the profile from recent conversations with confidence gating.
- Skills -- New
delegationandroutine-advisorSKILL.md files.
Critical Issue: Identity File Write Protection Removed
Both Gemini and Copilot flagged this independently. I agree -- this is a blocker.
Commit 2 removes PROTECTED_IDENTITY_FILES and all write checks for IDENTITY.md, SOUL.md, AGENTS.md, and USER.md. These files are injected directly into the system prompt. If an attacker (via prompt injection in tool output) tricks the LLM into calling memory_write(target="SOUL.md", content="[malicious instructions]"), every future session is permanently compromised.
The original code had explicit comments documenting this threat model. The removal has no explanation or replacement mitigation.
Fix options:
- Restore the write protection with an exception for
context/profile.json(which is the legitimate write target for profiling) - If the intent is that the LLM should be able to update these files during onboarding, add a gating mechanism (e.g., only allow writes to identity files during bootstrap, or require user approval)
Secondary Concerns
- Prompt injection in profile fields: Profile data (tone, goals, pain_points) populated from user conversation is rendered into system prompts without sanitization. If a user's early messages contain injection payloads, they persist in the profile and influence all future sessions.
- Unsupervised profile evolution: The weekly evolution routine re-analyzes the profile without user approval or notification. Should at minimum log/audit profile changes.
- PR size: 3,000+ lines across 30 files. The two-commit structure (add
onboarding_chat.rsthen immediately delete it) suggests the PR could have been squashed before submission.
What's Good
- Seed files in
src/workspace/seeds/is a clear improvement over hardcoded strings - Profile type system is well-designed with backward compatibility
- Confidence gating (only inject profile when
> 0.6) prevents premature personalization - The architectural shift from a dedicated onboarding engine to system-prompt-driven flow is sound
profile_onboarding_completedsetting prevents repeat bootstrap
Required Before Merge
- Restore identity file write protection or replace with an equivalent safeguard
- Address profile field sanitization (at minimum, document why it's safe)
Nice to Have
- Squash the two commits to eliminate the add-then-delete of
onboarding_chat.rs - Add tests for profile deserialization edge cases and system prompt injection
- Routine-advisor skill should use 5-field cron format to match tool docs
…nboarding-and-routine-advisor # Conflicts: # tests/support/test_rig.rs
…prompt injection Identity files (SOUL.md, AGENTS.md, USER.md, IDENTITY.md) are injected into every system prompt. Rather than hard-blocking writes (which broke onboarding), scan content through the existing Sanitizer and reject writes with High/Critical severity injection patterns. Medium/Low warnings are logged but allowed. Also clarifies AGENTS.md identity file roles (USER.md = user info, IDENTITY.md = agent identity) and adds IDENTITY.md setup as an explicit bootstrap step. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…wiring The field is now actively used by the agent loop to suppress BOOTSTRAP.md injection — remove the stale "not yet wired" TODO. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When the user authenticates via NEAR AI Cloud API key (option 4), api_key_login() stores the key via set_runtime_env(). But build_nearai_model_fetch_config() was using std::env::var() which doesn't check the runtime overlay — so model listing fell back to session-token auth and re-triggered the interactive NEAR AI authentication menu. Switch to env_or_override() which checks both real env vars and the runtime overlay. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
persist_assistant_response was called with channel="default",
user_id="system" but the assistant thread was created via
get_or_create_assistant_conversation("default", "gateway") which owns
the conversation as user_id="default", channel="gateway". The mismatch
caused ensure_writable_conversation to reject the write with:
WARN Rejected write for unavailable thread id user=system channel=default
[skip-regression-check]
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The Content-Security-Policy header (added in f48fe95) blocks inline JS via script-src 'self'. All onclick/onchange attributes in index.html are replaced with getElementById().addEventListener() calls. Dynamic inline handlers in app.js (jobs, routines, memory breadcrumb, code blocks, TEE report) are replaced with data-action attributes and a single delegated click handler on document. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…chema field
- Bootstrap IncomingMessage now uses ("default", "gateway") consistently
with persist and session registration calls
- Update bootstrap_greeting.json fixture: schema_version → version to
match current PROFILE_JSON_SCHEMA
[skip-regression-check]
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
[skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
src/workspace/mod.rs:489
- Prompt-injection scanning in
append()is currently applied only to the newly appendedcontent, but the actual file content that will be injected into the system prompt isnew_content(existing + appended). This allows bypassing detection by splitting a high-severity pattern across the boundary (e.g., existing ends with "ignore" and the append starts with " previous"). Since you already loaddoc.contentto buildnew_content, scan the combined content (or at least the boundary window) before writing.
pub async fn append(&self, path: &str, content: &str) -> Result<(), WorkspaceError> {
let path = normalize_path(path);
// Scan system-prompt-injected files for prompt injection.
if is_system_prompt_file(&path) && !content.is_empty() {
reject_if_injected(&path, content)?;
}
let doc = self
.storage
.get_or_create_document_by_path(&self.user_id, self.agent_id, &path)
.await?;
let new_content = if doc.content.is_empty() {
content.to_string()
} else {
format!("{}\n{}", doc.content, content)
};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let bootstrap_thread_id = if self | ||
| .workspace() | ||
| .is_some_and(|ws| ws.take_bootstrap_pending()) | ||
| { | ||
| tracing::debug!( | ||
| "Fresh workspace detected — persisting static bootstrap greeting to DB" | ||
| ); | ||
| if let Some(store) = self.store() { | ||
| let thread_id = store | ||
| .get_or_create_assistant_conversation("default", "gateway") | ||
| .await | ||
| .ok(); | ||
| if let Some(id) = thread_id { | ||
| self.persist_assistant_response(id, "gateway", "default", BOOTSTRAP_GREETING) | ||
| .await; | ||
| } | ||
| thread_id |
There was a problem hiding this comment.
Acknowledged — the hardcoded "default" user_id is consistent with the current single-tenant model used throughout the codebase (agent loop, session manager, workspace). Deriving from ws.user_id() is a good future improvement when multi-tenant support is added, but changing it here would create an inconsistency with the rest of the bootstrap flow without the broader refactor. No code change.
| // When testing bootstrap, the channel must be named "gateway" because | ||
| // the bootstrap greeting targets only the gateway channel. | ||
| let test_channel = if self.keep_bootstrap { | ||
| Arc::new(TestChannel::new().with_name("gateway")) | ||
| } else { | ||
| Arc::new(TestChannel::new()) | ||
| }; |
There was a problem hiding this comment.
False positive — self is destructured at line 458 (let TestRigBuilder { ..., keep_bootstrap, ... } = self;), and line 664 uses the destructured keep_bootstrap local, not self.keep_bootstrap. bool is Copy, so there is no move issue. This compiles and passes.
| fn reject_if_injected(path: &str, content: &str) -> Result<(), WorkspaceError> { | ||
| let sanitizer = Sanitizer::new(); | ||
| let warnings = sanitizer.detect(content); | ||
| let dominated = warnings.iter().any(|w| w.severity >= Severity::High); |
There was a problem hiding this comment.
Fixed in f29c541 — reject_if_injected now uses a static LazyLock<Sanitizer> instead of constructing a fresh instance on every write.
src/workspace/mod.rs
Outdated
| // may already have a profile from a previous install and doesn't need | ||
| // onboarding). This prevents existing users from getting a spurious | ||
| // first-run ritual after upgrading. | ||
| let has_profile = self.read(paths::PROFILE).await.is_ok(); |
There was a problem hiding this comment.
Fixed in f29c541 — has_profile now uses .is_ok_and(|d| !d.content.trim().is_empty()) so an empty or whitespace-only profile.json does not suppress onboarding. Added two libsql-backed integration tests (seed_if_empty_ignores_empty_profile and seed_if_empty_skips_bootstrap_with_populated_profile) covering both branches.
…tstrap Address Copilot review: - Use LazyLock<Sanitizer> to avoid rebuilding Aho-Corasick + regexes on every workspace write - has_profile check now requires non-empty content, not just file existence, to prevent empty profile.json from suppressing onboarding - Add seed_tests integration tests (libsql-backed) verifying: - Empty profile.json does not suppress BOOTSTRAP.md seeding - Non-empty profile.json correctly suppresses bootstrap for upgrades Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…nboarding-and-routine-advisor Resolved conflict in wizard.rs: staging extracted build_nearai_model_fetch_config into llm/models.rs, removed our duplicate. Applied env_or_override fix to staging's NearAiConfig::for_model_discovery(). Also includes: lazy-init sanitizer (LazyLock), non-empty profile check for bootstrap seeding, and seed_tests integration tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 40 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/support/test_rig.rs
Outdated
| let test_channel = if self.keep_bootstrap { | ||
| Arc::new(TestChannel::new().with_name("gateway")) | ||
| } else { | ||
| Arc::new(TestChannel::new()) | ||
| }; |
There was a problem hiding this comment.
Fixed in 3071493 — same as the earlier comment, changed to use the destructured local. Not a compile error (bool is Copy) but cleaner style.
src/channels/web/static/app.js
Outdated
| document.querySelectorAll('.language-option[data-lang]').forEach(btn => { | ||
| btn.addEventListener('click', () => switchLanguage(btn.dataset.lang)); | ||
| }); |
There was a problem hiding this comment.
Fixed in 3071493 — removed the direct .language-option click listeners. The delegated data-action="switch-language" handler already covers these buttons.
| if let Ok(api_key) = std::env::var("NEARAI_API_KEY") | ||
| && !api_key.is_empty() | ||
| && self.settings.llm_backend.as_deref() == Some("nearai") | ||
| { | ||
| // NEARAI_API_KEY is set and backend auto-detected — skip interactive prompts | ||
| print_info("NEARAI_API_KEY found — using NEAR AI provider"); | ||
| if let Ok(ctx) = self.init_secrets_context().await { | ||
| let key = SecretString::from(api_key.clone()); | ||
| if let Err(e) = ctx.save_secret("llm_nearai_api_key", &key).await { | ||
| tracing::warn!("Failed to persist NEARAI_API_KEY to secrets: {}", e); | ||
| } | ||
| } | ||
| self.llm_api_key = Some(SecretString::from(api_key)); | ||
| if self.settings.selected_model.is_none() { | ||
| let default = crate::llm::DEFAULT_MODEL; | ||
| self.settings.selected_model = Some(default.to_string()); | ||
| print_info(&format!("Using default model: {default}")); | ||
| } | ||
| self.persist_after_step().await; | ||
| } else { | ||
| print_step(1, 2, "Inference Provider"); | ||
| self.step_inference_provider().await?; | ||
| self.persist_after_step().await; | ||
|
|
||
| print_step(2, 2, "Model Selection"); | ||
| self.step_model_selection().await?; | ||
| self.persist_after_step().await; | ||
| } |
There was a problem hiding this comment.
By design — NEARAI is the only provider where we can confidently skip both provider and model selection from a single env var. Other providers (OpenAI, Anthropic) still need interactive model selection since they support many models with different capabilities/pricing. The env-based backend pre-fill just avoids re-asking which provider, not which model.
| if self.settings.llm_backend.is_none() { | ||
| if let Ok(b) = std::env::var("LLM_BACKEND") { | ||
| self.settings.llm_backend = Some(b); | ||
| } else if std::env::var("NEARAI_API_KEY").is_ok() { | ||
| self.settings.llm_backend = Some("nearai".to_string()); | ||
| } else if std::env::var("ANTHROPIC_API_KEY").is_ok() | ||
| || std::env::var("ANTHROPIC_OAUTH_TOKEN").is_ok() | ||
| { | ||
| self.settings.llm_backend = Some("anthropic".to_string()); | ||
| } else if std::env::var("OPENAI_API_KEY").is_ok() { | ||
| self.settings.llm_backend = Some("openai".to_string()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixed in 3071493 — added !b.trim().is_empty() guard so LLM_BACKEND="" is treated as absent and falls through to API-key-based auto-detection.
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Large PR (3900+ lines) that replaces interactive onboarding with a bootstrap greeting flow, adds workspace seed files, routine advisor integration, and web UI cleanup. CI is green.
Observations:
1. Security hardening addressed
The description mentions identity file write protection, sync_profile_documents() sanitization, and profile evolution prompt hardening with <user_data> delimiters. These were addressed in prior review -- good.
2. PR scope is broad
This combines: onboarding replacement, workspace seeding, routine advisor, env detection for setup wizard, bootstrap history persistence, and web UI cleanup. Each of these could arguably be its own PR. The risk is that a bug in one area blocks the entire feature set.
3. .unwrap() check
The .unwrap() calls I found appear to be in test code and seed file context, which is acceptable.
4. Manual testing incomplete
Three manual testing checkboxes are unchecked. For a user-facing onboarding flow change, manual testing is important -- especially the "fresh workspace triggers bootstrap greeting" path.
5. Title accuracy
"feat: chat onboarding and routine advisor" -- accurate for the primary changes, though it undersells the scope (env detection, web UI, seed files).
This needs manual testing of the onboarding flow before merge. The code changes look reasonable but the user experience impact is significant.
Address Copilot review on PR #927: - Remove duplicate language-option click listeners (delegated data-action handler already covers them) - Guard LLM_BACKEND env prefill against empty string to prevent suppressing API-key-based auto-detection - Use destructured local `keep_bootstrap` instead of `self.keep_bootstrap` in test_rig for consistency after destructure Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Re-review: chat onboarding and routine advisor
Both required items from my previous review have been resolved:
-
Identity file write protection -- Fixed across three commits:
67e1aadadded Sanitizer-based scanning at the tool layerfd3e8b6expanded scanning and hardened profile syncf0abb37moved scanning to Workspace::write/append (defense-in-depth -- all write paths now protected, not just memory_write)f29c541added LazyLock for the sanitizer (avoids rebuilding Aho-Corasick per write) and non-empty profile check
-
Profile field sanitization -- Addressed by moving injection scanning to the Workspace layer.
sync_profile_documentswrites now go through the same scanning as any other system-prompt file write. This is the right design -- no special-casing needed.
Additional improvements since last review:
env_or_overridefix for NEARAI_API_KEY (commit104e405)- CSP-compliant event handlers in web UI (commit
63c6637) - Cron normalization for routine_create (5-field -> 7-field)
- Bootstrap completion flag + in-memory safety net
CI fully green (all Clippy variants, formatting, panic check, regression enforcement).
Cross-PR note: This PR shares its first commit with #1277 (ux-overhaul). #1277 is a superset (42 commits vs 28 here). Merging one will require rebasing the other. Recommend landing #927 first since it's the smaller, focused change.
LGTM.
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Withdrawing approval -- code looks good but these changes need manual user testing before merge (onboarding flow, bootstrap greeting, profile generation). Leaving as comment review until testing is complete.
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Pending manual user testing (see previous comment).
| - **Name:** | ||
| - **Timezone:** | ||
| - **Preferences:** | ||
|
|
||
| The agent will fill this in as it learns about you. | ||
| You can also edit this directly to provide context upfront. No newline at end of file |
There was a problem hiding this comment.
One of the issues that we currently have are some (maybe all) of these markdowns are not editable by the agent at least last time I checked. We might need to file a follow-up to allow some of these .mds to be modifiable by the model.
There was a problem hiding this comment.
This is fixed in this PR. The default lock was removed and instead replace with Safety module to analyze for prompt injections at the memory update time in workspace.
Code reviewFound 1 issue:
The comment at lines 691–694 explicitly states:
But this PR adds Lines 80 to 85 in 3071493 Lines 688 to 697 in 3071493 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
…ion-check] BOOTSTRAP.md is now in SYSTEM_PROMPT_FILES and gets injection scanning on write. The old comment incorrectly stated it was not write-protected. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…nboarding-and-routine-advisor Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…gression-check] debug_assert! in execute_tool_with_safety and JobContext::transition_to panicked in test builds before the graceful error path could run. Existing tests (test_cancel_job_completed, test_execute_empty_tool_name_returns_not_found) already cover these paths — they were the ones failing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Fixed in 685f3d8 — updated the stale comments at lines 691-694 of The old comment said BOOTSTRAP.md was "intentionally NOT write-protected", but |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 42 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| parts.push(format!( | ||
| "PROFILE JSON SCHEMA:\nWrite to `context/profile.json` using `memory_write` with this exact structure:\n{}\n\n{}\n\n\ | ||
| If the conversation doesn't reveal enough about a dimension, use defaults/unknown.\n\ | ||
| For personality trait scores: 40-60 is average range. Default to 50 if unclear.\n\ | ||
| Only score above 70 or below 30 with strong evidence.", | ||
| crate::profile::ANALYSIS_FRAMEWORK, | ||
| crate::profile::PROFILE_JSON_SCHEMA, | ||
| )); |
There was a problem hiding this comment.
Fixed in 204bd85 — the format string now labels the two sections separately: PROFILE ANALYSIS FRAMEWORK: followed by the framework text, then PROFILE JSON SCHEMA: followed by the target JSON structure. The LLM can now clearly distinguish which blob is descriptive context vs the target schema.
| if self.settings.llm_backend.is_none() { | ||
| if let Ok(b) = std::env::var("LLM_BACKEND") | ||
| && !b.trim().is_empty() | ||
| { | ||
| self.settings.llm_backend = Some(b); | ||
| } else if std::env::var("NEARAI_API_KEY").is_ok() { | ||
| self.settings.llm_backend = Some("nearai".to_string()); | ||
| } else if std::env::var("ANTHROPIC_API_KEY").is_ok() | ||
| || std::env::var("ANTHROPIC_OAUTH_TOKEN").is_ok() | ||
| { | ||
| self.settings.llm_backend = Some("anthropic".to_string()); | ||
| } else if std::env::var("OPENAI_API_KEY").is_ok() { | ||
| self.settings.llm_backend = Some("openai".to_string()); | ||
| } |
There was a problem hiding this comment.
Fixed in 204bd85 — all four std::env::var(...).is_ok() checks in quick-mode backend auto-detection now use .is_ok_and(|v| !v.is_empty()) so empty env vars are not treated as present.
| // Sync derived identity documents when the profile is written. | ||
| let mut synced_docs: Vec<&str> = Vec::new(); | ||
| if path == paths::PROFILE { | ||
| match self.workspace.sync_profile_documents().await { | ||
| Ok(true) => { | ||
| tracing::info!("profile write: synced USER.md + assistant-directives.md"); | ||
| synced_docs.extend_from_slice(&[paths::USER, paths::ASSISTANT_DIRECTIVES]); |
There was a problem hiding this comment.
Fixed in 204bd85 — the target path is now normalized (trim + collapse double slashes) before comparing with paths::PROFILE, so non-canonical variants like context//profile.json still trigger profile sync and onboarding completion.
src/workspace/mod.rs
Outdated
| let has_profile = self | ||
| .read(paths::PROFILE) | ||
| .await | ||
| .is_ok_and(|d| !d.content.trim().is_empty()); |
There was a problem hiding this comment.
Fixed in 204bd85 — seed_if_empty() now requires successful serde_json::from_str::<serde_json::Value>() parse before treating profile.json as populated. Corrupted or non-JSON content no longer suppresses bootstrap seeding. Added regression test seed_if_empty_ignores_corrupted_profile.
…nboarding-and-routine-advisor
…lization, profile validation 1. Label ANALYSIS_FRAMEWORK and PROFILE_JSON_SCHEMA sections separately in bootstrap prompt so the LLM knows which blob is the target structure. 2. Wizard quick-mode backend auto-detection now rejects empty env vars (std::env::var().is_ok_and(|v| !v.is_empty())) to avoid selecting the wrong backend when e.g. NEARAI_API_KEY="" is set. 3. Normalize the target path before comparing with paths::PROFILE in memory_write so non-canonical variants like "context//profile.json" still trigger profile sync. 4. seed_if_empty now requires valid JSON parse of context/profile.json before treating it as a populated profile. Corrupted content no longer permanently suppresses bootstrap seeding. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…nboarding-and-routine-advisor Resolved conflict in src/tools/execute.rs (kept empty-name early return guard). Updated cron schedule assertion in e2e_builtin_tool_coverage to match staging's store-as-is behavior (6-field, not normalized 7-field).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 40 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub async fn append(&self, path: &str, content: &str) -> Result<(), WorkspaceError> { | ||
| let path = normalize_path(path); | ||
| // Scan system-prompt-injected files for prompt injection. | ||
| if is_system_prompt_file(&path) && !content.is_empty() { | ||
| reject_if_injected(&path, content)?; | ||
| } |
| !d.content.trim().is_empty() | ||
| && serde_json::from_str::<serde_json::Value>(&d.content).is_ok() |
| // Pre-populate backend from env so step_inference_provider | ||
| // can offer "Keep current provider?" instead of asking from scratch. | ||
| if self.settings.llm_backend.is_none() { | ||
| if let Ok(b) = std::env::var("LLM_BACKEND") | ||
| && !b.trim().is_empty() | ||
| { | ||
| self.settings.llm_backend = Some(b); | ||
| } else if std::env::var("NEARAI_API_KEY").is_ok_and(|v| !v.is_empty()) { |
Summary
onboarding_chat.rsenginesrc/workspace/seeds/instead of hardcoded stringsNEARAI_API_KEY,ANTHROPIC_API_KEY,OPENAI_API_KEY, andLLM_BACKENDenv vars, skipping redundant provider prompts during onboardingSecurity (addressed in review)
Sanitizer::detect()scanningsync_profile_documents()sanitized before writing to system-prompt-injected files<user_data>delimiterscontext/assistant-directives.mdadded to injection-scanned file setBuilds on the NPA psychographic profiling work from #321.
Test plan
cargo fmt— no formatting issuescargo clippy --all --benches --tests --examples --all-features— zero warningscargo test— unit tests passNEARAI_API_KEYin env skips provider prompt during onboardingCloses #321
🤖 Generated with Claude Code