Repository navigation
control-plane, frontend: routing suggestions on P4 slice 1 (reviewed, no auto-dispatch) - #50
OfficialAbhinavSingh wants to merge 23 commits into
Conversation
… on its own Its autouse fixture truncated plans before anything had migrated a fresh database, so the file passed only after another postgres test file had run (5 errors, UndefinedTable, when run alone). It now uses test_reputation's run(), which migrates and then empties every table, like the other ledger tests.
…king the ledger Each of task_outcomes' per-dispatch lookups (WHERE task_id = ? AND kind = ? ORDER BY id LIMIT 1) had no usable index: money_events_on_chain is partial (tx IS NOT NULL), so every lookup was a full scan and the view cost tasks x events. At 4,000 dispatches and 12,000 events the reputation pass's pending query took 12.1 s and the credit-only count behind the unauthenticated GET /v0/agents/reputation took 4.8 s; with money_events (task_id, kind, id) they take 0.036 s and 0.017 s, and EXPLAIN shows all four lookups use it. The migration backfill test holds back every migration that needs money_events, so 0018 joins 0006 and 0015 there.
…dited ones (paper §8.2) task_outcomes counted a bad verdict, or a pass and a release. The paper validates an outcome that was audited or challenged and adjudicated, so three adjudicated cases produced no row: released unaudited then a challenge upheld; a challenge upheld before release, then returned for the challenge; a challenge rejected and the task released, unaudited. 0019 replaces the view (same columns). An upheld challenge is a failure; with no pass-and-release to keep, it sits at the upheld resolution. A rejected challenge plus a release is a success at the later of the two. Every audited case keeps exactly its old success and position, which a new regression test pins. docs/reputation-v0.md's table says the same.
… outcome record_missing_costs read the same oldest 50 pending outcomes every pass and stopped at the first error of any kind. 50 outcomes whose commitment the gateway never saw therefore held every slot for good: with 51 failures and only the newest commitment known, its cost was never asked for, and one malformed answer stopped the queue the same way. Each pass now tries every pending outcome, oldest first. GatewayCosts tells a gateway that cannot answer at all (unreachable, a non-400/422 error, not JSON, not a list: GatewayUnavailable, which still ends the pass) from an unusable answer about one task (another task's commitment, a missing, garbled or negative cost, a row that is not an object, a 400/422: BadCommitment, which skips that task only). A cost that cannot be read is still left pending, never taken as zero. Also: a write is counted only when the INSERT ... ON CONFLICT DO NOTHING inserted a row (another instance may have won the race), and a gateway row that is not an object is a BadCommitment instead of an AttributeError that escaped the pass and skipped refresh_posteriors.
…3 requires Posterior.observe accepted cost == 0. A run of zero costs decays tau toward 0, and after ~14,000 of them density overflows to inf, which Starlette's JSON cannot carry, so GET /v0/agents/reputation would 500. It now refuses cost <= 0. No existing test used a zero cost. task_costs still allows 0 (it copies the commitment as it is), so the fold now reads only cost_micros > 0: a zero-cost outcome is left out like one whose cost is unknown, instead of raising on every refresh.
GET /v0/plans/{uuid}/suggestions?explore=true appends a replay record per
task to routing_decisions, which is append-only, on every call, with no
limit: any signed-in account with one draft plan could grow it without
bound. Exploring calls are now limited the way create_plan limits the
planner: a per-requester advisory lock, a count of the last hour's calls
in a new suggestion_attempts table (planning_attempts' shape), 429 past
SUGGESTIONS_PER_HOUR (default 60), and the attempt row written in the same
transaction as the decisions. Greedy calls write nothing and are not
limited.
…from the paper - Cost is folded only from validated outcomes, not every completed task as the doc claimed; the paper (§8.3) uses all completed tasks, and whether unaudited tasks' costs should count is now stated as open. - Greedy as the default is this slice's divergence from the paper, which allows exploration suppression only per task (§9.4: "§9.5 explains why it cannot be the default"); defensible while the §9.5 subsidy does not exist. - "What this is not" now names §8.5 partial pooling and §9.4 retry tolerance, neither of which is built. - GET /v0/agents/reputation lists credit-only pairs too, not only pairs with counted outcomes.
Fix round 2 for D12. Paper §8.3's c > 0 constrains only the cost (Gamma) update; the success (Beta) update of §8.2 has no cost precondition. The round-1 fold read only cost_micros > 0, so a validated failure whose receipt metered 0 (a run that died before its first billed call) left no trace at all, and that agent kept its prior and could outrank agents with a real failing record. Posterior.observe now takes cost=None for an outcome with no usable cost: alpha/beta decay and increment and n counts it, kappa/tau are left alone. It still refuses a cost <= 0. The fold reads every wallet outcome whose cost is recorded (pending ones still wait) and passes None for a 0. The round-1 test that pinned the dropped outcome (n == 1) asserted the wrong behaviour and is replaced. docs/reputation-v0.md says the same, and its table gains the pass + upheld challenge + no release row (F6).
…ach agent is left out
…fits when the draw just missed
…suggestion answers Explore-mode rows say their numbers are this draw's, not the agent's estimate. Each pick now carries cost_is_prior (the existing _cost_is_the_prior check), and a cost with nothing metered behind it shows as "no metered cost yet". Answers from an older control plane (no ineligible, no cost_is_prior) are read with safe defaults instead of taking the review page down. The draw switch is disabled while a request is in flight, and the re-read after a save shows as updating.
main gained discard-draft (DELETE /v0/plans/{uuid}), plan idempotency
(its own migration 0018_plan_idempotency) and a redeem_admin fix.
- Renumber this branch's migrations after main's 0018: 0018_money_events_task
-> 0019, 0019_task_outcomes_adjudicated -> 0020, 0020_suggestion_attempts ->
0021. Update the references to them by number (test_money's held-back set,
reputation.py's docstring, docs/reputation-v0.md). None of them was applied
anywhere outside tests.
- frontend/e2e/live/plans.ts: ApiCall keeps both sides' fields (query from
here, key from main) and the recorded call fills both.
routing_decisions (0016) referenced plans with no ON DELETE action, and an exploring draw is only made for a draft. Main's discard-draft really deletes the plan row, so any draft that had a "Try a random draw" failed to discard with a ForeignKeyViolation (500). Migration 0022 drops routing_decisions_plan_uuid_fkey. plan_uuid keeps its value and NOT NULL; the replay log is an audit record that outlives the discarded draft. The append-only trigger is unchanged and the new test checks it still refuses UPDATE and DELETE on the surviving rows.
readSuggestions defaulted a missing cost_is_prior to true, so against a control plane without the field (origin/main's current one) every agent with a real, measured cost read "no metered cost yet". A missing field is unknown, not "only the prior". cost_is_prior is now boolean | null: true and false are kept as sent, and anything else (absent, null, malformed) reads as null. The picks show null as "cost not reported": no dollar figure and no claim about the prior.
|
@Viscous106 this builds suggestion-only routing on top of your P4 slice (126a98b) rather than replacing it -- reviewed it cold first (9 defects fixed, 2 rounds), then added eligibility + feasibility + a frontend UI. No auto-dispatch anywhere, checked repeatedly. Caught your discard-draft change (447f521) landing mid-build -- it collided with routing_decisions' FK to plans, fixed by dropping the FK (migration 0022), didn't touch discard_plan itself. Two things in the description are your call, not mine: the reputation endpoint's privacy scope, and the probation-evasion-via-roster-removal gap (needs a migration to close properly). Also found settings.spec.ts:51 failing on your 0ec2c79 independent of anything here -- likely cause traced in the description if you want it. |
|
Hey Abhinav, great work on PR #50 the query speedup and the suggestion routing logic look super clean! |
Viscous106
left a comment
Viscous106
left a comment
There was a problem hiding this comment.
The control-plane half is careful work. The suggestion-only boundary holds (the only writes are suggestion_attempts and routing_decisions, with no dispatch, escrow or assignment path), and the owner check, 404 and 409 handling are right. The posteriors read funding='wallet' only. The feasibility-probability maths checks out, and the rate limit is transactional. The eligibility tests are good guard tests, and the docs honestly mark the parameters as ours. But the PR conflicts with main, one check is failing, and there are three issues I'd want settled first. I read the diff in full and ran nothing.
High
- The frontend duplicates what main already has. Commit
3f722b2already addedSuggestAgents.tsx,suggest.ts,getSuggestionsand aSuggestionstype, and editedDraftPlanEditor.tsx. This PR adds a secondgetSuggestions,SuggestionsResponse/Suggestiontypes,useSuggestionsand aSuggestionsPanel, and edits the sameDraftPlanEditor.tsx,plans.tsandrun.spec.tsregions. A naive conflict resolution leaves two suggestion panels or a duplicate-export typecheck failure. Please drop this PR's frontend and rebuild it onSuggestAgents, or replaceSuggestAgentsdeliberately. - The operator concurrency cap counts free-credit dispatches (
eligibility.py:119-134,_UNRESOLVED_BY_AGENT, noplans.fundingfilter). Someone with a free-credit grant could dispatch a few cheap credit plans to a rival operator's agents and make that operator's whole roster show "operator at concurrency cap" for everyone. That breaks invariant 6 (free-credit work must not influence routing). The probation rule counts credit plans deliberately and documents why. The cap has no such reason, and no test covers it. Count only wallet-funded dispatches, and add a guard test. - Migration 0020 widens the "validated outcome" rule (invariant 4): a rejected challenge plus an unaudited release now counts as a success. If rejection means the bond is forfeited to the operator, an operator could self-challenge its own unaudited releases and farm counted successes. Please confirm the economics against paper §8.2 and §8.7, and add a test or a docs caveat.
docs/reputation-v0.mdsays "not manipulation-resistant" but doesn't cover this path.
Medium
4. Conflicts with main (client.ts, types.ts, DraftPlanEditor.tsx, e2e/live/plans.ts, run.spec.ts, changed by 3f722b2, 49bec0a and 07fa701; routes_routing.py changed by 126a98b). Migrations 0019-0022 are still free. Rebase and re-check.
5. A CI check fails, and I can't tell which. Candidates: a prettier format:check failure on the long lines, npm ci (jsdom 30 needs Node ^22.22.2 or ^24.15.0 but package.json says >=22.12), pyright on the new cast/isinstance code in reputation.py, or a duplicate symbol from finding 1.
6. record_missing_costs lost its LIMIT, so every pass makes one sequential gateway request per pending outcome with no time bound. It runs under the updater, and the unauthenticated GET /v0/agents/reputation reads the same data. Add a time budget or a larger cap.
7. It's oversized, and CLAUDE.md says one concern per PR. Suggested split: (a) reputation hardening with 0019 and 0020, (b) eligibility and the routes, (c) the frontend, rebased on SuggestAgents.
Low: "72% success · about $0.15 a run" shows a decayed posterior mean as "success" with no "estimated" wording. The "at cap" and "in probation" reasons expose other requesters' in-flight activity. suggestion_attempts is never pruned. test_routes_routing.py runs TRUNCATE on a shared DB. Migration 0022's DROP CONSTRAINT has no IF EXISTS.
Not verified: tests, migrations against Postgres, the paper's §8.2 and §7.5 text, and the full frontend tests.
main already shows routing suggestions (SuggestAgents, 3f722b2) against the same /suggestions endpoint, so this branch's own frontend is dropped rather than merged next to it: frontend/ is now exactly main's. The endpoint's answer stays a superset of what SuggestAgents reads (it only adds cost_is_prior and the agents left out with their reasons).
…ator cap The cap counted every unresolved dispatch, so anyone with a free-credit grant could send a few cheap plans to a rival operator's agents and have its whole roster shown as "operator at concurrency cap" to every requester. That is free-credit work influencing routing, which invariant 6 rules out. Only wallet-funded dispatches count now; probation still counts every plan, for the reason its docstring gives. The new test fails on the old query.
… not fixed Counting a rejected challenge as evidence (migration 0020) assumes the challenger is adversarial and disinterested. Nothing enforces that: an operator could bond a second wallet as challenger against its own agent's unaudited release, pick only releases it already knows are good, and farm counted successes with no real audit. The database has no way to prove two addresses share a controller, so a filter on the obvious case (the agent's own registered operator) would be cosmetic rather than a real fix. Recorded in the existing manipulation-resistance list rather than coded around.
It read every pending-cost task in one pass with no bound, one sequential gateway request each, so a backlog (an outage, or heavy volume) made one pass of the reputation updater take arbitrarily long, every REPUTATION_POLL_S, on top of GET /v0/agents/reputation reading the same table. Two new settings cap it: REPUTATION_COST_BATCH (default 200 tasks, a query LIMIT) and REPUTATION_COST_BUDGET_S (default 10s wall-clock, checked before each request). Either stops the pass early; the rest waits for the next poll.
Every other DDL statement in this file is idempotent (CREATE ... IF NOT EXISTS); this was the one that was not.
Summary
126a98b("P4 slice 1") went straight tomainwith no review: cost-adjusted reputation and suggestion-only routing. This builds on it rather than replacing it — reviews and fixes the slice's own defects, then adds eligibility (probation, operator concurrency cap) and budget-feasibility screening with honest per-agent reasons.No auto-dispatch anywhere. Suggestions only ever advise; the requester always approves manually with their own picks via the existing
/approveflow. Verified at every task review and the final whole-branch review, independently, more than once.Frontend dropped from this PR. Main shipped its own suggestions UI (
3f722b2,SuggestAgents.tsx) against the same/suggestionsendpoint while this was in review, so this PR's own frontend (AgentPicks,SuggestionsPanel,useSuggestions) was removed rather than merged in next to it. The endpoint's answer stays a superset of whatSuggestAgentsreads.Plan:
docs/superpowers/plans/2026-11-02-routing-on-p4.md(4 tasks, all complete):126a98bcold (12 confirmed defects, 0 Critical, 5 Major), fixed 9 of them across 2 rounds — most notably a silent reputation-pipeline stall if 50 outcomes' costs never arrive from the gateway, a view that dropped challenge-adjudicated outcomes the slice's own doc claimed to cover, a missing index (12.08s → 0.036s on the same query), an unrate-limited write on a GET endpoint, and a zero-cost outcome that was dropping its success/failure signal entirely instead of just its cost signal.eligibility.py— probation for a proven falsification (credits-plan falsifications count, since probation is evidence of fraud, not reputation) and an operator concurrency cap, both derived from existing tables, no migration. One known, accepted gap: an agent removed from the roster stops counting toward its operator — needs a migration to close properly, named as a follow-up.SuggestAgentsUI; this PR's version was dropped (see "Frontend dropped" above).mainhad moved again mid-build — a just-shipped "Discard draft" feature (DELETE /v0/plans/{uuid}, a real row delete) collides with this PR'srouting_decisionsaudit table (inherited from126a98b, referencesplans, append-only). Fixed by dropping the FK: the audit log now survives a discarded draft, nothing in the discard endpoint itself was touched.Review round (Viscous106, 2026-11-02)
record_missing_costsis capped per pass by count and by wall-clock time, so a backlog or a slow gateway can't make one pass of the reputation updater run unbounded; migration 0022'sDROP CONSTRAINTis nowIF EXISTS; merged currentmain(frontend dropped, as above — no textual conflicts with3f722b2/49bec0a/07fa701/126a98bremained to resolve once the duplicate frontend was gone).docs/reputation-v0.md's "Not manipulation-resistant" list; needs a protocol-level answer (paper §8.2/§8.7), not a query change./v0/agents/reputationand/suggestionsstill expose other requesters' in-flight activity through the eligibility reasons;suggestion_attemptsis never pruned;test_routes_routing.pystill runs a shared-DBTRUNCATE.Type of change
feat— new functionalityWhat I verified
ruff/format/pyrightclean, full suite green including thedocker/postgres-marked tests CI runs separately.settings.spec.ts:51, previously noted here as pre-existing onmain(root cause:LiveSettings.tsxre-keyed the preferences form on save, so the "Preferences saved." message was lost before it was seen) — fixed onmainby frontend: keep the Preferences saved note after a save #51, merged in with everything else.126a98bitself).Open items for you / Yash (not fixed here, flagged on purpose)
/v0/agents/reputation(and/suggestions) leak more than intended: polling either lets anyone back out a requester's category, outcome timing, and exact cost. The design doc calls the reputation endpoint "public, like the agent list" — this is a scope question (should it be?), not purely a bug. Not touched.agents.toml/agents.json. Needs a migration to track operator history independent of roster membership.