Skip to content

Dev to 0.1 - #1

Merged
SingletC merged 7 commits into
mainfrom
dev
Mar 6, 2026
Merged

Dev to 0.1#1
SingletC merged 7 commits into
mainfrom
dev

Conversation

@SingletC

@SingletC SingletC commented Mar 6, 2026

Copy link
Copy Markdown
Owner

No description provided.

SingletC added 5 commits March 5, 2026 19:04
… widget UX

- Added .ipynb_checkpoints/ and .playwright-cli/ to .gitignore.
- Introduced CHEMFLOW_API_KEY and CHEMFLOW_BASE_URL environment variable support in the client.
- Updated widget to display a "Thinking harder..." placeholder during processing.
- Enhanced widget styling for better usability and responsiveness.
- Added tests for starting from an empty workspace and generating structures.
@github-actions

github-actions Bot commented Mar 6, 2026

Copy link
Copy Markdown

The patch introduces a high-risk self-hosted PR workflow and two behavioral regressions in the new empty-workspace/async widget paths. Those issues are significant enough that the change should not be treated as correct yet.

Full review comments:

  • [P1] Keep untrusted PRs off the self-hosted review runner — /home/tengcc/actions-runner-chemflow-client/_work/chemflow-client/chemflow-client/.github/workflows/codex-selfhost-review.yml:18-18
    Because this job runs on self-hosted for every pull_request, a forked PR can change repo files that codex review consumes (notably AGENTS.md) and have those instructions executed against your runner host. In the public-repo/forked-PR case this is a runner compromise/exfiltration path, so this workflow should be gated to trusted PRs or moved to a GitHub-hosted runner/manual trigger.

  • [P2] Preserve lattice metadata for empty ASE workspaces — /home/tengcc/actions-runner-chemflow-client/_work/chemflow-client/chemflow-client/src/chemflow_client/ase_adapter.py:19-20
    AseAtomsAdapter.to_payload() now drops cell/pbc whenever len(atoms) == 0. That breaks the new empty-workspace flow for callers who intentionally pass Atoms(cell=..., pbc=...) to generate inside a specified box, because the backend now receives only empty symbols/positions instead of the requested lattice. The previous serializer preserved those fields even for zero-atom inputs.

  • [P2] Release _client_lock before the blocking chat request — /home/tengcc/actions-runner-chemflow-client/_work/chemflow-client/chemflow-client/src/chemflow_client/widget.py:575-576
    In the async path, _background_chat_worker() holds _client_lock for the full self._client.chat(prompt) network round trip. While a request is in flight, any concurrent widget.get_atoms(), widget.close(), set_atoms(), or undo() from another notebook cell will block until the HTTP call returns (up to the 300s timeout), which undermines the new non-blocking API and can hang widget cleanup/shutdown on a slow request.

- Updated README to include new examples for generating methane with and without a specified box.
- Modified the ChemFlow3DClient to prepare chat requests and handle responses more efficiently.
- Improved the ASE adapter to handle empty atoms and preserve cell and periodic boundary conditions.
- Updated widget to maintain responsiveness during background chat operations.
- Added tests to ensure proper functionality of the ASE adapter and widget behavior under asynchronous conditions.
@github-actions

github-actions Bot commented Mar 6, 2026

Copy link
Copy Markdown

The patch introduces a broken PR review workflow permission set and a likely-invalid request shape for the new empty-workspace feature. Either issue is enough to warrant follow-up before considering the change correct.

Full review comments:

  • [P2] Grant issues: write before calling issues.createComment — /home/tengcc/actions-runner-chemflow-client/_work/chemflow-client/chemflow-client/.github/workflows/codex-selfhost-review.yml:7-9
    The new review workflow only grants pull-requests: write, but the final step uses github.rest.issues.createComment(...). On pull requests, that REST endpoint requires the issues: write permission, so the comment step will fail with Resource not accessible by integration and the workflow won't publish its review back to the PR.

  • [P2] Avoid seeding empty sessions with ASE's default zero cell — /home/tengcc/actions-runner-chemflow-client/_work/chemflow-client/chemflow-client/src/chemflow_client/client.py:44-46
    When start() takes this new atoms=None branch, it stores a bare ase.Atoms() instance. On the first request, AseAtomsAdapter.to_payload() will serialize that object as cell=[[0,0,0], …] and pbc=[False, False, False], so the new empty-workspace path is not actually empty. Backends that validate an incoming lattice, or that preserve the supplied cell on generated structures, can now reject chat3d(None, ...) or return atoms carrying an invalid zero-volume cell.

@SingletC
SingletC merged commit 3871273 into main Mar 6, 2026
5 checks passed
@github-actions

github-actions Bot commented Mar 6, 2026

Copy link
Copy Markdown

The patch introduces async widget state-management regressions and a workflow bug that can make Codex review compare against a stale base branch on reused self-hosted runners. These issues are discrete and worth fixing before relying on the new behavior.

Full review comments:

  • [P2] Force-refresh the base ref before running codex review — /home/tengcc/actions-runner-chemflow-client/_work/chemflow-client/chemflow-client/.github/workflows/codex-selfhost-review.yml:37-38
    On a self-hosted runner the checkout is reused, so fetching "${{ github.base_ref }}:${{ github.base_ref }}" without force can fail when the local base branch is stale or the branch was rewritten, and the trailing || true hides that failure. In that case codex review --base ... compares against an outdated local branch and can post findings for the wrong diff.

  • [P2] Guard clear_selection() during async requests — /home/tengcc/actions-runner-chemflow-client/_work/chemflow-client/chemflow-client/src/chemflow_client/widget.py:649-652
    This only blocks the frontend clear_selection message path. Because chat_async() now returns immediately, notebook code can still call widget.clear_selection() while busy is true; that method clears the highlights and sets status_text to Ready even though the background request is still running, which leaves the widget in an inconsistent state for programmatic users.

  • [P3] Let background error finalization run after close — /home/tengcc/actions-runner-chemflow-client/_work/chemflow-client/chemflow-client/src/chemflow_client/widget.py:495-497
    If the widget is closed while an in-flight chat_async() later fails, _run_on_main_thread() returns immediately here because _closed is already true. That skips _finalize_background_chat_error(), so _worker_thread is never cleared and the object stays stuck in busy=True, which breaks cleanup code that waits for the background request to finish after close().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant