[codex] Add implementation review workflow#1251
Draft
defin85 wants to merge 1 commit into
Draft
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
clay-good
added a commit
to clay-good/OpenSpec
that referenced
this pull request
Jun 29, 2026
…sign After a comprehensive sweep of open issues, PRs, and discussions, grounded the proposal in the complete adjacent landscape and answered the open design questions the cluster raises: - Fission-AI#783 (Cross-artifact quality review before apply) is now a primary Closes: it IS audit mode. Answer its open "new skill vs. extend validate" question via the determinism split — deterministic checks (drift/completeness/coverage) are CLI/validate-shaped; the semantic cross-artifact review is the skill. Added a skill spec scenario for the Fission-AI#783 patterns (scope contradiction, spec gap, duplication). - Discussion Fission-AI#1206 ("refine proposal now?") + prior-art PR Fission-AI#372: official answer is /opsx:update. - New design Decision 8 (command family): delineate /opsx:update from /opsx:clarify (Fission-AI#702, within-artifact), /opsx:review (Fission-AI#1251, plan-vs-code), and verify; /opsx:update consolidates update+regen+refine into one action, addressing skill-sprawl (Fission-AI#1263, Fission-AI#783). - Reuse, don't reinvent: audit's empty/incomplete check reuses Fission-AI#1098's artifactOutputComplete (same outputs.ts the digest helper lives in); capability coverage reuses Fission-AI#1277's validateChangeCapabilityCoverage. - New open questions: surface deterministic coherence in `validate` for a CI gate (Fission-AI#783-B, Fission-AI#829); naming reconciliation with Fission-AI#783's /opsx:refine. - Confirmed add-update-command* branches are the `openspec update` tool-file refresh (not artifact update) — no collision. Validates clean under --strict; 10 deltas; all relative links resolve. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
clay-good
added a commit
to clay-good/OpenSpec
that referenced
this pull request
Jun 29, 2026
…ission-AI#880 Grounded the surface so an implementer builds it without guessing, and added proportionate forward-compatibility: - New design "Data contracts" section with exact shapes: extended ArtifactStatus (requires/dependents/digest/drift/driftFrom — additive to the real interface at instruction-loader.ts:120), the --impact response, and the `.openspec.yaml` baselines ledger. All additive; nothing existing changes type. - Digest scheme tag (`sha256-relpath-v1:`) + forward-compat: drift compares only same-scheme digests; an unrecognized/older scheme reports `unknown` rather than silently mis-comparing — re-reconcile restores it. Added a cli spec scenario and tasks for it. - Grounded the ledger write: there is no central change-metadata writer today (change-metadata/index.ts only re-exports schema), so reconcile does a safe read-modify-write of .openspec.yaml mirroring the store's parse/serialize/writeStoreMetadataState pattern (foundation.ts). - Coverage: re-swept; folded Fission-AI#880 (/opsx:validate code-vs-living-specs) into the plan-vs-code delineation alongside Fission-AI#1251/Fission-AI#1073. Main unchanged (546224e); all citations still valid. Validates clean under --strict; 10 deltas; links resolve. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
clay-good
added a commit
to clay-good/OpenSpec
that referenced
this pull request
Jul 7, 2026
…ssion-AI#1278) * docs(openspec): propose add-update-workflow — graph-driven /opsx:update + cohesive audit Dogfooded OpenSpec proposal for the missing first-class "update" action: a /opsx:update workflow that propagates an edit to one artifact across its downstream dependents (targeted mode) or audits a whole change for stale/ incoherent artifacts (audit mode) — driven by the schema's artifact graph, never hardcoded filenames, editing planning artifacts only (never code). - artifact-graph: expose reverse-dependency queries (getDependents/getDownstream) + a requires-edge mtime staleness signal (the engine already builds the dependents map at graph.ts:98 and discards it). - cli-artifact-workflow: surface requires/dependents/stale on `openspec status --json` and add a `--impact <artifact>` downstream-revisit-order selector. - opsx-update-skill: the user-facing /opsx:update command (targeted + audit). Supersedes the proposal-only stub add-artifact-regeneration-support. Addresses the cluster Fission-AI#1188/Fission-AI#705/Fission-AI#673/Fission-AI#247 (closes), Fission-AI#694/Fission-AI#684/Fission-AI#618 (answers), and is graph-driven to avoid the Fission-AI#777/Fission-AI#666 hardcoded-artifact-pattern bug class. Validates clean under `openspec validate --strict`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(openspec): make add-update-workflow deterministic & grounded (Tabish review) Reframe per the steer "more deterministic and grounded in reality": - Deterministic spine: the CLI computes the impact set (which downstream artifacts to revisit, in build order, with paths) as a pure function of schema edges + filesystem. The agent only rewrites prose. Grounded in real APIs already present: getUnlockedArtifacts (direct dependents), getBuildOrder (order), resolveArtifactOutputs (paths); reverse map built at graph.ts:82-87. - Replace fragile mtime staleness with a newline-normalized SHA-256 content digest (reproducible cross-platform). Drift = upstream digest vs recorded baseline; no baseline => "unknown", never a false positive. mtime and pure-git rejected with rationale; digest ledger is a separable, optional layer. - Explicit determinism boundary decision (CLI decides files/order/drift; agent rewrites). Skill MUST source the file list/order from `openspec status --impact`, never compute it. - Corrected all code citations to verified lines (graph.ts:82-87, instruction-loader.ts:366/429, status.ts); noted Fission-AI#1277's coverage helpers are not in this branch's base (coordinate, don't reuse). - Specs updated: artifact-graph Content Digest requirement; cli status digest + deterministic impact ordering; skill determinism + baseline-aware audit. tasks add digest/determinism/cross-platform tests + optional ledger section. Still validates clean under `openspec validate add-update-workflow --strict`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(openspec): harden add-update-workflow determinism; drop direct name refs - Digest ledger tracks DIRECT upstream digests; document that transitive drift emerges hop-by-hop as downstream is reconciled (no transitive bookkeeping). - Ground audit's no-baseline structural facts on signals available in this branch (missing/empty output, blocked/incomplete); capability-coverage is an add-on only when Fission-AI#1277's validateChangeCapabilityCoverage is present. - Add the "update revises only existing downstream; defer not-yet-created ones to /opsx:continue" rule across proposal/design/specs/tasks; impact entries now carry existence/status. - Note artifact-level (not file-level) granularity and that getDownstream terminates by the schema's acyclic guarantee. - Remove direct personal references from the docs. Validates clean under `openspec validate add-update-workflow --strict`; 10 deltas. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(openspec): full issue/PR/discussion coverage + command-family design After a comprehensive sweep of open issues, PRs, and discussions, grounded the proposal in the complete adjacent landscape and answered the open design questions the cluster raises: - Fission-AI#783 (Cross-artifact quality review before apply) is now a primary Closes: it IS audit mode. Answer its open "new skill vs. extend validate" question via the determinism split — deterministic checks (drift/completeness/coverage) are CLI/validate-shaped; the semantic cross-artifact review is the skill. Added a skill spec scenario for the Fission-AI#783 patterns (scope contradiction, spec gap, duplication). - Discussion Fission-AI#1206 ("refine proposal now?") + prior-art PR Fission-AI#372: official answer is /opsx:update. - New design Decision 8 (command family): delineate /opsx:update from /opsx:clarify (Fission-AI#702, within-artifact), /opsx:review (Fission-AI#1251, plan-vs-code), and verify; /opsx:update consolidates update+regen+refine into one action, addressing skill-sprawl (Fission-AI#1263, Fission-AI#783). - Reuse, don't reinvent: audit's empty/incomplete check reuses Fission-AI#1098's artifactOutputComplete (same outputs.ts the digest helper lives in); capability coverage reuses Fission-AI#1277's validateChangeCapabilityCoverage. - New open questions: surface deterministic coherence in `validate` for a CI gate (Fission-AI#783-B, Fission-AI#829); naming reconciliation with Fission-AI#783's /opsx:refine. - Confirmed add-update-command* branches are the `openspec update` tool-file refresh (not artifact update) — no collision. Validates clean under --strict; 10 deltas; all relative links resolve. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(openspec): resolve open questions to committed decisions; drift in scope Per review steer, every open question is now a committed happy-path decision so build-out has no dangling forks, and the deterministic drift baseline is pulled into scope (it is what makes audit-mode drift deterministic vs. agent-guessed): - Digest ledger IN SCOPE (design Decision 3): per-artifact DIRECT upstream digests in ChangeMetadataSchema, written by a deterministic `openspec status --record`; pre-existing changes (no baseline) degrade to drift `unknown` + structural checks. Generating-flow auto-recording stays optional (graceful). - cli-artifact-workflow spec: folded drift into the digest requirement (record baseline / drift vs baseline / unknown-without-baseline) — stays at 10 deltas. - opsx-update-skill spec: skill records baseline via `--record` after each confirmed edit, so audits clear once reconciled. - Replaced "## Open Questions" with "## Decisions resolved": ledger in scope; targeted entry baseline-aware; apply stays standalone (points to update on drift); cross-change (Fission-AI#247), continue/ff de-hardcoding (Fission-AI#777), and validate CI-gate (Fission-AI#783-B/Fission-AI#829) are named follow-ups, not deferrals of the core feature; /opsx:update kept as the umbrella name. - Migration Plan + Capabilities + Impact + tasks updated; status JSON gains `drift`, CLI gains `--record`. Re-synced with upstream main (0 behind). Validates clean under --strict; 10 deltas; all links resolve. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(openspec): harden add-update-workflow — close cross-OS, read-only, edge gaps Stress-tested every claim against live source and fixed the soft spots: - Cross-OS digest determinism (real bug): resolveArtifactOutputs (outputs.ts:34) sorts ABSOLUTE paths via .sort(), which differs by OS — so a multi-file glob artifact (specs/**/*.md) would hash differently on Windows vs POSIX. Digest now specified to order files by change-relative forward-slash path and hash relpath+content. Added spec scenarios (cross-platform glob stability; rename changes digest) and a cross-OS test task. - Read-only status invariant: moved baseline recording OFF `openspec status` (a read command silently mutating the drift reference is a footgun) to a dedicated `openspec reconcile` write verb. Updated spec, skill, design, impact, capabilities, tasks; reconciled the "no new verb" claims. - Edge case: missing upstream at record time is stored as an explicit `absent` marker so later creating it registers as drift (spec scenario added). - Edge case: coherent change yields no edits (clean-path scenario). - Grounding fixes: continue-change hardcoded block is duplicated (skill 103-112 + command 225-234) — both must be fixed in the Fission-AI#777 follow-up; verified no content-hash util exists. - Fixed two stale claims the layered edits left: the Impact digest bullet (concatenation→relative-path) and the naming-boundary line. Validates clean under --strict; 10 deltas (4+3+3), 44 scenarios; all links resolve; re-synced with upstream main (0 behind); issue/PR/discussion sweep re-run, no new items. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(openspec): pin data contracts + digest forward-compat; delineate Fission-AI#880 Grounded the surface so an implementer builds it without guessing, and added proportionate forward-compatibility: - New design "Data contracts" section with exact shapes: extended ArtifactStatus (requires/dependents/digest/drift/driftFrom — additive to the real interface at instruction-loader.ts:120), the --impact response, and the `.openspec.yaml` baselines ledger. All additive; nothing existing changes type. - Digest scheme tag (`sha256-relpath-v1:`) + forward-compat: drift compares only same-scheme digests; an unrecognized/older scheme reports `unknown` rather than silently mis-comparing — re-reconcile restores it. Added a cli spec scenario and tasks for it. - Grounded the ledger write: there is no central change-metadata writer today (change-metadata/index.ts only re-exports schema), so reconcile does a safe read-modify-write of .openspec.yaml mirroring the store's parse/serialize/writeStoreMetadataState pattern (foundation.ts). - Coverage: re-swept; folded Fission-AI#880 (/opsx:validate code-vs-living-specs) into the plan-vs-code delineation alongside Fission-AI#1251/Fission-AI#1073. Main unchanged (546224e); all citations still valid. Validates clean under --strict; 10 deltas; links resolve. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(openspec): simplify add-update-workflow to a thin /opsx:update skill Rework per @TabishB review (PR Fission-AI#1278): the proposal over-built. Drop the deterministic-spine machinery and lean on the existing status command. - Cut the reverse-dependency graph API (getDependents/getDownstream), SHA-256 content digests, the .openspec.yaml baseline ledger, the `openspec reconcile` write op, the drift report, and `status --impact`. Removes the artifact-graph and cli-artifact-workflow spec deltas. - Reframe propagation as bidirectional coherence (editing design can require revising proposal), not downstream-only. - Center the feature on one thin skill over the existing `openspec status` / `openspec list`; design now sketches the actual minimal skill instruction body ("written by hand"). - v1 adds no new CLI/graph/schema code: just update-change.ts + wiring. Validates clean: `openspec validate add-update-workflow --strict`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(update-workflow): pin the status path contract to existingOutputPaths Address @alfred-openspec's review: the skill's write target was described loosely as "resolved paths." Make it precise across proposal/design/spec/tasks: - `openspec status --json` already returns everything the skill needs, in the top-level `artifactPaths` map — `resolvedOutputPath` and `existingOutputPaths` per artifact. No new CLI field is required. - The skill edits `existingOutputPaths` (the concrete, glob-expanded files) and never writes to `resolvedOutputPath`, which for a glob artifact like `specs/**/*.md` remains the glob pattern rather than a real file. - Add spec scenarios for editing a glob artifact's concrete files and for deferring a brand-new file under a glob artifact to `/opsx:continue`. - Tighten the cross-platform scenario and add a template test (3.4) asserting the write target is `existingOutputPaths`, not a glob `resolvedOutputPath`. Validates clean under `openspec validate add-update-workflow --strict`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(update-workflow): address review — default profile, next-step guidance, change-scoped naming - Register /opsx:update in the default core profile, not expanded-only (maintainer call on the PR) - Add next-step guidance: after updating, recommend /opsx:continue, /opsx:apply (esp. when the change was already implemented), or /opsx:archive — guidance only, never acted on - Pin naming scope: skill openspec-update-change, change proposals only; generalizing update to other graph types is an explicit non-goal Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(skills): implement the /opsx:update skill (openspec-update-change) Implements the approved add-update-workflow change: one thin skill over the existing status/list commands, in the default core profile. - new update-change.ts template (skill + command), registered across init, profiles, skill-generation, tool-detection, profile-sync-drift - update joins CORE_WORKFLOWS and ALL_WORKFLOWS - docs: opsx.md command row + usage note, commands.md reference section, supported-tools.md skill list - retire the superseded add-artifact-regeneration-support stub - template tests pin the guardrails (schema-driven ids, planning-only, existingOutputPaths write contract, next-step guidance); parity hashes regenerated; profile/init/update/config tests cover the new core set - tasks.md checked off; validate --strict passes Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Summary
reviewworkflow exposed as/opsx:reviewandopenspec-review-impl-vs-planWhy
/opsx:verifyis useful for archive readiness, but mature brownfield projects often need a stricter read-only implementation review that traces plan items to actual code, default runtime wiring, tests, config, examples, and docs before accepting a change as implemented.Validation
pnpm exec vitest run test/core/profiles.test.ts test/core/shared/skill-generation.test.ts test/core/templates/skill-templates-parity.test.ts test/commands/config-profile.test.ts test/commands/config.test.tspnpm run buildpnpm run lint(passes with an existing warning insrc/core/references.ts)node bin/openspec.js validate --specs --no-interactiveGenerated with Codex using GPT-5.