Skip to content

fix(kanban): reject completion artifacts that do not exist - #75572

Open
Christopher-Schulze wants to merge 2 commits into
NousResearch:mainfrom
Christopher-Schulze:fix/53699-kanban-durable-artifact-validation
Open

fix(kanban): reject completion artifacts that do not exist#75572
Christopher-Schulze wants to merge 2 commits into
NousResearch:mainfrom
Christopher-Schulze:fix/53699-kanban-durable-artifact-validation

Conversation

@Christopher-Schulze

@Christopher-Schulze Christopher-Schulze commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

kanban_complete accepted declared artifacts that live outside the scratch workspace without checking them at all. A completion referencing a path that never existed was recorded as done, and the artifact was reported as persisted even though no file existed to hand off. This makes the artifact contract of a completed task trustworthy: an artifact that will not exist for the user must fail the completion instead of being silently accepted.

The fix adds a durable-artifact validation gate to the completion path: every declared artifact outside the scratch workspace must exist, be a regular file, be readable, and be non-empty. Violations raise ArtifactPreservationError, which kanban_complete already maps to a structured "task is still in-flight" retry error — the task stays open and the worker can fix the path and retry the same handoff. Scratch-workspace artifacts keep their existing copy-and-preserve behavior unchanged.

This closes the remaining gap after the consolidated artifact fix #63619 (merged 2026-07-13), which validates and preserves scratch deliverables: artifacts outside the scratch workspace (the reporter's case — a durable path under /Users/richfeather/...) were still handed off by path alone. Regression tests on the current default branch (which includes #63619) confirm the gap: a nonexistent durable artifact is still accepted there, and rejected with this change.

Related Issue

Fixes #53699

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • hermes_cli/kanban_db.py: new _validate_durable_artifact() enforcing exist / regular file / readable / non-empty; wired into _persist_scratch_completion_artifacts for every artifact that is not copied from the scratch workspace — the non-scratch-task early return, the path-resolution failure branch, and the outside-workspace branch. Scratch copy semantics are untouched.
  • tests/hermes_cli/test_kanban_db.py: regression tests for missing, empty, and directory durable artifacts (completion rejected, task stays open) plus the existing-file happy path.
  • tests/tools/test_kanban_tools.py: worker-facing regression test asserting kanban_complete returns the in-flight error and the task is not marked done.

How to Test

  1. Reproduce on current main: create a task whose workspace is not a scratch workspace and complete it with an artifacts path that does not exist — completion succeeds and the artifact is accepted. On this branch it is rejected:
    kanban_complete returns the error ...declared durable artifact does not exist: <path>. Your task is still in-flight... and the task stays open.
  2. Regression proof: with the source fix stashed, the three new tests fail (DID NOT RAISE ArtifactPreservationError); with the fix applied, all pass:
    • scripts/run_tests.sh tests/tools/test_kanban_tools.py tests/hermes_cli/test_kanban_db.py -q — 61 passed
  3. Adjacent kanban suites remain green (74 + 34 tests across core, goal-mode, lifecycle, swarm, diagnostics, block-kinds, gateway notifier, dashboard plugin).

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15 (tested on macOS only; the change is platform-agnostic pathlib/os.access logic)

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

@alt-glitch alt-glitch added type/bug Something isn't working comp/cron Cron scheduler and job management P3 Low — cosmetic, nice to have labels Jul 31, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for closing the verified durable-artifact validation gap. The implementation targets the right persistence boundary: current main returns non-scratch artifacts unvalidated in hermes_cli/kanban_db.py:5018 and forwards them into completed-event payloads at hermes_cli/kanban_db.py:4844-4851.

Problems

  • tests/hermes_cli/test_kanban_notify.py:153-156 completes a task with one real and one nonexistent artifact, then waits for a notifier event. This PR correctly rejects that completion, so no event is created and the watcher times out. Required CI slice 2/8 fails at test_notifier_artifact_delivery_skips_missing_files (run 30655145914, job 91237724742).

Suggested changes

  • Rework that notifier regression to inject a historical completed payload with a missing path, preserving its defensive skip behavior; add an assertion that the mixed artifact completion itself is rejected and leaves the task incomplete.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 31, 2026
kanban_complete accepted declared artifacts outside the scratch
workspace without checking them, so a completion referencing a
path that never existed was recorded as done and the artifact
was reported as persisted. Validate durable artifacts (exist,
regular file, readable, non-empty) before completing, raising
ArtifactPreservationError so the worker gets the in-flight
retry error and the task stays open. Fixes NousResearch#53699.
…validation

Complete-time durable validation rejects worker-declared paths that never
existed, so the notifier suite no longer completes with a ghost path. Inject
the missing path into the stored completed event after a successful complete
to keep covering delivery-time skip behavior for vanished files.
@Christopher-Schulze
Christopher-Schulze force-pushed the fix/53699-kanban-durable-artifact-validation branch from 71a0add to 505cc53 Compare July 31, 2026 19:32
@Christopher-Schulze

Copy link
Copy Markdown
Contributor Author

Addressed sweeper feedback

Thanks for the precise CI diagnosis on test_notifier_artifact_delivery_skips_missing_files.

Change: Complete-time durable validation correctly rejects worker-declared missing paths, so the notifier suite no longer completes a task with a ghost artifact. The regression now:

  1. Completes successfully with only a real artifact.
  2. Injects a missing path into the stored completed event (post-complete vanish race).
  3. Asserts the notifier still delivers the real file and skips the ghost without hanging.

This preserves the delivery-time defensive skip while keeping the complete-time contract honest. Required CI should go green on this head.

@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

Two PRs address #53699 by preventing completion from accepting nonexistent durable artifacts. #53723 adds a narrower tool-boundary file check, while #75572 validates non-scratch artifacts at the persistence boundary and preserves the scratch-artifact lifecycle introduced by #63619.

Related pull requests

Duplicates

#53723 and #75572 overlap on rejecting nonexistent declared artifacts. The consolidation chain is #53723#63619 for the earlier artifact-lifecycle work, with #75572 covering the non-scratch durable-artifact validation gap that remained after #63619.

Suggested consolidation

Keep #75572 open with a salvage path: preserve its persistence-boundary validation, structured in-flight retry behavior, and revised notifier race regression, then verify the required CI slice on the updated head. Keep #53723 closed because contributor feedback identifies #63619 as its superseding implementation, while #75572 more completely covers the remaining durable-path case.

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
    I53699(["issue #53699 (open)"])
    subgraph Dup53723 ["PRs duplicating each other"]
        P53723["PR #53723 (closed)"]
        P75572["PR #75572 (open)"]
    end
    P75572 -->|best fix| I53699
    class I53699 open
    class P53723 closed
    class P75572 open
    class P75572 best
    class P75572 target
    click I53699 "https://github.com/NousResearch/hermes-agent/issues/53699"
    click P53723 "https://github.com/NousResearch/hermes-agent/pull/53723"
    click P75572 "https://github.com/NousResearch/hermes-agent/pull/75572"
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 2 pull requests and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 19 kB of PR diffs, 12 kB of issue/PR text, 1 kB of discussion (2 comments), 3 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

@alt-glitch alt-glitch added comp/cli CLI entry point, hermes_cli/, setup wizard and removed sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard comp/cron Cron scheduler and job management P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: kanban_complete accepts completion referencing a nonexistent durable artifact

4 participants