feat(build): require plan-id, relocate results, extend ledger - #1075
Conversation
…ery plan_id guard
…esults directory
Build output now lands under {plan_dir}/build-results/{scope}/ so a build's logs
live with the plan that caused it, instead of accumulating in the shared
.plan/temp/build-output tree. get_build_results_dir in tools-file-ops is the
single owner of that path; the NO_PLAN sentinel is the one main-anchored branch,
because a plan-less build belongs to no worktree and cwd-relative resolution
would scatter its results across every worktree that ran one.
Co-Authored-By: Claude <noreply@anthropic.com>
…, and outcome Record what actually ran, how long it took, and what it returned on the existing kind=build ledger row. build_record now emits command, duration_seconds, and outcome alongside the unchanged args field, so the executor-argv layer and the wrapper-reported layer stay distinguishable. The executor template sources command and duration from the build wrapper's own reported payload rather than from the executor argv; a malformed payload still produces a ledger row and still never changes the dispatch's exit code. Tests cover the three new fields plus args immutability, an end-to-end dispatch asserting the row carries the wrapper-reported command and duration, and the malformed-payload path. Also corrects the _build_entry docstring in test_pre_commit_verify_freshness.py, which claimed to mirror build_record's shape when it mirrors only the gate-relevant subset (kind, status, worktree_sha). Co-Authored-By: Claude <noreply@anthropic.com>
Deliverable 7: age out build results safely, never offering a running plan's
results for deletion.
D5 relocated build output into `{plan_dir}/build-results/`, which made it a
durable per-plan artifact but left it with no lifecycle: a real plan's results
are aged only once `archive-plan` moves the whole directory under
`archived-plans/`, and the `NO_PLAN` sentinel is never archived at all.
- `constants` gains `CLEANUP_TARGET_BUILD_RESULTS` and its member of the closed
`CLEANUP_TARGETS` tuple, so the dispatcher and the argparse choices both gain
the target from one edit.
- `_config_defaults` seeds `build_results_days` DERIVED from the same declared
value as `archived_plans_days` rather than restating its literal, so the two
shipped defaults cannot drift. `_cmd_cleanup.get_retention_settings`
additionally backfills an absent `build_results_days` from the project's
EFFECTIVE `archived_plans_days` — a project that lengthened plan archival
lengthens build-result retention in lock-step. Membership is tested against
the raw persisted mapping, not the normalized one, so an explicitly
configured value always wins over the backfill.
- `_cmd_cleanup` gains the cleaner and its `get_status` counterpart. The plan
set is ENUMERATED FROM THE STORE, never hand-listed, and a LIVE plan's tree
is skipped entirely — a plan is live for as long as its directory carries a
`status.json` marker, the same marker `require_plan_exists` and
`manage-status list-orphans` already treat as the managed-plan signal. A live
plan's results appear in NEITHER the cleanup nor the `cleanup-status`
population, so they are never counted as reclaimable rather than merely
spared deletion. Only a plan directory with no marker, and the never-archived
`NO_PLAN` sentinel, are subject to the age threshold. Paths come from
`file_ops.get_build_results_dir` — the declared single owner — so this
surface cannot drift from the resolver the build wrappers write through.
- `run_config` needed no enum change (its choices already derive from
`CLEANUP_TARGETS`); it gains the help-surface example for the new target.
Docs: the knob and the widened `--target` enum are mirrored into
manage-config data-model.md / api-reference.md / SKILL.md, manage-run-config
SKILL.md, and marshall-steward menu-maintenance.md, following the
`no-plan-bodies` precedent. data-model.md carries the canonical build-results
retention semantics; the other surfaces xref it. menu-maintenance.md's cleanup
output example was additionally corrected — it advertised a `deleted[N]` TOON
table the verb has never emitted.
Tests: the live-plan guard is asserted on BOTH surfaces (never deleted, never
counted), plus non-live ageing, sentinel ageing with marker survival, dry-run,
absent-tree no-op, target scoping, `--target all` inclusion, the effective-value
backfill, and explicit-wins-over-backfill. The backfill fixtures set
`archived_plans_days` to 30 — far from the shipped 5 — so a reader that fell
back to the shipped default fails instead of passing by coincidence. Fixtures
are written through `get_build_results_dir` itself, not a reconstructed literal.
A new CLI test pins the RENDERED `--target` help against `CLEANUP_TARGETS`, so
re-hardcoding the enum fails loudly.
Verified live: with this plan holding 21 build-result files,
`cleanup --target build-results --dry-run` reports zero reclaimable bytes and
`cleanup-status` reports `build_results_total: 0`.
Co-Authored-By: Claude <noreply@anthropic.com>
… contracts
Deliverable 8: bring the three published build extension-point standards back
in line with what D3, D4 and D5 actually shipped.
- `build-execution.md`: R1's log-file location and its component table now state
the plan-scoped path, with `{plan_id}` as a first-class component and the
`NO_PLAN` sentinel named once as part of the input contract rather than
repeated per example. The rationale block is rewritten — the old one argued
for `.plan/temp/` on gitignore/cleanup grounds that no longer apply; the new
one states the durable-per-plan-artifact rationale, the worktree-move
property, and the `build_results_days` retention that replaced temp cleanup.
The `execute_direct()` parameter table gains `plan_id` AND the three
parameters it had silently omitted (`env_vars`, `working_dir`,
`explicit_timeout`), so the table stops under-reporting the signature it
indexes; `plan_id` is documented as required keyword-only with no default,
with the wrapper-must-forward-on-retry obligation stated. The
execution-lifecycle diagram's `create_log_file` step was stale in two ways
(it still showed a `project_dir` parameter the function no longer takes and
omitted `plan_id`) and is corrected. Every TOON and JSON example and the
persistence-points table now quote the plan-scoped path.
- `build-systems-common.md`: the Log File Pattern block states the plan-scoped
path and xrefs R1 for the plan-scoping contract instead of restating it.
- `build-api-reference.md`: the two `run` output examples are re-pointed. The
`--plan-id` description no longer presents the flag as optional-meaning-none
— it is ALWAYS resolved, and omitting it selects the `NO_PLAN` sentinel, so
the omission is an attribution decision (log placement and ledger stamping)
rather than an absence. The subcommand availability table is completed
against the argparse-derived roster: it omitted `run-config-key` (all four
tools), maven's `rewrite-log`, and pyproject's `resolve-test-scope`, and so
misreported the build-class surface as six subcommands when it is nine
across 27 registrations. The universal `--plan-id` / `--project-dir` pair is
stated once for the whole table rather than per row, matching its post-D3
universality.
The one deliberate survivor of the retired `.plan/temp/build-output` literal is
`test/plan-marshall/tools-integration-ci/fixtures/ci-logs/SOURCE.md`, whose four
hits are dated capture-provenance records; rewriting them would falsify the
provenance rather than reconcile it.
Verified: `git grep build-output` over `extension-api/` returns nothing, and the
scoped plugin-doctor quality-gate reports `status: pass` with 0 findings across
all 31 rules.
Co-Authored-By: Claude <noreply@anthropic.com>
The NO_PLAN sentinel's build-results directory joins merge.lock, run-configuration.json, lessons-learned/, merge-queue.json, and orchestrator/ as a per-repo cross-session corpus resolved through resolve_main_anchored_path. Three count claims in ADR-002 stated five. Only the sentinel's build results are main-anchored; a real plan's build-results/ sits inside its plan directory and moves with it. Phrasing aligned with the same clarification already carried in cwd-policy.md. Co-Authored-By: Claude <noreply@anthropic.com>
…laims Pre-submission self-review findings, all four fixed: 1. ADR-002 said "exactly FIVE" main-anchored corpora in five further places the earlier count fix missed, including the authoritative bullet list. Added the sixth member and reconciled the heading and the four prose references. 2. cwd-policy.md carried the same split: its primary definitional statement said five with a five-item list while its later assertion already said six. A reader consults the former first. 3. add_project_dir_arg's docstring named two wrapper-local registrations; there are three. The omitted one is pyproject_build.py::_register_resolve_test_scope, whose vacuous plan_id guard this change fixed. 4. The roster-universality guarantee and build-api-reference's "COMPLETE build-class surface" both over-claimed: the roster covers build_main-routed wrappers, while the executor treats every script under a build-* skill as build-class. js_coverage.py declares its own parser and is outside the roster. Both claims narrowed to what holds. Co-Authored-By: Claude <noreply@anthropic.com>
The NO_PLAN sentinel is truthy, so every "is this build plan-bound?" site spelled out `plan_id and plan_id != NO_PLAN_SENTINEL` (or its inverse) plus a comment re-explaining why falsiness alone is not enough. Eight sites, eight copies of the reasoning, two polarities. Adds names_real_plan() next to the sentinel it interprets and routes all eight guards through it. The TypeGuard[str] return is load-bearing rather than decorative: the inline expression narrowed str | None to str for free, and a plain bool helper broke that in six places. Also drops the unused help_text parameter from add_root_arg — both call sites use the default. Co-Authored-By: Claude <noreply@anthropic.com>
get_build_results_dir derives a directory from plan_id and now creates
it and writes into it. The chain get_build_results_dir -> get_plan_dir
-> get_store_dir('plans', ...) performed no path-safety check, and the
asymmetry was visible in get_store_dir itself: its orchestrator branch
calls _reject_unsafe_entry_id, its plans branch did not.
Not exploitable today — build_main routes every real plan id through
resolve_plan_context, whose manage-status lookup rejects traversal via
validate_plan_id. But that is an incidental side effect of a worktree
lookup, not a deliberate path check: cache the worktree face or
short-circuit the lookup and the escape reopens silently.
Adds containment-by-resolution at the documented single owner, so it
also catches a plan directory symlinked out of the store and never
false-positives on a contained but odd directory name from the GC
enumerator. The unresolved path is still returned, preserving
create_log_file's unresolved_base semantics.
Also documents the secrets discipline on build_record: the kind=build
row now retains a secret-capable command/outcome while the sibling
build_server._audit_log explicitly refuses that data class. Accepted
here (the ledger is gitignored and the same text is co-resident in the
log the row points at), but the log ages out under build_results_days
while the ledger is append-only, so the excerpt outlives its source.
Co-Authored-By: Claude <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cuioss-oliver, your pull request is larger than the review limit of 150000 diff characters
📝 WalkthroughWalkthroughThis change introduces ChangesBuild state and execution flow
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Reviewer Guide 🔍
|
Triage dispositionsIn reply to comment_id:
|
The two-idiom split this plan established existed only as a docstring on one function, while the obligation it creates binds every future consumer of an optional identity across many skills. ADR-015 records both halves as one decision: an absent identity is a stated sentinel value so every record is attributable, and because that sentinel is truthy, every predicate that tested presence must be re-expressed as a meaning test through one named TypeGuard. Adopting the first half without the second reintroduces the vacuous-guard class the change set out to remove. The residual failure mode is recorded as accepted rather than solved: a truthy sentinel silently satisfies any new bare truthiness guard, and the type system cannot detect it, so the seam is a review convention rather than a compiler guarantee. Related to ADR-002 and ADR-009, superseding neither. Co-Authored-By: Claude <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
marketplace/bundles/plan-marshall/skills/extension-api/standards/build-api-reference.md (1)
28-42: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDerive build-class membership from one authoritative source.
The API table fixes the wrapper population at four. The ledger classifier relies on the fixed
_BUILD_CLASS_PREFIXESlist. A newly registeredbuild-*skill can be absent from the documentation and skipkind=buildledger records.
marketplace/bundles/plan-marshall/skills/extension-api/standards/build-api-reference.md#L28-L42: generate the wrapper roster from the argparse-derived build-class roster during documentation generation.marketplace/bundles/plan-marshall/skills/tools-script-executor/templates/execute-script.py.template#L978-L988: derive build-class classification from the registered notation’s build-skill segment instead of_BUILD_CLASS_PREFIXES.As per path instructions, a hardcoded list that mirrors registered handlers must be derived from its authoritative source. Based on learnings, derive detector populations from their authoritative source.
Sources: Path instructions, Learnings
🧹 Nitpick comments (5)
marketplace/bundles/plan-marshall/skills/manage-run-config/SKILL.md (1)
397-397: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive or validate the canonical cleanup target list.
This literal choice list must mirror
CLEANUP_TARGETSinmarketplace/bundles/plan-marshall/skills/tools-file-ops/scripts/constants.py. It can drift when a target is added or removed. Generate this block from the authoritative tuple, or add a non-vacuous contract test that compares the documented choices withCLEANUP_TARGETS.Based on learnings, guarded populations should come from the authoritative source and should not pass trivially over an empty set.
Sources: Path instructions, Learnings
test/plan-marshall/script-shared/test_build_execute.py (1)
512-524: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTighten the
pytest.raises(TypeError)assertion.The bare
TypeErrormatch accepts anyTypeErrorraised anywhere insideexecute_direct_base. Ifplan_idever regains a default, the call proceeds past argument binding: it is unmocked here, so it reachessubprocess.runwithwrapper='/usr/bin/test-tool'andcreate_log_file. An unrelatedTypeErrorfrom that path would keep this test green while the required-argument contract is gone.Add a
matchon the parameter name so the assertion pins the missing-argument case.♻️ Proposed change
def test_plan_id_is_required(self): """Omitting ``plan_id`` is a TypeError — the attribution has no default.""" - with pytest.raises(TypeError): + with pytest.raises(TypeError, match='plan_id'): execute_direct_base(test/plan-marshall/script-shared/test_build_execute_factory.py (1)
47-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth suites read
NO_PLAN_SENTINELfrom a test fixtures module, not from production.test/_shared/_resolve_project_dir_fixtures.pydefinesNO_PLAN_SENTINELas its own string literal rather than re-exporting the production constant frommarketplace_paths. Every sentinel assertion in these two files therefore compares production behavior against a test-local copy of the value.test/plan-marshall/script-shared/test_build_execute.pyalready imports the constant frommarketplace_paths, so the suite is inconsistent about its source of truth. A divergence fails these tests loudly, so this is a duplicated-constant concern, not a false-pass risk.
test/plan-marshall/script-shared/test_build_execute_factory.py#L47-L47: replace the_resolve_project_dir_fixturesimport withfrom marketplace_paths import NO_PLAN_SENTINEL.test/plan-marshall/build-server/test_build_server_client.py#L21-L21: replace the_resolve_project_dir_fixturesimport withfrom marketplace_paths import NO_PLAN_SENTINEL.As a follow-up, consider re-exporting the production constant from
_resolve_project_dir_fixturesso the fixtures module stops carrying its own literal.Source: Learnings
test/plan-marshall/tools-script-executor/test_execute_script.py (2)
589-589: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the build-class notation from
build_class_prefixes().
_BUILD_CLASS_NOTATIONhardcodes a notation that must stay inside the executor template's_BUILD_CLASS_PREFIXES. This PR addstest/_shared/_build_class_roster.py, which exposesbuild_class_prefixes()as the derived read of that same declaration. Build the notation from that source so a prefix rename moves the fixture with it.The current shape fails loudly rather than silently: if the notation stops being build-class, no
kind=buildrow is written and theassert ledger.is_file()on line 641 fails. Treat this as hardening, not a defect.Sources: Path instructions, Learnings
765-772: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the missing
outcomefield.The docstring says an unparseable payload leaves the three wrapper-sourced fields absent, but this test only pins
commandandduration_seconds. Addassert entry['outcome'] is Noneto fully cover the stated degraded ledger shape.♻️ Proposed change
# No wrapper facts are invented from an unusable payload. assert entry['command'] is None assert entry['duration_seconds'] is None + assert entry['outcome'] is None
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cc0867cb-acb7-49e2-b0dc-539a4ca63bbe
📒 Files selected for processing (74)
doc/adr/002-Plan-scoped_operations_move_into_a_cwd-pinned_hermetic_worktree.adocdoc/adr/015-An_absent_identity_is_a_stated_sentinel_value_and_every_presence_guard_becomes_a_meaning_guard.adocmarketplace/bundles/plan-marshall/skills/build-gradle/scripts/_gradle_cmd_discover.pymarketplace/bundles/plan-marshall/skills/build-gradle/scripts/gradle.pymarketplace/bundles/plan-marshall/skills/build-maven/scripts/_maven_cmd_discover.pymarketplace/bundles/plan-marshall/skills/build-maven/scripts/maven.pymarketplace/bundles/plan-marshall/skills/build-pyproject/scripts/_pyproject_execute.pymarketplace/bundles/plan-marshall/skills/build-pyproject/scripts/pyproject_build.pymarketplace/bundles/plan-marshall/skills/build-server-client/SKILL.mdmarketplace/bundles/plan-marshall/skills/build-server-client/scripts/build_server.pymarketplace/bundles/plan-marshall/skills/extension-api/standards/build-api-reference.mdmarketplace/bundles/plan-marshall/skills/extension-api/standards/build-execution.mdmarketplace/bundles/plan-marshall/skills/extension-api/standards/build-systems-common.mdmarketplace/bundles/plan-marshall/skills/manage-change-ledger/SKILL.mdmarketplace/bundles/plan-marshall/skills/manage-change-ledger/scripts/_ledger_core.pymarketplace/bundles/plan-marshall/skills/manage-change-ledger/scripts/manage-change-ledger.pymarketplace/bundles/plan-marshall/skills/manage-config/SKILL.mdmarketplace/bundles/plan-marshall/skills/manage-config/scripts/_config_defaults.pymarketplace/bundles/plan-marshall/skills/manage-config/standards/api-reference.mdmarketplace/bundles/plan-marshall/skills/manage-config/standards/data-model.mdmarketplace/bundles/plan-marshall/skills/manage-execution-manifest/scripts/_manifest_core.pymarketplace/bundles/plan-marshall/skills/manage-execution-manifest/scripts/manage-execution-manifest.pymarketplace/bundles/plan-marshall/skills/manage-execution-manifest/standards/decision-rules.mdmarketplace/bundles/plan-marshall/skills/manage-locks/SKILL.mdmarketplace/bundles/plan-marshall/skills/manage-locks/scripts/merge_lock.pymarketplace/bundles/plan-marshall/skills/manage-run-config/SKILL.mdmarketplace/bundles/plan-marshall/skills/manage-run-config/scripts/_cmd_cleanup.pymarketplace/bundles/plan-marshall/skills/manage-run-config/scripts/run_config.pymarketplace/bundles/plan-marshall/skills/manage-tasks/SKILL.mdmarketplace/bundles/plan-marshall/skills/marshall-steward/references/menu-maintenance.mdmarketplace/bundles/plan-marshall/skills/script-shared/scripts/build/_build_cli.pymarketplace/bundles/plan-marshall/skills/script-shared/scripts/build/_build_execute.pymarketplace/bundles/plan-marshall/skills/script-shared/scripts/build/_build_execute_factory.pymarketplace/bundles/plan-marshall/skills/script-shared/scripts/build/_build_queue_slot.pymarketplace/bundles/plan-marshall/skills/script-shared/scripts/build/_build_result.pymarketplace/bundles/plan-marshall/skills/script-shared/scripts/build/_build_server_protocol.pymarketplace/bundles/plan-marshall/skills/script-shared/scripts/build/_build_shared.pymarketplace/bundles/plan-marshall/skills/script-shared/scripts/marketplace_paths.pymarketplace/bundles/plan-marshall/skills/script-shared/scripts/resolve_project_dir.pymarketplace/bundles/plan-marshall/skills/tools-file-ops/scripts/constants.pymarketplace/bundles/plan-marshall/skills/tools-file-ops/scripts/file_ops.pymarketplace/bundles/plan-marshall/skills/tools-script-executor/standards/cwd-policy.mdmarketplace/bundles/plan-marshall/skills/tools-script-executor/templates/execute-script.py.templatetest/_shared/_build_class_roster.pytest/_shared/_resolve_project_dir_fixtures.pytest/plan-marshall/build-gradle/test_gradle_execute.pytest/plan-marshall/build-gradle/test_gradle_find_project_command.pytest/plan-marshall/build-gradle/test_gradle_run.pytest/plan-marshall/build-maven/test_maven.pytest/plan-marshall/build-maven/test_maven_execute.pytest/plan-marshall/build-maven/test_maven_run.pytest/plan-marshall/build-npm/test_npm.pytest/plan-marshall/build-npm/test_npm_execute.pytest/plan-marshall/build-npm/test_npm_run.pytest/plan-marshall/build-operations/test_timeout_truthful_evidence.pytest/plan-marshall/build-pyproject/test_pyproject_build.pytest/plan-marshall/build-pyproject/test_pyproject_execute.pytest/plan-marshall/build-server/test_build_server_client.pytest/plan-marshall/manage-change-ledger/test_manage_change_ledger.pytest/plan-marshall/manage-config/test_cache_retention_knobs.pytest/plan-marshall/manage-execution-manifest/test_classify_paths_via_extensions.pytest/plan-marshall/manage-run-config/test_cleanup.pytest/plan-marshall/manage-tasks/test_pre_commit_verify_freshness.pytest/plan-marshall/plan-marshall/test_worktree_contract_e2e.pytest/plan-marshall/script-shared/test_build_cli.pytest/plan-marshall/script-shared/test_build_execute.pytest/plan-marshall/script-shared/test_build_execute_factory.pytest/plan-marshall/script-shared/test_build_format.pytest/plan-marshall/script-shared/test_build_queue_slot.pytest/plan-marshall/script-shared/test_build_result.pytest/plan-marshall/script-shared/test_build_shared.pytest/plan-marshall/script-shared/test_build_timeout_truthfulness.pytest/plan-marshall/tools-file-ops/test_file_ops.pytest/plan-marshall/tools-script-executor/test_execute_script.py
|
|
||
| **The never-steal-a-claim invariant.** The infrastructure-config rule is a fallback, never a pre-filter. Running it over the residual unclaimed set — rather than ahead of the build extensions — makes stealing structurally impossible: `build-maven` legitimately claims `src/main/resources/application.yml` as `production`, and a stage-1 placement would strip that path from the extensions' view and silently downgrade a Maven production resource to `config`. This is why the documentation and infrastructure-config rules sit on opposite sides of the build-extension stage; the template rule sits on the same side as infrastructure config, and for the same reason. | ||
|
|
||
| No rule ever assigns `test`, and only the template rule's fail-closed terminal assigns `production` — deliberately, because an unrecognized render target must not be excused from verification. No route is added anywhere: `build.map` is byte-identical for every project, and an owner-less path triggers no build gate. The build extensions (`build-pyproject` / `build-maven` / `build-gradle` / `build-npm`) remain the sole source of production / test / config **claims**; the generic rules are the sole source of owner-less recognition. The six-bucket vocabulary is unchanged — `config` never influences the plan-wide bucket, so an infra-only footprint collapses to `documentation_only` and an infra-plus-code footprint keeps the code bucket. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the hardcoded build-extension roster.
Line 321 lists four build extensions. The authoritative population is extension_discovery.discover_build_extensions(). The document can drift when a build extension is registered or removed.
Refer to dynamically discovered BuildExtensionBase implementations without enumerating them here.
As per path instructions, “Treat a hardcoded list that must mirror a set defined elsewhere — build goals, enum constants, registered handlers, dispatch tables — as a defect unless it is derived from that source at build or run time.”
Source: Path instructions
| def cleanable_build_results_dirs() -> list[tuple[str, Path]]: | ||
| """Return ``(plan_id, build_results_dir)`` for every NON-live results tree. | ||
|
|
||
| The plan set is ENUMERATED FROM THE PLAN STORE, never from a hand-written | ||
| list, so a newly created plan is covered without an edit here. Each entry's | ||
| path comes from :func:`file_ops.get_build_results_dir` — the single owner of | ||
| the build-results path — so this cleanup surface cannot drift away from the | ||
| resolver the build wrappers actually write through. | ||
|
|
||
| Two populations survive the filter: | ||
|
|
||
| * A plan directory whose ``status.json`` is absent (see :func:`plan_is_live`) | ||
| — a leftover the plan lifecycle no longer tracks. | ||
| * The ``NO_PLAN`` sentinel, unconditionally. It is shared and permanent — | ||
| never archived, exactly the condition that made the ``no-plan-bodies`` | ||
| target necessary — so an age threshold is the only lifecycle its build | ||
| results can have. It is appended rather than discovered because its | ||
| results are main-anchored (``get_build_results_dir`` resolves the sentinel | ||
| through the main checkout), so a caller pinned to a worktree would | ||
| otherwise never see it. | ||
|
|
||
| A live plan appears in NEITHER the cleanup nor the ``cleanup-status`` | ||
| population: its build results are not merely spared deletion, they are never | ||
| counted as reclaimable. | ||
| """ | ||
| cleanable: list[tuple[str, Path]] = [] | ||
|
|
||
| plans_dir = PLAN_BASE_DIR / DIR_PLANS | ||
| if plans_dir.is_dir(): | ||
| for plan_dir in sorted(plans_dir.iterdir()): | ||
| if not plan_dir.is_dir() or plan_dir.name == NO_PLAN_SENTINEL: | ||
| continue | ||
| if plan_is_live(plan_dir): | ||
| continue | ||
| cleanable.append((plan_dir.name, get_build_results_dir(plan_dir.name))) | ||
|
|
||
| cleanable.append((NO_PLAN_SENTINEL, get_build_results_dir(NO_PLAN_SENTINEL))) | ||
| return cleanable |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard cleanable_build_results_dirs() against get_build_results_dir raising.
get_build_results_dir(plan_dir.name) can raise ValueError when the resolved directory escapes the plans/ root — for example, a plan directory symlinked outside the tree. Unlike this same call path's own docstring, which says production callers are protected by an upstream validate_plan_id, this call site iterates real on-disk directories directly and has no such upstream guard.
Line 356 has no try/except around the call, so one anomalous plan directory raises out of cleanable_build_results_dirs(), through clean_build_results() or get_status(), and aborts the entire cleanup/cleanup-status command with an unstructured traceback — even though temp, logs, archived-plans, and no-plan-bodies cleanup may have already completed. Sibling helpers in this same file (build_result_files, clean_no_plan_bodies) already catch per-item OSError to keep going; this call should follow the same pattern.
🐛 Proposed fix to skip an unsafe plan directory instead of crashing
if plan_is_live(plan_dir):
continue
- cleanable.append((plan_dir.name, get_build_results_dir(plan_dir.name)))
+ try:
+ cleanable.append((plan_dir.name, get_build_results_dir(plan_dir.name)))
+ except ValueError:
+ continue📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def cleanable_build_results_dirs() -> list[tuple[str, Path]]: | |
| """Return ``(plan_id, build_results_dir)`` for every NON-live results tree. | |
| The plan set is ENUMERATED FROM THE PLAN STORE, never from a hand-written | |
| list, so a newly created plan is covered without an edit here. Each entry's | |
| path comes from :func:`file_ops.get_build_results_dir` — the single owner of | |
| the build-results path — so this cleanup surface cannot drift away from the | |
| resolver the build wrappers actually write through. | |
| Two populations survive the filter: | |
| * A plan directory whose ``status.json`` is absent (see :func:`plan_is_live`) | |
| — a leftover the plan lifecycle no longer tracks. | |
| * The ``NO_PLAN`` sentinel, unconditionally. It is shared and permanent — | |
| never archived, exactly the condition that made the ``no-plan-bodies`` | |
| target necessary — so an age threshold is the only lifecycle its build | |
| results can have. It is appended rather than discovered because its | |
| results are main-anchored (``get_build_results_dir`` resolves the sentinel | |
| through the main checkout), so a caller pinned to a worktree would | |
| otherwise never see it. | |
| A live plan appears in NEITHER the cleanup nor the ``cleanup-status`` | |
| population: its build results are not merely spared deletion, they are never | |
| counted as reclaimable. | |
| """ | |
| cleanable: list[tuple[str, Path]] = [] | |
| plans_dir = PLAN_BASE_DIR / DIR_PLANS | |
| if plans_dir.is_dir(): | |
| for plan_dir in sorted(plans_dir.iterdir()): | |
| if not plan_dir.is_dir() or plan_dir.name == NO_PLAN_SENTINEL: | |
| continue | |
| if plan_is_live(plan_dir): | |
| continue | |
| cleanable.append((plan_dir.name, get_build_results_dir(plan_dir.name))) | |
| cleanable.append((NO_PLAN_SENTINEL, get_build_results_dir(NO_PLAN_SENTINEL))) | |
| return cleanable | |
| def cleanable_build_results_dirs() -> list[tuple[str, Path]]: | |
| """Return ``(plan_id, build_results_dir)`` for every NON-live results tree. | |
| The plan set is ENUMERATED FROM THE PLAN STORE, never from a hand-written | |
| list, so a newly created plan is covered without an edit here. Each entry's | |
| path comes from :func:`file_ops.get_build_results_dir` — the single owner of | |
| the build-results path — so this cleanup surface cannot drift away from the | |
| resolver the build wrappers actually write through. | |
| Two populations survive the filter: | |
| * A plan directory whose ``status.json`` is absent (see :func:`plan_is_live`) | |
| — a leftover the plan lifecycle no longer tracks. | |
| * The ``NO_PLAN`` sentinel, unconditionally. It is shared and permanent — | |
| never archived, exactly the condition that made the ``no-plan-bodies`` | |
| target necessary — so an age threshold is the only lifecycle its build | |
| results can have. It is appended rather than discovered because its | |
| results are main-anchored (``get_build_results_dir`` resolves the sentinel | |
| through the main checkout), so a caller pinned to a worktree would | |
| otherwise never see it. | |
| A live plan appears in NEITHER the cleanup nor the ``cleanup-status`` | |
| population: its build results are not merely spared deletion, they are never | |
| counted as reclaimable. | |
| """ | |
| cleanable: list[tuple[str, Path]] = [] | |
| plans_dir = PLAN_BASE_DIR / DIR_PLANS | |
| if plans_dir.is_dir(): | |
| for plan_dir in sorted(plans_dir.iterdir()): | |
| if not plan_dir.is_dir() or plan_dir.name == NO_PLAN_SENTINEL: | |
| continue | |
| if plan_is_live(plan_dir): | |
| continue | |
| try: | |
| cleanable.append((plan_dir.name, get_build_results_dir(plan_dir.name))) | |
| except ValueError: | |
| continue | |
| cleanable.append((NO_PLAN_SENTINEL, get_build_results_dir(NO_PLAN_SENTINEL))) | |
| return cleanable |
| | `build-results` | `build_results_days` | Aged build-result files under a NON-live plan's `build-results/` tree, and under the `NO_PLAN` sentinel's | | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Complete the build-results target description.
The table row ends with “under the NO_PLAN sentinel's” and does not identify the path or item being cleaned. Document the exact target, such as .plan/local/plans/NO_PLAN/build-results/, and state whether the cleanup removes files, directories, or both.
Proposed documentation fix
-| `build-results` | `build_results_days` | Aged build-result files under a NON-live plan's `build-results/` tree, and under the `NO_PLAN` sentinel's |
+| `build-results` | `build_results_days` | Aged build-result files under a NON-live plan's `build-results/` tree and under `.plan/local/plans/NO_PLAN/build-results/`. || **Output (TOON)** — a flat per-target count/bytes pair, with `status: dry_run` instead of `success` under `--dry-run`: | ||
| ```toon | ||
| status: success | ||
| operation: cleanup | ||
| dry_run: false | ||
|
|
||
| deleted[4]{category,count,size_bytes}: | ||
| logs 3 512 | ||
| archived_plans 2 8192 | ||
| temp 5 1024 | ||
| no_plan_bodies 2 4096 | ||
| target: all | ||
| temp_files: 5 | ||
| temp_bytes: 1024 | ||
| logs_deleted: 3 | ||
| logs_bytes: 512 | ||
| archived_plans_deleted: 2 | ||
| archived_plans_bytes: 8192 | ||
| no_plan_bodies_deleted: 2 | ||
| no_plan_bodies_bytes: 4096 | ||
| build_results_deleted: 4 | ||
| build_results_bytes: 16384 | ||
| total_bytes_freed: 29824 | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the arithmetic in the example total_bytes_freed.
The listed byte fields sum to 1024 + 512 + 8192 + 4096 + 16384 = 30208, but the example shows total_bytes_freed: 29824. Correct the example so the total matches the sum of its components.
📝 Proposed fix for the example total
-total_bytes_freed: 29824
+total_bytes_freed: 30208📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| **Output (TOON)** — a flat per-target count/bytes pair, with `status: dry_run` instead of `success` under `--dry-run`: | |
| ```toon | |
| status: success | |
| operation: cleanup | |
| dry_run: false | |
| deleted[4]{category,count,size_bytes}: | |
| logs 3 512 | |
| archived_plans 2 8192 | |
| temp 5 1024 | |
| no_plan_bodies 2 4096 | |
| target: all | |
| temp_files: 5 | |
| temp_bytes: 1024 | |
| logs_deleted: 3 | |
| logs_bytes: 512 | |
| archived_plans_deleted: 2 | |
| archived_plans_bytes: 8192 | |
| no_plan_bodies_deleted: 2 | |
| no_plan_bodies_bytes: 4096 | |
| build_results_deleted: 4 | |
| build_results_bytes: 16384 | |
| total_bytes_freed: 29824 | |
| ``` | |
| **Output (TOON)** — a flat per-target count/bytes pair, with `status: dry_run` instead of `success` under `--dry-run`: |
| plan_id: The submitting plan id — the ``NO_PLAN`` sentinel for a | ||
| plan-less build (the client resolves it before constructing the | ||
| spec, so the wire value is never the empty string). |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate the plan_id wire invariant in JobSpec.from_dict.
The updated contract says that plan_id is never empty. from_dict still accepts "" and converts non-string values such as None to strings. Reject non-string or empty values before constructing JobSpec.
Proposed fix
command = data['command']
if not isinstance(command, list) or not all(
isinstance(token, str) for token in command
):
raise ValueError('job spec command must be a list of strings')
+ plan_id = data['plan_id']
+ if not isinstance(plan_id, str) or not plan_id:
+ raise ValueError('job spec plan_id must be a non-empty string')
return cls(
command=list(command),
exec_path=str(data['exec_path']),
project_path=str(data['project_path']),
- plan_id=str(data['plan_id']),
+ plan_id=plan_id,
fingerprint=str(data.get('fingerprint', '')),
)As per path instructions, a stated validation guarantee needs an implementation mechanism.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| plan_id: The submitting plan id — the ``NO_PLAN`` sentinel for a | |
| plan-less build (the client resolves it before constructing the | |
| spec, so the wire value is never the empty string). | |
| command = data['command'] | |
| if not isinstance(command, list) or not all( | |
| isinstance(token, str) for token in command | |
| ): | |
| raise ValueError('job spec command must be a list of strings') | |
| plan_id = data['plan_id'] | |
| if not isinstance(plan_id, str) or not plan_id: | |
| raise ValueError('job spec plan_id must be a non-empty string') | |
| return cls( | |
| command=list(command), | |
| exec_path=str(data['exec_path']), | |
| project_path=str(data['project_path']), | |
| plan_id=plan_id, | |
| fingerprint=str(data.get('fingerprint', '')), | |
| ) |
Source: Path instructions
| **There is ONE sanctioned main-anchored resolver utility, used by a bounded set of six cross-session consumers.** `resolve_main_anchored_path` in `script-shared/marketplace_paths.py` is the single deliberate exception to cwd-relative resolution: it resolves a subpath under the MAIN checkout's `.plan/local` regardless of caller cwd (test override first, then `git rev-parse --git-common-dir`). It is consumed by exactly six genuinely-shared cross-session corpora, each reached through that one utility: | ||
|
|
||
| - `merge.lock` — the cooperative advisory lock acquired by `integrate_into_main.py` before move-back/merge (`merge_lock.py`); | ||
| - `run-configuration.json` — the adaptive-timeout corpus (`manage-run-config/run_config.py`); | ||
| - `lessons-learned` — the global lessons corpus, including the id-allocation `plans` scan (`manage-lessons/manage-lessons.py`); | ||
| - `merge-queue.json` — the cross-worktree merge-admission queue (`manage-locks`); | ||
| - `orchestrator` — the epic-orchestration store (`tools-file-ops/file_ops.py` `get_store_dir('orchestrator', entry_id)`), cross-plan state that outlives any single plan's worktree. | ||
| - `orchestrator` — the epic-orchestration store (`tools-file-ops/file_ops.py` `get_store_dir('orchestrator', entry_id)`), cross-plan state that outlives any single plan's worktree; | ||
| - `plans/NO_PLAN/build-results` — the build results of plan-less builds (`tools-file-ops/file_ops.py` `get_build_results_dir`). Only the `NO_PLAN` sentinel's build results are main-anchored; a real plan's `build-results/` sits inside its plan directory and therefore moves with it. | ||
|
|
||
| The utility is the ONE mechanism; the consuming scripts CALL it rather than each carrying its own git-common-dir copy. Every other **per-repo** resolution is cwd-relative. New **per-repo** cross-session shared state MUST route through this utility, not re-implement git-common-dir resolution. The worktree's `.plan/local` is a fully REAL directory with NO symlinks — cross-session visibility comes from the utility, not from a filesystem symlink. (Machine-**wide**, cross-repo state belongs instead to the machine-global home-root tier — see below.) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make the main-anchored consumer boundary executable.
The same root cause appears at all listed sites. They claim that resolve_main_anchored_path has a bounded six-consumer set, but the supplied resolver accepts arbitrary subpaths. Add the authoritative allowlist or dedicated wrappers in marketplace/bundles/plan-marshall/skills/script-shared/scripts/marketplace_paths.py, then make the documentation reference that source.
marketplace/bundles/plan-marshall/skills/tools-script-executor/standards/cwd-policy.md#L25-L34: enforce and source the six-consumer list.marketplace/bundles/plan-marshall/skills/manage-locks/SKILL.md#L130-L131: reference the authoritative exception set.marketplace/bundles/plan-marshall/skills/manage-locks/scripts/merge_lock.py#L138-L139: reference the authoritative exception set.marketplace/bundles/plan-marshall/skills/tools-script-executor/standards/cwd-policy.md#L52-L52: keep the shared-state list derived from the same source.marketplace/bundles/plan-marshall/skills/tools-script-executor/standards/cwd-policy.md#L75-L79: keep the assertion tied to the executable boundary.
As per path instructions, a stated boundary must have an executable mechanism.
📍 Affects 3 files
marketplace/bundles/plan-marshall/skills/tools-script-executor/standards/cwd-policy.md#L25-L34(this comment)marketplace/bundles/plan-marshall/skills/manage-locks/SKILL.md#L130-L131marketplace/bundles/plan-marshall/skills/manage-locks/scripts/merge_lock.py#L138-L139marketplace/bundles/plan-marshall/skills/tools-script-executor/standards/cwd-policy.md#L52-L52marketplace/bundles/plan-marshall/skills/tools-script-executor/standards/cwd-policy.md#L75-L79
Source: Path instructions
| def test_ledger_core_constructors_declare_plan_id_as_required_str() -> None: | ||
| """``build_record`` / ``job_record`` no longer accept ``str | None``. | ||
|
|
||
| The clean break is asserted on the SIGNATURE rather than on behaviour: the | ||
| ``| None`` union is removed outright (not deprecated), which is what makes a | ||
| caller that still passes ``None`` a type error rather than a silently-null | ||
| row. Behaviour-only assertions cannot see this, because Python would happily | ||
| store the ``None`` either way. | ||
| """ | ||
| import inspect | ||
|
|
||
| import _ledger_core | ||
|
|
||
| for constructor in (_ledger_core.build_record, _ledger_core.job_record): | ||
| annotation = inspect.signature(constructor).parameters['plan_id'].annotation | ||
| assert annotation == 'str', ( | ||
| f'{constructor.__name__} declares plan_id as {annotation!r}; the ' | ||
| 'str | None union must be removed, not deprecated.' | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a '_ledger_core.py' | xargs -I{} head -n 10 {}
fd -a 'pyproject.toml' -d 2 | xargs -I{} rg -n 'requires-python|python_requires' {}Repository: cuioss/plan-marshall
Length of output: 654
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import inspect
class C:
def f(self, plan_id: str = "x") -> None: ...
sig = inspect.signature(C.f)
param = sig.parameters["plan_id"]
print("runtime:", repr(param.annotation))
print("callable inspect signature:", callable(param.annotation))
print("str annotation equality:", param.annotation == "str")
print("type annotation equality:", param.annotation == str)
print("annotation is class str:", param.annotation is str)
PYRepository: cuioss/plan-marshall
Length of output: 306
Check _ledger_core’s actual annotation form at runtime.
_ledger_core.py has no future annotations import, and this project targets Python >=3.12; inspect.signature().parameters['plan_id'].annotation returns <class 'str'>, so the annotation == 'str' assertion will fail on Python 3.12+. Use annotation is str or a comparison that accounts for the actual annotation type.
| ) | ||
| return calls | ||
|
|
||
| @pytest.mark.parametrize('plan_id', [None, '', 'NO_PLAN']) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Import NO_PLAN_SENTINEL instead of hardcoding 'NO_PLAN'.
Every other test file in this PR that exercises the sentinel (test_build_result.py, test_build_queue_slot.py, test_manage_change_ledger.py, test_pyproject_execute.py) imports NO_PLAN_SENTINEL from marketplace_paths and parametrizes on the constant. Lines 297 and 309 hardcode the literal 'NO_PLAN' instead. If the sentinel value ever changes, these two tests silently stop testing the sentinel case: the guard treats an unrecognized string as a real plan id, so the suppression assertions (spy['store'] == [], spy['reconcile'] == []) would then verify the wrong branch of the guard without failing.
Import the constant and use it here to keep the tests aligned with the authoritative sentinel definition.
As per path instructions, "Treat a hardcoded list that must mirror a set defined elsewhere ... as a defect unless it is derived from that source at build or run time. Name the authoritative definition and the drift it permits."
🔧 Proposed fix
+from marketplace_paths import NO_PLAN_SENTINEL
+
class TestCmdRunCommonPlanIdGuards:
...
- `@pytest.mark.parametrize`('plan_id', [None, '', 'NO_PLAN'])
+ `@pytest.mark.parametrize`('plan_id', [None, '', NO_PLAN_SENTINEL])
def test_sentinel_suppresses_producer_side_finding_storage(self, spy, plan_id, capsys):
...
- `@pytest.mark.parametrize`('plan_id', [None, '', 'NO_PLAN'])
+ `@pytest.mark.parametrize`('plan_id', [None, '', NO_PLAN_SENTINEL])
def test_sentinel_suppresses_green_build_reconciliation(self, spy, plan_id, capsys):Also applies to: 309-309
Source: Path instructions
1 participant
Summary
Three chained changes to the build-class dispatch boundary:
plan_idbecomes mandatory across all 27 build-class subcommand registrations (9 distinct subcommands across the four wrappers), using the existing truthyNO_PLANsentinel. A build invoked outside any plan now states that fact instead of leaving the field absent — thekind=buildledger row'splan_idgoes fromstr | nullto never-null..plan/temp/build-output/tree into{plan_dir}/build-results/{scope}/, making every build log a durable plan artifact that moves with its plan.get_build_results_dirinmarketplace/bundles/plan-marshall/skills/tools-file-ops/scripts/file_ops.pyis the single owner of that path; theNO_PLANsentinel is the one main-anchored branch, because a plan-less build belongs to no worktree.kind=buildledger row gains three fields — the actual resolvedcommandthe wrapper ran,duration_seconds, andoutcome(the wrapper's whole stdout TOON).argsis unchanged and stays a distinct layer:argsis the executor argv,commandis what the wrapper resolved that argv into.Plus the two things the relocation made necessary: a retention lifecycle (
build_results_days,cleanup --target build-results) with a live-plan guard, and a generic*.templateclassification rule at themanage-execution-manifestaggregator.duration_secondswas already produced by every build result and silently discarded at the ledger boundary — item 3 is plumbing, not new instrumentation.Changes
Mandatory plan id (
NO_PLANsentinel)script-shared/scripts/build/_build_cli.pydeclares the--plan-id/--project-dirrouting pair on every build subcommand, so the surface is uniform rather than per-wrapper.script-shared/scripts/marketplace_paths.pygainsnames_real_plan(), aTypeGuard[str]next to the sentinel it interprets. The sentinel is truthy, soif plan_id:is a vacuous guard against it; every affected site was de-vacuumed.tools-script-executor/templates/execute-script.py.templatekeeps two deliberately distinct values:_active_plan_id(caller-supplied, selects a plan directory for the script log) and_ledger_plan_id(falls back to the sentinel, stamps the row). Collapsing them would redirect a plan-less build's script log out of the global path.Relocation + retention
tools-file-ops/scripts/file_ops.py/constants.py:get_build_results_dir, plusCLEANUP_TARGET_BUILD_RESULTSas a member of the closedCLEANUP_TARGETStuple so the dispatcher and the argparse choices come from one edit.manage-run-config/scripts/_cmd_cleanup.py: the cleaner and itscleanup-statuscounterpart. The plan set is enumerated from the store, and a live plan (one whose directory carries astatus.jsonmarker) appears in neither population — never deleted and never counted as reclaimable.manage-config/scripts/_config_defaults.py:build_results_daysis derived from the same declared value asarchived_plans_daysrather than restating its literal, and_cmd_cleanupbackfills an absent value from the project's effective setting, so a project that lengthens plan archival lengthens build-result retention in lock-step.Ledger
manage-change-ledger/scripts/_ledger_core.py:build_recordgainscommand/duration_seconds/outcome.Manifest
manage-execution-manifest/scripts/_manifest_core.pyrecognizes*.templatefiles at the classifier aggregator — a generic rule, not a special case for the executor template.Docs reconciled to what shipped:
extension-api/standards/build-execution.md,build-systems-common.md,build-api-reference.md,doc/adr/002-Plan-scoped_operations_move_into_a_cwd-pinned_hermetic_worktree.adoc,tools-script-executor/standards/cwd-policy.md, and themanage-config/manage-run-config/marshall-stewardsurfaces for the new knob.Worth a reviewer's attention
Three sweeps each found materially more than the outline claimed — and one of them had already been marked COMPLETE and was still a floor:
cwd-policy.mdcarried the same split — its primary definitional statement said five with a five-item list while a later assertion in the same document already said six. A reader consults the former first.add_project_dir_arg's docstring named two wrapper-local registrations; there are three. The omitted one ispyproject_build.py::_register_resolve_test_scope— precisely the site whose vacuousplan_idguard this change fixed.test/_shared/_build_class_roster.pycoversbuild_main-routed wrappers, while the executor treats every script under abuild-*skill as build-class (js_coverage.pydeclares its own parser and sits outside the roster).A live producer/consumer defect was found and fixed in
build-server-client/scripts/build_server.py._record_jobwroteplan_id or Nonewhile_latest_job_id_for_planmatchedplan_id or Noneverbatim. Resolving only the write half to the sentinel would have made every plan-less build's re-attach row unfindable — a silently lost running build, not an error. Both halves now store and match the sentinel verbatim.A security pass added a path-containment guard.
get_build_results_dirderives a directory fromplan_idand now creates it and writes into it, but the chainget_build_results_dir→get_plan_dir→get_store_dir('plans', ...)performed no path-safety check — the asymmetry was visible insideget_store_diritself, whose orchestrator branch calls_reject_unsafe_entry_idwhile its plans branch did not. Not exploitable today, but only becausebuild_mainroutes real plan ids through amanage-statusworktree lookup that happens to reject traversal. That is incidental, not deliberate: cache the worktree or short-circuit the lookup and the escape reopens silently. Containment-by-resolution is now applied at the documented single owner.A simplify pass collapsed 8 hand-written copies of the
plan_id and plan_id != NO_PLAN_SENTINELguard (in two polarities, each with its own re-explanation comment) intonames_real_plan(). TheTypeGuard[str]return is load-bearing rather than decorative — the inline expression narrowedstr | Nonetostrfor free, and a plainboolhelper broke that in six places.Explicitly NOT fixed by this PR
The false-fresh hole is out of scope and remains open. A zero-exit dispatch of a non-building build-class subcommand (
discover,run-config-key) still stamps akind=buildrow withstatus: successagainst the currentworktree_sha, because_is_build_class_notationgates on the notation, not on whether a build ran. It is handed to PLAN-TRUTH-010, named in this plan's request as a serialization pair (same dispatch boundary, samekind=buildrow).This change makes that hole read worse, not better: after these commits the bogus row carries a real
plan_idinstead ofnull, so it presents as that plan's successful build. Please do not read the never-nullplan_idas evidence the row is trustworthy.Also out of scope by design: no second or parallel build ledger (the existing
kind=buildentry was extended), and no deprecation shims or transitionary code (compatibility=breaking/simplicity=leanproject defaults).Test Plan
python3 .plan/execute-script.py plan-marshall:build-pyproject:pyproject_build run --command-args "verify")NO_PLAN), the three new ledger fields, and a non-empty derived roster--target allinclusion, and the effective-value backfill (fixtures setarchived_plans_days: 30, far from the shipped default, so a reader that fell back to the default fails rather than passing by coincidence)--targethelp againstCLEANUP_TARGETS, so re-hardcoding the enum fails loudlycleanup --target build-results --dry-runreports zero reclaimable bytes andcleanup-statusreportsbuild_results_total: 0Related Issues
Implements orchestrator spec PLAN-TRUTH-026 (
truthful-signalsepic). Serialization pair with PLAN-TRUTH-010 — see § Explicitly NOT fixed above.Generated by plan-finalize skill
Intent
The problem. A build's
plan_idwas nullable, so a caller that forgot to pass one was indistinguishable from a genuinely plan-less build — an absence that could mean either. Build output landed in a shared.plan/temp/build-output/tree, unattributable to the plan that caused it. And thekind=buildledger row recorded the executor argv but not the command the wrapper actually resolved and ran, nor the duration the wrapper already measured and then discarded at the ledger boundary.The chosen approach. Make "no plan" a stated value rather than an absence, using the existing truthy
NO_PLANsentinel, so the ledger field can be never-null and every row is attributable. Then relocate results under{plan_dir}/build-results/so the log is a plan artifact that moves with its plan, with the sentinel as the single main-anchored branch (a plan-less build belongs to no worktree). Then extend the existingkind=buildrow — not a second ledger — with the resolved command, the already-measured duration, and the wrapper's outcome TOON, keepingargs(executor argv) andcommand(what the wrapper ran) as two distinct layers. The three are ordered by dependency: results cannot be plan-scoped until a plan id is guaranteed, and the ledger cannot point at a plan-scoped result until the result has moved.Explicit non-goals. No second or parallel build ledger. No
[Intent truncated — 1389 of 1858 characters shown; full outline in the plan workspace]
Summary by CodeRabbit
New Features
build-resultslocations, with support for plan-less builds.Documentation