Skip to content

fix(#6867): refresh artifacts after settled-session recovery - #7067

Merged
nesquena-hermes merged 3 commits into
nesquena:masterfrom
rodboev:pr/6867-artifact-projection-recovery
Aug 17, 2026
Merged

nesquena-hermes merged 3 commits into
nesquena:masterfrom
rodboev:pr/6867-artifact-projection-recovery

Conversation

@rodboev

@rodboev rodboev commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Thinking Path

  • Workspace Artifacts is derived from current session messages and tool metadata, but the DOM changes only when a lifecycle path explicitly projects that state.
  • Normal completion and full session load already project. Settled-session recovery installs the same canonical state without refreshing the mounted panel.
  • One artifact-specific, owner-scoped projection helper now covers all three canonical snapshot installers. It revalidates the captured session against the current pane, while live tool scheduling and path existence safety remain unchanged.

What Changed

  • static/workspace.js: add one fail-closed current-pane authority for canonical Artifacts projection.
  • static/messages.js: route normal done and settled recovery through that authority, using the settled session id so compression A-to-B rotation keeps ownership correct.
  • static/sessions.js: route winning full-load completion through the same authority.
  • tests/test_issue6867_artifacts_settled_recovery.py: cover rendered stale-row replacement, production _restoreSettledSession() recovery, compression session-id rotation, ownership races, no-state outcomes, adapters, and path safety.
  • tests/test_issue2655_frontend.py: enforce shared-owner routing in the original Artifacts contract.

Why It Matters

Moved or replaced files stop leaving dead entries in the mounted Artifacts panel after stream-end recovery. The current path appears immediately without a session switch, and background recovery cannot repaint the pane being viewed.

Verification

The headless regression drives the real settled-recovery path and asserts on the rendered Artifacts DOM. It fails on master with the old row still mounted and passes with only the replacement row. Focused checks cover done, recovery, load, stale-owner and no-state isolation, live scheduling, missing-path safety, and syntax/lint. Responsive screenshot evidence isn't included in this contribution.

Risks / Follow-ups

This changes when the existing Artifacts renderer runs, not how paths are collected or opened. openArtifactPath() still checks existence through the workspace-scoped route. Live tool/tool_complete updates remain on their 100 ms debounce because they are incremental rather than canonical snapshot installs.

Contract Routing

  • Task type: browser lifecycle bug fix.
  • Touched areas: settled recovery; Workspace Artifacts projection; session ownership.
  • Relevant public docs: docs/GUIDELINES.md and docs/CONTRACTS.md; no edit is needed.
  • Scope boundaries: no artifact persistence, API, collector, renderer, row markup, path policy, layout, copy, i18n, or release change.
  • Evidence needed before claiming done: rendered DOM regression, owner/no-state negatives, all consumer adapters, and path safety. This projection-only change doesn't alter layout, so responsive screenshot proof isn't included.

Upstream

Closes #6867.

Model Used

OpenAI GPT-5 via Codex CLI.

@greptile-apps

greptile-apps Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR refreshes Workspace Artifacts after settled-session recovery while preventing background sessions from repainting the active pane.

  • Adds an owner- and pane-scoped artifact projection helper.
  • Routes normal completion, settled recovery, and full session loading through the helper.
  • Adds browser coverage for stale-row replacement, session rotation, ownership checks, and path safety.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
static/workspace.js Adds a fail-closed helper that renders artifacts only for the session owning the current pane.
static/messages.js Routes stream completion and settled-session recovery through owner-scoped artifact projection.
static/sessions.js Routes successful full-session loading through owner-scoped artifact projection.
tests/test_issue6867_artifacts_settled_recovery.py Adds rendered regression coverage for recovery, ownership isolation, session-id rotation, and missing artifact paths.
tests/test_issue2655_frontend.py Updates the existing artifact consumer-map assertions for the shared owner-scoped helper.
CHANGELOG.md Adds release-note wording describing the artifact recovery and ownership fix.

Reviews (2): Last reviewed commit: "docs(changelog): note the #6867 artifact..." | Re-trigger Greptile

Comment thread tests/test_issue2655_frontend.py
@nesquena-hermes nesquena-hermes added the size:L Large PR (>10 files or >250 LOC) label Aug 16, 2026
@nesquena-hermes

Copy link
Copy Markdown
Collaborator

🔬 Gate certification — GREEN ✅

Certified head: 0b89d33d7703313cb182a83fa08a4a4d0d909aa8
Rebased gate head: 87c5a9cfd6b05c301c8a4c8fd335ff05940aa790 on current origin/master d9c5b334 (was 4 behind, no file overlap; clean rebase).
Verdict: gate-pass — a genuinely minimal, well-scoped ownership-guard fix. After a settled-session recovery / compression rotation, the workspace Artifacts panel could render the WRONG session's artifacts (ownership leak, #6867). The fix guards the render so only the current session + current pane owner refreshes. No regression, no ownership leak, no legitimate-refresh false-negative — verified by both advisors and reviewer.

What I ran

Leg Result
Threat scan (via local diff) CLEAN (score 0)
Rebase-first Clean rebase onto d9c5b334 (no overlap).
Full pytest suite (sandboxed, -p no:xdist, to completion) 14,539 passed, 93 skipped, 1 xfailed, 2 xpassed, 34 subtests + 10 failed / 2 errors — every failing node is in the established same-box frozen-master baseline set (test_issue609, test_5774b, test_docs_gitignore_policy, test_issue2695, test_issue1499, extension SSE, glm catalog) → zero PR-owned suite failures. The PR's own test_issue6867_artifacts_settled_recovery.py + test_issue2655 pass.
Codex (reproduce) SAFE TO SHIP — no regression; ran 8 Chromium recovery tests + 6 neighboring ownership/recovery tests, all pass
Fable 5 (UX) SHIP-UX — principled guard, matches convention, no visual regression, blank-flash/stale-panel audit clean
Reviewer Reviewed the full 12-line production diff; ownership guard is correct and defensive

The fix (12 production lines)

projectSessionArtifactsForOwner(sessionId) (workspace.js) is an ownership guard:

function projectSessionArtifactsForOwner(sessionId){
  if(!sessionId||!S.session||S.session.session_id!==sessionId) return false;
  if(typeof _isSessionCurrentPane!=='function'||!_isSessionCurrentPane(sessionId)) return false;
  renderSessionArtifacts();
  return true;
}

It replaces 3 unconditional renderSessionArtifacts() calls — messages.js:6253 & 7018 (both attachLiveStream completion paths, passing completedSid) and sessions.js:2391 (loadSession, passing sid). It delegates ownership to _isSessionCurrentPane (the app's established pane-ownership predicate used at ~10 sites, which also handles the mid-switch _loadingSessionId window).

Verified SOUND (reproduced by Codex + Fable + reviewer)

  • No ownership leak: the guard returns before any DOM write when the sid is a foreign/loading pane — a background session finishing never repaints the foreground's Artifacts panel (restores the ChatGPT/Claude/Codex side-panel convention that master violated).
  • No false-negative (the bug it fixes still fixed): completedSid at both messages.js sites is the completing stream's own id (6102/6955), so a current-pane recovery passes the guard; the second call site adds the previously-missing refresh after renderMessages. loadSession's guarded call runs after _loadingSessionId is cleared (2367) and S.session is the loaded session, so normal switches render; a superseded-load race correctly skips the stale render.
  • Suppression can't blank the panel: returns before any DOM write, so the pane keeps showing the current session's artifacts (empty-state strings are only reachable on renders that DO run).
  • Principled split: the 3 remaining unguarded renderSessionArtifacts() callers (debounced scheduler, tab activation, file-tree refresh) are correctly left unguarded — they're user-initiated pane-local actions that render from current S state and can't carry a foreign sid.
  • Base-drift clean (4-behind rebase; no master conflict on attachLiveStream/loadSession/renderSessionArtifacts).

Minor (cosmetic, non-blocking): the guard's first line duplicates _isSessionCurrentPane's own first check — harmless redundancy, no action needed.

Recommendation to the next agent

Ready to merge. Cert fresh for sha 0b89d33d (rebased-clean at 87c5a9cf). Engineering is green from both advisors + full suite + reviewer. This is a crown-jewel-adjacent surface (workspace pane), but the change is a render-TIMING guard with zero visual restyle and 8 passing Chromium recovery tests, so screenshots would be confirmation-only, not a gate blocker — at ship time, a quick two-state Artifacts-tab shot (current session recovering vs a background session completing while viewing another) is a nice-to-have for the record once the shared browser daemon recovers. Re-confirm current-master mergeability at ship time. Preserve @rodboev attribution.

Gate leg: threat scan → full suite w/ frozen-master baseline reconciliation → Codex reproduce (SAFE, ran the PR's Chromium recovery tests) → Fable 5 UX (SHIP-UX, code+audit) → reviewer full-diff read. The shared browser daemon was unavailable this pass; the live-behavior evidence is the PR's 8 passing Chromium recovery tests + Codex's execution + Fable's audit — a fresh reviewer screenshot is deferred to ship time.

@nesquena-hermes nesquena-hermes added the gate-pass Full gate passed (Codex+Opus+suite+browser); queued Tier 1 for release agent label Aug 16, 2026
@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Gate-clean at the rebased head. Codex SAFE TO SHIP (ownership guards reject foreign-session and in-progress-pane repaint; completedSid follows the authoritative settled session including compression rotation; normal loads + live completion still render artifacts; browser smoke zero console errors). Full suite 14,625 passed / 0 failed; 4 rendered-browser + 52 ownership/recovery + 8 neighbor tests pass.

This is a behavior fix (dropping stale moved/deleted dead-link rows after settled-session recovery), so a static before/after cannot faithfully show it — the proof rests on the gate + the real Playwright recovery coverage. Ready to ship pending the visible-surface sign-off. Thanks @rodboev.

@nesquena-hermes
nesquena-hermes enabled auto-merge (squash) August 17, 2026 00:49
@nesquena-hermes
nesquena-hermes merged commit f60805d into nesquena:master Aug 17, 2026
23 of 24 checks passed
@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Shipped in experimental release exp-v0.52.233 — artifact re-projection is now scoped to the current session and pane, so a background session finishing no longer flashes its artifacts into the panel you are reading, and the current session refreshes correctly after recovery. Codex SAFE + Fable SHIP-UX + full suite green + 52 recovery tests + Nathan-approved. Thanks @rodboev!

🤖 Released by the release-manager agent.

alai04 pushed a commit to alai04/hermes-webui that referenced this pull request Aug 31, 2026
…esquena#7067)

* fix(nesquena#6867): preserve artifact ownership across compression rotation

* docs(changelog): note the nesquena#6867 artifacts-panel ownership-guard fix

---------

Co-authored-by: nesquena-hermes <nesquena+hermes@gmail.com>
Co-authored-by: n <a@n>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gate-pass Full gate passed (Codex+Opus+suite+browser); queued Tier 1 for release agent size:L Large PR (>10 files or >250 LOC)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Workspace Artifacts keeps stale, dead-link entries after files are moved or deleted

2 participants