feat(webapp): deterministic grounding facts and org-wide tokens for the dashboard agent - #4796
Open
kathiekiwi wants to merge 85 commits into
Open
feat(webapp): deterministic grounding facts and org-wide tokens for the dashboard agent#4796kathiekiwi wants to merge 85 commits into
kathiekiwi wants to merge 85 commits into
Conversation
…text Stops the assistant chat crashing to a full-screen error when the code-highlighting chunk fails to load. Retries twice with backoff, then renders plain text instead of throwing.
…d errors Extracts the lazy chunk-load factory as loadStreamdownRenderer so the plain-text fallback path is covered by a test. The fallback still re-raises the original error as an unhandled rejection so the deploy-skew asset-recovery reload can pick it up.
…s their organization
One read-only Lua script reads the base concurrency sets, every CK variant in ckIndex and the runningCounter together, so the run ids behind a queue's running count come with counts from the same snapshot.
Adds slotHolders and slotHolderFacts to the retrieved queue: which runs hold the queue's slots, their phase, and whether Redis membership matches the run's status. Both the Redis and Postgres reads degrade instead of failing.
The holder list can never claim completeness for a CK queue, so the contract now reports only what is provable: truncated when the cap was hit and unlistedRunning for dequeued holders that exist but aren't listed.
Pass slotHolders/holderResolution through from the queue live row when present, and ground the model on how to read them: name the holder when consistent, call out scheduler/run-state mismatches without saying leaked or stale, and never assert an executing run from runningNow alone.
Grounding block now covers partial resolution and unresolved holders, the none branch no longer asserts usage as fact, and the runningNow-mismatch clause fires only when holderResolution is complete.
Adds slotHolderFacts to withLiveState with the same independent gating as slotHolders/holderResolution. Grounding block covers admitted-vs- dequeued phase and prefers slotHolderFacts over comparing runningNow manually.
…ists slotHolders is never exhaustive by contract for per-key concurrency queues, so no field claims completeness. slotHolderFacts gains truncated/unlistedRunning as proof of unlisted holders, and its consistency can be unresolved (counts then unusable). Grounding rules and tests updated to match.
get_repo_info and read_file now surface the run-pinned deployment's dirty flag (built from a tree with uncommitted changes), and the source read ledger tracks it per-sha so evidence canonicalization can caveat citations instead of asserting an exact commit match.
A dirty run-pinned deploy and the clean tracked branch can share a sha. dirtyForSha was last-write-wins, so a later clean read of that sha erased the dirty caveat. Fixed to OR instead of overwrite.
…oundary The dashboard agent's delegated token now carries the organization alongside the environment. For a token with an organization, the request may name any environment in that org — re-authorized against the org and the user's membership — and the token's own environment is only the default. Tokens without one stay env-pinned.
…h alert The unsubscribe route read the environment off the token alone, so an org-wide token could subscribe an alert in a sibling environment but not remove it. It now resolves the environment the same way the other agent routes do, checked against the token's organization, which resolveAgentAlertContext requires its caller to pass.
Curated runs expose a computed wait (queued vs created basis, reliability flag) instead of raw timestamps the model had to subtract itself, mirroring dashboardAgentWatchRunChecks' queue-wait semantics. The webapp run presenter now selects queuedAt and derives queueWaitReliable from the raw status. Trace spans carry their spanId, and span evidence is validated against a per-turn span ledger (mirroring the source-read ledger) so a citation must come from this turn's trace read. Source evidence also gets a code-stamped dirty flag from the same ledger, fed by run-pinned/default snapshot dirtiness. Error groups expose a computed recurredSinceResolve instead of leaving the model to compare resolvedAt/lastSeen dates.
…akdown on get_queue The queue can show headroom while the environment is saturated, so the binding constraint may not be the queue itself. Add envConcurrency (limit, current) to QueueRetrievePresenter, guarded like slotHolders, and pass it through the dashboard agent's get_queue tool alongside the concurrency override breakdown (base/override/overriddenBy/overriddenAt) the route already returns but the tool was dropping.
… not the plain limit current >= limit is not the dequeue gate; it's current >= limit * burstFactor (burstFactor defaults to 2). Add burstFactor to EnvConcurrency and reword the get_queue description so the model reasons from the real gate instead of assuming current == limit means the environment is saturated.
Add optional scope/queueName/limit/current fields so the model knows which queue or env is saturated, instead of guessing from the page. Populated by the webapp from data already graded (no new queries), carried through verbatim by the dashboard-agent tool.
The bare-evidence test asserted the pre-validation contract without ever reading a trace. Now it drives get_run_trace for real before citing the span, plus a new case for a span id no trace read returned this turn.
Dedupe get_queue grounding between the tool description and the system prompt, and tighten verbose investigation/watch phrasing, to bring both prompt.chars ceilings back under budget without dropping any grounding rule.
Fabricates PG+Redis fixtures for S1-S6 and S10 of the dashboard-agent UAT scenarios in the local References/hello-world dev environment.
🦋 Changeset detectedLatest commit: d66edd1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 27 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- Restore process listeners in a finally block so a failed StreamdownRenderer test doesn't leak unhandledRejection listeners into later tests. - Import assertExhaustive from the core/utils subpath instead of the root. - Correct the envConcurrency.current comment: it's the displayed dequeued count, not the gated concurrency count, and can trail it. - Correct the dashboard-agent token comment: environmentId is the default, organizationId is the actual authorization boundary. - Cap the CK-variant scan in slotHoldersOfQueue's Lua so a queue with many concurrency-key variants can't turn a per-request read into an unbounded scan; report the cap via the existing truncated signal. - Carry queuedAt/queueWaitReliable through the run list presenter so list_runs computes wait the same way get_run does. - Print the seed script's ClickHouse fixture command as a heredoc so it survives copy-paste.
- The honesty rule's "single snapshot, never proof of a leak" no longer contradicts get_queue's own leaked/stale exception (both facts observed same-turn); it now points to that grounding instead of re-banning it. - The closing-message rule now also bans mid-sentence restatement of the card, reworded or not, not just opening with it.
Drop unused exports on internal slot-holder types, remove now-clean lefthook knip exemption, and route QueueRetrievePresenter's run lookup through runStore instead of the control-plane replica client.
# Conflicts: # packages/trigger-sdk/src/v3/createStartSessionAction.test.ts
Comment on lines
+342
to
+345
| spanLedger.recordTraceSpans( | ||
| runId, | ||
| curated.spans.map((s) => s.spanId).filter((id): id is string => typeof id === "string") | ||
| ); |
Contributor
There was a problem hiding this comment.
🟡 Cross-project span citations use wrong scope
After overridden get_run_trace reads a sibling environment, recordTraceSpans discards that target. Its accepted span evidence then links to the current environment.
Prompt for agents
Cross-project get_run_trace calls are authenticated against an ApiTarget, but SourceReadLedger records spans only by runId and spanId. canonicalizeEvidence later constructs trigger URIs from the current conversation's projectRef/environmentId. Extend the span ledger and evidence canonicalization to retain the resolved target project and canonical environment ID, then build the span URI from that recorded scope. Ensure identical run/span IDs in different environments remain distinct.
Was this helpful? React with 👍 or 👎 to provide feedback.
| runningReported, | ||
| // A CK-variant scan cap also makes the snapshot incomplete, same as a holder-list cap. | ||
| truncated: truncated === 1 || skippedVariants > 0, | ||
| unlistedRunning: Math.max(0, runningReported - dequeuedCount), |
Contributor
There was a problem hiding this comment.
🟡 Truncated holder count hides omissions
When the holder cap truncates dequeued runs, unlistedRunning subtracts the uncapped dequeuedCount. It reports zero omitted runs although the returned list excludes them.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+10
to
+14
| const mocks = vi.hoisted(() => ({ | ||
| authorizeWatchEnvironmentById: vi.fn(), | ||
| submitDashboardAgentWatch: vi.fn(), | ||
| findEnvironmentBySlug: vi.fn(), | ||
| })); |
Contributor
There was a problem hiding this comment.
🔍 New tests rely on mocks
Several added tests use vi.mock, vi.fn, or stubbed fetches. Repository rules require testcontainers and prohibit mocks.
Was this helpful? React with 👍 or 👎 to provide feedback.
…model Anthropic's newer models reject a request whose conversation ends with an assistant message. A failed turn leaves its own failure record last, and a resend of the same message id merges onto the user message already in history instead of appending after it, so every retry sent that record as a prefill and failed again. prepareMessages now drops a trailing assistant message.
Comment on lines
+570
to
+572
| // Mirrors dashboardAgentWatchRunChecks.describeRunWait: a resumed/retried/paused run's | ||
| // queuedAt is a leftover from an earlier enqueue, not this attempt's wait. | ||
| queueWaitReliable: run.queuedAt !== null && !STALE_QUEUED_AT_STATUSES.has(run.status), |
Contributor
There was a problem hiding this comment.
🟡 Retried runs report stale queue waits
After a resumed or retried run leaves its waiting status, queueWaitReliable becomes true although queuedAt still belongs to the first attempt. The agent reports that old interval as the latest queue wait.
Was this helpful? React with 👍 or 👎 to provide feedback.
The card-vs-sweep rule conflict let the model skip the investigation card on a not-found subject: it swept list_projects/list_environments to resolve scope, then read the phase-budget ban on those calls as blocking the card too. Legalize the resolve sweep as part of gather, narrow the post-card ban to after the card opens, and require a card (inconclusive) for a not-found subject too.
Contributor
Observability mapAs of 20/100 over 454 measured of 472 entry points (base 20, no change) What this PR changed FIX FIRST
AUDIT 3 of 50 sensitive mutations record an actor. 47 without one. What the score is made ofThe score and findings here are report-only and never gate the merge. Separately, a required test suite keeps this tool's symbol and route lists in sync with the code they name, and can fail a pull request that renames or removes a symbol they reference, or that adds the first route with a segment they anticipate. Each failure names the list to edit. The rules and their reasons: internal-packages/observability-map/README.md. |
The generated chat name is best-effort, but the await that collects it is the last gate before a turn completes and the provider request behind it has no deadline of its own. A request that stalled rather than failed hung the whole turn after the model's final word: nothing streamed, nothing was saved, and the run sat there until it was killed. The name now has a deadline; a late one still lands on the row.
…turn A tool's exception is handed to the same hook a stream failure is, so a single failed call marked the turn failed: the agent worked around it and answered, and the user still got "Something went wrong" under a finished answer. The model was also told the turn was over instead of what happened to that call, so it was flying blind on the retry, and the exception itself was never logged. A failed call now reads as a failed call, is logged, and leaves the turn alone; only a stream failure ends it.
Contributor
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 1 new potential issue.
1 security issue not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
Comment on lines
+25
to
+26
| if (previous === null || previous.chatId === activeChatId) return previous; | ||
| return { chatId: previous.chatId, expiresAt: now + TOOL_PENDING_DEADLINE_MS }; |
Contributor
There was a problem hiding this comment.
🟡 Chat switching prolongs stale activity
Each switch between other chats makes markerAfterActiveChat restart the detached chat’s expiry. Frequent switching can keep a finished chat marked busy indefinitely.
Suggested change
| if (previous === null || previous.chatId === activeChatId) return previous; | |
| return { chatId: previous.chatId, expiresAt: now + TOOL_PENDING_DEADLINE_MS }; | |
| if (previous === null || previous.chatId === activeChatId) return previous; | |
| return previous.expiresAt > now | |
| ? previous | |
| : { chatId: previous.chatId, expiresAt: now + TOOL_PENDING_DEADLINE_MS }; |
Was this helpful? React with 👍 or 👎 to provide feedback.
Contributor
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 1 new potential issue.
1 security issue not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
Comment on lines
+313
to
+322
| async function withinTitleDeadline(pending: Promise<void>): Promise<void> { | ||
| let timer: ReturnType<typeof setTimeout> | undefined; | ||
| const deadline = new Promise<void>((resolve) => { | ||
| timer = setTimeout(resolve, locals.get(dashboardAgentTitleDeadlineKey) ?? TITLE_DEADLINE_MS); | ||
| }); | ||
| try { | ||
| await Promise.race([pending, deadline]); | ||
| } finally { | ||
| clearTimeout(timer); | ||
| } |
Contributor
There was a problem hiding this comment.
🔍 Title timeout does not cancel generation
Promise.race releases turn settlement but leaves the provider request running. A stalled request can retain resources beyond the 15-second deadline.
Was this helpful? React with 👍 or 👎 to provide feedback.
Rename slotHolderFacts.consistency to counterAgreement, add the always-true ckAdmittedMayBeUnlisted, and report env-scoped admitted concurrency, so an all-zero holder list no longer reads as an idle queue.
Contributor
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
1 security issue not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
Comment on lines
584
to
588
| const workers = await apiGet( | ||
| origin, | ||
| `/api/v1/projects/${projectRef}/${environmentName}/workers/current`, | ||
| `/api/v1/projects/${effectiveProjectRef}/${effectiveEnvironmentName}/workers/current`, | ||
| userActorToken! | ||
| ); |
Contributor
There was a problem hiding this comment.
🟡 Sibling queue consumers stay unresolved
For an overridden custom queue, apiGet calls an environment-pinned worker endpoint with the org token. The rejected read silently omits its consumer tasks.
Prompt for agents
Cross-project get_queue reads live state through a target environment JWT, but its custom-queue consumer lookup calls api.v1.projects.$projectRef.$env.workers.$tagName with the delegated user-actor token. That route calls authenticatedEnvironmentForAuthentication without organizationScoped, so any target other than the token's original environment is rejected and tool-api.ts silently returns the queue without consumerTasks. Add an appropriately authorized org-scoped read path for this endpoint or perform the lookup through the target environment JWT, preserving membership and capability checks. Cover a sibling-project custom queue with a real route-level authorization test.
Was this helpful? React with 👍 or 👎 to provide feedback.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Grounds the dashboard agent's answers in deterministic platform facts instead of LLM inference, and widens its token to the organization.