feat(webapp): deterministic grounding facts and org-wide tokens for the dashboard agent - #4796
Open
kathiekiwi wants to merge 105 commits into
Open
feat(webapp): deterministic grounding facts and org-wide tokens for the dashboard agent#4796kathiekiwi wants to merge 105 commits into
kathiekiwi wants to merge 105 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: e07a4c6 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 |
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.
![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.
Move the sweep-to-card rule into the not-found rule itself, where the model reads it, and forbid re-aiming the answer at another run or queue.
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 3 new potential issues.
1 security issue not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
Comment on lines
+5703
to
+5704
| local admitted = redis.call('SMEMBERS', scopeKey .. ':currentConcurrency') | ||
| local dequeued = redis.call('SMEMBERS', scopeKey .. ':currentDequeued') |
Contributor
There was a problem hiding this comment.
🔴 Slot-holder reads can block Redis
Large queues make slotHoldersOfQueue run unbounded SMEMBERS calls inside Redis Lua. One dashboard read can delay unrelated queue operations.
Prompt for agents
The slotHoldersOfQueue Lua script caps only the returned holders, but each collect call first executes SMEMBERS over both complete sets. It repeats this for the base queue and up to 50 concurrency-key variants, so the cap does not bound Redis work or memory. Redesign enumeration to perform bounded member reads, while preserving enough count-only reads to report admittedCount, dequeuedCount, truncation, orphan consistency, and skipped variants accurately. Add a Redis test with sets larger than the holder limit to verify the script does not enumerate every member.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+75
to
+76
| const identity = textIdentity(message); | ||
| return identity === null || !currentTextIdentities.has(identity); |
Contributor
There was a problem hiding this comment.
🟡 Repeated failures lose their record
A repeated failed retry makes textIdentity match an older fixed apology. The new turn's failure record is then discarded.
Suggested change
| const identity = textIdentity(message); | |
| return identity === null || !currentTextIdentities.has(identity); | |
| const identity = textIdentity(message); | |
| const role = (message as { role?: unknown }).role; | |
| return role !== "user" || identity === null || !currentTextIdentities.has(identity); |
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+377
to
+386
| await withTimeout( | ||
| startDashboardAgentHeadStart({ | ||
| chatId, | ||
| messages: [firstMessage], | ||
| mode: repoSnapshot ? "code" : "assistant", | ||
| metadata: headStartMetadata, | ||
| }), | ||
| CHAT_CREATE_TIMEOUT_MS, | ||
| "Dashboard agent head start" | ||
| ); |
Contributor
There was a problem hiding this comment.
🟡 Timed-out starts can run invisibly
A late withTimeout result continues after its chat is deleted. Retrying can process the same message twice while hiding the first run.
Prompt for agents
The new 20-second timeout races the still-running startDashboardAgentHeadStart or startDashboardAgentSession promise. On timeout, the catch soft-deletes the chat, but the upstream create/trigger can commit afterward and process the first message invisibly. Make timeout cancellation or reconciliation part of the start protocol. For example, propagate a reliable abort signal if the API guarantees cancellation, or retain/reconcile the chat when the late idempotent start succeeds. A user retry must not create a second processing run for the same first message.
Was this helpful? React with 👍 or 👎 to provide feedback.
…ead tool Every environment-bound read now accepts project/environment/branch, with the chat's own scope as the default rather than the limit: list_tasks, list_errors, get_query_schema, run_query, get_report, list_deploys and get_deploy join the tools that already had it, and the source tools pass the target through the run-snapshot resolution. run_query's POST goes out on the target's env JWT. Preview and dev branches are targetable as environment "preview"/"dev" plus the branchName list_environments returned, forwarded as x-trigger-branch on both the JWT exchange and the delegated-token routes. A target naming a project that list_projects did not report this turn is refused before any exchange. That is client hygiene to save a pointless round trip, not a boundary: authorization stays server-side on the exchange and every route. Prompt prefix grows to 80,937 chars (assistant) / 88,550 (code); the ceilings and the committed measurement move with it.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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 3 new potential issues.
1 security issue not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
Comment on lines
+262
to
+263
| execute: async ({ runId, project, environment, branch }) => { | ||
| const snap = await snapshotFor(runId, { project, environment, branch }); |
Contributor
There was a problem hiding this comment.
🔴 Targeted source reads use wrong project
When get_repo_info targets another project without a run ID, it returns the current project’s snapshot. Subsequent source answers inspect the wrong code.
Prompt for agents
Cross-project source inputs are accepted by get_repo_info, list_files, read_file, and search_code, but repo-tools.ts snapshotFor returns defaultSnapshot whenever runId is absent and ignores the target. Add a target-aware resolver for a sibling project's branch-head snapshot, or reject target fields unless runId is supplied. Ensure every source tool and the read ledger use the same resolved snapshot and scope.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+262
to
+263
| execute: async ({ runId, project, environment, branch }) => { | ||
| const snap = await snapshotFor(runId, { project, environment, branch }); |
Contributor
There was a problem hiding this comment.
🟡 Sibling source access depends on current repo
Targeted get_repo_info exists only when the current project has a repository snapshot. Sibling code remains unreachable when only the sibling has one.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+181
to
+182
| const { slotHolders, slotHolderFacts } = await this.#slotHolders(environment, queue.name); | ||
| const envConcurrency = await this.#envConcurrency(environment); |
Contributor
There was a problem hiding this comment.
🔍 Queue detail latency expands substantially
Every queue detail request now adds a holder scan, database lookup, and environment reads. This affects the dashboard route beyond agent requests.
Was this helpful? React with 👍 or 👎 to provide feedback.
…ironment Four post-acceptance fixes: schedule_watch tells the model to use get_run's actual queue name, never guess task/<taskId>; branch without environment is now a plain tool error instead of silently reading the chat's own env; a stale "sweep" test describe/comment is renamed; locate's not-found guidance no longer licenses "not a scope or permissions issue" claims.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Repo tools (get_repo_info/list_files/read_file/search_code) and the source-read ledger skipped the branch-needs-environment check; add it at their shared snapshotFor chokepoint. curateRun/curateRuns dropped `queue` even though schedule_watch now tells the model to use it — the field wasn't in the API response at all, so add it to the run presenters (ApiRetrieveRunPresenter, ApiRunListPresenter) and surface it in curation.
Add queue to CommonRunFields so RetrieveRunResponse/ListRunResponseItem carry it, document it in the OpenAPI schema, and omit it (rather than sending an empty string) on a buffered run that doesn't have one yet.
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
+158
to
+165
| // A per-call override of which project/environment a data lookup targets, for reads | ||
| // that cross into another project of the same organization. Omitted fields fall back | ||
| // to the context's own project/environment. `branch` picks a preview/dev branch out of | ||
| // the family its name addresses; without it the name resolves to the parent. | ||
| export type ApiTarget = { projectRef?: string; environmentName?: string; branch?: string }; | ||
|
|
||
| /** A tool call's explicit target: the project, environment and branch it names. */ | ||
| export type TargetInput = { project?: string; environment?: string; branch?: string }; |
Contributor
There was a problem hiding this comment.
🔍 Target scope is fragmented
API reads, source reads, watches, URIs, and evidence each reconstruct targets separately. A shared resolved target would prevent further cross-project drift.
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.