Skip to content

remediation-audit.md

Latest commit

 

History

History
216 lines (137 loc) · 11.4 KB

remediation-audit.md

File metadata and controls

216 lines (137 loc) · 11.4 KB
Raw

Remediation audit report

Дата: 2026-05-26
Область: Phases C, E, F, G из .sisyphus/plans/remediation-roadmap.md
Источники ревью: основной аудит, @architect, @go-reviewer
Текущий статус: follow-up remediation implemented, документация обновлена Последняя полная верификация implementation follow-up: go test -race -count=1 ./... PASS, go vet ./... PASS, LSP 0 errors

Executive summary

Первичный аудит нашёл контрактные расхождения в Phases E, F и G. Последующий remediation follow-up закрыл основные блокеры:

  • backend.Registry теперь владеет conversion backends и processors на уровне instance scope.
  • Route policy применяется не только к preview, но и к actual CLI/API conversion execution.
  • API error envelope поддерживает typed validation details.
  • /api/v1 naming convention оформлена как compatibility policy без breaking response renames.
  • Plugin protocol v2 включает manifest version, diagnostics, conversion metadata, operations и explicit rejection для future protocol versions.
  • Engine.Route, request normalisation и plugin processing получили robustness fixes.

Итог: Phases C, E, F и G можно считать закрытыми для текущего follow-up scope. Оставшиеся пункты ниже отделены как deferred findings и не блокируют закрытие remediation follow-up.

Phase status after follow-up

Phase Plan intent Current status Verdict
C Instance-based processor registry, isolated tests, no hidden global lookup Implemented через unified backend.Registry PASS
E API contract hardening: error envelope, naming convention, docs, contract tests, server modes Error details implemented. Naming documented as v1 compatibility policy. PASS with deferred public API posture work
F Plugin protocol v2: protocol version, metadata, operations, compatibility v2 manifest metadata, diagnostics, operations and future-version rejection implemented. v1 bare arrays remain supported. PASS
G Quality-aware routing policy for speed/quality/avoid-lossy/max-hops Route preview and actual conversion execution use the same policy semantics. PASS

Confirmed plan-vs-implementation audit

Phase C: Instance-based ProcessorRegistry

Requirement Status Evidence
Export processor registry behavior PASS Processor methods live on backend.Registry: RegisterProcessor, FindProcessor, AllProcessors, ProcessorsByOp, AllOperations in internal/backend/registry.go.
Engine no longer depends on global processor lookup PASS Engine.Process uses e.registry.FindProcessor(req.Op) in internal/core/engine.go.
Keep global wrappers for compatibility PASS internal/backend/processor.go delegates wrappers to DefaultRegistry.
Isolated tests PASS Instance-scoped registry tests use independent backend.NewRegistry() values.
Unified composition root for built-ins DEFERRED Built-ins still use blank imports and DefaultRegistry. This is compatible with current CLI behaviour.

Verdict: core Phase C acceptance criteria are met. The implementation chose one unified Registry rather than a separate ProcessorRegistry; README now documents that decision.

Phase E: API contract hardening

Requirement Status Evidence
Unified error envelope PASS internal/server/errors.go implements {error:{code,message,details}} with details,omitempty.
Typed validation details PASS apiErrorDetail exposes field and message; apiValidationDetailsFromError maps validation errors into details.
JSON naming convention PASS /api/v1 keeps existing response names for compatibility; new request fields use snake_case.
API docs PASS docs/api.md documents error details, route policy inputs and naming compatibility.
Contract tests PASS internal/server/contract_test.go covers the public contract.
Local UI vs public API mode split DEFERRED ServerModePublicAPI exists, but broader public-service posture is future work.
Avoid leaking internal processor errors PASS internal/server/process.go maps processor failures to generic process failed.

Verdict: Phase E follow-up items are closed. Full response renaming is intentionally deferred to a future versioned API or schema migration.

Phase F: Plugin protocol v2

Requirement Status Evidence
protocol_version support PASS pkg/plugin/protocol.go defines Manifest.ProtocolVersion; object manifests without the field default to v2.
quality, lossy, generic metadata PASS V2Capability includes Quality, Lossy, Metadata.
version, diagnostics metadata PASS Manifest.Version and Manifest.Diagnostics are explicit v2 fields.
Processor operations PASS Operation and plugin processors support process operations.
v1 backward compatibility PASS Bare array capabilities parse as protocol version 1.
Future protocol rejection PASS protocol_version > 2 returns an unsupported protocol error.

Verdict: v2 protocol is documented and backward compatible. v1 remains conversion-only by design.

Phase G: Quality-aware routing

Requirement Status Evidence
RoutePolicy with mode, avoid-lossy, max-hops PASS internal/router/route.go defines RoutePolicy; default policy is speed with effective MaxHops 4.
Use Quality and Lossy in routing PASS internal/router/dijkstra.go applies quality/cost-aware routing.
Route preview exposes cost/quality/lossy PASS internal/core/engine.go returns route metadata via RouteWithPolicy.
API policy selection PASS GET /api/v1/routes accepts mode or policy, avoid_lossy, and max_hops.
CLI conversion policy selection PASS internal/cli/convert.go exposes --route-policy, --avoid-lossy, and --max-hops; jobs use FindRouteWithPolicy.
API conversion policy selection PASS POST /api/v1/convert accepts nested route_policy and top-level compatibility fields; queued conversions pass policy to Engine.Convert.

Verdict: Phase G blocker is closed. Preview and actual conversion execution now share route policy semantics.

Historical findings closed by follow-up

HIGH-1: Quality-aware routing was not wired into actual conversion execution

Status: CLOSED.

The original issue was that route preview could choose a quality-safe path while CLI/API conversion used default routing. Current code routes CLI jobs with FindRouteWithPolicy, carries API route policy through the conversion queue, and Engine.Convert calls FindWithPolicy with ConvertRequest.RoutePolicy.

HIGH-2: Error envelope lacked details

Status: CLOSED.

apiErrorBody now includes Details []apiErrorDetail with json:"details,omitempty". Validation helpers produce typed {field,message} entries.

HIGH-3: JSON naming convention remained mixed

Status: CLOSED AS COMPATIBILITY DECISION.

The remaining mixed response field names are preserved intentionally for /api/v1 compatibility. New request fields use snake_case. Full response renaming is deferred to a future versioned API or schema migration.

HIGH-4: Engine.Route ignored caller context

Status: CLOSED.

Engine.Route now passes the caller context to RouteWithPolicy.

MEDIUM-1: Plugin protocol v2 was incomplete relative to roadmap

Status: CLOSED.

The v2 manifest now includes explicit version, diagnostics, operation metadata and future protocol rejection. README documents v2 first and keeps v1 compatibility notes.

MEDIUM-8: pluginProcessor.Process could set input path as output path

Status: CLOSED.

Plugin processing follow-up fixed output handling so missing output contracts do not silently reuse an input path as the output path.

MEDIUM-9: normalizeProcessRequest used shallow copies for slices/maps

Status: CLOSED.

Process request normalisation now avoids shared mutable backing state for request slices/maps.

LOW-5: README plugin documentation was stale

Status: CLOSED.

README now documents protocol v2 manifests, operations, process, quality, lossy metadata, v1 limitations and future protocol rejection.

Deferred findings

These findings remain useful, but they are outside the completed remediation follow-up scope.

DEFERRED-1: Public API mode is mostly UI toggle, not a public-service posture

Source: @architect
Files: internal/server/server.go, internal/server/middleware.go, internal/cli/serve.go

Follow-up options:

  1. Make CLI --api-only use WithServerMode(ServerModePublicAPI) explicitly if that is the intended contract.
  2. Decide whether public mode should default to stricter CORS.
  3. Document auth, rate limit and CORS production readiness before public hosting.

DEFERRED-2: Long conversion holds Engine read lock

Source: @go-reviewer
File: internal/core/engine.go

Engine.Convert can still hold the route graph read lock through runner execution. This does not block current functionality, but graph rebuild latency could improve if the route is resolved under lock and execution happens after releasing it.

DEFERRED-3: /api/v1/process is synchronous and not queued

Source: @go-reviewer
File: internal/server/process.go

Consider routing process operations through the same bounded job queue model as conversions if processor operations become long-running or public-facing.

DEFERRED-4: JobManager can grow without automatic pruning

Source: @go-reviewer
File: internal/server/jobs.go

PruneJobs exists but is not automatically wired. A periodic cleanup goroutine or cleanup manager hook would make retention predictable.

DEFERRED-5: Low package coverage in backend packages

Source: @go-reviewer

Add targeted tests for registry edge cases, concurrent register/find snapshots and backend command construction with mocks.

DEFERRED-6: Plugin probe failures are skipped during discovery

Source: @go-reviewer
File: internal/backend/backends/plugin/plugin.go

Log warning-level diagnostics with plugin name and probe error to improve troubleshooting.

DEFERRED-7: Built-in composition root remains distributed through blank imports

Source: @architect
Files: cmd/convertr/main.go, cmd/convertr/backends_darwin.go, internal/cli/serve.go

Consider a single internal composition function, for example internal/appregistry.RegisterBuiltins(reg *backend.Registry), if built-in registration drift becomes a problem.

DEFERRED-8: Minor Go-style and validation wording cleanups

Source: @go-reviewer

  • validateOutputName can return more precise messages for . and .. cases.
  • priorityQueue.Push/Pop can use any instead of interface{}.
  • Plugin unsupported operation errors can include the plugin name.

Post-follow-up verification

To re-run the implementation verification:

go test -race -count=1 ./...
go vet ./...

For this documentation-only update, the required formatting sanity check is:

git diff --check

Final verdict

The remediation follow-up is complete for the approved scope. The audit remains as historical context, while active unresolved work is limited to the deferred findings above.