test: the gh simulator's api, faults, and one mock for lib.test.sh (#88)

.changeset/gh-simulator-api-faults.md

@@ -0,0 +1,5 @@
+---
+"github-agent-workflow": patch
+---
+
+The `gh` simulator for tests is finished. `gh api` answers the REST calls the package makes (a user, an item's comments, editing a comment in place, a PR's files, repository contents and tree, the PR list and one PR, and `update-branch`), a world's `faults` make a matching call fail or let another run get in first, and `tests/lib.test.sh` now runs against the simulator instead of its own stub, so there is one mock of `gh`. The checker's effect model gains `comment_edit`, so a lease edited in place is read back by `diffWorlds`.

AGENTS.md

@@ -38,7 +38,7 @@ skills/<bucket>/<skill>/SKILL.md     exactly three levels deep
 
 **Every result a skill can return has a scenario** under `tests/scenarios/<skill>/`: a world, the inputs, the expected result line and the net effects. `scripts/scenario.mjs` checks each against the skill's contract (scopes, invariants, eligibility oracles), and `npm run scenarios:coverage` lists results with none. A new result line without a scenario fails `npm test`. See `docs/adr/0012-scenarios.md`.
 
-**Every `gh` command a skill uses is listed in `tests/sim/surface.mjs`.** The `gh` simulator (`tests/sim/bin/gh`) answers `gh` from a world file, and a test greps `skills/` and `scripts/` for `gh` calls. A new one fails `npm test` until it is listed as `supported`, `planned` or `out-of-scope`. Use only `--json` fields that `gh` has. See `docs/adr/0014-gh-simulator.md`.
+**Every `gh` command a skill uses is listed in `tests/sim/surface.mjs`.** The `gh` simulator (`tests/sim/bin/gh`) answers `gh` from a world file, and a test greps `skills/` and `scripts/` for `gh` calls. A new one fails `npm test` until it is listed as `supported` (and the simulator answers it) or `out-of-scope`. Use only `--json` fields that `gh` has. `tests/lib.test.sh` runs against the same simulator, so there is one mock of `gh`. See `docs/adr/0014-gh-simulator.md`.
 
 ## Inputs and defaults
 

docs/adr/0012-scenarios.md

@@ -28,7 +28,7 @@ A **scenario** is a world, a skill and its inputs, and the expected outcome: one
 ## Consequences
 
 - A new result line, or a changed write scope, cannot land without a scenario. The old behaviour of the gap-9 bug (a conflicted PR skipped as `no-checks-yet`) is now a failing test.
