Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
# Decision: the two atomicity primitives (exclusive create, append) use `node:fs` directly, in leaf modules tested against a real temporary directory

## Date

2026-08-30

## Status

Active

## Category

Convention Adoption

## Context

Story #217's dispatch needs two filesystem operations whose whole value is **atomicity**:

- an **exclusive create** — the per-card lock that guarantees a trigger burst never starts two runs on one card;
- an **append** — the audit trail, whose lines must survive two dispatches writing the same file concurrently.

The project's convention is dependency injection through `FileSystemService`, with an `InMemoryFileSystemService` double instead of mocks. That service exposes neither primitive: `mkdirSync` is modelled in the double as "add the path to a set" (it cannot fail a second create at all), and the only write is `writeFile`, a full overwrite — so an append would have to be read-concat-write, which reintroduces exactly the lost update `O_APPEND` exists to prevent.

Widening `FileSystemService` was the obvious alternative, and it is the one worth stating why we did not take.

## Decision

`card-lock.ts` and `dispatch-audit.ts` call `node:fs` **directly** (`mkdirSync` without `recursive`, `appendFileSync`), and are:

- **leaf modules** — nothing else in the dispatch path touches the filesystem, so the untestable surface is two small files rather than a layer;
- **injected at the call site** — the handler takes a `LockAcquirer` and an `AuditAppender`, so every other test in the run pipeline stays hermetic and none of them touches a real working area;
- **tested against a real temporary directory** (`mkdtempSync`), because the properties under test — a second create fails, two appends both survive — are properties of the real filesystem and of nothing else. There is precedent in this repo: `path-containment.test.ts` tests symlink containment the same way, for the same reason.

The rule generalises: **when the behaviour under test IS an atomicity or containment guarantee of the operating system, test it against the operating system.** A double that cannot fail the way production fails proves nothing, and asserting against it is worse than not asserting — it reads like coverage.

## Alternatives Considered

- **Add `mkdirExclusive`/`appendFile` to `FileSystemService`**: correct in principle, but it widens a package shared by every other story in flight for two callers, and the in-memory double would still have to *simulate* the failure mode — so the double's fidelity, not the filesystem's behaviour, is what the tests would end up asserting. Reconsider when a third caller appears.
- **Read-concat-write the audit through `writeFile`**: loses records when two dispatches on different cards write the same audit file; the per-card lock does not protect a shared file.
- **Lock with `existsSync` + `mkdirSync`**: a check-then-act window, which is the exact race the lock exists to close.

## Consequences

- Two modules in `apps/pair-cli/src/commands/run/` bypass `FileSystemService`, each carrying a comment saying why and pointing here.
- Their tests are slower than the rest of the suite (real I/O in `os.tmpdir()`), and clean up after themselves.
- Handler-level tests inject fakes for both, so the dispatch pipeline remains testable in memory.
- A future third caller for either primitive is the trigger to revisit and put it on `FileSystemService` properly.

## Adoption Impact

- `adoption/tech/way-of-working.md` — Quality Gates section: records the exception to the "avoid mocks, use the in-memory double" convention for OS atomicity/containment guarantees.
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
# Decision: an empty `--card-tags` means "this card carries no labels", not a malformed flag

## Date

2026-08-30

## Status

Active

## Category

Convention Adoption

## Context

`pair-cli run` refuses every flag passed with an empty value — `--root ""`, `--filter ""`, `--skill ""` all fail at parse time, deliberately: a flag named with nothing behind it is a caller bug, and accepting it silently is how an unattended run ends up doing something nobody asked for.

Story #217's `--card-tags` inherited that rule, and the end-to-end test on a populated board (T5) showed it was the wrong rule for this one flag. The dispatch entry point is called by a **host trigger**, and the reference GitHub adapter renders the labels it observed as `join(github.event.issue.labels.*.name, ',')`. On an issue with **no labels** that expression renders `""`. So the very state AC2 is about — "an issue with no mapped tag runs nothing" — arrived at the parser as an empty value and was rejected with `--card-tags was passed with an empty value`, exit 1.

Two consequences, both bad, and neither visible from inside the module suites (they pass tag lists, not the empty string a host renders):

- the opt-in boundary of the whole feature — untagged ⇒ skipped, reported, exit 0 — became **unreachable through the entry point**;
- the commonest card on any board turned every trigger firing on it into a **failed CI job**, which is the noise that gets a trigger disabled.

## Decision

For `--card-tags`, and only for it, an **empty or whitespace-only value is data**: it is read as the observation "the trigger saw no labels on this card", producing an empty tag list. The dispatcher then does what it does for any card with no mapped tag — skips it, reports the reason, appends the skip to the audit trail, exits `0`.

A **hole inside a list** stays an error: `auto-dev,,risk:green` still HALTs. The two cases are genuinely different. An empty value is a complete observation of an empty set; a hole is an incomplete rendering of a non-empty one — the caller built a list and lost an item, which is exactly the string-interpolation bug worth failing on.

The general rule this instantiates: **a flag that carries an observation from an external system is empty-valid when the empty case is a real state of that system; a flag that carries an operator's intent is not.** `--root`, `--filter` and `--skill` are intent — nobody means "" by them. `--card-tags` is an observation, and "no labels" is a state of every board.

## Alternatives Considered

- **Keep the refusal, make the adapter skip the call when the label list is empty**: pushes an authorization-relevant decision — "should this card run?" — into every per-host adapter, where it is untested, duplicated per host, and free to drift. ADR-024 puts that decision in the routing core precisely so no adapter can widen or narrow it.
- **Keep the refusal, have the adapter pass a sentinel** (`--card-tags "(none)"`): invents a label that could collide with a real one and makes the trail lie about what the trigger saw.
- **Accept empty values on every flag**: loses the guard where it earns its keep — an empty `--root` or `--skill` is a caller bug with no legitimate reading.

## Consequences

- `apps/pair-cli/src/commands/run/parser.ts` reads an empty/whitespace `--card-tags` as an empty tag list; the empty-entry HALT for a hole inside a list is unchanged.
- An unlabelled card now produces the documented skip and exit `0` end-to-end, so a host adapter needs no pre-filter and no conditional call.
- The asymmetry between this flag and its neighbours is deliberate and must stay documented where a reader meets it: the parser module, the CLI reference, and the reference adapter in the KB.

## Adoption Impact

- `adoption/tech/way-of-working.md` — CLI conventions: records that flags carrying an external observation are empty-valid when the empty case is a real state of the observed system, while flags carrying operator intent are not.
Loading
Loading