Skip to content

feat(web): review cleanup — per-area feature flags, ASPM off by default - #925

Draft
arielshad wants to merge 12 commits into
mainfrom
feat/133-review-cleanup-cuts
Draft

arielshad wants to merge 12 commits into
mainfrom
feat/133-review-cleanup-cuts

Conversation

@arielshad

@arielshad arielshad commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

What

This is the cleanup PR from the T3 Code product review. It covers the items the owner agreed to, with one commit per item:

Commit Item Change
bb81a67 C1 The stale good-first-issue and monthly recap watchers no longer run in users' daemons (_serve, shep ui, dev:web). They now run from the new scheduled workflow .github/workflows/contributor-maintenance.yml, through two new commands, shep contributors stale-issues and shep contributors recap. Both reuse the existing use cases. The watcher services are deleted. The contributor view (leaderboard, contributor doctor) moves to /contributors, which is linked from CONTRIBUTING.md and not from the sidebar. /onboarding keeps only the collaboration tutorial and leaves the sidebar.
2810b71 C3 featureFlags.aspm now defaults to false in TypeSpec, the defaults factory and the web env fallback. Values users already saved are kept; no migration touches them. ROADMAP.md is updated to match. The e2e tests that visit /aspm turn the flag on for the test and back off afterwards.
0b9221f C4 Supply-chain security is folded into ASPM, because folding was cheap and removing it was not. Its own supplyChainSecurity flag is gone, and every surface follows aspm through isSupplyChainSecurityEnabled(): the feature-agent pre-check, the canvas badge, the Settings section and shep security enforce. SHEP_SUPPLY_CHAIN_SECURITY still overrides the flag in either direction. Shep's own security-enforce CI job already sets it to true, so the release gate keeps running. The feature_flag_supply_chain_security column stays and is no longer read.
d3a2157 C7 Removes system.autoUpdate and the placeholder agent types aider and continue. The sys_auto_update column is NOT NULL with no default, so it is still written as 1 but never read. A settings row that still holds aider or continue reads back as the default agent. agentQuestionBridge, AgentQuestionExecutorBridge and InteractionBubble are untouched.
ef9c9ae C2 Adds 12 software-factory flags, all on by default: spaces, trackers, knowledge, signals, opportunities, feedback, discovery, incidents, outcomes, docsFirst, autopilot, factory. They are stored in migration 166 (DEFAULT 1). Each flag gates its area's nav entry, pages, intake API, CLI group, daemon loop and, for docsFirst, the docs gate. A new view at /settings/feature-flags lists every flag with a one-line description, its default and an on/off switch; Settings links to it. shep settings flags [enable|disable <flag>] does the same from the CLI.
1b681a2 C5 Deletes the Vercel, Netlify, AWS Amplify and GCP Cloud Run stub providers, their enum members and all "Coming soon" UI. Cloudflare Pages is unchanged. An application that saved a removed provider reads it back as "no provider selected".

b4e7420 renumbers the spec to 135: the telemetry and unified-decisions workstreams took 133 and 134.

Why

Implements specs/135-review-cleanup-cuts/, from the "Where Shep Is Today and What to Cut" section of docs/competitors/t3code.md.

Screenshots / Recording

I checked the feature-flags view at desktop and phone width in the running app with pnpm dev:web and Playwright. Turning spaces off removed the Spaces link from the sidebar and made /spaces return 404; turning it back on restored both. (The screenshots are local to the session; I can attach them if needed.)

Testing

Local runs:

Check Result
pnpm generate committed output matches what it generates
pnpm tsp:compile no warnings
pnpm lint, pnpm format:check pass
pnpm typecheck, pnpm typecheck:web pass
pnpm test:unit 14,642 passed
pnpm test:int 2,107 passed
pnpm check:stories pass
pnpm build pass
pnpm build:storybook pass

The e2e specs I edited (aspm, ui-experience, ui-populated, cloud-deploy, contributor-onboarding) have not been run locally; CI runs them.

New tests:

  • feature-flag catalog completeness
  • List/SetFeatureFlag use cases
  • the CLI gate helper
  • shep settings flags
  • the background-sync gating
  • requireFeaturePage
  • feedback and alerts returning 404 while their flag is off
  • the docs-first gate
  • migration 166
  • a repository round-trip of every new flag
  • mapper fallbacks for the removed agent and provider values
  • the new contributor commands

Stories added: FeatureFlagsList, FeatureFlagsPageClient, SettingsRows, ProviderList, CloudProviderIcons.

Notes for the reviewer

  • Recap data in the workflow: the recap use case reads recognition events from the local database. In Actions that database starts empty, which matches what the committed recaps/ files already show (zero events), since recognition is still recorded by hand (see CONTRIBUTING). The stale-issues job reads GitHub and works as is.
  • Migration number: 166 is the next free number on main today. If the telemetry or unified-decisions PRs merge first, I'll merge main and renumber.
  • CLI name: shep security enforce keeps its name. Moving it under shep aspm would have broken existing CI scripts.

Checklist

  • pnpm lint passes
  • pnpm format:check passes
  • pnpm typecheck passes
  • pnpm test:unit and pnpm test:int pass
  • pnpm build succeeds
  • pnpm build:storybook succeeds and every new component has a colocated .stories.tsx
  • pnpm tsp:compile ran and output.ts is committed
  • Tests landed RED-first
  • No domain/ or application/ file imports from infrastructure/
  • Conventional Commits; feat/fix for user-visible changes
  • LESSONS.md updated: the flag checklist now points at the catalog, plus a note that agent worktrees are not gitignored

🤖 Generated with Claude Code

https://claude.ai/code/session_01TxtN2VcWzg6NTCLj4DbmfL

claude and others added 8 commits October 9, 2026 09:38
The stale good-first-issue and monthly recap watchers (spec 097) ran inside
every user's daemon. They are Shep-maintainer automation, so they now run
from .github/workflows/contributor-maintenance.yml through two new
subcommands, `shep contributors stale-issues` and `shep contributors recap`,
which reuse the existing use cases. The watcher services are deleted.

The contributor view (leaderboard, contributor doctor) moves from
/onboarding to /contributors, linked from CONTRIBUTING.md and not from the
sidebar. /onboarding keeps only the collaboration tutorial that the
supervisor and agents pages link to, and leaves the sidebar.

Claude-Session: https://claude.ai/code/session_01TxtN2VcWzg6NTCLj4DbmfL

Co-Authored-By: Shep Bot <shep-agent@users.noreply.github.com>
ROADMAP said ASPM ships behind a flag, but the flag defaulted to on.
ASPM is a separate product category, so new installs now start with
featureFlags.aspm = false (TypeSpec default, defaults factory and the web
env fallback). Values users already persisted are left as they are: no
migration touches feature_flag_aspm.

The e2e suites that visit /aspm turn the flag on from Settings for the
test and restore it afterwards.

Claude-Session: https://claude.ai/code/session_01TxtN2VcWzg6NTCLj4DbmfL

Co-Authored-By: Shep Bot <shep-agent@users.noreply.github.com>
Supply-chain security had its own `supplyChainSecurity` flag and overlapped
ASPM. It now has no flag of its own: isSupplyChainSecurityEnabled() derives
it from featureFlags.aspm, and the feature-agent pre-check, the canvas
security badge, the Settings section and `shep security enforce` all use
it. SHEP_SUPPLY_CHAIN_SECURITY still overrides it for CI: "false" turns it
off and "true" turns it on, which Shep's own security-enforce job already
sets, so the release gate keeps running on a fresh install.

The field leaves TypeSpec, the defaults, the mapper and the repository SQL.
The feature_flag_supply_chain_security column stays (NOT NULL DEFAULT 1)
so older builds keep reading it; no data is dropped.

Claude-Session: https://claude.ai/code/session_01TxtN2VcWzg6NTCLj4DbmfL

Co-Authored-By: Shep Bot <shep-agent@users.noreply.github.com>
Nothing read `system.autoUpdate`, so it leaves TypeSpec, the defaults and
the mapper. Its sys_auto_update column is NOT NULL with no default, so the
mapper keeps writing 1 (the old default) and never reads it; older builds
still see their usual value.

The Aider and Continue agent types were "coming soon" placeholders with no
executor. They leave the AgentType enum, the agent catalog, the canvas
icons, the TUI picker translations and the stories. A settings row that
still holds either value reads back as the default agent, so no data is
rewritten.

Claude-Session: https://claude.ai/code/session_01TxtN2VcWzg6NTCLj4DbmfL

Co-Authored-By: Shep Bot <shep-agent@users.noreply.github.com>
Specs 120–132 shipped with no flag. Each software-factory area now has one,
default on so nothing changes for users: spaces, trackers, knowledge,
signals, opportunities, feedback, discovery, incidents, outcomes, docsFirst,
autopilot and factory (TypeSpec, migration 166 with DEFAULT 1, mapper,
repository SQL, defaults, web flag state).

Each flag gates its area: sidebar entries, pages (requireFeaturePage),
POST /api/feedback and /api/alerts (404 while off), CLI groups
(gateByFeatureFlag hides the group and says how to turn it on; shep aspm
now uses it too), the daemon's sync, discovery, outcome and autopilot
passes (checked on every pass), and the docs-first planning instructions
and merge gate.

A domain catalog gives every flag a group and a one-line description.
ListFeatureFlagsUseCase and SetFeatureFlagUseCase serve both
/settings/feature-flags (every flag, its default and a switch; linked from
Settings) and `shep settings flags [enable|disable <flag>]`. The Settings
page's Feature Flags section renders the same catalog-driven list instead
of hand-written rows, which also adds the missing githubImport switch.

Claude-Session: https://claude.ai/code/session_01TxtN2VcWzg6NTCLj4DbmfL

Co-Authored-By: Shep Bot <shep-agent@users.noreply.github.com>
Cloudflare Pages was the only cloud deploy provider that worked. Vercel, Netlify,
AWS Amplify and GCP Cloud Run were stubs listed as "Coming soon" that threw on
connect and deploy. This removes them. Cloudflare Pages works as before.

- TypeSpec: CloudDeploymentProvider keeps only CloudflarePages (output.ts and
  apis/json-schema regenerated)
- core: delete the four *.provider.stub.ts files, base-provider-stub.ts, their DI
  registrations and ProviderNotImplementedError; drop `enabled` from
  ICloudDeploymentProvider, the registry descriptor and ListCloudProvidersUseCase
- the registry now lists only ids that have an adapter, and resolves them from the
  container it was registered in rather than the global root
- add domain parseCloudDeploymentProvider, replacing four copies of parseProvider
  in the CLI commands and web routes
- CLI: `shep app cloud-providers ls/connect` lose the coming-soon output
- web: drop the "Coming soon" rows, disabled states and removed brand icons from
  ProviderList, ProviderDropdown, DeployPanel, DeployButton, SmartDeployCluster
  and ConnectProviderModal; share labels and the list entry type in
  cloud-providers.ts; update stories, add ProviderList and CloudProviderIcons
  stories

Persisted values: connect rejected the stubs, so no cloud_provider_tokens row
can hold one, but `select-provider` and `shep app deploy start --provider` could
store a stub id in applications.cloud_deployment_provider. No migration rewrites
data. The application mapper reads an unknown provider id as no selection, so
deploy falls back to the first connected provider. Token listing skips unknown
ids too.

Claude-Session: https://claude.ai/code/session_01TxtN2VcWzg6NTCLj4DbmfL

Co-Authored-By: Shep Bot <shep-agent@users.noreply.github.com>
Co-Authored-By: Shep Bot <shep-agent@users.noreply.github.com>
@arielshad arielshad changed the title Review cleanup: move maintainer automation off users' machines, ASPM off by default, per-area flags feat(web): review cleanup — per-area feature flags, ASPM off by default Oct 9, 2026
claude and others added 3 commits October 9, 2026 11:41
Specs 133 and 134 are taken by the telemetry and unified-decisions workstreams.
Comments and test names that cite the spec follow the new number.

Co-Authored-By: Shep Bot <shep-agent@users.noreply.github.com>
The view's group headings jumped from h1 to h3; they are h2 on the page and stay
h3 inside the Settings section. The phone navigation test opens the ASPM Security
group, so it turns the aspm flag on first now that it defaults to off.

Co-Authored-By: Shep Bot <shep-agent@users.noreply.github.com>
git 2.56 on the Windows runner rejects GIT_CONFIG_GLOBAL=NUL, so every harness
real-git test failed in git init. Git for Windows maps /dev/null to its null device,
as the merge-step real-git tests already rely on.

Co-Authored-By: Shep Bot <shep-agent@users.noreply.github.com>

Copy link
Copy Markdown
Contributor Author

Unit Tests (windows-latest) failed on 8c0e469 in tests/integration/infrastructure/services/harness/tools/builtin-tools.test.ts. Every git init in the harness test helper failed with fatal: unable to access 'NUL': Invalid argument.

The cause is not this PR's code. The Windows runner now has git 2.56.0, which rejects GIT_CONFIG_GLOBAL=NUL, and tests/helpers/harness/temp-git-repo.ts sets exactly that. The last CI run on main (Oct 5) passed on an older git, but main will hit the same failure on the new runner image.

9264d3c fixes it here by using /dev/null on every platform. Git for Windows maps that path to its null device, and the merge-step real-git tests already use it and pass on this same runner. The harness integration tests pass locally on Linux; Windows CI will confirm.


