701 Commits

This Branch

This Branch

All Branches

Author	SHA1	Message	Date
xarmian	3f6bcc0ff2	feat(oauth): per-connection state tables + dual-read introspection gate (TASK-1520) (#581) ... * feat(oauth): per-connection state tables + dual-read introspection gate (TASK-1520) Phase A foundation for PLAN-1519 / IDEA-1517's per-OAuth-connection state overhaul. Promotes the consent-time workspace allow-list out of session.Extra (per-token, re-minted on every refresh-token rotation) into dedicated tables keyed by request_id (the grant chain identifier preserved across rotations). Schema (SQLite migration 059 + Postgres migration 038): - oauth_connections: one row per grant chain with name + three scope flags (may_create_workspaces, all_current_workspaces, include_future_workspaces). - oauth_connection_workspaces: mutable allow-list join table; PK on (request_id, workspace_id); FK ON DELETE CASCADE; added_by audit column. Store (internal/store/oauth_connections.go): Create/Get/Rename/SetScopeFlags/ Add+Remove+IsAllowed/Delete CRUD. GetOAuthConnectionAccess is the hot-path projection — one PK lookup + one indexed join when the wildcard flag is off, nothing else when it's on. Dual-read gate (internal/server/middleware_mcp_auth.go): OR-merges the legacy session.Extra allow-list with the new-table projection. A workspace is allowed iff either source allows it; either source's wildcard makes the gate unrestricted. New tables stay empty until Phase C writes the consent screen, so the dual-read is a no-op until then — and existing OAuth grants keep working unchanged through the Extra path. I/O errors on the new path fall back to the Extra path so a transient outage of the new tables can't regress existing connections. Tests: - 10 store tests cover CRUD, FK cascade, wildcard short-circuit, sorted slug projection, idempotent add/remove, ErrOAuthConnectionNotFound on missing rows. - 11-case table-driven test on mergeAllowedWorkspaces directly verifies PLAN-1519's acceptance criterion: "token with allow-list in session.Extra still passes; token with empty session.Extra but row in oauth_connection_workspaces also passes." Plus wildcard precedence, union dedup, and fail-closed-on-empty-scope. - BenchmarkMergeAllowedWorkspaces measures policy-function overhead on the hot path (the store-side lookup is the other half of the dual-read cost). Parent: PLAN-1519. * fix(oauth): fail-closed on connection lookup error per Codex review (round 1) PR #581 Codex review round 1 caught two issues: 1. middleware_mcp_auth.go: GetOAuthConnectionAccess errors fell through to "no connection" + nil allow-list = unrestricted. Post-Phase-C (when the new tables are authoritative and session.Extra is empty), a DB read error on a scoped token would silently grant access to every workspace the user belongs to. Now fails closed with a 401 matching the IntrospectToken storage-error policy, increments MCPAuthzDenialsTotal{connection_lookup_error} for ops visibility. 2. oauth_connections.go: CreateOAuthConnection's docstring claimed "scope flags default ON if not supplied," but the method writes the three Go bools verbatim — and Go zero-values for bool are false, not true. The schema-level DEFAULT TRUE is unreachable through this path. Docstring updated to clarify that defaults live at the form-rendering layer; the store is a faithful pass-through. Parent: PLAN-1519. * test(oauth): add store-side bench for GetOAuthConnectionAccess per Codex review (round 2) PR #581 Codex review round 2 flagged the docstring references to bench_oauth_connections_test.go pointing at a file that didn't exist — only the in-memory mergeAllowedWorkspaces bench was wired. Add the store-side bench so the documented file is real and PLAN-1519 Phase A's "Hot-path benchmark: dual-read overhead measured and documented" acceptance bullet is satisfied end-to-end. Three shapes covered: Wildcard (PK lookup, join short-circuited), Explicit (PK + indexed scan + small workspaces join), and NoRow (the dominant Phase-A path until Phase C wires the write path). Local numbers (Ryzen 3 5300U, SQLite WAL): 12µs/12µs/37µs respectively — all comfortably sub-millisecond. Parent: PLAN-1519.	2026-05-18 00:03:20 -04:00
xarmian	2a1a00e385	test,docs: blank+onboard+needs_onboarding integration test + CLAUDE.md update (TASK-1507,1508) (#580) ... PLAN-1496's final consolidation pair, shipping together because both are small cleanup passes that close the plan out. TASK-1507 (tests): Most of the test coverage required by this task was already added incrementally in the prior PRs that built each surface: - Blank template (4 focused tests, PR #575): TestSeedFromBlankTemplate, TestBlankTemplateShape, TestBlankTemplateExcludesSoftwareCollections, TestBlankTemplateUsesMinimalVocabularies, TestBlankTemplateAppearsInPicker - Onboard auto-seed (PR #576): TestSeedFromTemplateAlwaysIncludesOnboardPlaybook (walks all six templates), TestSeedWithEmptyTemplateNameSkipsOnboard (locks the empty-templateName escape-hatch invariant), TestOnboardPlaybook_Contract (invocation_slug, trigger, mode-enum, ADAPT-DON'T-CURATE rule in the body) - needs_onboarding (PR #578): TestBootstrapNeedsOnboardingFlag (lifecycle: fresh → user item → flag flips), TestBootstrapNeedsOnboardingIgnoresTemplateSeeds (template seeds don't count) - Retired-pattern updates (PR #577): TestSoftwareTemplatesShipNoSeedItems inverse invariant; TestDashboardOnboardingSeed_NilForAllTemplates collapsed from three IDEA-1/BACK-1/FEAT-1 tests. This commit adds ONE integration smoke test that ties the three subsystems together at the bootstrap layer: - TestBootstrapBlankWorkspaceOnboardReady creates a blank-template workspace, fetches bootstrap, and asserts: needs_onboarding=true (nudge fires) AND the onboard playbook is in bootstrap.playbooks AND its status is "active" AND its trigger is "manual" (in the blank template's seeded vocabulary). If any one of the three pieces regresses silently, the integration breaks and this test catches it before /pad onboard stops dispatching on day one. TASK-1508 (docs): - CLAUDE.md "Data Model / Templates" section: added Blank under a new "Custom" category bullet pointing at the new Onboarding section; called out the PLAN-1496 retirement of the IDEA-1 / BACK-1 / FEAT-1 first-person seed pattern; updated design history reference to include PLAN-1496. - CLAUDE.md "API" section: added the /api/v1/workspaces/{ws}/agent/bootstrap endpoint with a note about the needs_onboarding flag (was previously documented only inline in the Playbooks section). - CLAUDE.md: new top-level "Onboarding" section between Playbooks and Testing. Covers: auto-seeded everywhere, surface-agnostic body, adaptation posture (library entries are starting points), the three TASK-1510/1511/1512 mutation primitives, the needs_onboarding bootstrap flag + skill nudge, the four retired surfaces (pad onboard cobra, OnboardingPrimaryRef, *OnboardingItems generators, standalone skill workflow section), and a code map. Verification: - go test ./...: clean (full suite passes including the new integration test) - make lint: 0 issues Parent: PLAN-1496.	2026-05-17 16:53:29 -04:00
xarmian	403cf6b149	feat(web): Blank-first picker + post-create /pad onboard guidance (TASK-1506) (#579) ... * feat(web): surface Blank as featured card + post-create /pad onboard guidance (TASK-1506) The console workspace-creation page (web/src/routes/console/new) now makes the agent-driven flow first-class on both the picker and the post-create screen: Picker: - Blank template renders as a leading "featured" card above the grouped category list, with an "Agent-driven" badge so its positioning is intentional rather than visually accidental. - The grouped category iteration filters Blank out so it doesn't render twice. Older server builds that don't ship Blank degrade silently — the featured card just doesn't appear. Post-create success state: - Replaces the pre-task immediate goto-redirect that took users straight to the dashboard, hiding the canonical /pad onboard entry point. - Branches on template: - Blank: prominent onboard card with a heading, a copy/paste snippet (<pre><code>/pad onboard</code></pre>), and a help line pointing at the agent-connection docs. - Non-blank: subtle one-paragraph affordance — "want to customize further? run /pad onboard." - "Open <workspace>" CTA renders as an <a> styled like .submit-btn so the success screen is one click from the workspace dashboard. Implementation notes: - Script uses $state for createdWorkspace + createdTemplate to swap the layout, $derived for blankTemplate / grouped / username, $derived.by for workspaceUrl (multi-statement derivation). - 'goto' import removed (no longer needed — the swap replaces the redirect). Verification: - svelte-autofixer: 0 issues, 0 suggestions. - svelte-check: 0 errors (pre-existing warnings in unrelated files). - npm run build: clean. - go test ./... + golangci-lint: clean (Go side untouched). Parent: PLAN-1496. * fix(web): repoint Connect-MCP link to existing getpad.dev docs (Codex round 1) P2 finding on PR #579: /docs/agents is not a route in this app (no /docs prefix exists; only the marketing site at getpad.dev owns the docs surface). Clicking the link from the success state would land on the app's 404 page. Repointed to https://getpad.dev/docs — the same external URL the +error.svelte page already uses for 'Browse docs' (known-good and known-stable). Added target/rel attrs matching the existing external getpad.dev links elsewhere in the codebase. Parent: PLAN-1496, addressing Codex round 1 on PR #579 / TASK-1506.	2026-05-17 16:23:05 -04:00
xarmian	96a32aa39d	feat(bootstrap,skill): add needs_onboarding flag + retire legacy Onboarding workflow (TASK-1504,1505) (#578) ... PLAN-1496's bootstrap-signal + skill-cleanup pair, shipped together because TASK-1505's nudge rendering depends on TASK-1504's bootstrap field. TASK-1504 (bootstrap: needs_onboarding): - internal/store/items.go: new WorkspaceHasUserCreatedItems(workspaceID) store method. Backed by SELECT EXISTS with the predicate `source != 'template'` — defined as the inverse of template seeding rather than enumerating user-side source values, so new attribution surfaces (mcp, api, future) count automatically. - internal/server/handlers_bootstrap.go: AgentBootstrap struct gets the NeedsOnboarding bool field (always emitted — not omitempty, since the agent reads it on every /pad invocation). BuildAgentBootstrap computes it via the new store method. On query error the flag falls back to false (safe default: don't nag). - Visibility filtering deliberately omitted — needs_onboarding is a workspace-level state signal, not a per-user view. Two members reading bootstrap concurrently should see the same answer. - Two focused tests: - TestBootstrapNeedsOnboardingFlag walks the lifecycle (fresh workspace → create user item → flag flips). - TestBootstrapNeedsOnboardingIgnoresTemplateSeeds locks the template-seeds-don't-count invariant on the startup template, which ships seeded conventions, playbooks, and the onboard playbook itself. TASK-1505 (skill update): - skills/pad/SKILL.md: - Context Loading section: new bullet documenting needs_onboarding with the exact nudge wording the agent should render when true, plus the "don't nag past first user item" + "respect prior decline" rules. - Onboarding workflow section: deleted (~30 lines). Replaced with a one-paragraph pointer at the /pad onboard playbook. The skill is the dispatcher; the playbook body is the script. - Routing entry under "set up my workspace": simplified from the bloated PR #577 round-3/4 text into a clean two-bullet form (canonical phrasing + legacy IDEA-1 phrasing both → /pad onboard). - internal/mcp/prompts_data.go: the pad_onboard MCP prompt body was duplicating the same step-by-step script the SKILL.md section carried. Replaced with the same dispatch-to-playbook pointer. internal/mcp/prompts_test.go: TestPromptsLockstep_CoreCommands fragments updated to assert the new dispatch fragments (`pad playbook list`, `pad playbook show onboard`). Parent: PLAN-1496.	2026-05-17 13:57:12 -04:00
xarmian	0930743304	feat: retire IDEA-1 seed pattern + 'pad onboard' cobra + surface blank in init (TASK-1501/1502/1503) (#577) ... * feat: retire IDEA-1 seed pattern + 'pad onboard' cobra + surface blank in init (TASK-1501,1502,1503) PLAN-1496's legacy-onboarding teardown: TASK-1501 (remove seed items + update banner): - internal/collections/templates_onboarding.go (and the _product/_scrum siblings) deleted — these generated the IDEA-1/PLAN-2/TASK-3/DOC-4 + BACK-1/SPRINT-2/BUG-3/DOC-4 + FEAT-1/FB-2/ROAD-3/DOC-4 first-person seeds. The /pad onboard playbook (TASK-1499 / TASK-1500) is the replacement. - startup/scrum/product templates: SeedItems lines removed. - post-init banner in printOnboardingHints: now points at "/pad onboard" in one line, then web UI link, then dashboard hint. The "use pad to get IDEA-1 / BACK-1 / FEAT-1" branch is gone. TASK-1502 (retire cobra + OnboardingPrimaryRef plumbing): - OnboardingPrimaryRef struct field on WorkspaceTemplate removed. The dashboard's banner auto-discovers seeds via item_number=1 + source="template" + created_by="system", so the field was redundant even before retirement. - onboardingPrimaryRef() helper in cmd/pad/main.go removed. - 'pad onboard' Cobra subcommand removed (~160 lines). It scanned the project directory for build/test/CI markers and seeded library conventions — useful behavior but CLI-only, unreachable from MCP-only agents. The /pad onboard PLAYBOOK now covers it. - internal/cli/detect.go and workspace_context_detect.go stay; still used by the web-side workspace-context save path. TASK-1503 (Blank in interactive picker): - The picker already surfaces Blank because templates_picker.go iterates GroupTemplatesByCategory, and the IDEA-1479 Blank template entry lives in CategoryCustom. Verified the output renders correctly with the TASK-1498 description + icon update. - 'pad workspace init --help' Long now mentions Blank explicitly + points users at /pad onboard. Helps discoverability without restructuring the picker. Test changes (delete or rewrite tests that exercised the retired pattern): - internal/collections/templates_test.go: six tests deleted (StartupOnboardingItemsOrderAndShape, ScrumOnboardingItemsOrderAndShape, ProductOnboardingItemsOrderAndShape, Startup/ScrumProduct/TemplatesDeclareOnboardingPrimaryRef). New TestSoftwareTemplatesShipNoSeedItems replaces them with the inverse invariant: software templates ship zero seed items. - internal/server/handlers_dashboard_test.go: three IDEA-1/BACK-1/FEAT-1 expectation tests collapsed into TestDashboardOnboardingSeed_NilForAllTemplates, which asserts the auto-discovery finds no seed because seeds no longer ship. (Hiring + EmptyWorkspace tests untouched — they already expect nil for unrelated reasons.) - internal/store/items_test.go: TestSeedCollectionsFromTemplate{Startup,Scrum,Product}RefSequence and TestOnboardingFlow_FullWalkthrough_{Startup,Scrum,Product} deleted; these locked the IDEA-1 ref-sequence + walkthrough behavior. Unused helpers (findItemByTitle, extractStatus, safeFields, setItemStatus, countItemsInCollection) deleted alongside them. - internal/mcp/resources_test.go: TestReadItem_PreservesIDEAOneOnboardingBodyVerbatim → TestReadItem_PreservesBodyVerbatim. Property is the same (resource pipeline doesn't mangle markdown), but the fixture is now synthetic markdown instead of the IDEA-1 seed. Note: handlers_dashboard.go still has the auto-discovery code path (onboardingPrimaryCollectionSlugs map + the loop that probes for item_number=1 + source="template"). It's now dead code — no item will ever match the criteria after this PR. Left in place for a follow-up cleanup pass to keep this PR focused. Parent: PLAN-1496. * docs: replace 'pad workspace onboard' references with /pad onboard (Codex round 1) P2 finding on PR #577: README + CLAUDE.md still advertise the 'pad workspace onboard' subcommand in four places (README §Onboard agents to a new codebase, README §3 Teach your agents the rules, README CLI Reference, CLAUDE.md CLI). After this branch lands, those instructions return "unknown command." Replaced each with guidance pointing at /pad onboard (the playbook, auto-seeded into every workspace). The library-list commands still work and stay where they are. Parent: PLAN-1496. * docs: replace 'use pad to get IDEA-1' guidance with /pad onboard (Codex round 2) P1 finding on PR #577: README.md:33-39 and CLAUDE.md:111-117 still told users to 'use pad to get IDEA-1' after the post-init banner. Since this branch deletes templates_onboarding.go and stops seeding IDEA-1/PLAN-2/TASK-3/DOC-4, the quickstart instructions in both top-level docs pointed at items that no longer exist. Replaced each with /pad onboard guidance (the playbook is auto-seeded into every new workspace by TASK-1500). CLAUDE.md's CLI reference gets a one-line historical note explaining the pre-PLAN-1496 IDEA-1 pattern so readers reviewing older code/blame have context. Parent: PLAN-1496. * docs(skill): retire 'use pad to get IDEA-1' guidance in agent skill (Codex round 3) P1 finding on PR #577: skills/pad/SKILL.md:175 still taught agents that '"use pad to get IDEA-1"' should dispatch to 'pad item show IDEA-1'. This branch deletes the seed items, so any agent following the shipped skill in a fresh workspace would try to fetch a missing ref instead of running /pad onboard. Updated the routing entry to dispatch the legacy phrasing (kept as a recognized intent so older docs/conversations still work) to the /pad onboard playbook. Explicit "do NOT try to fetch IDEA-1 directly" to short-circuit the previously-trained behavior. A broader skill cleanup — removing the standalone Onboarding workflow section and adding the bootstrap nudge rendering — is TASK-1505's scope. This PR's update is the minimal change needed to unbreak the agent-facing routing. Parent: PLAN-1496. * docs(skill): add library-activation caveat to onboard routing entry (round 4) P2 finding on PR #577: the routing entry said /pad onboard is 'always invokable because every workspace auto-seeds it.' True for newly-created workspaces, but pre-existing workspaces (created before PLAN-1496 lands) won't have it. Auto-upgrade is intentionally not wired into SeedCollectionsFromTemplate for empty-template-name paths. Mirrored the same activation-fallback caveat /pad plan and /pad decompose carry: 'activate via library if the bootstrap's playbooks array lacks invocation_slug=onboard, status=active.' Parent: PLAN-1496.	2026-05-17 13:40:15 -04:00
xarmian	507793e565	feat(playbooks): author canonical /pad onboard library playbook (TASK-1499) (#576) ... * feat(playbooks): author canonical /pad onboard library playbook (TASK-1499) The fourth invokable library playbook (alongside ship/plan/decompose). This is the workspace bootstrap interview the agent runs to turn a freshly-created workspace into one whose collections, conventions, playbooks, and roles actually match the user's project. Files: - internal/collections/playbook_library_onboard.go (new): - onboardPlaybookBody — surface-agnostic instruction set teaching the agent to ADAPT seeded artifacts, not curate from the library. Mode-aware: build (blank workspace), audit (templated workspace), revisit (already-onboarded), defaults (escape hatch). The body explicitly tells the agent to use pad_item/pad_collection/pad_role MCP actions OR pad CLI — never assumes a shell. Lean on the TASK-1510/1511/1512 mutation primitives shipped earlier in PLAN-1496. - onboardPlaybookArguments — mode (enum), defaults (flag), skip-codebase (flag). Mirrors the body's ## Arguments section for the strict CLI parser. - OnboardPlaybook() — LibraryPlaybook constructor. - internal/collections/playbook_library.go: register OnboardPlaybook() in the agent-workflows category alongside ship/plan/decompose. - internal/collections/playbook_library_test.go: - TestPlaybookLibrary_InvokableEntriesPresent now expects 4 invokable entries (was 3) and includes onboard in wantSlugs. - New TestOnboardPlaybook_Contract locks the design contract: invocation_slug=onboard, trigger=manual (compatible with the blank template's minimal vocab), mode/defaults/skip-codebase argument shape, and presence of the "ADAPT, DON'T CURATE" posture in the body. Design notes captured at top of playbook_library_onboard.go: 1. Adapt, don't curate — library entries are starting points, rewrite using the project's actual commands. 2. Surface-agnostic — describe intent, not specific CLI commands; pure MCP users must follow the same flow. 3. Mode-aware — blank/audit/revisit/defaults paths. 4. Confirmation before mutation. 5. Self-removing nudge — the playbook produces user-created items which clear the bootstrap onboarding flag (TASK-1504, separate). Parent: PLAN-1496. Unblocked by TASK-1497 + TASK-1510/1511/1512. * fix: auto-seed onboard playbook + correct CLI form in body (Codex round 1) Addresses two PR #576 findings: 1. P1 — folding TASK-1500 into this PR: without auto-seed, the library entry alone makes /pad onboard manually-activatable but not invokable on day one. Codex correctly flagged that the PR as originally drafted shipped a half-feature. Wiring (PLAN-1496 / TASK-1500): - OnboardSeedPlaybook() in playbook_library_onboard.go returns the playbook as a SeedPlaybook with status=active, trigger=manual, scope=all, invocation_slug=onboard, arguments=onboardPlaybookArguments. Body + args are shared with the library entry (same pattern ShipPlaybook uses for ship) so they cannot drift. - SeedCollectionsFromTemplate appends OnboardSeedPlaybook to EVERY workspace created with a non-empty templateName — blank, startup, scrum, product, hiring, interviewing, demo. The empty-templateName path is preserved as the explicit backward-compat escape hatch (tests + direct API callers that want a bare workspace with zero items). cmd/pad/init.go always supplies a non-empty template (interactive picker or defaultTemplateName), so real user-facing workspace creation always lands in the seeded branch. Tests: - TestSeedFromTemplateAlwaysIncludesOnboardPlaybook walks all six real templates and confirms the onboard playbook is seeded into each. - TestSeedWithEmptyTemplateNameSkipsOnboard locks the escape-hatch invariant. - TestSeedFromBlankTemplate updated: blank workspace now ships exactly one item (the onboard playbook) instead of zero, because that's TASK-1500's whole point. 2. P2 — the body referenced 'pad library list-conventions', which doesn't exist. Corrected to 'pad library list --type conventions' (the actual CLI form), with a parenthetical pointing MCP users at pad_meta.action: bootstrap for the same data. This PR now covers both TASK-1499 (author playbook) and TASK-1500 (auto-seed) — combining them because Codex's P1 made it clear they ship together or not at all. * docs: correct MCP library-browse fallback in onboard body (Codex round 2) P2 finding on PR #576: the body told MCP-only users to read the convention library via 'pad_meta.action: bootstrap'. Bootstrap returns workspace STATE (collections, conventions, playbooks actually present in the workspace), not the global library catalog. So MCP users following that instruction would see only what's already activated, not what they could activate. The honest answer is that there is no MCP library-browse surface today. Updated the body to say so explicitly: if the agent has a shell, use 'pad library list'; if not, work from domain knowledge and have the user paste any library bodies they want as starting text. Captured the underlying gap as IDEA-1514 (Expose library catalog via MCP) and linked from the playbook body. Three options outlined there: new pad_library tool, pad_meta.action: library, or embed in bootstrap. Parent: PLAN-1496.	2026-05-17 11:52:20 -04:00
xarmian	cc0b1c0bf3	feat(templates): finalize 'blank' template with minimal-vocab seeds for /pad onboard (TASK-1498) (#575) ... * feat(templates): finalize 'blank' template for /pad onboard flow (TASK-1498) A blank template entry was already present in templates.go (drafted for IDEA-1479) but its seeded trigger/scope vocabularies leaked the software domain — on-commit, on-pr-create, on-implement, etc., baked into a template whose whole point is being domain-agnostic. The /pad onboard playbook (PLAN-1496 / TASK-1499) needs a true blank starting point so the interview can broaden vocabulary to match the project's actual domain, whatever it is. This commit: - Replaces the software-flavored seed with minimal vocab: trigger= always for conventions, trigger=manual for playbooks, scope=all on both. The constants live in templates_blank.go so future tweaks to the seed surface have a focused diff. The agent broadens via pad collection update (TASK-1510) during onboarding. - Updates the template's description and icon to point at the onboard flow ("Empty workspace — run /pad onboard to build it out", sparkles instead of memo). - Adds an in-place comment explaining the design choice so the next reader doesn't re-leak software triggers into the seed. - New test: TestBlankTemplateUsesMinimalVocabularies locks the minimal-seed posture; any regression that adds domain-flavored triggers fails this test and triggers a fresh design conversation. Pre-existing IDEA-1479 tests (Shape, ExcludesSoftwareCollections, AppearsInPicker) still pass — the contract they describe is preserved (2 system collections only, no user-facing leaks, Custom group placement). Parent: PLAN-1496. * fix(test): blank-vocab assertions use literal slices, not the vars they came from (round 1) P3 finding on PR #575: TestBlankTemplateUsesMinimalVocabularies compared template output to BlankConventionTriggers / BlankPlaybookTriggers — the same vars used to build the template. Widening either var would silently widen the "minimal" definition and the test would still pass, defeating the drift-guard intent. Switched to literal expected slices. Now any change to the var that adds a domain trigger fails the test loudly.	2026-05-17 07:52:58 -04:00
xarmian	8c9974f6fb	feat(cli,mcp): expose 'role update' via CLI and MCP catalog (TASK-1512) (#574) ... * feat(cli,mcp): expose 'role update' via CLI and MCP catalog (TASK-1512) Third of three TASK-1497 capability-spike follow-ups (after #572 and #573). The handlers_agent_roles.go::handleUpdateAgentRole PATCH handler and the internal/cli/client.go::UpdateAgentRole HTTP client method already existed. Only the agent-facing surfaces were missing. - cmd/pad: new 'pad role update <slug-or-uuid>' Cobra subcommand with --name / --slug / --description / --icon / --tools / --sort-order flags. Uses cmd.Flags().Changed for omit-if-unset. Positional arg = lookup ref; --slug = new slug value (rename). Empty-string clears for description and icon (the store treats *string("") as "clear", matching collection update semantics). - internal/mcp/catalog_role: new 'update' action + supporting params (new_slug, sort_order). The catalog disambiguates lookup-slug (in path) from rename-target (in body) with the new_slug input, avoiding the conflated-semantics footgun. - internal/mcp/dispatch_http_routes: new mapRoleUpdate mapper. Path uses input.slug for the lookup; body's "slug" key is sourced from input.new_slug. String fields use key-presence semantics so empty-string clears round-trip to the store. - Tests cover canonical body (with AgentRoleUpdate round-trip), new_slug-to-body-slug mapping, empty-string clearing, and required-arg validation. - README.md + internal/mcp/instructions.md pad_role action lists updated to include "update". Pairs with TASK-1510 + TASK-1511 to complete the workspace-mutation trio the /pad onboard playbook (TASK-1499) needs to adapt seeded roles, collections, and schemas to each project's actual shape. Parent: PLAN-1496. * fix(cli,mcp): rename role-update flag --slug → --new-slug (Codex round 1) P1 finding on PR #574: pad_role.update via local stdio MCP was silently broken. BuildCLIArgs translates MCP property "slug" to the CLI's positional <slug> AND to the --slug flag (same key reused), so: pad_role.update slug=<uuid> → pad role update <uuid> --slug <uuid> → tries to rename the role's slug to the literal UUID. BAD. pad_role.update slug=implementer new_slug=engineer → pad role update implementer --slug implementer → new_slug ignored entirely, no rename. The HTTP dispatcher had the disambiguation right (mapRoleUpdate already mapped MCP new_slug → body slug). The CLI flag name was the problem. Renamed --slug to --new-slug. Now MCP "slug" maps to the positional only (lookup), and MCP "new_slug" maps to --new-slug (rename target). Both transports symmetric. Updated example in --help, the liveCmdhelpDoc fake, and the change-detect block. Parent: PLAN-1496, Codex round 1 on PR #574 / TASK-1512.	2026-05-17 02:33:36 -04:00
xarmian	f76520f6e7	feat(cli,mcp): expose 'collection delete' via CLI and MCP catalog (TASK-1511) (#573) ... * feat(cli,mcp): expose 'collection delete' via CLI and MCP catalog (TASK-1511) Mirrors TASK-1510 (collection update). The HTTP handler at handlers_collections.go::handleDeleteCollection already supported DELETE on a collection (owner-only, soft-deletes the collection and every item in it). Wires both agent-facing surfaces: - internal/cli/client.go: new DeleteCollection client method. - cmd/pad: new 'pad collection delete <slug>' Cobra subcommand (no --force; the help text is the confirmation contract). - internal/mcp/catalog_collection: 'delete' action passes through to the CLI; tool description updated. - internal/mcp/dispatch_http_routes: simple routeSpec entry for DELETE /api/v1/workspaces/{workspace}/collections/{slug}. No custom mapper needed — no body, no field coercion. Pairs with TASK-1510 as the second adaptation primitive for the /pad onboard playbook (TASK-1499): when the onboard interview discovers a seeded collection that doesn't fit the project, the agent now has a way to remove it before creating the right one. Tests: - TestRouteTable_CollectionDelete (route substitutes correctly) - catalog_readonly bijection + liveCmdhelpDoc fake updated. Parent: PLAN-1496. * docs: correct collection delete contract per Codex review (round 1) Two findings on PR #573 — both documentation, no code behavior change: 1. CLI Long help / Short blurb / MCP description claimed delete "removes seeded collections" and the onboard use case targets template-seeded collections. But store.DeleteCollection refuses any collection where is_default=true, and every template seed is is_default=true. The advertised use case wouldn't actually work. Updated docs to clarify: delete is for USER-CREATED collections; template seeds must be adapted via 'pad collection update'. 2. Both CLI help and MCP description claimed "AND every item in it" gets archived. The store delete path only sets collections.deleted_at and never touches items. The web UI hides them via the join, but raw API queries still surface them. Updated docs to be honest: items are NOT cascaded. Captured the underlying behavior limitation as a follow-up: IDEA-1513 ("Lift is_default restriction on collection delete or add a cascade-items option") — surfaces options 1-4 for lifting the guard plus the items-orphan issue. Parent: PLAN-1496, addressing Codex round 1 on PR #573 / TASK-1511. * docs: tighten collection delete contract per Codex review (round 2) Three P3 documentation-drift findings: 1. internal/cli/client.go::DeleteCollection Go doc still said "and all items in it" — missed it in round 1. Updated to describe the actual behavior (collections.deleted_at only; items orphaned with soft-deleted collection_id; is_default rejected). 2. CLI Long help and MCP description claimed restore is available "via the API," but there is no restore endpoint and no RestoreCollection client method. Recovery is database-backup only. Both docs updated. 3. catalog_collection.go:33 slug ParamDef only mentioned action=update; action=delete needs it too. And the headline description still said "list, create, and update" — three actions when there are now four. Both fixed. Parent: PLAN-1496, addressing Codex round 2 on PR #573 / TASK-1511. * docs: update pad_collection action lists in instructions.md + README (round 3) Codex round 3 finding: two top-level reference docs still advertised pad_collection as list/create only. internal/mcp/instructions.md is embedded into the MCP initialize() handshake instructions — stale guidance there means MCP clients miss update/delete entirely. README's catalog table had the same drift. Parent: PLAN-1496, Codex round 3 on PR #573 / TASK-1511.	2026-05-17 02:15:37 -04:00
xarmian	f5579300fb	feat(cli,mcp): expose 'collection update' via CLI and MCP catalog (TASK-1510) (#572) ... * feat(cli,mcp): expose 'collection update' via CLI and MCP catalog (TASK-1510) The HTTP handler at handlers_collections.go::handleUpdateCollection already supported PATCHing a collection's name, icon, description, prefix, schema, settings, and sort_order (plus field-value migrations). The CLI and MCP surfaces never exposed it, so agents couldn't rename collections, swap icons, or reshape schemas — a hard blocker for the adaptive /pad onboard playbook (TASK-1499) which needs to rewrite seeded collections to match each project's actual vocabulary. This wires both agent-facing surfaces to the existing handler: - cmd/pad: new 'pad collection update <slug>' Cobra subcommand with --name / --icon / --description / --prefix / --schema / --fields / --sort-order flags. Only flags explicitly set are sent (uses cmd.Flags().Changed); --schema and --fields reuse the existing collectionSchemaJSONFromFlags helper so DSL parity stays. - internal/mcp/catalog_collection: add 'update' action plus the slug, prefix, and sort_order params on padCollectionTool. - internal/mcp/dispatch_http_routes: new mapCollectionUpdate handles the schema-object-vs-string coercion. The catalog declares schema as a JSON object for MCP ergonomics, but models.CollectionUpdate.Schema is *string — and its UnmarshalJSON only flexes settings, not schema. The mapper re-marshals object input to its JSON-string form before sending, symmetric to what the CLI does via collectionSchemaJSONFromFlags. Tests cover canonical body, schema-object-to-string coercion (round-trip through CollectionUpdate.UnmarshalJSON), schema-string pass-through, empty-field omission, and required-arg validation. catalog_readonly_test bijection + liveCmdhelpDoc fake updated. Parent: PLAN-1496. * fix(mcp): collection update — clear-on-empty + fields DSL parity per Codex review (round 1) Addresses two P2 findings on PR #572: 1. The catalog advertises `icon=""` / `description=""` / `prefix=""` as clear-the-field, and the CLI flag help says the same, but the HTTP mapper filtered empty strings via `v != ""` — leaving MCP HTTP callers unable to clear fields the CLI can. Switched to key-presence semantics for the four string fields so explicit empty strings round-trip to the store (which honors *string("") as "clear"). 2. The catalog advertises `fields OR schema` as mutually exclusive (mirroring `pad collection create`), but the mapper only consumed `schema`. An MCP HTTP request with `fields=...` produced a `{}` PATCH body silently. Extracted the DSL parser to a shared package (internal/collections/dsl.go::ParseFieldsDSL + FieldsDSLToSchemaJSON) so the CLI and the mapper share one parser; mapper now resolves fields-or-schema with the same mutual-exclusion guard the CLI has. Tests added in dispatch_http_routes_extras_test.go: - TestMapCollectionUpdate_EmptyStringClearsField - TestMapCollectionUpdate_AcceptsFieldsDSL (round-trips through models.CollectionSchema to confirm the parsed shape) - TestMapCollectionUpdate_RejectsFieldsAndSchemaTogether cmd/pad/main.go's parseFieldsDSL becomes a one-line alias for collections.ParseFieldsDSL so the CLI's behavior stays identical. Parent: PLAN-1496, fixing PR #572 / TASK-1510. * fix(mcp): collection update — use encodeSchemaForBody + normalize empty schema (round 2) Addresses two more findings from Codex round 2 on PR #572: 1. P2: mapCollectionUpdate bypassed encodeSchemaForBody, so structured schemas didn't get label backfill and string schemas weren't validated before PATCH — diverged from collection create + CLI. Now reuses encodeSchemaForBody (the same encoder collection create uses at dispatch_http_routes.go:418), getting label-backfill via the Title-Case-of-key heuristic and shape validation for free. 2. P3: schema=null or schema="" plus a real fields=... update tripped the mutual-exclusion check. Now normalizes empty inputs as absent BEFORE checking exclusivity, matching the relaxed handling collection create has for optional empty params. Tests: - Renamed TestMapCollectionUpdate_PassesSchemaStringVerbatim to TestMapCollectionUpdate_AcceptsSchemaString — the new property is round-trip parity + label backfill, not verbatim pass-through. - New TestMapCollectionUpdate_EmptySchemaDoesNotBlockFields covers both nil and empty-string schema combined with a real fields value. Parent: PLAN-1496, addressing Codex round 2 on PR #572 / TASK-1510.	2026-05-17 01:45:40 -04:00
xarmian	e59d3904c9	feat(server): refuse to mark item terminal while it has open children (IDEA-1494) (#571) ... * feat(server): refuse to mark item terminal while it has open children (IDEA-1494) Server-side guard inside handleUpdateItem that rejects a non-terminal → terminal done-field transition when the item still has at least one non-terminal child. Returns HTTP 409 with code=open_children plus a structured details payload listing each blocking child's {ref, title, status, collection_slug} so MCP-driven agents can self-recover (ship the listed children, then retry) and the CLI can render the same list verbatim. Escape hatch: `--force` on `pad item update` / `pad item bulk-update` and `force: true` on the MCP pad_item.action: update / bulk-update inputs both forward into the same ItemUpdate.Force transport field the handler consumes before any store mutation. Trigger conditions are tight: the PATCH must change the done-field key (resolved via TerminalValuesForDoneField against the parent's schema + settings) AND the new value must be terminal AND the current value must NOT already be terminal. Terminal → terminal and no-op terminal transitions bypass the guard; only entering the terminal set is gated. Per-child evaluation uses the child's own collection schema so hierarchical workspaces with custom typed collections work without extra plumbing. Tests cover: rejection with one open child (with mutation-safety assertion on the parent), no children, all-terminal children, --force override, no-op terminal → terminal, terminal → terminal, non-terminal → non-terminal, custom collection terminal_options honored, and a parent task (not a plan) — IDEA-1494 optional extra #3. MCP coverage asserts --force round-trips through both ExecDispatcher and HTTPHandlerDispatcher and is omitted when force=false. * fix(server): open-children guard round 2 — visibility, MCP pass-through, TOCTOU (IDEA-1494) Three Codex round-1 issues, each fixed with the recommended shape: P1 — visibility leak. The 409 response previously listed every blocking child by ref/title/status, including children in collections the caller couldn't see. The INVARIANT still evaluates against ALL children (it's a data-integrity gate — a restricted user must not be able to close a parent whose blockers they can't see), but the response payload now filters to caller-visible children only. Hidden blockers surface as a new `details.hidden_blocker_count` field plus an alternate human message when every blocker is hidden ("blocked by N open children you don't have access to"). Mirrors the visibility helpers (`visibleCollectionIDs` + `isItemVisibleToGuest`) used by the per-parent progress endpoint so the two paths can't drift. P2 — MCP code/details pass-through. The HTTP classifier was collapsing 409 into the generic `conflict` code and dropping `details`; the stdio classifier was matching the human "cannot " message against the validation regex and surfacing `validation_failed`. Both now surface `open_children` with the structured details intact: - HTTP: classifyHTTPStatusKind's 409 branch extracts the upstream code; any non-empty, non-"conflict" code is passed through with its `details` RawMessage. Generalizes beyond open_children — any future structured 409 from a handler gets the same treatment. - Stdio: the CLI writes a `pad-error: {json}\n` marker line on stderr before the human-readable block (single source of truth for both views), and classifyExecError detects the marker and lifts the envelope verbatim. Marker is duplicated as a const between internal/cli and internal/mcp to avoid pulling the cli package into the classifier just for one string. A new ErrOpenChildren error code constant + `Details json.RawMessage` field on ErrorPayload back the wire shape. P2 — TOCTOU. The guard previously ran in the handler before the store transaction began; a concurrent child insert / child status flip could slip between the children-list read and the parent's UPDATE. Fix: - New `Store.UpdateItemWithPreCheck(id, input, precheck)` runs the caller's invariant check inside the same tx, after acquiring the workspace seq lock AND a new parent-children advisory lock keyed on the parent ID. UpdateItem is now a thin wrapper passing nil. - Every UpdateItem unconditionally acquires the parent-children advisory lock for its own parent (if any) AND for itself-as-parent, in a fixed order (parent first) so two updaters touching the same parent always grab that key before the more-specific one — no AB/BA deadlock. - New `GetChildItemsTx` reads via the caller's tx; on Postgres the advisory lock provides the snapshot guarantee (DISTINCT precludes `FOR UPDATE`), on SQLite the global BEGIN IMMEDIATE write lock serializes all writers. - Handler now passes a precheck closure into UpdateItemWithPreCheck at all three call sites (collab-snapshot path, applier-direct-write path, main path). The guard's openChildrenGuardError sentinel is unwrapped after each call so the 409 surfaces cleanly. Tests: - TestOpenChildrenGuard_VisibilitySanitizesPayload — restricted editor sees parent + visible child, hidden child contributes to hidden_blocker_count, no leak of ref/title/slug. - TestOpenChildrenGuard_AllBlockersHiddenSurfaceGenericMessage — open_children=[], hidden_blocker_count>0, message mentions "you don't have access to." - TestOpenChildrenGuard_TOCTOURace — 8 iterations of a child-flip racing a parent-terminal update; asserts the forbidden outcome (parent=completed AND child=open) never occurs. - TestClassifyHTTPStatus_OpenChildrenPreservesCodeAndDetails + inverse generic-409 test. - TestClassifyExecError_OpenChildrenMarkerLiftsStructuredPayload + no-marker-falls-through inverse. * fix(server): open-children guard round 3 — 7 Codex findings closed (IDEA-1494) P1 — visibility fail-closed. The handler was swallowing visibleCollectionIDs errors, leaving visIDs==nil which the guard treats as unrestricted, leaking hidden-child metadata. Now surfaces the error as 500 BEFORE installing the precheck. Test: TestOpenChildrenGuard_VisibilityLookupErrorFailsClosed closes the store DB and asserts no 409+children leak. P1 — link mutations acquire the advisory lock. SetParentLink, ClearParentLink, CreateItemLink (when link_type ∈ childLinkTypes via new isChildLinkType helper), DeleteItemLink (same condition), and RestoreItem now take `pad:parent-children:<id>` in canonical sorted order via new AcquireParentChildrenLocks helper. SetParentLink locks BOTH old and new parents (re-parenting case). Race test TestOpenChildrenGuard_LinkMutationRace asserts the forbidden "link-committed-before-parent-flip AND parent flip succeeded" never occurs by comparing link.created_at to parent.updated_at. Documented semantics: status-wins + link-after-commit is legal under the invariant "no open children EXIST AT THE MOMENT of transition" — the post-condition variant ("no open child may EVER attach to a terminal parent") is intentionally deferred. P1 — MoveItem bypass closed. New MoveItemWithPreCheck mirrors UpdateItemWithPreCheck — acquires workspace seq lock + parent-children locks, re-reads in tx, runs caller precheck. handleMoveItem builds the same guard closure using the DESTINATION schema for done-field resolution (conservative — honors the schema the item moves INTO). CLI gains `pad item move --force`, client gains MoveItemWithForce that appends `?force=true` to the move endpoint. MCP catalog + mapItemMove forward `force` through the route mapper. Tests: TestOpenChildrenGuard_MoveItem_RejectsTerminalWithOpenChildren and …_ForceOverrides. P2 — pre-tx field-read TOCTOU. UpdateItemWithPreCheck and MoveItemWithPreCheck now re-read the item via new getItemTx INSIDE the tx (after locks) and pass that fresh snapshot to the precheck closure; the precheck classifies the transition against the in-tx view, not the handler-side pre-tx capture. Handler precheck closure swaps `currentFieldsJS` from the in-tx snapshot. Test: TestOpenChildrenGuard_PrecheckReadsInTxSnapshot stages a between-load status mutation and asserts the precheck observes the post-mutation fields. P2 — bulk-update carries structured errors. cmd/pad/main.go's updateFailure struct extended with Code + Details (json.RawMessage). When client.UpdateItem returns *cli.APIError, the row preserves the structured envelope. Human-text output also renders the open-children list inline. Chose JSON-envelope route over per-row stderr markers because bulk-update already produces a structured envelope and ExecDispatcher returns stdout verbatim on exit-0 — no classifier change needed. Test: TestBulkUpdateStructuredFailuresCarryOpenChildrenDetails confirms the wire shape the CLI lifts. P3 — marker hardening. Marker bumped to versioned form `pad-structured-error/v1:` (was `pad-error:`). cli.StructuredErrorMarker + mcp.structuredErrorMarker kept in lockstep with cross-references. mcp.allowedStructuredErrorCodes whitelists known codes (currently just open_children); unknown codes fall back to regex classification. Marker must start the line after whitespace trim (embedded markers ignored). Last-marker-wins to defeat pre-emption attacks. Tests: TestClassifyExecError_{UnknownStructuredCode,OldMarkerVersion, MarkerEmbeddedMidLine,LastMarker}. P3 — soft-deleted collection schemas honored. New GetCollectionAnyState mirrors childrenDoneFiltersForParent's inclusion rule; guard uses it so a child still attached to a soft-deleted collection is evaluated against ITS schema (custom terminal_options) instead of the default- status fallback (which would mis-classify and false-block). Test: TestOpenChildrenGuard_SoftDeletedCollectionSchemaHonored seeds a custom collection, soft-deletes it while a child remains, and asserts the terminal status is correctly recognized. Comprehensive store-mutation audit results recorded in the PR description (every method touching items.fields / items.collection_id or item_links). * fix(server): open-children guard round 4 — multi-parent locks, enum parity, PATCH atomicity (IDEA-1494) Four Codex round-3 (blast-radius lens) findings, each fixed with the recommended shape. P1 — multi-parent lock set. acquireParentChildrenLocksForUpdate and RestoreItem previously used `LIMIT 1` against item_links, so a child with BOTH a `parent` link to P1 AND an `implements` link to P2 only locked one of them. The other parent's open-children precheck could race against the child's status flip and miss it. Fix: new listParentChildLockKeys helper runs the same query GetChildItems' inclusion rule uses (childLinkTypes), returns ALL distinct parent target_ids, and feeds them into the canonical multi-lock helper. Both UpdateItemWithPreCheck and RestoreItem now acquire locks on {self} ∪ {all-parents-via-childLinkTypes}. Test: TestOpenChildrenGuard_MultiParentChildLocksAll races a child status flip against terminal-updates on both parents simultaneously. P2 — lock-order asymmetry. The pre-fix codebase had multiple lock- acquisition shapes: parent-then-self in acquireParentChildrenLocksForUpdate, single-key in RestoreItem / CreateItemLink / DeleteItemLink / ClearParentLink, and a sorted multi-key in SetParentLink. Two concurrent callers using different ad-hoc orderings could AB/BA deadlock. Fix: removed the per-call-site AcquireParentChildrenLock helper entirely. Every site now goes through AcquireParentChildrenLocks (the canonical sorted multi-lock helper) — including ones that need only one ID (the variadic call still sorts a one-element slice). The helper's doc comment explicitly states the contract: "Ad-hoc single-key acquisition outside this helper is FORBIDDEN — two call sites taking distinct keys in different orders WILL deadlock." Test: TestOpenChildrenGuard_NoDeadlockUnderReverseOrderConcurrency runs reverse-order re-parents with a 5-second timeout; assertion fails on hang. P2 — HTTP/stdio code-surface parity. Round 2's HTTP pass-through ("any non-conflict upstream code") silently widened the ErrorCode enum beyond stdio's allow-list (`open_children` only). Agents saw different code surfaces depending on which dispatcher delivered the response. Fix: HTTP 409 branch in classifyHTTPStatusKind now consults the same allowedStructuredErrorCodes whitelist stdio does. Codes outside the set collapse to ErrConflict (no details), matching what stdio does for an unknown-code structured marker. Doc on allowedStructuredErrorCodes updated to make the dual-consumer contract explicit: "Adding a new structured code is a TWO-WAY change." Tests: TestClassifyHTTPStatus_UnknownConflictCodeFallsBackToErrConflict and TestStructuredErrorCodeParityAcrossTransports. P3 — PATCH atomicity. A combined PATCH with `parent` + `status=terminal` on an item with open children used to commit the parent-link change INLINE (before the guard ran) and then reject the field write. Caller saw 409 but the parent had already moved. Fix: parent-link mutation is now DEFERRED — captured into outer- scope vars during fields validation, executed AFTER UpdateItemWithPreCheck succeeds. A guard rejection returns before the link write block, so on rejection the link is untouched. Documented choice: "reorder, don't tx-wrap" — wrapping SetParentLink into the same store tx would require threading a *sql.Tx through the SetParentLink API (which is also called from the handler_item_links path); reordering is the smaller surgery and gives the correct outcome on the failure direction. A residual window remains in the OTHER direction (field write commits, link write fails) — not made worse by the reorder, and called out inline for a future tx-wrap pass. Test: TestOpenChildrenGuard_PatchAtomicRejectionPreservesParentLink sets up target → oldParent → openChild, sends PATCH {parent=newParent, status=completed}, asserts 409 AND target.parent_link still points at oldParent. * fix(server): open-children guard — emit details.open_children as [] not null on hidden-only rejection (IDEA-1494)	2026-05-17 00:15:52 -04:00
xarmian	9ebdfb503e	Revert "feat(cli): pad session shape — Claude Code context-window telemetry (IDEA-1491) (#569)" (#570) ... This reverts commit 351f83af3f.	2026-05-16 21:12:40 -04:00
xarmian	351f83af3f	feat(cli): pad session shape — Claude Code context-window telemetry (IDEA-1491) (#569) ... * feat(cli): pad session shape — Claude Code context-window telemetry (IDEA-1491) New `pad session shape [--session <id|path>] [--format json|table|markdown]` command that reads the active Claude Code session JSONL and reports tokens, context_pct (vs hardcoded per-agent-version budget), message counts, and elapsed time. Default format is JSON because agents are the primary caller. - internal/cli/claudecode.go: project-slug derivation, JSONL streaming parser, env/cwd/autodetect cascade resolver, per-version budget table (seeded with 2.1.* → 1M tokens per the IDEA-1491 recon update). - cmd/pad/session.go: top-level `session` group + `shape` subcommand, three output formats, registered via cmd/pad/main.go's rootCmd. - Tests cover slug derivation (live ~/.claude/projects/ verified cases), JSONL parse (normal / no-usage / sidechain fixtures), budget lookup, context-class bucketing, and the resolver cascade in t.TempDir(). Sidechain/sub-agent JSONL summing and the IDEA-body Pad-invocation-count fallback are intentionally deferred to v2 (TODOs in session.go). * fix(cli): session shape — TotalPrompt as context numerator + count parallel tool_use (IDEA-1491) Codex R1 review findings: P1 — context_pct numerator was CacheRead, which is a steady-state proxy that under-counts at turn boundaries when fresh content sits in cache_creation/input before being folded into the cached prefix. The correct denominator-of-budget is the full prompt footprint sent this turn: cache_read + cache_creation + input = TotalPrompt. Markdown renderer's context-tokens line follows suit; the explicit per-component breakdown rows keep CacheRead so the components remain visible. P2 — ToolInvocations was counting assistant-turns-with-any-tool-use, not tool invocations. A single assistant turn can emit multiple parallel tool_use blocks in message.content[]; the field name promises a count of invocations. Drop the early break. Test fixture normal.jsonl gains a 3-parallel-tool-use final turn; assert ToolInvocations==4 (up from 2) to pin the new behavior. * fix(cli): session shape — path-traversal, oversized-line, content-variant, explicit-flag errors (IDEA-1491) Codex R2 review findings: P1 — session-ID inputs (--session flag-id branch AND $CLAUDE_CODE_SESSION_ID env var) are now validated before they become filename fragments under ~/.claude/projects/<slug>/. Reject path separators, '..' segments, and anything that doesn't match a UUID-ish shape (hex+dashes, 8+ chars). New ErrInvalidSessionID wraps a descriptive message. Without this, `--session ../../../../../tmp/foo` or a poisoned env var could escape the projects dir on the candidate os.Stat. End-to-end smoke confirms `pad session shape --session '../../../etc/passwd'` now errors with "invalid session id: ... contains a path separator" and exits non-zero. P2a — Switch the JSONL line reader from bufio.Scanner (8 MiB cap, hard fail on overflow with "token too long") to bufio.Reader.ReadBytes('\n') (no cap). encoding/json has no size limit either, so oversized records — e.g. inline file attachments — parse cleanly. Both ParseSessionJSONL and tailLineCWD updated for parity. P2b — jsonlLine.Message.Content was []json.RawMessage at the outer decode, which made a schema variant where content is a string or object fail the WHOLE line's decode, losing type/timestamp/version/ usage data. Now Content is a raw json.RawMessage; the tool-use scan re-decodes it as []json.RawMessage on a best-effort basis and skips tool-counting when that fails, while preserving the rest of the line. P3 — `pad session shape --session <id|path>` no longer silently falls back when the resolver errors. An explicit flag means the caller has a specific session in mind; a typo or wrong UUID should fail loudly so automation surfaces the bug instead of emitting `agent: "unknown"`. The implicit (no-flag) path still falls back for non-Claude-Code harnesses. Tests added/extended: - TestResolveSessionLog_RejectsPathTraversal — flag-id AND env-id branches, full bad-input matrix. - TestParseSessionJSONL_OversizedLine — 10 MiB single record. - TestParseSessionJSONL_NonArrayContent — content-as-string variant must still contribute timestamp/version/usage. - TestParseSessionJSONL_ParallelToolUse — tight hermetic check of the R1 P2 multi-tool-per-turn fix. - TestBuildSessionShape_ExplicitFlagErrors — verify --session errors propagate. - TestBuildSessionShape_ImplicitFallback — verify implicit path still falls back.	2026-05-16 20:18:37 -04:00
xarmian	8094883869	feat(links): cross-workspace wiki-link resolution (IDEA-1492) (#568) ... * feat(links): cross-workspace wiki-link resolution (IDEA-1492) Adds [[workspace::REF]] and [[workspace::REF|Display]] wiki-link syntax that resolves cross-workspace, plus a Go route GET /{username}/{workspace}/ref/{REF} that 302-redirects to the canonical item URL. 404 leaks no info about workspace existence — malformed refs short-circuit before the workspace lookup, and access-denied returns 404 not 403. Frontend (web/src/lib/utils/markdown.ts): - renderMarkdown recognizes the workspace-prefix form and emits cross-workspace anchors (class doc-link cross-workspace) pointing at the resolver route. Same-workspace prefix is stripped and behaves identically to the legacy [[REF]] form. - wikiLinksToMarkdown emits the resolver URL for cross-workspace storage and same-workspace items resolve through the in-memory list. - markdownToWikiLinks rolls /<user?>/<ws>/ref/<REF> URLs back to [[workspace::REF]] (or |Display when display text differs from the ref). Legacy same-workspace round-trip is preserved. Backend (internal/server/handlers_ref_resolver.go): - Validates the ref shape before the DB hit (no oracle). - Reuses resolveWorkspace + GetItemByRef for ACL + lookup. - refResolverItemVisible mirrors requireItemVisible without depending on RequireWorkspaceAccess middleware (this route is reachable outside the workspace-scoped route group). - Redirect target matches itemUrlId() so the post-redirect URL is indistinguishable from a direct in-app navigation. Tests: 302 success, 404 unknown-workspace, 404 unknown-ref, 404 on a matrix of malformed refs (including url-encoded traversal). The no-access matrix is partially covered — the existing test surface doesn't compose a multi-user ACL fixture, so production-grade "member of A probes B" is gated by the real auth middleware stack and documented in TestRefResolver_NoAccess_DocumentsPreSetupBypass. * fix(links): codex round-1 fixes for cross-workspace resolver (IDEA-1492) P1.1 — Reserve "ref" as a collection slug. A collection slug of "ref" would shadow every item URL under the resolver's /{u}/{ws}/ref/... route. Added to reservedCollectionSlugs in internal/store/collections.go so it auto-suffixes to "ref-collection", matching the existing treatment of settings/activity/roles/etc. P1.2 — Extract checkItemVisible as a context-free helper. The previous refResolverItemVisible silently diverged from requireItemVisible by ignoring direct collection grants and member_collection_access for restricted members — a member with "specific" access on collection A plus a direct grant on collection B would 404 through the resolver even though they could see the item via the API. checkItemVisible now replays the same rules requireItemVisible inlined; requireItemVisible is now a thin wrapper, and the resolver derives its workspace role via resolverWorkspaceRole and delegates to checkItemVisible. Drift between the two paths is structurally impossible. P2.1 — Cross-workspace round-trip preserves explicit display overrides. The strip condition was `displayText === ref`, which would drop the override on [[other::TASK-1|TASK-1]] — then re-rendering would emit the default `other::TASK-1` and silently change visible link text. Fixed: only strip when displayText matches the actual render default `${ws}::${ref}`. P2.2 — Two-segment route /{workspace}/ref/{REF}. TimelineCommentCard and CommentThread call renderMarkdown without a username, so links in timeline comments emit the two-segment href shape. Registered the shorter route against the same handler; when the URL-path username is absent the handler falls back to the workspace owner's username via a new resolverOwnerUsername helper. Sanity sweep: - Dropped refItoa wrapper; use strconv.Itoa directly. The non-test `itoa` collision was a test-only `itoa` in handlers_admin_users_test.go, not a real symbol in non-test builds. - Removed encodeURIComponent on workspace + ref in renderMarkdown. parseCrossWorkspaceBody validates both against URL-safe regexes, and wikiLinksToMarkdown doesn't encode — both functions now emit identical bytes. Tests: - TestRefResolver_RejectsRefAsCollectionSlug — pins the reservation. - TestRefResolver_TwoSegmentRouteResolves — both URL shapes resolve; two-seg synthesizes the owner username. - TestRefResolver_RestrictedMemberWithCollectionGrant — the codex-flagged ACL case (restricted member + collection grant on a different collection) now resolves to 302, not 404. Frontend test for the round-trip override fix is documented as a gap (no vitest infra in the repo). * fix(links): codex round-2 fixes for cross-workspace resolver (IDEA-1492) P1.1 — Tokenized roles bypass user-nil check. Pre-fix, checkItemVisible rejected (nil user, "editor") tuples — exactly what RequireWorkspaceAccess synthesizes for legacy workspace-scoped API tokens — false-404'ing every requireItemVisible-gated handler hit by those tokens. Reordered the checkItemVisible rules so any tokenized role (owner / editor) bypasses the user-nil guard. checkItemVisible regression test (no HTTP layer) pins the bypass. P1.2 — System collections folded into item-grants branch. The pre-round-1 guestResourceFilterCore unioned ListSystemCollectionIDs into the fullCollIDs set; the round-1 refactor dropped that union, so a restricted member with conventions/playbooks (system collection) access plus an unrelated item grant could LIST system items but 404 on detail-fetch / ref-resolve. Restored the union inside checkItemVisible's item-grants branch (non-guest path only — matches the original guestResourceFilterCore semantics). P1.3 — Empty owner-username 404s instead of emitting broken redirect. When the workspace owner has no username on file (pre-setup ownerless workspaces, legacy accounts), the synthesized redirect target became `"/" + "" + "/" + slug + ...` → `//slug/...` — a protocol-relative URL browsers interpret as a network-path reference. Now 404s via refResolverNotFound rather than emitting the malformed Location header. P1.4 — URL shape changed to /-/r/{workspace}/{ref} (Option B). Pre-fix, the resolver lived at /{username}/{workspace}/ref/{ref}, which would intercept item URLs in workspaces with pre-existing `ref`-slugged collections (upgraded data; the round-1 reservation only blocks NEW creates). Picked Option B over a migration because the feature is unshipped, the new shape is more defensive (no future risk under any collection slug), and the only cost is the frontend emit-shape change. The leading `/-/r/` prefix can never collide with a user-namespace URL because username + slug grammar both require letter-led. Frontend renderMarkdown, wikiLinksToMarkdown, and markdownToWikiLinks all emit and parse the new shape; the round-1 collection-slug reservation stays as defense in depth. Tests: - TestCheckItemVisible_TokenizedRoleAllowsNilUser — P1.1 regression. - TestRefResolver_RestrictedMemberWithSystemCollection — P1.2 regression. - TestRefResolver_PreSetupBypass — P1.3 (ownerless workspace returns 404, not a broken `//slug/...` redirect). - TestRefResolver_URLShapeNonOverlap — P1.4 (resolver doesn't intercept `/{user}/{ws}/ref/{slug}` URLs). - Existing TestRefResolver_* updated to the /-/r/ shape; the previous two-segment fallback test is removed (the new URL shape has no username component, so there's no two-segment vs three-segment distinction to test). * fix(links): scope round-2 bypass to nil user (Codex round-3 P1) Round-2's checkItemVisible bypass for role in {"owner", "editor"} fired unconditionally, including for real authenticated members. Result: a member with workspace role "editor" and collection_access="specific" short-circuited the per-collection filter — they could GET/PATCH/DELETE items in collections their member_collection_access list excluded. Scoped the bypass to the tokenized-nil-user case only: if user == nil && (role == "owner" || role == "editor") This is the exact set the bypass was supposed to address — fresh-install mode (UserCount==0, role="owner") and legacy workspace-scoped API tokens (tokenWorkspaceID matches, role="editor"). Both paths set currentUser to nil; both are authorized by RequireWorkspaceAccess before checkItemVisible runs. Real authenticated members with the same roles now correctly fall through to the existing per-collection visibility filter. Workspace owners with default access still pass via the rule-4 "all access" short-circuit (member.CollectionAccess == "all"); restricted editors are now gated as intended. Updated the rule-1 doc comment to make the scope-to-nil-user discipline explicit — the prior wording conflated the tokenized and authenticated paths, which is what led to the over-broad bypass. Test: TestCheckItemVisible_AuthenticatedEditorWithRestrictedAccess seeds a real editor with collection_access="specific" granting only collection A, asserts visibility on a collection-B item returns false, and adds a sanity assertion that the same editor sees collection-A items. The existing TestCheckItemVisible_TokenizedRoleAllowsNilUser still passes — it covers the (nil, "editor") tuple the corrected bypass still allows. Direct callers of checkItemVisible (grep): only requireItemVisible (server.go) and resolverItemVisible (handlers_ref_resolver.go). Both pass real (user, role) from request context, so the narrower scope doesn't break any prior-green path. * fix(links): allow digit-leading workspace slugs in xw wiki-links Frontend WORKSPACE_SLUG_PATTERN was tighter than store.slugify (the canonical rule): slugify keeps digit-leading inputs (e.g. "2026 Roadmap" → "2026-roadmap") but the frontend regex rejected them. Effect: `[[2026-roadmap::TASK-1]]` fell through as a legacy title link, and `/-/r/2026-roadmap/TASK-1` URLs didn't round-trip back to wiki syntax. Two regex hunks, no behavior change beyond accepting the digit-led case: - WORKSPACE_SLUG_PATTERN: ^[a-z][a-z0-9-]*$ → ^[a-z0-9][a-z0-9-]*$ - markdownToWikiLinks reverse-regex workspace class: same widening Stale doc-comment citing the old pattern updated to match. Collection-slug grammar stays letter-led (the upstream rule differs; only workspace slugs accept digit-led). Only functional consumer of WORKSPACE_SLUG_PATTERN is parseCrossWorkspaceBody, which uses the match boolean — no other downstream code relied on the leading-letter constraint (Codex round-4).	2026-05-16 18:20:17 -04:00
xarmian	713421670a	feat(store): corrective backfill for collections.settings shape violations (IDEA-1489) (#567) ... * feat(store): corrective backfill for collections.settings shape violations (IDEA-1489) Migration 055 / pgmigrations/034 (PR #562) hardened collections.settings to NOT NULL DEFAULT '{}' but its backfill only matched `settings IS NULL`. Pre-existing rows with wrong-shape settings (empty string, JSON "null" literal, "[]" array, non-JSON garbage on SQLite; JSONB null, arrays, primitives on Postgres) survived the filter — NOT NULL satisfied but the IDEA-1484 contract (settings is always a JSON object) violated. PR #566 established the per-driver `json_valid()` / `jsonb_typeof()` widening pattern for its own four migrations (056, 057, pg/035, pg/036) but was scope-bounded out of retroactively repairing 055 / pg/034. This commit closes that gap. - migrations/058_collections_settings_shape_repair.sql: UPDATE collections SET settings = '{}' WHERE settings IS NULL OR json_valid(settings) = 0 OR json_type(settings) != 'object'; - pgmigrations/037_collections_settings_shape_repair.sql: UPDATE collections SET settings = '{}'::jsonb WHERE settings IS NULL OR jsonb_typeof(settings) != 'object'; - internal/store/collections_settings_shape_repair_test.go: TestCollectionsSettingsShapeRepair_SQLite seeds rows with every observable pre-055 pathology, applies 055-058, asserts each is repaired to '{}' and valid rows are preserved. TestCollectionsSettingsShapeRepair_Postgres seeds JSONB-valid-but- wrong-shape rows (NULL is unseedable post-pg/034) and asserts the pg/037 UPDATE clause repairs them. Toggle-verified: with the WHERE predicate reverted to `IS NULL` only, both tests fail on the non-NULL malformed seeds; with the full predicate both pass. Full ./... suite green on SQLite and Postgres. * chore: gofmt comment in shape repair test	2026-05-16 11:58:56 -04:00
xarmian	ec71903be7	feat(store): JSONB NOT NULL hardening on items/views + handler shape validation (IDEA-1486+1488) (#566) ... * feat(store): NOT NULL hardening on items.fields/tags + views.config (IDEA-1486) Paired ship of IDEA-1486 (sibling-table JSONB NOT NULL hardening) and IDEA-1488 (handler-layer shape validation for ViewUpdate/CollectionUpdate). Generalizes the IDEA-1484 / collections.settings precedent (PR #562) to the remaining nullable JSON columns and closes the shape-validation gap that NOT NULL alone doesn't cover. Schema layer (IDEA-1486 floor): - migrations/056_items_jsonb_not_null.sql: rebuild items with fields TEXT NOT NULL DEFAULT '{}' and tags TEXT NOT NULL DEFAULT '[]', preserving all 7 indexes, recreating the 3 items_fts triggers, and rebuilding the FTS5 index. Foreign-keys-off / on bookends are lifted outside the IDEA-1485 atomic-tx wrapper. - migrations/057_views_config_not_null.sql: rebuild views with config TEXT NOT NULL DEFAULT '{}'. - pgmigrations/035 + 036: SET NOT NULL + SET DEFAULT on the three JSONB columns. Split per-table to mirror the SQLite per-table file granularity. Store layer (IDEA-1486 floor): - items.go UpdateItem and views.go UpdateView normalize "" -> "{}" / "[]" before writing. Same boundary pattern as CreateItem and the IDEA-1484 precedent at collections.go:248. - export.go ImportWorkspace coerces empty-string AND malformed JSON at import time on items.fields, items.tags, and collections.settings. Malformed input is coerce-and-log via slog.Warn (length only, never raw value) so legacy bundles don't fail-stop on one bad row. - remapFieldIDs early-returns "{}" on empty input so the second-pass UPDATE can't write "" verbatim. - Migrated the existing fmt.Printf at export.go:329 to slog.Warn for consistency. Handler layer (IDEA-1488 ceiling): - ViewCreate / ViewUpdate UnmarshalJSON via flexJSONToString with new ErrInvalidConfigType sentinel. - CollectionCreate / CollectionUpdate UnmarshalJSON with new ErrInvalidSettingsType sentinel. - handlers_views.go and handlers_collections.go surface both sentinels as 400 with the domain-level message (mirrors the BUG-1144 precedent at handlers_items.go:641). Tests: - internal/store/items_views_jsonb_test.go: store-coercion + import coercion + log-and-coerce-on-malformed + SQLite schema introspection (7 indexes + 3 FTS triggers + items_fts virtual table survival) + Postgres NOT NULL enforcement + migration re-apply idempotency + item_links round-trip after rebuild. - internal/server/handlers_views_collections_jsonb_test.go: PATCH/POST flexible-shape coverage for views.config and collections.settings, including domain-level 400 message assertions that the response does not leak Go unmarshal internals. Refs: IDEA-1486, IDEA-1488, IDEA-1484 (precedent), IDEA-1485 (substrate). * fix(store,models): codex R1 follow-ups for IDEA-1486 / IDEA-1488 Three concrete defects surfaced by codex R1 against the initial paired ship. All three close holes that defeated parts of the original contract. P1.1: migration 056 missed the playbook invocation_slug unique index. - migrations/056_items_jsonb_not_null.sql: recreate the partial UNIQUE index idx_items_invocation_slug_per_collection from migration 054 verbatim after the other 7 indexes. Without it, the application-layer pre-check in handlers_items.go:checkUniqueFields would be a TOCTOU race with no DB-level guard — the original index that 054 explicitly added as the actual uniqueness backstop would be silently dropped during the items rebuild. - items_views_jsonb_test.go: the schema-introspection test now asserts 8 indexes, not 7. Verified via `grep -rn "ON items(" migrations/` that no other items-touching indexes were missed. P1.2: flexJSONToString didn't validate inner content of JSON-encoded strings. Pre-fix, `{"config": "[]"}` / `{"settings": "not json"}` / `{"fields": "[]"}` / `{"tags": "{}"}` slipped past the shape validators because the `case '"'` branch unmarshalled the envelope and returned the inner string verbatim — bypassing the whole point of IDEA-1488. - models/item.go: after unmarshalling the JSON-encoded string, validate that the trimmed inner content's first byte matches expectedStart ('{' / '[') AND parses as JSON. Empty inner strings still pass through to the store-layer empty-string coercion (IDEA-1486 floor), so legacy "" → default normalization is preserved. - The pre-existing ItemUpdate fields/tags path inherits the same tightening because it routes through this helper — covered by new test file handlers_items_jsonb_inner_shape_test.go. - Parallel handler tests for views.config and collections.settings added to handlers_views_collections_jsonb_test.go. P2: coerceJSONForImport accepted JSON null as well-formed. - store/export.go: json.Unmarshal("null", &m) returns err=nil with m staying nil; the prior code returned the raw "null" string verbatim, which lands as JSONB null on Postgres (satisfies NOT NULL since SQL NULL ≠ JSONB null) or text "null" on SQLite. The non-nil check on the unmarshalled value routes JSON null to the existing log-and-coerce path with the rest of the malformed shapes. - items_views_jsonb_test.go: extended import test with an item carrying fields=null / tags=null; expects both coerced to "{}" / "[]" and the structured slog.Warn emitted. Verified: make test (SQLite) and the full ./... suite against the existing port-5445 Postgres container both pass cleanly. Refs: IDEA-1486, IDEA-1488, codex R1 review. * fix(store,server): codex R2 follow-ups for IDEA-1486 / IDEA-1488 Two defects surfaced by codex R2. P1 is a real ship-breaker; P2 closes a parity gap that R1 missed. P1: migration backfill normalized only SQL NULL, not malformed/wrong- shape JSON. The four new migrations originally wrote `WHERE x IS NULL`. Rows with fields = '' / 'null' / '[]' / 'not json' all survived the filter, then violated the post-migration NOT NULL+shape contract. Concrete ship- breaker on SQLite: 056 recreates the partial UNIQUE index on json_extract(fields, '$.invocation_slug') from migration 054, and json_extract errors on rows whose fields fails json_valid — a single bad row breaks CREATE INDEX mid-migration. Toggle-verified: with the NULL-only WHERE, the new SQLite test fails at exactly that CREATE INDEX with "SQL logic error: malformed JSON (1)". Widened the backfill clauses: - migrations/056: UPDATE items WHERE fields IS NULL OR json_valid(fields)=0 OR json_type(fields)!='object' (same trio for tags with 'array'). - migrations/057: same trio for views.config. - pgmigrations/035: WHERE fields IS NULL OR jsonb_typeof(fields)!='object'. JSONB rejects invalid JSON on write so the json_valid leg isn't needed on Postgres; only the shape check matters. - pgmigrations/036: same shape check on views.config. Regression tests in internal/store/items_views_jsonb_test.go: - TestItemsViewsJSONB_SQLiteBackfillRepairsMalformedShapes: applies migrations through 053 (skipping 054 which would itself error on malformed rows), seeds every observable shape pathology — SQL NULL, empty string, JSON null literal, wrong-shape JSON, non-JSON garbage — then applies 055/056/057. Asserts every malformed row is repaired AND the partial UNIQUE index actually fires on duplicate invocation_slug post-rebuild (proving the CREATE INDEX path executed end-to-end). - TestItemsViewsJSONB_PostgresBackfillRepairsMalformedShapes: parallel Postgres coverage; seeds JSONB null / array / primitive via direct ::jsonb cast and asserts the widened WHERE clause repairs each. P2: handleCreateItem didn't unwrap ErrInvalidFieldsType/ErrInvalidTagsType. R1's flexJSONToString tightening propagated the sentinels through every UnmarshalJSON path, but handleCreateItem (POST /items) still returned 'invalid JSON: <wrapped>' from decodeJSON. PATCH and the view/collection POST/PATCH handlers already unwrapped — POST was the outlier. - internal/server/handlers_items.go: mirror the PATCH-side errors.Is handling at the POST path. Brief, three-line diff. - handlers_items_jsonb_inner_shape_test.go: new TestCreateItem_ JSONEncodedStringInnerShapeValidated covers POST with fields=`[]`, fields=42, tags=`{}`, tags={"x":1}, plus a valid positive control. Asserts no "invalid JSON:" wrapper and presence of the sentinel message verbatim. Backfill-pattern audit (codex R2's grep prompt): only 055 / pg-034 (collections.settings, already shipped) exhibits the same NULL-only WHERE gap. Per the brief: NOT touched — retroactive repair belongs to a separate IDEA. Other NULL-only backfills (043/pg-023's oauth_providers, 044/pg-024's expires_at) handle their respective shapes correctly or aren't JSON columns. Verified: make test (SQLite) clean. Full ./... suite against the existing port-5445 Postgres container clean (one unrelated flake in internal/collab passed on rerun). Refs: IDEA-1486, IDEA-1488, codex R2 review.	2026-05-16 10:15:59 -04:00
xarmian	853a7cd453	feat(store): wrap migrations in atomic transactions (IDEA-1485) (#565) ... * fix(store): wrap each migration in a transaction (IDEA-1485) Pad's SQLite migration runner exec'd statements one-by-one on the raw *sql.DB, then INSERT'd into schema_migrations afterwards. A crash between statement N and the bookkeeping INSERT left the schema in an intermediate state, and the next startup re-ran the migration against the already-mutated database — permanent data loss for the table-rebuild migrations 022 / 055. Wrap each migration body + the schema_migrations INSERT in a single BEGIN/COMMIT so they commit (or roll back) atomically. Same pattern for Postgres migrations. PRAGMA-foreign-keys handling for SQLite is load-bearing: the existing 022 / 055 migrations rely on `PRAGMA foreign_keys = OFF` around their DROP TABLE, and that PRAGMA is a no-op inside a SQLite transaction. The new runner lifts PRAGMA statements out of the migration body, pins the migration to a single connection via db.Conn() (foreign_keys is per- connection, so the PRAGMA must hit the same conn that opens BEGIN), and classifies pragmas: foreign_keys=OFF runs BEFORE BEGIN, foreign_keys=ON runs AFTER COMMIT. If a migration disables FKs but never re-enables them, the runner emits PRAGMA foreign_keys = ON itself so the pool conn doesn't leak foreign_keys=OFF. Regression coverage in migration_atomicity_test.go: a deliberately- failing multi-statement migration must NOT record a schema_migrations row AND its partial DDL must roll back, on both SQLite and Postgres. A separate test exercises the PRAGMA-lift path with a real FK rebuild. * fix(store): restore PRAGMA foreign_keys=ON on every exit path (IDEA-1485 P2) Codex R1 caught a P2 in applySQLiteMigration: once `PRAGMA foreign_keys = OFF` exec'd successfully on the pinned conn, any subsequent failure (BeginTx, execMulti, INSERT INTO schema_migrations, Commit, or a post-tx PRAGMA error) returned early before the success-path FK-restore block could run. The conn went back to the pool with foreign_keys=OFF, silently bypassing enforcement for whichever caller next checked it out. Replace the post-success restore block with an inline `defer` registered the moment a before-tx `foreign_keys=OFF` lands on the connection. The defer fires on every return path. The restore is best-effort: a failure is logged via slog.Error (so a real leak isn't silent) but does NOT override the migration's primary error. The migration's own `PRAGMA foreign_keys = ON` (if present) still runs via afterTx on the success path; the deferred re-set is then a harmless idempotent no-op. Drop the now-redundant `disabledFKs` / `reEnabledFKs` tracking and the unused `isForeignKeysOn` helper. Regression test `TestMigrationAtomicity_FailedSQLite_RestoresForeignKeysOnError` pins the pool to a single connection via SetMaxOpenConns(1), runs a migration that disables FKs and then fails on a duplicate PK, and asserts that on a follow-up checkout `PRAGMA foreign_keys` returns 1. Verified the test FAILS when the defer is removed.	2026-05-15 23:57:38 -04:00
xarmian	312cf06ce5	fix(web): merge defaults in parseSettings/parseSchema (IDEA-1487) (#564) ... * fix(web): merge defaults in parseSettings/parseSchema on successful parse (IDEA-1487) parseSettings and parseSchema only merged defaults in the catch branch. Post-PR #562 migration backfilled NULL collections.settings to '{}', so JSON.parse succeeds and returns a bare object — downstream consumers read settings.layout as undefined (rendering 'layout-undefined') and schema.fields.find as a TypeError on any collection with bare '{}'. Merge SETTINGS_DEFAULTS / SCHEMA_DEFAULTS into the parsed object in both branches. Explicit user-supplied fields still override defaults. Note: QuickActionsMenu spreads parseSettings() back to the wire on edit, so first quick-action save on a previously-bare collection now persists {layout:'balanced', default_view:'list'} alongside quick_actions. Left as-is — defaults migrating to wire is harmless and matches what the UI was already rendering. Reviewer flag, not a regression. * fix(web): fresh defaults per parse call to avoid shared mutable state (IDEA-1487 R1) The module-level SCHEMA_DEFAULTS / SETTINGS_DEFAULTS consts introduced in 8c177d0 hold a `fields: []` array that is copied by reference under shallow spread. Any caller that mutates `.fields` in place (push/splice/sort) on a parsed result that fell through to the default would pollute the shared array for every subsequent parseSchema call. No current caller mutates, so this is latent — but defense-in-depth at the exact boundary IDEA-1487 exists to harden. Switch to factory functions that return a fresh object (with a fresh nested array) per call. * fix(web): fresh array on getTerminalOptions fallback (IDEA-1487 R2) getTerminalOptions returned the module-level DEFAULT_TERMINAL_STATUSES array by reference on the fallback path. Same shared-mutable-state hazard as R1's parseSchema fix — latent today (only consumer iterates), but a defense-in-depth gap at the same boundary. Spread on return so each caller gets a fresh array.	2026-05-15 21:44:18 -04:00
xarmian	7e37dfc34e	refactor(store): collections.settings boundary normalization + scan revert (IDEA-1484 follow-up) (#563) ... * refactor(store): drop defensive sql.NullString scans on collections.settings (IDEA-1484 follow-up) PR #562 (squash 0766d7e) hardened collections.settings to NOT NULL DEFAULT '{}' at the schema level. The defensive sql.NullString scans introduced by PR #561 (BUG-1482, squash 714da48) and the paired import-side ""→"{}" coercion in ImportWorkspace are no longer load-bearing — the database now enforces the invariant the readers were defensively reconstructing. Reverted sites: - internal/store/collections.go: GetCollection, ListCollectionsMinimal, ListCollections — direct &c.Settings scans. - internal/store/export.go: ExportWorkspace scan + ImportWorkspace coercion. - internal/store/items.go: scanCollectionDoneFilters helper. - internal/store/item_stars.go: buildCollectionDoneContextMap helper. Test changes: - Removed TestExportImportRoundTripWithEmptyStringSettings, whose purpose evaporated with the import-side coercion. The constraint-outcome tests (TestCollectionsSettingsNotNullEnforced, TestCollectionsSettingsDefaultsToEmptyObject) from PR #562 remain — they assert the load-bearing schema invariant. * fix(store): restore import-side settings coercion (codex R1 P1) Codex R1 caught that the prior commit reverted the import-side `""→"{}"` coercion incorrectly. The NOT NULL DEFAULT '{}' schema constraint added by PR #562 only fires when the INSERT omits the settings column — but ImportWorkspace explicitly supplies the value. A legacy bundle or external JSON workspace import whose `collections[].settings` is "" would therefore bypass the default: Postgres rejects "" at JSONB type-validation; SQLite silently stores invalid JSON. The coercion was doing two jobs (BUG-1482 had folded them together): 1. Defending against NULL-materialized-as-"" on read — obsolete now that the column cannot hold NULL. 2. Defending against legacy/external "" settings on the import boundary — still required because schema constraints don't validate JSON. Job #1's defense (the sql.NullString scans) stays reverted; the column cannot hold NULL. Job #2's defense (the import-side coercion) is restored and renamed in the comment to reflect that it's a boundary normalizer for external data, not a transitional NULL-handler. `TestExportImportRoundTripWithEmptyStringSettings` is restored with an updated comment that makes the boundary-normalization framing explicit. The constraint-outcome tests from PR #562 remain unchanged. Tests pass on both drivers (SQLite full ./..., Postgres internal/store + internal/server). * fix(store): coerce empty-string settings in UpdateCollection (codex R2 P2) Codex R2 surfaced UpdateCollection as the last writer path in the collections.settings contract that didn't enforce the JSON-validity invariant. A PATCH sending {"settings": ""} would write the empty string verbatim, bypassing the NOT NULL DEFAULT '{}' constraint (which only fires on column omission). Same failure mode as the ImportWorkspace bug R1 caught: Postgres rejects "" at JSONB type-validation, SQLite silently stores invalid JSON. Mirrors the boundary normalization restored in ImportWorkspace at 6f22f94. Closes the contract loop for collections.settings — every writer path (CreateCollection via json marshalling, ImportWorkspace, UpdateCollection) now enforces JSON validity at the API boundary. Added TestUpdateCollectionCoercesEmptyStringSettings to guard the boundary. Also corrected a stale comment in TestCollectionsSettingsDefaultsToEmptyObject that referenced GetCollection's removed defensive scan. The sibling-table UPDATE paths (UpdateItem at items.go:1442, UpdateView at views.go:152) have the same defect class on items.fields/tags and views.config; they are pre-existing, not introduced by this PR, and are tracked in the sibling-table follow-up IDEA.	2026-05-15 19:57:22 -04:00
xarmian	0766d7ecf1	feat(store): enforce NOT NULL on collections.settings (IDEA-1484) (#562) ... * feat(store): enforce NOT NULL on collections.settings (IDEA-1484) Adds migration 055 (SQLite) and pg 034 (Postgres) to backfill any NULL collections.settings rows to '{}' and then enforce NOT NULL DEFAULT '{}' at the column level. Eliminates the bug class that BUG-1482 / PR #561 plugged defensively in the four reader sites. SQLite uses the standard table-rebuild recipe (PRAGMA foreign_keys=OFF, copy via COALESCE, RENAME, recreate the single dependent index from 032_permission_indexes.sql). Same PK values are preserved so FKs in items, views, collection_access, and grants remain valid. Postgres uses the simple in-place ALTER TABLE; SET DEFAULT is a no-op belt-and-braces since 001_initial.sql:115 already had DEFAULT '{}'. The defensive sql.NullString scans in collections.go / export.go and the import-side ""→"{}" coercion in export.go remain in place — they revert in a separate follow-up PR after this migration ships everywhere. Removes the four BUG-1482 NULL-only regression tests from collections_test.go (their `UPDATE collections SET settings = NULL` setup is now a hard write error against the new constraint and the NULL-scan branch they guarded is no longer reachable). Reworks TestExportImportRoundTripWithNullSettings into TestExportImportRoundTripWithEmptyStringSettings — it now mutates the exported bundle in-memory to carry the "" sentinel rather than forcing a NULL row, still exercising the import-side coercion path that survives this PR. * test(store): cover collections.settings NOT NULL outcome (IDEA-1484) Addresses Codex R1 P2: the migration test surface lacked direct constraint-check coverage. Adds two focused outcome tests against the post-migration schema: - TestCollectionsSettingsNotNullEnforced — raw INSERT with settings=NULL must fail. Error shape differs across SQLite (NOT NULL constraint failed) and Postgres (SQLSTATE 23502); we only assert err != nil. - TestCollectionsSettingsDefaultsToEmptyObject — raw INSERT omitting the settings column entirely must materialize the column DEFAULT as the Go string "{}" when read back via GetCollection. Same assertion on both drivers; the defensive sql.NullString scan + Postgres JSONB normalization both surface "{}". Both tests reuse createTestWorkspace + the testStore harness, so they run automatically on whichever driver the test invocation selects. R1 P1 (migration runner atomicity) is out of scope per established codebase precedent (022, 025 use the same pattern); will be filed as a follow-up IDEA.	2026-05-15 18:47:01 -04:00
xarmian	714da48442	fix(store): handle nullable collections.settings end-to-end (BUG-1482) (#561) ... * fix(store): make ListCollectionsMinimal Postgres-safe (BUG-1482) `COALESCE(settings, '')` failed at planner time on Postgres because `collections.settings` is JSONB and `''` is not valid JSON (SQLSTATE 22P02). The query failed regardless of row contents; SQLite is type-loose and accepted it, leaving the bug latent in the two production callers (`handlers_dashboard.go`, `handlers_items.go`). Switch the query to a plain `SELECT ... settings ...` and scan into `sql.NullString`, materializing NULL as the empty-string sentinel. This preserves the existing contract that downstream consumers (`buildDoneContextMap`, `ListCollections`'s own scan loop) gate on via `if c.Settings != ""`, so no caller-side changes are needed. Adds two regression tests in `collections_test.go` that exercise the NULL-settings case (the planner-time failure mode) and the happy-path JSON round-trip. Both run against SQLite and Postgres via the existing PAD_TEST_POSTGRES_URL switch in `testStore`. * test(store): tighten ListCollectionsMinimal happy-path assertion (BUG-1482) Codex review round 1 flagged TestListCollectionsMinimalReturnsSettingsJSON as too permissive: `Settings != ""` would pass for `{}` or any wrong JSON payload. Postgres JSONB also normalizes formatting/key order, so a string compare against the input literal would be brittle across drivers. Switch to a semantic compare: unmarshal both sides into map[string]any and reflect.DeepEqual. This actually verifies the JSON round-trips through the (now fixed) ListCollectionsMinimal path on both drivers. * fix(store): NULL-safe settings scan in GetCollection / ListCollections / ExportWorkspace (BUG-1482) Round-2 extension of the same fix shape. Direct `Scan(... &c.Settings ...)` into a Go string fails on Postgres for any row holding a real NULL with "Scan error: converting NULL to string is unsupported". The column is nullable on both drivers (TEXT DEFAULT '{}' / JSONB DEFAULT '{}'), so legacy or manually-poisoned rows can 500 every handler that goes through these readers — `GetCollection` is the hot reader on every item handler, `ListCollections` powers dashboard + sidebar, `ExportWorkspace` crashes the export pipeline before any data is emitted. Same fix as ListCollectionsMinimal: scan into sql.NullString, materialize NULL as "" to preserve the existing sentinel contract that downstream consumers gate on via `if c.Settings != ""` (handlers_dashboard.go:247, handlers_items.go:1626, collections.go:196 in ListCollections's own post-scan loop). Audited; no caller depends on a non-empty default. Adds TestGetCollectionHandlesNullSettings, TestListCollectionsHandlesNullSettings, and TestExportWorkspaceHandlesNullSettings — each forces a NULL via direct UPDATE (bypassing CreateCollection's empty→`{}` coercion) and asserts the function returns without error and surfaces "" downstream. All pass on SQLite and Postgres. * fix(store): coerce empty-string settings to {} on workspace import (BUG-1482) The earlier commits in this PR made ExportWorkspace, GetCollection, and ListCollections all return `""` for a NULL `collections.settings` row, preserving the in-process sentinel contract that downstream consumers (buildDoneContextMap and friends) already gate on via `c.Settings != ""`. That fix surfaced a paired contract gap: ImportWorkspace previously inserted `c.Settings` verbatim into the collections table. After the reader fixes, an exported NULL-settings row materializes as `""` in the bundle, which Postgres's JSONB column rejects at INSERT time. Without this commit, exporting a workspace with any NULL-settings row and re-importing it would have crashed on Postgres — turning one half of a symmetric contract green while leaving the other half broken. Mirror the same coercion CreateCollection applies on the normal create path: when the bundle's settings field is the empty-string sentinel, write `"{}"` instead. Add a round-trip regression test (TestExportImportRoundTripWithNullSettings) that NULL-poisons a workspace's settings, exports, re-imports, and asserts the re-imported collections hold valid JSON. Verified on both drivers. * style(store): rewrite doc comment to avoid gofmt apostrophe-pair rewrite Go 1.19+ gofmt's doc-comment formatter collapses `''` (two ASCII apostrophes) inside backtick code spans into a single `”` (U+201D right double quotation mark) — a typographic-pair heuristic that doesn't quite fit when the literal pair is the load-bearing thing being described (here: SQL's empty-string literal in COALESCE). CI's golangci-lint flagged the file as gofmt-dirty for this reason. Rewrite the prose to describe the bug without using `''` literally: "coalesced settings against an empty SQL string literal" reads more clearly than `COALESCE(settings, '')` becoming `COALESCE(settings, ”)` after gofmt normalization. Functionally identical comment; lint-clean.	2026-05-15 17:22:57 -04:00
xarmian	7c663a3d3f	feat(collections): add blank workspace template + retire auto-upgrade hook (IDEA-1479) (#560) ... * feat(collections): add blank workspace template (IDEA-1479) Introduces a `blank` workspace template that seeds only the two system collections (Conventions, Playbooks) — no Tasks/Ideas/Plans/Docs, no seeded items, no starter conventions or playbooks. Solves the agent-self / non-template-fit use case where the existing software templates leave undeletable ghost collections in the workspace. Adds a new `CategoryCustom` ("Custom") top-level category so the blank template doesn't mis-group with `startup` / `scrum` / `product`. Category is appended last in `CategoryOrder` so it doesn't displace recommended-path templates in the picker. Tests: - TestBlankTemplateShape — exactly 2 system collections, no seeds. - TestBlankTemplateExcludesSoftwareCollections — no tasks/ideas/plans/docs. - TestBlankTemplateAppearsInPicker — surfaces under a Custom group. - TestSeedFromBlankTemplate — bootstrapping produces 2 collections, 0 items. * fix: address codex review for blank template (IDEA-1479) - CreateWorkspaceModal: remove hard-coded 'blank' picker entry that silently fell through to collections.Defaults(). The API-driven blank template (under the Custom category) is now the canonical surface. - Dashboard: gate '+ New Task' button on tasks collection existence so blank workspaces don't render a button that targets a missing collection. - OnboardingChecklist: accept collectionSlugs prop and filter steps whose target collection (plans/tasks/docs) is absent. Conventions step remains unconditional since the conventions collection ships with every template, including blank. Empty-steps guard added to progressPct to avoid NaN. - web/src/lib/utils/templates.ts: add 'custom' -> 'Custom' to mirror the Go CategoryOrder + categoryLabels updates. - cmd/pad/templates_picker_test.go: extend the visible-template assertion list to include 'blank' and assert the Custom category header renders. * fix(store): gate SeedDefaultCollections on zero-collection workspaces (IDEA-1479) The server's startup auto-upgrade hook (cmd/pad/main.go) called SeedDefaultCollections against every workspace at boot. That hook dates to the initial release — long before workspace templates existed — and was written as a backfill for workspaces created before tasks/ideas/plans/docs landed in Defaults(). Post-templates, the hook unconditionally re-materialized the Software-template collections into any workspace missing them — including blank-template workspaces (IDEA-1479), which ship only Conventions + Playbooks by design. Result: every restart silently regrew the ghost user-facing collections the blank template was explicitly built to avoid. Fix: SeedDefaultCollections now returns nil immediately when the workspace has any existing collection (system or user-facing). The rescue path still triggers for genuinely-empty workspaces, preserving the original backfill intent. Tests: - TestBlankWorkspaceSurvivesSeedDefaultCollections — blank workspace remains 2 collections after auto-upgrade (and after a second pass). - TestEmptyWorkspaceStillGetsDefaults — zero-collection workspace still gets the full Software default set. * refactor(server): remove SeedDefaultCollections auto-upgrade at startup (IDEA-1479) The startup auto-upgrade hook in cmd/pad/main.go dated to the initial release, predating workspace templates entirely. Its original intent was per-collection backfill — workspaces created before a new entry landed in Defaults() would acquire it on next boot. Post-templates, that semantic is incompatible with templates that legitimately diverge from Defaults() (e.g. `blank`, which ships only Conventions + Playbooks by design). Round-2 of the IDEA-1479 review attempted to keep the hook by adding a "zero collections" guard, but Dave (after codex round 3) decided the cleanest fix is removing the hook entirely. The codebase has proper migration infrastructure now; any future "add a default collection" work should land as an explicit migration where the author chooses which workspaces to backfill. SeedDefaultCollections itself is preserved (with the round-2 guard) as a building block for any future explicit rescue command or migration. Its doc comment is updated to note it's no longer auto-invoked at startup. The round-2 regression tests (TestBlankWorkspaceSurvivesSeedDefaultCollections, TestEmptyWorkspaceStillGetsDefaults) still apply and pass unchanged. * fix(store): rescue gate uses COUNT(*), not ListCollectionsMinimal (IDEA-1479) Postgres CI on PR #560 caught a regression introduced in commit 3e71fe8: SeedDefaultCollections's zero-collection guard called ListCollectionsMinimal, whose SELECT uses COALESCE(settings, '') against a JSONB column. Postgres parses the '' literal as JSON at plan time and fails with SQLSTATE 22P02 (invalid input syntax for type json), breaking the rescue gate and ~12 cascade test fixtures that depend on the seeder succeeding. The gate only needs to know whether any collection exists, not their schema or settings. Switch to a direct COUNT(*) on the collections table: portable across both drivers, cheaper than the minimal lister, and avoids the broken JSON COALESCE path entirely. Verified locally against both drivers: - SQLite (default): go test ./... — all PASS - Postgres (make test-pg infra): PAD_TEST_POSTGRES_URL=... go test ./... — all PASS, including the three direct failures (TestBlankWorkspaceSurvives…, TestEmptyWorkspaceStillGetsDefaults, TestSeedDefaultCollections) and the cascade FTS/search fixtures. Note: ListCollectionsMinimal's COALESCE(settings, '') expression appears to also affect production callers (handlers_dashboard, handlers_items) on Postgres, but fixing that is out of scope for this PR — those paths have their own tests that aren't failing in CI. Flagged for separate follow-up.	2026-05-15 14:46:26 -04:00
xarmian	be68292e03	fix(store): workspace list freshness reflects item activity (BUG-1481) (#559) ... `pad workspace list` was showing `workspaces.updated_at`, which only moves on workspace-row mutations (rename, settings, members). After this fix the effective UpdatedAt surfaces item activity inside the workspace — answering "where is work happening?" instead of "when was this row last UPDATEd?". Implementation: scalar `MAX(items.updated_at)` subquery in `ListWorkspaces` and `GetUserWorkspaces`, then `effectiveWorkspaceUpdatedAt` picks the later of the two timestamps (portable across SQLite + Postgres, no GREATEST). Read-time approach per the bug's design notes. Visibility-aware: codex review surfaced that a naive MAX leaks activity timing for items the caller can't see. The member subquery mirrors `VisibleCollectionIDs` (`collection_access='all'` short-circuits; for `'specific'` members, system collections + `member_collection_access` + `collection_grants` + `item_grants` gate visibility). The guest subquery limits MAX to items reachable via `collection_grants` / `item_grants`. All grant lookups are workspace-scoped for defense-in-depth. Four regression tests cover: admin `ListWorkspaces`, member all-access, member specific-access leak guard, and guest leak guard.	2026-05-15 12:30:07 -04:00
xarmian	d3bd1958c5	feat(web): source_url ghost-field + Refresh from source affordance (TASK-1474) (#558) ... * feat(web): source_url ghost-field + Refresh from source affordance (TASK-1474) Final slice of PLAN-1467 — wires the editor's Insert-from-URL modal to a source_url + imported_at ghost-field stamp and adds a refresh affordance. Editor.svelte: - New onImportInserted prop. Forwarded to ImportFromUrlModal's onInserted so the host page learns when content was spliced in. Item editor page: - handleImportInserted(meta): only stamps when (a) item had no prior content AND (b) source_url is not already set, matching PLAN-1467's design rule. Stamping calls api.items.update with {fields: JSON.stringify({...fields, source_url, imported_at})}. source_url + imported_at are orphan keys — internal/items/validate.go only iterates declared schema fields, so unknown keys round-trip through PATCH without migration. - refreshFromSource(): a small button beneath the title, visible only when fields.source_url is set and the user has write access. Confirms with a window.confirm warning (diff-preview deferred per PLAN risks section; Yjs op-log provides recoverable history), re- fetches via api.importURL, replaces editor content via selectAll().deleteSelection().insertContent(html), and bumps imported_at. View-only users see a non-interactive chip that shows the import provenance without the refresh action. Both Editor mounts in the page (read-only and collab-editable branches) pass onImportInserted={handleImportInserted}. * fix(web): hide Refresh button in raw-markdown mode per Codex review (round 1) P2: In raw-markdown mode the rich Editor is unmounted and replaced by RawMarkdownEditor, but the parent retains a stale Tiptap editor instance from the previous mount. Refresh-from-source drives content replacement through that instance, so clicking it in raw mode either failed silently or updated an off-screen editor while the visible raw textarea stayed stale. Fix: gate the interactive Refresh button on `canEdit && !rawMode`. Read-only users AND raw-mode users now see the non-interactive provenance chip — they can still discover the import history but can't trigger a refresh from the inappropriate context. Switching back to rich mode re-enables the button. * fix(web): capture item identity across refresh await per Codex review (round 2) P1: If the user clicked Refresh from source and navigated to a different item before api.importURL() returned, the continuation would replace the NEW item's editor content with the OLD item's markdown AND stamp the OLD source URL onto the NEW item via stampSourceUrl. Both surfaces awaited the fetch without snapshotting the item / editor at call time. Fix in two places: - refreshFromSource: capture `targetItem = item` and `targetEditor = editorInstance` before any awaits; after the importURL await, bail if the live item.id no longer matches OR the editor instance was swapped (item navigation re-mounts the Editor with a new instance). Toast and spinner-clear are also gated on the identity match so the user who navigated away sees the destination item's UI, not stale feedback. - stampSourceUrl: capture `targetItem` + `targetWs` before the PATCH and gate the assignment to `item` on identity. Also gates the failure toast so a stamp on the wrong workspace doesn't surface an "imported, but source_url not saved" toast on an unrelated item. * fix(web): always clear `refreshing` in finally per Codex review (round 3) P2: The previous identity-guard fix only cleared `refreshing = false` when the live item still matched targetItem. Since the route component is reused across item navigation and loadData() doesn't reset `refreshing`, navigating away during an in-flight refresh left `refreshing = true` persisted on the page-level state. Opening any other item with a source_url showed a stuck "Refreshing…" label and a permanently-disabled refresh button. Fix: clear `refreshing` unconditionally in the finally. Per-item visual feedback is only meaningful while the user stays on the originating item; a navigation already signals "user moved on", so the spinner state shouldn't persist past it. * fix(web): use editor.isEmpty (live) instead of item.content (stale) for source_url stamp gate per Codex review (round 4) P2: The "stamp source_url only when item had no prior content" check read from `item.content`, which is the DATABASE snapshot — under collab the editor's authoritative state lives in the Y.Doc and isn't flushed to item.content until the debounced save fires. A user could type into a newly blank item, open Insert from URL before the autosave landed, click Insert, and the page would mark the (already mixed) document as source-backed and enable the destructive "Refresh from source" affordance over their typing. Fix: - ImportFromUrlModal: capture `editor.isEmpty` BEFORE insertContent runs, pass it via a new `InsertContext { wasEmpty: boolean }` argument on the `onInserted` callback. Reading isEmpty post- insert would always be false because we just added content. - Editor.svelte: update the onImportInserted prop signature to forward the InsertContext. - Page handleImportInserted: use ctx.wasEmpty instead of checking item.content. The previously-empty + not-already-stamped rule is preserved; only the source of "was empty" changes. Editor.isEmpty consults the live ProseMirror doc, which under collab reflects the Y.Doc state — so this is correct in both single-user and collab modes. * fix(web): namespace ghost-fields under pad_ prefix + narrow stamp race per Codex review (round 5) Two findings addressed: P2 #2: source_url collision with collection schema fields. Renamed the ghost-field keys to `pad_source_url` and `pad_imported_at` so they cannot collide with a user-defined `source_url` field on the collection schema. Every read site (handleImportInserted's already- stamped check, refreshFromSource, the chip's render gate + title, the page title-row block) now reads from the prefixed keys. P2 #1: race between concurrent field PATCHes. The `updateField` and `stampSourceUrl` paths both PATCH the full `fields` JSON blob, so a user field edit landing concurrently with our stamp would silently overwrite one of the two changes. Cannot be fully fixed without a server-side partial-fields update (a bigger refactor — tracked in IDEA-1480). Mitigation here: - stampSourceUrl now re-fetches the item with api.items.get just before the PATCH and merges its two keys onto the freshest server snapshot. This narrows the window from "between read and PATCH-land" to "between fetch and PATCH-land" (typically <100 ms). - In-code comment cites IDEA-1480 so future readers know the inherent race exists and where to track the system-wide fix. The existing project-wide updateField path has the same race inherent to the bulk-PATCH design; it'll be closed by IDEA-1480 when the partial-update API lands. * fix(web): reserve pad_ field-key prefix to prevent user-defined collision per Codex review (round 6) P2: The pad_source_url / pad_imported_at orphan keys introduced in round 5 are still user-definable in collection schemas. A field labelled "Pad Source URL" auto-generates pad_source_url through slugifyKey, shadowing the import-provenance metadata. Once shadowed, the destructive "Refresh from source" chip would render for ordinary user data and stampSourceUrl would overwrite the user's field on import. Fix: extend the UI-level field-key validator in field-editor-types.ts to reject any key starting with the RESERVED_FIELD_KEY_PREFIX = "pad_". The two known reserved keys (pad_source_url, pad_imported_at) are also enumerated explicitly in RESERVED_FIELD_KEYS so the failure message points to them by name when slugifyKey happens to produce one. Future Pad-managed orphan keys can land under the same prefix without retroactively breaking existing collections.	2026-05-15 02:15:47 -04:00
xarmian	3a2ee6a45d	feat(web): Insert from URL — TipTap toolbar button + modal (TASK-1473) (#557) ... * feat(web): Insert from URL — TipTap toolbar button + modal (TASK-1473) Wires the editor to POST /api/v1/import/url from TASK-1472. Pieces: - ImportURLResponse type + api.importURL() in lib/api/client.ts. - ImportFromUrlModal.svelte — focus-on-open URL input, fetch button, preview pane with detected-type tag (OpenAPI / Generic) + title + source_url, Insert / Cancel footer. ESC and backdrop click close. Insert converts markdown → HTML via the project's existing `marked` renderer (same shape the editor uses for setContent on load), then insertContent(html) splices at the cursor. - EditorToolbar.svelte — new 🌐 button in the blocks group opens the modal. Optional onImportInserted callback bubbles the response metadata so the parent (the item editor page in TASK-1474) can stamp source_url / imported_at into the item's fields. Validation: light client-side URL parse + scheme check before hitting the server. The server's canonical SSRF guard is the authority. Toast feedback on successful insert via toastStore.show('...', 'success'). Parent: PLAN-1467. * fix(web): wire ImportFromUrl into Editor's slash menu + race guard per Codex review (round 1) P1: EditorToolbar.svelte is unused legacy — the live editor mounts Editor.svelte directly with a slash-command UI. The previous diff added a toolbar button no user could reach. Now: - Revert EditorToolbar.svelte to its pre-PR state. - Add `importUrl` block type to block-types.ts (insertOnly so it appears in the slash menu but not in the "Turn into" menu). - Editor.svelte's execSlash handles the new case by setting `importUrlModalOpen = true`; the modal is mounted at the bottom of the editor template. The slash command surfaces via type "/url", "/fetch", "/web", "/openapi", "/import", or "/page". P2: closing or re-fetching during an in-flight request previously let a stale response land on a fresh modal session. Now a monotonic `requestGen` counter is bumped on (a) every new fetch start, (b) every cancel, and (c) every reopen via the open effect. handleFetch captures its generation before await and drops both the response and the error if requestGen has advanced past it.	2026-05-15 00:53:00 -04:00
xarmian	e621eacb9b	feat(server): POST /api/v1/import/url endpoint + integration tests (TASK-1472) (#556) ... Wires internal/urlimport into the API: Fetcher → Detect → converters. Side-effect-free; the editor's "Insert from URL" modal owns any item mutation (TASK-1474). Endpoint: - POST /api/v1/import/url, body {"url"}, response {markdown, detected_type, title?, source_url, fetched_at, content_type}. - Status mapping: 400 invalid URL / SSRF / malformed body; 502 upstream failure (incl. size cap); 504 fetch timeout; 422 conversion failure. - Swagger 2.0 fallback: detected as "openapi" by the sniffer but rejected by ConvertOpenAPI → falls through to ConvertGeneric and re-classifies the response as "generic" so the UI shows the right affordance. - 30s wall-clock budget on the whole pipeline via context.WithTimeout (Fetcher's own timeout is the HTTP-level cap). Package-level Fetcher is memoized via the existing sync.Once-cached safe transport (shared keep-alive pool, no per-request leak). The handler skips the pre-flight ValidateURL when the package fetcher has AllowLocal=true so tests can swap in a loopback-friendly fetcher without bypassing the production guard. Integration tests (handlers_import_test.go): - HTML happy path (httptest upstream, asserts detected_type/title/ source_url/fetched_at/content_type all wired up). - OpenAPI 3.x happy path (inline YAML upstream, asserts ConvertOpenAPI was invoked and detected_type=openapi). - Swagger 2.0 fallback (asserts detected_type re-classified to generic, markdown non-empty). - SSRF rejection (default fetcher, loopback URL → 400 mentioning "private"/"reserved"). - file:// scheme rejected (400). - Missing/malformed body rejected (400). - Upstream 5xx surfaces as 502. - Size cap exceeded surfaces as 502. - No-side-effects check: item count unchanged before/after import. Parent: PLAN-1467.	2026-05-15 00:36:41 -04:00
xarmian	8771f95ab2	feat(urlimport): OpenAPI 3.x → Markdown converter (TASK-1471) (#555) ... * feat(urlimport): OpenAPI 3.x → Markdown converter (TASK-1471) Adds ConvertOpenAPI to internal/urlimport — the "openapi" branch of the v1 importer. Built on pb33f/libopenapi. Layout: - H1 with the API title + version + description - Contact + License lines - Servers list - Endpoints section grouped by primary tag (or "Other" for the untagged). Per operation: `METHOD /path` heading, summary, description, deprecation marker, operation ID, parameter table, request-body summary (with media-type fences and YAML-rendered example), and response code table. - Schemas section with component schemas as Property/Type/Required/ Description tables, schema names sorted for stable output. Scope: - OpenAPI 3.x only. Swagger 2.0 detection returns an explicit "only 3.x" error so the import endpoint (TASK-1472) can fall through to the generic converter. - Recoverable libopenapi build errors (unresolved refs, etc.) are swallowed when the model is still produced — partial spec > no output. Tests: - testdata/petstore-openapi.yaml — full v3 fixture: tags, params, requestBody example, deprecated op, ref-typed schema array, two component schemas with required-field markers. - TestConvertOpenAPI_Petstore — 30+ markdown-substring assertions on the rendered output. - TestConvertOpenAPI_RejectsSwagger2 — explicit v2 error. - TestConvertOpenAPI_RejectsGarbage — non-spec input. - TestConvertOpenAPI_MinimalSpec — empty paths short-circuits. - Helpers: schemaTypeBrief(nil), escapeTableCell, singleLine. Dependency: github.com/pb33f/libopenapi v0.36.3 (MIT-licensed). Parent: PLAN-1467. * fix(urlimport): merge path-level + operation-level parameters per Codex review (round 1) MEDIUM: OpenAPI path-item-level parameters apply to every operation on the path. Previously only slot.op.Parameters was rendered, so common specs that hoist a shared {id} parameter to the path-item level emitted operations with the path parameter missing from the docs. Now opSlot carries item.Parameters as pathParams, and a new mergeParameters helper produces the spec-conformant union: - Path-level parameters first, in declared order. - Operation-level parameters with matching (name, in) override the path-level entry in place. - Operation-only parameters appended after. Tests: - TestConvertOpenAPI_PathLevelParametersMerged — inline fixture with a path-level widgetId + trace and an operation-level trace override + fields op-only param. Asserts widgetId survives, trace shows op-level (required=yes), no duplicate path-level trace row, fields appears. - TestMergeParameters_EmptyInputs — nil/nil short-circuit. * fix(urlimport): no double-backticks on array-of-ref schema types per Codex review (round 2) MEDIUM: schemaTypeBrief() previously wrapped refs in inline backticks ("`Pet`"). For array-of-ref schemas the brief became "array of `Pet`", and the table-cell call site (codeOrBlank) then wrapped the entire value in another pair, producing broken markdown like "`array of `Pet``". Schema properties whose type is an array of a component schema are a normal OpenAPI shape — `Litter.pets: array of Pet` — so this would have hit real specs immediately. Fixes: - schemaTypeBrief now returns plain text — ref names without surrounding backticks. Docstring updated to make the contract explicit ("never contains backticks; caller wraps"). - codeOrBlank strips any stray backticks from input before wrapping so the resulting cell always carries exactly one balanced pair. Defensive: the contract from schemaTypeBrief is plain text now, but stray backticks from any future caller can't corrupt the table. Tests added: - TestConvertOpenAPI_ArrayOfRefTypeCell — inline spec with a `Litter.pets: array of Pet` property. Asserts the type cell is exactly `` `array of Pet` `` and no malformed variants leak. - TestCodeOrBlank — 7-case table covering empty, plain, whitespace, pre-backticked, embedded-backtick, and backtick-only inputs.	2026-05-15 00:24:14 -04:00
xarmian	d1560606cb	feat(urlimport): generic HTML→Markdown converter (TASK-1470) (#553) ... * feat(urlimport): generic HTML→Markdown converter (TASK-1470) Adds ConvertGeneric to internal/urlimport — the v1 catch-all converter for "non-OpenAPI" URLs. Pipeline: 1. go-shiori/go-readability strips chrome/nav/ads/scripts and returns the page's primary article. 2. JohannesKaufmann/html-to-markdown/v2 converts the cleaned HTML to markdown. 3. cleanupMarkdown normalizes line endings, trims trailing whitespace, collapses blank-line runs, and ensures a single trailing newline. Fallback path: when Readability cannot identify an article (directory listings, single paragraphs, pages with no clear content container), the converter falls back to a whole-body conversion so callers still get usable markdown. Dependencies (license-checked): - github.com/JohannesKaufmann/html-to-markdown/v2 v2.5.1 (MIT) - github.com/go-shiori/go-readability (Apache-2.0) Fixtures + tests: - testdata/availity-shape.html — Availity-style div soup with heavy chrome (nav, ads, sidebar, footer, analytics script). Asserts the article content survives and the chrome is stripped. - testdata/mdn-shape.html — MDN-style semantic HTML (article/main + proper heading levels + code fences). Asserts structure preserved. - TestConvertGeneric_EmptyBody — empty-input rejection. - TestConvertGeneric_PlainTextFallback — Readability-can't-find-article fallback path. - TestCleanupMarkdown — 5-case table-driven cleanup verification. Parent: PLAN-1467. * fix(urlimport): preserve hard-line-break markers + apply WithDomain on fallback per Codex review (round 1) - MEDIUM: cleanupMarkdown was stripping the markdown two-trailing- spaces hard-line-break idiom. html-to-markdown emits <br> as " \n" — bulk-stripping trailing whitespace was demoting hard breaks to soft wraps. Now the cleanup steps line-by-line, keeps exactly-two trailing spaces (no tab), strips 1/3+/tab-mixed runs. - MEDIUM: The raw-HTML fallback path (used when Readability cannot identify an article) now passes converter.WithDomain(pageURL) so relative links/images resolve against the source URL rather than the Pad host where they'd 404. Tests added: - cleanupMarkdown: hard-line-break preserved, single/triple trailing spaces stripped, tab-mixed spaces stripped, blank-line-with-spaces collapsed (5 new cases). - TestConvertGeneric_RelativeURLsResolvedOnFallback: relative href in non-article HTML resolves to absolute URL via pageURL.	2026-05-15 00:03:16 -04:00
xarmian	0aa3988319	feat(urlimport): URL fetcher with SSRF guard + content-type detection (TASK-1469) (#552) ... * feat(urlimport): URL fetcher with SSRF guard + content-type detection (TASK-1469) First slice of PLAN-1467's "Insert from URL" feature. Adds the internal/urlimport package with: - fetch.go: SSRF-guarded HTTP GET (10s timeout, 5 MB body cap, redirect re-validation, redacted-error formatting). Blocks loopback, RFC1918, CGNAT, IPv4/IPv6 link-local (incl. 169.254.169.254 cloud-metadata), IPv6 unique-local, and the unspecified address. Hostnames are resolved and every returned IP is checked. - detect.go: Content-type + body-prefix sniff returning "openapi" (JSON or YAML, OpenAPI 3.x or Swagger 2.0) or "generic". Inspects at most 64 KiB. - fetch_test.go: Table-driven SSRF tests covering 24 cases plus happy-path, size-cap, timeout, non-2xx, context-cancel, and a stubbed-transport redirect re-validation. - detect_test.go: 20 detection cases including OpenAPI JSON, Swagger YAML, vendor media types, leading comments, indented-key negatives, and charset-parameter normalization. Package name is urlimport (not "import" — reserved word). No callers yet; the endpoint that consumes Fetcher + Detect lands in TASK-1472. Parent: PLAN-1467. * fix(urlimport): close DNS-rebinding gap + handle >64 KiB OpenAPI JSON per Codex review (round 1) - HIGH: Add safe dialer transport (newSafeTransport). ValidateURL no longer does DNS — the dialer resolves once and validates the resolved IP at dial time, then dials that exact IP. DNS rebinding can no longer slip a public-IP validation past a loopback fetch. ValidateURL becomes a pre-flight (scheme/credentials/IP-literal only) with the canonical guarantee now at the transport layer. - MEDIUM: For JSON bodies over the 64 KiB sniff cap, switch from full Unmarshal (which fails on a truncated tail) to a streaming json.Decoder scan that walks top-level keys and short-circuits as soon as `openapi` or `swagger` is seen. Real-world specs over 64 KiB are now classified correctly, including the case where the `openapi` key is not the first top-level entry. Tests added: - TestFetch_DialerBlocksLoopbackHostname (dial-time rebinding guard) - TestDetect_LargeOpenAPIJSON (>200 KiB OpenAPI body, key first) - TestDetect_LargeOpenAPIJSON_KeyNotFirst (openapi key after huge info) - TestDetect_LargeJSONNotOpenAPI (huge non-OpenAPI stays generic) Removed the DNS-resolution case from TestValidateURL's notes and added a positive case proving hostnames pass the pre-flight (the dial-time check is now the canonical guard). * fix(urlimport): disable env proxy and reuse safe transport per Codex review (round 2) - HIGH: Set Proxy=nil on the safe transport. ProxyFromEnvironment would route via HTTP_PROXY/HTTPS_PROXY, where the dialer connects to the proxy host instead of the target — silently bypassing the hostname-resolution SSRF check inside DialContext. Operators who need an outbound proxy can wire their own trusted transport into Fetcher.Transport. - MEDIUM: Memoize the default safe transport per Fetcher via sync.Once. Previously each Fetch built a fresh *http.Transport whose keep-alive idle-pool stayed in scope until GC, leaking FDs under repeated imports. Now one transport is shared by all Fetch calls on a Fetcher; AllowLocal is captured at first use.	2026-05-14 23:51:44 -04:00
xarmian	b1fcedd5b5	fix(editor): match slash menu against id + keywords (BUG-1419) (#551) ... Typing `/h2` (or `/ul`, `/hr`, `/todo`, etc.) in the tiptap editor auto-closed the slash menu because the filter only matched on `label` and `description`. "Heading 2".includes("h2") is false — the space between "Heading" and "2" breaks the substring match — and zero matches triggers closeSlash(), so the picker vanished as soon as the user typed the second character. Add optional `keywords?: string[]` to BlockType and populate common abbreviations per block (h1/h2/h3, ul/ol, todo/checkbox, hr/rule, quote/bq, code, html, tbl, etc.). Extend getFilteredSlash() to join label + description + id + keywords into a single lowercased haystack and substring-match the query against it. Pure UI filter change — Y.Doc / ProseMirror shape unchanged, no SCHEMA_VERSION bump. Turn-into menu unaffected (no filter there).	2026-05-14 22:22:35 -04:00
xarmian	38aa872864	fix(fields,activity): debounce typed-input field saves + collapse same-field activity runs (BUG-1466) (#549) ... * fix(web): debounce typed-input field saves to stop per-keystroke activity rows (BUG-1466) Text / number / URL fields in FieldEditor wired oninput directly to onchange, so every keystroke became an item PATCH and an activity row. Typing `ui/editor/tiptap` into a `component` field produced a 30-step keystroke chain in the audit metadata (visible on BUG-1419's timeline). Wrap the typed-input branches in a 500ms idle debounce, flush on blur so tabbing away commits immediately, and flush on unmount so navigation never drops a pending value. Discrete inputs (select / date / checkbox, number ±1 buttons) keep firing on the user action — they aren't typing. Mirrors the markdown content debounce pattern in the detail page. * fix(activity): collapse same-field runs in merged changes metadata + unify diff separator (BUG-1466) Follow-up to the web-side typing debounce. Two related changes: 1) collapseChanges() walks the merged "; "-delimited changes string and collapses runs of consecutive same-field entries into a single "field: first-old → last-new". Drops net no-ops (typed then backspaced). When the web-side debounce in FieldEditor still produces multiple PATCHes within the 5-minute coalesce window — or for older rows that pre-date the debounce — the timeline now reads as one transition instead of a chain. Run-based (not global) collapse so interleaved edits on different fields keep their chronology. 2) diffFields now joins entries with "; " instead of ", " so the joiner is consistent with mergeActivityMeta and TimelineActivityCard.svelte's split delimiter. Multi-field PATCHes previously rendered as a single unparseable blob in the web timeline because the parser only split on ";" — fixed as a side-effect. Adds TestCollapseChanges (10 cases including the BUG-1419 repro) and TestMergeActivityMeta_CollapsesSameFieldRun. Updates the existing TestDiffFieldsPrimitives expectation to match the new joiner. * fix(fields,activity): two follow-ups per Codex review (round 1) [P1] FieldEditor.svelte::handleNumberStep The ±1 buttons computed `(Number(value) || 0) + delta` AFTER calling flushPendingSave(). But `value` is the parent prop — flushing fires onchange asynchronously, so at the moment of the step computation the prop still holds the pre-typed value. Typing 10 over 5 and clicking + would flush 10 then send 6, overwriting the typed value. Compute `base` from `pendingValue` (if hasPending) BEFORE clearing the timer state, then send `base + delta` in one onchange call. [P2] activities.go::collapseChanges The drop-net-no-op step removed entries where `from == to`. But diffFields intentionally emits same-display entries for same-cardinality structured-field replacements like `implementation_notes: (1 note) → (1 note)` (see TestDiffFieldsSameCardinalityArrayChangeStillReported) — the labels match because formatChangeValue summarizes by count, not content, but the underlying data did change. Dropping them silently hides real updates from the activity feed. Track `mergedCount` per entry: increment when collapsing a run, initialize to 1 on parse. Only drop when `mergedCount > 1 && from == to` — i.e. only when the no-op resulted from collapsing multiple input segments (the typed-then-backspaced case). Tests: 2 new TestCollapseChanges cases (single structured-field preserved, interleaved structured-field around a typed run). * fix(fields,activity): two follow-ups per Codex review (round 2) [P1] FieldEditor.svelte number stepper focus race The number ±1 buttons race with the input's onblur handler: blur fires before click in the natural focus-transfer flow, so flushPendingSave clears hasPending → handleNumberStep reads stale `value` from the parent prop → typing 25 over 10 and clicking + sends 25 then 11, losing the typed value. Add onmousedown={preventDefault} on both ±1 buttons. Mousedown precedes blur, and preventDefault on mousedown suppresses the natural focus transfer — the input keeps focus through the click, so hasPending survives until handleNumberStep reads it. [P2] collapseChanges still drops repeated same-display structured runs Round 1's mergedCount>1 rule still dropped runs like `implementation_notes: (1 note) → (1 note); implementation_notes: (1 note) → (1 note)` — two real updates whose display strings happen to match because formatChangeValue summarises array-valued fields by count. Each PATCH represented a different underlying note (diffFields uses reflect.DeepEqual to detect that), but the merged display showed no transition. Track `hadTransition` per entry: true iff the run had a display- level transition (initial from != to, or a subsequent entry's `to` differed from the anchored from). Drop only when mergedCount > 1 && from == to && hadTransition — i.e. only true net-cancellations (typed-then-backspaced). Same-display structured repeats stay; real `foo → bar → foo` swings still drop. Tests: 2 new TestCollapseChanges cases (repeated same-display preserved, real foo→bar→foo swing still dropped). * fix(fields,activity): two follow-ups per Codex review (round 3) [P1] FieldEditor cross-item leak via debounce timer When the parent reuses a FieldEditor instance across an item swap (same schema, same field.key, different item — common when navigating between items in the same collection), the parent's `updateField` closure reads `item.id` at CALL time. A pending timer set while item A was active would fire after item B mounted, patching B with A's typed value. Two-pronged fix in FieldEditor: - Add a $effect that tracks the `value` prop and drops any pending save the moment the parent re-props us. The user actively typed for a now-stale context; aborting is safer than silently writing to the new context. Also covers external collab/SSE rebases of the same field on the same item. - Switch the unmount cleanup from flush → drop. When the parent navigates to an item whose schema lacks this field, the FieldEditor unmounts AFTER the parent's `item` has already been replaced, so a final onchange call would route through updateField → wrong item. Blur is the supported commit gesture (clicking elsewhere within the page, ±1 buttons, tab-out — all flush eagerly); unmount-without-blur is treated as "user abandoned the edit." [P2] collapseChanges still dropped structured count-return swings Round 2's hadTransition rule still dropped a run like `implementation_notes: (1 note) → (2 notes); implementation_notes: (2 notes) → (1 note)` — the user added a note then removed the original, ending with a different single note. The merged display reads `(1 note) → (1 note)` with hadTransition=true, indistinguishable from a typed-then- backspaced cancellation. But formatChangeValue summaries are LOSSY: same display label can wrap entirely different raw values. Add a `hasLossySummary` flag per entry — true iff either `from` or `to` matches the `(text)` format formatChangeValue emits for structured fields. Propagate stickily across the run (any lossy entry pins the whole run). Skip the drop step when the run carries lossy summaries — we can't recover the raw delta from the merged string, so we preserve the entry rather than risk hiding a real change. Tests: 2 new TestCollapseChanges cases (count-return swing preserved, lossy-on-one-side still pins the run). * fix(fields): demote hasPending from \$state to plain let — \$effect was cancelling every keystroke (round 4 [P1]) Critical regression in round 3: the value-track \$effect read \`hasPending\` inside its body, which Svelte 5 promotes to a reactive dependency. scheduleSave() setting \`hasPending = true\` retriggered the same \$effect, whose body cleared the typing timer + pendingValue + hasPending before the debounce could fire. Net effect: typing into any text / number / URL field was silently dropped — onchange never ran, the field never saved. hasPending is only read from imperative handlers (scheduleSave, flushPendingSave, handleNumberStep, the value-track \$effect, the unmount cleanup) — never from a template or other reactive context. Demoting it to a plain \`let\` removes the unwanted subscription while preserving the round-3 behaviour: external value-prop changes still trigger the \$effect (it tracks \`value\`), the body reads hasPending imperatively to decide whether to clear pending state. No tests added — this is a Svelte reactivity edge case that can't be unit-tested without a DOM. svelte-autofixer's pre-existing "variable assigned inside \$effect" suggestion previously flagged the hasPending mutation; that signal is gone now. Per Codex review round 4.	2026-05-14 22:10:12 -04:00
xarmian	d7b99de2fb	fix(web): await workspace items before Y.Doc seed so wiki-links don't bake in as text (BUG-1461) (#548) ... The slug page fired collectionStore.loadItems fire-and-forget, racing the Y.Doc seed effect. When the seed ran with an empty items array it fell back to raw markdown, baking literal [[X]] text into the Y.Doc. The seed's fragment.length > 0 gate made the corruption permanent — the seed never re-fires for that item. Fold loadItems into loadData's Promise.all and await it before `item` is set. Gate the call on a new workspace-scoped freshness check (itemsAreFreshFor) rather than items.length, so a stale items array left over from a previous workspace doesn't satisfy the guard. itemsWorkspace is stamped only on full-workspace loads; collection- scoped loads invalidate it so callers needing the full workspace correctly re-fetch. Existing items whose Y.Doc was already baked stay broken until edited and re-saved (markdownToWikiLinks rewrites them on the save round-trip). Verified by Codex second-opinion review.	2026-05-14 21:11:52 -04:00
xarmian	438cb6180a	fix(mcp): flexible JSON shapes on item create + clearer field surface (BUG-1431, BUG-1432) (#547) ... * fix(mcp): flexible JSON shapes on item create + clearer field surface (BUG-1431, BUG-1432) BUG-1432 root cause (real): models.ItemCreate.Tags is a Go string, so the default unmarshaler rejected the natural JSON-array shape every agent sends (`tags: ["foo","bar"]` → "cannot unmarshal array into Go struct field ItemCreate.tags of type string", HTTP 400). On Postgres the alternative — passing `tags: "foo,bar"` per the catalog's old "Comma-separated tags" description — landed as a non-JSON value in the JSONB column and surfaced as a generic HTTP 500. SQLite's TEXT column silently accepted the corrupt value, which is why local repros didn't show it. Codex's independent investigation called out the asymmetry: ItemUpdate already had a flexible UnmarshalJSON for `fields`/`tags` per BUG-1144, but ItemCreate didn't. This PR mirrors that flexibility on the create path and aligns the MCP surface description with reality. BUG-1431 root cause (real, not the misdiagnosis the agent reported): the dispatcher's `parseFieldKVP` only accepted the CLI-style array-of- "key=value" shape, rejecting the JSON-native `field: {key: value}` map shape with "expected array or string, got map[string]interface {}". Agents naturally try the map shape and got a non-actionable error; that drove the BUG-1409 agent to mis-blame status placement. Empirical repro confirmed that `status` actually works in both top-level AND inside-fields positions today (Tests 1, 4 in the investigation); the real surface problem was the missing map shape on `field`. Changes: - internal/models/item.go: add UnmarshalJSON to ItemCreate mirroring ItemUpdate's BUG-1144 pattern. Accepts `fields` as object or JSON-encoded string; `tags` as array or JSON-encoded string; either field absent / null leaves Go zero value. Wrong shapes surface ErrInvalidFieldsType / ErrInvalidTagsType (existing sentinels) so agents see clean domain errors instead of "Go struct field" leaks. - internal/mcp/dispatch_http.go: parseFieldKVP now accepts map[string]any in addition to the existing array/string shapes. Map shape preserves non-string values verbatim (e.g. number from a typed flag), matching the array path's existing pass-through for non-string entries. - internal/mcp/catalog_item.go: update `tags` description from "Comma-separated tags" (wrong on both SQLite and Postgres) to "Tags as a JSON array of strings, e.g. [\"v1\",\"frontend\"]". Update `field` description to clarify it's the escape hatch for SCHEMA-DECLARED custom fields, name the dedicated top-level params agents should reach for instead (status/priority/category/parent/ role/assign/tags), and note the new map-shape acceptance. Tool-level prose updated to match. Tests: - TestItemCreateUnmarshalFlexFields (mirror of TestItemUpdateUnmarshalFlexFields): 9 cases covering array/string/ null/absent/wrong-shape tags + object/string/array fields, plus a smoke test that other fields decode normally alongside the new flex paths. - TestParseFieldKVP_Variants: extended with 3 new map-shape cases (basic map, empty-key-skipped, non-string-value preserved). End-to-end verification: 5 input shapes via curl against the live handler. Pre-fix `tags: ["foo","bar"]` returned HTTP 400; post-fix returns HTTP 201 with `tags="[\"foo\",\"bar\"]"` in the column. `tags: {x:1}` (wrong shape) now returns a clean domain-level 400 instead of leaked Go internals. Existing back-compat paths (JSON-encoded string forms) preserved. Related: PR #546 (BUG-1430 rate limit) addressed the original 500 cascade that drove the agent's specific misdiagnoses in BUG-1409. * fix(mcp): forward tags array on update + drop unsupported map-shape doc per Codex review (round 1) Codex round 1 caught two issues: [P1] dispatch_http_advanced.go's PATCH builder filtered on `string` only when forwarding `tags`, so a schema-conforming `pad_item.update tags: ["a","b"]` was silently dropped. Now forwards verbatim like mapItemCreate does — the handler's ItemUpdate flex-unmarshaler (BUG-1144) normalizes any shape downstream. Regression test added. [P2] The `field` description claimed `{key: value}` map shape was accepted, but the schema Type stays `array<string>` so schema-following clients won't send the map shape. parseFieldKVP's map-shape handling (added in the previous commit) stays as defensive parsing for clients that ignore the schema, but the description no longer promises a shape the published schema doesn't advertise. Tool-level prose updated to match. * fix(mcp): revert speculative parseFieldKVP map-shape support per Codex review (round 2) Codex round 2 [P2] pointed out the map-shape parseFieldKVP support added in the first commit is dead code in practice: 1. The advertised schema for `field` is `array<string>` — no schema-conforming client sends a map. 2. `BuildCLIArgs` rejects map-shaped repeatable flags before they reach the HTTP dispatcher. 3. Even if a map did reach the dispatcher, `hasFieldChanges` doesn't recognize map shapes as field changes — `pad_item.update field: {effort: "l"}` would skip the merge and PATCH without `fields`. Either completing the support (fix hasFieldChanges + BuildCLIArgs + ItemUpdate Unmarshal) OR reverting was the right call. Reverting keeps the surface consistent with the schema and removes the unreachable code; future agents who want to override fields can use the documented `["key=value"]` array shape. BUG-1431's functional fix lands as the catalog description tightening (the empirical repro confirmed `status` placement already works in both forms; the agent's misdiagnosis was rooted in unclear docs, not broken code). BUG-1432's flexible JSON unmarshal on ItemCreate stays — that's the real fix verified by the live-handler repro. * fix(mcp): preserve empty-string tags no-op + table-driven test per Codex review (round 3) Codex round 3 [P2] caught a regression introduced in round 1's fix: by switching the tags forwarding guard from \`v.(string) && v != ""\` to \`v != nil\` to support array shapes, the empty-string filter for tags on update was lost. \`pad_item.update tags: ""\` would now forward an empty string to ItemUpdate, which treats it as an explicit empty-string write — corrupting the JSON/JSONB tags column (500 on Postgres). Fix: type-switch on tags. Empty string skips (matches pre-fix behaviour); arrays (including empty array \`[]\`, the legitimate "clear tags" case) and non-empty strings forward. Tests: the single-shape array test is replaced with a table-driven TestDispatchItemUpdate_TagsForwarding covering array, empty array, empty string (no-op), and comma-separated back-compat. Each case asserts the tags key's presence/absence and shape in the PATCH body.	2026-05-14 18:36:18 -04:00
xarmian	088ba2f839	fix(mcp): raise MCP per-token burst + classify 429 as ErrRateLimited (BUG-1430) (#546) ... * fix(mcp): raise MCP per-token burst + classify 429 as ErrRateLimited (BUG-1430) BUG-1409 reported an agent hitting "Pad backend 500s on parallel writes" during workspace onboarding via remote MCP on Pad Cloud. Triage split that umbrella into three children; this PR addresses BUG-1430 (the parallel-writes symptom). Root cause investigation showed the underlying write path is fine — local SQLite handled 24 parallel item-create POSTs cleanly (busy_timeout + BEGIN IMMEDIATE + WAL serialize writers without errors). The most plausible cause of the agent's "500 on parallel writes" report is the MCP per-token rate limiter (burst 20, 60/min) rejecting requests 21-24 of an onboarding burst with HTTP 429, which the dispatcher's classifier then collapsed into a generic ErrServerError envelope. Changes: - middleware_ratelimit.go: MCPPerToken burst 20 → 60. Sustained rate unchanged at 60/min/token. Matches the general API limiter's burst-60 per-user cap so the MCP path no longer imposes a tighter ceiling than the equivalent /api/v1 path. Comment expanded to record the rationale. - internal/mcp/errors.go: add ErrRateLimited error code and an explicit case http.StatusTooManyRequests in classifyHTTPStatusKind. 429s now surface as a first-class rate-limited envelope with an actionable hint pointing at Retry-After and the per-token cap, instead of landing in the generic ErrServerError "other 4xx" bucket. Agents implementing exponential backoff can switch on code without parsing free-form text. - handlers_cloud.go: add slog.Error instrumentation to enforcePlanLimit and enforceUserPlanLimit error paths. These are cloud-mode-only 500 candidates we couldn't exercise locally (local dev runs cloudMode=false); the structured logs give operators a grep-able tag the next time the symptom surfaces on real Pad Cloud, so we can rule the path in or out empirically without another investigation pass. - tests: bump iteration counts past the new burst (20 → 60), add 429 case to classifyHTTPStatus code-mapping table + envelope hint-shape table. Investigation context (full triage in BUG-1430): - ../pad-cloud sidecar is NOT in the /api/v1 or /mcp request path (nginx-router proxies those directly to pad backend). - featureCount + advisory-lock contention on Postgres remain plausible 500 candidates under heavy bursts; the new logging is intended to catch those if they fire. Siblings BUG-1431 (status field placement) and BUG-1432 (tags field) are tracked separately and not addressed here. * fix(mcp): drop hardcoded cap from rate-limit hint per Codex review (round 1) Codex round 1 [P2] caught that rateLimitHintFor's "the per-token cap is 60 req/min with a burst of 60" text was misleading: classifyHTTPStatusKind handles 429s from the dispatcher's SYNTHESIZED /api/v1/... requests, which come from the general API limiter (600/min, burst 60), the Search limiter (30/min, burst 10), and potentially others — NOT the MCP per-token limiter (which fires before the dispatcher runs and so never lands in this classifier path). Generalize the hint: point at Retry-After (which carries the correct limiter-specific wait) and drop the cap from prose. Update the matching test assertion to assert the generic shape ("burst-heavy" instead of "60 req/min").	2026-05-14 15:45:51 -04:00
xarmian	9fb6ac006b	fix(web): scroll restoration via SvelteKit snapshot API (BUG-1425) (#545) ... Replaces TASK-755's bespoke listing-only scroll-restoration code with a reusable createScrollRestoration helper built on SvelteKit's snapshot API, applied across every workspace top- level page (item detail, collection listing, workspace home, activity, starred, library, conventions, playbooks list/detail, roles). ## The bug On any workspace page, navigating away and back left the user near the top of the page even though they had scrolled down. The listing's old TASK-755 workaround also didn't work in practice once the layout's .main-content overflow-y:auto landed (it had been targeting window.scrollY which is permanently 0). ## The fix new `web/src/lib/scroll/restore.svelte.ts`: createScrollRestoration({ ready, persistKey? }) returns { snapshot } that the page re-exports as SvelteKit's snapshot contract. Layered restoration strategy: 1. SvelteKit snapshot (per-history-entry sessionStorage) for back/forward. 2. localStorage fallback for cross-tab / workspace-switcher goto() (no popstate) restoration, re-fires per persistKey change. 3. Per-key restoredKey one-shot so routes that reuse a component instance across URLs get fresh restoration on each new entry. 4. snapshotKey tracks the SvelteKit-claimed key so LS fallback yields to a popstate snapshot.restore that beats the effect. 5. ready() gate: caller-provided predicate must return true before we attempt to scroll, with the contract that for routes which reload on URL change the caller verifies content-vs-URL identity (e.g. item.slug === itemSlug || issue-id === itemSlug). itemUrlId() prefers refs over slugs so the issue-id branch is the dominant URL shape. 6. Retry loop with scrollHeight-stability gate (~250ms) and a 2s budget. Per-frame re-scroll handles Tiptap rendering content across many frames and async property-card fields. 7. User-input bail via wheel/touchmove/keydown listeners, NOT a scrollY diff. The browser's default overflow-anchor: auto adjusts scrollTop when content layout shifts; that browser-driven change isn't user input and mustn't trigger the bail. 8. Scroll target is .main-content (the app's actual overflow container set by the root layout), NOT window. Window's scrollY/scrollTo is a no-op for this app's chrome. ## Per-page integration Each workspace page calls createScrollRestoration() with a ready() predicate appropriate to its loading shape and a pathname-keyed persistKey. The collection listing's persistKey deliberately excludes ?search and showArchived (filter toggles call goto({replaceState}) and would otherwise jump scroll mid-interaction). ## Code path summary - web/src/lib/scroll/restore.svelte.ts — new helper (~460 lines with extensive design notes). - workspace +page.svelte and 9 other route files — thin call sites adding ~10-30 lines each. - [collection]/+page.svelte — removes ~140 lines of TASK-755's bespoke localStorage + double-RAF code in favor of the helper. Net: +637 / -149. ## Verification - make check passes (golangci-lint, go test, npm run build). - svelte-check 0 errors. - Manual repro of the canonical scenario (item → wiki-link child → back) confirmed working including the multi-section ChildItems layout that exposed the scroll-anchoring bail bug. ## Development trail (squashed from 11 commits) This commit is the final state of an unusually long iteration: Codex was consulted 5 times in a review loop and produced a sequence of correct-but-insufficient fixes (self-cancelling effect, lifetime-scoped guard, stale-content race, slug/ref match, snapshot/LS race, per-key reset) all of which were operating on the wrong measurement: window.scrollY. Once diagnostic console.logs were added (round 9) the actual problem fell out in two rounds — wrong scroll target, then wrong bail signal. Lesson: when behaviour doesn't match logic, instrument before iterating.	2026-05-14 13:22:54 -04:00
xarmian	de8679f535	chore(mcp): bump ToolSurfaceVersion 0.3 → 0.4; document v0.4 envelope (TASK-1418) (#544) ... * chore(mcp): bump ToolSurfaceVersion 0.3 → 0.4; document v0.4 envelope (TASK-1418) Final PR of PLAN-1410. The contractual announcement that the v0.4 bootstrap shape is stable. ## What 1. internal/mcp/version.go — ToolSurfaceVersion: "0.3" → "0.4". The godoc on the constant gains a full v0.4 changelog entry enumerating each shape change shipped by PLAN-1410's six bootstrap PRs: - BootstrapCollection projection (TASK-1412): drops id, workspace_id, created_at, updated_at, settings; schema as a nested JSON object. - BootstrapRole projection (TASK-1423): drops id, workspace_id, tools, created_at, updated_at. - Convention slug dropped (TASK-1413). - Top-level recent_activity duplicate removed (TASK-1413). - BootstrapDashboard wrapper caps five sub-arrays (TASK-1413 + TASK-1422): attention, recent_activity, active_items, active_plans, by_role at 5 entries each, parallel *_overflow_count fields. suggested_next deliberately excluded — already capped to 3 upstream. - Schema label omitted when label == TitleCase(key) (TASK-1424). Plus an explicit compatibility note: all v0.4 changes are additive or subtractive (no field renames); clients that read the preserved field names keep working unchanged. 2. CLAUDE.md updates: - "## MCP server" header: v0.3 catalog → v0.4 catalog, with a one-paragraph summary of what v0.4 shipped. - "Surface:" Tools bullet: v0.3 → v0.4, with a note that the tool/action surface is unchanged — only the bootstrap JSON these tools return has been trimmed. - "Stability contract": ToolSurfaceVersion (currently "0.4"), comprehensive single-paragraph description of the v0.4 envelope, cumulative size reduction (40% live / 54% fixture), and explicit additive/subtractive note. ## Why the strategy worked PLAN-1410's "version bump last" strategy paid off: - Each individual shape PR (TASK-1412/1413/1422/1423/1424) was reviewable in isolation against a stable v0.3 contract. - The six skill-side PRs (TASK-1414/1415/1416) had no MCP-shape impact and didn't need any version bump consideration. - v0.4 is now announced as a single comprehensive contract change, not five separate version bumps — easier for downstream MCP consumers (Claude Desktop, Cursor, future Pad Cloud remote MCP) to reason about. ## Verification - `make check` — golangci-lint 0 issues, all Go tests pass (including the version-tracking tests in catalog_meta_test.go that auto-pin to whatever ToolSurfaceVersion is set to), govulncheck clean, web build clean. - MCP handshake (verified via `pad mcp serve` + an initialize JSON-RPC request) advertises capabilities.experimental.padToolSurface.version = "0.4". padCmdhelp.version stays at "0.1" as expected. ## Post-merge follow-ups After this lands: - Update PLAN-1410's Result section with a "v0.4 announced" line and the final post-everything measurement (taken against docapp after `make install`). - Flip PLAN-1410 status from `active` → `completed`. These are pad-item operations, not git changes. Parent: PLAN-1410. Closes the plan. * fix(mcp): update stale v0.3 references after ToolSurfaceVersion bump (TASK-1418 follow-up) Address Codex P2 + P3 findings on PR #544: bumping ToolSurfaceVersion in version.go left four runtime/user-facing docs still claiming v0.3: P2 — runtime MCP docs: - internal/mcp/instructions.md "## Tool surface (v0.3)" → v0.4 - internal/mcp/catalog_meta.go "v0.3 server-introspection tool" → "(v0.4 catalog)" - internal/mcp/catalog_meta.go padMetaToolDescription twice: * "the v0.3 tool catalog" → "the v0.4 tool catalog" * "v0.3 catalog dump" → "v0.4 catalog dump" - internal/mcp/catalog_meta.go actionMetaToolSurface godoc: "v0.3 catalog" → "catalog" (de-versioned; the comment is about scope, not version) P3 — public README: - README.md "Tool catalog (v0.3)" → "Tool catalog (v0.4)" - README.md "tool_surface_version: '0.3'" → "'0.4'" with a pointer to PLAN-1410's bootstrap-trim summary and version.go's full v0.4 changelog. Without these, agents reading the initialize-instructions blob or pad_meta's tool description (both of which are part of the runtime MCP surface, not just internal docs) would see v0.3 while the handshake / pad_meta.action: version returned v0.4 — the exact "contradictory metadata depending on what you read" failure mode Codex flagged. Same skill-↔-code sync pattern that has been a running theme through PLAN-1410's review loops. The cluster of stale references is a classic side effect of a version bump landing late in a plan — the version constant is one string, but downstream prose that names it lives in multiple places. Verified no remaining "v0.3" claims that imply currency — `grep -rn "v0\.3\|tool_surface_version" --include="*.{go,md}"` returns only historical-context mentions in changelog godocs (correct) and the runtime constant readback (correctly returns "0.4" now). Parent: PLAN-1410 / TASK-1418. * fix(mcp): correct schema-type-change disclosure + stale cmdhelp-walker description (TASK-1418 follow-up) Address Codex round 2 P3 findings on PR #544: ## P3 — `cmd/pad/mcp.go` still described the retired leaf walker The `pad mcp serve` command's Long description said "every leaf command becomes an MCP tool, except the curated allow-list exclusions" — that was true under v0.1 but the cmdhelp leaf walker was retired in TASK-981 (PLAN-969's v0.2 rollout). The v0.2/v0.3/v0.4 surface has always been the hand-curated catalog of eight resource × action tools + pad_set_workspace. Updated the Long description to: - Name the v0.4 catalog explicitly. - List the eight resource × action tools. - Note that cmdhelp v0.1 still drives per-command arg schemas at dispatch time (so it's not gone, just no longer drives tool naming/count). - Reference TASK-981 for the cutover. ## P3 — "additive/subtractive only" was misleading The compatibility note in `version.go` and `CLAUDE.md` claimed all v0.4 changes were additive or subtractive. That glossed over one breaking change in TASK-1412: `collections[].schema` went from a JSON-encoded string ("schema":"{\"fields\":...}") to a nested JSON object ("schema":{"fields":...}). For any v0.3 consumer that read schema as a string and JSON.parse()'d it themselves, that's a TYPE change, not a no-op. Updated both godoc and CLAUDE.md to explicitly call this out as the one breaking change, separately from the additive/ subtractive bucket. Better for downstream MCP consumers to see the truth than to discover it via runtime failure. The remaining v0.4 changes ARE additive (overflow counts on BootstrapDashboard) or subtractive (dropped fields with named canonical alternatives) — those parts of the original note are accurate and kept. Honesty about compatibility is more valuable than a tidy narrative. Surfaced explicitly in the godoc + the public contract doc; PLAN-1410's Result section was already honest about the field-level deltas. Parent: PLAN-1410 / TASK-1418.	2026-05-13 18:02:34 -04:00
xarmian	d18a5d8140	refactor(bootstrap): omit redundant schema labels + omitempty sort_order (TASK-1424) (#543) ... Two small additive trims to the BootstrapCollection projection introduced in TASK-1412. ## 1. Omit redundant schema `label` when label == TitleCase(key) A schema field's `label` is auto-fillable from its `key` (the CLI and the MCP-side CreateCollection helper both apply TitleCase(key) when label is empty — see titleCaseLabel in internal/mcp/dispatch_http_routes.go). When the persisted label matches that rule, it's redundant — the agent can reconstruct it from the key. Examples on docapp: - {"key":"status", "label":"Status"} ← redundant - {"key":"due_date", "label":"Due Date"} ← redundant - {"key":"trigger", "label":"When"} ← CUSTOM, preserved Implementation: projectBootstrapCollection now passes the schema bytes through trimRedundantSchemaLabels, which: 1. Unmarshals into a parallel bootstrapSchema/bootstrapFieldDef struct purpose-built for the bootstrap shape. 2. Walks fields, clears any Label that equals TitleCase(Key). 3. Re-marshals — `omitempty` on bootstrapFieldDef.Label drops the empty-string labels from the output. Field ordering is preserved by struct-based marshalling. ## 2. Omitempty on BootstrapCollection.SortOrder `sort_order` defaults to 0. Most collections never get an explicit non-zero sort_order, so the field carried "sort_order":0 per entry needlessly. Added ,omitempty to the struct tag. ## Drift detection bootstrapFieldDef mirrors models.FieldDef field-for-field with two deliberate differences: `Label` is omitempty, and `Default` is json.RawMessage (so any default value round-trips verbatim without re-parsing). The risk: if models.FieldDef gains a new field, bootstrapFieldDef silently loses it from the bootstrap schema response. TestBootstrapFieldDefMirrorsModelsFieldDef catches this via reflection — compares NumField + per-field JSON tags + has an explicit allow-list for the Label tag delta. A new field added to models.FieldDef without mirroring here fails the test with a field-name-pointed error message. ## Test coverage - TestBootstrapFieldDefMirrorsModelsFieldDef — drift detector. - TestTrimRedundantSchemaLabels (4 subtests): * drops-redundant-labels * preserves-custom-labels (key="trigger", label="When" stays) * multi-word-keys-titlecase-correctly (due_date → Due Date) * malformed-schema-returns-raw (defensive: never block on parse error) - Existing TestBootstrapSizeBudget shows fixture collections section drop: 3,979 → 3,532 bytes (-11.2%). ## Measurements Fixture (TestBootstrapSizeBudget): 7,823 → 7,376 bytes (-447 b / -5.7%). Collections section alone: 3,979 → 3,532 bytes (-447 b / -11.2%) — all of the win is concentrated in schema bytes via the label trim. Live docapp expected savings: 9,384 → ~8,400 bytes (~10% drop on collections), totaling roughly 600-900 b additional reduction on the bootstrap response. ## Out of scope - ToolSurfaceVersion 0.3 → 0.4 bump — TASK-1418 (the FINAL PR in PLAN-1410). This is the last shape PR; TASK-1418 is now unblocked. Parent: PLAN-1410. With this merged, PLAN-1410's bootstrap-shape work is complete; only the contractual v0.4 announcement remains.	2026-05-13 17:34:57 -04:00
xarmian	ad87b960d7	refactor(bootstrap): slim Roles projection (BootstrapRole struct) (TASK-1423) (#542) ... Same drop-pattern as TASK-1412's BootstrapCollection applied to the bootstrap response's `roles` section. ## Struct + helper New BootstrapRole struct purpose-built for the bootstrap response: - Keeps: slug, name, description, icon, sort_order, item_count - Drops: id, workspace_id, tools, created_at, updated_at The `tools` field has no consumer outside the store CRUD (grep confirms — only referenced in internal/store/agent_roles.go); docapp has it set to "" on all three roles in practice. If it ever becomes load-bearing for the agent, it can be added back. New projectBootstrapRole(models.AgentRole) helper mirrors projectBootstrapCollection. ## BuildAgentBootstrap reorder Roles projection now happens AFTER the role-count recompute for restricted callers (which keys lookups by AgentRole.ID, which the projection drops). Same reorder pattern as TASK-1412's collections refactor — the local `roles` slice stays in models.AgentRole shape through the count recompute, then projects in a final pass alongside the collections projection. ## Tests New TestBootstrapRoleProjection seeds one role via the agent-roles endpoint and verifies the wire shape: - Positive: slug/name/description/item_count are present and correct. - Negative: id/workspace_id/tools/created_at/updated_at are NOT present in the marshalled JSON. The negative check is the load-bearing part — without it, a future refactor that "fixes" the projection by re-adding a UUID would pass the positive assertions silently. Mirrors TestBootstrapEmptyArraysNotNull's pattern for the recent_activity dedup. Existing TestBootstrapEmptyWorkspace and TestBootstrapEmptyArraysNotNull still pass — their `b.Roles != nil` and `roles != null` checks work at slice-level regardless of element type. ## Measurements Fixture (TestBootstrapSizeBudget) unchanged at 7,823 bytes — the fixture seeds 0 roles so the projection change doesn't affect the size budget (roles section was already at 2 bytes "[]"). Live docapp measurement deferred until make install — expected savings: roles section was 1,066 bytes for 3 roles; drop is ~30-40% (~350 b) of that section. ## Out of scope - Schema label + sort_order trim — TASK-1424. - ToolSurfaceVersion 0.3 → 0.4 — TASK-1418 (final PR). Parent: PLAN-1410.	2026-05-13 17:22:18 -04:00
xarmian	aaa581b105	refactor(bootstrap): extend dashboard caps to active_items/active_plans/by_role (TASK-1422) (#541) ... * refactor(bootstrap): extend dashboard caps to active_items/active_plans/by_role/suggested_next (TASK-1422) Implements IDEA-1421 (absorbed into PLAN-1410's v0.4 envelope). Extends the BootstrapDashboard wrapper from TASK-1413 to cap four more dashboard sub-arrays with parallel overflow counts, same shape and semantics as the existing attention/recent_activity caps. ## Struct + caps Four new int fields on BootstrapDashboard (all `,omitempty`): - active_items_overflow_count - active_plans_overflow_count - by_role_overflow_count - suggested_next_overflow_count Four new cap constants alongside the existing two: - bootstrapActiveItemsCap = 5 - bootstrapActivePlansCap = 5 - bootstrapByRoleCap = 5 - bootstrapSuggestedNextCap = 5 capBootstrapDashboard extended with four parallel truncate-and-count blocks — same shallow-copy mutation pattern, source pointer untouched (the dashboard endpoint still returns its full-length arrays per its own contract). ## Tests TestCapBootstrapDashboard rewritten to cover all six caps under one contract. `mk` now takes a `dashCounts` struct (Att/Rec/Items/Plans/ Role/Sugg) so each subtest exercises specific caps without populating the others. Each of the four existing subtests (under-cap-no-overflow, over-cap-truncates-and-counts-overflow, source-pointer-unchanged, exact-cap-no-overflow) now asserts the new caps too. Added tiny assertLen/assertOverflow helpers to keep the per-array assertion noise from drowning the contract being tested. bootstrapSectionBytes extended to surface the four new cap-effect lines when triggered, table-driven so future caps drop in cleanly. seedBootstrapSizeFixture updated to seed 6 in_progress tasks (was 5 open) so the new active_items cap fires visibly in the per-section breakdown: "active_items capped: 5 shown, 1 overflow". The status flip is deliberate — dashboard.active_items filters on isActiveStatus(), which excludes initial/terminal statuses; open tasks never appeared in the section. ## Budget bootstrapSizeBudget 7 KiB → 9 KiB. Note that this is FIXTURE-side growth, not shape-side regression: the fixture now seeds enough active items to exercise the new cap (active_items section was 0 bytes when tasks were status=open). The cap itself is purely a SAVINGS — on docapp it drops active_items from 7 → 5 entries with overflow_count=2. Budget history note in handlers_bootstrap_test.go updated with the TASK-1422 line and an explicit "fixture-side, not shape-side" explanation so future readers understand why the budget moved up. ## Out of scope - Slim BootstrapRole projection — TASK-1423. - Schema label + sort_order trim — TASK-1424. - ToolSurfaceVersion 0.3 → 0.4 bump — TASK-1418 (final PR). Parent: PLAN-1410. Resolves IDEA-1421 once merged. * fix(bootstrap): drop unreachable suggested_next cap (TASK-1422 follow-up) Address Codex P1 finding on PR #541: `suggested_next_overflow_count` was unreachable in production responses because `buildDashboardResponse` already truncates `SuggestedNext` to 3 upstream (see "Take top 3" comment in handlers_dashboard.go:854-858), while my bootstrap cap was 5. The cap-and-overflow logic could only have fired against synthetic test state, never against the real dashboard pipeline. Two responses to consider: 1. Lower bootstrap's cap to a number smaller than 3 — defeats the upstream design choice (3 IS the intentional limit). 2. Drop the bootstrap-side cap — clean, no dead surface. Going with (2). If the upstream cap is ever raised or removed, that's the moment to add a suggested_next_overflow_count back. Removed: - SuggestedNextOverflowCount field on BootstrapDashboard - bootstrapSuggestedNextCap constant - The truncate-and-count block in capBootstrapDashboard - The suggested_next row in TestCapBootstrapDashboard's dashCounts helper and all five subtest assertions - The suggested_next entry in bootstrapSectionBytes's cap-line loop The fixture still seeds tasks and a plan, so the no-cap path on SuggestedNext is naturally exercised through TestBootstrapSizeBudget. The godoc on BootstrapDashboard now explicitly calls out the exclusion + the upstream-cap rationale so a future reader knows why suggested_next is missing from the otherwise-uniform cap set. Parent: PLAN-1410 / TASK-1422. * docs(skill): align SKILL.md dashboard cap description with TASK-1422 Address Codex P3 finding on PR #541: the SKILL.md `Context Loading` section described only the two original cap fields (attention_overflow_count, recent_activity_overflow_count). After TASK-1422 the bootstrap response carries three more (active_items_overflow_count, active_plans_overflow_count, by_role_overflow_count), and the agent needs to know to pull the full set via `pad project dashboard` when any of them are > 0. Updated the bullet to enumerate all five capped sub-arrays and state the overflow-field pattern generically rather than per-field. Same in-PR-sync pattern used for TASK-1413, TASK-1415, TASK-1416. Parent: PLAN-1410 / TASK-1422. * docs(bootstrap): fix three stale comments after dropping suggested_next cap (TASK-1422 follow-up) Address Codex P3 finding on PR #541 round 3: three stale doc strings referenced the old shape (with suggested_next) after the cap was dropped in the prior commit. Updated: 1. handlers_bootstrap_test.go budget-history line for TASK-1422 — removed `suggested_next` from the cap list and added the "deliberately excluded — already capped to 3 upstream" rationale so future readers know why the otherwise-uniform cap set is missing one. 2. handlers_bootstrap.go BootstrapDashboard godoc — changed "two overflow counts" to "five overflow counts (one per capped sub-array)". 3. handlers_bootstrap.go capBootstrapDashboard godoc — changed "both caps are untriggered" to "all caps are untriggered". Tidy-up only, no behavior change. Same skill-↔-code sync hygiene that has been the running theme across PLAN-1410's review loops. Parent: PLAN-1410 / TASK-1422. * docs(bootstrap): update remaining stale call-site comment for capBootstrapDashboard (TASK-1422 follow-up) Final stale-doc cleanup per Codex P3 on PR #541 round 4: the BuildAgentBootstrap dashboard-wrapping call-site comment still listed only attention + recent_activity. Updated to enumerate all five capped sub-arrays for parity with the godoc on BootstrapDashboard / capBootstrapDashboard. Same hygiene as the previous commit; no behavior change. Parent: PLAN-1410 / TASK-1422.	2026-05-13 17:10:56 -04:00
xarmian	9c9aac3ea6	chore(bootstrap): tighten size budget 8 KiB → 7 KiB to lock in PLAN-1410's win (TASK-1417) (#540) ... PLAN-1410's bootstrap shape work is complete on main (TASK-1411..1416). The fixture still measures 6,355 bytes against the 8 KiB budget — too much headroom for a ratchet whose purpose is to detect shape regressions. Tightened to 7 KiB: - Fixture: 6,355 bytes - Budget: 7,168 bytes (7 KiB) - Headroom: 813 bytes (~12.8%) Tight enough to catch any meaningful shape regression (reintroducing a duplicated field, un-capping a dashboard array, re-stringifying schema), loose enough to absorb routine schema reordering or single-field additions without false alarms. Budget-history note updated with the TASK-1417 line and a forward-looking pointer at IDEA-1421 (next-round dashboard sub-array caps) which will land its own win under its own ratchet. The companion measurement (per-section bytes for fixture + live docapp + pre/post per-invocation totals) was recorded directly into PLAN-1410's body via `pad item update PLAN-1410 --stdin`, which is the canonical home for plan results. Cumulative win: fixture: 13,861 → 6,355 (-54.2%) live docapp bootstrap: 52,033 → 33,375 (-35.9%) live SKILL.md: 40,193 → 29,840 (-25.8%) live combined per /pad: 92,226 → 63,215 (-31.5%) The original ~43% target was set against a static-workspace assumption; the live shortfall is workspace-scale-dependent (conventions on docapp are 11.2 KB vs 0.5 KB on the fixture). IDEA-1421 captures the next round (dashboard active_items / active_plans / by_role / suggested_next caps, estimated ~3 KB additional live win, backwards-compatible). Parent: PLAN-1410. Final remaining task: TASK-1418 (ToolSurfaceVersion 0.3 → 0.4 contractual announcement).	2026-05-13 16:25:47 -04:00
xarmian	6a981e7433	docs(skill): compress NL Routing examples, trim playbook authoring, drop MCP note (TASK-1416) (#539) ... * docs(skill): compress NL Routing examples, trim playbook authoring section, drop MCP note (TASK-1416) Final SKILL.md trim in PLAN-1410's skill-side compression series. Three targeted changes: 1. NATURAL LANGUAGE ROUTING — compress example density The "example phrasing → command" pairs in each sub-category were over-enumerated — the model handles intent matching without an exhaustive lookup table. Compressed each sub-category to its canonical pattern(s) while keeping the section structural map intact (Role management, Creating items, Querying, Updating, Working with attachments, Planning, Ideation, Dependencies, Reports, Retrospective, Onboarding, Creating a playbook). Pattern: where a section had 5-11 "intent → command" lines all illustrating the same command verb with slight wording variants, collapsed to a 1-2 line summary that describes the routing rule directly. Where a section had genuinely-distinct commands (e.g. Querying covers dashboard / next / list / search), kept one canonical line per command verb. 2. PLAYBOOK AUTHORING — replace worked examples with a compact pointer The "Authoring trigger-only" and "Authoring slug-invocable with arguments" subsections previously included full ~25-line heredoc examples — useful when the schema-aware --field parsing was new, now memorizable scaffolding. Replaced with a 2-line pointer listing the two authoring surfaces (CLI / Web UI) and the key `--field 'arguments=[...]'` shape. The model knows the heredoc pattern; the worked example was redundant. 3. MCP NOTE — dropped The "Note for agents using MCP instead of this skill" preamble (~700 B) only applied to readers of the file — agents loading this skill are by definition using the CLI surface, not MCP. The MCP catalog reference at getpad.dev/mcp/local stays the canonical source for that surface; removing the note avoids carrying its bytes in every skill load. Measurements: Before TASK-1416: 34,786 b / 479 lines After: 29,448 b / 393 lines Delta this PR: -5,338 b (-15%) Cumulative PLAN-1410 skill-side reduction (TASK-1414/1415/1416): Baseline (TASK-1410 start): 40,193 b / 567 lines After all three trims: 29,448 b / 393 lines Total reduction: -10,745 b (-26.7%) Parent: PLAN-1410. Remaining: TASK-1417 (final measurement back into the plan body) and TASK-1418 (ToolSurfaceVersion 0.3 → 0.4). * fix(skill): add status=active activation requirement to playbook authoring (TASK-1416 follow-up) Address Codex P2 finding on PR #539: the compressed authoring pointer dropped `--field status=active` from the example. New playbooks default to status=draft, but slug routing and trigger-intent matching only dispatch status=active entries. Following the trimmed instructions would create /pad <slug> playbooks that silently fall through to NL routing. The original (pre-TASK-1416) worked example included status=active; I lost it when collapsing to a pointer. Restored explicitly: - New "**Activation matters**" paragraph calling out the default-draft pitfall and the silent-fall-through behavior. - CLI example now includes `--field status=active`. - Web UI bullet explicitly mentions flipping draft → active before save. Same in-PR-sync pattern as TASK-1413's SKILL.md alignment commit and TASK-1415's activation-check correction. Parent: PLAN-1410 / TASK-1416.	2026-05-13 16:16:50 -04:00
xarmian	335b145343	docs(skill): replace Planning/Decomposition workflow fallbacks with one-liner playbook pointers (TASK-1415) (#538) ... * docs(skill): replace Planning/Decomposition workflow fallbacks with one-liner playbook pointers (TASK-1415) The Planning and Decomposition workflows in SKILL.md each had: 1. A leading "use the <slug> playbook" pointer (the canonical entry point). 2. A multi-line "if the playbook isn't activated, fall back to this inline workflow" block duplicating most of the playbook's contract. For software templates the playbooks auto-seed via softwareStarterPlaybookTitles, so the fallback fires approximately never. Non-software workspaces activate from the library UI, which also makes the fallback transient at best. Replaced each section with a one-liner pointer that: - Names the canonical /pad <slug> invocation - Explains how to confirm activation (bootstrap's playbooks array or `pad playbook show <slug>`) - Tells the user to activate via the library UI when missing, and offer to walk through manually in the meantime — without duplicating the playbook's step contract here Measurements: SKILL.md total: 36,473 → 34,786 bytes (-1,687 b / -5%) SKILL.md lines: 501 → 479 Cumulative against PLAN-1410 baseline: SKILL.md total: 40,193 → 34,786 bytes (-13.5% so far) Parent: PLAN-1410. Next: TASK-1416 (NL Routing + authoring example + MCP note). * fix(skill): correct playbook activation-check guidance (TASK-1415 follow-up) Address Codex review findings on PR #538: P2 — `pad playbook show <slug>` is not a valid activation check. The resolver returns playbooks by invocation_slug regardless of status, and default output omits status. A draft/deprecated `plan` playbook would be treated as active. Corrected to direct the agent at the bootstrap's `playbooks` array (which carries status) for the activation check, and noted explicitly that `pad playbook show` alone is insufficient. P3 — The "**Planning:**" routing bullet still pointed at "otherwise inline workflow (see below)" after the inline fallbacks were removed in the parent commit. Replaced with a pointer to library activation, matching the (now-trimmed) workflow section. Both findings would have left agents in a workspace without active plan/decompose playbooks pointing at the wrong fallback. Fixed in-PR so skill ↔ playbook contract stays strictly synchronized (same pattern as TASK-1413's SKILL.md sync commit). Parent: PLAN-1410 / TASK-1415.	2026-05-13 16:08:29 -04:00
xarmian	efccea6dfe	docs(skill): compress CLI Reference to patterns the skill drives (TASK-1414) (#537) ... The CLI Reference section grew over time with every-flag enumeration, multiple worked examples per command, and edge-case commands the skill never invokes (webhooks REST API trivia, full --fields DSL walkthrough for collection create, multiple per-command examples that all illustrate the same flag pattern). Compressed to the patterns the natural-language routing actually drives, with `pad <cmd> --help` as the explicit escape hatch for anything else. Measurements: - SKILL.md total: 40,193 → 36,473 bytes (-3,720 b / -9%) - SKILL.md line count: 567 → 501 - CLI Reference section: ~6,500 → ~3,400 bytes (-48%) Preserved: - All command verbs the NL routing references (item create/list/ show/update/delete/search/comment/comments/bulk-update, role list/create/delete, project dashboard/next/standup/changelog, playbook list/show/run, attachment list/show/view/upload/ download, collection list/create, server info/open, auth whoami, bootstrap). - The hard rule against reading ~/.pad/attachments/ directly (kept the explanation tight: "bypasses ACLs, breaks on Pad Cloud / S3, skips the variant pipeline"). - The `--field key=value` schema-aware pattern with concrete examples for convention + playbook (the two collections most likely to drive its use). - The two-mode collection create (`--fields` DSL vs `--schema` full CollectionSchema), with the when-to-use guidance retained. Dropped or compressed: - Per-command multi-example listings (one canonical pattern each). - The full Webhooks subsection — webhooks are REST-API-only and the skill never invokes them directly; pointer in the catch-all. - The trailing "Output Formats" footer — replaced by an inline note at the section header that --format json works everywhere. - Verbose per-line comments inside code blocks; the patterns are self-documenting at this scale. Parent: PLAN-1410 / TASK-1414. Bootstrap-shape work already in main (TASK-1411/1412/1413). Remaining: SKILL.md workflow + NL routing trims (TASK-1415/1416), then final measurement + version bump.	2026-05-13 16:02:04 -04:00
xarmian	638a456f98	refactor(bootstrap): dedup recent_activity, drop convention slug, cap dashboard arrays (TASK-1413) (#536) ... * refactor(bootstrap): dedup recent_activity, drop convention slug, cap dashboard arrays (TASK-1413) Three bundled handler-level cleanups against PLAN-1410's bootstrap shape. Total fixture savings: 8,992 → 6,355 bytes (-2,637 b / -29%). 1. Drop duplicate top-level `recent_activity` AgentBootstrap.RecentActivity was bit-for-bit identical to AgentBootstrap.Dashboard.RecentActivity. Removed: - AgentBootstrap.RecentActivity field - capRecentActivity() helper - recentActivityWindow constant - the time import (no longer used) Fixture savings: -1,751 bytes. 2. Drop `slug` from AgentBootstrapConvention Agents address convention items by ref (CONVE-N); slug was dead weight. Removed the field + the population line in collectAlwaysOnConventions. Fixture savings: -78 bytes. 3. Cap dashboard.attention + dashboard.recent_activity to 5 in bootstrap New BootstrapDashboard wrapper embeds *DashboardResponse (so the wire shape stays compatible — same field names, same nesting) and adds two overflow counts: - attention_overflow_count (omitempty when zero) - recent_activity_overflow_count (omitempty when zero) The cap is applied via capBootstrapDashboard which shallow-copies the DashboardResponse before truncating the slices, so callers downstream of buildDashboardResponse (the dashboard endpoint itself, the web UI) see their original full-length arrays unchanged. `pad project dashboard` contract is preserved verbatim. Fixture savings: -789 bytes (recent_activity capped 9 → 5; attention untouched, fixture has 0 attention items). Coverage: - TestCapBootstrapDashboard (4 subtests): under-cap-no-overflow, over-cap-truncates-and-counts-overflow, source-pointer-unchanged, exact-cap-no-overflow. Locks in the cap contract independent of the full bootstrap pipeline. - TestBootstrapEmptyArraysNotNull updated: the top-level recent_activity key was removed from the required-keys list, with a separate assertion that guards against it reappearing. - TestBootstrapEmptyWorkspace updated: removed the b.RecentActivity nil-check; added a (defensive) check that dashboard's nested recent_activity is non-nil when dashboard is present. - bootstrapSectionBytes now surfaces the cap effect ("attention capped: 5 shown, 4 overflow") when triggered, so the trim's value is legible from CI output. bootstrapSizeBudget tightened 11 KiB → 8 KiB to lock in the win. Budget-history comment updated. Out of scope (handled by later PLAN-1410 PRs): - Skill-file trim (TASK-1414/1415/1416) - Final measurement (TASK-1417) - ToolSurfaceVersion 0.3 → 0.4 (TASK-1418, after all shape changes land) Parent: PLAN-1410. * docs(skill): align SKILL.md bootstrap shape with PLAN-1410 / TASK-1413 The skill's `Context Loading` section described the old wire shape: - `dashboard {...}` — active items, attention, suggested next, recent activity - `recent_activity [...]` — capped to the last 24h After TASK-1413 the top-level `recent_activity` field is gone (it was a bit-for-bit duplicate of `dashboard.recent_activity`), and the remaining `dashboard.recent_activity` is capped by COUNT (top 5) not by TIME (24h window). The two cap fields (attention_overflow_count and recent_activity_overflow_count) tell the agent how much was trimmed so it can decide whether to follow up with a full `pad project dashboard` query. Per the Codex P2 finding on PR #536: documenting the new contract in this PR keeps skill ↔ wire-shape strictly synchronized (no window where the docs are wrong about the shape this PR ships). Parent: PLAN-1410 / TASK-1413.	2026-05-13 15:29:59 -04:00
xarmian	8da0473e4e	refactor(bootstrap): slim Collections projection — drop id/timestamps/settings, parse schema inline (TASK-1412) (#535) ... Introduces BootstrapCollection — a purpose-built projection for the bootstrap response that replaces []models.Collection on AgentBootstrap.Collections. Drops fields the /pad skill never reads: - id, workspace_id — agent addresses collections by slug - created_at, updated_at, — irrelevant at context-load time deleted_at - settings — quick_actions + view defaults are web-UI chat-prompt config The remaining schema string is delivered as a nested JSON object (json.RawMessage) rather than a JSON-encoded string, killing the backslash-escape overhead so the agent sees real {}/[] structure instead of double-encoded quotes. json.Valid() gates the emission so a future migration leaving non-JSON in the column can't break agent-side json.Unmarshal — invalid/empty schemas are simply omitted (omitempty). Measured against the bootstrapSizeBudget fixture (TASK-1411): before after delta collections 8,848 b 3,979 b -4,869 b (-55%) total bootstrap 13,861 b 8,992 b -4,869 b (-35%) Budget tightened from 16 KiB to 11 KiB to lock in the win. Later PLAN-1410 PRs (TASK-1413's dedup + dashboard caps, TASK-1417's final measurement) tighten further. Wire-shape change details: - BuildAgentBootstrap holds collections as []models.Collection through the visibility-restricted role+count recompute (which keys lookups by Collection.ID), then projects to []BootstrapCollection at the end of that section. ID-keyed recompute logic is preserved verbatim — only the final wire shape changes. - printBootstrapMarkdown was already reading {slug, name, prefix} via its own anonymous struct; those three are preserved. - No web-UI consumers exist for /agent/bootstrap (grep confirms), so no client-side churn. Out of scope (handled by later PLAN-1410 PRs): - Dedup'ing top-level recent_activity, dropping convention slug, capping dashboard.attention/recent_activity (TASK-1413). - ToolSurfaceVersion bump 0.3 → 0.4 (TASK-1418, after all shape changes land). Parent: PLAN-1410.	2026-05-13 12:18:07 -04:00
xarmian	91f1c0a017	test(bootstrap): add size-budget benchmark for the agent bootstrap response (TASK-1411) (#534) ... Adds TestBootstrapSizeBudget which builds an AgentBootstrap blob against a seeded representative fixture (default template seeds + 2 always-on conventions with bodies + 1 slug-invocable playbook + 5 tasks + 1 plan) and asserts the marshalled JSON byte count stays at or below bootstrapSizeBudget (initially 16 KiB, against a current actual of ~13.8 KiB on the seeded fixture). On every run — pass or fail — the test logs a per-section breakdown (workspace / user / collections / conventions / roles / playbooks / dashboard / recent_activity) so size regressions are diagnosable from CI output alone, and so the cumulative trim across PLAN-1410 is visible as the budget tightens. This is the baseline ratchet for PLAN-1410's bootstrap-shape PRs: - TASK-1412 (slim Collections projection) tightens the budget down once schema-as-string and the redundant ids/timestamps come out. - TASK-1413 (dedup top-level recent_activity, drop convention slug, cap dashboard arrays) tightens further. - TASK-1418 records the final v0.4 actual. The docapp workspace currently measures ~52 KB / ~13K tokens on the real bootstrap payload; the fixture is intentionally small but exercises the same shape contributors so a regression in the projected shape (per-collection settings, schema-as-string, duplicate recent_activity, etc.) trips the budget at fixture scale. Parent: PLAN-1410.	2026-05-13 10:08:41 -04:00
xarmian	1db0e6a505	docs+test: closes the PLAN-1397 library overhaul loop (TASK-1404) (#533) ... * docs+test: closes the PLAN-1397 library overhaul loop (TASK-1404) ## Tests New `internal/collections/playbook_library_test.go` with 4 regression guards for the invokable-first library: - TestPlaybookLibrary_InvokableEntriesPresent — asserts ship + plan + decompose are all present by invocation_slug, that each has at least one Argument declared, and that at least 3 invokable entries exist. Catches T1 widening getting half-reverted or T6's library rebuild dropping an entry. - TestPlaybookLibrary_AllTriggersKnown — asserts every library entry's Trigger is one of the canonical values (manual, on-implement, on-review, on-plan, on-triage, on-release, on-deploy, on-pr-create, on-task-complete, always). Catches typos that would seed an invalid trigger. - TestPlaybookLibrary_ShipBodyShared — confirms the library `ship` entry and ShipPlaybook() seed share the same body constant. The whole point of T3 was to avoid duplication; this prevents drift. - TestPlaybookLibraryArchive_BodiesCompiled — keeps archivedPlaybooks() greppable and verifies the canonical retired title ("Implementation Workflow") is still in the archive. ## CLAUDE.md - Added a "Library — discovery surface" subsection to the Playbooks section explaining the invokable-first lineup, the archive, and how `softwareStarterPlaybookTitles` seeds plan + decompose into software workspaces. - Expanded "Code map" with the four new/refactored library files (playbook_library.go, playbook_library_plan.go, playbook_library_decompose.go, playbook_library_archive.go). - Cross-referenced PLAN-1397 alongside PLAN-1377 for design history. ## skills/pad/SKILL.md - "Planning: 'Let's create a plan'" and "Decomposition: 'Break plan X into tasks'" now name `/pad plan` and `/pad decompose` as the canonical entry points, with the inline workflow as the fallback when those playbooks aren't activated in the workspace. - The high-level "Planning:" intent list at the top of the routing section updated to match. - Note: SKILL.md is embedded into the Go binary at build time. The next `make install` distributes the skill update; the .agents/ copy regenerates automatically. Parent: PLAN-1397. Closes the loop — all 7 tasks of the playbook library overhaul shipped. * fix(test): source knownPlaybookTriggers from SoftwarePlaybookTriggers per Codex review (round 1) Round 1: the hand-maintained `knownPlaybookTriggers` map admitted `on-pr-create`, `on-task-complete`, and `always` — all valid in the schema for *conventions*, but NOT in `SoftwarePlaybookTriggers` (templates.go:266). A library entry could ship one of those and the regression test would pass, but the same trigger would be rejected when seeded into a software workspace at activation. Fix: derive the test's known-set from `SoftwarePlaybookTriggers` directly. If the schema widens or narrows, the test follows automatically — no drift between the assertion baseline and the actual schema. Also added a note clarifying that templates with domain-specific trigger vocabularies (e.g. hiring's on-candidate-advance) would need their own library scoped to that template. The set is now exactly: manual, on-implement, on-triage, on-release, on-plan, on-review, on-deploy.	2026-05-13 07:17:58 -04:00
xarmian	8fa1dd36f9	refactor(library): archive 9 pre-PLAN-1377 playbook bodies, rebuild library as invokable-first (TASK-1403) (#532) ... Retires the legacy trigger-only library entries from the public surface and replaces the 4-category structure with a single `agent-workflows` category housing the three invokable workflow playbooks (ship, plan, decompose). ## Changes - New file `internal/collections/playbook_library_archive.go` — holds all 9 retired bodies in package-private `archivedPlaybooks()`. Bodies stay compiled so they're greppable and refactor-safe; per-entry "convert to invokable" / "promote to convention" / "retire" decisions are tracked in IDEA-1396. `var _ = archivedPlaybooks` keeps the symbol referenced for unused-symbol linters. - `internal/collections/playbook_library.go::PlaybookLibrary()` — removed the 4 categories (workflow, planning, quality, operations) and the 9 bodies inline. Replaced with a single `agent-workflows` category containing ship + plan + decompose (the invokable trio landed in T3/T4/T5). All three carry InvocationSlug and Arguments so the library teaches the PLAN-1377 invocation model from the first card. - `internal/collections/playbook_library_plan.go` / `playbook_library_decompose.go` — bump each helper's `Category` field from `workflow` to `agent-workflows` to match the new registry grouping. - `internal/collections/templates.go::softwareStarterPlaybookTitles` — updated from the retired pair ("Implementation Workflow", "Code Review Process") to the new invokable pair ("Plan a new initiative", "Decompose a plan into tasks"). `startup` template separately prepends `ship` (templates.go:~441), so every software workspace now seeds the full invokable trio from day one. - `internal/mcp/dispatch_http_slice4_test.go` — the activate-by-title fixture used "Implementation Workflow"; switched to "Ship tasks" (still a real library entry with trigger+scope in its activation payload). T6's verify section flagged this fixture; addressing it here keeps the build green on this branch rather than deferring the breakage to T7. - `cmd/pad/main.go` — `pad library activate --help` example used "Implementation Workflow" as a sample title; switched to "Ship tasks" so the example still resolves. ## Verify - `go build ./...` clean (no dangling references to the 9 titles in production code paths). - `go vet ./...` clean. - `go test ./...` — all packages green. - `grep -r "Implementation Workflow" --include="*.go" --include="*.ts" --include="*.svelte"` returns only: - `playbook_library_archive.go` (expected — the archive) - `playbook_library_plan.go` (historical comment, intentional) - `templates.go:868` (demo-workspace seed item content — unrelated to library lookup; literal item title in a template, would not benefit from being retitled in this PR's scope) Pre-existing workspaces' already-activated copies of the 9 entries keep working — they live in workspace data, not library code. Only future activations are affected (the legacy titles no longer resolve via `pad library activate` or the web Library UI). Parent: PLAN-1397. Depends on T3/T4/T5 — the library is never empty because ship, plan, and decompose are already in place.	2026-05-13 07:11:01 -04:00
xarmian	8eb927cbbc	feat(library): author the decompose invokable playbook (TASK-1402) (#531) ... New library entry: `/pad decompose <PLAN-ref>` — takes a plan and creates the child task items its body implies, with the user's approval at every step. The natural follow-up to `/pad plan`: that playbook creates the plan; this one turns the breakdown into actionable items linked back via --parent. Code shape follows the templates_startup_ship.go pattern: - internal/collections/playbook_library_decompose.go (new) — holds decomposePlaybookBody + decomposePlaybookArguments + a DecomposePlaybook() helper. - internal/collections/playbook_library.go — registers DecomposePlaybook() in the workflow category right after PlanPlaybook(). Arguments: - target (required, ref) — the plan to decompose - dry-run (flag, default=false) — propose without creating - collection (optional, string, default=tasks) — for non-default workspaces (bugs, work-items, etc.) Body walks: load plan + existing children → analyze the body for actionable work → reconcile against existing children → propose task list with priorities and dependency hints → confirm in bulk → create approved tasks → wire dependencies via `pad item block` → report with suggested next moves. Mirrors skills/pad/SKILL.md's "Decomposition: 'Break plan X into tasks'" workflow so the conversational and invokable forms stay in lockstep (T7 covers SKILL.md sync). Closes the loop opened by T4: `/pad plan` step 7 now has a real `decompose` playbook to delegate to when it's activated in the workspace. Parent: PLAN-1397.	2026-05-13 07:01:49 -04:00
xarmian	ad3926f643	feat(library): author the plan invokable playbook (TASK-1401) (#530) ... * feat(library): author the plan invokable playbook (TASK-1401) New library entry: `/pad plan <topic>` — a conversation-first playbook for co-designing a new plan with the user. Subsumes the pre-PLAN-1377 "Plan Creation" library entry, reframed as a structured invokable procedure with explicit arguments. Code shape follows the templates_startup_ship.go pattern: - internal/collections/playbook_library_plan.go (new) — holds the body + argument spec as package-level constants and exports PlanPlaybook() LibraryPlaybook. - internal/collections/playbook_library.go — registers PlanPlaybook() in the workflow category right after ship. Arguments (mirroring the body's `## Arguments` section): - topic (required, string) - parent (optional, ref) - collection (optional, string, default=plans) Tone is generic across project types — no software-only vocabulary, no Pad-specific assumptions. A research workspace can plan an experiment; a hiring workspace can plan a recruiting push; the structure adapts via the conversation. Mirrors the "Planning: 'Let's create a plan'" workflow in skills/pad/SKILL.md so the conversational and invokable forms stay in lockstep (T7 docs pass calls this out). The body's step 7 invites the user to follow up with `/pad decompose <new-plan-ref>` — T5 (TASK-1402) authors that playbook next. Parent: PLAN-1397. * fix(library): plan playbook handles ref-form quick-action invocation, softens /pad decompose forward-reference per Codex review (round 1) Round 1 P2: /pad decompose forward-reference in step 7 would resolve to an unknown playbook until T5 (TASK-1402) merges. Reworded step 7 to check whether a `decompose` playbook is activated and fall back to inline `pad item create task` calls otherwise. Body now works regardless of whether T5 has landed. Round 1 P3: existing UI quick-action prompt (defaults.go:157) emits `/pad plan {ref} "{title}" — outline goals, deliverables, and timeline` on plan-collection cards. With my original topic-first arg shape, the agent's NL dispatcher would bind the ref as `topic` and treat the title as an extra positional, getting the wrong invocation mode. Fix: documented the dual-purpose `topic` argument explicitly. New "## Dispatch" section in the body tells the agent to detect: - First positional matches `^[A-Z]+-\d+$` AND resolves to a real item → elaborate mode (load item, work with user to expand it, update via `pad item update <ref> --stdin`) - Otherwise → create-new mode (full conversation flow below) planPlaybookArguments updated to mirror: topic now described as "string OR ref", parent/collection noted as create-new-mode only. Strict CLI parsing remains string-typed; the dual semantics are agent-interpreted per PLAN-1377's design. The existing "Plan this" quick-action prompt now routes correctly through the elaborate path without needing changes to defaults.go. * fix(library): align plan playbook philosophy section with step 7 fallback per Codex review (round 2) Round 2 P3: Philosophy said "never create tasks here" which contradicted step 7's new inline-task-creation fallback (added in round 1 to handle the case where no decompose playbook is active). Reworded to permit the fallback explicitly while keeping the "don't create before user approval" rule intact.	2026-05-13 06:57:29 -04:00