[#187] fix: target-layout packaging rewrites relative link depth#193
Merged
[#187] fix: target-layout packaging rewrites relative link depth#193
Conversation
- Reproduce bug: target-layout packaging doesn't adjust relative link depth - .claude/skills/ (3 deep) → .skills/ (2 deep) links not rewritten - Task: T-1 Refs: #187
- Rewrite relative links when packaging from target to source layout - Compute depth delta using originalDir (target) vs newDir (source) - Use content-ops rewriteLinksInFile for consistent link adjustment - Depth rewrite applied before --root absolute rewriting - Task: T-2 Refs: #187
- Source layout packaging does NOT rewrite links (AC-4) - External links and anchors preserved in target-layout (AC-5) - Same-depth source/target is a no-op - Task: T-3 Refs: #187
- Package from target layout with depth-adjusted links - Verify rewritten content in extracted ZIP - Verify package passes checksum verification - Task: T-4 Refs: #187
- MT-CP806: manual test for target-layout link depth rewriting - Renumber old CP806→CP807 - way-of-working: document qa/ as manual test location - CLAUDE.md: add qa/release-validation/ to Key References Refs: #187
rucka
commented
Apr 11, 2026
Collaborator
Author
rucka
left a comment
There was a problem hiding this comment.
Code Review: #193 — [#187] fix: target-layout packaging rewrites relative link depth
Review Information
Reviewer: AI-assisted review
Review Date: 2026-04-11
Story: #187 / Epic #67
Review Type: Bug Fix
Decision: ✅ APPROVED
Review Summary
Clean, minimal bug fix that adds depth-aware relative link rewriting when packaging from target layout. Reuses existing rewriteLinksInFile from @pair/content-ops — no new abstractions or dependencies. All 5 acceptance criteria are covered by tests.
Code Review Checklist
Functionality
- Requirements Met — all 5 AC validated by dedicated tests
- Business Logic — depth delta computation (
originalDirvsnewDir) is correct - Edge Cases — external links, anchors, same-depth registries all tested
- Performance — no measurable impact (rewriter already used in install/update paths)
Code Quality
- Readability — clear inline comment explaining the depth rewriting block
- Maintainability — reuses existing infrastructure, no new modules
- Naming —
originalDir/newDirclearly convey intent
Technical Standards
- Style Guide — quality gate passes (ts:check, test, lint, prettier, mdlint)
- Architecture — no new dependencies, no ADR needed
- Dependencies —
@pair/content-opsalready in tech-stack
Security Review
No security concerns — no user input, no external calls, no secrets.
Testing Review
- Unit Tests — 5 new tests: bug reproduction, source-layout regression, external/anchor preservation, same-depth no-op, mixed content
- Integration Tests — real FS round-trip: package → extract → verify checksum + link depth
- Manual Testing — MT-CP806 added to CP8 for release validation
- Test Clarity — tests are well-named and commented with depth explanations
Documentation Review
-
way-of-working.md— documentedqa/as manual test location -
CLAUDE.md— addedqa/release-validation/to Key References -
CP8-packaging.md— MT-CP806 added, MT-CP807 renumbered
Positive Feedback
- Exemplary bug-fix workflow: failing test first (T-1), then fix (T-2), then regression + integration (T-3, T-4)
- Smart reuse of
rewriteLinksInFile— avoided reimplementing link rewriting - Guard clause
originalDir !== newDirprevents unnecessary work for same-depth registries - Round-trip integration test with real FS + checksum verification is thorough
Risk Assessment
| Risk | Impact | Probability | Mitigation |
|---|---|---|---|
| Regression in source-layout packaging | Medium | Very Low | Dedicated regression test covers this |
--root + --layout target interaction |
Low | Low | Root rewrites to absolute first; depth adjustment is no-op on absolute links |
Tech Debt
No debt items flagged.
REVIEW COMPLETE:
├── PR: #193: [#187] fix: target-layout packaging rewrites relative link depth
├── Story: #187: Bug: target-layout link depth off-by-one
├── Decision: APPROVED
├── Issues: critical: 0 | major: 0 | minor: 0
├── Quality: PASS — all gates
├── DoD: met (pending merge + smoke tests)
├── Adoption: no new deps, manual check confirms compliance
├── Debt: 0 items flagged
└── Report: Posted as PR comment
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.
PR Information
PR Title: [#187] fix: target-layout packaging rewrites relative link depth
Story/Epic: #187 / #67
Type: Bug Fix
Priority: P0
Assignee: @rucka
Labels: bug
Summary
What Changed
Added depth-aware relative link rewriting in
copyFileToTempwhen packaging from target layout. After writing a.mdfile to the temp directory, iflayout === 'target', the fix computes the depth delta between the file's original target location and its destination source location, then callsrewriteLinksInFilefrom@pair/content-opsto adjust relative../links.Why This Change
pair package --layout targetreads files from target paths (e.g.,.claude/skills/pair-xxx/— 3 levels deep) but writes them to source paths (e.g.,.skills/pair-xxx/— 2 levels deep). Relative../links were copied verbatim, producing broken links in the packaged artifact. Consumers installing from the package got incorrect internal links.Story Context
User Story: As a KB author packaging from target layout, I want
pair package --layout targetto rewrite relative links to match source-layout depth so that the packaged artifact validates cleanly withkb-validate --layout source.Acceptance Criteria:
Changes Made
Implementation Details
copyFileToTemp, whenlayout === 'target'and file is.md, computeoriginalDir(target path relative to project root) andnewDir(source path), callrewriteLinksInFileto adjust relative linksrewriteLinksInFilefrom@pair/content-ops(same mechanism used by install/update commands)originalDir === newDir(same depth), skip rewriting entirely--rootabsolute rewritingqa/location in way-of-working.md and CLAUDE.mdFiles Changed
apps/pair-cli/src/commands/package/zip-creator.ts— added depth-aware link rewriting incopyFileToTempapps/pair-cli/src/commands/package/zip-creator.test.ts— 5 new test casesqa/release-validation/CP8-packaging.md— added MT-CP806, renumbered MT-CP807.pair/adoption/tech/way-of-working.md— documentedqa/as manual test locationCLAUDE.md— addedqa/release-validation/to Key ReferencesTesting
Test Coverage
zip-creator.test.ts— bug reproduction, regression, edge casesTest Results
Testing Strategy
--rootrewriting unaffectedQuality Assurance
Code Quality Checklist
Review Areas
originalDirvsnewDir) is correct for all registry configurationsrewriteLinksInFilefrom content-ops — no new abstractionsDeployment Information
Deployment Notes
@pair/content-opsexport)--layout targetpackaging pathRollback Plan
Revert commit; no data migration needed.
Dependencies & Related Work
Related PRs
Follow-up Work
Closes #187