Skip to content

Commit 9b3f9b4

Browse files
committed
Realign spec and plan with implementation; add spec case 28 E2E
Three review findings: - JSON contract drift: spec/plan documented `path, line, host, url` but `url` actually carried the full line excerpt. Update spec and plan to match the implemented shape: `path, line_no, host, line`. The new names describe what the fields contain. - Stale resolved-gix section: spec previously declared "No tree-vs- tree Platform machinery is used" and prescribed manual path-to- blob map walking. That approach silently broke renames, so the implementation now uses `old_tree.changes()` with `track_rewrites` and `for_each_to_obtain_tree`. Update the spec to describe the current approach and explain why the earlier resolution was reversed. Also refresh the staged/changed-vs pseudocode sketches. - Spec case 28 coverage gap: add an E2E test for the "feature branch behind base" topology where merge-base(base, HEAD) == HEAD, so the diff is empty even when the base ref has introduced a violation.
1 parent b228ef6 commit 9b3f9b4

3 files changed

Lines changed: 141 additions & 72 deletions

File tree

crates/trusted-server-cli/tests/lint_domains_cli.rs

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -306,6 +306,79 @@ fn changed_vs_reports_feature_branch_lines() {
306306
.stdout(predicate::str::contains("disallowed host test.com"));
307307
}
308308

309+
/// Spec case 28: when HEAD is behind the base ref, the merge-base
310+
/// is HEAD itself and the diff is empty — so no violations are
311+
/// reported even if the base ref has introduced one. This exercises
312+
/// the merge-base path with an "anti-symmetric" topology.
313+
#[test]
314+
fn changed_vs_branch_behind_base_reports_nothing() {
315+
let temp = tempfile::tempdir().expect("should create tempdir");
316+
let repo = common::init_repo(temp.path());
317+
318+
// Base: a single clean commit on `main`.
319+
std::fs::write(temp.path().join("a.rs"), "let ok = 1;\n").expect("should write base");
320+
common::stage_all(&repo);
321+
common::commit_all(&repo, "base");
322+
323+
// Branch from `main` at the base commit (no further commits on
324+
// the feature branch — HEAD is at the merge-base).
325+
common::create_and_checkout_branch(&repo, "feature");
326+
327+
// Advance `main` past the merge-base with a commit that, if
328+
// wrongly attributed to the feature branch, would be a
329+
// violation. Then move HEAD back to `feature`.
330+
use gix::refs::transaction::{Change, LogChange, PreviousValue, RefEdit, RefLog};
331+
use gix::refs::{FullName, Target};
332+
let main_ref: FullName = "refs/heads/main".try_into().expect("valid ref name");
333+
let head_edit = RefEdit {
334+
change: Change::Update {
335+
log: LogChange {
336+
mode: RefLog::AndReference,
337+
force_create_reflog: false,
338+
message: gix::bstr::BString::from("switch to main"),
339+
},
340+
expected: PreviousValue::Any,
341+
new: Target::Symbolic(main_ref),
342+
},
343+
name: "HEAD".try_into().expect("HEAD"),
344+
deref: false,
345+
};
346+
repo.edit_reference(head_edit)
347+
.expect("should switch HEAD to main");
348+
std::fs::write(
349+
temp.path().join("a.rs"),
350+
"let ok = 1;\nlet ahead = \"https://test.com\";\n",
351+
)
352+
.expect("should write main-ahead change");
353+
common::stage_all(&repo);
354+
common::commit_all(&repo, "main: ahead of feature");
355+
356+
// Move HEAD back to feature.
357+
let feature_ref: FullName = "refs/heads/feature".try_into().expect("valid ref name");
358+
let head_edit = RefEdit {
359+
change: Change::Update {
360+
log: LogChange {
361+
mode: RefLog::AndReference,
362+
force_create_reflog: false,
363+
message: gix::bstr::BString::from("switch to feature"),
364+
},
365+
expected: PreviousValue::Any,
366+
new: Target::Symbolic(feature_ref),
367+
},
368+
name: "HEAD".try_into().expect("HEAD"),
369+
deref: false,
370+
};
371+
repo.edit_reference(head_edit)
372+
.expect("should switch HEAD back to feature");
373+
374+
// `--changed-vs main`: merge-base(main, feature) == feature, so
375+
// diff is empty. The `main`-introduced violation must NOT appear.
376+
ts_in(&temp)
377+
.args(["dev", "lint", "domains", "--changed-vs", "main"])
378+
.assert()
379+
.code(0);
380+
}
381+
309382
/// A `--changed-vs` ref that doesn't resolve in any of the four
310383
/// fallback locations is an environment error (exit 2), not a
311384
/// violation (exit 1).

docs/superpowers/plans/2026-05-18-ts-dev-lint-domains.md

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2422,7 +2422,7 @@ pub fn run(args: crate::dev::lint::DomainsArgs)
24222422
path: line.path.clone(),
24232423
line: line.line_no,
24242424
host: v.host,
2425-
url_excerpt: line.content.clone(),
2425+
line_excerpt: line.content.clone(),
24262426
});
24272427
}
24282428
}
@@ -2451,10 +2451,11 @@ pub fn run(args: crate::dev::lint::DomainsArgs)
24512451
#[derive(Debug, serde::Serialize)]
24522452
pub struct FileViolation {
24532453
pub path: std::path::PathBuf,
2454+
#[serde(rename = "line_no")]
24542455
pub line: usize,
24552456
pub host: String,
2456-
#[serde(rename = "url")]
2457-
pub url_excerpt: String,
2457+
#[serde(rename = "line")]
2458+
pub line_excerpt: String,
24582459
}
24592460

24602461
fn emit_human(violations: &[FileViolation])

docs/superpowers/specs/2026-05-18-check-domains-design.md

Lines changed: 64 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -806,48 +806,40 @@ fn staged_added_lines(repo_path: &Path)
806806
-> Result<Vec<DiffLine>, Report<DomainsLintError>>
807807
{
808808
let repo = gix::open(repo_path).change_context(DomainsLintError::OpenRepo)?;
809-
let head_tree = repo
810-
.head_commit().change_context(DomainsLintError::OpenRepo)?
811-
.tree_id().change_context(DomainsLintError::OpenRepo)?;
812-
let head_tree = repo.find_tree(head_tree).change_context(DomainsLintError::OpenRepo)?;
813-
let index = repo.index().change_context(DomainsLintError::Index)?;
814-
815-
// Walk both sides into path -> blob_id maps.
816-
let head_map = tree_blob_map(&head_tree)?; // breadthfirst.files()
817-
let index_map = index_blob_map(&index); // entries() filtered to FILE
818-
819-
let mut out = Vec::new();
820-
for (path, head_id, index_id) in classify_changes(&head_map, &index_map) {
821-
if !path_is_scanned(&path) { continue; }
822-
let old = head_id.map(|id| read_blob(&repo, id)).transpose()?;
823-
let new = match index_id {
824-
Some(id) => read_blob(&repo, id)?,
825-
None => continue, // deletion — no added lines
826-
};
827-
for (line_no, content) in added_lines(old.as_deref(), &new) {
828-
out.push(DiffLine { path: path.clone(), line_no, content });
829-
}
830-
}
831-
Ok(out)
809+
let head_tree = match repo.head_commit() {
810+
Ok(c) => repo.find_tree(c.tree_id()?.detach())?,
811+
Err(_) => repo.empty_tree(), // unborn HEAD
812+
};
813+
// Materialise the index as a tree so we can diff trees uniformly
814+
// with rename detection enabled.
815+
let index_tree_id = write_index_to_tree(&repo)?;
816+
let index_tree = repo.find_tree(index_tree_id)?;
817+
collect_added_from_trees(&repo, &head_tree, &index_tree)
832818
}
833819
```
834820

835-
**The `gix` API surface is RESOLVED by the Phase 2 spike** — see
836-
`crates/trusted-server-cli/tests/spike_gix_staged_diff.rs` for the
837-
reference implementation and the
838-
[Resolved by the Phase 2 spike](#resolved-by-the-phase-2-spike)
839-
section for the full entry-point list. The conceptual operations:
821+
**The `gix` API surface is RESOLVED by the implementation** — see
822+
`crates/trusted-server-cli/src/dev/lint/domains.rs` for
823+
`collect_added_from_trees` and `write_index_to_tree`. The conceptual
824+
operations:
840825

841826
1. Open the repository — `gix::open(path)`.
842827
2. Resolve the HEAD commit's tree — `repo.head_commit()?.tree_id()?`
843-
then `repo.find_tree(id)?`.
844-
3. Read the index — `repo.index()?`.
845-
4. Walk the HEAD tree (`tree.traverse().breadthfirst.files()`) and
846-
the index (`index.entries()`) into `path → blob_id` maps, then
847-
compare the maps directly to classify Added / Modified / Deleted.
848-
**No tree-vs-tree `Platform`/`for_each_to_obtain_tree` machinery
849-
is used** — the direct map comparison is simpler and sidesteps
850-
the index→tree conversion gix 0.83 does not expose cleanly.
828+
then `repo.find_tree(id)?`. On an unborn HEAD use
829+
`repo.empty_tree()`.
830+
3. Materialise the index as a tree — iterate `index.entries()`
831+
filtered to `Mode::FILE`, `editor.upsert(entry.path(&index),
832+
EntryKind::Blob, entry.id)` on `repo.edit_tree(empty)`, then
833+
`editor.write()`. This lets the same tree-vs-tree machinery
834+
serve both staged and `--changed-vs` modes.
835+
4. Run a tree-vs-tree diff with rename detection —
836+
`old_tree.changes()?`
837+
`Platform::for_each_to_obtain_tree(&new_tree, callback)` with
838+
`track_rewrites(Some(Rewrites { copies: None, percentage:
839+
Some(0.5), limit: 1000, track_empty: false }))`. The callback
840+
matches `Change::{Addition, Modification, Rewrite, Deletion}`
841+
pure renames (same blob, new path) yield no added lines, and
842+
rename + edit diffs the matched old blob vs the new blob.
851843
5. Read each blob's content — `repo.find_object(id)?.data`.
852844
6. Run a line-level diff — `gix::diff::blob::Diff::compute(
853845
Algorithm::Myers, &InternedInput::new(old, new))`, then walk
@@ -859,8 +851,10 @@ Algorithm::Myers, &InternedInput::new(old, new))`, then walk
859851
- No diff-text parsing — line numbers and content come from typed
860852
hunk structs.
861853
- No locale / quote-path / `b/` prefix / `/dev/null` edge cases.
862-
- Renamed files are handled by `gix`'s change-detection (provides both
863-
old and new path).
854+
- Renamed files are handled by `gix`'s built-in rename detection
855+
(`track_rewrites` on the tree-diff `Platform`) — pure renames
856+
introduce no added lines; rename + edit reports only the truly new
857+
lines.
864858
- Filenames with spaces or non-UTF8 characters: `gix` paths are
865859
`BString` (byte strings). The script lossy-converts to UTF-8 for
866860
output and emits a stderr warning for non-UTF-8 paths.
@@ -881,31 +875,12 @@ fn changed_vs_added_lines(repo_path: &Path, reference: &str)
881875
.merge_base(base_id, head_id)
882876
.change_context_lazy(|| DomainsLintError::MergeBase { base: reference.into() })?
883877
.detach();
884-
let base_tree = repo.find_tree(
885-
repo.find_commit(merge_base)?.tree_id()?.detach()
886-
)?;
887-
let head_tree = repo.find_tree(
888-
repo.find_commit(head_id)?.tree_id()?.detach()
889-
)?;
878+
let base_tree = commit_tree(&repo, merge_base)?;
879+
let head_tree = commit_tree(&repo, head_id)?;
890880

