feat: add Stooq archive conversion command

[rustyeddy]

Adds trader data convert SPY D1 for native Stooq ZIP archives.

The command:

• extracts the matching native .txt member into a temporary directory;
• imports managed monthly raw partitions through the existing Stooq path;
• builds canonical data through Manager Plan/Build;
• removes the temporary extraction, leaving the ZIP as the immutable source.

Includes ZIP extraction tests and full-suite validation.

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved validation, output handling, API-boundary, and completeness issues remain.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 · 1

Open (3)

• Dataset format flag is ignored · New
• Manager accesses provider name before configuration guard · New
• Missing end-to-end test for registered conversion command · New

What changed in this PR

Adds trader data convert to extract Stooq ZIP archives, import raw data, and build canonical datasets.

Changes:

• Adds conversion request/response and service orchestration.
• Adds Manager integration and CLI ZIP extraction.
• Registers the command and adds extraction tests.

File	Summary
internal/​service/​marketdata/​response.go	Adds conversion response types.
internal/​service/​marketdata/​request.go	Adds conversion request inputs; identity validation remains needed.
internal/​service/​marketdata/​convert.go	Orchestrates import and build; interval and completeness validation remain needed.
internal/​marketdata/​convert.go	Adds Stooq import integration; boundary and configuration handling need revision.
cmd/​trader/​data/​convert.go	Implements conversion CLI; format handling, interval normalization, and skipped-action reporting need fixes.
cmd/​trader/​data/​convert_test.go	Tests ZIP extraction; command-level integration coverage is still needed.
cmd/​trader/​data/​command.go	Registers the conversion command.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[rustyeddy]

Reviewed PR #404 at head f89dafeb7465ef0dda2d8eaeb90605905693068c.

This is the right dedicated home for the Stooq conversion work, but I would not merge it yet. I agree with Copilot's three findings and found two additional scope/data-integrity concerns.

1. Blocker: DatasetRequest.Instrument and ConvertRequest.Symbol can disagree

This is the same identity bug I previously called out when these changes were mixed into #406.

Service.Convert imports raw data under req.Symbol, then calls Build using req.DatasetRequest.Instrument.

Those are independent inputs today.

A caller can therefore construct:

Instrument = SPY
Symbol     = QQQ

and import one instrument while asking Trader to build another.

Please eliminate the second source of truth.

Preferred fix: derive the provider/raw symbol from the already-resolved instrument/listing through Manager/Resolver. If that is awkward at this layer, validate exact identity equivalence before any raw partition is written.

This should be treated as a correctness blocker because this path mutates the managed raw archive.

2. Agree: Manager must use the normal configured guard first

ImportStooqArchive reads m.providerName before checking m.configured().

That breaks the zero-value/nil-manager contract the rest of Manager explicitly documents and tests.

Put the standard configuration guard first and return wrapped ErrInvalidConfig consistently.

3. Agree: do not expose --format if convert does not honor it

addDatasetArgFlags gives this command --format, but convert always prints one human-readable line.

Either:

• route the response through the normal formatter, or
• use a narrower flag set for convert and do not advertise --format.

Silently accepting --format json and returning text is the worse option.

4. Agree: add a real command/service vertical-slice test

The current tests only prove ZIP-member extraction.

For this command, we should exercise the actual path:

ZIP
 -> command
 -> instrument registration
 -> Convert service
 -> managed raw monthly partitions
 -> Manager Plan/Build
 -> canonical publish
 -> output

At minimum verify:

• source ZIP hash is unchanged;
• expected raw partitions appear;
• expected canonical partitions are published;
• resulting bars are readable through Manager;
• temporary extraction is gone afterward;
• invalid provider/interval/range fail clearly.

This PR introduces a mutating operator workflow, so the end-to-end contract matters more than the extraction helper itself.

5. Remove the duplicate batch helper before merge

The PR currently contains both:

bin/stooq-convert-all
tmp/stooqs2canonical.sh

They are effectively the same bulk-conversion script.

We should not merge two copies of the same operational logic.

Given #407 now defines the intended ergonomic stq2bars direction, my preference is to keep #404 focused on the general-purpose trader data convert service/CLI and remove the batch helpers from this PR entirely. Then implement the convenient operator wrapper deliberately in #407.

That keeps the layering clean:

#404  general primitive:
      trader data convert ...

#407  ergonomic wrapper:
      stq2bars SPY

6. Do not bulk-promote Stooq directory names into listing identity yet

The helper scripts currently encode:

