ADR-400: implement queue_post.py - #184
Conversation
build-and-test: Python test resultsStatus: ✅ Passed Test log |
jodavis-claude
left a comment
jodavis-claude
left a comment
There was a problem hiding this comment.
First-pass review of ADR-400 (queue_post.py). All 7 explicit exit criteria are met and the 15 pytest tests pass. Found one correctness/fault-tolerance gap on the write path (see inline comment) that I'd like addressed before sign-off; everything else looked solid (hand-written YAML round-trips correctly through merge_config.parse_yaml, id-uniqueness verified under 300 rapid invocations, DEV_TEAM_STATE_DIR override honored, no shell-injection risk since content is always read from a file path).
|
Review posted: 1 correctness/fault-tolerance issue requested to be addressed (unhandled OSError on the mkdir/write path in queue_post.py — see inline review comment). All 7 exit criteria are otherwise met and the 15 pytest tests pass. |
|
Review complete: changes requested. Priority 2 issue (correctness): The Suggested fix: Wrap these operations in |
Fixes PR #184 review comment: mkdir(parents=True) and write_text() were outside the try/except that already covered the content-file read, so a failure there (permission denied, disk full, a stale file blocking directory creation) crashed with an unhandled traceback instead of the documented Error: .../exit-1 contract. Now raises ValueError, caught by main()'s existing except ValueError.
jodavis-claude
left a comment
jodavis-claude
left a comment
There was a problem hiding this comment.
Sign-off review — ADR-400
Prior thread (1 of 1): The Priority 2 correctness/fault-tolerance issue — posts_dir.mkdir(...) and record_path.write_text(...) sitting outside the function's try/except OSError block — is resolved. Both calls are now wrapped in try/except OSError, re-raised as ValueError with the record path in the message, matching main()'s existing except ValueError → Error: .../exit-1 contract. Two new tests (test_queue_post_raises_value_error_when_pending_posts_dir_cannot_be_created, test_queue_post_raises_value_error_when_record_write_fails) reproduce the reviewer's exact repro and a write failure, respectively — both confirmed failing before the fix and passing after. Full suite: 17/17 passing, verified locally.
New issues in the modified files: none found. The fix is minimal, scoped exactly to the reported gap, and doesn't introduce any new correctness, security, performance, or documentation concerns.
All exit criteria are met. Sign-off: approved.
|
Sign-off passed with no unresolved issues. Handing off to code review. |
Fixes PR #184 review comment: mkdir(parents=True) and write_text() were outside the try/except that already covered the content-file read, so a failure there (permission denied, disk full, a stale file blocking directory creation) crashed with an unhandled traceback instead of the documented Error: .../exit-1 contract. Now raises ValueError, caught by main()'s existing except ValueError.
f943864 to
bc15413
Compare
Writes one pending-post YAML record (target, channel, content, event_context, created_at) to a repo-slug-keyed pending-posts directory under $DEV_TEAM_STATE_DIR/~/.dev-team. Content is always read from --content-file, never inline, so arbitrary comment text never passes through a shell command string. Hand-written YAML output matches merge_config.parse_yaml's literal block-scalar (|) format so records remain parseable by the repo's only existing YAML reader.
Fixes PR #184 review comment: mkdir(parents=True) and write_text() were outside the try/except that already covered the content-file read, so a failure there (permission denied, disk full, a stale file blocking directory creation) crashed with an unhandled traceback instead of the documented Error: .../exit-1 contract. Now raises ValueError, caught by main()'s existing except ValueError.
bc15413 to
40aecd8
Compare
| from pathlib import Path | ||
|
|
||
| _WORKFLOW_ORCHESTRATE_SCRIPTS = ( | ||
| Path(__file__).resolve().parent.parent.parent / "workflow-orchestrate" / "scripts" |
There was a problem hiding this comment.
Can we put this "queue_post" script into its own skill, so that other skills can simply invoke that skill rather than hard-coding the path to this run-hook-instructions location? I'm kind of bothered by the number of shared scripts that exist in workflow-orchestrate too, but that's outside the scope of this change. Let's not make the problem worse.
| parser.add_argument( | ||
| "--content-file", required=True, help="Path to a file containing the post content" | ||
| ) |
There was a problem hiding this comment.
Could content be read from stdin, so it doesn't have to be written to a temp file just to be copied to another file?
| lines = content.split("\n") | ||
| if lines and lines[-1] == "": | ||
| lines = lines[:-1] | ||
| body = "\n".join(f" {line}" if line else "" for line in lines) |
There was a problem hiding this comment.
Can literal block scalars contain blank lines without ending the value?
| if lines and lines[-1] == "": | ||
| lines = lines[:-1] | ||
| body = "\n".join(f" {line}" if line else "" for line in lines) | ||
| return f"content: |\n{body}" if body else "content: |" |
There was a problem hiding this comment.
Could the body content contain other YAML-like content that could terminate the literal block early, that we should escape here?
3 participants
Writes one pending-post YAML record (target, channel, content, event_context, created_at) to a repo-slug-keyed pending-posts directory under $DEV_TEAM_STATE_DIR/~/.dev-team. Content is always read from --content-file, never inline, so arbitrary comment text never passes through a shell command string. Hand-written YAML output matches merge_config.parse_yaml's literal block-scalar (|) format so records remain parseable by the repo's only existing YAML reader.
Stack created with GitHub Stacks CLI • Give Feedback 💬