Generated by Claude Code

Copy link
Copy Markdown
Contributor Author

Unit Tests (windows-latest) failed on 9264d3c in one test only: pty-terminal-session.service.test.ts › "creates terminal successfully with valid working directory". It failed with EBUSY: resource busy or locked, rmdir …\shep-test-3qKjXS during teardown, after its assertions had passed. The other 14,622 tests passed, and the harness git fix from the previous comment worked.

I don't think this PR causes it. The PR does not touch the terminal service or that test, and the same unit step passed on Windows on the previous commit (8c0e469). The test already waits for the shell to exit and then retries rmdir for about 3 s (removeDirWithRetry). The runner log ends with "Terminate orphan process: pid (1732) (conhost)", so the ConPTY host was still holding the temp directory after that window.

I tried to re-run the failed job and got a 403, because this integration cannot re-run workflows. Could someone with access re-run "Unit Tests (windows-latest)"?

If it keeps happening, my proposed fix (not applied here, to keep this PR's scope) is to make that test's teardown tolerate a lingering ConPTY handle on Windows. The temp directory lives under the OS temp folder and does not affect any assertion:

} finally {
  try {
    removeDirWithRetry(tempDir);
  } catch (error) {
    // Windows: conhost can outlive the shell and keep tempDir open.
    if (process.platform !== 'win32' || (error as NodeJS.ErrnoException).code !== 'EBUSY') throw error;
  }
}

Generated by Claude Code

On Windows the conhost.exe that node-pty starts with the shell inherits its cwd and
can outlive the shell's exit event, so the pty test's teardown failed with EBUSY even
after waiting for exit and retrying. The test now leaves that temp dir to the OS on
that one Windows error; any other error still fails it.

Co-Authored-By: Shep Bot <shep-agent@users.noreply.github.com>

Copy link
Copy Markdown
Contributor Author

Unit Tests (windows-latest) failed again on 4d1b86f, this time with a different cause. vitest's esbuild transform service on the Windows runner died about 25 seconds into the job (13:33:31Z). After that, every test file that had not loaded yet failed with Error: The service is no longer running (vite:esbuild): 1,232 files failed to load, while all 2,827 tests that did run passed. No assertion failed.

No test in this PR's diff ran before the service died, and the terminal test fixed in 4d1b86f was among the files that never loaded. Before this run, Windows unit tests had completed with only one failure in total (the teardown fixed in 4d1b86f). Every other job on 4d1b86f is green.

This needs a re-run of Unit Tests (windows-latest). This integration gets a 403 when it tries to re-run workflows, so could someone with access re-run it? If esbuild dies again, it is reproducible and I will dig into what is taking the transform service down.


Generated by Claude Code

Copy link
Copy Markdown
Contributor Author

I looked into whether this PR causes the esbuild crash in Unit Tests (windows-latest) on 4d1b86f, and found no link to its diff.

  • When the service died. test:unit started at 13:33:06Z, and the first file reporting (0 test) appeared at 13:33:31Z. Before that point 68 test files had loaded and run.
  • The PR's own tests that ran before the crash all passed. Four of the 68 files are in this PR's diff:
    • settings.mapper.test.ts (132 tests)
    • agent-executor-factory.test.ts (62)
    • cloud-deploy-use-cases.test.ts (22)
    • settings-defaults.factory.test.ts (42)
  • The log shows nothing unusual before the crash. I checked from the job start to 13:33:31 for a crash, an out-of-memory error or an esbuild error, and found none. The last lines are normal ✓ results (status.command.test.ts, ensure-infra.node.test.ts, start-daemon stdout). The only process killing nearby, in deployment.service.test.ts, goes through an injected deps.kill mock.
  • Nothing in the diff targets the test runner or kills processes. The PR does not change any vitest or vite config, pool setting, test setup file or package.json. None of its new modules spawn processes or do heavy work at import time.
  • The same code passed this job on earlier commits. Earlier Windows unit runs on this PR (b4e7420, 8c0e469, 9264d3c) loaded and ran all 1,301 files. The only code change since 9264d3c is the teardown in pty-terminal-session.service.test.ts, and that file never loaded in this run (it is among the (0 test) files).

I conclude that the esbuild service on the runner died in the middle of the run, and that nothing in this PR caused it. A re-run of Unit Tests (windows-latest) should confirm it: https://github.com/shep-ai/shep/actions/runs/37937489099. This integration cannot re-run it (403). If it fails the same way again, it is reproducible and I will chase it.


Generated by Claude Code

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants