refactor(skills): drop founder-validation framing from spec pipeline#4
Merged
Merged
Conversation
Spec phase skills (/forge, /draft-prd, /draft-design, /ingest-spec) and their templates were asking founder-validation questions (unfair advantage, north-star metric, kill criteria) instead of product/spec questions. /draft-design also baked in an @inherit pattern that couples adopter projects to a maintainer-global brand book. Reframe the spec phase around engineering-delivery, not startup validation: - /forge: 6 forcing questions -> 4 product questions (what/who, v1 scope, non-goals, open unknowns). Drop "Heavy ceremony" framing. - /draft-prd: add per-feature discovery loop (what/flow/acceptance/ edge-cases/non-goals per v1 feature). Drop north-star references. - /draft-design: drop @inherit pattern entirely. Each project owns its DESIGN.md or references an external asset as a generation guideline. Init prompt asks project_owned vs reference_external. - /ingest-spec: validation checklist updated for new BRIEF/PRD/DESIGN shape. Drop north-star + @inherit checks. - Templates (BRIEF/PRD/DESIGN) updated to match. - README: "brand-book inheritance" bullet -> "project-owned design systems". - .gitignore: exclude forge dogfooding artifacts (spec/, plans/, .forge/) so adopters and this repo don't accidentally publish project-specific files. BREAKING for adopters who configured @inherit in DESIGN.md. The forge migrate command (shipping in v0.3.0) auto-converts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
@codex test and review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f932e40e3b
ℹ️ 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".
This was referenced May 13, 2026
firatcand
added a commit
that referenced
this pull request
May 13, 2026
* [FORGE-67] ci: add GitHub Actions workflow (typecheck/test/build/smoke/gitleaks) First .github/workflows/ in the repo. Wires ETHOS principle #4 ("Test-or-die — every PR ships with tests") into an automated gate so every PR to main is verified before merge instead of relying on the author's word that local tests passed. Five jobs, Node 22.x + 24.x matrix: 1. typecheck — `tsc --noEmit`; fastest gate, no `needs:` 2. test — `node --test --import tsx 'test/**/*.test.ts'`; runs in parallel with typecheck 3. build — `tsdown`; needs typecheck+test green, uploads dist-node-{version}/ as a per-matrix artifact 4. smoke — `node dist/bin/forge.cjs --version` + `--help`; downloads the matching dist artifact and exercises the bundled CJS binary (the gate that catches bundler-only regressions per docs/learnings/2026-Q2/ship-gates-missed-build-step.md) 5. gitleaks — `gitleaks-action@v2`; parallel from start, auto-selects diff mode on PR / full-history mode on push to main Concurrency cancel-in-progress on PR only (not on push:main) so the merge history retains every run as an audit record. Branch protection (`gh api ... PUT`) is a manual post-merge step — the required-status-check names only stabilise after the first successful run. The plan and FORGE-67 issue describe the exact command. Plan: plans/tasks/FORGE-67.plan.md (gitignored per dogfooding rule). * [FORGE-62] fix(tests): remove phases.test.ts AC3 — duplicates AC1 + reads gitignored file Surfaced by the new CI in this branch (FORGE-67). Test failed in both matrix legs because it reads `<worktreeRoot>/plans/phases.yaml`, which is gitignored by the forge-dogfooding rule and therefore absent from any fresh git worktree — including every worktree CI runs on. This is the exact bug FORGE-62 was filed to fix. FORGE-62 is marked Done in Linear but no commit references it — the test migration never landed. This commit completes that work. AC3 was redundant: AC1 already exercises the real-shape parse via the tracked `test/fixtures/phases/live-snapshot.yaml` fixture, which is a committed snapshot of the live phases.yaml taken at decompose-time. The deleted AC3 added no coverage beyond what AC1 gives us — it was a worktree-blindness landmine waiting for CI to step on it. Also drops the now-orphaned `worktreeRoot` const. * [FORGE-67] ci: smoke job installs prod deps before running bundled binary The bundled CJS output keeps `dependencies` (chalk, zod, @inquirer/prompts, etc.) as external require()s — the correct shape for an npm package, since end users get those resolved by `npm install @firatcand/forge`. tsdown's default is to mark `dependencies` as external, so the dist/ artifact is not self-contained on purpose. The initial smoke job design assumed a fully-bundled artifact and skipped both checkout and npm ci. CI caught the mismatch on the first push: both Node 22 + 24 smoke legs failed with `Cannot find module 'chalk'` from dist/schemas-Di0DNjC3.cjs. Locally the smoke worked only because node_modules/ was populated from a prior npm ci — masking the bug. This is precisely the bundler-vs-runtime divergence the smoke job is designed to catch. Working as intended; design was wrong, fix is one step: `npm ci --omit=dev` before exercising the binary. That mirrors how end users actually install and invoke forge from npm. Reference learnings: - ship-gates-missed-build-step.md (this divergence class) - chalk-v5-cjs-bundle-double-wrap.md (chalk-specific bundler footgun)
This was referenced May 14, 2026
firatcand
added a commit
that referenced
this pull request
May 14, 2026
… (Codex review) (#95) Closes 4 implementation bugs and 2 cheap enhancements that an earlier Codex review of FORGE-64 (PR #87) surfaced in the question/answer atomic writer. Architectural choice (link+unlink over rename for OS-level enforcement of the "never overwritten" invariant) is unchanged. - Bug #1: try/finally ensures temp-file cleanup on every code path including DUPLICATE_ID. Cleanup ownership moved from inside placeAtomic to the caller's finally block — single unconditional unlink point. - Bug #2: writeSync loop with Buffer offsets. Short writes are recoverable (NFS / quota / signal-interrupted); writeSync returning 0 with bytes still to write is treated as IO_ERROR rather than an infinite-loop trigger. - Bug #3: closeSync errors surface as IO_ERROR when no prior write/fsync error exists (delayed write-back on NFS/quota paths per close(2)). Prior errors win — close error is informational, prior error is actionable. - Bug #4: payload byte-size cap enforced at the write boundary (PAYLOAD_TOO_LARGE) before any disk I/O. Schema caps strings by UTF-16 code units; the cap is enforced in UTF-8 bytes. A schema-valid payload of 4-byte-per-codepoint characters can exceed 64KB even while passing zod validation — the cap closes that gap. - Enh #6: temp filenames use crypto.randomBytes(8).toString('hex') instead of Math.random(). Closes the hostile-same-UID-process question definitively. - Enh #7: durability contract documented in spec/ORCHESTRATOR.md "File semantics" — file fsync protects against partial writes, not power-loss durability of placement; dispatcher reconciles from tracker on restart. - Enh #5: deferred to plans/BACKLOG.md (capability probe → forge doctor). Internal: adds __fsForTesting export on src/orchestrator/questions/writer.ts as a test seam. node:test's mock.method cannot patch the frozen node:fs Module Namespace directly; the seam is a plain mutable object holding fs method references used internally by the writer. Tests use mock.method on this object to simulate partial writes, closeSync failures, and validate the random-bytes temp-name shape. Not part of the public API. Test coverage: +8 unit tests covering each bug and Enh #6. Total 16 tests in writer.test.ts (was 8), all pass. Full suite 609 pass / 0 fail / 8 pre-existing skipped. Build OK, smoke ./dist/bin/forge.cjs --version OK. Codex second-opinion review on the diff: no findings. Unblocks FORGE-20 (dispatcher core). Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
firatcand
added a commit
that referenced
this pull request
May 17, 2026
Step 7 of plans/tasks/FORGE-113.plan.md.
test/integration/cli/orchestrate/phases.staleness.test.ts (new):
· 25h-stale file: exit 0 + stderr contains
"phases.yaml: ⚠ STALE — synced 1d ago from linear (SPEC@<digest>)"
Exits 0 (file is usable); staleness is surfaced via stderr, not exit
code — matches AC #7 wording ("still works but prints the staleness
duration prominently").
· 23h-fresh file: exit 0, no STALE marker, "synced Nh ago" format.
Pins the 24h boundary the unit tests already cover, exercised
end-to-end through the verb.
· No source block: exit 0, stderr matches documented fallback
"phases.yaml: no source metadata (run forge orchestrate reconcile
--pull to sync)" — confirms AC #4 in the actual CLI surface, not
just in computeFreshnessLine unit tests.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
firatcand
added a commit
that referenced
this pull request
May 17, 2026
…eshness display (P2.5-T17) (#171) * chore(phases): amend FORGE-113 AC + migrate top-level tracker_project_id Pre-implementation migration (Step 1 of plans/tasks/FORGE-113.plan.md): - plans/phases.yaml P2.5-T17 entry: drop tracker_revision from schema + AC; add explicit AC for the breaking removal of top-level tracker_project_id; add AC for missing-source freshness line; update freshness format example. - This repo's plans/phases.yaml: move line-2 tracker_project_id into a new top-level `source:` block (tracker, project_id, synced_at, spec_revision). Schema still accepts the old shape (tracker_project_id was optional; source is currently unknown-key-stripped). Pre-stages the file for the schema change in Step 2. Decision: tracker_revision dropped → live-drift detection deferred to follow-up issue (filed in Step 10) per user answer after Codex 2nd-pass. Decision: source.project_id replaces top-level tracker_project_id (breaking) per user answer; tracker_url stays at top level per user answer on plan review. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(schemas): add Source + remove tracker_project_id from PhasesSchema (FORGE-113) Step 2 of plans/tasks/FORGE-113.plan.md. - Introduce TRACKER_TYPES enum + SourceSchema (zod, strict): { tracker, project_id, synced_at: ISO-8601, spec_revision } - Attach `source: SourceSchema.optional()` to PhasesSchema - Remove `tracker_project_id` from PhasesSchema top level (breaking) - Export `Source` and `TrackerType` types Tests: - Replace existing tracker_project_id+tracker_url combined test with a tracker_url-only test - Add coverage for: optional source-block round-trip, rejected unknown tracker enum, rejected non-ISO synced_at, rejected extra keys inside source (strict mode catches tracker_revision and other unintended fields) Decision: SourceSchema is `.strict()` so the deliberately-omitted `tracker_revision` field cannot be silently smuggled back in by a hand-edit or stale tool. Future addition (FORGE-XXX follow-up for live-drift detection) will require an explicit schema change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(core): add freshness + spec-revision helpers (FORGE-113) Step 3 of plans/tasks/FORGE-113.plan.md. Pure helpers behind the freshness summary printed by every CLI verb that reads phases.yaml. src/core/freshness.ts: - formatRelativeAge(ms): "just now" | "Nmin" | "Nh" | "Nd" - computeFreshnessLine(source, now): two output shapes · `phases.yaml: synced 47min ago from linear (SPEC@8fa2226)` · `phases.yaml: ⚠ STALE — synced 1d ago from linear (SPEC@8fa2226)` when age > 24h, OR the documented fallback when source is absent. - 24h stale threshold is exclusive (24h exactly is not stale). - Defensive branch for unparseable synced_at past the zod boundary. src/core/spec-revision.ts: - computeSpecRevision(cwd): git sha of HEAD when spec/SPEC.md was last touched, falling back to a 40-char sha256 content digest when the file is untracked or the directory isn't a git working tree. - Throws SpecRevisionError if spec/SPEC.md doesn't exist — that's a misconfiguration, not a defensive fallback. - Uses execFileSync (not exec) for the git invocation; arg array prevents shell injection paths from a hostile cwd. Tests cover: relative-age boundaries, 24h stale boundary on both sides, missing-source fallback string, unparseable defense, all 3 tracker types verbatim, short spec_revision slicing; git-sha for tracked, content-digest for untracked + non-git, last-commit-touching-SPEC.md semantics (HEAD advancing without touching SPEC.md doesn't change the rev), throw on missing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(core,cli): loadPhases returns {phases, freshnessLine} (FORGE-113) Step 4 of plans/tasks/FORGE-113.plan.md. Loader contract change (decision: FORGE-113 plan §0 Q1): - `loadPhases(path): Phases` → `loadPhases(path): LoadPhasesResult` where LoadPhasesResult = { phases, freshnessLine }. - freshnessLine is computed at parse time via computeFreshnessLine against the optional `source` block. Loader stays pure of stderr I/O; each caller decides where (or whether) to surface the line. - Failures (FILE_NOT_FOUND, PARSE_ERROR, SCHEMA_INVALID) still throw PhasesError — the new return shape applies only to the success path. Callers updated to plumb freshnessLine to stderr before main output: - src/cli/orchestrate/phases.ts:87 - src/bin/sync-status-render.ts:31 (The third reader, reconcile's loadPhasesWithDocument, is handled in Step 6 alongside its --pull write path.) Tests: - test/unit/core/phases.test.ts updated to the new return shape on the two success-path assertions; new "result includes a freshnessLine string" coverage. Error-path tests unchanged — throw semantics preserved. Schema cleanup: - Removed duplicate `export type TrackerType` from src/schemas/phases.ts (canonical alias remains in src/trackers/types.ts; barrel re-export was ambiguous through src/index.ts). - src/core/index.ts re-exports LoadPhasesResult, Source, computeFreshnessLine, formatRelativeAge, computeSpecRevision, SpecRevisionError for downstream consumers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(reconcile): stamp source stanza + freshness stderr on --pull (FORGE-113) Step 5 of plans/tasks/FORGE-113.plan.md. src/cli/orchestrate/reconcile.ts: - loadPhasesWithDocument now returns {phases, doc, raw, freshnessLine}; caller prints freshnessLine to stderr immediately after a successful load, before any further verb work. - runPull resolves a Source object and installs it on the YAML Document via setIn(['source', ...]) before writeAtomic. Existing doc-level mutations from applyPlanToDocument are preserved. - runPull now writes phases.yaml UNCONDITIONALLY on a successful --pull (not gated on mutationsDoc > 0). Rationale: synced_at means "last successful sync attempt", not "last mutation". A zero-diff --pull is still a successful sync and should bump the timestamp. `applied` in the JSON envelope continues to reflect plan mutations only (so consumers checking "did issues change?" still get the right answer). Source resolution prefers, in order: 1. The existing `source.project_id` (preserved across --pull runs) 2. The legacy top-level `tracker_project_id` (migration path; the schema strips this on parse but the raw Document still has it until the first --pull rewrites the file). Throws SOURCE_RESOLUTION_FAILED if neither is found — fail loudly rather than fabricate a project_id. setSourceOnDocument also deletes the legacy top-level tracker_project_id key from the Document — idempotent migration runs on every --pull until adopters are fully on the v0.4 shape. Tests: - MINIMAL_PHASES fixture extended with a `source:` block so all existing tests survive the new resolution requirement. - mkScratchWorktree now seeds spec/SPEC.md (computeSpecRevision needs it; falls back to content digest in the absence of git). - stripFreshness() helper for tests that JSON.parse stderr — the freshness line is prepended ahead of any structured envelope. - New unit tests: · "--pull writes source stanza atomically" (AC #3) · "--pull migrates legacy tracker_project_id into source" (AC #2) · "freshness line printed to stderr before main output" - E2E PHASES_FIXTURE updated identically; scratch dir gets SPEC.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(integration): phases staleness e2e (FORGE-113 AC #7) Step 7 of plans/tasks/FORGE-113.plan.md. test/integration/cli/orchestrate/phases.staleness.test.ts (new): · 25h-stale file: exit 0 + stderr contains "phases.yaml: ⚠ STALE — synced 1d ago from linear (SPEC@<digest>)" Exits 0 (file is usable); staleness is surfaced via stderr, not exit code — matches AC #7 wording ("still works but prints the staleness duration prominently"). · 23h-fresh file: exit 0, no STALE marker, "synced Nh ago" format. Pins the 24h boundary the unit tests already cover, exercised end-to-end through the verb. · No source block: exit 0, stderr matches documented fallback "phases.yaml: no source metadata (run forge orchestrate reconcile --pull to sync)" — confirms AC #4 in the actual CLI surface, not just in computeFreshnessLine unit tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs,templates,skills: migrate references to source block (FORGE-113) Step 8 of plans/tasks/FORGE-113.plan.md. Aligns all surfaces that reference the removed top-level `tracker_project_id` field with the new `source` block. - templates/phases.template.yaml: drop top-level `tracker_project_id`; add commented `source:` block scaffold so adopters see the post-reconcile shape on first inspection. Real values are populated by /push-to-tracker → /reconcile --pull. - test/fixtures/orchestrator/phases.yaml: migrate to source block. - skills/push-to-tracker/SKILL.md: update both occurrences — the tracker-agnostic-keys list and the update-mode edge-case comment. - docs/LIFECYCLE.md: update the /push-to-tracker outputs description. - docs/EXAMPLES.md: update the post-bootstrap line. - docs/trackers/linear.md: update the post-push field list. Migration is automatic for existing adopter projects: the first /reconcile --pull transplants legacy top-level tracker_project_id into source.project_id and deletes the legacy key. No adopter action needed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(spec): align §source block + freshness format with shipped reality (FORGE-113) Step 9 of plans/tasks/FORGE-113.plan.md. Surgical update — does NOT refresh the broader stale §phases.yaml schema code block (filed as follow-up). §`phases.yaml` is a derived snapshot: - Source block field list now matches shipped schema: `{tracker, project_id, synced_at, spec_revision}` (no tracker_revision). - Freshness format updated to "from <tracker>" (no "@<tracker_revision>"): `phases.yaml: synced 47min ago from linear (SPEC@a3c2d1f)`. - Added the missing-source fallback string verbatim. - Documented the 24h "⚠ STALE — " prominence prefix and the "exit 0 + stderr surfacing" semantic. - Rationale paragraph for the deliberate absence of tracker_revision: no v0.4 consumer; the right shape for live drift detection is Tracker.getCurrentRevision() (provider-native cheap rev), filed as a follow-up issue. - Breaking change note: top-level tracker_project_id moved into source.project_id; migration is automatic on first --pull. §`phases.yaml` schema (zod): - Added single-line note pointing readers at src/schemas/phases.ts as canonical (the in-spec code block predates FORGE-96 lifecycle fields and FORGE-113 source block; broader refresh deferred). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(spec): point follow-up reference at filed FORGE-123 (FORGE-113) Step 10 of plans/tasks/FORGE-113.plan.md. Filed FORGE-123 (Add Tracker.getCurrentRevision() for live drift detection) and updated the SPEC.md rationale paragraph to reference the real issue ID. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (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
/forgewith 4 product/spec questions; reframe as engineering-delivery, not startup validation/draft-prd; drop north-star references/draft-designaround project-owned vs reference-external (drop@inherit)/ingest-specvalidation checklist for new BRIEF/PRD/DESIGN shape.gitignoreWhy
The spec pipeline was asking startup-validation questions (unfair advantage, north-star metric, kill criteria) when forge's actual user is a developer with an idea who wants delivery structure, not idea validation. The
@inheritpattern in/draft-designcoupled adopter projects to a maintainer-global brand book — wrong for a tool meant to be installed by other people.Breaking changes
@inheritlines inspec/DESIGN.mdneed migration. Theforge migratecommand shipping in v0.3.0 will auto-convert with diff preview.Test plan
npx @firatcand/forge installstill installs all skills cleanly/forgeagainst a fresh project — confirm 4 product questions appear/draft-designon a project with no brand assets — confirm project_owned vs reference_external prompt appears/ingest-specagainst a v0.2.1-shape project — confirm clear validation errors point at the new required sections🤖 Generated with Claude Code