Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions .claude/skills/final-review/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,15 @@ Pre-merge review: `/final-review`

## Process

### 0. Fetch Latest
### 0. Fetch Latest and Identify Changes

Run `git fetch origin main` to ensure comparisons use the latest main branch.

**IMPORTANT:** Always use `origin/main` (not `main`) for all diff comparisons to ensure you're comparing against the actual remote state, not a potentially stale local branch.

### 1. Test Coverage

- Run `git diff main --name-only` to identify changed files
- Run `git diff origin/main --name-only` to identify changed files
- Confirm each core module (`src/*.rs` excluding test modules) has corresponding tests
- Current modules requiring tests: `loader.rs`, `executor.rs`, `state.rs`
- Note: `main.rs`, `lib.rs`, `templates.rs`, and `src/commands/` do not require separate unit tests
Expand Down Expand Up @@ -57,7 +59,9 @@ Check for:

### 4. Version Update

Check `Cargo.toml` version against change scope:
Check if `Cargo.toml` version changed in this PR using `git diff origin/main -- Cargo.toml`.

Evaluate version against change scope:

- **Major:** Breaking changes (removed features, incompatible API changes)
- **Minor:** New features (new CLI commands, new public API functions)
Expand All @@ -78,8 +82,8 @@ To trigger a release, simply bump the version in `Cargo.toml` before merging.
### 5. PR Metadata (if PR exists)

- `gh pr view` - check current title/description
- `git log main..HEAD --oneline` - see commits
- `git diff main --stat` - see change scope
- `git log origin/main..HEAD --oneline` - see commits
- `git diff origin/main --stat` - see change scope

**Fix:** Use `gh pr edit --title` and `gh pr edit --body` to update.

