Skip to content

Extend architecture test: denylist scope + stepdown rule check#117

Open
saltyskip wants to merge 1 commit intomainfrom
extend-architecture-test
Open

Extend architecture test: denylist scope + stepdown rule check#117
saltyskip wants to merge 1 commit intomainfrom
extend-architecture-test

Conversation

@saltyskip
Copy link
Copy Markdown
Owner

@saltyskip saltyskip commented May 1, 2026

Summary

Two upgrades to src/architecture_tests.rs that close the gaps surfaced by review:

1. is_enforced_file flips from allowlist to denylist

Was a fixed set of filenames (routes.rs, service.rs, repo.rs, etc.) which silently missed any helper file with a different name. landing.rs, qr.rs, stripe_webhook.rs, and any future helper file slipped through unchecked.

Now: enforce every .rs under src/ except models.rs, lib.rs, main.rs, architecture_tests.rs, and *_tests.rs. New files inherit enforcement automatically.

2. Stepdown rule check (Clean Code, Robert C. Martin)

stepdown_rule_at_depth_zero flags any free private fn that appears before the first free public (or pub(crate)) fn at depth 0 in the same file. Methods inside impl blocks aren't checked — they're nested.

The caller-above-callee half of the rule needs a real call graph and isn't mechanically checked; it stays review-only.

What the denylist flip exposed

The old allowlist was quietly missing real violations. Categorized:

Impl containers needing markers (fixed in this PR):

  • migrations/m001_auth_split.rs::M001AuthSplit (and m002, m003, m004)
  • core/cdp.rs::CdpFacilitator
  • core/threat_feed.rs::ThreatFeed
  • core/rate_limit.rs::RateLimiter
  • mcp/server.rs::RiftMcp
  • services/billing/repos/resource_counts_adapter.rs::RepoResourceCounts

Real data-type extractions (deferred to backlog):

Tracked in a new PUB_TYPES_CLEANUP_BACKLOG const with per-file // TODO comments. The backlog is debt to chip away at:

File Cleanup needed
core/config.rs Config — needs core/models.rs
core/webhook_dispatcher.rs 4 event payloads — extract to core/models.rs
error.rs ErrorResponse, AppError — top-level error types
mcp/tools.rs 10 input DTOs — rename to mcp/models.rs
app.rs AppState — single-struct top-level file
services/billing/handoff.rs 5 data types → billing/models.rs (BillingHandoffService is impl container)
services/billing/stripe_client.rs 6 data types → billing/models.rs
services/billing/limits.rs PlanLimits → billing/models.rs
services/billing/repos/event_counters.rs EventCounterDoc → models; EventCountersRepo is impl container
services/billing/repos/stripe_webhook_dedup.rs StripeDedupDoc → models; StripeWebhookDedupRepo is impl container

Stepdown rule fixes (3 small reorders)

  • api/mod.rs: serve_rift_js (helper) was above router (the public fn) → moved to a Helpers section at the bottom
  • services/links/service.rs: to_doc_value (private helper) was above build_canonical_link_url (pub fn) → moved to file bottom
  • services/webhooks/dispatcher.rs: compute_hmac (pub(crate)) was below cached_find_active_for_event (private) → reordered so public surface comes first

CLAUDE.md updates

  • Documents the pub(crate) carve-out explicitly (the rule is about external API surface, not crate-internal scaffolding)
  • Adds the stepdown rule as a separately-enforced bullet point with reference to the test

Test plan

  • cargo fmt -- --check
  • cargo clippy --all-targets -- -D warnings
  • cargo test — 112 lib + 145 integration tests pass (was 107; +5 from new arch test parser unit tests)
  • No behavior change in production code

Why a backlog instead of fixing everything

This PR is meant to land the enforcement infrastructure cleanly. Fixing every pre-existing violation (15+ files, ~30 types) in one PR would balloon the diff and make review harder. The backlog approach makes the debt visible and tractable: each file is a self-contained follow-up.

🤖 Generated with Claude Code

@saltyskip @claude
Extend architecture test: denylist + stepdown rule
2fdb6f0 
Two upgrades to `src/architecture_tests.rs`:

