chore: decommission legacy bash install flow#3
Merged
Conversation
Removes the original bash-based install path now that npm publishing is the canonical adopter route: - delete `setup.sh` (87 lines): symlinked skills/agents into ~/.claude and the bash forge launcher into ~/.local/bin/forge — both jobs are now done by `npx @firatcand/forge install` - delete `forge` (168 lines): bash CLI with `forge init`, `forge templates`, `forge upgrade`, `forge version` — all replaced by bin/forge.js subcommands shipped via npm - README: drop the "Prefer the original bash flow?" fallback line - CONTRIBUTING: update bug-fix scope to `bin/forge.js` + `lib/tools.js`, replace `bash -n setup.sh forge lib/*.sh` validation with `node --check bin/forge.js lib/*.js` plus `bash -n lib/*.sh` (the lib helpers stay) Why: dual install paths (bash + npm) are exactly the kind of drift that caused this PR's parent investigation. Consolidating to a single canonical flow removes a documented-but-stale fallback. CHANGELOG entry for v1.0.0 deliberately left intact as historical record. Local impact: the orphaned `~/.local/bin/forge` symlink that was created by setup.sh on this machine has already been removed manually. Future contributors who clone fresh use `npm install` then `node bin/forge.js`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
…RGE-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>
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
Removes the original bash-based install path. npm publishing (
npx @firatcand/forge) is now the canonical adopter route — dual install paths were drift waiting to happen.Deleted:
setup.sh(87 lines): symlinked skills/agents into~/.claudeand the bash forge launcher into~/.local/bin/forge. Both jobs are now done bynpx @firatcand/forge install.forge(168 lines): bash CLI withforge init,forge templates list,forge templates copy,forge upgrade,forge version. All replaced bybin/forge.jssubcommands shipped via npm in v0.2.x.Updated:
README.md: drop the "Prefer the original bash flow?" fallback line below the install section.CONTRIBUTING.md: bug-fix scope now points atbin/forge.js+lib/tools.js; local validation block updated frombash -n setup.sh forge lib/*.shtonode --check bin/forge.js lib/*.js(lib's.shhelpers stay and still getbash -n'd).Untouched on purpose:
lib/*.sh(github-helpers, linear-helpers, validators, worktree-helpers) — these are sourced from skill bodies and templates, not from the deleted bash launcher. Out of scope for this PR.CHANGELOG.mdv1.0.0 entry mentioningsetup.sh— historical record, immutable.0.2.1. No published-API change:setup.sh/forgewere never inpackage.json:files, so the npm tarball is byte-identical.Why now
The parent investigation that produced PR #1 (
forge doctor) traced its root cause back to silent drift between forge's stated cross-tool support and what actually worked. Carrying two install paths — npm-shipped and bash-shipped — multiplies that same risk. One canonical path is cheaper to reason about and harder to lie about in docs.Test plan
node --check bin/forge.js lib/*.jspassesbash -n lib/*.shpassesnode bin/forge.js --version→0.2.1node bin/forge.js doctorruns cleanly./setup.shor the bash./forge(will spot-check once landed)🤖 Generated with Claude Code