mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-24 03:16:43 +00:00
00a91dfcf463037af8ada3e8f3edec01ad40e664
600 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 xarmian
|
00a91dfcf4 |
feat(push): session targeting — target_session_id + delivered_sessions (TASK-2588) (#1108)
* watchevents: add session-targeted push delivery predicate
PLAN-2558 S5 (TASK-2588). Notification gains TargetSessionID,
evaluated in the existing per-connection KindPush predicate in
watchNotificationVisible alongside TargetUserID — one delivery path,
targeted is broadcast-with-a-predicate, no bus changes. Empty
TargetSessionID (the pre-S5 shape) still matches every one of the
target user's sessions.
* server: accept target_session_id on push, report delivered_sessions
PLAN-2558 S5 (TASK-2588). POST .../items/{slug}/push accepts an
optional target_session_id (an id from GET /api/v1/sessions) and the
response gains delivered_sessions — a prediction read from the S1
presence registry at push time, scoped to the caller's own
ListForUser(userID) so a vanished id and one belonging to a different
user are both an honest 200/0 with no existence oracle across users.
Omitting the field keeps the exact pre-S5 request/response shape.
* web: session picker in the push composer, targeted-miss handling
PLAN-2558 S5 (TASK-2588). PushToAgentDialog gains a target picker
(broadcast default + one option per live session), reusing the
presence read already fetched for the count — no second GET
/api/v1/sessions. Selecting a session passes target_session_id;
leaving it untouched keeps the exact pre-S5 3-argument push() call.
A targeted miss (delivered_sessions === 0) toasts "that session is
gone — refresh the list", drops the selection back to broadcast, and
re-polls presence instead of closing — zero delivery means nothing
was sent, so nothing is duplicated by resending.
* server: bound target_session_id, skip publish on a targeted miss
Codex round 1 fixes for TASK-2588:
- Cap target_session_id at 256 runes (400 over-cap) so an authenticated
caller can't park arbitrary garbage in the bus's shared replay buffer;
a registry-issued id (36 runes) can never hit this bound.
- Snapshot presence BEFORE publish instead of counting after: the old
order raced a target disconnecting between publish and count, which
could report delivered_sessions=0 on a push that had already landed
once. A targeted push now skips the publish entirely when its id
isn't in the pre-publish snapshot — session ids are per-connection
and never reused, so a target absent now can never be matched later,
making the 0 a guarantee rather than a race. Broadcast is unaffected
(still publish-always, pre-publish count).
Strengthened the targeted-miss and cross-user tests to assert the bus
does not grow (not just that the notification fails to arrive
downstream) — verified this fails if the skip-on-miss guard is
reverted.
* push targeting: document the pushed ruling, fix stale picker selection, guard mixed-version responses
Codex round 2 dispositions for TASK-2588:
- pushed:true on a skipped publish is RULED, not a bug (dispatcher):
moved the ruling from a test comment onto the contract itself —
pushResponse.Pushed's own doc comment in Go, mirrored in the TS
ItemPushResult doc comment.
- Fixed a real sharp edge: when a presence refresh drops the selected
session, a <select> can visually fall back to "All connected
sessions" while the bound value stays the stale id, so the wire
would carry a dead target the UI no longer shows as selected.
Added reconcileSelectedSession(), called at every point `sessions`
is reassigned outside the fresh-open reset (a live poll, a failed
read, and the staleness-expiry path).
- Guarded the mixed-version hazard with a cheap check, not capability
negotiation (the deployment shape — web assets embedded in the
server binary — bounds this to a transient stale tab, argument
recorded in the comment): delivered_sessions is now optional on the
wire type, and a targeted send whose response omits it entirely is
treated as UNKNOWN (info toast, dismiss like a normal success) —
never inferred as a confirmed miss.
Verified all three new/changed legs actually catch their regression
by temporarily reverting each fix and confirming the corresponding
test fails, then restoring.
* push targeting: fix stale publish-guarantee comments (codex round 3)
Two doc-comment remnants of round 2's skip-on-miss fix, both claiming
push unconditionally publishes:
- watchevents.KindPush's doc comment ("publishes exactly one of
these") now notes handlePushToItem decides whether to publish at
all, and points at TargetSessionID / pushResponse.DeliveredSessions
for why.
- api.items.push()'s JSDoc in client.ts no longer claims a resolved
promise means "published to the bus" unconditionally — a targeted
miss resolves with delivered_sessions: 0 and nothing published.
Comment-only; no behavior change.
|
||
|
xarmian
|
d7da237198 |
feat(mcp,cli): clear_assigned_user / clear_agent_role — a discoverable unassign (IDEA-2584) (#1107)
* feat(mcp,cli): clear_assigned_user / clear_agent_role — a discoverable unassign (IDEA-2584)
v0.16 and v0.17 made unassigning WORK. Nothing advertised it. The params
that do it — `assigned_user_id` / `agent_role_id` — were never in the
catalog, so an agent reading the tool schema to find out how saw only
`assign` (a name) and reached for `assign: ""`, which is a no-op and
deliberately stays one. The capability existed with no name an agent
could find.
`clear_assigned_user` / `clear_agent_role` booleans on `pad_item`, backed
by new `--clear-assigned-user` / `--clear-agent-role` bareword flags on
`pad item update`.
WHY BOOLEANS rather than declaring the existing string params. Two
reasons, and the second decided it:
1. An empty DECLARED string is inert everywhere else on this tool
(title, content, comment, tags), so a client that pads optional
params with "" instead of omitting them is harmless today. Giving
one a destructive meaning would turn that same client into one that
silently unassigns every item it touches. A boolean carries its
meaning in its name and can't be tripped that way.
2. Only a boolean can REACH local stdio. BuildCLIArgs emits the CLI's
real flags, so a catalog param with no flag behind it is dropped
before dispatch — declaring `assigned_user_id` would have left the
direct form remote-only, i.e. would not have closed the gap this
change exists to close. That fact reframed the design fork and is
what the ruling turned on.
Server-side this is WIRING, not new semantics:
models.ItemUpdate.ClearAssignedUser / ClearAgentRole already existed and
the store has honoured them since BUG-2566, on the same branch as the
empty-string form. The older forms keep working and are NOT deprecated;
they're just not what the schema advertises.
UPDATE ONLY, deliberately asymmetric with create, and recorded in-place
at both the flag registration and the catalog description so a
symmetry-minded reader meets the reasoning before the "fix": clearing at
create is a request to not-set something never set, whose only honest
behaviour is a no-op — it teaches a wrong affordance and pads every
create call's schema. A test fails if someone adds them there.
CLI precedence is the OPPOSITE of the --field lift's, deliberately: an
explicit `--clear-assigned-user` beats `--assign`, because that
combination is a contradiction the user typed and the reading that
cannot silently assign somebody is the safer one. Tested.
The dispatcher forwards the booleans VERBATIM rather than only-when-true.
A `&& b` guard would read as the thing protecting a param-padding client
and would be lying: what makes `false` inert is the store. Same call I
made on #1106's `len(patch) > 0` — a guard that reads as load-bearing
while doing nothing is worse than none.
ToolSurfaceVersion 0.17 -> 0.18, ADDITIVE bump per the v0.5 / v0.6
precedent: no existing tool, action or param changed shape.
Consumed artifacts moved in the same commit, which is the whole point of
this change — the schema IS the deliverable: catalog_item.go (the schema
agents read, plus an `assign` description that now says where to find the
clear), instructions.md (leads with the boolean, mentions the older forms
as still-working), version.go, README, CLAUDE.md.
VERIFIED LIVE, five legs, both transports:
CLI --clear-assigned-user -> assigned=None, role intact
CLI --clear-agent-role -> role=None
stdio clear_assigned_user:false -> assignment SURVIVES and the
update still applied (title
changed) — the control that
makes the boolean safe to
declare at all
stdio clear_assigned_user:true -> assigned=None
stdio clear_agent_role:true -> role=None
Three mutations, each failing only its own tests: dropping the dispatcher
forwarding; hardcoding true in the dispatcher (fails the false-control);
dropping the CLI flag wiring.
go test ./cmd/pad ./internal/mcp — pass. gofmt clean.
Closes IDEA-2584.
Claude-Session: https://claude.ai/code/session_01QCMLhHQBrMHVML3YKdm4Cd
* fix(mcp,cli): refuse a simultaneous set-and-clear (codex round 1)
Codex found a real bug, and the more useful half of the finding is that
MY OWN TEST FOR IT WAS VACUOUS.
The store's branch order is `if AssignedUserID != "" { set } else if
ClearAssignedUser { clear }`. So `--assign wren --clear-assigned-user`
assigned Wren and the clear evaporated. My in-place comment claimed the
opposite ("an explicit clear wins"), and the test I wrote to prove it
asserted `body["clear_assigned_user"] == true` — that the FLAG was set,
not that the item ended up unassigned. The flag was set. The behaviour
was backwards. A test that asserts a field is present says nothing about
which field wins.
Both surfaces now REFUSE the contradiction rather than silently resolving
it. Rejecting beats picking a winner here: the store already picks one
silently, which is the bug; and a caller who typed both wants to be told,
not guessed at. Precedent in the same command family — `item list`
already makes `--parent` and `--unparented` mutually exclusive.
PLACEMENT IS THE LOAD-BEARING PART, and I got it wrong first. There are
two routes to a competing value: `--assign` / `assigned_user_id`, which
resolve early, and `field: ["assigned_user_id=<uuid>"]`, which reaches
the payload via liftFieldsToColumns LATER. My first version checked
between them and its comment asserted the lift "has already" run — it
hadn't. That version rejects the direct case and lets the lifted case
through: a half-fix that reads as complete. The check now runs after
both, in the CLI after --assign/--role resolution and the lift, in the
dispatcher immediately before the body marshal.
That mutation is now a test: moving the dispatcher check back to the
pre-lift view fails ONLY the two `lifted …` subtests and passes the
direct one — the exact shape of the bug I nearly shipped.
Tests assert the OUTCOME, not the message: a refused conflict must leave
the item's assignment AND role untouched, and the CLI must issue no PATCH
at all. An error string alone wouldn't prove the write didn't happen.
Agent-facing text moved with it (the consumed-artifact step): both
catalog descriptions, instructions.md, and the v0.18 version entry now
say the combination is refused. An agent that pairs them gets a
structured refusal, so the schema has to say so.
go test ./cmd/pad ./internal/mcp — pass. gofmt clean.
Claude-Session: https://claude.ai/code/session_01QCMLhHQBrMHVML3YKdm4Cd
|
||
|
xarmian
|
847ee73327 |
fix(cli): lift assigned_user_id / agent_role_id onto their columns (BUG-2583) (#1106)
* fix(cli): lift assigned_user_id / agent_role_id onto their columns (BUG-2583)
`pad item update TASK-9 --field assigned_user_id=<uuid>` wrote the pair
into the item's FIELDS JSON BLOB while the column stayed stale, and then
printed "Updated TASK-9". Two defects in one line: a success message for
a write that did nothing the caller asked for, and a blob key shadowing a
real column's name, so the CLI surface diverged from store/HTTP/MCP
truth. The empty-string case was the same defect wearing a worse hat —
it was the only route an agent had to unassign an item.
Blast radius beyond the CLI: local stdio MCP (`pad mcp serve` — Claude
Desktop, Cursor, Windsurf) dispatches through ExecDispatcher, which
shells out to this CLI. So TASK-2571's fix reached the remote /mcp
transport only, and the transport most agents actually use still could
not unassign. This closes that half.
`cmd/pad/cmd_item.go` now lifts `columnFieldKeys` out of the --field map
onto the column pointers, on CREATE and UPDATE both, mirroring
internal/mcp/dispatch_http.go's liftFieldsToColumns — including its
INVARIANT, which is the part that matters: only keys with defined
clear-to-NULL semantics for "" belong in the list, and `tags` never does
(an empty write corrupts a JSONB column rather than clearing it). A test
fails if anyone adds it.
Two compat changes, ruled separately by the lead:
Q1 non-empty values move to the COLUMN and stop writing the blob key.
Accepted: relying on the old behaviour is relying on a shadowing
defect.
Q2 empty values clear the column. Falls out of the lift, inheriting
BUG-2566's store semantics.
`agent_role_id` gets identical treatment. Existing stray blob keys are
left alone per the ruling — this stops minting new ones; a sweep would
be its own change.
Precedence is explicit and tested: `--assign` / `--role` win over a
lifted --field value, matching liftFieldsToColumns' "caller-supplied
top-level values win". It is delivered by the ORDER of two blocks in the
command, which is exactly the kind of thing that gets reordered by
accident, so there is a test whose only job is to fail when it does.
A non-string --field value is deliberately NOT lifted: a collection that
genuinely declares a field with one of these names makes parseFieldFlag
return a typed value, which cannot address a column. It stays in the
blob — today's behaviour and the only lossless option.
ToolSurfaceVersion 0.16 -> 0.17, and v0.16's transport-scope paragraph
now points forward rather than claiming a limitation that no longer
holds. Behaviour-only bump again, same grounds as v0.16 and v0.9. The
CLI's own marker, CmdhelpVersion, deliberately does NOT move: its
contract is flag/arg SCHEMAS, and no flag or argument changed shape.
instructions.md — the text agents receive at handshake — drops the
"remote only" caveat it carried since TASK-2571. That file is the reason
this PR exists in the shape it does: it is the artifact the actor reads,
and it was the one place the previous PR overclaimed.
VERIFIED LIVE against a running server, with a negative control, because
the claim is about a transport rather than a function:
legs, fixed binary
--field assigned_user_id= -> column CLEARED, blob clean
--field assigned_user_id=<uuid> -> column SET, blob clean
--field agent_role_id= / <uuid> -> same, sibling column untouched
stdio MCP tools/call pad_item
action=update field=["assigned_user_id="]
-> column CLEARED, blob clean
control, PRE-FIX binary, same server + same item + same JSON-RPC bytes
-> column UNCHANGED, blob polluted
with {"assigned_user_id":""}
Six unit tests in cmd/pad/item_column_fields_test.go, four mutations each
failing only its own test (no lift; drop non-strings; flip the
lift/assign precedence; add `tags` to the list). One assertion was
rewritten after mutation testing showed it was VACUOUS: `len(fields_patch)
!= 0` passes whether the key is absent or present-and-empty, so it now
asserts key PRESENCE — confirmed by mutating `omitempty` off the model
field and watching the old form stay green. The redundant `len(patch) > 0`
guard that assertion was meant to cover is gone too; `omitempty` already
does that job, and a guard that reads as load-bearing while doing nothing
is worse than no guard.
go test ./cmd/pad ./internal/mcp — pass. gofmt clean.
Claude-Session: https://claude.ai/code/session_01QCMLhHQBrMHVML3YKdm4Cd
* test(cli): cover the create half of the column lift (BUG-2583)
Codex came back CLEAN, but the review reminded me I'd changed `item
create` and only tested it through `liftColumnFields` directly — no test
asserted what create actually puts on the wire. That's the weaker half to
leave uncovered, not the stronger one: on update a wrong write contradicts
a visible prior value, while on create the column-named key is simply
baked into the blob at birth with nothing to contradict it.
The assertion has to parse rather than index, because ItemCreate.Fields is
a JSON-encoded STRING and not a nested object — a body["fields"]["…"]
lookup would have been vacuous in a way that looks fine.
Mutation-tested like the rest: neutralizing the create-side lift fails
this test and only this test.
Claude-Session: https://claude.ai/code/session_01QCMLhHQBrMHVML3YKdm4Cd
* docs(mcp): say WHICH form of the unassign works on which transport (codex round 2)
Codex round 2, and it is the same class of defect as the previous PR's
round 2 — an overclaim in the artifact agents actually read. My
instructions.md said "works on BOTH transports" of two forms that do not
behave the same:
field: ["assigned_user_id="] clears on BOTH transports
assigned_user_id: "" clears on REMOTE ONLY
The direct params are not declared in pad_item's schema. They reach the
remote mapper only by riding the verbatim input map; on stdio,
BuildCLIArgs drops unknown keys, so the call does nothing.
VERIFIED, not accepted on the reviewer's word, and the verification
corrected my own first reading. My initial probe appeared to show the
stdio call CORRUPTING the fields blob — but that blob key was leftover
state from the earlier pre-fix control leg, not something the probe
wrote. Re-run against a freshly created item, the two forms separate
cleanly:
before assigned=b6786b13... fields={priority,status}
after assigned_user_id:"" assigned=b6786b13... fields={priority,status} (clean no-op)
after field:["assigned_user_id="] assigned=None fields={priority,status} (cleared)
So the stdio behaviour of the direct param is a DROP, not a corruption —
worth stating precisely, because "it corrupts the blob" would have sent
the next reader hunting a bug that isn't there. (Identity-doc rule: a
guessed mechanism stated as the reason is a claim, not a hedge.)
instructions.md now leads with the form that works everywhere and names
the remote-only limitation of the other; version.go and CLAUDE.md say the
same. IDEA-2584 — declare the params properly — is the fix that would
collapse this distinction, and is now cited from all three.
Claude-Session: https://claude.ai/code/session_01QCMLhHQBrMHVML3YKdm4Cd
* fix(cli): don't lift a field the collection actually DECLARES (codex round 3)
Nothing reserves `assigned_user_id` or `agent_role_id` as field names, so a
collection may legally declare a field with one of those keys. For that
collection `--field assigned_user_id=foo` means the DECLARED field — and
the lift I just added would redirect it to the assignment column while
dropping the value the user set. Two wrongs from one line: the intended
write vanishes and an unintended one happens.
liftColumnFields is now schema-aware and never lifts a declared key. Cheap
to do here because both call sites already fetch the collection schema for
parseFieldFlag. The check is PER-KEY — an undeclared sibling still lifts,
so one collision doesn't disable the feature — and a schema-fetch failure
degrades toward lifting, matching how the rest of --field handling degrades.
This makes the CLI deliberately STRICTER than the MCP dispatcher it
otherwise mirrors. liftFieldsToColumns has the identical collision and
can't make the same check as written: it builds its fields map straight
from the tool input without fetching a schema. Filed as IDEA-2587 rather
than fixed here, because closing it costs a round-trip on a hot path while
the CLI fix was free — and recorded so the divergence is KNOWN, in the safe
direction, rather than something a later reader "fixes" by loosening the
CLI to match.
The old non-string branch stays as belt-and-braces: parseFieldFlag only
returns a non-string for a declared field, which the new check already
catches, but if that stops being true a non-string still can't address a
column.
Mutation-tested: ignoring the schema declaration fails the new test and
only that test.
Claude-Session: https://claude.ai/code/session_01QCMLhHQBrMHVML3YKdm4Cd
|
||
|
xarmian
|
ee05c58446 |
fix(mcp): let an agent clear an item assignment (TASK-2571) (#1104)
* fix(mcp): let an agent clear an item assignment (TASK-2571)
Two filters in the MCP dispatch path dropped an empty-string assignment
value before the request body was built, so an MCP agent had no way to
UNASSIGN an item — `assigned_user_id=""` was a silent no-op rather than a
clear or an error:
- mapItemUpdate's top-level pass-through (dispatch_http_advanced.go)
- liftFieldsToColumns (dispatch_http.go), which lifts `--field` entries
onto their columns. This is the path an agent actually reaches: the
catalog exposes `assign` (a name) and `field`, but no
`assigned_user_id` param, so `field: ["assigned_user_id="]` is the
only schema-visible way to ask.
Both were right when written — `""` had no defined meaning at the store
and bound an empty string into a FK column. BUG-2566 gave `""`
clear-to-NULL semantics for exactly these two columns and the HTTP
surface inherited it, which left MCP the odd surface out. Uniformity
restoration, not a new feature.
Compat posture ACCEPTED per the lead's ruling: a caller sending `""`
today gets a no-op, and after this gets a clear. That is the correct
reading of the input — nobody sends an empty assignment ID meaning
"leave it alone" — and the no-op is the surprising half. Option (b)'s
clear_assigned_user / clear_agent_role schema flags are deliberately
skipped as additive sugar.
The empty-string filter on `tags` three lines above STAYS (codex #547 r3
P2): `tags: ""` is not a clear, it is a corrupt write into a JSONB
column on Postgres and TEXT on SQLite. Same-looking guard, opposite
justification — the new test's control leg fails if someone "unifies"
them.
ToolSurfaceVersion 0.15 -> 0.16. No tool, action, or parameter shape
changed, so this is a BEHAVIOUR bump on the v0.9 precedent (which moved
for a return shape with an unchanged signature). Flagging it for the
lead as my call, not theirs — it is a one-line revert if they read the
contract differently.
TRANSPORT SCOPE, established live rather than assumed: this fixes the
REMOTE /mcp transport, where both filters lived. LOCAL STDIO MCP still
cannot clear, because ExecDispatcher shells out to the CLI and the CLI
has no unassign at all — `--assign`/`--role` skip on empty, and
`pad item update TASK-9 --field assigned_user_id=` writes
{"assigned_user_id":""} into the item's FIELDS BLOB while the column
stays set (observed against a running server). Separate defect, CLI-wide
blast radius, filed separately rather than riding along on a ruled-scope
PR. The version-history entry says so explicitly so the note can't be
read as covering it.
Tests: internal/mcp/dispatch_http_clear_assignment_test.go drives the
REAL server + store, not a recording handler — asserting the dispatcher
merely puts `""` in the payload would restate the fix rather than test
it. Three mutations, each failing only its own test: restoring the
top-level filter fails the two direct-param tests; restoring the lift
filter fails the --field test; removing the tags filter fails the
control leg.
go test ./internal/mcp ./internal/store ./internal/server — all pass.
Claude-Session: https://claude.ai/code/session_01QCMLhHQBrMHVML3YKdm4Cd
* docs(mcp): record why an empty `assign` alias still doesn't clear (codex round 1)
Codex's finding is REAL: the catalog exposes `assign` / `role`, not
`assigned_user_id` / `agent_role_id`, so an agent reading the schema will
reach for `assign: ""` to unassign and get a no-op. The fix as shipped
only covers the params an agent has to already know exist.
Its suggested remedy — map the empty aliases to a clear — is the riskier
of the two it lists, and I've deliberately not taken it.
`assign` is SCHEMA-DECLARED. Every other schema-declared string on this
mapper (title, content, comment, tags) follows one convention: empty
means NOT PROVIDED. An MCP client that fills declared optional params
with "" instead of omitting them is harmless today; making `assign: ""`
mean "clear" would turn that same client into one that silently
unassigns every item it touches — destructive, silent, and inconsistent
with the four params beside it. That is exactly why the same change IS
safe for `assigned_user_id`: an agent can only send it deliberately.
The remedy that closes the gap without that hazard is the other one
codex names — explicit clear_assigned_user / clear_agent_role params,
i.e. option (b) on TASK-2571, which the lead deferred as additive sugar.
This finding is new evidence for revisiting that, so it goes to the lead
as a decision rather than being taken unilaterally in a ruled-scope PR.
Adds the reasoning at both call sites and a test that pins the limit, so
a future "finish the job" edit fails a test and has to be a decision
rather than a drive-by. The MCP instructions already name the working
form meanwhile.
Claude-Session: https://claude.ai/code/session_01QCMLhHQBrMHVML3YKdm4Cd
* docs(mcp): scope the unassign instructions to the transport where it works (codex round 2)
Codex round 2, and it caught a defect in my own round-1 documentation
fix. instructions.md is the text sent to agents at handshake, and BOTH
transports serve the same string — so telling agents "pass
assigned_user_id: '' to unassign" was true on remote /mcp and a lie on
local stdio, where ExecDispatcher shells out to a CLI that has no
unassign path. I had scoped the claim carefully in version.go and the
commit message and then overclaimed in the one place agents actually
read.
The instructions now name the transport, say plainly that stdio ignores
the value, and tell the agent to verify rather than assume. An agent can
act on a conditional; it cannot act on a claim that is false half the
time.
Both gaps are now filed rather than merely described:
BUG-2583 — the CLI has no unassign at all, and `--field
assigned_user_id=` writes into the item's FIELDS BLOB
while the column stays set (verified live: fields became
{"assigned_user_id":"", ...} and the CLI printed
"Updated TASK-9"). This is what makes stdio MCP fail.
IDEA-2584 — the catalog exposes `assign` / `role`, not the ID params,
so an agent reading the schema still cannot discover the
clear. Reopens option (b) with codex's evidence.
version.go and CLAUDE.md now cite both refs, so the version-history
entry can't be read as covering more than it does.
Claude-Session: https://claude.ai/code/session_01QCMLhHQBrMHVML3YKdm4Cd
* docs(mcp): cite BUG-2583 / IDEA-2584 in the version history and CLAUDE.md
Follow-up to the previous commit: its scripted edits to version.go and
CLAUDE.md silently no-op'd (a gofmt rewrap moved the anchor text), so
only instructions.md actually changed. Caught by grepping for the refs
rather than trusting the commit.
Both files now name the two filed gaps, so the v0.16 entry cannot be
read as covering more than it does:
BUG-2583 — the CLI has no unassign, which is why local stdio MCP
still can't clear.
IDEA-2584 — the catalog exposes `assign` / `role`, not the ID params,
so the clear stays undiscoverable from the schema.
Claude-Session: https://claude.ai/code/session_01QCMLhHQBrMHVML3YKdm4Cd
|
||
|
xarmian
|
79b3220c61 |
test(server): drive the reval-fault test off the fault seam instead of a sleep (BUG-2570) (#1102)
* test(server): drive the reval-fault test off the fault seam instead of a sleep (BUG-2570) The reload-fault closure now narrows the member's access on faulting tick 1 and lifts the fault on faulting tick 2, so consecutive reload failures stop at exactly 2 — strictly below the clear-the-watch-set bound — and the green path carries no timing bet at any load. The 300ms sleep is gone; readiness is signaled by the tick sequence itself. Codex round 1 on this fix surfaced that regression DETECTION still has a window (a successful tick 3 masks a hypothetical reset-skipped-on- fault regression), so the interval is set to 500ms to give the revoked PATCH ~10x headroom over measured loaded-runner request latency, and the control-leg wait — the one that timed out in both CI instances — is widened to 10s since it asserts delivery-at-all, not latency. Verified: 5x -race green at both 50ms and 500ms; counterfactual mutant (reset moved to the reload success path) leaks 3/3; full suite + lint green; Postgres leg 2x -race green. Claude-Session: https://claude.ai/code/session_01BhQoeaWXxJbvw86ezzK8dt * test(server): drive reval ticks through a seam — deterministic in both directions (BUG-2570) Codex rounds on the first fix found two regression-DETECTION windows the interval-tuned shape could not close: a stray successful tick before fault installation or after the tick-2 lift resets visCache / reloads the watch list, masking the reset-skipped-on-fault regression this test exists to catch. Interval tuning trades green-determinism against detection-determinism; a free-running ticker cannot give both. So the handler gains watchRevalTickOverride — a test seam mirroring watchPredicatesLoadFault (atomic pointer, read once at stream setup) that lets a test substitute the reval tick source. The test now drives exactly ONE tick, after the access change, with the reload fault active: no early tick can mask via a pre-fault reset, no late tick can mask via a post-lift reload, and one faulting tick can never reach the clear-the-watch-set bound. No sleeps, no interval mutation, no wall- clock bets in either direction. Claude-Session: https://claude.ai/code/session_01BhQoeaWXxJbvw86ezzK8dt |
||
|
xarmian
|
e03ba45b5c |
feat(web): push-to-agent composer in the item view (TASK-2561) (#1099)
* feat(web): push-to-agent composer in the item view (TASK-2561)
PLAN-2558 S3 — the web half of IDEA-2544's push-to-harness. Adds
`api.items.push`, a new `api.sessions.list`, and a "Push to agent…" row
in the item pane's ⋯ menu that opens a small composer.
The deliverable is the presence line, not the textarea. `pad push` is
fire-and-forget — no durable inbox, no ack, no "nobody was listening"
warning — which is defensible for a CLI verb typed by someone who knows
their own session is running, and indefensible for a button. So the
dialog answers "is anything listening?" before the click, and keeps
three states apart rather than two:
N > 0 send, worded "N session connected", never "will be
delivered" — the registry can name a session that died up
to ~30s ago and no push gets a receipt
N == 0 send DISABLED. Nothing listening means the message is
lost, not queued; the empty state offers the clipboard
instead (the fallback S4 rules for quick actions)
can't tell send ENABLED, uncertainty stated. A 503/401/network
failure is not zero — rendering it as zero is the exact
lie handleListSessions returns 503 rather than an empty
list to avoid
The menu row is gated on a resolved user, not on canEdit: push is
self-addressed, so a viewer pushing an item into their own session is a
read. Without a user the endpoint 401s.
$lib/push/message mirrors the server's rune-after-collapse accounting so
an over-length message is caught in the composer instead of coming back
as a 400. It deliberately does not use JS `\s`: Go's unicode.IsSpace and
`\s` disagree in both directions (U+0085 is whitespace to Go only,
U+FEFF to JS only), so a `\s` client under-counts a pasted BOM and
over-counts a pasted NEL. The agreement is pinned by a shared fixture
(internal/server/testdata/push_message_cases.json) read by BOTH
internal/server/push_message_collapse_test.go and the web unit test — a
TS-only table would assert a belief about Go rather than Go's behaviour.
Claude-Session: https://claude.ai/code/session_01QCMLhHQBrMHVML3YKdm4Cd
* fix(web): close the push composer's races and ambiguity gaps (codex review)
Round-1 review findings on the S3 composer, all real:
- ItemDetail did not reset `pushDialogOpen` on an item switch. The dialog
is {#key itemSlug}-remounted while `open` is owned by the parent, so a
stale `true` silently REOPENED the composer pointed at the new item.
The reset block's existing comment (written for copyDialogOpen)
describes this exact failure. Verified live, with the counterfactual:
reverting the one-line fix reopens the dialog on item B after a
client-side navigation. (The typed draft does NOT carry over — the
{#key} remount clears it — so the defect is the silent reopen, not a
retargeted message.)
- Presence polls shared one generation counter, which fences OPENINGS,
not requests. A stalled poll could resolve after a later one and
overwrite a fresh count with a stale one, re-arming Push against a
session list already known to be empty. Added a per-request sequence;
only a strictly newer response is applied.
- Nothing bounded a `/sessions` read, and 'checking' disables Push, so a
request that never settled stranded the composer with a dead button and
no explanation. It now degrades to the honest "can't tell" state after
5s; a later response still lands and upgrades the answer.
- A failed send re-armed Push unconditionally. The handler publishes
BEFORE writing its response, so an unstructured failure (rejected
fetch, non-JSON 502) leaves the outcome genuinely unknown and a second
click can deliver the instruction twice on an endpoint with no
idempotency key. Split on the same line CopyItemDialog draws (DR-13):
a structured PadApiError means the server refused before publishing —
re-arm; anything else latches an outcome-unknown state.
- `willCollapse` compared against `String.trim()`, reintroducing the very
JS-vs-Go whitespace mismatch $lib/push/message exists to avoid (JS
trims a leading U+FEFF the server keeps; it leaves a U+0085 the server
strips). Added `trimPushMessage`, which trims with Go's class.
- The textarea described only the counter, so the collapse note and the
over-length error reached no screen reader. Both now live in one stable
referenced node that swaps text rather than mounting and unmounting —
an aria-describedby pointing at an absent id resolves to nothing.
- Positive presence wording implied the count was current. It now says
"as of the last check" and names the ~30s window.
Test changes: the Go fixture test duplicated `strings.Fields` rather than
invoking the handler, so a change to the handler's normalization would
have left BOTH suites green — demonstrated by mutating the join
separator, which the copied-expression test did not notice and the new
handler-driven test caught on 22 cases. The bound is likewise now
asserted through the endpoint at 4096/4097 instead of comparing the
constant to a copy of itself.
Claude-Session: https://claude.ai/code/session_01QCMLhHQBrMHVML3YKdm4Cd
* fix(web): fence the push composer against destroyed instances, unrecognised errors, and a frozen count (codex round 2)
Three findings, one of them introduced by round 1's own fix:
- The send/copy continuation fence used the generation counter, which
cannot see a keyed REMOUNT. `{#key itemSlug}` gives item B a fresh
instance with its own counter, so item A's in-flight send still saw its
own `gen` unchanged and called the SHARED parent `onclose` — closing the
composer the user had just opened for B. Added a per-instance
`destroyed` flag, which is what actually distinguishes "still mine to
close" from "I no longer exist".
- The outcome-unknown split treated any PadApiError as proof the server
refused before publishing. It isn't: the API client turns EVERY JSON
error envelope into one, including a gateway 5xx invented after the
handler published. Replaced with a whitelist of codes the handler and
its middleware actually emit pre-publish; everything unrecognised is
now ambiguous. The asymmetry is deliberate — an unnecessary "we can't
tell" costs the user a check, a wrong re-arm delivers twice.
- PRESENCE_STALL_MS only rescued the FIRST read. A later poll that hung
froze the count at its last value indefinitely while the UI kept
rendering "1 session connected" as fact. A known answer now expires to
"can't tell" after 30s without a refresh — the server's own presence
staleness bound, so past it our answer carries no more authority.
Also dropped the status→alert role swap on the composer's live region:
changing a live region's role and its text together is not reliably
honoured, so the escalation was a promise the markup couldn't keep. The
blocking condition rides `aria-invalid` on the textarea instead.
The "latest ARRIVED, not latest ISSUED" behaviour of the sequence fence
is kept and now documented as a choice: dropping an early-arriving
response because a newer request exists strands the UI when that newer
request is the one that never settles.
Each fix mutation-tested 1:1 against its new test.
Claude-Session: https://claude.ai/code/session_01QCMLhHQBrMHVML3YKdm4Cd
* fix(web): complete the pre-publish whitelist and retire in-flight polls on expiry (codex round 3)
Two of round 3's three findings were real:
- `csrf_error` and `email_not_verified` are middleware refusals, written
strictly before the handler runs, so they belong in
PRE_PUBLISH_ERROR_CODES. Without them a CSRF mismatch told the user we
couldn't tell whether their message was sent, when nothing had been.
- The 30s staleness expiry didn't fence requests already in flight. A
poll issued before the expiry could land after it and reinstate the
very count we had just declared too old to trust. Expiry now advances
`presenceAppliedSeq` to the current `presenceSeq`, retiring those
responses; the poll issued in the same tick carries a newer seq and
still applies.
The third finding — that `archived` belongs in the whitelist, and that
the launcher should be hidden for archived items because "the endpoint
always rejects them" — is REFUTED. handlePushToItem has no archived gate
(`requireItemVisible` admits archived items), and pushing to an archived
item against a running server returns 200 with `pushed: true`. There is
no `archived` error code on this path to whitelist, and hiding the
launcher would remove a capability that works. Recorded rather than
silently skipped so the next reader doesn't re-derive it.
Claude-Session: https://claude.ai/code/session_01QCMLhHQBrMHVML3YKdm4Cd
|
||
|
xarmian
|
7943234dd7 |
fix(store): treat empty assignment IDs as clear-to-NULL (BUG-2566) (#1098)
* fix(store): treat empty assignment IDs as clear-to-NULL (BUG-2566)
PATCH with {"assigned_user_id": ""} 500'd with a raw FK constraint
error and left the item assigned. An explicit empty string is what a
JSON client sends when a user blanks the field (JSON null on a *string
decodes to nil = "don't change", so it can't express clearing), and
validateAssignmentScope already skips "" as nothing-to-validate — the
SQL builder just never learned the same convention and bound it
verbatim into the FK column.
Store-level fix so every surface (HTTP, bulk, MCP, CLI) inherits it:
"" now clears assigned_user_id / agent_role_id exactly like
ClearAssignedUser / ClearAgentRole on update, and binds NULL on
create. The mutation signal keeps the ClearAssignedUser shape (tested)
so watch notifications are unaffected. parent_id deliberately NOT
given the same coercion: parent relations also live in item_links, and
clearing the column alone would desync them.
Web UI is unaffected either way — it already sends
clear_assigned_user: true; only API clients hit this.
Tests reproduce the exact FK failure pre-fix (verified by stash-run)
and pass on both SQLite and PostgreSQL post-fix.
Claude-Session: https://claude.ai/code/session_01BhQoeaWXxJbvw86ezzK8dt
* fix(store): relocate nullIfEmptyID out of createItemTx's doc comment
Codex round 2 P3: the helper was inserted between createItemTx's doc
block and the function, orphaning the doc comment.
Claude-Session: https://claude.ai/code/session_01BhQoeaWXxJbvw86ezzK8dt
|
||
|
David Barkhausen
|
b9381bf5f1 |
feat(cli): markdown output on the remaining list surfaces; broaden ANSI stripping (#1080)
Completes #898 and fixes #1076. Markdown on the seven surfaces left out of #1070, so `--format markdown` is now honestly global and the flag help collapses to "table, json, markdown": - `item comments`, `item deps`, `project activity`, `attachment list`, `library list`, `role list`, `workspace members`. Two of those are not tabular, and markdown follows the terminal shape rather than forcing a table onto them: - `item comments` keeps the attribution-line-then-body form, and the body is emitted VERBATIM. A comment body is authored as markdown; escaping it would turn its lists and code fences into literal text. Only the attribution line, which we construct, is sanitized. - `item deps` keeps its two sections as `## Blocks` / `## Blocked by` lists. Colour carried the direction in the terminal (yellow out, red in); headings carry it here. New shared spine: `cli.RenderMarkdownTable(w, headers, rows)`. Every cell is escaped, and ragged rows are padded or truncated to the header width so a short or long row can't shift the column count and break the table. Wiring a surface is now naming columns and mapping rows. #1076 — ANSI stripping covered only SGR (`ESC[…m`), so non-SGR CSI sequences, OSC-8 hyperlinks, and stray C0 controls survived, both in the table width maths and in markdown output whose doc comment promised escape-free text. Replaced `sgrPattern` with `ansiPattern` + `stripANSI` covering OSC, CSI, two-character Fe escapes, and stray C0/DEL, with TAB/LF/CR deliberately preserved for callers that normalize them. `displayWidth` now uses it too: a control sequence is zero-width, so counting it was a column-alignment bug of the same family. Tests: 12 stripping cases, 4 table-helper cases (including ragged rows), 4 renderer cases for the two non-tabular surfaces, and the routing test extended to 8 subtests — one per surface, driven through cobra against an httptest server. Also covers the two gaps named in #1076: `item starred` and the scoped `item list <collection>` path. Each new guard was proven by mutating the source and watching it fail, not just by passing. Gates: go build ./... PASS; go vet PASS; gofmt clean; golangci-lint 0 issues. Both touched packages show the same 6+2 pre-existing Windows failures as clean main under an identical sandboxed run. |
||
|
xarmian
|
c84cf7437c |
feat(sessions): announce session identity on the event stream (PLAN-2558 S2, TASK-2560) (#1094)
* feat(sessions): announce session identity on the event stream (TASK-2560) PLAN-2558 S2. S1 gave the presence registry a count of anonymous uuids; this makes each row nameable, which is what S3 needs for an honest empty state and S5 needs for a target picker. A monitor now announces itself when it opens the stream: X-Pad-Session-Label (the working directory's basename) and X-Pad-Session-Pid. The server sanitizes both and stores them on the LiveSession; GET /api/v1/sessions returns them. TRANSPORT. The task body sketched "the stream connect carries it" without picking a mechanism and explicitly left the call open. Headers, because a query param would put the label and pid into every access-log line (this server logs path= for each request) and any proxy log in front of it — which is the same "don't let local detail travel further than it needs to" the privacy line below is about — and a separate registration POST would need its own correlation to the connection it describes, plus a matching lifecycle, when the registry entry already lives and dies with the stream. Headers ride the request that exists and sit alongside Last-Event-ID, already doing this job on this endpoint. Cost, written into the code rather than discovered later: a browser EventSource cannot set headers, so a future web-tab consumer needs a deliberate query-param fallback or a fetch-based SSE reader. PRIVACY. The basename crosses, never the full cwd — "/home/dave/Dev/ docapp" additionally hands over a home directory and usually an account name for no gain — and messaging_socket_path never leaves the machine. Pinned by a test rather than by the implementation being one line. WHAT THIS DELIBERATELY DOESN'T DO: read ~/.pad/sessions/. The task framed S2 as giving `pad session register` its first consumer, and the monitor cannot honestly be one. Registry entries are written by whatever process ran that command — a different pid — and the only matchable fields are pid and cwd, so two agent sessions in one checkout are indistinguishable and "pick the newest" is a coin flip that would put a confident wrong name in the S5 picker. Process ancestry settles it exactly and is platform-specific (this binary ships for macOS and Windows). The monitor's own cwd basename and pid are never wrong and answer the question the label exists to answer; correlating a stream to the agent session that spawned it needs an identifier the harness passes down, which is worth doing when something needs it and worth not faking until then. Also moves S1's STALENESS doc block, which sat above LiveSession.Label where it read as documenting the name rather than the whole entry. Tests: sanitizer units (whitespace collapse, control-char stripping, rune-not-byte truncation), header wiring, the end-to-end labelled session, the unannounced-client compatibility leg (a pre-S2 monitor must still register and still stream), a hostile-input leg over the wire, the client's omit-when-unset behaviour, and the basename promise. Measured rather than assumed: Go's server answers 400 to a header value containing a control byte before any handler runs (verified with a raw socket, since Go's own client refuses to send one and the two refusals are indistinguishable from a normal client test). So that arm of the sanitizer is unreachable over HTTP; it stays as defence in depth for the next caller in, and both the comment and the wire test say so instead of the test quietly passing because the transport refused the input. Mutation-tested four ways, each revert grep-verified: handler ignoring the parsed identity, monitor sending the full cwd, dropping the truncation, and the client always setting the headers. Refs TASK-2560, PLAN-2558 * fix(cli): sanitize the session label client-side per Codex review (round 1) Codex round 1's only finding, and it is a bigger deal than a missing label. Unix directory names may contain control bytes — "doc\napp" is a legal directory — and Go's http.Client REFUSES to send a request whose header value holds one: Do returns "invalid header field value" and nothing is transmitted. In the monitor that is indistinguishable from an unreachable padd, so the retry loop backs off and tries again, forever, printing nothing by contract. A user who named a directory that way would simply stop receiving notifications, with no signal anywhere. The server cannot defend against a request that never arrives. Reproduced before fixing, with a real directory and a real client, rather than reasoned about from the error message. Sanitizing in NewWatchEventsStreamRequest rather than in monitorSessionIdentity: the invariant is "this function never builds an unsendable request", which belongs at the point where a value becomes a header, not at one caller. The client's cap (256 runes) is deliberately looser than and independent of the server's (64): the server decides what a label should look like, the client only has to keep the request sane, and neither has to track the other to stay correct. The regression test does the ROUND TRIP instead of inspecting the header, because the header contents were never the bug — http.Header.Set stores anything, so an assertion on the value passes against the broken version too. Only attempting the request tells the two apart. Mutation-verified: reverting the sanitizer fails the test with exactly the "invalid header field value" error from the field report. |
||
|
xarmian
|
599fdbd3f4 |
feat(watch): drop assignment from the addressed-to-you stream (IDEA-2544 Phase 2, TASK-2551) (#1092)
* feat(watch): drop assignment from the addressed-to-you stream (TASK-2551) IDEA-2544 Phase 2. Assignment is bookkeeping (who owns this); push is dispatch (where attention goes now). Conflating them meant one triage session assigning N items sprayed N notifications into every open session of the assignee, so Dave's product call (day-33) was to drop assignment from addressed-to-you entirely — no opt-in flag, no config key. watchNotificationVisible loses its KindAssignment early-return; an assignment notification now falls through to the watch-map check like any other item-level fact, which is what an unconditional watch already promises to deliver. Producers are untouched and AssignedUserID is still populated, so a future opt-in re-addressing would be a consumer-side change only. KindPush is now the only addressed kind. Tests: six tests rode the deleted path and are reworked, not deleted. The two mid-stream visibility tests needed new vehicles — the persistent-reload-failure test uses a push (same watch-map-independent property), and the reval-ordering test uses collection-access revocation with a still-granted control item, since push is self-addressed only and its subject is a user losing access. That test's reval interval goes 50ms -> 200ms: at 50ms the clear-the-watch-set bound landed ~30ms behind the assertion and the control leg lost the race. New coverage for the asymmetry the change creates: a push stays exclusive of watch-matched delivery, an assignment does not — a watcher is entitled to see who an item was assigned to. Mutation-tested three ways (restore the old branch; make assignment exclusive addressed-only; couple visCache.reset() to reload success); each is caught by the intended test and each revert was grep-verified. Live: assigning a fresh unwatched item to the connected user leaves the plugin monitor silent, pushing the same item prints one line, and assigning a WATCHED item still delivers — verified end to end against a sandboxed server, not just in tests. Refs TASK-2551, IDEA-2544 * docs(watch): note the deferred plugin wording per Codex review (round 1) Codex's only finding: plugin/monitors/monitors.json and plugin/skills/pad/SKILL.md still describe assignment as addressed-to-you traffic. Correct observation, deliberately out of scope — installed plugins are version-pinned at install, so plugin-visible text reaches nobody without a version bump, and TASK-2564 (PLAN-2558 S6) owns the wording and the bump together. Recording it in code next to the deleted branch rather than leaving a reader to discover the mismatch, and on TASK-2564 with the exact line refs so the follow-up does not have to re-find them. |
||
|
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
|
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
|
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
|
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.
|
||
|
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
|
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
|
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
|
417929c5a4 |
Merge pull request #1037 from jairbj/feat/nix-flake-packaging
feat(nix): add flake packaging with CI build |
||
|
xarmian
|
bda6987125 |
Merge pull request #1058 from danfinn5/fix/cloud-mode-error-messages
fix(cli): surface actionable errors for cloud-mode setup failures |
||
|
Dan Finn
|
20b061902b |
fix(cli): surface actionable errors for cloud-mode setup failures
When a user picks Cloud mode during `pad init` but doesn't have a Pad Cloud account, the CLI hits the cloud server and surfaces raw server errors like "Missing CSRF token" — which is an implementation detail that gives no indication of what went wrong or how to fix it. This patch: - Splits the ModeCloud and ModeRemote branches in printSetupRequiredHint so cloud users see "sign up or switch to local" instead of the generic "run pad auth setup on the server" message. - Returns a cloud-specific error from pad init when setup_required is true in cloud mode. - Intercepts csrf_error responses in the CLI HTTP client and replaces the raw server message with an actionable "run pad auth login" message, since the CLI never sends CSRF cookies and this error always indicates a stale or mismatched session. |
||
|
xarmian
|
6f8105b01d |
refactor(attachments): consolidate icon helpers onto an SVG set (TASK-2417)
Replaces the three independent emoji icon helpers on the live attachment surfaces with one mapper and one monochrome SVG icon set (PLAN-2392 DR-3, DR-3a, DR-3b). - display.ts: categoryIcon -> iconForAttachment(mime, filename), returning an icon identifier rather than an emoji. MIME first, filename extension second, generic file last -- never a question mark. isImage and formatBytes keep their signatures; StorageTab imports all three. - attachments/icons/: one currentColor-driven icon per format family, with TWO render paths over one path table -- AttachmentIcon.svelte for Svelte call sites, iconSvg() for the editor chip, which builds DOM imperatively and cannot mount a component. - attachment-chip.ts: iconForMime, iconForFilename and its local formatBytes deleted. The call site keeps its hide-zero/unknown-size conditional; the shared formatter renders "0 B" and does not grow a mode (DR-3b). - mime-families.json: the shared MIME -> family map, inside the web root because vitest cannot read outside it. A Go test asserts the server upload allowlist is fully covered by it (and carries no strays), so the two lists cannot drift silently; the web test covers one representative MIME per family plus the unknown-MIME and no-extension cases. CopyItemDialog and markdown/attachments.ts are deliberately untouched. Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC |
||
|
xarmian
|
b90e7edaeb | docs(attachments): record the lock-held pool I/O hazard at the call site (BUG-2409) | ||
|
xarmian
|
e12feb46cb |
fix(copy): authorize attachment references in cross-workspace copy (TASK-2408)
Cross-workspace copy authorized the source item and the destination collection but never the individual attachments it cloned. PlanAttachmentCopy scoped every lookup to `workspace_id = SourceWorkspaceID AND deleted_at IS NULL` — but the workspace is not the caller, so a restricted member who could edit any item in the source workspace could paste `pad-attachment:<uuid>` for an attachment on an item they could not see, copy that item into a workspace they own, and read the bytes through the ordinary blob endpoint (BUG-2407). The planner now consults an AttachmentAuthorizer supplied by the caller, applied to every row it resolves: the referenced rows, the parents it adopts as clone roots, and the variants it follows. A denial DELETES the row from the resolution map, so it is indistinguishable from a row that was never there — the reference lands in UnresolvableRefs beside dangling, soft-deleted and foreign ids, and attachment_count / attachment_bytes / unresolvable_ref_count read identically. The preflight's numbers stay oracle-free. It is a callback because the rule is the read path's — resolve the parent, reject a foreign or non-live one, check item visibility, apply the orphan rule — and every input to it lives in package server. It cannot run BEFORE planning either: the copy re-reads the source content under its locks and computes destination fields inside its transaction, so a reference set enumerated beforehand is not the set the planner resolves. Authorizing the rows the planner actually resolved keeps the dry run and the copy on one path, which is the property DR-11 exists to protect. Both endpoints take the authorizer off the same shared resolution (resolveAuthorizedCopy), so what the preview calls unresolvable is what the copy refuses to clone. Mutation-verified: without the authorizer the secret PNG is cloned into the destination, referenced by the rewritten body, and served byte-identical to the attacker through the destination workspace. |
||
|
xarmian
|
2318a17e49 |
fix(attachments): classify derived rows after authorization on delete
Found by the convergence sweep of this branch, which enumerated every attachment-touching path and compared each against its siblings' gates. The delete handler answered 400 derived_attachment as soon as it saw a ParentID, before any visibility, restriction, role or edit gate. That 400 is reachable only for a row that exists and is live, so a guessed thumbnail UUID answered 400 while an absent, foreign, or deleted id answered the shared 404 — and a caller who could not see the parent, or was restricted out of its collection, learned about the row anyway. Fifth instance of this handler family's existence oracle. Moved after the authorization switch. The classification is a usage error, so it may only be reported to someone already entitled to act on the row; the test pins BOTH halves, so the fix cannot regress into blanket-404ing a legitimate mistake by an authorized caller. Mutation-verified: restoring the previous position makes the restricted caller receive 400 again. Gates: make check exit 0, make test-pg exit 0, zero failures. |
||
|
xarmian
|
ba848af85f |
fix(attachments): check restriction before the role gate on orphan delete
Found by the convergence review of this branch. The orphan branch of the
delete path called requireMinRole("editor") before
attachmentCallerIsRestricted, so a restricted member who guessed a live
orphan's UUID got 403 while a bad UUID got 404 — confirming the row
exists. Fourth instance of the same existence oracle on this branch, and
the one path whose gate ORDER the refactor did not re-check.
Notable because attachmentCallerIsRestricted's own contract, added in the
previous commit, states that callers must apply it ahead of any role gate
that would answer 403. Centralizing the invariant did not fix call-site
ordering; only re-reviewing did.
Test covers both restricted roles: a viewer and an editor answer
differently at the role gate (403 vs success), and NEITHER may be
distinguishable from the lookup miss. Mutation-verified — restoring the
previous order yields exactly "status = 403, want 404".
Gates: make check exit 0, make test-pg exit 0, zero failures.
|
||
|
xarmian
|
1da96106e8 |
refactor(attachments): centralize parent resolution, close orphan-read and delete-denial gaps
Per the final full-diff review of this branch. The six task commits each
added authorization to a different attachment path, and each was reviewed
CLEAN on its own — but they hand-rolled the same invariant four ways, and
the drift between them opened two real gaps that no per-task review could
see.
Root cause: the blob read, transform, thumbnail derivation and delete
paths each loaded the parent item, checked workspace identity and checked
liveness in their own shape. resolveAttachmentParentItem is now the one
place that invariant lives, returning a four-way outcome (orphan / ok /
gone / foreign) so callers keep their own denial behaviour — which is
deliberate, not accidental: the HTTP paths must not distinguish the
outcomes (any split is an existence oracle), derivation logs a distinct
WARN per outcome (greppable ahead of PLAN-2397's repair), and delete
passes includeArchived because the storage listing intentionally surfaces
archived-parent rows so their quota can be reclaimed.
Gaps the drift opened, both closed here:
- Orphan GET lacked the full-access gate transform and delete apply, so a
restricted member who guessed an orphan attachment's UUID could download
it — while transform, delete and the listing all refused. Now shared as
attachmentCallerIsRestricted, applied ahead of any role gate, since a
403 reached only for rows that exist is itself the oracle.
- The delete path still routed invisible parents through requireItemVisible
("Item not found") while missing and foreign attachments got "Attachment
not found" — the same existence oracle already closed twice on this
branch, left inconsistent on the one path the tasks did not touch. Every
delete denial now goes through the shared writer, asserted byte-identical.
Also folds in the live-parent write invariant on upload, which had been
applied to transform only: upload validated the item before spooling and
then inserted with plain CreateAttachment, so archiving during the upload
window bound a row to an archived parent. Derivation deliberately still
does NOT take the lock — that trade is documented on deriveThumbnails.
Gates: make check exit 0, make test-pg exit 0 (zero failures). Both new
guards mutation-verified; attachment authz suite clean under -race -count=2.
|
||
|
xarmian
|
9ad718178d |
fix(store): workspace-scope the item-grant lookup (TASK-2403)
ResolveUserPermission matched item grants on item_id alone, so a grant on an item in workspace B resolved for a request scoped to workspace A. This is the underlying lookup behind the delete escalation PLAN-2382 fixed at the handler; closing it here means the next caller does not have to remember the workspace-identity guard. The adjacent collection-grant lookup had the identical defect and the identical safety argument, so it is scoped in the same commit rather than leaving a second unscoped lookup three lines below the one DR-5 names. Safe for every caller: all three (requireEditPermission, the collab access check, crossWorkspaceEditAllowed) already pass the workspace the item/collection was resolved in, and grant rows carry the workspace they were minted in — the same scoping listUserItemGrants already uses. Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC |
||
|
xarmian
|
90eb871da3 |
fix(attachments): skip derivation for an archived parent (TASK-2404)
deriveThumbnails checked only that the parent ATTACHMENT row was live and then copied parent.ItemID verbatim into every derived row. After TASK-2401's read gate that is a waste with a cost: a variant of an archived item's attachment is quota-counted storage that the blob path (DR-13) refuses to serve, so the bytes are written, charged, and unreadable until the item is restored. The same holds for a malformed item_id — the column has no FK and no same-workspace constraint, so a row can name a foreign-workspace item or no item at all. Derivation now resolves the parent item at entry, before the blob is even opened, and skips when it is soft-deleted, unresolvable, or in another workspace. GetItem, not GetItemIncludeDeleted, so "live" means the same thing here as on the read path. Orphan rows (item_id NULL) have no item to check and still derive. This is internal background work with no HTTP response, so there is no 404 shape to match: it skips and logs a WARN alongside the existing decode/resize/persist skip logs, with the malformed cases carrying distinct messages so they are greppable ahead of PLAN-2397's repair. The post-check window is DELIBERATELY ACCEPTED, and the comment on thumbnailParentItemLive says so at length so the next reader does not file it as a bug. The check is point-in-time — item deletion commits in its own transaction and the read/decode/resize/encode/Put in between is unbounded work — so an item archived mid-flight can still get a variant. Transform (TASK-2402) closes its equivalent window with store.CreateAttachmentForLiveItem; derivation deliberately does NOT, and makes the opposite trade: transform is user-initiated and low-volume, whereas derivation is a background worker fanning out from every image upload, so an item lock here is disproportionate to the harm. What leaks through is a thumbnail — small, unreadable for as long as its item stays archived, and tombstoned by the delete cascade with its parent attachment. Tests cover the sequential cases only: already-archived (with a sanity check that DeleteItem really is a soft delete), unresolvable item_id, and a foreign-workspace item_id that resolution alone would accept. The raced case is deliberately not asserted — it is permitted behaviour, and pinning it either way would constrain what the design leaves free. Two controls keep the skips honest: a live parent and an orphan row must both still derive from the same fixture and the same bytes, so a fixture that stopped deriving at all would fail loudly rather than pass the skip assertions vacuously. All three skip tests were mutation-verified against a short-circuited guard, and the file passes -race -count=3 and make test-pg. Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC |
||
|
xarmian
|
380b75e12c |
fix(attachments): gate transform on item visibility (TASK-2402)
handleTransformAttachment opened with a flat requireMinRole("editor") and
never looked at the attachment's parent item at all. A restricted editor —
one whose collection access excludes that item — could transform an
attachment on an item they cannot see, given only the attachment id: the
handler read the source blob and returned output metadata plus a new row.
The output URL inherits ItemID and is gated by TASK-2401's read gate, so
this was not direct byte exfiltration, but it crossed the same boundary and
leaked processing behaviour and metadata for an invisible item.
The handler now authorizes per-attachment, in the order the read path uses
(PLAN-2391 DR-10): load the row -> workspace identity -> load the parent
with GetItem -> parent workspace identity -> checkItemVisible -> edit
permission -> transform. Every denial goes through writeAttachmentNotFound,
so a missing attachment, a foreign parent, a soft-deleted parent and an
invisible item are byte-identical; a distinguishable code or message would
be an existence oracle. Malformed non-null parents that resolve nowhere are
rejected by the same guard.
Edit permission is requireEditPermission rather than the flat editor role:
an item- or collection-grant editor can already attach to the item
(BUG-1661), so refusing them a rotate on their own upload would be an
inconsistency, not a boundary. Orphan rows keep the flat editor gate and,
matching the DELETE path (PLAN-2382 DR-4), require unrestricted workspace
access — the storage listing hides orphans from restricted members, so the
transform must not confirm one exists.
DR-14's race is closed, not narrowed. The parent check is point-in-time:
item deletion commits in its own transaction, and the blob read, decode,
transform, encode and Put in between are unbounded work, so the item can be
archived mid-flight and the insert then writes a quota-counted live row
against an archived item whose bytes DR-13 refuses to serve. The new
store.CreateAttachmentForLiveItem re-checks the parent under a row lock
inside the insert's own transaction: the row is written against a live item
or not written at all. FOR NO KEY UPDATE, not FOR UPDATE — DeleteItem's
UPDATE touches no key column so the archival still blocks, while the many
tables with a REFERENCES items(id) foreign key (comments, stars, the Yjs
op-log) keep taking FOR KEY SHARE on the parent uncontended. SQLite skips
the clause: _txlock=immediate already serializes writers there.
Tests fail against the pre-fix code: the restricted-editor transform
returns 404 with a body byte-identical to the missing-attachment body, and
the mid-flight test archives the item from inside the processor's Encode —
between the up-front check and the insert — asserting the hook actually ran
so it cannot pass vacuously. The Postgres lock test polls pg_stat_activity
until the statement is registered as lock-blocked rather than sleeping, and
watches the completion channel so a missing lock fails immediately. Both
were mutation-verified.
Recorded, not fixed here: a refused insert leaves a rowless blob on disk,
and the orphan GC is row-driven so nothing reclaims it. Pre-existing on the
upload and thumbnail paths too; filed as BUG-2406 with the dedupe guard a
correct fix needs. The comment claiming GC reclaims a transform's original
was wrong and is corrected — only an orphan original is GC-eligible.
Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC
|
||
|
xarmian
|
6e2b972fb0 |
fix(attachments): gate blob reads on item visibility (TASK-2401)
handleGetAttachment opened with a flat requireMinRole("viewer").
roleLevel("guest") is 0, below viewer's 1, so every grant-based guest
was rejected before any item-level check ran and inline images broke in
items shared with them (BUG-2386).
The handler now authorizes per-attachment, in the order PLAN-2391 DR-10
fixes: load the row -> verify the parent item's workspace identity ->
check item visibility -> serve. Orphan rows keep the flat viewer+ gate;
the workspace-wide storage listing is untouched.
Also closes two defects sitting immediately around that gate:
DR-16 - GetAttachmentVariant scoped on parent_id/variant/deleted_at but
not workspace_id, so a foreign-workspace variant sharing a parent id
would be served after the local parent was authorized. Fixed at the
store API rather than in the handler because the other caller,
thumbnail derivation, has its own stake in the scope: an unscoped
"does this variant exist?" probe lets a foreign row suppress generation
of a legitimate local one.
DR-13 - the parent is loaded with GetItem, so a soft-deleted parent
404s. The DELETE path keeps GetItemIncludeDeleted, unchanged.
Denial paths now carry Cache-Control: private, no-store, set as the
handler's first statement (writeError calls WriteHeader immediately, so
anything later never reaches the wire); the positive private,
max-age=3600 is set only after authorization succeeds. Every
authorization-dependent refusal goes through one writer so the
responses are byte-identical and can't be used as an existence oracle.
The MCP image resource pad://workspace/{ws}/attachments/{id} inherits
the gate; asserted against a real server rather than assumed.
Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC
|
||
|
xarmian
|
27b71fe4f6 |
fix(attachments): resolve item_id across both upload channels (TASK-2400)
The upload handler read item_id from two places with different rules: authorization resolved only the query-string value, while the association step fell back to the multipart-form value and persisted it verbatim. Since ResolveItem accepts a UUID, a ref, or a slug, a form-supplied ref or a foreign-workspace id could land in attachments.item_id unauthorized and unresolvable — the malformed-row invariant BUG-2387's cross-workspace leak rests on. Three coupled changes (PLAN-2391 DR-2): 1. One effective item_id. Each non-empty channel is resolved in the request workspace and the RESOLVED canonical ids are compared — not the caller's spelling, so query "TASK-12" + form "<uuid>" is agreement, not conflict. Absent and explicitly-empty both mean "no value" (compared after TrimSpace). item.ID is what gets persisted. The form value is read from r.MultipartForm.Value rather than r.FormValue, which merges the query string back in and would collapse the two channels into one. A channel that repeats item_id has every value resolved rather than first-wins, since net/http otherwise silently discards the rest; the value count per channel is capped, because exact-string dedup can't bound the lookups on its own (TASK-7 / task-7 / TASK-0007 resolve alike). 2. Auth ordering. The no-item workspace-editor gate is deferred until after multipart parsing; firing it pre-parse 403'd a form-only item-grant guest (the CLI's shape) before the association that authorizes them was read. The query channel is still resolved and authorized pre-parse so a doomed upload never spools. The route's auth/workspace-access middleware chain is unchanged. 3. Spool cleanup. file.Close() closes the spooled multipart temp file but never removes it; added r.MultipartForm.RemoveAll() on every exit path, including success, where it leaked today too. Status codes (the pinned contract): an item_id that does not resolve in the request workspace → 404 item_not_found on either channel, cross-workspace UUIDs included; two channels — or two values on one channel — that each resolve but to different items → 400 item_id_conflict. Folded in from review: each resolved item is gated on requireItemVisible (404) before the values are compared and before requireEditPermission (403). Without that, the status split is an existence oracle for items a restricted member or ungranted guest can't see — directly via 404-vs-403, or by pairing a visible id with the id being probed and reading 400-vs-404. It also closes requireEditPermission's editor/owner fast path, which never consults collection visibility, so a collection_access="specific" member could otherwise attach to an item in a collection hidden from them. Two intentional behaviour narrowings, both following from DR-2's "reject a non-empty value that does not resolve": an item_id for a soft-deleted item now 404s where a workspace editor previously got a 201 (ResolveItem is live-only) — consistent with DR-13/DR-14 keeping archived parents from accruing new bytes; and an unresolvable item_id no longer falls back to the flat editor gate and silently stores the caller's string. Tests: extends TestUpload_GrantBasedEditorCanAttach with the form-only and both-channel grant-guest cases, the ungranted-item 404, and the paired-probe oracle check; adds canonical-UUID persistence, 404/400 rejection with no row written, repeated conflicting values, the value-count cap, and a >1 MiB isolated-TMPDIR fixture for the spool (a tiny in-memory body never spills to disk, so it would pass either way). The auth-ordering and spool tests were mutation-checked against the pre-fix behaviour. Gates: make check (exit 0), make test-pg (exit 0). Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC |
||
|
xarmian
|
eae42b843e |
fix(store): scope attachment list JOINs by workspace (TASK-2399)
WorkspaceAttachments joined `items` (and, through it, `collections`) on item_id alone, so an attachment whose item_id points at another workspace's item borrowed that item's title, slug, and collection into the storage listing. Both queries — the count and the result — now join with `ON i.id = a.item_id AND i.workspace_id = a.workspace_id`. The predicate is deliberately in ON, not WHERE: in WHERE the LEFT JOIN degenerates into an inner join and the malformed row would vanish from the listing entirely, hiding a row that still consumes quota and that the PLAN-2397 repair has to be able to see. In ON the row survives with NULL item/collection metadata. Keeping the two queries in step matters — they are separate SQL and a restricted caller's count must not diverge from their rows. Review turned up a second hop of the same leak, folded in here: items.collection_id has no composite workspace foreign key, so a LOCAL item can reference a FOREIGN collection and surface its slug even through a scoped item join. The collections join now carries its own workspace predicate, same ON-clause rule. Two fixtures pin both hops, each verified by mutation to fail when its predicate is moved to WHERE or removed. PLAN-2391 DR-3. |
||
|
xarmian
|
e115bb255e |
feat(web): delete attachments from the item strip (TASK-2384)
Adds the first in-item delete path for an attachment (PLAN-2382 phase 2).
Before this the only surface was Settings > Storage, which is
workspace-wide and disconnected from the item you're looking at.
Server: handleDeleteWorkspaceAttachment no longer opens with a flat
requireMinRole("editor"). That gate contradicted the UI's grant-aware
canEdit (permissions.ts::canEditItem), which is true for a viewer holding
an item- or collection-level edit grant -- so that user saw the affordance
and got a 403, even though upload already admits them (BUG-1661).
Authorization is now per-attachment, mirroring the upload handler:
- item-bound: requireItemVisible THEN requireEditPermission. The order
is load-bearing -- an attachment on an item the caller can't see must
keep returning 404, not the 403 that would confirm it exists.
- orphans: unchanged flat editor-role gate plus the guest filter, since
there's no item context to authorize against.
UI: per-tile delete control, in the DOM unconditionally so it's keyboard
reachable (CSS reveals it on hover/focus-within). Gated on ItemDetail's
mutationsEnabled, not raw canEdit, so a peeking master stays a complete
read-only freeze. Optimistic removal with rollback + toast on failure,
fenced so a switch mid-delete can't resurrect A's tile under B.
The confirm warns when the id is referenced in this item's body, and
deliberately hedges otherwise -- comment bodies, other items' content and
fields JSON are not visible client-side, so it says "may still be
referenced" rather than claiming non-use.
Editor: the attachment-image NodeView assigned img.src with no error
path, so a delete left the browser's broken-image glyph until reload --
reading as a network blip for what is a permanent state. It now degrades
to the same .attachment-missing placeholder the markdown renderer uses,
re-armed on uuid swap so rotate/crop clears a stale placeholder.
Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC
|
||
|
xarmian
|
d9d96b85c9 | refactor(store): delete two unused item-workspace-move accessors (TASK-2374) | ||
|
xarmian
|
3bbd326857 | test(store): make the copy concurrency and attachment assertions bite (TASK-2372) | ||
|
xarmian
|
c6ebe5a3e3 |
refactor(store): unify the collection column list and scan (TASK-2368)
Three accessors read a full collection row and each carried a verbatim copy of the same 15-column projection and scan/hydration block: GetCollection, GetCollectionAnyState, and the transactional getCollectionInWorkspaceTx used by the cross-workspace copy. A column added to the model had to be added in three places, and the copy path drifted silently if only GetCollection was updated. Extract collectionColumns plus scanCollectionRow, parameterized over rowQueryer (the uniqueSlugQ / validateAssignmentScopeQ pattern from TASK-2362) so the same read runs against *sql.DB or inside a caller's *sql.Tx. Each accessor's full statement is assembled from constants, so the WHERE predicate is the only per-caller difference, the SQL is built at compile time rather than per call, and no runtime-assembled fragment is ever handed to s.q. Preserved deliberately: s.q placeholder rewriting (applied once, inside the helper, so no call site can skip it); nil-on-sql.ErrNoRows at every accessor -- the helper returns real errors unwrapped so each keeps its own distinct prefix; the transactional lookup stays workspace-scoped and active-only, which is the security boundary that makes a foreign collection a not-found rather than a cross-workspace write. lockCollectionRows is untouched: its SELECT id ... FOR UPDATE is a locking primitive that duplicates nothing, and its sorted acquisition is load-bearing. ListCollections is deliberately left out and documented as such: it is an aggregate multi-row query with aliased columns, a trailing COUNT and no deleted_at, so sharing a projection would need a second count-aware scanner and would reshape a hot query for no correctness gain. TestCollectionAccessorsShareOneHydration pins all three to one hydration. Every scanned column except deleted_at is asserted against a literal, distinct value rather than against another accessor's output, since cross-accessor equality alone cannot catch a mutation in the shared projection; created_at and updated_at are set to different instants so transposing them fails, and deleted_at is pinned by the soft-delete branch, the only state in which it is non-nil. Verified by mutation: a transposed slug/prefix projection, a transposed created_at/updated_at projection, a dropped workspace scope on the transactional read, a flattened deleted-state predicate, and a miss turned into an error each fail the test. |
||
|
xarmian
|
98c638fc86 | refactor(server): extract resolveAuthorizedCopy shared by preflight and copy (TASK-2370) | ||
|
xarmian
|
c783d36a13 |
fix(store): make migration 077 constraint-equivalent to 055 per final review
Postgres' BOOLEAN admits exactly two values; SQLite's bare INTEGER admits any. A stray 2 would scan as true through BoolToInt while the partial index the moved-to lookup uses is WHERE archived_source = 1 — a row that reads as a move but is invisible to the query that finds moves, which the Postgres schema cannot represent. Add the CHECK, and make id NOT NULL explicit since SQLite does not imply it for a TEXT PRIMARY KEY. Migration 077 is unreleased, so amending it in place is safe. The test is mutation-verified. Its first draft was NOT: it used placeholder ids and passed against a schema with no CHECK at all, because the foreign keys rejected the insert before the constraint under test was reached. It now uses real fixture rows and asserts the same row inserts cleanly with archived_source = 1. Found by the final full-diff Codex pass over PLAN-2357, data-at-rest angle. Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT |
||
|
xarmian
|
cfc83e8c57 |
fix(server): report partial and legacy relationships in the copy dry-run (TASK-2369)
Two ways the cross-workspace copy preflight told a user "nothing to lose" when there was, both violations of PLAN-2357 DR-17's "none of this may be silent". P1 — the five relationship counters are ACL-filtered by the caller's collection visibility (correct, and TASK-2364 chose it deliberately), but "none" and "none that you can see" rendered identically. A caller with edit rights on the source and none on its relatives could read `children_orphaned: false` and run a MOVE believing nothing was stranded, while hidden children were orphaned in place. The filtering stays; the uncertainty is now surfaced. Every point that drops a relationship for visibility reasons sets a new `warnings.relationships_partial` boolean. It is a BARE BOOLEAN by design: how many are hidden, of what type and in which collection are exactly the facts the filter exists to withhold, and a marker that varied with the hidden count would reinstate the leak DR-10a, DR-10b and the moved-to pointer each closed separately. A negative test asserts byte equality of the whole warnings block across two workspaces that differ only in how much is hidden. It is false for an unrestricted caller AND for a restricted caller with nothing hidden, so the common case renders exactly as it did before. P2 — a child reachable only by a lone legacy `plan` edge was invisible to GetChildItems (its join is restricted to store.ChildLinkTypes), so an incoming `plan` relationship reported `child_count: 0` / `children_orphaned: false` even though archiving the source strands it. The link scan now folds such an edge into the child set, deduplicated against the two mechanisms already covered and subject to the same visibility, liveness and workspace guards. The outgoing direction (the item's own parent) already reported correctly. The mutating copy reports no relationship counters at all (ItemCopyResultWarnings is deliberately narrower), so there is nothing for assertPreflightMatchesCopy to disagree about. CLI renders the qualifier on the five affected lines plus a plain-language explanation; TS types carry the field for Phase 3's dialog. Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT |
||
|
xarmian
|
66fa464699 |
fix(store): distinguish SQLite lock timeout from a real deadlock per final review
isDeadlockError matched SQLite's "database is locked" alongside Postgres' 40P01, and the rollback path logged both as deadlock=true at ERROR. But SQLite is single-writer with a 30-second busy timeout, so "database is locked" is an expected saturation mode under burst load — it says the box is busy. A 40P01 says DR-9's lock ordering, which is meant to make deadlock impossible, is wrong. Reporting both identically left an operator unable to tell a lock-ordering bug from ordinary load, defeating the only signal this log exists to carry. Split the predicates and add lock_timeout to the log line. Classification test is mutation-verified: reintroducing the conflation fails it. Found by the final full-diff Codex pass over PLAN-2357, operability angle. Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT |
||
|
xarmian
|
2b9da9412b |
fix(store): classify unique violations as expected copy rejections per final review
The store logged a unique-constraint violation as an unexpected rollback incident while the HTTP layer mapped the same error to a caller-facing 409. A workspace-unique field colliding in the destination — a playbook's invocation_slug, say — reaches this on ordinary input, so every routine 409 fired an operator warning and buried the deadlock signal the log exists to surface. Found by the final full-diff Codex pass over PLAN-2357 (P2: two commits classified the same error two ways). Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT |
||
|
xarmian
|
f15ba86db0 |
docs(server): correct the cross-workspace authz re-check contract per final review
The helper's doc mandated that a mutating caller "re-apply the check" inside its write transaction. Its only mutating consumer deliberately does not, and is right not to: these functions read through s.store rather than the caller's tx, so under READ COMMITTED the re-check would judge locked resources against authorization state read at several unsynchronised moments — reading as a write-time guarantee while providing none. State what a mutating caller actually owes (re-read the authorized resource IDENTITY in-tx and refuse if it moved) and what it must not do, so the contract and copyResourceInvariantPreCheck no longer disagree. Found by the final full-diff Codex pass over PLAN-2357 (P1: a documented write-time guard was in fact a TOCTOU check). Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT |
||
|
xarmian
|
1e48a7a1dd |
feat(cli): add pad item copy for cross-workspace copy and move (TASK-2366)
Wraps PLAN-2357's two endpoints behind one command:
pad item copy <ref> --to-workspace <slug> --collection <slug>
[--dry-run] [--archive-source] [--field key=value ...]
--dry-run renders the preflight's three contract buckets (carried /
dropped / needs_value) and DR-15's full warning set. Every bucket header
and every warning line prints unconditionally, zeros and empties
included: omitting a zero would make "no attachments" indistinguishable
from "this CLI does not report attachments", and DR-17's whole point is
that none of it is silent. Schema-supplied strings are escaped and list
members quoted, so a comma or newline in an option value cannot forge an
entry or a row.
--format json emits the endpoint's own response. json.Indent is a lexical
transform, so key order, unmodelled fields and int64 precision all
survive; the bytes are never round-tripped through a Go value.
DR-13, the no-retry obligation. There is no idempotency key, so a blind
re-run duplicates the item. Four mechanisms, each with a test:
1. the mutating copy runs on its own *http.Client AND its own
transport. The transport half is the one that matters: retry in Go
is almost always a RoundTripper wrapper, which a merely-dedicated
http.Client would inherit. A plain *http.Transport is cloned so
proxy/TLS config carries; a wrapper is not used at all;
2. its body is hidden behind an opaque reader, leaving Request.GetBody
nil so net/http's own nothing-written replay cannot fire;
3. redirects are refused rather than followed with the POST body;
4. failures are classified into three exclusive outcomes, because each
licenses a different thing to say. UNKNOWN (transport failure, 500
copy_failed) sends the user to check the destination and never
suggests a retry. COMMITTED-BUT-UNREPORTED (a 2xx whose body could
not be read or decoded) exits ZERO -- a non-zero exit would tell a
script the copy did not happen, which is the DR-13 duplicate
arrived at through the reporting layer. A 4xx is a refusal made
before any write and passes through plainly.
The same asymmetry governs stdout: a write failure on the dry run is an
error (nothing happened), while a write failure after the copy committed
goes to stderr and leaves the exit code at 0.
Refuse to guess. The preflight always runs first (it is read-only), and a
non-empty needs_value refuses before any mutating request, naming each
field and the exact --field flags to add. Mirrors the web dialog's
disabled confirm rather than round-tripping the user into an error they
could have been shown.
--field values are typed against the DESTINATION collection's schema, so
a number lands as a number. A malformed --field is a hard error here
rather than the silent skip `pad item create` does: this command's
contract is "you were told what to supply", and dropping a supplied value
would make the refusal a lie.
The response types in internal/cli mirror internal/server's. That is a
layering choice, not a cycle -- nothing in server imports cli, and the
mirror test imports server freely. It follows the posture already
recorded in internal/cli/bootstrap.go: this package is the HTTP client
and does not depend on the server package. An external cli_test package
walks both response shapes and fails on any JSON contract drift.
MCP is deliberately untouched: no pad_item.action: copy, and
ToolSurfaceVersion stays 0.15.
|
||
|
xarmian
|
f8ff5742e5 |
feat(server): add cross-workspace copy endpoint with post-commit fanout (TASK-2365)
Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT |
||
|
xarmian
|
01d640978c |
feat(server): add cross-workspace copy dry-run preflight endpoint (TASK-2364)
Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT |
||
|
xarmian
|
0fad869a28 |
feat(store): add CopyItemAcrossWorkspaces atomic orchestration (TASK-2363)
PLAN-2357 DR-9 / DR-9a / DR-11 / DR-12 / DR-14 / DR-16 / DR-17. One
store operation, one transaction: create in B, clone attachments,
archive A on a move, write provenance.
Lock order (the whole point of DR-9):
1. Both workspaces' advisory locks, sorted and deduplicated by the
hashtext LOCK KEY — sorting the ID strings does not order their
hashes, so two opposing movers could still deadlock.
2. Both collection rows FOR UPDATE, sorted by collection ID —
MigrateFields consumes both schemas.
3. Source item re-read under those locks; that snapshot is copied.
Both primitives are dialect-gated: FOR UPDATE is a syntax error on
SQLite, where BEGIN IMMEDIATE already serializes writers.
Pipeline: migrate -> overrides -> validate (DR-12: MigrateFields'
errors are stale once an override lands) -> quota -> PlanAttachmentCopy
INSIDE the tx -> rewrite content AND fields via the plan's IDMap ->
create in B -> attachment rows (originals before variants, item_id set
from the outset, uploaded_by = the actor) -> archive A -> provenance.
Seq (DR-14): B always advances; A advances only on ArchiveSource, and
a plain copy leaves A completely untouched. Quota (DR-16) runs inside
the transaction after the destination lock so two concurrent copies
cannot jointly exceed the cap.
Cross-backend attachment copies are REFUSED in v1
(ErrCopyCrossBackendAttachments): the store has no AttachmentStore
handle, and a byte transfer under both workspaces' locks would block
every writer in both workspaces on unbounded I/O with no rollback.
Supporting changes:
- CreateAttachmentTx: tx-taking insert (CreateAttachment is
self-committing), sharing one body with the pool form.
- CheckLimitTx: the feature COUNT reads through the caller's tx.
- createItemTxWithID: createItemTx with a caller-supplied id, so the
destination item id exists before the attachment plan is built.
Tests: creation parity, seq on both sides, DR-12 ordering, DR-8/DR-17
scrubs, attachment clone + rewrite (including refs in code fences),
DR-11a unresolvable refs, rollback at all four stages, quota. Postgres
only: opposing A->B / B->A copies do not deadlock, concurrent copies
cannot jointly exceed the cap, and colliding hashtext keys take one
lock. All three verified falsifiable by mutating the production code.
Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT
|
||
|
xarmian
|
60dd3e1a37 |
feat(store): add attachment resolution planner for cross-workspace copy (TASK-2354)
Implements PLAN-2357 DR-11 / DR-11a. PlanAttachmentCopy takes the copied content plus the FINAL destination fields and returns the old->new attachment UUID map, the rows to create (originals followed by their variants, parent_id remapped), the byte total, and the unresolvable-ref list. It writes nothing, takes no *sql.Tx, and is shared by the copy orchestration and the dry-run endpoint so their numbers cannot drift. DR-11a: every resolution is scoped to workspace_id = A AND deleted_at IS NULL, and the parent/variant traversal carries the identical scope. The reference set comes from user-controlled content, so an unscoped lookup would let a user clone another workspace's blob into a workspace they control, bypassing the download handler's workspace check. Refs that resolve to nothing under that scope -- dangling, soft-deleted, or foreign -- are never cloned and never fatal: they get no map entry, so the rewrite preserves the literal text and the copy renders exactly as broken as the source did. A cross-backend row emits an empty storage_key with the source key in SourceStorageKey, so the plan never contains a key the target backend cannot resolve. CreateAttachment now rejects an empty storage_key, which turns that contract into an enforced invariant: an orchestration that skips the Get/Put byte transfer fails at insert rather than creating a live attachment that 404s on download. Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT |