C6 — Repo-correct PR risk, warm review modules, and the daemon fork guard (PLN-2027 PR 6 + FR11) - #21
Merged
Merged
Conversation
FR12: the blast factor and affected tests now come from open_file_graph(repo_root).blast_radius(), which reads the repository's own imports table, instead of SemanticFileMemoryCapability.change_impact, which reads a machine-wide index spanning every repository summarize_file has touched. A sibling checkout importing this one's module counted as an importer, and one of its tests could clear this file's test gap. open_file_graph fails loud on an absent or mid-rebuild index (PR 1). pr_risk catches that and degrades the blast factor rather than propagating it: its contract is fail-open per delegated factor. The per-file result gains `objective` and `truncated` beside the existing keys, additively. This does NOT make pr_risk read-only, and it stays refused through the broker. The plan's premise for this change is false: _file_complexity reaches summarize_file, and pr_risk summarizes the changed file outright, so both paths still write the machine-wide semantic index. Removing those writes is not in PR 6's change list. FR13: _PRO_CODE_PATH_MODULES gains the five review modules the review_* tools import on a request thread, so a release build finishes building that mypyc group before serving instead of racing it.
… fork build
Operator-directed added scope, not part of any PLN-2027 unit: the FR11 gap PR 5
left open. The background auto-updater carried its own release path around the
guard PR 5 shipped -- a second "lemoncrow-lab/lemoncrow" literal under a
"keep in lockstep with update.py" comment that PR 5 made false, and an
_update_via_release() that downloaded upstream's install.sh and ran it detached
with no fork check.
The repo now comes from lemoncrow._distribution, with the same
ModuleNotFoundError fallback update.py uses: the module's absence is the
upstream build, because the public mirror denies the file. The release arm
refuses on a fork build and logs the fork update command.
Refusal is unconditional and adds no new environment variable. FR11 asks that
no update path replace a fork build with an upstream release unless the
operator is told and confirms; a daemon has nobody to prompt, so refusing and
naming `lc update`'s alternative is the only behaviour that satisfies it.
LEMONCROW_AUTO_UPDATE_RELEASE is not consent: it says "auto-update from
releases", not "replace my fork with upstream".
Known, deliberately not fixed here: scripts/local.sh writes .dev_mode to
${HOME}/.lemoncrow unconditionally while _is_dev_install reads it under the
active root, so a non-default --root install has a dev gate that fails open.
With the fork check in place .dev_mode is no longer the FR11 defence, and
changing what the installer writes is a separate blast radius.
…rve (PLN-2027 PR 6)
…ves its root (PLN-2027 PR 6) Review feedback on PR 6. FR13: adding the five review modules to _PRO_CODE_PATH_MODULES only moves the build to start-up for whoever calls _warm_pro_code_modules(). The review_* tools never did -- they import lemoncrow.pro.capabilities.review.* straight from the request thread. The stdio path is safe because serve() warms before the reader starts; the HTTP daemon is not, because it starts serving while _warm_daemon's thread is still inside the same mypyc group. Listing the modules without warming at the entry points gave that request thread a concurrent importer it did not have before, which is the race rather than the fix. All three handlers now warm before their lazy import, the way _code_context_engine already did. FR12: _op_graph_code_health resolved a relative repo_root against the process cwd while every sibling file-graph kind goes through _code_repo_root, which takes it against the active workspace. That did not show while the blast radius came from the machine-wide index keyed by absolute path; now that pr_risk opens <repo_root>/.lemoncrow/workspace, a relative root against a daemon standing elsewhere finds no index and silently degrades every file.
…arn (PLN-2027 PR 6) Review feedback on PR 6. _blast_radius_for copied FileGraph.blast_radius's envelope-level `objective` and dropped its per-file `indexed` flag. blast_radius answers for a path it has never seen -- zero importers, zero tests -- and the envelope's objective is computed from whether the imports TABLE has rows, not from whether THIS file was examined. A changed file absent from the index therefore came back as impacted_files 0, missing_tests true, objective "exhaustive", truncated false, with no reason: indistinguishable from a file that genuinely has no importers and no tests. A PR that adds a file is exactly pr_risk's workload, so that was the common case, and it reintroduced at file granularity the failure the completeness contract exists to prevent. `indexed` now decides the objective per file. No data is also not a known test gap. The degraded shape asserted missing_tests: true for every file, adding 0.20 to every score while reporting risk_level "low" -- a penalty charged for ignorance, and a verdict no closure was walked to reach. Both are now None / "unknown", so an unknown gap costs nothing, the way churn and complexity already degrade. IndexRebuilding and CodeIntelUnavailable are caught separately. They are deliberately not in a subclass relationship because they are different conditions -- one answers the same call properly in a moment, the other needs an index built -- and a single reason string put that back out of reach. The redundant cap.summarize_file(abs_path) pre-seed is gone: its consumer (change_impact) left with FR12, _file_complexity already does the same write under a guard, and this was the one unguarded index write, so its failure aborted the whole report instead of degrading one factor. pr_risk still writes through _file_complexity and stays refused through the read-only broker. Not changed: _blast_factor's curve. The docstring PR 6 corrected describes what ships; whether the saturating shape the original prose asked for is what is wanted is an open question for the operator, since it moves every score across the low/medium/high tier boundaries.
…ribution Review feedback on the FR11 added scope. The servicectl commit removed the duplicated repo literal but left the two pieces derived from it as independent copies: _is_fork_build() was byte-identical to update.py's, and the remediation hint was hand-typed inline instead of built by update.py's _fork_update_command(). Rename the install script and only `lc update` picks it up -- the daemon keeps logging the stale command, and the drift test caught none of it because it pinned the constants only. is_fork_build() and fork_update_command() move next to the constants they read, in lemoncrow._distribution, and both modules import them. servicectl does NOT import update.py: that would drag click and the whole CLI import graph into a long-running daemon and point runtime infrastructure at the CLI layer. Each importer keeps its own ModuleNotFoundError fallback, where the module's absence is the upstream build -- the predicate is False and no hint is ever needed. The drift test now pins the predicate and the hint by identity, which only one shared implementation satisfies, and rebinding DISTRIBUTION_REPO has to move the hint in both modules. The fork guard's ORDERING is pinned too. It sits above the LEMONCROW_AUTO_UPDATE_RELEASE opt-in, and the existing test set that flag, so it passed whichever side of the check the guard sat on. Below the opt-in, clearing the flag turns the refusal into the opt-out's quiet return, and setting it makes an "auto-update from releases" flag read as consent to replace the fork with upstream. The refusal is now asserted at every value of the flag, and asserted to be the fork refusal rather than a no-op.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
C6 — Repo-correct PR risk, warm review modules, and the daemon's fork guard
Lands PLN-2027 PR 6 (FR12, FR13) plus one operator-directed fix that belongs to no plan unit: closing the FR11 gap the previous chunk carried forward. Base
origin/main; not stacked. The two are in separate commits.What it does
PR 6. PR risk takes blast radius and affected tests from the repository’s own import graph instead of a machine-wide index spanning other checkouts, and five review modules join the start-up warm list.
Operator-directed (FR11). The background auto-updater refuses to install an upstream release over a fork build, logging the command that updates the fork instead. The refusal is checked before the auto-update opt-in, so that flag cannot read as consent, and no new flag or environment variable was added — a daemon has nobody to prompt, so refusing is the only behaviour that satisfies "the operator is told and confirms".
Acceptance ledger
18 criteria: 17 met, 1 met differently, 0 unmet.
The one met differently is the honest one: this PR does not make
pr_riskread-only, and it stays refused through the read-only broker. PR 6’s own text claims complexity "reads the file’s own content, not other repositories". That is false:_file_complexitycallscap.get_cached(...) or cap.summarize_file(...), and both write the machine-wide index. Moving blast radius does not change that. An unresolved plan comment said so and was never folded in.broker_policy.pyis untouched by this diff.Review
Deep review: NEEDS_ATTENTION, 7 verified findings, 0 rejected, 0 pending. All 7 fixed, 0 pushed back, 1 declined. Re-review: APPROVED, no blocking or high findings.
The two HIGHs were both real:
impacted_files: 0,missing_tests: true,objective: "exhaustive"with no reason — indistinguishable from a file that genuinely has no importers and no tests. New files in a PR are exactly this tool’s workload, so it was the common case, and it reintroduced at file granularity the silent-zero failure the completeness contract exists to prevent. Fixed: the index’sindexedflag is forwarded, and an unindexed file reportspartialwith a reason. An indexed sibling in the same call still answers exhaustively.The five MEDIUMs were all fixed: the MCP seam now resolves a relative root workspace-relatively like its sibling kinds; a rebuilding index is distinguished from an absent one in the reason; a no-data result no longer scores as a test gap (it was adding a penalty for ignorance while reporting "low"); and one redundant unguarded index write was removed — its failure aborted the entire report instead of degrading one factor.
FR13’s race is closed, not deferred. The three review-tool handlers imported the review group without taking the import lock while warming ran on background threads under a serving daemon. It turned out to be the existing lock taken in three places, so the criterion this chunk claims is genuinely met rather than met on paper.
Declined (1):
_blast_factor’s curve. Its docstring claimed a saturating shape (1 importer → 0.2, 3 → ~0.5) while the body has beenmin(1.0, impact_total / 10.0)— linear — since the function’s first commit, so the prose never described shipped behaviour. The docstring is corrected here; the formula is deliberately not touched, because changing it moves every score across the low/medium/high tier boundaries. The reviewer judged the saturating curve was the real intent, and that is a product call, recorded as an open question rather than closed quietly by a doc edit.One fix made after the re-review gate
The re-review approved but confirmed a MEDIUM one level up from the first HIGH: per-file honesty had landed, the envelope had not.
pr_riskover a single brand-new file still headlinedoverall_tier: "low"with noobjective— a verdict over files it had just admitted, row by row, that it could not read. The earlier fix made that quieter, because dropping the unearned test-gap penalty lowered the very score the headline is computed from.Fixed in the final commit: the envelope carries
objective, andoverall_tieris"unknown"when every scored file is partial. Pinned by two tests, each verified to fail under a targeted mutation.Verification
tests/core/test_code_health_pr_risk.py(14) ·tests/gateway/test_completeness_contract.py,test_pro_import_warm.py,tests/infra/test_servicectl_autoupdate.py(97 together) ·test_update_command.py(14) ·test_cap_tools_list_gate.py(51) ·test_mypyc_compile_safety.py(3) ·test_mirror_denylist.py(9) ·mypy --strictclean on every changed source file ·ruff check --no-cache src benchmarks tests scripts integrationsclean ·black --check src testsclean. The repo-wide lint was re-run after the final commit on a cold cache.Beyond the suite, three invariants were probed directly rather than through the tests that cover them: disarming the fork guard fails exactly one test and no other; with the fork-only module absent, both the CLI and the daemon import cleanly and report an upstream build; and the mirror’s own
is_public()still denies exactly that one file while both importers stay public.The plan’s §0 full-suite-in-a-scratch-worktree row is left to CI, which runs that lane sharded.
Deviations
pr_riskstays broker-refused (recorded in the commit body, the module docstring and the plan thread).What is still owed
_blast_factor’s curve shape is an open product question. Shipped: linear to a cap at 10, so one importer contributes 0.10. The prose asked for 0.20. Changing it shifts every tier.graph kind="pr_risk"yet.Artifacts