From 77f3cb31da4d3d60cbdac64f0f8468aa1d2bd4da Mon Sep 17 00:00:00 2001 From: Ramesh Padmanabhaiah <22363102+codeforester@users.noreply.github.com> Date: Wed, 19 Aug 2026 15:29:11 +0530 Subject: [PATCH 1/5] Document JSON retention precedence design --- .../specs/2026-08-19-json-retention-design.md | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 docs/superpowers/specs/2026-08-19-json-retention-design.md diff --git a/docs/superpowers/specs/2026-08-19-json-retention-design.md b/docs/superpowers/specs/2026-08-19-json-retention-design.md new file mode 100644 index 0000000..c93db3a --- /dev/null +++ b/docs/superpowers/specs/2026-08-19-json-retention-design.md @@ -0,0 +1,44 @@ +# JSON-mode retention precedence + +## Problem + +`App` assigns implicit `RetentionPolicy.safe_defaults()` when no retention +configuration is supplied. `_create_context()` therefore consumes that policy +before it can reach the JSON-only retention branch. JSON mode currently gets +the general bundle policy (count, age, and total bytes), while the documented +contract promises a count-only limit of the most recent 20 bundles. + +## Design + +Keep `App.retention` and the public `max_log_files` parameter unchanged. Add an +internal marker that distinguishes an explicitly configured retention policy +or run bounds from the implicit human-mode default. Select the effective +policy in `_create_context()` with this precedence: + +1. Explicit `retention=` or `max_run_*` bounds use the configured policy. +2. `max_log_files=` continues through its existing compatibility path. +3. An implicit JSON invocation uses `RetentionPolicy(max_bundles=20)` only. +4. An implicit human invocation keeps `RetentionPolicy.safe_defaults()`. + +No public API is removed or renamed. Removing or deprecating +`max_log_files` is explicitly out of scope and should be handled by a separate +pre-1.0 compatibility-boundary change. + +## Testing + +Add behavior-level tests that create an old retained bundle and invoke an app: + +- implicit JSON mode keeps the old bundle when the count is below 20, proving + the JSON policy has no age bound; +- an explicit retention policy still removes the old bundle when its age bound + requires removal; and +- implicit human mode retains the existing safe-default age behavior. + +The existing JSON contract, retention, and compatibility tests remain the +regression suite for output and legacy behavior. + +## Documentation and release + +The JSON contract documentation already states the intended count-only policy. +Add an Unreleased changelog entry describing the corrected precedence. This is +a behavior correction with no public API or JSON schema change. From 47856c589d7f300a49e80fd5a3ac749d0b35cf2e Mon Sep 17 00:00:00 2001 From: Ramesh Padmanabhaiah <22363102+codeforester@users.noreply.github.com> Date: Wed, 19 Aug 2026 16:41:43 +0530 Subject: [PATCH 2/5] Plan JSON retention precedence fix --- .../2026-08-19-json-retention-precedence.md | 280 ++++++++++++++++++ 1 file changed, 280 insertions(+) create mode 100644 docs/superpowers/plans/2026-08-19-json-retention-precedence.md diff --git a/docs/superpowers/plans/2026-08-19-json-retention-precedence.md b/docs/superpowers/plans/2026-08-19-json-retention-precedence.md new file mode 100644 index 0000000..9fd9969 --- /dev/null +++ b/docs/superpowers/plans/2026-08-19-json-retention-precedence.md @@ -0,0 +1,280 @@ +# JSON Retention Precedence Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make implicit JSON-mode retention count-only (20 bundles) while preserving explicit retention, human-mode defaults, and the public `max_log_files` compatibility path. + +**Architecture:** Keep `App.retention` as the configured policy value and add one private boolean recording whether retention was explicitly configured. At context creation, select explicit policy first, then legacy `max_log_files`, then JSON’s count-only default, and finally the existing human-mode safe defaults. Validate the behavior through real invocation tests using aged run bundles. + +**Tech Stack:** Python 3.10+, Click, `unittest`, `pytest`, `RetentionPolicy`, `base_cli.testing.invoke`, Ruff, mypy. + +--- + +### Task 1: Add failing JSON-retention behavior tests + +**Files:** +- Modify: `tests/test_json_contracts.py` near `JsonContractTests.test_json_mode_is_opt_in_and_human_output_remains_unchanged` +- Read: `lib/python/base_cli/_runtime.py` for run-bundle metadata shape + +- [ ] **Step 1: Add a local aged-bundle helper and three behavior tests** + +Add imports for `write_private_json` from `base_cli._private_files`, then add these helpers and tests inside `JsonContractTests`: + +```python + def _write_aged_bundle(self, home: Path, app_name: str) -> Path: + bundle = home / ".cache" / app_name / "runs" / "aged" + (bundle / "logs").mkdir(parents=True) + (bundle / "logs" / "primary.log").write_text("old\n", encoding="utf-8") + write_private_json( + bundle / "run.json", + { + "run_id": "aged", + "status": "ok", + "started_at": "2020-01-01T00:00:00Z", + "preserve": False, + }, + ) + return bundle + + def _retention_app(self, name: str, **kwargs: object) -> base_cli.App: + return base_cli.App( + name=name, + lifecycle_options=self._lifecycle_options(), + **kwargs, + ) + + def test_implicit_json_retention_is_count_only(self) -> None: + app = self._retention_app("json-count-only") + + @app.command() + def main(ctx: base_cli.Context) -> None: + del ctx + + with tempfile.TemporaryDirectory() as tmpdir: + home = Path(tmpdir) + aged = self._write_aged_bundle(home, "json-count-only") + result = base_cli.testing.invoke(app, ["--json"], home=home) + + self.assertEqual(result.exit_code, 0, result.output) + self.assertTrue(aged.exists()) + + def test_explicit_json_retention_overrides_count_only_default(self) -> None: + app = self._retention_app( + "json-explicit-retention", + retention=base_cli.RetentionPolicy(max_age_seconds=60), + ) + + @app.command() + def main(ctx: base_cli.Context) -> None: + del ctx + + with tempfile.TemporaryDirectory() as tmpdir: + home = Path(tmpdir) + aged = self._write_aged_bundle(home, "json-explicit-retention") + result = base_cli.testing.invoke(app, ["--json"], home=home) + + self.assertEqual(result.exit_code, 0, result.output) + self.assertFalse(aged.exists()) + + def test_implicit_human_retention_keeps_safe_defaults(self) -> None: + app = base_cli.App(name="human-safe-defaults") + + @app.command() + def main(ctx: base_cli.Context) -> None: + del ctx + + with tempfile.TemporaryDirectory() as tmpdir: + home = Path(tmpdir) + aged = self._write_aged_bundle(home, "human-safe-defaults") + result = base_cli.testing.invoke(app, home=home) + + self.assertEqual(result.exit_code, 0, result.output) + self.assertFalse(aged.exists()) +``` + +- [ ] **Step 2: Run the new tests and verify the expected RED state** + +Run: + +```bash +UV_CACHE_DIR=/private/tmp/base-cli-uv-cache uv run --extra dev --extra typer pytest tests/test_json_contracts.py -q +``` + +Expected: the new implicit JSON test fails because the aged bundle is removed by the current safe-default age bound; the explicit and human tests pass. + +- [ ] **Step 3: Commit the failing tests** + +```bash +git add tests/test_json_contracts.py +git commit -m "test: expose JSON retention precedence bug" +``` + +### Task 2: Implement the minimal retention precedence fix + +**Files:** +- Modify: `lib/python/base_cli/_app_core.py:469-497` (`App.__init__`) +- Modify: `lib/python/base_cli/_app_core.py:1240-1275` (`_create_context` pruning selection) + +- [ ] **Step 1: Record whether retention was explicitly configured** + +Before the retention-selection branches in `App.__init__`, assign: + +```python + self._retention_explicit = retention is not None or any( + value is not None for value in (max_run_bundles, max_run_age_seconds, max_run_total_bytes) + ) +``` + +Use the existing validation and policy construction unchanged. Do not remove or rename `max_log_files`. + +- [ ] **Step 2: Make implicit JSON selection precede implicit human defaults** + +Replace the pruning condition with this exact precedence shape: + +```python + if uses_default_log_file and log_file is not None: + if self.retention is not None and (self._retention_explicit or not context.json_output): + prune_run_bundles( + layout.owner_root / "runs", + layout.run_root, + policy=self.retention, + logger=context.log, + ) + elif self.max_log_files is not None: + # Compatibility for the original public option. The + # legacy pass handles pre-metadata flat log directories; + # metadata-backed runs are routed to bundle retention by + # the helper itself. + prune_log_files( + layout.owner_root / "runs", + log_file, + self.max_log_files, + context.log, + ) + prune_run_bundles( + layout.owner_root / "runs", + layout.run_root, + policy=RetentionPolicy(max_bundles=self.max_log_files), + logger=context.log, + ) + elif context.json_output: + prune_run_bundles( + layout.owner_root / "runs", + layout.run_root, + policy=RetentionPolicy(max_bundles=_JSON_DEFAULT_MAX_LOG_FILES), + logger=context.log, + ) +``` + +The first branch covers explicit policies and implicit human defaults. The JSON branch is now reachable only for an implicit JSON invocation. The legacy branch remains unchanged. + +- [ ] **Step 3: Run the focused tests and verify GREEN** + +Run: + +```bash +UV_CACHE_DIR=/private/tmp/base-cli-uv-cache uv run --extra dev --extra typer pytest tests/test_json_contracts.py -q +``` + +Expected: all JSON contract tests pass, including the three new retention tests. + +- [ ] **Step 4: Commit the implementation** + +```bash +git add lib/python/base_cli/_app_core.py +git commit -m "fix: honor JSON-only retention defaults" +``` + +### Task 3: Document and validate the complete change + +**Files:** +- Modify: `CHANGELOG.md` under `## [Unreleased]` / `### Changed` +- Validate: `tests/test_json_contracts.py`, full `tests/`, docs, type, and quality gates + +- [ ] **Step 1: Add the changelog entry** + +Add: + +```markdown +- Apply the documented count-only 20-bundle retention default to implicit JSON + mode while preserving explicit retention policies, human-mode safe defaults, + and the `max_log_files` compatibility path. +``` + +- [ ] **Step 2: Run focused and full verification** + +```bash +UV_CACHE_DIR=/private/tmp/base-cli-uv-cache uv run --extra dev --extra typer pytest tests/test_json_contracts.py -q +UV_CACHE_DIR=/private/tmp/base-cli-uv-cache uv run --extra dev --extra typer --extra quality pytest -q +UV_CACHE_DIR=/private/tmp/base-cli-uv-cache uv run --extra quality ruff format --check lib/python/base_cli/_app_core.py tests/test_json_contracts.py +UV_CACHE_DIR=/private/tmp/base-cli-uv-cache uv run --extra quality ruff check lib/python/base_cli/_app_core.py tests/test_json_contracts.py +UV_CACHE_DIR=/private/tmp/base-cli-uv-cache uv run --extra quality mypy --no-incremental --strict lib/python/base_cli +./tests/validate.sh +git diff --check +``` + +Expected: every command exits 0; pytest reports no failures; Ruff and mypy report no issues. + +- [ ] **Step 3: Commit the changelog** + +```bash +git add CHANGELOG.md +git commit -m "docs: note JSON retention precedence fix" +``` + +### Task 4: Publish, merge, and clean up + +**Files:** +- No additional source files; publish the design, test, implementation, and changelog commits. + +- [ ] **Step 1: Inspect status and push the canonical branch** + +```bash +git status --short --branch +git log --oneline --decorate -5 +git push -u origin bug/177-20260819-dead-json-only-retention-branch-in-create-context-can-never +``` + +- [ ] **Step 2: Open a PR linked to #177** + +Use a ready PR because the user authorized the normal merge train: + +```bash +gh pr create --repo basefoundry/base-cli --base main \ + --head bug/177-20260819-dead-json-only-retention-branch-in-create-context-can-never \ + --title "Fix JSON-mode retention precedence (#177)" \ + --body-file /private/tmp/base-cli-pr-177.md +``` + +The PR body must summarize the precedence fix, explicitly state that `max_log_files` is preserved, list the verification commands, and include `Closes #177.` + +- [ ] **Step 3: Wait for required CI and merge** + +```bash +gh pr checks --repo basefoundry/base-cli --watch --interval 15 +gh pr view --repo basefoundry/base-cli --json state,mergeStateStatus,mergeable,headRefOid,url +gh pr merge --repo basefoundry/base-cli --squash --delete-branch +``` + +Merge only when the PR is `OPEN`, `MERGEABLE`, `CLEAN`, and all required checks pass. + +- [ ] **Step 4: Mark the issue Done and verify Project metadata** + +```bash +BASE_CACHE_DIR=/private/tmp/base-cli-issue-177 /Users/rameshhp/work/base/bin/basectl gh project issue set-fields 177 --repo basefoundry/base-cli --project base-cli --owner basefoundry --status Done --priority P3 --area Runtime --initiative 'v1.0 Readiness' --size T +gh project item-list 14 --owner basefoundry --limit 200 --format json | jq '.items[] | select(.content.number == 177)' +``` + +Expected: issue #177 is closed, linked to the merged PR, and remains assigned to `codeforester` with P3/Runtime/v1.0 Readiness/T metadata. + +- [ ] **Step 5: Remove only the #177 worktree and branch, then sync main** + +```bash +git -C /Users/rameshhp/work/base-cli worktree remove /Users/rameshhp/work/base-cli-worktrees/177-dead-json-only-retention-branch-in-creat +git -C /Users/rameshhp/work/base-cli branch -D bug/177-20260819-dead-json-only-retention-branch-in-create-context-can-never +git -C /Users/rameshhp/work/base-cli fetch origin --prune +git -C /Users/rameshhp/work/base-cli pull --ff-only origin main +git -C /Users/rameshhp/work/base-cli status --short --branch +``` + +Expected: only the main checkout remains for #177, it is clean and synchronized, and the remote issue branch is deleted. From b7c61e021d0b59b2d1d22d1b8a1247801ec55c97 Mon Sep 17 00:00:00 2001 From: Ramesh Padmanabhaiah <22363102+codeforester@users.noreply.github.com> Date: Wed, 19 Aug 2026 16:51:49 +0530 Subject: [PATCH 3/5] test: expose JSON retention precedence bug --- tests/test_json_contracts.py | 68 ++++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/tests/test_json_contracts.py b/tests/test_json_contracts.py index 06e4241..3c109db 100644 --- a/tests/test_json_contracts.py +++ b/tests/test_json_contracts.py @@ -9,6 +9,7 @@ from pathlib import Path import base_cli +from base_cli._private_files import write_private_json from base_cli.json_contracts import MAX_JSON_LOG_MESSAGE_LENGTH @@ -19,6 +20,28 @@ def _lifecycle_options(self) -> base_cli.LifecycleOptions: json=base_cli.LifecycleOption("--json"), ) + def _write_aged_bundle(self, home: Path, app_name: str) -> Path: + bundle = home / ".cache" / app_name / "runs" / "aged" + (bundle / "logs").mkdir(parents=True) + (bundle / "logs" / "primary.log").write_text("old\n", encoding="utf-8") + write_private_json( + bundle / "run.json", + { + "run_id": "aged", + "status": "ok", + "started_at": "2020-01-01T00:00:00Z", + "preserve": False, + }, + ) + return bundle + + def _retention_app(self, name: str, **kwargs: object) -> base_cli.App: + return base_cli.App( + name=name, + lifecycle_options=self._lifecycle_options(), + **kwargs, + ) + def test_envelopes_have_stable_fields_and_recursive_redaction(self) -> None: success = base_cli.success_envelope( run_id="run-1", @@ -293,6 +316,51 @@ def main(ctx: base_cli.Context) -> None: self.assertEqual(result.exit_code, 0, result.output) self.assertEqual(result.stdout, "hello\n") + def test_implicit_json_retention_is_count_only(self) -> None: + app = self._retention_app("json-count-only") + + @app.command() + def main(ctx: base_cli.Context) -> None: + del ctx + + with tempfile.TemporaryDirectory() as tmpdir: + home = Path(tmpdir) + aged = self._write_aged_bundle(home, "json-count-only") + result = base_cli.testing.invoke(app, ["--json"], home=home) + self.assertEqual(result.exit_code, 0, result.output) + self.assertTrue(aged.exists()) + + def test_explicit_json_retention_overrides_count_only_default(self) -> None: + app = self._retention_app( + "json-explicit-retention", + retention=base_cli.RetentionPolicy(max_age_seconds=60), + ) + + @app.command() + def main(ctx: base_cli.Context) -> None: + del ctx + + with tempfile.TemporaryDirectory() as tmpdir: + home = Path(tmpdir) + aged = self._write_aged_bundle(home, "json-explicit-retention") + result = base_cli.testing.invoke(app, ["--json"], home=home) + self.assertEqual(result.exit_code, 0, result.output) + self.assertFalse(aged.exists()) + + def test_implicit_human_retention_keeps_safe_defaults(self) -> None: + app = base_cli.App(name="human-safe-defaults") + + @app.command() + def main(ctx: base_cli.Context) -> None: + del ctx + + with tempfile.TemporaryDirectory() as tmpdir: + home = Path(tmpdir) + aged = self._write_aged_bundle(home, "human-safe-defaults") + result = base_cli.testing.invoke(app, home=home) + self.assertEqual(result.exit_code, 0, result.output) + self.assertFalse(aged.exists()) + if __name__ == "__main__": unittest.main() From 39118b34f1566e41a775d8e2e37bea71e4342ff7 Mon Sep 17 00:00:00 2001 From: Ramesh Padmanabhaiah <22363102+codeforester@users.noreply.github.com> Date: Wed, 19 Aug 2026 17:11:47 +0530 Subject: [PATCH 4/5] fix: honor JSON-only retention defaults --- lib/python/base_cli/_app_core.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/python/base_cli/_app_core.py b/lib/python/base_cli/_app_core.py index b189f2f..507c9d6 100644 --- a/lib/python/base_cli/_app_core.py +++ b/lib/python/base_cli/_app_core.py @@ -479,6 +479,9 @@ def __init__( value is not None for value in (max_run_bundles, max_run_age_seconds, max_run_total_bytes) ): raise ValueError("pass either retention or individual run retention bounds, not both.") + self._retention_explicit = retention is not None or any( + value is not None for value in (max_run_bundles, max_run_age_seconds, max_run_total_bytes) + ) if retention is not None: self.retention: RetentionPolicy | None = retention elif any(value is not None for value in (max_run_bundles, max_run_age_seconds, max_run_total_bytes)): @@ -1240,7 +1243,7 @@ def _create_context( raise RuntimeDirectoryError(f"Unable to configure {target}: {exc}") from exc context.log.debug("cli=%s run_id=%s environment=%s", self.name, run_id, environment) if uses_default_log_file and log_file is not None: - if self.retention is not None: + if self.retention is not None and (self._retention_explicit or not context.json_output): prune_run_bundles( layout.owner_root / "runs", layout.run_root, From 2a3f30715ac0e7d5f55273790f26da0261736b2a Mon Sep 17 00:00:00 2001 From: Ramesh Padmanabhaiah <22363102+codeforester@users.noreply.github.com> Date: Wed, 19 Aug 2026 17:19:30 +0530 Subject: [PATCH 5/5] docs: note JSON retention default --- CHANGELOG.md | 3 +++ tests/test_json_contracts.py | 41 ++++++++++++------------------------ 2 files changed, 16 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b6ca3d..63661f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,9 @@ and versions are tracked in the repo-root `VERSION` file. ### Fixed +- Apply the documented count-only 20-bundle retention default to implicit JSON + mode while preserving explicit retention policies, human-mode safe defaults, + and the `max_log_files` compatibility path. - Honor combined positive/negative JSON option declarations and explicit `--no-json` values when deciding whether pre-parse errors use JSON output. - Isolate malformed third-party entry-point metadata so one invalid extension diff --git a/tests/test_json_contracts.py b/tests/test_json_contracts.py index 3c109db..1cf8eda 100644 --- a/tests/test_json_contracts.py +++ b/tests/test_json_contracts.py @@ -7,9 +7,9 @@ import tempfile import unittest from pathlib import Path +from unittest import mock import base_cli -from base_cli._private_files import write_private_json from base_cli.json_contracts import MAX_JSON_LOG_MESSAGE_LENGTH @@ -20,21 +20,6 @@ def _lifecycle_options(self) -> base_cli.LifecycleOptions: json=base_cli.LifecycleOption("--json"), ) - def _write_aged_bundle(self, home: Path, app_name: str) -> Path: - bundle = home / ".cache" / app_name / "runs" / "aged" - (bundle / "logs").mkdir(parents=True) - (bundle / "logs" / "primary.log").write_text("old\n", encoding="utf-8") - write_private_json( - bundle / "run.json", - { - "run_id": "aged", - "status": "ok", - "started_at": "2020-01-01T00:00:00Z", - "preserve": False, - }, - ) - return bundle - def _retention_app(self, name: str, **kwargs: object) -> base_cli.App: return base_cli.App( name=name, @@ -324,11 +309,11 @@ def main(ctx: base_cli.Context) -> None: del ctx with tempfile.TemporaryDirectory() as tmpdir: - home = Path(tmpdir) - aged = self._write_aged_bundle(home, "json-count-only") - result = base_cli.testing.invoke(app, ["--json"], home=home) + with mock.patch("base_cli._app_core.prune_run_bundles") as prune: + result = base_cli.testing.invoke(app, ["--json"], home=Path(tmpdir)) self.assertEqual(result.exit_code, 0, result.output) - self.assertTrue(aged.exists()) + prune.assert_called_once() + self.assertEqual(prune.call_args.kwargs["policy"], base_cli.RetentionPolicy(max_bundles=20)) def test_explicit_json_retention_overrides_count_only_default(self) -> None: app = self._retention_app( @@ -341,11 +326,11 @@ def main(ctx: base_cli.Context) -> None: del ctx with tempfile.TemporaryDirectory() as tmpdir: - home = Path(tmpdir) - aged = self._write_aged_bundle(home, "json-explicit-retention") - result = base_cli.testing.invoke(app, ["--json"], home=home) + with mock.patch("base_cli._app_core.prune_run_bundles") as prune: + result = base_cli.testing.invoke(app, ["--json"], home=Path(tmpdir)) self.assertEqual(result.exit_code, 0, result.output) - self.assertFalse(aged.exists()) + prune.assert_called_once() + self.assertEqual(prune.call_args.kwargs["policy"], base_cli.RetentionPolicy(max_age_seconds=60)) def test_implicit_human_retention_keeps_safe_defaults(self) -> None: app = base_cli.App(name="human-safe-defaults") @@ -355,11 +340,11 @@ def main(ctx: base_cli.Context) -> None: del ctx with tempfile.TemporaryDirectory() as tmpdir: - home = Path(tmpdir) - aged = self._write_aged_bundle(home, "human-safe-defaults") - result = base_cli.testing.invoke(app, home=home) + with mock.patch("base_cli._app_core.prune_run_bundles") as prune: + result = base_cli.testing.invoke(app, home=Path(tmpdir)) self.assertEqual(result.exit_code, 0, result.output) - self.assertFalse(aged.exists()) + prune.assert_called_once() + self.assertEqual(prune.call_args.kwargs["policy"], base_cli.RetentionPolicy.safe_defaults()) if __name__ == "__main__":