"nyse etfs"   -> ARCA / ETF
"nasdaq etfs" -> NASDAQ / ETF
"nyse stocks" -> NYSE / equity
...

That may be correct for some files, but it is a provider-directory heuristic being turned into canonical listing identity.

Before using this across the entire Stooq universe, we need either:

• a verified/documented mapping, or
• Trader's instrument registry/resolver to own the listing metadata.

This is another reason I would leave the bulk scripts out of #404 and handle the ergonomic/discovery policy in #407.

Conclusion

The core idea of #404 is good: make native Stooq ZIP -> managed raw -> canonical build a first-class Trader operation.

Before merge I would require:

1. remove/validate the independent Symbol identity;
2. add Manager configuration guard;
3. fix/remove the ignored format flag;
4. add service/command end-to-end coverage;
5. remove the duplicate batch helpers from this PR.

After that, #404 becomes a clean low-level conversion primitive that #407 can wrap with the simple stq2bars UX.

[rustyeddy]

Addressed the review findings in the latest commit: conversion now derives the Stooq symbol from the resolved instrument listing, Manager checks its configured guard first, convert no longer advertises the unsupported --format flag, added command/service vertical coverage, and removed both batch helper scripts so #407 can own that UX. Full tests and targeted lint pass.

[rustyeddy]

Re-reviewed PR #404 at head 65f8814b801a9a440e56dcd72573ec4d00f038b5.

The recent changes address essentially all of my previous review:

• removed the independent ConvertRequest.Symbol identity;
• Manager.ImportStooqArchive now resolves the provider symbol from the registered instrument.ID;
• the standard m.configured() guard now comes first;
• convert no longer inherits the unsupported --format flag;
• the bulk conversion scripts were removed, leaving #404 as the low-level conversion primitive for #407 to wrap;
• a service-level conversion test was added;
• a command-level ZIP -> raw -> canonical integration test was added;
• the command integration test verifies the source ZIP remains byte-for-byte unchanged.

That is a much cleaner shape.

I found one remaining test correctness issue:

TestConvertRejectsNonDailyIntervalBeforeImport does not actually prove the interval guard

The test builds:

DatasetRequest{
    Interval: marketdata.H1,
    Range: span,
}

but leaves Instrument zero.

Service.Convert begins with:

if err := req.Validate(); err != nil {
    return ConvertResponse{}, err
}

and DatasetRequest.Validate() checks the instrument before the interval.

So this test currently passes because the instrument is zero, not because H1 is rejected by:

if req.Interval != marketdata.D1 { ... }

That means the newly added D1-only guard could be accidentally removed and this test would still stay green.

Please give this test a valid registered instrument (or otherwise construct a fully valid DatasetRequest except for H1) and assert the returned error/message specifically reflects the D1-only conversion restriction.

A useful assertion would be both:

require.ErrorIs(t, err, svc.ErrInvalidRequest)
require.ErrorContains(t, err, "supports only D1")

Once that is corrected, I do not see another substantive blocker in the current diff.

The command integration test could eventually be strengthened to read the resulting bars back through Manager rather than only checking the canonical file, but given that the service test already exercises the Manager Build path and this PR is a narrow conversion primitive, I would not hold #404 solely for that.

After fixing the ineffective non-D1 test, I expect PR #404 to be merge-ready.

[rustyeddy]

Fixed the remaining test issue in commit 6d448c3: the H1 test now supplies a valid ETF instrument and asserts the specific supports only D1 error, so it genuinely exercises the conversion interval guard.

[rustyeddy]

Final review of PR #404 at head 6d448c3d979f3a72419f12c8be45e0d4e5bfc303.

The last remaining test issue is fixed correctly.

TestConvertRejectsNonDailyIntervalBeforeImport now supplies a non-zero instrument ID and explicitly asserts:

require.ErrorIs(t, err, svc.ErrInvalidRequest)
require.ErrorContains(t, err, "supports only D1")

so the test genuinely exercises the D1-only conversion guard instead of failing earlier on request validation.

I re-checked the rest of the diff as well:

• single source of truth for instrument identity;
• provider symbol derived through Resolver;
• Manager configured guard is first;
• no misleading --format support;
• service test covers import + canonical build;
• command integration test covers ZIP -> raw -> canonical and source immutability;
• bulk-conversion scripts are no longer part of this PR;
• scope is cleanly the low-level Stooq conversion primitive that #407 can wrap ergonomically.

I found no new substantive issues.

PR #404 is merge-ready from my review.

GitHub currently reports no combined-status entries for this head, so this conclusion is based on the code/diff and test coverage visible in the PR rather than an external CI status signal.