feat(web): shared-topology wizard + SCM role controls (spec 024, plan 5/5 — spec complete) - #1546
Conversation
Plans 1-4 made shared JIRA keys route, scope and stamp correctly, but nothing could SET a discriminator. buildIntegrationConfig rebuilds the config from wizard state, so a value written out-of-band was wiped by the next save — sharing a board was configurable only in theory, which is why the README has carried a "do not configure this in production" caveat since plan 2. The step is optional and empty by default: a project that owns its key outright leaves it alone and saves a config byte-identical to before this plan. "None" is a legitimate choice, not an unfinished one — a key may have exactly one project without a discriminator, and that project is the key's default owner. The step mirrors the backend jiraConfigSchema constraints inline (no quote or backslash; no whitespace in a label) so the operator is told here rather than by a save-time rejection, and warns that a component must already exist on the JIRA project because JIRA will not create it. Placed after status mapping in BOTH the manifest and the wizard step list. The existing "one step per wizardSpec entry, in the same order" test caught that I had them disagreeing — the manifest at position 4 and the wizard at position 6. Round-trip pinned: set → serialize → hydrate → re-serialize preserves the discriminator, which is precisely the wipe this closes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Plan 4 made a repository shareable and enforced exactly one primary per
repo at save time; this is how an operator says which they are. Without
it the choice existed only in the API.
The role is omitted from the payload when untouched. That matters
backend-side: an update that does not mention the role preserves it, so
an unrelated save — a base-branch edit, say — leaves a shared
repository's topology alone rather than restating it. It is also omitted
when there is no repository at all, since the backend rejects that pair
and there is no reason to let the UI build a request that cannot succeed.
The payload shape lives in a pure builder rather than being asserted
through a DOM render: whether a field is ABSENT is the load-bearing part
here, and absence is what a rendered assertion sees worst.
The existing error slot already renders the mutation message verbatim, so
plan 4's operator-actionable rejections ("Repository X is already used by
project Y") surface as written rather than as a generic failure.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The "do not configure a shared key in production" warning has ridden along since plan 2. Its two reasons are now gone: read-scoping shipped in plan 3, and the wizard field it warned about — the one whose absence meant a discriminator could only be written out-of-band and was then wiped by the next save — exists as of this plan. Replaces it with what an operator actually needs: how to pick a discriminator (label unless you have a reason, since JIRA creates labels on demand and will not create components), that exactly one project per key may be left on None, and that exactly one project per repository is the primary. Marks AC #5 deferred rather than done — the visual-placement protocol needs a live dev API and a JIRA project, which this session does not have. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review round 1 must-fix, and it invalidated the caveat removal this plan is built on. cascade-tools commands do not read the database: CredentialScopedCommand rebuilds the project from env vars via synthesizeProjectFromEnv, and augmentProjectSecrets emitted only projectKey, baseUrl, authType and statuses. So inside the worker `config.routing` was undefined and the JIRA provider stamped nothing on the work items an agent created and scoped nothing on the ones it listed. The consequence is a silent, permanent misroute: a friction report or split child filed via `cascade-tools pm create-work-item` carried no discriminator, matched no sibling on its next webhook, and was handed to the key's DEFAULT project. One team's work quietly became another's. The mirror break made `list-work-items` return every sibling team's issues. This is the NINTH hand-picked projection of the JIRA config in this spec's territory — a hop nobody had enumerated. authType got its env var when MNG-1736 threaded it; spec 024 had not. The README prose added by this plan asserted the broken behaviour verbatim. The round-trip test drives the REAL projection in both directions — actual augmentProjectSecrets output into actual synthesizeProjectFromEnv — because a hand-built config on either side would only prove the two shapes I wrote agree with each other. A malformed value degrades to "no discriminator" rather than throwing at the head of every invocation. Also from the review: - The value input unmounted mid-edit: clearing the box cleared the kind, which drove the input's own visibility. Selection is now view state, so the box survives being emptied. - `repoPrimary` vs `effectiveRepoPrimary` were two same-shaped bindings six lines apart, one never undefined; substituting one for the other would have destroyed the absence invariant with every test still green. Renamed to `repoPrimaryChoice` / `displayedRepoPrimary`. - `SCMTabProject` now declares `repoPrimary` instead of casting it, so a renamed field or new upstream projection is a compile error rather than a secondary silently displaying "Primary". - A test named "does not emit a value the backend would reject" asserted that the wizard DOES emit one. Renamed to what it proves. - `describeInvalid` duplicated the backend constraints untested; now pinned against the real jiraConfigSchema, and mutation-proved. - The README claimed two projects on None are rejected at save time. Not for grandfathered pre-024 pairs — corrected, and three stale "until plan 5" parentheticals updated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review round 2 found a TENTH hand-picked projection of the JIRA config: `reportFriction.ts` had its own env→ProjectConfig synthesizer, a near-clone of the cascade-tools one, hand-picking the same four fields and so never learning about routing. Friction reports were filed unstamped and routed to the key's default project on a shared board — and `claude-code` is the default engine on that path, so this was the default configuration. Two details make it worse than one more hop. This branch's own README asserted the opposite in prose. And the failure was asymmetric: if the immediate filing failed, the sidecar drain re-filed it via the DB project and stamped correctly, so success was unstamped and retry was stamped. Adding a third copy would have guaranteed an eleventh, so there is now one: `src/jira/config-from-env.ts`. A field added there reaches every worker-side consumer; a field forgotten there is missing from all of them at once, which is far easier to notice than missing from exactly one. A static guard fails if any file outside it reads the CASCADE_JIRA_* vars. The precedent supports this — CASCADE_JIRA_AUTH_TYPE landed in all three sites at once when MNG-1736 threaded it; spec 024 managed two of three. Also from the review: - AC #2 was marked done on a FALSE premise. I recorded "renders a backend rejection verbatim" as uncoverable because the dashboard suites have no DOM harness. They do — project-worker-image.test.ts renders in this exact directory, and vitest.config.ts carries a react-query alias dedupe added to make it work. Now covered for real. - That render test then gave FALSE ASSURANCE on its own headline claim: a wrap mutation on the save slot passed, because the component has a second error slot fed by the same mocked hook and the text query found the untouched one. Scoped by testid; the wrap now kills two tests. - Lint had regressed 13 → 14: the round-1 fix pushed secretBuilder past the cognitive-complexity threshold. Extracted `injectJiraConfig`, matching the file's existing inject* shape. Back to baseline. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Shared board and repository topologies are now configurable by an operator rather than only by the API, which is what the README's "do not configure this in production" caveat had been waiting on since plan 2. That caveat is gone. Three review rounds, four must-fixes. The first invalidated this plan's premise outright: cascade-tools rebuilds its project from env vars, and the discriminator was not among them, so stamping and JQL scoping both silently no-opped inside every worker. Removing the caveat on top of that would have been actively dangerous. Three of the four were verification failures rather than coding ones — an AC claimed on a premise I never checked, and a render test that passed a wrap mutation because it matched by text and found a second, untouched error slot. AC #5 is deferred, not done: its manual protocol needs a live dev API. Round 1 found a real UX defect in that surface — the value input unmounted mid-edit — which is precisely what the deferred pass would have caught. Verified: 11205 unit tests, typecheck clean in both workspaces, lint at its 13-warning baseline. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… complete Several CASCADE projects can now share one JIRA project key or one GitHub repository. Events route to the project that owns them, a scoped project sees and creates only its own slice, the previously-silent shadowing configuration is unsaveable, and both are configurable in the dashboard. Five plans, thirteen review rounds, thirteen must-fixes. The dominant theme was not routing logic — that was largely right first time — but a JIRA config field surviving the projections between the database and the code that reads it. This spec hit that class TEN times, twice in code paths nobody had enumerated. It ended by collapsing the two worker-side synthesizers into one with a static guard, which is the only fix that addresses the pattern rather than an instance. CLAUDE.md untouched by this spec — nothing to audit. Decision records not written: docs/decisions/ does not exist in this repo, so extraction would mean establishing a ledger. Proposed to the operator rather than created unilaterally. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
nhopeatall
left a comment
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
APPROVE — plan 5/5 correctly closes spec 024. The root-cause fix (one env→config synthesizer in src/jira/config-from-env.ts, with routing threaded through secretBuilder) is sound and verified by a real bidirectional round-trip; the wizard state/serialization/hydration and the SCM role control round-trip correctly against the actual backend contracts.
What I verified
- The env hop is a real projection, not shape-agreement.
injectJiraConfigemitsCASCADE_JIRA_ROUTINGonly when set;buildJiraConfigFromEnvparses it defensively (degrades to no-discriminator on malformed JSON, so it can't take acascade-toolsinvocation down). The round-trip test drives the actualaugmentProjectSecrets→synthesizeProjectFromEnv, and the one-synthesizer guard holds — grep confirms the four guarded vars are read only inconfig-from-env.ts. - The baseUrl precedence flip (
||vs??) is unreachable —secretBuildersetsJIRA_BASE_URLandCASCADE_JIRA_BASE_URLto the same value, so no worker can observe a difference. Correctly claimed. - SCM role wiring is complete.
projects.updateacceptsrepoPrimary;getProjectFulldoes a fullselect()sogetByIdreturns it;falsesurvives?? true(only null/undefined defaults to Primary).buildScmSavePayloadomits the role when untouched or repo-less, matchingresolveRepoPrimary+assertRepoPrimaryHasRepo. - UI validation mirrors the backend.
describeInvalidagrees withjiraConfigSchema(quote/backslash always, whitespace for labels), and the test cross-checks against the real schema rather than hardcoded strings — so a schema tightening can't silently desync the UI. - Manifest
wizardSpecandjiraProviderWizard.stepsagree by id and order; the caveat removal in the README is justified now that the field exists; no active doc links point at the renamed.md.donefiles.
The self-corrected verification failures (AC #2 false-premise, the wrap-mutation that passed by finding a second error slot) are handled well — the data-testid-scoped assertion is the right fix.
Non-blocking
One observation on the routing step's selectedKind mitigation (inline). It closes the "value box unmounts mid-edit" defect for the create flow but not the edit flow. Disclosed under the deferred AC #5, so not a blocker.
🕵️ claude-code · claude-opus-4-8 · run details
| function JiraRoutingAdapter({ state, dispatch }: ProviderWizardStepProps): ReactElement { | ||
| // `selectedKind` is view state, not config: it keeps the value input mounted | ||
| // while the box is empty, which the reducer treats as "no discriminator". | ||
| const [selectedKind, setSelectedKind] = useState<JiraRoutingKind | undefined>(undefined); |
There was a problem hiding this comment.
Non-blocking, edit-flow only. selectedKind initializes to undefined, so shownKind = selectedKind ?? routingKind. Fresh project: fine — picking a kind from the dropdown seeds selectedKind, so clearing the value keeps the input mounted. Existing project: the saved discriminator is hydrated into routingKind (e.g. label) and the operator never touches the dropdown, so selectedKind stays undefined. Clearing the value box dispatches SET_JIRA_ROUTING_DISCRIMINATOR(label, empty); the reducer clears both fields, routingKind becomes empty, and shownKind falls back to empty → the input unmounts mid-edit. That is the exact round-1 defect, only closed for the create path. Seeding from the hydrated kind, e.g. useState(() => state.jiraRoutingKind || undefined), would cover edit too. Consistent with your deferral of AC #5, so flagging as non-blocking.
There was a problem hiding this comment.
Fixed. JiraRoutingAdapter now seeds selectedKind from the hydrated jiraRoutingKind via a mount effect, so clearing a saved value on the edit path keeps the value input mounted instead of dropping it (and the kind dropdown no longer snaps back to None).
I used a useEffect rather than useState(() => state.jiraRoutingKind || undefined): buildEditState reaches state through a deferred INIT_EDIT dispatch, so a useState initializer would read the pre-hydration '' and still miss it — the exact async caveat you raised on the sibling thread. The effect self-guards on selectedKind === undefined, so it never overrides an explicit dropdown choice (including None).
Added tests/unit/web/jira-routing-adapter-render.test.ts — a jsdom regression that drives the real reducer with a post-mount hydration dispatch and asserts the input survives a clear; verified failing against the pre-fix code before restoring the fix.
nhopeatall
left a comment
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Solid, well-tested completion of spec 024. Verdict: COMMENT — no correctness/security/data issues; one non-blocking UX gap worth the author's attention for the deferred AC #5 pass.
Verified end-to-end:
- Routing thread:
secretBuilder.injectJiraConfigemitsCASCADE_JIRA_ROUTING→buildJiraConfigFromEnvrebuilds it →JiraPMProviderreadsthis.config.routing?.discriminatorfor bothstampDiscriminatoranddiscriminatorJqlClause. Shapes match. - SCM repo-role:
getProjectFullselects all columns (incl.repoPrimary) →serializeProject→getById, so the tab receives the stored value;projects.updateacceptsrepoPrimary;buildScmSavePayloadomits it when untouched or repo-less (matchingassertRepoPrimaryHasRepo). - Wizard wiring: manifest
wizardSpec.stepsandjiraProviderWizard.stepsalign 1:1 by id/order, sojira-routing(index 3) rendersJiraRoutingAdapterviaManifestProviderWizardSection. - One-synthesizer guard is genuine: only
src/jira/config-from-env.tsreads the fourCASCADE_JIRA_*vars insrc/. - UI validation:
describeInvalidfaithfully mirrorsjiraConfigSchema(quote/backslash for both kinds; whitespace for labels).
Code Issues
Should Fix (non-blocking)
- web/src/components/projects/pm-providers/jira/wizard.ts:328 — the
selectedKindunmount-prevention has a residual gap on the edit path (same defect class Round 1 fixed for fresh-add). Details inline.
Minor
- src/api/routers/projects.ts:502-503 — the retained comment "the SCM tab … cannot yet send a role (that toggle is plan 5)" is now stale: this PR is plan 5 and ships the toggle. The
requested === undefinedbranch is still correct, but its rationale is now "the tab omits the role when the operator hasn't touched it," not "cannot yet send a role." - The acknowledged follow-ups in the PR description (no
docs/decisions/ledger to land the six strategic decisions; staledocs/BUGS.mdrepo-strand entry) are reasonable to defer.
🕵️ claude-code · claude-opus-4-8 · run details
| function JiraRoutingAdapter({ state, dispatch }: ProviderWizardStepProps): ReactElement { | ||
| // `selectedKind` is view state, not config: it keeps the value input mounted | ||
| // while the box is empty, which the reducer treats as "no discriminator". | ||
| const [selectedKind, setSelectedKind] = useState<JiraRoutingKind | undefined>(undefined); |
There was a problem hiding this comment.
The selectedKind view-state prevents the value input from unmounting mid-edit on the fresh-add path (picking a kind from the dropdown sets selectedKind, so shownKind = selectedKind ?? routingKind stays non-empty even when the reducer clears the value). But it's still reachable on the edit path — the exact defect Round 1 fixed:
buildEditStatehydratesjiraRoutingKind='component';selectedKindstaysundefined.shownKind = undefined ?? 'component' = 'component'→ value input shown with the saved value.- Operator backspaces the value to empty →
SET_JIRA_ROUTING_DISCRIMINATORwithvalue=''clearsjiraRoutingKindto''→shownKind = undefined ?? '' = ''→ the<Input>unmounts and the dropdown snaps to None mid-edit.
So an operator clearing a saved discriminator to retype it loses the box and must re-pick the kind. Consider seeding selectedKind from the hydrated routingKind when the step mounts with a discriminator already set. Note useState(state.jiraRoutingKind) alone won't fix it cleanly: '' is non-nullish (it would defeat ?? routingKind) and hydration is async, so a mount effect — or a visibility rule that also keeps the input while a value was recently non-empty — is safer. Non-blocking, but worth catching in the deferred AC #5 manual pass since that's precisely the surface Round 1's UX defect lived on.
There was a problem hiding this comment.
Done, and done the way you recommended — a mount effect, not a useState initializer.
JiraRoutingAdapter seeds selectedKind from the hydrated jiraRoutingKind once (guarded on selectedKind === undefined). After buildEditState's deferred INIT_EDIT dispatch, backspacing a saved discriminator now keeps the input mounted, while an explicit None still hides it. As you noted, useState(state.jiraRoutingKind) alone wouldn't do it — '' is non-nullish and hydration is async, so the initializer reads '' and would still unmount.
Regression: tests/unit/web/jira-routing-adapter-render.test.ts renders the real step under jsdom, simulates the deferred hydration, and pins both cases (clear keeps the box; None hides it).
…e input mounted Addresses review feedback on PR #1546. - JIRA Team routing step: the `selectedKind` unmount-prevention only covered the fresh-add path. On edit, `buildEditState` hydrates the saved discriminator via a deferred INIT_EDIT dispatch and the operator never touches the dropdown, so `selectedKind` stayed `undefined` — clearing a saved value then dropped `jiraRoutingKind` to '' and unmounted the input mid-edit (the Round 1 defect, still reachable on the edit path). Seed `selectedKind` from the hydrated kind via a mount effect; a `useState` initializer would read the pre-hydration '' and miss it. Adds a jsdom regression test that proves the fix under the real async-hydration timing (and fails against the naive initializer approach). - projects.ts: refresh the now-stale `resolveRepoPrimary` comment — plan 5 ships the role toggle, so the rationale is "the tab omits the role when untouched," not "cannot yet send a role." Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`resolveJiraBaseUrlFromEnv` reads the non-prefixed `JIRA_BASE_URL` before `CASCADE_JIRA_BASE_URL`, but the round-trip helper only propagates the emitted `CASCADE_JIRA*` secrets into the test env. When the shell already exports `JIRA_BASE_URL` (as some worker/dev environments do) it leaked past the `beforeEach` reset and won over the emitted value, failing the "leaves the other JIRA fields intact" assertion. Unset in CI, so it only bit locally / at pre-push. Clear it alongside the other JIRA vars, consistent with the existing stubs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
nhopeatall
left a comment
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
APPROVE — spec 024's final plan lands the JIRA routing-discriminator wizard field and the SCM repository-role control correctly, and the load-bearing fix (threading routing through the worker env) is verified end-to-end, not just at the shape level.
What I verified
Core fix, runtime-traced. injectJiraConfig now emits CASCADE_JIRA_ROUTING → buildJiraConfigFromEnv / parseJiraRoutingFromEnv rebuild it → getJiraConfig returns the whole object (no re-projection) into new JiraPMProvider(jiraConfig) → discriminatorJqlClause / stampDiscriminator read config.routing?.discriminator. The { discriminator: { kind, value } } shape matches at every hop (config-from-env.ts, pm/config.ts, pm/jira/adapter.ts). jira-routing-env-roundtrip.test.ts drives the real augmentProjectSecrets → synthesizeProjectFromEnv, so it proves the projection, not two hand-built shapes agreeing.
Consolidation is a genuine improvement. Collapsing the cascade-tools and friction synthesizers onto config-from-env.ts plus the static one-reader guard removes the exact drift class that produced the unstamped-friction misroute. The base-URL precedence flip on the friction path is unreachable because injectJiraConfig sets CASCADE_JIRA_BASE_URL and JIRA_BASE_URL to the same value inside every worker.
SCM data flow intact in real usage. repoPrimary flows DB column (repo_primary, NOT NULL default true) → getProjectFull (select()) → serializeProject (...rest) → getById → projectQuery.data → SCMTab prop, so the dropdown reflects stored state, not just the mocked prop. buildScmSavePayload omits repoPrimary when untouched or repo-less, matching resolveRepoPrimary's "absent preserves the stored role" contract and assertRepoPrimaryHasRepo.
UI validation agrees with the backend. describeInvalid mirrors jiraConfigSchema's quote/backslash + label-whitespace rules, pinned by it.each against the real schema; the empty value is handled by omitting routing (not by a min(1) divergence). The JQL-injection boundary (AND field = "${value}") is enforced at save time by the schema's ^[^"\\]+$ regex, so a validated config value is always safe by the time it reaches the adapter.
Wizard mechanics. Manifest ↔ provider-wizard step IDs and order align (jira-routing after status-mapping); the reducer's empty-value-clears rule plus the selectedKind view-state correctly keep the value input mounted mid-edit on both the add and the deferred-INIT_EDIT edit path — the jira-routing-adapter-render.test.ts timing reproduces the Round 1 defect.
Non-blocking
- AC #5 (visual placement of both controls) is deferred/unverified, as the description discloses. Behavior is covered at reducer / payload / jsdom-render level, but pixel placement and usability are not — worth the promised manual pass against
npm run dev:web, since Round 1's real defect surfaced in exactly that surface.
CI 7/7 green; 27 new tests; typecheck + lint clean in both workspaces.
🕵️ claude-code · claude-opus-4-8 · run details
3 participants
Summary
Plan 5 of 5 — the last of spec 024, which this PR marks
.done.Plans 1–4 made shared JIRA keys and shared GitHub repositories route, scope, stamp and validate correctly. But nothing could set a discriminator:
buildIntegrationConfigrebuilds the config from wizard state, so a value written out-of-band was wiped by the next save. Sharing a board was configurable only in theory — which is why the README has carried a "do not configure this in production" caveat since plan 2.That caveat is now gone, and this PR is mostly about earning its removal.
cascade-toolscommands do not read the database. They rebuild the project from environment variables, andaugmentProjectSecretsemitted norouting. So inside every workerconfig.routingwasundefined:stampDiscriminatorstamped nothing,discriminatorJqlClausereturned''.The consequence was the worst kind of silent:
That was the ninth hand-picked projection of the JIRA config in this spec's territory. Round 2 then found a tenth —
reportFriction.tshad its own near-clone synthesizer, on the default engine path, with a perverse asymmetry: immediate filing was unstamped, but the sidecar retry re-filed via the DB project and stamped correctly.Adding a third copy would have guaranteed an eleventh, so both now share
src/jira/config-from-env.ts, with a static guard test that fails if any other file reads theCASCADE_JIRA_*vars. The precedent supports this shape:CASCADE_JIRA_AUTH_TYPElanded in all three sites at once when MNG-1736 threaded it; spec 024 managed two of three.Removing the caveat on top of that would have been actively dangerous — and the operator prose I wrote in the same PR asserted the broken behaviour verbatim.
Two of my own verification failures, stated plainly
I marked an AC done on a premise I never checked. I recorded "renders a backend rejection verbatim" as uncoverable "because the dashboard suites have no DOM harness". They do —
project-worker-image.test.tsrenders in the same directory, andvitest.config.tscarries a react-query alias dedupe added to make it work.Then my replacement test gave false assurance on its own headline claim. Mutating the save slot to wrap the message passed all five tests — the component has a second error slot fed by the same mocked hook, and my text query found the untouched one. Scoped by
data-testid, the wrap now kills two tests.Both are worth knowing when weighing this PR's test evidence.
Testing
27 new tests — 15 routing/serialization, 6 SCM payload, 5 SCM render (jsdom), 6 env round-trip including the one-synthesizer guard. 11205 unit passing, typecheck clean in both workspaces, lint at its 13-warning baseline.
The env round-trip drives the real projection both ways — actual
augmentProjectSecretsoutput into actualsynthesizeProjectFromEnv— because a hand-built config on either side would only prove that two shapes I wrote agree with each other.AC #5 is deferred, not done
Its manual protocol needs
npm run dev:webagainst a live dev API and a JIRA project, neither available here. Round 1 found a real UX defect in exactly that surface — clearing the value box unmounted the box you were typing in — which is precisely what the deferred pass would have caught. Treat visual placement as unverified.Spec 024 closes here
Five plans, thirteen review rounds, thirteen must-fixes. The dominant theme was never the routing logic — that was largely right first time — but a config field surviving the hand-written projections between the database and the code that reads it. Ten instances, two in paths nobody had enumerated.
Not done, and worth a follow-up:
docs/decisions/does not exist in this repo, so the spec's six strategic decisions have nowhere to land. I did not create a ledger unilaterally.docs/BUGS.mdalso still carries the "moving a project's repo can strand the old one" entry.🤖 Generated with Claude Code