Conversation
Reverses three documented non-goals - no daemon, no signal bus or inbox, no
raw transcript upload - and records what each one cost. Covers component
boundaries, wire protocol, identity, storage, the inbox, the web UI, failure
behavior, a phased plan, and the threat model behind all of it.
Four decisions are recorded with the losing argument kept rather than dropped:
D-1 the push is the sharing decision. Sync is opt-in per repo, pushed run
events are team-readable, briefs stay private to the sender until shared.
D-1b the acceptor names the graph. An inbox message reaches an agent only as
pre-fenced data, never as the prompt that chose the task.
D-3 SQLite is the hub's truth; JSONL is a derived export. Revision 1 had it
backwards and claimed append-only was a physical property of a file,
which is false - sed -i disproves it. The laptop's event log is unchanged.
D-4 no central conversation store, on measured evidence: 61% of 80 sampled
transcripts on the author's machine carry a credential shape scan.ts
already recognises, and that is a floor, not an estimate.
Nothing under src/ is built yet. Three questions in section 14 need answering
before phase 1, one of which - where the brief encryption key is wrapped - has
no option that is both unattended-restartable and safe from a root operator.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fifteen commits for phase 1, then phases 2-5 at commit granularity, with a delegation verdict per commit. An adversarial review of the first draft found four defects that produce a wrong result rather than a failing build, all verified against the tree before being fixed here: - events was WITHOUT ROWID while cursors keyed on (received_at, rowid). Verified impossible: SELECT rowid fails with "no such column: rowid". - RunState carries content, not just status. vars values and nodes[*].output are raw agent stdout, so pushing a checkpoint verbatim would publish secrets team-readable in phase 1, a phase before any masking exists. Commit 1.10 now projects both out on the member's machine before anything leaves it. - INSERT OR REPLACE on events fires the append-only delete trigger, and the shortest path to a green test is deleting the trigger. Spelled out instead. - scan.test.ts asserts the complete ordered rule list, so adding a rule without updating it is red. Also settled what the review found under-specified: the wire types and that event lines are stored verbatim rather than re-serialized, the hash chain order, error body shapes, the body cap, hashToken's input, timingSafeEqual over equal-length digests, the data dir, the shebang, node >= 22.13 for node:sqlite, and that the batcher must gate on the repo opt-in, unref its timer, catch every batch promise, and compose with the existing onEvent rather than replace it. Design doc amended to match: no WITHOUT ROWID, a chain_head table, members in a table rather than members.jsonl, no admin http route, user_version, and a new section 4.1 on what a pushed state omits. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Commit 1.4 typed EventBatch.state as ProjectedState while commit 1.10 declared that interface, so 1.4 could not compile on its own. The type is wire vocabulary and belongs in src/hub/wire.ts; 1.10 now supplies only the function that produces it, and imports the type. Two consequences worth naming. The type omits streamId, because EventBatch already carries it at top level - which is also what makes it buildable before commit 1.9 adds the field to RunState. And vars becomes varKeys: string[] rather than a map with nulled values, because a nulled map still has a slot a later change can refill with nothing failing, whereas a list of key names has nowhere to put a value at all.
…wire Two defects from review of 8402e49. The plan had moved ahead of the design doc it defers to. Its own header says the design wins on disagreement, so introducing varKeys in the plan while design 4.1 still described a map with nulled values left the plan in violation of its own governance rule - with the better content. Design 4.1 now leads with the varKeys shape and the reason for it, and the plan follows. EventBatch names its run twice, once at the top level and once inside the projected state, and nothing said what happens when the two disagree. A client could push runId A carrying a state describing run B and the hub would store a row whose status, cost and node table belong to a different run: a wrong answer that never errors, which is the failure class the plan's preamble exists to catch. eventBatchSchema now refuses any batch where state.runId or state.graphName disagrees with the top-level value, 1.4 gains the two assertions, and 1.7 maps the refusal to 400 run identity mismatch with nothing stored. Refusal rather than precedence, and refusal rather than dropping the two fields from ProjectedState - dropping them would leave HubStore.runState() returning an object that cannot say which run it describes, and would make the jsonl export reconstruct identity by joining runs.
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
datj9-reader
left a comment
datj9-reader
left a comment
There was a problem hiding this comment.
Verdict: approve the code, but CI is red — GitGuardian must be dismissed before merge
Reviewed by probing the auth and isolation boundaries against a live in-memory hub, not by reading alone.
Verified by execution
npx vitest run-> 577/577 pass, 35 filesnpx tsc --noEmit-> 0 errorscheck (22)andcheck (24)both green
The CI failure is a false positive, and I verified it
GitGuardian Security Checks reports "5 secrets uncovered!" — all 5 in src/hub/auth.test.ts, flagged in commits 3852eee2 ("feat: add hub auth") and 8a2cfcc4.
The flagged literal decodes to:
MjM0NTY3ODkw -> 234567890
YWJjZGVmZ2hpamtsbW5v -> abcdefghijklmno
cHFyc3R1dnd4eQ -> pqrstuvwxy
It is the alphabet. Not a credential.
The current tree already fixes this — HEAD assembles the fixture from split parts at runtime (auth.test.ts:17) with a comment explaining exactly why. The old commit 3852eee2 had the bare literal on one line; 9ce1769 does not. Both flagged commits are ancestors of HEAD, so GitGuardian is scanning branch history, not the merged result.
Two consequences worth naming:
- This does not self-resolve on a normal merge — the commits enter
main's history. Squash-merge collapses them so only the mitigated tree lands. The repo allows squash. - Either way the incident needs dismissing in the GitGuardian dashboard as a test fixture, or it stays open against the repo.
Security probes — all clean
| Probe | Result |
|---|---|
Bob writes into Alice's namespace via member in body |
400 — member comes from the token, never the body |
Bob POSTs to Alice's streamId |
200 but filed under bob — writes are member-scoped |
| Read-only scope attempts ingest | 403 |
| No auth / malformed token | 401 / 401 |
| Path traversal in member segment | 404 |
| Exact replay | acked, duplicates: 1, nothing rewritten |
| Same seq, different content | 409 seq conflict — tamper-evident |
Event runId disagreeing with envelope |
400 run identity mismatch |
resolveMember uses timingSafeEqual over equal-length hex digests and guards the length mismatch so a malformed stored hash yields 401 rather than a 500. Token hashes are never stored in plaintext. Revocation is checked on lookup.
Findings
1. Non-contiguous seq is accepted silently (low). After seq 1, a batch at seq 5 is accepted and the high-water jumps to 5:
4. gap (seq 5 after 1) -> 200 {"highWaterSeq":5,"accepted":1}
Seqs 2-4 can then never be ingested — they are at or below the high-water mark and get dropped as duplicates. Since a hub outage is explicitly allowed to drop batches, a gap is a real scenario, and the missing events become permanently unacceptable with no signal. The 409-on-divergence policy is loud; the gap is silent. Worth either a gaps counter in IngestResult or a note in the design doc that gaps are accepted by design.
2. Cross-member reads are open to any read-scope token (by design — confirm). GET /v1/runs/alice/<id> returns 200 for Bob's token. That matches a shared-team model and the design doc never claims per-member read isolation, so I read it as intended. Flagging only so it is an explicit decision: any member token reads every member's run state and events.
Notes
feed()clamps withMath.max(1, Math.floor(limit)), solimit=-1,0,abcand999999all return sane pages.- Ingest is one transaction; a divergent seq aborts the whole batch with nothing written.
- Commit authorship on all commits resolves to
datj9.
# Conflicts: # src/commands/resume.ts # src/handoff/scan.test.ts # src/handoff/scan.ts
2 participants
Adds
lg-hub: a third binary that mirrors a member's runs to a shared SQLite-backed HTTP service, so a team can see each other's agent runs without anyone's transcript or variables leaving their machine.Implements phase 1 of
docs/hub-design.mdanddocs/hub-implementation-plan.md, both of which land in this branch.What ships
Runs then mirror automatically as they execute, and
curl /v1/runs/alice/<runId>returns them.The security boundary
src/team/project.tsis the file worth reading first. It decides what leaves a machine, and it is built field by field rather than as a filtered copy, so there is no field a secret could ride in on:varsreach the hub as key names only — values are never copied anywhere.outputnever reaches the hub.erroris path-rewritten, secret-masked viaSCAN_RULES, and capped at 200 characters.cwdis rewritten through the existingrewritePaths.ProjectedStateis a distinct hand-written type, neverOmit<RunState, …>— so a future content-carrying field added toRunStatecannot silently start publishing itself.Verified against the built binary, not just the suite
A 12-step acceptance run against a live hub, 25 assertions:
--var apiKey=sk-…syncs, and the secret value is absent from what the hub returns, whilevarKeys: ["apiKey"]is present and nooutputfield exists.lg-hub export --jsonlreproduces all 14 local event lines byte-for-byte.That last one is phase 1's central promise: a hub outage cannot affect a run. No hub code path can throw into the engine, change a checkpoint, or change an exit code.
Design notes worth knowing
eventsis append-only in the database too, enforced by trigger rather than convention, with a globalsha256(prev || json)hash chain. A divergent seq for an existing key aborts the whole transaction and acks nothing.membercomes from the token'skey_id; the wire schema has nomemberfield for a client value to occupy.lgthan the hub must still be able to sync.lg-hub serverefuses a non-loopback bind without--behind-tls-proxy. A bearer token over plaintext HTTP is a credential shape this project's own scanner has a rule for.src/core/is touched once, in1638030, to addstreamId. Legacy checkpoints derive one deterministically in memory and write nothing — re-saving insideloadwould recurse intosave, advance the checkpoint generation on a read, and mark untouched runs as modified.Two caveats, stated in the README rather than buried
SCAN_RULESknows and nothing else.Reviewing this
30 commits, each green under
npm run typecheck && npm test(577 tests, 35 files).Nine are
fix:commits repairing work landed earlier in this same branch, each naming its defect in the message — they are the record of what review caught, and every one was a wrong-result-with-a-green-build rather than a failing test. Suggested reading order:docs/hub-design.md§4.1 andsrc/team/project.ts— the security boundarysrc/hub/wire.ts— the frozen vocabulary and its deliberate strictness asymmetrysrc/core/store.ts:48-72— the one engine touch, and why the obvious implementation is wrongsrc/team/batch.ts— two timers with oppositeunrefrules, both load-bearingNot in this branch
No inbox (phase 3), no web UI (phase 4), no briefs, no encryption at rest, no redaction on read (all phase 2). Nothing on the hub is encrypted yet. No full transcripts, ever.
One known limitation recorded for phase 2: a sync cursor that ends up ahead of the hub is unrecoverable, because the client cannot learn which seqs the hub is missing. The hub can — returning its own high-water on a rejected push would fix it, but that is a protocol change.