chore(ci): guard against committed planning docs and remove re-added ones (#4777)

chore(ci): guard against committed planning docs; remove re-added ones

PR #4771 removed the docs/superpowers planning-doc archive and added a rule,
but #4765 force-added two more (git add -f bypasses .gitignore):
docs/superpowers/plans/2026-07-12-observability-single-writer.md and
docs/superpowers/specs/2026-07-12-observability-single-writer-design.md — an
agentic implementation plan and its design spec. Delete both.

Close the enforcement gap so this cannot recur:
- New scripts/check_no_planning_docs.sh fails if anything is tracked under
  docs/superpowers/, regardless of how it was added.
- Wire it into make pre-commit/pre-pr/dev-check.
- Run it in CI: ci.yml for code/mixed PRs, and ci-docs-only.yml (which was a
  green stub) so docs-only PRs can no longer slip a planning doc past the
  required 'Test and Lint' check.
- Document the guard in AGENTS.md and the arch-checks skill.
This commit is contained in:
Zhengchao An
2026-07-12 20:19:06 +08:00
committed by GitHub
parent 3b139e5267
commit f84ba243a1
8 changed files with 67 additions and 137 deletions
+8
View File
@@ -48,3 +48,11 @@ Instruction/architecture docs (`AGENTS.md`, `CLAUDE.md`, `ARCHITECTURE.md`,
`docs/architecture/*.md`) must not reference repo file paths that no longer `docs/architecture/*.md`) must not reference repo file paths that no longer
exist. If your refactor moved code, update the docs that point at it — the exist. If your refactor moved code, update the docs that point at it — the
error message lists `doc -> stale-path` pairs. error message lists `doc -> stale-path` pairs.
## `check_no_planning_docs.sh`
Planning-type documents must not be committed (see AGENTS.md "Sources of
Truth"). The guard fails if anything is tracked under `docs/superpowers/`
`.gitignore` already ignores it, but `git add -f` bypasses that, so this closes
the hole. Fix by removing the listed file(s) with `git rm`; keep the plan or
spec in the issue tracker or a local worktree instead.
+8 -3
View File
@@ -13,14 +13,19 @@ doc-paths-check: ## Check that instruction/architecture docs reference existing
@echo "📄 Checking doc path references..." @echo "📄 Checking doc path references..."
./scripts/check_doc_paths.sh ./scripts/check_doc_paths.sh
.PHONY: planning-docs-check
planning-docs-check: ## Check that no planning-type documents are committed
@echo "📄 Checking for committed planning docs..."
./scripts/check_no_planning_docs.sh
.PHONY: pre-commit .PHONY: pre-commit
pre-commit: fmt-check unsafe-code-check architecture-migration-check logging-guardrails-check tokio-io-uring-check extension-schema-check body-cache-whitelist-check doc-paths-check quick-check ## Run fast pre-commit checks without clippy/full tests pre-commit: fmt-check unsafe-code-check architecture-migration-check logging-guardrails-check tokio-io-uring-check extension-schema-check body-cache-whitelist-check doc-paths-check planning-docs-check quick-check ## Run fast pre-commit checks without clippy/full tests
@echo "✅ All pre-commit checks passed!" @echo "✅ All pre-commit checks passed!"
.PHONY: pre-pr .PHONY: pre-pr
pre-pr: fmt-check unsafe-code-check architecture-migration-check logging-guardrails-check tokio-io-uring-check extension-schema-check body-cache-whitelist-check doc-paths-check clippy-check test ## Run full pre-PR checks with clippy and tests pre-pr: fmt-check unsafe-code-check architecture-migration-check logging-guardrails-check tokio-io-uring-check extension-schema-check body-cache-whitelist-check doc-paths-check planning-docs-check clippy-check test ## Run full pre-PR checks with clippy and tests
@echo "✅ All pre-PR checks passed!" @echo "✅ All pre-PR checks passed!"
.PHONY: dev-check .PHONY: dev-check
dev-check: fmt-check unsafe-code-check architecture-migration-check logging-guardrails-check tokio-io-uring-check extension-schema-check body-cache-whitelist-check doc-paths-check quick-check ## Run fast local development checks dev-check: fmt-check unsafe-code-check architecture-migration-check logging-guardrails-check tokio-io-uring-check extension-schema-check body-cache-whitelist-check doc-paths-check planning-docs-check quick-check ## Run fast local development checks
@echo "✅ Fast development checks passed!" @echo "✅ Fast development checks passed!"
+11 -1
View File
@@ -56,5 +56,15 @@ jobs:
name: Test and Lint name: Test and Lint
runs-on: ubuntu-latest runs-on: ubuntu-latest
steps: steps:
- name: Checkout repository
uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7
# Docs-only PRs skip the full code CI, but they are exactly where a
# planning-type document could be slipped in (git add -f bypasses
# .gitignore). Run the guard here so the required "Test and Lint" check
# stays meaningful for docs-only changes.
- name: Check no planning docs committed
run: ./scripts/check_no_planning_docs.sh
- name: Satisfy required check for docs-only changes - name: Satisfy required check for docs-only changes
run: echo "Docs-only change — code CI is skipped by paths-ignore; reporting success for the required 'Test and Lint' check." run: echo "Docs-only change — code CI is skipped by paths-ignore; planning-docs guard passed, reporting success for the required 'Test and Lint' check."
+3
View File
@@ -134,6 +134,9 @@ jobs:
- name: Check body-cache whitelist guard - name: Check body-cache whitelist guard
run: ./scripts/check_body_cache_whitelist.sh run: ./scripts/check_body_cache_whitelist.sh
- name: Check no planning docs committed
run: ./scripts/check_no_planning_docs.sh
test-and-lint: test-and-lint:
name: Test and Lint name: Test and Lint
if: github.event_name != 'pull_request' || github.event.action != 'closed' if: github.event_name != 'pull_request' || github.event.action != 'closed'
+3
View File
@@ -80,6 +80,9 @@ the repository. Only durable reference — the architecture set under
and the test-suite references under `docs/testing/` — belongs in version and the test-suite references under `docs/testing/` — belongs in version
control; `.gitignore` ignores everything else under `docs/` by default, so a new control; `.gitignore` ignores everything else under `docs/` by default, so a new
plan file will not be tracked unless someone force-adds it — don't. plan file will not be tracked unless someone force-adds it — don't.
`scripts/check_no_planning_docs.sh` (wired into `make pre-commit`/`pre-pr` and
CI) fails the build if anything is committed under `docs/superpowers/`, even via
`git add -f`.
## Verification Before PR ## Verification Before PR
@@ -1,89 +0,0 @@
# Logging Safety Implementation Plan
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
**Goal:** Prevent RustFS log inode aliasing, bound ECStore hot-path tracing, and verify the combined fix across supported deployments.
**Architecture:** Resolve stdout mirroring once from a tri-state configuration, validate Unix sink identity before registering logging workers, and make the packaged systemd unit route process output to journald. Local and OTLP initialization share the same helpers. ECStore data-path spans are TRACE-only and never capture heavy arguments. Static and runtime tests pin the combined invariants.
**Tech Stack:** Rust, tracing/tracing-appender, Unix file metadata, systemd, shell guardrails.
**Status updated:** 2026-07-12 after pulling `origin/houseme/pr-4765-ci-fix`.
**Status legend:** `[x]` completed, `[ ]` pending final validation or external evidence.
---
### Task 1: Preserve stdout configuration intent
**Files:**
- Modify: `crates/obs/src/config.rs`
- Modify: `crates/obs/src/telemetry/local.rs`
- Modify: `crates/obs/src/telemetry/otel.rs`
- [x] Add table tests proving unset, explicit true, and explicit false behavior in production and development.
- [x] Run the focused tests and confirm non-production plus explicit false fails on the current implementation.
- [x] Preserve `None` when the environment variable is absent and add one shared mirror resolver.
- [x] Use the resolver in both local and OTLP file initialization.
- [x] Run focused tests and confirm all table cases pass.
### Task 2: Reject same-inode stdout and rolling file sinks
**Files:**
- Modify: `crates/obs/src/telemetry/local.rs`
- Modify: `crates/obs/src/telemetry/otel.rs`
- Modify: `crates/obs/src/telemetry/rolling.rs` only if a read-only active-path accessor is required
- Test: `crates/obs/src/telemetry/local.rs` test module or a focused Unix integration test
- [x] Add Unix tests for same inode, hardlink alias, and distinct regular files; isolate stdout descriptor mutation in a child process if descriptor replacement is required.
- [x] Run the focused tests and confirm the same-inode case is not rejected by current code.
- [x] Add a shared pre-registration validator comparing regular-file device and inode metadata.
- [x] Invoke validation from local and OTLP file setup before subscriber registration and cleanup startup.
- [x] Run the focused tests and existing rolling/cleaner tests.
### Task 3: Establish the packaged systemd single-writer contract
**Files:**
- Modify: `deploy/build/rustfs.service`
- Modify: `scripts/check_logging_guardrails.sh`
- [x] Add a guardrail assertion that fails while the unit appends stdout or stderr to a RustFS-managed log file.
- [x] Run the guardrail and confirm it fails against the current unit.
- [x] Change stdout and stderr to journald.
- [x] Run the guardrail and confirm it passes.
### Task 4: Bound ECStore hot-path tracing
**Files:**
- Modify: `crates/ecstore/src/disk/mod.rs`
- Modify: `crates/ecstore/src/disk/local.rs`
- Modify: `crates/ecstore/src/cluster/rpc/remote_disk.rs`
- [x] Change per-object/per-disk/per-RPC success spans to TRACE with `skip_all` across all three layers.
- [x] Keep useful scalar context only in existing explicit structured RemoteDisk trace events; do not automatically capture method arguments.
- [x] Add a static guard that rejects scoped instrumentation without exact TRACE + `skip_all`, and rejects DEBUG RemoteDisk success events.
- [ ] Run the guard and focused crate tests.
### Task 5: Add combined regression guardrails
**Files:**
- Modify: `scripts/check_logging_guardrails.sh`
- Modify or create focused deployment/rotation tests as required
- [x] Guard the packaged systemd unit against append-to-active-log regression.
- [x] Guard ECStore data-path instrumentation against implicit INFO and heavy argument capture.
- [x] Verify Docker and Helm file logging remain distinct from container stdout without changing their persistence defaults.
- [ ] Add the repeatable four-node Warp acceptance procedure and measurements to the implementation handoff.
### Task 6: Verify and review
**Files:**
- Review all files changed above
- [x] Run `cargo fmt --all --check`.
- [x] Run `cargo test -p rustfs-obs`.
- [x] Run `./scripts/check_logging_guardrails.sh`.
- [ ] Run `make pre-commit` and resolve any in-scope failures.
- [x] Run the applicable correctness, concurrency, compatibility, performance, and test-coverage adversarial passes over the final diff.
- [ ] Run `make pre-pr` after all findings are resolved.
- [x] Clean generated build artifacts according to repository policy.
@@ -1,44 +0,0 @@
# Logging Safety Design
## Problem
RustFS file logging and the packaged systemd service can open the same `rustfs.log` inode. The internal appender renames and later removes that inode during rotation while systemd continues writing through its original stdout file descriptor. Non-production logging also forces stdout mirroring even when the operator explicitly sets `RUSTFS_OBS_LOG_STDOUT_ENABLED=false`. The combination creates duplicated logs and an unbounded deleted file that bypasses retention limits.
## Goals
- Make the packaged systemd service send stdout and stderr to journald.
- Preserve explicit file logging as a supported sink.
- Give an explicitly configured stdout-mirror value precedence over environment defaults.
- Reject an exact same-inode stdout/file configuration before logging workers or cleanup start.
- Apply identical resolution and validation to local and OTLP file logging.
- Prevent ECStore data-path spans from serializing collections, metadata, or byte payloads at any level.
- Add deployment and end-to-end guardrails that verify the combined fix.
## Non-goals
- Replacing rename-based rolling or cleaner retention.
- Using `copytruncate`.
- Changing Docker or Helm persistent-log defaults.
- Adding probabilistic sampling, a new trace transport, or generalized error deduplication.
## Design
`log_stdout_enabled` remains optional when read from the environment. Unset means the runtime chooses the existing default: enabled outside production and disabled in production. Explicit `true` and `false` always win. A shared resolver is used by local and OTLP initialization.
On Unix, file logging validates sink ownership after the active file has been opened but before the tracing subscriber and cleanup task are registered. If stdout is a regular file and its device/inode pair equals the active rolling file's device/inode pair, initialization returns a typed observability error with an actionable message. Pipes, sockets, terminals, and different regular files remain valid. Non-Unix behavior is unchanged.
The packaged systemd unit explicitly selects journald for stdout and stderr, leaving the internal rolling appender as the sole owner of its active file.
ECStore per-object, per-disk, per-RPC, and per-batch success spans use TRACE with `skip_all` at the Disk facade, LocalDisk, and RemoteDisk layers. These automatic spans retain only the method/span identity and never serialize arguments. Existing explicit RemoteDisk trace events may retain selected scalar operation fields. INFO remains reserved for low-frequency lifecycle and state changes, while WARN and ERROR retain actionable failures.
Static guardrails pin both invariants: official deployment files must not redirect stdout to the active rolling file, and ECStore data-path instrumentation must declare an explicit non-INFO level and skip heavy payloads.
## Verification
- Table tests pin unset/true/false behavior in production and development.
- Unix tests pin same-inode detection and allow distinct sinks.
- Local and OTLP file paths use the shared helpers.
- A static guard prevents the packaged unit from appending stdout to `rustfs.log`.
- Existing rolling, compression, and cleaner tests remain unchanged and pass.
- Static guardrails prove every scoped ECStore data-path span is TRACE-only with `skip_all` and reject DEBUG RemoteDisk success events.
- The four-node concurrency-64 Warp workload is the final acceptance test for deleted descriptors, log growth, throughput, `unexpected EOF`, and `SlowDown`.
+34
View File
@@ -0,0 +1,34 @@
#!/usr/bin/env bash
set -euo pipefail
# Guard: planning-type documents must not be committed.
#
# One-shot implementation/optimization plans, migration-progress ledgers,
# design specs, and agent-generated working notes (e.g. anything a
# `superpowers`/scratch workflow produces) belong in the issue tracker or a
# local worktree, never in the repository — see AGENTS.md "Sources of Truth".
#
# .gitignore already keeps everything under docs/ out of the tree except the
# whitelisted durable subtrees (architecture/, operations/, testing/), but
# `git add -f` bypasses .gitignore. This guard closes that hole: any tracked
# file under docs/superpowers/ fails the check regardless of how it got added.
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
ROOT_DIR="${CHECK_NO_PLANNING_DOCS_ROOT:-$(cd "${SCRIPT_DIR}/.." && pwd)}"
cd "$ROOT_DIR"
status=0
while IFS= read -r file; do
[[ -z "$file" ]] && continue
printf '%s: planning-type document must not be committed (keep it in the issue tracker or a local worktree)\n' \
"$file" >&2
status=1
done < <(git ls-files 'docs/superpowers/*')
if [[ "$status" -ne 0 ]]; then
printf '\nRemove the file(s) above with `git rm`. See AGENTS.md "Sources of Truth".\n' >&2
fi
exit "$status"