Repository navigation
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
clawsweeper
Bot
added
clawsweeper:intake:feature
|
Codex review: needs maintainer review before merge. What this changesThis PR connects Agent revision history to its detail panel and separates Agent-wide Workspace files and Access navigation. Example: An operator views revision v6 while v7 remains selected for service.
Review scores
ProductKind: Feature · Worth it: Yes · Fix scope: Complete Merge readiness✅ Ready for maintainer review Keep open: the prior accessibility finding is fixed, and no additional source defect was found. The reviewed head has a failed Console Browser check whose cause needs triage before landing. Priority: P2 Before mergeNone. FindingsNone. Agent review detailsHow this fits togetherThe Console Agent page receives authorized Agent, Configuration, revision, and deployment responses and renders navigation, snapshot details, and existing draft controls. flowchart LR
Operator --> Console
Console --> RevisionNavigation
RevisionNavigation --> RevisionDetails
Console --> AgentSections
RevisionDetails --> ControllerAPI
AgentSections --> ControllerAPI
Technical reviewBest possible solution: Retain the integrated revision workspace and resolve the failed browser check before landing. Do we have a high-confidence way to reproduce the issue? This is a navigation feature; current-main source and the before screenshot establish the separate picker and flat tab layout it replaces. Is this the best way to solve the issue? The change fits the existing Agent page owner and preserves API, authorization, draft, and deployment handlers without adding a parallel subsystem. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 27ca6d843ea7. Provenance checked
TestingProof path: in-process harness. SecurityNone. EvidenceWhat I checked:
Likely related people:
Review metrics
LabelsLabel changes:
Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment HistoryReview history (1 earlier review cycle)
Reviewed October 29, 2026, 2:25 PM ET / 18:25 UTC (Revision 2). |
freeqaz
left a comment
freeqaz
left a comment
There was a problem hiding this comment.
Free's clanker here (an AI agent working on Free's behalf). This is a comment-only review of head 29e94513d2. The merge-tree with main is clean, and CI Required and First Agent Smoke are green. Three findings, two of them confirmed in a browser probe on a remote test VM (a throwaway test file, not pushed):
1. Medium: switching between Revisions and Workspace files/Access while scrolled makes the page jump (apps/controller/src/console/agents/detail.mts:3879-3891)
renderTab anchors the scroll on tabs.getBoundingClientRect(), which is the revision tab strip. setLayoutShape now hides that strip for workspace/access, so on one side of the switch its rect is all zeros and moved becomes the strip's whole offset. Probe at 1440×600 on a draft:
- At scrollY 400, clicking Workspace files moves the page to 221, and the Agent sections strip drops 179 px. Clicking Revisions then moves it to 551, and the strip ends at -165 px, so the tab just clicked scrolls out of view.
- At scrollY 150, clicking Workspace files jumps to 0.
On main, Workspace/Access were in the same strip, so the anchor held. Suggested fix: when either the old or the new tab is agent-wide, anchor on agentScopeTabs (or skip the compensation). It would help to add a scrolled Workspace⇄Revisions case to "Agent tabs replace only their content…" in console-agent-detail-2.test.mjs.
2. Low: duplicate DOM id (detail.mts:1408)
versionLayout.id = `${tabIds}-workspace` is the same id that the loop gives the Workspace files tab button (${tabIds}-${id}). The probe found one id on two elements. getElementById on the Revisions tab's aria-controls returns the Workspace tab button, and the Workspace tab's aria-controls points at itself. Any distinct id (for example ${tabIds}-sections-panel) fixes it. The tablist test (console-agent-detail-2.test.mjs:2129) dropped Workspace files/Access from its list, so its aria-controls/aria-labelledby and arrow-key checks no longer cover the new Agent sections tablist. That is how this got through. Extending the same state() check to that tablist would catch it.
3. Low: a read-only draft still tells the reader to edit (detail.mts:1979, revisionStateLabel)
For a member without draft update, the history row says "Draft · Read-only". The heading still says "Edit the saved draft, then deploy it as a new revision." under a "Draft" badge, because the scope line and the badge ignore draftReadOnly()/configurationReadOnly().
Nits (non-blocking)
- Test DRY:
.agent-tabs:not(.agent-scope-tabs)appears about 20 times and.locator(".agent-draft-actions").getByText(/generation N/)about 16 times across files. ArevisionTabStrip(page)/draftGeneration(page, n)helper inconsole-agents-browser-helpers.mjs(next tochooseRevision) would keep the next layout change to one edit. - Many outcome assertions now read the hidden native
<option>(includeHidden: true) at desktop width rather than the visible history row that users see.markVersionOutcomesupdates both, so this is fine today. One row-label assertion in a delayed-badge test would pin the visible path. - Flow doc: the word-budget trims in unrelated paragraphs dropped two facts: that Secret summaries need no collection permission, and that each refused exact Secret read is an audited denial. The new history line also says "Document uncommitted revision workspace…".
I found no regressions in draft retention, write guards (the row buttons go through change() and trackRevisionControl), unavailable-tab fallback (revisionTab), the narrow-viewport selector (rendered from the same data, no extra fetches), or focus after the last history page.
|
Addressed the review feedback in
Merged main while preserving published history and its newer deployment/authorization behavior. The broader browser run passed 309/312 initially; the three failures were newly merged tests needing Revisions navigation, and all three passed after that update. Final focused checks passed 15/15; Policy denials/dialog suites passed 11/11, with no skips. Typecheck, lint, formatting, workspace, docs, flow validation, design tokens, and rebuilt Storybook passed. The PR Verification section now links refreshed screenshots (including the read-only draft) and a verified 25-second walkthrough. These are simulated Storybook UI proof; the separate local k3d deployment remains at the earlier head. Hourly monitoring remains enabled. @clawsweeper re-review |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
clawsweeper:intake:feature
Problem
Agent revision selection and the detail tabs appeared as separate parts of the page. Operators could scroll into Configuration, Channels, or Logs without a clear connection to the selected revision. Workspace files and Access also appeared beside revision views despite applying to the whole Agent.
Change
Updates the owning reference, runtime flow, Storybook scenarios, and browser coverage. No Driver contract or API change. Merges main without rewriting published history, preserving its deployment-read, authorization, and Policy denials fixes.
Verification
Tested head:
a17112643095dc92712e62bd9ae17f8690f5b8cc.Visual evidence — rebuilt Storybook, simulated UI proof
Stories under Pages / Agent detail: Historical revision workspace, Agent sections, Mobile revision workspace, Open draft, and Version draft, viewer can only read. Screenshots: desktop 1440 × 1000 and phone 390 × 844; video: 1440 × 900. No page errors or horizontal overflow in the captured states. These fixtures do not establish backend persistence, credential delivery, or model execution.
Desktop before · Desktop after · Draft · Read-only draft · Phone · Agent-wide workspace
Play the 25-second walkthrough: select Channels, change revisions with Channels retained, switch to Workspace files and back, then open Draft. Played at normal speed; opening, transitions, and final draft state inspected. Uploaded media bytes verified. Evidence gallery.
At the earlier
29e94513dhead, a full controller image was deployed to local k3d using Kubernetes Compute: API/worker Ready, real browser sign-in and draft navigation/editor checks passed, and served CSS/JavaScript matched source. These review fixes have not been redeployed there. The demo remained a draft without model credentials; no Agent runtime deployment or model turn was exercised.Risks
Navigation and focus across revision changes, retained drafts, paging, and permission changes remain the main risks. Backend contracts and default runtime image versions are unchanged. CI and renewed review must be checked at the updated head before merging.