refactor to codex plugins/skills#245
Conversation
|
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| Commands: | ||
| review, r Start a diff-based code review | ||
| codex Build deterministic inputs for a Codex-led review |
There was a problem hiding this comment.
Minor consistency issue: The help footer includes Use "ocr <cmd> -h" hints for review, scan, rules, config, and llm, but there is no corresponding hint for the new codex command. Consider adding one for consistency, e.g.:
Use "ocr codex -h" for more information about codex.
| if err := recordCodexEvent( | ||
| repoDir, | ||
| options.sessionID, | ||
| bundle.BundleID, | ||
| "report", | ||
| session.CodexEvent{ | ||
| Files: comments.Summary.FilesReviewed, | ||
| Findings: len(comments.Comments), | ||
| Warnings: len(comments.Warnings), | ||
| DurationMS: time.Since(started).Milliseconds(), | ||
| ValidationValid: &valid, | ||
| FilesReviewed: paths, | ||
| }, | ||
| true, | ||
| ); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Bug: The session is finalized (finalize=true) before the report is actually rendered. This causes two problems:
- Incorrect duration:
DurationMSmeasures only flag parsing + input loading time, not the actual report rendering which happens after this call. - Premature finalization: If
RenderReporton line 74 fails, the session has already been marked as successfully completed with metrics, but no report was produced. This creates misleading session records.
Consider moving the recordCodexEvent call to after RenderReport succeeds (and include rendering time in the duration), or at minimum move it after rendering so that a render failure prevents finalization.
| if manifest.ManifestID == "" || manifest.Bundles == nil { | ||
| return nil, fmt.Errorf("invalid scan manifest schema: manifest_id and bundles are required") | ||
| } |
There was a problem hiding this comment.
Combining two distinct required-field checks into a single condition and error message reduces diagnostic clarity. When only one of the fields is missing, the caller receives a generic error mentioning both fields, making it harder to identify the actual problem. Consider separating them for consistency with LoadBundle and LoadComments, which each validate individual fields with specific error messages.
Suggestion:
| if manifest.ManifestID == "" || manifest.Bundles == nil { | |
| return nil, fmt.Errorf("invalid scan manifest schema: manifest_id and bundles are required") | |
| } | |
| if manifest.ManifestID == "" { | |
| return nil, fmt.Errorf("invalid scan manifest schema: manifest_id is required") | |
| } | |
| if manifest.Bundles == nil { | |
| return nil, fmt.Errorf("invalid scan manifest schema: bundles is required") | |
| } |
| for _, pattern := range patterns { | ||
| if _, safe := cleanProtocolPath(pattern); !safe { | ||
| return ContextResult{}, &ProtocolError{Code: "path_escape", Message: "file pattern must stay inside the repository"} | ||
| } | ||
| patternArguments = append(patternArguments, pattern) | ||
| } |
There was a problem hiding this comment.
Security Bug: Path traversal bypass in Search. The cleanProtocolPath check validates the cleaned path but discards the cleaned result (_) and appends the original uncleaned pattern to patternArguments. This means a malicious pattern like ../../etc/passwd would pass the safety check (since cleanProtocolPath would detect it as unsafe), but a subtly crafted pattern that normalizes differently could potentially bypass validation. More importantly, this is inconsistent with how Read and Diff correctly use the cleaned value. The cleaned value should be appended instead of the original.
Suggestion:
| for _, pattern := range patterns { | |
| if _, safe := cleanProtocolPath(pattern); !safe { | |
| return ContextResult{}, &ProtocolError{Code: "path_escape", Message: "file pattern must stay inside the repository"} | |
| } | |
| patternArguments = append(patternArguments, pattern) | |
| } | |
| 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) | |
| } |
| if maxLines <= 0 { | ||
| maxLines = 200 | ||
| } | ||
| endLine := startLine + maxLines - 1 |
There was a problem hiding this comment.
Doc/code mismatch: Missing upper bound on maxLines. The doc comment says "at most 500 target-version lines" but the code only sets a default of 200 when maxLines <= 0 and does not enforce any upper cap. While the underlying FileReadProvider internally caps at 500 lines (fileReadMaxLines), this method's contract should match its documentation. Either enforce the 500-line cap explicitly here for defense-in-depth, or update the doc comment to reflect the actual behavior.
Suggestion:
| if maxLines <= 0 { | |
| maxLines = 200 | |
| } | |
| endLine := startLine + maxLines - 1 | |
| if maxLines <= 0 { | |
| maxLines = 200 | |
| } | |
| if maxLines > 500 { | |
| maxLines = 500 | |
| } | |
| endLine := startLine + maxLines - 1 |
|
|
||
| exec codex --yolo -C "$ROOT" \ | ||
| -c 'mcp_servers.codegraph.command="codegraph"' \ | ||
| -c "mcp_servers.codegraph.args=[\"serve\",\"--mcp\",\"--path\",\"$ROOT\"]" \ |
There was a problem hiding this comment.
Potential issue: $ROOT is interpolated inside a double-quoted string that represents a JSON array value. If the resolved path contains characters like double quotes, backslashes, or other special characters, this could produce malformed input to codex. Consider escaping $ROOT for safe embedding within the JSON-like structure, or at minimum document the assumption that the project path must not contain such characters.
For example, a path like /home/user/my "project" would break the args array syntax.
| if value, ok := rec["controlPlane"].(string); ok { | ||
| vs.Summary.ControlPlane = value | ||
| } | ||
| if value, ok := rec["bundleId"].(string); ok { | ||
| vs.Summary.BundleID = value | ||
| } | ||
| if value, ok := rec["tokenUsage"].(string); ok && value == "not_available" { | ||
| vs.Summary.TokenUsageAvailable = false | ||
| } |
There was a problem hiding this comment.
Maintainability concern: The parsing logic for controlPlane, bundleId, and tokenUsage is duplicated verbatim between peekSession (lines 171-179) and LoadSession (lines 336-344). This was already a pre-existing pattern for other session_start fields, but adding three more fields increases the divergence risk. If a new field is added or parsing logic changes (e.g., a different sentinel value for tokenUsage), both locations must be updated consistently.
Consider extracting a shared helper like parseSessionStartFields(rec map[string]any, summary *SessionSummary) that both functions can call. This would reduce the chance of subtle bugs where one path is updated but the other isn't.
| content, _ := json.Marshal(details) | ||
| var fields map[string]any | ||
| _ = json.Unmarshal(content, &fields) |
There was a problem hiding this comment.
Bug (latent): Errors from json.Marshal and json.Unmarshal are silently discarded. If either fails (e.g., a future field addition introduces an unmarshalable type), fields will be nil and the subsequent map assignments (line 108+) will panic at runtime.
While CodexEvent currently contains only safely serializable types, this silent error suppression creates a fragile contract. Consider initializing fields defensively and/or handling the errors explicitly.
Suggestion:
| content, _ := json.Marshal(details) | |
| var fields map[string]any | |
| _ = json.Unmarshal(content, &fields) | |
| content, err := json.Marshal(details) | |
| if err != nil { | |
| // Fallback to empty map so callers never receive nil. | |
| content = []byte("{}") | |
| } | |
| fields := make(map[string]any) | |
| _ = json.Unmarshal(content, &fields) |
| if err := file.Chmod(0o600); err != nil { | ||
| _ = file.Close() | ||
| return fmt.Errorf("restrict Codex session: %w", err) | ||
| } |
There was a problem hiding this comment.
Redundant Chmod: os.OpenFile already creates the file with mode 0o600. Calling file.Chmod(0o600) on every single write is unnecessary — it adds a syscall per record without changing anything. For existing files, the mode passed to os.OpenFile is ignored (only used at creation), but the file was already created with 0o600 permissions. Remove this block to avoid the per-write overhead.
| if errors.Is(err, context.Canceled) { | ||
| return "", err | ||
| } | ||
| return fmt.Sprintf("Error: invalid git ref %q: %v", p.FileReader.Ref, err), nil |
There was a problem hiding this comment.
Bug: When resolveGrepRef fails due to a context deadline (timeout), the error falls through to the generic "invalid git ref" message, producing a misleading user-facing error like "Error: invalid git ref \"main\": context deadline exceeded". You should also check for context.DeadlineExceeded and propagate it as a real error, similar to how context.Canceled is handled.
Suggestion:
| if errors.Is(err, context.Canceled) { | |
| return "", err | |
| } | |
| return fmt.Sprintf("Error: invalid git ref %q: %v", p.FileReader.Ref, err), nil | |
| if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { | |
| return "", err | |
| } | |
| return fmt.Sprintf("Error: invalid git ref %q: %v", p.FileReader.Ref, err), nil |
2 participants
Description
Type of Change
How Has This Been Tested?
make testpasses locallyChecklist
go fmt,go vet)Related Issues