From 084a646190c2a943991b44def003d81a905df27c Mon Sep 17 00:00:00 2001 From: kridaydave Date: Fri, 21 Aug 2026 10:46:41 +0530 Subject: [PATCH 01/11] fix: track fixtures env, fix roadmap vs code, ignore exception - .gitignore: allow fixtures/repo/head/secrets/.env (dummy) so visibility test passes; all other *.env still ignored - fixtures/repo/head/secrets/.env: dummy API_KEY for cloaked verdict test - README: mark implemented items [x] (git ingestion, visibility, engine, docket, in-memory path, git adapter) and keep jj/hosted pending - clean untracked fixtures/base,head and duplicate tomls tests: 54/54 pass, clippy/fmt green --- .gitignore | 1 + README.md | 21 +++++++++++---------- fixtures/repo/head/secrets/.env | 2 ++ 3 files changed, 14 insertions(+), 10 deletions(-) create mode 100644 fixtures/repo/head/secrets/.env diff --git a/.gitignore b/.gitignore index c10debe..281953e 100644 --- a/.gitignore +++ b/.gitignore @@ -2,4 +2,5 @@ **/*.rs.bk *.cargo/config *.env +!fixtures/repo/head/secrets/.env *.DS_Store diff --git a/README.md b/README.md index ddb960b..b3f3dc5 100644 --- a/README.md +++ b/README.md @@ -63,14 +63,14 @@ 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 ingestion is in-memory. Jujutsu and hosted intent are next. -- [ ] 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 — Jujutsu snapshots pending +- [x] Visibility policy: private paths, private branches, embargo schedules (the governance spine) +- [x] Meaning disputes from the structural engine (tree-sitter Rust) — hosted intent check pending +- [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 — Jujutsu adapter and hosted model API pending ## Open source and the model @@ -78,13 +78,14 @@ The adjudication runtime, the docket format, and the adapters are MIT licensed. ## Try it -The runtime is not buildable to this shape yet. When it is: - ```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 . ``` ## Contribute diff --git a/fixtures/repo/head/secrets/.env b/fixtures/repo/head/secrets/.env new file mode 100644 index 0000000..d2c4057 --- /dev/null +++ b/fixtures/repo/head/secrets/.env @@ -0,0 +1,2 @@ +DATABASE_URL=postgres://localhost:5432/oot +API_KEY=do-not-leak-me From 4123fd4a787391fbb6859ebaa905ddb38c30e517 Mon Sep 17 00:00:00 2001 From: kridaydave Date: Fri, 21 Aug 2026 21:27:49 +0530 Subject: [PATCH 02/11] feat: Jujutsu 3-way snapshot ingestion adapter with revsets and conflict detection --- .github/workflows/ci.yml | 11 + .../jj-adapter/jj-adapter-research.md | 77 ++++ README.md | 8 +- src/adapter/jj.rs | 351 ++++++++++++++++++ src/adapter/mod.rs | 2 + src/main.rs | 64 +++- tests/jj_adapter_test.rs | 238 ++++++++++++ 7 files changed, 729 insertions(+), 22 deletions(-) create mode 100644 CD_res/implementation/jj-adapter/jj-adapter-research.md create mode 100644 src/adapter/jj.rs create mode 100644 tests/jj_adapter_test.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 62aa802..11d5402 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -39,6 +39,17 @@ 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 + tar -xzf /tmp/jj.tar.gz -C /tmp + sudo mv /tmp/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 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/README.md b/README.md index b3f3dc5..7ddc4ec 100644 --- a/README.md +++ b/README.md @@ -65,12 +65,14 @@ If a dispute crosses policy, Oot blocks the change or cloaks the private parts. Working seed — the engine runs, the docket renders, and git ingestion is in-memory. Jujutsu and hosted intent are next. -- [x] Change ingestion from git snapshots (in-memory via `git ls-tree`/`cat-file`) and materialized dirs — Jujutsu snapshots pending +- [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) — hosted model API pending - [x] Visibility policy: private paths, private branches, embargo schedules (the governance spine) - [x] Meaning disputes from the structural engine (tree-sitter Rust) — hosted intent check pending - [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 — Jujutsu adapter and hosted model API pending +- [x] Git adapter with 3-way adjudication +- [x] Jujutsu adapter with 3-way adjudication (`--source jj`, revsets accepted) — hosted model API pending ## Open source and the model @@ -86,6 +88,8 @@ cargo build --release ./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/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/main.rs b/src/main.rs index 35dad8a..5d5bcc4 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). @@ -104,27 +104,51 @@ 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()); 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" + ); +} From 249087cdee91ced2898d5c0a2684d84104630526 Mon Sep 17 00:00:00 2001 From: kridaydave Date: Fri, 21 Aug 2026 21:56:27 +0530 Subject: [PATCH 03/11] feat: multi-language structural engine with Go and JavaScript support --- .../multi-lang-engine-research.md | 79 +++++++++ Cargo.lock | 22 +++ Cargo.toml | 2 + README.md | 2 +- src/adapter/git.rs | 2 +- src/adapter/jj.rs | 2 +- src/engine/mod.rs | 132 +++++++++++---- src/main.rs | 6 +- tests/engine_test.rs | 158 +++++++++++++++++- tests/git_adapter_test.rs | 12 +- tests/jj_adapter_test.rs | 4 +- 11 files changed, 370 insertions(+), 51 deletions(-) create mode 100644 CD_res/implementation/multi-lang-engine/multi-lang-engine-research.md 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..ca68642 --- /dev/null +++ b/CD_res/implementation/multi-lang-engine/multi-lang-engine-research.md @@ -0,0 +1,79 @@ +# 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). diff --git a/Cargo.lock b/Cargo.lock index 532dac2..c9b47da 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -191,6 +191,8 @@ dependencies = [ "serde_json", "toml", "tree-sitter", + "tree-sitter-go", + "tree-sitter-javascript", "tree-sitter-rust", ] @@ -369,6 +371,26 @@ dependencies = [ "tree-sitter-language", ] +[[package]] +name = "tree-sitter-go" +version = "0.23.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b13d476345220dbe600147dd444165c5791bf85ef53e28acbedd46112ee18431" +dependencies = [ + "cc", + "tree-sitter-language", +] + +[[package]] +name = "tree-sitter-javascript" +version = "0.23.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bf40bf599e0416c16c125c3cec10ee5ddc7d1bb8b0c60fa5c4de249ad34dc1b1" +dependencies = [ + "cc", + "tree-sitter-language", +] + [[package]] name = "tree-sitter-language" version = "0.1.7" diff --git a/Cargo.toml b/Cargo.toml index 8b1bafc..2654cc8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -13,3 +13,5 @@ toml = "0.8" anyhow = "1" tree-sitter = "0.23" tree-sitter-rust = "0.23" +tree-sitter-go = "0.23" +tree-sitter-javascript = "0.23" diff --git a/README.md b/README.md index 7ddc4ec..1b78d55 100644 --- a/README.md +++ b/README.md @@ -68,7 +68,7 @@ Working seed — the engine runs, the docket renders, and git ingestion is in-me - [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) — hosted model API pending - [x] Visibility policy: private paths, private branches, embargo schedules (the governance spine) -- [x] Meaning disputes from the structural engine (tree-sitter Rust) — hosted intent check pending +- [x] Meaning disputes from the structural engine (tree-sitter: Rust, Go, JavaScript) — hosted intent check pending - [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 diff --git a/src/adapter/git.rs b/src/adapter/git.rs index d7fcc35..0c92d1c 100644 --- a/src/adapter/git.rs +++ b/src/adapter/git.rs @@ -177,7 +177,7 @@ impl GitAdapter { &self, base_ref: &str, head_ref: &str, - engine: &Engine, + engine: &mut Engine, meaning_policy: &MeaningPolicy, visibility_policy: &VisibilityPolicy, options: &GitAdjudicateOptions, diff --git a/src/adapter/jj.rs b/src/adapter/jj.rs index 48822fb..d506844 100644 --- a/src/adapter/jj.rs +++ b/src/adapter/jj.rs @@ -232,7 +232,7 @@ impl JjAdapter { &self, base_ref: &str, head_ref: &str, - engine: &Engine, + engine: &mut Engine, meaning_policy: &MeaningPolicy, visibility_policy: &VisibilityPolicy, options: &JjAdjudicateOptions, diff --git a/src/engine/mod.rs b/src/engine/mod.rs index d01bd82..9e4a133 100644 --- a/src/engine/mod.rs +++ b/src/engine/mod.rs @@ -8,24 +8,78 @@ use crate::dispute::{Dispute, Kind, Severity}; use std::collections::HashMap; use tree_sitter::{Node, Parser, Tree}; +/// Languages the structural engine can parse. +#[derive(Clone, Copy, PartialEq, Eq, Hash, Debug)] +enum Lang { + Rust, + Go, + JavaScript, +} + +impl Lang { + /// Detect the language of a file from its extension. + fn from_path(path: &str) -> Option { + let ext = path.rsplit('.').next()?; + match ext { + "rs" => Some(Lang::Rust), + "go" => Some(Lang::Go), + "js" | "mjs" | "cjs" => Some(Lang::JavaScript), + _ => None, + } + } + + fn language(self) -> tree_sitter::Language { + match self { + Lang::Rust => tree_sitter_rust::LANGUAGE.into(), + Lang::Go => tree_sitter_go::LANGUAGE.into(), + Lang::JavaScript => tree_sitter_javascript::LANGUAGE.into(), + } + } + + /// Node kinds that declare a function in this language. Every kind must + /// expose a `name` field. + fn function_kinds(self) -> &'static [&'static str] { + match self { + Lang::Rust => &["function_item"], + Lang::Go => &["function_declaration", "method_declaration"], + Lang::JavaScript => &[ + "function_declaration", + "function_expression", + "generator_function_declaration", + "generator_function", + "method_definition", + ], + } + } +} + /// Structural difference engine for code snapshots. pub struct Engine { - language: tree_sitter::Language, + parsers: HashMap, } impl Engine { - /// Create a new structural diff engine initialized with Rust grammar support. + /// Create a new structural diff engine with one parser per supported language. + /// + /// Fails loudly if any grammar's ABI is incompatible with the runtime — this + /// doubles as a version-drift tripwire for CI. pub fn new() -> anyhow::Result { - let language = tree_sitter_rust::LANGUAGE.into(); - Ok(Engine { language }) + let mut parsers = HashMap::new(); + for lang in [Lang::Rust, Lang::Go, Lang::JavaScript] { + let mut parser = Parser::new(); + parser.set_language(&lang.language())?; + parsers.insert(lang, parser); + } + Ok(Engine { parsers }) } /// Compare two snapshots and report Meaning disputes: functions that /// changed, were added, or were removed between base and head. - pub fn diff_snapshots(&self, base: &Snapshot, head: &Snapshot) -> anyhow::Result> { - let mut parser = Parser::new(); - parser.set_language(&self.language)?; - + pub fn diff_snapshots( + &mut self, + base: &Snapshot, + head: &Snapshot, + ) -> anyhow::Result> { let mut disputes = Vec::new(); let mut n = 1; @@ -34,16 +88,19 @@ impl Engine { paths.dedup(); for path in paths { - if !path.ends_with(".rs") { + let Some(lang) = Lang::from_path(path) else { continue; - } + }; let base_src = base.files.get(path); let head_src = head.files.get(path); match (base_src, head_src) { (Some(b), Some(h)) => { - let base_fns = extract_functions(parse(&mut parser, b).as_ref(), b); - let head_fns = extract_functions(parse(&mut parser, h).as_ref(), h); + let Some(parser) = self.parsers.get_mut(&lang) else { + continue; + }; + let base_fns = extract_functions(parse(parser, b).as_ref(), b, lang); + let head_fns = extract_functions(parse(parser, h).as_ref(), h, lang); for (name, (h_src, h_row)) in &head_fns { match base_fns.get(name) { Some((b_src, _)) if b_src != h_src => { @@ -106,14 +163,11 @@ impl Engine { /// Perform a 3-way semantic diff between a common merge-base ancestor, /// the target (ours) branch, and the incoming (theirs/head) branch. pub fn diff_3way( - &self, + &mut self, base: &Snapshot, ours: &Snapshot, theirs: &Snapshot, ) -> anyhow::Result> { - let mut parser = Parser::new(); - parser.set_language(&self.language)?; - let mut disputes = Vec::new(); let mut n = 1; @@ -127,9 +181,9 @@ impl Engine { paths.dedup(); for path in paths { - if !path.ends_with(".rs") { + let Some(lang) = Lang::from_path(path) else { continue; - } + }; let b_file = base.files.get(path); let o_file = ours.files.get(path); let t_file = theirs.files.get(path); @@ -137,9 +191,12 @@ 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(parse(&mut parser, b_src).as_ref(), b_src); - let o_fns = extract_functions(parse(&mut parser, o_src).as_ref(), o_src); - let t_fns = extract_functions(parse(&mut parser, t_src).as_ref(), t_src); + let Some(parser) = self.parsers.get_mut(&lang) else { + continue; + }; + let b_fns = extract_functions(parse(parser, b_src).as_ref(), b_src, lang); + let o_fns = extract_functions(parse(parser, o_src).as_ref(), o_src, lang); + let t_fns = extract_functions(parse(parser, t_src).as_ref(), t_src, lang); let mut all_fn_names: Vec<&String> = b_fns .keys() @@ -311,17 +368,21 @@ fn meaning(n: &mut i32, path: &str, row: usize, detail: String, severity: Severi } } -fn extract_functions(tree: Option<&Tree>, source: &str) -> HashMap { +fn extract_functions( + tree: Option<&Tree>, + source: &str, + lang: Lang, +) -> HashMap { let mut map = HashMap::new(); let Some(tree) = tree else { return map; }; - collect(tree.root_node(), source, &mut map); + collect(tree.root_node(), source, lang, &mut map); map } -fn collect(node: Node, source: &str, map: &mut HashMap) { - if node.kind() == "function_item" { +fn collect(node: Node, source: &str, lang: Lang, map: &mut HashMap) { + if lang.function_kinds().contains(&node.kind()) { if let Some(name_node) = node.child_by_field_name("name") { let name = name_node .utf8_text(source.as_bytes()) @@ -334,7 +395,7 @@ fn collect(node: Node, source: &str, map: &mut HashMap) } let mut cursor = node.walk(); for child in node.children(&mut cursor) { - collect(child, source, map); + collect(child, source, lang, map); } } @@ -342,9 +403,24 @@ fn collect(node: Node, source: &str, map: &mut HashMap) mod tests { use super::*; + #[test] + fn test_all_grammars_abi_compatible() { + // Tripwire: if a grammar crate is bumped past the runtime's supported + // ABI range, set_language fails here instead of at some user's build. + for lang in [Lang::Rust, Lang::Go, Lang::JavaScript] { + let mut parser = Parser::new(); + parser.set_language(&lang.language()).unwrap_or_else(|e| { + panic!( + "grammar for {:?} is ABI-incompatible with runtime: {}", + lang, e + ) + }); + } + } + #[test] fn test_engine_diff_functions() { - let eng = Engine::new().unwrap(); + let mut eng = Engine::new().unwrap(); let mut base = Snapshot::default(); base.files.insert( @@ -375,7 +451,7 @@ mod tests { #[test] fn test_engine_diff_3way_conflict_and_clean() { - let eng = Engine::new().unwrap(); + let mut eng = Engine::new().unwrap(); let mut base = Snapshot::default(); base.files.insert( diff --git a/src/main.rs b/src/main.rs index 5d5bcc4..9abb57f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -102,7 +102,7 @@ fn main() -> anyhow::Result<()> { Some(v) => VisibilityPolicy::load(std::path::Path::new(&v))?, None => VisibilityPolicy::default(), }; - let eng = Engine::new()?; + let mut eng = Engine::new()?; // VCS 3-way In-Memory Adjudication (git or jj) if let (Some(b_ref), Some(h_ref)) = (base_ref, head_ref) { @@ -123,7 +123,7 @@ fn main() -> anyhow::Result<()> { jj_adapter.adjudicate_3way( &b_ref, &h_ref, - &eng, + &mut eng, &meaning_policy, &visibility_policy, &options, @@ -143,7 +143,7 @@ fn main() -> anyhow::Result<()> { git_adapter.adjudicate_3way( &b_ref, &h_ref, - &eng, + &mut eng, &meaning_policy, &visibility_policy, &options, diff --git a/tests/engine_test.rs b/tests/engine_test.rs index 9d4b7c0..84eab4d 100644 --- a/tests/engine_test.rs +++ b/tests/engine_test.rs @@ -4,7 +4,7 @@ use oot::engine::Engine; #[test] fn test_engine_function_modification_detection() { - let engine = Engine::new().expect("Failed to initialize engine"); + let mut engine = Engine::new().expect("Failed to initialize engine"); let mut base = Snapshot::default(); base.files.insert( @@ -41,7 +41,7 @@ fn authenticate(user: &str, pass: &str) -> bool { #[test] fn test_engine_function_addition_and_deletion_detection() { - let engine = Engine::new().expect("Failed to initialize engine"); + let mut engine = Engine::new().expect("Failed to initialize engine"); let mut base = Snapshot::default(); base.files.insert( @@ -93,7 +93,7 @@ fn subtract(a: i32, b: i32) -> i32 { #[test] fn test_engine_identical_unchanged_files() { - let engine = Engine::new().expect("Failed to initialize engine"); + let mut engine = Engine::new().expect("Failed to initialize engine"); let source = r#" pub fn calculate_hash(data: &[u8]) -> u64 { @@ -127,8 +127,8 @@ pub fn verify_signature() -> bool { } #[test] -fn test_engine_non_rust_file_filtering() { - let engine = Engine::new().expect("Failed to initialize engine"); +fn test_engine_unsupported_extension_filtering() { + let mut engine = Engine::new().expect("Failed to initialize engine"); let mut base = Snapshot::default(); base.files.insert( @@ -168,13 +168,13 @@ 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" ); } #[test] fn test_engine_syntax_error_handling() { - let engine = Engine::new().expect("Failed to initialize engine"); + let mut engine = Engine::new().expect("Failed to initialize engine"); let mut base = Snapshot::default(); base.files.insert( @@ -201,7 +201,7 @@ fn test_engine_syntax_error_handling() { #[test] fn test_engine_file_added_and_removed() { - let engine = Engine::new().expect("Failed to initialize engine"); + let mut engine = Engine::new().expect("Failed to initialize engine"); let mut base = Snapshot::default(); base.files.insert( @@ -233,7 +233,7 @@ fn test_engine_file_added_and_removed() { #[test] fn test_engine_multiple_files_and_functions() { - let engine = Engine::new().expect("Failed to initialize engine"); + let mut engine = Engine::new().expect("Failed to initialize engine"); let mut base = Snapshot::default(); base.files.insert( @@ -260,3 +260,143 @@ fn test_engine_multiple_files_and_functions() { .any(|d| d.detail == "both sides changed `fa1`")); assert!(disputes.iter().any(|d| d.detail == "added function `fa3`")); } + +#[test] +fn test_engine_go_function_and_method_detection() { + let mut engine = Engine::new().expect("Failed to initialize engine"); + + let mut base = Snapshot::default(); + base.files.insert( + "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(), + ); + + 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 mut 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 mut engine = Engine::new().expect("Failed to initialize engine"); + + let mut base = Snapshot::default(); + base.files.insert( + "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( + "src/api.js".to_string(), + r#" +export function fetchUser(id) { + return { id, includeProfile: true }; +} + +class Client { + connect(timeoutMs) { + return timeoutMs > 0; + } +} +"# + .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`")); +} diff --git a/tests/git_adapter_test.rs b/tests/git_adapter_test.rs index e72bf10..1980a74 100644 --- a/tests/git_adapter_test.rs +++ b/tests/git_adapter_test.rs @@ -148,7 +148,7 @@ fn test_git_adapter_3way_semantic_conflict() { let _main_sha = repo.commit("main change"); let adapter = GitAdapter::new(&repo.path).expect("valid git repo"); - let engine = Engine::new().expect("valid engine"); + let mut engine = Engine::new().expect("valid engine"); let meaning_policy = MeaningPolicy::default(); let visibility_policy = VisibilityPolicy::default(); @@ -169,7 +169,7 @@ fn test_git_adapter_3way_semantic_conflict() { .adjudicate_3way( "main", "feature/auth", - &engine, + &mut engine, &meaning_policy, &visibility_policy, &options, @@ -209,7 +209,7 @@ fn test_git_adapter_3way_unilateral_clean() { repo.checkout("main"); let adapter = GitAdapter::new(&repo.path).expect("valid git repo"); - let engine = Engine::new().expect("valid engine"); + let mut engine = Engine::new().expect("valid engine"); let meaning_policy = MeaningPolicy::default(); let visibility_policy = VisibilityPolicy::default(); @@ -217,7 +217,7 @@ fn test_git_adapter_3way_unilateral_clean() { .adjudicate_3way( "main", "feature/new-fn", - &engine, + &mut engine, &meaning_policy, &visibility_policy, &GitAdjudicateOptions::default(), @@ -244,7 +244,7 @@ fn test_git_adapter_visibility_violation_cloaked() { repo.commit("add secrets"); let adapter = GitAdapter::new(&repo.path).expect("valid git repo"); - let engine = Engine::new().expect("valid engine"); + let mut engine = Engine::new().expect("valid engine"); let meaning_policy = MeaningPolicy::default(); let visibility_policy = VisibilityPolicy::default(); // defaults to secrets/ and .env private @@ -252,7 +252,7 @@ fn test_git_adapter_visibility_violation_cloaked() { .adjudicate_3way( "main", "feature/secrets", - &engine, + &mut engine, &meaning_policy, &visibility_policy, &GitAdjudicateOptions::default(), diff --git a/tests/jj_adapter_test.rs b/tests/jj_adapter_test.rs index 9e89136..8682a60 100644 --- a/tests/jj_adapter_test.rs +++ b/tests/jj_adapter_test.rs @@ -164,13 +164,13 @@ fn test_jj_adjudicate_3way_clean_unilateral_change() { return; } let (_repo, adapter) = setup_base_and_head(false); - let eng = Engine::new().unwrap(); + let mut eng = Engine::new().unwrap(); let docket = adapter .adjudicate_3way( "bookmarks(exact:main)", "@-", - &eng, + &mut eng, &MeaningPolicy::default(), &VisibilityPolicy::default(), &JjAdjudicateOptions { From d099055cebecd02f91ff6b19b3abdc7f507291f9 Mon Sep 17 00:00:00 2001 From: kridaydave Date: Fri, 21 Aug 2026 22:35:29 +0530 Subject: [PATCH 04/11] =?UTF-8?q?fix:=20apply=20adversarial=20review=20?= =?UTF-8?q?=E2=80=94=20JS=20const-arrow=20tracking,=20dotless-path=20regre?= =?UTF-8?q?ssion,=20ambiguity=20disputes,=20&self=20engine=20API?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../multi-lang-engine-research.md | 11 + README.md | 2 + src/adapter/git.rs | 2 +- src/adapter/jj.rs | 2 +- src/engine/mod.rs | 217 ++++++++++++------ src/main.rs | 6 +- tests/engine_test.rs | 171 +++++++++++++- tests/git_adapter_test.rs | 12 +- tests/jj_adapter_test.rs | 4 +- 9 files changed, 333 insertions(+), 94 deletions(-) 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 index ca68642..00a9eba 100644 --- a/CD_res/implementation/multi-lang-engine/multi-lang-engine-research.md +++ b/CD_res/implementation/multi-lang-engine/multi-lang-engine-research.md @@ -77,3 +77,14 @@ Findings from three adversarial passes and how the plan changed: 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/README.md b/README.md index 1b78d55..b2300ea 100644 --- a/README.md +++ b/README.md @@ -74,6 +74,8 @@ Working seed — the engine runs, the docket renders, and git ingestion is in-me - [x] Git adapter with 3-way adjudication - [x] Jujutsu adapter with 3-way adjudication (`--source jj`, revsets accepted) — hosted model API pending +**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. + ## Open source and the model 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. diff --git a/src/adapter/git.rs b/src/adapter/git.rs index 0c92d1c..d7fcc35 100644 --- a/src/adapter/git.rs +++ b/src/adapter/git.rs @@ -177,7 +177,7 @@ impl GitAdapter { &self, base_ref: &str, head_ref: &str, - engine: &mut Engine, + engine: &Engine, meaning_policy: &MeaningPolicy, visibility_policy: &VisibilityPolicy, options: &GitAdjudicateOptions, diff --git a/src/adapter/jj.rs b/src/adapter/jj.rs index d506844..48822fb 100644 --- a/src/adapter/jj.rs +++ b/src/adapter/jj.rs @@ -232,7 +232,7 @@ impl JjAdapter { &self, base_ref: &str, head_ref: &str, - engine: &mut Engine, + engine: &Engine, meaning_policy: &MeaningPolicy, visibility_policy: &VisibilityPolicy, options: &JjAdjudicateOptions, diff --git a/src/engine/mod.rs b/src/engine/mod.rs index 9e4a133..d513033 100644 --- a/src/engine/mod.rs +++ b/src/engine/mod.rs @@ -5,6 +5,7 @@ use crate::change::Snapshot; use crate::dispute::{Dispute, Kind, Severity}; +use anyhow::Context; use std::collections::HashMap; use tree_sitter::{Node, Parser, Tree}; @@ -19,8 +20,8 @@ enum Lang { impl Lang { /// Detect the language of a file from its extension. fn from_path(path: &str) -> Option { - let ext = path.rsplit('.').next()?; - match ext { + let ext = path.rsplit_once('.')?.1.to_ascii_lowercase(); + match ext.as_str() { "rs" => Some(Lang::Rust), "go" => Some(Lang::Go), "js" | "mjs" | "cjs" => Some(Lang::JavaScript), @@ -36,8 +37,8 @@ impl Lang { } } - /// Node kinds that declare a function in this language. Every kind must - /// expose a `name` field. + /// Node kinds that declare a function in this language. Kinds without a + /// `name` field fall back to the enclosing variable binding's name. fn function_kinds(self) -> &'static [&'static str] { match self { Lang::Rust => &["function_item"], @@ -48,6 +49,7 @@ impl Lang { "generator_function_declaration", "generator_function", "method_definition", + "arrow_function", ], } } @@ -55,31 +57,31 @@ impl Lang { /// Structural difference engine for code snapshots. pub struct Engine { - parsers: HashMap, + languages: HashMap, } impl Engine { - /// Create a new structural diff engine with one parser per supported language. + /// Create a new structural diff engine with one validated grammar per + /// supported language. /// /// Fails loudly if any grammar's ABI is incompatible with the runtime — this /// doubles as a version-drift tripwire for CI. pub fn new() -> anyhow::Result { - let mut parsers = HashMap::new(); + let mut languages = HashMap::new(); for lang in [Lang::Rust, Lang::Go, Lang::JavaScript] { - let mut parser = Parser::new(); - parser.set_language(&lang.language())?; - parsers.insert(lang, parser); + let language = lang.language(); + let mut probe = Parser::new(); + probe + .set_language(&language) + .with_context(|| format!("loading grammar for {lang:?}"))?; + languages.insert(lang, language); } - Ok(Engine { parsers }) + Ok(Engine { languages }) } /// Compare two snapshots and report Meaning disputes: functions that /// changed, were added, or were removed between base and head. - pub fn diff_snapshots( - &mut self, - base: &Snapshot, - head: &Snapshot, - ) -> anyhow::Result> { + pub fn diff_snapshots(&self, base: &Snapshot, head: &Snapshot) -> anyhow::Result> { let mut disputes = Vec::new(); let mut n = 1; @@ -91,17 +93,36 @@ impl Engine { let Some(lang) = Lang::from_path(path) else { continue; }; + let language = &self.languages[&lang]; let base_src = base.files.get(path); let head_src = head.files.get(path); match (base_src, head_src) { (Some(b), Some(h)) => { - let Some(parser) = self.parsers.get_mut(&lang) else { - continue; - }; - let base_fns = extract_functions(parse(parser, b).as_ref(), b, lang); - let head_fns = extract_functions(parse(parser, h).as_ref(), h, lang); - for (name, (h_src, h_row)) in &head_fns { + let (base_fns, mut dupes) = + extract_functions(parse_source(language, b)?.as_ref(), b, lang); + let (head_fns, head_dupes) = + extract_functions(parse_source(language, h)?.as_ref(), h, lang); + 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 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 => { disputes.push(meaning( @@ -124,16 +145,19 @@ impl Engine { _ => {} } } - for name in base_fns.keys() { - if !head_fns.contains_key(name) { - disputes.push(meaning( - &mut n, - path, - 0, - format!("removed function `{}`", name), - Severity::Review, - )); - } + let mut removed: Vec<&String> = base_fns + .keys() + .filter(|name| !head_fns.contains_key(*name)) + .collect(); + removed.sort(); + for name in removed { + disputes.push(meaning( + &mut n, + path, + 0, + format!("removed function `{}`", name), + Severity::Review, + )); } } (Some(_), None) => { @@ -163,7 +187,7 @@ impl Engine { /// Perform a 3-way semantic diff between a common merge-base ancestor, /// the target (ours) branch, and the incoming (theirs/head) branch. pub fn diff_3way( - &mut self, + &self, base: &Snapshot, ours: &Snapshot, theirs: &Snapshot, @@ -184,6 +208,7 @@ impl Engine { let Some(lang) = Lang::from_path(path) else { continue; }; + let language = &self.languages[&lang]; let b_file = base.files.get(path); let o_file = ours.files.get(path); let t_file = theirs.files.get(path); @@ -191,12 +216,28 @@ impl Engine { match (b_file, o_file, t_file) { // File exists in all three (Some(b_src), Some(o_src), Some(t_src)) => { - let Some(parser) = self.parsers.get_mut(&lang) else { - continue; - }; - let b_fns = extract_functions(parse(parser, b_src).as_ref(), b_src, lang); - let o_fns = extract_functions(parse(parser, o_src).as_ref(), o_src, lang); - let t_fns = extract_functions(parse(parser, t_src).as_ref(), t_src, lang); + let (b_fns, mut dupes) = + extract_functions(parse_source(language, b_src)?.as_ref(), b_src, lang); + let (o_fns, o_dupes) = + extract_functions(parse_source(language, o_src)?.as_ref(), o_src, lang); + let (t_fns, t_dupes) = + extract_functions(parse_source(language, t_src)?.as_ref(), t_src, lang); + 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() @@ -352,50 +393,84 @@ impl Engine { } } -fn parse(parser: &mut Parser, source: &str) -> Option { - parser.parse(source, None) -} - -fn meaning(n: &mut i32, path: &str, row: usize, detail: String, severity: Severity) -> Dispute { - let id = format!("D{:03}", n); - *n += 1; - Dispute { - id, - location: format!("{}:{}", path, row), - kind: Kind::Meaning, - severity, - detail, - } +fn parse_source(language: &tree_sitter::Language, source: &str) -> anyhow::Result> { + let mut parser = Parser::new(); + parser.set_language(language)?; + Ok(parser.parse(source, None)) } +/// Extract tracked functions as `name -> (source text, 1-based row)`, plus the +/// list of names defined more than once (only the first occurrence is kept). fn extract_functions( tree: Option<&Tree>, source: &str, lang: Lang, -) -> HashMap { +) -> (HashMap, Vec) { let mut map = HashMap::new(); - let Some(tree) = tree else { - return map; - }; - collect(tree.root_node(), source, lang, &mut map); - map + let mut duplicates = Vec::new(); + if let Some(tree) = tree { + collect(tree.root_node(), source, lang, &mut map, &mut duplicates); + } + (map, duplicates) } -fn collect(node: Node, source: &str, lang: Lang, map: &mut HashMap) { +fn collect( + node: Node, + source: &str, + lang: Lang, + map: &mut HashMap, + duplicates: &mut Vec, +) { if lang.function_kinds().contains(&node.kind()) { - if let Some(name_node) = node.child_by_field_name("name") { - let name = name_node - .utf8_text(source.as_bytes()) - .unwrap_or("") - .to_string(); - let src = node.utf8_text(source.as_bytes()).unwrap_or("").to_string(); - let row = node.start_position().row + 1; - map.insert(name, (src, row)); + let name = node + .child_by_field_name("name") + .and_then(|n| n.utf8_text(source.as_bytes()).ok()) + .map(str::to_string) + .or_else(|| inherited_name(node, source)); + if let Some(name) = name.filter(|n| !n.is_empty()) { + match map.entry(name.clone()) { + std::collections::hash_map::Entry::Occupied(_) => { + duplicates.push(name); + } + std::collections::hash_map::Entry::Vacant(slot) => { + let src = node.utf8_text(source.as_bytes()).unwrap_or("").to_string(); + let row = node.start_position().row + 1; + slot.insert((src, row)); + } + } } + // Do not recurse into matched functions: nested definitions are covered + // by the enclosing function's source span. + return; } let mut cursor = node.walk(); for child in node.children(&mut cursor) { - collect(child, source, lang, map); + collect(child, source, lang, map, duplicates); + } +} + +/// Name for anonymous functions bound to a variable: `const f = () => {}`. +fn inherited_name(node: Node, source: &str) -> Option { + let parent = node.parent()?; + if parent.kind() != "variable_declarator" { + return None; + } + parent + .child_by_field_name("name")? + .utf8_text(source.as_bytes()) + .ok() + .map(str::to_string) +} + +fn meaning(n: &mut i32, path: &str, row: usize, detail: String, severity: Severity) -> Dispute { + let id = format!("D{:03}", n); + *n += 1; + Dispute { + id, + location: format!("{}:{}", path, row), + kind: Kind::Meaning, + severity, + detail, } } @@ -420,7 +495,7 @@ mod tests { #[test] fn test_engine_diff_functions() { - let mut eng = Engine::new().unwrap(); + let eng = Engine::new().unwrap(); let mut base = Snapshot::default(); base.files.insert( @@ -451,7 +526,7 @@ mod tests { #[test] fn test_engine_diff_3way_conflict_and_clean() { - let mut eng = Engine::new().unwrap(); + let eng = Engine::new().unwrap(); let mut base = Snapshot::default(); base.files.insert( diff --git a/src/main.rs b/src/main.rs index 9abb57f..5d5bcc4 100644 --- a/src/main.rs +++ b/src/main.rs @@ -102,7 +102,7 @@ fn main() -> anyhow::Result<()> { Some(v) => VisibilityPolicy::load(std::path::Path::new(&v))?, None => VisibilityPolicy::default(), }; - let mut eng = Engine::new()?; + let eng = Engine::new()?; // VCS 3-way In-Memory Adjudication (git or jj) if let (Some(b_ref), Some(h_ref)) = (base_ref, head_ref) { @@ -123,7 +123,7 @@ fn main() -> anyhow::Result<()> { jj_adapter.adjudicate_3way( &b_ref, &h_ref, - &mut eng, + &eng, &meaning_policy, &visibility_policy, &options, @@ -143,7 +143,7 @@ fn main() -> anyhow::Result<()> { git_adapter.adjudicate_3way( &b_ref, &h_ref, - &mut eng, + &eng, &meaning_policy, &visibility_policy, &options, diff --git a/tests/engine_test.rs b/tests/engine_test.rs index 84eab4d..92bb4f5 100644 --- a/tests/engine_test.rs +++ b/tests/engine_test.rs @@ -4,7 +4,7 @@ use oot::engine::Engine; #[test] fn test_engine_function_modification_detection() { - let mut engine = Engine::new().expect("Failed to initialize engine"); + let engine = Engine::new().expect("Failed to initialize engine"); let mut base = Snapshot::default(); base.files.insert( @@ -41,7 +41,7 @@ fn authenticate(user: &str, pass: &str) -> bool { #[test] fn test_engine_function_addition_and_deletion_detection() { - let mut engine = Engine::new().expect("Failed to initialize engine"); + let engine = Engine::new().expect("Failed to initialize engine"); let mut base = Snapshot::default(); base.files.insert( @@ -93,7 +93,7 @@ fn subtract(a: i32, b: i32) -> i32 { #[test] fn test_engine_identical_unchanged_files() { - let mut engine = Engine::new().expect("Failed to initialize engine"); + let engine = Engine::new().expect("Failed to initialize engine"); let source = r#" pub fn calculate_hash(data: &[u8]) -> u64 { @@ -128,7 +128,7 @@ pub fn verify_signature() -> bool { #[test] fn test_engine_unsupported_extension_filtering() { - let mut engine = Engine::new().expect("Failed to initialize engine"); + let engine = Engine::new().expect("Failed to initialize engine"); let mut base = Snapshot::default(); base.files.insert( @@ -174,7 +174,7 @@ fn test_engine_unsupported_extension_filtering() { #[test] fn test_engine_syntax_error_handling() { - let mut engine = Engine::new().expect("Failed to initialize engine"); + let engine = Engine::new().expect("Failed to initialize engine"); let mut base = Snapshot::default(); base.files.insert( @@ -201,7 +201,7 @@ fn test_engine_syntax_error_handling() { #[test] fn test_engine_file_added_and_removed() { - let mut engine = Engine::new().expect("Failed to initialize engine"); + let engine = Engine::new().expect("Failed to initialize engine"); let mut base = Snapshot::default(); base.files.insert( @@ -233,7 +233,7 @@ fn test_engine_file_added_and_removed() { #[test] fn test_engine_multiple_files_and_functions() { - let mut engine = Engine::new().expect("Failed to initialize engine"); + let engine = Engine::new().expect("Failed to initialize engine"); let mut base = Snapshot::default(); base.files.insert( @@ -263,7 +263,7 @@ fn test_engine_multiple_files_and_functions() { #[test] fn test_engine_go_function_and_method_detection() { - let mut engine = Engine::new().expect("Failed to initialize engine"); + let engine = Engine::new().expect("Failed to initialize engine"); let mut base = Snapshot::default(); base.files.insert( @@ -307,7 +307,7 @@ func (s *Store) Name() string { #[test] fn test_engine_go_3way_method_conflict() { - let mut engine = Engine::new().expect("Failed to initialize engine"); + let engine = Engine::new().expect("Failed to initialize engine"); let base_src = r#" package store @@ -354,7 +354,7 @@ func (s *Store) Total() int { #[test] fn test_engine_javascript_function_detection() { - let mut engine = Engine::new().expect("Failed to initialize engine"); + let engine = Engine::new().expect("Failed to initialize engine"); let mut base = Snapshot::default(); base.files.insert( @@ -400,3 +400,154 @@ class Client { .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( + "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(), 1); + assert_eq!( + disputes[0].detail, "both sides changed `fetchUser`", + "arrow function bound to a const must be tracked under the binding name" + ); +} + +#[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!( + disputes.is_empty(), + "extension-less files must be skipped, got {:?}", + disputes + ); +} + +#[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/git_adapter_test.rs b/tests/git_adapter_test.rs index 1980a74..e72bf10 100644 --- a/tests/git_adapter_test.rs +++ b/tests/git_adapter_test.rs @@ -148,7 +148,7 @@ fn test_git_adapter_3way_semantic_conflict() { let _main_sha = repo.commit("main change"); let adapter = GitAdapter::new(&repo.path).expect("valid git repo"); - let mut engine = Engine::new().expect("valid engine"); + let engine = Engine::new().expect("valid engine"); let meaning_policy = MeaningPolicy::default(); let visibility_policy = VisibilityPolicy::default(); @@ -169,7 +169,7 @@ fn test_git_adapter_3way_semantic_conflict() { .adjudicate_3way( "main", "feature/auth", - &mut engine, + &engine, &meaning_policy, &visibility_policy, &options, @@ -209,7 +209,7 @@ fn test_git_adapter_3way_unilateral_clean() { repo.checkout("main"); let adapter = GitAdapter::new(&repo.path).expect("valid git repo"); - let mut engine = Engine::new().expect("valid engine"); + let engine = Engine::new().expect("valid engine"); let meaning_policy = MeaningPolicy::default(); let visibility_policy = VisibilityPolicy::default(); @@ -217,7 +217,7 @@ fn test_git_adapter_3way_unilateral_clean() { .adjudicate_3way( "main", "feature/new-fn", - &mut engine, + &engine, &meaning_policy, &visibility_policy, &GitAdjudicateOptions::default(), @@ -244,7 +244,7 @@ fn test_git_adapter_visibility_violation_cloaked() { repo.commit("add secrets"); let adapter = GitAdapter::new(&repo.path).expect("valid git repo"); - let mut engine = Engine::new().expect("valid engine"); + let engine = Engine::new().expect("valid engine"); let meaning_policy = MeaningPolicy::default(); let visibility_policy = VisibilityPolicy::default(); // defaults to secrets/ and .env private @@ -252,7 +252,7 @@ fn test_git_adapter_visibility_violation_cloaked() { .adjudicate_3way( "main", "feature/secrets", - &mut engine, + &engine, &meaning_policy, &visibility_policy, &GitAdjudicateOptions::default(), diff --git a/tests/jj_adapter_test.rs b/tests/jj_adapter_test.rs index 8682a60..9e89136 100644 --- a/tests/jj_adapter_test.rs +++ b/tests/jj_adapter_test.rs @@ -164,13 +164,13 @@ fn test_jj_adjudicate_3way_clean_unilateral_change() { return; } let (_repo, adapter) = setup_base_and_head(false); - let mut eng = Engine::new().unwrap(); + let eng = Engine::new().unwrap(); let docket = adapter .adjudicate_3way( "bookmarks(exact:main)", "@-", - &mut eng, + &eng, &MeaningPolicy::default(), &VisibilityPolicy::default(), &JjAdjudicateOptions { From cc91a4c2b376f8192efa1e79020a372de02549d4 Mon Sep 17 00:00:00 2001 From: kridaydave Date: Fri, 21 Aug 2026 22:50:25 +0530 Subject: [PATCH 05/11] feat: verdict-driven exit codes (0 ship, 1 hold) and oot's own visibility policy --- README.md | 19 +++++++---- src/main.rs | 22 +++++++++--- tests/cli_test.rs | 86 +++++++++++++++++++++++++++++++++++++++++++++-- visibility.toml | 11 ++++++ 4 files changed, 125 insertions(+), 13 deletions(-) create mode 100644 visibility.toml diff --git a/README.md b/README.md index b2300ea..96efdbf 100644 --- a/README.md +++ b/README.md @@ -63,22 +63,29 @@ If a dispute crosses policy, Oot blocks the change or cloaks the private parts. ## Status -Working seed — the engine runs, the docket renders, and git ingestion is in-memory. Jujutsu and hosted intent are next. +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. - [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) — hosted model API pending +- [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) — hosted intent check pending +- [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) — hosted model API pending +- [x] Jujutsu adapter with 3-way adjudication (`--source jj`, revsets accepted) **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. -## Open source and the model +## License -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. +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. + +- **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 diff --git a/src/main.rs b/src/main.rs index 5d5bcc4..9d4c8d0 100644 --- a/src/main.rs +++ b/src/main.rs @@ -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 { @@ -155,7 +155,7 @@ fn main() -> anyhow::Result<()> { 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 @@ -229,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/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/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 = [] From 8003000518ae476e1dcdb830675cb82d062813cf Mon Sep 17 00:00:00 2001 From: kridaydave Date: Fri, 21 Aug 2026 22:54:14 +0530 Subject: [PATCH 06/11] docs: track fixture .env policy-noise friction point --- TODO.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 TODO.md diff --git a/TODO.md b/TODO.md new file mode 100644 index 0000000..e98adc6 --- /dev/null +++ b/TODO.md @@ -0,0 +1,15 @@ +# Known friction + +## Fixture `.env` policy noise + +`visibility.toml` flags any path containing `.env`, so every branch touching +`fixtures/repo/head/secrets/.env` gets a CLOAKED verdict + exit 1 — even though +that file is an intentional test fixture (tracked on purpose, see `.gitignore` +exception). + +Correct behavior today, but it will fire on nearly every fixtures-touching +branch and train us to ignore exit codes. When it starts feeling like noise, +the fix is policy scoping — e.g. an ignore/exempt list in VisibilityPolicy for +`fixtures/` paths — not weakening the rule. + +First flagged: 2026-08-21, during the first dogfood run of oot on itself. From 3296ebf1f3fd0173c2eec6dbaf245b57785d898e Mon Sep 17 00:00:00 2001 From: kridaydave Date: Fri, 21 Aug 2026 22:58:22 +0530 Subject: [PATCH 07/11] fix: visibility checks only touched paths; add CI dogfood adjudication job --- .github/workflows/ci.yml | 21 +++++++++++++ src/visibility.rs | 65 ++++++++++++++++++++++++++++++++++++++-- 2 files changed, 83 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 11d5402..08ae024 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -61,3 +61,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/src/visibility.rs b/src/visibility.rs index a545aa2..4adb902 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,52 @@ 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"); + } } From f2c10d4f7cfd6170369d14439b3e70db4e4bad85 Mon Sep 17 00:00:00 2001 From: kridaydave Date: Fri, 21 Aug 2026 22:58:50 +0530 Subject: [PATCH 08/11] docs: mark .env policy noise resolved by touched-path fix --- TODO.md | 18 +++++++----------- src/visibility.rs | 9 ++++----- 2 files changed, 11 insertions(+), 16 deletions(-) diff --git a/TODO.md b/TODO.md index e98adc6..e3dbc6c 100644 --- a/TODO.md +++ b/TODO.md @@ -1,15 +1,11 @@ # Known friction -## Fixture `.env` policy noise +## ~~Fixture `.env` policy noise~~ RESOLVED 2026-08-21 -`visibility.toml` flags any path containing `.env`, so every branch touching -`fixtures/repo/head/secrets/.env` gets a CLOAKED verdict + exit 1 — even though -that file is an intentional test fixture (tracked on purpose, see `.gitignore` -exception). +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`. -Correct behavior today, but it will fire on nearly every fixtures-touching -branch and train us to ignore exit codes. When it starts feeling like noise, -the fix is policy scoping — e.g. an ignore/exempt list in VisibilityPolicy for -`fixtures/` paths — not weakening the rule. - -First flagged: 2026-08-21, during the first dogfood run of oot on itself. +No open items. diff --git a/src/visibility.rs b/src/visibility.rs index 4adb902..bd20803 100644 --- a/src/visibility.rs +++ b/src/visibility.rs @@ -186,14 +186,12 @@ mod tests { 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("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()); + head.files.insert("src/lib.rs".into(), "fn b() {}".into()); let change = Change { name: "feature/public".into(), @@ -215,7 +213,8 @@ mod tests { // Now modify the private file: it becomes touched and must flag. let mut head2 = change.head.clone(); - head2.files + head2 + .files .insert("secrets/key.pem".into(), "rotated".into()); let change2 = Change { From 03ad20993550b2352fe6725486659b59c41c6c7d Mon Sep 17 00:00:00 2001 From: kridaydave Date: Fri, 21 Aug 2026 23:53:15 +0530 Subject: [PATCH 09/11] fix: extract jj tarball into its own dir so tar cannot chmod /tmp --- .github/workflows/ci.yml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 08ae024..7f6a0f8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -43,8 +43,9 @@ jobs: - 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 - tar -xzf /tmp/jj.tar.gz -C /tmp - sudo mv /tmp/jj /usr/local/bin/jj + 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: | From 4928f9ce2a36dd0e831ff1c206ddfd8419fae7eb Mon Sep 17 00:00:00 2001 From: kridaydave Date: Sat, 22 Aug 2026 00:16:05 +0530 Subject: [PATCH 10/11] feat: pair renames by name-blank signature, summarize added files in docket --- src/engine/mod.rs | 206 +++++++++++++++++++++++++++++++++---------- tests/engine_test.rs | 79 ++++++++++++++++- 2 files changed, 238 insertions(+), 47 deletions(-) diff --git a/src/engine/mod.rs b/src/engine/mod.rs index 52a5e65..2647cce 100644 --- a/src/engine/mod.rs +++ b/src/engine/mod.rs @@ -76,12 +76,13 @@ impl Engine { )); } + 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]; + 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, @@ -91,13 +92,7 @@ impl Engine { )); } None => { - disputes.push(meaning( - &mut n, - path, - *h_row, - format!("added function `{}`", name), - Severity::Review, - )); + added.push((name, *h_row)); } _ => {} } @@ -107,7 +102,42 @@ impl Engine { .filter(|name| !head_fns.contains_key(*name)) .collect(); removed.sort(); - for name in removed { + + // 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, + *row, + format!("added function `{}`", new), + Severity::Review, + )); + } + } + for name in leftover_removed { disputes.push(meaning( &mut n, path, @@ -126,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, )); } @@ -213,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 @@ -235,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( @@ -316,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(_)) => { @@ -338,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, )); } @@ -376,13 +453,34 @@ fn meaning(n: &mut i32, path: &str, row: usize, detail: String, severity: Severi } } -/// Extract tracked functions as `name -> (source text, 1-based row)`, plus the +/// 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, Vec) { +) -> (HashMap, Vec) { let mut map = HashMap::new(); let mut duplicates = Vec::new(); if let Some(tree) = tree { @@ -394,7 +492,7 @@ fn extract_functions( fn collect( node: Node, source: &str, - map: &mut HashMap, + map: &mut HashMap, duplicates: &mut Vec, config: &LangConfig, ) { @@ -403,7 +501,8 @@ fn collect( if node.kind() == kind.node_kind { matched = true; if let Some(key) = config.function_key(kind, node, source) { - insert(key, node, source, map, duplicates); + let name_node = node.child_by_field_name("name"); + insert(key, node, name_node, source, map, duplicates); } } } @@ -425,7 +524,7 @@ fn collect( .utf8_text(source.as_bytes()) .unwrap_or("") .to_string(); - insert(key, body, source, map, duplicates); + insert(key, body, None, source, map, duplicates); } } // Do not recurse into nodes that yielded a function: nested definitions @@ -444,8 +543,9 @@ fn collect( fn insert( key: String, body: Node, + name_node: Option, source: &str, - map: &mut HashMap, + map: &mut HashMap, duplicates: &mut Vec, ) { if key.is_empty() { @@ -456,7 +556,24 @@ fn insert( 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; - slot.insert((src, row)); + // 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)); } } } @@ -482,7 +599,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 @@ -490,10 +609,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] diff --git a/tests/engine_test.rs b/tests/engine_test.rs index 92bb4f5..73dd4e4 100644 --- a/tests/engine_test.rs +++ b/tests/engine_test.rs @@ -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 mut 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 mut 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 mut engine = Engine::new().expect("Failed to initialize engine"); + + let mut 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"); From 69ba087de1b2018689b691f622559d0710cdd6a8 Mon Sep 17 00:00:00 2001 From: kridaydave Date: Sat, 22 Aug 2026 00:18:19 +0530 Subject: [PATCH 11/11] fix: FunctionMap alias and clippy nits --- src/engine/mod.rs | 10 +++++++--- tests/engine_test.rs | 8 ++++---- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/engine/mod.rs b/src/engine/mod.rs index 2647cce..a79893d 100644 --- a/src/engine/mod.rs +++ b/src/engine/mod.rs @@ -436,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) @@ -480,7 +484,7 @@ fn extract_functions( tree: Option<&Tree>, source: &str, config: &LangConfig, -) -> (HashMap, Vec) { +) -> (FunctionMap, Vec) { let mut map = HashMap::new(); let mut duplicates = Vec::new(); if let Some(tree) = tree { @@ -492,7 +496,7 @@ fn extract_functions( fn collect( node: Node, source: &str, - map: &mut HashMap, + map: &mut FunctionMap, duplicates: &mut Vec, config: &LangConfig, ) { @@ -545,7 +549,7 @@ fn insert( body: Node, name_node: Option, source: &str, - map: &mut HashMap, + map: &mut FunctionMap, duplicates: &mut Vec, ) { if key.is_empty() { diff --git a/tests/engine_test.rs b/tests/engine_test.rs index 73dd4e4..12c6e6f 100644 --- a/tests/engine_test.rs +++ b/tests/engine_test.rs @@ -233,7 +233,7 @@ fn test_engine_file_added_and_removed() { #[test] fn test_engine_rename_is_not_remove_add() { - let mut engine = Engine::new().expect("Failed to initialize engine"); + let engine = Engine::new().expect("Failed to initialize engine"); let mut base = Snapshot::default(); base.files.insert( @@ -263,7 +263,7 @@ fn test_engine_rename_is_not_remove_add() { #[test] fn test_engine_3way_rename_is_not_conflict() { - let mut engine = Engine::new().expect("Failed to initialize engine"); + 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 @@ -288,9 +288,9 @@ fn test_engine_3way_rename_is_not_conflict() { #[test] fn test_engine_added_file_summary_lists_functions() { - let mut engine = Engine::new().expect("Failed to initialize engine"); + let engine = Engine::new().expect("Failed to initialize engine"); - let mut base = Snapshot::default(); + let base = Snapshot::default(); let mut head = Snapshot::default(); head.files.insert( "src/newstuff.rs".to_string(),