From 1e213db45a5152b763bb24c0f0310ae3972fd891 Mon Sep 17 00:00:00 2001 From: JB Date: Fri, 4 Sep 2026 10:22:39 -0400 Subject: [PATCH] fix(skills): address develop workflow review --- ...0004-bun-extension-workspaces-01SEP2026.md | 6 ++- ...-imported-extension-snapshots-01SEP2026.md | 4 +- skills/develop/develop-workflow/SKILL.md | 20 +++++----- .../scripts/checkpoint-record.mjs | 37 ++++--------------- 4 files changed, 26 insertions(+), 41 deletions(-) diff --git a/docs/adr/0004-bun-extension-workspaces-01SEP2026.md b/docs/adr/0004-bun-extension-workspaces-01SEP2026.md index ba6297a..88a67e5 100644 --- a/docs/adr/0004-bun-extension-workspaces-01SEP2026.md +++ b/docs/adr/0004-bun-extension-workspaces-01SEP2026.md @@ -8,7 +8,9 @@ Publish packages to the `jb` owner endpoint of the Gitea npm registry. Repositor `extensions/shared` is the publishable `@jbcode/pi-shared` library. Extensions that import it declare a normal runtime dependency so Pi's production package installation receives the required code. -`extensions/_workflows` is reference code, not a package or publishable extension. The package-boundary decision remains valid, but [ADR 0006](./0006-skill-led-development-workflow-04SEP2026.md) supersedes the proposed runtime merge: `@jbcode/pi-workflow` now remains private, unpublished, and unexposed as historical pilot evidence. +`extensions/_workflows` is reference code, not a package or publishable extension. `@jbcode/pi-workflow` remains a private workspace until the required reference runtime is merged under `extensions/workflow`; this prevents publishing two workflow packages or publishing an incomplete package. + +> **Later decision:** [ADR 0006](./0006-skill-led-development-workflow-04SEP2026.md) supersedes the workflow-specific merge and release direction above. Keep the legacy workspace private, unpublished, and unexposed during the skill pilot. The existing `config/pi-links.json` policy remains the authority for live-development exposure. Package boundaries and publication do not automatically enable extensions globally. @@ -18,4 +20,4 @@ The existing `config/pi-links.json` policy remains the authority for live-develo - Catalog and workspace protocols require `bun publish` or `bun pm pack` before npm publication so concrete versions are written into package metadata. - Shared libraries must be published before extensions that depend on the same new version. - Pi packages continue shipping TypeScript source because Pi loads extension entrypoints directly. -- ADR 0006 removes a `workflow` extension release from the active plan; archival or removal follows the skill pilot. +- The `workflow` merge is an explicit blocker for its first release. diff --git a/docs/adr/0005-imported-extension-snapshots-01SEP2026.md b/docs/adr/0005-imported-extension-snapshots-01SEP2026.md index 7f21de1..8d42198 100644 --- a/docs/adr/0005-imported-extension-snapshots-01SEP2026.md +++ b/docs/adr/0005-imported-extension-snapshots-01SEP2026.md @@ -22,4 +22,6 @@ Rename the private `pi-workflow` extension identity to `workflow`. Its workspace - Published packages use the scoped Gitea registry instead of Pi-managed Git sources. - Upstream movement is detected but never merged automatically; imported changes require reconciliation with monorepo-specific manifests and APIs. - Only `main` is tracked. Source tags, branches, and historical commits are not copied. -- The renamed `workflow` extension remains a private historical snapshot. [ADR 0006](./0006-skill-led-development-workflow-04SEP2026.md) supersedes its merge/release prerequisite with the skill pilot and a later retirement decision. +- The renamed `workflow` extension remains private until its `_workflows` dependencies are merged and isolated packaging succeeds. + +> **Later decision:** [ADR 0006](./0006-skill-led-development-workflow-04SEP2026.md) supersedes this workflow-specific merge and release direction with the skill pilot and a later retirement decision. diff --git a/skills/develop/develop-workflow/SKILL.md b/skills/develop/develop-workflow/SKILL.md index 82200c2..14491aa 100644 --- a/skills/develop/develop-workflow/SKILL.md +++ b/skills/develop/develop-workflow/SKILL.md @@ -15,17 +15,21 @@ Choose the smallest sufficient path before mutation: - **Light:** make one obvious low-risk edit requiring at most two steps. Inspect, edit, run focused checks, and summarize. Record why this is safe when near the boundary. - **Full:** use the phases below for three or more steps, multiple files or modules, ambiguity, architecture, dependencies, migrations, security implications, non-trivial UI, or meaningful regression risk. -Promote light work to full before editing when discovery reveals meaningful scope or risk. Apply risk gates to every path. +Promote light work to full before editing when discovery reveals meaningful scope or risk. **Complete when:** observed scope and risk justify the selected path. +## Risk gates for every path + +Ask just in time before adding, removing, or materially upgrading dependencies; schema/data migration; production or live-database access; public API or compatibility breaks; destructive Git/filesystem actions outside the narrowly proven final cleanup; force push or shared-history rewrite; non-fast-forward integration; conflict resolution that changes approved behavior; material scope or acceptance expansion; security-posture changes; publishing; or deployment. + +Present concrete evidence, scope the authorization, and mark one option recommended with its reason. Checkpoint the decision before acting. Treat approval as specific to that action; ask again when later evidence broadens it. Promote light work to full before editing when meaningful risk appears. + ## 1. Preflight and isolate -For the full path, require Git worktrees, a writable repository record, `ask_user`, Git-capable independent reviewers, and target-interface validation when UI applies. Stop before product mutation when a mandatory capability is unavailable. In print or JSON mode, emit the pending gate or validation instructions and stop. +For the full path, require Git worktrees, a writable repository record, `ask_user`, Git-capable independent reviewers, and target-interface validation when UI applies. Stop before product mutation when a mandatory capability is unavailable. Let task tools degrade to the retained record and let `artifact-presenter` degrade to reporting the exact artifact path; neither optional convenience may bypass a gate. In print or JSON mode, emit the pending gate or validation instructions and stop. -Read repository instructions, relevant docs/ADRs/tests, and Git state. Capture the source branch and exact base commit. Create `./.worktrees/` on a descriptive feature branch. Treat collisions, unmerged paths, unexpected ownership, and an unignored worktree directory as blockers. For a dirty source checkout, list omitted changes and obtain explicit approval before proceeding. - -Use explicit `cd && ...` or `git -C ...`; prove the top-level path before every shell mutation. Prepare a unique record from [`assets/workflow-record.md`](assets/workflow-record.md) and publish it atomically with the checkpoint helper in [evidence and recovery](references/evidence-and-recovery.md). Create the task tree, run applicable documentation checks, and commit the initial record before product mutation. Use the helper for every later meaningful update. +Read repository instructions, relevant docs/ADRs/tests, and Git state. Follow [Git and integration](references/git-and-integration.md) to capture source evidence, resolve dirty-source approval, and establish the isolated branch/worktree. Prepare a unique record from [`assets/workflow-record.md`](assets/workflow-record.md) and publish it with the helper in [evidence and recovery](references/evidence-and-recovery.md). Create the available live progress view, run applicable documentation checks, and commit the initial record before product mutation. **Complete when:** worktree identity is proven and the initial record commit names the next safe action. @@ -64,11 +68,9 @@ Retain candidates on a dedicated throwaway branch and record its commit. Present ## 5. Implement owned slices -State exclusive file ownership before parallel mutation. Serialize by default; parallelize only disjoint low-integration paths. Give each implementer the approved plan, decisions, owned paths, acceptance criteria, test seam, allowed commands, and deadline. Tell workers to stop at the deadline and leave commits, integration, user interaction, and mutation delegation to the parent. +State one owner and exclusive paths for every mutating assignment. Serialize by default; parallelize only disjoint low-integration paths. Give each implementer the approved plan, decisions, acceptance criteria, test seam, allowed commands, and deadline. Tell workers to stop at the deadline and leave commits, integration, user interaction, and mutation delegation to the parent. -Capture branch, HEAD, status, changed paths, and a patch artifact before and after every mutating assignment. Freeze all mutation on unexpected commits, out-of-ownership paths, deadline expiry, missing delivery, or unknown child liveness. Follow the recovery choices in [evidence and recovery](references/evidence-and-recovery.md); never retry mutating work until the previous child is proven terminal. - -Use TDD at agreed seams where practical. Run focused tests after behavioral slices. Inspect each returned diff and repository state before marking it complete. +Follow [assignment evidence and recovery](references/evidence-and-recovery.md#assignment-evidence) before accepting, retrying, or reassigning mutation. Use TDD at agreed seams where practical. Run focused tests after behavioral slices. Inspect each returned diff and repository state before marking it complete. **Complete when:** every acceptance criterion has intentional owned code and test evidence. diff --git a/skills/develop/develop-workflow/scripts/checkpoint-record.mjs b/skills/develop/develop-workflow/scripts/checkpoint-record.mjs index 147fbb0..f985252 100644 --- a/skills/develop/develop-workflow/scripts/checkpoint-record.mjs +++ b/skills/develop/develop-workflow/scripts/checkpoint-record.mjs @@ -234,33 +234,18 @@ async function syncDirectory(io, directory) { } } -function requestFromArgs(targetOrRequest, input, options) { - if (typeof targetOrRequest === "string" || targetOrRequest instanceof URL) { - return { ...(options ?? {}), target: targetOrRequest, input }; - } - if (targetOrRequest === null || typeof targetOrRequest !== "object") { +/** Atomically create or replace a validated workflow record. */ +export async function checkpointRecord(request) { + if (request === null || typeof request !== "object") { throw new CheckpointError("INVALID_REQUEST", "checkpoint request must provide a target and input"); } - return targetOrRequest; -} - -/** - * Atomically create or replace a validated workflow record. - * - * Object form: `checkpointRecord({ target, input, expectedSha256, io })`. - * Positional form is also supported by `recordCheckpoint(target, input, options)`. - */ -export async function checkpointRecord(targetOrRequest, input, options) { - const request = requestFromArgs(targetOrRequest, input, options); - const targetValue = request.target; - if (typeof targetValue !== "string" && !(targetValue instanceof URL)) { + if (typeof request.target !== "string" || request.target.length === 0) { throw new CheckpointError("INVALID_REQUEST", "checkpoint target is required"); } - const target = resolve(targetValue instanceof URL ? fileURLToPath(targetValue) : targetValue); - const source = request.input ?? request.content; - // This deliberately happens before opening the lock or reading the destination. - const { bytes } = validateRecord(source); - const expected = expectedDigest(request.expectedSha256 ?? request.expectedDigest); + const target = resolve(request.target); + // Validate before opening the lock, creating a directory, or reading the destination. + const { bytes } = validateRecord(request.input); + const expected = expectedDigest(request.expectedSha256); const io = withIo(request.io); const lock = `${target}.lock`; const directory = dirname(target); @@ -398,12 +383,6 @@ export async function checkpointRecord(targetOrRequest, input, options) { return result; } -export async function recordCheckpoint(target, input, options = {}) { - return checkpointRecord(target, input, options); -} - -export const writeCheckpoint = recordCheckpoint; - function usage() { return "Usage: node checkpoint-record.mjs [--input ] [--expected-sha256 ]"; }