diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 00000000..a7eb1074 --- /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 00000000..efb3a531 --- /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 00000000..34b6cb02 --- /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 00000000..d151a39b --- /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 00000000..bce59b0e --- /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/goal_progress_hook.py b/amplifier_app_cli/goal_progress_hook.py index 81835a56..a2d0d5b9 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 @@ -86,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. @@ -198,13 +221,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,32 +243,70 @@ 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. - 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") @@ -246,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/amplifier_app_cli/runtime/config.py b/amplifier_app_cli/runtime/config.py index 9d7afa4a..17dfdfba 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_goal_progress_hook.py b/tests/test_goal_progress_hook.py index 1069f777..2d9c72c0 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) @@ -236,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( @@ -576,3 +642,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 diff --git a/tests/test_merge_utils.py b/tests/test_merge_utils.py index f063deb2..84f86799 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: