Closed
Conversation
🔍 Workspace Sync Dry-Run Complete✅ Dry-run completed successfully - no changes were made Dry-Run Results |
5 similar comments
🔍 Workspace Sync Dry-Run Complete✅ Dry-run completed successfully - no changes were made Dry-Run Results |
🔍 Workspace Sync Dry-Run Complete✅ Dry-run completed successfully - no changes were made Dry-Run Results |
🔍 Workspace Sync Dry-Run Complete✅ Dry-run completed successfully - no changes were made Dry-Run Results |
🔍 Workspace Sync Dry-Run Complete✅ Dry-run completed successfully - no changes were made Dry-Run Results |
🔍 Workspace Sync Dry-Run Complete✅ Dry-run completed successfully - no changes were made Dry-Run Results |
Replaces the reusable workflow from gh-actions with an inline workflow that expands JSON string fields into real lines and uses a Python script to extract just the changed sentence, making long text diffs readable. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🔍 Workspace Sync Dry-Run Complete✅ Dry-run completed successfully - no changes were made Dry-Run Results |
2 similar comments
🔍 Workspace Sync Dry-Run Complete✅ Dry-run completed successfully - no changes were made Dry-Run Results |
🔍 Workspace Sync Dry-Run Complete✅ Dry-run completed successfully - no changes were made Dry-Run Results |
🔍 Workspace Sync Dry-Run Complete✅ Dry-run completed successfully - no changes were made Dry-Run Results |
🔍 Workspace Sync Dry-Run Complete✅ Dry-run completed successfully - no changes were made Dry-Run Results |
There was a problem hiding this comment.
Pull request overview
This PR replaces an external reusable workflow with an inline implementation for generating skill diffs on pull requests. The new workflow processes JSON files in the Skills directory, formats changes at the sentence level for easier review, and posts the results as a sticky PR comment.
Changes:
- Replaces external workflow call with inline job implementation including Python diff script
- Adds sentence-level change detection to show full context around modifications
- Implements truncation and formatting for PR comments with diff output
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| obj = json.loads(text) | ||
| s = json.dumps(obj, indent=2, sort_keys=True, ensure_ascii=False) | ||
| return s.replace('\\n', '\n') | ||
| except: |
There was a problem hiding this comment.
The bare exception clause except: on line 46 catches all exceptions including system exits and keyboard interrupts, which can mask critical errors. Replace with except (ValueError, json.JSONDecodeError): to catch only JSON parsing errors specifically.
Suggested change
| except: | |
| except (ValueError, json.JSONDecodeError): |
| """Find the start of the sentence containing pos.""" | ||
| i = pos - 1 | ||
| while i > 0: | ||
| if text[i] in '.!?' and i + 1 < len(text) and text[i + 1] == ' ': |
There was a problem hiding this comment.
Inconsistent sentence boundary detection logic. Line 53 uses and while line 62 correctly uses or for the same pattern. Line 53 should be changed to: if text[i] in '.!?' and (i + 1 >= len(text) or text[i + 1] == ' '): to properly handle sentence-ending punctuation at the end of a string or not followed by a space.
Suggested change
| if text[i] in '.!?' and i + 1 < len(text) and text[i + 1] == ' ': | |
| if text[i] in '.!?' and (i + 1 >= len(text) or text[i + 1] == ' '): |
Comment on lines
+89
to
+90
| old_text = expand(open(sys.argv[1]).read()) | ||
| new_text = expand(open(sys.argv[2]).read()) |
There was a problem hiding this comment.
Missing error handling for file operations. The open() calls on lines 89-90 should include error handling (try/except) or use context managers to ensure files are properly closed even if errors occur. Additionally, there's no validation that sys.argv has the required 3 arguments before accessing them.
Suggested change
| old_text = expand(open(sys.argv[1]).read()) | |
| new_text = expand(open(sys.argv[2]).read()) | |
| if len(sys.argv) < 4: | |
| sys.stderr.write("Usage: smart_diff.py <old_file> <new_file> <filename>\n") | |
| sys.exit(1) | |
| try: | |
| with open(sys.argv[1]) as f: | |
| old_text = expand(f.read()) | |
| with open(sys.argv[2]) as f: | |
| new_text = expand(f.read()) | |
| except OSError as e: | |
| sys.stderr.write(f"Error reading input files: {e}\n") | |
| sys.exit(1) |
Comment on lines
+166
to
+173
| if [ $(wc -c < "$diff_file") -gt 60000 ]; then | ||
| head -c 60000 "$diff_file" > "$diff_file.tmp" | ||
| { | ||
| cat "$diff_file.tmp" | ||
| echo "" | ||
| echo "" | ||
| echo "> :warning: Diff truncated. See the full diff in the Files tab." | ||
| } > "$diff_file" |
There was a problem hiding this comment.
The truncation logic may create invalid markdown. When truncating at exactly 60000 bytes on line 167, you might cut in the middle of a multi-byte UTF-8 character or in the middle of markdown syntax (like code fences). Consider truncating at line boundaries or using a safer method that respects UTF-8 character boundaries.
Comment on lines
+67
to
+81
| def trim_pair(old_line, new_line): | ||
| """Extract the sentence containing the change.""" | ||
| prefix_len = 0 | ||
| for i in range(min(len(old_line), len(new_line))): | ||
| if old_line[i] == new_line[i]: | ||
| prefix_len = i + 1 | ||
| else: | ||
| break | ||
| start = sentence_start(old_line, prefix_len) | ||
| end_old = sentence_end(old_line, prefix_len) | ||
| end_new = sentence_end(new_line, prefix_len) | ||
| return ( | ||
| old_line[start:end_old], | ||
| new_line[start:end_new], | ||
| ) |
There was a problem hiding this comment.
The trim_pair function has a potential issue when all characters match between old_line and new_line. In this case, prefix_len will equal the minimum length of both strings, but if the strings differ only in length (one is a prefix of the other), calling sentence_start and sentence_end with this position may not capture the intended change. Consider handling the case where one string is a complete prefix of the other.
Comment on lines
+109
to
+136
| if line.startswith('-') and not line.startswith('---'): | ||
| minus, plus = [], [] | ||
| while i < len(diff) and diff[i].startswith('-') and not diff[i].startswith('---'): | ||
| minus.append(diff[i][1:]) | ||
| i += 1 | ||
| while i < len(diff) and diff[i].startswith('+') and not diff[i].startswith('+++'): | ||
| plus.append(diff[i][1:]) | ||
| i += 1 | ||
| if len(minus) == len(plus): | ||
| for m, p in zip(minus, plus): | ||
| if max(len(m), len(p)) > 120: | ||
| tm, tp = trim_pair(m, p) | ||
| out.append(f"-{tm}") | ||
| out.append(f"+{tp}") | ||
| else: | ||
| out.append(f"-{m}") | ||
| out.append(f"+{p}") | ||
| else: | ||
| for m in minus: | ||
| out.append(f"-{trim_long(m)}") | ||
| for p in plus: | ||
| out.append(f"+{trim_long(p)}") | ||
| continue | ||
| if line.startswith(' '): | ||
| out.append(f" {trim_long(line[1:])}") | ||
| else: | ||
| out.append(line) | ||
| i += 1 |
There was a problem hiding this comment.
The diff processing logic only handles lines starting with '-' on line 109, but doesn't handle standalone additions (lines starting with '+' that aren't preceded by '-' lines). These will fall through to the else clause on line 134-135 and be output as-is without the trim_long optimization. Consider adding a separate condition to handle '+' lines: elif line.startswith('+') and not line.startswith('+++'): before the space-handling condition.
| fi | ||
|
|
||
| # Truncate if too large for a PR comment | ||
| if [ $(wc -c < "$diff_file") -gt 60000 ]; then |
There was a problem hiding this comment.
The command substitution on line 166 is missing quotes around the variable expansion. Change if [ $(wc -c < "$diff_file") -gt 60000 ]; then to if [ "$(wc -c < "$diff_file")" -gt 60000 ]; then to prevent word splitting and ensure robust behavior.
Suggested change
| if [ $(wc -c < "$diff_file") -gt 60000 ]; then | |
| if [ "$(wc -c < "$diff_file")" -gt 60000 ]; then |
| try: | ||
| obj = json.loads(text) | ||
| s = json.dumps(obj, indent=2, sort_keys=True, ensure_ascii=False) | ||
| return s.replace('\\n', '\n') |
There was a problem hiding this comment.
The replace('\\n', '\n') operation on line 45 appears to be attempting to handle escaped newlines in JSON strings, but this is unnecessary and potentially problematic. json.dumps() already properly handles newlines, and this replace will convert literal backslash-n sequences (which might be intentional in the JSON) into actual newlines. Consider removing this line unless there's a specific reason for it.
Suggested change
| return s.replace('\\n', '\n') | |
| return s |
| git show "${{ github.event.pull_request.base.sha }}:$file" > "$RUNNER_TEMP/old.json" 2>/dev/null || echo "{}" > "$RUNNER_TEMP/old.json" | ||
| git show "${{ github.event.pull_request.head.sha }}:$file" > "$RUNNER_TEMP/new.json" 2>/dev/null || echo "{}" > "$RUNNER_TEMP/new.json" | ||
|
|
||
| FILE_DIFF=$(python3 "$RUNNER_TEMP/smart_diff.py" "$RUNNER_TEMP/old.json" "$RUNNER_TEMP/new.json" "$file") |
There was a problem hiding this comment.
The shell script doesn't handle the case where the Python script fails. On line 145, if smart_diff.py exits with an error, the script continues executing and may produce incorrect or incomplete output. Consider adding error checking: FILE_DIFF=$(python3 "$RUNNER_TEMP/smart_diff.py" "$RUNNER_TEMP/old.json" "$RUNNER_TEMP/new.json" "$file") || { echo "Error processing $file" >&2; continue; }
Suggested change
| FILE_DIFF=$(python3 "$RUNNER_TEMP/smart_diff.py" "$RUNNER_TEMP/old.json" "$RUNNER_TEMP/new.json" "$file") | |
| FILE_DIFF=$(python3 "$RUNNER_TEMP/smart_diff.py" "$RUNNER_TEMP/old.json" "$RUNNER_TEMP/new.json" "$file") || { echo "Error processing $file" >&2; continue; } |
Comment on lines
+151
to
+153
| echo '````diff' | ||
| echo "$FILE_DIFF" | ||
| echo '````' |
There was a problem hiding this comment.
The markdown code fence uses 4 backticks on line 151 and 153, but this is unusual. Standard markdown uses 3 backticks. While 4 backticks can work (to allow 3 backticks inside), if the diff content contains 4 backticks, it could break the fence. Consider using standard 3 backticks or ensuring the fence uses more backticks than any content it contains.
Suggested change
| echo '````diff' | |
| echo "$FILE_DIFF" | |
| echo '````' | |
| echo '```diff' | |
| echo "$FILE_DIFF" | |
| echo '```' |
Contributor
Author
|
closing in favor of #47 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a diff checking workflow for skills (i.e. json files under Skills). The diff will capture the whole sentence of where the change was made so it's clearer to review.
Example: