Skip to content

Commit 95b97ef

Browse files
yacosta738Kerrigan
andauthored
feat(agent): clarify approvals and add manual compaction (#305)
* feat(agent): clarify approvals and add manual compaction Add per-turn approval numbering and action/effect summaries, plus confirmation-gated /compact and /summarize commands that reuse the runtime compaction path. Archive the SDD artifacts and record interactive acceptance as pending. Refs: DALLAY-631 * fix(agent): address PR 305 review findings * fix(agent): address PR 305 second review findings * fix(agent): address PR 305 third review findings * fix(agent): bound PTY reader join without blocking on stale thread --------- Co-authored-by: Kerrigan <kerrigan@local>
1 parent 7e77ca0 commit 95b97ef

20 files changed

Lines changed: 2242 additions & 180 deletions

File tree

‎.agents/sdd/changes/archive/2026-10-09-dallay-631-approval-compact-ux/design.md‎

Lines changed: 394 additions & 0 deletions
Large diffs are not rendered by default.
Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
# Proposal: Approval numbering, header clarity, and explicit `/compact` for the agent CLI
2+
3+
## 1. Intent
4+
5+
**Problem.** Manual acceptance of DALLAY-631 surfaced two product-level UX gaps in `apps/agent`:
6+
7+
1. When the same tool triggers a second approval in the same turn (a denied edit followed by a model-driven retry with a different payload), the terminal prints the new diff and asks for `Approve once? [y/N]` again. The user sees two similar diffs in a row but no signal that the second request is *different* from the first. In one captured session a user denied the first diff but the next prompt already showed a reformulated, partially overlapping diff without re-stating action and target. The wording is technically correct but easy to misread under time pressure.
8+
2. The CLI has no way to proactively compact the session before the configured threshold. Goose's CLI offers `/compact` with a confirmation step; Cortex only compacts automatically when the conservative byte budget is exceeded. That is the right default for safety, but during interactive use it leaves the user without a knob to free room for a longer request, see the compacted state, and decide explicitly.
9+
10+
**Goal.** Reduce the chance of a user approving a different change than intended, and add a confirmation-gated `/compact` command that runs the existing real-model summarization path on demand. No new persistence format, no new provider, no new tool kind. The existing `ApprovalRequest` contract stays unchanged; the loop stays unchanged. All change lives in the terminal policy and the CLI command parser.
11+
12+
**Reference.** Goose's `ToolApprovalOperation` and `/compact` flow: in Goose the approval operation deduplicates and tags each request, and `/compact` asks for explicit confirmation before `do_compact` runs the real `summarize` with the configured `CompactionModel`. We adopt the *outcome* (numbered prompts, confirmation before compaction) without the *mechanism* (a state machine). Cortex's `StandardLoop` already records `ApprovalRequested` and `ApprovalResolved` events and re-summarizes on the same provider; we expose the entry point and tag the prompts.
13+
14+
---
15+
16+
## 2. Scope
17+
18+
### Changes
19+
20+
| Component | Change |
21+
|-------------------------------------------|------------------------------------------------------------------------------------------|
22+
| `apps/agent/src/terminal.rs` | Number approval requests per turn, print `Action:` and `Effect:` headers, show `different from #N` for repeats, and reword `Approve once?` to `Approve this exact change?` |
23+
| `apps/agent/src/main.rs` | Reset the per-turn approval counter when a new user prompt starts; add a `/compact` chat command that runs the loop's existing compaction with a confirmation prompt |
24+
| `apps/agent/src/terminal.rs` | Provide a `Policy::reset_turn()` helper invoked by the CLI before each new turn |
25+
| `apps/agent/tests/cli.rs` | New test covering two approvals in one turn (deny + confirm `different from #1`) |
26+
| `docs/agent/implementation-specification.md` | Update approval UX section to describe the new headers and `/compact` flow |
27+
| `docs/agent/linux-daily-use.md` | Mention `/compact` as a manual trigger |
28+
| `docs/agent/validation.md` | Note the change in the local Ollama record |
29+
30+
### Does NOT Change
31+
32+
- `ApprovalRequest` and `ApprovalPolicy` traits.
33+
- The streaming adapter, the MCP previews, the compaction itself, the SQLite session format, or the resume flow.
34+
- The tool registry, transport, or provider selection.
35+
36+
---
37+
38+
## 3. Approach
39+
40+
### High-Level Strategy
41+
42+
1. **Per-turn approval counter, no contract change.** `Policy` holds a `Mutex<u64>` that tracks how many approvals have been requested in the current turn. The CLI calls `policy.reset_turn()` when it starts a new turn (`chat` loop, `Run`, `Resume`). The `approve()` method increments the counter and renders a header like:
43+
44+
```
45+
Approval for native.write_file (request #1 this turn):
46+
Action: native.write_file
47+
Effect: write README.md
48+
<diff completo>
49+
Approve this exact change? [y/N]
50+
```
51+
52+
For a repeat in the same turn the header gains a tail line:
53+
54+
```
55+
Approval for native.write_file (request #2 this turn, different from #1):
56+
```
57+
58+
This keeps the existing `ApprovalRequest` struct untouched (no new required fields) and stays inside the per-turn scope we already document in `implementation-specification.md` (the action grant `last only for the current invocation`).
59+
60+
2. **Stable `Effect:` line derived from the action.** The first line of the `preview` is a `Directory:` for `native.shell` and the diff header `--- README.md / +++ README.md` for `native.write_file`/`native.edit_file`. MCP previews already identify server and tool. We render a single `Effect:` line — kept short — derived from the first non-empty line of the preview when the action is `native.*`. This avoids parsing the full preview and adds a second visible signal for action and target alongside the action name itself.
61+
62+
3. **`/compact` reuses the existing real-model summarization.** The CLI recognizes `/compact` (and keeps `/summarize` as a deprecated alias) before calling `implementation.0.run(...)`. The handler:
63+
- prints `Compact session now? Older history will be summarized; originals stay in the database. [y/N]`;
64+
- reads a confirmation from the same `Input` used for approvals;
65+
- on yes, calls a new `LoopService::compact_now(&mut session, &policy, &output, cancellation)` method that performs the same `summary_request` already executed in `StandardLoop::context()`, including the `Event::Compacted` emission and SQLite save;
66+
- on no, returns to the prompt and continues.
67+
68+
No new provider call path. No new request shape. The same model, the same system prompt, the same 1024-token `max_tokens` for the summary. The automatic path still triggers at the conservative byte threshold; `/compact` is a manual trigger that asks the same summarizer to run now.
69+
70+
4. **Confirmation uses the same `Input` reader.** We do not introduce a second reader. The `Input` is already decoupled from the Tokio runtime via the detached thread, so reusing it from `/compact` does not interfere with the read loop. We treat the confirmation the same as an approval denial: empty input defaults to no.
71+
72+
### Data Structures
73+
74+
```rust
75+
// apps/agent/src/terminal.rs
76+
77+
pub struct Policy {
78+
pub input: Option<Arc<Input>>,
79+
pub allowed: BTreeSet<String>,
80+
turn_count: Mutex<u64>,
81+
}
82+
83+
impl Policy {
84+
pub fn reset_turn(&self) {
85+
*self.turn_count.lock().unwrap() = 0;
86+
}
87+
88+
pub fn turn_count(&self) -> u64 {
89+
*self.turn_count.lock().unwrap()
90+
}
91+
}
92+
```
93+
94+
`LoopService::compact_now` is a thin wrapper over the existing summarization block. It must remain available without modifying the `AgentLoop` trait, so we expose it as an inherent method on the concrete loop type. The agent binary looks up the loop via the existing `composition::loop_id()` and downcasts, or we add a small trait extension `CompactNow` implemented only by `StandardLoop`. We choose the trait extension to keep the composition kernel unchanged.
95+
96+
### Concurrency Considerations
97+
98+
- The `Mutex<u64>` is only ever held for a few instructions per approval request; contention is bounded by the cost of one model turn. No async lock is needed.
99+
- `/compact` runs while the chat loop is between model turns (no overlapping calls), so a simple `Mutex` is sufficient.
100+
- The confirmation line is read through the existing `Input` reader, so a denial does not block an unrelated `read_file` later in the same session.
101+
102+
---
103+
104+
## 4. Alternatives Considered
105+
106+
| Alternative | Why Not Chosen |
107+
|--------------------------------------------------|----------------------------------------------------------------------------------------|
108+
| Add an `ApprovalRequest` field `attempt: u32` | Cross-crate contract change for a UI concern; the per-turn counter belongs to the policy |
109+
| Track diff hashes server-side | Bigger change for the same outcome; hashes add no information the human can act on |
110+
| Auto-retry denial as a hard rule in the loop | The user explicitly preferred transparent retry with a new approval, not a silent rule |
111+
| Add `/compact` as a CLI subcommand instead | A slash command in `chat` matches Goose UX and keeps the same prompt context |
112+
| Run compaction in a worker that always shows progress | The current `Compacted` event is sufficient; a progress bar is out of scope |
113+
114+
---
115+
116+
## 5. Risks
117+
118+
| Risk | Mitigation |
119+
|---------------------------------------------------|------------------------------------------------------------------------------|
120+
| Re-asking for confirmation distracts the user | Numbering and the `different from #N` line make repetition explicit, not opaque |
121+
| `/compact` summary consumes tokens and time | The existing summarizer is bounded by `max_tokens=1024` and the provider timeouts |
122+
| Resetting the counter at the wrong time | The CLI calls `reset_turn()` exactly once per new turn, before `run(...)` is invoked |
123+
| Reusing the `Input` reader for `/compact` blocks other input | The reader runs in a detached thread; we treat the line as confirmation, not as a new prompt |
124+
125+
---
126+
127+
## 6. Success Criteria
128+
129+
1. When the model triggers the same tool twice in a turn, the second prompt header includes `(request #2 this turn, different from #1)` and the action name is reprinted.
130+
2. Denying the first prompt in a pair still leaves the workspace unchanged.
131+
3. `/compact` in `chat` confirms once, then runs the real-model summarizer and prints `Context compacted; original history retained.`
132+
4. `/summarize` is still accepted and behaves identically to `/compact`.
133+
5. Existing focused tests pass; the new test passes.
134+
6. `cargo fmt --check` and `cargo clippy --workspace --all-targets -- -D warnings` stay clean.
135+
136+
---
137+
138+
## 7. Open Questions
139+
140+
- Should the action grant set (`--allow`) also reset per turn? The current contract is *invocation*, not *turn*; we will not change that here. If the user wants finer control later, it is a separate proposal.
141+
- Should the policy print a one-line summary at the end of the turn with `Approvals: #2 (1 denied, 1 approved)`? Likely useful but can be added later if needed; not in scope.
Lines changed: 225 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,225 @@
1+
# Specification: Approval numbering, header clarity, and explicit `/compact`
2+
3+
## Overview
4+
5+
This spec defines the behavior for two user-facing changes in the `agent` CLI:
6+
7+
1. Each interactive approval within a single user turn is numbered, and repeats carry a `different from #N` hint.
8+
2. The `chat` command supports `/compact` (and keeps `/summarize` as a deprecated alias) to run the existing real-model summarizer with a confirmation prompt.
9+
10+
The change is local to the terminal policy in `apps/agent/src/terminal.rs` and the chat loop in `apps/agent/src/main.rs`. The `agent-core` and `agent-runtime` crates do not change shape.
11+
12+
---
13+
14+
## 1. Requirements
15+
16+
### R1: Numbered approval prompts
17+
18+
**Given**: A turn in `chat` that triggers a tool with an `approval` preview.
19+
**When**: `Policy::approve(request, cancel)` is invoked.
20+
**Then**: The terminal prints the request number for that turn in the header line, and increments an internal counter that resets at the start of each new turn.
21+
22+
**Header shape**:
23+
24+
```
25+
Approval for <request.action> (request #N this turn):
26+
Action: <request.action>
27+
Effect: <one-line summary derived from preview>
28+
<preview>
29+
Approve this exact change? [y/N]
30+
```
31+
32+
When `N > 1`, the header suffix becomes `(request #N this turn, different from #<N-1>):`.
33+
34+
### R2: Per-turn counter lifecycle
35+
36+
**Given**: A turn has multiple approval requests.
37+
**When**: The next turn starts (`chat` reads a new line from `Input`, or `Run`/`Resume` is invoked).
38+
**Then**: The counter resets to 0 *before* the next call to `run(...)`.
39+
40+
The counter is held in a `Mutex<u64>` on `Policy`. The CLI calls `Policy::reset_turn()` exactly once before each new turn. `approve()` increments the counter on entry, after the `--allow` short-circuit is taken. `reset_turn()` is the only way to bring the counter back to 0.
41+
42+
### R3: `Effect:` line derivation
43+
44+
**Given**: An `ApprovalRequest` with a non-empty `preview`.
45+
**When**: The header is rendered.
46+
**Then**: A single `Effect:` line is included, drawn from the first non-empty line of `preview` if it starts with a known prefix (`Directory:`, `Trusted local MCP server`, `Server:`, `MCP tool`, or a unified-diff `--- ` line). Otherwise the line is omitted.
47+
48+
This avoids parsing the full diff and stays within the existing preview content. The diff itself is still printed in full below the header.
49+
50+
### R4: `/compact` command in `chat`
51+
52+
**Given**: A user types `/compact` (or `/summarize`) at the `agent>` prompt.
53+
**When**: The chat loop reads the line.
54+
**Then**: The CLI prints a confirmation prompt `Compact session now? Older history will be summarized; originals stay in the database. [y/N]`. Only a `y`/`Y`/`yes` line continues.
55+
56+
**On yes**: The CLI invokes the loop's real-model summarizer on the same session, persists the result, and emits the same `Event::Compacted` the automatic path emits. The terminal prints `Context compacted; original history retained.`
57+
58+
**On no or empty input**: The CLI prints `Compaction skipped.` and returns to `agent>` without touching the session.
59+
60+
`/compact` is not exposed in `Run` (single prompt, no chat). `Run` already calls the loop exactly once; the automatic threshold is the only compaction trigger there.
61+
62+
### R5: Reuse of existing summarization
63+
64+
**Given**: The loop is asked to compact now.
65+
**When**: It runs.
66+
**Then**: It must use the same `summary_request` already used by `StandardLoop::context()` (system prompt `Summarize repository work…`, no tools, `max_tokens=1024`, prior summary appended if present). The `Event::Compacted { through }` is recorded, `session.summary` and `session.summary_through` are updated, and the SQLite store is saved.
67+
68+
No new request shape. No new model call surface. No new provider path.
69+
70+
### R6: Approval state on denial
71+
72+
**Given**: A `Tool` whose `prepare()` returned an `approval` and the user denied.
73+
**When**: The denial reaches the loop.
74+
**Then**: The loop returns `AgentError::Tool("action denied by user; do not retry without a new user request")` for that iteration. The terminal does not block subsequent prompts; the next turn starts cleanly with a reset counter.
75+
76+
### R7: Backwards compatibility
77+
78+
**Given**: Existing callers and tests of the agent runtime and core crates.
79+
**When**: This change is built.
80+
**Then**: No public API of `agent-core` or `agent-runtime` changes. The only API change is the addition of `Policy::reset_turn()` and `Policy::turn_count()` on the binary-local `Policy` type. The `CompactNow` trait added to expose manual compaction is `impl`-only for `StandardLoop` and lives in the binary crate.
81+
82+
---
83+
84+
## 2. Scenarios
85+
86+
### Scenario 1: Single approval, no repeats
87+
88+
```
89+
agent> In README.md add a line "Approved once."
90+
Approval for native.write_file (request #1 this turn):
91+
Action: native.write_file
92+
Effect: write README.md
93+
--- README.md 2026-10-09
94+
+++ README.md 2026-10-09
95+
@@
96+
Marea Demo
97+
...
98+
+Approved once.
99+
Approve this exact change? [y/N] y
100+
Tool: write_file
101+
```
102+
103+
### Scenario 2: Denied then reformulated retry
104+
105+
```
106+
agent> In README.md add a line "Approved once."
107+
Approval for native.write_file (request #1 this turn):
108+
Action: native.write_file
109+
Effect: write README.md
110+
<diff A>
111+
Approve this exact change? [y/N] n
112+
Tool failed: tool: action denied by user; do not retry without a new user request
113+
114+
(model reformulates and asks again)
115+
116+
Approval for native.write_file (request #2 this turn, different from #1):
117+
Action: native.write_file
118+
Effect: write README.md
119+
<diff B>
120+
Approve this exact change? [y/N]
121+
```
122+
123+
### Scenario 3: `/compact` accepted
124+
125+
```
126+
agent> /compact
127+
Compact session now? Older history will be summarized; originals stay in the database. [y/N] y
128+
Context compacted; original history retained.
129+
agent>
130+
```
131+
132+
### Scenario 4: `/compact` declined
133+
134+
```
135+
agent> /compact
136+
Compact session now? Older history will be summarized; originals stay in the database. [y/N] n
137+
Compaction skipped.
138+
agent>
139+
```
140+
141+
### Scenario 5: `/summarize` alias
142+
143+
Same behavior as Scenario 3 / 4, with the alias accepted by the parser.
144+
145+
---
146+
147+
## 3. API Changes
148+
149+
### Terminal
150+
151+
```rust
152+
// apps/agent/src/terminal.rs
153+
154+
pub struct Policy {
155+
pub input: Option<Arc<Input>>,
156+
pub allowed: BTreeSet<String>,
157+
turn_count: std::sync::Mutex<u64>,
158+
}
159+
160+
impl Policy {
161+
pub fn reset_turn(&self) { ... }
162+
pub fn turn_count(&self) -> u64 { ... }
163+
}
164+
```
165+
166+
### Compact trait
167+
168+
```rust
169+
// apps/agent/src/main.rs (or a new file under apps/agent/src/)
170+
171+
#[async_trait::async_trait]
172+
pub trait CompactNow {
173+
async fn compact_now(
174+
&self,
175+
session: &mut Session,
176+
approvals: &dyn ApprovalPolicy,
177+
sink: &dyn EventSink,
178+
cancel: CancellationToken,
179+
) -> Result<()>;
180+
}
181+
182+
impl CompactNow for StandardLoop { ... }
183+
```
184+
185+
The composition kernel already returns `LoopService(pub Arc<dyn AgentLoop>)`. We either (a) downcast in the CLI when the concrete type is `StandardLoop` (no trait change), or (b) keep a parallel `Arc<StandardLoop>` for the binary. We choose (b) to avoid `Any::downcast` fragility; the loop is built locally in the binary so the concrete handle is available.
186+
187+
### CLI command
188+
189+
No new `clap` subcommand. `/compact` is intercepted inside the `chat` loop, before invoking `implementation.0.run(...)`. `Run` does not expose it.
190+
191+
---
192+
193+
## 4. Configuration
194+
195+
No configuration change. The temporary test config that lowers `context_tokens` to 4096 still triggers automatic compaction; `/compact` complements, not replaces, the threshold.
196+
197+
---
198+
199+
## 5. Metrics and Observability
200+
201+
No new metrics. The existing `Event::Compacted { through }` already records the path. Whether the user invoked `/compact` or the threshold fired is visible in the session log by the absence/presence of the user message preceding it.
202+
203+
---
204+
205+
## 6. Testing Requirements
206+
207+
### Unit / integration (apps/agent/tests/cli.rs)
208+
209+
- `two_consecutive_approvals_number_them_and_deny_preserves_workspace` — drives the loop with a scripted provider that emits `write_file` twice in one turn, asserts the captured stderr contains `request #1 this turn` and `request #2 this turn, different from #1`, denies the second, and verifies the file is unchanged.
210+
- `compact_command_summarizes_when_accepted` — uses the mock provider to send a long history, types `/compact`, accepts, and asserts a `summary` is now present in the session.
211+
- `compact_command_skips_when_declined` — same setup, declines, asserts the session has no `summary`.
212+
213+
### Existing tests must continue to pass
214+
215+
- `cargo test -p agent-runtime --test transports` (6 tests)
216+
- `cargo test -p agent-runtime --test coding_workflow` (27 tests)
217+
- `cargo test -p cortex-agent --test cli` (2 tests; new tests added to this file)
218+
219+
---
220+
221+
## 7. Open Questions (Deferred)
222+
223+
- Per-turn approval summary line at the end of the turn.
224+
- A `--strict` mode that re-prompts on denial instead of letting the loop surface the error to the model.
225+
- A `/compact dry-run` that prints the projected summary size without running the model.

0 commit comments

Comments
 (0)