diff --git a/.env.example b/.env.example index 0285181..420139b 100644 --- a/.env.example +++ b/.env.example @@ -279,6 +279,39 @@ PRXREF_MAX_CHUNKS=8 # a visible marker. Must be > 0. # PRXREF_TICKET_CONTEXT_MAX_CHARS=6000 +# Repository context (0.16.0): off (default) | diff | repo. off leaves every +# prompt, post, trace and default-verbosity log byte-identical to 0.15.0. +# diff adds cross-chunk definitions from other files already in the diff, +# plus diff-file entries, all read from the diff itself -- no repository +# reader is needed. repo also reads files outside the diff (import, +# path-convention and name-search definitions, plus contract excerpts) +# through the forge's repository reader when one is available, or +# --repo-dir. Matching is exact and case-sensitive, like PRXREF_FAIL_ON; any +# other value is a configuration error (exit 2). +# PRXREF_REPO_CONTEXT=off + +# Repository context (0.16.0): per-chunk character budget shared by the +# cross-chunk, contract and other repository-context entries. Must be > 0. +# PRXREF_REPO_CONTEXT_MAX_CHARS=12000 + +# Repository context (0.16.0): globs (matched like PRXREF_SIZE_IGNORE_GLOBS) +# selecting the contract files -- OpenAPI, JSON Schema, Liquibase/SQL +# migrations -- excerpted under "repo". A set value REPLACES the built-in +# set below rather than adding to it; an empty value reads as unset, so the +# built-in set stays -- there is no way to turn contract excerpts off on +# their own in 0.16.0 short of setting PRXREF_REPO_CONTEXT to off or diff. +# Built-in set: **/openapi*.y*ml, **/openapi*.json, **/openapi/**, +# **/swagger*, **/*.schema.json, **/db/changelog/**, **/db/migration/**, +# **/migrations/** +# PRXREF_CONTEXT_CONTRACT_GLOBS= + +# Repository context (0.16.0): globs (matched like PRXREF_SIZE_IGNORE_GLOBS) +# whose paths are never read for repository context, not even a diff file. +# ADDED to a floor that is always on: **/expected.json, **/cases.json, +# **/case.json, **/prxref-eval/**, **/.env*, **/*.pem, **/*.key. Empty +# (the default) adds nothing. +# PRXREF_CONTEXT_EXCLUDE_GLOBS= + # Jira base URL (scheme://host plus any context path) that ticket fetches are # looked up on, overriding a ticket URL's own base (a self-hosted board often # sits behind a different REST host than its browse URL). Jira credentials diff --git a/CHANGELOG.md b/CHANGELOG.md index 4fc9ff0..83d1c73 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,163 @@ this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm Issue numbers in entries before 0.14.0 refer to the project's previous issue tracker. +## [0.16.0] — 2026-09-25 + +The repository-context release (#17). A chunk worker can now see code outside +its own hunks: definitions from the pull request's other files and from files +the diff never touches, and excerpts of the API and database contracts a +change points at. It is off by default. `PRXREF_REPO_CONTEXT=diff` adds +definitions found in the pull request's own files, and `repo` also reads the +rest of the repository, through the forge or through a local checkout given +with `--repo-dir`. At `off`, the prompts, posted comments, trace and logs are +byte-identical to 0.15.0; the run record and `--format json` gain one key, +`repo_context`, which is `null`. + +### Added + +- **Repository context (#17).** `PRXREF_REPO_CONTEXT` sets how much of the + repository a chunk worker sees beyond its own hunks: `off` (the default), + `diff` or `repo`. `diff` shows the definitions that a chunk's added lines + reference from the pull request's other files and, for a type another chunk + changes, the changed lines inside it. Those files are read at the PR head + through the forge or `--repo-dir`; with no reader, the entries come from the + diff's hunk lines alone. `repo` also reads files outside the diff: + definitions found through a file's imports, through Java's same-package + path convention and by a search of the repository's file names, plus + contract excerpts. Definitions extend the prompt's `### Definitions + referenced by this chunk` block, and contract excerpts form a new + `### Contract excerpts` block after it. Only chunk workers get repository + context: the whole-PR sweep prompt is unchanged. At `repo`, a run with no + repository reader, or with no file listing, logs one WARNING saying what it + runs without. Matching is exact and case-sensitive, and any other value + exits 2 naming the variable. +- **Java definitions (#17).** 0.15.0 showed a chunk the definitions it + references for JavaScript, TypeScript and Python only. At `diff` and `repo`, + Java type declarations (`class`, `interface`, `record`, `enum` and + `@interface`, never a method or a field) are found too, in the chunk's own + Java files outside its hunks as well as in other files. At `off`, Java still + gets none. +- **`PRXREF_REPO_CONTEXT_MAX_CHARS` (#17).** The repository-context entries of + one chunk share a character budget, default `12000`, which must be greater + than 0. An entry costs the length of its `path:line: text` line. Entries are + admitted in the rank order of their `reason`: `cross-chunk`, `contract`, + `diff-file`, `import`, `path-convention`, then `name-search`, and within a + rank by path and line. Admission stops at the first entry that does not + fit, and a `… N more context entries omitted` line, not counted against the + budget, ends the block of the first entry left out. +- **Contract excerpts and `PRXREF_CONTEXT_CONTRACT_GLOBS` (#17).** At `repo`, a + chunk whose added lines use a route, an operation id, a table, or a name + that matches a schema gets the matching slice of the repository's contract + files: an OpenAPI operation or schema, a JSON Schema, or an earlier + migration of a changed one. `PRXREF_CONTEXT_CONTRACT_GLOBS` picks the + contract files once per run; its built-in set is `**/openapi*.y*ml`, + `**/openapi*.json`, `**/openapi/**`, `**/swagger*`, `**/*.schema.json`, + `**/db/changelog/**`, `**/db/migration/**` and `**/migrations/**`. A set + value replaces the built-in set instead of adding to it, and an empty value + keeps it. A chunk reads at most 6 spec files (`.yaml`, `.yml` or `.json`) + and, for each changed migration, at most the 4 nearest earlier migrations in + its directory; each excerpt is capped at 40 lines and 2,000 characters. +- **`PRXREF_CONTEXT_EXCLUDE_GLOBS` and an exclude floor (#17).** Repository + context never lists, reads or shows a path under a floor that is always on: + `**/expected.json`, `**/cases.json`, `**/case.json`, `**/prxref-eval/**`, + `**/.env*`, `**/*.pem` and `**/*.key`, which keeps eval labels, eval output, + dotenv files and keys out of the prompt. `PRXREF_CONTEXT_EXCLUDE_GLOBS` adds + globs to that floor; a `!` negation in it never re-admits a floor path. + Neither list hides a changed file's hunks, which still reach the prompt as + the diff. +- **Bounded repository reads (#17).** One reader serves the whole run: each + path is fetched at most once, and the file listing once. Reads of paths + outside the pull request's own files are capped at 16 per chunk and 200 per + run; a read past a cap is skipped, so the chunk gets fewer entries, and the + run record's `read_cap_hit` says so. The pull request's own files are read + without a cap, so the entries built from them never depend on which chunk + read a file first. +- **`--repo-dir PATH` (#17).** `prxref review --repo-dir PATH` names a local + checkout of the repository at the PR head. With `PRXREF_REPO_CONTEXT=repo`, + repository context reads and lists files there instead of calling the forge + (at `diff` it only reads there), so a `--diff-file` review gets repository + context with no network. A path that is not an existing directory exits 2 + before any network call. It is not a replay flag: on its own it neither + stops posting nor adds a `replay` stamp. +- **Repository context in `prxref eval` (#17).** A `cases.json` case takes an + optional `repo_dir`, read relative to the file, and a `case-*/` directory + takes a `repo/` directory; either is the case's `--repo-dir`. A `repo_dir` + that is not an existing directory exits 2 naming it. `run.json` records the + four new settings, so two runs that differ only in `PRXREF_REPO_CONTEXT` can + be told apart and compared. +- **`repo_context` in the run record (#17).** The run record and + `--format json` gain `repo_context`, always present and `null` at `off`. On, + it holds the level, the budget, both glob lists, the reader (`forge`, + `repo-dir` or `null`), the listing's path count and whether it is complete, + the read count, `read_cap_hit`, and one row per chunk naming each admitted + entry's path, line, symbol, kind, reason and length, how many entries were + left out, and whether the timeout retry dropped them. It never holds file + text. The JSONL trace gains one `chunk context` event per chunk and one + `repo_context ok` event per run, and in text output `-v` prints a + `repo context:` line with the level, the reader, the listing size, the read + count, whether a cap was hit, and the entries admitted and left out. With + the feature off none of them appears. +- **Repository listing on every forge (#17).** Each forge adapter can now list + the repository's files at a commit, which `repo` does once per run. GitHub + reads `GET /repos/{owner}/{repo}/git/trees/{sha}?recursive=1` in one request. + GitLab walks GraphQL's `project.repository.tree(recursive: true).blobs`, 100 + files a page, and falls back to the REST `repository/tree?recursive=true` + walk when the first GraphQL page is unusable. Bitbucket Cloud walks + `/2.0/repositories/{owner}/{repo}/src/{sha}/?max_depth=64&pagelen=100`, + Bitbucket Server walks `/rest/api/1.0/projects/{key}/repos/{slug}/files` at + the commit, 100 paths a page, and Azure DevOps reads the `items` endpoint + with `recursionLevel=Full` at the commit in one request. A paged walk stops + at 20 pages. Only files are listed: directories and submodules are dropped, + though whether Bitbucket Server returns submodules is unverified. A listing + that stopped early is marked incomplete (`complete: false` in the run + record, `(partial)` on the `-v` line), a listing whose first request fails + is none at all, and neither fails a review. See `docs/forges.md`. + +### Fixed + +- **An empty model reply is now asked for once more.** A reply with no text, or + only whitespace, is sent again once with the same prompt and the same + budget, after one WARNING (`: empty model reply + (finish_reason=); retrying once`). This covers chunks and the + systemic sweep. A reply the provider stopped at the budget (`finish_reason` + `length` or `max_tokens`) is not retried, because it already names + `PRXREF_LLM_MAX_TOKENS`. Neither is a non-empty reply that fails to parse, + nor a call that raised. Token counts and elapsed time cover both calls, and + the reported cost is their sum, or unknown when either call reported none. + In 0.15.0 and earlier the unit failed with `no parseable content` even + though the provider billed it; the review of 0.15.0's own pull request lost + 1 of 8 chunks this way. + +### Known limitations + +- **Read caps cut the lowest-ranked sources first.** A chunk issues its capped + reads in rank order, contract files before the files that imports, the path + convention and the name search point at, so those definitions are the first + left out once a chunk has made 16 reads or the run 200, and which chunk + meets the run cap first depends on thread scheduling. +- **The name search matches file names, not file contents.** Outside the pull + request, a definition is found only through an import, the Java path + convention, or a same-language file whose name, less its extension, matches + the referenced name, so a type declared in a differently named file is not + shown. +- **A file over 512 KiB is read as missing.** Every forge and `--repo-dir` + refuse a file past that size, so a large OpenAPI spec gives no excerpt. +- **A large repository is listed only in part.** A paged listing stops at 20 + pages and GitHub truncates the tree of a very large repository, so there the + name search and the contract globs see only the files listed; on a large + GitLab project the listing can take about a minute, once per run, at `repo` + only. +- **The Bitbucket Server listing is untested live.** It is built from the + Bitbucket Data Center REST documentation, tested against fakes of that + shape, and has not been run against a live instance. +- **A chunk that times out loses its repository context.** The timeout retry + drops the definitions and contract blocks to shrink the prompt, so with a + model slower than `PRXREF_LLM_TIMEOUT` (default `45` seconds) a chunk is + reviewed without them; raise the timeout for a slow model. +- **A pull request's file can be fetched twice.** The dependency and same-file + definition blocks of 0.15.0 keep their own reader, so a file both they and + repository context read costs two requests per run. + ## [0.15.0] — 2026-09-24 The tuning release. A team can now replace the review prompts, scope its review @@ -1580,7 +1737,9 @@ Development baseline. Never published to PyPI and never tagged; superseded by - Diff content is sent to whichever OpenAI-compatible endpoint you configure. - Requires Python 3.12+. Tested on 3.12 and 3.13. -[Unreleased]: https://github.com/sblattj/prxref/compare/v0.14.0...HEAD +[Unreleased]: https://github.com/sblattj/prxref/compare/v0.16.0...HEAD +[0.16.0]: https://github.com/sblattj/prxref/releases/tag/v0.16.0 +[0.15.0]: https://github.com/sblattj/prxref/releases/tag/v0.15.0 [0.14.0]: https://github.com/sblattj/prxref/releases/tag/v0.14.0 [0.13.0]: https://github.com/sblattj/prxref/releases/tag/v0.13.0 [0.12.2]: https://github.com/sblattj/prxref/releases/tag/v0.12.2 diff --git a/HANDOFF.md b/HANDOFF.md index b21aebb..a8d6a30 100644 --- a/HANDOFF.md +++ b/HANDOFF.md @@ -1,191 +1,193 @@ -# HANDOFF — v0.15.0 shipped: the tuning release +# HANDOFF — v0.16.0 shipped: repository context -**Repo:** `sblattj/prxref` (public) · **Released:** 2026-09-24 · **Supersedes** the -v0.14.0 handoff. +**Repo:** `sblattj/prxref` (public) · **Released:** 2026-09-25 · **Supersedes** the +v0.15.0 handoff. -0.15.0 gives a team the knobs to tune a review, and a harness to measure what -the tuning did. The knobs are prompt template overrides, path-scoped review -rules, finding grouping with per-severity caps, a per-rule cap that a review -rules file turns on, and an opt-in tier for reworded duplicates. The harness is -`prxref eval run|score|compare`. GitHub pull requests past the diff endpoint's -size limit are now reviewed, and a replay pins the PR's title and description to -the time it replays. The user-facing account is the `[0.15.0]` section of -`CHANGELOG.md`. This file is for whoever cuts the next release. The v0.14.0 -handoff is in git history. +0.16.0 lets a chunk worker see code outside its own hunks (#17): definitions +from the pull request's other files and from files the diff never touches, and +excerpts of the API and database contracts a change points at. +`PRXREF_REPO_CONTEXT` turns it on at `diff` or `repo`. It is off by default, +and at `off` the prompts, posted comments, trace and logs are byte-identical to +0.15.0. The release also retries an empty model reply once. The user-facing +account is the `[0.16.0]` section of `CHANGELOG.md`. This file is for whoever +cuts the next release. The v0.15.0 handoff is in git history. ## What landed -- **#10 Reworded duplicates.** `quality.apply_sweep_dedup(..., similarity=)` - has a second tier that runs after the exact one. It treats two active - findings in the same file on the same line as duplicates when - `quality.titles_similar` holds. That test is a Jaccard index over title - tokens (`quality.title_similarity`). `PRXREF_DEDUP_SIMILARITY` sets the - threshold, and leaving it unset skips the tier. A chunk copy always outlives - a sweep copy, and the tier never lowers a review's worst severity. -- **#11 Prompt template overrides.** `prompt_templates.load_prompt_templates` - reads `worker.md`, `systemic.md` and `summary.md` from `--prompts-dir` / - `PRXREF_PROMPTS_DIR`. A template that is not there falls back to the packaged - one. An override must carry every placeholder the packaged template has after - its context marker, except those in `OPTIONAL_PLACEHOLDERS`; a missing one - exits 2. `prxref prompts export` (`cli._cmd_prompts_export` → - `export_prompt_templates`) writes the packaged templates out as a starting - point. The run record's `prompt_templates` lists each override. -- **#12 Path-scoped rules.** `--scoped-rules` / `PRXREF_SCOPED_RULES` name rules - files whose `applies_to:` front-matter globs pick the chunks each one reaches. - They add to `PRXREF_REVIEW_RULES` and never replace it. Start at - `rules.load_scoped_rules`, `rules.match_globs` (where `**/` also matches zero - directories) and `ScopedRules.select` / `unit_block`. `unit_block` fits the - rules into `PRXREF_SCOPED_RULES_MAX_CHARS`. The orchestrator builds one block - per chunk in `_scoped_unit_blocks`, and the sweep gets the union. -- **#13 Grouping and per-severity caps.** With `PRXREF_GROUP_FINDINGS` on, the - worker prompt asks each finding for a `rule` (`triage.normalize_rule`, - `orchestrator._enforce_rule`). `quality.apply_rule_grouping` then folds the - chunk findings that break one rule in one file into one representative. Its - body ends `Also at:` and its `locations` lists the other lines. - `--format json` emits `rule` and `locations` (`cli._finding_json`), with - `locations` null on an ungrouped finding unless the per-rule cap (#18) - folded others into it. `PRXREF_MAX_WARNING_FINDINGS` and - `PRXREF_MAX_OUTOFSCOPE_FINDINGS` cap those two severities in - `quality.apply_quality_gate`, ranked by `quality.finding_rank_key`, and every - cap counts groups. -- **#14 Eval harness.** `prxref eval run|score|compare` goes from - `cli._cmd_eval` to `evals.eval_run`, `eval_score` and `eval_compare`. `eval` - adds no environment variable, since every setting is a flag, and it never - posts. `eval_cases.load_cases` reads a case: a diff file or a pinned `pr_url` - range, plus its expected labels. `eval_metrics` grades a label that has a - `must_match` predicate deterministically. Every other label goes to an LLM - judge, whose code is in `judge.py` and `eval_judge.py` and whose prompt is - `prompts/judge.md`. `compare` also warns when two runs' replay description - stamps differ. -- **#15 GitHub PRs past the diff limit.** In `forges/github.py`, `get_diff` - handles a 406 whose error code is `too_large` by calling - `_get_diff_past_the_limit`. That reads `get_pr` and then - `get_compare_diff(base.sha...head.sha)`. The compare diff is accepted only - when `_compare_mismatch` finds its file count and its `+`/`-` line sums equal - to the PR's `changed_files`, `additions` and `deletions`. On a mismatch or any - HTTP or transport failure, it logs one WARNING and falls back to - `_get_diff_from_files`. That pages `/pulls/{number}/files` and renders the - diff through `forges/_diff_render.render_diff_entries`. The fallback fails - closed: a listing shorter than `changed_files`, or totals that disagree with - the PR's (`_refuse_short_totals`), end the run as `Error`, never as a partial - review. -- **GitHub file content.** Start at `get_file_content` in `forges/github.py`. - `_is_json_envelope` judges the response by its media type: a type in - `_RAW_MEDIA_TYPES` is the file, and `application/json` or any other `+json` - type is an envelope that reads as no content. `orchestrator._make_file_reader` - wraps the adapter's reader in a per-run cache and feeds it to the chunk - context blocks (dependency versions and symbol definitions) and to - `quality.apply_manifest_claim_check`. The bug itself is under "Fixes found on - the way" below. -- **#16 Replay pins the title and description.** A `--pr-url` replay goes - through `cli._replay_forge` → `_resolve_description`. That reads the forge's - `get_pr_history` once: GitHub over GraphQL, Bitbucket Cloud over `/activity`. - `forges/replay.choose_cutoff` then picks the cutoff: `--as-of`, else the first - human review, else the head commit's date. `ReplayForge` shows the title and - description in force at the cutoff (`pin_pr_metadata`). `--description-file` - and `--no-description` fix the description instead. Every other outcome - replays the live text and logs a WARNING that says why. The run's `replay` - stamp records `description` (`pinned`, `live`, `file` or `none`) with the - cutoff and its source. -- **#18 Per-rule cap.** Start at `quality.apply_rule_cap`. `orchestrate_review` - calls it through `_cap_rules`, after the grouping pass and before - `apply_quality_gate`, only when `rule_cap_active` holds: - `max_findings_per_rule` (`PRXREF_MAX_FINDINGS_PER_RULE`, default 2) is above 0 - and `rules` or `scoped_rules` is not `None`. The same flag turns on #13's rule - request through `rule_active`, so a rules-file run asks every unit for a - `rule` with grouping off. The pass keys on the rule, or the normalized title, - across files, keeps the first `cap` ranked by severity and then - `finding_rank_key`, and folds the rest onto the first one's `locations` and - `Also at:` paragraph. `quality.rule_cap_counts` builds the run record's - `rule_counts`. -- **Fixes found on the way.** - - `quality.apply_example_echo_check` drops a finding whose title echoes a - prompt template's example finding. It is the first pass that drops - anything. - - `worker.md` lost a false sentence claiming the input "stays under roughly - 30k tokens". - - The GitHub adapter's `get_file_content` took GitHub's raw media type, - `application/vnd.github.raw+json`, for a JSON envelope and dropped every - file it read, so GitHub reviews got no full-file context. Earlier releases - are affected too. `_is_json_envelope` now decides by the media type. - - `orchestrator._split_at_sweep` re-derives the chunk/sweep boundary across - the quality gate by counting the chunk side. The old walk counted the sweep - side, and because the gate's stable sort puts a chunk copy ahead of its - identical sweep twin, it swapped every twin pair: a sweep finding repeating - a grouped or rule-capped chunk finding was posted twice. - `tests/test_sweep_boundary_drops.py` pins the gate's tie order it relies on. -- **Config went from 55 to 63 keys.** The eight new keys are: - - `PRXREF_DEDUP_SIMILARITY` (#10) - - `PRXREF_PROMPTS_DIR` (#11) - - `PRXREF_SCOPED_RULES` and `PRXREF_SCOPED_RULES_MAX_CHARS` (#12; the second - defaults to 24,000) - - `PRXREF_GROUP_FINDINGS`, `PRXREF_MAX_WARNING_FINDINGS` and - `PRXREF_MAX_OUTOFSCOPE_FINDINGS` (#13) - - `PRXREF_MAX_FINDINGS_PER_RULE` (#18; defaults to 2, and applies only while - a review rules file is loaded) - - The first seven are off or unset by default. The per-rule cap is on by - default whenever a review rules file is loaded. No existing config default - changed. +- **#17 Repository context, as a module map.** The first five modules below + do no I/O and import nothing from `prxref.forges`: they take a + `read(path)` callable plus plain data. + - `repo_unit.build_unit_context` is the place to start. It builds one + chunk's `UnitContext` from the sources below, ranks the entries by + `repo_context.REASONS` (`cross-chunk`, `contract`, `diff-file`, `import`, + `path-convention`, `name-search`), admits them into + `PRXREF_REPO_CONTEXT_MAX_CHARS`, and writes the `… N more context entries + omitted` line. `repo_unit.MODES` is the vocabulary: `off`, `diff`, `repo`. + - `repo_context`: `ContextEntry`, `find_definitions`, `language_of` + (`chunk_context`'s map plus `.java`, with `_JAVA_DEF_RE` for type + declarations), and the exclude floor, `EXCLUDE_FLOOR` and + `exclude_predicate`. + - `repo_crosschunk.diff_definitions`: definitions from the pull request's + other files, and, for a type another chunk changes, up to + `MAX_CHANGE_LINES` of its changed lines. + - `repo_resolve.resolve_candidates`: files outside the diff, found through + imports (Java, Python, TypeScript and JavaScript), Java's same-package + path convention, and the name search over the listing. + - `repo_contracts`: `contract_triggers` reads routes, operation ids, tables + and schema names off the added lines; `select_contract_files` and + `earlier_migrations` pick the files; `contract_excerpts` dispatches to the + OpenAPI, JSON Schema, SQL and Liquibase excerpters. + - `repo_reader.RepoReader` is the one reader a run shares, built by + `forge_reader` or `repo_dir_reader`. It fetches each path and the listing + at most once, single-flight across threads, and keeps the counters behind + the record's `reads` and `read_cap_hit`. + - `forges/repo_dir.RepoDir` reads and lists a `--repo-dir` checkout. + - `forges/base.PathListing(paths, complete)` and the optional + `Forge.list_paths(ref, *, sha)`, implemented by all five adapters. + `forges/replay.ReplayForge.list_paths` delegates at the pinned head. + - `orchestrator`: `_plan_repo_context` builds the run's reader, takes the + listing and selects the contract files once, and logs `repo`'s one + WARNING; `_routed_read` routes each read (lesson 3); each + chunk worker calls `repo_unit.build_unit_context`; `_repo_context_record` + and `_unit_row` build the record, `retry_dropped` included; and the + `chunk context` and `repo_context ok` trace events. + - `chunk_context.render_context_blocks` takes `extra_def_lines` and + `contract_lines`, the second under the new `CONTRACT_HEADER` + (`### Contract excerpts`). With both empty its output is unchanged. + - `cli`: `--repo-dir` (`_open_repo_dir`, which exits 2 before any network + call), the `repo_context` JSON key after `rule_counts`, and the `-v` line + (`_repo_context_line`). + - `eval_cases` and `evals`: a case's `repo_dir` field or `case-*/repo/` + directory, and four more `evals.RUN_CONFIG_KEYS` (16 in all). +- **Entry points and caps.** + - `orchestrate_review` takes `repo_context="off"`, + `repo_context_max_chars=12000`, `context_contract_globs=()`, + `context_exclude_globs=()` and `repo_dir=None`. `cli._run_review` passes + the four config values by name, and `repo_dir` from `--repo-dir` or the + eval case. + - Reads: `repo_reader.MAX_CHUNK_READS` (16) and `MAX_RUN_READS` (200) cap + the reads of paths outside the pull request; its own files are uncapped. + - Listing: `forges/base.MAX_LISTING_PAGES` (20) pages; `RepoDir` lists at + most 100,000 files (`forges/repo_dir._MAX_LISTED_FILES`); Bitbucket Cloud + walks at `max_depth=64` (`forges/bitbucket._LISTING_MAX_DEPTH`). + - Contracts: `repo_contracts.MAX_SPEC_FILES` (6), `MAX_EARLIER_MIGRATIONS` + (4), `MAX_CONTRACT_LINES` (40) and `MAX_CONTRACT_CHARS` (2,000). + - Definitions: `repo_resolve.MAX_NAME_SEARCH_PER_NAME` (3) and + `repo_crosschunk.MAX_CHANGE_LINES` (12). + - Files over 512 KiB read as missing, on every forge and in `--repo-dir` + (`forges/repo_dir._MAX_FILE_CONTENT_BYTES`). +- **GitLab lists through GraphQL.** `forges/gitlab.py` `list_paths` walks + `project.repository.tree(recursive: true).blobs`, 100 files a page + (`_list_paths_graphql`). It falls back to the REST + `repository/tree?recursive=true` walk (`_list_paths_rest`) only when the + first GraphQL page is unusable, including the empty page GraphQL answers + for a sha it cannot resolve. A later GraphQL page that fails returns the + paths read so far with `complete=False`. Lesson 1 says why. +- **An empty model reply is retried once.** `reviewer._invoke_and_parse` asks + again, with the same prompt and budget, when `_is_empty_reply` holds and the + provider did not stop at the budget (`finish_reason` `length` or + `max_tokens`). Chunks and the sweep share the path. `_fold_retry_usage` sums + both calls' tokens and costs, and the unit's `model` is the second call's. A + retry that raises fails the unit and keeps the first call's usage. The worst + case is 4 `llm.invoke` calls for a chunk (this retry times the + orchestrator's one timeout retry) and 2 for the sweep. +- **Config went from 63 to 67 keys.** The four new keys: + - `PRXREF_REPO_CONTEXT` (`off`, `diff` or `repo`; default `off`) + - `PRXREF_REPO_CONTEXT_MAX_CHARS` (default 12,000; must be above 0) + - `PRXREF_CONTEXT_CONTRACT_GLOBS` (default the built-in set; a set value + replaces it) + - `PRXREF_CONTEXT_EXCLUDE_GLOBS` (default empty; adds to the always-on + floor) + + The last three do nothing while `PRXREF_REPO_CONTEXT` is `off`, its + default. No existing config default changed. ## What this release taught Written down because each one cost real time. -1. **A new run-record or JSON key moves eight test pins in three files.** +1. **A live listing check found what the fakes could not.** GitLab's REST + `repository/tree?recursive=true` returns every directory before any file, + across the whole recursive walk. On gitlab-org/gitlab, all 2,100 entries + read (the 20-page cap plus one probe page) were directories, so the capped + walk listed 0 files. GraphQL's `tree.blobs` lists files only, and the same + project now lists 2,000 files. A fake built from the documented response + shape carries no ordering; record a real response past the page cap before + trusting a paged walk. +2. **"Off is byte-identical" is proven against the previous release, never + the tree.** `tests/fixtures/issue17/golden_off_prompts.json` was captured + by `make_golden.py` under `uv run --no-project --with prxref==0.15.0`, from + outside the repository. `TestOffMatchesTheReleased015` compares the tip's + `off` prompts and reads with it byte for byte, over two design points and + three keyword-argument variants. + - `test_the_golden_is_a_0_15_0_oracle_that_exercises_the_old_blocks` pins + the golden's provenance: `prxref_version` and `generated_from: + installed distribution`. + - `test_control_the_same_comparison_fails_for_repo` shows the comparison + can fail. + - A golden written from the tree would only compare the tree with itself. + When a later release changes an `off` prompt on purpose, capture the + golden again the same way, from the release the claim is now made + against, and move the version the provenance test pins. +3. **Reads of the pull request's own files are uncapped.** With one per-chunk + cap for every read, a chunk with many diff files spent its 16 reads before + it reached its contract file. `orchestrator._routed_read` now sends diff + paths to the uncapped `RepoReader.read`, and only other paths to the + capped `chunk_reader`. `TestReadCapStarvation` in + `tests/test_issue_17_acceptance.py` pins it, with a control that routes + every read through the capped reader and loses the spec. It also removed a + dependence on thread scheduling: which chunk reads a shared diff file first + no longer changes any chunk's entries. +4. **A thinking model needs more than prxref's defaults.** GLM 5.3 Flash with + thinking on, reached through a reasoning lane in llm-ferry, did not fit + `PRXREF_LLM_MAX_TOKENS` 4096 and `PRXREF_LLM_TIMEOUT` 45. + - At 4,096 tokens a smoke call stopped with `finish_reason=length`: one + spent 4,094 tokens on reasoning and returned no answer, another spent + 3,902 and returned a cut-off one. + - On the #17 eval fixture, 16,384 tokens and a 600 s timeout completed + every call. The largest reply used 11,433 completion tokens. + - On a larger pull request, 16,384 tokens was not enough: a chunk of + 11,161 input tokens ran out of room after 357 s. At 32,768 tokens and a + 900 s timeout the same chunk completed in 345 s, with 15,265 output + tokens. + - Single calls took 90 to 390 s. + + The timeout retry drops the repository-context blocks, so a timeout that + is too short quietly removes the feature. One small probe, 4 calls a + point, is an observation, not a measurement: `reasoning_effort` `low` and + unset gave overlapping reasoning-token ranges (212 to 800 and 496 to + 1,054), and a request with thinking disabled was answered by a different + model. +5. **`-v` summary lines are text output only.** With `--format json`, + `cli._cmd_review` prints the JSON object and skips `_print_summary`, as + 0.15.0 already did for every `-v` line. A live check scripted against + `--format json -v` found no `repo context:` line. Read the `repo_context` + key instead, or check the line through `cli._repo_context_line` + (`TestVerboseLine` in `tests/test_cli_repo_context.py`). +6. **Know what repository context cannot reach.** + - The name search matches a file name in the same language (ignoring + case, and also in snake case for Python), never file content. A type + declared in a differently named file is not found (`TestNameSearch` in + `tests/test_repo_context_resolve.py`). + - Only chunk workers get repository context. The sweep prompt is the same + at every level: `test_control_the_same_comparison_fails_for_repo` finds + every worker prompt changed at `repo` and the sweep's unchanged. + + A check that looks for either finds nothing, by design. +7. **A new run-record or JSON key moves twelve test pins in five files.** + 0.15.0's count of eight in three files was already short: the #12 and #18 + tests pin the key order too. - `tests/test_orchestrator.py`: three `set(res) == {...}` literals and `RESULT_KEYS`. - `tests/test_run_record.py`: `RECORD_KEYS` and `NULL_WHEN_OFF`. - `tests/test_cli_output.py`: `JSON_KEYS` and `NEW_RECORD_KEYS`. + - `tests/test_orchestrator_rule_cap.py`: `TestJsonOutput`'s next-key + assertion, and `TestOffPathMatchesBase`'s key set and JSON key list. + - `tests/test_cli_scoped_rules.py`: `TestJson`'s `sampling` offset. - All eight spell out the full key set, and the key order is pinned too. A new - exit from `orchestrate_review` also moves `TestOneChokePoint`'s count. -2. **Trace-event order is nondeterministic.** Chunk workers run on a - `ThreadPoolExecutor` capped at `max_workers`. A test that asserts trace - events must compare them as a multiset, or pin `max_workers=1`. -3. **A new prompt slot belongs in `prompt_templates.OPTIONAL_PLACEHOLDERS`.** - An override must carry every other placeholder the packaged template has. So - a slot left out of that set becomes required, and every existing override - that lacks it stops loading. A new slot also needs its `replace` line in - `tests/test_prompt_context.py`'s `_old_user`. -4. **The hub files take every issue's kwargs, so merge them one at a time.** - `orchestrator.py` was touched by six issues and `cli.py` by seven. Merging - one branch at a time kept them green. Each merge got a full gate, and each - kwarg-order conflict was resolved as the union of both sides. The order is - pinned by tests such as - `test_both_kwargs_are_keyword_only_and_follow_prompts` and the record-key - order tests in `tests/test_cli_prompts_dir.py` and - `tests/test_cli_scoped_rules.py`. -5. **With its feature off, a new key is present and null, and the output stays - byte-identical.** That was decided before any feature work, and every 0.15 - key keeps to it. The one deliberate exception is removing `worker.md`'s - false "stays under roughly 30k tokens" sentence. That moved every worker - user-prompt hash, including on runs with every feature off. It shows in the - trace, in `--trace-dir`, and in the golden tests of - `tests/test_rule_prompt_slot.py` and `tests/test_orchestrator_grouping.py`. - The system, sweep and summary-only hashes did not move. A run with a review - rules file moved further: the per-rule cap (#18) is on there by default, and - its rule request changes both prompt halves of every chunk and of the sweep, - which `PRXREF_MAX_FINDINGS_PER_RULE=0` undoes. Keep this in mind when - comparing hashes across 0.14 and 0.15 runs. -6. **Read a packaged template through `prompt_templates.packaged_text`.** The - stub `_contract_load_prompt` in `tests/test_orchestrator.py` asserts that the - orchestrator asks `reviewer.load_prompt` for the summary template only. - Route a new orchestrator read through `load_prompt` instead, and 248 tests - fail with a misleading `'worker.md' == 'summary'`. -7. **Mock the headers the server really sends.** The GitHub file-content bug - went unnoticed because the tests' success mocks answered - `text/plain`. github.com answers `application/vnd.github.raw+json`, and - only a live check showed it. So the `json` substring test that dropped - every file stayed green through five releases, 0.12.0 to 0.14.0. Copy - content types from a recorded response. -8. **An unverified API shape may add evidence, never veto verified evidence.** - Bitbucket Cloud's history reader was first built so that an unverified - snapshot field could fail the verified `changes.description` history of - every edited PR. The full gate was green, and only a final review caught it. -9. **A mutation check still needs `PYTHONDONTWRITEBYTECODE=1`.** Restore the - file and `cmp` it afterwards, or a same-size restore can run the cached - mutant again. + A keyword argument that the CLI builds rather than reads from config, as + `repo_dir` is, must also be named in + `test_run_review_passes_every_configured_orchestrate_kwarg`. ## The coupling that will catch the next person adding a config key @@ -207,41 +209,55 @@ together: A key that feeds `orchestrate_review` needs one more step. `tests/test_cli.py` (`test_run_review_passes_every_configured_orchestrate_kwarg`) requires `cli._run_review` to pass every orchestrator kwarg whose name equals a config -key. Of the 8 keys 0.15.0 added, 7 feed it this way; `prompts_dir` does not, -since `orchestrate_review` takes the loaded templates as `prompts`. Current -values: **63** keys, **1** legacy alias, **64** accepted names. +key. All 4 keys 0.16.0 added feed it this way. Current values: **67** keys, +**1** legacy alias, **68** accepted names. ## Release shape (follow this next time) -How 0.15.0 was built: - -1. **Maps and one plan first.** Each issue got a read-only map of the code it - would touch. One plan merged the maps, and the owner decided the cross-issue - questions before any code was written, including the rule that a new key is - null when its feature is off. -2. **Foundation.** One task landed every new config key, off by default, before - any feature work started. #18, which joined later, brought its own key. -3. **Lanes in waves.** Work was split into lanes by subsystem: quality (#10, - #13, #18), prompts (#11, #12), eval (#14), forge (#15) and replay (#16). - - Tasks ran in waves of parallel agents, each agent in its own worktree - against a pinned base commit. - - Each task was rated at most 5 of 10 for complexity and brought its own new - test file. - - 45 tasks merged in eight waves before the release documents were written: - the foundation, 5 forge, 9 quality, 11 prompts, 12 eval and 6 replay - tasks, and the example-echo fix. #18 joined the release after that, as 4 - more tasks in three waves: the pass and its config surfaces in parallel, - then the wiring, then the sweep-boundary fix its wiring turned up. +How 0.16.0 was built: + +1. **One map and one decisions file first.** #17 got a read-only map of the + code it would touch. A decisions file then settled every open question + before any code was written: off by default, `diff` without a reader + still adds its hunk-based entries, `list_paths` on all five adapters, + `--repo-dir` and the eval field, the contract glob set, the 512 KiB + ceiling and the 12,000-character budget. It also gave each new module one + owner and fixed the shared interface names, so parallel tasks merged + clean. +2. **Foundation.** One task landed all four config keys on every surface, + off by default, before any feature work started. +3. **Pure modules first, wiring last.** Tasks ran in rounds of parallel + agents, each agent in its own worktree against a pinned base commit. Each + task was rated at most 5 of 10 for complexity and brought its own new + test file. 20 tasks merged in seven rounds: + - 6: the config keys, the definitions core, the contract excerpters, the + `list_paths` Protocol with GitHub's listing, the `--repo-dir` reader, + and the acceptance fixture repository + - 5: the resolver, the cross-chunk definitions, the run reader, and the + listings of the other four forges + - 2: the contract triggers and file selection, and the eval case field + - 1: the per-chunk unit context + - 2: the orchestrator wiring, and the GitLab GraphQL listing that the + listing check brought in + - 3: the CLI and eval wiring, the acceptance tests through + `orchestrate_review`, and the empty-reply retry + - 1: the user documentation and the CHANGELOG section 4. **One integration gate per merge.** Each branch merged into `release/X.Y.Z` on its own. A merge stayed only if the full `uv run pytest` and - `uv run ruff check src tests` passed on the merged tree. -5. **Read-only live checks.** They ran against public PRs, with a guard that - blocked forge writes. Six ran: - - One found GitHub silently withholding patches once a listing page nears - 1 MB. That brought in the compare-first path before release. - - A later one found the GitHub file-content bug. -6. **Release.** Parallel tasks swept stale docs, bumped the version, and wrote - the CHANGELOG and this file. The file-content fix merged alongside them. + `uv run ruff check src tests` passed on the merged tree. Over the 20 + merges the passing count rose from 6,797 at 0.15.0 to 7,675 and never + fell; the documentation merge, the last, added no tests. +5. **Read-only live checks.** They ran against public repositories and pull + requests, with a guard that blocked forge writes, and reviews used one + model through one lane. Three ran: the listing on four live forges once + the listing tasks merged, then, once the wiring merged, `prxref eval` on + the #17 fixture and the bundled cases, and a review of one public pull + request end to end. The listing check found the GitLab ordering of lesson + 1, which brought in the GraphQL walk before release, and GitLab was + checked again after it. +6. **Release.** This commit bumps the version, dates the CHANGELOG and + rewrites this file, after the documentation task wrote the CHANGELOG + section. Cutting the release: @@ -267,9 +283,9 @@ pattern does not match GitHub's auto-generated source archive. ## Verified at release ``` -6797 passed uv run pytest -q +7675 passed uv run pytest -q All checks passed! uv run ruff check src tests -0.15.0 uv run prxref --version +0.16.0 uv run prxref --version ``` These counts come from the release branch, measured at the commit that last @@ -277,125 +293,144 @@ updated this file. ### Live checks -- **#15, before the fix: GitHub withholds patches.** sblattj/prxref#9 (127 - files, +32,003/-612) gets a 406 from the diff endpoint. An interim build - rebuilt it from the `/files` listing: the 110 files whose patch came back - matched the compare diff line for line, but 16 ordinary text files - (+6,880/-15, 21% of the added lines) came back at 0/0 with no patch at - `per_page=100`, and whole at `per_page=30`. That defect brought in the - compare-first path. 0.14.0 ends the same review as `Error`. -- **#15: GitHub's counts on 23 public PRs.** Across 11 repositories (binaries, - renames, a submodule, mode-only, empty and deleted files, 211- and 438-file - PRs, 48k-line lockfiles, 10 open PRs, 8 of them from forks), the `/files` - sums and the compare diff each matched the PR's counts on 23 of 23. The - compare diff was byte-identical to the PR diff on 22 of 22, resolved fork - SHAs on 17 of 17, and on 10 of 10 open PRs started from the base tip's merge - base. GitHub dropped the patch but kept the counts on 10 lockfile entries in - 4 ordinary PRs, so such a file is reviewed header-only with a warning. -- **#15: the shipped fix.** `get_diff` reads sblattj/prxref#9 (127 files, - +32,003/-612, refused at 20,000 lines) and ObiterDictum/obiter#226 (438 - files, refused at 300 files) whole from the compare endpoint in 3 GETs, with - no `/files` request; the listing alone would refuse #9 (+25,123/-597 against - +32,003/-612). The no-post review of #9 completes: `Request-Changes`, 8 - chunks plus the sweep, 3 findings, USD 0.0745. The same check found the - GitHub file-content bug fixed in this release: that review dropped 83 of 83 - file reads. -- **#11, #12 and #13 on pallets/click#3860.** It ran as 3 diff chunks at - `PRXREF_CHUNK_TOKEN_BUDGET=1500`. #12: each chunk got exactly the scoped - rules its globs matched, the sweep got both, and `--scoped-rules ""` turned - them off. #13: gpt-4o-mini named a rule on 20 of 20 raw findings and - gpt-4.1-mini on 20 of 26; 2 groups per model, `locations` matching `Also - at`; the warning cap left 1 warning each. #11: the record stamped the edited - `worker.md`'s SHA-256, and a copy without the marker exited 2 before any - network call. #10: 0 candidate pairs in 4 on/off pairs. USD 0.046 over 23 - runs. -- **Zero chunk findings on sblattj/prxref#9.** Chunk size is not the cause: 8 - chunks at 39,962-59,470 input tokens and 32 chunks at 9,488-19,532 both - returned 0 findings, as did an exact repeat and gpt-4.1-mini (0 of 56 chunk - calls). A positive control found 5 of 8 planted labels, all spec-tier, and 0 - of 3 generic bugs. `worker.md`'s "roughly 30k tokens" sentence was false at - defaults (8 of 8 chunks over it) and is removed. 9 of 12 gpt-4o-mini sweep - findings were `tests/evals/` fixture text. USD 0.323. -- **#16 on GitHub and Bitbucket Cloud.** On astral-sh/ruff#28750 (5 - description versions, 1 rename), `get_pr_history` read a complete history - ending at the live body. Two `--as-of` cutoffs pinned versions 2 and 3 with - the pre-rename title, no `--as-of` chose the first review, and with no token - the replay fell back to the live text with a WARNING naming - `PRXREF_GITHUB_TOKEN`. On Bitbucket Cloud, 4 of 4 cutoffs matched a hand - derivation, a 25-PR scan found live `changes.title` renames, a PR renamed - twice pinned at 6 of 6 cutoffs, and a two-page feed at 10 of 10. USD 0.0034. -- **This release's own pull request, sblattj/prxref#19.** GitHub refused its - unified diff with HTTP 406 (105 files, 33,096 changed lines), and a - `--no-post` review read the compare diff instead and reviewed all 105 files - in 8 chunks and the sweep. Every chunk prompt carried the full-file context - blocks (dependency versions, referenced definitions) that the GitHub - file-content fix restores. The example-echo pass dropped 1 sweep finding. - One chunk failed: gpt-4o-mini returned an empty reply with `finish=stop` - after 518 billed output tokens, and the review went on with - `chunks_failed: 1`. Verdict `Approved`, 38 s, USD 0.080. +- **`list_paths` on four live forges.** 19 of 19 checks passed, with no LLM + call. Each listing matched a raw walk exactly: sblattj/prxref on GitHub + (229 paths), gitlab-org/gitlab-test on GitLab (40, its 3 submodules + dropped), atlassian/forge-bitbucket-related-prs on Bitbucket Cloud (20), + and dnceng-public/public's dotnet-public-wiki on Azure DevOps (5). An + all-zero sha gave `None` on all four, reads were pinned to the head, and + torvalds/linux's truncated tree came back `complete=False` (67,498 of + 71,638 entries). Before the GraphQL walk, gitlab-org/gitlab listed 0 files + (lesson 1). Bitbucket Server has no public instance to check. +- **GitLab after the GraphQL walk.** Without a token, gitlab-org/gitlab-test + listed 40 paths, complete, in 0.4 s, and gitlab-org/gitlab listed 2,000 + paths in 57.4 s, stopped at the 20-page cap and marked incomplete. GitLab's + GraphQL endpoint honours `PRIVATE-TOKEN`: an invalid token gets 401, and no + header gets 200. +- **`prxref eval` on the #17 fixture and the bundled cases**, GLM 5.3 Flash + through `domestic.flash`, which served every call (19 of 19 header probes, + 0 fallbacks). 9 runs made 28 LLM calls with no failed case, at + `PRXREF_LLM_MAX_TOKENS=16384` and `PRXREF_LLM_TIMEOUT=600`; at prxref's + default of 4,096 tokens a smoke call was cut off (lesson 4). + - The fixture ran 3 times at `off` and 3 times at `repo`, reading its + `repo_dir`. Within each level the prompts were byte-identical. Between + the levels only the chunk prompt differed, by the added + repository-context block (62 lines, `### Contract excerpts` among them). + - The reader, the contract entries and the `off` null held in every run. + Each `repo` record named the `repo-dir` reader, its one chunk carried + both the `connectors.yaml` OpenAPI excerpt for the Java service and the + earlier migration for the SQL file, and every `off` record held + `repo_context: null`. The fixture's three files fit in one chunk at + default chunking, and the cross-chunk link needs two chunks: a run with + one file per chunk (3 chunks) confirmed it, with each contract entry in + its own file's chunk. + - An HTTP 201 status code that exists only in the injected OpenAPI excerpt + appeared in the findings of all 3 `repo` runs and of none of the 3 `off` + runs. The labels could not separate the levels: one matched in 0 of 3 + runs at `off` and 1 of 3 at `repo`, the other in none at either. + - On the three bundled cases, `diff` added no entry (single-file diffs, no + reader) and sent byte-identical prompts. Recall was 7 of 8 at both + levels, with two labels flipping in opposite directions. + - Each `run.json` recorded all 16 `RUN_CONFIG_KEYS`, the four new ones + included. +- **End to end on sblattj/multi-auto-claude-sub#5** (9 files, TypeScript), + GLM 5.3 Flash through `domestic.flash`, which served every call (8 of 8 + header probes, 0 fallbacks). + - `off` gave `repo_context: null`. + - `repo`, through the GitHub forge and through `--repo-dir`, built the same + 110 entries over 2 chunks, from a complete 64-path listing, with 22 reads + and no cap hit. 26 definitions from outside the diff reached the prompts, + 25 through imports and 1 through the name search. 1 entry was left out + under the 12,000-character budget, with its marker line. + - It needed `PRXREF_LLM_MAX_TOKENS=32768` and `PRXREF_LLM_TIMEOUT=900` + (lesson 4); at 16,384 an `off` run lost one of its two chunks. + - Findings: 2 at `off`, 3 through the forge and 0 through `--repo-dir`. The + last two runs sent identical prompts (27,466 input tokens each), so a + single run's finding count cannot measure the feature's effect. + - The forge run made 28 content reads for 22 paths: the double fetch in the + CHANGELOG's known limitations. The repository has no contract files, so + contract excerpts were not exercised live, and the empty-reply retry did + not fire. ## Still open — not part of this release -- **#17 Repository context outside the diff.** Mapped and planned for 0.16.0. -- **An empty model reply is not retried.** A reply with no text and - `finish=stop` fails its unit at parse time (`parser.py`, `no parseable - content`) and is not re-asked, though the provider billed it. The review of - this release's own pull request lost 1 of 8 chunks this way. 0.14.0 behaves - the same. +- **#20 JVM chunk context.** `chunk_context._language` still gives `.java` and + `.kt` no language, so the dependency-versions and same-file definitions + blocks skip JVM files at every level. Repository context adds Java type + declarations only (`repo_context.language_of`, `_JAVA_DEF_RE`). Methods, + fields, constants, Kotlin, and Maven or Gradle versions are open. +- **#21 A reply that does not parse is not retried.** The reviewer retries + only an empty reply (`reviewer._invoke_and_parse`, `_is_empty_reply`). A + reply that fails to parse, parses to a non-object, or has no `findings` + list still fails its unit. +- **Repository context's effect on findings is unmeasured.** + - On the #17 fixture, at 3 runs a level, one label matched in 0 of 3 runs + at `off` and 1 of 3 at `repo`, and the other in none. Sampling noise + explains that as well as the context does. The defect the second label + targets was raised as an active finding in 3 of 3 `off` runs and 1 of 3 + `repo` runs, never in the wording its predicate requires. + - The clearest sign that the model reads the context: an HTTP 201 status + code that exists only in the injected OpenAPI excerpt appeared in the + findings of all 3 `repo` runs and of none of the `off` runs. + - On sblattj/multi-auto-claude-sub#5, identical prompts gave 3 findings and + 0, so a single run's count cannot measure it. + + Measure with `prxref eval` over more runs a level before changing the + default from `off`. The known limitations, in full in the CHANGELOG: -- **#10 The reworded tier is unproven.** It ships off. Its suggested threshold - was chosen on invented negatives. A live check found no candidate pair in - eight runs, so the tier has not yet been seen to fire on a real PR. -- **#12 Chunks are not packed by language.** - - `triage.build_chunks` fills chunks by token budget and file cap. Among the - chunks with room, it prefers the one sharing the deepest directory - (`_shared_dir_depth`). - - So a chunk that holds two kinds of file gets the union of both files' - scoped rules. A live check saw exactly this. -- **#13 Grouping keys on the rule the model names.** - - A finding with no rule groups by title only. It never joins a group whose - findings name a rule, even with the same title in the same file. - - A member that line alignment moved to file level adds no `Also at` - location. - - The caps rank by `finding_rank_key` (confidence, then file path), not by - group size. So capping a representative drops all its folded locations from - the output. The same goes for the finding the per-rule cap (#18) folded a - rule's other findings into. -- **Findings on files outside the chunk.** A worker can report on a file it saw - only in the bounded `### Other files changed in this PR` excerpt - (`chunk_context.sibling_summary_block`). Location validation and line - alignment run over the whole PR's files, so such a finding is kept and - anchored like any other. Nothing marks it as coming from an excerpt. -- **#14 The judge shares the review's backend.** - - Only `--judge-model` changes which model judges. - - Self-judging is detected by exact model name. It warns and stamps - `self_judged`, and is never refused. - - Scoring needs human-labelled cases, and the repository ships three. -- **#15 Two ways a GitHub run past the diff limit can take long or fail.** - - A PR pushed to between the PR read and the listing pages can fail the - totals check. The run then ends as `Error`, never as a partial review. - - A compare read that times out is retried by the session. The retry policy - is `LoggingRetry(total=3)` with a 30-second read timeout, so reaching the - fallback can take up to four such timeouts. -- **#16 Only GitHub and Bitbucket Cloud read description history.** - - **Other forges.** On GitLab, Azure DevOps and Bitbucket Server, a replay - logs a WARNING and uses the live title and description. An explicit - `--as-of` there exits 2. - - **Unprobed endpoint.** GitHub Enterprise Server's GraphQL endpoint - (`https:///api/graphql`) has not been probed live. - - **Large PRs.** A PR with more than 5,000 reviews, comments or title renames - pins nothing, so it replays the live text even under `--as-of`. - - **Rate limit.** Bitbucket Cloud allows 60 anonymous reads an hour, and a - history read costs at least three. So a replay without a credential can - fall back to the live text. - - **Unverified Bitbucket Cloud shapes.** The `changes_requested` activity - entry has not been seen live (0 in 4 feeds) and is read by analogy with an - approval. It is also unverified whether the commit `date` used as the head - commit's date is the author date or the committer date. - - **CRLF.** GitHub stores some description versions with CRLF line endings, - and a pinned replay passes them to the prompt verbatim. +- **Read caps cut the lowest-ranked sources first.** Contract files are read + before import, path-convention and name-search files, so those definitions + go first once a chunk has made 16 reads or the run 200. Which chunk meets + the run cap first depends on thread scheduling. +- **No content search.** Outside the pull request, a definition is found + only through an import, the Java path convention, or a same-language file + named after it. +- **Files over 512 KiB read as missing**, so a large OpenAPI spec gives no + excerpt. +- **A large repository is listed only in part.** A paged listing stops at 20 + pages and GitHub truncates a very large tree, so the name search and the + contract globs see only the files listed. A large GitLab project's listing + takes about a minute, once per run, at `repo` only. +- **The Bitbucket Server listing is untested live.** It is built from the + Data Center REST documentation and tested against fakes of that shape. + Whether it returns submodules is unverified. +- **A chunk that times out loses its repository context**, because the + timeout retry drops the definitions and contract blocks (lesson 4). +- **A pull request's file can be fetched twice.** The 0.15.0 dependency and + same-file definition blocks keep their own reader. + +Listing, retry and cost notes: + +- **Bitbucket Cloud's `complete=false` is inferred from directory depth.** The + walk asks for `max_depth=64` and marks the listing incomplete when a path + reaches 63 slashes. The depth rule comes from probing, not from + Bitbucket's documentation, and the live check never came near it: its + deepest path had 1 slash. +- **Azure DevOps continuation tokens are not followed.** A listing that comes + back with `x-ms-continuationtoken` is marked incomplete with a WARNING. No + live listing has returned one. +- **GitLab's GraphQL listing is not retried on 429 or 5xx.** The session + retries only `GET`, `HEAD` and `OPTIONS` + (`allowed_methods` in `forges/gitlab.py`), and the listing is a `POST`. A + failed first page falls back to the REST walk; a failed later page gives a + partial listing. +- **An empty-reply retry followed by a timeout retry loses tokens.** When the + retry of an empty reply times out, `_run_worker`'s timeout retry replaces + the chunk's result, so the billed empty call's tokens drop out of the run + totals. +- **The retry's cost estimate uses the second call's model.** When either + call reports no cost, the unit's cost is unknown, and `costs.run_cost` + estimates it from the summed tokens at the rate of the unit's `model`, + the second call's, even when another model in the fallback chain answered + the first call. +- **prxref's defaults are too small for a thinking model.** + `PRXREF_LLM_MAX_TOKENS` 4096 and `PRXREF_LLM_TIMEOUT` 45 lost chunks to a + thinking model in the live checks (lesson 4). Neither default changed in + 0.16.0; raise both for such a model. The recipe under "Measuring repository + context" in `tests/evals/README.md` sets neither, so run verbatim against + such a model it cuts the replies off. Sizing, recall and GitHub notes: @@ -410,7 +445,8 @@ Sizing, recall and GitHub notes: which is above real prompt tokens. - A live check found that shrinking chunks fourfold did not change the result on one large PR. So this is a sizing note, not a known recall loss. - - Weigh it against #17's context budget. + - Repository context's character budget comes on top of the chunk token + budget, which does not count it. - **Generic-bug recall is unmeasured beyond a small sample.** On the three bundled eval cases, gpt-4o-mini found every spec-grounded label and none of the three generic bug labels. `worker.md`'s "Prefer zero findings over one @@ -420,7 +456,8 @@ Sizing, recall and GitHub notes: the size counts. A repository's test fixtures that contain deliberate violations are reviewed as code. In this repository, the sweep reviews the eval fixtures under `tests/evals/` and reports their planted violations as - findings. + findings. (Repository context's exclude floor keeps eval label files out of + the context entries, not out of the diff.) - **GitHub's limits are undocumented.** - The 406 `too_large` fires past 20,000 lines **or** 300 files. - The `/files` listing withholds `patch` in two ways: @@ -446,8 +483,56 @@ Sizing, recall and GitHub notes: 406 without `too_large`, or a compare diff whose counts disagree with the PR's. -Carried over from 0.14.0, still true: +Carried over from 0.15.0 and earlier, still true: +- **#10 The reworded tier is unproven.** It ships off. Its suggested threshold + was chosen on invented negatives, and a live check found no candidate pair + in eight runs. +- **#12 Chunks are not packed by language.** `triage.build_chunks` fills + chunks by token budget and file cap, preferring the chunk that shares the + deepest directory (`_shared_dir_depth`). So a chunk that holds two kinds of + file gets the union of both files' scoped rules. +- **#13 Grouping keys on the rule the model names.** + - A finding with no rule groups by title only. It never joins a group whose + findings name a rule, even with the same title in the same file. + - A member that line alignment moved to file level adds no `Also at` + location. + - The caps rank by `finding_rank_key` (confidence, then file path), not by + group size. So capping a representative drops all its folded locations + from the output, and the same goes for the finding the per-rule cap (#18) + folded a rule's other findings into. +- **Findings on files outside the chunk.** A worker can report on a file it saw + only in the bounded `### Other files changed in this PR` excerpt + (`chunk_context.sibling_summary_block`). Location validation and line + alignment run over the whole PR's files, so such a finding is kept and + anchored like any other. Nothing marks it as coming from an excerpt. +- **#14 The judge shares the review's backend.** Only `--judge-model` changes + which model judges. Self-judging is detected by exact model name; it warns + and stamps `self_judged`, and is never refused. Scoring needs + human-labelled cases, and the repository ships three. +- **#15 Two ways a GitHub run past the diff limit can take long or fail.** + - A PR pushed to between the PR read and the listing pages can fail the + totals check. The run then ends as `Error`, never as a partial review. + - A compare read that times out is retried by the session. The retry policy + is `LoggingRetry(total=3)` with a 30-second read timeout, so reaching the + fallback can take up to four such timeouts. +- **#16 Only GitHub and Bitbucket Cloud read description history.** + - **Other forges.** On GitLab, Azure DevOps and Bitbucket Server, a replay + logs a WARNING and uses the live title and description. An explicit + `--as-of` there exits 2. + - **Unprobed endpoint.** GitHub Enterprise Server's GraphQL endpoint + (`https:///api/graphql`) has not been probed live. + - **Large PRs.** A PR with more than 5,000 reviews, comments or title renames + pins nothing, so it replays the live text even under `--as-of`. + - **Rate limit.** Bitbucket Cloud allows 60 anonymous reads an hour, and a + history read costs at least three. So a replay without a credential can + fall back to the live text. + - **Unverified Bitbucket Cloud shapes.** The `changes_requested` activity + entry has not been seen live (0 in 4 feeds) and is read by analogy with an + approval. It is also unverified whether the commit `date` used as the head + commit's date is the author date or the committer date. + - **CRLF.** GitHub stores some description versions with CRLF line endings, + and a pinned replay passes them to the prompt verbatim. - **Spec digest.** - Unpunctuated keyword lines in one paragraph merge into one constraint, and a run of them past 400 characters is cut. @@ -475,12 +560,16 @@ Carried over from 0.14.0, still true: - Reading an MR's threads on gitlab.com needs `PRXREF_GITLAB_TOKEN`, even for a public project. Without one, thread dedup runs against no threads. - **Replay from a diff file.** A `--diff-file` run without `--pr-url` has no - file context and no threads. The title comes from the patch mail or the file - name. The description comes from the mail, `--description-file` or + file context and no threads. `--repo-dir` gives it repository context, + but the dependency-versions and same-file definitions blocks still read + only through the forge (`orchestrator._make_file_reader`), so they stay + empty. The title comes from the patch mail or the file name. The + description comes from the mail, `--description-file` or `--no-description`. - **Azure DevOps.** - Only anonymous reads are verified live: the forge reads, the dry-run output - shape, the pinned-range compare diff and a pinned-range replay. + shape, the pinned-range compare diff, a pinned-range replay, and, new in + 0.16.0, the repository file listing. - Posting, pruning, PAT and `SYSTEM_ACCESSTOKEN` authentication and service hooks are tested against recorded API shapes only. - Azure DevOps Server is untested. @@ -495,15 +584,20 @@ Carried over from 0.14.0, still true: Follow-ups a maintainer can act on: -- **After this release, tune #10's threshold.** Run `prxref eval` on a real - replay set, then consider changing the default. Use replays pinned by #16: - earlier tuning ran on replays that leaked the PR's current description. +- **Tune #10's threshold.** Run `prxref eval` on a real replay set, then + consider changing the default. Use replays pinned by #16: earlier tuning ran + on replays that leaked the PR's current description. - **Measure #18's default cap.** The issue's before and after numbers come from a simulation over graded replays, and were not re-measured on prxref's own output. `prxref eval compare` on two labelled runs measures it: `prxref eval run` the same cases with the same `--rules-file` twice, once with `PRXREF_MAX_FINDINGS_PER_RULE=0` and once at the default, `prxref eval score` both, then compare them. Each run's `run.json` records the cap it used. +- **The #17 fixture's second label under-counts.** In + `tests/fixtures/issue17/cases.json`, its `must_match` requires "mutually + exclusive" or "exactly one of", which is narrower than the wording the + model writes for the same defect. Widen the predicate, or grade it with + the judge, before using the fixture to measure repository context. - **Unchecked anchors.** `eval_cases.check_anchors` is public but never runs on a `pr_url` case's fetched diff, so those labels are only shape-checked. - **Eval cases cannot pin the description.** A case file cannot carry `as_of` or @@ -520,22 +614,23 @@ Follow-ups a maintainer can act on: - **Duplicated code in `rules.py`.** `rules._read_scoped_file` duplicates the read block of `load_review_rules`. A `_reason` helper exists in both `rules.py` and `prompt_templates.py`. -- **Missing definitions.** `chunk_context.referenced_definitions` looks only in - the same file, and `_language` has no Java entry, so a Java chunk gets no - definitions. +- **Missing definitions at `off`.** With repository context off, + `chunk_context.referenced_definitions` still looks only in the same file, + and only for JavaScript, TypeScript and Python. `diff` and `repo` + add other files and Java types; the rest of the JVM gap is #20. - **The history read is untraced.** The replay's history read is outside the trace, and only the `replay` stamp records its outcome. - **The sweep's example is always in force.** `orchestrator._example_titles` always reads the systemic template. So the sweep's example title is in force even on a run whose sweep never runs. -The v0.14.0 handoff's GitHub 20,000-line bullet is **fixed** (#15), and so is -its manual eval scoring (#14). +The v0.15.0 handoff's #17 bullet is **done**, and so is its empty-reply +bullet (the empty-reply retry). | Item | Value | |---|---| -| Released version | `0.15.0` (minor: new eval commands, new inputs and eight new config keys, seven off by default and the per-rule cap on by default whenever a review rules file is loaded; no existing config default changed, but a `--pr-url` replay now pins its title and description by default, a review rules file now also asks every unit for a rule and folds each rule's findings past the second, and every worker prompt hash moved) | -| Registration points | forges: the tuple in `forges/base.py` (`detect_forge`) and the `impls` dict in `config.py` (`make_forge`); LLM backends: `llm_backends.BACKENDS`; glyphs: `prxref.markers`; subcommands: `cli._build_parser`; prompt templates: `prompt_templates.TEMPLATE_NAMES` and `OPTIONAL_PLACEHOLDERS` | +| Released version | `0.16.0` (minor: repository context behind four new config keys, all off by default; one new CLI flag, `--repo-dir`; one new run-record and `--format json` key, `repo_context`, `null` when off; an empty model reply is retried once; no existing config default changed) | +| Registration points | forges: the tuple in `forges/base.py` (`detect_forge`) and the `impls` dict in `config.py` (`make_forge`); repository listing: the optional `Forge.list_paths` in `forges/base.py`, on every adapter; LLM backends: `llm_backends.BACKENDS`; glyphs: `prxref.markers`; subcommands: `cli._build_parser`; prompt templates: `prompt_templates.TEMPLATE_NAMES` and `OPTIONAL_PLACEHOLDERS` | | Version strings | `pyproject.toml`, `src/prxref/__init__.py`, and `uv.lock` | | Test command | `uv run pytest` (dev tools are a `[dependency-groups]` group, not an extra) | | Release assets | wheel **and** sdist attached by `release.yml`; PyPI by OIDC trusted publishing | diff --git a/README.md b/README.md index 8a3d17f..bf1a1b5 100644 --- a/README.md +++ b/README.md @@ -286,6 +286,31 @@ export PRXREF_PROMPTS_DIR=~/acme-prompts **Read the templates from a trusted checkout, never from the PR under review.** Whoever controls the directory controls the whole review, so a PR that commits templates into the checkout CI reviews rewrites its own review. Copy the directory from the target branch or keep it outside the repository. See [docs/prompt-templates.md](docs/prompt-templates.md) for each template's placeholders, the CI recipe, the daemon, and upgrading. +## Repository Context + +A chunk worker sees its chunk's diff and the definitions it references from the same file. Repository context also shows it the code the change leans on elsewhere: definitions from other files, and the API and database contracts a change points at. It is off by default: + +```bash +PRXREF_REPO_CONTEXT=repo prxref review --pr-url https://github.com/acme/widget/pull/42 --no-post + +# or from a local checkout of the PR head, with no forge at all +PRXREF_REPO_CONTEXT=repo prxref review --diff-file pr.diff --repo-dir ./widget +``` + +| `PRXREF_REPO_CONTEXT` | What each chunk worker also sees | +|---|---| +| `off` (default) | Nothing: prompts, posts, traces and logs are byte-identical to 0.15.0. | +| `diff` | Definitions from the PR's other changed files, and, for a type another chunk changes, the lines changed inside it. Java type declarations are found as well as JavaScript, TypeScript and Python ones. | +| `repo` | Also definitions from files outside the diff, found through imports, Java's same-package file convention, and a search of file names; and a `### Contract excerpts` block with the OpenAPI operation or schema, JSON Schema, or earlier migration that a changed route, table or name matches. | + +- **Where it reads.** Files are read at the PR head through the forge (every forge adapter can read and list files, see [docs/forges.md](docs/forges.md)), or from `--repo-dir` when given. With neither, `diff` builds its entries from the diff's hunk lines, and `repo` does the same with a WARNING. At `repo` the repository's file listing is read once per run; without one, a WARNING says the name search and the contract files outside the PR are off. +- **What it never reads.** A floor that is always on keeps `**/expected.json`, `**/cases.json`, `**/case.json`, `**/prxref-eval/**`, `**/.env*`, `**/*.pem` and `**/*.key` out of every listing, read and repository-context entry. `PRXREF_CONTEXT_EXCLUDE_GLOBS` adds globs to it, and cannot take a floor path back out. Neither hides a changed file's hunks, which reach the prompt as the diff. `PRXREF_CONTEXT_CONTRACT_GLOBS` replaces the built-in set of contract globs. +- **Budget.** `PRXREF_REPO_CONTEXT_MAX_CHARS` (default `12000`) caps each chunk's entries, admitted in rank order until one does not fit, and a closing line counts the rest. Reads of files outside the PR are capped at 16 per chunk and 200 per run. The timeout retry, which follows a chunk call that outlasts `PRXREF_LLM_TIMEOUT`, drops all of it to shrink the prompt. +- **Workers only.** The whole-PR sweep prompt is unchanged. +- **Recorded.** `--format json` and the run record carry `repo_context`: the reader, the listing size, the read counts, and each admitted entry's path, line and reason, never file text. The JSONL trace gains a `chunk context` event per chunk, and in text output `-v` prints a `repo context:` line. + +To measure what it changes, run [`prxref eval`](#evaluating-prxref) once per level and compare the two runs. Every setting: [docs/env-vars.md](docs/env-vars.md). + ## Finding Markers Each severity has one glyph. It is the same in the summary's counts line, the summary's findings list, and the header of every inline comment: @@ -320,8 +345,9 @@ Before 0.14.0, `outofscope` findings rendered 🟦. They now render ⬜ on every - `--scoped-rules PATH` — path-scoped team review rules: a rules file, or a directory whose `*.md` files are read one level deep. Each file reaches only the chunks whose paths match its `applies_to:` globs, and the whole-PR sweep gets the union. Repeatable. When given, the flags replace `PRXREF_SCOPED_RULES` entirely rather than adding to it, and `--scoped-rules ""` turns it off for this run. A file that fails its checks exits `2` before any network call. Read it from a trusted checkout, never from the PR under review. See [Path-scoped rules](docs/review-rules.md#path-scoped-rules). - `--context-file PATH` — the ticket the PR is meant to implement (plain text or Markdown). Every finding is then marked in, out of, or of unknown ticket scope, and an empty file means "this PR has no ticket". Overrides `PRXREF_TICKET_CONTEXT_FILE` for this run, and `--context-file ""` turns it off. See [Ticket Context and Scope](#ticket-context-and-scope). - `--prompts-dir DIR` — a directory of `worker.md`, `systemic.md` and `summary.md` templates that replace the packaged prompts; a template missing from it keeps the packaged one. Overrides `PRXREF_PROMPTS_DIR` for this run, and `--prompts-dir ""` turns it off. A directory that fails its checks exits `2` before any network call. Read it from a trusted checkout, never from the PR under review. See [Prompt Template Overrides](#prompt-template-overrides). +- `--repo-dir PATH` — a local checkout of the repository at the PR head. With `PRXREF_REPO_CONTEXT=repo`, repository context reads and lists files there instead of calling the forge (at `diff` it only reads there), so a `--diff-file` review gets repository context with no network. It is not a replay flag: on its own it neither turns posting off nor adds a `replay` stamp. A path that is not an existing directory exits `2` before any network call, even while `PRXREF_REPO_CONTEXT` is `off`, which ignores the directory. See [Repository Context](#repository-context). - `--trace-dir DIR` — write each review unit's exact prompt halves, raw model response, and metadata to `DIR` (`chunk0.system.md`, `chunk0.user.md`, `chunk0.response.json`, `chunk0.meta.json`, and so on for each chunk and for the whole-PR `sweep`). `PRXREF_TRACE_DIR` does the same for every run; the flag wins when both are set. The files number chunks from 0, while the JSONL trace's `chunk` events (`PRXREF_TRACE_FILE`) number them from 1 in `index`, so the event with `index` N pairs with `chunk{N-1}.*`. -- `-v, --verbose` — output run timing, token counts, cost, and finding breakdowns to stdout, plus one line each for the rules file, the scoped rules files, the prompt templates, the ticket context (with the active findings' scope counts), and the spec sources when they are configured. In text mode this also prints finding bodies and dropped findings, same as `--no-post`. +- `-v, --verbose` — output run timing, token counts, cost, and finding breakdowns to stdout, plus one line each for the rules file, the scoped rules files, the prompt templates, the ticket context (with the active findings' scope counts), the spec sources, and the repository context when they are configured. The repository-context line reads, for example, `repo context: mode=repo reader=forge listing=120(partial) reads=16 cap_hit=yes entries=3 omitted=3`, and is absent while `PRXREF_REPO_CONTEXT` is `off`. In text mode this also prints finding bodies and dropped findings, same as `--no-post`. These lines are text output only: with `--format json`, stdout holds the JSON object alone. - `--format {text,json}` — output format for `review` (default `text`). `json` prints exactly one JSON object to stdout, with these keys in this order: - `verdict`; - `findings`: active first, then dropped, each with `file`, `line`, `severity`, `confidence`, `scope` (`in`, `out`, or `unknown` against the ticket context; always `unknown` without one), `rule` (the rule the finding names; `null` when it names none, and always `null` with finding grouping off and the per-rule cap inactive), `locations` (on a grouped finding, the other places its `Also at:` list names, in that order, as `{"file", "line"}` objects; on the best finding of a rule the per-rule cap folded, every location folded into it, across files, even past the five its `Also at:` list shows; `null` on every other row, including each member dropped as `grouped into :`, which keeps its own rule; see [docs/quality.md](docs/quality.md)), `title`, `body`, `drop_reason`; @@ -332,6 +358,7 @@ Before 0.14.0, `outofscope` findings rendered 🟦. They now render ⬜ on every - `prompt_templates`: the prompt-template directory in force (`dir`, plus `templates` holding the `path`, `sha256` and `chars` of each template file present in the directory, edited or not; never the template text). Always present, and `null` when no prompts directory is configured. See [Prompt Template Overrides](#prompt-template-overrides); - `scoped_rules`: the path-scoped rules in force (`entries` as configured; `files`, one row per loaded file in load order with its `path`, `sha256`, `chars`, `max_chars`, `truncated`, `severity_map` and `applies_to`; `max_chars`, the per-unit cap; and `units`, the `path` and `chars` of the files each chunk and the sweep received; never the rules text). Always present, and `null` when no scoped rules are configured. See [Path-scoped rules](docs/review-rules.md#path-scoped-rules); - `rule_counts`: the per-rule cap's tally, one `{"rule", "kind", "total", "kept"}` object for each rule (`kind` `rule`), or for each normalized title among findings that name no rule (`kind` `title`), that at least two chunk findings share: `total` of them, `kept` active, most findings first. A rule or title with one finding gets no row. Always present: `[]` when the cap ran and nothing repeated, and `null` when it did not run (no review rules file, `PRXREF_MAX_FINDINGS_PER_RULE=0`, or a run that ended before the quality passes). See [docs/quality.md](docs/quality.md); + - `repo_context`: the repository context in force (`mode`, `max_chars`, `contract_globs` and `exclude_globs`; `reader`, which is `forge`, `repo-dir` or `null`; `listing`, `{"paths", "complete"}` or `null`; `reads` and `read_cap_hit`; and `units`, `{"chunks": [...]}` holding one `{"entries", "omitted", "retry_dropped"}` row per chunk in chunk order, whose entries carry `path`, `line`, `symbol`, `kind`, `reason` and `chars`; never the file text). Always present, and `null` when `PRXREF_REPO_CONTEXT` is `off`. See [docs/env-vars.md](docs/env-vars.md); - `sampling`: the `temperature`, `seed`, and `models` the run had in force (every review result carries it); - `replay`: the replay stamp (`base_sha`, `head_sha`, `threads`, `diff_file`, `description`, `as_of`, `as_of_source`), on replay runs only. diff --git a/docs/env-vars.md b/docs/env-vars.md index 4da761f..d10d84d 100644 --- a/docs/env-vars.md +++ b/docs/env-vars.md @@ -34,8 +34,8 @@ Configuration is loaded from built-in defaults, overridden by environment variab | `PRXREF_CHUNK_CONTEXT_LINES` | `3` | Context lines kept around each change when a chunk's diff is rendered into the worker prompt. Must be **>= 0**; `0` emits the changed lines only. The forge's diff is the only source of context — prxref trims what it received and never adds what it did not. | | `PRXREF_MAX_WORKERS` | `4` | Parallel chunk-review workers. Must be **greater than 0**. The cap that matters is usually the endpoint's rate limit, not the machine. | | `PRXREF_MAX_INLINE_COMMENTS` | `15` | Maximum inline comments posted per review, applied **after** the quality gate. Must be **greater than 0**. Findings past the cap are still listed in the summary comment; only the inline posting is trimmed. | -| `PRXREF_TRACE_FILE` | *(empty — off)* | Path to append a JSONL run trace to. Empty disables tracing and the tracer becomes a no-op, so there is no cost when unset. One event per line (`run`, `forge.get_pr`, `forge.get_diff`, `parse_diff`, `build_chunks`, `chunk`, `heartbeat`, `post`), flushed as it happens. A `chunk` event's `index` counts from 1 (of `total`), one more than the number in that chunk's `PRXREF_TRACE_DIR` file names. Each carries a phase: `start`, then `ok` or `fail`; `post` also uses `skip`, so a stage nobody asked to run is distinguishable from one the run never reached — a run still in flight, or one that was killed mid-hang, is as readable as a completed one. Render it to a standalone HTML pipeline view with `prxref trace render `. | -| `PRXREF_TRACE_DIR` | *(empty — off)* | Directory for per-unit prompt/response traces. Each review unit (`chunk0`, `chunk1`, … and the whole-PR `sweep`) writes four files there, its chunks numbered from 0, while the `chunk` events of the JSONL trace (`PRXREF_TRACE_FILE`) number them from 1 in `index`, so the event with `index` N pairs with the `chunk{N-1}.*` files: `.system.md` and `.user.md` (the exact rendered prompt halves), `.response.json` (the raw model text, JSON-encoded), and `.meta.json` (`unit`, `model`, token counts, `elapsed_ms`, `error`, `cost_usd` — the dollar figure the backend reported for the call, `null` when it reported none — and `cost_source`, where that figure came from, `""` when `cost_usd` is `null`). Unset disables the dump entirely — no directory is created and there is no cost. Writes are best-effort: a failure is a logged warning, never a review failure, and a timeout retry overwrites the unit's files so the trace shows the attempt whose result was used. `--trace-dir DIR` on `prxref review` is the per-run equivalent and wins when both are set. | +| `PRXREF_TRACE_FILE` | *(empty — off)* | Path to append a JSONL run trace to. Empty disables tracing and the tracer becomes a no-op, so there is no cost when unset. One event per line, flushed as it happens. The pipeline stages (`run`, `forge.get_pr`, `forge.get_diff`, `parse_diff`, `build_chunks`, `chunk`, `sweep`, `post`) each carry a phase: `start`, then `ok` or `fail`; `post` also uses `skip`, so a stage nobody asked to run is distinguishable from one the run never reached — a run still in flight, or one that was killed mid-hang, is as readable as a completed one. A `chunk` event's `index` counts from 1 (of `total`), one more than the number in that chunk's `PRXREF_TRACE_DIR` file names. Other phases: `heartbeat` `tick` while workers run; `chunk` `retry` when a timed-out chunk is re-sent; and, when `PRXREF_REPO_CONTEXT` is not `off`, one `chunk` `context` per chunk (`entries`, `omitted`, `chars`) and one `repo_context` `ok` per run (`mode`, `reader`, `listing`, `reads`, `read_cap_hit`). Run inputs and deterministic passes add one-off events (`rules`, `scoped_rules`, `ticket`, `prompts`, `specs`, `grouping`, `rulecap`) whose phase names what happened: `ok`, `fail`, `remap`, `relabel` or `echo`. Render it to a standalone HTML pipeline view with `prxref trace render `. | +| `PRXREF_TRACE_DIR` | *(empty — off)* | Directory for per-unit prompt/response traces. Each review unit (`chunk0`, `chunk1`, … and the whole-PR `sweep`) writes four files there, its chunks numbered from 0, while the `chunk` events of the JSONL trace (`PRXREF_TRACE_FILE`) number them from 1 in `index`, so the event with `index` N pairs with the `chunk{N-1}.*` files: `.system.md` and `.user.md` (the exact rendered prompt halves), `.response.json` (the raw model text, JSON-encoded), and `.meta.json` (`unit`, `model`, token counts, `elapsed_ms`, `error`, `cost_usd` — the dollar figure the backend reported for the call, `null` when it reported none — and `cost_source`, where that figure came from, `""` when `cost_usd` is `null`). Unset disables the dump entirely — no directory is created and there is no cost. Writes are best-effort: a failure is a logged warning, never a review failure, and a timeout retry overwrites the unit's files so the trace shows the attempt whose result was used. An empty-reply retry writes the unit's files once, after its second call, so they show that call's reply, and `.meta.json` counts the tokens of both calls when both returned. `--trace-dir DIR` on `prxref review` is the per-run equivalent and wins when both are set. | | `PRXREF_DRY_RUN` | `False` | Set to the literal `1` to run the full review and write nothing to the forge — no summary, no inline comments. Applies to the webhook daemon as well as the CLI, which is the only way to watch the daemon against a real repository before letting it comment. `--no-post` is the per-invocation equivalent and still wins when the environment says nothing. Only the literal `1` enables it. | | `PRXREF_FAIL_ON` | `never` | Exit-code policy for `prxref review`. `never` (the default) keeps the advisory contract — the exit code never reflects findings. `error` exits `1` when the completed review carries an active error-severity finding; `any` exits `1` on any active finding. Under either value a review that does not complete also exits `1` — it crashes, or it ends with verdict `Error` (the forge could not be read, the diff could not be parsed or chunked, or every chunk review failed) — so a gating lane cannot read a broken run as green. An empty PR diff is not a failure (verdict `Approved`, exit `0`). The webhook daemon has no exit code and is unaffected. See [Bad Configuration Is the Only Thing That Fails a Build](#bad-configuration-is-the-only-thing-that-fails-a-build). | | `PRXREF_POST_MODE` | `summary+inline` | What gets posted to the forge: `summary+inline` (the summary comment, then inline comments only if the summary landed), `summary` (the summary comment only — inline comments are never posted), or `inline` (inline comments only — no summary is posted on any path, including the error notice). Any other value raises `ConfigError` and `prxref review` exits `2`. A dry run posts nothing in any mode. | @@ -55,6 +55,10 @@ Configuration is loaded from built-in defaults, overridden by environment variab | `PRXREF_PROMPTS_DIR` | *(empty — off)* | **Prompt template overrides (0.15.0).** Directory holding replacement `worker.md`, `systemic.md` and `summary.md` prompt templates; a file that is absent keeps the packaged one. Every template is validated before any network call: `worker.md` and `systemic.md` must keep the `## Review Context` marker and every packaged placeholder below it (the feature slots `{scope_example}` and `{rule_example}` are optional), and `summary.md` needs only `{findings}`. A missing directory, a failed check, or a template over 256 KiB raises `ConfigError` and `prxref review` exits `2`; an empty directory or an unknown placeholder only warns. The run record's `prompt_templates` stamps the directory and, for each template file present in it, edited or not, its path, SHA-256 and length; a template absent from the directory has no entry. `--prompts-dir DIR` wins for one run, and `prxref prompts export DIR [--force]` writes the packaged templates to start from. The judge prompt of `prxref eval` cannot be overridden. Read it from a trusted checkout: a PR that commits a template rewrites its own review. Unset uses the packaged templates, byte for byte. | | `PRXREF_TICKET_CONTEXT_FILE` | *(empty — off)* | Plain-text or Markdown file holding the ticket this PR implements. When set, every finding is marked in, out of, or of unknown ticket scope. An empty or whitespace-only file means "this PR has no ticket". A missing or non-UTF-8 file raises `ConfigError` and `prxref review` exits `2`. The webhook daemon ignores it (and says so once). `--context-file PATH` wins for one run, and `--context-file ""` turns it off. | | `PRXREF_TICKET_CONTEXT_MAX_CHARS` | `6000` | Characters of ticket text kept in the prompt; longer text is truncated with a visible marker. Must be **greater than 0**. | +| `PRXREF_REPO_CONTEXT` | `off` | **Repository context (0.16.0).** `off` (the default) leaves every prompt, post, trace and default-verbosity log byte-identical to 0.15.0. `diff` adds definitions from the PR's own changed files (`cross-chunk` and `diff-file` entries, Java type declarations included): those files are read at the PR head through the forge's repository reader, or `--repo-dir`, when one is available, and without a reader the entries are built from the diff's hunk lines alone, with no warning. `repo` additionally reads files outside the diff — import, path-convention and name-search definitions, plus contract excerpts — through the same reader, and logs one WARNING when there is no reader or no file listing. Only the chunk workers' prompts carry repository context; the whole-PR sweep prompt is unchanged. Matching is exact and case-sensitive, like `PRXREF_FAIL_ON`: `Repo` or `REPO` is rejected. Any value outside `off`\|`diff`\|`repo` raises `ConfigError` and `prxref review` exits `2`. | +| `PRXREF_REPO_CONTEXT_MAX_CHARS` | `12000` | **Repository context (0.16.0).** Per-chunk character budget shared by the cross-chunk, contract and other repository-context entries admitted under `PRXREF_REPO_CONTEXT`. Must be **greater than 0**. | +| `PRXREF_CONTEXT_CONTRACT_GLOBS` | the built-in set below | **Repository context (0.16.0).** Globs (matched like `PRXREF_SIZE_IGNORE_GLOBS`: case-sensitive `fnmatch`, `*` crosses `/`) selecting the contract files — OpenAPI, JSON Schema, Liquibase/SQL migrations — excerpted under `repo`. A set value **REPLACES** the built-in set below, rather than adding to it; an empty value reads as unset, so the built-in set stays. There is no way to turn contract excerpts off on their own in 0.16.0 — set `PRXREF_REPO_CONTEXT` to `off` or `diff` instead. Built-in set: `**/openapi*.y*ml`, `**/openapi*.json`, `**/openapi/**`, `**/swagger*`, `**/*.schema.json`, `**/db/changelog/**`, `**/db/migration/**`, `**/migrations/**`. | +| `PRXREF_CONTEXT_EXCLUDE_GLOBS` | *(empty)* | **Repository context (0.16.0).** Globs (matched like `PRXREF_SIZE_IGNORE_GLOBS`) whose paths are never read for repository context, not even a diff file. **Added** to a floor that is always on: `**/expected.json`, `**/cases.json`, `**/case.json`, `**/prxref-eval/**`, `**/.env*`, `**/*.pem`, `**/*.key`. Empty (the default) adds nothing. | The replay flags of `prxref review` (`--base-sha`, `--head-sha`, `--no-threads`, `--diff-file`, `--as-of`, `--description-file`, `--no-description`) deliberately have no environment variable: set in the environment, a replay pin would silently pin every run, the webhook daemon's included. @@ -162,11 +166,11 @@ The two knobs above, plus the four opt-in levers added in 0.15.0 (`PRXREF_MAX_WA ## Environment Cross-Check & Defaults -The tables above define all **63** configuration keys in `src/prxref/config.py` (`_DEFAULTS`), and every one of them appears in `.env.example`: +The tables above define all **67** configuration keys in `src/prxref/config.py` (`_DEFAULTS`), and every one of them appears in `.env.example`: -- **LLM / Pipeline (45):** `PRXREF_LLM_BACKEND`, `PRXREF_LLM_BASE_URL`, `PRXREF_LLM_API_KEY`, `PRXREF_LLM_MODELS`, `PRXREF_LLM_REASONING_EFFORT`, `PRXREF_LLM_MAX_TOKENS`, `PRXREF_LLM_TIMEOUT`, `PRXREF_LLM_TEMPERATURE`, `PRXREF_LLM_SEED`, `PRXREF_LLM_CLI_PATH`, `PRXREF_LLM_CLI_CONCURRENCY`, `PRXREF_CONFIDENCE_FLOOR`, `PRXREF_MAX_ERROR_FINDINGS`, `PRXREF_MAX_WARNING_FINDINGS`, `PRXREF_MAX_OUTOFSCOPE_FINDINGS`, `PRXREF_MAX_FINDINGS_PER_RULE`, `PRXREF_GROUP_FINDINGS`, `PRXREF_DEDUP_SIMILARITY`, `PRXREF_MAX_CHUNKS`, `PRXREF_CHUNK_TOKEN_BUDGET`, `PRXREF_CHUNK_MAX_FILES`, `PRXREF_CHUNK_CONTEXT_LINES`, `PRXREF_MAX_WORKERS`, `PRXREF_MAX_INLINE_COMMENTS`, `PRXREF_FAIL_ON`, `PRXREF_DRY_RUN`, `PRXREF_TRACE_FILE`, `PRXREF_TRACE_DIR`, `PRXREF_POST_MODE`, `PRXREF_POST_VERDICT`, `PRXREF_PRICE_TABLE`, `PRXREF_POST_COST`, `PRXREF_SIZE_WARN_LINES`, `PRXREF_SIZE_WARN_FILES`, `PRXREF_SIZE_IGNORE_GLOBS`, `PRXREF_SPEC_SOURCES`, `PRXREF_SPEC_MAX_CHARS`, `PRXREF_SPEC_DIGEST_TOKENS`, `PRXREF_REVIEW_RULES`, `PRXREF_REVIEW_RULES_MAX_CHARS`, `PRXREF_SCOPED_RULES`, `PRXREF_SCOPED_RULES_MAX_CHARS`, `PRXREF_PROMPTS_DIR`, `PRXREF_TICKET_CONTEXT_FILE`, `PRXREF_TICKET_CONTEXT_MAX_CHARS` +- **LLM / Pipeline (49):** `PRXREF_LLM_BACKEND`, `PRXREF_LLM_BASE_URL`, `PRXREF_LLM_API_KEY`, `PRXREF_LLM_MODELS`, `PRXREF_LLM_REASONING_EFFORT`, `PRXREF_LLM_MAX_TOKENS`, `PRXREF_LLM_TIMEOUT`, `PRXREF_LLM_TEMPERATURE`, `PRXREF_LLM_SEED`, `PRXREF_LLM_CLI_PATH`, `PRXREF_LLM_CLI_CONCURRENCY`, `PRXREF_CONFIDENCE_FLOOR`, `PRXREF_MAX_ERROR_FINDINGS`, `PRXREF_MAX_WARNING_FINDINGS`, `PRXREF_MAX_OUTOFSCOPE_FINDINGS`, `PRXREF_MAX_FINDINGS_PER_RULE`, `PRXREF_GROUP_FINDINGS`, `PRXREF_DEDUP_SIMILARITY`, `PRXREF_MAX_CHUNKS`, `PRXREF_CHUNK_TOKEN_BUDGET`, `PRXREF_CHUNK_MAX_FILES`, `PRXREF_CHUNK_CONTEXT_LINES`, `PRXREF_MAX_WORKERS`, `PRXREF_MAX_INLINE_COMMENTS`, `PRXREF_FAIL_ON`, `PRXREF_DRY_RUN`, `PRXREF_TRACE_FILE`, `PRXREF_TRACE_DIR`, `PRXREF_POST_MODE`, `PRXREF_POST_VERDICT`, `PRXREF_PRICE_TABLE`, `PRXREF_POST_COST`, `PRXREF_SIZE_WARN_LINES`, `PRXREF_SIZE_WARN_FILES`, `PRXREF_SIZE_IGNORE_GLOBS`, `PRXREF_SPEC_SOURCES`, `PRXREF_SPEC_MAX_CHARS`, `PRXREF_SPEC_DIGEST_TOKENS`, `PRXREF_REVIEW_RULES`, `PRXREF_REVIEW_RULES_MAX_CHARS`, `PRXREF_SCOPED_RULES`, `PRXREF_SCOPED_RULES_MAX_CHARS`, `PRXREF_PROMPTS_DIR`, `PRXREF_TICKET_CONTEXT_FILE`, `PRXREF_TICKET_CONTEXT_MAX_CHARS`, `PRXREF_REPO_CONTEXT`, `PRXREF_REPO_CONTEXT_MAX_CHARS`, `PRXREF_CONTEXT_CONTRACT_GLOBS`, `PRXREF_CONTEXT_EXCLUDE_GLOBS` - **Per-Forge Auth (10):** `PRXREF_BITBUCKET_TOKEN`, `PRXREF_BITBUCKET_USER`, `PRXREF_BITBUCKET_APP_PASSWORD`, `PRXREF_BITBUCKET_SERVER_TOKEN`, `PRXREF_BITBUCKET_SERVER_USER`, `PRXREF_BITBUCKET_SERVER_PASSWORD`, `PRXREF_GITHUB_TOKEN`, `PRXREF_GITHUB_ENTERPRISE_TOKEN`, `PRXREF_GITLAB_TOKEN`, `PRXREF_AZURE_DEVOPS_TOKEN` - **Spec Sources / Jira (3):** `PRXREF_JIRA_BASE_URL`, `PRXREF_JIRA_EMAIL`, `PRXREF_JIRA_API_TOKEN` - **Webhooks (5):** `PRXREF_BITBUCKET_WEBHOOK_SECRET`, `PRXREF_GITHUB_WEBHOOK_SECRET`, `PRXREF_GITLAB_WEBHOOK_SECRET`, `PRXREF_AZURE_DEVOPS_WEBHOOK_SECRET`, `PRXREF_ALLOW_UNSIGNED` -*(63 configuration keys, plus one deprecated alias — `PRXREF_MAX_ERRORS` for `PRXREF_MAX_ERROR_FINDINGS` — for 64 accepted variable names.)* +*(67 configuration keys, plus one deprecated alias — `PRXREF_MAX_ERRORS` for `PRXREF_MAX_ERROR_FINDINGS` — for 68 accepted variable names.)* diff --git a/docs/evals.md b/docs/evals.md index 505751b..7840151 100644 --- a/docs/evals.md +++ b/docs/evals.md @@ -86,6 +86,7 @@ no case runs. "id": "local-1", "diff_file": "diffs/local-1.diff", "context_file": "tickets/local-1.md", + "repo_dir": "repos/local-1", "spec": ["docs/specs"], "expected": [] } @@ -105,15 +106,17 @@ is refused, and a field set to `null` counts as absent. | `diff_file` | one of these two | A unified diff (`git diff` or `git format-patch` output) holding at least one file diff. | | `base_sha`, `head_sha` | with `pr_url` | The pinned range, as the replay flags take it: a pair, full 40- or 64-character hex, two different commits. Stored lowercased. | | `context_file` | no | The ticket the PR implements, given to the review as `--context-file`. | +| `repo_dir` | no | A directory holding the repository at the PR head. Repository context (`PRXREF_REPO_CONTEXT=repo`) reads and lists files there instead of calling a forge; the field has no effect when repository context is off. | | `spec` | no | Spec sources, as `--spec` takes them: one string or an array of strings, each a local path or an `http(s)` URL. | A case replays either a local diff (`diff_file`), or a pull request pinned to a range (`pr_url` with both SHAs). It may also give `pr_url` beside `diff_file`, with or without the SHAs, as `prxref review` allows. SHAs need `pr_url`, and `pr_url` without `diff_file` needs both SHAs. A relative -`diff_file`, `context_file` or local `spec` path is read relative to the -directory holding `cases.json`. `context_file` and every local `spec` path -must exist, and a `spec` URL is kept as given. +`diff_file`, `context_file`, `repo_dir` or local `spec` path is read +relative to the directory holding `cases.json`. `context_file` and every +local `spec` path must exist, `repo_dir` must be an existing directory, and +a `spec` URL is kept as given. Each entry of `expected` is one label, a finding a human reviewer left: @@ -144,6 +147,7 @@ case, read in name order, and its id is the directory name. | `diff.patch` | yes | `diff_file` | | `expected.json` | yes | `expected`: a JSON array of labels | | `ticket.md` | no | `context_file` | +| `repo/` | no | `repo_dir` | | `docs/` | no | the case's one `spec` source | `meta.json` is not read. `expected.json` spells two label fields @@ -165,7 +169,8 @@ the path. case does not have. - Neither `pr_url` nor `diff_file`; a `pr_url` no forge recognises; SHAs that break the rules above; a `diff_file` that cannot be read or holds no - file diff; a `context_file` or local `spec` path that does not exist. + file diff; a `context_file` or local `spec` path that does not exist; a + `repo_dir` that is not an existing directory. - `expected` missing or not an array. - A label with a required field missing, a `line` below `1`, a `severity` outside the five, a `category`, `text` or `must_match` that is not a @@ -291,8 +296,8 @@ prxref-eval/ --out ``` - **`cases//case.json`** holds the case as the harness read it: `id`, - `pr_url`, `base_sha`, `head_sha`, `diff_file`, `context_file`, `spec` (a - list) and `expected`, each label with all eight fields, in the + `pr_url`, `base_sha`, `head_sha`, `diff_file`, `context_file`, `repo_dir`, + `spec` (a list) and `expected`, each label with all eight fields, in the `cases.json` spelling. `eval score` grades against this copy, so it needs no `--cases` and never opens the files the case names. - **`cases//record.json`** is the review's record, exactly what @@ -322,7 +327,7 @@ prxref-eval/ --out | `sampling` | the reviewer's `temperature`, `seed` and `models` | | `review_rules` | the record's stamp of the rules file, or `null` | | `scoped_rules` | the record's stamp of the path-scoped rules (`entries`, `files`, `max_chars`, `units`; never the rules text), or `null` | -| `config` | the settings `llm_backend`, `llm_models`, `llm_max_tokens`, `max_chunks`, `chunk_token_budget`, `chunk_max_files`, `dedup_similarity`, `group_findings`, `max_warning_findings`, `max_outofscope_findings`, `max_findings_per_rule`, `scoped_rules_max_chars` | +| `config` | the settings `llm_backend`, `llm_models`, `llm_max_tokens`, `max_chunks`, `chunk_token_budget`, `chunk_max_files`, `dedup_similarity`, `group_findings`, `max_warning_findings`, `max_outofscope_findings`, `max_findings_per_rule`, `scoped_rules_max_chars`, `repo_context`, `repo_context_max_chars`, `context_contract_globs`, `context_exclude_globs` | `prompts.sha256` always hashes the packaged templates, so an override shows only under `prompts.prompt_templates`. `prompt_templates`, `sampling`, @@ -331,7 +336,7 @@ order, whose record's verdict is not `Error`. They are `null` when there is none, and `review_rules`, `scoped_rules` and `prompt_templates` are `null` when their input is off. -**No credential is ever written.** `config` is an allowlist of the twelve +**No credential is ever written.** `config` is an allowlist of the sixteen settings above, and a record carries no credential. The traces do hold the prompts, and the prompts hold the diff, so treat a run directory like the code it reviewed. diff --git a/docs/forges.md b/docs/forges.md index 57b3423..2b05852 100644 --- a/docs/forges.md +++ b/docs/forges.md @@ -39,6 +39,7 @@ Every host is covered, but not by the same means. GitHub and GitLab are host-agn - **Signature Header:** `X-Hub-Signature` (HMAC-SHA256) validated against `PRXREF_BITBUCKET_WEBHOOK_SECRET`. - **Pinned Commit Range (Replay):** `GET /2.0/repositories/{owner}/{repo}/diff/{head_sha}..{base_sha}?topic=true` with `Accept: text/plain`, returning the changes on the head side of the merge-base. Bitbucket spells a range SOURCE..DEST, the reverse of git, so the head SHA comes first; the other order names the reverse range and returns a different diff that still parses. `topic=true` is the merge-base ("three-dot") form and is sent explicitly rather than left to the default, because `topic=false` diffs the two commits directly and so also shows whatever landed on the base after the fork. The text is returned unmodified, an empty range returns empty text, and an HTTP or transport error raises. - **Description History (Replay):** `GET /2.0/repositories/{owner}/{repo}/pullrequests/{number}` (the creation date, the author, and the current title and description), then the pull request's `GET …/pullrequests/{number}/activity` feed (`pagelen=50`, following `next`, at most 50 pages), then `GET /2.0/repositories/{owner}/{repo}/commit/{sha}` for the head commit's `date` (`--head-sha` when given, else the pull request's source commit). Each `update` entry's `changes.description` carries the old and the new text with the update's `date`, and `changes.title` a rename. The first human review is the earliest approval, change request or comment by a user account (not an app) other than the pull request's author, skipping deleted comments and prxref's own posts. A history that cannot be trusted whole pins nothing: a feed longer than the page budget, an unreadable change entry, or edits that do not chain to the current text leave the current title and description in place, with a warning. The requests carry the same credentials as everything above, and a public repository needs none. On 2026-09-24 the reader was run live, read-only and with no token, against a public pull request whose description had been edited: it read the whole history, took the approval as the first review (an app's comment was skipped), and pinned the text in force then. A later live check confirmed the rename shape: on three renamed public pull requests `changes.title` carried the old and the new title as `{old, new}` strings, and the reader pinned the right title at every cutoff tried across one pull request's two renames. The `changes_requested` entry a change request writes has not been seen live yet (none in the four activity feeds read), so the reader assumes it has an `approval`'s shape, a `user` and a `date`. Anonymous reads are limited to 60 an hour (`x-ratelimit-limit: 60;w=3600`), and one history read costs at least three of them (the pull request, each activity page, and the head commit), on top of the review's own reads, so replays without a token can exhaust the limit. A history read that fails, a `429` that outlasts the adapter's retries included, never fails the review: the replay keeps the current title and description and logs a WARNING (`replay shows the PR's CURRENT title and description: reading its description history failed …`). +- **Repository Listing (Repository Context):** at `PRXREF_REPO_CONTEXT=repo`, once per run, `GET /2.0/repositories/{owner}/{repo}/src/{head_sha}/?max_depth=64&pagelen=100`, following each page's `next` URL as given, at most 20 pages, with the same credentials as everything above. Only `commit_file` entries are kept, so directories and submodules are left out. The listing is marked incomplete (`complete: false` in the run record, `(partial)` on the `-v` line) when the walk stops at the 20-page cap, when a later page fails, or when a listed directory sits at the depth limit (63 slashes in its path), a rule inferred from probing and not documented by Bitbucket. A first request that fails (a missing read permission or any other non-2xx status, a transport error, or a body with no `values` list) gives no listing: the run logs one WARNING and continues without the name search and the contract files outside the PR. On 2026-09-24 it was run live, read-only and with no token, against a public repository, and returned exactly the files of a raw walk of that endpoint; the depth rule was not reached there. --- @@ -65,6 +66,7 @@ Every host is covered, but not by the same means. GitHub and GitLab are host-agn - **Signature Header:** `X-Hub-Signature-256` (HMAC-SHA256) validated against `PRXREF_GITHUB_WEBHOOK_SECRET`. - **Pinned Commit Range (Replay):** `GET /repos/{owner}/{repo}/compare/{base_sha}...{head_sha}` on the same base URL (GHES included), with `Accept: application/vnd.github.diff`. The three dots are the merge-base form and are required, because the two-dot spelling returns 404. Without the diff media type the endpoint returns its JSON comparison object rather than a diff. The text is returned unmodified, an empty range (a head already merged into the base) returns empty text, and an HTTP or transport error raises. The compare endpoint applies neither of the pull-request diff's limits, 20,000 lines or 300 files: two read-only probes of public tag ranges on 2026-09-24, one of about 1.0 million diff lines across 3,316 files and one of about 1.05 million lines across 10,373 files, each returned HTTP `200` with the whole diff. So a `--base-sha`/`--head-sha` replay is unaffected by either limit and has no files-listing fallback. - **Description History (Replay):** one GraphQL query, `POST https://api.github.com/graphql` for `github.com` or `POST https://{host}/api/graphql` on GHES, reads the pull request's description versions (`userContentEdits`, one full text per edit, ordered by `editedAt`), every title rename (the `RENAMED_TITLE_EVENT` timeline), its reviews and conversation comments, and the head commit's `committedDate` (of `--head-sha` when given, else of the pull request's last commit). A connection longer than one page (100 nodes) is read by a further POST, up to 50 in all. The first human review is the earliest submitted review or comment by a `User` (not a bot) other than the pull request's author, skipping prxref's own posts. GitHub's GraphQL API refuses anonymous reads, so this needs `PRXREF_GITHUB_TOKEN` (or `PRXREF_GITHUB_ENTERPRISE_TOKEN` on GHES); without one, and on any failed read, the replay keeps the current title and description and logs a warning. The fields the query reads were checked live against public `github.com` pull requests while the reader was designed, and its tests use recorded response shapes. The GHES endpoint has **not been probed**. +- **Repository Listing (Repository Context):** at `PRXREF_REPO_CONTEXT=repo`, once per run, `GET /repos/{owner}/{repo}/git/trees/{head_sha}?recursive=1` on the same base URL (GHES included), one request with no paging and the same token as everything above. Only `blob` entries are kept, so directories and submodules are left out. GitHub truncates the tree of a very large repository; the listing then keeps what was returned and is marked incomplete (`complete: false` in the run record, `(partial)` on the `-v` line). A failed request (a token that cannot read the repository or any other non-2xx status, a transport error, or a body with no `tree` list) gives no listing: the run logs one WARNING and continues without the name search and the contract files outside the PR. On 2026-09-24 it was run live, read-only and with a token, against a public repository, where it returned exactly the files of a raw tree walk, and against a large public repository whose tree GitHub truncated, where it returned an incomplete listing. GHES has not been probed. --- @@ -90,6 +92,7 @@ Every host is covered, but not by the same means. GitHub and GitLab are host-agn - **Signature Header:** `X-Gitlab-Token` (plain secret token) validated against `PRXREF_GITLAB_WEBHOOK_SECRET`. - **Pinned Commit Range (Replay):** `GET /repository/compare?from={base_sha}&to={head_sha}&straight=false`. `straight=false` is the merge-base form; `straight=true` would diff the two commits directly. `unidiff` is deliberately not requested, so each entry's `diff` holds only its hunks, and the entries are rendered by the same header reconstruction as **Diffs** above. A response with `compare_timeout: true` raises rather than reviewing an incomplete file list. An entry flagged `too_large` or `collapsed` carries no inline diff: it is logged as a warning and reviewed as a header-only file. An empty range returns empty text, and an HTTP or transport error raises. - **Description History (Replay):** none. The adapter cannot read an MR's description history, so a `--pr-url` replay here shows the MR's current title and description and logs a warning, and `--as-of` exits `2`. GitLab's system notes answered anonymous reads with HTTP `401`, and whether they carry the old description text is unverified. +- **Repository Listing (Repository Context):** at `PRXREF_REPO_CONTEXT=repo`, once per run. GitLab lists repository files through the GraphQL API (`POST https://{host}/api/graphql`, `project.repository.tree(recursive: true).blobs`, 100 per page, up to 20 pages), because the REST `repository/tree?recursive=true` listing returns every directory before any file and so, under the 20-page cap, loses files on any project with more than about 2000 entries; when the first GraphQL page is unusable (a transport or HTTP failure, GraphQL errors, an unknown project, an unexpected shape, or an empty first page, which is how GraphQL answers a sha it cannot resolve), the adapter falls back to the REST walk. The REST walk reads pages of 100 entries while `X-Next-Page` names a next one, also up to 20 pages, and keeps `blob` entries only, so directories and submodules are left out. Either walk marks the listing incomplete (`complete: false` in the run record, `(partial)` on the `-v` line) when it stops at the cap or a later page fails. When the REST walk's first page fails too (a token without access to the project or any other non-2xx status, a transport error, or a body that is not a list), there is no listing: the run logs one WARNING and continues without the name search and the contract files outside the PR. The GraphQL request carries the same `PRIVATE-TOKEN` header as everything above, and an invalid token was refused there with HTTP `401`. It is a POST, which the adapter never re-sends on a `429` or `5xx` answer, so such an answer to the first page falls back to REST and to a later page ends the listing incomplete. On 2026-09-24 the listing was run live, read-only and with no token: a small public project listed all 40 of its files, complete, in 0.4 s, and a large public monorepo stopped at the 20-page cap with 2,000 files, marked incomplete, after 57.4 s. So on a large project the listing can take about a minute; a `repo` run pays that once, and `off` and `diff` never list. --- @@ -185,6 +188,19 @@ paging rather than `page`/`pagelen`. It therefore gets its own adapter. description history (whether the `/activities` feed carries the old text is unverified), so a `--pr-url` replay here shows the current title and description and logs a warning, and `--as-of` exits `2`. +- **Repository Listing (Repository Context):** at `PRXREF_REPO_CONTEXT=repo`, once per run, + `GET {scheme}://{host}{context}/rest/api/1.0/projects/{key}/repos/{slug}/files?at={head_sha}&start={n}&limit=100`, + following `nextPageStart` until `isLastPage`, at most 20 pages, with the same + credentials as everything above. The values are file paths, so directories are not + listed. The listing is marked incomplete (`complete: false` in the run record, + `(partial)` on the `-v` line) when the walk stops at the cap, when a page says it is + not the last but names no next start, or when a later page fails. A first page that + fails (a missing read permission or any other non-2xx status, a transport error, or a + body with no `values` list) gives no listing: the run logs one WARNING and continues + without the name search and the contract files outside the PR. The endpoint comes + from the Bitbucket Data Center REST documentation and is tested against fakes of that + shape; it has **not been verified against a live instance**, and whether it lists + submodules is unverified. --- @@ -351,3 +367,17 @@ Azure DevOps Services and Azure DevOps Server (on-prem): both speak REST - **Description History (Replay):** none. The adapter cannot read a pull request's description history, so a `--pr-url` replay here shows the current title and description and logs a warning, and `--as-of` exits `2`. +- **Repository Listing (Repository Context):** at `PRXREF_REPO_CONTEXT=repo`, once per + run, one request with no paging: + `GET {base}/items?recursionLevel=Full&versionDescriptor.version={head_sha}&versionDescriptor.versionType=commit`. + Only `blob` entries not flagged `isFolder` are kept, so the root, directories and + submodules are left out, and each path loses its leading slash. A response that + carries an `x-ms-continuationtoken` header is marked incomplete (`complete: false` in + the run record, `(partial)` on the `-v` line), and the token is not followed: + continuation on this endpoint is undocumented and was never observed. A failed request + (a missing read permission or any other non-2xx status, a `203` sign-in page or other + non-JSON body, a transport error, or a body with no `value` list) gives no listing: the + run logs one WARNING and continues without the name search and the contract files + outside the PR. On 2026-09-24 it was run live, read-only and with no token, against a + public Azure DevOps Services project, and returned exactly the files of a raw walk of + that endpoint. Azure DevOps Server is untested, as above. diff --git a/docs/llm.md b/docs/llm.md index 67225a5..b825d61 100644 --- a/docs/llm.md +++ b/docs/llm.md @@ -5,7 +5,7 @@ ## Key Architectural Principles 1. **No Provider Credentials in `prxref`:** `prxref` never looks up, stores, or uses a provider credential (no AWS IAM keys, no OpenAI keys, no Google Cloud keys, and no Anthropic API keys), and its own settings are provider-neutral `PRXREF_*` names. The one key it sends is `PRXREF_LLM_API_KEY`, as a bearer token to the `openai-compat` endpoint you configured. Every provider key lives behind that endpoint, in the provider SDK's own environment (`litellm`), or inside your own logged-in CLI (`claude-cli`, `kiro-cli`). A CLI backend hands its CLI the environment `prxref` was started with; `claude-cli` first removes a fixed list of credential-routing variable *names*, so the CLI falls back to its subscription login, and never reads their values. -2. **Fast Caller-Side Failover:** Fallback is implemented as a fast sequential loop over the model list. If a model encounters HTTP 429 (rate limit), HTTP >= 500 (server/upstream error), connection failures, or timeouts, the client immediately advances to the next model in the chain without same-model retries. +2. **Fast Caller-Side Failover:** Fallback is implemented as a fast sequential loop over the model list. If a model encounters HTTP 429 (rate limit), HTTP >= 500 (server/upstream error), connection failures, or timeouts, the client immediately advances to the next model in the chain without same-model retries. Above the backends, the reviewer asks once more when a reply comes back empty (no text, or whitespace only, and not stopped at the token budget): the same prompt goes out as a new call, which walks the fallback chain like any other, so no backend ever retries a model in place. --- @@ -282,6 +282,7 @@ A review is its chunk workers plus the systemic sweep, and the total covers the - A run that made no LLM request (an empty diff, or a forge or diff error before the review) costs a known `0.0`. - Inside one `openai-compat` call, truncated completions that the fallback chain moved past were billed too, so they are added to that call's figure. Its token counts still cover only the answering model. If any of those completions came back without a figure, the call's figure is unknown. - The timeout retry (the one re-run with `context_lines=0`) replaces the first attempt's result, cost included, exactly as it replaces its tokens. +- The empty-reply retry adds to the first call instead: both calls were billed, so their tokens are summed, and the unit's reported cost is the sum of both figures, or unknown when either call reported none. - If the total cannot be computed at all, for example because a library caller passed a malformed table object, the run logs a WARNING and its cost is unknown. Cost accounting never fails a review. ### Where the cost shows @@ -310,8 +311,10 @@ Each worker sees one chunk's unified diff, trimmed to `PRXREF_CHUNK_CONTEXT_LINE - **`### Dependency versions`** — `name@version` for each third-party package the chunk's *added* lines import, resolved from the nearest manifest walking up from each changed file to the repository root: `package.json` (`dependencies` + `devDependencies`), `pyproject.toml` (`[project] dependencies` and `[tool.poetry.dependencies]`), `go.mod` `require` lines, and `Cargo.toml` `[dependencies]`. Only imported packages appear. Relative and `node:` specifiers, Python stdlib modules, and relative Python imports are excluded. Without this block a reviewer answers library semantics from whichever major dominates its training data. - **`### Definitions referenced by this chunk`** — for identifiers used on added lines whose definition sits in the same file but *outside* the rendered hunk, one `path:line: definition` entry each, taken from the file as served at the PR head. The entry is the defining line plus continuation lines up to a balanced bracket or 6 lines. Caps: at most 40 entries and 8000 characters, with a trailing `… N more definitions omitted` when trimmed; files over 512 KiB are skipped. Definitions the chunk itself adds are never repeated. -Both blocks are **best effort**. They are built from an optional forge method, `get_file_content(ref, path, *, sha) -> str | None`, resolved with `getattr` and always called at the PR head sha (`pr.source_sha`). Every read is cached per run, so one manifest is fetched once no matter how many chunks want it, and any exception from the adapter degrades to no block. A forge that does not implement the method — and a PR with no head sha — reviews exactly as before, with no header and no extra requests. Nothing here can fail a review. +Both blocks are **best effort**. They are built from an optional forge method, `get_file_content(ref, path, *, sha) -> str | None`, resolved with `getattr` and always called at the PR head sha (`pr.source_sha`). Every read is cached per run, so one manifest is fetched once no matter how many chunks want it, and any exception from the adapter degrades to no block. A forge that does not implement the method — and a PR with no head sha — reviews exactly as before, with no header and no extra requests, unless repository context (below) is on. Nothing here can fail a review. + +**Repository context (0.16.0).** With `PRXREF_REPO_CONTEXT` at `diff` or `repo`, the `### Definitions referenced by this chunk` block also carries definitions from other files, after the same-file entries: from the PR's other changed files at `diff`, Java type declarations included, and at `repo` also from files outside the diff that the chunk's imports, the Java path convention or a file-name search point at. At `repo`, a `### Contract excerpts` block of OpenAPI, JSON Schema and migration slices follows it. These entries share the per-chunk `PRXREF_REPO_CONTEXT_MAX_CHARS` budget instead of the caps above, and come from one run-wide reader over the forge or `--repo-dir`, not from the reader above; with neither, only the definitions from the PR's own files are left, built from the diff's hunk lines. Only chunk workers receive them; the whole-PR sweep prompt is unchanged. See `PRXREF_REPO_CONTEXT` in [env-vars.md](env-vars.md). The worker prompt also carries two confidence rules tied to these blocks: a finding that depends on third-party runtime semantics whose version is not listed, or on the semantics of a symbol whose definition is not shown, must cap confidence at 0.5 and be phrased as a question. -On the timeout retry — the one deterministic re-run with `context_lines=0` — the dependency block is kept and the definitions block is dropped, because shrinking the prompt is the entire point of that retry. +On the timeout retry — the one deterministic re-run with `context_lines=0` — the dependency block is kept and the definitions block is dropped, because shrinking the prompt is the entire point of that retry. Repository context goes with it: the retry carries neither the other-file definitions nor the contract excerpts, and the run record marks the chunk `retry_dropped`. The retry follows a chunk call that timed out (`PRXREF_LLM_TIMEOUT`, default `45` seconds), so a model slower than that reviews the chunk without its repository context. diff --git a/pyproject.toml b/pyproject.toml index f734a8a..ca6d170 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "prxref" -version = "0.15.0" +version = "0.16.0" description = "Fast automated AI code review for Bitbucket, GitLab, GitHub, and Azure DevOps" readme = "README.md" requires-python = ">=3.12" diff --git a/src/prxref/__init__.py b/src/prxref/__init__.py index 6765319..d241cb0 100644 --- a/src/prxref/__init__.py +++ b/src/prxref/__init__.py @@ -1,3 +1,3 @@ """prxref — fast automated AI code review for Bitbucket, GitLab, GitHub, and Azure DevOps.""" -__version__ = "0.15.0" +__version__ = "0.16.0" diff --git a/src/prxref/chunk_context.py b/src/prxref/chunk_context.py index d597634..265ccc2 100644 --- a/src/prxref/chunk_context.py +++ b/src/prxref/chunk_context.py @@ -39,6 +39,7 @@ DEPENDENCY_HEADER = "### Dependency versions" DEFINITIONS_HEADER = "### Definitions referenced by this chunk" SIBLING_HEADER = "### Other files changed in this PR" +CONTRACT_HEADER = "### Contract excerpts" _JS_SUFFIXES = (".js", ".jsx", ".mjs", ".cjs", ".ts", ".tsx", ".mts", ".cts") @@ -444,17 +445,31 @@ def referenced_definitions( return out -def render_context_blocks(dep_lines: Sequence[str], def_lines: Sequence[str]) -> str: - """Render the two prompt blocks, omitting each when it has no lines. - - Returns the empty string when both are empty, so the prompt slot leaves no - stray header behind. +def render_context_blocks( + dep_lines: Sequence[str], + def_lines: Sequence[str], + extra_def_lines: Sequence[str] = (), + contract_lines: Sequence[str] = (), +) -> str: + """Render the prompt blocks, omitting each when it has no lines. + + The dependency block comes first. The definitions block holds + ``def_lines`` followed by ``extra_def_lines`` (repository-context + definitions) under one :data:`DEFINITIONS_HEADER`, and is present when + either is non-empty. The contracts block, ``contract_lines`` under + :data:`CONTRACT_HEADER`, comes last. With the two optional arguments + empty, the output is exactly the two-block rendering repository context + predates. Returns the empty string when every list is empty, so the + prompt slot leaves no stray header behind. """ blocks: list[str] = [] if dep_lines: blocks.append(DEPENDENCY_HEADER + "\n\n" + "\n".join(dep_lines)) - if def_lines: - blocks.append(DEFINITIONS_HEADER + "\n\n" + "\n".join(def_lines)) + definitions = [*def_lines, *extra_def_lines] + if definitions: + blocks.append(DEFINITIONS_HEADER + "\n\n" + "\n".join(definitions)) + if contract_lines: + blocks.append(CONTRACT_HEADER + "\n\n" + "\n".join(contract_lines)) return "\n\n".join(blocks) diff --git a/src/prxref/cli.py b/src/prxref/cli.py index 93537cc..e5923af 100644 --- a/src/prxref/cli.py +++ b/src/prxref/cli.py @@ -43,6 +43,16 @@ before the URL is parsed, and a bad set exits 2 naming the flag. The webhook daemon never replays. +``--repo-dir PATH`` names a local checkout of the repository at the PR head. +With ``PRXREF_REPO_CONTEXT`` at ``diff`` or ``repo``, repository context +reads files there (and, at ``repo``, lists them) instead of calling the +forge, so a ``--diff-file`` review gets repository context with no network. +It is not a replay flag: on its own it neither stops posting nor stamps +``replay``. The directory is checked before any network call, so a path that +is not an existing directory exits 2 naming ``--repo-dir``; it is checked and +passed even when ``PRXREF_REPO_CONTEXT`` is ``off``, which ignores it. The +webhook daemon never takes one. + A ``--pr-url`` replay also pins the PR's title and description by default (issue #16): it shows the ones in force at a cutoff, which is ``--as-of TIME`` when given, else the PR's first human review, else its head commit's @@ -112,6 +122,7 @@ from prxref.costs import cost_label from prxref.forges.base import detect_forge from prxref.forges.replay import DescriptionPin, LocalDiffForge, ReplayForge, choose_cutoff, pin_status +from prxref.forges.repo_dir import RepoDir from prxref.llm import ConfigError from prxref.prompt_templates import export_prompt_templates, load_prompt_templates from prxref.rules import load_review_rules, load_scoped_rules @@ -257,6 +268,16 @@ def _build_parser() -> argparse.ArgumentParser: "implies no posting" ), ) + rev.add_argument( + "--repo-dir", + default=None, + metavar="PATH", + help=( + "a local checkout of the repository at the PR head; with " + "PRXREF_REPO_CONTEXT=repo, repository context reads and lists " + "files there instead of calling the forge" + ), + ) rev.add_argument( "--as-of", default=None, @@ -523,9 +544,12 @@ def _print_summary( (the number of scoped rules files, then ``=`` for each in load order, then ``cap=``), ``prompts:`` (the directory, then ``=`` for each overridden template - in name order), ``ticket:`` (with the active findings' scope counts), and - ``spec:``. ``result`` may be partial, or not a dict at all; a missing or - ``None`` record prints nothing. + in name order), ``ticket:`` (with the active findings' scope counts), + ``spec:``, and ``repo context:`` (the repository-context level, reader, + listing, read count, read cap and entry totals; see + :func:`_repo_context_line`). ``result`` may be partial, or not a dict at + all; a missing or ``None`` record prints nothing, so ``repo context:`` + is absent while ``PRXREF_REPO_CONTEXT`` is ``off``. """ target = sys.stdout if out is None else out record = result if isinstance(result, dict) else {} @@ -593,6 +617,38 @@ def _print_summary( f"{_dash(spec.get('constraints'))} constraint(s)", file=target, ) + repo = record.get("repo_context") + if isinstance(repo, dict): + print(_repo_context_line(repo), file=target) + + +def _repo_context_line(repo: dict) -> str: + """Render the ``-v`` summary line for a ``repo_context`` run-record value. + + ``repo context: mode= reader= listing= + reads= cap_hit= entries= omitted=`` on one line. + ``listing`` is the listed path count, suffixed ``(partial)`` when the + listing is incomplete, or ``-`` without one; ``entries`` and ``omitted`` + are summed over every chunk row of ``units`` and are 0 when ``units`` + is ``None``. A missing value prints ``-``. + """ + listing = repo.get("listing") + if isinstance(listing, dict): + partial = "(partial)" if listing.get("complete") is False else "" + listed = f"{_dash(listing.get('paths'))}{partial}" + else: + listed = "-" + units = repo.get("units") + chunks = units.get("chunks") if isinstance(units, dict) else None + rows = [row for row in chunks if isinstance(row, dict)] if isinstance(chunks, list) else [] + entries = sum(len(row["entries"]) for row in rows if isinstance(row.get("entries"), list)) + omitted = sum(row["omitted"] for row in rows if isinstance(row.get("omitted"), int)) + cap_hit = "yes" if repo.get("read_cap_hit") else "no" + return ( + f"repo context: mode={_dash(repo.get('mode'))} reader={_dash(repo.get('reader'))} " + f"listing={listed} reads={_dash(repo.get('reads'))} cap_hit={cap_hit} " + f"entries={entries} omitted={omitted}" + ) def _fmt_finding_line(f: Any) -> str: @@ -702,19 +758,20 @@ def _build_json_result(result: Any) -> dict: ``chunks_failed``, ``elapsed_ms``, ``input_tokens``, ``output_tokens``, ``cost_usd``, ``cost_estimated``, ``posted``, ``review_rules``, ``ticket_context``, ``spec_grounding``, ``size_advisory``, - ``prompt_templates``, ``scoped_rules``, ``rule_counts``, then - ``sampling`` and ``replay`` when present. + ``prompt_templates``, ``scoped_rules``, ``rule_counts``, + ``repo_context``, then ``sampling`` and ``replay`` when present. Tolerates an error-shaped or partial result (a dict missing keys, as an incomplete or failed run may return): every always-present key defaults to ``None`` and ``findings`` defaults to ``[]`` rather than raising. The - run-record keys new in 0.14 (``cost_usd`` through ``size_advisory``) and - 0.15's ``prompt_templates``, ``scoped_rules`` and ``rule_counts`` are - always emitted and are ``null`` when their feature is off - (``rule_counts`` whenever the per-rule cap did not run); ``cost_usd`` is - also ``null`` when no source could price the run, never ``0``. Every - ``findings`` row, active or dropped, carries 0.15's ``rule`` and - ``locations`` the same way (see :func:`_finding_json`). + run-record keys new in 0.14 (``cost_usd`` through ``size_advisory``), + 0.15's ``prompt_templates``, ``scoped_rules`` and ``rule_counts``, and + 0.16's ``repo_context`` are always emitted and are ``null`` when their + feature is off (``rule_counts`` whenever the per-rule cap did not run, + ``repo_context`` whenever ``PRXREF_REPO_CONTEXT`` is ``off``); + ``cost_usd`` is also ``null`` when no source could price the run, never + ``0``. Every ``findings`` row, active or dropped, carries 0.15's ``rule`` + and ``locations`` the same way (see :func:`_finding_json`). ``sampling`` and ``replay`` are forwarded only when the result already carries them. ``replay`` is on replay runs only, so a normal run's payload has no ``replay`` key at all. @@ -747,6 +804,7 @@ def _build_json_result(result: Any) -> dict: "prompt_templates": result.get("prompt_templates"), "scoped_rules": result.get("scoped_rules"), "rule_counts": result.get("rule_counts"), + "repo_context": result.get("repo_context"), } if "sampling" in result: payload["sampling"] = result["sampling"] @@ -1164,6 +1222,25 @@ def _load_prompts_dir(path: str | None, *, source: str) -> Any: raise ConfigError(f"{source}: cannot load prompts directory {path!r}: {exc}") from exc +def _open_repo_dir(path: str | None) -> RepoDir | None: + """Open ``--repo-dir`` as a :class:`~prxref.forges.repo_dir.RepoDir`, or ``None`` when it is not given. + + ``_run_review`` calls it before any network call, so a path that is not + an existing directory raises ``ConfigError`` naming ``--repo-dir`` and + exits 2. A blank path is refused the same way: the flag has no variable + for ``""`` to turn off, and a blank path would otherwise resolve to the + working directory. + """ + if path is None: + return None + if not path.strip(): + raise ConfigError(f"--repo-dir: no such directory {path!r}") + try: + return RepoDir(path) + except (OSError, ValueError) as exc: + raise ConfigError(f"--repo-dir: no such directory {path!r}") from exc + + def _run_review( url: str | None, *, @@ -1183,6 +1260,7 @@ def _run_review( as_of: str | None = None, description_file: str | None = None, no_description: bool = False, + repo_dir: str | None = None, ) -> Any: replay = _resolve_replay( url, base_sha=base_sha, head_sha=head_sha, no_threads=no_threads, @@ -1238,9 +1316,9 @@ def _run_review( "prompts_dir": "--prompts-dir", }, ) - # The rules files, the ticket file and the prompts directory are read - # here, after config and before make_forge and the LLM client, so an - # unusable one exits 2 before any network I/O. load_config stays I/O-free. + # The rules files, the ticket file, the prompts directory and --repo-dir + # are read here, after config and before make_forge and the LLM client, so + # an unusable one exits 2 before any network I/O. load_config stays I/O-free. # Each is reported under the input that supplied its path: the flag # whenever it was given, else the variable. The scoped rules are checked # against the always-on file, so one team word mapped to two tiers across @@ -1266,6 +1344,7 @@ def _run_review( cfg["prompts_dir"], source="--prompts-dir" if prompts_dir is not None else "PRXREF_PROMPTS_DIR", ) + repo = _open_repo_dir(repo_dir) # PRXREF_DRY_RUN is the standing "never write to the forge" switch and # --no-post is the per-invocation one; either alone suppresses posting, so # the flag still wins when the environment says nothing. This sits inside @@ -1346,6 +1425,11 @@ def _run_review( prompts=prompts, scoped_rules=scoped, scoped_rules_max_chars=cfg["scoped_rules_max_chars"], + repo_context=cfg["repo_context"], + repo_context_max_chars=cfg["repo_context_max_chars"], + context_contract_globs=cfg["context_contract_globs"], + context_exclude_globs=cfg["context_exclude_globs"], + repo_dir=repo, ) @@ -1443,6 +1527,7 @@ def _cmd_review(args: argparse.Namespace) -> int: as_of=args.as_of, description_file=args.description_file, no_description=args.no_description, + repo_dir=args.repo_dir, ) except ConfigError as exc: print(f"configuration error: {exc}", file=sys.stderr) diff --git a/src/prxref/config.py b/src/prxref/config.py index 193b281..ccd85b4 100644 --- a/src/prxref/config.py +++ b/src/prxref/config.py @@ -248,6 +248,45 @@ Characters of ticket text kept in the prompt; longer is truncated with a visible marker; positive int (default 6000) + PRXREF_REPO_CONTEXT Repository context (0.16.0): "off" (default) | + "diff" | "repo". "off" leaves every prompt, + post, trace and default-verbosity log + byte-identical to 0.15.0. "diff" adds + cross-chunk definitions from other files + already in the diff, plus diff-file entries, + all read from the diff itself; no repository + reader is needed. "repo" also reads files + outside the diff — import, path-convention and + name-search definitions, plus contract excerpts + — through the forge's repository reader when + one is available, or --repo-dir. Matching is + exact and case-sensitive, like PRXREF_FAIL_ON; + any other value is a configuration error + PRXREF_REPO_CONTEXT_MAX_CHARS Repository context (0.16.0): per-chunk + character budget shared by the cross-chunk, + contract and other repository-context entries; + positive int (default 12000) + PRXREF_CONTEXT_CONTRACT_GLOBS Repository context (0.16.0): globs (matched + like PRXREF_SIZE_IGNORE_GLOBS) selecting the + contract files — OpenAPI, JSON Schema, + Liquibase/SQL migrations — excerpted under + "repo". A set value REPLACES the built-in set + below rather than adding to it, and an empty + value reads as unset (the built-in set stays); + there is no way to turn contract excerpts off + on their own in 0.16.0 short of setting + PRXREF_REPO_CONTEXT to "off" or "diff". + Built-in set: **/openapi*.y*ml, + **/openapi*.json, **/openapi/**, **/swagger*, + **/*.schema.json, **/db/changelog/**, + **/db/migration/**, **/migrations/** + PRXREF_CONTEXT_EXCLUDE_GLOBS Repository context (0.16.0): globs (matched + like PRXREF_SIZE_IGNORE_GLOBS) whose paths are + never read for repository context, not even a + diff file. ADDED to a floor that is always on: + **/expected.json, **/cases.json, **/case.json, + **/prxref-eval/**, **/.env*, **/*.pem, + **/*.key. Empty (the default) adds nothing Spec sources / Jira: PRXREF_JIRA_BASE_URL Jira base URL (scheme://host plus any @@ -298,8 +337,9 @@ webhooks (default off; insecure) List-valued keys (PRXREF_LLM_MODELS, PRXREF_SPEC_SOURCES, -PRXREF_SIZE_IGNORE_GLOBS and PRXREF_SCOPED_RULES) split on any run of commas -and/or whitespace, so no item can contain either; a glob that must match a +PRXREF_SIZE_IGNORE_GLOBS, PRXREF_SCOPED_RULES, PRXREF_CONTEXT_CONTRACT_GLOBS +and PRXREF_CONTEXT_EXCLUDE_GLOBS) split on any run of commas and/or +whitespace, so no item can contain either; a glob that must match a literal space writes it as ``?``. Precedence: built-in defaults < environment < ``overrides`` kwargs. @@ -396,6 +436,26 @@ "prompts_dir": None, "ticket_context_file": "", "ticket_context_max_chars": 6000, + "repo_context": "off", + "repo_context_max_chars": 12000, + # The built-in contract-glob set. Unlike the + # other _LIST_KEYS defaults, this one is non-empty: an env value REPLACES + # it rather than adding to it, and an empty value reads as unset (the + # normal "empty or whitespace-only reads as unset" rule), so this set + # stays in place. A ``set`` literal would work too -- the _LIST_KEYS + # coercion branch always returns a ``list`` -- but the default is typed + # as a ``list`` from the start so both paths give callers the same type. + "context_contract_globs": [ + "**/openapi*.y*ml", + "**/openapi*.json", + "**/openapi/**", + "**/swagger*", + "**/*.schema.json", + "**/db/changelog/**", + "**/db/migration/**", + "**/migrations/**", + ], + "context_exclude_globs": [], "jira_base_url": "", "jira_email": "", "jira_api_token": "", @@ -424,7 +484,7 @@ "llm_cli_concurrency", "review_rules_max_chars", "ticket_context_max_chars", "size_warn_lines", "size_warn_files", "max_warning_findings", "max_outofscope_findings", "scoped_rules_max_chars", - "max_findings_per_rule", + "max_findings_per_rule", "repo_context_max_chars", }) _FLOAT_KEYS = frozenset({"confidence_floor", "llm_timeout", "dedup_similarity"}) _BOOL_KEYS = frozenset({ @@ -432,6 +492,7 @@ }) _LIST_KEYS = frozenset({ "llm_models", "spec_sources", "size_ignore_globs", "scoped_rules", + "context_contract_globs", "context_exclude_globs", }) # An enum-valued key has no numeric interval to check, so its legal vocabulary @@ -441,6 +502,7 @@ # PRXREF_FAIL_ON=eror into an undetected "never". _CHOICE_KEYS: dict[str, frozenset[str]] = { "fail_on": frozenset({"never", "error", "any"}), + "repo_context": frozenset({"off", "diff", "repo"}), } # The posting-behaviour vocabulary, validated rather than trusted. Restated in @@ -522,6 +584,7 @@ def describe(self) -> str: "max_outofscope_findings": _Range(0, low_inclusive=True), "max_findings_per_rule": _Range(0, low_inclusive=True), "scoped_rules_max_chars": _Range(0), + "repo_context_max_chars": _Range(0), "confidence_floor": _Range(0.0, 1.0, low_inclusive=True), "dedup_similarity": _Range(0.0, 1.0), } diff --git a/src/prxref/eval_cases.py b/src/prxref/eval_cases.py index f68aa60..31d5104 100644 --- a/src/prxref/eval_cases.py +++ b/src/prxref/eval_cases.py @@ -7,21 +7,21 @@ A ``cases.json`` file is ``{"version": 1, "cases": [...]}``. Each case is an object with a required ``id`` and ``expected`` and the optional ``pr_url``, -``base_sha``, ``head_sha``, ``diff_file``, ``context_file`` and ``spec`` (one -string or a list of them). ``expected`` is a list, possibly empty, of human -findings, each with ``id``, ``file``, ``line`` and ``severity`` and the -optional ``category``, ``accepted``, ``text`` and ``must_match``. An optional -field set to ``null`` counts as absent. A relative ``diff_file``, -``context_file`` or ``spec`` path is read relative to the directory holding -the ``cases.json`` file; a ``spec`` entry that is an ``http(s)`` URL is kept -as given. +``base_sha``, ``head_sha``, ``diff_file``, ``context_file``, ``repo_dir`` and +``spec`` (one string or a list of them). ``expected`` is a list, possibly +empty, of human findings, each with ``id``, ``file``, ``line`` and +``severity`` and the optional ``category``, ``accepted``, ``text`` and +``must_match``. An optional field set to ``null`` counts as absent. A +relative ``diff_file``, ``context_file``, ``repo_dir`` or ``spec`` path is +read relative to the directory holding the ``cases.json`` file; a ``spec`` +entry that is an ``http(s)`` URL is kept as given. A ``case-*/`` directory supplies ``diff.patch`` (required) as its ``diff_file``, ``expected.json`` (required: the ``expected`` list above) and, -when present, ``ticket.md`` as its ``context_file`` and ``docs/`` as its one -``spec`` source. Its id is the directory name. ``expected.json`` spells -``line`` as ``line_hint`` and ``category`` as ``source``, and every message -about it uses those spellings. +when present, ``ticket.md`` as its ``context_file``, ``repo/`` as its +``repo_dir`` and ``docs/`` as its one ``spec`` source. Its id is the +directory name. ``expected.json`` spells ``line`` as ``line_hint`` and +``category`` as ``source``, and every message about it uses those spellings. A case replays either a local diff (``diff_file``) or a pinned commit range of a pull request (``pr_url`` with both ``base_sha`` and ``head_sha``), and may @@ -72,7 +72,8 @@ _FULL_SHA_RE = re.compile(r"[0-9a-fA-F]{40}(?:[0-9a-fA-F]{24})?") _TOP_KEYS = ("version", "cases") _CASE_KEYS = ( - "id", "pr_url", "base_sha", "head_sha", "diff_file", "context_file", "spec", "expected", + "id", "pr_url", "base_sha", "head_sha", "diff_file", "context_file", "repo_dir", "spec", + "expected", ) _EXPECTED_KEYS = ( "id", "file", "line", "severity", "category", "accepted", "text", "must_match", @@ -109,12 +110,15 @@ class EvalCase: """One validated eval case: what to replay and the findings it should yield. ``id`` is a single safe path segment, unique within its dataset. - ``diff_file`` and ``context_file`` are paths ready to open, already - joined onto the ``cases.json`` directory or the case directory; ``spec`` - holds the spec sources in order, each such a path or an ``http(s)`` URL, - and is empty when the case sets none. ``base_sha`` and ``head_sha`` are - both full lowercased SHAs, and ``pr_url`` is set, or both are ``None``. - ``expected`` keeps the labels in file order and may be empty. + ``diff_file``, ``context_file`` and ``repo_dir`` are paths ready to open, + already joined onto the ``cases.json`` directory or the case directory; + ``repo_dir`` is a directory holding the repository at the PR head, read + by repository context (``PRXREF_REPO_CONTEXT=repo``) in place of a forge. + ``spec`` holds the spec sources in order, each such a path or an + ``http(s)`` URL, and is empty when the case sets none. ``base_sha`` and + ``head_sha`` are both full lowercased SHAs, and ``pr_url`` is set, or + both are ``None``. ``expected`` keeps the labels in file order and may + be empty. """ id: str @@ -125,6 +129,7 @@ class EvalCase: diff_file: str | None = None context_file: str | None = None spec: tuple[str, ...] = () + repo_dir: str | None = None def load_cases(path: str | os.PathLike[str], *, source: str = "--cases") -> list[EvalCase]: @@ -171,11 +176,12 @@ def case_to_json(case: EvalCase) -> dict[str, Any]: The keys follow the ``cases.json`` spelling in a fixed order: ``id``, ``pr_url``, ``base_sha``, ``head_sha``, ``diff_file``, ``context_file``, - ``spec`` (a list, empty when the case sets none) and ``expected``, a list - of labels each keyed ``id``, ``file``, ``line``, ``severity``, - ``category``, ``accepted``, ``text`` and ``must_match``. Every field is - written, ``null`` when unset. Paths are written exactly as the case holds - them, already joined onto the dataset directory when it was loaded. + ``repo_dir``, ``spec`` (a list, empty when the case sets none) and + ``expected``, a list of labels each keyed ``id``, ``file``, ``line``, + ``severity``, ``category``, ``accepted``, ``text`` and ``must_match``. + Every field is written, ``null`` when unset. Paths are written exactly + as the case holds them, already joined onto the dataset directory when + it was loaded. """ return { "id": case.id, @@ -184,6 +190,7 @@ def case_to_json(case: EvalCase) -> dict[str, Any]: "head_sha": case.head_sha, "diff_file": case.diff_file, "context_file": case.context_file, + "repo_dir": case.repo_dir, "spec": list(case.spec), "expected": [ {key: getattr(finding, key) for key in _EXPECTED_KEYS} for finding in case.expected @@ -219,7 +226,7 @@ def case_from_json_record(obj: Any, *, source: str = "case.json") -> EvalCase: ) texts = { key: _optional_text(obj, key, where, source) - for key in ("pr_url", "base_sha", "head_sha", "diff_file", "context_file") + for key in ("pr_url", "base_sha", "head_sha", "diff_file", "context_file", "repo_dir") } spec = obj.get("spec") if spec is None: @@ -292,6 +299,7 @@ def _case_from_json(entry: Any, index: int, base: Path, source: str) -> EvalCase head_sha = _optional_text(entry, "head_sha", where, source) diff_file = _optional_text(entry, "diff_file", where, source) context_file = _optional_text(entry, "context_file", where, source) + repo_dir = _optional_text(entry, "repo_dir", where, source) base_sha, head_sha = _check_replay(pr_url, base_sha, head_sha, diff_file, where, source) if pr_url is not None and detect_forge(pr_url) is None: raise ConfigError( @@ -302,6 +310,10 @@ def _case_from_json(entry: Any, index: int, base: Path, source: str) -> EvalCase context_file = _join(base, context_file) if not Path(context_file).is_file(): raise ConfigError(f"{source}: {where}: context_file: no such file {context_file!r}") + if repo_dir is not None: + repo_dir = _join(base, repo_dir) + if not Path(repo_dir).is_dir(): + raise ConfigError(f"{source}: {where}: repo_dir: no such directory {repo_dir!r}") spec = _spec_sources(entry.get("spec"), base, where, source) if "expected" not in entry: raise ConfigError(f"{source}: {where}: expected: required (an array of findings, possibly empty)") @@ -312,7 +324,7 @@ def _case_from_json(entry: Any, index: int, base: Path, source: str) -> EvalCase _check_anchors(expected, files, where, "expected", _JSON_SPELLING, source) return EvalCase( id=case_id, expected=expected, pr_url=pr_url, base_sha=base_sha, head_sha=head_sha, - diff_file=diff_file, context_file=context_file, spec=spec, + diff_file=diff_file, context_file=context_file, repo_dir=repo_dir, spec=spec, ) @@ -413,11 +425,13 @@ def _case_from_directory(case_dir: Path, source: str) -> EvalCase: _check_anchors(expected, files, where, "expected.json", _DIRECTORY_SPELLING, source) ticket = case_dir / "ticket.md" docs = case_dir / "docs" + repo = case_dir / "repo" return EvalCase( id=case_id, expected=expected, diff_file=diff_file, context_file=str(ticket) if ticket.is_file() else None, + repo_dir=str(repo) if repo.is_dir() else None, spec=(str(docs),) if docs.is_dir() else (), ) diff --git a/src/prxref/evals.py b/src/prxref/evals.py index 8c29144..a39d4a5 100644 --- a/src/prxref/evals.py +++ b/src/prxref/evals.py @@ -64,6 +64,10 @@ "max_outofscope_findings", "max_findings_per_rule", "scoped_rules_max_chars", + "repo_context", + "repo_context_max_chars", + "context_contract_globs", + "context_exclude_globs", ) SCORE_VERSION = 1 SCORE_RUN_KEYS = ("prompts", "sampling", "review_rules", "scoped_rules", "config") @@ -105,7 +109,8 @@ def eval_run( and ``no_threads=True``, so nothing is ever posted. ``context_file`` is ``""`` and ``spec_sources`` is ``[]`` unless the case sets them, so the environment's ticket and spec inputs cannot leak into a case. - ``args.rules_file``, ``args.scoped_rules`` and ``args.prompts_dir`` are + ``repo_dir`` is the case's ``repo_dir``, as ``review --repo-dir`` takes + it, or ``None``. ``args.rules_file``, ``args.scoped_rules`` and ``args.prompts_dir`` are passed to every case as given, as ``review --rules-file``, ``--scoped-rules`` and ``--prompts-dir`` take them: ``None`` leaves the variable in force, and ``""`` (``[""]`` for the scoped rules) turns it @@ -115,7 +120,8 @@ def eval_run( Each case is fenced. A crash, a ``None`` result (an unrecognised URL) or any other exception is recorded as that case's ``error.json`` and the next case still runs. That includes a ``ConfigError`` the review raises for one - case, such as an unreadable ``diff_file``: it is a failed case, not exit + case, such as an unreadable ``diff_file`` or a ``repo_dir`` that has + vanished since the dataset was loaded: it is a failed case, not exit 2. A review whose verdict is ``Error`` is recorded as a normal ``record.json`` carrying that verdict. The run returns 0 whatever the cases did. @@ -272,6 +278,7 @@ def _run_case( scoped_rules=args.scoped_rules, prompts_dir=args.prompts_dir, trace_dir=str(trace_dir), + repo_dir=case.repo_dir, ) if result is None: raise RuntimeError(f"unrecognized PR URL {case.pr_url!r}") diff --git a/src/prxref/forges/azure_devops.py b/src/prxref/forges/azure_devops.py index 626dd09..0f60fa0 100644 --- a/src/prxref/forges/azure_devops.py +++ b/src/prxref/forges/azure_devops.py @@ -35,6 +35,7 @@ SUMMARY_MARKER, FeedReadError, InlineComment, + PathListing, PRData, PRRef, Thread, @@ -642,6 +643,59 @@ def get_file_content(self, ref: PRRef, path: str, *, sha: str) -> str | None: return None return bytes(buf).decode("utf-8", errors="replace") + def list_paths(self, ref: PRRef, *, sha: str) -> PathListing | None: + """Return every file path in the repository at commit ``sha``, best-effort. + + One request to the ``items`` endpoint with ``recursionLevel=Full`` at + the commit, starting at the repository root, with no paging. Only + ``blob`` entries that are not flagged ``isFolder`` are kept, so the + root (``/``), directories (``tree``) and every other object type, + submodules included, are dropped. Azure DevOps returns each path with + a leading slash, which is stripped; the paths are sorted and + deduplicated. + + ``complete`` is ``False`` when the response carries an + ``x-ms-continuationtoken`` header. Continuation on this endpoint is + undocumented and was never observed, so the token is not followed: the + paths already returned are kept and the listing is marked incomplete. + An empty ``sha`` (no request is made), a transport failure, a non-2xx + status, a 203 sign-in page or other non-JSON body, or a body with no + ``value`` list gives ``None``. Never raises. + """ + if not sha: + return None + where = f"{ref.owner}/{ref.repo}@{sha}" + try: + resp = self._get( + f"{self._api_base(ref)}/items", + { + "recursionLevel": "Full", + "versionDescriptor.version": sha, + "versionDescriptor.versionType": "commit", + }, + ) + body = self._json(resp, "item listing") + except (requests.RequestException, ValueError) as e: + logger.debug("list_paths failed for %s: %s", where, e) + return None + values = body.get("value") + if not isinstance(values, list): + logger.debug("list_paths got no value list for %s", where) + return None + paths = { + entry["path"].lstrip("/") for entry in values + if isinstance(entry, dict) and entry.get("gitObjectType") == "blob" + and not entry.get("isFolder") + and isinstance(entry.get("path"), str) and entry["path"].lstrip("/") + } + complete = not resp.headers.get("x-ms-continuationtoken") + if not complete: + logger.debug( + "list_paths got an x-ms-continuationtoken for %s and did not follow it; " + "the listing is incomplete", where, + ) + return PathListing(paths=tuple(sorted(paths)), complete=complete) + def _read_threads(self, ref: PRRef) -> list[dict]: """Read every thread on the PR (one response; the API does not page them). diff --git a/src/prxref/forges/base.py b/src/prxref/forges/base.py index 8f9450b..3d329d5 100644 --- a/src/prxref/forges/base.py +++ b/src/prxref/forges/base.py @@ -155,6 +155,24 @@ def __post_init__(self) -> None: raise ValueError(f"PRHistory.title_renames must hold TitleRename, got {type(rename).__name__}") +MAX_LISTING_PAGES = 20 + + +@dataclass(frozen=True) +class PathListing: + """Every file path in a repository at one commit, from ``list_paths``. + + ``paths`` holds repo-relative FILE paths, sorted and deduplicated, with + no directories and no submodules. ``complete`` is ``False`` when the + forge truncated the listing or the walk stopped at ``MAX_LISTING_PAGES`` + pages, so a path missing from an incomplete listing may still exist. + A listing is immutable and hashable. + """ + + paths: tuple[str, ...] + complete: bool + + SUMMARY_MARKER = "" # The attribution prefix every posted comment carries (the full line is @@ -269,6 +287,20 @@ def get_pr_history(self, ref: PRRef, *, head_sha: str | None = None) -> PRHistor """ ... + def list_paths(self, ref: PRRef, *, sha: str) -> PathListing | None: + """Return every file path in the repository at commit ``sha``. + + Optional: callers resolve it with ``getattr(forge, "list_paths", None)``, + so a Forge without it is still valid, and not every forge implements it + (repository context then works without a listing). The paths are + repo-relative file paths, sorted and deduplicated, with no directories + and no submodules. A listing the forge truncated, or whose walk stopped + at ``MAX_LISTING_PAGES`` pages, is returned with ``complete=False``. + Returns ``None`` when no listing can be fetched at all; this method + never raises. + """ + ... + def detect_forge(url: str) -> PRRef | None: """Try each registered forge's URL parser in order. diff --git a/src/prxref/forges/bitbucket.py b/src/prxref/forges/bitbucket.py index 1e7ab8d..7c91dac 100644 --- a/src/prxref/forges/bitbucket.py +++ b/src/prxref/forges/bitbucket.py @@ -13,10 +13,12 @@ from prxref.forges.base import ( ATTRIBUTION_MARKER, + MAX_LISTING_PAGES, SUMMARY_MARKER, DescriptionVersion, FeedReadError, InlineComment, + PathListing, PRData, PRHistory, PRRef, @@ -36,6 +38,9 @@ # 500 comments. 50 puts the ceiling far past any real PR, and running out of # budget is now a refusal to post rather than an invisible short read. _MAX_PAGES = 50 +# list_paths asks /src for this many directory levels; the orchestrator's +# probe saw 64 accepted and the whole tree returned. +_LISTING_MAX_DEPTH = 64 # get_file_content is best-effort context, not the review itself: a body past # this size (or one that looks binary) is worth skipping rather than shipping # hundreds of KB into a worker prompt. @@ -467,6 +472,110 @@ def get_file_content(self, ref: PRRef, path: str, *, sha: str) -> str | None: return None return content.decode("utf-8", errors="replace") + def list_paths(self, ref: PRRef, *, sha: str) -> PathListing | None: + """Return every file path in the repository at commit ``sha``, best-effort. + + Reads the repository-level ``/src/{sha}/`` listing with ``max_depth`` + set to ``_LISTING_MAX_DEPTH`` and ``pagelen`` to ``_PAGE_SIZE``, then + follows each page's ``next`` URL verbatim, adding no parameters of its + own, until a page carries no ``next``. Only ``commit_file`` entries are + kept, so directories (``commit_directory``) and every other type, + submodules included, are dropped; the paths are sorted and + deduplicated. + + ``complete`` is ``False`` when the walk stops at ``MAX_LISTING_PAGES`` + pages with a ``next`` still to follow, when a page after the first + cannot be read (the paths read so far are returned), or when a + returned ``commit_directory`` sits at the depth limit, its path + holding ``_LISTING_MAX_DEPTH - 1`` slashes. That depth rule is + inferred from probing, not documented by Bitbucket: ``max_depth=N`` + was observed to list N directory levels, so a directory on level N is + listed without its contents. An empty ``sha`` (no request is made) + gives ``None``, as does a first request that fails in transport, + returns a non-2xx status, or returns a body that is not JSON or holds + no ``values`` list. Never raises. + """ + if not sha: + return None + headers, auth = self._get_auth() + url = f"{_API_BASE}/repositories/{ref.owner}/{ref.repo}/src/{quote(sha, safe='')}/" + params: dict[str, int] | None = {"max_depth": _LISTING_MAX_DEPTH, "pagelen": _PAGE_SIZE} + where = f"{ref.owner}/{ref.repo}@{sha}" + files: set[str] = set() + at_depth_limit = False + for page in range(MAX_LISTING_PAGES): + body = self._read_listing_page(url, params, headers, auth, where) + if body is None: + if page == 0: + return None + return PathListing(paths=tuple(sorted(files)), complete=False) + for entry in body["values"]: + if not isinstance(entry, dict): + continue + path = entry.get("path") + if not isinstance(path, str) or not path: + continue + if entry.get("type") == "commit_file": + files.add(path) + elif ( + entry.get("type") == "commit_directory" + and path.rstrip("/").count("/") >= _LISTING_MAX_DEPTH - 1 + ): + at_depth_limit = True + next_url = body.get("next") + if not next_url: + if at_depth_limit: + logger.debug( + "list_paths reached max_depth=%d for %s; the listing is incomplete", + _LISTING_MAX_DEPTH, where, + ) + return PathListing(paths=tuple(sorted(files)), complete=not at_depth_limit) + url = next_url + params = None + logger.debug( + "list_paths stopped at the %d-page cap for %s; the listing is incomplete", + MAX_LISTING_PAGES, where, + ) + return PathListing(paths=tuple(sorted(files)), complete=False) + + def _read_listing_page( + self, + url: str, + params: dict[str, int] | None, + headers: dict[str, str], + auth: tuple[str, str] | None, + where: str, + ) -> dict | None: + """Return one ``/src`` listing page, or ``None`` (logged at DEBUG) when it cannot be read. + + A page is readable when the request succeeds with a 2xx status and a + JSON object holding a ``values`` list and, if it has one, a string + ``next``. + """ + try: + resp = self._session.get( + url, params=params, headers=headers, auth=auth, timeout=_REQUEST_TIMEOUT + ) + except requests.RequestException as e: + logger.debug("list_paths failed for %s: %s", where, e) + return None + if not resp.ok: + logger.debug("list_paths got HTTP %s for %s", resp.status_code, where) + return None + try: + body = resp.json() + except ValueError as e: + logger.debug("list_paths got a non-JSON body for %s: %s", where, e) + return None + if ( + not isinstance(body, dict) + or not isinstance(body.get("values"), list) + or not isinstance(body.get("next") or "", str) + ): + logger.debug("list_paths got no values list for %s", where) + return None + return body + def prune_inline_comments(self, ref: PRRef) -> int: """Delete prxref-attributed inline comments; returns the count removed. diff --git a/src/prxref/forges/bitbucket_server.py b/src/prxref/forges/bitbucket_server.py index e64599b..3798029 100644 --- a/src/prxref/forges/bitbucket_server.py +++ b/src/prxref/forges/bitbucket_server.py @@ -12,9 +12,11 @@ from prxref.forges.base import ( ATTRIBUTION_MARKER, + MAX_LISTING_PAGES, SUMMARY_MARKER, FeedReadError, InlineComment, + PathListing, PRData, PRRef, Thread, @@ -603,6 +605,95 @@ def get_file_content(self, ref: PRRef, path: str, *, sha: str) -> str | None: return None return content.decode("utf-8", errors="replace") + def list_paths(self, ref: PRRef, *, sha: str) -> PathListing | None: + """Return every file path in the repository at commit ``sha``, best-effort. + + Walks the repository-level ``/files?at=`` listing with + ``start``/``limit`` paging, ``_PAGE_LIMIT`` entries a page. The + endpoint shape comes from the Bitbucket Server REST documentation + and was not probed live: a page is ``{"values": [...], "isLastPage", + "nextPageStart", ...}`` whose values are plain path strings, which + name files only. The non-empty strings are kept, sorted and + deduplicated. The walk ends when ``isLastPage`` is true (or absent) + or when ``nextPageStart`` is missing, as the activity walk does; a + page that says it is not the last but names no next start ends the + walk with ``complete=False``. It reads at most ``MAX_LISTING_PAGES`` + pages; when the last page it reads is not the last page, the paths + read so far come back with ``complete=False``. A failure on the first + page (a transport failure, a non-2xx status, a body that is not + JSON, or one with no ``values`` list) gives ``None``; the same + failure on a later page gives the paths read so far with + ``complete=False``. An empty ``sha`` gives ``None`` with no request. + ``ref.owner`` already carries the ``~slug`` form for a personal + repository. Never raises. + """ + if not sha: + return None + headers, auth = self._get_auth() + url = self._repo_url(ref, "/files") + where = f"{ref.owner}/{ref.repo}@{sha}" + paths: set[str] = set() + start = 0 + for page_number in range(1, MAX_LISTING_PAGES + 1): + try: + resp = self._session.get( + url, + params={"at": sha, "start": start, "limit": _PAGE_LIMIT}, + headers=headers, + auth=auth, + timeout=_REQUEST_TIMEOUT, + ) + except requests.RequestException as e: + return self._listing_stopped(paths, page_number, where, f"a transport failure ({e})") + if not resp.ok: + return self._listing_stopped(paths, page_number, where, f"HTTP {resp.status_code}") + try: + page = resp.json() + except ValueError as e: + return self._listing_stopped(paths, page_number, where, f"a non-JSON body ({e})") + values = page.get("values") if isinstance(page, dict) else None + if not isinstance(values, list): + return self._listing_stopped( + paths, page_number, where, f"a {type(page).__name__} body with no values list" + ) + paths.update(value for value in values if isinstance(value, str) and value) + if page.get("isLastPage", True): + return PathListing(paths=tuple(sorted(paths)), complete=True) + next_start = page.get("nextPageStart") + if next_start is None: + logger.debug( + "list_paths got a page that is not the last but has no " + "nextPageStart at page %d for %s; keeping the %d paths read so far", + page_number, where, len(paths), + ) + return PathListing(paths=tuple(sorted(paths)), complete=False) + start = next_start + logger.debug( + "list_paths stopped at the %d-page cap for %s with %d paths", + MAX_LISTING_PAGES, where, len(paths), + ) + return PathListing(paths=tuple(sorted(paths)), complete=False) + + @staticmethod + def _listing_stopped( + paths: set[str], page_number: int, where: str, reason: str + ) -> PathListing | None: + """Log why a ``list_paths`` walk stopped early and return what it has. + + A failure on the first page means there is no listing at all, so the + result is ``None``. A failure on a later page keeps the paths already + read, marked ``complete=False``, because a partial listing still + helps the name search. + """ + if page_number == 1: + logger.debug("list_paths got %s for %s", reason, where) + return None + logger.debug( + "list_paths got %s at page %d for %s; keeping the %d paths read so far", + reason, page_number, where, len(paths), + ) + return PathListing(paths=tuple(sorted(paths)), complete=False) + def prune_inline_comments(self, ref: PRRef) -> int: """Delete prxref-attributed inline comments; returns the count removed. diff --git a/src/prxref/forges/github.py b/src/prxref/forges/github.py index 112fdd7..85c5e11 100644 --- a/src/prxref/forges/github.py +++ b/src/prxref/forges/github.py @@ -22,6 +22,7 @@ DescriptionVersion, FeedReadError, InlineComment, + PathListing, PRData, PRHistory, PRRef, @@ -697,6 +698,52 @@ def get_file_content(self, ref: PRRef, path: str, *, sha: str) -> str | None: return None return content.decode("utf-8", errors="replace") + def list_paths(self, ref: PRRef, *, sha: str) -> PathListing | None: + """Return every file path in the repository at commit ``sha``, best-effort. + + One request to the Git Trees API, ``git/trees/{sha}?recursive=1``, + with no paging. Only ``blob`` entries are kept, so directories + (``tree``) and submodules (``commit``) are dropped, and the paths are + sorted and deduplicated. ``complete`` is ``False`` when GitHub reports + the tree ``truncated``. An empty ``sha`` (no request is made), a + transport failure, a non-2xx status, a body that is not JSON, or one + with no ``tree`` list gives ``None``. Never raises. + """ + if not sha: + return None + url = ( + f"{self._api_base(ref)}/repos/{ref.owner}/{ref.repo}/git/trees/" + f"{quote(sha, safe='')}" + ) + try: + resp = self.session.get( + url, headers=self._headers(ref.host), params={"recursive": "1"}, + timeout=_REQUEST_TIMEOUT, + ) + except requests.RequestException as e: + logger.debug("list_paths failed for %s/%s@%s: %s", ref.owner, ref.repo, sha, e) + return None + if not resp.ok: + logger.debug( + "list_paths got HTTP %s for %s/%s@%s", resp.status_code, ref.owner, ref.repo, sha + ) + return None + try: + body = resp.json() + except ValueError as e: + logger.debug("list_paths got a non-JSON body for %s/%s@%s: %s", ref.owner, ref.repo, sha, e) + return None + tree = body.get("tree") if isinstance(body, dict) else None + if not isinstance(tree, list): + logger.debug("list_paths got no tree list for %s/%s@%s", ref.owner, ref.repo, sha) + return None + paths = { + entry["path"] for entry in tree + if isinstance(entry, dict) and entry.get("type") == "blob" + and isinstance(entry.get("path"), str) and entry["path"] + } + return PathListing(paths=tuple(sorted(paths)), complete=not bool(body.get("truncated"))) + def prune_inline_comments(self, ref: PRRef) -> int: """Delete prxref-attributed inline comments; returns the count removed. diff --git a/src/prxref/forges/gitlab.py b/src/prxref/forges/gitlab.py index 08e6620..81e02aa 100644 --- a/src/prxref/forges/gitlab.py +++ b/src/prxref/forges/gitlab.py @@ -13,9 +13,11 @@ from prxref.forges._diff_render import render_diff_entries as _render_diff_entries from prxref.forges.base import ( ATTRIBUTION_MARKER, + MAX_LISTING_PAGES, SUMMARY_MARKER, FeedReadError, InlineComment, + PathListing, PRData, PRRef, Thread, @@ -46,6 +48,12 @@ # it is still bounded: GitLab's validation errors run long enough to bury the # log line that carries them. _ERROR_DETAIL_CHARS = 400 +_TREE_BLOBS_QUERY = ( + "query($p: ID!, $ref: String!, $after: String) { project(fullPath: $p) { repository { " + "tree(ref: $ref, recursive: true) { blobs(first: 100, after: $after) { " + "pageInfo { hasNextPage endCursor } nodes { path type mode } } } } } }" +) +_TREE_BLOBS_PATH = ("data", "project", "repository", "tree", "blobs") def _response_detail(resp: requests.Response) -> str: @@ -568,6 +576,208 @@ def get_file_content(self, ref: PRRef, path: str, *, sha: str) -> str | None: return None return content.decode("utf-8", errors="replace") + def list_paths(self, ref: PRRef, *, sha: str) -> PathListing | None: + """Return every file path in the repository at commit ``sha``, best-effort. + + Two walks, GraphQL first. The REST ``repository/tree?recursive=true`` + listing returns every directory before any file, across the whole + recursive walk, so on a project with more entries than the page cap + holds, the capped REST walk loses files, and returns none at all once + the directories alone fill the cap. GraphQL's ``tree.blobs`` + connection lists files only, so its whole page budget goes to paths. + + The GraphQL walk POSTs ``_TREE_BLOBS_QUERY`` (a read: the text begins + with ``query``) to ``https://{host}/api/graphql``, the host the REST + API is on, with the variables ``p`` (the project's full path), ``ref`` + (``sha``) and ``after`` (the previous page's ``endCursor``, ``None`` + on the first page), the same auth header as every other request, and + the request timeout. It follows ``endCursor`` while ``hasNextPage`` is + true and reads at most ``MAX_LISTING_PAGES`` pages of 100 nodes. Node + paths that are non-empty strings are kept, sorted and deduplicated. A + page with ``hasNextPage`` false ends the walk with ``complete=True``. + When the cap stops the walk while ``hasNextPage`` is still true, or a + later page fails, the paths read so far come back with + ``complete=False``. + + The REST walk answers instead when the FIRST GraphQL page is unusable: + a transport failure, a non-2xx status, a body that is not JSON, a + non-empty ``errors`` array, a null ``data.project``, any other wrong + shape (``hasNextPage`` true with no ``endCursor`` included), or a first + page with no usable path and ``hasNextPage`` false. GraphQL answers a + sha it cannot resolve with exactly that empty page, which it does not + distinguish from an empty tree, so REST decides between them. The + fallback reason is logged at DEBUG. + + The REST walk requests pages 1, 2, 3 and so on of + ``repository/tree?recursive=true``, ``_PAGE_SIZE`` entries a page. + Only ``blob`` entries are kept, so directories (``tree``) and + submodules (``commit``) are dropped, and the paths are sorted and + deduplicated. The walk ends when a page's ``X-Next-Page`` header is + absent or empty, never on a short page. It reads at most + ``MAX_LISTING_PAGES`` pages; when the last page it reads still names + a next page, the paths read so far come back with ``complete=False``. + A failure on the first page (a transport failure, a non-2xx status, a + body that is not JSON, or one that is not a list) gives ``None``; the + same failure on a later page gives the paths read so far with + ``complete=False``. An empty ``sha`` gives ``None`` with no request. + Never raises. + """ + if not sha: + return None + where = f"{self._project_path(ref)}@{sha}" + listing = self._list_paths_graphql(ref, sha=sha, where=where) + if listing is not None: + return listing + return self._list_paths_rest(ref, sha=sha, where=where) + + def _list_paths_graphql(self, ref: PRRef, *, sha: str, where: str) -> PathListing | None: + """Walk GraphQL's ``tree.blobs`` connection; ``None`` hands the listing to REST.""" + headers = self._get_auth_headers() + url = f"https://{ref.host}/api/graphql" + project_path = self._project_path(ref) + paths: set[str] = set() + after: str | None = None + for page_number in range(1, MAX_LISTING_PAGES + 1): + variables = {"p": project_path, "ref": sha, "after": after} + try: + resp = self._session.post( + url, + json={"query": _TREE_BLOBS_QUERY, "variables": variables}, + headers=headers, + timeout=_REQUEST_TIMEOUT, + ) + except requests.RequestException as e: + return self._graphql_stopped(paths, page_number, where, f"a transport failure ({e})") + try: + page_paths, has_next, after = self._read_blob_page(resp, first_page=page_number == 1) + except ValueError as e: + return self._graphql_stopped(paths, page_number, where, str(e)) + paths.update(page_paths) + if not has_next: + return PathListing(paths=tuple(sorted(paths)), complete=True) + logger.debug( + "list_paths stopped at the %d-page cap of the GraphQL listing for %s with %d paths", + MAX_LISTING_PAGES, where, len(paths), + ) + return PathListing(paths=tuple(sorted(paths)), complete=False) + + @staticmethod + def _read_blob_page( + resp: requests.Response, *, first_page: bool + ) -> tuple[list[str], bool, str | None]: + """Parse one ``tree.blobs`` page into (paths, hasNextPage, endCursor). + + Raises ``ValueError`` naming what makes the page unusable. A first + page with no usable path and ``hasNextPage`` false is unusable too, + because GraphQL gives that same page for a sha it cannot resolve. + """ + if not resp.ok: + raise ValueError(f"HTTP {resp.status_code}") + try: + body = resp.json() + except ValueError as e: + raise ValueError(f"a non-JSON body ({e})") from e + if not isinstance(body, dict): + raise ValueError(f"a {type(body).__name__} body, not an object") + if body.get("errors"): + raise ValueError(f"GraphQL errors ({_response_detail(resp)})") + node = body + for key in _TREE_BLOBS_PATH: + child = node.get(key) + if not isinstance(child, dict): + raise ValueError(f"no {key} object (a {type(child).__name__})") + node = child + page_info = node.get("pageInfo") + nodes = node.get("nodes") + if not isinstance(page_info, dict) or not isinstance(nodes, list): + raise ValueError("a blobs connection without a pageInfo object and a nodes list") + has_next = page_info.get("hasNextPage") + cursor = page_info.get("endCursor") + if not isinstance(has_next, bool): + raise ValueError(f"a {type(has_next).__name__} hasNextPage, not a bool") + if has_next and not (isinstance(cursor, str) and cursor): + raise ValueError("hasNextPage true with no endCursor") + paths = [ + entry["path"] for entry in nodes + if isinstance(entry, dict) and isinstance(entry.get("path"), str) and entry["path"] + ] + if first_page and not paths and not has_next: + raise ValueError("an empty first page, which is also the answer for a sha GitLab cannot resolve") + return paths, has_next, cursor if has_next else None + + def _graphql_stopped( + self, paths: set[str], page_number: int, where: str, reason: str + ) -> PathListing | None: + """Log why the GraphQL walk stopped; ``None`` on the first page falls back to REST.""" + if page_number == 1: + logger.debug( + "list_paths falls back to the REST tree walk for %s: the GraphQL listing gave %s", + where, reason, + ) + return None + return self._listing_stopped(paths, page_number, where, f"{reason} from the GraphQL listing") + + def _list_paths_rest(self, ref: PRRef, *, sha: str, where: str) -> PathListing | None: + """Walk the REST ``repository/tree?recursive=true`` listing page by page.""" + headers = self._get_auth_headers() + url = f"{self._api_base(ref)}/repository/tree" + paths: set[str] = set() + for page_number in range(1, MAX_LISTING_PAGES + 1): + params: dict[str, int | str] = { + "recursive": "true", + "per_page": _PAGE_SIZE, + "page": page_number, + "ref": sha, + } + try: + resp = self._session.get( + url, headers=headers, params=params, timeout=_REQUEST_TIMEOUT + ) + except requests.RequestException as e: + return self._listing_stopped(paths, page_number, where, f"a transport failure ({e})") + if not resp.ok: + return self._listing_stopped(paths, page_number, where, f"HTTP {resp.status_code}") + try: + entries = resp.json() + except ValueError as e: + return self._listing_stopped(paths, page_number, where, f"a non-JSON body ({e})") + if not isinstance(entries, list): + return self._listing_stopped( + paths, page_number, where, f"a {type(entries).__name__} body, not a list" + ) + paths.update( + entry["path"] for entry in entries + if isinstance(entry, dict) and entry.get("type") == "blob" + and isinstance(entry.get("path"), str) and entry["path"] + ) + if not (resp.headers.get("X-Next-Page") or "").strip(): + return PathListing(paths=tuple(sorted(paths)), complete=True) + logger.debug( + "list_paths stopped at the %d-page cap for %s with %d paths", + MAX_LISTING_PAGES, where, len(paths), + ) + return PathListing(paths=tuple(sorted(paths)), complete=False) + + @staticmethod + def _listing_stopped( + paths: set[str], page_number: int, where: str, reason: str + ) -> PathListing | None: + """Log why a ``list_paths`` walk stopped early and return what it has. + + A failure on the first page means there is no listing at all, so the + result is ``None``. A failure on a later page keeps the paths already + read, marked ``complete=False``, because a partial listing still + helps the name search. + """ + if page_number == 1: + logger.debug("list_paths got %s for %s", reason, where) + return None + logger.debug( + "list_paths got %s at page %d for %s; keeping the %d paths read so far", + reason, page_number, where, len(paths), + ) + return PathListing(paths=tuple(sorted(paths)), complete=False) + def prune_inline_comments(self, ref: PRRef) -> int: """Delete prxref-attributed inline comments; returns the count removed. diff --git a/src/prxref/forges/replay.py b/src/prxref/forges/replay.py index 514706d..2bf54d2 100644 --- a/src/prxref/forges/replay.py +++ b/src/prxref/forges/replay.py @@ -7,8 +7,8 @@ ``--pr-url``): no network, no threads, no file reads. - :class:`ReplayForge` wraps a real forge and pins what the orchestrator sees: the diff of a commit range (``base_sha``/``head_sha``, through the inner - forge's optional ``get_compare_diff``) or a diff text, file reads at the - pinned head, optionally no existing threads, and the PR's title and + forge's optional ``get_compare_diff``) or a diff text, file reads and path + listings at the pinned head, optionally no existing threads, and the PR's title and description as they stood at a cutoff (issue #16). Both raise on every write method, as defence in depth: the CLI already forces @@ -29,7 +29,7 @@ from pathlib import Path from typing import Literal -from .base import Forge, InlineComment, PRData, PRHistory, PRRef, Thread, _require_aware +from .base import Forge, InlineComment, PathListing, PRData, PRHistory, PRRef, Thread, _require_aware NEVER_POSTS = "replay runs never write to a forge" @@ -288,6 +288,16 @@ def get_file_content(self, ref: PRRef, path: str, *, sha: str) -> str | None: except Exception: # noqa: BLE001 - the Protocol says this never raises return None + def list_paths(self, ref: PRRef, *, sha: str) -> PathListing | None: + """The inner forge's path listing at ``sha``; ``None`` when it has none or it raises.""" + lister = getattr(self._inner, "list_paths", None) + if lister is None: + return None + try: + return lister(ref, sha=sha) + except Exception: # noqa: BLE001 - the Protocol says this never raises + return None + def post_summary(self, ref: PRRef, body: str) -> None: """Always raises ``RuntimeError``: a replay never writes to a forge.""" raise RuntimeError(NEVER_POSTS) diff --git a/src/prxref/forges/repo_dir.py b/src/prxref/forges/repo_dir.py new file mode 100644 index 0000000..3f6989a --- /dev/null +++ b/src/prxref/forges/repo_dir.py @@ -0,0 +1,121 @@ +"""Local filesystem reader that stands in for a PR head via ``--repo-dir``. + +``RepoDir`` gives a review or an eval case repository context read from a +local working tree, with no network access. ``read`` matches the shape of +the forge adapters' ``get_file_content`` contract (see +``forges/github.py``); ``list_files`` gives a bounded, confined directory +listing. The CLI flag and its wiring into the review pipeline live in +``prxref.cli``; this module only reads a confined filesystem tree. +""" + +from __future__ import annotations + +import os +import stat +from pathlib import PurePosixPath + +_MAX_FILE_CONTENT_BYTES = 512 * 1024 +_MAX_LISTED_FILES = 100_000 + + +class RepoDir: + """Reads a confined local directory tree in place of a forge working copy.""" + + def __init__(self, root: str | os.PathLike[str]) -> None: + """Resolve ``root`` with ``os.path.realpath``. + + Raises ``ValueError`` naming ``root`` when it is not an existing + directory. + """ + resolved = os.path.realpath(root) + if not os.path.isdir(resolved): + raise ValueError(f"repo-dir root is not an existing directory: {root}") + self.root = resolved + + def _confined_real_path(self, path: str) -> str | None: + """Return the confined, symlink-resolved filesystem path for ``path``. + + Returns None for an unsafe ``path`` (absolute, a ``..`` segment, a + backslash, a NUL byte, or empty), for one whose resolved real path + leaves the root, and for one that lands inside ``.git`` at any + depth. The confinement check compares resolved real paths, never + string prefixes. + """ + if not path or "\x00" in path or "\\" in path or os.path.isabs(path): + return None + if ".." in PurePosixPath(path).parts: + return None + candidate = os.path.join(self.root, path) + real = os.path.realpath(candidate) + root_prefix = self.root + os.sep + if real != self.root and not real.startswith(root_prefix): + return None + rel_parts = PurePosixPath(os.path.relpath(real, self.root)).parts + if ".git" in rel_parts: + return None + return real + + def read(self, path: str) -> str | None: + """Return the utf-8 text of ``path`` under the root, or None. + + Matches the forge adapters' ``get_file_content`` contract: never + raises, and returns None when the path is unsafe, missing, a + directory, escapes the root (directly or via a symlink), falls + inside ``.git``, is over 512 KiB, or holds a NUL byte. The size is + checked from a stat before the file is read. Otherwise the content + is decoded as utf-8 with ``errors="replace"``. + """ + real = self._confined_real_path(path) + if real is None: + return None + try: + st = os.stat(real) + except OSError: + return None + if not stat.S_ISREG(st.st_mode): + return None + if st.st_size > _MAX_FILE_CONTENT_BYTES: + return None + try: + with open(real, "rb") as fh: + content = fh.read() + except OSError: + return None + if b"\x00" in content: + return None + return content.decode("utf-8", errors="replace") + + def list_files(self) -> tuple[tuple[str, ...], bool]: + """Return sorted repo-relative POSIX file paths, and whether the walk was complete. + + The walk never follows a symlinked directory, skips any ``.git`` + directory at any depth, and omits symlinks whose target escapes the + root. It stops past ``_MAX_LISTED_FILES`` files and reports + ``complete=False`` only when a file beyond the cap was actually + dropped; a tree with exactly the cap's worth of files is complete. + """ + found: list[str] = [] + root_prefix = self.root + os.sep + complete = True + for dirpath, dirnames, filenames in os.walk(self.root, followlinks=False): + dirnames[:] = sorted(d for d in dirnames if d != ".git") + for name in sorted(filenames): + full = os.path.join(dirpath, name) + if os.path.islink(full): + real = os.path.realpath(full) + if real != self.root and not real.startswith(root_prefix): + continue + if not os.path.isfile(full): + continue + rel = PurePosixPath(os.path.relpath(full, self.root)) + if ".git" in rel.parts: + continue + if len(found) < _MAX_LISTED_FILES: + found.append(rel.as_posix()) + else: + complete = False + break + if not complete: + break + found.sort() + return tuple(found), complete diff --git a/src/prxref/formatter.py b/src/prxref/formatter.py index 6b071a4..94b123c 100644 --- a/src/prxref/formatter.py +++ b/src/prxref/formatter.py @@ -15,7 +15,7 @@ try: from .reviewer import load_prompt as _reviewer_load_prompt -except ImportError: # reviewer seat not landed yet; inline default applies +except ImportError: # reviewer's load_prompt unavailable; inline default applies _reviewer_load_prompt = None diff --git a/src/prxref/orchestrator.py b/src/prxref/orchestrator.py index 2584825..3715562 100644 --- a/src/prxref/orchestrator.py +++ b/src/prxref/orchestrator.py @@ -125,7 +125,7 @@ keys that :func:`_run_record` stamps on every exit (``cost_usd``, ``cost_estimated``, ``review_rules``, ``ticket_context``, ``spec_grounding``, ``size_advisory``, ``prompt_templates``, - ``scoped_rules``, ``rule_counts``; ``replay`` on replays only, and + ``scoped_rules``, ``rule_counts``, ``repo_context``; ``replay`` on replays only, and ``cost_api_equivalent`` on claude-cli-priced runs only). 7. Verdict: ``"Error"`` when every CHUNK review failed (a sweep success on a dead worker pool cannot carry the run); ``"Request-Changes"`` @@ -166,13 +166,13 @@ import threading import time from collections import Counter -from collections.abc import Mapping, Sequence +from collections.abc import Callable, Mapping, Sequence from concurrent.futures import ThreadPoolExecutor -from dataclasses import replace +from dataclasses import dataclass, replace from typing import Any from urllib.parse import urlparse -from . import chunk_context, costs, heuristics, reviewer, specs, systemic +from . import chunk_context, costs, heuristics, repo_contracts, repo_reader, repo_unit, reviewer, specs, systemic from .forges.base import ( ATTRIBUTION_MARKER, Forge, @@ -181,6 +181,7 @@ PRRef, Thread, ) +from .forges.repo_dir import RepoDir from .llm import LLMClient from .markers import OUT_OF_TICKET_MARKER, SEVERITY_MARKERS, inline_header, marker_for from .prompt_templates import CONTEXT_MARKER, REVIEW_TEMPLATES, PromptTemplates, packaged_text, placeholders @@ -209,6 +210,7 @@ prompt_example_titles, rule_cap_counts, ) +from .repo_context import exclude_predicate from .reviewer import NO_PROMPT_CONTEXT, PromptContext, fill_template from .trace import Tracer, get_tracer from .triage import ( @@ -431,6 +433,11 @@ def orchestrate_review( max_warning_findings: int | None = None, max_outofscope_findings: int | None = None, max_findings_per_rule: int = 2, + repo_context: str = "off", + repo_context_max_chars: int = 12000, + context_contract_globs: Sequence[str] = (), + context_exclude_globs: Sequence[str] = (), + repo_dir: RepoDir | None = None, ) -> dict: """Run one full review pass over a PR and optionally post results. @@ -438,9 +445,9 @@ def orchestrate_review( chunks_reviewed, chunks_failed, elapsed_ms, input_tokens, output_tokens, posted, sampling, cost_usd, cost_estimated, review_rules, ticket_context, spec_grounding, size_advisory, prompt_templates, scoped_rules, - rule_counts}``, plus ``replay`` on a replay run only. + rule_counts, repo_context}``, plus ``replay`` on a replay run only. Every exit, error and empty-diff exits included, goes through - :func:`_run_record`, so the last nine keys are always present and are + :func:`_run_record`, so the last ten keys are always present and are ``None`` (``cost_usd``: ``0.0`` before any LLM request; ``cost_estimated``: ``False``) when their feature is off or the run never reached it. ``cost_usd`` is ``None`` when the cost is unknown, never ``0``. @@ -454,6 +461,8 @@ def orchestrate_review( chunking, or LLM — the run degrades to verdict ``"Error"`` with a posted notice when ``post`` is true. Degenerate arguments are part of that: a caller passing ``max_chunks=0`` gets an error run, not a ``ValueError``. + The one exception is an unknown ``repo_context`` level, which raises + ``ValueError`` before any forge call (see below). ``chunk_count`` counts the review units: ``len(chunks)`` plus one for the systemic sweep, which runs whenever at least one chunk exists (an @@ -655,7 +664,69 @@ def orchestrate_review( unlimited and reads no environment variable; ``0`` drops every finding of that severity. ``outofscope`` is the minor severity, not the ticket scope ``out``. + + ``repo_context`` is the repository-context level + (``PRXREF_REPO_CONTEXT``): ``"off"`` (the default), ``"diff"`` or + ``"repo"`` (:data:`prxref.repo_unit.MODES`). Any other value raises + ``ValueError`` before any forge call; config rejects it long before, so + this guards a library caller only. ``repo_context_max_chars`` + (``PRXREF_REPO_CONTEXT_MAX_CHARS``) is each chunk's budget for it, + ``context_contract_globs`` (``PRXREF_CONTEXT_CONTRACT_GLOBS``) selects + the contract files, where ``()`` means none (the built-in set is + config's default, which the CLI passes), and ``context_exclude_globs`` + (``PRXREF_CONTEXT_EXCLUDE_GLOBS``) adds to the exclude floor of + :func:`prxref.repo_context.exclude_predicate`: an excluded path is + never read, listed or shown. ``repo_dir`` is a + :class:`prxref.forges.repo_dir.RepoDir` to read the repository from in + place of the forge; this function does not validate it (``RepoDir`` + does, when it is built). Off, nothing new is built or called: the + prompts, posts, trace and logs are exactly a run without these + arguments, and the record's ``repo_context`` key is ``None``. + + On, the run has ONE reader (:class:`prxref.repo_reader.RepoReader`): + over ``repo_dir`` when given, else over the forge's optional + ``get_file_content`` and ``list_paths`` at ``pr.source_sha``, and none + when the forge cannot read or the PR has no head sha. At ``"repo"`` with + a reader, the path listing is taken once and the contract files are + selected once, before the chunk workers run. ``"repo"`` with no reader + (only diff-only entries from hunk lines are left), or with a reader but + no listing (no name search and no glob-matched contract files), logs one + WARNING naming ``PRXREF_REPO_CONTEXT``; ``"diff"`` with no reader + builds its entries from hunk lines and logs nothing. Each chunk's worker + builds its context once, before its first attempt, with + :func:`prxref.repo_unit.build_unit_context`. It reads a PR diff file + through the reader's shared, uncapped ``read`` and every other path + through a fresh ``chunk_reader()``, so the per-chunk read cap is spent + on the paths outside the diff alone, and the entries do not depend on + which chunk reads a shared diff file first. The context's definition + lines extend the definitions block and its contract lines form a + ``### Contract excerpts`` block, on the first attempt only: the timeout + retry carries neither. A build that raises gives that chunk no context + and one WARNING naming the chunk; the review goes on. The dependency and + same-file definition blocks keep their own reader in every mode, so a + diff file can be fetched once by each reader. + + The ``repo_context`` key of every exit, when on, is ``{"mode", + "max_chars", "contract_globs", "exclude_globs", "reader", "listing", + "reads", "read_cap_hit", "units"}``. Until the chunk workers finish, + ``reader`` and ``listing`` are ``None``, ``reads`` is 0, + ``read_cap_hit`` is false and ``units`` is ``None``. After them, + ``reader`` is the reader's ``kind`` (``"forge"`` or ``"repo-dir"``) or + ``None``; ``listing`` (``{"paths", "complete"}`` or ``None``), + ``reads`` and ``read_cap_hit`` come from one + :meth:`~prxref.repo_reader.RepoReader.stats` snapshot, so ``reads`` + counts repository-context fetches only; and ``units`` is ``{"chunks": + [{"entries", "omitted", "retry_dropped"}, ...]}``, one row per chunk in + chunk order, where ``retry_dropped`` is true when the timeout retry ran + (and so ran without the chunk's repository context). The sweep has no + row. One ``chunk context`` trace event per chunk (``index``, ``total``, + ``entries``, ``omitted``, ``chars``) and one ``repo_context ok`` event + per run carry the same figures, only when on. """ + if repo_context not in repo_unit.MODES: + raise ValueError( + f"repo_context must be one of {repo_unit.MODES}, got {repo_context!r}" + ) t0 = time.perf_counter() tracer = get_tracer(trace_file) sampling = _sampling(llm) @@ -675,6 +746,7 @@ def orchestrate_review( "prompt_templates": None, "scoped_rules": None, "rule_counts": None, + "repo_context": None, } # Resolved once, before the first exit, so every exit records them and the # empty-diff summary gets the ticket note. An inactive (empty) ticket is @@ -689,6 +761,18 @@ def orchestrate_review( if scoped_rules is not None: scoped_meta = {**scoped_rules.record(), "max_chars": scoped_rules_max_chars} run_inputs["scoped_rules"] = {**scoped_meta, "units": None} + if repo_context != "off": + run_inputs["repo_context"] = { + "mode": repo_context, + "max_chars": repo_context_max_chars, + "contract_globs": list(context_contract_globs), + "exclude_globs": list(context_exclude_globs), + "reader": None, + "listing": None, + "reads": 0, + "read_cap_hit": False, + "units": None, + } summary_template = prompts.override("summary") if prompts is not None else "" ticket_active = ticket is not None and bool(ticket.active) ticket_note = ticket.note() if ticket is not None else "" @@ -961,12 +1045,30 @@ def orchestrate_review( prompts, feature="finding grouping" if group_findings else "the per-rule cap", ) reader = _make_file_reader(forge, ref, pr) + repo_plan: _RepoPlan | None = None + unit_records: list[dict[str, Any] | None] | None = None + if repo_context != "off": + repo_plan = _plan_repo_context( + repo_context, forge, ref, pr, files, run_inputs["repo_context"], repo_dir=repo_dir, + ) + unit_records = [None] * len(chunks) results = _run_workers( llm, chunks, pr, max_tokens=max_tokens, max_workers=max_workers, context_lines=context_lines, tracer=tracer, reader=reader, all_files=files, trace_dir=trace_dir, prompt_context=prompt_context, scoped_blocks=scoped_blocks, + repo_plan=repo_plan, unit_records=unit_records, ) + if repo_plan is not None and unit_records is not None: + run_inputs["repo_context"] = _repo_context_record( + run_inputs["repo_context"], repo_plan, unit_records, + ) + record = run_inputs["repo_context"] + tracer.event( + "repo_context", "ok", mode=record["mode"], reader=record["reader"], + listing=record["listing"], reads=record["reads"], + read_cap_hit=record["read_cap_hit"], + ) # One more worker-style unit, not inside the pool: the sweep digests the # WHOLE diff, so it only has something to say once every chunk result — @@ -1773,23 +1875,191 @@ def read(path: str) -> str | None: return read -def _context_blocks(chunk, reader, *, include_definitions: bool) -> str: - """Render the chunk's dependency and definition blocks; never raises.""" - if reader is None: +def _context_blocks( + chunk, reader, *, include_definitions: bool, unit: repo_unit.UnitContext | None = None, +) -> str: + """Render the chunk's dependency, definition and contract blocks; never raises. + + ``unit`` is the chunk's repository context. Its definition lines follow + the same-file definitions under one header and its contract lines form + the contracts block; they render with no ``reader`` too, over empty + dependency and same-file lists. ``None``, or a unit with no lines, is + exactly the rendering without repository context. + """ + extra = unit.definition_lines if unit is not None else () + contracts = unit.contract_lines if unit is not None else () + if reader is None and not (extra or contracts): return "" + deps: list[str] = [] + defs: list[str] = [] + if reader is not None: + try: + files = chunk_context.chunk_files(chunk) + deps = chunk_context.dependency_versions(files, reader) + defs = ( + chunk_context.referenced_definitions(files, reader) + if include_definitions else [] + ) + except Exception as e: # noqa: BLE001 + logger.debug("chunk context unavailable: %s", e) + if not (extra or contracts): + return "" + deps, defs = [], [] try: - files = chunk_context.chunk_files(chunk) - deps = chunk_context.dependency_versions(files, reader) - defs = ( - chunk_context.referenced_definitions(files, reader) - if include_definitions else [] + return chunk_context.render_context_blocks( + deps, defs, extra_def_lines=extra, contract_lines=contracts, ) - return chunk_context.render_context_blocks(deps, defs) except Exception as e: # noqa: BLE001 logger.debug("chunk context unavailable: %s", e) return "" +@dataclass(frozen=True) +class _RepoPlan: + """One run's repository-context inputs, fixed before the chunk workers start. + + ``reader`` is the run's one :class:`prxref.repo_reader.RepoReader`, or + ``None``. ``diff_paths`` holds every PR diff file's path: a read of one + goes to the shared, uncapped ``reader.read``. The listing and contract + fields are the ``"repo"`` level's once-per-run inputs, and stay empty at + ``"diff"`` or without a reader. + """ + + mode: str + reader: repo_reader.RepoReader | None + max_chars: int + exclude: Callable[[str], bool] + diff_paths: frozenset[str] + listing_paths: frozenset[str] | None = None + listing_complete: bool = False + contract_paths: tuple[str, ...] = () + contract_priority: tuple[str, ...] = () + + +def _plan_repo_context( + mode: str, forge: Forge, ref: PRRef, pr: PRData, files: Sequence[Any], + initial: Mapping[str, Any], *, repo_dir: RepoDir | None, +) -> _RepoPlan: + """Build the run's reader and its once-per-run inputs, for a level other than ``"off"``. + + ``initial`` is the run's initial ``repo_context`` record, whose + ``max_chars`` and glob lists are the inputs. The reader reads + ``repo_dir`` when it is given, else the forge at the PR's head sha. At + ``"repo"`` with a reader, the listing is taken here, once, and the + contract files are selected here, once. At ``"repo"``, a missing reader + or a missing listing logs the run's one WARNING naming + ``PRXREF_REPO_CONTEXT``. + """ + exclude = exclude_predicate(initial["exclude_globs"]) + if repo_dir is not None: + reader = repo_reader.repo_dir_reader(repo_dir, exclude=exclude) + else: + reader = repo_reader.forge_reader( + forge, ref, getattr(pr, "source_sha", "") or "", exclude=exclude, + ) + diff_paths = frozenset(f.path for f in files) + if mode != "repo": + return _RepoPlan(mode, reader, initial["max_chars"], exclude, diff_paths) + if reader is None: + logger.warning( + "PRXREF_REPO_CONTEXT=repo, but there is no repository reader (the forge cannot " + "read files at the PR head and no repository directory was given); repository " + "context is limited to diff-only entries from hunk lines", + ) + return _RepoPlan(mode, None, initial["max_chars"], exclude, diff_paths) + listing = reader.listing() + if listing is None: + logger.warning( + "PRXREF_REPO_CONTEXT=repo, but the repository path listing is unavailable; " + "repository context runs with no name search and no glob-matched contract files " + "outside the PR's own files", + ) + globs = list(initial["contract_globs"]) + return _RepoPlan( + mode, reader, initial["max_chars"], exclude, diff_paths, + listing_paths=frozenset(listing.paths) if listing is not None else None, + listing_complete=listing.complete if listing is not None else False, + contract_paths=tuple(repo_contracts.select_contract_files( + globs, listing=listing.paths if listing is not None else None, + diff_paths=[f.path for f in files if f.status != "removed"], + )), + contract_priority=tuple(repo_contracts.literal_contract_paths(globs)), + ) + + +def _routed_read(reader: repo_reader.RepoReader, diff_paths: frozenset[str]) -> Callable[[str], str | None]: + """One chunk's ``read``: a PR diff file through the shared ``reader.read``, any other path capped. + + The capped half is a fresh :meth:`~prxref.repo_reader.RepoReader.chunk_reader`, + so the per-chunk cap is spent on paths outside the diff alone. Both + halves refuse an excluded path. + """ + capped = reader.chunk_reader() + shared = reader.read + + def read(path: str) -> str | None: + return shared(path) if path in diff_paths else capped(path) + + return read + + +def _chunk_unit( + plan: _RepoPlan, chunk, all_files, *, index: int, total: int, +) -> repo_unit.UnitContext: + """Build one chunk's repository context in its worker; never raises. + + A build that raises gives :data:`prxref.repo_unit.EMPTY_UNIT` and one + WARNING naming the chunk. + """ + read = _routed_read(plan.reader, plan.diff_paths) if plan.reader is not None else None + try: + return repo_unit.build_unit_context( + chunk, all_files if all_files is not None else chunk, + mode=plan.mode, read=read, max_chars=plan.max_chars, + listing_paths=plan.listing_paths, listing_complete=plan.listing_complete, + contract_paths=plan.contract_paths, contract_priority=plan.contract_priority, + exclude=plan.exclude, + ) + except Exception as e: # noqa: BLE001 - context is never worth a failed review + logger.warning( + "[chunk %d/%d] repository context failed (continuing without it): %s", + index, total, e, + ) + return repo_unit.EMPTY_UNIT + + +def _unit_row(unit: repo_unit.UnitContext, *, retry_dropped: bool = False) -> dict[str, Any]: + """One chunk's ``repo_context`` units row: the unit's record plus ``retry_dropped``.""" + return {**unit.record(), "retry_dropped": retry_dropped} + + +def _repo_context_record( + initial: Mapping[str, Any], plan: _RepoPlan, unit_records: Sequence[dict[str, Any] | None], +) -> dict[str, Any]: + """The ``repo_context`` record once the chunk workers are done. + + ``reader`` is the reader's ``kind`` or ``None``; ``listing``, ``reads`` + and ``read_cap_hit`` come from one ``stats()`` snapshot (``None``, 0 + and false without a reader); ``units`` lists the rows in chunk order, + where a chunk whose worker left no row gets the empty unit's. + """ + reader = plan.reader + stats = reader.stats() if reader is not None else {"reads": 0, "read_cap_hit": False, "listing": None} + return { + **initial, + "reader": reader.kind if reader is not None else None, + "listing": stats["listing"], + "reads": stats["reads"], + "read_cap_hit": stats["read_cap_hit"], + "units": { + "chunks": [ + row if row is not None else _unit_row(repo_unit.EMPTY_UNIT) + for row in unit_records + ], + }, + } + + def _scoped_unit_blocks( scoped_rules: Any, chunks, always_on, *, max_chars: int, ) -> tuple[list[Any], Any]: @@ -1856,6 +2126,8 @@ def _run_workers( trace_dir: str | None = None, prompt_context: PromptContext = NO_PROMPT_CONTEXT, scoped_blocks: Sequence[Any] | None = None, + repo_plan: _RepoPlan | None = None, + unit_records: list[dict[str, Any] | None] | None = None, ) -> list[dict]: # Never below 1: ThreadPoolExecutor rejects a zero-width pool, and a # library caller is not gated by config's range check. @@ -1893,6 +2165,7 @@ def _heartbeat() -> None: trace_label=f"chunk{i}", trace_dir=trace_dir, prompt_context=prompt_context, scoped_block=scoped_blocks[i] if scoped_blocks is not None else None, + repo_plan=repo_plan, unit_records=unit_records, ) for i, chunk in enumerate(chunks) ] @@ -1941,6 +2214,7 @@ def _invoke_chunk( reader=None, *, include_definitions: bool = True, all_files=None, trace_label: str = "", trace_dir: str | None = None, prompt_context: PromptContext = NO_PROMPT_CONTEXT, + unit: repo_unit.UnitContext | None = None, ) -> dict: """One normalized :func:`reviewer.review_chunk` call; never raises. @@ -1960,14 +2234,16 @@ def _invoke_chunk( unchanged on both attempts: it is intent, not bulk context, and a dict-shaped finding keeps its ``scope`` only when :attr:`reviewer.PromptContext.scope_active`, and its ``rule`` only when - :attr:`reviewer.PromptContext.rule_active`. + :attr:`reviewer.PromptContext.rule_active`. ``unit`` is the chunk's + repository context (:func:`_context_blocks`); :func:`_run_worker` passes + it on the first attempt only, and ``None`` renders exactly as before. The shape carries the reviewer's reported ``cost_usd`` and ``cost_source`` beside the token counts; a call that raised, or a stub whose meta lacks them, gives ``None`` and ``""``. Pricing is left to :func:`_stamp_run_cost`, over the whole run. """ - blocks = _context_blocks(chunk, reader, include_definitions=include_definitions) + blocks = _context_blocks(chunk, reader, include_definitions=include_definitions, unit=unit) try: res = reviewer.review_chunk( llm, chunk, pr_title=pr.title, pr_description=pr.description, @@ -2025,6 +2301,8 @@ def _run_worker( trace_label: str = "", trace_dir: str | None = None, *, prompt_context: PromptContext = NO_PROMPT_CONTEXT, scoped_block: Any = None, + repo_plan: _RepoPlan | None = None, + unit_records: list[dict[str, Any] | None] | None = None, ) -> dict: tracer = tracer if tracer is not None else get_tracer() t0 = time.perf_counter() @@ -2046,9 +2324,20 @@ def _run_worker( files=[f.path for f in chunk], **({"rules": _scoped_rows(scoped_block)} if scoped_block is not None else {}), ) + unit: repo_unit.UnitContext | None = None + if repo_plan is not None: + unit = _chunk_unit(repo_plan, chunk, all_files, index=index, total=total) + if unit_records is not None: + unit_records[index - 1] = _unit_row(unit) + tracer.event( + "chunk", "context", index=index, total=total, + entries=len(unit.entries), omitted=unit.omitted, + chars=sum(len(entry.rendered()) for entry in unit.entries), + ) res = _invoke_chunk( llm, chunk, pr, max_tokens, context_lines, reader, all_files=all_files, trace_label=trace_label, trace_dir=trace_dir, prompt_context=unit_context, + unit=unit, ) if ( res["error"] @@ -2070,6 +2359,8 @@ def _run_worker( # The dependency block is a handful of tokens and survives; the # definitions block is the bulky one and is dropped, because shrinking # the prompt is the entire point of this retry. + if unit is not None and unit_records is not None: + unit_records[index - 1] = _unit_row(unit, retry_dropped=True) res = _invoke_chunk( llm, chunk, pr, max_tokens, _TIMEOUT_RETRY_CONTEXT_LINES, reader, include_definitions=False, all_files=all_files, diff --git a/src/prxref/repo_context.py b/src/prxref/repo_context.py new file mode 100644 index 0000000..909bb73 --- /dev/null +++ b/src/prxref/repo_context.py @@ -0,0 +1,265 @@ +"""Repository context outside the diff: shared types and the definitions core. + +``chunk_context`` answers what a referenced identifier IS only when its +definition sits in the SAME changed file, and it knows no Java. This module +holds the pieces the repository-context feature (``PRXREF_REPO_CONTEXT``) +builds on: the :class:`ContextEntry` record every context source emits, the +admission ranks in :data:`REASONS`, a language map and definition regexes that +add Java, the names an added line references, a definition scan over the +text of ANY file, and the exclude floor (:data:`EXCLUDE_FLOOR`, +:func:`exclude_predicate`) that keeps label files and secrets out of every read. + +The module is pure. It is stdlib plus :mod:`prxref.chunk_context` and +:func:`prxref.rules.match_globs`, performs no I/O and no network, and +callers hand it file text they already read. It +imports ``chunk_context``'s underscore helpers (``_language``, +``_definition_regexes``, ``_keywords``, ``_entry_text``, ``_IDENT_RE``) on +purpose, so both modules scan and render a definition the same way instead of +drifting apart. Java lives here rather than in +``chunk_context._definition_regexes`` because ``referenced_definitions`` runs +with the feature off, and feature-off output must stay byte-identical. +""" +from __future__ import annotations + +import re +from collections.abc import Callable, Iterable, Sequence +from dataclasses import dataclass + +from . import chunk_context +from .rules import match_globs + +REASONS = ("cross-chunk", "contract", "diff-file", "import", "path-convention", "name-search") +KINDS = ("definition", "contract") + +_JAVA_DEF_RE = re.compile( + r"^\s*(?:(?:@(?!interface\b)[A-Za-z_$][\w$.]*(?:\((?:[^()]|\([^()]*\))*\))?" + r"|public|protected|private|static|final|abstract|sealed|non-sealed|strictfp)\s+)*" + r"(?:class|interface|record|enum|@interface)\s+([A-Za-z_$][A-Za-z0-9_$]*)" +) + +_JAVA_IMPORT_RE = re.compile(r"^\s*import\s+(?:static\s+)?((?:java|javax)\.[\w$.]*[\w$])(?:\.\*)?\s*;") + +_JAVA_KEYWORDS = frozenset({ + "abstract", "assert", "boolean", "break", "byte", "case", "catch", "char", + "class", "const", "continue", "default", "do", "double", "else", "enum", + "exports", "extends", "false", "final", "finally", "float", "for", "goto", + "if", "implements", "import", "instanceof", "int", "interface", "long", + "module", "native", "new", "non", "null", "open", "opens", "package", + "permits", "private", "protected", "provides", "public", "record", + "requires", "return", "sealed", "short", "static", "strictfp", "super", + "switch", "synchronized", "this", "throw", "throws", "to", "transient", + "transitive", "true", "try", "uses", "var", "void", "volatile", "when", + "while", "with", "yield", +}) + +_JDK_NAMES = frozenset({ + "AbstractMap", "ArithmeticException", "ArrayDeque", "ArrayIndexOutOfBoundsException", + "ArrayList", "Arrays", "AssertionError", "AtomicBoolean", "AtomicInteger", + "AtomicLong", "AtomicReference", "AutoCloseable", "Base64", "BigDecimal", + "BigInteger", "BiConsumer", "BiFunction", "BinaryOperator", "BiPredicate", + "Boolean", "Byte", "Callable", "CharSequence", "Character", "Charset", + "ChronoUnit", "Class", "ClassCastException", "Clock", "Cloneable", + "Collection", "Collections", "Collectors", "Comparable", "Comparator", + "CompletableFuture", "CompletionStage", "ConcurrentHashMap", "ConcurrentMap", + "Consumer", "CountDownLatch", "Date", "DateTimeFormatter", "Deprecated", + "Deque", "Double", "Duration", "Enum", "Error", "Exception", "ExecutorService", + "Executors", "File", "Files", "Float", "Function", "FunctionalInterface", + "Future", "HashMap", "HashSet", "IllegalArgumentException", + "IllegalStateException", "IndexOutOfBoundsException", "InputStream", "Instant", + "IntStream", "Integer", "InterruptedException", "IOException", "Iterable", + "Iterator", "LinkedHashMap", "LinkedHashSet", "LinkedList", "List", + "LocalDate", "LocalDateTime", "LocalTime", "Locale", "Long", "LongStream", + "Map", "Matcher", "Math", "NoSuchElementException", "NullPointerException", + "Number", "NumberFormatException", "Object", "Objects", "OffsetDateTime", + "Optional", "OptionalDouble", "OptionalInt", "OptionalLong", "OutputStream", + "Override", "Path", "Paths", "Pattern", "Period", "Predicate", "PriorityQueue", + "Queue", "Random", "Reader", "Record", "ReentrantLock", "Runnable", "Runtime", + "RuntimeException", "SafeVarargs", "Set", "Short", "SortedMap", "SortedSet", + "StandardCharsets", "Stream", "String", "StringBuilder", "StringJoiner", + "Supplier", "SuppressWarnings", "System", "Thread", "ThreadLocal", "Throwable", + "TimeUnit", "TreeMap", "TreeSet", "UUID", "UnaryOperator", + "UncheckedIOException", "UnsupportedOperationException", "URI", "URL", "Void", + "Writer", "ZoneId", "ZoneOffset", "ZonedDateTime", +}) + + +@dataclass(frozen=True) +class ContextEntry: + """One repository-context entry admitted into a worker chunk's prompt. + + ``path`` is repository-relative, ``line`` the 1-based start line in that + file, ``symbol`` the type or definition name (or, for a contract, the + operationId, schema name, route template or table name). ``kind`` is a + member of :data:`KINDS`, ``reason`` a member of :data:`REASONS`, and + ``text`` the entry body with per-entry caps already applied and no + ``path:line:`` prefix. + """ + + path: str + line: int + symbol: str + kind: str + reason: str + text: str + + def rendered(self) -> str: + """The prompt line, ``path:line: text``, the shape ``referenced_definitions`` emits.""" + return f"{self.path}:{self.line}: {self.text}" + + def record(self) -> dict: + """The run-record row: path, line, symbol, kind, reason, and the rendered length as ``chars``.""" + return { + "path": self.path, + "line": self.line, + "symbol": self.symbol, + "kind": self.kind, + "reason": self.reason, + "chars": len(self.rendered()), + } + + +def language_of(path: str) -> str: + """The definition language of ``path``: ``chunk_context``'s map plus ``"java"`` for ``.java``. + + Returns ``""`` for a path no language claims. + """ + if path.lower().endswith(".java"): + return "java" + return chunk_context._language(path) + + +def definition_regexes(language: str) -> tuple[re.Pattern[str], ...]: + """The definition regexes for ``language``; group 1 of a match is the defined name. + + ``"java"`` gets one regex for a type declaration (``class``, ``interface``, + ``record``, ``enum`` or ``@interface``, after optional modifiers and + same-line annotations); it never matches a method, a field or a local + variable. Every other language delegates to ``chunk_context``, which covers + js and python and returns ``()`` otherwise. + """ + if language == "java": + return (_JAVA_DEF_RE,) + return chunk_context._definition_regexes(language) + + +def _jdk_imports(added: Iterable[str]) -> set[str]: + names: set[str] = set() + for text in added: + match = _JAVA_IMPORT_RE.match(text) + if match: + names.update(match.group(1).split(".")) + return names + + +def _type_like(name: str) -> bool: + return name[:1].isupper() and any(c.islower() for c in name) + + +def referenced_names(added: Iterable[str], language: str) -> list[str]: + """Identifiers the added lines reference, in first-appearance order, deduplicated. + + Language keywords are removed, and so is every name a + :func:`definition_regexes` match defines on the added lines themselves, as + ``chunk_context.referenced_definitions`` does. For Java, JDK names are also + removed (every segment of a ``java.*`` or ``javax.*`` import on the added + lines, plus a built-in set of common ``java.lang``, ``java.util``, + ``java.time`` and related types), and only type-like names are kept: an + uppercase first letter and at least one lowercase letter, since a Java + definition regex can only ever find a type. That drops constants, locals, + methods and single-letter type parameters. + """ + lines = list(added) + java = language == "java" + keywords = _JAVA_KEYWORDS if java else chunk_context._keywords(language) + defined: set[str] = set() + for text in lines: + for regex in definition_regexes(language): + match = regex.match(text) + if match: + defined.add(match.group(1)) + dropped = keywords | defined + if java: + dropped = dropped | _JDK_NAMES | _jdk_imports(lines) + out: dict[str, None] = {} + for text in lines: + for name in chunk_context._IDENT_RE.findall(text): + if name in dropped or name in out: + continue + if java and not _type_like(name): + continue + out[name] = None + return list(out) + + +def find_definitions( + text: str, + names: Iterable[str], + *, + language: str, + skip_lines: frozenset[int] = frozenset(), + max_lines: int = chunk_context.MAX_LINES_PER_DEFINITION, +) -> list[tuple[str, int, str]]: + """``(symbol, line, entry_text)`` for the first definition of each wanted name. + + Scans ``text`` line by line with :func:`definition_regexes`, the loop + ``chunk_context.referenced_definitions`` runs, generalized to any file: + 1-based lines in ``skip_lines`` are passed over, only the first regex that + matches a line is consulted, and each name yields at most its FIRST + definition. ``entry_text`` is the defining line plus continuation lines up + to a balanced bracket or ``max_lines``. Results are in line order. Returns + ``[]`` when ``text`` is empty or larger than ``chunk_context.MAX_FILE_BYTES`` + in UTF-8, when no name is wanted, or when the language has no regexes. + """ + regexes = definition_regexes(language) + wanted = set(names) + if not regexes or not wanted or not text: + return [] + if len(text.encode("utf-8", "ignore")) > chunk_context.MAX_FILE_BYTES: + return [] + lines = text.splitlines() + seen: set[str] = set() + out: list[tuple[str, int, str]] = [] + for idx, line in enumerate(lines): + number = idx + 1 + if number in skip_lines: + continue + for regex in regexes: + match = regex.match(line) + if not match: + continue + name = match.group(1) + if name in wanted and name not in seen: + seen.add(name) + out.append((name, number, chunk_context._entry_text(lines, idx, max_lines))) + break + return out + + +EXCLUDE_FLOOR = ( + "**/expected.json", + "**/cases.json", + "**/case.json", + "**/prxref-eval/**", + "**/.env*", + "**/*.pem", + "**/*.key", +) + + +def exclude_predicate(extra_globs: Sequence[str] = ()) -> Callable[[str], bool]: + """A ``path -> bool`` that is true for a path repository context must never read. + + A path is excluded when :func:`prxref.rules.match_globs` selects it with + :data:`EXCLUDE_FLOOR` (eval labels, eval output, dotenv files, keys), or + with ``extra_globs`` (``PRXREF_CONTEXT_EXCLUDE_GLOBS``) when that list is + non-empty. The two lists are matched separately, so the extra globs only + ever ADD exclusions: a ``!`` negation in ``extra_globs`` vetoes only the + extra list's own positives and can never re-admit a floor path, which it + would if both were one ``match_globs`` list. + """ + extra = tuple(extra_globs) + + def excluded(path: str) -> bool: + return match_globs(path, EXCLUDE_FLOOR) or bool(extra and match_globs(path, extra)) + + return excluded diff --git a/src/prxref/repo_contracts.py b/src/prxref/repo_contracts.py new file mode 100644 index 0000000..d14e24a --- /dev/null +++ b/src/prxref/repo_contracts.py @@ -0,0 +1,1172 @@ +"""Contract excerpts for repository context: the OpenAPI, JSON Schema, SQL and +Liquibase slices that a changed route, operation, schema or table points at. + +A diff that adds a route handler, a migration or a DTO often changes behavior +that a contract file elsewhere in the repository pins down: the OpenAPI +operation for the route, the schema a payload must satisfy, the unique index on +a table. The excerpters here cut the matching slice out of such a file so a +worker can check the change against it. + +The module is pure. It is stdlib plus the pure :mod:`prxref.repo_context`, +:mod:`prxref.chunk_context` and :func:`prxref.rules.match_globs`, and it +performs no I/O: callers pass the file's text, and :func:`contract_entries` +reads only through the ``read`` callable it is handed. There is deliberately +no YAML parser, because the core ships none, so YAML is sliced by indentation, +JSON goes through :mod:`json`, and SQL and XML are scanned with regular +expressions. Every excerpter degrades to ``[]`` on text it cannot read rather +than raising, because a review must never fail over missing context. Each +excerpt is capped at :data:`MAX_CONTRACT_LINES` lines and +:data:`MAX_CONTRACT_CHARS` characters. + +Names compare through :func:`normalize_name`, so a table called +``idempotency_keys`` matches a schema or class called ``IdempotencyKey``. +Routes compare through :func:`route_key`, so the code route +``/connectors/:id`` matches the spec path ``/connectors/{connectorId}``. + +The second half of the module turns a worker chunk into contract entries. +:func:`contract_triggers` reads a changed file's added lines for the routes, +tables, names and operation ids a contract can match. +:func:`select_contract_files` picks the repository's contract files from the +configured globs once per run, and :func:`literal_contract_paths` names the +globs that are plain paths. :func:`earlier_migrations` finds the migrations a +changed one builds on, :func:`contract_excerpts` sends one contract file to +the excerpter its format needs, and :func:`contract_entries` ties them +together for one chunk as :class:`prxref.repo_context.ContextEntry` records. +Every regular expression here runs in linear time on a 512 KiB line. +""" +from __future__ import annotations + +import json +import posixpath +import re +from collections.abc import Callable, Collection, Iterable, Iterator, Sequence +from dataclasses import dataclass + +from . import chunk_context +from .repo_context import _JAVA_KEYWORDS, ContextEntry, language_of, referenced_names +from .rules import match_globs + +MAX_CONTRACT_LINES = 40 +MAX_CONTRACT_CHARS = 2000 +MAX_EARLIER_MIGRATIONS = 4 +MAX_SPEC_FILES = 6 + +_BOM = chr(0xFEFF) +_HTTP_METHODS = frozenset({"get", "put", "post", "delete", "options", "head", "patch", "trace"}) +_TABLE_KEYS = frozenset({"tableName", "baseTableName", "referencedTableName"}) + +_BRACE_PARAM = re.compile(r"\{[^{}/]*\}") +_ANGLE_PARAM = re.compile(r"<[^<>/]*>") + +_YAML_KEY = re.compile( + r"""(?P\ *)(?P(?:-[ \t]+)*)""" + r"""(?P"(?:[^"\\]|\\.)*"[ \t]*|'(?:[^']|'')*'[ \t]*""" + r"""|(?:[^\s#"'\-\[\]{},?&*!|>%@`]|-(?=\S))(?:[^#:]|:(?![ \t]|$))*):(?:[ \t]|$)""" +) +_YAML_QUOTED = re.compile(r""""(?:[^"\\]|\\.)*"|'(?:[^']|'')*'""") +_YAML_BLOCK_SCALAR = re.compile(r"(?:^[ \t]*(?:-[ \t]+)+|:[ \t]+)[|>][-+1-9]{0,2}[ \t]*(?:#.*)?$") + +_SQL_TOKEN = re.compile( + r"(?P--[ \t]*changeset\b[^\n]*)" + r"|(?P--[^\n]*|/\*.*?(?:\*/|\Z))" + r"|(?P'(?:[^']|'')*'|\"(?:[^\"]|\"\")*\"|`[^`]*`)" + r"|(?P\$(?P[A-Za-z_]\w*|)\$.*?\$(?P=tag)\$)" + r"|(?P^[ \t]*go[ \t\r]*$)" + r"|(?P;)", + re.S | re.M | re.I, +) +_SQL_PART = r"""(?:"[^"]+"|`[^`]+`|\[[^\]]+\]|[\w$]+)""" +_SQL_NAME = rf"(?P{_SQL_PART}(?:\s*\.\s*{_SQL_PART})*)" +_SQL_HEADS = ( + re.compile( + r"create\s+(?:or\s+replace\s+)?(?:(?:global|local)\s+)?(?:(?:temp|temporary|unlogged)\s+)?" + rf"table\s+(?:if\s+not\s+exists\s+)?{_SQL_NAME}", + re.I, + ), + re.compile(rf"alter\s+table\s+(?:if\s+exists\s+)?(?:only\s+)?{_SQL_NAME}", re.I), + re.compile( + r"create\s+(?:unique\s+)?(?:(?:clustered|nonclustered|bitmap|fulltext|spatial)\s+)?" + rf"index\b[^;]*?\bon\s+(?:only\s+)?{_SQL_NAME}", + re.I, + ), +) +_SQL_NAME_PART = re.compile(_SQL_PART) +_NON_SPACE = re.compile(r"\S") + +_XML_COMMENT = re.compile(r"|\Z)", re.S) +_XML_CHANGESET = re.compile( + r"""<(?:[\w.-]+:)?changeSet\b(?:"[^"]*"|'[^']*'|[^'">/]|/(?!>))*(?:/>|>.*?)""", + re.S, +) +_XML_TABLE_ATTR = re.compile(r"""\b(?:tableName|baseTableName|referencedTableName)\s*=\s*(?:"([^"]*)"|'([^']*)')""") + + +@dataclass(frozen=True) +class Excerpt: + """One slice of a contract file. + + ``line`` is the 1-based line the slice starts on (best effort for JSON, and + 1 when unknown), ``symbol`` is what matched, written as the file writes it, + and ``text`` is the slice, already capped to :data:`MAX_CONTRACT_LINES` and + :data:`MAX_CONTRACT_CHARS`. + """ + + line: int + symbol: str + text: str + + +def normalize_name(name: str) -> str: + """Fold a type, schema or table name so its spellings compare equal. + + The name is lowercased, every ``_`` and ``-`` is dropped, and ONE trailing + ``s`` is stripped, so ``idempotency_keys``, ``idempotency-key`` and + ``IdempotencyKey`` all give ``idempotencykey``. Excerpters treat a name + that folds to the empty string as matching nothing. + """ + folded = name.lower().replace("_", "").replace("-", "") + return folded[:-1] if folded.endswith("s") else folded + + +def route_key(route: str) -> str: + """Normalize an HTTP route template so a code route and a spec path compare equal. + + Every parameter becomes ``{}``: an OpenAPI or Spring ``{id}`` or + ``{connectorId}``, a Flask ```` or ````, and an Express + segment that starts with ``:`` (``:id``, ``:id?``). A parameter inside a + segment is replaced in place (``/files/{name}.json`` gives + ``/files/{}.json``). Empty segments are dropped, which collapses duplicate + slashes and strips a trailing slash, and the result always starts with + ``/``. Case is kept. ``/`` gives ``/``; a blank route gives ``""``, which + excerpters treat as matching nothing. + """ + segments = [segment for segment in route.strip().split("/") if segment] + if not segments: + return "/" if route.strip() else "" + keyed = [ + "{}" if segment.startswith(":") else _ANGLE_PARAM.sub("{}", _BRACE_PARAM.sub("{}", segment)) + for segment in segments + ] + return "/" + "/".join(keyed) + + +def openapi_yaml_excerpts( + text: str, + *, + routes: Iterable[str] = (), + operation_ids: Iterable[str] = (), + schemas: Iterable[str] = (), +) -> list[Excerpt]: + """Slice the matching parts out of an OpenAPI or Swagger document written in YAML. + + There is no YAML parser: each match is an indentation slice, the key line + plus every following line indented deeper than it, with blank and comment + lines inside kept and trailing ones trimmed. The slice is dedented by the + key's indentation. Lines inside a ``|`` or ``>`` block scalar are text, + never keys. + + - A route matches a key directly under the top-level ``paths:`` when the + two :func:`route_key` values are equal, and yields that path item. + - An operation id matches an ``operationId: `` line, quoted or not, + whose enclosing key is an HTTP method (``get:``, ``post:`` and so on), + and yields that operation block. An ``operationId`` under ``links:`` is + a reference, not an operation, and is ignored. + - A schema name matches a key directly under ``components:`` then + ``schemas:`` (or under the top-level ``definitions:`` of Swagger 2.0) + when the :func:`normalize_name` values are equal, and yields that schema. + + ``line`` is the key line's 1-based number. ``symbol`` is the spec's own + path key, operation id or schema key, unquoted. Results come in line order + with no duplicates: an excerpt is dropped when another one already shows + every line of it, so a path item found by route absorbs an operation found + by id, unless the path item's caps cut that operation off. A text whose + first character is ``{`` is JSON, which is also YAML, and goes through + :func:`openapi_json_excerpts`. + """ + text = text.removeprefix(_BOM) + if text.lstrip().startswith("{"): + return openapi_json_excerpts(text, routes=routes, operation_ids=operation_ids, schemas=schemas) + route_keys = _keys(routes, route_key) + op_ids = _keys(operation_ids, str.strip) + schema_keys = _keys(schemas, normalize_name) + if not (route_keys or op_ids or schema_keys): + return [] + doc = _Yaml(text) + top = doc.top_level() + found: list[tuple[int, str]] = [] + if route_keys and "paths" in top: + found += [(i, doc.key[i]) for i in doc.children(top["paths"]) if route_key(doc.key[i]) in route_keys] + if op_ids: + for i, key in enumerate(doc.key): + if key != "operationId" or doc.value[i] not in op_ids: + continue + parent = doc.parent(i) + if parent is not None and doc.key[parent].lower() in _HTTP_METHODS: + found.append((parent, doc.value[i])) + if schema_keys: + containers: list[int] = [] + if "components" in top: + containers += [i for i in doc.children(top["components"]) if doc.key[i] == "schemas"] + if "definitions" in top: + containers.append(top["definitions"]) + for container in containers: + found += [(i, doc.key[i]) for i in doc.children(container) if normalize_name(doc.key[i]) in schema_keys] + return _line_excerpts(doc.spans(found)) + + +def openapi_json_excerpts( + text: str, + *, + routes: Iterable[str] = (), + operation_ids: Iterable[str] = (), + schemas: Iterable[str] = (), +) -> list[Excerpt]: + """Slice the matching parts out of an OpenAPI or Swagger document written in JSON. + + The semantics are :func:`openapi_yaml_excerpts`'s, through + :func:`json.loads`: a route matches a key of ``paths``, an operation id an + HTTP-method object whose ``operationId`` equals it, and a schema name a key + of ``components.schemas`` or of the Swagger 2.0 ``definitions``. + + ``text`` is the matched member rendered as ``"": `` followed by + ``json.dumps(node, indent=2)``, capped the same way. The key is kept + because a rendered context entry shows only ``path:line: text``, and a bare + ``{`` would not say which path, operation or schema it is. ``line`` is best + effort: the line of the matched key found by following its key path + through the source (``paths``, then the path, then the method), else the + line of the deepest ancestor found, else 1. Results come in line order + with no duplicates; here a container absorbs what it contains only when it + is shown whole. Invalid JSON, or a document that is not an object, gives + ``[]``. + """ + text = text.removeprefix(_BOM) + route_keys = _keys(routes, route_key) + op_ids = _keys(operation_ids, str.strip) + schema_keys = _keys(schemas, normalize_name) + if not (route_keys or op_ids or schema_keys): + return [] + doc = _load_json(text) + if not isinstance(doc, dict): + return [] + hits: list[_Hit] = [] + paths = doc.get("paths") + if route_keys and isinstance(paths, dict): + hits += [ + _json_hit(text, ("paths", key), key, key, item) + for key, item in paths.items() + if route_key(key) in route_keys + ] + if op_ids: + for pointer, node in _walk(doc): + key = pointer[-1] if pointer else None + if not (isinstance(key, str) and key.lower() in _HTTP_METHODS and isinstance(node, dict)): + continue + op_id = node.get("operationId") + if isinstance(op_id, str) and op_id in op_ids: + hits.append(_json_hit(text, pointer, op_id, key, node)) + if schema_keys: + containers: list[tuple[tuple[str, ...], dict]] = [] + components = doc.get("components") + if isinstance(components, dict) and isinstance(components.get("schemas"), dict): + containers.append((("components", "schemas"), components["schemas"])) + if isinstance(doc.get("definitions"), dict): + containers.append((("definitions",), doc["definitions"])) + for base, members in containers: + hits += [ + _json_hit(text, (*base, name), name, name, node) + for name, node in members.items() + if normalize_name(name) in schema_keys + ] + return _json_excerpts(hits) + + +def json_schema_excerpts(text: str, *, names: Iterable[str] = ()) -> list[Excerpt]: + """Slice the matching schemas out of a JSON Schema document. + + A name matches the document itself when it equals the root ``title`` by + :func:`normalize_name`, with whitespace in the title dropped first, so + ``transport_requests`` matches ``"title": "Transport Request"``; that + excerpt is the whole document at line 1. A name also matches any member + of a ``$defs`` or ``definitions`` object at any depth (not one that is a + property called ``definitions``), rendered and located as in + :func:`openapi_json_excerpts`. Results come in line order with no + duplicates; the root, when shown whole, absorbs its own definitions. + Invalid JSON, or a document that is not an object, gives ``[]``. + """ + text = text.removeprefix(_BOM) + keys = _keys(names, normalize_name) + if not keys: + return [] + doc = _load_json(text) + if not isinstance(doc, dict): + return [] + hits: list[_Hit] = [] + title = doc.get("title") + if isinstance(title, str) and normalize_name("".join(title.split())) in keys: + hits.append(_Hit((), title, None, doc, 1)) + for pointer, node in _walk(doc): + if not pointer or pointer[-1] not in ("$defs", "definitions") or not isinstance(node, dict): + continue + if len(pointer) > 1 and pointer[-2] in ("properties", "patternProperties"): + continue + hits += [ + _json_hit(text, (*pointer, name), name, name, member) + for name, member in node.items() + if normalize_name(name) in keys + ] + return _json_excerpts(hits) + + +def sql_excerpts(text: str, *, tables: Iterable[str] = ()) -> list[Excerpt]: + """Slice the statements that touch a table out of a SQL script or migration. + + Three statement kinds count, case-insensitively: ``CREATE TABLE`` + (``OR REPLACE``, ``TEMP``, ``UNLOGGED`` and ``IF NOT EXISTS`` allowed), + ``ALTER TABLE`` (``IF EXISTS`` and ``ONLY`` allowed), and + ``CREATE [UNIQUE] INDEX … ON ``. The table matches when its bare + name (the last dot-separated part, with ``"``, backticks and ``[]`` + removed) equals a requested table by :func:`normalize_name`. + + A statement runs from its keyword to its terminating ``;``. A ``;`` inside + a ``--`` or ``/* */`` comment, a quoted string or identifier, or a + ``$$`` body does not end it. A Liquibase formatted-SQL ``--changeset`` line + and a batch-separator line ``GO`` also end a statement, so a changeset's + last statement may omit its ``;``; trailing comments such as + ``--rollback`` stay outside it. Comment lines, including the + ``--liquibase formatted sql`` header, never start one. ``line`` is the + keyword's 1-based line and ``symbol`` the bare table name as written. + Results come in file order. + """ + text = text.removeprefix(_BOM) + keys = _keys(tables, normalize_name) + if not keys: + return [] + out: list[Excerpt] = [] + for start, end, name in _sql_statements(text): + bare = _bare_table(name) + if normalize_name(bare) in keys: + capped, _ = _cap(_slice_lines(text, start, end)) + out.append(Excerpt(text.count("\n", 0, start) + 1, bare, capped)) + return out + + +def liquibase_excerpts(text: str, *, tables: Iterable[str] = ()) -> list[Excerpt]: + """Slice the changeSets that touch a table out of a Liquibase changelog. + + The format follows the first non-blank character: ``<`` is XML, ``{`` or + ``[`` is JSON, anything else is YAML. A changeSet matches when it holds a + ``tableName``, ``baseTableName`` or ``referencedTableName`` (an XML + attribute, a YAML key or a JSON member) equal to a requested table by + :func:`normalize_name`. + + - XML yields the ```` element through ````; + changeSets inside ```` comments are ignored. + - YAML yields the indentation slice of the ``changeSet:`` line (usually + ``- changeSet:``), as :func:`openapi_yaml_excerpts` slices a key. + - JSON yields ``"changeSet": `` plus the member's ``json.dumps(indent=2)``. + + ``line`` is the changeSet's 1-based line (for JSON, the line of its own + ``"changeSet"`` key) and ``symbol`` the first matching table name as + written. Formatted-SQL changelogs go through :func:`sql_excerpts`. + Results come in file order; invalid JSON gives ``[]``. + """ + text = text.removeprefix(_BOM) + keys = _keys(tables, normalize_name) + if not keys: + return [] + head = text.lstrip()[:1] + if head == "<": + return _liquibase_xml(text, keys) + if head in ("{", "["): + return _liquibase_json(text, keys) + doc = _Yaml(text) + found: list[tuple[int, str]] = [] + for i, key in enumerate(doc.key): + if key != "changeSet": + continue + for j in range(i + 1, doc.block_end(i) + 1): + if doc.key[j] in _TABLE_KEYS and normalize_name(doc.value[j]) in keys: + found.append((i, doc.value[j])) + break + return _line_excerpts(doc.spans(found)) + + +def _keys(values: Iterable[str], fold: Callable[[str], str]) -> frozenset[str]: + return frozenset(key for key in map(fold, values) if key) + + +def _cap(lines: list[str]) -> tuple[str, int]: + """``(text, shown)``: the capped text and how many source lines it shows whole. + + A cut block keeps as many leading lines as fit and ends with an + ``… N more lines`` line, which counts toward both caps. A first line too + long to fit even alone is itself cut and ends with ``…``. + """ + text = "\n".join(lines) + if len(lines) <= MAX_CONTRACT_LINES and len(text) <= MAX_CONTRACT_CHARS: + return text, len(lines) + for keep in range(min(len(lines), MAX_CONTRACT_LINES) - 1, 0, -1): + capped = "\n".join([*lines[:keep], f"… {len(lines) - keep} more lines"]) + if len(capped) <= MAX_CONTRACT_CHARS: + return capped, keep + if len(lines) == 1: + return lines[0][: MAX_CONTRACT_CHARS - 1] + "…", 0 + marker = f"… {len(lines) - 1} more lines" + return f"{lines[0][: MAX_CONTRACT_CHARS - len(marker) - 2]}…\n{marker}", 0 + + +def _dedent(line: str, width: int) -> str: + return line[width:] if not line[:width].strip() else line.lstrip() + + +def _slice_lines(text: str, start: int, end: int) -> list[str]: + """The source between two offsets as lines, dedented by the start's column.""" + column = start - (text.rfind("\n", 0, start) + 1) + lines = [line.rstrip() for line in text[start:end].split("\n")] + return [lines[0], *(_dedent(line, column) for line in lines[1:])] + + +@dataclass(frozen=True) +class _Span: + start: int + end: int + shown: int + symbol: str + text: str + + +def _covers(outer: _Span, inner: _Span) -> bool: + """True when ``outer`` already shows every line ``inner`` shows.""" + if (outer.start, outer.end) == (inner.start, inner.end): + return True + return ( + outer.start <= inner.start + and inner.end <= outer.end + and inner.start + max(inner.shown, 1) <= outer.start + outer.shown + ) + + +def _line_excerpts(spans: list[_Span]) -> list[Excerpt]: + kept: list[_Span] = [] + for span in sorted(spans, key=lambda s: (s.start, -s.end)): + if not any(_covers(other, span) for other in kept): + kept.append(span) + return [Excerpt(span.start + 1, span.symbol, span.text) for span in kept] + + +def _unquote(token: str) -> str: + if len(token) >= 2 and token[0] == token[-1] == '"': + try: + value = json.loads(token) + except ValueError: + return token[1:-1] + return value if isinstance(value, str) else token[1:-1] + if len(token) >= 2 and token[0] == token[-1] == "'": + return token[1:-1].replace("''", "'") + return token + + +def _yaml_scalar(rest: str) -> str: + rest = rest.strip() + quoted = _YAML_QUOTED.match(rest) + if quoted: + return _unquote(quoted.group()) + return rest.split(" #", 1)[0].rstrip() + + +class _Yaml: + """Per-line indentation facts about a YAML text, enough for indentation slices.""" + + def __init__(self, text: str) -> None: + self.lines = [line.rstrip() for line in text.split("\n")] + self.indent: list[int] = [] + self.key: list[str | None] = [] + self.value: list[str] = [] + self.dash: list[bool] = [] + self.filler: list[bool] = [] + self.scalar: list[bool] = [] + scalar_floor: int | None = None + for line in self.lines: + stripped = line.lstrip(" ") + indent = len(line) - len(stripped) + in_scalar = scalar_floor is not None and (not stripped or indent > scalar_floor) + if not in_scalar: + scalar_floor = None + filler = not stripped or (not in_scalar and stripped.startswith("#")) + match = None if in_scalar or filler else _YAML_KEY.match(line) + self.indent.append(indent) + self.key.append(_unquote(match.group("key").rstrip()) if match else None) + self.value.append(_yaml_scalar(line[match.end():]) if match else "") + self.dash.append(bool(match and match.group("dash"))) + self.filler.append(filler) + self.scalar.append(in_scalar) + if not in_scalar and not filler and _YAML_BLOCK_SCALAR.search(line): + scalar_floor = indent + + def _content(self, index: int) -> bool: + return not self.filler[index] and not self.scalar[index] + + def top_level(self) -> dict[str, int]: + """Each column-0 mapping key's first line.""" + top: dict[str, int] = {} + for i, key in enumerate(self.key): + if key is not None and self.indent[i] == 0 and not self.dash[i] and self._content(i): + top.setdefault(key, i) + return top + + def block_end(self, start: int) -> int: + """The last content line of the block the key on ``start`` opens.""" + last = start + for j in range(start + 1, len(self.lines)): + if self.filler[j]: + continue + if not self.scalar[j] and self.indent[j] <= self.indent[start]: + break + last = j + return last + + def children(self, parent: int) -> list[int]: + """The mapping keys directly under the key on ``parent``.""" + out: list[int] = [] + child_indent: int | None = None + for j in range(parent + 1, self.block_end(parent) + 1): + if not self._content(j): + continue + if child_indent is None: + child_indent = self.indent[j] + if self.indent[j] == child_indent and self.key[j] is not None and not self.dash[j]: + out.append(j) + return out + + def parent(self, index: int) -> int | None: + """The key line one level above ``index``, or None when that line holds no key.""" + for j in range(index - 1, -1, -1): + if self._content(j) and self.indent[j] < self.indent[index]: + return j if self.key[j] is not None else None + return None + + def spans(self, found: Iterable[tuple[int, str]]) -> list[_Span]: + out: list[_Span] = [] + for start, symbol in found: + end = self.block_end(start) + width = self.indent[start] + text, shown = _cap([_dedent(line, width) for line in self.lines[start : end + 1]]) + out.append(_Span(start, end, shown, symbol, text)) + return out + + +def _load_json(text: str) -> object: + try: + return json.loads(text) + except (ValueError, RecursionError): + return None + + +def _walk(root: object) -> Iterator[tuple[tuple[str | int, ...], object]]: + """Every node under ``root`` with its key path, in document order.""" + stack: list[tuple[tuple[str | int, ...], object]] = [((), root)] + while stack: + pointer, node = stack.pop() + yield pointer, node + if isinstance(node, dict): + stack.extend(((*pointer, key), value) for key, value in reversed(list(node.items()))) + elif isinstance(node, list): + stack.extend(((*pointer, i), value) for i, value in reversed(list(enumerate(node)))) + + +def _json_key_re(key: str) -> re.Pattern[str]: + forms = dict.fromkeys((json.dumps(key, ensure_ascii=False), json.dumps(key))) + return re.compile("(?:" + "|".join(map(re.escape, forms)) + r")\s*:") + + +def _json_line(text: str, pointer: Iterable[str | int]) -> int: + """Best-effort line of a key path: each string key is searched after its parent.""" + position, found = 0, -1 + for key in pointer: + if not isinstance(key, str): + continue + match = _json_key_re(key).search(text, position) + if match is None: + break + found, position = match.start(), match.end() + return text.count("\n", 0, found) + 1 if found >= 0 else 1 + + +@dataclass(frozen=True) +class _Hit: + pointer: tuple[str | int, ...] + symbol: str + label: str | None + node: object + line: int + + +def _json_hit(text: str, pointer: tuple[str | int, ...], symbol: str, label: str | None, node: object) -> _Hit: + return _Hit(pointer, symbol, label, node, _json_line(text, pointer)) + + +def _json_excerpts(hits: list[_Hit]) -> list[Excerpt]: + rendered: list[tuple[_Hit, int, str, bool]] = [] + for order, hit in enumerate(hits): + body = json.dumps(hit.node, indent=2, ensure_ascii=False) + if hit.label is not None: + body = f"{json.dumps(hit.label, ensure_ascii=False)}: {body}" + lines = body.split("\n") + text, shown = _cap(lines) + rendered.append((hit, order, text, shown < len(lines))) + kept: list[tuple[_Hit, int, str, bool]] = [] + for entry in sorted(rendered, key=lambda e: len(e[0].pointer)): + pointer = entry[0].pointer + if any( + other[0].pointer == pointer or (not other[3] and pointer[: len(other[0].pointer)] == other[0].pointer) + for other in kept + ): + continue + kept.append(entry) + kept.sort(key=lambda e: (e[0].line, e[1])) + return [Excerpt(hit.line, hit.symbol, text) for hit, _, text, _ in kept] + + +def _blank(fragment: str) -> str: + return re.sub(r"[^\n]", " ", fragment) + + +def _sql_statements(text: str) -> Iterator[tuple[int, int, str]]: + """``(start, end, table)`` for each table statement: keyword to terminator.""" + pieces: list[str] = [] + bounds: list[tuple[int, int]] = [] + copied = begin = 0 + for match in _SQL_TOKEN.finditer(text): + kind = match.lastgroup + if kind == "comment": + pieces += [text[copied : match.start()], _blank(match.group())] + copied = match.end() + elif kind == "end": + bounds.append((begin, match.end())) + begin = match.end() + elif kind in ("changeset", "go"): + bounds.append((begin, match.start())) + begin = match.end() + pieces.append(text[copied:]) + bounds.append((begin, len(text))) + cleaned = "".join(pieces) + for begin, end in bounds: + first = _NON_SPACE.search(cleaned, begin, end) + if first is None: + continue + for head in _SQL_HEADS: + match = head.match(cleaned, first.start(), end) + if match: + stop = begin + len(cleaned[begin:end].rstrip()) + yield first.start(), stop, match.group("name") + break + + +def _bare_table(name: str) -> str: + parts = _SQL_NAME_PART.findall(name) + return parts[-1].strip('"`[]') if parts else name + + +def _liquibase_xml(text: str, keys: frozenset[str]) -> list[Excerpt]: + blanked = _XML_COMMENT.sub(lambda m: _blank(m.group()), text) + out: list[Excerpt] = [] + for change_set in _XML_CHANGESET.finditer(blanked): + for attr in _XML_TABLE_ATTR.finditer(change_set.group()): + name = attr.group(1) if attr.group(1) is not None else attr.group(2) + if normalize_name(name) in keys: + capped, _ = _cap(_slice_lines(text, change_set.start(), change_set.end())) + out.append(Excerpt(text.count("\n", 0, change_set.start()) + 1, name, capped)) + break + return out + + +def _liquibase_json(text: str, keys: frozenset[str]) -> list[Excerpt]: + doc = _load_json(text) + if doc is None: + return [] + starts = [text.count("\n", 0, m.start()) + 1 for m in _json_key_re("changeSet").finditer(text)] + hits: list[_Hit] = [] + seen = 0 + for pointer, node in _walk(doc): + if not pointer or pointer[-1] != "changeSet": + continue + line = starts[seen] if seen < len(starts) else 1 + seen += 1 + if not isinstance(node, dict): + continue + for inner_pointer, value in _walk(node): + key = inner_pointer[-1] if inner_pointer else None + if key in _TABLE_KEYS and isinstance(value, str) and normalize_name(value) in keys: + hits.append(_Hit(pointer, value, "changeSet", node, line)) + break + return _json_excerpts(hits) + + +_SPEC_SUFFIXES = (".yaml", ".yml", ".json") +_MIGRATION_SUFFIXES = (".sql", ".xml", ".yaml", ".yml", ".json") +_PREFIX_LOOKBACK_LINES = 40 + +_OPEN_CALL = re.compile(r"(?class|interface)[ \t]+[A-Za-z_$]" +) +_FLASK_PREFIX = re.compile( + r"""(? ContractTriggers: + """The contract triggers on one changed file's added lines. + + ``added`` holds the file's ``+`` lines; they are scanned joined by + newlines, so a statement or annotation that wraps across added lines + still counts. ``text`` is the file's full content at the head, or None. + + - ``routes``: the path literals of route declarations. Spring + ``@GetMapping``, ``@PostMapping``, ``@PutMapping``, ``@DeleteMapping``, + ``@PatchMapping`` and ``@RequestMapping`` give their positional string, + their ``value =`` or ``path =`` string, or every string of an array + form ``{"/a", "/b"}``, never a ``produces``, ``consumes``, ``name``, + ``headers`` or ``params`` string. JAX-RS ``@Path("...")`` counts, as do + FastAPI and Flask ``@.get|post|put|delete|patch|route|api_route("...")`` + and Express ``.get|post|put|delete|patch|all|use('/...')`` when + the literal starts with ``/``. Empty literals are dropped. When + ``text`` is given and a route was found, class-level prefixes are read + from it: a Spring ``@RequestMapping`` or JAX-RS ``@Path`` in the + annotations directly above a class or interface declaration, a Flask + ``Blueprint(..., url_prefix="...")`` and a FastAPI + ``APIRouter(prefix="...")``, at most the first of each kind in the + file. After the bare routes come, for each prefix, the prefix joined to + every route with exactly one ``/`` between them, because Spring and + Flask accept a segment written without its leading slash. + - ``tables``: every ``CREATE TABLE``, ``ALTER TABLE`` and + ``CREATE [UNIQUE] INDEX ... ON
`` head this module's + :func:`sql_excerpts` recognizes (the table, never the index name), each + ``REFERENCES
`` foreign-key target, and every Liquibase + ``tableName``, ``baseTableName`` and ``referencedTableName`` value, in + XML (``tableName="t"``), YAML (``tableName: t``) or JSON + (``"tableName": "t"``). Each is reduced to its bare name. This applies + to every language, because SQL also shows up in Python and Java + migrations. An index head is read only up to the next ``CREATE`` or + ``ALTER`` keyword, which keeps the scan linear. + - ``names``: :func:`prxref.repo_context.referenced_names` of the added + lines in the path's language. + - ``operation_ids``: the identifiers written directly before a ``(`` + (declarations and calls alike), minus the language's keywords. + """ + body = "\n".join(added) + language = language_of(path) + routes = _routes(body) + if routes and text is not None: + prefixes = _route_prefixes(text.removeprefix(_BOM)) + routes += [_join_route(prefix, route) for prefix in prefixes for route in routes] + keywords = _JAVA_KEYWORDS if language == "java" else chunk_context._keywords(language) + return ContractTriggers( + routes=_unique(routes), + tables=_unique(_tables(body)), + names=_unique(referenced_names(added, language)), + operation_ids=_unique(name for name in _OPEN_CALL.findall(body) if name not in keywords), + ) + + +def literal_contract_paths(globs: Sequence[str]) -> list[str]: + """The contract globs that are plain paths, in glob order, deduplicated. + + A glob is literal when it holds no ``*``, ``?`` or ``[`` and does not + start with ``!``. It names one repository-relative path, which is read + directly even when no listing shows it: a miss costs one read. A literal + that a ``!`` negation in the same list vetoes is left out, as + :func:`prxref.rules.match_globs` would leave it out. Blank globs are + ignored. + """ + out: dict[str, None] = {} + for glob in globs: + if not glob.strip() or glob.startswith("!") or any(c in glob for c in "*?["): + continue + if match_globs(glob, globs): + out.setdefault(glob) + return list(out) + + +def select_contract_files( + globs: Sequence[str], + *, + listing: Collection[str] | None, + diff_paths: Sequence[str], +) -> list[str]: + """The run's contract files: sorted, deduplicated repository-relative paths. + + A path from ``listing`` (the repository's file listing at the head, or + None when there is none) or from ``diff_paths`` counts when + :func:`prxref.rules.match_globs` selects it with ``globs``, and every + :func:`literal_contract_paths` path counts even when absent from both. + The caller passes the diff paths that were not removed; nothing is + filtered by status here. The result depends on no chunk, so it is + computed once per run. + """ + selected = {path for path in (*(listing or ()), *diff_paths) if path and match_globs(path, globs)} + selected.update(literal_contract_paths(globs)) + return sorted(selected) + + +def earlier_migrations(path: str, contract_paths: Sequence[str]) -> list[str]: + """The contract files a migration at ``path`` builds on, in ascending order. + + A candidate sits in the same directory as ``path``, has an extension + :func:`contract_excerpts` handles as a migration (``.sql``, ``.xml``, + ``.yaml``, ``.yml`` or ``.json``, case-insensitive), and sorts before + ``path`` under a natural sort of the basenames, where digit runs compare + as numbers, so ``V9__a.sql`` sorts before ``V10__b.sql``. At most + :data:`MAX_EARLIER_MIGRATIONS` are returned: the ones nearest to + ``path``. ``path`` itself is never returned. + """ + directory = posixpath.dirname(path) + own = _natural_key(posixpath.basename(path)) + earlier = sorted( + { + candidate + for candidate in contract_paths + if candidate != path + and posixpath.dirname(candidate) == directory + and _suffix(candidate) in _MIGRATION_SUFFIXES + and _natural_key(posixpath.basename(candidate)) < own + }, + key=lambda candidate: _natural_key(posixpath.basename(candidate)), + ) + return earlier[-MAX_EARLIER_MIGRATIONS:] + + +def contract_excerpts(path: str, text: str, triggers: ContractTriggers) -> list[Excerpt]: + """The excerpts one contract file gives for ``triggers``, chosen by extension. + + The extension is compared case-insensitively. ``names + tables`` below is + the two tuples concatenated and deduplicated. + + - ``.sql`` goes to :func:`sql_excerpts` with the tables. + - ``.xml`` goes to :func:`liquibase_excerpts` with the tables. + - ``.yaml`` and ``.yml``: a top-level ``openapi:`` or ``swagger:`` key (a + line that starts in column 0, the key optionally quoted) goes to + :func:`openapi_yaml_excerpts` with the routes, the operation ids and + ``names + tables`` as schemas. Otherwise a top-level + ``databaseChangeLog:`` goes to :func:`liquibase_excerpts`. Otherwise + the file is a fragment, such as one schema of a split spec: when the + :func:`normalize_name` of its stem equals that of a name or table, the + whole file is one excerpt at line 1 whose symbol is the stem, capped as + every excerpt is; otherwise it gives nothing. A text whose first + non-blank character is ``{`` is JSON and takes the ``.json`` branch. + - ``.json`` is parsed once to sniff its top-level keys: ``openapi`` or + ``swagger`` goes to :func:`openapi_json_excerpts`, + ``databaseChangeLog`` to :func:`liquibase_excerpts`, and anything else, + invalid JSON included, to :func:`json_schema_excerpts` with + ``names + tables``. + - Any other extension gives ``[]``. + + The stem is the basename up to its first dot, so + ``transport-config.schema.json`` has the stem ``transport-config``. + """ + suffix = _suffix(path) + text = text.removeprefix(_BOM) + schemas = _unique((*triggers.names, *triggers.tables)) + if suffix == ".sql": + return sql_excerpts(text, tables=triggers.tables) + if suffix == ".xml": + return liquibase_excerpts(text, tables=triggers.tables) + if suffix not in _SPEC_SUFFIXES: + return [] + if suffix != ".json" and not text.lstrip().startswith("{"): + if _YAML_OPENAPI.search(text): + return openapi_yaml_excerpts( + text, routes=triggers.routes, operation_ids=triggers.operation_ids, schemas=schemas + ) + if _YAML_CHANGELOG.search(text): + return liquibase_excerpts(text, tables=triggers.tables) + return _fragment_excerpts(path, text, schemas) + doc = _load_json(text) + keys = doc if isinstance(doc, dict) else {} + if "openapi" in keys or "swagger" in keys: + return openapi_json_excerpts( + text, routes=triggers.routes, operation_ids=triggers.operation_ids, schemas=schemas + ) + if "databaseChangeLog" in keys: + return liquibase_excerpts(text, tables=triggers.tables) + return json_schema_excerpts(text, names=schemas) + + +def contract_entries( + chunk: Sequence[object], + *, + contract_paths: Sequence[str], + read: Callable[[str], str | None] | None, + priority: Sequence[str] = (), +) -> list[ContextEntry]: + """The contract entries for one worker chunk, ordered by ``(path, line)``. + + ``chunk`` is the chunk's file diffs, duck-typed on ``path``, ``hunks`` and + ``lines`` (``kind``, ``text``, ``new_line``) as + :func:`prxref.chunk_context.chunk_files` reads them. ``contract_paths`` is + :func:`select_contract_files`'s result. ``read`` is the chunk's capped + reader: each call may cost a read, and a path it excludes or cannot read + gives None, which is skipped silently. ``read`` None gives ``[]`` with no + work. ``priority`` lists paths that rank first among the spec files, + normally :func:`literal_contract_paths` of the globs. + + Each chunk file's triggers come from :func:`contract_triggers`. The file's + own text is read only when its added lines hold a route, so its + class-level prefix can be found. The triggers of all the chunk's files + are merged in first-appearance order, and when every field is empty the + result is ``[]`` with no read at all. + + The files then read, in this order and once each, are: + + 1. when there are tables, the :func:`earlier_migrations` of each chunk + file that is itself a contract path, at most + :data:`MAX_EARLIER_MIGRATIONS` per file; + 2. at most :data:`MAX_SPEC_FILES` spec candidates, the contract paths + ending in ``.yaml``, ``.yml`` or ``.json``, ranked: a path in + ``priority``, then a path whose stem matches a name or table by + :func:`normalize_name`, then a basename that starts with ``openapi`` + or ``swagger`` or ends with ``.schema.json`` (case-insensitive), then + the rest, ties broken by path. The ranking is deterministic, so every + chunk picks the same root spec. + + A contract path that is itself a file of this chunk is never read, and + ``.sql`` and ``.xml`` contract files are read only as earlier + migrations. Each file read goes through :func:`contract_excerpts`, and + each :class:`Excerpt` becomes a ``ContextEntry`` of kind and reason + ``"contract"``. Entries that share a ``(path, line)`` keep the first. + No character budget applies here: the only caps are the per-excerpt + ones, :data:`MAX_EARLIER_MIGRATIONS` and :data:`MAX_SPEC_FILES`. + """ + if read is None: + return [] + files = chunk_context.chunk_files(chunk) + per_file: list[ContractTriggers] = [] + for changed in files: + triggers = contract_triggers(changed.path, changed.added) + if triggers.routes: + text = read(changed.path) + if text is not None: + triggers = contract_triggers(changed.path, changed.added, text=text) + per_file.append(triggers) + merged = ContractTriggers( + routes=_unique(route for t in per_file for route in t.routes), + tables=_unique(table for t in per_file for table in t.tables), + names=_unique(name for t in per_file for name in t.names), + operation_ids=_unique(op for t in per_file for op in t.operation_ids), + ) + if not (merged.routes or merged.tables or merged.names or merged.operation_ids): + return [] + in_chunk = {changed.path for changed in files} + queue: dict[str, None] = {} + if merged.tables: + contract_set = set(contract_paths) + for changed in files: + if changed.path in contract_set: + earlier = earlier_migrations(changed.path, contract_paths) + queue.update(dict.fromkeys(p for p in earlier if p not in in_chunk)) + wanted = _keys((*merged.names, *merged.tables), normalize_name) + ranks = frozenset(priority) + specs = sorted( + {p for p in contract_paths if _suffix(p) in _SPEC_SUFFIXES and p not in in_chunk and p not in queue}, + key=lambda p: (_spec_rank(p, ranks, wanted), p), + ) + queue.update(dict.fromkeys(specs[:MAX_SPEC_FILES])) + entries: dict[tuple[str, int], ContextEntry] = {} + for path in queue: + text = read(path) + if text is None: + continue + for excerpt in contract_excerpts(path, text, merged): + entries.setdefault( + (path, excerpt.line), + ContextEntry( + path=path, line=excerpt.line, symbol=excerpt.symbol, kind="contract", reason="contract", + text=excerpt.text, + ), + ) + return [entries[key] for key in sorted(entries)] + + +def _unique(values: Iterable[str]) -> tuple[str, ...]: + return tuple(dict.fromkeys(value for value in values if value)) + + +def _suffix(path: str) -> str: + return posixpath.splitext(path)[1].lower() + + +def _stem(path: str) -> str: + return posixpath.basename(path).split(".", 1)[0] + + +def _natural_key(name: str) -> tuple[tuple[object, ...], str]: + """A natural sort key: digit runs compare by value, without converting them to ``int``.""" + parts = _DIGITS.split(name) + return ( + tuple((len(part.lstrip("0")), part.lstrip("0")) if i % 2 else part for i, part in enumerate(parts)), + name, + ) + + +def _spec_rank(path: str, priority: frozenset[str], wanted: frozenset[str]) -> int: + if path in priority: + return 0 + if normalize_name(_stem(path)) in wanted: + return 1 + base = posixpath.basename(path).lower() + if base.startswith(("openapi", "swagger")) or base.endswith(".schema.json"): + return 2 + return 3 + + +def _fragment_excerpts(path: str, text: str, schemas: Iterable[str]) -> list[Excerpt]: + stem = _stem(path) + if normalize_name(stem) not in _keys(schemas, normalize_name): + return [] + lines = [line.rstrip() for line in text.split("\n")] + while lines and not lines[-1]: + lines.pop() + if not lines: + return [] + capped, _ = _cap(lines) + return [Excerpt(1, stem, capped)] + + +def _first_group(match: re.Match[str]) -> str: + return next(group for group in match.groups() if group is not None) + + +def _tables(body: str) -> list[str]: + """Bare table names in ``body``, in order of appearance.""" + found: list[tuple[int, str]] = [] + verbs = [match.start() for match in _SQL_VERB.finditer(body)] + for start, end in zip(verbs, [*verbs[1:], len(body)], strict=False): + for head in _SQL_HEADS: + match = head.match(body, start, end) + if match: + found.append((start, match.group("name"))) + break + found += [(match.start(), match.group("name")) for match in _SQL_REFERENCES.finditer(body)] + found += [(match.start(), _first_group(match)) for match in _LIQUIBASE_TABLE.finditer(body)] + bare = (_bare_table(name.strip()) for _, name in sorted(found)) + return [name for name in bare if _WORD.search(name)] + + +def _annotation_paths(args: str) -> list[str]: + """The route strings of a Java annotation's arguments: positional, ``value =`` or ``path =``.""" + out: list[str] = [] + for match in _ANNOTATION_ARG.finditer(args): + key, value = match.groups() + if key is not None and key not in ("value", "path"): + continue + out += _JAVA_STRING.findall(value) if value.startswith("{") else [value[1:-1]] + return out + + +def _routes(body: str) -> list[str]: + found: list[tuple[int, int, str]] = [] + for match in _SPRING_MAPPING.finditer(body): + found += [(match.start(), i, route) for i, route in enumerate(_annotation_paths(match.group(2)))] + for match in _JAXRS_PATH.finditer(body): + found += [(match.start(), i, route) for i, route in enumerate(_annotation_paths(match.group(1)))] + for regex in (_PY_ROUTE, _EXPRESS_ROUTE): + found += [(match.start(), 0, _first_group(match)) for match in regex.finditer(body)] + return [route for _, _, route in sorted(found) if route] + + +def _annotation_block(lines: list[str], index: int) -> list[str]: + """The annotation lines directly above line ``index``, a wrapped annotation's arguments included.""" + block: list[str] = [] + depth = 0 + for j in range(index - 1, max(index - 1 - _PREFIX_LOOKBACK_LINES, -1), -1): + line = lines[j] + depth += line.count(")") - line.count("(") + if depth <= 0 and not line.lstrip().startswith("@"): + break + block.append(line) + return block[::-1] + + +def _class_prefixes(text: str) -> tuple[str | None, str | None]: + """The first class-level Spring ``@RequestMapping`` and JAX-RS ``@Path`` paths, or None.""" + lines = text.split("\n") + spring: str | None = None + jaxrs: str | None = None + for index, line in enumerate(lines): + if "class" not in line and "interface" not in line: + continue + decl = _JAVA_TYPE_DECL.match(line) + if decl is None: + continue + head = "\n".join([*_annotation_block(lines, index), line[: decl.start("kw")]]) + if spring is None: + mappings = (m for m in _SPRING_MAPPING.finditer(head) if m.group(1) == "Request") + spring = next((p for m in mappings for p in _annotation_paths(m.group(2))), None) + if jaxrs is None: + jaxrs = next((p for m in _JAXRS_PATH.finditer(head) for p in _annotation_paths(m.group(1))), None) + if spring is not None and jaxrs is not None: + break + return spring, jaxrs + + +def _route_prefixes(text: str) -> list[str]: + prefixes = list(_class_prefixes(text)) + for regex in (_FLASK_PREFIX, _FASTAPI_PREFIX): + match = regex.search(text) + prefixes.append(_first_group(match) if match else None) + return [prefix for prefix in prefixes if prefix] + + +def _join_route(prefix: str, route: str) -> str: + return prefix.rstrip("/") + "/" + route.lstrip("/") diff --git a/src/prxref/repo_crosschunk.py b/src/prxref/repo_crosschunk.py new file mode 100644 index 0000000..3bd1b21 --- /dev/null +++ b/src/prxref/repo_crosschunk.py @@ -0,0 +1,234 @@ +"""Cross-chunk and diff-file definitions for one worker chunk. + +``chunk_context.referenced_definitions`` shows a worker the definition of a +name its added lines reference only when that definition sits in the SAME +changed file, and it knows no Java. When a PR changes a type in one chunk and +another chunk calls it, the calling chunk's worker never sees the new +invariant. :func:`diff_definitions` closes that gap for the ``diff`` level of +``PRXREF_REPO_CONTEXT``: it searches every file of the PR, not just the +chunk's own, and for a type changed in another chunk it also shows the changed +lines inside that type. + +File text comes from the ``read(path) -> str | None`` callable at the PR head. +With no reader, or when a read returns ``None``, the only text known is the +file's hunks (its context and ``+`` lines), and entries are built from those +alone, so a forge without file reads still gets hunk-based entries. The module +is pure: stdlib plus :mod:`prxref.repo_context` and :mod:`prxref.chunk_context`, +with no I/O except through ``read``. It applies no budget; per-entry caps are +the only limits here. +""" +from __future__ import annotations + +from collections.abc import Callable, Sequence + +from . import chunk_context +from .repo_context import ( + REASONS, + ContextEntry, + definition_regexes, + find_definitions, + language_of, + referenced_names, +) + +MAX_CHANGE_LINES = 12 + +_SAME_FILE_LANGUAGES = frozenset({"js", "python"}) + + +def _diff_lines(f: object) -> list[object]: + return [ + line + for hunk in getattr(f, "hunks", None) or [] + for line in getattr(hunk, "lines", None) or [] + ] + + +def _hunk_text(f: object) -> dict[int, str]: + known: dict[int, str] = {} + for line in _diff_lines(f): + new_line = getattr(line, "new_line", None) + if getattr(line, "kind", " ") in (" ", "+") and isinstance(new_line, int): + known.setdefault(new_line, getattr(line, "text", "")) + return known + + +def _added_text(f: object) -> dict[int, str]: + added: dict[int, str] = {} + for line in _diff_lines(f): + new_line = getattr(line, "new_line", None) + if getattr(line, "kind", " ") == "+" and isinstance(new_line, int): + added.setdefault(new_line, getattr(line, "text", "")) + return added + + +def _runs(numbers: Sequence[int]) -> list[list[int]]: + runs: list[list[int]] = [] + for number in sorted(numbers): + if runs and number == runs[-1][-1] + 1: + runs[-1].append(number) + else: + runs.append([number]) + return runs + + +def _hunk_definitions( + known: dict[int, str], + names: Sequence[str], + language: str, + skip: frozenset[int], +) -> list[tuple[str, int, str]]: + found: list[tuple[str, int, str]] = [] + seen: set[str] = set() + for run in _runs(list(known)): + wanted = [name for name in names if name not in seen] + if not wanted: + break + offset = run[0] - 1 + local_skip = frozenset(number - offset for number in run if number in skip) + text = "\n".join(known[number] for number in run) + for symbol, line, body in find_definitions( + text, wanted, language=language, skip_lines=local_skip + ): + seen.add(symbol) + found.append((symbol, line + offset, body)) + return found + + +def _indent(text: str) -> int: + return len(text) - len(text.lstrip()) + + +def _window_end(known: dict[int, str], line: int, language: str) -> int | None: + regexes = definition_regexes(language) + depth = _indent(known.get(line, "")) + for number in sorted(known): + if number <= line: + continue + text = known[number] + if _indent(text) <= depth and any(regex.match(text) for regex in regexes): + return number + return None + + +def _change_text(lines: Sequence[str]) -> str: + shown = [text.rstrip() for text in lines[:MAX_CHANGE_LINES]] + hidden = len(lines) - len(shown) + if hidden > 0: + shown.append(f"… {hidden} more changed lines") + return "\n".join(shown) + + +def _change_entries( + path: str, + symbol: str, + line: int, + end: int | None, + added: dict[int, str], +) -> list[ContextEntry]: + inside = [n for n in added if n > line and (end is None or n < end)] + return [ + ContextEntry( + path=path, + line=run[0], + symbol=symbol, + kind="definition", + reason="cross-chunk", + text=_change_text([added[n] for n in run]), + ) + for run in _runs(inside) + ] + + +def diff_definitions( + chunk: Sequence[object], + all_files: Sequence[object], + read: Callable[[str], str | None] | None, +) -> list[ContextEntry]: + """Definitions from the PR's diff files for the names one chunk's added lines reference. + + ``chunk`` holds the worker's ``triage.FileDiff`` records and ``all_files`` + the whole PR's, both duck-typed on ``path``, ``status`` and ``hunks`` + (whose ``lines`` carry ``kind``, ``text`` and ``new_line``). The wanted + names are the union, in first-appearance order, of + :func:`~prxref.repo_context.referenced_names` over each chunk file's added + lines in that file's language. + + Every file in ``all_files`` with definition regexes is searched with + :func:`~prxref.repo_context.find_definitions`, reading its text with + ``read`` or, when ``read`` is ``None`` or returns ``None``, taking it from + the file's hunks: each contiguous run of known new-file lines is scanned + on its own, so an entry carries true line numbers and never shows a line + the hunks lack. A removed file is skipped, and so are the lines of this + chunk's own hunks. A js or python file in the chunk is skipped entirely + when ``read`` is given, because ``referenced_definitions`` already covers + it; a Java file in the chunk is searched outside its hunks. + + A hit in a file outside the chunk that has ``+`` lines anywhere in the PR + has reason ``"cross-chunk"``, and every other hit ``"diff-file"``. A + cross-chunk hit also yields one entry per contiguous run of ``+`` lines + after the definition line and before the next definition that is not + indented deeper than it (or the end of the known text): same symbol and + reason, ``line`` the run's first new-file line, and the run's text capped + at :data:`MAX_CHANGE_LINES` lines plus a ``… N more changed lines`` line. + Every entry's kind is ``"definition"``. + + Entries are ordered by ``(reason rank in REASONS, path, line)`` and + deduplicated on ``(path, line)``, the first kept. No budget is applied. + """ + own_files = chunk_context.chunk_files(chunk) + own = {entry.path: entry for entry in reversed(own_files)} + names: dict[str, None] = {} + for entry in own_files: + for name in referenced_names(entry.added, language_of(entry.path)): + names.setdefault(name, None) + if not names: + return [] + wanted = list(names) + changed = { + getattr(f, "path", "") + for f in all_files + if any(getattr(line, "kind", " ") == "+" for line in _diff_lines(f)) + } + + collected: list[ContextEntry] = [] + searched: set[str] = set() + for f in all_files: + path = getattr(f, "path", "") or "" + if not path or path in searched: + continue + searched.add(path) + if getattr(f, "status", "") == "removed": + continue + language = language_of(path) + if not definition_regexes(language): + continue + mine = own.get(path) + if mine is not None and read is not None and language in _SAME_FILE_LANGUAGES: + continue + skip = mine.hunk_lines if mine is not None else frozenset() + text = read(path) if read is not None else None + if isinstance(text, str): + known = dict(enumerate(text.splitlines(), start=1)) + found = find_definitions(text, wanted, language=language, skip_lines=skip) + else: + known = _hunk_text(f) + found = _hunk_definitions(known, wanted, language, skip) + reason = "cross-chunk" if mine is None and path in changed else "diff-file" + for symbol, line, body in found: + collected.append(ContextEntry(path, line, symbol, "definition", reason, body)) + if reason == "cross-chunk": + added = _added_text(f) + for symbol, line, _ in found: + end = _window_end(known, line, language) + collected.extend(_change_entries(path, symbol, line, end, added)) + + collected.sort(key=lambda e: (REASONS.index(e.reason), e.path, e.line)) + out: list[ContextEntry] = [] + kept: set[tuple[str, int]] = set() + for entry in collected: + key = (entry.path, entry.line) + if key not in kept: + kept.add(key) + out.append(entry) + return out diff --git a/src/prxref/repo_reader.py b/src/prxref/repo_reader.py new file mode 100644 index 0000000..78e6f45 --- /dev/null +++ b/src/prxref/repo_reader.py @@ -0,0 +1,287 @@ +"""The run-scoped repository reader behind repository context (#17). + +One ``RepoReader`` serves every file read and the one path listing a review +run makes when repository context is on. It sits in front of a forge's +optional ``get_file_content`` and ``list_paths`` (``forge_reader``) or a +local ``--repo-dir`` tree (``repo_dir_reader``) and gives the chunk workers, +which run in parallel, one shared view of the repository: + +- every path is fetched at most once per run, single-flight across threads, + and a miss is cached like a hit; +- the listing is fetched at most once per run, single-flight as well; +- each chunk's reader is capped at ``MAX_CHUNK_READS`` uncached reads and all + chunk readers together at ``MAX_RUN_READS``, so repository context bounds + both its network cost and its prompt growth; +- an excluded path is never read and never listed, at the one boundary every + read crosses; +- ``stats`` reports the counters the run record carries. + +The module does no prompt work and knows nothing about which paths are +worth reading; the caller decides that and passes plain callables in. +""" + +from __future__ import annotations + +import logging +import threading +from collections.abc import Callable + +from prxref.forges.base import Forge, PathListing, PRRef +from prxref.forges.repo_dir import RepoDir + +logger = logging.getLogger(__name__) + +MAX_RUN_READS = 200 +MAX_CHUNK_READS = 16 +READER_KINDS = ("forge", "repo-dir") + + +class _Slot: + """One single-flight result: ``done`` is set once ``value`` is final.""" + + __slots__ = ("done", "value") + + def __init__(self) -> None: + self.done = threading.Event() + self.value: str | PathListing | None = None + + +class RepoReader: + """A cached, single-flight, capped view of one repository for one review run. + + ``fetch(path)`` returns a file's text or None; ``lister()`` returns a + ``PathListing`` or None, and ``lister=None`` means the source cannot list. + Either callable may raise or return the wrong type: an exception is + logged at DEBUG and read as None, and a ``fetch`` result that is not a + ``str`` (or a ``lister`` result that is not a ``PathListing``) is read as + None. ``kind`` is ``"forge"`` or ``"repo-dir"``, the run record's + ``reader`` field. ``exclude(path)`` returning true (or raising) makes a + path unreadable and unlisted. + + The object is thread-safe and meant to be shared by every chunk worker + of a run. ``read`` is cached and single-flight but not capped: its + fetches count in ``stats()["reads"]`` and never spend either cap. + ``chunk_reader`` hands each chunk a reader that also enforces + ``chunk_cap`` for that chunk and ``run_cap`` across all chunk readers. + A path already cached or in flight is served without counting against + either cap, whichever reader fetched it. + + Determinism: below the caps, what every read returns is independent of + the order in which parallel workers ask, because each path is fetched + once and every caller sees that one result. Once the run cap is reached, + WHICH chunk meets it first depends on thread scheduling; that is + accepted only because ``stats()["read_cap_hit"]`` records that it + happened. + """ + + def __init__( + self, + fetch: Callable[[str], str | None], + lister: Callable[[], PathListing | None] | None, + *, + kind: str, + exclude: Callable[[str], bool] | None = None, + run_cap: int = MAX_RUN_READS, + chunk_cap: int = MAX_CHUNK_READS, + ) -> None: + """Wrap ``fetch`` and ``lister``; raises ``ValueError`` for a ``kind`` outside ``READER_KINDS``.""" + if kind not in READER_KINDS: + raise ValueError(f"RepoReader kind must be one of {READER_KINDS}, got {kind!r}") + self.kind = kind + self._fetch = fetch + self._lister = lister + self._exclude = exclude + self._run_cap = run_cap + self._chunk_cap = chunk_cap + self._lock = threading.Lock() + self._slots: dict[str, _Slot] = {} + self._reads = 0 + self._run_used = 0 + self._read_cap_hit = False + self._listing_slot: _Slot | None = None + + def _excluded(self, path: str) -> bool: + if self._exclude is None: + return False + try: + return bool(self._exclude(path)) + except Exception as e: # noqa: BLE001 - an exclude that cannot answer fails closed + logger.debug("repository context exclude(%s) failed: %s", path, e) + return True + + def _fetch_value(self, path: str) -> str | None: + try: + value = self._fetch(path) + except Exception as e: # noqa: BLE001 - context is never worth a failed review + logger.debug("%s read of %s failed: %s", self.kind, path, e) + return None + return value if isinstance(value, str) else None + + def _get(self, path: str, admit: Callable[[], bool] | None) -> str | None: + if self._excluded(path): + return None + with self._lock: + slot = self._slots.get(path) + owner = slot is None + if owner: + if admit is not None and not admit(): + self._read_cap_hit = True + return None + slot = _Slot() + self._slots[path] = slot + self._reads += 1 + if owner: + try: + slot.value = self._fetch_value(path) + finally: + slot.done.set() + else: + slot.done.wait() + value = slot.value + return value if isinstance(value, str) else None + + def read(self, path: str) -> str | None: + """Return the text of ``path``, or None; cached and single-flight, never capped. + + The first caller for an uncached path fetches it; a concurrent caller + for the same path waits for that one fetch and gets its result. A + None result is cached too, so a miss costs one fetch per run. An + excluded path returns None without fetching or counting. + """ + return self._get(path, None) + + def chunk_reader(self) -> Callable[[str], str | None]: + """Return a new ``read(path) -> str | None`` for one chunk, enforcing both caps. + + A path already cached or in flight is served as ``read`` serves it, + without counting. An uncached path counts one against this chunk's + ``chunk_cap`` and one against the run's ``run_cap``, which every + chunk reader of this ``RepoReader`` shares. When either cap is + already reached, it returns None without fetching, leaves the path + uncached, and marks ``read_cap_hit``. An excluded path returns None + without fetching or counting. + """ + used = 0 + + def admit() -> bool: + nonlocal used + if used >= self._chunk_cap or self._run_used >= self._run_cap: + return False + used += 1 + self._run_used += 1 + return True + + def read(path: str) -> str | None: + return self._get(path, admit) + + return read + + def _list_value(self) -> PathListing | None: + try: + listing = self._lister() + except Exception as e: # noqa: BLE001 - context is never worth a failed review + logger.debug("%s path listing failed: %s", self.kind, e) + return None + if not isinstance(listing, PathListing): + return None + if self._exclude is None: + return listing + kept = tuple(path for path in listing.paths if not self._excluded(path)) + return PathListing(paths=kept, complete=listing.complete) + + def listing(self) -> PathListing | None: + """Return the repository's path listing, or None; the lister runs at most once per run. + + Concurrent callers share one call to ``lister``, and its result is + cached, including None, a raise read as None, and a result that is + not a ``PathListing`` read as None. Excluded paths are filtered out, + and ``complete`` stays as the source reported it. With + ``lister=None`` this returns None and calls nothing. + """ + if self._lister is None: + return None + with self._lock: + slot = self._listing_slot + owner = slot is None + if owner: + slot = self._listing_slot = _Slot() + if owner: + try: + slot.value = self._list_value() + finally: + slot.done.set() + else: + slot.done.wait() + value = slot.value + return value if isinstance(value, PathListing) else None + + def stats(self) -> dict: + """Return a snapshot of the run's counters for the run record. + + ``{"reads": int, "read_cap_hit": bool, "listing": {"paths": int, + "complete": bool} | None}``. ``reads`` counts every call that reached + ``fetch``, from ``read`` and chunk readers alike. ``read_cap_hit`` is + true once any chunk reader refused a path at a cap. ``listing`` is + None until ``listing()`` has finished, and when it gave None; its + ``paths`` counts the listing after exclusion. Taking a snapshot never + calls ``fetch`` or ``lister``. + """ + with self._lock: + reads = self._reads + cap_hit = self._read_cap_hit + slot = self._listing_slot + listing = None + if slot is not None and slot.done.is_set() and isinstance(slot.value, PathListing): + listing = {"paths": len(slot.value.paths), "complete": slot.value.complete} + return {"reads": reads, "read_cap_hit": cap_hit, "listing": listing} + + +def forge_reader( + forge: Forge, + ref: PRRef, + sha: str | None, + *, + exclude: Callable[[str], bool] | None = None, +) -> RepoReader | None: + """Return a ``RepoReader`` over a forge at commit ``sha``, or None. + + None when the forge has no ``get_file_content`` or ``sha`` is empty, + exactly as the orchestrator's same-file reader decides. Reads call + ``get_file_content(ref, path, sha=sha)``. The listing calls + ``list_paths(ref, sha=sha)`` when the forge has that method; otherwise + the reader has no lister and ``listing()`` is None. ``kind`` is + ``"forge"``. + """ + getter = getattr(forge, "get_file_content", None) + if getter is None or not sha: + return None + list_paths = getattr(forge, "list_paths", None) + + def fetch(path: str) -> str | None: + return getter(ref, path, sha=sha) + + def list_at_sha() -> PathListing | None: + return list_paths(ref, sha=sha) + + lister = None if list_paths is None else list_at_sha + return RepoReader(fetch, lister, kind="forge", exclude=exclude) + + +def repo_dir_reader( + repo_dir: RepoDir, + *, + exclude: Callable[[str], bool] | None = None, +) -> RepoReader: + """Return a ``RepoReader`` over a local ``RepoDir`` tree. + + Reads call ``repo_dir.read``; the listing wraps ``repo_dir.list_files()`` + into a ``PathListing``. Both caps apply as they do to a forge, because + they bound prompt growth as well as network cost. ``kind`` is + ``"repo-dir"``. + """ + + def lister() -> PathListing: + paths, complete = repo_dir.list_files() + return PathListing(paths=tuple(paths), complete=complete) + + return RepoReader(repo_dir.read, lister, kind="repo-dir", exclude=exclude) diff --git a/src/prxref/repo_resolve.py b/src/prxref/repo_resolve.py new file mode 100644 index 0000000..5933e72 --- /dev/null +++ b/src/prxref/repo_resolve.py @@ -0,0 +1,472 @@ +"""Candidate files for the definitions a changed file references (repository context). + +:func:`prxref.repo_context.referenced_names` says WHICH identifiers a chunk's +added lines use. This module says WHERE their definitions probably live: it +turns a referencing file plus those names into an ordered list of +:class:`Candidate` files. Reading the candidates and running +:func:`prxref.repo_context.find_definitions` over their text is the caller's +job, not this module's. + +The module is pure. It is stdlib plus :mod:`prxref.repo_context`, and it +performs no I/O, no reads and no network. Every candidate costs the caller a +read, and a miss costs one too, so the rules stay narrow and the resolution is +best effort by design: no build file, ``tsconfig`` path alias, ``sys.path`` or +classpath is consulted. + +Candidates come in three groups, highest rank first, each carrying a member of +:data:`prxref.repo_context.REASONS` as its ``reason``: + +- ``"import"``: Java imports from the file's own organization under the source + root its ``package`` line implies, Python ``from ... import`` statements, + and TS/JS relative ``import ... from`` and ``export ... from`` specifiers. +- ``"path-convention"``: a Java type from the file's own package, + ``/.java``. +- ``"name-search"``: listing files of the same language whose stem equals the + name, when the caller has a repository listing. +""" +from __future__ import annotations + +import posixpath +import re +import sys +from collections.abc import Collection, Sequence +from dataclasses import dataclass + +from .repo_context import definition_regexes, language_of + +MAX_NAME_SEARCH_PER_NAME = 3 + +_JAVA_PACKAGE_RE = re.compile(r"^[ \t]*package[ \t]+([\w$.]+)[ \t]*;", re.M) +_JAVA_IMPORT_RE = re.compile(r"^[ \t]*import[ \t]+(static[ \t]+)?([\w$.]+?)(\.\*)?[ \t]*;", re.M) +_JAVA_EXTERNAL_ROOTS = frozenset({"java", "javax"}) + +_PY_FROM_RE = re.compile( + r"^[ \t]*from[ \t]+(\.*)[ \t]*([A-Za-z_][\w.]*)?[ \t]+import[ \t]+(\([^)]*\)|\([^\n]*|[^\n]*)", + re.M, +) +_PY_ITEM_RE = re.compile(r"^([A-Za-z_]\w*)(?:\s+as\s+([A-Za-z_]\w*))?$") +_PY_STDLIB = sys.stdlib_module_names + +_JS_IDENT = r"[A-Za-z_$][\w$]*" +_JS_FROM_RE = re.compile( + r"(? str | None: + parts: list[str] = [] + for part in path.split("/"): + if part in ("", "."): + continue + if part == "..": + if not parts: + return None + parts.pop() + continue + parts.append(part) + return "/".join(parts) or None + + +def _java_root(directory: str, package: str) -> str | None: + if not package: + return None + package_dir = package.replace(".", "/") + if directory == package_dir: + return "" + if directory.endswith("/" + package_dir): + return directory[: -len(package_dir)] + return None + + +def _java_type_index(parts: Sequence[str]) -> int | None: + for index, part in enumerate(parts): + if part[:1].isupper(): + return index + return None + + +def _java( + directory: str, text: str, names: Sequence[str] +) -> tuple[list[_Found], list[_Found], set[str]]: + wanted = set(names) + package_match = _JAVA_PACKAGE_RE.search(text) + package = package_match.group(1) if package_match else "" + root = _java_root(directory, package) + org = package.split(".")[:2] if package else [] + statements = [ + (bool(m.group(1)), m.group(2).split("."), bool(m.group(3))) for m in _JAVA_IMPORT_RE.finditer(text) + ] + claimed = {parts[-1] for _, parts, wildcard in statements if not wildcard} + external: set[str] = set() + imports: list[_Found] = [] + for static, parts, wildcard in statements: + third_party = parts[0] in _JAVA_EXTERNAL_ROOTS or bool(org and parts[: len(org)] != org) + if third_party and not wildcard: + external.add(parts[-1]) + if static or third_party or root is None: + continue + index = _java_type_index(parts) + if wildcard: + for name in names: + if name in claimed: + continue + file_parts = parts[: index + 1] if index is not None else [*parts, name] + imports.append((name, name, root + "/".join(file_parts) + ".java")) + continue + name = parts[-1] + if name in wanted: + file_parts = parts[: index + 1] if index is not None else parts + imports.append((name, name, root + "/".join(file_parts) + ".java")) + conventions = [ + (name, name, f"{directory}/{name}.java" if directory else f"{name}.java") + for name in names + if name not in claimed + ] + return imports, conventions, external + + +def _py_items(clause: str) -> list[tuple[str, str]]: + body = re.sub(r"#[^\n]*", "", clause).strip() + if body.startswith("("): + body = body[1:].split(")", 1)[0] + else: + body = body.split(";", 1)[0] + items: list[tuple[str, str]] = [] + for raw in body.split(","): + item = " ".join(raw.split()) + if item == "*": + items.append(("*", "*")) + continue + match = _PY_ITEM_RE.match(item) + if match: + items.append((match.group(1), match.group(2) or match.group(1))) + return items + + +def _py_module_files(directory: str, dots: str, module: str) -> list[str]: + relative = module.replace(".", "/") + if dots: + base = "/".join([directory, *[".."] * (len(dots) - 1)]) + if relative: + probes = [f"{base}/{relative}.py", f"{base}/{relative}/__init__.py"] + else: + probes = [f"{base}/__init__.py"] + else: + probes = [ + f"{relative}.py", + f"{relative}/__init__.py", + f"src/{relative}.py", + f"src/{relative}/__init__.py", + ] + return [p for p in map(_normalize, probes) if p] + + +def _python(directory: str, text: str, names: Sequence[str]) -> tuple[list[_Found], set[str]]: + wanted = set(names) + imports: list[_Found] = [] + external: set[str] = set() + bound: set[str] = set() + stars: list[list[str]] = [] + for match in _PY_FROM_RE.finditer(text.replace("\\\n", " ")): + dots, module, clause = match.group(1), match.group(2) or "", match.group(3) + if not dots and not module: + continue + items = _py_items(clause) + bound.update(local for _, local in items if local != "*") + if not dots and module.split(".")[0] in _PY_STDLIB: + external.update(local for _, local in items if local != "*") + continue + files = _py_module_files(directory, dots, module) + for original, local in items: + if original == "*": + stars.append(files) + elif local in wanted: + imports.extend((local, original, f) for f in files) + for files in stars: + for name in names: + if name not in bound: + imports.extend((name, name, f) for f in files) + return imports, external + + +def _js_module_files(directory: str, specifier: str) -> list[str]: + if not (specifier in (".", "..") or specifier.startswith(("./", "../"))): + return [] + base = f"{directory}/{specifier}" if directory else specifier + lower = specifier.lower() + if specifier in (".", "..") or specifier.endswith("/"): + probes = [base.rstrip("/") + "/index.ts"] + elif lower.endswith(_JS_ASSET_SUFFIXES): + return [] + elif lower.endswith(".js"): + probes = [base[:-3] + suffix for suffix in _JS_ESM_PROBES] + elif lower.endswith(_JS_LITERAL_SUFFIXES): + probes = [base] + else: + probes = [base + suffix for suffix in _JS_PROBES] + return [p for p in map(_normalize, probes) if p] + + +def _js_clause(clause: str) -> tuple[list[tuple[str, str]], list[str]]: + pairs: list[tuple[str, str]] = [] + namespaces: list[str] = [] + brace = clause.find("{") + head = clause if brace == -1 else clause[:brace] + body = "" if brace == -1 else clause[brace + 1 : clause.rfind("}")] + for raw in head.split(","): + part = " ".join(raw.split()) + namespace = _JS_NAMESPACE_RE.match(part) + if namespace: + namespaces.append(namespace.group(1)) + elif part and part != "*" and re.fullmatch(_JS_IDENT, part): + pairs.append(("default", part)) + for raw in _JS_COMMENT_RE.sub("", body).split(","): + match = _JS_ITEM_RE.match(" ".join(raw.split())) + if match: + pairs.append((match.group(1), match.group(2) or match.group(1))) + return pairs, namespaces + + +def _js(directory: str, text: str, names: Sequence[str]) -> tuple[list[_Found], set[str]]: + wanted = set(names) + imports: list[_Found] = [] + external: set[str] = set() + for match in _JS_FROM_RE.finditer(text): + keyword, clause, specifier = match.group(1), match.group(2), match.group(4) + pairs, namespaces = _js_clause(clause) + files = _js_module_files(directory, specifier) + if not files: + if keyword == "import" and not specifier.startswith((".", "/")): + external.update(local for _, local in pairs) + continue + for original, local in pairs: + keys = (local,) if keyword == "import" else (original, local) + key = next((k for k in keys if k in wanted), None) + if key is None: + continue + target = local if original == "default" else original + imports.extend((key, target, f) for f in files) + if keyword != "import": + continue + for namespace in namespaces: + member_re = re.compile(rf"(? str: + dot = base.rfind(".") + stem = base[:dot] if dot > 0 else base + return stem[:-2] if stem.endswith(".d") else stem + + +def _snake_case(name: str) -> str: + return _SNAKE_RE.sub("_", name).lower() + + +def _shared_depth(left: str, right: str) -> int: + depth = 0 + for a, b in zip(left.split("/") if left else [], right.split("/") if right else [], strict=False): + if a != b: + break + depth += 1 + return depth + + +def _name_search( + source: str, language: str, names: Sequence[str], listing: Collection[str], skip: set[str] +) -> list[_Found]: + python = language == "python" + searched = [n for n in names if n not in skip and not (n.startswith("__") and n.endswith("__"))] + targets = {n.lower() for n in searched} + if python: + targets |= {_snake_case(n) for n in searched} + by_lower: dict[str, list[tuple[str, str]]] = {} + for raw in listing: + stem = _stem(raw.rsplit("/", 1)[-1]) + folded = stem.lower() + if folded not in targets or language_of(raw) != language: + continue + path = _normalize(raw) + if path is None or path == source: + continue + by_lower.setdefault(folded, []).append((stem, path)) + directory = posixpath.dirname(source) + + def ordered(paths: list[str]) -> list[str]: + return sorted(paths, key=lambda p: (-_shared_depth(posixpath.dirname(p), directory), p)) + + found: list[_Found] = [] + for name in searched: + folded = name.lower() + hits = by_lower.get(folded, []) + tiers = [ + [p for stem, p in hits if stem == name], + [p for stem, p in hits if stem != name], + ] + snake = _snake_case(name) + if python and snake != folded: + tiers.append([p for _, p in by_lower.get(snake, [])]) + picked: list[str] = [] + for tier in tiers: + for path in ordered(tier): + if path not in picked: + picked.append(path) + found.extend((name, name, p) for p in picked[:MAX_NAME_SEARCH_PER_NAME]) + return found + + +def resolve_candidates( + path: str, + text: str | None, + names: Sequence[str], + *, + listing: Collection[str] | None, + listing_complete: bool = False, +) -> list[Candidate]: + """Ordered candidate files that may define ``names``, as referenced from ``path``. + + ``path`` is the referencing file, repository-relative. ``text`` is its + content at the head, or the chunk's added lines joined with newlines when + there is no reader, so no ``package`` line or import is assumed present; + ``None`` means no imports are known. ``names`` comes from + :func:`prxref.repo_context.referenced_names`. ``listing`` holds the + repository's file paths, or is ``None``; ``listing_complete`` says the + listing was not truncated. + + Rules, by :func:`prxref.repo_context.language_of`: + + - Java. The source root is ``path``'s directory minus the package + directory of its first ``package`` line (``package com.acme.connectors;`` + in ``src/main/java/com/acme/connectors/X.java`` gives ``src/main/java/``); + with no package line, or a directory that does not end with the package + path, the root is unknown and no import resolves. Only an import sharing + the first two package segments with the file's own package counts: + ``import com.acme.x.T;`` gives ``com/acme/x/T.java`` when ``T`` is + referenced, and a wildcard ``import com.acme.x.*;`` gives + ``com/acme/x/.java`` for each referenced name no + single-name import binds. ``import static``, ``java.*``, ``javax.*`` and + other organizations' imports give nothing. A nested type is best effort: + the file is the first segment with an uppercase initial, so + ``a.b.Outer.Inner`` maps to ``a/b/Outer.java``. Then, as the + ``"path-convention"`` group, ``/.java`` for every name no + single-name import binds; a single-name import is any non-wildcard + import, static or not, of any organization, resolved or not. + - Python. ``from a.b import C`` (also parenthesized, aliased or ``*``) + gives ``a/b.py``, ``a/b/__init__.py``, ``src/a/b.py`` and + ``src/a/b/__init__.py``, in that order. Relative dots count from + ``path``'s directory (``.`` is that directory, each further dot one level + up), give ``/x.py`` and ``/x/__init__.py``, and for a bare + ``from . import C`` just ``/__init__.py``. ``import a.b`` gives + nothing (its names are attributes), and neither does a standard-library + module. + - TS/JS (``language_of`` reports ``"js"``). A relative specifier of + ``import {C} from``, ``import C from``, ``import * as N from`` (for each + referenced ``N.``) or ``export {C} from`` tries ``x.ts``, + ``x.tsx``, ``x.d.ts``, ``x.js`` and ``x/index.ts`` against ``path``'s + directory. A ``.js`` specifier (TypeScript ESM) tries the same stem as + ``.ts``, ``.tsx``, ``.d.ts`` and ``.js``; another script extension is + tried literally; an asset (``.css``, ``.json``, ``.svg`` and similar) + gives nothing. Bare package specifiers give nothing. + - Name search, every language, only when ``listing`` is not ``None``: + listing files of the same language whose stem (the basename less its + last extension and a ``.d`` before it) equals the name case-sensitively, + then case-insensitively, then, for Python, equals the snake_case form of + a CamelCase name. Within each tier the deepest directory shared with + ``path`` comes first, then the path. At most + :data:`MAX_NAME_SEARCH_PER_NAME` per name. Names bound by an import + judged external (another organization's Java import, a + standard-library Python module, a bare TS/JS package) and dunder names + are not searched. + + Candidates are ordered by group (imports, path conventions, name search), + then by the name's position in ``names``; within one name, imports keep + statement order and each rule's probe order above. Duplicates by + ``(name, path)`` keep the first, highest-ranked reason. ``path`` itself is + never a candidate. With a listing and ``listing_complete``, an import or + path-convention candidate the listing lacks is dropped. Paths are + normalized POSIX, and a candidate that would escape the repository root is + dropped. A language without definition regexes + (:func:`prxref.repo_context.definition_regexes`) returns ``[]``, since no + candidate could yield a definition. There is no error path: an + unresolvable name is simply absent. + """ + source = _normalize(path) + if source is None: + return [] + language = language_of(source) + if not definition_regexes(language): + return [] + ordered_names = list(dict.fromkeys(names)) + rank = {name: index for index, name in enumerate(ordered_names)} + directory = posixpath.dirname(source) + body = text or "" + conventions: list[_Found] = [] + if language == "java": + imports, conventions, external = _java(directory, body, ordered_names) + elif language == "python": + imports, external = _python(directory, body, ordered_names) + elif language == "js": + imports, external = _js(directory, body, ordered_names) + else: + imports, external = [], set() + listed: Collection[str] | None = None + if listing is not None and listing_complete: + listed = listing if isinstance(listing, (set, frozenset)) else frozenset(listing) + out: list[Candidate] = [] + seen: set[tuple[str, str]] = set() + + def admit(reason: str, found: list[_Found], *, filtered: bool) -> None: + for _, name, raw in sorted(found, key=lambda entry: rank[entry[0]]): + candidate = _normalize(raw) + if candidate is None or candidate == source: + continue + if filtered and listed is not None and candidate not in listed: + continue + if (name, candidate) in seen: + continue + seen.add((name, candidate)) + out.append(Candidate(name=name, path=candidate, reason=reason)) + + admit("import", imports, filtered=True) + admit("path-convention", conventions, filtered=True) + if listing is not None: + admit("name-search", _name_search(source, language, ordered_names, listing, external), filtered=False) + return out diff --git a/src/prxref/repo_unit.py b/src/prxref/repo_unit.py new file mode 100644 index 0000000..374a91f --- /dev/null +++ b/src/prxref/repo_unit.py @@ -0,0 +1,266 @@ +"""One worker chunk's repository context: sources, order, budget and exclusion (#17). + +:func:`build_unit_context` combines the three repository-context sources for +one chunk into the lines its prompt carries: + +1. :func:`prxref.repo_crosschunk.diff_definitions`: definitions from the PR's + own diff files (``cross-chunk`` and ``diff-file``), at the ``diff`` and + ``repo`` levels; +2. :func:`prxref.repo_contracts.contract_entries`: contract excerpts + (``contract``), at the ``repo`` level only; +3. the resolver, :func:`prxref.repo_resolve.resolve_candidates` plus + :func:`prxref.repo_context.find_definitions` over the candidate files + (``import``, ``path-convention``, ``name-search``), at the ``repo`` level + only. + +The sources are called in that order, which is their admission rank, so under +a capped reader the higher-ranked sources get their reads first. Every read +goes through one guard that refuses an excluded path without calling the +reader. The entries are then filtered by the exclude predicate, ordered by +``(REASONS rank, path, line)``, deduplicated on ``(path, line)`` and admitted +against one character budget, which stops at the first entry that does not +fit. + +The module is pure: stdlib plus the pure repository-context modules and +:mod:`prxref.chunk_context`, with no I/O except through the ``read`` callable +the caller passes, nothing from :mod:`prxref.forges`, and no threads. The same +inputs and the same reader answers give the same result and the same read +order. +""" +from __future__ import annotations + +from collections.abc import Callable, Collection, Sequence +from dataclasses import dataclass + +from .chunk_context import chunk_files +from .repo_context import ( + REASONS, + ContextEntry, + definition_regexes, + find_definitions, + language_of, + referenced_names, +) +from .repo_contracts import contract_entries +from .repo_crosschunk import diff_definitions +from .repo_resolve import resolve_candidates + +MODES = ("off", "diff", "repo") + + +@dataclass(frozen=True) +class UnitContext: + """The repository context admitted into one worker chunk's prompt. + + ``definition_lines`` and ``contract_lines`` are the rendered admitted + entries of each kind, in admission order, ready for + :func:`prxref.chunk_context.render_context_blocks` as ``extra_def_lines`` + and ``contract_lines``; when entries were left out, one omitted line (a + horizontal ellipsis, then ``N more context entries omitted``) ends the + list of the kind of the first entry left out. ``entries`` holds the admitted entries in + admission order, and ``omitted`` counts the entries the budget left out. + """ + + definition_lines: tuple[str, ...] + contract_lines: tuple[str, ...] + entries: tuple[ContextEntry, ...] + omitted: int + + def record(self) -> dict: + """The run-record row for this chunk: ``{"entries": [...], "omitted": N}``.""" + return {"entries": [entry.record() for entry in self.entries], "omitted": self.omitted} + + +EMPTY_UNIT = UnitContext((), (), (), 0) + + +def _excluded(exclude: Callable[[str], bool] | None, path: str) -> bool: + if exclude is None: + return False + try: + return bool(exclude(path)) + except Exception: # noqa: BLE001 + return True + + +def _guard( + read: Callable[[str], str | None] | None, + exclude: Callable[[str], bool] | None, +) -> Callable[[str], str | None] | None: + if read is None: + return None + + def guarded(path: str) -> str | None: + if _excluded(exclude, path): + return None + return read(path) + + return guarded + + +def _resolver_entries( + chunk: Sequence[object], + all_files: Sequence[object], + read: Callable[[str], str | None], + *, + listing_paths: Collection[str] | None, + listing_complete: bool, + exclude: Callable[[str], bool] | None, + found: Collection[str], +) -> list[ContextEntry]: + known = set(found) + removed = {getattr(f, "path", "") for f in chunk if getattr(f, "status", "") == "removed"} + diff_paths = {getattr(f, "path", "") for f in all_files} + out: list[ContextEntry] = [] + for changed in chunk_files(chunk): + if changed.path in removed: + continue + language = language_of(changed.path) + if not definition_regexes(language): + continue + names = [name for name in referenced_names(changed.added, language) if name not in known] + if not names: + continue + text = read(changed.path) + if not isinstance(text, str): + text = "\n".join(changed.added) + candidates = resolve_candidates( + changed.path, text, names, listing=listing_paths, listing_complete=listing_complete + ) + by_path: dict[str, dict[str, str]] = {} + for candidate in candidates: + by_path.setdefault(candidate.path, {}).setdefault(candidate.name, candidate.reason) + for path, reasons in by_path.items(): + if path in diff_paths or _excluded(exclude, path): + continue + wanted = [name for name in reasons if name not in known] + if not wanted: + continue + target = read(path) + if not isinstance(target, str): + continue + for symbol, line, body in find_definitions(target, wanted, language=language_of(path)): + out.append(ContextEntry(path, line, symbol, "definition", reasons[symbol], body)) + known.add(symbol) + return out + + +def _merge(entries: list[ContextEntry], exclude: Callable[[str], bool] | None) -> list[ContextEntry]: + kept = [entry for entry in entries if not _excluded(exclude, entry.path)] + kept.sort(key=lambda entry: (REASONS.index(entry.reason), entry.path, entry.line)) + out: list[ContextEntry] = [] + seen: set[tuple[str, int]] = set() + for entry in kept: + key = (entry.path, entry.line) + if key not in seen: + seen.add(key) + out.append(entry) + return out + + +def _admit(entries: list[ContextEntry], max_chars: int) -> UnitContext: + admitted: list[ContextEntry] = [] + used = 0 + for entry in entries: + size = len(entry.rendered()) + if used + size > max_chars: + break + admitted.append(entry) + used += size + omitted = len(entries) - len(admitted) + definition_lines = [entry.rendered() for entry in admitted if entry.kind == "definition"] + contract_lines = [entry.rendered() for entry in admitted if entry.kind != "definition"] + if omitted: + marker = f"\N{HORIZONTAL ELLIPSIS} {omitted} more context entries omitted" + if entries[len(admitted)].kind == "definition": + definition_lines.append(marker) + else: + contract_lines.append(marker) + return UnitContext(tuple(definition_lines), tuple(contract_lines), tuple(admitted), omitted) + + +def build_unit_context( + chunk: Sequence[object], + all_files: Sequence[object], + *, + mode: str, + read: Callable[[str], str | None] | None, + max_chars: int, + listing_paths: Collection[str] | None = None, + listing_complete: bool = False, + contract_paths: Sequence[str] = (), + contract_priority: Sequence[str] = (), + exclude: Callable[[str], bool] | None = None, +) -> UnitContext: + """The repository context for one worker chunk at level ``mode``. + + ``chunk`` holds the chunk's ``triage.FileDiff`` records and ``all_files`` + the whole PR's, duck-typed as :func:`prxref.repo_crosschunk.diff_definitions` + reads them. ``mode`` is ``"off"``, ``"diff"`` or ``"repo"``; any other + value raises ``ValueError``, and ``"off"`` returns :data:`EMPTY_UNIT` + without calling anything. ``read`` is the chunk's capped reader, or None + when there is none. ``listing_paths`` is the run's path listing as a set + built once per run (or None), and ``listing_complete`` says it was not + truncated. ``contract_paths`` and ``contract_priority`` are the run's + :func:`prxref.repo_contracts.select_contract_files` and + :func:`prxref.repo_contracts.literal_contract_paths` results. + ``exclude(path)`` true marks a path repository context must never read or + show, normally :func:`prxref.repo_context.exclude_predicate`; an + ``exclude`` that raises counts as true. + + Every read goes through a guard that returns None for an excluded path + without calling ``read``. The sources, called in this order: + + 1. :func:`prxref.repo_crosschunk.diff_definitions` with the guarded + reader, at both levels; + 2. at ``"repo"`` with a reader, :func:`prxref.repo_contracts.contract_entries`; + 3. at ``"repo"`` with a reader, the resolver. For each chunk file that is + not removed and whose language has definition regexes, the names its + added lines reference, less every symbol a definition entry at a + non-excluded path has already found, go to + :func:`prxref.repo_resolve.resolve_candidates` with the file's text + (its added lines when the read gives None). The candidates' distinct + paths are walked in first-appearance order; a path that is a PR diff + file or excluded is skipped, and so is one whose candidate names are + all found already, with no read. Each other path is read once, and + :func:`prxref.repo_context.find_definitions` over it gives a + ``definition`` entry whose reason is that name's candidate reason + there; its symbol then counts as found. + + ``"repo"`` with ``read`` None is exactly ``"diff"`` with ``read`` None. + + Entries at excluded paths are dropped (this covers the hunk-derived + entries of an excluded diff file), the rest are ordered by ``(REASONS + rank, path, line)`` and deduplicated on ``(path, line)``, the first kept. + They are admitted in that order while the sum of their rendered lengths + stays within ``max_chars``; admission stops at the first entry that does + not fit, even when a later one would. The omitted line does not count + toward ``max_chars``. + """ + if mode not in MODES: + raise ValueError(f"repository context mode must be one of {MODES}, got {mode!r}") + if mode == "off": + return EMPTY_UNIT + guarded = _guard(read, exclude) + entries = list(diff_definitions(chunk, all_files, guarded)) + if mode == "repo" and guarded is not None: + found = { + entry.symbol + for entry in entries + if entry.kind == "definition" and not _excluded(exclude, entry.path) + } + entries.extend( + contract_entries(chunk, contract_paths=contract_paths, read=guarded, priority=contract_priority) + ) + entries.extend( + _resolver_entries( + chunk, + all_files, + guarded, + listing_paths=listing_paths, + listing_complete=listing_complete, + exclude=exclude, + found=found, + ) + ) + return _admit(_merge(entries, exclude), max_chars) diff --git a/src/prxref/reviewer.py b/src/prxref/reviewer.py index 4898684..2b9763a 100644 --- a/src/prxref/reviewer.py +++ b/src/prxref/reviewer.py @@ -1,8 +1,9 @@ """Worker-review layer: one LLM call per diff chunk, plus the systemic sweep. The reviewer renders ``prompts/worker.md`` with the chunk's unified diff, -makes a single :meth:`LLMClient.invoke` call (no retries — the model -fallback chain handles transient failures), and maps the JSON response +makes a single :meth:`LLMClient.invoke` call (the model fallback chain +handles transient failures; the one retry here is for a reply that came +back empty, made once with the same prompt), and maps the JSON response onto :class:`prxref.triage.Finding` records. Unparseable or malformed responses degrade to ``([], [])`` with a logged warning; this layer never raises. :func:`review_systemic` is the same contract over the @@ -60,7 +61,7 @@ from typing import Any from .chunk_context import sibling_summary_block -from .costs import valid_usd +from .costs import combine_reported, valid_usd from .forges.base import Thread from .llm import LLMClient from .parser import loads_lenient @@ -156,6 +157,55 @@ def _budget_stop_reason(result: Any) -> str: return literal if literal.lower() in _TRUNCATION_FINISH_REASONS else "" +def _is_empty_reply(result: Any) -> bool: + """True when a completion came back with no text: empty or whitespace only. + + Only a string is judged. A backend or test double whose ``text`` is not a + string keeps the path it always had, where the parse names what it got. + """ + text = getattr(result, "text", None) + return isinstance(text, str) and not text.strip() + + +def _finish_reason_text(result: Any) -> str: + """The provider's stop reason for a log line, stripped; ``-`` when none was reported.""" + reason = getattr(result, "finish_reason", "") + if not isinstance(reason, str) or not reason.strip(): + return "-" + return reason.strip() + + +def _result_cost(result: Any) -> tuple[float | None, str]: + """``(cost_usd, cost_source)`` as the backend reported them for one call. + + The figure goes through :func:`prxref.costs.valid_usd`; the source is + ``""`` whenever the figure is ``None``. + """ + cost = valid_usd(getattr(result, "cost_usd", None)) + source = str(getattr(result, "cost_source", "") or "") if cost is not None else "" + return cost, source + + +def _fold_retry_usage(meta: dict, result: Any) -> None: + """Add the empty-reply retry's usage to ``meta``, which holds the first call's. + + Both calls were billed, so ``input_tokens`` and ``output_tokens`` are + summed. ``model`` becomes the retry's, the reply the unit went on to use. + The reported cost folds through :func:`prxref.costs.combine_reported`, + the rule a backend applies to the attempts inside one invoke: the sum + when both calls reported a figure, else ``None`` with source ``""``. A + ``None`` leaves the unit to :func:`prxref.costs.run_cost`, which prices it + from the summed tokens when a price table knows the model. + """ + meta["input_tokens"] = (meta["input_tokens"] or 0) + (result.input_tokens or 0) + meta["output_tokens"] = (meta["output_tokens"] or 0) + (result.output_tokens or 0) + meta["model"] = result.model + meta["cost_usd"], meta["cost_source"] = combine_reported([ + (meta["cost_usd"], meta["cost_source"]), + _result_cost(result), + ]) + + def load_prompt(name: str) -> str: """Load a prompt template from the packaged ``prxref/prompts`` directory.""" fname = f"{name}.md" if not name.endswith(".md") else name @@ -458,6 +508,10 @@ def _write_trace_files( Each file lands via a temp file plus :func:`os.replace`, so a concurrent reader never observes a half-written file, and a timeout retry simply overwrites: the trace ends up showing the attempt whose result was used. + The empty-reply retry in :func:`_invoke_and_parse` writes only once, + after its final call, so the files describe that call (the meta's token + counts cover both calls) and the empty first reply is visible only in its + WARNING log line. Empty ``trace_dir`` is the declared off switch and does nothing — no directory, no syscalls, no cost. Every failure (directory cannot be created, path unwritable, disk full) is a logged warning and nothing more: @@ -518,6 +572,24 @@ def _invoke_and_parse( ``accept_rule`` does the same for ``rule`` (through :func:`prxref.triage.normalize_rule`); false, the default, leaves every finding's ``rule`` at ``None``. + + A reply that came back EMPTY (no text, or whitespace only) is asked for + again, once, with the same prompt and the same budget, after one WARNING + ``