refactor(app): unify Open/OpenForRead into one mode-parameterized open path (links-va-001-app-factory-g4e.1)#211
Conversation
There was a problem hiding this comment.
Z.ai Coding Agent Review
Clean refactoring that consolidates Open, OpenForRead, and OpenFromWD into a single Open(ctx, cwd, mode) path. The AccessMode type moves from CLI-internal to the app package, establishing it as the one source of truth ([LAW:one-source-of-truth]). All call sites are mechanically updated. Tests cover the read/write behavioral contract, the read-after-write acceptance path, and rejection of unknown modes including the zero value.
No must-change issues found. Two notes for awareness:
-
openStoreswitch (app.go:46). Theswitch mdispatches to different underlying store functions. This is control flow selecting which code runs, which is the[LAW:dataflow-not-control-flow]pattern. However, the two store functions have genuinely different contracts (bootstrap vs. require-existing), so the switch is the minimum viable boundary mapping — and it replaces the prior scattered caller-side branching, which was worse. The ideal fix would be makingstore.Openandstore.OpenForReada single function parameterized by intent, but that's a store-layer change outside this PR's scope. -
AccessModeisstring(app.go:20). The zero value""is representable but semantically invalid, compensated by runtime validation inopenStore. In Go there's no sum-type mechanism to prevent this at compile time. The test atapp_test.go:81explicitly covers the zero-value rejection, so the gap is well-guarded.
✅ Approved
…n path (links-va-001-app-factory-g4e.1) app.AccessMode is now the single access-mode representation [LAW:one-source-of-truth]; the cli's private appAccessMode string type is deleted and every registration table consumes app.AccessMode directly. app.Open(ctx, cwd, mode) is the single construction path [LAW:single-enforcer]; the duplicate OpenForRead factory and the dead OpenFromWD are deleted. The old runWithApp switch granted write access to any unrecognized mode via its default arm; the absorbed switch fails closed with an explicit invalid-mode error [LAW:no-silent-failure], pinned by test.
9a875d8 to
085cc33
Compare
There was a problem hiding this comment.
Z.ai Coding Agent Review
This PR cleanly unifies Open/OpenForRead/OpenFromWD into a single Open(ctx, cwd, mode) entry point and moves AccessMode from the cli package to the app package where it belongs. The CLI layer no longer branches on access mode — it passes the value through to app.Open, which delegates to AccessMode.openStore for the single boundary dispatch. Tests cover the write-bootstrap, read-refuse, read-after-write, and unknown-mode-rejection contracts.
No must-change issues found. Two observations worth the author's attention:
-
Pre-existing:
resolveDoctorAccessMode(doctor.go:41–42) fails open to write access on flag-parse errors. Oncmd.ParseFlagserror, it returnsAccessWriterather thanAccessReador a hard error. This is pre-existing behavior unchanged by this diff, but it's worth auditing — a malformed flag string silently granting write access is the kind of silent-failure[LAW:no-silent-failure]guards against. -
The
openStoreswitch (app.go:46–51) is a single-point boundary mapping, not scattered control flow. The PR correctly eliminated the scatteredswitch accessModethat was inrunWithApp(old cli.go) and consolidated it into one place. A future step could push the read/write distinction intostore.Openitself (e.g.,store.Open(ctx, dir, wsID, store.ReadOnly)), eliminating the two separate store constructors and making the dataflow even cleaner — but that's a store-package refactor, not a blocker for this PR.
✅ Approved
Implements links-va-001-app-factory-g4e.1 (epic F-15: app.Factory mode unification).
What
app.AccessMode(string-sum, house idiom from the sealedRelationTypein Seal RelationType as a sum-type with one parse boundary (links-va-001-relation-type-raq.1) #206). The cli's privateappAccessModeis deleted; the registration tables,commandFamily, andresolveDoctorAccessModeconsumeapp.AccessModedirectly.[LAW:one-source-of-truth]— the type lives inappbecause cli depends on app, never the reverse.app.Open(ctx, cwd, mode). The duplicateOpenForReadfactory and the deadOpenFromWD(zero callers) are deleted.[LAW:single-enforcer]runWithAppswitch routed any unrecognized mode to itsdefault:arm — write access, the most-privileged interpretation of an illegal state. The absorbed switch returns an explicit invalid-mode error for unknown modes including the zero value.[LAW:no-silent-failure]Deviation from the epic's sketch
The epic proposed an
AppFactory struct { mode AccessMode }. A struct holding only a mode whose sole method isOpenis machinery callers must assemble before acting ([LAW:composability]); a mode parameter is the same single enforcer with less carrying cost. Likewise string-sum overint+iota: iota's zero value would silently mean ReadWrite.Tests
internal/app/app_test.go: mode contract parameterized by mode (write bootstraps an uninitialized workspace, read refuses it), read-after-write interop, unknown/zero mode fails closed.go vet ./...andgo test ./...clean.