891-
// Same map-comparison approach as staged_added_lines: walk both
892-
// trees into path -> blob_id maps, classify, blob-diff each
893-
// changed path. See the spike at tests/spike_gix_changed_vs.rs.
894-
let base_map = tree_blob_map(&base_tree)?;
895-
let head_map = tree_blob_map(&head_tree)?;
896-
let mut out = Vec::new();
897-
for (path, base_blob, head_blob) in classify_changes(&base_map, &head_map) {
898-
if !path_is_scanned(&path) { continue; }
899-
let old = base_blob.map(|id| read_blob(&repo, id)).transpose()?;
900-
let new = match head_blob {
901-
Some(id) => read_blob(&repo, id)?,
902-
None => continue,
903-
};
904-
for (line_no, content) in added_lines(old.as_deref(), &new) {
905-
out.push(DiffLine { path: path.clone(), line_no, content });
906-
}
907-
}
908-
Ok(out)
881+
// Same tree-vs-tree diff with rename tracking as staged mode;
882+
// the index is just swapped for the merge-base tree.
883+
collect_added_from_trees(&repo, &base_tree, &head_tree)
909884
}
910885
```
911886

@@ -1170,9 +1145,9 @@ Run `ts dev lint domains` (no args) for a full-repo audit.
11701145
"violations": [
11711146
{
11721147
"path": "crates/trusted-server-core/src/foo.rs",
1173-
"line": 42,
1148+
"line_no": 42,
11741149
"host": "test.com",
1175-
"url": "https://test.com/path"
1150+
"line": "let x = \"https://test.com/path\";"
11761151
}
11771152
],
11781153
"count": 1,
@@ -1899,15 +1874,35 @@ the question.
18991874
InternedInput}``Diff::compute(Algorithm::Myers, &input)`,
19001875
then `diff.hunks()`; each `Hunk.after` is the new-side token
19011876
(line) range.
1902-
- **No tree-vs-tree `Platform` machinery is used.** Both the
1903-
staged and `--changed-vs` collectors walk the two trees into
1904-
`path → blob_id` maps and compare directly — simpler than
1905-
`for_each_to_obtain_tree` and avoids the index→tree conversion
1906-
gix 0.83 does not expose cleanly.
1877+
- **Tree-vs-tree diff with rename detection.** Both the staged
1878+
and `--changed-vs` collectors call `old_tree.changes()`
1879+
`Platform::for_each_to_obtain_tree(&new_tree, ...)` with
1880+
`track_rewrites(Some(Rewrites { copies: None, percentage:
1881+
Some(0.5), limit: 1000, track_empty: false }))`. The callback
1882+
iterates `Change::{Addition, Modification, Rewrite,
1883+
Deletion}` — pure renames (same blob, new path) yield no
1884+
added lines; rename + edit diffs the matched old blob vs the
1885+
new blob.
1886+
1887+
**Resolution note:** an earlier revision of this spec rejected
1888+
`Platform`/`for_each_to_obtain_tree` and prescribed a manual
1889+
map-walk. That approach silently broke renames: a renamed file
1890+
hit `(None, Some(new_id))` and was diffed against an empty
1891+
blob, reporting every line of the renamed file as added
1892+
(including pre-existing violations the author never touched).
1893+
The current spec uses the `Platform` API so rename detection
1894+
is correct by construction.
1895+
1896+
For staged mode, the index is first materialised as a tree
1897+
via `Repository::edit_tree` → per-entry `Editor::upsert(path,
1898+
EntryKind::Blob, entry.id)``Editor::write()`, then the
1899+
same tree-vs-tree path serves both modes.
1900+
19071901
- **gix-config:** `File::from_path_no_includes(path,
19081902
Source::Local)`, `File::set_raw_value` (dotted `AsKey` form —
19091903
avoids the `File<'event>` invariance that bites
19101904
`set_raw_value_by`), `File::raw_value`, `File::to_bstring`.
1905+
19111906
2. **`gix` / `gix-config` version pins — RESOLVED.** `gix = 0.83`,
19121907
`gix-config = 0.56`, same gitoxide release family. See
19131908
[Cargo dependencies](#cargo-dependencies) for the full feature

0 commit comments

Comments
 (0)