diff --git a/src/changelog.rs b/src/changelog.rs index 3ef2383a..6b590bbb 100644 --- a/src/changelog.rs +++ b/src/changelog.rs @@ -83,12 +83,60 @@ fn read_file_at_ref(root: &Path, git_ref: &str, file_path: &str) -> Option Option<(String, String)> { - let parts: Vec<&str> = range.splitn(2, "..").collect(); - if parts.len() == 2 && !parts[0].is_empty() && !parts[1].is_empty() { - Some((parts[0].to_string(), parts[1].to_string())) + parse_range_full(range).map(|(from, to, _)| (from, to)) +} + +/// Parse a range into (from, to, three_dot). A three-dot range (`A...B`, +/// changes since the merge-base) must be split on `...` FIRST — splitting on +/// `..` mangles it into `A` / `.B`, and the bogus `.B` ref then silently +/// produced fabricated "removed" entries (#418). +pub fn parse_range_full(range: &str) -> Option<(String, String, bool)> { + if let Some((from, to)) = range.split_once("...") { + if !from.is_empty() && !to.is_empty() { + return Some((from.to_string(), to.to_string(), true)); + } + return None; + } + let (from, to) = range.split_once("..")?; + if from.is_empty() || to.is_empty() { + return None; + } + Some((from.to_string(), to.to_string(), false)) +} + +/// Verify that a git ref resolves to a real commit (`git rev-parse --verify`). +/// Returns the resolved full hash. +pub fn resolve_ref(root: &Path, git_ref: &str) -> Result { + let output = Command::new("git") + .args([ + "rev-parse", + "--verify", + "--end-of-options", + &format!("{git_ref}^{{commit}}"), + ]) + .current_dir(root) + .output() + .map_err(|e| format!("failed to run git rev-parse: {e}"))?; + if output.status.success() { + Ok(String::from_utf8_lossy(&output.stdout).trim().to_string()) } else { - None + Err(format!("unknown revision '{git_ref}'")) + } +} + +/// Compute the merge-base of two refs (`git merge-base`). +pub fn merge_base(root: &Path, a: &str, b: &str) -> Result { + let output = Command::new("git") + .args(["merge-base", "--end-of-options", a, b]) + .current_dir(root) + .output() + .map_err(|e| format!("failed to run git merge-base: {e}"))?; + if output.status.success() { + Ok(String::from_utf8_lossy(&output.stdout).trim().to_string()) + } else { + Err(format!("no merge-base between '{a}' and '{b}'")) } } diff --git a/src/cli.rs b/src/cli.rs index 68e30105..70421062 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -148,11 +148,11 @@ pub enum Command { }, /// Show export changes since last commit (useful for CI/PR comments) Diff { - /// Git ref to compare against (default: HEAD). - /// In GitHub Actions PR context, auto-detects the base branch - /// from GITHUB_BASE_REF when set to HEAD. - #[arg(long, default_value = "HEAD")] - base: String, + /// Git ref to compare against (default: auto-detect the PR base from + /// GITHUB_BASE_REF in GitHub Actions pull_request* events, else HEAD). + /// An explicit value always wins over environment auto-detection. + #[arg(long)] + base: Option, }, /// Manage agent instruction files and git hooks for spec awareness Hooks { diff --git a/src/commands/changelog.rs b/src/commands/changelog.rs index 8cbfeda3..0df0ccc3 100644 --- a/src/commands/changelog.rs +++ b/src/commands/changelog.rs @@ -7,7 +7,7 @@ use crate::config::load_config; use crate::types; pub fn cmd_changelog(root: &Path, range: &str, format: types::OutputFormat) { - let (from_ref, to_ref) = match changelog::parse_range(range) { + let (from_ref, to_ref, three_dot) = match changelog::parse_range_full(range) { Some(r) => r, None => { eprintln!( @@ -18,6 +18,37 @@ pub fn cmd_changelog(root: &Path, range: &str, format: types::OutputFormat) { } }; + // Validate BOTH endpoints before comparing: an unresolvable ref used to be + // silently treated as an empty tree, fabricating a plausible changelog + // with exit 0 — exactly the wrong output for CI (#418). + let to_ref = match changelog::resolve_ref(root, &to_ref) { + Ok(_) => to_ref, + Err(e) => { + eprintln!("{} {e}", "Error:".red().bold()); + process::exit(1); + } + }; + let from_ref = match changelog::resolve_ref(root, &from_ref) { + Ok(_) => from_ref, + Err(e) => { + eprintln!("{} {e}", "Error:".red().bold()); + process::exit(1); + } + }; + + // Three-dot range (A...B): compare from the real merge-base, not from A. + let from_ref = if three_dot { + match changelog::merge_base(root, &from_ref, &to_ref) { + Ok(base) => base, + Err(e) => { + eprintln!("{} {e}", "Error:".red().bold()); + process::exit(1); + } + } + } else { + from_ref + }; + let config = load_config(root); let report = changelog::generate_changelog(root, &config.specs_dir, &from_ref, &to_ref); diff --git a/src/commands/check.rs b/src/commands/check.rs index d089c5f6..a1c88e19 100644 --- a/src/commands/check.rs +++ b/src/commands/check.rs @@ -16,7 +16,8 @@ use crate::validator::{compute_coverage, get_schema_table_names}; use crate::config::is_legacy_layout; use super::{ - build_schema_columns, compute_exit_code, create_drift_issues, exit_with_status, + build_schema_columns, compute_exit_code, create_drift_issues, default_enforcement, + exit_with_status, filter_by_status, filter_specs, load_and_discover, run_validation, }; @@ -136,7 +137,7 @@ pub fn cmd_check( let enforcement = enforcement.unwrap_or(if strict { types::EnforcementMode::Strict } else { - config.enforcement + default_enforcement(&config) }); // Spec name filters that matched nothing are an error — don't fall through diff --git a/src/commands/comment.rs b/src/commands/comment.rs index a835f2ec..79b60868 100644 --- a/src/commands/comment.rs +++ b/src/commands/comment.rs @@ -9,7 +9,9 @@ use crate::ignore::IgnoreRules; use crate::types; use crate::validator::{compute_coverage, get_schema_table_names}; -use super::{build_schema_columns, compute_exit_code, load_and_discover, run_validation}; +use super::{ + build_schema_columns, compute_exit_code, default_enforcement, load_and_discover, run_validation, +}; pub fn cmd_comment( root: &Path, @@ -29,7 +31,7 @@ pub fn cmd_comment( let enforcement = enforcement.unwrap_or(if strict { types::EnforcementMode::Strict } else { - config.enforcement + default_enforcement(&config) }); // Use the same validation pipeline as `check` for consistent results diff --git a/src/commands/coverage.rs b/src/commands/coverage.rs index ef04d95c..c5ff5586 100644 --- a/src/commands/coverage.rs +++ b/src/commands/coverage.rs @@ -6,7 +6,8 @@ use crate::types; use crate::validator::{compute_coverage, get_schema_table_names}; use super::{ - build_schema_columns, compute_exit_code, exit_with_status, load_and_discover, run_validation, + build_schema_columns, compute_exit_code, default_enforcement, exit_with_status, + load_and_discover, run_validation, }; pub fn cmd_coverage( @@ -26,7 +27,7 @@ pub fn cmd_coverage( let enforcement = enforcement.unwrap_or(if strict { types::EnforcementMode::Strict } else { - config.enforcement + default_enforcement(&config) }); let schema_tables = get_schema_table_names(root, &config); let schema_columns = build_schema_columns(root, &config); @@ -79,6 +80,7 @@ pub fn cmd_coverage( "loc_total": coverage.total_loc, "modules": modules, "uncovered_files": uncovered_files, + "missing_files": coverage.missing_files, }); println!("{}", serde_json::to_string_pretty(&output).unwrap()); // Gate the exit code for machine consumers too (was an unconditional diff --git a/src/commands/deps.rs b/src/commands/deps.rs index 67eaa507..3c616bc5 100644 --- a/src/commands/deps.rs +++ b/src/commands/deps.rs @@ -6,7 +6,17 @@ use crate::config::load_config; use crate::deps; use crate::types; -pub fn cmd_deps(root: &Path, strict: bool, format: types::OutputFormat, mermaid: bool, dot: bool) { +use super::{compute_exit_code, default_enforcement, load_and_discover}; + +pub fn cmd_deps( + root: &Path, + strict: bool, + enforcement: Option, + require_coverage: Option, + format: types::OutputFormat, + mermaid: bool, + dot: bool, +) { let config = load_config(root); // --mermaid or --dot: output graph visualization and exit @@ -140,6 +150,37 @@ pub fn cmd_deps(root: &Path, strict: bool, format: types::OutputFormat, mermaid: if !report.errors.is_empty() || strict_fail { process::exit(1); } + + // --require-coverage gate (#419): was completely inert. Evaluate the same + // coverage computation `specsync coverage` uses and fail below the + // threshold. Honored in every output format; JSON/machine formats gate + // silently via the exit code (handled by not printing here when Json). + if let Some(req) = require_coverage { + let (_, spec_files) = load_and_discover(root, true); + let coverage = crate::validator::compute_coverage(root, &spec_files, &config); + let enforcement = + enforcement.unwrap_or_else(|| default_enforcement(&config)); + let code = compute_exit_code(0, 0, strict, enforcement, &coverage, Some(req)); + if format != types::OutputFormat::Json { + if code == 0 { + println!( + " {} Coverage {}% meets --require-coverage {req}%", + "✓".green(), + coverage.coverage_percent + ); + } else { + eprintln!( + "{} {req}%: actual coverage is {}% ({} file(s) missing specs)", + "--require-coverage".red(), + coverage.coverage_percent, + coverage.unspecced_files.len() + ); + } + } + if code != 0 { + process::exit(code); + } + } } /// Render the dependency graph as a Mermaid flowchart diagram. diff --git a/src/commands/diff.rs b/src/commands/diff.rs index a1b08054..7542c542 100644 --- a/src/commands/diff.rs +++ b/src/commands/diff.rs @@ -9,15 +9,37 @@ use crate::types; use super::load_and_discover; -pub fn cmd_diff(root: &Path, base: &str, format: types::OutputFormat) { +pub fn cmd_diff(root: &Path, base: Option<&str>, format: types::OutputFormat, strict: bool) { let (config, spec_files) = load_and_discover(root, false); - // Auto-detect PR context: when base is default "HEAD" and we're in a GitHub - // Actions pull_request event, compare against the PR's base branch instead. - let effective_base = if base == "HEAD" { - detect_pr_base().unwrap_or_else(|| base.to_string()) - } else { - base.to_string() + // An explicit --base always wins over environment auto-detection (a literal + // `--base HEAD` used to be hijacked by the PR-context fallback, since HEAD + // doubled as the "not specified" sentinel). + let effective_base = match base { + Some(b) => b.to_string(), + None => match detect_pr_base() { + Some(b) => b, + None => { + // Silent HEAD fallback = silent no-op in the default CI states + // (detached HEAD, push events): the working tree vs HEAD is + // empty by construction in a clean checkout, so nothing is + // ever compared while the run greens. Say so loudly. + eprintln!( + "specsync: no PR base detected (GITHUB_EVENT_NAME is not a pull_request* event); comparing against HEAD" + ); + eprintln!( + " In a clean CI checkout this compares nothing — pass --base to choose a real comparison base." + ); + if strict { + eprintln!( + "{} no usable base ref and none given — refusing to silently diff HEAD", + "--strict mode".red() + ); + process::exit(1); + } + "HEAD".to_string() + } + }, }; let base = effective_base.as_str(); @@ -318,7 +340,8 @@ pub fn cmd_diff(root: &Path, base: &str, format: types::OutputFormat) { /// or `None` otherwise. fn detect_pr_base() -> Option { let event = std::env::var("GITHUB_EVENT_NAME").ok()?; - if event != "pull_request" && event != "pull_request_target" { + // GitHub sets GITHUB_BASE_REF for all three PR-flavored events. + if event != "pull_request" && event != "pull_request_target" && event != "pull_request_review" { return None; } let base_ref = std::env::var("GITHUB_BASE_REF").ok()?; diff --git a/src/commands/generate.rs b/src/commands/generate.rs index d934cb50..dad9fc55 100644 --- a/src/commands/generate.rs +++ b/src/commands/generate.rs @@ -10,7 +10,8 @@ use crate::types; use crate::validator::{compute_coverage, get_schema_table_names}; use super::{ - build_schema_columns, compute_exit_code, exit_with_status, load_and_discover, run_validation, + build_schema_columns, compute_exit_code, default_enforcement, exit_with_status, + load_and_discover, run_validation, }; #[allow(clippy::too_many_arguments)] @@ -50,7 +51,7 @@ fn cmd_generate_all( let enforcement = enforcement.unwrap_or(if strict { types::EnforcementMode::Strict } else { - config.enforcement + default_enforcement(&config) }); let schema_tables = get_schema_table_names(root, &config); let schema_columns = build_schema_columns(root, &config); @@ -212,7 +213,7 @@ fn cmd_generate_batch( let enforcement = enforcement.unwrap_or(if strict { types::EnforcementMode::Strict } else { - config.enforcement + default_enforcement(&config) }); let coverage = compute_coverage(root, &spec_files, &config); diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 1b976378..d0ac0526 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -669,6 +669,24 @@ fn colorize_subscore(score: u32) -> String { } } +/// Resolve the enforcement mode when the user did NOT pass `--enforcement`. +/// +/// Consistent convention across all gate commands (`check`, `coverage`, +/// `report`, `score`, `generate`, `comment`): +/// - an explicit `enforcement` key in the config file is honored as-is +/// (including `warn`, which always exits 0); +/// - when enforcement is not configured anywhere, the default GATES ON +/// ERRORS (`Strict`-like): validation errors exit 1. Warnings remain +/// non-blocking unless `--strict` is also passed. This is the only safe +/// default for CI — a command that prints "1 failed" must not exit 0. +pub fn default_enforcement(config: &types::SpecSyncConfig) -> types::EnforcementMode { + if config.enforcement_set { + config.enforcement + } else { + types::EnforcementMode::Strict + } +} + /// Compute exit code without printing or exiting. pub fn compute_exit_code( total_errors: usize, diff --git a/src/commands/report.rs b/src/commands/report.rs index b14cca39..3f761130 100644 --- a/src/commands/report.rs +++ b/src/commands/report.rs @@ -7,15 +7,25 @@ use crate::parser; use crate::types; use crate::validator::compute_coverage; -use super::{filter_by_status, load_and_discover}; +use super::{compute_exit_code, default_enforcement, filter_by_status, load_and_discover}; +#[allow(clippy::too_many_arguments)] pub fn cmd_report( root: &Path, format: types::OutputFormat, stale_threshold: usize, exclude_status: &[String], only_status: &[String], + strict: bool, + enforcement: Option, + require_coverage: Option, ) { + let enforcement = enforcement.unwrap_or(if strict { + types::EnforcementMode::Strict + } else { + let config = crate::config::load_config(root); + default_enforcement(&config) + }); let (config, all_spec_files) = load_and_discover(root, true); let spec_files = filter_by_status(&all_spec_files, exclude_status, only_status); let coverage = compute_coverage(root, &spec_files, &config); @@ -26,6 +36,7 @@ pub fn cmd_report( module_name: String, source_files: Vec, coverage_pct: f64, + status: Option, stale: bool, stale_commits_behind: usize, incomplete: bool, @@ -81,9 +92,11 @@ pub fn cmd_report( continue; } let behind = git_commits_since(root, &spec_commit, source_file); + // Always track the real drift: `commits_behind` must reflect + // sub-threshold drift too, not only once the module is stale. + max_behind = max_behind.max(behind); if behind >= stale_threshold { stale = true; - max_behind = max_behind.max(behind); } } } @@ -135,6 +148,7 @@ pub fn cmd_report( module_name, source_files: fm.files.clone(), coverage_pct: cov, + status: fm.status.clone(), stale, stale_commits_behind: max_behind, incomplete, @@ -163,6 +177,7 @@ pub fn cmd_report( serde_json::json!({ "module": m.module_name, "spec_path": m.spec_path, + "status": m.status, "source_files": m.source_files, "coverage_pct": (m.coverage_pct * 100.0).round() / 100.0, "stale": m.stale, @@ -185,6 +200,57 @@ pub fn cmd_report( "modules": module_json, }); println!("{}", serde_json::to_string_pretty(&output).unwrap()); + // Machine consumers get the gate via the exit code alone (printing + // the human gate message would corrupt the JSON on stdout). + std::process::exit(compute_exit_code( + stale_count + incomplete_count, + 0, + strict, + enforcement, + &coverage, + require_coverage, + )); + } + types::OutputFormat::Csv => { + println!("module,spec_path,status,coverage_pct,stale,commits_behind,incomplete"); + for m in &modules { + println!( + "{},{},{},{:.0},{},{},{}", + m.module_name, + m.spec_path, + m.status.as_deref().unwrap_or(""), + m.coverage_pct, + m.stale, + m.stale_commits_behind, + m.incomplete, + ); + } + println!( + "SUMMARY,,,{overall_coverage:.1},{stale_count},,{incomplete_count}", + ); + } + types::OutputFormat::Markdown | types::OutputFormat::Github => { + println!("## Spec Coverage Report\n"); + println!( + "**Overall:** {}/{} files covered ({:.1}%) ", + coverage.specced_file_count, coverage.total_source_files, overall_coverage, + ); + println!( + "**Modules:** {total_modules} total, {stale_count} stale, {incomplete_count} incomplete\n" + ); + println!("| Module | Status | Coverage | Stale | Commits Behind | Incomplete |"); + println!("|--------|--------|----------|-------|----------------|------------|"); + for m in &modules { + println!( + "| {} | {} | {:.0}% | {} | {} | {} |", + m.module_name, + m.status.as_deref().unwrap_or(""), + m.coverage_pct, + if m.stale { "yes" } else { "no" }, + m.stale_commits_behind, + if m.incomplete { "yes" } else { "no" }, + ); + } } _ => { println!( @@ -275,4 +341,29 @@ pub fn cmd_report( println!(); } } + + // CI gating (#430): every enforcement path must be able to fail the + // process. Stale and incomplete modules count as failures; `--enforcement + // warn` still exits 0, `enforce-new` gates on unspecced files, and + // `--require-coverage` gates on real file coverage. CSV is a machine + // format — gate silently via the exit code like JSON (handled above). + let failures = stale_count + incomplete_count; + if matches!(format, types::OutputFormat::Csv) { + std::process::exit(compute_exit_code( + failures, + 0, + strict, + enforcement, + &coverage, + require_coverage, + )); + } + super::exit_with_status( + failures, + 0, + strict, + enforcement, + &coverage, + require_coverage, + ); } diff --git a/src/commands/resolve.rs b/src/commands/resolve.rs index 490a8096..6b34c4c6 100644 --- a/src/commands/resolve.rs +++ b/src/commands/resolve.rs @@ -79,10 +79,42 @@ impl SpecCache { } } -pub fn cmd_resolve(root: &Path, remote: bool, verify: bool, cache_ttl: u64) { +/// Verdict for a single local dependency entry. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum LocalVerdict { + /// The path exists and is confined to the project root. + Ok, + /// The path does not exist. + NotFound, + /// The path escapes the project root (absolute or `..` traversal). + OutsideRoot, +} + +impl LocalVerdict { + fn as_str(self) -> &'static str { + match self { + LocalVerdict::Ok => "ok", + LocalVerdict::NotFound => "not-found", + LocalVerdict::OutsideRoot => "outside-root", + } + } +} + +pub fn cmd_resolve( + root: &Path, + remote: bool, + verify: bool, + cache_ttl: u64, + format: crate::types::OutputFormat, + strict: bool, +) { + let json = matches!(format, crate::types::OutputFormat::Json); let (_config, spec_files) = load_and_discover(root, false); let mut cross_refs: Vec<(String, String, String)> = Vec::new(); - let mut local_refs: Vec<(String, String, bool)> = Vec::new(); + let mut local_refs: Vec<(String, String, LocalVerdict)> = Vec::new(); + // depends_on entries that look like refs but don't parse (e.g. `owner/repo@` + // with no module) — previously these vanished silently (#444). + let mut malformed_refs: Vec<(String, String)> = Vec::new(); // Track local spec exports for bidirectional checking let mut local_exports: HashMap> = HashMap::new(); @@ -114,43 +146,93 @@ pub fn cmd_resolve(root: &Path, remote: bool, verify: bool, cache_ttl: u64) { for dep in &parsed.frontmatter.depends_on { if validator::is_cross_project_ref(dep) { - if let Some((repo, module)) = validator::parse_cross_project_ref(dep) { - cross_refs.push((spec_path.clone(), repo.to_string(), module.to_string())); + match validator::parse_cross_project_ref(dep) { + Some((repo, module)) => { + cross_refs.push((spec_path.clone(), repo.to_string(), module.to_string())) + } + // Looks cross-project (`owner/repo@…`) but doesn't parse — + // surface it instead of dropping it silently. + None => malformed_refs.push((spec_path.clone(), dep.clone())), } + } else if dep.contains('@') { + // `repo@` / `@module` shapes: not local paths, not valid refs. + malformed_refs.push((spec_path.clone(), dep.clone())); } else { - let exists = root.join(dep).exists(); - local_refs.push((spec_path.clone(), dep.clone(), exists)); + // Confine to the project root: absolute paths and `..` + // traversal entries must never validate (#444). + let verdict = match crate::util::confine_path_to_root(root, dep) { + Some(path) if path.exists() => LocalVerdict::Ok, + Some(_) => LocalVerdict::NotFound, + None => LocalVerdict::OutsideRoot, + }; + local_refs.push((spec_path.clone(), dep.clone(), verdict)); } } } - println!( - "\n--- {} ------------------------------------------------", - "Dependency Resolution".bold() - ); + if !json { + println!( + "\n--- {} ------------------------------------------------", + "Dependency Resolution".bold() + ); + } - if local_refs.is_empty() && cross_refs.is_empty() { - println!("\n No dependencies declared in any spec."); + if local_refs.is_empty() && cross_refs.is_empty() && malformed_refs.is_empty() { + if json { + println!("{\"dependencies\":[],\"passed\":true}"); + } else { + println!("\n No dependencies declared in any spec."); + } return; } - if !local_refs.is_empty() { - println!("\n {} Local dependencies:", "Local".bold()); - for (spec, dep, exists) in &local_refs { - if *exists { - println!(" {} {spec} -> {dep}", "✓".green()); - } else { - println!(" {} {spec} -> {dep} (not found)", "✗".red()); + if !json && !local_refs.is_empty() { + println!("\n {}", "Local dependencies:".bold()); + for (spec, dep, verdict) in &local_refs { + match verdict { + LocalVerdict::Ok => println!(" {} {spec} -> {dep}", "✓".green()), + LocalVerdict::NotFound => { + println!(" {} {spec} -> {dep} (not found)", "✗".red()) + } + LocalVerdict::OutsideRoot => println!( + " {} {spec} -> {dep} (path escapes the project root)", + "✗".red() + ), } } } + if !json && !malformed_refs.is_empty() { + for (spec, dep) in &malformed_refs { + println!( + " {} {spec} -> {dep} (malformed reference — expected `owner/repo@module` or a project-relative path)", + "✗".red() + ); + } + } + + // Local verdicts are computed up-front so both the text and JSON paths — + // and the final exit code — share one source of truth (#444: unresolved + // local deps must fail the process even without --strict). + let unresolved_local = local_refs + .iter() + .filter(|(_, _, v)| *v != LocalVerdict::Ok) + .count() + + malformed_refs.len(); + + // Remote verification state — shared by the text report, the JSON report, + // and the final exit code so there is exactly one verdict (#422). + let mut remote_errors = 0usize; + let mut fetch_failures: Vec = Vec::new(); + let mut drift_issues: Vec = Vec::new(); + if !cross_refs.is_empty() { - println!("\n {} Cross-project references:", "Remote".bold()); + if !json { + println!("\n {} Cross-project references:", "Remote".bold()); + } if remote { // Fetch remote registries to verify cross-project refs - let mut remote_errors = 0; // Group refs by repo to avoid duplicate fetches let mut repos: HashMap> = HashMap::new(); @@ -160,10 +242,16 @@ pub fn cmd_resolve(root: &Path, remote: bool, verify: bool, cache_ttl: u64) { .or_insert_with(|| match registry::fetch_remote_registry(repo) { Ok(reg) => Some(reg), Err(e) => { - eprintln!( - " {} Failed to fetch registry for {repo}: {e}", - "!".yellow() - ); + // A failed fetch is a failed verification, not a + // neutral "?" — the ref cannot be confirmed, so the + // run must not pass (#422). + fetch_failures.push(repo.clone()); + if !json { + eprintln!( + " {} Failed to fetch registry for {repo}: {e}", + "!".yellow() + ); + } None } }); @@ -174,40 +262,48 @@ pub fn cmd_resolve(root: &Path, remote: bool, verify: bool, cache_ttl: u64) { match repos.get(repo) { Some(Some(reg)) => { if reg.has_spec(module) { - println!(" {} {spec} -> {repo}@{module}", "✓".green()); + if !json { + println!(" {} {spec} -> {repo}@{module}", "✓".green()); + } } else { - println!( - " {} {spec} -> {repo}@{module} (module not in registry)", - "✗".red() - ); + if !json { + println!( + " {} {spec} -> {repo}@{module} (module not in registry)", + "✗".red() + ); + } remote_errors += 1; } } Some(None) => { - println!( - " {} {spec} -> {repo}@{module} (registry fetch failed)", - "?".yellow() - ); + if !json { + println!( + " {} {spec} -> {repo}@{module} (registry fetch failed)", + "✗".red() + ); + } } None => { - println!( - " {} {spec} -> {repo}@{module} (no registry)", - "?".yellow() - ); + if !json { + println!( + " {} {spec} -> {repo}@{module} (no registry)", + "?".yellow() + ); + } } } } - if remote_errors > 0 { + if remote_errors > 0 && !json { println!( "\n {} {remote_errors} cross-project ref(s) could not be verified", - "Warning:".yellow() + "Error:".red().bold() ); } // Phase 2: Deep content verification (--verify) if verify { - let drift_issues = verify_remote_specs( + let drift_issues_found = verify_remote_specs( &cross_refs, &repos, &local_exports, @@ -215,14 +311,13 @@ pub fn cmd_resolve(root: &Path, remote: bool, verify: bool, cache_ttl: u64) { root, cache_ttl, ); + drift_issues = drift_issues_found; - if !drift_issues.is_empty() { + if !drift_issues.is_empty() && !json { println!("\n {} Content verification:", "Verify".bold()); - let mut drift_count = 0; for result in &drift_issues { for issue in &result.issues { - drift_count += 1; match issue { DriftIssue::Deprecated { status } => { println!( @@ -270,7 +365,7 @@ pub fn cmd_resolve(root: &Path, remote: bool, verify: bool, cache_ttl: u64) { } } - let drift_errors: usize = drift_issues + let drift_error_count: usize = drift_issues .iter() .flat_map(|r| &r.issues) .filter(|i| { @@ -281,26 +376,37 @@ pub fn cmd_resolve(root: &Path, remote: bool, verify: bool, cache_ttl: u64) { }) .count(); - if drift_errors > 0 { + if drift_error_count > 0 { println!( - "\n {} {drift_errors} drift issue(s) detected — specs have diverged from remote", + "\n {} {drift_error_count} drift issue(s) detected — specs have diverged from remote", "Error:".red().bold() ); - std::process::exit(1); } else { println!( - "\n {} {drift_count} warning(s), no breaking drift", - "Info:".cyan() + "\n {} {} warning(s), no breaking drift", + "Info:".cyan(), + drift_issues.iter().flat_map(|r| &r.issues).count() + ); + } + } else if !json { + // Never print this when zero registries were actually + // fetched — "verified" over zero successful fetches is a + // false green (#422). + if fetch_failures.is_empty() { + println!( + "\n {} All cross-project references verified — no drift detected", + "✓".green() + ); + } else { + println!( + "\n {} could not verify {} repo(s) — registry fetch failed", + "✗".red(), + fetch_failures.len() ); } - } else { - println!( - "\n {} All cross-project references verified — no drift detected", - "✓".green() - ); } } - } else { + } else if !json { for (spec, repo, module) in &cross_refs { println!(" {} {spec} -> {repo}@{module}", "→".cyan()); } @@ -312,6 +418,82 @@ pub fn cmd_resolve(root: &Path, remote: bool, verify: bool, cache_ttl: u64) { println!(" Use --verify for deep content verification and drift detection."); } } + + // ─── One verdict, one exit code (#422/#444) ──────────────────────── + let drift_errors: usize = drift_issues + .iter() + .flat_map(|r| &r.issues) + .filter(|i| matches!(i, DriftIssue::Deprecated { .. } | DriftIssue::MissingExport { .. })) + .count(); + let drift_warnings: usize = drift_issues.iter().flat_map(|r| &r.issues).count() - drift_errors; + + let mut failure_reasons: Vec = Vec::new(); + if unresolved_local > 0 { + failure_reasons.push(format!("{unresolved_local} unresolved local dependency(ies)")); + } + if remote_errors > 0 { + failure_reasons.push(format!( + "{remote_errors} cross-project ref(s) not found in remote registry" + )); + } + if !fetch_failures.is_empty() { + failure_reasons.push(format!( + "registry fetch failed for {} repo(s): {}", + fetch_failures.len(), + fetch_failures.join(", ") + )); + } + if drift_errors > 0 { + failure_reasons.push(format!("{drift_errors} drift issue(s) detected")); + } + // --strict makes verification warnings (non-bidirectional, unparsable or + // unfetchable remote spec content) fatal, matching `check --strict`. + if strict && drift_warnings > 0 { + failure_reasons.push(format!( + "{drift_warnings} verification warning(s) treated as errors (--strict)" + )); + } + let passed = failure_reasons.is_empty(); + + if json { + let local_json: Vec = local_refs + .iter() + .map(|(spec, dep, verdict)| { + serde_json::json!({"spec": spec, "dep": dep, "status": verdict.as_str()}) + }) + .collect(); + let cross_json: Vec = cross_refs + .iter() + .map(|(spec, repo, module)| { + serde_json::json!({"spec": spec, "repo": repo, "module": module}) + }) + .collect(); + let malformed_json: Vec = malformed_refs + .iter() + .map(|(spec, dep)| serde_json::json!({"spec": spec, "dep": dep})) + .collect(); + println!( + "{}", + serde_json::to_string_pretty(&serde_json::json!({ + "passed": passed, + "local": local_json, + "cross_project": cross_json, + "malformed": malformed_json, + "fetch_failures": fetch_failures, + "failures": failure_reasons, + })) + .unwrap() + ); + } else if !passed { + eprintln!("\n{}", "Dependency resolution FAILED:".red().bold()); + for reason in &failure_reasons { + eprintln!(" {} {reason}", "✗".red()); + } + } + + if !passed { + std::process::exit(1); + } } /// Deep-verify remote spec content for drift. diff --git a/src/commands/score.rs b/src/commands/score.rs index 2e6ee808..81392045 100644 --- a/src/commands/score.rs +++ b/src/commands/score.rs @@ -6,7 +6,8 @@ use crate::types; use crate::validator::compute_coverage; use super::{ - compute_exit_code, exit_with_status, filter_by_status, filter_specs, load_and_discover, + compute_exit_code, default_enforcement, exit_with_status, filter_by_status, filter_specs, + load_and_discover, }; #[allow(clippy::too_many_arguments)] @@ -34,12 +35,15 @@ pub fn cmd_score( if strict { types::EnforcementMode::Strict } else { - crate::config::load_config(root).enforcement + default_enforcement(&crate::config::load_config(root)) } }); let gate_requested = require_coverage.is_some() || !matches!(enforcement, types::EnforcementMode::Warn); - let (config, all_spec_files) = load_and_discover(root, gate_requested); + // JSON mode must ALSO bypass the no-spec early-exit: that path prints plain + // human text ("No spec files found…"), breaking CI parsers exactly in the + // bootstrap case (#441). The JSON below already handles zero specs. + let (config, all_spec_files) = load_and_discover(root, gate_requested || json); let spec_files = filter_specs(root, &all_spec_files, spec_filters); let spec_files = filter_by_status(&spec_files, exclude_status, only_status); let scores: Vec = spec_files @@ -107,6 +111,31 @@ pub fn cmd_score( } } + // --strict quality gate (#441): F-grade specs — or nothing scored at all, + // where the bar is unenforceable — must fail the process. JSON/CSV gate + // silently via the exit code to keep stdout machine-parseable. + if strict { + let f_grades = project + .spec_scores + .iter() + .filter(|s| s.grade == "F") + .count(); + if project.total_specs == 0 || f_grades > 0 { + if !json && !matches!(format, types::OutputFormat::Csv) { + eprintln!( + "{}: {} — failing the quality gate", + "--strict mode".red(), + if project.total_specs == 0 { + "no specs were scored".to_string() + } else { + format!("{f_grades} spec(s) scored F") + } + ); + } + std::process::exit(1); + } + } + // Honor the global gate flags. `score` is advisory by default (Warn config → // exit 0), but `--require-coverage`, `--enforcement enforce-new`, and a strict // config now gate the exit code — previously they were silent no-ops here. diff --git a/src/comment.rs b/src/comment.rs index a6146656..c7e095d2 100644 --- a/src/comment.rs +++ b/src/comment.rs @@ -447,6 +447,7 @@ mod tests { specced_loc: 1000, loc_coverage_percent: 100, unspecced_file_loc: vec![], + missing_files: vec![], }; let output = render_check_comment( 5, @@ -479,6 +480,7 @@ mod tests { specced_loc: 800, loc_coverage_percent: 80, unspecced_file_loc: vec![], + missing_files: vec![], }; let errors = vec![ "specs/auth.spec.md: Missing required section: Purpose".to_string(), @@ -523,6 +525,7 @@ mod tests { specced_loc: 0, loc_coverage_percent: 100, unspecced_file_loc: vec![], + missing_files: vec![], }; let output = render_check_comment(0, 0, 0, 0, &[], &[], &[], &coverage, true, None, None); assert!(output.contains("Generated by [specsync]")); @@ -542,6 +545,7 @@ mod tests { specced_loc: 0, loc_coverage_percent: 0, unspecced_file_loc: vec![], + missing_files: vec![], }; let output = render_check_comment(0, 0, 0, 0, &[], &[], &[], &coverage, true, None, None); assert!(output.contains("...and 5 more")); @@ -559,6 +563,7 @@ mod tests { specced_loc: 1, loc_coverage_percent: 100, unspecced_file_loc: vec![], + missing_files: vec![], }; let errors = vec![format!( "specs/large.spec.md: {}", @@ -592,6 +597,7 @@ mod tests { specced_loc: 500, loc_coverage_percent: 100, unspecced_file_loc: vec![], + missing_files: vec![], }; let body = render_comment_body(&violations, &coverage, Some("owner/repo"), Some("main")); assert!(body.contains("SpecSync: Failed")); @@ -616,6 +622,7 @@ mod tests { specced_loc: 500, loc_coverage_percent: 100, unspecced_file_loc: vec![], + missing_files: vec![], }; let body = render_comment_body(&violations, &coverage, Some("owner/repo"), Some("main")); assert!(body.contains("SpecSync: Passed")); diff --git a/src/config.rs b/src/config.rs index f5718005..5d772d8b 100644 --- a/src/config.rs +++ b/src/config.rs @@ -696,8 +696,14 @@ fn load_json_config(config_path: &Path, root: &Path) -> SpecSyncConfig { match serde_json::from_str::(&content) { Ok(config) => { + let mut config = config; + // An explicit `enforcement` key (even `"warn"`) opts into that mode; + // an unset key gates on errors (see commands::default_enforcement). + config.enforcement_set = serde_json::from_str::(&content) + .ok() + .and_then(|v| v.as_object().map(|o| o.contains_key("enforcement"))) + .unwrap_or(false); if !content.contains("\"sourceDirs\"") { - let mut config = config; config.source_dirs = detect_source_dirs(root); return config; } @@ -892,6 +898,10 @@ fn load_toml_config(config_path: &Path, root: &Path) -> SpecSyncConfig { } "enforcement" => { let s = parse_toml_string(value); + // Mark enforcement as explicitly configured: gate commands + // honor an explicit `warn` (exit 0), while an UNSET + // enforcement gates on errors (see default_enforcement). + config.enforcement_set = true; match s.as_str() { "strict" => { config.enforcement = crate::types::EnforcementMode::Strict; diff --git a/src/main.rs b/src/main.rs index 495b7c25..2e329d72 100644 --- a/src/main.rs +++ b/src/main.rs @@ -172,8 +172,17 @@ fn run() { remote, verify, cache_ttl, - } => commands::resolve::cmd_resolve(&root, remote || verify, verify, cache_ttl), - Command::Diff { base } => commands::diff::cmd_diff(&root, &base, format), + } => commands::resolve::cmd_resolve( + &root, + remote || verify, + verify, + cache_ttl, + format, + cli.strict, + ), + Command::Diff { base } => { + commands::diff::cmd_diff(&root, base.as_deref(), format, cli.strict) + } Command::Hooks { action } => commands::hooks::cmd_hooks(&root, action), Command::Agents { action } => commands::agents::cmd_agents(&root, action), Command::Compact { keep, dry_run } => commands::compact::cmd_compact(&root, keep, dry_run), @@ -185,9 +194,15 @@ fn run() { Command::Issues { create } => commands::issues::cmd_issues(&root, format, create), Command::New { name, full } => commands::new::cmd_new(&root, &name, full), Command::Wizard => commands::wizard::cmd_wizard(&root), - Command::Deps { mermaid, dot } => { - commands::deps::cmd_deps(&root, cli.strict, format, mermaid, dot) - } + Command::Deps { mermaid, dot } => commands::deps::cmd_deps( + &root, + cli.strict, + cli.enforcement, + cli.require_coverage, + format, + mermaid, + dot, + ), Command::Import { source, id, @@ -217,6 +232,9 @@ fn run() { stale_threshold, &cli.exclude_status, &cli.only_status, + cli.strict, + cli.enforcement, + cli.require_coverage, ), Command::Comment { pr, base } => commands::comment::cmd_comment( &root, @@ -314,6 +332,7 @@ mod tests { specced_loc: 0, loc_coverage_percent: 100, unspecced_file_loc: vec![], + missing_files: vec![], } } @@ -341,6 +360,7 @@ mod tests { specced_loc: 0, loc_coverage_percent: 0, unspecced_file_loc: vec![], + missing_files: vec![], } } diff --git a/src/output.rs b/src/output.rs index 2021f4d9..1b80668a 100644 --- a/src/output.rs +++ b/src/output.rs @@ -70,9 +70,21 @@ pub fn print_coverage_report(coverage: &types::CoverageReport) { } } - if coverage.unspecced_files.is_empty() { + // Files a spec references but that do not exist on disk must never sit + // under a green "all referenced" line — name them as failures instead. + if !coverage.missing_files.is_empty() { + println!( + "\n Referenced by specs but missing on disk ({}):", + coverage.missing_files.len() + ); + for file in &coverage.missing_files { + println!(" {} {file}", "✗".red()); + } + } + + if coverage.unspecced_files.is_empty() && coverage.missing_files.is_empty() { println!(" {} All source files referenced by specs", "✓".green()); - } else { + } else if !coverage.unspecced_files.is_empty() { let uncovered_loc: usize = coverage.unspecced_file_loc.iter().map(|(_, l)| l).sum(); println!( "\n Files not in any spec ({}, {} LOC uncovered):", @@ -244,6 +256,7 @@ mod tests { specced_loc: 1, loc_coverage_percent: loc_pct, unspecced_file_loc: Vec::new(), + missing_files: Vec::new(), } } diff --git a/src/parser.rs b/src/parser.rs index 8f15981d..9be953b3 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -113,10 +113,38 @@ fn set_scalar(fm: &mut Frontmatter, key: &str, value: &str) { // Handle inline bracket arrays like `implements: [42, 57]` "implements" => fm.implements = parse_inline_issue_numbers(value), "tracks" => fm.tracks = parse_inline_issue_numbers(value), + // A scalar `depends_on: alpha` (or inline `depends_on: [a, b]`) used to + // be silently DROPPED — the dependency edge vanished from validation + // and graphing with no diagnostic. Normalize it into the list so the + // edge is enforced, and warn so the typo is visible. + "depends_on" => { + fm.depends_on = parse_inline_string_list(value); + if !value.trim_start().starts_with('[') { + eprintln!( + "warning: scalar `depends_on: {value}` should be a YAML list (`depends_on: [{value}]`); treating it as a single dependency" + ); + } + } _ => {} } } +/// Parse an inline bracket array of strings (`[a, b]` → vec!["a", "b"]) or a +/// bare scalar (`a` → vec!["a"]). Quotes are stripped from each element. +fn parse_inline_string_list(value: &str) -> Vec { + let s = value.trim(); + let inner = if s.starts_with('[') && s.ends_with(']') { + &s[1..s.len() - 1] + } else { + s + }; + inner + .split(',') + .map(|v| v.trim().trim_matches('"').trim_matches('\'').to_string()) + .filter(|v| !v.is_empty()) + .collect() +} + /// Parse an inline bracket array of issue numbers: `[42, 57]` → vec![42, 57]. fn parse_inline_issue_numbers(value: &str) -> Vec { let s = value.trim(); @@ -143,7 +171,16 @@ fn set_field(fm: &mut Frontmatter, key: &str, values: &[String]) { match key { "files" => fm.files = values.to_vec(), "db_tables" => fm.db_tables = values.to_vec(), - "depends_on" => fm.depends_on = values.to_vec(), + // Dedupe at parse time (order-preserving): duplicate `depends_on` + // entries used to inflate edge counts and emit doubled mermaid edges. + "depends_on" => { + let mut seen = std::collections::HashSet::new(); + fm.depends_on = values + .iter() + .filter(|v| seen.insert((*v).clone())) + .cloned() + .collect(); + } "implements" => fm.implements = parse_issue_numbers(values), "tracks" => fm.tracks = parse_issue_numbers(values), "lifecycle_log" => fm.lifecycle_log = values.to_vec(), @@ -378,15 +415,46 @@ const STUB_PHRASES: &[&str] = &[ "\u{2026}", // ellipsis character ]; +/// Stock sentences emitted verbatim by `specsync new` / `generate` scaffolds. +/// They read like prose but are pure template — a spec consisting of them is +/// an untouched scaffold, not documentation (#441). +const SCAFFOLD_BOILERPLATE_PREFIXES: &[&str] = &[ + "document this module's responsibility", + "list runtime dependencies and the specific symbols", + "define an invariant that must remain true", + "**given** precondition", + "**when** action", + "**then** result", +]; + +/// Strip list markers (`- `, `* `, `> `, `1. `) from the start of a line. +fn strip_list_marker(mut s: &str) -> &str { + s = s.trim(); + for marker in ["- ", "* ", "> "] { + if let Some(rest) = s.strip_prefix(marker) { + s = rest.trim_start(); + } + } + let digits = s.len() - s.trim_start_matches(|c: char| c.is_ascii_digit()).len(); + if digits > 0 && s[digits..].starts_with(". ") { + s = s[digits + 2..].trim_start(); + } + s +} + +/// Check if a line is verbatim scaffold boilerplate (case-insensitive). +pub fn is_boilerplate_line(line: &str) -> bool { + let lower = strip_list_marker(line).to_ascii_lowercase(); + SCAFFOLD_BOILERPLATE_PREFIXES + .iter() + .any(|p| lower.starts_with(p)) +} + /// Check if a line is a stub/placeholder (case-insensitive). fn is_stub_line(line: &str) -> bool { - let t = line - .trim() - .trim_start_matches("- ") - .trim_start_matches("* ") - .trim_start_matches("> "); + let t = strip_list_marker(line); let lower = t.to_ascii_lowercase(); - STUB_PHRASES.contains(&lower.as_str()) + STUB_PHRASES.contains(&lower.as_str()) || is_boilerplate_line(line) } /// Find the byte offset of an exact `## Section` heading line. @@ -491,6 +559,45 @@ mod tests { assert!(parsed.frontmatter.db_tables.is_empty()); } + #[test] + fn test_scalar_depends_on_normalized_to_list() { + // Regression (#419): a scalar `depends_on: alpha` was silently dropped, + // disabling the dependency edge with no diagnostic. + let content = "---\nmodule: beta\nversion: 1\nstatus: active\nfiles: []\ndepends_on: alpha\n---\n\n# Beta\n"; + let parsed = parse_frontmatter(content).unwrap(); + assert_eq!(parsed.frontmatter.depends_on, vec!["alpha"]); + } + + #[test] + fn test_inline_bracket_depends_on_parsed() { + let content = "---\nmodule: beta\ndepends_on: [alpha, gamma]\n---\n\n# Beta\n"; + let parsed = parse_frontmatter(content).unwrap(); + assert_eq!(parsed.frontmatter.depends_on, vec!["alpha", "gamma"]); + } + + #[test] + fn test_duplicate_depends_on_deduped() { + // Regression (#419): duplicate entries inflated edge counts and doubled + // mermaid edges. + let content = "---\nmodule: beta\ndepends_on:\n - alpha\n - alpha\n - gamma\n - alpha\n---\n\n# Beta\n"; + let parsed = parse_frontmatter(content).unwrap(); + assert_eq!(parsed.frontmatter.depends_on, vec!["alpha", "gamma"]); + } + + #[test] + fn test_scaffold_boilerplate_detected() { + // Regression (#441): an untouched `specsync new` scaffold must not + // count as placeholder-free, meaningful content. + assert!(is_boilerplate_line( + "Document this module's responsibility, inputs, outputs, and ownership boundaries." + )); + assert!(is_boilerplate_line("- **Given** precondition")); + assert!(is_boilerplate_line( + "1. Define an invariant that must remain true for supported inputs." + )); + assert!(!is_boilerplate_line("Handles authentication tokens.")); + } + #[test] fn test_strip_yaml_comment() { assert_eq!(strip_yaml_comment("active"), "active"); diff --git a/src/registry.rs b/src/registry.rs index 3fed0ab2..f23bd4f7 100644 --- a/src/registry.rs +++ b/src/registry.rs @@ -63,28 +63,31 @@ pub struct RemoteSpec { pub body: String, } -/// Fetch a spec file's raw content from a GitHub repo. -/// -/// `repo` is `owner/repo`, `spec_path` is the relative path from the registry. -pub fn fetch_remote_spec(repo: &str, spec_path: &str) -> Result { - let url = format!("https://raw.githubusercontent.com/{repo}/HEAD/{spec_path}"); - +/// GET a raw file from GitHub, attaching `GITHUB_TOKEN` (or `GH_TOKEN`) as an +/// Authorization header when set — documented private-repo support, previously +/// never actually sent (#422). +fn github_raw_get(url: &str) -> Result { let agent = ureq::Agent::new_with_config( ureq::config::Config::builder() .timeout_global(Some(Duration::from_secs(10))) .build(), ); - let mut response = agent - .get(&url) + let mut request = agent.get(url); + if let Ok(token) = std::env::var("GITHUB_TOKEN") + .or_else(|_| std::env::var("GH_TOKEN")) + .map(|t| t.trim().to_string()) + && !token.is_empty() + { + request = request.header("Authorization", &format!("Bearer {token}")); + } + + let mut response = request .call() .map_err(|e| format!("HTTP request failed: {e}"))?; if response.status() != 200 { - return Err(format!( - "HTTP {} — could not fetch {spec_path} from {repo}", - response.status() - )); + return Err(format!("HTTP {}", response.status())); } response @@ -93,6 +96,15 @@ pub fn fetch_remote_spec(repo: &str, spec_path: &str) -> Result .map_err(|e| format!("Failed to read response body: {e}")) } +/// Fetch a spec file's raw content from a GitHub repo. +/// +/// `repo` is `owner/repo`, `spec_path` is the relative path from the registry. +pub fn fetch_remote_spec(repo: &str, spec_path: &str) -> Result { + let url = format!("https://raw.githubusercontent.com/{repo}/HEAD/{spec_path}"); + github_raw_get(&url) + .map_err(|e| format!("{e} — could not fetch {spec_path} from {repo}")) +} + /// Parse a fetched spec into its relevant metadata for verification. pub fn parse_remote_spec(module: &str, content: &str) -> Option { use crate::parser; @@ -112,35 +124,26 @@ pub fn parse_remote_spec(module: &str, content: &str) -> Option { }) } -/// Fetch `specsync-registry.toml` from a GitHub repo's default branch. +/// Fetch the registry from a GitHub repo's default branch. /// /// `repo` is in `owner/repo` format (e.g. `corvid-labs/algochat`). -/// Tries the GitHub raw content URL for the file at repo root. +/// Fetches the canonical v4 location `.specsync/registry.toml` first (what +/// `init-registry` publishes and the docs promise), falling back to the legacy +/// root `specsync-registry.toml` for un-migrated repos (#422 — previously only +/// the legacy path was fetched, so every tool-published registry 404'd). pub fn fetch_remote_registry(repo: &str) -> Result { - let url = format!("https://raw.githubusercontent.com/{repo}/HEAD/{REGISTRY_FILENAME}"); - - let agent = ureq::Agent::new_with_config( - ureq::config::Config::builder() - .timeout_global(Some(Duration::from_secs(10))) - .build(), - ); - - let mut response = agent - .get(&url) - .call() - .map_err(|e| format!("HTTP request failed: {e}"))?; - - if response.status() != 200 { - return Err(format!( - "HTTP {} — {repo} may not have a {REGISTRY_FILENAME}", - response.status() - )); - } - - let body = response - .body_mut() - .read_to_string() - .map_err(|e| format!("Failed to read response body: {e}"))?; + let v4_url = format!("https://raw.githubusercontent.com/{repo}/HEAD/{V4_REGISTRY_RELATIVE}"); + let legacy_url = format!("https://raw.githubusercontent.com/{repo}/HEAD/{REGISTRY_FILENAME}"); + + let body = match github_raw_get(&v4_url) { + Ok(body) => body, + Err(v4_err) => github_raw_get(&legacy_url).map_err(|legacy_err| { + format!( + "{v4_err} — {repo} may not have a {V4_REGISTRY_RELATIVE} \ + (legacy {REGISTRY_FILENAME} also failed: {legacy_err})" + ) + })?, + }; let entry = parse_registry(&body).ok_or_else(|| format!("Failed to parse registry from {repo}"))?; diff --git a/src/scoring.rs b/src/scoring.rs index 573f8ce9..d499e41a 100644 --- a/src/scoring.rs +++ b/src/scoring.rs @@ -120,27 +120,40 @@ pub fn score_spec(spec_path: &Path, root: &Path, config: &SpecSyncConfig) -> Spe let body = &parsed.body; // ─── Frontmatter (0-20) ────────────────────────────────────────── + // Presence alone is not enough (#441): `version: "notanumber"` and + // `status: bogus` used to score full marks. Validate the VALUES. + let version_valid = fm.version.as_deref().is_some_and(|v| { + let v = v.trim().trim_matches('"').trim_matches('\''); + !v.is_empty() + && v.split('.') + .all(|part| !part.is_empty() && part.chars().all(|c| c.is_ascii_digit())) + }); + let status_valid = fm.parsed_status().is_some(); let mut fm_points = 0u32; - let mut fm_missing: Vec<&str> = Vec::new(); + let mut fm_missing: Vec = Vec::new(); if fm.module.is_some() { fm_points += FM_MODULE_POINTS; } else { - fm_missing.push("module (-5pts)"); + fm_missing.push("module (-5pts)".to_string()); } - if fm.version.is_some() { + if version_valid { fm_points += FM_VERSION_POINTS; + } else if fm.version.is_some() { + fm_missing.push("valid version (-5pts)".to_string()); } else { - fm_missing.push("version (-5pts)"); + fm_missing.push("version (-5pts)".to_string()); } - if fm.status.is_some() { + if status_valid { fm_points += FM_STATUS_POINTS; + } else if fm.status.is_some() { + fm_missing.push("valid status (-4pts)".to_string()); } else { - fm_missing.push("status (-4pts)"); + fm_missing.push("status (-4pts)".to_string()); } if !fm.files.is_empty() { fm_points += FM_FILES_POINTS; } else { - fm_missing.push("files (-6pts)"); + fm_missing.push("files (-6pts)".to_string()); } score.frontmatter_score = fm_points; if !fm_missing.is_empty() { @@ -172,30 +185,26 @@ pub fn score_spec(spec_path: &Path, root: &Path, config: &SpecSyncConfig) -> Spe }, CriterionResult { name: "has_version".to_string(), - passed: fm.version.is_some(), - points: if fm.version.is_some() { - FM_VERSION_POINTS - } else { - 0 - }, + passed: version_valid, + points: if version_valid { FM_VERSION_POINTS } else { 0 }, max_points: FM_VERSION_POINTS, detail: if fm.version.is_none() { Some("add `version:` field".to_string()) + } else if !version_valid { + Some("`version` must be numeric (e.g. `1` or `1.2`)".to_string()) } else { None }, }, CriterionResult { name: "has_status".to_string(), - passed: fm.status.is_some(), - points: if fm.status.is_some() { - FM_STATUS_POINTS - } else { - 0 - }, + passed: status_valid, + points: if status_valid { FM_STATUS_POINTS } else { 0 }, max_points: FM_STATUS_POINTS, detail: if fm.status.is_none() { Some("add `status:` field".to_string()) + } else if !status_valid { + Some("`status` is not a recognized lifecycle status".to_string()) } else { None }, @@ -220,47 +229,34 @@ pub fn score_spec(spec_path: &Path, root: &Path, config: &SpecSyncConfig) -> Spe // ─── Sections (0-20) ───────────────────────────────────────────── let missing = get_missing_sections(body, &config.required_sections); - let present = config.required_sections.len() - missing.len(); let total_sections = config.required_sections.len(); - score.sections_score = if total_sections == 0 { - DIMENSION_MAX - } else { - ((present as f64 / total_sections as f64) * DIMENSION_MAX as f64).round() as u32 - }; - if !missing.is_empty() { - let lost = DIMENSION_MAX - score.sections_score; - let names = missing - .iter() - .take(3) - .cloned() - .collect::>() - .join(", "); - let suffix = if missing.len() > 3 { - format!(" (+{} more)", missing.len() - 3) - } else { - String::new() - }; - score - .suggestions - .push(format!("Sections (-{lost}pts): missing ## {names}{suffix}")); - } { let missing_set: HashSet<&str> = missing.iter().map(|s| s.as_str()).collect(); - let per_section_max = if total_sections > 0 { - ((DIMENSION_MAX as f64 / total_sections as f64).round() as u32).max(1) + // Distribute DIMENSION_MAX points across the sections EXACTLY (first + // `remainder` sections get one extra point). The previous round() gave + // every section ceil(20/n) points, so the criteria summed to e.g. 21 + // while the dimension reported /20 — the --explain output contradicted + // itself (#441). The dimension score is now literally the criteria sum. + let (base, remainder) = if total_sections > 0 { + ( + DIMENSION_MAX / total_sections as u32, + DIMENSION_MAX % total_sections as u32, + ) } else { - 0 + (0, 0) }; let section_criteria: Vec = config .required_sections .iter() - .map(|sec| { + .enumerate() + .map(|(i, sec)| { let present = !missing_set.contains(sec.as_str()); + let max = base + u32::from((i as u32) < remainder); CriterionResult { name: sec.clone(), passed: present, - points: if present { per_section_max } else { 0 }, - max_points: per_section_max, + points: if present { max } else { 0 }, + max_points: max, detail: if !present { Some(format!("add ## {sec} section")) } else { @@ -269,6 +265,11 @@ pub fn score_spec(spec_path: &Path, root: &Path, config: &SpecSyncConfig) -> Spe } }) .collect(); + score.sections_score = if total_sections == 0 { + DIMENSION_MAX + } else { + section_criteria.iter().map(|c| c.points).sum() + }; score.explain.push(ExplainDetail { dimension: "Sections".to_string(), score: score.sections_score, @@ -276,6 +277,23 @@ pub fn score_spec(spec_path: &Path, root: &Path, config: &SpecSyncConfig) -> Spe criteria: section_criteria, }); } + if !missing.is_empty() { + let lost = DIMENSION_MAX - score.sections_score; + let names = missing + .iter() + .take(3) + .cloned() + .collect::>() + .join(", "); + let suffix = if missing.len() > 3 { + format!(" (+{} more)", missing.len() - 3) + } else { + String::new() + }; + score + .suggestions + .push(format!("Sections (-{lost}pts): missing ## {names}{suffix}")); + } // ─── API Coverage (0-20) ───────────────────────────────────────── if !fm.files.is_empty() { @@ -410,7 +428,11 @@ pub fn score_spec(spec_path: &Path, root: &Path, config: &SpecSyncConfig) -> Spe // ─── Content Depth (0-20) ──────────────────────────────────────── let mut depth_points = 0u32; let todo_count = count_placeholder_todos(body); - let placeholder_count = count_placeholder_comments(body); + // Untouched `specsync new`/`generate` scaffolds contain no TODOs or + // `` markers, yet are pure boilerplate — count the scaffold's + // stock sentences as placeholders too, or an empty scaffold passes the + // "placeholder_free" check and clears the ≥80 bar (#441). + let placeholder_count = count_placeholder_comments(body) + count_boilerplate_lines(body); // Check each required section has meaningful content (stubs don't count) let sections_with_content = count_sections_with_content(body, &config.required_sections); @@ -536,7 +558,16 @@ pub fn score_spec(spec_path: &Path, root: &Path, config: &SpecSyncConfig) -> Spe stale_files += 1; } } - let file_penalty = if stale_files > 0 { + let file_penalty = if fm.files.is_empty() { + // `files_exist` was vacuously 15/15 when `files:` was missing entirely + // — nothing to check passed as everything checked out (#441). An empty + // files list makes freshness unverifiable, not perfect. + fresh_points = fresh_points.saturating_sub(FRESH_FILES_MAX); + score.suggestions.push(format!( + "Freshness (-{FRESH_FILES_MAX}pts): no files listed in frontmatter — freshness is unverifiable" + )); + FRESH_FILES_MAX + } else if stale_files > 0 { let penalty = (stale_files * FRESH_FILE_PENALTY_PER).min(FRESH_FILES_MAX); fresh_points = fresh_points.saturating_sub(penalty); score.suggestions.push(format!( @@ -600,6 +631,19 @@ pub fn score_spec(spec_path: &Path, root: &Path, config: &SpecSyncConfig) -> Spe } score.freshness_score = fresh_points; + // Budget the dimension so the --explain criteria sum EXACTLY to the + // reported score (previously criteria summed to 15+5+variable ≠ 20 — the + // raw max was silently rescaled, contradicting the dimension line, #441). + // fresh_points = 20 - file_penalty - dep_penalty - git_penalty, so: + // files_exist: max 15-dep_budget, points max-file_penalty + // deps_exist: max dep_budget, points budget-dep_penalty + // git: max 5, points 5-git_penalty + let dep_budget = if fm.depends_on.is_empty() { + 0 + } else { + dep_penalty.max(FRESH_DEP_PENALTY_PER) + }; + let files_budget = FRESH_FILES_MAX - dep_budget; score.explain.push(ExplainDetail { dimension: "Freshness".to_string(), score: fresh_points, @@ -607,10 +651,12 @@ pub fn score_spec(spec_path: &Path, root: &Path, config: &SpecSyncConfig) -> Spe criteria: vec![ CriterionResult { name: "files_exist".to_string(), - passed: stale_files == 0, - points: FRESH_FILES_MAX.saturating_sub(file_penalty), - max_points: FRESH_FILES_MAX, - detail: if stale_files > 0 { + passed: !fm.files.is_empty() && stale_files == 0, + points: files_budget.saturating_sub(file_penalty), + max_points: files_budget, + detail: if fm.files.is_empty() { + Some("no files listed in frontmatter".to_string()) + } else if stale_files > 0 { Some(format!("{stale_files} file(s) missing")) } else { None @@ -619,15 +665,8 @@ pub fn score_spec(spec_path: &Path, root: &Path, config: &SpecSyncConfig) -> Spe CriterionResult { name: "deps_exist".to_string(), passed: stale_deps == 0, - points: (stale_deps * FRESH_DEP_PENALTY_PER) - .saturating_sub(dep_penalty) - .min(if fm.depends_on.is_empty() { - 0 - } else { - stale_deps * FRESH_DEP_PENALTY_PER - }), - max_points: (fm.depends_on.len() as u32 * FRESH_DEP_PENALTY_PER) - .min(FRESH_DEP_PENALTY_PER * 2), + points: dep_budget.saturating_sub(dep_penalty), + max_points: dep_budget, detail: if stale_deps > 0 { Some(format!("{stale_deps} depends_on path(s) missing")) } else { @@ -724,6 +763,15 @@ fn count_placeholder_comments(body: &str) -> usize { stripped.matches("