-- The same checker can judge a real run. `diffWorlds(before, after)` in `scripts/scenario.mjs` turns the change a run left behind into the effects the checker takes, and `applyEffects(before, diffWorlds(before, after))` gives `after` back (tested on every golden scenario and on generated worlds). A change no effect can express, such as a title or a body edit of an item that already existed, comes out as an `unmodelled` effect, which the checker rejects unless the contract lists an `unmodelled` write. What a new item is made with (title, body, creation time, author and, for a PR, branch) is part of creating it, so a `create_issue` or `create_pr` effect carries it as `data`, and a `close` carries its `reason`. The `gh` simulator over a world file is described in `0014-gh-simulator.md`. The part that is not built is a runner that drives a model against it.
+- The same checker can judge a real run. `diffWorlds(before, after)` in `scripts/scenario.mjs` turns the change a run left behind into the effects the checker takes, and `applyEffects(before, diffWorlds(before, after))` gives `after` back (tested on every golden scenario and on generated worlds). A change no effect can express, such as a title or a body edit of an item that already existed, comes out as an `unmodelled` effect, which the checker rejects unless the contract lists an `unmodelled` write. What a new item is made with (title, body, creation time, author and, for a PR, branch) is part of creating it, so a `create_issue` or `create_pr` effect carries it as `data`, a `close` carries its `reason`, and a comment edited in place is a `comment_edit` (its index among the item's comments, and its new text). The `gh` simulator over a world file is described in `0014-gh-simulator.md`. The part that is not built is a runner that drives a model against it.
 
 ## What this does not prove
 

docs/adr/0014-gh-simulator.md

@@ -2,13 +2,13 @@
 
 ## Status
 
-Accepted. The read paths (#86) and the write paths (#87) are built. `api`, fault injection and the replacement of the stub in `tests/lib.test.sh` (#88) follow, and this record is updated with each.
+Accepted and built: the read paths (#86), the write paths (#87), and `api`, fault injection and the replacement of the old stub in `tests/lib.test.sh` (#88).
 
 ## Context
 
 ADR 0012 checks a scenario against a skill's contract, and `diffWorlds` turns the change a run left behind into the effects the checker takes. What was missing is something to run a skill against. A maintainer who wants to see whether a model follows `shepherd-github-pr` has to point it at a real repository, cannot repeat the run, and cannot make a merge fail or a check hang.
 
-`tests/lib.test.sh` has a stub `gh` for the shell helpers. It covers only what `lib.sh` calls, and it ignores which `--json` fields are asked for. That let `prs_for_issue` ask `gh` for a field it does not have (#89).
+`tests/lib.test.sh` had a stub `gh` for the shell helpers. It covered only what `lib.sh` calls, and it ignored which `--json` fields are asked for. That let `prs_for_issue` ask `gh` for a field it does not have (#89).
 
 ## Decision
 
@@ -37,16 +37,49 @@ ADR 0012 checks a scenario against a skill's contract, and `diffWorlds` turns th
 - **`issue reopen` is `unmodelled`.** No effect op reopens an item, so a run that reopens one needs a contract that allows it. Editing a title or body of an existing item is `unmodelled` as well.
 - **A PR is created from the checkout.** Its branch is `--head` or the current git branch, and its commit is that branch's, so `gh pr create` runs in a git repository. The issues it closes are the existing ones its body names with a closing keyword.
 
+### `api` (#88)
+
+`gh api` answers the nine REST calls the package makes, and refuses everything else as `unsupported` (graphql, another method on a path, `-F @file`, `--input`):
+
+| call | answered from |
+| --- | --- |
+| `GET user` | the world's agent login |
+| `GET repos/{owner}/{repo}/issues/N/comments` | the item's comments (an issue's or a PR's), as REST comments with `author_association`, `user.login` and `updated_at` |
+| `PATCH repos/{owner}/{repo}/issues/comments/ID -f body=` | edits a stored comment in place |
+| `GET .../pulls/N/files` | the PR's `files` (a rename is `{path, previous}`, which gives `previous_filename`) |
+| `GET .../contents/PATH?ref=` | the world's `files` (raw with the raw `Accept` header, otherwise a base64 object) |
+| `GET .../git/trees/HEAD?recursive=1` | the paths of the world's `files` |
+| `GET .../pulls?state=&per_page=&page=` and `GET .../pulls/N` | the world's PRs as REST pull requests, newest first. `author_association` is the PR's `association` (default `NONE`) |
+| `PUT .../pulls/N/update-branch -f expected_head_sha=` | gives a `BEHIND` PR a new head and makes it `CLEAN`. A stale sha, a conflict and a PR that is not behind are 422, as on GitHub |
+
+- **A comment edited in place is an effect.** Since the lease comment of a step is edited rather than added, the checker's effect model gained `comment_edit` (the comment's index and its new text), scoped like `comment`. `diffWorlds` reads an edited comment as one, and `applyEffects` makes it.
+- **A comment the simulator derives from a world field (a `lease`, a `readySha`) cannot be edited.** There is no stored comment to change, so the call is `unsupported` and `claim` falls back to posting a comment, as it does when an edit fails.
+- **Pagination is not modelled** beyond `per_page` and `page` on the PR list. `--paginate` returns a list whole.
+
+### Faults (#88)
+
+A world (and so a scenario's `world:`) may carry `faults: [{match, fail, exit?, before?, times?}]`.
+
+- A call that starts with `match` (whole words: `pr merge`, `api -X PATCH`, `api user`) fails with the message `fail` and the exit code `exit` (default 1), writes nothing, and is logged with `kind: "fault"`, the index of the fault and a `faultId`.
+- `before` is a list of effects applied to the world first. With `fail` it is a failure after something changed. Without `fail` the call goes on, which is how another run gets in ahead of this one (the creation race of `log_issue`). A scenario file is read-only, so `before` needs a world file.
+- `times` limits how often a fault fires. Firing is counted in the log, under the fault's definition (`faultId`, a hash of it), so `times` needs `GH_SIM_LOG`. A different fault that is later at the same place in the list starts fresh, and the same fault written again does not.
+- Faults are configuration, not state: they are the same in the world a run starts from and the one it leaves, so `diffWorlds` does not see them, and a scenario that carries them is judged as it was (tested on every golden scenario).
+
+### One mock (#88)
+
+`tests/lib.test.sh` runs against this simulator and nothing else. It sets a world up with a small `w` (jq over the world file) and a few helpers, runs the functions under test, and reads the result back through `gh`. The old stub is gone. Making the tests as strict as `gh` showed three things the stub let through: the `authorAssociation` field (#89), a repository with a label catalog refuses labels it does not have (the tests now start from the catalog `setup-github-agent-workflow` leaves), and issues and PRs share one number sequence (so a log issue made after PRs 70 to 74 is number 75). The last test of the file fails if any call in the run was `unsupported`, so a helper that swallows the failure of a call the simulator does not cover is still noticed.
+
 ## What it does not do
 
 - **It is not GitHub.** Rate limits, pagination, webhooks, merge queues, branch protection and reviews are not modelled. A merge is blocked by the merge state a world gives, nothing else. Some error messages are what `gh` prints and some are close to it (a merge of a closed PR, an unreadable body file).
 - **Some world fields cannot be shown.** `entries`, `kind`, `openQuestions`, `useCase` and `wontfix` stand for facts a model would read from prose (a log entry, a body), not for GitHub state. `seed.mjs` says which ones a world has. A scenario that depends on them needs a runner to write the prose (#61).
 - **A world has no branch names, check names or logs.** A PR is on `feat/<closed issue>-pr-<N>`, which matches the pattern `prs_for_issue` reads, unless it names a `branch`. A check is `check-<i>`. The log of a failing run is one fixed line.
 - **Its field lists are a copy.** They come from one `gh` version and can fall behind.
+- **It is slow next to a shell function.** Each call starts Node (about 75 ms, half of it loading `yaml` through the checker). `tests/lib.test.sh` makes about 390 calls and takes about 45 seconds, where the stub took 10. A compile cache saved little (12 ms a call), so it is not used.
 
 ## Where the simulator and the skills disagree
 
 Following `gh` exactly shows two places where the package assumes something `gh` does not do.
 
-- `lib.sh` asks for `authorAssociation` on a PR, which is not a PR field in any `gh` release (#89).
+- `lib.sh` asked for `authorAssociation` on a PR, which is not a PR field in any `gh` release. Fixed in #89, which the simulator made visible.
 - The shepherd skill's check-state one-liner expects `gh pr checks N --json bucket` to print `[]` for a PR with no checks, and so to answer `no-checks-yet`. `gh` instead exits 1 with `no checks reported on the '<branch>' branch` and prints nothing. The skill also says the exit code is non-zero while checks run, which is true without `--json` (8 while pending) and false with it (always 0). The simulator does what `gh` does, and a test records the difference.

scripts/scenario.mjs

@@ -20,7 +20,7 @@ export const MUTEX_PR = ["agent:needs-work", "agent:ready-for-review", "agent:hu
 export const LEASE_LABELS = ["agent:triage", "agent:in-progress"];
 const REPOSITORY_OPS = ["push", "merge", "delete_branch", "create_pr", "label_create", "label_update", "write_file"];
 // `unmodelled` is not something a skill does: diffWorlds reports a change it cannot express with it.
-const OPS = ["label_add", "label_remove", "comment", "close", "push", "merge", "delete_branch", "create_pr", "create_issue", "review_request", "label_create", "label_update", "write_file", "unmodelled"];
+const OPS = ["label_add", "label_remove", "comment", "comment_edit", "close", "push", "merge", "delete_branch", "create_pr", "create_issue", "review_request", "label_create", "label_update", "write_file", "unmodelled"];
 
 // ---- repository state: the label set and the agent instructions file ------------------------------
 // A world may hold `labels: [{name, description}]` (the repository's label set) and `files: {path: text}`.
@@ -185,7 +185,7 @@ export const ORACLES = {
 export function scopeOf(effect, ctx) {
   if (effect.op === "unmodelled") return "unmodelled";
   if (REPOSITORY_OPS.includes(effect.op)) return "repository";
-  if (effect.op === "comment") {
+  if (effect.op === "comment" || effect.op === "comment_edit") {
     const t = item(ctx.world, effect.target);
     if (t && (t.labels ?? []).includes("agent:log")) return "shared";
   }
@@ -236,6 +236,13 @@ export function applyEffects(world, effects) {
         if (!t.labels.includes(e.label)) throw new Error(`removes \`${e.label}\` from ${e.target}, which does not carry it`);
         t.labels = t.labels.filter((l) => l !== e.label); break;
       case "comment": t.comments.push({ marker: e.marker, text: e.text }); break;
+      // A comment the run edited in place (a lease): `index` is its place among the item's comments, `text` its new text.
+      case "comment_edit": {
+        const c = t.comments[e.index];
+        if (!c) throw new Error(`\`comment_edit\` on ${e.target}: it has no comment ${e.index}`);
+        c.text = e.text;
+        break;
+      }
       case "close": t.state = "closed"; if (e.reason) t.closeReason = e.reason; break;
       case "push": t.head = e.sha ?? `${t.head}+`; t.pushed = true; break;
       case "merge": t.state = "merged"; break;
@@ -308,8 +315,13 @@ export function diffWorlds(before, after) {
 
     const bc = b.comments ?? [], ac = a.comments ?? [];
     const same = (x, y) => x.marker === y.marker && x.text === y.text;
-    if (bc.length > ac.length || bc.some((c, i) => !same(c, ac[i]))) odd(target, "comments", bc, ac);
-    else for (const c of ac.slice(bc.length)) out.push({ op: "comment", target, ...defined({ marker: c.marker, text: c.text }) });
+    // A comment that keeps its marker and has new text was edited in place. Any other change to the comments is `unmodelled`.
+    const edited = (x, y) => y !== undefined && !same(x, y) && x.marker === y.marker && typeof y.text === "string";
+    if (bc.length > ac.length || bc.some((c, i) => !same(c, ac[i]) && !edited(c, ac[i]))) odd(target, "comments", bc, ac);
+    else {
+      bc.forEach((c, i) => { if (!same(c, ac[i])) out.push({ op: "comment_edit", target, index: i, text: ac[i].text }); });
+      for (const c of ac.slice(bc.length)) out.push({ op: "comment", target, ...defined({ marker: c.marker, text: c.text }) });
+    }
 
     if (a.state !== b.state) {
       const open = b.state === "open" || b.state === undefined;

tests/diff-worlds.test.mjs

@@ -118,6 +118,23 @@ test("a title or body edited after the item exists is still unmodelled, and a cr
   assert.deepEqual(unmodelled(W(), edit((w) => { w.issues.push({ number: 3, state: "open", labels: [], comments: [], title: "T", assignee: "x" }); })), ["issue#3 assignee"]);
 });
 
+test("a comment edited in place is a comment_edit, and it round-trips", () => {
+  const before = edit((w) => { w.issues[0].comments = [{ marker: "agent:lease", text: "Claimed <!-- agent:lease step=A -->" }, { author: "human" }]; });
+  const after = edit((w) => { w.issues[0].comments = [{ marker: "agent:lease", text: "Claimed again <!-- agent:lease step=A -->" }, { author: "human" }]; });
+  const diff = diffWorlds(before, after);
+  assert.deepEqual(diff, [{ op: "comment_edit", target: "issue#1", index: 0, text: "Claimed again <!-- agent:lease step=A -->" }]);
+  assert.ok(sameWorld(applyEffects(before, diff), after));
+  // an edit and a new comment in one run
+  const both = edit((w) => { w.issues[0].comments = [{ marker: "agent:lease", text: "x" }, { author: "human" }, { marker: "agent:outcome", text: "done" }]; });
+  assert.deepEqual(diffWorlds(before, both).map((e) => [e.op, e.index ?? e.marker]), [["comment_edit", 0], ["comment", "agent:outcome"]]);
+  assert.ok(sameWorld(applyEffects(before, diffWorlds(before, both)), both));
+  // a comment whose marker changed, or that lost its text, is still not an edit
+  assert.deepEqual(unmodelled(before, edit((w) => { w.issues[0].comments = [{ marker: "agent:outcome", text: "x" }, { author: "human" }]; })), ["issue#1 comments"]);
+  assert.deepEqual(unmodelled(before, edit((w) => { w.issues[0].comments = [{ marker: "agent:lease" }, { author: "human" }]; })), ["issue#1 comments"]);
+  assert.deepEqual(unmodelled(before, edit((w) => { w.issues[0].comments = [{ marker: "agent:lease", text: "Claimed <!-- agent:lease step=A -->" }]; })), ["issue#1 comments"], "a removed comment is not an edit");
+  assert.throws(() => applyEffects(before, [{ op: "comment_edit", target: "issue#1", index: 5, text: "x" }]), /has no comment 5/);
+});
+
 test("a close carries its reason, and the reason is not a change of its own", () => {
   const after = edit((w) => { w.issues[0].state = "closed"; w.issues[0].closeReason = "not planned"; });
   const diff = diffWorlds(W(), after);
@@ -131,7 +148,8 @@ test("unmodelled: fields, comments, state, branches, removals, new items and the
   assert.deepEqual(unmodelled(W(), edit((w) => { w.prs[0].checks = ["fail"]; })), ["pr#10 checks"]);
   assert.deepEqual(unmodelled(W(), edit((w) => { w.prs[0].draft = true; })), ["pr#10 draft"]);
   assert.deepEqual(unmodelled(W(), edit((w) => { w.prs[0].base = "develop"; })), ["pr#10 base"]);
-  assert.deepEqual(unmodelled(W(), edit((w) => { w.issues[0].comments[0].text = "edited"; })), ["issue#1 comments"]);
+  assert.deepEqual(unmodelled(W(), edit((w) => { w.issues[0].comments[0].text = "edited"; })), [], "an edited comment is a comment_edit now");
+  assert.deepEqual(diffWorlds(W(), edit((w) => { w.issues[0].comments[0].text = "edited"; })).map((e) => e.op), ["comment_edit"]);
   assert.deepEqual(unmodelled(W(), edit((w) => { w.issues[0].comments = []; })), ["issue#1 comments"]);
   assert.deepEqual(unmodelled(edit((w) => { w.issues[0].state = "closed"; }), W()), ["issue#1 state"]);
   assert.deepEqual(unmodelled(W(), edit((w) => { w.issues[0].state = "merged"; })), ["issue#1 state"]);