Skip to content

Commit 2567712

Browse files
authored
Merge branch 'main' into copilot/propose-improvements-to-counterfact
2 parents 1ad0064 + 6dbb527 commit 2567712

File tree

13 files changed

+350
-130
lines changed

13 files changed

+350
-130
lines changed

.changeset/renovate-0941c05.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'counterfact': patch
3+
---
4+
5+
Updated dependency `ajv` to `8.18.0`.

.github/copilot-instructions.md

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,14 @@ Each item must use this format:
3333
- [ ] Response includes `x-test` header when defined
3434
- [ ] Existing routes without examples behave unchanged
3535

36+
### Exception: Design PRs
37+
38+
When the goal of the **PR** is to **create more issues rather than write code** (e.g., proposing issue files under `.github/issue-proposals/`), treat the PR as a **design PR**:
39+
40+
1. Add the `design` label to the PR.
41+
2. Do **not** include a "## Manual acceptance tests" section — omit it entirely.
42+
3. The CI check for manual acceptance tests will automatically pass for PRs with the `design` label.
43+
3644
## Test-driven workflow
3745

3846
When implementing a change, work in a test-first or test-guided way whenever practical.
@@ -68,6 +76,30 @@ Add or update tests for:
6876
- generated code, when generation behavior is being changed
6977
- any change that affects external contracts, APIs, schemas, or CLI behavior
7078

79+
### File system operations in tests
80+
81+
When tests need to read or write files, always use `usingTemporaryFiles()` from the `using-temporary-files` package (already a devDependency). Never import `node:fs`, `fs`, `node:fs/promises`, or `fs/promises` directly in test files.
82+
83+
The `$` helper passed to the callback provides:
84+
- `$.add(relativePath, contents)` — create or overwrite a file
85+
- `$.addDirectory(relativePath)` — create a directory
86+
- `$.read(relativePath)` — read a file's contents (returns `Promise<string>`)
87+
- `$.remove(relativePath)` — delete a file
88+
- `$.path(relativePath)` — resolve an absolute path within the temporary directory (use this when passing paths to the code under test)
89+
90+
```ts
91+
import { usingTemporaryFiles } from "using-temporary-files";
92+
93+
it("example", async () => {
94+
await usingTemporaryFiles(async ($) => {
95+
await $.add("input.json", JSON.stringify({ key: "value" }));
96+
const result = await myFunction($.path("input.json"));
97+
const output = await $.read("output.txt");
98+
expect(output).toBe("expected content");
99+
});
100+
});
101+
```
102+
71103
### When tests are hard to add
72104

73105
If a test is difficult to write:
Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
---
2+
title: ".apply Command Design (Approach 1): Minimalist Function Injection"
3+
parentIssue: 1596
4+
labels:
5+
- enhancement
6+
- repl
7+
- design
8+
assignees: []
9+
milestone:
10+
---
11+
12+
## Summary
13+
14+
Implement `.apply` as a plain TypeScript module that exports one or more named functions. When the command is run, Counterfact dynamically imports the file, looks up the named export matching the last path segment, calls that function, and passes the live REPL environment as its argument.
15+
16+
This is the simplest possible design: no new abstractions, no DSL, and no framework. A script is just a named function.
17+
18+
---
19+
20+
## Design
21+
22+
### Script format
23+
24+
An apply script is a TypeScript file with one or more named function exports:
25+
26+
```ts
27+
// scenarios/sold-pets.ts
28+
import type { ApplyContext } from "./types";
29+
30+
export function soldPets($: ApplyContext) {
31+
$.context.petService.reset();
32+
$.context.petService.addPet({ id: 1, status: "sold" });
33+
$.context.petService.addPet({ id: 2, status: "available" });
34+
35+
$.routes.getSoldPets = $.route("/pet/findByStatus").method("get").query({ status: "sold" });
36+
}
37+
```
38+
39+
### The `ApplyContext` type
40+
41+
`ApplyContext` is a generated type that lives in `./types/`. In this first iteration it is always the same shape. In future iterations it will incorporate types from `_.context.ts` files, providing route-specific context types.
42+
43+
```ts
44+
export interface ApplyContext {
45+
/** Root context, same as loadContext("/") */
46+
context: Record<string, unknown>;
47+
/** Load a context object for a specific path */
48+
loadContext: (path: string) => Record<string, unknown>;
49+
/** Named route builders available in the REPL execution context */
50+
routes: Record<string, RouteBuilder>;
51+
/** Create a new RouteBuilder for a given path */
52+
route: (path: string) => RouteBuilder;
53+
}
54+
```
55+
56+
### Invocation
57+
58+
The argument to `.apply` is a slash-separated path. The last segment is the **function name** to call; everything before it is the **file path** (resolved relative to `<basePath>/scenarios/`, with `index.ts` as the default file):
59+
60+
```
61+
.apply foo # scenarios/index.ts → foo($)
62+
.apply foo/bar # scenarios/foo.ts → bar($)
63+
.apply foo/bar/baz # scenarios/foo/bar.ts → baz($)
64+
```
65+
66+
### Feedback output
67+
68+
After execution, the REPL prints:
69+
70+
```
71+
Applied sold-pets/soldPets
72+
```
73+
74+
---
75+
76+
## Implementation sketch
77+
78+
1. Add `.apply` as a dot-command in `src/repl/repl.ts`.
79+
2. Split the argument on `/`: the last segment is the function name; the rest form the file path.
80+
3. Dynamically import the resolved module (using `tsx` or the existing transpiler if the file is TypeScript).
81+
4. Look up the named export matching the function name and call it with the live environment objects.
82+
83+
---
84+
85+
## Trade-offs
86+
87+
| Aspect | Notes |
88+
|---|---|
89+
| **Simplicity** | Minimal API surface; trivial to implement and test |
90+
| **Flexibility** | Scripts have full control; no imposed lifecycle |
91+
| **Composability** | Scripts can call each other via normal `import` |
92+
| **Introspection** | Context changes are not automatically tracked |
93+
| **TypeScript support** | First-class; leverages the existing transpiler |
94+
95+
---
96+
97+
## Acceptance criteria
98+
99+
- [ ] `.apply foo` resolves `scenarios/index.ts` and calls the exported `foo` function
100+
- [ ] `.apply foo/bar` resolves `scenarios/foo.ts` and calls the exported `bar` function
101+
- [ ] `.apply foo/bar/baz` resolves `scenarios/foo/bar.ts` and calls the exported `baz` function
102+
- [ ] The function receives `$` with `{ context, loadContext, routes, route }` as properties
103+
- [ ] Routes injected by the script are available in the REPL after the command runs
104+
- [ ] The REPL prints `Applied <path>` after each successful apply
105+
- [ ] A meaningful error is shown when the file cannot be found or the export is not a function
106+
- [ ] Existing REPL commands and behavior are unaffected

.github/workflows/manual-acceptance-tests.yaml

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,15 +15,26 @@ jobs:
1515
env:
1616
PR_BRANCH: ${{ github.event.pull_request.head.ref }}
1717
PR_BODY: ${{ github.event.pull_request.body }}
18+
PR_LABELS: ${{ toJson(github.event.pull_request.labels.*.name) }}
1819
run: |
19-
if [[ "$PR_BRANCH" == copilot* ]] || grep -iq "## manual acceptance tests" <<<"$PR_BODY"; then
20+
if python3 -c "
21+
import json, os, sys
22+
try:
23+
labels = json.loads(os.environ.get('PR_LABELS', '[]'))
24+
sys.exit(0 if 'design' in labels else 1)
25+
except Exception as e:
26+
print(f'Error parsing PR labels: {e}', file=sys.stderr)
27+
sys.exit(1)
28+
"; then
29+
echo "enforce=false" >> "$GITHUB_OUTPUT"
30+
elif [[ "$PR_BRANCH" == copilot* ]] || grep -iq "## manual acceptance tests" <<<"$PR_BODY"; then
2031
echo "enforce=true" >> "$GITHUB_OUTPUT"
2132
else
2233
echo "enforce=false" >> "$GITHUB_OUTPUT"
2334
fi
2435
- name: Skip (no enforcement needed)
2536
if: steps.decide.outputs.enforce == 'false'
26-
run: echo "Skipping manual acceptance test validation (not a Copilot PR and no section present)"
37+
run: echo "Skipping manual acceptance test validation (design PR, not a Copilot PR, or no section present)"
2738

2839
- name: Validate PR body
2940
if: steps.decide.outputs.enforce == 'true'
Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
# ADR 001: .apply Command Design — Minimalist Function Injection
2+
3+
## Status
4+
5+
Accepted
6+
7+
## Context
8+
9+
Counterfact's REPL lets developers interact with the running mock server from the terminal. A common need is to transition the server into a specific state (e.g. "all pets sold", "service unavailable") in a reproducible, shareable way. Today, operators must manually call REPL commands one by one; there is no mechanism to save and replay a named scenario.
10+
11+
The `.apply` command is proposed to address this: given a path argument, it loads and executes a user-authored script that mutates REPL context and routes, then reports what changed.
12+
13+
Three designs were proposed as working documents in `.github/issue-proposals/`:
14+
15+
- `apply-approach-1-function-injection.md` — plain TypeScript named function exports, no framework coupling
16+
- `apply-approach-2-scenario-class-lifecycle.md` — a `Scenario` class interface with `setup()` / `teardown()` lifecycle hooks and a `dependencies` declaration
17+
- `apply-approach-3-proxy-based-tracking.md` — transparent reactive proxies that intercept all mutations and produce an automatic structured diff
18+
19+
### Key constraints
20+
21+
- Counterfact users are TypeScript developers who prefer writing ordinary code over learning framework-specific APIs.
22+
- The initial implementation must be straightforward to ship, test, and extend without committing to a heavy abstraction layer.
23+
- The REPL already provides `context` and `routes` as live objects; any solution must integrate cleanly with those.
24+
- TypeScript support is first-class via the existing transpiler.
25+
26+
## Decision
27+
28+
**Solution 1 (Minimalist Function Injection) is selected.**
29+
30+
An apply script is a TypeScript file with one or more named function exports. When `.apply <path>` is run, Counterfact splits the argument on `/`, uses the last segment as the function name and the rest as the file path (relative to `<basePath>/repl/`), dynamically imports the module, and calls the named function with a live `ApplyContext` (`$`) object:
31+
32+
```ts
33+
// repl/sold-pets.ts
34+
import type { ApplyContext } from "counterfact";
35+
36+
export function soldPets($: ApplyContext) {
37+
$.context.petService.reset();
38+
$.context.petService.addPet({ id: 1, status: "sold" });
39+
40+
$.routes.getSoldPets = $.route("/pet/findByStatus").method("get").query({ status: "sold" });
41+
}
42+
```
43+
44+
`ApplyContext` exposes `{ context, loadContext, routes, route }`. After the function returns, Counterfact diffs the `routes` object and prints a summary of what was added or removed.
45+
46+
Solution 1 was chosen because it introduces the smallest possible API surface, imposes no structural requirements on script authors, and integrates naturally with TypeScript `import` for composability. It is the right foundation to build on before adding lifecycle or tracking features.
47+
48+
## Options
49+
50+
### Solution 1: Minimalist Function Injection (selected)
51+
52+
Scripts export named functions that receive `$: ApplyContext`. Counterfact resolves the file/function from the path argument and calls the function directly. Route changes are diffed and reported; context changes are not automatically tracked.
53+
54+
**Why chosen:** Maximum simplicity. No new abstractions, no required boilerplate. Easy to implement, test, and understand. Composability via normal `import`.
55+
56+
### Solution 2: Scenario Class with Lifecycle Hooks
57+
58+
Scripts export a named class that implements a `Scenario` interface with `setup()` and optional `teardown()` methods. Counterfact instantiates the class, calls `setup()`, and tracks applied instances in a map for later `.unapply`. A static `dependencies` array enables ordered composition.
59+
60+
**Why not chosen:** Class syntax and lifecycle coupling add complexity that is not justified until the need for teardown and dependency ordering is proven in practice. These concerns can be layered on top of Solution 1 once the basic command exists.
61+
62+
### Solution 3: Reactive Proxy-Based Change Tracking
63+
64+
Identical surface syntax to Solution 1, but Counterfact wraps `context` and `routes` in transparent `Proxy` objects before calling the function. All mutations are intercepted, logged, and printed as a structured diff automatically.
65+
66+
**Why not chosen:** The proxy layer adds significant runtime complexity and a class of subtle edge cases (prototype method calls, deeply nested mutations, proxy-obscured stack traces). The automatic diff is appealing but is a refinement that can be added after Solution 1 is stable, without changing the script-author API.
67+
68+
## Consequences
69+
70+
### What this enables
71+
72+
- Developers can save named scenarios as TypeScript files and replay them from the REPL.
73+
- Scripts are ordinary TypeScript modules; they can import each other, use type-checking, and leverage the existing toolchain.
74+
- The feature ships quickly without a large API commitment.
75+
76+
### Trade-offs accepted
77+
78+
- Context changes are not automatically tracked; script authors must document or annotate context mutations manually.
79+
- There is no built-in teardown mechanism; reverting a scenario requires writing and calling a separate function.
80+
- Dependency ordering between scenarios is the script author's responsibility via normal `import`.
81+
82+
### Risks and downsides
83+
84+
- Without lifecycle hooks, accumulated state across many `.apply` calls may be difficult to reason about.
85+
- If teardown proves to be a common need, adding it later will require extending the API in a backward-compatible way.
86+
- Proxy-based auto-diffing (Solution 3) remains attractive for DX; deferring it means script authors will need to be disciplined about documenting context changes in the short term.
87+
88+
### Follow-up work
89+
90+
- Evaluate whether `teardown` support (from Solution 2) is needed and, if so, define a clean extension point.
91+
- Explore adding proxy-based context diffing (from Solution 3) as an opt-in enhancement once the core command is stable.
92+
- Define `ApplyContext` as a public exported type in `counterfact-types/`.
93+
94+
## Advice
95+
96+
- **Apply this decision** whenever a new scenario management capability is considered for the REPL. Start with a named function in a `.ts` file; reach for classes or proxy wrappers only when a concrete need for lifecycle or auto-tracking is demonstrated. (Copilot/Claude)
97+
- **Revisit this decision** if the lack of teardown creates significant friction for users who need to reset state cleanly, or if the absence of automatic context diffing makes scripts hard to audit. (Copilot/Claude)
98+
- **Prefer Solution 2 or 3** when: (a) scenarios need deterministic cleanup, (b) dependency ordering between scenarios must be enforced automatically, or (c) context mutation tracking is required for auditing or debugging. (Copilot/Claude)
99+
- **Rule of thumb:** keep scripts as plain TypeScript. If you find yourself writing setup/teardown boilerplate repeatedly, that is the signal to revisit lifecycle support. If you find yourself commenting every context change for reviewers, that is the signal to revisit proxy-based diffing. (Copilot/Claude)
100+
- **A natural extension point is the return value of the function** (currently `void`). It could be an optional string used to summarize the changes made by the script. It could also return an object containing a `teardown()` function, providing a lightweight path to lifecycle support without requiring a full class interface. (@pmcelhaney)

