feat: Phase 01 packets 7+8 — DX orchestrator + CI baseline - #3
Merged
Conversation
…ooks + e2e)
Phase 01 packet 7 wires the developer experience the prior packets all
depended on but did not ship:
- `Makefile` at the repo root with `dev` / `down` / `clean` / `logs` /
`ps` / `e2e-up` / `e2e-down` / `build` / `test` / `lint` / `format` /
`typecheck` / `seed` / `install` / `hooks`. The `install` target also
activates the git hooks via `git config core.hooksPath .githooks` so a
fresh clone is one command away from the project standards.
- `.env.example` at the repo root is the single source of truth for dev
credentials. `infra/compose/dev.yml` reads them through `${VAR:-default}`
interpolation with dev-safe fallbacks (so the stack still boots if no
`.env` exists). The Dapr Vault secret-store component reads the same
`VAULT_ROOT_TOKEN` via Dapr's `{{env.VAR}}` substitution — closing the
long-standing two-file token duplication the dapr README flagged for
Phase 07. A narrower `frontend/apps/web/.env.local.example` mirrors the
Next.js-side vars (Next reads `.env.local` from the app dir, not the
repo root).
- `.githooks/pre-commit` runs `dotnet format` on staged `*.cs`, prettier
on staged `*.{ts,tsx,js,jsx,mjs,cjs,json,md}`, and ESLint --fix on
staged JS-likes. YAML is excluded (compose / Dapr / APISIX YAMLs are
comment-heavy and prettier reflows them in a way that hurts review
readability). Activated by `make install` so a developer does not have
to remember `git config core.hooksPath` themselves.
- `infra/compose/e2e.yml` is the end-to-end overlay — swaps the named
volumes for tmpfs (postgres, seaweedfs, meilisearch, kafka) and tunes
Mailpit retention for ephemeral test runs. Images, ports, and
credentials match dev exactly so a Phase 11 bump in `dev.yml` flows
through without a second edit.
The compose README documents the new `make`-driven workflow, the e2e
overlay, and points at `.env.example` as the source of truth. The dapr
README's "Phase 07 commitment" note is replaced with the actual chain
description now that the work landed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Phase 01 packet 8 closes the phase:
- `.github/workflows/ci.yml` runs on every push to `main` and every PR.
Three jobs gate merges and are listed verbatim in
`.github/CONTRIBUTING.md` § Branch protection so GitHub Settings
matches the corpus:
- backend: dotnet format verify + Release build (TreatWarningsAsErrors
via CI=true) + unit + architecture + contract tests (integration
excluded — Testcontainers job is scaffolded as `if: false` and
activates in Phase 02a when the first integration test lands).
- frontend: pnpm install --frozen-lockfile + typecheck + lint + build
+ Vitest.
- meta: changed-Markdown broken-link sweep + a tightened
`docs/analysis/` residual scan (matches only `](docs/analysis/…)`
Markdown link targets and `from/require('docs/analysis/…)` code
imports, so legitimate meta-references in CLAUDE.md / standards /
roadmap pass cleanly).
Three jobs are scaffolded-but-deferred (`if: false`) so the activation
is a one-line flip in the owning phase: backend-integration (Phase
02a), openapi-diff (Phase 03), lighthouse-budget (Phase 04). Concurrency
cancels stale runs on the same ref; permissions are restricted to
`contents: read`.
- `scripts/seed.sh` (invoked by `make seed`) verifies the compose stack
is healthy, polls the OIDC discovery endpoint for each realm, and
prints the demo identities. The application-level tenant seeding is a
documented one-edit drop-in (the exact `dotnet run --project
LearnStack.Tools.Seeder` invocation is in the script) waiting on the
Phase 02a Tenancy DbContext — Phase 01 has no schema to write against.
- `.github/CONTRIBUTING.md` documents the branch-protection rules in
prose so the GitHub Settings page can be audited against the corpus
(required checks, approval count, signed-commits posture, no force-
push to main, no bypass). The commit-message + PR conventions cross-
link to CLAUDE.md instead of duplicating them.
- Status update: `docs/roadmap/phase-01-repository-tooling.md` marks
packets 7 + 8 ✅; CLAUDE.md and README.md flip the project status to
"Phase 01 complete" and surface the `make install / dev / seed` quick-
start. Next phase: 02a (Platform Kernel + Multi-Tenancy), with 02c
(Hub Foundation, separate repo) running in parallel.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ors)
Three review agents walked the branch in parallel. Aggregated findings,
validated each against the current code, applied every still-valid fix.
Blockers (both fixed; tests reproduced both bugs locally):
- Makefile `.ONESHELL:` makes the cwd of `cd backend && …` leak into
the next recipe line, so `make install` (cd backend → restore; cd
frontend → install) blows up on GNU Make 4.x (the macOS default 3.81
silently no-ops `.ONESHELL:` and hides the bug). Wrapped every
recipe that crosses subdirs in `(cd X && …)` subshells.
- The Dapr Vault component used `{{env.VAULT_ROOT_TOKEN}}` template
syntax — Dapr does not support that in component metadata. Switched
to the canonical pattern: new `secretstore-envvar.yaml` registers a
`secretstores.local.env` component named `envvar-secrets`; the Vault
component declares `auth.secretStore: envvar-secrets` and resolves
`vaultToken` via `secretKeyRef: { name: VAULT_ROOT_TOKEN, key: VAULT_ROOT_TOKEN }`.
daprd substitutes the literal at component-load time from the process
env that compose already passes through. The single-source-of-truth
chain is preserved (`.env` → compose env → dapr env → secretKeyRef).
Majors:
- Pre-commit hook re-staged formatted files via `git add` — silently
capturing any WIP unstaged hunks the developer was holding back
(classic lint-staged trap). Now stashes unstaged + untracked
changes with `git stash push --keep-index` and pops in an EXIT trap;
formatters only see indexed content.
- CI Markdown-link audit regex `\]\(\.\.?/[^)]+\)` only matched `./` and
`../` prefixes — bare-relative `[X](docs/foo.md)` links (the project's
convention per CLAUDE.md) slipped past. Broadened to capture every
non-anchor target; explicit external-scheme skip (http/https/mailto/
tel/ftp); anchor + query suffixes stripped before existence check;
resolves bare-relative against repo root.
- `.env.example` `SEAWEEDFS_ACCESS_KEY=learnstack-dev` did not match
`infra/seaweedfs/s3-identities.json` `accessKey: learnstack`. The env
var was unwired (SeaweedFS reads the JSON directly) but a future
storage-adapter consumer would have authenticated against an unknown
key. Aligned to `learnstack`; added a comment explaining the var must
match the JSON until Vault rotates both.
- Per Standards 20 § Secrets Management ("pre-commit hook scans for
high-entropy strings; CI fails on hits"), neither the hook nor the
workflow ran a secret scanner. Added `gitleaks protect --staged` to
the pre-commit hook (optional — warns and continues if gitleaks is
not on PATH; CI is the hard gate); added a `secret-scan` job to the
workflow using `gitleaks/gitleaks-action@v2`. `.gitleaks.toml` carries
the explicit dev-credential allowlist with one entry per intentional
in-repo literal and a "why" + production rotation path.
Minors:
- `cancel-in-progress: true` would have cancelled in-flight `main`
builds on back-to-back merges. Made it conditional on
`${{ github.event_name == 'pull_request' }}` so main builds always
finish.
- CI `docs/analysis/` residual scan did not cover `*.js` / `*.jsx` and
missed the dynamic `import("docs/analysis/…")` shape Next.js routes
use. Extended both lists.
- `.gitignore` `!*.example` was an unbounded un-ignore that would auto-
track any future `foo.example` file. Replaced with the two explicit
template paths.
- `Makefile` `.env: .env.example` recipe re-fired on every invocation
after a rebase shifted the example's mtime. Switched to `cp -n` +
`touch .env` for true idempotency.
- `Makefile` `test-integration` invoked `dotnet test
LearnStack.Tests.Integration` (not a valid project path from
`backend/`). Fixed to the actual `tests/.../*.csproj` path.
- `.githooks/pre-commit` had a dead `fmt_files=("${fmt_files[@]/#}")`
line whose comment claimed it stripped empties — it did not (a no-op
for non-empty elements, no-op for empties). The real filter is the
subsequent for-loop; removed the dead line + corrected the comment.
- `CLAUDE.md` status block read "Phase 01 complete … domain bodies are
empty" which a skimmer could parse as contradiction. Tightened to
"Phase 01 complete — repository scaffolding, local infrastructure, DX,
and CI baseline. No domain code yet — Phase 02a starts that."
- `infra/compose/README.md` still had a "What this file does NOT bring
up yet" section listing packets 7+8 as deferred. Replaced with the
remaining true deferrals (the .NET API host move, livekit-egress,
otel-collector, application-level seeding, production Vault).
Acknowledged (NOT fixed in this commit; cost > value or out of scope):
- Packet 7's `make seed` target references `scripts/seed.sh` that lands
in packet 8 — independent reviewability is reduced. Will collapse on
squash-merge to main; rebase-merge would keep the two commits and the
one-commit-only-checkout would have a dangling target. Branch is not
yet pushed, so future-me may amend; for now this commit explicitly
documents the dependency.
- Packet 7's commit subject is 82 characters (the convention is ≤72).
Same trade-off: amending past two commits is more disruption than
the violation merits when squash-merge is in play.
- seed.sh does not actively verify the demo users exist in each realm
(only OIDC discovery). Adding password-grant probes would require
token-handling complexity that belongs in Phase 02b. Deferred.
Verification: `docker compose -f infra/compose/dev.yml config -q` ✓ on
both dev and dev+e2e overlay; `make help` lists every target; `python3
yaml.safe_load` on the CI workflow + both Dapr component YAMLs ✓;
`bash -n` on the pre-commit hook and seed script ✓.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…sion `infra/compose/e2e.yml` uses Compose's `!reset []` override-merge tag on four service `volumes:` lists (lines 33, 52, 58, 68) to drop the parent `dev.yml`'s named-volume entries before adding a `tmpfs:` mount at the same target. Docker Compose accepts this — `docker compose config -q` exits 0 — but the redhat.vscode-yaml extension's generic YAML parser does not know `!reset` and flags every occurrence as an unknown tag. `yaml.customTags` in `.vscode/settings.json` is the upstream-documented way to declare the tags so the extension treats them as valid. Added `!reset` + `!override` in all four shape variants (sequence / mapping / scalar / untyped) per the extension's tag-suffix convention. Also added `redhat.vscode-yaml` and `ms-azuretools.vscode-docker` to `.vscode/extensions.json` recommendations so a fresh clone gets the schema-aware compose tooling without a manual extension hunt. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer's GuideImplements the Phase 01 DX orchestrator and CI baseline by adding a repo-root Makefile, environment scaffolding, pre-commit hooks, e2e compose overlay, CI workflow with backend/frontend/meta/secret-scan jobs, a seed script, and gitleaks configuration, while wiring Dapr Vault token handling through a single .env-based source of truth and updating docs and editor settings to reflect Phase 01 completion. Flow diagram for DX orchestrator commandsflowchart LR
dev[Developer] --> M1[make install]
M1 --> H[Git config core.hooksPath .githooks]
M1 --> D1[dotnet restore LearnStack.slnx]
M1 --> F1[pnpm install --frozen-lockfile]
dev --> M2[make dev]
M2 --> E[.env rule<br/>copy .env.example → .env]
M2 --> C1[docker compose -f infra/compose/dev.yml up -d]
dev --> M3[make e2e-up]
M3 --> E
M3 --> C2[docker compose -f dev.yml -f e2e.yml up -d]
dev --> M4[make seed]
M4 --> C1
M4 --> S[ scripts/seed.sh ]
S --> SH1[Check dev stack health]
S --> SH2[Verify Keycloak realms learnstack & learnstack-hub]
S --> SH3[Print demo credentials<br/>+ Phase 02a seeding deferral]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughPhase 01 delivers repo DX: env templates and gitignore, Makefile dev workflows, pre-commit leakwatch + format hooks, GitHub Actions CI baseline, Docker Compose dev/e2e paramaterization, Dapr→Vault envvar secret indirection, a seed script for health/realm checks, backend editorconfig/analyzer updates, frontend tsconfig, and docs marking Phase 01 complete. ChangesPhase 01 Complete - Repository Tooling & DX
🎯 4 (Complex) | ⏱️ ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RepoEnv as Repo `.env.example`
participant DockerCompose as Docker Compose / vault service
participant DaprSidecar as dapr-sidecar-api (process env)
participant EnvVarStore as Dapr component envvar-secrets
participant VaultComp as Dapr Vault secretstore
RepoEnv->>DockerCompose: `.env` copied / compose up reads VAULT_ROOT_TOKEN`
DockerCompose->>DaprSidecar: inject VAULT_ROOT_TOKEN into dapr-sidecar-api env
DaprSidecar->>EnvVarStore: envvar-secrets reads process env (VAULT_ROOT_TOKEN)
VaultComp->>EnvVarStore: secretKeyRef -> request VAULT_ROOT_TOKEN
EnvVarStore-->>VaultComp: return VAULT_ROOT_TOKEN
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
scripts/seed.shhealth check can be flaky right aftermake devbecause it exits immediately ifdocker compose psshows no running services; consider treatingstarting/unhealthyas a transient state and polling until either all services are healthy or a timeout elapses, instead of failing early with the "Runmake devfirst" message. - In
scripts/seed.sh, the Keycloak realm readiness probe only waits for the tenant realm but fails immediately for the hub realm, which can still be booting; mirroring the retry/timeout loop for both realms would make seeding more robust on cold starts.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `scripts/seed.sh` health check can be flaky right after `make dev` because it exits immediately if `docker compose ps` shows no running services; consider treating `starting`/`unhealthy` as a transient state and polling until either all services are healthy or a timeout elapses, instead of failing early with the "Run `make dev` first" message.
- In `scripts/seed.sh`, the Keycloak realm readiness probe only waits for the tenant realm but fails immediately for the hub realm, which can still be booting; mirroring the retry/timeout loop for both realms would make seeding more robust on cold starts.
## Individual Comments
### Comment 1
<location path="infra/compose/dev.yml" line_range="370-374" />
<code_context>
- # this service definition AND ../dapr/components/secretstore-vault.yaml.
- # If you change one you MUST change the other. Phase 07 (DX) wires both
- # to a single `.env.example` source so the duplication goes away.
+ # Single source of truth: `VAULT_ROOT_TOKEN` from `.env.example`. The Dapr
+ # Vault secret-store component (`infra/dapr/components/secretstore-vault.yaml`)
+ # reads the same env var through Dapr's `{{env.VAULT_ROOT_TOKEN}}` template
+ # substitution, so changing the token in `.env` updates every consumer in
+ # lockstep (Phase 07 DX commitment per `infra/dapr/README.md` § Vault token).
</code_context>
<issue_to_address>
**suggestion:** The comment references `{{env.VAULT_ROOT_TOKEN}}` templating, but the actual wiring uses `secretKeyRef` + `secretstores.local.env`.
This block should describe the actual wiring now used: `secretstore-vault.yaml` reads the token via `secretKeyRef` and the `secretstore-envvar.yaml` component (`auth.secretStore: envvar-secrets`), not `{{env.VAULT_ROOT_TOKEN}}`. Updating the comment to reflect the `secretKeyRef` + `envvar-secrets` chain will keep it aligned with the implementation and avoid confusion when debugging secret flow.
```suggestion
# Single source of truth: `VAULT_ROOT_TOKEN` from `.env.example`. In dev,
# this compose file injects the token into Vault via the `-dev-root-token-id`
# flag. In k8s, the Dapr Vault secret-store component
# (`infra/dapr/components/secretstore-vault.yaml`) reads the token from a
# Kubernetes Secret via `secretKeyRef`, and that Secret is populated from the
# `secretstores.local.env` / `secretstore-envvar.yaml` component
# (`auth.secretStore: envvar-secrets`). Updating `VAULT_ROOT_TOKEN` in `.env`
# keeps Vault and all Dapr consumers in lockstep (Phase 07 DX commitment per
# `infra/dapr/README.md` § Vault token).
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request completes Phase 01 by establishing the repository scaffolding, local infrastructure, developer experience (DX) tools, and CI baseline. Key additions include a root-level Makefile for task orchestration, a pre-commit hook for automated formatting and secret scanning, and an ephemeral Docker Compose overlay for E2E testing. Infrastructure configurations for Dapr and Vault were refactored to utilize a single source of truth for credentials. Review feedback identified an unused variable and potential logic issues in the pre-commit hook's stashing mechanism.
Comment on lines
+42
to
+43
| needs_stash=$(git status --porcelain | awk '$1 !~ /^M$|^A$|^D$|^R$|^C$|^\?\?$/ { found=1 } END { print found+0 }') | ||
| unstaged_changes=$(git diff --quiet || echo "yes") |
There was a problem hiding this comment.
The variable needs_stash is calculated here but never used in the script. Additionally, the awk logic as written might not correctly distinguish between staged and unstaged changes because awk trims leading whitespace by default, making $1 identical for both M (staged) and M (unstaged) statuses. Since you already have robust checks for unstaged changes on line 43, this line can be removed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
59-60: 💤 Low valueConsider adding
persist-credentials: falseto checkout actions.Static analysis (zizmor) flags that checkout actions persist credentials by default, which could leak through artifacts or subsequent steps. Adding
persist-credentials: falseis a security hardening measure. Since this workflow doesn't need to push commits, credentials aren't required post-checkout.🛡️ Suggested hardening (apply to all checkout steps)
- name: Checkout uses: actions/checkout@v4 + with: + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 59 - 60, The Checkout step (uses: actions/checkout@v4) currently leaves credentials persisted; update that step (and any other checkout steps) to include persist-credentials: false so the checkout action does not retain Git credentials for later steps or artifacts — add the persist-credentials: false key under the Checkout step configuration to harden the workflow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 197-202: Replace direct template expansion inside the shell block
by exporting GitHub inputs into environment variables and referencing those
variables in the script: when handling pull_request, set an environment variable
(e.g., TARGET_BASE_REF) to the template value for
github.event.pull_request.base.ref and use that variable when assigning base and
in git fetch; similarly, set TARGET_BASE_REF to github.event.before in the
non-pull_request branch and use "$TARGET_BASE_REF" in the base assignment and
git fetch invocation so no template string is directly expanded in the shell
context.
In @.gitleaks.toml:
- Around line 21-57: The current .gitleaks.toml uses broad [[allowlist]] entries
that whitelist entire file paths/dirs (e.g., the regex patterns like
'''^infra/compose/dev\.yml$''', '''^infra/dapr/components/.*\.ya?ml$''', and
others), which can suppress all detectors for those files; update the allowlist
to scope exceptions to specific known dev literals or rule IDs instead of
full-file allowlists — replace broad path entries with value-scoped regexes or
add the specific rule names/ids to each allowlist item (targeting the literal
strings used for dev credentials) so that only those exact literals/rules are
ignored while other secret rules still run on the referenced files.
In `@docs/roadmap/phase-01-repository-tooling.md`:
- Around line 62-63: The sentence mentioning the deferred `learnstack-hub`
compose overlay must name its owning phase: update the line referencing "The
`learnstack-hub` compose overlay remains deferred (lives in the separate
`learnstack-hub` repo per ADR-0019)" to explicitly state the owning phase (e.g.,
"owned by the Hub roadmap phase" or the specific phase name/id that tracks
`learnstack-hub`), so the deferment is unambiguous and follows the guideline;
ensure the phrase includes the exact phase identifier and retains the ADR-0019
reference for traceability.
- Around line 66-67: Summary: The roadmap text lists "three required jobs" in
`.github/workflows/ci.yml` but omits the `secret-scan` job required by Phase 01.
Fix: update the sentence that enumerates required CI jobs so it includes
`secret-scan` (e.g., change "three required jobs" to "four required jobs" or
remove the numeric count and add `secret-scan` alongside the existing backend
and frontend descriptions), and ensure the example job list explicitly names
`secret-scan` to match branch-protection and Phase 01 scope.
In `@infra/compose/dev.yml`:
- Around line 39-43: The Postgres healthcheck currently hardcodes "learnstack"
user/db causing false failures when POSTGRES_USER/POSTGRES_DB are overridden;
update the healthcheck command to use the same Compose variable substitutions
used for service envs (e.g., replace the fixed 'learnstack' occurrences in the
healthcheck with ${POSTGRES_USER:-learnstack} and ${POSTGRES_DB:-learnstack}) so
pg_isready (or the existing test) checks the parametrized user and database
consistent with the POSTGRES_USER/POSTGRES_DB settings.
- Around line 370-374: The comment wrongly states Dapr uses a
`{{env.VAULT_ROOT_TOKEN}}` template; update the text to reflect the actual
indirection: note that `VAULT_ROOT_TOKEN` is the single source of truth and that
the Dapr component `secretstore-vault.yaml` uses `auth.secretStore:
envvar-secrets` with `secretKeyRef` indirection (not `{{env...}}`) so describe
that the Vault token is read via the `envvar-secrets` secret store and
`secretKeyRef` mapping; mention `VAULT_ROOT_TOKEN`, `secretstore-vault.yaml`,
and `auth.secretStore: envvar-secrets` in the comment so future maintainers see
the correct wiring.
In `@infra/dapr/README.md`:
- Around line 106-107: Update the phase label text "Phase 07 (DX)" to the
correct phase identifier used in this PR and docs—e.g., "Phase 01 (packet
7/8)"—in the README line that mentions the Vault root token (the sentence
containing "VAULT_ROOT_TOKEN" and "Phase 07 (DX)"); ensure the surrounding
sentence still reads correctly and matches other documentation references to
Phase 01 packet 7/8.
In `@scripts/seed.sh`:
- Around line 65-69: The one-shot curl check for the hub realm
(KEYCLOAK_REALM_HUB / realm 'learnstack-hub') is flaky; replace it with the same
bounded retry loop used for the other realm discovery so the script retries with
a timeout instead of failing immediately. Locate the curl check that targets
"$KEYCLOAK_URL/realms/$KEYCLOAK_REALM_HUB/.well-known/openid-configuration" and
wrap it in the same retry logic (max attempts, sleep between attempts, and a
final error+exit) used for the learnstack realm check, preserving the existing
red error messages when the retries exhaust. Ensure you reference
KEYCLOAK_REALM_HUB and the 'learnstack-hub' check so the change mirrors the
other realm's retry behavior.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 59-60: The Checkout step (uses: actions/checkout@v4) currently
leaves credentials persisted; update that step (and any other checkout steps) to
include persist-credentials: false so the checkout action does not retain Git
credentials for later steps or artifacts — add the persist-credentials: false
key under the Checkout step configuration to harden the workflow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fe2ea46a-a616-4465-94a2-52205bc5a154
📒 Files selected for processing (20)
.env.example.githooks/pre-commit.github/CONTRIBUTING.md.github/workflows/ci.yml.gitignore.gitleaks.toml.vscode/extensions.json.vscode/settings.jsonCLAUDE.mdMakefileREADME.mddocs/roadmap/phase-01-repository-tooling.mdfrontend/apps/web/.env.local.exampleinfra/compose/README.mdinfra/compose/dev.ymlinfra/compose/e2e.ymlinfra/dapr/README.mdinfra/dapr/components/secretstore-envvar.yamlinfra/dapr/components/secretstore-vault.yamlscripts/seed.sh
Three review agents + the CI run on commit e0b7ae7 surfaced a mix of review nits and three genuine CI breakages. Aggregated and addressed together. CI failures (3): - backend job failed at restore: Testcontainers' transitive Azure.Identity 1.3.0 / Microsoft.IdentityModel.JsonWebTokens 6.8.0 / System.Drawing.Common 5.0.0 emit NU190x vulnerability warnings that flip to errors under `TreatWarningsAsErrors` (CI=true). Set `<NuGetAuditMode>direct</…>` in Directory.Build.props so audit applies to OUR direct deps only; transitive vulnerabilities from a test-only library no longer fail the build. Phase 02a's integration packet picks up Testcontainers properly and may reconsider. - frontend lint failed: `Cannot read file '.../node_modules/tsconfig.base.json'`. The chain was `apps/web/tsconfig.json → @learnstack/config/tsconfig/next.json → ./base.json → ../../../tsconfig.base.json`. Under pnpm's symlinked node_modules, the relative `../../../` resolved against the symlink destination's apparent parents, landing in `apps/web/node_modules/` where no `tsconfig.base.json` exists. Inlined the base-config content into `frontend/packages/config/tsconfig/base.json` (with a comment documenting the source) so the chain stays inside the @learnstack/config package and never escapes upward. - secret-scan job failed: gitleaks 8.24 surfaced `'Allowlist' expected a map, got 'slice'` because `[[allowlist]]` (singular, repeated) is no longer a valid 8.x shape. Renamed to `[[allowlists]]` (plural) per current docs. Review comments (11): - ci.yml: script-injection hardening — `github.event.pull_request.base.ref` and `github.event.before` now pass through `env:` block as `PR_BASE_REF` / `PUSH_BEFORE_SHA`, never inline `${{ … }}` inside the `run:` shell. Defense-in-depth: those values aren't user-controlled today, but the pattern keeps every step consistent. - ci.yml: added `persist-credentials: false` to all four `actions/checkout@v4` steps so the credential helper does not leak into downstream steps or artifacts. - .gitleaks.toml: replaced broad path-based `[[allowlist]]` entries (which suppressed EVERY detector on listed files — a real AWS key in `dev.yml` would have slipped through) with regex-scoped allowlists per known dev literal (Vault root token, Keycloak admin / demo passwords, LiveKit dev key + 32-byte secret, Coturn user / pass, SeaweedFS dev secret, Meilisearch master key, Kafka cluster id). Each entry cites WHY the literal is in-repo and which production path replaces it. The two `.env*.example` paths remain path-allowed by intent (they document credential SHAPES; never carry real values). - roadmap phase-01: "three required jobs" → "four" so the doc matches the four jobs branch protection enforces (backend / frontend / meta / secret-scan). - roadmap phase-01: the deferred `learnstack-hub` compose overlay now cites its actual owning phase — "the separate `learnstack-hub` repo's Phase 02c per ADR-0019" — instead of just "deferred (lives in separate repo)". - roadmap phase-01: replaced the stale `{{env.VAULT_ROOT_TOKEN}}` claim with the actual `secretKeyRef` + `secretstore-envvar.yaml` (`auth.secretStore: envvar-secrets`) chain. - dev.yml: `pg_isready` was hardcoded `-U learnstack -d learnstack`, so overriding `POSTGRES_USER` / `POSTGRES_DB` via `.env` would have made the healthcheck false-fail. Switched to `$${POSTGRES_USER:-learnstack}` / `$${POSTGRES_DB:-learnstack}` — Compose-escaped so the container shell evaluates against the container env at runtime, which is set from the same compose vars as the environment block. - dev.yml: two stale `{{env.VAULT_ROOT_TOKEN}}` comments (vault service block + dapr-sidecar-api env block) replaced with the actual `secretKeyRef` + `envvar-secrets` indirection narrative. - dev.yml: Kafka section's "Phase 07 (DX) ships … EXTERNAL listener" wording rewritten — Phase 07 in this project is Enrollment + Learner Portal, not DX; the EXTERNAL-listener question is a Phase 11 production-hardening item, not in scope here. Phase 01 packet 7 shipped the kafka-ui-only workflow as canonical. - dapr/README.md: "Phase 07 (DX)" → "Phase 01 packet 7 (DX)" — same terminology fix. - seed.sh: hub realm probe was one-shot; if the hub realm import finished a few seconds after the tenant realm, the script failed immediately. Extracted both realm checks into a `wait_for_realm()` helper that runs the same bounded retry loop. Step 1's compose health check was also fragile — `make seed: dev` brings the stack up immediately before, so most services are `starting` when the script hits them; rewrote to POLL for healthy until the timeout, with the "literally no services running" case the only immediate-fail. - .githooks/pre-commit: removed the dead `needs_stash=$(git status … | awk …)` probe whose result was never read AND whose awk logic could not reliably distinguish staged from unstaged (awk trims leading whitespace, so `M ` and ` M` collapse). The two checks immediately below (`git diff --quiet` and `git ls-files --others`) cover every WIP shape. Verification: `docker compose config -q` ✓ both files; `python3 yaml.safe_load` on ci.yml ✓; `bash -n` on both shell scripts ✓; `tsc --showConfig` from `apps/web` now resolves the full chain ✓. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per the project owner, the secret scanner of record is Leakwatch (github.com/cemililik/Leakwatch, v1.5.0) — the team's own MIT-licensed scanner with verifier coverage on 53/63 detectors, hybrid Aho-Corasick + regex + entropy engine, YAML custom rules, single-binary install. Gitleaks was the placeholder picked under "any vetted scanner satisfies Standards 20" but the team has its own well-tested tool. CI: - `.github/workflows/ci.yml` § secret-scan now runs `actions/setup-go@v5` + `go install github.com/cemililik/leakwatch@v1.5.0` + `leakwatch scan fs . --config .leakwatch.yaml --format sarif --output results.sarif --min-severity medium --no-verify`. The CLI invocation is inline (not the wrapper action) so the version pin + verification posture stay explicit. `--no-verify` keeps CI hermetic — dev credentials are entropy-filtered, real production secrets never reach the repo. SARIF results upload as an artifact for offline inspection. - Branch-protection required check renamed `secret scan (gitleaks)` → `secret scan (leakwatch)` in `.github/CONTRIBUTING.md`. Config: - New `.leakwatch.yaml` — entropy threshold 4.2 (slightly more selective than the 4.0 default so short low-entropy dev literals like `admin-dev-secret` don't fire), verification disabled, exclude-paths for node_modules / build artifacts / lock files / minified assets / docs/analysis. The three layers of intentional-credential handling (entropy filter → `.leakwatchignore` → inline `# leakwatch:ignore`) are documented in the file header. - New `.leakwatchignore` — path entries for the env templates, the LiveKit + Coturn confs (the 32-byte padded `devsecret` is intentionally high-entropy to satisfy LiveKit's secret-length requirement), the SeaweedFS S3 identity JSON, and the Keycloak realm seeds. Each entry cites WHY the literal is in-repo and which production path replaces it. - `.gitleaks.toml` deleted; the regex/path-allowlist hybrid it carried is no longer relevant. Pre-commit: - `.githooks/pre-commit` swaps the optional `gitleaks protect --staged` call for `leakwatch scan fs <file>` per staged file (Leakwatch has no `--staged` flag, so we iterate). Same on-PATH-or-skip pattern: hook warns and continues if `leakwatch` is missing, CI re-runs the same scan as the hard gate. The "install: brew install cemililik/tap/leakwatch" hint replaces the gitleaks one. Docs: - `.github/CONTRIBUTING.md` § Local checks updated with the Leakwatch install snippets + the three-tier "intentional dev credential" handling recipe (inline ignore > .leakwatchignore > config tweak). - `docs/roadmap/phase-01-repository-tooling.md` packet 8 description names Leakwatch v1.5.0 explicitly with a link to the upstream repo. - `Makefile` `hooks` target message updated. Verification: `python3 yaml.safe_load` on ci.yml + .leakwatch.yaml ✓; `bash -n` on the pre-commit hook ✓. The Leakwatch action is intentionally NOT used; the inline `go install` keeps the version pin auditable and avoids the wrapper action's `latest` default. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Phase 01 packets 1-6 backend scaffold + frontend monorepo predate
the CI workflow this PR adds, so the FIRST run of CI surfaced four
pre-existing analyzer violations and one missing vitest flag. Addressing
them all here so the secret-scan + format gates we just added can
actually go green.
Backend (CA1515, CA2234, CA1034 in dotnet format --verify-no-changes):
- `LearnStack.Api/Program.cs` carried `public partial class Program;` so
WebApplicationFactory<Program> in the test assemblies could resolve
the type. CA1515 (application types should not leak as `public` when
no external consumer needs them) flagged it. Switched to
`internal partial class Program;` and added
`<InternalsVisibleTo Include="LearnStack.Tests.Contract" />` +
`<InternalsVisibleTo Include="LearnStack.Tests.Integration" />` to
`LearnStack.Api.csproj` so the test assemblies still see it.
- `OpenApiContractTests.cs` nested a `Factory` class inside the test
class (CA1034: do not nest types) AND declared it `public` (CA1515)
AND called `client.GetAsync("/openapi/v1.json")` with a string
overload (CA2234: prefer the `Uri` overload). Extracted the factory
to a top-level `internal sealed class DevelopmentWebApplicationFactory`
in its own file; the test class now uses `IClassFixture<…>` against
that top-level type. The GetAsync call goes through a static
`Uri(string, UriKind.Relative)` cached at type init.
- `SmokeTests.cs` had the same CA2234 (string overload) — same fix
pattern (static Uri readonly field, GetAsync(Uri) overload).
Frontend (Vitest exits 1 when no test files match):
- `apps/web/package.json` `test` script was `vitest run`; with no
test files in the scaffold yet, Vitest exits 1 by default. Changed
to `vitest run --passWithNoTests` so the CI `pnpm -r test` step
passes until Phase 02a starts adding component tests, at which
point the flag becomes redundant but harmless.
Verification: locally pinned to .NET 9 (global.json wants 10.0.100),
cannot reproduce the CA-rule pass locally; pushing for CI to verify.
The fixes match the canonical recipes documented by the Roslyn
analyzer rule pages.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Round 6 took the failure count from 3 → 2; this round closes both.
Backend (CA1812):
- `DevelopmentWebApplicationFactory` is `internal` and instantiated by
xunit through `IClassFixture<T>` reflection — the Roslyn analyzer
cannot see that callsite and flagged the class as dead code. Added
`[SuppressMessage("Performance", "CA1812", Justification = "…xunit
IClassFixture<T> reflection…")]` with the rationale inline, so a
future reader sees why the suppression is correct (not arbitrary).
Leakwatch (4 findings):
- `.leakwatchignore` itself was being scanned, and the SeaweedFS comment
quoted the literal `learnstack-dev-secret` to explain WHICH dev cred
the entry covered — generic-api-key detector matched the literal in
the comment and flagged the ignore file. Rewrote the comment to
describe the credential without quoting it, so the file no longer
self-flags.
- `docs/architecture/27-custom-domain-tls.md` and
`docs/decisions/0022-custom-domain-tls.md` (×2) carry illustrative
`-----BEGIN ... PRIVATE KEY-----` blocks explaining the
custom-domain TLS flow per ADR-0022. They are documentation
examples, never live keys; production keys are Let's-Encrypt-issued
and live in Vault. Added both paths to `.leakwatchignore` with the
rationale.
Verification: locally cannot run leakwatch (binary not installed) or
dotnet 10 (global.json pin), pushing for CI to verify. The CA1812
suppression pattern is the canonical Roslyn recipe; the .leakwatchignore
changes are purely additive path entries + a comment rewrite that
removes the self-match.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Round 7 surfaced two more analyzer violations that the format-verify step hadn't reached before: - `LearnStack.SharedKernel/Results/Error.cs` CA1716: type named `Error` conflicts with a reserved keyword in VB. LearnStack is C#-only and the Result+Error pattern (FluentResults / Ardalis.Result lineage) requires the canonical name; renaming to e.g. `ResultError` would diverge from every reference in ADR-0032. Suppressed with the rationale. - `LearnStack.SharedKernel/Results/Result.cs` CA1000 (×2): static `Result<T>.Ok(value)` and `Result<T>.Fail(error)` factory members on the generic type. The non-generic alternative (`Result.Ok<T>(value)`) forces every callsite to repeat the type argument the type-inferrer already knows — bad ergonomics for the most-used handler-return pattern in the codebase. Suppressed with the rationale. Both suppressions carry inline docstrings citing ADR-0032 § Error Model so a future reader sees why the rule is off (not arbitrary). These are pre-existing scaffold types from Phase 01 packets 1-6; this PR surfaces them only because it's the first CI run against the TreatWarningsAsErrors-under-CI build configuration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… fix 2 real code bugs
Round 8 surfaced 11 backend errors that fall into three buckets:
1. CS0051 — my round-6 attempt at CA1515 "fix" backfired. Making
`Program` internal + `DevelopmentWebApplicationFactory` internal broke
the public test class constructors (a public ctor cannot take an
internal parameter type). Reverted both back to `public`. The CA1515
suppression now lives on Program via `#pragma warning disable` with a
clear rationale (xunit's WebApplicationFactory<Program> cannot see
internal types reliably even with InternalsVisibleTo). The
`InternalsVisibleTo` entries in `LearnStack.Api.csproj` are removed
(no longer needed).
2. Test-inappropriate analyzer rules (CA1707, CA1812, CA1515, CA1034,
CA2234) flagging 7+ test methods. xunit test code has its own
conventions: `Method_When_Returns` naming (CA1707), reflection-based
instantiation (CA1812), public classes for runner discovery (CA1515),
nested theory-data types (CA1034), and string-overload HTTP calls
(CA2234). Added them to `<NoWarn>` in `backend/Directory.Build.props`
under the existing `IsTestProject` condition, with inline
documentation explaining why each is suppressed in test scope and
confirming CA1305 (culture-invariant) + CA1861 (static-readonly
array) STAY ON.
3. Two real code-quality bugs in the architecture-test project:
- `RepositoryLayoutTests.cs:55` CA1861: `BeEquivalentTo(new[] { "web" })`
allocated a fresh array per test run. Extracted to a `static readonly
string[] AllowedFrontendApps = ["web"]` field at the class top.
- `ModuleDependencyTests.cs:77` CA1305: `string.Format(prefixTemplate,
moduleName)` defaulted to the runner's current culture. Switched to
`string.Format(CultureInfo.InvariantCulture, prefixTemplate, moduleName)`
so the same prefix resolves on every machine regardless of locale.
Pipeline state expected after this push: backend + frontend + meta +
secret-scan all green (the four required branch-protection checks).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… MSBuild NoWarn) Round 9 still failed because `dotnet format analyzers --verify-no-changes` invokes the Roslyn analyzers directly — it respects `.editorconfig` severity overrides but ignores the project's `<NoWarn>` MSBuild property. Build sees the NoWarn (which is why the Build step would have passed), but format-verify (which runs FIRST in the workflow) does not. Switched both suppression layers to `.editorconfig`: - `backend/src/LearnStack.Api/.editorconfig` — single rule: `dotnet_diagnostic.CA1515.severity = none`. Scopes the suppression to this project alone (the auto-generated `public partial class Program` for top-level statements). Comment explains why xunit's WebApplicationFactory<Program> requires the type to be public. - `backend/tests/.editorconfig` — five rules at `severity = none`: CA1707 (xunit underscore naming), CA1812 (xunit reflection instantiation), CA1515 (public test classes for runner discovery), CA1034 (nested theory-data types), CA2234 (HttpClient.GetAsync(string) in test assertions). CA1305 + CA1861 STAY ON. `backend/Directory.Build.props` keeps the `<NoWarn>` block too — belt-and-suspenders so future devs running `dotnet build` get the same suppressions the format-verify step now applies. `Program.cs` no longer needs `#pragma warning disable CA1515` (the editorconfig owns it). Removed the pragma, kept the explanatory comment. After this push, the four required CI jobs should be green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Phase 01 CI cycles 7-10 cost ~3 rounds chasing analyzer rules that fall
outside any standard the project has agreed to enforce — generic xunit
patterns, the auto-generated `public partial class Program`, the
FluentResults-style `Error` type name, etc. The pattern repeats every
time a real domain commit lands, so the policy is what needs adjustment,
not each downstream commit.
Switched `AnalysisMode` from `AllEnabledByDefault` (~450 rules — the
"everything Microsoft ships in the box") to `Recommended` (~120 rules —
the curated middle ground for production code: security, correctness,
reliability, maintainability defaults). The .NET team considers
`Recommended` the floor; everything beyond it is best-practice noise
that drowns out the signal.
The posture promotes a different philosophy:
- `Recommended` is the floor.
- Rules we SPECIFICALLY want as errors (security-sensitive CAs, our
own architecture-test contract, ADR-0032 cross-cutting analyzers)
get an explicit `dotnet_diagnostic.<id>.severity = error` line in
the relevant `.editorconfig`.
- Tests + Api keep their existing scope-overrides (CA1515 / CA1707 /
etc.) — those entries still apply because they are documented
suppressions, not consequences of the global mode.
Verified locally with .NET 10 SDK (10.0.300, global.json
`rollForward: latestFeature` resolves to it):
- `dotnet build LearnStack.slnx CI=true --configuration Release` →
0 warnings, 0 errors.
- `dotnet format LearnStack.slnx --verify-no-changes` → exit 0.
- `dotnet test ... --filter "!~Tests.Integration"` → 20 / 20 pass
(Unit: 2, Architecture: 17, Contract: 1).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
frontend/packages/config/tsconfig/base.json (1)
2-2: ⚡ Quick winConsider documenting the sync mechanism for the mirrored config.
The comment states that
frontend/tsconfig.base.jsonmirrors this file "for editor convenience only," but maintaining two copies manually creates a risk of divergence. Consider either:
- Adding a validation step (e.g., in CI or pre-commit) to ensure both files remain identical
- Documenting the sync process in CONTRIBUTING.md
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/packages/config/tsconfig/base.json` at line 2, The mirrored tsconfig comment (the "_comment" key) warns about a copied config but lacks guidance or validation; add a short section to CONTRIBUTING.md describing the sync process for the mirrored frontend tsconfig and required developer workflow, and implement a CI or pre-commit validation that runs a small check script which compares the two mirrored config files' contents and fails with a readable diff when they diverge; ensure the check script is referenced in CI config and pre-commit hooks so divergence of the "_comment"/mirrored config is detected automatically..githooks/pre-commit (1)
104-109: 💤 Low valueConsider showing leakwatch findings to help developers understand what triggered the failure.
When leakwatch detects a potential secret, the output is suppressed (
>/dev/null), so the developer only sees the filename but not what the scanner flagged. This makes it harder to determine whether it's a false positive requiring an ignore directive or a real issue.♻️ Suggested improvement to show findings
- if ! leakwatch scan fs "$f" --config .leakwatch.yaml --min-severity medium --no-verify >/dev/null; then - printf "\npre-commit: leakwatch found a likely secret in %s\n" "$f" >&2 + if ! leakwatch scan fs "$f" --config .leakwatch.yaml --min-severity medium --no-verify; then + printf "\npre-commit: leakwatch found a likely secret in %s (see above)\n" "$f" >&2🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.githooks/pre-commit around lines 104 - 109, The leakwatch invocation currently suppresses scanner output by redirecting to >/dev/null which hides what triggered the failure; update the block that runs leakwatch scan fs "$f" --config .leakwatch.yaml --min-severity medium --no-verify so that when it returns non-zero its stdout/stderr are preserved and printed to the developer (e.g., remove the >/dev/null redirection or capture output into a variable and echo it on failure), and ensure the script still returns exit 1 after printing the detailed leakwatch findings so the pre-commit hook blocks appropriately..github/workflows/ci.yml (1)
296-296: 💤 Low valueConsider pinning actions to commit SHAs for supply-chain security.
Static analysis flags that
actions/checkout@v4,actions/setup-go@v5, andactions/upload-artifact@v4in the secret-scan job use version tags rather than commit hashes. This applies to the entire workflow (all jobs use version tags). While version tags are common practice and provide automatic patch updates, pinning to commit SHAs provides stronger supply-chain guarantees per security best practices.This is a project-wide decision — if you adopt hash pinning, apply it consistently across all actions and consider using a tool like Dependabot or Renovate to manage updates.
Also applies to: 302-302, 320-320
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml at line 296, The workflow uses version tags for third-party actions (e.g., actions/checkout@v4, actions/setup-go@v5, actions/upload-artifact@v4) which the reviewer recommends pinning to commit SHAs for supply-chain security; update every occurrence of those action references in the workflow to the corresponding commit SHA (replace `@vX` with the specific full git SHA for the release you want to pin), apply this change consistently across all jobs including the secret-scan job, and consider adding tooling (Dependabot/Renovate) or comments documenting the update process so future updates use SHA pins.docs/roadmap/phase-01-repository-tooling.md (2)
218-227: ⚡ Quick winClarify deferred items in the CI Baseline scope.
The CI Baseline scope lists integration tests (line 222), OpenAPI breaking-change check (line 224), and Lighthouse budget check (line 225), but the Status section (lines 77-78) states these are scaffolded-but-deferred with phase ownership noted. Consider adding inline notes to the Scope section indicating these items are deferred, or removing them entirely to match the final delivered scope.
📝 Suggested clarification
- GitHub Actions workflow. - Backend build and unit + architecture + contract tests. -- Integration tests with Testcontainers PostgreSQL. +- Integration tests with Testcontainers PostgreSQL (deferred to Phase 02a). - Frontend install, typecheck, build, lint, component tests. -- OpenAPI breaking-change check. -- Lighthouse budget check on representative public pages. +- OpenAPI breaking-change check (deferred to Phase 03). +- Lighthouse budget check on representative public pages (deferred to Phase 04). - Required status checks on `main`.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/roadmap/phase-01-repository-tooling.md` around lines 218 - 227, Update the "CI Baseline" section to match the Status note by either marking the three items as deferred or removing them: specifically annotate "Integration tests with Testcontainers PostgreSQL", "OpenAPI breaking-change check", and "Lighthouse budget check on representative public pages" with a short inline note like "(scaffolded — deferred, see Status)" or delete those bullet items so the Scope and Status are consistent; ensure the "CI Baseline" heading text remains unchanged so references to that section still resolve.
202-203: ⚡ Quick winClarify that the
learnstack-huboverlay does not live in this repository.The wording "Optional
learnstack-hubcompose overlay for local Hub development" may suggest the overlay is present in this repository. The Status section (lines 65-67) explicitly states it "never lives here" and is owned by the separatelearnstack-hubrepo per ADR-0019. Consider rephrasing to clarify the overlay is external and consumed from the separate repository.📝 Suggested clarification
-- Optional `learnstack-hub` compose overlay for local Hub development (depends on - the same Keycloak / Postgres / Kafka / Vault / APISIX stack). +- Integration with the `learnstack-hub` compose overlay for local Hub development + (overlay lives in the separate `learnstack-hub` repository per ADR-0019; depends on + the same Keycloak / Postgres / Kafka / Vault / APISIX stack from this repo).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/roadmap/phase-01-repository-tooling.md` around lines 202 - 203, The line "Optional `learnstack-hub` compose overlay for local Hub development" is ambiguous about location; update that sentence to state the overlay is external and maintained in the separate learnstack-hub repository (per ADR-0019). Specifically, change the fragment referencing the `learnstack-hub` compose overlay so it reads something like "Optional external `learnstack-hub` compose overlay (maintained in the separate learnstack-hub repository per ADR-0019) for local Hub development" to make ownership and location explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/seed.sh`:
- Around line 50-52: The health check collects only .Health so services without
healthchecks (empty .Health) are ignored; update the docker compose ps
formatting and the awk test used in the not_healthy assignment (and the
identical block later) to include .State (e.g., --format
'{{.Name}}\t{{.Health}}\t{{.State}}') and treat empty Health or a non-running
State as unhealthy (awk condition: $2 != "healthy" || $3 != "running" or $2 ==
"" ). Ensure you change the variable assignment for not_healthy and the
corresponding loop/conditional in the other block (the same code duplicated
later) so both use the new fields and logic.
---
Nitpick comments:
In @.githooks/pre-commit:
- Around line 104-109: The leakwatch invocation currently suppresses scanner
output by redirecting to >/dev/null which hides what triggered the failure;
update the block that runs leakwatch scan fs "$f" --config .leakwatch.yaml
--min-severity medium --no-verify so that when it returns non-zero its
stdout/stderr are preserved and printed to the developer (e.g., remove the
>/dev/null redirection or capture output into a variable and echo it on
failure), and ensure the script still returns exit 1 after printing the detailed
leakwatch findings so the pre-commit hook blocks appropriately.
In @.github/workflows/ci.yml:
- Line 296: The workflow uses version tags for third-party actions (e.g.,
actions/checkout@v4, actions/setup-go@v5, actions/upload-artifact@v4) which the
reviewer recommends pinning to commit SHAs for supply-chain security; update
every occurrence of those action references in the workflow to the corresponding
commit SHA (replace `@vX` with the specific full git SHA for the release you want
to pin), apply this change consistently across all jobs including the
secret-scan job, and consider adding tooling (Dependabot/Renovate) or comments
documenting the update process so future updates use SHA pins.
In `@docs/roadmap/phase-01-repository-tooling.md`:
- Around line 218-227: Update the "CI Baseline" section to match the Status note
by either marking the three items as deferred or removing them: specifically
annotate "Integration tests with Testcontainers PostgreSQL", "OpenAPI
breaking-change check", and "Lighthouse budget check on representative public
pages" with a short inline note like "(scaffolded — deferred, see Status)" or
delete those bullet items so the Scope and Status are consistent; ensure the "CI
Baseline" heading text remains unchanged so references to that section still
resolve.
- Around line 202-203: The line "Optional `learnstack-hub` compose overlay for
local Hub development" is ambiguous about location; update that sentence to
state the overlay is external and maintained in the separate learnstack-hub
repository (per ADR-0019). Specifically, change the fragment referencing the
`learnstack-hub` compose overlay so it reads something like "Optional external
`learnstack-hub` compose overlay (maintained in the separate learnstack-hub
repository per ADR-0019) for local Hub development" to make ownership and
location explicit.
In `@frontend/packages/config/tsconfig/base.json`:
- Line 2: The mirrored tsconfig comment (the "_comment" key) warns about a
copied config but lacks guidance or validation; add a short section to
CONTRIBUTING.md describing the sync process for the mirrored frontend tsconfig
and required developer workflow, and implement a CI or pre-commit validation
that runs a small check script which compares the two mirrored config files'
contents and fails with a readable diff when they diverge; ensure the check
script is referenced in CI config and pre-commit hooks so divergence of the
"_comment"/mirrored config is detected automatically.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f11f186a-538a-4550-99dc-b84b8a71659a
📒 Files selected for processing (23)
.githooks/pre-commit.github/CONTRIBUTING.md.github/workflows/ci.yml.leakwatch.yaml.leakwatchignoreMakefilebackend/Directory.Build.propsbackend/src/LearnStack.Api/.editorconfigbackend/src/LearnStack.Api/Program.csbackend/src/LearnStack.SharedKernel/Results/Error.csbackend/src/LearnStack.SharedKernel/Results/Result.csbackend/tests/.editorconfigbackend/tests/LearnStack.Tests.Architecture/ModuleDependencyTests.csbackend/tests/LearnStack.Tests.Architecture/RepositoryLayoutTests.csbackend/tests/LearnStack.Tests.Contract/DevelopmentWebApplicationFactory.csbackend/tests/LearnStack.Tests.Contract/OpenApiContractTests.csbackend/tests/LearnStack.Tests.Integration/SmokeTests.csdocs/roadmap/phase-01-repository-tooling.mdfrontend/apps/web/package.jsonfrontend/packages/config/tsconfig/base.jsoninfra/compose/dev.ymlinfra/dapr/README.mdscripts/seed.sh
✅ Files skipped from review due to trivial changes (12)
- frontend/apps/web/package.json
- backend/tests/.editorconfig
- backend/tests/LearnStack.Tests.Contract/DevelopmentWebApplicationFactory.cs
- .leakwatchignore
- backend/src/LearnStack.SharedKernel/Results/Result.cs
- backend/src/LearnStack.SharedKernel/Results/Error.cs
- backend/src/LearnStack.Api/.editorconfig
- backend/tests/LearnStack.Tests.Architecture/ModuleDependencyTests.cs
- backend/tests/LearnStack.Tests.Architecture/RepositoryLayoutTests.cs
- backend/src/LearnStack.Api/Program.cs
- .github/CONTRIBUTING.md
- infra/dapr/README.md
Cleaned every finding the post-CI review flagged. No behavioural change;
the substantive work in this PR was already green on round 11.
Majors (stale narratives in onboarding-first files):
- CLAUDE.md (line 36): pre-commit "gitleaks" → "Leakwatch". CLAUDE.md
is the entry file; a stale tool name here misleads every new
contributor on first read.
- docs/roadmap/phase-01-repository-tooling.md: packet-7 hook
description still cited `gitleaks protect --staged`; packet-8 two
lines below correctly cited Leakwatch. Rewrote packet-7 to match
the actual hook + cross-linked install instructions to
`.github/CONTRIBUTING.md`.
- .env.example: two comment blocks still claimed Dapr substitutes
`{{env.VAULT_ROOT_TOKEN}}` template — Dapr does not support that
syntax, the chain runs through `secretKeyRef` + the local-env
secret store. Rewrote both blocks to mirror the four-step chain
the infra/dapr/README.md and infra/compose/dev.yml comments
describe.
Minors:
- backend/Directory.Build.props: dropped citation of non-existent
"ADR-0032 § Cross-cutting analyzers / Standards 17 § Code Review"
for the AnalysisMode = Recommended choice. The policy is genuinely
un-anchored in the corpus today; replaced the bad citation with a
note that the first ADR-0032 amendment in Phase 02a should pin
this posture explicitly.
- Standards-20 vs Standards-12 citation mix-up (4 places — was 3 +
ci.yml: the "pre-commit hook scans for high-entropy strings"
policy lives in Standards 12 § Secrets Management, not Standards
20 § ISecretProvider. Retargeted in .leakwatch.yaml,
.githooks/pre-commit, docs/roadmap/phase-01-repository-tooling.md,
AND .github/workflows/ci.yml.
- .leakwatchignore: added the five infra/**/README.md paths
(compose, dapr, keycloak, livekit, seaweedfs) that mirror dev
credentials inline for orientation tables. Symmetry with the
YAML/conf files they document; the literals scan clean today
(entropy < 4.2 threshold) but the inconsistency was rot-bait.
- frontend/tsconfig.base.json deleted. The repo-root mirror existed
only because of the pnpm-symlink relative-extends bug round-5
surfaced; once that was fixed by inlining the config into
`frontend/packages/config/tsconfig/base.json`, the mirror became
drift bait. Nothing in the tree extends the deleted file (verified
via grep over `**/*.json`); the packages-config copy is now sole
source of truth. Also dropped the `_comment` field (tsc ignores
it; the rationale lives in this commit message + git blame).
- backend/src/LearnStack.Api/.editorconfig: tightened CA1515 glob
from `[*.cs]` to `[Program.cs]`. Phase 02a wires IExceptionHandler
+ MediatR pipeline + IErrorTrackingProvider adapters into this
assembly per ADR-0032 § Composition Root; the wider glob would
have exempted every one of those types. Narrow glob keeps CA1515
gating everything except the top-level-statements Program type
that xunit's WebApplicationFactory<Program> needs public.
- .githooks/pre-commit: collapsed the duplicate `staged_files()`
invocation. Single walk fills the per-language buckets AND the
flat `all_staged` array for the Leakwatch loop in one pass —
one fewer `git diff --cached` per commit.
- docs/standards/12-infrastructure.md § Image Conventions:
clarified that "pinned by digest" is the PRODUCTION rule;
dev compose uses explicit version tags (`:1.2.3`, never
`:latest`) because per-bump digest pinning is operationally
heavy and the re-push risk for the images we use is vanishingly
low. Documents what `infra/compose/dev.yml` already does.
Verification: local .NET 10 (SDK 10.0.300) build CI=true → 0
warning/0 error; `dotnet format --verify-no-changes` exit 0;
20/20 tests pass; both compose configs validate; no orphan
`frontend/tsconfig.base.json` references; no remaining
"Standards 20 § Secret" citations.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Round 12 meta job failed on three legitimate sibling-relative links inside `docs/standards/12-infrastructure.md` (`20-infrastructure-stack.md`, `10-observability.md`). Both files exist alongside the source — but the prior audit logic treated any link without a `./` or `../` prefix as repo-relative, missing the Markdown-default sibling-relative case. Fixed by checking BOTH resolutions before flagging broken — a link that resolves either way is ok. Same code path now covers: [X](foo.md) → sibling-relative (Markdown default) [X](./foo.md) → current-dir explicit [X](../foo.md) → parent-dir explicit [X](docs/foo.md) → repo-relative (CLAUDE.md § Cross-link convention) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… clarity One inline + three nitpicks from the review. The fourth nitpick (frontend tsconfig _comment field) was already closed in 1a5f245. Inline (scripts/seed.sh): - The health-check loop captured only `.Health`; a service with no healthcheck (empty Health column) was treated as already-healthy. All current dev.yml services carry a healthcheck per Standards 12, but the policy should be enforced not assumed. Rewrote the awk to consume `{{.Name}}\t{{.State}}\t{{.Health}}` and flag three distinct failure shapes — service not running, service running but with no healthcheck defined, service running but healthcheck not yet `healthy`. Each shape prints its own diagnostic so the developer can see whether to wait or to fix a missing healthcheck. Nitpick (.githooks/pre-commit): - Leakwatch failure path piped scanner output to `/dev/null`, leaving the developer guessing which detector fired. Now captures stdout + stderr, replays it before exiting 1 — exit code drives the gate, scanner output drives the diagnosis. Nitpicks (docs/roadmap/phase-01-repository-tooling.md): - The CI Baseline section listed Integration / OpenAPI-diff / Lighthouse as plain bullets, contradicting the in-progress status block above that calls them deferred. Each now carries an inline *(scaffolded as `if: false` placeholder; activates in Phase 0Xx when …)* note so Scope and Status read coherently. - "Optional `learnstack-hub` compose overlay for local Hub development" was ambiguous about ownership — could be read as "lives in this repo." Rewritten to "Optional **external** `learnstack-hub` compose overlay (maintained in the separate `learnstack-hub` repository per ADR-0019)" so location + ownership are unambiguous. Skipped: - Action SHA pinning (.github/workflows/ci.yml): genuine supply- chain hardening recommendation, but maintaining SHA pins without Renovate / Dependabot is operationally heavy for a Phase 01 PR. Track for Phase 11 production-hardening — the addition there bundles SHA pinning + a Renovate config in one change. The current `@v4` / `@v5` major-tag pins are the GitHub-recommended Phase-01-grade posture. Verification: `bash -n` clean on both shell scripts; `python3 yaml.safe_load` clean on ci.yml; roadmap links resolve under the new sibling-relative + repo-relative auditor. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
302-308:⚠️ Potential issue | 🔴 Criticalleakwatch package at
github.com/cemililik/leakwatch@v1.5.0could not be verified as publicly available.Go version 1.25 is valid and supported by
setup-go@v5. However, web search found no public repository forgithubproxy.fjygbaifeng.eu.org/cemililik/leakwatchor v1.5.0 release. If this is a private package, ensure it is accessible to the CI environment (may require authentication or hosted on a private Go module proxy). If the package path or version is incorrect, update accordingly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 302 - 308, The "Install Leakwatch" CI step runs "go install github.com/cemililik/leakwatch@v1.5.0" but that module/version cannot be verified as public; update the workflow to either point to a valid public module/version (replace the module path/version in the "Install Leakwatch" run command), remove the step if not needed, or configure CI to access a private module (set GOPRIVATE/GONOPROXY or add authentication/secrets and a private Go proxy before the "Install Leakwatch" run). Ensure the change targets the "Install Leakwatch" step and its go install invocation so the action can successfully fetch the dependency.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 302-308: The "Install Leakwatch" CI step runs "go install
github.com/cemililik/leakwatch@v1.5.0" but that module/version cannot be
verified as public; update the workflow to either point to a valid public
module/version (replace the module path/version in the "Install Leakwatch" run
command), remove the step if not needed, or configure CI to access a private
module (set GOPRIVATE/GONOPROXY or add authentication/secrets and a private Go
proxy before the "Install Leakwatch" run). Ensure the change targets the
"Install Leakwatch" step and its go install invocation so the action can
successfully fetch the dependency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08e38c1e-e78d-4e05-9530-7cf03a406992
📒 Files selected for processing (13)
.env.example.githooks/pre-commit.github/workflows/ci.yml.leakwatch.yaml.leakwatchignoreCLAUDE.mdbackend/Directory.Build.propsbackend/src/LearnStack.Api/.editorconfigdocs/roadmap/phase-01-repository-tooling.mddocs/standards/12-infrastructure.mdfrontend/packages/config/tsconfig/base.jsonfrontend/tsconfig.base.jsonscripts/seed.sh
💤 Files with no reviewable changes (2)
- frontend/tsconfig.base.json
- frontend/packages/config/tsconfig/base.json
✅ Files skipped from review due to trivial changes (4)
- docs/standards/12-infrastructure.md
- .leakwatch.yaml
- CLAUDE.md
- .env.example
7 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.
Summary
Closes Phase 01 by shipping the remaining two packets:
After merge, `docs/roadmap/phase-01-repository-tooling.md` marks every packet ✅ and `CLAUDE.md` / `README.md` flip the project status to "Phase 01 complete." Next: Phase 02a (Platform Kernel + Multi-Tenancy) and Phase 02c (Hub Foundation, parallel, separate repo).
Commits (4)
```
e0b7ae7 chore(vscode): declare Compose !reset / !override tags for YAML extension
e239ecf fix: address phase-01 packets 7+8 review (2 blockers, 5 majors, 6 minors)
03ddffb feat(ci): Phase 01 packet 8 — CI baseline + seed orchestrator
6e843a3 feat(infra): Phase 01 packet 7 — DX orchestrator (Makefile + .env + hooks + e2e)
```
Stats: 18 files, +1207/-92.
Test plan
🤖 Generated with Claude Code
Summary by Sourcery
Finalize Phase 01 by adding a repo-root developer orchestrator, CI baseline, and Dapr/Vault secret handling improvements, and by updating documentation and VS Code settings to reflect the completed phase.
New Features:
scripts/seed.shhelper to validate local stack health, verify Keycloak realm imports, and surface demo credentials..github/CONTRIBUTING.mdto codify branch-protection rules, required checks, and PR/commit hygiene..env.exampleand frontend.env.local.exampleas the single source of truth for dev configuration.Bug Fixes:
Enhancements:
make-based workflows and the e2e overlay in the compose README, including guidance for raw Docker Compose usage whenmakeis unavailable.CI:
ciworkflow that runs backend build/tests and format verification, frontend typecheck/lint/build/tests, metadata checks (link audit and docs/analysis guard), and gitleaks-based secret scanning on pushes and pull requests.Documentation:
.github/CONTRIBUTING.mdcovering branch protection rules, required CI checks, commit conventions, and local verification steps.Tests:
Chores:
Summary by CodeRabbit
New Features
Chores
.envexample as single source of truth and refine compose/dev startup behavior.Documentation