docs: add AGENTS.md contributor and agent conventions#120
Conversation
|
On it! I'm picking up PR #120 and will review the AGENTS.md documentation for correctness and rule-compliance, then post a consolidated review. |
There was a problem hiding this comment.
Thanks for adding AGENTS.md — a contributor/agent guide like this is genuinely useful, and most of it matches the codebase. I ran a per-rule compliance check (PR title is Conventional Commits, opened ready-for-review) and verified the doc against the actual source. Three factual inaccuracies would actively mislead an agent/contributor following the doc, so requesting changes before merge:
- Feature-flags path (L52) — file is
src/lib/feature-flags.ts($lib/feature-flags), notsrc/lib/server/feature-flags.ts. - CSP location (L77) — CSP lives in
svelte.config.js(kit.csp.reportOnly), nothooks.server.ts. - Coverage gate (L86) — no
--coverage, no provider dep, no threshold invite.config.ts, no CI coverage step; the gate does not exist.
Everything else (logger/request-id, /health vs /readyz, migration-safety db:check, business_ table prefix, Yivi/IRMA session model, i18n locale-leak gotcha, /irma proxy) checks out. Fix the three above and this is good to merge.
| ## Config | ||
|
|
||
| - Read env via `$env/dynamic/private` (server) — never hardcode. | ||
| - **Feature flags** are `FF_*` env vars resolved in `src/lib/server/feature-flags.ts`; |
There was a problem hiding this comment.
Wrong feature-flags path. This says FF_* flags resolve in src/lib/server/feature-flags.ts, but the file is src/lib/feature-flags.ts — every import in the repo uses $lib/feature-flags (no server/ segment), and there is no file under src/lib/server/feature-flags.ts. An agent following this can't find the file, and it contradicts the "server-only code lives in src/lib/server/" convention stated earlier in the same doc.
| - **Demo vs prod attributes**: `YIVI_DEMO_ATTRIBUTES=true` uses the `irma-demo` | ||
| scheme; unset uses the `pbdf` production scheme. | ||
| - **API keys** are SHA-256-hashed; the plaintext prefix is shown once at creation. | ||
| - Security headers + a report-only **CSP** are set in `hooks.server.ts`; CSP |
There was a problem hiding this comment.
CSP location is wrong. The report-only CSP is configured in svelte.config.js (kit.csp.reportOnly, with report-uri: /api/csp-report), not in hooks.server.ts. hooks.server.ts only sets the non-CSP headers (X-Frame-Options, X-Content-Type-Options, Referrer-Policy, Permissions-Policy). An agent looking for the CSP in hooks.server.ts won't find it. (The /api/csp-report report-uri claim itself is correct.)
| - Unit tests: `tests/unit/**` (and colocated `*.test.ts`), node env. Mock | ||
| server deps with `vi.mock` + `vi.hoisted` (see `tests/unit/dns-verification.test.ts`). | ||
| - E2E: `tests/e2e/**/*.e2e.ts` (Playwright). CI runs both against a real Postgres. | ||
| - Coverage is gated (`vitest --coverage`) over `src/lib/**` (migrations excluded); |
There was a problem hiding this comment.
Fabricated coverage gate. This claims coverage is gated via vitest --coverage over src/lib/** with a floor in vite.config.ts. In reality: vite.config.ts has no coverage/thresholds block, package.json's test:unit is plain vitest (no --coverage) with no @vitest/coverage-* provider dependency, and CI runs no coverage step. vitest --coverage would fail for lack of an installed provider, and there is no floor to "keep above". Either wire up the gate or drop this claim.
|
Hey @rubenhensen, I left a REQUEST_CHANGES review on this PR — found three accuracy issues in the new AGENTS.md that should be fixed before merging:
Everything else looks good — the PR title is Conventional Commits compliant and the PR is correctly marked ready-for-review. Happy to re-review once these are addressed! |
|
Fixed all three factual issues in ad228e7:
Thanks for the catch — all three would've mis-directed anyone following the doc. /dobby ready for re-review. |
|
On it, @rubenhensen! I'm picking up the re-review of #120 now and will post a consolidated review with my findings shortly. |
There was a problem hiding this comment.
Re-review after ad228e7. Verified all three fixed claims against the checked-out branch — feature-flags path (src/lib/feature-flags.ts / $lib/feature-flags), CSP location (kit.csp.reportOnly in svelte.config.js → /api/csp-report, non-CSP headers in hooks.server.ts), and the dropped coverage-gate claim — all correct now. Ran a per-rule compliance pass (conventional-commit title docs: ✓, no public security disclosure ✓, CSP report-uri present ✓) and re-checked ~20 other doc claims against the codebase; all accurate.
One remaining factual inaccuracy of the same class as the three just fixed: the Gotchas note claims the coverage/ directory is git-ignored, but it is not (git check-ignore coverage → exit 1; absent from both .gitignore and .prettierignore). See inline comment. Requesting changes so this last one is corrected before merge — everything else is good.
| locale is carried on `event.locals.locale` and applied in `+layout.ts`. | ||
| - The IRMA/Yivi server is reached only through the server proxy `/irma/[...path]` | ||
| (which injects the auth token); the browser never talks to it directly. | ||
| - A leftover `coverage/` directory can trip `prettier --check` locally — it's |
There was a problem hiding this comment.
Factual inaccuracy of the same class as the three just fixed in ad228e7. The Gotchas note says the leftover coverage/ directory "can trip prettier --check locally — it's git-ignored". It is not git-ignored: coverage/ appears in neither .gitignore nor .prettierignore, and git check-ignore coverage returns exit 1 (not ignored). The "can trip prettier --check" part is true, but the "it's git-ignored" clause is false — and it actively contradicts the following "don't commit it" advice, since a non-ignored directory is exactly what gets committed by accident.
Fix, either:
- add
coverage/to.gitignore(and.prettierignore) and keep the wording, or - drop the "it's git-ignored" clause.
|
Re-reviewed after ad228e7 — three of the four issues are confirmed fixed: the feature-flags path, CSP config location, and the coverage-gate claim are all accurate now. Nice work on those. One thing still needs attention: says is git-ignored, but it isn't — it's absent from both and , so the adjacent advice to not commit it has no enforcement. I've left an inline comment suggesting either adding to both ignore files or dropping the clause entirely. Review: #120 (review) |
|
Fixed the last one in the latest commit: dropped the false "it's git-ignored" clause. The |
|
On it, @rubenhensen! I'm picking up the re-review of this PR now and will post a consolidated review shortly. |
There was a problem hiding this comment.
✅ Re-review complete — the coverage/ note fix looks good.
The latest commit drops the false "it's git-ignored" clause; the note now just warns that a leftover coverage/ dir can trip prettier --check locally and says to delete it / not commit it — accurate regardless of the #117 merge order.
Compliance: ran a per-rule pass over all ~75 org rules + the 3 postguard-business repo rules via parallel checks — every rule is compliant (most are Dobby-operational and N/A to a docs PR). No content, formatting, docs-drift, or convention issues found. PR title docs: ... is Conventional-Commit compliant. 0 findings from both the review pass and the rule pass.
(Note: this PR is already merged, so this is an after-the-fact confirmation.)
|
Re-reviewed after your latest commit — the coverage note is accurate and everything looks clean. Ran a full compliance pass across all org rules: 0 findings. I've posted an APPROVE review (link), superseding the earlier CHANGES_REQUESTED. (PR was already merged, so this is a formal after-the-fact sign-off. All good! 🟢) |
Closes #101.
Adds a root
AGENTS.mdso contributors and coding agents don't have to reverse-engineer the repo's non-obvious rules. ComplementsREADME.md(setup/URLs) rather than duplicating it.Covers: commands; formatting/TS/Svelte-5 conventions; the structured logger +
event.locals.log/ request-id //healthvs/readyz;$env+FF_*feature flags; file-based migrations + the safety rules and expand/contract pattern; the shared-Postgresbusiness_table prefix; the Yivi/IRMA auth + session model and demo-vs-prod schemes; testing + the coverage gate; Conventional-Commit PR titles + release-please + the security scans; and gotchas (the svelte-i18n global-locale leak, the/irmaproxy, the straycoverage/dir).Reflects the current
main(post #110–#118), so it documents the logging, readiness, migration-safety, and CI-security behavior that just landed.