Skip to content

feat: add AWS Bedrock provider transport - #415

Open
sairam0424 wants to merge 5 commits into
interviewstreet:mainfrom
sairam0424:feat/aws-bedrock-provider
Open

feat: add AWS Bedrock provider transport#415
sairam0424 wants to merge 5 commits into
interviewstreet:mainfrom
sairam0424:feat/aws-bedrock-provider

Conversation

@sairam0424

Copy link
Copy Markdown

Summary

  • Add BedrockConverseProvider (models.py) as a new LLM transport wired through providers.json / config.py / llm_utils.py, so any configured model can route through AWS Bedrock's Converse API (SigV4 auth via boto3's standard credential chain) alongside the existing OpenAI-compatible transport. transport defaults to "openai" when omitted, so all existing providers are unaffected.
  • Handle real observed Bedrock/Claude response quirks in the new transport: envelope-unwrapping, stringified-nested-JSON-with-stray-brace repair, and prose-vs-JSON disambiguation for structured/tool-forced output — verified against real models (Nova, Claude Sonnet 4.6, Claude Sonnet 5, and others).
  • Fix a data-integrity bug in project selection (github.py / transform.py): resolve_selected_projects no longer trusts the LLM's echoed project-selection JSON, which was silently dropping project_type and contributor_count and could re-type commit/star counts from memory. Selected projects are now re-keyed from the authoritative fetched GitHub data, keeping only the model's own reason_for_project_selection.
  • Add the repo's first automated test suite (45 tests across 3 files) covering the Bedrock provider transport/normalization logic and the project-selection resolution fix, with pytest declared as a dependency so the suite runs immediately after a clean install.

Testing

  • Full test suite: pytest -v45 passed, 0 failed, 0 skipped, 5 warnings in 0.45s. Breakdown: tests/test_bedrock_provider.py (29 tests), tests/test_github_signal_integrity.py (8 tests), tests/test_project_resolution.py (8 tests). This is the repo's entire test suite — main has zero pre-existing tests, so there is no regression risk to any prior suite. The 5 warnings are pre-existing, unrelated PyMuPDF/SWIG DeprecationWarnings.

  • Live AWS Bedrock verification across all 3 sampling-parameter regimes — real Converse API calls via boto3 (SigV4, no mocking), driven through the actual production call path (initialize_llm_provider(model).chat(...)):

    • temperature+top_p regime → amazon.nova-pro-v1:0: clean structured-output parse in 1.71s.
    • temperature-only regime → us.anthropic.claude-sonnet-4-6: clean structured-output parse in 4.22s.
    • neither param ({}) regime → us.anthropic.claude-sonnet-5: clean structured-output parse in 4.08s.
  • Project-metadata fix verified live, end-to-end: ran score.py <resume.pdf> --role software_engineering_intern through the full pipeline against real, previously-fetched GitHub data for 7 repositories. The returned evidence text cites the corrected computed signals directly rather than guessing — e.g. quoting real commit counts per repository and reasoning about contributor counts and project type — exactly the fields resolve_selected_projects now restores from the authoritative fetched record instead of trusting the LLM's own echoed (and bug-prone) selection JSON.

  • Fresh-clone install verification, cold: in an isolated git worktree, built a brand-new Python 3.11 virtualenv and ran pip install -r requirements.txt from a clean slate — exit code 0, zero errors. Then ran pytest tests/ immediately, with no manual package installation of any kind45 passed, 5 warnings in 2.74s. This confirms a first-time contributor or CI runner following only the README's own instructions can install and run the full suite, fully offline (the Bedrock tests mock the Converse client; no AWS credentials or .env file required).

Bedrock over SigV4 is not OpenAI-compatible, so it cannot be added as just
another base_url. This introduces an explicit `transport` discriminator in
providers.json alongside a BedrockConverseProvider, and keeps every existing
provider working by defaulting `transport` to "openai".

Why a new transport rather than a base_url:
  - auth is SigV4 from the AWS credential chain, not a bearer token
  - `system` is a top-level Converse parameter, not a message role
  - there is no `response_format`; structured output requires declaring a tool
    whose inputSchema.json is the caller's schema and forcing toolChoice

Both providers return {"message": {"content": ...}}, so pdf.py, evaluator.py and
github.py are untouched by the transport choice. In structured mode the tool
payload is re-serialized to a JSON string so extract_json_from_response and the
existing json.loads call sites keep working unchanged.

Two capability facts, both established against the live API rather than assumed:

  - structured_output is now resolvable per model, not only per provider.
    Claude and Nova honour a forced toolConfig; google.gemma-3-* ignores it and
    answers in prose with no error, so those models declare "none". Declaring
    capability per provider would silently degrade them.

  - Bedrock's Anthropic models reject temperature and top_p together with a
    ValidationException. pdf.py:94 read model_params["top_p"] with bracket
    access and evaluator.py:66 defaulted it to 0.9, so both forced the pair in
    and every Bedrock Anthropic call failed. Both now spread the resolved
    params, so a model that declares one sampling parameter sends one. This is
    a latent bug in the existing code that any single-parameter model would
    have hit.

Also fixes .gitignore, which matched `test_*.py` at any depth and therefore made
tests/ uncommittable. Narrowed to `/test_*.py` to preserve the original intent
of ignoring stray root-level scratch files.

Verified: 20 offline tests pass, and a full end-to-end
`score.py <resume> --role software_engineering_intern` run completes against
us.anthropic.claude-haiku-4-5-20251001-v1:0 and amazon.nova-pro-v1:0 with
schema-conforming output.
…irks

Adds claude-opus-5, claude-sonnet-5, claude-opus-4-8, claude-sonnet-4-6 and
claude-sonnet-4-5 to the bedrock provider, plus a measurement script that picks
between them on evidence instead of spec sheets.

Sampling parameters turn out to have three regimes within this one provider,
each enforced by Bedrock with a ValidationException:
  - nova / qwen / gemma        temperature + top_p
  - haiku-4-5, sonnet-4-5/4-6  temperature only
  - opus-5, sonnet-5, opus-4-8 neither; both are deprecated
Models in the third group therefore declare {} and rely on the params being
spread rather than named. A consequence worth knowing: temperature=0 is not
available on the newest models, so it cannot be used to damp score variance.

Tool-input normalization. Forcing toolChoice guarantees a toolUse block but not
a well-formed one. Two quirks were captured against the live API while scoring a
real resume, and in both the model's content was correct and only its envelope
was wrong, so repair is lossless rather than a guess:

  1. Envelope wrapping. opus-5 and sonnet-5 nest the entire object under one
     arbitrary key -- {"parameter_name": {...}}, {"response": {...}}, and the
     literal placeholder {"$PARAMETER_NAME": {...}}. Unwrapped only when that
     lone key is not itself a schema property and the nested dict does contain
     schema properties, so a genuine single-property response is untouched.

  2. Stringified nested values. haiku-4-5 emits `scores` and `bonus_points` as
     JSON strings, each with one spurious trailing brace, so json.loads rejects
     an otherwise complete object. Parsed with raw_decode and accepted only when
     the trailing remainder is pure punctuation noise; if any real content would
     be dropped the string is left as-is so validation fails loudly rather than
     silently recording a partial score.

Only strings beginning with { or [ are considered, so prose fields such as
`evidence` and `breakdown` are never reinterpreted.

Measured effect, 6 runs of one resume through the scoring call:
                        before   after
  opus-5                  1/5     6/6
  sonnet-5                1/5     4/6
  sonnet-4-6              5/5     6/6
  haiku-4-5               1/5     2/6
The first comparison run was measuring this serialization gap rather than model
reliability. haiku-4-5 remains unfit for this schema and the README says so.

29 tests, including both quirks asserted against the exact payloads captured
from the API, and a case proving real trailing content is refused.
…uesses

criteria.jinja:60 makes "all projects are self_project" a hard 10-point ceiling on
open_source and system_message.jinja:23/49 tells the model to read project_type.
github.py computes that field at the cost of one contributors call per repository.
It never reached the prompt, so the rule was decided by hallucination.

Observed on a real resume before this change: the pipeline printed
"12 open source, 20 self projects" and the model then asserted "all 7 featured
projects are self-projects (single contributor, 0 forks each)" and scored
open_source 12/35 on that basis. `grep -c project_type transform.py` was 0.

Two separate defects, both fixed:

1. The selection step discarded the computed fields. generate_projects_json
   appended the model's own echoed dict, which omits project_type and
   contributor_count entirely and re-types stars, forks and commit counts from
   memory — so a hallucinated number would enter the scoring prompt as if it had
   been fetched. Extracted resolve_selected_projects(), which keeps the model's
   choice, ordering and stated rationale but takes every factual field from the
   fetched record, and drops a selection naming a repository that was never
   fetched. Note the two exception fallbacks were already doing this correctly;
   only the happy path was wrong.

2. convert_github_data_to_text omitted the fields even when present. Now emits
   project_type, contributor_count, and author/total commit counts, plus live_url
   — a working demo is the largest single modifier in the rubric (+10-20% on
   self_projects, and -2 to -3 per project without one), and it was being fetched
   and then dropped. Missing values render as "unknown"/"N/A" rather than
   vanishing, so a cache written before this change is still legible.

Measured effect, claude-opus-5, scoring call only, same cached extraction:

              open_source        total
  before      12 [12-12] ±0   74 [71-76] ±5    (6 runs)
  after       16 [16-16] ±0   82 [78-82] ±4    (5 runs)

±0 spread on both sides, so the +4 is a deterministic consequence of the model
receiving real data rather than run-to-run variance. The model's reasoning also
became checkable: it now writes "despite several repos being marked
multi-contributor, the candidate authored ~95-99% of commits in each" instead of
asserting single-contributor status it had no data for.

16 new tests: 8 covering serializer output, 8 covering resolution integrity
including hallucinated-fact rejection, invented-project rejection, order
preservation and non-mutation of the fetched records.
A clean `pip install -r requirements.txt` followed by `pytest tests/`
failed with ModuleNotFoundError: ho module named 'pytest' -- it only
worked in the maintainer's local venv because pytest was installed
there manually, outside the dependency manifest. Pinned to 9.1.1,
the version already verified against all 45 tests in this PR.
…exist

The module docstring pointed to test_bedrock_live.py, which was never
added to the repo.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants