docs(skills): visual-critic capture-quality + verify-vs-pixels lessons - #1172
Conversation
📝 WalkthroughPriority Level: P4/NIT
Optional: If you want to reduce future review noise, consider adding a brief cross-reference from the new roadmap doc to the updated visual-critic guidance where the lessons are first applied. WalkthroughThe PR updates a Visual Critic skill note, adds a roadmap decision document about splitting engine tool serving into pinned and deferred MCP servers, and regenerates the QA scores ledger with a wider schema and a new run entry. ChangesTool schema budget deferral
QA scores ledger refresh
Visual Critic skill guidance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Confidence: 91% Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/visual-critic/SKILL.md:
- Around line 36-38: The blockquote in SKILL.md has an empty line between quoted
lines, which triggers MD028. Update the quoted text so the two “CAPTURE QUALITY”
and “VERIFY VISUAL CLAIMS vs PIXELS” lines are part of a single continuous
blockquote in the same section, keeping the content unchanged but removing the
blank quoted separator.
- Line 38: The visual-critic guidance uses a fixed “6-lens panel” phrase that
conflicts with the surrounding 5–6 lens wording. Update the wording in SKILL.md
so the reference stays flexible and consistent with the rest of the note,
keeping the verification guidance aligned with the panel-based review flow.
In `@docs/roadmap/tool-schema-budget-deferral.md`:
- Around line 65-71: Reconcile the beat-count sample math in the roadmap note:
the sample summary in the opening paragraph and the later “40 runs / 359 beats”
headline must agree. Update the beat counts or add a clear note if one
transcript was excluded, and make sure the totals referenced in the sample and
headline are consistent with the same source set.
- Around line 145-152: Reconcile the partition totals in the roadmap table so
the byte math matches the stated baseline. Update the “Recommended partition”
figures by checking the values for the pinned `worldos-engine` and deferred
`worldos-engine-ext` sections, then make sure their sum exactly equals the
“Today (all pinned)” baseline before keeping the claimed headroom/savings. Use
the partition summary table in the doc as the source of truth and correct
whichever byte count is off so the numbers close cleanly.
- Around line 229-231: The sequencing note mixes a pinned hot-path tool with
deferred-tail trim examples, so update the “Tactical, now” list to avoid using
generate_parley_options as a trim target. In the roadmap section, keep the small
reversible docstring trim examples limited to truly deferred-tail items and
reference the existing classification of generate_parley_options as PER-BEAT HOT
PATH; if needed, replace it with a non-pinned symbol already in the deferred
tail such as author_companion_gauges or reroll_character so the unblock plan
stays internally consistent.
- Around line 220-221: Restore the missing blank lines around the affected
markdown headings in the roadmap document so the sections follow markdownlint
spacing rules. Update the nearby heading blocks such as the “Options considered”
section and the other referenced sections so each heading is separated from its
content by a blank line, keeping the document consistent and avoiding docs CI
formatting failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0861b4d9-7445-4483-b13e-709570541500
⛔ Files ignored due to path filters (1)
qa/scores.dbis excluded by!**/*.db
📒 Files selected for processing (3)
.claude/skills/visual-critic/SKILL.mddocs/roadmap/tool-schema-budget-deferral.mdqa/scores_ledger.md
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: viewer-tests
- GitHub Check: test
- GitHub Check: Analyze (python)
- GitHub Check: test
- GitHub Check: viewer-tests
⚠️ CI failures not shown inline (2)
GitHub Actions: LLM Quality Gate (advisory) / quality-gate: docs(skills): visual-critic capture-quality + verify-vs-pixels lessons
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mecho "[selfcheck] bash -n on the duo runner + scorer"�[0m
�[36;1mbash -n qa/run_duo.sh�[0m
�[36;1mbash -n qa/score.sh�[0m
�[36;1mtest -x qa/run_duo.sh�[0m
�[36;1mtest -x qa/score.sh�[0m
�[36;1m�[0m
�[36;1mecho "[selfcheck] regression detector imports + answers --help (pure reader, no DB write)"�[0m
�[36;1muv run --directory servers/engine python "${GITHUB_WORKSPACE}/qa/detect_regression.py" --help >/dev/null�[0m
�[36;1m�[0m
�[36;1mecho "[selfcheck] scorer guard path emits a hashed artifact into a TEMP dir (gateway-free)"�[0m
�[36;1mtmp="$(mktemp -d)"�[0m
�[36;1mprintf '# transcript\n' > "$tmp/t.md"�[0m
�[36;1mprintf '{}\n' > "$tmp/state.json"�[0m
�[36;1mWORLDOS_SCORE_GUARD_ONLY=1 bash qa/score.sh \�[0m
�[36;1m "$tmp/t.md" "$tmp/state.json" qa/rubric.md qa/score_schema.json "$tmp/out.json" 0.01�[0m
�[36;1mtest -s "$tmp/out.json"�[0m
�[36;1mrm -rf "$tmp"�[0m
�[36;1m�[0m
�[36;1mecho "[selfcheck] jq present (scorecard tool)"�[0m
�[36;1mcommand -v jq >/dev/null || { echo "::error title=LLM Quality Gate::jq missing on runner"; exit 1; }�[0m
GitHub Actions: LLM Quality Gate (advisory) / 0_quality-gate.txt: docs(skills): visual-critic capture-quality + verify-vs-pixels lessons
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mecho "[selfcheck] bash -n on the duo runner + scorer"�[0m
�[36;1mbash -n qa/run_duo.sh�[0m
�[36;1mbash -n qa/score.sh�[0m
�[36;1mtest -x qa/run_duo.sh�[0m
�[36;1mtest -x qa/score.sh�[0m
�[36;1m�[0m
�[36;1mecho "[selfcheck] regression detector imports + answers --help (pure reader, no DB write)"�[0m
�[36;1muv run --directory servers/engine python "${GITHUB_WORKSPACE}/qa/detect_regression.py" --help >/dev/null�[0m
�[36;1m�[0m
�[36;1mecho "[selfcheck] scorer guard path emits a hashed artifact into a TEMP dir (gateway-free)"�[0m
�[36;1mtmp="$(mktemp -d)"�[0m
�[36;1mprintf '# transcript\n' > "$tmp/t.md"�[0m
�[36;1mprintf '{}\n' > "$tmp/state.json"�[0m
�[36;1mWORLDOS_SCORE_GUARD_ONLY=1 bash qa/score.sh \�[0m
�[36;1m "$tmp/t.md" "$tmp/state.json" qa/rubric.md qa/score_schema.json "$tmp/out.json" 0.01�[0m
�[36;1mtest -s "$tmp/out.json"�[0m
�[36;1mrm -rf "$tmp"�[0m
�[36;1m�[0m
�[36;1mecho "[selfcheck] jq present (scorecard tool)"�[0m
�[36;1mcommand -v jq >/dev/null || { echo "::error title=LLM Quality Gate::jq missing on runner"; exit 1; }�[0m
🧰 Additional context used
🪛 LanguageTool
docs/roadmap/tool-schema-budget-deferral.md
[grammar] ~154-~154: Ensure spelling is correct
Context: ...6 B (60 544 B headroom); aggressive pin-steady≥5 % = 17 tools / 19 179 B (100 821 B headro...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
qa/scores_ledger.md
[grammar] ~17-~17: Ensure spelling is correct
Context: ...t: L10 Paladin=L3 slots). 4/5 personas: newbie8 veteran7 adversarial7 narrative8 optimizer5. Real lever: min-maxer planning layer. |...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~20-~20: Ensure spelling is correct
Context: ...2 -> clean +1.0 compare proving the 4.5 was the behavioral false-cap. Cross-ruler v...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.22.1)
.claude/skills/visual-critic/SKILL.md
[warning] 37-37: Blank line inside blockquote
(MD028, no-blanks-blockquote)
docs/roadmap/tool-schema-budget-deferral.md
[warning] 220-220: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 233-233: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 243-243: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 248-248: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 289-289: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🪛 SkillSpector (2.2.3)
.claude/skills/visual-critic/SKILL.md
[warning] 228: [EA4] Unbounded Resource Access: Skill allows unbounded resource consumption (API calls, storage, compute). Without rate limits or quotas, a compromised or misbehaving agent can cause denial-of-service or cost overruns.
Remediation: Set explicit rate limits, timeouts, and resource quotas for API calls, file operations, and compute. Implement circuit breakers for runaway loops.
(Excessive Agency (EA4))
🔇 Additional comments (1)
qa/scores_ledger.md (1)
13-13: 🔒 Security & Privacy | ⚡ Quick winRedact the host/user metadata in
source_path.Confidence: 88%. This new row publishes
root@178.104.123.213:/root/worldos-qa/results/RRI.json, which leaks infrastructure details and an administrative username. If this ledger is ever shared beyond the private QA team, replace it with an opaque run reference or repo-relative artifact path before regenerating.
- render at super_size=4 BEFORE scoring (low-res captures inflate the critic). - ALWAYS verify an agent's visual success claim against the actual frame (a lean self-critic over-scored ~7.3 vs the rigorous 5-6-lens panel ~4-5.5). Docs/skill only. Slimmed from the original: dropped the tool-schema roadmap doc (needs a byte/beat-math reconciliation pass) + the QA ledger (infra-path redaction) — they will land separately.
763d0ec to
5ab540b
Compare
1 participant
Two hard-won visual-critic lessons from the graphics-fork work + the tool-schema deferral doc. Docs/skills only, no engine/viewer code.