Skip to content

feat(implement): make OpenSpec mandatory when configured (Phase 0)#13

Merged
lapc506 merged 4 commits into
mainfrom
feat/openspec-mandatory-in-implement
May 10, 2026
Merged

feat(implement): make OpenSpec mandatory when configured (Phase 0)#13
lapc506 merged 4 commits into
mainfrom
feat/openspec-mandatory-in-implement

Conversation

@lapc506
Copy link
Copy Markdown
Collaborator

@lapc506 lapc506 commented May 5, 2026

Summary

The /make-no-mistakes:implement skill currently treats OpenSpec context as optional in step 4b ("if exists"). Since OpenSpec was officially adopted at Dojo Coding on 2026-03-30 (Slack thread C0AE5MKAX7B), this gap allows implementations to skip the spec by accident — exactly the failure mode adoption was meant to prevent.

This PR introduces a new Phase 0: OpenSpec Context that runs before Phase 1 (Setup) and makes spec-first mandatory when linear-setup.json has openspec.changesPath configured.

Behavior change

  • Before: OpenSpec is checked "if exists" — easy to skip silently
  • After: When OpenSpec is configured AND the issue is non-trivial, the implementer STOPS and creates proposal.md + design.md + tasks.md as the first commit on the branch
  • Escape hatch: trivial issues (typos, single-line fixes) can opt out with a one-line PR note
  • No-op for non-adopted repos: if openspec.changesPath is unset, Phase 0 is skipped entirely

Files changed

  • commands/implement.md — new Phase 0 section, Phase 1 step 4a (defers commit until after the worktree exists), Phase 2–4 step renumber so cross-phase references are unambiguous (1, 2, 3, 4, 4a → 5, 6, 7 → 8, 9, 10, 11, 12 → 13, 14, 15, 16, 17)
  • .claude-plugin/plugin.json, package.json — version bump 1.5.0 → 1.6.0 (minor — Phase 0 is a behavioral break for agents relying on the old optional behavior)
  • .claude-plugin/marketplace.json — version bump 1.4.0 → 1.6.0 (catching up from a pre-existing drift where it had been left at 1.4.0 while plugin.json/package.json were already at 1.5.0; aligns all three at 1.6.0)

Greptile fix history

Round Findings addressed
1 (3/5) Chicken-and-egg branch ordering, hardcoded openspec/changes/ path, semver patch→minor
2 (3/5 → 4/5) {changes-path} placeholder undefined → use $CHANGES_PATH; Phase 2 step numbering collision → renumber 1–17 sequentially
3 (4/5 → ?/5) Step 4a source path ambiguous → resolve $MAIN_TREE via git worktree list --porcelain and cp -r "$MAIN_TREE/$CHANGES_PATH/..." before git add

Test plan

  • Run /make-no-mistakes:implement DOJ-XXX against an issue WITHOUT an OpenSpec change in a repo where openspec.changesPath is configured → verify skill stops and prompts to create the spec
  • Run against an issue WITH an OpenSpec change → verify the three artifacts are read and the implementer treats them as primary context
  • Run against a repo where openspec.changesPath is unset → verify Phase 0 is skipped (back-compat)
  • Run against a repo where openspec.changesPath is specs/changes/ (non-default) → verify grep resolves correctly and the spec is detected
  • Run end-to-end: Phase 0 drafts files in main tree → Phase 1 creates worktree → step 4a copies via $MAIN_TREE → first commit on the branch is docs(openspec): {change-slug} and is non-empty

Created by Claude Code on behalf of @lapc506.

Generated with Claude Code

@lapc506
Copy link
Copy Markdown
Collaborator Author

lapc506 commented May 5, 2026

@greptile review

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 5, 2026

Greptile Summary

This PR introduces Phase 0 (OpenSpec Context) to the /make-no-mistakes:implement skill, making spec-first development mandatory when openspec.changesPath is configured in linear-setup.json. It also adds step 4a inside Phase 1 to guarantee the drafted spec artifacts become the very first commit on the implementation branch, and renumbers Phase 2–4 steps sequentially (5–17) to eliminate cross-phase numbering ambiguity.

  • Phase 0 logic: resolves CHANGES_PATH dynamically via jq, greps for an existing spec, drafts three artifacts if missing (with deterministic slug derivation), or skips entirely when the project hasn't adopted OpenSpec. A trivial-issue escape hatch (step 4) is provided.
  • Step 4a (first-commit guarantee): resolves the main tree path via git worktree list --porcelain, copies drafted files from the main tree into the new worktree with cp -r, then stages and commits — correctly threading the chicken-and-egg problem flagged in earlier rounds.
  • Version alignment: all three version manifests (plugin.json, marketplace.json, package.json) are aligned at 1.6.0; marketplace.json catches up from a pre-existing drift at 1.4.0.

Confidence Score: 5/5

Safe to merge — Phase 0 logic is well-structured, all previously blocking findings are resolved, and version manifests are consistently aligned.

All three previously blocking findings (branch ordering, hardcoded openspec/changes/ path, unbound {change-slug} template) have been addressed across successive rounds. MAIN_TREE resolution via git worktree list --porcelain is correct, cp -r correctly copies into the worktree before staging, and the idempotency guard prevents double-copy on restart.

No files require special attention; remaining items are non-blocking polish around main-tree cleanup after step 4a and Phase 0 step-numbering cosmetics.

Important Files Changed

Filename Overview
commands/implement.md Adds Phase 0 (OpenSpec Context), inserts step 4a for the first-commit guarantee, and renumbers Phase 2-4 steps 5-17; all previous blocking findings are addressed.
.claude-plugin/plugin.json Minor version bump 1.5.0 → 1.6.0; correct semver level for the behavioral change introduced in this PR.
.claude-plugin/marketplace.json Version bumped from 1.4.0 to 1.6.0, catching up with plugin.json/package.json; the intentional skip of 1.5.0 is explained in the PR description.
package.json Minor version bump 1.5.0 → 1.6.0 to match plugin.json; correct semver level for the Phase 0 behavioral change.

Reviews (5): Last reviewed commit: "fix(implement): bind {change-slug} as $C..." | Re-trigger Greptile

Comment thread commands/implement.md Outdated
Comment thread commands/implement.md
Comment thread package.json Outdated
Introduces a new Phase 0 in /make-no-mistakes:implement that runs BEFORE
Phase 1 (Setup). When linear-setup.json has openspec.changesPath
configured, every issue MUST have a corresponding OpenSpec change
(proposal.md + design.md + tasks.md) before implementation begins.

Previously, step 4b in Phase 2 read OpenSpec context only "if found",
which made the spec-first discipline silently skippable. Since OpenSpec
was officially adopted at Dojo Coding on 2026-03-30 (Slack thread
C0AE5MKAX7B), that gap re-introduces exactly the failure mode adoption
was meant to prevent: implementations diverging from intent because
nobody wrote the intent down.

Behavior change:
- When openspec.changesPath is set AND the issue is non-trivial
  (>2 files, architectural decisions, or reviewer-flagged risk),
  the implementer STOPS and creates the OpenSpec change as the
  first commit on the implementation branch.
- Trivial issues (typo fixes, dependency bumps, single-line changes)
  may opt out with a one-line PR note.
- Repos without openspec.changesPath are unaffected — Phase 0 is
  skipped entirely, preserving back-compat.

Bumps version to 1.4.1 across plugin.json, marketplace.json, and
package.json.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@lapc506 lapc506 force-pushed the feat/openspec-mandatory-in-implement branch from 9508df5 to 019604f Compare May 10, 2026 00:37
@lapc506
Copy link
Copy Markdown
Collaborator Author

lapc506 commented May 10, 2026

Rebased onto main and addressed all 3 Greptile findings in 019604f.

1. Chicken-and-egg branch ordering (Issue 1)
Phase 0 step 3 no longer says "commit FIRST". It now drafts the OpenSpec artifacts on disk and explicitly defers the commit. Phase 1 picks up that responsibility in a new step 4a — "Commit the OpenSpec change as the first commit", which runs after Phase 1 step 3 creates the branch and worktree. Order is now linear: write spec → create branch → commit spec → implement.

2. Hardcoded openspec/changes/ path (Issue 2)
Phase 0 step 1 now resolves the path from linear-setup.json before grepping:

CHANGES_PATH=$(jq -r '.openspec.changesPath' linear-setup.json)
grep -r "{issue-id}" "$CHANGES_PATH"/*/proposal.md 2>/dev/null

All subsequent references in steps 2 & 4a use <changes> as a placeholder for that resolved value, and step 1 explicitly calls out that hardcoding openspec/changes/ would silently miss specs in projects with non-default paths.

3. Semver: 1.4.0 → 1.4.1 → 1.6.0 (Issue 3)
The rebase had to reconcile against main (now at 1.5.0 after PR #9). Bumped to 1.6.0 across package.json, .claude-plugin/plugin.json, .claude-plugin/marketplace.json to reflect the new mandatory enforcement phase as a minor release rather than a silent patch — agents that relied on the old optional behavior will now stop mid-workflow, which deserves a discoverable changelog entry.

Conflicts resolved: kept main's updated description text in package.json and plugin.json (it correctly mentions the manifest-driven hooks added in PR #9). marketplace.json was at 1.4.0 on main; bumped both top-level and nested plugin version to 1.6.0 to stay consistent.

@greptile review

Comment thread commands/implement.md
Comment thread commands/implement.md Outdated
…ntially

Greptile re-review on PR #13 caught two real defects in the new Phase 0/Phase 1 text:

1. Phase 1 step 4a's git add command used an undefined placeholder
   "{changes-path}" — the variable established in Phase 0 step 1 is
   "\$CHANGES_PATH" (set via jq from linear-setup.json). An agent
   running the step verbatim would stage nothing, producing an empty
   first commit and silently breaking the spec-first guarantee the
   entire phase enforces. Step 4a now re-exports CHANGES_PATH inside
   the worktree (the parent shell's variable doesn't survive the cd
   into a fresh worktree) and uses "\$CHANGES_PATH/{change-slug}/"
   in the git add.

2. The original document restarted step numbering at 4 in Phase 2,
   colliding with Phase 1's existing step 4 + 4a. Cross-phase
   references like "step 4" became ambiguous. Renumbered the entire
   chain so steps run sequentially across all four phases:
     - Phase 1: 1, 2, 3, 4, 4a
     - Phase 2: 5, 6, 7
     - Phase 3: 8, 9, 10, 11, 12
     - Phase 4: 13, 14, 15, 16, 17

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@lapc506
Copy link
Copy Markdown
Collaborator Author

lapc506 commented May 10, 2026

Addressed both Greptile findings in 60e2cbb.

1. git add "{changes-path}/..."git add "$CHANGES_PATH/..."
Real defect: the placeholder I used in step 4a ({changes-path}) was never defined; the actual shell variable from Phase 0 step 1 is $CHANGES_PATH. An agent running the step verbatim would stage nothing → empty first commit → silent failure of the spec-first guarantee. Step 4a now re-exports CHANGES_PATH inside the worktree (the Phase 0 shell variable doesn't survive the cd into a fresh worktree per Phase 1 step 3) and uses "$CHANGES_PATH/{change-slug}/" in the git add.

2. Step numbering collision
The original doc restarted at 4 in Phase 2 (collided with Phase 1's step 4 + 4a). Renumbered the entire chain so cross-phase references are unambiguous:

  • Phase 1: 1, 2, 3, 4, 4a
  • Phase 2: 5, 6, 7
  • Phase 3: 8, 9, 10, 11, 12
  • Phase 4: 13, 14, 15, 16, 17

@greptile review

Comment thread commands/implement.md Outdated
…list

Greptile re-review on PR #13 caught a P1 ambiguity in step 4a: Phase 0
drafts spec files in the main working tree at {main-tree}/\$CHANGES_PATH/
{change-slug}/, but Phase 1 step 3 cd's into a freshly created worktree
that does NOT carry over uncommitted files. The previous wording said
"move/copy if not already there" without naming the source path — an
agent inside the worktree could resolve it incorrectly and end up
running git add on a non-existent path, producing a silently empty
docs(openspec) commit and defeating the spec-first guarantee.

Step 4a now:
  1. Re-exports \$CHANGES_PATH inside the worktree (the parent shell's
     variable doesn't survive the cd).
  2. Computes \$MAIN_TREE via git worktree list --porcelain (first
     entry is always the primary tree, regardless of which worktree we're
     currently in).
  3. Skips the copy when the directory already exists in the worktree
     (idempotent on retry).
  4. Copies "\$MAIN_TREE/\$CHANGES_PATH/{change-slug}" into the worktree
     before git add — explicit, unambiguous, agent-runnable verbatim.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@lapc506
Copy link
Copy Markdown
Collaborator Author

lapc506 commented May 10, 2026

Round 3 fixes pushed in 553d81a + PR description updated to reflect actual 1.6.0 bump (was misleadingly stating 1.4.0 → 1.4.1).

Fix for Issue 1 (step 4a source path)

Phase 0 step 3 drafts files in the main working tree at {main-tree}/$CHANGES_PATH/{change-slug}/. Phase 1 step 3 creates a fresh worktree from {baseBranch} which does NOT inherit those uncommitted files. Step 4a now resolves the main-tree path explicitly and copies the directory in before staging:

CHANGES_PATH=$(jq -r '.openspec.changesPath' linear-setup.json)
MAIN_TREE=$(git worktree list --porcelain | awk '/^worktree/ {print $2; exit}')
if [ ! -d "$CHANGES_PATH/{change-slug}" ]; then
  mkdir -p "$CHANGES_PATH"
  cp -r "$MAIN_TREE/$CHANGES_PATH/{change-slug}" "$CHANGES_PATH/"
fi
git add "$CHANGES_PATH/{change-slug}/"
git commit -m "docs(openspec): {change-slug}"

Used git worktree list --porcelain | awk instead of git rev-parse --show-toplevel because inside a worktree the latter returns the worktree root, not the primary tree. The first row of git worktree list --porcelain is always the primary tree, regardless of which worktree we're currently in. Idempotent (skips copy if directory already exists).

Fix for Issue 2 (PR description)

Updated body to reflect actual 1.5.0 → 1.6.0 bump (and 1.4.0 → 1.6.0 for marketplace.json with explicit note about the pre-existing drift). Added Greptile fix history table for traceability.

@greptile review

Comment thread commands/implement.md Outdated
Greptile round-4 caught the same class of defect as round 2 but for a
different placeholder: {change-slug} was a bare template literal in the
step 4a bash block, sitting next to a properly bound \$CHANGES_PATH. An
agent running the block verbatim would execute
  git add "\$CHANGES_PATH/{change-slug}/"
which stages a literal path containing the brace string — staging
nothing — and produces a silently empty docs(openspec) commit.

Two paired fixes:

1. Phase 0 step 3 now states the slug naming convention explicitly:
   {issue-id-lowercase}-{short-kebab-description} (lowercase ASCII +
   hyphens, e.g. doj-3946-atomic-primitives-sprint). Same shape as
   the implementation branch so agents and humans converge on the same
   directory name across runs.

2. Step 4a bash block now binds CHANGE_SLUG="<change-slug>" at the top
   (placeholder agents/humans MUST replace before running) and uses
   "\$CHANGE_SLUG" in every file path expression. mkdir/cp/git add/git
   commit all reference the bound variable, so verbatim execution
   produces a non-empty commit even if the surrounding text is unread.

Documentation-template uses of {change-slug} elsewhere in the file are
left intact — they refer to the literal directory name in prose, not
shell variables, and agents reading prose interpret them correctly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@lapc506
Copy link
Copy Markdown
Collaborator Author

lapc506 commented May 10, 2026

Round 4 fix in d7ea9c9. Same class of defect as round 2 — bare template literal next to a properly-bound shell variable.

Two paired changes:

  1. Phase 0 step 3 now states the slug naming rule explicitly:

    "The slug MUST follow the same shape as the implementation branch: {issue-id-lowercase}-{short-kebab-description} (e.g., doj-3946-atomic-primitives-sprint). Lowercase only, ASCII alphanumerics + hyphens, no leading/trailing hyphens, no other separators."

    Both agents and humans now converge on the same directory name across runs and restarts.

  2. Step 4a bash block binds CHANGE_SLUG="<change-slug>" as the first variable in the script and uses "$CHANGE_SLUG" everywhere:

    CHANGES_PATH=$(jq -r '.openspec.changesPath' linear-setup.json)
    CHANGE_SLUG="<change-slug>"   # placeholder — replace before running
    MAIN_TREE=$(git worktree list --porcelain | awk '/^worktree/ {print $2; exit}')
    if [ ! -d "$CHANGES_PATH/$CHANGE_SLUG" ]; then
      mkdir -p "$CHANGES_PATH"
      cp -r "$MAIN_TREE/$CHANGES_PATH/$CHANGE_SLUG" "$CHANGES_PATH/"
    fi
    git add "$CHANGES_PATH/$CHANGE_SLUG/"
    git commit -m "docs(openspec): $CHANGE_SLUG"

    Verbatim execution now produces a non-empty commit (the only literal that needs replacement is the <change-slug> quoted string at the top — and the comment flags it explicitly).

Documentation-template uses of {change-slug} elsewhere in the file (prose contexts referring to the directory name) are intentionally left intact — they aren't shell expressions.

@greptile review

@lapc506 lapc506 merged commit 7043033 into main May 10, 2026
1 check passed
@lapc506 lapc506 deleted the feat/openspec-mandatory-in-implement branch May 10, 2026 01:15
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.

1 participant