feat: raise per-cycle limit to 8, remove daily limit

[colehurwitz]

Summary

• Raises Bob Shell per-cycle limit from 3 to 8
• Adds early warning mechanism when ≤2 invocations remain
• Removes daily/session limit entirely (per user preference)
• Adds Runners documentation section to README.md (consolidated from docs: add Runners section to README documenting CLI backends #129)

This merges the best of PRs #132 and #134 while respecting the user's preference for no daily limit.

Changes:

• factory/runners/usage.py: Raised default, removed daily ceiling, added CeilingWarning
• factory/ceo_completion.py: Simplified _budget_allows_respawn (always allow respawn)
• CLAUDE.md: Updated documentation
• README.md: Added Runners section with runner selection, Bob Shell setup, guardrails, and dry-run docs
• factory.md: Updated scope (README.md and docs/** now modifiable)
• Tests updated accordingly

Closes #131, #133
Supersedes #132, #134
Consolidates #129

Test plan

• pytest tests/test_runners.py -v — 43 tests pass
• pytest tests/test_ceo_completion.py -v — 43 tests pass
• pytest -v — all 1145 tests pass
• ruff check and mypy pass

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 94.44444% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.26%. Comparing base (c04f996) to head (c63f9ae).
⚠️ Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
factory/runners/usage.py	91.30%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #136      +/-   ##
==========================================
+ Coverage   86.23%   86.26%   +0.02%     
==========================================
  Files          45       45              
  Lines        6307     6304       -3     
==========================================
- Hits         5439     5438       -1     
+ Misses        868      866       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[colehurwitz]

Review: feat: raise per-cycle limit to 8, remove daily limit

Summary

This PR consolidates work from #132 and #134 to simplify Bob Shell guardrails: per-cycle limit raised from 3→8, daily limit removed, and an early warning mechanism added when ≤2 invocations remain.

Observations

Good:

• Clean removal of daily ceiling complexity — the per-cycle limit is the more meaningful guardrail for preventing runaway loops
• CeilingWarning dataclass and event emission (bob.ceiling_warning) provide useful observability before hitting the hard limit
• Test coverage is comprehensive, including edge cases for warning at exactly 1 and 2 remaining invocations
• _budget_allows_respawn() simplification is sound — per-cycle enforcement belongs in BobRunner, not in the respawn check

Questions/Concerns:

1. Unrelated change in factory.md — Moving README.md from read-only to writable scope and adding docs/** doesn't relate to Bob Shell limits. Should this be a separate PR, or is there context I'm missing?

2. Behavioral change in count_cycle_invocations() — Previously, passing cycle_start=None fell back to count_today_invocations() as an approximation. Now it returns 0. Any callers relying on the old fallback behavior would silently get 0 counts. Worth confirming no other code paths depend on this.

3. Warning threshold hardcoded at 2 — The ≤2 remaining threshold is reasonable but not configurable. If someone sets FACTORY_BOB_MAX_INVOCATIONS_PER_CYCLE=3, they'd get a warning on the very first invocation (3-0=3, next check 3-1=2 → warning). Minor, but could be noisy for low limits.

Verdict

Changes look correct and well-tested. The concerns above are minor — would appreciate clarification on the factory.md scope change.

[colehurwitz]

Thanks for the thorough review! Addressing all three concerns:

Concern 1 (factory.md scope change):
Reverted. The scope expansion for README.md and docs/** came from a separate documentation task (commit 8b92df8) and doesn't belong in this PR.

Concern 2 (count_cycle_invocations returning 0):
No callers rely on the old fallback. Traced all call sites:

• BobRunner always sets self.cycle_start = cycle_start or datetime.now(timezone.utc) in __init__ (line 170)
• ceo_completion.py passes explicit datetime.now(timezone.utc) (line 337)
• Only the old tests passed None, and they've been updated

The fallback to count_today_invocations() was dead code.

Concern 3 (hardcoded warning threshold):
Acknowledged as minor. With default limit of 8, warning fires at invocation 6 (75% usage). For edge cases with limit=3, the warning fires earlier but this is acceptable since the PR explicitly raises the default. No change made per reviewer guidance.

[akashgit]

Review — PR #136

Overview

Consolidates #132 and #134 into a cleaner approach: removes the daily/session limit entirely, keeps only per-cycle ceiling (raised from 3→8), adds near-ceiling warnings (CeilingWarning dataclass + bob.ceiling_warning event), and simplifies _budget_allows_respawn to always return True.

Issue: Per-cycle ceiling is silently ineffective

get_runner() in factory/runners/__init__.py creates BobRunner() with no arguments:

return _RUNNERS[resolved]()

This means cycle_start defaults to datetime.now(timezone.utc) — the moment the runner is instantiated. But get_runner() is called inside invoke_agent() (line 129 of factory/agents/runner.py), which runs per agent invocation. So each agent call creates a fresh BobRunner with cycle_start=now(), resetting the cycle counter to zero every time.

With a per-cycle ceiling of 8 and each agent invocation starting its own "cycle" from zero, the ceiling never triggers — each agent only makes one bob invocation per invoke_agent call.

The test works because it manually passes cycle_start=datetime.now() - timedelta(seconds=5) and pre-logs a usage entry. But that's not how production uses it.

Fix options:

1. Pass cycle_start from the CEO's cycle state through invoke_agent → get_runner → BobRunner
2. Store cycle_start in .factory/state/cycle.json (which already exists from the completion guard) and have BobRunner.__init__ read it
3. Use the completion guard's CycleState.started_at as the authoritative cycle start

Without this fix, the only ceiling is effectively "no ceiling" since the daily/session limits were removed and the per-cycle limit doesn't accumulate across agent invocations.

Everything else looks good

• Removing count_today_invocations and get_daily_ceiling — clean deletion
• _budget_allows_respawn always returning True with per-cycle-only limits — logically correct since respawns start new cycles
• count_cycle_invocations returning 0 when cycle_start is None — safe default
• CeilingWarning with lazy emit_event import and except Exception — same pattern from #132, still good
• Tests properly updated: budget tests simplified, ceiling tests pass cycle_start, warning tests cover near/sufficient/exact-one cases
• CLAUDE.md accurately reflects the change (only per-cycle listed, warning documented)

[colehurwitz]

✅ Factory Review: KEEP

Experiment: #11
Hypothesis: Fix BobRunner per-cycle ceiling by reading cycle start from cycle.json

Score Comparison

Metric	Value
Before	0.7593
After	0.7589
Delta	-0.0004 (0.04% - within noise)
Threshold	0.8000

Guard Checks

Check	Result
scope	✅ PASS
eval_immutable	✅ PASS
anti_pattern	✅ PASS
smoke_test	✅ PASS

CEO Override Rationale

Precheck failed on score_direction but this is a false positive:

• This is a correctness bug fix, not a feature — score impact is not the goal
• Score delta is 0.04% — within measurement noise
• Pre-existing below-threshold (0.76 < 0.80) is not caused by this change
• Without this fix, PR 136's per-cycle ceiling is dangerously ineffective

Addresses Review Issue

Fixes @akashgit's critical finding: the per-cycle ceiling was silently ineffective because each BobRunner started its own clock. Now invocation counts correctly accumulate across agent spawns within a cycle by reading started_at from cycle.json.

Verdict: KEEP — Ready for human review and merge.

---

Posted by Factory CEO

[colehurwitz]

Code Review: PR #136

Overview

This PR raises the Bob Shell per-cycle invocation limit from 3 to 8, adds an early warning mechanism when ≤2 invocations remain, and removes the daily limit entirely. Importantly, it also fixes a bug where the per-cycle ceiling wasn't accumulating correctly across invoke_agent calls.

Analysis

Bug Fix (Key Change)
The core fix addresses a subtle issue where each get_runner() call created a fresh BobRunner with cycle_start=now(), causing every invocation to be counted as the "first" in a cycle. The fix passes project_path to BobRunner, which now reads the cycle's started_at from .factory/state/cycle.json. This ensures all invocations within a cycle share the same start time and accumulate correctly.

Code Quality

• Clean refactoring of _budget_allows_respawn() — it now always returns True since per-cycle limits are enforced within BobRunner.headless() rather than at respawn time
• The CeilingWarning dataclass and event emission add good observability
• The count_cycle_invocations returning 0 when cycle_start is None is correct — no cycle context means no per-cycle enforcement

Test Coverage

• TestCeilingAccumulationAcrossInvocations directly reproduces and verifies the fix
• TestCeilingWarning covers the new warning functionality
• Tests for removed daily ceiling are appropriately removed
• Mock signature updates for get_runner are correct (lambda *args, **kwargs: MockRunner())

Suggestions

1. Minor: In bob.py line 178, consider adding a debug log when reading cycle_start from cycle.json — useful for troubleshooting ceiling issues.

2. Consider: The count_cycle_invocations returning 0 when cycle_start is None means no ceiling enforcement without explicit cycle tracking. This is intentional but worth noting in the docstring (the current docstring is good).

Potential Risks

• Removing the daily limit removes a safety net for runaway loops. The per-cycle limit (8) still provides protection, but an infinite loop of cycles could theoretically run unbounded. This is per user preference, so acceptable.
• The raised limit (3→8) is configurable via env var, so users can adjust as needed.

Verdict

LGTM — The bug fix is correct and well-tested. The limit changes are per user preference with appropriate configurability. The warning mechanism adds useful observability.

[akashgit]

Closing — stale factory experiment branch. Reopen or recreate if still needed.

[colehurwitz]

@akashgit my last review was ltgm if you are good with it

[colehurwitz]

Code Review: PR #136

feat: raise per-cycle limit to 8, remove daily limit

---

Overview

This PR consolidates several related changes to Bob Shell guardrails:

• Increases per-cycle invocation limit from 3 → 8
• Removes the daily limit entirely
• Adds early warning mechanism when ≤2 invocations remain
• Fixes a bug where get_runner() created fresh BobRunner instances with cycle_start=now(), preventing ceiling accumulation
• Adds Runners documentation to README.md

---

Code Quality & Correctness

Strengths:

• Bug fix is sound (factory/runners/__init__.py:52-54, bob.py:186-208): Passing project_path through get_runner() and reading cycle.json ensures all invocations within a cycle share the same cycle_start. This correctly fixes the accumulation bug.

• Clean CeilingWarning design (usage.py:92-99): The dataclass is simple and effective. Emitting events to events.jsonl maintains observability.

• Excellent test coverage: New tests in TestCeilingWarning and TestCeilingAccumulationAcrossInvocations thoroughly verify the new behavior, including the specific bug scenario.

• Simplified respawn logic (ceo_completion.py:315-134): Removing the pre-check makes sense since per-cycle limits are now enforced within BobRunner.headless().

Concerns:

1. Lazy import in __init__ (bob.py:204-207):

   from factory.ceo_completion import read_cycle_state
   state = read_cycle_state(project_path)

   This works but is a code smell. Consider moving to module level with proper import ordering, or document why it's necessary for circular dependency avoidance.

2. Silent exception swallowing (usage.py:116):

   except Exception:
       log.debug("Failed to emit ceiling warning event", exc_info=True)

   This is intentional (don't fail the invocation just because event emission failed), but logging at debug level may make issues hard to diagnose in production. Consider warning level.

3. Daily limit removal risk: Without a daily limit, a bug causing rapid cycles could burn through tokens quickly. The per-cycle limit of 8 is reasonable, but consider whether there should be any circuit breaker for runaway scenarios.

---

Style & Conventions

• Follows project conventions (snake_case, structured logging, Pydantic models)
• Documentation updates are well-written and comprehensive
• Test mock updates (*args, **kwargs) are correct for the new signature

---

Performance

No concerns. The read_cycle_state() call in BobRunner.__init__ is a single file read, which is negligible.

---

Security

No security concerns. The changes don't affect authentication or expose sensitive data.

---

Suggestions

1. Minor: In check_ceilings(), the return type change from None to CeilingWarning | None could surprise callers who ignore the return value. The current call sites don't use the return value, so this is safe, but consider documenting this more explicitly.

2. Consider: Adding a configurable "soft daily cap" as an optional safety net that logs warnings but doesn't block, for users who want some visibility into daily usage without hard limits.

3. Test improvement (test_runners.py:672-741): The integration test test_ceiling_accumulates_across_invoke_agent_calls is thorough but quite long. Consider extracting the setup into a fixture.

---

Verdict

Approve with minor suggestions. The core fix is correct, well-tested, and the documentation is thorough. The concerns raised are minor polish items rather than blockers.

[colehurwitz]

Review Feedback Addressed

Commit 8b4e834 addresses the minor suggestions from the latest review:

1. Lazy import comment (bob.py:179): Clarified to explicitly state the circular dependency: "ceo_completion imports runners, so module-level import would cycle"

2. Warning log level (usage.py:132): Changed log.debug → log.warning for ceiling warning event failures, improving production diagnosability

The third concern (daily limit removal risk) was acknowledged as a design tradeoff per user preference with per-cycle limits still providing protection.

Ready for merge.

[colehurwitz]

Code Review: PR #136

Overview

This PR raises the Bob Shell per-cycle limit from 3→8, removes the daily limit entirely, and adds early warnings when ≤2 invocations remain. It also fixes a bug where BobRunner wasn't tracking cycle_start correctly across invocations, consolidates runner documentation into README.md, and updates factory.md scope.

---

Analysis

Correctness

• The core bug fix is solid: get_runner() now passes project_path to BobRunner, which reads cycle_start from cycle.json. This ensures invocations accumulate correctly within a cycle.
• The lazy import in factory/runners/bob.py:204-207 is necessary to avoid circular imports — good approach.
• count_cycle_invocations() returning 0 when cycle_start is None is correct — without a cycle start, there's nothing to count.

Style & Conventions

• Uses X | Y union types ✓
• Follows project structlog pattern ✓
• Test classes are well-organized with descriptive docstrings ✓

Potential Issues

1. Return type change in check_ceilings(): Changed from -> None to -> CeilingWarning | None. This is a breaking change for any external callers that might be checking the return value. However, the function previously had no return statement, so existing callers ignoring the return should still work.

2. Silenced exception in _emit_warning_event(): Appropriate — failing to emit a warning shouldn't abort the invocation.

3. _budget_allows_respawn() simplification: Now always returns True. The docstring correctly explains why: per-cycle limits are enforced within BobRunner during execution, not at respawn decision time.

Test Coverage

• Excellent coverage of the new behavior
• TestCeilingAccumulationAcrossInvocations is a good regression test for the bug being fixed
• Tests verify both the warning case (≤2 remaining) and no-warning case (>2 remaining)

Documentation

• README.md Runners section is clear and well-organized
• CLAUDE.md updated appropriately
• factory.md scope change makes sense (README.md → modifiable)

---

Suggestions

1. Minor: The comment in count_cycle_invocations() could be slightly clearer — consider: "Without cycle_start, we can't determine which invocations are in-cycle, so return 0."

2. Consider: The warning threshold of ≤2 is hardcoded. If the per-cycle limit is lowered (e.g., to 3), ≤2 remaining means a warning fires on the first successful invocation. This is fine behavior, but worth documenting.

---

Verdict

Approve — This is a well-structured PR that fixes a real bug (cycle_start not persisting across invocations), simplifies the codebase by removing the daily limit, adds useful early warnings, and includes thorough tests. The documentation consolidation from PR #129 is a nice addition.

[akashgit]

All issues addressed: per-cycle ceiling accumulation fix is solid (threads project_path → BobRunner → cycle.json), factory.md scope leak reverted, minor items (lazy import comment, log.warning) done. Rebased on main, all tests pass. Ship it.