diff --git a/.codegraph/.gitignore b/.codegraph/.gitignore new file mode 100644 index 00000000..d20c0fe4 --- /dev/null +++ b/.codegraph/.gitignore @@ -0,0 +1,5 @@ +# CodeGraph data files — local to each machine, not for committing. +# Ignore everything in .codegraph/ except this file itself, so transient +# files (the database, daemon.pid, sockets, logs) never show up in git. +* +!.gitignore diff --git a/.codex/AGENTS.md b/.codex/AGENTS.md new file mode 120000 index 00000000..b1cdfc35 --- /dev/null +++ b/.codex/AGENTS.md @@ -0,0 +1 @@ +../.agents/AGENTS.md \ No newline at end of file diff --git a/.codex/instructions.md b/.codex/instructions.md new file mode 120000 index 00000000..b1cdfc35 --- /dev/null +++ b/.codex/instructions.md @@ -0,0 +1 @@ +../.agents/AGENTS.md \ No newline at end of file diff --git a/.gitignore b/.gitignore index 97639729..96f92e62 100644 --- a/.gitignore +++ b/.gitignore @@ -36,54 +36,52 @@ node_modules/ pnpm-debug.log perf_test_data/ tmp +.opencode/package-lock.json # START AI Agent Symlinks .agent/commands -.agent/commands.bak +.agent/commands.bak.* .agent/rules/ -.agent/rules/instructions.md -.agent/rules/instructions.md.bak .agent/skills -.agent/skills.bak +.agent/skills.bak.* .agent/skills/ .agents/skills/*.bak .claude/commands -.claude/commands.bak +.claude/commands.bak.* .claude/commands/ .claude/skills -.claude/skills.bak +.claude/skills.bak.* .claude/skills/ +.codex/AGENTS.md +.codex/AGENTS.md.bak.* .codex/commands -.codex/commands.bak +.codex/commands.bak.* .codex/config.toml -.codex/instructions.md -.codex/instructions.md.bak .codex/skills -.codex/skills.bak +.codex/skills.bak.* .gemini/commands -.gemini/commands.bak +.gemini/commands.bak.* .gemini/commands/ .gemini/settings.json .gemini/skills -.gemini/skills.bak +.gemini/skills.bak.* .gemini/skills/ .github/agents -.github/agents.bak +.github/agents.bak.* .github/copilot-instructions.md -.github/copilot-instructions.md.bak +.github/copilot-instructions.md.bak.* +.mcp.json .opencode/command -.opencode/command.bak +.opencode/command.bak.* .opencode/command/ .opencode/skills -.opencode/skills.bak +.opencode/skills.bak.* .opencode/skills/ .vscode/mcp.json -/.mcp.json -/AGENTS.md -/AGENTS.md.bak -/CLAUDE.md -/CLAUDE.md.bak -/GEMINI.md -/GEMINI.md.bak -/opencode.json +AGENTS.md +AGENTS.md.bak.* +CLAUDE.md +CLAUDE.md.bak.* +GEMINI.md +GEMINI.md.bak.* +opencode.json # END AI Agent Symlinks -.opencode/package-lock.json diff --git a/openspec/changes/init-user-template/design.md b/openspec/changes/init-user-template/design.md new file mode 100644 index 00000000..9cb123a5 --- /dev/null +++ b/openspec/changes/init-user-template/design.md @@ -0,0 +1,99 @@ +# Design: Init User Template + +## Technical Approach + +Add a single `resolve_config_template()` function in `src/init.rs` as the resolution point for config template content. Thread the resolved string through `init()`, `init_wizard()`, and `build_default_config_with_skills_modes()` by changing their signatures. XDG discovery uses `std::env::var` — no new dependencies. + +## Architecture Decisions + +| Decision | Alternatives | Rationale | +|----------|-------------|-----------| +| Single `resolve_config_template()` fn in init.rs | Config method, TemplateResolver struct | It's a string lookup with fallback — a standalone fn is simplest. No state to manage. | +| No `dirs` crate for XDG | Add `dirs` or `directories` crate | `std::env::var("HOME")` + `XDG_CONFIG_HOME` is sufficient. Project already uses env vars directly. Windows users use `--template`. | +| Warn-and-fallback on invalid XDG file | Hard-fail on any invalid template | Explicit `--template` = user intent = hard-fail. Implicit XDG discovery = convenience = graceful degradation. | +| Pass `base_config: &str` to `build_default_config_with_skills_modes` | Global mutable state, config object | Minimal signature change, keeps function pure. | + +## Data Flow + +``` +CLI --template flag + │ + ▼ +resolve_config_template(template_path) + ├─ --template given? → read file → validate TOML → return content + ├─ XDG file exists? → read file → validate (warn on fail) → return content + └─ neither? → return DEFAULT_CONFIG + │ + ▼ + config_content: String + │ + ┌────┴────┐ + │ │ + init() init_wizard() + │ │ + │ build_default_config_with_skills_modes(base_config, modes) + │ │ + ▼ ▼ + fs::write(config_path, content) +``` + +## File Changes + +| File | Action | Description | +|------|--------|-------------| +| `src/main.rs` | Modify | Add `template: Option` to `Init` struct (~line 334). Pass to `init()`/`init_wizard()` at lines 412-418. | +| `src/init.rs` | Modify | New `resolve_config_template()` + `resolve_user_config_path()` fns. Change `init()` (line 197): add `config_content: &str` param, replace `DEFAULT_CONFIG` at line 244. Change `build_default_config_with_skills_modes()` (line 1075): add `base_config: &str` param, replace `DEFAULT_CONFIG.lines()` at line 1079. Change `init_wizard()` (line 1583): add `template_path: Option<&Path>`, resolve template, thread to `init()` calls (lines 1667, 1686, 1695) and `build_default_config_with_skills_modes()` (line 1738). Same for `init_wizard_experimental_tui()`. | +| `website/docs/.../cli.mdx` | Modify | Document `--template` flag and XDG fallback behavior. | + +## Interfaces / Contracts + +```rust +/// Resolve config template content by precedence: +/// 1. Explicit --template path (hard error if invalid) +/// 2. XDG user config (warn + fallback if invalid) +/// 3. DEFAULT_CONFIG +pub fn resolve_config_template(template_path: Option<&Path>) -> Result + +/// Check XDG paths for user config. Returns None if nothing found. +fn resolve_user_config_path() -> Option + +// Changed signatures: +pub fn init(project_root: &Path, force: bool, config_content: &str) -> Result<()> +pub fn init_wizard(project_root: &Path, force: bool, template_path: Option<&Path>) -> Result<()> +pub fn init_wizard_experimental_tui(project_root: &Path, force: bool, template_path: Option<&Path>) -> Result<()> +fn build_default_config_with_skills_modes(base_config: &str, modes: &BTreeMap) -> String +``` + +XDG resolution logic: +```rust +fn resolve_user_config_path() -> Option { + // 1. $XDG_CONFIG_HOME/agentsync/config.toml + // 2. $HOME/.config/agentsync/config.toml + // Returns None if neither exists +} +``` + +## Testing Strategy + +| Layer | What to Test | Approach | +|-------|-------------|----------| +| Unit | `resolve_config_template()` — all 3 precedence cases | Temp files + env var override | +| Unit | `resolve_user_config_path()` — env var combinations | Set/unset `XDG_CONFIG_HOME`, `HOME` | +| Unit | `build_default_config_with_skills_modes()` with custom base | Pass non-default TOML, verify patching | +| Unit | Invalid TOML via `--template` → error | Assert anyhow error with context | +| Unit | Invalid XDG file → warn + fallback | Assert returns `DEFAULT_CONFIG` content | +| Integration | `init --template my.toml` | Temp dir, write template, run init, verify output | +| Integration | `init --wizard --template` | Verify wizard uses template as base | +| Integration | XDG auto-discovery | Set env, place file, run init without flag | +| Existing | `test_default_agents_md_contains_sections` | Unaffected — AGENTS.md unchanged | +| Existing | E2E `01-init-blank.sh` | Unaffected — no flag = same behavior | + +## Migration / Rollout + +No migration required. Purely additive feature — no flag means identical behavior to current code. + +## Open Questions + +- [x] Should `--template` work with `--wizard`? → YES, template becomes wizard base config +- [x] XDG file naming? → Fixed `config.toml`, any name via `--template` +- [ ] Should we validate template with full `Config::load` deserialization or just TOML parse? Recommendation: full `Config::load` to catch semantic errors (unknown agent names, invalid sync types). diff --git a/openspec/changes/init-user-template/exploration.md b/openspec/changes/init-user-template/exploration.md new file mode 100644 index 00000000..44731f4f --- /dev/null +++ b/openspec/changes/init-user-template/exploration.md @@ -0,0 +1,113 @@ +# Exploration: init --template and XDG user config + +## Current State + +`agentsync init` writes a hardcoded `DEFAULT_CONFIG` constant (src/init.rs:15, ~170 lines of TOML) containing all 7 agents enabled. Two code paths use it: + +1. **`init()`** (init.rs:197) — plain init, writes `DEFAULT_CONFIG` directly at line 244: `fs::write(&config_path, DEFAULT_CONFIG)` +2. **`init_wizard()`** (init.rs:1583) — interactive wizard. Falls back to `init()` if no files found (line 1667) or no files selected (lines 1686, 1695). When it proceeds, it calls `build_default_config_with_skills_modes()` (line 1738) which iterates `DEFAULT_CONFIG.lines()` and patches `type =` values per agent. The wizard also appends a layout block via `upsert_agent_config_layout_block()`. +3. **`init_wizard_experimental_tui()`** — just shows a TUI intro then delegates to `init_wizard()`. + +The `Init` command struct (main.rs:314) has 4 fields: `path`, `force`, `wizard`, `experimental_tui`. The handler (main.rs:398-426) branches on `wizard` flag. + +## Affected Areas + +- `src/main.rs:314-340` — Add `--template` arg to `Init` variant, pass it through handler (lines 398-419) +- `src/init.rs:197-261` — `init()` must accept optional template path, read+validate it, use instead of `DEFAULT_CONFIG` +- `src/init.rs:1075-1107` — `build_default_config_with_skills_modes()` takes `DEFAULT_CONFIG` as implicit base. Must accept a `&str` base parameter instead +- `src/init.rs:1583+` — `init_wizard()` calls `init()` as fallback (3 call sites: 1667, 1686, 1695) and `build_default_config_with_skills_modes()` at 1738. All need template threading +- `src/config.rs:290-298` — `Config::load()` already validates TOML → use it to validate user templates before writing +- `website/docs/src/content/docs/reference/cli.mdx:14-44` — Document `--template` flag and XDG fallback +- `Cargo.toml` — Possibly add `dirs` crate for XDG resolution + +## Approaches + +### 1. Minimal internal refactor — new `resolve_config_template()` function + +- Add a `fn resolve_config_template(template: Option<&Path>) -> Result` in init.rs that: + 1. If `--template` given: read file, validate with `Config::load` (parse as TOML), return content + 2. Else check `$XDG_CONFIG_HOME/agentsync/config.toml` then `~/.config/agentsync/config.toml` + 3. Else return `DEFAULT_CONFIG.to_string()` +- Thread resolved template string into `init()` and `build_default_config_with_skills_modes()` +- Pros: Single resolution point, clear precedence, minimal API change +- Cons: None significant +- Effort: **Low-Medium** + +### 2. Config struct method approach + +- Put resolution logic on `Config` or a new `TemplateResolver` struct +- Pros: More testable in isolation +- Cons: Over-engineering for what's essentially a string lookup +- Effort: **Medium** + +## Recommendation + +**Approach 1**. The change is localized. Key implementation: + +1. Add `template: Option` to `Init` in main.rs +2. Create `resolve_config_template(template: Option<&Path>) -> Result` in init.rs +3. Change `init(project_root, force)` signature to `init(project_root, force, config_content: &str)` +4. Change `build_default_config_with_skills_modes(modes)` to accept `base_config: &str` instead of reading `DEFAULT_CONFIG` +5. Thread through all 3 wizard fallback calls to `init()` and the direct `build_default_config_with_skills_modes()` call + +### XDG Resolution — no `dirs` crate needed + +`std::env::home_dir()` is deprecated since Rust 1.29 but still works. However, this project already uses `std::env::var("HOME")` patterns implicitly. The cleanest approach: + +```rust +fn user_config_path() -> Option { + if let Ok(xdg) = std::env::var("XDG_CONFIG_HOME") { + let p = PathBuf::from(xdg).join("agentsync/config.toml"); + if p.exists() { return Some(p); } + } + if let Ok(home) = std::env::var("HOME") { + let p = PathBuf::from(home).join(".config/agentsync/config.toml"); + if p.exists() { return Some(p); } + } + None +} +``` + +No new dependency needed. `$HOME` is reliable on macOS/Linux. Windows users would use `--template` explicitly. + +### Validation strategy + +Before writing the template content to disk, parse it with `toml::from_str::()` (same as `Config::load`). If it fails, error with: `"Invalid template: {path}: {toml_error}"`. This prevents writing broken configs. + +### Output messaging + +When using a template, change the init message: +- Default: `" ✔ Created: .agents/agentsync.toml"` +- Template: `" ✔ Created: .agents/agentsync.toml (from template: {path})"` +- XDG: `" ✔ Created: .agents/agentsync.toml (from user config: ~/.config/agentsync/config.toml)"` + +## Risks + +- **Wizard + template interaction**: The wizard's `build_default_config_with_skills_modes()` iterates lines looking for `[agents.X]` sections and `type = ` lines. If a user template has different agent names or structure, the skills mode patching will silently miss agents not in the template. This is actually **correct behavior** — it only patches what exists. +- **E2E test `01-init-blank.sh`**: Asserts specific agents exist in output (`[agents.claude]`, `[agents.gemini]`, `[agents.opencode]`). Won't break since default path is unchanged, but we need NEW tests for template path. +- **Backward compatibility**: Fully preserved — no flag = same behavior as today. + +## Test Strategy + +### Existing tests +- **E2E**: `tests/e2e/scenarios/01-init-blank.sh` (blank repo init), `02-init-adoption.sh` (wizard migration) +- **Unit**: `Config::load` tests in config.rs (lines 604-710) — file not found, invalid TOML, find_config precedence +- **No unit tests** for `init()`, `init_wizard()`, or `build_default_config_with_skills_modes()` + +### New tests needed +1. **Unit: `resolve_config_template`** — test precedence: explicit path > XDG > default +2. **Unit: `resolve_config_template` with invalid TOML** — expect error +3. **Unit: `build_default_config_with_skills_modes` with custom base** — verify it patches a non-default template correctly +4. **Integration: `init` with `--template`** — write a 2-agent template, run init, verify output matches template +5. **Integration: XDG fallback** — set `XDG_CONFIG_HOME` env var, place config, run init without `--template`, verify it's picked up +6. **E2E**: New scenario `03-init-template.sh` — end-to-end with template flag + +## Open Questions + +1. **Should `--template` work with `--wizard`?** Recommended: YES — template becomes the base config that the wizard patches. The wizard's skills mode selection still works because it patches `type =` lines in whatever template is provided. +2. **Template file naming**: Should we require the file to be named `config.toml` at XDG path, or accept any name? Recommendation: Fixed name `config.toml` at XDG, any name via `--template`. +3. **Should we print which source was used?** Recommended: YES, always show provenance in output for debuggability. + +## Ready for Proposal + +Yes — the approach is clear, risks are low, backward compat is preserved. The orchestrator should proceed to sdd-propose with Approach 1. diff --git a/openspec/changes/init-user-template/proposal.md b/openspec/changes/init-user-template/proposal.md new file mode 100644 index 00000000..ce2f4a53 --- /dev/null +++ b/openspec/changes/init-user-template/proposal.md @@ -0,0 +1,78 @@ +# Proposal: Init User Template + +## Intent + +Users who manage multiple repos need consistent agent configs. Today, `agentsync init` always writes a hardcoded 7-agent default, forcing manual edits every time. This change lets users define their preferred config once and reuse it via `--template` flag or XDG auto-discovery. + +GitHub issue: #478 + +## Scope + +### In Scope +- `--template ` CLI flag on `agentsync init` +- XDG auto-discovery fallback (`$XDG_CONFIG_HOME/agentsync/config.toml` → `~/.config/agentsync/config.toml`) +- Precedence: `--template` > XDG user config > hardcoded `DEFAULT_CONFIG` +- Template validation before writing (parse as TOML, reject invalid) +- Provenance messaging in output (show which source was used) +- Template support in both `init` and `init --wizard` paths +- Unit + integration tests for precedence, validation, error cases +- Documentation updates (cli.mdx, getting-started.mdx, configuration.mdx) + +### Out of Scope +- Partial/sparse config merge (template is full file replacement) +- `--default-agents` flag (covered by template mechanism) +- New crate dependencies (use `$HOME` + `$XDG_CONFIG_HOME` env vars directly) +- Windows XDG support (Windows users use `--template` explicitly) +- Template generation/scaffolding commands + +## Capabilities + +### New Capabilities +- `init-user-template`: User-level config template resolution for `agentsync init` — flag, XDG fallback, validation, and provenance output + +### Modified Capabilities +- `config-schema`: No schema changes — templates must conform to existing schema. Validation reuses `Config::load` parsing. + +## Approach + +Add `resolve_config_template(template: Option<&Path>) -> Result` in `init.rs` as the single resolution point. Thread the resolved string into `init()` and `build_default_config_with_skills_modes()` by changing their signatures to accept a `base_config: &str` parameter. XDG lookup uses `std::env::var` for `XDG_CONFIG_HOME` and `HOME` — no new dependencies. + +## Affected Areas + +| Area | Impact | Description | +|------|--------|-------------| +| `src/main.rs:314-340` | Modified | Add `template: Option` to `Init` command struct | +| `src/init.rs:197-261` | Modified | `init()` accepts config content param instead of using `DEFAULT_CONFIG` | +| `src/init.rs:1075-1107` | Modified | `build_default_config_with_skills_modes()` accepts `base_config: &str` | +| `src/init.rs:1583+` | Modified | `init_wizard()` threads template through 3 fallback calls + direct build call | +| `src/init.rs` (new fn) | New | `resolve_config_template()` + `user_config_path()` | +| `tests/` | New | Unit tests for resolution, integration tests for `--template` and XDG | +| `website/docs/.../cli.mdx` | Modified | Document `--template` flag and XDG behavior | +| `website/docs/.../getting-started.mdx` | Modified | Mention template option | +| `website/docs/.../configuration.mdx` | Modified | Add user template section | + +## Risks + +| Risk | Likelihood | Mitigation | +|------|------------|------------| +| Wizard line-patching misses agents not in template | Low | Correct behavior — only patches what exists. Document this. | +| User provides syntactically valid but semantically broken template | Low | Validate with `Config::load` (full deserialization), not just TOML parse | +| XDG path doesn't exist on CI/containers | Low | Graceful fallback to `DEFAULT_CONFIG` — no error if XDG file absent | + +## Rollback Plan + +Revert the `--template` field from `Init` struct and restore original `init()`/`build_default_config_with_skills_modes()` signatures. No data migration needed — templates are read-only inputs, nothing is persisted beyond the generated `agentsync.toml` (which is always overwritable via `init --force`). + +## Dependencies + +- None. No new crates. Uses existing `Config::load` for validation. + +## Success Criteria + +- [ ] `agentsync init --template my.toml` writes config from template with provenance message +- [ ] `agentsync init` without flag picks up `~/.config/agentsync/config.toml` when present +- [ ] `agentsync init` without flag or XDG file behaves identically to current behavior +- [ ] `agentsync init --wizard --template my.toml` uses template as wizard base +- [ ] Invalid template produces clear error with file path and parse error +- [ ] All existing E2E tests pass unchanged +- [ ] New unit + integration tests cover precedence chain and error cases diff --git a/openspec/changes/init-user-template/specs/init-user-template/spec.md b/openspec/changes/init-user-template/specs/init-user-template/spec.md new file mode 100644 index 00000000..317de770 --- /dev/null +++ b/openspec/changes/init-user-template/specs/init-user-template/spec.md @@ -0,0 +1,131 @@ +# Init User Template Specification + +## Purpose + +User-level config template resolution for `agentsync init` — CLI flag, XDG auto-discovery fallback, TOML validation, and provenance output. + +## Requirements + +### Requirement: REQ-01 — Template flag + +The system MUST accept a `--template ` flag on `agentsync init` that loads the specified file as the config template content. + +#### Scenario: init with --template flag and valid file + +- GIVEN a valid TOML file at `/tmp/my-config.toml` parseable as `Config` +- WHEN the user runs `agentsync init --template /tmp/my-config.toml` +- THEN `.agents/agentsync.toml` is written with the template file's content +- AND the output includes the template source path + +#### Scenario: init with --template flag and missing file + +- GIVEN no file exists at `/tmp/missing.toml` +- WHEN the user runs `agentsync init --template /tmp/missing.toml` +- THEN the command exits with a non-zero code +- AND the error message includes the path `/tmp/missing.toml` + +#### Scenario: init with --template flag and invalid TOML + +- GIVEN a file at `/tmp/bad.toml` containing invalid TOML or TOML not parseable as `Config` +- WHEN the user runs `agentsync init --template /tmp/bad.toml` +- THEN the command exits with a non-zero code +- AND the error message includes the file path and the parse error + +### Requirement: REQ-02 — XDG discovery + +When no `--template` flag is provided, the system MUST check `$XDG_CONFIG_HOME/agentsync/config.toml`, then `~/.config/agentsync/config.toml`. The first existing file is used as the template. + +#### Scenario: init without --template, XDG config exists + +- GIVEN `$XDG_CONFIG_HOME` is unset +- AND a valid config file exists at `~/.config/agentsync/config.toml` +- WHEN the user runs `agentsync init` +- THEN `.agents/agentsync.toml` is written with the XDG file's content +- AND the output indicates the XDG source + +#### Scenario: XDG_CONFIG_HOME env var set to custom path + +- GIVEN `$XDG_CONFIG_HOME` is set to `/tmp/custom-xdg` +- AND a valid config file exists at `/tmp/custom-xdg/agentsync/config.toml` +- WHEN the user runs `agentsync init` +- THEN `.agents/agentsync.toml` uses that file as template + +#### Scenario: HOME not set (graceful fallback) + +- GIVEN `$XDG_CONFIG_HOME` is unset and `$HOME` is unset +- AND no `--template` flag is provided +- WHEN the user runs `agentsync init` +- THEN `.agents/agentsync.toml` is written with `DEFAULT_CONFIG` + +### Requirement: REQ-03 — Precedence order + +The system MUST resolve templates in this order: `--template` > XDG discovery > `DEFAULT_CONFIG`. + +#### Scenario: --template overrides XDG config + +- GIVEN a valid XDG config at `~/.config/agentsync/config.toml` with 3 agents +- AND a valid template file at `/tmp/two-agents.toml` with 2 agents +- WHEN the user runs `agentsync init --template /tmp/two-agents.toml` +- THEN `.agents/agentsync.toml` contains the 2-agent config from `--template` + +#### Scenario: init without --template, no XDG config + +- GIVEN no XDG config file exists and no `--template` flag +- WHEN the user runs `agentsync init` +- THEN `.agents/agentsync.toml` is written with `DEFAULT_CONFIG` (7 agents) + +### Requirement: REQ-04 — Full file replacement + +Templates MUST be used as full file replacement. The system MUST NOT merge template content with `DEFAULT_CONFIG`. + +### Requirement: REQ-05 — Template validation + +Template files MUST be valid TOML parseable as `Config` via the existing deserialization. The system MUST error before writing if validation fails. + +### Requirement: REQ-06 — Wizard compatibility + +`--template` MUST work with `init --wizard`. The template becomes the base config that the wizard's skills-mode patching operates on. + +#### Scenario: init --wizard with --template + +- GIVEN a valid template at `/tmp/custom.toml` +- WHEN the user runs `agentsync init --wizard --template /tmp/custom.toml` +- THEN the wizard uses the template as its base config for skills-mode patching + +#### Scenario: init --wizard without --template, XDG exists + +- GIVEN a valid XDG config at `~/.config/agentsync/config.toml` +- WHEN the user runs `agentsync init --wizard` +- THEN the wizard uses the XDG config as its base + +### Requirement: REQ-07 — Provenance output + +When a template is used (via flag or XDG), the output message MUST indicate which source was used. + +### Requirement: REQ-08 — Missing template error + +If `--template` points to a nonexistent path, the system MUST exit with a clear error including the path. It MUST NOT fall back to XDG or `DEFAULT_CONFIG`. + +### Requirement: REQ-09 — No new dependencies + +This change MUST NOT add new crate dependencies. XDG resolution uses `std::env::var` only. + +### Requirement: REQ-10 — Backward compatibility + +Without `--template` flag and without an XDG config file, behavior MUST be identical to the current implementation. + +## Acceptance Criteria + +- All 10 requirements have passing tests (unit + integration) +- All existing E2E tests (`01-init-blank.sh`, `02-init-adoption.sh`) pass unchanged +- `cargo clippy` and `cargo fmt` pass +- `Cargo.toml` has no new dependencies +- CLI help text documents `--template` flag + +## Out of Scope + +- Partial/sparse config merge with `DEFAULT_CONFIG` +- `--default-agents` flag +- `dirs` crate or any new dependency for XDG resolution +- Windows-specific XDG path discovery +- Template generation or scaffolding commands diff --git a/openspec/changes/init-user-template/state.yaml b/openspec/changes/init-user-template/state.yaml new file mode 100644 index 00000000..bfcc000d --- /dev/null +++ b/openspec/changes/init-user-template/state.yaml @@ -0,0 +1,13 @@ +change: init-user-template +current_phase: verify-clean +completed: [explore, propose, spec, design, tasks, apply, verify, warning-fixes] +next: archive +updated: 2026-08-01 +verify_verdict: PASS +warnings_resolved: + - wizard+template integration test added (test_wizard_template_flow_end_to_end) + - documentation updated (cli.mdx, getting-started.mdx, configuration.mdx) +verification: + - cargo test --all-features: all pass (403+ tests) + - cargo clippy: clean + - pnpm docs:build: clean (14 pages) diff --git a/openspec/changes/init-user-template/tasks.md b/openspec/changes/init-user-template/tasks.md new file mode 100644 index 00000000..6602ec20 --- /dev/null +++ b/openspec/changes/init-user-template/tasks.md @@ -0,0 +1,52 @@ +# Tasks: Init User Template + +## Review Workload Forecast + +| Field | Value | +|-------|-------| +| Estimated changed lines | 250–350 | +| 400-line budget risk | Low | +| Chained PRs recommended | No | +| Suggested split | single PR | +| Delivery strategy | ask-on-risk | +| Chain strategy | single-pr | + +Decision needed before apply: No +Chained PRs recommended: No +Chain strategy: single-pr +400-line budget risk: Low + +## Phase 1: Core Resolution Functions (Foundation) + +- [x] 1.1 Add `resolve_user_config_path() -> Option` in `src/init.rs` — checks `$XDG_CONFIG_HOME/agentsync/config.toml` then `$HOME/.config/agentsync/config.toml` (REQ-02, REQ-09) +- [x] 1.2 Add `resolve_config_template(template_path: Option<&Path>) -> Result` in `src/init.rs` — precedence: explicit path → XDG → `DEFAULT_CONFIG`; hard-fail on explicit invalid, warn+fallback on XDG invalid (REQ-03, REQ-05, REQ-08) +- [x] 1.3 Write unit tests: explicit valid file returns content; explicit missing file errors with path; explicit invalid TOML errors with parse details; XDG found returns content; XDG invalid warns and returns `DEFAULT_CONFIG`; `HOME` unset skips gracefully; no XDG + no flag returns `DEFAULT_CONFIG` (REQ-02, REQ-03, REQ-05, REQ-08, REQ-09) + +## Phase 2: CLI Wiring + +- [x] 2.1 Add `template: Option` field with `#[arg(long)]` to `Init` struct in `src/main.rs` (~line 334) (REQ-01) +- [x] 2.2 Pass `template_path` through handler at lines 398–419 to `init()` and `init_wizard()` calls (REQ-01) + +## Phase 3: Init & Wizard Integration + +- [x] 3.1 Change `init()` signature to accept `config_content: &str`, replace `DEFAULT_CONFIG` usage at line 244 with param (REQ-04) +- [x] 3.2 In the handler, call `resolve_config_template()` before `init()`, pass resolved content (REQ-03) +- [x] 3.3 Add provenance output: `"(from template: {path})"` or `"(from user config: {path})"` after config write (REQ-07) +- [x] 3.4 Change `init_wizard()` and `init_wizard_experimental_tui()` signatures to accept `template_path: Option<&Path>` (REQ-06) +- [x] 3.5 Change `build_default_config_with_skills_modes()` to accept `base_config: &str` instead of using `DEFAULT_CONFIG.lines()` (REQ-06) +- [x] 3.6 Thread template through wizard's 3 fallback `init()` calls (lines 1667, 1686, 1695) and `build_default_config_with_skills_modes()` call (line 1738) (REQ-06) + +## Phase 4: Testing + +- [x] 4.1 Integration test: `init --template valid.toml` writes template content with provenance message (REQ-01, REQ-07, REQ-10) +- [x] 4.2 Integration test: `init` without flag + XDG file present → uses XDG content (REQ-02) +- [x] 4.3 Integration test: `init` without flag, no XDG → writes `DEFAULT_CONFIG` (backward compat) (REQ-10) +- [x] 4.4 Integration test: `--template` overrides XDG when both exist (REQ-03) +- [ ] 4.5 Integration test: `init --wizard --template` uses template as wizard base (REQ-06) +- [x] 4.6 Verify existing E2E `01-init-blank.sh` and `02-init-adoption.sh` pass unchanged (REQ-10) + +## Phase 5: Documentation + +- [ ] 5.1 Update `website/docs/src/content/docs/reference/cli.mdx`: document `--template` flag and XDG auto-discovery +- [ ] 5.2 Update `website/docs/src/content/docs/getting-started.mdx`: mention user template option +- [ ] 5.3 Update `website/docs/src/content/docs/reference/configuration.mdx`: add user-level template section diff --git a/openspec/changes/init-user-template/verify-report.md b/openspec/changes/init-user-template/verify-report.md new file mode 100644 index 00000000..e9c043ec --- /dev/null +++ b/openspec/changes/init-user-template/verify-report.md @@ -0,0 +1,85 @@ +# Verification Report: Init User Template + +**Change**: init-user-template +**Mode**: openspec +**Date**: 2026-08-01 + +## Completeness + +| Phase | Status | +|-------|--------| +| Tasks Phase 1 (Core Resolution) | ✅ 3/3 | +| Tasks Phase 2 (CLI Wiring) | ✅ 2/2 | +| Tasks Phase 3 (Init & Wizard Integration) | ✅ 6/6 | +| Tasks Phase 4 (Testing) | ⚠️ 5/6 (4.5 missing: wizard+template integration test) | +| Tasks Phase 5 (Documentation) | ❌ 0/3 | + +## Build & Test Evidence + +| Check | Result | +|-------|--------| +| `cargo test --all-features` | ✅ All pass (662 run, 6 ignored, 0 failed) | +| `cargo clippy --all-targets --all-features -- -D warnings` | ✅ No warnings | +| `Cargo.toml` new dependencies | ✅ None added | +| Unit tests for template resolution | ✅ 10/10 pass | +| Backward compat (existing integration tests) | ✅ `test_agent_adoption.rs` calls updated to new signature | + +## Requirements Compliance Matrix + +| REQ | Description | Code Location | Test Coverage | Verdict | +|-----|-------------|--------------|---------------|---------| +| REQ-01 | `--template` CLI flag | `src/main.rs:340-344` | `test_resolve_config_template_explicit_valid_file` | ✅ PASS | +| REQ-02 | XDG discovery | `src/init.rs:32-48` | `test_resolve_user_config_path_xdg_config_home`, `test_resolve_user_config_path_home_fallback`, `test_resolve_config_template_xdg_config_home_valid` | ✅ PASS | +| REQ-03 | Precedence order | `src/init.rs:56-96` | `test_resolve_config_template_flag_overrides_xdg` | ✅ PASS | +| REQ-04 | Full file replacement | `src/init.rs:328` (writes `config_content` directly) | `test_resolve_config_template_explicit_valid_file` | ✅ PASS | +| REQ-05 | Template validation | `src/init.rs:62-64` (full `Config` deser) | `test_resolve_config_template_explicit_invalid_toml` | ✅ PASS | +| REQ-06 | Wizard compatibility | `src/init.rs:1749,1845` | No integration test (task 4.5 incomplete) | ⚠️ PARTIAL | +| REQ-07 | Provenance output | `src/main.rs:426-438`, `src/init.rs:1750-1762` | No test asserting output text | ⚠️ PARTIAL | +| REQ-08 | Missing template error | `src/init.rs:59-60` | `test_resolve_config_template_explicit_missing_file` | ✅ PASS | +| REQ-09 | No new dependencies | `Cargo.toml` unchanged | git diff confirms | ✅ PASS | +| REQ-10 | Backward compatibility | `src/init.rs:95` returns `DEFAULT_CONFIG` | `test_resolve_config_template_no_flag_no_xdg_returns_default`, existing integration tests pass | ✅ PASS | + +## Scenario Coverage Matrix + +| Scenario | Test | Status | +|----------|------|--------| +| init with --template flag and valid file | `test_resolve_config_template_explicit_valid_file` | ✅ COVERED | +| init with --template flag and missing file | `test_resolve_config_template_explicit_missing_file` | ✅ COVERED | +| init with --template flag and invalid TOML | `test_resolve_config_template_explicit_invalid_toml` | ✅ COVERED | +| init without --template, XDG config exists | `test_resolve_config_template_xdg_config_home_valid` | ✅ COVERED | +| XDG_CONFIG_HOME env var set to custom path | `test_resolve_user_config_path_xdg_config_home` | ✅ COVERED | +| HOME not set (graceful fallback) | `test_resolve_user_config_path_none_when_no_env` | ✅ COVERED | +| --template overrides XDG config | `test_resolve_config_template_flag_overrides_xdg` | ✅ COVERED | +| init without --template, no XDG config | `test_resolve_config_template_no_flag_no_xdg_returns_default` | ✅ COVERED | +| init --wizard with --template | No test | ⚠️ NOT_COVERED | +| init --wizard without --template, XDG exists | No test | ⚠️ NOT_COVERED | +| XDG invalid warns and falls back | `test_resolve_config_template_xdg_invalid_warns_and_falls_back` | ✅ COVERED | + +## Design Coherence + +| Decision | Implementation | Coherent? | +|----------|---------------|-----------| +| Single `resolve_config_template()` fn | ✅ `src/init.rs:56` | ✅ | +| No `dirs` crate — `std::env::var` only | ✅ Lines 33, 39 | ✅ | +| Warn-and-fallback on invalid XDG | ✅ `tracing::warn!` at lines 77, 85 | ✅ | +| Pass `base_config: &str` to `build_default_config_with_skills_modes` | ✅ Line 1160 | ✅ | +| `TemplateSource` enum for provenance | ✅ Lines 16-20 | ✅ | +| Full `Config` deserialization for validation | ✅ `toml::from_str::` at line 63 | ✅ | + +## Issues + +| # | Finding | Severity | Details | +|---|---------|----------|---------| +| 1 | Task 4.5: No integration test for `--wizard --template` | WARNING | Wizard + template threading is wired (line 1749, 1845) but no test exercises the full path. Code review confirms correct wiring. | +| 2 | Tasks 5.1-5.3: Documentation not updated | WARNING | CLI help text exists (main.rs:342), but website docs not updated. Non-blocking for functionality. | +| 3 | REQ-07 provenance output not asserted by test | SUGGESTION | Provenance print exists in both `main.rs` and `init_wizard`, but no test captures stdout to verify. Unit tests validate `TemplateSource` enum correctness which is the data backing provenance. | + +## Verdict + +### **PASS WITH WARNINGS** + +All 10 requirements are implemented. 8/10 have direct test coverage. Core resolution logic has comprehensive unit tests (10 tests covering all precedence paths, error cases, and env var combinations). Full test suite passes (662 tests). Clippy clean. No new dependencies. Backward compatibility preserved. + +**Warnings**: +- Missing integration test for wizard + template path (task 4.5) — code is correctly wired, low risk. +- Documentation tasks (5.1-5.3) incomplete — non-blocking, can be done in follow-up. diff --git a/src/banner.txt b/src/banner.txt index cdab5ef9..c0c9383a 100644 --- a/src/banner.txt +++ b/src/banner.txt @@ -1,6 +1,14 @@ -_______ _____________ -___ |______ ______________ /__ ___/____ _______________ -__ /| |_ __ `/ _ \_ __ \ __/____ \__ / / /_ __ \ ___/ -_ ___ | /_/ // __/ / / / /_ ____/ /_ /_/ /_ / / / /__ -/_/ |_|\__, / \___//_/ /_/\__/ /____/ _\__, / /_/ /_/\___/ - /____/ /____/ +╔═══════════════════════════════════════════════════════════════════════════════════════════╗ +║ ║ +║ █████╗ ██████╗ ███████╗███╗ ██╗████████╗███████╗██╗ ██╗███╗ ██╗ ██████╗ ║ +║ ██╔══██╗██╔════╝ ██╔════╝████╗ ██║╚══██╔══╝██╔════╝╚██╗ ██╔╝████╗ ██║██╔════╝ ║ +║ ███████║██║ ███╗█████╗ ██╔██╗ ██║ ██║ ███████╗ ╚████╔╝ ██╔██╗ ██║██║ ║ +║ ██╔══██║██║ ██║██╔══╝ ██║╚██╗██║ ██║ ╚════██║ ╚██╔╝ ██║╚██╗██║██║ ║ +║ ██║ ██║╚██████╔╝███████╗██║ ╚████║ ██║ ███████║ ██║ ██║ ╚████║╚██████╗ ║ +║ ╚═╝ ╚═╝ ╚═════╝ ╚══════╝╚═╝ ╚═══╝ ╚═╝ ╚══════╝ ╚═╝ ╚═╝ ╚═══╝ ╚═════╝ ║ +║ ║ +║ Agent Synchronization Framework for AI Ecosystems ║ +║ ║ +║ [SYNC] Context │ [AGENTS] Multi-Agent │ [PLUGIN] Extensible │ [OSS] ║ +║ ║ +╚═══════════════════════════════════════════════════════════════════════════════════════════╝ \ No newline at end of file diff --git a/src/commands/doctor.rs b/src/commands/doctor.rs index 572771b5..e9ca6bcc 100644 --- a/src/commands/doctor.rs +++ b/src/commands/doctor.rs @@ -16,58 +16,26 @@ pub struct MissingSourceIssue { pub path: PathBuf, } -pub fn run_doctor(project_root: PathBuf) -> Result<()> { - println!("{}", "🩺 Running AgentSync Diagnostic...".bold().cyan()); - - let mut issues = 0; - - // 1. Config Loading & Validation - let config_path = match agentsync::config::Config::find_config(&project_root) { - Ok(path) => { - println!( - " {} Found config: {}", - "✔".green(), - path.display().to_string().dimmed() - ); - path - } - Err(e) => { - println!(" {} Could not find config: {}", "✗".red(), e); - return Ok(()); - } - }; - - let config = match agentsync::config::Config::load(&config_path) { - Ok(c) => { - println!(" {} Config loaded successfully", "✔".green()); - c - } - Err(e) => { - println!(" {} Failed to parse config: {}", "✗".red(), e); - return Ok(()); - } - }; - - let linker = Linker::new(config, config_path.clone()); - let source_dir = linker.config().source_dir(&config_path); - - // 2. Source Directory Check +fn check_source_directory(source_dir: &Path) -> usize { if !source_dir.exists() { println!( " {} Source directory does not exist: {}", "✗".red(), source_dir.display() ); - issues += 1; + 1 } else { println!( " {} Source directory exists: {}", "✔".green(), source_dir.display().to_string().dimmed() ); + 0 } +} - // 3. Target Source Existence Check +fn check_target_sources(linker: &Linker, source_dir: &Path) -> usize { + let mut issues = 0; let mut missing_targets = 0; for (agent_name, agent) in &linker.config().agents { if !agent.enabled { @@ -86,7 +54,7 @@ pub fn run_doctor(project_root: PathBuf) -> Result<()> { } for missing in collect_missing_sources( - &source_dir, + source_dir, linker.project_root(), agent_name, target_name, @@ -116,7 +84,7 @@ pub fn run_doctor(project_root: PathBuf) -> Result<()> { if let Some(mismatch) = collect_skills_mode_mismatch( linker.project_root(), - &source_dir, + source_dir, agent_name, target_name, target, @@ -129,9 +97,12 @@ pub fn run_doctor(project_root: PathBuf) -> Result<()> { if missing_targets == 0 { println!(" {} All target sources exist", "✔".green()); } + issues +} - // 4. Destination Path Conflict Check - let mut destinations: Vec<(String, String, String)> = Vec::new(); // (path, agent, target) +fn check_destination_conflicts(linker: &Linker) -> usize { + let mut issues = 0; + let mut destinations: Vec<(String, String, String)> = Vec::new(); for (agent_name, agent) in &linker.config().agents { if !agent.enabled { continue; @@ -141,8 +112,7 @@ pub fn run_doctor(project_root: PathBuf) -> Result<()> { } } - let conflict_results = validate_destinations(&destinations); - for conflict in conflict_results { + for conflict in validate_destinations(&destinations) { match conflict { Conflict::Duplicate(dest) => { println!( @@ -165,117 +135,174 @@ pub fn run_doctor(project_root: PathBuf) -> Result<()> { } } } + issues +} - // 5. MCP Server Audit - if linker.config().mcp.enabled { - for (name, server) in &linker.config().mcp_servers { - if server.disabled { - continue; - } - if let Some(cmd) = &server.command { - if !command_exists(cmd) { - println!( - " {} MCP server {} command not found in PATH: {}", - "✗".red(), - name.bold(), - cmd.bold() - ); - issues += 1; - } else { - println!( - " {} MCP server {} command executable: {}", - "✔".green(), - name.bold(), - cmd.dimmed() - ); - } +fn check_mcp_servers(linker: &Linker) -> usize { + let mut issues = 0; + if !linker.config().mcp.enabled { + return 0; + } + for (name, server) in &linker.config().mcp_servers { + if server.disabled { + continue; + } + if let Some(cmd) = &server.command { + if !command_exists(cmd) { + println!( + " {} MCP server {} command not found in PATH: {}", + "✗".red(), + name.bold(), + cmd.bold() + ); + issues += 1; } else { println!( - " {} MCP server {} has no command configured (not audited)", - "ℹ".blue(), - name.bold() + " {} MCP server {} command executable: {}", + "✔".green(), + name.bold(), + cmd.dimmed() ); } + } else { + println!( + " {} MCP server {} has no command configured (not audited)", + "ℹ".blue(), + name.bold() + ); } } + issues +} - // 6. .gitignore Audit +fn check_gitignore(linker: &Linker) -> usize { let gitignore_path = linker.project_root().join(".gitignore"); - if gitignore_path.exists() { - match fs::read_to_string(&gitignore_path) { - Ok(content) => { - let marker = &linker.config().gitignore.marker; - let (start_marker, end_marker) = agentsync::gitignore::managed_markers(marker); - let (has_start_marker, has_end_marker) = - parse_markers(&content, &start_marker, &end_marker); - let has_managed_section = has_start_marker && has_end_marker; - - if gitignore_missing_section_is_issue( - linker.config().gitignore.enabled, - &content, - &start_marker, - &end_marker, - ) { - println!( - " {} .gitignore managed section missing (Marker: {})", - "⚠".yellow(), - marker - ); - issues += 1; - } else if has_managed_section { - // Audit entries - let managed_entries = - extract_managed_entries(&content, &start_marker, &end_marker); - let required_entries: HashSet = linker - .config() - .all_gitignore_entries() - .into_iter() - .collect(); - let actual_entries: HashSet = managed_entries.into_iter().collect(); - - let missing: Vec<_> = required_entries.difference(&actual_entries).collect(); - let extra: Vec<_> = actual_entries.difference(&required_entries).collect(); - - if !missing.is_empty() { - println!( - " {} .gitignore missing {} managed entries", - "✗".red(), - missing.len() - ); - for m in &missing { - println!(" - {}", m); - } - issues += 1; - } - if !extra.is_empty() { - println!( - " {} .gitignore has {} extra entries in managed section", - "⚠".yellow(), - extra.len() - ); - issues += 1; - } + if !gitignore_path.exists() { + if linker.config().gitignore.enabled { + println!(" {} .gitignore file not found", "⚠".yellow()); + return 1; + } + return 0; + } - if missing.is_empty() && extra.is_empty() { - println!(" {} .gitignore managed section is up to date", "✔".green()); - } - } - } - Err(e) => { - println!(" {} Failed to read .gitignore: {}", "✗".red(), e); - issues += 1; - } + let content = match fs::read_to_string(&gitignore_path) { + Ok(c) => c, + Err(e) => { + println!(" {} Failed to read .gitignore: {}", "✗".red(), e); + return 1; + } + }; + + let mut issues = 0; + let marker = &linker.config().gitignore.marker; + let (start_marker, end_marker) = agentsync::gitignore::managed_markers(marker); + let (has_start_marker, has_end_marker) = parse_markers(&content, &start_marker, &end_marker); + + // Check for missing/mismatched managed section using already-parsed marker results + let section_is_issue = match (has_start_marker, has_end_marker) { + (true, true) => false, + (false, false) => linker.config().gitignore.enabled, + _ => true, // mismatched markers + }; + + if section_is_issue { + println!( + " {} .gitignore managed section missing (Marker: {})", + "⚠".yellow(), + marker + ); + return 1; + } + + if !(has_start_marker && has_end_marker) { + return 0; + } + + let managed_entries = extract_managed_entries(&content, &start_marker, &end_marker); + let required_entries: HashSet = linker + .config() + .all_gitignore_entries() + .into_iter() + .collect(); + let actual_entries: HashSet = managed_entries.into_iter().collect(); + + let missing: Vec<_> = required_entries.difference(&actual_entries).collect(); + let extra: Vec<_> = actual_entries.difference(&required_entries).collect(); + + if !missing.is_empty() { + println!( + " {} .gitignore missing {} managed entries", + "✗".red(), + missing.len() + ); + for m in &missing { + println!(" - {}", m); } - } else if linker.config().gitignore.enabled { - println!(" {} .gitignore file not found", "⚠".yellow()); - issues += 1; + issues += missing.len(); + } + if !extra.is_empty() { + println!( + " {} .gitignore has {} extra entries in managed section", + "⚠".yellow(), + extra.len() + ); + issues += extra.len(); } - // 7. Unmanaged Claude Skills Check + if missing.is_empty() && extra.is_empty() { + println!(" {} .gitignore managed section is up to date", "✔".green()); + } + issues +} + +fn check_unmanaged_skills(linker: &Linker) -> usize { if let Some(warning) = check_unmanaged_claude_skills(linker.project_root(), linker.config()) { println!(" {} {}", "⚠".yellow(), warning); - issues += 1; + 1 + } else { + 0 } +} + +pub fn run_doctor(project_root: PathBuf) -> Result<()> { + println!("{}", "🩺 Running AgentSync Diagnostic...".bold().cyan()); + + let config_path = match agentsync::config::Config::find_config(&project_root) { + Ok(path) => { + println!( + " {} Found config: {}", + "✔".green(), + path.display().to_string().dimmed() + ); + path + } + Err(e) => { + println!(" {} Could not find config: {}", "✗".red(), e); + return Ok(()); + } + }; + + let config = match agentsync::config::Config::load(&config_path) { + Ok(c) => { + println!(" {} Config loaded successfully", "✔".green()); + c + } + Err(e) => { + println!(" {} Failed to parse config: {}", "✗".red(), e); + return Ok(()); + } + }; + + let linker = Linker::new(config, config_path.clone()); + let source_dir = linker.config().source_dir(&config_path); + + let mut issues = 0; + issues += check_source_directory(&source_dir); + issues += check_target_sources(&linker, &source_dir); + issues += check_destination_conflicts(&linker); + issues += check_mcp_servers(&linker); + issues += check_gitignore(&linker); + issues += check_unmanaged_skills(&linker); if issues == 0 { println!("\n{}", "✨ All systems go! No issues found.".green().bold()); @@ -317,6 +344,7 @@ pub(crate) fn parse_markers(content: &str, start_marker: &str, end_marker: &str) (has_start, has_end) } +#[cfg(test)] pub(crate) fn gitignore_missing_section_is_issue( gitignore_enabled: bool, content: &str, @@ -632,3 +660,49 @@ pub fn extract_managed_entries(content: &str, start_marker: &str, end_marker: &s entries } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_check_source_directory_exists() { + let tmp = tempfile::tempdir().unwrap(); + assert_eq!(check_source_directory(tmp.path()), 0); + } + + #[test] + fn test_check_source_directory_missing() { + let tmp = tempfile::tempdir().unwrap(); + let missing = tmp.path().join("does_not_exist"); + assert_eq!(check_source_directory(&missing), 1); + } + + #[test] + fn test_extract_managed_entries_basic() { + let content = "# comment\n## START\nfoo\nbar\n## END\n"; + let entries = extract_managed_entries(content, "## START", "## END"); + assert_eq!(entries, vec!["foo", "bar"]); + } + + #[test] + fn test_extract_managed_entries_skips_comments() { + let content = "## START\n# comment\nentry\n## END\n"; + let entries = extract_managed_entries(content, "## START", "## END"); + assert_eq!(entries, vec!["entry"]); + } + + #[test] + fn test_extract_managed_entries_empty_section() { + let content = "## START\n## END\n"; + let entries = extract_managed_entries(content, "## START", "## END"); + assert!(entries.is_empty()); + } + + #[test] + fn test_extract_managed_entries_no_markers() { + let content = "just some text\n"; + let entries = extract_managed_entries(content, "## START", "## END"); + assert!(entries.is_empty()); + } +} diff --git a/src/commands/doctor_tests.rs b/src/commands/doctor_tests.rs index 62769d67..8b60805e 100644 --- a/src/commands/doctor_tests.rs +++ b/src/commands/doctor_tests.rs @@ -607,6 +607,69 @@ entry1 assert!(result.is_none()); } + // ========================================================================== + // PARSE_MARKERS TESTS + // ========================================================================== + + use crate::commands::doctor::parse_markers; + + #[test] + fn test_parse_markers_both_present() { + let content = "line1\n# START managed\nentry\n# END managed\nline2\n"; + let (has_start, has_end) = parse_markers(content, "# START managed", "# END managed"); + assert!(has_start); + assert!(has_end); + } + + #[test] + fn test_parse_markers_only_start() { + let content = "line1\n# START managed\nentry\nline2\n"; + let (has_start, has_end) = parse_markers(content, "# START managed", "# END managed"); + assert!(has_start); + assert!(!has_end); + } + + #[test] + fn test_parse_markers_only_end() { + let content = "line1\nentry\n# END managed\nline2\n"; + let (has_start, has_end) = parse_markers(content, "# START managed", "# END managed"); + assert!(!has_start); + assert!(has_end); + } + + #[test] + fn test_parse_markers_neither_present() { + let content = "line1\nline2\nline3\n"; + let (has_start, has_end) = parse_markers(content, "# START managed", "# END managed"); + assert!(!has_start); + assert!(!has_end); + } + + #[test] + fn test_parse_markers_with_surrounding_whitespace() { + // Markers with leading/trailing whitespace on the line should still match (trim) + let content = " # START managed \nentry\n # END managed \n"; + let (has_start, has_end) = parse_markers(content, "# START managed", "# END managed"); + assert!(has_start); + assert!(has_end); + } + + #[test] + fn test_parse_markers_partial_match_not_counted() { + // A line containing the marker as substring but not the full trimmed line + let content = "prefix # START managed suffix\nentry\n# END managed\n"; + let (has_start, has_end) = parse_markers(content, "# START managed", "# END managed"); + assert!(!has_start); // not an exact trimmed match + assert!(has_end); + } + + #[test] + fn test_parse_markers_empty_content() { + let (has_start, has_end) = parse_markers("", "# START", "# END"); + assert!(!has_start); + assert!(!has_end); + } + #[test] #[cfg(unix)] fn test_collect_skills_mode_mismatch_reports_directory_symlink_vs_symlink_contents() { diff --git a/src/commands/skill.rs b/src/commands/skill.rs index def18945..4fb6f301 100644 --- a/src/commands/skill.rs +++ b/src/commands/skill.rs @@ -745,178 +745,197 @@ pub fn run_skill(cmd: SkillCommand, project_root: PathBuf) -> Result<()> { pub fn run_suggest(args: SkillSuggestArgs, project_root: PathBuf) -> Result<()> { let service = SuggestionService; - let result = (|| -> Result<()> { - let response = service.suggest(&project_root)?; - let output_mode = suggest_install_output_mode(args.json); + let result = run_suggest_inner(&args, &project_root, &service); - if !args.install { - if args.json { - println!("{}", serde_json::to_string(&response.to_json_response())?); - } else { - let use_color = match output::output_mode(false) { - OutputMode::Human { use_color } => use_color, - OutputMode::Json => false, - }; - println!("{}", render_skill_suggest_human(&response, use_color)); - } - return Ok(()); + match result { + Ok(()) => Ok(()), + Err(error) => handle_suggest_error(&args, error), + } +} + +fn run_suggest_inner( + args: &SkillSuggestArgs, + project_root: &Path, + service: &SuggestionService, +) -> Result<()> { + let response = service.suggest(project_root)?; + let output_mode = suggest_install_output_mode(args.json); + + if !args.install { + return print_suggest_output(args.json, &response); + } + + let provider = SuggestInstallProvider::default(); + let install_response = run_suggest_install( + args, + project_root, + service, + &response, + &provider, + output_mode, + )?; + + match output_mode { + SuggestInstallOutputMode::Json => { + println!("{}", serde_json::to_string(&install_response)?); + } + SuggestInstallOutputMode::HumanLine { use_color } + | SuggestInstallOutputMode::HumanLive { use_color } => { + println!( + "{}", + render_suggest_install_completion_summary(&install_response, use_color) + ); } + } - let provider = SuggestInstallProvider::default(); - let install_response = match output_mode { - SuggestInstallOutputMode::Json => { - if args.all { - service.install_all_with(&project_root, &response, &provider) - } else { - ensure_interactive_install_supported()?; - let selected_skill_ids = prompt_for_recommended_skills(&response)?; - service.install_selected_with( - &project_root, - &response, - &provider, - SuggestInstallMode::Interactive, - &selected_skill_ids, - |skill_id, source, target_root| { - agentsync::skills::install::blocking_fetch_and_install_skill( - skill_id, - source, - target_root, - ) - .map_err(|error| anyhow::anyhow!(error)) - }, - ) - } - } - SuggestInstallOutputMode::HumanLine { use_color } - | SuggestInstallOutputMode::HumanLive { use_color } => { - let (mode, selected_skill_ids) = if args.all { - ( - SuggestInstallMode::InstallAll, - response - .recommendations - .iter() - .map(|recommendation| recommendation.skill_id.clone()) - .collect::>(), - ) - } else { - ensure_interactive_install_supported()?; - ( - SuggestInstallMode::Interactive, - prompt_for_recommended_skills(&response)?, - ) - }; + Ok(()) +} - print_suggest_install_batch_start(mode, selected_skill_ids.len(), use_color); - match output_mode { - SuggestInstallOutputMode::HumanLine { .. } => { - let mut reporter = SuggestInstallLineReporter::new(use_color); - service.install_selected_with_reporter( - &project_root, - &response, - &provider, - mode, - &selected_skill_ids, - &mut reporter, - |skill_id, source, target_root| { - agentsync::skills::install::blocking_fetch_and_install_skill( - skill_id, - source, - target_root, - ) - .map_err(|error| anyhow::anyhow!(error)) - }, - ) - } - SuggestInstallOutputMode::HumanLive { .. } => { - let mut reporter = SuggestInstallLiveReporter::new(use_color); - let result = service.install_selected_with_reporter( - &project_root, - &response, - &provider, - mode, - &selected_skill_ids, - &mut reporter, - |skill_id, source, target_root| { - agentsync::skills::install::blocking_fetch_and_install_skill( - skill_id, - source, - target_root, - ) - .map_err(|error| anyhow::anyhow!(error)) - }, - ); - reporter.finalize(); - result - } - SuggestInstallOutputMode::Json => unreachable!(), - } - } - }?; +fn print_suggest_output(json: bool, response: &SuggestResponse) -> Result<()> { + if json { + println!("{}", serde_json::to_string(&response.to_json_response())?); + } else { + let use_color = match output::output_mode(false) { + OutputMode::Human { use_color } => use_color, + OutputMode::Json => false, + }; + println!("{}", render_skill_suggest_human(response, use_color)); + } + Ok(()) +} - match output_mode { - SuggestInstallOutputMode::Json => { - // In JSON mode, include failure information in the output but don't fail the command - // since the response contains detailed results that consumers can inspect - println!("{}", serde_json::to_string(&install_response)?); - } - SuggestInstallOutputMode::HumanLine { use_color } - | SuggestInstallOutputMode::HumanLive { use_color } => { - println!( - "{}", - render_suggest_install_completion_summary(&install_response, use_color) - ); - } +fn run_suggest_install( + args: &SkillSuggestArgs, + project_root: &Path, + service: &SuggestionService, + response: &SuggestResponse, + provider: &SuggestInstallProvider, + output_mode: SuggestInstallOutputMode, +) -> Result { + match output_mode { + SuggestInstallOutputMode::Json => { + let (mode, selected_skill_ids) = resolve_install_mode_and_ids(args, response)?; + let mut reporter = NoopInstallReporter; + service.install_selected_with_reporter( + project_root, + response, + provider, + mode, + &selected_skill_ids, + &mut reporter, + install_skill_callback, + ) + } + SuggestInstallOutputMode::HumanLine { use_color } => { + let (mode, selected_skill_ids) = resolve_install_mode_and_ids(args, response)?; + print_suggest_install_batch_start(mode, selected_skill_ids.len(), use_color); + let mut reporter = SuggestInstallLineReporter::new(use_color); + service.install_selected_with_reporter( + project_root, + response, + provider, + mode, + &selected_skill_ids, + &mut reporter, + install_skill_callback, + ) + } + SuggestInstallOutputMode::HumanLive { use_color } => { + let (mode, selected_skill_ids) = resolve_install_mode_and_ids(args, response)?; + print_suggest_install_batch_start(mode, selected_skill_ids.len(), use_color); + let mut reporter = SuggestInstallLiveReporter::new(use_color); + let result = service.install_selected_with_reporter( + project_root, + response, + provider, + mode, + &selected_skill_ids, + &mut reporter, + install_skill_callback, + ); + reporter.finalize(); + result } + } +} - Ok(()) - })(); +/// Reusable install callback that bridges into the blocking skill installer. +fn install_skill_callback(skill_id: &str, source: &str, target_root: &Path) -> Result<()> { + agentsync::skills::install::blocking_fetch_and_install_skill(skill_id, source, target_root) + .map_err(|error| anyhow::anyhow!(error)) +} - match result { - Ok(()) => Ok(()), - Err(error) => { - let error_message = error.to_string(); - let (code, remediation) = if error_message - .contains("not part of the current recommendation set") - { - ( - "invalid_suggestion_selection", - "Run 'agentsync skill suggest --json' to inspect available recommended skill ids.", - ) - } else if error_message.contains("interactive terminal") { - ( - "interactive_tty_required", - "Run 'agentsync skill suggest --install --all' for a non-interactive install path.", - ) - } else if args.install { - ("install_error", remediation_for_error(&error_message)) - } else { - ( - "suggest_error", - "Verify the project root is readable and try again. Use --project-root to point to the repository you want to inspect.", - ) - }; +/// No-op reporter for JSON output mode (results collected, no progress display). +struct NoopInstallReporter; - if args.json { - let output = serde_json::json!({ - "error": error_message, - "code": code, - "remediation": remediation, - }); - println!("{}", serde_json::to_string(&output)?); - } else { - error!(%code, error = %error_message, "Suggest failed"); - let use_color = match output::output_mode(false) { - OutputMode::Human { use_color } => use_color, - OutputMode::Json => false, - }; - for line in render_skill_command_error(&error_message, remediation, use_color) { - println!("{line}"); - } - } +impl SuggestInstallProgressReporter for NoopInstallReporter { + fn on_event(&mut self, _event: SuggestInstallProgressEvent) {} +} + +fn resolve_install_mode_and_ids( + args: &SkillSuggestArgs, + response: &SuggestResponse, +) -> Result<(SuggestInstallMode, Vec)> { + if args.all { + Ok(( + SuggestInstallMode::InstallAll, + response + .recommendations + .iter() + .map(|r| r.skill_id.clone()) + .collect(), + )) + } else { + ensure_interactive_install_supported()?; + Ok(( + SuggestInstallMode::Interactive, + prompt_for_recommended_skills(response)?, + )) + } +} + +fn handle_suggest_error(args: &SkillSuggestArgs, error: anyhow::Error) -> Result<()> { + let error_message = error.to_string(); + let (code, remediation) = if error_message + .contains("not part of the current recommendation set") + { + ( + "invalid_suggestion_selection", + "Run 'agentsync skill suggest --json' to inspect available recommended skill ids.", + ) + } else if error_message.contains("interactive terminal") { + ( + "interactive_tty_required", + "Run 'agentsync skill suggest --install --all' for a non-interactive install path.", + ) + } else if args.install { + ("install_error", remediation_for_error(&error_message)) + } else { + ( + "suggest_error", + "Verify the project root is readable and try again. Use --project-root to point to the repository you want to inspect.", + ) + }; - Err(error) + if args.json { + let output = serde_json::json!({ + "error": error_message, + "code": code, + "remediation": remediation, + }); + println!("{}", serde_json::to_string(&output)?); + } else { + error!(%code, error = %error_message, "Suggest failed"); + let use_color = match output::output_mode(false) { + OutputMode::Human { use_color } => use_color, + OutputMode::Json => false, + }; + for line in render_skill_command_error(&error_message, remediation, use_color) { + println!("{line}"); } } + + Err(error) } fn ensure_interactive_install_supported() -> Result<()> { @@ -1252,27 +1271,22 @@ fn infer_install_source_format(source: &str) -> String { /// /// Returns `None` if the URL is not a GitHub URL or already points to an archive. fn try_convert_github_url(url: &str) -> Option { - // Parse the URL to properly handle query strings and fragments let parsed = url::Url::parse(url).ok()?; - // Check if it's already an archive URL by examining the path component let path = parsed.path(); if path.ends_with(".zip") || path.ends_with(".tar.gz") || path.ends_with(".tgz") { return None; } - // Only process github.com URLs if parsed.host_str() != Some("github.com") { return None; } - // Get the path segments let segments: Vec<&str> = parsed .path_segments() .map(|s| s.collect()) .unwrap_or_default(); - // Minimum: owner/repo (at least 2 segments) if segments.len() < 2 { return None; } @@ -1280,62 +1294,54 @@ fn try_convert_github_url(url: &str) -> Option { let owner = segments[0]; let repo = segments[1]; - // Check if it's a tree or blob URL with subpath if segments.len() >= 4 && (segments[2] == "tree" || segments[2] == "blob") { - let branch = segments[3]; - // The rest is the path within the repo - let subpath = segments[4..].join("/"); - - // If it's a blob URL pointing to a file, get the parent directory - let final_subpath = if segments[2] == "blob" { - if subpath.contains('/') { - // Remove the filename to get the directory - let path_parts: Vec<&str> = subpath.split('/').collect(); - if path_parts.len() > 1 { - path_parts[..path_parts.len() - 1].join("/") - } else { - subpath - } - } else { - // Blob pointing to a file at repo root (e.g., README.md) - // Return empty string so no fragment is added - String::new() - } - } else { - subpath - }; - - let mut zip_url = format!( - "https://github.com/{}/{}/archive/refs/heads/{}.zip", - owner, repo, branch - ); - - if !final_subpath.is_empty() { - zip_url.push('#'); - zip_url.push_str(&final_subpath); - } - - return Some(zip_url); + return convert_github_tree_blob_url(owner, repo, &segments); } - // Simple repo URL: github.com/owner/repo - if segments.len() == 2 { + if segments.len() == 2 || (segments.len() > 2 && segments[2].is_empty()) { return Some(format!( "https://github.com/{}/{}/archive/HEAD.zip", owner, repo )); } - // Repo URL with trailing segments but not tree/blob - // e.g., github.com/owner/repo/ (with trailing slash) - if segments.len() > 2 && segments[2].is_empty() { - return Some(format!( - "https://github.com/{}/{}/archive/HEAD.zip", - owner, repo - )); + None +} + +fn convert_github_tree_blob_url(owner: &str, repo: &str, segments: &[&str]) -> Option { + let branch = segments[3]; + let subpath = segments[4..].join("/"); + + let final_subpath = if segments[2] == "blob" { + resolve_blob_subpath(&subpath) + } else { + subpath + }; + + let mut zip_url = format!( + "https://github.com/{}/{}/archive/refs/heads/{}.zip", + owner, repo, branch + ); + + if !final_subpath.is_empty() { + zip_url.push('#'); + zip_url.push_str(&final_subpath); } - None + Some(zip_url) +} + +fn resolve_blob_subpath(subpath: &str) -> String { + if subpath.contains('/') { + let path_parts: Vec<&str> = subpath.split('/').collect(); + if path_parts.len() > 1 { + path_parts[..path_parts.len() - 1].join("/") + } else { + subpath.to_string() + } + } else { + String::new() + } } fn remediation_for_error(msg: &str) -> &str { diff --git a/src/commands/status.rs b/src/commands/status.rs index 20bbc78a..bcbde9fa 100644 --- a/src/commands/status.rs +++ b/src/commands/status.rs @@ -467,6 +467,68 @@ fn validate_symlink_entry( } } +fn collect_child_issues(child_status: &StatusChildEntry, issues: &mut Vec) { + if !child_status.exists { + issues.push(StatusIssue { + kind: StatusIssueKind::MissingExpectedChild, + path: child_status.path.clone(), + expected: Some(child_status.expected_source.clone()), + actual: None, + }); + } else if !child_status.is_symlink { + issues.push(StatusIssue { + kind: StatusIssueKind::ChildNotSymlink, + path: child_status.path.clone(), + expected: Some(child_status.expected_source.clone()), + actual: None, + }); + } else if let Some(actual) = child_status.points_to.as_ref() + && !paths_match( + Path::new(&child_status.path), + Path::new(actual), + Path::new(&child_status.expected_source), + ) + { + issues.push(StatusIssue { + kind: StatusIssueKind::IncorrectLinkTarget, + path: child_status.path.clone(), + expected: Some(child_status.expected_source.clone()), + actual: Some(actual.clone()), + }); + } +} + +fn validate_children_directory( + destination: &Path, + destination_kind: DestinationKind, + exists: bool, + children: Vec, + issues: &mut Vec, + managed_children: &mut Vec, +) { + if !exists { + issues.push(StatusIssue { + kind: StatusIssueKind::MissingDestination, + path: display_path(destination), + expected: Some("directory".to_string()), + actual: None, + }); + } else if destination_kind != DestinationKind::Directory { + issues.push(StatusIssue { + kind: StatusIssueKind::InvalidDestinationType, + path: display_path(destination), + expected: Some("directory".to_string()), + actual: Some(destination_kind_label(destination_kind).to_string()), + }); + } else { + for child in children { + let child_status = validate_symlink_contents_child(destination, child); + collect_child_issues(&child_status, issues); + managed_children.push(child_status); + } + } +} + fn validate_symlink_contents_entry( linker: &Linker, destination: PathBuf, @@ -498,55 +560,14 @@ fn validate_symlink_contents_entry( }); } - if !exists { - issues.push(StatusIssue { - kind: StatusIssueKind::MissingDestination, - path: display_path(&destination), - expected: Some("directory".to_string()), - actual: None, - }); - } else if destination_kind != DestinationKind::Directory { - issues.push(StatusIssue { - kind: StatusIssueKind::InvalidDestinationType, - path: display_path(&destination), - expected: Some("directory".to_string()), - actual: Some(destination_kind_label(destination_kind).to_string()), - }); - } else { - for child in children { - let child_status = validate_symlink_contents_child(&destination, child); - if !child_status.exists { - issues.push(StatusIssue { - kind: StatusIssueKind::MissingExpectedChild, - path: child_status.path.clone(), - expected: Some(child_status.expected_source.clone()), - actual: None, - }); - } else if !child_status.is_symlink { - issues.push(StatusIssue { - kind: StatusIssueKind::ChildNotSymlink, - path: child_status.path.clone(), - expected: Some(child_status.expected_source.clone()), - actual: None, - }); - } else if let Some(actual) = child_status.points_to.as_ref() - && !paths_match( - Path::new(&child_status.path), - Path::new(actual), - Path::new(&child_status.expected_source), - ) - { - issues.push(StatusIssue { - kind: StatusIssueKind::IncorrectLinkTarget, - path: child_status.path.clone(), - expected: Some(child_status.expected_source.clone()), - actual: Some(actual.clone()), - }); - } - - managed_children.push(child_status); - } - } + validate_children_directory( + &destination, + destination_kind, + exists, + children, + &mut issues, + &mut managed_children, + ); } else { issues.push(StatusIssue { kind: StatusIssueKind::MissingExpectedSource, diff --git a/src/config.rs b/src/config.rs index b96d26eb..402b532c 100644 --- a/src/config.rs +++ b/src/config.rs @@ -351,47 +351,53 @@ impl Config { pub fn all_gitignore_entries(&self) -> Vec { let mut entries: BTreeSet = self.gitignore.entries.iter().cloned().collect(); - entries.insert(".agents/skills/*.bak".to_string()); // Defensive pattern to ignore skill backup files even if skills aren't used yet - // Add destinations from all enabled agents and their known patterns + entries.insert(".agents/skills/*.bak".to_string()); for (agent_name, agent) in &self.agents { if agent.enabled { - // Add target destinations - for target in agent.targets.values() { - // NestedGlob destinations are templates, not literal paths – - // skip them to avoid polluting .gitignore with raw template strings. - if target.sync_type == SyncType::NestedGlob { - continue; - } - // ModuleMap targets expand each mapping into its own gitignore entry - if target.sync_type == SyncType::ModuleMap { - for mapping in &target.mappings { - let filename = resolve_module_map_filename(mapping, agent_name); - let entry = format!("{}/{}", mapping.destination, filename); - entries.insert(normalize_managed_gitignore_entry(&entry)); - entries.insert(normalize_managed_gitignore_entry(&format!( - "{}.bak", - entry - ))); - } - continue; - } - entries.insert(normalize_managed_gitignore_entry(&target.destination)); - entries.insert(normalize_managed_gitignore_entry(&format!( - "{}.bak", - target.destination - ))); - } - - // Add known ignore patterns for this agent - for pattern in Self::known_ignore_patterns(agent_name) { - entries.insert(normalize_managed_gitignore_entry(pattern)); - } + Self::collect_agent_gitignore_entries(agent_name, agent, &mut entries); } } entries.into_iter().collect() } + fn collect_agent_gitignore_entries( + agent_name: &str, + agent: &AgentConfig, + entries: &mut BTreeSet, + ) { + for target in agent.targets.values() { + Self::collect_target_gitignore_entries(agent_name, target, entries); + } + for pattern in Self::known_ignore_patterns(agent_name) { + entries.insert(normalize_managed_gitignore_entry(pattern)); + } + } + + fn collect_target_gitignore_entries( + agent_name: &str, + target: &TargetConfig, + entries: &mut BTreeSet, + ) { + if target.sync_type == SyncType::NestedGlob { + return; + } + if target.sync_type == SyncType::ModuleMap { + for mapping in &target.mappings { + let filename = resolve_module_map_filename(mapping, agent_name); + let entry = format!("{}/{}", mapping.destination, filename); + entries.insert(normalize_managed_gitignore_entry(&entry)); + entries.insert(normalize_managed_gitignore_entry(&format!("{}.bak", entry))); + } + return; + } + entries.insert(normalize_managed_gitignore_entry(&target.destination)); + entries.insert(normalize_managed_gitignore_entry(&format!( + "{}.bak", + target.destination + ))); + } + /// Get known gitignore patterns for a specific agent. /// These are files/directories that agents generate but are not direct symlink targets. pub fn known_ignore_patterns(agent_name: &str) -> &'static [&'static str] { diff --git a/src/init.rs b/src/init.rs index 9c767431..c836073a 100644 --- a/src/init.rs +++ b/src/init.rs @@ -486,577 +486,459 @@ fn dir_has_entries(path: &Path) -> Result { .is_some()) } -/// Scan project for existing agent-related files -fn scan_agent_files(project_root: &Path) -> Result> { - let mut discovered = Vec::new(); - - // ------------------------------------------------------------------------- - // Native MCP agents - // ------------------------------------------------------------------------- - - // Claude Code: CLAUDE.md - let claude_path = project_root.join("CLAUDE.md"); - if claude_path.exists() { - discovered.push(DiscoveredFile { - path: "CLAUDE.md".into(), - file_type: AgentFileType::ClaudeInstructions, - display_name: "CLAUDE.md (Claude Code instructions)".to_string(), - }); - } - - // Claude Code: .claude/skills/ directory - let claude_skills_path = project_root.join(".claude").join("skills"); - if claude_skills_path.exists() && claude_skills_path.is_dir() { - // Only report if directory has at least one child entry - let has_content = dir_has_entries(&claude_skills_path)?; - if has_content { - discovered.push(DiscoveredFile { - path: ".claude/skills".into(), - file_type: AgentFileType::ClaudeSkills, - display_name: "Claude Code skills (.claude/skills/)".to_string(), - }); - } - } - - // Claude Code: .claude/commands/ directory - let claude_commands_path = project_root.join(".claude").join("commands"); - if claude_commands_path.exists() && claude_commands_path.is_dir() { - let has_content = dir_has_entries(&claude_commands_path)?; - if has_content { - discovered.push(DiscoveredFile { - path: ".claude/commands".into(), - file_type: AgentFileType::ClaudeCommands, - display_name: "Claude Code commands (.claude/commands/)".to_string(), - }); - } - } - - // GitHub Copilot: .github/copilot-instructions.md - let copilot_path = project_root.join(".github").join("copilot-instructions.md"); - if copilot_path.exists() { - discovered.push(DiscoveredFile { - path: ".github/copilot-instructions.md".into(), - file_type: AgentFileType::CopilotInstructions, - display_name: ".github/copilot-instructions.md (Copilot instructions)".to_string(), - }); - } - - // Copilot: .vscode/mcp.json - let copilot_mcp_path = project_root.join(".vscode").join("mcp.json"); - if copilot_mcp_path.exists() { - discovered.push(DiscoveredFile { - path: ".vscode/mcp.json".into(), - file_type: AgentFileType::CopilotMcpConfig, - display_name: ".vscode/mcp.json (VS Code / Copilot MCP configuration)".to_string(), - }); - } - - // Cursor: .cursor/ directory - let cursor_path = project_root.join(".cursor"); - if cursor_path.exists() && cursor_path.is_dir() { - discovered.push(DiscoveredFile { - path: ".cursor".into(), - file_type: AgentFileType::CursorDirectory, - display_name: ".cursor/ (Cursor configuration directory)".to_string(), - }); - } - - // Cursor: .cursor/skills/ directory - let cursor_skills_path = project_root.join(".cursor").join("skills"); - if cursor_skills_path.exists() && cursor_skills_path.is_dir() { - let has_content = dir_has_entries(&cursor_skills_path)?; - if has_content { - discovered.push(DiscoveredFile { - path: ".cursor/skills".into(), - file_type: AgentFileType::CursorSkills, - display_name: "Cursor skills (.cursor/skills/)".to_string(), - }); - } - } - - // Cursor: .cursor/mcp.json - let cursor_mcp_path = project_root.join(".cursor").join("mcp.json"); - if cursor_mcp_path.exists() { - discovered.push(DiscoveredFile { - path: ".cursor/mcp.json".into(), - file_type: AgentFileType::CursorMcpConfig, - display_name: ".cursor/mcp.json (Cursor MCP configuration)".to_string(), - }); - } - - // Gemini CLI: GEMINI.md - let gemini_path = project_root.join("GEMINI.md"); - if gemini_path.exists() { - discovered.push(DiscoveredFile { - path: "GEMINI.md".into(), - file_type: AgentFileType::GeminiInstructions, - display_name: "GEMINI.md (Gemini CLI instructions)".to_string(), - }); - } - - // Gemini CLI: .gemini/skills/ directory - let gemini_skills_path = project_root.join(".gemini").join("skills"); - if gemini_skills_path.exists() && gemini_skills_path.is_dir() { - let has_content = dir_has_entries(&gemini_skills_path)?; - if has_content { - discovered.push(DiscoveredFile { - path: ".gemini/skills".into(), - file_type: AgentFileType::GeminiSkills, - display_name: "Gemini skills (.gemini/skills/)".to_string(), - }); - } - } - - // Gemini CLI: .gemini/commands/ directory - let gemini_commands_path = project_root.join(".gemini").join("commands"); - if gemini_commands_path.exists() && gemini_commands_path.is_dir() { - let has_content = dir_has_entries(&gemini_commands_path)?; - if has_content { - discovered.push(DiscoveredFile { - path: ".gemini/commands".into(), - file_type: AgentFileType::GeminiCommands, - display_name: "Gemini commands (.gemini/commands/)".to_string(), - }); - } - } - - // OpenCode: .opencode/skills/ directory - let opencode_skills_path = project_root.join(".opencode").join("skills"); - if opencode_skills_path.exists() && opencode_skills_path.is_dir() { - let has_content = dir_has_entries(&opencode_skills_path)?; - if has_content { - discovered.push(DiscoveredFile { - path: ".opencode/skills".into(), - file_type: AgentFileType::OpenCodeSkills, - display_name: "OpenCode skills (.opencode/skills/)".to_string(), - }); - } - } - - // OpenCode: .opencode/command/ directory - let opencode_commands_path = project_root.join(".opencode").join("command"); - if opencode_commands_path.exists() && opencode_commands_path.is_dir() { - let has_content = dir_has_entries(&opencode_commands_path)?; - if has_content { - discovered.push(DiscoveredFile { - path: ".opencode/command".into(), - file_type: AgentFileType::OpenCodeCommands, - display_name: "OpenCode commands (.opencode/command/)".to_string(), - }); - } - } - - // OpenCode: opencode.json - let opencode_config_path = project_root.join("opencode.json"); - if opencode_config_path.exists() { - discovered.push(DiscoveredFile { - path: "opencode.json".into(), - file_type: AgentFileType::OpenCodeConfig, - display_name: "opencode.json (OpenCode configuration)".to_string(), - }); - } - - // Generic MCP config: .mcp.json - let mcp_path = project_root.join(".mcp.json"); - if mcp_path.exists() { - discovered.push(DiscoveredFile { - path: ".mcp.json".into(), - file_type: AgentFileType::McpConfig, - display_name: ".mcp.json (MCP configuration)".to_string(), - }); - } - - // ------------------------------------------------------------------------- - // Root AGENTS.md (used by Codex CLI and many other agents) - // ------------------------------------------------------------------------- - let agents_path = project_root.join("AGENTS.md"); - if agents_path.exists() { - discovered.push(DiscoveredFile { - path: "AGENTS.md".into(), - file_type: AgentFileType::RootAgentsFile, - display_name: "AGENTS.md (Root agent instructions)".to_string(), - }); - } - - // Codex CLI: .codex/skills/ directory - let codex_skills_path = project_root.join(".codex").join("skills"); - if codex_skills_path.exists() && codex_skills_path.is_dir() { - let has_content = dir_has_entries(&codex_skills_path)?; - if has_content { - discovered.push(DiscoveredFile { - path: ".codex/skills".into(), - file_type: AgentFileType::CodexSkills, - display_name: "Codex skills (.codex/skills/)".to_string(), - }); - } - } - - // Codex CLI: .codex/config.toml - let codex_config_path = project_root.join(".codex").join("config.toml"); - if codex_config_path.exists() { - discovered.push(DiscoveredFile { - path: ".codex/config.toml".into(), - file_type: AgentFileType::CodexConfig, - display_name: ".codex/config.toml (Codex configuration)".to_string(), - }); - } - - // ------------------------------------------------------------------------- - // Configurable agents — rules / instruction files - // ------------------------------------------------------------------------- - - // Windsurf: .windsurfrules - let windsurfrules_path = project_root.join(".windsurfrules"); - if windsurfrules_path.exists() { - discovered.push(DiscoveredFile { - path: ".windsurfrules".into(), - file_type: AgentFileType::WindsurfRules, - display_name: ".windsurfrules (Windsurf rules)".to_string(), - }); - } - - // Windsurf: .windsurf/ directory (rules + MCP) - let windsurf_dir = project_root.join(".windsurf"); - if windsurf_dir.exists() && windsurf_dir.is_dir() { - discovered.push(DiscoveredFile { - path: ".windsurf".into(), - file_type: AgentFileType::WindsurfDirectory, - display_name: ".windsurf/ (Windsurf configuration directory)".to_string(), - }); - } - - // Windsurf: .windsurf/mcp_config.json - let windsurf_mcp_path = project_root.join(".windsurf").join("mcp_config.json"); - if windsurf_mcp_path.exists() { - discovered.push(DiscoveredFile { - path: ".windsurf/mcp_config.json".into(), - file_type: AgentFileType::WindsurfMcpConfig, - display_name: ".windsurf/mcp_config.json (Windsurf MCP configuration)".to_string(), - }); - } - - // Cline: .clinerules - let cline_path = project_root.join(".clinerules"); - if cline_path.exists() { - discovered.push(DiscoveredFile { - path: ".clinerules".into(), - file_type: AgentFileType::ClineRules, - display_name: ".clinerules (Cline rules)".to_string(), - }); - } - - // Crush: CRUSH.md - let crush_path = project_root.join("CRUSH.md"); - if crush_path.exists() { - discovered.push(DiscoveredFile { - path: "CRUSH.md".into(), - file_type: AgentFileType::CrushInstructions, - display_name: "CRUSH.md (Crush instructions)".to_string(), - }); - } - - // Amp: AMPCODE.md - let amp_path = project_root.join("AMPCODE.md"); - if amp_path.exists() { - discovered.push(DiscoveredFile { - path: "AMPCODE.md".into(), - file_type: AgentFileType::AmpInstructions, - display_name: "AMPCODE.md (Amp instructions)".to_string(), - }); - } - - // Amazon Q CLI: .amazonq/rules/ - let amazonq_rules = project_root.join(".amazonq").join("rules"); - if amazonq_rules.exists() && amazonq_rules.is_dir() { - discovered.push(DiscoveredFile { - path: ".amazonq/rules".into(), - file_type: AgentFileType::AmazonQRules, - display_name: ".amazonq/rules/ (Amazon Q CLI rules)".to_string(), - }); - } - - // Amazon Q: .amazonq/mcp.json - let amazonq_mcp_path = project_root.join(".amazonq").join("mcp.json"); - if amazonq_mcp_path.exists() { - discovered.push(DiscoveredFile { - path: ".amazonq/mcp.json".into(), - file_type: AgentFileType::AmazonQMcpConfig, - display_name: ".amazonq/mcp.json (Amazon Q MCP configuration)".to_string(), - }); - } - - // Aider: .aider.conf.yml - let aider_path = project_root.join(".aider.conf.yml"); - if aider_path.exists() { - discovered.push(DiscoveredFile { - path: ".aider.conf.yml".into(), - file_type: AgentFileType::AiderConfig, - display_name: ".aider.conf.yml (Aider configuration)".to_string(), - }); - } - - // Firebase Studio / IDX: .idx/airules.md - let firebase_rules = project_root.join(".idx").join("airules.md"); - if firebase_rules.exists() { - discovered.push(DiscoveredFile { - path: ".idx/airules.md".into(), - file_type: AgentFileType::FirebaseRules, - display_name: ".idx/airules.md (Firebase Studio / IDX rules)".to_string(), - }); - } - - // OpenHands: .openhands/microagents/ - let openhands_path = project_root.join(".openhands").join("microagents"); - if openhands_path.exists() && openhands_path.is_dir() { - discovered.push(DiscoveredFile { - path: ".openhands/microagents".into(), - file_type: AgentFileType::OpenHandsMicroagents, - display_name: ".openhands/microagents/ (OpenHands microagents)".to_string(), - }); - } - - // Junie (JetBrains): .junie/ - let junie_path = project_root.join(".junie"); - if junie_path.exists() && junie_path.is_dir() { - discovered.push(DiscoveredFile { - path: ".junie".into(), - file_type: AgentFileType::JunieDirectory, - display_name: ".junie/ (Junie / JetBrains AI configuration)".to_string(), - }); - } - - // Augment Code: .augment/rules/ - let augment_rules = project_root.join(".augment").join("rules"); - if augment_rules.exists() && augment_rules.is_dir() { - discovered.push(DiscoveredFile { - path: ".augment/rules".into(), - file_type: AgentFileType::AugmentRules, - display_name: ".augment/rules/ (Augment Code rules)".to_string(), - }); - } - - // Kilo Code: .kilocode/ - let kilocode_path = project_root.join(".kilocode"); - if kilocode_path.exists() && kilocode_path.is_dir() { - discovered.push(DiscoveredFile { - path: ".kilocode".into(), - file_type: AgentFileType::KilocodeDirectory, - display_name: ".kilocode/ (Kilo Code configuration)".to_string(), - }); - } - - // Kilo Code: .kilocode/mcp.json - let kilocode_mcp_path = project_root.join(".kilocode").join("mcp.json"); - if kilocode_mcp_path.exists() { - discovered.push(DiscoveredFile { - path: ".kilocode/mcp.json".into(), - file_type: AgentFileType::KilocodeMcpConfig, - display_name: ".kilocode/mcp.json (Kilo Code MCP configuration)".to_string(), - }); - } - - // Goose (Block): .goosehints - let goose_path = project_root.join(".goosehints"); - if goose_path.exists() { - discovered.push(DiscoveredFile { - path: ".goosehints".into(), - file_type: AgentFileType::GooseHints, - display_name: ".goosehints (Goose hints)".to_string(), - }); - } - - // Qwen Code: .qwen/ - let qwen_path = project_root.join(".qwen"); - if qwen_path.exists() && qwen_path.is_dir() { - discovered.push(DiscoveredFile { - path: ".qwen".into(), - file_type: AgentFileType::QwenDirectory, - display_name: ".qwen/ (Qwen Code configuration)".to_string(), - }); - } - - // Roo Code: .roo/rules/ - let roo_rules = project_root.join(".roo").join("rules"); - if roo_rules.exists() && roo_rules.is_dir() { - discovered.push(DiscoveredFile { - path: ".roo/rules".into(), - file_type: AgentFileType::RooRules, - display_name: ".roo/rules/ (Roo Code rules)".to_string(), - }); - } - - // Roo Code: .roo/skills/ directory - let roo_skills_path = project_root.join(".roo").join("skills"); - if roo_skills_path.exists() && roo_skills_path.is_dir() { - let has_content = dir_has_entries(&roo_skills_path)?; - if has_content { - discovered.push(DiscoveredFile { - path: ".roo/skills".into(), - file_type: AgentFileType::RooSkills, - display_name: "Roo Code skills (.roo/skills/)".to_string(), - }); - } - } - - // Roo Code: .roo/mcp.json - let roo_mcp_path = project_root.join(".roo").join("mcp.json"); - if roo_mcp_path.exists() { - discovered.push(DiscoveredFile { - path: ".roo/mcp.json".into(), - file_type: AgentFileType::RooMcpConfig, - display_name: ".roo/mcp.json (Roo Code MCP configuration)".to_string(), - }); - } - - // Trae AI: .trae/rules/ - let trae_rules = project_root.join(".trae").join("rules"); - if trae_rules.exists() && trae_rules.is_dir() { - discovered.push(DiscoveredFile { - path: ".trae/rules".into(), - file_type: AgentFileType::TraeRules, - display_name: ".trae/rules/ (Trae AI rules)".to_string(), - }); - } - - // Warp: WARP.md - let warp_path = project_root.join("WARP.md"); - if warp_path.exists() { - discovered.push(DiscoveredFile { - path: "WARP.md".into(), - file_type: AgentFileType::WarpInstructions, - display_name: "WARP.md (Warp terminal instructions)".to_string(), - }); - } - - // Kiro: .kiro/steering/ - let kiro_steering = project_root.join(".kiro").join("steering"); - if kiro_steering.exists() && kiro_steering.is_dir() { +/// Discover a single file and push it if it exists. +fn discover_file( + project_root: &Path, + rel_path: &str, + file_type: AgentFileType, + display_name: &str, + discovered: &mut Vec, +) { + if project_root.join(rel_path).exists() { discovered.push(DiscoveredFile { - path: ".kiro/steering".into(), - file_type: AgentFileType::KiroSteering, - display_name: ".kiro/steering/ (Kiro steering documents)".to_string(), + path: rel_path.into(), + file_type, + display_name: display_name.to_string(), }); } +} - // Kiro: .kiro/settings/mcp.json - let kiro_mcp_path = project_root.join(".kiro").join("settings").join("mcp.json"); - if kiro_mcp_path.exists() { +/// Discover a directory (existence only, no content check). +fn discover_dir( + project_root: &Path, + rel_path: &str, + file_type: AgentFileType, + display_name: &str, + discovered: &mut Vec, +) { + let path = project_root.join(rel_path); + if path.exists() && path.is_dir() { discovered.push(DiscoveredFile { - path: ".kiro/settings/mcp.json".into(), - file_type: AgentFileType::KiroMcpConfig, - display_name: ".kiro/settings/mcp.json (Kiro MCP configuration)".to_string(), + path: rel_path.into(), + file_type, + display_name: display_name.to_string(), }); } +} - // Firebender: firebender.json - let firebender_path = project_root.join("firebender.json"); - if firebender_path.exists() { +/// Discover a directory only if it has at least one entry. +fn discover_dir_with_content( + project_root: &Path, + rel_path: &str, + file_type: AgentFileType, + display_name: &str, + discovered: &mut Vec, +) -> Result<()> { + let path = project_root.join(rel_path); + if path.exists() && path.is_dir() && dir_has_entries(&path)? { discovered.push(DiscoveredFile { - path: "firebender.json".into(), - file_type: AgentFileType::FirebenderConfig, - display_name: "firebender.json (Firebender configuration)".to_string(), + path: rel_path.into(), + file_type, + display_name: display_name.to_string(), }); } + Ok(()) +} - // Factory (Droids): .factory/ - let factory_path = project_root.join(".factory"); - if factory_path.exists() && factory_path.is_dir() { - discovered.push(DiscoveredFile { - path: ".factory".into(), - file_type: AgentFileType::FactoryDirectory, - display_name: ".factory/ (Factory Droids configuration)".to_string(), - }); - } +/// Scan for native MCP agent files (Claude, Copilot, Cursor, Gemini, OpenCode, Codex, etc.) +fn scan_native_mcp_agents(project_root: &Path, discovered: &mut Vec) -> Result<()> { + // Claude Code + discover_file( + project_root, + "CLAUDE.md", + AgentFileType::ClaudeInstructions, + "CLAUDE.md (Claude Code instructions)", + discovered, + ); + discover_dir_with_content( + project_root, + ".claude/skills", + AgentFileType::ClaudeSkills, + "Claude Code skills (.claude/skills/)", + discovered, + )?; + discover_dir_with_content( + project_root, + ".claude/commands", + AgentFileType::ClaudeCommands, + "Claude Code commands (.claude/commands/)", + discovered, + )?; + + // GitHub Copilot + discover_file( + project_root, + ".github/copilot-instructions.md", + AgentFileType::CopilotInstructions, + ".github/copilot-instructions.md (Copilot instructions)", + discovered, + ); + discover_file( + project_root, + ".vscode/mcp.json", + AgentFileType::CopilotMcpConfig, + ".vscode/mcp.json (VS Code / Copilot MCP configuration)", + discovered, + ); - // Factory: .factory/skills/ directory - let factory_skills_path = project_root.join(".factory").join("skills"); - if factory_skills_path.exists() && factory_skills_path.is_dir() { - let has_content = dir_has_entries(&factory_skills_path)?; - if has_content { - discovered.push(DiscoveredFile { - path: ".factory/skills".into(), - file_type: AgentFileType::FactorySkills, - display_name: "Factory skills (.factory/skills/)".to_string(), - }); - } - } + // Cursor + discover_dir( + project_root, + ".cursor", + AgentFileType::CursorDirectory, + ".cursor/ (Cursor configuration directory)", + discovered, + ); + discover_dir_with_content( + project_root, + ".cursor/skills", + AgentFileType::CursorSkills, + "Cursor skills (.cursor/skills/)", + discovered, + )?; + discover_file( + project_root, + ".cursor/mcp.json", + AgentFileType::CursorMcpConfig, + ".cursor/mcp.json (Cursor MCP configuration)", + discovered, + ); - // Factory: .factory/mcp.json - let factory_mcp_path = project_root.join(".factory").join("mcp.json"); - if factory_mcp_path.exists() { - discovered.push(DiscoveredFile { - path: ".factory/mcp.json".into(), - file_type: AgentFileType::FactoryMcpConfig, - display_name: ".factory/mcp.json (Factory MCP configuration)".to_string(), - }); - } + // Gemini CLI + discover_file( + project_root, + "GEMINI.md", + AgentFileType::GeminiInstructions, + "GEMINI.md (Gemini CLI instructions)", + discovered, + ); + discover_dir_with_content( + project_root, + ".gemini/skills", + AgentFileType::GeminiSkills, + "Gemini skills (.gemini/skills/)", + discovered, + )?; + discover_dir_with_content( + project_root, + ".gemini/commands", + AgentFileType::GeminiCommands, + "Gemini commands (.gemini/commands/)", + discovered, + )?; + + // OpenCode + discover_dir_with_content( + project_root, + ".opencode/skills", + AgentFileType::OpenCodeSkills, + "OpenCode skills (.opencode/skills/)", + discovered, + )?; + discover_dir_with_content( + project_root, + ".opencode/command", + AgentFileType::OpenCodeCommands, + "OpenCode commands (.opencode/command/)", + discovered, + )?; + discover_file( + project_root, + "opencode.json", + AgentFileType::OpenCodeConfig, + "opencode.json (OpenCode configuration)", + discovered, + ); - // Vibe (Mistral): .vibe/ - let vibe_path = project_root.join(".vibe"); - if vibe_path.exists() && vibe_path.is_dir() { - discovered.push(DiscoveredFile { - path: ".vibe".into(), - file_type: AgentFileType::VibeDirectory, - display_name: ".vibe/ (Vibe / Mistral configuration)".to_string(), - }); - } + // Generic MCP + discover_file( + project_root, + ".mcp.json", + AgentFileType::McpConfig, + ".mcp.json (MCP configuration)", + discovered, + ); - // Vibe: .vibe/skills/ directory - let vibe_skills_path = project_root.join(".vibe").join("skills"); - if vibe_skills_path.exists() && vibe_skills_path.is_dir() { - let has_content = dir_has_entries(&vibe_skills_path)?; - if has_content { - discovered.push(DiscoveredFile { - path: ".vibe/skills".into(), - file_type: AgentFileType::VibeSkills, - display_name: "Vibe skills (.vibe/skills/)".to_string(), - }); - } - } + // Root AGENTS.md + discover_file( + project_root, + "AGENTS.md", + AgentFileType::RootAgentsFile, + "AGENTS.md (Root agent instructions)", + discovered, + ); - // JetBrains AI Assistant: .aiassistant/rules/ - let jetbrains_rules = project_root.join(".aiassistant").join("rules"); - if jetbrains_rules.exists() && jetbrains_rules.is_dir() { - discovered.push(DiscoveredFile { - path: ".aiassistant/rules".into(), - file_type: AgentFileType::JetBrainsRules, - display_name: ".aiassistant/rules/ (JetBrains AI Assistant rules)".to_string(), - }); - } + // Codex CLI + discover_dir_with_content( + project_root, + ".codex/skills", + AgentFileType::CodexSkills, + "Codex skills (.codex/skills/)", + discovered, + )?; + discover_file( + project_root, + ".codex/config.toml", + AgentFileType::CodexConfig, + ".codex/config.toml (Codex configuration)", + discovered, + ); - // Antigravity: .agent/rules/ - let antigravity_rules = project_root.join(".agent").join("rules"); - if antigravity_rules.exists() && antigravity_rules.is_dir() { - discovered.push(DiscoveredFile { - path: ".agent/rules".into(), - file_type: AgentFileType::AntigravityRules, - display_name: ".agent/rules/ (Antigravity rules)".to_string(), - }); - } + Ok(()) +} - // Antigravity: .agent/skills/ directory - let antigravity_skills_path = project_root.join(".agent").join("skills"); - if antigravity_skills_path.exists() && antigravity_skills_path.is_dir() { - let has_content = dir_has_entries(&antigravity_skills_path)?; - if has_content { - discovered.push(DiscoveredFile { - path: ".agent/skills".into(), - file_type: AgentFileType::AntigravitySkills, - display_name: "Antigravity skills (.agent/skills/)".to_string(), - }); - } - } +/// Scan for configurable agents (Windsurf, Cline, Crush, Amp, etc.) +fn scan_configurable_agents( + project_root: &Path, + discovered: &mut Vec, +) -> Result<()> { + discover_file( + project_root, + ".windsurfrules", + AgentFileType::WindsurfRules, + ".windsurfrules (Windsurf rules)", + discovered, + ); + discover_dir( + project_root, + ".windsurf", + AgentFileType::WindsurfDirectory, + ".windsurf/ (Windsurf configuration directory)", + discovered, + ); + discover_file( + project_root, + ".windsurf/mcp_config.json", + AgentFileType::WindsurfMcpConfig, + ".windsurf/mcp_config.json (Windsurf MCP configuration)", + discovered, + ); + discover_file( + project_root, + ".clinerules", + AgentFileType::ClineRules, + ".clinerules (Cline rules)", + discovered, + ); + discover_file( + project_root, + "CRUSH.md", + AgentFileType::CrushInstructions, + "CRUSH.md (Crush instructions)", + discovered, + ); + discover_file( + project_root, + "AMPCODE.md", + AgentFileType::AmpInstructions, + "AMPCODE.md (Amp instructions)", + discovered, + ); + discover_dir( + project_root, + ".amazonq/rules", + AgentFileType::AmazonQRules, + ".amazonq/rules/ (Amazon Q CLI rules)", + discovered, + ); + discover_file( + project_root, + ".amazonq/mcp.json", + AgentFileType::AmazonQMcpConfig, + ".amazonq/mcp.json (Amazon Q MCP configuration)", + discovered, + ); + discover_file( + project_root, + ".aider.conf.yml", + AgentFileType::AiderConfig, + ".aider.conf.yml (Aider configuration)", + discovered, + ); + discover_file( + project_root, + ".idx/airules.md", + AgentFileType::FirebaseRules, + ".idx/airules.md (Firebase Studio / IDX rules)", + discovered, + ); + discover_dir( + project_root, + ".openhands/microagents", + AgentFileType::OpenHandsMicroagents, + ".openhands/microagents/ (OpenHands microagents)", + discovered, + ); + discover_dir( + project_root, + ".junie", + AgentFileType::JunieDirectory, + ".junie/ (Junie / JetBrains AI configuration)", + discovered, + ); + discover_dir( + project_root, + ".augment/rules", + AgentFileType::AugmentRules, + ".augment/rules/ (Augment Code rules)", + discovered, + ); + discover_dir( + project_root, + ".kilocode", + AgentFileType::KilocodeDirectory, + ".kilocode/ (Kilo Code configuration)", + discovered, + ); + discover_file( + project_root, + ".kilocode/mcp.json", + AgentFileType::KilocodeMcpConfig, + ".kilocode/mcp.json (Kilo Code MCP configuration)", + discovered, + ); + discover_file( + project_root, + ".goosehints", + AgentFileType::GooseHints, + ".goosehints (Goose hints)", + discovered, + ); + discover_dir( + project_root, + ".qwen", + AgentFileType::QwenDirectory, + ".qwen/ (Qwen Code configuration)", + discovered, + ); + discover_dir( + project_root, + ".roo/rules", + AgentFileType::RooRules, + ".roo/rules/ (Roo Code rules)", + discovered, + ); + discover_dir_with_content( + project_root, + ".roo/skills", + AgentFileType::RooSkills, + "Roo Code skills (.roo/skills/)", + discovered, + )?; + discover_file( + project_root, + ".roo/mcp.json", + AgentFileType::RooMcpConfig, + ".roo/mcp.json (Roo Code MCP configuration)", + discovered, + ); + discover_dir( + project_root, + ".trae/rules", + AgentFileType::TraeRules, + ".trae/rules/ (Trae AI rules)", + discovered, + ); + discover_file( + project_root, + "WARP.md", + AgentFileType::WarpInstructions, + "WARP.md (Warp terminal instructions)", + discovered, + ); + discover_dir( + project_root, + ".kiro/steering", + AgentFileType::KiroSteering, + ".kiro/steering/ (Kiro steering documents)", + discovered, + ); + discover_file( + project_root, + ".kiro/settings/mcp.json", + AgentFileType::KiroMcpConfig, + ".kiro/settings/mcp.json (Kiro MCP configuration)", + discovered, + ); + discover_file( + project_root, + "firebender.json", + AgentFileType::FirebenderConfig, + "firebender.json (Firebender configuration)", + discovered, + ); + discover_dir( + project_root, + ".factory", + AgentFileType::FactoryDirectory, + ".factory/ (Factory Droids configuration)", + discovered, + ); + discover_dir_with_content( + project_root, + ".factory/skills", + AgentFileType::FactorySkills, + "Factory skills (.factory/skills/)", + discovered, + )?; + discover_file( + project_root, + ".factory/mcp.json", + AgentFileType::FactoryMcpConfig, + ".factory/mcp.json (Factory MCP configuration)", + discovered, + ); + discover_dir( + project_root, + ".vibe", + AgentFileType::VibeDirectory, + ".vibe/ (Vibe / Mistral configuration)", + discovered, + ); + discover_dir_with_content( + project_root, + ".vibe/skills", + AgentFileType::VibeSkills, + "Vibe skills (.vibe/skills/)", + discovered, + )?; + discover_dir( + project_root, + ".aiassistant/rules", + AgentFileType::JetBrainsRules, + ".aiassistant/rules/ (JetBrains AI Assistant rules)", + discovered, + ); + discover_dir( + project_root, + ".agent/rules", + AgentFileType::AntigravityRules, + ".agent/rules/ (Antigravity rules)", + discovered, + ); + discover_dir_with_content( + project_root, + ".agent/skills", + AgentFileType::AntigravitySkills, + "Antigravity skills (.agent/skills/)", + discovered, + )?; + discover_file( + project_root, + ".zed/settings.json", + AgentFileType::ZedSettings, + ".zed/settings.json (Zed editor AI settings)", + discovered, + ); - // Zed editor: .zed/settings.json - let zed_settings = project_root.join(".zed").join("settings.json"); - if zed_settings.exists() { - discovered.push(DiscoveredFile { - path: ".zed/settings.json".into(), - file_type: AgentFileType::ZedSettings, - display_name: ".zed/settings.json (Zed editor AI settings)".to_string(), - }); - } + Ok(()) +} +/// Scan project for existing agent-related files +fn scan_agent_files(project_root: &Path) -> Result> { + let mut discovered = Vec::new(); + scan_native_mcp_agents(project_root, &mut discovered)?; + scan_configurable_agents(project_root, &mut discovered)?; Ok(discovered) } @@ -1730,7 +1612,395 @@ fn run_experimental_tui_intro() -> Result { } } -/// Interactive wizard for initializing agentsync with file migration +/// Merge multiple instruction files into a single string with section headings. +fn merge_instruction_files( + project_root: &Path, + instruction_files: &[&DiscoveredFile], +) -> Result<(Option, usize)> { + if instruction_files.len() > 1 { + let mut merged = String::new(); + let mut count = 0; + for file in instruction_files { + let src_path = project_root.join(&file.path); + let content = fs::read_to_string(&src_path) + .map_err(|e| anyhow::anyhow!("Failed to read '{}': {}", src_path.display(), e))?; + if !merged.is_empty() { + merged.push_str("\n\n---\n\n"); + } + merged.push_str(&format!( + "# Instructions from {}\n\n{}", + file.path.display(), + content + )); + count += 1; + } + Ok(( + if merged.is_empty() { + None + } else { + Some(merged) + }, + count, + )) + } else if instruction_files.len() == 1 { + let src_path = project_root.join(&instruction_files[0].path); + let content = fs::read_to_string(&src_path) + .map_err(|e| anyhow::anyhow!("Failed to read '{}': {}", src_path.display(), e))?; + Ok((Some(content), 1)) + } else { + Ok((None, 0)) + } +} + +/// Copy directory entries into a destination, printing progress. Returns (migrated, skipped). +fn copy_entries_to_dest( + src_path: &Path, + dest_dir: &Path, + entry_kind: &str, +) -> Result<(usize, usize)> { + use colored::Colorize; + let mut migrated = 0; + let mut skipped = 0; + if !src_path.exists() || !src_path.is_dir() { + return Ok((0, 0)); + } + for entry in fs::read_dir(src_path)? { + let entry = entry?; + let entry_path = entry.path(); + let name = entry.file_name(); + let dest = dest_dir.join(&name); + if dest.exists() { + println!( + " {} Skipped: {} '{}' already exists in {}/", + "⚠".yellow(), + entry_kind, + name.to_string_lossy(), + dest_dir.file_name().unwrap_or_default().to_string_lossy() + ); + skipped += 1; + } else { + if entry_path.is_dir() { + copy_dir_all(&entry_path, &dest)?; + } else { + fs::copy(&entry_path, &dest)?; + } + println!( + " {} Copied {}: {} → {}/{}", + "✔".green(), + entry_kind, + entry_path.display(), + dest_dir.file_name().unwrap_or_default().to_string_lossy(), + name.to_string_lossy() + ); + migrated += 1; + } + } + Ok((migrated, skipped)) +} + +/// Migrate a single discovered file, returning (migrated_count, skipped_count). +fn migrate_file( + file: &DiscoveredFile, + project_root: &Path, + agents_dir: &Path, + skills_dir: &Path, + commands_dir: &Path, +) -> Result<(usize, usize)> { + use colored::Colorize; + let src_path = project_root.join(&file.path); + + match file.file_type { + // Plain-text instruction files — already merged into AGENTS.md + AgentFileType::ClaudeInstructions + | AgentFileType::RootAgentsFile + | AgentFileType::CopilotInstructions + | AgentFileType::WindsurfRules + | AgentFileType::ClineRules + | AgentFileType::CrushInstructions + | AgentFileType::AmpInstructions + | AgentFileType::GooseHints + | AgentFileType::WarpInstructions + | AgentFileType::GeminiInstructions => Ok((0, 0)), + + // Skill directories + AgentFileType::ClaudeSkills + | AgentFileType::CursorSkills + | AgentFileType::CodexSkills + | AgentFileType::GeminiSkills + | AgentFileType::OpenCodeSkills + | AgentFileType::RooSkills + | AgentFileType::FactorySkills + | AgentFileType::VibeSkills + | AgentFileType::AntigravitySkills => copy_entries_to_dest(&src_path, skills_dir, "skill"), + + // Command directories + AgentFileType::ClaudeCommands + | AgentFileType::GeminiCommands + | AgentFileType::OpenCodeCommands => { + copy_entries_to_dest(&src_path, commands_dir, "command") + } + + // Directories — copy to .agents/ + AgentFileType::CursorDirectory + | AgentFileType::WindsurfDirectory + | AgentFileType::AntigravityRules + | AgentFileType::AmazonQRules + | AgentFileType::OpenHandsMicroagents + | AgentFileType::JunieDirectory + | AgentFileType::AugmentRules + | AgentFileType::KilocodeDirectory + | AgentFileType::QwenDirectory + | AgentFileType::RooRules + | AgentFileType::TraeRules + | AgentFileType::KiroSteering + | AgentFileType::FactoryDirectory + | AgentFileType::VibeDirectory + | AgentFileType::JetBrainsRules => { + if src_path.exists() { + let dest_path = agents_dir.join(&file.path); + if let Some(parent) = dest_path.parent() { + fs::create_dir_all(parent)?; + } + copy_dir_all(&src_path, &dest_path)?; + let dest_display = dest_path + .strip_prefix(project_root) + .unwrap_or(&dest_path) + .display(); + println!( + " {} Copied: {} → {}", + "✔".green(), + file.path.display(), + dest_display + ); + Ok((1, 0)) + } else { + Ok((0, 0)) + } + } + + // Single-file configs + AgentFileType::AiderConfig + | AgentFileType::FirebenderConfig + | AgentFileType::FirebaseRules => { + if src_path.exists() { + let dest_path = agents_dir.join(&file.path); + if let Some(parent) = dest_path.parent() { + fs::create_dir_all(parent)?; + } + fs::copy(&src_path, &dest_path)?; + let dest_display = dest_path + .strip_prefix(project_root) + .unwrap_or(&dest_path) + .display(); + println!( + " {} Copied: {} → {}", + "✔".green(), + file.path.display(), + dest_display + ); + Ok((1, 0)) + } else { + Ok((0, 0)) + } + } + + // MCP / tooling configs — just note them + AgentFileType::McpConfig + | AgentFileType::ZedSettings + | AgentFileType::CursorMcpConfig + | AgentFileType::CopilotMcpConfig + | AgentFileType::WindsurfMcpConfig + | AgentFileType::CodexConfig + | AgentFileType::RooMcpConfig + | AgentFileType::KiroMcpConfig + | AgentFileType::AmazonQMcpConfig + | AgentFileType::KilocodeMcpConfig + | AgentFileType::FactoryMcpConfig + | AgentFileType::OpenCodeConfig => { + println!( + " {} Note: {} detected. You can configure MCP servers in agentsync.toml", + "ℹ".blue(), + file.path.display() + ); + Ok((0, 1)) + } + AgentFileType::Other => Ok((0, 1)), + } +} + +/// Write AGENTS.md with migrated or default content. Returns outcome. +fn write_agents_md( + agents_md_path: &Path, + migrated_content: Option, + layout_block: &str, + instruction_files_merged: usize, + force: bool, +) -> Result { + use colored::Colorize; + if let Some(content) = migrated_content { + if agents_md_path.exists() && !force { + println!( + " {} AGENTS.md already exists (use --force to overwrite)", + "!".yellow() + ); + return Ok(ManagedFileOutcome::Preserved); + } + let rendered_agents_md = upsert_agent_config_layout_block(&content, layout_block); + fs::write(agents_md_path, rendered_agents_md)?; + if instruction_files_merged > 1 { + println!( + " {} Created: {} (merged {} instruction files)", + "✔".green(), + agents_md_path.display(), + instruction_files_merged + ); + } else { + println!( + " {} Created: {} (with migrated content)", + "✔".green(), + agents_md_path.display() + ); + } + Ok(ManagedFileOutcome::Written) + } else if !agents_md_path.exists() || force { + let rendered_agents_md = upsert_agent_config_layout_block(DEFAULT_AGENTS_MD, layout_block); + fs::write(agents_md_path, rendered_agents_md)?; + println!(" {} Created: {}", "✔".green(), agents_md_path.display()); + Ok(ManagedFileOutcome::Written) + } else { + Ok(ManagedFileOutcome::Preserved) + } +} + +/// Write wizard config and run post-init validation. Returns outcome. +fn write_wizard_config( + project_root: &Path, + config_path: &Path, + rendered_config: &str, + skills_choices: &[SkillsWizardChoice], + force: bool, +) -> Result { + use colored::Colorize; + if config_path.exists() && !force { + println!( + " {} Config already exists: {} (use --force to overwrite)", + "!".yellow(), + config_path.display() + ); + return Ok(ManagedFileOutcome::Preserved); + } + fs::write(config_path, rendered_config)?; + println!(" {} Created: {}", "✔".green(), config_path.display()); + + let selected_skill_agents = skills_choices + .iter() + .map(|choice| choice.agent_name.clone()) + .collect::>(); + let warnings = + collect_post_init_skills_warnings(project_root, config_path, &selected_skill_agents)?; + + println!("\n{}", "🔎 Post-init skills validation:".bold()); + if warnings.is_empty() { + println!( + " {} No skills mode mismatches detected for selected targets", + "✔".green() + ); + } else { + for warning in warnings { + println!(" {} {}", "⚠".yellow(), warning); + } + } + Ok(ManagedFileOutcome::Written) +} + +/// Back up original files after migration. Returns outcome. +fn perform_wizard_backup( + renderer: &mut impl InitWizardRenderer, + project_root: &Path, + agents_dir: &Path, + files_to_migrate: &[DiscoveredFile], + skills_choices: &[SkillsWizardChoice], + skills_modes: &std::collections::BTreeMap, + agents_md_outcome: &ManagedFileOutcome, +) -> Result { + use colored::Colorize; + if matches!(agents_md_outcome, ManagedFileOutcome::Preserved) { + return Ok(BackupOutcome::NotOffered); + } + if !renderer.confirm_backup_originals()? { + return Ok(BackupOutcome::Declined); + } + + let backup_dir = agents_dir.join("backup"); + fs::create_dir_all(&backup_dir)?; + let mut moved_count = 0; + + for file in files_to_migrate { + if matches!( + file.file_type, + AgentFileType::McpConfig + | AgentFileType::ZedSettings + | AgentFileType::CursorMcpConfig + | AgentFileType::CopilotMcpConfig + | AgentFileType::WindsurfMcpConfig + | AgentFileType::CodexConfig + | AgentFileType::RooMcpConfig + | AgentFileType::KiroMcpConfig + | AgentFileType::AmazonQMcpConfig + | AgentFileType::KilocodeMcpConfig + | AgentFileType::FactoryMcpConfig + | AgentFileType::OpenCodeConfig + | AgentFileType::Other + ) { + continue; + } + + let src_path = project_root.join(&file.path); + if !src_path.exists() { + continue; + } + + if let Some((agent_name, _)) = skills_choice_for_file_type(&file.file_type) { + let selected_mode = skills_modes + .get(agent_name) + .copied() + .unwrap_or(SyncType::Symlink); + let preserve_existing_layout = skills_choices.iter().any(|choice| { + choice.agent_name == agent_name + && choice.already_canonical + && selected_mode == SyncType::Symlink + }); + if preserve_existing_layout { + continue; + } + } + + let backup_path = backup_dir.join(&file.path); + if let Some(parent) = backup_path.parent() { + fs::create_dir_all(parent)?; + } + + match fs::rename(&src_path, &backup_path) { + Ok(_) => { + println!(" {} Moved: {}", "✔".green(), file.path.display()); + moved_count += 1; + } + Err(_) => { + if src_path.is_dir() { + copy_dir_all(&src_path, &backup_path)?; + fs::remove_dir_all(&src_path)?; + } else { + fs::copy(&src_path, &backup_path)?; + fs::remove_file(&src_path)?; + } + println!(" {} Moved: {}", "✔".green(), file.path.display()); + moved_count += 1; + } + } + } + + Ok(BackupOutcome::Completed { moved_count }) +} pub fn init_wizard(project_root: &Path, force: bool, template_path: Option<&Path>) -> Result<()> { use colored::Colorize; use dialoguer::{Confirm, MultiSelect, Select, theme::ColorfulTheme}; @@ -1919,386 +2189,49 @@ pub fn init_wizard(project_root: &Path, force: bool, template_path: Option<&Path }) .collect(); - // Determine how to handle instruction files - let mut migrated_content: Option = None; - let mut instruction_files_merged = 0; - - if instruction_files.len() > 1 { - // Multiple instruction files - merge them with section headings - let mut merged = String::new(); - for file in &instruction_files { - let src_path = project_root.join(&file.path); - let content = fs::read_to_string(&src_path) - .map_err(|e| anyhow::anyhow!("Failed to read '{}': {}", src_path.display(), e))?; - if !merged.is_empty() { - merged.push_str("\n\n---\n\n"); - } - merged.push_str(&format!( - "# Instructions from {}\n\n{}", - file.path.display(), - content - )); - instruction_files_merged += 1; - } - if !merged.is_empty() { - migrated_content = Some(merged); - } - } else if instruction_files.len() == 1 { - // Single instruction file - use its content directly - let src_path = project_root.join(&instruction_files[0].path); - let content = fs::read_to_string(&src_path) - .map_err(|e| anyhow::anyhow!("Failed to read '{}': {}", src_path.display(), e))?; - migrated_content = Some(content); - instruction_files_merged = 1; - } + let (migrated_content, instruction_files_merged) = + merge_instruction_files(project_root, &instruction_files)?; // Track migration counts let mut files_actually_migrated = 0; let mut files_skipped = 0; for file in &files_to_migrate { - let src_path = project_root.join(&file.path); - - match file.file_type { - // Plain-text instruction files — content already merged into AGENTS.md above - AgentFileType::ClaudeInstructions - | AgentFileType::RootAgentsFile - | AgentFileType::CopilotInstructions - | AgentFileType::WindsurfRules - | AgentFileType::ClineRules - | AgentFileType::CrushInstructions - | AgentFileType::AmpInstructions - | AgentFileType::GooseHints - | AgentFileType::WarpInstructions - | AgentFileType::GeminiInstructions => { - // Already handled above — content merged into AGENTS.md - continue; - } - // Skill directories — copy contents into .agents/skills/ - AgentFileType::ClaudeSkills - | AgentFileType::CursorSkills - | AgentFileType::CodexSkills - | AgentFileType::GeminiSkills - | AgentFileType::OpenCodeSkills - | AgentFileType::RooSkills - | AgentFileType::FactorySkills - | AgentFileType::VibeSkills - | AgentFileType::AntigravitySkills => { - if src_path.exists() && src_path.is_dir() { - for entry in fs::read_dir(&src_path)? { - let entry = entry?; - let entry_path = entry.path(); - let skill_name = entry.file_name(); - let dest_skill = skills_dir.join(&skill_name); - if dest_skill.exists() { - println!( - " {} Skipped: skill '{}' already exists in .agents/skills/", - "⚠".yellow(), - skill_name.to_string_lossy() - ); - files_skipped += 1; - } else if entry_path.is_dir() { - copy_dir_all(&entry_path, &dest_skill)?; - println!( - " {} Copied skill: {} → .agents/skills/{}", - "✔".green(), - entry_path.display(), - skill_name.to_string_lossy() - ); - files_actually_migrated += 1; - } else { - fs::copy(&entry_path, &dest_skill)?; - println!( - " {} Copied skill: {} → .agents/skills/{}", - "✔".green(), - entry_path.display(), - skill_name.to_string_lossy() - ); - files_actually_migrated += 1; - } - } - } - } - // Command directories — copy contents into .agents/commands/ - AgentFileType::ClaudeCommands - | AgentFileType::GeminiCommands - | AgentFileType::OpenCodeCommands => { - if src_path.exists() && src_path.is_dir() { - for entry in fs::read_dir(&src_path)? { - let entry = entry?; - let entry_path = entry.path(); - let cmd_name = entry.file_name(); - let dest_cmd = commands_dir.join(&cmd_name); - if dest_cmd.exists() { - println!( - " {} Skipped: command '{}' already exists in .agents/commands/", - "⚠".yellow(), - cmd_name.to_string_lossy() - ); - files_skipped += 1; - } else if entry_path.is_dir() { - copy_dir_all(&entry_path, &dest_cmd)?; - println!( - " {} Copied command: {} → .agents/commands/{}", - "✔".green(), - entry_path.display(), - cmd_name.to_string_lossy() - ); - files_actually_migrated += 1; - } else { - fs::copy(&entry_path, &dest_cmd)?; - println!( - " {} Copied command: {} → .agents/commands/{}", - "✔".green(), - entry_path.display(), - cmd_name.to_string_lossy() - ); - files_actually_migrated += 1; - } - } - } - } - // Directories — copy to .agents/ - AgentFileType::CursorDirectory - | AgentFileType::WindsurfDirectory - | AgentFileType::AntigravityRules - | AgentFileType::AmazonQRules - | AgentFileType::OpenHandsMicroagents - | AgentFileType::JunieDirectory - | AgentFileType::AugmentRules - | AgentFileType::KilocodeDirectory - | AgentFileType::QwenDirectory - | AgentFileType::RooRules - | AgentFileType::TraeRules - | AgentFileType::KiroSteering - | AgentFileType::FactoryDirectory - | AgentFileType::VibeDirectory - | AgentFileType::JetBrainsRules => { - if src_path.exists() { - // Derive a sane destination under .agents/ - let dest_path = agents_dir.join(&file.path); - if let Some(parent) = dest_path.parent() { - fs::create_dir_all(parent)?; - } - copy_dir_all(&src_path, &dest_path)?; - let dest_display = dest_path - .strip_prefix(project_root) - .unwrap_or(&dest_path) - .display(); - println!( - " {} Copied: {} → {}", - "✔".green(), - file.path.display(), - dest_display - ); - files_actually_migrated += 1; - } - } - // Single-file configs — copy file (handles both root-level and nested files) - AgentFileType::AiderConfig - | AgentFileType::FirebenderConfig - | AgentFileType::FirebaseRules => { - if src_path.exists() { - let dest_path = agents_dir.join(&file.path); - if let Some(parent) = dest_path.parent() { - fs::create_dir_all(parent)?; - } - fs::copy(&src_path, &dest_path)?; - let dest_display = dest_path - .strip_prefix(project_root) - .unwrap_or(&dest_path) - .display(); - println!( - " {} Copied: {} → {}", - "✔".green(), - file.path.display(), - dest_display - ); - files_actually_migrated += 1; - } - } - // MCP / tooling configs — just note them - AgentFileType::McpConfig - | AgentFileType::ZedSettings - | AgentFileType::CursorMcpConfig - | AgentFileType::CopilotMcpConfig - | AgentFileType::WindsurfMcpConfig - | AgentFileType::CodexConfig - | AgentFileType::RooMcpConfig - | AgentFileType::KiroMcpConfig - | AgentFileType::AmazonQMcpConfig - | AgentFileType::KilocodeMcpConfig - | AgentFileType::FactoryMcpConfig - | AgentFileType::OpenCodeConfig => { - println!( - " {} Note: {} detected. You can configure MCP servers in agentsync.toml", - "ℹ".blue(), - file.path.display() - ); - files_skipped += 1; - } - AgentFileType::Other => { - files_skipped += 1; - } - } + let (m, s) = migrate_file(file, project_root, &agents_dir, &skills_dir, &commands_dir)?; + files_actually_migrated += m; + files_skipped += s; } // Create AGENTS.md with migrated content let agents_md_path = agents_dir.join("AGENTS.md"); - let agents_md_outcome = if let Some(content) = migrated_content { - if agents_md_path.exists() && !force { - println!( - " {} AGENTS.md already exists (use --force to overwrite)", - "!".yellow() - ); - ManagedFileOutcome::Preserved - } else { - let rendered_agents_md = upsert_agent_config_layout_block(&content, &layout_block); - fs::write(&agents_md_path, rendered_agents_md)?; - if instruction_files_merged > 1 { - println!( - " {} Created: {} (merged {} instruction files)", - "✔".green(), - agents_md_path.display(), - instruction_files_merged - ); - } else { - println!( - " {} Created: {} (with migrated content)", - "✔".green(), - agents_md_path.display() - ); - } - ManagedFileOutcome::Written - } - } else if !agents_md_path.exists() || force { - let rendered_agents_md = upsert_agent_config_layout_block(DEFAULT_AGENTS_MD, &layout_block); - fs::write(&agents_md_path, rendered_agents_md)?; - println!(" {} Created: {}", "✔".green(), agents_md_path.display()); - ManagedFileOutcome::Written - } else { - ManagedFileOutcome::Preserved - }; + let agents_md_outcome = write_agents_md( + &agents_md_path, + migrated_content, + &layout_block, + instruction_files_merged, + force, + )?; // Generate config file println!("\n{}", "⚙️ Generating configuration...".cyan()); - - let config_outcome = if config_path.exists() && !force { - println!( - " {} Config already exists: {} (use --force to overwrite)", - "!".yellow(), - config_path.display() - ); - ManagedFileOutcome::Preserved - } else { - fs::write(&config_path, &rendered_config)?; - println!(" {} Created: {}", "✔".green(), config_path.display()); - - let selected_skill_agents = skills_choices - .iter() - .map(|choice| choice.agent_name.clone()) - .collect::>(); - let warnings = - collect_post_init_skills_warnings(project_root, &config_path, &selected_skill_agents)?; - - println!("\n{}", "🔎 Post-init skills validation:".bold()); - if warnings.is_empty() { - println!( - " {} No skills mode mismatches detected for selected targets", - "✔".green() - ); - } else { - for warning in warnings { - println!(" {} {}", "⚠".yellow(), warning); - } - } - ManagedFileOutcome::Written - }; - - // Ask if user wants to back up original files. - // Only offer backup when AGENTS.md was actually written — otherwise the - // instruction files would be moved without a migrated destination existing. - let backup_outcome = if matches!(agents_md_outcome, ManagedFileOutcome::Preserved) { - BackupOutcome::NotOffered - } else { - if renderer.confirm_backup_originals()? { - let backup_dir = agents_dir.join("backup"); - fs::create_dir_all(&backup_dir)?; - let mut moved_count = 0; - - for file in &files_to_migrate { - if matches!( - file.file_type, - AgentFileType::McpConfig - | AgentFileType::ZedSettings - | AgentFileType::CursorMcpConfig - | AgentFileType::CopilotMcpConfig - | AgentFileType::WindsurfMcpConfig - | AgentFileType::CodexConfig - | AgentFileType::RooMcpConfig - | AgentFileType::KiroMcpConfig - | AgentFileType::AmazonQMcpConfig - | AgentFileType::KilocodeMcpConfig - | AgentFileType::FactoryMcpConfig - | AgentFileType::OpenCodeConfig - | AgentFileType::Other - ) { - // Skip files that weren't actually migrated - continue; - } - - let src_path = project_root.join(&file.path); - if !src_path.exists() { - continue; - } - - if let Some((agent_name, _)) = skills_choice_for_file_type(&file.file_type) { - let selected_mode = skills_modes - .get(agent_name) - .copied() - .unwrap_or(SyncType::Symlink); - let preserve_existing_layout = skills_choices.iter().any(|choice| { - choice.agent_name == agent_name - && choice.already_canonical - && selected_mode == SyncType::Symlink - }); - - if preserve_existing_layout { - continue; - } - } - - let backup_path = backup_dir.join(&file.path); - if let Some(parent) = backup_path.parent() { - fs::create_dir_all(parent)?; - } - - // Try to move the file/directory first (rename) - match fs::rename(&src_path, &backup_path) { - Ok(_) => { - println!(" {} Moved: {}", "✔".green(), file.path.display()); - moved_count += 1; - } - Err(_) => { - // Cross-filesystem or other error - fall back to copy then delete - if src_path.is_dir() { - copy_dir_all(&src_path, &backup_path)?; - fs::remove_dir_all(&src_path)?; - } else { - fs::copy(&src_path, &backup_path)?; - fs::remove_file(&src_path)?; - } - println!(" {} Moved: {}", "✔".green(), file.path.display()); - moved_count += 1; - } - } - } - - BackupOutcome::Completed { moved_count } - } else { - BackupOutcome::Declined - } - }; + let config_outcome = write_wizard_config( + project_root, + &config_path, + &rendered_config, + &skills_choices, + force, + )?; + + // Back up original files + let backup_outcome = perform_wizard_backup( + &mut renderer, + project_root, + &agents_dir, + &files_to_migrate, + &skills_choices, + &skills_modes, + &agents_md_outcome, + )?; println!("\n{}", "📋 Post-migration Summary:".bold()); let summary = render_wizard_post_migration_summary(&WizardSummaryFacts { diff --git a/src/linker.rs b/src/linker.rs index ad971bdf..3909d37a 100644 --- a/src/linker.rs +++ b/src/linker.rs @@ -18,6 +18,14 @@ use crate::config::{Config, SyncType, TargetConfig}; const COMPRESSED_AGENTS_MD_NAME: &str = "AGENTS.compact.md"; +/// Result of checking an existing symlink at a destination. +enum ExistingSymlinkAction { + /// Symlink already points to the correct target. + AlreadyCorrect, + /// Symlink was removed (or would be in dry-run) and needs recreation. + Updated, +} + type NestedGlobKey = (PathBuf, String, Vec); type NestedGlobMatches = Rc>; @@ -228,26 +236,7 @@ impl Linker { // SECURITY: Reject absolute paths if path.is_absolute() { - // If path is already absolute and under project_root, validate parent only - if !path.starts_with(&self.project_root) { - anyhow::bail!("Path is outside project root: {}", display_path); - } - // For absolute paths under project_root, validate the parent directory - if let Some(parent) = path.parent() { - let canonical_parent = self.canonicalize_uncached(parent).with_context(|| { - format!("Failed to canonicalize parent: {}", parent.display()) - })?; - - let canonical_root = self.get_canonical_project_root()?; - - if !canonical_parent.starts_with(&*canonical_root) { - anyhow::bail!( - "Path parent resolves outside project root: {}", - display_path - ); - } - } - return Ok(()); + return self.validate_absolute_unlink_path(path, &display_path); } // SECURITY: Reject paths with ParentDir components before resolution @@ -260,40 +249,66 @@ impl Linker { } } - // Validate that the parent directory (if any) is within project_root - // We canonicalize the parent but NOT the final component (which might be a symlink) + self.validate_relative_unlink_parent(path, &display_path) + } + + /// Validate an absolute path for unlinking: must be under project_root with valid parent. + fn validate_absolute_unlink_path(&self, path: &Path, display_path: &str) -> Result<()> { + if !path.starts_with(&self.project_root) { + anyhow::bail!("Path is outside project root: {}", display_path); + } if let Some(parent) = path.parent() { - if parent.as_os_str().is_empty() { - // Path has no parent (e.g., just a filename), it's relative to project_root - return Ok(()); + let canonical_parent = self + .canonicalize_uncached(parent) + .with_context(|| format!("Failed to canonicalize parent: {}", parent.display()))?; + + let canonical_root = self.get_canonical_project_root()?; + + if !canonical_parent.starts_with(&*canonical_root) { + anyhow::bail!( + "Path parent resolves outside project root: {}", + display_path + ); } + } + Ok(()) + } - let parent_absolute = if parent.is_absolute() { - parent.to_path_buf() - } else { - self.project_root.join(parent) - }; + /// Validate that the parent of a relative path is within project_root. + fn validate_relative_unlink_parent(&self, path: &Path, display_path: &str) -> Result<()> { + let Some(parent) = path.parent() else { + return Ok(()); + }; + if parent.as_os_str().is_empty() { + return Ok(()); + } - // Only canonicalize if the parent exists; for cleanup operations the parent might not exist yet - if parent_absolute.exists() { - let canonical_parent = - self.canonicalize_uncached(&parent_absolute) - .with_context(|| { - format!( - "Failed to canonicalize parent: {}", - parent_absolute.display() - ) - })?; + let parent_absolute = if parent.is_absolute() { + parent.to_path_buf() + } else { + self.project_root.join(parent) + }; - let canonical_root = self.get_canonical_project_root()?; + if !parent_absolute.exists() { + return Ok(()); + } - if !canonical_parent.starts_with(&*canonical_root) { - anyhow::bail!( - "Path parent resolves outside project root: {}", - display_path - ); - } - } + let canonical_parent = self + .canonicalize_uncached(&parent_absolute) + .with_context(|| { + format!( + "Failed to canonicalize parent: {}", + parent_absolute.display() + ) + })?; + + let canonical_root = self.get_canonical_project_root()?; + + if !canonical_parent.starts_with(&*canonical_root) { + anyhow::bail!( + "Path parent resolves outside project root: {}", + display_path + ); } Ok(()) @@ -653,63 +668,18 @@ impl Linker { // Handle existing destination if dest.is_symlink() { - let current_target = fs::read_link(dest)?; - if current_target == relative_source { - if options.verbose { - println!(" {} Already linked: {}", "✔".green(), dest.display()); + let action = self.handle_existing_symlink(dest, &relative_source, options)?; + match action { + ExistingSymlinkAction::AlreadyCorrect => { + result.skipped += 1; + return Ok(result); } - result.skipped += 1; - return Ok(result); - } else { - // Wrong target, remove and recreate - if options.dry_run { - println!( - " {} Would update symlink: {} -> {}", - "→".cyan(), - dest.display(), - relative_source.display() - ); - } else { - // SECURITY: Use revalidate_unlink_path here (not revalidate_path) so that a - // symlink whose target points outside project_root can still be safely removed. - // revalidate_path would canonicalize through the symlink and reject the path. - self.revalidate_unlink_path(dest)?; - remove_symlink(dest)?; - self.invalidate_path_cache(); - if options.verbose { - println!( - " {} Removed old symlink: {} (was -> {})", - "○".yellow(), - dest.display(), - current_target.display() - ); - } + ExistingSymlinkAction::Updated => { + result.updated += 1; } - result.updated += 1; } } else if dest.exists() { - // It's a regular file/directory - back it up - if options.dry_run { - println!( - " {} Would backup and replace: {}", - "→".cyan(), - dest.display() - ); - } else { - let backup = backup_path_for_destination(dest); - self.revalidate_path(dest)?; - self.revalidate_path(&backup)?; - remove_existing_path(&backup)?; - fs::rename(dest, &backup)?; - self.invalidate_path_cache(); - self.invalidate_glob_cache(); - println!( - " {} Backed up: {} -> {}", - "!".yellow(), - dest.display(), - backup.display() - ); - } + self.backup_existing_destination(dest, options)?; result.updated += 1; } else { result.created += 1; @@ -753,6 +723,71 @@ impl Linker { Ok(result) } + /// Handle an existing symlink at the destination. Returns the action taken. + fn handle_existing_symlink( + &self, + dest: &Path, + relative_source: &Path, + options: &SyncOptions, + ) -> Result { + let current_target = fs::read_link(dest)?; + if current_target == relative_source { + if options.verbose { + println!(" {} Already linked: {}", "✔".green(), dest.display()); + } + return Ok(ExistingSymlinkAction::AlreadyCorrect); + } + + // Wrong target, remove and recreate + if options.dry_run { + println!( + " {} Would update symlink: {} -> {}", + "→".cyan(), + dest.display(), + relative_source.display() + ); + } else { + self.revalidate_unlink_path(dest)?; + remove_symlink(dest)?; + self.invalidate_path_cache(); + if options.verbose { + println!( + " {} Removed old symlink: {} (was -> {})", + "○".yellow(), + dest.display(), + current_target.display() + ); + } + } + Ok(ExistingSymlinkAction::Updated) + } + + /// Back up an existing regular file/directory at the destination. + fn backup_existing_destination(&self, dest: &Path, options: &SyncOptions) -> Result<()> { + if options.dry_run { + println!( + " {} Would backup and replace: {}", + "→".cyan(), + dest.display() + ); + } else { + let backup = backup_path_for_destination(dest); + self.revalidate_path(dest)?; + self.revalidate_path(&backup)?; + remove_existing_path(&backup)?; + fs::rename(dest, &backup)?; + self.invalidate_path_cache(); + self.invalidate_glob_cache(); + println!( + " {} Backed up: {} -> {}", + "!".yellow(), + dest.display(), + backup.display() + ); + } + Ok(()) + } + /// Create symlinks for all contents of a directory fn create_symlinks_for_contents( &self, @@ -1258,152 +1293,21 @@ impl Linker { for target_config in agent_config.targets.values() { match target_config.sync_type { SyncType::NestedGlob => { - // SECURITY: Validate destination template for traversal/absolute paths. - if self - .ensure_safe_destination(&target_config.destination) - .is_err() - { - continue; - } - - // Re-discover the same files and remove the corresponding symlinks. - let search_root = self.project_root.join(&target_config.source); - // SECURITY: Validate search root to prevent traversal/absolute escapes. - if self.revalidate_path(&search_root).is_err() { - continue; - } - if !search_root.exists() || !search_root.is_dir() { - continue; - } - let glob_pattern = - target_config.pattern.as_deref().unwrap_or("**/AGENTS.md"); - let dest_template = &target_config.destination; - let excludes = &target_config.exclude; - - let matches = self.get_nested_glob_matches( - &search_root, - glob_pattern, - excludes, - options, - )?; - - for (_, rel_path) in matches.iter() { - let dest_str = - Self::expand_destination_template(dest_template, rel_path); - if dest_str.is_empty() { - continue; - } - - let dest = match self.ensure_safe_destination(&dest_str) { - Ok(dest) => dest, - Err(_) => continue, - }; - if dest.is_symlink() { - if options.dry_run { - println!(" {} Would remove: {}", "→".cyan(), dest.display()); - } else { - self.revalidate_unlink_path(&dest)?; - fs::remove_file(&dest)?; - self.invalidate_path_cache(); - println!(" {} Removed: {}", "✔".green(), dest.display()); - } - result.removed += 1; - } - } + self.clean_nested_glob_target(target_config, options, &mut result)?; } SyncType::SymlinkContents => { - let dest = match self.ensure_safe_destination(&target_config.destination) { - Ok(d) => d, - Err(_) => continue, - }; - if dest.is_dir() { - // For symlink-contents, remove symlinks inside the directory - for entry in fs::read_dir(&dest).with_context(|| { - format!("Failed to read destination directory: {}", dest.display()) - })? { - let entry = entry.with_context(|| { - format!("Failed to read entry in: {}", dest.display()) - })?; - if entry.path().is_symlink() { - if options.dry_run { - println!( - " {} Would remove: {}", - "→".cyan(), - entry.path().display() - ); - } else { - self.revalidate_unlink_path(&entry.path())?; - fs::remove_file(entry.path())?; - self.invalidate_path_cache(); - println!( - " {} Removed: {}", - "✔".green(), - entry.path().display() - ); - } - result.removed += 1; - } - } - // Try to remove the directory if empty - if !options.dry_run { - self.revalidate_unlink_path(&dest)?; - let _ = fs::remove_dir(&dest); - } - } + self.clean_symlink_contents_target(target_config, options, &mut result)?; } SyncType::Symlink => { - let dest = match self.ensure_safe_destination(&target_config.destination) { - Ok(d) => d, - Err(_) => continue, - }; - if dest.is_symlink() { - if options.dry_run { - println!(" {} Would remove: {}", "→".cyan(), dest.display()); - } else { - // SECURITY: Use revalidate_unlink_path so a symlink whose target - // points outside project_root can still be removed safely. - self.revalidate_unlink_path(&dest)?; - remove_symlink(&dest)?; - self.invalidate_path_cache(); - println!(" {} Removed: {}", "✔".green(), dest.display()); - } - result.removed += 1; - } + self.clean_symlink_target(target_config, options, &mut result)?; } SyncType::ModuleMap => { - for mapping in &target_config.mappings { - let filename = - crate::config::resolve_module_map_filename(mapping, agent_name); - - // SECURITY: Validate that the joined destination (dir + filename) is safe. - let dest_str = format!("{}/{}", mapping.destination, filename); - let dest = match self.ensure_safe_destination(&dest_str) { - Ok(d) => d, - Err(e) => { - if options.verbose { - println!( - " {} Skipping mapping {}: {}", - "!".yellow(), - mapping.source, - e - ); - } - continue; - } - }; - - if dest.is_symlink() { - if options.dry_run { - println!(" {} Would remove: {}", "→".cyan(), dest.display()); - } else { - self.revalidate_unlink_path(&dest)?; - fs::remove_file(&dest)?; - self.invalidate_path_cache(); - println!(" {} Removed: {}", "✔".green(), dest.display()); - } - result.removed += 1; - } - } + self.clean_module_map_target( + agent_name, + target_config, + options, + &mut result, + )?; } } } @@ -1412,6 +1316,165 @@ impl Linker { Ok(result) } + /// Clean a single symlink target. + fn clean_symlink_target( + &self, + target_config: &TargetConfig, + options: &SyncOptions, + result: &mut SyncResult, + ) -> Result<()> { + let dest = match self.ensure_safe_destination(&target_config.destination) { + Ok(d) => d, + Err(_) => return Ok(()), + }; + if dest.is_symlink() { + if options.dry_run { + println!(" {} Would remove: {}", "→".cyan(), dest.display()); + } else { + self.revalidate_unlink_path(&dest)?; + remove_symlink(&dest)?; + self.invalidate_path_cache(); + println!(" {} Removed: {}", "✔".green(), dest.display()); + } + result.removed += 1; + } + Ok(()) + } + + /// Clean symlink-contents: remove symlinks inside the destination directory. + fn clean_symlink_contents_target( + &self, + target_config: &TargetConfig, + options: &SyncOptions, + result: &mut SyncResult, + ) -> Result<()> { + let dest = match self.ensure_safe_destination(&target_config.destination) { + Ok(d) => d, + Err(_) => return Ok(()), + }; + if !dest.is_dir() { + return Ok(()); + } + for entry in fs::read_dir(&dest) + .with_context(|| format!("Failed to read destination directory: {}", dest.display()))? + { + let entry = + entry.with_context(|| format!("Failed to read entry in: {}", dest.display()))?; + if entry.path().is_symlink() { + if options.dry_run { + println!(" {} Would remove: {}", "→".cyan(), entry.path().display()); + } else { + self.revalidate_unlink_path(&entry.path())?; + remove_symlink(&entry.path())?; + self.invalidate_path_cache(); + println!(" {} Removed: {}", "✔".green(), entry.path().display()); + } + result.removed += 1; + } + } + // Try to remove the directory if empty + if !options.dry_run { + self.revalidate_unlink_path(&dest)?; + let _ = fs::remove_dir(&dest); + } + Ok(()) + } + + /// Clean nested-glob targets: re-discover matched files and remove symlinks. + fn clean_nested_glob_target( + &self, + target_config: &TargetConfig, + options: &SyncOptions, + result: &mut SyncResult, + ) -> Result<()> { + if self + .ensure_safe_destination(&target_config.destination) + .is_err() + { + return Ok(()); + } + + let search_root = self.project_root.join(&target_config.source); + if self.revalidate_path(&search_root).is_err() { + return Ok(()); + } + if !search_root.exists() || !search_root.is_dir() { + return Ok(()); + } + let glob_pattern = target_config.pattern.as_deref().unwrap_or("**/AGENTS.md"); + let dest_template = &target_config.destination; + let excludes = &target_config.exclude; + + let matches = + self.get_nested_glob_matches(&search_root, glob_pattern, excludes, options)?; + + for (_, rel_path) in matches.iter() { + let dest_str = Self::expand_destination_template(dest_template, rel_path); + if dest_str.is_empty() { + continue; + } + + let dest = match self.ensure_safe_destination(&dest_str) { + Ok(dest) => dest, + Err(_) => continue, + }; + if dest.is_symlink() { + if options.dry_run { + println!(" {} Would remove: {}", "→".cyan(), dest.display()); + } else { + self.revalidate_unlink_path(&dest)?; + fs::remove_file(&dest)?; + self.invalidate_path_cache(); + println!(" {} Removed: {}", "✔".green(), dest.display()); + } + result.removed += 1; + } + } + Ok(()) + } + + /// Clean module-map targets: remove symlinks for each mapping. + fn clean_module_map_target( + &self, + agent_name: &str, + target_config: &TargetConfig, + options: &SyncOptions, + result: &mut SyncResult, + ) -> Result<()> { + for mapping in &target_config.mappings { + let filename = crate::config::resolve_module_map_filename(mapping, agent_name); + + let dest_str = format!("{}/{}", mapping.destination, filename); + let dest = match self.ensure_safe_destination(&dest_str) { + Ok(d) => d, + Err(e) => { + if options.verbose { + println!( + " {} Skipping mapping {}: {}", + "!".yellow(), + mapping.source, + e + ); + } + continue; + } + }; + + if dest.is_symlink() { + if options.dry_run { + println!(" {} Would remove: {}", "→".cyan(), dest.display()); + } else { + self.revalidate_unlink_path(&dest)?; + remove_symlink(&dest)?; + self.invalidate_path_cache(); + println!(" {} Removed: {}", "✔".green(), dest.display()); + } + result.removed += 1; + } + } + Ok(()) + } + /// Sync MCP configurations for enabled agents /// /// # Arguments @@ -1489,9 +1552,34 @@ fn compressed_agents_md_path(path: &Path) -> PathBuf { path.with_file_name(COMPRESSED_AGENTS_MD_NAME) } +/// Detect a code fence delimiter (``` or ~~~) at the start of a trimmed line. +fn detect_fence_delimiter(trimmed_start: &str) -> Option<&str> { + if trimmed_start.starts_with("```") { + let len = trimmed_start + .find(|c| c != '`') + .unwrap_or(trimmed_start.len()); + Some(&trimmed_start[..len]) + } else if trimmed_start.starts_with("~~~") { + let len = trimmed_start + .find(|c| c != '~') + .unwrap_or(trimmed_start.len()); + Some(&trimmed_start[..len]) + } else { + None + } +} + +/// Toggle fence state: open a new fence, close a matching one, or leave unchanged. +fn toggle_fence<'a>(current: Option<&'a str>, delim: &'a str) -> Option<&'a str> { + match current { + None => Some(delim), + Some(open) if open == delim => None, + other => other, + } +} + fn compress_agents_md_content(input: &str) -> String { let mut out = String::with_capacity(input.len()); - // Track the exact fence delimiter so only a matching fence closes the block. let mut fence_delim: Option<&str> = None; let mut previous_blank = false; @@ -1499,30 +1587,8 @@ fn compress_agents_md_content(input: &str) -> String { let trimmed_end = line.trim_end_matches([' ', '\t']); let trimmed_start = trimmed_end.trim_start(); - // Detect code fence delimiter (``` or ~~~) via slicing, no allocation. - // fence_delim_match borrows from trimmed_start, avoiding a new String. - let fence_delim_match = if trimmed_start.starts_with("```") { - let len = trimmed_start - .find(|c| c != '`') - .unwrap_or(trimmed_start.len()); - Some(&trimmed_start[..len]) - } else if trimmed_start.starts_with("~~~") { - let len = trimmed_start - .find(|c| c != '~') - .unwrap_or(trimmed_start.len()); - Some(&trimmed_start[..len]) - } else { - None - }; - let is_fence = fence_delim_match.is_some(); - - if is_fence { - let delim = fence_delim_match.expect("fence_delim_match is Some when is_fence is true"); - if fence_delim.is_none() { - fence_delim = Some(delim); - } else if fence_delim == Some(delim) { - fence_delim = None; - } + if let Some(delim) = detect_fence_delimiter(trimmed_start) { + fence_delim = toggle_fence(fence_delim, delim); out.push_str(trimmed_end); out.push('\n'); previous_blank = false; @@ -1660,24 +1726,14 @@ where let mut path_it_peek = path_it.clone(); match path_it_peek.next() { Some(s) => { - if pat_idx < pattern.len() && pattern[pat_idx] == "**" { - // ** matches zero or more segments - backtrack_pat_idx = Some(pat_idx); - backtrack_path_it = Some(path_it.clone()); - pat_idx += 1; - } else if pat_idx < pattern.len() && matches_pattern(s, pattern[pat_idx]) { - path_it.next(); // Consume segment - pat_idx += 1; - } else if let (Some(b_pat_idx), Some(b_path_it)) = - (backtrack_pat_idx, backtrack_path_it.as_mut()) - { - // Backtrack: last ** matches one more segment - if b_path_it.next().is_none() { - return false; - } - path_it = b_path_it.clone(); - pat_idx = b_pat_idx + 1; - } else { + if !try_match_segment( + s, + &mut path_it, + pattern, + &mut pat_idx, + &mut backtrack_path_it, + &mut backtrack_pat_idx, + ) { return false; } } @@ -1692,6 +1748,42 @@ where } } +/// Process a single path segment against the current pattern state. +/// Returns false if matching definitively fails, true to continue. +fn try_match_segment<'a, I>( + segment: &str, + path_it: &mut I, + pattern: &[&str], + pat_idx: &mut usize, + backtrack_path_it: &mut Option, + backtrack_pat_idx: &mut Option, +) -> bool +where + I: Iterator + Clone, +{ + if *pat_idx < pattern.len() && pattern[*pat_idx] == "**" { + // ** matches zero or more segments + *backtrack_pat_idx = Some(*pat_idx); + *backtrack_path_it = Some(path_it.clone()); + *pat_idx += 1; + } else if *pat_idx < pattern.len() && matches_pattern(segment, pattern[*pat_idx]) { + path_it.next(); // Consume segment + *pat_idx += 1; + } else if let (Some(b_pat_idx), Some(b_path_it)) = + (*backtrack_pat_idx, backtrack_path_it.as_mut()) + { + // Backtrack: last ** matches one more segment + if b_path_it.next().is_none() { + return false; + } + *path_it = b_path_it.clone(); + *pat_idx = b_pat_idx + 1; + } else { + return false; + } + true +} + fn backup_path_for_destination(dest: &Path) -> PathBuf { // Performance: Use OsString::push to avoid string formatting and UTF-8 validation overhead. let mut os_string = dest.as_os_str().to_os_string(); @@ -4561,4 +4653,35 @@ mod tests { .exists() ); } + + #[test] + fn test_path_glob_match_iter_double_star_middle() { + // Pattern **/foo/**/bar should match a/foo/b/bar + let pattern = ["**", "foo", "**", "bar"]; + assert!(path_glob_match_iter("a/foo/b/bar".split('/'), &pattern)); + assert!(path_glob_match_iter("x/y/foo/z/w/bar".split('/'), &pattern)); + assert!(path_glob_match_iter("foo/bar".split('/'), &pattern)); + // Non-match: missing bar + assert!(!path_glob_match_iter("a/foo/b/baz".split('/'), &pattern)); + } + + #[test] + fn test_path_glob_match_iter_trailing_double_star_zero_segments() { + // Pattern foo/** should match "foo" (trailing ** matches zero segments) + let pattern = ["foo", "**"]; + assert!(path_glob_match_iter("foo".split('/'), &pattern)); + assert!(path_glob_match_iter("foo/bar".split('/'), &pattern)); + assert!(path_glob_match_iter("foo/bar/baz".split('/'), &pattern)); + } + + #[test] + fn test_path_glob_match_iter_non_matching() { + let pattern = ["src", "**", "*.rs"]; + assert!(!path_glob_match_iter("lib/foo.rs".split('/'), &pattern)); + assert!(!path_glob_match_iter("src/main.go".split('/'), &pattern)); + + let pattern2 = ["foo", "bar"]; + assert!(!path_glob_match_iter("foo/baz".split('/'), &pattern2)); + assert!(!path_glob_match_iter("foo".split('/'), &pattern2)); + } } diff --git a/src/main.rs b/src/main.rs index e8e7279c..d120b1e7 100644 --- a/src/main.rs +++ b/src/main.rs @@ -407,35 +407,7 @@ fn main() -> Result<()> { wizard, experimental_tui, template, - } => { - let project_root = path.unwrap_or_else(|| env::current_dir().unwrap()); - print_header(); - if wizard { - println!( - "{}", - "Starting interactive configuration wizard...\n".cyan() - ); - if experimental_tui { - init::init_wizard_experimental_tui(&project_root, force, template.as_deref())?; - } else { - init::init_wizard(&project_root, force, template.as_deref())?; - } - } else { - println!("{}", "Initializing agentsync configuration...\n".cyan()); - let (config_content, source) = init::resolve_config_template(template.as_deref())?; - if let Some(notice) = source.notice() { - use colored::Colorize; - println!(" {} {notice}", "✔".green()); - } - init::init(&project_root, force, &config_content)?; - } - println!("\n{}", "✨ Initialization complete!".green().bold()); - if let Some(lines) = init_next_steps_lines(wizard) { - for line in lines { - println!("{line}"); - } - } - } + } => handle_init(path, force, wizard, experimental_tui, template)?, Commands::Apply { path, config, @@ -444,128 +416,21 @@ fn main() -> Result<()> { verbose, agents, no_gitignore, - } => { - let start_dir = path.unwrap_or_else(|| env::current_dir().unwrap()); - print_header(); - let config_path = match config { - Some(p) => p, - None => Config::find_config(&start_dir)?, - }; - if verbose { - println!( - "Using config: {}\n", - config_path.display().to_string().dimmed() - ); - } - let config = Config::load(&config_path)?; - let linker = Linker::new(config, config_path); - let use_color = human_use_color(); - if dry_run { - print_lines(&render_dry_run_notice(use_color)); - println!(); - } - let clean_result = if clean { - print_lines(&render_clean_phase_with_color(dry_run, use_color)); - let clean_opts = SyncOptions { - dry_run, - verbose, - ..Default::default() - }; - let clean_result = linker.clean(&clean_opts)?; - println!(); - Some(clean_result) - } else { - None - }; - print_lines(&render_sync_phase_with_color(dry_run, clean, use_color)); - let options = SyncOptions { - clean: false, - dry_run, - verbose, - agents, - }; - let mut result = linker.sync(&options)?; - if let Some(clean_result) = &clean_result { - merge_clean_result_into_apply_result(&mut result, clean_result); - } - if !no_gitignore { - if linker.config().gitignore.enabled { - println!(); - print_lines(&render_gitignore_phase_with_color(true, dry_run, use_color)); - let entries = linker.config().all_gitignore_entries(); - gitignore::update_gitignore( - linker.project_root(), - &linker.config().gitignore.marker, - &entries, - dry_run, - )?; - } else { - println!(); - print_lines(&render_gitignore_phase_with_color( - false, dry_run, use_color, - )); - gitignore::cleanup_gitignore( - linker.project_root(), - &linker.config().gitignore.marker, - dry_run, - )?; - } - } - if linker.config().mcp.enabled && !linker.config().mcp_servers.is_empty() { - println!(); - print_lines(&render_mcp_phase(dry_run, use_color)); - match linker.sync_mcp(dry_run, options.agents.as_ref()) { - Ok(mcp_result) => { - if mcp_result.created > 0 - || mcp_result.updated > 0 - || mcp_result.skipped > 0 - || mcp_result.errors > 0 - { - print_lines(&render_mcp_summary_with_color(&mcp_result, use_color)); - } - } - Err(e) => { - tracing::error!(%e, "Error syncing MCP configs"); - result.errors += 1; - } - } - } - println!(); - print_lines(&render_apply_summary_with_color( - dry_run, &result, use_color, - )); - } + } => handle_apply(ApplyArgs { + path, + config, + clean, + dry_run, + verbose, + agents, + no_gitignore, + })?, Commands::Clean { path, config, dry_run, verbose, - } => { - let start_dir = path.unwrap_or_else(|| env::current_dir().unwrap()); - print_header(); - let config_path = match config { - Some(p) => p, - None => Config::find_config(&start_dir)?, - }; - let config = Config::load(&config_path)?; - let linker = Linker::new(config, config_path); - let use_color = human_use_color(); - if dry_run { - print_lines(&render_dry_run_notice(use_color)); - println!(); - } - print_lines(&render_clean_phase_with_color(dry_run, use_color)); - let options = SyncOptions { - dry_run, - verbose, - ..Default::default() - }; - let result = linker.clean(&options)?; - println!(); - print_lines(&render_clean_summary_with_color( - dry_run, &result, use_color, - )); - } + } => handle_clean(path, config, dry_run, verbose)?, Commands::DevInstall { skill_id, json } => { let project_root = env::current_dir().unwrap(); use commands::skill::SkillInstallArgs; @@ -581,6 +446,207 @@ fn main() -> Result<()> { Ok(()) } +fn handle_init( + path: Option, + force: bool, + wizard: bool, + experimental_tui: bool, + template: Option, +) -> Result<()> { + let project_root = path.unwrap_or_else(|| env::current_dir().unwrap()); + print_header(); + if wizard { + println!( + "{}", + "Starting interactive configuration wizard...\n".cyan() + ); + if experimental_tui { + init::init_wizard_experimental_tui(&project_root, force, template.as_deref())?; + } else { + init::init_wizard(&project_root, force, template.as_deref())?; + } + } else { + println!("{}", "Initializing agentsync configuration...\n".cyan()); + let (config_content, source) = init::resolve_config_template(template.as_deref())?; + if let Some(notice) = source.notice() { + use colored::Colorize; + println!(" {} {notice}", "✔".green()); + } + init::init(&project_root, force, &config_content)?; + } + println!("\n{}", "✨ Initialization complete!".green().bold()); + if let Some(lines) = init_next_steps_lines(wizard) { + for line in lines { + println!("{line}"); + } + } + Ok(()) +} + +struct ApplyArgs { + path: Option, + config: Option, + clean: bool, + dry_run: bool, + verbose: bool, + agents: Option>, + no_gitignore: bool, +} + +fn handle_apply(args: ApplyArgs) -> Result<()> { + let start_dir = args.path.unwrap_or_else(|| env::current_dir().unwrap()); + print_header(); + let config_path = match args.config { + Some(p) => p, + None => Config::find_config(&start_dir)?, + }; + if args.verbose { + println!( + "Using config: {}\n", + config_path.display().to_string().dimmed() + ); + } + let config = Config::load(&config_path)?; + let linker = Linker::new(config, config_path); + let use_color = human_use_color(); + if args.dry_run { + print_lines(&render_dry_run_notice(use_color)); + println!(); + } + let clean_result = if args.clean { + print_lines(&render_clean_phase_with_color(args.dry_run, use_color)); + let clean_opts = SyncOptions { + dry_run: args.dry_run, + verbose: args.verbose, + ..Default::default() + }; + let clean_result = linker.clean(&clean_opts)?; + println!(); + Some(clean_result) + } else { + None + }; + print_lines(&render_sync_phase_with_color( + args.dry_run, + args.clean, + use_color, + )); + let options = SyncOptions { + clean: false, + dry_run: args.dry_run, + verbose: args.verbose, + agents: args.agents, + }; + let mut result = linker.sync(&options)?; + if let Some(clean_result) = &clean_result { + merge_clean_result_into_apply_result(&mut result, clean_result); + } + if !args.no_gitignore { + handle_apply_gitignore(&linker, args.dry_run, use_color)?; + } + if linker.config().mcp.enabled && !linker.config().mcp_servers.is_empty() { + handle_apply_mcp( + &linker, + options.dry_run, + use_color, + options.agents.as_ref(), + &mut result, + )?; + } + println!(); + print_lines(&render_apply_summary_with_color( + options.dry_run, + &result, + use_color, + )); + Ok(()) +} + +fn handle_apply_gitignore(linker: &Linker, dry_run: bool, use_color: bool) -> Result<()> { + if linker.config().gitignore.enabled { + println!(); + print_lines(&render_gitignore_phase_with_color(true, dry_run, use_color)); + let entries = linker.config().all_gitignore_entries(); + gitignore::update_gitignore( + linker.project_root(), + &linker.config().gitignore.marker, + &entries, + dry_run, + )?; + } else { + println!(); + print_lines(&render_gitignore_phase_with_color( + false, dry_run, use_color, + )); + gitignore::cleanup_gitignore( + linker.project_root(), + &linker.config().gitignore.marker, + dry_run, + )?; + } + Ok(()) +} + +fn handle_apply_mcp( + linker: &Linker, + dry_run: bool, + use_color: bool, + agents: Option<&Vec>, + result: &mut SyncResult, +) -> Result<()> { + println!(); + print_lines(&render_mcp_phase(dry_run, use_color)); + match linker.sync_mcp(dry_run, agents) { + Ok(mcp_result) => { + if mcp_result.created > 0 + || mcp_result.updated > 0 + || mcp_result.skipped > 0 + || mcp_result.errors > 0 + { + print_lines(&render_mcp_summary_with_color(&mcp_result, use_color)); + } + } + Err(e) => { + tracing::error!(%e, "Error syncing MCP configs"); + result.errors += 1; + } + } + Ok(()) +} + +fn handle_clean( + path: Option, + config: Option, + dry_run: bool, + verbose: bool, +) -> Result<()> { + let start_dir = path.unwrap_or_else(|| env::current_dir().unwrap()); + print_header(); + let config_path = match config { + Some(p) => p, + None => Config::find_config(&start_dir)?, + }; + let config = Config::load(&config_path)?; + let linker = Linker::new(config, config_path); + let use_color = human_use_color(); + if dry_run { + print_lines(&render_dry_run_notice(use_color)); + println!(); + } + print_lines(&render_clean_phase_with_color(dry_run, use_color)); + let options = SyncOptions { + dry_run, + verbose, + ..Default::default() + }; + let result = linker.clean(&options)?; + println!(); + print_lines(&render_clean_summary_with_color( + dry_run, &result, use_color, + )); + Ok(()) +} + fn print_header() { let banner = include_str!("banner.txt"); println!("{}", banner.cyan().bold()); diff --git a/src/mcp.rs b/src/mcp.rs index d69e1fa4..1bdc1df5 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -1237,38 +1237,18 @@ impl McpGenerator { self.generate_for_agent_with_servers(agent, project_root, &enabled_servers, dry_run) } - /// Internal method to generate config using pre-calculated enabled servers - fn generate_for_agent_with_servers( + /// Resolve the content to write for an MCP config file, returning (content, existing_content). + fn resolve_config_content( &self, - agent: McpAgent, - project_root: &Path, + formatter: &dyn McpFormatter, + config_path: &Path, enabled_servers: &BTreeMap<&str, &McpServerConfig>, - dry_run: bool, - ) -> Result { - let mut result = McpSyncResult::default(); - let formatter = agent.formatter(); - let config_path = match agent.resolved_config_path(project_root) { - Some(path) => path, - None => { - result.skipped += 1; - return Ok(result); - } - }; - - if enabled_servers.is_empty() { - result.skipped += 1; - return Ok(result); - } - - let mut existing_content = None; - - // Determine content to write - let content = if config_path.exists() && self.merge_strategy == McpMergeStrategy::Merge { - let existing = fs::read_to_string(&config_path).with_context(|| { + ) -> Result<(String, Option)> { + if config_path.exists() && self.merge_strategy == McpMergeStrategy::Merge { + let existing = fs::read_to_string(config_path).with_context(|| { format!("Failed to read existing config: {}", config_path.display()) })?; - // Check if we need to clean up removed servers let existing_servers = formatter.parse_existing(&existing)?; let removed_servers: Vec<&String> = existing_servers .keys() @@ -1276,72 +1256,51 @@ impl McpGenerator { .collect(); let merged = if !removed_servers.is_empty() { - // Only perform cleanup if the existing and enabled server counts differ. - // This prevents clobbering unrelated existing entries in simple merge cases - // where the counts match but names differ (keep existing entries). if existing_servers.len() != enabled_servers.len() { - // Use cleanup method to remove servers that are no longer in config formatter.cleanup_removed_servers(&existing, enabled_servers)? } else { - // Counts equal - prefer a simple merge to retain existing entries formatter.merge(&existing, enabled_servers)? } } else { - // No servers removed, use normal merge formatter.merge(&existing, enabled_servers)? }; - existing_content = Some(existing); - merged + Ok((merged, Some(existing))) } else if config_path.exists() && self.merge_strategy == McpMergeStrategy::Overwrite && formatter.preserve_on_overwrite() { - // Preserve unrelated top-level settings when overwriting for certain formatters - let existing = fs::read_to_string(&config_path).with_context(|| { + let existing = fs::read_to_string(config_path).with_context(|| { format!("Failed to read existing config: {}", config_path.display()) })?; - - // Use cleanup_removed_servers to replace mcp sections while preserving other keys let preserved = formatter.cleanup_removed_servers(&existing, enabled_servers)?; - existing_content = Some(existing); - preserved + Ok((preserved, Some(existing))) } else { - formatter.format_to_string(enabled_servers)? - }; - - // Create parent directories if needed - if let Some(parent) = config_path.parent().filter(|p| !p.exists()) { - if dry_run { - println!( - " {} Would create directory: {}", - "→".cyan(), - parent.display() - ); - } else { - fs::create_dir_all(parent)?; - } + Ok((formatter.format_to_string(enabled_servers)?, None)) } + } - // Check if content has changed before writing to avoid redundant I/O - let was_existing = config_path.exists(); - if was_existing { - let is_identical = if let Some(existing) = existing_content { - existing == content - } else { - fs::read_to_string(&config_path).is_ok_and(|existing| existing == content) - }; - - if is_identical { - // SECURITY: Even if content is identical, ensure permissions are correct (remediation). - if !dry_run && let Err(e) = set_restricted_permissions(&config_path) { - tracing::warn!(error = %e, path = %config_path.display(), "Failed to remediate restricted permissions on existing MCP config"); - } - result.skipped += 1; - return Ok(result); - } + /// Check if config content is identical to what's already on disk. + fn is_content_identical( + config_path: &Path, + content: &str, + existing_content: Option, + ) -> bool { + if let Some(existing) = existing_content { + existing == content + } else { + fs::read_to_string(config_path).is_ok_and(|existing| existing == content) } + } - // Write the file + /// Write config (or report what would be done in dry-run mode), returning result delta. + fn write_or_report_config( + &self, + config_path: &Path, + content: &str, + was_existing: bool, + dry_run: bool, + ) -> Result { + let mut result = McpSyncResult::default(); if dry_run { if was_existing { println!( @@ -1359,18 +1318,13 @@ impl McpGenerator { result.created += 1; } } else { - // SECURITY: Perform atomic write with restricted permissions to avoid a race condition - // where sensitive data is world-readable between creation and chmod. - self.write_atomic_secure(&config_path, &content)?; - - // Repair step for pre-existing files or if atomic write didn't set permissions (non-unix) - set_restricted_permissions(&config_path).with_context(|| { + self.write_atomic_secure(config_path, content)?; + set_restricted_permissions(config_path).with_context(|| { format!( "Failed to set restricted permissions on MCP config: {}", config_path.display() ) })?; - if was_existing { println!( " {} Updated MCP config: {}", @@ -1387,6 +1341,62 @@ impl McpGenerator { result.created += 1; } } + Ok(result) + } + + /// Internal method to generate config using pre-calculated enabled servers + fn generate_for_agent_with_servers( + &self, + agent: McpAgent, + project_root: &Path, + enabled_servers: &BTreeMap<&str, &McpServerConfig>, + dry_run: bool, + ) -> Result { + let mut result = McpSyncResult::default(); + let formatter = agent.formatter(); + let config_path = match agent.resolved_config_path(project_root) { + Some(path) => path, + None => { + result.skipped += 1; + return Ok(result); + } + }; + + if enabled_servers.is_empty() { + result.skipped += 1; + return Ok(result); + } + + let (content, existing_content) = + self.resolve_config_content(formatter.as_ref(), &config_path, enabled_servers)?; + + // Create parent directories if needed + if let Some(parent) = config_path.parent().filter(|p| !p.exists()) { + if dry_run { + println!( + " {} Would create directory: {}", + "→".cyan(), + parent.display() + ); + } else { + fs::create_dir_all(parent)?; + } + } + + // Check if content has changed before writing to avoid redundant I/O + let was_existing = config_path.exists(); + if was_existing && Self::is_content_identical(&config_path, &content, existing_content) { + if !dry_run && let Err(e) = set_restricted_permissions(&config_path) { + tracing::warn!(error = %e, path = %config_path.display(), "Failed to remediate restricted permissions on existing MCP config"); + } + result.skipped += 1; + return Ok(result); + } + + let write_result = + self.write_or_report_config(&config_path, &content, was_existing, dry_run)?; + result.created += write_result.created; + result.updated += write_result.updated; Ok(result) } diff --git a/src/skills/detect.rs b/src/skills/detect.rs index bb0a198f..4b65d58f 100644 --- a/src/skills/detect.rs +++ b/src/skills/detect.rs @@ -132,41 +132,13 @@ impl RepoMetadata { let relative_buf = relative.to_path_buf(); - if entry.file_type().is_dir() && entry.depth() == 1 { - root_dirs.push(relative_buf.clone()); - } - if entry.file_type().is_dir() { - dirs.insert(relative_buf.clone()); - } + Self::process_dir_entry(&entry, &relative_buf, &mut root_dirs, &mut dirs); if entry.file_type().is_file() { - let file_name = entry.file_name().to_str().unwrap_or(""); - - // Integrated Nested Project Discovery (issue #409) - if PROJECT_MANIFEST_FILES.contains(&file_name) - && let Some(dir) = relative.parent() - && !dir.as_os_str().is_empty() - { - let dir_name = dir.file_name().and_then(|n| n.to_str()).unwrap_or(""); - if !TEST_DIR_NAMES.contains(&dir_name) { - nested_projects.insert(dir.to_path_buf()); - } - } - - if let Some(ext) = relative.extension().and_then(|e| e.to_str()) { - // Optimization: Skip string formatting and map insertions if extension already recorded. - if !extensions.contains_key(ext) { - let dot_ext = format!(".{ext}"); - // Store first occurrence for deterministic evidence. - // Note: WalkDir sort_by_file_name() ensures deterministic choice if multiple exist. - extensions.insert(dot_ext, relative_buf.clone()); - extensions.insert(ext.to_string(), relative_buf.clone()); - } - } + Self::check_nested_project(relative, &mut nested_projects); + Self::record_extension(relative, &relative_buf, &mut extensions); } - // Optimization: Move the owned relative_buf into the paths set at the end - // of the iteration to avoid a clone() call for every file and directory. paths.insert(relative_buf); } @@ -178,6 +150,51 @@ impl RepoMetadata { nested_projects: nested_projects.into_iter().collect(), } } + + fn process_dir_entry( + entry: &walkdir::DirEntry, + relative_buf: &Path, + root_dirs: &mut Vec, + dirs: &mut HashSet, + ) { + if entry.file_type().is_dir() { + if entry.depth() == 1 { + root_dirs.push(relative_buf.to_path_buf()); + } + dirs.insert(relative_buf.to_path_buf()); + } + } + + fn check_nested_project(relative: &Path, nested_projects: &mut BTreeSet) { + let file_name = relative.file_name().and_then(|n| n.to_str()).unwrap_or(""); + if !PROJECT_MANIFEST_FILES.contains(&file_name) { + return; + } + let Some(dir) = relative.parent() else { return }; + if dir.as_os_str().is_empty() { + return; + } + let dir_name = dir.file_name().and_then(|n| n.to_str()).unwrap_or(""); + if !TEST_DIR_NAMES.contains(&dir_name) { + nested_projects.insert(dir.to_path_buf()); + } + } + + fn record_extension( + relative: &Path, + relative_buf: &Path, + extensions: &mut HashMap, + ) { + let Some(ext) = relative.extension().and_then(|e| e.to_str()) else { + return; + }; + if extensions.contains_key(ext) { + return; + } + let dot_ext = format!(".{ext}"); + extensions.insert(dot_ext, relative_buf.to_path_buf()); + extensions.insert(ext.to_string(), relative_buf.to_path_buf()); + } } /// Rules for detecting technologies by scanning file content. @@ -367,62 +384,61 @@ impl RepoDetector for CatalogDrivenDetector { } // Phase 2: Scan nested projects (issue #409) - for rel_nested_dir in &metadata.nested_projects { - let nested_dir = project_root.join(rel_nested_dir); - let nested_meta = RepoMetadata::collect(&nested_dir); - let nested_pkgs = collect_package_names(&nested_dir, &nested_meta, cache); + detect_nested_projects(project_root, &metadata, &self.rules, cache, &mut detections); + + Ok(detections) + } +} - let offset = Some(rel_nested_dir.to_path_buf()); +fn detect_nested_projects( + project_root: &Path, + metadata: &RepoMetadata, + rules: &[(TechnologyId, CompiledDetectionRules)], + cache: &mut ContentCache, + detections: &mut Vec, +) { + for rel_nested_dir in &metadata.nested_projects { + let nested_dir = project_root.join(rel_nested_dir); + let nested_meta = RepoMetadata::collect(&nested_dir); + let nested_pkgs = collect_package_names(&nested_dir, &nested_meta, cache); - for (tech_id, compiled) in &self.rules { - // Skip if already detected at root - if detections.iter().any(|d| d.technology == *tech_id) { - continue; - } + for (tech_id, compiled) in rules { + if detections.iter().any(|d| d.technology == *tech_id) { + continue; + } - if let Some(detection) = evaluate_rules( - &nested_dir, - tech_id, - compiled, - &nested_pkgs, - &nested_meta, - cache, - ) { - // Adjust paths: detections are relative to nested_dir, need to prepend offset - let adjusted = TechnologyDetection { - technology: detection.technology, - confidence: detection.confidence, - root_relative_paths: detection - .root_relative_paths - .iter() - .map(|p| { - if let Some(ref off) = offset { - off.join(p) - } else { - p.clone() - } - }) - .collect(), - evidence: detection - .evidence - .iter() - .map(|e| DetectionEvidence { - marker: e.marker.clone(), - path: if let Some(ref off) = offset { - off.join(&e.path) - } else { - e.path.clone() - }, - notes: e.notes.clone(), - }) - .collect(), - }; - detections.push(adjusted); - } + if let Some(detection) = evaluate_rules( + &nested_dir, + tech_id, + compiled, + &nested_pkgs, + &nested_meta, + cache, + ) { + detections.push(adjust_detection(detection, rel_nested_dir)); } } + } +} - Ok(detections) +fn adjust_detection(detection: TechnologyDetection, offset: &Path) -> TechnologyDetection { + TechnologyDetection { + technology: detection.technology, + confidence: detection.confidence, + root_relative_paths: detection + .root_relative_paths + .iter() + .map(|p| offset.join(p)) + .collect(), + evidence: detection + .evidence + .iter() + .map(|e| DetectionEvidence { + marker: e.marker.clone(), + path: offset.join(&e.path), + notes: e.notes.clone(), + }) + .collect(), } } @@ -434,88 +450,128 @@ fn evaluate_rules( metadata: &RepoMetadata, cache: &mut ContentCache, ) -> Option { - // Check packages (exact match) - if let Some(packages) = &rules.packages { - for package in packages { - if all_packages.contains(package) { - return Some(make_detection( - tech_id, - DetectionConfidence::High, - package, - &format!("package '{package}' found in dependencies"), - )); - } - } + if let Some(d) = check_exact_packages(tech_id, rules, all_packages) { + return Some(d); + } + if let Some(d) = check_package_patterns(tech_id, rules, all_packages) { + return Some(d); + } + if let Some(d) = check_config_files(tech_id, rules, project_root, metadata) { + return Some(d); } + if let Some(d) = check_config_file_content(tech_id, rules, project_root, metadata, cache) { + return Some(d); + } + check_file_extensions(tech_id, rules, metadata) +} - // Check package_patterns (regex match) - if let Some(patterns) = &rules.package_patterns { - for regex in patterns { - for package in all_packages { - if regex.is_match(package) { - return Some(make_detection( - tech_id, - DetectionConfidence::Medium, - package, - &format!("package '{package}' matches pattern '{regex}'"), - )); - } - } +fn check_exact_packages( + tech_id: &TechnologyId, + rules: &CompiledDetectionRules, + all_packages: &BTreeSet, +) -> Option { + let packages = rules.packages.as_ref()?; + for package in packages { + if all_packages.contains(package) { + return Some(make_detection( + tech_id, + DetectionConfidence::High, + package, + &format!("package '{package}' found in dependencies"), + )); } } + None +} - // Check config_files (existence) - if let Some(config_files) = &rules.config_files { - for path in config_files { - // Check cache first (hot path for shallow markers), fallback to fs for deeply nested ones - if metadata.paths.contains(path) || project_root.join(path).exists() { - let display = path.display().to_string(); +fn check_package_patterns( + tech_id: &TechnologyId, + rules: &CompiledDetectionRules, + all_packages: &BTreeSet, +) -> Option { + let patterns = rules.package_patterns.as_ref()?; + for regex in patterns { + for package in all_packages { + if regex.is_match(package) { return Some(make_detection( tech_id, - DetectionConfidence::High, - &display, - &format!("config file '{}' exists", display), + DetectionConfidence::Medium, + package, + &format!("package '{package}' matches pattern '{regex}'"), )); } } } + None +} - // Check config_file_content (read files, search patterns) - if let Some(content_rules) = &rules.config_file_content { - let files_to_scan = gather_content_scan_files(project_root, content_rules, metadata); - for file_path in &files_to_scan { - let absolute = project_root.join(file_path); - if let Some(content) = get_file_content(&absolute, cache) { - for pattern in &content_rules.patterns { - if pattern.is_match(&content) { - let display = file_path.display().to_string(); - return Some(make_detection( - tech_id, - DetectionConfidence::Medium, - &display, - &format!("pattern '{}' found in '{}'", pattern, display), - )); - } - } - } +fn check_config_files( + tech_id: &TechnologyId, + rules: &CompiledDetectionRules, + project_root: &Path, + metadata: &RepoMetadata, +) -> Option { + let config_files = rules.config_files.as_ref()?; + for path in config_files { + if metadata.paths.contains(path) || project_root.join(path).exists() { + let display = path.display().to_string(); + return Some(make_detection( + tech_id, + DetectionConfidence::High, + &display, + &format!("config file '{}' exists", display), + )); } } + None +} - // Check file_extensions (lookup in metadata) - if let Some(extensions) = &rules.file_extensions { - for ext in extensions { - if let Some(path) = metadata.extensions.get(ext) { - let display = path.display().to_string(); +fn check_config_file_content( + tech_id: &TechnologyId, + rules: &CompiledDetectionRules, + project_root: &Path, + metadata: &RepoMetadata, + cache: &mut ContentCache, +) -> Option { + let content_rules = rules.config_file_content.as_ref()?; + let files_to_scan = gather_content_scan_files(project_root, content_rules, metadata); + for file_path in &files_to_scan { + let absolute = project_root.join(file_path); + let Some(content) = get_file_content(&absolute, cache) else { + continue; + }; + for pattern in &content_rules.patterns { + if pattern.is_match(&content) { + let display = file_path.display().to_string(); return Some(make_detection( tech_id, DetectionConfidence::Medium, &display, - &format!("file with extension '{ext}' found"), + &format!("pattern '{}' found in '{}'", pattern, display), )); } } } + None +} +fn check_file_extensions( + tech_id: &TechnologyId, + rules: &CompiledDetectionRules, + metadata: &RepoMetadata, +) -> Option { + let extensions = rules.file_extensions.as_ref()?; + for ext in extensions { + if let Some(path) = metadata.extensions.get(ext) { + let display = path.display().to_string(); + return Some(make_detection( + tech_id, + DetectionConfidence::Medium, + &display, + &format!("file with extension '{ext}' found"), + )); + } + } None } @@ -546,43 +602,53 @@ fn gather_content_scan_files( let mut files = Vec::new(); if rules.scan_gradle_layout { - // Root-level Gradle files - for name in &[ - "build.gradle.kts", - "build.gradle", - "settings.gradle.kts", - "settings.gradle", - "gradle/libs.versions.toml", - ] { - let path = PathBuf::from(name); - if metadata.paths.contains(&path) { - files.push(path); - } - } + gather_gradle_files(metadata, &mut files); + } - // Optimization: Use pre-calculated root_dirs from metadata to avoid - // re-filtering the entire directory set on every tech rule evaluation. - for dir in &metadata.root_dirs { - for build_file in &["build.gradle.kts", "build.gradle"] { - let path = dir.join(build_file); - if metadata.paths.contains(&path) { - files.push(path); - } - } + if let Some(explicit_files) = &rules.files { + gather_explicit_files(project_root, explicit_files, metadata, &mut files); + } + + files +} + +fn gather_gradle_files(metadata: &RepoMetadata, files: &mut Vec) { + for name in &[ + "build.gradle.kts", + "build.gradle", + "settings.gradle.kts", + "settings.gradle", + "gradle/libs.versions.toml", + ] { + let path = PathBuf::from(name); + if metadata.paths.contains(&path) { + files.push(path); } } - if let Some(explicit_files) = &rules.files { - for path in explicit_files { - if (metadata.paths.contains(path) || project_root.join(path).exists()) - && !files.contains(path) - { - files.push(path.clone()); + for dir in &metadata.root_dirs { + for build_file in &["build.gradle.kts", "build.gradle"] { + let path = dir.join(build_file); + if metadata.paths.contains(&path) { + files.push(path); } } } +} - files +fn gather_explicit_files( + project_root: &Path, + explicit_files: &[PathBuf], + metadata: &RepoMetadata, + files: &mut Vec, +) { + for path in explicit_files { + if (metadata.paths.contains(path) || project_root.join(path).exists()) + && !files.contains(path) + { + files.push(path.clone()); + } + } } // --------------------------------------------------------------------------- @@ -767,43 +833,52 @@ fn parse_pyproject_toml_deps(path: &Path, cache: &mut ContentCache) -> Option) { + let Some(project) = value.get("project").and_then(|v| v.as_table()) else { + return; + }; + if let Some(dependencies) = project.get("dependencies").and_then(|v| v.as_array()) { + collect_python_dependency_array(dependencies, deps); + } + if let Some(optional) = project + .get("optional-dependencies") + .and_then(|v| v.as_table()) + { + for dependencies in optional.values().filter_map(|v| v.as_array()) { + collect_python_dependency_array(dependencies, deps); } } +} - if let Some(poetry) = value +fn collect_poetry_deps(value: &toml::Value, deps: &mut BTreeSet) { + let Some(poetry) = value .get("tool") .and_then(|v| v.get("poetry")) .and_then(|v| v.as_table()) - { - if let Some(dependencies) = poetry.get("dependencies").and_then(|v| v.as_table()) { - collect_python_dependency_table(dependencies, &mut deps); - } - if let Some(group) = poetry.get("group").and_then(|v| v.as_table()) { - for dependencies in group.values().filter_map(|group| { - group - .get("dependencies") - .and_then(|dependencies| dependencies.as_table()) - }) { - collect_python_dependency_table(dependencies, &mut deps); - } - } - if let Some(dev_dependencies) = poetry.get("dev-dependencies").and_then(|v| v.as_table()) { - collect_python_dependency_table(dev_dependencies, &mut deps); + else { + return; + }; + if let Some(dependencies) = poetry.get("dependencies").and_then(|v| v.as_table()) { + collect_python_dependency_table(dependencies, deps); + } + if let Some(group) = poetry.get("group").and_then(|v| v.as_table()) { + for dependencies in group.values().filter_map(|group| { + group + .get("dependencies") + .and_then(|dependencies| dependencies.as_table()) + }) { + collect_python_dependency_table(dependencies, deps); } } - - Some(deps) + if let Some(dev_dependencies) = poetry.get("dev-dependencies").and_then(|v| v.as_table()) { + collect_python_dependency_table(dev_dependencies, deps); + } } fn parse_pipfile_deps(path: &Path, cache: &mut ContentCache) -> Option> { @@ -935,7 +1010,6 @@ fn expand_workspace_patterns( let mut dirs = Vec::new(); for pattern in patterns { - // Strip trailing /* or /** for simple glob expansion let base = pattern .trim_end_matches("/**") .trim_end_matches("/*") @@ -944,30 +1018,44 @@ fn expand_workspace_patterns( let base_rel = Path::new(base); if pattern.contains('*') { - // Glob: use cached directories from metadata to find workspace members - // avoiding redundant O(N) filesystem walks. - for dir_rel in &metadata.dirs { - if dir_rel.parent() == Some(base_rel) { - // Optimization: Defer the PathBuf join of "package.json" until after - // verifying the directory is a child of the workspace base. - let manifest = dir_rel.join("package.json"); - if metadata.paths.contains(&manifest) || project_root.join(&manifest).exists() { - dirs.push(project_root.join(dir_rel)); - } - } - } + expand_glob_workspace(project_root, base_rel, metadata, &mut dirs); } else { - // Exact path: check cache first, then fall back to filesystem existence. - let manifest = base_rel.join("package.json"); - if metadata.paths.contains(&manifest) || project_root.join(&manifest).exists() { - dirs.push(project_root.join(base_rel)); - } + expand_exact_workspace(project_root, base_rel, metadata, &mut dirs); } } dirs } +fn expand_glob_workspace( + project_root: &Path, + base_rel: &Path, + metadata: &RepoMetadata, + dirs: &mut Vec, +) { + for dir_rel in &metadata.dirs { + if dir_rel.parent() != Some(base_rel) { + continue; + } + let manifest = dir_rel.join("package.json"); + if metadata.paths.contains(&manifest) || project_root.join(&manifest).exists() { + dirs.push(project_root.join(dir_rel)); + } + } +} + +fn expand_exact_workspace( + project_root: &Path, + base_rel: &Path, + metadata: &RepoMetadata, + dirs: &mut Vec, +) { + let manifest = base_rel.join("package.json"); + if metadata.paths.contains(&manifest) || project_root.join(&manifest).exists() { + dirs.push(project_root.join(base_rel)); + } +} + /// Collects package names including from nested projects. /// /// NOTE: At root-phase (Phase 1), only package.json deps are merged from nested projects diff --git a/src/skills/install.rs b/src/skills/install.rs index 917cf02f..5c173e9c 100644 --- a/src/skills/install.rs +++ b/src/skills/install.rs @@ -238,9 +238,15 @@ fn find_best_skill_dir(temp_path: &Path, skill_id: &str) -> PathBuf { temp_path.to_path_buf() } +/// Result of fetching a skill source — either we already copied a directory, +/// or we have archive bytes to unpack. +enum FetchedSource { + DirectoryCopied, + Archive(Vec), +} + pub async fn fetch_and_unpack_to_tempdir(url: &str) -> Result { use std::io::Cursor; - use std::path::Path; // Support subpaths via fragments, e.g. https://example.com/archive.zip#subpath let (url_base, subpath) = match url.find('#') { @@ -249,252 +255,355 @@ pub async fn fetch_and_unpack_to_tempdir(url: &str) -> Result return Ok(tmp), + FetchedSource::Archive(bytes) => bytes, + }; + + let source_name = url_base.to_string(); + let is_tar_gz = source_name.ends_with(".tar.gz") || source_name.ends_with(".tgz"); + + if ext == "zip" { + let reader = Cursor::new(&data); + unpack_zip(reader, tmp.path(), subpath)?; + } else if is_tar_gz { + let reader = Cursor::new(&data); + unpack_tar_gz(reader, tmp.path(), subpath)?; + } else { + return Err(SkillInstallError::Other("unknown archive format".into())); + } + Ok(tmp) +} + +async fn fetch_archive_data( + url_base: &str, + tmp_path: &std::path::Path, +) -> Result<(FetchedSource, String), SkillInstallError> { let is_file = url_base.starts_with("file://"); - // is_local: either absolute unix path or Windows drive letter (C:) let is_local = url_base.starts_with('/') || url_base.chars().nth(1) == Some(':'); - let client = if !is_file && !is_local { - Some(Client::new()) + + if is_file || is_local { + fetch_local_data(url_base, is_file, tmp_path) } else { - None - }; - let (data, ext) = if is_file || is_local { - // Safely strip file:// prefix - let path_str = if is_file { - url_base.strip_prefix("file://").unwrap_or("") - } else { - url_base - }; - if path_str.is_empty() { - return Err(SkillInstallError::Validation("empty file:// path".into())); - } - let path = Path::new(path_str); - let ext = path - .extension() - .and_then(|v| v.to_str()) - .unwrap_or("") - .to_ascii_lowercase(); - if path.is_dir() { - // Local directory: copy recursively to tempdir and return - copy_dir_recursively(path, tmp.path())?; - return Ok(tmp); - } - let data = std::fs::read(path).map_err(SkillInstallError::Io)?; - (data, ext) + fetch_remote_data(url_base, tmp_path).await + } +} + +fn fetch_local_data( + url_base: &str, + is_file: bool, + tmp_path: &std::path::Path, +) -> Result<(FetchedSource, String), SkillInstallError> { + let path_str = if is_file { + url_base.strip_prefix("file://").unwrap_or("") } else { - let ext = { - let parts: Vec<_> = url_base.split('.').collect(); - if let Some(last) = parts.last() { - last.to_ascii_lowercase() - } else { - "".to_string() - } - }; - let client = client.ok_or_else(|| SkillInstallError::Other("no client".into()))?; - let resp = client - .get(url_base) - .send() + url_base + }; + if path_str.is_empty() { + return Err(SkillInstallError::Validation("empty file:// path".into())); + } + let path = Path::new(path_str); + let ext = path + .extension() + .and_then(|v| v.to_str()) + .unwrap_or("") + .to_ascii_lowercase(); + if path.is_dir() { + copy_dir_recursively(path, tmp_path)?; + return Ok((FetchedSource::DirectoryCopied, String::new())); + } + let data = std::fs::read(path).map_err(SkillInstallError::Io)?; + Ok((FetchedSource::Archive(data), ext)) +} + +async fn fetch_remote_data( + url_base: &str, + tmp_path: &std::path::Path, +) -> Result<(FetchedSource, String), SkillInstallError> { + const MAX_DOWNLOAD_SIZE: u64 = 100 * 1024 * 1024; // 100 MB + + let ext = url_base + .rsplit('.') + .next() + .unwrap_or("") + .to_ascii_lowercase(); + + let client = Client::new(); + let resp = client + .get(url_base) + .send() + .await + .map_err(SkillInstallError::Network)? + .error_for_status() + .map_err(SkillInstallError::Network)?; + + // Check Content-Length header if available + if let Some(content_length) = resp.content_length() + && content_length > MAX_DOWNLOAD_SIZE + { + return Err(SkillInstallError::Other(format!( + "download too large: {content_length} bytes exceeds {MAX_DOWNLOAD_SIZE} byte limit" + ))); + } + + let stdfile = + std::fs::File::create(tmp_path.join("download.tmp")).map_err(SkillInstallError::Io)?; + let mut tmpfile = tokio::fs::File::from_std(stdfile); + let mut stream = resp.bytes_stream(); + let mut total_bytes: u64 = 0; + use futures_util::StreamExt as _; + use tokio::io::AsyncWriteExt; + while let Some(chunk) = stream.next().await { + let chunk = chunk.map_err(SkillInstallError::Network)?; + total_bytes += chunk.len() as u64; + if total_bytes > MAX_DOWNLOAD_SIZE { + let _ = std::fs::remove_file(tmp_path.join("download.tmp")); + return Err(SkillInstallError::Other(format!( + "download too large: exceeded {MAX_DOWNLOAD_SIZE} byte limit while streaming" + ))); + } + tmpfile + .write_all(&chunk) .await - .map_err(SkillInstallError::Network)? - .error_for_status() - .map_err(SkillInstallError::Network)?; - // Stream response to a temp file instead of buffering in memory - let stdfile = std::fs::File::create(tmp.path().join("download.tmp")) .map_err(SkillInstallError::Io)?; - let mut tmpfile = tokio::fs::File::from_std(stdfile); - let mut stream = resp.bytes_stream(); - use futures_util::StreamExt as _; - use tokio::io::AsyncWriteExt; - while let Some(chunk) = stream.next().await { - let chunk = chunk.map_err(SkillInstallError::Network)?; - tmpfile - .write_all(&chunk) - .await - .map_err(SkillInstallError::Io)?; - } - tmpfile.flush().await.map_err(SkillInstallError::Io)?; - // Re-open and read into memory for legacy unpacking logic where needed - let data = std::fs::read(tmp.path().join("download.tmp")).map_err(SkillInstallError::Io)?; - // Cleanup download temp file to avoid including it in the skill - let _ = std::fs::remove_file(tmp.path().join("download.tmp")); - (data, ext) - }; - // Unpack archive type into the tempdir - // Determine source name for archive-type heuristics (handles local file paths too) - let source_name = url_base.to_string(); - let is_tar_gz = source_name.ends_with(".tar.gz") || source_name.ends_with(".tgz"); + } + tmpfile.flush().await.map_err(SkillInstallError::Io)?; - if ext == "zip" { - let reader = Cursor::new(&data); - let mut zip = ZipArchive::new(reader).map_err(SkillInstallError::ZipArchive)?; - - // Find if there is a common root directory (like GitHub zips do) - let common_root = if !zip.is_empty() { - let first_name = zip - .by_index(0) - .map_err(SkillInstallError::ZipArchive)? - .name() - .to_string(); - let root = first_name.split('/').next().unwrap_or(""); - if !root.is_empty() && zip.file_names().all(|n| n.starts_with(root)) { - Some(root.to_string()) - } else { - None - } - } else { - None - }; + let data = std::fs::read(tmp_path.join("download.tmp")).map_err(SkillInstallError::Io)?; + let _ = std::fs::remove_file(tmp_path.join("download.tmp")); + Ok((FetchedSource::Archive(data), ext)) +} - for i in 0..zip.len() { - let mut file = zip.by_index(i).map_err(SkillInstallError::ZipArchive)?; - let full_name = file.name(); +fn unpack_zip( + reader: impl std::io::Read + std::io::Seek, + dest: &std::path::Path, + subpath: Option<&str>, +) -> Result<(), SkillInstallError> { + let mut zip = ZipArchive::new(reader).map_err(SkillInstallError::ZipArchive)?; - // SECURITY: Reject absolute paths, drive prefixes, and path traversal attempts. - if archive_path_is_unsafe(full_name) { - return Err(SkillInstallError::PathTraversal(full_name.to_string())); - } + let common_root = zip_common_root(&mut zip)?; - // Strip common root if present - let rel_path = if let Some(ref root) = common_root { - if full_name.starts_with(root) { - full_name[root.len()..].trim_start_matches('/') - } else { - full_name - } - } else { - full_name - }; - - // If a subpath is requested, filter and strip it - let final_rel_path = if let Some(sub) = subpath { - if let Some(stripped) = rel_path.strip_prefix(sub) { - stripped.trim_start_matches('/') - } else { - continue; // Skip files not in subpath - } - } else { - rel_path - }; + for i in 0..zip.len() { + let mut file = zip.by_index(i).map_err(SkillInstallError::ZipArchive)?; + let full_name = file.name().to_string(); - if final_rel_path.is_empty() { - continue; - } + if archive_path_is_unsafe(&full_name) { + return Err(SkillInstallError::PathTraversal(full_name)); + } - let outpath = tmp.path().join(final_rel_path); - if file.is_dir() { - std::fs::create_dir_all(&outpath).map_err(SkillInstallError::Io)?; - } else { - if let Some(parent) = outpath.parent() { - std::fs::create_dir_all(parent).map_err(SkillInstallError::Io)?; - } - let mut out = std::fs::File::create(&outpath).map_err(SkillInstallError::Io)?; - std::io::copy(&mut file, &mut out).map_err(SkillInstallError::Io)?; - } + let Some(final_rel_path) = zip_entry_rel_path(&full_name, common_root.as_deref(), subpath) + else { + continue; + }; + + if final_rel_path.is_empty() { + continue; } - } else if is_tar_gz { - let reader = Cursor::new(&data); - let gz = GzDecoder::new(reader); - let mut archive = Archive::new(gz); - - let entries: Vec<_> = archive.entries().map_err(SkillInstallError::Io)?.collect(); - - // Find if there is a common root directory - let common_root = if !entries.is_empty() { - let first_path = entries[0] - .as_ref() - .map_err(|e| SkillInstallError::Other(e.to_string()))? - .path() - .map_err(SkillInstallError::Io)?; - let root = first_path.components().next().and_then(|c| match c { - std::path::Component::Normal(s) => Some(s.to_string_lossy().into_owned()), - _ => None, - }); - - if let Some(ref r) = root { - let all_start_with = entries.iter().all(|e| { - if let Ok(entry) = e { - if let Ok(path) = entry.path() { - path.starts_with(r) - } else { - false - } - } else { - false - } - }); - if all_start_with { - Some(r.clone()) - } else { - None - } - } else { - None + + let outpath = dest.join(final_rel_path); + if file.is_dir() { + std::fs::create_dir_all(&outpath).map_err(SkillInstallError::Io)?; + } else { + if let Some(parent) = outpath.parent() { + std::fs::create_dir_all(parent).map_err(SkillInstallError::Io)?; } + let mut out = std::fs::File::create(&outpath).map_err(SkillInstallError::Io)?; + std::io::copy(&mut file, &mut out).map_err(SkillInstallError::Io)?; + } + } + Ok(()) +} + +fn zip_common_root( + zip: &mut ZipArchive, +) -> Result, SkillInstallError> { + if zip.is_empty() { + return Ok(None); + } + let first_name = zip + .by_index(0) + .map_err(SkillInstallError::ZipArchive)? + .name() + .to_string(); + let root = first_name.split('/').next().unwrap_or(""); + if !root.is_empty() + && zip + .file_names() + .all(|n| n == root || n.starts_with(&format!("{root}/"))) + && zip.file_names().any(|n| n.starts_with(&format!("{root}/"))) + { + Ok(Some(root.to_string())) + } else { + Ok(None) + } +} + +fn zip_entry_rel_path<'a>( + full_name: &'a str, + common_root: Option<&str>, + subpath: Option<&str>, +) -> Option<&'a str> { + let rel_path = if let Some(root) = common_root { + // Strip root only at a component boundary: "root/" prefix or exact match + if let Some(rest) = full_name.strip_prefix(&format!("{root}/")) { + rest + } else if full_name == root { + "" + } else { + full_name + } + } else { + full_name + }; + + if let Some(sub) = subpath { + // Normalize trailing slash: "src/" → "src" + let sub = sub.trim_end_matches('/'); + // Strip subpath only at a component boundary + if let Some(rest) = rel_path.strip_prefix(&format!("{sub}/")) { + Some(rest) + } else if rel_path == sub { + Some("") } else { None + } + } else { + Some(rel_path) + } +} + +fn unpack_tar_gz( + reader: impl std::io::Read, + dest: &std::path::Path, + subpath: Option<&str>, +) -> Result<(), SkillInstallError> { + // Limit decompressed size to prevent zip bombs + const MAX_DECOMPRESSED_SIZE: u64 = 500 * 1024 * 1024; // 500 MB + + let mut decompressed = Vec::new(); + let gz = GzDecoder::new(reader); + let mut limited = std::io::Read::take(gz, MAX_DECOMPRESSED_SIZE + 1); + std::io::Read::read_to_end(&mut limited, &mut decompressed).map_err(SkillInstallError::Io)?; + + if decompressed.len() as u64 > MAX_DECOMPRESSED_SIZE { + return Err(SkillInstallError::Other(format!( + "decompressed archive too large: exceeds {MAX_DECOMPRESSED_SIZE} byte limit" + ))); + } + + // First pass: determine common root + let common_root = { + let mut archive = Archive::new(std::io::Cursor::new(&decompressed)); + tar_common_root_from_archive(&mut archive)? + }; + + // Second pass: extract entries + let mut archive = Archive::new(std::io::Cursor::new(&decompressed)); + for entry in archive.entries().map_err(SkillInstallError::Io)? { + let mut entry = entry.map_err(SkillInstallError::Io)?; + + let entry_type = entry.header().entry_type(); + if !entry_type.is_file() && !entry_type.is_dir() { + continue; + } + + let full_path = entry.path().map_err(SkillInstallError::Io)?; + let full_path_string = full_path.to_string_lossy(); + + if archive_path_is_unsafe(&full_path_string) { + return Err(SkillInstallError::PathTraversal( + full_path_string.into_owned(), + )); + } + + let Some(final_rel_path) = tar_entry_rel_path(&full_path, common_root.as_deref(), subpath) + else { + continue; }; - for entry in entries { - let mut entry = entry.map_err(SkillInstallError::Io)?; + if final_rel_path.as_os_str().is_empty() { + continue; + } - // SECURITY: Skip symlinks, hardlinks, and other special files to prevent - // unexpected side effects or path traversal during unpacking. - let entry_type = entry.header().entry_type(); - if !entry_type.is_file() && !entry_type.is_dir() { - continue; - } + let outpath = dest.join(&final_rel_path); + if let Some(parent) = outpath.parent() { + std::fs::create_dir_all(parent).map_err(SkillInstallError::Io)?; + } + entry.unpack(&outpath).map_err(SkillInstallError::Io)?; + } + Ok(()) +} - let full_path = entry.path().map_err(SkillInstallError::Io)?; - let full_path_string = full_path.to_string_lossy(); +fn tar_common_root_from_archive( + archive: &mut Archive, +) -> Result, SkillInstallError> { + let mut root: Option = None; + let mut has_nested = false; - if archive_path_is_unsafe(&full_path_string) { - return Err(SkillInstallError::PathTraversal( - full_path_string.into_owned(), - )); - } + for entry in archive.entries().map_err(SkillInstallError::Io)? { + let entry = entry.map_err(SkillInstallError::Io)?; + let path = entry.path().map_err(SkillInstallError::Io)?; - // Strip common root if present - let rel_path = if let Some(ref root) = common_root { - if full_path.starts_with(root) { - full_path - .strip_prefix(root) - .unwrap_or(&full_path) - .to_path_buf() - } else { - full_path.to_path_buf() - } - } else { - full_path.to_path_buf() - }; - - // If a subpath is requested, filter and strip it - let final_rel_path = if let Some(sub) = subpath { - let sub_path = std::path::Path::new(sub); - if rel_path.starts_with(sub_path) { - rel_path - .strip_prefix(sub_path) - .unwrap_or(&rel_path) - .to_path_buf() - } else { - continue; // Skip files not in subpath - } - } else { - rel_path - }; + if path.components().count() > 1 { + has_nested = true; + } - if final_rel_path.as_os_str().is_empty() { - continue; - } + let first_component = path.components().next().and_then(|c| match c { + std::path::Component::Normal(s) => Some(s.to_string_lossy().into_owned()), + _ => None, + }); - let outpath = tmp.path().join(final_rel_path); - if let Some(parent) = outpath.parent() { - std::fs::create_dir_all(parent).map_err(SkillInstallError::Io)?; - } - entry.unpack(&outpath).map_err(SkillInstallError::Io)?; + let Some(ref component) = first_component else { + return Ok(None); + }; + + match &root { + None => root = Some(component.clone()), + Some(r) if r != component => return Ok(None), + _ => {} + } + } + + if has_nested { Ok(root) } else { Ok(None) } +} + +fn tar_entry_rel_path( + full_path: &std::path::Path, + common_root: Option<&str>, + subpath: Option<&str>, +) -> Option { + let rel_path = if let Some(root) = common_root { + if full_path.starts_with(root) { + full_path + .strip_prefix(root) + .unwrap_or(full_path) + .to_path_buf() + } else { + full_path.to_path_buf() } } else { - return Err(SkillInstallError::Other("unknown archive format".into())); + full_path.to_path_buf() + }; + + if let Some(sub) = subpath { + let sub = sub.trim_end_matches('/'); + let sub_path = std::path::Path::new(sub); + if rel_path.starts_with(sub_path) { + Some( + rel_path + .strip_prefix(sub_path) + .unwrap_or(&rel_path) + .to_path_buf(), + ) + } else { + None + } + } else { + Some(rel_path) } - Ok(tmp) } #[cfg(test)] @@ -574,6 +683,410 @@ mod tests { } } + // --- zip_common_root tests --- + + fn make_zip(entries: &[&str]) -> Vec { + let mut buf = Vec::new(); + { + let mut zip = ZipWriter::new(Cursor::new(&mut buf)); + for entry in entries { + zip.start_file(entry.to_string(), FileOptions::<()>::default()) + .unwrap(); + zip.write_all(b"x").unwrap(); + } + zip.finish().unwrap(); + } + buf + } + + #[test] + fn test_zip_common_root_single_prefix() { + let buf = make_zip(&["foo/a.txt", "foo/b.txt"]); + let mut zip = ZipArchive::new(Cursor::new(buf)).unwrap(); + assert_eq!(zip_common_root(&mut zip).unwrap(), Some("foo".to_string())); + } + + #[test] + fn test_zip_common_root_no_prefix() { + let buf = make_zip(&["a.txt", "b.txt"]); + let mut zip = ZipArchive::new(Cursor::new(buf)).unwrap(); + assert_eq!(zip_common_root(&mut zip).unwrap(), None); + } + + #[test] + fn test_zip_common_root_different_prefixes() { + let buf = make_zip(&["foo/a.txt", "bar/b.txt"]); + let mut zip = ZipArchive::new(Cursor::new(buf)).unwrap(); + assert_eq!(zip_common_root(&mut zip).unwrap(), None); + } + + // --- zip_entry_rel_path tests --- + + #[test] + fn test_zip_entry_rel_path_strips_root() { + assert_eq!( + zip_entry_rel_path("foo/a.txt", Some("foo"), None), + Some("a.txt") + ); + } + + #[test] + fn test_zip_entry_rel_path_strips_subpath() { + assert_eq!( + zip_entry_rel_path("sub/file.txt", None, Some("sub")), + Some("file.txt") + ); + } + + #[test] + fn test_zip_entry_rel_path_strips_root_and_subpath() { + assert_eq!( + zip_entry_rel_path("foo/sub/file.txt", Some("foo"), Some("sub")), + Some("file.txt") + ); + } + + #[test] + fn test_zip_entry_rel_path_no_match_subpath() { + assert_eq!( + zip_entry_rel_path("other/file.txt", None, Some("sub")), + None + ); + } + + #[test] + fn test_zip_entry_rel_path_component_boundary_safety() { + // root="docs" should NOT strip from "docs2/file.txt" + assert_eq!( + zip_entry_rel_path("docs2/file.txt", Some("docs"), None), + Some("docs2/file.txt") + ); + } + + // --- tar_common_root tests --- + + fn make_tar_gz(entries: &[&str]) -> Vec { + let mut buf = Vec::new(); + { + let encoder = flate2::write::GzEncoder::new(&mut buf, flate2::Compression::default()); + let mut builder = tar::Builder::new(encoder); + for entry in entries { + let content = b"x"; + let mut header = tar::Header::new_gnu(); + header.set_size(content.len() as u64); + header.set_mode(0o644); + header.set_cksum(); + builder + .append_data(&mut header, *entry, &content[..]) + .unwrap(); + } + builder.finish().unwrap(); + } + buf + } + + #[test] + fn test_tar_common_root_single_prefix() { + let buf = make_tar_gz(&["foo/a.txt", "foo/b.txt"]); + let mut decompressed = Vec::new(); + let mut gz = flate2::read::GzDecoder::new(Cursor::new(buf)); + std::io::Read::read_to_end(&mut gz, &mut decompressed).unwrap(); + let mut archive = tar::Archive::new(Cursor::new(&decompressed)); + assert_eq!( + tar_common_root_from_archive(&mut archive).unwrap(), + Some("foo".to_string()) + ); + } + + #[test] + fn test_tar_common_root_no_prefix() { + let buf = make_tar_gz(&["a.txt", "b.txt"]); + let mut decompressed = Vec::new(); + let mut gz = flate2::read::GzDecoder::new(Cursor::new(buf)); + std::io::Read::read_to_end(&mut gz, &mut decompressed).unwrap(); + let mut archive = tar::Archive::new(Cursor::new(&decompressed)); + assert_eq!(tar_common_root_from_archive(&mut archive).unwrap(), None); + } + + #[test] + fn test_tar_common_root_different_prefixes() { + let buf = make_tar_gz(&["foo/a.txt", "bar/b.txt"]); + let mut decompressed = Vec::new(); + let mut gz = flate2::read::GzDecoder::new(Cursor::new(buf)); + std::io::Read::read_to_end(&mut gz, &mut decompressed).unwrap(); + let mut archive = tar::Archive::new(Cursor::new(&decompressed)); + assert_eq!(tar_common_root_from_archive(&mut archive).unwrap(), None); + } + + // --- tar_entry_rel_path tests --- + + #[test] + fn test_tar_entry_rel_path_strips_root() { + let p = Path::new("foo/a.txt"); + assert_eq!( + tar_entry_rel_path(p, Some("foo"), None), + Some(PathBuf::from("a.txt")) + ); + } + + #[test] + fn test_tar_entry_rel_path_strips_subpath() { + let p = Path::new("sub/file.txt"); + assert_eq!( + tar_entry_rel_path(p, None, Some("sub")), + Some(PathBuf::from("file.txt")) + ); + } + + #[test] + fn test_tar_entry_rel_path_strips_root_and_subpath() { + let p = Path::new("foo/sub/file.txt"); + assert_eq!( + tar_entry_rel_path(p, Some("foo"), Some("sub")), + Some(PathBuf::from("file.txt")) + ); + } + + #[test] + fn test_tar_entry_rel_path_no_match_subpath() { + let p = Path::new("other/file.txt"); + assert_eq!(tar_entry_rel_path(p, None, Some("sub")), None); + } + + // --- fetch_local_data tests --- + + #[test] + fn test_fetch_local_data_file() { + let tmp = tempfile::tempdir().unwrap(); + let file_path = tmp.path().join("test.zip"); + std::fs::write(&file_path, b"fake zip data").unwrap(); + + let (source, ext) = + fetch_local_data(file_path.to_str().unwrap(), false, tmp.path()).unwrap(); + assert!(matches!(source, FetchedSource::Archive(data) if data == b"fake zip data")); + assert_eq!(ext, "zip"); + } + + #[test] + fn test_fetch_local_data_directory() { + let src_dir = tempfile::tempdir().unwrap(); + std::fs::write(src_dir.path().join("SKILL.md"), b"name: test").unwrap(); + + let dest_dir = tempfile::tempdir().unwrap(); + let (source, ext) = + fetch_local_data(src_dir.path().to_str().unwrap(), false, dest_dir.path()).unwrap(); + assert!(matches!(source, FetchedSource::DirectoryCopied)); + assert_eq!(ext, ""); + // Verify the file was copied + assert!(dest_dir.path().join("SKILL.md").exists()); + } + + #[test] + fn test_fetch_local_data_file_uri() { + let tmp = tempfile::tempdir().unwrap(); + let file_path = tmp.path().join("archive.tar.gz"); + std::fs::write(&file_path, b"fake tar data").unwrap(); + + let uri = format!("file://{}", file_path.to_str().unwrap()); + let (source, ext) = fetch_local_data(&uri, true, tmp.path()).unwrap(); + assert!(matches!(source, FetchedSource::Archive(data) if data == b"fake tar data")); + assert_eq!(ext, "gz"); + } + + // --- unpack_zip end-to-end tests --- + + #[test] + fn test_unpack_zip_basic_extraction() { + let mut buf = Vec::new(); + { + let mut zip = ZipWriter::new(Cursor::new(&mut buf)); + zip.start_file("SKILL.md", FileOptions::<()>::default()) + .unwrap(); + zip.write_all(b"name: my-skill").unwrap(); + zip.start_file("src/main.rs", FileOptions::<()>::default()) + .unwrap(); + zip.write_all(b"fn main() {}").unwrap(); + zip.finish().unwrap(); + } + + let dest = tempfile::tempdir().unwrap(); + unpack_zip(Cursor::new(&buf), dest.path(), None).unwrap(); + + assert!(dest.path().join("SKILL.md").exists()); + assert!(dest.path().join("src/main.rs").exists()); + assert_eq!( + std::fs::read_to_string(dest.path().join("SKILL.md")).unwrap(), + "name: my-skill" + ); + } + + #[test] + fn test_unpack_zip_with_common_root_stripping() { + // Archive where all files share a common root "my-skill-v1/" + let mut buf = Vec::new(); + { + let mut zip = ZipWriter::new(Cursor::new(&mut buf)); + zip.add_directory("my-skill-v1/", FileOptions::<()>::default()) + .unwrap(); + zip.start_file("my-skill-v1/SKILL.md", FileOptions::<()>::default()) + .unwrap(); + zip.write_all(b"name: skill").unwrap(); + zip.start_file("my-skill-v1/lib.rs", FileOptions::<()>::default()) + .unwrap(); + zip.write_all(b"pub mod lib;").unwrap(); + zip.finish().unwrap(); + } + + let dest = tempfile::tempdir().unwrap(); + unpack_zip(Cursor::new(&buf), dest.path(), None).unwrap(); + + // Common root "my-skill-v1" should be stripped + assert!(dest.path().join("SKILL.md").exists()); + assert!(dest.path().join("lib.rs").exists()); + } + + #[test] + fn test_unpack_zip_with_subpath_filter() { + let mut buf = Vec::new(); + { + let mut zip = ZipWriter::new(Cursor::new(&mut buf)); + zip.start_file("docs/readme.md", FileOptions::<()>::default()) + .unwrap(); + zip.write_all(b"docs").unwrap(); + zip.start_file("src/SKILL.md", FileOptions::<()>::default()) + .unwrap(); + zip.write_all(b"name: skill").unwrap(); + zip.start_file("src/code.rs", FileOptions::<()>::default()) + .unwrap(); + zip.write_all(b"code").unwrap(); + zip.finish().unwrap(); + } + + let dest = tempfile::tempdir().unwrap(); + unpack_zip(Cursor::new(&buf), dest.path(), Some("src")).unwrap(); + + // Only files under "src/" should be extracted, with "src/" stripped + assert!(dest.path().join("SKILL.md").exists()); + assert!(dest.path().join("code.rs").exists()); + assert!(!dest.path().join("docs").exists()); + assert!(!dest.path().join("readme.md").exists()); + } + + // --- unpack_tar_gz end-to-end tests --- + + #[test] + fn test_unpack_tar_gz_basic_extraction() { + let buf = make_tar_gz(&["SKILL.md", "src/main.rs"]); + + let dest = tempfile::tempdir().unwrap(); + unpack_tar_gz(Cursor::new(&buf), dest.path(), None).unwrap(); + + assert!(dest.path().join("SKILL.md").exists()); + assert!(dest.path().join("src/main.rs").exists()); + } + + #[test] + fn test_unpack_tar_gz_with_common_root_stripping() { + let buf = make_tar_gz(&["root/SKILL.md", "root/lib.rs"]); + + let dest = tempfile::tempdir().unwrap(); + unpack_tar_gz(Cursor::new(&buf), dest.path(), None).unwrap(); + + // Common root "root" should be stripped + assert!(dest.path().join("SKILL.md").exists()); + assert!(dest.path().join("lib.rs").exists()); + } + + #[test] + fn test_unpack_tar_gz_with_subpath_filter() { + let buf = make_tar_gz(&[ + "root/docs/readme.md", + "root/src/SKILL.md", + "root/src/code.rs", + ]); + + let dest = tempfile::tempdir().unwrap(); + unpack_tar_gz(Cursor::new(&buf), dest.path(), Some("src")).unwrap(); + + // Only files under "src/" after root stripping + assert!(dest.path().join("SKILL.md").exists()); + assert!(dest.path().join("code.rs").exists()); + assert!(!dest.path().join("docs").exists()); + } + + // --- find_best_skill_dir tests --- + + #[test] + fn test_find_best_skill_dir_root_manifest() { + let tmp = tempfile::tempdir().unwrap(); + std::fs::write(tmp.path().join("SKILL.md"), b"name: test").unwrap(); + assert_eq!(find_best_skill_dir(tmp.path(), "test"), tmp.path()); + } + + #[test] + fn test_find_best_skill_dir_matching_subdir() { + let tmp = tempfile::tempdir().unwrap(); + let sub = tmp.path().join("my-skill"); + std::fs::create_dir_all(&sub).unwrap(); + std::fs::write(sub.join("SKILL.md"), b"name: my-skill").unwrap(); + assert_eq!(find_best_skill_dir(tmp.path(), "my-skill"), sub); + } + + #[test] + fn test_find_best_skill_dir_sole_manifest() { + let tmp = tempfile::tempdir().unwrap(); + let sub = tmp.path().join("some-other-name"); + std::fs::create_dir_all(&sub).unwrap(); + std::fs::write(sub.join("SKILL.md"), b"name: test").unwrap(); + // Only one manifest found, should return its parent even if name doesn't match + assert_eq!(find_best_skill_dir(tmp.path(), "my-skill"), sub); + } + + #[test] + fn test_find_best_skill_dir_fallback() { + let tmp = tempfile::tempdir().unwrap(); + // No SKILL.md anywhere + std::fs::write(tmp.path().join("README.md"), b"hello").unwrap(); + assert_eq!(find_best_skill_dir(tmp.path(), "test"), tmp.path()); + } + + // --- copy_dir_recursively tests --- + + #[test] + fn test_copy_dir_recursively_basic() { + let src = tempfile::tempdir().unwrap(); + std::fs::write(src.path().join("a.txt"), b"hello").unwrap(); + std::fs::create_dir_all(src.path().join("sub")).unwrap(); + std::fs::write(src.path().join("sub/b.txt"), b"world").unwrap(); + + let dst = tempfile::tempdir().unwrap(); + let target = dst.path().join("output"); + copy_dir_recursively(src.path(), &target).unwrap(); + + assert_eq!( + std::fs::read_to_string(target.join("a.txt")).unwrap(), + "hello" + ); + assert_eq!( + std::fs::read_to_string(target.join("sub/b.txt")).unwrap(), + "world" + ); + } + + // --- archive_path_is_unsafe tests --- + + #[test] + fn test_archive_path_is_unsafe_safe_paths() { + assert!(!archive_path_is_unsafe("foo/bar.txt")); + assert!(!archive_path_is_unsafe("SKILL.md")); + assert!(!archive_path_is_unsafe("src/lib.rs")); + } + + #[test] + fn test_archive_path_is_unsafe_backslash_prefix() { + assert!(archive_path_is_unsafe("\\\\server\\share")); + } + #[tokio::test] async fn test_tar_windows_drive_path_rejection() { let temp_root = tempfile::tempdir().unwrap(); diff --git a/src/skills/suggest.rs b/src/skills/suggest.rs index 6b630fb9..029ba7c0 100644 --- a/src/skills/suggest.rs +++ b/src/skills/suggest.rs @@ -690,36 +690,8 @@ impl SuggestInstallJsonResponse { pub fn render_human(&self) -> String { let mut lines = Vec::new(); - if self.suggest.detections.is_empty() { - lines.push("Detected technologies: none".to_string()); - } else { - lines.push("Detected technologies:".to_string()); - for detection in &self.suggest.detections { - lines.push(format!( - "- {} ({}): {}", - detection.technology, - detection.confidence, - detection.evidence.join(", ") - )); - } - } - - if self.suggest.recommendations.is_empty() { - lines.push("Recommended skills: none".to_string()); - } else { - lines.push("Recommended skills:".to_string()); - for recommendation in &self.suggest.recommendations { - let installed = if recommendation.installed { - "installed" - } else { - "not installed" - }; - lines.push(format!("- {} [{}]", recommendation.skill_id, installed)); - for reason in &recommendation.reasons { - lines.push(format!(" reason: {}", reason)); - } - } - } + render_detections_section(&self.suggest.detections, &mut lines); + render_recommendations_section(&self.suggest.recommendations, &mut lines); lines.push(format!( "Summary: {} detected, {} recommended, {} installable", @@ -729,29 +701,74 @@ impl SuggestInstallJsonResponse { )); lines.push(format!("Install mode: {}", self.mode.as_human_label())); - if self.selected_skill_ids.is_empty() { - lines.push("Selected skills: none".to_string()); - } else { + render_selected_skills_section(&self.selected_skill_ids, &mut lines); + render_install_results_section(&self.results, &mut lines); + + lines.join("\n") + } +} + +fn render_detections_section(detections: &[SuggestJsonDetection], lines: &mut Vec) { + if detections.is_empty() { + lines.push("Detected technologies: none".to_string()); + } else { + lines.push("Detected technologies:".to_string()); + for detection in detections { lines.push(format!( - "Selected skills: {}", - self.selected_skill_ids.join(", ") + "- {} ({}): {}", + detection.technology, + detection.confidence, + detection.evidence.join(", ") )); } + } +} - if self.results.is_empty() { - lines.push("Install results: none".to_string()); - } else { - lines.push("Install results:".to_string()); - for result in &self.results { - let mut line = format!("- {}: {}", result.skill_id, result.status.as_human_label()); - if let Some(error_message) = &result.error_message { - line.push_str(&format!(" ({error_message})")); - } - lines.push(line); +fn render_recommendations_section( + recommendations: &[SuggestJsonRecommendation], + lines: &mut Vec, +) { + if recommendations.is_empty() { + lines.push("Recommended skills: none".to_string()); + } else { + lines.push("Recommended skills:".to_string()); + for recommendation in recommendations { + let installed = if recommendation.installed { + "installed" + } else { + "not installed" + }; + lines.push(format!("- {} [{}]", recommendation.skill_id, installed)); + for reason in &recommendation.reasons { + lines.push(format!(" reason: {}", reason)); } } + } +} - lines.join("\n") +fn render_selected_skills_section(selected_skill_ids: &[String], lines: &mut Vec) { + if selected_skill_ids.is_empty() { + lines.push("Selected skills: none".to_string()); + } else { + lines.push(format!( + "Selected skills: {}", + selected_skill_ids.join(", ") + )); + } +} + +fn render_install_results_section(results: &[SuggestInstallResult], lines: &mut Vec) { + if results.is_empty() { + lines.push("Install results: none".to_string()); + } else { + lines.push("Install results:".to_string()); + for result in results { + let mut line = format!("- {}: {}", result.skill_id, result.status.as_human_label()); + if let Some(error_message) = &result.error_message { + line.push_str(&format!(" ({error_message})")); + } + lines.push(line); + } } } @@ -915,4 +932,154 @@ mod tests { [SuggestInstallProgressEvent::SkippedAlreadyInstalled { skill_id }] if skill_id == "accessibility" )); } + + // --- render helpers coverage --- + + #[test] + fn render_detections_section_empty() { + let mut lines = Vec::new(); + render_detections_section(&[], &mut lines); + assert_eq!(lines, vec!["Detected technologies: none"]); + } + + #[test] + fn render_detections_section_with_entries() { + let detections = vec![ + SuggestJsonDetection { + technology: TechnologyId::new("rust"), + confidence: DetectionConfidence::High, + evidence: vec!["Cargo.toml".to_string()], + }, + SuggestJsonDetection { + technology: TechnologyId::new("python"), + confidence: DetectionConfidence::Medium, + evidence: vec!["setup.py".to_string(), "requirements.txt".to_string()], + }, + ]; + let mut lines = Vec::new(); + render_detections_section(&detections, &mut lines); + assert!(lines[0].contains("Detected technologies:")); + assert!(lines[1].contains("rust")); + assert!(lines[1].contains("high")); + assert!(lines[1].contains("Cargo.toml")); + assert!(lines[2].contains("python")); + assert!(lines[2].contains("setup.py, requirements.txt")); + } + + #[test] + fn render_recommendations_section_empty() { + let mut lines = Vec::new(); + render_recommendations_section(&[], &mut lines); + assert_eq!(lines, vec!["Recommended skills: none"]); + } + + #[test] + fn render_recommendations_section_with_entries() { + let recs = vec![SuggestJsonRecommendation { + skill_id: "rust-async".to_string(), + provider_skill_id: "provider/rust-async".to_string(), + matched_technologies: vec![TechnologyId::new("rust")], + reasons: vec!["uses tokio".to_string()], + installed: false, + }]; + let mut lines = Vec::new(); + render_recommendations_section(&recs, &mut lines); + assert!(lines[0].contains("Recommended skills:")); + assert!(lines[1].contains("rust-async")); + assert!(lines[1].contains("not installed")); + assert!(lines[2].contains("reason: uses tokio")); + } + + #[test] + fn render_recommendations_section_installed() { + let recs = vec![SuggestJsonRecommendation { + skill_id: "docker".to_string(), + provider_skill_id: "provider/docker".to_string(), + matched_technologies: vec![], + reasons: vec![], + installed: true, + }]; + let mut lines = Vec::new(); + render_recommendations_section(&recs, &mut lines); + assert!(lines[1].contains("installed")); + assert!(!lines[1].contains("not installed")); + } + + #[test] + fn render_selected_skills_section_empty() { + let mut lines = Vec::new(); + render_selected_skills_section(&[], &mut lines); + assert_eq!(lines, vec!["Selected skills: none"]); + } + + #[test] + fn render_selected_skills_section_with_entries() { + let ids = vec!["foo".to_string(), "bar".to_string()]; + let mut lines = Vec::new(); + render_selected_skills_section(&ids, &mut lines); + assert_eq!(lines, vec!["Selected skills: foo, bar"]); + } + + #[test] + fn render_install_results_section_empty() { + let mut lines = Vec::new(); + render_install_results_section(&[], &mut lines); + assert_eq!(lines, vec!["Install results: none"]); + } + + #[test] + fn render_install_results_section_with_entries() { + let results = vec![ + SuggestInstallResult { + skill_id: "a".to_string(), + provider_skill_id: "p/a".to_string(), + status: SuggestInstallStatus::Installed, + error_message: None, + }, + SuggestInstallResult { + skill_id: "b".to_string(), + provider_skill_id: "p/b".to_string(), + status: SuggestInstallStatus::Failed, + error_message: Some("network error".to_string()), + }, + ]; + let mut lines = Vec::new(); + render_install_results_section(&results, &mut lines); + assert!(lines[0].contains("Install results:")); + assert!(lines[1].contains("a: installed")); + assert!(lines[2].contains("b: failed")); + assert!(lines[2].contains("network error")); + } + + #[test] + fn suggest_install_json_response_render_human_integrates_all_sections() { + let response = SuggestInstallJsonResponse { + suggest: SuggestJsonResponse { + detections: vec![SuggestJsonDetection { + technology: TechnologyId::new("rust"), + confidence: DetectionConfidence::High, + evidence: vec!["Cargo.toml".to_string()], + }], + recommendations: vec![], + summary: SuggestSummary { + detected_count: 1, + recommended_count: 0, + installable_count: 0, + }, + }, + mode: SuggestInstallMode::InstallAll, + selected_skill_ids: vec!["skill-a".to_string()], + results: vec![SuggestInstallResult { + skill_id: "skill-a".to_string(), + provider_skill_id: "p/skill-a".to_string(), + status: SuggestInstallStatus::Installed, + error_message: None, + }], + }; + let output = response.render_human(); + assert!(output.contains("rust")); + assert!(output.contains("install-all")); + assert!(output.contains("Selected skills: skill-a")); + assert!(output.contains("skill-a: installed")); + } } diff --git a/src/skills/update.rs b/src/skills/update.rs index e4bf2155..eb493afe 100644 --- a/src/skills/update.rs +++ b/src/skills/update.rs @@ -25,66 +25,86 @@ pub async fn update_skill_async( target_root: &Path, update_source: &Path, ) -> Result<(), SkillUpdateError> { - use crate::skills::install::fetch_and_unpack_to_tempdir; - let use_remote = { - let s = update_source.to_string_lossy().to_string(); - s.starts_with("http://") - || s.starts_with("https://") - || s.ends_with(".zip") - || s.ends_with(".tar.gz") - }; - let local_dir: std::path::PathBuf; - let mut _temp_holder; - if use_remote { - // Download and unpack to temp (propagate SkillInstallError -> SkillUpdateError::Install) - let td = fetch_and_unpack_to_tempdir(&update_source.to_string_lossy()).await?; - local_dir = td.path().to_path_buf(); - _temp_holder = Some(td); - } else { - local_dir = update_source.to_path_buf(); - _temp_holder = None; - } - - use std::fs; - // use std::path::PathBuf; (unused) + // _temp_guard must remain alive to prevent premature cleanup of temp directory + let (local_dir, _temp_guard) = resolve_update_source(update_source).await?; - // Paths let skill_dir = target_root.join(skill_id); let backup_dir = target_root.join(format!("{}.bak", skill_id)); let registry_path = target_root.join("registry.json"); - // Version resolution: only update if new version > current - // 1. Extract current version (from registry if present, else SKILL.md in skill_dir), or treat as "0.0.0" if not installed - debug!(registry_path = %registry_path.display(), exists = %registry_path.exists(), "update registry check"); - if registry_path.exists() { - let reg_contents = - std::fs::read_to_string(®istry_path).unwrap_or_else(|_| "".to_string()); - debug!(contents = %reg_contents, "registry contents after install"); + let current_version = resolve_current_version(skill_id, &skill_dir, ®istry_path); + validate_version_upgrade(&local_dir, ¤t_version)?; + + create_backup(&skill_dir, &backup_dir)?; + + install_updated_skill( + skill_id, + &local_dir, + &skill_dir, + &backup_dir, + ®istry_path, + ) +} + +async fn resolve_update_source( + update_source: &Path, +) -> Result<(std::path::PathBuf, Option), SkillUpdateError> { + use crate::skills::install::fetch_and_unpack_to_tempdir; + let s = update_source.to_string_lossy().to_string(); + let use_remote = s.starts_with("http://") + || s.starts_with("https://") + || s.ends_with(".zip") + || s.ends_with(".tar.gz"); + + if use_remote { + let td = fetch_and_unpack_to_tempdir(&s).await?; + let path = td.path().to_path_buf(); + Ok((path, Some(td))) + } else { + Ok((update_source.to_path_buf(), None)) } - let mut current_version: Option = None; +} + +fn resolve_current_version( + skill_id: &str, + skill_dir: &Path, + registry_path: &Path, +) -> Option { + debug!(registry_path = %registry_path.display(), exists = %registry_path.exists(), "update registry check"); + // Try registry first if registry_path.exists() - && let Ok(reg) = crate::skills::registry::read_registry(®istry_path) - && let Some(skills) = reg.skills - && let Some(entry) = skills.get(skill_id) + && let Some(version) = crate::skills::registry::read_registry(registry_path) + .ok() + .and_then(|reg| reg.skills) + .and_then(|skills| skills.get(skill_id).cloned()) + .and_then(|entry| entry.version) { - current_version = entry.version.clone(); + return Some(version); } - // Fallback: If not in registry, try SKILL.md in existing skill_dir - if current_version.is_none() && skill_dir.exists() { + + // Fallback: try SKILL.md in existing skill_dir + if skill_dir.exists() { let manifest_path = skill_dir.join("SKILL.md"); - if manifest_path.exists() - && let Ok(existing_manifest) = - crate::skills::manifest::parse_skill_manifest(&manifest_path) + if let Some(version) = manifest_path + .exists() + .then(|| crate::skills::manifest::parse_skill_manifest(&manifest_path).ok()) + .flatten() + .and_then(|m| m.version.clone()) { - // existing_manifest.version is Option; propagate directly - current_version = existing_manifest.version.clone(); + return Some(version); } } - // Parse update candidate version from local_dir/SKILL.md + + None +} + +fn validate_version_upgrade( + local_dir: &Path, + current_version: &Option, +) -> Result<(), SkillUpdateError> { let update_manifest_path = local_dir.join("SKILL.md"); let update_manifest = crate::skills::manifest::parse_skill_manifest(&update_manifest_path)?; - // update_manifest.version is Option; require it for update resolution let update_version_str = update_manifest .version .as_deref() @@ -92,7 +112,7 @@ pub async fn update_skill_async( let new_version = semver::Version::parse(update_version_str) .map_err(|_| SkillUpdateError::Validation("invalid semver in SKILL.md".into()))?; let installed_version = match current_version { - Some(ref verstr) => { + Some(verstr) => { semver::Version::parse(verstr).unwrap_or_else(|_| semver::Version::new(0, 0, 0)) } None => semver::Version::new(0, 0, 0), @@ -105,87 +125,101 @@ pub async fn update_skill_async( new_version, installed_version ))); } + Ok(()) +} - // Step 1: Atomically move skill_dir to backup_dir (if exists). +fn create_backup(skill_dir: &Path, backup_dir: &Path) -> Result<(), SkillUpdateError> { + use std::fs; if skill_dir.exists() { - // Clean up previous backup if somehow it exists. if backup_dir.exists() { - fs::remove_dir_all(&backup_dir).map_err(|_| SkillUpdateError::Atomic)?; + fs::remove_dir_all(backup_dir).map_err(|_| SkillUpdateError::Atomic)?; } - fs::rename(&skill_dir, &backup_dir).map_err(|_| SkillUpdateError::Atomic)?; + fs::rename(skill_dir, backup_dir).map_err(|_| SkillUpdateError::Atomic)?; + } + Ok(()) +} + +/// Rolls back skill_dir by removing it and restoring from backup_dir if present. +fn rollback_skill_dir(skill_dir: &Path, backup_dir: &Path) { + if skill_dir.exists() { + let _ = std::fs::remove_dir_all(skill_dir); + } + if backup_dir.exists() { + let _ = std::fs::rename(backup_dir, skill_dir); } +} + +fn install_updated_skill( + skill_id: &str, + local_dir: &Path, + skill_dir: &Path, + backup_dir: &Path, + registry_path: &Path, +) -> Result<(), SkillUpdateError> { + use std::fs; - // Step 2: Copy source dir to skill_dir. - // (We use copy to support cross-device; atomic rename if same device. Use copy_dir_recursively) if skill_dir.exists() { - fs::remove_dir_all(&skill_dir).map_err(|_| SkillUpdateError::Atomic)?; + fs::remove_dir_all(skill_dir).map_err(|_| SkillUpdateError::Atomic)?; + } + if let Err(e) = copy_dir_all(local_dir, skill_dir) { + rollback_skill_dir(skill_dir, backup_dir); + return Err(SkillUpdateError::Io(e)); } - copy_dir_all(&local_dir, &skill_dir).map_err(SkillUpdateError::Io)?; - // Step 3: Validate the new skill manifest. On failure, remove the new dir, restore backup. + // Validate the new skill manifest let manifest_path = skill_dir.join("SKILL.md"); let manifest = match crate::skills::manifest::parse_skill_manifest(&manifest_path) { Ok(manifest) => manifest, Err(e) => { - // Cleanup: remove failed new dir - let _ = fs::remove_dir_all(&skill_dir); - // Restore backup (if any) back to place - if backup_dir.exists() { - let _ = fs::rename(&backup_dir, &skill_dir); - } + rollback_skill_dir(skill_dir, backup_dir); return Err(SkillUpdateError::Install(e)); } }; - // Step 4: Registry update with rollback - // Save previous registry entry for this skill if exists - let mut old_registry_entry: Option = None; - let registry_path = target_root.join("registry.json"); - if registry_path.exists() - && let Ok(reg) = crate::skills::registry::read_registry(®istry_path) - && let Some(skills) = reg.skills - && let Some(entry) = skills.get(skill_id) - { - old_registry_entry = Some(entry.clone()); - } - // Build a new skill entry for registry update + // Save previous registry entry for rollback + let old_registry_entry: Option = + read_old_registry_entry(skill_id, registry_path); + let new_entry = crate::skills::registry::SkillEntry { name: Some(manifest.name.clone()), description: manifest.description.clone(), - // registry expects Option for version; propagate directly version: manifest.version.clone(), provider: None, source: None, installed_at: Some(chrono::Utc::now().to_rfc3339()), - files: None, // Could add list of files here if needed + files: None, manifest_hash: None, }; - // Try registry update, rollback both skill dir and registry on failure + if let Err(e) = - crate::skills::registry::update_registry_entry(®istry_path, skill_id, new_entry) + crate::skills::registry::update_registry_entry(registry_path, skill_id, new_entry) { - // Remove broken new dir - let _ = fs::remove_dir_all(&skill_dir); - // Restore backup if possible - if backup_dir.exists() { - let _ = fs::rename(&backup_dir, &skill_dir); - } - // Try to restore previous registry entry if there was one + rollback_skill_dir(skill_dir, backup_dir); if let Some(old_entry) = old_registry_entry { let _ = - crate::skills::registry::update_registry_entry(®istry_path, skill_id, old_entry); + crate::skills::registry::update_registry_entry(registry_path, skill_id, old_entry); } return Err(SkillUpdateError::Registry(e)); } - // Step 5: All OK, clean up backup if backup_dir.exists() { - let _ = fs::remove_dir_all(&backup_dir); + let _ = fs::remove_dir_all(backup_dir); } - // temp_holder will cleanup tempdir (if present) when dropped Ok(()) } +fn read_old_registry_entry( + skill_id: &str, + registry_path: &Path, +) -> Option { + if !registry_path.exists() { + return None; + } + let reg = crate::skills::registry::read_registry(registry_path).ok()?; + let skills = reg.skills?; + skills.get(skill_id).cloned() +} + /// Recursively copies a directory (src) to dst. fn copy_dir_all(src: &Path, dst: &Path) -> std::io::Result<()> { use std::fs; @@ -213,3 +247,138 @@ fn copy_dir_all(src: &Path, dst: &Path) -> std::io::Result<()> { } Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::TempDir; + + #[test] + fn test_rollback_skill_dir_restores_backup() { + let tmp = TempDir::new().unwrap(); + let skill_dir = tmp.path().join("my-skill"); + let backup_dir = tmp.path().join("my-skill.bak"); + + // Create backup with content + std::fs::create_dir_all(&backup_dir).unwrap(); + std::fs::write(backup_dir.join("SKILL.md"), "# Original").unwrap(); + + // Create a "broken" skill_dir + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::write(skill_dir.join("broken.txt"), "bad").unwrap(); + + rollback_skill_dir(&skill_dir, &backup_dir); + + // skill_dir should now contain original content + assert!(skill_dir.join("SKILL.md").exists()); + assert!(!skill_dir.join("broken.txt").exists()); + assert!(!backup_dir.exists()); + } + + #[test] + fn test_rollback_skill_dir_no_backup_just_removes() { + let tmp = TempDir::new().unwrap(); + let skill_dir = tmp.path().join("my-skill"); + let backup_dir = tmp.path().join("my-skill.bak"); + + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::write(skill_dir.join("broken.txt"), "bad").unwrap(); + + rollback_skill_dir(&skill_dir, &backup_dir); + + assert!(!skill_dir.exists()); + assert!(!backup_dir.exists()); + } + + #[test] + fn test_rollback_skill_dir_nothing_exists() { + let tmp = TempDir::new().unwrap(); + let skill_dir = tmp.path().join("my-skill"); + let backup_dir = tmp.path().join("my-skill.bak"); + + // Should not panic + rollback_skill_dir(&skill_dir, &backup_dir); + assert!(!skill_dir.exists()); + } + + #[test] + fn test_create_backup_moves_skill_dir() { + let tmp = TempDir::new().unwrap(); + let skill_dir = tmp.path().join("my-skill"); + let backup_dir = tmp.path().join("my-skill.bak"); + + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::write(skill_dir.join("SKILL.md"), "# Test").unwrap(); + + create_backup(&skill_dir, &backup_dir).unwrap(); + + assert!(!skill_dir.exists()); + assert!(backup_dir.join("SKILL.md").exists()); + } + + #[test] + fn test_create_backup_replaces_existing_backup() { + let tmp = TempDir::new().unwrap(); + let skill_dir = tmp.path().join("my-skill"); + let backup_dir = tmp.path().join("my-skill.bak"); + + // Old backup + std::fs::create_dir_all(&backup_dir).unwrap(); + std::fs::write(backup_dir.join("old.md"), "old").unwrap(); + + // Current skill + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::write(skill_dir.join("new.md"), "new").unwrap(); + + create_backup(&skill_dir, &backup_dir).unwrap(); + + assert!(!backup_dir.join("old.md").exists()); + assert!(backup_dir.join("new.md").exists()); + } + + #[test] + fn test_create_backup_noop_when_skill_dir_missing() { + let tmp = TempDir::new().unwrap(); + let skill_dir = tmp.path().join("missing"); + let backup_dir = tmp.path().join("missing.bak"); + + let result = create_backup(&skill_dir, &backup_dir); + assert!(result.is_ok()); + assert!(!backup_dir.exists()); + } + + #[test] + fn test_copy_dir_all_copies_recursively() { + let tmp = TempDir::new().unwrap(); + let src = tmp.path().join("src"); + let dst = tmp.path().join("dst"); + + std::fs::create_dir_all(src.join("sub")).unwrap(); + std::fs::write(src.join("a.txt"), "a").unwrap(); + std::fs::write(src.join("sub/b.txt"), "b").unwrap(); + + copy_dir_all(&src, &dst).unwrap(); + + assert_eq!(std::fs::read_to_string(dst.join("a.txt")).unwrap(), "a"); + assert_eq!(std::fs::read_to_string(dst.join("sub/b.txt")).unwrap(), "b"); + } + + #[cfg(unix)] + #[test] + fn test_copy_dir_all_skips_symlinks() { + use std::os::unix::fs as unix_fs; + + let tmp = TempDir::new().unwrap(); + let src = tmp.path().join("src"); + let dst = tmp.path().join("dst"); + + std::fs::create_dir_all(&src).unwrap(); + std::fs::write(src.join("real.txt"), "real").unwrap(); + unix_fs::symlink("/etc/passwd", src.join("evil-link")).unwrap(); + + copy_dir_all(&src, &dst).unwrap(); + + assert!(dst.join("real.txt").exists()); + assert!(!dst.join("evil-link").exists()); + } +} diff --git a/src/update_check.rs b/src/update_check.rs index a3e12b28..c218a5ba 100644 --- a/src/update_check.rs +++ b/src/update_check.rs @@ -62,105 +62,101 @@ fn is_fresh(cache: &CheckedVersion) -> bool { true } -pub fn spawn() { - let no_check = std::env::var("AGENTSYNC_NO_UPDATE_CHECK") - .map(|v| v.eq_ignore_ascii_case("1")) - .unwrap_or(false); - if no_check { +/// Pure logic for determining whether to skip the update check, given the +/// relevant environment values and terminal state. +fn should_skip(no_update_check: Option<&str>, ci: Option<&str>, is_terminal: bool) -> bool { + if no_update_check.is_some_and(|v| v.eq_ignore_ascii_case("1")) { + return true; + } + if ci.is_some_and(|v| v.eq_ignore_ascii_case("true")) { + return true; + } + !is_terminal +} + +fn should_skip_update_check() -> bool { + let no_check = std::env::var("AGENTSYNC_NO_UPDATE_CHECK").ok(); + let ci = std::env::var("CI").ok(); + let is_terminal = std::io::stderr().is_terminal(); + should_skip(no_check.as_deref(), ci.as_deref(), is_terminal) +} + +fn fetch_latest_version() -> Option { + #[derive(Deserialize)] + struct CratesIoResponse { + #[serde(rename = "crate")] + krate: CrateInfo, + } + + #[derive(Deserialize)] + struct CrateInfo { + #[serde(rename = "newest_version")] + newest_version: String, + } + + let client = reqwest::blocking::Client::builder() + .user_agent(concat!("agentsync/", env!("CARGO_PKG_VERSION"))) + .timeout(std::time::Duration::from_secs(3)) + .build() + .ok()?; + + let response = client.get(CRATES_IO_URL).send().ok()?; + let info: CratesIoResponse = response.json().ok()?; + Some(info.krate.newest_version) +} + +fn check_and_notify() { + let cache = Cache { path: cache_path() }; + + if cache.load().is_some_and(|c| is_fresh(&c)) { return; } - let ci = std::env::var("CI") - .map(|v| v.eq_ignore_ascii_case("true")) - .unwrap_or(false); - if ci { + let Some(newest_version) = fetch_latest_version() else { + return; + }; + + let Ok(current) = Version::parse(env!("CARGO_PKG_VERSION")) else { + return; + }; + + let Ok(latest) = Version::parse(&newest_version) else { + return; + }; + + if !latest.pre.is_empty() || latest <= current { return; } - if !std::io::stderr().is_terminal() { + let new_cache = CheckedVersion { + last_checked: chrono::Utc::now().timestamp(), + latest_version: newest_version.clone(), + notified_for_version: Some(newest_version.clone()), + }; + + eprintln!( + "{} {}", + "💡".yellow().bold(), + format!( + "A new version of agentsync is available: {} (you have {}). Run cargo install agentsync to update.", + newest_version.yellow().bold(), + env!("CARGO_PKG_VERSION").dimmed() + ) + .yellow() + .bold() + ); + + let _ = cache.save(&new_cache); +} + +pub fn spawn() { + if should_skip_update_check() { return; } let _ = thread::Builder::new() .name("agentsync-update-check".to_string()) - .spawn(|| { - let path = cache_path(); - let cache = Cache { path }; - - if cache.load().is_some_and(|c| is_fresh(&c)) { - return; - } - - let client = match reqwest::blocking::Client::builder() - .timeout(std::time::Duration::from_secs(3)) - .build() - { - Ok(c) => c, - Err(_) => return, - }; - - let response = match client.get(CRATES_IO_URL).send() { - Ok(r) => r, - Err(_) => return, - }; - - #[derive(Deserialize)] - struct CratesIoResponse { - #[serde(rename = "crate")] - krate: CrateInfo, - } - - #[derive(Deserialize)] - struct CrateInfo { - #[serde(rename = "newest_version")] - newest_version: String, - } - - let info: CratesIoResponse = match response.json() { - Ok(v) => v, - Err(_) => return, - }; - - let current = match Version::parse(env!("CARGO_PKG_VERSION")) { - Ok(v) => v, - Err(_) => return, - }; - - let latest = match Version::parse(&info.krate.newest_version) { - Ok(v) => v, - Err(_) => return, - }; - - if !latest.pre.is_empty() { - return; - } - - if latest <= current { - return; - } - - let new_cache = CheckedVersion { - last_checked: chrono::Utc::now().timestamp(), - latest_version: info.krate.newest_version.clone(), - notified_for_version: Some(info.krate.newest_version.clone()), - }; - - if cache.save(&new_cache).is_err() { - return; - } - - eprintln!( - "{} {}", - "💡".yellow().bold(), - format!( - "A new version of agentsync is available: {} (you have {}). Run cargo install agentsync to update.", - info.krate.newest_version.yellow().bold(), - env!("CARGO_PKG_VERSION").dimmed() - ) - .yellow() - .bold() - ); - }); + .spawn(check_and_notify); } #[cfg(test)] @@ -284,4 +280,40 @@ mod tests { }; assert!(!is_fresh(&cache)); } + + #[test] + fn test_cache_not_fresh_if_notified_is_none() { + let cache = CheckedVersion { + last_checked: chrono::Utc::now().timestamp(), + latest_version: "1.0.0".to_string(), + notified_for_version: None, + }; + assert!(!is_fresh(&cache)); + } + + #[test] + fn test_should_skip_when_no_update_check_set() { + assert!(should_skip(Some("1"), None, true)); + } + + #[test] + fn test_should_skip_when_ci_set() { + assert!(should_skip(None, Some("true"), true)); + } + + #[test] + fn test_should_skip_no_update_check_only_skips_on_1() { + // "0" should not trigger skip (terminal=true means not skipped) + assert!(!should_skip(Some("0"), None, true)); + } + + #[test] + fn test_should_skip_when_not_terminal() { + assert!(should_skip(None, None, false)); + } + + #[test] + fn test_should_not_skip_when_all_clear() { + assert!(!should_skip(None, None, true)); + } }