Skip to content
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
d799d0a
Add workflow option to stats command
Toby-Masters-SF Feb 15, 2026
1f00cb4
[241] Create new getWorkflowTemplateSummary function
Toby-Masters-SF Feb 15, 2026
dd2cfd3
Get template lists from the workflow
Toby-Masters-SF Mar 18, 2026
13bc804
Add filter for files in countYamlFiles and complete assignments
Toby-Masters-SF Mar 18, 2026
175f7c9
[241] Update yamlFilesActivity function for workflows
Toby-Masters-SF Mar 31, 2026
5a2bd92
[241] Create displayWorkflowOverview function
Toby-Masters-SF Mar 31, 2026
c785f4b
fixup! [241] Update yamlFilesActivity function for workflows
Toby-Masters-SF Mar 31, 2026
c6942a6
[241] Create a createWorkflowRow function
Toby-Masters-SF Mar 31, 2026
b0fe77e
[241] Create a saveWorkflowOverviewToFile function
Toby-Masters-SF Mar 31, 2026
ed90203
[241] Split generateOverview functions
Toby-Masters-SF Mar 31, 2026
e1853da
Add review all workflows functionality
Toby-Masters-SF Mar 31, 2026
84ae281
Add sample workflows for tests
Toby-Masters-SF Jul 29, 2026
7a4d8fe
Add date and workflow input checks to CLI command
Toby-Masters-SF Jul 29, 2026
fd2a70e
Add guidance in README.md to stats and workflow folder?
Toby-Masters-SF Jul 29, 2026
fa5b3c8
Add initial Claude harness
Toby-Masters-SF Aug 11, 2026
5f9bffb
Add guidance to error handling
Toby-Masters-SF Aug 11, 2026
0321fa1
Update AI harness to better handle skill writing
Toby-Masters-SF Aug 11, 2026
ff09d08
Update version number
Toby-Masters-SF Aug 11, 2026
1817f89
Update error catching
Toby-Masters-SF Aug 11, 2026
20917e6
Handle CSV write failures in stats
Toby-Masters-SF Aug 18, 2026
c8e980c
Record where a private helper belongs in a file
Toby-Masters-SF Aug 18, 2026
86a0e50
Count templates instead of yaml files in stats
Toby-Masters-SF Aug 19, 2026
9382d21
Skip workflow templates missing from the repository
Toby-Masters-SF Aug 19, 2026
b2713dd
Document stats workflow counting
Toby-Masters-SF Aug 19, 2026
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions .claude/hooks/skill-reminder.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
#!/usr/bin/env bash
# PreToolUse hook: maps the file about to be written to the skill that governs it,
# and injects a reminder into the model's context. Silent for unmapped paths.
set -euo pipefail

input=$(cat)
file=$(printf '%s' "$input" | jq -r '.tool_input.file_path // empty')
[ -z "$file" ] && exit 0

