feat(delegate): add delegate mode for host-agent code review#364
feat(delegate): add delegate mode for host-agent code review#364lizhengfeng101 wants to merge 1 commit into
Conversation
Add `ocr delegate` command that prepares a deterministic review bundle and emits a Markdown workflow for host agents to follow. This enables code review without requiring an API key — the host agent uses its own reasoning to review, then validates and reports via OCR's anti-hallucination pipeline. Key components: - `ocr delegate` — prepare bundle + emit workflow - `ocr delegate validate` — verify comments against bundle evidence - `ocr delegate report` — render validated comments as report - `internal/reviewbundle` — bundle preparation, validation, reporting - `internal/reviewfilter` — file filtering by glob patterns - Neutral schema naming (review-bundle/v1, review-comments/v1) - Updated SKILL.md and plugin commands to dual-mode (Mode A/B)
|
🔍 OpenCodeReview found 19 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| delegate Prepare evidence and emit a host-agent review workflow | ||
| session, sessions List and inspect saved review sessions |
There was a problem hiding this comment.
The delegate command is listed in the Commands section but is missing a corresponding "Use ... for more information" hint line at the bottom of the usage text, unlike all other subcommands (review, scan, rules, config, llm, session). For consistency, consider adding:
Use "ocr delegate -h" for more information about delegate.
Also consider adding an example usage line in the Examples section (e.g., ocr delegate --from master --to dev).
| ) | ||
|
|
||
| func writePrivateFile(path string, content []byte) error { | ||
| file, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o600) |
There was a problem hiding this comment.
On Unix systems, when os.OpenFile opens an existing file with O_TRUNC, the mode argument (0o600) is ignored — it only takes effect when creating a new file via O_CREATE. If the file already exists with more permissive permissions (e.g., 0o644), this function will truncate and rewrite it while preserving the original permissive mode, potentially exposing sensitive bundle/validation data.
Consider explicitly setting permissions with os.Chmod after opening, or removing the file first before recreating it, to ensure the restrictive permissions are always applied.
Suggestion:
| file, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o600) | |
| // Remove existing file first so O_CREATE applies the mode. | |
| _ = os.Remove(path) | |
| file, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_EXCL, 0o600) |
| if startLine-1 > totalLines || (totalLines > 0 && startLine-1 >= totalLines) { | ||
| return "", fmt.Errorf("file %q has only %d lines, requested range starting at %d", path, totalLines, startLine) | ||
| } |
There was a problem hiding this comment.
The boundary check for startLine has a redundant first condition and produces misleading output for empty files. When totalLines == 0 and startLine == 1, neither condition triggers (0 > 0 is false, and totalLines > 0 is false), so execution continues and emits LINE_RANGE: 1-0 with zero content lines. The two conditions can be simplified to a single clear check, and empty files should be handled explicitly.
Suggestion:
| if startLine-1 > totalLines || (totalLines > 0 && startLine-1 >= totalLines) { | |
| return "", fmt.Errorf("file %q has only %d lines, requested range starting at %d", path, totalLines, startLine) | |
| } | |
| if startLine > totalLines { | |
| return "", fmt.Errorf("file %q has only %d lines, requested range starting at %d", path, totalLines, startLine) | |
| } |
| 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.
ready() calls validateFreshTarget on every single Read/Find/Diff/Search operation. For scan-mode bundles, this reads and hashes every file from disk each time; for git-based targets, it runs ResolveTarget (git commands). In workflows that make many sequential context queries, this creates significant cumulative latency. Consider caching the validation result (e.g., validate once in the constructor or add a TTL-based cache) to avoid redundant I/O and git operations.
| return true | ||
| } | ||
| for _, pattern := range patterns { | ||
| if matched, _ := filepath.Match(pattern, path); matched { |
There was a problem hiding this comment.
filepath.Match uses OS-specific path separators. On Windows, it treats backslash as a separator, but bundle file paths consistently use forward slashes (as seen in cleanProtocolPath which normalizes via filepath.ToSlash). This means patterns like src/*.go will fail to match on Windows because filepath.Match won't interpret / as a separator. Use path.Match instead, which always uses forward slashes, or normalize both pattern and path before matching.
Suggestion:
| if matched, _ := filepath.Match(pattern, path); matched { | |
| if matched, _ := path.Match(pattern, path); matched { |
| if f.FileFilter != nil && f.FileFilter.HasInclude() && f.FileFilter.IsUserIncluded(path) { | ||
| return model.ExcludeNone | ||
| } | ||
|
|
||
| extension := extensionFromPath(path) | ||
| if extension != "" && !allowedext.IsAllowedExt(extension) { | ||
| return model.ExcludeExtension | ||
| } | ||
| if allowedext.IsExcludedPath(path) { | ||
| return model.ExcludeDefaultPath | ||
| } | ||
| return model.ExcludeNone |
There was a problem hiding this comment.
Bug: Include-list semantics are inverted. When HasInclude() is true but IsUserIncluded(path) returns false, execution falls through to extension/path checks and can still return ExcludeNone at line 39. This means files that do NOT match any user include pattern are still included in review — defeating the purpose of an explicit include list.
When a user configures an include list, the expected behavior is: only files matching the include patterns should be reviewed; everything else should be excluded. The current logic only short-circuits to ExcludeNone when a file IS included, but does not exclude files that fail the include check.
The same issue exists in ExcludeScanReason (line 54-56).
Suggestion:
| if f.FileFilter != nil && f.FileFilter.HasInclude() && f.FileFilter.IsUserIncluded(path) { | |
| return model.ExcludeNone | |
| } | |
| extension := extensionFromPath(path) | |
| if extension != "" && !allowedext.IsAllowedExt(extension) { | |
| return model.ExcludeExtension | |
| } | |
| if allowedext.IsExcludedPath(path) { | |
| return model.ExcludeDefaultPath | |
| } | |
| return model.ExcludeNone | |
| if f.FileFilter != nil && f.FileFilter.HasInclude() { | |
| if f.FileFilter.IsUserIncluded(path) { | |
| return model.ExcludeNone | |
| } | |
| return model.ExcludeUserRule | |
| } | |
| extension := extensionFromPath(path) | |
| if extension != "" && !allowedext.IsAllowedExt(extension) { | |
| return model.ExcludeExtension | |
| } | |
| if allowedext.IsExcludedPath(path) { | |
| return model.ExcludeDefaultPath | |
| } | |
| return model.ExcludeNone |
| // ExcludeScanReason preserves the native full-scan filter ordering. | ||
| func (f Filter) ExcludeScanReason(path string, isBinary bool) model.ExcludeReason { |
There was a problem hiding this comment.
Bug: Inconsistent filter ordering between ExcludeReason and ExcludeScanReason. These two methods apply the same set of checks but in different order, which can produce different exclusion results for the same file:
ExcludeReason: user-exclude → user-include → extension → default-pathExcludeScanReason: user-exclude → extension → user-include → default-path
For example, a file matching a user include pattern but having a disallowed extension would be included by ExcludeReason (include check short-circuits before extension) but excluded by ExcludeScanReason (extension check runs first). This inconsistency means diff-based review and full-scan review can classify the same file differently.
Consider aligning both methods to use the same canonical ordering, or documenting why the difference is intentional.
| return "sha256:" + hex.EncodeToString(hasher.Sum(nil)) | ||
| } | ||
|
|
||
| func writeHashFields(hasher hash.Hash, length [8]byte, fields ...[]byte) { |
There was a problem hiding this comment.
The length array is passed by value to writeHashFields, meaning the function operates on a copy. While the current code happens to work correctly (because line 217 directly writes into the caller's own length before using it on line 218), this pattern is fragile and misleading. A future maintainer might reasonably assume that writeHashFields updates the caller's length buffer, leading to subtle hash-corruption bugs.
Consider passing a pointer to make the data flow explicit and prevent future mistakes:
func writeHashFields(hasher hash.Hash, length *[8]byte, fields ...[]byte) {
for _, field := range fields {
binary.BigEndian.PutUint64(length[:], uint64(len(field)))
_, _ = hasher.Write(length[:])
_, _ = hasher.Write(field)
}
}This also makes the regular-file branch at lines 216-218 clearly share the same buffer with writeHashFields, eliminating any ambiguity about whether state leaks between calls.
| if err != nil || hashFields(content) != file.ContentSHA256 { | ||
| addValidationError( | ||
| result, | ||
| "stale_bundle", | ||
| file.Path, | ||
| nil, | ||
| "scan file changed after bundle creation", | ||
| ) |
There was a problem hiding this comment.
When readTargetFile returns an error (e.g., file not found, permission denied, symlink escape), the code still reports "scan file changed after bundle creation" which is misleading. Additionally, hashFields(content) is called on nil/empty content when err != nil, which is harmless but unnecessary. Consider separating the error cases to provide more accurate diagnostics.
Suggestion:
| if err != nil || hashFields(content) != file.ContentSHA256 { | |
| addValidationError( | |
| result, | |
| "stale_bundle", | |
| file.Path, | |
| nil, | |
| "scan file changed after bundle creation", | |
| ) | |
| if err != nil { | |
| addValidationError( | |
| result, | |
| "stale_bundle", | |
| file.Path, | |
| nil, | |
| fmt.Sprintf("cannot verify scan file: %v", err), | |
| ) | |
| } else if hashFields(content) != file.ContentSHA256 { | |
| addValidationError( | |
| result, | |
| "stale_bundle", | |
| file.Path, | |
| nil, | |
| "scan file changed after bundle creation", | |
| ) |
| if bundle.SchemaVersion != BundleSchemaVersion || | ||
| comments.SchemaVersion != CommentsSchemaVersion { | ||
| addValidationError(&result, "invalid_schema", "", nil, "unsupported protocol schema version") | ||
| } | ||
| if comments.BundleID != bundle.BundleID { | ||
| addValidationError( | ||
| &result, | ||
| "bundle_id_mismatch", | ||
| "", | ||
| nil, | ||
| "comments bundle_id does not match the review bundle", | ||
| ) | ||
| } |
There was a problem hiding this comment.
After detecting a schema version mismatch or bundle ID mismatch, the function continues to validate individual comments and target freshness. These are fundamental protocol-level errors that make all subsequent validation unreliable — for example, if the schema version differs, field semantics may have changed, and if the bundle ID doesn't match, the comments were produced for a different review entirely. Consider returning early after these critical errors to avoid producing misleading downstream diagnostics.
Suggestion:
| if bundle.SchemaVersion != BundleSchemaVersion || | |
| comments.SchemaVersion != CommentsSchemaVersion { | |
| addValidationError(&result, "invalid_schema", "", nil, "unsupported protocol schema version") | |
| } | |
| if comments.BundleID != bundle.BundleID { | |
| addValidationError( | |
| &result, | |
| "bundle_id_mismatch", | |
| "", | |
| nil, | |
| "comments bundle_id does not match the review bundle", | |
| ) | |
| } | |
| if bundle.SchemaVersion != BundleSchemaVersion || | |
| comments.SchemaVersion != CommentsSchemaVersion { | |
| addValidationError(&result, "invalid_schema", "", nil, "unsupported protocol schema version") | |
| result.Valid = false | |
| return result | |
| } | |
| if comments.BundleID != bundle.BundleID { | |
| addValidationError( | |
| &result, | |
| "bundle_id_mismatch", | |
| "", | |
| nil, | |
| "comments bundle_id does not match the review bundle", | |
| ) | |
| result.Valid = false | |
| return result | |
| } |
1 participant
Summary
Add
ocr delegatecommand that enables code review without requiring an API key. The host agent (e.g. Claude Code, Cursor) uses its own reasoning to review code changes, while OCR provides the deterministic data plane — bundle preparation, anti-hallucination validation, and report rendering.Key changes:
ocr delegate— prepares a review bundle and emits a Markdown workflow for the host agent to followocr delegate validate— verifies review comments against immutable bundle evidence (anti-hallucination)ocr delegate report— renders validated comments as a formatted reportinternal/reviewbundle— full package for bundle preparation, validation, reporting, and schema enforcementinternal/reviewfilter— file filtering by glob patternsreview-bundle/v1,review-comments/v1(no vendor prefix)ocr llm testHow it works:
ocr delegate→ receives Markdown workflow + bundle filereview-comments/v1JSONocr delegate validatechecks every comment against bundle evidenceocr delegate reportrenders the final reportTest plan
make checkpasses (fmt + vet)make testpasses (all unit tests green)delegate_cmd_test.gocovering workspace/range/commit modes, validate/report roundtrips, flag validation, help output, and schema neutralityocr delegate→ construct comments → validate → report