feat(shares): move share keyring from ~/.arc/shares.json to data.db - #41
Merged
Conversation
Add Share/ShareKind types with Validate/IsValid methods to internal/types, four new methods on the storage.Storage interface (UpsertShare/GetShare/ ListShares/DeleteShare) with ErrShareNotFound sentinel, migration 017_shares.sql with goose pragmas, schema.sql update, /shares OpenAPI endpoints with regenerated Go and TypeScript bindings, and authorized stub Store methods.
Replace T0 stubs in shares.go with real sqlc-backed implementations. Add shares.sql queries, regenerate shares.sql.go, and write unit tests covering upsert (insert + replace), get (found + ErrShareNotFound), list (ordered newest first + empty), and delete (remove + idempotent).
Implements listShares, getShare, upsertShare, and deleteShare handlers in internal/api/shares.go, registers routes in server.go, and covers all 10 spec-required scenarios in shares_test.go (200/400/404/204 status codes, upsert replace semantics, idempotent delete, newest-first ordering).
Implement four methods on *Client to wrap the shares API endpoints: - ListShares: returns all keyring entries - GetShare: retrieves a share by ID, returns ErrShareNotFound on 404 - UpsertShare: inserts/replaces a share and returns server-stamped record - DeleteShare: removes a share, idempotent on 404 Includes comprehensive test coverage using httptest.NewServer pattern matching labels.go style. All tests pass; whole codebase builds.
Add assertion that verifies the request body sent by UpsertShare client method matches the input share fields exactly. The test now creates a custom test server that captures the decoded request body and asserts each field (ID, Kind, URL, KeyB64Url, EditToken, PlanFile) matches what was passed to UpsertShare.
Replace file-based share registry with a thin HTTP wrapper over the new /api/v1/shares endpoints. Preserves the full public API (Add, Find, Load, Remove, Share, File, ErrShareNotFound) so cmd/arc/share.go callers compile unchanged. Adds SetClientFactory indirection for test injection, and LegacyPath() for T6 startup wiring. Tests use injected fakeClient; no temp dirs.
… endpoints (T6) Wires together the work from T0-T5 so the share keyring runs end-to-end through arc-server. - internal/server: invoke ImportLegacySharesJSON after the api.Server is constructed and before the listener starts so a one-shot migration of ~/.arc/shares.json into the shares table runs at startup. Idempotent (skips when the table is non-empty) and renames the JSON to .bak on success so subsequent boots are no-ops. - cmd/arc/main.go: install sharesconfig.SetClientFactory in init() so every `arc share *` invocation has a live HTTP client. The factory is lazy — getClient() is called at command time, not process start. - internal/api/server: expose a small RegisterShareRoutes(g) helper so test fixtures outside the api package can mount the keyring routes without recreating the full registerRoutes() surface. - cmd/arc/share_test.go: extend startTestPasteServer to also stand up an in-process /api/v1/shares against a real sqlite store backed by the full migration set (017_shares.sql included). Each test gets a fresh client factory pointed at the test server, restored on cleanup.
Schema cleanup (QUAL-1, QUAL-2, MISS-3):
- Convert created_at TEXT/STRICT to TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP
matching every other table in the codebase. sqlc now generates time.Time
natively, so the silent t, _ := time.Parse(...) error swallow in rowToShare
goes away entirely.
- Add CHECK (kind IN ('local', 'shared')) constraint at the schema level since
Validate() already enforces it at the type level.
- Move the IsZero -> time.Now() default into Store.UpsertShare so HTTP handlers
and the legacy import path share one default-stamping site.
Sentinel handling (QUAL-3, AUTH-2):
- Test fakeClient now returns client.ErrShareNotFound directly instead of
errors.New("share not found"), so the err.Error() string-equality fallback
in sharesconfig.Find can be deleted in favor of plain errors.Is.
- Replace U+2014 em-dash with ASCII colon in the factory-not-initialized error
message.
Route dedup (QUAL-4):
- registerRoutes calls s.RegisterShareRoutes(v1) instead of repeating the four
route registrations inline.
Handler cleanup (AUTH-1):
- Drop the read-after-write GetShare in upsertShare. The store stamps
CreatedAt on the passed-in *types.Share, so the same struct already has the
correct value when we return it.
User-facing strings (QUAL-8):
- Replace "~/.arc/shares.json" wording in cobra Short descriptions, Println
output, and CLI error messages with "the local arc keyring", reflecting
the post-migration storage. Code comments referencing the legacy file are
also updated.
Test cleanup (AUTH-3, IDIOM-2):
- Remove the boilerplate "// TestX verifies ..." doc comments from
shares_test.go that mechanically restate each test's name.
- Replace map[string]interface{} with map[string]any and 0644/0600/0700
literals with 0o644/0o600/0o700 in shares_import_test.go to match the
rest of the codebase (Go 1.18+ idioms).
Skipped findings (defensible or out of scope):
- QUAL-5 (use c.get/c.delete helpers in shares client): the helpers run
checkError before returning, which closes the body and string-formats the
error, leaving no clean way to express 404-aware semantics without string
matching. The status-aware bypass in GetShare/DeleteShare is the right
shape given the helper API.
- QUAL-6: 3-package ErrShareNotFound — already downgraded by the reviewer.
- QUAL-7 (transaction in import loop): defensible. Import is one-shot and
recoverable; changing the abort-on-validation-failure semantic would
require updating the existing test contract for marginal benefit.
- MISS-1 (wasteful api.New in test fixture): would require a new public
constructor for marginal allocation savings.
- MISS-2 (context.Background in startup): borderline; startup is short-lived.
- IDIOM-1 (testify in client tests vs stdlib in api tests): pre-existing
pattern conflict not introduced by this branch.
The original importer had two problems that combined into an unrecoverable
upgrade path:
1. The loop ran outside any transaction, so a per-entry validation or
constraint failure left entries 0..N-1 durably committed while the
.bak rename never happened.
2. The "skip if shares table has rows" guard then prevented the next
startup from retrying — the partial state silently shadowed the
remaining legacy entries forever.
Replace both with a single transactional batch:
- Add Storage.UpsertShares([]*types.Share) — wraps the per-row UpsertShare
calls in BeginTx + WithTx + Commit, so any validation failure rolls the
whole batch back. The old single-entry UpsertShare delegates to the same
shared body so behavior outside the import path is unchanged.
- Drop the row-count guard. The natural idempotency signal is the file
itself: presence means "import me," absence (or a successful import that
renamed it to .bak) means "nothing to do." This makes user-restored
shares.json files re-importable cleanly via upsert semantics.
Test contract changes:
- TestImportLegacySharesJSONValidationFailureMidImport now asserts atomic
rollback (0 rows, file un-renamed, error returned) instead of the broken
partial-write behavior.
- TestImportLegacySharesJSONNonEmptyTable replaced by
TestImportLegacySharesJSONIdempotentUpsert, which exercises the new
contract: pre-existing rows that aren't in the file are preserved, rows
that are in both get overwritten, and the .bak rename still happens.
Found by code review on b3ceed2.
- errcheck: handle json.Encoder.Encode error in client/shares_test.go - gosec G306: tighten shares_import_test temp-file perms 0o644 -> 0o600 - revive confusing-naming: rename helper upsertShare -> upsertShareWith so it no longer differs from UpsertShare only by capitalization - testifylint error-is-as: assert.True(t, errors.Is(...)) -> assert.ErrorIs(t, err, ...) (drops the unused errors import too) - testpackage: switch internal/types/shares_test.go and internal/sharesconfig/sharesconfig_test.go to *_test packages, since every symbol they touch is already exported
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Migrate the author-side share keyring (currently
~/.arc/shares.json) into the arc-server's SQLite database (~/.arc/data.db), exposed via/api/v1/shares. Both the CLI and a future webui share-management UI talk to the same endpoints. The CLI's user-visible behavior is unchanged.The original branch also includes the earlier
arc share delete --forcework (5410482,7a4c873) that hadn't been PR'd yet — it stays bundled here since the keyring rewrite supersedes the file-handling code those commits touched.Why
shares.jsonis a client-side keyring of edit tokens. Today only the CLI reads it; the local arc webui can't (browsers don't reach the filesystem). The planned QoL feature of webui-driven share management requires the keyring to live where the daemon owns it —data.db— so multiple clients can read and mutate it through a single API surface.Storage format alone wouldn't have justified the change. The multi-client requirement is what tips it.
What changed
sharestable (migration017_shares.sql) withCHECK(kind IN ('local','shared'))andcreated_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMPmatching every other table in the codebase.storage.Storageinterface gainsUpsertShare/UpsertShares/GetShare/ListShares/DeleteShareplus anErrShareNotFoundsentinel.UpsertSharesis transactional so legacy imports are atomic./api/v1/sharesendpoints (internal/api/shares.go) mirroring the labels handler pattern. OpenAPI spec updated; Go bindings + TypeScript types regenerated.internal/client/shares.goCLI HTTP client.internal/sharesconfigrewritten as a thin HTTP shim that preserves the public API (Add/Find/Load/Remove/Share/File/ErrShareNotFound) socmd/arc/share.gocallers compile unchanged. Client factory injected fromcmd/arc/main.go init().~/.arc/shares.json, atomically upserts viaUpsertShares, renames toshares.json.bakon success. File-presence is the idempotency signal — partial commits are no longer possible because the batch is transactional, and the next startup retries cleanly if the operator fixes a bad entry.~/.arc/shares.jsonreferences to "the local arc keyring".arc share delete --forceflag (from earlier work on this branch) cleans up orphaned local entries when the server has already deleted the share.Architecture decisions
/api/v1/shares(vs/api/client-sharesor/api/planner/shares) — matches the existing flat resource pattern under/api/v1(issues, labels, plans). The/api/paste/blob host stays intentionally unversioned.edit_tokenstorage — these are bearer tokens we hand back to clients; hashing makes the keyring useless. Same security posture as the JSON file (file mode 0600 → API on 127.0.0.1 with same trust boundary).Review-driven cleanups in this branch
Three rounds of review iteration are in the history:
b3ceed2(schema TEXT/STRICT → TIMESTAMP, em-dash → ASCII, route dedup, read-after-write removal, staleshares.jsonstrings, etc.). The skipped findings are documented in that commit's message.0378177addsStorage.UpsertShares(transactional batch) and replaces the row-count guard with file-presence semantics.8f045efresolves allmake lintfindings the changes introduced (errcheck, gosec G306, revive, testifylint, testpackage).Test plan
go test ./...— full suite green (14 packages)make lint— 0 issuesmake build— cleanmake gen— regenerated bindings committedapi/openapi.yaml→internal/api/openapi.gen.go+web/src/lib/api/types.tssharesCRUD + upsert-replace + idempotent-delete + transactional batcherrors.Is(err, ErrShareNotFound)round-triparc share *round-trip tests pass against the new endpoints