Conversation
github-actions
Bot
added
workshop/frontend
Preview:
|
![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.
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Submitted 3 actionable inline findings. |
69657eb to
ef484c8
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
ee99ed3 to
384ff68
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
384ff68 to
81c9da4
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Submitted 4 actionable inline findings. |
81c9da4 to
395cf4c
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Submitted 2 actionable inline findings. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
A cleanup pass over the branch, removing checks whose failing branch no code path can enter, and docs that restated a rule already stated where it belongs. `isMethodMissing` guarded against a coded Git pack error whose message resembled workerd's method-missing prose. The four pack messages are fixed strings that contain no such text, so the guard, its import, and the test that had to hand-mutate an error's message to reach it are gone. `GitPackBuilderImpl` rechecked `action` and `gatekeeperId` on a record it looked up by a map keyed on exactly those values, populated from this connection's own pending plan; both fields are immutable on a record. The dispose-time map clear was dead behind the `#active` flag. The fourth code, GIT_PACK_ACTION_DECLARES_NO_PUSH, is folded into "not authorized": the constructor filters on `pushedCommits?.length`, so an empty declaration never enters the map, and the contract already tells implementers to omit the field rather than pass `[]`. The next commit closes the one producer that could forward `[]`. `applyThrough` re-read the boundary's connection inside the queue and compared it to the pre-queue read; a record never changes connection. `reject()` re-read the record after the gatekeeper call, but the decision queue holds across that await, so nothing can decide it meanwhile. The legacy veto loop logged and rethrew a failure the run loop already logs and batch callers already receive. Docs: the publish-order rule was stated three times; `applyActionsThrough` now points at the normative text on `submitAction`. The hand-written function types on the pack error exports now match their siblings in api.ts. Tests: the two-scenario pack test is split so a failure names its scenario, the frontend card renderer is shared between the pending and resolved cases, and the RPC-safe `expectGitPackCode` helper gains a comment saying why it must not become `expect().rejects` -- handing an RPC-stub promise to vitest leaves an unhandled rejection behind in workerd.
The kit forwarded a provider's `pushedCommits` whenever it was truthy, and `[]` is truthy. A `describe()` that computed "commits to push" and found none would put `pushedCommits: []` on the wire, which the overseer reads as a push declaration: it verifies no ancestry and marks no objects for it, and the pack builder refuses to build for it, so the action could only stop at apply time with a message that did not name the mistake. Filter on length instead of presence. An empty list is "no git", the same as no key, and now produces the same wire shape. The kit was the only producer that could forward `[]`; the GitHub gatekeeper always declares one commit directly. The existing no-key test is parameterized over the empty list so the truthy hole stays pinned as observable wire behavior.
The offer's own comment says it appears only when enabling a rule would actually apply this action, and it checked three of the conditions for that: a tagged action, on a connection, that the gatekeeper marked auto-approvable. A recorded failure is a fourth, added later -- it disqualifies the action from the rule path, since nothing unattended may retry a side effect whose outcome the gatekeeper never confirmed. So on a stopped card the button still appeared, the confirm dialog still promised application, and enabling the rule left that action pending, with any agent turn awaiting it still suspended. Check the failure in both gates. The rule stays creatable from the auto-approval panel and Approve still works on the card, so nothing is lost except a promise that could not be kept. The backend is untouched: refusing to replay a stopped action unattended is the behaviour the offer was misreporting, not a bug in it.
09b2925 to
2ff9a51
Compare
|
@ndisidore Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
`ActionSyncStorage` restated the overseer's action schema by hand: a collection plus the two indexes the driver reads. A hand-mirrored shape can drift from the thing it mirrors, and the tests already build their storage from the production `makeOverseerStorage`, so the interface bought nothing the real type does not give. The `applyLegacyAction` hook existed for a narrower reason: the driver had no way to build an action-scoped `GitCacheImpl`, so the overseer supplied the whole call instead of just the cache. Derive the storage type with `Pick`, and give `createGitCache` the optional action id the legacy path needs so the driver can make its own call. Two hooks become one and the legacy call site now reads like the native one above it. Tests lose a seam they only had to route around: `makeDriver` no longer reimplements the apply, `putAction` defers to the shared fixture, and `rejectionOf`/`rejectBatchProbe` replace the try/catch and the hand-written missing-method TypeError that several suites had each spelled out.
The chat card and the Activity row each decided independently whether to offer "Always approve this type", with the same four-part condition written out twice. Keeping two copies in step by hand is what let them drift: when a recorded failure became a reason to withhold the offer, both copies needed the new clause, and the type the confirmation dialog consumes was declared inline in both files as well. `autoApproveTargetOf` now owns that decision, beside `actionStatusLabel`, which is the same move for the same reason. Each caller narrows to an action entry and asks. The rationale for each clause lives with the code that applies it rather than in a comment duplicated next to each copy.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Performance is inconclusive: comparable passes rose from 17/20 to 19/20. Project-doc improved 7/10→9/10 and ran 13.3% faster, while appointment-desk stayed 10/10 but slowed 32.4%; expense-ledger was incomparable. Project-doc more often kept placeholder details inside list items, avoiding baseline failures with bare labels. Ten trials per cohort leave substantial stochastic uncertainty. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Performance regressed on latency while correctness stayed unchanged at 23/30 passes. Mean duration rose 57.8% for appointment-desk and 16.7% for expense-ledger, while project-doc was flat. Appointment-desk added a model turn and more tool calls on average, with one candidate trajectory querying blueprints before creating the gadget; expense-ledger conversely reduced tool errors from 1.5 to 0.1. With only 10 trials per cohort, stochastic latency variation limits confidence. |
A gatekeeper that has already applied an action cannot honour a veto of it, and the contract told it to ignore the veto silently. The caller then acknowledges a rejection it never got: the record is written `rejected` for work the provider executed, and because `persistRejected` also runs `clearPushMarks`, a Git push additionally loses the `onRemote` proof that makes its objects re-pullable. The gatekeeper knows which it is; nothing asked it. Report those ids in `alreadyApplied` and reconcile them to applied. Reported in the result rather than thrown, matching `invalidatedByVeto`: a throw would abort every other veto and apply in the batch, and since the caller replays its staged vetoes it would throw again on each retry and strand them. The record keeps no resolver, because the pass that applied it lost its response before recording one and this pass only knows the vetoer, and it sheds any `failure` left by an earlier stop, since `ActionLogEntry.failure` is cleared when an action applies and both cards render the note whatever the state. Only ids this call actually sent are honoured, since vetoes beyond the frontier stay staged and undelivered by design. Inert until a gatekeeper implements `applyActionsThrough`, so populating it is an acceptance criterion of the first native port rather than something today's legacy path can exercise.
c6fe678 to
725b82d
Compare
Eval runs comparisonBaseline
|
Eval runs reviewMeasured. No cohorts are comparable; all three are marked “eval definition changed.” |
A veto the gatekeeper refuses because it had already applied the action reconciles the record from rejected to approved, and `vetoRefused` tells the caller why. Only `applyActionsThrough` reads it. `approveAction` and the background rule passes both ride staged vetoes too, and neither reports anything, so the card flips from Denied to Approved with nothing on it to say the user asked for the opposite. Throwing from those routes is not the answer: `approveAction` returns early once the clicked action reads approved, so the check would have to precede that return and fail a click that succeeded, over a reversal on a different card. Mark the record instead, and give the marked state its own label. The flag rides the same path as `cascadedFrom`, the label comes out of the one helper both surfaces already share, and no route needs to learn to report anything. `actionStatusLabel` now covers both states that would otherwise read as a verdict nobody gave: taken down by an earlier rejection, and applied before a rejection could land.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Eval runs reviewMeasured. No cohorts are comparable; all three are marked “eval definition changed.” |
Three gaps around rejecting an action, each reachable from the card. A veto the gatekeeper refuses as already applied reconciles the record to approved, so the resume gate read it as an approval and could resume the turn once its siblings applied. The user asked to stop; a late refusal does not change that. The gate now treats `vetoRefused` like a rejection. Rejecting an action also unblocks any rule-approved actions queued behind it, but nothing ran a pass until the next unrelated trigger, leaving their turns suspended. `rejectAction` now runs the same background pass-and-resume that enabling a rule does, shared as `#applyDecidedInBackground`. A cold reconnect re-fetched only pending cards, so a Denied card whose veto was refused while disconnected kept reading Denied. It now re-fetches every card that is not approved.
| // Deny leaves the turn ended, like denyConnectionRequest. The rejected record also prevents a | ||
| // sibling approval from resuming this turn. | ||
| // sibling approval from resuming this turn. Rule-approved actions it unblocked apply now. | ||
| this.#applyDecidedInBackground(action.gatekeeperId); |
There was a problem hiding this comment.
[P1] Resume with the suspended turn's original user context. This new pass runs through the rejecter's OverseerClientInterface: if Alice's await-decision action is auto-authorized behind a blocker and Bob rejects that blocker, the action applies under Alice's saved rule, but #resumeSuspendedAgent() resolves the original model ID through Bob's #clientUser and attributes the continuation to Bob. Bob may not have that model, leaving Alice's chat suspended, or a matching ID can run with Bob's model config and credentials. Persist/use the suspended turn's initiator user rather than the collaborator who happened to trigger reconciliation.
There was a problem hiding this comment.
Hmm so Bob can resume Alice's suspended turn without deciding anything about it. That turn then runs on Bob's model record and Bob's user id. It needs a workspace with several builders who share a connection, a rule, and an awaited action.
main does't really do any tracking here: I could add this with about 15-20 lines of kernel code but for now it may be pragmatic to defer
|
Submitted 1 actionable inline finding. |
Eval runs reviewMeasured. No cohorts are comparable; all three are marked “eval definition changed.” |
1 participant
This adds the shared
applyActionsThroughcontract and replaces the overseer’s auto-approval drainer withActionSyncDriver. The driver resolves pending actions in ordered passes, stages vetoes, records failures, and falls back to the existing per-action calls for gatekeepers that have not migrated. The publicapproveActionandrejectActionRPC signatures remain unchanged but are now depreciated.Frontend behavior is intentionally minimal and the primary difference is that action cards now render the gatekeeper's failure reason, and there is distinction between "Denied" and "Invalidated" (new label); the existing per-action controls stay in place, and batch UI application work will follow separately.
Design Decisions