Repository navigation
Distill figure revision heuristics - #1
Merged
Merged
Conversation
Reviewer's GuideStrengthens the systems-paper figurecraft skill by separating semantic vs style authority, adding semantic-rank/edge-inventory and micro-glyph guidance, refining revise-mode behavior and iteration rules, expanding figure grammars for provenance/contracts, and enforcing documentation hygiene via new tests. Flow diagram for revised Revise mode semantics and completion evidenceflowchart TD
U[User request] --> M[Select mode]
M -->|Revise| R[Revise mode]
R --> Q{"User asks for plan/critique only?"}
Q -->|Yes| P[Planning-only revision]
Q -->|No| E[Semantic asset revision]
P --> PD[Diagnosis + revision contract]
PD --> PG[Mark asset gates as N/A]
E --> SE[Bounded semantic edits]
SE --> VC[Semantic + visual checks]
VC --> CG[Consistency + compiled-paper gates]
style U fill:#f5f5f5,stroke:#333
style M fill:#f5f5f5,stroke:#333
style R fill:#f5f5f5,stroke:#333
style Q fill:#fff8e1,stroke:#333
style P fill:#e3f2fd,stroke:#333
style E fill:#e3f2fd,stroke:#333
style PD fill:#e8f5e9,stroke:#333
style PG fill:#e8f5e9,stroke:#333
style SE fill:#e8f5e9,stroke:#333
style VC fill:#e8f5e9,stroke:#333
style CG fill:#e8f5e9,stroke:#333
Flow diagram for semantic vs style authority in figure generationflowchart TD
PT[Paper text + captions] --> RV[Resolve with user's confirmed target version]
RV -->|Reconciled| SF[Reconciled source figure]
SF --> SA[Semantic authority]
SA --> GP[Generation prompt]
ST[Style reference image] --> STA[Style authority - visual tokens only]
STA --> GP
GP --> FG[Generated figure]
subgraph Semantic_channels
SA
end
subgraph Style_channels
STA
end
style PT fill:#f5f5f5,stroke:#333
style RV fill:#fff8e1,stroke:#333
style SF fill:#f5f5f5,stroke:#333
style SA fill:#e3f2fd,stroke:#333
style ST fill:#f5f5f5,stroke:#333
style STA fill:#e3f2fd,stroke:#333
style GP fill:#e8f5e9,stroke:#333
style FG fill:#e8f5e9,stroke:#333
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The
test_markdown_fragment_links_resolveslug logic manually normalizes headings and may diverge from the actual Markdown renderer’s anchor rules, especially for punctuation and non-ASCII titles (e.g., in the Chinese README); consider using a library or matching the renderer’s algorithm more closely to avoid false failures. - The
test_skill_body_stays_concisecheck hard-codes a 500-line limit on SKILL.md, which makes test outcomes sensitive to formatting and wrapping rather than semantic size; you might instead base this on a rough word count or exclude boilerplate sections to avoid brittle failures when the file is reformatted.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `test_markdown_fragment_links_resolve` slug logic manually normalizes headings and may diverge from the actual Markdown renderer’s anchor rules, especially for punctuation and non-ASCII titles (e.g., in the Chinese README); consider using a library or matching the renderer’s algorithm more closely to avoid false failures.
- The `test_skill_body_stays_concise` check hard-codes a 500-line limit on SKILL.md, which makes test outcomes sensitive to formatting and wrapping rather than semantic size; you might instead base this on a rough word count or exclude boilerplate sections to avoid brittle failures when the file is reformatted.
## Individual Comments
### Comment 1
<location path="tests/test_skill_package.py" line_range="62-71" />
<code_context>
+ if len(text.splitlines()) > 100:
+ self.assertIn("## Contents", text, reference.name)
+
+ def test_markdown_fragment_links_resolve(self) -> None:
+ documents = [ROOT / "README.md", ROOT / "README_en.md", *SKILL_DIR.rglob("*.md")]
+ for document in documents:
+ text = document.read_text(encoding="utf-8")
+ headings = {
+ re.sub(r"[^\w\- ]", "", match.group(1))
+ .strip()
+ .lower()
+ .replace(" ", "-")
+ for match in re.finditer(r"^#{1,6}\s+(.+?)\s*$", text, re.MULTILINE)
+ }
+ for target in re.findall(r"\]\(#([^)]+)\)", text):
+ self.assertIn(target, headings, f"{document.name}: #{target}")
+
+ def test_public_docs_have_no_user_paths_or_thread_ids(self) -> None:
</code_context>
<issue_to_address>
**issue (testing):** Fragment link test is currently misaligned with heading normalization and may flag valid links as failures.
Heading slugs are normalized (strip, lowercase, spaces→dashes, punctuation removed), but the `target` extracted from `"](#...)"` is compared raw. This means `#Section-One` won’t match the normalized slug `section-one`, even though Markdown renderers treat it as equivalent. Please normalize `target` with the same slugging logic (or factor out a shared slug function and compare those) before `assertIn`, so the test only fails on genuinely broken links, not casing or punctuation differences.
</issue_to_address>
### Comment 2
<location path="tests/test_skill_package.py" line_range="77-80" />
<code_context>
+ for target in re.findall(r"\]\(#([^)]+)\)", text):
+ self.assertIn(target, headings, f"{document.name}: #{target}")
+
+ def test_public_docs_have_no_user_paths_or_thread_ids(self) -> None:
+ documents = [ROOT / "README.md", ROOT / "README_en.md", *SKILL_DIR.rglob("*.md")]
+ text = "\n".join(path.read_text(encoding="utf-8") for path in documents)
+ self.assertIsNone(re.search(r"/(?:Users|home)/[^/\s]+/", text))
+ self.assertIsNone(
+ re.search(
+ r"\b[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}\b",
</code_context>
<issue_to_address>
**suggestion:** Extend the public-docs safety test to cover Windows-style user paths as an additional edge case.
Currently the test only flags POSIX-style user paths and UUID-like thread IDs. To also catch Windows user-local paths, add a regex for patterns like `C:\Users\<name>\...` (e.g. `r"[A-Za-z]:\\Users\\[^\\\s]+\\"`) so the "no user paths" requirement is enforced consistently across platforms.
```suggestion
documents = [ROOT / "README.md", ROOT / "README_en.md", *SKILL_DIR.rglob("*.md")]
text = "\n".join(path.read_text(encoding="utf-8") for path in documents)
# POSIX-style user/home paths
self.assertIsNone(re.search(r"/(?:Users|home)/[^/\s]+/", text))
# Windows-style user-local paths (e.g. C:\Users\username\...)
self.assertIsNone(re.search(r"[A-Za-z]:\\Users\\[^\\\s]+\\", text))
self.assertIsNone(
```
</issue_to_address>
### Comment 3
<location path="skills/systems-paper-figurecraft/references/iteration-playbook.md" line_range="92-94" />
<code_context>
+ model repeatedly cannot route correctly.
+
+Collapsing internal edges is allowed only when containment, attachment, or
+aligned fields makes the relationship unambiguous and the reader does not need
+the internal dependency to verify the claim.
+
</code_context>
<issue_to_address>
**issue (typo):** Fix subject–verb agreement in the "Collapsing internal edges" sentence.
The compound subject "containment, attachment, or aligned fields" is plural, so the verb should be "make" instead of "makes" ("…containment, attachment, or aligned fields make the relationship unambiguous…").
```suggestion
Collapsing internal edges is allowed only when containment, attachment, or
aligned fields make the relationship unambiguous and the reader does not need
the internal dependency to verify the claim.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+62
to
+71
| def test_markdown_fragment_links_resolve(self) -> None: | ||
| documents = [ROOT / "README.md", ROOT / "README_en.md", *SKILL_DIR.rglob("*.md")] | ||
| for document in documents: | ||
| text = document.read_text(encoding="utf-8") | ||
| headings = { | ||
| re.sub(r"[^\w\- ]", "", match.group(1)) | ||
| .strip() | ||
| .lower() | ||
| .replace(" ", "-") | ||
| for match in re.finditer(r"^#{1,6}\s+(.+?)\s*$", text, re.MULTILINE) |
There was a problem hiding this comment.
issue (testing): Fragment link test is currently misaligned with heading normalization and may flag valid links as failures.
Heading slugs are normalized (strip, lowercase, spaces→dashes, punctuation removed), but the target extracted from "](#...)" is compared raw. This means #Section-One won’t match the normalized slug section-one, even though Markdown renderers treat it as equivalent. Please normalize target with the same slugging logic (or factor out a shared slug function and compare those) before assertIn, so the test only fails on genuinely broken links, not casing or punctuation differences.
Comment on lines
+77
to
+80
| documents = [ROOT / "README.md", ROOT / "README_en.md", *SKILL_DIR.rglob("*.md")] | ||
| text = "\n".join(path.read_text(encoding="utf-8") for path in documents) | ||
| self.assertIsNone(re.search(r"/(?:Users|home)/[^/\s]+/", text)) | ||
| self.assertIsNone( |
There was a problem hiding this comment.
suggestion: Extend the public-docs safety test to cover Windows-style user paths as an additional edge case.
Currently the test only flags POSIX-style user paths and UUID-like thread IDs. To also catch Windows user-local paths, add a regex for patterns like C:\Users\<name>\... (e.g. r"[A-Za-z]:\\Users\\[^\\\s]+\\") so the "no user paths" requirement is enforced consistently across platforms.
Suggested change
| documents = [ROOT / "README.md", ROOT / "README_en.md", *SKILL_DIR.rglob("*.md")] | |
| text = "\n".join(path.read_text(encoding="utf-8") for path in documents) | |
| self.assertIsNone(re.search(r"/(?:Users|home)/[^/\s]+/", text)) | |
| self.assertIsNone( | |
| documents = [ROOT / "README.md", ROOT / "README_en.md", *SKILL_DIR.rglob("*.md")] | |
| text = "\n".join(path.read_text(encoding="utf-8") for path in documents) | |
| # POSIX-style user/home paths | |
| self.assertIsNone(re.search(r"/(?:Users|home)/[^/\s]+/", text)) | |
| # Windows-style user-local paths (e.g. C:\Users\username\...) | |
| self.assertIsNone(re.search(r"[A-Za-z]:\\Users\\[^\\\s]+\\", text)) | |
| self.assertIsNone( |
Comment on lines
+92
to
+94
| Collapsing internal edges is allowed only when containment, attachment, or | ||
| aligned fields makes the relationship unambiguous and the reader does not need | ||
| the internal dependency to verify the claim. |
There was a problem hiding this comment.
issue (typo): Fix subject–verb agreement in the "Collapsing internal edges" sentence.
The compound subject "containment, attachment, or aligned fields" is plural, so the verb should be "make" instead of "makes" ("…containment, attachment, or aligned fields make the relationship unambiguous…").
Suggested change
| Collapsing internal edges is allowed only when containment, attachment, or | |
| aligned fields makes the relationship unambiguous and the reader does not need | |
| the internal dependency to verify the claim. | |
| Collapsing internal edges is allowed only when containment, attachment, or | |
| aligned fields make the relationship unambiguous and the reader does not need | |
| the internal dependency to verify the claim. |
There was a problem hiding this comment.
Pull request overview
This PR distills new, reusable heuristics for revising systems-paper figures by explicitly separating semantic authority from style references, translating qualitative critique into concrete structural repair actions, and tightening validation and prompting guidance accordingly.
Changes:
- Expanded the core skill guidance (modes, completion gates, revision-contract behavior) and added semantic-rank / edge-inventory / micro-glyph auditing rules.
- Added/updated reference docs (visual system, validation, iteration playbook, figure grammars, prompt templates, contracts) to encode the new heuristics and new “field-provenance / contract-ledger” grammars.
- Added unit tests enforcing skill conciseness, contents sections for long references, intra-doc fragment link validity, and absence of user paths / UUID-like thread IDs in public docs.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_skill_package.py | Adds packaging/docs quality gates (size, contents sections, fragment links, privacy scans). |
| skills/systems-paper-figurecraft/SKILL.md | Updates mode semantics and completion gates; adds authority separation, semantic rank, edge inventories, and connector escalation guidance. |
| skills/systems-paper-figurecraft/references/visual-system.md | Adds semantic-rank/attachment and micro-geometry semantics guidance; extends whitespace/edge inventory rules. |
| skills/systems-paper-figurecraft/references/validation.md | Strengthens semantic/visual validation with boundary-authority checks, micro-example semantics, aggregate-stat fidelity, and connector/edge-inventory checks. |
| skills/systems-paper-figurecraft/references/iteration-playbook.md | Replaces generic failure table with critique→structural repair mapping, connector escalation steps, and micro-example audit checklist. |
| skills/systems-paper-figurecraft/references/generation-prompts.md | Adds explicit semantic-vs-style authority sections plus addenda for logic-preserving style transfer and semantic-rank/edge inventories; updates edit escalation. |
| skills/systems-paper-figurecraft/references/figure-grammars.md | Introduces “fields/provenance/contracts” grammar family and abstraction calibration rules; updates split-panel guidance. |
| skills/systems-paper-figurecraft/references/contracts.md | Extends briefs/ledgers to capture semantic authority, rank patterns, edge inventories, glyph semantics, and aggregate/statistical semantics. |
| README.md | Updates Chinese highlights/mode semantics and adds new grammar + iteration heuristics. |
| README_en.md | Updates English highlights/mode semantics and adds iteration rules reflecting the new heuristics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5d86badc0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def test_public_docs_have_no_user_paths_or_thread_ids(self) -> None: | ||
| documents = [ROOT / "README.md", ROOT / "README_en.md", *SKILL_DIR.rglob("*.md")] | ||
| text = "\n".join(path.read_text(encoding="utf-8") for path in documents) | ||
| self.assertIsNone(re.search(r"/(?:Users|home)/[^/\s]+/", text)) |
There was a problem hiding this comment.
When documentation is generated in a typical Codex container, private absolute paths commonly start with /root/ or /workspace/, but this regex only rejects Unix /home/... and /Users/... paths. As a result, strings such as /root/private-paper/figure.png pass while the test claims to prevent user-path disclosure; extend the detector to cover these homes and Windows-style user paths as well.
Useful? React with 👍 / 👎.
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
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.
What
Why
Recent figure-revision work exposed reusable failure modes that were not explicit enough in the skill. This change generalizes those lessons without retaining project-specific mechanisms, names, paths, figures, or values.
Validation
Summary by Sourcery
Clarify and strengthen figure-revision guidance to preserve semantics during style transfer, handle critique as structural evidence, and enforce mode-aware completion and documentation constraints.
New Features:
Enhancements:
Documentation:
Tests: