[issue-3077][slice-4/6] Gateway artifact-read endpoint (strict... - #3143
Conversation
… + sandbox) Land the strict, spec-driven artifact-read endpoint and its sandbox helper so reviewers can fetch a peer's coordination artifact by spec-registered name + commit SHA — no shared object store, no prompt-prose fetch instructions, no raw path on the wire. * TASK-4-1: orchestrator/routes/artifacts.py — new POST /api/v1/artifacts/get route. Resolves the path via egg_contracts.artifact_spec.resolve_artifact_path (slice-2), identifier via routes.pipelines._pipeline_identifier (lazy import, same convention propose-time validation uses), worktree via contract_store.resolve_pipeline_worktree with a fallback to the main repo, then runs ``git show <ref>:<path>`` against the authoritative repo (no shared-object-store assumption). Output is capped at 256 KiB with the ``truncated`` flag, mirroring the event_prompt git-log cap. STRICT per HITL Q2: the schema has no ``path`` field; unknown name → 400 listing registered names; non-hex ref → 400; ``invalid object name`` / ``unknown revision`` → 422; path-absent-at-ref → 404. Subprocess timeouts and OSErrors land as 503 — never 500. Registered in orchestrator/api.py alongside the other route blueprints. * TASK-4-2: gateway/artifact_api.py — gateway blueprint modeled on contract_api.py. Session auth, role from session metadata (never the request body), pipeline_id falls back to the session id. Same wire- level path-field rejection so a misbehaving client can't reach the orchestrator's schema check, plus pre-flight non-hex-ref rejection to avoid the round-trip. Orchestrator 4xx bodies pass through verbatim (no 500-wrap); connection failures map to 502. Registered in gateway/gateway.py next to contract_bp and phase_bp. * TASK-4-3: sandbox/scripts/egg-artifact — bash CLI following the sandbox/scripts/jira conventions (GATEWAY_URL + EGG_SESSION_TOKEN fail-closed, gateway health probe, RETURN-trap tmpfile cleanup). ``egg-artifact get <name> --ref <sha> [--identifier <id>] [--repo <hint>]`` writes the served content verbatim to stdout; the ``truncated`` flag becomes a stderr notice; structured errors print ``ERROR: <message>`` plus the registered-names detail on the unknown-name 400. ``--path`` is rejected locally before the request fires. Cross-links: - #3002 (this endpoint is the blocking prerequisite for replacing the shared-object-store / per-agent-worktree coordination channel). - #3077 slice-2 (artifact_spec registry — the single source of name → path resolution this route serves). - #3077 slice-3 (the propose-time presence validator that runs the same ``git show`` against the same registry).
… contract (#3077 TASK-4-4) Adds the slice-4 tester suite for #3077: byte-equality + strict-mode rejection contract on the orchestrator's new /api/v1/artifacts/get route, and the session-auth + verbatim 4xx passthrough contract on the gateway's /api/v1/artifact/get forwarder. orchestrator/tests/test_artifact_routes.py (new): * Happy path: registered name + 40-hex ref returns the git-show stdout byte-identical; envelope carries name/ref/path/content/truncated; the spawned git command targets the worktree the route resolved (not a per-agent worktree) — pins the authoritative-repo invariant. * Qualified pipeline_id (issue-3077-replan) resolves to the qualified .egg-state/drafts/issue-3077-replan-plan.md path through routes.pipelines._pipeline_identifier; pins the slice-2 collision fix for concurrent runs on the same issue. * Non-UTF-8 blob round-trips through errors='replace' instead of 500ing — defense against a binary-ish committed artifact. * Strict rejections (HITL Q2): - unregistered name => 400 listing every registered alternative (analysis-draft, plan-draft, architect-output, architect-slices, risk-analyst-output), so egg-artifact can render a usable hint; - 8 non-hex / malformed refs (branch name, HEAD, shell metachar injection, path traversal, too-short, empty, non-hex 40-chars) => 400 without ever spawning git show — pins the pre-flight that keeps shell metachars off the wire; - absent-at-ref / unresolvable-ref => structured 4xx with success=false (404 vs 422 left to the coder, never 500); - body 'path' field => 400 + git show never invoked; - TimeoutExpired => 503 (not 500), so the gateway's 502 wrap stays reserved for "orchestrator unreachable". * Cap boundary: monkeypatch _ARTIFACT_MAX_BYTES to 16 to exercise the at-cap (truncated=false) and over-cap (truncated=true, content is exactly the cap-sized head slice) branches without materialising a ~256 KiB response. raising=True so a future rename of the constant ratchets both the test and any docs that reference it. * Schema: missing body => 400; each required field individually required and the rejection message names the missing field; PipelineNotFoundError from get_state_store_for_pipeline => 4xx. A composite _ArtifactRouteSeams context manager patches the three lazy seams the route uses — routes.get_state_store_for_pipeline, contract_store.resolve_pipeline_worktree, and routes.artifacts.subprocess.run — so test bodies read as one-liner spec declarations and the lazy-import contract is not broken. gateway/tests/test_artifact_api.py (new): * Forwarding: POST /api/v1/artifact/get reaches /api/v1/artifacts/get on the orchestrator with X-Egg-Role set from the session (reviewer_code -> reviewer) — NEVER from the request body, the same anti-forgery rule contract_api.mutate_contract enforces. * No path field: body-level 'path' is rejected at the gateway BEFORE forwarding; urlopen is asserted never called so the malicious body never lands in the orchestrator audit log. * Session auth: unauthenticated request => 401/403 and the forwarder is never invoked — tighter than test_contract_api.py. * Local schema: missing 'ref' => 400 from the gateway, no upstream round-trip; saves orchestrator load for missing-field cases. * 4xx passthrough: orchestrator strict-mode bodies for unregistered name (with registered-names list), non-hex ref, and absent-at-ref are forwarded verbatim — egg-artifact reads .message directly to print the user-facing stderr line, so re-encoding would silently drop the registered-names hint. * Orchestrator unreachable => 502 (distinguishes "down" from "absent"). * truncated=true passes through unchanged to the sandbox helper. * URL-prefix ratchet: /api/v1/artifact/get is the only registered URL on the gateway — the plural /api/v1/artifacts/get returns Flask 404 (a future "let's align the prefixes" rewrite has to confront this ratchet first). gateway/tests/conftest.py: * Pre-load gateway/artifact_api.py through the existing _load_module_with_replaced_imports bootstrap, guarded with Path.exists() so the conftest stays runnable on tester branches where the producer hasn't landed yet (BRC parallel mode). Threads the 'from .artifact_api import' replacement into the gateway module loader so gateway.py's blueprint registration resolves against the pre-loaded module. Note on test execution: the sandbox cannot run pytest (pip install fails on uv -> pygments TLS verification, same as slice-3's notes), so checks were run as ``ruff check`` + ``ruff format --check`` only. Reviewers run the suite in their own environment via the consensus_wrapper merge of the slice-4 coder + tester branches. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
Autofix tracking{"Lint/Python": 1, "Test/Unit Tests": 1} |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…ision
Python's logging module reserves 'name' (the logger name) as a LogRecord
attribute. Passing it via extra={} raises KeyError on _log/makeRecord,
which 500'd every artifact_get hit and broke 5 test_artifact_routes
happy-path / cap-boundary tests.
This comment has been minimized.
This comment has been minimized.
![james-in-a-box[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
There was a problem hiding this comment.
Contract Verification — PR #3143 (issue-3077 slice-4/6)
Verdict: Approve (contract-compliant). Posted as a comment (self-authored PR). All four tasks are fully implemented and every acceptance criterion is objectively satisfied by code + tests. Two minor, non-blocking advisory notes at the end.
Note on criterion marking: the orchestrator is unreachable from this session, so
egg-contract verify-criterioncannot persist. The contract's top-levelacceptance_criteriaarray is also empty ([]) — criteria are stored as task-level strings, so there are noac-Nidentifiers to mark. Verification is recorded per-task below.
TASK-4-1 — Orchestrator artifact-read route (orchestrator/routes/artifacts.py) ✅
- Byte-identical content + truncated flag —
_decode_with_capreturns the blob unchanged below_ARTIFACT_MAX_BYTES(256 KiB) and a capped head slice withtruncated: trueabove it. Pinned bytest_happy_path_byte_equalityand the twoTestArtifactGetCapboundary tests (at-cap → false, over-cap → true). - Strict rejections — unregistered
name→ 400 withregistered_nameslist (spec_by_nameKeyError, line 386-393); non-hexref→ 400 via_HEX_REF_REbefore any git invocation (line 379); absent-at-ref → 404, unresolvable-ref → 422 (_run_git_showclassifies git stderr). Timeout/OSError → 503, never 500. All covered byTestArtifactGetRejections(incl. shell-metachar/traversal refs) +test_subprocess_timeout_returns_503_not_500. - No path field (schema-level) —
if "path" in body→ 400 at line 360, asserted bytest_path_field_is_rejected_400. - No shared object store — server-side
git show -C <worktree>only (list-form, no shell); worktree resolved viaresolve_pipeline_worktreewith a main-repo fallback. The happy-path test pins the authoritative-repo invariant by asserting thegit -C <worktree> showargv. - #3002 cross-linked in the module docstring. ✓
TASK-4-2 — Gateway blueprint (gateway/artifact_api.py, gateway/gateway.py) ✅
- Authenticated fetch + response parity —
@require_session_auth, forwards to orchestrator/api/v1/artifacts/getvia_proxy_post, relays the payload verbatim (test_forwards_to_orchestrator_with_role_header). - Role from session, never body —
_role_from_contextreads session metadata (X-Egg-Role only behindEGG_ENABLE_TEST_ROLE_HEADER), the request body'sroleis never consulted; forwarded asX-Egg-Role. Path field is stripped at the gateway before forwarding (test_forwarded_body_strips_path_fieldasserts urlopen is never called). - Auth rejection parity —
test_requires_session_authconfirms unauthenticated requests are rejected and the forwarder is never invoked. - 4xx passthrough, no 500-wrap —
_proxy_postforwardsHTTPError.code+ body verbatim; only true connection failure maps to 502. Pinned bytest_orchestrator_4xx_relayed_verbatimandtest_orchestrator_unreachable_502. Blueprint registered alongsidecontract_bp/phase_bp(gateway.py:508-517). ✓
TASK-4-3 — Sandbox helper (sandbox/scripts/egg-artifact) ✅
get <name> --ref <sha> [--identifier <id>] [--repo <hint>]writescontentto stdout withend=''(newline-preserving), exits 0 on success.- Gateway 4xx → clean
ERROR: <message>on stderr (+registered names:line for unknown-name), non-zero exit, no stack trace;truncated→ explicit stderr notice. - No git invocation — served read only;
--pathrejected locally before the request fires. Fail-closed on missingGATEWAY_URL/EGG_SESSION_TOKEN+ health probe, matchingsandbox/scripts/jiraconventions. ✓
TASK-4-4 — Endpoint tests ✅
orchestrator/tests/test_artifact_routes.py(16 tests) andgateway/tests/test_artifact_api.py(11 tests) cover every strict-rejection branch on both sides, byte-equality on the happy path, and cap behavior at/over the boundary. Gateway tests assert role-from-session, path-strip-before-forward, verbatim 4xx passthrough, 502-on-unreachable, and the/api/v1/artifact/get(singular) vs orchestrator/api/v1/artifacts/get(plural) URL ratchet. ✓
Cross-cutting checks
- Logger-collision fix (final commit d852982) is complete: the reserved LogRecord key
namewas renamed toartifact_namein theextra={}dict (line 430). The remaining keys (pipeline_id,ref,path,bytes,truncated,spec_phase,spec_producer) do not collide with any reserved LogRecord attribute. - No injection surface:
refis hex-validated andrel_pathis spec-derived (resolve_artifact_path);git showuses list-form argv with no shell. - No orphaned code / scope creep: all changes map to the four tasks (the two
.egg-state/brc-history/*files are pipeline-generated, and thetest_consensus_wrapper.pydelta is the automated-formatting fixup commit).
Non-blocking advisory notes
orchestrator/routes/artifacts.pydecodes git-show output witherrors="replace", so a genuinely non-UTF-8 blob is not byte-identical (U+FFFD substitution). This is a documented, deliberate tradeoff and correct for the registered artifacts, which are all markdown/JSON text drafts — flagging only so a future binary artifact type doesn't silently inherit lossy reads.sandbox/scripts/egg-artifact: underset -euo pipefail, a hard curl failure onhttp_code=$(curl …)would exit viaset -ebefore the explicitif [ $curl_exit -ne 0 ]handler runs, making that branch partly unreachable. The preceding gateway health probe covers the common case, andcurl -sreturns 0 for HTTP 4xx (routed through the Python parser), so the user-facing behavior is correct; the dead-ish branch is cosmetic.
Neither note blocks merge.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Review: PR #3143 — Gateway artifact-read endpoint (#3077 slice-4)
Thorough pass over all 9 code files (the .egg-state/brc-history/* artifacts are skipped per review conventions). No blocking issues. The strict "no-path" design is enforced at three layers and the error taxonomy is correct. A few non-blocking suggestions below.
What I verified
Security / strict-resolution (the core invariant) — holds.
refis gated by^[0-9a-fA-F]{7,40}$on both the gateway (artifact_api.py:99,260) and orchestrator (artifacts.py:104,379) before any subprocess runs. This blocks shell metacharacters, symbolic refs (HEAD, branch names), and traversal-style refs.subprocess.runuses list-form argv with no shell, so even a hypothetical bad ref can't inject.- The forbidden
pathfield is rejected at the gateway wire boundary (artifact_api.py:247) and re-rejected at the orchestrator (artifacts.py:360) — defense in depth, and the orchestrator rejection lists registered names. - Role is taken from the session (
_role_from_context), never the request body — matchescontract_api.py. The body'sroleis never consulted. - I empirically confirmed
git show <ref>:<path>does not normalize..(it treats..as a literal, non-existent tree entry), so the agent-controlledidentifierinterpolated into the path template cannot traverse out of.egg-state/drafts//.egg-state/agent-outputs/. The strict guarantee survives even the identifier vector.
Correctness / robustness — correct.
- Error taxonomy is right: path-absent → 404, unresolvable ref → 422, timeout/OSError → 503, and crucially never a 500 (which the gateway would 502-wrap as "unreachable" — wrong category).
test_subprocess_timeout_returns_503_not_500andtest_absent_at_ref_returns_structured_4xx_not_500pin this. - Non-UTF-8 blobs decode with
errors="replace"so a binary-ish artifact can't 500 the route (_decode_with_cap,artifacts.py:327). - Cap boundary is a clean
<=(intact,truncated:false) vs>(head slice,truncated:true), re-decoded at the byte boundary. Pinned at the boundary by the cap tests. - Worktree resolution prefers the shared pipeline worktree (same seam slice-3's propose-time validator reads from) and falls back to the main repo post-pruning. End-to-end path is consistent with the established slice-3
git showmechanism, so the served read is functional in its normal path. - The
name→artifact_namelogger-extra rename (final commit) correctly avoids theLogRecordcollision.
Tests — exercise the production path.
- Both suites drive the real Flask blueprints; seams (
subprocess.run,get_state_store_for_pipeline,resolve_pipeline_worktree,urlopen) are mocked but the route/forwarder logic runs. The happy-path byte-equality blob is hand-authored, not regenerated from the implementation — not a self-seeding golden. Givengit initis blocked in the sandbox, mockinggit showoutput is the right call. test_consensus_wrapper.pychanges are pure auto-formatting (line reflow), no logic change.
Non-blocking suggestions
-
egg-artifact: thecurl_exiterror branch is unreachable underset -euo pipefail. Atsandbox/scripts/egg-artifact:121,http_code=$(curl ...)is a plain assignment; when curl fails at the connection level (exit 7/28),set -eexits the script before line 129–134, so the "Failed to connect to gateway" stderr message never prints — the user gets a silent non-zero exit. The health check pre-empts most cases, but a transient mid-request drop hits this. Considerhttp_code=$(curl ...) || curl_exit=$?(or a localset +ewindow) so the diagnostic actually surfaces. The non-zero exit contract still holds; only the message is lost. -
identifieris interpolated into the git path without a shape check (defense-in-depth). Git's non-normalization of..neutralizes traversal today, so this is not a vulnerability — but the explicitidentifierstill lets a caller influence the resolved path (e.g. a subdirectory) despite the deliberately strict no-path design. A cheap schema-level guard on the explicit-identifier branch (digits, or anissue-…-shaped safe charset; reject/and..) would make the strict guarantee hold at the wire rather than relying ongit show's pathspec semantics.artifacts.py:158-165is the spot. -
Gateway forwards
X-Egg-Rolebut the orchestrator route ignores it. Unlikecontracts.py(which role-validates mutations via_role_from_request),artifacts.get_artifactnever reads the role — reads are intentionally not role-gated. The forwarding is harmless future-proofing, but a one-line comment on the orchestrator side noting "reads are not role-gated; role forwarded for audit/future use" would prevent a future reader from assuming the consumer_roles allow-list is enforced here.
Solid, well-documented slice. Ship-ready modulo the suggestions above (all non-blocking).
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
… test_consensus_wrapper.py Took slice-3's deletion of the unused 'repo, _ = ...' assignment in _run_harness (dead code; repo is never used in the function) over slice-4's automated single-line reformatting of the same lines.
Conflict Resolution SummaryResolved merge conflicts with
DetailThe single conflict was in
Since the line is genuinely unused, slice-3's deletion is the cleaner, semantically-correct resolution and supersedes a pure-formatting change of dead code. No functional behavior is lost. Verification
Please review: the resolution removes one line of unused test scaffolding — low risk, but confirm you're comfortable that — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: PR #3143 — merge of slice-3 into slice-4 (#3077 slice-4)
Re-reviewed the delta since my prior review at d85298206. The only new work is the merge commit f3cbf922 (slice-3 → slice-4) and its single conflict resolution. No new blocking issues; the resolution is correct.
Verified
Slice-4 feature code is untouched by the merge. git diff d85298206..HEAD shows zero changes to gateway/artifact_api.py, orchestrator/routes/artifacts.py, orchestrator/routes/artifacts.py tests, sandbox/scripts/egg-artifact, or gateway/tests/test_artifact_api.py — byte-identical to what I reviewed before. The merge only pulled in slice-3 base content plus one conflict fix, so my earlier comment-verdict assessment of the feature still stands.
Conflict resolution is correct (confirming the author's ask). The sole conflict was in _run_harness (test_consensus_wrapper.py). I independently confirmed the deleted line —
repo, _ = (None, None) if not (tmp_path / "repo").exists() else (tmp_path / "repo", None)
— was dead: _run_harness uses str(tmp_path / "repo") directly and never references the repo local (the repo in _init_repo is a separate scope, unaffected). Taking slice-3's deletion over slice-4's reformat-of-dead-code loses no behavior.
The del monkeypatch change is internally consistent. I checked the apparent contradiction — the resolved _script drops monkeypatch.setenv("EGG_BRC_EVENT_PUMP", "true") while ~30 sibling tests still set it. TestEventPumpTemplateSelection pins that production build_consensus_wrapped_command no longer reads the flag (it's inert post task-4-2). So the dropped setenv was a no-op, correctly removed; the remaining setenv calls are harmless no-ops, several of which exist precisely to assert inertness.
Banner-wording update applied consistently. Slice-3's reworded "use the rendered git log / git show fallback commands" banner landed in both the production SYNC_FAILURE_BANNERS append sites (consensus_wrapper.py:553,571) and the test assertions/regex that pin them (test_consensus_wrapper.py:1555-1601) — no code/test drift.
No conflict markers anywhere in the changed tree.
Carried-over non-blocking suggestions (unchanged, not addressed by this merge)
These were advisory in my prior review and remain so — none block merge:
egg-artifact:121—curl_exitdiagnostic branch is unreachable underset -euo pipefail; non-zero exit contract still holds, only the message is lost.artifacts.py:158-165— a shape check on the explicitidentifierwould harden the strict no-path guarantee at the wire (git's..non-normalization neutralizes it today; not a vuln).artifacts.get_artifact— a one-line comment noting reads are intentionally not role-gated (role forwarded for audit/future use).
Clean merge. Ship-ready modulo the long-standing non-blocking notes.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Contract Re-Verification — PR #3143 (slice-4/6)
Verdict: Approve (clean re-review). Incremental verification against my prior review at d85298206a9b17474d0a2aa1fb8286c1acefd57a. The only PR-authored change since then is the merge commit f3cbf922 ("Merge origin/egg/issue-3077/slice-3 into slice-4"). No slice-4 feature regression; all previously-verified acceptance criteria still hold.
What changed since last review
git log d852..HEAD --not origin/egg/issue-3077/slice-3 yields exactly one commit: the slice-3→slice-4 merge. The diff splits cleanly:
- vs slice-3 parent (
71f3872d): the slice-4 feature files appear as pure additions (gateway/artifact_api.py,gateway/gateway.py,orchestrator/routes/artifacts.py,orchestrator/api.py,sandbox/scripts/egg-artifact, and both test files) — the merge preserved slice-4's work intact. - vs slice-4 parent (
d852): only slice-3's own independent changes landed (event_prompt.py,pipelines.py,signals.py,coordination-state.md,REVIEWER-SYNC.md, slice-3 tests), plus the documented conflict resolution.
Verification of the merge
- slice-4 feature files are byte-identical to d852 —
git diff d852..HEADreports 0 changed lines for every TASK-4-1…4-4 deliverable. The implementation I verified previously (orchestrator artifact route, gateway blueprint,egg-artifacthelper, endpoint tests) is unchanged, so AC for task-4-1 through task-4-4 remain satisfied. - Conflict resolution is sound —
orchestrator/consensus_wrapper.pyresolved to slice-3's version (identical to parent71f3872d), differing from slice-4 only in non-functional WARNING-banner prose (slice-3's text is more accurate: "use the renderedgit log/git showfallback commands" vs "thegit showcommands below"). Matches the commit message's stated intent (took slice-3's deletion of the unusedrepo, _ = …over slice-4's reformatting). test_artifact_spec.pyrefactor came wholesale from slice-3 (merge result identical to slice-3 parent) — delegates theall_specsfixture toegg_contracts.artifact_spec.all_specs(registered_specs). Verified the symbol exists (shared/egg_contracts/artifact_spec.py:188), alongsidespec_by_name,specs_for,resolve_artifact_path— all imports resolve.- No regressions to slice-4 dependencies —
_get_draft_pathstill present (orchestrator/routes/pipelines.py:5250); slice-3'spipelines.pychange only swaps bare filenames forresolve_artifact_pathspec-name lookups (the slice-2 single-source-of-truth ratchet), which does not affect the artifact-read endpoint. - No leftover conflict markers anywhere in the merged delta.
Note on criterion marking
The orchestrator was unreachable during this review (egg-orch health → Status: UNREACHABLE; gateway healthy), so egg-contract verify-criterion could not write. Because the slice-4 feature is byte-identical to the state I verified at d852, no criteria changed status and no re-marking is required — the prior verifications stand. If the orchestrator's contract needs the verifications re-applied, they can be replayed once it is reachable; no code change is needed.
No contract violations found. The merge is mechanical and correctly preserves the slice-4 contract.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- egg-artifact: capture curl exit with '|| curl_exit=$?' so a hard connection failure surfaces the diagnostic instead of being swallowed by set -e before the error branch runs. - artifacts route: add a defense-in-depth shape check on the explicit 'identifier' (safe slug, no '/' or '..') so the strict no-path guarantee holds at the wire rather than relying on git-show pathspec semantics. Reject unsafe identifiers with a structured 400. - artifacts route: document that reads are intentionally not role-gated (role forwarded for audit/future use). - Add parametrized test covering unsafe-identifier rejection.
Review feedback — dispositionsThanks for the thorough passes. Both bots approved with non-blocking advisory notes; I've addressed all three actionable suggestions in-PR (commit 1. 2. 3. 4. Non-UTF-8 blobs decode with Verified locally: — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: PR #3143 — slice-4 review-feedback hardening (#3077 slice-4)
Re-reviewed the delta since my prior review at f3cbf922. The only new work is commit 89daad6 ("Address review feedback"), which addresses all three of my prior non-blocking suggestions. No blocking issues — the changes are correct.
Verified — all three suggestions correctly addressed
1. egg-artifact curl_exit unreachable branch — fixed correctly. The plain assignment http_code=$(curl …) now ends in || curl_exit=$? (sandbox/scripts/egg-artifact:130). I confirmed the root cause: with set -euo pipefail (line 24) and the local http_code declaration separate from the assignment, the assignment was a plain command whose non-zero exit (curl 7/28) tripped set -e and aborted the script before local curl_exit=$? ran — the "Failed to connect to gateway" branch was genuinely dead. The || curl_exit=$? short-circuits set -e and captures curl's real exit; on success curl_exit stays 0. No pipefail interaction (no pipe in the curl line). Correct fix.
2. Explicit-identifier shape check — correct, exercises the production path. _SAFE_IDENTIFIER_RE = ^(?!.*\.\.)[A-Za-z0-9][A-Za-z0-9._-]*$ (artifacts.py:115) runs in _resolve_identifier before the .isdigit() coercion and before any subprocess. I verified the regex against every test case and a few of my own:
-
All 8 rejection cases (
../../../etc/passwd,..,issue/../3077,a/b,issue 3077,issue;rm,"",.hidden) → rejected. -
Valid ids (
3077,issue-3077,plan.draft) → accepted. -
Ordering bonus: because the regex (ASCII-only charset) runs first, the
str.isdigit()Unicode-digit gotcha ('٣'.isdigit() == True) can never reachint()— non-ASCII fails the charset.The new
test_unsafe_explicit_identifier_400(8 parametrized cases) drives the real wire path (client.post→get_artifact→_resolve_identifier) and assertssubprocess_mockis not called — it exercises production code, not a hand-built fixture. Single caller of_resolve_identifier(the route), so the early-return is self-contained.
3. Role-gating comment — accurate. The new comment at get_artifact (artifacts.py:375) correctly documents that reads are intentionally not role-gated; X-Egg-Role is forwarded for audit only and the route never consults it. Matches actual behavior (no role read anywhere in the handler).
Non-blocking (one new minor nit)
artifacts.py:175— usere.fullmatch(or\Z) instead ofre.match. Python's$matches before a single trailing newline, so_SAFE_IDENTIFIER_RE.match("issue-3077\n")and…match("3077\n")both succeed. The impact is benign — a trailing-newline identifier still can't introduce..or/(multi-lineabc\n..\nis correctly rejected because.doesn't cross newlines), so the worst case is a non-existent interpolated path → truthful 404. Since this whole check is defense-in-depth, tightening it tofullmatch/\Zcloses the last gap at no cost. Not blocking.
Clean, well-tested follow-up. Ship-ready modulo the optional fullmatch tightening.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Contract Re-Verification — PR #3143 (slice-4)
Verdict: Approve (no new contract violations; the delta strengthens previously-verified criteria).
I previously verified this PR at f3cbf922. After fetching and deepening, the only PR-authored change since then is a single commit:
89daad6c— Address review feedback: harden egg-artifact + identifier shape check
git log f3cbf922..HEAD --not origin/egg/issue-3077/slice-3 -p shows three files touched, all mapping cleanly to existing slice-4 tasks. No new tasks introduced; the change is additive hardening.
Delta review
1. orchestrator/routes/artifacts.py (task-4-1) — identifier shape check + role-gate doc
- New
_SAFE_IDENTIFIER_RE = ^(?!.*\.\.)[A-Za-z0-9][A-Za-z0-9._-]*$rejects any explicitidentifiercontaining..,/, whitespace, or shell metachars with a structured 400, applied in_resolve_identifier(artifacts.py:175) beforeresolve_artifact_path(artifacts.py:437) interpolates it into the path template. Correctly placed — the rejection happens at the wire, before any subprocess. - This is pure defense-in-depth for the "No request field accepts a repo path (schema-level)" criterion (task-4-1): it makes the strict no-path guarantee hold independent of
git showpathspec semantics. It does not weaken any prior behavior — valid identifiers (3077,issue-3077) still match and the all-digit→int coercion is preserved. - Added a docstring on
get_artifactdocumenting that reads are intentionally not role-gated (X-Egg-Roleforwarded for audit/future use only). Accurate to the code — the route never consults the role, consistent with the slice design where the no-path boundary is the access control.
2. sandbox/scripts/egg-artifact (task-4-3) — curl exit capture
http_code=$(curl …) || curl_exit=$?replaces the priorlocal curl_exit=$?that followed the assignment. This fixes a real bug: underset -e, a hard connection failure (curl exit 7/28) made thehttp_code=$(…)assignment abort the script before the diagnostic branch ran, silently losing the "Failed to connect to gateway" message. The|| curl_exit=$?idiom is correct and the error branch (if [ "$curl_exit" -ne 0 ]) now reliably fires.- Does not regress the "helper performs no git invocation" criterion (no git added) and improves the "gateway 4xx surfaces as a clear stderr message" criterion.
3. orchestrator/tests/test_artifact_routes.py (task-4-4) — unsafe-identifier coverage
- New
test_unsafe_explicit_identifier_400parametrized over 8 cases (../../../etc/passwd,..,issue/../3077,a/b,issue 3077,issue;rm,"",.hidden). I verified each against the regex: all are correctly rejected (traversal///whitespace/metachar fail the charset;..fails the lookahead; empty and leading-dot fail the alnum-start anchor). - Asserts both the 400 status and
not seams.subprocess_mock.called, matching the existing pre-subprocess rejection-test pattern (e.g. thepath-field and non-hex-ref rejection tests). Strengthens the "All strict-resolution rejection branches covered" criterion (task-4-4).
Regression check
No previously-verified criterion is broken. The delta is strictly additive: a tighter input guard, a shell-robustness fix, and matching test coverage. All require-changes-level concerns from the prior cycle appear addressed.
CI
All required checks green on HEAD 89daad6c (Unit Tests, Python, Shell, Security Scan, Integration Tests, Lint — all SUCCESS). Per review conventions I did not re-run the suite.
⚠️ Note on criterion marking
The orchestrator was unreachable for the duration of this review (egg-contract show / verify-criterion and egg-orch health both report Orchestrator: UNREACHABLE; gateway is healthy). I was therefore unable to run egg-contract verify-criterion --criterion ac-N to persist the verification state, and the only local contract snapshot is the stale refine-phase parent (issue-3077.json) whose top-level acceptance_criteria is empty. The verification verdict above stands on the code/test evidence; the criterion-marking step should be re-run (by me on the next event, or by an operator) once the orchestrator is reachable.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Review feedback — dispositionsThanks for the re-review. One new actionable nit since the last cycle; addressed in-PR. 1. All other prior suggestions (curl_exit branch, identifier shape check, role-gating comment) were already addressed in commit Verified locally: — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: PR #3143 — slice-4 (#3077), trailing-newline tightening
Re-reviewed the delta since my prior review. The PR branch was rebased (my prior commit 89daad6 is no longer reachable), but the sole PR-authored change since that review is commit e88b47a ("Tighten identifier check to re.fullmatch"). No blocking issues — the change is correct and complete.
Verified — the one prior nit is correctly addressed
artifacts.py:175 — re.match → re.fullmatch. This closes exactly the gap I flagged: Python's $ matches before a single trailing newline, so the old _SAFE_IDENTIFIER_RE.match("issue-3077\n") and …match("3077\n") both spuriously succeeded. I verified the behavioral difference directly against the live regex ^(?!.*\.\.)[A-Za-z0-9][A-Za-z0-9._-]*$:
| input | .match() (old) |
.fullmatch() (new) |
|---|---|---|
"issue-3077\n" |
accepted | rejected |
"3077\n" |
accepted | rejected |
"issue-3077", "3077" |
accepted | accepted |
"../../../etc/passwd", "..", "a/b", ".hidden", "" |
rejected | rejected |
fullmatch requires the whole string to satisfy the charset, and \n isn't in [A-Za-z0-9._-], so the trailing-newline forms now fail. All valid/invalid cases are otherwise unchanged. The retained ^/$ anchors are redundant under fullmatch but harmless and self-documenting — fine to leave.
As before, the real-world impact of the old gap was benign (a trailing-newline identifier still couldn't introduce .. or /, so the worst case was a truthful 404 on a non-existent path), so this is pure defense-in-depth tightening at no cost.
Test coverage — exercises the production path. test_unsafe_explicit_identifier_400 gained the two new cases ("issue-3077\n", "3077\n") at test_artifact_routes.py:535-536. The test drives the real wire path (_post → get_artifact → _resolve_identifier), asserts 400, asserts the message names identifier, and asserts not seams.subprocess_mock.called — i.e. the rejection fires before any git show. Not a hand-built fixture; it goes through the production handler. These two cases would have failed under the old .match() (confirmed above), so they genuinely pin the regression.
Everything else
Unchanged from the prior cycle's approval. The curl_exit fix, the identifier shape check, and the role-gating doc remain as previously verified; the non-UTF-8 errors="replace" item stands as a documented, deliberate tradeoff.
Clean, minimal, correctly-tested follow-up. Ship-ready.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Contract re-verification — incremental (delta since 89daad6c)
Re-reviewed the single delta commit since my last verification:
e88b47af — "Tighten identifier check to re.fullmatch (close trailing-newline gap)."
Delta scope
Only two reviewable files changed (everything else is .egg-state/ pipeline state):
orchestrator/routes/artifacts.py—_SAFE_IDENTIFIER_RE.match(...)→.fullmatch(...)in_resolve_identifier(line 175).orchestrator/tests/test_artifact_routes.py— two new parametrized rejection cases ("issue-3077\n","3077\n").
Assessment: compliant, strengthens existing criteria
The regex is ^(?!.*\.\.)[A-Za-z0-9][A-Za-z0-9._-]*$ (artifacts.py:115). In Python $ also matches just before a trailing newline, so re.match would have accepted "issue-3077\n" — a trailing-newline bypass of the strict identifier guard added in the previous review-feedback round. re.fullmatch requires the entire string to be consumed; \n is outside the character class, so the malformed identifier is now correctly rejected with a structured 400.
This is a pure hardening of task-4-1's "No request field accepts a repo path (schema-level)" / strict-validation acceptance criterion — it does not change the happy path (isdigit() coercion and the safe-slug acceptance are unaffected) and breaks none of the criteria verified at 89daad6c. No regressions across TASK-4-1…4-4.
Verification evidence
Ran the affected targeted test (full make test deliberately not run, per review conventions):
orchestrator/tests/test_artifact_routes.py::TestArtifactGetRejections::test_unsafe_explicit_identifier_400
10 passed in 1.04s
All ten cases pass, including the two new trailing-newline cases.
Note on criterion marking
The orchestrator is currently UNREACHABLE (egg-orch health → orchestrator UNREACHABLE; gateway ok), so egg-contract verify-criterion could not be run this cycle. The delta introduces no new criteria and no previously-verified criterion is invalidated, so no criterion-state change is owed beyond what was recorded at 89daad6c. The mark can be (re)applied once the orchestrator is reachable.
Verdict: contract-compliant. The delta is a correct, well-tested security hardening with no contract violations.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
| egg is addressing review feedback... |
Review feedback — dispositionsThe two latest reviews (egg-reviewer and james-in-a-box contract-verification) are both against the current HEAD 1. 2. 3. 4. 5. Non-UTF-8 blobs decode with No code change was needed this cycle — the current HEAD already incorporates every fix and both fresh reviews confirm it. — Authored by egg |
|
egg feedback addressed. View run logs 18 previous review(s) hidden. |
…3152) * docs: mark #3077 slices 2-4 shipped in coordination-state doc Slices 2 (#3141), 3 (#3142), and 4 (#3143) have all merged since the coordination-state doc was last updated (it still read "as of slice-1"). Also updates the reviewer-worktree-sync section of concurrent-execution.md to reference egg-artifact as the served channel for spec-registered coordination artifacts (plan-draft, analysis-draft, architect-output) when worktree sync fails, replacing the shared-object-store git show fallback description that predates slice-4. * docs: mark #3077 slices 5-6 shipped in coordination-state All six slices of the #3077 epic have landed on main. Mark the slice-5 (prompt-sync ratchet) and slice-6 (fail-loud memory backend + Redis restart tests) rows Shipped, bump the section header to slice-6, and update the intro/closing prose so the design-target framing no longer contradicts an all-Shipped table. --------- Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
1 participant
POST /api/v1/artifact/get serves committed artifact content by spec-registered name + hex-validated ref via git show on the authoritative repo (gateway blueprint forwarding to a new orchestrator route), with a sandbox-side helper. Unblocks #3002.
Base PR: #3139
What's in this PR
Commits (3):
This slice
Gateway artifact-read endpoint (strict, name-resolving) + sandbox helper
Files affected:
orchestrator/routes/artifacts.pygateway/artifact_api.pygateway/gateway.pysandbox/scripts/egg-artifactorchestrator/tests/test_artifact_routes.pygateway/tests/test_artifact_api.pyTasks (4) + acceptance criteria
orchestrator/routes/artifacts.py(NEW), registered per the existingorchestrator/routes/convention. Request: artifactname(spec-registered),ref(hex-validated commit SHA), and the issue/pipeline identifier. Resolves the path viaartifact_spec.resolve_artifact_pathand returns the content ofgit show <ref>:<path>from the authoritative repo, applying the existing output caps with atruncatedflag. STRICT per HITL Q2: the schema has NO path field; an unregistered name returns 400 listing registered names; a non-hex ref returns 400; an unresolvable ref or path-absent-at-ref returns a structured 4xx, never a 500. Cross-link Deploy egg on GKE (minimal first deploy): Terraform-provisioned cluster + GKE overlay + registry pull path #3002 in the module docstring.truncated: true. - Unregistered name → 400 listing registered names; non-hex ref → 400; absent-at-ref → structured 4xx. - No request field accepts a repo path (schema-level). - Works with no shared object store between agent worktree and orchestrator repo (server-side git show only).gateway/artifact_api.py(NEW) implementingPOST /api/v1/artifact/get, modeled ongateway/contract_api.py— session auth, role taken from the session (never the request body), forward to the orchestrator route from TASK-4-1, propagate structured 4xx errors and thetruncatedflag verbatim. Wire the blueprint into the gateway app ingateway/gateway.pyalongside the contract API registration.sandbox/scripts/egg-artifact(NEW): a thin CLI wrappingPOST /api/v1/artifact/getfollowing the existing sandbox script conventions (gateway base URL/session wiring as insandbox/scripts/jira), withegg-artifact get <name> --ref <sha> [--identifier <id>]semantics: content to stdout, structured errors to stderr, non-zero exit on failure, an explicit notice whentruncatedis set. This is the single verb prompt templates reference after slice 5 deletes path prose.egg-artifact get plan-draft --ref <sha>prints served content and exits 0. - Gateway 4xx surfaces as a clear stderr message and non-zero exit (no stack trace); truncated responses print a notice. - The helper performs no git invocation (served read only).orchestrator/tests/test_artifact_routes.py(happy path byte-equality, unregistered-name 400 listing registered names, non-hex ref 400, absent-at-ref structured 4xx, cap + truncated flag at the boundary) and newgateway/tests/test_artifact_api.pyfollowing thetest_contract_api.pystyle (session auth, forwarding, 4xx passthrough, no path field accepted).Stack
issue-3077egg/issue-3077/slice-3