case "$file" in
*/node_modules/*) exit 0 ;;
esac

case "$file" in
*/tests/*) skill="writing-tests" ;;
*/package.json|*/package-lock.json|*/CHANGELOG.md)
skill="bumping-cli-version" ;;
*/lib/*|*/bin/*) skill="adding-methods" ;;
*) exit 0 ;;
esac

jq -n --arg s "$skill" '{
hookSpecificOutput: {
hookEventName: "PreToolUse",
additionalContext: ("This file is governed by the \"" + $s +
"\" skill. If you have not already read .claude/skills/" + $s +
"/SKILL.md this session, read it now and follow it for this edit.")
}
}'
16 changes: 16 additions & 0 deletions .claude/settings.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
"hooks": {
"PreToolUse": [
{
"matcher": "Write|Edit",
"hooks": [
{
"type": "command",
"command": "\"$CLAUDE_PROJECT_DIR/.claude/hooks/skill-reminder.sh\" 2>/dev/null || true",
"timeout": 10
}
]
}
]
}
}
77 changes: 77 additions & 0 deletions .claude/skills/adding-methods/SKILL.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
---
name: adding-methods
description: Use when adding a new function, method, or class to this repo, or when editing an existing one so that its behaviour, signature, or responsibilities change - including "add a helper", "extract this", "make X also do Y", and new CLI commands or API endpoints.
---

# Adding or changing a method

Every method in this repo has one home and one job. Before you write it, decide the home; while you write it, hold the job to one.

## Step 1 — Read the map

Read [docs/ARCHITECTURE.md](../../../docs/ARCHITECTURE.md) before writing the method. It states what each file is for, which layer a method belongs in, and the naming conventions. Do not rely on the file you happen to have open.

## Step 2 — Place it

Name the method, then pick its home from its name and its dependencies:

| The method... | Lives in |
|---|---|
| touches `fs` (read, write, scan, exists) | `lib/utils/fsUtils.js` |
| makes an HTTP request to Silverfin | `lib/api/sfApi.js` |
| knows a template type's config keys or folder layout | the matching `lib/templates/*.js` class |
| produces a user-facing error message | `lib/utils/errorUtils.js` |
| validates a CLI option or prompts the user | `lib/cli/utils.js` |
| sequences an API call plus a disk write | `index.js` |
| parses or transforms data with no I/O | the relevant `lib/utils/*.js` |

Two checks before you commit to a location:

- **Does it already exist?** Grep `lib/utils/` and the layer you picked. A near-duplicate helper is a call, not a new method.
- **Would it skip a layer?** `bin/cli.js` must not call `lib/api/`; `lib/api/` must not touch `fs`. If your method forces a skip, it is in the wrong place.

If the method fits nowhere in the table, say so and propose where it should go before writing it. A new home is a decision for the user, not a default.

## Step 3 — One responsibility

The method does one thing, at one level of abstraction, for one reason to change.

Three tests it must pass:

1. **The name test.** You can name it without "and", "then", "Or", or a vague noun (`handle`, `process`, `manage`, `doStuff`). If the honest name needs "and", it is two methods.
2. **The layer test.** It does not both decide and perform I/O. Deciding *which* template to fetch and *fetching* it are separate methods.
3. **The reason test.** You can state one change to the product that would require editing it. Two unrelated reasons means split it.

When you split, the caller keeps the sequencing and each new method keeps one step.

A split usually produces a **private helper**: a small unexported function whose only caller is the file it came out of. Leave it there, next to that caller. The Step 2 table places methods other files will reach for; it does not evict a helper from the only file that uses it. Splitting for one responsibility and keeping the pieces together is not a failed split.

Move a helper out only when one of these is true:

- a second file needs it — then it goes to the home its own row gives it, and gets exported
- it is generic (it knows nothing about the subject of the file it sits in) **and** you can name the other caller that wants it. "Someone might" is not a caller
- the public function's tests cannot reach one of its branches. That means it is a unit in its own right: move it, export it, and test it directly

"It has no test of its own" is not a reason to move it. A private helper is tested through the function that calls it.

Applies equally to edits: if you are asked to make an existing method "also" do something, the answer is a second method plus a caller, not a longer method. Say that in your response rather than silently growing the function.

## Step 4 — Report

State, in one line each:

- where you put it and which row of the table put it there
- the one responsibility it has
- anything you split out, and where that went — for a private helper you kept in the same file, say so and say why it stayed

Then add the test in the mirrored `tests/` path, and run `npx jest <path>` before claiming it works.

## Red flags

- "I'll just add it to the file I'm already in" — said about a method other files will call
- Exporting a private helper only so a test can reach it
- "It's only a few lines, no need to check the doc"
- A new method with `fs` and `axios` both in scope
- A parameter named `options` that switches behaviour between two unrelated jobs
- A boolean parameter that selects which of two things the method does — that is two methods
- Editing an exported method's signature without checking its callers
62 changes: 62 additions & 0 deletions .claude/skills/bumping-cli-version/SKILL.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
---
name: bumping-cli-version
description: Use when preparing a pull request, committing a user-facing change, releasing, or bumping the version - and whenever package.json, package-lock.json or CHANGELOG.md is about to be edited. Also use when the "CLI version check" GitHub action fails.
---

# Bumping the CLI version

Every PR to `main` runs [.github/workflows/cli_version.yml](../../../.github/workflows/cli_version.yml), which fails unless the version was bumped correctly or the bump was explicitly skipped. There is no partial credit — miss one of the three and CI is red.

## Does this change need a bump?

Bump when the change affects what a user of the installed CLI gets: a command, its output, a fix, a dependency update.

Skip only for changes with no effect on the published package — docs, tests, CI config, comments. To skip, the PR body must contain the checkbox ticked exactly:

```
- [x] Skip bumping the CLI version
```

That string is matched literally against the PR body. Ticking it in a commit message or a comment does nothing.

## The three edits

All three, or CI fails.

1. **`package.json`** — `version` strictly greater than the version on `main`. Equal is a failure, not a pass.
2. **`package-lock.json`** — the top-level `version` set to the *same* string. The workflow compares them directly. `npm version` normally handles both; if the lockfile is stale, edit it rather than reinstalling — see the environment note in [docs/DEVELOPMENT.md](../../../docs/DEVELOPMENT.md).
3. **`CHANGELOG.md`** — a new entry at the top of the list whose heading contains the literal `## [<version>]`.

Verify before pushing:

```bash
jq -r .version package.json package-lock.json # must print the same string twice
grep -F "## [$(jq -r .version package.json)]" CHANGELOG.md
git fetch origin main && git show origin/main:package.json | jq -r .version # must be lower
```

## Changelog entry format

Match the existing entries — heading, then one line of plain description, no bullet:

```markdown
## [1.56.2] (11/08/2026)
Add workflow statistics to the stats command.
```

Date is `DD/MM/YYYY`. Write what changed for the user, not what changed in the code.

`lib/cli/changelogReader.js` parses this file at runtime to show users what changed when they update, so the heading format is load-bearing. Do not reformat old entries, add sub-bullets under a version, or introduce `### ` levels.

## Which number

- **Patch** (`1.56.1` → `1.56.2`) — bug fix, dependency bump, no interface change.
- **Minor** (`1.56.1` → `1.57.0`) — new command, new option, new capability.
- **Major** — a breaking change to an existing command or its output. Ask before taking one; it is a release decision, not a code decision.

## Red flags

- Bumping `package.json` alone and assuming `npm install` will fix the lockfile — it fails with `EACCES` in this checkout
- A changelog entry written from the diff ("refactored `fsUtils`") rather than from the user's view
- Reformatting or re-dating existing changelog entries
- Assuming the skip checkbox applies because the change "feels internal" — if it ships in the package, it needs a bump
115 changes: 115 additions & 0 deletions .claude/skills/handling-errors/SKILL.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
---
name: handling-errors
description: Use when writing or changing a catch block, adding a process.exit, reporting a new failure mode, or handling ENOENT / EACCES / a 4xx API response in this repo - including "the CLI crashed with a stack trace", "it printed the whole error object", "it exited halfway through the batch", and errors that are silently swallowed.
---

# Handling errors in the CLI

Every failure this CLI reports is one of two things, and the user can tell them apart by what they see:

| Kind | Cause | What the user sees | Who prints it |
|---|---|---|---|
| **Expected failure** | Something in their repo, command, or firm is wrong: missing file, missing ID, bad date, 404, 403 | One sentence naming the thing, plus the next command to run | a named function in `lib/utils/errorUtils.js` |
| **Bug** | Our code is wrong: `TypeError`, `ReferenceError`, anything unrecognised | Stack trace, versions, and the "open an issue" banner | `errorUtils.uncaughtErrors` only |

**A stack trace is a bug report, not an error message.** Printing one for a missing file tells the user to open an issue about their own typo. Printing a bare sentence for a `TypeError` throws away the only evidence we would have had.

## Step 1 — Classify before you write the catch

Ask: can the user fix this without changing our code?

- **Yes** → expected failure. It needs a message in `errorUtils.js` and a next step.
- **No** → bug. Hand it to `errorUtils.errorHandler(error)` and write nothing else. `errorHandler` recognises `ENOENT` and routes everything else to `uncaughtErrors`.
- **Don't know yet** → recognise the cases you know (`error.code`, `error.response.status`) and let the rest fall through to `errorHandler`. Never let "don't know" become a generic message.

## Step 2 — Write the catch block

A catch block in this repo has four parts, in this order. Anything missing is a defect:

1. **Recognise** — branch on `error.code` (`ENOENT`, `EACCES`) or `error.response.status` (400, 403, 404, 422). One branch per case you can name.
2. **Report** — call a named function from `lib/utils/errorUtils.js`. New failure mode means a new function there, not an inline `consola.error` string. Messages go through `consola`, never `console.log`; suggested commands get `chalk.bold`.
3. **Keep the cause** — the raw error goes to `consola.debug(error)` so `-v` still shows it. Recognising an error is not a reason to discard it.
4. **Decide the exit** — see Step 3. Falling off the end of a catch block is a decision too, and usually the wrong one.

```javascript
// lib/utils/errorUtils.js — the message and the next step live here
function missingWorkflowConfig(handle) {
consola.error(`Workflow ${handle}: config.json was not found in the workflows folder`);
consola.log(`Try running: ${chalk.bold(`silverfin import-workflow --handle ${handle}`)}`);
return false;
}

// the caller recognises, reports, keeps the cause, and decides
try {
return await sfApi.readWorkflow(envId, handle);
} catch (error) {
consola.debug(error);
if (error.code === "ENOENT") {
return errorUtils.missingWorkflowConfig(handle); // false — caller decides what next
}
errorUtils.errorHandler(error); // unrecognised: stack trace + issue URL
}
```

## Step 3 — Only the boundary exits

`process.exit` is a statement about the whole run, so only code that owns the whole run may call it.

| Layer | On failure |
|---|---|
`bin/cli.js`, `lib/cli/utils.js` | validate input up front and `process.exit(1)` — nothing has happened yet, so stopping is free
`index.js` | report through `errorUtils`, then exit or return depending on whether more work remains
`lib/utils/`, `lib/api/`, `lib/templates/` | `return` a falsy value or `throw`. **Never exit.** These modules do not know whether they are one step of a 200-template loop

`lib/utils/*` and `lib/api/*` do contain legacy `process.exit(1)` calls. They are the pattern to move away from, not the one to copy: an exit inside a helper cannot be tested without mocking `process.exit`, skips the spinner teardown, and kills a batch that had 199 templates left.

## Step 4 — In a loop, defer

A failure on template 3 of 200 must not end the run. Follow the deferred pattern already in `index.js` `publishAll*`: push a tagged object, keep going, summarise at the end.

```javascript
deferredErrors.push({ kind: "exception", handle, message, stack: error.stack });
// after the loop
errorUtils.printReconciliationBatchErrorSummary(deferredErrors);
```

`kind` is the repo's stand-in for custom error classes: `"missing_id"`, `"update_failed"`, `"exception"`. Reuse those three. A new kind means teaching the matching `print<Type>BatchErrorSummary` to print it — a kind nothing prints is a swallowed error.

## Step 5 — Touching existing code: raise the legacy pattern, don't silently fix it

Much of the existing error handling predates this skill. When your change lands in or next to code that matches one of the seven patterns in [legacy-patterns.md](legacy-patterns.md), read that file and name the match in your response: the pattern, the location, what it costs the user, the smallest fix, and whether you should do it now. Then let the dev choose.

Fix it in the same change only when it sits inside the block you were already editing and the fix is a few lines. Wider than that is a separate branch — say so rather than growing the diff.

Two failure modes here, both wrong:

- **Silently rewriting** surrounding error handling because it offends this skill. The diff stops being reviewable.
- **Silently copying** it because it is the local convention. New code follows Steps 1–4 even when its neighbours don't.

## Step 6 — Global handlers are already installed

`bin/cli.js` calls `cliUtils.handleUncaughtErrors()`, which wires `uncaughtException` and `unhandledRejection` to `uncaughtErrors`. Do not add `process.on` handlers elsewhere, and do not rely on them as your error handling: a promise that reaches them prints "open an issue" for what may be an ordinary missing file.

## Never

- **An empty catch.** `catch { continue }` with no message hides a corrupt YAML file as "not found". Log at `consola.debug` at minimum, and say what was skipped.
- **`consola.error(error)` as the whole handler.** It prints an object or a stack where a sentence belongs, and `process.exit(1)` after it means the user gets a crash for a typo.
- **A message that omits the identifier.** Every message names the handle, name, or path it is about — the user has 200 templates.
- **Discarding the cause.** `catch (err) { consola.error("The URL provided is not correct"); }` loses the one fact that would explain why.
- **`try`/`catch` as flow control.** If you are catching to test whether a file exists, call `configExists` instead.
- **Exposing raw axios errors.** They carry request URLs, params, and tokens. `apiUtils.responseErrorHandler` is where status codes get turned into messages; extend it rather than printing `error.response` at a call site.

## Red flags

- "I'll just `consola.error(error)` and exit here" — that is the pattern this skill exists to stop
- A `process.exit` in a file under `lib/utils/` or `lib/api/`
- A new error string typed directly into `index.js` or `bin/cli.js`
- A catch block whose `error` parameter is never read
- A batch loop with `process.exit(1)` inside it
- `errorHandler` called for a failure you could name — it will tell the user to open an issue
- "The file already does it this way, so I'll match it" — Step 5, not a licence
- A diff that quietly rewrites error handling the task never asked about

## Then test it

Error paths are the paths users hit. Add the case in the mirrored `tests/` path and run `npx jest <path>` before claiming it works. Use the **writing-tests** skill for how to mock `consola`, `sfApi`, and `process.exit` in this repo.
Loading