-
Notifications
You must be signed in to change notification settings - Fork 0
add review skill, curl installer, model inheritance #96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
2acec90
add review skill, curl installer, model inheritance, external skills …
yourconscience 75cd404
fix review findings: checksum exit, version tag handling, skill count…
yourconscience 52b4e45
fix bot review: detect default branch, run tests in fix loop
yourconscience File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,136 @@ | ||
| --- | ||
| name: review | ||
| description: Multi-persona code review with structured findings. Use when the user says /review, asks for a code review, wants to review a diff/PR/branch before merging, or needs a pre-submit quality check. | ||
| --- | ||
|
|
||
| # review | ||
|
|
||
| Proactive multi-persona code review. Produces structured findings before a PR is submitted or merged. | ||
|
|
||
| Not the same as `/pr-triage`, which reacts to existing PR comments and CI failures. Use `/review` when you want fresh review findings on a diff. | ||
|
|
||
| ## Usage | ||
|
|
||
| ``` | ||
| /review # review staged + unstaged changes | ||
| /review HEAD~3..HEAD # review last 3 commits | ||
| /review main..HEAD # review current branch vs base | ||
| /review --fix # review and fix findings (iterative) | ||
| /review --persona security # single-persona review | ||
| ``` | ||
|
|
||
| ## Phase 1: Scope | ||
|
|
||
| Determine the diff to review. | ||
|
|
||
| If the user provided a range, use it. Otherwise detect: | ||
|
|
||
| ```bash | ||
| # If there are staged/unstaged changes, review those | ||
| git diff HEAD | ||
|
|
||
| # If clean working tree, review current branch vs default base | ||
| BASE="$(git symbolic-ref refs/remotes/origin/HEAD 2>/dev/null | sed 's|refs/remotes/origin/||')" | ||
| BASE="${BASE:-main}" | ||
| git log --oneline "${BASE}..HEAD" | ||
| git diff "${BASE}...HEAD" | ||
| ``` | ||
|
|
||
| Identify relevant context files: CONTRIBUTING.md, coding standards, project CLAUDE.md/AGENTS.md, any spec or PRD referenced in recent commit messages. | ||
|
|
||
| If the diff is large (>2000 lines), ask the user whether to review everything or focus on specific files/directories. | ||
|
|
||
| ## Phase 2: Parallel Persona Review | ||
|
|
||
| Spawn 2+ reviewer sub-agents in parallel. Each persona writes free-form prose -- do not constrain their output format. | ||
|
|
||
| ### Default personas | ||
|
|
||
| **Standards reviewer:** Does the code follow project conventions? Check naming, error handling patterns, test coverage expectations, import ordering, existing abstractions. Reference CONTRIBUTING.md and project style if available. Ignore style issues that a linter would catch. | ||
|
|
||
| **Correctness reviewer:** Are there bugs, edge cases, race conditions, or logic errors? Does the code handle failures correctly? Are there security issues (injection, auth bypass, secret leaks)? Does it do what the commit messages claim? | ||
|
|
||
| ### Optional personas (user-requested or auto-detected) | ||
|
|
||
| **Security reviewer:** Activated when the diff touches auth, crypto, user input handling, or API endpoints. Focus on OWASP top 10, secret leaks, permission checks. | ||
|
|
||
| **Performance reviewer:** Activated when the diff touches hot paths, database queries, or algorithms. Focus on complexity, unnecessary allocations, N+1 queries. | ||
|
|
||
| Each persona prompt should include: | ||
| - The full diff | ||
| - Relevant context files (standards, spec) | ||
| - Instruction to write freely, be specific (file:line), and focus on what matters | ||
|
|
||
| ## Phase 3: Normalize Findings | ||
|
|
||
| After all persona reviews return, extract structured findings from each persona's prose. Use a separate, cheaper model call for this extraction when possible. | ||
|
|
||
| For each finding, extract: | ||
|
|
||
| ``` | ||
| File: <path> | ||
| Lines: <start>-<end> | ||
| Severity: critical | high | medium | low | ||
| Confidence: <0.0-1.0> | ||
| Problem: <one paragraph -- what is wrong and why it matters> | ||
| Fix: <one paragraph -- concrete fix, not "add tests" or "consider refactoring"> | ||
| ``` | ||
|
|
||
| Normalization rules: | ||
| - Do not invent findings that the reviewer did not mention | ||
| - Do not fabricate line numbers -- if the reviewer was vague, mark lines as "unknown" | ||
| - Vague fix text ("add tests", "consider refactoring", "improve error handling") must be expanded to a concrete action or the finding is downgraded to low severity | ||
| - If a reviewer flagged the same issue multiple times, deduplicate | ||
|
|
||
| ## Phase 4: Aggregate | ||
|
|
||
| Combine findings from all personas. Deduplicate findings that reference the same file:line range with overlapping descriptions. | ||
|
|
||
| Upgrade severity when multiple personas independently flag the same issue. Downgrade when only one persona flagged it and confidence is below 0.5. | ||
|
|
||
| ### Output format | ||
|
|
||
| ``` | ||
| ## Review: <range or description> | ||
|
|
||
| ### Must Fix (<count>) | ||
| Critical severity. Each with file:line, problem, fix. | ||
|
|
||
| ### Major Issues (<count>) | ||
| High severity. Each with file:line, problem, fix. | ||
|
|
||
| ### Review Carefully (<count>) | ||
| Medium severity. Collapsed unless user asks for detail. | ||
|
|
||
| ### Minor (<count>) | ||
| Low severity. One-line summaries only. | ||
|
|
||
| ### Summary | ||
| - Total findings: <N> (<critical> critical, <high> high, <medium> medium, <low> low) | ||
| - Personas used: <list> | ||
| - Files reviewed: <count> | ||
| ``` | ||
|
|
||
| ## Phase 5: Fix Loop (only with --fix) | ||
|
|
||
| Only runs when the user passes `--fix` or explicitly asks to fix findings. | ||
|
|
||
| For each finding rated critical or high with a concrete fix: | ||
|
|
||
| 1. Apply the fix (use the builder role if available, otherwise fix directly) | ||
| 2. Run tests or build if available to catch regressions before re-review | ||
| 3. Re-run the persona that originally flagged the finding on the changed file | ||
| 3. If the reviewer confirms the fix, mark as resolved | ||
| 4. If the reviewer finds a new issue with the fix, iterate (max 3 rounds per finding) | ||
|
|
||
| After all fixes, re-run all personas on the final diff to catch issues introduced by fixes. The final aggregate uses only fresh findings from this re-run, not stale output from the original review. | ||
|
|
||
| ## Rules | ||
|
|
||
| - Review is read-only by default. Only modify files in `--fix` mode. | ||
| - Do not post review comments to GitHub. Output findings to the conversation. The user decides what to do with them. | ||
| - When spawning persona sub-agents, use the `reviewer` role if available. Sub-agents inherit the parent model unless the user specifies otherwise. | ||
| - If the harness does not support sub-agents, run personas sequentially instead of in parallel. | ||
| - Be specific. Every finding must reference a file and ideally a line range. Findings without file references are noise. | ||
| - Do not nitpick formatting, whitespace, or style issues that a linter handles. Focus on correctness, security, and spec compliance. | ||
| - Large diffs (>5000 lines): warn the user that review quality degrades with size. Suggest splitting by directory or concern. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,123 @@ | ||
| #!/bin/sh | ||
| # Install dotagents binary from GitHub Releases. | ||
| # Usage: curl -fsSL https://raw.githubusercontent.com/yourconscience/dotagents/main/scripts/install.sh | sh | ||
|
|
||
| set -e | ||
|
|
||
| REPO="yourconscience/dotagents" | ||
| BINARY="dotagents" | ||
| INSTALL_DIR="${DOTAGENTS_INSTALL_DIR:-$HOME/.local/bin}" | ||
|
|
||
| # Detect OS | ||
| OS="$(uname -s)" | ||
| case "$OS" in | ||
| Darwin) OS="darwin" ;; | ||
| Linux) OS="linux" ;; | ||
| *) | ||
| echo "error: unsupported OS: $OS" >&2 | ||
| exit 1 | ||
| ;; | ||
| esac | ||
|
|
||
| # Detect arch | ||
| ARCH="$(uname -m)" | ||
| case "$ARCH" in | ||
| x86_64) ARCH="amd64" ;; | ||
| arm64|aarch64) ARCH="arm64" ;; | ||
| *) | ||
| echo "error: unsupported architecture: $ARCH" >&2 | ||
| exit 1 | ||
| ;; | ||
| esac | ||
|
|
||
| # Resolve latest release tag | ||
| echo "Fetching latest release..." | ||
| LATEST_URL="https://api.github.com/repos/${REPO}/releases/latest" | ||
| if command -v curl >/dev/null 2>&1; then | ||
| TAG="$(curl -fsSL "$LATEST_URL" | grep '"tag_name"' | sed 's/.*"tag_name": *"\([^"]*\)".*/\1/')" | ||
| elif command -v wget >/dev/null 2>&1; then | ||
| TAG="$(wget -qO- "$LATEST_URL" | grep '"tag_name"' | sed 's/.*"tag_name": *"\([^"]*\)".*/\1/')" | ||
| else | ||
| echo "error: curl or wget is required" >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| if [ -z "$TAG" ]; then | ||
| echo "error: could not determine latest release tag" >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Strip leading v for archive filename (goreleaser convention) | ||
| VERSION="${TAG#v}" | ||
|
|
||
| echo "Installing ${BINARY} ${VERSION} (${OS}/${ARCH})..." | ||
|
|
||
| ARCHIVE="${BINARY}_${VERSION}_${OS}_${ARCH}.tar.gz" | ||
| BASE_URL="https://github.com/${REPO}/releases/download/${TAG}" | ||
| ARCHIVE_URL="${BASE_URL}/${ARCHIVE}" | ||
| CHECKSUM_URL="${BASE_URL}/checksums.txt" | ||
|
|
||
| # Create temp dir, cleaned up on exit | ||
| TMP_DIR="$(mktemp -d)" | ||
| trap 'rm -rf "$TMP_DIR"' EXIT | ||
|
|
||
| # Download archive and checksums | ||
| download() { | ||
| url="$1" | ||
| dest="$2" | ||
| if command -v curl >/dev/null 2>&1; then | ||
| curl -fsSL -o "$dest" "$url" | ||
| else | ||
| wget -qO "$dest" "$url" | ||
| fi | ||
| } | ||
|
|
||
| echo "Downloading ${ARCHIVE}..." | ||
| download "$ARCHIVE_URL" "${TMP_DIR}/${ARCHIVE}" | ||
|
|
||
| echo "Downloading checksums.txt..." | ||
| download "$CHECKSUM_URL" "${TMP_DIR}/checksums.txt" | ||
|
|
||
| # Verify checksum | ||
| echo "Verifying checksum..." | ||
| EXPECTED="$(grep "${ARCHIVE}" "${TMP_DIR}/checksums.txt" | awk '{print $1}')" | ||
| if [ -z "$EXPECTED" ]; then | ||
| echo "error: archive not found in checksums.txt" >&2 | ||
| exit 1 | ||
| elif command -v sha256sum >/dev/null 2>&1; then | ||
| ACTUAL="$(sha256sum "${TMP_DIR}/${ARCHIVE}" | awk '{print $1}')" | ||
| if [ "$ACTUAL" != "$EXPECTED" ]; then | ||
| echo "error: checksum mismatch (expected ${EXPECTED}, got ${ACTUAL})" >&2 | ||
| exit 1 | ||
| fi | ||
| elif command -v shasum >/dev/null 2>&1; then | ||
| ACTUAL="$(shasum -a 256 "${TMP_DIR}/${ARCHIVE}" | awk '{print $1}')" | ||
| if [ "$ACTUAL" != "$EXPECTED" ]; then | ||
| echo "error: checksum mismatch (expected ${EXPECTED}, got ${ACTUAL})" >&2 | ||
| exit 1 | ||
| fi | ||
| else | ||
| echo "warning: no sha256sum or shasum found; skipping checksum verification" >&2 | ||
| fi | ||
|
|
||
| # Extract binary | ||
| echo "Extracting..." | ||
| tar -xzf "${TMP_DIR}/${ARCHIVE}" -C "$TMP_DIR" | ||
|
|
||
| # Install | ||
| mkdir -p "$INSTALL_DIR" | ||
| mv "${TMP_DIR}/${BINARY}" "${INSTALL_DIR}/${BINARY}" | ||
| chmod +x "${INSTALL_DIR}/${BINARY}" | ||
|
|
||
| echo "" | ||
| echo "${BINARY} ${VERSION} installed to ${INSTALL_DIR}/${BINARY}" | ||
|
|
||
| # PATH hint if needed | ||
| case ":$PATH:" in | ||
| *":${INSTALL_DIR}:"*) ;; | ||
| *) | ||
| echo "" | ||
| echo "Add ${INSTALL_DIR} to your PATH:" | ||
| echo " export PATH=\"${INSTALL_DIR}:\$PATH\"" | ||
| ;; | ||
| esac |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For users following this new curl path specifically because they do not have Go installed, the next documented step (
git clone ... ~/.agentsthendotagents setup) still fails:runSetupcallsinstallDotagentsBinary, and for the cloned dotagents source repo that function immediately requiresexec.LookPath("go")before syncing. Eithersetupneeds to skip rebuilding when adotagentsbinary is already on PATH, or the README should not claim the full setup is "no Go required".Useful? React with 👍 / 👎.