1. **`is_enforced_file` flips from allowlist to denylist.** Was a fixed
   set of filenames (`routes.rs`, `service.rs`, `repo.rs`, etc.) which
   silently missed any helper file with a different name (`landing.rs`,
   `qr.rs`, `stripe_webhook.rs`, future additions). Now: enforce every
   `.rs` under `src/` except `models.rs`, `lib.rs`, `main.rs`,
   `architecture_tests.rs`, and `*_tests.rs`. New files inherit
   enforcement automatically.

2. **New `stepdown_rule_at_depth_zero` test.** Codifies the public-vs-
   private half of the stepdown rule: in any enforced file, no free
   private `fn` may appear before the first free public/`pub(crate)`
   `fn` at depth 0. The caller-above-callee half needs a real call
   graph and is still review-only.

The denylist flip exposed pre-existing violations the old allowlist
quietly missed. Two categories:

- **Impl containers** that needed `impl_container!` markers — the
  obvious ones (migrations m001-m004, CdpFacilitator, ThreatFeed,
  RateLimiter, RiftMcp, RepoResourceCounts) are marked in this PR.
- **Real data-type extractions** that need `models.rs` migration —
  10 files tracked in a new `PUB_TYPES_CLEANUP_BACKLOG` const with
  per-file TODO comments. Each entry can be cleaned up independently
  in follow-up PRs; the backlog is the lever to chip away at.

Stepdown rule fixes (3 small reorders to land green):
- `api/mod.rs`: `serve_rift_js` moved below `router` to a Helpers section
- `services/links/service.rs`: `to_doc_value` moved below
  `build_canonical_link_url` to file bottom
- `services/webhooks/dispatcher.rs`: `compute_hmac` (pub(crate)) moved
  above `cached_find_active_for_event` (private) so public surface
  comes first

CLAUDE.md updated to describe both rules and the `pub(crate)` carve-out.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@vercel
Copy link
Copy Markdown
Contributor

vercel Bot commented May 1, 2026 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
rift Ready Ready Preview, Comment May 1, 2026 6:13pm

Request Review

@saltyskip saltyskip mentioned this pull request May 1, 2026
Open
4 tasks
saltyskip added a commit that referenced this pull request May 1, 2026
@saltyskip @claude
Clear PUB_TYPES_CLEANUP_BACKLOG: extract every remaining inline pub type
080641a 
Stacks on PR #117. The backlog list introduced there held 10 files of
pre-existing pub-data-type violations. This PR works through every
entry so the backlog is empty.

**Billing slice — moved to services/billing/models.rs:**
- handoff.rs: BillingIntent, BillingTier (+ impl), BillingHandoffError,
  HandoffOutcome, BillingHandoffConfig
- stripe_client.rs: StripeConfig (+ impl), StripeError (+ Display),
  CheckoutSession, HandoffCheckoutOpts, PortalSession,
  WebhookVerifyError (+ Display)
- limits.rs: PlanLimits
- repos/event_counters.rs: EventCounterDoc (EventCountersRepo marked
  impl_container)
- repos/stripe_webhook_dedup.rs: StripeDedupDoc (StripeWebhookDedupRepo
  marked impl_container)

Source files now `pub use` from models.rs to keep external import paths
stable. BillingHandoffService also marked as impl_container.

**Core slice — new core/models.rs:**
- 4 webhook payloads (ClickEventPayload, AttributionEventPayload,
  ConversionEventPayload, WebhookPayload) extracted from
  webhook_dispatcher.rs. The trait stays.
- core/config.rs::Config marked as impl_container (env-loaded settings
  with from_env constructor — same pattern as repo containers).

**Single-struct top-level files — converted to directories:**
- error.rs → error/{mod.rs (the IntoResponse impl), models.rs
  (ErrorResponse, AppError)}. mod.rs re-exports both, so
  `crate::error::ErrorResponse` keeps working everywhere.
- app.rs → app/{mod.rs (re-export), models.rs (AppState)}. Same
  re-export pattern; `crate::app::AppState` unchanged.

**MCP renamed:**
- mcp/tools.rs → mcp/models.rs. The file already held DTO-shaped tool
  input types — name now matches the pattern. `mcp/server.rs` and
  `mcp/mod.rs` updated.

**Architecture test:**
PUB_TYPES_CLEANUP_BACKLOG drops to `&[]`. Every `.rs` under `src/`
(except models.rs, lib.rs, main.rs, architecture_tests.rs, *_tests.rs)
is now enforced and clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@saltyskip