refactor(agent)!: slim the tablassert agent CLI surface - #60
Conversation
Remove three knobs that were dead weight or redundant and fold the agent workspace under the shared `.tablassert/` parent: - Drop `--qc-threshold`/`-qt`: echoed into the run metrics but never gated any decision (accept/MAPPED is driven solely by `--map-threshold` coverage). `qc_pass_rate` is still recorded per record. - Drop `--executor`/`-e` and its Docker sandbox option: model-written code now always runs on the in-process `local` executor (pinned in `build_agent`). - Drop `--no-fetch`/`-nf`: the supervisor always fetches the PMC payload; the snapshot-replay branch and its two supervisor tests are removed. - Lower the `--map-threshold` default from 0.8 to 0.25. - Move the agent workspace default from `.tablassert-agent/` to `.tablassert/agent/` (already auto-created and git-ignored via `*.tablassert/`). Docs, the docs-contract test, and the CHANGELOG are updated to match; no test relies on the old default (all pass it explicitly).
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughThe ChangesAgent CLI Surface Simplification
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/tablassert/agent.py (2)
2143-2154: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve aggregate metrics across checkpoint/resume.
all_metricsis recreated empty on every invocation, while terminal records are skipped. On a resume with no new work, this assignment resets token, step, and tool-call totals to zero; a partial resume also discards prior totals. Seed these counters from persisted metrics or aggregate per-record metrics before overwritingstate.metrics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tablassert/agent.py` around lines 2143 - 2154, Update the metrics aggregation before assigning state.metrics to preserve persisted totals across checkpoint/resume. When all_metrics is empty or records are skipped, seed total_tokens, total_steps, total_tool_calls, failed_tool_calls, wrong_tool_calls, and redundant_tool_calls from existing state.metrics or per-record aggregates, while retaining newly computed values for fresh work.
1623-1632: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftClarify that
executor_type: "local"is not a sandbox. Model-generated code still runs against the host process; the minimal import allowlist only narrows imports and does not provide OS-level isolation. Add an explicit warning indocs/agent.md, or move untrusted execution into an isolated process/container.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tablassert/agent.py` around lines 1623 - 1632, The Agent configuration’s executor_type "local" runs model-generated code in the host process and is not a sandbox; document this explicitly in docs/agent.md near the executor configuration, including that additional_authorized_imports only restricts imports and provides no OS-level isolation. No direct code change is required in src/tablassert/agent.py unless untrusted execution is instead moved to an isolated process or container.
🧹 Nitpick comments (2)
docs/agent.md (1)
126-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the non-migration of existing workspaces. The new default does not automatically discover
.tablassert-agent/checkpoints.
docs/agent.md#L126-L148: explain that users must pass the old--state-diror migrate the directory before resume.CHANGELOG.md#L8-L8: add the same breaking-change caveat.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/agent.md` around lines 126 - 148, Document that existing .tablassert-agent/ workspaces are not automatically discovered under the new default: in docs/agent.md lines 126-148, instruct users to pass the previous --state-dir or migrate the workspace before resuming; add the same breaking-change caveat to CHANGELOG.md line 8.tests/test_agent_cli.py (1)
97-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winProtect the removed-flag contract with negative tests.
tests/test_agent_cli.py#L97-L108: assert the new default state directory and parser rejection of--executor,--no-fetch, and--qc-threshold.tests/test_agent_docs.py#L37-L38: assert those removed flags are absent fromdocs/agent.md.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_agent_cli.py` around lines 97 - 108, The test_agent_cli.py agent flag-parsing coverage must also assert the new default state directory and that parsing rejects --executor, --no-fetch, and --qc-threshold. In tests/test_agent_docs.py lines 37-38, add assertions that docs/agent.md omits all three removed flags; update only these specified test sites.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/agent.md`:
- Line 130: Update the fenced workspace-tree code block in the documentation
around the `.tablassert/agent/` listing to specify an appropriate language,
preferably text, after the opening fence so it satisfies markdownlint MD040.
---
Outside diff comments:
In `@src/tablassert/agent.py`:
- Around line 2143-2154: Update the metrics aggregation before assigning
state.metrics to preserve persisted totals across checkpoint/resume. When
all_metrics is empty or records are skipped, seed total_tokens, total_steps,
total_tool_calls, failed_tool_calls, wrong_tool_calls, and redundant_tool_calls
from existing state.metrics or per-record aggregates, while retaining newly
computed values for fresh work.
- Around line 1623-1632: The Agent configuration’s executor_type "local" runs
model-generated code in the host process and is not a sandbox; document this
explicitly in docs/agent.md near the executor configuration, including that
additional_authorized_imports only restricts imports and provides no OS-level
isolation. No direct code change is required in src/tablassert/agent.py unless
untrusted execution is instead moved to an isolated process or container.
---
Nitpick comments:
In `@docs/agent.md`:
- Around line 126-148: Document that existing .tablassert-agent/ workspaces are
not automatically discovered under the new default: in docs/agent.md lines
126-148, instruct users to pass the previous --state-dir or migrate the
workspace before resuming; add the same breaking-change caveat to CHANGELOG.md
line 8.
In `@tests/test_agent_cli.py`:
- Around line 97-108: The test_agent_cli.py agent flag-parsing coverage must
also assert the new default state directory and that parsing rejects --executor,
--no-fetch, and --qc-threshold. In tests/test_agent_docs.py lines 37-38, add
assertions that docs/agent.md omits all three removed flags; update only these
specified test sites.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 05429a04-18b8-43bd-bb35-5cf167611179
📒 Files selected for processing (9)
.gitignoreCHANGELOG.mddocs/agent.mdsrc/tablassert/agent.pysrc/tablassert/cli.pytests/test_agent_cli.pytests/test_agent_docs.pytests/test_agent_fetch.pytests/test_agent_supervisor.py
💤 Files with no reviewable changes (2)
- .gitignore
- tests/test_agent_supervisor.py
Addresses CodeRabbit's one actionable suggestion on PR #60: the directory-listing fence had no language, tripping markdownlint MD040.
1 participant
Why
tablassert agentaccumulated knobs that were either unused or redundant, and its workspace lived in a standalone.tablassert-agent/dir instead of the shared.tablassert/parent. This slims the surface down to what actually drives behavior.What (breaking)
--qc-threshold/-qt. It was echoed into the run metrics but never gated any decision — accept/MAPPEDis driven solely by--map-thresholdcoverage, so the flag implied an enforcement that didn't exist. (qc_pass_rateis still measured and recorded per record.)--executor/-eand the Docker sandbox. Model-written code now always runs on the in-processlocalexecutor, pinned inbuild_agent. Alocal-only flag would be dead weight, so the flag goes entirely.--no-fetch/-nf. The supervisor always fetches the PMC payload; the snapshot-replay branch is deleted.--map-thresholddefault0.8→0.25— a more achievable mapping bar for real articles..tablassert-agent/→.tablassert/agent/— under the shared, already-auto-created and git-ignored.tablassert/parent (utils.BASE).Files
src/tablassert/cli.py,src/tablassert/agent.py— flag/param removal, default changes, fetch-branch collapse,executor_typepinned tolocal,qc_thresholddropped from the metrics dict.tests/— updatedtest_agent_cli.py+test_agent_docs.py; deleted the twotest_supervisor_no_fetch_*tests (removed functionality); reworded a docstring. No dead imports remain (yaml/_column_cfgstill used).docs/agent.md— stripped the removed flags, renamed the workspace path, deleted the Docker-sandbox defense bullet, fixed the re-download prose..gitignore— dropped the now-redundant.tablassert-agent/line.CHANGELOG.md— Breaking Changes bullet under Unreleased.Testing
uv run tablassert agent --help—--map-thresholdshows[default: 0.25],--state-dirshows[default: .tablassert/agent];--qc-threshold,--executor,--no-fetchare gone.uv sync --extra agent && uv run pytest -q tests/test_agent_*.py tests/test_cover_agent_propose.py→ 132 passed.ruff check+ruff format --checkclean on all edited files.Migration notes
--qc-threshold,--executor, or--no-fetchnow errors as an unknown flag — remove them..tablassert-agent/workspaces are not migrated: re-run the agent or move the dir to.tablassert/agent/. Configs'source.localis absolute, so a moved workspace still needssource.localfixed (unchanged caveat).Summary by CodeRabbit
Breaking Changes
tablassert agentCLI by removing QC, executor, and no-fetch options..tablassert/agent/.Documentation