feat(mcp): add manage_adr mode='append'

[andis777]

Problem

manage_adr can only replace: mode='update' overwrites the stored document in full. Adding a single entry to a long-lived ADR therefore costs a full re-send of the whole document.

That is more than an efficiency problem. The caller has to reproduce every byte it did not intend to change, so the prefix is only as safe as the round-trip that carried it — an agent re-emitting ~60 KB of prose to append one paragraph has ~60 KB of opportunity to silently drop or mangle text that nobody asked it to touch. Verifying the result means fetching the document back and diffing it, which doubles the cost again.

Change

Adds mode='append', which concatenates server-side so the stored copy is the only source of the prefix:

• Trailing newlines on the stored content are trimmed and exactly one blank line is inserted, so repeated appends cannot accumulate whitespace.
• An empty or missing ADR degrades to a plain create (no leading blank line), so append is safe as a first write.
• append without content returns status='missing_content' and isError instead of falling through to the get branch — a caller that meant to write must not receive a success-shaped read. (update has the same fall-through today; left alone to keep this change scoped.)
• The response carries content_length and appended_length so callers can confirm the write landed without re-fetching the document.

get, update, store and sections are untouched.

The mode enum and content now carry descriptions, mostly so update = REPLACE the whole document is visible at the call site rather than something you learn by overwriting an ADR.

Tests

Four cases in tests/test_mcp.c:

• tool_manage_adr_append_extends_without_replacing — prefix survives verbatim, new chunk lands after it, exactly one blank line joins them, both sections still parse via mode='sections'.
• tool_manage_adr_append_creates_when_absent — exact-match assert that no leading blank line is introduced.
• tool_manage_adr_append_without_content_errors — errors and leaves the stored ADR byte-identical.
• tool_manage_adr_append_is_advertised — the mode appears in tools/list, so callers can discover it instead of continuing to pay for rewrites.

Verification I could not do

I was unable to build or run the test suite — no C toolchain was available on the machine this was written on. Please treat compilation as unverified; I would rather flag this than have it discovered in review.

What I did instead:

• Checked every API used against existing call sites in the tree (heap_strdup, SKIP_ONE from src/foundation/constants.h, yyjson_mut_obj_add_uint as used in src/pipeline/artifact.c, cbm_mcp_handle_tool, ASSERT_STR_EQ), and mirrored the existing size_t/SKIP_ONE indexing idiom from adr_list_sections_from_content to stay within the project's -Werror settings.
• Ported adr_append_content to a scratch script and ran the boundary cases: CRLF endings, multiple trailing newlines, empty existing content, content consisting only of newlines, empty addition, and four appends in a row (no triple newlines accumulate). All behaved as asserted in the C tests.
• Ran scripts/security-audit.sh: passes. It reports src/mcp/mcp.c has 17 file read operations (expected max 15), but that is pre-existing — git show HEAD~1:src/mcp/mcp.c | grep -c 'fopen\|fread\|read_file' is also 17. This change adds no file reads.

Happy to adjust naming (append vs add), the separator policy, or the missing_content status if you'd prefer different conventions.

🤖 Generated with Claude Code

[andis777]

CI is green — this closes the verification gap flagged in the description

The PR description says compilation was unverified because no C toolchain was available where this was written. CI has now covered exactly that, so please disregard that caveat: all 25 checks pass.

Builds and full suites on every target: pr-smoke (ubuntu-latest, macos-14, windows-latest), test-unix (ubuntu-latest + ubuntu-24.04-arm with gcc/g++, macos-14 + macos-15-intel with cc/c++), test-windows (CLANG64/x86_64), test-tsan (ubuntu-latest, ubuntu-24.04-arm, macos-14). Plus lint, security / security-static, security / codeql-gate, security / license-gate, dco, test / test-windows-guards, test / shard-completeness.

The four new tests actually executed

"Suite passed" is not the same claim as "my tests ran", so I checked. Per-test names are not in the CI logs — a control grep for the pre-existing tool_manage_adr_unified_backend_issue256 also returns nothing, so their absence proves nothing either way. The test counts do prove it. Comparing this branch against the main run at b6b55192:

shard	main	this PR	delta
test-unix (ubuntu-latest, gcc, 1/3)	1854 passed	1858 passed	+4
test-unix (ubuntu-latest, gcc, 2/3)	1959 passed	1959 passed	0
test-unix (ubuntu-latest, gcc, 3/3)	2804 passed	2804 passed	0

tests/test_mcp.c runs in shard 1/3, which is up by exactly the four added cases, with 0 failed throughout. No other shard moved, so nothing else was disturbed.

One note on the audit, in case it comes up in review: scripts/security-audit.sh passes but prints REVIEW: src/mcp/mcp.c has 17 file read operations (expected max 15). That is pre-existing drift against EXPECTED_MAX, not something this PR introduces — git show HEAD~1:src/mcp/mcp.c | grep -c 'fopen\|fread\|read_file' is also 17. This change adds no file reads. Happy to bump the constant in a separate commit if you'd like it brought back in line, but I left it alone to keep this PR scoped.

[DeusData]

Thanks for the careful tests and docs. This needs an ADR-model decision before implementation approval. The current append mode is a non-idempotent read-concatenate-replace operation in the MCP handler, so a retry after a lost response duplicates content; it also bypasses the existing store-level section-update path and its 8,000-character bound. Leading newlines in the addition violate the advertised one-blank-line rule, and empty content can report success while appending zero bytes. If append semantics are accepted, the implementation will need store-layer atomicity, explicit retry/idempotency and size rules, plus tests for repeated requests, concurrent append, leading newlines, empty content, and response length fields. No need to revise until the maintainer direction is decided.

[DeusData]

Thanks for this, and sorry for the wait. Queued for review.

MERGEABLE with all 28 checks green, +205/-3 across three files.

An append mode for manage_adr is a sensible-sounding addition — ADR content is naturally accumulative and a read-modify-write from the client side is easy to get wrong. It does extend the MCP tool contract, so it will get a look on interface design as well as implementation.

[DeusData]

Reviewed in full. The implementation is clean and the security question came back clean too — what is holding this is a surface decision that belongs to the maintainer, not a problem with your work.

The security question I most wanted answered, answered. Because this mode writes content at an LLM's request, I went looking for a path-traversal surface. There isn't one: the append path performs no filesystem write at all. Storage is the SQLite project_summaries table through a parameterised UPSERT with project and content as bound parameters, so no path is ever constructed from tool arguments — a hostile project value is simply an unmatched database key and the handler errors out before any write. No raw fopen added anywhere; the only nearby file read is the pre-existing legacy-migration path, which builds from the store-registered root and already uses cbm_fopen(). The UPSERT is atomic, so an interrupted process leaves the old row or the new one, never a half-written document.

Things you got right that are worth naming:

• The missing_content guard is a genuinely good catch. Without it, append with no content would fall through to the get branch and hand a writer a success-shaped read. You spotted a real pre-existing trap pattern and closed it — and you deliberately left the analogous update fall-through out of scope with a note, which is exactly the right scoping instinct.
• The tests are better than most. Byte-precise ASSERT_STR_EQ on the exact stored document, the single-blank-line contract asserted as literal bytes, and — the detail I liked most — the no-content case verifies the stored ADR is byte-identical afterwards, not merely that an error came back. All four fail without the change.
• The whitespace policy (trim trailing \n/\r, exactly one blank line, degrade-to-create on an empty or missing ADR) is carefully reasoned, and the CRLF handling shows you were thinking about the Windows story this project cares about.
• Your PR description was honest that you could not compile locally, and explained what you did instead — checking every API against existing call sites and porting the join logic to a scratch harness. CI has since confirmed green across all platforms and the sanitizer legs, so that caveat is resolved.

Why it is not merged yet. Adding a mode to manage_adr is permanent public MCP surface — a one-way door — and the same bytes are reachable today via get, client-side concatenation, and update. Your counter-argument is a good one and I have passed it on rather than paraphrasing it away: with update the agent must re-emit the whole document, so every unchanged byte survives only as well as the LLM round-trip, whereas server-side concatenation makes the stored copy the authority for the prefix. That is a data-integrity argument, not a token-saving one, and it is the strongest thing in favour.

One alternative the maintainer will weigh, which you could not have known about: the store layer already contains an unexposed cbm_store_adr_update_sections (store.c:7381) with zero callers outside store.c, and it already enforces the 8000-character CBM_ADR_MAX_LENGTH cap. A section-level update mode may end up being the shape we open instead.

Three things worth fixing regardless of which way that lands:

1. No size cap on append. CBM_ADR_MAX_LENGTH is enforced only by that unexposed section function — MCP update and now append both bypass it. Append in particular makes unbounded incremental growth cheap, and each call is an O(n) full re-read and re-write, after which an oversized ADR bloats every get_architecture response. Either reject above a cap (status='too_large'), or we decide deliberately that ADRs are uncapped and delete the dead constant.
2. A lost-update race that is new. get → merge → store are separate statements rather than a transaction. update today is a single atomic UPSERT, so this read-modify-write window does not currently exist — append introduces it. A concurrent writer (the UI /api/adr full replace, or a second MCP client) landing between the read and the UPSERT gets silently overwritten by stale-prefix-plus-addition. Low severity for a local single-user tool, and BEGIN IMMEDIATE … COMMIT around the pair closes it.
3. Minor: content: "" passes the !content check and rewrites the row while appending nothing — it probably deserves missing_content too.

I will come back to you with the direction answer. Thank you for the care on this one; the review was quick because the work was tidy.