fix(discovery): keep absolute project root out of persisted EXPLORE.md#458
Conversation
The explore and explore-deep skills inject a live `git rev-parse --show-toplevel` as pre-computed context. That absolute machine path could bleed into the persisted EXPLORE.md handoff, making the artifact machine-specific. Mark the pre-computed project root as session-orientation only in both skills, and add a binary outcome-gate criterion requiring repo-relative artifact paths. The live root stays available for resolving files while working; it is never echoed into the handoff. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 28s —— View job PR Review — fix(discovery): keep absolute project root out of persisted EXPLORE.md
SummaryThis is a clean, well-scoped fix. The problem is real — Review findingsAnnotation placement and scoping — correct Only explore/SKILL.md L15 · explore-deep/SKILL.md L17 Outcome gate criterion — correct and well-placed The new gate criterion in Minor: forward cross-reference in The annotation on line 15 says Instruction-based enforcement (acknowledged, appropriate) The fix is model-compliance-based, which is inherent to this kind of system. The outcome gate is the only structural backstop, and it's binary and explicit. This tradeoff is appropriate here — there's no viable alternative that doesn't require schema changes. CHANGELOG and semver — correct 0.7.0 → 0.7.1 is the right semver for a bug fix. The changelog entry is accurate, describes the mechanism (not just the symptom), and covers both affected skills. VerdictApprove. The fix is correct and complete. The PR description correctly notes that |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce1fa00383
ℹ️ 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".
…ists
The explore outcome gate's new path criterion required every artifact
path to be "relative to the repo root", but the topic-docs contract
explicitly supports invocations with no repo root (no git toplevel or
project marker), where artifacts resolve under the current directory or
${CLAUDE_PLUGIN_DATA}. In those runs the binary criterion was
unsatisfiable and blocked the EXPLORE.md handoff.
Reframe the criterion around the real invariant — no absolute machine
path in the artifact — with the relative base being the repo root when
one exists, else the current working directory. Align the pre-computed
context annotations in both skills and the CHANGELOG entry.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 25s —— View job PR Review — fix(discovery): keep absolute project root out of persisted EXPLORE.md
Review of updated HEAD (
|
| Location | Before | After |
|---|---|---|
explore/SKILL.md annotation (L15) |
"records repo-relative paths (see the outcome gate)" | "records relative paths (see the outcome gate below)" |
explore/SKILL.md outcome gate (L154) |
"Paths are repo-relative" | "Paths are machine-agnostic" |
explore-deep/SKILL.md annotation (L17) |
"requires repo-relative paths" | "requires relative, machine-agnostic paths" |
No drift between the two skills; the explore-deep annotation stays in sync with the gate it delegates to. The minor "see the outcome gate below" improvement (noted in the prior review as a style point) was picked up here as well — good.
CHANGELOG — accurate
The 0.7.1 entry now correctly describes the full semantic: "relative to the repo root, or to the current working directory when no repo root exists." The version bump (0.7.0 → 0.7.1) remains correct for a bug fix.
No new issues
The second commit is surgical — 6 lines changed across 3 files, all wording updates, no structural changes. No regressions to prior behavior; the absolute-path prohibition still holds. The fix doesn't require a version bump beyond what was already added.
Verdict
Approve. d76e8f1 cleanly resolves the Codex P2 concern without introducing new issues. The PR is complete and ready to merge.
…#461) ## 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.0` → `0.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 - Stacks on PR #458 (issue #409, discovery plugin); if #458 merges first this PR needs a trivial version/CHANGELOG rebase to keep 0.7.2 above 0.7.1. - No ADRs or decision-log entries are closed by this PR. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
# Conflicts: # plugins/discovery/.claude-plugin/plugin.json # plugins/discovery/CHANGELOG.md
|
Claude finished @kyle-sexton's task in 1m 55s —— View job PR Review — fix(discovery): keep absolute project root out of persisted EXPLORE.md
Review of updated HEAD (
|
| Field | Before merge | After merge |
|---|---|---|
plugin.json version |
0.7.1 |
0.7.3 |
| CHANGELOG heading | 0.7.1 |
0.7.3 |
Why the jump to 0.7.3? Another PR (ecd6e4a) landed a 0.7.2 entry on origin/main while this branch was under review (a setup-skill change in the same plugin). The merge correctly picked up that 0.7.2 CHANGELOG entry and bumped this PR's version to 0.7.3 to avoid a collision. The semver choice remains correct: this is a bug fix → patch increment on top of the now-current 0.7.2 baseline. No version entry is missing, duplicated, or out of order.
Substantive changes — verified unchanged
All three touch-points reviewed and approved at d76e8f1 are byte-identical to the current HEAD:
| Location | Content |
|---|---|
explore/SKILL.md L15 |
Session-only annotation; bars absolute machine path from artifact |
explore/SKILL.md L154 |
Outcome gate: "Paths are machine-agnostic" + CWD fallback for no-repo-root case |
explore-deep/SKILL.md L17 |
Fork annotation; delegates gate enforcement to Step 3 |
The Codex P2 concern (gate unsatisfiable in no-repo-root runs) remains correctly resolved by the CWD fallback phrase. No drift between the two skills.
No regressions
The merge touched no logic, no schema, and no structural content in either SKILL.md file. The CHANGELOG 0.7.2 entry (from origin/main) and the new 0.7.3 entry are both present and consistent.
Verdict
Approve. The merge is clean. Version handling is correct. All prior-reviewed fixes are intact and unmodified. Ready to merge.
Problem
plugins/discovery/skills/explore/SKILL.mdandexplore-deep/SKILL.mdinject a live!git rev-parse --show-toplevel`` as pre-computedProject root:context at skill load. That value is the consumer's absolute machine path. Because `EXPLORE.md` is a handoff artifact meant to let a fresh session resume, the absolute root can bleed into the persisted file — making the artifact machine-specific and violating the marketplace agnosticism boundary (`docs/PLUGIN-PHILOSOPHY.md`: runtime behavior must not depend on absolute machine paths).Fix
Per the issue's fix direction — keep the live root for orientation, but ensure it is never echoed into the persisted artifact:
Project root:line as session-orientation only — usable for resolving files while working, never persisted intoEXPLORE.md.exploreoutcome gate: add one binary criterion requiring repo-relative artifact paths (no absolute machine path echoed).explore-deepalready runs this same gate in its Step 3, so the check propagates without duplication.No schema change: the 7-section
EXPLORE.mdoutput format and its consumers are untouched. The three pre-computed values (branch, working-tree status, root) still render for orientation; only the absolute root is barred from the artifact.Verification
markdownlint-cli2(repo config.markdownlint-cli2.jsonc) — clean on all changed files.typos(repo config_typos.toml) — clean.plugin.jsonversion bump added per the plugin's per-change convention.The
research/research-deepskills do not carry this pre-computed root pattern, so nothing else in the plugin needed the change.Related
docs/PLUGIN-PHILOSOPHY.md; sibling PR fix(implementation): demote Clean-Architecture layer names to an example #457 (issue implementation: Clean-Architecture layer vocabulary baked as universal execution order #404, implementation plugin) addresses the same design boundary.Closes #409