feat: playbooks — reusable per-user instruction packs with sharing and import/export#581
feat: playbooks — reusable per-user instruction packs with sharing and import/export#581Viphava280444 wants to merge 26 commits into
Conversation
…eact, schema) Re-integrate the playbook feature's lower-risk layers onto current main: - src/utils/playbook_service.py: PlaybookService CRUD + SKILL.md render/parse + owner resolution + invocation-text helpers (new file) - src/archi/.../tools/playbook_tools.py: Playbook (load) tool, listing middleware, save/update/delete tools, per-request owner ContextVar (new file) - postgres_service_factory.py: lazy playbook_service property - base_react.py: generalized so every agent gets the Playbook tool + listing middleware; _tool_definitions() exposes save/update/delete; legacy tool-name aliases + removed-name handling in _select_tools_from_registry - cms_comp_ops_agent.py: merge base playbook tools via super()._tool_definitions() - init.sql / sql.py / redmine.py: playbooks table + conversations.playbook_name column (kept main's TIMESTAMPTZ), INSERT/SELECT wiring - examples/agents/cms-comp-ops.md: list playbook authoring tools + guidance Trace-fidelity tool-arg tests dropped from the playbook suite: that logic now lives in RunMemory on main (superseded the old base_react static helpers). 123 playbook unit tests pass; full unit suite shows no new failures (remaining errors are pre-existing missing-dep modules: flask/mistune/langchain_community).
Integrate the playbook layer into the refactored app.py: - imports: io/posixpath/zipfile + playbook_service + playbook_tools symbols - ChatRequestContext.playbook_name; clean content is stored/titled while the expanded command-block+body feeds the agent via history (agent_content) - ChatWrapper._ensure_playbook_schema (idempotent migration for existing volumes; team->public normalization) called at startup; _last_user_playbook_name - insert_conversation + the A/B user-store + assistant-store now write the 10th SQL_INSERT_CONVO column (playbook_name on user turns, NULL on assistant rows) - _prepare_chat_context: /name invocation expansion + regenerate re-apply - conversation-load mapping reads playbook_name at row[6] (main keeps model_used at row[5]; appended, not replaced) - FlaskAppWrapper: _resolve_playbook_owner / _playbook_svc / _stage_playbook_for_request + REST handlers (list/get/create/update/delete/ export/import) + _parse_playbook_upload; 7 routes registered - both chat endpoints stage owner+pending playbook before the pipeline reads them No new unit-test regressions (app.py needs flask/mistune to import; verified via ast parse + the importable suite: 176 passed).
…ort)
Port the playbook frontend onto main's chat.js/index.html/chat.css additively,
preserving main's A/B and trace features:
- CONFIG.ENDPOINTS.PLAYBOOKS + API playbook CRUD/export/import methods
- PlaybookMenu object: /name slash autocomplete over the user's + public playbooks
- Playbooks panel modal (list/create/edit/delete/share private|public, export zip,
import zip/SKILL.md) wired to the REST API
- playbook chip on /name-invoked user turns (live + on reload via msg.playbook_name)
- playbook_name threaded into both send paths (stream + A/B compare) as snake_case
- bare /name invokes a self-contained playbook ("Run the <name> playbook.")
Adapted to main where it diverged: kept main's _readNDJSON reader; threaded the
playbook through streamABComparison (main's dedicated A/B endpoint) instead of the
reference's streamABResponse; omitted the reference's regenerate button (main uses
like/dislike/comment); added position:relative to .input-container so the menu anchors.
node --check passes; index.html div balance 76/76; all JS-referenced DOM IDs/CSS
classes verified present.
tests/smoke/init-test.sql mirrors the production schema for the smoke stack; add the playbooks table + conversations.playbook_name so the playbook smoke test (tests/smoke/test_playbooks_integration.py) has its schema.
The panel methods (openPlaybooksPanel/closePlaybooksPanel/loadPlaybooksPanel/ setPlaybookEditorReadOnly/savePlaybookFromPanel) were appended after the Chat object's closing brace, inside window.__ARCHI_PLAYWRIGHT__ — so Chat.openPlaybooksPanel was undefined (the Playbooks button threw 'is not a function') and their `this` was wrong. Move them into Chat. Caught by live Playwright testing. Verified end-to-end on the live stack: panel lists playbooks, /name slash menu autocompletes, a /name invocation renders the ⚡ chip, injects the body (agent replied with the playbook-dictated text), persists playbook_name to the DB, and the chip re-renders on conversation reload.
37adbe9 to
0623d36
Compare
langgraph 1.0.2 allows langgraph-prebuilt >=1.0.2,<1.1.0, so a fresh install resolves the latest (1.0.10), which imports `ExecutionInfo` from langgraph.runtime — a symbol 1.0.2 doesn't provide. That breaks `from langchain.tools import tool` (the whole agent-tools stack), which surfaced as the CI unit-tests failure once the playbook tools were imported. 1.0.8 is the known-good match (it's what the running deployment image uses with the full agent/A-B/trace/MCP feature set).
12412fe to
1873e04
Compare
… 500 An exhaustive adversarial test pass found a client-controllable 500: a NUL byte in the playbook body — or in client_id (used as owner_id) — reached the psycopg2 INSERT (Postgres TEXT cannot store NUL), raised ValueError, and was swallowed by the generic except into a blanket 'Internal server error' 500. - _validate(): screen body for NUL (only NUL — newlines/tabs stay valid in a multi-line markdown body, unlike the single-line description which rejects all control chars). - resolve_playbook_owner(): reject a client_id containing NUL at the owner chokepoint, so every endpoint (read + write) returns a clean 400. Both now return 400 with a clear message instead of 500 (verified live). +3 regression tests (NUL body rejected, multi-line body still accepted, NUL client_id rejected).
AntuBattle
left a comment
There was a problem hiding this comment.
First review.
The implementation looks fine, left comments where I thought we could simplify or improve the implementation, or where I had doubts.
Some general considerations:
-
Do we want to couple playbooks to
BaseReActAgent? This means we cannot opt-out of them for any other service (for example, the redmine service ended up using them and instantiating thePlaybookServiceeven though it probably does not need them? If they are a feature of the chat app only, then they should be opt-in forCompOpsAgentvia some sort of extension or inheritance, or at least confined to theCompOpsAgentsubclass only. Otherwise we can leave as is. -
The implementation currently does not work with A/B testing because of
ContextVars. They reset by default within theThreadexecution space used by A/B testing execution flow. So we might want to look into that.
...
| humanfriendly==10.0 | ||
| croniter==2.0.5 | ||
| langgraph==1.0.2 | ||
| langgraph-prebuilt==1.0.8 |
There was a problem hiding this comment.
Is this needed for the playbooks? Or was it some other fix?
There was a problem hiding this comment.
It's not a playbook thing, it's a version skew. I pinned it to the same version we have in our current public base image. The previous commit just showed the bug when CI runs, it reloads the requirements and gets a langgraph-prebuilt higher than 1.0.8, higher than the one in our base image, and found out it breaks. So we need to pin it to this version to avoid breaking the other services.
a140732 to
ebb5ed4
Compare
Track per-turn playbook use in a dedicated conversation_playbook_turns side table (chat-app only) instead of a conversations column, leaving the shared SQL_INSERT_CONVO and other services untouched; extract a SupportsPlaybooks mixin so BaseReActAgent stays playbook-free (only CMSCompOpsAgent opts in); create the schema idempotently on boot; return 4xx (not 500) for non-object JSON request bodies; and sharpen the save_playbook authoring guidance.
Unit tests for PlaybookService (validation, SSO owner resolution, render/parse SKILL.md) and the playbook tools + authoring guidance. The integration (Postgres :5439) and UI (Playwright) suites are kept local-only (gitignored) since they do not run in the unit-tests CI job.
ebb5ed4 to
c655b39
Compare
Stage the playbook on the A/B request (chip + body via the shared history) and carry the owner ContextVar into each arm worker thread (it does not cross a threading.Thread boundary). Avoids copy_context so RBAC/role-prompt behavior in the arms stays unchanged. Adds unit guards for the propagation contract.
'team' was a pre-rename artifact of this branch — it always collapsed to 'public', never shipped on main, and was flagged in review (I4/I6). Removes the _normalize_visibility alias + its callers, the SKILL.md/import 'team' handling, and the now-dead boot migration; 'team' is now simply an invalid visibility rejected by _validate. Drops the 5 unit tests that only exercised the alias.
…esolution (I1) A display name isn't guaranteed unique, so it's a poor owner key. resolve_playbook_owner now resolves the owner as email > sub > id (all unique/immutable); if none is present the request fails closed instead of using a mutable display name. In practice 'name' was already unreachable (session id = the mandatory OIDC sub), so this only hardens the broken-IdP case.
…all/stale-reuse) when a playbook tool is unavailable
…een bodies can't be saved Adds a confirmed=false preview to save_playbook (mirrors delete_playbook) plus a public PlaybookService.validate wrapper, and clarifies the authoring docstring: $ARGUMENTS is one flat string, and draft first instead of an upfront questionnaire.
…-only confabulation)
fcefcd6 to
ddd896b
Compare
…ns, persist session key - resolve_playbook_owner: prefer the OIDC subject (session 'id') over email, so a user's playbooks key on the same id as users.id / conversation_metadata.user_id (email is mutable and diverged from those FK-linked tables). - UserService.record_login: bump login_count + set last_login_at on SSO login; the callback upserted the user but never tracked logins, so login_count stayed 0 / last_login_at NULL. - read_or_create_persistent_secret: persist an auto-generated Flask session key in the DATA_PATH volume so signed sessions survive a restart instead of logging everyone out; an explicit FLASK_UPLOADER_APP_SECRET_KEY still takes precedence.
… a plain turn
SQL_LAST_PLAYBOOK_NAME_FOR_SENDER used an INNER JOIN, so regenerating a plain
message returned the most recent turn that had ANY playbook (e.g. an earlier
/foo) instead of the playbook of the turn actually being regenerated. That
body was then injected into the plain turn, forcing the answer through an
unrelated playbook's template.
Use a LEFT JOIN so the query returns the newest sender turn's playbook_name,
or NULL when that turn had none — matching the _last_user_playbook_name
docstring ("most recent sender turn, or None"). The refresh path already
injects nothing when the name is None, so no app.py change is needed.
Public playbooks become opt-in per user, fixing the unbounded public-listing that bloated every model prompt and eventually exhausted the context window (correctness bug #1). A user's always-in-context listing and their /name + Playbook-tool invocation are scoped to their own playbooks plus the public ones they explicitly enable. - Schema: user_enabled_playbooks(user_id, playbook_id) opt-in join table. - Service: enable/disable/list_enabled_playbook_ids (with a visibility guard so a user can't enable another user's private playbook, and connection rollback on the guard/error paths); list_listing_playbooks (own + enabled-public) for the model prompt; resolve_invokable_playbook (by-name invocation gate). - Gates: format_playbook_listing, the model Playbook tool, the /name slash path, and the refresh/regenerate path all resolve only own + enabled-public. - REST: is_enabled on the playbooks list payload; enable/disable endpoints. - UI: a playbooks panel with My playbooks / Public-to-add tabs, a name + description search with live match counts, Add/Remove, and an "add then run" prompt when a typed /name is an unadded public playbook; the / menu lists only the user's own + enabled playbooks.
…s in a fixed footer
… + side-table SQL into PlaybookService - playbook_routes.py: the 9 /api/playbooks* handlers and the zip import/export parsing move out of app.py into a Blueprint registered via register_playbooks(app, ...), mirroring the service_alerts convention (blanket before_request auth probe; owner resolution and the pooled service factory are injected from the wrapper at registration). - PlaybookService gains ensure_schema() (idempotent schema migration incl. the one-time legacy conversations.playbook_name copy), record_playbook_turn() and last_playbook_name_for_sender(). Service methods raise; best-effort policy stays at the chat-flow call sites. - service_chat.main() runs ensure_schema() once at service startup before the Flask app is built; ChatWrapper.__init__ no longer executes DDL. - app.py: ~590 lines of playbook plumbing removed; the chat-flow integration (_stage_playbook_for_request, per-turn tracking call sites) stays put.
…utton styling - The panel now lives as a "Playbooks" section in the Settings modal (new nav item + section); the standalone modal and the agent-dropdown entry are gone. The /name slash-menu invocation flow is untouched. - The list is regrouped per review: tabs "Active (N)" / "Add from public (N)", with the Active tab split under "Yours" and "Added from public" headers; action labels tighten to Edit/Delete and View/Remove/Add. - Footer and row action buttons follow the app's button language (accent primary, tertiary secondaries) instead of native browser defaults.
src/cli/templates/init.sql gained the user_enabled_playbooks table with the per-user public-playbook opt-in (7d8116d), but the smoke-test fixture was missed. tests/smoke/docker-compose.integration.yaml seeds its database from this file, so a fresh clone got a schema without the table the opt-in code queries.
…hardening batch The Blueprint refactor left ChatWrapper calling self._playbook_svc() while the accessor lived only on FlaskAppWrapper: every chat-flow playbook call raised AttributeError, silently swallowed by the best-effort excepts - turn recording, regenerate re-apply and A/B turn recording all no-oped (chips vanished on reload). A shared _pooled_playbook_service() now backs a real accessor on both classes, and the store paths are deduped into _insert_conversation_rows + _record_playbook_turn_best_effort so one seam serves every path. Verified live: the /name turn writes the side-table row, the chip survives a real reload, and regenerate re-applies the playbook from the DB. Also, from the same review: - load_conversation falls back to a chip-less query (new SQL_QUERY_CONVO_WITH_FEEDBACK_NO_PLAYBOOKS) instead of 500ing every load when the side-table migration failed, and closes its connection in a finally; a failed boot-time ensure_schema now logs at ERROR. - resolve_playbook_owner rejects non-string client_id as a 400-path error; enable/disable coerce non-dict JSON bodies (same guard as delete). - parse_playbook_md rejects non-string frontmatter values loudly instead of silently renaming playbooks named no/off/false/0 (YAML 1.1 coercion). - SupportsPlaybooks fails fast at class definition when a terminal base precedes it in the MRO (the whole feature silently vanished); cooperative super()-calling mixins are exempt. - playbook_routes keeps per-app state on app.config instead of module globals - a second FlaskAppWrapper used to crash blueprint setup and repoint the first app's routes; the auth probe registers once at module level. - GET /api/playbooks no longer fetches bodies it discards. - uploader_app persists its auto-generated session key (restarts logged every user out) under its own key file - chat shares the data volume and, historically, the secret name, and the two apps must not sign with one key. - update/delete tools share one owned-playbook fallback ladder; chat.js playbook editor fields live behind one selector map. - tests/unit/conftest.py centralizes the guarded heavy-dep test stubs (find_spec-based), replacing three per-file copies.
…isted session key The nine /api/playbooks* endpoints and the Settings -> Playbooks panel shipped without docs; add them to the API reference and user guide. Ratify the persisted auto-generated Flask session key in troubleshooting: sessions now survive restarts by design, and rotating the configured secret (or deleting the persisted key file) is the lever to invalidate them.
What this adds
Playbooks are reusable instruction packs, one set per user. You save a procedure once — like "how we triage held jobs" or "our end-of-day report format" — and the agent follows it every time instead of you re-typing it.
Two ways they run:
summarize-ticketplaybook by itself./name argsto force a specific one for that turn, through a/autocomplete menu.The description is what the agent matches on, so it's written to say what the playbook does and when to use it.
How it works
/name. So 100 playbooks cost a short menu per turn, not 100 bodies.client_idin anonymous mode. Reads and writes are owner-scoped in SQL, and other users' ids never leak in anonymous mode. Bodies from other users are treated as untrusted when loaded.<name>/SKILL.mdfiles (frontmatter + markdown). Import takes that zip, a single.md, or an old JSON backup. Imports always land private, and one bad file doesn't break the batch.Sharing (opt-in)
A playbook is private by default, or public. Public ones are visible to everyone and read-only for non-owners — but they don't show up in your agent automatically. You opt in: enable the public playbooks you want, and only then do they join your agent's list and become usable (auto or
/name). There's a per-user cap on how many you can enable, and your own playbook shadows a public one with the same name.Storage
Three Postgres tables, all created or upgraded by an idempotent migration on startup:
playbooks(the packs),conversation_playbook_turns(which playbook ran on which message — drives the chip and re-apply on regenerate), anduser_enabled_playbooks(the per-user opt-in).Testing
214 unit tests: the tools, the system-prompt listing, SKILL.md round-trips, validation, visibility + opt-in, the save-confirm and refuse-don't-fabricate behavior, and SSO vs anonymous ownership. Plus a manual run against a live deployment — 44 scenarios (REST, security, concurrency, a full browser journey, restart persistence): 43 passed, 1 known pre-existing issue, no security findings.