docs(site): Architecture + ADR index — 5 pages#62
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e5f08fae8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Via Basic auth (no cookie needed) | ||
| curl -X POST http://localhost:8080/api/workspaces/$WS/tokens \ | ||
| -u admin@pal.local:<your-bootstrap-password> \ |
There was a problem hiding this comment.
The token-minting examples currently instruct users to call /api/workspaces/{workspaceId}/tokens with -u Basic auth, but the app never authenticates Basic credentials: Program.cs only forwards Authorization headers that start with Bearer to ApiKeyAuthenticationHandler, and otherwise falls back to cookie auth. In practice this command path returns an auth challenge instead of creating a token, so first-time automation bootstrap fails unless users perform /account/login and reuse the session cookie.
Useful? React with 👍 / 👎.
| ## What changes when you create a new workspace | ||
|
|
||
| - All workspace-scoped routes (`/api/workspaces/{newId}/...`) become reachable for org members. | ||
| - A new directory under `Storage:LocalRoot/uploads/{newId}/` gets created lazily on first upload. |
There was a problem hiding this comment.
This says creating a workspace leads to Storage:LocalRoot/uploads/{newId}/, but upload storage is content-addressed by SHA-256 (uploads/{sha256}/...) and is shared across workspaces (LocalDiskStorageProvider.CommitUploadAsync). Operators following this guidance will look for (or script against) a directory structure that never exists, which can break cleanup and backup procedures.
Useful? React with 👍 / 👎.
| │ ├── report.json | ||
| │ ├── report.html | ||
| │ ├── report.md (only if Markdown was generated) |
There was a problem hiding this comment.
The storage layout documents report files as report.json/report.html/report.md, but the API writes report.pal-report.json|html|md (LocalDiskStorageProvider.WriteReportAsync). Any operational tooling built from this page (backup validation, incident triage, file probes) will fail to find report artifacts under the documented names.
Useful? React with 👍 / 👎.
Step 8. Architecture is the contributor's-eye view of how PAL-X is structured. Stacked on docs/step-7-operations for cross-references to Concepts (multitenancy-and-auth) and Operations (postgres-setup, backup-and-restore). PAGES (5 new): - architecture/index.md — project map (8 .NET projects across 4 layers: engine / shared / infrastructure / surface), layer dependency diagram with two enforced rules (engine depends on nothing; application doesn't reference persistence), three runtime modes (CLI / API+worker / Blazor), and a "where ground truth lives" table mapping topics to authoritative source files. - architecture/data-flow.md — six-hop pipeline diagram (input → collector dispatch → canonical metric IDs → pack loading → RuleEngine → reports → charts), explained hop by hop, with the CLI synchronous and API asynchronous modes both shown sharing the same engine. Calls out determinism, host_context informational- fallback, pack-level applicability fast-skip, and the in-memory Channel<Guid> trade-off. - architecture/persistence.md — why Postgres (jsonb, cascade semantics, managed offerings); schema layout grouped into identity /tenancy/data-plane/phase-4/EF-bookkeeping; the two DbContext registrations (factory + scoped, for workers vs Identity); the tenant query filter pattern with the !HasValue||==GetValueOrDefault specifically explained (per CLAUDE.md guidance — non-negotiable because EF parameter extraction crashes on .Value with null Nullable<Guid>); migrations list; what's NOT in Postgres (uploaded files, report renderings, dataset artifacts, chart SVGs). - architecture/schema-evolution.md — two schemas (pack v1→v1.1 shipped, report v1 unchanged), three change flavours (additive minor, breaking major, deprecation), v1→v1.1 case study with what we got right vs would do differently, versioning policy going forward, stable finding_id across schema bumps as a contract consumers can rely on. - architecture/adr/index.md — table of all 4 ADRs with status, date, and one-line summary; reading and authoring guidance; explicit list of what does NOT warrant an ADR (bug fixes, refactors, tactical choices, configuration defaults). GROUND-TRUTHED against: - PalDbContext.cs lines 92-202 (7 entities with query filters; the exact !HasValue || == GetValueOrDefault pattern) - Pal.Persistence/Migrations/ (14 migrations enumerated) - Pal.Engine layer dependencies (verified with project file scan) - ADR 0001-0004 (status/date/one-line summary read from each file) Top-level docs/toc.yml adds Architecture after Operations. Build verified: dotnet docfx docs/docfx.json --warningsAsErrors — 0 warnings, 0 errors. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
joshuatownsend
force-pushed
the
docs/step-8-architecture
branch
from
7e5f08f to
5c1938d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c1938d325
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| The engine pipeline is identical. What's different is the orchestration: HTTP enqueues, the worker dequeues, repositories persist, and additional services (`PolicyEvaluator`, `IAutoCompareService`, `NotificationService`) extend the post-analysis flow with alerting and comparisons. | ||
|
|
||
| The in-process `Channel<Guid>` keeps the API simple — no external message broker, no Postgres `LISTEN/NOTIFY`. The trade-off: if the API process crashes, queued-but-not-started jobs are lost (the worker channel is in-memory). Jobs that have started but not finished are detected on restart and marked `failed`. This is documented as a Phase 5 improvement candidate. |
There was a problem hiding this comment.
This sentence describes restart behavior opposite to the implementation: jobs are persisted in analysis_jobs before channel enqueue, AnalysisWorker.StartAsync re-enqueues queued jobs from the DB, and ResetOrphanedJobsAsync moves running jobs back to queued (not failed). In operational incidents, this doc would cause readers to treat restart as lossy/final-failure and perform unnecessary manual recovery.
Useful? React with 👍 / 👎.
| ├── analysis_job_packs (M:N) | ||
| ├── analysis_results | ||
| ├── analysis_reports (json / html / markdown) | ||
| ├── analysis_job_dataset_artifact (one per job, opt-in) |
There was a problem hiding this comment.
The schema list names analysis_job_dataset_artifact, but dataset artifact metadata is stored as columns on analysis_results (added by migration 20260427173608_AddDatasetArtifact and written via AnalysisRepository.SaveDatasetArtifactAsync). SQL/runbooks derived from this page will query a table that does not exist and fail at runtime.
Useful? React with 👍 / 👎.
| If `--include-charts` is set (CLI) or charts are otherwise requested, the engine attaches `ChartRef` entries to findings and writes SVGs via `ScottPlot.Plot.Save`. One SVG per (finding × metric) pair, capped by `--chart-limit` (default 20). | ||
|
|
||
| Charts are written to `out/charts/<report-name>-<chart-id>.svg`. The HTML report embeds them inline. The JSON report references them by relative path in each finding's `evidence.charts[]`. |
There was a problem hiding this comment.
This section documents a chart pipeline (--include-charts, evidence.charts[], out/charts/...) that the current code path does not implement: findings do not carry chart references and neither CLI nor API analysis flow invokes chart rendering/writes chart files. Users and automation following this architecture contract will wait for artifacts that are never produced.
Useful? React with 👍 / 👎.
1 participant
Replaces closed PR #58, auto-closed when its base branch (
docs/step-7-operations) was deleted on PR #61 merge.Original PR body and review threads at #58. Architecture had no substantive review comments per the pr-resolve summary.
Summary
Step 8: Architecture section (5 pages) — index, data-flow, persistence, schema-evolution, ADR index.
Ground-truthed against
PalDbContext.cs(tenant query filter pattern), 14 EF migrations, and ADRs 0001-0004.Test plan
dotnet docfx docs/docfx.json --warningsAsErrors— 0 warnings, 0 errors./architecture/.🤖 Generated with Claude Code