feat: add Codex-owned review workflow#276
Conversation
|
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| func printCodexReportUsage(writer io.Writer) { | ||
| fmt.Fprintln(writer, `Usage: | ||
| ocr codex report --bundle FILE --comments FILE | ||
| [--validation FILE] [--format markdown|text|json] | ||
| [--output FILE]`) | ||
| } |
There was a problem hiding this comment.
The usage text omits the --repo, --session-id, and --validation flags that are registered in parseCodexReportFlags. Users invoking --help will not see these options. Consider documenting all supported flags, similar to how printCodexPrepareUsage documents its flags.
Suggestion:
| func printCodexReportUsage(writer io.Writer) { | |
| fmt.Fprintln(writer, `Usage: | |
| ocr codex report --bundle FILE --comments FILE | |
| [--validation FILE] [--format markdown|text|json] | |
| [--output FILE]`) | |
| } | |
| func printCodexReportUsage(writer io.Writer) { | |
| fmt.Fprintln(writer, `Usage: | |
| ocr codex report --bundle FILE --comments FILE | |
| [--validation FILE] [--format markdown|text|json] | |
| [--output FILE] [--repo PATH] [--session-id ID]`) |
| if comments.Comments == nil { | ||
| return nil, fmt.Errorf("invalid comments schema: comments must be an array") | ||
| } |
There was a problem hiding this comment.
Minor: The error message "comments must be an array" is slightly misleading. This nil check triggers both when the comments field is entirely absent from the JSON and when it is explicitly set to null. In both cases the decoded slice is nil. Consider a more accurate message like "comments field is required and must be an array" to help callers diagnose the issue faster.
Suggestion:
| if comments.Comments == nil { | |
| return nil, fmt.Errorf("invalid comments schema: comments must be an array") | |
| } | |
| if comments.Comments == nil { | |
| return nil, fmt.Errorf("invalid comments schema: comments field is required and must be an array") | |
| } |
| patternArguments := make([]any, 0, len(patterns)) | ||
| for _, pattern := range patterns { | ||
| cleaned, safe := cleanProtocolPath(pattern) | ||
| if !safe { | ||
| return ContextResult{}, &ProtocolError{Code: "path_escape", Message: "file pattern must stay inside the repository"} | ||
| } | ||
| patternArguments = append(patternArguments, cleaned) | ||
| } |
There was a problem hiding this comment.
Bug: cleanProtocolPath applies filepath.Clean() to file patterns, which will mangle git pathspec syntax. For example, a pattern like :(exclude)*_test.go would be transformed into something invalid by filepath.Clean, and glob patterns like *.go may also be affected since filepath.Clean can alter special characters.
The underlying code_search.go tool passes these patterns directly as git pathspec arguments (cmdArgs = append(cmdArgs, pathspec...)), so they need to preserve their original form.
Consider using a lighter validation for search patterns that only checks for path traversal (e.g., rejecting absolute paths and .. components) without running filepath.Clean, or skip sanitization entirely for patterns since they are used as git pathspecs, not filesystem paths.
| func (service *ContextService) ready(ctx context.Context) error { | ||
| if service.bundle == nil { | ||
| return fmt.Errorf("bundle is required") | ||
| } | ||
| if service.repoDir == "" { | ||
| return fmt.Errorf("repository directory is required") | ||
| } | ||
| result := ValidationResult{Errors: make([]ValidationNotice, 0)} | ||
| validateFreshTarget(ctx, &result, service.bundle, service.repoDir, service.runner) | ||
| if len(result.Errors) > 0 { | ||
| return &ProtocolError{Code: "stale_bundle", Message: result.Errors[0].Message} | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Performance concern: ready() calls validateFreshTarget() on every single context operation (Read, Find, Diff, Search). For scan-mode bundles, this reads and hashes every file in the bundle. For other modes, it resolves the target via git commands. When an agent performs multiple sequential context operations during a review session, this repeated validation adds significant overhead.
Consider caching the validation result (e.g., validate once at construction time or use a sync.Once), or providing a way to skip re-validation when the caller knows the bundle is fresh.
| empty, encoded, err := buildDiffPartition(full, nil, maxBundleSize) | ||
| estimated := int64(4096) | ||
| if err == nil { | ||
| estimated = int64(len(encoded)) + empty.Contract.BundleSizeBytes |
There was a problem hiding this comment.
Bug: The base size estimate is doubled. After marshalWithStableSize, empty.Contract.BundleSizeBytes equals len(encoded) (they converge to the same value). So this line computes estimated = len(encoded) + len(encoded) = 2 * len(encoded), which overestimates the empty-partition overhead by 2×.
This causes partitions to be under-filled (fewer files per partition than necessary), leading to more bundles than needed and wasted capacity. If the intent was to use just the encoded size as the base, remove the + empty.Contract.BundleSizeBytes term.
Suggestion:
| estimated = int64(len(encoded)) + empty.Contract.BundleSizeBytes | |
| estimated = int64(len(encoded)) |
| ROOT_JSON="${ROOT//\\/\\\\}" | ||
| ROOT_JSON="${ROOT_JSON//\"/\\\"}" |
There was a problem hiding this comment.
The JSON escaping here only handles backslashes and double quotes, but JSON strings also require escaping of control characters (e.g., newlines \n, tabs \t, carriage returns \r, etc.). While file paths rarely contain these characters, if $ROOT ever contains such characters (e.g., on unusual systems or misconfigured environments), the resulting JSON will be malformed.
Consider using a proper JSON encoding tool like jq for robustness:
ROOT_JSON=$(printf '%s' "$ROOT" | jq -Rs '.')This produces a fully escaped JSON string (including the surrounding quotes), so you'd adjust the args line accordingly.
| defer f.Close() | ||
|
|
||
| var summary SessionSummary | ||
| summary := SessionSummary{ControlPlane: "ocr-llm", TokenUsageAvailable: true} |
There was a problem hiding this comment.
The default ControlPlane value "ocr-llm" and TokenUsageAvailable: true are duplicated in both peekSession (line 132) and LoadSession (line 285). If this default ever needs to change, both locations must be updated in sync, which is error-prone. Consider extracting these into a package-level constant or a constructor function, e.g.:
const defaultControlPlane = "ocr-llm"
func newDefaultSessionSummary() SessionSummary {
return SessionSummary{ControlPlane: defaultControlPlane, TokenUsageAvailable: true}
}This ensures consistency between the session list view (peekSession) and the session detail view (LoadSession).
| <tr> | ||
| <td>{{.Event}}</td> | ||
| <td>{{if .BundleID}}<code>{{.BundleID}}</code>{{else}}-{{end}}</td> | ||
| <td>{{.DurationMS}} ms</td> |
There was a problem hiding this comment.
Minor UX issue: When duration_ms is absent from the event record, DurationMS defaults to 0 and the template displays "0 ms", which may be misleading. Consider adding a conditional check similar to how BundleID and Error are handled, e.g.:
<td>{{if .DurationMS}}{{.DurationMS}} ms{{else}}-{{end}}</td>
This would make it clearer that the duration was not recorded rather than being zero.
Suggestion:
| <td>{{.DurationMS}} ms</td> | |
| <td>{{if .DurationMS}}{{.DurationMS}} ms{{else}}-{{end}}</td> |
| if ref := p.FileReader.Ref; ref != "" { | ||
| cmdArgs = append(cmdArgs, "--end-of-options") | ||
| cmdArgs = append(cmdArgs, ref) | ||
| } |
There was a problem hiding this comment.
Removing --end-of-options from the grep command eliminates a defense-in-depth layer. While resolveGrepRef now validates and resolves the ref to a commit SHA before it reaches buildGrepArgs, keeping --end-of-options would provide an additional safety net against future regressions (e.g., if someone adds a new call site for buildGrepArgs that doesn't go through resolveGrepRef). Since the resolved value is always a hex commit SHA, re-adding --end-of-options here has no functional downside.
Suggestion:
| if ref := p.FileReader.Ref; ref != "" { | |
| cmdArgs = append(cmdArgs, "--end-of-options") | |
| cmdArgs = append(cmdArgs, ref) | |
| } | |
| if ref := p.FileReader.Ref; ref != "" { | |
| cmdArgs = append(cmdArgs, "--end-of-options", ref) | |
| } |
| // extFromPath is retained for native package compatibility. | ||
| func extFromPath(path string) string { | ||
| return scanExtension(path) | ||
| } |
There was a problem hiding this comment.
Dead code: extFromPath has no callers within the scan package. The only previous caller was whyExcluded in agent.go, which was refactored in this same changeset to call ExcludeReason directly. Since extFromPath is unexported, it cannot be called from outside the package either. The comment says "retained for native package compatibility" but there is nothing to be compatible with. Consider removing it to avoid unnecessary API surface and confusion.
|
Thanks a lot for picking this back up and carefully working through all the earlier bot feedback — this is a substantial, well-structured contribution, and the two-layer split (OCR as the deterministic data plane, the host agent as the reasoning layer) is exactly the right direction. Splitting Two directions we'd love to see this evolve toward: 1. An agent-agnostic ("host-agnostic") tooling model. 2. Zero impact on the existing The refactor that extracts the inline file-filter logic from if f.FileFilter != nil && f.FileFilter.HasInclude() {
if !f.FileFilter.IsUserIncluded(path) {
return model.ExcludeUserRule
}
}Neither the old review path nor the old scan path had this branch. Previously, a file not matching the The new semantics are arguably more intuitive, but they do change what native (Minor, same theme: Overall: love the design and the discipline here. If we can (1) generalize it beyond Codex and (2) guarantee the native kernel behavior is untouched, this becomes a really elegant and safe foundation. Thanks again for the great work! |
|
Thanks for the detailed review — agreed on both points. I’ll first address the hard compatibility requirement in this PR: the On the host-agnostic direction, I agree as well. The deterministic bundle/context/validate-comments/report layer is not inherently Codex-specific. To avoid expanding this already-large PR too much, I’d prefer to keep this PR focused on the Codex-first adapter plus compatibility fixes, then follow up with a separate generalization PR for a neutral surface such as I’ll push the behavior-preserving include fix first. |
|
Pushed the behavior-preserving include fix in What changed:
Verification:
|
munsunouk
force-pushed
the
codex/fix-codex-owned-review
branch
from
24c91bb to
a24764d
Compare
|
Thanks for the quick turnaround — I verified Fully agree on keeping this PR focused and landing the host-agnostic generalization separately — that's the right scope call. One thing I'd like to raise before that follow-up, specifically about the CLI surface we commit to here: I'd suggest not shipping The concern is both debt and newcomer UX:
Since I don't think this needs to expand the PR, two low-cost options:
Either way, the guiding principle would be: host differences (codex/claude/cursor) live entirely in the adapter layer (plugin/skill/MCP), and the CLI top level stays limited to capability commands like Not a blocker for the compatibility work — just want to settle the public surface before |
|
Thanks, that makes sense to me. I agree that I’ll update this PR so To keep this PR scoped, I’ll limit the change to the CLI, docs, plugin, and skill instructions. I won’t rename broad internal package/type/schema identifiers here unless they are user-facing or required for tests. A deeper protocol/internal rename can be handled separately if we decide it is worth doing. |
|
Updated this PR to make I removed the I also made the user-facing observability path agent-neutral: I intentionally kept broader internal/schema names unchanged to avoid expanding this PR beyond the public CLI/docs/plugin surface. Validation: |
Remove sync.Once caching in ContextService so cancelled contexts do not poison later reads, validate bundle IDs when loading plain bundles, omit content hashes for deleted files, and drop redundant chmod in private output writes. Add cmd and unit tests for path escape, bundle ID mismatch, and cancelled-context recovery.
Document --preview, --output, scan tuning, and manifest --bundle-index usage in CODEX_USAGE and host-agent SKILL files. Align CODEX_SKILL_FEASIBILITY with ocr agent commands and expand bundle schema docs for validation codes, manifest identity hashing, and partial semantics.
munsunouk
force-pushed
the
codex/fix-codex-owned-review
branch
2 times, most recently
from
3ab8e10 to
e0d607a
Compare
munsunouk
force-pushed
the
codex/fix-codex-owned-review
branch
7 times, most recently
from
95d0778 to
f8d30a5
Compare
munsunouk
force-pushed
the
codex/fix-codex-owned-review
branch
3 times, most recently
from
00dd088 to
7c2bf4a
Compare
|
Thanks again for the detailed feedback here — especially the push toward a host-agnostic surface and the requirement to keep the existing I reworked the landing plan to make the review scope smaller and cleaner. Instead of continuing this as one large PR, I’m splitting it into a stacked series and opening the first PR first:
I’m starting with PR1 only so the core runtime/protocol/session/reporting layer can be reviewed independently before I layer the surface rename and the Cursor adapter and CI bridge on top. New PR1: #301 I’ll use that as the canonical review path going forward, and then follow with PR2/PR3 after PR1 direction is confirmed. This PR can then be closed as superseded once the split path is in place. Thanks again — the discussion on this PR directly shaped the split. |
4 participants
Summary
This re-submits the Codex-owned review flow from #245 and addresses the OCR bot feedback from that PR.
The change keeps OCR as the deterministic data plane for host-agent-led reviews:
ocr agent preparebuilds review bundles/manifests without invoking OCR's LLM backendocr agent contextexposes target-aware read/diff/search helpersocr agent validate-commentsvalidates agent findings against immutable bundle evidenceocr agent reportrenders validated findingsThis is intended to address the Codex-native workflow requested in #275 without requiring OCR to call Codex CLI as a chat-completions provider.
Maintainer follow-up
ocr review/ocr scaninclude-filter behavior; strict include allowlist semantics are intentionally left out of this PR.ocr agent ...CLI surface in this PR and removed the publicocr codexcommand path.codex_event/codex-ownedrecords has also been removed.OCR bot feedback addressed
ocr codex$defsnamingpartialbudget-drivenocr codexCLI surface in favor ofocr agentcodex_event/codex-ownedviewer recordsTests
GOTOOLCHAIN=auto go test ./...GOTOOLCHAIN=auto go build -o dist/opencodereview ./cmd/opencodereview