mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-10-12 07:58:41 +00:00
5003718802
deliveredSessionCount applied three of watchNotificationVisible's four gates, missing the first thing delivery checks: vis.allows(CollectionID, ItemID). Broadcast over-reported. Targeted was worse — the publish-skip reads this count, so the gate passed, the push went out, the stream dropped it on visibility, and the response said delivered_sessions: 1. An instruction lost behind a success. Per Dave's day-49 ruling, visibility is RE-RESOLVED at push time rather than snapshotted: membership and grants are revocable, so a value cached at connect goes wrong exactly when revocation is what makes it matter. The one input that cannot be re-resolved is the target connection's auth transport — computeWatchAccessVisibility consults isBearerAuth exactly once, inside the admin bypass, and the pushing request only knows its own. So SessionOrigin.BearerAuth is recorded at Add(). That is NOT the snapshot the ruling rejected: auth transport is a property of the connection, fixed when it opened and not revocable while held, so it cannot go stale. Armed is the precedent. SessionOrigin is kept separate from SessionIdentity because that type documents itself as self-declared and never verified; folding a server-derived security fact in there would silently retract the warning for one field. Both comments state the rule for future extenders: connection properties are admissible, derived authorization state never is. computeWatchAccessVisibility now takes a bool instead of an *http.Request, which makes the per-connection input visible in the signature and lets the count answer for a connection it is not serving. COST: "re-resolve per counted session" reads like N access checks per push. It is at most TWO, and sessionVisibility's memo makes that true by construction rather than by careful calling — every other input is per-user and identical across the sessions counted, so one varying boolean bounds the answers at two. Pinned by a test with 50 sessions. Codex round 1 (P1): the first version swallowed store errors into "not visible", reintroducing BUG-2698 through this fix — a targeted push reporting 0 SKIPS the publish, so a DB blip would drop the instruction and answer 200, in a function whose own doc comment says why 0 is load-bearing. Round 2 (P1): the same class one layer down — computeWatchAccessVisibility collapsed FOUR store failures into a denial, two discarded into underscores. Fixed as a class per CONVE-18. Resolution and policy are now separate: stream-side callers discard the error explicitly with reasons, only the counting caller propagates. Round 3 CLEAN. CONVE-23 sweep found three consumer-facing artifacts still describing the old mechanism, none on a line this diff touched: the plugin skill doc, the web push dialog, and pad push --help. All three corrected to name what actually remains rather than deleting the caveat. Plugin 0.3.1 -> 0.3.2, since installed plugins are version-pinned at install. NOT fixed, deliberately: the UNDER-count. A stream past maxSessionsPerUser receives broadcasts while never entering the registry. delivered_sessions remains an estimate with error in both directions, and every consumer-facing description now says so. Two coverage gaps recorded rather than rounded off: mutation M11 survives (the reporting test reaches only the first of four store calls, because closing the DB fails it first), and no test drives the whole chain store-fault-to-503 (the DB-close instrument kills the request earlier, so such a test would have gone green against the wrong 500 — deleted rather than relaxed). Also lands the BUG-2752 refutation sentinel: that item claimed the OAuth workspace allow-list went unenforced on /api/v1/events/stream. Refuted — no allow-list-bearing credential can authenticate to /api/v1/* at all. The test guards that format gate, so if it ever widens, the refutation's premise fails loudly instead of silently reopening a leak. Gates on the merged tip: make test 27 pkgs, make lint 0 issues, full Postgres suite 27 pkgs, govulncheck, codex CLEAN, CI 7/7. Claude-Session: https://claude.ai/code/session_01Cpr3teiHHsgcTmg2xhHA86
174 lines
7.5 KiB
Go
174 lines
7.5 KiB
Go
package main
|
|
|
|
import (
|
|
"errors"
|
|
"fmt"
|
|
"os"
|
|
"strings"
|
|
|
|
"github.com/spf13/cobra"
|
|
|
|
"github.com/PerpetualSoftware/pad/internal/cli"
|
|
)
|
|
|
|
// maxPushMessageLenForHelp mirrors server.maxPushMessageLen
|
|
// (handlers_push.go) — KEEP IN SYNC. Duplicated rather than shared
|
|
// because the two live in different packages with no existing shared
|
|
// constants package for a single value; it exists here purely so
|
|
// `pad push --help` states the bound instead of a user only learning it
|
|
// from a 400. The server is still the enforcing source of truth — this
|
|
// value is advisory (help text), not validated client-side.
|
|
const maxPushMessageLenForHelp = 4096
|
|
|
|
// pushCmd is `pad push <ref> -m "message"` (IDEA-2544 Phase 1) — the
|
|
// "push this to my agent" verb: an explicit, user-authored instruction
|
|
// bound to an item, delivered to the caller's own connected monitor
|
|
// sessions over the same watch-events stream `pad watch --stream
|
|
// --for-session` consumes. Distinct from `pad watch` (a durable, item-
|
|
// scoped subscription): a push is one-shot, transient, and IS an
|
|
// instruction rather than a passive fact — see the plugin skill's
|
|
// notification-etiquette section for the receiving-agent behavior
|
|
// contract.
|
|
func pushCmd() *cobra.Command {
|
|
var message string
|
|
|
|
cmd := &cobra.Command{
|
|
Use: "push <ref>",
|
|
Short: "Push an item + instruction to your own connected agent session(s)",
|
|
Long: fmt.Sprintf(`pad push <ref> -m "message"
|
|
Publish a self-addressed push notification on an item. It reaches
|
|
your OWN plugin-monitor sessions (pad watch --stream --for-session)
|
|
that are ACCEPTING pushes and can see the item — a connected session
|
|
that hasn't opted in, or that lacks access to the item, does not
|
|
receive it. Fire-and-forget over the watch-events bus, with no
|
|
durable inbox: a push nothing is listening for is simply not seen
|
|
(Phase 1 scope).
|
|
|
|
On a multi-instance deployment (PAD_REDIS_URL set) both broadcast
|
|
and session-targeted pushes reach your sessions on every instance:
|
|
the notification bus and the session-presence registry are both
|
|
shared, so a session connected to one server is visible and
|
|
addressable from any of them. Addressable is still not the same as
|
|
delivered. The count does check that a session is accepting pushes
|
|
and that it can see the item (BUG-2725 — it applies the same
|
|
visibility predicate delivery applies), but it is taken before the
|
|
publish and nothing acknowledges receipt, so treat it as a
|
|
prediction rather than a receipt. It can also under-count: a session
|
|
past the per-user registry cap receives broadcasts while never
|
|
entering the registry.
|
|
|
|
-m/--message is required and must not be blank; it is the
|
|
instruction text the receiving agent acts on (load the item first,
|
|
then do what the message says — see the plugin skill's notification
|
|
section for the full contract). Newlines are collapsed to spaces:
|
|
the monitor stream is a one-line-per-event wire contract. Limited to
|
|
%d characters after that collapse — the server rejects anything
|
|
longer rather than truncating it.`, maxPushMessageLenForHelp),
|
|
Args: cobra.ExactArgs(1),
|
|
RunE: func(cmd *cobra.Command, args []string) error {
|
|
trimmed := strings.Join(strings.Fields(message), " ")
|
|
if trimmed == "" {
|
|
return fmt.Errorf("-m/--message is required and must not be blank")
|
|
}
|
|
|
|
client, _ := getClient()
|
|
ws := getWorkspace()
|
|
|
|
result, err := client.PushItem(ws, args[0], trimmed)
|
|
if err != nil {
|
|
// AMBIGUOUS FAILURES GET SAID OUT LOUD (codex round 9 on
|
|
// BUG-2699). The handler publishes BEFORE it writes its
|
|
// response, so an error is not proof the instruction went
|
|
// nowhere: a response lost in transit, a truncated body, a
|
|
// gateway envelope, or the server's own push_unconfirmed all
|
|
// reach here identically to a clean refusal. A user who reads
|
|
// "error" and re-runs the command delivers the push twice —
|
|
// there is no idempotency key, and the receiving agent acts
|
|
// on each copy.
|
|
//
|
|
// The web dialog has drawn this distinction since TASK-2588;
|
|
// the CLI had not, so the same outcome produced a safe
|
|
// message in a browser and a misleading one in a terminal.
|
|
if !pushRefusedBeforePublishing(err) {
|
|
fmt.Fprintln(os.Stderr,
|
|
"warning: the push may or may not have been delivered — check your agent session before re-running. "+
|
|
"Re-sending an instruction that already landed delivers it twice.")
|
|
}
|
|
return err
|
|
}
|
|
|
|
if formatFlag == "json" {
|
|
return cli.PrintJSON(result)
|
|
}
|
|
fmt.Printf("Pushed %s\n", args[0])
|
|
return nil
|
|
},
|
|
}
|
|
|
|
cmd.Flags().StringVarP(&message, "message", "m", "",
|
|
fmt.Sprintf("instruction text to push (required, max %d characters after whitespace collapse)", maxPushMessageLenForHelp))
|
|
return cmd
|
|
}
|
|
|
|
// pushPrePublishRefusalCodes are the error codes the push endpoint can
|
|
// only produce BEFORE it publishes, so the instruction provably did not go
|
|
// out and re-running is safe.
|
|
//
|
|
// Deliberately mirrors web/src/lib/push/dispatch.ts's
|
|
// PUSH_PRE_PUBLISH_ERROR_CODES — the two surfaces answer the same question
|
|
// about the same endpoint, and letting them drift would mean a push that is
|
|
// safe to retry in a browser and unsafe in a terminal, or the reverse.
|
|
// Keep them in step.
|
|
//
|
|
// `unavailable` covers both the no-bus branch and a bus that was already
|
|
// closed; both are refusals with nothing published. `archived` is the 409
|
|
// writeItemResolveError writes when the ref names a soft-deleted item —
|
|
// item resolution happens before anything is published (codex round 12).
|
|
//
|
|
// ENUMERATED from the handler rather than taken one at a time: the codes
|
|
// handlePushToItem and its helpers can write before the publish are
|
|
// bad_request, unauthorized, unavailable, not_found (getWorkspace,
|
|
// requireItemVisible, writeItemResolveError), archived, and internal_error,
|
|
// plus the middleware codes below it. Round 12 named `archived`; the
|
|
// enumeration is what found `internal_error` alongside it.
|
|
//
|
|
// Notably ABSENT, both deliberately:
|
|
// - push_unconfirmed, which exists precisely to say the outcome is
|
|
// unknown.
|
|
// - internal_error. It IS pre-publish today — this handler only reaches
|
|
// writeInternalError from the item-resolution path — but unlike every
|
|
// other code here that is a property of where one call sits, not of
|
|
// what the code means. A future writeInternalError added after the
|
|
// publish would silently make this entry wrong, and wrong in the
|
|
// direction that costs a duplicate dispatch. A spurious warning on a
|
|
// 500 costs a sentence.
|
|
var pushPrePublishRefusalCodes = map[string]bool{
|
|
"bad_request": true,
|
|
"unauthorized": true,
|
|
"not_found": true,
|
|
"forbidden": true,
|
|
"permission_denied": true,
|
|
"unavailable": true,
|
|
"archived": true,
|
|
"rate_limited": true,
|
|
"plan_limit_exceeded": true,
|
|
"csrf_error": true,
|
|
"email_not_verified": true,
|
|
}
|
|
|
|
// pushRefusedBeforePublishing reports whether err is a refusal the server
|
|
// provably wrote before publishing.
|
|
//
|
|
// Defaults to FALSE for anything it cannot identify — a transport error, a
|
|
// non-JSON gateway response, an unrecognised code. That default is the
|
|
// whole point: an unknown outcome must be treated as possibly-delivered,
|
|
// because the cost of being wrong in the other direction is a duplicate
|
|
// dispatch.
|
|
func pushRefusedBeforePublishing(err error) bool {
|
|
var apiErr *cli.APIError
|
|
if !errors.As(err, &apiErr) {
|
|
return false
|
|
}
|
|
return pushPrePublishRefusalCodes[apiErr.Code]
|
|
}
|