Skip to content

Add focused OAR reviewer profiles - #49

Open
johnnygreco wants to merge 17 commits into
mainfrom
johnny/reviewer-profiles
Open

Add focused OAR reviewer profiles#49
johnnygreco wants to merge 17 commits into
mainfrom
johnny/reviewer-profiles

Conversation

@johnnygreco

Copy link
Copy Markdown
Collaborator

Summary

  • add code-reviewer and technical-writing-reviewer profiles with runtime prompt variables and scored JSON results
  • exercise document and repository inputs directly through OAR
  • add a trusted-PR smoke test that starts an ephemeral OpenShell gateway and runs both profiles end to end

Validation

  • make check
  • make build
  • python3 tests/test_render_dev_notes.py
  • scripts/build-docs.sh
  • direct CLI dry runs for both reviewer profiles

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown

Documentation preview

View the deployed preview

Built from 443ac5e.

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown

Reviewer profile smoke test

✅ The full OAR pipeline completed successfully. View workflow run.

Profile Verdict Score Findings
Code reviewer ⚠️ Needs Changes 67/100 3
Technical writing reviewer ⚠️ Needs Changes 64/100 6
Code reviewer — 67/100 · ⚠️ Needs Changes

Summary

The repository is a single 10-line module (src/totals.py) exposing arithmetic_mean, plus a two-sentence README. The implementation matches its documented contract: it returns sum(values)/len(values) for a non-empty list, and the README explicitly assigns empty-input responsibility to callers, so the absence of a guard is consistent rather than a defect. The material gaps are around verification and consumability: there are no tests at all (even though the module docstring claims the helpers are used by end-to-end tests), and there is no packaging metadata or package init.py, so src/totals.py cannot be imported by a consumer without ad-hoc sys.path manipulation. The failure mode for the excluded empty-list case is also undocumented and surfaces as a low-level ZeroDivisionError.

Rubric

Criterion Score Rationale
Correctness 88 The single function computes the arithmetic mean exactly as documented for non-empty lists, and the type annotation matches the README's stated input. No behavioral mismatch with the documented contract was found.
Robustness Security 75 There is no trust boundary, I/O, or untrusted input, so security is essentially not applicable. Robustness is proportionate: the contract explicitly excludes empty input, so no guard is required. Points are withheld only because the failure mode for the excluded case is undocumented and surfaces as a low-level ZeroDivisionError.
Maintainability Complexity 88 The module is minimal and carries an SPDX header, module docstring, function docstring, and type hints. Complexity is appropriate to the purpose, ownership is obvious, and nothing is over-abstracted.
Tests Verification 30 There are zero test files and no test configuration or runner in the repository. The sole public function, including its documented non-empty precondition, has no credible verification, and the module docstring asserts a testing role the repository does not implement.
Usability Integration 55 There is no pyproject.toml, setup.cfg, package directory, or init.py. src/totals.py is a loose module under a src/ root, so a consumer cannot import totals without manual sys.path setup, and the README gives no install or usage example.

Findings

  • HIGH — No tests exist for the library's only function · src/totals.py:7
    • Add a minimal test module (e.g. tests/test_totals.py) covering a multi-element mean, a single-element list, and the documented empty-list failure mode, so the contract is executable.
  • MEDIUM — Module is not importable as a package: no packaging metadata or init.py · src/totals.py
    • Either add a minimal pyproject.toml with a src layout and a package directory containing init.py, or move totals.py to the repository root and state the import path in the README.
  • LOW — Behavior for the excluded empty-list case is undocumented · src/totals.py:9
    • Document the raised exception in the docstring (e.g. "Raises ZeroDivisionError if values is empty"), or raise a ValueError with an explicit message and document that instead.

Strengths

  • The implementation matches its documented contract exactly, with no unnecessary defensive code for a state the README explicitly excludes.
  • The module carries an SPDX license header, a module docstring, a function docstring, and accurate type annotations despite its tiny size.
Technical writing reviewer — 64/100 · ⚠️ Needs Changes

Summary

