mirror of
https://github.com/Rain-kl/OpenFlare.git
synced 2026-10-10 17:26:38 +08:00
Merge remote-tracking branch 'wavelet/feat/cordis-alignment' into cordis
# Conflicts: # .agents/skills/cache-framework/SKILL.md # .agents/skills/clickhouse-batchwriter/SKILL.md # .agents/skills/database-migration/SKILL.md # .agents/skills/file-upload/SKILL.md # .agents/skills/logstore/SKILL.md # .agents/skills/new-api/SKILL.md # .agents/skills/new-api/references/handler_example.go # .agents/skills/new-api/references/logics_example.go # .agents/skills/new-api/references/service_example.go # .agents/skills/new-async-task/SKILL.md # .agents/skills/new-async-task/references/CODE-EXAMPLES.md # .agents/skills/new-setting/SKILL.md # .agents/skills/push-notification/SKILL.md # .agents/skills/release-guide/SKILL.md # .auto/checks.sh # .auto/ideas.md # .auto/log.jsonl # .auto/measure.sh # .auto/prompt.md # .dockerignore # .env.example # .github/copilot-instructions.md # .github/workflows/build-release.yml # .gitignore # .golangci.yml # AGENTS.md # Makefile # README.md # backend/cmd/app.go # backend/cmd/app_test.go # backend/cmd/banner.go # backend/cmd/banner_test.go # backend/docs/docs.go # backend/docs/swagger.json # backend/docs/swagger.yaml # backend/go.mod # backend/go.sum # backend/main.go # config.example.yaml # docker/Dockerfile # docker/Dockerfile.backend # docker/Dockerfile.cross # scripts/swagger.sh # scripts/update_go_license.sh
This commit is contained in:
ryan
@@ -0,0 +1,242 @@
|
||||
# Autoresearch lessons — Wavelet / Cordis quality run
|
||||
|
||||
Accumulated wisdom across iterations. Read this before forming a hypothesis.
|
||||
Weight recent lessons higher: the yardstick and codebase change under us.
|
||||
|
||||
## Lesson 1 — iterations 0-1
|
||||
**Pattern**: The project's committed gate (`golangci-lint run` with `.golangci.yml`)
|
||||
had already been driven to 0 issues by a previous run, so it could no longer
|
||||
measure anything.
|
||||
**Why it worked**: Measuring against a pinned snapshot + extra analyzers in
|
||||
`.auto/lint.ref.yaml` (hash-locked by the Guard) restored headroom and made it
|
||||
impossible to lower the number by editing the config.
|
||||
**Conditions**: Any repo whose own lint gate is already green.
|
||||
**Anti-pattern**: Optimising `tagliatelle` (325 findings) or `wrapcheck` (290).
|
||||
Those are pure cosmetics — error-message wording and tag naming. A run that
|
||||
chases them will look productive while shuffling strings.
|
||||
**Metric delta**: baseline re-established at 102 instead of a dead 0.
|
||||
|
||||
## Lesson 2 — iterations 1-4
|
||||
**Pattern**: Triage every analyzer finding for reality before "fixing" it.
|
||||
**Why it worked**: Three buckets turned out to be false positives:
|
||||
`forcetypeassert` in `core/events.go` is guarded by `returnsErr` (the handler's
|
||||
declared last out really is `error`), and both `exhaustive` switches already
|
||||
have `default:` arms — `exhaustive` only flags them because
|
||||
`default-signifies-exhaustive` defaults to false.
|
||||
**Conditions**: Always, but especially for linters whose defaults assume a
|
||||
different project convention.
|
||||
**Anti-pattern**: Adding `if !ok { ... }` branches or empty `case:` arms that
|
||||
cannot execute. That raises the score and lowers the code.
|
||||
**Metric delta**: 3 of 16 candidate linters dropped from the plan (0 gained,
|
||||
real regressions avoided).
|
||||
|
||||
## Lesson 3 — iterations 1, 4
|
||||
**Pattern**: Pair the metric drop with a mechanically provable defect: write the
|
||||
regression test, commit, then revert *only* the source files and require the
|
||||
test to fail (`.auto/prove_fix.sh`).
|
||||
**Why it worked**: It caught a live bug that no counter measures — a
|
||||
singleflight body capturing the first caller's request context, so one
|
||||
disconnecting browser poisoned every concurrent request for that image.
|
||||
Iteration 4 kept debt flat at 93 yet was the most valuable change so far.
|
||||
**Conditions**: Every behavioural fix. A change that survives its own revert is
|
||||
not a fix, it is a rename.
|
||||
**Anti-pattern**: Calling something "hardening" without a test that fails
|
||||
without it.
|
||||
**Metric delta**: 0 for the proven bug (kept under the fix gate), 8 for the rest.
|
||||
|
||||
## Lesson 4 — iteration 5
|
||||
**Pattern**: Strengthen the architecture gate; it is a generator of real,
|
||||
previously invisible debt.
|
||||
**Why it worked**: `check_cordis_architecture.sh` only grepped `go func(`, so
|
||||
`go w.run()` — the shape used by four long-lived cleanup loops — passed
|
||||
silently, each one able to take down the process on a panic. Widening the
|
||||
pattern surfaced them immediately.
|
||||
**Conditions**: Whenever a gate has been green for a long time. A green gate
|
||||
proves the checks exist, not that they cover anything.
|
||||
**Anti-pattern**: Weakening `.golangci.yml` (blocked outright by the Guard via
|
||||
`check_gate_weaken.py` + a SHA lock on the yardstick).
|
||||
**Metric delta**: 4 uncovered crash-on-panic sites hardened.
|
||||
|
||||
## Lesson 5 — iteration 3
|
||||
**Pattern**: Deduplicate by extracting the shared *classification*, not the
|
||||
shared *response*.
|
||||
**Why it worked**: Two handlers mapped upload-lookup errors with copy-pasted
|
||||
blocks that had quietly drifted (different fallback status, different synonym
|
||||
constant for the same message). `filesrv.AbortUploadRecordError` handles the
|
||||
200/400 branches, and each endpoint keeps its own fallback it can still
|
||||
justify. Deleting the orphaned `ErrInvalidUploadID` constant was part of the
|
||||
change, not extra cleanup.
|
||||
**Conditions**: Duplicated error-mapping or validation blocks in sibling handlers.
|
||||
**Anti-pattern**: Silently unifying HTTP status codes across endpoints to make a
|
||||
helper fit — that is a behaviour change wearing a refactor's clothes.
|
||||
**Metric delta**: -2.
|
||||
|
||||
## Lesson 6 — iterations 15-21
|
||||
**Pattern**: Delegate a broad read-only audit for what mechanical gates cannot
|
||||
see (N+1s, locks held over I/O, resource leaks, layering), then re-verify each
|
||||
claim yourself before touching code.
|
||||
**Why it worked**: The audit produced the run's best findings — the per-request
|
||||
CORS database query, the orphan cron dispatching to a task nobody registered,
|
||||
media temp dirs nothing ever removed. It also produced a wrong one: it asserted
|
||||
telebot falls back to `http.DefaultClient` with no timeout, when telebot itself
|
||||
constructs a client with a one minute deadline. Acting on that would have added
|
||||
a tunable dressed up as a bug fix.
|
||||
**Conditions**: Whenever the committed gates are green and the easy signal is
|
||||
exhausted.
|
||||
**Anti-pattern**: Trusting an audit summary's file:line as evidence. One
|
||||
referenced file did not exist.
|
||||
**Metric delta**: 0 for three landed fixes (all kept under the proven-fix gate),
|
||||
but they were the run's highest-impact changes.
|
||||
|
||||
## Lesson 7 — iteration 16
|
||||
**Pattern**: Prove query-reduction with a functional test double that counts
|
||||
loader invocations, and assert the counter for both the batch and the looped
|
||||
form in the same test.
|
||||
**Why it worked**: Asserting "1 query" alone is vacuous — it also passes when
|
||||
nothing ran. Asserting batch=1 and per-id=3 in one test makes the instrument
|
||||
itself checked, so the claim cannot silently degrade.
|
||||
**Conditions**: Any change whose whole value is doing less I/O.
|
||||
**Anti-pattern**: Fixing an N+1 by reaching around the contract into another
|
||||
plugin's repository. The layering was the reason the slow path existed; the
|
||||
right move was to extend the contract with a batch method.
|
||||
**Metric delta**: 0 (kept under the proven-fix gate).
|
||||
|
||||
## Lesson 8 — iterations 17-22
|
||||
**Pattern**: Strengthen a gate only alongside the code that satisfies it, and
|
||||
never rewrite history in a shared worktree.
|
||||
**Why it worked**: Deleting 24 dead lint suppressions paid off exactly as the
|
||||
self-correcting design predicted: two of them were load-bearing under the
|
||||
project's own gate even though the analyzer called them unused, the Guard
|
||||
vetoed, and their removal surfaced two verified `contextcheck` false positives
|
||||
worth documenting instead of silently swallowing. Meanwhile a concurrent
|
||||
session was committing plan documents in the same tree, so `git add -A` swept
|
||||
one of its in-flight edits into my commit — unfixable by rebase without
|
||||
destroying their work, so the repair was to stage explicit paths from then on.
|
||||
**Conditions**: Always, in this repo. Assume another agent is editing `docs/`
|
||||
and `backend/core` concurrently.
|
||||
**Anti-pattern**: `git add -A` outside the first setup commit. Also: trusting
|
||||
"unused directive" as "safe to delete" — check the strictest config, not just
|
||||
the pinned yardstick.
|
||||
**Metric delta**: -25 in one iteration.
|
||||
|
||||
## Lesson 9 — iterations 23-24
|
||||
**Pattern**: Cross-check every service a plugin's `Apply` reads out of the
|
||||
container against what that plugin's `Inject()` declares. `Inject()` is the only
|
||||
thing `App.reconcileLocked` gates on, so anything consumed as a *value* at Apply
|
||||
time but left undeclared is resolved from a container that may not hold it yet.
|
||||
**Why it worked**: It found the run's worst defect, invisible to every
|
||||
mechanical gate: `user` declared only `DBService` while capturing
|
||||
`contracts.AuthService` to build its route guard, and `cmd/app.go` lists `user`
|
||||
before `auth`. Because user's dep set is a strict subset of auth's and it sits
|
||||
earlier in the slice, user *always* mounts first — deterministically, not a
|
||||
race — so `loginMW` fell back to a `c.Next()` closure and
|
||||
`/api/v1/user/{change-password,profile,access-tokens}` mounted unguarded. The
|
||||
same lookups in `admin` read a package global that its own `OnDispose` nils, so
|
||||
in-flight requests fail open during dispose.
|
||||
**Conditions**: Any Cordis plugin whose Apply assigns a contract result to a
|
||||
variable used later (middleware, handler closures). Services bound through
|
||||
`core.When` late binding are exempt — that is the correct pattern for genuinely
|
||||
late deps, so do not blanket-declare everything.
|
||||
**Anti-pattern**: Assuming a checked `x, ok :=` assertion is safe. All three
|
||||
plugins used the checked form and all three failed *open* — checked syntax,
|
||||
unchecked semantics.
|
||||
**Metric delta**: 0 across both iterations (kept under the proven-fix gate),
|
||||
but this is the run's highest-severity finding. `RouterRegistry` records each
|
||||
route's `Handlers`/`Middlewares`, which makes "is this route actually guarded?"
|
||||
directly assertable from the route table — the cheapest available oracle for
|
||||
security properties here.
|
||||
|
||||
## Lesson 10 — iteration 23 review
|
||||
**Pattern**: When the remaining metric is dominated by a positional or
|
||||
taste-based analyzer, say so and refuse to spend iterations on it.
|
||||
**Why it worked**: `funcorder` was 21 of 54 findings (39%) — pure function
|
||||
*ordering within a file*. Reordering private helpers to the bottom of a file
|
||||
moves the number and changes nothing a reader or the machine cares about, which
|
||||
is Lesson 1's "looks productive while shuffling strings" with a different label.
|
||||
Skipping it kept the loop honest. Triage also cleared 12 of 13
|
||||
`forcetypeassert` (guarded by construction) and 2 of 3 `unparam` (deliberate
|
||||
constructor symmetry behind one factory switch).
|
||||
**Conditions**: Whenever one linter dominates a shrinking total, break the count
|
||||
down per linter *before* picking a hypothesis.
|
||||
**Anti-pattern**: Treating a large single-linter share as an easy win. Real
|
||||
headroom at this point is ~10 findings, so a plateau in `debt` no longer means a
|
||||
stalled loop.
|
||||
**Metric delta**: 0 spent, ~21 findings deliberately left in place.
|
||||
|
||||
## Lesson 11 — iterations 24-25
|
||||
**Pattern**: Run the Guard after every single commit, and confirm which commit a
|
||||
proof script is actually reverting against.
|
||||
**Why it worked**: Four `staticcheck ST1023` findings from iteration 24 shipped
|
||||
straight through `go build ./...` and a green 47-package `go test ./...` —
|
||||
neither runs the project linter, so only `checks.sh` section 3 catches them.
|
||||
Separately, `prove_fix.sh` reverts to `HEAD^`; appending the iteration-23 log
|
||||
commit shifted `HEAD^` to the *fixed* state and reported "PROVE FAILED: tests
|
||||
still pass without the fix" on a genuinely load-bearing fix. Re-checking against
|
||||
the explicit pre-fix commit (`git checkout <sha> -- <files>`) showed the real
|
||||
answer. A false negative here is worse than no proof: it reads like the fix was
|
||||
cosmetic.
|
||||
**Conditions**: Always. Also note zsh does not word-split unquoted variables, so
|
||||
`git checkout $FILES` passes one bogus pathspec and silently reverts nothing —
|
||||
the command still exits 0.
|
||||
**Anti-pattern**: Batch-verifying at ship time. And any shell loop built on the
|
||||
bash word-splitting habit in this environment.
|
||||
**Metric delta**: -0, 1 wrong verdict corrected.
|
||||
|
||||
## Lesson 12 — iterations 27-32
|
||||
**Pattern**: Two things produced every substantive win: (1) find a place where
|
||||
correctness rests on a *prose comment* instead of an enforced constraint, and (2)
|
||||
find immutable startup work being redone inside a request path.
|
||||
**Why it worked**: Lint cannot see either class, so `debt` barely moved while real
|
||||
defects did. The comment "field comes from call sites, never from user input" sat
|
||||
on a function that interpolated its column argument straight into `WHERE` — the
|
||||
tautology payload executed and returned a row with `err=<nil>`, a filter bypass,
|
||||
not a hypothetical. The comment "contracts are pure abstractions" sat on a DTO
|
||||
carrying `TableName()`, which is exactly the handle four plugins used to read
|
||||
`w_users` instead of calling `UserService`. On the second pattern, three packages
|
||||
each re-normalised and re-split static whitelist patterns per request: hoisting
|
||||
that to registration cut 14 allocs/op to 1.
|
||||
**Conditions**: Any exported function taking a string that reaches SQL, a path
|
||||
matcher, or a shell. Any loop over configuration inside a request handler.
|
||||
**Anti-pattern**: Believing `nolintlint`'s "unused directive" means "safe to
|
||||
delete" — hit twice now, and the project gate vetoed it both times. Also believing
|
||||
a doc comment's self-assessment: verify the claim or leave it alone.
|
||||
**Metric delta**: 64 -> 63 across five keeps. Four of the five kept changes had
|
||||
delta 0. Under a pure-debt loop this run would have looked stalled while fixing a
|
||||
security bypass and a hot-path allocation bug.
|
||||
|
||||
## Standing notes
|
||||
- **The golangci-lint cache is machine-wide** (`~/.cache/golangci-lint`), so a
|
||||
sibling worktree analysing identical sources replays here carrying *that*
|
||||
checkout's absolute paths — 12 of 63 findings pointed outside the repo, which
|
||||
misattributes findings and can serve a stale Guard verdict. `measure.sh` and
|
||||
`checks.sh` now key `GOLANGCI_LINT_CACHE` per checkout (iteration 31). It is
|
||||
count-neutral (cold and warm both 63), but check path attribution before
|
||||
trusting any finding's location.
|
||||
- **Do not delegate a repo-wide audit to one subagent.** Both broad audits
|
||||
(architecture, bugs/perf) hit the 150-turn cap after ~45M tokens combined and
|
||||
returned nothing usable. Everything this run found came from targeted inline
|
||||
greps followed by reading the specific function. If delegating, bound it to one
|
||||
package cluster and a small finding budget.
|
||||
- Run decisions for this run: real defects first with `debt` as a secondary gate,
|
||||
commits directly on `main`, small file moves allowed but large package
|
||||
restructuring goes to a written proposal first.
|
||||
- Upstream moves fast in this repo: `origin/main` gained 11 commits mid-run
|
||||
(Cordis config extension point — `ctx.Config().Bind`, `DeclareConfig()`,
|
||||
`core.ConfigGatedPlugin`), which raised measured `debt` 54 -> 64 and
|
||||
`nolint_dirs` 72 -> 73 on its own. Rebase early and re-run the Guard after;
|
||||
a clean rebase does not mean a green one.
|
||||
- `cmd.TestNewWaveletAppWithRedisEnabled` needs a live Redis on
|
||||
`127.0.0.1:6379` and fails without one. Pre-existing on `origin/main`, so
|
||||
`tests_passed` 46 vs 47 is environmental, not a regression. Confirm against a
|
||||
scratch `git worktree` of `origin/main` before blaming a change for it.
|
||||
- Repo facts: backend module rooted at `backend/`, gofumpt orders a single
|
||||
import group as `Wavelet/...` before stdlib (uppercase sorts first); new Go
|
||||
files need the Apache license header or `scripts/update_go_license.sh --check`
|
||||
fails the Guard.
|
||||
- Handler edits require `make swagger` (cheap: it regenerates identical docs
|
||||
when only bodies change).
|
||||
- Dead suppressions are tracked by the `nolint_dirs` counter; removing one that
|
||||
is still needed re-raises the original finding, so the metric self-corrects.
|
||||
24 were removed in iteration 22; 72 remain, each still doing work (73 after
|
||||
the upstream rebase).
|
||||
|
||||
@@ -0,0 +1,12 @@
|
||||
# Autoresearch baseline — captured at iteration #0 (2026-08-29).
|
||||
# The Guard compares live values against these floors; the PRIMARY metric is debt.
|
||||
BASE_DEBT=102
|
||||
BASE_NOLINT=96
|
||||
BASE_TESTS_PASSED=46
|
||||
BASE_TEST_FUNCS=232
|
||||
BASE_TEST_FILES=76
|
||||
BASE_ARCH_VIOL=0
|
||||
BASE_COVERAGE=34.09
|
||||
|
||||
# SHA-256 of the pinned yardstick config. Guard aborts if it changes.
|
||||
REF_SHA=e881bda167bd688489f1356b7cd4056b8a6960f48b6778b095bdd2e44f627b82
|
||||
@@ -0,0 +1,117 @@
|
||||
#!/usr/bin/env python3
|
||||
"""Anti-cheat: prove .golangci.yml was only ever strengthened, never weakened.
|
||||
|
||||
Compares the live gate against the immutable snapshot taken at run start.
|
||||
Exits non-zero with a reason if any hardening rule is violated.
|
||||
"""
|
||||
|
||||
import os
|
||||
import sys
|
||||
|
||||
import yaml
|
||||
|
||||
ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
|
||||
BASELINE = os.path.join(ROOT, ".auto", "gate.baseline.yml")
|
||||
LIVE = os.path.join(ROOT, ".golangci.yml")
|
||||
|
||||
# threshold-like knobs: (path, direction) where direction "max" means the value
|
||||
# is an upper bound (smaller == stricter), "min" means a lower bound.
|
||||
STRICTNESS = [
|
||||
(("linters", "settings", "dupl", "threshold"), "max"),
|
||||
(("linters", "settings", "cyclop", "max-complexity"), "max"),
|
||||
(("linters", "settings", "cyclop", "package-average"), "max"),
|
||||
(("linters", "settings", "nestif", "min-complexity"), "min"),
|
||||
(("linters", "settings", "funlen", "lines"), "max"),
|
||||
(("linters", "settings", "funlen", "statements"), "max"),
|
||||
(("linters", "settings", "gocyclo", "min-complexity"), "min"),
|
||||
(("linters", "settings", "lll", "line-length"), "max"),
|
||||
]
|
||||
|
||||
|
||||
def load(path):
|
||||
with open(path, encoding="utf-8") as fh:
|
||||
return yaml.safe_load(fh) or {}
|
||||
|
||||
|
||||
def dig(doc, path):
|
||||
node = doc
|
||||
for key in path:
|
||||
if not isinstance(node, dict) or key not in node:
|
||||
return None
|
||||
node = node[key]
|
||||
return node
|
||||
|
||||
|
||||
def enabled_linters(doc):
|
||||
lint = doc.get("linters") or {}
|
||||
if lint.get("enable-presets"):
|
||||
return None # preset based; fall back to "any removal is suspicious"
|
||||
return set(lint.get("enable") or [])
|
||||
|
||||
|
||||
def main():
|
||||
try:
|
||||
base, live = load(BASELINE), load(LIVE)
|
||||
except OSError as exc:
|
||||
print(f"gate snapshot unreadable: {exc}")
|
||||
return 1
|
||||
except yaml.YAMLError as exc:
|
||||
print(f".golangci.yml is not parseable: {exc}")
|
||||
return 1
|
||||
|
||||
problems = []
|
||||
|
||||
base_lint, live_lint = base.get("linters") or {}, live.get("linters") or {}
|
||||
if (base_lint.get("default") or "none") != (live_lint.get("default") or "none"):
|
||||
problems.append("linters.default changed")
|
||||
|
||||
base_set, live_set = enabled_linters(base), enabled_linters(live)
|
||||
if base_set is None or live_set is None:
|
||||
if set((base.get("linters") or {}).get("enable-presets") or []) - set(
|
||||
(live.get("linters") or {}).get("enable-presets") or []
|
||||
):
|
||||
problems.append("an enable-preset was removed")
|
||||
elif dropped := base_set - live_set:
|
||||
problems.append(f"linters disabled: {sorted(dropped)}")
|
||||
|
||||
for path, direction in STRICTNESS:
|
||||
old, new = dig(base, path), dig(live, path)
|
||||
if old is None or new is None:
|
||||
continue
|
||||
try:
|
||||
old_f, new_f = float(old), float(new)
|
||||
except (TypeError, ValueError):
|
||||
continue
|
||||
if direction == "max" and new_f > old_f:
|
||||
problems.append(f"{'.'.join(path)} loosened {old} -> {new}")
|
||||
if direction == "min" and new_f < old_f:
|
||||
problems.append(f"{'.'.join(path)} loosened {old} -> {new}")
|
||||
|
||||
base_mnd = set(dig(base, ("linters", "settings", "mnd", "checks")) or [])
|
||||
live_mnd = set(dig(live, ("linters", "settings", "mnd", "checks")) or [])
|
||||
if base_mnd - live_mnd:
|
||||
problems.append(f"mnd checks dropped: {sorted(base_mnd - live_mnd)}")
|
||||
|
||||
issues_live = live.get("issues") or {}
|
||||
for key in ("exclude-rules", "exclude-patterns"):
|
||||
if issues_live.get(key) and not (base.get("issues") or {}).get(key):
|
||||
problems.append(f"issues.{key} added (suppresses reporting)")
|
||||
|
||||
for key in ("max-issues-per-linter", "max-same-issues"):
|
||||
old = (base.get("issues") or {}).get(key)
|
||||
new = issues_live.get(key)
|
||||
if old == 0 and new != 0:
|
||||
problems.append(f"issues.{key} no longer 0 — findings would be truncated")
|
||||
|
||||
# Exclusions expressed through the newer 'linters.exclusions' block.
|
||||
if (live_lint.get("exclusions") or {}) and not (base_lint.get("exclusions") or {}):
|
||||
problems.append("linters.exclusions added")
|
||||
|
||||
if problems:
|
||||
print("\n".join(f" - {p}" for p in problems))
|
||||
return 1
|
||||
return 0
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
sys.exit(main())
|
||||
@@ -0,0 +1,61 @@
|
||||
version: "2"
|
||||
|
||||
run:
|
||||
timeout: 5m
|
||||
tests: false
|
||||
|
||||
linters:
|
||||
default: none
|
||||
enable:
|
||||
# 基础检查
|
||||
- govet
|
||||
- staticcheck
|
||||
- errcheck
|
||||
- ineffassign
|
||||
- unused
|
||||
|
||||
# 代码坏味道
|
||||
- dupl # 重复代码
|
||||
- mnd # 魔法数字
|
||||
- goconst # 不必要的字符串常量
|
||||
- cyclop # 包/函数复杂度
|
||||
- nestif # if 嵌套太深
|
||||
- maintidx # 维护性指数
|
||||
- revive # 风格/命名/坏味道
|
||||
- gocritic # 各类代码问题
|
||||
- funlen # 函数过长
|
||||
|
||||
- gosec # 安全问题检查
|
||||
- bodyclose # HTTP response body 没有正确关闭
|
||||
- noctx # 没有传递 context.Context
|
||||
- contextcheck # 其他检查
|
||||
- sqlclosecheck # SQL rows 没有正确关闭
|
||||
- unconvert # 不必要的类型转换
|
||||
- nilerr # 函数返回 nil 错误
|
||||
|
||||
settings:
|
||||
dupl:
|
||||
threshold: 80
|
||||
|
||||
cyclop:
|
||||
max-complexity: 20
|
||||
package-average: 10
|
||||
|
||||
nestif:
|
||||
min-complexity: 5
|
||||
|
||||
funlen:
|
||||
lines: 200
|
||||
statements: 100
|
||||
|
||||
mnd:
|
||||
checks:
|
||||
- argument
|
||||
- condition
|
||||
- return
|
||||
|
||||
|
||||
# 完整上报所有问题（取消 golangci 默认 50/3 截断，保证 code-check 与度量真实）
|
||||
issues:
|
||||
max-issues-per-linter: 0
|
||||
max-same-issues: 0
|
||||
@@ -0,0 +1,78 @@
|
||||
version: "2"
|
||||
|
||||
# Pinned autoresearch yardstick. IMMUTABLE for the duration of a run.
|
||||
# Snapshot of the committed .golangci.yml plus the extra analyzers that report
|
||||
# genuine defects (correctness / panics / dead code) rather than cosmetics.
|
||||
# Keeping this separate from .golangci.yml means strengthening the project gate
|
||||
# can never silently lower the measured debt.
|
||||
|
||||
run:
|
||||
timeout: 5m
|
||||
tests: false
|
||||
|
||||
linters:
|
||||
default: none
|
||||
enable:
|
||||
# --- from the committed project gate ---
|
||||
- govet
|
||||
- staticcheck
|
||||
- errcheck
|
||||
- ineffassign
|
||||
- unused
|
||||
- dupl
|
||||
- mnd
|
||||
- goconst
|
||||
- cyclop
|
||||
- nestif
|
||||
- maintidx
|
||||
- revive
|
||||
- gocritic
|
||||
- funlen
|
||||
- gosec
|
||||
- bodyclose
|
||||
- noctx
|
||||
- contextcheck
|
||||
- sqlclosecheck
|
||||
- unconvert
|
||||
- nilerr
|
||||
# --- extra real-risk analyzers (defects, not cosmetics) ---
|
||||
- errorlint # err == / %v instead of errors.Is/As and %w
|
||||
- forcetypeassert # unchecked type assertions can panic
|
||||
- nilnil # (value, nil) breaks the nil-check contract
|
||||
- predeclared # shadowing builtins
|
||||
- unparam # dead params/results
|
||||
- wastedassign # dead stores
|
||||
- exhaustive # enum switches missing cases
|
||||
- makezero # append to preallocated slice
|
||||
- rowserrcheck # sql.Rows error after iteration
|
||||
- durationcheck # multiplied time.Duration
|
||||
- prealloc # slice growth in loops
|
||||
- copyloopvar # loop-var capture
|
||||
- nonamedreturns
|
||||
- funcorder # struct methods scattered across files
|
||||
- nolintlint # suppression audit (must stay 0)
|
||||
|
||||
settings:
|
||||
dupl:
|
||||
threshold: 80
|
||||
|
||||
cyclop:
|
||||
max-complexity: 20
|
||||
package-average: 10
|
||||
|
||||
nestif:
|
||||
min-complexity: 5
|
||||
|
||||
funlen:
|
||||
lines: 200
|
||||
statements: 100
|
||||
|
||||
mnd:
|
||||
checks:
|
||||
- argument
|
||||
- condition
|
||||
- return
|
||||
|
||||
issues:
|
||||
max-issues-per-linter: 0
|
||||
max-same-issues: 0
|
||||
@@ -0,0 +1,129 @@
|
||||
# Deferred proposals — autoresearch run (iterations 27-36)
|
||||
|
||||
Five verified findings deliberately **not** changed by the loop: each needs either a
|
||||
contract/API decision or a multi-package restructure, which this run was scoped to
|
||||
propose rather than perform. Evidence is from reading the cited files in this
|
||||
checkout at commit `ea97b64` plus iterations 27-35.
|
||||
|
||||
---
|
||||
|
||||
## P1 — Cross-driver storage migration cannot move objects (severity: data availability)
|
||||
|
||||
`plugins/domain/upload/task/storage_migration.go` computes `target` from the payload,
|
||||
then calls:
|
||||
|
||||
```go
|
||||
migrated, err := migrateObjects(ctx, storageSvc, storageSvc, total)
|
||||
```
|
||||
|
||||
`sourceBackend` and `targetBackend` are the **same** `contracts.StorageService`. That
|
||||
service resolves its backend per call and only ever to the currently active one
|
||||
(`plugins/infra/storage/plugin.go:89` → `s.backend` or `objectstore.Active(ctx)`), and
|
||||
the target config is persisted **after** the migration loop
|
||||
(`uploadstorage.SaveActiveConfig(ctx, target)`).
|
||||
|
||||
Consequence for a non-empty source: `migrateSingleObject` reads and writes the same
|
||||
backend; `shouldSkipMigration` finds every object already "present in the target" and
|
||||
skips it, yet `migrated` is still incremented, so the task returns
|
||||
`存储迁移完成，共迁移 N 个对象，活动存储已切换为 <driver>` having copied **zero** bytes,
|
||||
and then points the platform at an empty backend. The same-driver and
|
||||
`total == 0` branches are harmless and legitimately need no copying.
|
||||
|
||||
Why the tests miss it: `shared.MockStorageService` is one instance serving both
|
||||
parameters, so a copy-to-self looks correct.
|
||||
|
||||
Proposed fix (needs a contract decision — this is a feature, not a patch):
|
||||
1. Extend `contracts.StorageService` with the ability to operate against an explicitly
|
||||
supplied `StorageConfigDTO` (e.g. `BackendFor(ctx, cfg) (StorageReader, error)`),
|
||||
implemented in `plugins/infra/storage` where the `objectstore` backends live. They
|
||||
are unexported today and `plugins/domain/upload` must not import them (cross-plugin
|
||||
import ban), so the contract is the only correct route.
|
||||
2. In the task, build the target from `target` and pass distinct source/target.
|
||||
3. Only save the active config after a verified copy, and assert `src != dst` at
|
||||
entry.
|
||||
4. Interim safety option if a decision is needed sooner: make the
|
||||
`target.Driver != active.Driver && total > 0` branch return an explicit
|
||||
not-implemented error instead of reporting success. Rejected by this loop because
|
||||
it disables an advertised admin operation, which is a product call, and because
|
||||
the machinery it would strand (`migrateObjects`, `migrateSingleObject`,
|
||||
`shouldSkipMigration`) becomes dead code the project gate then rejects.
|
||||
|
||||
Size: contract + infra impl + task wiring + a two-backend test double. Roughly one
|
||||
focused session, not a loop iteration.
|
||||
|
||||
---
|
||||
|
||||
## P2 — `w_system_configs` has one migration owner and many writers (Cordis single-owner)
|
||||
|
||||
Owner per migrations: `plugins/domain/admin`. Still read/written with raw SQL from
|
||||
`plugins/domain/system/repository.go:31`, `plugins/domain/cap/repository.go:50`,
|
||||
`plugins/domain/auth/repository.go:122`, `plugins/domain/upload/storage/migration.go:96,108`,
|
||||
`plugins/domain/upload/ingest/helpers.go:55`,
|
||||
`plugins/domain/message_gateway/repository/push.go` and `plugins/drivers/driver_http`.
|
||||
|
||||
Iteration 34 fixed one instance of the real damage this causes (a failed read looked
|
||||
identical to "unconfigured", silently dropping notifications); iteration 35 fixed
|
||||
another (a failed read cached a narrowed whitelist for a whole TTL). The remaining
|
||||
sites carry the same trap.
|
||||
|
||||
Proposed fix: one settings accessor contract (`Get(ctx, key) (string, error)` /
|
||||
`GetAll(ctx, keys...)`) owned by the settings subsystem, then delete the raw table
|
||||
access. Keys should be declared where they are used rather than string-matched.
|
||||
Size: medium, touches seven plugins; do it key-group by key-group so each step is
|
||||
independently revertable.
|
||||
|
||||
---
|
||||
|
||||
## P3 — Two tables are modelled twice (schema drift hazard)
|
||||
|
||||
* `w_task_executions`: `plugins/domain/admin/model/entity.go:207` **and**
|
||||
`plugins/drivers/driver_asynq_worker/types.go:49`. The two `TaskExecution` structs
|
||||
and their status enums are byte-for-byte identical today.
|
||||
* `w_schedules`: `plugins/domain/admin/model/entity.go:169` **and**
|
||||
`plugins/drivers/driver_asynq_cron/schedule.go:25`.
|
||||
|
||||
Nothing is broken yet — that is the risk: the migration owner was only recently moved
|
||||
to `admin` (`49f9d10`), and a column added to one struct will silently diverge from
|
||||
the other, so whichever writer holds the stale struct zeroes or omits the new column.
|
||||
|
||||
Proposed fix: pick the single owner per P2's rules and have the other side go through
|
||||
a contract (execution recording already has DTOs in `contracts`), then delete the
|
||||
duplicate model. Consider a gate check rejecting two non-`testhelper` packages
|
||||
declaring the same `w_` table — it will fail until these two are resolved, so land it
|
||||
with the fix (the pattern that worked in iterations 5 and 19).
|
||||
|
||||
---
|
||||
|
||||
## P4 — `user` deletes rows from tables owned by `auth`
|
||||
|
||||
`plugins/domain/user/repository.go:327,330` issues `DELETE` against `w_access_tokens`
|
||||
and `w_external_accounts`, both owned and migrated by `plugins/domain/auth`, inside
|
||||
user deletion. It works, but ownership is inverted: revoke-on-delete is auth's
|
||||
invariant, and encoding it in `user` means any other deletion path silently skips it.
|
||||
|
||||
Proposed fix: emit a typed `user:deleted` event from `user` and let `auth` cascade
|
||||
within its own transaction boundary, or expose an explicit `AuthService.RevokeForUser`.
|
||||
Size: small-to-medium; needs a test that the revocation still happens on delete.
|
||||
|
||||
---
|
||||
|
||||
## P5 — Package `cap` shadows the predeclared identifier (8 of 62 debt)
|
||||
|
||||
Every file in `plugins/domain/cap` declares `package cap`, which shadows the builtin.
|
||||
It is the single largest block of non-cosmetic lint debt this run declined to chase,
|
||||
and it is also a readability cost (`cap.Something` reads as a builtin call).
|
||||
|
||||
Proposed fix: rename to a non-shadowing identifier (e.g. `capacity` / `proofwork`,
|
||||
matching what the plugin actually does) across its own files and importers. Mechanical
|
||||
but wide; needs a decision on the new name first, which is why it is not done here.
|
||||
|
||||
---
|
||||
|
||||
## Explicitly rejected as metric-chasing
|
||||
|
||||
23 `funcorder`, 5 `exhaustive` (both flagged only because
|
||||
`default-signifies-exhaustive` defaults to false), 4 `nonamedreturns` and the 17
|
||||
`forcetypeassert` cluster in `core/events.go` and `core/extpoints/config_resolve.go`
|
||||
— verified guarded by construction (`convertString` etc. return `(any, error)` and
|
||||
always yield the asserted type when `err == nil`). Reordering functions or adding
|
||||
unreachable `if !ok` branches would raise the score and lower the code.
|
||||
Executable
+57
@@ -0,0 +1,57 @@
|
||||
#!/bin/bash
|
||||
# Mechanically prove a FIX iteration is load-bearing.
|
||||
#
|
||||
# Usage: .auto/prove_fix.sh <package> <changed source file> [<more files>...]
|
||||
#
|
||||
# Run immediately AFTER committing the fix, with a clean worktree. It reverts
|
||||
# only the non-test source files to their pre-fix state (keeping the new test),
|
||||
# runs the package tests, and requires them to FAIL. Then it restores HEAD.
|
||||
# A fix nobody can break with a revert is not a fix.
|
||||
set -uo pipefail
|
||||
ROOT="$(cd "$(dirname "$0")/.." && pwd)"
|
||||
|
||||
if [ ! -z "$(git -C "${ROOT}" status --porcelain)" ]; then
|
||||
echo "PROVE ABORT: worktree must be clean (commit the change first)"
|
||||
exit 2
|
||||
fi
|
||||
|
||||
PKG="$1"; shift
|
||||
SRC_FILES=("$@")
|
||||
if [ "${#SRC_FILES[@]}" -eq 0 ]; then
|
||||
echo "PROVE ABORT: no source files given"
|
||||
exit 2
|
||||
fi
|
||||
|
||||
cd "${ROOT}/backend" || exit 2
|
||||
|
||||
restore() {
|
||||
git -C "${ROOT}" checkout HEAD -- "${SRC_FILES[@]}" 2>/dev/null
|
||||
}
|
||||
trap restore EXIT
|
||||
|
||||
for f in "${SRC_FILES[@]}"; do
|
||||
if git -C "${ROOT}" cat-file -e "HEAD^:${f}" 2>/dev/null; then
|
||||
git -C "${ROOT}" checkout "HEAD^" -- "${f}" || { echo "PROVE ABORT: cannot revert ${f}"; exit 2; }
|
||||
else
|
||||
# File did not exist before this commit — removing it is the revert.
|
||||
rm -f "${ROOT}/${f}"
|
||||
fi
|
||||
done
|
||||
|
||||
echo "--- tests against pre-fix source ---"
|
||||
OUT=$(go test -count=1 "${PKG}" 2>&1)
|
||||
RC=$?
|
||||
echo "${OUT}" | tail -15
|
||||
if [ "${RC}" -eq 0 ]; then
|
||||
echo "PROVE FAILED: tests still pass without the fix — this is not a real bug fix"
|
||||
exit 1
|
||||
fi
|
||||
if echo "${OUT}" | grep -q 'build failed'; then
|
||||
KIND="compile (signature changed; behaviour proven by inspection)"
|
||||
elif echo "${OUT}" | grep -qE '^--- FAIL'; then
|
||||
KIND="assertion"
|
||||
else
|
||||
KIND="failure"
|
||||
fi
|
||||
echo "PROVED: test fails without the fix (${KIND})"
|
||||
exit 0
|
||||
@@ -0,0 +1,37 @@
|
||||
iteration commit metric delta status guard description
|
||||
0 - 102 0.0 baseline pass initial measurement (pinned yardstick: repo gate + real-risk analyzers)
|
||||
1 1c5731b 100 -2.0 keep pass core: Using2/Using3 now wrap dependency causes via errors.Join (proven: test fails on revert)
|
||||
2 37ad586 95 -5.0 keep pass sentinel == comparisons -> errors.Is across admin/upload/cap-pow (5 sites)
|
||||
3 686e3ef 93 -2.0 keep pass filesrv.AbortUploadRecordError dedups error mapping + errors.As (2 sites, drops dead ErrInvalidUploadID)
|
||||
4 7e6b9e7 93 0.0 keep pass PROVEN FIX: singleflight image generation no longer dies with the first caller canceled ctx (test fails on revert)
|
||||
5 381c794 93 0.0 keep pass CORDIS: gate widened to catch bare "go call()" + 4 unprotected cleanup goroutines moved to util.Go (arch violations 4->0)
|
||||
6 ce33997 92 -1.0 keep pass BUGFIX admin logs: negative cursor was accepted (bool ignored by callers) -> error-only contract; proven via revert (compile-level) + contract test
|
||||
7 c66399e 89 -3.0 keep pass push channels share title/content/level extraction (3 dead inits gone, ~20 fewer lines)
|
||||
8 18820b1 89 0.0 keep pass BUGFIX push: synthesized notification content had random field order (map iteration); sorted keys, test observed failing pre-fix
|
||||
9 22ecafd 87 -2.0 keep pass unparam: always-nil error returns dropped, 4 unreachable branches removed
|
||||
10 c4068ef 84 -3.0 keep pass errorlint cleared to 0: %%w at push test + telegram fallback, errors.As in config loader
|
||||
11 3d2038a 80 -4.0 keep pass nilnil: unimplemented auth mocks now return a sentinel instead of (nil,nil)
|
||||
12 101cb2f 79 -1.0 keep pass nilnil: inproc driver GetExecution returns error, matching asynq driver semantics
|
||||
14 6932b54 79 0.0 keep pass DATA-LOSS BUGFIX: cache read error no longer clobbers buffered task log (proven: assertion fails on revert)
|
||||
15 2c41563 79 0.0 keep pass PERF: CORS origin check no longer hits DB per request (5s cached read); proven - loader count 0 vs 1 on revert
|
||||
16 976f9b1 79 0.0 keep pass PERF: contract-level batch user lookup replaces N+1 in access-log enrichment (test proves 1 query vs 3)
|
||||
17 1b1c452 79 0.0 keep pass BUGFIX: orphan cron message_gateway:cleanup_pairing_codes now has a handler; invariant test added (proven by stash-revert)
|
||||
18 8c4955c 79 0.0 keep pass BUGFIX: removed phantom user:daily_audit cron (dispatched to unregistered task); cross-plugin invariant test added
|
||||
19 84eaf3f 79 0.0 keep pass CORDIS+BUGFIX: task handlers were asynq-typed so 4 upload tasks could not run under the in-process worker; made driver-agnostic + gate check 7 (proven: gate names all 3 files pre-fix)
|
||||
20 efa7555 79 0.0 keep pass BUGFIX telegram: LongPoller.Timeout was 10 nanoseconds -> getUpdates timeout=0 -> busy polling; now 10s (proven by reverting the constant)
|
||||
21 1023fa3 79 0.0 keep pass DISK LEAK: telegram inbound media scratch dirs were never removed (no consumer reads them); cleanup on handler exit. No test possible (needs live download)
|
||||
22 ad83841 54 -25.0 keep pass dead lint suppressions removed (24); 2 were load-bearing -> restored+narrowed with reasons after guard veto exposed verified contextcheck FPs
|
||||
23 de938de 54 0.0 keep pass SECURITY/BUGFIX fail-open auth: user+message_gateway consumed contracts.AuthService in Apply but declared only DBService, so reconcile mounted user before auth and loginMW degraded to a pass-through (user change-password/profile/access-tokens unguarded in production, deterministically); declared the dep + added reconcile-level ordering test (PROVED: assertion fails on revert)
|
||||
24 62b48e9 54 0.0 keep pass SECURITY: all three auth-middleware fallbacks were c.Next() (fail-open). Reachable at runtime in admin: OnDispose->ResetServices() nils the global the per-request guard reads, so in-flight requests pass as authenticated. Added ginutil.AuthUnavailable() + table test driving each registered guard (PROVED: abort assertion fails on revert to 577d795)
|
||||
25 f58f5a4 54 0.0 keep pass staticcheck ST1023 x4 from iter 24 (redundant gin.HandlerFunc on typed-RHS decls) - caught by GUARD only, go build/go test both stayed green; lesson: run checks.sh after EVERY commit, not just before ship
|
||||
26 - 64 +10.0 rebaseline pass upstream config-extension + auth/user/task work raised debt 54->64; re-measured at HEAD ea97b64, 47 pkgs pass, arch 0 viol. Run focus agreed: real defects primary, debt secondary (proven-fix gate keeps delta-0 fixes)
|
||||
27 31f3af6 63 -1.0 keep pass dead contextcheck suppression on cmd.newWaveletApp removed; the core.App.Run one was load-bearing (guard veto: project gate contextcheck Run->Start, verified FP on variadic ctx) -> restored narrowed + documented. Lesson 8 trap re-hit: nolintlint "unused" != safe to delete
|
||||
28 5037097 63 0.0 discard fail CORDIS gate: contracts DTO must not carry TableName + removed UserDTO.TableName(). DISCARDED: my grep used -g !*_test.go and missed upload/handler/routers_test.go:669 which does db.Create(&contracts.UserDTO{}) into w_users - that suppression exists precisely to enable the cross-plugin write. Lesson: contracts-purity changes must scan test files too.
|
||||
29 2ff0cb8 63 0.0 keep pass CORDIS contracts purity: gate check 2.2 forbids TableName()/gorm tags in core/contracts + removed UserDTO.TableName(); upload/handler test now seeds via explicit .Table("w_users") (precedent: filesrv test). Proven twice over: gate named auth.go:33 pre-fix, and iter-28 revert broke 1 package without the test fix. Delta-0 keep under the agreed real-defect gate
|
||||
30 9ea0e2b 63 0.0 keep pass SECURITY (assertion-proven): FindUserByFieldRecord interpolated its column arg into WHERE with only a prose comment as guard. Pre-fix the tautology "username = '' OR 1=1 --" EXECUTED and returned a row with err=<nil> (filter bypass). Now an allow-list rejects before GetDB. Also first test in the repository pkg: tests_passed 47->48, funcs 282
|
||||
31 5193bd0 63 0.0 keep pass HARNESS INTEGRITY: measure.sh and checks.sh now key GOLANGCI_LINT_CACHE per checkout. The default cache is machine-wide, so entries written by a sibling worktree replayed here carrying ITS absolute paths (12 of 63 lines pointed at an outside checkout), misattributing findings and risking a stale Guard verdict. Proven count-neutral: cold and warm both 63; foreign paths now 0. Delta 0 by design, kept under the agreed real-defect gate
|
||||
32 f7a86d3 63 0.0 keep pass PERF+DEDUP: three packages hand-rolled mutex+[]string+MatchPathPattern loop, re-normalising and re-splitting immutable patterns per request. New extpoints.PathWhitelist compiles patterns at registration and absorbs all three. Mechanically asserted: 14 allocs/op -> 1 allocs/op; equivalence test pins Match against the legacy loop over a full pattern x path matrix; race-clean. funcs 282->288, coverage 35.02
|
||||
33 99fca9e 62 -1.0 keep pass BUGFIX+DEDUP: task.loadActiveStorageConfig and saveActiveStorageConfig duplicated uploadstorage.LoadStorageConfig/SaveActiveConfig but swallowed all three failures (nil db, read error, json parse) returning zero config + nil error, making the caller-s already-written error branch dead: a storage migration could run from an unknown active driver. Now routed through the canonical accessors; regression test corrupts the stored config and asserts Execute errors (assertion-proven via revert). funcs 289
|
||||
34 b22f863 62 0.0 keep pass BUGFIX+PERF: LoadSMTPConfigRecord fired four single-key queries and discarded every error with underscore assignment, so an unreadable w_system_configs returned four blank strings both callers could only read as "SMTP not configured" -> notification silently dropped. Now one IN query plus a real error channel; callers log at the boundary and keep their own values. Proof is signature-level and exact: the old API had no error return, so the failure was unrepresentable. 4 queries -> 1, funcs 291
|
||||
35 d7c851b 62 0.0 keep pass BUGFIX+PERF: access_cache discarded the whitelist read error with underscore assignment, then unconditionally set valid=true and CheckedAt=now, so one transient DB failure pinned the RESTRICTED default public-access list for the whole TTL and silently narrowed an admin-configured whitelist. Now the error is logged, last-good is served when known, and a cold failure stays invalid so the next request retries. Assertion-proven by dropping and restoring the table mid-test. funcs 292
|
||||
36 - 62 0.0 keep skip PROPOSALS (.auto/proposals.md): five verified items deliberately not auto-fixed per the agreed scope. Headline P1: cross-driver storage migration passes the SAME service as source and target (getBackend only ever resolves the active backend) and saves the target config afterwards, so it reports "migrated N objects" having copied zero bytes and then points the platform at an empty backend. Needs a contracts.StorageService capability decision, not a patch.
|
||||
|
Can't render this file because it contains an unexpected character in line 7 and column 63.
|
Reference in New Issue
Block a user