Skip to content

Commit d0d7520

Browse files
Copilotpmcelhaney
andauthored
Address ADR and proposal review feedback: attributions, scenarios dir, ApplyContext note, remove diffs
Agent-Logs-Url: https://github.com/counterfact/api-simulator/sessions/848979b4-0054-4f46-a6a1-0e023afbc87d Co-authored-by: pmcelhaney <51504+pmcelhaney@users.noreply.github.com>
1 parent d33b8cd commit d0d7520

File tree

2 files changed

+18
-21
lines changed

2 files changed

+18
-21
lines changed

.github/issue-proposals/apply-approach-1-function-injection.md

Lines changed: 13 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,8 @@ This is the simplest possible design: no new abstractions, no DSL, and no framew
2424
An apply script is a TypeScript file with one or more named function exports:
2525

2626
```ts
27-
// repl/sold-pets.ts
28-
import type { ApplyContext } from "counterfact";
27+
// scenarios/sold-pets.ts
28+
import type { ApplyContext } from "./types";
2929

3030
export function soldPets($: ApplyContext) {
3131
$.context.petService.reset();
@@ -38,6 +38,8 @@ export function soldPets($: ApplyContext) {
3838

3939
### The `ApplyContext` type
4040

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+
4143
```ts
4244
export interface ApplyContext {
4345
/** Root context, same as loadContext("/") */
@@ -53,27 +55,22 @@ export interface ApplyContext {
5355

5456
### Invocation
5557

56-
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>/repl/`, with `index.ts` as the default file):
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):
5759

5860
```
59-
.apply foo # repl/index.ts → foo($)
60-
.apply foo/bar # repl/foo.ts → bar($)
61-
.apply foo/bar/baz # repl/foo/bar.ts → baz($)
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($)
6264
```
6365

6466
### Feedback output
6567

66-
After execution, Counterfact compares the environment state before and after the script runs and prints a diff summary:
68+
After execution, the REPL prints:
6769

6870
```
6971
Applied sold-pets/soldPets
70-
71-
Routes added:
72-
getSoldPets
7372
```
7473

75-
Context diffs are not automatically tracked in this approach — the script author is responsible for noting any context changes in a comment or in the summary.
76-
7774
---
7875

7976
## Implementation sketch
@@ -82,7 +79,6 @@ Context diffs are not automatically tracked in this approach — the script auth
8279
2. Split the argument on `/`: the last segment is the function name; the rest form the file path.
8380
3. Dynamically import the resolved module (using `tsx` or the existing transpiler if the file is TypeScript).
8481
4. Look up the named export matching the function name and call it with the live environment objects.
85-
5. Snapshot `routes` before/after and print the diff.
8682

8783
---
8884

@@ -100,11 +96,11 @@ Context diffs are not automatically tracked in this approach — the script auth
10096

10197
## Acceptance criteria
10298

103-
- [ ] `.apply foo` resolves `repl/index.ts` and calls the exported `foo` function
104-
- [ ] `.apply foo/bar` resolves `repl/foo.ts` and calls the exported `bar` function
105-
- [ ] `.apply foo/bar/baz` resolves `repl/foo/bar.ts` and calls the exported `baz` function
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
106102
- [ ] The function receives `$` with `{ context, loadContext, routes, route }` as properties
107103
- [ ] Routes injected by the script are available in the REPL after the command runs
108-
- [ ] The REPL prints a summary of routes added and removed after each apply
104+
- [ ] The REPL prints `Applied <path>` after each successful apply
109105
- [ ] A meaningful error is shown when the file cannot be found or the export is not a function
110106
- [ ] Existing REPL commands and behavior are unaffected

docs/adr/001-apply-command-with-function-injection.md

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,8 @@ Identical surface syntax to Solution 1, but Counterfact wraps `context` and `rou
9393

9494
## Advice
9595

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.
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.
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.
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.
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)

0 commit comments

Comments
 (0)