This is a very short source-checkout installation page for a Python library called Totals. Its sequencing is sound and its success criterion (tests exit successfully) is a good touch, but for a developer installing the library for the first time it omits the prerequisites and context needed to actually run the two commands: uv is never named as a required tool or given an install pointer, and the reader is never told to obtain the repository or change into its directory (both of which uv sync --locked requires). The step label "Create an environment" also does not match what the command visibly does, and there is no guidance on activating/using the resulting environment or on what to do when a command fails. Five targeted additions (a prerequisites line, a get-the-source step, a corrected step label, a post-install usage pointer, and one failure-mode note) would make the page usable without changing its concise voice.

Rubric

Criterion Score Rationale
Accuracy Grounding 78 Nothing stated is technically wrong: uv sync --locked and uv run pytest are correct modern uv invocations, and the Python 3.12 floor is a plausible, clearly stated constraint. The one accuracy weakness is the mismatch between the instruction "Create an environment and install the project" and the single command shown, which creates the environment implicitly rather than asking the reader to create one.
Clarity Precision 70 Sentences are short and unambiguous in isolation, and the success criterion is stated plainly. However, key terms are unintroduced (uv, --locked, the implied virtual environment), so a first-time reader cannot tell what tool they are being asked to use or what the flag guarantees.
Completeness 45 The document omits several prerequisites its own commands depend on: installing uv, obtaining the project source, and being in the project directory. It also omits what to do after installation succeeds (activating or using the environment) and any failure mode, such as a lockfile that no longer matches pyproject.toml under --locked.
Structure Navigation 80 The requirement-then-install-then-verify ordering is the right shape for a short install page, and the indented command blocks are easy to scan. It would need only one additional step slot near the top (prerequisites and source checkout) rather than reorganization.
Audience Fit 55 The page assumes the reader already has uv installed, already has the repository checked out, and already understands uv's project workflow. For developers installing the library for the first time, that assumed baseline is too high and is nowhere stated as a precondition.
Actionability Evidence 55 The two commands are copy-pasteable and the verification step is concrete and testable, which is genuinely actionable. But a first-time reader following the page top to bottom will hit a missing-command or missing-lockfile error before reaching that verification, and no expected output or recovery guidance is offered.

Findings

  • HIGH — uv is required but never introduced or installed · line 7
    • Add a prerequisites line before line 5, for example: "Totals uses uv for environment and dependency management. Install it first (see the uv installation guide), then verify with uv --version."
  • HIGH — No step to obtain the project source or change into its directory · line 5
    • Insert an explicit first step with the concrete commands, e.g. git clone <repo-url> followed by cd totals, and state that the remaining commands are run from the project root.
  • MEDIUM — Step label does not describe what the command does · line 5
    • Reword to describe the command's actual effect, e.g. "Create the virtual environment and install the pinned dependencies (uv does both):", and note that the environment is created at .venv in the project root.
  • MEDIUM — No guidance on using the installed library after setup · line 13
    • Add one closing sentence, e.g. "Run project commands with uv run <command>, or activate the environment with source .venv/bin/activate (.venv\\Scripts\\activate on Windows)."
  • MEDIUM — No failure modes or Python-version handling for the two commands · line 3
    • Add a short note such as: "Check your version with python --version; uv will download a suitable interpreter if none is available. If uv sync --locked reports that the lockfile is out of date, run uv sync to regenerate it."
  • LOW — Scope of "Installing Totals" is ambiguous for library consumers · line 1
    • Narrow the heading or add one orienting sentence, e.g. "This page covers setting up Totals from source for development. To use Totals as a dependency, run uv add totals in your own project."

Strengths

  • The verification step gives an unambiguous, testable definition of done ("ready when the test command exits successfully") rather than leaving success to the reader's judgment.
  • Ordering is correct and appropriately paced for a short install page: requirement, install, verify, with commands set off in scannable indented blocks.
  • The commands themselves are current, minimal, and copy-pasteable, with no redundant or outdated tooling advice.

Tested PR head 443ac5e. Full JSON results are available from the workflow run's artifacts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant