feat!: read-only GitHub Action, external state, and a product-quality pass - #8
Conversation
… pass
Gitgotchi promised to be an ambient observer but wrote to the repo it was
watching. This makes that promise true, then builds the GitHub Action that
becomes possible once it is.
BREAKING CHANGE: pet state no longer lives in `.gitgotchi/state.json`.
State moves outside the working tree
- `GITGOTCHI_STATE_DIR`, else `$XDG_STATE_HOME/gitgotchi/`, else
`~/.local/state/gitgotchi/` (`%LOCALAPPDATA%\gitgotchi\` on Windows).
- Per-repo slot is `<basename>-<sha256(abs path)[0:12]>`, so two checkouts
named `api` never collide.
- `ensureGitignore` is gone. A check-in never touches the user's `.gitignore`.
- Pre-0.2 pets are read once for migration and never written back.
CLI fixes
- `card` called `load()` and rendered stale state; it now performs a real
check-in.
- `messages.forEach(io.out)` passed the index and array straight into
`console.log`, printing `Saved card.svg 0 [ 'Saved card.svg' ]`.
- `oneShot`/`watch` bypassed the injected `Io` and wrote to `console.log`,
which is why no CLI integration tests existed. They are threaded now.
- `-o` and `-i` with a missing or non-numeric value silently defaulted; each
gets one friendly line instead.
One run, one check-in
- `--report <path>` and `--card <path>` render extra artifacts from the same
tick, so a script can have the numbers, the picture and the human block
without ageing the pet three times.
- `card -o -` writes the SVG to stdout.
Input handling
- `safeName`/`stripControl` scrub control bytes at the persistence boundary
and again at the SVG boundary, so a pet name cannot carry an escape
sequence into a terminal, a shared card, or a `$GITHUB_OUTPUT` line.
The card
- Seeded by the pet's condition instead of the last check-in timestamp, so an
unchanged pet renders byte-identical output and the commit workflow stays
quiet.
- Carries its own attribution and `npx gitgotchi` install line.
The Action
- Composite, read-only. Job summary by default, `actions/cache` for
persistence between runs, nine outputs.
- Inputs reach bash through `env:`, never `${{ }}` interpolation inside
`run:`. Card paths are rejected if absolute or containing `..`.
CI and release
- Windows added to the test matrix; git fixtures get the env it needs there.
- A `package` job packs the tarball, installs it into a scratch project and
drives the real bin.
- Action smoke test on Linux, macOS and Windows.
- Tag-triggered release with npm provenance.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @dvd90's task in 2m 10s —— View job Reviewing PR #8
ReviewThis is a well-scoped milestone PR and it holds up against CLAUDE.md's rules:
One real bug, low severity:
Fix would be tracking which path was actually read and backing up from there, e.g.: const source = /* file or legacyFile, whichever readFile succeeded on */;
await backup(source, bak);Minor nits (non-blocking):
I wasn't able to run Overall: solid, principle-restoring change. One small, low-severity backup-path bug worth a quick fix + test; everything else checks out against CLAUDE.md. |
Both caught by the new CI jobs on their first run. `prepublishOnly` runs on `npm publish` but not on `npm pack`, so the packed tarball contained no `dist/` and its `bin` pointed at a file that was not there. `prepack` runs on both. Windows runners check out with `core.autocrlf=true`, which rewrites every file to CRLF and makes `prettier --check` fail on all 73 of them. `.gitattributes` pins the checkout to LF, leaving the width-sensitive sprite files untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @dvd90's task in 1m 9s —— View job Reviewing PR #8 (re-review after new commit)
What changed since the last reviewThe new commit (
Both are sensible, minimal fixes for problems the new CI matrix caught on its own first run, and neither needs its own test (they're packaging/checkout config, not behavior). This doesn't change the substance of the earlier review — the rest of the 1086-line diff is identical to what was already reviewed. Still holds from the previous pass
Still-open issue: legacy-file backup path (unchanged)The bug flagged in the previous review is still present in try {
raw = await readFile(file, 'utf8');
} catch {
try {
raw = await readFile(legacyFile, 'utf8'); // source may be legacyFile
} catch {
return newbornState(key, clock);
}
}
...
} catch {
await backup(file, bak); // always backs up `file`, even when `legacyFile` was the one actually read
return newbornState(key, clock);
}When the pre-0.2 Minimal fix: track which path was actually read and pass that into let raw: string;
let source = file;
try {
raw = await readFile(file, 'utf8');
} catch {
source = legacyFile;
try {
raw = await readFile(legacyFile, 'utf8');
} catch {
return newbornState(key, clock);
}
}
...
await backup(source, bak);plus a test seeding a corrupt/invalid Minor nits (non-blocking, unchanged)
I don't have Overall: still a solid, principle-restoring change with one small, low-severity, easy-to-fix backup-path bug carried over from the previous review — not addressed by the latest commit, which only fixed unrelated CI/build issues. |
Both are test-side; the source was already correct on Windows.
`commands.test.ts` derived the expected folder name with
`join(repo).split('/').pop()`, which returns the whole path when the separator
is a backslash. `initPet` uses `basename()`, so the test now does too.
The first CLI integration test timed out at the 5s default. These tests drive
real git in real fixture repos, which takes seconds on a Windows runner.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @dvd90's task in 46s —— View job Reviewing PR #8 (re-review after synchronize)
What changed since the last reviewThe new commit (
Both are minimal, correctly-scoped fixes for problems the new Windows CI matrix (added in this same PR) caught. No product code touched, so this doesn't change the substance of the prior two reviews. Still holds from previous passes
Still-open issue: legacy-file backup path (unaddressed, carried over)Confirmed still present in try {
raw = await readFile(file, 'utf8');
} catch {
try {
raw = await readFile(legacyFile, 'utf8'); // source may be legacyFile
} catch {
return newbornState(key, clock);
}
}
...
} catch {
await backup(file, bak); // always backs up `file`, even when `legacyFile` was actually read
return newbornState(key, clock);
}When the pre-0.2 Minimal fix: track which path was actually read and back up from there: let raw: string;
let source = file;
try {
raw = await readFile(file, 'utf8');
} catch {
source = legacyFile;
try {
raw = await readFile(legacyFile, 'utf8');
} catch {
return newbornState(key, clock);
}
}
...
await backup(source, bak);plus a test seeding a corrupt/invalid Minor nits (non-blocking, unchanged)
I don't have Overall: still a solid, principle-restoring change. This synchronize only fixed unrelated Windows CI flakiness — the one low-severity backup-path bug from earlier reviews remains open. |
`main()` imports its command modules on demand, so whichever integration test ran first absorbed the cost of transforming the whole Ink/React tree. On a cold Windows runner that was over 20s for a call whose siblings take ~1.2s. Paying it in `beforeAll` puts the first test back in line with the rest (591ms local). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @dvd90's task in 40s —— View job Reviewing PR #8 (re-review after synchronize)
What changed since the last reviewThe new commit (
Sensible, minimal fix for CI flakiness the new Windows matrix surfaced. Doesn't touch Still holds from previous passes
Still-open issue: legacy-file backup path (unaddressed across three reviews)Confirmed still present in try {
raw = await readFile(file, 'utf8');
} catch {
try {
raw = await readFile(legacyFile, 'utf8'); // source may be legacyFile
} catch {
return newbornState(key, clock);
}
}
...
} catch {
await backup(file, bak); // always backs up `file`, even when `legacyFile` was actually read
return newbornState(key, clock);
}When the pre-0.2 Minimal fix: track which path was actually read and back up from there: let raw: string;
let source = file;
try {
raw = await readFile(file, 'utf8');
} catch {
source = legacyFile;
try {
raw = await readFile(legacyFile, 'utf8');
} catch {
return newbornState(key, clock);
}
}
...
await backup(source, bak);plus a test seeding a corrupt/invalid Minor nits (non-blocking, unchanged)
I don't have Overall: still a solid, principle-restoring change. This synchronize only fixed unrelated Windows CI timing — the one low-severity backup-path bug from earlier reviews remains open and untouched. |
CI follow-upsThe three new jobs caught four real problems on their first run. All fixed; all 11 checks now pass.
Windows checked out CRLF. Runners default to A latent test bug. A slow first test, not a slow suite. One integration test took over 20s on Windows while its siblings took ~1.2s. |
1 participant
Gitgotchi promised to be an ambient observer but wrote to the repo it was watching. This PR makes that promise true, then builds the GitHub Action that becomes possible once it is.
Breaking: pet state no longer lives in
.gitgotchi/state.json. Existing pets migrate automatically on the next check-in.161 tests (was 131).
npm test && npm run typecheck && npm run lintall pass.1. State moved out of the repo
save()used to write.gitgotchi/state.jsonand append to the user's.gitignore. That is a surprise write to a tracked file, and it makes the tool unusable in a read-only CI context.State now resolves to
GITGOTCHI_STATE_DIR→$XDG_STATE_HOME/gitgotchi/→~/.local/state/gitgotchi/(%LOCALAPPDATA%\gitgotchi\on Windows), in a per-repo slot named<basename>-<sha256(abs path)[0:12]>so two checkouts namedapinever collide.ensureGitignoreis deleted.A pre-0.2
.gitgotchi/state.jsonis read once for migration and never written back; the old file is left alone and can be deleted.hasState()checks both locations, so an upgraded pet does not get greeted as a fresh egg.test/setup.tspoints every test file at its own tmp state root — without it the suite would write into the developer's real state directory.2. CLI bugs
Four real ones, each now covered by a test:
cardrendered stale state. It calledload()instead ofcollectAndAdvance(), so on a fresh clone you got a day-0 egg at health 70 no matter what the repo looked like.messages.forEach(io.out)passed the index and array intoconsole.log, printingSaved card.svg 0 [ 'Saved card.svg' ]. Found while regenerating the README image.oneShot/watchbypassed the injectedIoand wrote toconsole.log. This is why the project had no CLI integration tests — nothing was capturable.main()also takesrepoPathnow, so tests can drive it against a fixture repo.card -owrote to the default path;watch -iandwatch -i soonfell back to 60s. Each gets one friendly line instead.New
test/cli.integration.test.tsdrivesmain()end to end against real git fixtures, including an assertion that the working tree stays clean.3. One run, one check-in
--report <path>and--card <path>are extra renderings of a single tick, not extra ticks:npx gitgotchi --report pet.json --card pet.svg npx gitgotchi card -o - # SVG to stdoutThis is what lets the Action produce a summary, outputs and a card from one check-in. Without it the Action would have aged the pet three times per run.
4. Input and path handling
safeName/stripControl(src/state/name.ts) drop control bytes, collapse whitespace and cap at 32 chars. Applied at the zod boundary — so a hand-editedstate.jsonis covered too — and again in the card'sesc().A pet name is typed by the user, read back from disk, rendered into a terminal, embedded in a shared SVG, and written to
$GITHUB_OUTPUT. A newline alone would corrupt that last one.The Action rejects card paths that are absolute or contain
.., and passes every input throughenv:rather than${{ }}interpolation insiderun:.5. The card
Attribution: an
npx gitgotchiinstall line bottom-left,<repo> · github.com/dvd90/gitgotchibottom-right. A card gets shared away from its repo, so it carries its own provenance.Determinism, for real. The flavor line was seeded from the last check-in timestamp, so every run produced different bytes — which would have made the commit-the-card workflow churn on every push.
cardSeed()now seeds from the pet's condition (stage, mood, four vitals), so an unchanged pet renders byte-identical output.The day counter still ticks, so the README says "roughly one commit a day", not "no noise".
6. The Action
Composite, read-only. Default behaviour is the job summary — no commits, no artifacts, no extra token scope, and the pet shows up on the run page:
actions/cachekeeps the pet alive between runs so it actually grows up. Inputs:path,version(localbuilds the checked-out copy),cache,summary,card,github-token. Outputs:name,species,stage,mood, the four vitals,report,card-path.README documents both workflows you asked for — artifact-only via
upload-artifact, and the opt-in SVG commit with itscontents: writescope spelled out.7. CI, cross-platform, release readiness
test/helpers/repo.tsnow passesSystemRoot/TEMP/ComSpecthrough, which git needs to start there. The collectors were already Windows-safe —collectTodosusesgit ls-filesand Node, notgrep.packagejob packs the tarball, installs it into a scratch project, drives the realnode_modules/.bin/gitgotchi, and asserts the repo stays clean. Verified locally.release.ymlonv*tags: full check, tag-matches-package.jsonguard,npm publish --provenance.0.2.0.Notes
npm init -y --prefix <dir>writes to the cwd'spackage.json, not the prefix. It polluted the rootpackage.jsonwhile I was testing the pack flow; reverted, and the CI script subshells into the scratch dir instead.This is a single commit rather than seven.
src/cli.tsandsrc/ui/launch.tsxare touched by nearly every slice, so splitting would have produced intermediate commits that don't pass tests — which CLAUDE.md forbids. Happy to restructure into stacked PRs if you'd rather review it in pieces.docs/gitgotchi-card.svgwas updated by hand to add the footer, keeping its healthy stat values — regenerating it from this repo would have baked in a critical pet with zeroed vitals.🤖 Generated with Claude Code