Repository navigation
feat(llc/frontend): projects can be edited, through the route that already existed (#17681) - #17695
Merged
Merged
Conversation
…ready existed (#17681) `PATCH /api/llc/projects/{id}` (`llc/api/sprints.py:685`) has worked since it was written, with its IDOR guard, and no frontend called it. Projects were write-once at creation. The owner reported this as "I can create a project but cannot edit it", which was exactly what the code did. The field set is read off `ProjectUpdate`, not off `ProjectResponse` -- the response returns more than the route accepts. Five of its eight fields are in the form; the three left out, each for a reason rather than by omission: lead_agent_id / lead_user_id a person-picker here would settle #17682's open ruling on how people attach to a project, one field at a time env free-form JSONB; a textarea of raw JSON is a worse editor than none `status` is a select over the five members of the `projectstatus` DB enum, not a text field: `ProjectUpdate.status` is an unvalidated `str` while the column is an enum, so a plausible typo like "in-progress" is refused by Postgres as a 500 rather than by FastAPI as a 422. Filed as #17694 with the other half of that finding -- `exclude_none=True` means no field can be cleared back to null, so the form cannot offer a "clear" control for the target date and does not pretend to. The request carries only the fields the user actually changed. Sending everything also works, and quietly rewrites values nobody touched, so the tests assert the body rather than the rendered card -- a card-only assertion passes against either. Not gated on `lifecycle_state`: renaming is not a lifecycle operation, and an archived project with a wrong name is exactly the one that needs fixing. A 404 reports as "no longer available". The IDOR guard answers 404 for a project outside the caller's org as well as for one that does not exist, so the message does not claim a permission verdict it cannot distinguish. Thirteen keys across all 11 locales. Every behaviour change is mutation-tested: sending every field instead of a diff, prefilling from blanks, collapsing the 404, dropping a status option, and skipping the no-change early return are each CAUGHT by a named test. Closes #17681 Refs #17682 Refs #17694
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (13)
✨ Finishing Touches📝 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 |
Contributor
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
This was referenced Sep 28, 2026
mrveiss
added a commit
that referenced
this pull request
Sep 28, 2026
… that broke the contract pin (#17698) Two more failures on the same base, both of which only surface on a PR that actually runs `python-suite` -- #17698's mechanism. 1. REACH FLOOR, restated rather than nudged. `undefined-css-var-scan` declared `floor=1400, growth=60` against a measurement of 1452 files. The tree is now **1463**, so the slack reached 63 against an allowance of 60 and `verify_floor` refused the DECLARATION -- the scan itself was fine. Re-measured and restated at **1433**, not 1403. The old pin sat 52 of its 60 allowance below its own measurement, leaving room for 8 more files before a bump came due; 11 arrived. 1433 puts the slack at 30, half the allowance, so the next bump is due after 30 more files. `population - growth` would put the slack exactly at the allowance and go red on the next file anyone adds, which is the state this just came out of. 2. INLINE GENERIC -- and this one is MINE, not pre-existing base debt. `inline_generics` went 562 -> 563 because #17695's edit form named a response type at the call site in `saveProject`. Same story as the CSS tokens: merged green, invisible to a frontend-only PR. I had told the coordinator this pin "passes at 563". That was wrong and worth naming: I had run `frontend_fragmentation_ratchet_test.py`, which contains the string `inline_generics` **zero times** -- the pin lives in `frontend_api_contract_ratchet_test.py`. A correct measurement of a different question, and I quoted a number I had not measured. The pin is shrink-only and does not move. Fixed by not asserting a shape at the call site: the response is neither typed nor read, and the list is reloaded from the endpoint that already types it -- what `createProject` and `deleteProject` in this file already do after a mutation. The guard's own advice is that naming a response type at the call site is a claim TypeScript cannot check, and a refetch removes the claim rather than relocating it. The explaining comment deliberately does NOT contain the offending syntax: the detector reads comments, so writing it there to explain it would be counted. My first attempt did exactly that and left the count at 563. The edit test now asserts the REFETCH and not merely the rendered name -- a handler that read the response would also end up showing the new name, so the two are indistinguishable by output alone. Mutation-tested: restoring the inline generic is CAUGHT by both the contract ratchet and the edit test. Refs #17698 Refs #17552 Refs #12363
mrveiss
added a commit
that referenced
this pull request
Sep 28, 2026
…tem CSS, stale SLM locales (#17698) (#17699) * fix(frontend): unblock base — undefined tokens, duplicated design-system CSS, stale SLM locales (#17698) Three failures on `origin/main` at 290fae3, all from my #17691 and #17695. Nothing else in this commit. 1. FIVE UNDEFINED TOKEN REFERENCES. `--color-text-primary` (x2) and `--color-text-secondary` (x3) are not tokens; the real names are `--text-primary` / `--text-secondary`. No fallback added -- the ratchet's own message says why: a fallback hard-codes a colour that then ignores the theme. The rest of `ProjectBrowserView.vue` already used the correct spelling on twenty surrounding lines, which is what makes this a slip rather than a misunderstanding. 2. A SECOND IMPLEMENTATION OF AN EXISTING DESIGN-SYSTEM COMPONENT. `DesktopAccessDenied.vue` hand-rolled `.desktop-denied{,-icon,-title,-desc}` -- the exact four-part structure `assets/css/components.css:649` already ships as `.empty-state{,-icon,-title,-desc}`. Likewise `.lifecycle-hint` duplicated `.field-label-hint` and `.create-checkbox` duplicated `.checkbox-label`/`.checkbox-input`. That is a canonical-design-system violation on its own, and it is also exactly what put three fragmentation ratchets over: +1 components_declaring_styles, +6 css_rule_declarations, +6 distinct_class_names. Attributed by counting, not assumed -- four rules and four classes from the new component plus two of each from `ProjectBrowserView`, which is 1/6/6 exactly. Fixed by using the canonical classes, so `DesktopAccessDenied.vue` now has no `<style>` block at all. The ratchets return to their pins by REMOVAL rather than re-pinning, which is what a shrink-only ratchet is for. `inline_generics` was NOT over on this tree -- it passes here at 563, so I have left it alone. 3. STALE GENERATED SLM LOCALES -- and this was the actual red check. `SLM Frontend Check` failed at `Locales match the lifting rule`, not at the ratchets. `lift_locale_translations.py` lifts a translation into `autobot-slm-frontend` for every key whose English source appears verbatim in `autobot-frontend`'s locales. The twenty keys I added in #17691 and #17695 gave the rule sources it did not have, so five locales (fa, he, lv, pl, ur) stopped being what the rule produces. Regenerated. Verified rather than trusted: 35,123 keys before and after, ZERO lost and ZERO added, 36 replacements only. They are strings that were stuck in English and now carry the translations added in #17695 -- `"Completed"` becomes `"Pabeigts"` in lv, and so on. This is 36 SLM strings becoming translated, not merely a checksum satisfied. Closes #17698 is NOT claimed: the instrument hole that let two green PRs break base is f8's to fix and is tracked there. This commit only removes the damage. * fix(guards): restate the css-var reach floor, drop the inline generic that broke the contract pin (#17698) Two more failures on the same base, both of which only surface on a PR that actually runs `python-suite` -- #17698's mechanism. 1. REACH FLOOR, restated rather than nudged. `undefined-css-var-scan` declared `floor=1400, growth=60` against a measurement of 1452 files. The tree is now **1463**, so the slack reached 63 against an allowance of 60 and `verify_floor` refused the DECLARATION -- the scan itself was fine. Re-measured and restated at **1433**, not 1403. The old pin sat 52 of its 60 allowance below its own measurement, leaving room for 8 more files before a bump came due; 11 arrived. 1433 puts the slack at 30, half the allowance, so the next bump is due after 30 more files. `population - growth` would put the slack exactly at the allowance and go red on the next file anyone adds, which is the state this just came out of. 2. INLINE GENERIC -- and this one is MINE, not pre-existing base debt. `inline_generics` went 562 -> 563 because #17695's edit form named a response type at the call site in `saveProject`. Same story as the CSS tokens: merged green, invisible to a frontend-only PR. I had told the coordinator this pin "passes at 563". That was wrong and worth naming: I had run `frontend_fragmentation_ratchet_test.py`, which contains the string `inline_generics` **zero times** -- the pin lives in `frontend_api_contract_ratchet_test.py`. A correct measurement of a different question, and I quoted a number I had not measured. The pin is shrink-only and does not move. Fixed by not asserting a shape at the call site: the response is neither typed nor read, and the list is reloaded from the endpoint that already types it -- what `createProject` and `deleteProject` in this file already do after a mutation. The guard's own advice is that naming a response type at the call site is a claim TypeScript cannot check, and a refetch removes the claim rather than relocating it. The explaining comment deliberately does NOT contain the offending syntax: the detector reads comments, so writing it there to explain it would be counted. My first attempt did exactly that and left the count at 563. The edit test now asserts the REFETCH and not merely the rendered name -- a handler that read the response would also end up showing the new name, so the two are indistinguishable by output alone. Mutation-tested: restoring the inline generic is CAUGHT by both the contract ratchet and the edit test. Refs #17698 Refs #17552 Refs #12363
2 tasks
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.
Thinking Path
The owner reported "I can create a project, but can't edit, delete or assign anyone to it". This is the edit third, and the reason it was reported as broken is that it was:
PATCH /api/llc/projects/{id}(llc/api/sprints.py:685) has existed and worked the whole time, IDOR guard included, and no frontend ever called it. Projects were write-once at creation.Pre-filters first, per the standing instruction: no PR references #17681, and the last commit touching
ProjectBrowserView.vuewas my own #17685. Clean.The field set is read off
ProjectUpdate, not offProjectResponse— the response returns more than the route accepts, and assuming from the response is how a form ends up posting fields that are silently dropped. Five of the eight are here. The three omissions are decisions, not oversights:lead_agent_id,lead_user_idenvstatusis a select, not a text field, and that is a finding rather than a UI preference.ProjectUpdate.statusisOptional[str]with no validation, while the column is a DB enum ofbacklog / planned / in_progress / completed / cancelled. So{"status": "in-progress"}— a hyphen where the enum has an underscore — passes FastAPI, reachessetattr, and fails at Postgres on commit. The caller gets a 500 naming a Postgres type instead of a 422 naming the allowed values. Filed as #17694, together with the other half of the same handler's contract problem:model_dump(exclude_none=True)meansnullis indistinguishable from not supplied, so no optional field can ever be cleared — the form therefore does not offer a "clear" control for the target date rather than offering one that silently does nothing.I referenced #17690 in two code comments before filing anything. #17690 turned out to be an unrelated milestone-hygiene issue belonging to someone else. Filed the real one, got #17694, corrected both references. Second time this shift I have written an issue number ahead of the issue; the fix is to file first and reference after, and I have stopped doing it the other way.
What Changed
The request carries only the fields the user actually changed. Sending everything also works — and quietly rewrites values nobody touched. Which is why the tests assert the request body and not the rendered card: a card-only assertion passes against either implementation, so it cannot see the difference.
Not gated on
lifecycle_state. Renaming is not a lifecycle operation, and an archived project with a wrong name is exactly the one you need to fix. Archive/Restore/Delete stay gated as #17683 left them.A 404 reports as "no longer available". The route's IDOR guard (
:697, #10148) answers 404 both for a project outside the caller's org and for one that does not exist, so the message deliberately does not claim a permission verdict it has no way to distinguish.Thirteen keys across all 11 locales. The modal mirrors the existing create modal's structure rather than introducing a second form idiom.
Verification
Mutation-tested, each applied then reverted:
The first and last are a pair on purpose. "Only changed fields" is satisfied by a handler that always sends everything — every field then legitimately differs from nothing — so the no-change case is what distinguishes them.
One thing I noticed and did not fix: the neighbouring lifecycle fixtures use
status: 'active', which is not a member of theprojectstatusenum. Harmless where they use it, wrong for a test that drives the status control, so this file's fixture uses'planned'. Not changing theirs — it is not this issue's file-set and the fixtures are not asserting on status.Not claimed: no host verification. The owner will see this after the next deploy; until then it rests on the unit tests and the route contract.
Closes #17681
Refs #17682
Refs #17694
Model Used
Claude Opus 5 (1M context)