Repository navigation
feat(roadmap): read-only epic status and ready work items (#333) - #356
Conversation
mctl-api reads the RoadmapPublication that mctlhq/.github publishes to its
roadmap-state branch and serves it; it never evaluates readiness.
- internal/roadmap: verifies the publication (apiVersion, sha256 of every
derived file, one ready set and one health per named manifest, ready list
agreeing with item states) and selects from it. Anything unverified is
refused; a failed publication is never replaced by a cached one.
- gitops.Reader: Revision and ReadFiles, one consistent checkout per read.
- REST: GET /api/v1/roadmap/{epics,epic-status,ready}; 503 means cannot say.
- MCP: mctl_get_epic_status, mctl_get_ready_work_items (portal: disabled,
pending an owner decision).
- Every answer carries provenance: state/evaluator/source revisions, the
capture time and its age.
- Tests read a real publication copied byte for byte (testdata/live).
Claude-Session: https://claude.ai/code/session_01TpRNAyt63CpPb9xpQPBy2G
|
Claude finished @mashkovd's task in 7m 30s —— View job Code review complete
Verdict: changes requested — 0 P1, 2 P2, 3 P3. Findings are posted as inline comments on the diff. What holds upThe verification core in
P2 (blocking)
P3 (non-blocking)
Checked and not flagged
Caveat: |
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit Findings[P3] Missing 200 OK test coverage for
|
…o range copy Also accept "f" as false in the kill switch, matching #354. Claude-Session: https://claude.ai/code/session_01TpRNAyt63CpPb9xpQPBy2G
|
Claude finished @mashkovd's task in 6m 20s —— View job Code review complete
Verdict: changes requested — 1 P2, 6 P3. Findings are posted inline on the pinned commit. P2
P3
What holds upThe verification chain is the strong part of this PR: apiVersion/kind pinning, the sha256 check against Note: I could not run |
Antigravity (agy) reviewagy returned a result this workflow could not use; see the workflow log. Treated as a failed review. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| mcplib.WithString("epic", | ||
| mcplib.Description(`Epic name or root issue ("mctlhq/.github#42"). Omit for every active epic.`), | ||
| ), | ||
| mcplib.WithString("required_only", |
There was a problem hiding this comment.
P2 — required_only is declared as a string, so a boolean argument is silently dropped and the answer flips to the opposite of what was asked.
stringArg (internal/mcp/server.go:2640) type-asserts to string. A client that sends the JSON boolean false — which is what an LLM will often emit for a parameter whose description reads "false" to include optional items. Defaults to true. — gets "" from stringArg, line 84 skips q.Set, and the REST handler applies its required_only=true default. The caller asked for optional items, receives only required ones, and gets no error: the ready list simply looks shorter than it is. use_temporal (internal/mcp/server.go:3030) already uses mcplib.WithBoolean for exactly this reason.
Either declare it with WithBoolean and read it with a bool-aware accessor, or accept both shapes in the handler (switch v := req.GetArguments()["required_only"].(type)) so a boolean is not silently discarded.
There was a problem hiding this comment.
Fixed in fd9bbb1: declared with WithBoolean; the handler takes a bool, still accepts a string, and refuses anything else instead of dropping it (TestGetReadyWorkItemsToolBuildsTheQuery; fails with the bool arm removed).
| mcplib.Description(`"false" to include optional items. Defaults to true.`), | ||
| ), | ||
| ) | ||
| handler := func(ctx context.Context, req mcplib.CallToolRequest) (*mcplib.CallToolResult, error) { |
There was a problem hiding this comment.
P2 — neither MCP handler has a test.
internal/mcp/ ships handler tests for its tools in server_test.go (httptest backend, assert method + path + body). The two tools added here are covered only by annotations_test.go, which checks the hint map and the tool count — it never invokes the handlers. Nothing pins:
- that
mctl_get_ready_work_itemsbuilds/api/v1/roadmap/ready?epic=…&required_only=…and omits each parameter when blank (the bug in the comment above would be caught here); - that
mctl_get_epic_statusURL-encodes#inmctlhq/.github#57rather than truncating the query at the fragment; - that a 503 from the REST side surfaces as a tool error whose text still says the publication is unavailable, which is the whole
"cannot say" != "nothing is ready"contract these tools advertise in their descriptions.
A TestGetReadyWorkItemsTool / TestGetEpicStatusTool pair in the style of the existing tool tests would cover all three.
There was a problem hiding this comment.
Added in fd9bbb1: internal/mcp/roadmap_test.go covers query building and omission, '#' encoding, and 503 vs 404 (mutation-checked).
| pub, err := h.opts.Roadmap.Current() | ||
| if err != nil { | ||
| slog.Warn("roadmap publication unavailable", "error", err) | ||
| writeErrorCode(w, http.StatusServiceUnavailable, roadmapCodeUnavailable, err.Error(), nil) |
There was a problem hiding this comment.
P3 — the 503 body is err.Error() verbatim, which carries internal detail out to the client.
Current() wraps whatever the checkout returned, so this can serve roadmap publication unavailable: roadmap-state checkout: open /tmp/roadmap-state/publication.json: no such file or directory or raw git rev-parse stderr. The token itself is safe (redactTokenLocked runs inside gitOutput), so this is disclosure of local paths and git internals rather than a credential leak, and the audience is authenticated. Still, the caller only needs to know that no publication could be verified; the detail already goes to slog.Warn on the line above. A fixed message plus the logged detail would keep the contract intact.
There was a problem hiding this comment.
Fixed in fd9bbb1: fixed 503 message, detail only in the log; the 503 test asserts no path leaks.
| if r == nil || r.src == nil { | ||
| return nil, unavailable("no roadmap-state source is configured") | ||
| } | ||
| rev, err := r.src.Revision() |
There was a problem hiding this comment.
P3 — every request forks a git rev-parse, and a broken publication is re-read and re-verified on every request.
Revision() runs exec.Command("git", …) under the gitops read lock, so each /api/v1/roadmap/* call costs a process fork; a cache miss costs two, because ReadFiles calls revisionLocked() again. More notably there is no negative cache: when Parse fails, r.cached keeps the old (different-revision) publication, so the next call misses again and repeats the full four-file read, three sha256 sums and JSON parse. With the 300 req/min per-user limit, a publication that stays broken turns every roadmap call into that work for as long as the branch stays broken. Caching the failure alongside its revision — serve the stored ErrUnavailable until rev moves — would keep the "never serve stale" guarantee (which TestReaderReverifiesOnlyWhenTheCheckoutMovesAndNeverServesStale pins) while bounding the cost to once per revision.
There was a problem hiding this comment.
Fixed in fd9bbb1: HEAD is recorded at the end of refresh, so no fork per request; a failed verification is cached for its revision and re-read only when the checkout moves (reader test asserts reads=2 across two broken calls).
| func (p *Publication) find(key string) (*epicEntry, error) { | ||
| key = strings.TrimSpace(key) | ||
| for _, e := range p.epics { | ||
| if e.epic.Name == key || (e.epic.Issue != nil && strings.EqualFold(e.epic.Issue.String(), key)) { |
There was a problem hiding this comment.
P3 — the two lookup keys disagree on case: the epic name matches exactly, the root issue matches case-insensitively.
epic=MCTLHQ/.github%2357 resolves (and TestAcceptanceExamplesFollowThePublication pins that it does), but epic=Lifecycle-Ownership is a 404. Callers copying an epic name out of a document with different casing get "epic not found" where the sibling key would have worked. Either fold both with strings.EqualFold or fold neither; the asymmetry is the part that will surprise.
There was a problem hiding this comment.
Fixed in fd9bbb1: names fold case like root issues (test added).
| if name == "" || name != filepath.Base(name) || name == "." || name == ".." { | ||
| return nil, "", fmt.Errorf("invalid file name %q", name) | ||
| } | ||
| data, err := os.ReadFile(filepath.Join(r.localPath, name)) |
There was a problem hiding this comment.
P3 — the name guard blocks path separators but not symlinks, and os.ReadFile follows them.
name != filepath.Base(name) correctly rejects ../x and .git/config (the test covers both), but a file inside the checkout that is a symlink still resolves wherever it points: a publication.json committed to roadmap-state as a symlink to a file outside the checkout is read and fed to json.Unmarshal. For the roadmap caller the blast radius is small — three of the four files are digest-gated, and whoever controls the branch could commit the content directly — but ReadFiles is a general-purpose method on Reader, and the other Reader in this process is pointed at mctl-gitops, whose contents arrive by PR. An os.Lstat check for fs.ModeSymlink before the read closes it in three lines and keeps the method safe for the next caller.
There was a problem hiding this comment.
Fixed in fd9bbb1: Lstat, only regular files are read; test with a symlink out of the checkout.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 0 P1, 2 P2, 3 P3.
The verification core in internal/roadmap is solid — digest gating, the manifest/ready-set/health bijection, the ready-list/state cross-check and the revision-keyed cache are all correct, and the tamper table plus the byte-for-byte fixture tests are the right way to pin them. ReadFiles/Revision take the read lock correctly, so the "one checkout" claim holds and there is no race with refresh().
P2: (1) required_only is declared as an MCP string, so a client sending the JSON boolean false has it silently dropped and receives required-only results — the opposite of what was asked, with no error. (2) Neither new MCP handler has a test; annotations_test.go only covers hints and the tool count, so query-string construction and the 503-vs-404 distinction are unpinned.
P3: internal error text in the 503 body; no negative cache on a failed Parse (plus a git rev-parse fork per request); asymmetric case handling between epic name and root issue; ReadFiles follows symlinks inside the checkout.
Note: I could not run go build/go test in this environment, so the review is static only.
| if r == nil || r.src == nil { | ||
| return nil, unavailable("no roadmap-state source is configured") | ||
| } | ||
| rev, err := r.src.Revision() |
There was a problem hiding this comment.
P2 — Current() forks a git process on every request, including cache hits.
r.src.Revision() is called unconditionally before the mutex and before the r.cached.revision == rev check, and gitops.Reader.Revision() is exec.Command("git", "-C", path, "rev-parse", "HEAD"). So every GET /api/v1/roadmap/epics, /epic-status and /ready — and every mctl_get_epic_status / mctl_get_ready_work_items call, which routes through the same REST handler — spawns a subprocess even when the verified publication is already in memory and nothing has changed.
Failure scenario: the authenticated read group allows 300 req/min per user, and these tools are built for agent polling (the shepherd sweeper already polls /agents/dev-loop/<id> once per proposal per tick). A handful of polling clients becomes a steady few-hundred-fork-per-minute stream on the API pod, each fork holding the gitops RLock and contending with refresh(). No correctness impact, but it is entirely avoidable: refresh() already runs rev-parse --short HEAD at the end while holding the write lock. Record the full HEAD in a Reader field there and have Revision() / ReadFiles return it, so the cache-hit path costs one mutex instead of a fork+exec.
There was a problem hiding this comment.
Fixed in fd9bbb1, see above: Revision/ReadFiles return the HEAD refresh() recorded under the write lock.
| } | ||
| names := map[string]bool{} | ||
| for _, m := range pub.Manifests { | ||
| if m.Epic == nil || m.Epic.Name == "" || m.Epic.Lifecycle == "" { |
There was a problem hiding this comment.
P3 — an empty manifests[].sha256 passes verification and is served as provenance.
This loop requires m.Epic.Name and m.Epic.Lifecycle to be non-empty but never checks m.SHA256. Line 300 then compares doc.wire.Epic.Manifest.SHA256 != m.SHA256, which is satisfied when both sides are empty.
Failure scenario: a publication written with an empty (or omitted) sha256 on every manifest verifies cleanly, and /api/v1/roadmap/epic-status answers 200 with epic.manifest.sha256 empty. The acceptance criterion "provenance explains every answer: manifest path and sha256 per epic" is then satisfied only in shape. The file-digest loop above already guards this case for the three derived files by refusing an empty want; the same guard here would close it. (m.Path empty is already caught indirectly, since parseList refuses an empty manifest path and the lookup then misses.)
There was a problem hiding this comment.
Fixed in fd9bbb1: a manifest without a digest is refused (tamper case added; fails with the check removed).
| func (p *Publication) find(key string) (*epicEntry, error) { | ||
| key = strings.TrimSpace(key) | ||
| for _, e := range p.epics { | ||
| if e.epic.Name == key || (e.epic.Issue != nil && strings.EqualFold(e.epic.Issue.String(), key)) { |
There was a problem hiding this comment.
P3 — lookup is case-insensitive by root issue but case-sensitive by name.
e.epic.Issue.String() is compared with strings.EqualFold, while e.epic.Name == key is exact. TestAcceptanceExamplesFollowThePublication pins the forgiving half (MCTLHQ/.github#57 resolves), so a caller reasonably infers the same tolerance applies to the other argument form.
Failure scenario: an MCP client that title-cases or upper-cases an epic name — Lifecycle-Ownership instead of lifecycle-ownership — gets 404 epic_not_found from /api/v1/roadmap/epic-status, which reads as "no such epic" rather than "wrong case". Either EqualFold both comparisons or make both exact; the asymmetry is the part that surprises.
| pub, err := h.opts.Roadmap.Current() | ||
| if err != nil { | ||
| slog.Warn("roadmap publication unavailable", "error", err) | ||
| writeErrorCode(w, http.StatusServiceUnavailable, roadmapCodeUnavailable, err.Error(), nil) |
There was a problem hiding this comment.
P3 — the 503 body returns the raw err.Error(), which carries server-side filesystem paths.
roadmap.Reader.Current() wraps I/O errors verbatim, so this writes things like roadmap publication unavailable: roadmap-state checkout: open /tmp/roadmap-state/publication.json: no such file or directory, or git stderr from rev-parse, straight into the response and into the MCP tool result the model sees.
Failure scenario: any authenticated caller hitting the endpoint before the first successful clone learns ROADMAP_STATE_LOCAL_PATH and the state of the pod filesystem. Low severity — the group is authenticated and gitops.Reader redacts tokens from git output — but the rest of the package tends to log the detail (slog.Warn above already does) and return a fixed sentence. A constant message such as "no verified roadmap publication" would keep the "cannot say" semantics intact while dropping the internals.
| if epic == "" { | ||
| return mcplib.NewToolResultError("missing required argument: epic"), nil | ||
| } | ||
| body, err := s.apiGet(ctx, "/api/v1/roadmap/epic-status?"+url.Values{"epic": {epic}}.Encode()) |
There was a problem hiding this comment.
P3 — apiGet collapses 404 and 503 into the same tool error, discarding the status the description promises to distinguish.
doRequest turns every status >= 400 into fmt.Errorf("%s", apiErr.Error), keeping only the error string and dropping both the status code and the code field (roadmap_unavailable vs epic_not_found). The tool then reports both as Failed to read epic status: .... The description added just above states that an unavailable publication means "cannot say", never "nothing is ready" — a promise that now rests entirely on the model reading the prose in the message.
This repo already has the counterpart: apiGetStatus exists precisely because "the lifecycle read has to tell 503 from 404, and those two are one sentence apart in the body and a whole decision apart in meaning" (server.go:2547). The same argument applies verbatim here. Using apiGetStatus and branching on the status would make the distinction structural rather than textual.
There was a problem hiding this comment.
Fixed in fd9bbb1: the tools use apiGetStatus; 503 → 'readiness is UNKNOWN, not empty', 404 epic_not_found → 'Epic not found', anything else → HTTP status + message.
| mcplib.WithString("epic", | ||
| mcplib.Description(`Epic name or root issue ("mctlhq/.github#42"). Omit for every active epic.`), | ||
| ), | ||
| mcplib.WithString("required_only", |
There was a problem hiding this comment.
P3 — required_only is declared as a string, so a client sending a JSON boolean silently gets the default.
stringArg does req.GetArguments()[name].(string) and returns "" on a type mismatch. A model that reads required_only as a flag and sends "required_only": false (a JSON boolean, which the schema type does not forbid at every client) fails the assertion, the parameter is dropped from the query string, and the handler applies requiredOnly = true — the caller asked for optional items and gets only the required ones, with no error anywhere.
mcplib.WithBoolean would make the schema say what is meant; if the string form is deliberate (to keep stringArg/extractStringParams uniform), a bool fallback in the handler would at least not silently invert the request.
| } | ||
| } | ||
|
|
||
| func TestRoadmap_EpicStatusAndReady(t *testing.T) { |
There was a problem hiding this comment.
P3 — no 200 test for GET /api/v1/roadmap/epics.
TestRoadmap_UnconfiguredOrBrokenAnswers503 covers the 503 path for all three routes, and this test covers the 200 shape for /epic-status and /ready, but nothing asserts the success shape of /epics. ListRoadmapEpics is the only handler whose response body (epics + provenance) is unverified, so a regression in its shaping — a renamed key, an empty list from a lifecycle filter added later — would pass the suite. roadmap_test.go covers Publication.Epics() itself, not the HTTP envelope.
A few lines in this test asserting 200, a non-empty epics array and a populated provenance would close it. (Also raised by the Antigravity pass.)
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 1 P2, 6 P3.
P2: roadmap.Reader.Current() calls src.Revision() before the cache check, and gitops.Reader.Revision() shells out to git rev-parse HEAD — so every roadmap REST request and every MCP roadmap tool call forks a subprocess even on a cache hit. refresh() already computes HEAD under the write lock; caching it there removes the fork.
P3: empty manifests[].sha256 passes verification; name lookup is case-sensitive while root-issue lookup is not; the 503 body returns raw err.Error() with local paths; apiGet collapses 404 and 503 (the apiGetStatus precedent applies); required_only declared as a string silently defaults on a JSON boolean; no 200 test for /api/v1/roadmap/epics.
The verification logic, the tamper table, the revision-keyed cache and the byte-for-byte passthrough all hold up; ReadFiles correctly takes files and HEAD under one read lock, the routes sit inside auth and outside the write group, and the kill switch fails toward off. Could not run go build / go test in this environment, so the review is static.
One kill-switch reader for WORK_ITEMS_DISABLED and ROADMAP_STATE_DISABLED; docs list f as false, matching the code. Claude-Session: https://claude.ai/code/session_01TpRNAyt63CpPb9xpQPBy2G
…rrors Review round on #356: - gitops.Reader records HEAD at the end of every refresh; Revision and ReadFiles read it instead of forking git per request. - The roadmap reader caches a failed verification per revision too, so a broken publication is re-read only when the checkout moves. - mctl_get_ready_work_items declares required_only as a boolean (a string is still accepted; anything else is refused, never dropped). - MCP tools use the status: 503 says readiness is UNKNOWN, 404 says epic not found. - A fixed 503 message; the detail stays in the log. - Refuse a manifest without a digest; fold case on epic names like issues; ReadFiles refuses symlinks. - Tests: MCP handler tests (query building, encoding, 503 vs 404), 200 on /epics, negative cache, symlink, revision after refresh. Claude-Session: https://claude.ai/code/session_01TpRNAyt63CpPb9xpQPBy2G
|
Claude finished @mashkovd's task in 5m 43s —— View job Code review complete
Verdict: changes requested — 0 P1, 1 P2, 2 P3. Findings are posted inline on What the fix commit closedEvery finding from the previous round is addressed, and each one is pinned by a test rather than by prose:
The defer that records P2 (blocking)
P3 (non-blocking)
Checked and not flagged
Caveat: |
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit No significant issues found. |
| return out, "0123abc", nil | ||
| } | ||
|
|
||
| func roadmapRouter(h *Handlers) chi.Router { |
There was a problem hiding this comment.
P2 — nothing pins that the three roadmap routes are registered, and this file is the only roadmap test that touches HTTP.
roadmapRouter builds a fresh chi.NewRouter() and mounts the handlers directly, so the whole suite passes without NewRouter ever being consulted. Delete the three r.Get("/roadmap/...") lines from internal/api/router.go and go test ./... stays green while GET /api/v1/roadmap/epics answers 404 — and the MCP tools, which go through REST, report Failed to read epic status (HTTP 404): 404 page not found, which a model will read as "no such epic" rather than "the endpoint is gone".
This repo has already been bitten by exactly this and wrote the guard twice, with the reasoning spelled out above each: TestLifecycleRoutesAreRegistered ("deleting a route line left the suite green while the endpoint did not exist ... that is how recover came to be listed in the PR description and absent from the router") and TestUsageRoutesAreRegistered ("a handler with no route is an endpoint that does not exist"). The same chi.Walk over NewRouter(Options{}), asserting the three GET /api/v1/roadmap/* paths, is about eight lines here. It is also the only thing that would notice if the routes later moved out of the authenticated group or into the 20/min write group — neither of which this file can see, since it never builds the real router.
There was a problem hiding this comment.
Added in 639bdcc: TestRoadmapRoutesAreRegistered walks NewRouter; deleting a roadmap route line fails it.
| if m.Epic == nil || m.Epic.Name == "" || m.Epic.Lifecycle == "" { | ||
| return nil, unavailable("manifest %s carries no epic identity", m.Path) | ||
| } | ||
| if names[m.Epic.Name] { |
There was a problem hiding this comment.
P3 — the duplicate-epic guard is case-sensitive, but find is now case-insensitive, so two epics that differ only in case verify cleanly and then resolve ambiguously.
names[m.Epic.Name] matches exactly, while find (roadmap.go:410) folds both the name and the root issue with strings.EqualFold — the change this commit made in response to the previous round. A publication carrying lifecycle-ownership and Lifecycle-Ownership as two manifests therefore passes verification, and EpicStatus("Lifecycle-Ownership") silently answers with whichever of the two comes first in manifest order: the readiness and health of the wrong epic, served 200, with provenance naming the wrong manifest path and digest.
Low likelihood — the names come from a generator, and the rest of this function refuses far less exotic inconsistencies. But it is the one invariant in Parse whose comparison no longer agrees with the lookup it protects; folding the key here keeps "two epics are the same epic" as one definition instead of two.
There was a problem hiding this comment.
Fixed in 639bdcc: the guard folds case; tamper case 'two epics equal but for case' added (fails with the exact-match guard).
| // may not, and an absent checkout has none. | ||
| defer func() { | ||
| r.head = "" | ||
| if out, err := r.gitOutput(nil, "rev-parse", "HEAD"); err == nil { |
There was a problem hiding this comment.
P3 — git rev-parse HEAD walks up to a parent repository, so head can name a commit that has nothing to do with this checkout.
Caching HEAD here is the right fix for the per-request fork, and the defer's placement is correct: registered after defer r.mu.Unlock() and before the credential-scrub defer, so LIFO runs it under the write lock and after the scrub. The gap is the command itself — git -C <localPath> rev-parse HEAD succeeds when localPath is not a repository but sits inside one, and answers with the enclosing repository's HEAD.
Failure scenario: ROADMAP_STATE_LOCAL_PATH points at a path under an existing checkout (a dev box, or a job whose workspace is the repo), and the clone fails or has not happened yet. refresh() returns an error, which RefreshLoop only logs, but this defer still records a plausible 40-hex head. Revision() then reports it, ReadFiles reads whatever publication.json is at that path, and if that file verifies internally it is served with state_revision naming an unrelated commit — the one field clients are told to trust for "which roadmap-state commit am I looking at". Checking rev-parse --git-dir (or comparing --show-toplevel with localPath) keeps the field meaning what it says.
There was a problem hiding this comment.
Fixed in 639bdcc: HEAD is recorded only when localPath has its own .git; test clones a missing repo into a dir inside another repo and expects no revision (fails without the check).
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 0 P1, 1 P2, 2 P3.
Every finding from the previous round is fixed and pinned by a test: required_only is now WithBoolean with a bool/string/refuse switch (TestGetReadyWorkItemsToolBuildsTheQuery), roadmapFetch uses apiGetStatus and keeps 503 apart from 404, the 503 body is a constant while the detail goes to slog, an empty manifests[].sha256 is refused, name lookup folds like the root-issue lookup, ReadFiles rejects non-regular files (symlink test included), head is recorded under the write lock in refresh() so the cache-hit path no longer forks git, and Parse failures are cached per revision.
P2: the roadmap HTTP tests build their own chi router, so nothing exercises NewRouter — deleting the three r.Get("/roadmap/...") lines leaves the suite green with the endpoints gone. This repo wrote that exact guard twice already (TestLifecycleRoutesAreRegistered, TestUsageRoutesAreRegistered); an eight-line chi.Walk closes it and is also the only thing that would notice the routes drifting out of the auth group or into the write budget.
P3: the duplicate-epic guard in Parse is still case-sensitive while find now folds case, so two epics differing only in case verify and then resolve ambiguously; git -C <path> rev-parse HEAD in the refresh defer walks up to an enclosing repository, so a misconfigured or unclonable ROADMAP_STATE_LOCAL_PATH can record a head that belongs to another repo and be served as state_revision.
The verification core, the tamper table, the byte-for-byte fixture comparisons, the revision-keyed cache and the one-checkout locking in ReadFiles all hold up, and the routes sit inside AuthMiddleware and outside the 20/min write group. go build and go test were not permitted in this environment, so this is a static review; CI should be checked before merge.
…EAD only - TestRoadmapRoutesAreRegistered walks NewRouter for the three reads. - The duplicate-epic guard folds case, as lookup does. - refresh records HEAD only when localPath has its own .git, so a failed clone inside another repository never borrows that repository's HEAD. Claude-Session: https://claude.ai/code/session_01TpRNAyt63CpPb9xpQPBy2G
|
Claude finished @mashkovd's task in 5m 1s —— View job Code review complete
Verdict: approved — 0 P1, 0 P2, 3 P3. Findings are posted inline on What the fix commit closed
The nested-repo test is not vacuous: What holds upThe verification chain is the strong part and it is unchanged: apiVersion/kind pinning, the sha256 check of Concurrency is sound.
P3 (non-blocking)
Caveat: |
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit No significant issues found. |
| // Folded, because lookup folds: two names equal but for case would | ||
| // otherwise both verify and then resolve ambiguously. | ||
| folded := strings.ToLower(m.Epic.Name) | ||
| if names[folded] { |
There was a problem hiding this comment.
P3 — the duplicate guard now folds the epic name, but nothing guards the other lookup key: the root issue.
find (roadmap.go:407) resolves on either e.epic.Name or e.epic.Issue.String(), and the root-issue form is the one the MCP tool description advertises ("mctlhq/.github#57") and the one TestAcceptanceExamplesFollowThePublication exercises. entry.epic.Issue comes straight from r.wire.Epic.Issue (roadmap.go:313) with no uniqueness check anywhere in Parse.
Failure scenario: two EpicDefinitions in mctlhq/.github carry the same issue block (a copy-paste in a generated manifest is the realistic way this happens). Both manifests verify — distinct names, distinct paths, matching digests — and GET /api/v1/roadmap/epic-status?epic=mctlhq/.github%2357 answers 200 with whichever comes first in manifest order: the wrong epic's readiness and health, with provenance naming the wrong manifest path and sha256. The same ErrEpicNotFound-vs-ambiguity distinction this commit just closed for names is still open for issues, and the name case is the one that could only ever have come from a generator bug too.
Folding the issue into the same names set (or a sibling set keyed on Issue.String()) closes it with one line and the tamper table already has the shape for the test.
| roadmapGit = gitops.NewReader( | ||
| envOr("ROADMAP_STATE_REPO_URL", "https://github.com/mctlhq/.github.git"), | ||
| envOr("ROADMAP_STATE_BRANCH", "roadmap-state"), | ||
| envOr("ROADMAP_STATE_LOCAL_PATH", "/tmp/roadmap-state"), |
There was a problem hiding this comment.
P3 — the second checkout defaults onto the container's root filesystem, where the first one deliberately does not.
GITOPS_LOCAL_PATH is /data/mctl-gitops in helm/values.yaml, and helm/templates/deployment.yaml:120 backs it with a dedicated emptyDir carrying sizeLimit: 500Mi. /tmp/roadmap-state has neither: it lands on the node's ephemeral storage, which the chart sets no request or limit for, so the only thing bounding it is the node's disk-pressure eviction.
The blast radius is small in practice — refresh() clones --depth=1 --single-branch (reader.go:337) and re-fetches at the same depth, so the checkout stays around a megabyte and does not grow with the branch's history. What is left is the asymmetry: an operator reading the chart sees one checkout with a sized volume and no sign that a second one exists, and repeated fetch --depth=1 on a long-lived pod does accumulate loose objects with no git gc in the loop.
A ROADMAP_STATE_LOCAL_PATH entry under values.yaml's env: map (which already passes through generically, so no template change is needed) pointing at the existing volume, or a second small emptyDir, would make the storage story explicit.
| AUDIT_DB_URL= | ||
| # workitem/v1 store (/api/v1/work-items*); falls back to AUDIT_DB_URL. | ||
| # Any WORK_ITEMS_DISABLED value except false/0/no/off turns it off (routes answer 503). | ||
| # Any WORK_ITEMS_DISABLED value except false/f/0/no/off turns it off (routes answer 503). |
There was a problem hiding this comment.
P3 — this commit touches .env.example to correct the WORK_ITEMS_DISABLED wording, but does not add the five ROADMAP_STATE_* variables it introduces.
README.md gains all five rows (ROADMAP_STATE_REPO_URL, _BRANCH, _LOCAL_PATH, _TOKEN, _DISABLED), and .env.example is the file an operator copies to .env — WORK_ITEMS_DB_URL / WORK_ITEMS_DISABLED are both listed there for exactly that reason. Someone bringing up a local API from the template has no indication that a second git checkout is about to be cloned into /tmp/roadmap-state, nor the one variable that stops it. That matters more for this feature than for most, because the reader starts unconditionally at boot and fails only into log lines.
Five commented lines mirroring the WORK_ITEMS_* block would keep the template and the README saying the same thing.
There was a problem hiding this comment.
No P1/P2 findings (3 P3). Good to merge.
Every finding from the previous round is closed, each pinned by a test rather than by prose: TestRoadmapRoutesAreRegistered walks the real NewRouter for the three GETs, the duplicate-epic guard folds to match find, and the refresh defer now gates rev-parse HEAD on localPath/.git so a failed clone inside another repository cannot borrow its HEAD (the nested-repo test is genuine: work is a real checkout, so it would fail without the guard).
Verification core, locking and wiring all hold up: digest gating against publication.json, the manifest/ready-set/health bijection with the leftover check, the ready-list vs. item-state cross-check over a closed vocabulary, the revision-keyed cache that caches failures per revision and never serves stale, ReadFiles taking files and HEAD under one read lock with Lstat-gated regular files only, head written under the write lock and read under the read lock, routes inside AuthMiddleware and outside the 20/min write group, and killSwitchOn failing toward off. The OpenAPI additions are well-formed and inherit the global BearerAuth.
P3: no uniqueness guard on the root issue, which is a first-class lookup key alongside the now-folded name; ROADMAP_STATE_LOCAL_PATH defaults to /tmp while the sibling checkout gets a size-limited emptyDir; the five new ROADMAP_STATE_* vars are in README but not .env.example.
Caveat: go build/go vet/go test were not permitted in this environment, so this is a static review — CI should be green before merge.
Closes #333.
mctl-api is a read-only consumer of the RoadmapPublication that mctlhq/.github publishes to its generated
roadmap-statebranch (.github#119/#121, epic metadata in .github#123). It never runs the evaluator, never infers readiness, never writes to GitHub.What
internal/roadmapverifies a publication, then selects from it:apiVersion/kindmust beroadmap.mctl.ai/v1alpha1/RoadmapPublication;snapshot.json,ready-set.jsonandhealth.jsonmust matchpublication.json;capturedAt;Anything that fails any of these is refused (
ErrUnavailable→ 503). A cached publication is served only for the commit it was read at, never in place of a broken newer one.gitops.ReadergainsRevision()andReadFiles(), which read all files and HEAD under one read lock, so they come from one checkout.cmd/apistarts a second Reader onhttps://github.com/mctlhq/.github.git@roadmap-state(public, no credential), refreshed every 5 min.ROADMAP_STATE_*env, including a kill switch.REST, authenticated, outside the write group:
GET /api/v1/roadmap/epicsGET /api/v1/roadmap/epic-status?epic=<name | owner/repo#N>: the published RoadmapReadySet and RoadmapHealth for the epic, byte for byte, plus lifecycle, title, goal and manifest.GET /api/v1/roadmap/ready?epic=&required_only=(defaulttrue): the items the published ready list names. Withoutepic, everylifecycle: activeepic.MCP:
mctl_get_epic_status,mctl_get_ready_work_items, both read-only.enabled: false: exposing them on the shared portal is an owner decision.Provenance on every answer:
state_revision(roadmap-state commit),evaluator_revision,source(repository, ref, revision),observation(mode,capturedAt, apiBase) andage_seconds. A client judges freshness itself; 503 means "cannot say", never "nothing is ready".Acceptance criteria
TestAcceptanceExamplesFollowThePublication).TestReadyItemsAreExactlyThePublishedReadyList,TestEpicStatusIsThePublishedDocumentsByteForByte). The ordering itself is tested in .github's evaluator fixtures.Tests
internal/roadmap/testdata/liveis a real publication copied byte for byte fromroadmap-state@a36af84(verified withpublish.py verify). Tests compare against those bytes and never restate a readiness of their own.ErrUnavailable.required_onlydefault;ReadFilestest against a real git checkout; kill-switch test.required_onlyfilter and the ready-state filter;required_onlydefault.go test -p 1 ./...with Postgres: green.Note:
internal/openapi/openapi.yamlonmaindoes not parse (line ~2498); #354 fixes it and addsTestSpecParses. The roadmap paths added here parse once that line is fixed; checked locally.🤖 Generated with Claude Code
https://claude.ai/code/session_01TpRNAyt63CpPb9xpQPBy2G