feat(rfc): Pass Nine — what's missing, what shouldn't be there
P9-1: core/go imports os/exec in app.go — violates its own rule P9-2: reflect used for service name — magic, fragile on pkg rename P9-3: No JSON primitive — every consumer imports encoding/json P9-4: No time primitive — 3 different timestamp formats P9-5: No ID generation — 3 different patterns (rand, counter, fmt) P9-6: No validation primitive — path traversal check copy-pasted 3x P9-7: Error codes exist but nobody uses them P9-8: No health/observability primitive — go-process has it per-daemon only Key finding: core/go imports os/exec (P9-1), violating the rule it established in Section 17. App.Find() uses exec.LookPath — a process concern that belongs in go-process. Missing primitives: ID generation, validation, health checks. Judgment calls: JSON wrapping (maybe noise), time formatting (convention). Nine passes, 72 findings, 3,100+ lines. Co-Authored-By: Virgil <virgil@lethean.io>
This commit is contained in:
parent
c847b5d274
commit
a06af7b6ad
1 changed files with 139 additions and 0 deletions
139
docs/RFC.md
139
docs/RFC.md
|
|
@ -2909,6 +2909,145 @@ The guardrail primitives (Array[T], Registry[T], ConfigVar[T]) are Core's answer
|
|||
|
||||
---
|
||||
|
||||
## Pass Nine — What's Missing, What Shouldn't Be There
|
||||
|
||||
> Ninth review. What should Core provide that it doesn't? What does Core
|
||||
> contain that belongs elsewhere?
|
||||
|
||||
### P9-1. core/go Imports os/exec — Violates Its Own Rule
|
||||
|
||||
`app.go` imports `os/exec` for `exec.LookPath`:
|
||||
|
||||
```go
|
||||
func (a App) Find(filename, name string) Result {
|
||||
path, err := exec.LookPath(filename)
|
||||
```
|
||||
|
||||
Core says "zero os/exec — go-process is the only allowed user" (Section 17). But Core itself violates this. `App.Find()` locates binaries on PATH — a process concern that belongs in go-process.
|
||||
|
||||
**Resolution:** Move `App.Find()` to go-process or make it a `process.which` Action. Core shouldn't import the package it tells consumers not to use.
|
||||
|
||||
### P9-2. core/go Uses reflect for Service Name Discovery
|
||||
|
||||
```go
|
||||
typeOf := reflect.TypeOf(instance)
|
||||
pkgPath := typeOf.PkgPath()
|
||||
name := Lower(parts[len(parts)-1])
|
||||
```
|
||||
|
||||
`WithService` uses `reflect` to auto-discover the service name from the factory's return type's package path. This is magic (P5-4) and fragile — renaming a package changes the service name.
|
||||
|
||||
**Resolution:** `WithName` already exists for explicit naming. Consider making `WithService` require a name, or document that the auto-discovered name IS the package name and changing it is a breaking change.
|
||||
|
||||
### P9-3. No JSON Primitive — Every Consumer Imports encoding/json
|
||||
|
||||
core/agent's agentic package has 15+ files importing `encoding/json`. Core provides string helpers (`core.Contains`, `core.Split`) but no JSON helpers. Every consumer writes:
|
||||
|
||||
```go
|
||||
data, err := json.Marshal(thing) // no core wrapper
|
||||
json.NewDecoder(resp.Body).Decode(&v) // no core wrapper
|
||||
```
|
||||
|
||||
**Resolution:** Consider `core.JSON.Marshal()` / `core.JSON.Unmarshal()` or `core.ToJSON()` / `core.FromJSON()`. Same guardrail pattern as strings — one import, one pattern, model-proof. Or accept that `encoding/json` is stdlib and doesn't need wrapping.
|
||||
|
||||
**The test:** Does wrapping JSON add value beyond import consistency? Strings helpers add value (they're on `core.` so agents find them). JSON helpers might just be noise. This one is a judgment call.
|
||||
|
||||
### P9-4. No Time Primitive — Timestamps Are Ad-Hoc
|
||||
|
||||
Every service formats time differently:
|
||||
|
||||
```go
|
||||
time.Now().Format("2006-01-02T15-04-05") // agentic events
|
||||
time.Now().Format(time.RFC3339) // review queue
|
||||
time.Now() // status.UpdatedAt
|
||||
```
|
||||
|
||||
No standard timestamp format across the ecosystem.
|
||||
|
||||
**Resolution:** `core.Now()` returning a Core-standard timestamp, or `core.FormatTime(t)` with one format. Or document the convention: "all timestamps use RFC3339" and let consumers use `time` directly.
|
||||
|
||||
### P9-5. No ID Generation Primitive — Multiple Patterns Exist
|
||||
|
||||
```go
|
||||
// plan.go — crypto/rand hex
|
||||
b := make([]byte, 3)
|
||||
rand.Read(b)
|
||||
return slug + "-" + hex.EncodeToString(b)
|
||||
|
||||
// task.go — atomic counter
|
||||
taskID := "task-" + strconv.FormatUint(c.taskIDCounter.Add(1), 10)
|
||||
|
||||
// go-process — atomic counter
|
||||
id := fmt.Sprintf("proc-%d", s.idCounter.Add(1))
|
||||
```
|
||||
|
||||
Three different ID generation patterns. No standard `core.NewID()`.
|
||||
|
||||
**Resolution:** `core.ID()` that returns a unique string. Implementation can be atomic counter, UUID, or whatever — but one function, one pattern. Same guardrail logic as everything else.
|
||||
|
||||
### P9-6. No Validation Primitive — Input Validation Is Scattered
|
||||
|
||||
```go
|
||||
// Repo name validation — agentic/prep.go
|
||||
repoName := core.PathBase(input.Repo)
|
||||
if repoName == "." || repoName == ".." || repoName == "" {
|
||||
return core.E("prep", "invalid repo name", nil)
|
||||
}
|
||||
|
||||
// Command path validation — command.go
|
||||
if path == "" || HasPrefix(path, "/") || HasSuffix(path, "/") || Contains(path, "//") {
|
||||
return Result{E("core.Command", "invalid command path", nil), false}
|
||||
}
|
||||
|
||||
// Plan ID sanitisation — agentic/plan.go
|
||||
safe := core.PathBase(id)
|
||||
if safe == "." || safe == ".." || safe == "" {
|
||||
safe = "invalid"
|
||||
}
|
||||
```
|
||||
|
||||
Every validation is hand-rolled. The same path traversal check appears in three places with slight variations.
|
||||
|
||||
**Resolution:** `core.ValidateName(s)` / `core.ValidatePath(s)` / `core.SanitisePath(s)` — reusable validation primitives. One check, used everywhere. A model can't forget the `..` check if it's in the primitive.
|
||||
|
||||
### P9-7. Error Codes Are Optional and Unused
|
||||
|
||||
```go
|
||||
func WrapCode(err error, code, op, msg string) error { ... }
|
||||
func NewCode(code, msg string) error { ... }
|
||||
func ErrorCode(err error) string { ... }
|
||||
```
|
||||
|
||||
Error codes exist in the type system but nobody uses them. No consumer calls `WrapCode` or checks `ErrorCode`. The infrastructure is there but the convention isn't established.
|
||||
|
||||
**Resolution:** Either remove error codes (dead feature) or define a code taxonomy and use it. Error codes are useful for API responses (HTTP status codes map to error codes) and for monitoring (count errors by code). But they need a convention to be useful.
|
||||
|
||||
### P9-8. Core Has No Observability Primitive
|
||||
|
||||
Core has:
|
||||
- `c.Log()` — structured logging
|
||||
- `c.Error()` — panic recovery + crash reports
|
||||
- `ActionTaskProgress` — progress tracking
|
||||
|
||||
Core does NOT have:
|
||||
- Metrics (counters, gauges, histograms)
|
||||
- Tracing (span IDs, parent/child relationships)
|
||||
- Health checks (readiness, liveness)
|
||||
|
||||
go-process has health checks. But they're per-daemon, not per-service. There's no `c.Health()` primitive that aggregates all services' health status.
|
||||
|
||||
**Resolution:** Consider `c.Health()` as a primitive:
|
||||
|
||||
```go
|
||||
c.Health().Ready() // bool — all services started
|
||||
c.Health().Live() // bool — not in shutdown
|
||||
c.Health().Check("brain") // per-service health
|
||||
```
|
||||
|
||||
Metrics and tracing are extension concerns — they belong in separate packages like go-process. But health is fundamental enough to be a Core primitive. Every service could implement `Healthy() bool` alongside `Startable`/`Stoppable`.
|
||||
|
||||
---
|
||||
|
||||
## Versioning
|
||||
|
||||
### Release Model
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue