refactor: Remove unused functions from skills module

- Remove is_embedded_skill() from discovery.rs (unused) - Remove get_embedded_skills_map() from embedded.rs (unused) - Remove associated tests for deleted functions - Inline path check in test_repo_overrides_embedded test This eliminates dead code warnings and reduces module surface area without changing any behavior. Agent: fowler
2026-02-05 14:17:56 +11:00
parent 38da6a56ef
commit c3549ce043
4 changed files with 170 additions and 27 deletions
--- a/analysis/breaker/2025-02-05.md
+++ b/analysis/breaker/2025-02-05.md
@@ -0,0 +1,138 @@
+# Breaker Report: 2025-02-05
+
+Focused on changes in commits b6d2582..9443f933 (past 10 commits).
+
+## Issue 1: JSON Escaping Bug in g3-research Script
+
+### Title
+`g3-research` produces invalid JSON when query contains actual newlines
+
+### Repro
+```bash
+# In skills/research/g3-research, the write_status function uses:
+escaped_query=$(echo -n "$query" | sed 's/\\/\\\\/g; s/"/\\"/g; s/\n/\\n/g')
+
+# Test with actual newlines:
+QUERY=$'What is\nthe best\nRust library?'
+escaped=$(echo -n "$QUERY" | sed 's/\\/\\\\/g; s/"/\\"/g; s/\n/\\n/g')
+echo "{\"query\": \"$escaped\"}" | python3 -m json.tool
+# Output: Invalid control character at: line 1 column 19 (char 18)
+```
+
+**Expected**: Valid JSON with `\n` escape sequences  
+**Actual**: Invalid JSON with literal newline characters
+
+### Diagnosis
+- **File**: `skills/research/g3-research:66`
+- **Root cause**: The sed pattern `s/\n/\\n/g` matches the literal two-character string `\n`, not actual newline characters. Sed processes line-by-line by default and doesn't see newlines in the pattern space.
+- **Triggering condition**: User query contains actual newline characters (e.g., from multi-line input or programmatic construction)
+- **Deterministic**: Yes
+
+### Impact
+- **Severity**: Incorrect behavior - `status.json` becomes unparseable
+- **Likelihood**: Uncommon but possible - queries are typically single-line, but multi-line queries from programmatic sources or copy-paste could trigger this
+
+### Fix
+Replace sed with perl which handles newlines correctly:
+```bash
+escaped_query=$(echo -n "$query" | perl -pe 's/\\/\\\\/g; s/"/\\"/g; s/\n/\\n/g')
+```
+
+---
+
+## Issue 2: Embedded Skill Path Not Readable
+
+### Title
+Embedded skills have non-existent file paths that agents are instructed to `read_file`
+
+### Repro
+```
+# When no repo skills/ directory exists, the research skill is loaded from embedded
+# The generated prompt contains:
+<skill>
+  <name>research</name>
+  <description>...</description>
+  <location><embedded:research>/SKILL.md</location>
+</skill>
+
+# The prompt instructs:
+"read the full skill file using `read_file` to get detailed instructions"
+
+# Agent attempts:
+read_file("<embedded:research>/SKILL.md")
+# Result: File not found error
+```
+
+**Expected**: Agent can read skill documentation  
+**Actual**: File path doesn't exist on disk
+
+### Diagnosis
+- **File**: `crates/g3-core/src/skills/discovery.rs:97` - sets path to `<embedded:name>/SKILL.md`
+- **File**: `crates/g3-core/src/skills/prompt.rs:14-15` - instructs agent to use `read_file`
+- **Root cause**: Embedded skills use a synthetic path marker, but the prompt doesn't account for this
+- **Triggering condition**: User has no `skills/` directory in their repo (embedded skill not overridden)
+- **Deterministic**: Yes
+
+### Impact
+- **Severity**: Annoying - agent will fail to read skill docs and may hallucinate or ask for help
+- **Likelihood**: Common for users outside the g3 repo itself
+
+### Possible Fixes
+1. Include the full skill body in the prompt for embedded skills (increases prompt size)
+2. Add special handling in `read_file` for `<embedded:*>` paths
+3. Change prompt to say "skill instructions are below" for embedded skills and inline the body
+
+---
+
+## Issue 3: Hardcoded 'main' Branch in SDLC Pipeline
+
+### Title
+`studio sdlc` assumes default branch is named 'main'
+
+### Repro
+```bash
+# In a repo where default branch is 'master':
+studio sdlc run
+
+# has_commits_on_branch runs:
+git rev-list --count main..sdlc/session-branch
+# Fails silently (returns Ok(false)) because 'main' doesn't exist
+
+# merge_to_main runs:
+git checkout main
+# Fails with "Failed to checkout main"
+```
+
+**Expected**: Works with any default branch name  
+**Actual**: Fails or behaves incorrectly on repos using 'master' or other branch names
+
+### Diagnosis
+- **File**: `crates/studio/src/main.rs:720` - `has_commits_on_branch()` hardcodes `main..{branch}`
+- **File**: `crates/studio/src/git.rs` - `merge_to_main()` hardcodes `checkout main`
+- **Root cause**: No detection of actual default branch name
+- **Triggering condition**: Repository uses 'master' or custom default branch
+- **Deterministic**: Yes
+
+### Impact
+- **Severity**: Incorrect behavior - merge fails or skipped incorrectly
+- **Likelihood**: Common - many repos still use 'master'
+
+### Fix
+Detect default branch:
+```bash
+git symbolic-ref refs/remotes/origin/HEAD | sed 's@^refs/remotes/origin/@@'
+# or
+git config --get init.defaultBranch
+```
+
+---
+
+## Summary
+
+| # | Issue | Severity | Likelihood |
+|---|-------|----------|------------|
+| 1 | JSON escaping with newlines | Incorrect behavior | Uncommon |
+| 2 | Embedded skill path unreadable | Annoying | Common |
+| 3 | Hardcoded 'main' branch | Incorrect behavior | Common |
+
+All issues are deterministic and reproducible.
--- a/analysis/memory.md
+++ b/analysis/memory.md
@@ -1,5 +1,5 @@
 # Workspace Memory
-> Updated: 2026-02-04T23:42:21Z | Size: 19.9k chars
+> Updated: 2026-02-05T03:06:55Z | Size: 23.5k chars

 ### Remember Tool Wiring
 - `crates/g3-core/src/tools/memory.rs` [0..5000] - `execute_remember()`, `get_memory_path()`, `merge_memory()`
@@ -412,3 +412,33 @@ allowed-tools: Bash Read  # Optional/experimental
 # Skill Title
 Detailed instructions...
 ```
+
+### Embedded Skills System
+Skills are portable packages with SKILL.md + optional scripts, discovered from multiple locations.
+
+- `crates/g3-core/src/skills/embedded.rs`
+  - `EmbeddedSkill` [22..28] - name, skill_md content, scripts array
+  - `EMBEDDED_SKILLS` [32..42] - static array with include_str! for research skill
+  - `get_embedded_skill()` [48..50] - lookup by name
+
+- `crates/g3-core/src/skills/extraction.rs`
+  - `extract_script()` [28..85] - extracts embedded script to `.g3/bin/`, tracks version hash
+  - `needs_update()` [107..118] - compares stored hash vs current content
+  - `compute_hash()` [121..128] - DefaultHasher for version tracking
+
+- `crates/g3-core/src/skills/discovery.rs`
+  - `discover_skills()` [38..85] - scans 5 locations in priority order (embedded → global → extra → workspace → repo)
+  - `load_embedded_skills()` [88..102] - sets synthetic path `<embedded:name>/SKILL.md`
+  - `is_embedded_skill()` [161..163] - checks if path starts with `<embedded:`
+
+- `skills/research/g3-research` - bash script for async research
+  - `write_status()` [52..88] - writes status.json (BUG: sed doesn't escape real newlines)
+  - `extract_report()` [98..135] - extracts between markers or falls back to filtering
+
+### SDLC Pipeline Merge
+- `crates/studio/src/main.rs`
+  - `has_commits_on_branch()` [715..728] - counts commits ahead of main (hardcodes 'main')
+  - `cmd_sdlc_run()` [664..708] - merges on completion, preserves worktree on failure
+
+- `crates/studio/src/git.rs`
+  - `merge_to_main()` - checkouts main and merges branch (hardcodes 'main')
--- a/crates/g3-core/src/skills/discovery.rs
+++ b/crates/g3-core/src/skills/discovery.rs
@@ -161,11 +161,6 @@ fn expand_tilde(path: &str) -> PathBuf {
    PathBuf::from(expanded.as_ref())
 }

-/// Check if a skill is from an embedded source.
-pub fn is_embedded_skill(skill: &Skill) -> bool {
-    skill.path.starts_with("<embedded:")
-}
-
 #[cfg(test)]
 mod tests {
    use super::*;
@@ -247,7 +242,7 @@ mod tests {
        
        let research = skills.iter().find(|s| s.name == "research").unwrap();
        assert_eq!(research.description, "Custom research skill");
-        assert!(!is_embedded_skill(research), "Should not be marked as embedded");
+        assert!(!research.path.starts_with("<embedded:"), "Should not be marked as embedded");
    }
    
    #[test]
@@ -373,11 +368,4 @@ mod tests {
        let no_tilde = expand_tilde("/absolute/path");
        assert_eq!(no_tilde, PathBuf::from("/absolute/path"));
    }
-    
-    #[test]
-    fn test_is_embedded_skill() {
-        let skills = discover_skills(None, &[]);
-        let research = skills.iter().find(|s| s.name == "research").unwrap();
-        assert!(is_embedded_skill(research));
-    }
 }
--- a/crates/g3-core/src/skills/embedded.rs
+++ b/crates/g3-core/src/skills/embedded.rs
@@ -10,8 +10,6 @@
 //! 4. Global `~/.g3/skills/` directory
 //! 5. Embedded skills (this module - always available)

-use std::collections::HashMap;
-
 /// An embedded skill with its SKILL.md content and optional scripts.
 #[derive(Debug, Clone)]
 pub struct EmbeddedSkill {
@@ -48,11 +46,6 @@ pub fn get_embedded_skill(name: &str) -> Option<&'static EmbeddedSkill> {
    EMBEDDED_SKILLS.iter().find(|s| s.name == name)
 }

-/// Get embedded skills as a map for easy lookup.
-pub fn get_embedded_skills_map() -> HashMap<&'static str, &'static EmbeddedSkill> {
-    EMBEDDED_SKILLS.iter().map(|s| (s.name, s)).collect()
-}
-
 #[cfg(test)]
 mod tests {
    use super::*;
@@ -78,10 +71,4 @@ mod tests {
        assert!(get_embedded_skill("research").is_some());
        assert!(get_embedded_skill("nonexistent").is_none());
    }
-
-    #[test]
-    fn test_skills_map() {
-        let map = get_embedded_skills_map();
-        assert!(map.contains_key("research"));
-    }
 }