feat: GitHub integration, context pinning, agent profiles#50
Merged
kienbui1995 merged 1 commit intomainfrom Apr 13, 2026
Merged
feat: GitHub integration, context pinning, agent profiles#50kienbui1995 merged 1 commit intomainfrom
kienbui1995 merged 1 commit intomainfrom
Conversation
1. /gh commands — leverages gh CLI (already installed): /gh prs, /gh pr-create, /gh issues, /gh issue <N>, /gh status, /gh checks, /gh repo, /gh browse 2. Context pinning — /pin marks last message as pinned: - Pinned messages survive compaction (both naive and smart) - ConversationMessage.pinned field (serde default false) - Wired in both compact_session and smart_compact 3. Agent profiles — /profile save|load|list: - Saves model, effort, plan_mode, theme to ~/.config/magic-code/profiles/ - Load switches config instantly 4. Streaming cost — already working (confirmed in status bar) 274 tests, 0 fail.
📝 WalkthroughWalkthroughThe changes introduce a message pinning feature that allows conversations to preserve specific messages through compaction, adds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mc/crates/mc-core/src/compact.rs (1)
89-149:⚠️ Potential issue | 🟠 Major
compact_sessionstill compacts pinned messages.This path never filters
msg.pinnedout ofold, so pinned history can still be summarized or dropped whenever the non-LLM compactor is used. That breaks the new feature contract outsidesmart_compact.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mc/crates/mc-core/src/compact.rs` around lines 89 - 149, compact_session currently removes and summarizes messages without respecting ConversationMessage.pinned, causing pinned messages to be compacted; modify compact_session so that when you build the "old" set to process you exclude pinned messages (i.e., do not drain or include messages where msg.pinned == true), or if you must drain, immediately reinsert any drained pinned messages back into session.messages at their original positions before scoring/summarizing; update the variables around old, scored, to_summarize and the reinsertion loop (references: compact_session, session.messages, old, scored, to_summarize, ConversationMessage::pinned) so pinned messages are preserved unchanged and not summarized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mc/crates/mc-cli/src/main.rs`:
- Around line 1278-1284: The PendingCommand::Pin branch can silently do nothing
because it ignores runtime.try_lock() failures and the empty-session case;
change it to acquire the runtime lock in a way that guarantees the operation or
reports failure (e.g., use runtime.lock() or handle the Err from try_lock() and
send/log an error), then only set msg.pinned = true when
rt.session.messages.last_mut() returns Some, and otherwise report/log/notify
that there was no message to pin so the TUI doesn't incorrectly show success;
reference PendingCommand::Pin, runtime.try_lock()/runtime.lock(), and
rt.session.messages.last_mut()/msg.pinned to locate and update the code.
In `@mc/crates/mc-core/src/compact.rs`:
- Around line 203-209: The naive-fallback early return drops previously drained
pinned messages; before returning from the Some(Err(e)) branch in the compaction
logic, reinsert the drained pinned messages back into session.messages
(alongside the newly inserted
ConversationMessage::user(build_naive_summary(&to_summarize))) so pinned entries
are preserved. Locate the branch handling Some(Err(e)) and restore the pinned
collection (the variable named pinned or equivalent) into session.messages in
the correct order/positions prior to returning Ok(()).
In `@mc/crates/mc-tui/src/commands.rs`:
- Around line 591-610: The profile save/load branches currently use the raw
variable name when building paths (dir.join(format!("{name}.toml"))), allowing
path traversal; validate and sanitize name before joining by reusing the same
basename validation used for plugin names (apply the plugin-name validation
function or regex to the local variable name in both the "save" and "load"
arms), reject or normalize names that contain path separators or traversal
components, and only then call dir.join with the sanitized basename so that
Model/effort/profile writes and reads cannot escape the profiles directory.
- Around line 611-626: The /profile load currently only updates UI fields (e.g.,
app.model and app.theme) and ignores saved runtime settings like effort and
plan_mode, so the active ConversationRuntime keeps using the old model; parse
and apply all runtime-related fields (model, effort, plan_mode) from the loaded
content (similar to the existing strip_prefix/strip_suffix logic) and then call
the runtime update API on the live ConversationRuntime instance (e.g.,
runtime.set_model(...), runtime.set_effort(...), runtime.set_plan_mode(...) or
the equivalent methods on the ConversationRuntime object referenced from app or
state) so the loaded profile affects actual execution before calling app.push to
show the “Profile loaded” message.
- Around line 667-674: The code currently interpolates untrusted user input
(rest, title) into a shell string and sets app.pending_command =
Some(PendingCommand::RunShell(...)), which allows shell injection; instead, stop
passing a single sh -c string and either (A) use a safe argument-based command
form (e.g., add/choose a PendingCommand variant that takes a program +
Vec<String> and set app.pending_command = Some(PendingCommand::RunArgs("gh",
vec!["pr","create","--title", title, "--body","", "--fill"])) for the
"pr-create" arm and similarly for the "issue"/"issues" arms), or (B) at minimum
escape/shell-quote user values using a vetted library before interpolation;
update the match arms handling "pr-create", "issue", and "issues" to build
commands as arg vectors (or escaped strings) rather than formatting rest/title
directly into RunShell.
---
Outside diff comments:
In `@mc/crates/mc-core/src/compact.rs`:
- Around line 89-149: compact_session currently removes and summarizes messages
without respecting ConversationMessage.pinned, causing pinned messages to be
compacted; modify compact_session so that when you build the "old" set to
process you exclude pinned messages (i.e., do not drain or include messages
where msg.pinned == true), or if you must drain, immediately reinsert any
drained pinned messages back into session.messages at their original positions
before scoring/summarizing; update the variables around old, scored,
to_summarize and the reinsertion loop (references: compact_session,
session.messages, old, scored, to_summarize, ConversationMessage::pinned) so
pinned messages are preserved unchanged and not summarized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 993cf8a7-863a-4121-8aaf-b5685cb2fa2c
📒 Files selected for processing (6)
mc/crates/mc-cli/src/main.rsmc/crates/mc-core/src/compact.rsmc/crates/mc-core/src/session.rsmc/crates/mc-core/src/subagent.rsmc/crates/mc-tui/src/app.rsmc/crates/mc-tui/src/commands.rs
Comment on lines
+1278
to
+1284
| PendingCommand::Pin => { | ||
| if let Ok(mut rt) = runtime.try_lock() { | ||
| if let Some(msg) = rt.session.messages.last_mut() { | ||
| msg.pinned = true; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
/pin can silently do nothing.
This uses try_lock() and ignores both lock contention and the empty-session case. Since the TUI already reports success when the command is queued, the user can believe the pin was applied when nothing changed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mc/crates/mc-cli/src/main.rs` around lines 1278 - 1284, The
PendingCommand::Pin branch can silently do nothing because it ignores
runtime.try_lock() failures and the empty-session case; change it to acquire the
runtime lock in a way that guarantees the operation or reports failure (e.g.,
use runtime.lock() or handle the Err from try_lock() and send/log an error),
then only set msg.pinned = true when rt.session.messages.last_mut() returns
Some, and otherwise report/log/notify that there was no message to pin so the
TUI doesn't incorrectly show success; reference PendingCommand::Pin,
runtime.try_lock()/runtime.lock(), and rt.session.messages.last_mut()/msg.pinned
to locate and update the code.
Comment on lines
203
to
209
| Some(Err(e)) => { | ||
| tracing::warn!("smart compaction failed, falling back to naive: {e}"); | ||
| session | ||
| .messages | ||
| .insert(0, ConversationMessage::user(build_naive_summary(&old))); | ||
| session.messages.insert( | ||
| 0, | ||
| ConversationMessage::user(build_naive_summary(&to_summarize)), | ||
| ); | ||
| return Ok(()); |
There was a problem hiding this comment.
Reinsert pinned messages before returning from the naive fallback.
On this branch, pinned has already been drained out of session.messages. Returning immediately after inserting the fallback summary drops every pinned message from the compacted region.
Suggested fix
Some(Err(e)) => {
tracing::warn!("smart compaction failed, falling back to naive: {e}");
session.messages.insert(
0,
ConversationMessage::user(build_naive_summary(&to_summarize)),
);
+ for (i, msg) in pinned.iter().cloned().enumerate() {
+ session.messages.insert(1 + i, msg);
+ }
return Ok(());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Some(Err(e)) => { | |
| tracing::warn!("smart compaction failed, falling back to naive: {e}"); | |
| session | |
| .messages | |
| .insert(0, ConversationMessage::user(build_naive_summary(&old))); | |
| session.messages.insert( | |
| 0, | |
| ConversationMessage::user(build_naive_summary(&to_summarize)), | |
| ); | |
| return Ok(()); | |
| Some(Err(e)) => { | |
| tracing::warn!("smart compaction failed, falling back to naive: {e}"); | |
| session.messages.insert( | |
| 0, | |
| ConversationMessage::user(build_naive_summary(&to_summarize)), | |
| ); | |
| for (i, msg) in pinned.iter().cloned().enumerate() { | |
| session.messages.insert(1 + i, msg); | |
| } | |
| return Ok(()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mc/crates/mc-core/src/compact.rs` around lines 203 - 209, The naive-fallback
early return drops previously drained pinned messages; before returning from the
Some(Err(e)) branch in the compaction logic, reinsert the drained pinned
messages back into session.messages (alongside the newly inserted
ConversationMessage::user(build_naive_summary(&to_summarize))) so pinned entries
are preserved. Locate the branch handling Some(Err(e)) and restore the pinned
collection (the variable named pinned or equivalent) into session.messages in
the correct order/positions prior to returning Ok(()).
Comment on lines
+591
to
+610
| "save" => { | ||
| let name = parts.get(1).copied().unwrap_or("default"); | ||
| let _ = std::fs::create_dir_all(&dir); | ||
| let profile = format!( | ||
| "model = \"{}\"\neffort = \"{}\"\nplan_mode = {}\ntheme = \"{}\"", | ||
| app.model, | ||
| app.effort.symbol(), | ||
| app.plan_mode, | ||
| app.theme | ||
| ); | ||
| let path = dir.join(format!("{name}.toml")); | ||
| match std::fs::write(&path, profile) { | ||
| Ok(()) => app.push(&format!("✅ Profile saved: {name}")), | ||
| Err(e) => app.push(&format!("✗ {e}")), | ||
| } | ||
| } | ||
| "load" => { | ||
| let name = parts.get(1).copied().unwrap_or("default"); | ||
| let path = dir.join(format!("{name}.toml")); | ||
| match std::fs::read_to_string(&path) { |
There was a problem hiding this comment.
Sanitize profile names before joining them into paths.
name is used verbatim in dir.join(format!("{name}.toml")), so /profile save ../../foo and /profile load ../../foo can read or write outside the profiles directory. Reuse the same basename validation pattern you already apply to plugin names.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mc/crates/mc-tui/src/commands.rs` around lines 591 - 610, The profile
save/load branches currently use the raw variable name when building paths
(dir.join(format!("{name}.toml"))), allowing path traversal; validate and
sanitize name before joining by reusing the same basename validation used for
plugin names (apply the plugin-name validation function or regex to the local
variable name in both the "save" and "load" arms), reject or normalize names
that contain path separators or traversal components, and only then call
dir.join with the sanitized basename so that Model/effort/profile writes and
reads cannot escape the profiles directory.
Comment on lines
+611
to
+626
| Ok(content) => { | ||
| for line in content.lines() { | ||
| if let Some(model) = line | ||
| .strip_prefix("model = \"") | ||
| .and_then(|s| s.strip_suffix('"')) | ||
| { | ||
| app.model = model.to_string(); | ||
| } | ||
| if let Some(theme) = line | ||
| .strip_prefix("theme = \"") | ||
| .and_then(|s| s.strip_suffix('"')) | ||
| { | ||
| app.theme = theme.to_string(); | ||
| } | ||
| } | ||
| app.push(&format!("✅ Profile loaded: {name} (model: {})", app.model)); |
There was a problem hiding this comment.
/profile load only updates the UI, not the actual runtime model.
This loader ignores the saved effort and plan_mode, and changing app.model here does not switch ConversationRuntime to that model. After loading, the UI can show one profile while subsequent turns still run with another.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mc/crates/mc-tui/src/commands.rs` around lines 611 - 626, The /profile load
currently only updates UI fields (e.g., app.model and app.theme) and ignores
saved runtime settings like effort and plan_mode, so the active
ConversationRuntime keeps using the old model; parse and apply all
runtime-related fields (model, effort, plan_mode) from the loaded content
(similar to the existing strip_prefix/strip_suffix logic) and then call the
runtime update API on the live ConversationRuntime instance (e.g.,
runtime.set_model(...), runtime.set_effort(...), runtime.set_plan_mode(...) or
the equivalent methods on the ConversationRuntime object referenced from app or
state) so the loaded profile affects actual execution before calling app.push to
show the “Profile loaded” message.
Comment on lines
+667
to
+674
| "pr-create" => { | ||
| let title = if rest.is_empty() { "WIP" } else { rest }; | ||
| app.pending_command = Some(PendingCommand::RunShell(format!("gh pr create --title '{title}' --body '' --fill"))); | ||
| } | ||
| "issues" => app.pending_command = Some(PendingCommand::RunShell("gh issue list --limit 10".into())), | ||
| "issue" => if rest.is_empty() { app.push("Usage: /gh issue <number>"); } else { | ||
| app.pending_command = Some(PendingCommand::RunShell(format!("gh issue view {rest}"))); | ||
| }, |
There was a problem hiding this comment.
Do not interpolate raw /gh arguments into RunShell.
rest and title come from user input and are later executed via sh -c in PendingCommand::RunShell. Quotes or shell metacharacters here become arbitrary command execution.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mc/crates/mc-tui/src/commands.rs` around lines 667 - 674, The code currently
interpolates untrusted user input (rest, title) into a shell string and sets
app.pending_command = Some(PendingCommand::RunShell(...)), which allows shell
injection; instead, stop passing a single sh -c string and either (A) use a safe
argument-based command form (e.g., add/choose a PendingCommand variant that
takes a program + Vec<String> and set app.pending_command =
Some(PendingCommand::RunArgs("gh", vec!["pr","create","--title", title,
"--body","", "--fill"])) for the "pr-create" arm and similarly for the
"issue"/"issues" arms), or (B) at minimum escape/shell-quote user values using a
vetted library before interpolation; update the match arms handling "pr-create",
"issue", and "issues" to build commands as arg vectors (or escaped strings)
rather than formatting rest/title directly into RunShell.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Leveraging existing ecosystem
/gh — GitHub via gh CLI (zero new deps)
/pin — Context pinning
Pinned messages survive compaction.
/pinmarks the last message./profile — Agent profiles
274 tests, 0 fail.
Summary by CodeRabbit
/pincommand to preserve specific messages during session compaction./ghcommand for GitHub operations (view issues, create PRs, check status, browse repos)./profilecommand to save, load, and list configuration profiles (model, theme, effort, plan mode).