fix: [good first issue] Add a sample archive generator for demos and onboarding - #18
webbrain-one wants to merge 1 commit into
Conversation
Introduce `tools/scripts/sample_archive.py` to create small synthetic archives with realistic metadata and placeholder media. Includes validation checks, corresponding tests, and CLI integration to streamline user onboarding and demo workflows. Closes atomize-lab#3
atomize-lab
left a comment
atomize-lab
left a comment
There was a problem hiding this comment.
Maintainer review
Thanks for the contribution. I reviewed commit 6354ea2 in an isolated worktree. This needs changes before merge because the current implementation does not pass its own tests and misses required safety/CLI/documentation acceptance criteria.
Blocking results
python -m pytest tests/test_sample_archive.py -v: 2 failed. The generator invokestweet_validate.pywith theaccounts/directory as one tweet directory, so validation returnsE001(missing tweet.json). Use the validator's recursive--rootmode.- Full suite: 242 passed, 2 failed (the same two sample tests).
python tools/citeseal.py lintpasses. python tools/citeseal.py sample --help: exits 2 (invalid choice). The PR does not add the required unifiedsamplesubcommand.- The PR changes only the generator and its test; the required README quickstart update is absent.
- Reusing an output path overwrites an existing deterministic
tweet.jsonbefore the command fails. A controlled sentinel changed and was not preserved. - If
<output>/accountsis an existing symlink, generation writes outside the resolved output root. A controlled probe confirmed the escaped file was created. --count 10creates malformed paths/datetimes such as202608010_...and2026-08-010T...; non-positive counts also need an explicit policy.
Please make generation fail safely before writing: preserve unrelated/existing archive data by default, enforce resolved-path containment for every generated path, validate the count/date range, wire cs sample, update the README, and add regression tests for collision/non-destruction, symlink/traversal containment, invalid counts, and unified CLI registration. Then rerun the targeted suite, full suite, CLI lint, and a manual generate + recursive validate smoke test.
| validate_target = out / "accounts" | ||
| print(f"Validating generated archive at {validate_target}...") | ||
| result = subprocess.run( | ||
| [sys.executable, str(validate_script), str(validate_target)], |
There was a problem hiding this comment.
🔴 Blocking correctness issue: tweet_validate.py treats a positional path as one tweet directory. Here accounts/ has no tweet.json, so both new tests fail with E001. Invoke the recursive interface (--root, followed by this path) and keep a test that asserts the validator actually inspected the generated item directories.
| tweet_json["media"] = [{"file": "01.png", "type": "image", "alt_text": "placeholder blue square"}] | ||
| tweet_json["components"] = ["text", "images"] | ||
|
|
||
| tweet_json_path = item_dir / "tweet.json" |
There was a problem hiding this comment.
🔴 Blocking data-safety issue: this deterministically overwrites an existing tweet.json when --output is reused. I pre-created the first generated path with a sentinel; the command exited 1 but the sentinel was replaced. Refuse collisions/non-empty archive targets before any writes (or use an explicit ownership-aware policy), and add a regression test proving existing data remains byte-for-byte unchanged.
| handle = "demo_user" | ||
| year = "2026" | ||
| month = "2026-08" | ||
| base_dir = out / "accounts" / handle / "tweets" / year / month |
There was a problem hiding this comment.
🔴 Blocking path-containment issue: joining beneath out is not sufficient when an existing component is a symlink. With <output>/accounts symlinked to an external directory, the generator created the tweet tree outside the requested output root. Resolve the output root and every destination, reject symlink/traversal escapes before writing, and add a containment regression test.
|
|
||
| for i in range(count): | ||
| day = i + 1 | ||
| ts = f"2026080{day}_100000" |
There was a problem hiding this comment.
--count 10 creates 202608010_... and datetime_utc=2026-08-010T...; zero/negative counts currently generate nothing and can appear successful once root validation is fixed. Either validate a documented range or generate dates with a real date type, with boundary tests.
atomize-lab
added
enhancement
|
Nightly maintainer follow-up (2026-08-20): this PR is still at I re-ran the exact Head in an isolated worktree using the repository
Please push a new commit to this same PR addressing the existing blocking review: recursive validation invocation, unified |
|
Nightly maintainer CI update (2026-08-21): after reviewing the exact Head The remote matrix has now completed and confirms the local result:
CI run: https://github.com/atomize-lab/citeseal/actions/runs/31636935868 The existing CHANGES_REQUESTED review remains authoritative. Please push fixes to this PR; maintainers will rerun the full acceptance and safety gates on the new Head. |
2 participants
Closes #3