bd-a7e: Bootstrap Rust project and directory structure
This commit is contained in:
teernisse
158
prd-swagger-cli.feedback-2.md
Normal file
158
prd-swagger-cli.feedback-2.md
Normal file
@@ -0,0 +1,158 @@
|
||||
Proposed revisions below are all new and avoid anything in your `## Rejected Recommendations` list.
|
||||
|
||||
1. **Add fetch-time policy enforcement (SSRF + insecure transport controls)**
|
||||
Analysis: Right now external ref allowlisting is good, but primary `fetch` still permits risky network targets. Adding policy gates prevents accidental access to metadata endpoints/internal services and makes CI behavior more deterministic. This is a high-impact security and reliability improvement with low implementation risk.
|
||||
```diff
|
||||
@@ FR-1: Spec Fetching and Caching / Acceptance Criteria
|
||||
+- ✓ Enforce fetch network policy by default: block loopback/link-local/RFC1918/multicast targets for remote URL fetches
|
||||
+- ✓ Require HTTPS for remote URLs by default; plain HTTP requires explicit opt-in
|
||||
+- ✓ Resolve and validate final connected IP after redirects to mitigate DNS-rebinding bypasses
|
||||
|
||||
@@ Command: swagger-cli fetch <url> [OPTIONS]
|
||||
+ --allow-private-host <HOST> Allow specific private/internal hosts (repeatable)
|
||||
+ --allow-insecure-http Permit http:// URLs (default: reject)
|
||||
|
||||
@@ Error cases
|
||||
+| Policy blocked | 16 | `POLICY_BLOCKED` |
|
||||
```
|
||||
|
||||
2. **Harden alias and filesystem path safety**
|
||||
Analysis: Alias names currently appear to map directly to directory names, which creates path traversal and portability risk if not constrained. Strict alias validation plus canonical path handling for local files removes a whole class of corruption/security bugs.
|
||||
```diff
|
||||
@@ FR-1: Spec Fetching and Caching / Acceptance Criteria
|
||||
+- ✓ Validate alias format: `^[A-Za-z0-9][A-Za-z0-9._-]{0,63}$`
|
||||
+- ✓ Reject aliases containing path separators, `..`, or reserved device names
|
||||
+- ✓ Canonicalize local file paths before ingest; fail fast on unreadable targets
|
||||
```
|
||||
|
||||
3. **Promote `diff` to Phase 2 with breaking-change classification**
|
||||
Analysis: You already compute sync deltas; exposing this directly as a first-class command is highly compelling for CI, release checks, and agent workflows. It turns passive metadata into an actionable compatibility contract.
|
||||
```diff
|
||||
@@ Functional Requirements
|
||||
+### FR-11: Spec Diff and Compatibility
|
||||
+**Description:** Compare two spec states and classify changes as breaking/non-breaking.
|
||||
+**Acceptance Criteria:**
|
||||
+- ✓ Compare alias vs alias, alias vs URL, or alias generation vs generation
|
||||
+- ✓ Classify endpoint and schema changes (`breaking`, `non_breaking`, `unknown`)
|
||||
+- ✓ Robot mode emits machine-actionable compatibility summary
|
||||
+**Command:**
|
||||
+`swagger-cli diff <LEFT> <RIGHT> [--fail-on breaking] [--details] [--robot]`
|
||||
|
||||
@@ Phase 2 (Polish) - Week 2
|
||||
+- ✓ Diff command with compatibility classification for CI gates
|
||||
```
|
||||
|
||||
4. **Formalize robot contract with JSON Schema artifacts and compatibility policy**
|
||||
Analysis: Golden tests are good, but schema files give stronger guarantees and external tooling compatibility. This materially improves long-term API stability for agent consumers.
|
||||
```diff
|
||||
@@ Appendix C: Robot Mode Output Schemas
|
||||
+Canonical schemas are versioned and published:
|
||||
+- docs/robot-schema/v1/success.schema.json
|
||||
+- docs/robot-schema/v1/error.schema.json
|
||||
+
|
||||
+Compatibility policy:
|
||||
+- Additive fields: no schema_version bump
|
||||
+- Removed/renamed/type-changed fields: MUST bump schema_version
|
||||
+- `meta.command_version` added for command-specific payload evolution
|
||||
```
|
||||
|
||||
5. **Reduce write amplification from `last_accessed` updates**
|
||||
Analysis: Updating metadata on every query can create unnecessary I/O, contention, and SSD churn, especially with frequent agent calls. Coalescing updates preserves LRU usefulness while improving performance/reliability under concurrency.
|
||||
```diff
|
||||
@@ FR-10 Decision rationale
|
||||
-- **LRU eviction:** Uses `last_accessed` timestamp (updated on every query command) for fair eviction ordering.
|
||||
+- **LRU eviction:** Uses coalesced `last_accessed` writes (e.g., write only when older than 10 minutes) to reduce lock contention and write amplification.
|
||||
|
||||
@@ src/core/cache.rs / Read protocol
|
||||
-- Update meta.last_accessed timestamp on successful read (best-effort, no lock required).
|
||||
+- Coalesce last_accessed updates with minimum write interval; skip metadata rewrite for hot-read bursts.
|
||||
```
|
||||
|
||||
6. **Shift to async HTTP + explicit service layering**
|
||||
Analysis: `sync --all` and retry/per-host controls are cleaner and more robust with async primitives. Separating CLI parsing from application services and infra adapters improves testability and lowers long-term maintenance cost.
|
||||
```diff
|
||||
@@ Technology Stack
|
||||
-reqwest = { version = "0.13", default-features = false, features = ["json", "blocking", "rustls-tls"] }
|
||||
+reqwest = { version = "0.13", default-features = false, features = ["json", "rustls-tls", "http2"] }
|
||||
+tokio = { version = "1", features = ["rt-multi-thread", "macros", "time", "sync"] }
|
||||
+tracing = "0.1"
|
||||
+tracing-subscriber = { version = "0.3", features = ["env-filter"] }
|
||||
|
||||
@@ Project Structure
|
||||
+├── src/app/ # Use-case orchestration (fetch/list/show/sync services)
|
||||
+├── src/infra/ # HTTP client, fs/cache, lock, clock abstractions
|
||||
```
|
||||
|
||||
7. **Add resumable sync and failure budget controls**
|
||||
Analysis: Large multi-alias sync runs can fail mid-way and currently require full reruns. Resume checkpoints and configurable failure budgets improve reliability for CI and large fleet use.
|
||||
```diff
|
||||
@@ FR-7: Sync and Updates / OPTIONS
|
||||
+ --resume Resume from last interrupted --all sync checkpoint
|
||||
+ --max-failures <N> Abort run after N alias failures (default: unlimited)
|
||||
|
||||
@@ FR-7 Acceptance Criteria
|
||||
+- ✓ `sync --all` persists per-alias progress checkpoint for resumable execution
|
||||
+- ✓ Supports controlled abort via failure budget to limit noisy upstream incidents
|
||||
```
|
||||
|
||||
8. **Upgrade secret handling model beyond plaintext token field**
|
||||
Analysis: You already warn on file perms, but adding env/keyring sources avoids persisting sensitive tokens where possible. This is a meaningful security upgrade with backward-compatible defaults.
|
||||
```diff
|
||||
@@ src/core/config.rs
|
||||
-pub struct AuthConfig {
|
||||
- pub url_pattern: String,
|
||||
- pub auth_type: AuthType,
|
||||
- pub token: String,
|
||||
-}
|
||||
+pub struct AuthConfig {
|
||||
+ pub url_pattern: String,
|
||||
+ pub auth_type: AuthType,
|
||||
+ pub credential: CredentialSource,
|
||||
+}
|
||||
+
|
||||
+pub enum CredentialSource {
|
||||
+ Literal(String),
|
||||
+ EnvVar(String),
|
||||
+ Keyring { service: String, account: String },
|
||||
+}
|
||||
```
|
||||
|
||||
9. **Expand supply-chain controls (dependency policy + SBOM + provenance)**
|
||||
Analysis: Checksums/signatures help artifact integrity, but you still need dependency and provenance gates in CI for stronger trust. This is especially valuable for an agent-facing CLI that may run in privileged automation.
|
||||
```diff
|
||||
@@ .gitlab-ci.yml
|
||||
+security:deps:
|
||||
+ stage: test
|
||||
+ image: rust:1.93
|
||||
+ script:
|
||||
+ - cargo install cargo-deny cargo-audit
|
||||
+ - cargo deny check
|
||||
+ - cargo audit
|
||||
+
|
||||
+release:sbom:
|
||||
+ stage: release
|
||||
+ script:
|
||||
+ - cargo install cargo-cyclonedx
|
||||
+ - cargo cyclonedx --format json --output-file sbom.cdx.json
|
||||
+ - cosign attest --predicate sbom.cdx.json --type cyclonedx $RELEASE_ARTIFACT
|
||||
```
|
||||
|
||||
10. **Fix installer portability and cleanup behavior**
|
||||
Analysis: `sha256sum` is not guaranteed on macOS, and current temp-file handling is not robust on interrupted runs. These fixes reduce installation friction and avoid leaving sensitive artifacts in `/tmp`.
|
||||
```diff
|
||||
@@ install.sh
|
||||
-set -e
|
||||
+set -euo pipefail
|
||||
+TMP_DIR="$(mktemp -d)"
|
||||
+trap 'rm -rf "$TMP_DIR"' EXIT
|
||||
|
||||
@@ checksum verification
|
||||
-ACTUAL=$(sha256sum "$INSTALL_DIR/swagger-cli" | awk '{print $1}')
|
||||
+if command -v sha256sum >/dev/null 2>&1; then
|
||||
+ ACTUAL=$(sha256sum "$INSTALL_DIR/swagger-cli" | awk '{print $1}')
|
||||
+else
|
||||
+ ACTUAL=$(shasum -a 256 "$INSTALL_DIR/swagger-cli" | awk '{print $1}')
|
||||
+fi
|
||||
```
|
||||
|
||||
If you want, I can produce one consolidated full-PRD patch (single unified diff) applying all 10 changes in-place so you can drop it directly into the next iteration.
|
||||
Reference in New Issue
Block a user