Skip to content

fix(runs): count fetched URLs and enforce count ceilings - #115

Open
Zhao0335 wants to merge 9 commits into
jordan-gibbs:mainfrom
Zhao0335:fix/run-spend-reconcile-92
Open

Zhao0335 wants to merge 9 commits into
jordan-gibbs:mainfrom
Zhao0335:fix/run-spend-reconcile-92

Conversation

@Zhao0335

@Zhao0335 Zhao0335 commented Sep 11, 2026 •

Copy link
Copy Markdown

What changed

  • set_step reconciles counts at every hpr run step --status done boundary, and run reconcile can refresh them directly. notes_written counts notes tagged with the run's vault_tag; sources_fetched counts distinct URLs in sources linked to those notes. This includes HTML pages and PDFs. It does not use file modification times or raw_file.
  • Removed _count_agent_spawns. No pipeline skill emits the event types it scanned, so reconciliation leaves the agent-reported agents_spawned value alone.
  • Optional run init --max-sources and --max-notes ceilings block when observed counts reach them. run resume and run step --status done --force clear a count block against a saved count. Unchanged counts stay clear; an increase blocks again. Both clear paths share a cap of three, checked before a step is recorded. Budget and verify blocks cannot be cleared by step --force.
  • run reconcile now reports a bad run tag as a CLI error, consistent with the other run commands. The changelog entry sits with the existing Unreleased bullets and restores the en-dashes in the Pipeline constants are authored twice: profile fields vs hardcoded values #101 entry.

Limits that remain

--budget itself stays inert in normal pipeline runs: nothing calls hpr run spend, so estimated_usd remains zero unless a caller explicitly reports spend. This PR does not add unit prices or automatic dollar estimates.

The count ceilings only protect real runs if the entry skill passes --max-sources or --max-notes to run init. The current entry skill does not pass them, so runs started by it have no active count ceiling by default.

Addresses #92.

Verification

  • python -m pytest tests/test_cli tests/test_core/test_runs.py -q — passed; one optional crawl4ai test skipped because the extra is not installed.
  • ruff check on the touched Python files — passed.
  • git diff --check — passed.
  • All five GitHub CI jobs passed: Python 3.11–3.14 on Linux and 3.11 on Windows.
  • Regression coverage includes five HTML notes plus one PDF, count-clear persistence, reblocking on an increased count, the three-clear cap (including run resume), and bad-tag CLI output.

Completed runs reported zero spend because pipeline agents never call
hpr run spend, so --budget could never fire. Fold notes/raw sources and
spawn events into counters on finish, add hpr run reconcile, and record
pid + heartbeat_at so a dead host session is distinguishable from a live one.

Fixes jordan-gibbs#92
@jordan-gibbs

Copy link
Copy Markdown
Owner

Thanks for taking this on. The set_step hook is the right place for it: every skill already calls run step --status done, so a gate there needs no prompt changes. The Windows _pid_alive is done carefully too. I can't merge this shape though.

The ceiling only fires because of DEFAULT_USD_PER_* at runs.py:60-62. Those are pricing numbers, which this repo doesn't carry, and on the run in #92 they come out to 2.65 against a ceiling of 25, so the exact reproduction still finishes done. Drop the coefficients and have the gate watch what the CLI can actually see: an optional --max-sources / --max-notes on run init, checked against reconciled counts at step boundaries.

For the counts, don't use mtime. note update, repair, and sources score all rewrite old notes, and concurrent runs share the vault, so the number drifts with time instead of tracking the run. Every note the pipeline fetches is already tagged with the vault_tag. Count by tag with a DB query, and get raw files through those notes' raw_file: field.

Drop pid and heartbeat_at. os.getpid() in run init is the CLI subprocess, which has exited by the time anyone looks, so run status says "host session likely died" on every healthy run. I reproduced that. heartbeat_at is written from the same now as updated_at, so it adds nothing, and possibly_stalled already covers liveness.

Two smaller things: mention the set_step hook in the PR body, since it changes every step boundary and the description doesn't say so, and check that run resume can get past a block. Right now the next step-done re-blocks even with no new files.

@jordan-gibbs jordan-gibbs added the needs-decision Needs a maintainer call on direction label Sep 11, 2026
Rework of jordan-gibbs#92 for maintainer review:

- Gate lives in set_step (every hpr run step --status done); no prompt
  changes and no DEFAULT_USD_PER_* coefficients.
- init_run --max-sources / --max-notes are the observable ceilings.
- Counts come from the vault DB by vault_tag + each note's raw_file
  frontmatter, not mtime (concurrent runs and repair rewrite files).
- Drop pid and heartbeat_at; possibly_stalled uses updated_at.
- run step --status done --force clears a count block so resume can
  proceed; cannot clear budget/verify; force-clears are capped.

Fixes jordan-gibbs#92
@Zhao0335

Copy link
Copy Markdown
Author

Reworked in 86d9cda per your review:

  • Gate is set_step (every hpr run step --status done); no prompt changes.
  • --max-sources / --max-notes on
    un init; no DEFAULT_USD_PER_*.
  • Counts by �ault_tag in the DB +
    aw_file, not mtime.
  • Dropped pid / heartbeat_at; possibly_stalled uses updated_at.

un step --status done --force clears a count block only (not budget/verify) so resume can proceed when no new files landed.

Ready for another look.

Resolve CHANGELOG (keep Unreleased jordan-gibbs#92 + 0.11.1) and runs.py
(chapter-plan fold from jordan-gibbs#114 + updated_at stall detection without
heartbeat).
@jordan-gibbs

Copy link
Copy Markdown
Owner

Thanks for the rework, this is the shape I was hoping for. I ran it against a run that just finished here (91 notes, 87 fetched pages) and it reports notes_written: 91 and sources_fetched: 0. The count only sees notes with a raw_file, which means PDFs, and most of what a run fetches is HTML. Every fetch already lands a row in the sources table (cli/fetch.py writes it with the note_id), so joining that to tags on the vault tag gives the number the counter is named for:

SELECT COUNT(DISTINCT s.url) FROM sources s JOIN tags t ON t.note_id = s.note_id WHERE t.tag = ?

That query returns 87 on the same run.

One more thing while you're in there: _count_agent_spawns looks for agent_spawn / spawn / agent events and no skill emits any of those, so it always returns the manifest value. I'd drop it rather than leave a counter that can't move.

With the sources count fixed I'll merge.

Keep jordan-gibbs#92 ceilings + force; retain VaultError handling and upstream Unreleased notes.
@jordan-gibbs

Copy link
Copy Markdown
Owner

The Sep 14 push is just a merge of main, so the two things from my last comment are still open: sources_fetched still counts only notes with a raw_file (a vault with 5 HTML notes and 1 PDF reports 1), and _count_agent_spawns is still called.

A few more came up reading it again:

  • After run resume or --force, the next step marked done blocks the run again on unchanged counts. A forced clear should hold until the count actually goes up.
  • Once the force cap is hit, --force still records steps as done while the run stays blocked, so the cap doesn't stop anything.
  • run reconcile catches only RunError, so a bad tag gives a traceback. The rest of run_cmd catches VaultError too.
  • The changelog changes the en-dashes in the Pipeline constants are authored twice: profile fields vs hardcoded values #101 entry, and the new heading sits above existing bullets.
  • The PR title still says pid/heartbeat.

Please also say plainly in the description that --budget itself stays inert, since nothing calls run spend. The count ceilings only protect real runs if the entry skill passes them to run init.

@Zhao0335 Zhao0335 changed the title fix(runs): reconcile spend from disk and record pid/heartbeat fix(runs): count fetched URLs and enforce count ceilings Sep 23, 2026
@Zhao0335

Zhao0335 commented Sep 23, 2026 •

Copy link
Copy Markdown
Author

Addressed the open review points in 9582456 and merged current main in 71a3507 (the PR is mergeable again).

  • sources_fetched now counts distinct sources.url values joined to the run's tagged notes. The regression case has five HTML notes and one PDF and reports six sources.
  • Removed _count_agent_spawns; reconciliation preserves only explicitly reported agents_spawned.
  • Count-block clears from either run resume or step --force save the current count, so later steps with unchanged counts proceed. A higher count blocks again. The three-clear cap is checked before writing the next step, and run resume cannot bypass it.
  • run reconcile catches VaultError, including a bad tag, and returns a normal CLI error.
  • The changelog entry is an Unreleased bullet after the existing entries, and the Pipeline constants are authored twice: profile fields vs hardcoded values #101 en-dashes are restored. The PR title and description are updated.

The description states the remaining limits plainly: --budget stays inert in normal pipeline runs because nothing calls run spend; count ceilings affect real runs only if the entry skill passes them to run init.

The CLI and run tests pass after the merge (one optional crawl4ai test skipped), as do ruff and git diff --check. All five GitHub CI jobs passed (Python 3.11–3.14 on Linux and 3.11 on Windows).

@Zhao0335

Copy link
Copy Markdown
Author

Rebased the review state by merging current main in 998de4d. The PR is mergeable again. The only code conflict was run resume: it now retains both the count-block clear/cap and upstream's tier-aware next-step selection. I added a regression for a blocked run declared light at step 1; after resume, it clears the block once and continues at step 10.

Validation on the merged tree: full local test suite passed (one Windows-only skip), ruff check src/ tests/ passed, and the sdist/wheel build passed. All five upstream CI jobs passed, including Windows 3.11 and Linux 3.11–3.14. The count-ceiling and --budget limits stated in the PR description still apply.

This branch has not been deployed

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

Labels

needs-decision Needs a maintainer call on direction

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants