Skip to content

fix(rosettify): validate plan names on writes - #285

Merged
isolomatov-gd merged 2 commits into
griddynamics:mainfrom
shin4141:codex/fix-225-plan-name-validation
Aug 19, 2026
Merged

fix(rosettify): validate plan names on writes#285
isolomatov-gd merged 2 commits into
griddynamics:mainfrom
shin4141:codex/fix-225-plan-name-validation

Conversation

@shin4141

Copy link
Copy Markdown
Contributor

Summary

  • route the shared plan write validation through the existing validatePlanName helper
  • reject empty and whitespace-only names on both create and upsert paths
  • preserve the existing Unnamed Plan default when a name is omitted
  • add regression coverage for the validator and both write paths

Why

validatePlanName already enforced the required non-empty plan name, but the create and upsert paths only called validateSizeLimits. Its truthy length guard allowed empty or whitespace-only names to be written. Reusing the existing helper in the shared validation path keeps the name rules in one place.

Validation

  • targeted: 3 files, 114 tests passed
  • npm run typecheck
  • npm test — 60 files, 1,572 tests passed
  • git diff --check

Fixes #225

Checklist

  • Scope is narrow and explicit
  • Existing omitted-name behavior is preserved
  • New behavior has regression coverage
  • Local component validation passes
  • Commit includes the required DCO sign-off

AI assistance

AI assistance was used to inspect the issue, implement the focused change, and run validation. I reviewed every changed line.

Signed-off-by: Shin <128954611+shin4141@users.noreply.github.com>
@isolomatov-gd

isolomatov-gd commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Thanks @shin4141.

To fix:

  • Guard non-string names: typeof name !== "string".
  • Add a test for name: 123 and name: {}.

validatePlanName does !name || !name.trim(). For name: 123, [], or {} the !name check is false and .trim() does not exist, so it throws. Plan.name is typed string, so TypeScript cannot catch it, and the payload is user-controlled JSON. Result is {"error":"internal_error: name.trim is not a function"} — a leaked JS message. On main these inputs silently persisted "name": 123, so this path is newly reachable through your change.

Not yours, no action needed: size_limit_exceeded is an odd code for an empty name, but that lives at core.ts:309 and main already asserts it — you only made it reachable. And update_status still skips name validation on both main and this branch.

What checks out — the real risk, and it held:

  • No data loss. Planted plan files with name empty / absent / whitespace / 123 / [], then upserted an unrelated field across three target kinds, plus a 20-way concurrent burst of rejected writes. Original file always intact at its original path, zero .bakNNN, zero stranded .lock. That is structural: doc-io.ts runs the mutator at :221 before the guard rename at :237, and releases the lock in finally.
  • All four write paths reject, on both frontends — create, upsert, create-with-template, upsert-with-template × CLI and --mcp stdio, driven for real.
  • "Unnamed Plan" survives: omitted and null default correctly, "" rejects and creates no file.
  • 256 accepted / 257 rejected unchanged. Mutation test kills 6 tests across 3 files, so they are not tautological.
  • 1572/1572 pass; coverage improves (branches 90.54% → 90.67%).

No version bump needed from you — the companion PR carries rosettify 3.2.2.

Signed-off-by: Shin <128954611+shin4141@users.noreply.github.com>
@shin4141

Copy link
Copy Markdown
Contributor Author

#285

Thanks for flagging this. I added the non-string guard before .trim() and regression coverage for name: 123 and name: {} across the validator and create/upsert paths. Both now return size_limit_exceeded instead of internal_error. Focused tests, typecheck, build, and the full suite—60 files / 1,578 tests—pass in 522eda7. Ready for re-review.

@isolomatov-gd isolomatov-gd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @shin4141 — both items done, and the guard is confirmed by execution across three trees: at your pre-fix commit 123/{}/[] returned internal_error: name.trim is not a function; on main they silently persisted name=123; on your head all three return size_limit_exceeded on both the CLI and --mcp, across all four write paths, with no leftover file and no .bakNNN. null and omitted still give "Unnamed Plan".

typeof name !== "string" is the right layer here — core.ts:389-397 already uses exactly that defensive idiom on parsed JSON, and there is no runtime schema layer to move it into. Widening the signature to unknown is correct for a runtime guard. Your tests assert the specific error code plus the absence of a written file, which is the right shape.

Two citations in my earlier comment were wrong, though the substance stood: the error-code enumeration is FR-PLAN-0021, not FR-PLAN-0022 (which is "No-Args Shows Help"), and internal_error is in fact documented, in FR-SHRD-0004. Sorry for the misdirection.

1578 tests pass, branches 90.67% against the 90% gate. No version bump needed from you — this branch's companion PR carries rosettify 3.2.2.

Approved.

@isolomatov-gd
isolomatov-gd merged commit 788d906 into griddynamics:main Aug 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ROSETTA] rosettify validatePlanName unused; actually-used validateSizeLimits skips empty/whitespace name check

2 participants