refactor(#111): consolidate overlapping shiplog workflow PRs#112
refactor(#111): consolidate overlapping shiplog workflow PRs#112devallibus merged 3 commits intomasterfrom
Conversation
Code reviewFound 2 issues:
Lines 2 to 4 in 9e9bd68 shiplog/skills/shiplog/references/artifact-envelopes.md Lines 10 to 12 in 9e9bd68
shiplog/skills/shiplog/references/commit-workflow.md Lines 124 to 126 in 9e9bd68 Additional issues found below the confidence threshold (75 or below) but worth noting:
Reviewed with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
devallibus
left a comment
There was a problem hiding this comment.
Review — PR #112: Consolidate overlapping shiplog workflow PRs
The structural decomposition is well-executed — the decision-tree dispatcher in SKILL.md routes cleanly to self-contained sub-skills, cross-references between sub-skills are correct, and signing rules are properly centralised without duplication. The license attribution for internalised workflows is properly handled.
However, there are two blocking issues and five important regressions from the consolidation that should be addressed before merge.
Blocking (must fix)
1. Encoding corruption in 4 files
README.md, brainstorm.md, references/artifact-envelopes.md, and references/closure-and-review.md contain double-encoded UTF-8 characters. Em-dashes render as mojibake (â€"), section signs render as §, arrows and box-drawing characters are similarly corrupted. This is a UTF-8-to-Latin-1-to-UTF-8 re-encoding error from the consolidation.
Fix: Re-encode with correct UTF-8, or replace with ASCII equivalents (--, ->, Section).
2. Stale references to phase-templates.md
Three references in newly created files point to phase-templates.md (now just a compatibility redirect) instead of the actual sub-skill files:
references/pr-workflow.md:102— should reference../pr.mdreferences/pr-workflow.md:169— should reference../pr.mdor../timeline.mdreferences/commit-workflow.md:125— should reference../commit.md
Important (should fix)
3. Brand formatting rule dropped — The rule from PR #93 (bold shiplog in user-facing text) was removed entirely. Not present in SKILL.md or any sub-skill. This is a regression unless intentionally removed.
4. "Quiet mode — feature PR merges" edge case dropped — Master documents closing the --log PR when the feature PR merges. This wasn't moved to any sub-skill or reference file.
5. Shell Portability and GitHub Labels sections removed from SKILL.md — These cross-cutting quick-reference sections were removed without adding pointers in the top-level dispatcher. An agent loading only SKILL.md won't know these concerns exist.
6. Missing trailing newline in 6 files — SKILL.md, commit.md, lookup.md, pr.md, timeline.md, references/labels.md all lack POSIX-standard trailing newlines.
7. ork:fix-issue integration dropped — Removed from the Integration Map without explanation. ork:issue-progress-tracking also removed from the table (though still referenced in timeline.md).
What's good
- Decision-tree dispatcher works cleanly
- Cross-references between all sub-skills are correct
- Signing rules are complete and canonical in one place
- Model routing is consistent across all sub-skills
- Triage fields, lifecycle labels, and implementation-issue capture are well-integrated
- License attribution for internalised workflows is properly handled
- The
phase-templates.mdcompatibility pointer is a good migration strategy
Reviewed-by: Claude (claude-opus-4-6)
…ntent - Fix UTF-8 double-encoding corruption in README.md, brainstorm.md, artifact-envelopes.md, and closure-and-review.md - Update stale phase-templates.md references to point to actual sub-skill files (pr.md, commit.md) in pr-workflow.md and commit-workflow.md - Restore brand formatting rule (bold **shiplog** in user-facing text) - Restore Shell Portability and GitHub Labels cross-cutting sections - Restore quiet mode edge cases (feature PR merges, branch rebased) - Add ork:fix-issue and ork:issue-progress-tracking back to Integration Map - Add trailing newlines to 6 files missing them Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Reviewed-by: openai/gpt-5.4 (cursor) Findings:
|
Repair quiet-mode and discovery comment examples so they use signed body-file artifacts, and stop classifying every discovery as a blocker. Made-with: Cursor
|
Reviewed-by: openai/gpt-5.4 (cursor) Review ContractPR: #112 � refactor(#111): consolidate overlapping shiplog workflow PRs What to review
Review checklist
Output required
|
|
Reviewed-by: anthropic/claude-opus-4-6 (claude-code) Prior findings status
New findingsF1: brainstorm.md issue envelope missing triage fields (medium)SKILL.md's Triage Field Maintenance table mandates "Set all four triage fields at creation" for Phase 1, but the issue envelope template in Suggested fix: Add the four triage fields to the brainstorm.md envelope template. F2: Unverified-claim guardrail truncated (low)Master's SKILL.md (line 216) includes: "Do not turn an unverified claim into a task requirement, acceptance criterion, or architectural decision without a verification step." The new Suggested fix: Restore the sentence to brainstorm.md's claim classification section. F3: Inconsistent
|
Summary
Replaces four overlapping PRs (#96, #101, #104, #109) with a single consolidated changeset that reconciles all changes into a coherent whole.
SKILL.mdinto composable phase sub-skills (brainstorm.md,branch.md,commit.md,discovery.md,lookup.md,pr.md,timeline.md)references/commit-workflow.md,references/pr-workflow.md)references/signing.mdreferences/model-routing.md)Supersedes
Test plan
SKILL.mdcorrectly references new sub-skillsreferences/signing.mdcovers all prior signing rulesCloses #111
Authored-by: Claude (claude-opus-4-6)
🤖 Generated with Claude Code