eslint.config.cjs

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,33 @@ module.exports = [
114114
"max-lines": "off",
115115
"new-cap": ["error", { capIsNewExceptionPattern: "GET|PUT|POST|DELETE" }],
116116
"no-magic-numbers": "off",
117+
"no-restricted-imports": [
118+
"error",
119+
{
120+
paths: [
121+
{
122+
name: "fs",
123+
message:
124+
"Do not import 'fs' in tests. Use usingTemporaryFiles() from 'using-temporary-files' instead.",
125+
},
126+
{
127+
name: "node:fs",
128+
message:
129+
"Do not import 'node:fs' in tests. Use usingTemporaryFiles() from 'using-temporary-files' instead.",
130+
},
131+
{
132+
name: "fs/promises",
133+
message:
134+
"Do not import 'fs/promises' in tests. Use usingTemporaryFiles() from 'using-temporary-files' instead.",
135+
},
136+
{
137+
name: "node:fs/promises",
138+
message:
139+
"Do not import 'node:fs/promises' in tests. Use usingTemporaryFiles() from 'using-temporary-files' instead.",
140+
},
141+
],
142+
},
143+
],
117144
"no-shadow": "off",
118145
},
119146
},

package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@
126126
"@apidevtools/json-schema-ref-parser": "13.0.5",
127127
"@hapi/accept": "6.0.3",
128128
"@types/json-schema": "7.0.15",
129-
"ajv": "6.14.0",
129+
"ajv": "8.18.0",
130130
"chokidar": "5.0.0",
131131
"commander": "14.0.3",
132132
"debug": "4.4.3",

src/server/request-validator.ts

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import type { OpenApiOperation } from "../counterfact-types/index.js";
44

55
const ajv = new Ajv({
66
allErrors: true,
7-
unknownFormats: "ignore",
7+
strict: false,
88
coerceTypes: false,
99
});
1010

@@ -57,10 +57,7 @@ export function validateRequest(
5757

5858
if (!valid && ajv.errors) {
5959
for (const error of ajv.errors) {
60-
const path =
61-
(error as { instancePath?: string }).instancePath ??
62-
error.dataPath ??
63-
"";
60+
const path = error.instancePath ?? "";
6461

6562
errors.push(`body${path} ${error.message ?? "is invalid"}`);
6663
}
@@ -78,10 +75,7 @@ export function validateRequest(
7875

7976
if (!valid && ajv.errors) {
8077
for (const error of ajv.errors) {
81-
const path =
82-
(error as { instancePath?: string }).instancePath ??
83-
error.dataPath ??
84-
"";
78+
const path = error.instancePath ?? "";
8579

8680
errors.push(`body${path} ${error.message ?? "is invalid"}`);
8781
}

0 commit comments

Comments
 (0)