Skip to content

fix: type --max-concurrent-uploads as int, validate input and default to 5 - #344

Closed
BlueLabelscotch wants to merge 10 commits into
mainfrom
fix/har-max-concurrent-uploads-int
Closed

BlueLabelscotch wants to merge 10 commits into
mainfrom
fix/har-max-concurrent-uploads-int

Conversation

@BlueLabelscotch

Copy link
Copy Markdown
Collaborator

Fixes #337

  • Add an is_int flag type to the spec/registry so non-numeric input is rejected by the flag parser
  • Type --max-concurrent-uploads as int in har.spec.yaml (generic, maven, python)
  • Error on values < 1 instead of silently falling back
  • Align default to 5 (matches hc)

@augmentcode

augmentcode Bot commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

CLI Grammar Review · session (Harness team members only) · e1674d8

CLI grammar changes (beta) · Advisory only, FYI, not required changes.

  • 🟡 Changed harness push artifact:generic, harness push artifact:maven, harness push artifact:python: --max-concurrent-uploads (now int, default 5)
    • Default changes 4 → 5 and non-numeric or <1 values now error; may affect existing scripts. Consider noting in PR.
    • New is_int flag type is a core spec change bundled with module specs; AGENTS.md prefers core PRs land separately.
  • 🟢 Changed harness update trigger <pipeline>/<trigger>: --set, --del (mutable: name, enabled, description, tags)
  • 🟢 Changed harness update trigger:enable <pipeline>/<trigger>, harness update trigger:disable <pipeline>/<trigger>: --preview-request now provided by the core flag

Overall: 🟡 Recommend manual review

@augmentcode augmentcode Bot 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.

Deep Code Review Agent🐛

Review completed with 3 suggestions.

Reviewed commit: e1674d8

Comment thread pkg/registry/endpoint.go
contentType := resolveContentType(ep, method)
var updateBody any = body
if contentType == "application/yaml" {
out, err := yaml.Marshal(integralFloatsToInts(body))

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.

Deep Code Review Agent🐛

The new YAML update path silently changes untouched integers that cannot be represented exactly as float64: yamlParse preserves 9007199254740993, but the preceding jsonCopyMap rounds it to 9007199254740992. integralFloatsToInts cannot recover that lost precision, so even a name edit or enable/disable operation can PUT corrupted values elsewhere in the configuration.

Severity: medium


🤖 Was this useful? React with 👍 or 👎

expr: it.type
- id: enabled
expr: it.enabled
mutable_path: trigger.enabled

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.

Deep Code Review Agent🐛

Exposing enabled through the default scalar field handler makes --set enabled=false write the YAML string enabled: "false", because mutateScalar returns the operand unchanged and yaml.Marshal preserves its string type. This violates the field's boolean representation and differs from trigger:disable, whose body_params expression produces an actual boolean.

Severity: medium


🤖 Was this useful? React with 👍 or 👎

Comment thread pkg/spec/har.spec.yaml
description: "Include hidden files and directories (names starting with '.') when walking directory inputs"
- name: max-concurrent-uploads
description: "Maximum number of files uploaded concurrently (default: 4, use 1 for sequential)"
is_int: true

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.

Deep Code Review Agent🐛

This adopts the new core is_int capability in HAR in the same PR that introduces it, and the pipeline edits likewise adopt the new core YAML-update capability. Per root AGENTS.md's “PR scope: core vs module” rule, reinforced by the permanent review memory from PR #308, core features cannot be bundled with module adoption unless the module edits are required for compatibility; these edits add module behavior rather than maintain compatibility.

Severity: low


🤖 Was this useful? React with 👍 or 👎

@BlueLabelscotch
BlueLabelscotch deleted the fix/har-max-concurrent-uploads-int branch October 23, 2026 11:52
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.

HAR: --max-concurrent-uploads is untyped, wrong default (4 vs 5)

1 participant