feat: v3.3 — lifecycle, custom rules, views, compaction & archival#94
feat: v3.3 — lifecycle, custom rules, views, compaction & archival#94corvid-agent wants to merge 8 commits intomainfrom
Conversation
Add SHA-256 content hashing to skip re-validation of unchanged specs. Hashes are stored in .specsync/hashes.json (gitignored local state). - Hash spec files, companion files, and source files listed in frontmatter - Skip validation when no backing files have changed - --force flag to bypass cache and re-validate everything - --strict always re-validates (warnings become errors, different semantics) - Cache only saved when validation has no errors Closes #90 (Phase 1) Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
…regen (Phase 2) Adds ChangeKind enum and classify_changes() to detect what specifically changed (spec, requirements, companion, source) instead of just whether anything changed. Reports staleness warnings in check output, supports interactive re-validation prompting, auto-regenerates specs via AI when --fix is used with requirements drift, and surfaces stale entries in JSON/MCP output. Fixes companion file detection to check both plain names (requirements.md) and legacy prefixed names. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Cache keys now always use forward slashes regardless of OS, fixing Windows test failures where backslash paths didn't match cache entries. Also applies cargo fmt formatting fixes. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Tests failed on machines where Cursor is installed because the error message starts with "Cursor" (capital C) instead of "cursor" (lowercase). Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Add status lifecycle (draft/active/stable/deprecated), custom validation rules in .specsync.toml, agent resource policy, role-based views, changelog compaction, task archival, and watch mode hash cache integration. New commands: specsync view, specsync compact, specsync archive-tasks New modules: view.rs, compact.rs, archive.rs 221 tests passing (141 unit + 80 integration). Co-Authored-By: Claude Opus 4.6 <[email protected]>
corvid-agent
left a comment
There was a problem hiding this comment.
PR Review: v3.3 — Lifecycle, Custom Rules, Views, Compaction & Archival
Reviewer: CorvidAgent (Lead)
Scope: Full diff review (+2181/-29, 16 files)
Summary
Big feature release adding 4 new modules (archive.rs, compact.rs, hash_cache.rs, view.rs) plus significant enhancements to validation, config, and watch mode. The change classification system and hash caching are well-designed. Test coverage is solid across all new modules.
Architecture: ✅ Good
- Hash cache + change classification is a clean design. The
ChangeKindenum andChangeClassificationstruct provide good type safety. Caching only on zero errors is the right call. - Role-based views are well-scoped — section filtering per role is a clean approach.
- Lifecycle statuses with draft-aware validation relaxation (skipping Public API checks) makes sense.
- Companion file discovery handles both current and legacy naming conventions.
Issues Found
1. 🔴 Inline SHA-256 implementation (~120 lines in hash_cache.rs)
Rolling your own SHA-256 is strongly discouraged even when correctness is verified by test vectors. The sha2 crate is mature, audited, and already commonly used in the Rust ecosystem. This adds maintenance burden and subtle risk for no benefit.
Recommendation: Replace with sha2 crate. It's one dependency and eliminates ~120 lines of crypto code.
2. 🟡 Duplicated code in auto_regen_stale_specs (main.rs)
The legacy-path branch and the standard-path branch are nearly identical (~20 lines each), differing only in which req_path is passed. This should be deduplicated:
let req_path = if parent.join("requirements.md").exists() {
parent.join("requirements.md")
} else {
let legacy = parent.join(format!("{module}.req.md"));
if legacy.exists() { legacy } else { continue; }
};
// ... single call to regenerate_spec_with_ai3. 🟡 Fragile changelog header detection (compact.rs)
The header detection checks for strings "Date", "Change", "Author" in table rows:
if trimmed.contains("Date") || trimmed.contains("Change") || trimmed.contains("Author")A data row like | 2026-03-01 | Author service refactored | would be misidentified as a header. Consider checking if the row is the first table row (position-based) rather than content-based matching.
4. 🟡 require_depends_on rule checks body instead of frontmatter
In apply_custom_rules, the require_depends_on rule searches for ### Consumes in the body with a regex, despite the comment acknowledging it should check fm.depends_on. The frontmatter is parsed earlier — pass it into apply_custom_rules to check fm.depends_on.is_empty() directly.
5. 🟢 Minor: strip_frontmatter hardcoded offsets
In view.rs, strip_frontmatter uses &content[4..] and &content[end + 8..]. These magic numbers assume ---\n is exactly 4 bytes. Consider using "---\n".len() for clarity.
6. 🟢 Minor: COMPANION_OTHER_LEGACY_SUFFIXES naming
The constant COMPANION_OTHER_LEGACY_SUFFIXES contains ["context.md", "tasks.md"] — same values as COMPANION_OTHER_NAMES. If legacy naming uses the same filenames, the dedup check in find_companion_files will always skip them. Verify this is intentional (legacy format is {module}.context.md vs context.md).
Testing: ✅ Solid
- Unit tests for all 4 new modules with good edge case coverage
- Integration tests for hash cache lifecycle (create, skip, force, source change)
- SHA-256 test vectors verify correctness of the inline implementation
- Cursor provider tests updated for case-insensitive matching
Config & Compatibility: ✅
- TOML section parsing (
[rules]) is a nice addition - JSON config extends cleanly with
rulesandtaskArchiveDays .specsync/correctly added to.gitignoreCargo.tomlexcludes test/spec dirs from package — good practice
Verdict
No blocking issues. The inline SHA-256 (#1) is the biggest concern but isn't a security risk since it's used for cache invalidation only. The duplicated regen code (#2) and fragile header detection (#3) are worth addressing but not blocking.
Recommend merging after addressing #2 and #3 at minimum. #1 can be a fast-follow.
🤖 Agent: CorvidAgent | Model: Opus 4.6
There was a problem hiding this comment.
Pull request overview
This PR delivers SpecSync v3.3 features focused on spec lifecycle metadata, configurable validation rules, role-based spec views, changelog/task maintenance utilities, and hash-based caching to speed up repeated check runs.
Changes:
- Add SHA-256 content-hash caching and staleness classification to skip unchanged spec validations (plus
--forcesupport and watch-mode integration). - Introduce lifecycle/status +
agent_policyfrontmatter support, and custom validation rules via config ([rules]). - Add new CLI commands:
view(role-filtered output),compact(changelog compaction), andarchive-tasks(move completed tasks into an archive section).
Reviewed changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration.rs | Expands integration coverage for hash caching and provider error message casing. |
| src/watch.rs | Enhances watch mode to force full initial validation, then incremental checks with caching. |
| src/view.rs | Implements role-based filtering for spec markdown output. |
| src/validator.rs | Adds lifecycle validation behaviors and applies configurable custom validation rules. |
| src/types.rs | Introduces typed spec status + config structures for rules and task archival settings. |
| src/parser.rs | Extends frontmatter parsing to include agent_policy. |
| src/mcp.rs | Adds staleness classification output/behavior to MCP specsync_check. |
| src/main.rs | Adds new subcommands (view, compact, archive-tasks), --force, hash-cached validation, and AI regen for stale specs. |
| src/hash_cache.rs | New hashing + change classification module backing the cache and staleness detection. |
| src/config.rs | Adds parsing support for new config keys and TOML [rules] section. |
| src/compact.rs | Implements changelog compaction logic + tests. |
| src/archive.rs | Implements tasks archival logic + tests. |
| src/ai.rs | Adds AI-based “regen from requirements drift” prompt + regeneration entrypoint. |
| Cargo.toml | Bumps version to 3.3.0, sets rust-version, updates metadata/excludes. |
| Cargo.lock | Updates locked package version. |
| .gitignore | Ignores .specsync/ cache directory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/hash_cache.rs
Outdated
| let dir = root.join(CACHE_DIR); | ||
| fs::create_dir_all(&dir)?; | ||
| let path = dir.join(CACHE_FILE); | ||
| let json = serde_json::to_string_pretty(self)?; |
There was a problem hiding this comment.
HashCache::save returns io::Result<()>, but serde_json::to_string_pretty(self)? uses ? on a serde_json::Error which doesn’t convert into io::Error. This won’t compile. Map the serde_json error into an io::Error (or change the return type to a broader error type).
| let json = serde_json::to_string_pretty(self)?; | |
| let json = serde_json::to_string_pretty(self) | |
| .map_err(|err| io::Error::new(io::ErrorKind::InvalidData, err))?; |
src/ai.rs
Outdated
| let truncated = if content.len() > 30_000 { | ||
| &content[..30_000] | ||
| } else { | ||
| content.as_str() | ||
| }; | ||
| prompt.push_str(&format!("### `{path}`\n\n```\n{truncated}\n```\n\n")); |
There was a problem hiding this comment.
This truncation uses &content[..30_000], which can panic at runtime if the string contains non-UTF8-boundary bytes (non-ASCII / multibyte UTF-8). Truncate by character boundary (or use a safe byte/char truncation helper) to avoid panics when source files contain Unicode.
| // Interactive prompting: if TTY and requirements drift detected, offer re-validation | ||
| if !requirements_stale_specs.is_empty() | ||
| && matches!(format, Text) | ||
| && !fix | ||
| && std::io::stdin().is_terminal() | ||
| { | ||
| eprint!( | ||
| "{} Re-validate spec(s) against new requirements? [y/N] ", | ||
| "?".cyan() | ||
| ); | ||
| let _ = std::io::stderr().flush(); | ||
| let mut answer = String::new(); | ||
| let _ = std::io::stdin().read_line(&mut answer); | ||
| if !answer.trim().eq_ignore_ascii_case("y") { | ||
| // User declined — just continue with normal validation | ||
| println!(" Skipping re-validation. Use --fix to auto-regenerate.\n"); | ||
| } | ||
| } |
There was a problem hiding this comment.
The prompt asks whether to re-validate against new requirements, but answering "y" doesn’t change behavior (the code only prints a message when the user declines). Either remove the prompt or implement the "yes" path (e.g., trigger regeneration/validation behavior, or instruct the user how to proceed).
src/watch.rs
Outdated
| println!( | ||
| "{} Hash cache active — only changed specs will be re-validated", | ||
| ">>>".cyan() | ||
| ); |
There was a problem hiding this comment.
This message claims the hash cache will skip unchanged specs, but cmd_check currently bypasses the cache when --strict is enabled (it validates all specs). In watch mode with strict=true, this output becomes misleading; consider conditioning the message on !strict or adjusting caching behavior in strict mode.
| println!( | |
| "{} Hash cache active — only changed specs will be re-validated", | |
| ">>>".cyan() | |
| ); | |
| if strict { | |
| println!( | |
| "{} Strict mode active — all specs will be re-validated on each run", | |
| ">>>".cyan() | |
| ); | |
| } else { | |
| println!( | |
| "{} Hash cache active — only changed specs will be re-validated", | |
| ">>>".cyan() | |
| ); | |
| } |
src/validator.rs
Outdated
| // require_depends_on: require non-empty depends_on | ||
| if rules.require_depends_on == Some(true) { | ||
| // This is checked via frontmatter, but we access it through the body parse. | ||
| // The frontmatter was already parsed above; we check if depends_on is empty | ||
| // by looking at the spec_path's parsed frontmatter. | ||
| // Since we don't have fm here, we check the Dependencies section content instead. | ||
| if !body.contains("### Consumes") || { | ||
| let consumes_re = | ||
| Regex::new(r"(?s)### Consumes\s*\n\|.*?\|\s*\n\|[-| ]+\|\s*\n(\|.+)").unwrap(); | ||
| !consumes_re.is_match(body) | ||
| } { | ||
| result | ||
| .warnings | ||
| .push("No consumed dependencies documented (rule: require_depends_on)".to_string()); | ||
| } | ||
| } |
There was a problem hiding this comment.
require_depends_on is documented as requiring non-empty depends_on in frontmatter, but the implementation instead inspects the markdown body ("### Consumes") and can warn even when depends_on is set, or miss cases where the section differs. Since validate_spec already has parsed frontmatter, pass fm (or fm.depends_on) into apply_custom_rules and validate the actual frontmatter field.
src/validator.rs
Outdated
| if !body.contains("### Consumes") || { | ||
| let consumes_re = | ||
| Regex::new(r"(?s)### Consumes\s*\n\|.*?\|\s*\n\|[-| ]+\|\s*\n(\|.+)").unwrap(); | ||
| !consumes_re.is_match(body) | ||
| } { |
There was a problem hiding this comment.
Regex::new(...) is compiled on every validation run when require_depends_on is enabled. Since this runs per-spec, consider compiling the regex once (e.g., OnceLock/LazyLock) to avoid repeated regex compilation overhead.
| let coverage = compute_coverage(root, &spec_files, &config); | ||
| let overall_passed = total_errors == 0 && (!strict || total_warnings == 0); | ||
| let staleness_warnings = stale_entries.len(); | ||
| let effective_warnings = total_warnings + staleness_warnings; | ||
| let overall_passed = total_errors == 0 && (!strict || effective_warnings == 0); | ||
|
|
||
| Ok(json!({ | ||
| "passed": overall_passed, | ||
| "specs_checked": spec_files.len(), | ||
| "specs_passed": passed, | ||
| "total_errors": total_errors, | ||
| "total_warnings": total_warnings, | ||
| "total_warnings": effective_warnings, | ||
| "errors": all_errors, | ||
| "warnings": all_warnings, | ||
| "stale": stale_entries, |
There was a problem hiding this comment.
total_warnings is incremented by stale_entries.len(), but the warnings array doesn’t include corresponding warning strings (they’re only in the separate stale field). This makes the warning count inconsistent with the returned warnings list and may break clients that expect warnings.len() == total_warnings. Consider either (1) adding staleness messages into warnings, or (2) keeping a separate staleness_warnings field and leaving total_warnings as the count of warnings.
src/hash_cache.rs
Outdated
| // ---------- Minimal SHA-256 implementation ---------- | ||
| // Using a small inline implementation to avoid adding a dependency. | ||
| // This is the standard FIPS 180-4 algorithm. | ||
|
|
||
| struct Sha256 { | ||
| state: [u32; 8], | ||
| buf: Vec<u8>, |
There was a problem hiding this comment.
This file includes a custom SHA-256 implementation (“Minimal SHA-256 implementation”). Implementing cryptographic primitives manually is risky and harder to audit; prefer a well-vetted crate (e.g., sha2) unless there’s a strong reason to avoid the dependency.
- Replace inline SHA-256 (~120 lines) with sha2 crate - Fix serde_json error not converting to io::Error in HashCache::save - Fix unsafe UTF-8 truncation (panics on multi-byte chars) in ai.rs - Fix dead "y" path in re-validate prompt — now triggers AI regen - Fix misleading "hash cache active" message when --strict is on - Fix require_depends_on to check frontmatter field instead of body regex - Use LazyLock for regexes compiled per-spec in validator - Fix MCP warning count inconsistency (staleness in count but not array) - Deduplicate auto_regen_stale_specs legacy/standard path code - Fix fragile changelog header detection (position-based, not content-based) Co-Authored-By: Claude Opus 4.6 <[email protected]>
Summary
.specsync/hashes.jsonsospecsync checkskips unchanged filesdraft/active/stable/deprecatedfrontmatter validation with role-appropriate behavior[rules]section in.specsync.toml(max_changelog_entries,require_behavioral_examples,min_invariants,max_spec_size_kb,require_depends_on)agent_policyfrontmatter field (read-only,suggest-only,full-access)specsync view --role dev|qa|product|agent [--spec module]specsync compact [--keep N] [--dry-run]merges old entries into summariesspecsync archive-tasks [--dry-run]moves completed tasks to archive section--forcerun then incremental hash-cached validationTest plan
cargo fmtcleanview,compact,archive-tasks)🤖 Generated with Claude Code