feat(sync): add rules and hooks#127
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive support for managed resources, specifically rules and hooks, across the CLI and API server. It enables collecting, managing, and syncing these resources alongside skills, supported by new UI components for CRUD operations and enhanced API endpoints. Feedback identifies several critical areas for improvement: the restore process and file writing in the sync logic should be made atomic to ensure data consistency and prevent destructive failures. Additionally, increasing backup timestamp precision is recommended to avoid directory collisions, and the TOML patching regex should be refined to handle multiple inline tables correctly. Robustness can be further improved by supporting JSON with comments during unmarshaling and refactoring duplicated utility functions into a shared internal package.
internal/backup/restore.go
Outdated
| if err := replaceRestorePath(resolvedPath); err != nil { | ||
| return err | ||
| } | ||
| if err := os.MkdirAll(filepath.Dir(resolvedPath), 0o755); err != nil { | ||
| return fmt.Errorf("failed to create restore directory: %w", err) | ||
| } | ||
| if err := copyFile(storagePath, resolvedPath); err != nil { | ||
| return fmt.Errorf("failed to restore file %s: %w", resolvedPath, err) | ||
| } |
There was a problem hiding this comment.
The restore process for files and directories is not atomic and is potentially destructive. It deletes the existing path at the destination (replaceRestorePath) before attempting to copy the new one from the backup (copyFile or copyDir). If the copy operation fails (e.g., due to disk space, permissions, or an interruption), the original data is already gone, and the destination is left in a broken state. To prevent data loss and ensure consistency, you should copy the backup data to a temporary location on the same filesystem first, and then use an atomic rename to replace the destination.
cmd/skillshare/managed_resources.go
Outdated
| if err := os.MkdirAll(filepath.Dir(file.Path), 0o755); err != nil { | ||
| return nil, nil, err | ||
| } | ||
| if err := os.WriteFile(file.Path, []byte(file.Content), 0o644); err != nil { |
There was a problem hiding this comment.
Writing the compiled resource file directly using os.WriteFile is not atomic. If the write operation is interrupted, the target file could be left in a truncated or corrupted state. It is safer to write the content to a temporary file in the same directory and then rename it to the target path, ensuring that the file is either fully updated or remains unchanged. This pattern is already correctly implemented in the hook and rule stores.
internal/backup/manifest.go
Outdated
| return "", fmt.Errorf("cannot determine backup directory: home directory not found") | ||
| } | ||
|
|
||
| timestamp := time.Now().Format("2006-01-02_15-04-05") |
There was a problem hiding this comment.
The timestamp format used for the backup directory name has only second precision. If multiple snapshots are created within the same second for the same target (which can happen in automated scripts or rapid CLI usage), they will collide, leading to data corruption or overwriting of previous snapshots. Consider adding sub-second precision or a unique suffix to the directory name.
| timestamp := time.Now().Format("2006-01-02_15-04-05") | |
| timestamp := time.Now().Format("2006-01-02_15-04-05.000") |
| codexFeaturesHeaderRE = regexp.MustCompile(`^\s*\[features\]\s*(?:#.*)?$`) | ||
| codexAnyTableHeaderRE = regexp.MustCompile(`^\s*\[\[?[^\[\]]+\]\]?\s*(?:#.*)?$`) | ||
| codexHooksAssignmentRE = regexp.MustCompile(`^(\s*codex_hooks\s*=\s*)([^#\r\n]*?)(\s*(#.*)?)$`) | ||
| codexInlineFeaturesRE = regexp.MustCompile(`^(\s*features\s*=\s*\{)(.*)(\}\s*(#.*)?)$`) |
There was a problem hiding this comment.
The codexInlineFeaturesRE regex uses a greedy match (.*) which can cause issues if a line contains multiple inline tables (e.g., features = { ... }, other = { ... }). The regex will match from the start of the first table to the end of the last one, potentially leading to incorrect patching or corruption of the TOML file. While the anchors ^ and $ mitigate this for simple cases, a more robust parsing approach or a non-greedy match with careful boundary checking would be safer.
| func mergeJSONConfig(raw string, updates map[string]any) (string, error) { | ||
| doc := map[string]any{} | ||
| if strings.TrimSpace(raw) != "" { | ||
| if err := json.Unmarshal([]byte(raw), &doc); err != nil { |
There was a problem hiding this comment.
Using json.Unmarshal on the raw configuration string will fail if the file contains comments (JSONC). Since many users include comments in their configuration files (especially for tools like Claude), this could cause sync operations to fail unexpectedly. Consider using a JSON parser that supports comments or stripping them before unmarshaling to improve robustness.
| func applyCompiledFiles(files []adapters.CompiledFile, dryRun bool) ([]string, []string, error) { | ||
| if files == nil { | ||
| files = []adapters.CompiledFile{} | ||
| } | ||
|
|
||
| sorted := append([]adapters.CompiledFile(nil), files...) | ||
| sort.Slice(sorted, func(i, j int) bool { | ||
| return sorted[i].Path < sorted[j].Path | ||
| }) | ||
|
|
||
| updated := make([]string, 0, len(sorted)) | ||
| skipped := make([]string, 0, len(sorted)) | ||
| for _, file := range sorted { | ||
| same, err := compiledFileMatches(file.Path, file.Content) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| if same { | ||
| skipped = append(skipped, file.Path) | ||
| continue | ||
| } | ||
|
|
||
| updated = append(updated, file.Path) | ||
| if dryRun { | ||
| continue | ||
| } | ||
| if err := os.MkdirAll(filepath.Dir(file.Path), 0o755); err != nil { | ||
| return nil, nil, err | ||
| } | ||
| if err := os.WriteFile(file.Path, []byte(file.Content), 0o644); err != nil { | ||
| return nil, nil, err | ||
| } | ||
| } | ||
|
|
||
| return updated, skipped, nil | ||
| } |
There was a problem hiding this comment.
The applyCompiledFiles function is identical to the one defined in cmd/skillshare/managed_resources.go. To improve maintainability and avoid logic drift, consider moving this shared logic to a common internal package (e.g., internal/resources/apply).
salmonumbrella
force-pushed
the
pr/managed-rules-hooks-ui-parity
branch
from
75eef73 to
6fcc90c
Compare
salmonumbrella
changed the title
salmonumbrella
changed the title
salmonumbrella
changed the title
salmonumbrella
changed the title
salmonumbrella
changed the title
salmonumbrella
force-pushed
the
pr/managed-rules-hooks-ui-parity
branch
from
6fcc90c to
ac54308
Compare
|
@runkids handing off to you. big one. updated to handle |
1 participant
rulesandhooksparity with skill-style browsing, editing, sync, collect, restore/requestshooks-n-rules-for-willie.mp4
@runkids it needs heavy lifting from you; this pairs well with #126