Validate drive-memory disk-size syntax at compile time and suggest 100M - #54831
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot normalize lowercases inputs to the upper case format required by actions, time white space |
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully! Pre-fetched diff (pr-diff.patch) only contains two generated/lock files: .github/workflows/agentic_commands.yml (regenerated slash-routing table) and .github/workflows/smoke-drive.lock.yml (recompiled lock file). Per ponytail-review skill boundaries, generated files and lock files are out of scope. The actual hand-written source changes for this PR (pkg/workflow/drive_memory_config.go, schema, docs, tests) are not present in the capped diff, so there is no reviewable content for an over-engineering pass.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #54831 does not have the 'implementation' label and has only 32 new lines of code in business logic directories (threshold: 100).
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
COMMENT — I don't see a blocking bug in the changed lines.
Notes
I checked the new schema regex, the parser-side fallback validation, the smoke workflow update, and the added tests. The implementation is internally consistent: uppercase-only suffixes are documented, enforced in schema and Go, and covered by tests; bare numeric sizes still remain accepted as intended.
The only open question I found is product behavior rather than a code defect: the PR discussion asks for normalizing lowercase/trimmed input, but this change intentionally rejects those forms instead of normalizing them. If that behavior is desired, it needs a follow-up design change rather than a review blocker here.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 6.34 AIC · ⌖ 8.74 AIC · ⊞ 4.6K
Comment /review to run again
There was a problem hiding this comment.
Pull request overview
Adds compile-time validation for drive-memory disk sizes and updates the recommended smoke-test size.
Changes:
- Enforces uppercase
K/M/G/Tdisk-size syntax. - Adds tests and drive-size documentation.
- Regenerates smoke workflow outputs and command routing.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/schemas/github-workflow.json |
Reorders the Drives permission. |
pkg/workflow/drive_memory_test.go |
Tests disk-size parsing. |
pkg/workflow/drive_memory_config.go |
Adds parser-side validation. |
pkg/parser/schemas/main_workflow_schema.json |
Constrains disk-size syntax. |
docs/src/content/docs/reference/frontmatter-full.md |
Regenerates schema reference. |
docs/src/content/docs/reference/drive-memory.md |
Documents drive sizing. |
.github/workflows/smoke-drive.md |
Uses the suggested 100M size. |
.github/workflows/smoke-drive.lock.yml |
Regenerates compiled workflow. |
.github/workflows/agentic_commands.yml |
Adds smoke-drive command routing. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 5
- Review effort level: Balanced
| entry.Description = description | ||
| } | ||
| if diskSize, ok := raw["disk-size"].(string); ok { | ||
| if diskSize != "" && !driveDiskSizePattern.MatchString(diskSize) { |
| // defaultDriveMemoryDiskSize is the suggested drive size when creating a new drive. | ||
| defaultDriveMemoryDiskSize = "100M" |
|
|
||
| ## Drive size | ||
|
|
||
| `disk-size` sets the size used when creating a new drive; it is ignored for an existing drive. The value must be a number with an optional `K`, `M`, `G`, or `T` suffix (for example `100M`). Suffixes such as `1GB` are rejected at compile time. A drive of `100M` is enough for typical memory files: |
| # Drive size used when creating the drive, as a number with an optional K, M, G, | ||
| # or T suffix (suggested: '100M'; ignored for an existing drive) |
| name: "disk size with invalid suffix", | ||
| raw: map[string]any{"disk-size": "1GB"}, | ||
| wantErr: "invalid drive-memory disk-size", |
There was a problem hiding this comment.
Review: harden + audit
The changes are clean and well-targeted: compile-time validation of disk-size syntax using a regex, updated defaults (1GB → 100M), docs, schema descriptions, and test coverage.
🟡 Non-blocking suggestion
Zero-byte disk size passes validation — see inline comment on drive_memory_config.go line 152. The regex ^[0-9]+[KMGT]?$ admits "0" and "0M", which are syntactically valid but semantically meaningless (and will fail at runtime). A one-character fix ([1-9][0-9]*) closes this.
✅ What looks good
- Regex is anchored (
^...$), which prevents partial matches. - Error message includes the rejected value and an example of a valid one — actionable for users.
- Lowercase suffix test case (
100m) correctly rejected. - Schema
patternfield mirrors the runtime regex — consistent. - Documentation prose accurately describes the constraint.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 28.5 AIC · ⌖ 9.12 AIC · ⊞ 6.2K
Comments that could not be inline-anchored
pkg/workflow/drive_memory_config.go:152
The pattern ^[0-9]+[KMGT]?$ accepts "0", "0K", "0M", etc. — zero-byte disk sizes that pass compile-time validation but would fail at runtime when the drive is created.
Consider tightening to require at least one non-zero leading digit:
var driveDiskSizePattern = regexp.MustCompile(`^[1-9][0-9]*[KMGT]?$`)This rejects meaningless zero sizes at compile time without changing the accepted valid-size set.
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnosing-bugs — approving with two minor suggestions.
📋 Key Themes & Highlights
Key Themes
- Test coverage gap: happy-path tests for individual valid suffixes (
K,M,G,T) are absent — only a bare number is tested as valid. A future regex typo in[KMGT]would go undetected. - Empty-string guard: the
diskSize != ""check inside the validation branch means an explicit empty string is stored silently instead of being treated as absent or rejected.
Positive Highlights
- ✅ Dual-layer defence: schema pattern + parser-side check catches invalid values regardless of how the config reaches the compiler.
- ✅ Clear, actionable error message with an embedded example (
100M). - ✅ Smoke workflow updated consistently in both the markdown and its compiled lock file.
- ✅ Docs section added with a minimal working example — good discoverability for users hitting the new error.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 30.2 AIC · ⌖ 9.9 AIC · ⊞ 7.6K
Comment /matt to run again
| wantErr: "invalid drive-memory disk-size", | ||
| }, | ||
| } | ||
|
|
There was a problem hiding this comment.
[/tdd] The new tests cover rejection cases, but there's no happy-path test for each valid suffix (K, M, G, T). Only a bare number ("500") is tested as valid, so a future regex typo in [KMGT] could break all suffix cases silently.
💡 Suggested additions
{name: "disk size K suffix", raw: map[string]any{"disk-size": "512K"}, wantDrives: 1},
{name: "disk size M suffix", raw: map[string]any{"disk-size": "100M"}, wantDrives: 1},
{name: "disk size G suffix", raw: map[string]any{"disk-size": "10G"}, wantDrives: 1},
{name: "disk size T suffix", raw: map[string]any{"disk-size": "1T"}, wantDrives: 1},This makes the test suite a living spec for all four accepted suffixes.
@copilot please address this.
| entry.Description = description | ||
| } | ||
| if diskSize, ok := raw["disk-size"].(string); ok { | ||
| if diskSize != "" && !driveDiskSizePattern.MatchString(diskSize) { |
There was a problem hiding this comment.
[/diagnosing-bugs] The diskSize != "" guard means an explicitly empty string passes validation and gets stored. An empty disk-size: "" in YAML is almost certainly a misconfiguration; it would be cleaner to either reject it or treat it as absent.
💡 Suggested fix
if diskSize, ok := raw["disk-size"].(string); ok && diskSize != "" {
if !driveDiskSizePattern.MatchString(diskSize) {
return entry, fmt.Errorf(...)
}
entry.DiskSize = diskSize
}This keeps the empty-string path from silently setting a blank disk-size in the emitted YAML.
@copilot please address this.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in a827b72's follow-up (7b5e3e0): |
|
🎉 This pull request is included in a new release. Release: |
The Smoke Drive job failed post-cleanup with
Invalid disk size: '1GB'. Expected a number with optional K/M/G/T suffix (e.g. '10G')—tools.drive-memory.disk-sizewas accepted as any non-empty string, so invalid values only surfaced at runtime after the agent had already run.Changes
pkg/parser/schemas/main_workflow_schema.json):disk-sizenow constrained bypattern: ^[0-9]+[KMGT]?$(both the object and array forms); description suggests100Minstead of10G.pkg/workflow/drive_memory_config.go): parser-side check with an explicit error, so the failure is reported even when a workflow bypasses schema validation..github/workflows/smoke-drive.mdusesdisk-size: 100M; lock file recompiled.frontmatter-full.mdregenerated from the schema.1GB, and rejected lowercase100m.Compiling an invalid value now fails early with a line reference:
Suffixes are uppercase-only, matching the error text emitted by the GitHub Drives checkout action; lowercase (
100m) is rejected rather than silently normalized.