Repository navigation
feat(operator-queue): an agent sees its pending asks and can replace one (#3247) - #3256
Conversation
9737639 to
33a0cc6
Compare
b8ad596 to
c650216
Compare
…3247) CP1 of the replace-a-pending-ask slice. `operator_queue` gains two nullable TEXT link columns: `replaces` on the successor (the predecessor row's uuid) and `replaced_by` on the predecessor (the successor row's uuid). Both are stamped in the one per-agent locked transaction the next checkpoint adds, so each row is self-describing on every surface that holds only one of the pair. Both schema tracks (Invariant #9): SQLite entry `operator_queue_replace` and Alembic `0090_operator_queue_replace` chained after this branch's head `0089_supersede_queue_flood_backlog` — `0090` is also taken by the independent #3246 branch and open PR #3145, so expect a renumber at merge; `check_alembic_heads.py` names the fork. `schema.py` and `tables.py` DDL updated (the `disposed_by` comment gains `agent`); `_row_to_item` / `_SELECT_COLS` carry the columns; `_MACHINE_ROW_FIELDS` and the ent#715 `MACHINE_ROW_KEYS` pin are extended in the same commit. No index. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
CP2 of the replace-a-pending-ask slice. `create_native_item` gains `replaces=<predecessor uuid>`, handled inside the existing per-agent lock after the replay check: a compare-and-set ends the predecessor (`cancelled` / `disposed_by='agent'` / `disposition_reason='replaced'`, `replaced_by` = the uuid this call minted) only if it is still `pending`, not past its deadline (respond's clause, so "denied by timeout" cannot be sidestepped), under this agent AND `raised_by='agent'` — the belt beneath the sink's ownership gate. The successor is inserted with `replaces` set. Lost CAS: the predecessor already ended → `replaces_ended`, the row as it stands, nothing written — a person's answer always wins. Still pending past its deadline → expired here as the clock would (`expired_now`, T5b), then refused. Not this agent's own agent-raised ask → `replaces_not_own`, nothing written. Over the cap after the CAS, or a colliding insert after it (the PG file-poller race), `_RollBack` unwinds the whole transaction so a half-replace never commits; a replay never re-applies the replace. The `database.py` facade mirrors the parameter (both `_FACADE` pins stay green). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…g asks (#3247) CP3 of the replace-a-pending-ask slice. `ask_operator` / `raise_ask` gain `replaces=<request_id>`: `_replace_target` is the security gate — the lookup is scoped to `(agent_name, request_id)` and the row must carry `raised_by == 'agent'` ON THE COLUMN (never `_raiser_of`, which calls an unreserved NULL-raiser platform alarm "agent"); one uniform 422 `invalid_replaces` for malformed / self / missing / another agent's / a gate's / a platform's / a pre-#611 row. The CAS belts it again. The predecessor ends `cancelled` / `disposed_by='agent'` / `disposition_reason='replaced'` through the one ending sink (`_ended`): an agent-keyed `replaced` audit row, a thin `operator_queue_cancelled`, and the observers — `_wake_filer` skips an ending the agent authored itself (T6), asserted on the post-CAS row. A predecessor that already ended is refused 409 `replaces_ended` with ids and enums only (a person's answer stands, read with `get_my_ask`); still pending past its deadline it is expired in-transaction and the normal expiry event fires first (T5b). T8: a new ask whose canonical `proposal` equals one of the agent's own PENDING asks is refused 409 `already_pending {request_id}` unless `replaces` names it (`list_pending_proposals_for_agent` + facade). Receipt gains `replaces`, `replaced_by`, `disposed_by`; `_differs` compares `replaces`; the readback maps both links to request_ids; `OperatorAskCreate.replaces`. Pins re-set in the same commit: `RECEIPT_KEYS`, the ent#611 `_FACADE` list and `READBACK_KEYS`, the ent#329 `_SET_CAS_ACCESSORS` (the native create is a CAS writer whose `predecessor` is the won row), and the ent#611 body helper now proposes a distinct action per ask (T8's accepted consequence). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Every composed turn (push, chat and pull-claimed — all go through compose_system_prompt) now lists the agent's own pending asks beside the ended ones: `request_id (type, age) "title"`, oldest first, at most 8, titles sanitized and cut to 48 characters, then "and N more — list them with list_operator_queue". Drawn from _own_pending_conds, the predicate that spends the open-ask budget, so the line and queue_full cannot disagree. Omitted when there are none; a failed read omits the line, never the turn. An ending the agent authored itself (disposed_by=agent) reads `replaced` on the Ended asks line. Tests: TestPendingLine in test_3247_replace_ask.py (red before the renderer existed: no Pending asks line), facade pin in test_ent611. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- ask_operator: one clause right after the idempotency sentence ("Never
re-ask what is still pending … set replaces"); the full rule and every
refusal (replaces_ended, invalid_replaces, already_pending, replaced_by)
in the new `replaces` parameter description. The "(and in your owner's
Workspace …)" aside moved into the `to` parameter to pay for it. The
published description is 1,762 characters (bound: 1,800, unchanged).
- get_my_ask: disposed_by gains `agent`; names replaced_by / replaces.
- types.ts: replaces on the create, replaces/replaced_by/disposed_by on
the receipt and the item.
- Platform prompt (both authored copies): "Do not re-ask what is still
pending" paragraph naming replaces_ended and already_pending; contract
step 3 says the Execution Context also lists pending asks. Agent guide
follows. SENTINELS += the rule, replaces_ended, already_pending.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
utils/operatorQueue.js — one rule for every Operations surface: - endingKind: a cancel with disposed_by (or the Workspace projection's ended_by) = 'agent' is `replaced`; ENDING_LABELS.replaced = 'Replaced'; queueEnding.who = 'the agent' → "Replaced by the agent". A person's cancel, a timeout and a platform ending keep their own words. - queueReaskBadges: the successor says "Replaces <request_id>" from its own `replaces`, the predecessor "Replaced by <request_id>" from its own `replaced_by`; the loaded list only supplies the other ask's request_id (a fallback label when it is not loaded). ResolvedCard hides the raw `replaced` reason token on an agent-authored ending; both cards key the badge testid by the badge key. /m inherits. Spec operatorQueueReplace.spec.js mounts ResolvedCard, QueueCard and MobileAdmin; red before the change (a replaced ask read "Cancelled"). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A replaced ask reads "Replaced by the agent" on every Workspace view and leaves the "needs you" count; the replacement shows "Replaces <request_id>". - `_ending_of` maps `disposed_by == 'agent'` to `ended_by: agent` BEFORE the email arm, so the agent's replace is never read as a person's or the operator's ending. - `WorkspaceAsk` gains `replaces` / `replaced_by`: the OTHER ask's request_id, nothing else about it, and only when this ask's addressee could already see that ask (same agent, same addressee, a visible kind). A replacement addressed to someone else or to the operator is never named. - `PortalAsks.endingLine`, `PortalInboxList.askStatusLabel` and `portalChatAsks.askHistoryLine` read through one `workspaceEndingText`. Red with the source reverted: workspaceReplacedAsk.mount.spec.js (5 of 9) and TestWorkspaceProjection (5 of 5) fail; green restored. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… false (#3247) - requirements/security.md §26.9: `disposed_by` is person | timeout | platform | agent; "only a person ends an ask" names its one exception (an agent replacing its OWN pending ask); the Execution Context line covers pull-claimed turns and the new pending line; the replaced-ask surfaces, with the accepted deviation stated: the two asks name each other by request_id in a badge, and there is no deep link to a single item. - §26.10: replace (`invalid_replaces`, `replaces_ended`, atomicity, no wake), `already_pending` (T8), and the pending line. - feature-flows/operating-room.md: the replace flow, the ledger enum, the stale pull-turn line (pull-claimed turns DO compose the platform prompt), a changelog row. - architecture/api-endpoints.md, backend.md, database.md (the two link columns and `disposed_by = 'agent'`). - user-docs/automation/approvals.md (Replace concept, `replaces?`, who can end an item, the pending list) and recommended-fleet-prompt.md ("Only a person ends an ask" gains the replace exception). - A learnings fragment; tests/registry.json entries for test_3247_replace_ask.py, operatorQueueReplace.spec.js and workspaceReplacedAsk.mount.spec.js. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n, and a refused gate raise surfaces its real code (#3247) The T8 guard refused the skill gate's second approval for the same occurrence (trinity-enterprise#751 raises one approval per occurrence with the same proposal): four ent#751 tests were red on this branch and green on its base. The guard now runs only for an agent-raised ask and compares only against the pending asks the same agent raised itself (`raised_by == 'agent'` on the column); a gate raise and a platform row are never refused by it and never counted by it. The refusal also exposed an older bug: `skill_gate_service` read `e.status` where `AskRejected` carries `status_code`, turning any sink refusal into an AttributeError (a 503 `approval_unavailable`). A refused gate raise now reaches the caller with its real status and code. Every other caller of the raise path was checked: the three gate sites pass `raised_by="gate"`, the router passes `raised_by="agent"`, and nothing in the private submodule calls it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…approvals only, disposed_by on replaces_ended, no titles on outside turns, stale strings (#3247) I2: the duplicate-proposal guard moves from the sink into the create's own locked transaction (`_pending_proposal_match`, after the replace's compare-and-set and before the depth count), so two concurrent raises with one proposal have exactly one winner and a refusal rolls the CAS back with everything else; a test models the interleaving with two threads. I3: the guard applies to an agent's `approval` with a non-empty proposal only — a question or an alert is never guarded, and a missing or empty proposal is not a proposal, on both sides of the comparison. I4: `replaces_ended` carries `disposed_by` and its message is branched on it, so a platform ending is not called an operator's. I1: the pending line shows the agent's own ask titles only on a turn serving the owner or an operator; on `public` and `paid` turns each entry is `request_id (type, age)` with no title (`build_execution_context`, the one place the audience is known). Both modes tested; security requirement and operating-room flow updated. I5: the two "person | timeout" comments (dependencies.py, types.ts) name all four authors, and respond_to_operator_queue's description now reads: a person answers or cancels, an agent can only replace its own pending ask, the platform expires. ask_operator's description is unchanged (1,762 chars). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…o the branch base (#3247) `docs/user-docs/agents/recommended-fleet-prompt.md` mirrors a canonical prompt text that is owned and edited elsewhere first; CP7's change to its "Only a person ends an ask" sentence is the owner's call, not this branch's. The file is restored to its content at the branch base; whether that sentence should mention the agent's own replace now needs the owner's decision. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
c650216 to
0752863
Compare
|
Re-parented onto Local evidence on the rebased head
Still owed per the merge-train note on #3255: once #3255 lands, this branch's |
|
✅ Alembic head check clear — merging this PR into Previously flagged; resolved. Advisory — this check does not block merge. · head_sha: |
AndriiPasternak31
left a comment
There was a problem hiding this comment.
Reviewed at 0752863. #3253 is in (merge-base fa50b38), so the base is right.
The core works. I reverted each piece in a scratch copy, and the matching test went red every time:
- the atomic replace (cap rollback, collision rollback)
- the answered-first race and its message
- own-asks-only (other agent, gate row, platform alarms)
- the open-ask limit after a replace
- the contract text (the MCP description and both prompt copies)
- "Replaced by the agent" and the successor link on the Operating Room, /m, the Workspace and get_my_ask
In total, 20 of 22 mutations went red.
Three changes before merge:
-
Titles on outside turns.
_OUTSIDE_AUDIENCES = {"public", "paid"}(platform_prompt_service.py:55) is a denylist. Channel turns dispatch with triggered_by set to the channel type (telegram / slack / whatsapp, message_router.py:871), and voip, room and a2a aren't in it either. I checked: an ask titled "Refund 900 USD to customer X" shows its title in the Pending asks line on all of those. Could we flip it to an allowlist of owner/operator triggers, like canvas_service._OPERATOR_SIDE_TRIGGERS, and add telegram plus an unknown label to the outside-turn test? -
Order-dependent test. test_a_colliding_insert_after_the_cas_rolls_the_replace_back fails whenever test_1595_sync_health_signals.py runs first ("created" != "replayed"), because the patch lands on a stale OperatorQueueOperations. Repro (I re-ran it: 1 failed; the file alone is 76/76):
pytest tests/unit/test_1595_sync_health_signals.py tests/unit/test_3247_replace_ask.py -p no:randomly
Fix: patch type(real_db._operator_queue_ops), both the real_cas read and the setattr, as test_ent611_native_ask does. With that, the pair passes and the test still catches a half-replace.
- "Never edited in place" has no test. If the CAS also overwrote the predecessor's title, all 76 tests would stay green. One assertion that the old ask's title, question, options and proposal are unchanged after a replace would pin it.
Migration / rebase: #3255 has landed, so this branch now conflicts with dev (the files the PR body predicted), and 0090_operator_queue_replace forks against dev's 0090_platform_alert_subjects. #3145 (0091) and #3269 (0092) are on today's train, so re-parent onto whatever dev's head is when you merge it in. If both land, that is 0093_operator_queue_replace on 0092_portal_messages_attachments; I checked that this gives a single head. ALEMBIC_REVISION / ALEMBIC_PARENT in test_3247_replace_ask.py:36-37 move with it. The findings above are in files that don't conflict, so they still apply after the merge.
Smaller, not blocking:
- If the agent wins the race, a person who then answers is told "This ask was just answered elsewhere" (client_portal/asks/service.py:492/502). Nobody answered; the agent replaced it. That case deserves its own message.
- The Pending asks line also lists NULL-raiser rows (legacy file rows, skills-reconcile- / retention-guard- alarms). The agent is told to replace them, but replacing one gets 422 invalid_replaces.
- The PR body still says "stacked on #3253 / draft until the chain is in".
Not verified on my side: PostgreSQL (CI has no PG pytest), and shuffled full-suite seeds other than CI's 12345.
|
✅ Nightly unit-suite clean when this PR is merged into |
|
Resolve by merging |
…ending-ask Conflicts: 17 files. Both sides kept throughout — #3246 (platform alert subjects, the `platform` ending) and trinity-enterprise#815 from dev beside #3247's replace link and `agent` ending. Alembic: 0090_operator_queue_replace forked against dev's 0090_platform_alert_subjects. Renumbered to 0097_operator_queue_replace and re-parented onto dev's head 0096_a2a_internal_scope (one head again); the SQLite `operator_queue_replace` entry now sits after dev's newest entries. Semantic, not textual: get_my_ask's description now lists `person | timeout | platform | agent` (dev added `platform`, this branch `agent`), so the mcp-server test pinning that string follows it. tests/registry.json rebuilt from the merge stages (dev's list plus this branch's three new entries). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…atches the live class, the old ask is pinned unedited (#3247) Review of 2026-10-06: 1. Titles on outside turns. The Pending asks line withheld the agent's own ask titles by denylist (`public`, `paid`), so a channel turn (its trigger label is the channel type: telegram / slack / whatsapp), voip, a room, inbound a2a and any label added later all showed them. Flipped to an allowlist of owner-or-operator triggers, the canvas_service._OPERATOR_SIDE_TRIGGERS shape plus `user` and `agent`, which the existing test already held as owner-side. Every other label, and no label, gets `request_id (type, age)` alone. 2. test_a_colliding_insert_after_the_cas_rolls_the_replace_back patched an imported OperatorQueueOperations, which is a stale class once an earlier test has reloaded the module (test_1595_sync_health_signals.py first: "created" != "replayed"). It now patches type(real_db._operator_queue_ops). 3. "Never edited in place" had no test: a CAS that also rewrote the predecessor's title left the file green. One test now holds title, question, options and proposal, and that only the ending fields moved. Also: a Workspace answer that loses the race to the agent's replace said "This ask was just answered elsewhere". Nobody answered it; it now says the agent replaced it (same 409 already_resolved). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@AndriiPasternak31 your 10-06 review is addressed in Blocking
Migration. Re-parented onto dev's current head: now Smaller
Open for discussion
Merge notes. 17 textual conflicts, both sides kept. Two were semantic: Local results. 354 related unit files: 10890 passed. Full unit suite, seed 12345: 24536 passed, 4 failed, the same 4 local pin-parity failures as on |
|
merge-train: pushed Not merged this batch: it waits for @AndriiPasternak31's re-review. Validation at Open for that re-review:
|
AndriiPasternak31
left a comment
There was a problem hiding this comment.
Blockers 2 and 3 from 10-06 and the migration are fixed, and I checked each by mutation. Blocker 1 is fixed for channels but not for the whole class: three triggers on the new allowlist also run turns whose reply goes to someone who is not the owner or an operator.
Re-reviewed at 02ec804de (merge d27c2d3c3 on dev 82b99c302, plus F4). Full unit suite, CI's command (seed 12345, --dist loadfile, Python 3.13): 24548 passed, 0 failed. The base 82b99c302 gives 24456 passed, 0 failed. The nine touched backend test files: 630 passed on each of seeds 12345, 67890 and 99999. Touched frontend specs: 37 passed. pg-migrations on this head ran 0096_a2a_internal_scope -> 0097_operator_queue_replace.
Since then, 2203d37ce (the merge-train's merge of dev) landed. It touches none of the files cited here except tests/registry.json, so everything below still holds at that head. The suite numbers above are from 02ec804de.
What I verified from the 10-06 review
- 1. Titles by allowlist (platform_prompt_service.py:59-64, :1028). I put the
{public, paid}denylist back, and 9 cases went red on 3/3 seeds: telegram, slack, whatsapp, voip, room, a2a, an unknown label,""andNone. Addingtelegramto the allowlist turns exactly that case red. Settinginside = Falseturns the five owner-side cases and both composer tests red. So the test catches errors in both directions. The membership still has a problem; see Blocking 1. - 2. Order-dependent collide test (test_3247_replace_ask.py:270-288). With the old import back, your repro pair goes red (
'created' == 'replayed') and the file alone stays green. So the fix is what makes it order-independent. Removing the collision rollback (operator_queue.py:473-474) goes red alone and after test_1595, on 3/3 seeds. The pair now passes in both orders (99/99). - 3. Never edited in place (test_3247_replace_ask.py:159-182). I made the CAS also write
title, thenoptions, thenproposal. Each time that one test went red and nothing else did, on 3/3 seeds. - Migration.
0097_operator_queue_replacesits on0096_a2a_internal_scope.check_alembic_heads.pyreports 98 revisions and 1 head, both on this tree and on an in-memory merge with today's dev (a18b02437). The SQLite entry is last (migrations.py:5693),ALEMBIC_REVISION/ALEMBIC_PARENTmoved (test:36-37), and nothing still refers to0090_operator_queue_replace. - (a) Race message (client_portal/asks/service.py:506-509). Removing it turns
test_answering_an_ask_the_agent_just_replaced_says_sored on 3/3 seeds. When a person wins the race, the message still reads "answered elsewhere". - (b) NULL-raiser rows. This is partly resolved, but by the dev merge, not by this PR. #3246 reserved
skills-reconcile-andretention-guard-(operator_queue_service.py:253-255), so those alarms are off the line now. I probed it: a reserved alarm row is not listed, but a legacy NULL-raiser row still is, with "replace it". Replacing it gets 422invalid_replaces. - (c) PR body. Still says "Stacked on #3253", "Draft until the chain is in", and
0090.
Blocking
-
Ask titles still reach people outside the owner and operators, on three allowlisted triggers. The allowlist decides by trigger label, but these labels also run turns whose reply goes to someone else:
operator_response. When the addressee answered in the Workspace, the resumed run takes their discussion chat as its destination. The run is told "Your final reply is posted to them in that chat, verbatim" (operator_resume_service.py:98, :136-161, :276).schedule(and a manual or webhook fire of that schedule). A schedule withdeliver_to_workspace_emailruns as that person's seat and posts its result to them (src/scheduler/service.py:1223, routers/internal.py:511-525).agent. A delegated child copies its parent's Slack or Telegram thread onto its own row and reports into it (chat_execution_service.py:1228-1243, :1705).
I probed
compose_system_promptwith each of these labels and an agent ask titled "Refund 900 USD to customer X": the full title is on the Pending asks line every time. I traced the delivery side in the code but did not run a real turn.Two weaker cases are on the same list.
mcpis also the inline email sign-in path, which the code itself treats as public-facing (mcp_auth_service.py:64, :355-378). Andchat/mcpare what a connector key produces (dependencies.py:745-760).Either fix is fine with me:
- (a) Drop titles from the line.
request_id (type, age)is enough for "don't re-ask", andget_my_askreturns the title when the agent needs it. This is the simplest fix, and the allowlist goes away with it. - (b) Keep titles, but decide by where the reply goes, not by the label. Show titles only when the trigger is on the list AND the execution row has no
source_channelor Workspace destination. That is the questionturn_audience.audience_ofalready answers (turn_audience.py:155-186).ExecutionContexthasexecution_idbut not the destination, so this needs plumbing plus a test for each of the three routes above.
This also answers your open question about
agent: going by the label alone, it can't count as owner-side.
Should fix
- The replace's ending write skips the lag check from the dev merge.
_note_commit_lag(operator_queue.py:31-52) arrived with the merge, and its docstring says it is "called by the five writers that end an ask". The replace CAS (operator_queue.py:505-521) and the expire-now in the same function (:534-538) are two more writers that stampdisposed_at, and neither calls it.test_ent815_queue_walk.py:296also walks only the five. The fix is one call after the replace transaction commits, plus adding the replace to that test's writer list. - (b), what's left. Keep the NULL-raiser rows on the line so it still equals the budget, but stop telling the agent to replace those entries. Mark them instead, e.g.
legacy-1 (approval, 7d, not replaceable). That needs one more selected column (raised_by) inlist_pending_asks_for_agent(operator_queue.py:994-1021), and it keeps the line-equals-budget pin. - PR body. Update the stacked, draft and
0090lines. Merge dev again.Done in2203d37ce. The only conflict wastests/registry.json.
Nits
- ask_service.py:958-961 still says the platform files alarms "with a NULL raiser and an unreserved prefix (
skills-reconcile-,retention-guard-, …)". Those prefixes have been reserved since #3246. - No test runs
0097'supgrade(). I emptied it and every test stayed green.pg-migrationscan't catch that either:0001_baselinebuilds the head schema, so on a fresh PostgreSQL0097does nothing. Only an existing PostgreSQL install ever runs that ALTER. Other revisions follow the same pattern. One test that callsupgrade()against a recordingopwould pin it. - The expire-now on a replace (ask_service.py:624-634) is announced with no broadcast trigger, and outside the poller cycle that normally sends one. So an open page shows the ask as pending until its next poll. It's small.
Not verified on my side: an alembic upgrade from 0096 on an existing PostgreSQL with rows (CI only upgrades a fresh database), full-suite seeds other than 12345, and real end-to-end delivery on the three routes in Blocking 1.
…orable secret value is a named 422 (#3325) (#3417) Fixes #3325 — independent fix, one of ten small bug fixes in the trinity-pm `chain-easy-1008` run. Base is `dev`. **Draft until the operator merges.** ## What - `src/backend/services/credential_encryption.py::decrypt`: every malformed envelope shape raises `ValueError`, as the function promises — JSON that is not an object (`[]`, `null`), and a non-string nonce or ciphertext. Importing such a `.credentials.enc` now returns **400** instead of a 500 with a stack trace in the log. The envelope format and every successful decrypt are unchanged. - `src/backend/services/secret_settings.py::encrypt_secret_setting`: the key is checked first, in its own `try`, and only that check can raise `MissingEncryptionKeyError`. A value that cannot be encoded (for example a lone surrogate) raises the new `SecretSettingValueError(ValueError)`. Its message names the setting, never the value. - `src/backend/error_handlers.py` + `src/backend/main.py`: one app-level handler maps `SecretSettingValueError` to **422**. `src/backend/routers/settings/credentials.py`: three pass-through lines in the routes whose catch-all would have turned it into a 500 (Anthropic, GitHub PAT, `PUT /slack`). - Tests: the three strict-xfail markers in `test_ec_credential_crypto_edges.py` are removed; new `tests/unit/test_3325_credential_error_types.py` (registered in `tests/registry.json`) asserts no value in the message or the exception chain, a missing key reported before a bad value, `PUT /api/settings/api-keys/anthropic` → 422, and that `main.py` registers the handler. ## Rulings carried (orchestrator, on the operator's behalf — plan file) - TD-1: one app-level handler plus the three pass-through lines — as recommended. - TD-2: a `.credentials.enc` that decrypts correctly but whose contents are not an object is deferred — as recommended; forging one needs the platform key. - TD-3: 422, matching the existing 422 for refused cleartext writes — as recommended. ## Review + security `/review` reading pass (claude-fable-5-1, report-only): **MERGEABLE**, no critical findings. The secret cannot leave through the new error: it is raised after the `except` block, so `__context__` is cleared, not merely hidden; the handler returns only the message; nothing logs it; audit rows are written only after a successful write. The only residual carrier is traceback frame locals, which nothing in the backend renders. The handler is keyed on the subclass, so no other `ValueError` is swallowed or relabelled. `/cso --diff`: no findings. Evidence sweep (claude-opus-5-5): **GREEN**. 23 unit files import the touched modules; the 16 most direct ran shuffled (seed 12345), one file per process, all passed. All 7 tests in the new file ran, 0 skipped. With both service files taken from `dev`, exactly the three formerly-xfail tests fail (8 cases). Seven less direct files are left to CI and named in the review file. `git merge-tree` against `dev` f6bedcd is clean. ## Tests Targeted counts above. Not run here: the full unit island (CI). ## Before merge - Startup auto-import (`lifecycle.py`) on a malformed envelope now lands in its `ValueError` retry arm: it retries without a warning per attempt and ends in the same "failed" result, with the existing error log after the last attempt. - Enterprise callers of `decrypt` get `ValueError` where they got `AttributeError` / `TypeError` on a malformed envelope. Two catch broadly, three propagate as before; no change needed there. - The new test file has a module-level `importorskip` for `sqlalchemy` and `cryptography`: it skips silently where those are missing. They are present on the engineer and in CI. - Ten open PRs share `tests/registry.json` or `src/backend/main.py` with this branch (#3412, #3411, #3407, #3304, #3271, #3256, #2984, #2956, #2713, #2709). A test-merge of this head against each adds no conflict beyond what that PR already has with `dev`. ## Handoffs (not in this diff) - TD-2 above: the decrypted-but-not-an-object case can still 500 on import. - `PUT /api/settings/slack` saves the client ID before a later secret write fails (partial update, pre-existing). - The GitHub PAT, Resend and Gemini routes are covered by the app-level handler but not exercised by the new route test. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…L is refused, not stored (#3323) (#3420) Fixes #3323 — independent fix, one of ten small bug fixes in the trinity-pm `chain-easy-1008` run. Base is `dev`. **Draft until the operator merges.** ## What - `src/backend/utils/url_validation.py::reject_embedded_credentials`: strips its input and treats a leading `//` as already having a host, the same rule `strip_url_credentials` uses. `//<token>@github.com/o/r` is now refused like the `https://` form, including with leading whitespace or mixed case. All three write paths that store a skills-library URL go through this one function (`routers/skills.py` create and update, legacy adoption in `services/skill_service.py`), so the token is no longer saved in plaintext to `skill_sources.url` or the audit details. - If `urlparse` refuses an input that starts with `//`, the function falls back to the module's existing userinfo regex instead of raising, so this fix adds no new 500. - Tests: the D3 strict-xfail marker is removed; four more refused shapes and a four-case "must not raise / must not over-reach" test are added. ## Rulings carried (orchestrator, on the operator's behalf — plan file) - TD-1: the fallback applies only to inputs starting with `//`; the bare `ValueError` for every other malformed input is #3322's and is unchanged (D4 stays a strict xfail) — as recommended. - TD-2: keep the username/password check; `//@host` carries no secret and is not refused — as recommended. - TD-3, TD-4: deferred, see Handoffs — as recommended. ## Review + security `/review` reading pass (claude-fable-5-1, report-only): **MERGEABLE**, zero critical, five informational. The pre-fix and post-fix modules were executed side by side over forty inputs: nothing that was refused before now passes, credential-free inputs are unchanged, the fallback regex is anchored and linear. `/cso --diff`: no findings. Fixed after review: - `546f329a` — the `tests/registry.json` descriptions no longer call #3323 a strict xfail. - `a75a5e04` — the reject stub in `test_ent183_skill_packages.py` mirrors the fixed `//` rule. It is deliberately stricter than production on a malformed bracket (`//[oops/x` raises). Evidence sweep after the fix (claude-opus-5-5): **GREEN**. 28 unit files import the module; the 14 most direct ran shuffled (seed 12345), one file per process. Three did not come back clean, none because of this branch: - `test_ent236_skills_lifecycle.py::TestNonRepoDirectoryRecovers::test_real_repo_still_pulls` and `test_skill_service_user_agent.py` (collection error) fail the same way on `dev` f6bedcd in the engineer's environment. - `test_ent237_skill_sources.py` hit the 240 s per-file limit at about 72 % with no failure printed — left to CI. `git merge-tree` against `dev` f6bedcd is clean. ## Tests 895 passed across the 11 files that completed cleanly. Not run here: the 14 less direct importer files, `test_ent237_skill_sources.py` to completion, and the full unit island (CI). ## Before merge - Ten open PRs share `tests/registry.json` with this branch (#3417, #3412, #3411, #3407, #3304, #3271, #3256, #2984, #2956, #2709). A test-merge of this head against each adds no conflict beyond what that PR already has with `dev`. - One stale docstring remains at `tests/unit/test_ec_url_validation_properties.py:36` (still calls D3 a strict xfail). ## Handoffs (not in this diff) - TD-3 (P3): shapes that still pass both reject and strip — `/\t/tok@`, `/\n/tok@`, `///tok@`, `https:/tok@` (`urlparse` removes tab and newline after the check). Fixing it needs the same change in `strip_url_credentials` and cases in both test families. - TD-4: a property test that reject and strip always agree over the shared `test_2052` corpus; it needs exceptions for empty userinfo. - #3322 (bare `ValueError` / `UnicodeError` → 500) is the sibling issue in the same file and is untouched. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…er reads as a rewrite (#3314) (#3414) Fixes #3314 — independent fix, one of ten small bug fixes in the trinity-pm `chain-easy-1008` run. Base is `dev`. **Draft until the operator merges.** The fix for #3385 (bounding `operator_queue.type`) is stacked on this branch. ## What - `src/backend/services/operator_queue_service.py` - `_canonical_options`: any falsy `options` (`[]`, `""`, `{}`, `0`, `False`, `None`) canonicalises to empty, the way `db/operator_queue.py` stores it (NULL). An entry written with `options: []` no longer reads as `changed:options`, so answers are no longer refused with 409 `item_diverged`. - New `_scalar_text`: a numeric or boolean `title` / `question` is turned into text in `_clamp_ingested_item` and in `_entry_content`, so what is stored and what is fingerprinted agree on SQLite and PostgreSQL. - `db/operator_queue.py` is untouched. - Tests: the strict-xfail markers on r88–r91 are removed, a boolean-title case is added, and the real-SQLite round-trip property test now covers falsy options and non-string titles and questions. ## Rulings carried (orchestrator, on the operator's behalf — plan file) - TD-1: a boolean title or question is stored as text (`"True"` / `"False"`) — as recommended. - TD-2: no number conversion before the #3243 title-length check — as recommended; scope stays narrow. ## Review + security `/review` reading pass (claude-fable-5-1, report-only): **MERGEABLE**, no critical findings. For every value the ingest path accepts, the stored row and the fingerprint agree on both databases; real rewrites of options, title and question are still detected; the 409 `item_diverged` path and the #3243 title hold are not in the diff. `_scalar_text` stringifies only bool, int and float, and its output is still clamped. `/cso --diff`: clean. Evidence sweep (claude-opus-5-5): **GREEN**. 56 test files reference the operator queue: 156 passed on the two changed files (shuffled, seed 12345) and 1,641 passed on the other 54, run one file at a time because several take 2–4 minutes each. With the fix reverted, six regression tests go red, including both tests the issue names. `git merge-tree` against `dev` f6bedcd is clean. ## Tests Targeted counts above. Not run here: the full unit island (CI). ## Before merge - **Rows stored before this fix:** an ask whose title or question was a boolean, or a float that SQLite renders differently from Python, stays flagged as rewritten on both SQLite and PostgreSQL. Integer titles recover. No migration. (The commit message says this for SQLite booleans only; this paragraph is the complete statement — review I1.) - Open PR #3256 (replace a pending ask) touches the operator queue; a `git merge-tree` of the two heads is clean. - Deliberately not detected as rewrites, because they store and display identically: `[]` ↔ `""` ↔ `None`, and `123` ↔ `"123"`. ## Handoffs - #3385 is stacked on this branch and edits the same two functions. - A null `type` is still out of scope here (r92 stays a strict xfail; #3315). 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…t the database (#3385) (#3421) Fixes #3385 — one of ten small bug fixes in the trinity-pm `chain-easy-1008` run. **Stacked on #3414** (the fix for #3314): base is `feature/3314-queue-fingerprint-falsy`, because both issues edit `_clamp_ingested_item` and the fingerprint helpers. Merge #3414 first, then retarget this to `dev`. **Draft until the operator merges.** ## What - `src/backend/services/operator_queue_service.py`: new `OPERATOR_QUEUE_TYPE_MAX = 64` and `_bounded_type`. An agent-written `type` longer than 64 characters is shortened with the existing `…[truncated]` marker at ingest (`_clamp_ingested_item`); the ask is never refused or lost. `_comparable_type` applies the same bound, so a shortened row does not read as a rewrite (#2915) and a row stored before this fix at full length still compares equal. - `src/backend/db/operator_queue.py::_insert_values`: `_DB_BELT_TYPE_MAX_BYTES = 1024`; above it the insert raises `ValueError`, like the id / title / question / context belts. All three create paths go through it. - Docs: five lines in `architecture/api-endpoints.md`, `feature-flows/operating-room.md` and `requirements/security.md` that list the clamp and belt fields now name `type`. - Tests in `tests/unit/test_1632_operator_queue_caps.py`: an oversized type is shortened with the marker; every platform-emitted type and every type `ask_operator` accepts is unchanged; the belt rejects an oversized type and passes 64 four-byte characters; `test_3385_clamped_type_is_not_a_rewrite` (clamped row vs raw entry, legacy full-length row, a real rewrite inside the first 52 characters). ## Rulings carried (orchestrator, on the operator's behalf — plan file) - T1: a fixed constant, no environment variable — as recommended. - T2: 64 characters — as recommended. The longest type the platform emits is `workspace_problem_report`, 24. - T3: the DB belt raises, like its siblings — as recommended. - Orchestrator: stacked on #3314 instead of branching from `dev`, so the two fixes do not hand the operator a conflict in the same two functions. ## Review + security `/review` reading pass (claude-fable-5-1, report-only, against the stacked base): **MERGEABLE**, zero critical. Census of every literal `type` the platform and the enterprise submodule pass to the insert paths: longest is 24 characters, none is altered. Every truncated value ends in the marker, which no set member can match, so truncation cannot move a value into or out of a set that decides control flow (`_BUDGETED_ALERT_TYPES` and neighbours). The new `ValueError` is reachable only from the file sync loop, which quarantines it under its existing handler. `/cso --diff`: no findings; the error text carries the constant, never the value. Fixed after review: `eab91c10` — I1, the five doc lines. Noted, not changed: I2, a non-string `type` over 1 KiB now raises the named `ValueError` where the base failed at the bind — same quarantine outcome. Evidence sweep after the fix (claude-opus-5-5): **GREEN**. 13 operator-queue files ran shuffled (seed 12345), one file per process: 836 passed, 7 xfailed. With both production files taken from the #3314 branch, the two `TestClampType` cases, the belt test and the clamped-row fingerprint case go red. `git merge-tree` against `dev` f6bedcd is clean; the base branch has not moved (`923b1edc`). ## Tests Counts above. Not run here: `test_ent815_queue_walk`, `test_ent815_broad_list_agent_scope`, `test_ent751_gate_entries` (2–4 minutes each; they ran green on the #3314 base in that PR's sweep) and the full unit island. ## Before merge - **This PR's checks are weaker than a dev-based PR's:** a PR whose base is a feature branch runs a reduced CI set and `backend-unit-test` does not run. The first full run happens when it retargets to `dev` after #3414 merges — read that run before merging. - Accepted trade-off: an agent that rewrites an oversized type only after character 52 is not detected as a change. Titles and questions already behave this way. - Thirteen open PRs share a file with this branch (mostly `tests/registry.json` and `docs/memory/architecture/api-endpoints.md`; #3256 also `db/operator_queue.py` and the three docs). A test-merge of this head against each of #3420, #3417, #3412, #3411, #3407, #3304, #3271, #3256, #2984, #2956, #2713 and #2709 adds no conflict beyond what that PR already has with `dev`. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…le (#3247) Re-review of #3256 (2026-10-08). The title allowlist decided by trigger label, but a label does not say who reads the reply: an `operator_response` resume answers the addressee in their Workspace chat, a `schedule` can deliver to a Workspace seat, and a delegated `agent` child reports into its parent's channel thread. The allowlist is gone; every entry is `request_id (type, age)` and the read behind the line no longer selects a title. `get_my_ask` returns it. Also: - the replace's compare-and-set and the expire-now beside it now run the ent#815 commit-lag check, like the five other writers that end an ask - a pending row the agent did not raise (no raiser, pre-#611) stays on the line, since it spends the same budget, but is marked `not replaceable`; the "replace it" hint is dropped when nothing listed can be replaced - the expire-now on a replace sends a thin, agent-scoped trigger, so an open page refetches instead of waiting for its next poll - a test runs `0097`'s `upgrade()` / `downgrade()` against a recording `op` - the `_replace_target` comment no longer calls `skills-reconcile-` and `retention-guard-` unreserved (#3246 reserved them) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Dismissed on the author's instruction to merge. The 2026-10-08 blocking point (ask titles on the Pending asks line) is fixed in 4ef09d1 by option (a): titles are off the line and the read no longer selects them. The same commit adds the commit-lag check to both replace writers, marks non-agent rows not replaceable, sends the refresh trigger and tests 0097's upgrade(). All checks green at that SHA. Not re-run by mutation here.
trinity-ability
left a comment
There was a problem hiding this comment.
Approved at 4ef09d1 on the author's instruction after /validate-pr: Lane C+schema, all checks green, one Alembic head against dev. The fix for the 10-08 review was confirmed by reading, not by mutation.
Fixes #3247 — third of the chain #3242 → #3243 → #3247. The base is
dev: #3253 and #3255 have landed, and this branch carries a merge ofdevon top.What
A scheduled run no longer has to re-ask what is already pending: it is shown its own open asks, and it can replace one.
request_id (type, age), oldest first, at most 8, then "and N more". It reads the same rows that count toward the open-ask limit, is omitted when empty, and never fails the turn. No entry carries the ask's title on any turn: a trigger label does not say who reads the reply, so the read behind the line does not select it, andget_my_askreturns it. A pending row the agent did not raise itself is listed asnot replaceable.replacesonask_operator. The predecessor ends ascancelled/disposed_by = agent/ reasonreplaced, the successor is created, and the two are linked (replaces/replaced_by) — one step inside the per-agent locked transaction. An approval is never changed in place.409 replaces_endedwith ids and enums only. A predecessor past its deadline is expired on the spot and the replace is refused, sosupersedes_expiredworks at once.raised_by = 'agent'can be replaced — never a platform-raised, gate or NULL-raiser row, never another agent's. Every such attempt gets one uniform422 invalid_replaces, and the compare-and-set repeats the check.already_pending. An agent-raised approval whose proposal equals one of the agent's own pending agent-raised approvals is refused with409 already_pending {request_id}unlessreplacesnames it. It runs inside the lock, applies to approvals with a non-empty proposal only, and never refuses or counts a gate or platform raise./m, the Workspace ask views,get_my_ask. The Workspace names the other ask only when the same person could already see it. No wake fires for the replaced ask.0097_operator_queue_replaceon0096_a2a_internal_scope).ask_operatordescription (1,762 characters, under the 1,800 test from feat(operator-queue): atomic asks — authoring rules in the ask_operator contract, option and title limits (#3243) #3253) and the full rule in thereplacesparameter; the prompt gains "do not re-ask what is still pending; if the facts changed, replace it".Rulings carried (orchestrator, on the operator's behalf — plan file)
already_pendingguard (added by the plan, not in the issue).request_idin a badge; the Operating Room has no deep link to a single item.Review + security
Correctness + security review (claude-fable-5-1, by reading): MERGEABLE, no critical finding. A separate wide test sweep then found what reading had missed:
b12152f9: thealready_pendingguard also refused the skill-approval gate's deliberate one-approval-per-occurrence raises (four ent#751 tests red on the branch, green on its base). The guard is now the agent's own. The same commit fixes an older bug the refusal exposed:skill_gate_servicereade.statuswhere the exception carriesstatus_code.cdd31c0f: the guard moved inside the lock (two concurrent identical raises could both insert); approvals with a real proposal only;replaces_endedcarriesdisposed_byand no longer calls every cancel an operator's; no ask titles on public and paid turns (later widened to every turn, see Review rounds); stale "person | timeout" strings.b8ad5960restoresdocs/user-docs/agents/recommended-fleet-prompt.mdto its base content: that page mirrors a canonical prompt owned elsewhere, and its sentence "Only a person ends an ask" is the owner's to reword./cso --diff: nothing supported.Tests
Reported by the builders on the fix tip: named sets 581 + 382 passed; the 43-file importer sweep in shuffled order 461 + 478 + 489 passed; mcp-server 697 passed,
tsc --noEmitclean; frontend unit 267 files / 4,670 passed (before the fix commits, which touch no frontend file). One pre-existing collection error at the base too:test_inter_agent_timeout_unit.py(dispatch_breaker_activemissing).Review rounds
02ec804de): titles decided by an allowlist of triggers instead of a denylist; the order-dependent collide test; a test that a replace never edits the old ask in place. The migration was re-parented to0097.4ef09d16b): the allowlist still showed titles on three triggers whose reply can go to someone other than the owner or an operator (operator_response,schedule,agent). Titles are now off the line entirely. The same commit adds the commit-lag check to the replace's two ending writes, marks rows the agent cannot replace, sends a refresh trigger when a replace expires an overdue ask, and tests0097'supgrade().Before merge
alembic upgradefrom0096on an existing PostgreSQL with rows (CI upgrades a fresh database only), and a real end-to-end turn on the Workspace, schedule-delivery and delegated-agent routes.db/operator_queue.py::list_pending_proposals_for_agent,database.py::list_pending_operator_queue_proposals,ask_service.AGENT_ENDING_REASONS.Handoffs
🤖 Generated with Claude Code