mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-25 03:42:06 +00:00
21001bc4c364f8988fd32e2ccb0bd3229dc085cd
1292 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 xarmian
|
21001bc4c3 |
feat(sessions): session-presence registry + GET /api/v1/sessions (PLAN-2558 S1) (#1091)
* feat(sessions): session-presence registry + GET /api/v1/sessions (PLAN-2558 S1)
Slice 1 of PLAN-2558 (IDEA-2544 Phase 3, web-UI push). The server can now
answer "is anything actually listening right now?" for the calling user.
WHY. `pad push` (Phase 1,
|
||
|
xarmian
|
da6ce642da |
feat(push): pad push — user-authored instruction dispatch to agent sessions (IDEA-2544 Phase 1) (#1090)
* feat(push): add pad push <ref> -m vertical (IDEA-2544 Phase 1)
Self-addressed, human-to-harness dispatch over the existing watch-events
bus/stream: CLI -> POST .../items/{itemSlug}/push -> a new KindPush
Notification (carrying the generalized TargetUserID addressed-to field
KindAsk will later share) -> watchNotificationVisible delivers it back
to the pushing user's own connected monitor sessions. Transient,
fire-and-forget by design (no migration, no durable inbox) since
assignment already covers the durable-notification case and this is
meant to be the explicit, no-inference dispatch verb instead.
* docs(plugin): document the push notification contract (IDEA-2544 Phase 1)
Push is the one notification kind that IS an instruction rather than a
passive fact, so it gets its own lead bullet in the plugin skill's
notification-etiquette section (ahead of the read-only/park default,
which it explicitly lifts) and a mention in the monitor's description.
The embed-source skills/pad/SKILL.md has no notification section to
mirror this into (the two files diverge by design) and is left
untouched.
* fix(push): reject over-long push messages instead of unbounded Summary
Comments truncate their notification Summary to a preview (the full
body is still fetchable), but a push message IS the payload — silently
truncating it would corrupt the instruction with nothing to recover it
from. Add maxPushMessageLen (4096, measured post-collapse) and reject
anything over it with a 400 rather than truncating; state the same
bound in `pad push --help` so it's discoverable before a 400, not only
from one.
* fix(push): close the watch-fallthrough leak, disambiguate SKILL.md exceptions
Codex round 1 P1: watchNotificationVisible's push branch only returned
early on a MATCH — a non-target caller fell through to the watch-map
check below it, so anyone holding an unconditional (or predicated)
watch on the item received every push addressed to every OTHER user,
instruction text included. Push is addressed private dispatch, not an
item-level fact watchers have a legitimate claim on (unlike assignment,
which watchers are expected to see per `pad watch --help`) — the branch
now returns unconditionally for KindPush, gating strictly on
TargetUserID and never reaching the watch-map fallback either way.
Pinned explicitly since Phase 4's session targeting is expected to
inherit this same exclusivity.
Also (codex P2): reworded the SKILL.md notification-etiquette bullets —
the new push exception and the pre-existing assignment/ask exception
literally contradicted each other ("the ONE narrow exception" claimed
singularity after push had already claimed exception status). Now
explicitly enumerated as the first and second exceptions to the
never-write rule.
* test(cli): pin that PushItem inherits X-Pad-Agent (BUG-2542 rebase)
Verified, not assumed: PushItem builds its request via c.post ->
c.newRequest like every other mutating client method (CreateWatch
included), so the attribution fix's client.agentName wiring covers it
for free with zero code changes needed on this branch. Adds a live
httptest assertion rather than trusting the code-path read alone —
the same shape as TestClientSendsResolvedAgentHeader, scoped to
PushItem specifically since that's the one method this PR added.
* fix(push): disambiguate workspace in the monitor line and skill contract
Codex round 2 P1: the watch-events stream is user-scoped ACROSS every
workspace a caller has watches in, but formatMonitorLine printed only
ItemRef/Kind/Actor/Summary and dropped the Workspace field the wire
payload already carried — a session linked to workspace A receiving a
notification for workspace B would resolve the wrong item (or 404) with
no signal in the line that anything was off.
Fixed universally, not push-only: grepped plugin/ and skills/ for
anything parsing "PAD ..." lines and found none — the Claude Code
plugin host ingests the stdout line as free-text notification prose,
formatMonitorLine's only real consumer is its own fmt.Println, so there
is no wire-format consumer a workspace prefix could break. The
ambiguity predates push (any watched item across workspaces already had
it); push just makes the consequence sharper because it carries an
instruction rather than a passive fact.
SKILL.md's push bullet now tells the agent to resolve with
`pad --workspace <workspace> item show <ref>` using the slug read off
the notification line, not a bare `pad item show <ref>`.
* fix(push): respect --format json instead of hardcoding plain text
Codex round 2 P2: pushCmd's RunE ignored the global format flag and
always printed "Pushed <ref>", silently discarding --format json.
- server.pushResponse replaces the bare map the handler wrote before —
a typed {ref, workspace, pushed, message} shape, with workspace
resolved to the CANONICAL slug via s.getWorkspace (not merely echoed
from whatever the URL contained), matching the same disambiguation
need the round-2 P1 fix addressed for the monitor line.
- cli.PushItem now returns (*PushResult, error) instead of discarding
the response body.
- pushCmd checks formatFlag == "json" and calls cli.PrintJSON, mirroring
runCreateWatch's existing pattern.
internal/cli/agent_identity_test.go's TestPushItemSendsResolvedAgentHeader
needed a one-line update for PushItem's new two-value return — caught by
`go vet ./...`, not `go build ./...` (which doesn't compile test files);
folding vet into my own pre-flight going forward.
|
||
|
xarmian
|
f9195c5b09 |
ci: make the go test timeout explicit everywhere (TASK-2545) (#1089)
* ci: make the go test timeout explicit everywhere (TASK-2545) The v0.13.0 release pre-flight died on `panic: test timed out after 10m0s` in internal/store, on a commit whose Go tree was identical to a green run an hour earlier. Nothing hung — the package's runtime simply crossed a budget nobody had chosen. `go test` without -timeout uses a 10m per-test-binary default. This repo raised the two RACE steps to 45m twice as the suite grew (BUG-1371 30m, BUG-1913 30m→45m), each time with a careful comment — and each time left their non-race siblings on the silent default. Three steps were still running on it, including the release gate: ci.yml "Run tests" (SQLite) ci.yml "Run tests against PostgreSQL" (the one that panicked) release.yml "Run tests" (the release gate itself) All three now carry -timeout=45m, matching the race legs so the file has one number, with comments saying it is a hang-catcher rather than a performance budget and that job wall-clock is the signal for "the suite got slow". Measured at |
||
|
xarmian
|
212d59e7c6 |
fix(cli,server): make agent attribution actually happen (BUG-2542) (#1088)
* fix(cli,server): make agent attribution actually happen (BUG-2542)
Agent CLI writes were recorded as the human whose credentials they used.
Three independent defects, each verified by reading the path AND by
probing a live instance — the item deliberately held the mechanism open,
so none of this is inherited.
1. THE HEADER WAS NEVER SENT. actorFromRequest sets actor="agent" on one
signal: the X-Pad-Agent header. The only code that sets it took the
value from `agent_name` in .pad.toml and nowhere else — no
environment detection, no session detection. This repo's .pad.toml
has only `workspace`, so the header has never been sent from here and
every agent write has looked human. ResolveAgentName now resolves
.pad.toml → $PAD_AGENT → detected runtime.
2. ITEM CREATE DISCARDED THE ACTOR. createItemChecked called
actorFromRequest and kept only the source (`_, src :=`), never
setting input.CreatedBy, so store.CreateItem fell through to its
"user" default — even for an agent that DID send the header.
Comments have always stamped it correctly; item creation silently did
not, which made the skill's own contract false on its own terms.
3. SINGLE-ITEM PATCH NEVER STAMPED LastModifiedBy. Bulk ops do
(handlers_items_bulk.go); the single-item path did not, so an item
edited only by agents read as human-edited.
Only entries VERIFIED against a live session belong in the runtime
detection table, so it has exactly one: Claude Code exports CLAUDECODE=1
to child processes, confirmed by reading a pad subprocess's environment
inside one. Guessing at Cursor/Windsurf/Aider variable names would put
unverified claims in a shipped binary and misattribute silently when
wrong; those set $PAD_AGENT until someone confirms a signature.
WHAT THIS DOES NOT DO, stated in the code and the skill rather than left
for someone to assume: the header is client-supplied and self-declared.
An agent that omits it is indistinguishable from the human it borrows
credentials from, and a human running `! pad ...` inside an agent's
terminal inherits that environment and is attributed to the agent. This
makes the trail HONEST, not VERIFIED — it is not a basis for
machine-verifiable human-approval provenance, which needs a channel the
agent cannot author at all. The incident behind this item is exactly
that distinction: an agent's relay of a human's words was recorded
indistinguishably from the human typing them.
Contract corrected in both skill copies, since the item's first question
was which of contract and behavior was wrong. It was the contract: it
promised automatic agent attribution that only ever applied to
workspaces that had opted in.
Tests, each mutation-tested against its own defect reverted alone:
- TestResolveAgentName — precedence plus the negative that makes it mean
something: a plain human shell must still resolve to "". Fails 2/5
reverted.
- TestItemAttribution_AgentVsHuman — agent and human legs for create,
update and the create-stamp-survives-edit invariant. Fails on the
create stamp reverted; fails 2/2 on the update stamp reverted.
The update leg deliberately uses the OTHER writer: insertItemTx seeds
last_modified_by FROM created_by, so a same-writer edit passes whether
or not the PATCH stamps anything — the first version of this test did
exactly that and passed its own counterfactual. Caught only because
each fix was reverted separately.
- TestItemAttribution_ExplicitBodyValueWins — an explicit body value
still beats the header.
End-to-end on a live instance through the real CLI, no .pad.toml opt-in:
agent session → created_by/last_modified_by/comment all `agent`; same
binary with CLAUDECODE stripped → all `user`.
Claude-Session: https://claude.ai/code/session_01QGbUKZBAZoWdEgiTNWsXag
* fix(server): artifact import wrote a UUID into created_by (BUG-2542)
Found by Codex while reviewing the attribution fix. handleImportArtifact
set `input.CreatedBy = u.ID`, which is the wrong DOMAIN for the field
rather than merely the wrong value: created_by holds the role — "user"
or "agent" — and consumers compare it against those literals
(CommentThread.svelte, TimelineVersionCard.svelte). An imported item
matched neither and rendered as neither.
It also would have defeated the fix in the parent commit at this path: a
non-empty CreatedBy suppresses the actor stamp, so imports would have
kept a UUID while every other create path started recording the actor.
The line contradicted the comment directly above it, which said Source
was being left blank precisely so createItemChecked could stamp it "like
every other create path". Now both fields are left blank and stamped
together.
The user's identity has its own home — the items.created_by_user_id
column — which no create path currently populates. That is a separate
gap and is not widened into this change.
Claude-Session: https://claude.ai/code/session_01QGbUKZBAZoWdEgiTNWsXag
* fix: close the remaining attribution bypasses Codex found (BUG-2542)
Review found no P1s and three P2 families beyond the artifact-import bug
already fixed in
|
||
|
xarmian
|
b87b3028e7 |
chore(nix): bump package version to 0.13.0 ahead of the release tag
Claude-Session: https://claude.ai/code/session_01CXHLbTC1AiwSC87xwThRGTv0.13.0 |
||
|
xarmian
|
f241a6298e |
docs(skill): port the applicable plugin-review corrections into the embed source (TASK-2537) (#1087)
* docs(skill): port the applicable plugin-review corrections into the embed source
skills/pad/SKILL.md is the //go:embed source that `pad agent install`
writes into user projects; it had drifted from corrections made to the
plugin's copy during TASK-2534's review rounds. Selective port — the two
files diverge by design (surface-agnostic vs Claude-Code-only), so none
of the plugin's Claude-Code-specific material comes across.
Each ported item re-verified against the code, since line numbers and
wording differ between the copies:
- Ideation example passed `--content "..." --stdin` together.
cmd_item.go:141 shows --stdin OVERWRITES the --content value and then
blocks on io.ReadAll with nothing piped, so the example as written
hangs. Dropped --stdin.
- Retro's plan-status flip carried no --comment, contradicting Key
Principle 2 four lines below it.
- Key Principle 2 named `blocked` as a task status. templates.go:157 has
open / in-progress / done / cancelled — no `blocked`.
- The role-board pointers claimed `pad server open` → /{workspace}/roles.
cmd_server.go:999 appends only the workspace slug; there is no
sub-path. Replaced with navigate-to-the-Roles-page wording.
Two more from the same review rounds that apply here and were not listed
on the task, found by diffing the copies:
- The convention/playbook BODY loads used `--format json` without
`--full`. Since the v0.9 summary shape, `pad item list` returns
cli.ToItemSummaries, which has no `content` field at all — so a skill
told to "follow ALL returned conventions" was reading titles. Added
--full to the seven trigger-load examples and to the retro's task
load, where the bodies are the point.
- `open "$IMG"` is macOS-only, in a file installed on every platform.
Not ported, and not a gap in this file: the whoami-gated
`pad workspace init` routing. That correction guards a blind self-heal
in the plugin's bootstrap-failure branch, and the embed source has no
such branch — its only `pad workspace init` mention is a neutral
see-also. The absence of ANY bootstrap-failure guidance here may be
worth its own item; inventing that section is outside a port.
Verified through the real embed path, not by reading the diff: rebuilt
and ran `pad agent install` for both targets (claude → .claude/skills,
codex/cursor/windsurf/opencode → .agents/skills) into scratch dirs, and
confirmed the installed artifacts carry every correction and none of the
superseded strings. go test ./... green.
Claude-Session: https://claude.ai/code/session_01QGbUKZBAZoWdEgiTNWsXag
* docs(skill): --full on the eighth body-load; file the bootstrap-failure gap
Codex found one miss and one scope question.
The miss: the `convention_index` bullet's own body-load example lacked
both `--format json` and `--full`, so the call that the bullet exists to
describe — pull the triggered convention BODIES this index only names —
came back in the summary shape with no `content`. It is the eighth such
call; I corrected seven and missed the one inside prose rather than in a
code block. Added, with a clause saying why, since this is the bullet a
reader consults specifically to learn how to fetch bodies.
The scope question: the embed source has no bootstrap-failure branch at
all, so an unlinked workspace routes into onboarding that cannot run,
and a naive `pad workspace init` self-heal can hang the tool call
indefinitely. Codex is right that it is a real gap and right that it is
surface-agnostic. It is not a port, though — the plugin's correction
guards a self-heal this file does not contain, so there is nothing here
to correct, only something to write. Filed as BUG-2541 with the failure
modes, the verified hang, and the de-Claude-ing needed, rather than
widened into this PR.
Re-verified through `pad agent install`: 9 body-load calls now carry
--full in the installed artifact.
Claude-Session: https://claude.ai/code/session_01QGbUKZBAZoWdEgiTNWsXag
* docs(skill): warn off the blind `pad workspace init` self-heal
Codex held on the bootstrap-failure gap after I deferred it to BUG-2541,
and the objection is fair on one point: deferring the whole branch left
the shipped artifact one step from the hazard, since the documented
fallback ("use the individual CLI calls") needs the same workspace link
that just failed, so a reader following this file has nowhere to go and
`pad workspace init` is the obvious next reach.
So the hazard gets addressed here and the structure stays in BUG-2541.
Four sentences, surface-agnostic: bootstrap failing usually means setup;
the individual-call fallback won't help; do NOT run `pad workspace init`
blind, because on a configured-but-unauthenticated machine it hangs
indefinitely on a browser-setup URL with no timeout and no
non-interactive fallback — wedging the tool call rather than failing it;
gate on `pad auth whoami` and hand back to the user otherwise.
That IS what TASK-2537's finding-1 asked for — "the same whoami-gated
treatment" for this file's onboarding-adjacent text. I first read its
parenthetical ("if any references pad workspace init as a blind
self-heal") as gating the whole item, and since this file has no such
reference, as nothing to do. The intent was to keep the file from
leading an agent into the hang, which it could.
What stays in BUG-2541: the two stderr signatures as named cases, the
Onboarding routing entry's missing precondition, and re-verifying the
hang rather than inheriting the claim. Noted there.
Verified through `pad agent install` again, not the diff.
Claude-Session: https://claude.ai/code/session_01QGbUKZBAZoWdEgiTNWsXag
* docs(skill): narrow the workspace-init warning to what the code does
I wrote that `pad workspace init` "hangs indefinitely ... with no
timeout", inherited from TASK-2534's write-up. Codex flagged it and it
is wrong. What the code does:
- first-admin setup polls under an explicit 20-minute cap
(internal/cli/bootstrap.go::bootstrapPollTimeout);
- the configured-but-unauthenticated branch polls a CLI auth session
every 2s with no wall-clock timeout of its own, but exits on the
server's `expired` status, and that session's TTL is 5 minutes (20 for
the setup handoff) — internal/store/cli_auth_sessions.go.
Bounded, then. The hazard is still real and still worth the warning — it
blocks an agent's tool call for minutes on a flow only a human at a
browser can finish — but the text now says that instead of claiming a
permanent wedge. Also qualified the whoami claim: it returns immediately
in NON-INTERACTIVE use; getConfiguredConfig can prompt on an interactive
TTY (cmd/pad/configure.go:80).
Worth naming because it is the same failure I had just written into
BUG-2541 as work to do — "re-verify the hang claim rather than
inheriting it" — and then committed the inherited claim as fact in the
same breath. An explanation I have not checked is a claim, not a hedge,
and putting it in a file that ships to users makes it everyone's. The
correction is on BUG-2541 too, so that item's body isn't left asserting
it.
Claude-Session: https://claude.ai/code/session_01QGbUKZBAZoWdEgiTNWsXag
|
||
|
xarmian
|
e91c4fc261 |
fix(store): include the cursor's own second in /changes deltas (BUG-2539) (#1086)
* fix(store): include the cursor's own second in /changes deltas (BUG-2539)
items.updated_at / items.deleted_at are RFC3339 whole-second strings
(store.now()), while the /changes cursor is a unix-millisecond value —
normally the previous response's server_time. ItemsModifiedSince
formatted that cursor with the same second precision, truncating it
DOWN, then compared with a strict `>`. Every change landing in the
cursor's own second compared equal and was dropped, permanently: the
caller advances its cursor past that second and nothing reaches back.
User-visible symptom: a bulk archive ~450ms after a page seeded its
cursor left the item rendering as LIVE indefinitely — no banner, no
redirect — while the server had deleted_at set. It was never
archive-specific (updates were dropped identically); a missed update is
usually re-delivered by the next event, a missed deletion never is.
Compare inclusively against the truncated second instead. The boundary
second may be re-delivered, which every consumer of this endpoint
applies idempotently, and it is bounded to one second of changes per
sync. Sub-second storage is the other fix and is a migration, not a
one-liner: these comparisons are lexicographic on TEXT columns and
mixing precisions inverts them ("…20.451Z" sorts BEFORE "…20Z").
Verified against a live instance with four cursors all strictly earlier
than the archive in real time: two inside its second MISS, two in
earlier seconds HIT.
Tests:
- TestItemsModifiedSince_SameSecondCursor — same-second leg plus a
previous-second control. Fails 3/3 unfixed, passes 3/3 fixed; the
control passes on both.
- e2e bug-2539-sync-window — the banner must appear in the already-open
page AND follow a /changes delta that carried the deletion, so a
reload cannot satisfy it. The 450ms leg fails unfixed; 1200ms control
passes on both.
Claude-Session: https://claude.ai/code/session_01QGbUKZBAZoWdEgiTNWsXag
* test(store,e2e): close the review gaps in the BUG-2539 counterfactuals
Codex review of
|
||
|
xarmian
|
d8627968d0 |
feat(plugin): pad Claude Code plugin — nested layout, monitors, skills (PLAN-2469 Phase 2) (TASK-2534) (#1085)
* feat(plugin): assemble Phase-0 scaffold v3 into a nested plugin layout — PLAN-2469 Phase 2 (TASK-2534)
Drops scaffold v3 (plugin.json, marketplace.json, monitors.json, the pad/
status/capture/onboard skills) in under plugin/, verbatim, rather than at
repo root. Repo root already embeds skills/pad/SKILL.md into the binary
as the surface-agnostic `pad agent install` source (embed.go); loading
the plugin flat at repo root would register BOTH that copy and the
plugin's Claude-Code-only trimmed copy as separate skills in the same
session (confirmed empirically via --plugin-dir) and would ship the
whole repo as the marketplace install payload (confirmed via a real
marketplace add + install + cache inspection). Nesting under plugin/
with marketplace.json's source: "./plugin" scopes both discovery and
the install payload to exactly the plugin's own files, with zero
content changes to the scaffold itself.
The stale minimal .claude-plugin/plugin.json at repo root is removed —
superseded by the real one at plugin/.claude-plugin/plugin.json; repo
root's .claude-plugin/ now holds only marketplace.json, which is what
`/plugin marketplace add PerpetualSoftware/pad` needs to find there.
`claude plugin validate --strict` passes clean on both the repo root
(marketplace) and plugin/ (plugin manifest) independently.
* fix(plugin): round-1 review fixes — invocation reality, degradation ordering, confirm-by-default, license (TASK-2534)
Six findings from codex round 1 + dispatcher's shallow pass:
- pad/SKILL.md's "How This Works" claimed a literal `/pad <anything>`
command, but under plugin namespacing this skill registers as
`pad:pad`, not a bare `/pad`. Rewrote to state the true DR-1 model
(contextual trigger by description, not typed) and defined the
document's own `/pad <anything>` notation as shorthand for "what the
user said," not literal syntax. Fixes the earlier /pad:pad cosmetic
note at its root and the two "onboarding" sections that repeated the
same wrong dual-syntax framing.
- status/SKILL.md and capture/SKILL.md invoked `pad` before stating
their missing-CLI fallback; reordered so the failure modes are
handled as part of the same instruction, not an afterthought.
monitors.json's command had no guard at all — a padless machine
would hit a raw spawn error, not silence. Added a `command -v pad`
shell guard (verified live, both branches: silent when pad is
absent, invokes correctly when present) and re-verified the two
original degradation legs (no .pad.toml, unreachable padd) through
the exact guarded command string, not just the bare binary.
- `pad bootstrap` hard-exits ("no workspace linked...") when unlinked
(cmd/pad/main.go's getWorkspace()), but the skill assumed it always
succeeds and the onboarding NL routing tried to load the onboard
PLAYBOOK before a workspace — and therefore that playbook — could
exist. Added the unlinked-workspace branch to Context Loading and
taught onboarding routing to run `pad workspace init` first in that
case, matching what skills/onboard/SKILL.md already did correctly.
- capture/SKILL.md's "no ceremony" immediate-create contradicted the
main skill's "always confirm before creating/modifying." Made
confirm-first the default, with an explicit opt-out for workspaces
whose conventions declare autonomous capture.
- Two `open "$IMG"` examples were macOS-only; added the Linux
(xdg-open) alternative and a describe-the-path fallback.
- plugin.json declared MIT; the repo is Apache-2.0 (LICENSE,
web/package.json). Corrected.
`claude plugin validate --strict` clean on both the repo root
(marketplace) and plugin/ (plugin manifest) after the fixes.
* fix(plugin): make DR-4's no-write default unmistakable (TASK-2534)
Second independent dogfood run found the etiquette text underdetermined
behavior: one run correctly parked a context nudge, another interpreted
"fold it in" as license to run pad item comments and post a real reply
to the watched item — a write action from a context notification, which
DR-4 never intended and the plugin's noise-discipline promise depends on
never happening. Rewrote the section: read-only one-line park-and-summarize
is now the explicit default for every watch notification and any addressed-
to-you event the model isn't certain about; writing to Pad in reaction to a
notification is called out as prohibited by name (no more "fold it in"
euphemism), with a single narrow exception (an assignment/ask explicitly
addressed to the session's user, where acting is unambiguously expected)
and an explicit "when in doubt, park" tiebreaker.
claude plugin validate --strict clean on both repo root (marketplace) and
plugin/ (plugin manifest).
* fix(plugin): loop the pad-missing monitor guard instead of a one-shot sleep (TASK-2534)
Verifying the finding-2 guard through Claude Code's REAL monitor spawn
path (--bg --plugin-dir, not a manual `sh -c` test) surfaced a real gap
the manual test couldn't: the runner does NOT relaunch a monitor command
after it exits. The round-1 guard (`command -v pad && exec ... || sleep
3600`) is a single check-then-sleep-then-exit — once that one hour
elapses, the process exits and, per this observed behavior, is never
restarted, leaving the session permanently monitor-less for the rest of
its life on a machine where pad wasn't yet on PATH at session start.
That silently breaks DOC-2479's "sleep-retry hourly" contract, which the
Go binary's own `pad watch --stream --for-session` loop honors correctly
for its two conditions (no .pad.toml, padd unreachable) via an actual
internal retry loop — this shell guard needs the same shape for its one
condition (pad binary absent).
Wrapped the check in `while ! command -v pad; do sleep 3600; done` so it
keeps re-checking indefinitely instead of dying after one interval, then
falls through to the same `exec pad watch --stream --for-session` once
pad becomes resolvable. Verified live through the real spawn path with a
shortened sleep interval on a throwaway plugin copy: a fresh sleep child
appears every cycle (confirms the loop keeps running, not one-shot), then
re-verified the actual committed command (real sleep 3600) resolves a
live `pad watch --stream --for-session` process when pad is present and
sits in a healthy, silent, no-error `sleep 3600` loop when it's absent —
both through claude --bg --plugin-dir, not manual shell invocation.
claude plugin validate --strict clean on both repo root (marketplace)
and plugin/ (plugin manifest).
* fix(plugin): probe watch capability, not just binary existence, in the monitor guard (TASK-2534)
Codex round 2, P1: the round-1 fix looped on `command -v pad`, but an
old pad binary predating `pad watch` resolves on PATH fine and then
dies non-zero the moment `exec pad watch --stream --for-session` runs
— and the previous commit already established the runner never
relaunches an exited monitor command, so that death is permanent for
the session. `command -v` only proves the binary exists, not that it
has the subcommand this guard needs.
Changed the loop condition to `pad watch --help`, which probes the
actual capability in one call: covers both missing-binary (no such
command) and old-binary-without-watch (unknown subcommand, non-zero
exit) with a single condition. Verified first that `--help` is safe to
call as a probe — it succeeds (exit 0) with no `.pad.toml` anywhere
and padd unreachable, so it can't deadlock a legitimate unlinked-
project or padd-down user behind the guard.
claude plugin validate --strict clean on both repo root (marketplace)
and plugin/ (plugin manifest).
* fix(plugin): round-2 doc fixes — capture routing, confirm scope, invocation wording, CLI accuracy (TASK-2534)
Findings 2-8 from codex round 2:
- capture/SKILL.md: no unlinked-workspace branch — would fail opaquely
on `pad collection list` without .pad.toml. Mirrors the same routing
fix the main skill already has: run `pad workspace init` first.
- pad/SKILL.md's notification write-exception didn't say whether the
confirm-always principle (Key Principles #3) still applies inside it.
It does — the exception only lifts never-write, not confirm-first —
now stated explicitly where the exception lives, same rule as
capture's autonomous-capture opt-out.
- The `/pad` shorthand disclaimer scoped itself to "the playbook-routing
and examples sections below," missing Context Loading's "on every
`/pad invocation`" a few lines above it. Widened to cover the whole
document. plugin.json's own description repeated the bare "/pad
conversational surface" claim; reworded to name the real typed
shortcuts instead, and synced marketplace.json's plugin-entry
description to match (was already a paraphrase, now identical).
- Ideation example passed both `--content` and `--stdin` to the same
`pad item create` call — verified --stdin wins and hangs on ReadAll
with no piped input. Dropped --stdin, kept --content.
- Plan-retro example was missing the mandatory `--comment` this same
doc teaches elsewhere. Key Principles #2's status enumeration listed
"blocked" as a task status — verified against
internal/collections/templates.go: the tasks schema's actual options
are open/in-progress/done/cancelled, no "blocked". Fixed both.
- Two role-board pointers claimed `pad server open` lands on
`/{workspace}/roles` directly; verified cmd/pad/cmd_server.go's open
command only appends the workspace slug, never a sub-path. Reworded
both to describe navigating to Roles from the opened UI rather than
asserting an unverified deep-link.
claude plugin validate --strict clean on both repo root (marketplace)
and plugin/ (plugin manifest).
* fix(plugin): safe (non-hanging) unlinked-workspace self-heal, onboarding offer in status, ask-events wording (TASK-2534)
Codex round 3, findings 1-3:
- The routing added in rounds 1-2 told the skill to run bare
`pad workspace init` whenever bootstrap reported "no workspace
linked." Verified live this can HANG INDEFINITELY: with pad
configured (mode/URL set) but no admin account created yet /
session not authenticated, `pad workspace init` calls a
browser-based setup/login flow with no TTY guard and no
non-interactive fallback — unlike `pad init`, which has an explicit
`!canPromptForConfig()` check at the same step and fails fast with a
headless-bootstrap hint instead. Bootstrap's own two failure
signatures ("Pad is not configured" vs "no workspace linked") don't
fully disambiguate this either — verified live that "no workspace
linked" fires even when the deeper problem is "no admin account
exists yet," which is exactly the state that hangs.
Fixed by adding a `pad auth whoami` preflight (verified live: fast,
safe, never blocks, in all three states — unconfigured, configured-
but-unauthenticated, and fully set up) before ever attempting
`pad workspace init`. Only self-heal when whoami reports a real
user; otherwise tell the user to run `pad init` themselves in an
interactive terminal (Claude Code: suggest `! pad init`). Applied
consistently across pad/SKILL.md's Context Loading and Onboarding
routing, onboard/SKILL.md, and capture/SKILL.md's unlinked-workspace
branch (round 2 addition, same hang risk).
- status/SKILL.md ran `pad project dashboard --format json` but never
checked its `needs_onboarding` field (verified present on that
response too, not just bootstrap's — internal/server/handlers_
dashboard.go's NeedsOnboarding), so the mandated onboarding offer
couldn't fire via `/pad:status`. Added the same offer wording the
main skill uses.
- monitors.json's description claimed the stream delivers "asks for
your role" alongside assignment; verified against
internal/watchevents/watchevents.go: KindAsk is contract-reserved
with no Phase 1 producer. Reworded to describe what ships today,
noting ask-events as reserved-not-yet-emitted.
Filed docapp TASK-2537 for the fourth finding (embed-source
skills/pad/SKILL.md has the same --content/--stdin bug and has
drifted from these corrections) — selective port, out of this diff's
scope per the two skills' by-design framing divergence.
claude plugin validate --strict clean on both repo root (marketplace)
and plugin/ (plugin manifest).
* fix(plugin): --full on body-promising queries, capture loads always-on conventions, onboard's already-set-up path (TASK-2534)
Codex round 4, findings 1-3:
- Every `pad item list conventions|playbooks --format json` example whose
surrounding prose promises "bodies" was missing `--full` — verified
against cmd/pad/cmd_item.go: the flag exists specifically because JSON
output defaults to a token-light summary shape with no content.
Without it, all 8 of these calls (the Context Loading bootstrap-field
description at :41, the 7 trigger-query examples under "Before
Performing Work") would return exactly what the surrounding text says
they're loading bodies to avoid: metadata with no content. Added
--full to all 8, not just the block the finding cited — the "Template"
lines carry the identical "pull their bodies" promise as the
"Concrete examples" lines a few lines below them, so leaving one set
fixed and the other not would just move the inconsistency.
Swept the rest of the file's `item list ... --format json` examples
(Ideation load-context, Status Check role queue, Daily Standup) —
left those alone; their surrounding prose asks for enumeration/counts
("3 items in your queue", "Yesterday/Today/Blockers"), not body
content. The one exception found: Retrospective's task-load line
promises "what shipped, what was deferred, lessons learned" — that
needs actual task content to synthesize, so it got --full too, with
a one-line note explaining why (the other fixed lines didn't need
one; their promise was already explicit in the surrounding prose).
- capture/SKILL.md never loaded conventions at all. Per the dispatcher's
ruling: low ceremony means skipping bootstrap's dashboard/playbook/
role weight, not skipping mandatory project rules (trigger=always
conventions exist precisely to be small and always-applied). Capture
now runs the always-on conventions query (with --full) before
creating and applies whatever it returns; the autonomous-capture
exception now checks the SAME load instead of running its own
separate (previously --full-less) query. One sentence documents the
tradeoff as deliberate: always-on rules yes, full bootstrap no.
- onboard/SKILL.md only defined the needs_onboarding=true branch. Added
the false branch: say the workspace is already set up, summarize what
exists (collection/convention counts from the bootstrap payload
already in hand), and offer extend/audit instead of re-running
first-time setup — same confirm-first rule as everywhere else.
claude plugin validate --strict clean on both repo root (marketplace)
and plugin/ (plugin manifest).
|
||
|
xarmian
|
2cb5e0aab0 |
fix(web): surface swallowed sync failures; defer a mid-sync sync_required (BUG-2508) (#1084)
* fix(web): stop losing sync changes when a consumer fails to apply them (BUG-2508)
Three defects on the incremental-sync path, all reproduced before any fix. The
reproduction IS the regression test (syncCursor.svelte.test.ts) rather than a
throwaway, because what proves the loss and what pins it are the same artifact.
1. THE CURSOR ADVANCED BEFORE CONSUMERS APPLIED. `triggerSync` set
`lastSyncTime = changes.server_time` and then notified, so a consumer whose
refetch failed left the cursor past changes nobody had applied — and since
`/changes` is asked FROM that cursor, the server could never re-deliver them.
Silent and permanent. The `full_refresh` arm three lines below already had
the right discipline ("don't advance until pages confirm success"), so the
file disagreed with itself; the incremental arm now shares it via `deliver`.
2. ASYNC CONSUMERS' REJECTIONS WERE NEVER OBSERVED AT ALL. `notify` wrapped
`cb(result)` in try/catch, which catches synchronous throws only, and two of
the five consumers are async. Their rejections did not reach that catch —
they surfaced as unhandled rejections while the service went on believing
the sync had been applied. Callbacks are now awaited, so "caught and ignored"
and "not caught at all" collapse into one honest answer, and the failure is
logged instead of dropped.
3. A `sync_required` ARRIVING MID-SYNC WAS DROPPED, not deferred. The in-flight
request was issued before that signal, so its window cannot cover it, and
nothing re-announces the gap. `triggerSync` now records it and runs one more
pass; the flag is cleared before each request so a signal arriving during one
is not swallowed by the pass that predates it.
Plus the second half of the report: the timeline's SSE-driven refresh caught its
failures and did nothing at all with them, leaving the panel quietly missing a
comment somebody else had just posted, with no indication and no retry — the
next refresh only comes with the next relevant SSE event, which may never
arrive. It now logs and retries ONCE on a backoff. Not a banner (this is a
background refresh, and a modal-weight failure surface would be worse than the
bug) and not unbounded retries (the debounce exists because SSE replay can
hammer that endpoint).
Scope held to the triage: `doIncrementalOrFull`'s fallback behaviour is
untouched.
The tests assert the `since` ARGUMENT OF THE NEXT REQUEST, never an end state
(team CONVE-12): "changes lost", "changes never made" and "refetch succeeded
with nothing to do" are indistinguishable by end state, and what separates them
is whether the server can still be asked. A control leg pins that a clean sync
still DOES advance the cursor, so a fix that simply never advances it fails.
Three mutants, each failing only its own tests. The rig itself nearly produced a
false green — the service is a module singleton whose `setWorkspace` issues its
own `/changes` call, and the first version counted it and leaked cursor state
between tests, which made one leg pass on another leg's calls.
* test(web): cover the timeline's SSE-refresh retry, and make its SSE mock fan out (BUG-2508)
The retry added in the previous commit had no test. Adding one required fixing
the harness first: this file's `sseService` mock returned a disposer and dropped
the callback, so the component was subscribed to nothing and any test of how it
REACTS to an event would have passed vacuously. It now fans out, which is the
same mock defect BUG-2509 hit on the attachment bus.
Both legs assert on the REQUESTS issued, not on rendered entries: a timeline that
never refreshed and one that refreshed successfully with nothing new look
identical on screen (CONVE-12). The control leg — a successful refresh must NOT
retry — is what stops a fix that simply retries unconditionally from passing,
which would double every successful refresh on an endpoint the debounce exists to
protect.
Two mutants: removing the retry and dropping the once-only guard each fail only
this test.
* fix(web): fence the timeline's SSE refresh against teardown (BUG-2508)
The retry added earlier in this branch could fire after unmount: a rejected
request schedules it from its own catch, and the identity fence there
(reqSlug/reqWs) is not a teardown fence — a remounted panel can legitimately
carry the same identity, so "same item" never meant "still alive". `onDestroy`
only unsubscribed SSE and left both timers running.
Now `onDestroy` clears the shared timer and latches `destroyed`, which every
continuation that can outlive the mount checks: entry, the success path before
it writes state, and the failure path before it schedules the retry.
Found by an independent review pass on this branch — the leak was mine, introduced
with the retry.
* fix(web): revert the cursor gate; keep the failures observable (BUG-2508)
Scope call after review: keep this bug narrow, and revert the coordinator change
rather than ship it.
The gate ("advance the cursor only if every consumer applied") was correct at the
service boundary and INERT in production, because no consumer reports failure —
verified at four sites, and confirmed by a live leg that behaved identically on
gated and ungated builds. Shipping it would have read as a fix for the reported
bug while changing nothing, which is worse than the open bug: the next person
cites it as handled. Reverting an inert change loses nothing users ever had.
The design half — consumer contract, the poison-consumer case that would pin the
cursor for everyone against an unbounded /changes window, and the
markSynced/onTabResume inconsistencies — is filed as IDEA-2535.
What ships here, all within "surface the failures":
- FAILURES ARE OBSERVABLE. The old try/catch caught synchronous throws only, and
two of the five consumers are async with their promise discarded — those
rejections were not "caught and ignored" but unobserved entirely, surfacing as
unhandled rejections with nothing tying them to the sync that caused them. A
rejection handler is now attached to whatever a callback returns, and both arms
log. Deliberately NOT awaited: consumers keep running concurrently and delivery
stays synchronous, so observability does not smuggle in an ordering change.
- A `sync_required` ARRIVING MID-SYNC IS DEFERRED, not dropped. The in-flight
request was issued before that signal, so its window cannot cover it, and
nothing re-announces the gap.
- Cursor semantics are UNCHANGED, and a test pins that deliberately: if someone
reinstates the gate, it fails and sends them to IDEA-2535 rather than letting an
inert contract ship quietly a second time.
Three mutants (drop the async observer, drop the sync log, drop the deferral
flag), each failing only its own test, plus a control that a sync with nothing
pending runs exactly one pass.
|
||
|
xarmian
|
d8b68c443e |
fix(web): clear the attachment NodeView missing-latch on parent restore (BUG-2509) (#1083)
* fix(web): clear attachment NodeView missing-latch on parent restore (BUG-2509)
Archiving an item 404s its attachments without deleting them (DR-13). Any
attachment surface that PROBES inside that window therefore observes exactly
what a deletion produces, latches it as permanent, and stays dead after the
restore. The strip and the timeline already reconcile this themselves via the
`parentArchived` prop plus their own epoch + no-store re-probe; the editor
NodeViews could not — their latch is closure-private state inside a Tiptap
view, unreachable from a prop — so they were routed out of PLAN-2392 3c-iii's
scope and never got the equivalent.
Two distinct defects, both reproduced live before any code was written:
1. The shared HEAD metadata cache memoizes `missing` for the page lifetime, on
the premise that a settled result is a durable fact about a content-addressed
row. True for deletion, false for archive: a `missing` observed in the
archived window is a fact WITH an expiry cached as though it had none. Every
later reader replays a 404 the server would no longer give — INCLUDING a
NodeView constructed fresh, which is why remounting the editor did not heal
the file chip.
2. The NodeView `deleted` latch is cleared only by a uuid swap, and a restore
does not change the uuid.
Which one bites depends on `canEdit`, which is forced false while archived: an
edit-permissioned user's restore flips the content branch and builds a fresh
editor (healing the image, whose load event repaints it, but not the chip, which
makes no request and reads the poisoned cache), while a viewer keeps the SAME
editor across the whole flip and sees both stay dead.
The fix is a restore channel on the attachment bus — deliberately NOT the mirror
of the deletion channel. Deletion is authoritative and subscribers latch it;
this signal carries NO VERDICT and only prompts a re-ask. Subscribers re-probe
no-store and clear the latch ONLY on an authoritative `ok`, so an attachment
genuinely deleted while its parent was archived 404s and stays dead, and a
mis-routed signal costs one HEAD and changes nothing. That is what keeps restore
from becoming an undo-resurrection vector (DR-17).
`announceAttachmentParentRestored` does both halves because they cover different
populations: the notify reaches surfaces already mounted and latched (a cache
invalidation cannot — their latch is not a cache read they repeat), and the
invalidation covers surfaces built later, which a notify cannot reach because
they did not exist when it fired. Invalidate first, so a subscriber re-probing
synchronously inside the notify is not answered from the entry being dropped.
Verified in a real browser against an isolated instance, on both legs (owner and
viewer, item opened while archived then restored with no reload), plus the DR-17
leg (attachment row genuinely deleted during the archived window — stays dead)
and the control (opened live, then archive/restore — never latches, since the
latch requires the NodeView to be CONSTRUCTED inside the archived window).
Tests: the three bus mocks now fan the restore channel out to subscribers rather
than only recording the subscription — a spy-only mock leaves the NodeView
subscribed to nothing and every reaction test passes vacuously. New coverage for
the cache invalidation, the channel (including the invalidate-before-notify
ordering, asserted through its observable consequence), and both NodeViews' heal
/ stay-dead / routing / teardown behaviour. Each new assertion was mutation-
tested: no-op'ing the invalidation fails 3, short-circuiting either listener
fails the heal tests.
* fix(web): fence the restore probe against deletion, workspace and item-switch (BUG-2509)
Three defects an independent review pass found in the previous commit. The
first is the safety property that commit claimed to protect.
1. DR-17 RESURRECTION, reachable in one interleaving. The deletion bus sets the
latch synchronously, but the restore continuation fenced only on teardown and
a uuid swap — so "restore probe starts → delete is confirmed and broadcast →
probe resolves ok" cleared the latch and repainted a row the server no longer
has. The browser leg missed it because there the delete preceded the signal,
so the probe itself 404'd; the hole is only in the overlap. Fixed with a
monotonic deletion generation captured before the probe and re-checked after —
a bare `deleted` re-check would not do, since the latch can be set and cleared
again while one probe is in flight.
Relatedly, a non-`ok` result was treated as "do nothing" when `missing` is in
fact the authoritative existence answer the probe asked for. It now latches,
which also settles two concurrent probes resolving out of order rather than
letting whichever answered first win.
2. The continuations never re-read the address after the await, so an answer
about ws-A's copy could heal a node whose editor had since moved to ws-B —
the same cross-workspace staleness that made these files read the workspace
off the live reader in the first place. Re-checked on landing, as
ItemTimeline's probe already does.
3. The ItemDetail edge was keyed on the LEVEL (`itemMatchesRef && isArchived`),
which also drops when you navigate AWAY from an archived item — so every such
navigation announced a restore that never happened, invalidating a workspace's
metadata cache and prompting probes. The latch now carries the item id: a
restore is "the same loaded, matched item went archived → live", nothing else.
Extracted as `parentRestoreEdge` and unit tested, because ItemDetail resists
jsdom mounting and that case is otherwise unpinnable.
Also: the two teardown tests passed vacuously — a leaked listener still sees
`destroyed` and returns, so they stayed green with the dispose call deleted.
They now assert the registry is empty. Mutation testing then caught the same
class in two of my own new tests: the image's mid-probe-deletion and
workspace-change cases asserted only that the placeholder was still visible,
which is true either way until a `load` fires. They now assert the observable
consequence of a wrong heal — the cache-busted reload it would arm.
Every new fence was mutation-tested individually (5 mutants, each failing only
its own tests). Full suite 1496 passed; svelte-check 0 errors; all three browser
legs re-verified against a binary built from this tree.
* fix(web): generation-fence every probe; key the restore signal by item (BUG-2509)
Round two of independent review. Three more findings, all confirmed against the
source; the third means the previous fix was incomplete for a realistic
navigation, not merely unfenced.
1. A STALE PROBE COULD STILL REACH THE LATCH PATH. The previous fence was
per-CAUSE (did a deletion land?) rather than per-TRANSITION, so it only caught
one direction. The other: an OLDER probe answering after a NEWER transition —
the image's archived-window probe still in flight when the restore heals, then
re-latching what the restore just fixed; and the chip's CONSTRUCTION probe
(issued inside the archived window, which is the whole bug) landing after the
restore signal and marking a live chip dead with nothing left to undo it.
Replaced with one monotonic per-NodeView generation, bumped on every
authoritative transition — deletion, missing-latch, heal, uuid swap, and
receipt of a restore signal — captured by every continuation that mutates
presentation. The invariant is now structural rather than a list of cases:
a continuation may only act if nothing authoritative happened since it
started. Bumping on receipt is what invalidates a probe issued before the
restore, including the early-return path where there is nothing to heal.
2. UUID FENCING WAS BY VALUE, so a swap away and back passed the check again —
and the deletion listener filters on the CURRENT uuid, so a delete arriving
while the node pointed elsewhere was ignored. Stale ok + same uuid on return =
resurrection. The swap now bumps the generation, which is what the
continuations compare.
3. THE EMITTER MISSED RESTORES THAT HAPPEN WHILE THE PANE IS AWAY. Archive A,
navigate to B, let someone else restore A, come back: no archived→live edge is
ever observed by this tab, so nothing was announced and a fresh chip read the
archived window's cached 404 — the original bug by another route. A per-mount
edge latch cannot see this, and keying it to the item id does not help; the
memory has to outlive the mount. Replaced `parentRestoreEdge` with
`archivedItemRegistry`: mark an item when seen archived, announce when it is
next seen live. That subsumes every case the latch handled (restore in place,
navigate away, mount-on-archived) and covers the one it structurally could not.
Verified live, same document throughout (asserted, not assumed — the leg stamps
the document and checks the stamp survives, since a full reload would drop the
very state under test). Control: the same leg against a binary built from the
PREVIOUS commit's emitter leaves the chip dead on return while the image heals,
which is exactly the predicted symptom — so the leg discriminates rather than
passing for free.
Tests: registry unit tests including both the false positive and the miss; the
stale-probe orderings for both NodeViews; the uuid away-and-back case; and the
restore probes' `cache: 'no-store'` (the mocks were discarding the options
argument, so the point of the re-probe was untested). Ten mutants total across
both rounds, each failing only its own tests — one (the uuid-swap bump) survived
first time and got the test it was missing.
* fix(web): make the latch fence structural; correct three stale comments (BUG-2509)
Round three of review. The generation fence I added covered the two probes I had
been looking at and missed three others that also latch: both toolbar MIME probes
and the activation probe. Same defect as the one already fixed — a probe issued
inside the archived window answering 404 after the restore healed the node — at
call sites I had not enumerated.
Rather than patch three more sites, the fence moved INTO `latchMissing`, whose
signature now REQUIRES the captured generation. Every path into it is an async
probe answering a question it asked earlier, and a latch is destructive and
permanent, so "the caller will remember" was the wrong shape: an unfenced call
site is now a type error rather than a bug found in review. Fixing this class one
site at a time is what produced the miss.
Three comments were left saying things that are no longer true, which in this
file is not cosmetic — the comments are how the next reader learns the rules:
- the PLAN-2411 seam note still said the restore channel was "stated, not built
here" and that only that future channel could clear the latch. This branch is
that channel. Rewritten to state what actually holds now (the signal never
clears anything; only a server `ok` does).
- my own generation docstring claimed EVERY mutating continuation captures it,
which the transforms and the activation-open path do not. Restated to say
what it governs (the latch and the heal) and what it deliberately does not
(opening a viewer, which `activationSeq` owns; rotate/crop, which mint a new
attachment).
- the metadata cache's "no staleness concern" predates the distinction this bug
turns on. Split: a settled `ok` cannot go stale, a settled `missing` can,
because it describes reachability rather than contents.
Two review findings are DECLINED, recorded in the code where the next reader will
ask. Re-checking `address().itemId` after the await: itemId is routing, not
ownership, and the probe's answer is about the ATTACHMENT — fencing on it would
imply a relationship the code does not have. Eviction for the archived-item
registry: it holds one uuid per archived item viewed, and dropping a mark early
reintroduces this bug silently for long sessions only, which is the worse trade.
The new toolbar-probe test passed vacuously at first (`selectAll` does not build
the toolbar, so the probe under test never ran and the stale-release handle was a
no-op default). It now drives a real NodeSelection and asserts the probe fired.
Twelve mutants across the three rounds; two survived first time and each got the
test it was missing.
|
||
|
xarmian
|
ec7fd027fc |
feat(server,cli): watches, user-scoped event stream, plugin monitor command — PLAN-2469 Phase 1 (TASK-2533) (#1082)
* feat(store): race-free status/assignment mutation signal (TASK-2533)
Adds models.Item.LastMutation (ItemMutationSignal), populated inside the
SAME transaction that already writes status_transitions / assigned_user_id
in UpdateItemWithParentLink and MoveItemWithPreCheck. This is the
foundation for TASK-2533's watch-notification pipeline: a before/after
snapshot taken in the HTTP handler layer would race concurrent writers of
the same item, so the signal is computed where the authoritative diff
already happens, in-transaction.
* feat(store): watches table migration, both drivers (TASK-2533)
watches(id, workspace_id, user_id, item_id, predicate, created_at) per
DOC-2479's subscription-table design: durable, server-side subscriptions
that survive both the plugin-monitor process and a padd restart.
uq_watches_user_item makes `pad watch <ref>` idempotent (re-watching
upserts the predicate). Wires watches into the workspace-purge child-delete
list, mirroring item_stars.
* feat(watchevents): add in-process notification bus (TASK-2533)
New package: a global (not per-workspace) in-process pub/sub bus carrying
watch-worthy Notifications (status-change / assignment / comment; ask
reserved in the enum with no producer yet — see the follow-up server
commit). Bus is an interface specifically so a Redis-backed implementation
can slot in later without touching any caller; only MemoryBus exists today.
Package doc comment states the single-process/multi-instance limitation
explicitly, mirroring internal/events' shape.
* feat(store): watches CRUD (TASK-2533)
models.Watch + Store.CreateWatch (upsert on user+item)/GetWatchByUserItem/
ListWatchesForUser (unscoped by workspace — a watch is personal, and the
event-stream handler needs every watch a caller holds across all their
workspaces)/DeleteWatch.
* feat(server): watch/nudge event stream + CRUD endpoints (TASK-2533)
GET /api/v1/events/stream (DOC-2479): a user-scoped, cross-workspace SSE
stream, filtered server-side to the caller's watches (with optional
--until field=value predicate) plus "addressed to you" — narrowed to
assignment-to-you only for Phase 1, confirmed with the dispatcher: this
codebase has neither a Collection.Kind field nor any user->active-role
binding to ground DOC-2479's "human-gate-shaped collection targets your
active role" half mechanically. watchevents.KindAsk stays in the wire
enum with no producer. `pad session register` is the natural future hook
for a session-carried role identity.
POST/DELETE .../items/{slug}/watch, GET /api/v1/watches (unscoped,
mirrors /auth/tokens' shape for a personal, not workspace, resource).
Producer wiring (TASK-2533 audit) publishes from every live mutation path
that can produce a LastMutation signal or a new comment: handleUpdateItem
(incl. its collab sub-paths and the comment-attached-to-update path, which
bypasses handleCreateComment entirely), handleMoveItem, handleCreateComment,
item creation with an initial assignee, and the bulk-items loop (covers
archive/restore/move/set-priority/tag/untag/assign uniformly via one call
site). Named, not silent, bypasses: import bundle, status_transitions
backfill, workspace restore/purge — none are live human-facing mutations.
Known Phase-1 tradeoff, flagged not fixed: bulk mutations are NOT batched
into one notification the way the existing SSE/webhook bulk path is — a
bulk-assign of N items surfaces N individual notifications. Each is still
correctly scoped by the recipient's own watches/addressed-to-you filter
(a narrower audience than the workspace-wide SSE firehose the existing
batching protects), so this is a noise-discipline tradeoff, not a leak.
* feat(cli): pad watch + pad session register (TASK-2533)
pad watch <ref> [--until field=value] creates/upserts a durable watch;
pad watch list / pad watch remove <ref> are the hygiene companions the
dispatcher asked to be included explicitly rather than silently added.
pad watch --stream --for-session is the plugin-monitor command: one
stdout line per matching event ("PAD TASK-214 -> kind (actor): summary"),
silent on startup with no .pad.toml (hourly retry) or an unreachable padd
(backoff retry) per DOC-2479's noise-discipline contract. The retry/
backoff math and line formatting are pure, unit-tested functions; the
actual sleep loop is not (per the dispatcher's ask).
pad session register writes ~/.pad/sessions/<pid>.json (pid, cwd,
CLAUDE_CODE_MESSAGING_SOCKET when set) -- forward-looking infra for
Phase 3's live-sessions/presence surface; nothing consumes it yet in
Phase 1/2.
* fix(server): comment replies never published a watch notification (TASK-2533)
Codex round 1 finding 2 (verified real, not a false positive):
handleCreateReply is a SEPARATE code path from handleCreateComment — it
calls store.CreateComment directly via POST .../comments/{id}/replies,
not POST .../comments — and was missing the watch-notification hook
entirely. A reply to a comment on a watched item produced zero
notification. Same kind=comment publish as the top-level path, plus a
regression test covering the reply route specifically.
* fix(server): re-check current access before serving/delivering watches (TASK-2533)
Codex round 1 finding 1: ListWatchesForUser filtered only by user_id — a
watch row survives a revoked workspace membership or grant (nothing
deletes it), so GET /api/v1/watches and the event-stream's notification
filter could keep leaking item title/ref, workspace slug, actor, and
summary for access the caller no longer has.
Adds Store.ListWatchesForUser's ItemCollectionID column (needed for the
visibility check) and server.filterWatchesByCurrentAccess, which mirrors
computeSSEVisibility's RBAC resolution (handlers_events.go) — admin
bypass, VisibleCollectionIDs for member/guest full-collection access,
GuestVisibleResources for item-level grants — grouped by workspace since
a caller's watches can span many, unlike a single SSE connection scoped
to one. Fails closed on any lookup error.
Wired into handleListWatches here; the event-stream's loadWatchPredicates
call site picks up the same filter in the next commit, which also
restructures that function's Subscribe/replay sequence and therefore
touches the same lines.
* fix(watchevents): atomic ID assignment + subscribe-and-replay (TASK-2533)
Codex round 1, findings 3 and 4 (same subsystem, fixed together):
Finding 4 — sequence assignment and replay-buffer insertion happened
under SEPARATE locks in MemoryBus.Publish. Two concurrent Publish calls
could append to the ring buffer out of ID order, corrupting since()'s
ordering assumptions (it walks the ring oldest→newest assuming monotonic
IDs). Fixed by unifying seq assignment, buffer append, and the
subscriber-list snapshot under one lock; the (already non-blocking)
fan-out send still happens after releasing it.
Finding 3 — GET /api/v1/events/stream called Subscribe() and, later
(when resuming via Last-Event-ID), EventsSince() as two separate calls.
A Notification published in the window between them landed in BOTH the
replay result and the live channel, double-delivering it. Bus gains
SubscribeAndReplaySince(sinceID), which atomically subscribes and reads
the replay buffer under the SAME lock; the stream handler now uses it
whenever a Last-Event-ID is present (this commit carries that call-site
change, plus the finding-1 loadWatchPredicates filter wiring from the
previous commit — both land in the same lines of this function).
Adds a concurrent-publish ID-ordering test and a subscribe-then-
concurrent-publish no-duplicate test, both run with -race.
* fix(cli): monitor silent-start ordering + sync_required handling (TASK-2533)
Codex round 1, findings 5 and 6:
Finding 5 (P1) — runWatchMonitor called getClient() once, before the
loop and before the .pad.toml check. getClient() -> getConfiguredConfig()
os.Exit(1)s when unconfigured with no TTY, or launches an INTERACTIVE
configuration wizard when one is attached — either way a direct violation
of DOC-2479's silent-start contract, which requires "not ready yet" to be
a silent retry, never a crash or a prompt. Adds monitorClient(), which
builds the client the same way but returns a plain error instead of
exiting or prompting; client construction now happens INSIDE the loop,
after the .pad.toml gate, on every iteration, and its failure folds into
the existing padd-unreachable backoff path.
Finding 6 (P2) — streamWatchEvents ignored "sync_required" (the server's
signal that the requested Last-Event-ID was evicted from its replay
buffer), so a stale cursor got resent on every reconnect forever. Now
clears the cursor on sync_required so the next reconnect is a fresh,
non-resuming subscription instead.
Both covered by tests that assert the goroutine returns promptly on
context cancellation (proving no os.Exit / no blocking prompt was hit,
since the test process itself is still running to observe the return)
and that streamWatchEvents clears/re-tracks the cursor correctly around
sync_required.
* fix(server): uniform current-access gate for watch AND addressed-to-you delivery (TASK-2533)
Codex round 2, findings 1 and 2 — same subsystem (watch/nudge delivery
access control), fixed together; finding 2 explicitly falsifies finding
1's fix's own admin-bypass argument, so this replaces that reasoning
rather than patching around it.
Finding 1 (confirmed real): VisibleCollectionIDs / GuestVisibleCollectionIDs
deliberately over-widen for navigation — a collection ID is included if the
caller has an item grant on ANY item inside it, explicitly leaving
item-level narrowing to the caller (their own doc comments say so).
computeWatchAccessVisibility used that over-wide set directly as the
"fully visible" gate, so a guest granted item A was treated as having full
access to A's WHOLE collection, including an ungranted sibling item B.
Fixed by building the "genuinely full access" set from
GuestVisibleResources' fullCollectionIDs (populated only from direct
collection_grants, never widened by an item grant) + GetMemberCollectionAccess
/ ListSystemCollectionIDs for an actual member — exactly computeSSEVisibility's
own fullCollSet construction, not an approximation of it.
Finding 2 (confirmed real): the addressed-to-you (KindAssignment) branch in
watchNotificationVisible returned true unconditionally, with NO access
check. validateAssignmentScope (internal/store/items.go) only checks
WORKSPACE membership, never collection access, so an item can be assigned
to a "specific"-access member whose granted collections don't include it
at all — an ordinary assignment, no revocation timing required. Fixed by
gating EVERY notification kind — watch-matched and addressed-to-you alike —
through the SAME watchAccessVisibility check before either branch runs.
watchevents.Notification gains CollectionID so the check has what it needs
without a second lookup; the stream handler resolves it lazily per
workspace via a small connection-scoped cache (workspaces aren't known in
advance for addressed-to-you the way watch workspaces are), cleared on the
same reval tick that reloads the watches map.
This also required replacing computeWatchAccessVisibility's admin-bypass
argument, not just its code: "every call site filters the caller's OWN
watches" stopped being a sufficient justification once addressed-to-you
(which is fundamentally about *this* caller's own assignment activity
across every workspace) shares the same gate — a bearer-borne admin token
unconditionally trusted for that is exactly BUG-1616's blast radius. Now
mirrors computeSSEVisibility's cookie-vs-bearer distinction exactly.
Tests: guest-with-item-grant no longer sees a sibling item's watch or
stream notification (filter-level and HTTP/SSE-level); an assignment
outside a restricted member's granted collections is denied at both
levels; addressed-to-you is proven still gated (denied with no access,
visible once granted) as a pure unit test.
* fix(store): always re-read existing under lock, not just for precheck/patch updates (TASK-2533)
Codex round 2 finding 4, verified real: updateItemWithParentLinkOnce's
`existing` snapshot was only refreshed under the write lock when precheck
!= nil, ExpectedUpdatedAt != "", or FieldsPatch != nil — any update
touching none of those (e.g. a plain title-only PATCH) kept the STALE
pre-tx `existing` for the rest of the function, including the
LastMutation assignment-delta comparison added in TASK-2533's first
round. A concurrent OTHER transaction's assignment change landing between
this transaction's pre-tx read and its lock acquisition would get
misattributed to THIS transaction: a title-only update could report a
spurious, wrongly-attributed AssignmentChanged for a transition it never
made, duplicating the one the other transaction already reported
correctly (or missing a real one, depending on interleaving).
The status-transition capture already defended against exactly this with
its own separate conditional re-read; the assignment-delta capture added
later did not replicate that guard. Fixed by making the re-read
unconditional — once, right after the locks are held, before any SET-
clause building or the UPDATE itself — so every existing.* comparison in
this function is race-free by construction, not by each caller
remembering to guard itself. Also removes the now-redundant duplicate
re-read the status code had of its own.
Reproduces the exact race deterministically using UpdateItemWithPreCheck's
precheck hook as a synchronization point (TX2's assignment change blocks
mid-transaction while TX1's title-only update races its own pre-tx read
against it) — the new test fails reliably against the pre-fix code and
passes reliably (including under -race, and in Postgres mode) against
the fix.
* fix(watchevents): send under the same lock Unsubscribe/Close use (TASK-2533)
Codex round 2 finding 3, confirmed real and high-severity: Publish
snapshotted subscriber channels under the lock, released it, and only
then sent to them. A concurrent Unsubscribe or Close could close one of
those channels in the window between the snapshot and the send — a send
on a closed channel PANICS in Go, which crashes the whole padd process,
not just one subscriber's connection. The reasoning for releasing the
lock before sending ("a slow subscriber would stall everyone else") didn't
hold up: the send is already non-blocking (select/default — a full
channel is dropped-and-logged, never awaited), so holding the lock
through it costs nothing and closes the window structurally.
Adds a hammer test (many iterations of concurrent Publish / Subscribe /
Unsubscribe / Close, short-lived churned channels, recover()-wrapped so a
regression fails cleanly instead of crashing the whole `go test` run) that
reproduces "send on closed channel" dozens of times per run against the
pre-fix code (plus an independent -race detection) and passes cleanly,
repeatedly, against the fix.
* fix(server): re-fetch the user, not just the vis map, on each reval tick (TASK-2533)
Codex round 3, confirmed real: watchVisCache captured *models.User ONCE
at connect time (newWatchVisCache) and never re-fetched it; reset()
cleared only the per-workspace visibility map. computeSSEVisibility's own
doc comment explains why it re-fetches the user fresh on every call —
"so mid-stream role changes (admin demotion, user.disabled flips) take
effect on the next tick" — and the round-2 commit claimed to mirror that
"exactly," but only carried over the collection/bearer logic, not the
re-fetch itself. Net effect: a demoted or disabled admin kept fullAccess
on an open stream (both watch-matched and addressed-to-you delivery,
since both go through this same cache) until reconnect.
Adds watchVisCache.refreshUser, called by both the constructor and
reset() so the cadence matches computeSSEVisibility's actual cadence in
handlers_events.go (that function is invoked once at connect and again
only on each membershipCheck tick — never per event — so "per cache
reset" here is the same cadence, not a narrower one). Deliberately fails
CLOSED (not open-to-stale like computeSSEVisibility's own transient-error
fallback) on a fetch error, a deleted user, or a disabled user — a nudge
stream's wrong failure mode is delivering a fact to someone who
shouldn't see it, not a dropped UI update, so this trades
computeSSEVisibility's availability-leaning fallback for a stricter one
and says so in the comment rather than repeating the "mirrors exactly"
claim the fix falsified.
Tests: a unit-level pair (mirroring handlers_events_revalidation_test.go's
existing admin-demotion/disable coverage of the analogous SSE gap
exactly) proves an admin loses fullAccess after a demotion + reset(),
and a disabled user is denied outright; an HTTP/SSE-level test proves a
live stream stops delivering entirely once its connected user is
disabled and a reval tick passes. All three reproduce the bug reliably
against the pre-fix code and pass cleanly against the fix.
The HTTP-level test deliberately runs serially (not t.Parallel()): it
mutates the package-level watchListRevalInterval var, which every other
parallel watch-stream test in this package also reads via its own
ticker — writing to it from a t.Parallel() test raced against those
reads under -race (misattributed by the race detector to a whole
cluster of unrelated concurrently-running tests before this was
diagnosed). Full server package -race pass is clean after the fix.
* fix(server): decouple vis-cache reset from watch-list reload success (TASK-2533)
Codex round 4, confirmed real: on a reval tick, if ListWatchesForUser
errored, the handler's `continue` skipped visCache.reset() entirely —
the two were coupled, with reset() only reachable on the reload's
success path. A demoted or disabled user's stale identity/visibility
(round 3's fix) stayed live for exactly as long as that UNRELATED query
kept failing, so the round-3 leak reopens for the duration of any
watch-list reload error.
Fixed by running visCache.reset() first, unconditionally, before
attempting the watch-list reload. On a reload failure, the stale watch
list is kept (its own staleness is already bounded by
watchListRevalInterval's "eventually consistent" contract) but is now
gated by the FRESH visCache regardless — a demoted/disabled user is
denied via visCache even while the watch list itself lags a tick.
Chose this over dropping all delivery for the tick (the other option the
finding offered) because tying stream availability to an unrelated
query's transient health seemed like the wrong tradeoff; the comment at
the call site states this choice explicitly.
Adds a watchPredicatesLoadFault test seam on *Server (mirrors the
existing restoreAckFault pattern) so the reload failure can be forced
deterministically without breaking the DB connection for the whole test.
Reproduces the exact bug: forces the reload to fail on every tick while
concurrently disabling the connected user, and asserts addressed-to-you
delivery (which depends only on visCache, never the watch list) is
denied anyway. Fails reliably against a reverted (pre-fix, coupled)
version of the reval branch and passes cleanly against the fix.
Full server package -race pass, full suite (SQLite + Postgres) pass,
lint clean — this is the pre-PR verification matrix; round 5 will be a
narrow re-verify of this fix only.
* fix(server): bound stale watch set under persistent reload failure; atomic test seam (TASK-2533)
Codex round 5, two P2s, both confirmed real:
Finding 1 — `watches = fresh` only ran on the reload's success path, so
under a PERSISTENT (not single-tick) reload failure the watch set stayed
live indefinitely: a dead watch (removed, item deleted) kept matching
forever, and a watch created during the outage was silently missed
forever — visCache (round 4) gates current ACCESS, not whether a watch
still legitimately exists, so it couldn't catch this on its own. Fixed
by tracking consecutive reload failures and clearing the watch set once
maxConsecutiveWatchReloadFailures (3 ticks) is crossed, failing closed
on watch-matched delivery specifically while addressed-to-you delivery
(visCache-only, unaffected either way) continues throughout. Updated the
tradeoff comment at the call site so the "eventually-consistent" claim
now matches the bounded, not unbounded, behavior it actually describes.
Finding 2 — the watchPredicatesLoadFault test seam was a plain `func()
error` field, written by a test AFTER the SSE stream's background
goroutine was already running and reading it on every reval tick:
genuinely racy, unlike restoreAckFault's own use of the identical field
shape, which is set once, synchronously, before the single HTTP request
that reads it — goroutine creation's happens-before edge makes THAT
usage safe without any extra synchronization. Verified restoreAckFault
does not share the flaw and left it untouched. Fixed the watch seam with
atomic.Pointer[func() error] instead.
Test for finding 1: forces maxConsecutiveWatchReloadFailures+1
consecutive reload failures via the (now-atomic) fault seam and asserts
watch-matched delivery is suppressed once the bound is crossed while
addressed-to-you keeps delivering, then clears the fault and confirms
watch-matched delivery resumes on the next successful reload — a bounded
outage response, not a one-way ratchet. Fails reliably against the
bound disabled, passes cleanly restored.
This is the (re-run) pre-PR verification matrix per the dispatcher:
SQLite + Postgres + full-suite -race + lint + gofmt, all clean. Round 6
is a narrow re-verify of these two fixes only.
* test(store): bound the concurrent mutation-signal test's wait (TASK-2533)
CI-triage follow-up: PR #1082's plain Postgres step hit go test's default
10-minute per-binary timeout. Investigated whether any store test added by
this branch scales with runner slowness (lock-wait defaults, sleep-based
polling, transaction-hold durations):
- Watches CRUD tests (8): 0.63-0.80s each under Postgres, isolated and in
the full 741-test package run.
- Mutation-signal tests (6), including the precheck-hook two-transaction
race test: 0.48-0.80s each; the race test held at 0.48-0.51s across 10
consecutive runs (no variance) and across the full-package run.
- Full store package under Postgres: 279.17s and 277.12s across two runs
on this branch, matching the ~275s/297s baseline team-lead measured
locally and on PR #1081 — no reproducible slowdown from anything this
branch adds.
No pathological test found locally. The one test with genuine
cross-goroutine DB lock contention (TestLastMutation_AssignmentDelta_
NotMisattributedUnderConcurrentWrite) had an unbounded wg.Wait() as its
only unbounded wait — TX2's release was already unconditional (fixed 50ms
sleep, not gated on TX1's progress), so there's no deadlock risk, but
there was no ceiling on how long legitimate lock contention could
stretch it under a slow/shared runner. Replaced with a bounded 10s wait
that fails fast with a diagnostic instead of silently consuming
test-binary budget if it's ever exceeded. Verified the regression test
still fails reliably (5/5) against a revert of the round-2 fix it guards.
Could not reproduce the CI timeout locally; likely the pre-existing
~297s CI baseline (already noted as close to the 10-minute ceiling)
plus environmental variance on the shared runner, not a specific test
this branch adds.
|
||
|
xarmian
|
7332a7f9f8 |
feat(collections): add spec workspace template — spec-driven development (IDEA-2527) (#1081)
* refactor(collections): extract tasksCollection/ideasCollection helpers
Pulls Tasks and Ideas out of Defaults() into standalone functions,
mirroring the existing docsCollection extraction. Seeded schema is
byte-identical; this just lets a template compose Tasks/Ideas without
also getting Plans, which the upcoming spec template (IDEA-2527) needs.
* docs(collections): generalize decompose playbook to plan-or-spec
The shared `decompose` library playbook was worded plan-only
(target description, pre-flight checks, body-analysis step). Broadens
the wording to also recognize a spec's `## Implementation plan` /
`## Acceptance criteria` sections as decomposition sources, ahead of
the spec template (IDEA-2527) reusing this playbook. Title is
unchanged (looked up by exact string elsewhere); wording is additive
so startup/scrum/product, which have no Specs collection, are
unaffected.
* feat(collections): add spec workspace template (IDEA-2527)
New "spec" template positions Pad as a spec-driven-development
platform: Specs (SPEC, draft→in-review→approved→implemented→
superseded, version+area fields, content_template skeleton) replaces
Plans as the parenting artifact — idea/bug → spec → tasks → PRs.
Deliberately no Plans collection; implementation-plan material lives
in the spec body's optional "## Implementation plan" section instead
(cheaper than maintaining two overlapping artifacts).
Ships:
- SpecConventionTriggers/SpecPlaybookTriggers, extending the
software trigger vocab with on-spec-draft/approve/change
- Four seed conventions gating implementation, PR review, and
spec-edit discipline on the spec lifecycle
- Three full-prose playbooks: `/pad spec` (draft-first interview
with IDEA/BUG graduation), `/pad verify` (walk acceptance
criteria against the diff/behavior), `/pad extract-specs`
(brownfield extraction with a subsystem-map human checkpoint and
provenance-marked observed-behavior specs)
- Reuses `decompose` (generalized in the previous commit) and
`ship` (unchanged) as the remaining two seed playbooks
Registered in templates.go; adds the three new playbook bodies to
TestInvocationFramingStaysNLCanonical's scanned surfaces. Dedicated
tests in templates_sdd_test.go cover registration, Specs schema
shape, extended (not replaced) trigger vocab, the four conventions,
and the five playbooks.
Positioning/marketing page descoped from this PR — recon found no
marketing-page infrastructure in this repo to extend (the root route
redirects straight to /console); tracked separately.
* fix(collections): gate spec graduation, fix strict-parser arg contract
Codex round 1, findings 1-2:
- `/pad spec` accepted ANY ref matching the generic ref pattern
(e.g. TASK-7) and would enter graduation mode, terminalizing an
unrelated item's status. Dispatch now resolves the ref, checks its
collection is actually Ideas-like or Bugs-like before graduating,
and otherwise falls back to using it as recon context for a
new-topic draft with no status flip.
- `target` was documented/declared optional, but the strict CLI/MCP
parser only fills REQUIRED args positionally (internal/server/
handlers_playbooks.go) — `pad playbook run spec "<topic>"` silently
failed to bind it. Made `target` required, matching the `plan`
playbook's `topic` precedent. `extract-specs`'s `target` stays
optional by design (bare invocation is a supported flow); its
Arguments docs now show the strict-path key=value form instead of
implying positional works.
Adds a regression test (TestSpecPlaybookTargetArgumentRequirement)
pinning target's required-ness for both playbooks.
* docs: sync decompose's structured arg metadata + SKILL.md to plan-or-spec
Codex round 1, finding 3: the decompose playbook body was generalized
to plan-or-spec in an earlier commit, but its structured
`arguments` JSON metadata (the queryable contract) and
skills/pad/SKILL.md's Decomposition entry were left plan-only —
exactly the drift the body's own comment says these two surfaces
must not have.
* fix(collections): treat unedited Implementation-plan placeholder as absent
Codex round 1, finding 4: the spec content_template always ships a
populated "## Implementation plan" section, which meant decompose's
"no implementation plan -> fall back to acceptance criteria" path
never triggered for skeleton-created specs — it would treat the
placeholder's angle-bracket instruction text as a real task
candidate. The skeleton placeholder now tells the author to delete
the section if unused, and decompose's source-analysis step treats
an unedited placeholder the same as a missing section.
* docs(collections): drop phantom TASK-2528 reference
Codex round 1, finding 5: TASK-2528 doesn't exist in the docapp
workspace. The tasksCollection/ideasCollection extraction comments
now cite IDEA-2527 only.
* docs: add spec-target routing example to SKILL.md Planning section
Codex round 2, finding 1: the Planning section's decomposition routing
example only showed a plan target ("break plan 2 into tasks" → PLAN-2).
Adds a spec-target example alongside it, consistent with the
Decomposition entry further down (already generalized to plan-or-spec)
and the decompose playbook body/arguments.
* fix(collections): generalize ship playbook target to plan-or-spec
Codex round 3, P1: the spec template seeds ShipPlaybook() unchanged,
but its target contract documented only PLAN-ref | TASK-ref. Decompose's
step-7 report tells the user to run `/pad ship <target-ref>` on the
source ref, which in a spec workspace is SPEC-N — so the seeded
idea->spec->tasks->ship handoff broke at the last step.
Generalizes target's wording across all three surfaces (the Arguments
line, the argument-parsing PLAN-ref bullet, and the arguments-JSON
description) to PLAN-ref | SPEC-ref | TASK-ref, mirroring the same
additive pattern already used for decompose: a spec is a parenting
artifact with identical expansion mechanics to a plan (same
--parent-child wiring), so the change is inert for startup/scrum/product,
which have no Specs collection. No test pins the exact argument text,
so existing structural tests (TestStartupTemplateShipsShipPlaybook,
TestPlaybookLibrary_ShipBodyShared) pass unchanged.
* fix(collections): generalize ship's remaining plan-only mentions
Codex round 3 follow-up: two plan-only mentions left over from the
target-contract generalization.
- Commit-message template's "Parent: PLAN-XXX." -> "Parent: PLAN-XXX /
SPEC-XXX.", plain aliasing matching everything else already
generalized.
- Step 11's parent-closing guidance keeps the existing plan sentence
as-is and adds the spec case as a judgment-trigger pointer rather
than a parallel unconditional flip: a spec's terminal status is
gated by verification (that's what the `verify` playbook is for),
so ship tells the agent to run `/pad verify SPEC-XXX` instead of
flipping the spec's status directly — it moves the spec to
`implemented` only once the acceptance criteria actually hold.
* fix(collections): ship's PR-body guidance cites specs and their criteria
Codex round 4, P1: ship's PR-context generation still said "parent
plan" and templated the PR body under <PLAN-REF> only — so
`/pad ship SPEC-N` produced a PR that never cited the governing spec,
directly violating the spec template's own seeded on-pr-create
convention ("PRs cite the spec and which criteria they satisfy").
Generalizes the PR-body template to <PARENT-REF> (PLAN-ref or
SPEC-ref) and adds explicit guidance: when the parent is a spec, the
PR must also list which acceptance criteria it satisfies (e.g.
"Implements TASK-12 under SPEC-4, satisfies AC-1, AC-2") — this is
what makes /pad verify fast later, since the reviewer walks the cited
criteria instead of re-deriving intent. Also generalized step 1's
"check the parent plan's content" to plan-or-spec, since a spec
parent's acceptance criteria are exactly what step 8 needs to cite.
* fix(collections): verify gates the implemented flip on spec approval
Codex round 4, P1: /pad verify only excluded draft specs from the
flip-to-implemented, so it could promote an in-review or superseded
spec straight to implemented on the strength of passing acceptance
criteria alone — bypassing the workspace's own approval lifecycle.
Verification still runs and reports AC results regardless of status
(useful information either way), but the Resolve step's all-pass path
now branches on status: approved -> offer the flip (unchanged);
already implemented -> report the re-verify confirms it still holds,
nothing to flip; in-review -> report the pass but tell the user
approval isn't done yet, point at finishing review; superseded ->
report the pass but point at whatever spec replaced this one, since
that's the one that should be verified and implemented going forward.
* fix(collections): decompose treats placeholder ACs as absent too
Codex round 4, P2: the AC-fallback path treated unedited skeleton
placeholders (AC-1: <a statement...>) as real task candidates, so a
fresh untouched spec could decompose into bogus tasks. Extends the
same placeholder-as-absent rule already applied to the
Implementation-plan section: an AC-N line still holding the unedited
angle-bracket instruction text isn't a real criterion and doesn't get
a task proposed for it. If every AC-N is still a placeholder, there's
nothing to decompose from either source — decompose stops and tells
the user the spec has no real acceptance criteria yet.
* fix(collections): unify AC placeholder idiom, cover bare-ellipsis form
Codex round 5, P2: the seeded skeleton's AC-2 used a bare-ellipsis
placeholder ("AC-2: ...") while AC-1 used angle brackets and the
decompose placeholder rule only named the angle-bracket form — a
literal-minded agent could propose a bogus task for an untouched AC-2.
Two one-line fixes: the skeleton's AC-2 now uses the same
angle-bracket idiom as AC-1 ("AC-2: <the next verifiable criterion>"),
so the seeded skeleton has one placeholder style; decompose's
placeholder rule now also names bare ellipsis ("AC-N: ...") as a
placeholder form, as belt-and-suspenders for user-typed shorthand
beyond just the seeded skeleton.
No test pinned the AC-2 text, so no test changes needed.
* fix(collections): spec's circulate-for-review branch actually sets in-review
Codex round 6, P2: the "circulate for review" branch said to leave the
spec at in-review and stop, but the create command always uses
--status draft and no update followed it on that path — so the spec
silently stayed draft forever. Since round 4's fix gates verify's
implemented-flip on approval status, an item stuck at draft (never
even reaching in-review) is a stuck workflow, not just a label
mismatch.
Adds the explicit `pad item update <new-spec-ref> --status in-review
--comment ...` step to the circulate branch, parallel to the
approved-outright branch's existing update command, keeping the
audit-comment habit consistent with the rest of the body.
* fix(collections): add resume mode so circulate-for-review specs converge
Codex round 7, P1: the circulate branch stopped the playbook before
the graduation step, and nothing ever completed it — a later
`pad item update SPEC-N --status approved` was a bare status flip with
no agent step attached, so the source IDEA/BUG never got terminalized.
Worse, the advertised rerun path was broken: `/pad spec SPEC-N`
dispatched as non-graduation (a spec isn't Ideas/Bugs-like per the
round-1 gate) and would have created a SECOND spec instead of
resuming the first.
Three coordinated edits:
- New dispatch mode: a target resolving to the specs collection
itself enters resume mode, never creates anything. Branches on the
spec's status — in-review is the normal resume case (confirm
approval, complete any pending graduation, offer decompose);
draft/approved/implemented/superseded get the sensible remainder
(offer the original choice again, report already-resolved state, or
point at the successor spec).
- The circulate branch now records a "Graduation pending approval:
<source-ref>" comment on the new spec when in graduation mode, so
resume mode has something mechanical to find rather than relying on
re-deriving intent from the Context section.
- The circulate branch's stop text now tells the user how the loop
closes: rerun `/pad spec SPEC-N` when review is done.
Updated the `target` argument docs (body + arguments JSON) to name the
third accepted form. templates_sdd_test.go doesn't assert argument
description text, so no test changes needed.
* fix(collections): restructure graduation as one idempotent reconcile rule
Codex round 8, two P1s: the approved/implemented branch of Resume
never checked the pending-graduation marker (so a plain manual
`--status approved` never graduated the source), and the circulate
branch's marker write sat after "stop here" — a skippable step three
rounds of findings kept landing on. Scattering graduation state and
handling across branches was the actual bug; restructuring so the
class can't recur, not another branch-local patch.
- The graduation-link comment now gets written unconditionally at
spec-creation time (step 5, graduation mode), before any
approve/circulate branching — no ordering problem, no skippable
step, exists on every path including a crash before either branch
completes.
- One Reconcile rule, stated once in the Resume section: on every
resume that reaches it (draft/superseded stop earlier and skip it;
in-review-not-yet-approved also skips it), if the spec is now
approved or implemented and its comments name a graduation source
that's still open, complete the graduation — idempotent, so it's
safe to run on every resume regardless of how approval happened
(through this playbook, a crash-recovery rerun, or a bare manual
status flip outside the playbook entirely).
- Step 6 (approve-outright path) is now just an invocation of
Reconcile rather than parallel instructions — graduation mechanics
described exactly once, referenced from both call sites.
Dispatch and Arguments text re-read coherent after the restructure;
no changes needed there beyond what round 7 already added.
* fix(collections): graduation idempotent by source, custom collection, promise wording
Codex round 9, five findings — the last substantive round before
remaining crash-window-shaped gaps become documented limitations
rather than more branches:
1. (High) Rerunning /pad spec IDEA-x mid-flight created a second
spec — graduation wasn't idempotent by SOURCE, only by spec ref.
Pre-flight step 2 (graduation mode) now checks, before drafting,
whether the source's trail already shows a "Graduating into
<spec-ref>" comment, or whether a search of the specs collection
finds a spec whose Context names this source. Either match means
this run is really a resume — switch to resume mode on the found
spec instead of creating.
2. (High) A crash between create and the marker comment left an
unlinked spec. Step 5 now writes markers on BOTH sides (source and
new spec) immediately after create, back-to-back, shrinking the
window. Documents the actual recovery mechanism instead of
pretending atomicity: the skeleton's Context section always names
the source, so finding 1's recon check catches even a marker-less
spec on the next run.
3. (Medium) The resume-mode dispatch check tested only the literal
"specs" collection, breaking for a custom `collection` argument.
Now tests against the resolved `collection` argument (checking
`pad collection list` if renamed), consistent with the existing
ideas/bugs check.
4. (Medium) The opening promise ("nothing gets created until the
user approves") contradicted the circulate path, which creates an
in-review item. Reworded to match actual behavior: nothing is
created until the user chooses approve-or-circulate; the draft is
always presented in chat first.
5. (Note) The superseded branch skipped Reconcile unconditionally,
stranding any pending graduation. Now checks the marker before
stopping: if the source is still open, the pending graduation
transfers to the successor spec (if findable) via the same marker
comment, so the successor's own future Reconcile picks it up.
* fix(collections): make graduation's recovery claims actually true
Codex round 10, four sentence-scale edits closing the gap between
what the prose claimed and what it actually did:
1. (High) Pre-flight step 2's "recon check is the actual recovery"
claim was false for a marker-less spec after a crash: the search
found the spec, but Reconcile still needed marker comments that
were never written, so it would no-op and strand the source. Now
the discovery path repairs — writes both sides' markers right then
if missing — before proceeding to resume, so the recovery claim
holds by construction instead of by accident.
2. (High) Transferring a pending graduation to an already-approved-or-
implemented successor (superseded branch) wrote the marker but
never re-triggered anything to act on it. Now runs Reconcile on the
successor immediately in that case (idempotent, source ref already
in hand) instead of waiting on a rerun that might never come.
3. (Medium) "Skip straight to Resume below" bypassed Resume's own
pre-flight (loading the spec's comments), which Reconcile depends
on. Now explicit: run Resume's pre-flight first.
4. (Medium/borderline) "Never creates a second spec" overstated the
guarantee for a SPEC-ref passed with a mismatched --collection.
Softened to "never creates a duplicate within the resolved specs
collection" everywhere the claim appears (Arguments prose, Dispatch,
pre-flight, and the arguments JSON description) — consistent
scoped truth in every location rather than a strong claim in one
place and a weaker one elsewhere.
* fix(collections): enforce the Context-citation premise, walk the chain
Codex round 11, two findings:
1. (High) The whole crash-recovery mechanism (pre-flight step 2's
search, step 5's recovery claim) depends on a graduated spec's
Context section naming its source — but nothing enforced that; the
skeleton's own Context hint says "(if any)" since most specs
aren't graduated, and step 3 never mandated the citation for the
ones that are. Added the explicit rule to step 3: in graduation
mode, Context MUST cite the source ref by ID (e.g. "Grew from
IDEA-12"), stated with the reason — it's the search key crash
recovery depends on. Reinforced in step 5's and pre-flight step
2's claim text: attributed the guarantee to the playbook's own
mandate, not to "the skeleton," which doesn't itself enforce
anything.
2. (Medium) Transferring a pending graduation to a successor that is
itself superseded parked the marker somewhere no rerun would ever
look — chains of supersession weren't walked. The superseded
branch now walks to the LIVE HEAD of the chain (bounded, ~10 hops)
before transferring or reconciling; a loop or dead-end mid-chain is
treated the same as no successor found, rather than guessed at or
walked forever.
* fix(collections): don't silently pick a branch in the supersession chain
Codex round 12, the last: the chain-walk from round 11 silently picked
one live head when supersession branches (more than one spec claims to
supersede the same spec). One clause, grouped with the existing
loop/dead-end stop rule: if more than one spec claims to supersede the
same spec at any point in the walk, stop and ask the user which is
canonical before transferring — don't pick silently.
|
||
|
xarmian
|
a66c9ab402 |
chore(nix): bump version to 0.12.0
Pre-tag version sync required by PLAYB-1160 step 1 — the flake builds from source at whatever ref the user names, so the tag snapshot must self-report the version being tagged. Without this, binaries from 'nix run github:PerpetualSoftware/pad/release' report 0.11.0 forever. Refs TASK-2445.v0.12.0 v0.12.0-rc.1 |
||
|
xarmian
|
dd95d755d2 |
Merge pull request #1079 from PerpetualSoftware/fix/bug-2334-sse-toast-deflake
fix(e2e): silence cross-actor SSE creation toasts suite-wide (BUG-2334) |
||
|
xarmian
|
7b46894413 |
fix(e2e): silence cross-actor SSE creation toasts suite-wide (BUG-2334)
The e2e suite shares one pad instance and one workspace, so items seeded by OTHER concurrently-running specs arrive over SSE and stack "X created: ..." info toasts bottom-right — directly over bottom-right UI (the graph drawer's detail card), turning unrelated specs' clicks into a race. pane-content-link-anchors:238 paid a ~40-minute rerun tail at nearly every merge gate. The fix is a narrowly-scoped test-surface kill switch, not a retry: - `quietExternalToasts()` (toast store): reads a localStorage flag no production code ever sets; never throws whatever storage does. - The ONE call site announcing another actor's SSE work — the external `item_created` toast in the workspace layout — checks it. Toasts the page earns with its own actions are untouched, so specs still exercise the real toast surface (copy-dialog's no-force-click policy keeps its protective value). - The shared e2e fixture installs the flag on every context via `quietCrossActorToasts()`; collab-persistence's self-built contexts install it explicitly; account-delete's contexts never enter workspace routes and stay bare. - sse-toast-quiet.spec.ts pins BOTH sides: the quiet leg anchors on the layout branch's own by-uuid GET (pre-attached response log — no arm-order race; SSE-stream response gates the create; bounded settle before the negative assert), and a deliberately unflagged CONTROL context proves the product toast still fires — the real behavior cannot silently regress behind the suite-wide silence. Evidence: three consecutive full local suite runs with ZERO failures (baseline: 1-3 interception/load flakes per run); unit tests pin the helper's contract. Reviewed to fresh-angle CLEAN over four Codex rounds (vacuous-anchor, arm-order, SSE-connectedness, and self-built-context holes all found and fixed by the loop). Claude-Session: https://claude.ai/code/session_01WFBYxdBuSZs2tjipATxAZu |
||
|
xarmian
|
a0ca8fa028 |
Merge pull request #1078 from PerpetualSoftware/feat/attachment-touch-gestures
PLAN-2392 phase 3d: viewer touch gestures + mobile proof |
||
|
xarmian
|
837035e3ce |
fix(viewer): disarm failed promotion, gate degrade, rebase pinch on flip (PLAN-2392 3d fix round)
Three final-pass P2s in the attachment viewer's touch gestures: 1. FAILED-PROMOTION / STALE-OWNER: tryPromoteToPinch's missing-founder (!a || !b) path returned with the pan scalars still set — a stranded phantom pan that ate later gestures. It now fully disarms (release capture, clear scalars, drop the stale entry, disarm the tap, clear swallow) and re-arms the incoming touch as a fresh first touch, superseding the stale owner the way the first-touch reconcile does. The onTouchDown non-touch guard now requires the owner registry entry PRESENT, so a reconciled-out owner routes to that disarm instead of being misclassified as a live non-touch owner and swallowing the press. 2. GATES-ON-DEGRADE: the 2->1 degrade armed a survivor pan without the pointerGatesOpen check every START path carries — a native modal or stacked viewer opening mid-pinch left a pan that resumed when the layer closed. degradeToPan now gates on the leased viewer root and full-clears (before clearing `pinching`, so the held suppressClick drops too) when the gates are shut. 3. FLIP-MID-PINCH: the sheet class flips synchronously with the breakpoint while the ResizeObserver re-clamp is async, so an immediate post-flip move mixed the new stage rect origin with the old midpoint baseline and jumped the offset. onPinchMove now tracks the baseline's rect origin and re-seeds the midpoint (zero delta; scale is rect-independent) the moment the origin shifts, before the async re-clamp catches up. Four discriminating tests (all fail on the reverted code); the 36-test V2 pinch suite + all existing suites stay green unmodified. Claude-Session: https://claude.ai/code/session_01WFBYxdBuSZs2tjipATxAZu |
||
|
xarmian
|
84eef5dd38 |
test(viewer): CDP mobile touch-gesture proof + device checklist (TASK-2519)
PLAN-2392 phase 3d V3 (final task of the plan) — the mobile browser proof for the attachment viewer's touch gestures shipped in V2 (TASK-2518). Real touch is driven through the compositor via CDP Input.dispatchTouchEvent (a CdpTouch helper in the e2e lib), since Playwright's touchscreen is single-tap only. New web/e2e/attachment-viewer-touch.spec.ts (mobile-chromium), 9 legs: - two-finger spread/converge zoom in/out - off-centre affine anchor oracle: a moving-midpoint translate+spread keeps a known image-local point under the midpoint (sub-pixel residual; a zoom-around-centre mutant misses by ~75px, TOL=6px) - double-tap fit<->actual + single-image-tap-inert + backdrop-tap-close - 2->1 lift degrade: jump-free hand-off + survivor pan arms - '+' mid-pinch rebase (no stale-baseline snap; sampled on a tiny post-+ move) - touchCancel all-cancel teardown + next gesture arms fresh - letterbox touch never pans + letterbox tap closes - image/stage touch-action:none, backdrop auto - tap-to-load first-tap priority CDP semantics empirically pinned (not assumed): touchStart/Move carry the full active set; touchEnd names the ending point (pointerup#<id> observed); touchCancel is all-or-nothing. Emulation boundary recorded honestly in DOC-2521 (device-proof checklist): the 2->1 survivor-pan CONTINUATION can't be expressed in CDP — synthetic touch releases the survivor's implicit pointer-capture on the next move, tearing the fresh pan down (a real digitiser keeps it), so the leg proves arm+no-jump and the continuation is device-verified. Also checklisted: gesture feel/arbitration, momentum, real touchCancel, iOS Safari (no WebKit CI project), off-root release. Mutation-verified (build web+go at worktree root, fresh CI server per run): pinch handler disabled -> spread/oracle/rebase red; anchor->stage-centre -> oracle red while spread stays green (discrimination); double-tap disabled -> toggle red; degrade disabled -> 2->1 red; rebase disabled -> rebase red. Codex: 3 rounds, final CLEAN (r1 flagged a stage-settle race -> fixed, and a docs-based touchEnd objection -> refuted empirically; r2 flagged the rebase test wasn't discriminating -> sampled on a tiny move + mutation-proved). Claude-Session: https://claude.ai/code/session_01WFBYxdBuSZs2tjipATxAZu |
||
|
xarmian
|
9d9670e5cc |
feat(viewer): touch pan, pinch, double-tap + touch-action:none (TASK-2518)
3d-V2 of PLAN-2392: the attachment viewer now owns touch. `touch-action: none` on the image and stage lets pointer handlers drive single-touch pan, two-finger pinch, and double-tap-to-toggle; the letterbox stays a native tap-to-close (the backdrop keeps `touch-action: auto`). Gesture state machine (built on V1's pointer registry): - Touch gestures arm only on a PAINTED-IMAGE hit, gated by a per-element paint generation (`paintedGen === loadToken`) so retry-loading is inert while the thumb→original upgrade stays live. The accept-gate snapshots the loader's fence inputs before `decoded()` mutates them. - Pinch composes ONE candidate at the clamped final scale (anchor-zoom around the previous midpoint + midpoint translation), clamped once; PINCH_MIN_DIST=12 with the below-min HELD-scale skip and re-entry rebase. - 1→2 promotion surrenders the pan capture (swallowing its lostpointercapture); 2→1 degrade rebases to the surviving founder; third-and-beyond touches are registry-only; per-pointer pointercancel routes degrade-vs-full-clear. - DOUBLE_TAP_MS=300 / SLOP=24, image-only, with compat-dblclick dedup; a live touch gesture is never seized by a mouse press, and a mouse pan keeps the looser bitmapPresent arm. Owed premise inversions: the sheet e2e now asserts touch-action none; the restore guard + test comments updated (the viewer owns touch via pointer events, but a touchmove is still not defaultPrevented, so the origin check remains the catch). Claude-Session: https://claude.ai/code/session_01WFBYxdBuSZs2tjipATxAZu |
||
|
xarmian
|
cf85e8b8d3 |
feat(viewer): pointer registry + touch tap semantics (TASK-2517)
PLAN-2392 phase 3d V1 — the multi-pointer plumbing V2's pinch needs,
shipped inert. A `pointerId -> {x,y,type}` Map ('registry') tracks every
primary pointer (mouse, pen, touch); the mouse/pen drag keeps byte-
identical semantics as the 1-entry case (ownership still keys off
`gesturePointerId`, capture off `capturedPointerId`).
- Remove the `touch stays native` early-return gate; branch touch at the
top of onPointerDown: touch ENTERS the registry but arms NO drag and
takes NO capture. Taps still work — a touch tap falls through to the
backdrop onclick (close), the chrome exclusion, and the deferred
tap-to-load button (first-tap priority).
- Registry hygiene precedes every guard (round-2 P1): registry.delete
runs FIRST in onPointerUp AND onPointerCancel, before any owner guard,
so browser-claimed touches (which pointercancel routinely under
touch-action:auto) never leak. abortGesture / onLostPointerCapture
delete the pointer too; cancelGesture is restructured to clear the
registry UNCONDITIONALLY (before its no-gesture early return).
- Reconcile a STALE armed owner (an off-root missed pointerup, pre-
capture) out of the registry on the next superseding press, guarded so
a same-id re-press never drops the entry it just set.
- The id-change effect deliberately does NOT touch the registry: nav
doesn't change the physical pointer set.
Test inversion (falsify-don't-contort): the former ':2248' test pinned
"a touch pointerdown is IGNORED". It is replaced by the V1 contract,
split honestly into (a) touch press+move arms nothing / mouse byte-
identical and (b) a real no-move touch tap closes via the backdrop.
Added: registry drains on pointerup AND pointercancel (direct assert via
a test-only __registrySize accessor — the registry is inert in V1, so a
leak has no indirect observable), a pointercancel-storm-during-mouse-drag
leak/ownership test, a chrome-tap-inert test, a stale-owner
reconciliation test, and a touch tap-to-load first-tap-priority test.
NO touch pan, NO touch-action change, NO pinch in this task (V2).
Sheet-swipe-dismiss routed out of V1 as IDEA-2520.
Codex: 2 rounds, final CLEAN (round 1 caught the stale-owner leak + a
conflated inverted test; both fixed).
Claude-Session: https://claude.ai/code/session_01WFBYxdBuSZs2tjipATxAZu
|
||
|
xarmian
|
d01873b508 |
Merge pull request #1077 from PerpetualSoftware/feat/attachment-lifecycle-completeness
PLAN-2392 phase 3c-iii: attachment lifecycle completeness |
||
|
xarmian
|
3d191017ed |
fix(web): fence ordinary attachment loads against in-flight deletes
U2 (TASK-2511) added the ref-counted `inFlightDeletes` marker but applied it only to `revalidateAfterRestore`'s merge. The ordinary list-load reconciliation paths still filtered on `deletedIds` alone, and that set is latched only AFTER a delete's API await (via the deletion bus self- broadcast). In the gap between an optimistic removal and that broadcast a row is gone from `attachments` but not yet tombstoned, so an ordinary list() response issued (or in flight) across that window — a mount/retry load, or the restore path deferring to an in-flight same-view load — could carry the row and repaint the tile the user just removed. Honor `isDeleting(id)` on every ordinary-load reconciliation path, exactly as the restore merge already does: the response row filter, the pending- upload merge, and the load-failure repaint. The settle-time rollback stays a direct write into `attachments` (the marker is cleared in performDelete's `finally`, after the catch re-inserts the row), so a genuinely failed delete still rolls its row back into view. Continuation-count math is unchanged in spirit — `rows`/`missed` simply exclude the same ids the restore path already excludes. Tests (jsdom, each mutation-verified): the P1 flow via a retry load, the pending-upload merge leg, the load-failure repaint, and the rollback- after-failure discipline surviving a list response that landed mid-delete. Claude-Session: https://claude.ai/code/session_01WFBYxdBuSZs2tjipATxAZu |
||
|
xarmian
|
885871a638 |
test(web): browser-prove attachment lifecycle completeness (TASK-2514)
The falsifiable subset jsdom can't see for PLAN-2392 phase 3c-iii — one Playwright leg per lifecycle mechanism the U1-U3 chain built, plus the fix for the U3 count test that was authored but never run. - Navigation-step (U3): a 3-image set whose viewer order is DERIVED at runtime (created_at-DESC ties are the DB's, not the upload order); the middle-navigated "arrival" is deleted via a SEPARATE API context so the process-local bus never tombstones it, its metadata is primed with a cacheable 200 in the PAGE context, and arrowing onto it after a reopen forces a no-store HEAD that 404s DESPITE the primed 200 (armed waitForResponse, causally the arrow's probe) → tombstone-advance to a distinct survivor. - Restore-revalidate (U2): on an ISOLATED workspace (the shared suite's SSE stream starves the delta-sync cursor), archive via per-item event then RESTORE via the BULK endpoint (items_bulk_updated, no item_id) — proving the prop-driven strip revalidation covers what a per-item SSE subscription would miss. Asserts no attachments.list on archive, and a one-shot route HOLDS the restore's revalidation list in flight to prove the tiles never blank DURING the fetch, not just after. - Timeline (U1): a strip-UI delete (so announceAttachmentDeleted runs on the process-local bus) reconciles a comment thumbnail img→missing live, with the document + timeline element stamped to prove no reload or remount. Also fixes attachment-surface-chrome.spec.ts's U3 count barrier: a bodyless HEAD is reported as net::ERR_ABORTED after its headers arrive, so it fires requestfailed, never requestfinished — the completion barrier now keys on the response. New e2e/lib/attachment-viewer.ts helpers: createWorkspace, createDoc, archiveItem, restoreItem, bulkItems (workspace-slug-aware), STRIP_DELETE. Claude-Session: https://claude.ai/code/session_01WFBYxdBuSZs2tjipATxAZu |
||
|
xarmian
|
ffaba632cf |
test(web): pin attachment open-set mutation contracts (TASK-2513)
PLAN-2392 phase 3c-iii U4. Pin the open-set mutation contracts DR-15 style — assert the chosen behavior, don't assume it. - Upload-during-open: a new REAL end-to-end test mounts the strip + the AttachmentSurfaceHost, opens the surface on a 2-image set, fires the upload bus, and asserts the open surface's set is unchanged (counter stays 1/2) while the strip's own tile list DOES gain the row. The two legs are independent: a dead upload bus fails the strip leg, a live-following surface fails the surface leg — neither masks the other. It pins the no-live-follow half; the in-place-mutation half stays pinned by events.test.ts's deep-snapshot test. - Rename/metadata-change: no channel exists to exercise it (api.attachments has no rename/update-in-place op; transform mints a new peer row; metadata is immutable), so the contract is WRITTEN DOWN in the events.ts deep-snapshot doc rather than tested, and the future channel is routed to IDEA-2515. Codex-reviewed (3 rounds): tightened the doc to scope claims to the event channel, correct the api.attachments surface, and account for deletion reconciliation + downstream metadata completion. Claude-Session: https://claude.ai/code/session_01WFBYxdBuSZs2tjipATxAZu |
||
|
xarmian
|
c5190d6a96 |
feat(web): revalidate attachment metadata per navigation step (TASK-2512)
Generalize T6's per-open forced-probe (`forcedNonce`) to per-(openNonce,
attachment): `forcedFor: { nonce, ids: Set }`. The opened entry AND every entry
navigated to now gets exactly one automatic `no-store` revalidation, while
arrowing BACK to an already-probed entry within the same open takes the fast
path. A cross-tab deletion of a sibling is no longer invisible when arrowing to
it. A reopen mints a fresh nonce, so the set resets and every entry re-probes.
Two semantics pin the accounting:
- COMPLETION, not dispatch (round-2 P1): a pair is recorded only when its forced
probe resolves non-stale. A probe discarded stale (arrow away before it
resolves) leaves the pair unseen, so arrow-back re-probes rather than painting
a maybe-deleted entry live off the seed.
- AUTOMATIC only (round-4 P2): a Retry-/restore-driven forced probe (the reload
path) never records the pair, keeping the two mechanisms independent — an
arrow-back after a Retry still gets its one automatic probe.
The mark is a plain-object write in the async continuation, guarded by the
existing `req.stale()` check and keyed to the pair the run dispatched for, so it
joins no tracked scope and cannot self-invalidate the effect.
Tests: this task owns the T6-era expectations its behavior change INVERTS.
- surfaceMetadata.svelte.test.ts: the two "navigation keeps the nonce → no
additional forced probe" tests now assert navigation to a fresh sibling forces
a second no-store revalidation (complete OR incomplete seed); added an
arrow-back-is-fast-path test and two new-behavior tests (delayed probe →
stale-discarded → re-probes; completed Retry does not record → arrow-back still
auto-probes), both mutation-verified to fail on the naive regressions.
- AttachmentSurfaceHost.svelte.test.ts: the arrow test inverts to "arrowing to a
fresh entry forces one no-store probe of the arrival; arrowing back does not".
- Lightbox.svelte.test.ts: corrected two tombstone-advance comments that claimed
advanced-to entries use the plain fetch (they now force per U3).
- attachment-surface-chrome.spec.ts (e2e, not runnable in this worktree): the
no-store counting test inverts — arrowing to a fresh sibling now forces one
HEAD of the arrival; final counts a:2,b:1. Kept to race-free claims only.
Claude-Session: https://claude.ai/code/session_01WFBYxdBuSZs2tjipATxAZu
|
||
|
xarmian
|
08b6d94d30 |
feat(web): revalidate attachment strip content on parent restore (TASK-2511)
The item attachment strip has no SSE subscription — its only live inputs are the in-process delete/upload buses — so a restore that happened while this browser was elsewhere never reaches it, and its rows keep rendering from the pre-archive fetch (thumb URLs work again by accident, metadata may be stale). Thread `parentArchived` from ItemDetail (the same signal the timeline U1 and the surface host DR-14 take, so it covers BULK archive/restore whose `items_bulk_updated` carries no item_id) and reconcile the CONTENT gap (PLAN-2392 3c-iii U2): - RESTORE (true->false edge): `revalidateAfterRestore` re-fetches the attachment list and MERGES it over the current rows — a DIFFERENT, gentler path than the load effect's non-retry rerun, which blanks attachments/expanded/pendingDelete/ deletedIds/pendingUploads synchronously. It never blanks (rows stay painted until replaced), preserves the tombstones / pending uploads / expanded-overflow / open confirmation the reset path would wipe, recomputes the continuation count off the fresh `total`, and clears a stale load error on success. A failed revalidation is swallowed (not surfaced as the blocking error row) — the strip already holds a good pre-archive list. - ARCHIVE (false->true edge): a content no-op. Tiles keep their painted bytes; the interaction paths already fail server-side (DR-14's 404 correction). Edge correctness: - The archived latch is keyed to VIEW IDENTITY, not just the boolean: the strip persists across item switches, so an archived item A -> active item B is also a true->false transition — reseeding the latch on any view-key change keeps a SWITCH from firing a duplicate racing load (round-3 P2). - `inFlightDeletes` (ref-counted, so a concurrent second delete of the same id can't be cleared early) excludes a row whose optimistic removal has run but whose tombstone broadcast — post-await in `performDelete` — hasn't yet, so a restore refetch landing in that window can't repaint the just-removed tile (round-3 P1). - The revalidation DEFERS to a load already fetching THIS view (per-view counter, since the api client has no request abort and a stale prior-view load lingers): that load returns fresh-enough data, and superseding it would strand the strip empty if the revalidation then failed. Latches are plain `let`s read/written under `untrack` (the Svelte self-write trap). Adds the strip's `parentArchived` prop mount in ItemDetail. 22 new unit tests; each guard mutation-verified. Claude-Session: https://claude.ai/code/session_01WFBYxdBuSZs2tjipATxAZu |
||
|
xarmian
|
e3d80619f9 |
feat(web): reconcile timeline attMeta across attachment lifecycle (TASK-2510)
ItemTimeline cached attachment HEAD-probe results in `attMeta` with no
invalidation path: a deleted attachment stayed a live `<img>`, an archived
parent kept painting a soon-to-be-broken image, and a restore never escaped a
`missing` cached while archived.
Add three reconciliation surfaces (PLAN-2392 3c-iii U1):
- Deletion bus subscription + per-id `tombstoned` set. A HEAD that resolves ok
AFTER the delete can't repopulate `attMeta`; the tombstone is per-id so one
deletion never false-fences another attachment's in-flight probe.
- A per-timeline lifecycle epoch captured at probe dispatch and checked before
every authoritative write, bumped by both archive/restore edges — a
pre-archive ok or pre-restore missing that lands after the edge refuses to
write.
- A `parentArchived` PROP (threaded from ItemDetail, mirroring the surface
host's DR-14 prop — NOT an SSE subscription, so it covers bulk
archive/restore whose `items_bulk_updated` carries no item_id). While true,
every probe goes through `revalidateAttachmentMetadata(..., {cache:'no-store'})`
so a stale cached ok can't repaint a broken image and a genuine 404 lands as
missing (the LEVEL rule). The false->true edge drops this item's tracked
attMeta/probe state; true->false re-probes the unresolved set no-store via a
reactive `probeNonce`.
Both lifecycle edges reconcile over the whole tracked set (referenced ∪ attMeta
∪ probed ∪ unresolved), not just currently-referenced ids, so an attachment
resolved-then-unreferenced can't replay a stale ok as a broken image or skip a
restore re-probe on a stale probe mark.
Claude-Session: https://claude.ai/code/session_01WFBYxdBuSZs2tjipATxAZu
|
||
|
xarmian
|
dfc3442bba |
Merge pull request #1069 from PerpetualSoftware/feat/attachment-surface-convergence
PLAN-2392 phase 3c-ii: one attachment surface — the convergence |
||
|
xarmian
|
bbb5a69219 |
fix(attachments): dock-clear the viewer nav on the mobile sheet (PLAN-2392 3c-ii)
The prev/next arrows were direct children of the fixed backdrop, centred `top: 50%` against the FULL viewport. The T5 phone sheet shortens the stage and docks meta+toolbar at the bottom, but the arrows had no sheet-scoped anchor, so on short/landscape phones they landed in or over the dock — obscured, or stealing the dock's taps. Move the two `.lightbox-nav` buttons INSIDE `.lightbox-stage`. On desktop the stage is `position: static`, so their `position: absolute` still resolves against the fixed backdrop — byte-identical full-viewport centring. In the sheet the stage is `position: relative`, so `top: 50%` re-anchors to the shortened stage box and the arrows clear the dock with no magic-number dock height. Add `pointer-events: auto` to `.lightbox-nav` (the stage is `pointer-events: none`); on desktop that was already the inherited value. Nav now trails the toolbar in DOM order (Close, toolbar, Previous, Next); accessible-name addressing keeps the trap tests green — adjusted the two order-naming assertions in the modal-contract spec. Adds a 720x400 landscape e2e leg asserting the arrows centre on the stage (not the viewport), sit clear of the dock, and stay clickable; the pre-fix DOM fails the stage-centre assertion by a dock-half. Claude-Session: https://claude.ai/code/session_01WFBYxdBuSZs2tjipATxAZu |
||
|
xarmian
|
9c9107176c |
test(e2e): reconcile attachment e2e with the converged surface (TASK-2493)
PLAN-2392 phase 3c-ii T7 — the e2e half of the convergence (one host, one Lightbox for ANY attachment; the options-panel + image-viewer channels retired). Falsified + rewritten (the convergence changed the premise, so these were rewritten to assert the new behaviour, not deleted): - strip file-tile / editor file-chip open the role=dialog surface (no-bytes fallback arm), not a role=menu options panel - modal two-stacked-viewers -> the SUPERSEDE invariant (one host mounts at most one Lightbox by construction) - owner-4 BottomSheet source moved from the retired file-panel to the surviving strip delete-confirm menu - parity/two-host exact dialog-name -> anchored RegExp (T2b grew the accessible name to "name, type · size"); enforced in the hostile-name leg - zoom thumb->original timeline, switch-safety, and mobile deferred-load counts: filter to GET (the T6 always-revalidate-on-open no-store HEAD hits the same variant-less URL and polluted the counts) New legs: PDF/ZIP fallback integration (Open for PDF, none for ZIP); T6 no-store HEAD count (one per open, none on arrow, one on reopen); DR-14 archived-parent probe-gate + archive-while-open close; dual-host peeked addressing + un-peek; Pixel-7 sheet geometry / dock contiguity / backdrop-vs-chrome dismissal / shortened-stage zoom / file route / overlay-centring / DR-18 label reveal / native-pinch touch-action / forced-colors Canvas plate; desktop-unchanged contrast. Each of the four load-bearing behaviours was MUTATION-verified (break in source, rebuild the worktree ./pad, confirm the targeted leg FAILS, restore, confirm green): fallback admission, host event addressing, archive-close transition, and the T6 forced no-store probe. Codex-reviewed to CLEAN over five rounds. New selectors live in web/e2e/lib/attachment-viewer.ts, addressed by class or accessible name (never a bare [role="dialog"]); assertions are item-scoped / by-id / by-anchored-name to avoid the BUG-2504 unscoped-list pagination trap. Claude-Session: https://claude.ai/code/session_01WFBYxdBuSZs2tjipATxAZu |
||
|
xarmian
|
5ee40d728f |
feat(attachments): mobile phone-sheet layout for the viewer (TASK-2492)
The AM-3 Lightbox-owned phone-sheet layout (PLAN-2392 3c-ii / T5). A reactive `isSheet = $derived(viewport.isMobile)` toggles a `.lightbox-sheet` class on the existing dialog root, and CSS scoped under that class re-lays-out the EXISTING chrome into a bottom-anchored sheet: the toolbar and meta leave their desktop absolute anchors and dock, stacked, to the bottom edge (via `position: static` + `order`), the stage fills the space above (and becomes its own containing block so its overlays centre over the shortened stage, not the dock), and the counter moves to the top-left. A class, not a bare `@media`, so JS and CSS share the one app breakpoint and the flip is a DOM fact the modal-contract jsdom suite can drive and read. The layout is fully layout-independent of the modal contract: the portal, lease, focus trap, escapeStack registration, loader and zoom transform are untouched, and nothing is keyed on the viewport, so a breakpoint flip mid-open re-lays-out the SAME instance with zoom/selection state intact. No `BottomSheet`/`Menu` instance nests, no swipe dismissal, and no `touch-action`/pointer-capture changes (per the amended DR-6). Every rule is scoped under `.lightbox-sheet`, so the desktop layout is byte-identical, and the sheet chrome carries its own forced-colors boundary. Tests: sheet selection, a mid-open flip proving same-instance re-layout (root + img identity, src + zoom survive), the docked chrome staying excluded from all three pointer-gesture lists (pointerdown/wheel/dblclick, each with a live control), no dismissal on a chrome click, and the full modal contract re-run under the mobile viewport mock. Geometry, touch, `@media`/forced-colors visuals are named for T7's browser legs. Claude-Session: https://claude.ai/code/session_01WFBYxdBuSZs2tjipATxAZu |
||
|
xarmian
|
44ee5ad806 |
feat(attachments): always-revalidate-on-open via per-open nonce (TASK-2491)
The converged attachment surface now revalidates the OPENED entry's metadata
on every open, so a cross-tab / background delete is caught rather than shown
as a live-looking row backed by a stale HEAD.
Mechanism: AttachmentSurfaceHost mints a per-open `openNonce`, incremented once
per accepted surface request and ridden on the request object into Lightbox (so
the `{#key request}` remount carries the matching nonce). The nonce joins the
metadata machine's SUBJECT identity (ws, attachmentId, nonce) and drives a
`forcedNonce` tracker that forces exactly one probe of the opened entry per open
— gated on the probe's own precondition (`isOpen && addressable`) so a not-yet-
open/addressable subject can't burn the nonce. Navigation keeps the nonce, so
arrowing does not force (3c-iii owns navigation-step revalidation). Unlike
`seenReload`, the nonce is deliberately NOT seeded from the incoming value: the
guarantee is to force on the first nonce seen.
The forced probe passes the literal `cache: 'no-store'` fetch option, threaded
through revalidateAttachmentMetadata -> fetchAttachmentMetadata -> fetch init,
so the endpoint's `max-age=3600` HEAD cannot serve a stale cached HEAD and
defeat detection. A `missing` result routes through the existing tombstone path.
Deliberate behavior change: the strip's zero-probe fast path is gone. A
complete-seed open previously issued no HEAD; it now issues exactly one forced
no-store revalidation (the displayed fields still come from the seed — seed-wins
merge — so the header is unchanged). A HEAD is not a byte fetch: the mobile
deferred cell's no-auto-bytes rule is untouched. The renderer load key does NOT
gain the nonce — a reopen is a whole new keyed mount, so cross-open coherence is
the remount's job and the nonce is constant within an open.
Claude-Session: https://claude.ai/code/session_01WFBYxdBuSZs2tjipATxAZu
|
||
|
xarmian
|
af2a6ed57d |
refactor(attachments): retire the two legacy attachment channels (TASK-2490)
3c-ii T4b: with every producer already on the surface channel (T4a), delete
the two legacy channels and the cutover-window bridge. Gone from
`events.ts`: `notifyViewerOpen`, `notifyAttachmentPanelOpen`,
`ViewerReadyImage`, `ViewerOpenRequest`, `AttachmentViewerOpenEvent`,
`AttachmentPanelOpenEvent`, both legacy predicates
(`isAttachment{Viewer,Panel}EventForHost`) and registries
(`registerAttachment{Viewer,Panel}Listener`), plus the producer-boundary
MIME-drop gate (it lived inside `notifyViewerOpen`). The surface channel —
no admission MIME gate, renderer arm decides — is the sole open channel.
`AttachmentSurfaceHost` loses its two legacy subscriptions and the
`fromPanel` / `fromViewer` translators, and its `wsSlug` prop retires (the
surface channel captures its own workspace at emit); `ItemDetail` stops
passing it. The now-dead viewer-toolbar context props left by T4a
(`mutationsEnabled` / `getItemContent` / `getLiveContent`) delete from
`ItemAttachmentStrip` and `ItemTimeline` — the host forwards them to
`Lightbox` directly. Stale `AttachmentViewerHost` / panel-channel comments
updated across the touched files.
Tests: the legacy-channel unit + bridge tests delete with the channel; the
three host tests that exercised real surface behavior through a legacy event
re-point onto the surface channel (and the invoker + one-probe tests gain
discriminating assertions). Full suite green (1357), `npm run check` 0 errors.
Reviewed to a fresh-angle CLEAN over four Codex rounds.
Claude-Session: https://claude.ai/code/session_01WFBYxdBuSZs2tjipATxAZu
|
||
|
xarmian
|
72a7491dff |
feat(attachments): all six producers emit the surface channel (TASK-2489)
3c-ii T4a: every attachment producer now emits `notifyAttachmentSurfaceOpen` with a captured `workspaceSlug`, its `invoker`, and its seeds — the two direct `Lightbox` mounts (the strip's and the timeline's) are gone, and the ONE `AttachmentSurfaceHost` owns every open. The bridge stays subscribed throughout, so this window still ships a working route. - Strip IMAGE tile (`openLightbox`): direct mount deleted; emits the raster `lightboxImages` set at the clicked index, invoker = the tile. - Strip FILE tile (`openOptions`): panel → surface, a single-image set from the list row, invoker = the anchor. The flat seeds are normalized to match images[0] exactly (`filename || null`) — a blank `''` filename against a `null` record would fail the notify validator and silently drop the open. - Timeline (`openLightboxFromImg`): direct mount deleted; emits the sibling `viewerImageFor` list, invoker = the img. - Image NodeView: the raster/non-raster fork collapses to ONE surface emit after the (unchanged) resolve-before-emit gate — svg and raster both emit the same event, and the surface's own `getSurfaceRenderer` picks the arm. - Chip: panel → surface single; `workspaceSlug` captured from the live address. Both direct-mount producers gain a `paint.isCurrent()` STALE-ACTIVATION fence they lacked: the strip reuses its existing paint fence; the timeline gains one (a `viewIdentity` + `createPaintFence` recorded in an effect that tracks `entries` and captures the view through `untrack`, so a bare workspace change that has not yet reloaded keeps the old paint and refuses a stale click). The timeline's old `lightbox`-clear-on-switch is retired — the host closes the open surface on the resource switch now. Tests: the nine producer suites migrate to the surface channel — each producer's emitted workspaceSlug / invoker / seeds / index asserted, and the strip/timeline opens asserted THROUGH a mounted `AttachmentSurfaceHost` (the real Lightbox end to end). `viewerImagePayload`'s direct-mount premise is falsified and rewritten to assert the emitted set. The svg cases assert one surface emit / the fallback arm rather than "opens nothing". Housekeeping: the stale `AttachmentViewerHost` comments in the touched files updated. npm run test 1389 pass, npm run check 0 errors; the blank-filename open is regression-pinned; reviewed to a fresh-angle CLEAN (round 1 caught the flat-seed mismatch, round 2 clean). Claude-Session: https://claude.ai/code/session_01WFBYxdBuSZs2tjipATxAZu |
||
|
xarmian
|
9dc569c866 |
feat(attachments): the atomic cutover — one surface host, panel retires (TASK-2488)
3c-ii T2b, the commit users feel. ItemDetail's two attachment hosts collapse to ONE `AttachmentSurfaceHost` at the top-level position, carrying the union prop set (wsSlug for the legacy-panel bridge + itemId + hostToken + resourceGen + mutationsEnabled + the content getters + parentArchived). BUG-2413's server fail-closed disposition is on the base, satisfying the merge-gate. Deleted: `AttachmentPanelHost`, `AttachmentViewerHost`, `AttachmentDetailsPanel` (551 lines, + its CSS + its Menu/MenuItem usage). `AttachmentDeleteConfirm` survives as the drill-down inside the surface. The panel's `closeAfterNavigation` retires with it — the surface stays open after Open/Download, where the panel closed itself. Every open now flows through the one host and the file-capable Lightbox (T3): a strip file tile → the fallback arm + file toolbar (through the legacy panel channel + the bridge — producers repoint in T4a), a chip → the same, an image NodeView → the raster arm, a non-raster redirect → the fallback arm. Preserved three panel behaviors the cutover would otherwise have dropped (the round-3 "what did the panel do that nobody ported" lens): - A SINGLE-item surface whose file 404s shows the panel's inert "no longer available" overlay instead of flash-closing — `soleMissing` keys on `images.length === 1` (a panel open is always single), disposes the loader (no bytes) and keeps the toolbar inert; a MULTI-image set still advances / closes through the tombstone path, and an EXTERNAL bus delete still closes a single, exactly as the panel host did. - The dialog's accessible name is the display name plus the header's type · size, not a bare alt. - A null-filename file is named with the shared "Untitled file" fallback (the bridge uses `displayFilename`), not the Lightbox's bare "Attachment". Test migration (named, not silently dropped): the extraction grep-gate → `Lightbox.extraction.test.ts` (same contract, new consumer); the NodeView → host → Lightbox whole-route test retargeted onto this host (the SVG redirect now lands on the fallback arm); the three host suites consolidated into `AttachmentSurfaceHost.svelte.test.ts` (lifecycle/addressing from T2a) and the Lightbox suite (panel behavior), with a migration manifest naming what moved where. Grep: zero PRODUCTION references to the three deleted components. npm run test 1388 pass (Lightbox 190, host 22), npm run check 0 errors; the single-item overlay + the aria-name changes are pinned; reviewed to a fresh-angle CLEAN (round 1 caught the three dropped behaviors, round 2 clean). Claude-Session: https://claude.ai/code/session_01WFBYxdBuSZs2tjipATxAZu |
||
|
xarmian
|
e0c06b0bdf |
feat(attachments): unified surface host with three-channel bridge (TASK-2487)
3c-ii T2a: `AttachmentSurfaceHost` — the one host that replaces the legacy panel
host + viewer host, mounting the grown Lightbox that opens ANY attachment. Built
COMPLETE but mounted NOWHERE: T2b does the atomic cutover (mount this, delete the
two legacy hosts + the panel), T4a repoints the six producers. Only its own suite
mounts it (grep confirms zero app-code references).
Three channels, one request (the bridge invariant). During the migration the
producers still emit on the two LEGACY channels, so the host subscribes to all
three (surface, panel, viewer) and TRANSLATES the legacy shapes INTERNALLY into
its own request state — it never calls `notifyAttachmentSurfaceOpen`. One `$state`
request → `{#key request}` → one Lightbox mount, so an accidental old+new double
emission for the same open supersedes rather than double-opens.
Translation, explicit: a legacy PANEL event becomes a single-open request with
`invoker = anchor` (never the live activeElement; no positioning), a one-element
images set from the seeds, and `workspaceSlug = the host's wsSlug prop` — the
transitional exception, since the panel channel carries no workspace (the new one
always does). A legacy VIEWER event maps field-for-field, keeping its captured
workspace and filling the flat seeds from images[index].
Lifecycle, ported from the two hosts and stated as the rule T3 deferred:
- Archive-closes / restore-revalidates, TRANSITION-based — which naturally splits
archive-while-open (close) from open-while-already-archived (no transition → the
surface mounts probe-gated INERT, not a flash-close: the user asked for the file,
so show the inert "unavailable" state rather than blink it shut).
- Resource-switch clear on itemId change OR resourceGen advance (the complete rule).
- External-deletion close-when-SINGLE; a multi-image set is left to the Lightbox's
own tombstone path (advance / close-last), never preempted by the host.
- closeRequest bound to its target (stale-continuation fence) and `request?.` guards
on the lazily-read Lightbox props (a delete continuation reads them after the
close nulled request).
Adds a minimal `revalidateToken` prop to Lightbox (threaded into the metadata
address, replacing the hardcoded 0) so restore re-probes an archived-at-open
surface; T6's always-revalidate-on-open openNonce layers onto the same input.
Tests: the three subscriptions; exact-once per channel (one legacy event → one
request → one mount → one probe → one focus return; old+new double emission does
not double-open); translation fidelity incl. the transitional-wsSlug and
captured-wsSlug cases; anchor→invoker for focused/null/disconnected; dual-host
addressing isolation; and the ported panel-host lifecycle (archive/restore/
item-switch/resourceGen/deletion). npm run test 1440 (host 21), npm run check 0
errors; the set-vs-single deletion guard mutation-verified; reviewed to a
fresh-angle CLEAN.
Claude-Session: https://claude.ai/code/session_01WFBYxdBuSZs2tjipATxAZu
|
||
|
xarmian
|
af055a9bc9 |
feat(attachments): Lightbox admits any attachment — file route and reclassification (TASK-2486)
3c-ii T3: the image viewer becomes the converged surface that opens ANY
attachment. PURELY ADDITIVE — no producer routes files/archived to it yet (T4a),
so only the tests exercise the new capability and production behavior is
unchanged.
Admission flips (DR-20 final form). The last-mile filter that kept only
allowlisted-raster MIMEs navigable and REFUSED null/unsafe is gone, along with
the `unsafeAtOpenIds` snapshot: every entry is admitted, and safety moves to the
ARM. `shownRenderer` (and the toolbar's Open, and the fallback icon) derive from
the RESOLVED MIME — the seed's, or what the metadata machine's HEAD probe filled —
so `'raster-image'` mounts the `<img>` + bytes while a non-raster type (unsafe,
a file, or a still-unresolved MIME) mounts the no-bytes icon fallback. Admitting
unsafe/unresolved renders no hostile bytes: the arm fails closed on the resolved
MIME, joins the load key, and the loader is disposed off the raster arm.
Reclassification. A null-seed open shows the fallback until its probe answers,
then re-derives: raster → the image arm, PDF → fallback + Open, ZIP → fallback
without Open. The raster load hands the loader the RESOLVED mime
(`{ ...img, mime_type: resolvedMime }`) so its own DR-16 gate — which reads the
img it is given — agrees with the arm rather than refusing a null-seed row the
arm admitted.
Archived parent. `parentArchived` is a prop now (was hardcoded false), threaded
to the metadata machine so an archived-parent open forces a reachability probe
(DR-14). Every toolbar action is inert while `missing || unreachablePending`,
where `unreachablePending = parentArchived && (phase !== 'ok' || slow) && !missing`
— inert until a SETTLED ok, covering seeded, transient (slow-timeout), and a
forced re-probe after a prior ok. Disabled anchors drop their href (keyboard-inert),
not just aria-disabled. Threading the prop through the production hosts is T2a/T4a;
the archived lifecycle (archive-closes / restore-revalidates) lands with the host.
Tests: swept and rewrote the falsified 3c-i at-open-refusal pins to
admission+fallback assertions; added the file route (PDF Open present, ZIP absent),
delayed null-seed reclassification (raster/PDF/ZIP), and archived-parent gating
(pending / transient-stays-inert / ok→archived re-probe / re-enable-on-ok). The
3c-i unsafe-mid-view tests still pass. npm run test 1419 (Lightbox 190), npm run
check 0 errors; key invariants mutation-verified; reviewed to a fresh-angle CLEAN.
Claude-Session: https://claude.ai/code/session_01WFBYxdBuSZs2tjipATxAZu
|
||
|
xarmian
|
da1b3f82ed |
feat(attachments): unified surface-open channel (TASK-2485)
The 3c-ii convergence's first task (T1): one bus channel for opening ANY attachment on the grown Lightbox — image, file, or a row whose type is not yet resolved — alongside the existing panel and viewer channels. Purely ADDITIVE: the two legacy channels are untouched and nothing repoints yet (T2a builds the host, T4a repoints producers, T4b deletes the legacy channels). Adds `AttachmentSurfaceOpenEvent`, `notifyAttachmentSurfaceOpen`, `isAttachmentSurfaceEventForHost`, and `registerAttachmentSurfaceListener` to events.ts, matching the sibling channels' conventions (DR-8 addressing via `isAddressable`, one host token per mount, same guard order and comment voice). Differs from the viewer channel by design: NO admission MIME gate (files and null-MIME/unresolved rows pass — the allowlist governs the render arm downstream, never admission), and no `anchor` (the centered surface returns focus via `invoker`). The event carries a CAPTURED-at-emit workspaceSlug (required, no host fallback) and nullable single-attachment seeds that, when present, describe images[index]. The emitter is the convergence boundary every producer will funnel through, so it is hardened accordingly: it reads every input EXACTLY ONCE (event scalars, the array length, each of the seven record fields), enforces the event's own invariants at the boundary (index in range; images[index].id === attachmentId; a non-null flat seed must agree with that record), and delivers a DEEP snapshot built by explicit field projection — a fresh all-primitive record per entry and an explicit event projection — so a caller that keeps mutating its set, a getter/proxy TOCTOU, a shadowed `.map`, a stray property, or a non-string identity field cannot reach an open surface. `invoker` is the one intentional live reference (the focus target). Tests (events.test.ts): predicate address isolation + null event; the capture rule; null-MIME pass-through (the old gate's drop asserted ABSENT here); the four boundary cases each their own test (out-of-range index, id mismatch, inconsistent seed, deep snapshot of array AND records); plus record-integrity, projection, and undefined-seed cases. cd web && npm run test (1410) + npm run check (0 errors) green; key assertions mutation-verified; reviewed to a fresh-angle CLEAN. Claude-Session: https://claude.ai/code/session_01WFBYxdBuSZs2tjipATxAZu |
||
|
xarmian
|
18b3b33530 |
Merge pull request #1070 from b4rk13/feat/markdown-list-output
feat(cli): markdown output for the list commands |
||
|
David Barkhausen
|
f900b0aefb |
fix(cli): address review on markdown list output
All three requested changes from @xarmian's review of #1070, plus both nits. 1. Escape backslashes before pipes in escapeMarkdownCell. A title containing "\|" became "\|", which GFM reads as an escaped backslash followed by a LIVE pipe, so the row still gained a column. Backslash-first turns it into "\\|". Confirmed the bug with a failing test before fixing it. 2. Sanitize the group headings. Extracted SanitizeMarkdownText (SGR strip + newline collapse) and ran the collection icon and name through it, so a newline in a collection name can no longer inject a second "## " heading. Sanitizing happens per part, before joining, because it trims and would otherwise eat the separating space. Pipes are deliberately not escaped outside a table. 3. Tightened the --format help to the precise enumeration: "markdown on: item list/starred, collection list, item show, project changelog" per option (a) on #898. Nits: - `item starred --format markdown` on an empty result now says "No starred items." rather than the shared renderer's "No items found."; the empty check moved above the format branch so both paths agree. - Added format_markdown_routing_test.go: three end-to-end tests driving `item list` and `collection list` through cobra against an httptest server, asserting the markdown branch is actually reached and that the table and markdown paths don't leak into each other. Proven by disabling the markdown branch and watching the test fail. Follows the item_open_test.go pattern, with USERPROFILE set alongside HOME since os.UserHomeDir reads USERPROFILE on Windows — worth noting, as tests that set only HOME are why part of the credential-store suite fails there. Gates: go build ./... PASS; go vet PASS; gofmt clean; golangci-lint 0 issues; all markdown tests PASS. Both touched packages show the same 6+2 pre-existing Windows failures as clean main under an identical sandboxed run. |
||
|
David Barkhausen
|
4ab7b10b35 |
feat(cli): markdown output for the list commands
Implements `--format markdown` on the list commands that lacked it, so the format is honestly global rather than honestly-partial (#898, the option (a) follow-up to #851). - `pad item list` — grouped `## Icon Name (N)` sections with a table each when listing across collections (mirroring the table layout), a single table when scoped to one collection. Heading style matches `project changelog`. - `pad item starred` — single table. - `pad collection list` — Name / Slug / Items / Default. - `--format` help no longer carries the "markdown on select commands" caveat. The markdown renderers deliberately do NOT reuse the colorized helpers (ColorizedStatus, PriorityColor, Dim): markdown goes to a file, a PR body or an agent's context, never a terminal, so raw values go in and the reader's renderer styles them. Every cell is escaped — an unescaped `|` in a title silently adds a column and corrupts the row. Refs #898 |
||
|
xarmian
|
e08901df28 |
fix(e2e): resolve imported attachment via content reference, not list page 1 (BUG-2504) (#1075)
The round-trip spec's final assertion fished the imported workspace's unscoped attachment list, which defaults to limit 50 / created_at_desc. Once sibling specs (PLAN-2392 browser proofs) grew the shared e2e workspace past one page, the seeded logo — old in the sort — fell off page 1 and the find() failed, holding main's CI red since 2026-08-06. The trigger window contained only CI-action bumps; the race was latent and runner-timing shifts made it deterministic. Resolve the rewritten pad-attachment: UUID straight from the imported item's content instead — the contract the UI actually follows — and assert filename via Content-Disposition plus a byte-for-byte download match. Immune to suite growth by construction. Claude-Session: https://claude.ai/code/session_01VxyZv1g6W6rGx7nuaGcH3i |
||
|
xarmian
|
be7baa7173 |
Merge pull request #1068 from PerpetualSoftware/fix/bug-2413-mime-disposition
fix(server): force-download unknown and disallowed stored MIME (BUG-2413) |
||
|
xarmian
|
3bd6244001 |
fix(server): force-download unknown and disallowed stored MIME (BUG-2413)
The attachment read path chose Content-Disposition from the stored MIME and DEFAULTED unknown types to inline, flipping to attachment only for the RenderForceDownload bucket. A legacy or mislabelled image/svg+xml, an extensionless SVG stored as text/xml, or an unrecognized row was therefore served inline from the app's own origin — active same-origin content, one click away once 3c-ii's converged surface gives every row a Copy-link. Fail closed. Content-Disposition now defaults to attachment; a row is served inline only when its stored MIME is on the allowlist AND in an EXPLICIT inline-safe set (MIMEEntry.ServeInline) — the passive raster/audio/video types the app embeds, plus PDF and plain text. The set is a standalone allowlist, not a function of RenderMode, so a future RenderInline entry can't silently auto-inline an active type; a new type fails safe (downloads) until explicitly listed. A MIME that isn't on the allowlist at all is additionally served as application/octet-stream so its bytes are never echoed back as a type the browser might act on. X-Content-Type-Options: nosniff was already set. The gate is at the single choke point: GET, HEAD, share-link access, and the ?variant= path all flow through handleGetAttachment (the transform endpoint only decodes images into a new raster thumbnail; the bundle/account exports never serve individual bytes inline). Regression tests cover an SVG-labelled row, a text/xml row, an unknown-MIME row (attachment + octet-stream), and the variant path forced to attachment, plus PDF and plain text staying inline — GET and HEAD. Mutation-verified: reverting to the old fail-open default fails exactly the SVG/text-xml/unknown/variant tests. Reviewed to a fresh-angle CLEAN. Claude-Session: https://claude.ai/code/session_01WFBYxdBuSZs2tjipATxAZu |
||
|
xarmian
|
1364254c79 |
Merge pull request #1067 from PerpetualSoftware/wt/task-2478
test(store): pin DR-15 attachment clone/move/bundle semantics (TASK-2478) |
||
|
xarmian
|
e9af504cbc |
test(store): pin DR-15 attachment clone/move/bundle semantics (TASK-2478)
Three store-level pinning tests for PLAN-2392's DR-15 — no behavior changes, locking down assumptions the copy / move / bundle paths rely on silently: - Clone independence: a cross-workspace copy mints a fresh, live attachment row that shares only the bytes (content hash), and soft-deleting the clone or the source never cascades to the other (both directions pinned; there is no store-level attachment restore, so delete stands in for the row-separation invariant a restore would ride). - A move charges both workspaces: ArchiveSource soft-deletes only the source ITEM; the source attachment rows (the original attached via item_id) stay live and their bytes are counted in both workspaces' storage usage. The clones land live on the copied item with the variant reparented to the new original. - Bundle round-trip orphan: WorkspaceAttachmentsForExport includes a live attachment whose parent item is soft-deleted, while ExportWorkspace's item list excludes that item — the divergence that leaves import unable to remap ItemID (handlers_import_bundle.go:479-510), landing the row as an orphan. Pinned as known behavior, not fixed. Dual-dialect (store helpers only; passes under make test-pg). Each pin was mutation-verified to fail when its behavior regresses (content-hash delete cascade, archive→attachment cascade by item_id, and an export that stops excluding soft-deleted items), and reviewed to a fresh-angle CLEAN. Claude-Session: https://claude.ai/code/session_01WFBYxdBuSZs2tjipATxAZu |
||
|
xarmian
|
0f4b695e2d |
Merge pull request #1065 from PerpetualSoftware/feat/attachment-surface-chrome
PLAN-2392 phase 3c-i: the attachment surface grows its chrome |
||
|
xarmian
|
7e85d1ce4e |
fix(attachments): viewer honors the missing metadata phase; comment refresh (PLAN-2392 3c-i)
Route the surface metadata machine's authoritative `missing` (404) phase through the viewer's existing tombstone/advance path: an out-of-page delete (another tab, a job, the API) never crosses the process-local deletion bus, but where the header probes the shown image its HEAD returns `missing`, which is just as authoritative (DR-17). A sentinel + untrack keep the effect CONVE-1688-safe and terminating (a whole gone set cascades advance→advance→close, one 404 at a time). Only `missing` latches; a `transient` stays retryable. Fence `runToolbarAction` against the shown identity and reset `toolbarBusy`/ `toolbarError` on subject change, so a confirmed or slow delete of the shown image that races the advance can't strand "Deleting…" or an error on the survivor now on screen. Refresh the final-state comments: the viewer is now the second consumer of the shared action list; the viewer host carries live `mutationsEnabled` since C1 while the open channel stays permission-free; the deletion bus is process-local, and the missing path's probe-scope (strip images seed mime+size and aren't re-probed) is stated where it matters. Tests: missing→advance, missing-only→close, whole-set cascade→close, transient→ no-op, and shown-delete-while-confirm-up→clean-toolbar (mutation-verified against the reset). Claude-Session: https://claude.ai/code/session_01WFBYxdBuSZs2tjipATxAZu |
||
|
xarmian
|
a593407a21 |
test(attachments): browser proof for the 3c-i surface chrome (TASK-2484)
DR-9's rule — the a11y and interaction work of the A-E chain is verified in a
browser or it is not verified. Desktop-chromium legs for the viewer's toolbar,
metadata header, delete flow, permission gate and gesture seams (the sheet
layout has no mobile e2e until 3c-ii).
New spec web/e2e/attachment-surface-chrome.spec.ts:
- Toolbar renders on all THREE origins (strip, timeline, body NodeView), with
Open/Download as real anchors carrying the EXACT canonical variant-less URL
(^/api/v1/workspaces/{ws}/attachments/{id}$) and the exact download filename.
- The permission gate: a peeked side withholds the delete affordance
(mutationsEnabled=false reaches it) and the active side's viewer toolbar
offers Delete.
- The delete flow: toolbar Delete → drill-down reached BY KEYBOARD with the
roving tabindex asserted (0/-1 ↔ -1/0), confirmed with Enter, the viewer
ADVANCES to the survivor (not the retired C1 close), and the deleted strip
tile disappears (bus reconciliation).
- The metadata header: name/type/size visible, a 180-char filename clipped with
a resolved text-overflow:ellipsis and the full value in title (DR-13), and the
inert-label contract proven by a DRAG on the header that does not pan a
zoomed BIG_PNG image nor dismiss the viewer.
- The gesture seams: a wheel over the toolbar zooms neither the image nor the
inert page behind it, with a control wheel over the stage that DOES zoom.
Every leg was mutation-checked against this worktree's built binary (revert the
impl line, rebuild, confirm the test fails, restore) — wheel exclusion,
peek-permission (mutationGate canEdit && !peeking), header name, delete advance,
toolbar-render, and the header pointer-exclusion. The wheel and header
mutation-checks each surfaced a false-pass that was fixed (a zoom-out clamped to
fit; a too-small image with no pan bound).
The FALLBACK arm + no-bytes invariant is a documented test.fixme: it is not
reachable through the real producers (they snapshot the viewer set at open and
filter unsafe MIME before it reaches the viewer), so it is jsdom-proven
(TASK-2476, via direct prop mutation). The peeked-side no-Delete VIEWER is
similarly jsdom-proven (TASK-2474): the content click that opens a viewer
re-activates (un-peeks) that side under the invisible-freeze model.
Two existing modal-spec trap tests were updated: the toolbar added focusable
controls, so the "last control" is derived (focusViewerLastControl) rather than
named, and the wrap is still asserted by name at both edges.
https://claude.ai/code/session_01WFBYxdBuSZs2tjipATxAZu
|
||
|
xarmian
|
10b99dec2e |
feat(attachments): viewer deletion subscription — advance or close by identity (TASK-2477)
The image Lightbox subscribes to the deletion bus and reconciles a delete by IDENTITY (DR-5c): the shown image is tracked by id, the index derived, so a delete advances to a survivor or closes — never lands on a position that now names a different member. - The index-based `current` state is replaced by `shownId` (the shown image's id) + a `tombstones` Set. `survivors` = navigable minus tombstones (composes with D's unsafe/unresolved exclusions — a fallback-arm entry is deletable too); `shownIndex` derives from `shownId` (falls to 0 when it dangles); prev/next write `shownId`. Tombstones are per-instance and never reset — every producer keys the mount, so a reopen is a fresh empty set (no cross-open leakage). - handleDeletion(uuid): idempotent; a delete NOT in the surviving set (unsafe/ unresolved, another item's attachment, a dangling shown id) is tombstoned and ignored (no advance, no closing an already-empty viewer). An in-set delete advances `shownId` to the entry that followed the deleted one (wrap when the last) when the SHOWN one went, closes when zero survive, and leaves `shownId` put otherwise (identity, not index — deleting an earlier image keeps the same one shown). The id-keyed zoom-reset fires exactly on a real advance. - ONE path for both origins: the toolbar's own Delete announces on the bus (the descriptor's `announceAttachmentDeleted`), identical to an external delete, so the survivor logic can't tell them apart. The C1 close-on-delete latch is retired (the viewer had no survivor logic then); the toolbar ctx now omits `onDeleted` (that stays the panel's). The listener is disposed in the same teardown that releases the backdrop lease. Tests: the DR-5c case table (only/zero-left → close, one-left, deleting-shown → advance, wrap-around, deleting-earlier → same image by identity, reopen clears tombstones), plus delete-during-drag, delete racing the confirm drill-down, a non-image fallback delete, the dangling-id / empty-viewer guard cases, and a two-image toolbar integration in the strip suite (confirm → api → announce → advance). The strip's events mock now fans `announceAttachmentDeleted` out to listeners, matching production. Claude-Session: https://claude.ai/code/session_01WFBYxdBuSZs2tjipATxAZu |