Expand Down
4 changes: 2 additions & 2 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,10 @@ await fs.writeFile(`${projectRoot}/config.json`, '{}');
- `src/lib.rs` - Core types and public API
- `src/loader.rs` - Migration discovery
- `src/executor.rs` - Subprocess execution
- `src/state.rs` - History tracking (`.history` file)
- `src/state.rs` - History tracking (consolidated `history` file with applied migrations and baseline)
- `src/version.rs` - Base36 version generation and parsing
- `src/templates.rs` - Embedded migration templates
- `src/baseline.rs` - Baseline management (`.baseline` file)
- `src/baseline.rs` - Baseline validation and file deletion
- `src/commands/` - CLI command implementations
- `mod.rs` - Command module exports
- `status.rs` - Status command (shows version summary)
Expand Down
2 changes: 1 addition & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "migrate"
version = "0.4.0"
version = "0.4.1"
edition = "2021"
description = "Generic file migration tool for applying ordered transformations to a project directory"
license = "MIT"
Expand Down
7 changes: 3 additions & 4 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ await fs.writeFile(

### 3. Applying Migrations

Run `migrate up` to apply all pending migrations in order. Each successful migration is recorded in `.history`, so it won't run again.
Run `migrate up` to apply all pending migrations in order. Each successful migration is recorded in `history`, so it won't run again.

```bash
migrate up # Apply all pending
Expand Down Expand Up @@ -175,7 +175,7 @@ migrate up --baseline --keep # Apply and baseline without deleting files
- You want to reduce clutter in the migrations directory

**What baselining does:**
- Creates/updates `.baseline` file with the baseline version
- Records the baseline version in the `history` file
- Optionally deletes migration files at or before that version
- Future `migrate up` skips migrations covered by the baseline

Expand All @@ -184,8 +184,7 @@ migrate up --baseline --keep # Apply and baseline without deleting files
```
your-project/
├── migrations/
│ ├── .history # Tracks applied migrations (auto-generated)
│ ├── .baseline # Baseline marker (optional, from baselining)
│ ├── history # Tracks applied migrations and baseline (auto-generated)
│ ├── 1fc2h-add-prettier.sh
│ └── 1fc3h-configure-ci.ts
└── ...
Expand Down
161 changes: 6 additions & 155 deletions src/baseline.rs

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 validate_baseline fails to account for migrations covered by existing baseline

The validate_baseline function prevents moving a baseline forward when migrations covered by an existing baseline are not in the applied history.

Click to expand

Scenario

  1. User creates a baseline at version 1f700 (migration files deleted, baseline recorded)
  2. User applies new migrations 1f710 and 1f720 (these get recorded in history)
  3. User tries to move baseline forward to 1f720
  4. Validation fails with "Cannot baseline: migration '1f700-xxx' has not been applied"

Root Cause

At src/baseline.rs:64-71, the validation checks that ALL migrations at or before the target version are in the applied list:

for migration in available {
    if version_lte(&migration.version, version) && !applied_ids.contains(migration.id.as_str())
    {
        bail!("Cannot baseline: migration '{}' has not been applied", migration.id);
    }
}

However, the function receives existing_baseline (line 41) but only uses it to check for backward movement (lines 50-57). It doesn't consider that migrations at or before the existing baseline version should be treated as "applied" for validation purposes.

Impact

Users cannot advance their baseline once they have an initial baseline set, unless all prior migrations happen to still exist in the applied history. This breaks the intended workflow of progressively moving baselines forward over time.

(Refers to lines 64-71)

Recommendation: Modify the validation to also consider migrations covered by the existing baseline as 'applied'. Add a check like: if let Some(b) = existing_baseline { if version_lte(&migration.version, &b.version) { continue; } } before checking the applied_ids.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Original file line number Diff line number Diff line change
@@ -1,125 +1,8 @@
use anyhow::{bail, Context, Result};
use chrono::{DateTime, Utc};
use std::fs;
use std::path::Path;

const BASELINE_FILE: &str = ".baseline";

/// A baseline assertion: migrations with version <= this are no longer required as files
#[derive(Debug, Clone)]
pub struct Baseline {
/// Version string (e.g., "1fb2g")
pub version: String,
/// When the baseline was created
pub created: DateTime<Utc>,
/// Optional description of what migrations are included
pub summary: Option<String>,
}

/// Read the baseline file if it exists.
pub fn read_baseline(migrations_dir: &Path) -> Result<Option<Baseline>> {
let baseline_path = migrations_dir.join(BASELINE_FILE);

if !baseline_path.exists() {
return Ok(None);
}

let content = fs::read_to_string(&baseline_path)
.with_context(|| format!("Failed to read baseline file: {}", baseline_path.display()))?;

parse_baseline(&content).map(Some)
}

/// Write the baseline file.
pub fn write_baseline(migrations_dir: &Path, baseline: &Baseline) -> Result<()> {
let baseline_path = migrations_dir.join(BASELINE_FILE);

let mut content = format!(
"version: {}\ncreated: {}\n",
baseline.version,
baseline.created.to_rfc3339()
);

if let Some(summary) = &baseline.summary {
content.push_str("summary: |\n");
for line in summary.lines() {
content.push_str(" ");
content.push_str(line);
content.push('\n');
}
}

fs::write(&baseline_path, content)
.with_context(|| format!("Failed to write baseline file: {}", baseline_path.display()))?;

Ok(())
}

/// Parse baseline file content into a Baseline struct.
fn parse_baseline(content: &str) -> Result<Baseline> {
let mut version: Option<String> = None;
let mut created: Option<DateTime<Utc>> = None;
let mut summary: Option<String> = None;
let mut in_summary = false;
let mut summary_lines: Vec<String> = Vec::new();

for line in content.lines() {
if in_summary {
// Summary lines are indented with 2 spaces
if let Some(stripped) = line.strip_prefix(" ") {
summary_lines.push(stripped.to_string());
continue;
} else if line.starts_with(' ') || line.is_empty() {
// Still in summary block
if line.is_empty() {
summary_lines.push(String::new());
} else {
summary_lines.push(line.trim_start().to_string());
}
continue;
} else {
// End of summary block
in_summary = false;
summary = Some(summary_lines.join("\n").trim_end().to_string());
summary_lines.clear();
}
}

if let Some(stripped) = line.strip_prefix("version:") {
version = Some(stripped.trim().to_string());
} else if let Some(stripped) = line.strip_prefix("created:") {
let timestamp_str = stripped.trim();
created = Some(
DateTime::parse_from_rfc3339(timestamp_str)
.with_context(|| format!("Invalid timestamp in baseline: {}", timestamp_str))?
.with_timezone(&Utc),
);
} else if let Some(stripped) = line.strip_prefix("summary:") {
let rest = stripped.trim();
if rest == "|" {
// Multi-line summary
in_summary = true;
} else if !rest.is_empty() {
// Single-line summary
summary = Some(rest.to_string());
}
}
}

// Handle summary at end of file
if in_summary && !summary_lines.is_empty() {
summary = Some(summary_lines.join("\n").trim_end().to_string());
}

let version = version.context("Baseline file missing 'version' field")?;
let created = created.context("Baseline file missing 'created' field")?;

Ok(Baseline {
version,
created,
summary,
})
}
use crate::state::Baseline;
use crate::{AppliedMigration, Migration};

/// Compare two version strings. Returns true if v1 <= v2.
pub fn version_lte(v1: &str, v2: &str) -> bool {
Expand All @@ -130,7 +13,7 @@ pub fn version_lte(v1: &str, v2: &str) -> bool {
/// Returns the list of deleted file paths.
pub fn delete_baselined_migrations(
baseline_version: &str,
available: &[crate::Migration],
available: &[Migration],
) -> Result<Vec<String>> {
let mut deleted = Vec::new();

Expand All @@ -153,8 +36,8 @@ pub fn delete_baselined_migrations(
/// Returns an error if validation fails.
pub fn validate_baseline(
version: &str,
available: &[crate::Migration],
applied: &[crate::AppliedMigration],
available: &[Migration],
applied: &[AppliedMigration],
existing_baseline: Option<&Baseline>,
) -> Result<()> {
// Check if the version matches any migration
Expand Down Expand Up @@ -194,41 +77,9 @@ pub fn validate_baseline(
#[cfg(test)]
mod tests {
use super::*;
use crate::{AppliedMigration, Migration};
use chrono::Utc;
use std::path::PathBuf;

#[test]
fn test_parse_baseline_simple() {
let content = "version: 1fb2g\ncreated: 2024-06-15T14:30:00Z\n";
let baseline = parse_baseline(content).unwrap();
assert_eq!(baseline.version, "1fb2g");
assert!(baseline.summary.is_none());
}

#[test]
fn test_parse_baseline_with_summary() {
let content = r#"version: 1fb2g
created: 2024-06-15T14:30:00Z
summary: |
Initial project setup
TypeScript config
"#;
let baseline = parse_baseline(content).unwrap();
assert_eq!(baseline.version, "1fb2g");
assert_eq!(
baseline.summary,
Some("Initial project setup\nTypeScript config".to_string())
);
}

#[test]
fn test_parse_baseline_single_line_summary() {
let content = "version: 1fb2g\ncreated: 2024-06-15T14:30:00Z\nsummary: Initial setup\n";
let baseline = parse_baseline(content).unwrap();
assert_eq!(baseline.version, "1fb2g");
assert_eq!(baseline.summary, Some("Initial setup".to_string()));
}

#[test]
fn test_version_lte() {
assert!(version_lte("1f700", "1f700"));
Expand Down
16 changes: 6 additions & 10 deletions src/commands/baseline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,9 @@ use anyhow::Result;
use chrono::Utc;
use std::path::Path;

use crate::baseline::{
delete_baselined_migrations, read_baseline, validate_baseline, write_baseline, Baseline,
};

use crate::baseline::{delete_baselined_migrations, validate_baseline};
use crate::loader::discover_migrations;
use crate::state::read_history;
use crate::state::{append_baseline, read_history, Baseline};

/// Create a baseline at the specified version
pub fn run(
Expand All @@ -33,11 +30,10 @@ pub fn run(
}

let available = discover_migrations(&migrations_path)?;
let applied = read_history(&migrations_path)?;
let existing_baseline = read_baseline(&migrations_path)?;
let state = read_history(&migrations_path)?;

// Validate the baseline
validate_baseline(version, &available, &applied, existing_baseline.as_ref())?;
validate_baseline(version, &available, &state.applied, state.baseline.as_ref())?;

// Find migrations that would be deleted
let to_delete: Vec<_> = available
Expand Down Expand Up @@ -82,8 +78,8 @@ pub fn run(
summary: summary.map(|s| s.to_string()),
};

write_baseline(&migrations_path, &baseline)?;
println!("Created .baseline file");
append_baseline(&migrations_path, &baseline)?;
println!("Added baseline to history file");

// Delete old migration files unless --keep was specified
if !keep && !to_delete.is_empty() {
Expand Down
Loading