From 628c17d6cda6bad6ab6bfc446da1ab2c597e2cb5 Mon Sep 17 00:00:00 2001 From: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> Date: Sun, 2 Aug 2026 23:53:50 -0700 Subject: [PATCH 1/3] feat: apply display-length clip for goal progress summary at render time Goal progress renderer now clips run summaries to display length (120 chars), moved from orchestrator so stored events retain full text while terminal output stays single-line. Blocker and reason prose are deliberately never clipped. Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- amplifier_app_cli/goal_progress_hook.py | 62 +++++++++++-- tests/test_goal_progress_hook.py | 112 +++++++++++++++++++++++- 2 files changed, 164 insertions(+), 10 deletions(-) diff --git a/amplifier_app_cli/goal_progress_hook.py b/amplifier_app_cli/goal_progress_hook.py index 81835a5..2439d0d 100644 --- a/amplifier_app_cli/goal_progress_hook.py +++ b/amplifier_app_cli/goal_progress_hook.py @@ -46,6 +46,18 @@ # header below. _TERMINAL_STATES = frozenset({"achieved", "cap_hit", "cancelled", "error", "stalled"}) +# Display-only bound on the model-generated `summary` field, so one goal +# summary never spans more than one terminal line. The orchestrator +# (amplifier-module-loop-streaming) stores and emits the model's full +# summary text unclipped -- storage and display are different concerns, +# and only this renderer needs a one-line guarantee. Matches the +# character target the orchestrator's per-state summary prompts already +# ask the model for (see _GOAL_SUMMARY_SYSTEM_PROMPTS), so truncation is +# the rare case, not the common one. Never applied to `reason`: that field +# is short by construction at the source and upstream tests pin it as +# surviving verbatim regardless of length. +_SUMMARY_DISPLAY_MAX_CHARS = 120 + # Grammar for every terminal header: " -- ". Status is # always one of exactly these three phrases, and the glyph always matches -- # a skimmer reading only the first three words gets the answer, and nothing @@ -198,13 +210,21 @@ def _body_lines(state: str, data: dict[str, Any]) -> list[str]: """The (at most two) prose lines under a terminal header, by state. - stalled: the collapsed blocker phrase (never the raw blocker list). - - cap_hit: the summary (preferred) or reason, prefixed "still open:", - plus a static hint -- correct in exactly this state, because this is - the one state where more turns might actually finish the job. - - cancelled / error: an optional single line from reason (preferred) or - summary, verbatim -- no label, since the header already carries the - cause. + - cap_hit: the summary (preferred, clipped to + ``_SUMMARY_DISPLAY_MAX_CHARS`` for display) or reason (verbatim, + never clipped), prefixed "still open:", plus a static hint -- correct + in exactly this state, because this is the one state where more turns + might actually finish the job. + - cancelled / error: an optional single line from reason (preferred, + verbatim) or summary (clipped for display) -- no label, since the + header already carries the cause. - achieved: never called; see ``_render_terminal``. + + Only the `summary` field is ever clipped for display -- `reason` is + always rendered verbatim, since it's already short by construction at + the source (unlike `summary`, which is unbounded free-form model text -- + the orchestrator stores/emits it in full; see amplifier-module-loop- + streaming's ``_summarize_goal_run``). """ if state == "stalled": line = _stalled_line(data) @@ -212,19 +232,43 @@ def _body_lines(state: str, data: dict[str, Any]) -> list[str]: if state == "cap_hit": lines: list[str] = [] - narrative = data.get("summary") or data.get("reason") + summary = data.get("summary") + narrative = _clip_for_display(summary) if summary else data.get("reason") if narrative: lines.append(f"still open: {narrative.strip()}") lines.append("rerun with a higher cap to finish") return lines if state in ("cancelled", "error"): - narrative = data.get("reason") or data.get("summary") - return [narrative.strip()] if narrative else [] + reason = data.get("reason") + if reason: + return [reason.strip()] + summary = data.get("summary") + return [_clip_for_display(summary).strip()] if summary else [] return [] +def _clip_for_display(text: str, max_chars: int = _SUMMARY_DISPLAY_MAX_CHARS) -> str: + """Hard-clip ``text`` to at most ``max_chars`` for one-line console + display, breaking at the last whole word rather than mid-word. + + Display-only: the orchestrator stores/emits the model's full summary + text unclipped (see amplifier-module-loop-streaming's + ``_summarize_goal_run``). This is the sole place that bounds it, and + only for what gets printed here -- callers must never persist or + re-emit this clipped result as if it were the stored value. + """ + text = text.strip() + if len(text) <= max_chars: + return text + truncated = text[:max_chars] + last_space = truncated.rfind(" ") + if last_space > 0: + truncated = truncated[:last_space] + return truncated.rstrip() + + def _stalled_line(data: dict[str, Any]) -> str | None: """Build the single stalled-state prose line. diff --git a/tests/test_goal_progress_hook.py b/tests/test_goal_progress_hook.py index 1069f77..dd08cdb 100644 --- a/tests/test_goal_progress_hook.py +++ b/tests/test_goal_progress_hook.py @@ -26,7 +26,11 @@ sys.path.insert(0, str(Path(__file__).parent)) -from amplifier_app_cli.goal_progress_hook import GoalProgressHook +from amplifier_app_cli.goal_progress_hook import ( + _SUMMARY_DISPLAY_MAX_CHARS, + GoalProgressHook, + _clip_for_display, +) WIDTHS = (40, 80, 200) @@ -576,3 +580,109 @@ async def test_very_long_prose_survives_verbatim_no_truncation(self, monkeypatch out = buffer.getvalue() assert very_long_reason in out assert "\u2026" not in out + + +class TestClipForDisplay: + """Unit-level coverage of `_clip_for_display` in isolation -- exact, + hardcoded expected output, independent of any event rendering.""" + + def test_text_under_cap_is_unchanged(self): + text = "short and under the cap" + assert _clip_for_display(text) == text + + def test_text_over_cap_clips_at_word_boundary(self): + # 130 chars, well over the default 120-char cap, with clean word + # boundaries throughout so the expected clip point is exact. + text = ( + "the evaluator keeps reporting the exact same blocker every " + "single turn and no new progress has been made toward the " + "condition at all whatsoever" + ) + assert len(text) > _SUMMARY_DISPLAY_MAX_CHARS + result = _clip_for_display(text) + assert len(result) <= _SUMMARY_DISPLAY_MAX_CHARS + # Clipped at the last whole word within the cap -- never a partial + # word, and never the raw text[:120] slice verbatim. + assert text.startswith(result) + assert not result.endswith(" ") + cutoff = text[:_SUMMARY_DISPLAY_MAX_CHARS] + assert result == cutoff[: cutoff.rfind(" ")] + + def test_custom_max_chars_honored(self): + text = "one two three four five six seven eight nine ten" + result = _clip_for_display(text, max_chars=10) + assert len(result) <= 10 + assert result == "one two" + + +class TestSummaryClippedForDisplayOnly: + """Display-time clipping of the `summary` field -- storage/emission is + the orchestrator's concern (amplifier-module-loop-streaming stores and + emits the model's full summary text unclipped); this hook clips only + what it prints, so a one-line render is guaranteed regardless of how + long the upstream stored/graph-recorded text is. `reason` is never + clipped -- see TestWidthAgnosticRendering. + test_very_long_prose_survives_verbatim_no_truncation for that guarantee. + """ + + @pytest.mark.asyncio + async def test_cap_hit_overlong_summary_is_clipped_in_still_open_line(self, hook): + overlong_summary = ( + "the evaluator determined the goal was not fully satisfied " + "because several acceptance criteria remained unaddressed " + "including the changelog entry and the regression tests" + ) + assert len(overlong_summary) > _SUMMARY_DISPLAY_MAX_CHARS + await hook.on_goal_progress( + "orchestrator:goal_progress", + { + "state": "cap_hit", + "cap": 8, + "continuations": 8, + "summary": overlong_summary, + }, + ) + out = _joined(hook) + assert overlong_summary not in out + assert _clip_for_display(overlong_summary) in out + + @pytest.mark.asyncio + async def test_cap_hit_overlong_reason_fallback_is_not_clipped(self, hook): + """When there's no summary, cap_hit falls back to `reason` -- + which, unlike `summary`, is never clipped even if it happens to be + long.""" + overlong_reason = ( + "the evaluator determined the goal was not fully satisfied " + "because several acceptance criteria remained unaddressed " + "including the changelog entry and the regression tests" + ) + assert len(overlong_reason) > _SUMMARY_DISPLAY_MAX_CHARS + await hook.on_goal_progress( + "orchestrator:goal_progress", + { + "state": "cap_hit", + "cap": 8, + "reason": overlong_reason, + "summary": None, + }, + ) + out = _joined(hook) + assert overlong_reason in out + + @pytest.mark.asyncio + async def test_error_overlong_summary_fallback_is_clipped(self, hook): + """cancelled/error prefer `reason`; when absent, the `summary` + fallback is clipped just like cap_hit's.""" + overlong_summary = ( + "the evaluator determined the goal was not fully satisfied " + "because several acceptance criteria remained unaddressed " + "including the changelog entry and the regression tests" + ) + assert len(overlong_summary) > _SUMMARY_DISPLAY_MAX_CHARS + await hook.on_goal_progress( + "orchestrator:goal_progress", + {"state": "error", "reason": None, "summary": overlong_summary}, + ) + out = _joined(hook) + assert overlong_summary not in out + assert _clip_for_display(overlong_summary) in out From dc0bed608473eb466438004946c9a4d3b0b95790 Mon Sep 17 00:00:00 2001 From: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> Date: Mon, 3 Aug 2026 00:49:49 -0700 Subject: [PATCH 2/3] feat: stall detection now fires while agent is busy Previously a stall was only detectable on a continuation turn that made zero tool calls. This missed the dominant real-world failure mode: the three worst stalls in the corpus (31, 24, and 54 turns) made tool calls every turn while the goal had become unsatisfiable, so the detector never fired. A second trigger, independent of tool activity, now runs alongside the original. It uses a cheap deterministic pre-filter over recent evaluator reasons every turn (no model call) and only consults the stall judge when the pre-filter trips, preserving the invariant that a mechanical condition alone never trips a stall. The stall judge was reframed from binary yes/no into a verdict taxonomy: resolvable, time-locked, structure-locked, or history-locked. The last three all mean stop now and name the dead end, which users need to rewrite the goal. The judge's prompts were split by trigger, since the original prompt hard-codes the assertion that the assistant took no tool actions. The CLI's stalled-state wording now derives from that verdict instead of from a distinct-blocker count. The old counter compared strings with only whitespace and case normalization, so an evaluator that rephrased a blocker each turn produced a distinct signature per turn, yielding a false "flailing" verdict. The evaluator reason list is now capped before being passed to the summary model; it previously grew unbounded and a 54-turn run shipped all 54 reasons. Replayed against the three recorded reason chains: new pre-filter first consults judge at turn 14 (stalled at 31), turn 12 (stalled at 24), and turn 18 (stalled at 54). On achieved organic runs from the same sessions: zero false trips. Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- amplifier_app_cli/goal_progress_hook.py | 45 ++++++++++++++---- tests/test_goal_progress_hook.py | 62 +++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 8 deletions(-) diff --git a/amplifier_app_cli/goal_progress_hook.py b/amplifier_app_cli/goal_progress_hook.py index 2439d0d..a2d0d5b 100644 --- a/amplifier_app_cli/goal_progress_hook.py +++ b/amplifier_app_cli/goal_progress_hook.py @@ -98,6 +98,17 @@ # doesn't double-count or fail to collapse entries that arrive pre-annotated. _REPEAT_SUFFIX = re.compile(r"\s*\(\u00d7(\d+)\)\s*$") +# `stall_verdict` values (see amplifier-module-loop-streaming's +# `_judge_stall`) that mean the judge confirmed a durable dead end, as +# opposed to "resolvable" (more work could plausibly close it) or `None` +# (an older orchestrator that predates the taxonomy, or -- in principle -- +# a stalled event whose judge call was never actually confirmed). Used by +# `_stalled_line` to decide wall-vs-flailing wording; see that function's +# docstring and GOAL-HARDENING-DESIGN.md sec 1.5 for why `distinct_blockers` +# alone gets this wrong for the normal case (an evaluator that rephrases +# the same blocker every turn). +_LOCKED_VERDICTS = frozenset({"time-locked", "structure-locked", "history-locked"}) + class GoalProgressHook: """Renders ``orchestrator:goal_progress`` events to the CLI console. @@ -272,16 +283,30 @@ def _clip_for_display(text: str, max_chars: int = _SUMMARY_DISPLAY_MAX_CHARS) -> def _stalled_line(data: dict[str, Any]) -> str | None: """Build the single stalled-state prose line. - Never prints the blocker list. Instead: if every (collapsed) blocker is - the same, says so with a turn count ("same blocker 4 turns running: - ") -- a wall. If the blockers genuinely differ, says that - instead ("4 turns, 4 different blockers, none resolved") -- flailing is - a different diagnosis from a wall, and the phrase should say which. + Never prints the blocker list. Instead: if this is a wall -- either + every (collapsed) blocker is textually the same, OR the judge's + `stall_verdict` (see amplifier-module-loop-streaming's `_judge_stall`) + says so -- says so with a turn count ("same blocker 4 turns running + (history-locked): "). If the blockers genuinely differ AND no + locked verdict is present, says that instead ("4 turns, 4 different + blockers, none resolved") -- flailing is a different diagnosis from a + wall, and the phrase should say which. + + `stall_verdict` is preferred over re-deriving wall-vs-flailing from the + collapsed-reason count: an evaluator that REPHRASES the same blocker + every turn (the normal case) yields a distinct signature per turn, + which read as "flailing" here even when it was really a wall (see + GOAL-HARDENING-DESIGN.md sec 1.5). The verdict is a semantic signal by + construction -- a judge confirmed it, not a string comparison -- so a + locked verdict always wins over the collapsed-reason heuristic below. + Falls back to that heuristic alone when `stall_verdict` is absent (an + older orchestrator that predates the taxonomy). Falls back to `stall_detail`, then bare `reason`, for an older orchestrator that doesn't emit `reasons` at all. """ reasons = data.get("reasons") or [] + verdict = data.get("stall_verdict") if reasons: collapsed = _collapse_consecutive(reasons) continuations = data.get("continuations") @@ -290,9 +315,13 @@ def _stalled_line(data: dict[str, Any]) -> str | None: if isinstance(continuations, int) else sum(count for _, count in collapsed) ) - if len(collapsed) == 1: - blocker, _count = collapsed[0] - return f"same blocker {total_turns} turns running: {blocker}" + if len(collapsed) == 1 or verdict in _LOCKED_VERDICTS: + # A wall -- textually (one collapsed entry) or semantically (a + # locked verdict, regardless of how many distinct reason + # strings accumulated). Show the most recent phrasing. + blocker = collapsed[-1][0] + suffix = f" ({verdict})" if verdict in _LOCKED_VERDICTS else "" + return f"same blocker {total_turns} turns running{suffix}: {blocker}" return ( f"{total_turns} turns, {len(collapsed)} different blockers, none resolved" ) diff --git a/tests/test_goal_progress_hook.py b/tests/test_goal_progress_hook.py index dd08cdb..2d9c72c 100644 --- a/tests/test_goal_progress_hook.py +++ b/tests/test_goal_progress_hook.py @@ -240,6 +240,68 @@ async def test_different_blockers_reads_as_flailing(self, hook): assert "blocked A" not in out assert "blocked B" not in out + @pytest.mark.asyncio + async def test_locked_verdict_reads_as_wall_despite_rephrased_reasons(self, hook): + """The bug this fixes (GOAL-HARDENING-DESIGN.md sec 1.5): an + evaluator that REPHRASES the same blocker every turn -- the normal + case -- previously yielded a distinct signature per turn and read + as "flailing" even though a judge confirmed it's really a wall. A + locked `stall_verdict` must override the collapsed-reason count. + """ + await hook.on_goal_progress( + "orchestrator:goal_progress", + { + "state": "stalled", + "continuations": 3, + "reasons": [ + "blocked: missing the activation code", + "still can't proceed without the activation code", + "the activation code remains the blocker here", + ], + "stall_verdict": "history-locked", + }, + ) + out = _joined(hook) + assert "same blocker 3 turns running" in out + assert "history-locked" in out + assert "flailing" not in out + assert "different blockers" not in out + + @pytest.mark.asyncio + async def test_resolvable_verdict_does_not_force_wall_wording(self, hook): + """A `stall_verdict` of "resolvable" must never itself force wall + wording -- only the three LOCKED verdicts do (see _LOCKED_VERDICTS). + In practice a "stalled" event with a "resolvable" verdict shouldn't + occur (is_stalled implies a locked verdict), but the renderer must + not crash or mis-render defensively if it ever does. + """ + await hook.on_goal_progress( + "orchestrator:goal_progress", + { + "state": "stalled", + "continuations": 4, + "reasons": ["blocked A", "blocked B", "blocked C", "blocked D"], + "stall_verdict": "resolvable", + }, + ) + out = _joined(hook) + assert "4 turns, 4 different blockers, none resolved" in out + + @pytest.mark.asyncio + async def test_missing_verdict_falls_back_to_collapsed_count_heuristic(self, hook): + """An older orchestrator that predates the taxonomy (no + `stall_verdict` key at all) must render exactly as before.""" + await hook.on_goal_progress( + "orchestrator:goal_progress", + { + "state": "stalled", + "continuations": 4, + "reasons": ["blocked A", "blocked B", "blocked C", "blocked D"], + }, + ) + out = _joined(hook) + assert "4 turns, 4 different blockers, none resolved" in out + @pytest.mark.asyncio async def test_falls_back_to_stall_detail_when_no_reasons(self, hook): await hook.on_goal_progress( From 896dc093fc6b9bd7430e7033ba049d688d008cbd Mon Sep 17 00:00:00 2001 From: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> Date: Mon, 3 Aug 2026 07:26:41 -0700 Subject: [PATCH 3/3] feat: ship goalify skill inside CLI as version-locked packaged data MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit goalify composes a /goal stop-condition from the current conversation and lints it against known termination-failure patterns before showing it to the user. It has a hard prerequisite — it is only meaningful when the CLI provides the /goal command — so co-locating it with the CLI satisfies that prerequisite by construction rather than by configuration. The skill lives at amplifier_app_cli/data/skills/goalify/ and is registered by appending a package-relative path in _ensure_default_skills_dirs(). Resolution is by installed package location, never by git URI: a git reference would have co-located the files in the repo while still versioning them independently of the code they depend on, which defeats the purpose. The wheel build declares packages = ["amplifier_app_cli"], so repo-root files are excluded from the distribution entirely. The skill directory is appended in Python rather than declared through a bundle's tool-skills config, because bundle-level skills config replaces rather than appends — the same hazard _ensure_default_skills_dirs was originally written to work around. Verified that workspace and user skills still load alongside the packaged one. The skill body contains only instructions to the invoking agent. Authoring rationale and the lint-rule evidence base were moved to sibling PROVENANCE.md, which load_skill does not read. The lint output table now reports "no known pattern detected" rather than "PASS", because a column of PASS reads as a validation claim the linter cannot support — it detects known patterns from a finite corpus. A repo-root AGENTS.md records the decision rule for what may live in amplifier_app_cli/data/ versus an external bundle: the asset must depend on something the CLI uniquely provides, no non-CLI host would want it, and unconditional triggers belong in the bundle layer while gated ones stay in Python. Assets resolve by package path only, and this location is auto-loaded for every user in every session, so its token budget discipline is stricter than anywhere else. Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- AGENTS.md | 36 ++++ .../data/skills/goalify/PROVENANCE.md | 69 +++++++ .../data/skills/goalify/SKILL.md | 188 ++++++++++++++++++ .../goalify/examples/known-bad-L1-ordering.md | 25 +++ .../examples/known-bad-L2-quantifier.md | 30 +++ amplifier_app_cli/runtime/config.py | 19 +- tests/test_merge_utils.py | 29 ++- 7 files changed, 390 insertions(+), 6 deletions(-) create mode 100644 AGENTS.md create mode 100644 amplifier_app_cli/data/skills/goalify/PROVENANCE.md create mode 100644 amplifier_app_cli/data/skills/goalify/SKILL.md create mode 100644 amplifier_app_cli/data/skills/goalify/examples/known-bad-L1-ordering.md create mode 100644 amplifier_app_cli/data/skills/goalify/examples/known-bad-L2-quantifier.md diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..a7eb107 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,36 @@ +# AGENTS.md — amplifier-app-cli + +## Boundary rule: `amplifier_app_cli/data/` vs. an external bundle + +`amplifier_app_cli/data/` ships inside the CLI's own wheel — anything placed there loads +for every user, every session, version-locked to the installed CLI, with no bundle +composition step in between. That reach is exactly why it must stay small. Before adding +anything here, run it through these three tests, in order: + +1. **Does it depend on something the CLI uniquely provides, that cannot move?** A slash + command, a settings key, a terminal affordance — something with no home outside this + process. If no → it belongs in an external bundle, not here. +2. **Would a non-CLI host ever want it?** If yes → external bundle. The CLI may still + *include* it (compose the bundle), but must not *own* it — ownership belongs wherever + the capability is portable to. +3. **Is the trigger unconditional?** If the asset is gated on settings, an env var, a flag, + or runtime state, the asset itself may still live here, but the compose/injection + *decision* stays in Python (see `runtime/config.py::_ensure_default_skills_dirs` for the + pattern) — never encode conditional loading in a bundle YAML that lives alongside it. + +If the answer to 1 is "no" or the answer to 2 is "yes," it's an external bundle question, +not a `data/` question. + +**Resolution rule:** assets under `amplifier_app_cli/data/` are always resolved by +**package-relative path** (e.g. `Path(__file__).parent.parent / "data" / "..."`), **never** +by git URI. A git URI decouples the asset's version from the installed wheel's version — +defeating the reason for co-locating it here in the first place. If it needs independent +versioning, it isn't a `data/` asset. + +**Token budget:** this location is auto-loaded for every user, every session — its budget +discipline is stricter than anywhere else in the ecosystem. No always-on context files +here without an explicit, named exception recorded in this section. Prefer mechanisms +that load on demand (skills, agent-scoped context) over anything injected unconditionally. + +This section exists to keep `data/` from becoming a junk drawer — re-run the three tests +before adding, not after. diff --git a/amplifier_app_cli/data/skills/goalify/PROVENANCE.md b/amplifier_app_cli/data/skills/goalify/PROVENANCE.md new file mode 100644 index 0000000..efb3a53 --- /dev/null +++ b/amplifier_app_cli/data/skills/goalify/PROVENANCE.md @@ -0,0 +1,69 @@ +# goalify — rule provenance + +Not loaded by `load_skill`. This file exists so the lint rules in `SKILL.md` +can be audited and re-derived. Read it when changing a rule, not when using +the skill. + +## Where the rules come from + +Every BLOCKER traces to an observed `/goal` run that failed to terminate. +That property is the ruleset's credibility. **Do not add a rule without a +named run or a corpus measurement behind it.** + +| Rule | Origin | +|---|---| +| L1 — ordering/provenance constraint | A run whose condition required `PROVEN with evidence you produced yourself`. The evaluator crystallised this into a constraint on the transcript's own past and stated it could not be retroactively repaired. Stalled after 54 turns; unreachable from roughly turn 45. | +| L2 — universal quantifier with exempt members | A run requiring evidence for `all 9 sites` in a uniform structure, where one site could not structurally produce that evidence. Stalled after 24 turns. | +| L3 — elapsed wall-clock requirement | A run whose only available proof standard was real-world use over time. Stalled after 31 turns. | +| L4 — human-in-the-loop dependency | A condition containing `Stop and ask me if you need a decision from me`, which contradicts unattended continuation. 12 turns consumed in 35 seconds. | +| L5 — open enumeration | `all editing features of Publisher` — never terminated. Its sibling run in the same session, same codebase, same day (`until I have a usable app`) achieved on turn 1. | +| L0 — cross-clause consistency | The L2 run above carried a textbook four-verdict exit vocabulary and stalled anyway, because one other sentence silently overrode it. | +| L6 — missing disjunctive exit | Advisory, not blocking. See below. | + +## Why L6 is a WARNING, not a BLOCKER + +A lint regression over 30 scored real runs with known outcomes measured L6 +firing on 37% of all real conditions at a 9% hit rate — the largest single +source of false positives of any rule, consistent across the tuning split, +the held-back split, and the full set. L6 judges exit-clause presence in +isolation and cannot see turn position or residual scope, so it routinely +flagged short finisher-style conditions written late in a long session. + +On that corpus, presence of a disjunctive exit was only weakly correlated +with actual termination — several conditions with strong exit language +stalled anyway, for reasons L1/L2/L0 cover independently. + +Demoting L6 raised measured precision from 20% to 43%, recall unchanged +at 100%. L0 and L1–L5 each fired only 1–2 times in that evaluation — too few +observations to justify changing their classification, so none was changed. + +## How to read the precision number + +The 20%/43% figures were measured on a corpus dominated by **human-authored** +conditions, which terminate about 96% of the time. On a population that +rarely fails, any linter's precision is bounded by arithmetic — a rule that +flags a condition which succeeded anyway counts as a false positive. + +This skill lints **agent-authored** conditions, which in the same corpus +terminated about 60% of the time. Same rules, roughly ten times the base +rate of true positives. **Treat 43% as a floor measured on the easiest +available population, not as the operating precision.** Re-scoring against +the agent-authored subset alone is the outstanding measurement. + +The asymmetry also justifies the operating point: a false positive costs one +in-session rewrite pass; a false negative costs a 24-to-54-turn unrepairable +stall. High recall at moderate precision is the correct trade here. + +## Standing caveats + +- **The corpus ages.** These rules encode `/goal` evaluator semantics as + observed at the time of measurement. If the goal loop's evaluator changes, + nothing will automatically flag that the rules have rotted. +- **Effective sample size is smaller than it looks.** The scored runs came + from roughly 8 distinct sessions; runs within a session share an author, a + project, and phrasing habits. +- **The skill drafts; the human edits.** Automating goal authoring is itself + the thing that raises failure rates (60% vs 96%). The lint is the bet that + it closes that gap, and that bet has not been measured end to end. The + human review step is load-bearing, which is why the skill offers and never + auto-runs. diff --git a/amplifier_app_cli/data/skills/goalify/SKILL.md b/amplifier_app_cli/data/skills/goalify/SKILL.md new file mode 100644 index 0000000..34b6cb0 --- /dev/null +++ b/amplifier_app_cli/data/skills/goalify/SKILL.md @@ -0,0 +1,188 @@ +--- +name: goalify +description: > + Compose a /goal stop-condition from the current conversation and lint it + against known termination-failure patterns before showing it to the user. + Use when the user wants to turn the current task into a /goal loop, asks to + "goalify this", wants a stop condition for autonomous work, says "write a + goal condition", "make this a /goal", "turn this into a goal", or asks for + help wording a condition for /goal. +user-invocable: true +version: 1.1.0 +license: MIT +--- + +Run this procedure yourself, in the current conversation. Do not delegate it +to a sub-agent or forked session — Phase 1 reads the live transcript. + +$ARGUMENTS + +If the user supplied focus text above, use it to scope Phase 1. If empty, +extract from conversation alone — do not ask the user to restate what they +already said. + +--- + +## Phase 1 — Extract + +From the conversation so far, determine: + +- **The target end state.** What does "finished" concretely look like? It + must be a state that can be checked, not an activity that can be performed + indefinitely. ("a usable app" is checkable; "finish building the project" + is not — there is no test for "finished building".) +- **What is already done.** Re-read the transcript for completed sub-tasks, + passing tests, merged changes, or resolved questions. These become + candidates for the KNOWN section (Phase 2) or for narrowing scope + (SCOPE-OUTS). +- **What is explicitly NOT required.** Anything the user has ruled out, + deferred, or said isn't needed. This is the raw material for SCOPE-OUTS. + +If the end state genuinely cannot be determined from context (not merely +effortful to determine), ask one direct question. Otherwise proceed — +guessing and then showing your extraction for correction is faster than +front-loading a question the transcript already answers. + +## Phase 2 — Compose + +Emit a candidate condition using this structure. Every element is required +unless marked optional. + +1. **One-sentence outcome** naming a checkable end state (not an activity). +2. **Disjunctive exit**: the condition must be satisfiable by *either* + reaching the end state *or* conclusively demonstrating it cannot be + reached (naming the blocker). Never phrase a condition with only one exit. +3. **Per-item negative terminal**, if the condition lists multiple items + (tasks, sites, phases, experiments). Each item must be able to resolve to + its own PASS / FAIL / BLOCKED-with-named-reason — a blocker on one item + converts *that item* to a residual; it must not block the whole goal. +4. **SCOPE-OUTS section** — an explicit list of what is *not* required. Write + this by directly converting anything from Phase 1's "not required" list + into a plain negative statement (e.g. "No production soak time required." + / "Uniformity across all N items is NOT the goal."). +5. **KNOWN section (optional)** — facts already established, so the actor + doesn't re-derive them. Label it explicitly as a speed aid: it prevents + wasted turns, it does not by itself prevent stalls, so it never replaces + items 1–4. + +## Phase 3 — Lint + +Check the full composed document against every rule below. Work through all +BLOCKERS first; a document with any BLOCKER triggered is not ready to show. +Then check WARNINGS, which are advisory and do not block presentation. + +**Read the whole document for each check.** Several of these rules are only +detectable by considering the document as one system — a single clause +elsewhere can silently defeat a correct-looking rule everywhere else. Do not +scan for keywords in isolation and stop at the first clean-looking match. + +### BLOCKERS — fix all before presenting + +- **L1 — Ordering/provenance constraint on the transcript's own history.** + Any phrasing that requires evidence to precede, or be produced independent + of, events that already exist in the transcript (e.g. "verify it yourself, + then state what you verified", "proof must precede the claim", "evidence + you produced yourself" applied to something already reported by a + sub-agent or prior turn). This class of requirement is **unrepairable** — + no later turn can change what already happened earlier in the transcript, + so it cannot be fixed by adding more work. If the condition constrains + ordering, it must constrain only *future* actions, never re-litigate what + is already in the history. + +- **L2 — Universal quantifier over a set with possibly-exempt members.** + "all N", "every X", "each of the Y", "uniform/uniformity", "complete + parity", applied to a set, is a blocker **unless** each item individually + carries a negative terminal (see Compose #3) or the condition names which + members are exempt and why. Without one of those, a single member that + cannot structurally produce the required evidence makes the whole + condition permanently unsatisfiable. + +- **L3 — Elapsed wall-clock requirement.** Anything that requires real time + to pass beyond the current session: "production soak", "after N days of + use", "monitor over time", "verify in real-world use". A single session + cannot advance wall-clock time; this can never be satisfied in-session. + +- **L4 — Human-in-the-loop or external-actor dependency mid-loop.** + "stop and ask me if you need a decision", "once a reviewer merges this", + "wait for approval before continuing". This directly conflicts with + unattended continuation — the loop will halt waiting on an event that a + condition-checking loop cannot itself produce. + +- **L5 — Open enumeration.** Scope phrased as an unbounded or unenumerated + set: "all editing features of X", "complete parity with Y", "everything + needed to fully support Z". An evaluator can always name one more item + under this phrasing, so it never terminates. Convert to a closed, named + list, or to a single representative artifact. + +- **L0 — Cross-clause consistency (meta-rule).** *An escape hatch is only as + strong as the strictest other clause in the same document.* After + confirming L1–L5 pass individually and a disjunctive exit exists, re-read + the document once more asking only: **is there any other sentence, anywhere + in the document, that is stricter than the stated exit and would override + it?** A document can have a textbook-perfect exit clause and still be + unsatisfiable because one unrelated sentence elsewhere re-imposes an L1–L5 + style constraint the exit clause doesn't cover. Confirming an exit clause + exists is not sufficient — confirm nothing else in the document is + stricter than it. + +### WARNINGS — advisory, do not block presentation + +- **L6 — Missing disjunctive exit.** The document should state achievement + *or* a way to conclusively end in "not achievable, here is why" (see + Compose #2). Flag and fix its absence where practical, but do not block + presentation on it alone. +- **W1** — Multiple items are listed but not all of them carry their own + negative terminal (some do, some don't). +- **W2** — No clause asking the actor to show evidence inline in the + transcript as it's produced, rather than only asserting a result. +- **W3** — Scope reads like more than one session's worth of work (multi-week + rollout language, coordination across many independent repos/teams, + phased production deployment). +- **W4** — The condition contains a cautionary anecdote or narrative about a + failure mode (e.g. "don't repeat what went wrong last time", "make sure + this doesn't stall like before") rather than a plain instruction. Any such + narrative addressed to the actor is read by the evaluator too, and can + silently become a criterion the evaluator judges against instead of + guidance the actor merely follows. State requirements as plain criteria, + never as stories. + + **This applies to the condition you are composing right now.** Write every + clause as a direct instruction to the actor, never as a story about a past + run. If you catch yourself writing "so that we don't repeat X", rewrite it + as the direct requirement it implies, with no reference to the incident. + +### If a BLOCKER cannot be cleared + +Rewrite and re-check. Allow up to three rewrite passes. If a BLOCKER still +fires after three passes, stop and surface the specific tension to the user +by name (e.g. "the user's own request requires enumerating an open-ended set +— L5 fires no matter how I phrase it; how would you like to bound this?"). +Do not present a condition that still fails a BLOCKER. + +--- + +## Output format + +Always output the condition inside a fenced code block — terminal reflow +will otherwise destroy its multi-line structure. Follow it with the lint +report as a table, then offer (do not auto-run) `/goal`. + +``` + +``` + +| Rule | Result | Note | +|------|--------|------| +| L0 | no known pattern detected | ... | +| L1 | no known pattern detected | ... | +| L2 | no known pattern detected | ... | +| L3 | no known pattern detected | ... | +| L4 | no known pattern detected | ... | +| L5 | no known pattern detected | ... | +| L6, W1–W4 | (list only the ones that fired) | ... | + +A clean table means no *known* failure pattern was detected — not that the +condition is validated. Say so if the user reads it as a guarantee. + +Then: "Pass this to `/goal` to start the loop — want me to run it now, or +would you like to adjust anything first?" diff --git a/amplifier_app_cli/data/skills/goalify/examples/known-bad-L1-ordering.md b/amplifier_app_cli/data/skills/goalify/examples/known-bad-L1-ordering.md new file mode 100644 index 0000000..d151a39 --- /dev/null +++ b/amplifier_app_cli/data/skills/goalify/examples/known-bad-L1-ordering.md @@ -0,0 +1,25 @@ +# Known-bad example — L1 (ordering/provenance constraint) + +Used to sanity-check the lint in `SKILL.md` Phase 3. This condition is +modeled on a real run that stalled for many turns because the ordering +constraint it imposes on the transcript's own history became impossible to +satisfy once evidence had already been reported by a sub-agent. + +## Condition text + +``` +Done when the migration is PROVEN complete, with evidence you produced +yourself. A sub-agent's report is not proof. Verify it yourself, then state +what you verified — or show a named blocker and stop. +``` + +## Expected lint result + +- **L1: FAIL** — "with evidence you produced yourself" combined with "verify + it yourself, then state what you verified" imposes an ordering constraint + on the transcript's own history. If a sub-agent already reported a result + earlier in the transcript, no later turn can retroactively make that + report "verified by you first" — the requirement is unrepairable once the + transcript already contains the sub-agent's report. +- L6 present (disjunctive exit exists: "or show a named blocker and stop"), + but L1 alone should be sufficient to fail this condition. diff --git a/amplifier_app_cli/data/skills/goalify/examples/known-bad-L2-quantifier.md b/amplifier_app_cli/data/skills/goalify/examples/known-bad-L2-quantifier.md new file mode 100644 index 0000000..bce59b0 --- /dev/null +++ b/amplifier_app_cli/data/skills/goalify/examples/known-bad-L2-quantifier.md @@ -0,0 +1,30 @@ +# Known-bad example — L2 / L0 (universal quantifier + cross-clause override) + +Used to sanity-check the lint in `SKILL.md` Phase 3. Modeled on a real run +that stalled because a uniform requirement applied to every member of a set +made the condition permanently unsatisfiable once one member could not +structurally produce the required evidence. + +## Condition text + +``` +Done when all 9 sites are migrated in a uniform structure, verified working, +or proven impossible with a named blocker. +``` + +## Expected lint result + +- **L2: FAIL** — "all 9 sites" + "uniform structure" is a universal + quantifier over a set, with no per-item negative terminal and no named + exemption for a site that cannot physically satisfy "uniform structure." +- **L0: FAIL** — the document has a disjunctive-looking exit ("or proven + impossible with a named blocker"), but that exit applies to the *goal as a + whole*, not per-site. The stricter clause ("uniform structure" across all + 9) is not covered by the escape hatch, because the escape hatch only + fires once — it can't let 8 sites succeed uniformly while 1 is blocked and + still call the set "uniform." Confirming the exit clause exists is not + enough; it does not cover the stricter clause. +- L6 nominally present (a disjunctive-shaped phrase exists) but does not + actually rescue the condition — this is exactly the L0 case: an escape + hatch that reads as satisfied but is overridden by a stricter clause + elsewhere in the same document. diff --git a/amplifier_app_cli/runtime/config.py b/amplifier_app_cli/runtime/config.py index 9d7afa4..17dfdfb 100644 --- a/amplifier_app_cli/runtime/config.py +++ b/amplifier_app_cli/runtime/config.py @@ -6,6 +6,7 @@ import logging import os import re +from pathlib import Path from typing import TYPE_CHECKING from typing import Any @@ -698,7 +699,7 @@ def _ensure_cwd_in_write_paths(tools: list[dict[str, Any]]) -> list[dict[str, An def _ensure_default_skills_dirs(tools: list[dict[str, Any]]) -> list[dict[str, Any]]: - """Ensure workspace and user skill directories are in tool-skills config. + """Ensure workspace, user, and packaged skill directories are in tool-skills config. This is a CLI policy decision: .amplifier/skills/ (workspace) and ~/.amplifier/skills/ (user) follow the same project-first, user-second @@ -706,13 +707,25 @@ def _ensure_default_skills_dirs(tools: list[dict[str, Any]]) -> list[dict[str, A configure explicit remote skill sources, the module's get_default_skills_dirs() fallback is bypassed and workspace skills become invisible. + Also appends the CLI's own packaged skills directory + (amplifier_app_cli/data/skills/), resolved from the installed package's + location on disk -- never a git URI. This keeps packaged skills + version-locked to the installed CLI wheel: a user pinned to CLI v0.1.1 + gets the skill assets that shipped in v0.1.1, not whatever is on a + branch tip. + Args: tools: List of tool configurations Returns: - Tools with workspace and user skill dirs in tool-skills's config.skills + Tools with workspace, user, and packaged skill dirs in tool-skills's config.skills """ - default_paths = [".amplifier/skills", "~/.amplifier/skills"] + packaged_skills_dir = Path(__file__).parent.parent / "data" / "skills" + default_paths = [ + ".amplifier/skills", + "~/.amplifier/skills", + str(packaged_skills_dir), + ] result = [] for tool in tools: diff --git a/tests/test_merge_utils.py b/tests/test_merge_utils.py index f063deb..84f8679 100644 --- a/tests/test_merge_utils.py +++ b/tests/test_merge_utils.py @@ -4,8 +4,11 @@ from amplifier_app_cli.lib.merge_utils import _provider_key, merge_module_lists from amplifier_app_cli.lib.settings import AppSettings, SettingsPaths -from amplifier_app_cli.runtime.config import _ensure_cwd_in_write_paths -from amplifier_app_cli.runtime.config import _ensure_default_skills_dirs +from amplifier_app_cli.runtime import config +from amplifier_app_cli.runtime.config import ( + _ensure_cwd_in_write_paths, + _ensure_default_skills_dirs, +) def _make_settings(tmp_path: Path) -> AppSettings: @@ -368,7 +371,27 @@ def test_default_paths_appended_after_configured(self): ] result = _ensure_default_skills_dirs(tools) skills = result[0]["config"]["skills"] - assert skills == ["url1", "url2", ".amplifier/skills", "~/.amplifier/skills"] + packaged_skills_dir = str( + Path(config.__file__).parent.parent / "data" / "skills" + ) + assert skills == [ + "url1", + "url2", + ".amplifier/skills", + "~/.amplifier/skills", + packaged_skills_dir, + ] + + def test_packaged_skills_dir_is_package_relative(self): + """Packaged skills dir must resolve from the installed package location, + never a git URI -- this is what keeps packaged skills version-locked to + the installed CLI wheel rather than drifting to a branch tip.""" + tools = [{"module": "tool-skills"}] + result = _ensure_default_skills_dirs(tools) + skills = result[0]["config"]["skills"] + expected = str(Path(config.__file__).parent.parent / "data" / "skills") + assert expected in skills + assert (Path(expected) / "goalify" / "SKILL.md").exists() class TestProviderScopeMerge: