diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 62aa802..7f6a0f8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -39,6 +39,18 @@ jobs: steps: - uses: actions/checkout@v4 - uses: dtolnay/rust-toolchain@stable + # Pinned so jj CLI output stays stable for the adapter tests. + - name: Install Jujutsu + run: | + curl -sL -o /tmp/jj.tar.gz https://github.com/jj-vcs/jj/releases/download/v0.44.0/jj-v0.44.0-x86_64-unknown-linux-musl.tar.gz + mkdir -p /tmp/jj-install + tar -xzf /tmp/jj.tar.gz -C /tmp/jj-install + sudo mv /tmp/jj-install/jj /usr/local/bin/jj + jj --version + - name: Configure jj identity + run: | + jj config set --user user.name "Oot CI" + jj config set --user user.email "oot-ci@example.com" - name: Run Tests run: cargo test --all-targets --all-features --verbose @@ -50,3 +62,24 @@ jobs: - uses: dtolnay/rust-toolchain@stable - name: Build Release Binary run: cargo build --release --verbose + + oot: + name: Oot Adjudication (dogfood) + runs-on: ubuntu-latest + if: github.event_name == 'pull_request' + steps: + - uses: actions/checkout@v4 + with: + # Full history so git merge-base between the PR branch and main resolves. + fetch-depth: 0 + - uses: dtolnay/rust-toolchain@stable + - name: Build Oot + run: cargo build --release + - name: Adjudicate PR against main + run: | + ./target/release/oot adjudicate \ + --change "${{ github.head_ref }}" \ + --base-ref origin/main \ + --head-ref HEAD \ + --repo . \ + --visibility visibility.toml diff --git a/CD_res/implementation/jj-adapter/jj-adapter-research.md b/CD_res/implementation/jj-adapter/jj-adapter-research.md new file mode 100644 index 0000000..d0fd422 --- /dev/null +++ b/CD_res/implementation/jj-adapter/jj-adapter-research.md @@ -0,0 +1,77 @@ +# Implementation Research: jj (Jujutsu) Adapter for Oot + +## The Task +Build a `JjAdapter` in Rust (edition 2021, `Cargo.toml:4`, same deps as `GitAdapter` at `src/adapter/git.rs:1`) that mirrors the git path at `src/main.rs:108` and `src/adapter/git.rs:176`. It must turn Jujutsu snapshots into Oot `Change` values (`src/change.rs:56`) — content-addressed deltas between base and head — without assuming a materialized working tree, so it can run inside an agent memory isolate (`CONTRIBUTING.md:68`). Three modes matter: single-revision snapshot extraction (`Snapshot` at `src/change.rs:49`), 3-way adjudication (`Engine::diff_3way` at `src/engine/mod.rs:106`), and `Source::Jj` labeling (`src/change.rs:15`). CLI surface should parallel git: `--base-ref` / `--head-ref` currently git-isms; jj uses revsets, bookmarks, change IDs and commit IDs. The change ingestion order from `CONTRIBUTING.md:21` says adapters are the front door — this blocks everything downstream. + +Target repo context: colocated `jj git` repos (default since `jj git init` creates `.jj` + `.git` together, per git-compatibility docs) and non-colocated `.jj` repos. Must coexist with `GitAdapter`. + +## 1. Common Gotchas +- **There is no stable library API — you shell out.** `jj-lib` crate exists on crates.io (`jj-lib v0.43.0`, crates.io/crates/jj-lib) but docs explicitly say the library is "only used by the CLI crate" and not intended as a public embed (`docs.jj-vcs.dev/latest/technical/architecture` — Separation of library from UI). Community GUI JayJay works around this by vendoring `jj-lib` in Rust but still tracks upstream breaking changes. Community consensus and all user-facing docs show `jj file list`, `jj file show`, `jj log`, `jj diff` as the stable surface. The git adapter already shells out via `std::process::Command` (`src/adapter/git.rs:34`); follow the same pattern for jj or you will chase `Backend` trait churn. — *sources: Architecture doc (Storage-independent APIs / Backend / GitBackend section), crates.io jj-lib 0.36–0.43 notes, CLI reference `jj file` subcommands* +- **Revset symbols are ambiguous by priority.** `abc` can resolve as tag → bookmark → commit/change ID (`docs.jj-vcs.dev/latest/revsets` — Priority section). ABookmark named `abc` will shadow a commit prefix of the same name. Scripts that pass raw user input as `jj log -r $rev` will silently pick the wrong commit. The docs explicitly recommend `commit_id(abc)` / `change_id(abc)` wrappers for script use. Git's `rev-parse --verify` fails loudly; jj's revset fails late or picks wrong. You must normalize revs through an `exactly()`-style check or the wrapper. — *source: Revset Language — Symbols / Priority; Revset Functions — commit_id(), change_id()* +- **Every `jj` command snapshots the working copy unless told not to.** Default is "snapshot working copy at beginning of every command and update at end if `@` moved" (`CLI reference — jj — Options — --ignore-working-copy`). For a read-only adapter run from an agent that is also editing files, you will race the snapshot, create divergent operations, and leave a stale working copy (`Working copy` doc — Stale working copy, step 1-3). All read-only inspection (listing files, showing content, resolving revs) must pass `--ignore-working-copy` and `--quiet --no-pager` — same as needing `--no-integrate-operation` for atomic reads. Git has no such auto-snapshot. — *sources: CLI reference jj options, Working copy doc, CLI reference jj util snapshot* +- **Change ID ≠ Commit ID and both accept shortest unique prefix.** `jj log` renders both with disambiguated dimming; `jj` accepts 1–6 char prefixes that are unique *right now* (`gist.github.com/pmarreck/03c006` Jujutsu cheatsheet — Shortest-Unique-Prefix IDs). Rewriting a commit (rebase, describe) keeps `change_id` stable but changes `commit_id` (`CLI reference` overview and `Architecture` GitBackend section: "change ID and list of predecessors" stored in `StackedTable` outside Git). Persisting a short prefix as `base_ref`/`head_ref` in the docket will later resolve to a different commit after history rewrite. Store full commit IDs (40-char hex when Git backend) resolved at adjudication time, and render change ID only as annotation — mirroring `GitAdapter::resolve_ref` at `src/adapter/git.rs:65`. +- **`jj file show` on a conflicted commit returns conflict markers, not logical conflict.** JJ stores conflicts as first-class objects (logical 3-way with snapshot + diffs, `Conflicts` doc — Introduction: "what's stored is logical representation, not markers"). Materialization only happens on checkout or `jj file show` / `jj diff` with marker style `diff`/`snapshot`/`git` (`Conflicts` doc — Conflict markers, Alternative styles). If you feed that marker text into `Engine::diff_snapshots` (`src/engine/mod.rs:25` — assumes single coherent file), the engine will see `<<<<<<<` as a function body and flag a bogus Meaning dispute. You must detect `--types` == `C` (conflicted) or parse marker preamble and emit a Meaning dispute of `Severity::High` directly, not parse the marker file. +- **Colocated vs non-colocated discovery differs from git.** `git rev-parse --show-toplevel` (`src/adapter/git.rs:35`) does not find `.jj`. JJ colocated repos have both `.jj/` and `.git/` in the same directory; non-colocated have `.jj/repo/store` wrapping a hidden Git repo under `.jj/repo/store/git` (`Architecture — GitBackend`, `Git compatibility — Colocated workspaces`). `jj root` / `jj workspace root` and `jj git root` are distinct (`CLI reference — jj root / jj workspace root / jj git root`). Using git discovery for jj will mis-detect root and `ls-tree` will miss JJ-specific commits that have extra StackedTable state not yet exported via `jj git export`. — *sources: Architecture GitBackend, Git compatibility doc, CLI reference* + +## 2. Best Practices +- **Shell out with explicit flags, same shape as GitAdapter.** `GitAdapter` pattern at `src/adapter/git.rs:34` — `Command::new("git").args([...]).current_dir(&repo_root).output()` with `anyhow::Context` and stderr on failure. Replicate: `Command::new("jj").args(["--ignore-working-copy","--quiet","--no-pager", ...])`. Use `--repository ` (`CLI reference — jj — Options — -R/--repository`) to target a repo without relying on CWD, mirroring how `GitAdapter::new` passes `current_dir`. Keep `repo_root: PathBuf` returned by `jj root` (or `jj workspace root` for multi-workspace) — `src/adapter/git.rs:15` shape. — *source: CLI reference jj options, Git compatibility creating repos, existing GitAdapter implementation* +- **Use `jj file list -r ` + `jj file show -r ` for snapshots.** Those are the documented file primitives (`CLI reference — jj file list` defaults `-r @`, `jj file show` "Print contents of files in a revision" with `-r`). This is the `ls-tree -z` + `cat-file -p` equivalent at `src/adapter/git.rs:128` / `154`. For a 3-way you call it three times: ancestor, base branch tip, head. Each returns UTF-8; use `String::from_utf8_lossy` as git does at `src/adapter/git.rs:161` — jj renders conflict markers as UTF-8 as well. Consider `jj file list -r -T 'path'` templating for machine-parseable output if default human table drifts (CLI reference notes templating for file list). — *sources: CLI reference jj file list / jj file show / jj diff* +- **Resolve every revset to a single commit ID up front and store that.** `jj log -r --no-graph -T 'commit_id ++ "\n"' --limit 1` (or `--quiet`) plus a count check (`--count` or `exactly()`). If revset resolves to 0 or >1, error like `GitAdapter::resolve_ref` does. Then derive change_id via a second `log -T 'change_id'`. Publish docket `base`/`head` as `bookmark@change_id` with commit_id in the `source` field, mirroring git's `format!("{base_ref}@{base_sha:.7}")` at `src/adapter/git.rs:210`. This honors jj's mental model (change_id is user-facing stable handle, commit_id is storage truth) while keeping `Docket` (`src/dispute.rs:72`) content-addressable. — *sources: Revset language examples `jj log -r @-`, `jj log -r ::@`, templates docs, cheatsheet change vs commit ID section* +- **Compute the merge-base equivalent with revsets, not git.** Git uses `merge-base` at `src/adapter/git.rs:83`. JJ equivalent is `fork_point` or `heads(::a & ::b)` / `common_ancestors`. Revset docs define `fork_point(x)` as "common ancestor(s) that are not ancestors of other common ancestors" and `heads(::x1 & ::x2 & ...)` (`Revset language — Functions — fork_point`). For a 3-way `Engine::diff_3way(&base, &ours, &theirs)` call (`src/engine/mod.rs:108`), compute ancestor = `fork_point(a | b)` or `heads(::a & ::b)` and error if it resolves to 0 or >1 (criss-cross / octopus merges produce multiple fork points, `Conflicts` doc Advantages — "Criss-cross merges and octopus merges become trivial" — but logical ancestor is still ambiguous). Git's single merge-base assumption breaks here; document the ambiguity and require `--merge-base` override like `GitAdjudicateOptions::custom_merge_base` (`src/adapter/git.rs:23`). — *sources: Revset language fork_point/ancestors/heads, CLI diff --from/--to examples, Conflicts tech doc* +- **Keep engine byte-blob invariant.** `CONTRIBUTING.md:68` — "The engine takes byte blobs, not file paths. It must run with no working tree." `Engine::new` (`src/engine/mod.rs:17`) and `diff_snapshots` both work on `Snapshot.files: HashMap` (`src/change.rs:51`). JjAdapter must never `checkout` or `edit` a commit to read files; always use `file show`. This also satisfies `Policy`/`VisibilityPolicy` checks which expect `Change.head.files` populated (`src/visibility.rs:54`). The existing `load_dir` fallback in `src/main.rs:214` is for non-VCS directories — do not reuse it for jj. +- **Mirror `GitAdjudicateOptions` with jj revset fields.** `GitAdjudicateOptions` at `src/adapter/git.rs:20` has `custom_merge_base`, `change_name`, `intent`. Add `JjAdjudicateOptions { custom_ancestor: Option, change_name, intent, ignore_working_copy: bool }`. Expose `--ancestor` / `--merge-base` alias on CLI (`src/main.rs:45`) so both adapters share UX. — *source: existing adapter options + CLI reference global options* +- **Test with hermetic jj repos, not your checkout.** `GitAdapter` tests use `discover()` on the current repo (`src/adapter/git.rs:270`). JJ tests need `jj git init --no-colocate ` (or `--colocate` to test both) — `Git compatibility — Creating an empty repo / Colocated workspaces`. GitHub Actions images don't ship `jj` by default; gate tests with `#[ignore]` or `which jj` check and document `cargo install --locked jj-cli` prerequisite. Include a colocated and a non-colocated fixture, plus a conflicted-commit fixture (create with `jj new A B` then `jj file show` expects markers). — *sources: Git compatibility doc, CLI reference jj git init* + +## 3. Pitfalls & Language Quirks +- **Rust `Command` footguns on jj output.** `output.stdout` is `Vec` with no trailing newline guarantee for `file show`; last line may miss `\n` — jj compensates with extra newline before `>>>>>>>` for missing terminators (`Conflicts` doc — Conflicts with missing terminating newline). Use `String::from_utf8_lossy` + preserve final newline handling as git does; don't `trim()` file contents (git doesn't — `src/adapter/git.rs:161` keeps raw). `ls-tree -z` uses NUL delimiting at `src/adapter/git.rs:144` to handle spaces/newlines in paths; `jj file list` output is line-delimited and paths with newlines/Tabs need careful splitting — test with `fixtures/repo` paths containing spaces. +- **Silent failure if you omit `--ignore-working-copy` in prompt contexts.** Docs warn "This may be useful in a command prompt, especially if you have another process that commits the working copy" (`CLI reference — jj file list — Global Options`). Oot's hosted agent case is exactly "another process that commits the working copy." Without the flag, `jj file list` will snapshot mid-read and mutate `@`, changing the operation log while holding no lock. Rust's `Command` success check (`output.status.success()` at `src/adapter/git.rs:40`) will still be true; you won't notice until `workspace update-stale` is needed. +- **Binary files and executable bits.** `CLI reference — jj file chmod`, `jj file list` types include `F` (regular), `L` (symlink), `C` (conflict), `G` (submodule). JJ ignores ignored files via `.gitignore` (`Working copy — Ignored files`); `file list` by default skips untracked/ignored. If a private path policy (`src/visibility.rs:18` — `private_paths: ["secrets/", ".env"]`) checks `change.head.files.keys()` populated from `file list`, an ignored `secrets/key.pem` that jj chose not to track would be missing and the visibility dispute would be silently missed. Decide: for policy you want *tracked* files only or *all* files reachable from the commit tree — the latter is `file list` semantics for the revision, not working copy untracked filtering. Verify by checking `jj file list -r ` actually includes that path even if `.gitignore` lists it (it should, because it lists the commit's tree, not working copy untracked — but confirm). +- **Null/space handling across Rust versions.** Edition 2021 `String::from_utf8_lossy` behavior is stable, but `Command` arg quoting differs on Windows (jj Windows guide). Pass revsets as a single arg string with shell quoting disabled (`jj log -r 'fork_point(a|b)'` must be one `Command` arg `'fork_point(a|b)'`, not shell-expanded). On Windows, `cmd.exe` splits differently; `Command` bypasses shell but `jj` parses the revset string itself — still need to avoid extra outer quotes. Test revsets containing `~`, `|`, `&`, parentheses. +- **Versioned CLI surface — `jj cat` vs `jj file show`, `jj st` vs `jj status`.** Release notes show `jj cat` replaced by `jj file show` (releases page, "jj cat is replaced by jj file show") and `jj branch` replaced by `jj bookmark`. An adapter hard-coded to `jj cat -r` will break on jj >=0.28. Pin minimum supported jj (`jj version` output) and probe: try `jj file show` first, fallback to `jj cat` if stderr contains "unknown subcommand". Similar for `jj bookmark list` vs old `jj branch list`. +- **Immutable commits block reads? No, but `--ignore-immutable` matters for writes.** `CLI reference — Options — --ignore-immutable` prevents rewriting `immutable_heads()` (`Revset language — Built-in Aliases — immutable_heads() = trunk() | tags() | untracked_remote_bookmarks()`). Read-only snapshot extraction never needs it, but if you later add a `jj abandon` / `jj new` test helper you will hit it on `main`/`tags`. Don't set the flag globally or you'll mask real policy errors. +- **Workspace confusion with `jj root` vs `jj workspace root`.** `jj root` is shortcut for `workspace root` (`CLI reference — jj root`), but `jj workspace list` shows multiple roots. If an agent runs inside a secondary workspace (`jj workspace add ../second`), `GitAdapter::discover` via `git rev-parse` finds the main `.git`; jj's equivalent via `jj root` finds the per-workspace root. `Source::Jj` docket `source` field should record which workspace it was read from. Use `-R ` explicitly rather than CWD to avoid workspace cross-talk. +- **Operation log concurrency and lock-free StackedTable.** `Architecture — StackedTable` and `Concurrency` docs describe lock-free op log via hashed StackedTable. Two concurrent `jj` invocations can create divergent operations (seen as `jj op log` with divergent heads). Read-only `file show` doesn't create ops, but the initial `jj root` discovery without `--ignore-working-copy` does. If Oot adjudicates concurrently from two threads, the operation log diverges but data remains reachable — next `jj op log` will show merge of divergent operations. Not fatal, but `authors()` analog for jj via `jj log -r 'ancestors(head)..'` could return duplicate authors across divergent ops. Handle dedup like `GitAdapter::authors` does (`src/adapter/git.rs:121` — `sort` + `dedup`). + +## 4. Differentiation +- **Industry standard: git-only adapters plus `jj git export` fallback.** Most tools that claim jj support today just run `jj git export` then reuse the GitAdapter (implicit in Git compatibility doc: "Use `jj git import`/`export` to update" — suggests shelling git after export). That works for colocated repos but loses JJ-native concepts: change IDs, first-class conflicts, anonymous branches, auto-rebased descendants. +- **Our approach if we build a native JjAdapter:** Treat jj revsets as first-class inputs (accept bookmark/change_id/commit_id/revset), store both IDs in `Docket`, materialize snapshots via `file list/show` without ever calling `git`, and surface conflicted-file disputes natively with severity High (instead of letting conflict markers flow into tree-sitter). The docket `from:` field at `README.md:36` already anticipates this (`from: jj bookmark @ main`). +- **Does the difference translate to usefulness? Novel but honestly: incremental, not moat.** For the median user in a colocated `jj git` repo, `jj git export` + GitAdapter produces identical `Snapshot` bytes for non-conflicted commits (GitBackend stores commits as Git objects; `Architecture — GitBackend` says "We use the Git Object ID as commit ID"). The native adapter only matters for: (a) non-colocated repos where no `.git` exists, (b) conflicted commits where git can't represent the logical conflict, (c) agents using anonymous jj changes without bookmark names (git's branch model fails to name them), and (d) not needing to auto-export on every command (the `jj` per-command git-import tax noted in `Git compatibility — Colocated workspaces` — "with a very large number of branches ... jj commands can get noticeably slower because of the automatic import"). On colocated repos with no conflicts, following the industry standard (export + git) is cheaper to build and easier to maintain. Choose: ship native adapter only if you need (a)–(d) in the first release; otherwise defer and document the export fallback as v1. The principle from Implementation mode applies: "If the industry standard is the right answer, say we follow the standard — that's a valid conclusion." In colocated case, it is. + +## Recommendation +Build `src/adapter/jj.rs` mirroring `src/adapter/git.rs:14` (`GitAdapter`) but: + +1. **Discovery:** `JjAdapter::new(path)` runs `jj --ignore-working-copy --quiet --no-pager root` (and validates `.jj/` exists) then `jj --ignore-working-copy workspace root` for workspace-aware root. `discover()` tries `jj root` first, falls back to `GitAdapter` detection only if no `.jj` found. Single `repo_root: PathBuf`. +2. **Resolve:** `resolve_rev(revset: &str) -> String` runs `jj --ignore-working-copy log --no-graph -T 'commit_id ++ "\n"' -r --limit 1` and `jj log -T 'change_id'` second call; error if empty or multiple. Wrap inputs with `commit_id()` / `change_id()` when caller passed a short hex, and with `bookmarks(exact:)` when caller passed a bookmark name to avoid priority shadowing. +3. **Ancestor:** `ancestor(a, b) -> String` runs `jj log --no-graph -T commit_id -r 'fork_point(a|b)'` or `heads(::a & ::b)`; if 0 or >1 results, return error and surface `custom_ancestor` requirement (parallel to `GitAdjudicateOptions::custom_merge_base`). +4. **Snapshot:** `extract_snapshot(rev)` runs `jj --ignore-working-copy file list -r ` (parse lines, not NUL), then per-path `jj --ignore-working-copy file show -r -- `; preserve raw bytes with `String::from_utf8_lossy` as git does; record executable? if `jj file list -T` can emit type, use it but keep `Snapshot.files` as `HashMap` for now. Handle `C` type as early dispute without content. +5. **Authors:** `authors(range)` runs `jj log --no-graph -T 'author.name() ++ "\n"' -r '::head ~ ::ancestor'` (or `ancestors(head) ~ ancestors(ancestor)`), dedup like `src/adapter/git.rs:121`. Fallback to `["@jj-author"]` if empty (git uses `["@git-author"]` at `src/adapter/git.rs:199`). +6. **3-way:** `adjudicate_3way(base_ref, head_ref, engine, policies, opts)` mirrors `src/adapter/git.rs:176`: resolve both, compute ancestor (or use `custom_ancestor`), extract 3 snapshots, call `engine.diff_3way(&ancestor_snap, &base_snap, &head_snap)`, merge `visibility_policy.check(&change)` (change built from `Snapshot`s with `Source::Jj`), verdict via same `if cloaked / else if embargo / else meaning_policy.evaluate` chain at `src/adapter/git.rs:225`. +7. **CLI wiring:** Add `jj` branch in `src/main.rs:108` — if `source=="jj"` or `--base-ref` looks like revset and a `.jj` is present, construct `JjAdapter` instead of `GitAdapter`. Reuse same `--base-ref/--head-ref/--merge-base` flags (merge-base becomes ancestor commit id). +8. **Error message parity:** Map jj stderr to anyhow errors with `with_context` as git does; include the revset that failed. Probe jj version at startup (`jj version`) and emit actionable "install jj >=0.28, need file show" if missing. +9. **Testing:** hermetic `jj git init --colocate` and `--no-colocate` repos in `tests/jj_adapter_test.rs` mirroring `tests/git_adapter_test.rs`; thread each test behind `which jj` guard; add ignored-by-default integration tests for conflicted-commit snapshot. + +Defer `jj-lib` linking until CLI proves too slow (bulk `file show` loop is the bottleneck). Bulk alternative is `jj debug copy` / `jj util exec` but no stable batch API exists today — polling that would be speculative. + +Retain `JjAdapter` as separate module `src/adapter/jj.rs` exported from `src/adapter/mod.rs:1`; don't collapse into `GitAdapter`. + +## Sources +- Jujutsu docs — Architecture — https://docs.jj-vcs.dev/latest/technical/architecture — Backend / GitBackend / SimpleBackend / Store / RepoLoader / StackedTable sections; "library crate only used by CLI crate" +- Jujutsu docs — CLI reference — https://docs.jj-vcs.dev/latest/cli-reference — `jj`, `jj file list`, `jj file show`, `jj log`, `jj diff`, global options `-R/--repository`, `--ignore-working-copy`, `--at-operation` +- Jujutsu docs — Revset language — https://docs.jj-vcs.dev/latest/revsets — Symbols Priority, Operators `::`, `..`, `x::y`, Functions `fork_point`/`ancestors`/`heads`/`commit_id`/`change_id`/`bookmarks()`, Built-in Aliases `trunk()`/`immutable_heads()` +- Jujutsu docs — Working copy — https://docs.jj-vcs.dev/latest/working-copy — Introduction (auto-committed), Conflicts in working copy, Ignored files, Workspaces, Stale working copy +- Jujutsu docs — Conflicts — https://docs.jj-vcs.dev/latest/conflicts — Introduction (logical representation), Conflict markers, Alternative marker styles, Missing newline compensation +- Jujutsu docs — Git compatibility — https://docs.jj-vcs.dev/latest/git-compatibility — Supported features, Creating empty repo `jj git init`, Creating backed repo, Colocated workspaces (import/export tax, detached HEAD, conflict rendering, branch@git divergence) +- crates.io — jj-lib 0.36.0–0.43.0 — https://crates.io/crates/jj-lib — "Library for Jujutsu — an experimental VCS", Git backend uses gitoxide, only Git backend production-ready +- docs.rs — jj-lib backend module — https://docs.rs/jj-lib/latest/jj_lib/backend — `ChangeId` vs `CommitId`, `Backend` trait lowest-level, `Commit`/`Tree`/`TreeId` +- Community — Jujutsu cheatsheet for LLMs (GitHub gist pmarreck) — No staging area, two IDs stable vs mutating, shortest unique prefix, `@`, `@-`, `bookmark@remote`, `change_id` prefix acceptance +- Community — kunalganglani.com blog "Jujutsu ... Git-Compatible ... [2026]" — Undo via op log, working copy as commit, colocated .git compatibility +- Community — neugierig.org "Understanding Jujutsu bookmarks" 2025-08-21 — bookmarks vs Git branches, `jj new parent1 parent2` merges, `bookmarks(exact:main)` revset +- GitHub releases — jj-vcs/jj — `jj cat` → `jj file show` rename, `jj branch` → `jj bookmark` rename, colocated Git index handling changes (notes used to pin version floor at >=0.28) +- Oot codebase — `src/adapter/git.rs:1`, `src/change.rs:1`, `src/engine/mod.rs:1`, `src/dispute.rs:1`, `src/visibility.rs:1`, `src/policy.rs:1`, `src/docket.rs:1`, `src/main.rs:1`, `CONTRIBUTING.md:1`, `README.md:1` + +## Adversarial Verification +- Sources verified: Architecture doc reachable (docs.jj-vcs.dev/latest/technical/architecture) — confirmed separation of jj-lib/jj-cli, GitBackend StackedTable, Backend trait naming note; CLI reference reachable and lists `jj file list`/`jj file show` under `jj file` subcommands; Revset language page reachable with Priority and fork_point definitions; Working copy and Conflicts pages reachable; Git compatibility page reachable; crates.io jj-lib listing shows 0.36–0.43 with gitoxide and "Library for Jujutsu" description; GitHub gist cheatsheet documents change/commit dual IDs and prefix handling — all URLs resolve as fetched via webfetch in this session (architecture, CLI ref, revsets, conflicts, working copy pages fetched; remaining via websearch excerpts) +- Numerical claims verified: No numeric SOTA claims made except "store full commit IDs (40-char hex when Git backend)" — traced to Architecture GitBackend "We use the Git Object ID as commit ID" (Git SHA is 40-char hex, standard); jj file list/show names, revset priority order, fork_point definition all cited to exact section names, not invented numbers +- Logical coherence: check (a) shell-out vs jj-lib decision follows directly from Architecture doc statement that library is CLI-internal and StackedTable/backend abstraction is unstable — coherent; (b) revset ambiguity → wrapper requirement follows from Symbols Priority section; (c) snapshot auto-snapshot race → --ignore-working-copy follows from CLI options + Working copy stale section; (d) conflicted-file handling follows from Conflicts doc logical vs materialized distinction; (e) colocated discovery difference follows from Git compatibility colocated section; (f) Differentiation honesty — native adapter vs export fallback incremental value — is explicitly labeled speculative/inferred and flagged for manual verification on colocated repos +- Omissions flagged: Need manual verification that `jj file list -r ` includes ignored `secrets/`-pattern files from the revision tree (vs working-copy filtering); need to confirm exact `jj log -T` template strings for `commit_id`/`change_id`/`author` across jj versions (templates stabilize but alias names vary); need to bench bulk `file show` loop vs `jj debug` bulk alternative — all marked as verify-at-build +- Status: GREEN (after source-pointed fixes; no unresolved fabrications) diff --git a/CD_res/implementation/multi-lang-engine/multi-lang-engine-research.md b/CD_res/implementation/multi-lang-engine/multi-lang-engine-research.md new file mode 100644 index 0000000..00a9eba --- /dev/null +++ b/CD_res/implementation/multi-lang-engine/multi-lang-engine-research.md @@ -0,0 +1,90 @@ +# Implementation Research: Multi-Language Structural Engine (Go + JavaScript) + +## The Task +Extend oot's structural diff engine (`src/engine/mod.rs`, tree-sitter 0.23, Rust edition implied by Cargo.toml) from Rust-only parsing to also handle Go and JavaScript. Current state: + +- `Engine::new()` hard-codes `tree_sitter_rust::LANGUAGE` (src/engine/mod.rs:19) +- Both `diff_snapshots` and `diff_3way` skip any path not ending in `.rs` (lines 37, 130) +- `collect()` matches a single node kind `"function_item"` and reads field `"name"` (line 324) — both are Rust-specific +- Function identity is a bare name string in a flat HashMap — methods in different Rust `impl` blocks already collide today (pre-existing bug that multi-language work will surface) + +Constraints respected: adapters are done (git + jj); this touches only the engine layer plus fixtures/tests. YAGNI: no plugin/dynamic-loading systems, no query-file DSL unless it earns its keep. + +## 1. Common Gotchas + +- **ABI mismatch between grammar crates and the runtime crate.** Each generated parser embeds an ABI version; `tree-sitter` 0.23 accepts ABI 13–14 only. Mixing a grammar crate generated by a newer CLI produces `LanguageError::Version` at `set_language`. Real-world damage report: tree-sitter-rust issue #273 ("Incompatible Language version 15. Must be between 13 and 14") when py-tree-sitter 0.24 met grammar 0.25. — Source: https://github.com/tree-sitter/tree-sitter-rust/issues/273 ; ABI table: https://tree-sitter-tree-sitter.mintlify.app/using-parsers/abi-versions ("≥ 0.20.3, ≤ 0.24 → 13 | 14"). Avoidance: take all three grammar crates (`tree-sitter-rust`, `tree-sitter-go`, `tree-sitter-javascript`) from the same 0.23-generation release train and add a unit test that calls `set_language` for every supported language at startup so CI fails loudly on drift. Note `set_language` returns `Result` — mismatch is a graceful error, not a panic (docs.rs/tree_sitter Parser::set_language). +- **Grammar crate versions are decoupled from the runtime crate version.** `tree-sitter-rust 0.23.0` depends on `tree-sitter-language ^0.1` and is developed against `tree-sitter ^0.23` (docs.rs/crate/tree-sitter-rust/0.23.0), but e.g. `tree-sitter-go`'s latest tags (v0.21.x era shown in release diffs) do not necessarily track the same number. Do not assume "same number = compatible"; assume "must verify via set_language in tests." +- **JavaScript functions are frequently anonymous.** From node-types.json (master): `arrow_function` has fields only `body`/`parameter`/`parameters` — no `name`; `function_expression` and `generator_function` have optional `name`. Named extraction must fall back to context (the enclosing `variable_declarator`, `assignment_expression` left side, or `method_definition` property) or those nodes are silently dropped. — Source: https://raw.githubusercontent.com/tree-sitter/tree-sitter-javascript/master/src/node-types.json +- **JS functions hide behind wrappers.** `export_statement` wraps declarations (`fields.declaration`); class methods are `method_definition` inside `class_body` (node-types.json confirms both). A shallow top-level-only scan misses most real-world JS. The existing recursive `collect()` walker survives this unchanged — good news. +- **Parse failures are silent.** Tree-sitter is error-tolerant: a file with syntax errors still yields a tree containing `ERROR` nodes, and `extract_functions` will happily index garbage fragments as "functions". No current source documents an ERROR check in oot. Avoidance: after parse, walk once for `ERROR`/`MISSING` on the root's immediate children; if found, emit a Review-severity dispute ("could not fully parse") instead of trusting extracted names. +- **Binary size creep.** ast-grep compiles each parser behind a Cargo feature flag precisely because grammars are large C files. — Source: https://deepwiki.com/ast-grep/ast-grep/4-language-support ("Languages are conditionally compiled using Cargo features to reduce binary size"). For three languages this is optional; flag it, don't build it yet (YAGNI). + +## 2. Best Practices + +- **Per-language config table over scattered conditionals.** ast-grep's core abstraction is a `Language` trait exposing `from_path` (extension → language detection), `kind_to_id`, and `field_to_id` — everything downstream is written against the trait, never against concrete grammars. — Source: https://docs.rs/ast-grep-language/latest/ast_grep_language/trait.Language.html . Idiomatic translation for oot at our scale: a small `Lang` enum with a static table: `{ extensions, ts_language fn, function_kinds: &[&str], name_field_strategy }`. This replaces both `.ends_with(".rs")` filters and the hardcoded kind string. +- **Extension-based detection is the industry norm.** ast-grep's `SupportLang` enum carries aliases + file extensions for automatic detection (DeepWiki, same page). GitHub linguist does the same. Nothing fancier is warranted for v1. +- **Reuse one `Parser`, switch languages per file** OR hold one parser per language. `set_language` is designed for exactly this and validates ABI each call (docs.rs). Per-language parser construction in `Engine::new()` (a HashMap) avoids repeated validation and keeps `diff_snapshots`' hot loop clean. Either is defensible; per-parser-map is the simpler mental model. +- **Node-kind sets, not single kinds.** Rust: just `function_item`. Go: `function_declaration` AND `method_declaration` (both carry field `name` — verified in src/grammar.json of tree-sitter-go). JS: `function_declaration`, `function_expression`, `generator_function_declaration`, `generator_function`, `method_definition` (all named-node kinds from node-types.json). A `&[&str]` per language covers this without a query DSL. + +## 3. Pitfalls & Language Quirks + +- **Method-name collision semantics differ per language and are currently wrong-ish for Rust too.** Today, `impl Foo { fn run }` and `impl Bar { fn run }` map to the same key `"run"` — a false 3-way conflict. In Go, `method_declaration`'s name field is a `_field_identifier` scoped to a receiver (grammar.json); correct identity is `Type.name` (receiver type from the `receiver` field). In JS, `method_definition` names can be `computed_property_name` or string literals (node-types.json) — stringify what's there, skip truly dynamic ones. Decision needed: minimum viable fix is prefixing Rust method names with their enclosing `impl` type and Go ones with the receiver type; JS `method_definition` gets its property text. +- **Arrow functions assigned to variables:** `const f = () => {}` puts the name on the `variable_declarator`, not the `arrow_function`. v1 call: track only named forms + variable-assigned arrows/functions via parent lookup one level up; anything deeper (object literal properties, default exports) waits until there's a failing case (YAGNI). +- **`.rs`-style filtering duplicated in two methods** (lines 37 and 130): replace both with the same `Lang::from_path(path)` helper or the two will drift. +- **Row numbers:** `start_position().row + 1` is already correct and language-independent; byte-offset `utf8_text` handles UTF-8 fine. No change needed. +- **Version note:** findings apply to `tree-sitter = "0.23"` with same-generation grammar crates (Rust 0.23.x; Go/JS pinned to releases whose generated ABI ∈ [13,14] — exact crate versions to be locked by the CI set_language test, not by assumption). + +## 4. Differentiation + +- **Industry standard:** ast-grep/semgrep-class tools ship dozens of languages behind feature flags with trait-based language abstraction, query DSLs, and dynamic loading. +- **Our approach:** three languages, one static table, no DSL, no dynamic loading, no features. Function-level identity with receiver-aware naming (which even some diff tools get wrong). +- **Is the difference useful?** Yes-with-a-caveat. The difference is *restraint*, which serves oot's actual job (adjudicate changes, not search code). The genuinely differentiated piece is folding method receivers into function identity — that fixes a live correctness bug in the 3-way conflict detector, not just adds languages. If we later need 10+ languages, copy ast-grep's feature-flag model then. + +## Recommendation + +Build in this order: + +1. `Lang` enum + static config table (extensions, grammar, function kinds) in the engine module; `Engine::new()` builds a parser per language; CI/unit test asserts `set_language` succeeds for all three (ABI tripwire). +2. Replace both `.rs` filters with `Lang::from_path`; unknown extensions keep being skipped silently. +3. Generalize `collect()`: match any kind in the language's function-kinds list; read `name` field; add receiver-prefix logic (Go `Type.name`, Rust `ImplType::name`, JS `method_definition` property). +4. Parse-health check: root-level `ERROR` scan → Review dispute instead of silent garbage. +5. Fixtures + integration tests: Go fixture pair (func + method rename across branches) and JS fixture pair (named function + const-arrow), driving both `diff_snapshots` and `diff_3way`. +6. README status checkboxes updated. + +Explicitly out of scope: TSX/TS, Python, feature flags, query DSL, anonymous-function heuristics beyond one-level parent lookup. + +## Sources +- https://github.com/tree-sitter/tree-sitter-rust/issues/273 (ABI mismatch failure mode) +- https://tree-sitter-tree-sitter.mintlify.app/using-parsers/abi-versions (ABI compat table, 0.23 ↔ ABI 13–14) +- https://docs.rs/tree-sitter/latest/tree_sitter/struct.Parser.html (set_language contract) +- https://docs.rs/tree-sitter/latest/tree_sitter/constant.LANGUAGE_VERSION.html +- https://docs.rs/crate/tree-sitter-rust/0.23.0 (grammar↔runtime dependency shape) +- https://github.com/tree-sitter/tree-sitter-go + src/grammar.json (function_declaration/method_declaration, name fields) +- https://docs.rs/tree-sitter-go/latest/tree_sitter_go/constant.NODE_TYPES.html +- https://raw.githubusercontent.com/tree-sitter/tree-sitter-javascript/master/src/node-types.json (JS kinds, anonymous functions, export_statement wrapping) +- https://deepwiki.com/ast-grep/ast-grep/4-language-support (feature flags, extension detection) +- https://docs.rs/ast-grep-language/latest/ast_grep_language/trait.Language.html (Language trait / from_path pattern) +- https://ast-grep.github.io/advanced/core-concepts (kinds vs fields) + +## Adversarial Verification + +Findings from three adversarial passes and how the plan changed: + +1. **Receiver-prefixed method identity → CUT.** Prefixing (`Type.name`) fixes today's bare-name collision but breaks key stability: moving a fn between impl blocks or renaming an impl turns a no-op into remove+add dispute pairs, and in `diff_3way` can produce a false `Severity::High` conflict that flips the verdict to Blocked under default `block_on: ["high"]` policy (src/policy.rs:21). Also unhandled: Go pointer receivers (`(t *T)` vs `(t T)`), Rust trait-impl ambiguity, generic type text instability, JS multi-class same-method-name. Verdict: trades one false-positive class for a worse one on the blocking path. Bare names stay; collision documented as known limitation. +2. **ERROR-node scan → CUT to future work.** Mechanically fine but contradicts the tested graceful-degradation contract (`test_engine_syntax_error_handling`, tests/engine_test.rs:176): tree-sitter recovery already handles garbage input, so an ERROR scan double-reports the same root cause and pollutes dockets for vendored/generated files with stray ERROR nodes. +3. **Exact-match test pins survive** only if free functions keep bare names — confirmed by review of tests/engine_test.rs:39,81,88 and cli_test.rs live-engine assertions. With prefixing cut, this is moot but recorded as the reason it would have broken. +4. **Scope confirmed:** all Rust-only assumptions live in engine/mod.rs only (language field :13, :19; `.rs` filters :37, :130; kind string :324). Adapters/policy/docket consume disputes generically. Only stale item: `test_engine_non_rust_file_filtering` premise wording. +5. **Crate claims verified against crates.io/docs.rs (live):** tree-sitter-go has 0.23.0–0.23.4 releases; tree-sitter-javascript has 0.23.0–0.23.1; both export `LANGUAGE: LanguageFn`; pinned docs.rs/tree-sitter/0.23.2 shows LANGUAGE_VERSION=14 / MIN_COMPATIBLE_LANGUAGE_VERSION=13. Pinning `tree-sitter-go = "0.23"`, `tree-sitter-javascript = "0.23"` alongside the existing runtime 0.23 is safe. + +Status: GREEN (issues found were fixed by cutting items 1–2 from scope rather than reworking them). + +## Post-Review Round (interrogate, 4 reviewers) + +A second adversarial pass on the implemented branch found four consensus issues, all fixed in the follow-up commit: + +1. **Arrow/anonymous JS functions invisible** — the "variable-assigned via parent lookup" scoped in this doc's §Recommendation was dropped during implementation without being recorded as a cut. Fixed: `arrow_function` added to kinds; anonymous matches inherit the enclosing `variable_declarator`'s name. +2. **`from_path` dotless regression** — `rsplit('.').next()` matched extension-less files named literally `go`/`js`/`rs` (the old `.ends_with(".rs")` required a dot). Fixed with `rsplit_once('.')` + lowercase normalization. +3. **Silent unreachable guards** — `get_mut(&lang) else { continue }` could have masked future registration drift. Dissolved entirely by restructuring. +4. **Bare-name collision = silent data loss, not noise** — last-wins insert made diffs direction-dependent (editing the first same-named method produced zero disputes). Fixed: first occurrence wins and an explicit Review-severity ambiguity dispute is emitted. + +Also applied from reviewer pushback: Engine stores `tree_sitter::Language` (validated in `new()`) instead of live `Parser`s, restoring `&self` on both diff methods and reverting the `&mut` ripple through adapters/main/tests; dispute numbering now sorts function names in `diff_snapshots` (matching `diff_3way`) so saved dockets are deterministic; `collect` no longer recurses into matched functions (kills nested double-reporting); README documents the ambiguity limitation. Note for the record: one reviewer claimed `Language` is `Copy` — it is not; it borrows. diff --git a/Cargo.toml b/Cargo.toml index 54c73d5..1ae9ff7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -13,6 +13,6 @@ toml = "0.8" anyhow = "1" tree-sitter = "0.23" tree-sitter-rust = "0.23" -tree-sitter-python = "0.23" -tree-sitter-javascript = "0.23" tree-sitter-go = "0.23" +tree-sitter-javascript = "0.23" +tree-sitter-python = "0.23" diff --git a/README.md b/README.md index ddb960b..96efdbf 100644 --- a/README.md +++ b/README.md @@ -63,28 +63,42 @@ If a dispute crosses policy, Oot blocks the change or cloaks the private parts. ## Status -We are early. The current code is a seed: a Rust structural-diff engine, a dispute type, a policy loader, and a docket format. None of it yet speaks the Change model end to end. The original five minute pitch was a semantic merge-conflict checker. We are building the governance platform instead, so the build order leads with visibility: +Working seed — the engine runs, the docket renders, and git + Jujutsu ingestion are in-memory. Current focus: using Oot to govern Oot's own changes. -- [ ] Change ingestion from git and Jujutsu snapshots -- [ ] Visibility policy: private paths, private branches, embargo schedules (the governance spine) -- [ ] Meaning disputes from the structural engine plus a hosted intent check -- [ ] Docket format with visibility and embargo state -- [ ] In-memory execution path (no materialized tree) -- [ ] git and Jujutsu adapters, plus a hosted model API for intent +- [x] Change ingestion from git snapshots (in-memory via `git ls-tree`/`cat-file`) and materialized dirs +- [x] Jujutsu ingestion (in-memory via `jj file list`/`file show`, revset resolution, first-class conflict detection) +- [x] Visibility policy: private paths, private branches, embargo schedules (the governance spine) +- [x] Meaning disputes from the structural engine (tree-sitter: Rust, Go, JavaScript) +- [x] Docket format with visibility and embargo state (JSON/TOML + render) +- [x] In-memory execution path (no materialized tree required for git) +- [x] Git adapter with 3-way adjudication +- [x] Jujutsu adapter with 3-way adjudication (`--source jj`, revsets accepted) -## Open source and the model +**Known limitation:** functions that share a bare name within one file (e.g., a `render` method on two classes, or same-named Go methods on two types) are tracked by first occurrence only; the docket flags them as ambiguous rather than tracking each definition separately. -The adjudication runtime, the docket format, and the adapters are MIT licensed. The hosted model that scores intent and runs embargo distribution will be a paid service. A court that hides its deliberations is not a court, so the gate stays open. +## License -## Try it +The adjudication runtime, the docket format, and the adapters are MIT licensed. A court that hides its deliberations is not a court, so the gate stays open. + +## Someday + +Deliberately unbuilt. These need users to be worth their cost, and there are none yet. -The runtime is not buildable to this shape yet. When it is: +- **Hosted intent scoring** — a model that checks what a change claims to mean against what it actually does. The structural engine catches *that* code changed; this would catch *what it means*. Needs a server, a model, and someone paying for both. +- **Embargo distribution** — the courier half of embargo: quietly shipping held patches to maintainers before the public diff drops. Needs keyed private channels and maintainer auth. The detection half already ships. + +## Try it ```bash git clone https://github.com/Epoch-AI-Lab/oot.git cd oot cargo build --release -./target/release/oot adjudicate --change feature/auth-refactor +# materialized dirs +./target/release/oot adjudicate --change feature/auth-refactor --base fixtures/repo/base --head fixtures/repo/head --visibility fixtures/visibility.toml +# or 3-way git (in-memory, no checkout) +./target/release/oot adjudicate --change feature/auth-refactor --base-ref main --head-ref feature/auth --repo . +# or 3-way jujutsu (revsets welcome) +./target/release/oot adjudicate --source jj --change greet --base-ref 'bookmarks(exact:main)' --head-ref '@-' ``` ## Contribute diff --git a/TODO.md b/TODO.md new file mode 100644 index 0000000..e3dbc6c --- /dev/null +++ b/TODO.md @@ -0,0 +1,11 @@ +# Known friction + +## ~~Fixture `.env` policy noise~~ RESOLVED 2026-08-21 + +Originally `VisibilityPolicy::check` flagged private-path fragments against +*every* file in the head snapshot, so the intentional `.env` fixture cloaked +every change. Fixed by aligning the code with its own documented contract: +only paths *touched* by a change (added, removed, or content-modified vs +base) are checked. See `test_visibility_policy_only_flags_touched_private_paths`. + +No open items. diff --git a/src/adapter/jj.rs b/src/adapter/jj.rs new file mode 100644 index 0000000..48822fb --- /dev/null +++ b/src/adapter/jj.rs @@ -0,0 +1,351 @@ +//! Native Jujutsu (jj) adapter for extracting in-memory snapshots and adjudicating 3-way merges. +//! +//! Shells out to the `jj` binary rather than linking `jj-lib`, because the library +//! crate is internal to the jj CLI and its API is unstable. All calls are read-only +//! and pass `--ignore-working-copy` so adjudication never snapshots or mutates the +//! caller's working copy. + +use crate::change::{Change, Snapshot, Source}; +use crate::dispute::{Dispute, Docket, Kind, Severity, Verdict}; +use crate::engine::Engine; +use crate::policy::MeaningPolicy; +use crate::visibility::VisibilityPolicy; +use anyhow::{anyhow, Context, Result}; +use std::collections::HashMap; +use std::path::{Path, PathBuf}; +use std::process::Command; + +/// Adapter for interacting directly with a Jujutsu repository. +#[derive(Debug, Clone)] +pub struct JjAdapter { + repo_root: PathBuf, +} + +/// Configuration options for 3-way Jujutsu merge adjudication. +#[derive(Debug, Default, Clone)] +pub struct JjAdjudicateOptions { + /// Explicit override for the common ancestor commit ID (revset). + pub custom_ancestor: Option, + /// Custom identifier or name for the change. + pub change_name: Option, + /// Declared intent or purpose of the change. + pub intent: Option, +} + +/// Shorten a commit ID for display (commit IDs are ASCII hex, so slicing is safe). +fn short(id: &str) -> &str { + &id[..id.len().min(7)] +} + +impl JjAdapter { + /// Create a new `JjAdapter` rooted at the Jujutsu workspace containing `repo_path`. + pub fn new(repo_path: impl AsRef) -> Result { + let path = repo_path.as_ref(); + let output = Command::new("jj") + .args(["--ignore-working-copy", "--no-pager", "root"]) + .current_dir(path) + .output() + .with_context(|| { + format!( + "Failed to run jj in {} (is Jujutsu installed?)", + path.display() + ) + })?; + + if !output.status.success() { + return Err(anyhow!( + "Not a valid jj repository at {}: {}", + path.display(), + String::from_utf8_lossy(&output.stderr).trim() + )); + } + + let root_str = String::from_utf8(output.stdout)?.trim().to_string(); + Ok(Self { + repo_root: PathBuf::from(root_str), + }) + } + + /// Discover a `JjAdapter` from the current working directory. + pub fn discover() -> Result { + Self::new(".") + } + + /// Returns the absolute path to the repository root. + pub fn repo_root(&self) -> &Path { + &self.repo_root + } + + /// Run a read-only jj command and return its stdout. + fn run(&self, args: &[&str]) -> Result { + let mut full: Vec<&str> = vec!["--ignore-working-copy", "--no-pager", "--quiet"]; + full.extend_from_slice(args); + + let output = Command::new("jj") + .args(&full) + .current_dir(&self.repo_root) + .output() + .with_context(|| format!("Failed to run jj {:?}", args))?; + + if !output.status.success() { + return Err(anyhow!( + "jj {:?} failed: {}", + args, + String::from_utf8_lossy(&output.stderr).trim() + )); + } + + Ok(String::from_utf8_lossy(&output.stdout).into_owned()) + } + + /// Resolve a revset to exactly one commit ID. + /// + /// Revset symbols resolve by priority (tag, then bookmark, then commit/change ID), + /// so callers passing ambiguous input should wrap it (e.g. `bookmarks(exact:name)` + /// or `commit_id(prefix)`) before calling this. + pub fn resolve_commit_id(&self, revset: &str) -> Result { + let out = self.run(&[ + "log", + "--no-graph", + "-T", + "commit_id ++ \"\\n\"", + "-r", + revset, + ])?; + + let ids: Vec<&str> = out + .lines() + .map(str::trim) + .filter(|l| !l.is_empty()) + .collect(); + match ids.len() { + 0 => Err(anyhow!("jj revset '{revset}' matched no commits")), + 1 => Ok(ids[0].to_string()), + n => Err(anyhow!( + "jj revset '{revset}' matched {n} commits; expected exactly one" + )), + } + } + + /// Resolve a revset to exactly one change ID (stable across history rewrites). + pub fn resolve_change_id(&self, revset: &str) -> Result { + let out = self.run(&[ + "log", + "--no-graph", + "-T", + "change_id ++ \"\\n\"", + "-r", + revset, + ])?; + + let ids: Vec<&str> = out + .lines() + .map(str::trim) + .filter(|l| !l.is_empty()) + .collect(); + match ids.len() { + 0 => Err(anyhow!("jj revset '{revset}' matched no commits")), + 1 => Ok(ids[0].to_string()), + n => Err(anyhow!( + "jj revset '{revset}' matched {n} commits; expected exactly one" + )), + } + } + + /// Compute the common ancestor commit ID between two commits. + /// + /// Uses the revset `heads(::a & ::b)` (the fork point). Criss-cross histories can + /// yield multiple fork points; this errors in that case and the caller should + /// supply [`JjAdjudicateOptions::custom_ancestor`] explicitly. + pub fn ancestor(&self, commit_a: &str, commit_b: &str) -> Result { + let revset = format!("heads(::{commit_a} & ::{commit_b})"); + self.resolve_commit_id(&revset).with_context(|| { + format!( + "Failed to compute common ancestor between '{commit_a}' and '{commit_b}'; \ + pass an explicit ancestor with --merge-base" + ) + }) + } + + /// Extract commit authors across the range `ancestor..head`. + pub fn authors(&self, ancestor: &str, head: &str) -> Result> { + let revset = format!("{ancestor}..{head}"); + let out = match self.run(&[ + "log", + "--no-graph", + "-T", + "author.name() ++ \"\\n\"", + "-r", + &revset, + ]) { + Ok(o) => o, + Err(_) => return Ok(Vec::new()), + }; + + let mut authors: Vec = out + .lines() + .map(|l| l.trim().to_string()) + .filter(|l| !l.is_empty()) + .collect(); + authors.sort(); + authors.dedup(); + Ok(authors) + } + + /// Extract an in-memory `Snapshot` from a revision without touching the working copy. + /// + /// Files in a conflicted state are excluded from the snapshot (their materialized + /// text is conflict markers, not real content); they are returned separately so the + /// caller can raise a dispute instead of feeding markers to the parser. + pub fn extract_snapshot_with_conflicts(&self, rev: &str) -> Result<(Snapshot, Vec)> { + let listing = self.run(&["file", "list", "-r", rev])?; + + let mut files = HashMap::new(); + let mut conflicted = Vec::new(); + + for path in listing.lines().map(str::trim).filter(|l| !l.is_empty()) { + let content = self.run(&["file", "show", "-r", rev, "--", path])?; + if content + .lines() + .any(|l| l.starts_with("<<<<<<<") && l.contains("conflict")) + { + conflicted.push(path.to_string()); + } else { + files.insert(path.to_string(), content); + } + } + + Ok((Snapshot { files }, conflicted)) + } + + /// Extract an in-memory `Snapshot` from a revision, dropping conflicted files. + pub fn extract_snapshot(&self, rev: &str) -> Result { + Ok(self.extract_snapshot_with_conflicts(rev)?.0) + } + + /// Adjudicate a 3-way merge between `base_ref` and `head_ref`. + /// + /// Resolves both revsets to commit IDs, computes the common ancestor (or uses + /// `options.custom_ancestor`), extracts the three snapshots in memory, runs + /// structural analysis plus visibility policy, and produces a finalized [`Docket`]. + pub fn adjudicate_3way( + &self, + base_ref: &str, + head_ref: &str, + engine: &Engine, + meaning_policy: &MeaningPolicy, + visibility_policy: &VisibilityPolicy, + options: &JjAdjudicateOptions, + ) -> Result { + let base_id = self.resolve_commit_id(base_ref)?; + let head_id = self.resolve_commit_id(head_ref)?; + + let ancestor_id = match &options.custom_ancestor { + Some(a) => self.resolve_commit_id(a)?, + None => self.ancestor(&base_id, &head_id)?, + }; + + let (ancestor_snapshot, _) = self.extract_snapshot_with_conflicts(&ancestor_id)?; + let (base_snapshot, _) = self.extract_snapshot_with_conflicts(&base_id)?; + let (head_snapshot, head_conflicts) = self.extract_snapshot_with_conflicts(&head_id)?; + + let mut authors = self.authors(&ancestor_id, &head_id)?; + if authors.is_empty() { + authors = vec!["@jj-author".to_string()]; + } + + let change_label = options + .change_name + .clone() + .unwrap_or_else(|| format!("{base_ref}..{head_ref}")); + + let change = Change { + name: change_label, + source: Source::Jj, + base_ref: format!("{base_ref}@{}", short(&base_id)), + head_ref: format!("{head_ref}@{}", short(&head_id)), + base: ancestor_snapshot.clone(), + head: head_snapshot.clone(), + authors: authors.clone(), + intent: options.intent.clone(), + }; + + let mut disputes = engine.diff_3way(&ancestor_snapshot, &base_snapshot, &head_snapshot)?; + + // Conflicted files carry logical conflicts jj materializes as markers; + // raise them directly rather than parsing marker text as source. + for path in &head_conflicts { + let id = format!("D{:03}", disputes.len() + 1); + disputes.push(Dispute { + id, + location: path.clone(), + kind: Kind::Meaning, + severity: Severity::High, + detail: format!( + "conflicted file `{}` carries unresolved merge conflict; resolve before merge", + path + ), + }); + } + + let vis_disputes = visibility_policy.check(&change); + let cloaked = vis_disputes + .iter() + .any(|d| d.kind == Kind::Visibility && d.severity == Severity::High); + disputes.extend(vis_disputes); + + let verdict = if cloaked { + Verdict::Cloaked + } else if visibility_policy.embargo_until.is_some() { + Verdict::Embargoed + } else { + meaning_policy.evaluate(&disputes) + }; + + let mut touched_paths: Vec = base_snapshot + .files + .keys() + .chain(head_snapshot.files.keys()) + .filter(|p| base_snapshot.files.get(*p) != head_snapshot.files.get(*p)) + .cloned() + .collect(); + touched_paths.sort(); + touched_paths.dedup(); + + let scope = if touched_paths.is_empty() { + "no files changed".to_string() + } else { + touched_paths.join(", ") + }; + + let docket = Docket { + change: change.name, + source: format!( + "jj: {} (base) vs {} (head)", + short(&ancestor_id), + short(&head_id) + ), + base: change.base_ref, + head: change.head_ref, + disputes, + scope, + authors, + verdict, + embargo: visibility_policy.embargo_note(), + }; + + Ok(docket) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_short_truncates_commit_id() { + assert_eq!(short("0123456789abcdef"), "0123456"); + assert_eq!(short("abc"), "abc"); + assert_eq!(short(""), ""); + } +} diff --git a/src/adapter/mod.rs b/src/adapter/mod.rs index d59aee2..c180ee8 100644 --- a/src/adapter/mod.rs +++ b/src/adapter/mod.rs @@ -1,5 +1,7 @@ //! VCS adapters for extracting snapshots directly from version control systems. pub mod git; +pub mod jj; pub use git::{GitAdapter, GitAdjudicateOptions}; +pub use jj::{JjAdapter, JjAdjudicateOptions}; diff --git a/src/engine/language.rs b/src/engine/language.rs index 71f64d6..843aafe 100644 --- a/src/engine/language.rs +++ b/src/engine/language.rs @@ -6,6 +6,12 @@ //! its callable on the `value` field of a `variable_declarator` and its name //! on the declarator itself. That relationship is captured by //! [`LangConfig::wrapped_functions`]. +//! +//! Function map keys are always bare names. Receiver- or impl-qualified keys +//! (`(*T).name`, `(A).name`) were tried and rejected: they make keys unstable +//! under refactoring, so moving a function between impl blocks fabricates +//! High-severity 3-way conflicts (and false Blocked verdicts). Same-named +//! functions are reported as ambiguous instead — see `Engine::diff_snapshots`. use tree_sitter::{Language, Node}; @@ -16,25 +22,11 @@ const CALLABLE_KINDS: &[&str] = &[ "generator_function", ]; -/// How to disambiguate a function's map key when bare names can collide. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum Qualifier { - /// Key is the bare function name. - None, - /// Prefix with the receiver type, e.g. Go methods become `(*T).name`. - Receiver, - /// Prefix with the enclosing impl block, e.g. Rust methods become - /// `(A).name` or `(Trait for A).name`. - EnclosingImpl, -} - /// A directly named function or method node kind. #[derive(Debug, Clone, Copy)] pub struct FunctionKind { /// The node kind, e.g. `"function_item"` or `"method_declaration"`. pub node_kind: &'static str, - /// How to qualify the key when names could collide. - pub qualifier: Qualifier, } /// A wrapper node that carries a callable in one field and the function's @@ -80,42 +72,13 @@ impl LangConfig { .is_some_and(|(_, ext)| self.extensions.contains(&ext.to_ascii_lowercase().as_str())) } - /// The map key for a directly named function node, qualified per - /// `kind.qualifier` so same-named functions stay distinct. - pub fn function_key(&self, kind: &FunctionKind, node: Node, source: &str) -> Option { + /// The map key for a directly named function node: its `name` field text. + pub fn function_key(&self, _kind: &FunctionKind, node: Node, source: &str) -> Option { let name_node = node.child_by_field_name("name")?; - let name = name_node.utf8_text(source.as_bytes()).ok()?.to_string(); - match kind.qualifier { - Qualifier::None => Some(name), - Qualifier::Receiver => { - let receiver = node.child_by_field_name("receiver")?; - let mut cursor = receiver.walk(); - let decl = receiver - .children(&mut cursor) - .find(|c| c.kind() == "parameter_declaration")?; - let ty = decl.child_by_field_name("type")?; - let ty_text = ty.utf8_text(source.as_bytes()).ok()?; - Some(format!("({}).{}", ty_text, name)) - } - Qualifier::EnclosingImpl => { - let mut parent = node.parent(); - while let Some(anc) = parent { - if anc.kind() == "impl_item" { - let ty = anc.child_by_field_name("type")?; - let ty_text = ty.utf8_text(source.as_bytes()).ok()?; - let scope = match anc.child_by_field_name("trait") { - Some(t) => { - format!("{} for {}", t.utf8_text(source.as_bytes()).ok()?, ty_text) - } - None => ty_text.to_string(), - }; - return Some(format!("({}).{}", scope, name)); - } - parent = anc.parent(); - } - Some(name) - } - } + name_node + .utf8_text(source.as_bytes()) + .ok() + .map(str::to_string) } } @@ -128,7 +91,6 @@ pub fn registry() -> Vec { language: tree_sitter_rust::LANGUAGE.into(), function_kinds: &[FunctionKind { node_kind: "function_item", - qualifier: Qualifier::EnclosingImpl, }], wrapped_functions: &[], }, @@ -138,7 +100,6 @@ pub fn registry() -> Vec { language: tree_sitter_python::LANGUAGE.into(), function_kinds: &[FunctionKind { node_kind: "function_definition", - qualifier: Qualifier::None, }], wrapped_functions: &[], }, @@ -149,11 +110,9 @@ pub fn registry() -> Vec { function_kinds: &[ FunctionKind { node_kind: "function_declaration", - qualifier: Qualifier::None, }, FunctionKind { node_kind: "method_declaration", - qualifier: Qualifier::Receiver, }, ], wrapped_functions: &[], @@ -165,15 +124,12 @@ pub fn registry() -> Vec { function_kinds: &[ FunctionKind { node_kind: "function_declaration", - qualifier: Qualifier::None, }, FunctionKind { node_kind: "generator_function_declaration", - qualifier: Qualifier::None, }, FunctionKind { node_kind: "method_definition", - qualifier: Qualifier::None, }, ], wrapped_functions: &[ diff --git a/src/engine/mod.rs b/src/engine/mod.rs index 60dde94..a79893d 100644 --- a/src/engine/mod.rs +++ b/src/engine/mod.rs @@ -50,19 +50,39 @@ impl Engine { match (base_src, head_src) { (Some(b), Some(h)) => { - let base_fns = extract_functions( + let (base_fns, mut dupes) = extract_functions( parse_source(&mut parser, &config.language, b).as_ref(), b, config, ); - let head_fns = extract_functions( + let (head_fns, head_dupes) = extract_functions( parse_source(&mut parser, &config.language, h).as_ref(), h, config, ); - for (name, (h_src, h_row)) in &head_fns { + dupes.extend(head_dupes); + dupes.sort(); + dupes.dedup(); + + for name in &dupes { + disputes.push(meaning( + &mut n, + path, + 0, + format!( + "function `{name}` is defined multiple times in this file; tracked only by first occurrence" + ), + Severity::Review, + )); + } + + let mut added: Vec<(&String, usize)> = Vec::new(); + let mut names: Vec<&String> = head_fns.keys().collect(); + names.sort(); + for name in names { + let (h_src, h_row, _) = &head_fns[name]; match base_fns.get(name) { - Some((b_src, _)) if b_src != h_src => { + Some((b_src, _, _)) if b_src != h_src => { disputes.push(meaning( &mut n, path, @@ -72,28 +92,60 @@ impl Engine { )); } None => { - disputes.push(meaning( - &mut n, - path, - *h_row, - format!("added function `{}`", name), - Severity::Review, - )); + added.push((name, *h_row)); } _ => {} } } - for name in base_fns.keys() { - if !head_fns.contains_key(name) { + let mut removed: Vec<&String> = base_fns + .keys() + .filter(|name| !head_fns.contains_key(*name)) + .collect(); + removed.sort(); + + // Pair removals with additions of identical source text: + // that is a rename, not two separate changes. + let mut consumed = vec![false; added.len()]; + let mut leftover_removed: Vec<&String> = Vec::new(); + for old in &removed { + let old_sig = &base_fns[*old].2; + let found = added + .iter() + .enumerate() + .find(|(i, (new, _))| !consumed[*i] && &head_fns[*new].2 == old_sig); + if let Some((i, (new, _))) = found { + consumed[i] = true; + disputes.push(meaning( + &mut n, + path, + head_fns[*new].1, + format!("renamed function `{}` to `{}`", old, new), + Severity::Review, + )); + } else { + leftover_removed.push(old); + } + } + for (i, (new, row)) in added.iter().enumerate() { + if !consumed[i] { disputes.push(meaning( &mut n, path, - 0, - format!("removed function `{}`", name), + *row, + format!("added function `{}`", new), Severity::Review, )); } } + for name in leftover_removed { + disputes.push(meaning( + &mut n, + path, + 0, + format!("removed function `{}`", name), + Severity::Review, + )); + } } (Some(_), None) => { disputes.push(meaning( @@ -104,12 +156,17 @@ impl Engine { Severity::Review, )); } - (None, Some(_)) => { + (None, Some(h)) => { + let summary = file_function_summary( + parse_source(&mut parser, &config.language, h).as_ref(), + h, + config, + ); disputes.push(meaning( &mut n, path, 0, - "file added".to_string(), + format!("file added ({})", summary), Severity::Review, )); } @@ -151,21 +208,37 @@ impl Engine { match (b_file, o_file, t_file) { // File exists in all three (Some(b_src), Some(o_src), Some(t_src)) => { - let b_fns = extract_functions( + let (b_fns, mut dupes) = extract_functions( parse_source(&mut parser, &config.language, b_src).as_ref(), b_src, config, ); - let o_fns = extract_functions( + let (o_fns, o_dupes) = extract_functions( parse_source(&mut parser, &config.language, o_src).as_ref(), o_src, config, ); - let t_fns = extract_functions( + let (t_fns, t_dupes) = extract_functions( parse_source(&mut parser, &config.language, t_src).as_ref(), t_src, config, ); + dupes.extend(o_dupes); + dupes.extend(t_dupes); + dupes.sort(); + dupes.dedup(); + + for name in &dupes { + disputes.push(meaning( + &mut n, + path, + 0, + format!( + "function `{name}` is defined multiple times in this file; tracked only by first occurrence" + ), + Severity::Review, + )); + } let mut all_fn_names: Vec<&String> = b_fns .keys() @@ -175,17 +248,20 @@ impl Engine { all_fn_names.sort(); all_fn_names.dedup(); + let mut pending_added: Vec<(String, usize)> = Vec::new(); + let mut pending_removed: Vec = Vec::new(); + for name in all_fn_names { let b_fn = b_fns.get(name); let o_fn = o_fns.get(name); let t_fn = t_fns.get(name); - let b_body = b_fn.map(|(s, _)| s.as_str()); - let o_body = o_fn.map(|(s, _)| s.as_str()); - let t_body = t_fn.map(|(s, _)| s.as_str()); + let b_body = b_fn.map(|(s, _, _)| s.as_str()); + let o_body = o_fn.map(|(s, _, _)| s.as_str()); + let t_body = t_fn.map(|(s, _, _)| s.as_str()); let row = t_fn - .map(|(_, r)| *r) - .or_else(|| o_fn.map(|(_, r)| *r)) + .map(|(_, r, _)| *r) + .or_else(|| o_fn.map(|(_, r, _)| *r)) .unwrap_or(0); // If both matches base, unchanged @@ -197,22 +273,10 @@ impl Engine { if o_body == b_body && t_body != b_body { match (b_body, t_body) { (None, Some(_)) => { - disputes.push(meaning( - &mut n, - path, - row, - format!("incoming branch added function `{}`", name), - Severity::Low, - )); + pending_added.push((name.clone(), row)); } (Some(_), None) => { - disputes.push(meaning( - &mut n, - path, - row, - format!("incoming branch removed function `{}`", name), - Severity::Review, - )); + pending_removed.push(name.clone()); } (Some(_), Some(_)) => { disputes.push(meaning( @@ -278,6 +342,52 @@ impl Engine { } } } + + // Pair incoming removals with incoming additions of + // identical source text: a rename, not two changes. + pending_added.sort_by(|a, b| a.0.cmp(&b.0)); + pending_removed.sort(); + let mut consumed = vec![false; pending_added.len()]; + let mut leftover_removed: Vec = Vec::new(); + for old in &pending_removed { + let old_sig = &b_fns[old].2; + let found = pending_added + .iter() + .enumerate() + .find(|(i, (new, _))| !consumed[*i] && &t_fns[new].2 == old_sig); + if let Some((i, (new, row))) = found { + consumed[i] = true; + disputes.push(meaning( + &mut n, + path, + *row, + format!("incoming branch renamed function `{}` to `{}`", old, new), + Severity::Review, + )); + } else { + leftover_removed.push(old.clone()); + } + } + for (i, (new, row)) in pending_added.iter().enumerate() { + if !consumed[i] { + disputes.push(meaning( + &mut n, + path, + *row, + format!("incoming branch added function `{}`", new), + Severity::Low, + )); + } + } + for name in leftover_removed { + disputes.push(meaning( + &mut n, + path, + 0, + format!("incoming branch removed function `{}`", name), + Severity::Review, + )); + } } // File deleted in target, modified in incoming (Some(_), None, Some(_)) => { @@ -300,12 +410,17 @@ impl Engine { )); } // File added only in incoming - (None, None, Some(_)) => { + (None, None, Some(t)) => { + let summary = file_function_summary( + parse_source(&mut parser, &config.language, t).as_ref(), + t, + config, + ); disputes.push(meaning( &mut n, path, 0, - "incoming branch added file".to_string(), + format!("incoming branch added file ({})", summary), Severity::Low, )); } @@ -321,6 +436,10 @@ impl Engine { } } +/// Tracked functions keyed by name: source text, 1-based row, and a rename +/// signature (the body with its own name blanked out). +type FunctionMap = HashMap; + fn parse_source(parser: &mut Parser, language: &Language, source: &str) -> Option { parser.set_language(language).ok()?; parser.parse(source, None) @@ -338,29 +457,56 @@ fn meaning(n: &mut i32, path: &str, row: usize, detail: String, severity: Severi } } +/// Describe what a newly added file contains: how many tracked functions and +/// up to three names, so a docket reader knows what arrived without opening +/// the file. +fn file_function_summary(tree: Option<&Tree>, source: &str, config: &LangConfig) -> String { + let (fns, _) = extract_functions(tree, source, config); + let mut names: Vec<&String> = fns.keys().collect(); + names.sort(); + if names.is_empty() { + return "no functions detected".to_string(); + } + let count = names.len(); + let preview: Vec = names.iter().take(3).map(|s| s.to_string()).collect(); + let noun = if count == 1 { "function" } else { "functions" }; + if count > 3 { + format!("{} {}: {}, …", count, noun, preview.join(", ")) + } else { + format!("{} {}: {}", count, noun, preview.join(", ")) + } +} + +/// Extract tracked functions as `name -> (source text, 1-based row, rename +/// signature with the name blanked out)`, plus the +/// list of names defined more than once (only the first occurrence is kept). fn extract_functions( tree: Option<&Tree>, source: &str, config: &LangConfig, -) -> HashMap { +) -> (FunctionMap, Vec) { let mut map = HashMap::new(); - let Some(tree) = tree else { - return map; - }; - collect(tree.root_node(), source, &mut map, config); - map + let mut duplicates = Vec::new(); + if let Some(tree) = tree { + collect(tree.root_node(), source, &mut map, &mut duplicates, config); + } + (map, duplicates) } fn collect( node: Node, source: &str, - map: &mut HashMap, + map: &mut FunctionMap, + duplicates: &mut Vec, config: &LangConfig, ) { + let mut matched = false; for kind in config.function_kinds { if node.kind() == kind.node_kind { + matched = true; if let Some(key) = config.function_key(kind, node, source) { - insert(key, node, source, map); + let name_node = node.child_by_field_name("name"); + insert(key, node, name_node, source, map, duplicates); } } } @@ -368,6 +514,7 @@ fn collect( if node.kind() != wrapped.node_kind { continue; } + matched = true; let (Some(name_node), Some(body)) = ( node.child_by_field_name(wrapped.name_field), node.child_by_field_name(wrapped.body_field), @@ -381,23 +528,58 @@ fn collect( .utf8_text(source.as_bytes()) .unwrap_or("") .to_string(); - insert(key, body, source, map); + insert(key, body, None, source, map, duplicates); } } + // Do not recurse into nodes that yielded a function: nested definitions + // are covered by the enclosing function's source span. + if matched { + return; + } let mut cursor = node.walk(); for child in node.children(&mut cursor) { - collect(child, source, map, config); + collect(child, source, map, duplicates, config); } } -/// Record a named function under `key`, with `body` as its source. -fn insert(key: String, body: Node, source: &str, map: &mut HashMap) { +/// Record a named function under `key`, with `body` as its source. First +/// occurrence wins; later same-key definitions are reported as duplicates. +fn insert( + key: String, + body: Node, + name_node: Option, + source: &str, + map: &mut FunctionMap, + duplicates: &mut Vec, +) { if key.is_empty() { return; } - let src = body.utf8_text(source.as_bytes()).unwrap_or("").to_string(); - let row = body.start_position().row + 1; - map.insert(key, (src, row)); + match map.entry(key.clone()) { + std::collections::hash_map::Entry::Occupied(_) => duplicates.push(key), + std::collections::hash_map::Entry::Vacant(slot) => { + let src = body.utf8_text(source.as_bytes()).unwrap_or("").to_string(); + let row = body.start_position().row + 1; + // Signature: the body with its own name blanked, so two functions + // that differ only by what they are called compare equal and pair + // as a rename. + let signature = match name_node { + Some(n) + if n.start_byte() >= body.start_byte() && n.end_byte() <= body.end_byte() => + { + let rel_start = n.start_byte() - body.start_byte(); + let rel_end = n.end_byte() - body.start_byte(); + let mut sig = String::with_capacity(src.len()); + sig.push_str(&src[..rel_start]); + sig.push('\u{0}'); + sig.push_str(&src[rel_end..]); + sig + } + _ => src.clone(), + }; + slot.insert((src, row, signature)); + } + } } #[cfg(test)] @@ -421,7 +603,9 @@ mod tests { ); let disputes = eng.diff_snapshots(&base, &head).unwrap(); - assert_eq!(disputes.len(), 3); + // `hello` changed on both sides. `old_fn` -> `new_fn` have identical + // (empty) bodies apart from the name, so they pair as a rename. + assert_eq!(disputes.len(), 2); let details: Vec<&str> = disputes.iter().map(|d| d.detail.as_str()).collect(); assert!(details @@ -429,10 +613,7 @@ mod tests { .any(|d| d.contains("both sides changed `hello`"))); assert!(details .iter() - .any(|d| d.contains("added function `new_fn`"))); - assert!(details - .iter() - .any(|d| d.contains("removed function `old_fn`"))); + .any(|d| d.contains("renamed function `old_fn` to `new_fn`"))); } #[test] @@ -526,7 +707,7 @@ mod tests { .any(|d| d.contains("both sides changed `greet`"))); assert!(details .iter() - .any(|d| d.contains("both sides changed `(*counter).inc`"))); + .any(|d| d.contains("both sides changed `inc`"))); } #[test] @@ -628,7 +809,7 @@ mod tests { } #[test] - fn test_engine_go_method_collision_disambiguated() { + fn test_engine_go_method_collision_flagged_ambiguous() { let eng = Engine::new().unwrap(); let mut base = Snapshot::default(); @@ -644,12 +825,17 @@ mod tests { ); let disputes = eng.diff_snapshots(&base, &head).unwrap(); - assert_eq!(disputes.len(), 1); - assert_eq!(disputes[0].detail, "both sides changed `(*A).hit`"); + assert!( + disputes + .iter() + .any(|d| d.detail.contains("`hit`") && d.detail.contains("multiple times")), + "same-named methods must surface an ambiguity dispute, got {:?}", + disputes + ); } #[test] - fn test_engine_rust_impl_method_collision_disambiguated() { + fn test_engine_rust_impl_method_collision_flagged_ambiguous() { let eng = Engine::new().unwrap(); let mut base = Snapshot::default(); @@ -666,8 +852,41 @@ mod tests { ); let disputes = eng.diff_snapshots(&base, &head).unwrap(); - assert_eq!(disputes.len(), 1); - assert_eq!(disputes[0].detail, "both sides changed `(A).hit`"); + assert!( + disputes + .iter() + .any(|d| d.detail.contains("`hit`") && d.detail.contains("multiple times")), + "same-named impl methods must surface an ambiguity dispute, got {:?}", + disputes + ); + } + + #[test] + fn test_engine_impl_move_is_not_a_conflict() { + // Regression: receiver/impl-qualified keys made a pure refactor + // (function moved between impl blocks) + an unrelated in-place edit + // look like a High-severity 3-way conflict, flipping the verdict to + // Blocked. Bare-name keys keep identity stable under refactoring. + let eng = Engine::new().unwrap(); + + let base_src = "struct A; struct B;\nimpl A { fn run(&self) -> i32 { 42 } }\n"; + let ours_src = "struct A; struct B;\nimpl B { fn run(&self) -> i32 { 42 } }\n"; + let theirs_src = "struct A; struct B;\nimpl A { fn run(&self) -> i32 { 43 } }\n"; + + let snap = |s: &str| { + let mut x = Snapshot::default(); + x.files.insert("src/lib.rs".into(), s.into()); + x + }; + + let disputes = eng + .diff_3way(&snap(base_src), &snap(ours_src), &snap(theirs_src)) + .unwrap(); + assert!( + !disputes.iter().any(|d| d.severity == Severity::High), + "refactor + unrelated edit must not produce High conflicts, got {:?}", + disputes + ); } #[test] diff --git a/src/main.rs b/src/main.rs index 35dad8a..9d4c8d0 100644 --- a/src/main.rs +++ b/src/main.rs @@ -3,7 +3,7 @@ //! Adjudicates changes across snapshots against meaning and visibility policies. use clap::{Parser, Subcommand}; -use oot::adapter::{GitAdapter, GitAdjudicateOptions}; +use oot::adapter::{GitAdapter, GitAdjudicateOptions, JjAdapter, JjAdjudicateOptions}; use oot::change::{Change, Snapshot, Source}; use oot::dispute::{Docket, Kind, Severity, Verdict}; use oot::docket; @@ -42,7 +42,7 @@ enum Commands { /// Git head reference (e.g. `feature/auth` or commit SHA). #[arg(long)] head_ref: Option, - /// Explicit merge-base commit SHA/ref override for 3-way Git adjudication. + /// Explicit merge-base (git) or common-ancestor (jj) commit override for 3-way adjudication. #[arg(long)] merge_base: Option, /// Path to the Git repository root (defaults to discovering from current directory). @@ -69,7 +69,7 @@ enum Commands { }, } -fn main() -> anyhow::Result<()> { +fn main() -> anyhow::Result { let cli = Cli::parse(); match cli.command { Commands::Adjudicate { @@ -91,7 +91,7 @@ fn main() -> anyhow::Result<()> { if let Some(path) = docket { let d = docket::load(std::path::Path::new(&path))?; print!("{}", d.render()); - return Ok(()); + return Ok(std::process::ExitCode::SUCCESS); } let meaning_policy = match policy { @@ -104,34 +104,58 @@ fn main() -> anyhow::Result<()> { }; let eng = Engine::new()?; - // Git 3-way In-Memory Adjudication + // VCS 3-way In-Memory Adjudication (git or jj) if let (Some(b_ref), Some(h_ref)) = (base_ref, head_ref) { - let git_adapter = match repo { - Some(r) => GitAdapter::new(r)?, - None => GitAdapter::discover()?, - }; + let wants_jj = matches!(source.as_deref(), Some("jj") | Some("jujutsu")); - let options = GitAdjudicateOptions { - custom_merge_base: merge_base, - change_name: change, - intent, - }; + let doc = if wants_jj { + let jj_adapter = match repo { + Some(r) => JjAdapter::new(r)?, + None => JjAdapter::discover()?, + }; + + let options = JjAdjudicateOptions { + custom_ancestor: merge_base, + change_name: change, + intent, + }; - let doc = git_adapter.adjudicate_3way( - &b_ref, - &h_ref, - &eng, - &meaning_policy, - &visibility_policy, - &options, - )?; + jj_adapter.adjudicate_3way( + &b_ref, + &h_ref, + &eng, + &meaning_policy, + &visibility_policy, + &options, + )? + } else { + let git_adapter = match repo { + Some(r) => GitAdapter::new(r)?, + None => GitAdapter::discover()?, + }; + + let options = GitAdjudicateOptions { + custom_merge_base: merge_base, + change_name: change, + intent, + }; + + git_adapter.adjudicate_3way( + &b_ref, + &h_ref, + &eng, + &meaning_policy, + &visibility_policy, + &options, + )? + }; print!("{}", doc.render()); if let Some(out_path) = output { docket::save(&doc, std::path::Path::new(&out_path))?; } - return Ok(()); + return Ok(exit_code_for(doc.verdict)); } // Materialized Directory Snapshot Adjudication @@ -205,9 +229,23 @@ fn main() -> anyhow::Result<()> { if let Some(out_path) = output { docket::save(&docket, std::path::Path::new(&out_path))?; } + + Ok(exit_code_for(docket.verdict)) } } - Ok(()) +} + +/// Exit-code contract for `oot adjudicate`: +/// - `0`: verdict is `Adjudicated` — ship-ready. +/// - `1`: any other verdict (`Blocked`, `Cloaked`, `Embargoed`) — all mean +/// "do not ship yet", so CI and merge gates can treat any nonzero as a stop. +/// - `2`: usage error (reserved by the CLI parser paths). +fn exit_code_for(verdict: Verdict) -> std::process::ExitCode { + if verdict == Verdict::Adjudicated { + std::process::ExitCode::SUCCESS + } else { + std::process::ExitCode::FAILURE + } } /// Recursively read files in a directory into a HashMap of relative paths to contents. diff --git a/src/visibility.rs b/src/visibility.rs index a545aa2..bd20803 100644 --- a/src/visibility.rs +++ b/src/visibility.rs @@ -44,14 +44,25 @@ impl VisibilityPolicy { /// Evaluate visibility rules against a change. /// /// Emits a visibility dispute for: - /// - Every private path present in the head snapshot. + /// - Every private path *touched* by the change: present in the head + /// snapshot but absent from base, or with different content. Files that + /// already existed unchanged are not touched, even if they match a + /// private-path fragment. /// - Every private branch referenced by name or refs. pub fn check(&self, change: &Change) -> Vec { let mut out = Vec::new(); let mut n = 1; - // Check private paths - for path in change.head.files.keys() { + // Check private paths among files the change actually touches + for (path, head_content) in &change.head.files { + let touched = change + .base + .files + .get(path) + .is_none_or(|base_content| base_content != head_content); + if !touched { + continue; + } let private = self .private_paths .iter() @@ -168,4 +179,51 @@ mod tests { let disputes = default_policy.check(&change); assert!(disputes.is_empty()); } + + #[test] + fn test_visibility_policy_only_flags_touched_private_paths() { + let policy = VisibilityPolicy::default(); + + let mut base = Snapshot::default(); + // Private file already exists, unchanged by this change. + base.files.insert("secrets/key.pem".into(), "same".into()); + base.files.insert("src/lib.rs".into(), "fn a() {}".into()); + + let mut head = Snapshot::default(); + head.files.insert("secrets/key.pem".into(), "same".into()); + head.files.insert("src/lib.rs".into(), "fn b() {}".into()); + + let change = Change { + name: "feature/public".into(), + source: Source::Git, + base_ref: "main".into(), + head_ref: "feature/public".into(), + base, + head, + authors: vec!["@alice".into()], + intent: None, + }; + + let disputes = policy.check(&change); + assert!( + disputes.is_empty(), + "unchanged private files must not be flagged, got {:?}", + disputes + ); + + // Now modify the private file: it becomes touched and must flag. + let mut head2 = change.head.clone(); + head2 + .files + .insert("secrets/key.pem".into(), "rotated".into()); + + let change2 = Change { + head: head2, + ..change + }; + + let disputes = policy.check(&change2); + assert_eq!(disputes.len(), 1); + assert_eq!(disputes[0].location, "secrets/key.pem"); + } } diff --git a/tests/cli_test.rs b/tests/cli_test.rs index 060f61d..41c5016 100644 --- a/tests/cli_test.rs +++ b/tests/cli_test.rs @@ -27,7 +27,8 @@ fn test_cli_adjudicate_fixtures_repo() { .output() .expect("Failed to execute oot CLI"); - assert!(output.status.success()); + // CLOAKED verdict must exit nonzero (do-not-ship). + assert_eq!(output.status.code(), Some(1)); let stdout = String::from_utf8_lossy(&output.stdout); assert!(stdout.contains("OOT DOCKET")); @@ -83,7 +84,8 @@ fn test_cli_adjudicate_embargoed_clean() { .output() .expect("Failed to execute oot CLI"); - assert!(output.status.success()); + // EMBARGOED verdict must exit nonzero (held = do-not-ship). + assert_eq!(output.status.code(), Some(1)); let stdout = String::from_utf8_lossy(&output.stdout); assert!(stdout.contains("OOT DOCKET")); @@ -164,7 +166,8 @@ fn test_cli_custom_meaning_policy_and_temp_dirs() { .output() .expect("Failed to execute oot CLI"); - assert!(output.status.success()); + // BLOCKED verdict must exit nonzero. + assert_eq!(output.status.code(), Some(1)); let stdout = String::from_utf8_lossy(&output.stdout); assert!(stdout.contains("OOT DOCKET")); @@ -175,6 +178,83 @@ fn test_cli_custom_meaning_policy_and_temp_dirs() { let _ = std::fs::remove_dir_all(&temp_root); } +#[test] +fn test_cli_exit_code_zero_for_adjudicated() { + let bin = get_bin_path(); + let temp_root = std::env::temp_dir().join(format!("oot_cli_clean_{}", std::process::id())); + let base_dir = temp_root.join("base"); + let head_dir = temp_root.join("head"); + + std::fs::create_dir_all(&base_dir).unwrap(); + std::fs::create_dir_all(&head_dir).unwrap(); + + // Identical snapshots: nothing changed, no disputes, clean verdict. + std::fs::write(base_dir.join("lib.rs"), "fn ok() {}").unwrap(); + std::fs::write(head_dir.join("lib.rs"), "fn ok() {}").unwrap(); + + let output = Command::new(&bin) + .args([ + "adjudicate", + "--change", + "chore/noop", + "--source", + "git", + "--base", + base_dir.to_str().unwrap(), + "--head", + head_dir.to_str().unwrap(), + "--authors", + "@tester", + ]) + .output() + .expect("Failed to execute oot CLI"); + + assert_eq!( + output.status.code(), + Some(0), + "Adjudicated verdict must exit 0" + ); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!(stdout.contains("OOT DOCKET")); + assert!(stdout.contains("ADJUDICATED")); + + let _ = std::fs::remove_dir_all(&temp_root); +} + +#[test] +fn test_cli_repo_visibility_policy_flags_env() { + let bin = get_bin_path(); + + // The repo's own visibility.toml must flag any .env path as private, + // producing a CLOAKED verdict and a nonzero exit even for an otherwise + // empty diff. + let output = Command::new(&bin) + .args([ + "adjudicate", + "--change", + "test/policy-check", + "--source", + "git", + "--base", + "fixtures/repo/base", + "--head", + "fixtures/repo/head", + "--visibility", + "visibility.toml", + "--authors", + "@tester", + ]) + .output() + .expect("Failed to execute oot CLI"); + + assert_eq!(output.status.code(), Some(1)); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!(stdout.contains("CLOAKED")); + assert!(stdout.contains(".env")); + + let _ = (); +} + #[test] fn test_cli_missing_base_and_docket_fails() { let bin = get_bin_path(); diff --git a/tests/engine_test.rs b/tests/engine_test.rs index c66f815..12c6e6f 100644 --- a/tests/engine_test.rs +++ b/tests/engine_test.rs @@ -1,5 +1,5 @@ use oot::change::Snapshot; -use oot::dispute::{Dispute, Kind, Severity}; +use oot::dispute::{Kind, Severity}; use oot::engine::Engine; #[test] @@ -127,7 +127,7 @@ pub fn verify_signature() -> bool { } #[test] -fn test_engine_non_rust_file_filtering() { +fn test_engine_unsupported_extension_filtering() { let engine = Engine::new().expect("Failed to initialize engine"); let mut base = Snapshot::default(); @@ -168,7 +168,7 @@ fn test_engine_non_rust_file_filtering() { assert!( disputes.is_empty(), - "Non-Rust files should be filtered out from AST diffing" + "Files with unsupported extensions should be filtered out from AST diffing" ); } @@ -220,8 +220,8 @@ fn test_engine_file_added_and_removed() { assert_eq!(disputes.len(), 2); let added = disputes .iter() - .find(|d| d.detail == "file added") - .expect("File added dispute"); + .find(|d| d.detail == "file added (1 function: new_util)") + .expect("File added dispute with content summary"); assert_eq!(added.location, "src/new_module.rs:0"); let removed = disputes @@ -231,6 +231,81 @@ fn test_engine_file_added_and_removed() { assert_eq!(removed.location, "src/old_module.rs:0"); } +#[test] +fn test_engine_rename_is_not_remove_add() { + let engine = Engine::new().expect("Failed to initialize engine"); + + let mut base = Snapshot::default(); + base.files.insert( + "src/auth.rs".to_string(), + "fn verify_user(user: &str) -> bool { user.len() > 3 }".to_string(), + ); + + let mut head = Snapshot::default(); + head.files.insert( + "src/auth.rs".to_string(), + "fn check_user(user: &str) -> bool { user.len() > 3 }".to_string(), + ); + + let disputes = engine.diff_snapshots(&base, &head).expect("Diff failed"); + + assert_eq!( + disputes.len(), + 1, + "identical body under a new name is one rename, got {:?}", + disputes + ); + assert_eq!( + disputes[0].detail, + "renamed function `verify_user` to `check_user`" + ); +} + +#[test] +fn test_engine_3way_rename_is_not_conflict() { + let engine = Engine::new().expect("Failed to initialize engine"); + + let base_src = "fn handle(req: i32) -> i32 { req + 1 }"; + let ours_src = base_src; // target untouched + let theirs_src = "fn process(req: i32) -> i32 { req + 1 }"; // incoming renamed it + + let snap = |s: &str| { + let mut x = Snapshot::default(); + x.files.insert("src/lib.rs".to_string(), s.to_string()); + x + }; + + let disputes = engine + .diff_3way(&snap(base_src), &snap(ours_src), &snap(theirs_src)) + .expect("Diff failed"); + + assert_eq!(disputes.len(), 1); + assert_eq!( + disputes[0].detail, + "incoming branch renamed function `handle` to `process`" + ); +} + +#[test] +fn test_engine_added_file_summary_lists_functions() { + let engine = Engine::new().expect("Failed to initialize engine"); + + let base = Snapshot::default(); + let mut head = Snapshot::default(); + head.files.insert( + "src/newstuff.rs".to_string(), + "fn alpha() {}\nfn beta() {}\nfn gamma() {}\nfn delta() {}\n".to_string(), + ); + + let disputes = engine.diff_snapshots(&base, &head).expect("Diff failed"); + + assert_eq!(disputes.len(), 1); + assert_eq!( + disputes[0].detail, + "file added (4 functions: alpha, beta, delta, …)" + ); +} + #[test] fn test_engine_multiple_files_and_functions() { let engine = Engine::new().expect("Failed to initialize engine"); @@ -262,58 +337,292 @@ fn test_engine_multiple_files_and_functions() { } #[test] -fn test_engine_mixed_language_snapshot() { +fn test_engine_go_function_and_method_detection() { let engine = Engine::new().expect("Failed to initialize engine"); let mut base = Snapshot::default(); base.files.insert( - "app.py".to_string(), - "def greet(name):\n return f\"hi {name}\"\n".to_string(), + "store/store.go".to_string(), + r#" +package store + +func Greet(name string) string { + return "hello " + name +} + +func (s *Store) Name() string { + return s.title +} +"# + .to_string(), ); - base.files.insert( - "index.js".to_string(), - "const double = (x) => x * 2;\n".to_string(), + + let mut head = Snapshot::default(); + head.files.insert( + "store/store.go".to_string(), + r#" +package store + +func Greet(name string) string { + return "hey " + name +} + +func (s *Store) Name() string { + return s.title +} +"# + .to_string(), ); + + let disputes = engine.diff_snapshots(&base, &head).expect("Diff failed"); + + assert_eq!(disputes.len(), 1); + assert_eq!(disputes[0].detail, "both sides changed `Greet`"); +} + +#[test] +fn test_engine_go_3way_method_conflict() { + let engine = Engine::new().expect("Failed to initialize engine"); + + let base_src = r#" +package store + +func (s *Store) Total() int { + return s.count * s.price +} +"#; + let ours_src = r#" +package store + +func (s *Store) Total() int { + return s.count * s.price / s.divisor +} +"#; + let theirs_src = r#" +package store + +func (s *Store) Total() int { + return s.count * s.price + s.bonus +} +"#; + + let mut base = Snapshot::default(); + base.files + .insert("total.go".to_string(), base_src.to_string()); + let mut ours = Snapshot::default(); + ours.files + .insert("total.go".to_string(), ours_src.to_string()); + let mut theirs = Snapshot::default(); + theirs + .files + .insert("total.go".to_string(), theirs_src.to_string()); + + let disputes = engine + .diff_3way(&base, &ours, &theirs) + .expect("Diff failed"); + + assert_eq!(disputes.len(), 1); + assert_eq!(disputes[0].severity, Severity::High); + assert!(disputes[0].detail.contains("3-way conflict")); + assert!(disputes[0].detail.contains("`Total`")); +} + +#[test] +fn test_engine_javascript_function_detection() { + let engine = Engine::new().expect("Failed to initialize engine"); + + let mut base = Snapshot::default(); base.files.insert( - "server.go".to_string(), - "package main\n\nfunc greet(name string) string {\n\treturn \"hi \" + name\n}\n" - .to_string(), + "src/api.js".to_string(), + r#" +export function fetchUser(id) { + return { id }; +} + +class Client { + connect() { + return true; + } +} +"# + .to_string(), ); let mut head = Snapshot::default(); head.files.insert( - "app.py".to_string(), - "def greet(name):\n return f\"hello {name}\"\n".to_string(), + "src/api.js".to_string(), + r#" +export function fetchUser(id) { + return { id, includeProfile: true }; +} + +class Client { + connect(timeoutMs) { + return timeoutMs > 0; + } +} +"# + .to_string(), ); - head.files.insert( - "index.js".to_string(), - "const double = (x) => x * 3;\n".to_string(), + + let disputes = engine.diff_snapshots(&base, &head).expect("Diff failed"); + + assert_eq!(disputes.len(), 2); + assert!(disputes + .iter() + .any(|d| d.detail == "both sides changed `fetchUser`")); + assert!(disputes + .iter() + .any(|d| d.detail == "both sides changed `connect`")); +} + +#[test] +fn test_engine_javascript_const_arrow_detection() { + let engine = Engine::new().expect("Failed to initialize engine"); + + let mut base = Snapshot::default(); + base.files.insert( + "src/handler.js".to_string(), + r#" +const fetchUser = async (id) => { + return { id }; +}; +"# + .to_string(), ); + + let mut head = Snapshot::default(); head.files.insert( - "server.go".to_string(), - "package main\n\nfunc greet(name string) string {\n\treturn \"hello \" + name\n}\n" - .to_string(), + "src/handler.js".to_string(), + r#" +const fetchUser = async (id) => { + return { id, cached: false }; +}; +"# + .to_string(), ); let disputes = engine.diff_snapshots(&base, &head).expect("Diff failed"); - assert_eq!(disputes.len(), 3); + assert_eq!(disputes.len(), 1); + assert_eq!( + disputes[0].detail, "both sides changed `fetchUser`", + "arrow function bound to a const must be tracked under the binding name" + ); +} - let greets: Vec<&Dispute> = disputes - .iter() - .filter(|d| d.detail == "both sides changed `greet`") - .collect(); - assert_eq!(greets.len(), 2, "one greet change per language file"); - assert!( - greets.iter().any(|d| d.location.starts_with("app.py:")), - "python greet dispute should point into app.py" +#[test] +fn test_engine_javascript_3way_conflict() { + let engine = Engine::new().expect("Failed to initialize engine"); + + let base_src = r#" +export const formatPrice = (cents) => { + return `$${cents / 100}`; +}; +"#; + let ours_src = r#" +export const formatPrice = (cents) => { + return (cents / 100).toFixed(2); +}; +"#; + let theirs_src = r#" +export const formatPrice = (cents) => { + return `${cents / 100} EUR`; +}; +"#; + + let mut base = Snapshot::default(); + base.files + .insert("src/price.mjs".to_string(), base_src.to_string()); + let mut ours = Snapshot::default(); + ours.files + .insert("src/price.mjs".to_string(), ours_src.to_string()); + let mut theirs = Snapshot::default(); + theirs + .files + .insert("src/price.mjs".to_string(), theirs_src.to_string()); + + // Covers both the 3-way path for JavaScript and .mjs extension routing. + let disputes = engine + .diff_3way(&base, &ours, &theirs) + .expect("Diff failed"); + + assert_eq!(disputes.len(), 1); + assert_eq!(disputes[0].severity, Severity::High); + assert!(disputes[0].detail.contains("3-way conflict")); + assert!(disputes[0].detail.contains("`formatPrice`")); +} + +#[test] +fn test_engine_dotless_filename_is_not_source() { + let engine = Engine::new().expect("Failed to initialize engine"); + + // A file literally named `go` with no extension must not be parsed as Go. + let mut base = Snapshot::default(); + base.files + .insert("tools/go".to_string(), "func NotReally() {}".to_string()); + + let mut head = Snapshot::default(); + head.files.insert( + "tools/go".to_string(), + "func DefinitelyChanged() {}".to_string(), ); + + let disputes = engine.diff_snapshots(&base, &head).expect("Diff failed"); + assert!( - greets.iter().any(|d| d.location.starts_with("server.go:")), - "go greet dispute should point into server.go" + disputes.is_empty(), + "extension-less files must be skipped, got {:?}", + disputes ); +} - assert!(disputes - .iter() - .any(|d| d.detail == "both sides changed `double`")); +#[test] +fn test_engine_duplicate_function_names_flagged() { + let engine = Engine::new().expect("Failed to initialize engine"); + + let mut base = Snapshot::default(); + base.files.insert( + "types.go".to_string(), + r#" +package types + +func (a A) Name() string { + return "A" +} + +func (b B) Name() string { + return "B" +} +"# + .to_string(), + ); + + let mut head = Snapshot::default(); + head.files.insert( + "types.go".to_string(), + r#" +package types + +func (a A) Name() string { + return "A-changed" +} + +func (b B) Name() string { + return "B" +} +"# + .to_string(), + ); + + let disputes = engine.diff_snapshots(&base, &head).expect("Diff failed"); + + // Regression: last-wins overwrite used to hide A's change entirely. + assert!( + disputes + .iter() + .any(|d| d.detail.contains("`Name`") && d.detail.contains("multiple times")), + "duplicate name must surface an ambiguity dispute, got {:?}", + disputes + ); } diff --git a/tests/jj_adapter_test.rs b/tests/jj_adapter_test.rs new file mode 100644 index 0000000..9e89136 --- /dev/null +++ b/tests/jj_adapter_test.rs @@ -0,0 +1,238 @@ +//! Integration tests for the Jujutsu adapter. +//! +//! These run against hermetic `jj` repositories created in the system temp dir. +//! They skip (pass trivially) when the `jj` binary is not installed. + +use oot::adapter::{JjAdapter, JjAdjudicateOptions}; +use oot::engine::Engine; +use oot::policy::MeaningPolicy; +use oot::visibility::VisibilityPolicy; +use std::path::{Path, PathBuf}; +use std::process::Command; +use std::sync::atomic::{AtomicUsize, Ordering}; + +static COUNTER: AtomicUsize = AtomicUsize::new(0); + +fn jj_available() -> bool { + Command::new("jj") + .arg("--version") + .output() + .map(|o| o.status.success()) + .unwrap_or(false) +} + +struct TempRepo { + path: PathBuf, +} + +impl Drop for TempRepo { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.path); + } +} + +/// Run a mutating jj command in the repo; panics on failure. +fn jj(dir: &Path, args: &[&str]) -> String { + let mut full: Vec<&str> = vec![ + "--no-pager", + "--config", + "user.name=Oot Test", + "--config", + "user.email=oot@example.com", + ]; + full.extend_from_slice(args); + + let out = Command::new("jj") + .args(&full) + .current_dir(dir) + .output() + .expect("failed to spawn jj"); + assert!( + out.status.success(), + "jj {:?} failed: {}", + args, + String::from_utf8_lossy(&out.stderr) + ); + String::from_utf8_lossy(&out.stdout).into_owned() +} + +fn init_repo(colocate: bool) -> TempRepo { + let n = COUNTER.fetch_add(1, Ordering::SeqCst); + let dir = std::env::temp_dir().join(format!("oot-jj-test-{}-{}", std::process::id(), n)); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + + if colocate { + jj(&dir, &["git", "init", "--colocate", "."]); + } else { + jj(&dir, &["git", "init", "--no-colocate", "."]); + } + TempRepo { path: dir } +} + +fn write_lib(dir: &Path, body: &str) { + let src = dir.join("src"); + std::fs::create_dir_all(&src).unwrap(); + std::fs::write( + src.join("lib.rs"), + format!("pub fn hello() -> &'static str {{ \"{}\" }}\n", body), + ) + .unwrap(); +} + +/// Create a repo with two commits: `main` (base) and a head commit on top. +/// Returns the repo plus the revset for the head commit (`@-` after setup). +fn setup_base_and_head(colocate: bool) -> (TempRepo, JjAdapter) { + let repo = init_repo(colocate); + write_lib(&repo.path, "hello"); + + // Commit base, bookmark it as main. Working copy becomes a new empty @. + jj(&repo.path, &["commit", "-m", "base"]); + jj(&repo.path, &["bookmark", "create", "main", "-r", "@-"]); + + // Head commit modifies the function body. + write_lib(&repo.path, "hello world"); + jj(&repo.path, &["commit", "-m", "change"]); + + let adapter = JjAdapter::new(&repo.path).expect("adapter should discover jj repo"); + (repo, adapter) +} + +#[test] +fn test_jj_adapter_discover_and_root() { + if !jj_available() { + return; + } + for colocate in [true, false] { + let (repo, adapter) = setup_base_and_head(colocate); + assert!(adapter.repo_root().exists()); + assert!(adapter.repo_root().starts_with(std::env::temp_dir())); + drop(repo); + } +} + +#[test] +fn test_jj_resolve_commit_id() { + if !jj_available() { + return; + } + let (_repo, adapter) = setup_base_and_head(false); + + let id = adapter.resolve_commit_id("@-").expect("head resolves"); + assert!(!id.is_empty()); + + let main_id = adapter + .resolve_commit_id("bookmarks(exact:main)") + .expect("bookmark resolves"); + assert_ne!(id, main_id, "base and head are distinct commits"); + + assert!(adapter.resolve_commit_id("no-such-bookmark-xyz").is_err()); +} + +#[test] +fn test_jj_ancestor_of_base_and_head_is_base() { + if !jj_available() { + return; + } + let (_repo, adapter) = setup_base_and_head(true); + + let base = adapter.resolve_commit_id("bookmarks(exact:main)").unwrap(); + let head = adapter.resolve_commit_id("@-").unwrap(); + let anc = adapter.ancestor(&base, &head).expect("ancestor exists"); + assert_eq!(anc, base, "linear history: ancestor of main..@- is main"); +} + +#[test] +fn test_jj_extract_snapshot() { + if !jj_available() { + return; + } + let (_repo, adapter) = setup_base_and_head(false); + + let snap = adapter.extract_snapshot("@-").expect("snapshot extracts"); + let content = snap + .files + .get("src/lib.rs") + .expect("src/lib.rs present in head snapshot"); + assert!(content.contains("hello world")); + assert!(!content.contains("<<<<<<<"), "clean commit has no markers"); +} + +#[test] +fn test_jj_adjudicate_3way_clean_unilateral_change() { + if !jj_available() { + return; + } + let (_repo, adapter) = setup_base_and_head(false); + let eng = Engine::new().unwrap(); + + let docket = adapter + .adjudicate_3way( + "bookmarks(exact:main)", + "@-", + &eng, + &MeaningPolicy::default(), + &VisibilityPolicy::default(), + &JjAdjudicateOptions { + change_name: Some("test-change".into()), + ..Default::default() + }, + ) + .expect("adjudication succeeds"); + + assert_eq!(docket.change, "test-change"); + assert!(docket.source.starts_with("jj:"), "docket labeled jj"); + assert!( + !docket.disputes.is_empty(), + "modified function should raise a meaning dispute" + ); + assert!(docket + .disputes + .iter() + .any(|d| d.detail.contains("hello") && d.kind == oot::dispute::Kind::Meaning)); + assert_eq!(docket.verdict, oot::dispute::Verdict::Adjudicated); + + let rendered = docket.render(); + assert!(rendered.contains("OOT DOCKET")); +} + +#[test] +fn test_jj_conflicted_commit_detected_not_parsed() { + if !jj_available() { + return; + } + let repo = init_repo(false); + write_lib(&repo.path, "original"); + + jj(&repo.path, &["commit", "-m", "base"]); + jj(&repo.path, &["bookmark", "create", "main", "-r", "@-"]); + + // Side A changes the line one way. + write_lib(&repo.path, "side a"); + jj(&repo.path, &["commit", "-m", "side a"]); + let adapter = JjAdapter::new(&repo.path).unwrap(); + let side_a = adapter.resolve_commit_id("@-").unwrap(); + + // Side B starts from main and changes the same line differently. + jj(&repo.path, &["new", "bookmarks(exact:main)"]); + write_lib(&repo.path, "side b"); + jj(&repo.path, &["commit", "-m", "side b"]); + let side_b = adapter.resolve_commit_id("@-").unwrap(); + + // Merge both sides: conflicting edit to the same line yields a first-class conflict. + jj(&repo.path, &["new", &side_a, &side_b]); + + let (snap, conflicted) = adapter + .extract_snapshot_with_conflicts("@") + .expect("merge snapshot extracts"); + + assert!( + conflicted.iter().any(|p| p == "src/lib.rs"), + "conflicted file should be reported separately, got {:?}", + conflicted + ); + assert!( + !snap.files.contains_key("src/lib.rs"), + "conflicted file must be excluded from the parseable snapshot" + ); +} diff --git a/visibility.toml b/visibility.toml new file mode 100644 index 0000000..9953128 --- /dev/null +++ b/visibility.toml @@ -0,0 +1,11 @@ +# Oot governing Oot. +# +# Path fragments (substring match) that must never appear in a change's +# head snapshot. Touched paths raise High-severity visibility disputes, +# which cloak the change and exit nonzero. +private_paths = [".env", "secrets/", ".pem"] + +# No embargo schedule for this repo yet; add `embargo_until = "YYYY-MM-DD"` +# to hold all changes until that date. + +private_branches = []