Feature/improvesprint1 - #2
Merged
Merged
Conversation
Three places where something quietly succeeded when it should have refused. A PUT over the cap was stored truncated. handlePutFile read through a 32 MiB io.LimitReader, which stops at the cap and reports success, so io.ReadAll cannot tell a body that ended from one that had more to give. Upload 40 MiB and the server answered 200 with a document two thirds of the way through a sentence. Silent truncation on a write path is the worst kind of bug, because the client is told its file is safe. readAtMost asks for one byte past the limit, which is what tells the two apart, and the limit now comes from the space's own max_file_size rather than a constant that had nothing to do with it — a 413 with the actual number, not a shorter file. config.yaml and the server's config.yaml were written 0644 while holding git_token, s3_secret_key and sql_dsn with the password inline. The bearer token next to them was already 0600. On a shared host, or in a container with a second user, that is a credential given away for nothing. They are written owner-only now and repaired on read, because a config written by an earlier version is already on disk and nobody is going to go looking. A file left stricter than we ask for is left alone. The MCP reader compared path strings, which a symlink walks straight past: a link at team/notes.md pointing to ~/.ssh/id_rsa passed every check and was then read and handed to the model. That is not hypothetical for a client space — its contents arrive from a server and `contextd pull` writes the paths the server names, so a hostile or compromised server plants the link and the contents leave the machine inside a prompt. storage.ResolveUnder existed for exactly this and is used everywhere else; it resolves the deepest existing ancestor before comparing, so the link is followed first and judged afterwards. The listing skips them too, so the model is never offered a file the reader will refuse, and a single file is capped at 1 MiB because the whole body goes into a tool result. This package had no tests at all, which is unfortunate for the one surface that hands files to a language model. It has them now, and the escape was verified by restoring the old check and watching the test return the private key. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Nothing told anyone a newer contextd had shipped. The hard part is not the check, it is not becoming the thing people turn off — and a version notice has three ways to go wrong, in order of damage. It corrupts something. `contextd mcp serve` speaks JSON-RPC over stdio, so a stray line there breaks the AI client outright; --json and --yaml exist to be piped into a parser; anything on stdout can end up inside a pipe. So the notice goes to stderr and never stdout, and the commands whose output belongs to a machine — mcp serve, daemon run, completion, server start — are excluded by name rather than trusted to be careful. It costs time. So the check never blocks: a command prints whatever is already cached and refreshes in the background for next time. The first run after an install is therefore silent, which is right — somebody who just downloaded this does not need to hear about a release. It repeats. Being told the same thing daily is what makes people reach for the off switch, and then they never hear about the release that matters. One notice per new version, at most one check a day, and a failed check still counts as having asked so an offline machine does not retry on every command. What is left is a single line the first time a release appears, to a person at a terminal, with a permanent opt-out in config (no_update_check) and an environment one (CONTEXTD_NO_UPDATE_CHECK). Scripts, CI and AI integrations see nothing, ever. Two things the tests found rather than confirmed. A development build was going to be nagged: the comment said 0.0.0-dev had no place in the sequence, but the parser stripped the suffix and compared the numbers, so every contributor would have been told they were nine releases behind. parseRelease now answers the "should we speak" question separately from the "which is newer" one, and a dev build does not even reach for the network, because no answer would change what it does. And the first version of the concurrency test waited on the wrong condition, so it read the previous answer and passed for the wrong reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Local takes a flock around every operation. This backend, implementing the same interface — one that promises compare-and-swap — took nothing. Two failures follow. The compare and the swap were separate steps: read the file, hash it, compare, write. Two writers both passed the comparison and the second silently discarded the first. And go-git's worktree is not safe for concurrent use, because Add and Commit share one index file; overlapping commits corrupt it outright. One mutex over the whole backend rather than one per path, because the index and HEAD belong to the repository and not to any single file. A commit per write is already the slow part, so contending on a lock costs nothing that was not already being paid. Push is split into a locked entry point and an unlocked pushLocked for the callers already holding it — separated rather than made reentrant, because a mutex that is sometimes held twice is a deadlock waiting for the day somebody adds a call. The tests assert outcomes rather than timing, since a race reproduces unreliably: exactly one winner per contended create, per contended overwrite and per contended head update, and a repository still readable afterwards. Removing the locks fails all four, and the fourth fails with "unknown extension" and "EOF" from go-git's index parser — which is what a corrupted index reads like, and is worse than any lost write. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two defects with one cause: the key said nothing about the path. It was SHA-256 truncated to eight bytes. Sixty-four bits deciding which file you are reading, where Local used the full digest for the same job and nothing recorded why S3 was cut short. A collision means one file silently overwriting another with no error anywhere, and sixty-four bits is inside reach — about 2^32 candidates to hit by chance, far fewer to construct deliberately. And because the key was opaque, listing a space meant downloading every object in the bucket to read the path back out of its body. One GET with the whole file on the wire, per file, on every tree, every changes and every quota check. That is a bill and a wait for an answer S3 already had. Keys are now the path, escaped only where S3 or a person would trip over it — slashes kept, so a bucket listing looks like the space it holds. Collisions become impossible rather than unlikely, and a listing recovers every path from the keys alone. The version still has to be right. It is the CAS token callers compare against, so inventing one from the ETag — which is a hash of the stored record, not of the file — would have made S3 disagree with every other backend about what version a file is. It is stamped into the object's user metadata instead and read back with a HeadObject: headers, no body. N round trips still, but nothing is downloaded, which was the actual cost. A single-request listing needs an index object, and that is a design change with its own concurrency problems, not something to smuggle in here. Existing buckets keep working. A read falls back to the old key, a write to a path still living there moves it and removes the old copy, and a delete removes whichever key actually held the object. A bucket converges as it is used, and a listing pays the old price only for the objects nobody has touched yet. Legacy keys are recognised by shape so the listing cannot mistake one for a file named after a hash. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The local record keeps a file's bytes in the same JSON document as its path and version, base64-encoded. List decoded into that record, so answering "what is in here" read and base64-decoded every byte of every file — on every tree, every changes and every quota check, while holding the store's single exclusive lock, which meant nothing else could touch the space until it finished. Decoding into a struct without the Data field is enough: encoding/json skips what it has nowhere to put. A hundred-megabyte space now costs a hundred megabytes of reads to list instead of a hundred megabytes of reads and decodes, and the lock is held for a fraction of the time. The tests check the part that would make this a bug rather than a speedup: the version a listing reports has to be the one Get reports, or the two disagree about what a caller is holding. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two small things on the request path, both of the same kind: a value nobody checked, kept somewhere it did not belong. The rate-limit key was "bearer:" plus the token. The limiter holds its buckets in a map that lives as long as the process, so every token presented to the server stayed in memory as a working credential long after the request that carried it — reachable by a core dump, a crash reporter, a debugger. It is hashed now. The limiter only ever needed to tell callers apart, which a hash does exactly as well. The existing test asserted the literal "bearer:cv-kim-secret", which pinned the defect as the intended behaviour; it now asserts the property instead. The request id came from a caller-controlled header and went verbatim into structured logs and the error envelope. A newline forges a log line, control bytes confuse whatever reads them, and an unbounded string is an unbounded log record. A caller may still bring its own id so a trace can be followed across services, but it has to be short and plainly printable; anything else is replaced rather than escaped, because a correlation id has no business carrying punctuation somebody has to reason about. The generated id is random rather than UnixNano. The clock is guessable, which let a caller predict and then claim somebody else's id, and on a platform with a coarse clock two requests in the same tick shared one — precisely when telling them apart is the point. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three things in the sync path that a person would notice.
A push walked the tree and put every file, every time, base64-encoded into one
JSON body. Publishing an edit to one document re-uploaded the whole space.
LocalState now records the content this machine last sent, so a push carries
what changed — hashes rather than the server's version markers, because the
question is "did I change this", which is about local content.
And a push never said "delete". A file removed here stayed on the server and
came back on the next pull, which reads as the tool undoing your work. A path in
the record with no file beside it is a deletion, and now travels as one. A file
that never left the machine cannot delete anything by disappearing, and a
never-sync path is left alone either way.
The record is written only after the server accepts the batch. Written earlier,
a failed push would look delivered and the next one would skip exactly the files
that never arrived. An empty batch is not sent at all: it would move the head
for no reason and make every other client re-check a space that did not change.
ResolveMode decides what leaves the machine, and could not be reasoned about.
Two passes: the first with conditions that contradicted each other, the second
overwriting it with a >= comparison that made the answer depend on the order the
rules happened to be listed in — so the same rules in a different order could
differ on whether somebody's identity file was published to their team. It is
one pass now, longest match wins, an exact path beats a prefix of the same
length. That is what every ignore-file anyone has used already does.
ListSpaces could never succeed. The server answers {"spaces":[...]} and this
decoded into a bare []SpaceInfo, so every call failed — and the wizard's
fallback swallowed the error and told the person "the server did not return a
listing for this token", blaming the server for a bug on this side. The space
picker has never once run.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CI ran go test and nothing else, so everything the last three sprints found would have passed it. -race on every platform, because the first run under the detector found a real data race and the class of bug it catches is the one least likely to reproduce on demand. go vet, including the integration-tagged files nothing ever compiled in CI. govulncheck, which reports a dependency only when a reachable call path exists and so stays quiet enough to be read. And golangci-lint. The linter set is deliberately narrow. The full one reports 489 findings — 409 errcheck on deliberate discards like `defer f.Close()`, 43 noctx, 15 errorlint. Switching all of that on produces a gate that is red from its first commit, and a gate nobody can get green is a gate somebody deletes. Those three are recorded as their own piece of work, with the numbers, instead of being enabled and then permanently ignored. What is enabled passes today, so a new finding is a new mistake — the only way a linter earns a place in CI. It found one real defect. Backend migration verified nothing: an `if` with an empty body, under a comment saying the content must match. A migration is the one operation where "it probably worked" will not do, because the operator is about to point the server at the new backend and stop consulting the old one, so a file that arrived wrong is wrong from then on. It reads back and compares bytes now, and stops on the first mismatch rather than carrying on. Content rather than version markers, because the comment was right about that part: two drivers derive versions differently from the same bytes. The rest was dead code, now gone: doBytes, skipStoragePath, orDefaultInt, auditError, two unused lipgloss styles, and an assignment overwritten before it was ever read. One was not dead but unused — structuredOutput, written to keep progress lines off a JSON stream and never called. The update notice added last sprint had hand-rolled the same check against the flags directly; it uses the helper now, because two answers to one question are free to drift apart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A versioned write is three objects: the version blob, the metadata, the live mirror. No backend writes three things at once, so what can be arranged is that every way of stopping part-way leaves a state somebody can explain. It did not. The order was version blob, live, metadata. Stop after the second and the live file holds the new content while the metadata still calls the old version current. A read then returned the new bytes labelled with the old version, so every CAS token handed out described content the caller had never seen, and the next write compared against it and passed. Nothing detected it; nothing repaired it. Reordering alone would only move the inconsistency, so the read moved with it. Get now reads the version blob the metadata names rather than the live copy. Those were always two objects, and labelling one from the other was the bug underneath the ordering. With that, metadata is a real commit point. Stopping before it leaves an unreferenced blob — storage spent, nothing else, and the next attempt at that version number overwrites it. Stopping after it leaves the live mirror stale, which costs a listing that is briefly behind and no reader a wrong answer. The error says which of the two happened rather than reporting a clean failure over a committed write. The mirror stays, because it is what makes a space enumerable: List walks real paths, and a tree of hashed version blobs is not something anybody can look at. It is a mirror, and this was the one place treating it as the truth. A version the metadata names whose blob is missing is now an error rather than a silent fall back to the mirror, which would hand back content belonging to a different version than the one being reported. Verified by restoring the old read: two of the tests fail, one of them with the exact symptom — "read \"first\" at \"2\"". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Login failures counted against the username and nothing else, so anybody who knew a name could lock its owner out by typing the wrong password five times. A denial of service that costs the attacker nothing, that the victim cannot distinguish from a real attack, and that is worse than the attack the lockout was protecting against. Failures now count twice: against the account and against the address they came from. The address budget is the tight one, because it is the budget an attacker actually spends and the only one that cannot be aimed at somebody else. The account budget stays, larger, for the case it is really for — failures arriving from many addresses, where no single address ever reaches its own limit. A success clears the account but not the address. One correct password from a machine working through a list of names must not wipe the record of the failures around it. The two counters are namespaced, because "203.0.113.9" is a legal username and sharing a map with addresses would let one lock the other. What this still does not solve is written down rather than implied: the counters live in one process, so a fleet behind a load balancer hands an attacker a fresh budget per replica and a restart clears everything. The OSS server has no shared state by design — its own docs call the HA "stateless, no clustering" — so an operator running replicas has to rate-limit authentication at the router. These values are a floor, not a fleet-wide guarantee. The existing test asserted lockout after MaxLoginFailures against the account, which pinned the old model; it now uses the account budget. Verified by putting the old counting back: the test that matters fails with "the real owner was locked out by somebody else's failures". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Quota accounting listed the backend and then called os.Stat on the working-tree mirror to learn each file's size. The mirror is written by whichever replica handled the write, so with the s3 or sql driver every other replica found no file, counted it as nothing, and read the space as far smaller than it is — then let it grow past its limit. The server's own documentation calls its HA "stateless, no clustering", which is exactly the arrangement where this is wrong, so it was not a hypothetical for anyone running more than one process. storage.Entry carries a Size now and every backend fills it from what it actually knows. Local records the length when it writes the record, so a listing reports it without touching the body. SQL asks the database with octet_length rather than shipping every blob to count its bytes. S3 stamps the content's own length into object metadata beside the version — the object's reported size is the JSON wrapper, a third larger because of base64, so it is the wrong number. Git measures the file. Prefixed passes it through. Zero means "the backend did not say", not "the file is empty", so the working tree is still consulted as a fallback rather than a file of unknown size being counted as free space. An actually-empty file is zero either way, and a quota check treating it as zero is correct. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A checksum file signs nothing. Whoever can replace the binary on the release page can replace checksums.txt beside it, and the install script fetches both from the same URL — so the check answered "did this arrive intact", never "is this ours". That matters more here than for most projects, because the documented way to install this is `curl … | bash`. Releases are signed with cosign, keylessly: the certificate is issued to the GitHub Actions identity that ran the release and the fact is recorded in the transparency log. There is no private key for anybody to lose or rotate, and the question a verifier can answer is the useful one — "was this built by this repository's release workflow" rather than "do I recognise this key". The checksum file covers every artifact, so signing it signs the release. An SBOM per archive, because otherwise answering "is this release affected by CVE-x" means rebuilding it and hoping the dependency graph has not moved since. The installer verifies before it checksums, and is deliberately forgiving about one thing: a missing cosign is a warning, not a refusal. Refusing to install because a verification tool is absent pushes people to bypass the script entirely, which leaves them worse off than an unverified install they were told about. A signature that exists and does not verify is fatal. Releases published before this have no signature, and the installer says so rather than failing on them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
What this changes
Why
How it was verified
go test ./...passesgofmt -lis clean for the files I touchedDocumentation
--helptext is accurate for anything I added or changeddocs/updated if a command, flag or output format changedCHANGELOG.mdunder[Unreleased], written for someone deciding whether to upgrade