Skip to content

fix(cron): add attach_to_session normalization to dashboard cron update (#67706) - #67711

Open
webtecnica wants to merge 2 commits into
NousResearch:mainfrom
webtecnica:fix/67706-cron-whitelist
Open

fix(cron): add attach_to_session normalization to dashboard cron update (#67706)#67711
webtecnica wants to merge 2 commits into
NousResearch:mainfrom
webtecnica:fix/67706-cron-whitelist

Conversation

@webtecnica

Copy link
Copy Markdown
Contributor

Summary

Fixes #67706: Adds attach_to_session boolean normalization to the dashboard cron update adapter _normalize_dashboard_cron_updates.

Problem

The dashboard adapter was missing explicit handling for attach_to_session. The agent tool path (in cronjob_tools.py) already coerces the value to bool(), but the dashboard path silently passes it through as-is, creating a mismatch between the two paths.

Furthermore, the upcoming whitelist PR (#67660) introduces _DASHBOARD_ALLOWED_UPDATE_FIELDS which will reject unknown keys — attach_to_session needs to be explicitly normalized before that lands.

Changes

  • hermes_cli/web_server.py: Added bool() coercion for attach_to_session in _normalize_dashboard_cron_updates, matching the agent tool path in cronjob_tools.py.

How to Test

pytest tests/hermes_cli/test_web_server_cron_profiles.py -q

Checklist

  • Follows Conventional Commits
  • Changes scoped to this fix only
  • Cross-platform impact: none (dashboard adapter only)

…try for transient failures (NousResearch#67637)

The Windows dependency stage's baseline-import gate discarded all
Python output with 2>&1 | Out-Null, so when imports failed the log
lost the actual ModuleNotFoundError/ImportError traceback and
replaced it with a wrong-venv assertion that was often incorrect.

Changes:
- Capture stderr+stdout with Out-String instead of discarding with
  Out-Null so the real Python traceback is available in the error.
- Retry the baseline import probe up to 3 times (800ms between
  attempts) so transient failures from file locks or antivirus
  scanning can self-heal before the gate aborts.
- On persistent failure, report the exact interpreter path, module
  list, exit codes, and retained traceback in the error.
- Only claim a sibling .venv when it actually exists on disk.
- Apply the same output-preservation pattern to the web deps
  import/compile checks (fastapi/uvicorn, py_compile).
…te (NousResearch#67706)

The dashboard adapter _normalize_dashboard_cron_updates was missing
boolean normalization for attach_to_session, causing the field to be
silently ignored or (after the upcoming whitelist PR NousResearch#67660) rejected
as unknown. Add explicit bool() coercion matching the agent tool path
in cronjob_tools.py.

Closes NousResearch#67706
@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard comp/cron Cron scheduler and job management comp/dashboard Web dashboard / control panel UI (dashboard/, landing) platform/windows Native Windows-specific behavior or breakage P2 Medium — degraded but workaround exists needs-decision Awaiting maintainer decision before any implementation sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Jul 19, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #67706 and #67637. This branch combines the dashboard cron-field fix with an independent Windows installer diagnostics/retry fix; CI currently fails the installer syntax-probe assertion, so please split the work or update the corresponding test.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for isolating the dashboard normalization change. Current main still sends the dashboard update map from hermes_cli/web_server.py:12024-12054 to cron.jobs.update_job unchanged, while cron/scheduler.py:645-647 only recognizes a boolean attach_to_session; the cron premise is therefore still present.

Problems

  • This branch also contains an unrelated installer change. As the member triage comment notes, it should be split. It additionally fails the existing regression: tests/test_install_ps1_web_server_syntax_probe.py:41-45 requires the literal syntax-check throw, but the changed installer code uses $diag and throw $diag at scripts/install.ps1:2232-2236.
  • No test covers the new cron normalization. The neighboring dashboard normalization test is tests/hermes_cli/test_web_server_cron_profiles.py:360-390.

Suggested changes

  • Salvage the cron commit independently and add an update-path test asserting attach_to_session is persisted as a boolean.
  • Keep the installer diagnostics/retry work in its own PR and update its dedicated regression contract there.

Automated hermes-sweeper review.

Comment thread scripts/install.ps1
if ($webCompileOutput) {
$diag += "`nSyntax check output: $($webCompileOutput.Trim())"
}
throw $diag

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the literal throw asserted by tests/test_install_ps1_web_server_syntax_probe.py:41-45, causing the reported slice-8 CI failure. Please split this installer work from the cron fix and update its regression contract in the installer-specific change.

@teknium1 teknium1 added sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/install-update Installer, updater, packaging, wheels, doctor labels Jul 25, 2026
@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

One PR addresses #67706. #67711 adds dashboard-side boolean normalization for attach_to_session, which addresses the scheduler type mismatch but not the reported dashboard/core whitelist drift, and its diff also contains unrelated Windows installer diagnostics and retry changes.

Related pull requests

  • fix(cron): add attach_to_session normalization to dashboard cron update (#67706) #67711 best fix — (+49/-9) — n/a: The complete diff normalizes attach_to_session with bool() in hermes_cli/web_server.py, but it does not add the field to a dashboard whitelist; the unrelated scripts/install.ps1 changes alter the syntax-check throw that the contributor review says is asserted by tests/test_install_ps1_web_server_syntax_probe.py:41-45. Consistent with the COMMENTED keep_open review, this recorded best existing fix remains salvageable if the cron change is isolated and covered by an update-path test.

Suggested consolidation

Keep #67711 open with a salvage path: retain the isolated dashboard normalization, add a regression test proving attach_to_session is persisted as a boolean, and split the installer diagnostics/retry work into a separate PR with its regression contract updated. Before closing #67706, also reconcile the applicable dashboard whitelist with the core whitelist or document why the difference is intentional.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I67706(["issue #67706 (open)"])
    P67711["PR #67711 (open)"]
    P67711 -->|best fix| I67706
    class I67706 open
    class P67711 open
    class P67711 best
    class P67711 target
    click I67706 "https://github.com/NousResearch/hermes-agent/issues/67706"
    click P67711 "https://github.com/NousResearch/hermes-agent/pull/67711"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 1 pull request and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 6 kB of PR diffs, 3 kB of issue/PR text, 2 kB of discussion (5 comments), 2 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

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

Labels

area/install-update Installer, updater, packaging, wheels, doctor comp/cli CLI entry point, hermes_cli/, setup wizard comp/cron Cron scheduler and job management comp/dashboard Web dashboard / control panel UI (dashboard/, landing) needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists platform/windows Native Windows-specific behavior or breakage sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants