diff --git a/.auto/proposals.md b/.auto/proposals.md new file mode 100644 index 00000000..8f40d735 --- /dev/null +++ b/.auto/proposals.md @@ -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 个对象，活动存储已切换为 ` 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. diff --git a/.auto/results.tsv b/.auto/results.tsv index 46c64258..d4a89235 100644 --- a/.auto/results.tsv +++ b/.auto/results.tsv @@ -34,3 +34,4 @@ iteration commit metric delta status guard description 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.