Repository navigation
fix(phase-validation): 'UI/UX 100%' measured only that files exist (#17089) - #17505
Conversation
…17089) "Phase 6: Enhanced UI/UX -- 100%" meant `App.vue` and `package.json` exist. TWO DEFECTS PRODUCED ONE NUMBER. 1. THE DENOMINATOR SHRANK WITH THE SKIPS. #7496 skipped the endpoint, service and feature checks under `--ci-mode` for a sound reason -- the workflow never brings a stack up -- but the phase figure is `passed / total`, and skipping removed those checks from `total` as well. Phase 6's two surviving checks were file-existence tests, so 2/2 read as 100% complete. The less a run could measure, the higher it scored. Its own comment says the skip was to stop them "drag[ging] the score below the threshold", which is the inversion stated outright: not-looked-at became nothing-missing. 2. 14 OF 15 FEATURE-CHECK PATHS DID NOT EXIST, so even with a live stack those checks could only report False. #7496's comment claims the paths were refreshed for the current layout; it refreshed the criteria dict and left the lambdas 700 lines below pointing at `src/`, `backend/` and `autobot-vue/`. The comment was true about what it sat above and false about the file. Nothing failed, because the same change began skipping those checks in CI. WHAT CHANGED `phase_score.py` is new and holds the policy: a phase with even one skipped group cannot be called complete at any ratio, and its figure is named `structural_presence_percentage`. `completion_percentage` is ABSENT from the report when anything was skipped -- not zero, not null -- so a consumer reaching for it gets a KeyError instead of a number it would have believed. `overall_maturity` is `None` in CI mode and `ready_for_production` is `None` rather than False, because False reads as "assessed and failed". A separate module because the policy must be testable, and `phase_validation_system` imports aiohttp, psutil, requests and `autobot_shared.redis_client` at module scope. That last one has side effects -- it writes secret key material when none is configured -- so a guard that imported it would mutate the tree it guards. The tests read the system as data with `ast` and load only the policy. A policy nothing can exercise is how this one survived. The 15 paths are corrected to their real homes (`llm_interface_pkg` -> `llm_shared`, #6941; `autobot-vue/` -> `autobot-frontend/`; `src/` and `scripts/` -> their current packages). The gate keeps its 60/80 thresholds and the same number, now read from `structural_presence` and labelled as such -- `.get("overall_maturity", 0)` would have returned None and broken `bc`. Phase 6 now names the workflows that actually test the UI (frontend-test, visual-regression, stylelint-tokens, frontend-typecheck-regression) rather than claiming a UI verdict from file presence. A GUARD ADDED BECAUSE THIS PR BROKE IT ITSELF. Removing the superseded `_determine_phase_status` with a slice bounded by the next ` def ` swallowed the ` async def` that followed, deleting `_validate_phase_features` while its call site stayed. `py_compile` passed -- Python resolves attribute names at call time, and in `--ci-mode` that line sits in the branch CI does not take. The method is restored verbatim from `origin/main`, and `test_no_self_call_points_at_a_missing_method` now fails on exactly that deletion; mutation-proved by redoing it. repo_tests/phase_validation_paths_and_skips_17089_test.py 17 passed criteria paths referenced / missing 39 / 0 feature-check paths referenced / missing 15 / 0 methods defined (24 on main, 1 intentionally removed) 23 Closes #17089
…17089) Consumer of the report shape changed in `bbc2c9a28d`, and where that defect had consequences rather than just a misleading number. `phase_progression_manager` calls `validate_all_phases()` and decided a phase was completed with: if phase_data["completion_percentage"] >= self.config["validation_threshold"] Under `--ci-mode` only the file/directory checks run, so that percentage was the share of EXISTENCE checks that passed. A phase whose declared files happened to exist scored 100 and was marked completed -- and this module PROGRESSES phases from that set, so the consequence was automatic promotion on "the files are present". Both decision sites now consult `complete`, which the policy sets False whenever any check group was skipped, at any percentage. THE THRESHOLD WAS DUPLICATED AND IS CONSOLIDATED, not dropped. `self.config["validation_threshold"]` was the literal `95.0` in a hard-coded dict with no external merge -- a constant wearing a config key's clothes -- and `phase_score.COMPLETE_AT` is also `95.0`. Two copies of one bar, kept in step by hand. The policy owns it now. That consolidation is what let this land without raising a ceiling: the file was pinned at 851 and my first version took it to 859. Removing the duplicate brought it to 850, so the pin is LOWERED to 850 in both baseline copies, in this commit, as the shrink-only ratchet requires. THREE THINGS THIS PR'S OWN GUARDS CAUGHT, all mine: - Extracting `_SHARED_SCRIPTS` to fix three over-long lines split those paths into a Name plus a Constant, and the lambda-path scanner -- which collected string literals -- stopped seeing them. Its floor failed immediately. The scanner now resolves module-level string constants and flattens the `root / a / b` chain, reading the path the code builds rather than the fragments it is written in. Back to 15 of 15. - That floor is why this was a failure and not a silent loss of coverage: a scanner quietly finding 4 paths instead of 15 would have reported "no missing paths" and been believed. - The pin comment itself then broke E501 at 130 characters, in the very file that records line-count limits. Guarded against regression: `TestTheProgressionManagerDoesNotPromoteOnPresence` fails if `["completion_percentage"]` returns to this file or either decision stops consulting `complete`; mutation-proved by putting the numeric read back. BOUNDARY, since tracing this grew the target set: `project_state_manager.py` and `project_state_tracking/reports.py` also carry a `completion_percentage` and are NOT affected -- that is their own SQLite-backed dataclass field, reached through the state tracker's `summary`, not through `validate_all_phases()`. Followed the data flow rather than matching the symbol. repo_tests/phase_validation_paths_and_skips_17089_test.py 18 passed flake8 (all touched files) clean check_python_file_size.py clean phase_progression_manager.py 851 -> 850 Refs #17089
… it (#17089) Completes AC3, which I had only half delivered in `bbc2c9a28d`. It asks that the UI/UX phase report the real UI gates' verdicts, "or the phase links to them and doesn't report its own percentage". I added the links and kept publishing a structural figure -- still a percentage next to a phase whose verdict lives elsewhere, so the criterion was not met and I had written it up as though it were. A phase declaring `authoritative_gates` now reports NO figure at all: status: deferred-to-dedicated-gates authoritative_gates: [frontend-test, visual-regression, stylelint-tokens, frontend-typecheck-regression] why_no_score: "this phase is verified by dedicated workflows, not by file presence" Whether a UI is consistent, responsive and internationalised is not a question a file-existence sweep can answer at any percentage. The owner objected to a number, so the fix is to stop producing one here rather than to produce a better-labelled one. AND IT CONTRIBUTES NOTHING TO THE AGGREGATE. Excluding it from its own report while leaving it in the weighted average would make "reports no score" true of the phase and false of the run -- its 100% would still reach the CI gate through the mean. `overall()` now drops deferring phases and DISCLOSES that it did: `phases_excluded_from_score` and `verified_by_dedicated_gates` are in the output, because an average over fewer phases than the run evaluated is exactly the silent denominator change this issue is about. Four consumers needed the absent key handled, one of which would have crashed: `_generate_recommendations` subscripted `structural_presence_percentage` directly, so a deferring phase was a KeyError -- caught by writing the tests, not by running the script, which cannot be run here (it imports `autobot_shared.redis_client`, which writes key material when none is configured). The JSON output emits `None` rather than `0` for a phase with no score: 0 renders as "measured and found empty", the confusion this issue is about. One consequence worth stating plainly: `phase_progression_manager` can now never mark Phase 6 completed, because `complete` is False for a deferring phase. That is correct -- this report cannot verify that phase -- but it means UI progression must come from the dedicated gates rather than from here. repo_tests/phase_validation_paths_and_skips_17089_test.py 22 passed flake8 (3 touched files) clean Refs #17089
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 16 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (9)
📝 WalkthroughWalkthroughPhase validation now separates structural presence from measured completion. It records skipped checks, delegates designated phases to authoritative workflows, and uses the ChangesPhase validation scoring
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🟠 High · up to After this change, the phase-validation CI check fails on every run. The validator's JSON output omits the structural-presence figure that the gate now reads, so the gate treats it as 0%. The PR status comment also displays Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Separating file presence from measured completion prevents skipped checks from advancing phases. However, the new readiness calculation can omit a deferred phase without verifying its named checks, and an integration check still expects the old numeric result. These gaps affect the reliability of release decisions; no direct attacker-access or privilege expansion was established. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 53.19% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 47 functions across 4 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟠 Major · Include the structural score in the JSON output. · phase_validation_system.py:944-949
autobot-infrastructure/shared/scripts/phase_validation_system.py:944-949
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInclude the structural score in the JSON output.
validate_all_phases()addsstructural_presence,measures, andskipped_detailto the result. However,_output_json_results()omits these top-level fields. The workflow parses this JSON and falls back to0whenstructural_presenceis absent. The gate therefore fails every run.Suggested fix
output = { "timestamp": datetime.now().isoformat(), - "overall_maturity": results.get("overall_maturity", 0), + "overall_maturity": results.get("overall_maturity"), + "structural_presence": results.get("structural_presence"), + "measures": results.get("measures"), + "skipped_detail": results.get("skipped_detail", {}), "phases": [],
🟡 Minor · The PR comment still reads keys that were removed. · phase_validation.yml:220-226
.github/workflows/phase_validation.yml:220-226
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe PR comment still reads keys that were removed.
phase.completion_percentageno longer exists in the JSON output.results.overall_maturityisnullin CI mode. The comment therefore showsundefined%for the maturity line and for every phase. The pass/fail mapping also marksstructural-presence-onlyanddeferred-to-dedicated-gatesasFAIL.Read
structural_presence_percentage. For phases that reportauthoritative_gates, list those gates instead of a figure.
🟡 Minor · Handle an unmeasured overall_maturity. · phase_validation_system.py:1041-1047
autobot-infrastructure/shared/scripts/phase_validation_system.py:1041-1047
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle an unmeasured
overall_maturity.When counted phases contain skipped groups,
overall_maturityisNone.main()then evaluatesNone < 50, catches the resultingTypeError, and exits with status 3. Summary mode also passesNoneto%.1f, which causes a logging format error. The CI step ignores this status with|| true, so the workflow step continues, but direct CLI use still reports failure.Suggested fix
- maturity = results.get("overall_maturity", 0) - logger.info("Overall System Maturity: %.1f%%", maturity) + maturity = results.get("overall_maturity") + if maturity is None: + logger.info("Overall System Maturity: not measured") + else: + logger.info("Overall System Maturity: %.1f%%", maturity) ... - maturity = results.get("overall_maturity", 0) + maturity = results.get("overall_maturity") + if maturity is None: + maturity = results.get("structural_presence", 0)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: bbf9d6ba-077d-4f2e-a02f-2cbad9257085
⛔ Files ignored due to path filters (2)
repo_tests/python_file_size_ratchet_baseline.pyis excluded by!repo_tests/python_file_size_ratchet_baseline.pyscripts/python_file_size_known_large.pyis excluded by!scripts/python_file_size_known_large.py
📒 Files selected for processing (5)
.github/workflows/phase_validation.ymlautobot-backend/phase_progression_manager.pyautobot-infrastructure/shared/scripts/phase_score.pyautobot-infrastructure/shared/scripts/phase_validation_system.pyrepo_tests/phase_validation_paths_and_skips_17089_test.py
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
|
Cause
spec = importlib.util.spec_from_file_location("phase_score", repo_root() / _POLICY)
module = importlib.util.module_from_spec(spec)
sys.modules["phase_score"] = module
spec.loader.exec_module(module)
return moduleThe registration itself is correct and its docstring says why — Note what the guard reports: FixThe guard's own instruction applies: install and remove in the same @contextlib.contextmanager
def _policy_module():
spec = importlib.util.spec_from_file_location("phase_score", repo_root() / _POLICY)
module = importlib.util.module_from_spec(spec)
previous = sys.modules.get("phase_score")
sys.modules["phase_score"] = module
try:
spec.loader.exec_module(module)
yield module
finally:
if previous is None:
sys.modules.pop("phase_score", None)
else:
sys.modules["phase_score"] = previousRestoring A session-scoped fixture wrapping that would also avoid the ten repeated path-loads, if the call sites are worth touching in the same pass. Why I am reporting rather than fixing
One thing to check while you are in thereThe leak was attributed to this file but first seen at |
Review finding on #17505, and it is the defect I caught in this same scanner this morning, one layer out. Correct, and worse than reported. THE BLIND SPOT. The lambda-path scanner filtered on `"/" in joined`, which silently dropped every SINGLE-SEGMENT path -- so `root / "src"`, `root / "tests"` and two `root / "scripts"` were never collected. `src` and `tests` are exactly the pre-reorganisation roots this PR exists to remove. The guard reported "15 of 15 paths exist" while not looking at five more, two of them missing. This morning the same scanner saw 4 paths instead of 15 after I extracted a constant, and its FLOOR caught it, because a floor asserts a population. There was no floor over single-segment paths, so this blind spot was silent. A floor cannot supply what was missing here -- a fixture carrying a MISSING single-segment path -- which is exactly the contrast pair the reviewer named. Added the fixture first and watched it fail against the un-widened filter, then widened it, rather than trusting the instrument that had just missed four roots to confirm it now sees them. 15 -> 20 paths collected. AND THE PART THE REVIEW DID NOT REACH, which indicts my own fix. Correcting `autobot-vue/src/components` to `autobot-frontend/src/components` earlier in this PR made the DIRECTORY right and left the CHECK dead: `glob` does not recurse and those components live in subdirectories. So the path guard passed -- it asked only whether the base directory existed -- while four checks could still only ever report False: (scripts).glob('*security*') 0 matches (autobot-frontend/src/components).glob('*Terminal*') 0 <- "fixed" here (autobot-frontend/src/components).glob('*Settings*') 0 <- "fixed" here (scripts).glob('*profile*') 0 EXISTENCE OF A PATH IS NOT EVIDENCE THAT A CHECK CAN SUCCEED, and my guard asserted the weaker property. Repaired all six dead checks, match counts measured: terminal_interface components rglob '*Terminal*' 24 settings_panel components rglob '*Settings*' 29 container_security shared scripts glob '*security*' 2 code_quality_checks shared scripts glob '*profile*' 5 task_planning autobot-backend glob '*orchestrat*' 2 unit_testing autobot-backend/tests exists `TestNoFeatureCheckIsDeadOnArrival` now asserts every glob check matches at least one file, with a positive control on a fixture and a test that `glob` vs `rglob` is recorded -- that distinction is the difference between dead and alive for two of these. Mutation-proved: reverting one `rglob` to `glob` fails the guard naming that line. repo_tests/phase_validation_paths_and_skips_17089_test.py 29 passed lambda paths collected / missing 20 / 0 glob checks / dead 7 / 0 Refs #17089
…shape (#17089) The strict contract from `bbc2c9a28d` -- `completion_percentage` ABSENT when a check group was skipped -- broke every consumer that read it unconditionally. I missed them, and the reason is worth recording: my consumer sweep ended in `head -20`, so I read a truncated list as complete and concluded two files were affected when five were. The command meant to enumerate the set was capped, and I trusted its output as the whole. Two production consumers, 14 live read sites, both invoked by the same workflow this PR already touches: `validation_dashboard_generator.py` -- 5 reads of `overall_assessment["system_maturity_score"]`, renamed here to `structural_presence_score`, plus 9 reads of `phase_data["completion_percentage"]`. It runs as `... > validation_dashboard.html || echo "Dashboard generation failed"`, so the KeyError would not have failed the job: it would have SILENTLY STOPPED PRODUCING A DASHBOARD. The same defect this issue is about, wearing a third hat. `project_state_tracking/tracker.py` -- 5 sites, including a `TrackingMetric.PHASE_COMPLETION` counted by thresholding that percentage, and a snapshot storing it per phase. WHAT THE MIGRATION DOES, rather than defaulting to 0 anywhere: - `phase_figure()` returns `None` when a phase publishes no figure, and callers branch on it. 0 would read as "measured and found empty". - "Completed phases" counts come from the `complete` FLAG in both consumers, not from a threshold on a percentage. That flag is False whenever a group was skipped, so a `--ci-mode` run cannot inflate either count. - Unscored phases sort LAST rather than as zero -- they are not the worst-performing phases, they are the ones the report does not judge -- and are excluded from "stalled" alerts for the same reason. - The tracker's snapshot stores only phases that published a figure: a `None` written there becomes a score, and an unscored phase is not a zero-scoring one. - A deferring phase gets its own recommendation naming the workflows that verify it, instead of appearing as a phase needing attention. ONE DESIGN CORRECTION MADE BY A TEST, which is the right way round. My first version precomputed `percentage_display` and `bar_width` in the caller, and `_generate_phase_html` then demanded keys its own existing test does not pass -- that test calls it directly with just `completion_percentage`. The formatting decision belongs in the renderer, so it derives both locally and the existing assertions pass unchanged. A renderer that requires pre-formatted input cannot be tested in isolation. AND ONE CEILING RESPECTED RATHER THAN RAISED. Inlining the two helpers took `tracker.py` from 571 to 604 lines, past the 600 ceiling. They now live in `project_state_tracking/phase_report.py`, which brings it to 583 -- split, not pinned, and no baseline entry added. Contrast pair for the new render path: a phase with no figure renders `not scored` with a 0-width bar and explicitly NOT `0.0%`; the control asserts a phase with a figure still renders it. validation_dashboard_generator_test.py 5 passed (2 new) tracker_test.py 3 passed phase_validation_paths_and_skips 29 passed check_python_file_size.py clean (tracker 583 < 600) flake8 (5 touched files) clean Refs #17089
The `python-suite shard 8/12` red on #17505. Cause, from the shard log: repo_tests/phase_validation_paths_and_skips_17089_test.py leaves sys.modules key 'phase_score' installed outside repo_tests/ (first seen at tools/lint/check_spdx_header_test.py) Mine, and the guard was right. `_policy()` loads the policy module by path and must register it in `sys.modules` WHILE the body executes -- `dataclasses` resolves `cls.__module__` through it and the class body raises without it -- but I left it there afterwards. A module parked under a bare name can be picked up by anything later in the same process, which is why the file that noticed was an unrelated one in another directory entirely. Registration is now scoped to the exec and removed in a `finally`, and the loaded module is cached so repeated calls neither re-execute it nor re-enter `sys.modules`. WHAT I COULD NOT REPRODUCE, STATED PLAINLY. I could not make the leak guard fire locally, with or without the fix: the same two files that CI failed on pass here and the guard prints no section at all, for a reason I did not identify. So this is not "verified locally" -- the guard's own verdict is CI's to give. What I did verify is the mechanism the guard named, directly: phase_score in sys.modules before _policy(): False after _policy(): False policy usable: True cached (same object on 2nd call): True That is the property the shard complained about, checked without relying on an instrument I could not get to run. The shard log also noted the local environment is below four dependency floors (websockets, starlette, uvicorn, greenlet), which is a separate pre-existing condition and not this failure. repo_tests/phase_validation_paths_and_skips_17089_test.py 29 passed with tools/lint/check_spdx_header_test.py 50 passed Refs #17089
mrveiss
left a comment
mrveiss
left a comment
There was a problem hiding this comment.
Verdict: the red is not yours, and the work is good
python-suite shard 1/12 is a genuine failure (zero cancellations on this PR, so it is not
the serialisation sweep), but it is not caused by this change. It fails 24 seconds before
any test runs, in the dependency-floor gate:
4 of 182 declared versions are NOT satisfied by the interpreter running this check (python 3.14.7).
This IS the CI job's own environment -- these are the packages CI itself installed
opentelemetry-api: installed 1.45.0, declared ==1.44.0 (requirements.txt:51)
opentelemetry-sdk: installed 1.45.0, declared ==1.44.0
opentelemetry-exporter-otlp-proto-grpc: installed 1.45.0, declared ==1.44.0
opentelemetry-proto: installed 1.45.0, declared ==1.44.0
##[error]Process completed with exit code 1.
Timeline in the log: the gate exits at 12:06:17; the first test output is at 12:06:41.
And this PR touches no requirements file — the 11 changed paths are the phase-validation
modules, its test, and the two size-ratchet mirrors.
Why it will keep happening until someone owns it
requirements.txt:51-58 pins opentelemetry-*==1.44.0. The CI plane
(requirements-ci.txt → requirements-ci/*.txt) never declares opentelemetry at all — I
grepped the whole tree for it. So CI resolves it transitively and unconstrained, takes whatever
is newest, and the strict floor check then compares that against the production pin. The drift
appears the moment upstream publishes, on whichever PR next builds a cold cache — which is why
it lands on an unrelated PR rather than on the change that caused it.
That is precisely the two-declaration-planes defect of #17448, and #17502 adds
constraints_differ() plus a constraint-drift baseline that detects this class. This instance
is not in that baseline, so it is worth a line on #17448 either way.
Nothing for you to do here beyond a rerun once the planes agree. Please do not chase it.
Acceptance criteria — all four met
- CI mode no longer shows a completion %.
as_report()omitscompletion_percentage
entirely when anything was skipped, and emitsstructural_presence_percentageplus
not_checkedinstead. Test:test_the_report_omits_completion_percentage_entirely_when_anything_was_skipped. - Feature-check paths exist, with a guard. The
autobot-vue/references survive only in
comments describing the defect.TestEveryReferencedPathExistscovers both the criteria
paths and the lambda paths, andtest_the_parse_found_*guards against the scanner
silently finding nothing — which is the right shape, since an empty parse would make the
other two vacuous. - UI/UX defers to the real gates.
defers_tomakes the phase report no figure and name
the authoritative workflows.test_the_ui_phase_declares_the_workflows_that_verify_it,
andtest_a_deferring_phase_contributes_nothing_to_the_aggregatecloses the hole where
the number would still reach the gate through the average. - A skipped phase cannot read complete.
test_a_phase_with_a_skipped_group_is_not_complete_even_at_100_percent.
What is notably right
Three decisions I would have got wrong:
Withholding the claim instead of capping the number. The docstring's reasoning is correct —
a cap makes 99% mean two things and invites someone to raise it later.
completion_percentage absent rather than zero or null. A consumer reaching for it gets a
KeyError instead of a number it would have believed. That is the same discipline as a floor
guard raising rather than returning an empty set.
ready_for_production: None, not False. "Not assessed" and "assessed and failed" are
different facts and False would have collapsed them — which is the exact inversion this PR
exists to remove, so it is good to see it applied to the aggregate too.
TestEveryMethodItCallsOnItselfExists and TestTheSkipListStaysInStepWithWhatIsSkipped are
both guarding the staleness failure mode rather than the current values. That is the part
that stops #17089 recurring.
One non-blocking note
PhaseScore.complete and PhaseScore.status ignore defers_to, while as_report()
short-circuits and hardcodes complete: False for a deferring phase. So a deferring phase with
no skips and a high ratio returns complete == True from the property and "complete": False
from the report.
Nothing reads the properties today — phase_progression_manager.py:453 correctly reads
phase_data["complete"] from the dict — so this is latent, not live, and I am not asking you
to hold the PR for it. But it is the same trap the PR removes elsewhere: two accessors for one
question that disagree, where the safe one happens to be the one in use. A one-line
if self.defers_to: return False in complete, and the matching branch in status, would
close it before someone reaches for the property.
|
Second review pass — e5 reviewed this first, so this is not the initial one. One thing to fix before merge, and it is a regression this PR introduced rather than a pre-existing gap. Everything else is verified sound or filed separately. The core fix is real, and I tried to break it
Ratchet verified compliant: Fix before merge — the schema rename broke a consumer this PR did not update
comment += `**System Maturity:** ${results.overall_maturity?.toFixed(1)}%\n\n`;
comment += `${emoji} **${phase.name}**: ${phase.completion_percentage?.toFixed(1)}%\n`;Under the new schema Fix: read Why it is not visible today, and why that makes it worth fixing now. The step is gated on pull_request:
branches: [ release ]This repo's PR base is I have deliberately not claimed "every PR comment will read undefined%". It will, for PRs targeting The trigger scope itself is pre-existing and out of this PR's stated scope — worth a follow-up so the gate this PR hardens actually blocks the PRs it is meant to, rather than only running post-merge on Filed separately — #17559
That is the same inversion this PR fixes, one layer below it. It does not overlap with the fix — your live-stack grouping means those groups are skipped in Worth separating two claims that read as one: "#17089 is fixed" is true; "an unmeasured feature cannot report implemented" is not yet. Minor
Not verified
|
1 participant
Thinking Path
"Phase 6: Enhanced UI/UX — 100%" meant
autobot-frontend/src/App.vueandpackage.jsonexist.Two causes produced that one number, and the second is why nothing ever failed.
1. Skipping the live checks shrank the denominator. A phase's figure is
passed / total. #7496skipped the endpoint, service and feature checks under
--ci-modefor a sound reason — the workflownever brings a stack up — but it removed them from
totalas well as the numerator. Phase 6's twosurviving checks were file-existence tests, so 2/2 read as 100% complete. The less a run could
measure, the higher it scored. The original comment says the skip existed to stop those checks
"drag[ging] the score below the threshold", which states the inversion outright: not looked at
became nothing missing.
2. 14 of 15 feature-check paths did not exist. They pointed at the pre-reorganization layout
(
src/,backend/,autobot-vue/), so even with a live stack those checks could only ever reportFalse. #7496's comment claims the paths were refreshed for the current layout — it refreshed the
criteria dict and left the lambdas ~700 lines below untouched. The comment was true about what it
sat above and false about the file, and the same change began skipping those checks in CI, so the
resulting zeros never surfaced.
The consequence the issue did not mention, and the reason this is a correctness defect rather
than a dashboard one. Following the consumers rather than grepping the symbol:
autobot-backend/phase_progression_manager.pycallsvalidate_all_phases()and then decided aphase was completed with
phase_data["completion_percentage"] >= self.config["validation_threshold"]— and that module progresses phases from the resulting set. So a
--ci-moderun could promote aphase on "the declared files are present". A reviewer reading only the issue would take this for a
reporting fix.
What Changed
New:
autobot-infrastructure/shared/scripts/phase_score.py— the policy deciding when a numbermay be called completion. A phase with even one skipped group cannot be called complete at any
ratio, and its figure is named
structural_presence_percentage.completion_percentageisabsent from the report when anything was skipped — not zero, not null — so a consumer reaching
for it gets a
KeyErrorrather than a number it would have believed.overall_maturityisNoneinCI mode and
ready_for_productionisNonerather thanFalse, becauseFalsereads as "assessedand failed".
It is a separate module because the policy has to be testable, and
phase_validation_systemimportsaiohttp,psutil,requestsandautobot_shared.redis_clientat module scope. A policy nothingcan exercise is how this one survived.
The UI/UX phase now reports no figure at all (AC3's second option). It names the four workflows
that actually test it —
frontend-test,visual-regression,stylelint-tokens,frontend-typecheck-regression— and reportsstatus: deferred-to-dedicated-gates. Whether a UI isconsistent, responsive and internationalised is not a question a file-existence sweep can answer at
any percentage. It also contributes nothing to the aggregate: leaving it in the weighted mean
would make "reports no score" true of the phase and false of the run, since its 100% would still
reach the CI gate through the average.
overall()excludes deferring phases and discloses thatit did, via
phases_excluded_from_scoreandverified_by_dedicated_gates— an average over fewerphases than the run evaluated is the same silent denominator change this issue is about.
All 54 referenced paths now exist — 39 in
PHASE_CRITERIA(llm_interface_pkg→llm_shared,renamed in #6941) and 15 in the feature lambdas (
autobot-vue/→autobot-frontend/;src/andscripts/→ their current packages).The gate keeps its 60/80 thresholds and the same number, now read from
structural_presenceandlabelled as such.
.get("overall_maturity", 0)would have returnedNoneand brokenbcratherthan failing loudly.
Three defects this PR caused and its own guards caught — all mine, all recorded because the
guards are the deliverable as much as the fix is:
_determine_phase_statuswith a slice bounded by the nextdefswallowed the
async defthat followed, deleting_validate_phase_featureswhile its callsite stayed.
py_compilepassed — Python resolves attribute names at call time — and in--ci-modethat line sits in the branch CI does not take. Three independent reasons nothingfailed. Restored verbatim from
origin/main;test_no_self_call_points_at_a_missing_methodnowfails on exactly that deletion, mutation-proved.
_SHARED_SCRIPTSto fix three E501s split those paths into aNameplus aConstant,and the path scanner — which collected string literals — saw 4 paths instead of 15. Its floor
failed immediately. A scanner finding 4 would have reported "no missing paths" and been believed;
the floor caught it because it asserts a population, not an absence. The scanner now resolves
module-level string constants and flattens the
root / a / bchain.limits.
A duplicated threshold consolidated rather than a ceiling raised. My first version took
phase_progression_manager.pyfrom its 851-line pin to 859.config["validation_threshold"]wasthe literal
95.0in a hard-coded dict with no external merge — a constant wearing a config key'sclothes — and
phase_score.COMPLETE_ATis also95.0. Two copies of one bar, kept in step by hand.The policy owns it now, which brought the file to 850, so the pin is lowered to 850 in both
baseline copies in the same commit.
Single-issue rationale
#17089 is one issue and is delivered whole; there is no second issue to batch with it. The nearest
related work is deliberately not here: #17497 (a prototype-chain
inallow-list inNodeServicesPanel.vue) is a different file and risk class, and #17482 covers theredis_client-on-import key-material incident referenced under Verification. Batching either wouldhave mixed a milestone fix with unrelated risk.
Verification
What I did not verify, stated rather than implied. I did not run
phase_validation_system.py. Importing it pullsautobot_shared.redis_clientat module scope,which generates and persists key material when none is configured — #17482 is a live incident
from exactly that, where the generated key file blocked a
code_sourcersync until it was removed.So a guard that imported this module would mutate the tree it guards. The tests therefore read the
system with
astand load only the policy module, and the end-to-end behaviour of the assembledreport is CI's to prove.
py_compileis not a substitute and did not catch the deleted methodabove, which is why the self-call guard exists.
Mutation-proved, not merely asserted: reinstating the numeric read in
phase_progression_managerfails its guard, and re-deleting
_validate_phase_featuresfails the self-call guard naming thatmethod.
Model Used
Claude Opus 5 (1M context)
Closes #17089
Summary by CodeRabbit