diff --git a/docs/spec-mate.md b/docs/spec-mate.md index 7dccea07e..80121b8ef 100644 --- a/docs/spec-mate.md +++ b/docs/spec-mate.md @@ -2982,16 +2982,32 @@ the next one. Describing never opens a change, and with none open there is nothi **A change's pictures (2026-09-29; in HQ since 2026-10-02).** The pictures that prove a change are the screenshots `zerops_browser` takes, and each is kept as one of the Mate's pictures (`workflow.KeepPicture`, under the state dir) and named in the tool's result — `screenshot.picture`, -`shot-3`, an id never given twice; the newest twenty are kept, and every one a kept description -names. A description shows one as `![what it shows](shot-3)`: `describe-change` sends each picture +`shot-3`, an id never given twice, with how long it is kept (`screenshot.keptFor`). The bound is +time and bytes, not a count: a picture is kept for a week after it is taken +(`workflow.PictureKeepFor`) — a change's "before" is often taken hours, or a whole step, ahead of +its description (run 12: 30 pictures taken over seven hours, refused under a newest-twenty rule) — +while the store holds at most 256 MiB (`workflow.PictureStoreBytes`), past which the oldest go +first (counting the files on disk); the newest is always kept, and every one a kept description +names outlives both — a pair record that does not parse is skipped, and nothing is pruned while the +pairs' directory cannot be read. A picture already on a change outlives its file: its entry keeps the +address HQ serves it at while that change is a pair's current one (90 days at most), so a change made +a draft by a push is described again with the same picture; only another change needs the file, and +a refusal lists only the pictures the change being described can show. Reading the store writes +nothing. One store +per state dir, shared under its own lock by every zcp process of the Mate (its conversation, +helpers, subagents). A description shows one as `![what it shows](shot-3)`: `describe-change` sends each picture it names to HQ as the change's attachment (`POST /api/mate/changes/:repo/:n/attachments`, the PNG as the body, at most 20 MiB, once per change, the address remembered with the picture) and publishes it as `` at `https:///api/apps//changes///attachments/`, which the client reads as the person — the width and height the picture's shape at 720 pixels wide at most, so a reader reserves its box before the bytes arrive. Never a body with a broken picture: a -picture the Mate does not keep is refused before anything is written, and one HQ will not keep -writes nothing and keeps the words. `TestDescribeChange_Pictures`, `TestKeepBrowserPicture`, -`TestKeepPicture_*`, `TestPictureRefs`. +picture the Mate does not keep is refused before anything is written — the refusal names the +pictures it does keep and how long one is kept; one pruned while the words went on is refused the +same way and the words are not kept — and one HQ will never take (over 20 MiB, or a 4xx other than +408/429) writes nothing and drops the words, while HQ refusing for now keeps them for the next +delivery. A picture's file kept only as another change's address is one the change being described +cannot show. `TestDescribeChange_Pictures`, `TestKeepBrowserPicture`, +`TestKeepPicture_*`, `TestDescribeChange_MissingPictureNamesWhatIsKept`, `TestPictureRefs`. **A merge may come at any moment (2026-10-03; delivered through HQ).** A change appears in its Mate's conversation as soon as it opens, and the person may merge it while the agent still @@ -3239,7 +3255,7 @@ and a build HQ did not make is followed by its Zerops process (`GET /process/{id | MB-31 | In a Mate, enrolled yet or not, launch-production refuses before it reads a scope or writes anything, and its next step names only a change the Mate's fresh state says is still open; a merged one points at the projects page, and none, or one closed without merging, says to deliver through the stage half first; with no readable enrollment it says the answer is HQ's. A container that is not a Mate keeps its route, an enrollment file on its disk notwithstanding. zcp `TestHandleLaunchProduction_AMateRefusesBeforeAnyStep` (the handler, Mate and not, enrolled and not: no SSH read, admin client, staged token or state file before the refusal), `TestLaunchProduction_AMateDeliveringThroughHQRefusesAndSaysWhatIsTrue`, `TestLaunchProduction_AMateWithoutItsEnrollmentSaysSo` (the next step). | | MB-32 | A project's flow and its one next step are one derivation the projects page, the left menu and a Mate's conversation all draw (D29): worst first, a failed deploy the next step — production's before a stage's — a failed stage never hiding a release, and a failed production keeping the release that might clear it; production "After the first merge" with nothing to press while `main` is empty; a preview only the stage half beside its own dev half; a group stage drawn only where one exists; the conversation's step from the flow — its own mergeable change, as _Review_ at the composer's top, and nothing of the project's. The page lays it out one way: a _Next steps_ item carries its row's own verb (_Review_, _Review release_, _+ Add production_), a _Review_ opening the review and nothing merging or releasing from the page, and a step with no verb to press is not listed; a cell holds at most two lines, a row one verb at the end of the cell it acts on (and _Review release_ beside a failed production); an empty step is a muted word, never a dashed place; _Add stage_ and _Add production_ are the project menu's, never a footer link; a Mate opens from its chip, its tile or its card. The left menu draws production as one chip on the project's heading, a stage only where there is no production, and dots no heading — a folded one shows its busy Mates' faces. `groupFlow.test.ts` — "takes the worst step first: $case", "reads production as $case", "offers Add production only where $case → $addable", "still ranks a failed production above the release", "names production's failure before a stage's, and keeps the release a failed stage does not block", `pairPreviewRoute` "is $case"; `mateNextStep.test.ts` — "$case" (its cases include "its own change, waiting for the person's review", "another Mate's change", "its own change that does not merge"); `OverviewView.test.tsx` — "carries the step's verb %s on each strip item, the row's own verb", "leaves a step with no verb to press out of the strip", "never holds more than two lines in a cell: %s", "puts the verb in the cell it belongs to", "still offers the release beside a broken production, not just the build (D28)", "draws a group stage under main only where one exists — never an empty slot", "reads production as coming after the first merge, with nothing to press", "opens any Mate a row names into its conversation, by a real button in reading order", "opens a tile's Mate from the whole tile, with no Open button beside it"; `ProjectsView.test.tsx` — "names the next step in its header, without the verb", "keeps Add stage and Add production in the group menu: no footer links", "draws each Mate as a row that carries its Preview on its first line"; `flowSteps.test.tsx` — "says its word in the muted hand, never dashed: %s", "holds its verbs at the cell's end, in order"; `projectsView.logic.test.ts` — "says %s awaits somebody: %s"; `SidebarZeropsTree.test.tsx` — "never dots a project for the production it does not have"; `SidebarProductionChip.logic.test.ts` — "$state" (its cases include "no production, only stage"), "draws no chip where there is neither a production nor a stage"; `SidebarProjects.logic.test.ts` — "headingFaces — who a folded project's heading shows (M15)"; `ZeropsReviewDoors.test.tsx` — "every door opens the review and never acts itself (R1)". | | MB-33 | A Mate proposes only the recipe tiers `main` of the application's recipe repository in HQ lacks (D30): a tier directory `main` has any file in is left whole, the group's first recipe lands whole, a top-level file `main` has is never proposed. The proposal is the Mate's own change in `group`, titled exactly "Mate: the group's import files" with no description, its branch `main`'s tree with the missing files written over it, so it only adds; it moves forward only — a changed composition on its head, with `main` as a second parent once `main` moved — and one `main` has overtaken is brought to `main`'s tree and adds nothing. A group whose `main` has every tier gets no change opened, and the agent's `group-recipe` answers that `main` already carries every tier. Every `buildFromGit` is the pair's repository in HQ. zcp `TestMissing_ATierTheRepositoryHas_IsLeftWhole`, `TestGroupRecipe_ProposedAsTheMatesChange`, `TestGroupRecipe_ProposesOnlyWhatMainLacks`, `TestGroupRecipe_FollowsTheProjectOnTheSameChange`, `TestGroupRecipe_MainMovedUnderAnOpenProposal`, `TestGroupRecipe_AProposalMainOvertookAddsNothing`, `TestGroupRecipe_OpensNothingMainAlreadyHas`, `TestHandleGroupRecipe_Table`. | -| MB-34 | A Mate's description of its change is its change's body in HQ: set on the pair's open change — on record or named by the Mate's state, never opened by describing — kept on the pair while HQ does not answer and put on with the next push that moves nothing (one that opens or moves the change drops it: the change stays a draft until described again), and never put on a change it was not written for (one that merged or closed first is named to the Mate and nothing is kept; words kept for a change that is gone are dropped); every delivery or push that leaves a change open asks for it. The pictures it names are the Mate's own kept screenshots, attached to the change once each and published as `` at HQ's address; a picture not kept is refused before anything is written, and one HQ will not keep writes nothing and keeps the words. A wired pair's push credential is brought to the enrollment's before a delivery or a git-push, and a credential refusal marks the pair, which heals once a fresh session authenticates. zcp `TestDescribeChange`, `TestDescribeChange_KeptWordsMeetThePushAfterThem`, `TestDescribeChange_Refusals`, `TestWorkflowTool_DescribeChangeReachesItsHandler`, `TestDescribeChange_Pictures`, `TestKeepBrowserPicture`, `TestADeliveryBringsTheCredentialToTheCurrentOne`. | +| MB-34 | A Mate's description of its change is its change's body in HQ: set on the pair's open change — on record or named by the Mate's state, never opened by describing — kept on the pair while HQ does not answer and put on with the next push that moves nothing (one that opens or moves the change drops it: the change stays a draft until described again), and never put on a change it was not written for (one that merged or closed first is named to the Mate and nothing is kept; words kept for a change that is gone are dropped); every delivery or push that leaves a change open asks for it. The pictures it names are the Mate's own kept screenshots, attached to the change once each and published as `` at HQ's address; a picture not kept for that change is refused before anything is written, and one HQ will never take writes nothing and drops the words (HQ refusing for now keeps them). A wired pair's push credential is brought to the enrollment's before a delivery or a git-push, and a credential refusal marks the pair, which heals once a fresh session authenticates. zcp `TestDescribeChange`, `TestDescribeChange_KeptWordsMeetThePushAfterThem`, `TestDescribeChange_Refusals`, `TestWorkflowTool_DescribeChangeReachesItsHandler`, `TestDescribeChange_Pictures`, `TestKeepBrowserPicture`, `TestADeliveryBringsTheCredentialToTheCurrentOne`. | | MB-35 | A new Mate stands up from its application's AI Agent tier, read from HQ, in one call (D32): every pair built from the application's repositories in HQ is read by zcp's naming, not its setups' names, adopted as the adopt route records it with the tier's setups, checked out onto the Mate's branch from the repository the tier names (never one HQ would have to make), and deployed every dev half at once, the first call answering once they stand with the stages queued, and on the second call each stage from its dev half once what its build reads stands (with no reads, every stage above it by priority); a stage never called for stays `READY_TO_DEPLOY` — with nothing committed, pushed or proposed; the runtimes the browser is still importing are waited for, bounded; a refusal before anything is touched names the adopt route, a pair that fails stops alone with the model's next step and holds only the halves whose builds read it, which say what they waited for, and a second call skips what is done. Registered in a Mate only, and a Mate's AGENTS.md sends "Stand up development of the project." to it first. zcp `TestStandup_StandsUpEveryPairFromTheRecipe`, `TestStandupAfter_EveryDevHalfStartsAtOnce`, `TestStandupReads_FromTheRecipe`, `TestStandupReads_EveryRouteTheWriterCounts`, `TestStandup_ReturnsOnceDevelopmentIsUp`, `TestStandup_ASecondCallContinuesAndSkipsWhatIsDone`, `TestStandup_TheModelIsTheBackup`, `TestStandup_WaitsForTheRuntimesTheBrowserIsImporting`, `TestParseMateTier`, `TestParseMateTier_PairsOnlyRepositoriesOfThisApplication`, `TestParseRecipeImportShape_RolesFollowTheHostnameConvention`, `TestAdoptPair_RecordsWhatTheAdoptRouteRecords`, `TestServer_StandupToolGating`, `TestBuildAgentsMD_Container_StandUpRoutesToTheTool`; integration `TestStandup_OverMCP_AMateInNoApplicationIsRefusedAndNothingIsTouched`. | | MB-36 | A kept Mate session is presented only after the descriptor names the expected project and its environment and the Mate confirms it with every scope the client asks for; one the Mate ended or one short of a scope is forgotten and a throwaway opens a new one; no word keeps it and mints; it never waits on the mint pace; the account's close, a refused stored login and a displaced session each end it at its Mate (D33). `keptSessions.test.ts` (client runtime and web), `identityExchange.test.ts`, `exchangeDriver.test.ts`, `signIn.guards.test.tsx`. | | MB-37 | A group recipe writes each search engine at no less than 2 GB `minRam` and 0.5 GB `minFreeRamGB` on every tier and never lowers a value; an unread scale writes no block; `profileOverrides` and the free-memory buffer are carried; a scaling proposal is the Mate's change in the recipe repository in HQ, written over `main`, rewriting only that host's block in every tier naming it; it refuses a block it cannot splice safely, moves forward on the same change for the same host, brings a stale one to `main`'s tree when nothing differs, and is refused while the Mate's additive proposal is open there, which in turn leaves an open scaling proposal alone; the steer speaks only after a change that finished (D34). `group_floors_test.go`, `host_scaling_test.go`, `group_profile_overrides_test.go`, `scale_test.go`; zcp `TestGroupRecipeScaling_SteersAndProposesOneHostsBlock`, `TestGroupRecipeScaling_ProposesFromMainsHeadEveryTierFresh`, `TestGroupRecipeScaling_OneOpenChangeInTheRecipeRepository`, `TestScratch_File`. | diff --git a/internal/knowledge/themes/core.md b/internal/knowledge/themes/core.md index 6806914b0..92fa6cb14 100644 --- a/internal/knowledge/themes/core.md +++ b/internal/knowledge/themes/core.md @@ -49,22 +49,22 @@ services[]: # REQUIRED ``` ### Preprocessor Functions -Enable with `#zeropsPreprocessor=on` as first line. Syntax: `<@function()>`, chain modifiers with `|`: `<@generateRandomString(<32>)|sha256>`. +Enable with `#zeropsPreprocessor=on` as first line. Syntax: `<@function()>`, chain modifiers with `|`: `<@generateRandomString(<32>)|sha256>`. Write a space after each comma between arguments (`<@f(, )>`): without it the preprocessor fails. **Functions:** - `<@generateRandomString()>` -- random alphanumeric string - `<@generateRandomBytes()>` -- random bytes (binary) -- `<@generateRandomInt(,)>` -- random integer in range -- `<@pickRandom(,,...)>` -- pick random from options -- `<@setVar(,)>` / `<@getVar()>` -- store and retrieve variables -- `<@generateRandomStringVar(,)>` -- generate + store string variable -- `<@generateJWT(,)>` -- JWT token generation -- `<@getDateTime(,[])>` -- formatted datetime +- `<@generateRandomInt(, )>` -- random integer in range +- `<@pickRandom(, , ...)>` -- pick random from options +- `<@setVar(, )>` / `<@getVar()>` -- store and retrieve variables +- `<@generateRandomStringVar(, )>` -- generate + store string variable +- `<@generateJWT(, )>` -- JWT token generation +- `<@getDateTime(, [])>` -- formatted datetime - `<@generateED25519Key()>`, `<@generateRSA2048Key()>`, `<@generateRSA4096Key()>` -- key pairs (stores pubKey/privKey) **Modifiers** (applied with `|`): `sha256`, `sha512`, `bcrypt`, `argon2id` (hashing) | `toHex`, `toString` (encoding) | `upper`, `lower`, `title` (case) | `noop` (testing) -**Rules:** Functions return strings. Two-phase processing: preprocessing then YAML parsing. Values generated once at import -- fixed after, not regenerated. Escape special characters: `\<`, `\>`, `\|` (double-escape `\\` for backslash) +**Rules:** Functions return strings. Two-phase processing: preprocessing then YAML parsing. Values generated once at import -- fixed after, not regenerated. Escape special characters where the preprocessor reads them — an import YAML with `#zeropsPreprocessor=on`, and a `zerops_env` value holding a `<@…>` expression: `\<`, `\>`, `\|` (double-escape `\\` for backslash). A `zerops_env` value without `<@` — and so a `project.envVariables` value in `zerops_import` — is stored exactly as written: no escaping there. **Always-available** `${...}` functions: `${random(length)}`, `${randomInt(min,max)}`, `${sha256(value)}`, `${bcrypt(value,rounds)}`, `${argon2id(value)}`, `${jwt(algo,secret,payload)}`, `${generateRSAKeyPair(bits)}`, `${generateEd25519KeyPair()}` diff --git a/internal/ops/browser.go b/internal/ops/browser.go index e5a8c35d1..112b594bd 100644 --- a/internal/ops/browser.go +++ b/internal/ops/browser.go @@ -155,6 +155,9 @@ type BrowserScreenshotResult struct { // ("shot-3") — how a change's description shows it, as // ![what it shows](shot-3). Set by the tools layer, which keeps it. Picture string `json:"picture,omitempty"` + // KeptFor is how long the picture is kept after it is taken ("7 days"), + // so the Mate knows how long it has to show it. Set with Picture. + KeptFor string `json:"keptFor,omitempty"` } // NetworkRequest is one entry from BrowserBatchResult.NetworkOutput. diff --git a/internal/ops/env.go b/internal/ops/env.go index 71afe85fc..7f2ef1653 100644 --- a/internal/ops/env.go +++ b/internal/ops/env.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "strconv" "strings" "github.com/zeropsio/zcp/internal/platform" @@ -212,8 +213,9 @@ func EnvSet( // time. Gives recipe-creation workflows a single source of truth for // shared-secret values: the workspace setup and the published deliverable // run the exact same expression, so a bug caught at workspace time - // can't reappear at deploy time. Keys are batched into one parse so - // setVar/getVar correlate across variables in a single call. + // can't reappear at deploy time. Only the values holding <@…> are + // expanded, together in the order given; every other value is stored + // as given. if err := expandPairs(ctx, pairs); err != nil { return nil, err } @@ -543,29 +545,60 @@ func rejectEncodingPrefixedSecrets(pairs []envPair, originalVariables []string) return nil } -// expandPairs runs each pair's value through the zParser-backed preprocess -// wrapper in one batch, so setVar/getVar correlations work across variables -// in the same call. Pairs with no preprocessor syntax pass through untouched. -// Batching means one shared variable store and one parse — cheaper, and -// matches how the platform's own preprocessor handles multi-key imports. +// expandPairs expands the pairs whose value holds a preprocessor expression +// (<@…>) through zParser together, in the order given, so setVar/getVar and +// a key pair's other half (generateRSA2048Key's Private) span entries, +// as in the platform's own import. Every other value is left byte for byte +// as given — a password with "<" or a snippet of HTML is no expression, and +// zParser would eat its brackets or the batch's delimiter with them. A +// failure names the entry by its key, never by a value. func expandPairs(ctx context.Context, pairs []envPair) error { - keys := make([]string, len(pairs)) - inputs := make(map[string]string, len(pairs)) + var keys []string + inputs := map[string]string{} + at := map[string]int{} for i, p := range pairs { - // Use the index as the batch key — pair keys may repeat (shouldn't, - // but we don't want to silently drop duplicates at this layer). - batchKey := fmt.Sprintf("%d", i) - keys[i] = batchKey + if !strings.Contains(p.Value, "<@") { + continue + } + // The index is the batch key — pair keys may repeat. + batchKey := strconv.Itoa(i) + keys = append(keys, batchKey) inputs[batchKey] = p.Value + at[batchKey] = i + } + if len(keys) == 0 { + return nil } expanded, err := preprocess.Batch(ctx, keys, inputs) if err != nil { + named := keys + if failed := failingExpression(ctx, keys, inputs); failed != "" { + named = []string{failed} + } + names := make([]string, len(named)) + for i, k := range named { + names[i] = pairs[at[k]].Key + } + name := strings.Join(names, ", ") return platform.NewPlatformError(platform.ErrInvalidParameter, - fmt.Sprintf("preprocessor expansion failed: %v", err), - "Check your <@...> syntax, or omit it for literal values") + fmt.Sprintf("preprocessor expansion of %s failed: %v", name, err), + "Check the <@...> syntax in "+name+", or omit it for a literal value") } - for i := range pairs { - pairs[i].Value = expanded[keys[i]] + for _, k := range keys { + pairs[at[k]].Value = expanded[k] } return nil } + +// failingExpression is the batch key of the first expression a batch fails +// at — the shortest failing prefix of keys, in order, since a later entry +// may read what an earlier one set — or "" when no prefix fails alone. Run +// only once the whole batch failed. +func failingExpression(ctx context.Context, keys []string, inputs map[string]string) string { + for n := 1; n <= len(keys); n++ { + if _, err := preprocess.Batch(ctx, keys[:n], inputs); err != nil { + return keys[n-1] + } + } + return "" +} diff --git a/internal/ops/env_generate.go b/internal/ops/env_generate.go index c23f055e5..a6057d515 100644 --- a/internal/ops/env_generate.go +++ b/internal/ops/env_generate.go @@ -152,9 +152,6 @@ type refExpander struct { // messaging (0 means full success), // - infrastructure / cycle errors that abort the whole operation. func (r *refExpander) expandRefs(ctx context.Context, value, sourceService string, visited map[string]bool, depth int) (string, int, error) { - if depth > maxRefExpansionDepth { - return "", 0, fmt.Errorf("ref expansion depth exceeded (>%d) at %q", maxRefExpansionDepth, value) - } matches := FindEnvRefs(value) if len(matches) == 0 { return value, 0, nil @@ -262,6 +259,11 @@ func (r *refExpander) expandRefs(ctx context.Context, value, sourceService strin continue } + // Named by the reference, never by its value: a value half + // expanded holds the secrets resolved into it. + if depth+1 > maxRefExpansionDepth { + return "", 0, fmt.Errorf("ref expansion depth exceeded (>%d) at %s", maxRefExpansionDepth, key) + } nextVisited := make(map[string]bool, len(visited)+1) for k := range visited { nextVisited[k] = true diff --git a/internal/ops/env_generate_test.go b/internal/ops/env_generate_test.go index 365094bc0..ed3d500e5 100644 --- a/internal/ops/env_generate_test.go +++ b/internal/ops/env_generate_test.go @@ -3,6 +3,7 @@ package ops import ( "context" "errors" + "fmt" "os" "path/filepath" "strings" @@ -1367,3 +1368,48 @@ func TestGenerateDotenv_ConcurrentInvocations_Serialize(t *testing.T) { t.Errorf(".env should contain expected value after concurrent writes; got:\n%s", string(body)) } } + +// TestExpandRefs_DepthErrorNamesTheRef: a chain of references too deep to +// follow is named by the reference it stopped at, never by a value — a +// value half expanded holds the secrets resolved into it. +func TestExpandRefs_DepthErrorNamesTheRef(t *testing.T) { + t.Parallel() + secret := "s3cr3t-" + "value-5aa1" + tests := []struct { + name string + links int // how many vars the chain runs through + wantErr bool + }{ + {name: "a chain within the bound", links: maxRefExpansionDepth - 1}, + {name: "a chain past the bound", links: maxRefExpansionDepth + 4, wantErr: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + db := platform.ServiceStack{ID: "svc-db", Name: "db"} + envs := make([]platform.ServiceEnvVar, 0, tt.links+1) + for i := range tt.links { + envs = append(envs, platform.ServiceEnvVar{Key: fmt.Sprintf("v%d", i), Content: fmt.Sprintf("%s-%d ${v%d}", secret, i, i+1)}) + } + envs = append(envs, platform.ServiceEnvVar{Key: fmt.Sprintf("v%d", tt.links), Content: "end"}) + r := &refExpander{ + classifier: NewEnvRefClassifier([]platform.ServiceStack{db}), + serviceIndex: map[string]platform.ServiceStack{"db": db}, + cache: map[string][]platform.ServiceEnvVar{"db": envs}, + } + _, _, err := r.expandRefs(context.Background(), "${db_v0}", "", map[string]bool{}, 0) + if (err != nil) != tt.wantErr { + t.Fatalf("err = %v, want error %v", err, tt.wantErr) + } + if err == nil { + return + } + if strings.Contains(err.Error(), secret) { + t.Errorf("the error repeats a value: %v", err) + } + if !strings.Contains(err.Error(), "db.v") { + t.Errorf("the error does not name the reference it stopped at: %v", err) + } + }) + } +} diff --git a/internal/ops/env_test.go b/internal/ops/env_test.go index 47aadf702..4f5b68acd 100644 --- a/internal/ops/env_test.go +++ b/internal/ops/env_test.go @@ -3,6 +3,8 @@ package ops import ( "context" + "crypto/sha256" + "encoding/hex" "errors" "fmt" "strings" @@ -403,6 +405,152 @@ func TestEnvSet_Project_PreprocessorSyntaxError(t *testing.T) { } } +// TestEnvSet_Project_PlainValuesStoredAsGiven: only a value holding a +// preprocessor expression (<@…>) is expanded, each on its own; every other +// value is stored byte for byte — a password with "<" or a snippet of HTML +// is no expression, alone or next to one. +func TestEnvSet_Project_PlainValuesStoredAsGiven(t *testing.T) { + t.Parallel() + tests := []struct { + name string + input []string + plain map[string]string // key → the value it must be stored as + }{ + {name: "a password with <", input: []string{"DB_PASSWORD=Xy<9z"}, plain: map[string]string{"DB_PASSWORD": "Xy<9z"}}, + {name: "html", input: []string{"BANNER="}, plain: map[string]string{"BANNER": ""}}, + {name: "a > and a <", input: []string{"CMP=a>bb", input: []string{"HASHED="}, plain: map[string]string{"HASHED": ""}}, + { + name: "plain values next to an expression", + input: []string{"DB_PASSWORD=Xy<9z", "APP_KEY=<@generateRandomString(<16>)>", "BANNER="}, + plain: map[string]string{"DB_PASSWORD": "Xy<9z", "BANNER": ""}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + mock := &countingProjectEnvMock{Client: platform.NewMock()} + if _, err := EnvSet(context.Background(), mock, "proj-1", "", true, tt.input); err != nil { + t.Fatalf("EnvSet: %v", err) + } + if len(mock.calls) != len(tt.input) { + t.Fatalf("want %d calls, got %d", len(tt.input), len(mock.calls)) + } + for _, call := range mock.calls { + if want, isPlain := tt.plain[call.Key]; isPlain && call.Value != want { + t.Errorf("%s stored as %q, want %q", call.Key, call.Value, want) + } + if call.Key == "APP_KEY" && len(call.Value) != 16 { + t.Errorf("APP_KEY = %q, want 16 expanded characters", call.Value) + } + } + }) + } +} + +// TestEnvSet_Project_ExpressionsShareOneStore: the values holding <@…> +// expand together, in the order given, so a key pair's other half and a +// variable set by one entry reach the next — while a plain value between +// them is stored as given. +func TestEnvSet_Project_ExpressionsShareOneStore(t *testing.T) { + t.Parallel() + tests := []struct { + name string + input []string + check func(t *testing.T, got map[string]string) + }{ + { + name: "a key pair", + input: []string{"JWT_PUB=<@generateRSA2048Key()>", "BANNER=", "JWT_PRIV=<@getVar(jwtPrivate)>"}, + check: func(t *testing.T, got map[string]string) { + t.Helper() + if !strings.Contains(got["JWT_PUB"], "PUBLIC KEY") || !strings.Contains(got["JWT_PRIV"], "PRIVATE KEY") { + t.Errorf("want both halves of the pair, got %q / %q", got["JWT_PUB"], got["JWT_PRIV"]) + } + if got["BANNER"] != "" { + t.Errorf("BANNER = %q, want ", got["BANNER"]) + } + }, + }, + { + name: "a variable chain", + input: []string{"TOKEN=<@generateRandomStringVar(, <24>)>", "PW=Xy<9z", "TOKEN_HASH=<@getVar(tok)|sha256>"}, + check: func(t *testing.T, got map[string]string) { + t.Helper() + sum := sha256.Sum256([]byte(got["TOKEN"])) + if len(got["TOKEN"]) != 24 || got["TOKEN_HASH"] != hex.EncodeToString(sum[:]) { + t.Errorf("TOKEN_HASH = %q, want the sha256 of TOKEN %q", got["TOKEN_HASH"], got["TOKEN"]) + } + if got["PW"] != "Xy<9z" { + t.Errorf("PW = %q, want Xy<9z", got["PW"]) + } + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + mock := &countingProjectEnvMock{Client: platform.NewMock()} + if _, err := EnvSet(context.Background(), mock, "proj-1", "", true, tt.input); err != nil { + t.Fatalf("EnvSet: %v", err) + } + got := map[string]string{} + for _, c := range mock.calls { + got[c.Key] = c.Value + } + tt.check(t, got) + }) + } +} + +// TestEnvSet_PreprocessorErrorQuotesNoValue: a failing expression is named by +// its key with zParser's reason, never by what it holds — neither its own +// value nor another entry's reaches the error the agent and the UI show. +func TestEnvSet_PreprocessorErrorQuotesNoValue(t *testing.T) { + t.Parallel() + secret := "s3cr3t-" + "value-71be0a" + tests := []struct { + name string + input []string + wantKey string + }{ + {name: "another entry's secret", input: []string{"API_SECRET=" + secret, "APP_KEY=<@nosuchfn(<1>)>"}, wantKey: "APP_KEY"}, + {name: "a secret in the failing entry", input: []string{"TOKEN=" + secret + "<@nosuchfn(<1>)>"}, wantKey: "TOKEN"}, + {name: "a secret with < before a failing entry", input: []string{"API_SECRET=" + secret + ")>"}, wantKey: "ZCP_BROKEN_EXPR"}, + { + name: "an expression's secret before the failing one", + input: []string{"A_SET=<@setVar(, <" + secret + ">)>", "B_BROKEN_EXPR=<@getVar(k)|nosuchfn>"}, + wantKey: "B_BROKEN_EXPR", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + mock := &countingProjectEnvMock{Client: platform.NewMock()} + _, err := EnvSet(context.Background(), mock, "proj-1", "", true, tt.input) + pe, ok := err.(*platform.PlatformError) + if !ok { + t.Fatalf("expected *PlatformError, got %T: %v", err, err) + } + for _, said := range []string{pe.Error(), pe.Message, pe.Suggestion, pe.Diagnostic} { + if strings.Contains(said, secret) { + t.Errorf("the error repeats a value: %q", said) + } + } + if !strings.Contains(pe.Message, "expansion of "+tt.wantKey+" failed") || !strings.Contains(pe.Message, "nosuchfn") { + t.Errorf("message = %q, want it to name %s and zParser's reason", pe.Message, tt.wantKey) + } + if len(mock.calls) != 0 { + t.Errorf("a failed expansion stored %d values", len(mock.calls)) + } + }) + } +} + func TestEnvSet_Project_PartialFailure(t *testing.T) { t.Parallel() diff --git a/internal/ops/helpers.go b/internal/ops/helpers.go index c9afcfc50..4ba5d6dd0 100644 --- a/internal/ops/helpers.go +++ b/internal/ops/helpers.go @@ -152,12 +152,14 @@ type envPair struct { // Splits on first '=' only (value may contain '='). func parseEnvPairs(vars []string) ([]envPair, error) { pairs := make([]envPair, 0, len(vars)) - for _, v := range vars { + for i, v := range vars { key, value, ok := strings.Cut(v, "=") if !ok { + // Named by its place, never by what it holds: an entry with no + // '=' is often a secret pasted alone. return nil, platform.NewPlatformError( platform.ErrInvalidEnvFormat, - fmt.Sprintf("Invalid format '%s', expected KEY=value", v), + fmt.Sprintf("Invalid format in entry %d, expected KEY=value", i+1), "Format: KEY=value (split on first '=')", ) } diff --git a/internal/ops/helpers_test.go b/internal/ops/helpers_test.go index 9dd3cc43b..35e3a4db0 100644 --- a/internal/ops/helpers_test.go +++ b/internal/ops/helpers_test.go @@ -196,6 +196,40 @@ func TestParseSince(t *testing.T) { } } +// TestParseEnvPairs_RefusalNamesNoValue: a malformed entry is named by its +// place, never by what it holds — an entry without "=" is often a secret +// pasted alone, and the refusal reaches the agent and the person's screen. +func TestParseEnvPairs_RefusalNamesNoValue(t *testing.T) { + t.Parallel() + secret := "s3cr3t-" + "value-9f2c41" + tests := []struct { + name string + input []string + wantEntry string + }{ + {name: "a secret pasted alone", input: []string{secret}, wantEntry: "entry 1"}, + {name: "after good entries", input: []string{"A=1", "B=2", "TOKEN " + secret}, wantEntry: "entry 3"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + _, err := parseEnvPairs(tt.input) + pe, ok := err.(*platform.PlatformError) + if !ok { + t.Fatalf("expected *PlatformError, got %T: %v", err, err) + } + for _, said := range []string{pe.Error(), pe.Message, pe.Suggestion, pe.Diagnostic} { + if strings.Contains(said, secret) { + t.Errorf("the refusal repeats the value: %q", said) + } + } + if !strings.Contains(pe.Message, tt.wantEntry) { + t.Errorf("message = %q, want it to name %s", pe.Message, tt.wantEntry) + } + }) + } +} + func TestParseEnvPairs(t *testing.T) { t.Parallel() diff --git a/internal/ops/import_test.go b/internal/ops/import_test.go index 65d35bcc8..e50ee43e0 100644 --- a/internal/ops/import_test.go +++ b/internal/ops/import_test.go @@ -119,6 +119,42 @@ services: // project-env channel (EnvSet) before service creation, eliminating the // historical 2-step "set project envs, then import services" workflow. // See plans/multi-runtime-audit-followup.md §5 Fix B1. +// TestImport_ProjectEnvVariables_ExpressionsShareOneStore: project +// envVariables go through zerops_env set's path, in key order, so a variable +// one sets reaches the next, and a key pair's other half is there. +func TestImport_ProjectEnvVariables_ExpressionsShareOneStore(t *testing.T) { + t.Parallel() + mock := importMock() + content := `project: + envVariables: + A_JWT_PUB: <@generateRSA2048Key()> + B_JWT_PRIV: <@getVar(jwtPrivate)> + C_TOKEN: <@generateRandomStringVar(, <24>)> + D_TOKEN_UPPER: <@getVar(tok)|upper> + E_PLAIN: Xy<9z +services: + - hostname: api + type: nodejs@22 + mode: NON_HA +` + if _, err := Import(context.Background(), mock, "proj-1", content, "", false); err != nil { + t.Fatalf("Import: %v", err) + } + got := map[string]string{} + for _, c := range mock.CapturedProjectEnvCreations { + got[c.Key] = c.Content + } + if !strings.Contains(got["A_JWT_PUB"], "PUBLIC KEY") || !strings.Contains(got["B_JWT_PRIV"], "PRIVATE KEY") { + t.Errorf("want both halves of the key pair, got %q / %q", got["A_JWT_PUB"], got["B_JWT_PRIV"]) + } + if got["D_TOKEN_UPPER"] != strings.ToUpper(got["C_TOKEN"]) || len(got["C_TOKEN"]) != 24 { + t.Errorf("D_TOKEN_UPPER = %q, want C_TOKEN %q in upper case", got["D_TOKEN_UPPER"], got["C_TOKEN"]) + } + if got["E_PLAIN"] != "Xy<9z" { + t.Errorf("E_PLAIN = %q, want Xy<9z", got["E_PLAIN"]) + } +} + func TestImport_ProjectEnvVariablesOnly_AppliedInline(t *testing.T) { t.Parallel() diff --git a/internal/preprocess/preprocess.go b/internal/preprocess/preprocess.go index 4b83cde02..28da76484 100644 --- a/internal/preprocess/preprocess.go +++ b/internal/preprocess/preprocess.go @@ -131,26 +131,41 @@ func expand(ctx context.Context, input string, _ map[string]string) (string, err parser.WithMaxFunctionCount(MaxFunctionCount), parser.WithMultilineOutputHandling(parser.MultilinePreserved), ) - if err := p.Parse(ctx); err != nil { - return "", wrapParseError(input, err) + if err := parse(ctx, p); err != nil { + return "", wrapParseError(err) } return out.String(), nil } +// errParserPanic is zParser failing by a panic rather than an error. v2.1.2 +// panics on a call whose next argument follows its comma directly — +// <@generateRandomInt(<10>,<50>)> — where <@generateRandomInt(<10>, <50>)> +// expands. The form is not rewritten: the platform's import runs the same +// zParser, so a value zcp accepted could still fail there. +var errParserPanic = errors.New("the preprocessor could not read this expression; write a space after each comma between arguments, as in <@generateRandomInt(<10>, <50>)>") + +// parse runs p, answering a panic inside zParser as errParserPanic: one bad +// value must never take the whole zcp process — and the agent's MCP server — +// down with it. The panic's own words stay out: they say nothing the agent +// can act on. +func parse(ctx context.Context, p *parser.Parser) (err error) { + defer func() { + if recover() != nil { + err = errParserPanic + } + }() + return p.Parse(ctx) +} + // wrapParseError converts zParser's errors into something the MCP layer can -// surface cleanly. The underlying metaError carries line/char info that is -// noisy for single-value expansion — we include a short context excerpt -// from the input instead. -func wrapParseError(input string, err error) error { - excerpt := input - const maxExcerpt = 80 - if len(excerpt) > maxExcerpt { - excerpt = excerpt[:maxExcerpt] + "..." - } +// surface cleanly: zParser's reason, never the input — a value is often a +// secret, and the error reaches the agent and the person's screen. The +// caller names the entry that failed. +func wrapParseError(err error) error { // Deadline exceeded surfaces as a wrapped context error — preserve that // so callers can distinguish timeout from syntax error. if errors.Is(err, context.DeadlineExceeded) { - return fmt.Errorf("preprocess %q: timeout (%s)", excerpt, DefaultTimeout) + return fmt.Errorf("preprocess: timeout (%s)", DefaultTimeout) } - return fmt.Errorf("preprocess %q: %w", excerpt, err) + return fmt.Errorf("preprocess: %w", err) } diff --git a/internal/preprocess/preprocess_test.go b/internal/preprocess/preprocess_test.go index f50a9b8af..998981176 100644 --- a/internal/preprocess/preprocess_test.go +++ b/internal/preprocess/preprocess_test.go @@ -151,6 +151,61 @@ func TestExpand_UnknownFunction(t *testing.T) { } } +// TestExpand_ErrorQuotesNoInput: an expansion's error carries zParser's +// reason, never the input — a value is often a secret. +func TestExpand_ErrorQuotesNoInput(t *testing.T) { + t.Parallel() + secret := "s3cr3t-" + "value-0c9d" + tests := []string{ + secret + "<@nosuchfn(<1>)>", + "<@nosuchfn(<" + secret + ">)>", + } + for _, in := range tests { + _, err := Expand(context.Background(), in) + if err == nil { + t.Fatalf("Expand(%q): want an error", in) + } + if strings.Contains(err.Error(), secret) || !strings.Contains(err.Error(), "nosuchfn") { + t.Errorf("Expand error = %q, want zParser's reason without the input", err) + } + } + _, err := Batch(context.Background(), []string{"A", "B"}, map[string]string{"A": secret, "B": "<@nosuchfn(<1>)>"}) + if err == nil || strings.Contains(err.Error(), secret) { + t.Errorf("Batch error = %v, want one without another entry's value", err) + } +} + +// TestExpand_CommaWithoutSpace: zParser v2.1.2 panics on a call whose +// argument follows its comma directly — <@generateRandomInt(<10>,<50>)>. +// The panic is caught and answered as an error that says how to write it, +// never the input; the process lives. With the space, the call expands. +func TestExpand_CommaWithoutSpace(t *testing.T) { + t.Parallel() + tests := []struct { + in string + wantErr bool + }{ + {in: "<@generateRandomInt(<10>,<50>)>", wantErr: true}, + {in: "<@setVar(,)><@getVar(k)>", wantErr: true}, + {in: "<@pickRandom(,\\)>", wantErr: true}, + {in: "<@generateRandomInt(<10>, <50>)>"}, + {in: "<@setVar(, )><@getVar(k)>"}, + } + for _, tt := range tests { + got, err := Expand(context.Background(), tt.in) + if (err != nil) != tt.wantErr { + t.Errorf("Expand(%q) = %q, %v; want error %v", tt.in, got, err, tt.wantErr) + continue + } + if err != nil && (!strings.Contains(err.Error(), "space after") || strings.Contains(err.Error(), "v-91f2")) { + t.Errorf("Expand(%q) error = %q, want how to write it and no input", tt.in, err) + } + } + if _, err := Batch(context.Background(), []string{"A"}, map[string]string{"A": "<@generateRandomInt(<1>,<2>)>"}); err == nil { + t.Error("Batch: want an error for the comma without a space") + } +} + func TestBatch_SimpleKeys(t *testing.T) { t.Parallel() keys := []string{"A", "B", "C"} diff --git a/internal/tools/browser.go b/internal/tools/browser.go index ec21087a2..31cfbe198 100644 --- a/internal/tools/browser.go +++ b/internal/tools/browser.go @@ -69,7 +69,7 @@ func RegisterBrowser(srv *mcp.Server, stateDir string) { "Do NOT use [\"eval\",...] — it is stripped; dedicated commands produce structured output. " + "Pass screenshot=true to capture a screenshot after your commands run — returned as " + "an image content block alongside the text result, not inlined into it, and kept as a picture the " + - "result names (screenshot.picture, e.g. \"shot-3\"): write ![what it shows](shot-3) in a " + + "result names (screenshot.picture, e.g. \"shot-3\") for as long as screenshot.keptFor says: write ![what it shows](shot-3) in a " + "zerops_workflow action=\"describe-change\" description to show it to the person. " + "Returns: steps[] (each with errorKind on failure), errorsOutput (from [errors]), " + "consoleOutput (from [console]), networkOutput (4xx/5xx requests from [network requests], " + @@ -113,6 +113,7 @@ func keepBrowserPicture(stateDir string, result *ops.BrowserBatchResult) { return } shot.Picture = pic.ID + shot.KeptFor = pictureKeepWords() } // browserToolResult builds the zerops_browser CallToolResult: the JSON diff --git a/internal/tools/browser_test.go b/internal/tools/browser_test.go index 29820a6f3..c565d85d3 100644 --- a/internal/tools/browser_test.go +++ b/internal/tools/browser_test.go @@ -102,11 +102,15 @@ func TestKeepBrowserPicture(t *testing.T) { var shown struct { Screenshot struct { Picture string `json:"picture"` + KeptFor string `json:"keptFor"` } `json:"screenshot"` } if err := json.Unmarshal([]byte(resultText(t, browserToolResult(result))), &shown); err != nil || shown.Screenshot.Picture != "shot-1" { t.Errorf("the result must name the picture: %+v (%v)", shown, err) } + if shown.Screenshot.KeptFor != "up to 7 days" { + t.Errorf("keptFor = %q, want the result to say how long the picture is kept (up to 7 days)", shown.Screenshot.KeptFor) + } if _, png, err := workflow.KeptPicture(stateDir, "shot-1"); err != nil || !bytes.Equal(png, []byte("\x89PNG-browser")) { t.Errorf("the picture is not kept: %v", err) } diff --git a/internal/tools/env.go b/internal/tools/env.go index 46d49a352..2e0b790b3 100644 --- a/internal/tools/env.go +++ b/internal/tools/env.go @@ -194,7 +194,7 @@ type envChangeResult struct { func RegisterEnv(srv *mcp.Server, client platform.Client, projectID, selfHostname string) { mcp.AddTool(srv, &mcp.Tool{ Name: "zerops_env", - Description: "Manage env vars. Actions: get (read), set (upsert), delete, generate-dotenv (write local .env from local zerops.yaml). Scope: service via serviceHostname, or project=true. set values expand <@...> via zParser; encoding prefixes (base64:, hex:) are rejected. Response 'stored' verifies what landed. set/delete auto-restart affected services unless skipRestart=true. For bulk env reads across many services, prefer zerops_discover includeEnvs=true.", + Description: "Manage env vars. Actions: get (read), set (upsert), delete, generate-dotenv (write local .env from local zerops.yaml). Scope: service via serviceHostname, or project=true. set expands <@...> values (shared setVar/getVar; |modifiers only there); others stay verbatim, unescaped. base64:/hex: prefixes rejected. Response 'stored' verifies what landed. set/delete auto-restart affected services unless skipRestart=true. For bulk env reads across many services, prefer zerops_discover includeEnvs=true.", InputSchema: envInputSchema(), Annotations: &mcp.ToolAnnotations{ Title: "Manage environment variables", @@ -428,26 +428,21 @@ func detectSetShadows(ctx context.Context, client platform.Client, projectID str } // formatLayeredShadow renders a single cross-layer shadow as agent-actionable -// guidance. The winning value is redacted via the single masking owner -// (RedactCredentialValue) when its key is a ZCP-owned credential — never on -// the platform Sensitive flag, which is not authoritative. The winning layer -// is always a runtime's yaml/service value (managed services are excluded -// from project-set shadow detection), so the "" serviceType is correct: only -// the ZCP-owned class can mask here. The fix differs by winning layer: +// guidance: the key and the two places — the project, and the yaml or the +// service that wins — never the winning value. zerops_env get returns keys, +// not values, and a warning reaches the agent and the person's screen the +// same way; the agent reads the value with zerops_discover +// includeEnvValues=true when it needs it. The fix differs by winning layer: // yaml-baked is edit-yaml-and-redeploy (the key is owned by the yaml, spec // §2); service userData is change-or-delete the service var. func formatLayeredShadow(s ops.LayeredShadow) string { - val := s.WinningValue - if masked, isCredential := ops.RedactCredentialValue(s.Key, s.WinningValue, ""); isCredential { - val = masked - } switch s.WinningLayer { case ops.EnvLayerYamlBaked: - return fmt.Sprintf("%q set at project scope is shadowed on %s: its zerops.yaml run.envVariables bakes %s=%s (yaml owns the key — spec §2). %s reads the yaml value, not the project one. Edit %s's zerops.yaml and redeploy to change it there.", - s.Key, s.Hostname, s.Key, val, s.Hostname, s.Hostname) + return fmt.Sprintf("%q set at project scope is shadowed on %s: its zerops.yaml run.envVariables bakes %s (yaml owns the key — spec §2). %s reads the yaml value, not the project one. Edit %s's zerops.yaml and redeploy to change it there.", + s.Key, s.Hostname, s.Key, s.Hostname, s.Hostname) case ops.EnvLayerService: - return fmt.Sprintf("%q set at project scope is shadowed on %s: a service-level env sets %s=%s (service > project — spec §2). %s reads the service value. Change or delete the service-level %s on %s.", - s.Key, s.Hostname, s.Key, val, s.Hostname, s.Key, s.Hostname) + return fmt.Sprintf("%q set at project scope is shadowed on %s: a service-level env sets %s (service > project — spec §2). %s reads the service value. Change or delete the service-level %s on %s.", + s.Key, s.Hostname, s.Key, s.Hostname, s.Key, s.Hostname) case ops.EnvLayerProject: // Project is the lowest-precedence layer (spec §2) — it can never be the // WINNING/shadowing layer (DetectLayeredShadows only ever sets diff --git a/internal/tools/env_test.go b/internal/tools/env_test.go index 572cdd663..3e81538ad 100644 --- a/internal/tools/env_test.go +++ b/internal/tools/env_test.go @@ -565,61 +565,70 @@ func TestEnvSet_ServiceScope_NoShadowDetection(t *testing.T) { } } -// TestEnvSet_ProjectScope_ShadowRedaction_IsKeyBased pins the layered-shadow -// message redaction under the key-based masking model (the plan's "never -// Sensitive"): the winning (shadowing) value is redacted iff its KEY is a -// ZCP-owned credential — routed through the single owner RedactCredentialValue -// — NOT when the platform Sensitive flag is set. A generic-keyed value, even -// one the platform classified SECRET, is now SHOWN: the Sensitive flag is not -// authoritative (it does not persist), so only key ownership drives masking. -func TestEnvSet_ProjectScope_ShadowRedaction_IsKeyBased(t *testing.T) { +// TestEnvSet_ProjectScope_ShadowNamesNoValue: a shadow warning names the key +// and the two places — the project, and the yaml or the service that wins — +// never the value that wins, a credential or not: zerops_env get returns keys, +// not values, and a warning reaches the agent and the person's screen the +// same way. +func TestEnvSet_ProjectScope_ShadowNamesNoValue(t *testing.T) { t.Parallel() - mock := shadowSetMock( - // Type:"USER" — the 2026-08 app-version model classifies every - // yaml-baked record Sensitive=false regardless of Type (no SECRET - // value to derive from any more). GIT_TOKEN is a ZCP-owned - // credential key → its winning value masks; API_SECRET is generic - // → its winning value is shown despite looking secret-ish — proof - // the masking never depended on the (now-gone) Sensitive derivation. - []platform.ServiceEnvVar{ - {Key: ops.GitTokenEnvKey, Content: "ghp_BAKED_SECRET", Type: platform.ServiceEnvUser}, - {Key: "API_SECRET", Content: "topsecret-baked", Type: platform.ServiceEnvUser}, + tests := []struct { + name string + baked []platform.ServiceEnvVar + slim []platform.ServiceEnvVar + variables []any + winning []string // values that must not appear + wantPlaces []string + }{ + { + name: "values the yaml bakes", + baked: []platform.ServiceEnvVar{ + {Key: ops.GitTokenEnvKey, Content: "baked-git-" + "token-77", Type: platform.ServiceEnvUser}, + {Key: "API_SECRET", Content: "baked-api-" + "secret-42", Type: platform.ServiceEnvUser}, + }, + variables: []any{"GIT_TOKEN=mytoken", "API_SECRET=myval"}, + winning: []string{"baked-git-token-77", "baked-api-secret-42"}, + wantPlaces: []string{"GIT_TOKEN", "API_SECRET", "zerops.yaml", "project", "api"}, + }, + { + name: "a value the service sets", + slim: []platform.ServiceEnvVar{{Key: "API_SECRET", Content: "service-api-" + "secret-13"}}, + variables: []any{"API_SECRET=myval"}, + winning: []string{"service-api-secret-13"}, + wantPlaces: []string{"API_SECRET", "service-level", "project", "api"}, }, - nil, - ) - srv := mcp.NewServer(&mcp.Implementation{Name: "test", Version: "0.1"}, nil) - RegisterEnv(srv, mock, "proj-1", "") - - result := callTool(t, srv, "zerops_env", map[string]any{ - "action": "set", "project": true, - "variables": []any{"GIT_TOKEN=mytoken", "API_SECRET=myval"}, - }) - if result.IsError { - t.Fatalf("unexpected IsError: %s", getTextContent(t, result)) - } - - text := getTextContent(t, result) - // ZCP-owned credential key → winning value redacted, never echoed. - if strings.Contains(text, "ghp_BAKED_SECRET") { - t.Fatalf("shadowWarning leaked the GIT_TOKEN winning value: %s", text) - } - // JSON marshaling escapes '<' → <, so match the unescaped marker body. - if !strings.Contains(text, "redacted: ZCP-managed credential") { - t.Fatalf("expected the GIT_TOKEN winning value to be redacted: %s", text) - } - // Generic key (even SECRET-classified) → winning value shown, because the - // Sensitive flag no longer drives redaction. - if !strings.Contains(text, "topsecret-baked") { - t.Fatalf("expected the generic API_SECRET winning value to be shown: %s", text) - } - - var parsed map[string]any - if err := json.Unmarshal([]byte(text), &parsed); err != nil { - t.Fatalf("parse result: %v", err) } - warns, _ := parsed["shadowWarnings"].([]any) - if len(warns) < 2 { - t.Fatalf("expected shadowWarnings for both shadowed keys, got: %v", parsed) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + srv := mcp.NewServer(&mcp.Implementation{Name: "test", Version: "0.1"}, nil) + RegisterEnv(srv, shadowSetMock(tt.baked, tt.slim), "proj-1", "") + result := callTool(t, srv, "zerops_env", map[string]any{"action": "set", "project": true, "variables": tt.variables}) + if result.IsError { + t.Fatalf("unexpected IsError: %s", getTextContent(t, result)) + } + var parsed struct { + ShadowWarnings []string `json:"shadowWarnings"` + } + text := getTextContent(t, result) + if err := json.Unmarshal([]byte(text), &parsed); err != nil { + t.Fatalf("parse result: %v", err) + } + if len(parsed.ShadowWarnings) != len(tt.variables) { + t.Fatalf("want a shadow warning per key, got %v", parsed.ShadowWarnings) + } + for _, value := range tt.winning { + if strings.Contains(text, value) { + t.Errorf("the result repeats the winning value %q: %s", value, text) + } + } + all := strings.Join(parsed.ShadowWarnings, "\n") + for _, want := range tt.wantPlaces { + if !strings.Contains(all, want) { + t.Errorf("the warnings miss %q: %s", want, all) + } + } + }) } } diff --git a/internal/tools/hq_change.go b/internal/tools/hq_change.go index 14a8b426e..74af0562e 100644 --- a/internal/tools/hq_change.go +++ b/internal/tools/hq_change.go @@ -38,8 +38,9 @@ type changeRef struct { // Described is true when this call put the description the Mate kept for // its change onto it (hq_change_description.go). Described bool `json:"described,omitempty"` - // DescriptionNote says why the kept description did not go on — its - // pictures could not be attached — and is "" otherwise. + // DescriptionNote is a sentence saying the description the Mate kept for + // the change was dropped, not put on, and why — a picture HQ will never + // take, or one no longer kept — and is "" otherwise. DescriptionNote string `json:"descriptionNote,omitempty"` // Draft is true when this call moved the change — opened it, or pushed // work onto it — and no description describes it yet: HQ asks the person diff --git a/internal/tools/hq_change_description.go b/internal/tools/hq_change_description.go index a319d54f1..887902d4c 100644 --- a/internal/tools/hq_change_description.go +++ b/internal/tools/hq_change_description.go @@ -6,8 +6,11 @@ import ( "fmt" "html" "math" + "net/http" "sort" + "strconv" "strings" + "time" "unicode/utf8" "github.com/modelcontextprotocol/go-sdk/mcp" @@ -57,10 +60,26 @@ type changeDescriptionResult struct { Message string `json:"message"` } -// errCannotAttach is a picture HQ would not keep for a change: nothing is -// written then, never a description with a broken picture. +// errCannotAttach is a picture HQ will never keep for a change — larger than +// it takes, or refused for what it is: nothing is written then, never a +// description with a broken picture, and words that show it are dropped, +// since they could never go on. HQ refusing for now (429, a 5xx) is not one. var errCannotAttach = errors.New("a picture could not be attached") +// picturesGoneError is pictures a description shows that are no longer kept +// for the change it goes onto — pruned since the words were checked, or kept +// only as the address of another change's attachment. Such words can never +// go on: they are refused, never kept. +type picturesGoneError struct{ ids []string } + +func (e *picturesGoneError) Error() string { + verb := "is" + if len(e.ids) > 1 { + verb = "are" + } + return fmt.Sprintf("%s %s no longer kept, so it cannot be shown on this change", strings.Join(e.ids, ", "), verb) +} + // handleDescribeChange is zerops_workflow action="describe-change": the // Mate's description of its change, set as the description of the pair's // open change in HQ, or kept until one is open. @@ -79,7 +98,7 @@ func handleDescribeChange( ), WithRecoveryStatus()), nil, nil } text := strings.TrimSpace(input.Description) - if refusal := describeTextRefusal(stateDir, text); refusal != nil { + if refusal := describeTextRefusal(text); refusal != nil { return convertError(refusal, WithRecoveryStatus()), nil, nil } title := strings.TrimSpace(input.Title) @@ -90,6 +109,9 @@ func handleDescribeChange( if refusal != nil { return refusal, nil, nil } + if refusal := describePicturesRefusal(stateDir, text, pictureTargetOf(meta, meta.HQ.Change)); refusal != nil { + return convertError(refusal, WithRecoveryStatus()), nil, nil + } // The change on record is only as fresh as the last read of the Mate's // state, and words are never put on one that merged or closed in @@ -131,12 +153,22 @@ func handleDescribeChange( url := hqc.ChangeURL(meta.HQ.AppID, meta.HQ.Repo, number) if err := putChangeDescription(ctx, hqc, stateDir, meta, number, text, title); err != nil { + var gone *picturesGoneError + if errors.As(err, &gone) { + forgetChangeDescription(stateDir, meta, text) + return convertError(platform.NewPlatformError( + platform.ErrInvalidParameter, + fmt.Sprintf("Nothing was written onto change #%d: %v. %s", number, err, keptPicturesSentence(stateDir, pictureTargetOf(meta, number))), + pictureRetakeSuggestion, + ), WithRecoveryStatus()), nil, nil + } if errors.Is(err, errCannotAttach) { + forgetChangeDescription(stateDir, meta, text) return convertError(platform.NewPlatformError( platform.ErrPrerequisiteMissing, - fmt.Sprintf("Nothing was written onto change #%d: %v. The description is kept, and goes onto #%d with %s unless that delivery changes its work.", - number, err, number, nextDeliveryOf(meta)), - "Describe the change again without that picture to put the words on it now.", + fmt.Sprintf("Nothing was written onto change #%d: %v. HQ will not take that picture, so the description is dropped: words that show it could never go on.", + number, err), + "Describe the change again without that picture, or with another screenshot of the same thing.", ), WithRecoveryStatus()), nil, nil } if keepErr != nil { @@ -163,9 +195,9 @@ func handleDescribeChange( } // describeTextRefusal refuses words no description can carry: none, longer -// than HQ keeps, a NUL — which no text in HQ keeps — or a picture this Mate -// does not keep. Nil for words that can go on. -func describeTextRefusal(stateDir, text string) *platform.PlatformError { +// than HQ keeps, or a NUL — which no text in HQ keeps. Nil for words that can +// go on; their pictures are checked for the change (describePicturesRefusal). +func describeTextRefusal(text string) *platform.PlatformError { if text == "" { return platform.NewPlatformError( platform.ErrInvalidParameter, @@ -187,17 +219,35 @@ func describeTextRefusal(stateDir, text string) *platform.PlatformError { "Keep it to what the change does and why, and how you checked it — logs and long output belong in the conversation, not in the review.", ) } - if missing := missingPictures(stateDir, text); len(missing) > 0 { - return platform.NewPlatformError( - platform.ErrInvalidParameter, - fmt.Sprintf("The description shows %s, and this Mate keeps no such picture — the newest %d screenshots are kept, and every one a kept description shows.", - strings.Join(missing, ", "), workflow.PictureKeep), - "Take the screenshot again with zerops_browser screenshot=true and use the picture its result names, or leave the picture out.", - ) - } return nil } +// describePicturesRefusal refuses words that show a picture the change target +// (workflow.PictureTarget) cannot show — none kept, or one kept only as the +// address of another change's attachment — or, as itself, a store that cannot +// be read. Nil when every picture can go on. +func describePicturesRefusal(stateDir, text, target string) error { + missing, err := missingPictures(stateDir, text, target) + if err != nil { + return fmt.Errorf("read this Mate's pictures: %w", err) + } + if len(missing) == 0 { + return nil + } + return platform.NewPlatformError( + platform.ErrInvalidParameter, + fmt.Sprintf("The description shows %s, and this Mate keeps no such picture. %s", + strings.Join(missing, ", "), keptPicturesSentence(stateDir, target)), + pictureRetakeSuggestion, + ) +} + +// pictureTargetOf is how the picture store names change number of m's +// repository. +func pictureTargetOf(m *workflow.ServiceMeta, number int) string { + return workflow.PictureTarget(m.HQ.AppID, m.HQ.Repo, number) +} + // describeTitleRefusal refuses a title no change can carry: more than one // line, a NUL, or longer than HQ keeps. Nil for "" — no title given — and for // one that can go on. @@ -334,13 +384,29 @@ func changeBody(ctx context.Context, hqc hq.Client, stateDir string, m *workflow if len(ids) == 0 { return text, nil } - target := fmt.Sprintf("%s/%s#%d", m.HQ.AppID, m.HQ.Repo, number) - tags := make(map[string]func(alt string) string, len(ids)) + target := pictureTargetOf(m, number) + // Every picture is read before any is attached: the words go on whole + // or not at all. One already on this change needs only its address. + pics := make(map[string]workflow.Picture, len(ids)) + pngs := make(map[string][]byte, len(ids)) + var gone []string for _, id := range ids { pic, png, err := workflow.KeptPicture(stateDir, id) - if err != nil { - return "", fmt.Errorf("%s is no longer kept, so it cannot be shown: take the screenshot again", id) + if err != nil && !errors.Is(err, workflow.ErrPictureNotKept) { + return "", fmt.Errorf("read picture %s: %w", id, err) + } + if err != nil || (png == nil && pic.Uploads[target] == "") { + gone = append(gone, id) + continue } + pics[id], pngs[id] = pic, png + } + if len(gone) > 0 { + return "", &picturesGoneError{ids: gone} + } + tags := make(map[string]func(alt string) string, len(ids)) + for _, id := range ids { + pic, png := pics[id], pngs[id] url := pic.Uploads[target] if url == "" { if len(png) > changePictureMaxBytes { @@ -350,7 +416,7 @@ func changeBody(ctx context.Context, hqc hq.Client, stateDir string, m *workflow kept, err := hqc.Attach(callCtx, m.HQ.Repo, number, png) cancel() var refused *hq.RefusedError - if errors.As(err, &refused) { + if errors.As(err, &refused) && refusedForGood(refused) { return "", fmt.Errorf("%w: HQ refused %s (%s)", errCannotAttach, id, hqRefusalWords(refused)) } if err != nil { @@ -385,15 +451,45 @@ func pictureTag(alt, url string, width, height int) string { return fmt.Sprintf(`%s`, html.EscapeString(alt), width, height, html.EscapeString(url)) } -// missingPictures is the pictures text shows that this Mate does not keep. -func missingPictures(stateDir, text string) []string { +// pictureRetakeSuggestion is what to do about a picture this Mate does not +// keep. +const pictureRetakeSuggestion = "Show a picture it keeps, or take the screenshot again with zerops_browser screenshot=true and show the picture its result names — or leave the picture out." + +// refusedForGood reports whether HQ's refusal of an attachment is one it +// will repeat: a 4xx other than a timeout or too many requests. +func refusedForGood(refused *hq.RefusedError) bool { + return refused.Status >= 400 && refused.Status < 500 && + refused.Status != http.StatusRequestTimeout && refused.Status != http.StatusTooManyRequests +} + +// keptPicturesSentence says which pictures this Mate keeps for the change +// target, and for how long. +func keptPicturesSentence(stateDir, target string) string { + rule := fmt.Sprintf("each for %s after it is taken (past %d MiB of pictures the oldest go first), every one a kept description shows, and one already on a change for that change", + pictureKeepWords(), workflow.PictureStoreBytes>>20) + ids, err := workflow.KeptPictures(stateDir, target) + if err != nil || len(ids) == 0 { + return "It keeps no picture now; it keeps a screenshot " + rule + "." + } + return "It keeps " + keptPicturesWords(ids) + ", " + rule + "." +} + +// missingPictures is the pictures text shows that the change target cannot +// show: none kept, or one whose file is gone and whose address is another +// change's. Any other failure to read the store is an error, never a +// picture gone. +func missingPictures(stateDir, text, target string) ([]string, error) { var missing []string for _, id := range workflow.PictureRefs(text) { - if _, _, err := workflow.KeptPicture(stateDir, id); err != nil { + pic, png, err := workflow.KeptPicture(stateDir, id) + if err != nil && !errors.Is(err, workflow.ErrPictureNotKept) { + return nil, err + } + if err != nil || (png == nil && pic.Uploads[target] == "") { missing = append(missing, id) } } - return missing + return missing, nil } // putKeptChangeDescription puts what the pair keeps onto change number — the @@ -420,8 +516,10 @@ func putKeptChangeDescription(ctx context.Context, hqc hq.Client, stateDir strin return false, "", true } err = putChangeDescription(ctx, hqc, stateDir, m, number, kept.Text, kept.Title) - if errors.Is(err, errCannotAttach) { - return false, err.Error(), false + var gone *picturesGoneError + if errors.As(err, &gone) || errors.Is(err, errCannotAttach) { + forgetChangeDescription(stateDir, m, kept.Text) + return false, "The description you wrote is dropped, not on it: " + err.Error() + ".", false } return err == nil, "", false } @@ -458,7 +556,7 @@ func nextDeliveryOf(m *workflow.ServiceMeta) string { // call, that they did. func describeLine(ref *changeRef, hostname string) string { if ref.DescriptionNote != "" { - return fmt.Sprintf(`The description you wrote is kept, not on it yet: %s. Describe it again without that picture with zerops_workflow action="describe-change" service=%q to put the words on now.`, + return fmt.Sprintf(`%s Describe it again with zerops_workflow action="describe-change" service=%q, without that picture or with one this Mate keeps.`, ref.DescriptionNote, hostname) } if ref.Described { @@ -485,3 +583,57 @@ func sentenceOf(line string) string { } return strings.ToUpper(line[:1]) + line[1:] + "." } + +// keptPicturesShown is how many runs of kept pictures a refusal names: the +// newest, so the list stays a line however many are kept. +const keptPicturesShown = 8 + +// keptPicturesWords names the kept pictures ids lists (oldest first) as runs +// — "shot-4, shot-50 to shot-52" — the newest keptPicturesShown of them, with +// how many older ones there are besides. +func keptPicturesWords(ids []string) string { + type run struct{ first, last, count int } + var runs []run + for _, id := range ids { + n, err := strconv.Atoi(strings.TrimPrefix(id, "shot-")) + if err != nil { + continue + } + if len(runs) > 0 && runs[len(runs)-1].last == n-1 { + runs[len(runs)-1].last = n + runs[len(runs)-1].count++ + continue + } + runs = append(runs, run{first: n, last: n, count: 1}) + } + var words []string + if len(runs) > keptPicturesShown { + older := 0 + for _, r := range runs[:len(runs)-keptPicturesShown] { + older += r.count + } + noun := "pictures" + if older == 1 { + noun = "picture" + } + words = append(words, fmt.Sprintf("%d older %s", older, noun)) + runs = runs[len(runs)-keptPicturesShown:] + } + for _, r := range runs { + switch r.count { + case 1: + words = append(words, fmt.Sprintf("shot-%d", r.first)) + case 2: + words = append(words, fmt.Sprintf("shot-%d, shot-%d", r.first, r.last)) + default: + words = append(words, fmt.Sprintf("shot-%d to shot-%d", r.first, r.last)) + } + } + return strings.Join(words, ", ") +} + +// pictureKeepWords is how long a picture is kept at most, in days: the byte +// bound can prune it sooner. +func pictureKeepWords() string { + return fmt.Sprintf("up to %d days", int(workflow.PictureKeepFor/(24*time.Hour))) +} diff --git a/internal/tools/hq_change_description_test.go b/internal/tools/hq_change_description_test.go index b83f8a80d..71fd51b93 100644 --- a/internal/tools/hq_change_description_test.go +++ b/internal/tools/hq_change_description_test.go @@ -5,6 +5,7 @@ package tools import ( "context" + "fmt" "strings" "testing" @@ -268,7 +269,7 @@ func TestDescribeChange_Refusals(t *testing.T) { {name: "more than a page", pairs: []string{"appdev"}, description: strings.Repeat("a", changeDescriptionMaxRunes+1), wantText: []string{"INVALID_PARAMETER", "20000"}}, {name: "a picture this Mate does not keep", pairs: []string{"appdev"}, description: "![gone](shot-7)", - wantText: []string{"INVALID_PARAMETER", "shot-7", "zerops_browser"}}, + wantText: []string{"INVALID_PARAMETER", "shot-7", "keeps no picture", "zerops_browser"}}, {name: "two pairs and no service", pairs: []string{"appdev", "apidev"}, description: describedWords, wantText: []string{"SERVICE_REQUIRED", `service=\"apidev\"`, `service=\"appdev\"`}}, {name: "a service that is no pair", pairs: []string{"appdev"}, service: "db", description: describedWords, @@ -332,6 +333,71 @@ func TestDescribeChange_Refusals(t *testing.T) { } } +// TestDescribeChange_MissingPictureNamesWhatIsKept: a refusal for a picture +// this Mate does not keep says which it does keep — the newest by id, the +// runs of them in one — and how long one is kept, so the Mate picks a kept +// one or retakes the screenshot instead of guessing. +func TestDescribeChange_MissingPictureNamesWhatIsKept(t *testing.T) { + tests := []struct { + name string + takes int + named []string // ids the description shows + wantText []string + }{ + {name: "a run of kept pictures", takes: 3, named: []string{"shot-2", "shot-9"}, + wantText: []string{"shot-9", "keeps shot-1 to shot-3", "7 days", "zerops_browser screenshot=true"}}, + {name: "one kept picture", takes: 1, named: []string{"shot-4"}, + wantText: []string{"shot-4", "keeps shot-1,", "7 days"}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + lab := deliveredLab(t) + for i := range tt.takes { + keptScreenshot(t, lab.stateDir, fmt.Appendf(nil, "\x89PNG-%d", i), 800, 600) + } + var words strings.Builder + for _, id := range tt.named { + words.WriteString("![a page](" + id + ")\n") + } + text, isError := lab.describe("", words.String()) + if !isError { + t.Fatalf("want a refusal, got:\n%s", text) + } + for _, want := range tt.wantText { + if !strings.Contains(text, want) { + t.Errorf("the refusal misses %q:\n%s", want, text) + } + } + }) + } +} + +// TestKeptPicturesWords: the kept ids read as runs, oldest first, and a long +// list keeps its newest runs. +func TestKeptPicturesWords(t *testing.T) { + t.Parallel() + many := []string{} + for i := 1; i <= 40; i += 2 { + many = append(many, fmt.Sprintf("shot-%d", i)) + } + tests := []struct { + ids []string + want string + }{ + {nil, ""}, + {[]string{"shot-5"}, "shot-5"}, + {[]string{"shot-4", "shot-5"}, "shot-4, shot-5"}, + {[]string{"shot-4", "shot-50", "shot-51", "shot-52", "shot-69"}, "shot-4, shot-50 to shot-52, shot-69"}, + {many, "12 older pictures, shot-25, shot-27, shot-29, shot-31, shot-33, shot-35, shot-37, shot-39"}, + {many[:9], "1 older picture, shot-3, shot-5, shot-7, shot-9, shot-11, shot-13, shot-15, shot-17"}, + } + for _, tt := range tests { + if got := keptPicturesWords(tt.ids); got != tt.want { + t.Errorf("keptPicturesWords(%v) = %q, want %q", tt.ids, got, tt.want) + } + } +} + // keptScreenshot keeps a picture the way zerops_browser does, and returns its // id. func keptScreenshot(t *testing.T, stateDir string, png []byte, width, height int) string { @@ -368,21 +434,24 @@ func TestDescribeChange_Pictures(t *testing.T) { } }) - t.Run("a picture HQ will not keep publishes nothing and keeps the words", func(t *testing.T) { + t.Run("a picture HQ will not keep publishes nothing and drops the words", func(t *testing.T) { lab := deliveredLab(t) id := keptScreenshot(t, lab.stateDir, []byte("not a png at all"), 800, 600) words := "![The count](" + id + ")" text, isError := lab.describe("", words) - for _, want := range []string{"PREREQUISITE_MISSING", "Nothing was written onto change #1", "not_png", "without that picture"} { + for _, want := range []string{"PREREQUISITE_MISSING", "Nothing was written onto change #1", "not_png", "dropped", "without that picture"} { if !isError || !strings.Contains(text, want) { t.Errorf("want a refusal saying %q, got:\n%s", want, text) } } + if strings.Contains(text, "goes onto") { + t.Errorf("HQ refuses that picture every time, yet the refusal promises the words go on:\n%s", text) + } if body := lab.hq.change(1).Body; body != "" { t.Errorf("a body went onto the change with its picture refused: %q", body) } - if kept := lab.meta().HQ.ChangeDescription; kept == nil || kept.Text != words { - t.Errorf("kept = %+v, want the words kept", kept) + if kept := lab.meta().HQ.ChangeDescription; kept != nil { + t.Errorf("kept = %+v, want the words dropped: they could never go on", kept) } }) } diff --git a/internal/tools/hq_change_pictures_test.go b/internal/tools/hq_change_pictures_test.go new file mode 100644 index 000000000..3f4dfe725 --- /dev/null +++ b/internal/tools/hq_change_pictures_test.go @@ -0,0 +1,274 @@ +// Tests for: tools/hq_change_description.go — the pictures a description +// shows, as the store keeps them (workflow.KeepPicture), run in the HQ lab. +package tools + +import ( + "net/http" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/zeropsio/zcp/internal/workflow" +) + +// prunePictureFile takes a picture's file away the way a prune does once the +// picture is past the store's bounds — its entry stays only for its address. +func prunePictureFile(t *testing.T, stateDir, id string) { + t.Helper() + if err := os.Remove(filepath.Join(stateDir, "pictures", id+".png")); err != nil { + t.Fatal(err) + } +} + +// TestDescribeChange_PicturePrunedAfterItWentOn: a change already carries +// the picture, HQ still serves it, and a push made the change a draft. The +// Mate describes it again with the same picture after its file was pruned: +// on that change the address it was attached at is enough. Another change +// needs the file, and the refusal says the picture is gone. +func TestDescribeChange_PicturePrunedAfterItWentOn(t *testing.T) { + tests := []struct { + name string + // attachedTo is the change the picture went onto before its file was + // pruned, "" for the change being described. + attachedTo string + wantText []string + wantError bool + }{ + {name: "the same change shows it from HQ", wantText: []string{`"described":true`}}, + {name: "another change needs the file", attachedTo: labApp + "/appdev#7", wantError: true, + wantText: []string{"INVALID_PARAMETER", "shot-1", "keeps no such picture", "zerops_browser"}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + lab := deliveredLab(t) + id := keptScreenshot(t, lab.stateDir, []byte("\x89PNG\r\n\x1a\n-a-before"), 1280, 720) + words := "## Before\n\n![The old home page](" + id + ")\n" + if tt.attachedTo == "" { + if text, isError := lab.describeTitled("", "", words); isError { + t.Fatalf("not described:\n%s", text) + } + } else if err := workflow.RecordPictureUpload(lab.stateDir, id, tt.attachedTo, "https://hq.example.invalid/att-7"); err != nil { + t.Fatal(err) + } + prunePictureFile(t, lab.stateDir, id) + + text, isError := lab.describeTitled("", "", words) + if isError != tt.wantError { + t.Fatalf("isError = %v, want %v:\n%s", isError, tt.wantError, text) + } + for _, want := range tt.wantText { + if !strings.Contains(text, want) { + t.Errorf("the answer misses %q:\n%s", want, text) + } + } + attached := len(lab.hq.attachments) + if tt.attachedTo == "" { + body := lab.hq.change(1).Body + if attached != 1 || !strings.Contains(body, "/attachments/att-1") { + t.Errorf("attached %d times, body %q: want the first attachment's address reused", attached, body) + } + return + } + if attached != 0 || lab.hq.change(1).Body != "" { + t.Errorf("a refused description attached %d pictures or wrote %q", attached, lab.hq.change(1).Body) + } + }) + } +} + +// TestDescribeChange_PicturePrunedWhileDescribing: the check passed, and a +// helper's screenshot pruned the picture before the words went on. The Mate +// is told plainly which picture is gone, nothing is written, and the words +// are not kept — kept, they would protect their other pictures and fail on +// every later delivery. +func TestDescribeChange_PicturePrunedWhileDescribing(t *testing.T) { + lab := deliveredLab(t) + kept := keptScreenshot(t, lab.stateDir, []byte("\x89PNG\r\n\x1a\n-kept"), 800, 600) + gone := keptScreenshot(t, lab.stateDir, []byte("\x89PNG\r\n\x1a\n-pruned"), 800, 600) + // The fake HQ's failOn sees every request: here it only prunes, at the + // moment the describe reads the Mate's state, after its check. + lab.hq.mu.Lock() + lab.hq.failing, lab.hq.failOn = 1, func(r *http.Request) bool { + if r.URL.Path == "/api/mate/self" { + _ = os.Remove(filepath.Join(lab.stateDir, "pictures", gone+".png")) + } + return false + } + lab.hq.mu.Unlock() + + text, isError := lab.describeTitled("", "", "![after]("+kept+")\n![before]("+gone+")") + if !isError { + t.Fatalf("want a refusal, got:\n%s", text) + } + for _, want := range []string{"INVALID_PARAMETER", gone, "no longer kept", "Nothing was written"} { + if !strings.Contains(text, want) { + t.Errorf("the refusal misses %q:\n%s", want, text) + } + } + if strings.Contains(text, "goes onto #") { + t.Errorf("the refusal promises the words go on later:\n%s", text) + } + if words := lab.meta().HQ.ChangeDescription; words != nil { + t.Errorf("the words are kept: %+v", words) + } + if len(lab.hq.attachments) != 0 || lab.hq.change(1).Body != "" { + t.Errorf("attached %d pictures, body %q: want nothing written", len(lab.hq.attachments), lab.hq.change(1).Body) + } +} + +// TestDescribeChange_KeptWordsMeetAGonePicture: words kept while HQ did not +// answer, whose picture's file is lost before the delivery that would put +// them on, are dropped with a line naming the picture — never kept to fail +// again on every delivery. +func TestDescribeChange_KeptWordsMeetAGonePicture(t *testing.T) { + lab := deliveredLab(t) + id := keptScreenshot(t, lab.stateDir, []byte("\x89PNG\r\n\x1a\n-a-picture"), 800, 600) + lab.hq.setDown(true) + if text, isError := lab.describeTitled("", "", "![The count]("+id+")"); isError || !strings.Contains(text, `"kept":true`) { + t.Fatalf("want the words kept while HQ does not answer:\n%s", text) + } + lab.hq.setDown(false) + prunePictureFile(t, lab.stateDir, id) + + delivery := lab.deliver() + if delivery == nil || delivery.Change == nil { + t.Fatalf("delivery: %+v", delivery) + } + if !strings.Contains(delivery.Line, id) || !strings.Contains(delivery.Line, "no longer kept") { + t.Errorf("the line does not say %s is gone:\n%s", id, delivery.Line) + } + if words := lab.meta().HQ.ChangeDescription; words != nil { + t.Errorf("the words are still kept: %+v", words) + } +} + +// TestDescribeChange_RefusalListsWhatThisChangeCanShow: a picture whose file +// is gone, kept only as the address of another change's attachment, is not +// one this change can show — the quick check refuses it, and the list of +// kept pictures leaves it out. +func TestDescribeChange_RefusalListsWhatThisChangeCanShow(t *testing.T) { + lab := deliveredLab(t) + for range 3 { + keptScreenshot(t, lab.stateDir, []byte("\x89PNG\r\n\x1a\n-a-page"), 800, 600) + } + if err := workflow.RecordPictureUpload(lab.stateDir, "shot-1", workflow.PictureTarget(labApp, "appdev", 3), "https://hq.example.invalid/att-3"); err != nil { + t.Fatal(err) + } + prunePictureFile(t, lab.stateDir, "shot-1") + attachedBefore := len(lab.hq.attachments) + + text, isError := lab.describeTitled("", "", "![The old page](shot-1)") + if !isError { + t.Fatalf("want a refusal, got:\n%s", text) + } + for _, want := range []string{"shows shot-1", "keeps no such picture", "It keeps shot-2, shot-3,"} { + if !strings.Contains(text, want) { + t.Errorf("the refusal misses %q:\n%s", want, text) + } + } + if len(lab.hq.attachments) != attachedBefore || lab.hq.change(1).Body != "" { + t.Errorf("a refused description wrote onto the change") + } +} + +// TestDescribeChange_PictureStoreUnreadable: a store that cannot be read is +// not a picture gone. The describe says what failed instead of "no longer +// kept", and words kept for a delivery stay kept. +func TestDescribeChange_PictureStoreUnreadable(t *testing.T) { + spoil := func(t *testing.T, stateDir string) { + t.Helper() + if err := os.WriteFile(filepath.Join(stateDir, "pictures", "index.json"), []byte("{not json"), 0o600); err != nil { + t.Fatal(err) + } + } + t.Run("the describe says what failed", func(t *testing.T) { + lab := deliveredLab(t) + id := keptScreenshot(t, lab.stateDir, []byte("\x89PNG\r\n\x1a\n-a-page"), 800, 600) + spoil(t, lab.stateDir) + text, isError := lab.describeTitled("", "", "![The page]("+id+")") + if !isError || !strings.Contains(text, "pictures index") || strings.Contains(text, "no longer kept") || strings.Contains(text, "no such picture") { + t.Errorf("want an error naming the unreadable store, got:\n%s", text) + } + }) + t.Run("kept words stay kept", func(t *testing.T) { + lab := deliveredLab(t) + id := keptScreenshot(t, lab.stateDir, []byte("\x89PNG\r\n\x1a\n-a-page"), 800, 600) + lab.hq.setDown(true) + words := "![The page](" + id + ")" + if text, isError := lab.describeTitled("", "", words); isError || !strings.Contains(text, `"kept":true`) { + t.Fatalf("want the words kept while HQ does not answer:\n%s", text) + } + lab.hq.setDown(false) + spoil(t, lab.stateDir) + delivery := lab.deliver() + if delivery == nil || delivery.Change == nil { + t.Fatalf("delivery: %+v", delivery) + } + if strings.Contains(delivery.Line, "dropped") { + t.Errorf("the line drops the words over an unreadable store:\n%s", delivery.Line) + } + if kept := lab.meta().HQ.ChangeDescription; kept == nil || kept.Text != words { + t.Errorf("kept = %+v, want the words still kept", kept) + } + }) +} + +// TestDescribeChange_AttachRefusedForNow: HQ refusing an attachment for now +// (429, a 5xx) is not a picture it will never take: the words are kept for +// the next delivery, as when HQ does not answer. +func TestDescribeChange_AttachRefusedForNow(t *testing.T) { + tests := []struct { + name string + status int + }{ + {name: "too many requests", status: http.StatusTooManyRequests}, + {name: "a server error", status: http.StatusBadGateway}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + lab := deliveredLab(t) + id := keptScreenshot(t, lab.stateDir, []byte("\x89PNG\r\n\x1a\n-a-page"), 800, 600) + lab.hq.mu.Lock() + lab.hq.failing, lab.hq.failStatus, lab.hq.failCode = 1, tt.status, "busy" + lab.hq.failOn = func(r *http.Request) bool { return strings.HasSuffix(r.URL.Path, "/attachments") } + lab.hq.mu.Unlock() + words := "![The page](" + id + ")" + text, _ := lab.describeTitled("", "", words) + if !strings.Contains(text, `"kept":true`) || !strings.Contains(text, "goes onto #1") { + t.Errorf("want the words kept for the next delivery, got:\n%s", text) + } + if kept := lab.meta().HQ.ChangeDescription; kept == nil || kept.Text != words { + t.Errorf("kept = %+v, want the words kept", kept) + } + }) + } +} + +// TestDescribeChange_KeptWordsMeetARefusedPicture: words kept for a delivery +// whose picture HQ will never take are dropped with a line that says so, +// as whole sentences, never promised again. +func TestDescribeChange_KeptWordsMeetARefusedPicture(t *testing.T) { + lab := deliveredLab(t) + id := keptScreenshot(t, lab.stateDir, []byte("not a png at all"), 800, 600) + lab.hq.setDown(true) + if text, isError := lab.describeTitled("", "", "![The count]("+id+")"); isError || !strings.Contains(text, `"kept":true`) { + t.Fatalf("want the words kept while HQ does not answer:\n%s", text) + } + lab.hq.setDown(false) + delivery := lab.deliver() + if delivery == nil || delivery.Change == nil { + t.Fatalf("delivery: %+v", delivery) + } + for _, want := range []string{"dropped", "not_png", `action="describe-change"`} { + if !strings.Contains(delivery.Line, want) { + t.Errorf("the line misses %q:\n%s", want, delivery.Line) + } + } + if strings.Contains(delivery.Line, ",.") || strings.Contains(delivery.Line, ", with zerops_workflow") { + t.Errorf("the line reads as a fragment:\n%s", delivery.Line) + } + if words := lab.meta().HQ.ChangeDescription; words != nil { + t.Errorf("the words are still kept: %+v", words) + } +} diff --git a/internal/tools/preprocess.go b/internal/tools/preprocess.go index fbdac6e19..f6d2672a1 100644 --- a/internal/tools/preprocess.go +++ b/internal/tools/preprocess.go @@ -26,7 +26,7 @@ type PreprocessResult struct { func RegisterPreprocess(srv *mcp.Server) { mcp.AddTool(srv, &mcp.Tool{ Name: "zerops_preprocess", - Description: "Expand Zerops preprocessor expressions via the official zParser library — output is byte-for-byte what the platform produces at recipe import time. Functions: generateRandomString(), generateRandomInt(,), generateRandomBytes(), generateED25519Key/RSA2048Key/RSA4096Key(), pickRandom, setVar, getVar. Modifiers: sha256, sha512, bcrypt, argon2id, toHex, toString, upper, lower, title. Single: input=expr. Batch: inputs={name:expr} + order=[...] shares a variable store so setVar/getVar span keys. Primary use: generating workspace shared-secret values matching the deliverable's preprocessor output exactly. zerops_env set also auto-expands values containing <@...> syntax — use this tool when you need the raw value, batched correlation, or key-pair generation.", + Description: "Expand Zerops preprocessor expressions via the official zParser library — output is byte-for-byte what the platform produces at recipe import time. Functions: generateRandomString(), generateRandomInt(, ), generateRandomBytes(), generateED25519Key/RSA2048Key/RSA4096Key(), pickRandom, setVar, getVar — a space after each comma between arguments, or the preprocessor fails. Modifiers: sha256, sha512, bcrypt, argon2id, toHex, toString, upper, lower, title. Single: input=expr. Batch: inputs={name:expr} + order=[...] shares a variable store so setVar/getVar span keys. Primary use: generating workspace shared-secret values matching the deliverable's preprocessor output exactly. zerops_env set also auto-expands values containing <@...> syntax — use this tool when you need the raw value, batched correlation, or key-pair generation.", Annotations: &mcp.ToolAnnotations{ Title: "Expand Zerops preprocessor expressions", ReadOnlyHint: true, diff --git a/internal/workflow/pictures.go b/internal/workflow/pictures.go index 8496bfa9d..2893ace97 100644 --- a/internal/workflow/pictures.go +++ b/internal/workflow/pictures.go @@ -19,12 +19,34 @@ import ( // it as the change's attachments; the store remembers where HQ serves each // one, so a picture goes onto a change once. // -// Kept: the newest PictureKeep, and every picture a kept description names — -// words waiting for a change to carry them must find their pictures there. +// Kept: every picture for PictureKeepFor after it is taken — a change's +// "before" is often taken hours, or a whole step, ahead of its description, +// so the bound is time, not a count of the newest — while the store holds at +// most PictureStoreBytes on disk, past which the oldest go first; and, beyond +// both, every picture a kept description names: words waiting for a change to +// carry them must find their pictures there. The newest is always kept. A +// picture already on a change outlives its file: the store keeps where HQ +// serves it, so the Mate can describe that change again with it — only +// another change needs the file — while that change is a pair's current one, +// and for PictureAddressKeepFor at most. One store per state dir, shared by +// every zcp process on it under its own lock; reading it writes nothing. // An id is never given twice: the next one only ever grows. -// PictureKeep is how many of the newest pictures are kept. -const PictureKeep = 20 +// PictureKeepFor is how long a picture is kept after it is taken. +const PictureKeepFor = 7 * 24 * time.Hour + +// PictureStoreBytes bounds the pictures on disk: past it, the oldest go first. +const PictureStoreBytes int64 = 256 << 20 + +// PictureAddressKeepFor bounds how long the address of a picture whose file +// is gone is kept, however long its change stays open. +const PictureAddressKeepFor = 90 * 24 * time.Hour + +// PictureTarget is how the store names a change a picture went onto: its +// application, repository and number ("{appId}/{repo}#{number}"). +func PictureTarget(appID, repo string, number int) string { + return fmt.Sprintf("%s/%s#%d", appID, repo, number) +} const ( picturesDir = "pictures" @@ -34,7 +56,7 @@ const ( ) // ErrPictureNotKept is a picture id the store has no picture for: one never -// taken, or one pruned since. +// taken, or one pruned since that no change carries. var ErrPictureNotKept = errors.New("no such picture is kept") // Picture is one kept picture. @@ -81,6 +103,37 @@ func ReplacePictureRefs(text string, replace func(alt, id string) string) string // KeepPicture keeps png as the next picture, with its size in pixels, and // forgets what the store no longer keeps. func KeepPicture(stateDir string, png []byte, width, height int) (Picture, error) { + return keepPicture(stateDir, png, width, height, time.Now(), defaultPictureBounds()) +} + +// pictureBounds is how long and how much the store keeps. +type pictureBounds struct { + keepFor time.Duration + maxBytes int64 +} + +func defaultPictureBounds() pictureBounds { + return pictureBounds{keepFor: PictureKeepFor, maxBytes: PictureStoreBytes} +} + +// KeptPictures is the id of every picture the store keeps for the change +// target (PictureTarget), oldest first: each one on disk, and each whose file +// is gone that is already on that change. "" is no change: those on disk. +func KeptPictures(stateDir, target string) ([]string, error) { + var ids []string + err := readPictures(stateDir, func(index *pictureIndex) error { + for _, pic := range index.Pictures { + if _, onDisk := pictureFileSize(stateDir, pic.ID); onDisk || (target != "" && pic.Uploads[target] != "") { + ids = append(ids, pic.ID) + } + } + return nil + }) + return ids, err +} + +// keepPicture is KeepPicture taken at now, within bounds. +func keepPicture(stateDir string, png []byte, width, height int, now time.Time, bounds pictureBounds) (Picture, error) { var kept Picture err := withPictures(stateDir, func(index *pictureIndex) error { if index.Next < 1 { @@ -88,30 +141,33 @@ func KeepPicture(stateDir string, png []byte, width, height int) (Picture, error } kept = Picture{ ID: pictureIDPrefix + strconv.Itoa(index.Next), Width: width, Height: height, - TakenAt: time.Now().UTC().Format(time.RFC3339), + TakenAt: now.UTC().Format(time.RFC3339), } if err := os.WriteFile(picturePath(stateDir, kept.ID), png, 0o600); err != nil { return fmt.Errorf("keep picture %s: %w", kept.ID, err) } index.Next++ index.Pictures = append(index.Pictures, kept) - prunePictures(stateDir, index) + prunePictures(stateDir, index, now, bounds) return nil }) return kept, err } // KeptPicture is the picture id names and its bytes, or ErrPictureNotKept. +// A picture whose file is gone but which a change carries is found without +// its bytes (nil): its Uploads say where HQ serves it, and only there can it +// be shown. func KeptPicture(stateDir, id string) (Picture, []byte, error) { var found Picture var png []byte - err := withPictures(stateDir, func(index *pictureIndex) error { + err := readPictures(stateDir, func(index *pictureIndex) error { for _, pic := range index.Pictures { if pic.ID != id { continue } data, err := os.ReadFile(picturePath(stateDir, id)) - if err != nil { + if err != nil && len(pic.Uploads) == 0 { return fmt.Errorf("%s: %w", id, ErrPictureNotKept) } found, png = pic, data @@ -140,33 +196,150 @@ func RecordPictureUpload(stateDir, id, target, url string) error { }) } -// prunePictures forgets every picture past the newest PictureKeep that no kept -// description names — its entry and its file. -func prunePictures(stateDir string, index *pictureIndex) { - if len(index.Pictures) <= PictureKeep { +// prunePictures forgets every picture the store no longer keeps: one taken +// longer than bounds.keepFor before now, and, newest to oldest, every one past +// bounds.maxBytes of files on disk; never the newest, nor one a kept +// description names. A forgotten picture loses its file, and its entry too +// unless it is on a pair's current change and younger than +// PictureAddressKeepFor (its address is all that is left then); an entry +// whose file is already gone is forgotten the same way. Nothing is pruned +// while the pairs' directory cannot be read: which pictures a kept +// description names is unknown then. +func prunePictures(stateDir string, index *pictureIndex, now time.Time, bounds pictureBounds) { + metas, err := readablePairs(stateDir) + if err != nil { return } named := map[string]bool{} - if metas, err := ListServiceMetas(stateDir); err == nil { - for _, m := range metas { - if m == nil || m.HQ == nil || m.HQ.ChangeDescription == nil { + current := map[string]bool{} + for _, m := range metas { + if m.HQ == nil { + continue + } + if m.HQ.Change != 0 { + current[PictureTarget(m.HQ.AppID, m.HQ.Repo, m.HQ.Change)] = true + } + if m.HQ.ChangeDescription == nil { + continue + } + for _, id := range PictureRefs(m.HQ.ChangeDescription.Text) { + named[id] = true + } + } + keepFile := make([]bool, len(index.Pictures)) + var held int64 + full := false + for i := len(index.Pictures) - 1; i >= 0; i-- { + pic := index.Pictures[i] + size, onDisk := pictureFileSize(stateDir, pic.ID) + if !onDisk { + continue + } + if named[pic.ID] { + keepFile[i] = true + continue + } + newest := i == len(index.Pictures)-1 + if !newest && (full || pictureExpired(pic, now, bounds.keepFor) || held+size > bounds.maxBytes) { + full = full || held+size > bounds.maxBytes + continue + } + keepFile[i] = true + held += size + } + kept := make([]Picture, 0, len(index.Pictures)) + for i, pic := range index.Pictures { + if !keepFile[i] { + _ = os.Remove(picturePath(stateDir, pic.ID)) + if !addressKept(pic, current, now) { continue } - for _, id := range PictureRefs(m.HQ.ChangeDescription.Text) { - named[id] = true - } } + kept = append(kept, pic) } - oldest := len(index.Pictures) - PictureKeep - kept := make([]Picture, 0, PictureKeep+len(named)) - for i, pic := range index.Pictures { - if i >= oldest || named[pic.ID] { - kept = append(kept, pic) + index.Pictures = kept +} + +// addressKept reports whether the entry of pic, whose file is gone, is kept +// for its address: it is on a current change, and not older than +// PictureAddressKeepFor. +func addressKept(pic Picture, current map[string]bool, now time.Time) bool { + if pictureExpired(pic, now, PictureAddressKeepFor) { + return false + } + for target := range pic.Uploads { + if current[target] { + return true + } + } + return false +} + +// readablePairs is every pair record that parses. One that does not is +// skipped: its words could never go onto a change either. Only a directory +// that cannot be read fails it. +func readablePairs(stateDir string) ([]*ServiceMeta, error) { + dir, names, err := serviceMetaFiles(stateDir) + if err != nil { + return nil, err + } + var metas []*ServiceMeta + for _, name := range names { + data, err := os.ReadFile(filepath.Join(dir, name)) + if err != nil { continue } - _ = os.Remove(picturePath(stateDir, pic.ID)) + if meta, err := parseMeta(data); err == nil && meta != nil { + metas = append(metas, meta) + } } - index.Pictures = kept + return metas, nil +} + +// pictureFileSize is the size of the picture id's file, and whether it is on +// disk at all. +func pictureFileSize(stateDir, id string) (int64, bool) { + info, err := os.Stat(picturePath(stateDir, id)) + if err != nil { + return 0, false + } + return info.Size(), true +} + +// pictureExpired reports whether pic was taken longer than keepFor before +// now. A picture whose time cannot be read is not expired: the byte bound +// still holds it. +func pictureExpired(pic Picture, now time.Time, keepFor time.Duration) bool { + taken, err := time.Parse(time.RFC3339, pic.TakenAt) + return err == nil && now.Sub(taken) > keepFor +} + +// readPictures runs fn on the store's index under the store's lock, and +// writes nothing: a read never fails for a store it cannot write. +func readPictures(stateDir string, fn func(*pictureIndex) error) error { + return withFileLock(filepath.Join(stateDir, picturesLockName), func() error { + index, err := loadPictureIndex(stateDir) + if err != nil { + return err + } + return fn(&index) + }) +} + +// loadPictureIndex reads the store's index; none yet is an empty one. +func loadPictureIndex(stateDir string) (pictureIndex, error) { + var index pictureIndex + data, err := os.ReadFile(filepath.Join(stateDir, picturesDir, picturesIndex)) + if errors.Is(err, os.ErrNotExist) { + return index, nil + } + if err != nil { + return index, fmt.Errorf("pictures index: %w", err) + } + if err := json.Unmarshal(data, &index); err != nil { + return index, fmt.Errorf("pictures index: %w", err) + } + return index, nil } // withPictures runs fn on the store's index under the store's own lock — a @@ -177,14 +350,9 @@ func withPictures(stateDir string, fn func(*pictureIndex) error) error { return fmt.Errorf("pictures dir: %w", err) } return withFileLock(filepath.Join(stateDir, picturesLockName), func() error { - var index pictureIndex - path := filepath.Join(stateDir, picturesDir, picturesIndex) - if data, err := os.ReadFile(path); err == nil { - if jsonErr := json.Unmarshal(data, &index); jsonErr != nil { - return fmt.Errorf("pictures index: %w", jsonErr) - } - } else if !errors.Is(err, os.ErrNotExist) { - return fmt.Errorf("pictures index: %w", err) + index, err := loadPictureIndex(stateDir) + if err != nil { + return err } if err := fn(&index); err != nil { return err @@ -193,6 +361,7 @@ func withPictures(stateDir string, fn func(*pictureIndex) error) error { if err != nil { return fmt.Errorf("pictures index: %w", err) } + path := filepath.Join(stateDir, picturesDir, picturesIndex) tmp := path + ".tmp" if err := os.WriteFile(tmp, data, 0o600); err != nil { return fmt.Errorf("pictures index: %w", err) diff --git a/internal/workflow/pictures_test.go b/internal/workflow/pictures_test.go index ac53b21bc..1848d14f2 100644 --- a/internal/workflow/pictures_test.go +++ b/internal/workflow/pictures_test.go @@ -7,7 +7,9 @@ import ( "os" "path/filepath" "slices" + "sync" "testing" + "time" ) // fakePNG is not a picture anyone could look at; the store never decodes one. @@ -41,41 +43,409 @@ func TestKeepPicture_NamesEachPictureOnce(t *testing.T) { } } -// TestKeepPicture_KeepsTheNewestAndThoseAKeptDescriptionNames: the store keeps -// the newest twenty, and every picture a kept description names however old -// it is — those are waiting to go onto a change. -func TestKeepPicture_KeepsTheNewestAndThoseAKeptDescriptionNames(t *testing.T) { +// TestKeepPicture_KeepsAWorkingDaysPictures: run 12's Sage took 69 pictures +// over seven and a half hours — the "before" of every page at the start, the +// "after" at the end — and described its change with 30 of them. Every one is +// still kept when the description comes: a picture is kept for PictureKeepFor, +// not only while it is among the newest few. +func TestKeepPicture_KeepsAWorkingDaysPictures(t *testing.T) { t.Parallel() stateDir := t.TempDir() + start := time.Date(2026, 10, 5, 14, 37, 0, 0, time.UTC) + for i := range 69 { + at := start.Add(time.Duration(i) * 6 * time.Minute) + if _, err := keepPicture(stateDir, fakePNG(i+1), 1440, 900, at, defaultPictureBounds()); err != nil { + t.Fatalf("keep picture %d: %v", i+1, err) + } + } + for i := 1; i <= 69; i++ { + if _, _, err := KeptPicture(stateDir, fmt.Sprintf("shot-%d", i)); err != nil { + t.Errorf("shot-%d is gone after a day's work: %v", i, err) + } + } +} + +// TestKeepPicture_ForgetsByAgeAndSize: the store's bound is time and bytes — +// a picture goes once it is older than the store keeps, or, oldest first, once +// the newer ones fill the bytes the store keeps — and never the newest, nor +// one a kept description names, which is waiting to go onto a change. +func TestKeepPicture_ForgetsByAgeAndSize(t *testing.T) { + t.Parallel() + start := time.Date(2026, 10, 1, 9, 0, 0, 0, time.UTC) + size := int64(len(fakePNG(1))) + tests := []struct { + name string + bounds pictureBounds + taken []time.Duration // after start, one picture each + described string // a kept description's words + wantKept []string + }{ + { + name: "a picture older than the store keeps goes", + bounds: pictureBounds{keepFor: 48 * time.Hour, maxBytes: 1 << 20}, + taken: []time.Duration{0, time.Hour, 49 * time.Hour}, + wantKept: []string{"shot-2", "shot-3"}, + }, + { + name: "past the store's bytes the oldest go first", + bounds: pictureBounds{keepFor: 48 * time.Hour, maxBytes: 3 * size}, + taken: []time.Duration{0, time.Minute, 2 * time.Minute, 3 * time.Minute, 4 * time.Minute}, + wantKept: []string{"shot-3", "shot-4", "shot-5"}, + }, + { + name: "the newest is kept even when it alone fills the store", + bounds: pictureBounds{keepFor: 48 * time.Hour, maxBytes: 1}, + taken: []time.Duration{0, time.Minute}, + wantKept: []string{"shot-2"}, + }, + { + name: "a kept description's picture outlives both bounds", + bounds: pictureBounds{keepFor: 48 * time.Hour, maxBytes: 2 * size}, + taken: []time.Duration{0, time.Minute, 2 * time.Minute, 3 * time.Minute, 72 * time.Hour}, + described: "## How I checked it\n\n![The count](shot-2)", + wantKept: []string{"shot-2", "shot-5"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + stateDir := t.TempDir() + if tt.described != "" { + if err := WriteServiceMeta(stateDir, &ServiceMeta{ + Hostname: "appdev", StageHostname: "appstage", BootstrapSession: "test", BootstrappedAt: "2026-09-29", + HQ: &HQRepoRef{AppID: "a1", Repo: "appdev", Branch: "mate/p1", ChangeDescription: &ChangeDescription{Text: tt.described}}, + }); err != nil { + t.Fatalf("WriteServiceMeta: %v", err) + } + } + for i, after := range tt.taken { + if _, err := keepPicture(stateDir, fakePNG(i+1), 800, 600, start.Add(after), tt.bounds); err != nil { + t.Fatalf("keep picture %d: %v", i+1, err) + } + } + kept, err := KeptPictures(stateDir, "") + if err != nil { + t.Fatalf("KeptPictures: %v", err) + } + if !slices.Equal(kept, tt.wantKept) { + t.Errorf("kept %v, want %v", kept, tt.wantKept) + } + for i := range tt.taken { + id := fmt.Sprintf("shot-%d", i+1) + _, statErr := os.Stat(filepath.Join(stateDir, "pictures", id+".png")) + if slices.Contains(tt.wantKept, id) != (statErr == nil) { + t.Errorf("%s's file: %v, want it there only while it is kept", id, statErr) + } + } + next, err := KeepPicture(stateDir, fakePNG(99), 800, 600) + if want := fmt.Sprintf("shot-%d", len(tt.taken)+1); err != nil || next.ID != want { + t.Errorf("the next picture is %q (%v), want %s: an id is never given twice", next.ID, err, want) + } + }) + } +} + +// writePair writes a pair whose current change in HQ is appdev#change of +// application a1, with words kept for it when described is not "". +func writePair(t *testing.T, stateDir string, change int, described string) { + t.Helper() + ref := &HQRepoRef{AppID: "a1", Repo: "appdev", Branch: "mate/p1", Change: change} + if described != "" { + ref.ChangeDescription = &ChangeDescription{Text: described, Change: change} + } if err := WriteServiceMeta(stateDir, &ServiceMeta{ - Hostname: "appdev", StageHostname: "appstage", BootstrapSession: "test", BootstrappedAt: "2026-09-29", - HQ: &HQRepoRef{AppID: "a1", Repo: "appdev", Branch: "mate/p1", ChangeDescription: &ChangeDescription{ - Text: "## How I checked it\n\n![The count](shot-2)", - }}, + Hostname: "appdev", StageHostname: "appstage", BootstrapSession: "test", BootstrappedAt: "2026-09-29", HQ: ref, }); err != nil { t.Fatalf("WriteServiceMeta: %v", err) } - for i := 1; i <= PictureKeep+5; i++ { +} + +// TestKeepPicture_PrunedPictureKeepsItsAddress: a picture already on a change +// is still served by HQ when its file is pruned, and a push makes the change +// a draft that the Mate describes again with the same picture. So the store +// forgets its file but keeps where HQ serves it — while that change is a +// pair's current one, and for PictureAddressKeepFor at most. One never +// attached goes entirely. +func TestKeepPicture_PrunedPictureKeepsItsAddress(t *testing.T) { + t.Parallel() + start := time.Date(2026, 10, 1, 9, 0, 0, 0, time.UTC) + bounds := pictureBounds{keepFor: 48 * time.Hour, maxBytes: 1 << 20} + url := "https://hq.example.invalid/api/apps/a1/changes/appdev/1/attachments/att-1" + tests := []struct { + name string + currentChange int // the pair's current change, 0 for none + pruneAfter time.Duration // when the picture that prunes is taken + wantAddress bool + }{ + {name: "its change is the pair's current one", currentChange: 1, pruneAfter: 72 * time.Hour, wantAddress: true}, + {name: "the pair moved on to another change", currentChange: 2, pruneAfter: 72 * time.Hour}, + {name: "the pair has no change now", pruneAfter: 72 * time.Hour}, + {name: "older than an address is kept", currentChange: 1, pruneAfter: PictureAddressKeepFor + time.Hour}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + stateDir := t.TempDir() + writePair(t, stateDir, tt.currentChange, "") + for i := 1; i <= 2; i++ { + if _, err := keepPicture(stateDir, fakePNG(i), 800, 600, start, bounds); err != nil { + t.Fatalf("keep picture %d: %v", i, err) + } + } + if err := RecordPictureUpload(stateDir, "shot-1", PictureTarget("a1", "appdev", 1), url); err != nil { + t.Fatalf("RecordPictureUpload: %v", err) + } + if _, err := keepPicture(stateDir, fakePNG(3), 800, 600, start.Add(tt.pruneAfter), bounds); err != nil { + t.Fatalf("keep picture 3: %v", err) + } + + pic, png, err := KeptPicture(stateDir, "shot-1") + if got := err == nil && png == nil && pic.Uploads[PictureTarget("a1", "appdev", 1)] == url; got != tt.wantAddress { + t.Errorf("shot-1 = %+v, %q, %v; want its address kept without its bytes: %v", pic, png, err, tt.wantAddress) + } + if _, statErr := os.Stat(filepath.Join(stateDir, "pictures", "shot-1.png")); !os.IsNotExist(statErr) { + t.Errorf("shot-1's file is still there (%v)", statErr) + } + if _, _, err := KeptPicture(stateDir, "shot-2"); !errors.Is(err, ErrPictureNotKept) { + t.Errorf("shot-2, never attached, is still kept (%v)", err) + } + }) + } +} + +// TestKeptPictures_ForTheChange: the pictures kept for a change are those on +// disk and those whose address is that change's; another change's address +// shows nothing on this one. +func TestKeptPictures_ForTheChange(t *testing.T) { + t.Parallel() + stateDir := t.TempDir() + for i := 1; i <= 3; i++ { if _, err := KeepPicture(stateDir, fakePNG(i), 800, 600); err != nil { t.Fatalf("KeepPicture %d: %v", i, err) } } - for _, gone := range []string{"shot-1", "shot-3", "shot-4", "shot-5"} { - if _, _, err := KeptPicture(stateDir, gone); !errors.Is(err, ErrPictureNotKept) { - t.Errorf("%s is still kept (%v)", gone, err) + if err := RecordPictureUpload(stateDir, "shot-1", PictureTarget("a1", "appdev", 3), "https://hq.example.invalid/att-1"); err != nil { + t.Fatal(err) + } + if err := os.Remove(filepath.Join(stateDir, "pictures", "shot-1.png")); err != nil { + t.Fatal(err) + } + tests := []struct { + target string + want []string + }{ + {target: PictureTarget("a1", "appdev", 3), want: []string{"shot-1", "shot-2", "shot-3"}}, + {target: PictureTarget("a1", "appdev", 9), want: []string{"shot-2", "shot-3"}}, + {target: "", want: []string{"shot-2", "shot-3"}}, + } + for _, tt := range tests { + if kept, err := KeptPictures(stateDir, tt.target); err != nil || !slices.Equal(kept, tt.want) { + t.Errorf("KeptPictures(%q) = %v (%v), want %v", tt.target, kept, err, tt.want) } - if _, err := os.Stat(filepath.Join(stateDir, "pictures", gone+".png")); !os.IsNotExist(err) { - t.Errorf("%s's file is still there (%v)", gone, err) + } +} + +// TestKeptPicture_ReadsWithoutWriting: reading a picture writes nothing — a +// store it cannot write (a full disk, a read-only mount) still answers. +func TestKeptPicture_ReadsWithoutWriting(t *testing.T) { + t.Parallel() + stateDir := t.TempDir() + if _, err := KeepPicture(stateDir, fakePNG(1), 800, 600); err != nil { + t.Fatalf("KeepPicture: %v", err) + } + dir := filepath.Join(stateDir, "pictures") + if err := os.Chmod(dir, 0o500); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(dir, 0o700) }) + if _, png, err := KeptPicture(stateDir, "shot-1"); err != nil || !bytes.Equal(png, fakePNG(1)) { + t.Errorf("KeptPicture = %q, %v; want the picture from a store it cannot write", png, err) + } + if kept, err := KeptPictures(stateDir, ""); err != nil || !slices.Equal(kept, []string{"shot-1"}) { + t.Errorf("KeptPictures = %v, %v; want [shot-1]", kept, err) + } +} + +// TestKeepPicture_EntryWithoutItsFile: a process that died between removing +// a file and writing the index leaves an entry with no picture. It is not +// kept — read, listed or counted — unless HQ serves it on the pair's current +// change, and the next prune drops it. +func TestKeepPicture_EntryWithoutItsFile(t *testing.T) { + t.Parallel() + stateDir := t.TempDir() + writePair(t, stateDir, 1, "") + for i := 1; i <= 3; i++ { + if _, err := KeepPicture(stateDir, fakePNG(i), 800, 600); err != nil { + t.Fatalf("KeepPicture %d: %v", i, err) } } - for _, kept := range []string{"shot-2", "shot-6", fmt.Sprintf("shot-%d", PictureKeep+5)} { - if _, _, err := KeptPicture(stateDir, kept); err != nil { - t.Errorf("%s is gone: %v", kept, err) + target := PictureTarget("a1", "appdev", 1) + if err := RecordPictureUpload(stateDir, "shot-2", target, "https://hq.example.invalid/att-2"); err != nil { + t.Fatalf("RecordPictureUpload: %v", err) + } + for _, id := range []string{"shot-1", "shot-2"} { + if err := os.Remove(filepath.Join(stateDir, "pictures", id+".png")); err != nil { + t.Fatal(err) + } + } + tests := []struct { + id string + kept bool + }{{"shot-1", false}, {"shot-2", true}, {"shot-3", true}} + for _, tt := range tests { + if _, _, err := KeptPicture(stateDir, tt.id); (err == nil) != tt.kept { + t.Errorf("KeptPicture(%s) = %v, want kept %v", tt.id, err, tt.kept) + } + } + if kept, err := KeptPictures(stateDir, target); err != nil || !slices.Equal(kept, []string{"shot-2", "shot-3"}) { + t.Errorf("kept %v (%v), want [shot-2 shot-3]", kept, err) + } + if _, err := KeepPicture(stateDir, fakePNG(4), 800, 600); err != nil { + t.Fatalf("KeepPicture 4: %v", err) + } + var ids []string + if err := readPictures(stateDir, func(i *pictureIndex) error { + for _, pic := range i.Pictures { + ids = append(ids, pic.ID) + } + return nil + }); err != nil { + t.Fatal(err) + } + if !slices.Equal(ids, []string{"shot-2", "shot-3", "shot-4"}) { + t.Errorf("index holds %v after a prune, want [shot-2 shot-3 shot-4]", ids) + } +} + +// TestKeepPicture_CountsTheBytesOnDisk: the byte bound counts each picture's +// file as it is on disk — a size the index records, from an older store or a +// file changed since, counts for nothing. +func TestKeepPicture_CountsTheBytesOnDisk(t *testing.T) { + t.Parallel() + tests := []struct { + name string + fileSize int // shot-1's file as it is on disk + recorded string // shot-1's index entry, as an older store wrote it + wantKept []string + }{ + {name: "a file larger than its entry says", fileSize: 4096, recorded: `"bytes":10`, wantKept: []string{"shot-2"}}, + {name: "an entry with no size and a small file", fileSize: 10, recorded: `"width":800`, wantKept: []string{"shot-1", "shot-2"}}, + {name: "an entry claiming more than its file", fileSize: 10, recorded: `"bytes":999999`, wantKept: []string{"shot-1", "shot-2"}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + stateDir := t.TempDir() + if err := os.MkdirAll(filepath.Join(stateDir, "pictures"), 0o700); err != nil { + t.Fatal(err) + } + index := `{"next":2,"pictures":[{"id":"shot-1","takenAt":"2026-10-01T09:00:00Z",` + tt.recorded + `}]}` + if err := os.WriteFile(filepath.Join(stateDir, "pictures", "index.json"), []byte(index), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(stateDir, "pictures", "shot-1.png"), bytes.Repeat([]byte("x"), tt.fileSize), 0o600); err != nil { + t.Fatal(err) + } + bounds := pictureBounds{keepFor: 48 * time.Hour, maxBytes: 1024} + if _, err := keepPicture(stateDir, fakePNG(2), 800, 600, time.Date(2026, 10, 1, 10, 0, 0, 0, time.UTC), bounds); err != nil { + t.Fatalf("keepPicture: %v", err) + } + if kept, err := KeptPictures(stateDir, ""); err != nil || !slices.Equal(kept, tt.wantKept) { + t.Errorf("kept %v (%v), want %v", kept, err, tt.wantKept) + } + }) + } +} + +// TestKeepPicture_UnreadablePairRecords: a pair record that cannot be parsed +// is skipped — its words could never go on anyway — and the prune goes on +// for the rest; only a pairs' directory that cannot be read at all stops +// it, since what a kept description names is unknown then. +func TestKeepPicture_UnreadablePairRecords(t *testing.T) { + t.Parallel() + tests := []struct { + name string + spoil func(t *testing.T, servicesDir string) + wantKept []string + }{ + { + name: "an unparseable record", + spoil: func(t *testing.T, dir string) { + t.Helper() + if err := os.WriteFile(filepath.Join(dir, "webdev.json"), []byte("{not json"), 0o600); err != nil { + t.Fatal(err) + } + }, + wantKept: []string{"shot-1", "shot-3"}, + }, + { + name: "a directory that cannot be read", + spoil: func(t *testing.T, dir string) { + t.Helper() + if err := os.Chmod(dir, 0o000); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(dir, 0o755) }) + }, + wantKept: []string{"shot-1", "shot-2", "shot-3"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if os.Geteuid() == 0 { + t.Skip("root reads any directory") + } + stateDir := t.TempDir() + writePair(t, stateDir, 1, "![The count](shot-1)") + tt.spoil(t, filepath.Join(stateDir, "services")) + bounds := pictureBounds{keepFor: 48 * time.Hour, maxBytes: 1} + for i := 1; i <= 3; i++ { + if _, err := keepPicture(stateDir, fakePNG(i), 800, 600, time.Date(2026, 10, 1, 9, 0, 0, 0, time.UTC), bounds); err != nil { + t.Fatalf("keep picture %d: %v", i, err) + } + } + if kept, err := KeptPictures(stateDir, ""); err != nil || !slices.Equal(kept, tt.wantKept) { + t.Errorf("kept %v (%v), want %v", kept, err, tt.wantKept) + } + }) + } +} + +// TestKeepPicture_OneStoreForEveryProcess: the Mate's conversation, its +// helpers and their subagents each run their own zcp, all on one state dir; +// pictures they take at once share one store and one count — every id once, +// every picture kept. +func TestKeepPicture_OneStoreForEveryProcess(t *testing.T) { + t.Parallel() + stateDir := t.TempDir() + const takers, each = 6, 5 + var wg sync.WaitGroup + ids := make(chan string, takers*each) + for range takers { + wg.Go(func() { + for i := range each { + pic, err := KeepPicture(stateDir, fakePNG(i), 800, 600) + if err != nil { + t.Errorf("KeepPicture: %v", err) + return + } + ids <- pic.ID + } + }) + } + wg.Wait() + close(ids) + seen := map[string]bool{} + for id := range ids { + if seen[id] { + t.Errorf("%s given twice", id) } + seen[id] = true } - next, err := KeepPicture(stateDir, fakePNG(99), 800, 600) - if err != nil || next.ID != fmt.Sprintf("shot-%d", PictureKeep+6) { - t.Errorf("the next picture is %q (%v), want a new id", next.ID, err) + kept, err := KeptPictures(stateDir, "") + if err != nil || len(kept) != takers*each || len(seen) != takers*each { + t.Errorf("kept %d pictures of %d given (%v), want all %d", len(kept), len(seen), err, takers*each) } } diff --git a/internal/workflow/service_meta.go b/internal/workflow/service_meta.go index b5c15ad10..ec4d8ad44 100644 --- a/internal/workflow/service_meta.go +++ b/internal/workflow/service_meta.go @@ -708,33 +708,45 @@ func ReadServiceMeta(baseDir, hostname string) (*ServiceMeta, error) { // ListServiceMetas reads all service metadata files from baseDir/services/. // Returns an empty slice if the directory does not exist or is empty. func ListServiceMetas(baseDir string) ([]*ServiceMeta, error) { - dir := filepath.Join(baseDir, "services") - entries, err := os.ReadDir(dir) + dir, names, err := serviceMetaFiles(baseDir) if err != nil { - if errors.Is(err, fs.ErrNotExist) { - return nil, nil - } - return nil, fmt.Errorf("list services dir: %w", err) + return nil, err } - var metas []*ServiceMeta - for _, entry := range entries { - if entry.IsDir() || filepath.Ext(entry.Name()) != ".json" { - continue - } - data, readErr := os.ReadFile(filepath.Join(dir, entry.Name())) + for _, name := range names { + data, readErr := os.ReadFile(filepath.Join(dir, name)) if readErr != nil { - return nil, fmt.Errorf("read service meta %s: %w", entry.Name(), readErr) + return nil, fmt.Errorf("read service meta %s: %w", name, readErr) } meta, unmarshalErr := parseMeta(data) if unmarshalErr != nil { - return nil, fmt.Errorf("unmarshal service meta %s: %w", entry.Name(), unmarshalErr) + return nil, fmt.Errorf("unmarshal service meta %s: %w", name, unmarshalErr) } metas = append(metas, meta) } return metas, nil } +// serviceMetaFiles is the services directory and the name of every pair +// record in it; none when the directory does not exist yet. +func serviceMetaFiles(baseDir string) (string, []string, error) { + dir := filepath.Join(baseDir, "services") + entries, err := os.ReadDir(dir) + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + return dir, nil, nil + } + return dir, nil, fmt.Errorf("list services dir: %w", err) + } + var names []string + for _, entry := range entries { + if !entry.IsDir() && filepath.Ext(entry.Name()) == ".json" { + names = append(names, entry.Name()) + } + } + return dir, names, nil +} + // PruneServiceMetas removes service meta files that don't match any live // hostname. A container-mode meta is kept if its Hostname OR StageHostname // exists in liveHostnames.