Skip to content

feat(review): codex review correctness (PR 2 of 3) #1370

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.

Sign up for GitHub

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

Open
peyton-alt wants to merge 4 commits into
base: review-review
Choose a base branch
from
Open

feat(review): codex review correctness (PR 2 of 3) #1370

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3492367
feat(review): codex review correctness (skills, discovery, live tokens)
peyton-alt Jun 4, 2026
242e9bb
fix(review): address #1370 bugbot findings
peyton-alt Jun 4, 2026
69cc486
fix(review): address #1370 Copilot findings (codex)
peyton-alt Jun 4, 2026
5974ba9
fix(review): capture Claude reviewer model via transcript backfill (#…
peyton-alt Jun 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
284 changes: 24 additions & 260 deletions cmd/entire/cli/agent/claudecode/discovery.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,9 @@ package claudecode

import (
"context"
"errors"
"log/slog"
"os"
"path/filepath"
"sort"
"strings"

"golang.org/x/mod/semver"

"github.com/entireio/cli/cmd/entire/cli/agent"
"github.com/entireio/cli/cmd/entire/cli/agent/skilldiscovery"
Expand All @@ -21,16 +16,18 @@ import (
// (nil, nil) when HOME is unreadable or directories are missing — discovery
// is best-effort.
//
// Claude Code exposes three kinds of invocable content per plugin:
// - skills: <plugin>/skills/<name>/SKILL.md (YAML frontmatter with name + description)
// - commands: <plugin>/commands/<name>.md (YAML frontmatter with description; name = filename)
// - agents: <plugin>/agents/<name>.md (YAML frontmatter with description; name = filename)
// Claude Code exposes three kinds of invocable content per plugin, all invoked
// via the same slash-prefix syntax (`/name`, `/plugin:name`):
// - skills: <plugin>/skills/<name>/SKILL.md (frontmatter: name + description)
// - commands: <plugin>/commands/<name>.md (frontmatter: description; name = filename)
// - agents: <plugin>/agents/<name>.md (frontmatter: description; name = filename)
//
// All three are walked because any can be a review tool — the pr-review-toolkit
// plugin, for example, ships its review skills as commands/agents (not skills/).
//
// All three are walked because users invoke them via the same slash-prefix
// syntax (`/plugin:name`) and any of them can be a review tool. The
// pr-review-toolkit plugin, for example, ships its review skills as
// commands/agents (not skills/), and was silently missed by a skills-only
// walker.
// The generic SKILL.md / markdown scanning, version dedupe, and frontmatter
// parsing live in the shared skilldiscovery package; this method supplies the
// Claude-specific roots and slash invocation form.
//
//nolint:unparam // error return is part of SkillDiscoverer contract; future implementations may report hard failures
func (c *ClaudeCodeAgent) DiscoverReviewSkills(ctx context.Context) ([]agent.DiscoveredSkill, error) {
Expand All @@ -40,255 +37,22 @@ func (c *ClaudeCodeAgent) DiscoverReviewSkills(ctx context.Context) ([]agent.Dis
return nil, nil
}

form := skilldiscovery.SlashForm
var found []agent.DiscoveredSkill
found = append(found, scanPluginCache(ctx, filepath.Join(home, ".claude", "plugins", "cache"))...)
found = append(found, scanUserSkills(ctx, filepath.Join(home, ".claude", "skills"))...)
found = append(found, scanFlatMarkdownDir(ctx, filepath.Join(home, ".claude", "commands"), "")...)
found = append(found, scanFlatMarkdownDir(ctx, filepath.Join(home, ".claude", "agents"), "")...)
found = dedupeByInvocation(found)
found = append(found, skilldiscovery.ScanPluginCache(ctx, filepath.Join(home, ".claude", "plugins", "cache"),
func(versionRoot, pluginName string) []agent.DiscoveredSkill {
var out []agent.DiscoveredSkill
out = append(out, skilldiscovery.ScanSkillsDir(ctx, filepath.Join(versionRoot, "skills"), pluginName, form)...)
out = append(out, skilldiscovery.ScanFlatMarkdownDir(ctx, filepath.Join(versionRoot, "commands"), pluginName, form)...)
out = append(out, skilldiscovery.ScanFlatMarkdownDir(ctx, filepath.Join(versionRoot, "agents"), pluginName, form)...)
return out
})...)
found = append(found, skilldiscovery.ScanSkillsDir(ctx, filepath.Join(home, ".claude", "skills"), "", form)...)
found = append(found, skilldiscovery.ScanFlatMarkdownDir(ctx, filepath.Join(home, ".claude", "commands"), "", form)...)
found = append(found, skilldiscovery.ScanFlatMarkdownDir(ctx, filepath.Join(home, ".claude", "agents"), "", form)...)
found = skilldiscovery.DedupeByInvocation(found)
if len(found) == 0 {
return nil, nil
}
return found, nil
}

// dedupeByInvocation collapses entries sharing an invocation name. Plugins
// can ship a skill and a same-named command wrapper that forwards to it;
// scan order keeps the skill over its wrapper.
func dedupeByInvocation(in []agent.DiscoveredSkill) []agent.DiscoveredSkill {
if len(in) < 2 {
return in
}
seen := make(map[string]struct{}, len(in))
out := make([]agent.DiscoveredSkill, 0, len(in))
for _, s := range in {
if _, dup := seen[s.Name]; dup {
continue
}
seen[s.Name] = struct{}{}
out = append(out, s)
}
return out
}

// scanPluginCache walks <root>/<marketplace>/<plugin>/<version>/{skills,commands,agents}/
// One plugin can contribute through any or all three directories.
//
// Multiple version directories per plugin are common after upgrades. Walking
// every version produces duplicate skills (same invocation name, same
// description) — confusing in the picker and wasteful in the prompt. We pick
// a single version per plugin via pickLatestVersion: prefer valid semver
// (highest), fall back to lexicographic max.
func scanPluginCache(ctx context.Context, root string) []agent.DiscoveredSkill {
entries, err := os.ReadDir(root)
if err != nil {
logging.Debug(ctx, "claude-code discovery: plugin cache unreadable",
slog.String("root", root), slog.String("error", err.Error()))
return nil
}
var found []agent.DiscoveredSkill
for _, marketEntry := range entries {
if !marketEntry.IsDir() {
continue
}
marketRoot := filepath.Join(root, marketEntry.Name())
pluginEntries, err := os.ReadDir(marketRoot)
if err != nil {
continue
}
for _, pluginEntry := range pluginEntries {
if !pluginEntry.IsDir() {
continue
}
pluginName := pluginEntry.Name()
pluginRoot := filepath.Join(marketRoot, pluginName)
versionEntries, err := os.ReadDir(pluginRoot)
if err != nil {
continue
}
versionDir, ok := pickLatestVersion(versionEntries)
if !ok {
continue
}
versionRoot := filepath.Join(pluginRoot, versionDir)
found = append(found, readSkillsDir(ctx, filepath.Join(versionRoot, "skills"), pluginName)...)
found = append(found, scanFlatMarkdownDir(ctx, filepath.Join(versionRoot, "commands"), pluginName)...)
found = append(found, scanFlatMarkdownDir(ctx, filepath.Join(versionRoot, "agents"), pluginName)...)
}
}
return found
}

// pickLatestVersion returns the name of the "newest" version directory among
// entries. Strategy:
//
// - If any entry name parses as semver (with or without a leading "v"), pick
// the highest semver among those that parse. Non-semver entries are
// ignored when at least one semver entry exists.
// - Otherwise, fall back to the lexicographic max of all directory names.
// This handles the "unknown" sentinel some plugins ship and one-off names.
//
// Returns ("", false) if no usable directory entry exists.
func pickLatestVersion(entries []os.DirEntry) (string, bool) {
var dirs []string
for _, e := range entries {
if e.IsDir() {
dirs = append(dirs, e.Name())
}
}
if len(dirs) == 0 {
return "", false
}
var semverDirs []string
for _, d := range dirs {
if semver.IsValid(semverWithV(d)) {
semverDirs = append(semverDirs, d)
}
}
if len(semverDirs) > 0 {
sort.Slice(semverDirs, func(i, j int) bool {
return semver.Compare(semverWithV(semverDirs[i]), semverWithV(semverDirs[j])) > 0
})
return semverDirs[0], true
}
sort.Sort(sort.Reverse(sort.StringSlice(dirs)))
return dirs[0], true
}

// semverWithV ensures a version string has the "v" prefix that
// golang.org/x/mod/semver requires. Plugin version dirs are usually bare
// (e.g. "0.1.0"), but we tolerate either form.
func semverWithV(s string) string {
if strings.HasPrefix(s, "v") {
return s
}
return "v" + s
}

// scanUserSkills walks ~/.claude/skills/<skill>/SKILL.md.
func scanUserSkills(ctx context.Context, root string) []agent.DiscoveredSkill {
return readSkillsDir(ctx, root, "" /* no plugin prefix */)
}

// readSkillsDir reads each skill subdirectory's SKILL.md, parses frontmatter,
// and emits a DiscoveredSkill if Matches() returns true.
func readSkillsDir(ctx context.Context, dir, pluginName string) []agent.DiscoveredSkill {
entries, err := os.ReadDir(dir)
if err != nil {
return nil
}
var found []agent.DiscoveredSkill
for _, skillEntry := range entries {
if !skillEntry.IsDir() {
continue
}
skillDir := filepath.Join(dir, skillEntry.Name())
skillFile := filepath.Join(skillDir, "SKILL.md")
data, err := os.ReadFile(skillFile) //nolint:gosec // G304: skillFile is constructed from a ReadDir walk under HOME, not user input
if err != nil {
continue
}
name, description, parseErr := parseSkillFrontmatter(data)
if parseErr != nil {
logging.Debug(ctx, "claude-code discovery: skipping malformed SKILL.md",
slog.String("path", skillFile), slog.String("error", parseErr.Error()))
continue
}
if name == "" {
name = skillEntry.Name()
}
invocation := invocationName(name, pluginName)
if !skilldiscovery.Matches(invocation, description) {
continue
}
found = append(found, agent.DiscoveredSkill{
Name: invocation,
Description: description,
SourcePath: skillFile,
})
}
return found
}

// scanFlatMarkdownDir reads *.md files directly under dir (no nesting), parses
// their YAML frontmatter for `description:`, and derives the invocation name
// from the filename (stripping the .md suffix). Used for both plugin
// commands/agents and user-level ~/.claude/commands and ~/.claude/agents.
//
// Frontmatter shape differs from SKILL.md — no `name:` field, so the
// filename is the source of truth for the invocation name.
func scanFlatMarkdownDir(ctx context.Context, dir, pluginName string) []agent.DiscoveredSkill {
entries, err := os.ReadDir(dir)
if err != nil {
return nil
}
var found []agent.DiscoveredSkill
for _, entry := range entries {
if entry.IsDir() || !strings.HasSuffix(entry.Name(), ".md") {
continue
}
baseName := strings.TrimSuffix(entry.Name(), ".md")
if strings.EqualFold(baseName, "README") {
continue
}
filePath := filepath.Join(dir, entry.Name())
data, err := os.ReadFile(filePath) //nolint:gosec // G304: filePath is constructed from a ReadDir walk under HOME, not user input
if err != nil {
continue
}
_, description, parseErr := parseSkillFrontmatter(data)
if parseErr != nil {
logging.Debug(ctx, "claude-code discovery: skipping malformed command/agent",
slog.String("path", filePath), slog.String("error", parseErr.Error()))
continue
}
invocation := invocationName(baseName, pluginName)
if !skilldiscovery.Matches(invocation, description) {
continue
}
found = append(found, agent.DiscoveredSkill{
Name: invocation,
Description: description,
SourcePath: filePath,
})
}
return found
}

// invocationName builds the slash-prefixed invocation form. Plugin-prefixed
// names use "/plugin:name"; bare names use "/name".
func invocationName(name, pluginName string) string {
if pluginName == "" {
return "/" + name
}
return "/" + pluginName + ":" + name
}

// parseSkillFrontmatter extracts `name:` and `description:` from a minimal
// YAML frontmatter block. Purpose-built for the tiny subset of YAML these
// SKILL.md / command / agent files actually use.
//
// Trims surrounding double-quotes from values so `description: "foo bar"`
// is returned as `foo bar` — the command/agent frontmatter quotes values;
// SKILL.md files usually don't.
func parseSkillFrontmatter(data []byte) (name, description string, err error) {
s := string(data)
if !strings.HasPrefix(s, "---\n") && !strings.HasPrefix(s, "---\r\n") {
return "", "", errors.New("no frontmatter delimiter")
}
body := strings.TrimPrefix(strings.TrimPrefix(s, "---\r\n"), "---\n")
end := strings.Index(body, "\n---")
if end < 0 {
return "", "", errors.New("no closing frontmatter delimiter")
}
for _, line := range strings.Split(body[:end], "\n") {
line = strings.TrimSpace(line)
switch {
case strings.HasPrefix(line, "name:"):
name = strings.Trim(strings.TrimSpace(strings.TrimPrefix(line, "name:")), `"`)
case strings.HasPrefix(line, "description:"):
description = strings.Trim(strings.TrimSpace(strings.TrimPrefix(line, "description:")), `"`)
}
}
return name, description, nil
}
34 changes: 31 additions & 3 deletions cmd/entire/cli/agent/claudecode/reviewer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,17 @@ func buildReviewCmd(ctx context.Context, cfg reviewtypes.RunConfig) *exec.Cmd {
// Emits Started first, Finished{Success:...} last (success follows result.is_error).
// On a scanner error (torn stream), emits RunError then Finished{Success:false}.
//
// Tokens are emitted only at the terminal `result` envelope, not
// incrementally — claude's per-assistant `usage` fields aren't cumulative
// and summing them across messages would double-count.
// Live-token semantics: Claude's assistant envelopes carry a usage snapshot
// taken at the START of each turn — input_tokens/cache_* are populated but
// output_tokens is essentially zero (a 1–8 token "initial decision" count
// that does not update as text streams). The true per-turn output is only
// surfaced on `result` (aggregate across all turns in the run) or on the
// late `message_delta` event of --include-partial-messages mode.
//
// We emit `Tokens{In: <sum>, Out: 0}` on each assistant envelope so the TUI
// can show context size growing across multi-turn runs, and `Tokens{In, Out}`
// on `result` with the final aggregate. Sending the misleading early
// "Out: 1" snapshot was removed after capturing real claude stream-json.
//
// Package-private; called directly from this package's tests so they can
// drive raw stdout fixtures through the parser without going through the
Expand Down Expand Up @@ -110,6 +118,20 @@ func parseClaudeOutputBuf(r io.Reader, maxBuf int) <-chan reviewtypes.Event {
out <- reviewtypes.ToolCall{Name: block.Name, Args: string(block.Input)}
}
}
// The usage snapshot here is captured at the start of the
// turn, so output_tokens is essentially zero (1–8 tokens of
// initial decision, not the true output count). Surface input
// only — TUI consumers render input-only Tokens differently
// from finalized {In, Out} pairs. The final tally comes from
// the `result` envelope below.
if env.Message.Usage.InputTokens > 0 ||
env.Message.Usage.CacheReadInputTokens > 0 ||
env.Message.Usage.CacheCreationInputTokens > 0 {
in := env.Message.Usage.InputTokens +
env.Message.Usage.CacheReadInputTokens +
env.Message.Usage.CacheCreationInputTokens
out <- reviewtypes.Tokens{In: in, Out: 0}
}
case "result":
sawResult = true
resultErr = env.IsError
Expand Down Expand Up @@ -145,6 +167,12 @@ type claudeEnvelope struct {

type claudeMessage struct {
Content []claudeBlock `json:"content"`
// Usage on assistant envelopes is the per-turn-START snapshot — input
// counts are populated but output_tokens reflects only the model's
// initial decision, not the streamed text. Final aggregate usage
// arrives on the `result` envelope. Reuses messageUsage (declared in
// types.go) to stay aligned with the transcript-parser usage shape.
Usage messageUsage `json:"usage"`
}

type claudeBlock struct {
Expand Down
Loading
Loading