You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
add durable repository/issue/author-bound trust decisions with idempotent trust and untrust actions
fail closed before start, resume, continuation approval, every plan-item prompt, and every publish attempt; recheck the bound decision at prompt construction and actual push/open/refresh boundaries
include every bounded issue comment only for a matching explicitly trusted author and record the exact comment count plus SHA-256 digest on each execute attempt
add Trust this issue / Trust all comments / Remove trust controls with focus preservation, stale-response guards, independent overlapping updates, durable error ownership, and local demo support
use one settlement-inclusive deadline for sequential issue access/text reads and revalidate issue authors plus included collaborator-comment authors at the prompt and publish boundaries
Revocation affects the next admission, prompt construction, or irreversible publish boundary; it does not interrupt an agent invocation already running.
A collaborator author is eligible without a local decision, while an explicit author-bound decision widens the prompt to all bounded comments.
Publishing is guarded in addition to the issue acceptance minimum so untrusted work cannot be pushed or published.
The runner view suppresses controls when the latest complete issue board knows trust is blocked; action-time reads remain authoritative.
One issue-read operation owns a 30-second wall budget, including the subprocess shutdown and pipe-drain tail.
These decisions are also recorded in docs/implementation/issue-prioritization.md and docs/architecture.md.
Validation
Validated head: 3e6c43f34aad3fe0eaac0e4d4211c4f3bbb0482b (based on origin/main6dd57c884a932d2c25aaf9e487751bc77cdcee09)
final independent exact-head review — no concrete findings
final Copilot exact-head review — no findings
CI test (push) — passed on validated head
CI test (pull_request) — passed on validated head
Agent isolation real-docker — passed on validated head
Review rounds
Independent review found publish retries bypassing trust revocation, stale replay views, and known-untrusted controls. Fixed all with retry admission, fresh rendering, and board-derived suppression.
Independent and Copilot rounds found revocation windows around publish checks and Git reads, cross-issue UI overwrites, shutdown ownership gaps, and access-failure misclassification. Added immediate external-boundary guards, target-only merges, abort-and-await ownership, and durable upstream failures.
Review found authorization-to-text and per-item prompt handoff gaps. Added author/access snapshot revalidation, exact comment evidence, and prompt-boundary validation for every item.
Review found stale refresh/trust response orderings. Trust responses now merge only strictly newer author-bound trust fields; deterministic tests cover both response orders and fixed-clock trust/untrust/retrust transitions.
Copilot found saved runner outcomes were rejected during shutdown. Durable replay now precedes shutdown guards; independent review added real partial-request shutdown coverage.
Copilot found issue-link focus loss, equal-version collaborator-state overwrite, and inaccurate included-comment size wording. Fixed and independently re-reviewed clean.
Copilot found stale issue authors across collaborator pagination. Coherent issue/collaborator/issue reads now guard runner, planning, prompt, and publish boundaries; independent review additionally found and fixed removed included-comment authors.
Copilot found doubled sequential issue-read budgets and overlapping UI errors being erased. Added a shared settlement-inclusive deadline and generation-owned rendered trust/refresh errors; independent review added late-success refusal and refresh reconciliation watermark coverage.
Final Copilot exact-head review returned no findings. The required post-Copilot independent exact-head pass also returned no concrete findings.
No findings were declined. All findings are covered by existing AGENTS.md rules: async lifecycle ownership and per-item currentness, async review UI compare-and-swap/focus preservation, guarded external-action revalidation, overall deadline budgeting, idempotency replay before guards, and review-readiness regression requirements. No new rule would add information rather than repeat those rules.
The reason will be displayed to describe this comment to others. Learn more.
Review of 19fd41f. There are two blocking findings: trust revocation does not stop the remaining items of a run, and overlapping trust requests break the Issues screen. Three findings are smaller: missing tests, read failures labelled as refusals, and the approve-continuation gate.
Checks that passed:
Publishing: every publish path goes through #schedule admission: the action, startup publish, publish after a run and retries.
Push guard: placed after cat-file/ls-remote, with no await before git push.
Schema v17: migration is idempotent and tested.
Repository scope: keys are lower-cased.
Comment digest: covers exactly the prompt's comments array.
Return replayed outcome after concurrent issue access
web/server.ts:304
Two identical requests can both miss the replay check and overlap during issueAccess. If one commits success before the other's read fails, userAction returns that saved success here, but the unconditional throw still sends the second caller a 502. Return the replayed response so every caller observes the first durable outcome.
Persist issue-access failures for idempotent retries
web/server.ts:561
An issue-access failure bypasses userAction, so no outcome is saved under the action ID and retrying the same trust/untrust request performs a fresh GitHub read. Record the UpstreamFailure through userAction (while leaving shutdown aborts resendable) to preserve durable idempotency.
Addressed the two summary-only findings in 09f4668: - Concurrent runner action access reads: the late failing caller now returns the first durable userAction outcome instead of unconditionally throwing 502. The deterministic regression overlaps identical start requests, commits one success, then releases the other read failure and asserts both callers receive the same durable result with one attempt. - Trust/untrust access failures: definite UpstreamFailure outcomes now go through userAction and replay as 502 without another GitHub read; shutdown/abort remains resendable. The endpoint regression changes the gateway to success before replay and still observes the saved 502 with one access read. Focused validation on this head: typecheck and diff check passed; 8 files / 545 tests passed. Independent re-review found no remaining concrete issue.
Automated re-review gate for validated head 09f4668: author self-review complete; independent concurrency review complete with no remaining findings; focused validation passed 545 tests across 8 files plus typecheck and diff check; all three exact-head CI jobs are in progress; GitHub reports MERGEABLE with UNSTABLE only because checks are pending; unresolved review threads are 0; deferred follow-up issues are none.
Record comment evidence only at the agent launch boundary
runner/execution.ts:185
This records comment evidence before workspace materialization, link snapshots, the pre-launch tree check, and start(). Any preparation failure, cancellation, or stale-context stop therefore leaves a non-null digest even though no agent invocation received the prompt, so the audit record cannot reliably answer which comments the agent was given. Record the evidence at the launch boundary (or add an explicit prepared-vs-delivered state) and cover a pre-launch failure regression.
Addressed the current-head summary-only review concern in fca4c4baf8b0eadae8f5abf2646c9e2c7f78a983. Comment evidence is now persisted only after the launcher returns an owned handle. Preparation and synchronous launcher failures leave it null; an evidence-write failure cancels and awaits the live handle and retains start-not-saved.
Pre-review gate:
Independent review loop: converged with no findings.
Exact-head local validation: typecheck; 69 test files, 1,807 passed and 1 skipped; 11/11 Issues browser tests.
Current-head CI: push and pull-request test jobs passed; real-docker in progress.
Live PR state: mergeable; all 15 inline review threads resolved.
Deferred follow-ups: none.
Requesting another Copilot review round for this exact head.
Addressed Copilot round on aee97fd in e473f07. The post-review independent loop found two further race/coverage gaps: inverse refresh ordering could restore old metadata, and same-author cross-client decisions needed a CAS version. The final design overlays trust fields only, orders decisions with monotonic trustChangedAt, and covers both response orderings plus a frozen-clock trust → untrust → retrust sequence. Final independent round: no findings; typecheck, Store/board 59/59, issue browser 15/15, and diff check passed.
The shutdown guard runs before the durable replay lookup, so a previously completed start/resume/continuation/publish action returns 503 once the coordinator is closing instead of replaying its saved outcome. Replay the action first, and only reject an unsaved action for shutdown.
Addressed the Copilot summary finding at exact head 03465cb: trust-gated runner actions now replay a matching durable outcome before applying either the server-stopping or runner-admission shutdown guard. Added deterministic regressions for both runner admission closure and a partially received identical request that completes during real server shutdown; the latter reopens the database and proves no duplicate attempt was created. Fresh unsaved actions still return retryable 503. Validation: test/runner-start.test.ts 53/53, typecheck, and diff-check all pass. Independent review round 1 found the missing real-shutdown coverage; round 2 found no remaining issues. No findings were declined.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
Equal-version responses can restore stale collaborator state, and trust completion can drop keyboard focus from issue links.
Review effort: Balanced Findings: None
Previously missed (3)
In code that hasn't changed since last review
Preserve focus on any table control during pending trust updates
web/public/app.js:878
This preserves focus only while it remains on a trust button. If a keyboard user tabs to an issue-title link while the request is pending, the response rebuilds the entire table and removes that focused link, dropping focus to the page. Preserve any focused control in the issues table (or update only the affected row) before replacing its DOM.
Reject equal trust versions to prevent stale decisions overwriting refreshes
web/public/app.js:890
Require a strictly newer trust decision here. A delayed untrust response can carry the same trustChangedAt as a newer refresh but an older collaborator-derived trust value; >= lets it overwrite the refresh and show “Collaborator” after that author has lost collaborator access. Equal versions represent the same durable decision, so the current row’s fresher issue metadata must win.
Correct size-error text for outside or ghost-authored comments
github/issues.ts:279
When explicit trust includes an outside or ghost-authored comment, the size failure still says only “collaborator comments” were too large. That misidentifies the newly admitted content; describe these as the included comments instead.
Addressed all three Copilot summary findings at exact head cc75957: (1) issue-table rerenders now preserve focus for both trust controls and title links, with a controlled pending-response browser regression; (2) equal trust-decision versions no longer replace fresher collaborator-derived state, with a regression that asserts the equal intermediate versions before releasing the stale response; and (3) explicitly trusted outsider/ghost comment overflow is accurately described as included comments. Validation: issue unit tests 52/52, issue browser tests 17/17, typecheck, and diff-check pass. Independent review confirmed all three findings, found no additional issue, and its post-fix pass found no remaining concrete findings. No findings were declined.
Post-Copilot independent loop for c74247c: the reported issue-author pagination race was confirmed and fixed. Independent review then found the analogous included-comment-author race; that was also fixed, regressed, and re-reviewed to a clean pass before this next Copilot request. Focused evidence: 136/136 issue+publishing tests, typecheck, and diff-check. No findings were declined.
Use one overall timeout budget for sequential issue read stages
runner/production.ts:252
The admission and text stages each start a separate 30-second timeout, so every plan item can spend about 60 seconds on this “bounded” issue read before workspace preparation begins. Share one outer deadline signal across both calls so sequential stages consume one overall budget.
Share one timeout deadline across issue access and text reads
web/planning.ts:45
issueAccess and issueText each receive a fresh 30-second allowance, so one planning issue read can run for roughly 60 seconds (plus subprocess shutdown grace). ISSUE_READ_TIMEOUT_MS is documented as bounding the whole read; create one deadline signal before the first stage and share it across both calls.
Preserve independent trust errors during overlapping requests
web/public/app.js:941
A successful overlapping trust request calls renderIssuesWithPending(), which resets #issues-status to the board status. If another issue's request failed first, this later success erases its “Could not trust…” error, so independent overlapping actions can hide failures. Track action errors per issue/generation (or render them per row) and clear only the error owned by the action that later succeeds or retries.
Addressed the three Copilot summary findings at exact head 3e6c43f, plus the related gaps found by independent review. Runner and planning now share one deadline signal across access/text stages, abort active GitHub work at 29.25s, await the 750ms subprocess settlement tail inside the 30s wall budget, preserve caller cancellation, and reject even a late successful callback. Trust and refresh failures are rendered state with per-issue generation/completion ownership: unrelated successes cannot erase them, successful refresh reconciles only older errors, and failures completing during refresh survive. Planning docs now describe explicit author-bound all-comment trust accurately. Focused evidence: 104/104 unit tests, issue browser 20/20, typecheck, diff-check; final independent pass clean. No findings were declined.
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
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.
mchwang
left a comment
Summary
Relates to #108.
Decisions
These decisions are also recorded in
docs/implementation/issue-prioritization.mdanddocs/architecture.md.Validation
Validated head:
3e6c43f34aad3fe0eaac0e4d4211c4f3bbb0482b(based onorigin/main6dd57c884a932d2c25aaf9e487751bc77cdcee09)npm run typechecknpx vitest run --project parallel --maxWorkers=2— 69 files passed; 1,827 tests passed; 1 skippednpx playwright test test/browser/issues.spec.ts— 20 passedgit diff --check origin/main...HEADReview rounds
No findings were declined. All findings are covered by existing
AGENTS.mdrules: async lifecycle ownership and per-item currentness, async review UI compare-and-swap/focus preservation, guarded external-action revalidation, overall deadline budgeting, idempotency replay before guards, and review-readiness regression requirements. No new rule would add information rather than repeat those rules.Deferred follow-up issues: none.