feat(engine): add a spec lens that checks the PR against its committed spec - #376
Conversation
…d spec Repositories that drive work from a committed specification (OpenSpec, GitHub Spec Kit, Kiro) leave the spec sitting next to the diff, and nobody checks the two against each other. The reviewer reads the code; the spec is read once, by the agent that wrote the code, and never audited again. The spec lens is the intent lens pointed at a better source of truth. It reports both directions: the diff falling short of the spec (contradicted requirements, ticked tasks that were not delivered, criteria with no test) and the spec falling short of the diff (behaviour no requirement covers, a stale requirement, an unresolved [NEEDS CLARIFICATION]) — the second being the more reliable half, since its evidence is entirely in the diff. Detection is a pure filesystem probe and selection is deterministic: the PR edits the spec, the branch names it, or the stated intent does. Nothing detected or nothing matched drops the lens before the fan-out, so a repository without specs pays no call and no prompt bytes. When it does fire it is a lens of its own — a fifth call under the fast preset — because its block is large and correctness already carries the intent block there. The highest-precision signal costs nothing: all three systems track progress with markdown checkboxes, and a PR that implements tasks flips them. That `- [ ]` → `- [x]` pair is already in the diff, so the author's delivery claims are extracted with no model call and handed to the lens as claims to verify. Trust and visibility follow the diff's posture, not a config file's: - Spec text is redacted and wrapped in its own neutralised SPEC block. A spec is usually committed in the PR that implements it, so on a fork the author controls the requirements their own change is judged against; the worst case is a suppressed spec finding, and no other lens sees the block. - Files the PR changes are read from its head text (the base branch does not have them yet); everything else comes from the workspace, which on pull_request_target is the trusted base branch. - Each call is told which of the PR's files it cannot see. A requirement is delivered by code, so without this the lens reports every requirement implemented in another batch as undelivered. - reflect.py's gap-finding carve-out now names spec mismatches, or the auditor prunes them all as cross-file absence claims. LLMReviewEngine takes an injectable workspace_root (default Path.cwd()), so the eval harness roots a review at its fixture instead of the operator's working directory — which also stops a host repository's own specs leaking into a fixture run. Adds the spec-delivery eval fixture, ReviewConfig.spec_review / spec_paths, CLI --spec/--no-spec, and the spec_review Action input. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013pq1xMbeKhX9AW3sLsgkrt
📝 WalkthroughWalkthroughAdds a default-enabled, detection-gated specification-review lens. It supports OpenSpec, Spec Kit, Kiro, and custom paths; selects up to two matching specs; extracts completed task claims; redacts and isolates spec content; and integrates CLI, Action, prompt, reflection, and evaluation support. ChangesSpecification review
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ReviewConfig
participant LLMReviewEngine
participant SpecSystem
participant ReviewProvider
ReviewConfig->>LLMReviewEngine: enable specification review
LLMReviewEngine->>SpecSystem: detect and select matching specifications
SpecSystem-->>LLMReviewEngine: redacted specification block and task claims
LLMReviewEngine->>ReviewProvider: execute separate specification lens call
ReviewProvider-->>LLMReviewEngine: specification findings
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![lgtmaybe[bot]](https://avatars.githubusercontent.com/in/3987976?s=60&v=4){kind=small}
There was a problem hiding this comment.
💬 3 earlier lgtmaybe conversations are still unresolved on this PR — this run's count covers what it reviewed now, not those.
2 findings · provider openrouter · model openai/gpt-5.6-luna · lgtmaybe 1.13.1
Incremental review of the changes since 760cfa9 — earlier findings stay open until fixed.
| """ | ||
| if not cfg.spec_review: | ||
| return None | ||
| bundles = specs.detect(root, cfg.spec_paths) |
There was a problem hiding this comment.
[MEDIUM · correctness · 100%] New PR-only specs are never detected
Spec detection runs exclusively against root, which is the checked-out workspace/base branch. If a PR adds a new spec directory that does not exist on the base branch, specs.detect returns no bundles before ctx.file_contents can be used, so the promised spec lens is skipped even though the PR commits the specification it implements.
| def _score(bundle: SpecBundle, changed_files: Sequence[str], branch: str, intent_text: str) -> int: | ||
| score = 0 | ||
| prefix = f"{bundle.root}/" | ||
| if any(path.startswith(prefix) for path in changed_files): |
There was a problem hiding this comment.
[LOW · performance · 80%] Spec selection rescans all changed files for every candidate
Selection performs a linear scan of changed_files for each detected spec bundle. In a monorepo with many spec directories and a large PR, this makes the matching phase O(specs × changed_files), even though the path-prefix evidence can be indexed or grouped once. This work runs before every review that has detected specs.
There was a problem hiding this comment.
Not acting on this one — the complexity isn't worth buying back.
Selection runs once per review, over bundles that are already capped by what detection finds. The realistic worst case is a monorepo with a few dozen spec directories and a large PR: 40 × 500 = 20,000 str.startswith calls, which is well under a millisecond. It sits next to a stage that does real filesystem I/O and, when it fires at all, a model call that costs seconds.
Indexing the changed paths by prefix would mean building and maintaining a directory-prefix structure to save time that isn't measurable, on the path that decides whether to spend a model call. That's the trade the ponytail lens exists to flag.
Happy to revisit if a --profile run ever shows the spec_context stage as anything but noise.
Generated by Claude Code
There was a problem hiding this comment.
{"findings": []}
| assert "docstring" in keywords or "stale" in keywords, "no documentation finding" | ||
|
|
||
|
|
||
| def test_spec_delivery_fixture_can_score_the_spec_lens() -> None: |
There was a problem hiding this comment.
[LOW · tests · 100%] Fixture test does not execute the spec lens
This test only verifies that the fixture contains expected keywords and corpus files; it never runs the review engine or checks that the spec lens produces the three expected findings. A regression that disables spec detection, selection, prompt delivery, or scoring would still pass this test. Add an assertion against the actual fixture review results, analogous to the other fixture-scoring tests.
Detection-gated committed spec reviewStructureflowchart LR
n0["Review configuration<br/>Pydantic / CLI / Action<br/>enables spec_review and custom spec paths<br/>(changed)"]
n1["PR context<br/>GitHub REST / local Git<br/>provides changed files, head text, intent, and branch<br/>(changed)"]
n2["Spec resolver<br/>Python filesystem probe<br/>detects, selects, loads, redacts, and extracts task claims<br/>(new)"]
n3["Review engine<br/>Python<br/>gates the spec lens and fans out review calls<br/>(changed)"]
n4["Spec lens prompt<br/>LLM prompt templates<br/>judges delivery gaps and uncovered behavior<br/>(new)"]
n5["LLM provider<br/>Provider API<br/>returns structured review findings"]
n0 -->|"configures lens"| n3
n1 -->|"supplies PR data"| n2
n2 -->|"resolves spec"| n3
n3 -->|"builds lens"| n4
n4 -->|"submits review"| n5
n3 -->|"fans out calls"| n5
Text versionSequencesequenceDiagram
participant n1 as PR context (changed)
participant n2 as Spec resolver (new)
participant n3 as Review engine (changed)
participant n4 as Spec lens prompt (new)
participant n5 as LLM provider
n1->>n2: provides PR metadata
n2->>n2: detects matching specs
n2->>n2: loads and redacts text
n2-->>n3: returns spec block
n3->>n3: adds gated lens
n3->>n4: wraps spec block
n4->>n5: runs spec review
n5-->>n3: returns findings
Text versionChanged components are marked in the nodes; the spec lens is skipped when disabled, undetected, unmatched, or unreadable. Workspace-root injection also affects directory-context and evaluation isolation, but those collaborators are omitted for compactness. |
Detection walked the workspace only, which on pull_request_target is the base branch. A PR that ADDS a spec directory therefore found no bundles and the lens was skipped — in exactly the case it exists for, since spec-driven work commits the spec in the pull request that implements it. Head text fixed the content of a spec the PR changes but never its discovery, and the workaround was visible in the test that wrote the spec tree to the workspace first just to get detection to fire. Detection now runs over the workspace UNION the PR's own changed paths, via a small _Tree that answers existence questions from both. Adding the paths there rather than in a second detection pass keeps ONE set of layout rules: the `specs/` spec.md+plan.md guard, the openspec archive exclusion, and the reading order cannot drift between a pre-existing spec and one this PR introduces. It is existence only — no content is read and no path is resolved against the filesystem, so a hostile path still cannot escape the root, and an ordinary `docs/design.md` still invents no spec. Also adds the end-to-end fixture test that was missing: the structural checks could not see a broken chain, so a renamed fixture directory or a mis-wired workspace root would leave spec-delivery silently scoring a lens that never ran. It runs the real engine over the real fixture with a fake provider and asserts the spec block reaches exactly one call carrying both the requirements and the ticked-task claims. Both raised by lgtmaybe on its own PR (#376). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013pq1xMbeKhX9AW3sLsgkrt
| """ | ||
| if not cfg.spec_review: | ||
| return None | ||
| bundles = specs.detect(root, cfg.spec_paths, ctx.changed_files) |
There was a problem hiding this comment.
[MEDIUM · correctness · 90%] Deleted spec files are treated as present
ctx.changed_files generally represents all changed paths, not only additions. Passing that collection directly into detection makes a deleted known spec file satisfy tree.is_file(), so a PR that removes a spec can still produce a bundle for it and review the stale base-branch contents. Detection should distinguish paths present in the PR head from deletions, or receive only added/modified paths that exist in the head.
|
|
||
|
|
||
| def detect( | ||
| root: Path, extra_paths: Sequence[str] = (), changed_files: Sequence[str] = () |
There was a problem hiding this comment.
[LOW · documentation · 90%] Update detect documentation for changed-file input
The public detect API now builds a virtual tree from both the workspace and changed_files, but its docstring still describes detection as a “pure filesystem probe.” That description is stale and can mislead callers about why the new parameter is required and what paths it considers.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
tests/engine/test_preset.py (1)
106-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis call-count assertion now depends on the process working directory.
LLMReviewEnginedefaultsworkspace_roottoPath.cwd(), and_resolve_specprobes it. This repository commits its ownopenspec/tree, so when pytest runs from the repository root the probe detects real bundles. The assertion holds today only because several bundles exist and none matches_CTX, which makesselectreturn an empty list. If the repository ever holds exactly one spec directory, the single-bundle fallback inspecs.selectselects it, a fifth lens is built, and this test fails for a reason unrelated to presets.
evals/run.pyalready guards against the same leak by pinningworkspace_rootto the fixture tree. Pin it here too.♻️ Proposed isolation
`@pytest.mark.parametrize`("provider", [Provider.ollama, Provider.openai]) - def test_fast_review_makes_four_calls_on_any_provider(self, provider: Provider) -> None: + def test_fast_review_makes_four_calls_on_any_provider( + self, provider: Provider, tmp_path: Path + ) -> None: """Worker count changes how the four calls are scheduled, never how many there are — a single-slot provider runs the same four, serially.""" fake = FakeProvider() - LLMReviewEngine(fake).review(_CTX, make_cfg(provider=provider)) + # An empty workspace, so the repo's own `openspec/` tree can never leak + # a fifth spec call into a preset assertion. + LLMReviewEngine(fake, workspace_root=tmp_path).review(_CTX, make_cfg(provider=provider)) assert len(fake.calls) == 4The same reasoning applies to
tests/engine/test_engine.pyaround line 720, where the comment states that no specification is detected in the workspace.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/engine/test_preset.py` around lines 106 - 112, Pin LLMReviewEngine’s workspace_root to the isolated fixture tree in test_fast_review_makes_four_calls_on_any_provider instead of allowing Path.cwd() to influence _resolve_spec. Apply the same workspace_root isolation to the engine test that verifies no specification is detected, preserving its existing assertion behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@action.yml`:
- Around line 109-112: Update the spec_review input description in action.yml to
include the GitHub Spec Kit .specify/ layout alongside the existing openspec/
and specs/NNN-slug/ layouts, without changing the input’s default or behavior.
In `@docs/explanation/what-gets-reviewed.md`:
- Around line 319-322: Update the code block around the T014 checklist diff in
the reviewed documentation to satisfy the configured MD046 indented style, or
adjust the markdownlint configuration if fenced diff blocks with language hints
are intentionally required; preserve the displayed before-and-after checklist
content and confirm the project’s preferred syntax-highlighting behavior.
In `@docs/llms-full.txt`:
- Around line 5556-5639: Update the source documentation and regenerate
docs/llms-full.txt so all references reflect the spec lens contract: include
spec_review in the Action inputs table, change the default category count from
nine to the current count including spec review, and update the default category
list consistently. Locate the related category-count, default-list, and
Action-input documentation symbols/sections rather than changing only the shown
spec-lens section.
In `@src/lgtmaybe/engine/specs.py`:
- Around line 147-175: Update _Tree.dirs_in and _Tree.glob_dirs in
src/lgtmaybe/engine/specs.py (lines 147-175) to catch OSError, ValueError, and
NotImplementedError from directory iteration or globbing and return no matches.
Add a defensive boundary in _resolve_spec in src/lgtmaybe/engine/engine.py
(lines 1690-1735) that logs probe failures and returns None so the spec lens is
skipped. Add coverage in tests/engine/test_specs.py for an absolute or
unreadable spec_paths pattern.
- Around line 163-175: Update glob_dirs to materialize self._root.glob(pattern)
inside a try/except, catching NotImplementedError for non-relative patterns and
treating it as no filesystem matches; then iterate over the collected matches
while preserving the existing directory and relative-path handling.
In `@tests/engine/test_engine.py`:
- Around line 728-730: Update the test around _review_calls(provider) to make
specification detection deterministic by using an empty tmp_path workspace or
explicitly disabling spec_review in the LLMReviewEngine configuration. Revise
the comment to say “no matching specification,” while preserving the expected
category count adjustment.
---
Nitpick comments:
In `@tests/engine/test_preset.py`:
- Around line 106-112: Pin LLMReviewEngine’s workspace_root to the isolated
fixture tree in test_fast_review_makes_four_calls_on_any_provider instead of
allowing Path.cwd() to influence _resolve_spec. Apply the same workspace_root
isolation to the engine test that verifies no specification is detected,
preserving its existing assertion behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 90cc7fda-5b14-4ba7-8197-3f28135f05cf
📒 Files selected for processing (34)
CLAUDE.mdaction.ymldocs/explanation/what-gets-reviewed.mddocs/llms-full.txtdocs/reference/config.mdevals/fixtures/spec-delivery/diff.txtevals/fixtures/spec-delivery/expected.jsonevals/fixtures/spec-delivery/repo/.kiro/specs/link-expiry/requirements.mdevals/fixtures/spec-delivery/repo/.kiro/specs/link-expiry/tasks.mdevals/fixtures/spec-delivery/repo/src/links/models.pyevals/fixtures/spec-delivery/repo/src/links/repo.pyevals/run.pyevals/scorer.pyopenspec/specs/prompt-and-lenses/anchors.ymlopenspec/specs/prompt-and-lenses/spec.mdsrc/lgtmaybe/cli/__init__.pysrc/lgtmaybe/cli/commands.pysrc/lgtmaybe/core/models.pysrc/lgtmaybe/engine/engine.pysrc/lgtmaybe/engine/injection.pysrc/lgtmaybe/engine/prompt.pysrc/lgtmaybe/engine/reflect.pysrc/lgtmaybe/engine/specs.pysrc/lgtmaybe/github/rest_gateway.pysrc/lgtmaybe/local/__init__.pytests/engine/test_engine.pytests/engine/test_injection.pytests/engine/test_preset.pytests/engine/test_prompt.pytests/engine/test_spec_lens.pytests/engine/test_specs.pytests/evals/test_fixtures.pytests/snapshots/PRContext.jsontests/snapshots/ReviewConfig.json
| spec_review: | ||
| description: "Check the diff against a specification the repository commits (OpenSpec `openspec/`, GitHub Spec Kit `specs/NNN-slug/`, Kiro `.kiro/specs/`): requirements the change falls short of, task-list entries it ticks off without doing, and behaviour no requirement covers. On by default, but gated on detection — no spec in the repo, or none matching this PR, and the lens never runs, so a repository without specs pays no extra call. Set false to disable." | ||
| required: false | ||
| default: "" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the .specify/ layout in the Action help.
GitHub Spec Kit also supports the .specify/ layout, but this description lists only specs/NNN-slug/. Add .specify/ so users do not mistake the supported layout set.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@action.yml` around lines 109 - 112, Update the spec_review input description
in action.yml to include the GitHub Spec Kit .specify/ layout alongside the
existing openspec/ and specs/NNN-slug/ layouts, without changing the input’s
default or behavior.
| ```diff | ||
| -- [ ] T014 [US1] Enforce the 30-day link expiry in src/links/service.py | ||
| +- [x] T014 [US1] Enforce the 30-day link expiry in src/links/service.py | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
markdownlint reports MD046 on this code block.
The configured code-block style is indented, and this block is fenced. Convert it to the indented style, or set the block style in the markdownlint configuration if fenced blocks with a language hint are wanted in this file. A fenced diff block loses its syntax highlighting when indented, so confirm which outcome the project wants before changing it.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 319-319: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/explanation/what-gets-reviewed.md` around lines 319 - 322, Update the
code block around the T014 checklist diff in the reviewed documentation to
satisfy the configured MD046 indented style, or adjust the markdownlint
configuration if fenced diff blocks with language hints are intentionally
required; preserve the displayed before-and-after checklist content and confirm
the project’s preferred syntax-highlighting behavior.
Source: Linters/SAST tools
| ## Spec — does the PR deliver the specification it commits to? | ||
|
|
||
| If your repository drives its work from a committed specification, that spec is | ||
| a far better statement of intent than a PR description: it is structured, it | ||
| predates the code, and its task list records what the author claims to have | ||
| finished. The spec lens checks the diff against it — in **both** directions. | ||
|
|
||
| lgtmaybe recognises three layouts out of the box, plus your own: | ||
|
|
||
| | Workflow | Detected by | Read | | ||
| |---|---|---| | ||
| | [OpenSpec](https://github.com/Fission-AI/OpenSpec) | `openspec/changes/<id>/`, `openspec/specs/<capability>/` | proposal, delta specs, design, tasks (archived changes are ignored) | | ||
| | [GitHub Spec Kit](https://github.com/github/spec-kit) | `.specify/`, or `specs/<slug>/` with a `spec.md` **and** a `plan.md` | spec, plan, tasks | | ||
| | [Kiro](https://kiro.dev/docs/specs/) | `.kiro/specs/<feature>/` | requirements, design, tasks | | ||
| | Your own layout | `spec_paths` globs in `.lgtmaybe.yml` | whichever of those filenames are present | | ||
|
|
||
| ### What it reports | ||
|
|
||
| **The diff falling short of the spec** | ||
|
|
||
| - **Contradicts an explicit requirement** — the code does the opposite of a | ||
| stated SHALL/MUST or acceptance criterion (`high`). | ||
| - **A ticked task that is not delivered** (`medium`) — see below. | ||
| - **A requirement in scope with nothing implementing it** (`medium`). | ||
| - **An acceptance criterion with no test** (`low`). | ||
|
|
||
| **The spec falling short of the diff** — the half people miss, and the most | ||
| reliable of the two, because the evidence is entirely in the diff: | ||
|
|
||
| - **Behaviour no requirement covers** — a new endpoint, state, error path, limit | ||
| or side effect the spec never mentions (`low`/`info`). | ||
| - **A requirement the change made stale** (`low`). | ||
| - **An unresolved `[NEEDS CLARIFICATION]`** still sitting in a requirement this | ||
| PR implements (`info`). | ||
|
|
||
| ### Ticked checkboxes are claims | ||
|
|
||
| All three workflows track progress with markdown checkboxes, and a PR that | ||
| implements tasks *flips* them: | ||
|
|
||
| ```diff | ||
| -- [ ] T014 [US1] Enforce the 30-day link expiry in src/links/service.py | ||
| +- [x] T014 [US1] Enforce the 30-day link expiry in src/links/service.py | ||
| ``` | ||
|
|
||
| That flip is already in the diff. lgtmaybe extracts it with no model call and no | ||
| extra file read, and hands the lens the resulting list as *claims the author made | ||
| in this pull request* — turning a vague question ("did this deliver the spec?") | ||
| into a precise one ("is T014 actually here?"). A ticked task is something to | ||
| **check**, never to assume false: the lens flags it only when the diff positively | ||
| shows the work is absent. | ||
|
|
||
| ### Which spec, and when it stays quiet | ||
|
|
||
| A monorepo can hold forty spec directories, so lgtmaybe ranks them against the | ||
| PR — it edits the spec, its branch is named after one (Spec Kit names branches | ||
| after the spec directory), or its title, description or commits name one — and | ||
| sends at most two. **When nothing matches, the lens does not run at all**: no | ||
| model call, no prompt bytes. The same is true when no spec system is present, | ||
| which is the common case, so a repository without specs pays nothing for this. | ||
|
|
||
| Because it needs its own large block, the spec lens is a call of its own — a | ||
| fifth one under the default `fast` preset, and only in repositories where a spec | ||
| actually matched. Turn it off with `--no-spec`, `spec_review: false` in | ||
| `.lgtmaybe.yml`, or `spec_review: false` on the Action. | ||
|
|
||
| ### The lens is told what it was not shown | ||
|
|
||
| A requirement is delivered by *code*, so this lens is even more exposed than the | ||
| intent lens to the filtered-diff trap: told nothing, it reports every requirement | ||
| implemented in another batch as undelivered. It gets the same correction — the | ||
| list of the PR's files this call cannot see, and the rule that a requirement | ||
| delivered in one of them is **not shown, not undelivered**. | ||
|
|
||
| Spec text is treated exactly like the diff: redacted, wrapped as untrusted data | ||
| in its own neutralised block, and never obeyed. That is deliberate rather than | ||
| paranoid — a spec is usually committed in the same PR that implements it, so on | ||
| a fork PR the author controls the requirements their own change is judged | ||
| against. Files the PR changes are read from its head text for that same reason | ||
| (the base branch does not have them yet); everything else comes from the | ||
| checked-out workspace, which on `pull_request_target` is the trusted base | ||
| branch. The worst a planted spec can do is suppress a spec finding — no other | ||
| lens ever sees the block. | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Synchronize the existing category and Action-input references.
This section states that a matched specification adds a fifth fast call and that spec_review is an Action setting. The same document still describes the default as nine categories and omits spec_review from the Action inputs table near Line 577. Update the source documentation and regenerate this corpus so the category counts, default list, and Action input table agree with the new contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/llms-full.txt` around lines 5556 - 5639, Update the source documentation
and regenerate docs/llms-full.txt so all references reflect the spec lens
contract: include spec_review in the Action inputs table, change the default
category count from nine to the current count including spec review, and update
the default category list consistently. Locate the related category-count,
default-list, and Action-input documentation symbols/sections rather than
changing only the shown spec-lens section.
| def dirs_in(self, rel: str) -> list[str]: | ||
| """Immediate subdirectory names of *rel*, sorted, from disk and the PR.""" | ||
| names = { | ||
| PurePosixPath(d).name for d in self._added_dirs if str(PurePosixPath(d).parent) == rel | ||
| } | ||
| parent = self._root / rel | ||
| if parent.is_dir(): | ||
| names |= {child.name for child in parent.iterdir() if child.is_dir()} | ||
| return sorted(names) | ||
|
|
||
| def spec_files_in(self, rel_root: str) -> tuple[str, ...]: | ||
| """The known spec files directly inside *rel_root*, in reading order.""" | ||
| return tuple( | ||
| f"{rel_root}/{name}" for name in _SPEC_FILE_ORDER if self.is_file(f"{rel_root}/{name}") | ||
| ) | ||
|
|
||
| def glob_dirs(self, pattern: str) -> list[str]: | ||
| """Directories matching a ``spec_paths`` glob, from disk and the PR.""" | ||
| found = { | ||
| d for d in self._added_dirs if fnmatchcase(d, pattern) or fnmatchcase(f"{d}/", pattern) | ||
| } | ||
| for match in self._root.glob(pattern): | ||
| if not match.is_dir(): | ||
| continue | ||
| try: | ||
| found.add(match.relative_to(self._root).as_posix()) | ||
| except ValueError: # a pattern that climbed out of the workspace | ||
| continue | ||
| return sorted(found) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
An unguarded detection probe can abort the review instead of skipping the spec lens. detect reaches the filesystem through _Tree.dirs_in and _Tree.glob_dirs, and neither call is wrapped. iterdir raises OSError on an unreadable directory, and Path.glob raises on a non-relative spec_paths pattern. No caller converts either into a skip, so an optional context stage can fail the whole review. Every neighbouring stage degrades instead: load_spec_files and load_context_files both drop unreadable files.
src/lgtmaybe/engine/specs.py#L147-L175: wrapparent.iterdir()indirs_inandself._root.glob(pattern)inglob_dirsso anOSError,ValueError, orNotImplementedErroryields no matches rather than propagating.src/lgtmaybe/engine/engine.py#L1690-L1735: add a defensive boundary in_resolve_specthat logs the probe failure and returnsNone, so the spec lens is skipped and the other lenses still run.
Add a test in tests/engine/test_specs.py for an absolute or unreadable spec_paths pattern, as required for changes to skip filters.
As per coding guidelines: "Skip generated and binary files, apply include/exclude path filters with exclusions winning" and "When changing redaction, injection defense, prompts, skip filters, parsing, or engine behavior, extend the corresponding test suites."
📍 Affects 2 files
src/lgtmaybe/engine/specs.py#L147-L175(this comment)src/lgtmaybe/engine/engine.py#L1690-L1735
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lgtmaybe/engine/specs.py` around lines 147 - 175, Update _Tree.dirs_in
and _Tree.glob_dirs in src/lgtmaybe/engine/specs.py (lines 147-175) to catch
OSError, ValueError, and NotImplementedError from directory iteration or
globbing and return no matches. Add a defensive boundary in _resolve_spec in
src/lgtmaybe/engine/engine.py (lines 1690-1735) that logs probe failures and
returns None so the spec lens is skipped. Add coverage in
tests/engine/test_specs.py for an absolute or unreadable spec_paths pattern.
Source: Coding guidelines
| def glob_dirs(self, pattern: str) -> list[str]: | ||
| """Directories matching a ``spec_paths`` glob, from disk and the PR.""" | ||
| found = { | ||
| d for d in self._added_dirs if fnmatchcase(d, pattern) or fnmatchcase(f"{d}/", pattern) | ||
| } | ||
| for match in self._root.glob(pattern): | ||
| if not match.is_dir(): | ||
| continue | ||
| try: | ||
| found.add(match.relative_to(self._root).as_posix()) | ||
| except ValueError: # a pattern that climbed out of the workspace | ||
| continue | ||
| return sorted(found) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Python pathlib Path.glob absolute pattern NotImplementedError non-relative patterns unsupported 3.12 3.13
💡 Result:
In Python's pathlib module, the pathlib.Path.glob and rglob methods are explicitly designed to match patterns relative to the Path object instance [1][2][3]. If you provide an absolute pattern (e.g., one starting with / on POSIX or a drive/root on Windows), these methods raise a NotImplementedError with the message "Non-relative patterns are unsupported" [1][3]. This behavior is consistent across Python 3.12 and 3.13 [1][3]. It reflects the core philosophy of pathlib, where a glob operation is intended to search within the specific subtree represented by the Path object [2][3]. Allowing absolute patterns would effectively ignore the path of the object instance itself, contradicting how these methods are intended to function [4]. To fix this error, you should remove the leading slash or anchor from your glob pattern so that it is relative to your starting path [5]. If you have an absolute path string and need to glob it, the recommended approach is to split the absolute path into its base directory and the relative pattern, then use the base directory to initiate the glob [4]: from pathlib import Path # Instead of this: # Path('/some/dir').glob('/some/dir/.txt') # Raises NotImplementedError # Do this: base = Path('/some/dir') for path in base.glob('.txt'): print(path) For scenarios where you need to match arbitrary absolute paths without a specific root, consider using the glob.glob function from the standard glob module instead, which is designed for this purpose [2][6].
Citations:
- 1: https://github.com/python/cpython/blob/v3.12.2/Lib/pathlib.py
- 2: https://docs.python.org/3/library/pathlib.html
- 3: https://github.com/python/cpython/blob/3.13/Lib/pathlib/_local.py
- 4: https://discuss.python.org/t/pathlib-objects-as-glob-template/66177
- 5: https://stackoverflow.com/questions/60323019/notimplementederror-non-relative-patterns-are-unsupported-from-attempted-loopin
- 6: Glob behavior is inconsistent across libraries python/cpython#106747
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- candidate files ---'
fd -t f 'specs\.py$|test.*spec|spec.*test' . | head -80
printf '%s\n' '--- relevant symbols and configuration text ---'
rg -n -C 4 'def glob_dirs|_added_dirs|spec_paths|unreadable|drop|glob\(' src tests 2>/dev/null | head -240
printf '%s\n' '--- Python runtime probe ---'
python3 - <<'PY'
import pathlib
import sys
root = pathlib.Path(".")
print("python", sys.version)
for pattern in ("/etc/*", "/tmp/**", "relative/*"):
try:
iterator = root.glob(pattern)
print(pattern, "iterator_created", type(iterator).__name__)
print(" first_result", next(iterator, None))
except Exception as exc:
print(pattern, type(exc).__name__, repr(str(exc)))
PYRepository: MattJColes/lgtmaybe
Length of output: 21184
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- project Python targets ---'
rg -n -C 3 'requires-python|python_requires|Programming Language :: Python|python-version' pyproject.toml setup.cfg setup.py tox.ini .github 2>/dev/null | head -160
printf '%s\n' '--- specs.py structure ---'
ast-grep outline src/lgtmaybe/engine/specs.py
printf '%s\n' '--- glob_dirs and callers ---'
rg -n -C 12 'def glob_dirs|glob_dirs\(' src/lgtmaybe/engine/specs.py src tests/engine/test_specs.py
printf '%s\n' '--- focused tests and surrounding implementation ---'
sed -n '1,230p' src/lgtmaybe/engine/specs.py
sed -n '330,410p' tests/engine/test_specs.pyRepository: MattJColes/lgtmaybe
Length of output: 25612
Guard Path.glob against non-relative spec_paths patterns.
On supported Python versions, iterating over Path.glob("/etc/*") raises NotImplementedError("Non-relative patterns are unsupported") before the loop body. Catch this exception around list(self._root.glob(pattern)) so an invalid pattern does not abort the review.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lgtmaybe/engine/specs.py` around lines 163 - 175, Update glob_dirs to
materialize self._root.glob(pattern) inside a try/except, catching
NotImplementedError for non-relative patterns and treating it as no filesystem
matches; then iterate over the collected matches while preserving the existing
directory and relative-path handling.
| # intent and spec are skipped: _CTX states no intent, and no committed | ||
| # specification is detected in the workspace to review against. | ||
| assert len(_review_calls(provider)) == len(cfg.categories) - 2 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the no-spec assertion independent of the checkout.
LLMReviewEngine defaults workspace_root to Path.cwd() and scans it before building lenses. A matching specification can add the spec call and make this assertion fail. Use an empty tmp_path workspace, or set spec_review=False explicitly for this test. Also change the comment to “no matching specification”; this repository can contain committed specifications that do not match _CTX.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/engine/test_engine.py` around lines 728 - 730, Update the test around
_review_calls(provider) to make specification detection deterministic by using
an empty tmp_path workspace or explicitly disabling spec_review in the
LLMReviewEngine configuration. Revise the comment to say “no matching
specification,” while preserving the expected category count adjustment.
2 participants
Why
Teams increasingly drive work from a committed specification — OpenSpec, GitHub Spec Kit, Kiro. All three commit the spec into the repo, so it sits right next to the diff, and nobody checks the two against each other. The reviewer reads the code; the spec is read once, by the agent that wrote the code, and never audited again.
The spec lens is the intent lens pointed at a better source of truth: structured, predating the code, with a task list recording what the author claims to have finished.
What it reports
Both directions.
The diff falling short of the spec — contradicted requirements (
high), a ticked task that was not delivered (medium), a requirement with nothing implementing it (medium), an acceptance criterion with no test (low).The spec falling short of the diff — behaviour no requirement covers, a requirement the change made stale, an unresolved
[NEEDS CLARIFICATION](info/low). This half is the more reliable of the two, because its evidence is entirely in the diff.Ticked checkboxes are the highest-precision signal, and they cost nothing
All three systems track progress with markdown checkboxes, and a PR that implements tasks flips them:
That pair is already in the diff.
specs.ticked_tasksextracts it with no model call and no extra file read, turning "did this deliver the spec?" into "is T014 actually here?". A ticked task is something to check, never to assume false.Cost is gated on detection
Detection is a pure filesystem probe; selection is deterministic (the PR edits the spec, the branch names it, or the stated intent does — a lone spec directory wins by default), capped at two bundles. Nothing detected or nothing matched drops the lens before the fan-out: no model call, no prompt bytes. When it does fire it is a lens of its own — a fifth
fastcall — because its block is large and correctness already carries the intent block there.Trust and visibility follow the diff's posture, not a config file's
SPECblock. A spec is usually committed in the PR that implements it, so on a fork the author controls the requirements their own change is judged against. The worst case is a suppressed spec finding; no other lens sees the block.pull_request_targetis the trusted base branch.reflect.py's gap-finding carve-out now names spec mismatches — without it the auditor prunes them all as cross-file absence claims, which would silently gut the feature.Notable side change
LLMReviewEnginetakes an injectableworkspace_root(defaultPath.cwd()), replacing two hardcodedPath.cwd()calls. This lets the eval harness root a review at its fixture, and stops a host repository's own specs leaking into a fixture run — an eval result should not depend on the operator's working directory.Testing
tests/engine/test_specs.py(36 new) — detection for each layout and for none, selection ranking and determinism, ticked-task extraction, workspace containment refusal, head-text-preferred-over-workspace, budget and redaction.tests/engine/test_spec_lens.py(14 new) — the block reaches exactly one call, the lens is dropped when nothing matches,--no-specworks, a forgedSPEC_ENDcannot break out, and the shared cacheable prefix and system preamble are byte-identical with the feature on and off.tests/engine/test_injection.py,test_prompt.py— new family registration, wrapper coverage, and the spec rubric's assertions (including "not shown ≠ undelivered").evals/fixtures/spec-delivery/— a Kiro spec whose diff ticks four tasks and delivers two, plus aforcebypass no requirement covers, and three forbidden cross-file traps the corpus refutes. Line arithmetic verified againstchanged_line_index.ruff check/ruff format --check/mypy/ 1941 passed, andpytest tests/specs+openspec validate --specs.Verified end-to-end against the fixture with a fake provider: Kiro detected → lens enabled → 5 calls, exactly 1 carrying the spec block with the requirements text and the extracted claims.
Honest limits
files_not_visiblelist bounds the claim.Docs
New "Spec — does the PR deliver the specification it commits to?" section in
explanation/what-gets-reviewed.md, a new anchored requirement inopenspec/specs/prompt-and-lenses/, plus regeneratedconfig.mdandllms-full.txt.🤖 Generated with Claude Code
https://claude.ai/code/session_013pq1xMbeKhX9AW3sLsgkrt
Generated by Claude Code
Summary by CodeRabbit