Skip to content

fix(discovery): inline deferred-backend rationale, drop dead ADR path#461

Merged
kyle-sexton merged 2 commits into
mainfrom
fix/407-inline-adr-rationale
Jul 19, 2026
Merged

fix(discovery): inline deferred-backend rationale, drop dead ADR path#461
kyle-sexton merged 2 commits into
mainfrom
fix/407-inline-adr-rationale

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Summary

/discovery:setup cited a marketplace-repo ADR by bare path
(docs/adr/0001-defer-gitbook-as-knowledge-vault-backend.md) at both vault_backend: gitbook
deferral call sites (skills/setup/SKILL.md:47 and :63). That ADR is a publisher-repo file — not
bundled in the plugin and not present in the consumer repo — so in the cache-isolated installed
plugin form the reference resolves to nothing, leaving a consumer reading the setup skill on a dead
path. This violates PLUGIN-PHILOSOPHY cache-isolation and the never-cross-reference-file-names rule.

Change

  • Inlined the one-sentence rationale directly at both call sites — git remains the storage layer
    because GitBook offers no concurrency-safe, lossless write path
    — replacing the bare ADR path.
    The existing "deferred and non-writable; durable writes target docs" wording is preserved.
  • No dead path remains; behavior is unchanged.
  • Bumped plugins/discovery 0.7.00.7.2 and added a Keep-a-Changelog ## [0.7.2] Changed
    entry. (0.5.1's historical CHANGELOG entry citing the same ADR path is intentionally left as-is.)

Closes #407

Related

The setup skill cited a marketplace-repo ADR by bare path
(docs/adr/0001-defer-gitbook-as-knowledge-vault-backend.md) at both
gitbook-deferral call sites. That file is not bundled in the plugin, so
in the cache-isolated installed form the reference resolves to nothing.

Inline the one-sentence rationale (git remains the storage layer because
GitBook offers no concurrency-safe, lossless write path) at both sites,
keeping the installed skill text self-contained per PLUGIN-PHILOSOPHY
cache-isolation. Behavior is unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jul 19, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 50s —— View job


PR Review — fix(discovery): inline deferred-backend rationale, drop dead ADR path

  • Gather context
  • Read changed files
  • Verify version bump logic
  • Review SKILL.md inline rationale
  • Check CHANGELOG format
  • Post review findings

Summary

Clean, focused fix that correctly addresses a cache-isolation violation. The dead bare-path ADR reference (docs/adr/0001-defer-gitbook-as-knowledge-vault-backend.md) is a publisher-repo file that is not bundled with the plugin and does not exist in a consumer repo — the inline rationale replacement is the right call. Behavior is preserved at both sites. No security, architectural, or blocking concerns. Three minor nits follow.


Blocking Criteria

All "always check" items from REVIEW.md (authorization, tenant scoping, secrets, injection, audit logging, atomicity) are N/A — this is a text-only change in a skill instruction file with no runtime code paths.


Findings

🟡 Nit 1 — Wording inconsistency between the two call sites

Site 1 (L47):

git remains the vault's storage layer because GitBook offers no concurrency-safe, lossless write path

Site 2 (L64):

git remains the storage layer because GitBook offers no concurrency-safe, lossless write path

The two sites explain the same architectural decision; "vault's" appears in one but not the other. Should be identical for a reader who encounters both. Fix this →


🟡 Nit 2 — Version skips 0.7.1

plugin.json goes 0.7.0 → 0.7.2, leaving no 0.7.1. The PR body explains this is intentional because it stacks on #458, which will occupy 0.7.1. If #458 is reverted, abandoned, or renumbered, the gap becomes permanent. Not blocking, but worth noting the dependency is structural — if the stacking assumption changes, this PR's version needs a rebase too (which the PR body already acknowledges).


🟡 Nit 3 — Em-dash + colon structure at Site 2 reads awkwardly

L63–65:

gitbook is reserved but not enabled as a vault_backend value — git remains the storage layer because GitBook offers no concurrency-safe, lossless write path**:** when offering or preserving it, report…

The colon immediately follows the inline rationale em-dash clause, producing a — rationale: continuation pattern where a period or semicolon before "when offering" would flow more naturally. Minor, but the check site (Site 1) does not have this because the parenthetical clause ends with an em-dash already.


File-by-file verdict

File Status
plugins/discovery/.claude-plugin/plugin.json ✅ Version bump correct; format unchanged
plugins/discovery/CHANGELOG.md ✅ Entry format consistent with prior entries; 0.5.1 historical cite left intact (intentional per PR body)
plugins/discovery/skills/setup/SKILL.md ✅ Both call sites updated; ADR path removed; "deferred and non-writable / durable writes target docs" semantics preserved

Overall: approve with the Nit 1 wording alignment worth a quick fixup — the two sites should be verbatim-identical since they describe the same decision.

Addresses the PR review nits on the deferred-backend rationale.

Nit 1: the `check` site read "git remains the vault's storage layer"
while the `apply` site and the commit-message rationale both omit the
possessive. Drop "vault's" from the `check` site so both sites are
verbatim-identical, matching the canonical phrasing.

Nit 3: split the run-on at the `apply` site — the em-dash rationale
clause was followed by a colon, chaining two independent statements.
A sentence break reads cleaner. Behavior is unchanged.
@claude

claude Bot commented Jul 19, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 10s —— View job


  • Read latest commit changes
  • Verify Nit 1 fix (wording alignment across both call sites)
  • Check for any new issues introduced
  • Post updated review

Re-review — commit 4ceda07

The follow-up commit squarely addresses both outstanding nits from the first review pass.

Nit 1 resolved — verbatim-identical phrasing at both call sites

"vault's" has been dropped from the check site. Both sites now read identically:

git remains the storage layer because GitBook offers no concurrency-safe, lossless write path

L47 and L63 are now verbatim-identical for the core rationale phrase. ✅

Nit 3 resolved — em-dash + colon run-on broken up

The apply site now ends the inline rationale with a period before "When offering or preserving it, report…" (L64), replacing the awkward — rationale: continuation chain. Reads cleanly. ✅

Nit 2 (version gap 0.7.0 → 0.7.2) — acknowledged, no change needed

The PR body explains the gap is intentional (stacking on #458 which will occupy 0.7.1). No new issues introduced by this commit.


File-by-file verdict

File Status
plugins/discovery/.claude-plugin/plugin.json ✅ Unchanged from original PR — version bump correct
plugins/discovery/CHANGELOG.md ✅ Unchanged from original PR — format correct
plugins/discovery/skills/setup/SKILL.md ✅ Both call sites now verbatim-identical; sentence break at apply site resolves the flow issue

Overall: all review nits addressed. PR is clean and ready to merge (modulo the #458 stacking note in the PR body, which is the author's responsibility to track).

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Per-finding classification:

# Finding Classification Evidence Reacted
1 Nit 1 — "vault's" wording inconsistency between the two call sites VALID — fixed Removed "vault's" from the check site so both sites now read verbatim git remains the storage layer because GitBook offers no concurrency-safe, lossless write path. Aligned toward the un-possessive phrasing because that is what the apply site and the commit-message rationale already use (2 of 3 author renderings). 👍
2 Nit 2 — version skips 0.7.1 ACKNOWLEDGED — no action Intentional and documented in the PR body: 0.7.1 is reserved by the stacked PR #458. Structural dependency noted, not a defect. 👍
3 Nit 3 — em-dash + colon run-on at the apply site VALID — fixed Split the clause into two sentences (colon → period break before "When offering") for cleaner flow. Behavior unchanged. 👍

Fixed in 4ceda07 (Nits 1 and 3, both wording-only). Nit 2 needs no change.

@kyle-sexton
kyle-sexton merged commit 14d7640 into main Jul 19, 2026
15 checks passed
@kyle-sexton
kyle-sexton deleted the fix/407-inline-adr-rationale branch July 19, 2026 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

discovery: setup skill cites marketplace-repo ADR by bare path — dead reference in installed plugin form

1 participant